From c0f330e70c795cd0322cb99c1e54afc468f6a030 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Sat, 20 Jun 2026 23:16:12 -0700 Subject: [PATCH] fix(review): apply autofix feedback Address confirmed code-review findings on the CE workflow-step change: - Graph-path spawn lifecycle (adversarial A-1/A-2): the graph path returns from execute() before its outer finally that calls terminateAllChildren, so U8's new coding-mode fn_spawn_agent children orphaned their sessions/worktrees and their ids accumulated in the per-parent spawn budget, starving later steps' fan-out. Call terminateAllChildren in maybeExecuteWorkflowGraph's finally (mirrors the non-graph cleanup). - INVERSION CONTRACT parity (api-contract AC-2 + testing TF-001): add skillName to the workflow-steps-to-ir round-trip projections + a skill-step fixture, so the contract the comment claims is actually asserted. - Silent skill-load degradation (adversarial A-3 / Risk-4): warn when a step names a skill but FUSION_CE_SKILLS_DIR is unset, instead of failing silent. - Dead branch (maintainability M-01): drop the always-false unattendedRun guard; keep the delete + extension-point comment. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../__tests__/workflow-steps-to-ir.test.ts | 14 +++++++ packages/engine/src/executor.ts | 37 ++++++++++++++++--- 2 files changed, 45 insertions(+), 6 deletions(-) diff --git a/packages/core/src/__tests__/workflow-steps-to-ir.test.ts b/packages/core/src/__tests__/workflow-steps-to-ir.test.ts index de041be2d3..9b68f47fa5 100644 --- a/packages/core/src/__tests__/workflow-steps-to-ir.test.ts +++ b/packages/core/src/__tests__/workflow-steps-to-ir.test.ts @@ -38,6 +38,7 @@ function visible(input: WorkflowStepInput) { prompt: input.mode === "script" ? undefined : (input.prompt ?? ""), scriptName: input.scriptName, toolMode: input.mode === "script" ? undefined : input.toolMode, + skillName: input.mode === "script" ? undefined : input.skillName, modelProvider: input.modelProvider, modelId: input.modelId, }; @@ -52,6 +53,7 @@ function visibleStep(s: WorkflowStep) { prompt: s.mode === "script" ? undefined : (s.prompt ?? ""), scriptName: s.mode === "script" ? s.scriptName : undefined, toolMode: s.mode === "script" ? undefined : (s.toolMode ?? "readonly"), + skillName: s.mode === "script" ? undefined : s.skillName, modelProvider: s.mode === "prompt" ? s.modelProvider : undefined, modelId: s.mode === "prompt" ? s.modelId : undefined, }; @@ -89,6 +91,18 @@ describe("stepsToWorkflowIr — round-trip parity (R4/KTD-2)", () => { modelId: "claude-sonnet-4-5", phase: "pre-merge", }), + // U1 / INVERSION CONTRACT: a skill-executor step (pre-merge, grouped with + // the other pre-merge steps so declaration order matches compiled order) + // must round-trip its skillName through stepInputToNode → nodeToStepInput. + step({ + id: "WS-6", + name: "CE skill step", + mode: "prompt", + gateMode: "advisory", + prompt: "Invoke the skill", + skillName: "compound-engineering:ce-work", + phase: "pre-merge", + }), step({ id: "WS-4", name: "Document", diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 612866fecd..ea50d34ac2 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -4189,12 +4189,11 @@ export class TaskExecutor { // no FUSION_HEADLESS, so a board task can only ever park (a human can answer // via the await-input card button), never silently skip approval. When such // an entrypoint is added, it sets `unattended` here. - const unattendedRun = false; - if (unattendedRun) { - this.graphUnattendedRuns.add(task.id); - } else { - this.graphUnattendedRuns.delete(task.id); - } + // No entrypoint sets this today, so clear any stale entry; a board run never + // sets FUSION_HEADLESS. When an LFG/pipeline/disable-model-invocation + // entrypoint is added, call `this.graphUnattendedRuns.add(task.id)` here and + // the finally below clears it. + this.graphUnattendedRuns.delete(task.id); const runner = new WorkflowGraphTaskRunner({ store: { @@ -4299,6 +4298,20 @@ export class TaskExecutor { } return true; } finally { + // FNXC:WorkflowGraph 2026-06-20-23:35: + // Terminate child agents spawned by this graph run's coding-mode skill steps. + // U8 registered fn_spawn_agent for coding-mode steps, but the graph path + // returns from execute() at the graphOwned early-return — BEFORE execute()'s + // outer finally that calls terminateAllChildren. Without this, graph-step + // children orphan their sessions/worktrees, and their ids accumulate in the + // per-parent spawn budget (spawnedAgents[taskId]), starving later steps' + // fan-out (e.g. ce-code-review's reviewer panel). Mirror the non-graph + // cleanup; run it before the per-run graph bookkeeping below. + try { + await this.terminateAllChildren(task.id); + } catch (err) { + executorLog.warn(`terminateAllChildren failed for graph task ${task.id}: ${err instanceof Error ? err.message : String(err)}`); + } this.graphRouting.delete(task.id); // Clear per-run step-inversion pins (KTD-8: pinned only for the run's life). this.graphStepSessionPinned.delete(task.id); @@ -12710,6 +12723,18 @@ You have access to the file system to review changes.${verdictBlock}`; requestedSkillNames: mergedNames, }; } + // FNXC:WorkflowSteps 2026-06-20-23:35: + // A named skill with no discovery path silently degrades to the role-fallback + // skill (the exact pre-fix bug this change exists to kill). If the injected + // FUSION_CE_SKILLS_DIR never arrived (degraded/throwing plugin, missing install + // dir), warn loudly so an env-threading regression is visible on a board run + // instead of failing silent with a green hand-fed test. + if (workflowStep.skillName && workflowStep.skillName.trim() && !ceSkillsDir) { + await this.store.logEntry( + task.id, + `[skill-load] Workflow step '${workflowStep.name}' requests skill '${workflowStep.skillName}' but FUSION_CE_SKILLS_DIR is unset — the skill cannot be discovered; the step runs with role-fallback skills only.`, + ); + } const additionalSkillPaths = ceSkillsDir ? [ceSkillsDir] : undefined; // (U8b) Coding-mode skill steps fan out to ce- subagents via