feat(agents): one-click Pi install in Harnesses (slice 5/5) - #330
salman1993 wants to merge 3 commits into
Conversation
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
…ain failures Signed-off-by: Salman Mohammed <smohammed@squareup.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 474e9544a5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pub use import::{CredentialedImport, ImportPreview, Imports, LegacySource, PreparedImport}; | ||
| pub use restart::{RestartChange, RestartDiffEntry}; | ||
| pub use runtime::{installed, Action, Controller, GooseModelContext, ModelContext}; | ||
| pub use runtime::{installed, managed_tool, Action, Controller, GooseModelContext, ModelContext}; |
There was a problem hiding this comment.
Commit 18a59f3cb73f23d8e8fe29d8dc9a822311967e3b has no Signed-off-by trailer, so it violates the repository’s per-commit DCO requirement and will fail the hosted DCO Check. Recreate the commit with git commit --signoff using the verified author identity before submitting it.
AGENTS.md reference: AGENTS.md:L153-L156
Useful? React with 👍 / 👎.
| for path in [ | ||
| prefix.to_path_buf(), | ||
| prefix.join("bin"), | ||
| prefix.join("lib"), | ||
| prefix.join("lib/node_modules"), | ||
| prefix.join("cache"), |
There was a problem hiding this comment.
When node-tools/lib/node_modules/@earendil-works already exists as a symlink, every path inspected here remains a real directory, but npm follows that scoped parent and writes pi-coding-agent into the symlink target. Thus Install can modify files outside the promised app-owned prefix; validate the complete destination ancestry, including the package scope, or install into a clean staging prefix before invoking npm.
AGENTS.md reference: AGENTS.md:L10-L12
Useful? React with 👍 / 👎.
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord’s automated source review (via Wes’s account)
No actionable findings in the reviewed revision.
- Head:
474e9544a5883dabcc1f22e1f1b61809e9538d7c - Base / merge base:
85d6bf82c54d1c8d930d58444597a1fe31cc8975
Reviewed the full 20-file diff and supported callers: managed Node download/checksum/staging, app-owned npm prefix and environment, install ownership and Quit cleanup, Pi detection and launch/model-discovery paths, missing-executable restart admission, native IPC permissions, and Settings progress/recovery. Re-read the lifecycle and path-selection boundaries independently of the added tests; the source preserves complete user installations and saved absolute executable paths. The pinned Node checksums also match the existing repository pins and old Buzz source.
Validation limits: source analysis only—no PR code, tests, installs, app launches, credentials, or live services were exercised. A single read-only exact-head CI snapshot showed CI required, JavaScript, Rust/tool integration, browser journeys/measurements, DCO and security checks passing; Windows validation was skipped. This does not establish live desktop installation, proxy/certificate behavior, restart/Quit acceptance, sign-in or inference. The PR’s reported local tests/install were not independently rerun. Integration with the separately developing #329 is outside this pinned review; its acknowledged overlap still needs reconciliation and validation.
Non-blocking COMMENT review only; not GitHub approval or merge authorization.
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord's automated source review (via Wes's account)
No new actionable findings. This is a follow-up to the no-findings review at 474e9544a5883dabcc1f22e1f1b61809e9538d7c, limited to the subsequent code change and its affected callers.
- Head:
5d7991a43ca3396a037ef6204f8937292b242098 - Base / merge base:
85d6bf82c54d1c8d930d58444597a1fe31cc8975
The revision comparison contains only src-tauri/src/managed_pi.rs: four additional directory checks and expanded existing test coverage. The change meets the 9/10 source-review bar for minimalness, elegance, and correctness within that scope.
refuse_linked_prefix now also checks the package scope, both installed package directories, and the app-owned npm configuration directory. The existing guard runs before prefix creation and before each npm step (managed_pi.rs:319–324). Missing paths remain valid for a first install; existing regular directories remain valid; links and non-directories return the existing user-facing failure. Normal npm executable shims under bin are not rejected by these new directory checks. The test source covers each newly checked location and verifies that the cleaned directory structure is accepted.
Traced the unchanged install owner and failure reporting: a guard failure reaches InstallReport with ready: false, and pi_install returns before re-detection/restarts (harness_setup.rs:150–189,399–405). No new install or process-lifecycle owner is introduced. Repository instructions, the agent-control contract, and relevant Buzz vision were checked; reviewed blobs were hash-verified with no dirty source inputs.
Validation limits: source analysis only. No PR code, tests, installs, app launches, credentials, or live services were exercised. One exact-head CI snapshot at 2026-09-28 03:07:23 UTC showed DCO, Semgrep, and zizmor passing; JavaScript, Rust/tool integration, browser journeys, and browser measurements were still running; Windows validation was skipped. The reported prior-head install/test evidence was not rerun and does not validate this head. Live desktop installation, proxy/certificate handling, restart/Quit behavior, and integration with #329 remain outside this review's validation.
Non-blocking COMMENT review only; not GitHub approval or merge authorization.
Slice 5/5 — one-click Pi install (draft)
Adds Install to the Pi Harnesses row only for
CLI needed/Adapter neededon supported macOS/Linux. The two existing copy commands remain the manual path; only Buzz Agent, Goose, and Pi appear.Design
https://nodejs.org/dist/v24.18.0/<filename>, 90-MiB download cap and SHA-256 checksums from old Buzzdesktop/src-tauri/src/commands/agent_discovery/managed_node.rs:8–43,300–323,362–424. Verify before extracting the tarball into the new app's app-dataruntimes/node/v24.18.0/<platform>; unsupported platforms keep the manual path. The app-datanode-toolsnpm prefix rejects linked directories.env_clear, a minimal tool PATH, the real HOME (so~/.npmrcmirrors/auth apply), explicit proxy/cert variables and anynpm_config_*/NPM_CONFIG_*settings except prefix, cache and globalconfig, which stay app-owned. npm failures name the registry/~/.npmrcfix. Install exactly@earendil-works/pi-coding-agentthengit+https://github.com/salman1993/buzz-pi-acp.git#86b201ewith--install-links=true; logs stay owner-only (0600). No user-global install or~/.localwrites.Verification / limitations
At signed-off
474e9544(review fixes):pnpm check, Rust fmt,cargo clippy --workspace --all-targets -D warnings, 107 serial native-library tests (three ignored), controller tests (81 + 5 + 11), full Vitest 4,434/4,434, and 38/38 Chromium/WebKit agent-control browser journeys passed. A real install into a throwaway HOME/app-data under/tmp, with the user's~/.npmrccopied in, downloaded and verified Node v24.18.0, installed Pi 0.87.1 and buzz-pi-acp 0.0.33 into the app-owned prefix through the configured mirror,managed_tooldetected all three, and the adapter answered ACPinitializevia managed Node. The 0600 log was confirmed, then the scratch directory was removed. A live native desktop install/restart/proxy/Quit has not been verified.Slice 4 is developing against main concurrently. Read-only
git merge-treeof current #329/#330 heads reports content conflicts incrates/agent-controller/src/runtime/tests.rs,docs/agent-control.md,src-tauri/src/agents.rsandsrc-tauri/src/agents/tests.rs; other overlapping settings/control/native wiring files auto-merge but still need semantic review. This PR does not edit defaults or Save behavior. Browser-only changes: none; existing journeys are rerun, and the new visibility/remount matrix is covered in Vitest.