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) <noreply@anthropic.com>
This commit is contained in:
@@ -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",
|
||||
|
||||
@@ -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-<persona> subagents via
|
||||
|
||||
Reference in New Issue
Block a user