diff --git a/CHANGELOG.md b/CHANGELOG.md index 54c133503..38a4e8609 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,14 @@ parallel copies under `docs/` or `scripts/notes/`. At cut time: rename reports directly, not by re-parsing the parent-facing report's prose. Removes the `isXxxSubAgentReport` classifier family and per-reason parent hint functions in favor of a single structured switch. +- Removed the `never-edited`, `never-acted`, and `no-ship` sub-agent salvage + classes and the sticky hard-block that refused an identical re-dispatch + after one fired. `task` re-dispatch is never refused now; turn-budget + salvage still throttles repeated same-brief retries. Also removed the + now-dead shell-write half of the shell-evidence detector (read detection + for `requireEvidence` is unchanged) and the shell-write contribution to + `editedPaths` diagnostics. + ### Internal - Removed the dead Ink-era kill ring copy (`src/tui/kill-ring.ts`); the OpenTUI diff --git a/src/agent/director.ts b/src/agent/director.ts index e8e8c4e72..87b1e3cfa 100644 --- a/src/agent/director.ts +++ b/src/agent/director.ts @@ -21,10 +21,6 @@ import { isInternalRecoveryAbortRaw } from "../inference-abort.js"; import { LOG_NAMESPACE_ROOT } from "../branding.js"; import { resolveModelFamilyPolicy, type ModelFamilyPolicy } from "./model-family-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 type { ForcedStopReason } from "../subagent/stop-policy.js"; -import { PRIMARY_SALVAGE_NUDGE } from "./look-tour.js"; const logger = getLogger([LOG_NAMESPACE_ROOT, "agent", "director"]); @@ -394,10 +390,6 @@ class ChatDirectorImpl extends DefaultDirector { private toolOnlyStreak = 0; private toolOnlyNudgeFired = false; private pendingToolOnlyNudge = false; - // One-shot nudge after a hard-block worker salvage. Not a look-count quota. - private salvageNudgeFired = false; - private pendingSalvageNudge: string | null = null; - private pendingTaskCallIds = new Set(); constructor( systemPrompt: string, @@ -489,26 +481,6 @@ class ChatDirectorImpl extends DefaultDirector { return rewritten; } - /** - * One-shot salvage nudge after a worker hard-block. Fingerprint thrash - * (applyToolOnlyLoopProtection) wins when both apply. Attaches to the infer - * after pending tools have executed. - */ - private applySalvageNudge( - actions: ReactorAction[], - capabilities: ReactorCapabilities, - ): ReactorAction[] | null { - if (this.pendingSalvageNudge === null) return null; - const inferIndex = actions.findIndex((a) => a.type === "infer"); - if (inferIndex === -1) return null; - const text = this.pendingSalvageNudge; - this.pendingSalvageNudge = null; - const rewritten = [...actions]; - const existing = actions[inferIndex] as Extract; - rewritten[inferIndex] = inferWithNudge(capabilities, text, existing.options); - return rewritten; - } - private withCurrentTools( result: ReactorAction | ReactorAction[], ): ReactorAction | ReactorAction[] { @@ -637,15 +609,6 @@ class ChatDirectorImpl extends DefaultDirector { this.toolOnlyStreak = 0; this.toolOnlyNudgeFired = false; this.pendingToolOnlyNudge = false; - // Only a message carrying OPERATOR_ORIGINATED_FLAG resets the salvage - // nudge — not every message.received. Synthetic system sends - // (compaction continuations, retries, future director continuations) - // also fire message.received but are not a genuine operator checkpoint. - if (isOperatorOriginated(event.message.flags)) { - this.salvageNudgeFired = false; - this.pendingSalvageNudge = null; - this.pendingTaskCallIds.clear(); - } } if (onTurnBoundary(event)) this.inferenceRecoveries = 0; @@ -700,16 +663,6 @@ class ChatDirectorImpl extends DefaultDirector { // toolOnlyStreak is narration-sensitive: any turn with text clears it // (same as a fresh user message), and it only drives the soft // check-in nudge at toolOnlyTurnNudgeAt, never a stop. - const turnContent = event.turn.content as readonly { - type: string; - name?: string; - id?: string; - }[]; - for (const block of turnContent) { - if (block.type === "tool_call" && block.name === "task" && typeof block.id === "string") { - this.pendingTaskCallIds.add(block.id); - } - } if (hasToolCalls && !hasText) { this.toolOnlyStreak++; } else { @@ -755,19 +708,6 @@ class ChatDirectorImpl extends DefaultDirector { } } - if (event.type === "tool.done" && this.pendingTaskCallIds.has(event.result.callId)) { - this.pendingTaskCallIds.delete(event.result.callId); - const detail = event.result.detail as { stopReason?: ForcedStopReason } | undefined; - const salvage = classifyBriefSalvage({ - ...(detail?.stopReason !== undefined ? { stopReason: detail.stopReason } : {}), - wasCancelled: false, - }); - if (salvage !== null && isHardBlockSalvage(salvage) && !this.salvageNudgeFired) { - this.salvageNudgeFired = true; - this.pendingSalvageNudge = PRIMARY_SALVAGE_NUDGE; - } - } - if (event.type === "tool.done" && this.workflowCalls.has(event.result.callId)) { const call = this.workflowCalls.get(event.result.callId); this.workflowCalls.delete(event.result.callId); @@ -838,8 +778,6 @@ class ChatDirectorImpl extends DefaultDirector { // wiring in src/subagent/index.ts). const toolOnlyRewrite = this.applyToolOnlyLoopProtection(baseActions, capabilities); if (toolOnlyRewrite !== null) return toolOnlyRewrite; - const lookRewrite = this.applySalvageNudge(baseActions, capabilities); - if (lookRewrite !== null) return lookRewrite; const coordinator = this.workflowCoordinator; if (coordinator?.isActive() && !coordinator.currentStepIsGate()) { diff --git a/src/agent/look-tour.test.ts b/src/agent/look-tour.test.ts deleted file mode 100644 index 661d27474..000000000 --- a/src/agent/look-tour.test.ts +++ /dev/null @@ -1,10 +0,0 @@ -import { describe, expect, test } from "bun:test"; -import { PRIMARY_SALVAGE_NUDGE } from "./look-tour.js"; - -describe("primary salvage nudge", () => { - test("tells Skywalker not to search the repo after a failed worker", () => { - expect(PRIMARY_SALVAGE_NUDGE).toContain("stopped without finishing"); - expect(PRIMARY_SALVAGE_NUDGE).toContain("Do not search the repo yourself"); - expect(PRIMARY_SALVAGE_NUDGE).toContain("Change the brief"); - }); -}); diff --git a/src/agent/look-tour.ts b/src/agent/look-tour.ts deleted file mode 100644 index 52ba39090..000000000 --- a/src/agent/look-tour.ts +++ /dev/null @@ -1,7 +0,0 @@ -/** - * After a worker hard-block salvage, nudge Skywalker once. - * Event-driven — not a look-count quota. Unique reads are legal at any volume. - */ - -export const PRIMARY_SALVAGE_NUDGE = - "A worker stopped without finishing. Synthesize Blockers for the operator. Do not search the repo yourself. Change the brief (success_criteria / do_not / agent) before starting another worker, or stop."; diff --git a/src/agent/prompts.ts b/src/agent/prompts.ts index bf3721fab..46be70d76 100644 --- a/src/agent/prompts.ts +++ b/src/agent/prompts.ts @@ -153,7 +153,6 @@ 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 bounded inference budget (unset is unbounded). 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 a thrash / no-ship / never-acted / never-edited 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.", "- 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 2d5f3f00b..603db8f01 100644 --- a/src/subagent/brief-dispatch.ts +++ b/src/subagent/brief-dispatch.ts @@ -1,12 +1,9 @@ /** * Parent-side re-dispatch caps 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 + * 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. * * Session-scoped: one ledger per createTaskTool instance (parent chat tool). */ @@ -14,9 +11,6 @@ import type { TaskIntent } from "./report.js"; import type { ForcedStopReason } from "./stop-policy.js"; -/** Salvage classes that must not be re-dispatched with an identical brief. */ -export type HardBlockSalvage = "no-ship" | "never-acted" | "never-edited"; - // Every forced-stop reason a leaf can report maps 1:1 onto a salvage kind // the parent ledger cares about. export type BriefSalvageKind = ForcedStopReason; @@ -32,8 +26,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; } /** @@ -43,12 +35,6 @@ export interface BriefDispatchRecord { */ export const TURN_BUDGET_STOP_AFTER_DISPATCHES = 3; -const HARD_BLOCK_SALVAGES = new Set(["no-ship", "never-acted", "never-edited"]); - -export function isHardBlockSalvage(kind: BriefSalvageKind): kind is HardBlockSalvage { - return HARD_BLOCK_SALVAGES.has(kind); -} - /** * Classify a completed dispatch as a salvage kind the parent ledger cares * about, from the structured stop reason the run reported directly — never @@ -93,13 +79,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, returning the 1-based dispatch count that will be used. */ + admit: (fingerprint: string) => { dispatchCount: number }; /** Record the outcome of an admitted run (salvage kind or null on success). */ recordOutcome: (fingerprint: string, salvage: BriefSalvageKind | null) => void; /** @@ -119,77 +100,33 @@ 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 } : {}), - }); - return { ok: true, dispatchCount: nextCount }; + byFingerprint.set(fingerprint, { dispatchCount: nextCount }); + return { 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; - } 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. + // A successful complete resets the same-brief retry budget. byFingerprint.set(fingerprint, { dispatchCount: 0 }); return; } - byFingerprint.set(fingerprint, { - dispatchCount: existing.dispatchCount, - lastSalvage: salvage, - }); + const existing = byFingerprint.get(fingerprint); + byFingerprint.set(fingerprint, { dispatchCount: existing?.dispatchCount ?? 1 }); }, 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 5c4db5292..d9b3a8d66 100644 --- a/src/subagent/index.test.ts +++ b/src/subagent/index.test.ts @@ -152,7 +152,6 @@ describe("sub-agent stop helpers", () => { expect( evaluateSubAgentStop({ hasToolCalls: false, - everHadToolCalls: true, turnsCompleted: 2, maxTurns: 10, }), @@ -183,7 +182,6 @@ describe("sub-agent stop helpers", () => { expect( evaluateSubAgentStop({ hasToolCalls: false, - everHadToolCalls: true, turnsCompleted: 2, maxTurns: 10, lastAssistantText: SUMMARY_ONLY_NARRATION, @@ -195,7 +193,6 @@ describe("sub-agent stop helpers", () => { expect( evaluateSubAgentStop({ hasToolCalls: false, - everHadToolCalls: true, turnsCompleted: 3, maxTurns: 10, lastAssistantText: SUMMARY_ONLY_NARRATION, @@ -208,7 +205,6 @@ describe("sub-agent stop helpers", () => { expect( evaluateSubAgentStop({ hasToolCalls: false, - everHadToolCalls: true, turnsCompleted: 2, maxTurns: 10, lastAssistantText: FULL_REPORT_ENVELOPE, @@ -242,7 +238,6 @@ describe("sub-agent stop helpers", () => { expect( evaluateSubAgentStop({ hasToolCalls: false, - everHadToolCalls: true, turnsCompleted: 2, maxTurns: 10, lastAssistantText: FULL_REPORT_ENVELOPE, @@ -261,7 +256,6 @@ describe("sub-agent stop helpers", () => { expect( evaluateSubAgentStop({ hasToolCalls: false, - everHadToolCalls: true, turnsCompleted: 2, maxTurns: 10, lastAssistantText: FULL_REPORT_ENVELOPE, @@ -280,7 +274,6 @@ describe("sub-agent stop helpers", () => { expect( evaluateSubAgentStop({ hasToolCalls: false, - everHadToolCalls: true, turnsCompleted: 2, maxTurns: 10, lastAssistantText: FULL_REPORT_ENVELOPE, @@ -290,35 +283,6 @@ describe("sub-agent stop helpers", () => { ).toBe("complete"); }); - test("evaluateSubAgentStop returns never-acted when the run never used tools", () => { - expect( - evaluateSubAgentStop({ - hasToolCalls: false, - everHadToolCalls: false, - turnsCompleted: 1, - maxTurns: 10, - }), - ).toBe("never-acted"); - }); - - test("evaluateSubAgentStop returns never-edited when requireEdit and tools never wrote files", () => { - const thrashState = { - totalToolCalls: 4, - readCounts: new Map([["src/a.ts", 2]]), - editedPaths: new Set(), - }; - expect( - evaluateSubAgentStop({ - hasToolCalls: false, - everHadToolCalls: true, - turnsCompleted: 5, - maxTurns: 30, - thrashState, - requireEdit: true, - }), - ).toBe("never-edited"); - }); - test("evaluateSubAgentStop does not hard-stop implement for many unique reads", () => { let thrash = EMPTY_THRASH_STATE; for (let i = 0; i < 200; i++) { @@ -329,59 +293,17 @@ describe("sub-agent stop helpers", () => { expect( evaluateSubAgentStop({ hasToolCalls: true, - everHadToolCalls: true, turnsCompleted: 40, maxTurns: 60, thrashState: thrash, - requireEdit: true, }), ).toBeNull(); - expect( - evaluateSubAgentStop({ - hasToolCalls: true, - everHadToolCalls: true, - turnsCompleted: 40, - maxTurns: 60, - thrashState: thrash, - }), - ).toBeNull(); - }); - - test("evaluateSubAgentStop still completes implement when an edit path was recorded", () => { - const thrashState = { - totalToolCalls: 4, - readCounts: new Map([["src/a.ts", 1]]), - editedPaths: new Set(["src/a.ts"]), - }; - expect( - evaluateSubAgentStop({ - hasToolCalls: false, - everHadToolCalls: true, - turnsCompleted: 5, - maxTurns: 30, - thrashState, - requireEdit: true, - }), - ).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, - requireEdit: true, - }), - ).toBe("never-acted"); }); test("evaluateSubAgentStop trips turn-budget when the leaf is still making progress", () => { expect( evaluateSubAgentStop({ hasToolCalls: true, - everHadToolCalls: true, turnsCompleted: 10, maxTurns: 10, }), @@ -392,38 +314,12 @@ describe("sub-agent stop helpers", () => { expect( evaluateSubAgentStop({ hasToolCalls: true, - everHadToolCalls: true, turnsCompleted: 5, maxTurns: 10, }), ).toBeNull(); }); - test("shell-only work is not never-edited or incomplete-report (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, - thrashState: shellState, - requireEdit: true, - requireEvidence: true, - lastAssistantText: report, - }), - ).toBe("complete"); - }); - test("re-read pressure no longer stops a worker; turn-budget still does (CL-6936)", () => { let thrash = EMPTY_THRASH_STATE; thrash = nextThrashState(thrash, [ @@ -437,7 +333,6 @@ describe("sub-agent stop helpers", () => { expect( evaluateSubAgentStop({ hasToolCalls: true, - everHadToolCalls: true, turnsCompleted: 10, maxTurns: 10, thrashState: thrash, @@ -449,7 +344,6 @@ describe("sub-agent stop helpers", () => { expect( evaluateSubAgentStop({ hasToolCalls: true, - everHadToolCalls: true, turnsCompleted: 8, maxTurns: 10, thrashState: EMPTY_THRASH_STATE, @@ -459,7 +353,6 @@ describe("sub-agent stop helpers", () => { expect( evaluateSubAgentStop({ hasToolCalls: true, - everHadToolCalls: true, turnsCompleted: 9, maxTurns: 10, thrashState: EMPTY_THRASH_STATE, @@ -468,7 +361,6 @@ describe("sub-agent stop helpers", () => { expect( evaluateSubAgentStop({ hasToolCalls: true, - everHadToolCalls: true, turnsCompleted: 10, maxTurns: 10, thrashState: EMPTY_THRASH_STATE, @@ -486,7 +378,6 @@ describe("sub-agent stop helpers", () => { expect( evaluateSubAgentStop({ hasToolCalls: true, - everHadToolCalls: true, turnsCompleted: 5, maxTurns: 20, thrashState: thrash, @@ -501,19 +392,8 @@ describe("sub-agent stop helpers", () => { 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, "never-edited")).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.", @@ -527,28 +407,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 = appendSubAgentParentHints(reparsed, "never-acted"); - 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"); @@ -599,10 +475,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", + "turn-budget", forcedStopReport("cancelled", "inner", "inner reason"), ); - expect(stopReasonFromReport(nested)).toBe("never-acted"); + expect(stopReasonFromReport(nested)).toBe("turn-budget"); // A clean report has no Stopped line. expect(stopReasonFromReport("## Summary\nDone.\n\n## Findings\nx")).toBe(null); }); @@ -773,7 +649,6 @@ describe("thrash edge cases", () => { const stop = (turnsCompleted: number, maxTurns: number, thrashState = EMPTY_THRASH_STATE) => evaluateSubAgentStop({ hasToolCalls: true, - everHadToolCalls: true, turnsCompleted, maxTurns, thrashState, @@ -1922,83 +1797,43 @@ describe("brief re-dispatch ledger (CL-4343 / CL-5203)", () => { expect(changed).not.toBe(a); }); - test("hard-blocks identical brief after no-ship salvage; allows changed brief", () => { + test("admit always succeeds, even after a repeated salvage on the same fingerprint", () => { const ledger = createBriefDispatchLedger(); - const fp = fingerprintTaskBrief({ prompt: "fix no-ship job", intent: "implement" }); - expect(ledger.admit(fp).ok).toBe(true); - ledger.recordOutcome(fp, "no-ship"); - 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-ship"); - - const other = fingerprintTaskBrief({ - prompt: "fix no-ship job with narrower scope", - intent: "implement", - successCriteria: ["one file only"], - }); - expect(ledger.admit(other).ok).toBe(true); + const fp = fingerprintTaskBrief({ prompt: "fix a job", intent: "implement" }); + expect(ledger.admit(fp).dispatchCount).toBe(1); + ledger.recordOutcome(fp, "deadline"); + expect(ledger.admit(fp).dispatchCount).toBe(2); + ledger.recordOutcome(fp, "deadline"); + expect(ledger.admit(fp).dispatchCount).toBe(3); }); - test("hard-blocks no-ship, never-acted, never-edited; not turn-budget", () => { - for (const salvage of ["no-ship", "never-acted", "never-edited"] 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); - } + test("turn-budget dispatch count advances across repeated same-brief admits", () => { const ledger = createBriefDispatchLedger(); const fp = fingerprintTaskBrief({ prompt: "budget job" }); - expect(ledger.admit(fp).ok).toBe(true); + expect(ledger.admit(fp).dispatchCount).toBe(1); 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", () => { + test("successful complete resets retry budget", () => { const ledger = createBriefDispatchLedger(); const fp = fingerprintTaskBrief({ prompt: "ok job" }); - expect(ledger.admit(fp).ok).toBe(true); + expect(ledger.admit(fp).dispatchCount).toBe(1); ledger.recordOutcome(fp, "turn-budget"); - expect(ledger.admit(fp).ok).toBe(true); - // Success clears soft salvage and zeros dispatchCount so the next admit is 1. + expect(ledger.admit(fp).dispatchCount).toBe(2); + // Success zeros dispatchCount so the next admit is 1. ledger.recordOutcome(fp, null); const afterSuccess = ledger.admit(fp); - expect(afterSuccess.ok).toBe(true); - if (!afterSuccess.ok) throw new Error("expected admit"); 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-ship"); - // ...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" }); - expect(ledger.admit(fp).ok).toBe(true); + expect(ledger.admit(fp).dispatchCount).toBe(1); ledger.release(fp); const again = ledger.admit(fp); - expect(again.ok).toBe(true); - if (!again.ok) throw new Error("expected admit"); expect(again.dispatchCount).toBe(1); }); @@ -2006,10 +1841,9 @@ describe("brief re-dispatch ledger (CL-4343 / CL-5203)", () => { // classifyBriefSalvage takes no report text at all — only the structured // stopReason and an independently-observed wasCancelled flag. expect(classifyBriefSalvage({ wasCancelled: false })).toBeNull(); - expect(classifyBriefSalvage({ stopReason: "no-ship", wasCancelled: false })).toBe("no-ship"); - // An operator cancel wins even when the run's own reason disagrees. - expect(classifyBriefSalvage({ stopReason: "no-ship", wasCancelled: true })).toBe("cancelled"); expect(classifyBriefSalvage({ stopReason: "deadline", wasCancelled: false })).toBe("deadline"); + // An operator cancel wins even when the run's own reason disagrees. + expect(classifyBriefSalvage({ stopReason: "deadline", wasCancelled: true })).toBe("cancelled"); }); test("turn-budget parent hint flips after re-dispatch threshold", () => { @@ -2024,10 +1858,10 @@ 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-ship salvage", async () => { + test("createTaskTool always re-dispatches an identical brief after a forced-stop salvage", async () => { const thrash = { - report: forcedStopReport("no-ship", "Repeated the same call"), - stopReason: "no-ship" as const, + report: forcedStopReport("deadline", "Repeated the same call"), + stopReason: "deadline" as const, }; let runs = 0; const sessions = createSubAgentSessionStore(); @@ -2048,33 +1882,16 @@ describe("brief re-dispatch ledger (CL-4343 / CL-5203)", () => { intent: "implement", }; const first = await callTask(tool, args); - expect(first).toContain("without writing any"); - expect(first).toContain("identical brief"); + expect(first).toContain("deadline"); expect(runs).toBe(1); expect(sessions.list().filter((s) => s.status === "running")).toHaveLength(0); + // Re-dispatching the identical brief is admitted, not refused. const second = await callTask(tool, args); - expect(second).toContain("refused re-dispatch"); - expect(second).toContain("no-ship"); - expect(runs).toBe(1); - // Refuse must not leave a ghost running session on the Agents strip. + expect(second).not.toContain("refused re-dispatch"); + expect(runs).toBe(2); 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("without writing any"); - expect(runs).toBe(2); }); 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 2ef21897a..f24874d12 100644 --- a/src/subagent/index.ts +++ b/src/subagent/index.ts @@ -66,12 +66,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..5dd90f25b 100644 --- a/src/subagent/nudge-director.test.ts +++ b/src/subagent/nudge-director.test.ts @@ -461,19 +461,17 @@ 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 a report envelope nudges for one, not a hard stop", async () => { 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).toContainEqual({ + type: "checkpoint", + message: "subagent-incomplete-report-nudge", + }); + expect(result.some((action) => action.type === "reply")).toBe(false); }); }); diff --git a/src/subagent/nudge-director.ts b/src/subagent/nudge-director.ts index c72aaab0b..44a060fd8 100644 --- a/src/subagent/nudge-director.ts +++ b/src/subagent/nudge-director.ts @@ -75,12 +75,9 @@ function withEphemeralNudge( export class SubAgentDirector extends DefaultDirector { private readonly compaction: CompactionGovernor; private readonly maxTurns: 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 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) @@ -160,7 +157,6 @@ export class SubAgentDirector extends DefaultDirector { maxTurns: number, stallTimeoutMs?: number, now: () => number = Date.now, - requireEdit = false, requireEvidence = false, ) { super(systemPrompt, toolDefinitions, {}); @@ -169,7 +165,6 @@ export class SubAgentDirector extends DefaultDirector { this.stallTimeoutMs = stallTimeoutMs; this.now = now; this.lastActivityAt = now(); - this.requireEdit = requireEdit; this.requireEvidence = requireEvidence; } @@ -223,17 +218,14 @@ export class SubAgentDirector extends DefaultDirector { this.lastAssistantText = lastText(content); const hasToolCalls = content.some((block) => block.type === "tool_call"); if (hasToolCalls) { - this.everHadToolCalls = true; this.thrashState = nextThrashState(this.thrashState, content); } const stop = evaluateSubAgentStop({ hasToolCalls, - everHadToolCalls: this.everHadToolCalls, turnsCompleted: this.turnsCompleted, maxTurns: this.maxTurns, thrashState: this.thrashState, - requireEdit: this.requireEdit, requireEvidence: this.requireEvidence, lastAssistantText: this.lastAssistantText, incompleteReportNudgeFired: this.incompleteReportNudgeFired, @@ -297,15 +289,9 @@ export class SubAgentDirector extends DefaultDirector { }, state: this.interventionState(), }); - } else if (stop === "turn-budget" || stop === "never-acted" || stop === "never-edited") { - const checkpoint = - stop === "never-acted" - ? "subagent-never-acted" - : stop === "never-edited" - ? "subagent-never-edited" - : "subagent-turn-budget"; - const detail = - stop === "turn-budget" ? `${this.turnsCompleted}/${this.maxTurns} turns` : undefined; + } else if (stop === "turn-budget") { + const checkpoint = "subagent-turn-budget"; + const detail = `${this.turnsCompleted}/${this.maxTurns} turns`; this.interventions({ id: stop, class: "stop", diff --git a/src/subagent/run.ts b/src/subagent/run.ts index 7fff343f0..2e2a33296 100644 --- a/src/subagent/run.ts +++ b/src/subagent/run.ts @@ -605,7 +605,6 @@ export async function runSubAgent(params: RunSubAgentParams): Promise { diff --git a/src/subagent/shell-evidence.test.ts b/src/subagent/shell-evidence.test.ts index 8f9848331..dc4c64d87 100644 --- a/src/subagent/shell-evidence.test.ts +++ b/src/subagent/shell-evidence.test.ts @@ -3,24 +3,6 @@ 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", () => { - 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 +14,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 every segment", () => { + const evidence = classifyShellFileEvidence("cat src/a.ts && grep needle 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..d90254a4e 100644 --- a/src/subagent/shell-evidence.ts +++ b/src/subagent/shell-evidence.ts @@ -1,11 +1,9 @@ // --- 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 / CritiqueDirector gate measures whether +// a worker did real work by counting typed tool calls. Reads done through +// run_shell were invisible to it, so a worker that only read with `cat` +// salvaged as incomplete-report even though it had looked at real files. // // 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 +47,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 +58,6 @@ 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. */ const EVIDENCE_VALUE_FLAGS: ReadonlySet = new Set([ ...HEAD_TAIL_VALUE_FLAGS, @@ -114,53 +90,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 +105,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 +119,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 e941ec9f6..3d435c138 100644 --- a/src/subagent/stop-policy.ts +++ b/src/subagent/stop-policy.ts @@ -73,13 +73,7 @@ export function subAgentTurnLimitExceeded(turnsCompleted: number, maxTurns: numb } export type SubAgentStopReason = - | "complete" - | "turn-budget" - | "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. @@ -87,11 +81,10 @@ export type SubAgentStopReason = * "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 + * 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 @@ -99,19 +92,11 @@ export type SubAgentStopReason = */ 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; /** When set, the near-budget force-report nudge is evaluated after tool-budget checks. */ 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 @@ -129,20 +114,10 @@ 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. 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" @@ -174,14 +149,12 @@ export function evaluateSubAgentStop(input: { // 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). The hard turn -// budget stops a leaf that would otherwise 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 stops a leaf that would otherwise 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--) { @@ -205,23 +178,13 @@ export function partialTextFromEvent(event: ReactorEmittedEvent): string | null } export type ForcedStopReason = - | "turn-budget" - | "never-acted" - | "never-edited" - | "cancelled" - | "deadline" - | "no-ship" - | "stalled" - | "incomplete-report"; + "turn-budget" | "cancelled" | "deadline" | "stalled" | "incomplete-report"; // Exact Summary text rendered for each forced-stop reason. Human-facing only — // forcedStopReport is the sole reader; the parent classifies outcomes from the // structured ForcedStopReason value itself (see run.ts/task-tool.ts), never by // parsing this text back out of the report. const FORCED_STOP_SUMMARIES: Record = { - "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: @@ -245,25 +208,18 @@ export function forcedStopReport( ): string { const summary = FORCED_STOP_SUMMARIES[reason]; const blockers = - 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 === "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 === "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()) @@ -284,18 +240,9 @@ 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.]"; - /** Options for parent-hint stacking (session re-dispatch ledger state). */ export interface SubAgentParentHintOptions { /** @@ -329,14 +276,8 @@ export function appendSubAgentParentHints( const hint = count >= stopAfter ? TURN_BUDGET_STOP_PARENT_HINT : TURN_BUDGET_PARENT_HINT; return `${hint}\n\n${report}`; } - case "never-acted": - return `${NEVER_ACTED_PARENT_HINT}\n\n${report}`; - case "never-edited": - return `${NEVER_EDITED_PARENT_HINT}\n\n${report}`; case "deadline": return `${DEADLINE_PARENT_HINT}\n\n${report}`; - case "no-ship": - return `${NO_SHIP_PARENT_HINT}\n\n${report}`; default: return report; } diff --git a/src/subagent/task-tool.ts b/src/subagent/task-tool.ts index acd8214ad..8d9153324 100644 --- a/src/subagent/task-tool.ts +++ b/src/subagent/task-tool.ts @@ -76,7 +76,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 a no-ship / never-acted / never-edited 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). 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: { @@ -259,15 +259,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 @@ -615,8 +606,6 @@ export function createTaskTool(deps: TaskToolDeps): AgentTool { ...(doNot.length > 0 ? { doNot } : {}), ...(reportFocus !== undefined && reportFocus.length > 0 ? { reportFocus } : {}), }); - // Parent re-dispatch caps (CL-4343 / CL-5203): admit before session start so - // a thrash-class refuse never leaves a ghost "running" Agents-strip row. const fingerprint = fingerprintTaskBrief({ prompt, ...(agentId !== undefined && agentId.length > 0 ? { agent: agentId } : {}), @@ -624,16 +613,7 @@ export function createTaskTool(deps: TaskToolDeps): AgentTool { ...(successCriteria.length > 0 ? { successCriteria } : {}), ...(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 dispatchCount = briefLedger.admit(fingerprint).dispatchCount; const agentLabel = agentId !== undefined && agentId.length > 0 ? agentId : (resolvedDirectorId ?? "worker"); diff --git a/src/subagent/thrash.test.ts b/src/subagent/thrash.test.ts index bb0258cc9..06132fdc0 100644 --- a/src/subagent/thrash.test.ts +++ b/src/subagent/thrash.test.ts @@ -73,7 +73,7 @@ describe("thrash pure module", () => { expect(thrashForceReport(8, 10, true)).toBe(true); expect(thrashForceReport(9, 10, true)).toBe(false); expect(thrashForceReport(10, 10, true)).toBe(false); - // No tools this turn → not force-report (tool-less is complete/never-acted). + // No tools this turn → not force-report (tool-less turns are complete). expect(thrashForceReport(8, 10, false)).toBe(false); expect(evaluateThrashStop({ hasToolCalls: true, turnsCompleted: 8, maxTurns: 10 })).toBe( @@ -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..3bc645f44 100644 --- a/src/subagent/thrash.ts +++ b/src/subagent/thrash.ts @@ -8,10 +8,12 @@ * 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. + * requireEvidence check, not by a stop of its own. 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` (from typed write + * tools only) is diagnostics for interventions.jsonl; no stop decision + * depends on it. */ import { isProductMutationTool, productMutationPaths } from "../agent/product-mutation-tools.js"; @@ -131,8 +133,9 @@ 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 reads are evidence too (CL-6937) — the prompt prohibits shell + // file work, but a prompt violation deserves a correction, not a + // verdict that the work never happened. const command = args.command; if (typeof command === "string" && command.length > 0) { const evidence = classifyShellFileEvidence(command); @@ -142,10 +145,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);