fix: stale session-state guard + SID fallback for _notify_once - #13
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughGuard hook now reads Changes
Sequence DiagramsequenceDiagram
participant Hook as Hook Script
participant State as Session-State JSON
participant Env as Environment
participant FS as Filesystem (Markers)
participant Out as Output/Exit
Hook->>State: read max_epoch, start_epoch, session_id, min_break_seconds
Hook->>Env: read HUMAN_GUARD_SESSION_ID
Env-->>Hook: env SID or empty
Hook->>Hook: derive SID = env SID || state.session_id
Hook->>Hook: compute STALE_LIMIT = max_epoch + min_break_seconds
Hook->>Hook: compare NOW >= STALE_LIMIT?
alt Stale (NOW ≥ STALE_LIMIT)
Hook->>Out: exit 0 (suppress notifications)
else Fresh (NOW < STALE_LIMIT)
Hook->>FS: check marker .notified.session_limit.<SID>
FS-->>Hook: marker exists?
alt no marker
Hook->>Out: emit session_limit notification
Hook->>FS: write .notified.session_limit.<SID>
else marker exists
Hook->>Out: exit 0 (one-shot suppression)
end
end
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)
📝 Coding Plan
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: 1
🧹 Nitpick comments (1)
guard/hook.sh (1)
103-106: Make the stale guard independent ofstart_epoch.Lines 103-106 already derive staleness from
MAX_EPOCH + min_break_seconds, soSTART_EPOCH > 0only disables the fix for legacy or partially-written state files that omitstart_epoch.♻️ Minimal change
-if [ "$NOW" -ge "$MAX_EPOCH" ] && [ "$START_EPOCH" -gt 0 ] 2>/dev/null; then +if [ "$NOW" -ge "$MAX_EPOCH" ]; then MIN_BREAK_STALE=$(jq -r '.min_break_seconds // 900' "$STATE" 2>/dev/null || echo 900) STALE_LIMIT=$(( MAX_EPOCH + MIN_BREAK_STALE )) [ "$NOW" -ge "$STALE_LIMIT" ] && exit 0 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@guard/hook.sh` around lines 103 - 106, The staleness check currently gates on START_EPOCH > 0 which prevents the MAX_EPOCH + min_break_seconds logic from running for legacy/partial state files; change the conditional so it does not depend on START_EPOCH: evaluate if [ "$NOW" -ge "$MAX_EPOCH" ] (preserving the 2>/dev/null) and then compute MIN_BREAK_STALE from the STATE via jq and set STALE_LIMIT=$(( MAX_EPOCH + MIN_BREAK_STALE )), finally exit 0 if [ "$NOW" -ge "$STALE_LIMIT" ]; keep the same variable names (NOW, MAX_EPOCH, START_EPOCH, MIN_BREAK_STALE, STALE_LIMIT, STATE) so the logic is independent of START_EPOCH but still supports existing legacy handling elsewhere.
🤖 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/hook.sh`:
- Around line 101-107: The stale-session guard currently only checks
START_EPOCH/MAX_EPOCH; also read and validate the stored end_allowed_epoch from
the state (e.g. END_ALLOWED_EPOCH=$(jq -r '.end_allowed_epoch // 0' "$STATE"
2>/dev/null || echo 0)) and treat the state as stale if that epoch is older than
NOW (or older than MAX_EPOCH as appropriate); add a check alongside the existing
START_EPOCH check so if END_ALLOWED_EPOCH is stale the script exits 0 just like
the existing stale-case (update variables END_ALLOWED_EPOCH and include it in
the stale-limit logic or a separate comparison mirroring the START_EPOCH logic).
---
Nitpick comments:
In `@guard/hook.sh`:
- Around line 103-106: The staleness check currently gates on START_EPOCH > 0
which prevents the MAX_EPOCH + min_break_seconds logic from running for
legacy/partial state files; change the conditional so it does not depend on
START_EPOCH: evaluate if [ "$NOW" -ge "$MAX_EPOCH" ] (preserving the
2>/dev/null) and then compute MIN_BREAK_STALE from the STATE via jq and set
STALE_LIMIT=$(( MAX_EPOCH + MIN_BREAK_STALE )), finally exit 0 if [ "$NOW" -ge
"$STALE_LIMIT" ]; keep the same variable names (NOW, MAX_EPOCH, START_EPOCH,
MIN_BREAK_STALE, STALE_LIMIT, STATE) so the logic is independent of START_EPOCH
but still supports existing legacy handling elsewhere.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed2a59b0-7904-4b64-a1ef-b396a88e6c94
📒 Files selected for processing (2)
guard/hook.shtests/test_guard.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
guard/hook.sh (1)
74-78:⚠️ Potential issue | 🟠 MajorStale-state predicate is too narrow for
end_allowed_epoch-expired state.This guard only treats state as stale when
NOWpassesMAX_EPOCH + min_break. IfEND_EPOCHis already stale butMAX_EPOCHis still in the future, the script can still hit outside-hours/blocked checks and block a new unmanaged session.Please either include
END_EPOCHfreshness in stale detection or enforce/document an invariant thatMAX_EPOCH <= END_EPOCH.Proposed adjustment (conceptual)
-if [ "$NOW" -ge "$MAX_EPOCH" ] && [ "$START_EPOCH" -gt 0 ] 2>/dev/null; then +if [ "$START_EPOCH" -gt 0 ] 2>/dev/null; then MIN_BREAK_STALE=$(jq -r '.min_break_seconds // 900' "$STATE" 2>/dev/null || echo 900) - STALE_LIMIT=$(( MAX_EPOCH + MIN_BREAK_STALE )) - [ "$NOW" -ge "$STALE_LIMIT" ] && exit 0 + STALE_LIMIT_MAX=$(( MAX_EPOCH + MIN_BREAK_STALE )) + STALE_BY_MAX=0 + STALE_BY_END=0 + + [ "$NOW" -ge "$STALE_LIMIT_MAX" ] && STALE_BY_MAX=1 + if [ "$END_EPOCH" -gt 0 ] 2>/dev/null; then + STALE_LIMIT_END=$(( END_EPOCH + MIN_BREAK_STALE )) + [ "$NOW" -ge "$STALE_LIMIT_END" ] && STALE_BY_END=1 + fi + + { [ "$STALE_BY_MAX" -eq 1 ] || [ "$STALE_BY_END" -eq 1 ]; } && exit 0 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@guard/hook.sh` around lines 74 - 78, The stale-state check currently only considers MAX_EPOCH + min_break (variables MAX_EPOCH, MIN_BREAK_STALE, STALE_LIMIT) and ignores END_EPOCH, so states whose END_EPOCH has already passed but MAX_EPOCH is still in the future remain treated as active; update the predicate in the block containing NOW, MAX_EPOCH, START_EPOCH and STATE to also read END_EPOCH from STATE (via jq) and treat the state as stale if NOW >= END_EPOCH OR if NOW >= MAX_EPOCH + MIN_BREAK_STALE (i.e., include END_EPOCH freshness in the stale detection), or alternatively enforce/document an invariant that MAX_EPOCH <= END_EPOCH and validate that invariant early and fail fast.
🤖 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/hook.sh`:
- Around line 74-78: The stale-state check currently only considers MAX_EPOCH +
min_break (variables MAX_EPOCH, MIN_BREAK_STALE, STALE_LIMIT) and ignores
END_EPOCH, so states whose END_EPOCH has already passed but MAX_EPOCH is still
in the future remain treated as active; update the predicate in the block
containing NOW, MAX_EPOCH, START_EPOCH and STATE to also read END_EPOCH from
STATE (via jq) and treat the state as stale if NOW >= END_EPOCH OR if NOW >=
MAX_EPOCH + MIN_BREAK_STALE (i.e., include END_EPOCH freshness in the stale
detection), or alternatively enforce/document an invariant that MAX_EPOCH <=
END_EPOCH and validate that invariant early and fail fast.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6863261f-b9e3-4f83-9625-9bf71dda42f6
📒 Files selected for processing (2)
guard/hook.shtests/test_guard.py
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Two bugs caused "Llevas 2 horas y media" to spam every tool use when Claude was launched outside the wrapper (IDE, command claude): 1. _notify_once had no dedup when HUMAN_GUARD_SESSION_ID was unset — now falls back to session_id from session-state.json. 2. No staleness check on session-state.json — old epochs from a previous session triggered session_limit immediately. Now skips informational notifications when past max_epoch + min_break.
teseo
force-pushed
the
fix/stale-session-state-and-sid-fallback
branch
from
March 16, 2026 22:31
a94f1b7 to
a995523
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_guard.py (1)
2422-2489: Consider extracting shared hook test utilities to a module-level fixture/helper.
hook_envand_make_statehere substantially overlap with the earlier hook integration section, increasing maintenance overhead when hook contract changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_guard.py` around lines 2422 - 2489, The hook test utilities (the pytest fixture hook_env and helper function _make_state) are duplicated and should be extracted to a shared module-level helper so all hook tests reuse a single implementation; refactor by moving the hook_env fixture and _make_state function into a common test helper (e.g., a top-level conftest/shared test utilities module), import or reference them from the current test file, and update any tests to call the shared hook_env fixture and _make_state helper instead of their local copies to ensure a single source of truth for hook behavior and state construction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_guard.py`:
- Around line 2612-2631: Add an assertion to ensure the
test_stale_blocked_period_does_not_block path also enforces the silent output
contract: after calling hook_env["run_hook"](state, sid="active-sid") and
asserting rc == 0, assert that stdout (or stdout.strip()) is empty (no
notification text), matching the same silent-output checks used in the other
tests around Line 2506/2610; reference the stdout and rc variables and the
run_hook call to locate where to insert this assertion.
- Around line 2550-2591: The dedup tests call hook_env["run_hook"](state,
sid=...) but ignore the return code (they unpack as stdout, _), which lets
non-zero exits pass; update both
test_no_sid_notification_fires_once_not_every_call and
test_with_sid_env_still_works to capture the full return (e.g., stdout1, rc1 =
hook_env["run_hook"](…)) and add assertions that rc1 == 0 (and similarly for the
second call rc2 == 0) before asserting on stdout contents; reference the
run_hook calls and stdout1/stdout2 variables in the two tests when adding these
rc assertions.
---
Nitpick comments:
In `@tests/test_guard.py`:
- Around line 2422-2489: The hook test utilities (the pytest fixture hook_env
and helper function _make_state) are duplicated and should be extracted to a
shared module-level helper so all hook tests reuse a single implementation;
refactor by moving the hook_env fixture and _make_state function into a common
test helper (e.g., a top-level conftest/shared test utilities module), import or
reference them from the current test file, and update any tests to call the
shared hook_env fixture and _make_state helper instead of their local copies to
ensure a single source of truth for hook behavior and state construction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 728403a0-5de7-4e52-a786-7d2885da8a0f
📒 Files selected for processing (2)
guard/hook.shtests/test_guard.py
🚧 Files skipped from review as they are similar to previous changes (1)
- guard/hook.sh
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
Test plan
Summary by CodeRabbit
New Features
Tests
Documentation