chore: reconcile toolchains and terminal auth onto master - #101
Merged
Merged
Conversation
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
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
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 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.
PRs #98 and #99 were squash-merged into their stacked base branches instead of master, leaving the managed toolchains and terminal auth features stranded off-mainline. This merge lands both onto master (including all review-hardening from the #97->#98->#99 stack) and keeps master's fail-closed settlement semantics where the branches disagreed (settlement files taken from master; toolchain routes, auth runtime, and launch-pipeline integration added on top).
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 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 88 / 100 (Great) · 1 fixed · vs 3 warnings
Reviewed by React Doctor for commit |
Confidence Score: 1/5The PR is not safe to merge until terminal authentication is isolated between clients and the dialog correctly handles pending closure and early completion events. The changed authentication flow permits cross-client PTY input injection and contains two concrete lifecycle races that can leave completed or cancelled authentication flows stuck or running invisibly. Files Needing Attention: packages/ui/settings/components/AcpAuthDialog.tsx, apps/daemon/src/host/acpAuthRuntime.ts
|
| Filename | Overview |
|---|---|
| apps/daemon/src/host/acpAuthRuntime.ts | Implements agent and PTY authentication lifecycles, but terminal input authorization relies on a globally disclosed run identifier. |
| packages/ui/settings/components/AcpAuthDialog.tsx | Adds the shared ACP sign-in dialog, with pending-close and response/event ordering races that can leave authentication running invisibly or stuck onscreen. |
| apps/daemon/src/host/toolchains/service.ts | Adds centralized toolchain resolution, source persistence, command rewriting, installation jobs, and synchronous cache integration. |
| apps/daemon/src/host/toolchains/install.ts | Adds streamed, checksum-verified installation with staged extraction, atomic activation, and rollback. |
| apps/daemon/src/dispatch/daemonDispatcher.ts | Registers toolchain and ACP authentication routes; auth input ultimately reaches a run without client ownership enforcement. |
| packages/acp-runtime/src/process/acpProcessManager.ts | Adds host command-and-argument rewriting and correctly advertises terminal-auth capability independently of the initialize response. |
Prompt To Fix All With AI
### Issue 1
packages/ui/settings/components/AcpAuthDialog.tsx:148-151
**Pending authentication escapes cancellation**
If the dialog closes while `startAcpAuth` is still setting up the connection, `flowState` remains `"select"`, so this condition does not request cancellation. Connection setup can take minutes, after which the daemon may launch an invisible terminal login process that blocks another authentication attempt for the same agent. Cancellation must also cover a pending start, not only a flow already marked `"running"`.
### Issue 2
packages/ui/settings/components/AcpAuthDialog.tsx:137-138
**Completion state gets overwritten**
With a warm connection and an immediate agent-auth response, the one-time `"ready"` or `"error"` event can arrive before `startAcpAuth` returns. The event handler records completion, but these unconditional assignments then change the dialog back to `"running"`. Because the daemon does not replay that completion event, the dialog remains stuck even though authentication has finished.
### Issue 3
apps/daemon/src/host/acpAuthRuntime.ts:144-150
**Auth runs lack ownership**
A live terminal-auth `runId` is broadcast to every client subscribed to `providers.acpAuth.changed`, while this method accepts that identifier as the only authority needed to write arbitrary data to the PTY. Another authenticated daemon client can therefore observe the identifier and inject keystrokes into a different client's login session. Runs must be bound to the client that created them, or write and cancel operations must otherwise enforce ownership.
**How this was verified:** The globally broadcast event exposes each live run identifier, and the write path forwards caller-controlled data to the matching PTY without checking client ownership.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "merge: reconcile toolchains and terminal..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+148
to
+151
| const handleClose = (nextOpen: boolean) => { | ||
| if (!nextOpen && flowState === "running" && agentId) { | ||
| void providerClient.cancelAcpAuth(agentId).catch(() => undefined); | ||
| } |
There was a problem hiding this comment.
If the dialog closes while startAcpAuth is still setting up the connection, flowState remains "select", so this condition does not request cancellation. Connection setup can take minutes, after which the daemon may launch an invisible terminal login process that blocks another authentication attempt for the same agent. Cancellation must also cover a pending start, not only a flow already marked "running".
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/settings/components/AcpAuthDialog.tsx
Line: 148-151
Comment:
**Pending authentication escapes cancellation**
If the dialog closes while `startAcpAuth` is still setting up the connection, `flowState` remains `"select"`, so this condition does not request cancellation. Connection setup can take minutes, after which the daemon may launch an invisible terminal login process that blocks another authentication attempt for the same agent. Cancellation must also cover a pending start, not only a flow already marked `"running"`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+137
to
+138
| setRunId(result.runId); | ||
| setFlowState("running"); |
There was a problem hiding this comment.
With a warm connection and an immediate agent-auth response, the one-time "ready" or "error" event can arrive before startAcpAuth returns. The event handler records completion, but these unconditional assignments then change the dialog back to "running". Because the daemon does not replay that completion event, the dialog remains stuck even though authentication has finished.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/settings/components/AcpAuthDialog.tsx
Line: 137-138
Comment:
**Completion state gets overwritten**
With a warm connection and an immediate agent-auth response, the one-time `"ready"` or `"error"` event can arrive before `startAcpAuth` returns. The event handler records completion, but these unconditional assignments then change the dialog back to `"running"`. Because the daemon does not replay that completion event, the dialog remains stuck even though authentication has finished.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+144
to
+150
| write(runId: string, data: string): void { | ||
| const run = this.runsById.get(runId); | ||
| if (!run || run.state !== "running" || !run.write) { | ||
| throw new Error(`No active terminal auth run: ${runId}`); | ||
| } | ||
| run.write(data); | ||
| } |
There was a problem hiding this comment.
A live terminal-auth runId is broadcast to every client subscribed to providers.acpAuth.changed, while this method accepts that identifier as the only authority needed to write arbitrary data to the PTY. Another authenticated daemon client can therefore observe the identifier and inject keystrokes into a different client's login session. Runs must be bound to the client that created them, or write and cancel operations must otherwise enforce ownership.
How this was verified: The globally broadcast event exposes each live run identifier, and the write path forwards caller-controlled data to the matching PTY without checking client ownership.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/daemon/src/host/acpAuthRuntime.ts
Line: 144-150
Comment:
**Auth runs lack ownership**
A live terminal-auth `runId` is broadcast to every client subscribed to `providers.acpAuth.changed`, while this method accepts that identifier as the only authority needed to write arbitrary data to the PTY. Another authenticated daemon client can therefore observe the identifier and inject keystrokes into a different client's login session. Runs must be bound to the client that created them, or write and cancel operations must otherwise enforce ownership.
**How this was verified:** The globally broadcast event exposes each live run identifier, and the write path forwards caller-controlled data to the matching PTY without checking client ownership.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.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
![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.
| ); | ||
| } | ||
|
|
||
| function ToolchainCard({ |
There was a problem hiding this comment.
React Doctor · react-doctor/no-high-complexity-react-function (warning)
ToolchainCard has cyclomatic complexity 18, cognitive complexity 24, and maximum nesting depth 3, 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 13, 2026
Pre-existing master breakage from #101 surfaced by this PR's CI: - state.quarantine used Date.now()-only names; two corruptions within the same millisecond collided on fast runners (second rename replaced the first), so the repeated-corruption test saw one file instead of two. Add a UUID suffix so quarantines are unique. - toolchainsService checksum test wrote bin/node on Linux without creating the bin directory (nodeBin() nests on non-Windows); create the deepest directory instead of only the version dir.
dvaJi
added a commit
that referenced
this pull request
Sep 14, 2026
* feat(ui): thread sidebar experiment polish Fixes (docs/issues/thread-sidebar-fixes): - Load older sessions on scroll (the experiment missed store pagination) - Keep workingSince elapsed across restarts via first-observation diffing - Prune settled/snoozed/workingSince entries on delete and after full load - Replace window.confirm with the shared delete dialog; surface rename/ delete failures inline; renames/deletes go through the session store - Keep the rename draft in sync with externally updated titles Polish (docs/features/thread-sidebar-polish): - Clearable search, no-results state, search bypasses collapsed shelves - Keyboard nav only targets rendered rows; hover action overlays the time slot (no layout shift) and is keyboard reachable - The open session is never hidden by snooze; snoozed shelf expansion persists; pinned+settled rows render settled state - Collapsed rail (both modes) with attention indicator; Alt/Cmd+1..9 shortcut badges work in experiment mode - Drop the per-second store tick; per-field selectors, memoized rows, coarse clock buckets; extract ThreadSection; storage-event sync * fix(ui): address thread sidebar review feedback Review findings from CodeRabbit and Greptile on #102: - Exclude collapsed Settled rows from keyboard navigation (flatResults now gates on settledExpanded) - Keep loading pages while the list is shorter than the viewport so short first pages can still reach older sessions - Derive experiment shortcut targets and badge numbering from the rows ThreadSidebarList actually renders (onVisibleRowsChange): search-aware and always in sync with the live clock, replacing the mount-time partition reconstruction - Filter rail attention by sidebar visibility rules (drafts/subagents never light the rail) - Gate the lifecycle sweep on hasLoadedInitialPage (hasMore defaults to false and upserts can land before the initial page) - Detect settled records by key presence: legacy v1 entries (timestamp 0) render as Settled with the updatedAt fallback, matching the row rendering instead of contradicting it Docs: shortcut AC updated to rendered-row targets with the original sidebar's ten slots (1-9 plus 0). * fix(daemon): stabilize toolchain tests on linux runners Pre-existing master breakage from #101 surfaced by this PR's CI: - state.quarantine used Date.now()-only names; two corruptions within the same millisecond collided on fast runners (second rename replaced the first), so the repeated-corruption test saw one file instead of two. Add a UUID suffix so quarantines are unique. - toolchainsService checksum test wrote bin/node on Linux without creating the bin directory (nodeBin() nests on non-Windows); create the deepest directory instead of only the version dir.
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.
Context
PRs #97 → #98 → #99 were a stacked series. During merge, they were combined out of order: #98 and #99 were squash-merged into their base branches (
fix/acp-agent-removal-settlement, thenfeat/managed-toolchains) instead of master, so their content never reached mainline. Master ended up with only #97 (settlement) and #100 (hygiene sweep).Nothing was lost — the complete toolchains + terminal-auth features (with all review fixes) live on
feat/managed-toolchains@de324790. This PR reconciles them onto master.What it lands
uv tool runfallback, managed revert, valid state reset, darwin-x64 catalog, streaming downloads, nvm enumeration, settings error/retry.5f6f3a79's settlement review hardening content that was missing from the stacked branch lineage.Verification
bun run testgreen (desktop + daemon),bun run typecheckclean (desktop + UI + daemon),bun run lintclean (all architecture guards, 419 routes), format clean.