Repository navigation
Choose model and effort for mentioned agents - #856
Conversation
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: c41320fdbc
ℹ️ 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".
| ...(delivery.model ? { model: delivery.model } : {}), | ||
| ...(delivery.effort ? { effort: delivery.effort } : {}), |
There was a problem hiding this comment.
When this conversation already has lane.active, these captured values never reach execute: the active branch sends only turn/steer, while the model and effort are consulted only when starting or resuming a fresh turn. A user who selects a different model or effort while Codex is working therefore has the message steered into the existing turn under its old settings. Queue a picked message as a new turn, or make the picker unavailable while the conversation is active.
Useful? React with 👍 / 👎.
| const changed = JSON.stringify(next) !== JSON.stringify(valueRef.current); | ||
| valueRef.current = next; | ||
| updateDraft(next); | ||
| setPicks({}); |
There was a problem hiding this comment.
When attachment upload or preparation fails after sendInBackground begins, this clears the selected model and effort immediately, but the recovery callback restores only the draft and attachments. The recovered composer therefore looks ready to retry while silently sending the agent at its default settings. Store the captured picks in the recovery record or restore them alongside the captured draft.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 Follow-up fix for this: #887 — the recovery record now carries the captured model/effort picks and restores them alongside the draft and attachments (also covers the user-initiated Cancel path, which recovers through the same callback). No new persistence; picks still do not survive reload by design.
| <Popover.Popup | ||
| className={styles.popup} |
There was a problem hiding this comment.
When several tunable agents are mentioned or text is enlarged, this raw Base UI popup uses local styling with no max-height or scrolling, so lower model/effort controls can extend beyond the available viewport and become unreachable. The shared PopoverPopup already owns viewport bounds, overflow, placement motion, and focus restoration; use it with the appropriate padding rather than recreating the frame locally.
AGENTS.md reference: AGENTS.md:L35-L41
Useful? React with 👍 / 👎.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Changes needed: keep model-discovery failures visible and retryable (new P2 inline). I also corroborated the existing Codex steering, attachment-retry, and popup-bounds findings at this head; linking rather than duplicating them.
Star Lord’s automated source review via Wes’s account. Head ba91a8bf23ab1d70e0d09df3932d31adea6d6e75; base 430392dd1e5dcc23adaf5343e0cac8bb02a96ab4. Source-only: no tests, app/runtime execution, rendered UI inspection, or CI verification. Public diff, commit messages, and PR description checked; the description has no attached images. Non-blocking COMMENT, not approval.
| } catch (error) { | ||
| console.warn( | ||
| `Agent type ${type.key} could not list its models`, | ||
| error, | ||
| ); | ||
| return []; |
There was a problem hiding this comment.
[P2] Preserve a failed discovery state with a retry action
If the model lookup rejects (for example, Codex config/read/model/list times out or the CLI fails to start), this catch returns the same empty result as an agent with no configurable models. With one mentioned agent, ComposerModels then returns null, removing both the gauge and any open popup; the user sees no failure and has no retry control. Editing the message or the runtime clearing its failed cache does not change this effect’s snapshot/types/key dependencies, so discovery is not retried for the unchanged mention. Keep a per-agent error choice mounted and expose retry, preserving any usable choices and popup focus. This follows DESIGN.md’s “States, feedback, and motion” requirement to distinguish failed loading from absence and put recovery beside the failed operation. Add a rejected-lookup → retry-success regression, including keyboard focus.
Mentioning your own Claude Code or Codex agent shows a gauge in the composer. Its popover is an effort slider with the model name in the slider's tab; the name opens a compact menu of the agent's models. The pick rides on that message as `model` and `effort` tags for that agent, and the host honors them only from the owner and only for listed models and levels. - Codex passes the pick to `turn/start` for that turn only. - Claude Code lists models from `initialize`, sends `set_model` and `apply_flag_settings` ahead of the picked turn, and restores its own on the next unpicked one. - The slider opens at the effort an unpicked run uses (Claude Code's session effort, Codex's configured effort or the model's default); moving back to it is no pick. - A configured Codex model its CLI does not list keeps the picker, with the listed models still pickable. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Squashed-from: baxen/model Squashed-head: 053cee0dba93bb946f544c0c87804ceee90a49ef Signed-off-by: Bradley Axen <baxen@squareup.com> Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Composer model controls also use a popup class. Select the completion-specific data-mention variant instead of whichever CSS module loads first. Preserves all token and geometry assertions; reproduced before and passed in Chromium and WebKit after. Co-authored-by: codex00 <6922125dbee102fcf02d2b32441fd56927c1298ac16119167f82f72529d13676@buzz.block.builderlab.xyz> Signed-off-by: Bradley Axen <baxen@squareup.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
kalvinnchau
left a comment
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 Two additional P2 findings: revalidate the live community binding after discovery, and preserve picker state across unrelated run-status updates. Existing findings are not repeated.
| Promise.resolve().then(async () => { | ||
| const pick = await pickFor(job.trigger, agent, type); | ||
| signal.throwIfAborted(); | ||
| return run( |
There was a problem hiding this comment.
🤖 [P2] Revalidate the live community binding after discovery
If the owner disconnects the community while model discovery is pending, this await can resume after disconnect cleanup and still call run using the captured agent/community. The runner signal does not include the community binding, and the Claude runtime can recreate its disposed entry, starting local work after the explicit disconnect. A bounded fake-process check using the production discovery/session/runtime classes and extracted drain logic observed a new process and user message after disconnect; a post-discovery readiness guard prevented both. Revalidate the current binding/generation, agent/type and cancellation before launching, preserving the intended reconnect/queue policy. Add a held-discovery → disconnect → resolve regression that asserts no launch, plus reconnect coverage.
| ] | ||
| : []; | ||
| }); | ||
| } |
There was a problem hiding this comment.
🤖 [P2] Keep resolved choices mounted during unrelated run-status updates
useMentionModels keys its resolved result on the entire Agents2 snapshot. Starting or finishing any owned agent run replaces that snapshot (service.ts:828–837), so resolved?.snapshot === snapshot fails and the picker returns to its loading state until discovery completes again. AgentChoice then unmounts its model menu and effort slider (ComposerModels.tsx:137–143), closing an open menu and losing slider focus even when the mentioned agent configuration has not changed. Key discovery on the relevant agent/configuration identities, or retain resolved choices while refreshing and reserve the loading replacement for the initial lookup. Add a regression that keeps an open picker/focused slider stable while an unrelated agent starts or finishes.
4 participants
Mentioning your own Claude Code or Codex agent exposes a model/effort control for its next run. The control appears immediately while discovery fills in a loading state. Choices are sent as owner-scoped message tags; an unpicked turn returns to the agent defaults, and unsupported model/effort choices are rejected by the existing service/runtime boundaries.
The picker uses the existing IconButton, Menu and Popover owners with the effort slider. Codex discovery reuses #852’s shared server. Claude discovery reads an existing session or uses a temporary process that is released after reading; it never assigns the shared warm spare to an agent merely to inspect settings. Cached own-model metadata survives a first-turn override.
Validation: 124 runtime/hook tests passed, 268 composer tests passed, TypeScript passed. Independent integration review found two lifecycle/default-discovery defects; both were fixed with regressions. Manual automated-browser harness exercised pending → ready, keyboard focus, slider changes and popup bounds in Chromium and WebKit, both themes, narrow/wide widths and enlarged text (12 combinations). No permanent browser cases added; pending-state assertions use held promises in the existing component tests. Native and human acceptance of the combined stack remain pending.
Browser integration validation
The shared popup token test now identifies the Completions surface by its data-mention variant, so the model picker’s separate popup class cannot change which surface is checked. The failure reproduced before the correction; the full spec passes in Chromium and WebKit (2 cases), with all CSS assertions retained.