sync: merge upstream/main v0.0.21 (17 commits)#75
Merged
aaditagrawal merged 21 commits intomainfrom Apr 25, 2026
Merged
Conversation
Co-authored-by: Julius Marminge <julius0216@outlook.com>
…f42fba0` (pingdotgg#2276) Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Heinz Gericke <115458264+heinzgericke@users.noreply.github.com>
Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Julius Marminge <juliusmarminge@users.noreply.github.com>
Co-authored-by: macroscopeapp[bot] <170038800+macroscopeapp[bot]@users.noreply.github.com>
Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: cursor[bot] <206951365+cursor[bot]@users.noreply.github.com> Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Julius Marminge <juliusmarminge@users.noreply.github.com> Co-authored-by: codex <codex@users.noreply.github.com>
Upstream changes pulled in (take or adapt): - 00b5c3e Add task sidebar auto-open setting (autoOpenPlanSidebar) - 188df6d Fix Claude session cwd resume drift (adapter + reactor) - 0d55a42 Ignore stale runtime projection snapshots - 0ee302e Add dynamic_tool_call to permission request - d5b7690 Exclude subscribe RPCs from latency tracking - b0b7b38 Detect localized Windows command errors - fd3b96b Add IntelliJ project icon to favicon paths - aa2d385 Restore CODEX_HOME tilde expansion - e25db3a Fix provider cache atomic write temp path collisions - b8305af Increase Claude auth probe timeout to 10s - b7c89cf Refresh Codex protocol bindings - 3a1daa8 Add close buttons to toasts - 055897f Enforce opencode >= 1.14.19 + Wayland reveal window - 40b3a80 Trim OpenCode provider model names - f6978db Pass through PATHEXT in turbo Fork features preserved: - Provider-specific typed ModelOptions (Codex/Claude/Cursor/OpenCode/ Copilot/GeminiCli/Amp/Kilo) kept; upstream's 8d1d699 refactor toward generic Array<{id, value}> option shape rejected in contracts, shared, server adapters, and web UI. - Dropped upstream's 026_CanonicalizeModelSelectionOptions migration (would corrupt fork's provider-specific on-disk format) and the unused builtInProviderCatalog helper. - Migration registration keeps fork IDs 23/24 (NormalizeLegacyProviderKinds, RepairProjectionThreadProposedPlanImplementationColumns); upstream's new migrations remain at fork's 25/26/27. - buildServerProvider() made presentation optional so fork's other providers don't need to adopt upstream's new presentation object. - composerProviderState.{tsx,test.tsx} dropped; fork uses composerProviderRegistry (provider-specific dispatch).
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 49 minutes and 54 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds version bumps across packages; introduces a bindFirstRevealTrigger utility and tests to coordinate multi-event window reveal; adds an Effect-based atomic file-write helper and refactors multiple persistence paths to use it; extends provider metadata/presentation and session cwd tracking with telemetry; refactors toast UI with stackedThreadToast and widespread usage; adds shell event gating and multiple tests and minor tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant Main as Main.ts
participant Window as BrowserWindow
participant WebContents as webContents
participant Coordinator as bindFirstRevealTrigger
participant Reveal as revealWindow()
Main->>Window: create hidden BrowserWindow
Main->>Coordinator: register subscribers (ready-to-show, did-finish-load on Linux)
par Platform subscriptions
Coordinator->>Window: window.once("ready-to-show")
Coordinator->>WebContents: webContents.once("did-finish-load")
end
alt ready-to-show fires first
Window->>Coordinator: ready-to-show
else did-finish-load fires first
WebContents->>Coordinator: did-finish-load
end
Coordinator->>Coordinator: if not revealed -> call reveal()
rect rgba(100,150,200,0.5)
Coordinator->>Reveal: revealWindow()
Reveal->>Window: show() + focus()
end
Coordinator->>Coordinator: mark revealed = true
Note over Coordinator: Subsequent events ignored
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: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: 5
🧹 Nitpick comments (10)
apps/server/src/processRunner.test.ts (1)
25-41: Consider parameterizing to cover all 6 locale patterns.Production code adds 6 localized regexes but the test only exercises the German one. A future edit that breaks one of the other five (e.g., the wildcard placement in Portuguese/Italian/French) would slip through.
it.eachkeeps this cheap:♻️ Proposed parameterized test
describe("isWindowsCommandNotFound", () => { - it("matches the localized German cmd.exe error text", () => { - const originalPlatform = process.platform; - Object.defineProperty(process, "platform", { value: "win32", configurable: true }); - - try { - expect( - isWindowsCommandNotFound( - 1, - "wird nicht als interner oder externer Befehl, betriebsfahiges Programm oder Batch-Datei erkannt", - ), - ).toBe(true); - } finally { - Object.defineProperty(process, "platform", { value: originalPlatform, configurable: true }); - } - }); + const cases: ReadonlyArray<readonly [string, string]> = [ + ["English", "'foo' is not recognized as an internal or external command, operable program or batch file."], + ["Portuguese", "'foo' não é reconhecido como um comando interno ou externo, programa operável ou arquivo em lotes."], + ["Italian", "'foo' non è riconosciuto come comando interno o esterno, un programma eseguibile o un file batch."], + ["French", "'foo' n'est pas reconnu en tant que commande interne ou externe, un programme exécutable ou un fichier de commandes."], + ["Spanish", "'foo' no se reconoce como un comando interno o externo, programa o archivo por lotes ejecutable."], + ["German", "'foo' wird nicht als interner oder externer Befehl, betriebsfähiges Programm oder Batch-Datei erkannt."], + ]; + + it.each(cases)("matches localized cmd.exe error text (%s)", (_locale, stderr) => { + const originalPlatform = process.platform; + Object.defineProperty(process, "platform", { value: "win32", configurable: true }); + try { + expect(isWindowsCommandNotFound(1, stderr)).toBe(true); + } finally { + Object.defineProperty(process, "platform", { value: originalPlatform, configurable: true }); + } + }); });Minor nit unrelated to the above: the current German fixture uses
betriebsfahiges(no umlaut); the realcmd.exemessage isbetriebsfähiges. The test still passes because the pattern terminates atbefehl, but using the authentic text makes the fixture more representative.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/processRunner.test.ts` around lines 25 - 41, Update the test for isWindowsCommandNotFound to parameterize inputs for all six localized cmd.exe error messages (rather than only the German fixture) using a table-driven it.each so each locale regex is exercised; include the actual localized strings used in production (fix the German fixture to use "betriebsfähiges" with the umlaut) and assert each returns true for exit code 1, restoring process.platform after each case as currently done.apps/server/src/processRunner.ts (1)
40-57: LGTM — centralized multi-locale detection is a clean improvement.The
.wildcard deliberately tolerates diacritics/apostrophes (n.o→ "não",n.est→ "n'est",.→ " é "), which is pragmatic and unlikely to produce false positives against realcmd.exeoutput. Thecode === 9009fast-path still covers cases where stderr is empty or uses a locale not enumerated here (CJK, RU, PL, NL, etc.).Optional follow-up: if you later see reports from non-covered locales where 9009 isn’t emitted (e.g., some PowerShell or ConEmu wrappers), consider extending the pattern list or anchoring on the shared English token
Befehl/command/comandoacross locales.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/processRunner.ts` around lines 40 - 57, No code change required—the multi-locale detection in WINDOWS_COMMAND_NOT_FOUND_PATTERNS with helper hasWindowsCommandNotFoundMessage and the isWindowsCommandNotFound fast-path (code === 9009) is correct; if you later encounter missed locales (e.g., PowerShell/ConEmu wrappers) extend WINDOWS_COMMAND_NOT_FOUND_PATTERNS with additional locale regexes or add anchoring on shared tokens like /Befehl|command|comando/i inside hasWindowsCommandNotFoundMessage to broaden detection while keeping the 9009 fast-path in isWindowsCommandNotFound.apps/web/src/environments/runtime/service.ts (1)
192-202: Minor:markAppliedProjectionEventcarries over the prior snapshot'supdatedAtunder a higher sequence.When an event advances the sequence past the last snapshot, the stored
updatedAtis the old snapshot's timestamp paired with the new sequence. This is harmless in the current compare logic becausecompareAppliedProjectionVersiononly consultsupdatedAtwhen sequences tie — but it's slightly misleading as stored state. Consider clearingupdatedAttonullon event advance to make the invariant ("updatedAtis only meaningful for snapshots at this sequence") self-documenting:♻️ Suggested tweak
lastAppliedProjectionVersionByEnvironment.set(environmentId, { sequence, - updatedAt: currentVersion?.updatedAt ?? null, + updatedAt: null, });No behavioral change expected given the current comparator, so feel free to skip.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/environments/runtime/service.ts` around lines 192 - 202, The markAppliedProjectionEvent function currently carries over the previous snapshot's updatedAt when advancing sequence; modify it so that when sequence > currentVersion.sequence (i.e., an event advances the version) you set updatedAt to null instead of reusing currentVersion.updatedAt, preserving the invariant that updatedAt only applies to snapshot versions; refer to markAppliedProjectionEvent and the comparator compareAppliedProjectionVersion when making this change.apps/server/src/atomicWrite.ts (1)
22-23: Add fsync before rename for durability on crash.
writeFileString→renameguarantees the target path swaps atomically, but without syncing the temp file to disk before the rename, a power loss or kernel panic between the two operations can leave the renamed file present but empty or partially written. For cache files likeproviderStatusCachethis is acceptable, but forkeybindings,serverRuntimeState, andserverSettings, it means user-visible state can silently reset.Effect's FileSystem now exposes
File.sync()on file handles (available since February 2025). Refactor to useFileSystem.open()instead ofwriteFileString, then callsync()before rename to ensure data is durably written:const file = yield* fs.open(tempPath, { flag: "w" }); yield* file.write(input.contents); yield* file.sync(); yield* fs.close(file); yield* fs.rename(tempPath, input.filePath);For additional safety on POSIX systems, sync the containing directory after rename.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/atomicWrite.ts` around lines 22 - 23, The current atomic write uses fs.writeFileString(tempPath, input.contents) then fs.rename(tempPath, input.filePath) and lacks an fsync before rename; change to open the temp file (fs.open(tempPath, { flag: "w" })), write the contents with the returned File handle (file.write(input.contents)), call file.sync() to durably flush data, close the handle (fs.close(file)), then rename (fs.rename(tempPath, input.filePath)); also, after rename, optionally sync the parent directory on POSIX to ensure directory entry durability. Ensure you replace direct writeFileString usage and reference tempPath, input.contents, input.filePath, fs.open, file.write, file.sync, fs.close and fs.rename accordingly.apps/web/src/components/PlanSidebar.tsx (1)
108-121: Intentional asymmetry between success and error toasts?The error toast now routes through
stackedThreadToast(...)while the success toast on Lines 108–112 keeps the raw-payload form. If the stacking behavior is meant specifically to group repeated failures from the same save flow, this is fine — just flagging in case the success path should also be stacked for consistency with how the "Plan saved" toast coexists with a retry path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/PlanSidebar.tsx` around lines 108 - 121, The success toast currently calls toastManager.add with a raw payload ("Plan saved" and description result.relativePath) while the error path wraps via stackedThreadToast; decide and implement consistent behavior by either (A) wrapping the success toast in stackedThreadToast(...) like the error path (update the call where toastManager.add({ type: "success", title: "Plan saved", description: result.relativePath }) to toastManager.add(stackedThreadToast({ type: "success", title: "Plan saved", description: result.relativePath })) so saved-toasts stack with failures) or (B) leave the raw payload but add a clarifying comment and use the same plain format for errors (change the error branch to not use stackedThreadToast). Locate and update the calls to toastManager.add and the use of stackedThreadToast in PlanSidebar.tsx to make the paths symmetric.apps/server/src/serverSettings.ts (1)
245-263: Delegation looks correct;provideServicecalls are redundant.
fsandpathServiceare already yielded into the enclosingmakeServerSettingsscope, sowriteFileStringAtomically'sFileSystem.FileSystem | Path.Pathrequirements would be satisfied by the ambient environment without the explicitEffect.provideServicecalls. Keeping them is harmless (and matches the pattern inkeybindings.ts), butserverRuntimeState.tsandproviderStatusCache.tsopted to let the requirements propagate, so the codebase is now split on style.If you want consistency, dropping the two
provideServicecalls here (and inkeybindings.ts) would makewriteSettingsAtomicallylazier and match the leaner helpers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/serverSettings.ts` around lines 245 - 263, The writeSettingsAtomically function currently wraps writeFileStringAtomically with explicit Effect.provideService(FileSystem.FileSystem, fs) and Effect.provideService(Path.Path, pathService); remove those two provideService calls so the effect's FileSystem.FileSystem | Path.Path requirements are satisfied by the ambient environment from makeServerSettings and the helper becomes lazier and consistent with serverRuntimeState.ts/providerStatusCache.ts (also mirror the same removal in keybindings.ts if present). Keep the JSON stringify, settingsPath, and ServerSettingsError mapping intact and only delete the two Effect.provideService(...) invocations surrounding writeFileStringAtomically in writeSettingsAtomically.apps/web/src/components/ChatMarkdown.tsx (1)
350-354: Minor inconsistency: one error toast remains unwrapped.The "Open in editor is unavailable" error toast at lines 350-353 is not wrapped with
stackedThreadToast, while the sibling error paths in the same component (lines 358-364, 370-376, 389-395) are. If this was intentional (e.g., short-form errors don't need stacked layout), no action needed; otherwise consider wrapping for consistency.Also applies to: 358-364
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/ChatMarkdown.tsx` around lines 350 - 354, The error toast "Open in editor is unavailable" in ChatMarkdown.tsx is inconsistent with other error paths: wrap the toastManager.add call that shows this message with the same stackedThreadToast wrapper used in the sibling error branches (the other toastManager.add calls around the open-in-editor flow), so replace the direct toastManager.add usage for that message with stackedThreadToast(...) to match the pattern used in the open-in-editor handlers (look for the open-in-editor related function and the other toastManager.add calls near it).apps/web/src/components/settings/SettingsPanels.tsx (1)
1306-1309: Third fallback is effectively unreachable.Every entry in
PROVIDER_SETTINGS(lines 127–195) hard‑codes a non‑emptytitle, soproviderCard.titleis always truthy andformatProviderKindLabel(providerCard.provider)can never run. Not a bug — just note the defense‑in‑depth path is dead today. If you want the label to actually trigger for non‑built‑in providers in the future you'd needproviderCard.titleto become optional; otherwise consider dropping the third arm to avoid importingformatProviderKindLabeljust for this site.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/settings/SettingsPanels.tsx` around lines 1306 - 1309, providerDisplayName currently uses three fallbacks but the third (formatProviderKindLabel(providerCard.provider)) is unreachable because every entry in PROVIDER_SETTINGS supplies a non-empty providerCard.title; remove the dead fallback and the import of formatProviderKindLabel to simplify the code, or alternatively make providerCard.title optional in PROVIDER_SETTINGS and ensure callers handle when title is missing (update providerDisplayName logic to keep the formatProviderKindLabel fallback if you choose the optional-title route).apps/web/src/providerModels.ts (1)
16-22: Acronym casing is lost (e.g.geminiCli→ "Gemini Cli").
formatProviderKindLabel("geminiCli")produces"Gemini Cli"rather than"Gemini CLI". InSettingsPanelsthis is masked byproviderCard.titlealways winning, butProviderStatusBannerfalls through to this helper when the server snapshot omitsdisplayName, so users can see the lower‑cased acronym there. Optional: normalize a small allow‑list (cli,api) to uppercase after title‑casing, or add a per‑kind override map for the known providers.💡 Illustrative tweak
export function formatProviderKindLabel(provider: ProviderKind): string { - return provider + const label = provider .replace(/([a-z])([A-Z])/g, "$1 $2") .replace(/[_-]+/g, " ") .trim() .replace(/\b\w/g, (char) => char.toUpperCase()); + return label.replace(/\b(Cli|Api)\b/g, (word) => word.toUpperCase()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/providerModels.ts` around lines 16 - 22, The formatting function formatProviderKindLabel currently title-cases identifiers like "geminiCli" into "Gemini Cli", losing acronym casing; update formatProviderKindLabel to post-process the title-cased string by either applying a small allow-list of acronyms (e.g., "CLI", "API") to uppercase or by consulting a per-kind override map for known providers (e.g., map "geminiCli" → "Gemini CLI") so the returned label preserves correct uppercase acronyms while keeping the existing spacing and capitalization logic.apps/web/src/components/ui/toast.tsx (1)
113-228: Expandable toast sections look solid, one small a11y nit.
ToastExpandableSectionandToastDescriptionAndExpandableare well‑structured — state is isolated per‑toast,aria-expandedis wired, andEnter/SpacepreventDefaultis correct (Space would otherwise scroll).Minor: in the
expandableDescriptionTriggerbranch (lines 186–224), the outerrole="button"div relies on theToast.Descriptiontext content for its accessible name; ifToast.Descriptionever renders with anaria-hiddenwrapper or empty text, the button becomes unlabeled. Adding a descriptivearia-label(e.g. derived from the title or a static "Toggle error details") would be more robust than leaning ontitlefor screen readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/ui/toast.tsx` around lines 113 - 228, The interactive container in ToastDescriptionAndExpandable currently relies on the visible Toast.Description for its accessible name, which can be empty or hidden; fix it by adding an explicit aria-label to the role="button" div (use the existing expandLabel/collapseLabel and open state) so screen readers always get a clear name (e.g. aria-label={open ? collapseLabel : expandLabel} with a fallback like "Toggle details"); update the element in ToastDescriptionAndExpandable (referencing expandLabel, collapseLabel, and open) accordingly.
🤖 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/provider/Layers/ClaudeAdapter.ts`:
- Around line 3010-3033: The trace is emitting raw extraArgs by serializing
extraArgs into the Effect.annotateCurrentSpan attribute
"claude.query.extra_args_json", which may contain secrets; modify the code
around Effect.annotateCurrentSpan in ClaudeAdapter.ts to avoid logging sensitive
fields by either removing "claude.query.extra_args_json" entirely or replacing
it with a redacted/filtered representation (e.g., only include a allowlist of
non-secret keys or a boolean flag like "claude.query.extra_args_present"),
ensure any helper that serializes extraArgs (referenced as extraArgs) strips or
masks credentials before producing a string, and keep other attributes
(settings, model, session ids) unchanged.
In `@apps/server/src/provider/Layers/ProviderService.test.ts`:
- Line 965: The test asserts runtimePayload.cwd equals "/tmp/project-send-turn"
but startSession in this test does not set cwd, making the expectation
order-dependent; fix by making the test set or control cwd explicitly—either
pass cwd="/tmp/project-send-turn" into the startSession call used in this test
or change the assertion to validate a deterministic property (e.g., assert
runtimePayload.cwd is defined or matches path.resolve(expected)) so the test no
longer relies on prior state; update the assertion referencing runtimePayload
and the startSession setup in ProviderService.test.ts accordingly.
In `@apps/web/src/components/ui/toast.logic.ts`:
- Around line 78-84: When computing frontmostHeight, preserve the last visible
toast's height while the final toasts are in "ending" state: change the
selection logic that sets frontmostLiveToast so it first finds a toast with
transitionStatus !== "ending" and, if none exists, falls back to the frontmost
visible toast (e.g., visibleToasts[0] or the appropriate frontmost element)
instead of leaving it undefined; then keep using
normalizeToastHeight(frontmostLiveToast?.height) to compute frontmostHeight.
This ensures frontmostHeight doesn't drop to 0 while the last exit animation
runs and references the existing symbols frontmostLiveToast, visibleToasts,
frontmostHeight, and normalizeToastHeight.
In `@apps/web/src/components/ui/toast.tsx`:
- Around line 578-588: The corner dismiss button (inside Toast.Root) remains
visible and clickable even when Toast.Content is collapsed via
hideCollapsedContent; update the DOM/props so the dismiss wrapper uses the same
collapse behavior — apply hideCollapsedContent (or conditional class based on
Toast state) to the element using toastCornerDismissClass (or to the button
using toastCornerOrbClass) so behind/peek toasts hide the orb; ensure you still
call toastManager.close(toast.id) on click but only allow the orb to be
visible/interactive for the frontmost toast (match the visibility logic used for
Toast.Content).
In `@apps/web/src/rpc/requestLatencyState.ts`:
- Around line 38-40: The current shouldTrackRpcAck function uses
tag.includes("subscribe") which falsely excludes tags like "thread/unsubscribe";
update it to only exclude true subscribe operations by using
tag.startsWith("subscribe") or, for namespaced cases, match a segment with a
regex like /(^|[.\/:])subscribe/i so only tags that begin (or have a clear
segment) of "subscribe" are skipped; modify the logic inside shouldTrackRpcAck
accordingly.
---
Nitpick comments:
In `@apps/server/src/atomicWrite.ts`:
- Around line 22-23: The current atomic write uses fs.writeFileString(tempPath,
input.contents) then fs.rename(tempPath, input.filePath) and lacks an fsync
before rename; change to open the temp file (fs.open(tempPath, { flag: "w" })),
write the contents with the returned File handle (file.write(input.contents)),
call file.sync() to durably flush data, close the handle (fs.close(file)), then
rename (fs.rename(tempPath, input.filePath)); also, after rename, optionally
sync the parent directory on POSIX to ensure directory entry durability. Ensure
you replace direct writeFileString usage and reference tempPath, input.contents,
input.filePath, fs.open, file.write, file.sync, fs.close and fs.rename
accordingly.
In `@apps/server/src/processRunner.test.ts`:
- Around line 25-41: Update the test for isWindowsCommandNotFound to
parameterize inputs for all six localized cmd.exe error messages (rather than
only the German fixture) using a table-driven it.each so each locale regex is
exercised; include the actual localized strings used in production (fix the
German fixture to use "betriebsfähiges" with the umlaut) and assert each returns
true for exit code 1, restoring process.platform after each case as currently
done.
In `@apps/server/src/processRunner.ts`:
- Around line 40-57: No code change required—the multi-locale detection in
WINDOWS_COMMAND_NOT_FOUND_PATTERNS with helper hasWindowsCommandNotFoundMessage
and the isWindowsCommandNotFound fast-path (code === 9009) is correct; if you
later encounter missed locales (e.g., PowerShell/ConEmu wrappers) extend
WINDOWS_COMMAND_NOT_FOUND_PATTERNS with additional locale regexes or add
anchoring on shared tokens like /Befehl|command|comando/i inside
hasWindowsCommandNotFoundMessage to broaden detection while keeping the 9009
fast-path in isWindowsCommandNotFound.
In `@apps/server/src/serverSettings.ts`:
- Around line 245-263: The writeSettingsAtomically function currently wraps
writeFileStringAtomically with explicit
Effect.provideService(FileSystem.FileSystem, fs) and
Effect.provideService(Path.Path, pathService); remove those two provideService
calls so the effect's FileSystem.FileSystem | Path.Path requirements are
satisfied by the ambient environment from makeServerSettings and the helper
becomes lazier and consistent with serverRuntimeState.ts/providerStatusCache.ts
(also mirror the same removal in keybindings.ts if present). Keep the JSON
stringify, settingsPath, and ServerSettingsError mapping intact and only delete
the two Effect.provideService(...) invocations surrounding
writeFileStringAtomically in writeSettingsAtomically.
In `@apps/web/src/components/ChatMarkdown.tsx`:
- Around line 350-354: The error toast "Open in editor is unavailable" in
ChatMarkdown.tsx is inconsistent with other error paths: wrap the
toastManager.add call that shows this message with the same stackedThreadToast
wrapper used in the sibling error branches (the other toastManager.add calls
around the open-in-editor flow), so replace the direct toastManager.add usage
for that message with stackedThreadToast(...) to match the pattern used in the
open-in-editor handlers (look for the open-in-editor related function and the
other toastManager.add calls near it).
In `@apps/web/src/components/PlanSidebar.tsx`:
- Around line 108-121: The success toast currently calls toastManager.add with a
raw payload ("Plan saved" and description result.relativePath) while the error
path wraps via stackedThreadToast; decide and implement consistent behavior by
either (A) wrapping the success toast in stackedThreadToast(...) like the error
path (update the call where toastManager.add({ type: "success", title: "Plan
saved", description: result.relativePath }) to
toastManager.add(stackedThreadToast({ type: "success", title: "Plan saved",
description: result.relativePath })) so saved-toasts stack with failures) or (B)
leave the raw payload but add a clarifying comment and use the same plain format
for errors (change the error branch to not use stackedThreadToast). Locate and
update the calls to toastManager.add and the use of stackedThreadToast in
PlanSidebar.tsx to make the paths symmetric.
In `@apps/web/src/components/settings/SettingsPanels.tsx`:
- Around line 1306-1309: providerDisplayName currently uses three fallbacks but
the third (formatProviderKindLabel(providerCard.provider)) is unreachable
because every entry in PROVIDER_SETTINGS supplies a non-empty
providerCard.title; remove the dead fallback and the import of
formatProviderKindLabel to simplify the code, or alternatively make
providerCard.title optional in PROVIDER_SETTINGS and ensure callers handle when
title is missing (update providerDisplayName logic to keep the
formatProviderKindLabel fallback if you choose the optional-title route).
In `@apps/web/src/components/ui/toast.tsx`:
- Around line 113-228: The interactive container in
ToastDescriptionAndExpandable currently relies on the visible Toast.Description
for its accessible name, which can be empty or hidden; fix it by adding an
explicit aria-label to the role="button" div (use the existing
expandLabel/collapseLabel and open state) so screen readers always get a clear
name (e.g. aria-label={open ? collapseLabel : expandLabel} with a fallback like
"Toggle details"); update the element in ToastDescriptionAndExpandable
(referencing expandLabel, collapseLabel, and open) accordingly.
In `@apps/web/src/environments/runtime/service.ts`:
- Around line 192-202: The markAppliedProjectionEvent function currently carries
over the previous snapshot's updatedAt when advancing sequence; modify it so
that when sequence > currentVersion.sequence (i.e., an event advances the
version) you set updatedAt to null instead of reusing currentVersion.updatedAt,
preserving the invariant that updatedAt only applies to snapshot versions; refer
to markAppliedProjectionEvent and the comparator compareAppliedProjectionVersion
when making this change.
In `@apps/web/src/providerModels.ts`:
- Around line 16-22: The formatting function formatProviderKindLabel currently
title-cases identifiers like "geminiCli" into "Gemini Cli", losing acronym
casing; update formatProviderKindLabel to post-process the title-cased string by
either applying a small allow-list of acronyms (e.g., "CLI", "API") to uppercase
or by consulting a per-kind override map for known providers (e.g., map
"geminiCli" → "Gemini CLI") so the returned label preserves correct uppercase
acronyms while keeping the existing spacing and capitalization logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2ecd2190-142f-4ba3-aab5-6fe80862a012
⛔ Files ignored due to path filters (4)
bun.lockis excluded by!**/*.lockpackages/effect-codex-app-server/src/_generated/meta.gen.tsis excluded by!**/_generated/**packages/effect-codex-app-server/src/_generated/namespaces.gen.tsis excluded by!**/_generated/**packages/effect-codex-app-server/src/_generated/schema.gen.tsis excluded by!**/_generated/**
📒 Files selected for processing (57)
.cursor/.gitignoreapps/desktop/package.jsonapps/desktop/src/clientPersistence.test.tsapps/desktop/src/main.tsapps/desktop/src/windowReveal.test.tsapps/desktop/src/windowReveal.tsapps/server/package.jsonapps/server/src/atomicWrite.tsapps/server/src/keybindings.tsapps/server/src/orchestration/Layers/ProviderCommandReactor.test.tsapps/server/src/orchestration/Layers/ProviderCommandReactor.tsapps/server/src/processRunner.test.tsapps/server/src/processRunner.tsapps/server/src/project/Layers/ProjectFaviconResolver.tsapps/server/src/provider/Layers/ClaudeAdapter.tsapps/server/src/provider/Layers/ClaudeProvider.tsapps/server/src/provider/Layers/CodexProvider.tsapps/server/src/provider/Layers/CodexSessionRuntime.tsapps/server/src/provider/Layers/ProviderRegistry.test.tsapps/server/src/provider/Layers/ProviderService.test.tsapps/server/src/provider/Layers/ProviderService.tsapps/server/src/provider/opencodeRuntime.tsapps/server/src/provider/providerSnapshot.tsapps/server/src/provider/providerStatusCache.tsapps/server/src/serverRuntimeState.tsapps/server/src/serverSettings.tsapps/web/package.jsonapps/web/src/components/BranchToolbarBranchSelector.tsxapps/web/src/components/ChatMarkdown.tsxapps/web/src/components/CommandPalette.tsxapps/web/src/components/GitActionsControl.browser.tsxapps/web/src/components/GitActionsControl.tsxapps/web/src/components/PlanSidebar.tsxapps/web/src/components/Sidebar.tsxapps/web/src/components/WebSocketConnectionSurface.tsxapps/web/src/components/chat/ProposedPlanCard.tsxapps/web/src/components/chat/ProviderStatusBanner.tsxapps/web/src/components/settings/ConnectionsSettings.tsxapps/web/src/components/settings/SettingsPanels.tsxapps/web/src/components/sidebar/SidebarUpdatePill.tsxapps/web/src/components/ui/toast.logic.test.tsapps/web/src/components/ui/toast.logic.tsapps/web/src/components/ui/toast.tsxapps/web/src/components/ui/toastHelpers.tsapps/web/src/environments/runtime/service.test.tsapps/web/src/environments/runtime/service.tsapps/web/src/hooks/useThreadActions.tsapps/web/src/localApi.test.tsapps/web/src/providerModels.tsapps/web/src/routes/__root.tsxapps/web/src/rpc/requestLatencyState.tsapps/web/src/session-logic.tspackages/contracts/package.jsonpackages/contracts/src/server.tspackages/contracts/src/settings.tspackages/effect-codex-app-server/scripts/generate.tsturbo.json
💤 Files with no reviewable changes (1)
- apps/server/src/provider/opencodeRuntime.ts
The 10min timeout was too tight — install + lint + typecheck + test (13min locally) + browser test + build doesn't fit. Hit the wall on first uncached sync/upstream branch install.
- ClaudeAdapter.ts: redact extraArgs / settings JSON from span attributes (avoid telemetry leaking secrets); record key list + arg count instead. - ProviderService.test.ts: pass cwd explicitly to startSession so the runtimePayload.cwd assertion is self-contained. - toast.logic.ts: fall back to first visible toast when all are 'ending' so frontmostHeight doesn't collapse to 0 mid-exit animation. - toast.tsx: hide corner dismiss orb on collapsed (peek) toasts so it matches the body opacity behavior. - requestLatencyState.ts: match 'subscribe' as a leading segment so RPCs like thread/unsubscribe still get tracked.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/server/src/provider/Layers/ClaudeAdapter.ts (1)
3010-3033: Past extra_args secret-leak concern is resolved; minor polish suggestions only.The previous flag on emitting
claude.query.extra_args_jsonhas been addressed — the span now records onlyclaude.query.extra_args_count(Line 3031) andclaude.query.settings_keys(Line 3030, key names only, no values). That keeps operator-supplied secrets out of trace attributes while still preserving observability signal.A couple of small, optional polish items on this attribute set:
- Line 3019:
"claude.resume.turn_count": resumeState?.turnCount ?? -1uses-1as a sentinel for "no resume cursor". For trace backends/aggregations this can skew numeric stats. Consider omitting the key entirely (or using0) whenresumeState?.turnCountisundefined, sinceEffect.annotateCurrentSpanaccepts a partial attribute record andcompactTraceAttributesalready filtersundefinedvalues (apps/server/src/observability/Attributes.ts:85-93).- Lines 3020 / 3028:
claude.query.cwdandclaude.query.additional_directoriescapture the user's working directory verbatim. For local-file tracing today this is fine, but if/when these spans are exported off-device, paths can contain usernames/PII. Worth a follow-up to gate this behind a redaction setting if traces ever leave the box.Neither is blocking — flagging as nice-to-have.
♻️ Suggested tweak for the turn_count sentinel
- "claude.resume.turn_count": resumeState?.turnCount ?? -1, + ...(typeof resumeState?.turnCount === "number" + ? { "claude.resume.turn_count": resumeState.turnCount } + : {}),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/ClaudeAdapter.ts` around lines 3010 - 3033, The span attribute "claude.resume.turn_count" currently uses -1 as a sentinel (set in the call to Effect.annotateCurrentSpan); change this so the attribute is omitted when resumeState?.turnCount is undefined (or set to 0 if you prefer non-omission) to avoid skewing numeric metrics—i.e., only include "claude.resume.turn_count" in the attributes object when resumeState?.turnCount != null; update the object construction around Effect.annotateCurrentSpan (referencing resumeState?.turnCount and "claude.resume.turn_count") and rely on compactTraceAttributes to drop undefined values.apps/web/src/components/ui/toast.tsx (2)
578-594: Past concern about the corner dismiss button on peek/behind toasts is resolved.Wrapping the orb in a div that gets
not-data-expanded:pointer-events-none not-data-expanded:opacity-0whenhideCollapsedContentis true correctly mirrors theToast.Contentcollapse behavior — the orb now fades and becomes non-interactive in lockstep with the body on non-frontmost peek items. Nice fix.One minor a11y nit (optional):
opacity-0+pointer-events-nonedoes not remove the button from the tab order nor from the accessibility tree, so a keyboard user could still tab onto a visually-hidden orb (and screen readers may announce it). If you want full parity with the visual collapse, consider gating the focusability when collapsed, e.g.:♻️ Optional: hide from AT/keyboard while collapsed
- <div - className={cn( - toastCornerDismissClass, - hideCollapsedContent && - "not-data-expanded:pointer-events-none not-data-expanded:opacity-0", - )} - > - <button - aria-label="Dismiss notification" - className={toastCornerOrbClass} - data-slot="toast-close" - onClick={() => toastManager.close(toast.id)} - type="button" - > + <div + aria-hidden={hideCollapsedContent || undefined} + className={cn( + toastCornerDismissClass, + hideCollapsedContent && + "not-data-expanded:pointer-events-none not-data-expanded:opacity-0", + )} + > + <button + aria-label="Dismiss notification" + className={toastCornerOrbClass} + data-slot="toast-close" + onClick={() => toastManager.close(toast.id)} + tabIndex={hideCollapsedContent ? -1 : 0} + type="button" + >Note: the same caveat applies to the existing
Toast.Contentcollapse pattern below, so this is just a polish item rather than a regression.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/ui/toast.tsx` around lines 578 - 594, The dismiss orb remains focusable and in the accessibility tree when collapsed; update the button inside the div (referencing toastCornerOrbClass and hideCollapsedContent) to be removed from keyboard and AT while collapsed by setting appropriate attributes when hideCollapsedContent is true — e.g., set aria-hidden={true} and tabIndex={-1} (or disabled) when collapsed so the button is not tabbable or announced; keep the existing onClick (toastManager.close(toast.id)) behavior for the non-collapsed state.
73-83: Minor: clamp threshold uses strict<, so a 180-character description still gets clamped.
description.length < ERROR_DESCRIPTION_CLAMP_MIN_CHARSreturnsundefinedonly for lengths strictly under 180; at exactly 180 the clamp class is applied. If "MIN_CHARS" was intended as the inclusive minimum that triggers clamping that's correct, but the name reads like a threshold below which clamping is skipped. Either rename or flip to<=for readability — purely cosmetic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/ui/toast.tsx` around lines 73 - 83, The clamp threshold logic in errorDescriptionClampClass is off-by-one: change the length check so descriptions of exactly ERROR_DESCRIPTION_CLAMP_MIN_CHARS are treated as "too short" and not clamped; update the condition in errorDescriptionClampClass (currently checking description.length < ERROR_DESCRIPTION_CLAMP_MIN_CHARS) to use <= instead, or alternatively rename ERROR_DESCRIPTION_CLAMP_MIN_CHARS to make the inclusive behavior explicit—adjust the check in the errorDescriptionClampClass function accordingly.
🤖 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/provider/Layers/ClaudeAdapter.ts`:
- Around line 3010-3033: The span attribute "claude.resume.turn_count" currently
uses -1 as a sentinel (set in the call to Effect.annotateCurrentSpan); change
this so the attribute is omitted when resumeState?.turnCount is undefined (or
set to 0 if you prefer non-omission) to avoid skewing numeric metrics—i.e., only
include "claude.resume.turn_count" in the attributes object when
resumeState?.turnCount != null; update the object construction around
Effect.annotateCurrentSpan (referencing resumeState?.turnCount and
"claude.resume.turn_count") and rely on compactTraceAttributes to drop undefined
values.
In `@apps/web/src/components/ui/toast.tsx`:
- Around line 578-594: The dismiss orb remains focusable and in the
accessibility tree when collapsed; update the button inside the div (referencing
toastCornerOrbClass and hideCollapsedContent) to be removed from keyboard and AT
while collapsed by setting appropriate attributes when hideCollapsedContent is
true — e.g., set aria-hidden={true} and tabIndex={-1} (or disabled) when
collapsed so the button is not tabbable or announced; keep the existing onClick
(toastManager.close(toast.id)) behavior for the non-collapsed state.
- Around line 73-83: The clamp threshold logic in errorDescriptionClampClass is
off-by-one: change the length check so descriptions of exactly
ERROR_DESCRIPTION_CLAMP_MIN_CHARS are treated as "too short" and not clamped;
update the condition in errorDescriptionClampClass (currently checking
description.length < ERROR_DESCRIPTION_CLAMP_MIN_CHARS) to use <= instead, or
alternatively rename ERROR_DESCRIPTION_CLAMP_MIN_CHARS to make the inclusive
behavior explicit—adjust the check in the errorDescriptionClampClass function
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7e117e4-a3e8-46cb-98ca-4a2eb0d9bd49
📒 Files selected for processing (6)
.github/workflows/ci.ymlapps/server/src/provider/Layers/ClaudeAdapter.tsapps/server/src/provider/Layers/ProviderService.test.tsapps/web/src/components/ui/toast.logic.tsapps/web/src/components/ui/toast.tsxapps/web/src/rpc/requestLatencyState.ts
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/ci.yml
- apps/server/src/provider/Layers/ProviderService.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/rpc/requestLatencyState.ts
Previous regex required subscribe to be flanked by a delimiter, which broke 'subscribeServerConfig'-style RPC names that have no separator after 'subscribe'. Loosen to allow any continuation while still distinguishing 'thread/unsubscribe' (subscribe preceded by 'un', not a path delimiter).
3 tasks
aaditagrawal
added a commit
that referenced
this pull request
May 5, 2026
PR #79's install fix made these visible. Five distinct issues, all from PR #77's manual reconciliation of upstream's multi-provider SPI: 1. ProjectionSnapshotQuery.listThreadRows had a fork-added WHERE clause filtering by `json_extract(model_selection_json, '$.provider')` — but upstream's new SPI stores `$.instanceId` instead. The filter dropped every thread silently, so projection_threads was always empty in the snapshot and every integration-engine test hung waiting for projections that would never arrive. Removing the filter (matches upstream). 2. Migration test bounds were off-by-two. Fork's filenames 026/027/028/029 stayed put but were registered as IDs 28/29/30/31 (because fork's old 023/024 displaced them). The tests still passed numeric IDs matching the filenames, so they ran the canonicalize migration before its deps existed and blew up with `projection_thread_sessions has no column named provider_instance_id`. 3. CodexAdapter native-flush regex /NTIVE: .../ was missing the leading `A` (re-introduced by the merge from a stale upstream revision; we already fixed it once in PR #75 review). Restoring /NATIVE: .../. 4. ProviderRegistry.test asserted only the 4 upstream providers, but BUILT_IN_DRIVERS now contains all 8 (incl. fork's amp/copilot/ geminiCli/kilo). Updated the expected list. Local: `bunx vitest run integration/orchestrationEngine.integration.test.ts` goes from 11 failed/1 skipped → 11 passed/1 skipped.
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
13 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
Brings fork up to upstream
pingdotgg/t3code@v0.0.21(ada410bc). 17 upstream commits merged.Upstream changes pulled in
00b5c3e1Task sidebar auto-open setting (autoOpenPlanSidebar)188df6daFix Claude session cwd resume drift (ClaudeAdapter + ProviderCommandReactor)0d55a428Ignore stale runtime projection snapshots0ee302e2Adddynamic_tool_callto permission requestd5b7690fExclude subscribe RPCs from latency trackingb0b7b38dDetect localized Windows command errorsfd3b96b4Add IntelliJ project icon to favicon pathsaa2d385aRestoreCODEX_HOMEtilde expansione25db3a5Fix provider cache atomic write temp path collisionsb8305afaIncrease Claude auth probe timeout to 10s (AUTH_PROBE_TIMEOUT_MS)b7c89cf4Refresh Codex protocol bindings (ChatGPT Pro 20x/prolitelabels)3a1daa87Close buttons on toasts (toastHelpers.ts,stackedThreadToast)055897f0Enforceopencode >= 1.14.19+ Wayland window reveal40b3a800Trim OpenCode provider model namesf6978db6PassPATHEXTthrough turboFork features preserved
ModelOptionskept across contracts, shared, server adapters, and web UI. Upstream commit8d1d699fmoves toward a genericArray<{id, value}>option shape — the fork already evolved past that to typed structs (CodexModelOptions,ClaudeModelOptions,CursorModelOptions,OpencodeModelOptions,CopilotModelOptions,GeminiCliModelOptions,AmpModelOptions,KiloModelOptions). The fork's direction is kept.026_CanonicalizeModelSelectionOptions— would rewrite stored options to upstream's array shape and corrupt fork's provider-specific on-disk format.023 NormalizeLegacyProviderKinds/024 RepairProjectionThreadProposedPlanImplementationColumnsstay put; upstream's new migrations register at fork IDs 25/26/27.buildServerProvider()presentation made optional so fork's non-Claude providers don't all need an upstream-style presentation object.composerProviderState.{tsx,test.tsx}removed; fork usescomposerProviderRegistry(provider-specific dispatch).builtInProviderCatalog.ts.Verification
bun typecheck— all 10 packages pass.bun run fmt— clean.bun lint— 0 errors (33 pre-existing warnings, same as before merge).bun run test— previously 2 upstream-related failures (stale cwd assertion inProviderService.test.ts; missing cwd inProviderCommandReactor.test.tsmock session); both fixed in this branch.Test plan
trueSummary by CodeRabbit
New Features
Bug Fixes
Improvements