Merge pull request #579 from corbitsdev/cl-6964-delete-the-unreachable-thrash-matcher-hint-and-salvage-class

Kill the sticky salvage hard-block's false positives

Author: TheGreatAxios

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<BriefSalvageKind>([
-  "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;
       }

src/subagent/index.test.ts

@@ -2150,19 +2150,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"],
     });
@@ -2187,7 +2187,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);
@@ -2199,13 +2199,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", () => {
@@ -2239,6 +2250,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 });

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,

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<ForcedStopReason, string> = {
+  "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,16 +627,15 @@ 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 {
   if (!isNoProgressSubAgentReport(report)) return report;
   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,
         ),
       ),