Test provider connections before model selection - #500
Conversation
Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Salman Mohammed <smohammed@squareup.com>
Co-authored-by: Codex <noreply@openai.com> 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: a7e6dfff67
ℹ️ 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".
| pending.current?.abort(); | ||
| pending.current = null; | ||
| setBusy(false); | ||
| setStatus(""); |
There was a problem hiding this comment.
When Pi’s automatic catalog load—or an explicit Goose browse—is still running, starting the newly available connection test aborts that lookup and clears its status but leaves attempted.current === key. After the test finishes, onBrowse refuses to run another lookup for that key, and clearing status also removes the Retry button, so the user cannot browse models without changing another context field or reselecting the harness. Reset attempted.current when the test cancels a lookup, or retain an explicit retry state.
AGENTS.md reference: AGENTS.md:L83-L86
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 Fixed in 764bd13 (current head c7b2c3d). Test now clears the lookup attempt marker when retiring the lookup, so an explicit Browse starts another request for the same draft. The shared Pi/Goose form regression failed for both harnesses before the one-line fix and now verifies a returned model option with the draft still unchanged.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Reviewed head a7e6dfff67bacc47278304bb0cee22449f897eb4 against base 0124f3fdfc9ece433d1ecb71aa43f2a378980f19.
Recommend fixing before merge; not approved.
- Two P2 product findings: Independently confirmed the existing Browse-after-Test recovery finding; linking rather than duplicating its thread. The additional Pi provider-scope false negative is detailed inline. Restore same-draft Browse retry and make blank-model selection truly provider-specific, with regression coverage for both.
- Separate CI/test regression: The native Pi IPC fixture is stale after the handshake change, detailed inline. Rust CI fails that test (203 passed, 1 failed, 7 ignored). Repair the fixture without weakening selection verification and obtain a green required check.
- Validation: Source/contract review across UI cancellation, native admission, selection and write-only overrides. Pi scope behavior checked against supported floor 0.99.0 and current 0.99.2 source. Existing JavaScript and Chromium/WebKit CI passed. No local native build or live-provider test performed for this review; native UI, cross-platform behavior and valid-key OpenAI/Anthropic/Google/OpenRouter checks remain unverified, as the PR notes.
| .map(String::from), | ||
| ); | ||
| if model.is_empty() { | ||
| args.extend(["--models".into(), format!("{provider}/*")]); |
There was a problem hiding this comment.
[P2] Select an exact provider rather than treating provider/* as a provider filter
Pi's --models glob is not provider-specific: it matches both provider/modelId and the bare modelId (supported Pi 0.99.0 resolver). Pi then prefers its saved default when that model is in the scope (selection).
With working OpenAI credentials and an available saved model from another provider whose bare ID is openai/<model> (for example an aggregator or extension), choosing OpenAI and leaving Model blank can select that other provider through openai/*. The subsequent provider check correctly prevents inference to it, but reports “No test model available… Add its API key” instead of testing the available OpenAI model. Changing the valid OpenAI key cannot repair this false negative.
Choose from Pi's available models using exact model.provider == requested_provider, then select that exact pair (for example via RPC set_model) and retain the pre-prompt verification. Add a regression with both providers authenticated and a saved cross-provider namespaced model. The same matching/selection behavior is present in Pi 0.99.2; this is not hypothetical glob syntax.
There was a problem hiding this comment.
🤖 Assessed this separately from the fallback case we tested earlier. On installed Pi 0.99.1, an isolated configuration with a saved review-aggregator/openai/gpt-4o selected review-aggregator under --models openai/* even though 44 OpenAI models were available. No prompt was sent. Exact set_model RPC followed by get_state selected OpenAI correctly. Our existing guard prevents wrong-provider inference, but this confirms a false connection failure is possible. I am holding this production change pending the author’s decision; the Pi scope implementation is unchanged in this update.
| .write_all(b"{\"id\":\"selection\",\"type\":\"get_state\"}\n") | ||
| .await | ||
| .map_err(|_| TEST_FAILURE)?; | ||
| let selection = response(&mut reader, "selection").await?; |
There was a problem hiding this comment.
[P2] Update the existing IPC fixture for the new selection handshake
This get_state round trip deterministically breaks agents::tests::pi_connection_test_prompts_the_draft_selection (src-tauri/src/agents/tests.rs:2257–2285). Its shell fake reads one request and emits only message_end, so response(..., "selection") reaches EOF and the success case unwraps an error. The fake also still matches --model model-a instead of --model databricks/model-a and omits the provider/model/text now required in the reply.
The hosted Rust job fails at line 2281 on this exact head. Update that native IPC fixture to answer and assert get_state before accepting prompt, check the qualified model argument, and emit matching provider/model plus assistant text for success. Retain the rejected-key case. This is a stale test-contract regression, not evidence that real Pi cannot complete the handshake.
There was a problem hiding this comment.
🤖 Fixed in 764bd13 (current head c7b2c3d). The native IPC fixture now expects canonical databricks/model-a or model-b, answers get_state before accepting prompt, and includes provider/model/text in the assistant reply. It also checks testedModel and retains the rejected-key case. I reproduced the original CI failure locally; the corrected IPC test passes. Hosted CI is running at the new head.
Update the Pi native IPC fixture for verified model selection and retain the rejected-key check. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Salman Mohammed <smohammed@squareup.com>
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7b2c3d1cc
ℹ️ 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".
Preserve bundled Goose inference and temporary-session cleanup while resolving blank models through provider metadata. Signed-off-by: Salman Mohammed <smohammed@squareup.com>
* origin/main: (82 commits) Test provider connections before model selection (#500) Bundle Goose ACP with Buzz (#497) Discover saved identities across joined communities with names, pictures and retry (#291) Clarify design-system documentation and unify component examples (#498) feat(composer): convert typed Markdown live and refuse control characters committed as text (#455) fix(messages): stop three timeline scroll races that flake CI (#456) Improve Agent defaults pickers and provider keys (#392) fix(threads): keep thread history painted after scroll corrections (#493) feat(plugins): expose the agent protection service (#421) perf(sidebar): re-render only the changed row on a channel-list publish (#480) feat(agents): copy protection defaults into new agents (#420) feat(agents): support native launch protection providers (#415) fix(composer): prevent WebKit overpainting mention selections (#490) fix(composer): prevent arrow keys from inserting control characters (#488) perf(channels): fall back to one exact roster read when confirming agent adds (#485) fix(media): pause video only on comment composer focus (#483) fix(channels): dismiss management modals with outside clicks (#479) perf: reuse message date formats and stable reaction shortcuts (#477) feat(profile): run an unattended scenario file in web profiling (#476) feat(channels): administer channel members and roles (#453) ... Signed-off-by: John Tennant <jtennant@block.xyz> # Conflicts: # src/app/shell/usePanelLauncher.ts # src/bundled/agents/AgentsPage.tsx # src/bundled/agents/InventoryIdentityCard.tsx # src/bundled/agents/InventoryView.tsx # src/bundled/agents/UnifiedInventory.tsx # src/bundled/agents/index.tsx
* origin/main: (82 commits) Test provider connections before model selection (#500) Bundle Goose ACP with Buzz (#497) Discover saved identities across joined communities with names, pictures and retry (#291) Clarify design-system documentation and unify component examples (#498) feat(composer): convert typed Markdown live and refuse control characters committed as text (#455) fix(messages): stop three timeline scroll races that flake CI (#456) Improve Agent defaults pickers and provider keys (#392) fix(threads): keep thread history painted after scroll corrections (#493) feat(plugins): expose the agent protection service (#421) perf(sidebar): re-render only the changed row on a channel-list publish (#480) feat(agents): copy protection defaults into new agents (#420) feat(agents): support native launch protection providers (#415) fix(composer): prevent WebKit overpainting mention selections (#490) fix(composer): prevent arrow keys from inserting control characters (#488) perf(channels): fall back to one exact roster read when confirming agent adds (#485) fix(media): pause video only on comment composer focus (#483) fix(channels): dismiss management modals with outside clicks (#479) perf: reuse message date formats and stable reaction shortcuts (#477) feat(profile): run an unattended scenario file in web profiling (#476) feat(channels): administer channel members and roles (#453) ... Signed-off-by: Sol <49aa1f65411fd096d2e2ec144f1e7aa36fdc76d1b907cfdf7be000c66f9d3b8e@buzz.block.builderlab.xyz> # Conflicts: # src/app/shell/usePanelLauncher.ts # src/bundled/agents/AgentsPage.tsx # src/bundled/agents/InventoryIdentityCard.tsx # src/bundled/agents/InventoryView.tsx # src/bundled/agents/UnifiedInventory.tsx # src/bundled/agents/index.tsx
…page-icon * origin/main: Count unread replies only in conversations you are part of (block#471) Animate the terminal welcome with a compact hex wordmark (block#508) Use top tabs in the new-tab picker (block#505) Polish media controls, panel headers, and menus (block#496) harden pinned browser CI setup and native fixture provenance (block#494) perf(relay): confirm membership hints with exact channel reads (block#486) test(browser): wait for menu and wheel completion (block#492) fix(links): render one hash on completed channel links (block#506) ci: publish Windows and Linux alongside macOS previews (block#491) fix(channels): keep conversations open through archive and restore (block#452) feat(channels): align create and edit forms with draft protection (block#482) Test provider connections before model selection (block#500) Signed-off-by: Matthew Boston <mboston@squareup.com>
Why
Create agent requires a model before showing Test connection, but the model list can need valid provider credentials first. This leaves users unable to verify their API key at the point they enter it.
What
Show Test connection below the provider/API key and above Model for both Goose and Pi. A successful test reports the model used without filling or changing the draft. Testing can cancel a pending lookup; Browse models remains available afterward.
How
Reuse the shared form, request lane, cancellation, and result handling. For a blank model, Goose supplies its provider's default through metadata; Pi chooses within a provider scope. Buzz verifies Pi's actual selection before prompting, so an empty scope cannot send a request to a saved provider. There is no default-model mapping.
The merge preserves #497’s bundled
goose-acprouting, ACP inference, recorded-reply verification, and temporary-session cleanup. Provider-default lookup uses the resolved runtime’s arguments: none for the sidecar,acpfor an explicit external CLI. Goose keeps its token and thinking defaults. The external CLI still requires reported usage because provider errors can appear as assistant text with a successful exit. Saved environment overrides remain write-only.Risk
This changes explicit connection tests for bundled Goose ACP, external Goose CLI pins, and Pi. External Goose CLI providers that omit usage cannot confirm success. Bundled Goose retains #497’s temporary-session cleanup; an unresponsive process can leave a test session in Goose history. Agent native UI verification and valid-key checks for OpenAI, Anthropic, Google, and OpenRouter remain deferred. Pi can choose another provider whose model ID matches the provider glob. Buzz rejects that selection before prompting, but can report a false failure; the exact-provider follow-up awaits the author’s decision.
Testing
BUZZ_TEST_PI_ADAPTER=/opt/homebrew/bin/buzz-pi-acp BUZZ_TEST_PI_PROVIDER=databricks BUZZ_TEST_PI_MODEL=databricks-claude-opus-5-5 bin/cargo test -p buzz-foundation pi_models::tests::installed_pi_connection_test_uses_production_context --lib --locked -- --ignored --nocaptureLive checks preceded the rebase onto main. Focused checks passed after the rebase. The latest update merges main’s shared API-key field and repairs the stale native IPC fixture while retaining key rejection coverage. The shared Pi/Goose Browse regression failed before the one-line repair and passed afterward. The local native suite hit three timing failures in unchanged host-command tests; that full module passed separately. No browser journeys were added or removed. Builds and broad cross-platform validation are left to CI.
The resolution against #497 passed independent merge review and a compile-only check (
bin/cargo check -p buzz-foundation --tests --locked). At the author’s request, no additional native or live test suites were executed after the merge. The required pre-push hooks passed when publishing the resolution. The earlier hosted CI run was canceled. The latest merged behavior remains unverified in the native app.Follows #383.
Generated with Codex