From ce7ac197a02574b215f056b433513efb7ced9502 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Mon, 24 Aug 2026 00:15:34 -0700 Subject: [PATCH] Delete never-edited/never-acted/no-ship/no-progress salvage classes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two peer agent loops have no concept of a stop that bars retry, and neither treats a tool-using run with no net file diff as a failure. This project had five: no-ship, no-progress, never-acted, never-edited, and repetition all hard-blocked identical re-dispatch for the rest of the session. A worker sharing a directory can issue real edits that a concurrent lane absorbs, leaving no net diff — the never-edited class read that as "did nothing" and discarded the worker's real work while also blocking retry. Deletes HARD_BLOCK_SALVAGES/isHardBlockSalvage and the fingerprint-refusal logic in brief-dispatch.ts (the ledger now always admits), the requireEdit branch and the consecutive-identical no-progress check in stop-policy.ts (with their bookkeeping: DEFAULT_SUBAGENT_REPEAT_LIMIT, ToolCallStreak, nextToolCallStreak, subAgentNoProgress), and the four now-false "it will be refused" parent hints. shell-evidence.ts's write-detection existed solely to feed requireEdit and is now dead; shrunk to reads-only (still feeds the CritiqueDirector requireEvidence gate). editedPaths bookkeeping in thrash.ts stays for intervention-log diagnostics only. repetition keeps its detector (a sibling change owns deleting that) but loses its hard-block membership here, matching every other salvage class: it ends the run and reports the loop, but does not refuse a later re-dispatch. --- CHANGELOG.md | 12 + src/agent/director.test.ts | 6 +- src/agent/director.ts | 4 +- src/agent/prompts.ts | 2 +- src/subagent/brief-dispatch.ts | 106 ++------ src/subagent/index.test.ts | 358 +++++----------------------- src/subagent/index.ts | 11 - src/subagent/nudge-director.test.ts | 17 +- src/subagent/nudge-director.ts | 68 +----- src/subagent/run.ts | 3 - src/subagent/shell-evidence.test.ts | 26 +- src/subagent/shell-evidence.ts | 86 +------ src/subagent/stop-policy.ts | 234 ++++-------------- src/subagent/task-tool.ts | 19 +- src/subagent/thrash.test.ts | 8 +- src/subagent/thrash.ts | 20 +- 16 files changed, 179 insertions(+), 801 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 282385557..cc44609a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,18 @@ matching `## [X.Y.Z]` section (plus install instructions). Do not maintain parallel copies under `docs/` or `scripts/notes/`. At cut time: rename `## [Unreleased]` to `## [X.Y.Z] - YYYY-MM-DD`, then run the release script. +## [Unreleased] + +### Agent + +- Removed the `never-edited`, `never-acted`, `no-ship`, and `no-progress` leaf + salvage classes and the sticky hard-block that refused an identical + re-dispatch after any of them fired. A worker sharing a directory can issue + real edits that a concurrent writer absorbs, leaving no net diff — that is + not a failure, and no salvage class now treats it as one. `turn-budget`, + `deadline`, `stalled`, `cancelled`, `incomplete-report`, and `repetition` + are unaffected. + ## [0.2.108] - 2026-08-24 ### Agent diff --git a/src/agent/director.test.ts b/src/agent/director.test.ts index 11af0ea4d..aeb33ab50 100644 --- a/src/agent/director.test.ts +++ b/src/agent/director.test.ts @@ -840,13 +840,13 @@ describe("ChatDirector tool-only loop protection", () => { expect(actions.some((a) => a.type === "infer")).toBe(true); }); - test("after a hard-block salvage, Skywalker is nudged once and unique reads do not pause", async () => { + test("after a repetition salvage, Skywalker is nudged once and unique reads do not pause", async () => { const director = createChatDirector("system", [], { onTasksChange: () => {}, provider: providerlessPolicy, }); const capabilities = makeCapabilities(); - const salvage = forcedStopReport("no-ship", "mapped the tree, never edited"); + const salvage = forcedStopReport("repetition", "looped mid-stream"); await director.decide( { type: "inference.done", @@ -1071,7 +1071,7 @@ describe("ChatDirector tool-only loop protection", () => { await director.decide(taskTurn(id), mockState, capabilities); const result = actionsArray( await director.decide( - taskDoneEvent(id, { content: forcedStopReport("no-progress", "x") }), + taskDoneEvent(id, { content: forcedStopReport("turn-budget", "x") }), mockState, capabilities, ), diff --git a/src/agent/director.ts b/src/agent/director.ts index 7fc147b5d..9ba74a375 100644 --- a/src/agent/director.ts +++ b/src/agent/director.ts @@ -31,7 +31,7 @@ import { } from "../subagent/stop-policy.js"; import { PRESENT_VIEW_PRIMITIVES_GUIDANCE } from "./tool-schema-normalize.js"; import { isOperatorOriginated } from "./message-provenance.js"; -import { classifyBriefSalvage, isHardBlockSalvage } from "../subagent/brief-dispatch.js"; +import { classifyBriefSalvage } from "../subagent/brief-dispatch.js"; import { PRIMARY_SALVAGE_NUDGE } from "./look-tour.js"; // Fired when turnsSinceUserMessage reaches TURNS_SINCE_USER_MESSAGE_BACKSTOP. @@ -935,7 +935,7 @@ class ChatDirectorImpl extends DefaultDirector { this.pendingTaskCallIds.delete(event.result.callId); const body = typeof event.result.content === "string" ? event.result.content : ""; const salvage = classifyBriefSalvage(body); - if (salvage !== null && isHardBlockSalvage(salvage) && !this.salvageNudgeFired) { + if (salvage === "repetition" && !this.salvageNudgeFired) { this.salvageNudgeFired = true; this.pendingSalvageNudge = PRIMARY_SALVAGE_NUDGE; } diff --git a/src/agent/prompts.ts b/src/agent/prompts.ts index e008b6a17..598911118 100644 --- a/src/agent/prompts.ts +++ b/src/agent/prompts.ts @@ -153,7 +153,7 @@ export function buildGuidelines( "- Prefer the typed spawn contract on every worker: `intent`, `success_criteria` (done-when), `do_not` (scope fence), and `report_focus` so workers finish instead of thrashing. Free-form `prompt` alone is weaker.", "- After workers return, merge their Summary/Findings into a coherent answer for the operator; do not paste raw sub-agent dumps.", "- Pass `maxTurns` on `task` when a job needs a larger inference budget (default 30, no hard upper cap). On turn-budget salvage, re-dispatch with continuation context and a higher maxTurns only a few times on the same brief — after the re-dispatch cap, change approach instead of bumping turns again.", - "- After thrash / no-progress / repetition / never-acted salvage, do not re-dispatch an identical brief (prompt/agent/intent/success_criteria/do_not) — it is refused. Change the brief to force a re-run; maxTurns alone does not unlock it.", + "- After a repetition salvage, re-dispatching an identical brief (prompt/agent/intent/success_criteria/do_not) unchanged will likely loop again — change the brief before retrying.", "- Use manage_tasks for your own coordination checklist; spawning workers is `task`, not manage_tasks.", "- If context is compacted automatically, do not stop tasks early due to token fear; persist progress via manage_tasks and worker reports.", ]), diff --git a/src/subagent/brief-dispatch.ts b/src/subagent/brief-dispatch.ts index 7cf8d6fbf..e3175e149 100644 --- a/src/subagent/brief-dispatch.ts +++ b/src/subagent/brief-dispatch.ts @@ -1,12 +1,11 @@ /** - * Parent-side re-dispatch caps for task briefs (CL-4343 + CL-5203). + * Parent-side re-dispatch tracking for task briefs (CL-4343 + CL-5203). * - * Leaf stops already salvage no-progress / turn-budget / etc. This - * module tracks how often the *parent* re-spawns the same brief so: - * - hard-block-class salvages refuse an identical re-dispatch for the rest of - * the parent chat session (sticky until the fingerprint changes) - * - turn-budget salvage flips from "raise maxTurns" to "stop" after enough - * same-brief dispatches without a successful complete + * Leaf stops already salvage turn-budget / deadline / etc. This module + * tracks how often the *parent* re-spawns the same brief so turn-budget + * salvage flips from "raise maxTurns" to "stop" after enough same-brief + * dispatches without a successful complete. No salvage class refuses + * re-dispatch (CL-6994) — every dispatch is admitted. * * Session-scoped: one ledger per createTaskTool instance (parent chat tool). */ @@ -15,20 +14,12 @@ import type { TaskIntent } from "./report.js"; import { isDeadlineSubAgentReport, isForcedStopSubAgentReport, - isNeverActedSubAgentReport, - isNeverEditedSubAgentReport, - isNoProgressSubAgentReport, - isNoShipSubAgentReport, isRepetitionSubAgentReport, isTurnBudgetSubAgentReport, } from "./stop-policy.js"; -/** Salvage classes that must not be re-dispatched with an identical brief. */ -export type HardBlockSalvage = - "no-ship" | "no-progress" | "repetition" | "never-acted" | "never-edited"; - export type BriefSalvageKind = - HardBlockSalvage | "turn-budget" | "deadline" | "stalled" | "cancelled" | "incomplete-report"; + "turn-budget" | "deadline" | "stalled" | "cancelled" | "incomplete-report" | "repetition"; export interface TaskBriefFingerprintInput { prompt: string; @@ -41,8 +32,6 @@ export interface TaskBriefFingerprintInput { export interface BriefDispatchRecord { /** How many times this fingerprint has been accepted for run (including first). */ dispatchCount: number; - /** Last salvage class observed for this fingerprint, if any. */ - lastSalvage?: BriefSalvageKind; } /** @@ -52,18 +41,6 @@ export interface BriefDispatchRecord { */ export const TURN_BUDGET_STOP_AFTER_DISPATCHES = 3; -const HARD_BLOCK_SALVAGES = new Set([ - "no-ship", - "no-progress", - "repetition", - "never-acted", - "never-edited", -]); - -export function isHardBlockSalvage(kind: BriefSalvageKind): kind is HardBlockSalvage { - return HARD_BLOCK_SALVAGES.has(kind); -} - /** True when the worker returned a stall salvage report. */ export function isStalledSubAgentReport(report: string): boolean { return isForcedStopSubAgentReport(report, "stalled"); @@ -85,11 +62,7 @@ export function isIncompleteReportSubAgentReport(report: string): boolean { */ export function classifyBriefSalvage(report: string): BriefSalvageKind | null { // Order: more specific salvage phrases first. - if (isNoShipSubAgentReport(report)) return "no-ship"; if (isRepetitionSubAgentReport(report)) return "repetition"; - if (isNeverEditedSubAgentReport(report)) return "never-edited"; - if (isNeverActedSubAgentReport(report)) return "never-acted"; - if (isNoProgressSubAgentReport(report)) return "no-progress"; if (isTurnBudgetSubAgentReport(report)) return "turn-budget"; if (isDeadlineSubAgentReport(report)) return "deadline"; if (isStalledSubAgentReport(report)) return "stalled"; @@ -127,13 +100,8 @@ function serializeList(items: readonly string[] | undefined): string { export interface BriefDispatchLedger { get: (fingerprint: string) => BriefDispatchRecord | undefined; - /** - * Pre-run gate. Returns ok with the 1-based dispatch count that will be used, - * or a reject message for the parent tool result. - */ - admit: ( - fingerprint: string, - ) => { ok: true; dispatchCount: number } | { ok: false; message: string }; + /** Pre-run gate. Always admits; returns the 1-based dispatch count that will be used. */ + admit: (fingerprint: string) => { ok: true; dispatchCount: number }; /** Record the outcome of an admitted run (salvage kind or null on success). */ recordOutcome: (fingerprint: string, salvage: BriefSalvageKind | null) => void; /** @@ -153,77 +121,31 @@ export function createBriefDispatchLedger(): BriefDispatchLedger { admit(fingerprint) { const existing = byFingerprint.get(fingerprint); - if (existing?.lastSalvage !== undefined && isHardBlockSalvage(existing.lastSalvage)) { - return { - ok: false, - message: hardBlockMessage(existing.lastSalvage, existing.dispatchCount), - }; - } const nextCount = (existing?.dispatchCount ?? 0) + 1; - byFingerprint.set(fingerprint, { - dispatchCount: nextCount, - ...(existing?.lastSalvage !== undefined ? { lastSalvage: existing.lastSalvage } : {}), - }); + byFingerprint.set(fingerprint, { dispatchCount: nextCount }); return { ok: true, dispatchCount: nextCount }; }, recordOutcome(fingerprint, salvage) { - const existing = byFingerprint.get(fingerprint); - if (existing === undefined) { - // admit() always runs first in production; keep defensive for unit tests. - byFingerprint.set(fingerprint, { - dispatchCount: salvage === null ? 0 : 1, - ...(salvage !== null ? { lastSalvage: salvage } : {}), - }); - return; - } + // A successful complete resets the same-brief retry budget. Any other + // salvage leaves dispatchCount as admit() already recorded it. if (salvage === null) { - // CL-6710: a successful complete clears the sticky hard-block too. - // Two concurrent identical-brief dispatches can both admit; if one - // salvages and the other succeeds, the success proves the brief is - // re-dispatchable, so it must not leave the sibling's hard-block - // standing for the rest of the session. byFingerprint.set(fingerprint, { dispatchCount: 0 }); - return; } - byFingerprint.set(fingerprint, { - dispatchCount: existing.dispatchCount, - lastSalvage: salvage, - }); }, release(fingerprint) { const existing = byFingerprint.get(fingerprint); if (existing === undefined) return; if (existing.dispatchCount <= 1) { - if (existing.lastSalvage !== undefined) { - byFingerprint.set(fingerprint, { - dispatchCount: 0, - lastSalvage: existing.lastSalvage, - }); - } else { - byFingerprint.delete(fingerprint); - } + byFingerprint.delete(fingerprint); return; } - byFingerprint.set(fingerprint, { - dispatchCount: existing.dispatchCount - 1, - ...(existing.lastSalvage !== undefined ? { lastSalvage: existing.lastSalvage } : {}), - }); + byFingerprint.set(fingerprint, { dispatchCount: existing.dispatchCount - 1 }); }, }; } -function hardBlockMessage(salvage: HardBlockSalvage, priorDispatches: number): string { - return ( - `Error: refused re-dispatch of an identical task brief after a ${salvage} salvage ` + - `(already dispatched ${priorDispatches} time${priorDispatches === 1 ? "" : "s"}). ` + - `Change the brief (prompt, agent, intent, success_criteria, and/or do_not) before retrying — ` + - `raising maxTurns alone will not unlock this fingerprint. ` + - `To force a re-run of the same work, alter at least one of those fields so the fingerprint changes.` - ); -} - /** * Whether turn-budget parent hint should recommend stopping rather than * re-dispatching with a higher maxTurns. diff --git a/src/subagent/index.test.ts b/src/subagent/index.test.ts index 5584b21b9..5ef108ac4 100644 --- a/src/subagent/index.test.ts +++ b/src/subagent/index.test.ts @@ -11,18 +11,15 @@ import { createSubAgentSessionStore, createSubAgentSpawnRegistryPlugin, DEFAULT_SUBAGENT_MAX_TURNS, - DEFAULT_SUBAGENT_REPEAT_LIMIT, disposeSubAgentSession, evaluateSubAgentStop, fingerprintToolCalls, forcedStopReport, formatSubAgentReport, - nextToolCallStreak, parseSubAgentReport, repetitionStopDetail, stopReasonFromReport, appendDeadlineParentHint, - appendNeverActedParentHint, appendSubAgentParentHints, createBriefDispatchLedger, fingerprintTaskBrief, @@ -39,7 +36,6 @@ import { subAgentToolName, SUBAGENT_DEADLINE_MARGIN_MS, SUBAGENT_PLUGIN_SPAWN_TEARDOWN_LIMITS, - subAgentNoProgress, subAgentTurnLimitExceeded, SubAgentDirector, TaskToolArgs, @@ -158,44 +154,21 @@ describe("sub-agent stop helpers", () => { ).toBe(false); }); - test("no-progress trips at the default repeat limit", () => { - expect(DEFAULT_SUBAGENT_REPEAT_LIMIT).toBe(5); - expect(subAgentNoProgress(4, DEFAULT_SUBAGENT_REPEAT_LIMIT)).toBe(false); - expect(subAgentNoProgress(5, DEFAULT_SUBAGENT_REPEAT_LIMIT)).toBe(true); - }); - - test("legitimate polling (2-4 identical fingerprints) does not hard-stop (CL-6776)", () => { - // A worker rerunning `git status` or polling a build a few times while - // waiting must not be hard-blocked on identical re-dispatch. - for (const consecutive of [2, 3, 4]) { - expect(subAgentNoProgress(consecutive, DEFAULT_SUBAGENT_REPEAT_LIMIT)).toBe(false); + test("repeated identical tool calls do not hard-stop (CL-6994: no salvage class gates retry)", () => { + // Two peer agent loops have no concept of a stop that bars retry on a + // repeated tool call — a worker rerunning `git status` or polling a + // build must not be treated as stuck just because the fingerprint repeats. + for (const turnsCompleted of [2, 6, 29]) { expect( evaluateSubAgentStop({ hasToolCalls: true, - everHadToolCalls: true, - turnsCompleted: consecutive, + turnsCompleted, maxTurns: DEFAULT_SUBAGENT_MAX_TURNS, - consecutiveIdentical: consecutive, - repeatLimit: DEFAULT_SUBAGENT_REPEAT_LIMIT, }), ).toBeNull(); } }); - test("a true runaway (>5 identical fingerprints) still hard-stops", () => { - expect(subAgentNoProgress(6, DEFAULT_SUBAGENT_REPEAT_LIMIT)).toBe(true); - expect( - evaluateSubAgentStop({ - hasToolCalls: true, - everHadToolCalls: true, - turnsCompleted: 6, - maxTurns: DEFAULT_SUBAGENT_MAX_TURNS, - consecutiveIdentical: 6, - repeatLimit: DEFAULT_SUBAGENT_REPEAT_LIMIT, - }), - ).toBe("no-progress"); - }); - test("fingerprint is null when a turn has no tool calls", () => { expect(fingerprintToolCalls([{ type: "text" }])).toBeNull(); }); @@ -237,11 +210,8 @@ describe("sub-agent stop helpers", () => { expect( evaluateSubAgentStop({ hasToolCalls: false, - everHadToolCalls: true, turnsCompleted: 2, maxTurns: 10, - consecutiveIdentical: 0, - repeatLimit: 2, }), ).toBe("complete"); }); @@ -270,11 +240,8 @@ describe("sub-agent stop helpers", () => { expect( evaluateSubAgentStop({ hasToolCalls: false, - everHadToolCalls: true, turnsCompleted: 2, maxTurns: 10, - consecutiveIdentical: 0, - repeatLimit: 2, lastAssistantText: SUMMARY_ONLY_NARRATION, }), ).toBe("incomplete-report"); @@ -284,11 +251,8 @@ describe("sub-agent stop helpers", () => { expect( evaluateSubAgentStop({ hasToolCalls: false, - everHadToolCalls: true, turnsCompleted: 3, maxTurns: 10, - consecutiveIdentical: 0, - repeatLimit: 2, lastAssistantText: SUMMARY_ONLY_NARRATION, incompleteReportNudgeFired: true, }), @@ -299,11 +263,8 @@ describe("sub-agent stop helpers", () => { expect( evaluateSubAgentStop({ hasToolCalls: false, - everHadToolCalls: true, turnsCompleted: 2, maxTurns: 10, - consecutiveIdentical: 0, - repeatLimit: 2, lastAssistantText: FULL_REPORT_ENVELOPE, }), ).toBe("complete"); @@ -335,11 +296,8 @@ describe("sub-agent stop helpers", () => { expect( evaluateSubAgentStop({ hasToolCalls: false, - everHadToolCalls: true, turnsCompleted: 2, maxTurns: 10, - consecutiveIdentical: 0, - repeatLimit: 2, lastAssistantText: FULL_REPORT_ENVELOPE, thrashState, requireEvidence: true, @@ -356,11 +314,8 @@ describe("sub-agent stop helpers", () => { expect( evaluateSubAgentStop({ hasToolCalls: false, - everHadToolCalls: true, turnsCompleted: 2, maxTurns: 10, - consecutiveIdentical: 0, - repeatLimit: 2, lastAssistantText: FULL_REPORT_ENVELOPE, thrashState, requireEvidence: true, @@ -377,11 +332,8 @@ describe("sub-agent stop helpers", () => { expect( evaluateSubAgentStop({ hasToolCalls: false, - everHadToolCalls: true, turnsCompleted: 2, maxTurns: 10, - consecutiveIdentical: 0, - repeatLimit: 2, lastAssistantText: FULL_REPORT_ENVELOPE, thrashState, requireEvidence: false, @@ -389,20 +341,22 @@ describe("sub-agent stop helpers", () => { ).toBe("complete"); }); - test("evaluateSubAgentStop returns never-acted when the run never used tools", () => { + test("evaluateSubAgentStop completes a run that never used tools when the text is a full envelope (CL-6994)", () => { + // No salvage class gates a zero-tool-call run any more — the two peer + // agent loops this project measured itself against have no such concept. expect( evaluateSubAgentStop({ hasToolCalls: false, - everHadToolCalls: false, turnsCompleted: 1, maxTurns: 10, - consecutiveIdentical: 0, - repeatLimit: 2, + lastAssistantText: FULL_REPORT_ENVELOPE, }), - ).toBe("never-acted"); + ).toBe("complete"); }); - test("evaluateSubAgentStop returns never-edited when requireEdit and tools never wrote files", () => { + test("a run with tool calls that left no net edit still completes (CL-6994)", () => { + // A worker sharing a directory can issue real edits that a concurrent + // writer absorbs, leaving editedPaths empty. That is not a failure. const thrashState = { totalToolCalls: 4, readCounts: new Map([["src/a.ts", 2]]), @@ -411,18 +365,15 @@ describe("sub-agent stop helpers", () => { expect( evaluateSubAgentStop({ hasToolCalls: false, - everHadToolCalls: true, turnsCompleted: 5, maxTurns: 30, - consecutiveIdentical: 0, - repeatLimit: 2, thrashState, - requireEdit: true, + lastAssistantText: FULL_REPORT_ENVELOPE, }), - ).toBe("never-edited"); + ).toBe("complete"); }); - test("evaluateSubAgentStop does not hard-stop implement for many unique reads", () => { + test("evaluateSubAgentStop does not hard-stop on many unique reads", () => { let thrash = EMPTY_THRASH_STATE; for (let i = 0; i < 200; i++) { thrash = nextThrashState(thrash, [ @@ -432,29 +383,14 @@ describe("sub-agent stop helpers", () => { expect( evaluateSubAgentStop({ hasToolCalls: true, - everHadToolCalls: true, turnsCompleted: 40, maxTurns: 60, - consecutiveIdentical: 0, - repeatLimit: 2, - thrashState: thrash, - requireEdit: true, - }), - ).toBeNull(); - expect( - evaluateSubAgentStop({ - hasToolCalls: true, - everHadToolCalls: true, - turnsCompleted: 40, - maxTurns: 60, - consecutiveIdentical: 0, - repeatLimit: 2, thrashState: thrash, }), ).toBeNull(); }); - test("evaluateSubAgentStop still completes implement when an edit path was recorded", () => { + test("evaluateSubAgentStop still completes when an edit path was recorded", () => { const thrashState = { totalToolCalls: 4, readCounts: new Map([["src/a.ts", 1]]), @@ -463,117 +399,63 @@ describe("sub-agent stop helpers", () => { expect( evaluateSubAgentStop({ hasToolCalls: false, - everHadToolCalls: true, turnsCompleted: 5, maxTurns: 30, - consecutiveIdentical: 0, - repeatLimit: 2, thrashState, - requireEdit: true, + lastAssistantText: FULL_REPORT_ENVELOPE, }), ).toBe("complete"); }); - test("evaluateSubAgentStop ignores requireEdit when the run never used tools (never-acted wins)", () => { - expect( - evaluateSubAgentStop({ - hasToolCalls: false, - everHadToolCalls: false, - turnsCompleted: 1, - maxTurns: 10, - consecutiveIdentical: 0, - repeatLimit: 2, - requireEdit: true, - }), - ).toBe("never-acted"); - }); - - test("evaluateSubAgentStop prefers no-progress over turn-budget", () => { + test("evaluateSubAgentStop trips turn-budget when the leaf is still making identical progress", () => { expect( evaluateSubAgentStop({ hasToolCalls: true, - everHadToolCalls: true, turnsCompleted: 10, maxTurns: 10, - consecutiveIdentical: 2, - repeatLimit: 2, - }), - ).toBe("no-progress"); - }); - - test("evaluateSubAgentStop trips turn-budget when the leaf is still making progress", () => { - expect( - evaluateSubAgentStop({ - hasToolCalls: true, - everHadToolCalls: true, - turnsCompleted: 10, - maxTurns: 10, - consecutiveIdentical: 1, - repeatLimit: 2, }), ).toBe("turn-budget"); }); - test("evaluateSubAgentStop keeps running while fingerprints change under budget", () => { + test("evaluateSubAgentStop keeps running under budget", () => { expect( evaluateSubAgentStop({ hasToolCalls: true, - everHadToolCalls: true, turnsCompleted: 5, maxTurns: 10, - consecutiveIdentical: 1, - repeatLimit: 2, }), ).toBeNull(); }); - test("evaluateSubAgentStop prefers no-progress over the turn budget", () => { + test("repeated identical tool calls do not preempt the turn budget (CL-6994)", () => { let thrash = EMPTY_THRASH_STATE; for (let i = 0; i < 4; i++) { thrash = nextThrashState(thrash, [ { type: "tool_call", name: "read_file", arguments: { path: "a.ts" } }, ]); } - thrash = nextThrashState(thrash, [ - { type: "tool_call", name: "edit_file", arguments: { path: "a.ts" } }, - ]); - thrash = nextThrashState(thrash, [ - { type: "tool_call", name: "read_file", arguments: { path: "a.ts" } }, - ]); expect( evaluateSubAgentStop({ hasToolCalls: true, - everHadToolCalls: true, - turnsCompleted: 5, + turnsCompleted: 20, maxTurns: 20, - consecutiveIdentical: 2, - repeatLimit: 2, thrashState: thrash, }), - ).toBe("no-progress"); + ).toBe("turn-budget"); }); - test("shell-only work is not never-edited or incomplete-report (CL-6937)", () => { + test("shell-only reads count as CritiqueDirector evidence (CL-6937)", () => { const shellState = nextThrashState(EMPTY_THRASH_STATE, [ { type: "tool_call", name: "run_shell", arguments: { command: "cat src/a.ts" } }, - { - type: "tool_call", - name: "run_shell", - arguments: { command: "sed -i '' 's/a/b/' src/a.ts" }, - }, ]); const report = "## Summary\nDid it\n\n## Findings\nx\n\n## Blockers\nNone\n\n## Paths\nsrc/a.ts"; expect( evaluateSubAgentStop({ hasToolCalls: false, - everHadToolCalls: true, turnsCompleted: 4, maxTurns: 30, - consecutiveIdentical: 0, - repeatLimit: 5, thrashState: shellState, - requireEdit: true, requireEvidence: true, lastAssistantText: report, }), @@ -593,11 +475,8 @@ describe("sub-agent stop helpers", () => { expect( evaluateSubAgentStop({ hasToolCalls: true, - everHadToolCalls: true, turnsCompleted: 10, maxTurns: 10, - consecutiveIdentical: 1, - repeatLimit: 2, thrashState: thrash, }), ).toBe("turn-budget"); @@ -607,11 +486,8 @@ describe("sub-agent stop helpers", () => { expect( evaluateSubAgentStop({ hasToolCalls: true, - everHadToolCalls: true, turnsCompleted: 8, maxTurns: 10, - consecutiveIdentical: 1, - repeatLimit: 2, thrashState: EMPTY_THRASH_STATE, }), ).toBe("report-forced"); @@ -619,22 +495,16 @@ describe("sub-agent stop helpers", () => { expect( evaluateSubAgentStop({ hasToolCalls: true, - everHadToolCalls: true, turnsCompleted: 9, maxTurns: 10, - consecutiveIdentical: 1, - repeatLimit: 2, thrashState: EMPTY_THRASH_STATE, }), ).toBeNull(); expect( evaluateSubAgentStop({ hasToolCalls: true, - everHadToolCalls: true, turnsCompleted: 10, maxTurns: 10, - consecutiveIdentical: 1, - repeatLimit: 2, thrashState: EMPTY_THRASH_STATE, }), ).toBe("turn-budget"); @@ -650,57 +520,22 @@ describe("sub-agent stop helpers", () => { expect( evaluateSubAgentStop({ hasToolCalls: true, - everHadToolCalls: true, turnsCompleted: 5, maxTurns: 20, - consecutiveIdentical: 1, - repeatLimit: 2, thrashState: thrash, }), ).toBeNull(); }); - test("nextToolCallStreak increments on identical fingerprints and resets on change", () => { - let streak = nextToolCallStreak( - { lastFingerprint: undefined, consecutiveIdentical: 0 }, - 'read_file:{"path":"a.ts"}', - ); - expect(streak.consecutiveIdentical).toBe(1); - streak = nextToolCallStreak(streak, 'read_file:{"path":"a.ts"}'); - expect(streak.consecutiveIdentical).toBe(2); - streak = nextToolCallStreak(streak, 'read_file:{"path":"b.ts"}'); - expect(streak.consecutiveIdentical).toBe(1); - streak = nextToolCallStreak(streak, null); - expect(streak).toEqual({ lastFingerprint: undefined, consecutiveIdentical: 0 }); - }); - test("forcedStopReport is a real envelope with salvage findings, not a summarize instruction", () => { - const noProgress = forcedStopReport("no-progress", "Found auth in gate.ts"); - const parsed = parseSubAgentReport(noProgress); - expect(parsed.summary).toContain("no progress"); - expect(parsed.findings).toContain("gate.ts"); - expect(parsed.blockers.length).toBeGreaterThan(0); - expect(noProgress.toLowerCase()).not.toContain("summarize what you found"); - const budget = forcedStopReport("turn-budget", ""); const budgetParsed = parseSubAgentReport(budget); expect(budgetParsed.summary).toContain("Turn budget"); expect(budgetParsed.findings).toContain("no partial findings"); expect(budget.toLowerCase()).not.toContain("summarize progress"); - const neverActed = forcedStopReport("never-acted", "I'll write the red tests next"); - const neverParsed = parseSubAgentReport(neverActed); - expect(neverParsed.summary).toContain("without using any tools"); - expect(neverParsed.findings).toContain("red tests"); - expect(neverParsed.blockers).toContain("unexecuted"); - expect(neverActed.toLowerCase()).not.toContain("summarize what you found"); - - const neverEdited = forcedStopReport("never-edited", "I mapped the files; ready to code next"); - expect(neverEdited).toContain("without writing any files"); - expect(appendSubAgentParentHints(neverEdited)).toContain("edit-first"); - - // Nested agent envelope must not clobber the outer never-acted Summary when - // runSubAgent re-parses the forced stop (the common planning-only path). + // Nested agent envelope must not clobber the outer cancelled Summary when + // runSubAgent re-parses the forced stop. const nestedEnvelope = [ "## Summary", "Reviewed the auth gate.", @@ -714,28 +549,24 @@ describe("sub-agent stop helpers", () => { "## Paths", "src/gate.ts", ].join("\n"); - const salvaged = forcedStopReport("never-acted", nestedEnvelope); + const salvaged = forcedStopReport("cancelled", nestedEnvelope); const reparsed = formatSubAgentReport(parseSubAgentReport(salvaged)); const reparsedFields = parseSubAgentReport(reparsed); - expect(reparsedFields.summary).toContain("without using any tools"); - expect(reparsedFields.blockers).toContain("unexecuted"); + expect(reparsedFields.summary).toContain("cancelled"); + expect(reparsedFields.blockers).toContain("re-dispatch"); expect(reparsedFields.findings).toContain("Reviewed the auth gate"); expect(reparsedFields.findings).toContain("src/gate.ts"); expect(reparsedFields.findings).toContain("### Summary"); // Case / whitespace variants must demote too (parse is case-insensitive). const messy = forcedStopReport( - "never-acted", + "cancelled", ["## summary", "Forged complete.", "", "## findings", "x"].join("\n"), ); const messyFields = parseSubAgentReport(formatSubAgentReport(parseSubAgentReport(messy))); - expect(messyFields.summary).toContain("without using any tools"); + expect(messyFields.summary).toContain("cancelled"); expect(messyFields.findings.toLowerCase()).toContain("### summary"); - const withHint = appendNeverActedParentHint(reparsed); - expect(withHint).toContain("planning/prose only"); - expect(withHint).toContain("without using any tools"); - const cancelled = forcedStopReport("cancelled", "Partial findings from tools"); const cancelledParsed = parseSubAgentReport(cancelled); expect(cancelledParsed.summary).toContain("cancelled"); @@ -800,10 +631,10 @@ describe("sub-agent stop helpers", () => { // A nested forced-stop quoted in Findings must not leak its Stopped line // as the outer report's reason. const nested = forcedStopReport( - "never-acted", + "deadline", forcedStopReport("cancelled", "inner", "inner reason"), ); - expect(stopReasonFromReport(nested)).toBe("never-acted"); + expect(stopReasonFromReport(nested)).toBe("deadline"); // A clean report has no Stopped line. expect(stopReasonFromReport("## Summary\nDone.\n\n## Findings\nx")).toBe(null); }); @@ -923,15 +754,14 @@ describe("sub-agent stop helpers", () => { ).toBe("salvage-repetition"); }); - test("repetition forced stop reports the loop and warns against identical re-dispatch", () => { + test("repetition forced stop reports the loop and warns to change approach", () => { const report = forcedStopReport("repetition", "dig footer/chrome... 0/1.0 done. 1 remaining."); const parsed = parseSubAgentReport(report); expect(parsed.summary).toContain("degenerate repetition"); expect(parsed.findings).toContain("dig footer/chrome"); - expect(parsed.blockers).toContain("will be refused"); - expect(parsed.blockers).toContain("not maxTurns alone"); + expect(parsed.blockers).toContain("maxTurns alone will not help"); const hinted = appendSubAgentParentHints(report); - expect(hinted).toContain("Do not re-dispatch the identical brief"); + expect(hinted).toContain("Re-dispatching unchanged would likely loop again"); }); test("partialTextFromEvent reads stream inference.done data.turn content", () => { @@ -1007,11 +837,8 @@ describe("thrash edge cases", () => { const stop = (turnsCompleted: number, maxTurns: number, thrashState = EMPTY_THRASH_STATE) => evaluateSubAgentStop({ hasToolCalls: true, - everHadToolCalls: true, turnsCompleted, maxTurns, - consecutiveIdentical: 0, - repeatLimit: 2, thrashState, }); @@ -1137,8 +964,8 @@ describe("SubAgentDirector report-forced wiring", () => { }); }); - // Turn 1 of 3 fires report-forced (a nudge); repeating one identical call - // to the repeat limit then fires no-progress (a stop). + // Turn 1 of 3 fires report-forced (a nudge); repeating the same identical + // call no longer stops the run (CL-6994) — it runs out the turn budget. for (let i = 0; i < 6; i++) { await director.decide( makeInferenceDoneEvent([{ id: "r1", name: "read_file", args: { path: "a.ts" } }]), @@ -1149,7 +976,7 @@ describe("SubAgentDirector report-forced wiring", () => { const nudge = recorded.find((r) => r.id === "report-forced"); expect(nudge?.class).toBe("nudge"); - const stop = recorded.find((r) => r.id === "no-progress"); + const stop = recorded.find((r) => r.id === "turn-budget"); expect(stop?.class).toBe("stop"); expect(stop?.value).toBeGreaterThanOrEqual(stop?.threshold ?? 0); }); @@ -1267,7 +1094,7 @@ describe("SubAgentDirector stall management", () => { test("no nudge fires before the stall timeout elapses", async () => { let now = 0; - const director = new SubAgentDirector("system", [], undefined, 30, undefined, 1000, () => now); + const director = new SubAgentDirector("system", [], undefined, 30, 1000, () => now); const capabilities = makeCapabilities(); await director.decide(toolCallDoneEvent("tc-1"), mockState, capabilities); @@ -1286,7 +1113,7 @@ describe("SubAgentDirector stall management", () => { test("first stall past the timeout gets one continuation nudge", async () => { let now = 0; - const director = new SubAgentDirector("system", [], undefined, 30, undefined, 1000, () => now); + const director = new SubAgentDirector("system", [], undefined, 30, 1000, () => now); const capabilities = makeCapabilities(); await director.decide(toolCallDoneEvent("tc-1"), mockState, capabilities); @@ -1305,7 +1132,7 @@ describe("SubAgentDirector stall management", () => { test("a second consecutive stall escalates to the salvage report", async () => { let now = 0; - const director = new SubAgentDirector("system", [], undefined, 30, undefined, 1000, () => now); + const director = new SubAgentDirector("system", [], undefined, 30, 1000, () => now); const capabilities = makeCapabilities(); await director.decide(toolCallDoneEvent("tc-1"), mockState, capabilities); @@ -1325,7 +1152,7 @@ describe("SubAgentDirector stall management", () => { test("real activity between pings resets the stall streak", async () => { let now = 0; - const director = new SubAgentDirector("system", [], undefined, 30, undefined, 1000, () => now); + const director = new SubAgentDirector("system", [], undefined, 30, 1000, () => now); const capabilities = makeCapabilities(); await director.decide(toolCallDoneEvent("tc-1"), mockState, capabilities); @@ -2143,41 +1970,16 @@ describe("brief re-dispatch ledger (CL-4343 / CL-5203)", () => { expect(changed).not.toBe(a); }); - test("hard-blocks identical brief after no-progress salvage; allows changed brief", () => { - const ledger = createBriefDispatchLedger(); - const fp = fingerprintTaskBrief({ prompt: "fix no-progress job", intent: "implement" }); - expect(ledger.admit(fp).ok).toBe(true); - ledger.recordOutcome(fp, "no-progress"); - const blocked = ledger.admit(fp); - expect(blocked.ok).toBe(false); - if (blocked.ok) throw new Error("expected block"); - expect(blocked.message).toContain("refused re-dispatch"); - expect(blocked.message).toContain("no-progress"); - - const other = fingerprintTaskBrief({ - prompt: "fix no-progress job with narrower scope", - intent: "implement", - successCriteria: ["one file only"], - }); - expect(ledger.admit(other).ok).toBe(true); - }); - - test("hard-blocks no-progress, repetition, never-acted, never-edited; not turn-budget", () => { - for (const salvage of ["no-progress", "repetition", "never-acted", "never-edited"] as const) { + test("no salvage kind blocks re-dispatch of an identical brief (CL-6994)", () => { + for (const salvage of ["repetition", "turn-budget", "deadline", "stalled"] as const) { const ledger = createBriefDispatchLedger(); const fp = fingerprintTaskBrief({ prompt: `job ${salvage}` }); expect(ledger.admit(fp).ok).toBe(true); ledger.recordOutcome(fp, salvage); - expect(ledger.admit(fp).ok).toBe(false); + const second = ledger.admit(fp); + expect(second.ok).toBe(true); + expect(second.dispatchCount).toBe(2); } - const ledger = createBriefDispatchLedger(); - const fp = fingerprintTaskBrief({ prompt: "budget job" }); - expect(ledger.admit(fp).ok).toBe(true); - ledger.recordOutcome(fp, "turn-budget"); - const second = ledger.admit(fp); - expect(second.ok).toBe(true); - if (!second.ok) throw new Error("expected admit"); - expect(second.dispatchCount).toBe(2); }); test("successful complete resets retry budget and clears soft salvage", () => { @@ -2194,24 +1996,6 @@ describe("brief re-dispatch ledger (CL-4343 / CL-5203)", () => { expect(afterSuccess.dispatchCount).toBe(1); }); - test("CL-6710: a parallel sibling success clears a hard-block salvage on the same fingerprint", () => { - const ledger = createBriefDispatchLedger(); - const fp = fingerprintTaskBrief({ prompt: "parallel identical brief" }); - - // Two concurrent identical-brief dispatches both admit before either finishes. - expect(ledger.admit(fp).ok).toBe(true); - expect(ledger.admit(fp).ok).toBe(true); - - // One sibling salvages (hard-block class)... - ledger.recordOutcome(fp, "no-progress"); - // ...but the other sibling succeeds in the same wave. - ledger.recordOutcome(fp, null); - - // The brief already produced a good report this wave — it must stay - // re-dispatchable, not stuck behind the losing sibling's hard-block. - expect(ledger.admit(fp).ok).toBe(true); - }); - test("release undoes admit when run never produces a body", () => { const ledger = createBriefDispatchLedger(); const fp = fingerprintTaskBrief({ prompt: "crash job" }); @@ -2224,11 +2008,7 @@ describe("brief re-dispatch ledger (CL-4343 / CL-5203)", () => { }); test("classifyBriefSalvage maps forced-stop envelopes", () => { - expect(classifyBriefSalvage(forcedStopReport("no-progress", "x"))).toBe("no-progress"); expect(classifyBriefSalvage(forcedStopReport("repetition", "x"))).toBe("repetition"); - expect(classifyBriefSalvage(forcedStopReport("never-acted", "x"))).toBe("never-acted"); - expect(classifyBriefSalvage(forcedStopReport("never-edited", "x"))).toBe("never-edited"); - expect(classifyBriefSalvage(forcedStopReport("no-ship", "x"))).toBe("no-ship"); expect(classifyBriefSalvage(forcedStopReport("turn-budget", "x"))).toBe("turn-budget"); expect( classifyBriefSalvage( @@ -2244,14 +2024,6 @@ describe("brief re-dispatch ledger (CL-4343 / CL-5203)", () => { }); test("CL-6704: a successful Summary containing forced-stop phrases is not classified as a salvage", () => { - const noProgressPhrase = formatSubAgentReport({ - summary: "Investigated the flaky test; root cause is a race, not no progress on our side.", - findings: "Fixed the race in retry logic.", - blockers: "None", - paths: "src/retry.ts", - }); - expect(classifyBriefSalvage(noProgressPhrase)).toBeNull(); - const cancelledPhrase = formatSubAgentReport({ summary: "Implemented the cancelled-order refund flow end to end.", findings: "Added refund handler and tests.", @@ -2270,7 +2042,7 @@ describe("brief re-dispatch ledger (CL-4343 / CL-5203)", () => { }); test("CL-6704: true forced-stop Summary strings still classify as their salvage kind", () => { - expect(classifyBriefSalvage(forcedStopReport("no-progress", "x"))).toBe("no-progress"); + expect(classifyBriefSalvage(forcedStopReport("repetition", "x"))).toBe("repetition"); expect(classifyBriefSalvage(forcedStopReport("cancelled", "x"))).toBe("cancelled"); expect(classifyBriefSalvage(forcedStopReport("stalled", "x"))).toBe("stalled"); expect(classifyBriefSalvage(forcedStopReport("deadline", "x"))).toBe("deadline"); @@ -2288,8 +2060,8 @@ describe("brief re-dispatch ledger (CL-4343 / CL-5203)", () => { expect(third).toContain(TURN_BUDGET_STOP_PARENT_HINT.slice(1, 40)); }); - test("createTaskTool refuses identical re-dispatch after no-progress salvage", async () => { - const thrash = forcedStopReport("no-progress", "Repeated the same call"); + test("createTaskTool admits an identical re-dispatch after a repetition salvage (CL-6994)", async () => { + const thrash = forcedStopReport("repetition", "Looped the same output window"); let runs = 0; const sessions = createSubAgentSessionStore(); const tool = createTaskTool({ @@ -2309,33 +2081,15 @@ describe("brief re-dispatch ledger (CL-4343 / CL-5203)", () => { intent: "implement", }; const first = await callTask(tool, args); - expect(first).toContain("no progress"); - expect(first).toContain("identical brief"); + expect(first).toContain("degenerate repetition"); expect(runs).toBe(1); expect(sessions.list().filter((s) => s.status === "running")).toHaveLength(0); + // No salvage class refuses an identical re-dispatch any more. const second = await callTask(tool, args); - expect(second).toContain("refused re-dispatch"); - expect(second).toContain("no-progress"); - expect(runs).toBe(1); - // Refuse must not leave a ghost running session on the Agents strip. - expect(sessions.list().filter((s) => s.status === "running")).toHaveLength(0); - expect(sessions.list().filter((s) => s.description === "Thrash job")).toHaveLength(1); - - // maxTurns alone does not unlock - const bumped = await callTask(tool, { ...args, maxTurns: 99 }); - expect(bumped).toContain("refused re-dispatch"); - expect(runs).toBe(1); - expect(sessions.list().filter((s) => s.status === "running")).toHaveLength(0); - - // Changed brief is allowed - const third = await callTask(tool, { - ...args, - prompt: "do the thrashy work with a narrower scope", - success_criteria: ["one file"], - }); - expect(third).toContain("no progress"); + expect(second).toContain("degenerate repetition"); expect(runs).toBe(2); + expect(sessions.list().filter((s) => s.status === "running")).toHaveLength(0); }); test("createTaskTool flips turn-budget hint on third same-brief dispatch", async () => { diff --git a/src/subagent/index.ts b/src/subagent/index.ts index 5d4c79096..43ad3fe8f 100644 --- a/src/subagent/index.ts +++ b/src/subagent/index.ts @@ -45,11 +45,8 @@ export { type TaskIntent, } from "./report.js"; export { - DEFAULT_SUBAGENT_REPEAT_LIMIT, SUBAGENT_DEADLINE_MARGIN_MS, appendDeadlineParentHint, - appendNeverActedParentHint, - appendNoProgressParentHint, appendRepetitionParentHint, appendSubAgentParentHints, appendTurnBudgetParentHint, @@ -57,23 +54,17 @@ export { fingerprintToolCalls, forcedStopReport, isDeadlineSubAgentReport, - isNeverActedSubAgentReport, - isNeverEditedSubAgentReport, - isNoProgressSubAgentReport, isRepetitionSubAgentReport, isTurnBudgetSubAgentReport, - nextToolCallStreak, partialTextFromEvent, preferCompletedSubAgentReply, resolveSubAgentCatchOutcome, resolveSubAgentDeadlineMs, - subAgentNoProgress, subAgentTurnLimitExceeded, TURN_BUDGET_STOP_PARENT_HINT, type SubAgentCatchOutcome, type SubAgentParentHintOptions, type SubAgentStopReason, - type ToolCallStreak, } from "./stop-policy.js"; export { @@ -81,12 +72,10 @@ export { classifyBriefSalvage, createBriefDispatchLedger, fingerprintTaskBrief, - isHardBlockSalvage, shouldStopTurnBudgetRedispatch, type BriefDispatchLedger, type BriefDispatchRecord, type BriefSalvageKind, - type HardBlockSalvage, type TaskBriefFingerprintInput, } from "./brief-dispatch.js"; diff --git a/src/subagent/nudge-director.test.ts b/src/subagent/nudge-director.test.ts index f6c804e9a..4407ff2d5 100644 --- a/src/subagent/nudge-director.test.ts +++ b/src/subagent/nudge-director.test.ts @@ -461,19 +461,20 @@ describe("SubAgentDirector incomplete-report wiring", () => { expect(reply.content).not.toContain("narrated instead of writing a report envelope"); }); - test("zero-tool first turn still salvages never-acted", async () => { + test("zero-tool first turn without an envelope gets the incomplete-report nudge, not a hard stop (CL-6994)", async () => { + // A run that never called a tool is no longer a distinct failure — it is + // just a tool-less turn, judged the same way any other one is: nudged + // once for a missing envelope rather than immediately salvaged. const director = new SubAgentDirector("system", [], undefined, 30); const caps = capabilities(); const result = actions( await director.decide(inferenceDoneText("I'll write the red tests next"), state, caps), ); - expect(result.some((action) => action.type === "infer")).toBe(false); - expect(result).toContainEqual({ type: "checkpoint", message: "subagent-never-acted" }); - const reply = result.find((action) => action.type === "reply"); - expect(reply).toBeDefined(); - if (reply === undefined || reply.type !== "reply") throw new Error("expected reply action"); - expect(reply.content).toContain("without using any tools"); - expect(reply.content).not.toContain("narrated instead of writing a report envelope"); + expect(result.some((action) => action.type === "infer")).toBe(true); + expect(result).toContainEqual({ + type: "checkpoint", + message: "subagent-incomplete-report-nudge", + }); }); }); diff --git a/src/subagent/nudge-director.ts b/src/subagent/nudge-director.ts index 0ec5cde09..7f0828ef1 100644 --- a/src/subagent/nudge-director.ts +++ b/src/subagent/nudge-director.ts @@ -23,13 +23,10 @@ import { } from "./thrash.js"; import { NOOP_INTERVENTION_SINK, type InterventionSink } from "./intervention-log.js"; import { - DEFAULT_SUBAGENT_REPEAT_LIMIT, evaluateSubAgentStop, fingerprintToolCalls, forcedStopReport, lastText, - nextToolCallStreak, - type ToolCallStreak, } from "./stop-policy.js"; const REPORT_FORCED_WRAP_UP_NUDGE = @@ -78,17 +75,9 @@ function withEphemeralNudge( export class SubAgentDirector extends DefaultDirector { private readonly compaction: CompactionGovernor; private readonly maxTurns: number; - private readonly repeatLimit: number; - /** When true (intent=implement), tool-less finish without edits salvages as never-edited. */ - private readonly requireEdit: boolean; /** When true (CritiqueDirector), empty readCounts is not a successful complete. */ private readonly requireEvidence: boolean; private turnsCompleted = 0; - private everHadToolCalls = false; - private streak: ToolCallStreak = { - lastFingerprint: undefined, - consecutiveIdentical: 0, - }; private thrashState: ThrashState = EMPTY_THRASH_STATE; // Armed for wrap-up (report-forced) or failed-tool recovery so the // follow-up infer (after pending tool calls from THIS turn have executed) @@ -115,9 +104,9 @@ export class SubAgentDirector extends DefaultDirector { // (directors are pure decide(event, ...) functions — see requestContinuation // above), so the run loop periodically pings this same continuation channel // and the director only acts on a ping if genuinely nothing happened since - // the last one. Precedence: this check sits below no-progress / - // turn-budget (evaluateSubAgentStop, above) — those fire from real - // inference.done turns and always take priority; stall pings only ever + // the last one. Precedence: this check sits below turn-budget + // (evaluateSubAgentStop, above) — that fires from real inference.done + // turns and always takes priority; stall pings only ever // fire on a continuation message that inference.done/tool.done handling // did not already consume this cycle. private readonly stallTimeoutMs: number | undefined; @@ -157,20 +146,16 @@ export class SubAgentDirector extends DefaultDirector { toolDefinitions: ToolDefinition[], requestContinuation: (() => void) | undefined, maxTurns: number, - repeatLimit: number = DEFAULT_SUBAGENT_REPEAT_LIMIT, stallTimeoutMs?: number, now: () => number = Date.now, - requireEdit = false, requireEvidence = false, ) { super(systemPrompt, toolDefinitions, {}); this.compaction = createCompactionGovernor(requestContinuation, systemPrompt, toolDefinitions); this.maxTurns = maxTurns; - this.repeatLimit = repeatLimit; this.stallTimeoutMs = stallTimeoutMs; this.now = now; this.lastActivityAt = now(); - this.requireEdit = requireEdit; this.requireEvidence = requireEvidence; } @@ -223,22 +208,16 @@ export class SubAgentDirector extends DefaultDirector { }[]; this.lastAssistantText = lastText(content); const fingerprint = fingerprintToolCalls(content); - this.streak = nextToolCallStreak(this.streak, fingerprint); const hasToolCalls = fingerprint !== null; if (hasToolCalls) { - this.everHadToolCalls = true; this.thrashState = nextThrashState(this.thrashState, content); } const stop = evaluateSubAgentStop({ hasToolCalls, - everHadToolCalls: this.everHadToolCalls, turnsCompleted: this.turnsCompleted, maxTurns: this.maxTurns, - consecutiveIdentical: this.streak.consecutiveIdentical, - repeatLimit: this.repeatLimit, thrashState: this.thrashState, - requireEdit: this.requireEdit, requireEvidence: this.requireEvidence, lastAssistantText: this.lastAssistantText, incompleteReportNudgeFired: this.incompleteReportNudgeFired, @@ -301,42 +280,21 @@ export class SubAgentDirector extends DefaultDirector { }, state: this.interventionState(), }); - } else if ( - stop === "no-progress" || - stop === "turn-budget" || - stop === "never-acted" || - stop === "never-edited" - ) { - const checkpoint = - stop === "no-progress" - ? "subagent-no-progress" - : stop === "never-acted" - ? "subagent-never-acted" - : stop === "never-edited" - ? "subagent-never-edited" - : "subagent-turn-budget"; - const detail = - stop === "no-progress" - ? `identical tool call × ${this.streak.consecutiveIdentical}` - : stop === "turn-budget" - ? `${this.turnsCompleted}/${this.maxTurns} turns` - : undefined; + } else if (stop === "turn-budget") { + const detail = `${this.turnsCompleted}/${this.maxTurns} turns`; this.interventions({ id: stop, class: "stop", - measurement: - stop === "no-progress" - ? { - metric: "consecutiveIdentical", - value: this.streak.consecutiveIdentical, - threshold: this.repeatLimit, - } - : { metric: "turnsCompleted", value: this.turnsCompleted, threshold: this.maxTurns }, + measurement: { + metric: "turnsCompleted", + value: this.turnsCompleted, + threshold: this.maxTurns, + }, state: this.interventionState(), - ...(detail !== undefined ? { detail } : {}), + detail, }); const terminal: ReactorAction[] = [ - capabilities.checkpoint(checkpoint), + capabilities.checkpoint("subagent-turn-budget"), capabilities.reply(forcedStopReport(stop, lastText(content), detail)), ]; this.compaction.noteIdleTurn(event, terminal); @@ -377,7 +335,7 @@ export class SubAgentDirector extends DefaultDirector { * First stall past the timeout: one continuation nudge, asking the leaf to * report status or keep going. A second consecutive stall (no activity * since the nudge) escalates to the existing salvage path, same shape as - * no-progress/turn-budget above. Returns null when this event is not + * turn-budget above. Returns null when this event is not * a stall check the director should act on (let it fall through as an * ordinary continuation). */ diff --git a/src/subagent/run.ts b/src/subagent/run.ts index 8ad655c70..89021bc5f 100644 --- a/src/subagent/run.ts +++ b/src/subagent/run.ts @@ -96,7 +96,6 @@ import { subAgentToolName, } from "./report.js"; import { - DEFAULT_SUBAGENT_REPEAT_LIMIT, forcedStopReport, partialTextFromEvent, preferCompletedSubAgentReply, @@ -636,10 +635,8 @@ export async function runSubAgent(params: RunSubAgentParams): Promise { }), requestContinuation, maxTurns, - DEFAULT_SUBAGENT_REPEAT_LIMIT, modelFamilyPolicy.subAgentStallTimeoutMs, Date.now, - params.intent === "implement", shouldRequireEvidence(params), ); director.observeInterventions((event) => { diff --git a/src/subagent/shell-evidence.test.ts b/src/subagent/shell-evidence.test.ts index 8f9848331..2f15e778d 100644 --- a/src/subagent/shell-evidence.test.ts +++ b/src/subagent/shell-evidence.test.ts @@ -3,24 +3,11 @@ import { describe, expect, test } from "bun:test"; import { classifyShellFileEvidence } from "./shell-evidence.js"; describe("classifyShellFileEvidence (CL-6937)", () => { - test("in-place editors count as writes", () => { - expect(classifyShellFileEvidence("sed -i '' 's/a/b/' src/a.ts").writes).toContain("src/a.ts"); - expect(classifyShellFileEvidence("perl -pi -e 's/a/b/' src/b.ts").writes).toContain("src/b.ts"); - expect(classifyShellFileEvidence("sed -i.bak 's/a/b/' src/c.ts").writes).toContain("src/c.ts"); - }); - - test("sed without an in-place flag is a read, not a write", () => { + test("sed reads its file operand", () => { const evidence = classifyShellFileEvidence("sed -n '1,20p' src/a.ts"); - expect(evidence.writes).toEqual([]); expect(evidence.reads).toContain("src/a.ts"); }); - test("redirection is a write regardless of program", () => { - expect(classifyShellFileEvidence("echo hi > out.txt").writes).toContain("out.txt"); - expect(classifyShellFileEvidence("printf x >> out.txt").writes).toContain("out.txt"); - expect(classifyShellFileEvidence("cat <<'EOF' > gen.ts\nx\nEOF").writes).toContain("gen.ts"); - }); - test("readers count as reads with their file operand", () => { expect(classifyShellFileEvidence("cat src/a.ts").reads).toContain("src/a.ts"); expect(classifyShellFileEvidence("head -n 5 src/a.ts").reads).toContain("src/a.ts"); @@ -32,20 +19,17 @@ describe("classifyShellFileEvidence (CL-6937)", () => { }); test("wrapped payloads are inspected, not trusted", () => { - expect(classifyShellFileEvidence("bash -c \"sed -i '' s/a/b/ src/a.ts\"").writes).toContain( - "src/a.ts", - ); + expect(classifyShellFileEvidence('bash -c "cat src/a.ts"').reads).toContain("src/a.ts"); }); - test("chained commands contribute both sides", () => { - const evidence = classifyShellFileEvidence("cat src/a.ts && tee src/b.ts < src/a.ts"); + test("chained commands contribute reads from both sides", () => { + const evidence = classifyShellFileEvidence("cat src/a.ts && cat src/b.ts"); expect(evidence.reads).toContain("src/a.ts"); - expect(evidence.writes).toContain("src/b.ts"); + expect(evidence.reads).toContain("src/b.ts"); }); test("commands that touch no files yield nothing", () => { const evidence = classifyShellFileEvidence("bun run check"); expect(evidence.reads).toEqual([]); - expect(evidence.writes).toEqual([]); }); }); diff --git a/src/subagent/shell-evidence.ts b/src/subagent/shell-evidence.ts index 9fa58d783..f884632d8 100644 --- a/src/subagent/shell-evidence.ts +++ b/src/subagent/shell-evidence.ts @@ -1,11 +1,11 @@ // --- Shell file evidence (CL-6937) ----------------------------------------- // -// The stop policy measures whether a worker did real work by counting typed -// tool calls. Work done through run_shell was invisible to it, so a worker that -// edited with `sed -i` salvaged as never-edited (a sticky hard block) and one -// that read with `cat` salvaged as incomplete-report. The prompt does prohibit -// shell file work, but a prompt violation should produce a correction, not a -// verdict that the work never happened. +// The stop policy's requireEvidence gate (CritiqueDirector) measures whether +// a worker read anything by counting typed tool calls. Work done through +// run_shell was invisible to it, so a worker that read with `cat`/`rg` +// salvaged as incomplete-report. The prompt does prohibit shell file work, +// but a prompt violation should produce a correction, not a verdict that the +// work never happened. // // This reuses the same subject expansion the auto-shell policy uses, so // `bash -c`, `env -S`, and xargs payloads are inspected rather than trusted. @@ -49,30 +49,9 @@ const SHELL_READ_PROGRAMS: ReadonlySet = new Set([ "ls", ]); -/** Programs whose ordinary use rewrites a file operand in place. */ -const SHELL_WRITE_PROGRAMS: ReadonlySet = new Set([ - "tee", - "cp", - "mv", - "install", - "touch", - "truncate", - "patch", - "ln", -]); - -/** In-place editors: only a write when the in-place flag is actually present. */ -const SHELL_IN_PLACE_PROGRAMS: ReadonlySet = new Set(["sed", "perl", "ruby", "gsed"]); - -const IN_PLACE_FLAG = /^-{1,2}(i|in-place)(=.*)?$/; -/** `sed -i.bak`, `perl -pi -e`, `sed -Ei` — the flag is fused with other letters. */ -const FUSED_IN_PLACE_FLAG = /^-[A-Za-z]*i/; - export interface ShellFileEvidence { /** Keys for paths (or programs) the command read. */ reads: string[]; - /** Keys for paths (or programs) the command wrote. */ - writes: string[]; } function evidenceKey(program: string, operand: string | undefined): string { @@ -81,7 +60,7 @@ function evidenceKey(program: string, operand: string | undefined): string { /** * Flags whose value is a separate token, so `head -n 5 f` does not read "5". - * Union of the reader flag sets above plus the common in-place/script ones. + * Union of the reader flag sets above plus the common script ones. */ const EVIDENCE_VALUE_FLAGS: ReadonlySet = new Set([ ...HEAD_TAIL_VALUE_FLAGS, @@ -114,53 +93,13 @@ function firstOperand(args: readonly string[], skip: number): string | undefined return undefined; } -/** A redirect target is one word: heredoc bodies arrive in the same string. */ -function redirectTarget(raw: string): string | undefined { - const word = raw.replace(/['"]/g, "").trim().split(/\s/)[0]; - return word !== undefined && word.length > 0 ? word : undefined; -} - function classifySegment(segment: string, evidence: ShellFileEvidence): void { const tokens = tokenizeSegment(segment); if (tokens.length === 0) return; - // Output redirection is a write regardless of the program: `echo x > f`, - // heredocs (`cat <<'EOF' > f`), `>>` appends. - for (let i = 0; i < tokens.length; i++) { - const token = tokens[i]!; - const match = /^>{1,2}$/.exec(token); - if (match !== null) { - const target = tokens[i + 1]; - const named = target === undefined ? undefined : redirectTarget(target); - if (named !== undefined) evidence.writes.push(named); - continue; - } - const fused = /^>{1,2}(?!$)(.+)$/.exec(token); - if (fused !== null) { - const named = redirectTarget(fused[1]!); - if (named !== undefined) evidence.writes.push(named); - } - } - const program = programBasename(tokens[0]!); const args = tokens.slice(1); - if (SHELL_IN_PLACE_PROGRAMS.has(program)) { - const inPlace = args.some( - (arg) => IN_PLACE_FLAG.test(arg) || (arg.startsWith("-") && FUSED_IN_PLACE_FLAG.test(arg)), - ); - if (inPlace) { - // sed/perl take the script before the file operand, unless -e already - // consumed it (`perl -pi -e 's/a/b/' f`). - const scriptInFlag = args.some((arg) => arg === "-e" || arg === "--expression"); - evidence.writes.push(evidenceKey(program, firstOperand(args, scriptInFlag ? 0 : 1))); - return; - } - } - if (SHELL_WRITE_PROGRAMS.has(program)) { - evidence.writes.push(evidenceKey(program, firstOperand(args, 0))); - return; - } if (SHELL_READ_PROGRAMS.has(program)) { // grep-likes take the pattern first, so their file operand is the second. const skip = program === "grep" || program === "egrep" || program === "fgrep" ? 1 : 0; @@ -169,14 +108,12 @@ function classifySegment(segment: string, evidence: ShellFileEvidence): void { } /** - * Reads and writes a run_shell command performs on files, for the stop policy's - * requireEdit / requireEvidence checks. Best effort by design: a missed read - * costs a worker nothing (the typed tools remain the primary evidence), while a - * missed write is exactly the false salvage this exists to prevent, so writes - * are recognized from redirection as well as from the program name. + * Reads a run_shell command performs on files, for the stop policy's + * requireEvidence check. Best effort by design: a missed read costs a + * worker nothing (the typed tools remain the primary evidence). */ export function classifyShellFileEvidence(command: string): ShellFileEvidence { - const evidence: ShellFileEvidence = { reads: [], writes: [] }; + const evidence: ShellFileEvidence = { reads: [] }; const { subjects } = expandShellSubjects(command); for (const subject of subjects) { for (const segment of splitChainedCommand(subject)) { @@ -185,6 +122,5 @@ export function classifyShellFileEvidence(command: string): ShellFileEvidence { } return { reads: [...new Set(evidence.reads)], - writes: [...new Set(evidence.writes)], }; } diff --git a/src/subagent/stop-policy.ts b/src/subagent/stop-policy.ts index 193183d8f..00f3d6c61 100644 --- a/src/subagent/stop-policy.ts +++ b/src/subagent/stop-policy.ts @@ -1,6 +1,6 @@ /** - * Pure stop / salvage policy for leaf sub-agents: turn budget, no-progress, - * thrash, deadlines, and parent-facing salvage reports. + * Pure stop / salvage policy for leaf sub-agents: turn budget, thrash, + * deadlines, and parent-facing salvage reports. */ import type { ReactorEmittedEvent } from "@intx/inference"; @@ -14,15 +14,6 @@ import { parseSubAgentReport, } from "./report.js"; -// Consecutive identical tool-call fingerprints before a leaf is forced to -// stop. Mirrors IDENTICAL_REPEAT_MIN below (the director-level period-1 -// thrash threshold): the same forensic scan found zero occurrences of even -// two consecutive identical fingerprints in local trace history, and CL-5611 -// found the previous 4-repeat hard pause false-positived on legitimate -// polling (rerunning a flaky test, polling a build) — hence a threshold set -// above 4, not at 2. -export const DEFAULT_SUBAGENT_REPEAT_LIMIT = 5; - // Minimum gap kept between an opt-in internal deadline and the outer // tool-execution watchdog, so there is time left for the salvage report to // unwind and return before the outer watchdog would discard the run wholesale. @@ -93,10 +84,6 @@ export function subAgentTurnLimitExceeded(turnsCompleted: number, maxTurns: numb return turnsCompleted >= maxTurns; } -export function subAgentNoProgress(consecutiveIdentical: number, repeatLimit: number): boolean { - return consecutiveIdentical >= repeatLimit; -} - // Stable JSON so key insertion order does not create false progress between turns. function stableJson(value: unknown): string { if (value === null || typeof value !== "object") return JSON.stringify(value); @@ -261,51 +248,30 @@ export function detectTurnsSinceUserMessageBackstop(turnsSinceUserMessage: numbe export const TOOL_FINGERPRINT_HISTORY_CAP = TOOL_FINGERPRINT_MAX_PERIOD * IDENTICAL_REPEAT_MIN; export type SubAgentStopReason = - | "complete" - | "turn-budget" - | "no-progress" - | "never-acted" - | "never-edited" - | "report-forced" - | "incomplete-report" - | "incomplete-report-stop"; + "complete" | "turn-budget" | "report-forced" | "incomplete-report" | "incomplete-report-stop"; /** * Pure stop decision for leaf workers. Null means keep running tools. * - * Precedence when tools are still firing: - * no-progress (identical fingerprints) > turn-budget (hard cap). - * Look volume never hard-stops. - * "report-forced" and "incomplete-report" - * are not competing stop reasons — they are one-shot signals telling the - * caller to inject a wrap-up / redirect nudge and keep running; turn-budget - * remains reachable afterward. Tool-less turns end as never-acted - * or never-edited when those apply; otherwise a tool-less turn after tools - * completes only when the assistant text has a four-heading envelope - * (Summary, Findings, Blockers, Paths). Omitting `lastAssistantText` - * still completes (back-compat). Missing envelope nudges - * once (`incomplete-report`) then salvages (`incomplete-report-stop`). - * When `requireEvidence` is set (CritiqueDirector), an empty `readCounts` - * is not complete even with all four headings — same incomplete-report - * nudge then salvage, so a wrap-up envelope cannot fake a real review. + * "report-forced" and "incomplete-report" are not competing stop reasons — + * they are one-shot signals telling the caller to inject a wrap-up / redirect + * nudge and keep running; turn-budget remains reachable afterward. A + * tool-less turn (including one that never called a tool at all) completes + * only when the assistant text has a four-heading envelope (Summary, + * Findings, Blockers, Paths). Omitting `lastAssistantText` still completes + * (back-compat). Missing envelope nudges once (`incomplete-report`) then + * salvages (`incomplete-report-stop`). When `requireEvidence` is set + * (CritiqueDirector), an empty `readCounts` is not complete even with all + * four headings — same incomplete-report nudge then salvage, so a wrap-up + * envelope cannot fake a real review. */ export function evaluateSubAgentStop(input: { hasToolCalls: boolean; - /** True when any turn in this run (including the current one) issued tools. */ - everHadToolCalls: boolean; turnsCompleted: number; maxTurns: number; - consecutiveIdentical: number; - repeatLimit: number; - /** When set, the near-budget force-report nudge is evaluated after no-progress. */ + /** When set, the near-budget force-report nudge is evaluated. */ thrashState?: ThrashState; thrashConfig?: Partial; - /** - * When true (intent=implement), a tool-using run that never wrote/edited a - * file is not a successful complete — salvage as never-edited so the parent - * does not treat a pure-explore "plan" as shipped work. - */ - requireEdit?: boolean; /** * When true (CritiqueDirector leaf), a tool-using run that never * read or searched a file is not a successful complete — even a four-heading @@ -323,20 +289,11 @@ export function evaluateSubAgentStop(input: { /** True after the one-shot incomplete-report wrap-up nudge has been injected. */ incompleteReportNudgeFired?: boolean; }): SubAgentStopReason | null { - // Planning-only prose is never-acted; implement intent that only - // read/searched (no edit_file/write_file/delete_file) is never-edited — - // both hard-block identical re-dispatch. After those, a tool-less turn - // following tools is complete only with a report envelope (or when - // lastAssistantText is omitted). CritiqueDirector additionally requires - // at least one read/search in thrashState.readCounts. + // A tool-less turn is complete only with a report envelope (or when + // lastAssistantText is omitted). CritiqueDirector additionally requires at + // least one read/search in thrashState.readCounts. Neither zero tool calls + // nor tool calls that left no net edit are treated as a failure here. if (!input.hasToolCalls) { - if (!input.everHadToolCalls) return "never-acted"; - if ( - input.requireEdit === true && - (input.thrashState === undefined || input.thrashState.editedPaths.size === 0) - ) { - return "never-edited"; - } if (input.lastAssistantText !== undefined && !hasReportEnvelope(input.lastAssistantText)) { return input.incompleteReportNudgeFired === true ? "incomplete-report-stop" @@ -352,8 +309,6 @@ export function evaluateSubAgentStop(input: { } return "complete"; } - // No-progress is more specific than the turn budget when both could apply. - if (subAgentNoProgress(input.consecutiveIdentical, input.repeatLimit)) return "no-progress"; if (input.thrashState !== undefined) { const thrashStop = evaluateThrashStop({ hasToolCalls: true, @@ -368,39 +323,15 @@ export function evaluateSubAgentStop(input: { return null; } -export interface ToolCallStreak { - lastFingerprint: string | undefined; - consecutiveIdentical: number; -} - -/** Advance consecutive-identical bookkeeping for one inference.done turn. */ -export function nextToolCallStreak( - prev: ToolCallStreak, - fingerprint: string | null, -): ToolCallStreak { - if (fingerprint === null) { - return { lastFingerprint: undefined, consecutiveIdentical: 0 }; - } - if (fingerprint === prev.lastFingerprint) { - return { - lastFingerprint: fingerprint, - consecutiveIdentical: prev.consecutiveIdentical + 1, - }; - } - return { lastFingerprint: fingerprint, consecutiveIdentical: 1 }; -} - // A sub-agent is a worker, not a chat partner: it runs until it stops calling // tools, at which point its final assistant text is the result handed back to -// the dispatcher — unless it never called tools at all, in which case the -// result is a never-acted salvage report rather than a successful implement. -// It has no submit_output or ask_operator; consequential tools still go through -// the parent's permission gate (grants, auto mode, or prompts). Hard stops also -// fire on identical tool fingerprints (no-progress) and the hard turn budget -// so a looping leaf cannot burn the full budget -// with no parent-visible report. Near the budget the leaf gets a one-shot -// wrap-up nudge (report-forced) rather than a stop, so turn-budget stays -// reachable for a leaf that is genuinely still making progress. +// the dispatcher. It has no submit_output or ask_operator; consequential +// tools still go through the parent's permission gate (grants, auto mode, or +// prompts). The hard turn budget still fires so a looping leaf cannot burn +// the full budget with no parent-visible report. Near the budget the leaf +// gets a one-shot wrap-up nudge (report-forced) rather than a stop, so +// turn-budget stays reachable for a leaf that is genuinely still making +// progress. export function lastText(content: readonly { type: string }[]): string { for (let i = content.length - 1; i >= 0; i--) { @@ -424,16 +355,7 @@ export function partialTextFromEvent(event: ReactorEmittedEvent): string | null } export type ForcedStopReason = - | "no-progress" - | "turn-budget" - | "never-acted" - | "never-edited" - | "cancelled" - | "deadline" - | "no-ship" - | "stalled" - | "repetition" - | "incomplete-report"; + "turn-budget" | "cancelled" | "deadline" | "stalled" | "repetition" | "incomplete-report"; // Exact Summary text for each forced-stop reason. This is the single source // of truth for both forcedStopReport (the producer) and the isXxxSubAgentReport @@ -443,10 +365,6 @@ export type ForcedStopReason = // stop actually produces closes that false-positive path without a report // schema change (a typed marker would need one; see CL-6786, out of scope). const FORCED_STOP_SUMMARIES: Record = { - "no-progress": "Stopped: repeated the same tool calls with no progress.", - "no-ship": "Stopped: implement intent searched many files without writing any.", - "never-acted": "Stopped: completed without using any tools.", - "never-edited": "Stopped: implement intent finished without writing any files.", cancelled: "Stopped: cancelled by operator before finishing.", deadline: "Stopped: wall-clock deadline reached before finishing.", stalled: @@ -471,29 +389,20 @@ export function forcedStopReport( ): string { const summary = FORCED_STOP_SUMMARIES[reason]; const blockers = - reason === "no-progress" - ? "Identical tool-call fingerprint repeated consecutively; parent must not re-dispatch the identical brief (it will be refused) — tighten success_criteria/do_not or change approach." - : reason === "no-ship" - ? "Implement searched many files without writing any; parent must not re-dispatch the identical brief (it will be refused) — re-dispatch with an edit-first brief, tighter success_criteria, and do_not. Do not search the repo yourself first." - : reason === "never-acted" - ? "Worker returned planning/prose only (zero tool calls in the run); parent must not re-dispatch the identical brief (it will be refused) — re-dispatch only with a tighter brief, or treat findings as unexecuted." - : reason === "never-edited" - ? "Worker used tools but never called edit_file/write_file/delete_file under intent=implement; parent must not re-dispatch the identical brief (it will be refused) — re-dispatch with an edit-first brief, or treat findings as unexecuted." - : reason === "cancelled" - ? "Operator or parent cancelled the worker mid-run; parent may re-dispatch with the partial findings below." - : reason === "deadline" - ? "Worker wall-clock deadline elapsed mid-run; parent may re-dispatch with a longer deadline or a narrower scope for the remaining work." - : reason === "stalled" - ? "Worker went quiet (e.g. parked on a long-running background command) past the stall timeout after an initial nudge; parent may re-dispatch to finish or check on the background work directly." - : reason === "repetition" - ? "The model looped the same output window mid-stream; the tail of the loop is in Findings. Re-dispatching the identical brief will be refused and would likely loop again — change prompt/intent/success_criteria/do_not/agent, not maxTurns alone." - : reason === "incomplete-report" - ? "Worker ended a tool-using run with a tool-less turn that had no four-heading report envelope (Summary/Findings/Blockers/Paths) after a wrap-up nudge. Findings below are the narration, not a structured report." - : "Worker turn budget exhausted; parent may re-dispatch for remaining work."; + reason === "cancelled" + ? "Operator or parent cancelled the worker mid-run; parent may re-dispatch with the partial findings below." + : reason === "deadline" + ? "Worker wall-clock deadline elapsed mid-run; parent may re-dispatch with a longer deadline or a narrower scope for the remaining work." + : reason === "stalled" + ? "Worker went quiet (e.g. parked on a long-running background command) past the stall timeout after an initial nudge; parent may re-dispatch to finish or check on the background work directly." + : reason === "repetition" + ? "The model looped the same output window mid-stream; the tail of the loop is in Findings. Re-dispatch with a changed prompt/intent/success_criteria/do_not/agent if it would likely loop again — maxTurns alone will not help." + : reason === "incomplete-report" + ? "Worker ended a tool-using run with a tool-less turn that had no four-heading report envelope (Summary/Findings/Blockers/Paths) after a wrap-up nudge. Findings below are the narration, not a structured report." + : "Worker turn budget exhausted; parent may re-dispatch for remaining work."; // Demote nested report-section headings so runSubAgent's parse/format pass // cannot clobber this outer Summary/Blockers with an agent-shaped envelope - // stuffed into Findings (never-acted planning envelopes; cancel after a - // structured partial). + // stuffed into Findings (cancel after a structured partial). const findings = partialText.trim().length > 0 ? demoteNestedReportHeadings(partialText.trim()) @@ -523,16 +432,6 @@ export function isTurnBudgetSubAgentReport(report: string): boolean { return isForcedStopSubAgentReport(report, "turn-budget"); } -/** True when the worker returned a never-acted salvage report for the parent. */ -export function isNeverActedSubAgentReport(report: string): boolean { - return isForcedStopSubAgentReport(report, "never-acted"); -} - -/** True when implement intent finished without any write/edit tools. */ -export function isNeverEditedSubAgentReport(report: string): boolean { - return isForcedStopSubAgentReport(report, "never-edited"); -} - /** True when the worker returned a deadline salvage report for the parent. */ export function isDeadlineSubAgentReport(report: string): boolean { return isForcedStopSubAgentReport(report, "deadline"); @@ -550,23 +449,11 @@ const TURN_BUDGET_PARENT_HINT = export const TURN_BUDGET_STOP_PARENT_HINT = "[Sub-agent hit its turn budget again on the same brief (re-dispatch cap). Stop raising maxTurns on this fingerprint — restate the task, change approach (intent / success_criteria / do_not / prompt / agent), or finish from Findings. Further identical dispatches are still admitted but will not invite more maxTurns bumps.]"; -const NEVER_ACTED_PARENT_HINT = - "[Sub-agent finished without using any tools (planning/prose only). Treat findings as unexecuted; re-dispatch with a tighter brief if the work still needs doing. An identical brief will be refused.]"; - -const NEVER_EDITED_PARENT_HINT = - "[Sub-agent finished implement intent without writing any files (read/search only). Treat findings as unexecuted; re-dispatch with an edit-first brief. An identical brief will be refused.]"; - const DEADLINE_PARENT_HINT = "[Sub-agent hit an explicit wall-clock deadline before finishing. Continue from Findings rather than redoing completed work; re-dispatch with continuation context and a longer deadline only if more wall-clock time is warranted.]"; -const NO_SHIP_PARENT_HINT = - "[Sub-agent stopped after searching many files without writing any. Do not search the repo yourself and do not re-dispatch the identical brief (it will be refused) — change success_criteria and do_not, or treat findings as unexecuted.]"; - const REPETITION_PARENT_HINT = - "[Sub-agent aborted after its streamed output degenerated into a loop. Do not re-dispatch the identical brief — it will be refused and would likely loop again; change prompt, intent, success_criteria, do_not, and/or agent (maxTurns alone does not change the fingerprint).]"; - -const NO_PROGRESS_PARENT_HINT = - "[Sub-agent stopped for no-progress (identical tool-call fingerprint). Do not re-dispatch the identical brief (it will be refused) — tighten success_criteria and do_not, or change approach.]"; + "[Sub-agent aborted after its streamed output degenerated into a loop. Re-dispatching unchanged would likely loop again; change prompt, intent, success_criteria, do_not, and/or agent (maxTurns alone does not fix it).]"; /** Options for parent-hint stacking (session re-dispatch ledger state). */ export interface SubAgentParentHintOptions { @@ -594,59 +481,22 @@ export function appendTurnBudgetParentHint( return `${hint}\n\n${report}`; } -export function appendNeverActedParentHint(report: string): string { - if (!isNeverActedSubAgentReport(report)) return report; - return `${NEVER_ACTED_PARENT_HINT}\n\n${report}`; -} - -export function appendNeverEditedParentHint(report: string): string { - if (!isNeverEditedSubAgentReport(report)) return report; - return `${NEVER_EDITED_PARENT_HINT}\n\n${report}`; -} - export function appendDeadlineParentHint(report: string): string { if (!isDeadlineSubAgentReport(report)) return report; return `${DEADLINE_PARENT_HINT}\n\n${report}`; } -/** True when the worker returned a no-ship (search-tour) salvage report. */ -export function isNoShipSubAgentReport(report: string): boolean { - return isForcedStopSubAgentReport(report, "no-ship"); -} - -export function appendNoShipParentHint(report: string): string { - if (!isNoShipSubAgentReport(report)) return report; - return `${NO_SHIP_PARENT_HINT}\n\n${report}`; -} - export function appendRepetitionParentHint(report: string): string { if (!isRepetitionSubAgentReport(report)) return report; return `${REPETITION_PARENT_HINT}\n\n${report}`; } -/** True when the worker returned a no-progress salvage report. */ -export function isNoProgressSubAgentReport(report: string): boolean { - return isForcedStopSubAgentReport(report, "no-progress"); -} - -export function appendNoProgressParentHint(report: string): string { - if (!isNoProgressSubAgentReport(report)) return report; - return `${NO_PROGRESS_PARENT_HINT}\n\n${report}`; -} - -/** Stack parent-visible salvage hints for budget / never-acted / deadline / repetition / no-progress. */ +/** Stack parent-visible salvage hints for turn-budget / deadline / repetition. */ export function appendSubAgentParentHints( report: string, options: SubAgentParentHintOptions = {}, ): string { return appendDeadlineParentHint( - appendNeverEditedParentHint( - appendNeverActedParentHint( - appendTurnBudgetParentHint( - appendNoProgressParentHint(appendNoShipParentHint(appendRepetitionParentHint(report))), - options, - ), - ), - ), + appendTurnBudgetParentHint(appendRepetitionParentHint(report), options), ); } diff --git a/src/subagent/task-tool.ts b/src/subagent/task-tool.ts index fa95ee4c8..934d21965 100644 --- a/src/subagent/task-tool.ts +++ b/src/subagent/task-tool.ts @@ -75,7 +75,7 @@ export const TaskToolArgs = type({ export const taskToolDefinition: ToolDefinition = { name: "task", description: - 'Spawn a sub-agent (a short-lived child agent) for one self-contained job. This is not a checklist item — use manage_tasks for your own work list. The sub-agent has the full file, search, and shell toolset, uses this session\'s permission gate (saved grants and auto mode when eligible; you may be prompted for other consequential actions), and returns a structured report (Summary / Findings / Blockers / Paths). Use it to parallelize exploration ("map every caller of X") or hand off a well-scoped implementation so your own context stays focused. Fire several task calls in one turn to run sub-agents in parallel. When launching multiple agents with the same profile, assign each a distinct lens in description and prompt so they do not duplicate work. The sub-agent cannot ask you questions. Depending on dispatch configuration it either shares your working tree directly, or runs isolated in its own git worktree snapshotted from your last commit — in the isolated case, any uncommitted or untracked changes in your working tree are excluded. Write a clear brief: context = durable background; prompt = actionable goal; goals = optional manage_tasks seeds. Prefer the typed spawn contract so workers finish without thrashing: intent (explore|implement|review|plan|general), success_criteria (done-when checklist), do_not (scope fence), report_focus (what Findings must cover). After no-progress / repetition / never-acted salvage, re-dispatching the identical brief (same prompt/agent/intent/success_criteria/do_not) is refused — change the brief to retry; maxTurns alone does not unlock it. Turn-budget salvage may invite a higher maxTurns a few times, then stops recommending re-dispatch until a successful complete resets the same-brief retry budget.', + 'Spawn a sub-agent (a short-lived child agent) for one self-contained job. This is not a checklist item — use manage_tasks for your own work list. The sub-agent has the full file, search, and shell toolset, uses this session\'s permission gate (saved grants and auto mode when eligible; you may be prompted for other consequential actions), and returns a structured report (Summary / Findings / Blockers / Paths). Use it to parallelize exploration ("map every caller of X") or hand off a well-scoped implementation so your own context stays focused. Fire several task calls in one turn to run sub-agents in parallel. When launching multiple agents with the same profile, assign each a distinct lens in description and prompt so they do not duplicate work. The sub-agent cannot ask you questions. Depending on dispatch configuration it either shares your working tree directly, or runs isolated in its own git worktree snapshotted from your last commit — in the isolated case, any uncommitted or untracked changes in your working tree are excluded. Write a clear brief: context = durable background; prompt = actionable goal; goals = optional manage_tasks seeds. Prefer the typed spawn contract so workers finish without thrashing: intent (explore|implement|review|plan|general), success_criteria (done-when checklist), do_not (scope fence), report_focus (what Findings must cover). After a repetition salvage, re-dispatching the identical brief (same prompt/agent/intent/success_criteria/do_not) unchanged will likely loop again — change the brief before retrying. Turn-budget salvage may invite a higher maxTurns a few times, then stops recommending re-dispatch until a successful complete resets the same-brief retry budget.', inputSchema: { type: "object", properties: { @@ -247,15 +247,6 @@ export function createTaskTool(deps: TaskToolDeps): AgentTool { const telemetry = deps.telemetry ?? NOOP_TELEMETRY; // Session-scoped re-dispatch ledger: one per parent task tool instance. const briefLedger = createBriefDispatchLedger(); - // A refused re-dispatch is the sharpest false-positive signal we have: the - // parent wanted this brief again and the harness said no on the strength of - // an earlier salvage classification (CL-6938). Logged on the parent side - // because no leaf run exists to log it. - let refusalLog: InterventionSink | null = null; - const recordRefusal = (event: Parameters[0]): void => { - refusalLog ??= createInterventionLog(deps.getWorkdirBase(), { role: "parent" }); - refusalLog(event); - }; // Every completed dispatch gets an outcome record — the log otherwise // carries shape and run state but never what the run actually produced. // Tagged with the dispatched child's provider/model/family (CL-6968) so @@ -618,14 +609,6 @@ export function createTaskTool(deps: TaskToolDeps): AgentTool { ...(doNot.length > 0 ? { doNot } : {}), }); const admission = briefLedger.admit(fingerprint); - if (!admission.ok) { - recordRefusal({ - id: "re-dispatch-refused", - class: "block", - detail: admission.message.slice(0, 300), - }); - return taskToolResult(call.id, admission.message); - } const dispatchCount = admission.dispatchCount; const agentLabel = diff --git a/src/subagent/thrash.test.ts b/src/subagent/thrash.test.ts index bb0258cc9..86a4fcf5f 100644 --- a/src/subagent/thrash.test.ts +++ b/src/subagent/thrash.test.ts @@ -126,18 +126,12 @@ describe("thrash pure module", () => { expect(state.totalToolCalls).toBe(1); }); - test("run_shell file work counts as read and edit evidence (CL-6937)", () => { + test("run_shell file reads count as read evidence (CL-6937)", () => { const shell = (command: string): ThrashToolCallBlock => ({ type: "tool_call", name: "run_shell", arguments: { command }, }); - const edited = applyAll([shell("sed -i '' 's/a/b/' src/a.ts")]); - expect(edited.editedPaths.has("src/a.ts")).toBe(true); - - const heredoc = applyAll([shell("cat <<'EOF' > src/gen.ts\nx\nEOF")]); - expect(heredoc.editedPaths.has("src/gen.ts")).toBe(true); - const readOnly = applyAll([shell("head -n 40 src/a.ts")]); expect(readOnly.readCounts.get("src/a.ts")).toBe(1); expect(readOnly.editedPaths.size).toBe(0); diff --git a/src/subagent/thrash.ts b/src/subagent/thrash.ts index a5eec9253..33a3f93fb 100644 --- a/src/subagent/thrash.ts +++ b/src/subagent/thrash.ts @@ -7,11 +7,13 @@ * evidence that they repeat, and a raw re-read count cannot tell four reads * spread across real progress from four reads in a loop (CL-6936). * - * The state this module accumulates is consumed by evaluateSubAgentStop's - * requireEdit / requireEvidence checks, not by a stop of its own. Reads and - * writes performed through run_shell count as evidence there (CL-6937) — the - * prompt prohibits shell file work, but a prompt violation deserves a - * correction, not a verdict that the work never happened. + * readCounts feeds evaluateSubAgentStop's requireEvidence check (the + * CritiqueDirector gate). Reads performed through run_shell count as evidence + * there too (CL-6937) — the prompt prohibits shell file work, but a prompt + * violation deserves a correction, not a verdict that the work never + * happened. editedPaths is recorded purely for intervention-log diagnostics + * (CL-6994 deleted the never-edited stop that used to consume it — no + * salvage class is gated on it). */ import { isProductMutationTool, productMutationPaths } from "../agent/product-mutation-tools.js"; @@ -131,8 +133,8 @@ export function nextThrashState( const key = searchKey(name, args); readCounts.set(key, (readCounts.get(key) ?? 0) + 1); } else if (name === SHELL_TOOL) { - // Shell file work is evidence too, or a worker that edits with sed -i - // salvages as never-edited and is then refused re-dispatch (CL-6937). + // Shell file reads are evidence too, or a worker that reads with cat/rg + // falsely fails the CritiqueDirector requireEvidence gate (CL-6937). const command = args.command; if (typeof command === "string" && command.length > 0) { const evidence = classifyShellFileEvidence(command); @@ -142,10 +144,6 @@ export function nextThrashState( readCounts.set(key, (readCounts.get(key) ?? 0) + 1); } } - if (evidence.writes.length > 0) { - if (editedPaths === null) editedPaths = new Set(prev.editedPaths); - for (const key of evidence.writes) editedPaths.add(key); - } } } else if (isProductMutationTool(name)) { const paths = productMutationPaths(name, args);