feat(loop-stall): reject the ways an autonomous arc returns control it should keep (BRO-1700)#90
Merged
Merged
Conversation
… it should keep — BRO-1700 The #1-ranked finding of the July-1 session-leverage audit and the Tier-1 autonomy-spine flagship. The leverage loop (v0.30.0) MEASURES this as m1 (~180 continue-nudges / 64 sessions); this ships the ACTUATOR the m1 setpoint already names — the machine-checkable core, keyed on one session-scoped arc file so the pieces compose instead of bolting on separately. - autonomous-arc.sh — shared arc-state substrate (set|next|complete|status|active|get|bump); the complete-sentinel (active:false) is the false-positive guard the hooks rely on. - autonomous-posture-hook.sh (UserPromptSubmit) — self-bootstraps the arc on /autonomous (no skill change needed), re-stamps sticky posture each turn (disturbance #5). - arc-continuation-hook.sh (Stop) — the m1 core. Active arc + no-op-terminal final turn (empty/"No response requested") + no tool calls → {"decision":"block"} so the harness continues. Guards: complete-sentinel + reconcile_count<2 runaway cap. Never blocks a productive turn (disturbances #3/#4). - limit-stall-resume-hook.sh (Stop) — LOG-ONLY calibration probe; live auto-resume deferred on two empirical unknowns, so it captures the real rate-limit string without ever acting (disturbance #1). - tests/loop-stall-hooks.test.sh — 30 assertions (predicate + guards + wiring), all green. - doctor §24 — loop-stall wiring + arc-file health. Snippet wires all 3 hooks. Deferred: the sequencing-question ban (#2) is reasoning-enforced → skills-monorepo SKILL.md, not a hook (a hard deny would false-block the 3 legitimate mid-arc pauses). Dogfooded against this session's real 2005-line transcript: correctly did NOT block a live productive turn. shellcheck clean, JSON valid, doctor exit 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…edicate, auto-release, consecutive cap, flock, staleness Cross-model adversarial review (3 diverse lenses) FAILED the first cut (5/10, 2 high). Every reproduced defect is now closed: - HIGH transcript race: CC flushes the final assistant entry ~125ms AFTER Stop fires (BRO-1616). Added a bounded drain-retry that waits for an entry written at/after hook start (via the entry timestamp) before classifying — the mitigation Genesis already shipped, which this hook had skipped. - HIGH over-broad no-op predicate: ok/okay/understood/got it/done/… were treated as no-ops → the hook force-continued the agent against a user's mid-arc pause. Narrowed to empty text + a SEARCH for the literal "No response requested" sentinel (+ bare "acknowledged"); dropped every conversational-acknowledgement word. - reconcile_count was a LIFETIME cap (disarmed after 2 total stalls) and was reset by `set` (re-typing /autonomous re-armed it). Now: reset on any productive/substantive turn (bounds CONSECUTIVE stalls) and posture only `set`s when no arc is active. - No auto-release: a finished arc persisted and fought later unrelated turns. Now a completion phrase calls `complete`; plus an 8h staleness backstop in `active`. - Non-atomic save + shared .tmp → concurrent-write corruption. Now mkstemp+os.replace under an flock (tested: 8 parallel bumps → no lost updates). - Dropped limit-stall-resume-hook.sh entirely: the review showed the raw-tail probe self-pollutes (any session discussing limits logs itself), defeating calibration. Disturbance #1 deferred until it can gate on a real terminal-error transcript shape. Tests 30→39 (adds drain-race, auto-release, consecutive-reset, staleness, flock, set-if-absent). shellcheck clean, doctor exit 0, re-dogfooded on the real transcript. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…an't prematurely release the arc Self-caught while addressing P20 round-1: the auto-release used COMPLETE_RE.search(), so 'the first task is complete, moving on' would match mid-arc and deactivate a live arc (silent under-protection — worse than a missed nudge). Anchored (^…$) so only a completion-dominant final message releases; a real 'All milestones shipped.' still does. Test 39→40. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…in, lifetime cap, stop_hook_active Round-2 adversarial review found the round-1 fixes traded one edge case for another (knob-tuning). Root cause: the two failure modes are asymmetric — a false positive (force-continue a legit stop) fights the user; a false negative (miss a stall) costs one manual nudge. Redesigned bias-to-safety, robust-by-construction: - HIGH trivial-tool-loop defeated the cap + stop_hook_active was parsed-but-unused: added a LIFETIME block cap (total_blocks<5, never reset) via an atomic `try-block` verb, AND the consecutive counter now resets on a productive turn only when NOT in a hook-continuation chain (stop_hook_active) — so a block→trivial-tool→block loop terminates. - SKEW race for fast (<2s) loops: replaced the 2s timestamp window with an IDENTITY-based drain (wait for an entry whose signature differs from the one at hook start) — no timestamp dependence, no misfire on close-together turns. - Unanchored sentinel search force-continued substantive turns that merely mention "no response requested": anchored to the sentinel-as-whole-message. Dropped the conversational-acknowledgement set entirely — block ONLY empty + exact sentinel. - Over-tight anchored COMPLETE missed natural completions: broadened (leading- anchored) to catch "All done!", "Everything's shipped and merged.", etc., while still refusing a mid-arc "the first task is complete, moving on". Tests 40→43 (adds lifetime cap, in-chain suppression, substantive-sentinel-mention, identity fast-loop). shellcheck clean, re-dogfooded on the real transcript. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ntry (+ widen tail read)
Round-3 verification scored 8/10 and confirmed all round-2 findings FIXED, but caught
one new HIGH in the harmful direction: Claude Code writes each extended-thinking block
as its OWN assistant JSONL entry (content=[{type:thinking}]) — 245 such entries in this
session's real transcript. The classifier dropped thinking blocks, so a thinking-only
entry parsed as empty text → force-continued a legitimate stop; and the identity-drain
could break on that intermediate thinking entry before the real text yield landed.
Fix (robust-by-construction, both directions):
- parse_entry() now detects thinking/redacted_thinking.
- the drain waits for a REAL yield (text or tool_use), skipping thinking-only
intermediate entries.
- final classify: a thinking-only entry → SKIP, never the empty→BLOCK branch.
- widened the tail read 256KB→1MB so a large thinking block can't truncate the final
entry (round-3 LOW).
Validated empirically: dogfooded against a REAL thinking-only entry from the live
transcript → NOT blocked, while a real 'No response requested.' still blocks (sanity).
Tests 43→46. shellcheck clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a new "loop-stall rejection" hook system: scripts for per-session autonomous arc state (autonomous-arc.sh), a posture hook and a Stop/continuation hook with bias-to-safety logic, doctor.sh diagnostics, settings.json.snippet wiring, a shell test suite, and a 0.32.0 changelog/VERSION bump. ChangesLoop-stall rejection hooks (BRO-1700)
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant PostureHook as autonomous-posture-hook.sh
participant ContinuationHook as arc-continuation-hook.sh
participant ArcSh as autonomous-arc.sh
participant ArcFile
User->>PostureHook: prompt containing "/autonomous"
PostureHook->>ArcSh: active?
ArcSh->>ArcFile: read state
PostureHook->>ArcSh: set (if not active)
ArcSh->>ArcFile: write active arc
PostureHook-->>User: sticky posture message
User->>ContinuationHook: Stop event (transcript_path, session_id)
ContinuationHook->>ArcSh: active?
ContinuationHook->>ContinuationHook: classify transcript tail
alt PRODUCTIVE
ContinuationHook->>ArcSh: reset
else COMPLETE
ContinuationHook->>ArcSh: complete
else BLOCK
ContinuationHook->>ArcSh: try-block
ArcSh-->>ContinuationHook: BLOCK or CAP
ContinuationHook-->>User: decision block (reason)
end
Compact metadata
Poem 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
🧹 Nitpick comments (1)
tests/loop-stall-hooks.test.sh (1)
164-169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
doctor.shSection 24 itself.The wiring tests only check that
autonomous-posture-hook.sh/arc-continuation-hook.shappear in thesettings.json.snippettemplate. There's no test that exercisesdoctor.sh's new Section 24 (theok/gaplogic against an actualsettings.json), so a regression there (e.g., wrong hook name string, brokengrep, mis-alignedLOOPSTALL_HOOKS/LOOPSTALL_LABELSarrays) wouldn't be caught by this suite.Consider adding a small case that seeds a fixture
.claude/settings.json(with and without the hooks present) and assertsdoctor.sh's ok/gap output for Section 24.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/loop-stall-hooks.test.sh` around lines 164 - 169, Add test coverage for doctor.sh’s Section 24 by exercising the actual ok/gap logic instead of only checking settings.json.snippet wiring. Update tests/loop-stall-hooks.test.sh to seed a fixture .claude/settings.json with and without the expected hooks, then run doctor.sh and assert Section 24 reports ok when both hook names are present and gap when they are missing. Reference doctor.sh, the Section 24 loop, and the LOOPSTALL_HOOKS/LOOPSTALL_LABELS checks so regressions in hook names, grep matching, or array alignment are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/arc-continuation-hook.sh`:
- Around line 59-62: The completion detector in COMPLETE_RE is too permissive
and can falsely mark an arc as finished from partial phrases like “task
complete; continuing…”. Tighten the regex in arc-continuation-hook.sh so it only
accepts explicit whole-message completion signals, anchoring the pattern to the
full status text and removing broad generic matches like “task complete” unless
they are clearly standalone and unambiguous. Use COMPLETE_RE as the target
symbol when updating the detection logic.
In `@scripts/autonomous-posture-hook.sh`:
- Around line 42-45: The autonomous bootstrap check is too permissive and
matches `/autonomous` anywhere in the prompt, including quoted prose. Update the
prompt detection in the autonomous-posture hook so it only fires when the
trimmed prompt starts with the slash command, using the existing PROMPT handling
around the shell `grep` check and the `ARC_HELPER` active/set flow.
---
Nitpick comments:
In `@tests/loop-stall-hooks.test.sh`:
- Around line 164-169: Add test coverage for doctor.sh’s Section 24 by
exercising the actual ok/gap logic instead of only checking
settings.json.snippet wiring. Update tests/loop-stall-hooks.test.sh to seed a
fixture .claude/settings.json with and without the expected hooks, then run
doctor.sh and assert Section 24 reports ok when both hook names are present and
gap when they are missing. Reference doctor.sh, the Section 24 loop, and the
LOOPSTALL_HOOKS/LOOPSTALL_LABELS checks so regressions in hook names, grep
matching, or array alignment are caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 81a868e2-39dd-4de4-ae18-b8ba01827947
📒 Files selected for processing (8)
CHANGELOG.mdVERSIONassets/templates/settings.json.snippetscripts/arc-continuation-hook.shscripts/autonomous-arc.shscripts/autonomous-posture-hook.shscripts/doctor.shtests/loop-stall-hooks.test.sh
…trap + whole-message auto-release Two Major findings, both in the false-positive (harmful) direction the design guards against: - autonomous-posture-hook.sh: "please don't use /autonomous" matched mid-prose and bootstrapped an arc. Now leading-anchored (^\s*/autonomous) — only a real invocation at the prompt start creates an arc. - arc-continuation-hook.sh: COMPLETE_RE was leading-anchored, so "Task complete; continuing with the next slice" auto-released and DISABLED loop-stall protection for the rest of the arc. Now fully anchored (^…$, whole message modulo trailing punct): only a message that IS a completion releases; multi-clause/keep-going messages keep protection active. Consistent with bias-to-safety (under-release costs a nudge; the explicit `complete` call + staleness remain the primary release). Tests 46→49 (leading-anchored bootstrap; whole-message vs multi-clause completion). shellcheck clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
What
The Tier-1 autonomy-spine flagship and the #1-ranked finding of the July-1 session-leverage audit: an autonomous arc returns control it should keep (~180 continue-nudges across 64 sessions; 4h13m + 5h29m of dead wall-clock on rate-limit kills; 24 "No response requested." mid-arc stops;
/autonomousmanually re-stamped 143×). The leverage loop (v0.30.0) measures this asm1; this PR ships the actuator them1setpoint already names.Keyed on one session-scoped arc file (
~/.config/broomva/autonomous/<session_id>.arc) so the pieces compose:autonomous-arc.sh— shared arc-state substrate (set|next|complete|status|active|get|bump|reset|try-block).flock+mkstemp+os.replacewrites (no concurrent-corruption);try-blockenforces a consecutive and a lifetime cap; 8h staleness backstop.autonomous-posture-hook.sh(UserPromptSubmit) — self-bootstraps the arc on/autonomous(only if none active), re-stamps sticky posture each turn so the stance doesn't decay (disturbance feat(bstack): doctor + ROSTER updates for P12 #5).arc-continuation-hook.sh(Stop) — them1core. Blocks only unambiguous no-ops (empty terminal, or the exactNo response requested.sentinel) so the harness continues the arc (disturbances feat(bstack): doctor extends to P11 (Empirical Feedback Loop) #3/feat(bstack): rewrite SKILL.md per skill-creator + scaffold/wire/doctor pipeline #4).doctor§24 + snippet wiring +VERSION0.32.0.Design principle: bias-to-safety
The two failure modes are asymmetric — a false positive (force-continue a legitimate stop) fights the user and burns tokens; a false negative (miss a stall) costs one manual nudge (the status quo). So the hook is robust-by-construction: block only on unambiguous no-ops, accept every false negative, with a hard lifetime ceiling.
P20 cross-model adversarial gate — 3 rounds (verdict logged)
This PR was hardened through the mandatory P20 gate; each round caught real, reproduced defects:
stop_hook_activeparsed-but-unused; 2s-SKEW race for fast loops; unanchored sentinel force-continuing substantive mentions; over-tightCOMPLETEThe resolution was a redesign around the asymmetry (round 2→3), not knob-tuning. Fixes: identity-based drain (skips thinking intermediates, waits for a real yield); bias-to-safety predicate; consecutive + lifetime caps with
stop_hook_active-gated reset; anchored auto-release.Validation
tests/loop-stall-hooks.test.sh) — predicate, drain race, thinking-only, both caps, flock concurrency (8 parallel bumps → no lost updates), staleness, set-if-absent, wiring.doctorexit 0.No response requested.still blocks; a live productive session is not blocked.Deferred (tracked)
bstack doctor— verify primitive contract #2) — reasoning-enforced →/autonomousSKILL.md prose (skills-monorepo PR), not a hook.BRO-1700. Part of the Tier-1 autonomy-spine batch (with BRO-1701 background-visibility, BRO-1702 auth pre-flight).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation