feat: migrate Claude off claude -p to ACP runtime + bridge (Route B + U10; Route A in progress)#1680
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ChangesComplete MCP-over-ACP bridge implementation
Sequence Diagram(s)sequenceDiagram
participant Client as pi-claude-cli
participant Router as streamSimple
participant Bridge as Claude ACP Bridge
participant ACP as ACP Session
participant Handler as clientHandler
Client->>Router: streamSimple(context, model, prompt)
Router->>Router: resolve ACP bridge path
alt Bridge available
Router->>Bridge: spawn with stdio + MCP servers
Router->>ACP: create client connection
ACP->>ACP: initialize session
Router->>ACP: send prompt (system + user blocks)
Bridge->>ACP: sessionUpdate (text chunk)
ACP->>Handler: sessionUpdate callback
Handler->>Handler: emit pi text_delta
Bridge->>ACP: sessionUpdate (tool_call)
ACP->>Handler: sessionUpdate callback
Handler->>Handler: emit pi toolcall_start, break
Bridge->>ACP: requestPermission (tool execution)
ACP->>Handler: requestPermission callback
Handler->>Handler: deny by default
Router->>ACP: stop session
ACP->>Bridge: close stdio
Bridge-->>Router: exit
Router-->>Client: AssistantMessageEventStream
else Bridge unavailable
Router->>Router: fall back to streamViaCli
Router-->>Client: CLI-based event stream
end
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 docstrings
🧪 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 |
Route A unblock + the first Route A code increment. - U9 verdict recorded (plan OQ1 + docs/acp-contract.md): in an authenticated interactive session the pinned claude-code-cli-acp 0.1.1 bridge forwards session/new mcpServers to Claude, Claude invokes the forwarded Fusion tool, and the call traverses the ACP permission gate (session/request_permission). Both security-critical answers resolve positively — overturns the headless NOT-GO chain (FN-6466/6467/6473/6476), whose only blocker was running detached from the login keychain session. - R17 (daemon auth) recorded and closed for the supported setup: creds are macOS Keychain-only; the user's login-session fn daemon has keychain access (the existing claude -p provider authenticates there), so the bridge does too. - U10: thread an optional mcpServers list through the ACP runtime contract. newAcpSession now forwards it (was hardcoded []); AgentRuntimeOptions (engine + plugin-local copy) gains the field; defaults to [] to preserve Route B's read-only ask posture. Tool calls still route through the U5 permission floor. Plugin typechecks clean; provider-session tests 12/12 (incl. 2 new forwarding tests). U11-U13 (provider transport swap, picker/auth, workflow verify) remain. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Reverse-engineered the streamViaCli contract and locked the U11 build plan: AssistantMessageEventStream shape to match, the kill-switch branch point in streamSimple, the KTD10 injection seam (pi-claude-cli adds @agentclientprotocol/sdk; bridge path injected like mcpConfigPath), MCP-server construction reusing ensureMcpConfig, full-history prompting (R13), ACP->pi event translation paralleling event-bridge.ts, and live-bridge verification via the U9 harness. No transport change yet — the live `claude -p` path is untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
gsxdsm
force-pushed
the
feature/acp-route-a
branch
from
348b713 to
03d12b6
Compare
Contributor
Greptile SummaryThis PR migrates Fusion's Claude integration from
Confidence Score: 5/5Safe to merge: the entire new transport is dormant behind an OFF-by-default double-key kill-switch, the live -p path is byte-for-byte unchanged, and the permission gate denies bridge-side tool execution by default. Route A is unreachable without explicit operator action (two env vars must be set), so the production risk surface is zero until a deliberate rollout. The break-early logic, env allow-list, inactivity timeout, and process-registry wiring are all sound. The two style findings do not affect runtime behaviour. packages/pi-claude-cli/src/mcp-config.ts has a duplicate AcpMcpServerSpec declaration that will drift from the canonical one in acp-driver.ts. All touched source files are missing the required FNXC comment prefixes. Important Files Changed
Sequence DiagramsequenceDiagram
participant pi as pi (ExtensionAPI)
participant idx as index.ts (streamSimple)
participant driver as acp-driver.ts (streamViaAcp)
participant bridge as claude-code-cli-acp bridge
participant claude as Claude process
pi->>idx: streamSimple(model, context, opts)
idx->>idx: resolveAcpBridgePath()
alt kill-switch OFF (default)
idx->>pi: streamViaCli (unchanged -p path)
else kill-switch ON + bridge path present
idx->>idx: resolveToolDefs() → buildAcpMcpServers()
idx->>driver: streamViaAcp(model, ctx, opts)
driver->>bridge: spawn(bridgePath, [], env allow-listed)
driver->>bridge: "initialize({protocolVersion})"
bridge-->>driver: "{protocolVersion}"
driver->>bridge: "newSession({cwd, mcpServers})"
bridge->>claude: spawn claude with MCP schema server
bridge-->>driver: "{sessionId}"
driver->>bridge: "prompt({sessionId, blocks})"
loop sessionUpdate stream
bridge-->>driver: agent_message_chunk → text delta
bridge-->>driver: tool_call (ToolSearch/internal) → pass through
bridge-->>driver: "tool_call (pi-known fn_*) → surfaceToolAndBreak"
bridge-->>driver: requestPermission → always cancelled
end
driver-->>pi: AssistantMessageEventStream (done: stop or toolUse)
end
Reviews (5): Last reviewed commit: "chore(acp): apply subagent-review follow..." | Re-trigger Greptile |
…l-switch OFF) Adds streamViaAcp: a drop-in alternative to streamViaCli that drives Claude through the claude-code-cli-acp bridge over ACP instead of `claude -p`. Returns the same AssistantMessageEventStream, so streamSimple dispatches to either transport behind a kill-switch (FUSION_CLAUDE_ACP=1 + an injected bridge path), OFF by default — the live `-p` path is byte-for-byte untouched until soak. - Full-history prompt every turn (buildPrompt) — the ACP path has no --resume (R13). - Forwards schema-only MCP servers so Claude emits correct tool calls; breaks early on the first tool_call (cancel turn, surface to pi) so the bridge never executes Fusion's tools — mirrors the `-p` break-early pattern. - Translation reuses the tested createEventBridge by synthesizing Claude stream events from ACP session/updates, sharing pi sequencing + tool-name mapping. - Bridge env forwards only HOME/PATH so `claude` authenticates from the login session (R17); never inherited process.env or API keys. Verified: 3/3 translation unit tests; real-bridge session/update shapes confirmed (agent_message_chunk text + tool_call); 326/326 existing pi-claude-cli tests green; typecheck clean. Remaining for Route A: engine injection of the bridge path (KTD10), U12 picker/ auth/status, U13 workflow verification. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/engine/src/cli-agent-validator.ts (1)
52-114:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPrecedence comment mismatch with code order.
The docstring at lines 55-59 states precedence as:
- explicit
verdict/statusstring token- boolean
passedandblocked === true- prose inference
- undecidable → error
But the code checks
blocked === true(line 74) before the verdict/status tokens (line 84). The comment should reflect the actual order: blocked flag → verdict/status token → passed boolean → prose → error.Suggested docstring fix
/** * Map a parsed one-shot result payload into a validator verdict. * * Precedence: - * 1. explicit `verdict`/`status` string token (normalized) - * 2. boolean `passed` (true→pass, false→fail) and `blocked === true` + * 1. explicit `blocked === true` flag + * 2. explicit `verdict`/`status` string token (normalized) + * 3. boolean `passed` (true→pass, false→fail) - * 3. prose inference from the result text (fail/blocked only; never pass) - * 4. nothing decodable → error (NEVER pass) + * 4. prose inference from the result text (fail/blocked only; never pass) + * 5. nothing decodable → error (NEVER pass) */🤖 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/cli-agent-validator.ts` around lines 52 - 114, Update the precedence comment in the docstring of the mapParsedToVerdict function to match the actual code execution order. The code checks in this order: explicit blocked flag, explicit verdict/status tokens, boolean passed flag, prose inference, and finally error. Revise the numbered list (lines 55-59) to reflect: 1. explicit blocked flag, 2. explicit verdict/status/result tokens, 3. boolean passed flag, 4. prose inference, 5. undecidable error.
🧹 Nitpick comments (4)
packages/engine/src/cli-agent-validator.ts (1)
186-206: 💤 Low valueOutput tail cap inconsistent with
ONE_SHOT_STDERR_CAP_BYTES.Line 202 caps the output tail at 4000 characters, while
ONE_SHOT_STDERR_CAP_BYTESinone-shot-session.tsis 8KB. Consider using a shared constant for consistency across failure diagnostics.🤖 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/cli-agent-validator.ts` around lines 186 - 206, The hardcoded 4000 character limit for the output tail in the runCliAgentValidation function is inconsistent with the ONE_SHOT_STDERR_CAP_BYTES constant defined in one-shot-session.ts (which is 8KB). Replace the hardcoded 4000 value in the result.text.slice(-4000) call on line 202 with the ONE_SHOT_STDERR_CAP_BYTES constant by importing it from one-shot-session.ts, ensuring consistent failure diagnostic handling across the codebase.packages/engine/src/cli-agent-ask.ts (2)
74-93: ⚖️ Poor tradeoffTimeout returns early but does not abort the underlying ACP prompt.
When the timeout fires,
askAcpOncereturns immediately withreason: "timeout", butpromptPromisecontinues executing in the background until the ACP session completes or fails. The session is disposed infinally, but if disposal doesn't forcibly terminate the underlying process, the ACP turn may continue consuming resources.This matches the test expectation (context snippet 4 shows
session.disposeis called on timeout), but consider whether the runtime should expose a cancellation mechanism for long-running prompts.🤖 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/cli-agent-ask.ts` around lines 74 - 93, When a timeout occurs in the askAcpOnce function, the code returns early with reason "timeout" but the underlying promptPromise from runtime.promptWithFallback continues executing in the background. To fix this, implement a cancellation mechanism using AbortController/AbortSignal (or equivalent mechanism supported by the runtime) that gets passed to the promptWithFallback call. When the timeout fires and timedOut is set to true, trigger the cancellation on the AbortController so the underlying prompt operation is forcibly terminated rather than just being abandoned. Ensure the disposal in the finally block is still called as a cleanup measure, but the actual cancellation should happen in the timeout handler itself.
38-49: Awaitsession.dispose()for consistency and completeness.
AgentSession.dispose()can returnvoid | Promise<void>(as seen ininteractive-ai-session.ts). The current code does not await it, leaving async cleanup incomplete. The function already awaitsruntime.dispose(session)(line 45), sosession.dispose()should be awaited symmetrically to ensure all cleanup is settled before returning.Suggested fix
- session.dispose(); + await session.dispose();🤖 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/cli-agent-ask.ts` around lines 38 - 49, In the disposeSession function, the final call to session.dispose() is not awaited, which can leave async cleanup incomplete since AgentSession.dispose() can return Promise<void>. Add await before session.dispose() at the end of the function to ensure all cleanup is properly settled before the function returns, making it consistent with how runtime.dispose(session) is handled earlier in the function.packages/engine/src/__tests__/merger-ai.test.ts (1)
332-338: ⚡ Quick winAdd invariant assertions for post-demotion/finalization task state.
Please also assert lifecycle consistency (
statuscleared andresult.task.columnaligned with final column) in both new cases. That catches stale-task/status regressions on this path.As per coding guidelines, bug-class regressions should assert the general invariant across known surfaces, not only one symptom.
Also applies to: 364-368
🤖 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__/merger-ai.test.ts` around lines 332 - 338, The test assertions are incomplete for validating post-demotion/finalization task state consistency. In the test cases around lines 332-338 and 364-368, add invariant assertions to verify lifecycle consistency: assert that the task's status field is cleared and that result.task.column aligns with the expected final column value. These assertions should be added in both affected test cases to catch stale-task/status regressions and ensure the general invariant is verified across all known surfaces affected by the demotion/finalization path.Source: Coding guidelines
🤖 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 `@docs/plans/2026-06-14-001-feat-claude-acp-runtime-plan.md`:
- Around line 155-162: The fenced code block that lists the file structure
(starting with packages/engine/src/ and containing cli-agent-ask.ts, setup.ts,
and test files) is missing a language identifier on the opening fence, which
triggers markdownlint rule MD040. Add the language identifier "text" to the
opening fence so it reads ```text instead of just ```, then the linting error
will be resolved.
In `@docs/settings-reference.md`:
- Around line 622-626: The documentation across multiple files incorrectly
describes the OAuth credential refresh behavior by mentioning a pre-expiry
refresh buffer, but the actual implementation only attempts refresh after the
credential is already expired. At docs/settings-reference.md lines 622-626,
remove or qualify the phrase "within the refresh buffer" so the description
accurately reflects that refresh only happens when the access token is expired,
not before expiration. At the sibling location in docs/dashboard-guide.md lines
383-387, apply the identical wording change to ensure both sections provide
consistent and accurate guidance about the actual refresh trigger being
expiration only.
In `@packages/dashboard/app/hooks/useBackgroundSessions.ts`:
- Around line 400-402: The needsInput calculation in the active session filter
now correctly includes both awaiting_input and waiting_on_input statuses, but
the sync broadcasts that communicate needsInput state to other tabs are not
including waiting_on_input, causing the counters to become out of sync across
tabs. Find where the sync payload for needsInput is being constructed (likely in
a broadcast or sync function related to background sessions) and update that
code to use the same filtering logic that includes both awaiting_input and
waiting_on_input statuses, ensuring consistency between the local state
calculation and the broadcasted state.
In `@packages/engine/src/__tests__/cli-agent-ask.test.ts`:
- Around line 92-95: The test "times out a never-resolving prompt and disposes"
relies on a real 5ms timeout which causes CI flakiness. Replace the real timeout
mechanism with fake timers: enable Jest fake timers before executing the test
logic, then after calling askAcpOnce, manually advance the timers to trigger the
timeout behavior instead of waiting for real time to elapse. This ensures the
test is deterministic, fast, and reliably exercises the timeout code path
without depending on system timing.
In `@packages/engine/src/merger-ai.ts`:
- Around line 1136-1163: The early-return path that calls store.moveTask to
demote the task back to "todo" status returns the stale pre-merge task snapshot
which still has the "merging" status that was set earlier at line 1044. After
the store.moveTask call in the return statement preparation, fetch the updated
task from the store (likely via store.getTask(taskId) or similar) and return
that updated task object instead of the stale task variable. This ensures the
returned task object reflects the current state without the "merging" status,
which is particularly important for immutable TaskStore implementations.
In `@packages/engine/src/merger.ts`:
- Around line 7382-7405: The telemetry operations (store.logEntry and
audit.database calls) must be made best-effort to prevent them from blocking the
critical moveTask demotion. At line range 7382-7405 in
packages/engine/src/merger.ts (anchor), wrap the store.logEntry and
audit.database calls in a try/catch block so that telemetry failures do not
prevent execution of the moveTask call that demotes the task back to "todo" with
preserved progress. Apply the same best-effort wrapping pattern to the
recordRunAuditEvent call sites at lines 8439-8464 (sibling) and lines 8747-8771
(sibling) before their respective return statements, ensuring that telemetry
failures cannot bypass the guard by preventing task demotion.
In `@packages/engine/src/self-healing.ts`:
- Around line 5146-5167: The error is being set via updateTask before the
moveTask call, but the scheduler clears the error field when moving a task from
in-progress to todo. Move the await this.store.updateTask(task.id, { error:
reason }) call to execute after the await this.store.moveTask call completes, so
the error is reapplied and preserved after the task movement.
In `@plugins/fusion-plugin-acp-runtime/src/cli-spawn.ts`:
- Around line 116-121: The bundled binary resolution check only sets binaryPath
when kind is "resolved" and path is present, but when resolution is
not_resolved, binaryPath stays as the bare sentinel command which allows
PATH-based fallback to a non-plugin binary. Fix this by changing the condition
to set binaryPath from binaryResolution.path whenever the path is present,
regardless of the kind value. Update the if statement after the
resolveBundledClaudeBridgeBinary() call to directly check if
binaryResolution.path exists rather than first checking if kind equals
"resolved", ensuring unresolved bundled installs fail deterministically instead
of falling back to PATH.
In `@plugins/fusion-plugin-acp-runtime/src/setup.ts`:
- Around line 22-25: The isInside function does not handle the case where
relative() returns an absolute path on Windows when comparing paths on different
drives. The current validation logic checks for empty strings or relative paths
starting with "..", but does not reject absolute paths, allowing external
binaries to bypass the bundled-bridge identity enforcement. Import isAbsolute
from the path module and update the return statement in the isInside function to
also check that the relative path is not absolute; the function should return
true only when rel is empty, or when rel is a relative path (not starting with
".." and not containing sep followed by ".."). This ensures cross-drive paths on
Windows are properly rejected.
---
Outside diff comments:
In `@packages/engine/src/cli-agent-validator.ts`:
- Around line 52-114: Update the precedence comment in the docstring of the
mapParsedToVerdict function to match the actual code execution order. The code
checks in this order: explicit blocked flag, explicit verdict/status tokens,
boolean passed flag, prose inference, and finally error. Revise the numbered
list (lines 55-59) to reflect: 1. explicit blocked flag, 2. explicit
verdict/status/result tokens, 3. boolean passed flag, 4. prose inference, 5.
undecidable error.
---
Nitpick comments:
In `@packages/engine/src/__tests__/merger-ai.test.ts`:
- Around line 332-338: The test assertions are incomplete for validating
post-demotion/finalization task state consistency. In the test cases around
lines 332-338 and 364-368, add invariant assertions to verify lifecycle
consistency: assert that the task's status field is cleared and that
result.task.column aligns with the expected final column value. These assertions
should be added in both affected test cases to catch stale-task/status
regressions and ensure the general invariant is verified across all known
surfaces affected by the demotion/finalization path.
In `@packages/engine/src/cli-agent-ask.ts`:
- Around line 74-93: When a timeout occurs in the askAcpOnce function, the code
returns early with reason "timeout" but the underlying promptPromise from
runtime.promptWithFallback continues executing in the background. To fix this,
implement a cancellation mechanism using AbortController/AbortSignal (or
equivalent mechanism supported by the runtime) that gets passed to the
promptWithFallback call. When the timeout fires and timedOut is set to true,
trigger the cancellation on the AbortController so the underlying prompt
operation is forcibly terminated rather than just being abandoned. Ensure the
disposal in the finally block is still called as a cleanup measure, but the
actual cancellation should happen in the timeout handler itself.
- Around line 38-49: In the disposeSession function, the final call to
session.dispose() is not awaited, which can leave async cleanup incomplete since
AgentSession.dispose() can return Promise<void>. Add await before
session.dispose() at the end of the function to ensure all cleanup is properly
settled before the function returns, making it consistent with how
runtime.dispose(session) is handled earlier in the function.
In `@packages/engine/src/cli-agent-validator.ts`:
- Around line 186-206: The hardcoded 4000 character limit for the output tail in
the runCliAgentValidation function is inconsistent with the
ONE_SHOT_STDERR_CAP_BYTES constant defined in one-shot-session.ts (which is
8KB). Replace the hardcoded 4000 value in the result.text.slice(-4000) call on
line 202 with the ONE_SHOT_STDERR_CAP_BYTES constant by importing it from
one-shot-session.ts, ensuring consistent failure diagnostic handling across the
codebase.
🪄 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: 8ab8e399-2d2c-4922-9efa-7f5cdb46c9d1
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (68)
.changeset/fn-6458-cli-banner-actions.md.changeset/fn-6461-no-commits-finalize-guard.mdCONCEPTS.mddocs/acp-contract.mddocs/architecture.mddocs/dashboard-guide.mddocs/getting-started.mddocs/plans/2026-06-14-001-feat-claude-acp-runtime-plan.mddocs/settings-reference.mdpackages/core/src/__tests__/no-commits-finalize-guard.test.tspackages/core/src/index.tspackages/core/src/no-commits-finalize-guard.tspackages/dashboard/app/App.tsxpackages/dashboard/app/__tests__/app-cli-action-wiring.test.tsxpackages/dashboard/app/components/SessionNotificationBanner.csspackages/dashboard/app/components/SessionNotificationBanner.tsxpackages/dashboard/app/components/__tests__/ChatView.regular-composer-no-right-line.test.tsxpackages/dashboard/app/components/__tests__/MissionManager.test.tsxpackages/dashboard/app/components/__tests__/ModalReentry.test.tsxpackages/dashboard/app/components/__tests__/ModelSelectorTab.test.tsxpackages/dashboard/app/components/__tests__/NewAgentDialog.test.tsxpackages/dashboard/app/components/__tests__/OAuthReloginBanner.test.tsxpackages/dashboard/app/components/__tests__/PlanningModeModal.favorites.test.tsxpackages/dashboard/app/components/__tests__/PlanningModeModal.questions.test.tsxpackages/dashboard/app/components/__tests__/PlanningModeModal.swipe-back.test.tsxpackages/dashboard/app/components/__tests__/SessionNotificationBanner.test.tsxpackages/dashboard/app/components/__tests__/SkillsView.css.test.tspackages/dashboard/app/components/__tests__/mobile-css.test.tsxpackages/dashboard/app/hooks/__tests__/useBackgroundSessions.test.tspackages/dashboard/app/hooks/useBackgroundSessions.tspackages/dashboard/src/__tests__/mission-e2e.test.tspackages/dashboard/src/__tests__/planning.test.tspackages/dashboard/vitest.config.tspackages/engine/src/__tests__/cli-agent-ask.test.tspackages/engine/src/__tests__/cli-agent-validator.test.tspackages/engine/src/__tests__/interactive-ai-session.test.tspackages/engine/src/__tests__/merger-ai.test.tspackages/engine/src/__tests__/merger-finalize-unproven.real-git.test.tspackages/engine/src/__tests__/reliability-interactions/ai-merge-worktree-cleanup.test.tspackages/engine/src/__tests__/self-healing.test.tspackages/engine/src/agent-runtime.tspackages/engine/src/cli-agent-ask.tspackages/engine/src/cli-agent-validator.tspackages/engine/src/cli-agent/__tests__/one-shot-session.test.tspackages/engine/src/cli-agent/one-shot-session.tspackages/engine/src/index.tspackages/engine/src/interactive-ai-session.tspackages/engine/src/merger-ai.tspackages/engine/src/merger.tspackages/engine/src/run-audit.tspackages/engine/src/self-healing.tsplugins/fusion-plugin-acp-runtime/AGENTS.mdplugins/fusion-plugin-acp-runtime/CHANGELOG.mdplugins/fusion-plugin-acp-runtime/README.mdplugins/fusion-plugin-acp-runtime/package.jsonplugins/fusion-plugin-acp-runtime/src/__tests__/index.test.tsplugins/fusion-plugin-acp-runtime/src/__tests__/process-manager.test.tsplugins/fusion-plugin-acp-runtime/src/__tests__/provider-session.test.tsplugins/fusion-plugin-acp-runtime/src/__tests__/runtime-adapter.test.tsplugins/fusion-plugin-acp-runtime/src/__tests__/setup.test.tsplugins/fusion-plugin-acp-runtime/src/cli-spawn.tsplugins/fusion-plugin-acp-runtime/src/index.tsplugins/fusion-plugin-acp-runtime/src/process-manager.tsplugins/fusion-plugin-acp-runtime/src/provider.tsplugins/fusion-plugin-acp-runtime/src/runtime-adapter.tsplugins/fusion-plugin-acp-runtime/src/setup.tsplugins/fusion-plugin-acp-runtime/src/types.tsscripts/lib/test-quarantine.json
💤 Files with no reviewable changes (10)
- packages/dashboard/app/components/tests/ModalReentry.test.tsx
- packages/dashboard/app/components/tests/mobile-css.test.tsx
- packages/dashboard/app/components/tests/SkillsView.css.test.ts
- packages/dashboard/app/components/tests/PlanningModeModal.favorites.test.tsx
- packages/dashboard/app/components/tests/NewAgentDialog.test.tsx
- packages/dashboard/app/components/tests/ChatView.regular-composer-no-right-line.test.tsx
- packages/dashboard/app/components/tests/PlanningModeModal.questions.test.tsx
- packages/dashboard/app/components/tests/ModelSelectorTab.test.tsx
- packages/dashboard/app/components/tests/PlanningModeModal.swipe-back.test.tsx
- packages/dashboard/app/components/tests/OAuthReloginBanner.test.tsx
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/engine/src/cli-agent-validator.ts (1)
52-114:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPrecedence comment mismatch with code order.
The docstring at lines 55-59 states precedence as:
- explicit
verdict/statusstring token- boolean
passedandblocked === true- prose inference
- undecidable → error
But the code checks
blocked === true(line 74) before the verdict/status tokens (line 84). The comment should reflect the actual order: blocked flag → verdict/status token → passed boolean → prose → error.Suggested docstring fix
/** * Map a parsed one-shot result payload into a validator verdict. * * Precedence: - * 1. explicit `verdict`/`status` string token (normalized) - * 2. boolean `passed` (true→pass, false→fail) and `blocked === true` + * 1. explicit `blocked === true` flag + * 2. explicit `verdict`/`status` string token (normalized) + * 3. boolean `passed` (true→pass, false→fail) - * 3. prose inference from the result text (fail/blocked only; never pass) - * 4. nothing decodable → error (NEVER pass) + * 4. prose inference from the result text (fail/blocked only; never pass) + * 5. nothing decodable → error (NEVER pass) */🤖 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/cli-agent-validator.ts` around lines 52 - 114, Update the precedence comment in the docstring of the mapParsedToVerdict function to match the actual code execution order. The code checks in this order: explicit blocked flag, explicit verdict/status tokens, boolean passed flag, prose inference, and finally error. Revise the numbered list (lines 55-59) to reflect: 1. explicit blocked flag, 2. explicit verdict/status/result tokens, 3. boolean passed flag, 4. prose inference, 5. undecidable error.
🧹 Nitpick comments (4)
packages/engine/src/cli-agent-validator.ts (1)
186-206: 💤 Low valueOutput tail cap inconsistent with
ONE_SHOT_STDERR_CAP_BYTES.Line 202 caps the output tail at 4000 characters, while
ONE_SHOT_STDERR_CAP_BYTESinone-shot-session.tsis 8KB. Consider using a shared constant for consistency across failure diagnostics.🤖 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/cli-agent-validator.ts` around lines 186 - 206, The hardcoded 4000 character limit for the output tail in the runCliAgentValidation function is inconsistent with the ONE_SHOT_STDERR_CAP_BYTES constant defined in one-shot-session.ts (which is 8KB). Replace the hardcoded 4000 value in the result.text.slice(-4000) call on line 202 with the ONE_SHOT_STDERR_CAP_BYTES constant by importing it from one-shot-session.ts, ensuring consistent failure diagnostic handling across the codebase.packages/engine/src/cli-agent-ask.ts (2)
74-93: ⚖️ Poor tradeoffTimeout returns early but does not abort the underlying ACP prompt.
When the timeout fires,
askAcpOncereturns immediately withreason: "timeout", butpromptPromisecontinues executing in the background until the ACP session completes or fails. The session is disposed infinally, but if disposal doesn't forcibly terminate the underlying process, the ACP turn may continue consuming resources.This matches the test expectation (context snippet 4 shows
session.disposeis called on timeout), but consider whether the runtime should expose a cancellation mechanism for long-running prompts.🤖 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/cli-agent-ask.ts` around lines 74 - 93, When a timeout occurs in the askAcpOnce function, the code returns early with reason "timeout" but the underlying promptPromise from runtime.promptWithFallback continues executing in the background. To fix this, implement a cancellation mechanism using AbortController/AbortSignal (or equivalent mechanism supported by the runtime) that gets passed to the promptWithFallback call. When the timeout fires and timedOut is set to true, trigger the cancellation on the AbortController so the underlying prompt operation is forcibly terminated rather than just being abandoned. Ensure the disposal in the finally block is still called as a cleanup measure, but the actual cancellation should happen in the timeout handler itself.
38-49: Awaitsession.dispose()for consistency and completeness.
AgentSession.dispose()can returnvoid | Promise<void>(as seen ininteractive-ai-session.ts). The current code does not await it, leaving async cleanup incomplete. The function already awaitsruntime.dispose(session)(line 45), sosession.dispose()should be awaited symmetrically to ensure all cleanup is settled before returning.Suggested fix
- session.dispose(); + await session.dispose();🤖 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/cli-agent-ask.ts` around lines 38 - 49, In the disposeSession function, the final call to session.dispose() is not awaited, which can leave async cleanup incomplete since AgentSession.dispose() can return Promise<void>. Add await before session.dispose() at the end of the function to ensure all cleanup is properly settled before the function returns, making it consistent with how runtime.dispose(session) is handled earlier in the function.packages/engine/src/__tests__/merger-ai.test.ts (1)
332-338: ⚡ Quick winAdd invariant assertions for post-demotion/finalization task state.
Please also assert lifecycle consistency (
statuscleared andresult.task.columnaligned with final column) in both new cases. That catches stale-task/status regressions on this path.As per coding guidelines, bug-class regressions should assert the general invariant across known surfaces, not only one symptom.
Also applies to: 364-368
🤖 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__/merger-ai.test.ts` around lines 332 - 338, The test assertions are incomplete for validating post-demotion/finalization task state consistency. In the test cases around lines 332-338 and 364-368, add invariant assertions to verify lifecycle consistency: assert that the task's status field is cleared and that result.task.column aligns with the expected final column value. These assertions should be added in both affected test cases to catch stale-task/status regressions and ensure the general invariant is verified across all known surfaces affected by the demotion/finalization path.Source: Coding guidelines
🤖 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 `@docs/plans/2026-06-14-001-feat-claude-acp-runtime-plan.md`:
- Around line 155-162: The fenced code block that lists the file structure
(starting with packages/engine/src/ and containing cli-agent-ask.ts, setup.ts,
and test files) is missing a language identifier on the opening fence, which
triggers markdownlint rule MD040. Add the language identifier "text" to the
opening fence so it reads ```text instead of just ```, then the linting error
will be resolved.
In `@docs/settings-reference.md`:
- Around line 622-626: The documentation across multiple files incorrectly
describes the OAuth credential refresh behavior by mentioning a pre-expiry
refresh buffer, but the actual implementation only attempts refresh after the
credential is already expired. At docs/settings-reference.md lines 622-626,
remove or qualify the phrase "within the refresh buffer" so the description
accurately reflects that refresh only happens when the access token is expired,
not before expiration. At the sibling location in docs/dashboard-guide.md lines
383-387, apply the identical wording change to ensure both sections provide
consistent and accurate guidance about the actual refresh trigger being
expiration only.
In `@packages/dashboard/app/hooks/useBackgroundSessions.ts`:
- Around line 400-402: The needsInput calculation in the active session filter
now correctly includes both awaiting_input and waiting_on_input statuses, but
the sync broadcasts that communicate needsInput state to other tabs are not
including waiting_on_input, causing the counters to become out of sync across
tabs. Find where the sync payload for needsInput is being constructed (likely in
a broadcast or sync function related to background sessions) and update that
code to use the same filtering logic that includes both awaiting_input and
waiting_on_input statuses, ensuring consistency between the local state
calculation and the broadcasted state.
In `@packages/engine/src/__tests__/cli-agent-ask.test.ts`:
- Around line 92-95: The test "times out a never-resolving prompt and disposes"
relies on a real 5ms timeout which causes CI flakiness. Replace the real timeout
mechanism with fake timers: enable Jest fake timers before executing the test
logic, then after calling askAcpOnce, manually advance the timers to trigger the
timeout behavior instead of waiting for real time to elapse. This ensures the
test is deterministic, fast, and reliably exercises the timeout code path
without depending on system timing.
In `@packages/engine/src/merger-ai.ts`:
- Around line 1136-1163: The early-return path that calls store.moveTask to
demote the task back to "todo" status returns the stale pre-merge task snapshot
which still has the "merging" status that was set earlier at line 1044. After
the store.moveTask call in the return statement preparation, fetch the updated
task from the store (likely via store.getTask(taskId) or similar) and return
that updated task object instead of the stale task variable. This ensures the
returned task object reflects the current state without the "merging" status,
which is particularly important for immutable TaskStore implementations.
In `@packages/engine/src/merger.ts`:
- Around line 7382-7405: The telemetry operations (store.logEntry and
audit.database calls) must be made best-effort to prevent them from blocking the
critical moveTask demotion. At line range 7382-7405 in
packages/engine/src/merger.ts (anchor), wrap the store.logEntry and
audit.database calls in a try/catch block so that telemetry failures do not
prevent execution of the moveTask call that demotes the task back to "todo" with
preserved progress. Apply the same best-effort wrapping pattern to the
recordRunAuditEvent call sites at lines 8439-8464 (sibling) and lines 8747-8771
(sibling) before their respective return statements, ensuring that telemetry
failures cannot bypass the guard by preventing task demotion.
In `@packages/engine/src/self-healing.ts`:
- Around line 5146-5167: The error is being set via updateTask before the
moveTask call, but the scheduler clears the error field when moving a task from
in-progress to todo. Move the await this.store.updateTask(task.id, { error:
reason }) call to execute after the await this.store.moveTask call completes, so
the error is reapplied and preserved after the task movement.
In `@plugins/fusion-plugin-acp-runtime/src/cli-spawn.ts`:
- Around line 116-121: The bundled binary resolution check only sets binaryPath
when kind is "resolved" and path is present, but when resolution is
not_resolved, binaryPath stays as the bare sentinel command which allows
PATH-based fallback to a non-plugin binary. Fix this by changing the condition
to set binaryPath from binaryResolution.path whenever the path is present,
regardless of the kind value. Update the if statement after the
resolveBundledClaudeBridgeBinary() call to directly check if
binaryResolution.path exists rather than first checking if kind equals
"resolved", ensuring unresolved bundled installs fail deterministically instead
of falling back to PATH.
In `@plugins/fusion-plugin-acp-runtime/src/setup.ts`:
- Around line 22-25: The isInside function does not handle the case where
relative() returns an absolute path on Windows when comparing paths on different
drives. The current validation logic checks for empty strings or relative paths
starting with "..", but does not reject absolute paths, allowing external
binaries to bypass the bundled-bridge identity enforcement. Import isAbsolute
from the path module and update the return statement in the isInside function to
also check that the relative path is not absolute; the function should return
true only when rel is empty, or when rel is a relative path (not starting with
".." and not containing sep followed by ".."). This ensures cross-drive paths on
Windows are properly rejected.
---
Outside diff comments:
In `@packages/engine/src/cli-agent-validator.ts`:
- Around line 52-114: Update the precedence comment in the docstring of the
mapParsedToVerdict function to match the actual code execution order. The code
checks in this order: explicit blocked flag, explicit verdict/status tokens,
boolean passed flag, prose inference, and finally error. Revise the numbered
list (lines 55-59) to reflect: 1. explicit blocked flag, 2. explicit
verdict/status/result tokens, 3. boolean passed flag, 4. prose inference, 5.
undecidable error.
---
Nitpick comments:
In `@packages/engine/src/__tests__/merger-ai.test.ts`:
- Around line 332-338: The test assertions are incomplete for validating
post-demotion/finalization task state consistency. In the test cases around
lines 332-338 and 364-368, add invariant assertions to verify lifecycle
consistency: assert that the task's status field is cleared and that
result.task.column aligns with the expected final column value. These assertions
should be added in both affected test cases to catch stale-task/status
regressions and ensure the general invariant is verified across all known
surfaces affected by the demotion/finalization path.
In `@packages/engine/src/cli-agent-ask.ts`:
- Around line 74-93: When a timeout occurs in the askAcpOnce function, the code
returns early with reason "timeout" but the underlying promptPromise from
runtime.promptWithFallback continues executing in the background. To fix this,
implement a cancellation mechanism using AbortController/AbortSignal (or
equivalent mechanism supported by the runtime) that gets passed to the
promptWithFallback call. When the timeout fires and timedOut is set to true,
trigger the cancellation on the AbortController so the underlying prompt
operation is forcibly terminated rather than just being abandoned. Ensure the
disposal in the finally block is still called as a cleanup measure, but the
actual cancellation should happen in the timeout handler itself.
- Around line 38-49: In the disposeSession function, the final call to
session.dispose() is not awaited, which can leave async cleanup incomplete since
AgentSession.dispose() can return Promise<void>. Add await before
session.dispose() at the end of the function to ensure all cleanup is properly
settled before the function returns, making it consistent with how
runtime.dispose(session) is handled earlier in the function.
In `@packages/engine/src/cli-agent-validator.ts`:
- Around line 186-206: The hardcoded 4000 character limit for the output tail in
the runCliAgentValidation function is inconsistent with the
ONE_SHOT_STDERR_CAP_BYTES constant defined in one-shot-session.ts (which is
8KB). Replace the hardcoded 4000 value in the result.text.slice(-4000) call on
line 202 with the ONE_SHOT_STDERR_CAP_BYTES constant by importing it from
one-shot-session.ts, ensuring consistent failure diagnostic handling across the
codebase.
🪄 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: 8ab8e399-2d2c-4922-9efa-7f5cdb46c9d1
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (68)
.changeset/fn-6458-cli-banner-actions.md.changeset/fn-6461-no-commits-finalize-guard.mdCONCEPTS.mddocs/acp-contract.mddocs/architecture.mddocs/dashboard-guide.mddocs/getting-started.mddocs/plans/2026-06-14-001-feat-claude-acp-runtime-plan.mddocs/settings-reference.mdpackages/core/src/__tests__/no-commits-finalize-guard.test.tspackages/core/src/index.tspackages/core/src/no-commits-finalize-guard.tspackages/dashboard/app/App.tsxpackages/dashboard/app/__tests__/app-cli-action-wiring.test.tsxpackages/dashboard/app/components/SessionNotificationBanner.csspackages/dashboard/app/components/SessionNotificationBanner.tsxpackages/dashboard/app/components/__tests__/ChatView.regular-composer-no-right-line.test.tsxpackages/dashboard/app/components/__tests__/MissionManager.test.tsxpackages/dashboard/app/components/__tests__/ModalReentry.test.tsxpackages/dashboard/app/components/__tests__/ModelSelectorTab.test.tsxpackages/dashboard/app/components/__tests__/NewAgentDialog.test.tsxpackages/dashboard/app/components/__tests__/OAuthReloginBanner.test.tsxpackages/dashboard/app/components/__tests__/PlanningModeModal.favorites.test.tsxpackages/dashboard/app/components/__tests__/PlanningModeModal.questions.test.tsxpackages/dashboard/app/components/__tests__/PlanningModeModal.swipe-back.test.tsxpackages/dashboard/app/components/__tests__/SessionNotificationBanner.test.tsxpackages/dashboard/app/components/__tests__/SkillsView.css.test.tspackages/dashboard/app/components/__tests__/mobile-css.test.tsxpackages/dashboard/app/hooks/__tests__/useBackgroundSessions.test.tspackages/dashboard/app/hooks/useBackgroundSessions.tspackages/dashboard/src/__tests__/mission-e2e.test.tspackages/dashboard/src/__tests__/planning.test.tspackages/dashboard/vitest.config.tspackages/engine/src/__tests__/cli-agent-ask.test.tspackages/engine/src/__tests__/cli-agent-validator.test.tspackages/engine/src/__tests__/interactive-ai-session.test.tspackages/engine/src/__tests__/merger-ai.test.tspackages/engine/src/__tests__/merger-finalize-unproven.real-git.test.tspackages/engine/src/__tests__/reliability-interactions/ai-merge-worktree-cleanup.test.tspackages/engine/src/__tests__/self-healing.test.tspackages/engine/src/agent-runtime.tspackages/engine/src/cli-agent-ask.tspackages/engine/src/cli-agent-validator.tspackages/engine/src/cli-agent/__tests__/one-shot-session.test.tspackages/engine/src/cli-agent/one-shot-session.tspackages/engine/src/index.tspackages/engine/src/interactive-ai-session.tspackages/engine/src/merger-ai.tspackages/engine/src/merger.tspackages/engine/src/run-audit.tspackages/engine/src/self-healing.tsplugins/fusion-plugin-acp-runtime/AGENTS.mdplugins/fusion-plugin-acp-runtime/CHANGELOG.mdplugins/fusion-plugin-acp-runtime/README.mdplugins/fusion-plugin-acp-runtime/package.jsonplugins/fusion-plugin-acp-runtime/src/__tests__/index.test.tsplugins/fusion-plugin-acp-runtime/src/__tests__/process-manager.test.tsplugins/fusion-plugin-acp-runtime/src/__tests__/provider-session.test.tsplugins/fusion-plugin-acp-runtime/src/__tests__/runtime-adapter.test.tsplugins/fusion-plugin-acp-runtime/src/__tests__/setup.test.tsplugins/fusion-plugin-acp-runtime/src/cli-spawn.tsplugins/fusion-plugin-acp-runtime/src/index.tsplugins/fusion-plugin-acp-runtime/src/process-manager.tsplugins/fusion-plugin-acp-runtime/src/provider.tsplugins/fusion-plugin-acp-runtime/src/runtime-adapter.tsplugins/fusion-plugin-acp-runtime/src/setup.tsplugins/fusion-plugin-acp-runtime/src/types.tsscripts/lib/test-quarantine.json
💤 Files with no reviewable changes (10)
- packages/dashboard/app/components/tests/ModalReentry.test.tsx
- packages/dashboard/app/components/tests/mobile-css.test.tsx
- packages/dashboard/app/components/tests/SkillsView.css.test.ts
- packages/dashboard/app/components/tests/PlanningModeModal.favorites.test.tsx
- packages/dashboard/app/components/tests/NewAgentDialog.test.tsx
- packages/dashboard/app/components/tests/ChatView.regular-composer-no-right-line.test.tsx
- packages/dashboard/app/components/tests/PlanningModeModal.questions.test.tsx
- packages/dashboard/app/components/tests/ModelSelectorTab.test.tsx
- packages/dashboard/app/components/tests/PlanningModeModal.swipe-back.test.tsx
- packages/dashboard/app/components/tests/OAuthReloginBanner.test.tsx
🛑 Comments failed to post (9)
docs/plans/2026-06-14-001-feat-claude-acp-runtime-plan.md (1)
155-162:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language identifier to this fenced code block.
Line 155 opens a fenced block without a language, which triggers markdownlint MD040 and adds avoidable lint noise.
🛠️ Proposed fix
-``` +```text packages/engine/src/ cli-agent-ask.ts # U4: askAcpOnce runner + typed result __tests__/cli-agent-ask.test.ts # U4 tests (fake AgentRuntime) plugins/fusion-plugin-acp-runtime/src/ setup.ts # U3: PluginSetupManifest + checkSetup (bridge + claude/auth probe) __tests__/setup.test.ts # U3 tests</details> <!-- suggestion_start --> <details> <summary>📝 Committable suggestion</summary> > ‼️ **IMPORTANT** > Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements. ```suggestion🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 155-155: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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-14-001-feat-claude-acp-runtime-plan.md` around lines 155 - 162, The fenced code block that lists the file structure (starting with packages/engine/src/ and containing cli-agent-ask.ts, setup.ts, and test files) is missing a language identifier on the opening fence, which triggers markdownlint rule MD040. Add the language identifier "text" to the opening fence so it reads ```text instead of just ```, then the linting error will be resolved.Source: Linters/SAST tools
docs/settings-reference.md (1)
622-626:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTrim the pre-expiry refresh-buffer wording.
The verified
/api/auth/statusflow only attempts refresh after the OAuth credential is already expired, so these docs currently promise earlier behavior than the implementation provides. If a buffer exists elsewhere, link it; otherwise keep both sections aligned to the expired-only refresh path.
docs/settings-reference.md#L622-L626: remove or qualify “within the refresh buffer” so it matches the actual refresh trigger.docs/dashboard-guide.md#L383-L387: mirror the same wording here to avoid conflicting guidance.📍 Affects 2 files
docs/settings-reference.md#L622-L626(this comment)docs/dashboard-guide.md#L383-L387🤖 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/settings-reference.md` around lines 622 - 626, The documentation across multiple files incorrectly describes the OAuth credential refresh behavior by mentioning a pre-expiry refresh buffer, but the actual implementation only attempts refresh after the credential is already expired. At docs/settings-reference.md lines 622-626, remove or qualify the phrase "within the refresh buffer" so the description accurately reflects that refresh only happens when the access token is expired, not before expiration. At the sibling location in docs/dashboard-guide.md lines 383-387, apply the identical wording change to ensure both sections provide consistent and accurate guidance about the actual refresh trigger being expiration only.packages/dashboard/app/hooks/useBackgroundSessions.ts (1)
400-402:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign sync payload
needsInputsemantics with the newwaiting_on_inputcount.
needsInputnow includeswaiting_on_inputin returned state, but sync broadcasts still markneedsInputtrue only forawaiting_input. That can desync cross-tab counters/consumers.Suggested fix
- needsInput: updated.status === "awaiting_input", + needsInput: + updated.status === "awaiting_input" || updated.status === "waiting_on_input",🤖 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/dashboard/app/hooks/useBackgroundSessions.ts` around lines 400 - 402, The needsInput calculation in the active session filter now correctly includes both awaiting_input and waiting_on_input statuses, but the sync broadcasts that communicate needsInput state to other tabs are not including waiting_on_input, causing the counters to become out of sync across tabs. Find where the sync payload for needsInput is being constructed (likely in a broadcast or sync function related to background sessions) and update that code to use the same filtering logic that includes both awaiting_input and waiting_on_input statuses, ensuring consistency between the local state calculation and the broadcasted state.packages/engine/src/__tests__/cli-agent-ask.test.ts (1)
92-95:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse fake timers for deterministic timeout-path testing.
Line 94 currently depends on a real 5ms timeout, which can introduce CI flakiness. Use fake timers and advance time explicitly so this stays deterministic and fast.
As per coding guidelines, tests should prefer fake timers over real polling/time waits.
Suggested update
it("times out a never-resolving prompt and disposes", async () => { - const { runtime, session } = makeRuntime({ neverResolve: true }); - const result = await askAcpOnce(runtime, { prompt: "p", cwd: "/repo", timeoutMs: 5 }); - expect(result).toMatchObject({ ok: false, reason: "timeout" }); - expect(session.dispose).toHaveBeenCalledOnce(); + vi.useFakeTimers(); + try { + const { runtime, session } = makeRuntime({ neverResolve: true }); + const resultPromise = askAcpOnce(runtime, { prompt: "p", cwd: "/repo", timeoutMs: 5 }); + await vi.advanceTimersByTimeAsync(5); + const result = await resultPromise; + expect(result).toMatchObject({ ok: false, reason: "timeout" }); + expect(session.dispose).toHaveBeenCalledOnce(); + } finally { + vi.useRealTimers(); + } });🤖 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__/cli-agent-ask.test.ts` around lines 92 - 95, The test "times out a never-resolving prompt and disposes" relies on a real 5ms timeout which causes CI flakiness. Replace the real timeout mechanism with fake timers: enable Jest fake timers before executing the test logic, then after calling askAcpOnce, manually advance the timers to trigger the timeout behavior instead of waiting for real time to elapse. This ensures the test is deterministic, fast, and reliably exercises the timeout code path without depending on system timing.Source: Coding guidelines
packages/engine/src/merger-ai.ts (1)
1136-1163:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear merge status and return the demoted task object on blocked finalize.
This early-return path demotes the task but returns the pre-merge
tasksnapshot and never explicitly clears the"merging"status set earlier (Line 1044). With immutableTaskStoreimplementations,result.taskcan be stale after demotion.Proposed fix
- await store.updateTask(taskId, { error: reason }); + await store.updateTask(taskId, { status: null, error: reason }); @@ - await store.moveTask(taskId, "todo", { preserveProgress: true, moveSource: "engine" } as Parameters<TaskStore["moveTask"]>[2]); + const demotedTask = await store.moveTask( + taskId, + "todo", + { preserveProgress: true, moveSource: "engine" } as Parameters<TaskStore["moveTask"]>[2], + ); return { - task, + task: demotedTask, branch, merged: false, noOp: false,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.await store.updateTask(taskId, { status: null, error: reason }); await store.logEntry( taskId, `Finalize blocked (no-commits incomplete-work guard): ${reason} — moving back to todo with progress preserved`, JSON.stringify({ doneCount: noCommitsFinalize.doneCount, incompleteCount: noCommitsFinalize.incompleteCount, branch, integrationBranch, lane: "ai-empty-merge", }, null, 2), ); await audit.database({ type: "task:no-commits-finalize-blocked-incomplete-steps" as Parameters<typeof audit.database>[0]["type"], target: taskId, metadata: { reason, doneCount: noCommitsFinalize.doneCount, incompleteCount: noCommitsFinalize.incompleteCount, branch, integrationBranch, lane: "ai-empty-merge", }, }); const demotedTask = await store.moveTask( taskId, "todo", { preserveProgress: true, moveSource: "engine" } as Parameters<TaskStore["moveTask"]>[2], ); return { task: demotedTask, branch,🤖 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/merger-ai.ts` around lines 1136 - 1163, The early-return path that calls store.moveTask to demote the task back to "todo" status returns the stale pre-merge task snapshot which still has the "merging" status that was set earlier at line 1044. After the store.moveTask call in the return statement preparation, fetch the updated task from the store (likely via store.getTask(taskId) or similar) and return that updated task object instead of the stale task variable. This ensures the returned task object reflects the current state without the "merging" status, which is particularly important for immutable TaskStore implementations.packages/engine/src/merger.ts (1)
7382-7405:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake guard telemetry best-effort before demotion.
These paths await log/audit writes before
moveTask(..., "todo"). IflogEntry,audit.database, orrecordRunAuditEventthrows, the demotion is skipped; in the early fast-path the caller catches the helper error and falls through to the standard merge path, bypassing the guard entirely. Move the task state transition before non-essential telemetry, or wrap telemetry intry/catchso the guard cannot be defeated. Based on the PR objective, blocked no-commits tasks must be demoted back totodowith progress preserved.Suggested pattern
await store.updateTask(taskId, { error: reason }); - await store.logEntry( - taskId, - `Finalize blocked (no-commits incomplete-work guard): ${reason} — moving back to todo with progress preserved`, - JSON.stringify(metadata, null, 2), - ); - await audit.database({ - type: "task:no-commits-finalize-blocked-incomplete-steps" as Parameters<typeof audit.database>[0]["type"], - target: taskId, - metadata, - }); await store.moveTask(taskId, "todo", { preserveProgress: true, moveSource: "engine" } as any); + try { + await store.logEntry( + taskId, + `Finalize blocked (no-commits incomplete-work guard): ${reason} — moving back to todo with progress preserved`, + JSON.stringify(metadata, null, 2), + ); + await audit.database({ + type: "task:no-commits-finalize-blocked-incomplete-steps" as Parameters<typeof audit.database>[0]["type"], + target: taskId, + metadata, + }); + } catch (telemetryErr: unknown) { + log.warn(`${taskId}: failed to emit no-commits finalize-blocked telemetry: ${telemetryErr instanceof Error ? telemetryErr.message : String(telemetryErr)}`); + }Apply the same best-effort wrapping around the two
recordRunAuditEventcall sites before returning.Also applies to: 8439-8464, 8747-8771
🤖 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/merger.ts` around lines 7382 - 7405, The telemetry operations (store.logEntry and audit.database calls) must be made best-effort to prevent them from blocking the critical moveTask demotion. At line range 7382-7405 in packages/engine/src/merger.ts (anchor), wrap the store.logEntry and audit.database calls in a try/catch block so that telemetry failures do not prevent execution of the moveTask call that demotes the task back to "todo" with preserved progress. Apply the same best-effort wrapping pattern to the recordRunAuditEvent call sites at lines 8439-8464 (sibling) and lines 8747-8771 (sibling) before their respective return statements, ensuring that telemetry failures cannot bypass the guard by preventing task demotion.packages/engine/src/self-healing.ts (1)
5146-5167:
⚠️ Potential issue | 🔴 Critical🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Inspect TaskStore.moveTask implementations/tests to confirm whether an existing task.error # is preserved, cleared, or normalized when moving an in-review task back to todo. rg -nP -C8 '\b(async\s+)?moveTask\s*\(' rg -nP -C6 'error:\s*null|error:\s*undefined|preserveProgress|preserveStatus' rg -nP -C6 'no-commits-finalize-blocked-incomplete-steps|Finalize blocked \(no-commits incomplete-work guard\)|recoverStrandedCompletedTodoTasks'Repository: Runfusion/Fusion
Length of output: 50374
Apply
erroraftermoveTaskcompletes.The scheduler explicitly documents that
erroris cleared duringmoveTask(in-progress → todo). The error set at line 5152 will be lost; reapply it after the move to preserve the operator-facing guard state:Fix: reapply error after moveTask
await this.recordIntegrityAudit(task.id, "task:no-commits-finalize-blocked-incomplete-steps", { reason, doneCount: noCommitsFinalize.doneCount, incompleteCount: noCommitsFinalize.incompleteCount, classification: "proven-no-op", baseRef: classification.baseRef, lane: "self-healing-finalize-no-op-review", }); // `#1411`: backward recovery — skip order-derived adjacency. await this.store.moveTask(task.id, "todo", { preserveProgress: true, moveSource: "engine", recoveryRehome: true }); + await this.store.updateTask(task.id, { error: reason }); recovered++; continue;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.await this.store.updateTask(task.id, { error: reason }); await this.store.logEntry( task.id, `Finalize blocked (no-commits incomplete-work guard): ${reason} — moving back to todo with progress preserved`, JSON.stringify({ doneCount: noCommitsFinalize.doneCount, incompleteCount: noCommitsFinalize.incompleteCount, classification: "proven-no-op", baseRef: classification.baseRef, lane: "self-healing-finalize-no-op-review", }, null, 2), ); await this.recordIntegrityAudit(task.id, "task:no-commits-finalize-blocked-incomplete-steps", { reason, doneCount: noCommitsFinalize.doneCount, incompleteCount: noCommitsFinalize.incompleteCount, classification: "proven-no-op", baseRef: classification.baseRef, lane: "self-healing-finalize-no-op-review", }); // `#1411`: backward recovery — skip order-derived adjacency. await this.store.moveTask(task.id, "todo", { preserveProgress: true, moveSource: "engine", recoveryRehome: true }); await this.store.updateTask(task.id, { error: reason });🤖 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/self-healing.ts` around lines 5146 - 5167, The error is being set via updateTask before the moveTask call, but the scheduler clears the error field when moving a task from in-progress to todo. Move the await this.store.updateTask(task.id, { error: reason }) call to execute after the await this.store.moveTask call completes, so the error is reapplied and preserved after the task movement.plugins/fusion-plugin-acp-runtime/src/cli-spawn.ts (1)
116-121:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail closed when bundled bridge resolution misses.
At Line 116-121, when bundled resolution is
not_resolved,binaryPathstays as the bare sentinel command (claude-code-cli-acp). That reintroduces PATH selection and can execute a non-plugin binary. SetbinaryPathfrombinaryResolution.pathwhenever present so unresolved bundled installs fail deterministically instead of falling back to PATH.Suggested fix
if (requestedBinaryPath === CLAUDE_CODE_CLI_ACP_BINARY) { binaryResolution = resolveBundledClaudeBridgeBinary(); - if (binaryResolution.kind === "resolved" && binaryResolution.path) { + if (binaryResolution.path) { binaryPath = binaryResolution.path; } }As per coding guidelines: use the bundled
claude-code-cli-acpfrom plugin-ownednode_modulesand do not substitute with a PATH-resolved binary.🤖 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 `@plugins/fusion-plugin-acp-runtime/src/cli-spawn.ts` around lines 116 - 121, The bundled binary resolution check only sets binaryPath when kind is "resolved" and path is present, but when resolution is not_resolved, binaryPath stays as the bare sentinel command which allows PATH-based fallback to a non-plugin binary. Fix this by changing the condition to set binaryPath from binaryResolution.path whenever the path is present, regardless of the kind value. Update the if statement after the resolveBundledClaudeBridgeBinary() call to directly check if binaryResolution.path exists rather than first checking if kind equals "resolved", ensuring unresolved bundled installs fail deterministically instead of falling back to PATH.Source: Coding guidelines
plugins/fusion-plugin-acp-runtime/src/setup.ts (1)
22-25:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash node - <<'NODE' const path = require('node:path'); const parent = 'C:\\repo\\plugins\\fusion-plugin-acp-runtime\\node_modules'; const child = 'D:\\usr\\local\\bin\\claude-code-cli-acp.cmd'; const rel = path.win32.relative(path.win32.resolve(parent), path.win32.resolve(child)); console.log({ rel, isAbsoluteRel: path.win32.isAbsolute(rel), startsWithDotDot: rel.startsWith('..') }); NODERepository: Runfusion/Fusion
Length of output: 168
🏁 Script executed:
cat -n plugins/fusion-plugin-acp-runtime/src/setup.ts | head -40Repository: Runfusion/Fusion
Length of output: 2099
🏁 Script executed:
sed -n '1,30p' plugins/fusion-plugin-acp-runtime/src/setup.tsRepository: Runfusion/Fusion
Length of output: 1233
isInsideaccepts cross-drive absolute paths on Windows, bypassing bundled-bridge identity enforcement.At lines 22-25,
relative(resolve(parent), resolve(child))can return an absolute path on Windows when comparing different drives (e.g., parent onC:\and child onD:\). The current checkrel === "" || (!rel.startsWith("..") && !rel.includes(\..${sep}`))` does not reject absolute paths, allowing external binaries to pass validation. This violates the requirement that the ACP bridge must come from the plugin's bundled node_modules.Suggested fix
-import { dirname, relative, resolve, sep } from "node:path"; +import { dirname, isAbsolute, relative, resolve, sep } from "node:path";function isInside(parent: string, child: string): boolean { const rel = relative(resolve(parent), resolve(child)); - return rel === "" || (!rel.startsWith("..") && !rel.includes(`..${sep}`)); + return rel === "" || (!isAbsolute(rel) && !rel.startsWith("..") && !rel.includes(`..${sep}`)); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.import { dirname, isAbsolute, relative, resolve, sep } from "node:path"; function isInside(parent: string, child: string): boolean { const rel = relative(resolve(parent), resolve(child)); return rel === "" || (!isAbsolute(rel) && !rel.startsWith("..") && !rel.includes(`..${sep}`)); }🤖 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 `@plugins/fusion-plugin-acp-runtime/src/setup.ts` around lines 22 - 25, The isInside function does not handle the case where relative() returns an absolute path on Windows when comparing paths on different drives. The current validation logic checks for empty strings or relative paths starting with "..", but does not reject absolute paths, allowing external binaries to bypass the bundled-bridge identity enforcement. Import isAbsolute from the path module and update the return statement in the isInside function to also check that the relative path is not absolute; the function should return true only when rel is empty, or when rel is a relative path (not starting with ".." and not containing sep followed by ".."). This ensures cross-drive paths on Windows are properly rejected.Source: Coding guidelines
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/engine/src/agent-runtime.ts`:
- Around line 127-132: The JSDoc comment above the AgentPromptResult interface
at line 127 inaccurately states "Result of creating an agent session" when the
interface actually models a prompt result as evidenced by the stopReason field.
Update the JSDoc summary to accurately describe AgentPromptResult as the result
of executing a prompt operation, ensuring the documentation reflects the actual
contract of the interface and avoids confusion for implementers.
- Around line 35-45: All requirement comments across four locations are missing
the mandated FNXC format compliance. At packages/engine/src/agent-runtime.ts
lines 35-45 (the AgentMcpServerConfig interface comment block), rewrite the
comment to include the FNXC:Area-of-product prefix and add a FNXC_LOG
date-format line. Apply the same FNXC formatting fix at
packages/engine/src/agent-runtime.ts lines 93-99 (the mcpServers option block
comment), at plugins/fusion-plugin-acp-runtime/src/types.ts lines 23-34 (the ACP
MCP-server contract block comment), and at
plugins/fusion-plugin-acp-runtime/src/types.ts lines 108-113 (the plugin
mcpServers option block comment). Each comment must follow the coding guideline:
prefix all comments with FNXC:Area-of-product and include a FNXC_LOG statement
with the date of the change.
🪄 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: 1995ebca-cb2a-4f02-a906-ca9d106dc057
📒 Files selected for processing (7)
docs/acp-contract.mddocs/plans/2026-06-14-001-feat-claude-acp-runtime-plan.mdpackages/engine/src/agent-runtime.tsplugins/fusion-plugin-acp-runtime/src/__tests__/provider-session.test.tsplugins/fusion-plugin-acp-runtime/src/provider.tsplugins/fusion-plugin-acp-runtime/src/runtime-adapter.tsplugins/fusion-plugin-acp-runtime/src/types.ts
✅ Files skipped from review due to trivial changes (1)
- docs/acp-contract.md
🚧 Files skipped from review as they are similar to previous changes (3)
- plugins/fusion-plugin-acp-runtime/src/tests/provider-session.test.ts
- plugins/fusion-plugin-acp-runtime/src/provider.ts
- plugins/fusion-plugin-acp-runtime/src/runtime-adapter.ts
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 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/engine/src/agent-runtime.ts`:
- Around line 127-132: The JSDoc comment above the AgentPromptResult interface
at line 127 inaccurately states "Result of creating an agent session" when the
interface actually models a prompt result as evidenced by the stopReason field.
Update the JSDoc summary to accurately describe AgentPromptResult as the result
of executing a prompt operation, ensuring the documentation reflects the actual
contract of the interface and avoids confusion for implementers.
- Around line 35-45: All requirement comments across four locations are missing
the mandated FNXC format compliance. At packages/engine/src/agent-runtime.ts
lines 35-45 (the AgentMcpServerConfig interface comment block), rewrite the
comment to include the FNXC:Area-of-product prefix and add a FNXC_LOG
date-format line. Apply the same FNXC formatting fix at
packages/engine/src/agent-runtime.ts lines 93-99 (the mcpServers option block
comment), at plugins/fusion-plugin-acp-runtime/src/types.ts lines 23-34 (the ACP
MCP-server contract block comment), and at
plugins/fusion-plugin-acp-runtime/src/types.ts lines 108-113 (the plugin
mcpServers option block comment). Each comment must follow the coding guideline:
prefix all comments with FNXC:Area-of-product and include a FNXC_LOG statement
with the date of the change.
🪄 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: 1995ebca-cb2a-4f02-a906-ca9d106dc057
📒 Files selected for processing (7)
docs/acp-contract.mddocs/plans/2026-06-14-001-feat-claude-acp-runtime-plan.mdpackages/engine/src/agent-runtime.tsplugins/fusion-plugin-acp-runtime/src/__tests__/provider-session.test.tsplugins/fusion-plugin-acp-runtime/src/provider.tsplugins/fusion-plugin-acp-runtime/src/runtime-adapter.tsplugins/fusion-plugin-acp-runtime/src/types.ts
✅ Files skipped from review due to trivial changes (1)
- docs/acp-contract.md
🚧 Files skipped from review as they are similar to previous changes (3)
- plugins/fusion-plugin-acp-runtime/src/tests/provider-session.test.ts
- plugins/fusion-plugin-acp-runtime/src/provider.ts
- plugins/fusion-plugin-acp-runtime/src/runtime-adapter.ts
🛑 Comments failed to post (2)
packages/engine/src/agent-runtime.ts (2)
35-45:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNew requirement comments in both contracts miss the mandated FNXC format. The shared root cause is adding policy/requirement comments without the required
FNXC:Area-of-productprefix and dated FNXC_LOG style.
packages/engine/src/agent-runtime.ts#L35-L45: Rewrite the MCP-server contract block using the FNXC prefix and date-format policy.packages/engine/src/agent-runtime.ts#L93-L99: Rewrite themcpServersoption block using the FNXC prefix and date-format policy.plugins/fusion-plugin-acp-runtime/src/types.ts#L23-L34: Rewrite the ACP MCP-server contract block using the FNXC prefix and date-format policy.plugins/fusion-plugin-acp-runtime/src/types.ts#L108-L113: Rewrite the pluginmcpServersoption block using the FNXC prefix and date-format policy.As per coding guidelines, “Add FNXC_LOG comments describing the date of the change … Write FNXC:Area-of-product in front of all comments.”
📍 Affects 2 files
packages/engine/src/agent-runtime.ts#L35-L45(this comment)packages/engine/src/agent-runtime.ts#L93-L99plugins/fusion-plugin-acp-runtime/src/types.ts#L23-L34plugins/fusion-plugin-acp-runtime/src/types.ts#L108-L113🤖 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/agent-runtime.ts` around lines 35 - 45, All requirement comments across four locations are missing the mandated FNXC format compliance. At packages/engine/src/agent-runtime.ts lines 35-45 (the AgentMcpServerConfig interface comment block), rewrite the comment to include the FNXC:Area-of-product prefix and add a FNXC_LOG date-format line. Apply the same FNXC formatting fix at packages/engine/src/agent-runtime.ts lines 93-99 (the mcpServers option block comment), at plugins/fusion-plugin-acp-runtime/src/types.ts lines 23-34 (the ACP MCP-server contract block comment), and at plugins/fusion-plugin-acp-runtime/src/types.ts lines 108-113 (the plugin mcpServers option block comment). Each comment must follow the coding guideline: prefix all comments with FNXC:Area-of-product and include a FNXC_LOG statement with the date of the change.Source: Coding guidelines
127-132:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale JSDoc summary above
AgentPromptResult.Line 127 says “Result of creating an agent session,” but the type at Line 130 models prompt result (
stopReason). Update the heading to avoid contract confusion for implementers.🤖 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/agent-runtime.ts` around lines 127 - 132, The JSDoc comment above the AgentPromptResult interface at line 127 inaccurately states "Result of creating an agent session" when the interface actually models a prompt result as evidenced by the stopReason field. Update the JSDoc summary to accurately describe AgentPromptResult as the result of executing a prompt operation, ensuring the documentation reflects the actual contract of the interface and avoids confusion for implementers.
Three-reviewer pass (correctness/security/reliability) on the highest-risk file. P0: - Break-early now gates on isPiKnownClaudeTool: Claude's internal ToolSearch (used to load deferred MCP tools) no longer aborts the turn before the real fn_* call. Surface+break works from both tool_call updates and request_permission. New test replays the U9 [ToolSearch, fn_task_list] sequence. - Downgrade a tool_use turn that surfaced zero pi tool calls -> stop (mirrors provider.ts), so pi never dispatches non-existent tools. - register the bridge child in the process registry (no orphan on teardown). - inactivity timeout (30 min, re-armed per chunk) + per-RPC timeouts on newSession (a hung bridge now ends the stream and dies). P1: - capture bridge stderr + child 'close' handler -> surface exit code/stderr (no more silent, undebuggable failures). - sanitize untrusted agent output: strip ANSI/control chars, per-chunk + per-turn caps, bound tool ids/names (no terminal-escape injection / DoS). - validate bridge path (absolute + exists) before spawn. - preserve image content blocks in the prompt (flatten-to-text dropped vision). P2: - enforce the bridge env allow-list INSIDE the driver (HOME/PATH/terminal only), not trusting the caller-supplied object. Documented residual (kill-switch stays OFF until verified): the bridge's tool-execution ordering and native-tool (Bash/Read/Write) execution-prevention need a live behavioral test before any lane enables this path. pi-claude-cli: 330/330 tests green; typecheck clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/pi-claude-cli/src/acp-driver.ts (1)
53-59: 💤 Low valueDuplicate interface definition — consider importing from
mcp-config.ts.
AcpMcpServerSpecis already defined and exported inmcp-config.ts(lines 146-152). Importing it here avoids drift between the two definitions.-/** A stdio MCP server forwarded on `session/new` (schema-only — never executed here). */ -export interface AcpMcpServerSpec { - name: string; - command: string; - args: string[]; - env: { name: string; value: string }[]; -} +import type { AcpMcpServerSpec } from "./mcp-config.js"; +export type { AcpMcpServerSpec };🤖 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/pi-claude-cli/src/acp-driver.ts` around lines 53 - 59, The AcpMcpServerSpec interface is duplicated in acp-driver.ts when it is already defined and exported in mcp-config.ts. Remove the AcpMcpServerSpec interface definition from acp-driver.ts and instead import it from mcp-config.ts at the top of the file to maintain a single source of truth and prevent future drift between the two definitions.packages/pi-claude-cli/src/mcp-config.ts (1)
161-173: 💤 Low valueConsider extracting shared logic with
writeMcpConfig.
buildAcpMcpServersduplicates the temp-file path construction, schema-file writing, and server-path resolution fromwriteMcpConfig(lines 112-124). A shared helper likewriteSchemaFile(toolDefs, cacheKey): { schemaFilePath, serverPath }would reduce duplication.🤖 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/pi-claude-cli/src/mcp-config.ts` around lines 161 - 173, The buildAcpMcpServers function contains duplicated logic for constructing the temp-file path, writing the schema file, and resolving the server path that already exists in writeMcpConfig. Extract this shared logic into a new helper function (e.g., writeSchemaFile) that accepts toolDefs and cacheKey as parameters and returns an object containing both schemaFilePath and serverPath. Update buildAcpMcpServers to call this helper instead of duplicating the path construction, file writing, and server path resolution inline. Also update writeMcpConfig to use the same helper to eliminate the duplication there as well.
🤖 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.
Nitpick comments:
In `@packages/pi-claude-cli/src/acp-driver.ts`:
- Around line 53-59: The AcpMcpServerSpec interface is duplicated in
acp-driver.ts when it is already defined and exported in mcp-config.ts. Remove
the AcpMcpServerSpec interface definition from acp-driver.ts and instead import
it from mcp-config.ts at the top of the file to maintain a single source of
truth and prevent future drift between the two definitions.
In `@packages/pi-claude-cli/src/mcp-config.ts`:
- Around line 161-173: The buildAcpMcpServers function contains duplicated logic
for constructing the temp-file path, writing the schema file, and resolving the
server path that already exists in writeMcpConfig. Extract this shared logic
into a new helper function (e.g., writeSchemaFile) that accepts toolDefs and
cacheKey as parameters and returns an object containing both schemaFilePath and
serverPath. Update buildAcpMcpServers to call this helper instead of duplicating
the path construction, file writing, and server path resolution inline. Also
update writeMcpConfig to use the same helper to eliminate the duplication there
as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 827f6fee-8608-447c-9bdc-1b31463c1faa
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
packages/pi-claude-cli/index.tspackages/pi-claude-cli/package.jsonpackages/pi-claude-cli/src/__tests__/acp-driver.test.tspackages/pi-claude-cli/src/acp-driver.tspackages/pi-claude-cli/src/mcp-config.ts
✅ Files skipped from review due to trivial changes (1)
- packages/pi-claude-cli/package.json
streamSimple routes to streamViaAcp ONLY when FUSION_CLAUDE_ACP=1 AND a bridge path is provided; otherwise stays on the -p streamViaCli path. Covers the three cases (off / flag-without-path / flag+path) and asserts the bridge path + env are forwarded. 333/333 pi-claude-cli tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…leared Live run: forwarded MCP tools and native Bash both refuse to execute when we return cancelled to session/request_permission (no TOCTOU). streamViaAcp's deny-by-default + break-early is verified safe. Env allow-list (incl. XDG/USER) validated as required for the bridged claude to authenticate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The acp-runtime plugin's onLoad now publishes the identity-pinned bundled claude-code-cli-acp path to FUSION_CLAUDE_ACP_BRIDGE (when unset), so the pi-claude-cli kill-switch resolves the bridge WITHOUT a manual env var — no engine->plugin static coupling. Publishes the path only; the ACP transport stays OFF until an operator sets FUSION_CLAUDE_ACP=1 (rollout gate). Explicit env override wins; resolver is pinned to the plugin's node_modules/.bin shim. 204/204 plugin tests green (3 new KTD10 tests); typecheck clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… docs) Two-reviewer pass (security + architecture) on KTD10 + the full Route A increment: no code defects, no P0, merge-ready as a dormant increment. Applying the P1 follow-ups: - Add the feature changeset (@runfusion/fusion minor) — the one convention gap. - KTD10 tests: fail-closed (bridge not resolved -> env stays unset -> -p) and idempotency (second onLoad keeps the first published path). - Document the two intentional, parallel MCP-forwarding paths (U10 engine-adapter vs U11 provider-driver) so nobody double-forwards, and the known ACP-path-token-usage=0 residual so U12 doesn't treat it as a bug. Reviewers confirmed: dormancy invariant holds end-to-end (nothing sets FUSION_CLAUDE_ACP=1; both flag+path required; -p is the default); OAuth pi path untouched. 206/206 plugin tests, 333/333 pi-claude-cli tests, typecheck clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
Summary
Migrates Fusion's Claude integration off
claude -ponto the existingfusion-plugin-acp-runtime(runtimeIdacp) driving the pinned externalclaude-code-cli-acpbridge. Route B shipped; Route A core (U10+U11) built, reviewed, and hardened — dormant behind an OFF kill-switch. Draft until the enablement phase + a live tool-flow verification land.Plan:
docs/plans/2026-06-14-001-feat-claude-acp-runtime-plan.md(doc-reviewed; carries full execution history).Scope: Claude CLI path only. The Claude-via-pi OAuth path is unchanged (U10 adds an optional
mcpServersfield the default pi runtime ignores).What's in this PR
mcpServersforwarding through the ACP contract (engineAgentRuntimeOptions+ plugin); defaults to[](Route B posture byte-identical). 12/12 tests.streamViaAcpdrives Claude through the bridge instead ofclaude -p, returning the sameAssistantMessageEventStream. Dispatched behind a kill-switch (FUSION_CLAUDE_ACP=1+ injected bridge path), OFF by default → the live-ppath is byte-for-byte untouched. Full-history prompting (R13), schema-only MCP + break-early on pi-known tools, env allow-list (HOME/PATH; R17). 330/330pi-claude-clitests green.Key finding (the unblock)
U9 = GO, proven live: the bridge forwards
session/new.mcpServers, Claude invokes the Fusion tool, and the call traverses the ACP permission gate. R17 closed for the login-session daemon (Keychain auth). Overturns the prior headless NOT-GO chain.Review (3 reviewers — correctness / security / reliability)
All findings fixed in
0d6b3f666. Confirmed-safe invariants: kill-switch OFF default, MCP execution containment, deny-by-default permissions.Residual Review Findings
docs/acp-contract.md): forwarded MCP tools AND native Bash both refuse to execute whenstreamViaAcpreturnscancelledtosession/request_permission— no TOCTOU window. The deny-by-default + break-early design is verified safe. Auth requires the richer env allow-list (HOME/PATH + USER/SHELL/LANG/XDG_*), which the driver already forwards.acp-driver.ts: ACP-path token usage/cost is always zero (no usage source synthesized). Cost telemetry undercounts on this path until wired.model-node verification + latency). The engine-side seam threading mcpServers into the provider is unnecessary for the dormant increment (the provider builds its own; KTD10 supplies the path). Production rollout (flippingFUSION_CLAUDE_ACPper lane) is a separate ramp decision.🤖 Generated with Claude Code
Summary by CodeRabbit