feat(providers): add Oh My Pi support - #7763
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| Semaphore.make(1).pipe( | ||
| Effect.map((semaphore) => { | ||
| const next = new Map(current); | ||
| next.set(threadId, semaphore); |
There was a problem hiding this comment.
🟡 Medium Layers/OmpAdapter.ts:215
threadLocksRef retains a semaphore and the threadId key for every thread ever passed to getThreadSemaphore, so stopping a session does not release that per-thread state and a long-lived server grows its memory usage without bound. Remove unused lock entries when a thread is stopped, while preserving entries still needed by concurrent operations.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/OmpAdapter.ts around line 215:
`threadLocksRef` retains a semaphore and the `threadId` key for every thread ever passed to `getThreadSemaphore`, so stopping a session does not release that per-thread state and a long-lived server grows its memory usage without bound. Remove unused lock entries when a thread is stopped, while preserving entries still needed by concurrent operations.
| const properties = request.requestedSchema.properties ?? {}; | ||
| const entries = Object.entries(properties).filter(([key]) => !key.endsWith("__other")); |
There was a problem hiding this comment.
🟠 High acp/OmpAcpSupport.ts:214
ompElicitationQuestions emits a UserInputQuestion for every schema property, so optional ACP fields are treated as mandatory by the web/mobile answer builders and users cannot submit the form without inventing values or cancelling. Filter the emitted entries by requestedSchema.required (or otherwise expose optionality to clients).
- const properties = request.requestedSchema.properties ?? {};
- const entries = Object.entries(properties).filter(([key]) => !key.endsWith("__other"));
+ const properties = request.requestedSchema.properties ?? {};
+ const required = new Set(request.requestedSchema.required ?? []);
+ const entries = Object.entries(properties).filter(
+ ([key]) => !key.endsWith("__other") && required.has(key),
+ );🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/acp/OmpAcpSupport.ts around lines 214-215:
`ompElicitationQuestions` emits a `UserInputQuestion` for every schema property, so optional ACP fields are treated as mandatory by the web/mobile answer builders and users cannot submit the form without inventing values or cancelling. Filter the emitted entries by `requestedSchema.required` (or otherwise expose optionality to clients).
|
|
||
| function isOmpNativeCommandPath(commandPath: string): boolean { | ||
| const normalized = normalizeCommandPath(commandPath); | ||
| return normalized.endsWith("/omp") || normalized.endsWith("/omp.exe"); |
There was a problem hiding this comment.
🟡 Medium Drivers/OmpDriver.ts:47
isOmpNativeCommandPath classifies every resolved executable whose basename is omp or omp.exe as native, so /opt/homebrew/bin/omp and .../node_modules/.bin/omp select omp update instead of the Homebrew or npm/pnpm/bun update action. This prevents package-managed OMP installations from being upgraded or reinstalled through their package manager; restrict native detection to genuine native-install paths or ensure package-manager detection takes precedence.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Drivers/OmpDriver.ts around line 47:
`isOmpNativeCommandPath` classifies every resolved executable whose basename is `omp` or `omp.exe` as native, so `/opt/homebrew/bin/omp` and `.../node_modules/.bin/omp` select `omp update` instead of the Homebrew or npm/pnpm/bun update action. This prevents package-managed OMP installations from being upgraded or reinstalled through their package manager; restrict native detection to genuine native-install paths or ensure package-manager detection takes precedence.
| }), | ||
| ), | ||
| ).pipe( | ||
| Effect.catch((cause) => |
There was a problem hiding this comment.
🟠 High Layers/OmpAdapter.ts:689
A single notification-processing failure terminates ctx.notificationFiber while the session remains ready, so all subsequent ACP notifications are silently dropped. Because Effect.catch wraps Stream.runDrain, an error from one mapEffect callback (such as makeEventStamp() failing) exits the whole drain instead of recovering per event. Move recovery inside the per-event processing or mark the session failed when the consumer exits.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/OmpAdapter.ts around line 689:
A single notification-processing failure terminates `ctx.notificationFiber` while the session remains `ready`, so all subsequent ACP notifications are silently dropped. Because `Effect.catch` wraps `Stream.runDrain`, an error from one `mapEffect` callback (such as `makeEventStamp()` failing) exits the whole drain instead of recovering per event. Move recovery inside the per-event processing or mark the session failed when the consumer exits.
| }): Effect.Effect<void, E> { | ||
| return Effect.gen(function* () { | ||
| const model = input.model?.trim(); | ||
| if (model && model !== "default") { |
There was a problem hiding this comment.
🟠 High acp/OmpAcpSupport.ts:586
Selecting "default" after a concrete model leaves the ACP runtime on the previous model, while sendTurn reports "default", so the recorded model and actual model diverge. The model !== "default" guard skips setModel; remove that special case so switching back to the provider default resets the runtime model.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/acp/OmpAcpSupport.ts around line 586:
Selecting `"default"` after a concrete model leaves the ACP runtime on the previous model, while `sendTurn` reports `"default"`, so the recorded model and actual model diverge. The `model !== "default"` guard skips `setModel`; remove that special case so switching back to the provider default resets the runtime model.
| { label: "No", description: "Do not confirm this action." }, | ||
| ] | ||
| : choices.length > 0 | ||
| ? choices.map((choice) => ({ |
There was a problem hiding this comment.
🟠 High acp/OmpAcpSupport.ts:225
Selecting an elicitation option whose title differs from its const drops or misroutes the answer instead of sending the selected const value. ompElicitationQuestions exposes choice.title as the UI answer, but buildOmpElicitationContent validates against choice.const; preserve or restore the title-to-const mapping before normalization and __other handling.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/acp/OmpAcpSupport.ts around line 225:
Selecting an elicitation option whose `title` differs from its `const` drops or misroutes the answer instead of sending the selected `const` value. `ompElicitationQuestions` exposes `choice.title` as the UI answer, but `buildOmpElicitationContent` validates against `choice.const`; preserve or restore the `title`-to-`const` mapping before normalization and `__other` handling.
| mapError: ({ cause, method }) => | ||
| mapAcpToAdapterError(PROVIDER, input.threadId, method, cause), | ||
| }); | ||
| ctx.activeTurnId = turnId; |
There was a problem hiding this comment.
🟡 Medium Layers/OmpAdapter.ts:752
sendTurn publishes turn.started and leaves ctx.activeTurnId set even when the turn is rejected for empty input, an invalid attachment, an attachment read failure, or a failed ctx.acp.prompt; no matching turn.completed is emitted, so the session remains stuck on a failed turn. Move activation and turn.started until after prompt validation, and clear or complete the active turn when later processing fails.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/OmpAdapter.ts around line 752:
`sendTurn` publishes `turn.started` and leaves `ctx.activeTurnId` set even when the turn is rejected for empty input, an invalid attachment, an attachment read failure, or a failed `ctx.acp.prompt`; no matching `turn.completed` is emitted, so the session remains stuck on a failed turn. Move activation and `turn.started` until after prompt validation, and clear or complete the active turn when later processing fails.
| if (exact) return exact; | ||
| } | ||
| return normalizedAliases | ||
| .map((alias) => modes.find((mode) => normalizeModeSearchText(mode).includes(alias))) |
There was a problem hiding this comment.
🟠 High acp/OmpAcpSupport.ts:85
requestedOmpModeId can select a plan mode instead of an implementation mode when a plan mode’s description mentions an alias such as code, switching the session into plan mode unexpectedly. findModeByAliases uses unrestricted substring matching over the normalized concatenated text; match whole tokens and exclude plan modes when resolving implementation modes.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/acp/OmpAcpSupport.ts around line 85:
`requestedOmpModeId` can select a plan mode instead of an implementation mode when a plan mode’s description mentions an alias such as `code`, switching the session into plan mode unexpectedly. `findModeByAliases` uses unrestricted substring matching over the normalized concatenated text; match whole tokens and exclude plan modes when resolving implementation modes.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 17532ea. Configure here.
| } | ||
| if (normalized !== undefined) content[question.key] = normalized; | ||
| } | ||
| return content; |
There was a problem hiding this comment.
Elicitation answers ignore choice consts
Medium Severity
OMP elicitation options expose oneOf/anyOf titles to the UI, and T3 returns those labels as answers, but buildOmpElicitationContent / normalizeElicitationAnswer only match against const values. When title and const differ, a valid pick can be treated as a custom __other answer or dropped from multi-select payloads, so OMP receives the wrong form content.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 17532ea. Configure here.
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This PR adds a complete new provider integration (Oh My Pi) with ~4000 lines of new code across driver, adapter, ACP support, text generation, and UI components. Multiple High severity findings identify bugs in elicitation handling, notification processing, and model selection that need to be addressed. New provider integrations with this scope warrant careful review. Not approved because:
Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more. |
|
potential duplicate with #6160 |
2 participants
What Changed
This PR adds Oh My Pi (OMP) as a built-in provider across the typed contracts, server, web client, desktop wrapper, and mobile client.
subProvider.Oh My Pi · Moonshotin web and mobile model pickers.Why
T3 Code cannot currently use an OMP subscription. OMP can expose the same model name through several upstream providers. A model name without its upstream provider is ambiguous and can select a different service, price, or account than the user intended.
The implementation keeps provider-specific behavior at the adapter boundary and reuses the existing ACP and provider conventions. The contribution guide points feature proposals to Ideas, but GitHub currently reports that Discussions are disabled for this repository.
UI Changes
Before, duplicate Kimi K2.6 entries only showed
Oh My Pi.After, each entry shows its upstream provider.
Verification
git diff --checkpassed.Current OMP Limits
Checklist
Built with GPT-5.6 Sol in T3 Code through the Codex harness.
Note
Medium Risk
Adds a new agent runtime that spawns
omp acpand maps T3 permission modes to OMP approval, including auto-approve of edit gates. The provider is off by default, but session, permission, and process-spawn paths are security-sensitive.Overview
Adds Oh My Pi (
omp) as a built-in provider so T3 can drive an existing OMP install over ACP. It is off by default; users enable it in settings afteromp setup.The server discovers the OMP model catalog (min version 17.4.0), keeps each upstream as
subProvider(e.g. Moonshot vs OpenRouter), and runs sessions through a new ACP adapter. T3 permission modes map to OMP approval (always-ask/write/yolo); launch-arg approval flags cannot override that. Text generation uses a locked-down ACP spawn (no tools/extensions). Plan mode is hidden and provider rollback is unsupported.Web and mobile pickers, icons, drafts, and settings include OMP. Model rows show
Oh My Pi · {upstream}. ACP runtime also parses usage and available-command updates used by OMP.Reviewed by Cursor Bugbot for commit 17532ea. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add Oh My Pi (
omp) provider across server, web, mobile, and contractsompdriver: OmpDriver.ts, OmpAdapter.ts, OmpProvider.ts, and ACP support in OmpAcpSupport.ts with session management, model catalog parsing, version gating, and maintenance resolvers (npm, Homebrew, nativeomp update)ompin contracts (settings.ts, model.ts), builtInDrivers.ts, web provider picker/settings, mobile model options/icons, and docsUsageUpdatedandAvailableCommandsUpdatedevents; Cursor and Grok adapters updated to ignore themompis disabled by default; provider picker shows anewbadgeparseSessionUpdateEventnow emitsUsageUpdated/AvailableCommandsUpdatedevents — existing adapters that did not handle these now explicitly no-op them (Cursor, Grok); any out-of-tree ACP consumers must handle or ignore the new event variants📊 Macroscope summarized 17532ea. 35 files reviewed, 9 issues evaluated, 0 issues filtered, 8 comments posted
🗂️ Filtered Issues