fix: cumulative break enforcement + last_activity tracking - #6
Merged
Conversation
- Break only required after max_continuous_minutes (150min) of cumulative work - Walk backward through sessions, accumulate work time, stop at real breaks - Add touch_session sentinel file for accurate last_activity tracking - hook.sh writes local time to sentinel on each tool use - end_session reads sentinel into last_activity field - Align check_break/checkBreak signatures (now as 3rd param in both) - Add _parse_naive() helper for Z-suffix safety net
📝 WalkthroughWalkthroughSentinel-based session activity tracking added: hooks write per-session Changes
Sequence DiagramsequenceDiagram
participant Hook as Hook Script (hook.sh)
participant Sentinel as Sentinel File (.activity.{id})
participant Core as Guard Core (core.mjs / core.py)
participant Config as Config/State Files
Hook->>Sentinel: touchSession() / write timestamp
activate Sentinel
Hook->>Core: endSession(sessionId)
activate Core
Core->>Sentinel: read .activity.{id}
Sentinel-->>Core: return last_activity
Core->>Sentinel: delete sentinel file
deactivate Sentinel
Core->>Core: set s.last_activity or fallback to end_time
deactivate Core
Core->>Config: read historical sessions
activate Core
loop iterate sessions
Core->>Core: use last_activity for elapsed calc
Core->>Core: accumulate work until gap >= min_break_minutes
end
Core->>Core: compare cumulative work vs max_continuous_minutes
deactivate Core
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@guard/core.mjs`:
- Around line 482-495: The loop currently skips trivial sessions with "if
(durationMin < minBreakMinutes) continue;" but doesn't update prevSessionStart,
leaving a stale prevSessionStart that can create a false break boundary; change
the logic so that before continuing you update prevSessionStart to the current
session's start (e.g., compute sessionStart = new Date(s.start) and, if valid,
set prevSessionStart = sessionStart) so subsequent gapMin calculations use the
latest session boundary (ensure you still validate the date the same way you do
for sessionInteraction).
In `@guard/core.py`:
- Around line 405-408: Replace the bare try/except/pass around reading
activity_file with contextlib.suppress(Exception): wrap the
activity_file.read_text().strip() assignment (the block that sets
s["last_activity"]) in a contextlib.suppress(Exception) context manager so
exceptions are suppressed explicitly; add an import for contextlib if not
already present and ensure the suppressed block only contains the read/assign
operation referencing activity_file and s["last_activity"].
- Around line 508-525: The current early "continue" on short sessions (when
duration_min < min_break_minutes) skips computing session_interaction, causing
prev_session_start vs session_interaction gap checks to see an inflated gap and
falsely stop accumulation; to fix, compute session_interaction (using
s.get("last_activity") and _parse_naive with the same fallback to end) before
deciding to skip adding the session's duration, then if duration_min is short,
skip adding its duration but still use its session_interaction for the gap
calculation (i.e., move the duration_min check after the session_interaction
logic or compute session_interaction prior to the continue), keeping references
to s, duration_min, min_break_minutes, session_interaction, prev_session_start
and _parse_naive unchanged.
In `@guard/hook.sh`:
- Around line 31-34: The sentinel write may fail if the NOTIFY_DIR doesn't
exist; before writing to "$NOTIFY_DIR/.activity.$SID" (the file created when SID
is set), ensure the directory exists by creating it (e.g., mkdir -p
"$NOTIFY_DIR") and handle potential errors/permissions, then perform the date
write to ".activity.$SID"; update the block that checks SID in hook.sh to create
the directory if missing prior to redirecting the date output.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
guard/core.mjsguard/core.pyguard/hook.shpackage.jsonpyproject.tomltests/test_guard.mjstests/test_guard.py
…entinel dir - Move short-session skip after gap detection so trivial sessions still interrupt gap chaining and update prev_session_start (Py + JS) - Add mkdir -p for sentinel dir in hook.sh before writing activity file
There was a problem hiding this comment.
♻️ Duplicate comments (3)
guard/core.py (2)
464-545: Cumulative break logic is sound and the trivial-session gap-chaining fix is correctly applied.The prior issue — where an early
continueon short sessions leftprev_session_startstale and could create a false break boundary — is resolved:session_interactionis now computed before the gap check and before the trivial-skip branch, andprev_session_startis updated before thecontinueon Line 524. The accumulation,last_interactioncapture, and final elapsed comparison are all correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@guard/core.py` around lines 464 - 545, check_break's previous bug left prev_session_start stale causing false break detection; ensure in check_break you always compute session_interaction (parse s["last_activity"] or fallback to end) before any gap check, and when skipping trivial sessions (duration_min < min_break_minutes) update prev_session_start = start before continuing; also ensure last_interaction is set the first time you add to cumulative_work so elapsed uses the correct timestamp.
395-413: LGTM — sentinel read and fallback are correct.The prior SIM105 violation is resolved. The two-step pattern (suppress read, suppress unlink, then fall back to
end_time) correctly handles all failure paths: file missing, read error, and delete error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@guard/core.py` around lines 395 - 413, The review confirms no changes are needed: the end_session function correctly reads the sentinel activity file (activity_file = GUARD_DIR / f".activity.{session_id}") with exceptions suppressed and falls back to setting s["last_activity"] = s["end_time"], and it safely unlinks the file; leave end_session, the contextlib.suppress read/unlink pattern, and the subsequent fallback as-is.guard/core.mjs (1)
448-518:checkBreakcorrectly mirrors the Python cumulative logic with the trivial-session fix applied.
sessionInteractionis computed before the gap check and before the trivial-skip branch;prevSessionStartis updated on Line 497 before thecontinue, so gap chaining is not broken by short sessions.lastInteractionis captured only from the first (most recent) non-trivial session. Fully consistent withcore.py.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@guard/core.mjs` around lines 448 - 518, The checkBreak implementation is correct and needs no functional changes: sessionInteraction is set before the gap check, trivial sessions update prevSessionStart before the continue so they don't break gap chaining, and lastInteraction is only set from the first non-trivial session; leave the checkBreak function (and variables sessionInteraction, prevSessionStart, lastInteraction, cumulativeWork) as-is.
🧹 Nitpick comments (1)
guard/core.py (1)
416-424:log_pathparameter is accepted but never used.The sentinel path is always derived from the global
GUARD_DIR, not fromlog_path. The parameter exists purely for API symmetry withstart_session/end_session, which is understandable, but callers reading the signature may reasonably expect the log path to influence where the sentinel is written.Consider either removing the parameter (breaking change if callers already pass it) or documenting that it is intentionally ignored.
♻️ Proposed nit
-def touch_session(log_path, session_id): - """Write last_activity sentinel file for an active session. - - Uses a separate file to avoid racing with session-log.json writes. - The sentinel is read by end_session when the session closes. - """ +def touch_session(log_path, session_id): # log_path unused; sentinel always in GUARD_DIR + """Write last_activity sentinel file for an active session. + + The sentinel is stored in GUARD_DIR (not derived from log_path) to avoid + racing with session-log.json writes. log_path is accepted for API symmetry. + """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@guard/core.py` around lines 416 - 424, The touch_session function accepts a log_path parameter but never uses it—sentinel file path is built from GUARD_DIR; either remove the unused log_path parameter from touch_session's signature (and update all callers) or make the function use the provided log_path to derive the activity file instead of GUARD_DIR (e.g., use log_path / f".activity.{session_id}"), and update the docstring to state the chosen behavior; locate the symbol touch_session and the global GUARD_DIR in the diff to apply the change consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@guard/core.mjs`:
- Around line 448-518: The checkBreak implementation is correct and needs no
functional changes: sessionInteraction is set before the gap check, trivial
sessions update prevSessionStart before the continue so they don't break gap
chaining, and lastInteraction is only set from the first non-trivial session;
leave the checkBreak function (and variables sessionInteraction,
prevSessionStart, lastInteraction, cumulativeWork) as-is.
In `@guard/core.py`:
- Around line 464-545: check_break's previous bug left prev_session_start stale
causing false break detection; ensure in check_break you always compute
session_interaction (parse s["last_activity"] or fallback to end) before any gap
check, and when skipping trivial sessions (duration_min < min_break_minutes)
update prev_session_start = start before continuing; also ensure
last_interaction is set the first time you add to cumulative_work so elapsed
uses the correct timestamp.
- Around line 395-413: The review confirms no changes are needed: the
end_session function correctly reads the sentinel activity file (activity_file =
GUARD_DIR / f".activity.{session_id}") with exceptions suppressed and falls back
to setting s["last_activity"] = s["end_time"], and it safely unlinks the file;
leave end_session, the contextlib.suppress read/unlink pattern, and the
subsequent fallback as-is.
---
Nitpick comments:
In `@guard/core.py`:
- Around line 416-424: The touch_session function accepts a log_path parameter
but never uses it—sentinel file path is built from GUARD_DIR; either remove the
unused log_path parameter from touch_session's signature (and update all
callers) or make the function use the provided log_path to derive the activity
file instead of GUARD_DIR (e.g., use log_path / f".activity.{session_id}"), and
update the docstring to state the chosen behavior; locate the symbol
touch_session and the global GUARD_DIR in the diff to apply the change
consistently.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
guard/core.mjsguard/core.pyguard/hook.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- guard/hook.sh
This was referenced Mar 11, 2026
This was referenced Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
max_continuous_minutes(default 150min) of total work across sessions without sufficient breaks between themtouch_sessionsentinel file mechanism for accuratelast_activitytracking (written by hook.sh on each tool use, read byend_session)check_break/checkBreaksignatures across Python and JS (parameter order:log_path, min_break_minutes, now, max_continuous_minutes)_parse_naive()helper in Python for Z-suffix timestamp safety netTest plan
Summary by CodeRabbit
New Features
Improvements
Chores