fix(worker): refuse the no_change_needed exit when PR review feedback is pending (#325)

Author: outof-place

apps/worker/src/sandbox/context.test.ts

@@ -70,6 +70,68 @@ describe("assembleResearchPlanContext", () => {
     expect(result).toContain("resolutionEvidence");
   });
 
+  it("omits the Resolution Check when repository contexts carry PR review feedback", () => {
+    const result = assembleResearchPlanContext({
+      ticket: {
+        identifier: "TEST-10",
+        title: "Add login page",
+        description: "Build a login page",
+        acceptanceCriteria: "User can log in",
+        comments: [],
+      },
+      prompt: "",
+      branchName: "blazebot/test-10",
+      repositoryContexts: [
+        {
+          repository: {
+            provider: "github",
+            repoPath: "acme/api",
+            defaultBranch: "main",
+            selectedRationale: "workflow-owned branch for this ticket",
+          },
+          prComments: [
+            { author: "Bob", body: "please add the missing null check", liked: false },
+          ],
+          checkResults: [],
+          hasConflicts: false,
+        },
+      ],
+    });
+
+    expect(result).not.toContain("## Resolution Check");
+    expect(result).toContain("## Existing pull request — address this review feedback");
+    expect(result).toContain("Research is read-only");
+  });
+
+  it("keeps the Resolution Check when repository contexts have no PR comments", () => {
+    const result = assembleResearchPlanContext({
+      ticket: {
+        identifier: "TEST-11",
+        title: "Add login page",
+        description: "Build a login page",
+        acceptanceCriteria: "User can log in",
+        comments: [],
+      },
+      prompt: "",
+      branchName: "blazebot/test-11",
+      repositoryContexts: [
+        {
+          repository: {
+            provider: "github",
+            repoPath: "acme/api",
+            defaultBranch: "main",
+            selectedRationale: "workflow-owned branch for this ticket",
+          },
+          prComments: [],
+          checkResults: [],
+          hasConflicts: false,
+        },
+      ],
+    });
+
+    expect(result).toContain("## Resolution Check");
+  });
+
   it("assembles context for new ticket (no PR feedback)", () => {
     const result = assembleResearchPlanContext({
       ticket: {

apps/worker/src/sandbox/context.ts

@@ -83,6 +83,12 @@ export function assembleResearchPlanContext(input: ResearchPlanContextInput): st
   const selectedRepositoriesSection = renderSelectedRepositories(selectedRepositories, input.workspaceManifest);
   const repositoryContextSection = renderRepositoryContexts(repositoryContexts);
   const clarificationsSection = renderClarificationsSection(ticket.clarifications);
+  // Same condition as renderRepositoryContexts' remediation section: when the
+  // ticket's PR carries review feedback, that feedback is the task, so the
+  // Resolution Check must not offer the already-resolved exit.
+  const hasPrFeedback = (repositoryContexts ?? []).some(
+    (context) => context.prComments.length > 0,
+  );
 
   let md = `# Requirements
 
@@ -140,7 +146,9 @@ This protocol extends and overrides any older Output Format instructions above.
   repository.
 - Set fields that do not apply to \`null\`, as required by the structured schema.
 - Research is read-only: do not modify files, create commits, or change branches.
-
+`;
+  if (!hasPrFeedback) {
+    md += `
 ## Resolution Check
 
 - Before planning any implementation, check whether the ticket is already resolved:
@@ -157,6 +165,7 @@ This protocol extends and overrides any older Output Format instructions above.
   is resolved, do not set \`noChangeNeeded\`; follow the Repository Access
   Protocol instead.
 `;
+  }
   return md;
 }
 

apps/worker/src/workflows/agent-no-change.test.ts

@@ -1,6 +1,6 @@
 import { describe, expect, it } from "vitest";
 import type { ResearchResult } from "../sandbox/agents/types.js";
-import { buildResolutionEvidenceComment } from "./agent.js";
+import { buildResolutionEvidenceComment, resolveNoChangeAction } from "./agent.js";
 
 const research = (overrides: Partial<ResearchResult> = {}): ResearchResult => ({
   status: "completed",
@@ -58,3 +58,62 @@ describe("buildResolutionEvidenceComment", () => {
     expect(withMissing).toBe(withEmpty);
   });
 });
+
+describe("resolveNoChangeAction", () => {
+  const contextWith = (prComments: Array<{ author: string; body: string; liked: boolean }>) => [
+    { prComments },
+  ];
+  const humanComment = [
+    { author: "Bob", body: "please add the missing null check", liked: false },
+  ];
+
+  it("returns no_change for a complete signal with no repository contexts", () => {
+    expect(resolveNoChangeAction(research(), [], false)).toBe("no_change");
+  });
+
+  it("returns no_change when the ticket's PR has no comments", () => {
+    expect(resolveNoChangeAction(research(), contextWith([]), false)).toBe(
+      "no_change",
+    );
+  });
+
+  it("returns retry on the first declaration against pending PR feedback", () => {
+    expect(
+      resolveNoChangeAction(research(), contextWith(humanComment), false),
+    ).toBe("retry");
+  });
+
+  it("returns fail when the retry was already spent", () => {
+    expect(
+      resolveNoChangeAction(research(), contextWith(humanComment), true),
+    ).toBe("fail");
+  });
+
+  it("returns proceed for a half-filled signal even with pending PR feedback", () => {
+    expect(
+      resolveNoChangeAction(
+        research({ resolutionEvidence: [] }),
+        contextWith(humanComment),
+        false,
+      ),
+    ).toBe("proceed");
+    expect(
+      resolveNoChangeAction(
+        research({
+          writeRepositories: [
+            { provider: "github", repoPath: "acme/api", rationale: "fix lives here" },
+          ],
+        }),
+        contextWith(humanComment),
+        false,
+      ),
+    ).toBe("proceed");
+    expect(
+      resolveNoChangeAction(
+        research({ noChangeNeeded: undefined }),
+        contextWith(humanComment),
+        false,
+      ),
+    ).toBe("proceed");
+  });
+});

apps/worker/src/workflows/agent.ts

@@ -21,6 +21,7 @@ import type { TicketEvent } from "../adapters/messaging/types.js";
 import type { ActiveRunOwner } from "../lib/active-run-owner.js";
 import type { DownloadedAttachment } from "../sandbox/attachments.js";
 import type { SelectedRepository } from "../adapters/vcs/repository-directory.js";
+import type { SelectedRepositoryPromptContext } from "../sandbox/context.js";
 import {
   buildRuntimeGraph,
   createWorkflowExecutionErrorState,
@@ -2088,6 +2089,34 @@ export function buildResolutionEvidenceComment(research: ResearchResult): string
   return sections.join("\n\n");
 }
 
+/**
+ * Decide what to do with research's already-resolved declaration. A review
+ * comment on the ticket's own PR means a person explicitly asked for changes,
+ * so the no_change_needed exit must not be taken: the first declaration earns
+ * one corrective research retry, a repeat fails the block. Uses the same
+ * prComments condition as renderRepositoryContexts' remediation section, so
+ * the prompt and the engine agree on what counts as pending feedback. Pure so
+ * the decision table stays unit-testable.
+ */
+export function resolveNoChangeAction(
+  research: ResearchResult,
+  repositoryContexts: ReadonlyArray<
+    Pick<SelectedRepositoryPromptContext, "prComments">
+  >,
+  retryUsed: boolean,
+): "proceed" | "no_change" | "retry" | "fail" {
+  const noChangeSignal =
+    research.noChangeNeeded === true &&
+    (research.resolutionEvidence ?? []).length > 0 &&
+    (research.writeRepositories ?? []).length === 0;
+  if (!noChangeSignal) return "proceed";
+  const hasPrFeedback = repositoryContexts.some(
+    (context) => context.prComments.length > 0,
+  );
+  if (!hasPrFeedback) return "no_change";
+  return retryUsed ? "fail" : "retry";
+}
+
 export function v2TerminalBlockResult(input: {
   terminalStatus: TerminalStatus;
   postComment?: string;
@@ -4487,6 +4516,7 @@ async function agentWorkflowBody(
           }
 
           case "planning_agent": {
+            let noChangeRetryUsed = false;
             for (;;) {
             // AIW-147 IM-11: a human answer to the expansion-limit clarification
             // attaches the repositories it named beyond the model round limit
@@ -4540,15 +4570,22 @@ async function agentWorkflowBody(
               continue;
             }
             const expansionRound = ctx.repositoryExpansion.rounds;
+            // The retry re-runs the research phase, so both the label and the
+            // artifact phase must stay distinct from the first pass (same
+            // freshness trick as the -expansion-N suffix).
+            const noChangeRetrySuffix = noChangeRetryUsed ? " no-change retry" : "";
             const researchLabel =
               ctx.schemaVersion === 2
-                ? `Research ${node.id}${expansionRound > 0 ? ` expansion ${expansionRound}` : ""}`
-                : `Research${expansionRound > 0 ? ` expansion ${expansionRound}` : ""}`;
+                ? `Research ${node.id}${expansionRound > 0 ? ` expansion ${expansionRound}` : ""}${noChangeRetrySuffix}`
+                : `Research${expansionRound > 0 ? ` expansion ${expansionRound}` : ""}${noChangeRetrySuffix}`;
             const baseResearchArtifactPhase = agentArtifactPhase("research", execution);
-            const researchArtifactPhase =
+            const expandedResearchArtifactPhase =
               expansionRound > 0
                 ? `${baseResearchArtifactPhase}-expansion-${expansionRound}`
                 : baseResearchArtifactPhase;
+            const researchArtifactPhase = noChangeRetryUsed
+              ? `${expandedResearchArtifactPhase}-no-change-retry`
+              : expandedResearchArtifactPhase;
             const researchPhase = phaseKey(researchLabel, invocationAttempt);
             const { kind, model, runtime } = resolveAgentForNode(node);
             const workspace = await ensureCodeWorkspace(execution);
@@ -4600,25 +4637,33 @@ async function agentWorkflowBody(
                 RESEARCH_SCHEMA,
                 runtime,
               );
+            const researchAdditions = [...ctx.preSandboxAdditions.research];
+            if (ctx.repositoryExpansion.priorRequests.length > 0) {
+              researchAdditions.push({
+                target: ["research" as const],
+                title: "Repository expansion history",
+                content: [
+                  "The following repositories were requested and are now attached.",
+                  "Continue the same research; do not restart from assumptions.",
+                  JSON.stringify(ctx.repositoryExpansion.priorRequests),
+                ].join("\n"),
+              });
+            }
+            if (noChangeRetryUsed) {
+              researchAdditions.push({
+                target: ["research" as const],
+                title: "Do not declare this ticket already resolved",
+                content: [
+                  "A human requested changes in the PR review feedback above, and the previous research pass wrongly concluded no change was needed.",
+                  "Treat addressing every point of that review feedback as the task: produce an implementation plan for it, declare the writeRepositories it touches, and do not set noChangeNeeded.",
+                ].join("\n"),
+              });
+            }
             const researchContext = {
               ticket: resolveAgentTicketInput(resolvedInputs, ticketData, ctx.clarifications),
               branchName,
               attachments: downloadedAttachments,
-              preSandboxAdditions:
-                ctx.repositoryExpansion.priorRequests.length > 0
-                  ? [
-                      ...ctx.preSandboxAdditions.research,
-                      {
-                        target: ["research" as const],
-                        title: "Repository expansion history",
-                        content: [
-                          "The following repositories were requested and are now attached.",
-                          "Continue the same research; do not restart from assumptions.",
-                          JSON.stringify(ctx.repositoryExpansion.priorRequests),
-                        ].join("\n"),
-                      },
-                    ]
-                  : ctx.preSandboxAdditions.research,
+              preSandboxAdditions: researchAdditions,
               repositoryContexts: ctx.repositoryContexts,
               workspaceManifest: ctx.workspaceManifest ?? undefined,
             };
@@ -4748,14 +4793,31 @@ async function agentWorkflowBody(
 
             // An already resolved ticket (fix landed in an earlier commit, PR,
             // or ticket comment) ends the run here as a successful no-op: there
-            // is nothing for any downstream block to write. All three signals
-            // must agree, so a half-filled signal keeps the normal plan path and
-            // its researchDeclaredNoWritesGuard verdict untouched.
-            const noChangeSignal =
-              research.noChangeNeeded === true &&
-              (research.resolutionEvidence ?? []).length > 0 &&
-              (research.writeRepositories ?? []).length === 0;
-            if (noChangeSignal) {
+            // is nothing for any downstream block to write. A half-filled
+            // signal keeps the normal plan path and its
+            // researchDeclaredNoWritesGuard verdict untouched. When the
+            // ticket's own PR carries human review feedback, that request is
+            // the task, so the exit is refused: one corrective research retry,
+            // then a hard fail instead of a false success.
+            const noChangeAction = resolveNoChangeAction(
+              research,
+              ctx.repositoryContexts,
+              noChangeRetryUsed,
+            );
+            if (noChangeAction === "retry") {
+              console.warn(
+                "[agent] research declared no_change_needed despite pending PR review feedback; retrying research once with a corrective note",
+              );
+              noChangeRetryUsed = true;
+              continue;
+            }
+            if (noChangeAction === "fail") {
+              return executionError(
+                "research declared no change needed but the ticket's PR has unresolved human review feedback; refusing the no_change_needed exit",
+                { category: "engine", phase: "research" },
+              );
+            }
+            if (noChangeAction === "no_change") {
               // Ticket-bound side effects only, exactly like the terminate
               // dispatch: an uncorrelated entry has no ticket to comment on,
               // move, or notify about.