[Codex effort format] - Step 1: Collapse Codex models to one row per model - #2766
[Codex effort format] - Step 1: Collapse Codex models to one row per model#2766zeroliu wants to merge 1 commit into
Conversation
![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: 74335a9424
ℹ️ 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".
| const effort = selection.effort ?? firstAdvertisedEffort(session, selection.baseModelId); | ||
| await session.applyModelWireId(codexWire.encode({ ...selection, effort })); |
There was a problem hiding this comment.
AgentDefaultModelSetting.onModelChange deliberately persists { effort: null } to mean “let the agent choose,” but this fallback converts that value to the first advertised option—currently low. On the next Codex session, even if codex-acp starts the selected model at high, confirming the saved default sends model[low] and silently lowers reasoning effort. Keep the backend-reported/native effort when null is selected, or make the settings UI require an explicit Codex effort instead of treating the first option as the default.
Useful? React with 👍 / 👎.
| function firstAdvertisedEffort(session: AgentSession, baseModelId: string): string | null { | ||
| const entry = findModelEntry(session.getState()?.model, baseModelId); | ||
| return entry?.effortOptions.find((o) => o.value !== null)?.value ?? null; |
There was a problem hiding this comment.
When codex-acp advertises exactly one entry for a model, such as gpt-6[high], translateBackendState leaves that model's effortOptions empty because suffix efforts are currently derived only for groups with at least two variants. This lookup therefore returns null, and selecting or seeding the model sends the bare gpt-6, which this change notes codex-acp rejects. The decoded effort must remain available even when the catalog contains only one variant.
Useful? React with 👍 / 👎.
| * `configuredModelId`s that `backends.codex.enabledModels` references survive. | ||
| */ | ||
|
|
||
| import { parseCodexModelId } from "@/agentMode"; |
There was a problem hiding this comment.
This value import evaluates the entire @/agentMode barrel whenever the settings migrations load, including its top-level node:os/node:path imports and backend registry. main.ts deliberately imports that barrel only behind isDesktopRuntime() because Node is stubbed on mobile and under app.emulateMobile(true); the new unconditional migration import bypasses that gate and can crash the plugin during module initialization on those runtimes. Move this pure codec behind a Node-free module boundary that migrations can import without evaluating the Agent Mode barrel.
Useful? React with 👍 / 👎.
codex-acp addresses models as `<base>[<effort>]` — its own `set_model`
rejection names the contract ("Expected: modelId[effort]"). Copilot decoded
`<base>/<effort>`, a format codex-acp has never emitted, so every advertised
id decoded as an effort-less base model: the picker listed 34 rows with no
effort dropdown, and discovery enrolled one `ConfiguredModel` per (model ×
effort) pair, giving the Codex settings list one toggle per variant.
Widening `KNOWN_CODEX_EFFORTS` would not have helped — with the wrong
delimiter it never matched anything. The set is deleted rather than extended:
the bracket makes the effort token unambiguous, so Copilot enumerates no
effort levels at all and a CLI release that adds one (`max`, `ultra`, …)
collapses correctly with no plugin change.
Everything downstream was already correct and is untouched —
`translateBackendState` groups by decoded base model and `agentModelDiscovery`
enrolls the translated entries, so both the picker and the settings list
collapse once the codec reads the real format.
Settings v9 migration: rename the persisted per-effort rows in place (rather
than let the next probe's `syncAgentModels` prune them) so the
`configuredModelId`s that `backends.codex.enabledModels` references survive
the upgrade, and lift the bracketed level out of the sticky default's
`baseModelId` into its `effort`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
74335a9 to
2e6a779
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Add a gallery story for the collapsed Codex picker
This changes the picker from separate per-effort Codex rows to a meaningful new visual state—a collapsed model row with a sibling effort dropdown—but the diff adds no story that renders this state. Without the required gallery coverage, the new dropdown and model-specific effort sets cannot be visually verified; add or update the adjacent picker story with collapsed Codex models and differing effort options.
AGENTS.md reference: AGENTS.md:L23-L23
Document the new Codex effort selection behavior
After this upgrade, users see a collapsed Codex model list and a new per-model effort control, but docs/agent-mode-and-tools.md still says only that the picker chooses a model and never explains the effort selector or that available levels vary by model. Update that corresponding user-facing guide so users can understand the new control and persisted selection behavior.
AGENTS.md reference: AGENTS.md:L37-L47
ℹ️ 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".
1 participant
Relates to logancyang/obsidian-copilot-preview#271
Why
Open Settings → Agent → Codex today and the model list is 33 rows:
GPT-5.6-Sol (low),GPT-5.6-Sol (medium),GPT-5.6-Sol (high),GPT-5.6-Sol (xhigh),GPT-5.6-Sol (max),GPT-5.6-Sol (ultra), then the same spread for Terra, Luna, GPT-5.5, GPT-5.4, GPT-5.4-Mini and Codex-Spark. Each carries its own enable toggle, so a user can enable GPT-5.6-Sol athighand not atmax— a distinction that means nothing. The chat model picker shows the same 33 rows and never offers an effort dropdown.The cause is a wire format Copilot got wrong. codex-acp addresses a model as
<base>[<effort>]—gpt-5.6-sol[ultra]— and says so itself when given anything else:Copilot decoded
<base>/<effort>and gated the trailing segment against a hardcodedminimal | low | medium | high | xhighset. Splittinggpt-5.6-sol[ultra]on/yields one segment, so that set was never consulted and every advertised id decoded as an effort-less base model. Widening the set to addmaxandultrawould have changed nothing.Everything downstream already collapses correctly — it was being fed 33 distinct "models."
What
Codex models collapse to one row per base model, in both the settings list and the chat picker, with effort chosen from a sibling dropdown.
Effort levels now come only from what the installed CLI advertises, per model:
Copilot no longer keeps a list of effort names, so a codex-acp release that introduces another level collapses correctly with no plugin change.
On upgrade, a settings migration folds each user's per-effort rows into one row per base model. A base model stays enabled if any of its effort variants was, and a saved default of
gpt-5.6-sol[xhigh]becomesgpt-5.6-solatxhigh. No model disappears from the picker.Non goal
session/set_modelwith the bracketed id. It also advertises acategory:"model"config option and areasoning_effortselector; routing through those would cost a round-trip per model per probe for information the bracketed catalog already carries in full.GPT-5.6-Solreads "…Fast responses with lighter reasoning" until the next PR in this stack.fast-modeandcollaboration_mode, which codex-acp also advertises, stay unread.Screenshot
Not captured — the affected surfaces are Obsidian panes (Settings → Agent → Codex and the chat model picker) and this environment cannot drive the Obsidian UI to load the branch build.
In place of a rendered capture, the live catalog from
codex-acp@1.1.10was replayed through the branch's realwire.decodeandtranslateBackendState, which produce exactly what those panes render:Verification step 2 reaches the real panes.
Risk
High
codexModelId.test.tscovers parse/format including unseen effort tokens and malformed brackets;descriptor.test.tscollapses a transcribed live catalog to 7 entries with per-model effort sets;codexModelIdMigration.test.tscovers the collapse, the enabled set, the sticky default and idempotencyconfiguredModels[].info.idin place and stampssettingsVersion: 9. Reverting the code leaves base-form rows that the old codec's next probe would prune, dropping the user's enabled setConfiguredModel.info.id,backends.codex.enabledModels, andagentMode.backends.codex.defaultModelare rewritten undersettingsVersion9applySelectionare coveredReview: inspect
src/settings/migrations/codexModelIdMigration.ts—planCodexModelIdCollapseandstripEffortLabel— line by line against a real pre-upgradedata.json, since it is the only irreversible write here; thensrc/agentMode/backends/codex/codexModelId.ts(CODEX_WIRE_ID,parseCodexModelId) andsrc/agentMode/backends/codex/descriptor.ts(codexWire,applySelection,firstAdvertisedEffort). Run Verification steps 1–4, including step 4's upgrade-from-backup path.Verification
Requires
codex-acpinstalled and logged in, on a vault whose Codex models were already enrolled by a previous Copilot version..obsidian/plugins/copilot/data.jsonsomewhere safe, and note which Codex models are enabled and which are not.GPT-5.6-Sol: the effort dropdown offerslowthroughultra. SelectGPT-5.5: it offerslowthroughxhigh, with nomaxorultra. Chooseultraon Sol and send a message — the turn runs rather than erroring.data.json, delete its"settingsVersion"line, and relaunch. The list again shows one row per base model with the same enabled set — confirming the migration replays from a genuine pre-upgrade state, not just from already-migrated settings.Note
The issue proposed adding
maxandultratoKNOWN_CODEX_EFFORTS. That would not have worked: with the delimiter wrong, that set never matched any advertised id. It is deleted here rather than extended.The issue also asked for collapsing logic in the settings list and a matching gallery story. Neither is present, because neither is needed —
translateBackendStatealready groups by decoded base model andagentModelDiscoveryenrolls the grouped entries, soconfiguredModelGrouping.tsandConfiguredModelEnableListare unchanged and no component renders differently.