Merge upstream/main into fork (batch 4)#7
Merged
aaditagrawal merged 15 commits intomainfrom Mar 11, 2026
Merged
Conversation
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: codex <codex@users.noreply.github.com> Co-authored-by: Julius Marminge <julius0216@outlook.com>
- Update AGENTS.md task completion requirements to include `bun fmt` - Add an inline note explaining why Electron uses hash history
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Integrates 10 upstream commits including runtime orchestration stabilization, pushBus abstraction, cross-repo PR detection, checkpoint fixes, and wsTransport type safety improvements. Conflict resolutions: - ProviderCommandReactor: kept multi-provider schema validation and thread.deleted handler, adopted DrainableWorker pattern - wsServer: integrated pushBus abstraction while preserving all multi-provider model/usage handlers - wsTransport: accepted upstream type-safe push listeners and state tracking
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces deterministic orchestration primitives (drainable workers, receipts), a sequenced server push bus with readiness gates, typed WebSocket push envelopes and transport replay, cross-repo Git/PR discovery, many test-harness updates, and supporting docs and shared utilities for deterministic provider-neutral runtime orchestration. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser (Client)
participant WsTransport as WsTransport (client)
participant Server as Server (wsServer + PushBus)
participant Orchestration as OrchestrationEngine
participant Provider as ProviderRuntime
participant Receipts as RuntimeReceiptBus
Browser->>WsTransport: send request (JSON‑RPC)
WsTransport->>Server: forward request
Server->>Orchestration: enqueue command / emit domain event
Orchestration->>Provider: dispatch provider command
Provider-->>Orchestration: runtime events / results
Orchestration->>Receipts: publish receipt (e.g., checkpoint.diff.finalized)
Receipts-->>Orchestration: subscribers observe receipt
Orchestration->>Server: publish push (channel + sequence)
Server->>Browser: deliver push envelope (typed, sequenced)
Browser->>WsTransport: subscribe(channel, replayLatest) -> receives latest push
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
|
github-actions
bot
added
the
vouch:trusted
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/server/src/git/Layers/GitHubCli.ts (1)
244-259:⚠️ Potential issue | 🔴 CriticalAdd explicit
--repoparameter to prevent PR creation against wrong repository.The
createPullRequestcommand doesn't specify--repo, allowingghto infer the destination repository from local git state. In fork/multi-remote workflows, this is unsafe and can cause PRs to be created against the upstream or wrong repository. Add arepositoryfield to the input type and append--repoto the args.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/git/Layers/GitHubCli.ts` around lines 244 - 259, The createPullRequest implementation in GitHubCli calls gh pr create without an explicit repository, which can target the wrong repo; update the input type for createPullRequest to include a repository string (e.g., input.repository) and modify GitHubCli.createPullRequest to append "--repo" and input.repository to the args array (keeping existing args order) so gh is forced to create the PR against the intended repository.apps/server/integration/orchestrationEngine.integration.test.ts (1)
388-398:⚠️ Potential issue | 🟠 MajorWait for turn 1 quiescence before starting turn 2.
apps/server/src/orchestration/Services/RuntimeReceiptBus.ts(Lines 33-43) definesCheckpointDiffFinalizedReceiptandTurnProcessingQuiescedReceiptseparately, so this only proves the diff finished. Starting the second turn here can still race with leftover async work from turn 1, which is exactly the flakiness this receipt model is meant to remove. It also skips checking the first receipt status.Suggested fix
- yield* harness.waitForReceipt( + const firstReceipt = yield* harness.waitForReceipt( (receipt): receipt is CheckpointDiffFinalizedReceipt => receipt.type === "checkpoint.diff.finalized" && receipt.threadId === THREAD_ID && receipt.checkpointTurnCount === 1, ); + assert.equal(firstReceipt.status, "ready"); + yield* harness.waitForReceipt( + (receipt): receipt is TurnProcessingQuiescedReceipt => + receipt.type === "turn.processing.quiesced" && + receipt.threadId === THREAD_ID && + receipt.checkpointTurnCount === 1, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/integration/orchestrationEngine.integration.test.ts` around lines 388 - 398, The test currently only waits for CheckpointDiffFinalizedReceipt which proves the diff finished but can race with leftover async work; change the sequence to first assert the CheckpointDiffFinalizedReceipt for THREAD_ID and checkpointTurnCount === 1 (and verify any status field on that receipt if present) and then call harness.waitForReceipt again to wait for a TurnProcessingQuiescedReceipt for threadId === THREAD_ID and turnCount === 1 so the test guarantees turn 1 quiescence before starting turn 2; use the same THREAD_ID and the receipt types CheckpointDiffFinalizedReceipt and TurnProcessingQuiescedReceipt to locate the checks.
🧹 Nitpick comments (5)
apps/web/src/components/KeybindingsToast.browser.tsx (1)
351-357: Drop the fixed sleep from the replay assertion.The comment says replay is synchronous on subscribe, but Line 357 turns the check into a timing-based test anyway. This can still miss a delayed replay on a slow run and it adds avoidable latency to the suite. Prefer asserting immediately after remount, or wait on a deterministic signal instead of
setTimeout(500).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/KeybindingsToast.browser.tsx` around lines 351 - 357, The test uses a fixed 500ms sleep after calling mounted.cleanup() and remounting via mountApp(), which makes the assertion timing-based; remove the setTimeout and instead assert immediately after remount or await a deterministic signal from the replay mechanism (e.g., the subscribe/replay promise or a test-only hook) so the replay is confirmed synchronously; update the test to rely on mountApp() completing the replay (or to await a provided replay-complete promise/event from onServerConfigUpdated or the remounted instance) before checking that no toast appeared.apps/server/src/keybindings.ts (1)
840-846: Race condition in startup check-and-set.The sequence of
Ref.get(startedRef)followed byRef.set(startedRef, true)is not atomic. Two concurrent calls tostart()could both observealreadyStarted === falsebefore either sets the ref, potentially leading to duplicate startup attempts.Consider using
Ref.getAndSetfor atomic check-and-set:♻️ Proposed fix using atomic check-and-set
const start = Effect.gen(function* () { - const alreadyStarted = yield* Ref.get(startedRef); + const alreadyStarted = yield* Ref.getAndSet(startedRef, true); if (alreadyStarted) { return yield* Deferred.await(startedDeferred); } - - yield* Ref.set(startedRef, true); const startup = Effect.gen(function* () {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/keybindings.ts` around lines 840 - 846, The check-and-set in the start Effect is not atomic: replace the Ref.get(startedRef) + Ref.set(startedRef, true) sequence with an atomic Ref.getAndSet(startedRef, true) call inside the start function so you can inspect the previous value; if getAndSet returns true then immediately return by awaiting startedDeferred (same behavior as before), otherwise proceed with startup and resolve/fail startedDeferred appropriately—change logic in the start generator to use Ref.getAndSet on startedRef and base the branch on its returned previous value.apps/server/integration/OrchestrationEngineHarness.integration.ts (1)
123-123: Consider documenting the timeout increase.The default timeout was increased from 3000ms to 10000ms. While this may be appropriate for complex orchestration tests, consider adding a brief comment explaining the rationale to help future maintainers understand why this value was chosen.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/integration/OrchestrationEngineHarness.integration.ts` at line 123, Add a brief inline comment next to the timeoutMs default (timeoutMs = 10_000) in OrchestrationEngineHarness.integration.ts explaining why the default was increased from 3000ms to 10000ms (e.g., to accommodate longer-running orchestration/integration steps, external service latencies, or CI variability), so future maintainers understand the rationale and can adjust if tests or environment characteristics change; keep the comment short and specific and reference the timeoutMs declaration for location.apps/web/src/wsTransport.ts (1)
244-274: Consider bounding the outbound queue.The outbound queue can grow unbounded during prolonged disconnection. For production resilience, consider limiting queue size and dropping oldest messages when full.
♻️ Optional: Add queue size limit
const REQUEST_TIMEOUT_MS = 60_000; const RECONNECT_DELAYS_MS = [500, 1_000, 2_000, 4_000, 8_000]; +const MAX_OUTBOUND_QUEUE_SIZE = 100; const decodeWsResponse = decodeUnknownJsonResult(WsResponseSchema); // ... in send(): private send(encodedMessage: string) { if (this.disposed) { return; } this.outboundQueue.push(encodedMessage); + // Prevent unbounded queue growth during prolonged disconnection + while (this.outboundQueue.length > MAX_OUTBOUND_QUEUE_SIZE) { + this.outboundQueue.shift(); + } try { this.flushQueue();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/wsTransport.ts` around lines 244 - 274, The outboundQueue used by send() and flushed by flushQueue() is unbounded and may grow indefinitely during disconnection; update the logic in send() (and/or the outboundQueue handling) to enforce a maximum queue size (e.g., MAX_OUTBOUND_QUEUE), dropping oldest messages when the queue is full before pushing a new encodedMessage, and emit a warning/log when a drop occurs; ensure flushQueue() behavior remains unchanged and still re-enqueues on send failure (use the existing outboundQueue, send(), flushQueue(), ws, and disposed symbols to locate and modify the code).apps/server/src/wsServer/pushBus.ts (1)
40-40: Consider adding a finalizer for queue shutdown.The queue is scoped but lacks an explicit shutdown finalizer. While the fiber will be interrupted on scope close, adding a finalizer ensures clean resource cleanup:
♻️ Proposed fix
const queue = yield* Queue.unbounded<PushJob>(); + yield* Effect.addFinalizer(() => Queue.shutdown(queue).pipe(Effect.asVoid)); const encodePush = Schema.encodeUnknownEffect(Schema.fromJsonString(WsPush));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/wsServer/pushBus.ts` at line 40, The created in-scope queue (Queue.unbounded<PushJob>() assigned to queue) needs an explicit shutdown finalizer to ensure clean cleanup on scope close; register a finalizer that calls the queue's shutdown/close method (e.g., queue.shutdown() or queue.close() depending on the queue API) via the scope finalizer mechanism (scope.addFinalizer or Scope.finalizer) right after the queue is created so the queue is deterministically closed when the scope/fiber is interrupted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.docs/architecture.md:
- Around line 89-98: The documentation mentions a non-existent push channel
"orchestration.domainEvent" and points to orchestration.ts, but the current WS
contract (packages/contracts/src/ws.ts) only defines terminal.event,
server.welcome, and server.configUpdated; update the architecture doc to reflect
the actual WebSocket channels or add the missing channel to the contract.
Specifically, either change the diagram/text that references
orchestration.domainEvent and the path to orchestration.ts to reference the real
transport symbols used by WsTransport/nativeApi/wsServer/ServerPushBus (e.g.,
terminal.event/server.welcome/server.configUpdated), or add a new channel
definition to packages/contracts/src/ws.ts and ensure wsServer,
ProviderRuntimeIngestion, and OrchestrationEngine publish/subscribe to that new
channel so the doc and contracts stay in sync.
In @.github/VOUCHED.td:
- Line 29: The file's entries must remain alphabetically sorted; move the entry
"github:eggfriedrice24" from its current position to between
"github:cursoragent" and "github:gbarros-dev" so the list preserves alphabetical
order—locate the string "github:eggfriedrice24", remove it from line 29, and
insert it immediately after the line containing "github:cursoragent".
In @.mise.toml:
- Around line 1-3: The .mise.toml uses strict runtime pins ([tools] with node =
"24.13.1" and bun = "1.3.9") but apps/server/package.json has a broader
engines.node range; update apps/server/package.json's engines field to match the
exact pins by setting engines.node to "24.13.1" (and engines.bun to "1.3.9" if
present) so the package.json and .mise.toml enforce the same runtime boundary,
then run the usual lint/CI to verify no tooling warnings.
In `@apps/server/src/git/Layers/GitCore.ts`:
- Around line 884-899: The fast-path push uses currentUpstream.remoteName parsed
by splitting @{upstream}, which breaks remotes containing slashes; modify the
logic in the block that calls resolveCurrentUpstream and runGit (inside
GitCore.pushCurrentBranch.pushUpstream flow) to resolve the remote correctly
before invoking runGit: either read branch config via branch.<branch>.remote and
branch.<branch>.merge to get the exact remote and upstreamRef, or implement
longest-prefix matching against known remotes to derive the full remote name,
then pass that resolved remote name and upstreamRef into runGit and the returned
object (currentUpstream.remoteName → resolvedRemote, currentUpstream.upstreamRef
→ upstreamBranch).
In `@apps/server/src/git/Layers/GitManager.ts`:
- Around line 479-485: The code fails when a remote name contains "/" because
extractBranchFromRef only strips the first segment; fix by removing the entire
remote prefix from details.upstreamRef before calling extractBranchFromRef: if
details.upstreamRef and remoteName are present and
details.upstreamRef.startsWith(`${remoteName}/`) then strip that prefix (e.g.
upstreamRef = details.upstreamRef.slice(remoteName.length + 1)) and pass the
stripped upstreamRef to extractBranchFromRef when computing
headBranchFromUpstream (affecting variables remoteName, details.upstreamRef,
headBranchFromUpstream, and headBranch).
In `@apps/server/src/workspaceEntries.ts`:
- Around line 412-415: The current clearWorkspaceIndexCache simply deletes
entries from workspaceIndexCache and inFlightWorkspaceIndexBuilds but does not
prevent an older buildWorkspaceIndex() promise from later repopulating
workspaceIndexCache or deleting a newer inFlightWorkspaceIndexBuilds entry;
update getWorkspaceIndex()/buildWorkspaceIndex() so mutations inside the
promise's .then() and .finally() are gated by promise identity (compare the
stored promise in inFlightWorkspaceIndexBuilds[cwd] before writing) or by a
generation token tied to cwd (increment on clearWorkspaceIndexCache and only
apply results if token matches) and ensure clearWorkspaceIndexCache
increments/rotates that token or invalidates the stored identity to prevent
stale builders from touching shared state.
In `@apps/server/src/wsServer.test.ts`:
- Around line 239-266: The shared MessageChannel currently resolves the oldest
waiter unconditionally, which drops messages that don't match the waiter's
predicate/id and causes lost messages; change the channel protocol so waiters
carry a matching predicate (or expected id/channel) and modify enqueue (and
dequeue lookup) to search for a waiter whose predicate matches the incoming item
(e.g., check response.id or channel) and only resolve that waiter; if no
matching waiter exists, push the item onto channel.queue. Likewise, when
dequeue/waiting, instead of shifting the first queued item, scan channel.queue
for the first item that matches the caller's predicate (or id/channel) and
remove/return it, leaving non-matching messages in the queue. Apply this
matching-by-id/channel change to enqueue, dequeue, and the helpers
sendRequest()/waitForPush() so they provide the predicate/expected id to the
waiter rather than relying on post-filtering.
In `@apps/web/src/components/KeybindingsToast.browser.tsx`:
- Around line 239-246: The helper waitForToast currently allows matches.length
>= count which lets duplicate toasts slip through; update the assertion in
waitForToast (the function named waitForToast using queryToastTitles) to assert
the exact number by checking matches.length === count (e.g.,
expect(matches.length, `Expected ${count} "${title}" toast(s)`).toBe(count)) so
the test fails if extra duplicates are emitted. Ensure the changed expectation
runs inside the same vi.waitFor block with the same timeout/interval behavior.
In `@apps/web/src/wsTransport.test.ts`:
- Around line 175-184: The test asserts an exact SyntaxError text which varies
by runtime; update the first assertion that calls
expect(warnSpy).toHaveBeenNthCalledWith(1, ...) to use a flexible matcher for
the error string (e.g., replace the exact second argument with
expect.stringContaining('SyntaxError') or another stable substring) so the
assertion uses expect.stringContaining(...) like the second assertion; locate
the assertion referencing warnSpy and toHaveBeenNthCalledWith(1, ...) in
wsTransport.test.ts and change only the error-string argument to a
stringContaining matcher.
In `@packages/shared/src/DrainableWorker.ts`:
- Around line 38-40: The loop in makeDrainableWorker currently runs fallible
work (the process parameter typed as Effect.Effect<void, E, R>) inside
Effect.forever so any failure will kill the child fiber and leave queued items
stalled; fix by either changing the process signature to be infallible
(Effect.Effect<void, never, R>) so callers cannot pass failing effects, or wrap
the invocation of process inside a catch (e.g., process(item).catchAll(err => {
log the error; continue })) before the Effect.forever to ensure a single failed
item does not terminate the worker; apply the same change to the other
loop/fiber creation spots referenced (the branch around enqueue/drain handling).
---
Outside diff comments:
In `@apps/server/integration/orchestrationEngine.integration.test.ts`:
- Around line 388-398: The test currently only waits for
CheckpointDiffFinalizedReceipt which proves the diff finished but can race with
leftover async work; change the sequence to first assert the
CheckpointDiffFinalizedReceipt for THREAD_ID and checkpointTurnCount === 1 (and
verify any status field on that receipt if present) and then call
harness.waitForReceipt again to wait for a TurnProcessingQuiescedReceipt for
threadId === THREAD_ID and turnCount === 1 so the test guarantees turn 1
quiescence before starting turn 2; use the same THREAD_ID and the receipt types
CheckpointDiffFinalizedReceipt and TurnProcessingQuiescedReceipt to locate the
checks.
In `@apps/server/src/git/Layers/GitHubCli.ts`:
- Around line 244-259: The createPullRequest implementation in GitHubCli calls
gh pr create without an explicit repository, which can target the wrong repo;
update the input type for createPullRequest to include a repository string
(e.g., input.repository) and modify GitHubCli.createPullRequest to append
"--repo" and input.repository to the args array (keeping existing args order) so
gh is forced to create the PR against the intended repository.
---
Nitpick comments:
In `@apps/server/integration/OrchestrationEngineHarness.integration.ts`:
- Line 123: Add a brief inline comment next to the timeoutMs default (timeoutMs
= 10_000) in OrchestrationEngineHarness.integration.ts explaining why the
default was increased from 3000ms to 10000ms (e.g., to accommodate
longer-running orchestration/integration steps, external service latencies, or
CI variability), so future maintainers understand the rationale and can adjust
if tests or environment characteristics change; keep the comment short and
specific and reference the timeoutMs declaration for location.
In `@apps/server/src/keybindings.ts`:
- Around line 840-846: The check-and-set in the start Effect is not atomic:
replace the Ref.get(startedRef) + Ref.set(startedRef, true) sequence with an
atomic Ref.getAndSet(startedRef, true) call inside the start function so you can
inspect the previous value; if getAndSet returns true then immediately return by
awaiting startedDeferred (same behavior as before), otherwise proceed with
startup and resolve/fail startedDeferred appropriately—change logic in the start
generator to use Ref.getAndSet on startedRef and base the branch on its returned
previous value.
In `@apps/server/src/wsServer/pushBus.ts`:
- Line 40: The created in-scope queue (Queue.unbounded<PushJob>() assigned to
queue) needs an explicit shutdown finalizer to ensure clean cleanup on scope
close; register a finalizer that calls the queue's shutdown/close method (e.g.,
queue.shutdown() or queue.close() depending on the queue API) via the scope
finalizer mechanism (scope.addFinalizer or Scope.finalizer) right after the
queue is created so the queue is deterministically closed when the scope/fiber
is interrupted.
In `@apps/web/src/components/KeybindingsToast.browser.tsx`:
- Around line 351-357: The test uses a fixed 500ms sleep after calling
mounted.cleanup() and remounting via mountApp(), which makes the assertion
timing-based; remove the setTimeout and instead assert immediately after remount
or await a deterministic signal from the replay mechanism (e.g., the
subscribe/replay promise or a test-only hook) so the replay is confirmed
synchronously; update the test to rely on mountApp() completing the replay (or
to await a provided replay-complete promise/event from onServerConfigUpdated or
the remounted instance) before checking that no toast appeared.
In `@apps/web/src/wsTransport.ts`:
- Around line 244-274: The outboundQueue used by send() and flushed by
flushQueue() is unbounded and may grow indefinitely during disconnection; update
the logic in send() (and/or the outboundQueue handling) to enforce a maximum
queue size (e.g., MAX_OUTBOUND_QUEUE), dropping oldest messages when the queue
is full before pushing a new encodedMessage, and emit a warning/log when a drop
occurs; ensure flushQueue() behavior remains unchanged and still re-enqueues on
send failure (use the existing outboundQueue, send(), flushQueue(), ws, and
disposed symbols to locate and modify the code).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9dae8c50-bffd-48bb-bb55-60bb996bc4e7
📒 Files selected for processing (65)
.docs/architecture.md.docs/provider-architecture.md.docs/workspace-layout.md.github/VOUCHED.td.mise.toml.plans/17-provider-neutral-runtime-determinism.mdAGENTS.mdapps/desktop/src/main.tsapps/server/integration/OrchestrationEngineHarness.integration.tsapps/server/integration/orchestrationEngine.integration.test.tsapps/server/src/git/Layers/GitCore.test.tsapps/server/src/git/Layers/GitCore.tsapps/server/src/git/Layers/GitHubCli.tsapps/server/src/git/Layers/GitManager.test.tsapps/server/src/git/Layers/GitManager.tsapps/server/src/git/Services/GitHubCli.tsapps/server/src/keybindings.tsapps/server/src/orchestration/Layers/CheckpointReactor.test.tsapps/server/src/orchestration/Layers/CheckpointReactor.tsapps/server/src/orchestration/Layers/OrchestrationEngine.tsapps/server/src/orchestration/Layers/OrchestrationReactor.test.tsapps/server/src/orchestration/Layers/ProviderCommandReactor.test.tsapps/server/src/orchestration/Layers/ProviderCommandReactor.tsapps/server/src/orchestration/Layers/ProviderRuntimeIngestion.test.tsapps/server/src/orchestration/Layers/ProviderRuntimeIngestion.tsapps/server/src/orchestration/Layers/RuntimeReceiptBus.tsapps/server/src/orchestration/Services/CheckpointReactor.tsapps/server/src/orchestration/Services/ProviderCommandReactor.tsapps/server/src/orchestration/Services/ProviderRuntimeIngestion.tsapps/server/src/orchestration/Services/RuntimeReceiptBus.tsapps/server/src/orchestration/projector.tsapps/server/src/provider/Layers/ProviderService.tsapps/server/src/serverLayers.tsapps/server/src/workspaceEntries.tsapps/server/src/wsServer.test.tsapps/server/src/wsServer.tsapps/server/src/wsServer/pushBus.test.tsapps/server/src/wsServer/pushBus.tsapps/server/src/wsServer/readiness.tsapps/server/vitest.config.tsapps/web/public/mockServiceWorker.jsapps/web/src/components/BranchToolbar.logic.test.tsapps/web/src/components/BranchToolbar.logic.tsapps/web/src/components/ChatMarkdown.tsxapps/web/src/components/ChatView.browser.tsxapps/web/src/components/ChatView.tsxapps/web/src/components/ComposerPromptEditor.tsxapps/web/src/components/GitActionsControl.tsxapps/web/src/components/KeybindingsToast.browser.tsxapps/web/src/components/Sidebar.tsxapps/web/src/composerDraftStore.tsapps/web/src/main.tsxapps/web/src/routes/__root.tsxapps/web/src/routes/_chat.$threadId.tsxapps/web/src/wsNativeApi.test.tsapps/web/src/wsNativeApi.tsapps/web/src/wsTransport.test.tsapps/web/src/wsTransport.tsapps/web/vitest.browser.config.tspackages/contracts/src/ws.test.tspackages/contracts/src/ws.tspackages/shared/package.jsonpackages/shared/src/DrainableWorker.test.tspackages/shared/src/DrainableWorker.tspackages/shared/src/schemaJson.ts
💤 Files with no reviewable changes (2)
- apps/web/src/components/GitActionsControl.tsx
- apps/web/src/components/ChatView.tsx
Comment on lines
+1
to
+3
| [tools] | ||
| node = "24.13.1" | ||
| bun = "1.3.9" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify repo-wide Node/Bun version pins align with .mise.toml.
# Expected: either no other pins, or all matches point to Node 24.13.1 / Bun 1.3.9.
rg -n \
-g '.github/workflows/*.yml' \
-g '.github/workflows/*.yaml' \
-g 'package.json' \
-g 'README*' \
-g '*.md' \
-g '.nvmrc' \
-g '.node-version' \
-g 'Dockerfile*' \
-g '*.toml' \
'(setup-node|setup-bun|node-version|bun-version|engines|volta|^\s*node\s*=|^\s*bun\s*=)'Repository: aaditagrawal/t3code
Length of output: 193
🏁 Script executed:
cat -n package.json | sed -n '60,70p'Repository: aaditagrawal/t3code
Length of output: 327
🏁 Script executed:
cat -n apps/server/package.json | sed -n '45,55p'Repository: aaditagrawal/t3code
Length of output: 301
Nice to have a single toolchain pin here.
[tools] is the expected mise.toml shape for exact runtime pins. The root package.json engines field matches correctly: node = "^24.13.1" and bun = "^1.3.9". However, apps/server/package.json specifies a broader range (node = "^22.13 || ^23.4 || >=24.10"), which is inconsistent with the strict pin in .mise.toml. While 24.13.1 does satisfy that constraint, it's worth aligning apps/server/package.json to enforce the same version boundary.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.mise.toml around lines 1 - 3, The .mise.toml uses strict runtime pins
([tools] with node = "24.13.1" and bun = "1.3.9") but apps/server/package.json
has a broader engines.node range; update apps/server/package.json's engines
field to match the exact pins by setting engines.node to "24.13.1" (and
engines.bun to "1.3.9" if present) so the package.json and .mise.toml enforce
the same runtime boundary, then run the usual lint/CI to verify no tooling
warnings.
Comment on lines
+239
to
+266
| function enqueue<T>(channel: MessageChannel<T>, item: T) { | ||
| const waiter = channel.waiters.shift(); | ||
| if (waiter) { | ||
| if (waiter.timeoutId !== null) clearTimeout(waiter.timeoutId); | ||
| waiter.resolve(item); | ||
| return; | ||
| } | ||
| channel.queue.push(item); | ||
| } | ||
|
|
||
| function dequeue<T>(channel: MessageChannel<T>, timeoutMs: number): Promise<T> { | ||
| const queued = channel.queue.shift(); | ||
| if (queued !== undefined) { | ||
| return Promise.resolve(queued); | ||
| } | ||
|
|
||
| return new Promise((resolve, reject) => { | ||
| const waiter = { | ||
| resolve, | ||
| reject, | ||
| timeoutId: setTimeout(() => { | ||
| const index = channel.waiters.indexOf(waiter); | ||
| if (index >= 0) channel.waiters.splice(index, 1); | ||
| reject(new Error(`Timed out waiting for WebSocket message after ${timeoutMs}ms`)); | ||
| }, timeoutMs) as ReturnType<typeof setTimeout>, | ||
| }; | ||
| channel.waiters.push(waiter); | ||
| }); |
There was a problem hiding this comment.
Don't drop non-matching messages from the shared socket queues.
The shared MessageChannel resolves the oldest waiter before sendRequest() or waitForPush() know whether the message actually matches. When those helpers continue on a different id, channel, or predicate, that message is lost permanently. Concurrent waits—or a later assertion for a push you skipped here—can then hang even though the server already sent the data. response.id === "unknown" also lets an unrelated parse error satisfy the wrong request. Route by id/channel up front instead of filtering after dequeue.
Also applies to: 337-379
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/wsServer.test.ts` around lines 239 - 266, The shared
MessageChannel currently resolves the oldest waiter unconditionally, which drops
messages that don't match the waiter's predicate/id and causes lost messages;
change the channel protocol so waiters carry a matching predicate (or expected
id/channel) and modify enqueue (and dequeue lookup) to search for a waiter whose
predicate matches the incoming item (e.g., check response.id or channel) and
only resolve that waiter; if no matching waiter exists, push the item onto
channel.queue. Likewise, when dequeue/waiting, instead of shifting the first
queued item, scan channel.queue for the first item that matches the caller's
predicate (or id/channel) and remove/return it, leaving non-matching messages in
the queue. Apply this matching-by-id/channel change to enqueue, dequeue, and the
helpers sendRequest()/waitForPush() so they provide the predicate/expected id to
the waiter rather than relying on post-filtering.
- DrainableWorker: catch failures in worker loop to prevent one bad item from permanently killing the worker fiber - wsTransport.test: loosen SyntaxError assertion to use stringContaining for cross-runtime compatibility - KeybindingsToast.browser: use exact toast count assertion (toHaveLength) instead of toBeGreaterThanOrEqual - workspaceEntries: guard stale in-flight builders by checking promise identity before writing to shared state - GitCore: resolve upstream remote name via git config to handle remotes with slashes in their names - GitManager: strip full remote prefix when deriving headBranch - GitHubCli: add optional --repo parameter to createPullRequest to prevent PR creation against wrong repository - keybindings: use atomic Ref.getAndSet for startup check-and-set - pushBus: add explicit queue shutdown finalizer - VOUCHED.td: fix alphabetical ordering - architecture.md: fix outdated push channel references to match actual WS contract (terminal.event, server.configUpdated)
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
apps/web/src/components/KeybindingsToast.browser.tsx (1)
357-357: Consider documenting the delay rationale.The 500ms timeout is a reasonable buffer, but a brief inline comment explaining why this specific duration was chosen (e.g., "allow time for any async toast emission from replayed cache") would help future maintainers understand the test intent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/KeybindingsToast.browser.tsx` at line 357, Add a brief inline comment immediately above the await new Promise((resolve) => setTimeout(resolve, 500)); in the KeybindingsToast component/test to explain why 500ms was chosen (for example: "allow time for any async toast emission from replayed cache or UI debounce"), so future maintainers understand the delay rationale; keep the comment concise and mention the specific behavior it is buffering (async toast emission, replayed cache, debounce, etc.).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.docs/architecture.md:
- Line 138: Remove the unused markdown link definition "[11]:
../packages/contracts/src/orchestration.ts" from .docs/architecture.md; locate
the exact line containing that bracketed link reference and delete it so the
document no longer contains the orphaned [11] link definition.
In `@apps/server/src/keybindings.ts`:
- Around line 546-549: The startup gate currently sets startedRef true before
startup finishes and never resets startedDeferred on failure, which permanently
wedges subsequent start/ready calls; change the start sequence (the logic that
manipulates startedRef and startedDeferred) to only set startedRef = true after
startup completes successfully, and on any failure or cancellation ensure you
reset startedRef to false and replace startedDeferred with a fresh Deferred (or
complete the old Deferred with the error) so later callers can retry; update the
cleanup around watcherScope/Effect finalizer to run in the try/finally so
failed/cancelled starts always perform the reset and recreate, and add a test
that interrupts the first start and then calls start again to verify retries
succeed (refer to startedRef, startedDeferred, watcherScope, and the start/ready
logic).
In `@apps/web/src/components/KeybindingsToast.browser.tsx`:
- Around line 366-368: The catch currently always calls mounted.cleanup() which
is wrong when mounted was already cleaned or when remounted exists; change the
cleanup to handle both mount instances and their lifecycles: when an error is
thrown, if remounted is defined call await remounted.cleanup(), otherwise if
mounted is defined call await mounted.cleanup(), and ensure that when remounted
is created you also clean it up on errors (or use nested try/finally blocks
around the code that creates/uses mounted and remounted so each gets its own
finally that calls .cleanup()) — refer to the mounted and remounted variables
and their .cleanup() calls and update the catch/finally structure accordingly.
In `@packages/shared/src/DrainableWorker.ts`:
- Around line 50-105: The worker currently only shuts down the queue and can
leave callers hanging or silently drop work; add a closed flag to the shared
state (the Ref named state) and update the finalizer (the Effect.addFinalizer
that calls Queue.shutdown(queue)) to atomically set closed=true and resolve the
current idle Deferred (use the same flow as finishOne / Deferred.succeed) so any
pending drain completes; change enqueue (the const enqueue implementation that
uses Deferred.make, Ref.update and Queue.offer) to first read the closed flag
and fail (Effect.fail) if closed, and if Queue.offer returns false ensure you
decrement outstanding and resolve the idle exactly as finishOne does instead of
silently returning success; also ensure drain (the const drain which awaits the
idle Deferred) observes the closed state by returning immediately (or resolving)
when closed so it cannot block forever.
---
Nitpick comments:
In `@apps/web/src/components/KeybindingsToast.browser.tsx`:
- Line 357: Add a brief inline comment immediately above the await new
Promise((resolve) => setTimeout(resolve, 500)); in the KeybindingsToast
component/test to explain why 500ms was chosen (for example: "allow time for any
async toast emission from replayed cache or UI debounce"), so future maintainers
understand the delay rationale; keep the comment concise and mention the
specific behavior it is buffering (async toast emission, replayed cache,
debounce, etc.).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 16508f92-4e76-49f8-ad39-5c563bb21e74
📒 Files selected for processing (12)
.docs/architecture.md.github/VOUCHED.tdapps/server/src/git/Layers/GitCore.tsapps/server/src/git/Layers/GitHubCli.tsapps/server/src/git/Layers/GitManager.tsapps/server/src/git/Services/GitHubCli.tsapps/server/src/keybindings.tsapps/server/src/workspaceEntries.tsapps/server/src/wsServer/pushBus.tsapps/web/src/components/KeybindingsToast.browser.tsxapps/web/src/wsTransport.test.tspackages/shared/src/DrainableWorker.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/server/src/git/Services/GitHubCli.ts
Comment on lines
+486
to
+491
| * Await keybindings runtime readiness. | ||
| * | ||
| * Readiness means the config directory exists, the watcher is attached, the | ||
| * startup sync has completed, and the current snapshot has been loaded. | ||
| */ | ||
| readonly ready: Effect.Effect<void, KeybindingsConfigError>; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l apps/server/src/keybindings.tsRepository: aaditagrawal/t3code
Length of output: 97
🏁 Script executed:
# First, let's see the relevant sections mentioned in the review
cat -n apps/server/src/keybindings.ts | sed -n '480,495p'Repository: aaditagrawal/t3code
Length of output: 791
🏁 Script executed:
# Check the startWatcher implementation and surrounding context (lines 826-837)
cat -n apps/server/src/keybindings.ts | sed -n '820,850p'Repository: aaditagrawal/t3code
Length of output: 1431
🏁 Script executed:
# Let's get the full start method to understand the readiness flow
ast-grep --pattern $'const start = $_'Repository: aaditagrawal/t3code
Length of output: 2924
🏁 Script executed:
# Find the startWatcher definition
cat -n apps/server/src/keybindings.ts | sed -n '810,845p'Repository: aaditagrawal/t3code
Length of output: 1702
🏁 Script executed:
# Search for where startWatcher is defined
rg -n "startWatcher\s*=" apps/server/src/keybindings.tsRepository: aaditagrawal/t3code
Length of output: 115
🏁 Script executed:
# Let's look at the full context around the watcher scope and startWatcher definition
cat -n apps/server/src/keybindings.ts | sed -n '795,840p'Repository: aaditagrawal/t3code
Length of output: 2129
🏁 Script executed:
# Let's check if there's any error handling or feedback mechanism from the forked watcher back to startup
cat -n apps/server/src/keybindings.ts | sed -n '835,870p'Repository: aaditagrawal/t3code
Length of output: 1518
🏁 Script executed:
# Look for the ready implementation to understand how it relates to start
cat -n apps/server/src/keybindings.ts | sed -n '860,900p'Repository: aaditagrawal/t3code
Length of output: 1848
🏁 Script executed:
# Check for any monitoring or recovery of the watcher scope after start
rg -n "watcherScope" apps/server/src/keybindings.tsRepository: aaditagrawal/t3code
Length of output: 296
ready can succeed even when the file watcher fails to attach.
The startWatcher effect forks the Stream.runForEach(fs.watch(...)) to a background fiber in the watcherScope (line 837) and returns void immediately. This means startWatcher completes as soon as the fork is scheduled, without waiting for the watcher to actually attach. Additionally, the entire stream operation is wrapped with Effect.ignoreCause({ log: true }), which suppresses any errors from fs.watch() startup. Since ready simply awaits the deferred that resolves when start succeeds, it can resolve without a live watcher. This violates the documented contract (line 488-489) that readiness means "the watcher is attached." On-disk edits after startup will be silently missed if the watcher never comes up.
- architecture.md: remove orphaned [11] link definition - KeybindingsToast.browser: fix error cleanup to handle correct mount instance (hoist remounted variable for catch block access) - keybindings: reset startup gate on failure so subsequent callers can retry instead of permanently wedging the service - DrainableWorker: track closed state, resolve idle deferred on shutdown so drain cannot block forever, and skip enqueue when closed
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.docs/architecture.md (1)
3-4:⚠️ Potential issue | 🟡 MinorKeep the architecture doc provider-neutral.
This fork still supports multiple providers, but these sections now describe the runtime as if
codex app-serverwere the only backend. That makes the top-level architecture misleading for the preserved OpenCode/Copilot/Amp/GeminiCli/Kilo/Claude/Cursor paths. Please describe this layer as a provider runtime/adapter boundary and mention Codex as one implementation instead of the only one.📝 Suggested wording
-T3 Code runs as a **Node.js WebSocket server** that wraps `codex app-server` (JSON-RPC over stdio) and serves a React web app. +T3 Code runs as a **Node.js WebSocket server** that serves a React web app and coordinates one of several provider runtimes/adapters (for example Codex) over provider-specific transport boundaries. -│ codex app-server │ +│ Provider runtime / adapter │ +│ (for example Codex) │ -- **Provider runtime**: `codex app-server` does the actual provider/session work. The server talks to it over JSON-RPC on stdio and translates those runtime events into the app's orchestration model. +- **Provider runtime**: A provider-specific runtime/adapter does the actual provider/session work. The server talks to it through the appropriate transport boundary and translates those runtime events into the app's orchestration model. -3. [`ProviderService`][8] starts or resumes a session and talks to `codex app-server` over JSON-RPC on stdio. +3. [`ProviderService`][8] starts or resumes a session and talks to the selected provider runtime/adapter using its transport boundary (for example JSON-RPC over stdio for Codex).Also applies to: 22-25, 34-35, 95-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.docs/architecture.md around lines 3 - 4, Summary: The architecture doc currently describes the runtime as if "codex app-server" is the only backend; change wording to be provider-neutral. Update the sentence that reads "T3 Code runs as a Node.js WebSocket server that wraps `codex app-server` (JSON-RPC over stdio) and serves a React web app" to describe a "provider runtime / adapter boundary" that can host multiple implementations, and present `codex app-server` as one example implementation rather than the sole backend; apply the same provider-neutral phrasing to the other occurrences that reference `codex app-server` verbatim (the blocks around the earlier mentions currently at lines near the top, mid, and later sections) so all references describe a generic runtime/adapter boundary and list Codex as an implementation option.
♻️ Duplicate comments (2)
apps/server/src/keybindings.ts (2)
867-869:⚠️ Potential issue | 🟠 Major
readyis permanently pinned to the first deferred instance.On Line 869,
Deferred.await(startedDeferred)is evaluated once when the service is created. After Lines 859-860 replacestartedDeferredfollowing a failed start, laterreadycalls still await the original deferred and keep surfacing the first failure even after a successful retry.Suggested fix
- ready: Deferred.await(startedDeferred), + ready: Effect.suspend(() => Deferred.await(startedDeferred)),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/keybindings.ts` around lines 867 - 869, The returned object pins ready to the original Deferred by evaluating Deferred.await(startedDeferred) once; change ready so it resolves the current startedDeferred at call time (e.g., make ready a getter or a function that returns Deferred.await(startedDeferred)) so subsequent replaces of startedDeferred (done in start or error-retry logic) are observed; update references to ready accordingly (symbols: ready, Deferred.await, startedDeferred, start).
826-860:⚠️ Potential issue | 🟠 Major
startstill detaches watcher startup from readiness and rollback.
startWatcherreturns as soon as the watch fiber is forked on Line 837, andEffect.ignoreCausesuppresses attach failures, sostart/readycan complete without a live watcher. Also, if a later step in Lines 846-860 fails, the already-forked watcher is never closed before retry, so subsequent starts can stack duplicate watchers and double-process file events. Please keep watcher acquisition in the startup path and tear down/recreatewatcherScopeon failed startup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/keybindings.ts` around lines 826 - 860, start currently forks startWatcher outside the startup transaction and suppresses attach errors; change this so the watcher acquisition/fork is performed inside the startup Effect (the same Effect called "startup") and do not use Effect.ignoreCause to hide attach failures. Specifically, move the logic that forks the watcher (startWatcher) into the startup Effect so its failure will cause startupExit to be Failure, ensure the watcher is created with watcherScope or an Effect.scoped resource so it will be closed if startup fails, and on failure explicitly close or recreate watcherScope before retrying by resetting/creating a fresh watcherScope and startedDeferred (use the existing startedRef, startedDeferred, startWatcher and watcherScope symbols to locate and modify the code).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/shared/src/DrainableWorker.ts`:
- Around line 35-36: Update the stale JSDoc on DrainableWorker to reflect the
actual public API: change the return description to state the object exposes
enqueue and drain (not queue and drain), and ensure the summary/returns mention
the `enqueue` function name and `drain` method explicitly so the docs match the
exported DrainableWorker interface and functions (`enqueue`, `drain`, and the
DrainableWorker type).
---
Outside diff comments:
In @.docs/architecture.md:
- Around line 3-4: Summary: The architecture doc currently describes the runtime
as if "codex app-server" is the only backend; change wording to be
provider-neutral. Update the sentence that reads "T3 Code runs as a Node.js
WebSocket server that wraps `codex app-server` (JSON-RPC over stdio) and serves
a React web app" to describe a "provider runtime / adapter boundary" that can
host multiple implementations, and present `codex app-server` as one example
implementation rather than the sole backend; apply the same provider-neutral
phrasing to the other occurrences that reference `codex app-server` verbatim
(the blocks around the earlier mentions currently at lines near the top, mid,
and later sections) so all references describe a generic runtime/adapter
boundary and list Codex as an implementation option.
---
Duplicate comments:
In `@apps/server/src/keybindings.ts`:
- Around line 867-869: The returned object pins ready to the original Deferred
by evaluating Deferred.await(startedDeferred) once; change ready so it resolves
the current startedDeferred at call time (e.g., make ready a getter or a
function that returns Deferred.await(startedDeferred)) so subsequent replaces of
startedDeferred (done in start or error-retry logic) are observed; update
references to ready accordingly (symbols: ready, Deferred.await,
startedDeferred, start).
- Around line 826-860: start currently forks startWatcher outside the startup
transaction and suppresses attach errors; change this so the watcher
acquisition/fork is performed inside the startup Effect (the same Effect called
"startup") and do not use Effect.ignoreCause to hide attach failures.
Specifically, move the logic that forks the watcher (startWatcher) into the
startup Effect so its failure will cause startupExit to be Failure, ensure the
watcher is created with watcherScope or an Effect.scoped resource so it will be
closed if startup fails, and on failure explicitly close or recreate
watcherScope before retrying by resetting/creating a fresh watcherScope and
startedDeferred (use the existing startedRef, startedDeferred, startWatcher and
watcherScope symbols to locate and modify the code).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5089d9d5-1833-43a4-8f9c-59131b26dcfe
📒 Files selected for processing (4)
.docs/architecture.mdapps/server/src/keybindings.tsapps/web/src/components/KeybindingsToast.browser.tsxpackages/shared/src/DrainableWorker.ts
- keybindings: use Effect.suspend for ready to observe current deferred after startup retries instead of being pinned to the first instance - architecture.md: make provider-neutral by describing runtime/adapter boundary instead of assuming codex app-server is the only backend - DrainableWorker: fix stale JSDoc (enqueue, not queue)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/shared/src/DrainableWorker.ts (1)
14-21:⚠️ Potential issue | 🟠 MajorSurface shutdown to
enqueue()callers instead of returning success.Line 93 still drops work by returning success when the worker is closed, and the Line 92
Ref.get→ Line 111Queue.offersplit leaves a shutdown race whereenqueue()can still appear to succeed while teardown is in progress. This needs one atomic state transition for “check closed + reserve outstanding”, plus an observable closed result/error for callers.Also applies to: 90-115
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/DrainableWorker.ts` around lines 14 - 21, The enqueue implementation should atomically check/mark shutdown state and reserve outstanding work instead of separately calling Ref.get then Queue.offer; change enqueue to use a single Ref.modify/modifySome operation on the worker state (the same state used by drain()) that returns either a failure indicating closed or an updated state with an incremented outstanding count, then call Queue.offer; if Queue.offer fails revert the reservation (update the Ref) before returning the failure; ensure enqueue returns a failing Effect when closed (observable to callers) rather than silently succeeding.apps/server/src/keybindings.ts (1)
486-491:⚠️ Potential issue | 🟠 Major
readystill overstates watcher readiness.
startWatcherreturns once the watch fiber is forked, and the entire watcher path is wrapped inEffect.ignoreCause({ log: true }). A failedfs.watch(...)attach still lets startup complete, soreadycan resolve without a live watcher and later file edits are silently missed. Either weaken the readiness contract or add an explicit watcher-attach handshake/error propagation before completing startup.Also applies to: 826-837
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/keybindings.ts` around lines 486 - 491, The `ready` Effect currently overstates watcher readiness because startWatcher forks the watcher fiber and uses Effect.ignoreCause({ log: true }), allowing startup to succeed even if fs.watch failed; update the startup flow so readiness only completes after the watcher is successfully attached or clearly document/weaken the contract: either (A) change startWatcher to perform an explicit attach-handshake (e.g., attempt fs.watch synchronously or return an Effect that fails on attach error) and propagate that failure into the ready Effect instead of ignoring it, or (B) alter the KeybindingsService.ready contract/Docs to state it does not guarantee watcher attachment and remove the false expectation; locate and modify startWatcher, the code wrapping Effect.ignoreCause({ log: true }), and any references to ready to implement the chosen approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/keybindings.ts`:
- Around line 548-549: The watcherScope created with Scope.make and registered
with Effect.addFinalizer is not closed when startup fails, so subsequent retries
fork additional watcher fibers into the same scope; update the startup failure
handler (the block that resets the gate and replaces the deferred) to call
Scope.close(watcherScope, Exit.void) before reopening the gate and replacing the
deferred, or alternatively refactor so the watcher is created in a per-attempt
scope that is only promoted/kept after startup succeeds (i.e., create
watcherScope per attempt and only persist it on success, otherwise close it on
failure).
---
Duplicate comments:
In `@apps/server/src/keybindings.ts`:
- Around line 486-491: The `ready` Effect currently overstates watcher readiness
because startWatcher forks the watcher fiber and uses Effect.ignoreCause({ log:
true }), allowing startup to succeed even if fs.watch failed; update the startup
flow so readiness only completes after the watcher is successfully attached or
clearly document/weaken the contract: either (A) change startWatcher to perform
an explicit attach-handshake (e.g., attempt fs.watch synchronously or return an
Effect that fails on attach error) and propagate that failure into the ready
Effect instead of ignoring it, or (B) alter the KeybindingsService.ready
contract/Docs to state it does not guarantee watcher attachment and remove the
false expectation; locate and modify startWatcher, the code wrapping
Effect.ignoreCause({ log: true }), and any references to ready to implement the
chosen approach.
In `@packages/shared/src/DrainableWorker.ts`:
- Around line 14-21: The enqueue implementation should atomically check/mark
shutdown state and reserve outstanding work instead of separately calling
Ref.get then Queue.offer; change enqueue to use a single Ref.modify/modifySome
operation on the worker state (the same state used by drain()) that returns
either a failure indicating closed or an updated state with an incremented
outstanding count, then call Queue.offer; if Queue.offer fails revert the
reservation (update the Ref) before returning the failure; ensure enqueue
returns a failing Effect when closed (observable to callers) rather than
silently succeeding.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4283f089-db93-4e56-8b66-99c7407f7162
📒 Files selected for processing (3)
.docs/architecture.mdapps/server/src/keybindings.tspackages/shared/src/DrainableWorker.ts
Prevents stacked watcher fibers when startup fails and retries. Closes the existing scope to stop the forked watcher, then creates a fresh scope for the next attempt.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/server/src/keybindings.ts (1)
853-864: Consider atomicity of state reset on startup failure.There's a subtle race window between resetting
startedRef(line 859) and replacingstartedDeferred(line 863). A concurrent caller entering after line 859 but before line 863 would capture the old (failed) deferred and proceed with startup. If their startup succeeds, they'd callDeferred.succeedon an already-failed deferred, which is a no-op—but any callers awaiting that deferred would still see the original failure.This is unlikely in practice since
startis typically called once at initialization, but for robustness you could wrap the state reset in a single atomic update or use a more structured state machine.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/keybindings.ts` around lines 853 - 864, The reset of startedRef and replacement of startedDeferred is not atomic, allowing a race where a new caller captures the old failed deferred; fix by performing the reset-and-replace in a single atomic update: convert the two separate pieces of state into one Ref of a composite object (e.g. { started: boolean, deferred: Deferred<void, KeybindingsConfigError> }) or use Ref.modify/Ref.update to set started=false and swap in a fresh Deferred in one operation (ensuring you create the new Deferred inside the atomic update or use a safe pattern to avoid races), then continue to close watcherScope and fail the original deferred with startupExit.cause as before. Ensure you update usages that read startedRef and startedDeferred to read the composite Ref (or the modified API).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/server/src/keybindings.ts`:
- Around line 853-864: The reset of startedRef and replacement of
startedDeferred is not atomic, allowing a race where a new caller captures the
old failed deferred; fix by performing the reset-and-replace in a single atomic
update: convert the two separate pieces of state into one Ref of a composite
object (e.g. { started: boolean, deferred: Deferred<void,
KeybindingsConfigError> }) or use Ref.modify/Ref.update to set started=false and
swap in a fresh Deferred in one operation (ensuring you create the new Deferred
inside the atomic update or use a safe pattern to avoid races), then continue to
close watcherScope and fail the original deferred with startupExit.cause as
before. Ensure you update usages that read startedRef and startedDeferred to
read the composite Ref (or the modified API).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 57b3a07f-c9cc-4dac-b408-deec24f5263b
📒 Files selected for processing (1)
apps/server/src/keybindings.ts
This was referenced Mar 12, 2026
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pingdotgg/t3codemain branchConflict Resolutions
Schema.is(ProviderKind)validation (upstream hardcoded=== "codex"), keptthread.deletedhandler and provider options caching, adopted upstream'sDrainableWorkerpatternpushBusbroadcasting abstraction while preserving all multi-provider model/usage handlers (OpenCode, Copilot, Amp, GeminiCli, Kilo, Claude, Cursor, Codex)Verification
bun typecheckpasses (7/7 packages)bun lintpasses (0 errors)Summary by CodeRabbit
New Features
Bug Fixes
Chores / Tests