fix(acp): improve hints in pi agent setup guide - #7594
Conversation
🔐 Codex Security Review
|
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. |
salman1993
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7ee3be287
ℹ️ 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".
| RequirementPayload::MissingBinary { command } => { | ||
| format!("install `{command}` or add it to PATH") | ||
| } |
There was a problem hiding this comment.
When buzz-pi-acp is absent, this new variant now reaches the desktop card, but ConfigNudgeCard does not classify missing_binary as informational or Doctor-routed: shouldOpenDoctor is false and informationalOnly omits it, so clicking the card opens Edit Agent. That surface cannot install a binary or repair PATH and does not expose the Pi install guidance added to Agent runtimes; route Pi's missing adapter there, or make generic missing-binary cards non-clickable.
Useful? React with 👍 / 👎.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Reviewed head e7ee3be287f929136327162e3913be8ec60a7f14 against base e17cdd9d5c7e2b836b4670ae88bb87a79f94337a.
The new enum variant fixes the actual producer/consumer mismatch: Desktop already emits missing_binary, which previously made ACP exit while parsing the setup payload. Two recovery defects remain on the newly reachable path:
P2: Keep the Desktop missing-binary card out of Edit Agent
crates/buzz-acp/src/setup_mode.rs:195-197 now makes this requirement reach ConfigNudgeCard, but Desktop replaces the Rust prose with that card. informationalOnly omits missing_binary, and shouldOpenDoctor returns false, so its trigger opens Edit Agent instead of showing the Pi installation guidance in Agent runtimes. The new Rust test's !body.contains("Open Edit Agent") does not protect the actual Desktop behavior.
Reproduction: start a Pi agent without buzz-pi-acp, mention it, and click its configuration card. It opens Edit Agent, which cannot install the adapter or repair PATH. A focused probe using the exact new Rust fixture and production card function confirmed Open Edit Agent for Carol and an Edit Agent dispatch, with no Settings dispatch. See desktop/src/shared/ui/config-nudge-attachment.tsx:190-220,281-289 and desktop/src/shared/ui/markdown.tsx:1832-1839.
Route recognized preset installation requirements to Agent runtimes, or make the generic missing-binary card informational rather than clickable. Ensure the displayed card includes the necessary recovery instruction. Add coverage at the card consumer, not only the Rust prose.
P2: Tell users to restart Buzz, not only the agent
The new footer at crates/buzz-acp/src/setup_mode.rs:281-282 gives an insufficient recovery action. Desktop's resolve_command caches missing binaries for the app lifetime (discovery.rs:426-452). After an external npm install into an existing PATH directory, restarting the agent reuses cached None and emits the same setup payload again. Both the pair restart command and the frontend stop/start route reach spawn_agent_child without refreshing discovery (runtime_commands.rs:244-315,386-392; managedAgentRuntimeHooks.ts:163-201).
Reproduction: let Desktop classify buzz-pi-acp as missing, install it externally, then restart only the agent without a forced runtime recheck. It remains in setup mode despite the executable now existing. A focused probe of the production cache functions confirmed that creating the missing file does not change resolution until clear_resolve_cache is called.
The smallest fix is to say restart Buzz, matching the new preset install hint. Alternatively instruct an explicit runtime recheck followed by agent restart. No new cache/recovery subsystem is needed.
Non-blocking: crates/buzz-acp/TESTING.md:4-9 calls the adapter pinned but installs #main, whereas the preset uses #86b201e. Align those references for reproducible validation. I confirmed the existing Codex card-routing finding; the documentation-pin comment is a consistency issue, not a demonstrated current install failure. The fork is two commits ahead of the pin, but the verified comparison changes only README.md, not adapter code.
Validation: exact-head source tracing, clean-tree git diff --check, focused production-card and cache probes with dependencies mocked at their boundaries. No full local suites, global npm install, live relay nudge, or native Desktop workflow run. Existing CI covers ACP and Tauri tests, but major test/build checks were unfinished in the one status snapshot; this is not a green-CI claim.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes required. Source-only review of head e7ee3be287f929136327162e3913be8ec60a7f14 against base e17cdd9d5c7e2b836b4670ae88bb87a79f94337a.
The new MissingBinary variant repairs the existing Desktop → ACP serialization mismatch. The intended contract is a recoverable missing-adapter state, with Desktop remaining the sole readiness authority. Two P2 recovery defects remain on this newly reachable path. They are already detailed in the concurrent human-requested review; this automation record does not duplicate its inline findings or adopt its runtime-test claims.
- P2: The displayed Desktop nudge still opens Edit Agent and hides recovery instructions. Start an existing Pi agent without
buzz-pi-acp, mention it, then click its configuration card. The new requirement atcrates/buzz-acp/src/setup_mode.rs:195-197reaches the card, butcomputeConfigNudge.ts:42-46suppresses the Rust prose.desktop/src/shared/ui/config-nudge-attachment.tsx:190-220,281-289treats missing-binary-only cards as clickable Edit Agent cards; its row at386-398only says install/check PATH. Edit Agent cannot perform this installation, and the new Rust prose assertion does not cover the actual card. Make a generic missing-binary-only card informational, or route recognized preset installation needs to Agent runtimes, and show the recovery instruction in the card itself. - P2: “Restart the agent” is insufficient after manual installation. The new footer at
crates/buzz-acp/src/setup_mode.rs:281-282omits the necessary cache refresh. Reproduction: let Desktop resolvebuzz-pi-acpas absent, install it externally into an existing PATH directory, then restart only the agent without a forced runtime recheck.desktop/src-tauri/src/managed_agents/discovery.rs:426-452retains cachedNone; the stop/start path inmanagedAgentControlActions.ts:88-106→commands/agents.rs:163-282→managed_agents/runtime.rs:877-932reaches readiness without clearing that cache. The agent returns to setup mode. The smallest fix is restart Buzz, matching the new preset hint, or explicitly recheck runtimes and then restart the agent. No new recovery subsystem is required.
Non-blocking: crates/buzz-acp/TESTING.md:4-9 calls its install pinned while using mutable #main; production pins #86b201e. Align the documentation for reproducible validation. The independent adapter/install lane found no additional source-backed install blocker; a live npm installation was not performed.
Validation: exact-head source/diff review on Wes’s Studio; inspected payload producers/consumers, malformed-input fallback, authorization gating, startup/listener/restart paths, discovery invalidation, and Desktop prose/card rendering. Non-card clients retain the prose; no matching specialized nudge consumer was found in the searched mobile/lib and web/src trees. No checkout, build, tests, dependency installation, PR-code execution, or live Desktop workflow was performed by this automation. This is not a green-CI claim.
|
🤖 @wesbillman, the requested fixes are pushed. When you have a moment, could you please re-review? Thank you! |
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Re-review: both prior P2 findings are resolved. No remaining source-backed blockers in the reviewed fixes. Reviewed head fee8c88c447db5fe393d6d472229bf31d073d165 against base e17cdd9d5c7e2b836b4670ae88bb87a79f94337a, focusing on changes since e7ee3be287f929136327162e3913be8ec60a7f14.
- Desktop recovery route fixed.
config-nudge-attachment.tsx:57-79,204-234,285-303,400-413routes missing-binary cards to Agent runtimes, labels that action correctly, and visibly renders the recovery instructions. The fix reaches the actual card rather than relying on Rust prose that Desktop hides. - Restart guidance fixed.
crates/buzz-acp/src/setup_mode.rs:281-282and the rendered Desktop row now instruct users to install/update PATH, then restart Buzz. That satisfies the agreed recovery contract despite the existing negative command-resolution cache. No cache redesign is needed. - Documentation discrepancy resolved.
crates/buzz-acp/TESTING.md:4-16explicitly distinguishes the development adapter onmainfrom the reviewed revision pinned for Desktop users and supplies the Node/Pi/PATH prerequisites.
This resolves the two P2s from the previous source review. The added Desktop tests import the production routing/copy helpers; direct source inspection confirms the card consumes them. Independent Desktop review returned the same disposition.
Validation: exact-commit source/diff inspection on the Studio and clean git diff --check. No checkout, builds, tests, dependency installation, PR-code execution, live npm install, or native Desktop workflow was performed. The tests were inspected, not run; no green-CI claim. This is a non-approval review comment, not a GitHub approval.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Review clear: both prior P2 recovery findings are resolved. Reviewed head fee8c88c447db5fe393d6d472229bf31d073d165 against base e17cdd9d5c7e2b836b4670ae88bb87a79f94337a, focusing on the remediation since e7ee3be287f929136327162e3913be8ec60a7f14 and consuming the existing exact-head re-review.
The contract remains recoverable missing-adapter setup, with Desktop as the sole readiness authority. No new readiness/cache subsystem is needed.
- Desktop recovery route fixed.
desktop/src/shared/ui/config-nudge-attachment.tsx:57-79,204-234,285-303,400-413routes missing-binary cards to Agent runtimes, supplies the matching visible/accessibility action label, and renders installation/PATH/restart guidance in the actual card. The fix does not rely on prose that Desktop suppresses. The new tests import the production routing/copy helpers, and source inspection confirms the component consumes them. - Restart guidance fixed.
crates/buzz-acp/src/setup_mode.rs:270-282,753-768and the Desktop row now say restart Buzz, addressing the existing app-lifetime negative resolution cache. The current producer emits missing-binary as a single requirement (readiness.rs:433-441); hypothetical mixed payloads do not justify expanding this repair. Desktop serialization, ACP deserialization, and the existing frontend requirement validator agree onmissing_binarypluscommand. - Documentation discrepancy resolved.
crates/buzz-acp/TESTING.md:3-16explicitly distinguishes developmentmainfrom the pinned user-facing runtime catalog. The production install pin is unchanged from the earlier reviewed head.
Validation: exact-commit source/diff inspection on Wes’s Studio and git diff --check for the stated base/head. Independent Desktop source review integrated. Reused prior evidence for unchanged listener authorization/reconnect behavior and adapter installation; no fresh broad investigation of those paths. Non-card clients retain the corrected prose; remote deployment is outside this host-readiness change (docs/remote-agents.md:595-611). No checkout, build, tests, dependency installation, PR-code execution, live npm installation, or native Desktop workflow was performed. Tests were inspected, not run; no green-CI claim. This is a non-approval review comment.
Amp-Thread-ID: https://ampcode.com/threads/T-01a091db-fdb0-70de-a926-3863bb37dd2d Co-authored-by: Amp <amp@ampcode.com> Signed-off-by: Salman Mohammed <smohammed@squareup.com>
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
fee8c88 to
e719cd8
Compare
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Review clear: no remaining source-backed blockers in the missing-binary recovery change. Reviewed head e719cd85e91e9f0ef2ba5d571224571a852a3207 against base f3940ff21e8b6050ff617c645578aefa45db7e9f.
The contract remains recoverable missing-adapter setup, with Desktop as the sole readiness authority. Both earlier P2 findings remain resolved at this head; this review also consumed the prior remediation review.
- Producer/consumer agreement: Desktop’s existing
missing_binary+commandpayload now deserializes in ACP, survives sentinel serialization, and matches the Desktop validator (runtime/setup_payload.rs:78-81,crates/buzz-acp/src/setup_mode.rs:119-120,232-239,303-308,configNudge.ts:168-169). The setup listener retains its existing authorization and mention gates; no adapter process is started on this branch. - Actual Desktop recovery: the rendered card routes to Settings → Agents, supplies the matching visible/accessibility label, and displays install/PATH/restart Buzz instructions (
config-nudge-attachment.tsx:67-79,204-234,285-303,400-413). The destination includes runtime installation guidance and “Check again.” The Rust prose also says restart Buzz (setup_mode.rs:281-282), matching the app-lifetime negative command cache. Current readiness emits missing-binary alone (readiness.rs:433-441), so hypothetical mixed payloads do not require new machinery here. Independent frontend source review returned clear and was integrated. - Adapter guidance: the pinned fork revision resolves to
86b201e835d7bdb87d623da561aa916c95dc8925; its package declares thebuzz-pi-acpexecutable and build packaging. Development docs now explicitly distinguish mutablemainfrom the user-facing pin. The added regression tests call the production parser/routing/copy functions, and the card consumes those helpers.
Validation: exact-head source/diff inspection on Wes’s Studio and git diff --check for the stated base/head. Checked startup → readiness payload → ACP listener → Desktop card and discovery refresh, plus the pinned adapter’s package/build metadata. Non-card clients retain the corrected prose; no specialized nudge consumer matched the searched mobile/lib and web/src trees. Remote deployment and external CLI authentication behavior are outside this local missing-binary repair.
No checkout, builds, tests, dependency installation, PR-code execution, live npm installation, or native Desktop workflow was performed. Tests were inspected, not run; no green-CI claim. This is a non-approval review comment.
2 participants
Summary
Handle missing Pi adapter setup and update its install guidance.
Related issue
None found.
Testing
No manual testing.
Generated with Amp