fix(review): address PR #1788 review feedback

- workflow-graph-executor: wrap each post-merge walk() in try/catch so a malformed
  post-merge IR / traversal error is logged and skipped, never flipping an already-
  merged task to failed (non-blocking post-merge contract) [T9, real bug].
- Refresh stale FNXC comments now that graphNativePostMerge is default-ON and the
  legacy merger post-merge path was removed (experimental-features, workflow-graph-
  executor, workflow-graph-post-merge.test) [T6/T7/T8].
- Normalize FNXC timestamps to yyyy-MM-dd-hh:mm (TaskCard.test, taskProgress.test) [T2/T3].
- Changeset: category fix → feature to match the minor bump [T0].

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
2026-06-26 08:40:59 -07:00
parent c0f9f2b4a9
commit 005322eeaa
6 changed files with 53 additions and 32 deletions

View File

@@ -3,5 +3,5 @@
---
summary: Enabled optional workflow steps now run and show in task progress reliably.
category: fix
category: feature
dev: Fixes FN-7039. `Store.optionalGroupIdSet` falls back to `builtin:coding` (matching the executor's unselected-task resolution) so a toggled built-in group id (e.g. `browser-verification`) is no longer materialized into a legacy `WS-xxx` step row the graph never matches. Create-time optional-step controls (QuickEntryBox, TaskForm) resolve `builtin:coding` when no project default workflow is set, so the toggles appear. First unit of the broader graph-native workflow-step refactor.

View File

@@ -28,18 +28,19 @@ const RETIRED_EXPERIMENTAL_FEATURES = new Set<string>([
]);
/*
FNXC:WorkflowPostMerge 2026-06-26-09:00:
Post-merge workflow steps run GRAPH-NATIVE behind this default-OFF experimental flag
(U7 spike). When OFF (the default — the key is absent from DEFAULT_*_SETTINGS so
`isExperimentalFeatureEnabled` returns false), the merge-region stays collapsed exactly
as before: the graph routes merge-attempt success straight to `end` and the merger still
owns post-merge steps from the legacy table — zero behavior change, byte-identical
builtin:coding traversal. When ON, the graph executor lets traversal continue past a
SUCCESSFUL merge to any post-merge optional-group node reachable from the merge region,
running it via the same optional-group execution+recording path (phase:"post-merge",
non-blocking failures). This unit is additive + reversible: a later unit removes the
legacy merger post-merge path. Mirrors the WORKFLOW_INTERPRETER_DUAL_OBSERVE_FLAG read
plumbing (named constant + `isExperimentalFeatureEnabled`).
FNXC:WorkflowPostMerge 2026-06-26-15:30:
Post-merge workflow steps run GRAPH-NATIVE behind this DEFAULT-ON experimental flag
(see DEFAULT_ON_EXPERIMENTAL_FEATURES above). With the flag ON (the default — present in
DEFAULT_ON_EXPERIMENTAL_FEATURES so `isExperimentalFeatureEnabled` returns true unless an
explicit `false` opts out), the graph executor lets traversal continue past a SUCCESSFUL
merge to any post-merge optional-group node reachable from the merge region, running it via
the same optional-group execution+recording path (phase:"post-merge", non-blocking
failures). The graph is the SOLE owner of post-merge execution: U7c deleted the legacy
merger post-merge path entirely, so there is no legacy-table fallback — post-merge work runs
exactly once via the graph. An explicit `graphNativePostMerge: false` opts out and leaves the
merge-region collapsed (merge-attempt success routes straight to `end`). Mirrors the
WORKFLOW_INTERPRETER_DUAL_OBSERVE_FLAG read plumbing (named constant +
`isExperimentalFeatureEnabled`).
*/
export const GRAPH_NATIVE_POST_MERGE_FLAG = "graphNativePostMerge" as const;

View File

@@ -1909,7 +1909,7 @@ describe("TaskCard", () => {
expect(screen.queryByText("1 steps")).toBeNull();
});
// FNXC:WorkflowSteps 2026-06-25 — graph-written results drive the card progress; names come from
// FNXC:WorkflowSteps 2026-06-25-00:00 — graph-written results drive the card progress; names come from
// result.workflowStepName (with raw-id fallback), and advisory_failure (amber) is visually distinct
// from failed (red). No board-level name lookup is involved.
it("renders workflow checks after normal steps with graph-written statuses and phase badges", () => {

View File

@@ -3,7 +3,7 @@ import type { Task } from "@fusion/core";
import { getUnifiedTaskProgress } from "../taskProgress";
/*
FNXC:WorkflowSteps 2026-06-25 — graph-native progress model (plan U3).
FNXC:WorkflowSteps 2026-06-25-00:00 — graph-native progress model (plan U3).
These tests pin the render-state contract that the progress bar / Workflow tab rely on:
- names resolve from result.workflowStepName (no DB-row name lookup), with a raw-id fallback;
- a "pending" result with a startedAt and no completedAt is the `running` state, vs bare `pending`;

View File

@@ -5,13 +5,16 @@ import { postMergeOptionalGroupNode } from "@fusion/core";
import { WorkflowGraphExecutor, type WorkflowNodeHandler } from "../workflow-graph-executor.js";
/*
FNXC:WorkflowPostMerge 2026-06-26-09:00:
Graph-native post-merge steps (U7 spike). A post-merge optional-group node wired off
`merge-attempt` success must, WITH the `graphNativePostMerge` flag ON, run AFTER the
merge seam and record a WorkflowStepResult with phase:"post-merge". WITH the flag OFF
FNXC:WorkflowPostMerge 2026-06-26-15:30:
Graph-native post-merge steps. `graphNativePostMerge` is DEFAULT-ON; a post-merge
optional-group node wired off `merge-attempt` success runs AFTER the merge seam and
records a WorkflowStepResult with phase:"post-merge". An explicit opt-out
(`graphNativePostMerge: false`) DISABLES graph-native post-merge execution for that run:
the merge region stays collapsed and the post-merge node is never reached — it records
nothing via the graph (the legacy merger still owns post-merge). Post-merge failures are
non-blocking: the run still completes with the merge-success outcome.
nothing. There is NO legacy merger fallback anymore (the merger-side post-merge path was
removed in U7c), so an opt-out simply means post-merge work does not run via the graph.
Post-merge failures are non-blocking: the run still completes with the merge-success
outcome.
*/
const POST_MERGE_ID = "post-merge-docs";

View File

@@ -304,13 +304,15 @@ export class WorkflowGraphExecutor {
[WORKFLOW_ID_CONTEXT_KEY]: ir.name || "unknown",
};
/*
* FNXC:WorkflowPostMerge 2026-06-26-09:00:
* Graph-native post-merge steps, gated by the default-OFF `graphNativePostMerge`
* experimental flag. The merge-policy region is collapsed into ONE legacy merge
* FNXC:WorkflowPostMerge 2026-06-26-15:30:
* Graph-native post-merge steps, gated by the DEFAULT-ON `graphNativePostMerge`
* experimental flag (in DEFAULT_ON_EXPERIMENTAL_FEATURES; an explicit `false`
* opts out). The merge-policy region is collapsed into ONE legacy merge
* seam (see `runLegacyMergeSeam` + the `isMergeRegionKind` branch in
* `traverseChildren`), so a node wired off `merge-attempt` success is normally
* never traversed. With the flag ON we let traversal continue past a SUCCESSFUL
* merge to those post-merge entry nodes.
* never traversed. With the flag ON (the default) we let traversal continue past a
* SUCCESSFUL merge to those post-merge entry nodes; an explicit opt-out (`false`)
* leaves the set empty and skips the post-merge hop.
*
* `postMergeEntryNodeIds` = the (deterministic, id-sorted) set of edge targets `t`
* such that an edge leaves a merge-region node to `t`, where `t` is itself NOT a
@@ -769,12 +771,27 @@ export class WorkflowGraphExecutor {
* the merge region stays exactly as collapsed before.
*/
for (const entryId of postMergeEntryNodeIds) {
const postMerge = await walk(entryId);
// A post-merge entry node is never an enclosing rework head, so a
// ReworkSignal here would be malformed IR; ignore it rather than bubble a
// rework loop out of the merge boundary. Result is intentionally discarded
// (non-blocking).
void postMerge;
/*
* FNXC:WorkflowPostMerge 2026-06-26-15:30:
* Post-merge traversal must NEVER fail an already-merged run (non-blocking
* contract). A malformed post-merge IR or any traversal throw is caught,
* logged via the task log sink, and we CONTINUE to the next entry — the
* merged task still completes with the merge-success `aggregate`. Without
* this guard a throw would propagate out of the executor and flip a merged
* task into an executor failure.
*/
try {
const postMerge = await walk(entryId);
// A post-merge entry node is never an enclosing rework head, so a
// ReworkSignal here would be malformed IR; ignore it rather than bubble a
// rework loop out of the merge boundary. Result is intentionally discarded
// (non-blocking).
void postMerge;
} catch (err) {
this.deps.logTaskEntry?.(
`[post-merge] traversal error: ${err instanceof Error ? err.message : String(err)}`,
);
}
}
continue;
}