fix(engine): wire the Plan Review replan cap and tombstone its dead predecessor
Investigating the "dead cap" turned up the opposite of what it looked like, plus a worse problem next to it. The Plan Review replan loop was NOT unbounded. U3 re-owned the cap-park in the graph: requestPreMergeOptionalStepFix parks via parkPlanReviewReplanCapExhausted at awaiting-approval with reason plan-review-replan-cap, on both an explicit finite budget and the unbounded default. That capability has been live throughout. What was actually dead: 1. PLAN_REVIEW_GATE_REPLAN_CAP = 8 — an unread constant belonging to the out-of-graph triage gate (runPlanReviewBeforeExecution) that U10/R4 deleted. Its companion column Task.planReviewReplanCount was persisted, serialized and reset but never incremented or compared. A constant and a column that look like a live safety ceiling while enforcing nothing are worse than no ceiling: they answer "is this loop bounded?" with a confident yes. Deleted, ratcheted in legacy-tombstones.test.ts, and the column documented as legacy/never-written with the live owner named. 2. planReviewReplanCap — an operator-facing setting, declared, validated, documented in settings-reference.md and editable in the Workflow Editor, that NOTHING read. Lowering it changed nothing. The unbounded backstop was instead hardcoded to PLAN_REVIEW_FEEDBACK_HISTORY_LIMIT — 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". Two unrelated concerns shared one number, so trimming prompt history would have silently tightened a safety ceiling. The backstop now resolves from the setting, defaulting to the new DEFAULT_PLAN_REVIEW_REPLAN_CAP = 15 — the previously-effective value, so this is a pure re-wiring rather than a silent behavior change. The existing 15-attempt regression test passes unchanged, which is the evidence for that. 0 is honored as park-on-first-REVISE. An explicit planReviewMaxRevisions / node maxRevisions budget remains a stricter, earlier gate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
gsxdsm
7
.changeset/plan-review-replan-cap-wiring.md
Normal file
7
.changeset/plan-review-replan-cap-wiring.md
Normal file
@@ -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.
|
||||
@@ -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. |
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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";
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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" },
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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,
|
||||
);
|
||||
|
||||
@@ -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[],
|
||||
|
||||
Reference in New Issue
Block a user