feat(acp): enable Claude CLI via ACP bridge by default (experimental switch) + status#1681
Merged
Conversation
…lt ON Replace the manual FUSION_CLAUDE_ACP env enable with an experimental feature switch. `experimentalFeatures.claudeCliAcp` is ON by default (off only when explicitly set false); the engine translates it into the FUSION_CLAUDE_ACP dispatch the pi-claude-cli provider reads, at registerExtensionProviders time. - Still fail-closed: with no bridge path published (acp-runtime plugin absent), the provider falls back to `claude -p`. - Explicit FUSION_CLAUDE_ACP env always wins (operator / test override). - New testable helper claude-acp-enable.ts (6/6 tests); flag documented in the core experimentalFeatures doc. So with the acp-runtime plugin installed, Claude CLI now routes through the ACP bridge by default; set experimentalFeatures.claudeCliAcp=false to force `-p`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
GET /providers/claude-cli/status now returns an `acp` block:
{ enabled (experimental flag), bridgeAvailable (KTD10 published path), active
(enabled && acpEnabled && bridgeAvailable) } so operators can see whether Claude
CLI is routing through the ACP bridge vs `claude -p` — important for the
default-on rollout. Additive; typechecks clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a default-on ChangesClaude CLI ACP experimental flag and auth-failure handling
Sequence DiagramsequenceDiagram
participant User
participant Dashboard
participant Engine
participant ACPBridge
participant SignalFile as Temp File<br/>fusion-acp-bridge-auth.json
User->>Dashboard: Access Claude CLI status
Dashboard->>Engine: GET /providers/claude-cli/status
Engine->>SignalFile: Read auth-failure signal
SignalFile-->>Engine: authFailed, authReason
Engine-->>Dashboard: ACP state (enabled, bridgeAvailable, active, authFailed)
Dashboard-->>User: Show alert if authFailed=true
User->>Dashboard: Click "Use claude -p"
Dashboard->>Dashboard: Disable experimentalFeatures.claudeCliAcp
Dashboard->>SignalFile: Clear signal file
Dashboard-->>User: Show fallback active message
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
Contributor
Greptile SummaryThis PR flips the Route A ACP transport (Claude CLI via
Confidence Score: 4/5Safe to merge with one known UI state bug: the R17 auth-failure banner doesn't dismiss after clicking "Use claude -p". The enable/disable logic and status endpoint are sound. The one defect is in the R17 fallback flow: packages/dashboard/src/routes/register-auth-routes.ts and packages/dashboard/app/components/ClaudeCliProviderCard.tsx — the interplay between the fallback handler and the signal file needs the banner render condition or the status response to gate authFailed on whether ACP is actually enabled. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[createFnAgent / registerExtensionProviders] --> B[applyClaudeAcpEnable]
B --> C{FUSION_CLAUDE_ACP_FORCE?}
C -- =1 --> D[FUSION_CLAUDE_ACP=1]
C -- =0 --> E[FUSION_CLAUDE_ACP=0]
C -- not set --> F{claudeCliAcp !== false?}
F -- true --> D
F -- false --> E
D --> G[pi-claude-cli dispatch]
G --> H{FUSION_CLAUDE_ACP_BRIDGE resolvable?}
H -- yes --> I[streamViaAcp]
H -- no --> J[streamViaCli claude -p]
I --> K{Turn content?}
K -- short Not-logged-in --> L[write signal file]
K -- real response/tools --> M[delete signal file]
N[GET /providers/claude-cli/status] --> O[read FUSION_CLAUDE_ACP + signal file]
O --> P[acp: enabled/bridgeAvailable/active/authFailed]
P --> Q{authFailed?}
Q -- yes --> R[UI: auth-failure banner]
R --> S[handleFallbackToDashP: claudeCliAcp=false]
S --> T[refresh]
T -.->|signal file NOT cleared| O
Reviews (4): Last reviewed commit: "fix(review): address PR #1681 round-2 co..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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 (1)
packages/engine/src/claude-acp-enable.ts (1)
1-13: ⚡ Quick winAlign new ACP comments with the FNXC comment format requirement.
The new comments/JSDoc added here are missing the
FNXC:Area-of-productprefix and timestamp convention required by the repo guidelines.As per coding guidelines, “Add FNXC_LOG comments describing the date of the change (format yyyy-MM-dd-hh:mm) ... Write FNXC:Area-of-product in front of all comments.”
Also applies to: 23-27
🤖 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/claude-acp-enable.ts` around lines 1 - 13, Add the required FNXC comment format to all comments in this module. For the JSDoc comment block at the beginning of the file (lines 1-13) and any other comments mentioned in "Also applies to: 23-27", prepend each comment with the FNXC:Area-of-product prefix and include a timestamp in the format yyyy-MM-dd-hh:mm according to the repository guidelines. Ensure all comments follow the established FNXC convention to comply with coding standards.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 `@packages/dashboard/src/routes/register-auth-routes.ts`:
- Around line 600-612: The calculation for acp.active in the status response
does not account for the explicit FUSION_CLAUDE_ACP environment variable
override, which can cause the endpoint to report active: true when the actual
runtime routing is disabled. Add a check for the FUSION_CLAUDE_ACP environment
variable (similar to the existing check for FUSION_CLAUDE_ACP_BRIDGE) and
include it in the active field calculation so that acp.active accurately
reflects whether ACP will actually be active at runtime, respecting operator
overrides.
In `@packages/engine/src/__tests__/claude-acp-enable.test.ts`:
- Around line 18-38: Add a new test case in the applyClaudeAcpEnable describe
block that verifies the function correctly reverses a previously set
FUSION_CLAUDE_ACP environment variable. Create a test that initializes a
NodeJS.ProcessEnv object with FUSION_CLAUDE_ACP set to "1", then calls
applyClaudeAcpEnable with { experimentalFeatures: { claudeCliAcp: false } }, and
asserts both that the function returns false and that FUSION_CLAUDE_ACP is
properly cleared or set to "0" in the env object. This regression test ensures
the sticky routing bug is prevented where a prior auto-set value could remain
when the flag is later disabled.
In `@packages/engine/src/claude-acp-enable.ts`:
- Around line 32-35: The function applyClaudeAcpEnable latches ACP to enabled
once it is auto-enabled, preventing later disablement through settings changes.
The issue is on line 32 where any existing FUSION_CLAUDE_ACP environment
variable is treated as authoritative, but line 34 writes to this variable when
claudeAcpExperimentalEnabled returns true, causing subsequent calls to always
return true regardless of current settings. Remove or refactor the early return
check on line 32 that treats the environment variable as authoritative, and
ensure the function always evaluates the current settings through
claudeAcpExperimentalEnabled(globalSettings) to allow settings changes to
dynamically enable or disable ACP in a long-lived process.
---
Nitpick comments:
In `@packages/engine/src/claude-acp-enable.ts`:
- Around line 1-13: Add the required FNXC comment format to all comments in this
module. For the JSDoc comment block at the beginning of the file (lines 1-13)
and any other comments mentioned in "Also applies to: 23-27", prepend each
comment with the FNXC:Area-of-product prefix and include a timestamp in the
format yyyy-MM-dd-hh:mm according to the repository guidelines. Ensure all
comments follow the established FNXC convention to comply with coding standards.
🪄 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: 4d6fe21a-be68-431f-a00e-3f15079e160f
📒 Files selected for processing (5)
packages/core/src/types.tspackages/dashboard/src/routes/register-auth-routes.tspackages/engine/src/__tests__/claude-acp-enable.test.tspackages/engine/src/claude-acp-enable.tspackages/engine/src/pi.ts
Comment on lines
+600
to
+612
| const acpBridgeAvailable = | ||
| typeof process.env.FUSION_CLAUDE_ACP_BRIDGE === "string" && | ||
| process.env.FUSION_CLAUDE_ACP_BRIDGE.length > 0; | ||
|
|
||
| res.json({ | ||
| binary, | ||
| enabled, | ||
| extension, | ||
| acp: { | ||
| enabled: acpEnabled, | ||
| bridgeAvailable: acpBridgeAvailable, | ||
| active: enabled && acpEnabled && acpBridgeAvailable, | ||
| }, |
Contributor
There was a problem hiding this comment.
acp.active should include explicit FUSION_CLAUDE_ACP override resolution.
Line 611 can report active: true when an operator/test sets FUSION_CLAUDE_ACP=0. That makes the status endpoint disagree with actual runtime routing.
Proposed fix
const acpBridgeAvailable =
typeof process.env.FUSION_CLAUDE_ACP_BRIDGE === "string" &&
process.env.FUSION_CLAUDE_ACP_BRIDGE.length > 0;
+ const acpEnvOverride = process.env.FUSION_CLAUDE_ACP;
+ const acpResolvedEnabled =
+ typeof acpEnvOverride === "string" ? acpEnvOverride === "1" : acpEnabled;
res.json({
binary,
enabled,
extension,
acp: {
enabled: acpEnabled,
bridgeAvailable: acpBridgeAvailable,
- active: enabled && acpEnabled && acpBridgeAvailable,
+ active: enabled && acpResolvedEnabled && acpBridgeAvailable,
},🤖 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/src/routes/register-auth-routes.ts` around lines 600 -
612, The calculation for acp.active in the status response does not account for
the explicit FUSION_CLAUDE_ACP environment variable override, which can cause
the endpoint to report active: true when the actual runtime routing is disabled.
Add a check for the FUSION_CLAUDE_ACP environment variable (similar to the
existing check for FUSION_CLAUDE_ACP_BRIDGE) and include it in the active field
calculation so that acp.active accurately reflects whether ACP will actually be
active at runtime, respecting operator overrides.
…auth (R17) When the bridged `claude` can't authenticate (detached daemon / no keychain), the turn returns "Not logged in" instead of a real answer. Rather than silently relay that, detect it and let the user choose. - Driver: detect a "Not logged in"-only turn and write a cross-process signal (fusion-acp-bridge-auth.json); a real response clears it (acp-driver test). - Dashboard status: GET /providers/claude-cli/status reports acp.authFailed + authReason from the signal. - UI: the Claude CLI provider card shows an auth-failure banner with "Use claude -p" (sets experimentalFeatures.claudeCliAcp=false) and "I fixed auth — re-test", plus a fix hint (run `claude` to log in). - Enable resolution now recomputes each call with an operator force-override (FUSION_CLAUDE_ACP_FORCE), so the "Use -p" fallback takes effect on the next turn — no restart. claude-acp-enable tests updated. pi-claude-cli + engine tests green; dashboard typecheck clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…mments) - Greptile P2: `acp.active` now reflects the ACTUAL dispatch determinant (FUSION_CLAUDE_ACP, which includes the operator force-override), not the experimental flag alone — so the status isn't misleading when forced on/off. - CodeRabbit/Greptile P2: add FNXC:ClaudeAcp comments to the new code blocks per the AGENTS.md greppable-comment convention. Already fixed in the prior commit (daa37d0): the P1 "sticky env" / latch (applyClaudeAcpEnable now recomputes each call + FUSION_CLAUDE_ACP_FORCE override) and the enable->disable-on-same-env regression test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/dashboard/app/components/ClaudeCliProviderCard.tsx`:
- Line 263: Fix the inconsistent Tailwind CSS animation class name in the
Loader2 component. On line 263, change the className from "spin" to
"animate-spin" to match the correct Tailwind animation class used elsewhere in
the file (such as line 169). The current "spin" class name does not exist in the
Tailwind CSS framework and will prevent the spinner from animating.
In `@packages/pi-claude-cli/src/__tests__/acp-driver.test.ts`:
- Around line 124-135: The test "records the R17 auth-failure signal when the
bridge turn is only 'Not logged in'" only verifies the positive case where
authFailed is set to true, but does not assert the invariant across other
branches. Expand this test to include additional test cases or assertions within
the existing test that verify: (1) that normal agent messages with actual
content do not set authFailed:true, and (2) that tool-call turns do not set
authFailed:true even when the text contains login-related keywords. This ensures
the authFailed signal fix works correctly across all relevant code branches and
prevents false positives in different message types handled by the streamViaAcp
function.
In `@packages/pi-claude-cli/src/acp-driver.ts`:
- Line 90: The regex pattern NOT_LOGGED_IN_RE matches too broadly, detecting the
auth-failure keywords anywhere in a response rather than requiring them to be
the primary content of the turn. Update the NOT_LOGGED_IN_RE regex to use
anchors (^ and $) to match only when the not-logged-in or login message
constitutes the core response, not merely a substring within additional
legitimate content. This ensures the auth-failure detection at the usage site in
lines 209-214 only triggers for true authentication failures where the response
is specifically about missing login, preventing false positives that incorrectly
surface the auth-failure UI state.
🪄 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: a2b75e07-eb8e-48fd-8cfd-410d244cbf1f
📒 Files selected for processing (7)
packages/dashboard/app/api/legacy.tspackages/dashboard/app/components/ClaudeCliProviderCard.tsxpackages/dashboard/src/routes/register-auth-routes.tspackages/engine/src/__tests__/claude-acp-enable.test.tspackages/engine/src/claude-acp-enable.tspackages/pi-claude-cli/src/__tests__/acp-driver.test.tspackages/pi-claude-cli/src/acp-driver.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/engine/src/tests/claude-acp-enable.test.ts
Comment on lines
+124
to
+135
| it("records the R17 auth-failure signal when the bridge turn is only 'Not logged in'", async () => { | ||
| fsSpies.writeFileSync.mockClear(); | ||
| scriptedUpdates = [ | ||
| { sessionUpdate: "agent_message_chunk", content: { type: "text", text: "Not logged in · Please run /login" } }, | ||
| ]; | ||
| const stream = streamViaAcp(MODEL, CTX, OPTS) as unknown as { _events: Array<Record<string, unknown>> }; | ||
| await flush(); | ||
| // signal file written with authFailed:true | ||
| const wrote = fsSpies.writeFileSync.mock.calls.find((c) => String(c[1]).includes("authFailed")); | ||
| expect(wrote).toBeTruthy(); | ||
| expect(String(wrote![1])).toContain("\"authFailed\":true"); | ||
| }); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Expand this regression to assert the full auth-signal invariant across known surfaces.
This test only checks the reported symptom (authFailed:true write). Please also cover at least: (1) non-auth real output clears/doesn’t set auth-failed, and (2) tool-call turns don’t set auth-failed even if text mentions login, so the fix is protected across all relevant branches.
Suggested additions
+ it("clears auth-failure signal when a subsequent real response arrives", async () => {
+ fsSpies.writeFileSync.mockClear();
+ fsSpies.unlinkSync.mockClear();
+ scriptedUpdates = [{ sessionUpdate: "agent_message_chunk", content: { type: "text", text: "Not logged in · Please run /login" } }];
+ const s1 = streamViaAcp(MODEL, CTX, OPTS) as unknown as { _events: Array<Record<string, unknown>> };
+ await flush();
+ expect(eventsOf(s1).some((e) => e.type === "done")).toBe(true);
+
+ scriptedUpdates = [{ sessionUpdate: "agent_message_chunk", content: { type: "text", text: "Hello from Claude" } }];
+ const s2 = streamViaAcp(MODEL, CTX, OPTS) as unknown as { _events: Array<Record<string, unknown>> };
+ await flush();
+ expect(eventsOf(s2).some((e) => e.type === "done")).toBe(true);
+ expect(fsSpies.unlinkSync).toHaveBeenCalled();
+ });As per coding guidelines, for bug fixes the regression must assert the general invariant across all known surfaces, not just the single reproduction.
🤖 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/__tests__/acp-driver.test.ts` around lines 124 -
135, The test "records the R17 auth-failure signal when the bridge turn is only
'Not logged in'" only verifies the positive case where authFailed is set to
true, but does not assert the invariant across other branches. Expand this test
to include additional test cases or assertions within the existing test that
verify: (1) that normal agent messages with actual content do not set
authFailed:true, and (2) that tool-call turns do not set authFailed:true even
when the text contains login-related keywords. This ensures the authFailed
signal fix works correctly across all relevant code branches and prevents false
positives in different message types handled by the streamViaAcp function.
Source: Coding guidelines
- CodeRabbit: spinner class `spin` -> `animate-spin` (matches the card's other Loader2 usages). - CodeRabbit (major): tighten auth-failure detection so it only fires when the WHOLE turn is the short "Not logged in" message (<=80 chars), not when a long legitimate answer merely mentions the phrase — avoids false positives. - CodeRabbit (major): expand the auth-signal test to assert the full invariant — set on a not-logged-in turn, clear (unlink) on a real response, and NOT flag a long answer that mentions the phrase. pi-claude-cli acp-driver 5/5; typecheck clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Comment on lines
+634
to
+639
| acp: { | ||
| enabled: acpEnabled, | ||
| bridgeAvailable: acpBridgeAvailable, | ||
| active: enabled && acpBridgeAvailable && acpEnvOn, | ||
| authFailed: acpAuthFailed, | ||
| authReason: acpAuthReason, |
Contributor
There was a problem hiding this comment.
handleFallbackToDashP sets claudeCliAcp: false in settings and calls refresh(), but neither deletes the signal file nor asks the server to clear it. The next GET /providers/claude-cli/status call still finds the file, so authFailed: true is returned even though ACP has just been disabled. The UI banner only checks status?.acp?.authFailed (not acp.enabled), so "Claude CLI bridge can't authenticate" keeps showing indefinitely — the signal can only be cleared by a successful ACP turn, but ACP is now off.
The simplest server-side fix is to gate authFailed on acpEnabled:
authFailed: acpEnabled && acpAuthFailed,
authReason: acpEnabled ? acpAuthReason : undefined,
Alternatively, handleFallbackToDashP could call a dedicated clear endpoint, or the JSX could add && status?.acp?.enabled to the render condition.
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
Follow-up to #1680 (Route A core, merged). Flips the Route A ACP transport from a manual
FUSION_CLAUDE_ACPenv var to an experimental feature switch, default ON, and surfaces its state to operators.Changes
experimentalFeatures.claudeCliAcpis on unless explicitly setfalse. The engine translates it into theFUSION_CLAUDE_ACPdispatch thepi-claude-cliprovider reads, atregisterExtensionProviderstime (new testable helperclaude-acp-enable.ts, 6 tests).claude -p. So "default on" only engages where the bridge is actually available.FUSION_CLAUDE_ACPenv always wins (operator / test override).GET /providers/claude-cli/statusnow returns anacpblock (enabled/bridgeAvailable/active) so operators can see whether Claude CLI is routing through the ACP bridge vs-p.Net effect
With the acp-runtime plugin installed, Claude CLI routes through the
claude-code-cli-acpbridge by default; setexperimentalFeatures.claudeCliAcp=falseto force-p. The Claude-via-pi OAuth path is unchanged.The safety gate this rests on (forwarded MCP tools + native tools refuse to execute when cancelled; no TOCTOU) was verified live in #1680.
Remaining
U13 (workflow
model-node regression) is largely covered by the dispatch + enable unit tests (the workflow path goes through the samecreateFnAgent → streamSimplerouting). Runtime fallback-on-auth-failure (detached-daemon robustness) and OS sandboxing remain follow-ups.🤖 Generated with Claude Code
Summary by CodeRabbit
-pwith re-test support.claudeCliAcp, including effectiveness conditions and how to force-p.