V2 event provider adapters - #134
Conversation
|
Cursor Agent can help with this pull request. Just |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
| import { makeEventNdjsonLogger } from "./EventNdjsonLogger.ts"; | ||
|
|
||
| const PROVIDER = "cursor" as const; | ||
| const ACP_BINARY = "agent"; |
There was a problem hiding this comment.
🟠 High Layers/CursorAdapter.ts:49
Missing child 'error' handling: if the agent binary is invalid or not on PATH, spawn emits an 'error' that crashes the process. Suggest adding an 'error' listener immediately after spawn and converting it into a provider error with clean session cleanup.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/provider/Layers/CursorAdapter.ts around line 49:
Missing `child` `'error'` handling: if the `agent` binary is invalid or not on `PATH`, `spawn` emits an `'error'` that crashes the process. Suggest adding an `'error'` listener immediately after `spawn` and converting it into a provider error with clean session cleanup.
Evidence trail:
apps/server/src/provider/Layers/CursorAdapter.ts:507-513 (spawnAcpProcess function), apps/server/src/provider/Layers/CursorAdapter.ts:698 (child = spawnAcpProcess call), apps/server/src/provider/Layers/CursorAdapter.ts:722-737 (only exit event handled, no error event), git_grep for `child.(on|once).*'error'` returns no results confirming no error listener exists
| if (cause && typeof cause === "object" && "message" in cause) { | ||
| return String((cause as { message: unknown }).message); | ||
| } |
There was a problem hiding this comment.
🟢 Low Layers/CursorAdapter.ts:63
When cause is an Error with an empty message, the first check correctly skips it, but the object check on line 63 re-matches and returns the empty string anyway. Consider adding a length check to the object branch as well.
| if (cause && typeof cause === "object" && "message" in cause) { | |
| return String((cause as { message: unknown }).message); | |
| } | |
| if (cause && typeof cause === "object" && "message" in cause) { | |
| const msg = String((cause as { message: unknown }).message); | |
| if (msg.length > 0) return msg; | |
| } |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/provider/Layers/CursorAdapter.ts around lines 63-65:
When `cause` is an `Error` with an empty message, the first check correctly skips it, but the object check on line 63 re-matches and returns the empty string anyway. Consider adding a length check to the object branch as well.
Evidence trail:
apps/server/src/provider/Layers/CursorAdapter.ts lines 58-68 at REVIEWED_COMMIT. The function `toMessage` has the first check `if (cause instanceof Error && cause.message.length > 0)` at line 59, and the object check `if (cause && typeof cause === "object" && "message" in cause)` at line 63. An Error object satisfies all three conditions in the object check (truthy, typeof object, has message property), so an Error with empty message bypasses the length check but gets caught by line 63 and returns the empty string instead of the fallback.
| Effect.gen(function* () { | ||
| const context = yield* requireSession(sessionId); | ||
|
|
||
| if (!context.turnState) return; |
There was a problem hiding this comment.
🟡 Medium Layers/CursorAdapter.ts:911
interruptTurn should reuse the shared cleanup. Calling stopSessionInternal (or a shared interrupt helper) instead of setting context.stopped and child.kill() ensures pending RPCs/timers are cleared and the session is removed, avoiding hangs.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/provider/Layers/CursorAdapter.ts around line 911:
`interruptTurn` should reuse the shared cleanup. Calling `stopSessionInternal` (or a shared interrupt helper) instead of setting `context.stopped` and `child.kill()` ensures pending RPCs/timers are cleared and the session is removed, avoiding hangs.
Evidence trail:
apps/server/src/provider/Layers/CursorAdapter.ts lines 908-935 (interruptTurn implementation), lines 581-661 (stopSessionInternal implementation), lines 620-631 (pendingRpc cleanup in stopSessionInternal), line 661 (sessions.delete in stopSessionInternal), line 51 (DEFAULT_REQUEST_TIMEOUT_MS = 120_000)
| kind: "approval.requested", | ||
| summary: | ||
| event.requestKind === "command" | ||
| requestKind === "command" |
There was a problem hiding this comment.
🟡 Medium Layers/ProviderRuntimeIngestion.ts:119
Suggestion: Normalize event.payload.requestType to a simplified requestKind (command/file-change/other) via a shared helper and use it for both request.opened and request.resolved. Derive summary from this (use "Approval requested" for other) to fix the mismatch and incorrect "File-change..." text.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts around line 119:
Suggestion: Normalize `event.payload.requestType` to a simplified `requestKind` (`command`/`file-change`/`other`) via a shared helper and use it for both `request.opened` and `request.resolved`. Derive `summary` from this (use "Approval requested" for `other`) to fix the mismatch and incorrect "File-change..." text.
Evidence trail:
apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts lines 106-147 at REVIEWED_COMMIT. Lines 106-110 show normalization to `requestKind` (`command`/`file-change`/`other`). Lines 119-121 show summary only handles two cases, defaulting to "File-change approval requested" for both `file-change` AND `other`. Lines 140-141 show `request.resolved` passes raw `event.payload.requestType` directly as `requestKind` without normalization.
7a9b3d3 to
ac45b82
Compare
Second upstream sync on this branch: pingdotgg/t3code main at 2c7267a ("stop the reaper from silently killing live background subagents", pingdotgg#5677) into the DevGame fork. 19 conflicted paths, resolved under the standing doctrine (upstream structure wins; fork features re-expressed inside it; the fork's deliberate deletions stand). Highlights taken from upstream: background-subagent reaper + settling fixes (pingdotgg#5677/pingdotgg#5568), sidebar v2 promoted to THE sidebar (pingdotgg#5672 -- SidebarV2.tsx renamed Sidebar.tsx, old sidebar now LegacySidebar.tsx behind Settings -> Legacy features), plans fold into chat (pingdotgg#5558, plan sidebar deleted), agents observability panel (pingdotgg#5219), MCP tool-result payload slimming (pingdotgg#5482), thread pagination (pingdotgg#5493), per-device provider settings (pingdotgg#4479), theme library + configurable fonts (pingdotgg#5103), thread pinning (pingdotgg#5312/pingdotgg#5581), reconnect-warning grace (pingdotgg#5670), mobile 1.0.2. Notable resolution rulings: - Right panel collapses to agents-only: upstream retired "plan", the fork had already moved preview/terminal/diff/files to the dock. ChatView's right-panel plumbing reduced accordingly; persistence shim now ALLOWLISTS the surviving kind, tolerates corrupt entries, and prunes records it empties (three tests updated to the pruned contract). - Migration id space: fork ids 36/37 (spaces) stay where deployed DevGame databases recorded them; upstream's three arrivals take 38/39/40 with files renamed to match. Crossover hazard from stock-T3 data dirs is documented at the manifest and tracked with the storage-isolation task. - useThreadSidebarComponent re-pointed at Sidebar/LegacySidebar via useLegacySidebarEnabled (forceV1 gone -- /settings* mounts SettingsSidebarNav, no thread sidebar). The pingdotgg#111 aria-label derivation ported into the renamed Sidebar.tsx. - Locale-fragile upstream snooze tests (pingdotgg#4438 asserts en-US "PM") made locale-agnostic; ThemeSettings copy re-branded to DevGame; upstream's text-secondary-label token applied in the fork's MessageImageGrid. Verification: typecheck exit 0 across every package (incl. server + desktop run separately); suites green -- server 2197, web 2400, scripts 227, contracts 228. Four-lens Opus merge-gate review over the resolution: 31 findings, 29 fixed in this commit, 2 filed as follow-up tasks (pingdotgg#133 right-panel chooser UX -- pre-existing; pingdotgg#134 migration ledger guard). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Merges `pingdotgg/t3code` into the fork: upstream `2a3035353` from base `a37c66406`, 53 commits. Landed as a merge commit, not a cherry-pick. Tracker entry: `docs/fork/upstream-merge-log.md`. Gaps this merge opened or extended: `docs/fork/gaps.md`. ## Usable as-is Fork can expose these with no Moatless backend or deployment work. - **Project icons everywhere they belong** — favicon in the new-thread project picker (pingdotgg#10790), the project record passed to `ProjectFavicon` so icons cannot drift (pingdotgg#10714), and the same icon in the command palette (pingdotgg#10712). `apps/web/src/components/ProjectFavicon.tsx`, `DraftHeroHeadline.tsx`. - **Composer and sidebar layout settling** — footer held still while thread data loads (pingdotgg#10768), the bar under the composer no longer pops in (pingdotgg#10727), chat text no longer shows through a 1px gap under composer banners (pingdotgg#10635), scroll-to-end button kept close to the composer (pingdotgg#10543), sidebar rows no longer flash and shift on click (pingdotgg#10713), settings sidebar no longer shifts when switching pages (pingdotgg#10705). - **Minimap turn navigation** (pingdotgg#8531) — previous/next turn controls, `apps/web/src/components/chat/` minimap surface. Pure client state. - **Terminal** — copy selection with Ctrl+Insert (pingdotgg#8541), honor terminal link browser overrides (pingdotgg#10060). - **Usage panel** — account columns aligned across limit rows (pingdotgg#10690), email-bearing account labels hidden (pingdotgg#10668). - **Setup wizards consolidated into shared components** (pingdotgg#10832). - **File drops onto sidebar threads** (pingdotgg#7892) — rides the attachment upload path the fork already has; `useSidebarPendingFileDropStore` threaded through `ChatView.tsx` and `_chat.$environmentId.$threadId.tsx`. - **Mobile** — drag handles to arrange threads (pingdotgg#10496), Android wallpaper colors (pingdotgg#10691), optional Material You layout (pingdotgg#10692), tolerate native `Headers` without `getSetCookie` (pingdotgg#10851), respect notification permission when tokens rotate (pingdotgg#10850). - **Desktop** — macOS installer artwork (pingdotgg#10632, pingdotgg#10819, pingdotgg#10820), layout control hit targets (pingdotgg#10673), context menus in the browser (pingdotgg#10670), no declarations during bundling (pingdotgg#10679), keyring loading deferred until macOS cookie import (pingdotgg#10667). `electron-desktop` is kept in tree and is not a compliance target. - **Dependency and hygiene** — Effect `rc.112` and Alchemy `beta.76` (pingdotgg#10652) with reference syncs (pingdotgg#10653, pingdotgg#10654), and the knip export classification sweep across server modules (pingdotgg#10274–pingdotgg#10282). ## Unsupported in Moatless / needs implementation - **Attach files to question answers** (#7220dfe2c, pingdotgg#9871). Upstream added the wire capability `questionAttachments` in `packages/contracts/src/environment.ts` and threads `supportsQuestionAttachments` through `ChatView.tsx` → `ChatComposer.tsx`. The fork takes upstream's plumbing verbatim; the capability is simply absent from what Moatless reports, so the composer correctly offers nothing. **To implement:** accept attachments on the answer-submission path and report `capabilities.questionAttachments: true`. The sibling `ServerProvider.reportsContextWindow` flag lands in the same shape. Recorded in `gaps.md`. - **Pull request merge defaults** (#7d9aaf6a7, pingdotgg#8088). Adds `pullRequestMergeMethodOverrides` to `packages/contracts/src/settings.ts` — a per-project merge method plus a last-used default, surfaced in `ProjectSettingsPanel.tsx` and `PullRequestDetailPanel.tsx`. **To implement:** Moatless must persist these settings fields, and the panel that consumes them needs `pullRequests.detail`, which Moatless does not dispatch (it serves only `pullRequests.summary`). Recorded in `gaps.md` under Pull requests. - **Relay push-notification routing** (pingdotgg#10859, pingdotgg#10849, pingdotgg#10848) — current APNs registration routing for queued jobs, requeue checks for queued iOS alerts, shared notification policy prioritizing waiting agents. These land in `infra/relay/`, which belongs to the `cloud-relay-connect` concern the fork has decided out. No fork app code imports them; taken as upstream and left inert. ## Backend behavior to consider reproducing in Moatless Upstream server behavior the fork cannot use directly — `apps/server` is not what Moatless runs — but that would improve Moatless. - **Give completed turns a full session idle window** (#430fbd1ff, pingdotgg#10689). Upstream's `ProviderSessionReaper` was measuring idle time from a point that cut a completed turn's window short, so provider sessions were reaped earlier than intended and the next turn paid a cold start. Worth checking whatever Moatless uses to retire provider sessions against the same case. - **Release consumed event replay pages** (#08463e2c4, pingdotgg#10777). `OrchestrationEventStore` held every page it had produced while replaying, so a long thread's replay grew without bound. Upstream moved it to `Stream.paginate`. If Moatless replays orchestration events to reconnecting clients, it has the same shape of exposure. - **Stop Windows terminal processes when closing** (#47eed9fac, pingdotgg#10771) — terminal child processes outlived their session on Windows. Relevant to Moatless only if it hosts terminals on Windows runners. - **Generate thread titles with the selected model across connections** (#bc4b00666, pingdotgg#10526) — title generation was falling back to a default model rather than the connection's selected one. - **Keep preview snapshots usable by the agent and let it save them** (#061543e9e, pingdotgg#10501) — adds a `save` argument to the MCP `preview_snapshot` tool so the agent can persist a snapshot rather than only view it. Recorded in `gaps.md` under Preview automation. ## Conflicts 7 files. Full reasoning is in the tracker entry; the two worth reading here: - **`SettingsSidebarNav.tsx`** — upstream deleted the settings sub-section nav wholesale (`settingsSectionVisibility.ts` no longer exists upstream), which collided with the fork's admin/personal split. Kept upstream's removal and rebuilt the split on top of it, extracting `renderNavItem` so both nav groups render identical rows. - **`ChatComposer.tsx`** — both hunks resolved to upstream. The fork comment there documented `maxFileAttachmentBytes`, which upstream now owns itself. `pnpm-lock.yaml` was `theirs` then re-derived with `vp i`. `apps/server/src/cli/pair.ts` is the one file in the upstream range that did not land — the fork deletes that surface deliberately, and the tripwire confirms it is still deleted. ## A rename that no conflict marked Upstream's Effect bump renamed `Schema.TaggedErrorClass` to `Schema.TaggedError`. Upstream renamed its own two occurrences, so those merged clean — but the fork's three (`UnsupportedMethodError` in `packages/contracts/src/auth.ts`, `SandboxNotRunningError` in `packages/contracts/src/sandbox.ts`, and one in `apps/web/src/environments/primary/auth.ts`) have no upstream counterpart, so git carried them through untouched. Typecheck failed with one `TS2551` and about forty cascading `TS2740`s in `rpc.ts` behind it. All three renamed. ## Verification `verify.mjs` is green on seven checks: duplicate-adds, tripwires, resolution-check, unsupported-methods, `fmt:check`, lint, typecheck. `test` is red on **`@t3tools/desktop` only**, and it is the machine rather than this merge: `scripts/browser-secret-native.test.mjs` shells out to `pkg-config --cflags --libs libsecret-1`, which the sandbox image does not carry. The file is byte-identical to upstream and fails the same way on a clean tree; the standing entry is in `gaps.md`. 1289 tests pass, 1 suite fails to compile. Four packages did not finish under parallel load and all four pass alone — `@t3tools/web` 383 files, `t3` 293, `@t3tools/mobile` 155, `t3code-relay` 28. Unsupported methods: 1 ADD, 0 DROP, 2 KEEP, 4 known exceptions. The ADD is `sandbox.detail` and it is **pre-existing drift, not merge-introduced** — confirmed by re-running the derivation against `HEAD^1`. Applied anyway, with the union entry documenting that Moatless dispatches `sandbox.subscribeDetail` and not its one-shot sibling. `duplicate-adds.mjs` reported `target="_blank"` in `MessagesTimeline.tsx`. Both parents have it once, on two unrelated anchors — the fork's `MessageOriginIcon` and upstream's new question-attachment link. The script now skips a bare JSX attribute on its own line, for the same reason it already skips punctuation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- Moatless task: https://moatless.soaplabstest.com/tasks/ff7d0df5-d989-4ec6-95c6-4e730cf3fd6f
Update Codex and Claude Code adapters to use the v2 canonical event structure and add a new Cursor agent provider with v2 event support.
Note
Migrate provider adapters to emit V2 canonical runtime events and add Cursor adapter in serverLayers.ts
Adapters and tests switch to V2 event names, payloads, and Runtime* ID wrappers, unify message/tool events under
item.*, replace approvals withrequest.*, and add a newcursorprovider with ACP schemas and registry integration.📍Where to Start
Start with the event mapping pipeline in
events.mapToRuntimeEventsin CodexAdapter.ts, then reviewmakeClaudeCodeAdapterin ClaudeCodeAdapter.ts, and the Cursor layer in CursorAdapter.ts.📊 Macroscope summarized 32779bd. 10 files reviewed, 28 issues evaluated, 2 issues filtered, 4 comments posted
🗂️ Filtered Issues
apps/server/src/provider/Layers/CursorAdapter.ts — 3 comments posted, 13 evaluated, 2 filtered
item.completedevents due to state loss. TheCursorTurnStatedefinition (Code Object 2) only tracksseenToolCallIdsand fails to persist theCanonicalItemTypedetermined for each tool call. Consequently, when processing atool_call_updateat line 360, the code invokesclassifyToolItemType(Code Object 0) withundefinedarguments, forcing the return value to"dynamic_tool_call". This overwrites any specific type (e.g.,"command_execution") established at the start of the item, resulting in anitem.completedevent payload that conflicts with theitem.startedevent type. [ Out of scope ]turn.completedevents. ThesendTurnmethod initializesturnState(Code Object 2) and manages the turn lifecycle. IfinterruptTurnis called, it emits aturn.completedevent (state: interrupted) and kills the child process. However, the concurrentsendTurnfiber catches the resulting process termination error (lines 838-853) and blindly emits a secondturn.completedevent (state: failed) at line 879. ThesendTurnerror handler fails to checkcontext.stoppedorturnStatevalidity, leading to a protocol violation where the client receives two contradictory completion events for the same turn. [ Out of scope ]