fix(workflows): address review feedback on recovery and docs
This commit is contained in:
@@ -70,7 +70,7 @@ During initial investigation, the store-level explicit/default workflow paths al
|
||||
- AE2. Given the `builtin:coding` IR, when its foreach template is inspected, then it contains `step-execute` and a pass-through exit node, but no `step-review`.
|
||||
- AE3. Given optional-step defaults are resolved for `builtin:coding`, then `plan-review` and `code-review` are seeded by default and `browser-verification` is not.
|
||||
- AE3b. Given optional-step defaults are resolved for `builtin:stepwise-coding`, then `plan-review` and `code-review` are seeded by default and `browser-verification` is not.
|
||||
- AE4. Given docs list built-in workflows, then the new workflow is documented separately from `builtin:stepwise-coding`.
|
||||
- AE4. Given docs list built-in workflows, then `builtin:coding`, `builtin:legacy-coding`, and `builtin:stepwise-coding` are documented as distinct selectable workflows with their actual graph shapes.
|
||||
- AE5. Given the board/list workflow selector is set to Stepwise coding, when a new task is created from that workflow context, then `store.getTaskWorkflowSelection(task.id)?.workflowId` is `builtin:stepwise-coding`.
|
||||
- AE6. Given the board/list workflow selector is set to Compound engineering and the required plugin is available/enabled, when a new task is created from that workflow context, then `store.getTaskWorkflowSelection(task.id)?.workflowId` is `builtin:compound-engineering`.
|
||||
- AE7. Given Compound engineering is unavailable because `fusion-plugin-compound-engineering` is not installed/enabled, when a user tries to create/select a Compound engineering task, then the UI/API reports the plugin requirement instead of creating a default coding task.
|
||||
@@ -176,16 +176,16 @@ flowchart TB
|
||||
|
||||
### U1. Add the stepwise-final-review default Coding IR
|
||||
|
||||
- **Goal:** Create a Stepwise-derived workflow IR for `builtin:coding` that mirrors `builtin:stepwise-coding` except the foreach template omits `step-review` and routes `step-execute` success directly to a pass-through `step-done` exit.
|
||||
- **Goal:** Create a Stepwise-derived default Coding IR that is registered under `builtin:coding`, mirrors `builtin:stepwise-coding` through planning/Plan Review/parse/foreach, omits per-step `step-review`, and routes the final optional Code Review directly to merge.
|
||||
- **Requirements:** R1, R2, R3, R4, R5, R6
|
||||
- **Files:**
|
||||
- Create `packages/core/src/builtin-stepwise-final-review-coding-workflow-ir.ts`
|
||||
- Modify `packages/core/src/index.ts`
|
||||
- **Approach:** Copy the stable lifecycle skeleton from `packages/core/src/builtin-stepwise-coding-workflow-ir.ts`: same columns, `PROMPT.md` artifact declaration, planning/plan-review/parse/foreach, optional groups, merge region, and settings. Inside the foreach template, include only `step-execute` and `step-done` with a success edge. Remove per-step `step-review`, `outcome:revise`, `outcome:rethink`, rework-hold routing, and the mandatory final `review` seam.
|
||||
- **Approach:** Derive from the stable lifecycle skeleton in `packages/core/src/builtin-stepwise-coding-workflow-ir.ts`: same columns, `PROMPT.md` artifact declaration, planning/plan-review/parse/foreach, optional groups, merge region, and settings. Inside the foreach template, include only `step-execute` and `step-done` with a success edge. Remove per-step `step-review`, `outcome:revise`, `outcome:rethink`, rework-hold routing, and the mandatory final `review` seam. Register the derived IR as `builtin:coding`, not as a separate selectable workflow ID.
|
||||
- **FNXC comment requirement:** Add or update a concise FNXC comment in the new IR file explaining that this built-in exists because operators need graph-owned step execution with one whole-task review at the end rather than per-step review.
|
||||
- **Test Scenarios:**
|
||||
- The IR parses and round-trips.
|
||||
- The top-level graph contains `parse-steps`, `foreach`, `browser-verification`, `code-review`, `review`, and merge nodes.
|
||||
- The top-level graph contains `plan-review`, `parse-steps`, `foreach`, `browser-verification`, `code-review`, and merge nodes, with no mandatory final `review` node.
|
||||
- The foreach template contains `step-execute` and `step-done`.
|
||||
- The foreach template contains no `step-review`.
|
||||
- No template edge carries `kind: "rework"`.
|
||||
@@ -193,33 +193,34 @@ flowchart TB
|
||||
|
||||
### U2. Register and expose the new built-in workflow
|
||||
|
||||
- **Goal:** Make the workflow discoverable and selectable through the built-in workflow registry and package exports.
|
||||
- **Goal:** Make the new default Coding graph discoverable through the existing `builtin:coding` registry entry and package exports.
|
||||
- **Requirements:** R1, R6, R7
|
||||
- **Files:**
|
||||
- Modify `packages/core/src/builtin-workflows.ts`
|
||||
- Modify `packages/core/src/index.ts`
|
||||
- Possibly modify `packages/core/src/types.ts` if the enabled built-in IDs type or docs list is closed over explicit IDs.
|
||||
- **Approach:** Import the new IR, add a `BUILTIN_WORKFLOWS` entry near `builtin:stepwise-coding`, and provide a layout that includes `plan`, `parse`, `steps`, `browser-verification`, `code-review`, `review`, and merge/end nodes. Add the workflow ID to any tests or type-level inventories that enumerate selectable built-ins.
|
||||
- **Approach:** Import the derived IR, point the existing `builtin:coding` `BUILTIN_WORKFLOWS` entry at it, preserve the old monolithic IR as `builtin:legacy-coding`, and provide layouts that include the real node sets for default Coding and Coding (per-step review). Update tests or type-level inventories that enumerate selectable built-ins.
|
||||
- **Test Scenarios:**
|
||||
- `getBuiltinWorkflow("builtin:stepwise-final-review-coding")` returns the new workflow.
|
||||
- `defaultEnabledBuiltinWorkflowIds()` includes the new ID because it is selectable and not plugin-gated.
|
||||
- `NON_COMPILABLE_BUILTIN_IDS` includes the new ID in compiler tests because the graph uses interpreter-only node kinds.
|
||||
- `getBuiltinWorkflow("builtin:coding")` returns the derived Stepwise default Coding IR.
|
||||
- `getBuiltinWorkflow("builtin:legacy-coding")` returns the old monolithic coding IR.
|
||||
- `NON_COMPILABLE_BUILTIN_IDS` includes `builtin:coding` and `builtin:stepwise-coding` because the graphs use interpreter-only node kinds.
|
||||
- Built-in registry tests distinguish existing `builtin:stepwise-coding` as per-step-review and default `builtin:coding` as final-code-review-only.
|
||||
- **Verification:** Extend `packages/core/src/__tests__/builtin-workflows.test.ts`.
|
||||
|
||||
### U3. Preserve optional-gate behavior for the new workflow
|
||||
|
||||
- **Goal:** Ensure the new workflow has the same end-of-task optional gates as existing coding built-ins.
|
||||
- **Goal:** Ensure the default Coding and Coding (per-step review) workflows expose the required optional gates.
|
||||
- **Requirements:** R4, R5, R7, AE3
|
||||
- **Files:**
|
||||
- Modify `packages/core/src/__tests__/workflow-optional-steps.test.ts`
|
||||
- Modify `packages/core/src/__tests__/builtin-code-review-group.test.ts`
|
||||
- Modify `packages/core/src/__tests__/builtin-coding-workflow-ir.test.ts` if it asserts "both coding built-ins"
|
||||
- **Approach:** Expand test matrices that currently cover `BUILTIN_CODING_WORKFLOW_IR` and `BUILTIN_STEPWISE_CODING_WORKFLOW_IR` to include the new IR where the assertion is about shared coding optional gates. Keep per-step-review-specific assertions targeted only at `BUILTIN_STEPWISE_CODING_WORKFLOW_IR`.
|
||||
- **Approach:** Expand test matrices so default Coding and Coding (per-step review) both include default-on `plan-review`, default-off `browser-verification`, and default-on `code-review`, while Legacy coding continues to expose its original optional browser/code review gates.
|
||||
- **Test Scenarios:**
|
||||
- `resolveWorkflowOptionalSteps(newIr)` returns browser verification default off and code review default on.
|
||||
- `resolveWorkflowOptionalSteps(newIr)` returns plan review default on, browser verification default off, and code review default on.
|
||||
- `resolveDefaultOnOptionalGroupIds(newIr)` returns `["plan-review", "code-review"]`.
|
||||
- The new workflow routes `browser-verification -> code-review -> review`.
|
||||
- The default Coding workflow routes `browser-verification -> code-review -> merge-gate`.
|
||||
- Coding (per-step review) routes `browser-verification -> code-review -> review`.
|
||||
- Code review failure routes to `end` as in the other coding built-ins.
|
||||
- **Verification:** Run focused core tests listed in the Verification Contract.
|
||||
|
||||
|
||||
@@ -169,9 +169,11 @@ Fusion ships built-in workflows as read-only references:
|
||||
- `builtin:legacy-coding` — the original monolithic coding lifecycle for tasks that should not use graph-owned step execution.
|
||||
- `builtin:quick-fix` — a short path for trivial or no-commit/decision work.
|
||||
- `builtin:review-heavy` — a standard execute/review/merge path with an additional gated security review.
|
||||
- `builtin:marketing` — a marketing content pipeline with brief, draft, editorial review, and publish stages.
|
||||
- `builtin:compound-engineering` — a plugin-gated Compound Engineering pipeline: `/ce-plan` writes the CE plan doc, optional `ce-doc-review` can pressure-test plans (markdown gets autofix/Open Questions write-back; HTML uses DOM-safe helper mutations, including canonical checklist repair, only when safety is proven and otherwise report-only with no write), `/ce-work` implements, `/ce-code-review` gates merge, and autoMerge-off projects route through the CE PR/feedback skills before Fusion's manual merge seam.
|
||||
- `builtin:stepwise-coding` — Coding (per-step review): a graph variant with optional Plan Review before execution, per-step parse/execute/review/rework, and optional final Code Review before the final review/merge region.
|
||||
- `builtin:design` — a UI-heavy work path with a gated design/UX review before standard review and merge.
|
||||
- `builtin:lead-generation` — a lead workflow for sourcing, qualifying, enriching, and contacting prospects.
|
||||
|
||||
Built-ins can be viewed, exported, and used as templates, but their graph, columns, field declarations, and setting declarations are not editable. Their per-project setting **values** are editable from the Settings panel's Values tab. Selectable built-ins all use a capacity-released queue column (`todo` or a workflow-specific backlog) that dispatches to the active WIP column through the standard hold/release sweep.
|
||||
|
||||
|
||||
@@ -254,7 +254,7 @@ A v2 column can optionally name a **permanent agent** from the agent registry, s
|
||||
|
||||
### Workflow IR v2 — step inversion (foreach, loop, step-review, parse-steps, code, notify)
|
||||
|
||||
The **step-inversion** track makes task *steps* themselves workflow-modelable. Today the engine owns step policy end-to-end (PROMPT.md parsing, per-step review verdicts, RETHINK/REVISE control flow, merge blocking). Step inversion extracts exactly one new substrate capability — *run one step inside a task's session, and reset one step to its baseline* — and exposes everything else as authored graph structure. It is additive to IR v2 and gated by `experimentalFeatures.workflowGraphExecutor`. The default coding workflow is untouched and byte-identical (it keeps its monolithic `execute` seam and is the parity oracle); inversion is opt-in via custom workflows and a new built-in **stepwise coding workflow**.
|
||||
The **step-inversion** track makes task *steps* themselves workflow-modelable. The default Coding workflow now uses that graph-owned execution model: planning produces `PROMPT.md`, default-on Plan Review can approve the plan before execution, `parse-steps` writes the canonical step list, and `foreach` runs each step sequentially before the optional final Code Review gate. `builtin:legacy-coding` preserves the original monolithic `execute` seam for tasks that should not use graph-owned step execution, while `builtin:stepwise-coding` keeps the heavier per-step review/rework loop for work that needs review after every planned step. Step inversion remains additive to IR v2 for custom workflows that want to model their own step policy.
|
||||
|
||||
#### `parse-steps` node — step list as graph structure
|
||||
|
||||
|
||||
@@ -60,7 +60,9 @@ const RAW_BUILTIN_STEPWISE_FINAL_REVIEW_CODING_WORKFLOW_IR: WorkflowIr = (() =>
|
||||
if (!ir.edges.some((edge) => edge.from === "plan-review" && edge.to === "end" && edge.condition === "failure")) {
|
||||
ir.edges.push({ from: "plan-review", to: "end", condition: "failure" });
|
||||
}
|
||||
ir.edges.push({ from: "code-review", to: "merge-gate", condition: "success" });
|
||||
if (!ir.edges.some((edge) => edge.from === "code-review" && edge.to === "merge-gate" && edge.condition === "success")) {
|
||||
ir.edges.push({ from: "code-review", to: "merge-gate", condition: "success" });
|
||||
}
|
||||
|
||||
return ir;
|
||||
})();
|
||||
|
||||
@@ -340,6 +340,11 @@ describeIfGit("SelfHealingManager recoverAlreadyMergedReviewTasks (real git)", (
|
||||
expect(task.mergeDetails?.mergeConfirmed).not.toBe(true);
|
||||
expect((store as any).moveTask).not.toHaveBeenCalledWith("FN-TEST-FOREIGN-MISBOUND", "done");
|
||||
expect((store.logEntry as any).mock.calls.some((call: unknown[]) => String(call[1]).includes("already-merged rejected FN-TEST-FOREIGN-MISBOUND") && String(call[1]).includes("owner=FN-OTHER"))).toBe(true);
|
||||
expect((store as any).recordRunAuditEvent).toHaveBeenCalledWith(expect.objectContaining({
|
||||
mutationType: "task:auto-recover-already-merged-rejected",
|
||||
target: "FN-TEST-FOREIGN-MISBOUND",
|
||||
metadata: expect.objectContaining({ reason: "foreign-task-tip", candidateOwner: "FN-OTHER" }),
|
||||
}));
|
||||
}, 20_000);
|
||||
|
||||
it("is idempotent across two maintenance passes", async () => {
|
||||
|
||||
@@ -176,6 +176,7 @@ export async function findAlreadyMergedTaskCommit(
|
||||
FN-7143/FN-7187 proved patch-id and tree-equal fallbacks need branch identity proof, not just content equivalence. Only the canonical task branch may imply ownership for fallback matches, and any explicit foreign Fusion trailer on the branch tip or candidate commit rejects the recovery.
|
||||
*/
|
||||
const hasCanonicalBranchIdentity = branchName === canonicalBranchName;
|
||||
let branchTipOwnershipVerified = false;
|
||||
try {
|
||||
branchTip = execSync(`git rev-parse --verify ${shellQuote(branchName)}`, {
|
||||
cwd: repoDir,
|
||||
@@ -185,6 +186,7 @@ export async function findAlreadyMergedTaskCommit(
|
||||
if (await commitHasForeignTaskOwnership(repoDir, branchTip, taskId, lineageId)) {
|
||||
return null;
|
||||
}
|
||||
branchTipOwnershipVerified = true;
|
||||
|
||||
execSync(`git merge-base --is-ancestor ${shellQuote(branchTip)} ${shellQuote(baseBranch)}`, {
|
||||
cwd: repoDir,
|
||||
@@ -229,7 +231,7 @@ export async function findAlreadyMergedTaskCommit(
|
||||
if (!hasCanonicalBranchIdentity) {
|
||||
return null;
|
||||
}
|
||||
if (!branchTip) {
|
||||
if (!branchTip || !branchTipOwnershipVerified) {
|
||||
branchTip = execSync(`git rev-parse --verify ${shellQuote(branchName)}`, {
|
||||
cwd: repoDir,
|
||||
encoding: "utf-8",
|
||||
@@ -238,6 +240,7 @@ export async function findAlreadyMergedTaskCommit(
|
||||
if (await commitHasForeignTaskOwnership(repoDir, branchTip, taskId, lineageId)) {
|
||||
return null;
|
||||
}
|
||||
branchTipOwnershipVerified = true;
|
||||
}
|
||||
|
||||
let branchBase = baseCommitSha?.trim();
|
||||
|
||||
@@ -1891,13 +1891,16 @@ export class SelfHealingManager {
|
||||
candidateOwner?: string;
|
||||
taskBranch?: string | null;
|
||||
baseBranch: string;
|
||||
reason: "foreign-task-tip" | "foreign-lineage-tip" | "foreign-landed-commit";
|
||||
reason: "foreign-task-tip" | "foreign-lineage-tip" | "foreign-landed-commit" | "ownership-unverifiable";
|
||||
phase: string;
|
||||
}): Promise<void> {
|
||||
const { task, candidateSha, candidateOwner, taskBranch, baseBranch, reason, phase } = input;
|
||||
/*
|
||||
FNXC:WorkflowRecovery 2026-06-28-21:32:
|
||||
FN-7143 observed an already-merged tip that appeared to belong to FN-7187. Self-healing must make that cross-task proof visible and leave the review task alone; ambiguous or foreign tips are not safe evidence for mergeConfirmed/done finalization.
|
||||
|
||||
FNXC:WorkflowRecovery 2026-06-29-00:15:
|
||||
Ownership lookup failures are also unsafe evidence. Record them through the same rejection audit path with `ownership-unverifiable` so transient git-show/rev-parse failures cannot proceed into reclaim or auto-finalize as if the tip were verified non-foreign.
|
||||
*/
|
||||
await this.store.logEntry(
|
||||
task.id,
|
||||
@@ -1929,16 +1932,26 @@ export class SelfHealingManager {
|
||||
taskId: string;
|
||||
lineageId?: string;
|
||||
branch: string;
|
||||
}): Promise<{ sha: string; owner?: string; reason: "foreign-task-tip" | "foreign-lineage-tip" } | null> {
|
||||
}): Promise<{ sha: string; owner?: string; reason: "foreign-task-tip" | "foreign-lineage-tip" | "ownership-unverifiable" } | null> {
|
||||
const { taskId, lineageId, branch } = input;
|
||||
const { stdout } = await execAsync(`git rev-parse ${shellQuote(branch)}`, {
|
||||
cwd: this.options.rootDir,
|
||||
timeout: 30_000,
|
||||
maxBuffer: 1024 * 1024,
|
||||
});
|
||||
let stdout = "";
|
||||
try {
|
||||
({ stdout } = await execAsync(`git rev-parse ${shellQuote(branch)}`, {
|
||||
cwd: this.options.rootDir,
|
||||
timeout: 30_000,
|
||||
maxBuffer: 1024 * 1024,
|
||||
}));
|
||||
} catch {
|
||||
return { sha: "unverified", reason: "ownership-unverifiable" };
|
||||
}
|
||||
const sha = stdout.trim();
|
||||
if (!sha) return null;
|
||||
const ownership = await this.readCommitTaskOwnership(sha, taskId, lineageId);
|
||||
let ownership: Awaited<ReturnType<SelfHealingManager["readCommitTaskOwnership"]>>;
|
||||
try {
|
||||
ownership = await this.readCommitTaskOwnership(sha, taskId, lineageId);
|
||||
} catch {
|
||||
return { sha, reason: "ownership-unverifiable" };
|
||||
}
|
||||
if (ownership.rejectionReason === "foreign-task") {
|
||||
return { sha, owner: ownership.ownerTaskId, reason: "foreign-task-tip" };
|
||||
}
|
||||
@@ -3080,7 +3093,21 @@ export class SelfHealingManager {
|
||||
}
|
||||
if (inspection.kind === "tip-already-merged") {
|
||||
const branchName = task.branch;
|
||||
const ownership = await this.readCommitTaskOwnership(inspection.tipSha, task.id, task.lineageId).catch(() => null);
|
||||
const ownership = await this.readCommitTaskOwnership(inspection.tipSha, task.id, task.lineageId).catch(async () => {
|
||||
await this.rejectForeignAlreadyMergedCandidate({
|
||||
task,
|
||||
candidateSha: inspection.tipSha,
|
||||
candidateOwner: undefined,
|
||||
taskBranch: branchName,
|
||||
baseBranch: inspection.integrationRef,
|
||||
reason: "ownership-unverifiable",
|
||||
phase: "tip-already-merged",
|
||||
});
|
||||
return null;
|
||||
});
|
||||
if (!ownership) {
|
||||
continue;
|
||||
}
|
||||
if (ownership?.rejectionReason === "foreign-task" || ownership?.rejectionReason === "foreign-lineage") {
|
||||
await this.rejectForeignAlreadyMergedCandidate({
|
||||
task,
|
||||
|
||||
Reference in New Issue
Block a user