fix: make the compound-engineering workflow actually load skills and run the full CE flow - #1696
Merged
Merged
Conversation
Add skillName to WorkflowStep and WorkflowStepInput, and round-trip it through nodeToStepInput / stepInputToNode so a skill-executor node's skill is available to the step session. Honors the compiler INVERSION CONTRACT (parity test). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…path (U8/U1/U2/U3/U9) The builtin compound-engineering workflow runs via runGraphCustomNode, which never loaded the named skill or threaded the plugin-injected runtime env, and fn_spawn_agent was registered only in the main session. This wires the real seam: - U8: thread injected FUSION_CE_* env into skill/model graph steps (shared buildInjectedRuntimeEnv helper); register createSpawnAgentTool for coding-mode skill steps (readonly still strips spawn). - U1: merge the step's skillName (namespaced + bare) into requestedSkillNames and pass FUSION_CE_SKILLS_DIR as additionalSkillPaths so the bundled SKILL.md is discovered and selected. - U2: prepend the Fusion workflow-step conventions preamble (await-input sentinel, FUSION_HEADLESS degrade, persona fan-out via systemPromptOverride). - U3: explicit unattended opt-in sets FUSION_HEADLESS=1 (default-safe board run). - U9: path-confined persona read documented in the preamble; accepted write-capability posture documented at the coding-mode tool registration. - KTD-6: verdict-JSON contract required only for gate / skill-less steps. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…/U7) plan and code-review need coding so ce-plan/ce-code-review can fan out to their persona subagents via fn_spawn_agent; document needs coding so ce-compound can write docs/solutions. Test asserts the tool modes and that skillName is carried onto the compiled steps. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… headless, verdict (U6) Two engine tests for the new compound-engineering workflow-step wiring: - conventions: assert the exported preamble carries the await-input sentinel, FUSION_HEADLESS degrade, and path-confined persona/systemPromptOverride fan-out. - executor: drive runGraphCustomNode + executeWorkflowStep and assert skillName is carried onto the synthesized step, requestedSkillNames merges bare+namespaced with additionalSkillPaths=[FUSION_CE_SKILLS_DIR], fn_spawn_agent present only in coding, FUSION_HEADLESS only when unattended, and the verdict-JSON contract is required only for gate/skill-less steps (relaxed for non-gate skill steps). Session layer is mocked (asserts engine-owned wiring, not a model run); a full model-driven e2e remains a documented residual. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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>
Contributor
|
Warning Review limit reached
More reviews will be available in 12 minutes and 51 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR fixes the ChangesCE Workflow Skill Loading Fix
Sequence Diagram(s)sequenceDiagram
participant Graph as maybeExecuteWorkflowGraph
participant Runner as runGraphCustomNode
participant Step as executeWorkflowStep
participant Session as createFnAgent (AgentSession)
Graph->>Graph: clear graphUnattendedRuns[task.id]
Graph->>Runner: dispatch skill node
Runner->>Runner: prepend FUSION_WORKFLOW_STEP_CONVENTIONS_PREAMBLE to prompt
Runner->>Runner: set WorkflowStep.skillName from node config
Runner->>Runner: buildInjectedRuntimeEnv() → taskEnv
Runner->>Runner: read graphUnattendedRuns → unattended flag
Runner->>Step: executeWorkflowStep(step, task, { unattended })
Step->>Step: unattended=true → FUSION_HEADLESS=1
Step->>Step: resolve FUSION_CE_SKILLS_DIR → additionalSkillPaths
Step->>Step: coding mode → register fn_spawn_agent
Step->>Step: gate step? → inject verdictBlock JSON contract
Step->>Session: createFnAgent({ tools, skillNames, additionalSkillPaths, env })
Session-->>Step: prompt() → message events
Step-->>Runner: result
Graph->>Graph: finally — terminateAllChildren, clear graphUnattendedRuns
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Ready to review this PR? Stage has broken it down into 11 individual chapters for you: 
Chapters generated by Stage for commit 7235b25 on Jun 21, 2026 7:07am UTC. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
docs/plans/2026-06-20-001-fix-compound-engineering-workflow-skill-loading-plan.md (2)
56-58: 💤 Low valueOptional style improvement: Consider rephrasing "it is exactly the" for conciseness.
In the KTD-1 section, the phrase "this is the one missing hand-off, and it is exactly the 'workflow execution loads skills' requirement" could be tightened to avoid the slightly redundant "exactly" (see LanguageTool style note ~57). Example: "…and it represents the 'workflow execution loads skills' requirement" or simply remove "exactly".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/plans/2026-06-20-001-fix-compound-engineering-workflow-skill-loading-plan.md` around lines 56 - 58, In the KTD-1 section of the plan document, the phrase "this is the one missing hand-off, and it is exactly the 'workflow execution loads skills' requirement" contains redundant phrasing with the word "exactly". Rephrase this to improve conciseness by either replacing "exactly" with a more precise verb like "represents" (changing "it is exactly the" to "it represents the"), or remove "exactly" entirely to streamline the sentence while maintaining its meaning.Source: Linters/SAST tools
196-196: 💤 Low valueOptional style suggestion: Consider "brief" instead of "short".
In the U4 description, "Add a short comment on each" could read "Add a brief comment on each" for stronger, more precise wording per LanguageTool suggestion (~196).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/plans/2026-06-20-001-fix-compound-engineering-workflow-skill-loading-plan.md` at line 196, In the U4 description section where it states "Add a short comment on each", change the word "short" to "brief" for more precise and stronger wording that better conveys the intent to keep the comments concise while being clear and complete.Source: Linters/SAST tools
packages/engine/src/__tests__/ce-workflow-step-executor.test.ts (1)
226-234: ⚡ Quick winTest should assert
FUSION_WORKFLOW_STEP=1for the no-stepOptions case.The test description (line 197) states "always sets FUSION_WORKFLOW_STEP", but the third scenario (no stepOptions) only asserts that
FUSION_HEADLESSis undefined. For consistency with the other two cases and to verify the "always sets" claim, add an assertion forFUSION_WORKFLOW_STEP=1.✅ Add missing assertion
expect(cap.last?.taskEnv?.FUSION_HEADLESS).toBeUndefined(); + expect(cap.last?.taskEnv?.FUSION_WORKFLOW_STEP).toBe("1"); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/engine/src/__tests__/ce-workflow-step-executor.test.ts` around lines 226 - 234, The test case for the no-stepOptions scenario (which calls executeWorkflowStep with undefined stepOptions) is missing an assertion to verify that FUSION_WORKFLOW_STEP is set to 1. Add an assertion after the existing expect(cap.last?.taskEnv?.FUSION_HEADLESS).toBeUndefined() line to check that cap.last?.taskEnv?.FUSION_WORKFLOW_STEP equals "1", which will align this test case with the other two scenarios and verify the documented behavior that FUSION_WORKFLOW_STEP is always set.packages/engine/src/__tests__/ce-workflow-step-conventions.test.ts (1)
105-152: ⚡ Quick winConsider mocking
loadSkillsto avoid file I/O and narrow the test seam.Per coding guidelines, test files should prefer narrow seams and in-memory fakes over real file I/O. This test validates resolution logic (
resolveSessionSkills+createSkillsOverrideFromSelection), not discovery. MockingloadSkillsto return fake skills would:
- Focus the test on the system under test (resolution)
- Eliminate file I/O overhead
- Align with the "focused unit coverage" scope stated in the file comment
♻️ Example refactor to use mocked loadSkills
+import { vi } from "vitest"; + +// Mock loadSkills at the top level +vi.mock("`@earendil-works/pi-coding-agent`", () => ({ + loadSkills: vi.fn(), +})); + +import { loadSkills } from "`@earendil-works/pi-coding-agent`"; +const mockedLoadSkills = vi.mocked(loadSkills); describe("U1: dual-form (namespaced + bare) CE skill resolution", () => { - let tmp: string; let projectRootDir: string; - let agentDir: string; - let installRoot: string; - function materialize(id: string): void { - const dir = join(installRoot, id); - mkdirSync(dir, { recursive: true }); - writeFileSync( - join(dir, "SKILL.md"), - `---\nname: ${id}\ndescription: ${id} pipeline stage\n---\n\n# ${id}\n`, - ); - } - function resolveFor(requestedSkillNames: string[]): string[] { - const discovered = loadSkills({ - cwd: projectRootDir, - agentDir, - skillPaths: [installRoot], - includeDefaults: false, - }); + // Mock returns fake discovered skills + const discovered = { skills: [{ name: "ce-work", filePath: "/fake/ce-work/SKILL.md" }], diagnostics: [] }; const selection = resolveSessionSkills({ projectRootDir, requestedSkillNames, sessionPurpose: "executor", }); // ... rest unchanged } beforeEach(() => { - tmp = mkdtempSync(join(tmpdir(), "ce-conv-")); - projectRootDir = join(tmp, "project"); - agentDir = join(tmp, "agent"); - installRoot = join(tmp, ".fusion-ce-skills"); - mkdirSync(projectRootDir, { recursive: true }); - mkdirSync(agentDir, { recursive: true }); - materialize("ce-work"); + projectRootDir = "/fake/project"; + mockedLoadSkills.mockReturnValue({ skills: [{ name: "ce-work", filePath: "/fake/ce-work/SKILL.md" }], diagnostics: [] }); }); afterEach(() => { - rmSync(tmp, { recursive: true, force: true }); + vi.clearAllMocks(); });Then update the "without install dir" test to mock an empty skills array instead of repointing discovery.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/engine/src/__tests__/ce-workflow-step-conventions.test.ts` around lines 105 - 152, The test is performing unnecessary file I/O through the `materialize` function and `loadSkills` to validate resolution logic that should be tested in isolation. Mock the `loadSkills` function to return predetermined fake skills instead of creating actual files and directories. Update the `resolveFor` function to use the mocked `loadSkills` that returns known skills directly, removing the need for the `materialize` helper and related file system setup in `beforeEach`. This will narrow the test focus to only the resolution logic in `resolveSessionSkills` and `createSkillsOverrideFromSelection` without the overhead of real file I/O.Source: Coding guidelines
packages/engine/src/executor.ts (1)
981-995: ⚡ Quick winAdd FNXC headings to the new requirement comments.
Several new blocks document durable workflow behavior but omit the repo’s dated FNXC heading format. Add a short
FNXC:<Area> 2026-06-20-hh:mm:heading to these requirement comments, matching the style already used at Line 4301 and Line 12726.As per coding guidelines, “Add FNXC_LOG comments (format:
FNXC:Area-of-product yyyy-MM-dd-hh:mm:)”; based on learnings, use the establishedFNXC:heading rather than a literalFNXC_LOGtoken.Also applies to: 4055-4060, 4183-4196, 6059-6065, 6272-6377, 12533-12756, 15425-15434
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/engine/src/executor.ts` around lines 981 - 995, The multi-line comment block documenting the U2/KTD-2 and U9/KTD-7 workflow conventions and persona fan-out instruction is missing the FNXC dated heading format used elsewhere in the codebase. Add a short FNXC heading at the beginning of this comment block (before the existing "U2 / KTD-2" text) using the format FNXC:<Area> yyyy-MM-dd-hh:mm: to match the style established at other locations like Line 4301 and Line 12726. Apply this same fix to all other requirement comment blocks mentioned in the review (at line ranges 4055-4060, 4183-4196, 6059-6065, 6272-6377, 12533-12756, and 15425-15434).Sources: Coding guidelines, Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/__tests__/workflow-steps-to-ir.test.ts`:
- Around line 94-105: The step helper function in the WS-6 test case is dropping
the skillName property, which means the round-trip assertion through
stepInputToNode and nodeToStepInput is not actually validating that skillName is
preserved. Modify the step helper or the round-trip assertion logic to ensure
that the skillName field is retained and checked during the conversion process
so that the test can properly verify the compiler inversion contract for the
skillName property.
In `@packages/engine/src/executor.ts`:
- Around line 12693-12700: The stepEnv object spreads environment variables from
taskEnv or process.env, which can inherit an existing FUSION_HEADLESS variable
even when unattended is false. When constructing stepEnv, you need to explicitly
remove the FUSION_HEADLESS key from the inherited environment unless the
unattended flag is explicitly true. Modify the stepEnv object construction to
conditionally delete FUSION_HEADLESS from the spread parent environment when
unattended is false, or use a conditional spread pattern that excludes this key
in non-unattended scenarios.
- Around line 12757-12765: The issue is that codingCustomTools is empty in
readonly mode, so filterCustomToolsForReadonly never sees the spawn agent tool
created by createSpawnAgentTool and cannot add it to the denied list. Fix this
by always creating codingCustomTools with the spawn agent tool (regardless of
toolMode value), then pass it to filterCustomToolsForReadonly to properly
identify and log denied tools. This ensures the denial logging for
fn_spawn_agent works correctly in readonly mode instead of staying empty.
---
Nitpick comments:
In
`@docs/plans/2026-06-20-001-fix-compound-engineering-workflow-skill-loading-plan.md`:
- Around line 56-58: In the KTD-1 section of the plan document, the phrase "this
is the one missing hand-off, and it is exactly the 'workflow execution loads
skills' requirement" contains redundant phrasing with the word "exactly".
Rephrase this to improve conciseness by either replacing "exactly" with a more
precise verb like "represents" (changing "it is exactly the" to "it represents
the"), or remove "exactly" entirely to streamline the sentence while maintaining
its meaning.
- Line 196: In the U4 description section where it states "Add a short comment
on each", change the word "short" to "brief" for more precise and stronger
wording that better conveys the intent to keep the comments concise while being
clear and complete.
In `@packages/engine/src/__tests__/ce-workflow-step-conventions.test.ts`:
- Around line 105-152: The test is performing unnecessary file I/O through the
`materialize` function and `loadSkills` to validate resolution logic that should
be tested in isolation. Mock the `loadSkills` function to return predetermined
fake skills instead of creating actual files and directories. Update the
`resolveFor` function to use the mocked `loadSkills` that returns known skills
directly, removing the need for the `materialize` helper and related file system
setup in `beforeEach`. This will narrow the test focus to only the resolution
logic in `resolveSessionSkills` and `createSkillsOverrideFromSelection` without
the overhead of real file I/O.
In `@packages/engine/src/__tests__/ce-workflow-step-executor.test.ts`:
- Around line 226-234: The test case for the no-stepOptions scenario (which
calls executeWorkflowStep with undefined stepOptions) is missing an assertion to
verify that FUSION_WORKFLOW_STEP is set to 1. Add an assertion after the
existing expect(cap.last?.taskEnv?.FUSION_HEADLESS).toBeUndefined() line to
check that cap.last?.taskEnv?.FUSION_WORKFLOW_STEP equals "1", which will align
this test case with the other two scenarios and verify the documented behavior
that FUSION_WORKFLOW_STEP is always set.
In `@packages/engine/src/executor.ts`:
- Around line 981-995: The multi-line comment block documenting the U2/KTD-2 and
U9/KTD-7 workflow conventions and persona fan-out instruction is missing the
FNXC dated heading format used elsewhere in the codebase. Add a short FNXC
heading at the beginning of this comment block (before the existing "U2 / KTD-2"
text) using the format FNXC:<Area> yyyy-MM-dd-hh:mm: to match the style
established at other locations like Line 4301 and Line 12726. Apply this same
fix to all other requirement comment blocks mentioned in the review (at line
ranges 4055-4060, 4183-4196, 6059-6065, 6272-6377, 12533-12756, and
15425-15434).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cca04a8e-36d2-47e6-a187-35575c7e8556
📒 Files selected for processing (12)
.changeset/fix-ce-workflow-skill-loading.mddocs/plans/2026-06-20-001-fix-compound-engineering-workflow-skill-loading-plan.mdpackages/core/src/__tests__/builtin-workflows.test.tspackages/core/src/__tests__/workflow-steps-to-ir.test.tspackages/core/src/builtin-workflows.tspackages/core/src/types.tspackages/core/src/workflow-compiler.tspackages/core/src/workflow-steps-to-ir.tspackages/engine/src/__tests__/ce-workflow-step-conventions.test.tspackages/engine/src/__tests__/ce-workflow-step-executor.test.tspackages/engine/src/agent-runtime.tspackages/engine/src/executor.ts
Contributor
Greptile SummaryThis PR fixes the
Confidence Score: 5/5Safe to merge — the change fixes a silent no-op in the compound-engineering workflow path and is well-guarded by new tests. The core logic changes are well-contained: the FUSION_HEADLESS stripping guard prevents inherited-env leakage, terminateAllChildren placement in the graph finally correctly closes the spawn-lifecycle gap, the skill-loading path mirrors a proven interactive-session fix, and the verdict-contract relaxation is gated precisely on the isSkillStep && !isGate condition. All three new test files cover the failure modes the PR was designed to fix. No functional regressions to existing workflow-step, reviewer, or column-agent paths are apparent. No files require special attention — the executor.ts changes are the most complex but are thoroughly exercised by the new test suite. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant GR as maybeExecuteWorkflowGraph
participant RGN as runGraphCustomNode
participant BIRE as buildInjectedRuntimeEnv
participant EWS as executeWorkflowStep
participant Session as createResolvedAgentSession
GR->>RGN: skill node (cfg.skillName)
RGN->>BIRE: taskId, worktreePath, branch
BIRE-->>RGN: nodeEnv (FUSION_CE_SKILLS_DIR, FUSION_CE_AGENTS_DIR, PATH)
RGN->>RGN: prepend FUSION_WORKFLOW_STEP_CONVENTIONS_PREAMBLE
RGN->>RGN: "stepSkillName = cfg.skillName (U1)"
RGN->>EWS: "step{skillName}, nodeEnv, {unattended}"
EWS->>EWS: "stepEnv = {...nodeEnv, FUSION_WORKFLOW_STEP:1}"
EWS->>EWS: "unattended? set FUSION_HEADLESS=1 : delete FUSION_HEADLESS"
EWS->>EWS: merge bare+namespaced into requestedSkillNames (U1)
EWS->>EWS: "codingCustomTools = [createSpawnAgentTool] if coding (U8b)"
EWS->>Session: skillSelection, additionalSkillPaths, customTools, taskEnv
Session-->>EWS: session with CE skill loaded
GR->>GR: finally: terminateAllChildren(task.id)
GR->>GR: finally: clear graphUnattendedRuns, graphRouting, etc.
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant GR as maybeExecuteWorkflowGraph
participant RGN as runGraphCustomNode
participant BIRE as buildInjectedRuntimeEnv
participant EWS as executeWorkflowStep
participant Session as createResolvedAgentSession
GR->>RGN: skill node (cfg.skillName)
RGN->>BIRE: taskId, worktreePath, branch
BIRE-->>RGN: nodeEnv (FUSION_CE_SKILLS_DIR, FUSION_CE_AGENTS_DIR, PATH)
RGN->>RGN: prepend FUSION_WORKFLOW_STEP_CONVENTIONS_PREAMBLE
RGN->>RGN: "stepSkillName = cfg.skillName (U1)"
RGN->>EWS: "step{skillName}, nodeEnv, {unattended}"
EWS->>EWS: "stepEnv = {...nodeEnv, FUSION_WORKFLOW_STEP:1}"
EWS->>EWS: "unattended? set FUSION_HEADLESS=1 : delete FUSION_HEADLESS"
EWS->>EWS: merge bare+namespaced into requestedSkillNames (U1)
EWS->>EWS: "codingCustomTools = [createSpawnAgentTool] if coding (U8b)"
EWS->>Session: skillSelection, additionalSkillPaths, customTools, taskEnv
Session-->>EWS: session with CE skill loaded
GR->>GR: finally: terminateAllChildren(task.id)
GR->>GR: finally: clear graphUnattendedRuns, graphRouting, etc.
Reviews (2): Last reviewed commit: "Address PR review feedback (#1696)" | Re-trigger Greptile |
- step() test helper now carries skillName, so the WS-6 round-trip fixture actually exercises the INVERSION CONTRACT for skillName (was silently dropped). - executeWorkflowStep now strips an inherited FUSION_HEADLESS on board runs (unattended=false), preserving the U3 default-safe invariant — a board step nested under a headless-env parent could otherwise skip user questions. Added a regression test for the inherited-env strip. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
The
builtin:compound-engineeringworkflow looked wired — each node named a CE skill — but on the graph-node execution path it never actually ran the CE way: the named skill was only injected as prompt text (never loaded), the plugin-injectedFUSION_CE_*runtime env never reached step sessions, andfn_spawn_agentwas never registered for workflow steps, so skill loading and persona fan-out silently no-op'd. This makes the workflow genuinely load skills and run the full CE flow.Implements
docs/plans/2026-06-20-001-fix-compound-engineering-workflow-skill-loading-plan.md(units U1–U9).Changes
FUSION_CE_SKILLS_DIR/FUSION_CE_AGENTS_DIR) intorunGraphCustomNodeskill steps via a sharedbuildInjectedRuntimeEnvhelper, and registercreateSpawnAgentToolfor coding-mode skill steps (readonly still strips it).skillNamethrough theWorkflowStepround-trip (type + compilernodeToStepInput↔stepInputToNodeunder the INVERSION CONTRACT); inexecuteWorkflowStep, merge the bare + namespaced name intorequestedSkillNamesand passFUSION_CE_SKILLS_DIRasadditionalSkillPathsso the bundledSKILL.mdis discovered and selected (mirrors the interactive-session fix indocs/solutions/.../plugin-bundled-skills-not-loading-in-interactive-sessions.md).FUSION_HEADLESSdegrade, persona fan-out viasystemPromptOverride), so the bundled skills stay byte-for-byte upstream.unattendedopt-in setsFUSION_HEADLESS=1(default-safe: absent ⇒ board run). Entry-point wiring is deferred (no LFG/pipeline origin marker reaches the executor yet — see Follow-ups).plan/code-review(fan-out) anddocument(writesdocs/solutions).Code review (autofix) — fixes applied in this PR
A 9-persona review ran; the correctness-critical findings were verified and fixed in
fix(review): apply autofix feedback:execute()before its outerterminateAllChildren, so U8's new coding-mode children orphaned sessions/worktrees and accumulated the per-parent spawn budget, starving later steps' fan-out. NowterminateAllChildrenruns inmaybeExecuteWorkflowGraph's finally.skillNameadded to the round-trip projections + a skill-step fixture so the INVERSION CONTRACT is actually asserted.FUSION_CE_SKILLS_DIRis unset, instead of failing silent.unattendedRunguard.Test plan
@fusion/core+@fusion/enginetypecheck clean.workflow-graph-*, executor-core, runtime-env, column-agent, step-session, workflow-step verdict/review/readonly, and the new CE workflow-step tests).ce-workflow-step-conventions.test.ts,ce-workflow-step-executor.test.ts; parity + builtin-workflows tests extended.Residual Review Findings
Non-blocking; tracked here for follow-up:
WorkflowStep.skillNameis not persisted in theworkflow_stepstable. Moot today — the built-in CE workflow synthesizes itsWorkflowStepin-memory from IR and never persists skill-executor rows — but if skill steps ever become user-persistable, add askill_namecolumn + migration and round-trip it instore.createWorkflowStep/listWorkflowSteps.executeWorkflowStep; moveFUSION_WORKFLOW_STEP_CONVENTIONS_PREAMBLEinto its own module (decouples the test from the 15k-line executor); memoizecollectExecutorRuntimeEnvonce per graph run (currently per-node); exportstripSkillNamespacefromskill-resolver.tsinstead of inlining the namespace strip.FUSION_CE_SKILLS_DIRno-op; await-input sentinel →awaiting-user-inputparking;buildInjectedRuntimeEnvregression with a mockpluginRunnerreturning real keys; non-gate skill step succeeds without a verdict; spawn-tool input-schema assertion.FNXC:Area yyyy-MM-dd-hh:mmconvention (applied on the fix comments).disable-model-invocationcaller to setunattendedbefore genuinely-unattended runs can degrade honestly. Re-evaluate the accepted coding-mode write-capability posture (Risk-1) before enabling the CE workflow for unattended runs.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features