Skip to content

fix(native-chat): keep model picker choices stable during discovery - #36113

Merged
brennanb2025 merged 12 commits into
mainfrom
brennanb2025/model-picker-followup
Oct 30, 2026
Merged

brennanb2025 merged 12 commits into
mainfrom
brennanb2025/model-picker-followup

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 23 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​851 $\color{#cf222e}{\Huge{\mathbf{−}}}$​94 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​757
Prod 13 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​141 $\color{#cf222e}{\Huge{\mathbf{−}}}$​53 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​88

Saved model choices and the Model placeholder could disappear while a native chat waited for discovery, and the generic Model placeholder could still open by mouse, keyboard or a command. An in-flight catalog could expose built-in replacement choices before the installed list arrived. Codex could also lose its displayed default effort after its first reply.

The picker now keeps accepted seed and saved choices visible independently of discovery, keeps unknown pending inventory quiet, and uses one readiness result for its opening paths. Phone drawers honor the same descriptor boundary while keeping permissions reachable. Unlisted models get readable labels with the exact literal ID in the existing details. Codex can show a verified model’s declared effort default as presentation context after a reply; that value never becomes an explicit or saved choice.

Tracked in STA-11567, STA-11574 and STA-11620 (display half). The seed-visibility and unknown-catalog intent of #27641 is now covered here; merged #31208 already supplies its remaining ordering and ownership behavior. The legacy controller is not ported. The retired-Claude reset and error-copy work from the plan was already completed by #35266 and is not repeated here. Native OpenCode MiniMax-to-Flint substitution is tracked elsewhere in STA-11727 / STA-11445; this PR does not claim to fix it or acquire execution evidence.

Differences from the common pattern:

  • Intended — execution-host selection owner. The existing host query and finite operation controller remain the single selection owner across desktop, phone and SSH, including independently updated peers. No composer-local selection store or saved field is added.
  • Intended — literal seed input. A known seed stays visible before its row exists and is sent literally through the older Claude-host path, including its retained effort. This restores the shown-choice contract rather than demoting accepted input to a catalog default. Inventory omission never authorizes a replacement or blocks Send.
  • Intended — display context is separate from capture. A verified exact Codex row may supply its declared effort default in the existing default styling. Both canonical capture functions remain unchanged; first-send policy defaults still capture as before, while post-reply presentation adds no input. Missing evidence remains unknown.
  • Intended — descriptive fallback. Existing host labels win. Only synthetic unlisted rows use the shared formatter, with provider context and context-size suffix retained and the exact ID exposed in details. No alias mapping or inferred capabilities are added.
  • Intended — explicit inventory knowledge across mixed versions. The common pattern distinguishes inventory readiness from its rows and the selected value. Orca carries that distinction over its existing independently versioned options/catalog reads with one optional, derived modelsKnown: false field. Required models arrays remain unchanged. A new client retains its existing placeholder or last-good rows on false, still applies current values and permissions, and accepts later discovery even after an earlier live list. Pi’s live startup observation is unknown until its own listing completes; a completed empty live list remains authoritative. Old clients ignore the extra field and retain their existing recovery reads. Old hosts omit it, so new clients retain today’s authoritative-list behavior and recovery reads for those peers. This adaptation supports desktop, phone and SSH without a minimum host version. There is no new RPC, capability, saved data, Send refusal or provider-echo gate.
  • Intended — quiet loading controls. Orca retains its existing disabled Model placeholder rather than adding loading copy or a fabricated Default choice. Known input remains nameable during discovery, and settled model-less agents retain the existing no-model-control behavior. Terminal-unavailable fallback remains supported.

Validation: all selected checks are green for af5ef3d2b431d3bf6f30004a1f94de926524f6a5, including all 12 unit shards, static analysis/typecheck, Mobile, packaging and all three WSL jobs. Mobile CI collected and passed 27 picker tests and 6 first-frame tests. Focused contracts cover recorded Pi listing/state through production parsing, recorded ACP setup through its producer/host/shared boundary, literal dispatch, unknown-versus-empty inventory across versions, recovery reads and counted projection work at N and 10N.

The full review completed three rounds with zero new findings in the last round, plus architecture, whole-mechanism reference, performance and readiness source reviews. One pruning pass removed six redundant cases. Final test diff is +851/-94; production is +141/-53. Remaining cases cover distinct regressions and consumer/cross-version boundaries. Settled panel rulings repaired stale downstream expectations while retaining exact identity, effort, permissions and dispatch checks. Incoming main's launch matrix now preserves all 12 cases and their downstream assertions; its 17 guard plus 12 parity cases passed together, and 35 launcher contracts passed separately. Passing runs overlap and are not summed as unique coverage.

An unrelated inherited SSH upload error-attribution race surfaced in one CI shard. Its source/test bytes match main and the prior green head. The requested --failed retry passed; the underlying P2 remains tracked in STA-13013 and is unchanged here. The terminal display-label/echo concern remains deferred under the panel ruling without a demonstrated failing CLI input.

The QA table types these changes as UI/observation (qa=tests); no live-agent, provider-turn or lifecycle execution verdict is claimed. Desktop screenshots compare identical controlled catalog inputs on main and branch through shipped UI/hooks: loading and settlement, disabled pointer/ArrowDown/requested opens, independent permissions, provisional effort and an actual accepted Low host-write receipt. Native iOS proof uses the installed app's shipped native shell and hash-validated branch page bundle with an isolated synthetic host. It passed disabled opening, saved choice during unknown inventory, independent permissions, consumed /model without later replay and fresh opening after discovery. All owned copies/helpers/Metro stopped and the simulator shut down. Optional folder and readonly-model variations were not exercised in screenshots.

Desktop evidence remains attributed to 8ad5852 versus main d61f8ba; phone evidence is 57c5351. Independent integration checks carry those unchanged flows across tests-only 57c5351/04acf237, the display-equivalent effort optimization 79f95ad, and audited main merges 5e2de508, 02a8d804 and af5ef3d2. The last main merge preserves its fixture refactor and moves the complete repaired matrix into its parity file, with the former effective ask fixture input explicit; all other 35 reviewed paths, including production, are byte-unchanged. The working Mac received no extra mobile installation, standalone lint/typecheck or full suite.

@brennanb2025
brennanb2025 marked this pull request as ready for review October 29, 2026 22:50
@coderabbitai

coderabbitai Bot commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
Walkthrough

The changes track unknown model inventories in structured option results and preserve saved model selections while catalogs load. Model choices for unlisted IDs now use generated labels and retain the original IDs as descriptions. Display snapshots can show a provisional Codex effort default without adding it to canonical send data. Renderer and mobile pickers now gate opening on settable controls and avoid dispatching some repeated selections.

Priority: Normal

Merge Risk: Minimal · up to 57c53

The reviewed changes preserve model choices and prevent invalid picker interactions, with no established merge-blocking risk.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: keeping native chat model picker choices stable during discovery.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description is detailed and directly addresses the change, rationale, scope, testing, visual evidence, linked tracking items, compatibility, and known limitations. It does not use the template hea…
Full details: Description check

Explanation

The description is detailed and directly addresses the change, rationale, scope, testing, visual evidence, linked tracking items, compatibility, and known limitations. It does not use the template headings or checklist format, and it does not provide a conventional Fixes # issue reference, but the required information is mostly present.

Full details: Docstring Coverage

Explanation

Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 18 functions across 35 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Saved or seeded model choices could disappear while a native chat waited for discovery, and an unknown catalog could expose built-in replacement choices. Codex also lost its displayed default effort after the first reply. This change keeps the known choice visible, uses the same readiness rule for mouse, keyboard and requested opens, and gives unlisted choices readable labels with exact IDs. Display-only effort remains separate from accepted input; an explicit Low pick still records the user’s choice.

The full review completed three rounds with zero new findings in the last round. Repairs cover the synthetic host label, provisional equal-value selection, refused phone request replay, and redundant accepted-effort catalog scans. The repeated stale-test failures were resolved under settled full-panel rulings; independent verification confirmed the downstream dispatch, effort, legacy exclusion and PTY checks actually ran. One pruning pass removed six redundant cases. The final diff against merged main is tests +851/-94; production +141/-53. The remaining cases cover distinct regressions, consumers, recorded output or mixed-version contracts. Incoming main also exposed an accepted-seed launch contract mismatch. A second full panel settled a tests-only repair: all twelve launch cases remain, with exact payload, permission, absent replacement mode and downstream settlement assertions. The repair passed the 29-case guard and 35 launcher contracts; main then moved the matrix into a parity file, and both final-content files passed all 29 cases together.

All selected checks are green for final head af5ef3d2b431d3bf6f30004a1f94de926524f6a5, including all 12 unit shards, static/typecheck, Mobile, packaging and three WSL2 distros. Mobile CI actually ran 27 picker and six first-frame tests on the merged head. The earlier synthetic-label repair passed 96 tests in seven focused files. Recorded Pi and ACP output exercises the production observation/host/shared boundaries. Counted projection contracts at N and 10N cover both missing and accepted effort.

Whole-mechanism reference, performance and readiness reviews are complete. The optional derived inventory marker preserves required arrays and old-peer behavior; literal saved choices remain authoritative and are never automatically replaced. The existing execution-host owner and quiet disabled placeholder are intended adaptations of the common pattern. The deferred terminal display-label/echo coupling concern has no demonstrated failing CLI input and remains nonblocking under the panel ruling. Retired OpenCode substitution remains tracked separately in STA-11727 / STA-11445. An inherited SSH upload error-attribution race surfaced in CI and is tracked in STA-13013; the picker PR leaves those source/test bytes unchanged. A --failed retry was requested for the named timing-sensitive SSH test and passed; a retry pass is not a fix for that bug.

The QA table classifies every changed flow as UI/observation (qa=tests): no real-agent or lifecycle run is claimed. Desktop proof below uses the shipped screen/hooks with identical controlled catalog inputs; descriptive catalog fixtures do not establish provider execution. The explicit Low selection has an actual host write receipt. Native iOS proof at 57c5351 uses the installed app's shipped native shell and hash-validated branch page bundle with an isolated synthetic host. It passed disabled opening, saved-choice visibility during unknown inventory, independent permissions, consumed /model requests without replay on discovery, and fresh opening after discovery. All owned app/helper processes stopped and the simulator shut down. A hash-only isolation check found a real Claude config hash change; contents were never read and other hashes were unchanged.

Desktop screenshots are from 8ad5852b00c9050ccec8e03daace559f4726613a, against main d61f8ba92fc42c43c85dee2dc4ee10910e06bfad. They carry across 79f95ad (display-equivalent accepted-effort optimization, 42 focused tests plus independent verification), conflict-free main merge 5e2de508 (zero PR-file overlap), tests-only 57c5351 (96 focused tests), and main integration 02a8d804f11831ea0bef047e89ac23c879c85f0a. The integration resolved five conflicts across 13 overlapping files, passed the required merge hook and focused final-content checks, and received independent AGREE with zero findings; it preserved main's Pi effort/read-refresh work and the reviewed picker behavior. The tests-only repair 04acf237 and main integration af5ef3d2b431d3bf6f30004a1f94de926524f6a5 also carry this evidence. The latter preserved main's fixture refactor and the complete repaired matrix in its new parity file; all 13 PR production files remained byte-unchanged across those two commits. No visual flow changed in those commits. Optional folder variation and a readonly-model state with independently useful effort were not exercised; no pass is claimed for those variations.

For Brennan's review

Loading, main and branch:

main-loading.png

branch-loading.png

Settled, main and branch:

main-settled.png

branch-settled.png

Disabled placeholder and provisional/explicit effort:

branch-placeholder.png

branch-provisional-effort.png

branch-explicit-effort.png

Native phone: loading and saved choice during unknown inventory:

phone-loading.png

phone-saved-unknown.png

Independent permissions, discovery without replay, and a fresh picker open:

phone-permissions.png

phone-settled-no-replay.png

phone-settled-picker.png

Source spots: display-only effort, unlisted host label, phone refused request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant