diff --git a/.changeset/plan-review-replan-cap-wiring.md b/.changeset/plan-review-replan-cap-wiring.md new file mode 100644 index 0000000000..9e55059065 --- /dev/null +++ b/.changeset/plan-review-replan-cap-wiring.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: The Plan Review replan cap setting now actually works; lowering it takes effect. +category: fix +dev: `planReviewReplanCap` was declared, validated, documented and editable in the Workflow Editor but read by nothing — the unbounded-default backstop was hardcoded to `PLAN_REVIEW_FEEDBACK_HISTORY_LIMIT`, a bound on how much reviewer prose is replayed into the next planning prompt, so two unrelated concerns shared one number and trimming prompt history would have silently tightened a safety ceiling. `requestPreMergeOptionalStepFix` now resolves the backstop from the setting, defaulting to the new `DEFAULT_PLAN_REVIEW_REPLAN_CAP` (15, the previously-effective value, so this is a pure re-wiring); `0` parks on the first REVISE. An explicit `planReviewMaxRevisions` / node `maxRevisions` budget remains a stricter, earlier gate. Also deletes the dead `PLAN_REVIEW_GATE_REPLAN_CAP` constant (an unread ceiling belonging to the U10-deleted triage gate) and ratchets it in `legacy-tombstones.test.ts`; `Task.planReviewReplanCount` is documented as legacy/never-written, with the live owner named. diff --git a/docs/settings-reference.md b/docs/settings-reference.md index 4143f1254b..2a3a2ed0b1 100644 --- a/docs/settings-reference.md +++ b/docs/settings-reference.md @@ -332,11 +332,12 @@ Actions. It has two tabs: workflow's authored default, enter a non-negative integer to cap attempts, or enter `0` to disable automatic revision for that path. Compound Engineering's authored Code Review default is two passes; most other built-in review loops - remain unbounded. The separate triage Plan Review - replan ceiling, `planReviewReplanCap`, controls consecutive pre-execution - `REVISE` → replan cycles before Fusion requests manual approval; leave it empty - to use the built-in engine default, or set a non-negative integer (including - `0`) for the workflow. `plannerOversightLevel` is the + remain unbounded. The separate Plan Review replan ceiling, + `planReviewReplanCap`, backstops that unbounded default: it bounds consecutive + `REVISE` → replan cycles before Fusion parks the task at `awaiting-approval`. + Leave it empty to use the built-in 15, or set a non-negative integer (`0` parks + on the first REVISE). An explicit `planReviewMaxRevisions` is a stricter, + earlier gate and takes precedence over it. `plannerOversightLevel` is the workflow-native planner oversight mode and accepts `off`, `observe`, `steer`, or `autonomous` (default). `plannerHeartbeatPatrolEnabled` controls idle/no-task heartbeat patrol task creation separately and defaults to `true`. Edits batch @@ -423,7 +424,7 @@ The built-in workflows also declare triage/spec policy settings that were **not* | `codeReviewMaxRevisions` | unset | Workflow-native Code Review remediation cap. Unset/empty uses the workflow's authored default (Compound Engineering: 2; most other built-ins: unbounded); a non-negative integer overrides the cap; `0` disables automatic Code Review remediation. | | `planReviewBlockingSeverity` | `high` | Minimum finding severity that lets Plan Review block execution. A `REVISE` carrying no finding at or above this level is recorded as `APPROVE_WITH_NOTES` and its findings are written into PROMPT.md as non-blocking `## Review Advisory Notes` instead of forcing another planning round. Values: `critical` (P0 only), `high` (P0+P1, the default), `medium`, `low`, or `any` to restore the previous behavior where every `REVISE` blocks. **Fails closed:** a `REVISE` with no structured findings, or with any finding that omits `severity`, still blocks — so prose-only and custom reviewers keep their full blocking power. | | `codeReviewBlockingSeverity` | `critical` | Same gate for Code Review, defaulting to `critical` (P0 only) because Code Review findings land against real code the implementer can address inline. Same values and the same fail-closed contract as `planReviewBlockingSeverity`. | -| `planReviewReplanCap` | unset | Workflow-native triage Plan Review replan ceiling. It bounds consecutive pre-execution Plan Review `REVISE` → replan cycles before manual approval; unset/empty uses the built-in engine default, and a non-negative integer (including `0`) overrides it. | +| `planReviewReplanCap` | unset (built-in 15) | Ceiling on consecutive Plan Review `REVISE` → replan cycles before the task parks at `awaiting-approval` with reason `plan-review-replan-cap`. Applies to the **unbounded default only** — an explicit `planReviewMaxRevisions` or node `maxRevisions` budget is a stricter, earlier gate and wins. Unset uses the built-in 15; `0` parks on the first REVISE. (Until 2026-08-10 nothing read this setting: the backstop was hardcoded to a prompt-history constant, so changing this value had no effect.) | | `plannerOversightLevel` | `autonomous` | Workflow-native planner oversight mode. `off` disables oversight; `observe` watches only; `steer` injects guidance or suggests revisions; `autonomous` enables bounded retry and targeted-fix recovery — but merge/PR progression and any destructive or external-service side effect ALWAYS require an explicit, recorded human confirmation before they run, even at `autonomous` (FN-7513's confirmation gate; see `docs/architecture.md` → "Planner overseer confirmation gate"). Tasks may set a nullable `Task.plannerOversightLevel` override (same four values) that wins over this workflow value when present; `null`/unset means "inherit the workflow value". `resolveEffectivePlannerOversightLevel` in `@fusion/core` computes the effective level (task override → workflow effective → `autonomous`). The per-task override is exposed in the dashboard as a "Planner oversight" selector (Inherit from workflow / Off / Observe / Steer / Autonomous recovery) in both the New Task dialog and Task Detail edit form, threaded through `createTask`/`updateTask` (FN-7515); the project/global default is set via the **Workflow Editor → Values** tab on the default workflow's `plannerOversightLevel` value, not in Project Settings. FN-7517 additionally exposes a quick inline oversight-level select in the Task Detail modal's meta-controls cluster (same `updateTask` override plumbing, no parallel path) plus manual nudge/stop-oversight/explain-current-action controls that call the overseer runtime directly — see `docs/dashboard-guide.md`. Engine read-site behavior beyond the FN-7513 confirmation gate remains follow-up work (FN-7510+). | | `plannerOversightNotificationLevel` | `important` | Workflow-native planner-overseer notification verbosity (FN-7518). `silent` suppresses overseer notifications; `errors` notifies only on failures/escalations; `important` (the default) notifies on interventions/recovery actions and errors; `all` notifies on every observation. Resolves through the generic `resolveEffectiveSettings` default path with no special-casing, alongside `plannerOversightLevel`. This is a declaration-only setting: the notification-emission gating that reads it lands downstream in FN-7519 (intervention timeline) and FN-7520 (run-audit/activity events). | | `plannerOverseerAdvisorEnabled` | `false` | Master switch for the planner overseer **session advisor** (live LLM transcript review). **Off by default.** When false, no second-model advisor runs regardless of model fields or `plannerOversightLevel`. Lifecycle stage watching, stall recovery, and merge confirmation are unaffected. | diff --git a/docs/workflow-steps.md b/docs/workflow-steps.md index 375e63e63e..9f97349f13 100644 --- a/docs/workflow-steps.md +++ b/docs/workflow-steps.md @@ -443,7 +443,7 @@ Built-in optional gates ship as inlined IR builders, not as a template catalog: - `builtin:coding` carries the `browser-verification` optional-group node (`builtin-browser-verification-group.ts`), default-off, so browser verification runs only for tasks whose `enabledWorkflowSteps` includes `browser-verification`. - `builtin:coding` and `builtin:stepwise-coding` carry the `plan-review` optional-group node (`builtin-plan-review-group.ts`), default-on, before `parse-steps` so the plan can be reviewed before execution begins even when a task has not persisted explicit optional-step ids. - The `code-review` optional-group node (`builtin-code-review-group.ts`) is the inlined default-on code-review gate. On default `builtin:coding`, this is the only final review surface before merge; it is effective by default even when no explicit optional-step ids are stored. On `builtin:stepwise-coding`, it remains a post-foreach optional final review gate before the workflow's final review seam. -- Plan Review/spec revision and most Code Review remediation loops are unbounded by default for built-in workflows. Compound Engineering instead defaults CE Code Review to two remediation passes so persistent findings park for operator action. Operators can override these authored defaults per workflow from the Workflow Editor **Settings → Values** tab with `planReviewMaxRevisions` and `codeReviewMaxRevisions`; leaving the value empty uses the workflow's authored default, a non-negative integer caps automatic revisions, and `0` disables automatic revision for that review path. Separately, `planReviewReplanCap` bounds consecutive **triage pre-execution** Plan Review `REVISE` → replan cycles before Fusion parks the task for an explicit human Approve or Reject decision. Leave that setting empty to use the built-in engine default; a non-negative integer (including `0`) overrides it. These values are editable for read-only built-ins without duplicating the workflow. +- Plan Review/spec revision and most Code Review remediation loops are unbounded by default for built-in workflows. Compound Engineering instead defaults CE Code Review to two remediation passes so persistent findings park for operator action. Operators can override these authored defaults per workflow from the Workflow Editor **Settings → Values** tab with `planReviewMaxRevisions` and `codeReviewMaxRevisions`; leaving the value empty uses the workflow's authored default, a non-negative integer caps automatic revisions, and `0` disables automatic revision for that review path. Separately, `planReviewReplanCap` backstops the **unbounded** Plan Review default: it bounds consecutive `REVISE` → replan cycles before Fusion parks the task at `awaiting-approval` (reason `plan-review-replan-cap`) for an explicit human Approve or Reject decision. An explicit `planReviewMaxRevisions` is a stricter, earlier gate and takes precedence. Leave the setting empty to use the built-in 15; `0` parks on the first REVISE. These values are editable for read-only built-ins without duplicating the workflow. - A workflow (for example compound-engineering) can add a **post-merge** optional-group node via the generic `postMergeOptionalGroupNode(...)` builder (`builtin-post-merge-group.ts`) — e.g. a `document` step that runs after merge. Create-time optional-step controls appear in the quick-add action row and the **New Task** dialog inline quick buttons for the active workflow. They resolve the workflow's optional-group nodes (plus plugin-contributed palette templates, see [Plugin-Contributed Steps](#plugin-contributed-steps)) into toggleable rows. Selecting **Fast** clears currently enabled optional steps and submits `enabledWorkflowSteps: []` even if optional-step metadata is still loading, but the dropdown stays available once loaded; any manual reselection before create is submitted as explicit ids and executes even on the Fast task. Workflows with no optional groups render no trigger and omit `enabledWorkflowSteps` unless the operator selects Fast, where the explicit empty array preserves the speed-first opt-out. Unknown or removed ids are skipped during resolution so stale selections never render blank controls or break workflow loading. diff --git a/packages/core/src/index.gate.ts b/packages/core/src/index.gate.ts index d47c3d3e94..b25a70fadf 100644 --- a/packages/core/src/index.gate.ts +++ b/packages/core/src/index.gate.ts @@ -294,6 +294,7 @@ export { BUILTIN_OVERSIGHT_SETTINGS, DEFAULT_PLANNER_OVERSEER_EXECUTOR_STUCK_AFTER_MS, DEFAULT_PLANNING_TIMEOUT_MS, + DEFAULT_PLAN_REVIEW_REPLAN_CAP, PLANNER_HEARTBEAT_PATROL_ENABLED_SETTING_ID, renderTriagePolicyPlaceholders, } from "./workflows/builtin-workflow-settings.js"; diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 82b1e2b019..c2fe4d7807 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -339,6 +339,7 @@ export { BUILTIN_OVERSIGHT_SETTINGS, DEFAULT_MAX_POST_REVIEW_FIXES, DEFAULT_PLANNING_TIMEOUT_MS, + DEFAULT_PLAN_REVIEW_REPLAN_CAP, DEFAULT_PLANNER_OVERSEER_EXECUTOR_STUCK_AFTER_MS, PLANNER_HEARTBEAT_PATROL_ENABLED_SETTING_ID, renderTriagePolicyPlaceholders, diff --git a/packages/core/src/types/settings/settings-scope.ts b/packages/core/src/types/settings/settings-scope.ts index 63034973cd..22afbab0d0 100644 --- a/packages/core/src/types/settings/settings-scope.ts +++ b/packages/core/src/types/settings/settings-scope.ts @@ -2446,6 +2446,14 @@ export interface Settings extends GlobalSettings, ProjectSettings { * is aborted and the failure consumes one attempt of the bounded planning retry budget. * Default: {@link DEFAULT_PLANNING_TIMEOUT_MS}. */ planningTimeoutMs?: number; + /** Ceiling on consecutive Plan Review REVISE → replan cycles before the task is parked at + * `awaiting-approval` with `awaitingApprovalReason: "plan-review-replan-cap"`. + * + * FNXC:PlanReviewReplan 2026-08-10-18:32: + * Workflow-native. Applies to the UNBOUNDED default only: an explicit `planReviewMaxRevisions` + * or node `maxRevisions` budget is a stricter, earlier gate and wins. Unset uses + * {@link DEFAULT_PLAN_REVIEW_REPLAN_CAP}; `0` parks on the first REVISE. */ + planReviewReplanCap?: number; /** Index signature for dynamic settings access */ [key: string]: unknown; } diff --git a/packages/core/src/types/task/task-core.ts b/packages/core/src/types/task/task-core.ts index 1da48dce21..c88d1f65b4 100644 --- a/packages/core/src/types/task/task-core.ts +++ b/packages/core/src/types/task/task-core.ts @@ -938,15 +938,21 @@ export interface Task { * Review defaults to unbounded recovery so ordinary REVISE feedback does not * terminal-fail the task. */ postReviewFixCount?: number; - /** Number of consecutive triage pre-execution Plan Review REVISE replans this task - * has consumed. Incremented by the triage Plan Review gate - * (packages/engine/src/triage.ts runPlanReviewBeforeExecution) each time it blocks - * execution with a REVISE verdict and routes the task back to `needs-replan`. When it - * reaches `PLAN_REVIEW_GATE_REPLAN_CAP` the task is escalated to `awaiting-approval` - * (awaitingApprovalReason `plan-review-replan-cap`) instead of replanning again, so a - * planner/reviewer disagreement can never loop forever. Reset when the gate passes - * (APPROVE) or on a manual retry. Distinct from `postReviewFixCount`, which bounds the - * executor graph's post-merge/advisory optional-step REVISE budget. */ + /** LEGACY — persisted but NEVER WRITTEN. Do not read it as a live signal. + * + * FNXC:PlanReviewReplan 2026-08-10-18:32: + * This counted consecutive Plan Review REVISE replans for the out-of-graph triage gate + * (`runPlanReviewBeforeExecution`), which U10/R4 deleted along with its `PLAN_REVIEW_GATE_REPLAN_CAP` + * ceiling. The column survived the deletion; its writer did not. It is still serialized and still + * cleared by the operator Retry reset (harmless, and dropping it would need a schema migration for + * no behavioral gain), but it is permanently 0 and must not be used to decide anything. + * + * The live owner is the graph: `requestPreMergeOptionalStepFix` budgets Plan Review replans off the + * PERSISTED workflow-step results (`countPlanReviewRevisionAttempts`) rather than a task column, and + * parks via `parkPlanReviewReplanCapExhausted` at either an explicit `planReviewMaxRevisions` / + * node `maxRevisions` budget or, for the unbounded default, `PLAN_REVIEW_FEEDBACK_HISTORY_LIMIT`. + * Distinct from `postReviewFixCount`, which is the aggregate observability counter for the same + * graph remediation loop. */ planReviewReplanCount?: number; /** Number of bounded recovery retry attempts for transient executor/triage failures. * Distinct from `mergeRetries` (merge-conflict-specific). Incremented by the @@ -1063,11 +1069,15 @@ export interface Task { * that legacy value as an ordinary manual plan-approval hold (Approve/Reject Plan render * normally). - * FNXC:PlanReviewReplan 2026-07-15-11:09: - * Live writer: triage Plan Review REVISE replan-cap escalation stamps - * `plan-review-replan-cap` when automatic REVISE replans hit PLAN_REVIEW_GATE_REPLAN_CAP. - * Dashboard badge/detail banner/notifications must surface that reason so operators know - * approval is required because Plan Review did not converge — not a generic require-all gate. + * FNXC:PlanReviewReplan 2026-08-10-18:32: + * Live writer: `parkPlanReviewReplanCapExhausted` (packages/engine/src/executor/), called from + * `requestPreMergeOptionalStepFix` when the graph's Plan Review replan budget is exhausted — + * either an explicit `planReviewMaxRevisions` / node `maxRevisions` value, or + * `PLAN_REVIEW_FEEDBACK_HISTORY_LIMIT` backstopping the unbounded default. (Supersedes the + * pre-U10 triage gate and its deleted `PLAN_REVIEW_GATE_REPLAN_CAP`; the reason string is + * unchanged so no dashboard/notification surface moved.) Dashboard badge/detail banner/ + * notifications must surface that reason so operators know approval is required because Plan + * Review did not converge — not a generic require-all gate. * * FNXC:PullRequestMerge 2026-08-09-05:07: * The PR merge queue stamps `merge-blocked-by-policy` for branch-protection holds. diff --git a/packages/core/src/workflows/builtin-workflow-settings.ts b/packages/core/src/workflows/builtin-workflow-settings.ts index 52947c33ff..5319cfda74 100644 --- a/packages/core/src/workflows/builtin-workflow-settings.ts +++ b/packages/core/src/workflows/builtin-workflow-settings.ts @@ -9,6 +9,21 @@ ran p50 12.7 / p90 39.5 / p99 105.7 minutes, so a tighter bound would abort legi for the restart, which is the churn this bound exists to remove. */ export const DEFAULT_PLANNING_TIMEOUT_MS = 5_400_000; + +/* +FNXC:PlanReviewReplan 2026-08-10-18:32: +Built-in ceiling for consecutive Plan Review REVISE -> replan cycles when the resolved revision budget +is unbounded, used when the `planReviewReplanCap` workflow value is unset. + +15 preserves the ceiling that was ACTUALLY in force before this constant existed. That backstop was +`PLAN_REVIEW_FEEDBACK_HISTORY_LIMIT` — a bound on how much reviewer PROSE is replayed into the next +planning prompt, whose own comment states it is "bounded independently of persistence and retry +accounting". Two unrelated concerns were sharing one number, so trimming the prompt history would have +silently tightened a safety ceiling. Splitting them is the point; the value is held at 15 so the split +is a pure re-wiring rather than a silent behavior change. Operators can now lower it, which is what the +`planReviewReplanCap` setting always claimed to do but never did — nothing read it. +*/ +export const DEFAULT_PLAN_REVIEW_REPLAN_CAP = 15; import { DEFAULT_CODE_REVIEW_BLOCKING_SEVERITY, DEFAULT_PLAN_REVIEW_BLOCKING_SEVERITY, diff --git a/packages/engine/src/__tests__/legacy-tombstones.test.ts b/packages/engine/src/__tests__/legacy-tombstones.test.ts index 7bd1add9e4..63276b1010 100644 --- a/packages/engine/src/__tests__/legacy-tombstones.test.ts +++ b/packages/engine/src/__tests__/legacy-tombstones.test.ts @@ -67,6 +67,15 @@ const DELETED_SYMBOLS: Array<{ symbol: string; why: string }> = [ { symbol: "graphCompletionInterceptors", why: "shared per-task mutable state replaced by an explicit graphCompletion callback" }, // Out-of-graph Plan Review gate (R4) — the graph owns Plan Review exclusively. { symbol: "runPlanReviewBeforeExecution", why: "triage and the graph raced on Plan Review; the graph owns it" }, + /* + FNXC:PlanReviewReplan 2026-08-10-18:32: + The deleted triage gate's replan ceiling. It outlived the gate as an unread constant whose companion + column (`planReviewReplanCount`) was persisted and reset but never incremented or compared — a + ceiling that enforced nothing while reading as live safety. The graph re-owns the cap in + `requestPreMergeOptionalStepFix` -> `parkPlanReviewReplanCapExhausted`, budgeted off persisted + workflow-step results. Re-adding this name would recreate the second, silent authority. + */ + { symbol: "PLAN_REVIEW_GATE_REPLAN_CAP", why: "an unread ceiling for a deleted gate; the graph's parkPlanReviewReplanCapExhausted owns the cap" }, // In-session step reviewer (U10 pt2) — a second review authority inside the implementation session. { symbol: "createReviewStepTool", why: "review gates are graph nodes; an in-session reviewer duplicated Plan Review" }, { symbol: "fn_review_step", why: "the deleted in-session review tool's name must not be re-injected" }, diff --git a/packages/engine/src/__tests__/workflow-graph-optional-step-fix.test.ts b/packages/engine/src/__tests__/workflow-graph-optional-step-fix.test.ts index 627b786e12..812b8e8fcd 100644 --- a/packages/engine/src/__tests__/workflow-graph-optional-step-fix.test.ts +++ b/packages/engine/src/__tests__/workflow-graph-optional-step-fix.test.ts @@ -618,6 +618,77 @@ describe("TaskExecutor pre-merge optional-step fix seam", () => { ); }); + /* + FNXC:PlanReviewReplan 2026-08-10-18:32: + `planReviewReplanCap` is operator-facing — declared, validated, documented and editable in the + Workflow Editor — and until now NOTHING read it: lowering the cap changed nothing. The unbounded + backstop was `PLAN_REVIEW_FEEDBACK_HISTORY_LIMIT`, a bound on how much reviewer PROSE is replayed + into the next planning prompt, so trimming prompt history would silently have tightened a safety + ceiling. These pin the setting as the live backstop, and `0` as "park on the first REVISE". + */ + it("honors a lowered planReviewReplanCap as the unbounded-default backstop", async () => { + const store = createMockStore(); + const loopingTask = task({ + postReviewFixCount: 3, + column: "in-progress", + log: Array.from({ length: 3 }, (_, i) => revisionLog("Plan Review", "plan-review", i + 1)), + workflowStepResults: [repeatedPlanReviewResult(4)], + }); + store.getTask.mockResolvedValue(loopingTask); + // Unbounded revision budget, but the operator lowered the replan backstop to 3. + store.getSettings.mockResolvedValue({ maxPostReviewFixes: 9, planReviewReplanCap: 3 }); + const executor = new TaskExecutor(store, "/tmp/test"); + + await expect((executor as any).requestPreMergeOptionalStepFix(loopingTask.id, loopingTask, { + stepName: "Plan Review", + feedback: "still disagreeing", + phase: "pre-merge" as const, + status: "failed" as const, + verdict: "REVISE", + nodeId: "plan-review", + maxRevisions: "unbounded", + })).resolves.toBe(true); + + expect(store.moveTask).not.toHaveBeenCalled(); + expect(store.updateTask).toHaveBeenCalledWith( + "FN-7066", + expect.objectContaining({ status: "awaiting-approval", awaitingApprovalReason: "plan-review-replan-cap" }), + undefined, + ); + // The park names the OPERATOR's cap, not the built-in default. + expect(store.logEntry).toHaveBeenCalledWith( + "FN-7066", + expect.stringContaining("Plan Review replan cap reached"), + expect.stringContaining("cap 3"), + undefined, + ); + }); + + it("treats planReviewReplanCap 0 as park-on-first-REVISE", async () => { + const store = createMockStore(); + const freshTask = task({ column: "in-progress", workflowStepResults: [repeatedPlanReviewResult(1)] }); + store.getTask.mockResolvedValue(freshTask); + store.getSettings.mockResolvedValue({ maxPostReviewFixes: 9, planReviewReplanCap: 0 }); + const executor = new TaskExecutor(store, "/tmp/test"); + + await expect((executor as any).requestPreMergeOptionalStepFix(freshTask.id, freshTask, { + stepName: "Plan Review", + feedback: "first revise", + phase: "pre-merge" as const, + status: "failed" as const, + verdict: "REVISE", + nodeId: "plan-review", + maxRevisions: "unbounded", + })).resolves.toBe(true); + + expect(store.moveTask).not.toHaveBeenCalled(); + expect(store.updateTask).toHaveBeenCalledWith( + "FN-7066", + expect.objectContaining({ status: "awaiting-approval", awaitingApprovalReason: "plan-review-replan-cap" }), + 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. diff --git a/packages/engine/src/executor/request-pre-merge-optional-step-fix.ts b/packages/engine/src/executor/request-pre-merge-optional-step-fix.ts index 2d8a4840ce..a906c426c0 100644 --- a/packages/engine/src/executor/request-pre-merge-optional-step-fix.ts +++ b/packages/engine/src/executor/request-pre-merge-optional-step-fix.ts @@ -5,6 +5,12 @@ * FNXC:WorkflowOptionalStepFix 2026-06-26-16:35: * Inline graph optional-step remediation consumes `postReviewFixCount` BEFORE calling `sendTaskBackForFix`, matching self-healing's budget-first ordering. Persistent optional-step REVISE loops are bounded by the resolved optional-group budget; `"unbounded"` intentionally skips the ceiling check so the step cycles until it returns APPROVE/APPROVE_WITH_NOTES or a human intervenes. * + * FNXC:PlanReviewReplan 2026-08-10-18:32: + * PLAN REVIEW IS THE EXCEPTION to the note above: its `"unbounded"` budget is backstopped by + * `planReviewReplanCap` (default `DEFAULT_PLAN_REVIEW_REPLAN_CAP`), so it parks at + * `awaiting-approval` rather than cycling until a human notices. Every other optional group still + * cycles freely when unbounded. + * * FNXC:WorkflowRevisionBudget 2026-06-30-20:48: * Live Plan Review/spec and Code Review remediation must honor explicit workflow setting values before node `maxRevisions`, and must treat unset values as unbounded for those two built-in review paths. Browser Verification keeps the existing `maxPostReviewFixes` fallback unless its node config explicitly changes it. * @@ -28,6 +34,7 @@ import type { Task, TaskStore, WorkflowReviewFinding, WorkflowStepResult as CoreWorkflowStepResult } from "@fusion/core"; import { DEFAULT_MAX_POST_REVIEW_FIXES, + DEFAULT_PLAN_REVIEW_REPLAN_CAP, hasPreMergeRemediationAutoMergeHold, PLAN_REVIEW_GROUP_ID, resolveOptionalReviewRevisionBudget, @@ -45,7 +52,6 @@ import { import { countPlanReviewRevisionAttempts, formatPlanReviewRevisionFeedback, - PLAN_REVIEW_FEEDBACK_HISTORY_LIMIT, } from "../plan-review-feedback-history.js"; import { executorLog } from "../logger.js"; import type { EngineRunContext } from "../util/run-audit.js"; @@ -225,13 +231,35 @@ export async function requestPreMergeOptionalStepFix( * 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 so the gate falls through to a visible failed/parked state a human can act on. Explicit numeric operator budgets are still honored as-is above; this only backstops the unbounded DEFAULT. */ - if (budget.unbounded && currentCount >= PLAN_REVIEW_FEEDBACK_HISTORY_LIMIT) { - // U3: the unbounded-default safety ceiling now parks awaiting-approval with - // the replan-cap reason (re-owned from the deleted triage gate) so the - // non-convergence surfaces to a human instead of silently sitting in place. + /* + * FNXC:PlanReviewReplan 2026-08-10-18:32: + * The unbounded-default backstop is now the `planReviewReplanCap` workflow setting, not + * `PLAN_REVIEW_FEEDBACK_HISTORY_LIMIT`. + * + * Two bugs in one line. First, `planReviewReplanCap` is operator-facing — declared, validated, + * documented in settings-reference.md and editable in the Workflow Editor — and NOTHING read it: + * an operator lowering the cap changed nothing. Second, the ceiling it should have been was a + * bound on how much reviewer PROSE is replayed into the next planning prompt, whose own comment + * says it is "bounded independently of persistence and retry accounting" — so trimming the prompt + * history would have silently tightened a safety ceiling, and two unrelated concerns shared one + * number. `DEFAULT_PLAN_REVIEW_REPLAN_CAP` holds the previously-effective 15 so splitting them is + * a pure re-wiring, not a silent behavior change. + * + * `0` is honored (park on the first REVISE), which is why the comparison is `>=` against a + * possibly-zero cap rather than a truthiness check. + */ + const unboundedReplanCap = typeof settings.planReviewReplanCap === "number" + && Number.isInteger(settings.planReviewReplanCap) + && settings.planReviewReplanCap >= 0 + ? settings.planReviewReplanCap + : DEFAULT_PLAN_REVIEW_REPLAN_CAP; + if (budget.unbounded && currentCount >= unboundedReplanCap) { + // U3: the unbounded-default safety ceiling parks awaiting-approval with the replan-cap reason + // (re-owned from the deleted triage gate) so non-convergence surfaces to a human instead of + // silently sitting in place. await deps.parkPlanReviewReplanCapExhausted( taskId, - String(PLAN_REVIEW_FEEDBACK_HISTORY_LIMIT), + String(unboundedReplanCap), currentCount, feedback, ); diff --git a/packages/engine/src/triage.ts b/packages/engine/src/triage.ts index dafa2a3bd0..d43de6a1b9 100644 --- a/packages/engine/src/triage.ts +++ b/packages/engine/src/triage.ts @@ -104,23 +104,26 @@ function isPlanningLifecycleLockTransportError(error: unknown): error is Error { } /* -FNXC:PlanReviewReplan 2026-07-13-00:00: -The triage pre-execution Plan Review gate (runPlanReviewBeforeExecution) routes a REVISE -verdict back to `needs-replan`, which re-plans and re-reviews. Without a ceiling, a planner -and reviewer that persistently disagree loop plan → Plan Review REVISE → replan forever -(observed on TC-002), and in `planApprovalMode: require-all` there is no human escape because -the task never reaches `awaiting-approval`. Bound the consecutive REVISE replans with a -cap (default 8, mirroring the executor graph's PLAN_REVIEW_REPLAN_HARD_CAP backstop): after -this many replans the gate escalates the task to `awaiting-approval` for a human decision -instead of replanning again. The counter (Task.planReviewReplanCount) resets when the gate passes. +FNXC:PlanReviewReplan 2026-08-10-18:32 (TOMBSTONE — do not re-add): +`PLAN_REVIEW_GATE_REPLAN_CAP = 8` is DELETED. It belonged to the out-of-graph triage Plan Review gate +(`runPlanReviewBeforeExecution`, itself tombstoned in U10/R4), and it did not survive that deletion as +working code: nothing read it, and its companion counter `Task.planReviewReplanCount` was persisted, +serialized and reset but never incremented and never compared. A constant and a column that look like +a live safety ceiling while enforcing nothing are worse than no ceiling at all — they answer "is this +loop bounded?" with a confident yes. -FNXC:PlanReviewReplan 2026-07-15-11:09: -Raise the automatic REVISE replan ceiling from 3 to 8 so planner/reviewer pairs get more -room to converge before escalation. When the cap is hit, the dashboard must still make the -approval reason explicit (awaitingApprovalReason `plan-review-replan-cap`) so operators know -this is a non-converging Plan Review loop, not a routine require-all plan gate. +The capability was NOT lost, only re-owned. U3 moved the cap-park into the graph, where +`requestPreMergeOptionalStepFix` enforces it against a per-step budget derived from the persisted +workflow-step results (`countPlanReviewRevisionAttempts`) rather than a task column: + - an explicit finite budget (`planReviewMaxRevisions`, node `maxRevisions`) parks at `budget.max`; + - the unbounded default is backstopped at `PLAN_REVIEW_FEEDBACK_HISTORY_LIMIT`; +both call `parkPlanReviewReplanCapExhausted`, which parks `awaiting-approval` with +`awaitingApprovalReason: "plan-review-replan-cap"` — the same reason string the dashboard badge, +detail banner and notifications already key on. That is the live owner; look there, not here. + +Ratcheted by `packages/engine/src/__tests__/legacy-tombstones.test.ts`, which strips comments before +searching, so this note survives while the constant must not. */ -export const PLAN_REVIEW_GATE_REPLAN_CAP = 8; export function inlineTaskListFallback( lines: string[],