Add Pi agents with local model discovery - #216
Conversation
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
ff6bad7 to
6cafab4
Compare
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: 6cafab476f
ℹ️ 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".
| if [provider, id] | ||
| .iter() | ||
| .any(|s| s.is_empty() || s.len() > 512 || s.chars().any(char::is_control)) | ||
| || provider.contains('/') | ||
| { |
There was a problem hiding this comment.
When a Pi extension returns a provider or model ID containing a comma or beginning with -, this parser accepts it, so Browse exposes it and the editor can save it; however, PiContext::adapter_args rejects those same values during Start as an invalid provider/model ID. Validate catalog entries with the runtime's selection constraints so every selectable catalog result can actually launch.
Useful? React with 👍 / 👎.
|
screenshots from testing: 
|
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: one P2 catalog-to-launch contract defect. Reviewed head 6cafab476f68fc40c2e6f4db18c5d37f5557a1b6 against base 8842b3ac05862e069ab0adf2f30e11a3af084042, integrating independent UI, runtime, and process-lifecycle reviews.
I independently confirm the existing catalog-ID finding, rather than opening a duplicate thread. A catalog entry such as provider custom, model a,b passes parse_response, is selectable and saveable, but fails PiContext::adapter_args before launch. Leading-dash values have the same mismatch. The merge criterion is consistent supported-selection validation at the catalog boundary, with regressions proving rejected runtime values are not offered as selectable models. Preserve valid namespaced IDs.
No additional material blocker found in provider/model persistence, stale lookup fencing, or subprocess/ticket ownership. Current required CI is green: 2,871 frontend tests, native/tool integration, and Chromium/WebKit journeys. Broad suites were not rerun locally. Earlier installed-Pi/ACP probes predate the rebase; desktop create/edit → save → restart → first message, real authentication/inference, and signed packaging remain unverified, not proven by synthetic CI.
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Re-review: previous P2 resolved; no remaining code blockers found. Reviewed head 834a82be7b9d09def8d391c666c385e8eed5bc4e against base 8842b3ac05862e069ab0adf2f30e11a3af084042, focusing on changes since 6cafab476f68fc40c2e6f4db18c5d37f5557a1b6.
Catalog discovery and adapter launch now share selection validation, including persistence-compatible byte limits. The original comma/leading-dash mismatch is closed, with regression coverage preserving namespaced IDs. The new dropdown progress and bounded scrolling changes have no material finding after independent review. A focused isolated Chromium/WebKit probe confirmed loading text remains accessible in the popup and the outside status is aria-hidden until dismissal.
Required CI is green, including native/tool integration and Chromium/WebKit journeys. Broad suites were not rerun locally. Real desktop save → restart → first-message behavior, provider authentication/inference, and signed packaging remain unverified. This supersedes my previous changes-requested verdict; no approval is being submitted.
Carl: the original P2 is fixed at 834a82b. Verified re-review #216 (review) finds no remaining code blockers. Withdrawing the obsolete blocking verdict; no approval submitted.
Why
Buzz can run locally configured Pi agents, but needs to preserve the selected provider/model and discover extension-provided models before starting an agent. This adds Pi through the existing agent-control and model-picker paths.
What
How
Native discovery runs a short-lived Pi RPC process and requests only its available-model catalog. It reuses the model host's existing ticket and cancellation ownership. Credentials remain native; discovery neither saves settings nor starts a Buzz agent.
Risk
Goose #214 is merged. This PR is rebased onto main and contains only the Pi layer. Pi relies on locally installed
pi,buzz-pi-acpand Node, and an invalid custom model can fail only on the first message.Testing
After the review fix, the installed-Pi native catalog probe returned 112 models across four providers. The catalog regression rejected
custom/a,bonly after the fix; boundary coverage also preserves valid namespaced IDs.The existing Chromium/WebKit model-picker journey now checks bounded popup geometry, scrolling and searching beyond the initial visible results using an opt-in 20-model fixture. No browser cases were added or removed.
On the earlier Goose-based snapshot
0d138b8, the opt-in native probe used the installed Pi through the production context (including Advanced arguments) and returned 112 models across four providers without sending a prompt.Before the rebase, an actual ACP
session/newprobe reported the requested provider-qualified model and catalog without sending a relay message or inference prompt.No browser cases added or removed. CI exposed a custom
buzz-agentpath incorrectly inheriting bundled provider choices; executable-name fallback is now limited to Goose and Pi. The component regression failed before the repair and passed afterward. The existing model-picker journey passed in Chromium and WebKit, including the controlled-frame Escape boundary. The migrated component assertion waits for the shared combobox's deferred opening; an immediate assertion failed before that correction.Deferred: native desktop create/edit → save → restart → first-message round trip, real provider authentication/inference, signed release packaging and internal extension provisioning. Ready to try locally, not fully validated for live use.
Generated with Codex