Merge pull request #491 from corbitsdev/cl-6548-completeness-wrap-up-does-not-prove-critique-finishes-a-real

Stop treating a wrap-up envelope as a finished critique without reads

Author: TheGreatAxios

src/subagent/index.test.ts

@@ -34,6 +34,7 @@ import {
   preferCompletedSubAgentReply,
   resolveSubAgentCatchOutcome,
   resolveSubAgentDeadlineMs,
+  shouldRequireEvidence,
   subAgentToolName,
   SUBAGENT_DEADLINE_MARGIN_MS,
   SUBAGENT_PLUGIN_SPAWN_TEARDOWN_LIMITS,
@@ -45,6 +46,8 @@ import {
 } from "./index.js";
 
 import { type } from "arktype";
+import { formatDirectorSystemPrompt } from "../agent/directors/identity.js";
+import { DIRECTOR_REGISTRY } from "../agent/directors/registry.js";
 import type {
   ReactorAction,
   ReactorCapabilities,
@@ -277,6 +280,91 @@ describe("sub-agent stop helpers", () => {
     ).toBe("complete");
   });
 
+  test("shouldRequireEvidence is armed for CritiqueDirector prompt", () => {
+    expect(
+      shouldRequireEvidence({
+        systemPromptRole: formatDirectorSystemPrompt(DIRECTOR_REGISTRY.critique),
+      }),
+    ).toBe(true);
+    expect(
+      shouldRequireEvidence({
+        systemPromptRole: DIRECTOR_REGISTRY.critique.systemPrompt,
+      }),
+    ).toBe(true);
+  });
+
+  test("shouldRequireEvidence is off for greybeard even with intent=review", () => {
+    expect(
+      shouldRequireEvidence({
+        intent: "review",
+        systemPromptRole: formatDirectorSystemPrompt(DIRECTOR_REGISTRY.greybeard),
+      }),
+    ).toBe(false);
+  });
+
+  test("evaluateSubAgentStop does not complete a review/critique with empty readCounts even with a full envelope", () => {
+    const thrashState = {
+      totalToolCalls: 1,
+      readCounts: new Map(),
+      editedPaths: new Set<string>(),
+    };
+    expect(
+      evaluateSubAgentStop({
+        hasToolCalls: false,
+        everHadToolCalls: true,
+        turnsCompleted: 2,
+        maxTurns: 10,
+        consecutiveIdentical: 0,
+        repeatLimit: 2,
+        lastAssistantText: FULL_REPORT_ENVELOPE,
+        thrashState,
+        requireEvidence: true,
+      }),
+    ).toBe("incomplete-report");
+  });
+
+  test("evaluateSubAgentStop completes a review when readCounts has file evidence", () => {
+    const thrashState = {
+      totalToolCalls: 1,
+      readCounts: new Map([["src/gate.ts", 1]]),
+      editedPaths: new Set<string>(),
+    };
+    expect(
+      evaluateSubAgentStop({
+        hasToolCalls: false,
+        everHadToolCalls: true,
+        turnsCompleted: 2,
+        maxTurns: 10,
+        consecutiveIdentical: 0,
+        repeatLimit: 2,
+        lastAssistantText: FULL_REPORT_ENVELOPE,
+        thrashState,
+        requireEvidence: true,
+      }),
+    ).toBe("complete");
+  });
+
+  test("evaluateSubAgentStop completes greybeard spawn-only envelope when requireEvidence is off", () => {
+    const thrashState = {
+      totalToolCalls: 1,
+      readCounts: new Map(),
+      editedPaths: new Set<string>(),
+    };
+    expect(
+      evaluateSubAgentStop({
+        hasToolCalls: false,
+        everHadToolCalls: true,
+        turnsCompleted: 2,
+        maxTurns: 10,
+        consecutiveIdentical: 0,
+        repeatLimit: 2,
+        lastAssistantText: FULL_REPORT_ENVELOPE,
+        thrashState,
+        requireEvidence: false,
+      }),
+    ).toBe("complete");
+  });
+
   test("evaluateSubAgentStop returns never-acted when the run never used tools", () => {
     expect(
       evaluateSubAgentStop({

src/subagent/index.ts

@@ -116,6 +116,7 @@ export {
   coreSubAgentWebTools,
   createSubAgentRunController,
   runSubAgent,
+  shouldRequireEvidence,
   type SubAgentRunController,
 } from "./run.js";
 

src/subagent/nudge-director.ts

@@ -93,6 +93,8 @@ export class SubAgentDirector extends DefaultDirector {
   private readonly repeatLimit: number;
   /** When true (intent=implement), tool-less finish without edits salvages as never-edited. */
   private readonly requireEdit: boolean;
+  /** When true (CritiqueDirector), empty readCounts is not a successful complete. */
+  private readonly requireEvidence: boolean;
   private turnsCompleted = 0;
   private everHadToolCalls = false;
   private streak: ToolCallStreak = {
@@ -148,6 +150,7 @@ export class SubAgentDirector extends DefaultDirector {
     stallTimeoutMs?: number,
     now: () => number = Date.now,
     requireEdit: boolean = false,
+    requireEvidence: boolean = false,
   ) {
     super(systemPrompt, toolDefinitions, {});
     this.compaction = createCompactionGovernor(requestContinuation, systemPrompt, toolDefinitions);
@@ -157,6 +160,7 @@ export class SubAgentDirector extends DefaultDirector {
     this.now = now;
     this.lastActivityAt = now();
     this.requireEdit = requireEdit;
+    this.requireEvidence = requireEvidence;
   }
 
   override async decide(
@@ -217,6 +221,7 @@ export class SubAgentDirector extends DefaultDirector {
         repeatLimit: this.repeatLimit,
         thrashState: this.thrashState,
         requireEdit: this.requireEdit,
+        requireEvidence: this.requireEvidence,
         lastAssistantText: this.lastAssistantText,
         incompleteReportNudgeFired: this.incompleteReportNudgeFired,
       });

src/subagent/run.ts

@@ -89,6 +89,7 @@ import {
 } from "./dispose.js";
 import { createTaskTool } from "./task-tool.js";
 import type { RunSubAgentParams, SubAgentProvider } from "./types.js";
+import type { TaskIntent } from "./report.js";
 import { runWithSubAgentIdentity } from "./identity-context.js";
 
 export type {
@@ -223,6 +224,21 @@ export function createSubAgentRunController(
   };
 }
 
+/**
+ * Arm requireEvidence only for CritiqueDirector. Greybeard is also
+ * intent=review and may spawn-only then envelope; that is not a fake
+ * review — do not pull it into the empty-readCounts gate.
+ */
+export function shouldRequireEvidence(input: {
+  intent?: TaskIntent;
+  systemPromptRole?: string;
+}): boolean {
+  return (
+    typeof input.systemPromptRole === "string" &&
+    input.systemPromptRole.includes("CritiqueDirector")
+  );
+}
+
 // Spin up an isolated, autonomous agent loop, hand it one task, and return
 // its final report. `params.cwd` is either the dispatcher's own cwd (shared
 // mode) or a worktree snapshotted from the dispatcher's last commit
@@ -417,6 +433,7 @@ export async function runSubAgent(params: RunSubAgentParams): Promise<string> {
         modelFamilyPolicy.subAgentStallTimeoutMs,
         Date.now,
         params.intent === "implement",
+        shouldRequireEvidence(params),
       ),
   });
 

src/subagent/stop-policy.ts

@@ -275,6 +275,9 @@ export type SubAgentStopReason =
  * (Summary, Findings, Blockers, Paths). Omitting `lastAssistantText`
  * still completes (back-compat). Missing envelope nudges
  * once (`incomplete-report`) then salvages (`incomplete-report-stop`).
+ * When `requireEvidence` is set (CritiqueDirector), an empty `readCounts`
+ * is not complete even with all four headings — same incomplete-report
+ * nudge then salvage, so a wrap-up envelope cannot fake a real review.
  */
 export function evaluateSubAgentStop(input: {
   hasToolCalls: boolean;
@@ -293,6 +296,13 @@ export function evaluateSubAgentStop(input: {
    * 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
+   * envelope is incomplete-report so the parent does not treat a wrap-up
+   * narration as a finished review.
+   */
+  requireEvidence?: boolean;
   /**
    * Final assistant text of this turn. When omitted, a tool-less turn after
    * tools still completes (back-compat for existing unit tests). When provided,
@@ -307,7 +317,8 @@ export function evaluateSubAgentStop(input: {
   // 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).
+  // lastAssistantText is omitted). CritiqueDirector additionally requires
+  // at least one read/search in thrashState.readCounts.
   if (!input.hasToolCalls) {
     if (!input.everHadToolCalls) return "never-acted";
     if (
@@ -324,6 +335,14 @@ export function evaluateSubAgentStop(input: {
         ? "incomplete-report-stop"
         : "incomplete-report";
     }
+    if (
+      input.requireEvidence === true &&
+      (input.thrashState === undefined || input.thrashState.readCounts.size === 0)
+    ) {
+      return input.incompleteReportNudgeFired === true
+        ? "incomplete-report-stop"
+        : "incomplete-report";
+    }
     return "complete";
   }
   // No-progress is more specific than thrash or the turn budget when both could apply.