Add Goose agents with live Databricks model selection - #214
Conversation
9756c86 to
0d138b8
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: 0d138b804a
ℹ️ 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".
| draft={draft} | ||
| onChange={onChange} | ||
| /> | ||
| {goose && draft.provider !== "databricks_v2" ? ( |
There was a problem hiding this comment.
When a saved or imported Goose agent has GOOSE_PROVIDER=databricks_v2 as a write-only environment override but its selector is blank or names another provider, this condition hides AgentModelPicker even though native execution and goose_model_context correctly treat Databricks v2 as effective. The inverse also exposes a picker that native code rejects when the override selects another provider. Because saved override values are intentionally unavailable to React, gating solely on draft.provider prevents affected users from browsing the live catalog; the UI needs an override-aware flow rather than assuming the selector is authoritative.
AGENTS.md reference: AGENTS.md:L39-L42
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid edge case. The editor decides whether to show model browsing from the visible provider selector, while the runtime gives GOOSE_PROVIDER precedence. A saved, write-only override can therefore make the editor show the wrong control. It’s worth addressing, but doing it correctly needs a safe way for native code to tell the UI the effective provider without exposing the saved value. I would treat that as a focused follow-up rather than add a guess based on whether the override key exists.
There was a problem hiding this comment.
🤖 Fixed in 5c98864. The editor now uses a draft GOOSE_PROVIDER override when present and keeps Browse available when a saved override is write-only. Native code checks the effective provider before starting Goose. Mounted tests cover saved and draft overrides with blank and OpenAI selectors, plus the inverse draft override.
0d138b8 to
282190e
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Request changes at 282190eab9252b384aefc280307272e8a1a0c077 against e02fe33220fa497a2e7ee780c8295517b4f967fa.
Two P2 merge criteria:
- Resolve the existing effective-provider gating finding. I independently reproduced missing Browse for both saved and draft
GOOSE_PROVIDER=databricks_v2overrides with blank/OpenAI selectors in four mounted cases. This is part of the new discovery contract, not speculative hardening; keep discovery reachable without exposing saved environment values. - Preserve headless Goose Refresh, as detailed in the inline finding.
Validation: all reported hosted checks are green (Windows skipped); five isolated mounted probes confirmed the four gating cases and Refresh request wiring. Traced create/edit → IPC → persistence/runtime and the Goose ACP/auth implementation at upstream 80c1197583cc9dc909b7e010c78b4ad58c81e8ce. No production edits, live desktop/Goose launch, credentials, sign-in, or broad suite rerun. Actual installed-Goose integration and native click-through remain unverified.
Nonblocking compatibility note: catalog launch inherits desktop CWD, unlike runtime’s agent workspace. Upstream supports relative GOOSE_ADDITIONAL_CONFIG_FILES, so those configurations may resolve differently; this was source-traced, not reproduced against an installed Goose build. No additional merge criterion is inferred from that gap.
| let context = prepared?; | ||
| let model_overridden = context.model_overridden; | ||
| let models = crate::goose_models::fetch(context) | ||
| .await? |
There was a problem hiding this comment.
[P2] Keep Goose Refresh out of interactive OAuth
For a Goose Databricks v2 draft with a configured host but no usable cached token, Advanced → Model → Refresh models sends action: "refresh", yet this branch ignores the action and calls exactly the same Goose request as Browse. The documented contract says Refresh is always headless.
This is not merely a potentially interactive executable: upstream Goose’s supported-models handler constructs Databricks v2, whose missing-token fallback enters OAuth. On a cache miss or failed refresh, that flow opens a browser via webbrowser::open at line 415. Buzz passes no noninteractive mode, so a refresh can unexpectedly initiate sign-in and credential persistence.
Make Refresh fail with sign-in-required without opening a browser when noninteractive discovery is unavailable, or remove the separate Goose Refresh action and leave the explicit Browse/Retry sign-in path. Add coverage at the Goose auth boundary for absent/unrefreshable credentials. UI request wiring was reproduced; OAuth side effects were source-traced rather than triggered on a live account.
There was a problem hiding this comment.
🤖 Fixed in 5c98864. Goose no longer has a separate Refresh control. Native IPC rejects non-Connect Goose catalog requests before process spawn. The integration test verifies that Refresh does not invoke the fake Goose executable while explicit Browse still does.
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
Signed-off-by: Salman Mohammed <smohammed@squareup.com>
282190e to
5c98864
Compare
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 code blockers are resolved; no new blocking findings in the fixes. Reviewed 5c98864cad158ecd72640bc610541bc546d096bc against base 82bb3a631b96b92cd73940f16bcfb05fdaa7607e. The original feature patch is unchanged by the rebase; this pass covers the two agreed exit criteria and regressions in their changed paths.
- Provider browsing now respects known draft overrides and stays reachable for write-only saved overrides, with native effective-provider validation retained. Explicit removal/undo and custom model entry remain intact.
- Goose no longer offers Refresh; native non-Connect requests are rejected before Goose launch. Explicit Browse/Retry still use Connect, and Buzz Agent Refresh is unchanged.
- Validation: five additional mounted transition/cancellation probes passed locally. Hosted CI is green, including the no-spawn Refresh IPC regression and added provider-gating coverage. Windows native validation was skipped. No broad CI-equivalent rerun.
Native desktop create/edit/save/reopen and real Goose/OAuth execution remain unverified by reviewers. The installed-CLI catalog probe in the PR description is author-reported, not our execution. The earlier relative-config/CWD compatibility note remains nonblocking.
This comment supersedes the code objections in my earlier review; it is not a GitHub approval.
Carl, an automated reviewer, commenting via Wes’s GitHub account. Both code blockers are resolved at 5c98864; verified re-review: #214 (review). Dismissing my stale changes-requested verdict, not granting approval.
…rs-support * origin/main: feat: show owner-view agent memories in profiles (#231) Add opt-in Canvas-backed channel Todos (#222) Skip hidden folders when discovering plugins in a folder (#229) feat: add owned local agent actions to profiles (#190) feat(channels): move session creation into the context menu (#209) Add Goose as an agent harness option (#214) feat: preview channel agent activity in profiles (#187) Use context-aware identity names with human-first priority (#167) feat: add managed agents to channels from profiles (#196) Signed-off-by: Carl <c217fe6b9d958f41c3a5e030dccc7f626775a923089cb6491305eade75ea1f1b@buzz.block.builderlab.xyz> # Conflicts: # src/features/relay/outbox.ts
Why
Buzz users can choose Goose as an agent harness, but a mistyped Databricks v2 model ID only fails after an agent restarts and receives a message. Goose can list models available to the signed-in workspace.
What
GOOSE_PROVIDERoverride makes Databricks v2 effective.How
The desktop app starts a short-lived Goose ACP process and asks its supported-models method for Databricks v2 IDs. Native code resolves the effective provider, including write-only saved overrides, before it starts Goose. The editor uses known draft overrides and keeps Browse reachable when a saved override's value is unavailable. A direct Refresh request is rejected before Goose starts. Credentials stay in the native process.
Risk
Goose's supported-models ACP method is unstable, so a future Goose release could change it. Browse has a 60-second timeout and reports errors without saving settings. A saved override for another provider may show Browse; native validation then explains why Databricks discovery is unavailable. Existing Buzz Agent model discovery stays on its current path.
Testing
goose acpand requested_goose/unstable/providers/supported-models/listfordatabricks_v2. It returned 141 IDs, includingdata_workflow_tools.goose.goose-glm-5-3; the shortgoose-glm-5-3ID was absent.Generated with Codex