fix(FN-7561): stop Plan Review replan loop and fix "can't find the plan" reviews
The Plan Review pre-merge gate could loop a task through triage↔plan-review indefinitely (FN-7525 ran 13+ replans overnight with no operator visibility), and its reviewer frequently produced "no PROMPT.md found / data lives in a DB" non-verdicts that fed the loop. Root cause of the non-verdicts: the reviewer runs readonly with cwd set to the task worktree, but the spec lives at project-root .fusion/tasks/<id>/PROMPT.md — outside the worktree — so telling it to "Read PROMPT.md" had it search the wrong tree and give up. Four fixes: 1. Inject the PROMPT.md content (via readTaskArtifact, store-backed) directly into the Plan Review reviewer prompt so the verdict never depends on the agent locating the file. 2. Self-retry a malformed reviewer response once on the primary model when no fallback model is configured, so a single fumbled response gets a second chance instead of feeding the replan loop. 3. A malformed (advisory_failure, no parsed verdict) plan-review result can never trigger a triage replan — it is an infra failure, not a plan defect. 4. Cap the unbounded plan-review replan default at 15 attempts; past the cap it emits a loud halting log entry and leaves the task for a human instead of looping forever. Explicit numeric operator budgets are unchanged. Tests: cap halts at 15 / still replans at 14 / malformed never replans. Existing Plan Review replan and malformed-verdict-gate tests still pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
7
.changeset/fn-7561-plan-review-replan-loop.md
Normal file
7
.changeset/fn-7561-plan-review-replan-loop.md
Normal file
@@ -0,0 +1,7 @@
|
||||
---
|
||||
"@runfusion/fusion": patch
|
||||
---
|
||||
|
||||
summary: Stop Plan Review from looping tasks forever and fix its "can't find the plan" reviews.
|
||||
category: fix
|
||||
dev: FN-7561 — Plan Review pre-merge gate hardening in packages/engine/src/executor.ts. (1) The reviewer ran readonly with cwd=worktree but the spec lives at project-root .fusion/tasks/<id>/PROMPT.md, so "Read PROMPT.md" produced "no PROMPT.md found / data is in a DB" non-verdicts; the spec text is now injected into the reviewer prompt via readTaskArtifact. (2) A malformed reviewer response now self-retries once on the primary model when no fallback is configured. (3) A malformed (advisory_failure, no verdict) plan-review result can never trigger a triage replan. (4) The unbounded plan-review replan default is capped at 15 attempts with a loud halting log entry, so a persistently-disagreeing planner/reviewer no longer burns LLM calls indefinitely (FN-7525 ran 13+ attempts overnight).
|
||||
@@ -219,6 +219,90 @@ describe("TaskExecutor pre-merge optional-step fix seam", () => {
|
||||
expect(cappedStore.moveTask).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
/*
|
||||
* FN-7561: the unbounded Plan Review replan default must still stop at a finite
|
||||
* safety ceiling. Below the cap it keeps replanning; at the cap it halts with a
|
||||
* loud log entry and leaves the task for a human instead of looping forever
|
||||
* (FN-7525 ran 13+ attempts overnight with no operator visibility).
|
||||
*/
|
||||
it("keeps replanning an unbounded Plan Review loop just below the safety cap", async () => {
|
||||
const store = createMockStore();
|
||||
const belowLog = Array.from({ length: 14 }, (_, i) => revisionLog("Plan Review", "plan-review", i + 1));
|
||||
const loopingTask = task({ postReviewFixCount: 14, column: "in-progress", log: belowLog });
|
||||
store.getTask.mockResolvedValue(loopingTask);
|
||||
store.getSettings.mockResolvedValue({ maxPostReviewFixes: 9 }); // no planReviewMaxRevisions → unbounded
|
||||
const executor = new TaskExecutor(store, "/tmp/test");
|
||||
|
||||
await expect((executor as any).requestPreMergeOptionalStepFix(loopingTask.id, loopingTask, {
|
||||
stepName: "Plan Review",
|
||||
feedback: "one more disagreement",
|
||||
phase: "pre-merge" as const,
|
||||
status: "failed" as const,
|
||||
verdict: "REVISE",
|
||||
nodeId: "plan-review",
|
||||
maxRevisions: "unbounded",
|
||||
})).resolves.toBe(true);
|
||||
|
||||
expect(store.moveTask).toHaveBeenCalledWith("FN-7066", "triage");
|
||||
expect(store.logEntry).toHaveBeenCalledWith(
|
||||
"FN-7066",
|
||||
"Plan Review failed — moved to triage for automatic replan (attempt 15/unbounded)",
|
||||
expect.anything(),
|
||||
undefined,
|
||||
);
|
||||
});
|
||||
|
||||
it("halts the unbounded Plan Review replan loop at the safety cap and leaves the task for a human", async () => {
|
||||
const store = createMockStore();
|
||||
const cappedLog = Array.from({ length: 15 }, (_, i) => revisionLog("Plan Review", "plan-review", i + 1));
|
||||
const loopingTask = task({ postReviewFixCount: 15, column: "in-progress", log: cappedLog });
|
||||
store.getTask.mockResolvedValue(loopingTask);
|
||||
store.getSettings.mockResolvedValue({ maxPostReviewFixes: 9 }); // unbounded default
|
||||
const executor = new TaskExecutor(store, "/tmp/test");
|
||||
|
||||
await expect((executor as any).requestPreMergeOptionalStepFix(loopingTask.id, loopingTask, {
|
||||
stepName: "Plan Review",
|
||||
feedback: "still disagreeing after fifteen tries",
|
||||
phase: "pre-merge" as const,
|
||||
status: "failed" as const,
|
||||
verdict: "REVISE",
|
||||
nodeId: "plan-review",
|
||||
maxRevisions: "unbounded",
|
||||
})).resolves.toBe(false);
|
||||
|
||||
// Halted: no replan side effects.
|
||||
expect(store.moveTask).not.toHaveBeenCalled();
|
||||
expect(store.updateTask).not.toHaveBeenCalledWith("FN-7066", { postReviewFixCount: 16 }, undefined);
|
||||
// Loud, human-visible halt log.
|
||||
expect(store.logEntry).toHaveBeenCalledWith(
|
||||
"FN-7066",
|
||||
expect.stringContaining("Plan Review replan safety cap reached (15/15)"),
|
||||
expect.stringContaining("still disagreeing after fifteen tries"),
|
||||
undefined,
|
||||
);
|
||||
});
|
||||
|
||||
it("does not replan a malformed (advisory_failure, no verdict) Plan Review result", async () => {
|
||||
// FN-7561 invariant: a malformed reviewer response (no parseable verdict) is an
|
||||
// infra/formatting failure, not a plan defect, and must never bounce the task to triage.
|
||||
const store = createMockStore();
|
||||
const liveTask = task({ column: "in-progress" });
|
||||
store.getTask.mockResolvedValue(liveTask);
|
||||
store.getSettings.mockResolvedValue({ maxPostReviewFixes: 3 });
|
||||
const executor = new TaskExecutor(store, "/tmp/test");
|
||||
|
||||
await expect((executor as any).requestPreMergeOptionalStepFix(liveTask.id, liveTask, {
|
||||
stepName: "Plan Review",
|
||||
feedback: "unparseable reviewer output",
|
||||
phase: "pre-merge" as const,
|
||||
status: "advisory_failure" as const,
|
||||
verdict: undefined,
|
||||
nodeId: "plan-review",
|
||||
})).resolves.toBe(false);
|
||||
|
||||
expect(store.moveTask).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("clears stale pause-abort provenance silently before a fresh unpaused execution dispatch", async () => {
|
||||
const store = createMockStore();
|
||||
const liveTask = task({ column: "todo", paused: false, userPaused: false });
|
||||
|
||||
@@ -4209,6 +4209,11 @@ export class TaskExecutor {
|
||||
const liveTask = await this.store.getTask(taskId).catch(() => fallbackTask);
|
||||
const isPlanReview = info.nodeId === "plan-review" || info.stepName === "Plan Review";
|
||||
if (isPlanReview) {
|
||||
/*
|
||||
* FNXC:PlanReviewReplan 2026-07-05-17:32:
|
||||
* FN-7561: a malformed reviewer response arrives as `advisory_failure` with NO parsed verdict. That is an infra/formatting failure (e.g. the reviewer could not locate the spec, or fumbled its trailing JSON), not a plan defect — it must NEVER bounce the task to a triage replan. The graph already excludes malformed advisories from the fix handoff (shouldRequestPreMergeFix); this guard defends the explicit remediation-node path and any future caller so a malformed advisory can never drive the replan loop. A genuine REVISE (verdict === "REVISE", also carried as advisory_failure) still replans below.
|
||||
*/
|
||||
if (info.status === "advisory_failure" && info.verdict !== "REVISE") return false;
|
||||
if (info.verdict !== undefined && info.verdict !== "REVISE") return false;
|
||||
/*
|
||||
* FNXC:PlanReviewReplan 2026-06-29-00:41:
|
||||
@@ -4231,6 +4236,21 @@ export class TaskExecutor {
|
||||
const revisionKey = optionalStepRevisionKey(info.nodeId ?? "plan-review", info.stepName);
|
||||
const currentCount = countOptionalStepRevisionAttempts(liveTask, revisionKey, info.stepName);
|
||||
if (!budget.unbounded && currentCount >= budget.max) return false;
|
||||
/*
|
||||
* FNXC:PlanReviewReplanCap 2026-07-05-17:28:
|
||||
* FN-7561: an unset Plan Review revision budget resolves to "unbounded" (see FNXC:WorkflowRevisionBudget above), which by design skips the ceiling check — so a task whose planner and reviewer persistently disagree, or whose reviewer keeps hard-failing, replans triage↔plan-review forever, silently burning a triage + review LLM call every cycle (FN-7525 ran 13+ attempts overnight with zero operator visibility). Enforce a finite safety ceiling even when unbounded: once hit, emit a loud halting log entry and STOP replanning (return false) so the gate falls through to a visible failed/parked state a human can act on, instead of looping indefinitely. Explicit numeric operator budgets are still honored as-is above; this only backstops the unbounded DEFAULT.
|
||||
*/
|
||||
const PLAN_REVIEW_REPLAN_HARD_CAP = 15;
|
||||
if (budget.unbounded && currentCount >= PLAN_REVIEW_REPLAN_HARD_CAP) {
|
||||
await this.store.logEntry(
|
||||
taskId,
|
||||
`Plan Review replan safety cap reached (${currentCount}/${PLAN_REVIEW_REPLAN_HARD_CAP}) — halting automatic replan and leaving the task for human review`,
|
||||
`Plan Review requested another planning revision but the unbounded replan loop hit its safety ceiling of ${PLAN_REVIEW_REPLAN_HARD_CAP} attempts. This usually means the reviewer and planner disagree persistently, or the reviewer keeps failing to produce a verdict. The task is being left in place for a human to inspect rather than looping further. Latest feedback:\n${feedback}`,
|
||||
this.getRunContextFor(taskId),
|
||||
);
|
||||
executorLog.warn(`${taskId}: Plan Review replan safety cap (${PLAN_REVIEW_REPLAN_HARD_CAP}) reached after ${currentCount} attempts — halting automatic replan`);
|
||||
return false;
|
||||
}
|
||||
const nextCount = currentCount + 1;
|
||||
const totalFixCount = (liveTask.postReviewFixCount ?? 0) + 1;
|
||||
const budgetLabel = budget.unbounded ? "unbounded" : String(budget.max);
|
||||
@@ -14546,14 +14566,22 @@ ${scopeGuard}
|
||||
const requireExternalIntegrationEvidence =
|
||||
workflowStepMetadata.requireExternalIntegrationEvidence === true;
|
||||
|
||||
/*
|
||||
* FNXC:PlanReviewSpecInjection 2026-07-05-17:20:
|
||||
* FN-7561: the Plan Review reviewer runs readonly with cwd=worktree, but the spec artifact lives at the project root under `.fusion/tasks/<id>/PROMPT.md` — OUTSIDE the task worktree. Instructing the agent to "Read PROMPT.md" therefore had it search the worktree, fail to find the file, and emit "no PROMPT.md file was found / task data lives in a DB" prose instead of a parseable verdict. That malformed/hard-failed output fed the unbounded triage↔plan-review replan loop (FN-7525 looped 13+ times overnight; FN-7575 too). Load the spec text from the store (document layer → on-disk PROMPT.md) ONCE and inject it directly into the reviewer prompt so the verdict never depends on the agent locating the file. Read from the store, not fs, so it is correct regardless of worktree vs project-root layout.
|
||||
*/
|
||||
const planReviewSpecArtifact = isPlanReviewStep
|
||||
? await this.readTaskArtifact(task.id, "PROMPT.md")
|
||||
: undefined;
|
||||
const planReviewSpecText = typeof planReviewSpecArtifact === "string" ? planReviewSpecArtifact : "";
|
||||
|
||||
if (isPlanReviewStep && requireExternalIntegrationEvidence) {
|
||||
/*
|
||||
* FNXC:PlanValidation 2026-06-30-09:03:
|
||||
* Coding (per-step review) intentionally keeps external-integration evidence as a Plan Review gate. Enforce it here, not in triage, so only workflows that set `requireExternalIntegrationEvidence` block and failures route through the graph's normal plan-replan loop.
|
||||
*/
|
||||
const promptContent = await this.readTaskArtifact(task.id, "PROMPT.md");
|
||||
const evidenceGaps = detectExternalIntegrationEvidenceGaps({
|
||||
promptContent: typeof promptContent === "string" ? promptContent : "",
|
||||
promptContent: planReviewSpecText,
|
||||
});
|
||||
if (evidenceGaps.length > 0) {
|
||||
const diagnostic = formatExternalIntegrationEvidenceDiagnostic(evidenceGaps);
|
||||
@@ -14608,9 +14636,14 @@ ${scopeGuard}
|
||||
*/
|
||||
const scopeBlock = isPlanReviewStep
|
||||
? `Plan Review Scope:
|
||||
- Review the task plan artifact (PROMPT.md) and task metadata only.
|
||||
- Review the task plan artifact (PROMPT.md), reproduced verbatim below, and task metadata only.
|
||||
- The plan is embedded in this prompt — do NOT go looking for a PROMPT.md file in the worktree; it lives at the project root (\`.fusion/tasks/${task.id}/PROMPT.md\`), outside this worktree, so review the embedded copy.
|
||||
- Do NOT judge current implementation diffs, uncommitted worktree changes, or unrelated repository changes.
|
||||
- If PROMPT.md is internally consistent, complete, scoped, and verifiable, approve even when the worktree contains unrelated changes from another task.`
|
||||
- If the plan is internally consistent, complete, scoped, and verifiable, approve even when the worktree contains unrelated changes from another task.
|
||||
|
||||
--- BEGIN PROMPT.md ---
|
||||
${planReviewSpecText || `(The plan artifact could not be loaded into this prompt. Read it read-only from the project root at .fusion/tasks/${task.id}/PROMPT.md before judging; do not treat an unavailable artifact as a plan defect.)`}
|
||||
--- END PROMPT.md ---`
|
||||
: `Diff Scope (files changed by THIS task vs base):
|
||||
${scopeFileBlock}${diffShortstat ? `\nDiff stat: ${diffShortstat}` : ""}
|
||||
|
||||
@@ -15067,6 +15100,21 @@ You have access to the file system to review changes.${inlineFixBlock}${verdictB
|
||||
if (!primaryOutcome.timedOut && !primaryMalformed) return primaryOutcome;
|
||||
|
||||
if (!fallback) {
|
||||
/*
|
||||
* FNXC:ReviewLeniency 2026-07-05-17:24:
|
||||
* FN-7561: when NO fallback model is configured, a MALFORMED primary (unparseable verdict — a single fumbled response) still deserves one retry so a transient formatting fumble does not feed the plan-review replan loop. Self-retry once on the SAME primary model. Timeouts are NOT self-retried — they would likely just time out again and burn another full budget. If the self-retry is still malformed it is returned as a non-blocking advisory downstream.
|
||||
*/
|
||||
if (primaryMalformed && !primaryOutcome.timedOut) {
|
||||
executorLog.log(`${task.id}: workflow step '${workflowStep.name}' produced malformed output and no fallback is configured — retrying once on the primary model`);
|
||||
const retryOutcome = await runOnce(primaryProvider, primaryModelId, "primary-retry");
|
||||
const retryMalformed = (retryOutcome as { malformed?: boolean }).malformed === true;
|
||||
if (!retryMalformed) return retryOutcome;
|
||||
await this.store.logEntry(
|
||||
task.id,
|
||||
`Workflow step '${workflowStep.name}' produced malformed output on both the primary attempt and one self-retry — no fallback model configured (set settings.validatorFallbackProvider/Id or fallbackProvider/Id)`,
|
||||
);
|
||||
return retryOutcome;
|
||||
}
|
||||
const reason = primaryOutcome.timedOut ? "timed out" : "produced malformed output";
|
||||
executorLog.warn(`${task.id}: workflow step '${workflowStep.name}' ${reason} and no fallback model is configured`);
|
||||
await this.store.logEntry(
|
||||
|
||||
Reference in New Issue
Block a user