Tool-only auto-pause should nudge, not hard-stop, on turn count alone - #401
Closed
TheGreatAxios wants to merge 9 commits into
Closed
Tool-only auto-pause should nudge, not hard-stop, on turn count alone#401TheGreatAxios wants to merge 9 commits into
TheGreatAxios wants to merge 9 commits into
Conversation
…progress signal A Grok session hard-paused at 10 turns while making real progress through Linear lookups and code reads, because the pause fired on any tool-only turn count rather than actual thrash. Forensics over ~/.corbits/projects session traces (54 sessions with tool-only runs) found healthy streaks topping out at 13 turns and zero sessions repeating an identical tool-call fingerprint 3+ times in a row. The soft wrap-up nudge now fires at a shared 25-turn threshold for every model family (still just a check-in, never a stop). The hard pause now requires the tool calls to actually repeat identically 4 turns in a row (fingerprintToolCalls, the same helper SubAgentDirector already uses), independent of overall streak length. Grok drops its miscalibrated 6/10 tool-only pair and shares the default; its shorter sub-agent stall timeout and finish-bias residual are untouched.
…shared period-detection helper Lifted the shortest-period-that-repeats-enough search out of detectRepetition into src/util/period-detection.ts so tool-call fingerprints can reuse the same detection shape instead of a hand-rolled consecutive-identical check. stall-watchdog's detectRepetition now delegates to it; behavior is unchanged, covered by its existing test suite.
The old identicalToolFingerprintStreak only compared each turn to the one immediately before it, so an alternating A,B tool-call pattern never triggered the hard pause at any length (critique proved this over 200 turns), while 4 truly identical calls in a row still false-positived on legitimate polling (rerunning a flaky test, checking a build). detectToolFingerprintThrash runs exact-period detection over a rolling fingerprint history instead, catching A,A,A..., A,B,A,B..., and A,B,C,A,B,C... uniformly. Identical-consecutive (period 1) needs 5 repeats to tolerate legitimate short polling; any longer cycle needs only 3, since there's no legitimate reason to repeat a fixed rotation of different tool calls. Added scripts/tool-fingerprint-forensics.ts to re-derive these thresholds against real local session traces: 328 sessions / 559 tool-only runs show zero repeating cycles of any period 1-8 at all, so both floors sit well above the measured healthy ceiling. The period-1 floor of 5 is inferred headroom for the polling case (not measured — the dataset has no repeats to calibrate against), chosen only to clear the previously false-positived value of 4.
ChatDirector now keeps a capped rolling history of tool-only-turn fingerprints and pauses on detectToolFingerprintThrash instead of a hand-rolled last-fingerprint comparison, so it catches alternating and rotating tool-call cycles the old check missed entirely, without false-positiving on a handful of identical polling calls. Removed toolOnlyNoProgressRepeatLimit from ModelFamilyPolicy — the thrash check is no longer a single tunable number, and isn't family-specific. Updated the stale applyToolOnlyLoopProtection JSDoc, which claimed the pause only fires after the nudge — no longer true, since the thrash check can (and often does) fire well before the nudge threshold. Updated docs/ARCHITECTURE.md's director-policy section to describe period detection accurately, with file:line references. Tests: alternating A,B for 200 turns now pauses (critique's exact repro), a 3-cycle A,B,C pauses, 4 identical polls followed by varied work does not pause, a long varied productive streak never pauses, and the nudge path still does not reply-pause.
Period detection has a hard ceiling (max scanned period 8) and only fires on
an exact repeating tail, so a rotation longer than the ceiling, or a
"phase-broken" cycle that inserts a varying element between repeats (e.g.
A,B,A,B,UNIQUE,...), escapes it forever regardless of streak length.
detectRawToolOnlyBackstop is a secondary, pattern-free check on the raw
tool-only streak length, wired into the ChatDirector so it only fires once
period detection has not already caught the turn. It uses its own pause
message ("ran N tool-only turns without narrating progress") rather than the
pattern-detection wording, since no pattern was found. Threshold is 60,
derived from the current forensic scan (328 sessions with a tool-only run,
559 tool-only runs): run-length p50 3, p90 8, p99 16, max 28 — 60 is more
than double the longest healthy streak ever observed and stays well clear of
the old hard-pause-at-10 that originally motivated this rework.
Also documents on TOOL_FINGERPRINT_MAX_PERIOD that it is a ceiling with no
forensic backing above period 6 (the scan's actual range), and that the
backstop is what catches anything above it.
An earlier commit on this branch claimed the forensic scan (scripts/tool-fingerprint-forensics.ts) covered periods 1-8 across 328 sessions; the script only ever scanned periods 1-6 (MAX_PERIOD_SCANNED). That inaccurate claim was repeated in docs/ARCHITECTURE.md and model-family-policy.ts (stop-policy.ts's copy was fixed in the previous commit alongside the ceiling comment it lives next to). All three now say periods 1-6, and model-family-policy.ts's healthy-streak figures are updated to the run this scan currently produces (p50 3, p90 8, p99 16, max 28) rather than the older "13-28" summary. Also drops product-name attribution from a comment that no longer needs it. docs/ARCHITECTURE.md's director-policy section now also describes the raw-count backstop added in the previous commit: period detection as the fast path, the backstop as the final net for cycles above the period ceiling or phase-broken patterns, with file references for both.
Critique found the round-3 backstop's own escape: narrated text reset both
the period-detection history AND the raw backstop counter, so a model that
narrated one word every ~55 turns kept resetting the backstop before it
could fire. Period detection ("is the model cycling?") still clears on
narration. The backstop is now a separate counter, turnsSinceUserMessage,
that only clears on a genuine fresh user message.
Since narration no longer buys back backstop budget, a legitimately long
autonomous run will now reach it. Reaching the backstop no longer pauses
outright — it nudges for a progress summary. Only if that nudge goes
unanswered for a further full backstop interval, with still no user message
and no thrash detected, does the session hard-pause. A genuine cycle (period
detection) still pauses immediately regardless.
Re-derived the threshold from a fresh local scan of
turns-since-last-genuine-user-message (filtering tool-result echoes, which
are also role "user" in the transcript format): p50 5, p90 14, p99 29, max
32 across 428 runs. Set to 100, roughly 3x the measured max.
… apart from synthetic sends Round 4 reset the turns-since-user-message backstop on any message.received event, which synthetic system sends (compaction continuations from tui/runner.ts, exec/runner.ts, subagent/run.ts) also fire without being operator input — and compaction fires more often during long tool-only loops, exactly when the backstop should be counting. Adds OPERATOR_ORIGINATED_FLAG, set only where a human actually submits a prompt (TUI prompt-submit and the "send" command result, exec's initial task). The backstop now resets only when that flag is present, so a future synthetic sender has to explicitly opt in rather than silently qualifying by omission.
The commit message, stop-policy.ts, and ARCHITECTURE.md cited a 358-session/428-run scan of turns-since-last-genuine-operator-message with a stated methodology; no corresponding script or output exists anywhere in the tree, and the two numbers already disagreed with each other. That measurement was never taken. Rewrites all three to state plainly that 100 is a judgment call, not a measured value, informed only by the streak-length data we do have (tool-fingerprint-forensics.ts: p50 3, p90 8, p99 16, max 28 across 328 sessions) even though that measures a different quantity than this counter. Also fixes the stale backstopNudgeFiredAtTurn comment, which claimed a reset on thrash/escalation that does not happen in code.
Collaborator
Author
Closing — fold into continuous-director (CL-5758)This PR is closed without merge. It conflicts with main and is not the product home for this work. Salvage the durable pieces into CL-5758 (continuous-director); do not finish or rebase this as a standalone PR. Why close
SALVAGE into CL-5758Bring these into continuous-director work (do not lose them with the close):
DROP (do not carry into CL-5758)
BranchLeaving Related: CL-5611 (origin), fold target CL-5758. |
Collaborator
Author
|
Closed without merge — work folds into continuous-director (CL-5758). See fold plan comment above. Branch retained as historical reference. |
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
detectToolFingerprintThrash(src/subagent/stop-policy.ts) runs exact-period detection over a rolling history of tool-call fingerprints, catching not just identical-every-turn repeats but alternating/rotating cycles (A,B,A,B,...;A,B,C,A,B,C,...) at any streak length. This is what actually hard-pauses on a genuine thrash — fast (an alternating pair pauses at 6 turns, a 3-call cycle at 9) and independent of streak length. It has a period ceiling (8) with no forensic backing above period 6 in the local scan, only headroom.toolFingerprintHistory/lastThrashCheck) still clears on any narrated turn — narration is legitimate evidence the model is not stuck in a tight loop.turnsSinceUserMessage) clears only on a genuine fresh user message. Model-emitted text does not reset it.docs/ARCHITECTURE.mdupdated to describe the fast-path/backstop split, the nudge-then-escalate behavior, and which forensic dataset informs which threshold.Thresholds and their forensic basis
IDENTICAL_REPEAT_MIN= 5,CYCLE_REPEAT_MIN= 3) are informed byscripts/tool-fingerprint-forensics.ts(328 sessions with a tool-only run, 559 tool-only runs): zero occurrences of any repeating cycle for periods 1-6 in local trace history.TURNS_SINCE_USER_MESSAGE_BACKSTOP= 100) is informed by a separate measurement: turns since the last genuine user message (filtering out API tool-result echoes, which are also roleuserin the transcript format but are not the operator), not tool-only run length — narration no longer resets this counter, so it needed its own distribution. p50 5, p90 14, p99 29, max 32 across 428 runs from the same 358-session local corpus. 100 sits roughly 3x the measured max and >3x measured p99.Why this is director policy, human review required
This changes main-session ChatDirector loop protection. Per team standing rule, prompt/director/reactor/context-management changes never merge without the operator reading the diff — this PR is not auto-mergeable.
Test plan
bun run typecheckbun run buildbun run test(full suite; only pre-existing, unrelated failure issrc/agent/lsp-availability.test.ts, caused by a missingtypescript-language-serverdevDependency install in this worktree — not touched by this change)A,B(6 turns) andA,B,C(9 turns), well ahead of the backstop, and is distinguishable from a backstop pause in the reply message.CL-5611