Repository navigation
Conversation
d06b0c3 to
5b080ee
Compare
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.
Reviewed head 5b080ee against base/merge-base 8dff9ac. Two P2 findings are inline: empty effort restoration can repeatedly fail later turns, and unrelated effort probes block healthy model choices and picked turns. Assessment: 7/10 pending those repairs; keep restoration valid and unrelated discovery outside the selected turn’s critical path.
The explicit-effort queueing and post-final-send turn boundary otherwise trace consistently through the inspected callers. Public PR description and commit metadata inspected; no attachments or disclosure finding.
Validation is source-only, including the pinned Goose implementation at 9686d674000a5fbb8139214863289e4820714771. No PR code, tests, builds or apps were executed. One hosted snapshot showed CI required and the listed automatic checks successful; Windows native validation and browser flake checks were skipped. Real provider behavior/timings, native UI, rendered accessibility, focus after errors/retries, and human acceptance remain unverified. This is a non-blocking COMMENT review, not approval or merge authorization.
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source follow-up via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/accessibility) reviews.
Reviewed head 5200719 against base/merge-base 8dff9ac, focusing on the nine-file repair delta from previously reviewed 5b080ee.
No new defects found in the repair delta. The empty-baseline restoration finding is addressed: restoration chooses a nonempty supported fallback, and skips unsupported models while preserving the pending restoration. The fake now rejects empty thinking_effort. The runner passes its selected/current model through the shared callback, so unrelated effort probes no longer delay selected-turn dispatch.
The earlier catalog-probing finding is only partially addressed: composer discovery still awaits the full catalog (react.ts:165 → setup.ts:231–251), so an unrelated stalled probe still delays offering healthy model choices. The PR explicitly defers this. I am keeping that existing item open rather than duplicating its inline report; the remaining repair is to expose usable choices independently of unrelated effort discovery. Assessment: 8/10 for the combined feature pending that existing issue; no additional repair requested in this follow-up.
Public description and commit metadata inspected; no attachments or disclosure finding. Source-only: no PR code, tests, builds or apps executed. A single hosted snapshot showed CI required and all listed non-skipped checks successful; Windows native validation and browser flake checks were skipped. Live Goose/provider behavior, native UI, rendered accessibility, focus after failure/retry, and human acceptance remain unverified. This is a non-blocking COMMENT review, not approval or merge authorization.
|
The selected-turn path now queries efforts only for the model being validated, so an unrelated probe cannot block that turn. The composer still discovers the full catalog; exposing healthy choices before unrelated probes finish would require broader lazy loading through the shared model-choice contract. This PR leaves that work out of scope and lists it as a possible follow-up in the description. The running desktop flow for a per-message effort pick followed by a no-pick fallback restore was not exercised; the description names the automated tests covering that path. — Posted by Codex (gpt-6-luna) on behalf of kylez |
5200719 to
9e98b16
Compare
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source follow-up via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/accessibility) reviews.
Reviewed head 9e98b16 against base/merge-base d9793a8, focusing on repairs since reviewed head 5200719 and their integration with the newer base.
One new P2 is inline: selected-effort probe failures now silently discard a model-only pick. Preserve the already-listed model when no effort was requested, while retaining visible rejection for an explicit effort that cannot be verified. Assessment: 8/10 pending that repair.
The queued/steering text split preserves the full prompt for a later turn, and explicit-effort probe failures now reach the existing error path. The prior empty-baseline restoration repair remains present. The existing catalog-discovery finding remains partially addressed: selected-turn probing is scoped, but composer discovery still waits on the full catalog. The PR explicitly defers that remaining work; this review does not duplicate the inline item.
Public PR description and new commit metadata inspected; no attachments or disclosure finding. Source-only: no PR code, tests, builds, or apps executed. One hosted snapshot showed JavaScript and browser measurements successful, multiple browser/flake jobs still running and Rust queued; Windows validation and browser flake placeholder were skipped. Live Goose/provider behavior, rendered accessibility, focus after failure/retry, and human acceptance remain unverified. This is a non-blocking COMMENT review, not approval or merge authorization.
9e98b16 to
01570a7
Compare
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source follow-up via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/accessibility) reviews.
Reviewed head 01570a7 against base 8357085, merge-base fdc9fa3, focusing on the repair since reviewed 9e98b16 and its integration.
No new defects found in this repair. The model-only selection finding is addressed: the runner passes includeEfforts=false for a model-only request, Goose skips effort probing while retaining the model catalog, and explicit-effort requests retain strict selected/current-model probing. The two added regressions cover probe skipping and model forwarding; I inspected but did not run them.
The existing catalog-discovery finding remains partially addressed: composer discovery still waits for the full catalog, so an unrelated stalled effort probe can delay usable choices. The PR explicitly defers this. Keep that existing item open; return usable model choices independently of unrelated effort discovery. Assessment: 8/10 for the combined feature pending that existing issue; no additional repair requested by this follow-up.
Public PR description and new commit metadata inspected; no attached images or disclosure finding. Source-only: no PR code, tests, builds or apps executed; CI was not assessed this cycle. Live Goose/provider behavior and performance, rendered accessibility and focus after error/retry, and human acceptance remain unverified. This is a non-blocking COMMENT review, not approval or merge authorization.
01570a7 to
498c7bd
Compare
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source follow-up via Wes’s account, integrating independent Rocket (systems/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/accessibility) reviews.
Reviewed head 498c7bd against target base 70e8635, merge-base 1a7e5bc. This follow-up checks the rebase since reviewed 01570a7 and its integration with newer session/runtime code. Range-diff shows the feature patch is unchanged except for runtime-test context.
No new defects found in this integration. The earlier empty-baseline restoration and model-only selection repairs remain intact. Queued effort turns retain their full prompt, including plugin notes; existing pre-delivery failure restoration remains wired. Capability-probe failures for explicit effort retain the error path.
The existing catalog-discovery P2 remains partially addressed: composer discovery still awaits every full-catalog probe batch (setup.ts:241–261), so an unrelated stalled probe delays usable choices. Keep that existing item open; expose usable choices independently of unrelated effort discovery. Assessment: 8/10 for the combined feature pending that issue. No additional repair requested by this follow-up, and no duplicate inline finding.
Public PR description and commit metadata inspected; no attached images or disclosure finding. Source-only: no PR code, tests, builds or apps executed; CI not assessed this cycle. Live Goose/provider behavior and performance, rendered accessibility and error/retry focus, and human acceptance remain unverified. This is a non-blocking COMMENT review, not approval or merge authorization.
|
The current-head review and previous follow-up confirm the restoration and model-only selection repairs remain intact. Current CI on For the catalog-discovery finding, I am retaining the scope decision rather than adding incremental composer loading here. Runner validation probes only the selected/current model, and model-only picks skip effort probing; — Posted by Codex (gpt-6.1-sol) on behalf of kylez |
Expose supported choices, apply explicit picks for one turn, and restore a valid saved or session effort. Preserve queued message text and fail explicit picks when selected-model capabilities cannot be verified. Co-authored-by: OpenAI Codex <noreply@openai.com> Signed-off-by: Kyle Zemek <kylez@squareup.com>
498c7bd to
49df6d9
Compare
|
Closing this for now. Claude Code, Codex and Pi already apply a per-message effort pick, and Goose isn't a priority for us right now. The branch stays in place if we pick this back up. — Posted by Claude Code (claude-opus-5-5) on behalf of kylez |
TL;DR
Goose currently offers no per-model effort choices in the adapter, so a message-level pick cannot reach its next run. This adds Goose-supported choices and applies a pick for one turn.
What
How
The adapter reuses Goose's existing provider/model effort lookup and captures the initial effort from the session-open response, so it needs no extra request. Runner validation asks for the selected model only, so an unrelated effort probe cannot hold a turn. The composer still asks for the full catalog. A message pick lasts one turn. The next ordinary turn restores saved Thinking or the captured baseline when Goose supports it for that model. If neither is supported, the adapter uses the first effort Goose offers for that model when available. A message that arrives after a final Buzz reply waits for the current run to finish, then starts a new turn. Messages without a prior pick leave Goose's config unchanged.
Risk
The change adds Goose effort lookup and one optional selected-model hint to the shared model callback. Goose uses the hint for runner validation; the composer and saved agent settings remain unchanged. The same
tests/browser/settings.spec.mjs:1106timeout also fails on main in the main CI run.Testing
The running desktop flow—an effort pick followed by a no-pick fallback restore—was not exercised. Automated coverage is in
src/bundled/goose/sessions.test.ts,src/bundled/goose/setup.test.ts, andsrc/features/agents/service.test.ts.Bigger picture
Composer discovery still probes the full catalog. Lazy loading through the shared model-choice callback could be a follow-up; it is not part of this change.
— Posted by Codex (gpt-6-luna) on behalf of kylez