Conversation
- Discover model roles from `omp config get cycleOrder` instead of `omp models --json` - Pass the selected role at session start; drop per-session model/config switching - Always auto-approve permissions (yolo) and expose runtime mode per provider - Web and mobile model pickers respect provider-specific runtime mode choices
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: fd27af94db
ℹ️ 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".
There was a problem hiding this comment.
🟡 Changes recommended
The OhMyPi install docs currently promise picker filtering behavior that isn’t enforced by the implementation and should be rephrased for accuracy.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR updates the OhMyPi provider integration to treat OhMyPi “roles” (from omp config get cycleOrder --json) as the model catalog, selecting the role at session start, and removes in-session model/config rewriting. It also adds shared client-runtime helpers so provider-specific runtime-mode constraints (OhMyPi → Full access only) consistently flow through web + mobile pickers and queued/outbox behavior.
Changes:
- OhMyPi model catalog now mirrors
cycleOrderroles (in order), starts ACP sessions with--model <role>and--approval-mode yolo, and reportssessionModelSwitch: unsupported/requiresNewThreadForModelChange: true. - Introduces
resolveProviderRuntimeMode+runtimeModeOptionsForProviderinpackages/client-runtimeand applies them across web + mobile runtime-mode UI and state resolution paths. - Updates docs and adds/updates tests for the new OhMyPi behavior and runtime-mode filtering.
File summaries
| File | Description |
|---|---|
| packages/contracts/src/model.ts | Changes OhMyPi default model identifier to the default role. |
| packages/client-runtime/src/providerRuntimeMode.ts | Adds shared helpers to coerce/filter runtime modes per provider (OhMyPi full-access only). |
| packages/client-runtime/src/providerRuntimeMode.test.ts | Adds unit coverage for runtime-mode coercion/filtering. |
| packages/client-runtime/package.json | Exports new ./provider-runtime-mode entrypoint. |
| docs/user/permission-modes.md | Documents OhMyPi as Full access–only. |
| docs/user/install.md | Updates OhMyPi setup/model picker guidance for role-based catalogs. |
| docs/user/composer.md | Updates custom-model guidance to reflect OhMyPi role-based catalogs. |
| apps/web/src/modelSelection.ts | Avoids custom model injection for OhMyPi catalogs. |
| apps/web/src/modelSelection.test.ts | Adds coverage ensuring OhMyPi role order is preserved and no custom injection occurs. |
| apps/web/src/components/settings/ProjectDefaultsSettings.tsx | Filters/coerces runtime-mode selection based on active provider (OhMyPi). |
| apps/web/src/components/ChatView.tsx | Coerces runtime mode based on selected provider; disables permission-mode command for OhMyPi. |
| apps/web/src/components/chat/CompactComposerControlsMenu.tsx | Drives permission-mode menu from provider-filtered runtime mode options. |
| apps/web/src/components/chat/ChatComposer.tsx | Filters runtime-mode options per provider and plumbs them into composer controls. |
| apps/server/src/provider/Layers/OhMyPiAdapter.ts | Starts sessions with role at launch, removes in-session model/config rewriting, forces full-access session mode. |
| apps/server/src/provider/Drivers/OhMyPiModels.ts | Probes omp config get cycleOrder --json and publishes roles as models (with a computed default). |
| apps/server/src/provider/Drivers/OhMyPiDriver.ts | Exposes role-based models, sets requiresNewThreadForModelChange, updates user-facing messages. |
| apps/server/src/provider/Drivers/OhMyPiDriver.test.ts | Updates tests to assert role-based catalogs, yolo mode, and no in-session model/config writes. |
| apps/server/src/provider/acp/OhMyPiAcpSupport.ts | Launches ACP with --model <role> and --approval-mode yolo; removes runtimeMode-based approval-mode mapping and in-session model/config helpers. |
| apps/server/src/provider/acp/OhMyPiAcpSupport.test.ts | Updates tests to assert role-based launch args and absence of in-session overrides. |
| apps/server/src/provider/acp/OhMyPiAcpCliProbe.test.ts | Removes runtimeMode argument from ACP CLI probe runtime creation. |
| apps/mobile/src/state/use-thread-composer-state.ts | Coerces runtime-mode updates based on the selected provider. |
| apps/mobile/src/state/thread-outbox-model.ts | Coerces queued/outbox runtime mode based on provider driver. |
| apps/mobile/src/lib/modelOptions.ts | Avoids injecting stale fallback models into OhMyPi role catalogs. |
| apps/mobile/src/lib/modelOptions.test.ts | Adds coverage ensuring OhMyPi role order is preserved and fallback injection is skipped. |
| apps/mobile/src/features/threads/ThreadSettingsSheet.tsx | Filters runtime-mode choices based on provider driver. |
| apps/mobile/src/features/threads/ThreadComposer.tsx | Coerces displayed runtime mode based on selected provider. |
| apps/mobile/src/features/threads/thread-settings-options.ts | Adds helper to filter runtime-mode choices per provider. |
| apps/mobile/src/features/threads/new-task-flow-provider.tsx | Coerces and persists runtime mode for new-task flow based on provider constraints. |
Review details
- Files reviewed: 28/28 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
The Project Defaults permissions control can persist “full-access” when OhMyPi is the active default provider, unintentionally changing the default permissions for other providers.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
apps/web/src/components/settings/ProjectDefaultsSettings.tsx:265
- When the default provider is OhMyPi, the permissions dropdown only shows "Full access" but selecting it will persist
defaultRuntimeMode: "full-access", changing the global/project default for other providers too (even though docs say OhMyPi always runs full access regardless of the default). Consider ignoring writes while the active/default provider is OhMyPi so the stored default can still represent non-OhMyPi threads.
- Files reviewed: 28/28 changed files
- Comments generated: 0 new
- Review effort level: Lite
- Force OhMyPi threads to full-access at dispatch and session bind, treating provider-reported mode as authoritative - Map the legacy oh-my-pi-default model to the default role without triggering a session restart - Update role picker docs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49f797e0ca
ℹ️ 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".
There was a problem hiding this comment.
🟡 Changes recommended
The HTTP dispatch normalization path currently maps any normalization failure to invalid_command, which can incorrectly mask server-side errors as client errors.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 34/34 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
It changes cross-surface provider semantics (model catalogs, session startup, and permission/runtime-mode enforcement) across server + web + mobile, which warrants final human review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
docs/user/composer.md:86
- Grammar: the subject here is singular ("OhMyPi"), but the text says "OhMyPi do not". Rephrase the sentence so it reads correctly (and consider keeping Antigravity + OhMyPi as a combined subject).
- Files reviewed: 36/36 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d510f403e7
ℹ️ 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".
There was a problem hiding this comment.
🟢 Approval recommended
The changes are cohesive, well-covered by focused tests, and the only remaining feedback is a minor docs grammar nit with an inline fix suggestion.
Review details
- Files reviewed: 36/36 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41df9e6112
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19c4b365ae
ℹ️ 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".
There was a problem hiding this comment.
🟡 Changes recommended
There are a couple of small but concrete issues to address (doc grammar and avoiding duplicated constants to prevent drift).
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
docs/user/composer.md:86
- Grammatically, “OhMyPi do not support custom models” should use a singular verb (“does not”). Rephrase this sentence to avoid the subject/verb mismatch while keeping the intended meaning.
- Files reviewed: 40/40 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca2830d256
ℹ️ 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".
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces cross-cutting behavioral changes spanning server normalization, provider session startup, and multiple client UIs, so it warrants final human review despite strong test coverage.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
packages/client-runtime/src/providerRuntimeMode.ts:24
runtimeModeOptionsForProviderreturns a new array literal ([FULL_ACCESS_RUNTIME_MODE]) each call for OhMyPi. Because callers pass this value down into memoized components (e.g.ChatComposer->CompactComposerControlsMenu), the changing reference defeats shallow-prop memoization and causes avoidable re-renders. Consider hoisting a single frozen/const array for the full-access-only case so the reference is stable.
- Files reviewed: 41/41 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fda1747b51
ℹ️ 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".
There was a problem hiding this comment.
🔵 Needs a closer look
The server adapter maps the legacy OhMyPi default sentinel to the literal "default" role even though the role probe explicitly supports cycle orders without "default", which can break legacy threads when that role isn’t configured.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
apps/server/src/provider/Layers/OhMyPiAdapter.ts:371
probeOhMyPiModelsexplicitly falls back to treating the firstcycleOrderrole as default when the role named "default" is absent (seedefaultRole = roles.includes(OH_MY_PI_DEFAULT_MODEL) ? ... : roles[0]). In contrast, the session-start path here always rewrites the legacy sentinel "oh-my-pi-default" to the literal role "default". If a user’s configured cycle order omits "default", legacy threads will attempt to start ACP with a role that is not in the published catalog (and likely not configured), causing session start failures.
Consider mapping the legacy sentinel to undefined so we omit --model and let the CLI choose its configured default role, instead of forcing the literal "default" role name.
- Files reviewed: 43/43 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 190e926a36
ℹ️ 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".
| return input.draft; | ||
| } | ||
|
|
||
| return input.thread; |
There was a problem hiding this comment.
When a mobile user changes the role on an existing OhMyPi thread that has no session yet, the first turn starts with the draft role, but thread.turn.start does not persist that selection into the thread shell. As soon as the session becomes non-null, this return replaces the valid draft with the shell's old role, and the repair effect overwrites the draft; the next send then requests that old role and is rejected because the running session is locked to the role used for the first turn. Fresh evidence in the final tree is that mobile retains the pre-session selection only in its draft while the server reactor merely caches the turn selection, so the accepted role should be persisted or otherwise distinguished from a stale draft.
AGENTS.md reference: AGENTS.md:L67-L70
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🟡 Changes recommended
The web composer can still surface the legacy OhMyPi default slug (oh-my-pi-default) as the selected/displayed model instead of canonicalizing it to the "default" role, which is user-visible and inconsistent with the new role catalog.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 43/43 changed files
- Comments generated: 1
- Review effort level: Lite
| const selectionToResolve = retainedThreadSelection ?? activeSelection; | ||
| const selectedModel = selectionToResolve?.model | ||
| ? (resolveAppModelSelectionForInstance( | ||
| activeSelectionInstanceId, | ||
| retainedThreadSelection?.instanceId ?? activeSelectionInstanceId, | ||
| input.settings, |
What Changed
OhMyPi now exposes its configured model cycle roles (from
omp config get cycleOrder --json) as the provider's model catalog instead of the CLI's full model list. The selected role is passed to the ACP session at start time (--model <role> --approval-mode yolo), and per-session model/config rewriting is removed —sessionModelSwitchis nowunsupportedand OhMyPi is markedrequiresNewThreadForModelChange.Supporting changes:
resolveProviderRuntimeMode/runtimeModeOptionsForProviderhelpers inpackages/client-runtimelet per-provider runtime mode choices filter through web and mobile pickers (web composer, compact controls, project defaults, mobile thread settings and new-task flow, queued outbox resolution).Why
OhMyPi's roles are the unit users actually configure; the previous catalog of concrete provider models with thinking options didn't map to anything the CLI could honor mid-session, and session-time
set_model/set_config_optioncalls silently rewrote shared state. Picking a role when the session starts matches how the CLI works, keeps the shared catalog immutable, and simplifies the adapter (permission auto-approval now runs in yolo mode unconditionally).UI Changes
Model pickers on web and mobile now show the configured role list (
smol,default,slow, ...) in cycle order, and the runtime-mode picker is filtered per provider. Screenshots to follow.Checklist
Built with GLM by Z.ai via opencode.