feat: engagement gap to filter autonomous agent tool calls - #10
Merged
Conversation
Add min_activity_gap_seconds config option. When set, rapid tool calls (gap < threshold) don't accumulate as human engagement in work_since_break. Defaults to 0 (current behavior preserved).
Without it, end_session falls back to wall-clock duration, defeating the engagement gap feature entirely.
- Create wsb sentinel on first tool call so end_session never falls back to wall-clock for engagement-gap sessions - Clamp min_activity_gap_seconds below min_break_seconds - Fix README wording (at or above, not above)
- Hook only updates .activity on human engagement or break detection, not during autonomous sequences. This lets check_break give proper break credit when the agent runs while the human is away. - Coerce min_activity_gap_seconds via float() in Python (parity with parseInt in Node for values like "60.9"). - Clamp negative values to 0 in both runtimes.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds configurable min_activity_gap_seconds: parsed and clamped in session state, exposed by computeSessionState, used by hook logic to treat short gaps as autonomous activity (preserve human timestamp); docs updated and extensive tests added. Changes
Sequence Diagram(s)sequenceDiagram
participant Config as Configuration
participant State as computeSessionState
participant Hook as hook.sh
participant Sentinels as Sentinel Files (.activity, .work-since-break)
Config->>State: supply min_activity_gap_seconds (raw)
State->>State: normalize/coerce & clamp to [0, min_break_seconds-1]
State->>Hook: emit session state (includes min_activity_gap_seconds)
Hook->>Sentinels: read last activity timestamp, compute GAP
alt GAP < min_activity_gap_seconds
Note over Hook: treat as autonomous activity
Hook->>Sentinels: ensure .activity exists (do not update timestamp)
else GAP >= min_break_seconds
Note over Hook: intra-session break detected
Hook->>Sentinels: reset work-since-break, update .activity to NOW
else Normal engagement
Note over Hook: human engagement
Hook->>Sentinels: add GAP to work-since-break, update .activity to NOW
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
🧹 Nitpick comments (1)
tests/test_guard.mjs (1)
1654-1694: Add the non-integer coercion parity cases here too.This block covers integers, negatives, and upper-bound clamping, but it skips the
60.9/"60.9"/ invalid-string cases that the Python suite now exercises. Since cross-runtime parity is part of the feature, mirroring those cases here would make a JS-only regression much harder to miss.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_guard.mjs` around lines 1654 - 1694, Add parity tests in the same "Engagement gap (min_activity_gap_seconds)" describe block to cover non-integer coercion: add cases that pass min_activity_gap_seconds as 60.9 (float) and "60.9" (string) and assert that computeSessionState(config, now, 'Europe/London') returns the coerced integer (60) for both, and add a case with an invalid string (e.g., "not-a-number") and assert it falls back to the default/clamped value (0). Use the same helpers and fixtures as the other tests (parseYaml, SAMPLE_YAML, SAMPLE_CONFIG, fakeNow) and mirror the existing test patterns so these new cases sit alongside the integer/negative/clamping tests.
🤖 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.py`:
- Around line 659-663: The try/except around the coercion of raw_gap to
min_activity_gap currently only catches TypeError and ValueError but not
OverflowError (e.g., when raw_gap is "inf"/"-inf"); update the except clause for
the int(float(raw_gap)) conversion to also catch OverflowError so non-finite
floats fall back to the existing default (min_activity_gap = 0), keeping the
surrounding logic (clamping to valid range) unchanged; look for the conversion
site using the names raw_gap and min_activity_gap in guard/core.py to modify the
exception tuple.
In `@tests/test_guard.py`:
- Around line 1951-1957: The test captures subprocess.run into the variable
result but never verifies success; update the invocation around hook_path in
tests/test_guard.py so the test fails fast on hook errors by either passing
check=True to subprocess.run or immediately asserting result.returncode == 0
(include result.stdout/result.stderr in the assertion message for diagnostics);
ensure the result variable is used so the test stops if the hook subprocess
exits non‑zero before inspecting sentinel files.
- Line 1884: The test function test_check_writes_min_activity_gap_to_state_file
has an unused fixture parameter config_file; remove config_file from the
function signature so the test reads def
test_check_writes_min_activity_gap_to_state_file(self, session_state_path,
session_log_path, tmp_path): to eliminate the unused argument and reduce the
line length that is failing CI.
---
Nitpick comments:
In `@tests/test_guard.mjs`:
- Around line 1654-1694: Add parity tests in the same "Engagement gap
(min_activity_gap_seconds)" describe block to cover non-integer coercion: add
cases that pass min_activity_gap_seconds as 60.9 (float) and "60.9" (string) and
assert that computeSessionState(config, now, 'Europe/London') returns the
coerced integer (60) for both, and add a case with an invalid string (e.g.,
"not-a-number") and assert it falls back to the default/clamped value (0). Use
the same helpers and fixtures as the other tests (parseYaml, SAMPLE_YAML,
SAMPLE_CONFIG, fakeNow) and mirror the existing test patterns so these new cases
sit alongside the integer/negative/clamping tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 46b380b2-e16e-452b-a858-1a6926699e1a
📒 Files selected for processing (6)
README.mdguard/core.mjsguard/core.pyguard/hook.shtests/test_guard.mjstests/test_guard.py
- Catch OverflowError for int(float("inf")) in min_activity_gap coercion
- Handle legacy ISO-format activity sentinels in hook.sh (from touch_session)
- Assert hook subprocess returncode in all integration tests
- Remove unused config_file fixture parameter
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/test_guard.py (1)
1952-1959:⚠️ Potential issue | 🟠 MajorFail fast if
hook.shexits non-zero.These tests pre-seed sentinel files before the subprocess runs, so a broken
hook.shcan still leave the later assertions passing against stale data. Please assert success here, or usecheck=True, before reading any files back.🧪 Suggested fix
import subprocess - subprocess.run( + proc = subprocess.run( ["bash", str(hook_path)], input="{}", # stdin consumed by hook capture_output=True, text=True, env=env, ) + assert proc.returncode == 0, proc.stderr or proc.stdout🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_guard.py` around lines 1952 - 1959, The subprocess invocation running the hook in tests/test_guard.py should fail the test immediately if hook.sh exits non‑zero; update the call that currently uses subprocess.run([...], input="{}", capture_output=True, text=True, env=env) to either pass check=True to subprocess.run or capture the CompletedProcess and assert proc.returncode == 0 before any file reads; reference the invocation in tests/test_guard.py where the hook_path subprocess is executed to make the change so stale sentinel files cannot mask a failing hook.
🤖 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 2148-2175: The tests currently only assert that
min_activity_gap_seconds is less than min_break_seconds; change both assertions
to assert the exact clamp target: compute_session_state(...) should set
state["min_activity_gap_seconds"] == state["min_break_seconds"] - 1, so update
the asserts in test_threshold_clamped_below_min_break and
test_threshold_above_min_break_also_clamped to compare equality against
state["min_break_seconds"] - 1 (referencing the compute_session_state function
and the state dict keys "min_activity_gap_seconds" and "min_break_seconds").
---
Duplicate comments:
In `@tests/test_guard.py`:
- Around line 1952-1959: The subprocess invocation running the hook in
tests/test_guard.py should fail the test immediately if hook.sh exits non‑zero;
update the call that currently uses subprocess.run([...], input="{}",
capture_output=True, text=True, env=env) to either pass check=True to
subprocess.run or capture the CompletedProcess and assert proc.returncode == 0
before any file reads; reference the invocation in tests/test_guard.py where the
hook_path subprocess is executed to make the change so stale sentinel files
cannot mask a failing hook.
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
Closes #9. Adds
min_activity_gap_secondsconfig option to distinguish human engagement from autonomous agent tool calls in work time tracking.work_since_break. Only gaps at or above the threshold count as human engagement. Default0preserves current behavior..activityfile only updates on human engagement or break detection, not during autonomous sequences. This letscheck_breakgive proper break credit when the agent runs while the human is away.min_activity_gap_secondsis coerced to int (viafloat()in Python for parity withparseIntin Node), clamped to[0, min_break_seconds), and negatives are normalized to 0.blocked_periods.Config example
Test plan
Summary by CodeRabbit
New Features
Documentation
Tests