Repository navigation
Improve AI defaults and add Goose configuration - #1266
Conversation
Co-authored-by: fin <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz> Signed-off-by: klopez4212 <klopez4212@gmail.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. |
![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: 7d54fc8f76
ℹ️ 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".
Co-authored-by: fin <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz> Signed-off-by: klopez4212 <klopez4212@gmail.com>
Co-authored-by: fin <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz> Signed-off-by: klopez4212 <klopez4212@gmail.com>
Co-authored-by: fin <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz> Signed-off-by: klopez4212 <klopez4212@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbdb5c1ff4
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebe283eeba
ℹ️ 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".
Co-authored-by: fin <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz> Signed-off-by: klopez4212 <klopez4212@gmail.com>
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.
Verdict: not ready yet. Court review (Goose runtime, Claude/Codex, design-system lanes) at ebe283ee, from source only, then rechecked at 812a84f8. We ran no suites and launched no app, and hosted CI was still pending when we looked.
Confirmed Codex threads (please fix; details stay in each thread):
- r4297123192, r4297394985: Codex, Claude (
claude-code/tabs.tsx:411) and Goose have no way back to the blank, inherited model. This conflicts withdocs/agents.md. - r4297123199, r4297395008: the defaults rows have no error or retry when model discovery fails.
Addressed at 812a84f8: r4297326973 (P1). saveDefaults now calls runtime.defaultsChanged(), which bumps the fingerprint and reconfigures only agents with a blank provider. The new runtime test covers both the inherited and the explicit case. Source and test review only; the live journey below is still needed.
Refuted: r4297326980. The pinned Goose rejects unconfigured providers before it writes anything (reply in that thread).
New findings are inline: dismissal during key save, the trigger's accessible name, card chip appearance/overflow, and the Menu vs. Select/Combobox design contract. Optional nits on Combobox and a misplaced doc comment in runtime.ts.
Merge criteria: the threads above are fixed. Tests for the removed Claude retry journey and the Codex recommendation reset come back through the new controls. Someone exercises the Goose defaults → open-session journey, plus the card chip in both themes at narrow width.
| data-buzz-ui="" | ||
| className={`buzz-select-popup text-body ${className}`} | ||
| data-variant="field" | ||
| initialFocus={search ? true : undefined} |
There was a problem hiding this comment.
🤖 Optional: initialFocus={search ? true : undefined} looks redundant. Base UI already focuses an input inside the popup, and forcing true skips its touch branch, which focuses the popup itself so the virtual keyboard stays closed. Separately, the search input's hardcoded aria-label="Search" could take a caller label such as "Search providers".
| /** Brings the runtime in line with this type's agents: changed settings | ||
| * reopen idle sessions, and removed agents stop. goose stops with the | ||
| * last. */ | ||
| private defaultsRevision = 0; |
There was a problem hiding this comment.
🤖 Optional: this block was inserted between sync()'s doc comment ("Brings the runtime in line…") and sync(), so that comment now documents defaultsRevision. Move the new field and method above the comment.
There was a problem hiding this comment.
🤖 Still present at bd6ab479 (optional).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 812a84f86a
ℹ️ 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".
Co-authored-by: fin <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz> Signed-off-by: klopez4212 <klopez4212@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1dd7e3f31
ℹ️ 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".
Co-authored-by: fin <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz> Signed-off-by: klopez4212 <klopez4212@gmail.com>
Co-authored-by: fin <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz> Signed-off-by: klopez4212 <klopez4212@gmail.com>
Co-authored-by: fin <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz> Signed-off-by: klopez4212 <klopez4212@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b78cd0e774
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e5ed6f0fd
ℹ️ 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".
Co-authored-by: fin <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz> Signed-off-by: klopez4212 <klopez4212@gmail.com>
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.
Verdict: not ready yet. This is a Court re-review at bd6ab479, from source only. We ran no tests and didn't launch the app. Hosted CI is green.
Fixed since 812a84f8:
- Reset actions for Claude, Codex and Create Agent.
- Claude and Goose discovery retry.
- Blocking now covers the whole credential + defaults save.
- The trigger's accessible name and checked state.
- Chip appearance, ellipsis and title.
- External
goose configureinvalidation. - The menu exception is now documented.
Still open (details in each thread):
- r4297680079, r4299871282: changing only the effort pins the resolved model. Confirmed for Claude, Codex and Create Agent.
- r4299919501: the Settings model picker stays hidden after a credential save. Confirmed.
- New inline:
- Inherited and pinned models look identical on the family chips. This is the root cause of the effort bug.
- "Use provider default" on the Goose defaults row doesn't reset anything with real Goose.
- Merge
b78cd0e7reverted #1177's Goose Settings label and test.
Owner decision needed: c1dd7e3f reverses #1260's Goose and provider marks (inline thread).
Merge criteria:
- The threads above are fixed.
- Tests cover effort-on-inherited and the Goose defaults reset, with the fake matching pinned Goose.
- The #1260 owners confirm the reversal or the marks come back.
- Last review's remaining checks are done: the chip visual check and the Goose defaults → open session journey.
| includeDefault={false} | ||
| resetLabel="Use Goose default" | ||
| allowCustomModel | ||
| label="Goose model" |
There was a problem hiding this comment.
🤖 Owner decision: this reverses #1260. c1dd7e3f removes the Goose and provider marks that #1260 added to this chip and to the provider options. #1260 is another author's PR, merged to main today. The commit also rewrites #1260's test to expect the marks gone and leaves AwsLogoIcon/DatabricksLogoIcon unused. Model family icons are in this PR's stated scope. Removing provider identity is a separate change, and neither the PR description nor the documented menu exception mentions it. This isn't a correctness defect. Please get the #1260 owners to confirm it, or restore the marks alongside the family icon.
There was a problem hiding this comment.
🤖 Yes, this is intended. Kenny explicitly confirmed that provider dropdowns should be text-only and the Create Agent model control should show only the selected model family icon, not Goose or Databricks/provider icons. This intentionally supersedes that part of #1260; the configuration still preserves the provider identity.
| disabled={disabled} | ||
| loading={models.status === "loading"} | ||
| onConfigure={() => setOpen(true)} | ||
| onChange={(model) => onChange({ ...current, model, effort: "" })} |
There was a problem hiding this comment.
🤖 Optional: when a provider is pinned through Edit configuration, "Use Goose default" clears only the model ({ ...current, model: "", effort: "" }). The agent then gets that provider's default, not Goose's. Clear the provider too, or label the item "Use provider default" while a provider is pinned.
| const [error, setError] = useState(""); | ||
| return ( | ||
| <> | ||
| <ModelPicker |
There was a problem hiding this comment.
🤖 Optional: this control dropped #1177's state.modelsChecking presentation. Main passes it to the picker as loading and uses it to disable the control. While a check or retry runs, the row shows no loading state. Pass loading={state.modelsChecking}, as the Codex row does.
There was a problem hiding this comment.
🤖 Still present at 71ccfaed (optional). ModelDiscoveryRecovery gets loading={state.modelsChecking}, but that only affects its retry button. The picker itself stays enabled during initial discovery.
| establish working, typing, or online status. | ||
|
|
||
|
|
||
| ### Model-menu design exception |
There was a problem hiding this comment.
🤖 Optional: this exception has two gaps against DESIGN.md.
- The Goose Settings per-agent Model (
goose/tabs.tsx:422-445) also uses the family Menu, but with a leading Default radio and no reset. That falls outside "AI defaults and Goose creation controls" and contradicts "without duplicating the current model as the first option". - DESIGN.md records its other exceptions in DESIGN.md itself. Add a pointer from its picker table (
DESIGN.md:191-195).
Co-authored-by: fin <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz> Signed-off-by: klopez4212 <klopez4212@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62a187c93f
ℹ️ 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".
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s GitHub account.
Reviewed head 62a187c93ff73e9d584506e0878d68d5255d9ecb against base 0b92951c53bdd4aad0776f93f8f454bdd0cb97ce, focusing this follow-up on fixes since bd6ab479 and their affected callers.
Changes still needed; 8/10. The inheritance/effort fix, visible Default labels, Settings credential refresh, and removal of Goose’s ineffective device reset check out in source. The documented text-only provider/family-icon decision is preserved.
- One additional P2 is inline: retrying discovery removes the focused recovery button in the Claude and Goose defaults rows.
- The existing Create Agent discovery-recovery finding remains valid at this head; it is referenced here instead of duplicated.
Coverage: Star Lord integration; Rocket systems/performance/lifecycle; Mantis correctness/security/concurrency; Groot product/accessibility/recovery flows. Shared owners reviewed include ModelPicker/ModelFamilyMenu, Menu, Combobox, Dialog and PreferenceRow. Public description, changed material and commit messages inspected; the current description has no attached images. No disclosure finding.
Source-only: no tests, builds or app execution performed. One existing CI snapshot showed 30 successful checks and two skipped checks (Windows native and browser flake check), including successful CI required. Native provider/defaults-to-session behavior, rendered keyboard/focus and screen-reader behavior, both themes/narrow layouts, performance comparison and human acceptance remain unverified. This COMMENT review is not approval or merge authorization.
Co-authored-by: fin <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz> Signed-off-by: klopez4212 <klopez4212@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2e842f44a
ℹ️ 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".
kalvinnchau
left a comment
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 AI agent review at b2e842f. Changes needed: provider-only defaults can break inherited Goose sessions, and defaults refresh retains stale effort choices (inline). The focus regression is already tracked in #1266 (comment) and is not duplicated. Targeted tests and isolated protocol checks were performed; actual app configuration flows and the full suite remain unverified. This is not approval.
| await this.with((goose) => | ||
| goose.request("_goose/unstable/defaults/save", { | ||
| providerId: choice.provider.trim(), | ||
| ...(choice.model.trim() ? { modelId: choice.model.trim() } : {}), |
There was a problem hiding this comment.
🤖 AI agent review — [P1] Save a valid model when switching default providers
In the AI defaults dialog, choosing a different provider clears the model while model selection is hidden. This request therefore omits modelId. For a newly configured provider without a stored model, pinned Goose writes an empty model; readiness still becomes ready, but inherited sessions cannot resolve a model. An isolated protocol reproduction (cached Goose 4dea9b48, with the relevant handlers byte-identical to pinned 9686d674) configured Ollama, saved provider-only defaults, read modelId:null, and received -32603 / Failed to resolve model: Configuration value not found: GOOSE_MODEL from session/new. Send the provider's listed defaultModel when switching without a model, or expose model selection; require a usable model before reporting ready. This differs from the previous same-provider reset issue.
There was a problem hiding this comment.
🤖 Confirmed at 71ccfaed (blocker). We checked this against pinned Goose 9686d674 source:
- The defaults dialog hides the model (
tabs.tsx:283,hideModel = deviceDefaults), and a provider switch resetsmodel: "".saveDefaultstherefore omitsmodelId. - Goose then saves that provider's stored model, or
""(acp/server/config.rs:223-230). Key-only providers (anthropic, openai) never get a stored entry on credential save (acp/server/providers.rs:1033-1047). get_active_modelthen falls back to the legacy flatGOOSE_MODEL(config/providers.rs:75-87). The result is a stale model from the old provider, or "Failed to resolve model".- Meanwhile
check()reportsready, because it only checks that the provider is configured (setup.ts:132-135). The row also shows the catalogdefaultModel(tabs.tsx:168-171), so the UI looks fine. - The new
setup.test.tscase only covers a provider that already has a stored model.
Fix: when the provider changes in ProviderDialogBody (tabs.tsx:234), seed the draft with the new provider's catalog defaultModel. Or show the model picker for device defaults. Add a test for a provider with no stored model.
| ); | ||
| this.defaultsChanged(); | ||
| this.checkedDefaults = undefined; | ||
| await this.check(); |
There was a problem hiding this comment.
🤖 AI agent review — [P3] Invalidate cached effort choices when defaults change
This check refreshes listing/live caches but leaves thinking cached. An inherited agent requests efforts with blank provider/model, so its cache key remains unchanged after the device default model changes. Reopening Settings can therefore display the old model's levels and default label; selecting a level unsupported by the new model can fail at set_config_option thinking_effort. Clear the thinking cache during the defaults refresh, as credential saving already does. This is source-traced; the resulting runtime rejection was not reproduced.
There was a problem hiding this comment.
🤖 Confirmed at 71ccfaed by probe. Calling efforts({provider:"",model:""}) before and after saveDefaults + check() returns the same cached promise. ThinkingField's key changes only on credential save, so an inherited agent's Thinking levels stay stale until a restart. Clear thinking in check() as well, as save() already does.
There was a problem hiding this comment.
🤖 AI agent review correction at b2e842f: the duplicate P1 comment from this review has been removed. Canonical finding: #1266 (comment) . Validation includes an isolated protocol reproduction with cached Goose 4dea9b48 (relevant handlers reported byte-identical to pinned 9686d674), plus targeted tests. The pinned binary and full-app configuration/session workflow were not executed; the full suite remains unverified. Changes remain needed; this is not approval.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s GitHub account.
Reviewed head b2e842f44a75def0ef13b5829579dac6ce587858 against base 0b92951c53bdd4aad0776f93f8f454bdd0cb97ce. Follow-up scope: the six-file repair since 62a187c93ff73e9d584506e0878d68d5255d9ecb, affected callers, and new material evidence below.
Changes still needed; 8/10. The prior discovery-retry focus finding and missing Goose Create Agent recovery are repaired in source. The focused retry survives loading and repeated failure, transfers focus to the model picker after success, and does not reclaim focus after the user tabs away. No new actionable defect found in those repairs.
One separate P2 remains: the existing defaults dialog return-focus finding. Goose’s successful Save defaults calls check(), which changes readiness from ready to checking and back. AgentDefaults.tsx:106–122 replaces the original trigger with the checking button, then clears its shared ref when that button unmounts; the newly mounted model trigger does not repopulate it. ProviderDialog.tsx:82 therefore resolves no current return target on close. Keep the return target synchronized across readiness changes or resolve the current model trigger at close, with a regression for the actual successful-save/readiness cycle. This is new material evidence in an unchanged area, not a regression introduced by the six-file repair; the existing inline report is referenced rather than duplicated.
Coverage: Star Lord integration; Rocket systems/performance/lifecycle; Mantis correctness/security/concurrency; Groot product/accessibility/recovery flows. Shared owners include ModelPicker/ModelFamilyMenu, Button, Dialog and its focus helper; the documented provider/family-icon choice is preserved. Public PR description and repair commit inspected; no attached images or disclosure finding in those surfaces.
Source-only: no local tests, builds or app execution. One hosted CI snapshot had 28 successful checks, two skipped checks (Windows native and browser flake check), and WebKit 7/9 still running. Native provider/defaults-to-session behavior, rendered focus/screen-reader behavior, both themes/narrow layouts, comparative performance/test cost and human acceptance remain unverified. This COMMENT review is not approval or merge authorization.
Co-authored-by: Terp <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz> Signed-off-by: klopez4212 <klopez4212@gmail.com>
Co-authored-by: Terp <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz> Signed-off-by: klopez4212 <klopez4212@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71ccfaed3d
ℹ️ 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".
| check = (): Promise<void> => { | ||
| if (this.checking) return this.checking; | ||
| this.listing = undefined; | ||
| this.live.clear(); |
There was a problem hiding this comment.
When a successful defaults save or an external goose configure change follows a cached setup.efforts({ provider: "", model: "" }) call, this refresh clears the provider/model caches but leaves thinking intact. ThinkingField subsequently receives the same cached promise for its blank inherited choice—even after remounting—so it can show effort levels from the previous default model and let the user save an option the new model does not support. Clear the thinking cache whenever defaults are freshly checked.
AGENTS.md reference: AGENTS.md:L126-L129
Useful? React with 👍 / 👎.
| size={17} | ||
| aria-hidden="true" | ||
| className="motion-safe:animate-spin" | ||
| state.readiness === "ready" && DefaultControl ? ( |
There was a problem hiding this comment.
When a keyboard user selects a Goose model from the inline defaults menu, saveDefaults() runs check(), producing a ready → checking → ready transition. This conditional therefore unmounts the focused DefaultControl, temporarily replaces it with the checking button, and then mounts a new control without transferring focus, leaving focus on the document body after every successful inline model change. Keep the trigger mounted through the recheck or explicitly focus the remounted trigger, as the dialog-close path already does.
AGENTS.md reference: AGENTS.md:L46-L50
Useful? React with 👍 / 👎.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s GitHub account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/concurrency), and Groot (product/accessibility) lanes.
Reviewed head 9d11c915d4ea56d86d72b7ec2ac6d584afefed73 against base 0b92951c53bdd4aad0776f93f8f454bdd0cb97ce. Scope: the three-file repair since 71ccfaed3db9ed851a7af78022b4ab4dd48087c6, affected callers, and pinned Goose 9686d674 protocol/persistence source.
Changes needed; 8/10. The blank-model save/readiness problem is addressed for catalog-backed providers, but the repair introduces one P2: switching back to a provider overwrites its saved custom model with the catalog default (inline below).
Two existing findings remain applicable and are linked rather than duplicated:
- Stale inherited effort choices:
check()still clears listing/live caches without invalidatingthinking; inherited blank provider/model keys can retain the previous default’s choices. - Inline-save focus loss: the successful save’s ready → checking → ready transition unmounts the inline model control. The remounted trigger ref supports dialog return focus, but the inline path has no corresponding focus handoff. This is separate from the repaired dialog-close path.
Validation: immutable source and test inspection only; no tests, builds, app launches, or PR code executed. The one-time exact-head hosted snapshot still had JavaScript, Rust, browser journeys and measurements running; completed checks were successful, with Windows/browser-flake lanes skipped. Public description and both follow-up commits inspected; no attached images or actionable disclosure finding in those surfaces. Native Goose provider-switch → session behavior, rendered focus/accessibility, themes/viewports, test-cost timing and human acceptance remain unverified. This focused review is not an approval or merge authorization.
| (providerId === defaults.provider ? defaults.model?.trim() : "") || | ||
| providers | ||
| .find((provider) => provider.id === providerId) | ||
| ?.defaultModel.trim(); |
There was a problem hiding this comment.
🤖 [P2] Preserve the destination provider’s saved model before using its catalog default.
When a provider is selected in the device-defaults dialog, tabs.tsx:390–391 clears the draft model, and that dialog hides model selection. If A previously used a custom model, switching A → B → A now takes this catalog fallback because A is no longer the active provider. The subsequent explicit modelId overwrites A’s saved choice without the user selecting a replacement model.
Pinned Goose uses the stored provider model when modelId is omitted, while set_active_provider replaces the saved entry with a supplied model. The app’s fake models the same per-provider persistence. The new regression test covers a provider with no stored model, not a return to one with a custom model.
Preserve the destination’s stored choice and use catalog fallback only when it has none; add A(custom) → B → A coverage. The current inventory exposes catalog metadata, so the repair may need a Goose protocol change to expose/resolve the stored model. Simply restoring unconditional omission would reintroduce the no-model bug.
Co-authored-by: Terp <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz> Signed-off-by: klopez4212 <klopez4212@gmail.com>
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s GitHub account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security), and Groot (product/accessibility) lanes.
Reviewed head 107cf0e2ea536591dd93e07ae026553df3469485, base 0b92951c53bdd4aad0776f93f8f454bdd0cb97ce. Scope: the three-file follow-up since 9d11c915d4ea56d86d72b7ec2ac6d584afefed73, affected callers, and pinned Goose 9686d674 protocol/persistence source.
The custom-model overwrite reported in the preceding review is addressed in source: a provider switch now reads its saved model before falling back to catalog metadata. The request shape and stored providers[id].model match the bundled Goose pin. The new test covers switching away and back after recreating GooseSetup. No new actionable defect found in this repair.
Changes still needed; overall 8/10. Two earlier findings remain in unchanged code, linked without duplicate inline comments:
- Stale inherited effort choices:
GooseSetup.check()clears listing/live caches but retainsthinking, so inherited blank provider/model keys can return the previous default’s effort choices. Invalidate those cached choices when defaults change. - Inline-save focus loss: successful inline save calls
check(), and the ready → checking → ready transition unmounts the control inAgentDefaults. Dialog return-focus handling does not restore focus for this inline path. Preserve the control or provide a guarded focus handoff.
Validation: source and test inspection only; no tests, builds, app launches, or PR code executed. The one-time exact-head CI snapshot still had JavaScript, Rust, browser journeys/measurements and Vitest flake checks running; completed checks succeeded, with Windows/browser-flake lanes skipped. The public PR description and follow-up commit were inspected; no attachments or actionable disclosure findings in those surfaces. Native/live provider switching, rendered keyboard focus and accessibility, themes/viewports, test-cost timings, and human acceptance remain unverified. Config-read failure and malformed-value cases lack targeted new regression coverage; this is a test limitation, not a demonstrated new production defect. This focused source review is not approval or merge authorization.
Co-authored-by: Terp <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz> Signed-off-by: klopez4212 <klopez4212@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12d241acfc
ℹ️ 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".
| defaultsChanged() { | ||
| this.defaultsRevision++; | ||
| for (const entry of this.agents.values()) | ||
| if (!entry.config.provider.trim()) entry.sessions.reconfigure(); |
There was a problem hiding this comment.
When an agent has an explicit provider but model: "", changing that provider's Goose default model still affects the agent: defaults/save updates the provider's stored model, while an already-open GooseSession retains the model selected when it opened. This condition skips that agent, and the fingerprint at line 532 also excludes the defaults revision, so later turns continue using the old model until another setting reopens the session. Fresh evidence beyond the earlier inherited-defaults finding is that the new invalidation explicitly handles only a blank provider even though a blank model independently inherits provider configuration; invalidate sessions whose model is blank and include the revision in their fingerprint.
AGENTS.md reference: AGENTS.md:L126-L129
Useful? React with 👍 / 👎.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s GitHub account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/accessibility) lanes.
Reviewed head 12d241acfc171c7081b3ad4e37fdab0c01793670, base 0b92951c53bdd4aad0776f93f8f454bdd0cb97ce. Scope: the five-file follow-up since 107cf0e2ea536591dd93e07ae026553df3469485 and affected callers.
The prior inherited-effort cache finding is addressed in source by clearing thinking during check(). The inline-control unmount finding is also addressed in source: the control survives ready → checking → ready. Existing shared Menu/Dialog ownership and the documented model-menu design exception are preserved.
One new P2 regression, detailed inline; assessment 8/10. Retaining the control also retains its model-discovery result after a same-provider configuration save. Refresh that read without remounting the control.
Validation gap: the trigger is disabled during saving/blocking. The jsdom focus assertion does not establish real browser/webview focus restoration under slow saves, failures, or discovery. We have not confirmed a remaining native focus defect and are not reporting that possibility as another required change.
Source and test inspection only: no tests, builds, app launches or PR code executed. The one-time hosted CI snapshot showed a failed Vitest flake check (cause not assessed), JavaScript/Rust/browser jobs still running, and Windows/browser-flake lanes skipped. Native defaults-to-session behavior, rendered keyboard/accessibility checks, themes/viewports, test-cost timings and human acceptance remain unverified. Public description and follow-up commit inspected; no attachments or actionable disclosure finding. This is a focused follow-up, not an exhaustive feature re-audit, approval or merge authorization.
| const [listing, reload] = useRead( | ||
| () => setup.providers(), | ||
| `${state.readiness}:${!!state.blocking}`, | ||
| "providers", | ||
| ); |
There was a problem hiding this comment.
🤖 [P2] Refresh model discovery when the retained defaults control rechecks
Keeping DefaultControl mounted fixes the focus-reset mechanism, but this change refreshes only the provider/default listing. The models read immediately below remains keyed only by provider ID (lines 152–156). If a user opens Edit configuration and changes credentials or the host for the same provider—for example another Databricks workspace—saveDefaults() clears GooseSetup’s live cache and runs check(), yet this mounted useRead never calls setup.models(id) again. The menu continues offering the old endpoint list and omits the new workspace’s models until the settings control remounts; a successful old result exposes no retry button. Previously the readiness transition remounted the control and refreshed discovery. Invalidate/reload the model read on a completed configuration refresh as well, preserving its provider scope and the stable trigger. Add a mounted regression where a same-provider credential/host save changes the discovered models.
Co-authored-by: Terp <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz> Signed-off-by: klopez4212 <klopez4212@gmail.com>
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (lifecycle/performance), Mantis (correctness/concurrency), and Groot (product/accessibility) reviews.
Reviewed head e9e354c1454f639314be6fde78d6abc2cc4b9c3d, base 0b92951c53bdd4aad0776f93f8f454bdd0cb97ce. Scope: the two-file follow-up since 12d241acfc171c7081b3ad4e37fdab0c01793670 and affected callers, not a fresh audit of the whole feature.
The previous stale-model-discovery finding is fixed in source: readiness/blocking transitions now invalidate the mounted read after configuration changes. One introduced P2 is detailed inline. Assessment: 8/10 until the slow-discovery return-focus path is preserved.
Source-only: no tests, builds, app launches or PR code execution. The added regression test was inspected, not run. Public description and follow-up commit were inspected; no attached images or actionable disclosure finding. The single hosted-CI snapshot had JavaScript, Rust, browser journeys/measurements and Vitest flake checks in progress; completed non-skipped checks were successful, Windows and browser flake checks skipped. Native/live provider behavior, rendered keyboard/screen-reader behavior, both themes, viewport/zoom coverage and human acceptance remain unverified. This COMMENT is not approval or merge authorization.
| const [models, reloadModels] = useRead( | ||
| () => (id ? setup.models(id) : Promise.resolve([] as ModelChoice[])), | ||
| `${id}:${state.readiness}:${!!state.blocking}`, | ||
| id, |
There was a problem hiding this comment.
🤖 [P2] Preserve return focus while the refreshed model list is pending
Reloading on readiness/blocking fixes discovery, but it now disables the return-focus target on a same-provider dialog save. If the supported-model request is slower than dialog dismissal, saveDefaults() finishes its provider check and ProviderDialogBody calls onClose() (lines 266–267) while this read is still loading. ModelPicker passes disabled || loading to ModelFamilyMenu (ModelPicker.tsx:242), whose trigger renders a native disabled button; the pinned Base UI 1.8.0 MenuTrigger/useButton implementation confirms that behavior. ProviderDialog tries to return focus to that button once, so focus instead falls out of the closed dialog. Finishing discovery does not restore it: ModelDiscoveryRecovery only handles focus from its own Try again button. Keyboard and screen-reader users lose their position after saving.
Keep the pending trigger focusable while preventing activation, using the shared pending-control pattern, or provide a guarded deferred return-focus handoff that respects a subsequent user focus move. Add a regression with discovery deferred beyond dialog close and verify focus before/after it settles. The existing fake responds immediately and does not cover this ordering. This follows DESIGN.md’s modal requirement to preserve return focus.
Co-authored-by: Terp <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz> Signed-off-by: klopez4212 <klopez4212@gmail.com>
Co-authored-by: Terp <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz> Signed-off-by: klopez4212 <klopez4212@gmail.com>
Co-authored-by: Terp <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz> Signed-off-by: klopez4212 <klopez4212@gmail.com>
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/concurrency/security), and Groot (product/accessibility) reviews.
Reviewed head 8188fbfbbaec38744b935c97bacb202c75d1236e, base 0b92951c53bdd4aad0776f93f8f454bdd0cb97ce. Scope: the four-file follow-up since e9e354c1454f639314be6fde78d6abc2cc4b9c3d and affected callers.
No actionable changes requested for this follow-up; source assessment 9/10. The prior slow-discovery return-focus finding is addressed: ModelPicker separates loading from hard-disabled state, and ModelFamilyMenu uses the shared Button’s focusable loading behavior while MenuTrigger remains inactive. This keeps the dialog’s return target focusable without introducing a deferred refocus that could steal a later user move. The existing error/retry focus owner is unchanged. The Goose regression now holds discovery past dialog close and covers both retained focus and moving elsewhere; Codex covers inactive keyboard/pointer activation during recheck. Tests were inspected, not executed.
This is source-only follow-up review, not a new whole-feature audit or runtime acceptance. No tests, builds, app launches or PR code were executed. The one-time exact-head hosted-CI snapshot had JavaScript, Rust, browser journeys/measurements and Vitest flake checks still running; completed non-skipped checks succeeded, with Windows and browser-flake lanes skipped. Native/live defaults-to-session behavior, browser/webview focus and screen-reader behavior, both themes, viewport/zoom checks and human acceptance remain unverified. The public PR description and three follow-up commits were inspected; no attached images or actionable disclosure finding in those surfaces. This COMMENT is not approval or merge authorization.
Co-authored-by: Terp <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz> Signed-off-by: klopez4212 <klopez4212@gmail.com>
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord automated source review via Wes’s account.
No actionable findings in this follow-up. 9/10 for minimalness, elegance, and correctness.
Reviewed head 9befbbeb50fbee8f638e91c7d490a8b6b2bd1116, base 0b92951c53bdd4aad0776f93f8f454bdd0cb97ce. The direct comparison with previously reviewed 8188fbfbbaec38744b935c97bacb202c75d1236e changes only src/bundled/claude-code/tabs.test.tsx (+5/-1); production code is unchanged. This uses the small unchanged-follow-up solo exception, not a fresh review of the entire feature.
The retry test now observes a new discovery call and the pending aria-disabled state before checking focus and absence of native disabled. The existing explicit promise gate, finally release, failed retry, and successful focus handoff remain intact. No browser cases or assertions were removed. Public PR description and follow-up commit inspected; no attachments or actionable disclosure found.
Source-only: no tests, builds, or app execution. One hosted-check snapshot had 23 checks in progress, six successful and two skipped; this is not a CI pass or runtime acceptance. Completed test counts, wall time, summed execution time, and slowest-test evidence were unavailable in that snapshot, so no test-cost or flake-elimination claim is made. Native/browser focus, visual accessibility, live provider integration, and human acceptance remain unverified. This COMMENT review is not approval or merge authorization.
Co-authored-by: Terp <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz> Signed-off-by: klopez4212 <klopez4212@gmail.com>
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord automated source review via Wes’s account.
No actionable findings in this follow-up. 9/10 for minimalness, elegance, and correctness.
Reviewed head 4621ad2e9cb29fd4b63af74ab9bf642857d153fb, base 0b92951c53bdd4aad0776f93f8f454bdd0cb97ce. Direct comparison with previously reviewed 9befbbeb50fbee8f638e91c7d490a8b6b2bd1116 changes only five added lines in src/bundled/claude-code/tabs.test.tsx; production code and the base are unchanged. This uses the small unchanged-follow-up solo exception, not a fresh review of the whole feature.
The test now awaits the actual plugin-startup update promise, including its final readiness check, before rendering the recovery control. This establishes the lifecycle boundary that could otherwise overlap discovery retries. The explicit discovery promise gate, finally release, pending-state assertions, failed-retry focus retention, and successful focus handoff remain intact. No assertions or browser cases were removed. Public PR description and follow-up commit inspected; no attachments or actionable disclosure found.
Source-only: no tests, builds, or app execution. One hosted-check snapshot showed 23 checks in progress, six successful and two skipped. Completed test counts, elapsed wall time, summed test execution time and slowest-test evidence were unavailable in that snapshot; no test-cost or flake-elimination claim is made. Real browser/native focus, visual accessibility, live provider integration and human acceptance remain unverified. This COMMENT review is not approval or merge authorization.
Co-authored-by: Terp <5856f6900e8e0a4e0ea1f4f8d2ee33e4753d2119bc122f94ecafc660b46c0511@buzz.block.builderlab.xyz> Signed-off-by: klopez4212 <klopez4212@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd7bfe534c
ℹ️ 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".
| (current.provider | ||
| ? listing.value?.providers.find((item) => item.id === provider) | ||
| ?.defaultModel | ||
| : listing.value?.defaults.model) || ""; |
There was a problem hiding this comment.
When a draft has an explicit provider but an inherited model, this uses the provider catalog's defaultModel, even though Goose stores each provider's selected model separately (as saveDefaults() now acknowledges via _goose/unstable/config/read). If that stored choice is a custom model, the new Create control displays the wrong model and queries thinking levels for it, while the resulting agent actually runs the stored custom model. Resolve the provider's persisted model before populating the picker and effort lookup.
AGENTS.md reference: AGENTS.md:L126-L129
Useful? React with 👍 / 👎.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/accessibility) reviews.
No required changes found in this merge follow-up. Source assessment: 9/10.
Reviewed head bd7bfe534c9ba716d3874b2eca5c44a3b19f675c, base c97a3bc1a3cc410ac90651c2b41f527dae46ba98, against previously reviewed first parent 4621ad2e9cb29fd4b63af74ab9bf642857d153fb. The feature patch survives the merge; the manual conflict resolution removes retired Channel Usage documentation while preserving the model-menu exception. Integration checks covered the Goose storage/tools constructor arguments, session invalidation alongside persisted conversation timestamps, and prior discovery/retry/return-focus repairs. This is a focused follow-up, not a fresh whole-feature audit.
The stored-model premise in r4316539998 and r4308419502 does not hold for the reviewed explicit-provider path. Buzz passes that provider in session/new metadata. Pinned Goose 9686d674 resolves an explicit provider through resolve_provider_default_model_config, whose implementation uses registry metadata.default_model. Stored device-default resolution is a separate path. The catalog fallback in this preview therefore matches the pinned session creation behavior; these comments do not establish a required fix.
Optional wording: docs/agents.md:100,746 still calls the settings section “AI defaults”; main now labels it “Providers”. Aligning that wording would improve navigation guidance.
Source-only: no tests, builds, app launches or PR code executed. One exact-head hosted-check snapshot showed 30 successful checks and two skipped (Windows native validation and browser flake checks). This does not establish native/live defaults-to-session behavior, rendered focus, screen-reader behavior, themes/viewports, performance measurements by this review, or human acceptance. Public PR description and merge commit inspected; no attachments or actionable disclosure finding. This COMMENT is not approval or merge authorization.
* origin/main: Add native Huddles with call controls and conversation panels (#489) fix(claude-code): keep sessions alive while Claude's background work runs (#1102) Rename Agent setup "Providers" to "Harness defaults" (#1348) feat(plugins): feature core plugins and branded catalog browsing (#1246) Improve AI defaults and add Goose configuration (#1266) feat(sidebar): filter channel sections by unread and activity (#1297) feat(plugins): organize settings with guides and focused controls (#1176) Unify Solo chat menus, rename workspaces, quieter membership notices (#1326) Add identity restore UI (#1101) Add native identity backup recovery (#1107) Back up desktop identities only with saved consent (#1106) Add native identity backup consent hooks (#1137) remove the Channel Usage plugin (#1335) fix(messages): retain image geometry across virtualized remounts (#1271) fix(channels): read metadata for a labeled channel whose roster came first (#1289) fix(channel-members): allow adding members to system channels (#1288) fix(unread): recover dropped thread roots for unread thread activity (#1291) test(voice-notes): fix the draft-card race that turned main red (#1315) ci: check dependency policy with cargo-deny (#1321) Signed-off-by: jtennant <jtennant@squareup.com> # Conflicts: # src/bundled/inbox/InboxDetail.test.tsx # src/bundled/inbox/InboxDetail.tsx
…x-session-preview-live * origin/jt/inbox-page-tabs: (25 commits) refactor(inbox): open conversations through conversation.ui.Tabs fix(inbox): retire a Huddle room's retained tabs and keep their focus test(inbox): stop depending on Channels having loaded a thread's channel fix(inbox): close an opened conversation once its room becomes a Huddle Add native Huddles with call controls and conversation panels (#489) fix(inbox): offer no Archive shortcut while Drafts is shown fix(inbox): keep a restored tab's row selected after leaving Inbox fix(claude-code): keep sessions alive while Claude's background work runs (#1102) Rename Agent setup "Providers" to "Harness defaults" (#1348) feat(plugins): feature core plugins and branded catalog browsing (#1246) Improve AI defaults and add Goose configuration (#1266) feat(sidebar): filter channel sections by unread and activity (#1297) feat(plugins): organize settings with guides and focused controls (#1176) Unify Solo chat menus, rename workspaces, quieter membership notices (#1326) Add identity restore UI (#1101) Add native identity backup recovery (#1107) Back up desktop identities only with saved consent (#1106) Add native identity backup consent hooks (#1137) remove the Channel Usage plugin (#1335) fix(messages): retain image geometry across virtualized remounts (#1271) ...
3 participants
Adds Goose-owned AI defaults and inline model menus for Goose, Claude, and Codex, with family icons, clean Databricks labels, and supported effort choices. Create Agent uses the same picker and searchable provider configuration.
Preserves inherited models when changing effort, labels inherited choices, refreshes Goose controls after credential saves, and supports discovery retry and inherited-session refresh. Per-agent reset actions restore inheritance; the ineffective Goose device reset is removed.
Owner-approved design: provider dropdowns are text-only and model chips show only the selected family icon. Removing the Goose/provider marks from #1260 is intentional.
Validation: 206 affected tests, typecheck, CI lint, and normal push checks pass on the blocking-fix update. Native visual and live defaults-to-session verification remain pending.