feat: add controlled runtime install command - #2343
Conversation
baixiaohang
left a comment
baixiaohang
left a comment
There was a problem hiding this comment.
Recommendation: request changes
- Rationale: The controlled install contract is carefully bounded, but its result frames are not wired through the server WebSocket dispatcher, and the daemon reuse path bypasses the service logging/redaction boundary.
Risk level: A
- Path baseline: includes
apps/cli/**andpackages/client/**-> A - Semantic lift: crosses the HTTP, multi-replica notifier, WebSocket, and host-level runtime mutation boundary; remains A
PR summary
- Author / repo: Gandy2025 / agent-team-foundation/first-tree
- Problem: A future Web action needs to install a missing Codex or Claude Code runtime on the user's selected Computer without accepting arbitrary commands or packages.
- Approach: The PR adds an allowlisted synchronous HTTP-to-daemon command/result flow, exact Computer-owner and liveness checks, cross-replica notifier fan-out, one global daemon-side install guard, result redaction, and capability re-probing. It intentionally adds no UI or durable job state.
- Impacted modules: shared wire schemas, Client connection/runtime, daemon CLI runtime, Server client API and WebSocket routing, notifier RPC, focused tests
Review findings
❌ 1. runtime-install:result never reaches the newly added client-frame handler. The top-level dispatcher in packages/server/src/api/agent/ws-client/connection.ts:78 routes only client:register, heartbeat, and provider-models:result to handleClientFrame; it neither imports nor matches RUNTIME_INSTALL_RESULT_TYPE. Therefore the branch added at client-frames.ts:203 is unreachable in the real socket path: the daemon sends accepted/progress/terminal frames, the Server silently ignores them, and every install request times out. Please wire the type into this dispatcher and add a test that exercises the attached WebSocket path rather than resolving the HTTP waiter or publishing notifier results directly. [R5]
❌ 2. The remotely triggered daemon path still streams raw npm stderr through the global CLI Print layer. The new calls at apps/cli/src/commands/daemon/start.ts:425 reuse installClaudeRuntime / installCodexRuntime, whose stderr listeners call print.line(...) directly. In supervised mode this bypasses the logger-backed daemon output supplied by start.ts, and it happens before RuntimeInstallRunner redacts the terminal reason; registry errors containing credentials can therefore reach inherited supervisor logs unredacted even though the Cloud result is clean. Please make the installer output sink injectable (or quiet for this caller) and route/redact daemon-service diagnostics while preserving interactive CLI output. [R4]
❌ 3. This adds a durable Server-authorized host mutation workflow and capability-negotiation boundary, but the PR has no linked Context Tree update and the current Runtime Daemon node does not describe the command/result or authorization contract. The team's current maintenance rule requires CLI/workflow changes to update the owning node and durable code/tree changes to be paired and cross-linked. Please open the draft Tree PR (at minimum reconciling the Runtime Daemon boundary) and link both PRs before merge. [R5]
Action taken
- Submitted request changes on exact head
8b979c64ec491f83f76db8cb5e713e9c75d9864f.
yuezengwu
left a comment
yuezengwu
left a comment
There was a problem hiding this comment.
Independent review on exact head 8b979c64ec491f83f76db8cb5e713e9c75d9864f: do not merge yet.
The implementation goal is to add a backend-only, allowlisted HTTP → Server replica → daemon command/result flow for installing only Codex or Claude Code on the authenticated user's selected Computer. The core changes introduce shared wire schemas and two-sided capability negotiation, exact owner/liveness checks, cross-replica notifier routing, a daemon-wide single-flight installer, redacted terminal results, and a capability re-probe after success. There are no database schema changes or migrations, but this is a core wire-protocol and host-mutation boundary that needs human inspection.
I independently confirmed the current changes-requested findings:
-
Blocker — terminal/progress frames are unreachable in production.
packages/server/src/api/agent/ws-client/connection.ts:78-83does not import or dispatchRUNTIME_INSTALL_RESULT_TYPEtohandleClientFrame. The new branch inclient-frames.tstherefore never runs on a real socket, so every install request waits until timeout. Add the dispatcher case and an attached-WebSocket regression test; the current route tests bypass this path by resolving the waiter or publishing notifier results directly. -
Blocker — supervised installs bypass the daemon logging/redaction boundary.
daemon/start.tsreusesinstallClaudeRuntime/installCodexRuntime, but both helpers stream raw npm stderr through globalprint.line(...). In service mode this bypasses the injected logger-backed output and can place unredacted registry diagnostics in inherited supervisor logs before the runner sanitizes the Cloud result. Inject/disable the installer output sink for this caller and route sanitized diagnostics through the daemon logger while preserving interactive CLI output.
The current Runtime Daemon decision node also does not yet record this new Server-authorized remote install/capability boundary; the linked Context Tree update requested in the existing review is appropriate before merge.
|
The required durable Runtime Daemon contract is now captured in agent-team-foundation/first-tree-context#946. That Tree PR updates only the existing daemon owner node, passes Context Tree verification, and is linked to this source implementation. This source PR remains blocked until #946 completes provider review and merges, and until the two code findings on this exact head are resolved. |
|
Addressed the three requested blockers on exact head
Affected verification passed: 41 Server runtime-install/attached-WebSocket tests, 52 CLI runtime-install/daemon-start tests, the full CI-equivalent CLI 2/2 shard, repository typecheck, changed-file Biome, and |
baixiaohang
left a comment
baixiaohang
left a comment
There was a problem hiding this comment.
Recommendation: comment — the two code blockers are resolved; do not approve yet because the paired Tree PR is in the wrong live state/sequence.
- Rationale: The new head correctly wires the production WebSocket path and keeps raw npm stderr off supervised daemon streams, but the paired durable update is currently ready rather than draft and is described as merging before the source.
Risk level: A
- Path baseline: includes
apps/cli/**andpackages/client/**-> A - Semantic lift: cross-surface host mutation and wire-protocol boundary; remains A
PR summary
- Author / repo: Gandy2025 / agent-team-foundation/first-tree
- Problem: Enable a future Web action to install only Codex or Claude Code on the authenticated user's selected Computer without opening a general remote-command surface.
- Approach: Use an allowlisted synchronous HTTP -> replica notifier -> daemon command/result flow, negotiated capability support, exact owner/liveness checks, daemon-wide single flight, redacted terminal results, and a post-install capability re-probe.
- Impacted modules: shared schemas, Client connection/runtime, daemon CLI runtime, Server client API/WebSocket/notifier RPC, focused regression tests
Review findings
✅ 1. The previous delivery blocker is resolved on 9076e52f371ff707c704302984cdddfe90e2f849: RUNTIME_INSTALL_RESULT_TYPE now enters handleClientFrame, and the added attached-WebSocket test exercises command receipt, progress frames, terminal resolution, and the HTTP response through the real dispatcher.
✅ 2. The previous service logging blocker is resolved: both installers retain their interactive CLI Print default, while daemon-triggered installs inject a quiet stderr sink and publish only the runner's bounded/redacted terminal diagnostic through the daemon logger.
❌ 3. The paired Tree artifact exists and is cross-linked, but its live GitHub state is currently ready (isDraft=false), not draft, while the source comment says this PR remains blocked until the Tree PR merges. That reverses the current team rule: keep the Tree PR draft, merge source first, then reconcile the Tree against final source before marking it ready and merging it. Please convert first-tree-context#946 back to draft and update the sequencing language accordingly. The Tree PR body also still identifies 8b979c64... as its source head, so final reconciliation must use the actual merged source. [R5]
Action taken
- Submitted a comment review on exact source head
9076e52f371ff707c704302984cdddfe90e2f849; the existing changes-requested state remains until coordination is corrected and A-level approval is authorized.
yuezengwu
left a comment
yuezengwu
left a comment
There was a problem hiding this comment.
Re-reviewed the delta from 8b979c64ec491f83f76db8cb5e713e9c75d9864f to exact head 9076e52f371ff707c704302984cdddfe90e2f849. The two source-code blockers are resolved:
RUNTIME_INSTALL_RESULT_TYPEis now dispatched through the attached client WebSocket path, and the new regression exercises the owner-authorized HTTP request, real registered socket, progress frames, terminal result, and observed-progress response end to end.- The installers now accept an optional stderr sink. Interactive CLI callers keep the existing Print default, while the remotely triggered daemon path injects a quiet sink and leaves only the runner's bounded/redacted terminal diagnostic on the logger-backed path.
No additional code blockers found in the fix delta. There are still no database schema or migration changes; this remains a core wire-protocol and host-mutation boundary.
This approval covers the source diff on the exact head above. It does not waive the existing CHANGES_REQUESTED review or the paired Context Tree PR #946 review/merge gate. I did not rerun tests or QA for this review.
|
Product direction update: this backend foundation is paused and must not merge. The existing local The source PR is now draft. The paired Context Tree PR #946 merged prematurely before this source; its unshipped runtime claim will be reverted separately. If this direction is ever resumed, source must merge first, then the Tree update must be reconciled against the actual merged source before becoming ready. |
3 participants
Summary
Scope
This is the backend-only foundation for a later Web action. It adds no UI, database state, migrations, provider authentication, caller-supplied command/package/version input, or Windows-specific bootstrap behavior.
The paired durable runtime-boundary update is in draft Context Tree PR agent-team-foundation/first-tree-context#946.
Verification
pnpm typecheck@first-tree/clientsuite passedgit diff --checkpassedpnpm testwas also attempted. The shared suite completed with 933 tests passing, then Turbo concurrently ran the Client build and test presteps; both rebuildpackages/client/skills, producing anENOTEMPTYrace. Running the Client suite separately passed.Review
Independent Standards and Spec review on
8b979c64ec491f83f76db8cb5e713e9c75d9864fidentified and cleared the handler-less capability-advertisement and synthesized-progress findings. Maintainer review then identified two additional blockers: the top-level WebSocket dispatcher omitted runtime-install result frames, and supervised installs could stream raw npm stderr through CLI Print. Exact head9076e52f371ff707c704302984cdddfe90e2f849wires the attached-socket path with an end-to-end regression and injects a quiet stderr sink for remote daemon installs while preserving interactive CLI output; exact-head re-review is pending.