Skip to content

Restore OMP thinking choices for non-running models - #36158

Merged
brennanb2025 merged 5 commits into
mainfrom
brennanb2025/sta-11790
Oct 30, 2026
Merged

brennanb2025 merged 5 commits into
mainfrom
brennanb2025/sta-11790

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 5 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​429 $\color{#cf222e}{\Huge{\mathbf{−}}}$​12 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​417
Prod 6 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​58 $\color{#cf222e}{\Huge{\mathbf{−}}}$​8 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​50

Choosing an OMP model other than the running session’s model could hide the thinking-level selector. Orca discarded OMP’s native choices when the selected model’s own menu was still unknown. A newer unknown catalog observation could also replace an older known menu.

Restore the native choices in the existing picker for listed non-running OMP models without inventing their default effort. Keep known menus, including known-empty menus, over unknown suggestions. OMP resolves selected native levels for the target model; stricter agents keep their existing behavior. Model-before-effort writes and explicit native rejection handling remain unchanged.

tracked in STA-11790

Full review path: this changes catalog merge semantics and is a second fix in the ACP projection mechanism. The pre-code plan and review panel are complete; two correctness rounds found no production defect. Review repairs add persistence, phone and measured scaling contracts. Production changes remain 28 lines.

Intended differences from the common pattern: retain the existing model-row payload for independently updated desktop/mobile clients and hosts. Suggested menus remain unknown capability and are supplied only when the dialect declares native acceptance of unlisted levels. Preserve explicit rejection of user selections and avoid discovery writes that alter provider usage. Explicit menus and defaults retain their provenance; unlisted/model-less behavior is unchanged.

Validation on the final test-only head: projection 14/14, persisted/resting-picker contracts 10/10, desktop picker 30/30, phone options 29/29, and retained adapter/host contracts 56/56 passed. Recorded real OMP config-options output is paired with explicitly synthetic model context. The performance contract drives production read, publication, listing-key and installation at 10 and 100 models, including repeated and changed native menus. Normal pre-commit hooks, static analysis/typecheck and mobile verification passed. All CI checks passed on98e1f2b, including cross-version wire compatibility, persistence, packaging and all selected unit shards.

Real OMP 18.2.1 QA completed in an isolated gated Windows copy. A newly created chat changed from working A/High to unknown B/Low, let A finish naturally, then returned two real B replies. A saved A chat resumed through Session History offered B’s choices before starting its dormant provider, then returned two B replies. Native options confirmed B/low after each of all four replies, with B/low in accepted submissions and no clamping. The pinned baseline offered only read-only High during A work and no selectable alternatives after History resume; the branch offered seven native levels at both boundaries.

QA ran on 8191a87. Later commits 31e981d, b30be79, df0ce54 and 98e1f2b change only tests/fixtures. Recorded ownership checks stayed matched; the driver did not refresh the app identity API immediately before every action. Earlier retention-only and interrupted attempts are excluded from the completed-flow evidence.

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

coderabbitai Bot commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Walkthrough

The OMP dialect now exposes session effort choices for models without advertised effort information while keeping their capability status unknown. Catalog merging selects effort menus together with their known or unknown status. New tests cover menu scaling, persisted catalog evidence, and effort selection in mobile and renderer pickers.

Priority: Normal

Merge Risk: Minimal · up to 98e1f

The reviewed change has no evidenced merge-blocking risk in the supplied context.

Pre-merge checks: Passed 3 · Failed 2

Failed Failed checks: 2 warnings

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 50.00% 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.
Description check Warning The description has substantial technical content. It explains the problem (OMP native thinking choices are discarded for non-running models, and a newer unknown catalog observation can replace an old… Add an ELI5 section in plain language. Replace the 'What Changed' and 'Why' prose with explicit before/after behavior as a user sees it, the mechanism changed, and why this approach beats the alternatives. Replace the STA-11790 reference wi…
Passed Passed checks: 3 passed
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: restoring OMP thinking choices for models that are not currently running.
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.
Full details: Docstring Coverage

Explanation

Docstring coverage is 50.00% 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 2 functions across 11 files.

Resolution

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

Full details: Description check

Explanation

The description has substantial technical content. It explains the problem (OMP native thinking choices are discarded for non-running models, and a newer unknown catalog observation can replace an older known menu), the fix (restore native choices without inventing a default effort, keep known menus over unknown suggestions), and the scope (model-before-effort writes and rejection handling are unchanged). It includes a LoC summary, a tracking reference (STA-11790), validation results, and detailed QA evidence on real OMP 18.2.1. It is not off-topic.

The template has gaps. The ELI5 section is missing. The 'What Changed' and 'Why' sections are merged into prose without the explicit before/after the template asks for, and the 'why over alternatives' point is only partly covered. The 'Linked Issue' section has no 'Fixes #' issue reference; it cites STA-11790 instead, and the template requires an actual linked issue. The 'Visual Proof' section is missing. The description states that a branch screenshot was planned, but no before/after screenshot or video is attached, and the change touches renderer UI (the thinking-level picker). The 'Testing' checkboxes are not ticked, and the manual test checkbox is not clearly marked, although the QA narrative describes manual runs. The 'AI Disclosure' section is blank. The 'Agent skill upstream boundary' checkbox is not ticked. The Notes and Checklist sections are not addressed (for example, Remote SSH, mobile, and cross-platform impact are mentioned only in part). The objectives also say that CI, a screenshot, and live-agent QA were pending, but the description reports them as complete, so the author should reconcile that.

Resolution

Add an ELI5 section in plain language. Replace the 'What Changed' and 'Why' prose with explicit before/after behavior as a user sees it, the mechanism changed, and why this approach beats the alternatives. Replace the STA-11790 reference with a real linked issue using 'Fixes #<number>', or confirm the author is a maintainer who has no issue to attach. Add Visual Proof: a before and after screenshot or video of the thinking-level picker for a non-running OMP model, or write N/A with a reason. Complete the Testing section with reviewer steps and the platforms actually tested, and tick the manual and automated test boxes where they apply. Fill in the AI Disclosure section, or leave it blank only if the author is a StablyAI team member. Tick or address the Agent skill upstream boundary item. Address the Notes items (security, cross-platform, Remote SSH, mobile, backward compatibility, performance) and the Checklist items. Reconcile the status claims: the objectives say CI and live-agent QA were pending, while the description reports CI passed and QA completed, so state the final status clearly.

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

brennanb2025 commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor Author

Choosing another OMP model could remove selectable thinking levels before that model had reported its own menu. A newer unknown catalog observation could also replace an older known menu.

The existing picker now offers OMP’s native choices while keeping an unobserved model’s capability and default unknown. Known menus, including known-empty menus, win over unknown suggestions. Strict agents retain their behavior; native model-before-effort writes and rejection handling are unchanged.

Full review completed two correctness rounds, an architecture challenge, final comparison with the common pattern, readiness and performance passes. No production defect was found. Review fixes add persistence/resting-picker and phone contracts, measured 10/100-model operation/payload bounds, and repair the test import graph. No in-scope findings remain open.

All CI passed on 98e1f2b: 35 successful checks, 16 skipped. Named projection14/14, persisted/resting-picker10/10, desktop30/30, phone29/29 and retained adapter/host56/56 checks passed; these counts overlap. Recorded real native output and existing wire compatibility are covered.

Real OMP18.2.1 ran both target flows on gated Windows: a newly created chat selected B/Low while native A/High worked, let A finish naturally, then received two B replies; a completed A chat closed and resumed through Session History selected B/Low while its provider was dormant, then received two replies. B was unknown with seven selectable native choices before execution. Accepted submissions and native options confirmed B/low after each of all four replies; no clamping. Earlier retention-only, interrupted and restored-chat setup attempts are excluded.

The pinned baseline offered read-only High during A work and no selectable alternatives after History resume. Its working-A comparison used an A-only prompt in its resumed chat to reproduce the same native input; zero baseline B sends. Functional coverage is complete, with procedural qualifications: ownership checks matched and app identity repeatedly matched, but the driver did not refresh the app identity API before every action; two early baseline setup snapshots also lacked an immediately preceding ownership-file reread, with critical picker evidence rechecked.

For Brennan’s review — whole-window comparisons, with B selected and still unknown:

Input Baseline00de PR8191
A working, saved B Baseline: read-only High Branch: seven selectable levels
History resume, provider dormant Baseline: no selectable levels Branch: selectable native levels before Send
Completed new chat Completed History resume
Two real new-chat B/Low replies Two real resumed B/Low replies

The three judgment points are the native-acceptance declaration, unknown-model projection, and known-menu provenance.

Intended differences from the common pattern: retain existing model-row menus for independently updated hosts/desktop/phone clients, distinguish strict native rejection from OMP resolution, and avoid discovery writes that change provider use. Suggestions never become known capability or invented defaults. No temporary difference.

QA ran on 8191a87. Later commits 31e981d, b30be79, df0ce54 and 98e1f2b change only tests/fixtures. Final review covers98e. All test copies/drivers stopped and protected config hashes are unchanged.

The shared catalog change (which effort menu wins when model listings merge) is a tests row, covered by agent-model-catalog-entry.test.ts and the recorded-output tests.

@brennanb2025
brennanb2025 merged commit a05cb89 into main Oct 30, 2026
51 checks passed
@brennanb2025
brennanb2025 deleted the brennanb2025/sta-11790 branch October 30, 2026 04:26
AmoabaKelvin pushed a commit that referenced this pull request Oct 30, 2026
…6242)

#34592 made the effort picker hide any model whose effort menu is
unconfirmed (effortsKnown: false), which Pi needs because its menus are exact.
#36158 relies on OMP showing exactly such recorded menus for models that are
not running, so the two merged into failing OMP tests on main. The hide now
applies to Pi only; OMP, Claude and Codex offer unconfirmed menus as before.
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