diff --git a/.changeset/reporting-only-documentation-milestone.md b/.changeset/reporting-only-documentation-milestone.md new file mode 100644 index 0000000000..22f96ff371 --- /dev/null +++ b/.changeset/reporting-only-documentation-milestone.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Documentation now only documents — it can no longer hold a merge or send a card back with nothing to do. +category: fix +dev: Observed on a live card: the Documentation milestone returned an advisory REVISE, which recorded `advisory_failure`. `resolveRequiredPreMergeStepIds` included the group, so `evaluatePreMergeApprovals` read it as "not-approved" and held the merge door; the same REVISE also reached `requestPreMergeOptionalStepFix`, which bounced the card to `in-progress` where `sendTaskBackForFix` reopens nothing under the named-remediation policy — no pending step, foreach `already-expanded`, Code Review replayed over an unchanged tree, and the card merged when the second Documentation pass happened to pass. New opt-in `WorkflowOptionalGroupConfig.reportingOnly`, surfaced on `ResolvedWorkflowOptionalStep` and set only on `documentationDeliveryOptionalGroupNode`, excludes a reporting group from the required pre-merge approval set and refuses executor remediation for it. A general guard now also refuses any `stepReopenPolicy: "none"` bounce that appended no named steps, logging it on the card instead of looping. Code Review REVISE and the deterministic verification failure keep producing named fix steps; advisory gates that own remediation (browser verification) are untouched. diff --git a/packages/core/src/__tests__/builtin-coding-ideas-v2-workflow.test.ts b/packages/core/src/__tests__/builtin-coding-ideas-v2-workflow.test.ts index 17bdc9d152..2aa755ba77 100644 --- a/packages/core/src/__tests__/builtin-coding-ideas-v2-workflow.test.ts +++ b/packages/core/src/__tests__/builtin-coding-ideas-v2-workflow.test.ts @@ -139,6 +139,36 @@ describe("builtin:coding-ideas-v2", () => { } }); + /* + FNXC:ReportingOnlyGroup 2026-08-26-06:56: + The advisory failure edge above was NOT enough, measured on a real card: Documentation's REVISE + recorded `advisory_failure`, which the required-approval set read as "no current approval" — so the + reporter held the merge door shut — while the same REVISE also bounced the card to implementation. + `reportingOnly` states the contract once, and BOTH doors read it. + */ + it("cannot hold the merge: Documentation carries no approval", () => { + const documentation = BUILTIN_CODING_IDEAS_V2_WORKFLOW_IR.nodes.find((node) => node.id === "documentation-delivery"); + expect(documentation?.config?.reportingOnly).toBe(true); + + const required = resolveRequiredPreMergeStepIds(BUILTIN_CODING_IDEAS_V2_WORKFLOW_IR, undefined); + expect([...required].sort()).toEqual(["code-review", "plan-review"]); + expect(required.has("documentation-delivery"), "a reporter must never gate the merge").toBe(false); + + // Enabling it explicitly must not turn it into a gate either. + expect(resolveRequiredPreMergeStepIds( + BUILTIN_CODING_IDEAS_V2_WORKFLOW_IR, + ["plan-review", "code-review", "documentation-delivery"], + ).has("documentation-delivery")).toBe(false); + + // The gates that DO carry approval are untouched, here and on the inherited board. + expect([...resolveRequiredPreMergeStepIds(BUILTIN_CODING_IDEAS_WORKFLOW_IR, undefined)].sort()) + .toEqual(["code-review", "plan-review"]); + for (const groupId of ["plan-review", "code-review"]) { + expect(BUILTIN_CODING_IDEAS_V2_WORKFLOW_IR.nodes.find((node) => node.id === groupId)?.config?.reportingOnly) + .toBeUndefined(); + } + }); + it("returns a rejected review to in-progress as named work", () => { expect(BUILTIN_CODING_IDEAS_V2_WORKFLOW_IR.edges).toEqual(expect.arrayContaining([ { from: "code-review", to: "code-review-remediation", condition: "failure" }, diff --git a/packages/core/src/index.gate.ts b/packages/core/src/index.gate.ts index ec36574efd..e303bfb676 100644 --- a/packages/core/src/index.gate.ts +++ b/packages/core/src/index.gate.ts @@ -293,6 +293,7 @@ export { resolveWorkflowOptionalSteps, resolveDefaultOnOptionalGroupIds, isWorkflowOptionalGroupEnabled, + isReportingOnlyOptionalGroup, } from "./workflows/workflow-optional-steps.js"; export type { ResolvedWorkflowOptionalStep } from "./workflows/workflow-optional-steps.js"; export { resolveRequiredPreMergeStepIds, resolvePreMergeGateForTask } from "./merge/required-pre-merge-steps.js"; diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 53c156277c..cbbf0e56f7 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -349,6 +349,7 @@ export { resolveWorkflowOptionalSteps, resolveDefaultOnOptionalGroupIds, isWorkflowOptionalGroupEnabled, + isReportingOnlyOptionalGroup, } from "./workflows/workflow-optional-steps.js"; export type { ResolvedWorkflowOptionalStep } from "./workflows/workflow-optional-steps.js"; export { resolveRequiredPreMergeStepIds, resolvePreMergeGateForTask } from "./merge/required-pre-merge-steps.js"; diff --git a/packages/core/src/merge/required-pre-merge-steps.ts b/packages/core/src/merge/required-pre-merge-steps.ts index 22f6facdad..2851d33362 100644 --- a/packages/core/src/merge/required-pre-merge-steps.ts +++ b/packages/core/src/merge/required-pre-merge-steps.ts @@ -23,6 +23,14 @@ export function resolveRequiredPreMergeStepIds( return new Set( resolveWorkflowOptionalSteps(ir) .filter((step) => step.phase === "pre-merge") + /* + FNXC:ReportingOnlyGroup 2026-08-26-06:56: + A reporting group records what it observed; it carries no approval, so requiring one from it can + only produce a false blocker. Measured: the Documentation milestone returned an advisory REVISE, + which records `advisory_failure`, which this set turned into "task has enabled pre-merge + workflow steps without a current approval" — a reporter holding the merge door shut. + */ + .filter((step) => !step.reportingOnly) .filter((step) => isWorkflowOptionalGroupEnabled(enabledWorkflowSteps, step.templateId, step.defaultOn)) .map((step) => step.templateId), ); diff --git a/packages/core/src/workflows/builtin-documentation-delivery-group.ts b/packages/core/src/workflows/builtin-documentation-delivery-group.ts index 493c98405a..15743a09b5 100644 --- a/packages/core/src/workflows/builtin-documentation-delivery-group.ts +++ b/packages/core/src/workflows/builtin-documentation-delivery-group.ts @@ -32,6 +32,15 @@ export function documentationDeliveryOptionalGroupNode(column: string): Workflow config: { name: "Documentation", defaultOn: true, + /* + FNXC:ReportingOnlyGroup 2026-08-26-06:56: + Documentation ONLY documents. `gateMode: "advisory"` on the inner node was not enough and the + gap was measured on a real card: its REVISE recorded `advisory_failure`, which held the merge + door ("no current approval") AND bounced the card to implementation with no work to do, where + it re-ran Code Review against an unchanged tree and merged on the second pass by luck. + `reportingOnly` states the contract once: no approval to withhold, no remediation to request. + */ + reportingOnly: true, template: { nodes: [{ id: "documentation-delivery-step", diff --git a/packages/core/src/workflows/workflow-ir-types.ts b/packages/core/src/workflows/workflow-ir-types.ts index dc3c96b09b..28dfabe604 100644 --- a/packages/core/src/workflows/workflow-ir-types.ts +++ b/packages/core/src/workflows/workflow-ir-types.ts @@ -261,6 +261,22 @@ export interface WorkflowOptionalGroupConfig { `WorkflowStepResult.phase` and `[post-merge]` logs follow this value. */ phase?: "pre-merge" | "post-merge"; + /* + FNXC:ReportingOnlyGroup 2026-08-26-06:56: + A REPORTING group observes accepted work and records what it found. It carries no approval and can + never hold or reopen a card, so it is excluded from the required pre-merge approval set and may + not schedule executor remediation. + + `gateMode: "advisory"` on the inner node was NOT enough, and the gap was measured on a real card: + an advisory REVISE records `advisory_failure`, which `evaluatePreMergeApprovals` reads as + "not-approved" — so the merge door held — AND still reached `requestPreMergeOptionalStepFix`, which + bounced the card to implementation. The Documentation milestone therefore both blocked the merge + and demanded work, while its own contract says it reports and never vetoes. + + Deliberately opt-in and absent everywhere else: advisory gates that DO own remediation (browser + verification) keep their existing blocking behaviour untouched. + */ + reportingOnly?: boolean; template: { nodes: WorkflowIrNode[]; edges: WorkflowIrEdge[]; diff --git a/packages/core/src/workflows/workflow-optional-steps.ts b/packages/core/src/workflows/workflow-optional-steps.ts index 570d5b57d8..8a5a9c01d7 100644 --- a/packages/core/src/workflows/workflow-optional-steps.ts +++ b/packages/core/src/workflows/workflow-optional-steps.ts @@ -12,6 +12,13 @@ export interface ResolvedWorkflowOptionalStep { icon?: string; phase: NonNullable; defaultOn: boolean; + /* + FNXC:ReportingOnlyGroup 2026-08-26-06:56: + A reporting group observes accepted work: it holds no approval and never reopens implementation. + Surfaced here so merge admission and the remediation seam read ONE declaration instead of each + re-deriving "is this thing allowed to stop the card". + */ + reportingOnly: boolean; } /** Resolve one optional group's effective state consistently across runtime and @@ -114,6 +121,7 @@ export function resolveWorkflowOptionalSteps( */ phase: config.phase === "post-merge" ? "post-merge" : "pre-merge", defaultOn: config.defaultOn === true, + reportingOnly: config.reportingOnly === true, /* FNXC:WorkflowDefinitionSteps 2026-06-29-00:41: Definition/task creation surfaces must order optional groups by graph execution position, not raw node-array order. Derived built-ins can insert Plan Review between planning and parse while appending its node object, and operators still need the step list to show Plan Review before execution. @@ -146,3 +154,14 @@ Every optional-group node id in a workflow, regardless of `defaultOn`. These ids export function resolveAllOptionalGroupIds(ir: WorkflowIr): string[] { return resolveWorkflowOptionalSteps(ir).map((step) => step.templateId); } + +/* +FNXC:ReportingOnlyGroup 2026-08-26-06:56: +Resolve whether a node id names a REPORTING optional group — one that observes accepted work and +records what it found, holding no approval and owning no remediation. Shared by merge admission and +the remediation seam so "is this allowed to stop the card" has exactly one answer. +*/ +export function isReportingOnlyOptionalGroup(ir: WorkflowIr, nodeId: string | undefined): boolean { + if (!nodeId) return false; + return resolveWorkflowOptionalSteps(ir).some((step) => step.templateId === nodeId && step.reportingOnly); +} diff --git a/packages/engine/src/__tests__/fix-steps-from-failed-gates.test.ts b/packages/engine/src/__tests__/fix-steps-from-failed-gates.test.ts index af95058ad2..bafe470167 100644 --- a/packages/engine/src/__tests__/fix-steps-from-failed-gates.test.ts +++ b/packages/engine/src/__tests__/fix-steps-from-failed-gates.test.ts @@ -121,6 +121,28 @@ describe("fix steps appear on the card when a gate fails", () => { expect(sendTaskBackForFix).toHaveBeenCalledTimes(1); }); + /* + FNXC:VerificationRemediation 2026-08-26-06:31: + `performWorkflowRerunBounce` PERSISTS the bounce path onto `task.worktree`. A caller holding the + live checkout must therefore hand it over rather than let an unset task record supply "": that + would wipe the pointer the remediation is about to run in, render the card "Unassigned", and stop + self-healing reclaiming the worktree as idle. + */ + it("bounces into the checkout it was given, not an unset task record", async () => { + const { task, sendTaskBackForFix, store } = harness(); + task.worktree = undefined; + + await appendReviewRemediationSteps( + { store: store as never, readTaskArtifact: async () => task.prompt, sendTaskBackForFix }, + task, + { stepName: "Verification (test)", feedback: FAILING_OUTPUT, phase: "pre-merge", status: "failed", nodeId: "verification" }, + { worktreePath: "/tmp/live-checkout" }, + ); + + expect(sendTaskBackForFix).toHaveBeenCalledTimes(1); + expect(sendTaskBackForFix.mock.calls[0]?.[1]).toBe("/tmp/live-checkout"); + }); + it("turns a Code Review REVISE into named work on the card", async () => { const { deps, task, pending, sendTaskBackForFix } = harness(); @@ -193,6 +215,57 @@ describe("fix steps appear on the card when a gate fails", () => { expect(sendTaskBackForFix).not.toHaveBeenCalled(); }); + /* + FNXC:ReportingOnlyGroup 2026-08-26-06:56: + Documentation only documents. Measured on a real card (mult-021): its advisory REVISE asked for + implementation work, this seam bounced the card to `in-progress`, and under this workflow's + named-remediation policy `sendTaskBackForFix` reopened NOTHING — no pending step, foreach + `already-expanded`, Code Review replayed over an unchanged tree, and the card merged when the + second Documentation pass happened to pass. The demand was never implemented and two model calls + were spent proving nothing. + */ + it("records Documentation feedback without reopening implementation", async () => { + const { deps, task, store, pending, sendTaskBackForFix } = harness(); + + const scheduled = await requestPreMergeOptionalStepFix(deps as never, task.id, task, { + stepName: "Documentation", + feedback: "Implement the scoped removal and absence regression contract before documenting completion.", + phase: "pre-merge", + status: "advisory_failure", + verdict: "REVISE", + nodeId: "documentation-delivery", + }); + + expect(scheduled, "a reporter schedules no work").toBe(false); + expect(pending()).toHaveLength(0); + expect(sendTaskBackForFix, "the card must not bounce with an empty step list").not.toHaveBeenCalled(); + // The feedback still reaches the operator on the card. + expect(store.logEntry.mock.calls.some(([, title]) => String(title).includes("cannot reopen implementation"))).toBe(true); + }); + + /* + FNXC:EmptyBounceGuard 2026-08-26-06:56: + The general invariant behind that fix: under named-remediation policy, only the gates that can + APPEND work may bounce. Any other node reaching the bounce would send the card back with nothing + to do, which is indistinguishable from a hang and re-reviews an unchanged tree on a loop. + */ + it("refuses to bounce a card for a gate that owns no remediation", async () => { + const { deps, task, pending, sendTaskBackForFix } = harness(); + + const scheduled = await requestPreMergeOptionalStepFix(deps as never, task.id, task, { + stepName: "Some Custom Gate", + feedback: "please change something", + phase: "pre-merge", + status: "failed", + verdict: "REVISE", + nodeId: "some-custom-gate", + }); + + expect(scheduled).toBe(false); + expect(pending()).toHaveLength(0); + expect(sendTaskBackForFix).not.toHaveBeenCalled(); + }); + /* The appended shape is selected by the WORKFLOW, not by this seam: Coding (Ideas) reopens its trailing step instead, and must keep doing so. diff --git a/packages/engine/src/__tests__/verification-failure-named-remediation.test.ts b/packages/engine/src/__tests__/verification-failure-named-remediation.test.ts index 4276dccb1d..9f8b81473b 100644 --- a/packages/engine/src/__tests__/verification-failure-named-remediation.test.ts +++ b/packages/engine/src/__tests__/verification-failure-named-remediation.test.ts @@ -80,6 +80,14 @@ describe("deterministic verification failure → named remediation", () => { status: "failed", phase: "pre-merge", }), + /* + FNXC:VerificationRemediation 2026-08-26-06:31: + The checkout this gate just verified is handed over explicitly. `performWorkflowRerunBounce` + PERSISTS the path it receives onto `task.worktree`, so falling back to an empty task record + would wipe the pointer the remediation is about to run in — the card renders "Unassigned" and + self-healing can no longer reclaim the worktree. The legacy bounce below always passed it. + */ + { worktreePath: "/tmp/fn-vr-1" }, ); // Remediation performs the bounce itself; a second one would double-dispatch the executor. expect(deps.sendTaskBackForFix).not.toHaveBeenCalled(); diff --git a/packages/engine/src/executor/append-review-remediation-steps.ts b/packages/engine/src/executor/append-review-remediation-steps.ts index 186d2a67bb..d3c165b9ac 100644 --- a/packages/engine/src/executor/append-review-remediation-steps.ts +++ b/packages/engine/src/executor/append-review-remediation-steps.ts @@ -14,10 +14,20 @@ export type AppendReviewRemediationStepsDeps = { * refuses a blind return to implementation: no candidate, out-of-scope evidence, duplicate-only * work, or the fourth wave is a human hold rather than an empty executor dispatch. */ +/* +FNXC:VerificationRemediation 2026-08-26-06:31: +`worktreePath` lets a caller that already HOLDS the live checkout hand it in instead of falling back +to `task.worktree`. The executor's deterministic-verification gate is such a caller, and the fallback +is not safe for it: `performWorkflowRerunBounce` persists whatever path it receives back onto +`task.worktree`, so an empty fallback WIPES the pointer — the card renders "Unassigned" and +self-healing can no longer reclaim the worktree as idle. Graph-driven callers (the Code Review +remediation node) have no such path in hand and keep the task-record fallback. +*/ export async function appendReviewRemediationSteps( deps: AppendReviewRemediationStepsDeps, task: Task, info: RequestPreMergeOptionalStepFixInfo, + options: { worktreePath?: string } = {}, ): Promise { const gate = info.nodeId === "verification" ? "Verification" : info.nodeId === "code-review" ? "Code Review" : undefined; if (!gate) return false; @@ -47,7 +57,7 @@ export async function appendReviewRemediationSteps( await widenPromptFileScope(deps.store, task.id, prompt, remediationDeclaredFiles(appended.appended)); await deps.sendTaskBackForFix( live, - live.worktree ?? "", + options.worktreePath?.trim() || live.worktree || "", info.feedback, info.stepName, `Review gate ${gate} requested named remediation`, diff --git a/packages/engine/src/executor/bounce-verification-failure.ts b/packages/engine/src/executor/bounce-verification-failure.ts index 6411299bb2..65d3a4acf8 100644 --- a/packages/engine/src/executor/bounce-verification-failure.ts +++ b/packages/engine/src/executor/bounce-verification-failure.ts @@ -53,6 +53,7 @@ export type BounceVerificationFailureDeps = { status: "failed"; nodeId: string; }, + options?: { worktreePath?: string }, ) => Promise; sendTaskBackForFix: ( task: Task, @@ -96,13 +97,22 @@ export async function bounceVerificationFailure( * classify the executor's own just-written files as upstream work and park instead of fixing. */ const liveTask = await deps.store.getTask(task.id).catch(() => task); - const appended = await deps.appendReviewRemediationSteps(liveTask ?? task, { - stepName, - feedback, - phase: "pre-merge", - status: "failed", - nodeId: "verification", - }); + /* + * Hand over the checkout this gate just verified. Falling back to `task.worktree` would let an + * empty pointer reach `performWorkflowRerunBounce`, which persists it — wiping the worktree the + * remediation is about to run in. The legacy branch below has always passed this same path. + */ + const appended = await deps.appendReviewRemediationSteps( + liveTask ?? task, + { + stepName, + feedback, + phase: "pre-merge", + status: "failed", + nodeId: "verification", + }, + { worktreePath }, + ); if (appended) return "named-remediation"; /* Parked: drop the completed-task watchdog the bounce would otherwise have cleared. */ deps.clearCompletedTaskWatchdog(task.id); 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 2b45f027f8..5dd64d2083 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 @@ -40,6 +40,7 @@ import { resolveOptionalReviewRevisionBudget, resolveOptionalStepRevisionBudget, resolveStepReopenPolicy, + isReportingOnlyOptionalGroup, resolveWorkflowIrForTask, } from "@fusion/core"; import { mergeEffectiveSettings } from "../project/effective-settings.js"; @@ -383,6 +384,52 @@ export async function requestPreMergeOptionalStepFix( return deps.appendReviewRemediationSteps(liveTask, info); } + /* + FNXC:ReportingOnlyGroup 2026-08-26-06:56: + A REPORTING group cannot reopen implementation. It observes accepted work and records what it + found; it has no verdict to enforce, so a revision request from it is feedback, not a work order. + + Measured on a real card: the Documentation milestone returned an advisory REVISE asking for + implementation work. This seam bounced the card to `in-progress` — where, under this workflow's + named-remediation policy, `sendTaskBackForFix` reopens NOTHING. With no pending step the foreach + answered `already-expanded`, the walk replayed Code Review over an UNCHANGED tree, and the card + merged when the second Documentation pass happened to pass. The reviewer's demand was never + implemented, and 2 model calls plus a worktree acquisition were spent proving nothing. + + Returning false routes traversal down the node's own failure edge, which for a reporter reaches + the merge gate — the behaviour its contract always described. The feedback stays on the card. + */ + if (workflowIr && isReportingOnlyOptionalGroup(workflowIr, info.nodeId)) { + await deps.store.logEntry( + taskId, + `${info.stepName} recorded feedback but cannot reopen implementation`, + `${info.stepName} reports on accepted work and carries no approval, so no executor remediation was scheduled. Feedback:\n${info.feedback}`, + deps.getRunContextFor(taskId), + ); + return false; + } + + /* + FNXC:EmptyBounceGuard 2026-08-26-06:56: + Last line of defence for the named-remediation policy: under `none`, `sendTaskBackForFix` reopens + no step, so every bounce that did not append named work is a card sent back with nothing to do. + The two paths that CAN append have already returned above; anything still here would bounce empty. + Refuse visibly instead — a parked card an operator can see beats a silent loop that re-reviews an + unchanged tree until a non-deterministic verdict lets it through. + */ + if (resolveStepReopenPolicy(workflowIr) === "none") { + executorLog.warn( + `${taskId}: pre-merge remediation NOT scheduled for step "${info.stepName}" — this workflow appends named remediation steps, and no gate owns that for node "${info.nodeId ?? "unknown"}". Card left parked.`, + ); + await deps.store.logEntry( + taskId, + `${info.stepName} requested changes but no remediation owner exists for it`, + `This workflow reopens implementation only through named remediation steps, which are produced by the Verification and Code Review gates. Node "${info.nodeId ?? "unknown"}" has no such owner, so the card was not bounced with an empty step list. Feedback:\n${info.feedback}`, + deps.getRunContextFor(taskId), + ); + return false; + } + if (info.verdict !== "REVISE") { // FNXC:RemediationVisibility 2026-07-26-19:20: a hard-failed gate with no parsed REVISE // verdict schedules nothing, so the remediation node fails and the card parks. Say so.