feat(acp): terminal authentication for agent login - #99
Merged
Merged
Conversation
Agents that require login (MiniMax Code and similar) advertise authMethods only when the client declares clientCapabilities.auth.terminal. Our advertisement was dead code: enableTerminalAuth was computed from the initialize response's authMethods inside the initialize request itself, so it was always false and terminal-auth agents could never offer their login flow. In the normal session flow, auth_required failures surfaced as raw JSON-RPC error text (or were swallowed at draft preparation), and no execution path existed for terminal methods. - fix capability advertisement: auth.terminal is a client property and is now advertised via a canPresentTerminalAuth option (default true), with a wire-level regression test asserting the initialize payload - detect auth_required in the normal flow (turn failure + draft prep): publish an acp.auth.required event and replace the raw error block with an actionable sign-in message - add DaemonAcpAuthRuntime: agent-method authenticate on the warm connection (30s timeout, single-flight per agent), terminal-method login runs the verified launch spec plus the method args/env in a Bun.Terminal argv-style (no shell), streams chunked output (64KB chunks, 256KB cap), releases cached handles so the retry reconnects - add providers.startAcpAuth / writeAcpAuthInput / cancelAcpAuth routes and providers.acpAuth.changed events - add AcpAuthDialog (method selection, embedded xterm for the login TUI, retry) reachable from AcpDiagnostics and from a chat banner SDD: docs/features/acp-terminal-auth
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
React Doctor found 3 new issues in 3 files · 3 warnings · score 89 / 100 (Great) · 4 fixed · vs 3 warnings
Reviewed by React Doctor for commit |
Confidence Score: 1/5This PR is not safe to merge until the authentication lifecycle races, terminal startup cleanup, output buffering, and event identity filtering are corrected. Agent-method completion is deterministically overwritten by the caller, terminal startup failures can permanently block retries, initial PTY prompts can be dropped before xterm mounts, and unrelated authentication completion can dismiss an active chat prompt. Files Needing Attention: apps/daemon/src/host/acpAuthRuntime.ts, packages/ui/settings/components/AcpAuthDialog.tsx, packages/ui/src/components/chat/AcpAuthBanner.tsx
|
| Filename | Overview |
|---|---|
| apps/daemon/src/host/acpAuthRuntime.ts | Introduces agent and terminal authentication execution, but terminal startup errors can strand an active run. |
| apps/daemon/src/host/acp-provider-execution.ts | Wires the authentication runtime into the daemon and surfaces authentication-required session failures. |
| packages/acp-runtime/src/process/acpProcessManager.ts | Correctly makes terminal-auth advertisement a client capability with an explicit opt-out. |
| packages/ui/settings/components/AcpAuthDialog.tsx | Adds the authentication dialog, but agent completion is overwritten and early terminal output is lost. |
| packages/ui/src/components/chat/AcpAuthBanner.tsx | Adds chat authentication surfacing, but clears prompts on unrelated global authentication completions. |
| packages/shared-contracts/src/routes/providers.routes.ts | Defines typed start, input, and cancellation routes for ACP authentication. |
| packages/shared-contracts/src/events/providers.events.ts | Defines authentication-required and authentication-state event payloads. |
Comments Outside Diff (3)
-
packages/ui/settings/components/AcpAuthDialog.tsx, line 1505-1508 (link)Agent auth stays running
For agent-managed methods, the daemon publishes
ready,error, orcancelledbeforestartAcpAuthreturns. The event listener applies that result while this call is awaiting, but these lines then overwrite it withrunning. Because no later event is emitted, the dialog remains stuck on “Waiting” even though authentication has already finished or failed.Prompt To Fix With AI
This is a comment left during a code review. Path: packages/ui/settings/components/AcpAuthDialog.tsx Line: 1505-1508 Comment: **Agent auth stays running** For agent-managed methods, the daemon publishes `ready`, `error`, or `cancelled` before `startAcpAuth` returns. The event listener applies that result while this call is awaiting, but these lines then overwrite it with `running`. Because no later event is emitted, the dialog remains stuck on “Waiting” even though authentication has already finished or failed. --- For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
-
packages/ui/settings/components/AcpAuthDialog.tsx, line 1465-1467 (link)The daemon starts the login process before
startAcpAuthreturns, but xterm is created only after that response updates and re-renders the dialog. Output received during this interval is silently discarded becausextermRef.currentis still null and no replay buffer exists. An agent that immediately prints a device URL, code, or prompt can therefore hide information the user needs to complete authentication.Prompt To Fix With AI
This is a comment left during a code review. Path: packages/ui/settings/components/AcpAuthDialog.tsx Line: 1465-1467 Comment: **Startup output is lost** The daemon starts the login process before `startAcpAuth` returns, but xterm is created only after that response updates and re-renders the dialog. Output received during this interval is silently discarded because `xtermRef.current` is still null and no replay buffer exists. An agent that immediately prints a device URL, code, or prompt can therefore hide information the user needs to complete authentication. --- For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
-
packages/ui/src/components/chat/AcpAuthBanner.tsx, line 1899-1903 (link)This listener clears the current conversation’s prompt whenever any ACP authentication reports
ready, without checking the event’sagentIdorworkdir. For example, completing authentication for another agent from Settings will hide this chat banner even though the conversation’s own agent remains unauthenticated.Prompt To Fix With AI
This is a comment left during a code review. Path: packages/ui/src/components/chat/AcpAuthBanner.tsx Line: 1899-1903 Comment: **Unrelated auth clears banner** This listener clears the current conversation’s prompt whenever any ACP authentication reports `ready`, without checking the event’s `agentId` or `workdir`. For example, completing authentication for another agent from Settings will hide this chat banner even though the conversation’s own agent remains unauthenticated. --- For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Prompt To Fix All With AI
### Issue 1
packages/ui/settings/components/AcpAuthDialog.tsx:1505-1508
**Agent auth stays running**
For agent-managed methods, the daemon publishes `ready`, `error`, or `cancelled` before `startAcpAuth` returns. The event listener applies that result while this call is awaiting, but these lines then overwrite it with `running`. Because no later event is emitted, the dialog remains stuck on “Waiting” even though authentication has already finished or failed.
### Issue 2
apps/daemon/src/host/acpAuthRuntime.ts:231-246
**Startup failure blocks retries**
The run is marked active before the PTY and process are created. If terminal construction or spawning throws—for example because the command is missing, the working directory is invalid, or the terminal is unavailable—the run remains permanently marked `running`. Every later authentication attempt for that agent is then rejected as already running, and no error event is published.
### Issue 3
packages/ui/settings/components/AcpAuthDialog.tsx:1465-1467
**Startup output is lost**
The daemon starts the login process before `startAcpAuth` returns, but xterm is created only after that response updates and re-renders the dialog. Output received during this interval is silently discarded because `xtermRef.current` is still null and no replay buffer exists. An agent that immediately prints a device URL, code, or prompt can therefore hide information the user needs to complete authentication.
### Issue 4
packages/ui/src/components/chat/AcpAuthBanner.tsx:1899-1903
**Unrelated auth clears banner**
This listener clears the current conversation’s prompt whenever any ACP authentication reports `ready`, without checking the event’s `agentId` or `workdir`. For example, completing authentication for another agent from Settings will hide this chat banner even though the conversation’s own agent remains unauthenticated.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "feat(acp): terminal authentication for a..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+231
to
+246
| this.activeByAgent.set(input.agentId, run); | ||
| this.runsById.set(run.runId!, run); | ||
| this.publish({ run, state: "running" }); | ||
|
|
||
| const env = buildAuthEnv(spec.env ?? {}, methodEnv); | ||
|
|
||
| const terminal = this.deps.ptyFactory({ | ||
| cols: 80, | ||
| rows: 24, | ||
| onData: (data) => this.handleOutput(run, data), | ||
| }); | ||
| const proc = this.deps.spawnPty(argv, { | ||
| cwd: input.workdir || process.cwd(), | ||
| env, | ||
| terminal, | ||
| }); |
There was a problem hiding this comment.
The run is marked active before the PTY and process are created. If terminal construction or spawning throws—for example because the command is missing, the working directory is invalid, or the terminal is unavailable—the run remains permanently marked running. Every later authentication attempt for that agent is then rejected as already running, and no error event is published.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/daemon/src/host/acpAuthRuntime.ts
Line: 231-246
Comment:
**Startup failure blocks retries**
The run is marked active before the PTY and process are created. If terminal construction or spawning throws—for example because the command is missing, the working directory is invalid, or the terminal is unavailable—the run remains permanently marked `running`. Every later authentication attempt for that agent is then rejected as already running, and no error event is published.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
🟡 Changes recommended
The critical single-flight race and unresolved launch and UI-flow issues must be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds ACP agent and terminal authentication across the daemon runtime, shared contracts, and chat/settings UI.
Changes:
- Advertises terminal-auth capability and surfaces authentication-required failures.
- Adds daemon authentication routes, PTY execution, cancellation, and events.
- Adds embedded authentication UI, tests, and documentation.
File summaries
| File | Reviewed changes and findings |
|---|---|
packages/ui/src/pages/ChatPage.tsx |
Mounts the ACP authentication banner. |
packages/ui/src/components/chat/AcpAuthBanner.tsx |
Surfaces authentication prompts. Moderate (2 votes): ready events must only clear prompts for the matching agent. Moderate (2 votes): successful authentication does not retry the failed request as promised. Moderate (1 vote): one-shot state loses prompts across navigation, reloads, or background conversations. |
packages/ui/settings/components/AcpDiagnostics.tsx |
Opens authentication flows. Moderate (1 vote): passes the authentication method name as the agent name, producing an incorrect dialog title. |
packages/ui/settings/components/AcpAuthDialog.tsx |
Implements method selection and embedded terminal UI. Moderate (2 votes): PTY output arriving before xterm mounts is permanently dropped. Nit (1 vote): the settings renderer does not import xterm’s required stylesheet. |
packages/ui/api/ProviderClient.ts |
Adds authentication routes and event subscriptions. |
packages/shared/src/types/presenters/legacy.presenters.d.ts |
Extends authentication method metadata. |
packages/shared-contracts/src/routes/providers.routes.ts |
Defines ACP authentication route contracts. |
packages/shared-contracts/src/routes.ts |
Registers authentication routes. |
packages/shared-contracts/src/events/providers.events.ts |
Defines authentication events. |
packages/shared-contracts/src/events.ts |
Registers authentication events. |
packages/acp-runtime/src/process/acpProcessManager.ts |
Corrects terminal capability advertisement. |
packages/acp-runtime/src/debug/runAcpDebugAction.ts |
Preserves terminal method arguments and environment. |
docs/features/acp-terminal-auth/tasks.md |
Records implementation tasks and verification. |
docs/features/acp-terminal-auth/spec.md |
Specifies terminal-authentication behavior. |
docs/features/acp-terminal-auth/plan.md |
Documents the implementation plan. |
apps/desktop/test/main/presenter/llmProviderPresenter/acp/acpProcessManagerCapabilities.test.ts |
Tests wire-level capability advertisement. |
apps/daemon/test/acpAuthRuntime.test.ts |
Tests daemon authentication flows. Nit (2 votes): emitData is ineffective, so output streaming, chunking, and truncation are not exercised. |
apps/daemon/src/terminal/daemonTerminalRuntime.ts |
Exposes PTY constructor resolution. |
apps/daemon/src/host/acpAuthRuntime.ts |
Implements agent and terminal authentication. Critical (2 votes): concurrent starts can pass the single-flight check before an active run is reserved, launching duplicate authentication flows. |
apps/daemon/src/host/acp-provider-execution.ts |
Integrates authentication and required-auth handling. Moderate (1 vote): terminal authentication bypasses the managed launch pipeline, so commands unavailable on the daemon’s PATH can fail. |
apps/daemon/src/dispatch/daemonDispatcher.ts |
Dispatches authentication routes. |
Review details
Suppressed comments (7)
apps/daemon/src/host/acpAuthRuntime.ts:197
startAgentFlowdoes not return until authentication has already reachedready,error, or the 30 s timeout. The daemon publishes that terminal event before the route response is sent, butAcpAuthDialog.startMethodsetsflowStateback to"running"after awaiting this route, leaving agent-method flows stuck in the progress state. Start the authenticate work in the background and return the start result immediately (as the terminal path does), or include the terminal state in the response and stop overwriting it in the UI.
await Promise.race([authenticate, timeout]);
apps/daemon/src/host/acpAuthRuntime.ts:247
- Interactive input is wired to the spawned process, but Bun's terminal-mode subprocess does not expose
write; input must be written through theBun.Terminalobject. The existing terminal runtime does this viasession.terminal.write(data)(apps/daemon/src/terminal/daemonTerminalRuntime.ts:241-245). As written, the first keystroke calls an undefined function in production.
run.write = (data) => proc.write(data);
apps/daemon/src/host/acpAuthRuntime.ts:246
- The run is registered as active before PTY construction/spawn, but synchronous failures here are not caught. If
Bun.Terminalis unavailable, the cwd is invalid, or spawn throws, the route rejects while the agent remains permanentlyrunning; later starts are rejected and cancel cannot finish a process that never started. Catch setup failures, remove both map entries, mark/publisherror, and close any partially created terminal.
const proc = this.deps.spawnPty(argv, {
cwd: input.workdir || process.cwd(),
env,
terminal,
});
apps/daemon/src/host/acpAuthRuntime.ts:143
- Calling
kill()without a signal uses SIGTERM on Unix, which interactive PTY children can ignore. This repository's terminal runtime explicitly uses SIGKILL for that reason (apps/daemon/src/terminal/daemonTerminalRuntime.ts:37-45); otherwise Cancel can leave this auth run active indefinitely.
run.kill?.();
apps/daemon/src/host/acpAuthRuntime.ts:270
- This does not chunk a large PTY callback; it sends only the first 64 KB and silently discards the remainder whenever the callback is still under the 256 KB total cap. Iterate over the entire buffer in 64 KB slices so terminal prompts/output are not lost.
const chunk = data.byteLength > OUTPUT_CHUNK_BYTES ? data.subarray(0, OUTPUT_CHUNK_BYTES) : data;
this.publish({ run, state: "running", output: new TextDecoder().decode(chunk) });
apps/daemon/src/host/acpAuthRuntime.ts:241
- The created
Bun.Terminalis not retained or closed when the subprocess exits. The established terminal runtime explicitly callsterminal.close()on exit/shutdown (apps/daemon/src/terminal/daemonTerminalRuntime.ts:372-405); repeated auth attempts here can otherwise retain PTY resources. Store the terminal on the run and close it in every finish/setup-failure path.
const terminal = this.deps.ptyFactory({
cols: 80,
rows: 24,
onData: (data) => this.handleOutput(run, data),
});
packages/ui/settings/components/AcpAuthDialog.tsx:3
- This settings entry imports xterm itself but not its stylesheet. The only
@xterm/xterm/css/xterm.cssimport is insrc/components/sidepanel/TerminalPanel.tsx, which is not part of the settings renderer, so the auth terminal opened fromAcpDiagnosticswill render without xterm's required layout styles. Import the stylesheet alongsideTerminal(or from a shared stylesheet loaded by every renderer).
import { Terminal } from "@xterm/xterm";
- Files reviewed: 20/21 changed files
- Comments generated: 8
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+105
to
+109
| if (this.isActive(input.agentId)) { | ||
| throw new Error(`An authentication flow is already running for agent ${input.agentId}`); | ||
| } | ||
|
|
||
| const processManager = await this.deps.getProcessManager(); |
Comment on lines
+137
to
+140
| resolveLaunchSpec: async (agentId, workdir) => { | ||
| const spec = await this.configPresenter.resolveAcpLaunchSpec(agentId, workdir); | ||
| return { command: spec.command, args: spec.args ?? [], env: spec.env ?? null }; | ||
| }, |
Comment on lines
+82
to
+83
| if (payload.output) { | ||
| xtermRef.current?.write(payload.output); |
| <AcpAuthDialog | ||
| open | ||
| agentId={agentId} | ||
| agentName={terminalAuth.name} |
Comment on lines
+14
to
+18
| const [authPrompt, setAuthPrompt] = useState<{ agentId: string; workdir?: string | null } | null>(null); | ||
| const [dialogOpen, setDialogOpen] = useState(false); | ||
|
|
||
| useEffect(() => { | ||
| const offRequired = providerClient.onAcpAuthRequired((payload) => { |
Comment on lines
+24
to
+28
| const offChanged = providerClient.onAcpAuthChanged((payload) => { | ||
| if (payload.state === "ready") { | ||
| setAuthPrompt(null); | ||
| } | ||
| }); |
| agentId={authPrompt.agentId} | ||
| agentName={authPrompt.agentId} | ||
| workdir={authPrompt.workdir} | ||
| onAuthenticated={() => setAuthPrompt(null)} |
Comment on lines
+89
to
+91
| emitData: (text: string) => { | ||
| const onData = (deps.ptyFactory as any).mock?.calls?.[0]?.[0]?.onData; | ||
| void onData; |
Address reviewer findings on the terminal authentication PR: - reserve the agent synchronously at start so concurrent starts cannot double-launch; setup failures release the reservation and publish an error so the agent stays retryable - terminal-auth launch resolves through the managed toolchain service (npx rewrites + bin-dir PATH prepend) instead of the raw configured command, mirroring the normal session launch pipeline - keystrokes go through the Bun.Terminal object (the terminal-mode subprocess does not expose write); cancel force-kills with SIGKILL on unix; the PTY is closed when the run finishes - chunk oversized PTY buffers instead of dropping the remainder - agent-method authenticate runs in the background: the route returns immediately and the UI follows event state transitions - AcpAuthDialog: import the xterm stylesheet, buffer PTY output that arrives before the terminal mounts, pass the agent name from AcpDiagnostics - AcpAuthBanner: only the same agent's success clears the prompt; copy states that resending the message after sign-in is expected
React Doctor flags useCallback as unnecessary under the React Compiler; a plain function caches identically.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| * daemon; terminal methods run the agent's login TUI in an embedded PTY | ||
| * (xterm). Env-var methods render setup instructions. | ||
| */ | ||
| export default function AcpAuthDialog({ |
There was a problem hiding this comment.
React Doctor · react-doctor/no-high-complexity-react-function (warning)
AcpAuthDialog has cyclomatic complexity 18, cognitive complexity 19, and maximum nesting depth 2, so its React logic is hard to understand and change. Extract independent branches into components or hooks.
Fix → Extract independent render branches and state logic into focused components or hooks until the control flow is easy to follow.
dvaJi
added a commit
that referenced
this pull request
Sep 9, 2026
* fix(acp): settle sessions before agent removal Uninstalling a disabled or uninstalled ACP registry agent was a permanent dead-end: acp_sessions binding rows were never deleted by any production path, sessions bound to a disabled agent threw during transfer assessment (agent type lookup filtered to enabled+installed), and bulk delete/move routes performed raw row operations with no settlement - running generations were not cancelled and queued inputs leaked. - add sessionSettlement: discard queue-mode pending inputs (steer kept, cancel suppresses the drain), cancel active turns with a bounded wait for status to leave generating, purge ACP bindings best-effort - wire settlement into sessions.delete, sessions.deleteAgentSessions, sessions.moveAgentSessions, sessions.moveToAgent - add config.getAgentType route: state-agnostic type lookup so disabled or uninstalled registry agents stay assessable - add purgeAcpSessionData on the ACP execution port so the uninstall guard becomes accurate once conversations are gone - make daemon move handlers target-aware: Argos targets receive the target agent's default model instead of a hardcoded acp label that broke the next send - AcpSettings uninstall now offers move/delete of conversations via the shared AgentTransferDialog instead of failing the guard SDD: docs/issues/acp-agent-removal-settlement * feat(toolchains): managed installs for node, uv, ripgrep The daemon resolved external runtimes through identity no-op ports, so the headless deployment could only run npx/uvx agents and uvx MCP servers when Node/uv happened to be on PATH, with no verification and no toolchain UX. Modeled on ThinkInAIXYZ/deepchat#2193, re-designed for Argos' daemon-first architecture. - add a daemon-owned ToolchainService: explicit persisted sources (custom / unconfigured) over derived ones (managed / bundled / system), with precedence, a warm sync cache for sync host seams, and timestamped quarantine of corrupt state - managed installs download pinned Node (v24.18.0) and uv (0.9.18) archives, verify SHA-256, extract to staging, and activate atomically via rename; the previous tree rotates to .prev and a failed or cancelled install leaves it active - wire daemon consumers through the service: ACP launch resolves npx/npm/node/uvx (npx becomes node npx-cli.js via a new optional resolveCommandWithArgs host seam) and prepends resolved bin dirs to the spawn PATH; MCP stdio commands rewrite through the warm cache - probe bundled seeds for the headless daemon (execDir/../runtime, execDir/runtime, cwd/runtime, dataDir/runtime) - add a Toolchains settings page: per-tool source, path, version, install/repair, cancel, revert, custom path - fix bundled-runtime doc drift (no bundled Bun/rtk; seeds are uv + ripgrep) SDD: docs/features/managed-toolchains * fix(toolchains): review hardening for managed installs Address reviewer findings on the managed toolchains PR: - await toolchain warmup before MCP servers start so npx/uvx rewriting never falls back to PATH mid-startup - refresh the warm sync cache after a successful install; emit the activating phase before the atomic rename instead of after completion - revert now removes the managed tree so managed installs can fall back to bundled/system; the UI shows Revert for managed sources - uvx fallback without a sibling binary becomes "uv tool run" instead of passing uvx arguments to bare uv (both sync and async rewrites) - corrupt toolchain state resets to a valid serialized empty state instead of an empty string that re-corrupted on every load - add darwin-x64 to the uv catalog (sha256 captured from the release) - stream downloads to disk while hashing instead of buffering the whole archive; clean the staging tree when extraction is cancelled - expand nvm version directories for system Node detection - Toolchains settings: explicit load-error state with retry; drop manual memoization flagged by react-compiler lint SDD: docs/features/managed-toolchains (review-hardening section) * feat(acp): terminal authentication for agent login (#99) * feat(acp): terminal authentication for agent login Agents that require login (MiniMax Code and similar) advertise authMethods only when the client declares clientCapabilities.auth.terminal. Our advertisement was dead code: enableTerminalAuth was computed from the initialize response's authMethods inside the initialize request itself, so it was always false and terminal-auth agents could never offer their login flow. In the normal session flow, auth_required failures surfaced as raw JSON-RPC error text (or were swallowed at draft preparation), and no execution path existed for terminal methods. - fix capability advertisement: auth.terminal is a client property and is now advertised via a canPresentTerminalAuth option (default true), with a wire-level regression test asserting the initialize payload - detect auth_required in the normal flow (turn failure + draft prep): publish an acp.auth.required event and replace the raw error block with an actionable sign-in message - add DaemonAcpAuthRuntime: agent-method authenticate on the warm connection (30s timeout, single-flight per agent), terminal-method login runs the verified launch spec plus the method args/env in a Bun.Terminal argv-style (no shell), streams chunked output (64KB chunks, 256KB cap), releases cached handles so the retry reconnects - add providers.startAcpAuth / writeAcpAuthInput / cancelAcpAuth routes and providers.acpAuth.changed events - add AcpAuthDialog (method selection, embedded xterm for the login TUI, retry) reachable from AcpDiagnostics and from a chat banner SDD: docs/features/acp-terminal-auth * fix(acp): harden terminal auth lifecycle Address reviewer findings on the terminal authentication PR: - reserve the agent synchronously at start so concurrent starts cannot double-launch; setup failures release the reservation and publish an error so the agent stays retryable - terminal-auth launch resolves through the managed toolchain service (npx rewrites + bin-dir PATH prepend) instead of the raw configured command, mirroring the normal session launch pipeline - keystrokes go through the Bun.Terminal object (the terminal-mode subprocess does not expose write); cancel force-kills with SIGKILL on unix; the PTY is closed when the run finishes - chunk oversized PTY buffers instead of dropping the remainder - agent-method authenticate runs in the background: the route returns immediately and the UI follows event state transitions - AcpAuthDialog: import the xterm stylesheet, buffer PTY output that arrives before the terminal mounts, pass the agent name from AcpDiagnostics - AcpAuthBanner: only the same agent's success clears the prompt; copy states that resending the message after sign-in is expected * style(ui): drop manual memoization in AcpAuthDialog React Doctor flags useCallback as unnecessary under the React Compiler; a plain function caches identically. * fix(acp): cancel pending auth starts and bind keystrokes to agents Address reviewer findings on the reconciliation: - AcpAuthDialog: closing while a start request is still connecting now cancels the flow (a pending start previously escaped cancellation and could launch an invisible terminal login later); terminal events that arrive before the start response are preserved via a functional flow-state update instead of being clobbered back to running - terminal keystroke injection is bound to the owning agent: the write route takes agentId and the runtime rejects mismatches, so a runId observed on the broadcast event stream cannot be used to type into another client's login session - document the single-user trust model in the terminal-auth spec
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
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
Supports the ACP v1 terminal authentication flow so agents that require login (MiniMax Code
mcode acpand similar) can complete sign-in from Argos. Modeled on ThinkInAIXYZ/deepchat#2144 (fixed by #2195), re-implemented for our daemon-owned ACP runtime. SDD docs:docs/features/acp-terminal-auth/.Stacked on #98 (which is stacked on #97) — GitHub retargets each to
masteras the stack merges.The bug at the center
enableTerminalAuthwas computed fromhandleSeed.authMethodswhile building theinitializerequest — butauthMethodsonly arrive in that request's response. It was alwaysundefined, soclientCapabilities.auth.terminalwas never advertised, ever, and terminal-auth agents could not offer login. The fix treatsauth.terminalas what it is — a client capability ("this client can present a terminal login") — advertised via acanPresentTerminalAuthoption (defaulttrue; Argos has a PTY and an embeddable terminal on every surface). A wire-level test now asserts the initialize payload, so this cannot silently regress.What was added
session/new/ prompt failures matchingauth_required(-32042/-32800+ message heuristics) now publishacp.auth.required(sessionId, agentId, workdir) and render an actionable "this agent requires sign-in" error instead of raw JSON-RPC text. Draft preparation failures — previously swallowed — now publish the same event.DaemonAcpAuthRuntime(daemon):authenticateon the warm connection, 30 s timeout, single-flight per agent;args/envargv-style in aBun.Terminal(no shell), streams chunked output (64 KB chunks / 256 KB cap), thenrelease()s cached handles so the retry re-initializes with fresh credentials;providers.startAcpAuth/providers.writeAcpAuthInput/providers.cancelAcpAuth+providers.acpAuth.changedevents.AcpAuthDialog: method selection, agent-method progress, embedded xterm for the interactive login TUI (keyboard input forwarded to the PTY), retry on success. Reachable fromAcpDiagnostics(terminal methods previously hit the debugauthenticateRPC, which is invalid for them) and from a chat banner.UI
Testing
canPresentTerminalAuth: false); fulltest:maingreen.bun testgreen.bun run typecheck(desktop + UI + daemon),bun run lint(all guards, 419 routes),bun run formatclean.