feat: new features with code review fixes#2
Merged
aaditagrawal merged 3 commits intomainfrom Mar 9, 2026
Merged
Conversation
- Add Copilot turn tracking, cursor usage, command palette, plan sidebar, ghostty terminal split view, project thread navigation, and session text generation - Fix ProviderStartOptions schema: restore opencode/kilo fields and value re-export - Fix CopilotAdapter: restore concurrent-turn guard, session cleanup on failure, lastError lifecycle, teardown event emission, pending resolver cleanup, reconfigure error guard, attachment validation Effect channel - Fix orchestration: interrupt re-raise in approval/user-input handlers, threadProviderOptions cleanup on stop and delete - Fix frontend: CommandPalette render mutation, stale drag closure, assistantDeliveryMode dedup, timeout cleanups, onInterrupt memoization, PlanSidebar duplicate keys, settings navigation, onDragLeave handler - Fix server: satisfies vs as type safety, stopSession error logging, os.homedir() for macOS paths, editor app detection/launch mismatch
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds multi-provider support and session-scoped text generation; wires provider start options through orchestration, runtime, adapters, and contracts; expands provider discovery/usage (Cursor, Copilot, Claude Code); introduces many web UI features (command palette, plan sidebar, Ghostty), build/release scripts, CI vouch workflow, and numerous tests. Changes
Sequence Diagram(s)sequenceDiagram
participant GitManager as GitManager
participant SessionGen as SessionTextGeneration
participant Provider as ProviderSession
participant Repo as Repo/Git
rect rgba(52,152,219,0.5)
GitManager->>SessionGen: request generateCommitMessage(input, provider?, model?)
SessionGen->>Provider: open session + stream events (thread-scoped)
Provider-->>SessionGen: streaming deltas / JSON chunks
SessionGen->>SessionGen: assemble & validate JSON, sanitize
SessionGen-->>GitManager: structured commit/pr suggestion
GitManager->>Repo: create branch/commit/pr with generated content
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/wsTransport.ts (1)
41-47:⚠️ Potential issue | 🟠 MajorAvoid building
ws://host:when the page is on the default port.
window.location.portis empty on 80/443, so this fallback becomesws://example.com:/wss://example.com:andnew WebSocket()will reject it. Usewindow.location.hostso the port is only included when it exists.Suggested fix
this.url = url ?? (bridgeUrl && bridgeUrl.length > 0 ? bridgeUrl : envUrl && envUrl.length > 0 ? envUrl - : `${window.location.protocol === "https:" ? "wss" : "ws"}://${window.location.hostname}:${window.location.port}`); + : `${window.location.protocol === "https:" ? "wss" : "ws"}://${window.location.host}`);🤖 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 41 - 47, The URL fallback in the WebSocket transport builds a host:port string using window.location.port which is empty on default ports and produces an invalid trailing colon; update the construction of this.url (in wsTransport.ts, the assignment to this.url) to use window.location.host instead of window.location.port so the host includes the port only when present (i.e., use `${window.location.protocol === "https:" ? "wss" : "ws"}://${window.location.host}`) and keep the existing bridgeUrl/envUrl precedence logic unchanged.
🟡 Minor comments (8)
apps/server/src/provider/Layers/CodexAdapter.ts-1534-1534 (1)
1534-1534:⚠️ Potential issue | 🟡 MinorUse nullish checks for numeric rate-limit fields.
Line 1534 and Line 1565 currently treat
0as “missing”. If the provider ever returns0forresetsAtorwindowDurationMins, we’ll drop the reset timestamp and fall back to the generic"Session"label instead of formatting the actual value.🩹 Proposed fix
- const resetsAt = bucket.resetsAt ? new Date(bucket.resetsAt * 1000).toISOString() : undefined; + const resetsAt = + bucket.resetsAt != null ? new Date(bucket.resetsAt * 1000).toISOString() : undefined; ... - const sessionLabel = limits.primary?.windowDurationMins - ? formatCodexSessionWindowLabel(limits.primary.windowDurationMins) - : "Session"; + const sessionWindowDurationMins = limits.primary?.windowDurationMins; + const sessionLabel = + sessionWindowDurationMins != null + ? formatCodexSessionWindowLabel(sessionWindowDurationMins) + : "Session";Also applies to: 1565-1567
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/CodexAdapter.ts` at line 1534, The code currently treats numeric fields as missing via truthy checks (e.g., const resetsAt = bucket.resetsAt ? ...) which wrongly drops valid zero values; update both occurrences to use nullish checks instead so 0 is preserved — for example, guard with bucket.resetsAt != null (or the nullish coalescing approach) before constructing new Date(...) for resetsAt, and likewise check bucket.windowDurationMins != null when computing windowDurationMins/label; adjust the expressions that reference bucket.resetsAt and bucket.windowDurationMins in CodexAdapter.ts (the resetsAt and windowDurationMins assignments/labels) to use these nullish checks.apps/server/src/provider/claude-agent-sdk.d.ts-90-95 (1)
90-95:⚠️ Potential issue | 🟡 MinorRemove
readonlymodifiers fromThinkingConfigto match actual SDK types.The
ThinkingConfigandEffortLeveltype definitions are correctly structured, but the declaration file addsreadonlymodifiers that don't appear in the actual@anthropic-ai/claude-agent-sdkpackage types. The actual SDK defines:
ThinkingConfig:{ type: "adaptive" } | { type: "enabled"; budgetTokens?: number } | { type: "disabled" }EffortLevel:"low" | "medium" | "high" | "max"Remove
readonlyfrom the discriminant andbudgetTokensproperties to accurately reflect the package API and avoid stricter typing than the actual package provides.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/claude-agent-sdk.d.ts` around lines 90 - 95, The declared types in the file are too strict: remove the readonly modifiers from the ThinkingConfig union so its variants are { type: "adaptive" } | { type: "enabled"; budgetTokens?: number } | { type: "disabled" } and ensure EffortLevel remains "low" | "medium" | "high" | "max" without readonly; update the ThinkingConfig declaration (and any affected references) to match the SDK's shapes exactly by deleting readonly on the discriminant and on budgetTokens.apps/web/src/keybindings.test.ts-79-80 (1)
79-80:⚠️ Potential issue | 🟡 MinorMirror the real
commandPalette.toggleguard in this fixture.
apps/server/src/keybindings.tsLine 64 defines the default binding withwhen: "!terminalFocus", butDEFAULT_BINDINGSmakes it unconditional. These assertions will still pass even if the web matcher starts firing while terminal focus is active.🔧 Suggested test fixture update
const DEFAULT_BINDINGS = compile([ - { shortcut: modShortcut("k"), command: "commandPalette.toggle" }, + { + shortcut: modShortcut("k"), + command: "commandPalette.toggle", + whenAst: whenNot(whenIdentifier("terminalFocus")), + }, { shortcut: modShortcut("j"), command: "terminal.toggle" },assert.isFalse( isCommandPaletteShortcut(event({ key: "k", metaKey: true }), DEFAULT_BINDINGS, { platform: "MacIntel", context: { terminalFocus: true }, }), );Also applies to: 254-265
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/keybindings.test.ts` around lines 79 - 80, The test fixture DEFAULT_BINDINGS is missing the real guard used by the app for the "commandPalette.toggle" binding, so update the compiled bindings (the object passed into compile / DEFAULT_BINDINGS) to include when: "!terminalFocus" for the entry with command "commandPalette.toggle"; likewise update any other test fixtures around the later block (the other DEFAULT_BINDINGS usage at lines 254-265) so that isCommandPaletteShortcut calls use bindings that include the when: "!terminalFocus" guard (this will ensure events with terminalFocus true do not match).apps/web/src/components/CommandPalette.tsx-382-393 (1)
382-393:⚠️ Potential issue | 🟡 MinorShow bound shortcuts for script commands too.
packages/contracts/src/keybindings.tsLines 22-34 already allowscript.<id>.runcommands, but these items never populateshortcut. That makes user-defined script bindings invisible in the palette even though the action items surface theirs.packages/contracts/src/model.ts-19-21 (1)
19-21:⚠️ Potential issue | 🟡 MinorCopilot reasoning support is only half-wired here.
This schema now accepts
modelOptions.copilot.reasoningEffort, butREASONING_EFFORT_OPTIONS_BY_PROVIDER.copilotandDEFAULT_REASONING_EFFORT_BY_PROVIDER.copilotstill advertise no reasoning support on Lines 319-339. Any picker/helper built from those tables will stay out of sync with the contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/contracts/src/model.ts` around lines 19 - 21, The schema CopilotModelOptions now allows modelOptions.copilot.reasoningEffort, but the lookup tables REASONING_EFFORT_OPTIONS_BY_PROVIDER and DEFAULT_REASONING_EFFORT_BY_PROVIDER still omit a "copilot" entry; update those constants to include a "copilot" key whose options are CODEX_REASONING_EFFORT_OPTIONS (or the equivalent array/enum used elsewhere) and set an appropriate default value matching DEFAULT_REASONING_EFFORT_BY_PROVIDER's shape (e.g., the same default used for codex if intended). Touch the symbols REASONING_EFFORT_OPTIONS_BY_PROVIDER and DEFAULT_REASONING_EFFORT_BY_PROVIDER to add the copilot mapping so any pickers/helpers stay in sync with CopilotModelOptions.packages/contracts/src/git.ts-57-58 (1)
57-58:⚠️ Potential issue | 🟡 MinorUse the same non-empty model contract here.
modelis nowSchema.String, so""and whitespace-only overrides are accepted even though the rest of the contracts useTrimmedNonEmptyStringfor model selection. That makes it easy to pass an explicit-but-invalid model into the git generation path.Suggested fix
featureBranch: Schema.optional(Schema.Boolean), provider: Schema.optional(ProviderKind), - model: Schema.optional(Schema.String), + model: Schema.optional(TrimmedNonEmptyStringSchema), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/contracts/src/git.ts` around lines 57 - 58, The model field currently uses Schema.optional(Schema.String) which allows empty or whitespace-only values; change the model contract to use the same non-empty trimmed string type (e.g., Schema.optional(TrimmedNonEmptyString)) so empty/whitespace model values are rejected; update the import/usage to reference TrimmedNonEmptyString alongside ProviderKind and ensure any validations relying on model continue to work with the new type.apps/web/src/index.css-39-39 (1)
39-39:⚠️ Potential issue | 🟡 MinorUse the new mono token at the call site.
--font-monois introduced on Line 39 but never used. Line 166 still hardcodes the identical font stack in thepre, code, textarea, inputselector, creating duplicate sources of truth.Update Line 166 to use
font-family: var(--font-mono);instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/index.css` at line 39, Replace the hard-coded mono font stack in the pre, code, textarea, input selector with the new CSS variable --font-mono: find the selector that currently sets the font-family to "Geist Mono", "SF Mono", "SFMono-Regular", Consolas, "Liberation Mono", Menlo, monospace and change it to use font-family: var(--font-mono); so the --font-mono token is the single source of truth for the mono font stack.apps/web/src/components/ProjectScriptsControl.tsx-169-169 (1)
169-169:⚠️ Potential issue | 🟡 MinorReset the delete confirmation state when the edit dialog resets.
deleteConfirmOpenis controlled independently fromdialogOpen, but the main dialog’s close/reset path never clears it. If the editor closes while the confirmation is open, the alert can stay mounted and bleed into the next add/edit session.♻️ Proposed fix
const openAddDialog = () => { + setDeleteConfirmOpen(false); setEditingScriptId(null); setName(""); setCommand(""); @@ const openEditDialog = (script: ProjectScript) => { + setDeleteConfirmOpen(false); setEditingScriptId(script.id); setName(script.name); @@ onOpenChangeComplete={(open) => { if (open) return; + setDeleteConfirmOpen(false); setEditingScriptId(null); setName("");Also applies to: 488-501
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/ProjectScriptsControl.tsx` at line 169, The deleteConfirmOpen state is not reset when the edit/add dialog is closed, so ensure you setDeleteConfirmOpen(false) whenever the dialog resets/closes: add the reset call inside the same close/reset handler used for dialogOpen (e.g., the function that closes the editor modal) or add a useEffect that watches dialogOpen and clears deleteConfirmOpen when dialogOpen becomes false; apply the same change to the other dialog instance that manages deleteConfirmOpen so the confirmation alert cannot persist across sessions.
🧹 Nitpick comments (19)
apps/server/src/checkpointing/Utils.ts (1)
14-41: Consider extracting shared input type.Both
resolveThreadWorkspaceCwdandresolveExistingThreadWorkspaceCwduse identical input types. Extracting a shared type alias would reduce duplication.♻️ Optional: Extract shared type
+type ThreadWorkspaceInput = { + readonly thread: { + readonly projectId: ProjectId; + readonly worktreePath: string | null; + }; + readonly projects: ReadonlyArray<{ + readonly id: ProjectId; + readonly workspaceRoot: string; + }>; +}; + -export function resolveThreadWorkspaceCwd(input: { - readonly thread: { - readonly projectId: ProjectId; - readonly worktreePath: string | null; - }; - readonly projects: ReadonlyArray<{ - readonly id: ProjectId; - readonly workspaceRoot: string; - }>; -}): string | undefined { +export function resolveThreadWorkspaceCwd(input: ThreadWorkspaceInput): string | undefined {Then reuse
ThreadWorkspaceInputforresolveExistingThreadWorkspaceCwd.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/checkpointing/Utils.ts` around lines 14 - 41, Extract a shared input type alias (e.g., ThreadWorkspaceInput) that captures the repeated shape used by resolveThreadWorkspaceCwd and resolveExistingThreadWorkspaceCwd (thread with projectId and worktreePath, and projects array with id and workspaceRoot), then replace the concrete inline types in both function signatures with that alias so both functions reference the same type definition.apps/web/src/appSettings.ts (1)
98-101: DeriveBuiltInAppModelOptionfromAppModelOptionto avoid drift.This duplicates the
slug/nameshape already exported byAppModelOption. Using a derived type keeps both declarations aligned if those shared fields ever change.♻️ Proposed refactor
-export interface BuiltInAppModelOption { - slug: string; - name: string; -} +export type BuiltInAppModelOption = Pick<AppModelOption, "slug" | "name">;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/appSettings.ts` around lines 98 - 101, The BuiltInAppModelOption type duplicates fields from AppModelOption; change BuiltInAppModelOption to be an alias or extension of AppModelOption (e.g., "type BuiltInAppModelOption = AppModelOption" or "interface BuiltInAppModelOption extends AppModelOption {}") so the shape is derived from AppModelOption and cannot drift; update the export of BuiltInAppModelOption accordingly and remove the duplicated slug/name declaration.apps/web/src/components/Sidebar.tsx (1)
515-523: Consider extracting the provider exclusion logic.The hardcoded
provider !== "claudeCode"check could become harder to maintain as more providers are added. Consider using a configuration object or adding a property toUSAGE_PROVIDERSto indicate whether session usage should be shown.♻️ Suggested refactor
-const USAGE_PROVIDERS: ReadonlyArray<{ provider: ProviderKind; label: string }> = [ +const USAGE_PROVIDERS: ReadonlyArray<{ provider: ProviderKind; label: string; showSessionUsage?: boolean }> = [ { provider: "copilot", label: "Copilot" }, { provider: "codex", label: "Codex" }, { provider: "cursor", label: "Cursor" }, - { provider: "claudeCode", label: "Claude Code" }, + { provider: "claudeCode", label: "Claude Code", showSessionUsage: false }, { provider: "geminiCli", label: "Gemini" }, { provider: "amp", label: "Amp" }, ];Then in the rendering logic:
- if ( - provider !== "claudeCode" && - data?.sessionUsage && + const providerConfig = USAGE_PROVIDERS.find((p) => p.provider === provider); + if ( + providerConfig?.showSessionUsage !== false && + data?.sessionUsage && (data.sessionUsage.totalTokens || data.sessionUsage.totalCostUsd) ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/Sidebar.tsx` around lines 515 - 523, Extract the hardcoded provider exclusion into configuration by adding a boolean like showSessionUsage (or supportsSessionUsage) to the USAGE_PROVIDERS entries, then replace the inline check (provider !== "claudeCode") with a lookup against USAGE_PROVIDERS for the current provider; update the render condition that uses provider, data?.sessionUsage and ProviderSessionUsageBar to check that the provider's config allows session usage and that data.sessionUsage has tokens or cost before pushing ProviderSessionUsageBar.apps/server/src/provider/Layers/CursorAdapter.ts (1)
183-206: Consider handlingstderroutput inrunCursorModelCommand.The
execFilecallback ignoresstderr. If the command fails with diagnostic output on stderr, it may be useful for debugging. Consider logging or including stderr in the error.♻️ Optional: Include stderr in error context
(error, stdout, stderr) => { if (error) { - reject(error); + const enhancedError = new Error(error.message); + (enhancedError as any).stderr = stderr; + reject(enhancedError); return; } resolve(stdout); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/CursorAdapter.ts` around lines 183 - 206, The execFile callback in runCursorModelCommand ignores stderr, losing useful diagnostic output; update the callback signature to (error, stdout, stderr) and, when error is non-null, augment the rejected Error with stderr content (e.g., append stderr to error.message or wrap it in a new Error with both message and stderr) or include stderr as a property on the rejected Error; ensure any successful resolve still returns stdout only, and keep changes localized to the runCursorModelCommand function and its execFile invocation.apps/web/src/routes/__root.tsx (1)
38-50: Avoid duplicating provider literals in the web app.This array is now a second source of truth for
ProviderKind. The next provider added in contracts will silently skip usage invalidation here until someone updates both places. Prefer a shared runtime export from contracts instead of hard-coding the literals again.Also applies to: 218-224
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/__root.tsx` around lines 38 - 50, Replace the hard-coded PROVIDER_KINDS array and the duplicate isProviderKind logic with a single runtime export from the contracts package: import the shared provider-kind list (the runtime export of provider literals) and use that list as the source of truth (typed as ReadonlyArray<ProviderKind>) wherever PROVIDER_KINDS and isProviderKind are used (references: PROVIDER_KINDS, isProviderKind, ProviderKind); remove the duplicated literal array and instead call .includes on the imported list (and update the second occurrence around lines 218-224) so new providers in contracts are automatically reflected in the web app.apps/web/src/proposedPlan.ts (1)
57-67: URL revocation timing may be fragile on some browsers.Using
setTimeout(() => URL.revokeObjectURL(url), 0)revokes the blob URL immediately after the current event loop. While this typically works for small synchronous downloads, it could cause issues on slower systems or with larger files where the browser hasn't yet started the download.A safer approach is to use a longer delay or listen for user interaction completion, though this pattern is commonly used and works in most cases.
♻️ Optional: Use a longer delay for safer revocation
anchor.click(); - window.setTimeout(() => { - URL.revokeObjectURL(url); - }, 0); + // Delay revocation to ensure browser has initiated the download + window.setTimeout(() => { + URL.revokeObjectURL(url); + }, 100);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/proposedPlan.ts` around lines 57 - 67, The current downloadPlanAsTextFile function revokes the blob URL with setTimeout(..., 0), which can be too soon; update the URL.revokeObjectURL call so revocation occurs after the browser has started the download (e.g., replace the 0 delay with a longer delay like 1000ms or revoke inside a click event handler/after an onload-confirmation) to avoid cancelling downloads for larger files; adjust the setTimeout delay or move the revoke into the anchor click handler in downloadPlanAsTextFile so URL.revokeObjectURL(url) runs only after the download has had time to begin.apps/server/src/open.ts (1)
255-288: Consider extracting the repeated macOS app fallback pattern.The pattern
if (platform === "darwin" && !isCommandAvailable(editorDef.command)) { ... isMacAppInstalled ... }appears three times with slight variations. This could be simplified with a helper function.♻️ Optional: Extract macOS fallback helper
+function resolveMacAppFallback( + editorId: EditorId, + command: string, + cwd: string, + extraArgs?: ReadonlyArray<string>, +): EditorLaunch | null { + const macApp = MAC_APP_NAMES[editorId]; + if (macApp && isMacAppInstalled(macApp)) { + return { + command: "open", + args: ["-a", macApp, ...(extraArgs ?? []), cwd], + }; + } + return null; +}This would reduce the repeated conditional checks, though the current implementation is also readable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/open.ts` around lines 255 - 288, Extract the repeated macOS fallback into a helper (e.g. resolveMacOpenFallback or getMacOpenCommand) that accepts editorDef (or editorDef.id and editorDef.command) and the final args to use when the .app is installed; the helper should check platform === "darwin" && !isCommandAvailable(editorDef.command), look up MAC_APP_NAMES[editorDef.id], call isMacAppInstalled(macApp) and return the {command: "open", args: [...] } result (or null/undefined if no fallback), then replace the three repeated blocks in the code paths that handle shouldUseGotoFlag, WORKING_DIRECTORY_EDITORS and the final fallback with calls to this helper and fall back to the original return { command: editorDef.command, args: ... } when the helper returns nothing; keep references to editorDef.id, editorDef.command, MAC_APP_NAMES, isCommandAvailable and isMacAppInstalled so the search is easy.apps/server/src/git/Layers/SessionTextGeneration.test.ts (1)
5-8: Test only verifies export existence, not behavior.This smoke test confirms
SessionTextGenerationLiveis defined but doesn't exercise any actual functionality. Consider adding tests that verify the Layer can be built and that its service methods behave correctly.Would you like me to generate more comprehensive tests for the
SessionTextGenerationlayer, or should I open an issue to track this?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/git/Layers/SessionTextGeneration.test.ts` around lines 5 - 8, The current test only asserts that SessionTextGenerationLive is defined; expand it to construct/build the Layer and exercise its behavior by instantiating the layer (e.g., call the build/compose factory on SessionTextGenerationLive or the exported constructor) and invoking key service methods (such as the layer's generate/session methods or public API) with mocked dependencies to assert expected outputs and side effects; add mocks/stubs for external dependencies and assertions for success and error paths to ensure SessionTextGenerationLive not only exports but functions correctly.apps/web/src/components/GhosttyTerminalSplitView.tsx (2)
456-460: Global mutable counter may cause issues with HMR and SSR.The module-level
nextPaneCountervariable persists across hot module reloads during development, which can lead to non-deterministic pane IDs. Additionally, in SSR scenarios, this counter could be shared across requests.Consider using a component-scoped ref or a more robust ID generation approach.
🔧 Alternative using crypto.randomUUID
-let nextPaneCounter = 0; -function createPaneId(): string { - nextPaneCounter += 1; - return `ghostty-pane-${nextPaneCounter}-${Date.now().toString(36)}`; -} +function createPaneId(): string { + return `ghostty-pane-${crypto.randomUUID()}`; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/GhosttyTerminalSplitView.tsx` around lines 456 - 460, The module-level nextPaneCounter and createPaneId produce non-deterministic IDs across HMR/SSR; move ID generation into component scope by removing the top-level nextPaneCounter and instead generate pane IDs inside the GhosttyTerminalSplitView component using a stable per-instance ref (e.g. useRef counter) or a robust random UUID (crypto.randomUUID() with a safe fallback) inside a new createPaneId helper in the component; update all usages of createPaneId to call the component-scoped version so IDs are not shared across module reloads or requests.
686-690: SplitDivider resize handlers are not implemented.The
SplitDividercomponent is rendered with no-op handlers. The visual divider appears but dragging it does nothing. If horizontal pane resizing is intended, this needs implementation; otherwise, consider hiding the divider or adding a TODO.Would you like me to implement horizontal pane resizing logic, or should I open an issue to track this feature?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/GhosttyTerminalSplitView.tsx` around lines 686 - 690, The SplitDivider is wired with no-op pointer handlers so dragging does nothing; implement pointer-based drag resizing for the horizontal split inside GhosttyTerminalSplitView by wiring onPointerDown to record the initial pointer X and initial left/right pane sizes (e.g., in component state or refs), call event.currentTarget.setPointerCapture(event.pointerId), then onPointerMove compute deltaX from the stored startX to update the left/right pane widths (update state or CSS variables/flex-basis used by the panes), and onPointerUp release the pointer capture and clear drag state; update the handlers named onPointerDown/onPointerMove/onPointerUp on the SplitDivider and ensure you use the same pointerId for capture/release and clamp sizes to min/max values.apps/server/src/git/Layers/GitManager.test.ts (1)
287-295: Use the sharedProviderKindin this helper.This re-hardcodes the provider union in test code. Importing
ProviderKindfrom@t3tools/contractsremoves another drift point the next time providers are added or renamed.♻️ Suggested cleanup
+import type { ProviderKind } from "@t3tools/contracts"; + function runStackedAction( manager: GitManagerShape, input: { cwd: string; action: "commit" | "commit_push" | "commit_push_pr"; commitMessage?: string; featureBranch?: boolean; - provider?: "codex" | "copilot" | "claudeCode" | "cursor" | "opencode" | "geminiCli" | "amp" | "kilo"; + provider?: ProviderKind; model?: string; }, ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/git/Layers/GitManager.test.ts` around lines 287 - 295, The test helper runStackedAction currently hardcodes the provider union type; replace that union with the shared ProviderKind from `@t3tools/contracts` by importing ProviderKind and using it for the provider parameter type in runStackedAction (and any related test helpers/signatures in GitManager.test.ts) so the test stays in sync when providers change.apps/server/src/provider/Layers/CursorUsage.ts (4)
45-55: Consider adding a timeout toexecFileto prevent indefinite hangs.The
execFileTextfunction doesn't specify a timeout, which could cause the process to hang indefinitely if thesecuritycommand stalls.♻️ Proposed fix
function execFileText(command: string, args: ReadonlyArray<string>): Promise<string> { return new Promise((resolve, reject) => { - execFile(command, [...args], { env: process.env }, (error, stdout) => { + execFile(command, [...args], { env: process.env, timeout: 10_000 }, (error, stdout) => { if (error) { reject(error); return; } resolve(stdout); }); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/CursorUsage.ts` around lines 45 - 55, The execFileText helper (execFileText) can hang because execFile is called without a timeout; update execFile invocation to include a reasonable timeout option (e.g., { env: process.env, timeout: <ms> }) and handle the timeout error path when the callback returns an error (ensure promise rejects with that error and consider including context). Locate execFileText and add the timeout option to the execFile options object and ensure existing error handling covers timeout errors from execFile.
79-88: Consider the security implication of treating tokens without expiration as fresh.When
decodeJwtExpirationMsreturnsundefined(e.g., malformed JWT or missingexpclaim),hasFreshAccessTokenreturnstrue. This is a reasonable fallback to avoid blocking users, but could potentially use an expired or invalid token.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/CursorUsage.ts` around lines 79 - 88, The current hasFreshAccessToken function treats tokens with undefined expiration as fresh, which is risky; change hasFreshAccessToken to treat undefined expiration as not fresh (return false) or perform an additional verification step instead of assuming freshness: update the function (hasFreshAccessToken) to return false when decodeJwtExpirationMs(token) === undefined, and optionally add a debug/error log indicating a malformed/missing exp claim so callers can trigger a token refresh path; ensure callers of hasFreshAccessToken (e.g., whatever uses CursorUsage) continue to handle refresh when false.
163-179: Consider adding request timeout and error logging for API key exchange.The
exchangeCursorApiKeyfunction doesn't have a timeout and silently returnsundefinedon failure. While this may be acceptable for a best-effort operation, logging failed attempts would aid debugging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/CursorUsage.ts` around lines 163 - 179, The exchangeCursorApiKey function lacks a request timeout and does not log failures; wrap the fetch in a try/catch, use an AbortController with a short timeout (e.g., 5s) to cancel slow requests, and on any non-ok response or caught error log a clear message including CURSOR_API_BASE_URL and the error/response status before returning undefined; keep the return behavior (use normalizeNonEmptyString(parsed.accessToken)) but ensure errors/timeouts are logged (use the module's logger if available, otherwise console.error).
146-161: Auth data merging may cause unexpected behavior with stale tokens.When
hasFreshAccessToken(fileAuth.accessToken)is false, the code fetches from Keychain but then spreadsfileAuthfirst followed by Keychain values. This means Keychain values override file values, which is the intended behavior. However, if Keychain returns undefined foraccessToken, the stalefileAuth.accessTokenwill be used via the first spread.This seems intentional as a fallback strategy, but the comment could clarify this behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/CursorUsage.ts` around lines 146 - 161, readCursorAuthData currently falls back to a stale fileAuth.accessToken when Keychain returns undefined; change the merge so that fileAuth.accessToken is only used if it is fresh (use hasFreshAccessToken(fileAuth.accessToken)) and otherwise prefer Keychain values or undefined—update the return object in readCursorAuthData to explicitly set accessToken to accessToken ?? (hasFreshAccessToken(fileAuth.accessToken) ? fileAuth.accessToken : undefined) and apiKey to apiKey ?? fileAuth.apiKey, or add a clarifying comment explaining this exact fallback behavior if the fallback is intentional.apps/web/src/components/PlanSidebar.tsx (1)
100-130: Consider using.finally()instead of double.then()for cleanup.The current pattern with two
.then()callbacks (one for success, one for rejection of thecatch) works but is less idiomatic than using.finally().♻️ Proposed fix
void api.projects .writeFile({ cwd: workspaceRoot, relativePath: filename, contents: normalizePlanMarkdownForExport(planMarkdown), }) .then((result) => { toastManager.add({ type: "success", title: "Plan saved", description: result.relativePath, }); }) .catch((error) => { toastManager.add({ type: "error", title: "Could not save plan", description: error instanceof Error ? error.message : "An error occurred.", }); }) - .then( - () => setIsSavingToWorkspace(false), - () => setIsSavingToWorkspace(false), - ); + .finally(() => setIsSavingToWorkspace(false));🤖 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 100 - 130, In handleSaveToWorkspace, replace the current promise chain on api.projects.writeFile (used with .then(...).catch(...).then(...)) with a chain that uses .then(...) for success, .catch(...) for errors, and a .finally(...) to call setIsSavingToWorkspace(false) for cleanup; keep the success toast logic inside the first .then, error toast logic inside .catch, and ensure you still early-return when !api || !workspaceRoot || !planMarkdown while preserving buildProposedPlanMarkdownFilename, normalizePlanMarkdownForExport, and readNativeApi usage.apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts (1)
523-536: Consider the memory implications of usingMapfor unbounded segment state.
assistantSegmentStateByKeyandassistantSegmentKeysByTurnKeyare plainMapobjects without TTL or capacity limits, unlike theCacheinstances used elsewhere. While cleanup functions exist, a failure to clean up (e.g., due to an error beforeturn.completed) could cause memory growth.The existing cleanup on
session.exitedviaclearTurnStateForSessionmitigates this, but consider whether the lifetime matches expectations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts` around lines 523 - 536, The two plain Maps assistantSegmentStateByKey and assistantSegmentKeysByTurnKey can grow unbounded and risk memory leaks; replace them with bounded caches (use Cache.make) or wrap them with TTL/capacity logic similar to bufferedProposedPlanById so entries auto-evict, or at minimum add defensive eviction on errors and ensure clearTurnStateForSession is always invoked (e.g., use finally hooks around turn lifecycle); update references to assistantSegmentStateByKey and assistantSegmentKeysByTurnKey accordingly so lookups/updates use the new Cache API and preserve existing cleanup semantics.apps/server/src/git/Layers/SessionTextGeneration.ts (1)
150-165: Consider adding event filtering before forking to reduce unnecessary queue operations.The current implementation forks a scoped fiber that filters events by
threadIdinside the loop. While functionally correct, events for other threads are still queued before being discarded. This is acceptable given the short-lived nature of these sessions, but could be optimized if high event throughput becomes an issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/git/Layers/SessionTextGeneration.ts` around lines 150 - 165, The Stream.runForEach is currently checking event.threadId inside the loop which still invokes Queue.offer for every event; instead create a filtered stream before forking so only matching events are processed: use Stream.filter on providerService.streamEvents with (e) => e.threadId === threadId and then runForEach to Queue.offer(eventQueue, event). Keep the same Effect.asVoid/Effect.forkScoped usage and reference Stream.runForEach, providerService.streamEvents, event.threadId, threadId, Queue.offer, eventQueue and Effect.forkScoped when making the change.apps/server/src/provider/Layers/CopilotAdapter.ts (1)
1625-1626: Consider documenting the internal API dependency.The function accesses an undocumented internal RPC API via complex type assertion. This is brittle and could break with Copilot SDK updates.
📝 Suggested improvement
Consider adding a comment documenting this dependency and the fallback behavior:
try { await client.start(); + // NOTE: getQuota is an internal RPC endpoint; may break on SDK updates. + // Gracefully returns partial result on failure. const quota = await (client as unknown as { rpc: { account: { getQuota: () => Promise<{ quotaSnapshots?: unknown }> } } }).rpc.account.getQuota().catch(() => undefined);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/CopilotAdapter.ts` around lines 1625 - 1626, The code in CopilotAdapter.ts uses a brittle internal RPC access via the expression (client as unknown as { rpc: { account: { getQuota: () => Promise<{ quotaSnapshots?: unknown }> } } }).rpc.account.getQuota() and silently falls back to returning { provider: "copilot" } when that call fails; add an inline comment immediately above this expression that documents the internal API dependency (that we rely on client.rpc.account.getQuota returning quotaSnapshots), why the complex type assertion is necessary, and clearly state the intended fallback behavior when the RPC is absent or throws so future maintainers know this is an internal, unstable API and why we return the default provider object.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4abd7b9a-3c0c-4815-ae63-208255ae49ac
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (96)
.github/VOUCHED.td.github/pull_request_template.md.github/workflows/pr-vouch.yml.github/workflows/release.ymlCONTRIBUTING.mdREADME.mdapps/desktop/src/main.tsapps/server/scripts/cli.tsapps/server/src/checkpointing/Layers/CheckpointDiffQuery.test.tsapps/server/src/checkpointing/Layers/CheckpointDiffQuery.tsapps/server/src/checkpointing/Utils.tsapps/server/src/git/Layers/GitManager.test.tsapps/server/src/git/Layers/GitManager.tsapps/server/src/git/Layers/SessionTextGeneration.test.tsapps/server/src/git/Layers/SessionTextGeneration.tsapps/server/src/git/Services/SessionTextGeneration.tsapps/server/src/git/Services/TextGeneration.tsapps/server/src/keybindings.tsapps/server/src/main.test.tsapps/server/src/open.test.tsapps/server/src/open.tsapps/server/src/opencodeServerManager.tsapps/server/src/orchestration/Layers/ProviderCommandReactor.tsapps/server/src/orchestration/Layers/ProviderRuntimeIngestion.test.tsapps/server/src/orchestration/Layers/ProviderRuntimeIngestion.tsapps/server/src/orchestration/decider.tsapps/server/src/provider/Layers/ClaudeCodeAdapter.test.tsapps/server/src/provider/Layers/ClaudeCodeAdapter.tsapps/server/src/provider/Layers/CodexAdapter.tsapps/server/src/provider/Layers/CopilotAdapter.tsapps/server/src/provider/Layers/CursorAdapter.test.tsapps/server/src/provider/Layers/CursorAdapter.tsapps/server/src/provider/Layers/CursorUsage.test.tsapps/server/src/provider/Layers/CursorUsage.tsapps/server/src/provider/Layers/ProviderHealth.tsapps/server/src/provider/Layers/ProviderService.tsapps/server/src/provider/Layers/copilotCliPath.tsapps/server/src/provider/Layers/copilotTurnTracking.test.tsapps/server/src/provider/Layers/copilotTurnTracking.tsapps/server/src/provider/Services/CursorAdapter.test.tsapps/server/src/provider/Services/CursorAdapter.tsapps/server/src/provider/claude-agent-sdk.d.tsapps/server/src/serverLayers.tsapps/server/src/wsServer.test.tsapps/server/src/wsServer.tsapps/web/index.htmlapps/web/package.jsonapps/web/src/appSettings.tsapps/web/src/components/ChatView.tsxapps/web/src/components/CommandPalette.tsxapps/web/src/components/DiffPanel.tsxapps/web/src/components/GhosttyTerminalSplitView.tsxapps/web/src/components/GitActionsControl.logic.test.tsapps/web/src/components/GitActionsControl.logic.tsapps/web/src/components/GitActionsControl.tsxapps/web/src/components/Icons.tsxapps/web/src/components/PlanSidebar.tsxapps/web/src/components/ProjectScriptsControl.tsxapps/web/src/components/Sidebar.tsxapps/web/src/components/ThreadTerminalDrawer.tsxapps/web/src/components/ui/input.tsxapps/web/src/composerDraftStore.tsapps/web/src/hooks/useProjectThreadNavigation.tsapps/web/src/index.cssapps/web/src/keybindings.test.tsapps/web/src/keybindings.tsapps/web/src/lib/gitReactQuery.tsapps/web/src/proposedPlan.tsapps/web/src/routes/__root.tsxapps/web/src/routes/_chat.settings.tsxapps/web/src/session-logic.test.tsapps/web/src/session-logic.tsapps/web/src/store.test.tsapps/web/src/store.tsapps/web/src/wsTransport.tsapps/web/vite.config.tspackage.jsonpackages/contracts/src/editor.tspackages/contracts/src/git.tspackages/contracts/src/keybindings.test.tspackages/contracts/src/keybindings.tspackages/contracts/src/model.tspackages/contracts/src/orchestration.tspackages/contracts/src/provider.test.tspackages/contracts/src/provider.tspackages/contracts/src/server.tspackages/shared/src/model.test.tspackages/shared/src/model.tsscripts/build-desktop-artifact.tsscripts/dev-runner.tsscripts/install.shscripts/merge-mac-update-manifests.test.tsscripts/merge-mac-update-manifests.tsscripts/release-smoke.tsscripts/update-release-package-versions.tsupdate
| readonly model?: string; | ||
| readonly modelOptions?: ProviderModelOptions; | ||
| readonly serviceTier?: ProviderServiceTier | null; | ||
| readonly providerOptions?: ProviderStartOptions; |
There was a problem hiding this comment.
Provider option updates never reach active sessions.
These options are only consumed by startSession(). If a thread already has a live session and only providerOptions change, ensureSessionForThread() keeps the existing session and sendTurn() has no way to apply the new values. The update silently does nothing until some unrelated restart happens.
Also applies to: 220-222, 236-240, 255-255
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/orchestration/Layers/ProviderCommandReactor.ts` at line 210,
The ProviderStartOptions stored on the reactor are only applied in
startSession(), so when providerOptions changes for an already-active thread
ensureSessionForThread() keeps the old session and sendTurn() never sees the
update; change the flow so updates to providerOptions propagate to live sessions
by detecting option changes in ensureSessionForThread() (or a new
applyProviderOptionsToSession(session, providerOptions) helper) and
reconfiguring or restarting the existing session as needed, then call that
helper from places that currently only call startSession() (including sendTurn()
entry paths) so active sessions get the new ProviderStartOptions immediately;
reference ProviderStartOptions, startSession(), ensureSessionForThread(),
sendTurn() and the providerOptions property when implementing.
- ProviderCommandReactor: don't mark session stopped after failed stop; document provider options only apply at session start - ClaudeCodeAdapter: move executable path resolution inside Effect.try for proper error mapping in packaged environments - ProviderHealth: replace unsafe getQuotaInfo cast with documented client.rpc.account.getQuota() SDK API - ProviderService: preserve providerOptions across all directory.upsert calls (sendTurn, recovery, stopAll) to prevent silent data loss - ChatView: add removeKeybinding API to clean up stale shortcuts on script deletion; document snapshot resync trade-offs - CommandPalette: catch async onSelect failures to prevent unhandled promise rejections - ProjectScriptsControl: await onDeleteScript and surface errors via validation state instead of fire-and-forget - Settings: add aria-label to provider accent color pickers - wsTransport: close sockets that finish connecting after disposal - orchestration.ts: redact credentials (username/password) from persisted event payloads; use PositiveInt for port validation - install.sh: read prompts from /dev/tty for curl|bash compatibility; fix word-splitting vulnerability in find_bin path probing
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/components/ChatView.tsx (1)
2611-2641:⚠️ Potential issue | 🟡 MinorThe drag-over indicator can get stuck after hovering nested children.
Line 2616 increments
dragDepthReffor bubbleddragenters from descendants, but Lines 2634-2641 only decrement when the pointer leaves the composer entirely. After moving across children, the counter can remain above zero and keepisDragOverComposerstucktrue. Either rely only on the containment check here or make the enter/leave bookkeeping symmetrical.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/ChatView.tsx` around lines 2611 - 2641, The drag enter/leave bookkeeping is asymmetric: onComposerDragEnter increments dragDepthRef for bubbled enters, but onComposerDragLeave only decrements when leaving the composer entirely, which can leave dragDepthRef > 0 and keep isDragOverComposer true; fix by making leave decrement symmetrically — in onComposerDragLeave decrement dragDepthRef.current (clamped to >=0) for every leave event, then check event.relatedTarget containment (using event.currentTarget.contains(event.relatedTarget)) and only call setIsDragOverComposer(false) when dragDepthRef.current === 0 and the relatedTarget is outside; keep the preventDefault/Files type guard and ensure no negative counts.
♻️ Duplicate comments (2)
apps/server/src/orchestration/Layers/ProviderCommandReactor.ts (1)
240-244:⚠️ Potential issue | 🟠 MajorApply
providerOptionsdiffs to active sessions.Lines 240-244 update the cache, but Lines 295-297 reuse the current session unless the provider/runtime/model changed. A
providerOptions-only change is still a silent no-op until some unrelated restart. Treat option changes as a restart/reconfigure trigger too.Also applies to: 295-297
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/orchestration/Layers/ProviderCommandReactor.ts` around lines 240 - 244, The code updates threadProviderOptions but does not treat providerOptions-only changes as a reconfigure trigger; compare the new effectiveProviderOptions (from options?.providerOptions ?? threadProviderOptions.get(threadId)) against the previously stored providerOptions for threadId and, if different, force a session restart/reconfiguration rather than reusing the current session. Modify the session-reuse logic (the check around lines that reuse the current session unless provider/runtime/model changed) to include providerOptions in its equality check, or clear/reset the cached session entry for threadId when threadProviderOptions.set(threadId, options.providerOptions) is executed so that the code path that creates a new session runs. Ensure you reference the same threadId and threadProviderOptions keys when performing the comparison/reset.apps/web/src/components/ChatView.tsx (1)
3328-3359:⚠️ Potential issue | 🔴 CriticalDon't roll back a live implementation thread because a client-side resync/navigation step failed.
The chain on Lines 3291-3339 puts
getSnapshot(),syncServerReadModel(), andnavigate()under the samecatchthat deletesnextThreadIdon Lines 3345-3351. Ifthread.createandthread.turn.startsucceed but the snapshot fetch, local sync, or navigation fails, this will still delete a valid server thread and discard the newly-started implementation run. Keep rollback scoped to failures before the server-side create/start succeeds, and let theorchestration.domainEventwebsocket unblock client visibility/navigation instead of snapshot resyncs. As per coding guidelinesWeb app must consume orchestration domain events via WebSocket push on channel orchestration.domainEvent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/ChatView.tsx` around lines 3328 - 3359, The catch currently deletes the server thread (dispatchCommand type "thread.delete") for any failure including client-side snapshot sync/navigation; change this so rollback only happens if the server-side create/start did not succeed: introduce a local flag (e.g., threadCreated or threadStarted) that's set when the create/start RPCs return successfully and only invoke api.orchestration.dispatchCommand({type: "thread.delete", commandId: newCommandId(), threadId: nextThreadId}) inside the catch when that flag is false; keep the snapshot re-sync (api.orchestration.getSnapshot -> syncServerReadModel) and planSidebarOpenOnNextThreadRef/navigate logic as separate flows and rely on orchestration.domainEvent websocket updates for eventual visibility instead of deleting a live thread on client-side navigation/sync failures.
🧹 Nitpick comments (2)
scripts/install.sh (1)
274-275:NODE_BINis assigned but never used.The variable is set here but the script never references it afterward. Unlike
GIT_BIN(used instep_clone), Node.js is only checked for presence. Consider removing the assignment or documenting why it's retained for future use.♻️ Option 1: Remove the unused assignment
log_ok "Node.js found: ${BOLD}${node_bin}${RESET} (${node_ver})" - NODE_BIN="$node_bin" return 0Also remove from line 405:
- GIT_BIN="" NODE_BIN="" BUN_BIN="" + GIT_BIN="" BUN_BIN=""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.sh` around lines 274 - 275, The NODE_BIN variable is assigned from node_bin but never used; remove the unused assignment(s) to NODE_BIN (the initial NODE_BIN="$node_bin" and the later duplicate assignment) and any related dead code, or if you intend to keep it for future use, add a brief comment explaining why it’s retained; reference the variables NODE_BIN and node_bin and note that GIT_BIN is different (used in step_clone) for context.apps/server/src/provider/Layers/ProviderHealth.ts (1)
491-491: Fix semantically incorrect return value in error handler.The
catch(() => [])returns an empty array, which is semantically incorrect for astop()operation. While this doesn't affect runtime behavior since the result is discarded, it's misleading.🔧 Proposed fix
} finally { - await client.stop().catch(() => []); + await client.stop().catch(() => undefined); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/ProviderHealth.ts` at line 491, The error handler currently returns an empty array from the promise chain for client.stop(), which is semantically wrong; update the call that does await client.stop().catch(() => []) to instead swallow the error with a void/undefined return (e.g., .catch(() => undefined) or .catch(() => {}) ), or log the error and return undefined, so the catch reflects a no-value stop failure for the client.stop() invocation in ProviderHealth.
🤖 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/orchestration/decider.ts`:
- Around line 21-35: The redaction in redactProviderStartOptions currently
strips opencode/kilo credentials too early, preventing ProviderCommandReactor
(which reads thread.turn-start-requested and calls providerService.startSession)
from seeing providerOptions; change the flow so redactProviderStartOptions is
only used when creating the persisted event shape (materializing events) and do
not modify the in-memory event passed to ProviderCommandReactor or any code that
calls providerService.startSession; keep the raw providerOptions (with
opencode/kilo) on the live event and move calls to redactProviderStartOptions to
the persistence/serialization path (also update the other instance referenced at
the 317-325 location to follow the same pattern).
In `@apps/server/src/orchestration/Layers/ProviderCommandReactor.ts`:
- Around line 699-701: In the "thread.deleted" case inside
ProviderCommandReactor, stop any active provider session for that thread before
deleting its options to avoid orphaning resources: call the existing
session-teardown helper (e.g., stopProviderSession(threadId) or
stopActiveSession(threadId) — whichever function is defined in this module)
using event.payload.threadId, wait for it to complete (await if async), then
call threadProviderOptions.delete(event.payload.threadId) and return; this
ensures any running provider is cleanly stopped prior to removing its config.
In `@apps/server/src/provider/Layers/ClaudeCodeAdapter.ts`:
- Around line 266-279: The desktop diagnostic helpers diagnosticEnvKeys and
logDesktopClaudeDiagnostic currently expose too much sensitive data; restrict
diagnostic_env collection to a safe allowlist of specific non-secret keys (e.g.,
only include approved keys like APP_VERSION, PLATFORM, CLAUDE_CLIENT_VERSION)
instead of returning all DESKTOP_DIAGNOSTIC_ENV_PREFIXES matches, and change
logDesktopClaudeDiagnostic to never log raw stderr by default—either redact
sensitive text or require an explicit opt-in flag (e.g., a redactStderr=false
option) before emitting full stderr; update the functions (diagnosticEnvKeys,
logDesktopClaudeDiagnostic) and any callers to use the allowlist and
opt-in/redaction behavior and ensure only minimal non-sensitive fields and
summarized/error codes are written to console.warn.
In `@apps/web/src/components/ChatView.tsx`:
- Around line 6691-6702: The copyPath callback sets copiedPath before
navigator.clipboard.writeText resolves, causing false-positive UI on failures;
update the useCallback (copyPath) to await
navigator.clipboard.writeText(openInCwd) and only call setCopiedPath(true) after
the promise resolves, move the timeout setup/clearTimeout logic inside the
success path, and add a catch to handle/write failures (e.g., log or surface an
error) so failed writes do not show the success state.
In `@scripts/install.sh`:
- Around line 131-136: The case block handling OS_RAW contains overlapping
patterns where "MINGW*" precedes "MINGW64*", making "MINGW64*" unreachable;
update the case patterns in the OS detection switch (the case for OS_RAW) so the
more specific "MINGW64*" is either removed or moved before the more general
"MINGW*" (or simply keep a single "MINGW*" pattern) to avoid unreachable
alternatives while preserving the mapping to OS="windows".
---
Outside diff comments:
In `@apps/web/src/components/ChatView.tsx`:
- Around line 2611-2641: The drag enter/leave bookkeeping is asymmetric:
onComposerDragEnter increments dragDepthRef for bubbled enters, but
onComposerDragLeave only decrements when leaving the composer entirely, which
can leave dragDepthRef > 0 and keep isDragOverComposer true; fix by making leave
decrement symmetrically — in onComposerDragLeave decrement dragDepthRef.current
(clamped to >=0) for every leave event, then check event.relatedTarget
containment (using event.currentTarget.contains(event.relatedTarget)) and only
call setIsDragOverComposer(false) when dragDepthRef.current === 0 and the
relatedTarget is outside; keep the preventDefault/Files type guard and ensure no
negative counts.
---
Duplicate comments:
In `@apps/server/src/orchestration/Layers/ProviderCommandReactor.ts`:
- Around line 240-244: The code updates threadProviderOptions but does not treat
providerOptions-only changes as a reconfigure trigger; compare the new
effectiveProviderOptions (from options?.providerOptions ??
threadProviderOptions.get(threadId)) against the previously stored
providerOptions for threadId and, if different, force a session
restart/reconfiguration rather than reusing the current session. Modify the
session-reuse logic (the check around lines that reuse the current session
unless provider/runtime/model changed) to include providerOptions in its
equality check, or clear/reset the cached session entry for threadId when
threadProviderOptions.set(threadId, options.providerOptions) is executed so that
the code path that creates a new session runs. Ensure you reference the same
threadId and threadProviderOptions keys when performing the comparison/reset.
In `@apps/web/src/components/ChatView.tsx`:
- Around line 3328-3359: The catch currently deletes the server thread
(dispatchCommand type "thread.delete") for any failure including client-side
snapshot sync/navigation; change this so rollback only happens if the
server-side create/start did not succeed: introduce a local flag (e.g.,
threadCreated or threadStarted) that's set when the create/start RPCs return
successfully and only invoke api.orchestration.dispatchCommand({type:
"thread.delete", commandId: newCommandId(), threadId: nextThreadId}) inside the
catch when that flag is false; keep the snapshot re-sync
(api.orchestration.getSnapshot -> syncServerReadModel) and
planSidebarOpenOnNextThreadRef/navigate logic as separate flows and rely on
orchestration.domainEvent websocket updates for eventual visibility instead of
deleting a live thread on client-side navigation/sync failures.
---
Nitpick comments:
In `@apps/server/src/provider/Layers/ProviderHealth.ts`:
- Line 491: The error handler currently returns an empty array from the promise
chain for client.stop(), which is semantically wrong; update the call that does
await client.stop().catch(() => []) to instead swallow the error with a
void/undefined return (e.g., .catch(() => undefined) or .catch(() => {}) ), or
log the error and return undefined, so the catch reflects a no-value stop
failure for the client.stop() invocation in ProviderHealth.
In `@scripts/install.sh`:
- Around line 274-275: The NODE_BIN variable is assigned from node_bin but never
used; remove the unused assignment(s) to NODE_BIN (the initial
NODE_BIN="$node_bin" and the later duplicate assignment) and any related dead
code, or if you intend to keep it for future use, add a brief comment explaining
why it’s retained; reference the variables NODE_BIN and node_bin and note that
GIT_BIN is different (used in step_clone) for context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ff86c89-7d23-4b55-90fa-06cef11329ec
📒 Files selected for processing (18)
apps/server/src/keybindings.tsapps/server/src/orchestration/Layers/ProviderCommandReactor.tsapps/server/src/orchestration/decider.tsapps/server/src/provider/Layers/ClaudeCodeAdapter.tsapps/server/src/provider/Layers/ProviderHealth.tsapps/server/src/provider/Layers/ProviderService.tsapps/server/src/wsServer.tsapps/web/src/components/ChatView.tsxapps/web/src/components/CommandPalette.tsxapps/web/src/components/ProjectScriptsControl.tsxapps/web/src/routes/_chat.settings.tsxapps/web/src/wsNativeApi.tsapps/web/src/wsTransport.tspackages/contracts/src/ipc.tspackages/contracts/src/orchestration.tspackages/contracts/src/server.tspackages/contracts/src/ws.tsscripts/install.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/web/src/routes/_chat.settings.tsx
- apps/web/src/components/ProjectScriptsControl.tsx
- apps/web/src/components/CommandPalette.tsx
Comment on lines
+266
to
+279
| function diagnosticEnvKeys(env: Readonly<Record<string, string | undefined>>): ReadonlyArray<string> { | ||
| return Object.keys(env) | ||
| .filter((key) => DESKTOP_DIAGNOSTIC_ENV_PREFIXES.some((prefix) => key.startsWith(prefix))) | ||
| .sort(); | ||
| } | ||
|
|
||
| function logDesktopClaudeDiagnostic(message: string, data?: Record<string, unknown>): void { | ||
| if (!isDesktopRuntime()) return; | ||
| if (data) { | ||
| console.warn("[claudeCode][desktop]", message, data); | ||
| return; | ||
| } | ||
| console.warn("[claudeCode][desktop]", message); | ||
| } |
There was a problem hiding this comment.
Tighten the new desktop diagnostics before they start carrying user data.
This now logs cwd, every matching CLAUDE*/CODEX*/T3CODE* env key name, and raw Claude stderr. That combination can leak workspace paths, credential-related env metadata, or prompt/file content into persistent desktop logs. Please restrict diagnostics to a small safe allowlist and make raw stderr opt-in or redacted.
🔐 Possible tightening
-const DESKTOP_DIAGNOSTIC_ENV_PREFIXES = ["CLAUDE", "ELECTRON", "CODEX", "T3CODE"] as const;
+const DESKTOP_DIAGNOSTIC_ENV_KEYS = [
+ "ELECTRON_RUN_AS_NODE",
+ "ELECTRON_RENDERER_PORT",
+ "CLAUDECODE",
+ "T3CODE_MODE",
+] as const;
function diagnosticEnvKeys(env: Readonly<Record<string, string | undefined>>): ReadonlyArray<string> {
- return Object.keys(env)
- .filter((key) => DESKTOP_DIAGNOSTIC_ENV_PREFIXES.some((prefix) => key.startsWith(prefix)))
- .sort();
+ return DESKTOP_DIAGNOSTIC_ENV_KEYS.filter((key) => env[key] !== undefined);
}
@@
stderr: (message: string) => {
const trimmed = message.trimEnd();
- if (trimmed.length > 0) {
- console.warn("[claudeCode][stderr]", trimmed);
- }
+ if (trimmed.length === 0) return;
+ if (process.env.T3CODE_DEBUG_CLAUDE_STDERR === "1") {
+ console.warn("[claudeCode][stderr]", trimmed);
+ return;
+ }
+ console.warn("[claudeCode][stderr]", "Claude CLI wrote to stderr.");
},Also applies to: 1741-1760
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/provider/Layers/ClaudeCodeAdapter.ts` around lines 266 - 279,
The desktop diagnostic helpers diagnosticEnvKeys and logDesktopClaudeDiagnostic
currently expose too much sensitive data; restrict diagnostic_env collection to
a safe allowlist of specific non-secret keys (e.g., only include approved keys
like APP_VERSION, PLATFORM, CLAUDE_CLIENT_VERSION) instead of returning all
DESKTOP_DIAGNOSTIC_ENV_PREFIXES matches, and change logDesktopClaudeDiagnostic
to never log raw stderr by default—either redact sensitive text or require an
explicit opt-in flag (e.g., a redactStderr=false option) before emitting full
stderr; update the functions (diagnosticEnvKeys, logDesktopClaudeDiagnostic) and
any callers to use the allowlist and opt-in/redaction behavior and ensure only
minimal non-sensitive fields and summarized/error codes are written to
console.warn.
CodeRabbit round 3: - decider.ts: move credential redaction to persistence/WS boundaries so ProviderCommandReactor receives full providerOptions at runtime - ProviderCommandReactor: stop active session on thread.deleted before cleanup; don't mark session stopped after failed stop - ChatView: await clipboard.writeText before showing success indicator; fix asymmetric drag enter/leave bookkeeping; scope thread rollback to pre-start failures only - ProviderHealth: fix catch(() => []) to catch(() => undefined) - install.sh: remove unreachable MINGW64* pattern; remove unused NODE_BIN Internal specialist audit: - Security: redact credentials in ProviderService runtimePayload before SQLite persistence (all 3 upsert boundaries) - Remove 6 unused imports in ChatView.tsx (lint warnings eliminated) - Extract toMessage() to shared module (deduplicated across 4 adapters) - CursorAdapter: early return in extractCursorChunkText to prevent duplicate text when both text and content fields are populated - SessionTextGeneration: handle session.exited event to fail fast instead of blocking 180s on provider process crash - CopilotAdapter: eliminate double trimToUndefined call
This was referenced Mar 9, 2026
This was referenced Mar 18, 2026
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
1 participant
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
Bug Fixes
Critical
opencode/kiloprovider fields (6 fields each) that were silently stripped during decoding, and restored value re-export fromprovider.tslastErrorlifecycle (clear only on idle, not abort), teardown event emission, pending resolver.clear(), reconfigure error guard, attachment validation via Effect error channelMedium
catchCausehandlers;threadProviderOptionscleanup on both session stop and thread deletionsatisfiesoverascast for copilot usage result in wsServeronInterruptmemoization, duplicate keys in PlanSidebar, timeout cleanups in PlanSidebar and OpenInPickerLow
assistantDeliveryMode(was duplicated 3x)window.history.back()onDragLeavehandler for project drag indicatoros.homedir()instead ofprocess.env.HOMEfor macOS app detectionstopSessionerrors now logged before swallowing.appdetection/launch mismatch on macOS (fallback toopen -afor all editors)Test plan
bun typecheckpasses (7/7 packages, 0 errors)bun lintpasses (0 errors, 19 pre-existing warnings).appdetection pathSummary by CodeRabbit
New Features
Bug Fixes
Documentation