feat(acp): Route A follow-ups — token usage, opt-in headless auth, status tests + bridge-auth learning#1682
Merged
Merged
Conversation
…in session isolation Compound learning: the claude-code-cli-acp bridge returned 'Not logged in' despite a working claude -p, due to (1) a too-thin spawn env (needs XDG_*/USER/ SHELL beyond HOME/PATH) and (2) macOS login-Keychain session isolation for detached/headless processes. Six headless tasks misdiagnosed it as an upstream gap. Cross-linked from the ACP runtime integration pattern doc. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…l (U12)
GET /providers/claude-cli/status: asserts acp.{enabled,bridgeAvailable,active,
authFailed,authReason} reflect the FUSION_CLAUDE_ACP env + the bridge auth-failure
signal file, and that acp is inactive/clean when no bridge path is published.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Item 2 (OQ3): capture PromptResponse.usage from conn.prompt and feed it into the bridge before finish(), so ACP-path turns report token usage/cost instead of always zero. Zero-when-absent is safe; tool-use (break-early) turns inherently report zero (the prompt result never resolves). - Item 3 (R17): opt-in headless credential delivery. When FUSION_CLAUDE_ACP_FORWARD_AUTH=1, buildBridgeEnv forwards a SINGLE Claude auth token (CLAUDE_CODE_OAUTH_TOKEN > ANTHROPIC_AUTH_TOKEN > ANTHROPIC_API_KEY) from the operator's launch env so a detached daemon (no login Keychain) can authenticate. Default OFF — the secure no-secrets posture is unchanged. acp-driver tests 9/9 (usage + the three auth-opt-in cases); typecheck clean. Remaining: item 1 (connection reuse / resume latency). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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 due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesACP Bridge Auth, Token Usage, Connection Reuse, and Documentation
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 |
Contributor
Greptile SummaryThis PR delivers four follow-up items for the ACP (Route A) transport: opt-in warm connection reuse (
Confidence Score: 5/5Safe to merge. All new behaviour is opt-in behind env flags (default OFF), the default cold ACP path is functionally unchanged, and the reuse teardown logic correctly handles the tool-use break edge case. The connection-reuse logic is the most complex addition, and on inspection the key hazards (tool-use break releasing a pending prompt, cross-turn bleed, stale eviction of a newer entry's child, warm-child death hanging a reuse turn) are all correctly guarded and covered by dedicated tests. The only finding is a stale file-header sentence that no longer describes the reuse path accurately. packages/pi-claude-cli/src/acp-driver.ts — the file header comment about buildResumePrompt is now inaccurate; otherwise the implementation and tests look correct. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[streamViaAcp called] --> B{FUSION_CLAUDE_ACP_REUSE=1
& sessionId
& messages.length > 1?}
B -- No --> COLD[Cold path]
B -- Yes --> C{acpSessionCache.get
reuseKey?}
C -- Miss --> COLD
C -- Hit --> D{warm.inUse
or cwd mismatch?}
D -- Yes --> COLD
D -- No --> E[buildResumePrompt]
E --> F{Delta empty?}
F -- Yes --> EVICT[evictCachedAcpConn
cold-start instead]
EVICT --> COLD
F -- No --> WARM[Reuse path
skip spawn + session/new
prompt delta only]
WARM --> G{Tool-use break?}
G -- Yes --> TEARDOWN[endTurn sawToolCall=true
keepWarm=false
evict + kill]
G -- No --> RELEASE[endTurn keepWarm=true
set idle timer 5min
router handlers nulled]
COLD --> H[spawn bridge
registerProcess
create router]
H --> I[conn.initialize
conn.newSession]
I --> J{reuseKey set?}
J -- Yes --> CACHE[acpSessionCache.set]
J -- No --> PROMPT
CACHE --> PROMPT[conn.prompt full history]
PROMPT --> K{Tool-use break?}
K -- Yes --> TEARDOWN
K -- No --> L[emitUsage
finish stop]
L --> RELEASE
RELEASE --> IDLE[(Warm connection idle in cache)]
IDLE -- Next turn same sessionId --> C
IDLE -- Idle timeout 5min --> EVICT2[evictCachedAcpConn kill child]
IDLE -- Child dies --> EVICT2
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[streamViaAcp called] --> B{FUSION_CLAUDE_ACP_REUSE=1
& sessionId
& messages.length > 1?}
B -- No --> COLD[Cold path]
B -- Yes --> C{acpSessionCache.get
reuseKey?}
C -- Miss --> COLD
C -- Hit --> D{warm.inUse
or cwd mismatch?}
D -- Yes --> COLD
D -- No --> E[buildResumePrompt]
E --> F{Delta empty?}
F -- Yes --> EVICT[evictCachedAcpConn
cold-start instead]
EVICT --> COLD
F -- No --> WARM[Reuse path
skip spawn + session/new
prompt delta only]
WARM --> G{Tool-use break?}
G -- Yes --> TEARDOWN[endTurn sawToolCall=true
keepWarm=false
evict + kill]
G -- No --> RELEASE[endTurn keepWarm=true
set idle timer 5min
router handlers nulled]
COLD --> H[spawn bridge
registerProcess
create router]
H --> I[conn.initialize
conn.newSession]
I --> J{reuseKey set?}
J -- Yes --> CACHE[acpSessionCache.set]
J -- No --> PROMPT
CACHE --> PROMPT[conn.prompt full history]
PROMPT --> K{Tool-use break?}
K -- Yes --> TEARDOWN
K -- No --> L[emitUsage
finish stop]
L --> RELEASE
RELEASE --> IDLE[(Warm connection idle in cache)]
IDLE -- Next turn same sessionId --> C
IDLE -- Idle timeout 5min --> EVICT2[evictCachedAcpConn kill child]
IDLE -- Child dies --> EVICT2
Reviews (3): Last reviewed commit: "fix(review): address PR #1682 re-review ..." | 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
🤖 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/solutions/integration-issues/acp-bridge-not-logged-in-thin-env-keychain-isolation.md`:
- Around line 43-44: The documentation at lines 43-44 states "never API keys"
when describing the secure default behavior for environment variable forwarding
to the bridge subprocess, but does not acknowledge the opt-in exception that
allows forwarding exactly one auth credential when
FUSION_CLAUDE_ACP_FORWARD_AUTH=1 is set in the implementation. Update the
wording in both the main section (lines 43-44) and the related section (lines
79-83) to explicitly distinguish between the secure default behavior (no
credentials forwarded) and the explicit opt-in mechanism
(FUSION_CLAUDE_ACP_FORWARD_AUTH=1 environment variable). This clarification will
align the documentation with the actual implementation behavior.
In `@packages/pi-claude-cli/src/__tests__/acp-driver.test.ts`:
- Around line 173-176: The auth-forwarding tests are not properly isolating and
restoring the ANTHROPIC_AUTH_TOKEN environment variable, which can cause test
interference and leave an important surface unasserted. At the anchor site in
acp-driver.test.ts lines 173-176, add ANTHROPIC_AUTH_TOKEN to the saved object
alongside FUSION_CLAUDE_ACP_FORWARD_AUTH, CLAUDE_CODE_OAUTH_TOKEN, and
ANTHROPIC_API_KEY, then include it in the restoration logic within the afterEach
hook. Apply the same change pattern at the sibling sites in acp-driver.test.ts
lines 188-194 and 196-203 where similar test setup and teardown occurs, ensuring
all three locations capture, save, and restore ANTHROPIC_AUTH_TOKEN
consistently.
In `@packages/pi-claude-cli/src/acp-driver.ts`:
- Around line 143-147: The condition checking `v.length > 0` in the for loop
iterating over BRIDGE_AUTH_ENV_KEYS accepts whitespace-only strings as valid
authentication values, which can incorrectly prioritize an invalid
higher-preference token over valid lower-preference credentials. Modify the
condition to trim whitespace from the value and check that the trimmed result
has length greater than 0 before assigning it to env[key], ensuring
whitespace-only strings are treated as absent.
🪄 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: 1c3d984e-735d-4774-91f2-2506c744b21d
📒 Files selected for processing (5)
docs/solutions/architecture-patterns/acp-persistent-jsonrpc-agent-runtime-integration.mddocs/solutions/integration-issues/acp-bridge-not-logged-in-thin-env-keychain-isolation.mdpackages/dashboard/src/__tests__/routes-auth.test.tspackages/pi-claude-cli/src/__tests__/acp-driver.test.tspackages/pi-claude-cli/src/acp-driver.ts
…he tokens) - P2: event-bridge handleMessageDelta now consumes cache_read/cache_creation tokens (parity with handleMessageStart) — the OQ3 usage path carried them but they were silently dropped, understating cost for cached turns. - P2: validate the untrusted bridge usage payload — coerce each field to a finite, non-negative number before forwarding, so a malformed value (string/NaN/negative) can't corrupt totalTokens/cost. - Tests: usage now asserts cache tokens + totalTokens; new cases for malformed usage, tool-use turns reporting zero usage, the ANTHROPIC_AUTH_TOKEN middle precedence, and that the auth token is read from process.env (never a caller-supplied value — no token substitution). - Doc: state the auth-forwarding exposure trade-off in the code comment. acp-driver 13/13; event-bridge tests green; typecheck clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Keep a warm bridge connection + ACP session across turns of one conversation (gated by FUSION_CLAUDE_ACP_REUSE=1, default OFF), so multi-turn lanes skip the cold bridge/claude spawn and session/new round-trip and send only the latest-turn delta (buildResumePrompt). A stable router indirection serves each turn's handlers. Addresses the adversarial review of the reuse path: - P0: a warm-child death routes failure to the CURRENT owner turn via router.fail, so a reuse turn fails fast instead of hanging until the 30-min inactivity timeout. - P1: eviction is cache-identity-aware (evictCachedAcpConn only deletes the map key when it still points at the entry), so a concurrent cold turn / stale close handler / idle timer can't evict or kill a newer live entry's child. - P1: an empty resume delta cold-starts instead of issuing an empty prompt that could hang. - P2: a per-turn token drops cross-turn stray updates on the shared warm connection. - The idle reaper is unref'd so it never pins the process. Default OFF → the cold path is functionally unchanged (reviewer-verified). Adds multi-turn tests: reuse skips spawn+session/new, flag-off spawns fresh, fail-fast on warm-child death, empty-resume cold fallback. 346/346 pass, tsc clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/pi-claude-cli/src/__tests__/acp-driver.test.ts (1)
346-352:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear
ANTHROPIC_AUTH_TOKENin this case to keep precedence deterministic.This test expects API-key forwarding, but it can fail when ambient
process.env.ANTHROPIC_AUTH_TOKENis set. Adddelete process.env.ANTHROPIC_AUTH_TOKEN;in this test setup.As per coding guidelines, “the regression test must assert the general invariant across ALL known surfaces — not only the single reported reproduction.”
Proposed fix
it("forwards a single auth token when opted in", () => { process.env.FUSION_CLAUDE_ACP_FORWARD_AUTH = "1"; delete process.env.CLAUDE_CODE_OAUTH_TOKEN; + delete process.env.ANTHROPIC_AUTH_TOKEN; process.env.ANTHROPIC_API_KEY = "sk-secret"; const env = buildBridgeEnv({ HOME: "/h", PATH: "/b" }); expect(env.ANTHROPIC_API_KEY).toBe("sk-secret"); });🤖 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 346 - 352, The test function "forwards a single auth token when opted in" deletes CLAUDE_CODE_OAUTH_TOKEN and sets ANTHROPIC_API_KEY, but does not delete ANTHROPIC_AUTH_TOKEN from the environment. This can cause the test to fail non-deterministically if ANTHROPIC_AUTH_TOKEN is set in the ambient environment, since token precedence becomes unpredictable. Add a delete statement for process.env.ANTHROPIC_AUTH_TOKEN in the test setup, right after the other environment variable manipulations, to ensure the test behavior is deterministic regardless of the ambient environment state.Source: Coding guidelines
🧹 Nitpick comments (1)
packages/pi-claude-cli/src/__tests__/acp-driver.test.ts (1)
77-110: Use fake timers inbeforeEachinstead of real-timeflush()waits.The tests currently depend on a 30ms
setTimeoutto allow event-loop settlement before pollingstream._events. This pattern increases flakiness and suite runtime. Replace withvi.useFakeTimers()inbeforeEach, then callvi.runAllTimersAsync()orvi.advanceTimersToNextTimerAsync()to deterministically advance through pending callbacks.Per coding guidelines: "Prefer fake timers over real polling/time waits. Do not mask slowness by raising worker/concurrency knobs."
Existing precedent:
packages/pi-claude-cli/src/__tests__/process-manager.test.tsandprovider.test.tsalready use this pattern withvi.useFakeTimers()inbeforeEach/vi.useRealTimers()inafterEach.🤖 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 77 - 110, Replace the real-time setTimeout-based flush() pattern in the acp-driver.test.ts tests with Vitest fake timers to improve determinism and reduce flakiness. Add vi.useFakeTimers() to a beforeEach hook and vi.useRealTimers() to an afterEach hook. In each test function (feeds ACP token usage, ignores a malformed/untrusted usage payload, and does not emit usage on a tool-use turn), replace the await flush() call with vi.runAllTimersAsync() to deterministically advance through pending callbacks instead of relying on real-time delays. Follow the existing pattern already implemented in process-manager.test.ts and provider.test.ts in the same package.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.
Duplicate comments:
In `@packages/pi-claude-cli/src/__tests__/acp-driver.test.ts`:
- Around line 346-352: The test function "forwards a single auth token when
opted in" deletes CLAUDE_CODE_OAUTH_TOKEN and sets ANTHROPIC_API_KEY, but does
not delete ANTHROPIC_AUTH_TOKEN from the environment. This can cause the test to
fail non-deterministically if ANTHROPIC_AUTH_TOKEN is set in the ambient
environment, since token precedence becomes unpredictable. Add a delete
statement for process.env.ANTHROPIC_AUTH_TOKEN in the test setup, right after
the other environment variable manipulations, to ensure the test behavior is
deterministic regardless of the ambient environment state.
---
Nitpick comments:
In `@packages/pi-claude-cli/src/__tests__/acp-driver.test.ts`:
- Around line 77-110: Replace the real-time setTimeout-based flush() pattern in
the acp-driver.test.ts tests with Vitest fake timers to improve determinism and
reduce flakiness. Add vi.useFakeTimers() to a beforeEach hook and
vi.useRealTimers() to an afterEach hook. In each test function (feeds ACP token
usage, ignores a malformed/untrusted usage payload, and does not emit usage on a
tool-use turn), replace the await flush() call with vi.runAllTimersAsync() to
deterministically advance through pending callbacks instead of relying on
real-time delays. Follow the existing pattern already implemented in
process-manager.test.ts and provider.test.ts in the same package.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3e2b4480-1281-4f95-9767-e2c0f3128d89
📒 Files selected for processing (4)
.changeset/acp-route-a-claude-cli-bridge.mdpackages/pi-claude-cli/src/__tests__/acp-driver.test.tspackages/pi-claude-cli/src/acp-driver.tspackages/pi-claude-cli/src/event-bridge.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/acp-route-a-claude-cli-bridge.md
…dening) - P1 (Greptile): a tool-use break-early turn released the warm connection (inUse=false) while conn.prompt() was still pending, letting the next turn launch a concurrent prompt on the same ACP session (protocol corruption). keepWarm now requires !sawToolCall, so a tool-use turn tears the connection down like the non-reuse path; only a clean stop turn (prompt fully resolved before finish) keeps it warm. + test. - buildBridgeEnv: treat a whitespace-only auth var as absent (v.trim()), so a blank higher-preference token can't shadow a real lower-preference one and we never forward a useless blank token. + test. - Auth-forwarding tests: clear ambient auth vars in beforeEach so a runner-env token can't shadow the case under test (CodeRabbit). - Doc: clarify the allow-list never carries API keys by default; the single FUSION_CLAUDE_ACP_FORWARD_AUTH opt-in (default OFF) is the only exception. 348/348 pass, tsc clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Ready to review this PR? Stage has broken it down into 5 individual chapters for you: 
Chapters generated by Stage for commit 65c4958 on Jun 16, 2026 9:20am UTC. |
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
Route A follow-ups (after #1680 + #1681, merged): the remaining items from the post-merge backlog, now including item 1 (connection reuse, OQ2). Plus a compound learning doc capturing the hard-won bridge-auth diagnosis.
Changes
docs/solutions/integration-issues/acp-bridge-not-logged-in-thin-env-keychain-isolation.md: theclaude-code-cli-acp"Not logged in" failure (thin spawn env needingXDG_*/USER/SHELL, + macOS login-Keychain session isolation for headless processes) that defeated six autonomous tasks. Cross-linked from the ACP runtime pattern doc.streamViaAcpnow capturesPromptResponse.usageand feeds it into the event bridge, so ACP-path turns report token usage/cost instead of always zero. Zero-when-absent is safe; tool-use (break-early) turns inherently report zero. Untrusted bridge usage is coerced to finite, non-negative numbers; cache-read/creation tokens are propagated.FUSION_CLAUDE_ACP_FORWARD_AUTH=1, the bridge env forwards a single Claude auth token (CLAUDE_CODE_OAUTH_TOKEN>ANTHROPIC_AUTH_TOKEN>ANTHROPIC_API_KEY) from the operator's launch env, so a detached daemon with no login Keychain can authenticate. Default OFF — the secure no-secrets posture is unchanged.GET /providers/claude-cli/statusacpblock + auth-failure signal are now tested.Now included — item 1 (connection reuse / resume latency, OQ2)
Opt-in warm connection reuse, gated behind
FUSION_CLAUDE_ACP_REUSE(default OFF so the default path stays functionally identical — reviewer-verified). When ON andsessionId && messages.length>1 && cache hit && cwd match && !inUse, a turn reuses the warm bridge connection + ACP session, skipping the cold bridge/claudespawn andsession/newround-trip and sending only the latest-turn delta (buildResumePrompt) since the warmclaudesession already holds prior turns server-side. A stablerouterindirection lets the long-lived connection handler serve each turn's fresh state.Hardened per an adversarial review of the reuse path:
child.on("close")is bound to the cold turn that spawned it; arouter.failliveness hook routes a mid-prompt child death to the current owner turn, so a reuse turn ends immediately instead of hanging until the 30-min inactivity timeout.evictCachedAcpConn(key, entry)deletes the map key only when it still points at that entry, so a concurrent cold turn / stale close handler / idle timer can't evict or kill a newer live entry's child.unref'd so it never pins the process.New multi-turn tests: turn 2 reuses (no second spawn /
session/new, single connection'sprompt()twice), flag-off spawns fresh each turn, fail-fast on warm-child death, and empty-resume cold fallback.Tests: pi-claude-cli 346, engine enable 6, dashboard acp 2 — all green; typecheck clean. The default
claude -ppath and default ACP behavior are unchanged (all additions are additive/opt-in).Also genuinely deferred (operational, not code-fixable here)
FUSION_CLAUDE_ACP_REUSEon by default.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests