diff --git a/src/subagent/brief-dispatch.ts b/src/subagent/brief-dispatch.ts index d31d424a..7cf8d6fb 100644 --- a/src/subagent/brief-dispatch.ts +++ b/src/subagent/brief-dispatch.ts @@ -1,9 +1,9 @@ /** * Parent-side re-dispatch caps for task briefs (CL-4343 + CL-5203). * - * Leaf stops already salvage thrash / no-progress / turn-budget / etc. This + * Leaf stops already salvage no-progress / turn-budget / etc. This * module tracks how often the *parent* re-spawns the same brief so: - * - thrash-class salvages hard-block an identical re-dispatch for the rest of + * - 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 @@ -12,21 +12,20 @@ */ import type { TaskIntent } from "./report.js"; -import { parseSubAgentReport } from "./report.js"; import { isDeadlineSubAgentReport, + isForcedStopSubAgentReport, isNeverActedSubAgentReport, isNeverEditedSubAgentReport, isNoProgressSubAgentReport, isNoShipSubAgentReport, isRepetitionSubAgentReport, - isThrashSubAgentReport, isTurnBudgetSubAgentReport, } from "./stop-policy.js"; /** Salvage classes that must not be re-dispatched with an identical brief. */ export type HardBlockSalvage = - "thrash" | "no-ship" | "no-progress" | "repetition" | "never-acted" | "never-edited"; + "no-ship" | "no-progress" | "repetition" | "never-acted" | "never-edited"; export type BriefSalvageKind = HardBlockSalvage | "turn-budget" | "deadline" | "stalled" | "cancelled" | "incomplete-report"; @@ -54,7 +53,6 @@ export interface BriefDispatchRecord { export const TURN_BUDGET_STOP_AFTER_DISPATCHES = 3; const HARD_BLOCK_SALVAGES = new Set([ - "thrash", "no-ship", "no-progress", "repetition", @@ -68,20 +66,17 @@ export function isHardBlockSalvage(kind: BriefSalvageKind): kind is HardBlockSal /** True when the worker returned a stall salvage report. */ export function isStalledSubAgentReport(report: string): boolean { - const parsed = parseSubAgentReport(report); - return parsed.summary.toLowerCase().includes("long silence"); + return isForcedStopSubAgentReport(report, "stalled"); } /** True when the worker returned a cancel salvage report. */ export function isCancelledSubAgentReport(report: string): boolean { - const parsed = parseSubAgentReport(report); - return parsed.summary.toLowerCase().includes("cancelled"); + return isForcedStopSubAgentReport(report, "cancelled"); } /** True when the worker returned an incomplete-report salvage (narration, no envelope). */ export function isIncompleteReportSubAgentReport(report: string): boolean { - const parsed = parseSubAgentReport(report); - return parsed.summary.toLowerCase().includes("narrated instead of writing a report envelope"); + return isForcedStopSubAgentReport(report, "incomplete-report"); } /** @@ -90,7 +85,6 @@ export function isIncompleteReportSubAgentReport(report: string): boolean { */ export function classifyBriefSalvage(report: string): BriefSalvageKind | null { // Order: more specific salvage phrases first. - if (isThrashSubAgentReport(report)) return "thrash"; if (isNoShipSubAgentReport(report)) return "no-ship"; if (isRepetitionSubAgentReport(report)) return "repetition"; if (isNeverEditedSubAgentReport(report)) return "never-edited"; @@ -184,16 +178,11 @@ export function createBriefDispatchLedger(): BriefDispatchLedger { return; } if (salvage === null) { - // Successful complete resets the same-brief retry budget. Hard-block - // lastSalvage is sticky for the session and must not be cleared by a - // concurrent twin that finishes after thrash was already recorded. - if (existing.lastSalvage !== undefined && isHardBlockSalvage(existing.lastSalvage)) { - byFingerprint.set(fingerprint, { - dispatchCount: existing.dispatchCount, - lastSalvage: existing.lastSalvage, - }); - return; - } + // 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; } diff --git a/src/subagent/index.test.ts b/src/subagent/index.test.ts index 4e356be5..f41228dd 100644 --- a/src/subagent/index.test.ts +++ b/src/subagent/index.test.ts @@ -2146,19 +2146,19 @@ describe("brief re-dispatch ledger (CL-4343 / CL-5203)", () => { expect(changed).not.toBe(a); }); - test("hard-blocks identical brief after thrash salvage; allows changed brief", () => { + test("hard-blocks identical brief after no-progress salvage; allows changed brief", () => { const ledger = createBriefDispatchLedger(); - const fp = fingerprintTaskBrief({ prompt: "fix thrash", intent: "implement" }); + const fp = fingerprintTaskBrief({ prompt: "fix no-progress job", intent: "implement" }); expect(ledger.admit(fp).ok).toBe(true); - ledger.recordOutcome(fp, "thrash"); + 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("thrash"); + expect(blocked.message).toContain("no-progress"); const other = fingerprintTaskBrief({ - prompt: "fix thrash with narrower scope", + prompt: "fix no-progress job with narrower scope", intent: "implement", successCriteria: ["one file only"], }); @@ -2183,7 +2183,7 @@ describe("brief re-dispatch ledger (CL-4343 / CL-5203)", () => { expect(second.dispatchCount).toBe(2); }); - test("successful complete resets retry budget; thrash hard-block is sticky", () => { + test("successful complete resets retry budget and clears soft salvage", () => { const ledger = createBriefDispatchLedger(); const fp = fingerprintTaskBrief({ prompt: "ok job" }); expect(ledger.admit(fp).ok).toBe(true); @@ -2195,13 +2195,24 @@ describe("brief re-dispatch ledger (CL-4343 / CL-5203)", () => { 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-progress"); + // ...but the other sibling succeeds in the same wave. + ledger.recordOutcome(fp, null); - // Thrash is sticky for the session — success on a concurrent twin must not clear it. - const thrashFp = fingerprintTaskBrief({ prompt: "thrash sticky" }); - ledger.admit(thrashFp); - ledger.recordOutcome(thrashFp, "thrash"); - ledger.recordOutcome(thrashFp, null); - expect(ledger.admit(thrashFp).ok).toBe(false); + // 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", () => { @@ -2235,6 +2246,39 @@ 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.", + blockers: "None", + paths: "src/refunds.ts", + }); + expect(classifyBriefSalvage(cancelledPhrase)).toBeNull(); + + const longSilencePhrase = formatSubAgentReport({ + summary: "Reduced UI flicker with a long silence period before re-render.", + findings: "Debounced the re-render.", + blockers: "None", + paths: "src/ui.ts", + }); + expect(classifyBriefSalvage(longSilencePhrase)).toBeNull(); + }); + + 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("cancelled", "x"))).toBe("cancelled"); + expect(classifyBriefSalvage(forcedStopReport("stalled", "x"))).toBe("stalled"); + expect(classifyBriefSalvage(forcedStopReport("deadline", "x"))).toBe("deadline"); + }); + test("turn-budget parent hint flips after re-dispatch threshold", () => { const report = forcedStopReport("turn-budget", "partial"); const first = appendSubAgentParentHints(report, { dispatchCount: 1 }); diff --git a/src/subagent/index.ts b/src/subagent/index.ts index 2899ebb5..5d4c7909 100644 --- a/src/subagent/index.ts +++ b/src/subagent/index.ts @@ -52,7 +52,6 @@ export { appendNoProgressParentHint, appendRepetitionParentHint, appendSubAgentParentHints, - appendThrashParentHint, appendTurnBudgetParentHint, evaluateSubAgentStop, fingerprintToolCalls, @@ -62,7 +61,6 @@ export { isNeverEditedSubAgentReport, isNoProgressSubAgentReport, isRepetitionSubAgentReport, - isThrashSubAgentReport, isTurnBudgetSubAgentReport, nextToolCallStreak, partialTextFromEvent, diff --git a/src/subagent/stop-policy.ts b/src/subagent/stop-policy.ts index 1684e857..32a5d4ba 100644 --- a/src/subagent/stop-policy.ts +++ b/src/subagent/stop-policy.ts @@ -436,6 +436,27 @@ export type ForcedStopReason = | "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 +// classifiers (the consumers) — CL-6704: classifying on a free-text substring +// like "no progress" or "cancelled" hard-blocks a SUCCESSFUL report whose +// Summary happens to contain that phrase. Matching the exact string a forced +// 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: + "Stopped after a long silence with no tool activity. The parent can re-dispatch or check the background work directly.", + repetition: "Stopped: degenerate repetition in streamed output (same window looping mid-turn).", + "incomplete-report": "Stopped: worker narrated instead of writing a report envelope.", + "turn-budget": "Turn budget reached before finishing.", +}; + /** * Build the parent-facing report when a leaf is force-stopped. There is no * further inference, so this must already be a full envelope — not an @@ -449,26 +470,7 @@ export function forcedStopReport( partialText: string, detail?: string, ): string { - const summary = - reason === "no-progress" - ? "Stopped: repeated the same tool calls with no progress." - : reason === "no-ship" - ? "Stopped: implement intent searched many files without writing any." - : reason === "never-acted" - ? "Stopped: completed without using any tools." - : reason === "never-edited" - ? "Stopped: implement intent finished without writing any files." - : reason === "cancelled" - ? "Stopped: cancelled by operator before finishing." - : reason === "deadline" - ? "Stopped: wall-clock deadline reached before finishing." - : reason === "stalled" - ? "Stopped after a long silence with no tool activity. The parent can re-dispatch or check the background work directly." - : reason === "repetition" - ? "Stopped: degenerate repetition in streamed output (same window looping mid-turn)." - : reason === "incomplete-report" - ? "Stopped: worker narrated instead of writing a report envelope." - : "Turn budget reached before finishing."; + 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." @@ -506,40 +508,40 @@ export function forcedStopReport( }); } +/** + * True when a report's Summary is exactly the forced-stop text for `reason` + * (CL-6704: exact match, not a free-text substring — a successful report + * whose Summary happens to mention the same words must not classify as a + * forced stop). + */ +export function isForcedStopSubAgentReport(report: string, reason: ForcedStopReason): boolean { + const parsed = parseSubAgentReport(report); + return parsed.summary === FORCED_STOP_SUMMARIES[reason]; +} + /** True when the worker returned a turn-budget salvage report for the parent. */ export function isTurnBudgetSubAgentReport(report: string): boolean { - const parsed = parseSubAgentReport(report); - return parsed.summary.includes("Turn budget reached"); + return isForcedStopSubAgentReport(report, "turn-budget"); } /** True when the worker returned a never-acted salvage report for the parent. */ export function isNeverActedSubAgentReport(report: string): boolean { - const parsed = parseSubAgentReport(report); - return parsed.summary.includes("without using any tools"); + return isForcedStopSubAgentReport(report, "never-acted"); } /** True when implement intent finished without any write/edit tools. */ export function isNeverEditedSubAgentReport(report: string): boolean { - const parsed = parseSubAgentReport(report); - return parsed.summary.includes("without writing any files"); + return isForcedStopSubAgentReport(report, "never-edited"); } /** True when the worker returned a deadline salvage report for the parent. */ export function isDeadlineSubAgentReport(report: string): boolean { - const parsed = parseSubAgentReport(report); - return parsed.summary.includes("deadline reached"); -} - -/** True when the worker returned a progressive-thrash salvage report. */ -export function isThrashSubAgentReport(report: string): boolean { - const parsed = parseSubAgentReport(report); - return parsed.summary.includes("progressive thrash"); + return isForcedStopSubAgentReport(report, "deadline"); } /** True when the worker returned a streamed-repetition salvage report. */ export function isRepetitionSubAgentReport(report: string): boolean { - const parsed = parseSubAgentReport(report); - return parsed.summary.includes("degenerate repetition"); + return isForcedStopSubAgentReport(report, "repetition"); } const TURN_BUDGET_PARENT_HINT = @@ -558,9 +560,6 @@ const NEVER_EDITED_PARENT_HINT = 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 THRASH_PARENT_HINT = - "[Sub-agent stopped for progressive thrash (re-read pressure). Do not re-dispatch the identical brief (it will be refused) — change scope, success_criteria, and do_not; continue from Findings.]"; - 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.]"; @@ -611,15 +610,9 @@ export function appendDeadlineParentHint(report: string): string { return `${DEADLINE_PARENT_HINT}\n\n${report}`; } -export function appendThrashParentHint(report: string): string { - if (!isThrashSubAgentReport(report)) return report; - return `${THRASH_PARENT_HINT}\n\n${report}`; -} - /** True when the worker returned a no-ship (search-tour) salvage report. */ export function isNoShipSubAgentReport(report: string): boolean { - const parsed = parseSubAgentReport(report); - return parsed.summary.includes("searched many files without writing"); + return isForcedStopSubAgentReport(report, "no-ship"); } export function appendNoShipParentHint(report: string): string { @@ -634,8 +627,7 @@ export function appendRepetitionParentHint(report: string): string { /** True when the worker returned a no-progress salvage report. */ export function isNoProgressSubAgentReport(report: string): boolean { - const parsed = parseSubAgentReport(report); - return parsed.summary.includes("no progress"); + return isForcedStopSubAgentReport(report, "no-progress"); } export function appendNoProgressParentHint(report: string): string { @@ -643,7 +635,7 @@ export function appendNoProgressParentHint(report: string): string { return `${NO_PROGRESS_PARENT_HINT}\n\n${report}`; } -/** Stack parent-visible salvage hints for thrash / budget / never-acted / deadline / repetition / no-progress. */ +/** Stack parent-visible salvage hints for budget / never-acted / deadline / repetition / no-progress. */ export function appendSubAgentParentHints( report: string, options: SubAgentParentHintOptions = {}, @@ -652,9 +644,7 @@ export function appendSubAgentParentHints( appendNeverEditedParentHint( appendNeverActedParentHint( appendTurnBudgetParentHint( - appendNoProgressParentHint( - appendNoShipParentHint(appendThrashParentHint(appendRepetitionParentHint(report))), - ), + appendNoProgressParentHint(appendNoShipParentHint(appendRepetitionParentHint(report))), options, ), ),