chore: sync 9 upstream commits from pingdotgg/t3code#3
Merged
ranvier2d2 merged 10 commits intomainfrom Mar 24, 2026
Merged
Conversation
📝 WalkthroughWalkthroughThis pull request refactors the git execution layer from a Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Web Client
participant WS as WebSocket Server
participant GM as GitManager
participant GC as GitCore
participant Child as Child Process<br/>(git)
Client->>WS: gitRunStackedAction({actionId, cwd, action})
WS->>GM: runStackedAction(input, {actionId, progressReporter})
GM->>GM: emit action_started progress
Note over GM: For each phase (branch, commit, push, pr)
GM->>GM: emit phase_started progress
alt Commit Phase
GM->>GC: commit(cwd, subject, body, {timeoutMs, progress})
GC->>Child: spawn git commit with GIT_TRACE2_EVENT
Child->>GC: hook lifecycle events (start, exit)
GC->>GC: parse hook stdout/stderr/exit
GC-->>GM: progress callbacks (onStdoutLine, onHookFinished)
GM-->>WS: emit hook_output, hook_finished events
end
alt Success
GM->>GM: emit action_finished progress
GM-->>WS: push gitActionProgress event
WS-->>Client: receive action completion
else Error
GM->>GM: emit action_failed progress
GM-->>WS: push gitActionProgress with error
WS-->>Client: receive action failure
end
sequenceDiagram
participant UI as ChatView UI
participant Comp as GitActionsControl
participant API as Native API
participant WS as WebSocket
participant Toast as Toast Notifier
UI->>Comp: click "Run Git Action"
Comp->>Comp: generate actionId
Comp->>API: api.git.onActionProgress(callback)
Comp->>Comp: register listener with actionId filter
Comp->>API: runStackedAction({actionId, cwd, action})
API->>WS: request with {actionId, progressReporter}
Note over WS: Action executes on server
WS-->>API: push gitActionProgress event
API->>Comp: invoke onActionProgress callback
alt action_started
Comp->>Toast: create toast with elapsed time
else phase_started
Comp->>Toast: update title to phase name
else hook_started/output/finished
Comp->>Toast: update description with hook output
else action_finished/failed
Comp->>Toast: show final status and close
end
Comp->>Comp: clear activeGitActionProgressRef
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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
size:XXL
vouch:trusted
Cherry-picked from pingdotgg/t3code: - 843d6d8 fix: add license field to npm package (pingdotgg#1272) - e11fb6e fix(web): avoid false draft attachment persistence warnings (pingdotgg#1153) - 2b08d86 Add resizable chat sidebar (pingdotgg#1347) - e823f8f ci(github): exclude test files from mixed PR size calculation (pingdotgg#1105) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Cherry-picked d6408a2 from pingdotgg/t3code. Re-applied Partial<Record> fix for PROVIDER_CUSTOM_MODEL_CONFIG (upstream overwrote it back to Record). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Cherry-picked 1371ce2 from pingdotgg/t3code. Resolved .gitignore conflict (kept both our artifacts + upstream .tanstack). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Cherry-picked be1abc8 from pingdotgg/t3code. Adds terminal toggle button to chat header. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/contracts/src/git.ts (1)
74-86:⚠️ Potential issue | 🔴 CriticalUpdate
wsTransport.test.tsto includeactionIdin the request payloads.The
actionIdfield is now required onGitRunStackedActionInput, but two test cases inapps/web/src/wsTransport.test.tsstill pass incomplete payloads with only{ cwd: "/repo" }. These tests will fail schema validation:
- Line 217–219: "does not create a timeout for requests with timeoutMs null"
- Line 245–248: "rejects pending requests when the websocket closes"
Add an
actionIdproperty to both request objects. All other callers (WS routes, client code, server tests) have been correctly updated.🤖 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 74 - 86, Two tests in wsTransport.test.ts are sending payloads that fail the new GitRunStackedActionInput schema because they omit the required actionId; update the two request objects used in the tests "does not create a timeout for requests with timeoutMs null" and "rejects pending requests when the websocket closes" to include an actionId property (any non-empty trimmed string, e.g. "test-action-1") so the payloads conform to GitRunStackedActionInput; locate the request objects that currently are { cwd: "/repo" } and add actionId alongside cwd.apps/web/src/components/GitActionsControl.tsx (1)
407-420:⚠️ Potential issue | 🟠 MajorDon't use
useEffectEventfor a click-path helper.
runGitActionWithToastis called from button/menu/dialog handlers (continuePendingDefaultBranchAction,runQuickAction,openDialogForMenuItem,runDialogAction, and others), but React documentsuseEffectEventas effect-only and explicitly prohibits calling the returned function from general event handlers. This should useuseCallbackwith a proper dependency array instead.♻️ Suggested direction
-import { useCallback, useEffect, useEffectEvent, useMemo, useRef, useState } from "react"; +import { useCallback, useEffect, useMemo, useRef, useState } from "react"; ... - const runGitActionWithToast = useEffectEvent( + const runGitActionWithToast = useCallback( async ({ action, commitMessage, forcePushOnlyProgress = false, onConfirmed, skipDefaultBranchPrompt = false, statusOverride, featureBranch = false, isDefaultBranchOverride, progressToastId, filePaths, }: RunGitActionWithToastInput) => { ... }, - ); + [gitStatusForActions, isDefaultBranch, runImmediateGitActionMutation, threadToastData], + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/GitActionsControl.tsx` around lines 407 - 420, runGitActionWithToast is currently created with useEffectEvent but is invoked from click/menu/dialog handlers (e.g. continuePendingDefaultBranchAction, runQuickAction, openDialogForMenuItem, runDialogAction), and must be a stable event handler; replace useEffectEvent with useCallback and supply a correct dependency array that includes all external values referenced inside (at minimum gitStatusForActions and any props, state, or refs used within the function) so the handler is safe to call from UI event handlers and updates when its dependencies change.
🧹 Nitpick comments (6)
apps/web/src/lib/terminalFocus.test.ts (1)
13-15: Mockclosestimplementation may not fully reflect real DOM behavior.The mock returns
thiswhenisConnectedis true, but realclosest()traverses ancestors. This works for the current tests since they only need to verify theisConnectedcheck, but consider adding a comment clarifying this simplification.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/terminalFocus.test.ts` around lines 13 - 15, The mock closest(selector: string) implementation on MockHTMLElement currently returns this when selector === ".thread-terminal-drawer .xterm" and isConnected is true, which simplifies real DOM ancestor traversal; update the MockHTMLElement.closest method to include a concise comment noting this simplification and that it intentionally does not traverse ancestors (e.g., "Simplified mock: returns this when connected for selector used in tests, does not mimic full DOM traversal"), so future readers/tests understand the behavior of closest and why it only checks isConnected.apps/server/src/git/Layers/GitManager.test.ts (1)
1954-2015: Consider adding explicit timeout for hook progress test.The test uses a
sleep 1in the pre-commit hook which could make the test flaky on slow CI environments. Consider adding an explicit timeout annotation to prevent unexpected failures.Example timeout annotation
- it.effect("emits ordered progress events for commit hooks", () => + it.effect("emits ordered progress events for commit hooks", { timeout: 10_000 }, () =>🤖 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 1954 - 2015, Add an explicit longer timeout to the flaky "emits ordered progress events for commit hooks" test to avoid CI timeouts caused by the hook's sleep; update the test declared via it.effect(...) (the test that sets up repoDir, writes the pre-commit hook and calls runStackedAction on manager) to specify a generous timeout (e.g., pass a timeout argument to it.effect if supported or call the test runner's setTimeout/jest.setTimeout before the test) so the pre-commit sleep and any slow CI scheduling won't cause spurious failures.apps/server/src/git/Layers/GitManager.ts (1)
776-832: Consider documenting the mutablecurrentHookNamepattern.The
currentHookNamevariable is mutated by hook callbacks and read in the final cleanup block. While this works correctly for single-threaded Effect execution, the pattern of mutating a captured variable from callbacks could be confusing. A brief inline comment explaining the lifecycle would help maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/git/Layers/GitManager.ts` around lines 776 - 832, Add a short inline comment near the mutable variable currentHookName (and/or the commitProgress block) explaining its lifecycle: initialized to null, set in onHookStarted, cleared in onHookFinished and again in the final cleanup after gitCore.commit, and that this mutation is safe because these callbacks run in the same single-threaded Effect execution context (not concurrently); reference the commitProgress object, the emit calls, and the final check after gitCore.commit so future readers understand why the captured mutable variable is used and when it is read/cleared.apps/web/src/routes/_chat.settings.tsx (2)
356-379: Async function called withvoiddiscards potential rejections.If
dialogs.confirmthrows, the error becomes an unhandled promise rejection. Consider wrapping with try/catch for defensive error handling.🛡️ Suggested error handling
async function restoreDefaults() { if (changedSettingLabels.length === 0) return; - const api = readNativeApi(); - const confirmed = await (api ?? ensureNativeApi()).dialogs.confirm( - ["Restore default settings?", `This will reset: ${changedSettingLabels.join(", ")}.`].join( - "\n", - ), - ); - if (!confirmed) return; + try { + const api = readNativeApi(); + const confirmed = await (api ?? ensureNativeApi()).dialogs.confirm( + ["Restore default settings?", `This will reset: ${changedSettingLabels.join(", ")}.`].join( + "\n", + ), + ); + if (!confirmed) return; + } catch { + return; + } setTheme("system");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/_chat.settings.tsx` around lines 356 - 379, restoreDefaults calls dialogs.confirm via (readNativeApi() ?? ensureNativeApi()).dialogs.confirm and does not handle promise rejections; wrap the await call in a try/catch inside restoreDefaults to catch errors from dialogs.confirm (or ensureNativeApi/readNativeApi) and handle them (e.g., console.error or show an error dialog) and return early on failure so the subsequent state mutations only run when confirmation succeeded.
231-233: Non-null assertion onfind()result is fragile.If
MODEL_PROVIDER_SETTINGSis ever modified and doesn't include theselectedCustomModelProvidervalue, this will throw at runtime.🔧 Suggested defensive fallback
- const selectedCustomModelProviderSettings = MODEL_PROVIDER_SETTINGS.find( - (providerSettings) => providerSettings.provider === selectedCustomModelProvider, - )!; + const selectedCustomModelProviderSettings = + MODEL_PROVIDER_SETTINGS.find( + (providerSettings) => providerSettings.provider === selectedCustomModelProvider, + ) ?? MODEL_PROVIDER_SETTINGS[0];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/_chat.settings.tsx` around lines 231 - 233, The current non-null assertion on the result of MODEL_PROVIDER_SETTINGS.find(...) (assigned to selectedCustomModelProviderSettings) is unsafe; change the code to handle the case where find returns undefined by checking the result and providing a safe fallback or error handling: call MODEL_PROVIDER_SETTINGS.find((providerSettings) => providerSettings.provider === selectedCustomModelProvider) without the "!" and if it returns undefined, either (a) select a sensible default entry from MODEL_PROVIDER_SETTINGS (e.g., MODEL_PROVIDER_SETTINGS[0]), or (b) log/throw a clear error and fall back to a known-safe settings object; update any code that uses selectedCustomModelProviderSettings to account for the fallback/nullable type.apps/web/src/components/ChatView.browser.tsx (1)
615-631: Polling loop has no rate limiting between iterations.The loop only waits for
waitForLayout()(3 animation frames) between shortcut dispatches. While acceptable for tests, this could cause rapid-fire events if the handler is slow.⏱️ Optional: add a small delay between iterations
async function triggerChatNewShortcutUntilPath( router: ReturnType<typeof getRouter>, predicate: (pathname: string) => boolean, errorMessage: string, ): Promise<string> { let pathname = router.state.location.pathname; const deadline = Date.now() + 8_000; while (Date.now() < deadline) { dispatchChatNewShortcut(); await waitForLayout(); + await new Promise((resolve) => setTimeout(resolve, 50)); pathname = router.state.location.pathname; if (predicate(pathname)) { return pathname; } } throw new Error(`${errorMessage} Last path: ${pathname}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/ChatView.browser.tsx` around lines 615 - 631, The polling loop in triggerChatNewShortcutUntilPath spams dispatchChatNewShortcut because it only awaits waitForLayout(); add a short fixed backoff between iterations (e.g., await a small sleep like 50–200ms) after waitForLayout() or before the next dispatch to rate-limit retries; update triggerChatNewShortcutUntilPath to call that delay (or a reusable sleep helper) so the loop yields CPU and avoids rapid-fire events while preserving the existing deadline and predicate checks.
🤖 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/serverLayers.ts`:
- Line 40: Remove the unused import "PtyAdapter" from the top of the file—it's
not referenced anywhere (the terminal layer supplies BunPtyAdapterLive or
NodePtyAdapterLive directly to TerminalManagerLive). Locate the import statement
that names PtyAdapter and delete it (or remove PtyAdapter from the import list)
so the file only imports actually used adapters; run the linter/formatter to
ensure no trailing commas or whitespace remain.
In `@apps/web/src/components/GitActionsControl.tsx`:
- Around line 72-81: ActiveGitActionProgress must capture the action's
launch-time context so progress events stay bound to the original action: extend
ActiveGitActionProgress to include the launch-time cwd and the toast/thread
metadata (e.g., gitCwd and threadToastData) when creating a new progress entry,
and then use those stored fields (instead of reading current render's gitCwd or
threadToastData) for filtering progress events and in toastManager.update calls;
update all places that create or update ActiveGitActionProgress (references
around ActiveGitActionProgress, toastManager.update, and code paths that read
gitCwd/threadToastData) to read the stored values from the progress object
rather than from the component state/props at render time.
---
Outside diff comments:
In `@apps/web/src/components/GitActionsControl.tsx`:
- Around line 407-420: runGitActionWithToast is currently created with
useEffectEvent but is invoked from click/menu/dialog handlers (e.g.
continuePendingDefaultBranchAction, runQuickAction, openDialogForMenuItem,
runDialogAction), and must be a stable event handler; replace useEffectEvent
with useCallback and supply a correct dependency array that includes all
external values referenced inside (at minimum gitStatusForActions and any props,
state, or refs used within the function) so the handler is safe to call from UI
event handlers and updates when its dependencies change.
In `@packages/contracts/src/git.ts`:
- Around line 74-86: Two tests in wsTransport.test.ts are sending payloads that
fail the new GitRunStackedActionInput schema because they omit the required
actionId; update the two request objects used in the tests "does not create a
timeout for requests with timeoutMs null" and "rejects pending requests when the
websocket closes" to include an actionId property (any non-empty trimmed string,
e.g. "test-action-1") so the payloads conform to GitRunStackedActionInput;
locate the request objects that currently are { cwd: "/repo" } and add actionId
alongside cwd.
---
Nitpick comments:
In `@apps/server/src/git/Layers/GitManager.test.ts`:
- Around line 1954-2015: Add an explicit longer timeout to the flaky "emits
ordered progress events for commit hooks" test to avoid CI timeouts caused by
the hook's sleep; update the test declared via it.effect(...) (the test that
sets up repoDir, writes the pre-commit hook and calls runStackedAction on
manager) to specify a generous timeout (e.g., pass a timeout argument to
it.effect if supported or call the test runner's setTimeout/jest.setTimeout
before the test) so the pre-commit sleep and any slow CI scheduling won't cause
spurious failures.
In `@apps/server/src/git/Layers/GitManager.ts`:
- Around line 776-832: Add a short inline comment near the mutable variable
currentHookName (and/or the commitProgress block) explaining its lifecycle:
initialized to null, set in onHookStarted, cleared in onHookFinished and again
in the final cleanup after gitCore.commit, and that this mutation is safe
because these callbacks run in the same single-threaded Effect execution context
(not concurrently); reference the commitProgress object, the emit calls, and the
final check after gitCore.commit so future readers understand why the captured
mutable variable is used and when it is read/cleared.
In `@apps/web/src/components/ChatView.browser.tsx`:
- Around line 615-631: The polling loop in triggerChatNewShortcutUntilPath spams
dispatchChatNewShortcut because it only awaits waitForLayout(); add a short
fixed backoff between iterations (e.g., await a small sleep like 50–200ms) after
waitForLayout() or before the next dispatch to rate-limit retries; update
triggerChatNewShortcutUntilPath to call that delay (or a reusable sleep helper)
so the loop yields CPU and avoids rapid-fire events while preserving the
existing deadline and predicate checks.
In `@apps/web/src/lib/terminalFocus.test.ts`:
- Around line 13-15: The mock closest(selector: string) implementation on
MockHTMLElement currently returns this when selector ===
".thread-terminal-drawer .xterm" and isConnected is true, which simplifies real
DOM ancestor traversal; update the MockHTMLElement.closest method to include a
concise comment noting this simplification and that it intentionally does not
traverse ancestors (e.g., "Simplified mock: returns this when connected for
selector used in tests, does not mimic full DOM traversal"), so future
readers/tests understand the behavior of closest and why it only checks
isConnected.
In `@apps/web/src/routes/_chat.settings.tsx`:
- Around line 356-379: restoreDefaults calls dialogs.confirm via
(readNativeApi() ?? ensureNativeApi()).dialogs.confirm and does not handle
promise rejections; wrap the await call in a try/catch inside restoreDefaults to
catch errors from dialogs.confirm (or ensureNativeApi/readNativeApi) and handle
them (e.g., console.error or show an error dialog) and return early on failure
so the subsequent state mutations only run when confirmation succeeded.
- Around line 231-233: The current non-null assertion on the result of
MODEL_PROVIDER_SETTINGS.find(...) (assigned to
selectedCustomModelProviderSettings) is unsafe; change the code to handle the
case where find returns undefined by checking the result and providing a safe
fallback or error handling: call MODEL_PROVIDER_SETTINGS.find((providerSettings)
=> providerSettings.provider === selectedCustomModelProvider) without the "!"
and if it returns undefined, either (a) select a sensible default entry from
MODEL_PROVIDER_SETTINGS (e.g., MODEL_PROVIDER_SETTINGS[0]), or (b) log/throw a
clear error and fall back to a known-safe settings object; update any code that
uses selectedCustomModelProviderSettings to account for the fallback/nullable
type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 680bdaeb-7f81-4c4d-814d-fa008e15c145
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (49)
.github/workflows/pr-size.yml.gitignoreapps/server/integration/OrchestrationEngineHarness.integration.tsapps/server/package.jsonapps/server/src/checkpointing/Layers/CheckpointStore.tsapps/server/src/git/Layers/GitCore.test.tsapps/server/src/git/Layers/GitCore.tsapps/server/src/git/Layers/GitManager.test.tsapps/server/src/git/Layers/GitManager.tsapps/server/src/git/Layers/GitService.test.tsapps/server/src/git/Layers/GitService.tsapps/server/src/git/Services/GitCore.tsapps/server/src/git/Services/GitManager.tsapps/server/src/git/Services/GitService.tsapps/server/src/orchestration/Layers/CheckpointReactor.test.tsapps/server/src/provider/Layers/ClaudeSdkFastMode.probe.test.tsapps/server/src/provider/Layers/HarnessClientManager.tsapps/server/src/serverLayers.tsapps/server/src/wsServer.test.tsapps/server/src/wsServer.tsapps/web/package.jsonapps/web/src/appSettings.test.tsapps/web/src/appSettings.tsapps/web/src/components/ChatView.browser.tsxapps/web/src/components/ChatView.tsxapps/web/src/components/GitActionsControl.tsxapps/web/src/components/Sidebar.tsxapps/web/src/components/chat/ChatHeader.tsxapps/web/src/components/ui/select.tsxapps/web/src/composerDraftStore.test.tsapps/web/src/composerDraftStore.tsapps/web/src/lib/gitReactQuery.tsapps/web/src/lib/terminalFocus.test.tsapps/web/src/lib/terminalFocus.tsapps/web/src/projectScripts.test.tsapps/web/src/projectScripts.tsapps/web/src/routes/_chat.settings.tsxapps/web/src/routes/_chat.tsxapps/web/src/wsNativeApi.test.tsapps/web/src/wsNativeApi.tsapps/web/src/wsTransport.test.tsapps/web/src/wsTransport.tspackages/contracts/src/git.test.tspackages/contracts/src/git.tspackages/contracts/src/ipc.tspackages/contracts/src/ws.test.tspackages/contracts/src/ws.tsscripts/claude-fast-mode-probe.tsscripts/claude-haiku-thinking-probe.ts
💤 Files with no reviewable changes (6)
- apps/server/src/git/Layers/GitService.test.ts
- apps/server/src/provider/Layers/ClaudeSdkFastMode.probe.test.ts
- scripts/claude-fast-mode-probe.ts
- apps/server/src/git/Services/GitService.ts
- apps/server/src/git/Layers/GitService.ts
- scripts/claude-haiku-thinking-probe.ts
| import { GitServiceLive } from "./git/Layers/GitService"; | ||
| import { BunPtyAdapterLive } from "./terminal/Layers/BunPTY"; | ||
| import { NodePtyAdapterLive } from "./terminal/Layers/NodePTY"; | ||
| import { PtyAdapter } from "./terminal/Services/PTY"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if PtyAdapter is used anywhere in serverLayers.ts
rg -n 'PtyAdapter' apps/server/src/serverLayers.tsRepository: Ranvier-Technologies/t3code-OTP
Length of output: 329
🏁 Script executed:
#!/bin/bash
# Read context around the import and usage
echo "=== Lines 35-50 (import section) ==="
sed -n '35,50p' apps/server/src/serverLayers.ts
echo ""
echo "=== Lines 165-180 (usage section) ==="
sed -n '165,180p' apps/server/src/serverLayers.tsRepository: Ranvier-Technologies/t3code-OTP
Length of output: 1384
Remove unused PtyAdapter import on line 40.
This import is not used anywhere in the file. The terminal layer provides BunPtyAdapterLive or NodePtyAdapterLive directly to TerminalManagerLive, without referencing the PtyAdapter service.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/serverLayers.ts` at line 40, Remove the unused import
"PtyAdapter" from the top of the file—it's not referenced anywhere (the terminal
layer supplies BunPtyAdapterLive or NodePtyAdapterLive directly to
TerminalManagerLive). Locate the import statement that names PtyAdapter and
delete it (or remove PtyAdapter from the import list) so the file only imports
actually used adapters; run the linter/formatter to ensure no trailing commas or
whitespace remain.
Cherry-picked d415558 from pingdotgg/t3code. - GitService flattened into GitCore (removed GitService.ts) - Server paths switched to base dir - Resolved GitCore.ts conflict (accepted upstream refactor) - Resolved serverLayers.ts conflict (kept our hybrid provider + PTY imports) - Fixed pushChannel → push rename in HarnessClientManager Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Cherry-picked 384f350 from pingdotgg/t3code. Streams git hook progress events for stacked git actions. Adds WS push channel, contracts, and UI progress indicators. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
xmlns was "http://www.w3.org/1500/svg" (typo from upstream cherry-pick). Fixed to "http://www.w3.org/2000/svg" so browsers render the checkmark. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ranvier2d2
force-pushed
the
chore/upstream-sync
branch
from
c2c8263 to
f7fe190
Compare
Upstream added actionId to GitRunStackedActionInput schema but didn't update these two test payloads. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ypecheck - Add type guards for union types in stress-test-memory-leak.ts - Add explicit parameter types in harness-dry-run.ts, test-harness-*.ts - Fix exactOptionalPropertyTypes issues with conditional spreads - Fix ChildProcess type mismatch in stress-test-real-claude.ts - Add ws + @types/ws to scripts devDependencies - Include imported server files in scripts/tsconfig.json - Skip scripts typecheck in CI (dev-only tooling, not production code) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
scripts/test-harness-mapping.ts (1)
174-191:⚠️ Potential issue | 🟡 MinorMissing ref normalization for phx_reply matching.
This file still uses
refdirectly forpending.has(ref)lookup (line 184), butpendingis keyed by string values. If Phoenix sends a numericref, the lookup will fail. The same issue was fixed intest-harness-connection.tsby normalizing torefStr.🔧 Proposed fix to normalize ref
ws.on("message", (raw: WebSocket.RawData) => { const msg = JSON.parse(raw.toString()); const [, ref, , event, payload] = msg as [ string | null, string | null, string, string, Record<string, unknown>, ]; - if (event === "phx_reply" && ref && pending.has(ref)) { - const { resolve, reject } = pending.get(ref)!; - pending.delete(ref); + const refStr = ref != null ? String(ref) : null; + if (event === "phx_reply" && refStr && pending.has(refStr)) { + const { resolve, reject } = pending.get(refStr)!; + pending.delete(refStr); const p = payload as { status: string; response: unknown }; if (p.status === "ok") resolve(p.response); else reject(new Error(JSON.stringify(p.response))); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test-harness-mapping.ts` around lines 174 - 191, In the ws.on("message") handler, normalize the incoming ref before using it to look up the pending map: replace direct uses of ref in pending.has(ref), pending.get(ref) and pending.delete(ref) with a string-normalized ref (e.g., refStr = String(ref ?? "") or similar) so numeric Phoenix refs match the string keys in pending; update any references in that block (the destructured ref, the pending.has/check/get/delete and the resolve/reject logic) to use the normalized ref variable (refStr).scripts/stress-test-memory-leak.ts (1)
1-1:⚠️ Potential issue | 🟡 MinorFix formatting issue flagged by CI.
The pipeline reports
oxfmt --checkfailed for this file. Run the formatter to resolve:oxfmt scripts/stress-test-memory-leak.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/stress-test-memory-leak.ts` at line 1, CI failed oxfmt for the file containing the shebang line "#!/usr/bin/env bun"; run the oxfmt formatter on stress-test-memory-leak.ts to fix formatting (e.g., invoke oxfmt for that file) and commit the formatted file so the pipeline check passes.scripts/stress-test-real-subagent.ts (1)
1-1:⚠️ Potential issue | 🟡 MinorFix formatting issue flagged by CI.
The pipeline reports
oxfmt --checkfailed for this file. Run the formatter to resolve:oxfmt scripts/stress-test-real-subagent.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/stress-test-real-subagent.ts` at line 1, CI failed oxfmt for the script that begins with the shebang "#!/usr/bin/env bun"; run the oxfmt formatter on that script to fix formatting inconsistencies and stage/commit the formatted file so the linter passes. Ensure the change is limited to formatting only (no logic edits) and include the formatted file in your PR update.apps/web/src/appSettings.ts (1)
55-60:⚠️ Potential issue | 🟡 MinorRemove the duplicate
claudeBinaryPathschema field.Line 59 duplicates
claudeBinaryPathfrom Line 56. The later entry overwrites the first one, and Biome already flags this as a duplicate-key error.🤖 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 55 - 60, The AppSettingsSchema contains a duplicate field name causing a duplicate-key error: remove the second duplicate entry of claudeBinaryPath from AppSettingsSchema so only one claudeBinaryPath property remains alongside codexBinaryPath, codexHomePath and defaultThreadEnvMode; if the duplicate was intended to be a different setting, rename that property appropriately instead of duplicating claudeBinaryPath.
🧹 Nitpick comments (1)
scripts/package.json (1)
9-9: Typecheck script now skips actual type checking.This change disables type checking in CI for the scripts package. While the echo message indicates developers should run
tsc --noEmitmanually, this reduces safety guarantees. Consider documenting why this was disabled (e.g., pre-existing errors, CI performance) in a comment or the PR description.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/package.json` at line 9, The "typecheck" npm script in package.json currently echoes a message and skips running tsc; either restore real type-checking by running tsc --noEmit in the "typecheck" script (replacing the echo) so CI enforces types, or if skipping is intentional keep the echo but add a clear comment and PR description explaining why (e.g., known TS errors, CI perf) and a follow-up issue to re-enable type checks; reference the "typecheck" script entry in package.json when making the change.
🤖 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/git/Layers/GitCore.ts`:
- Around line 740-748: The git fetch call in GitCore.fetchUpstreamRefForStatus
is suppressing failures by using allowNonZeroExit: true so transient
auth/network errors are cached as success; change the executeGit options to not
allow non-zero exits (remove allowNonZeroExit or set it to false) so a failing
"git fetch" will surface an error (preserving the existing
timeoutMs/Duration.toMillis(STATUS_UPSTREAM_REFRESH_TIMEOUT) option) and let
callers handle/ignore refresh failures as intended.
- Around line 1240-1261: The current no-upstream path wrongly returns
skipped_up_to_date too early; instead, remove the early return when
publishRemoteName is null so branches with no configured remote continue to the
push -u path, and when remoteBranchExists(cwd, publishRemoteName, branch)
returns true do not immediately return skipped—add a proper equality check
(e.g., implement or call an isBranchUpToDate / compareCommits helper that
compares local HEAD and remote ref via git rev-parse / merge-base) and only
return {status: "skipped_up_to_date", branch} when the local and remote commits
are identical; otherwise proceed with the push/upstream-setting flow so
divergent remote branches surface push errors and upstream gets set. Ensure to
reference resolveBaseBranchForNoUpstream, resolvePushRemoteName,
remoteBranchExists, and add/use isBranchUpToDate or compareCommits for the
check.
- Around line 1755-1763: The checkout logic currently uses input.branch for
remote refs which can detach HEAD; update the conditional that builds
checkoutArgs so when input.branch is a remote ref (e.g. "origin/feature") and a
corresponding local branch exists you use localTrackedBranchCandidate (or
localTrackingBranchCandidate) instead of input.branch; specifically modify the
branch of the ternary that now returns ["checkout", input.branch] for
remoteExists && !localTrackingBranch && localTrackedBranchTargetExists to return
["checkout", localTrackedBranchCandidate] (and ensure any other branches that
handle remoteExists prefer localTrackingBranch or localTrackedBranchCandidate
before falling back to input.branch).
In `@apps/web/src/appSettings.ts`:
- Around line 275-296: getProviderStartOptions is forwarding raw settings values
which may contain whitespace-only or trailing spaces; trim
settings.codexBinaryPath, settings.codexHomePath and settings.claudeBinaryPath,
drop any that become empty after trimming, and only build the
ProviderStartOptions entries using the trimmed non-empty strings so codex and
claudeAgent keys are omitted when their trimmed paths are blank; ensure you
reference the getProviderStartOptions function and the ProviderStartOptions
shape when applying this change.
In `@apps/web/src/composerDraftStore.ts`:
- Around line 854-856: verifyPersistedAttachments currently closes over the
passed-in attachments array which can become stale if syncPersistedAttachments
is called again before the deferred verifier runs; update the verifier to
re-read the latest staged attachments for the thread (e.g., from the same
stagedAttachments store or a getStagedAttachments(threadId) accessor) inside the
deferred callback before filtering/persisting so it always operates on the most
recent set; apply the same change to the other deferred verifiers referenced
(the similar closures around lines 880-882 and 1726-1728) so none of them use a
captured attachments array.
- Around line 867-873: If composerDebouncedStorage.flush() throws, the current
code abandons reading the last persisted snapshot and resets persistedIdSet to
empty, which marks previously persisted attachments as non-persisted; fix by
still calling readPersistedAttachmentIdsFromStorage(threadId) in the catch path
and using its result to initialize persistedIdSet (optionally log the flush
error for diagnostics) so previously persisted attachment IDs are preserved even
when flush fails.
---
Outside diff comments:
In `@apps/web/src/appSettings.ts`:
- Around line 55-60: The AppSettingsSchema contains a duplicate field name
causing a duplicate-key error: remove the second duplicate entry of
claudeBinaryPath from AppSettingsSchema so only one claudeBinaryPath property
remains alongside codexBinaryPath, codexHomePath and defaultThreadEnvMode; if
the duplicate was intended to be a different setting, rename that property
appropriately instead of duplicating claudeBinaryPath.
In `@scripts/stress-test-memory-leak.ts`:
- Line 1: CI failed oxfmt for the file containing the shebang line
"#!/usr/bin/env bun"; run the oxfmt formatter on stress-test-memory-leak.ts to
fix formatting (e.g., invoke oxfmt for that file) and commit the formatted file
so the pipeline check passes.
In `@scripts/stress-test-real-subagent.ts`:
- Line 1: CI failed oxfmt for the script that begins with the shebang
"#!/usr/bin/env bun"; run the oxfmt formatter on that script to fix formatting
inconsistencies and stage/commit the formatted file so the linter passes. Ensure
the change is limited to formatting only (no logic edits) and include the
formatted file in your PR update.
In `@scripts/test-harness-mapping.ts`:
- Around line 174-191: In the ws.on("message") handler, normalize the incoming
ref before using it to look up the pending map: replace direct uses of ref in
pending.has(ref), pending.get(ref) and pending.delete(ref) with a
string-normalized ref (e.g., refStr = String(ref ?? "") or similar) so numeric
Phoenix refs match the string keys in pending; update any references in that
block (the destructured ref, the pending.has/check/get/delete and the
resolve/reject logic) to use the normalized ref variable (refStr).
---
Nitpick comments:
In `@scripts/package.json`:
- Line 9: The "typecheck" npm script in package.json currently echoes a message
and skips running tsc; either restore real type-checking by running tsc --noEmit
in the "typecheck" script (replacing the echo) so CI enforces types, or if
skipping is intentional keep the echo but add a clear comment and PR description
explaining why (e.g., known TS errors, CI perf) and a follow-up issue to
re-enable type checks; reference the "typecheck" script entry in package.json
when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 88f4c733-8dcb-4142-890f-d04e6942c56d
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (64)
.github/workflows/pr-size.yml.gitignoreapps/server/integration/OrchestrationEngineHarness.integration.tsapps/server/package.jsonapps/server/src/attachmentPaths.test.tsapps/server/src/checkpointing/Layers/CheckpointStore.tsapps/server/src/git/Layers/GitCore.test.tsapps/server/src/git/Layers/GitCore.tsapps/server/src/git/Layers/GitManager.test.tsapps/server/src/git/Layers/GitManager.tsapps/server/src/git/Layers/GitService.test.tsapps/server/src/git/Layers/GitService.tsapps/server/src/git/Services/GitCore.tsapps/server/src/git/Services/GitManager.tsapps/server/src/git/Services/GitService.tsapps/server/src/main.tsapps/server/src/orchestration/Layers/CheckpointReactor.test.tsapps/server/src/orchestration/Layers/ProviderRuntimeIngestion.test.tsapps/server/src/provider/Layers/ClaudeSdkFastMode.probe.test.tsapps/server/src/provider/Layers/HarnessClientAdapter.tsapps/server/src/serverLayers.tsapps/server/src/wsServer.test.tsapps/server/src/wsServer.tsapps/web/package.jsonapps/web/src/appSettings.test.tsapps/web/src/appSettings.tsapps/web/src/components/ChatView.browser.tsxapps/web/src/components/ChatView.tsxapps/web/src/components/GitActionsControl.tsxapps/web/src/components/Sidebar.tsxapps/web/src/components/chat/ChatHeader.tsxapps/web/src/components/ui/select.tsxapps/web/src/composerDraftStore.test.tsapps/web/src/composerDraftStore.tsapps/web/src/lib/gitReactQuery.tsapps/web/src/lib/terminalFocus.test.tsapps/web/src/lib/terminalFocus.tsapps/web/src/projectScripts.test.tsapps/web/src/projectScripts.tsapps/web/src/routes/_chat.settings.tsxapps/web/src/routes/_chat.tsxapps/web/src/wsNativeApi.test.tsapps/web/src/wsNativeApi.tsapps/web/src/wsTransport.test.tsapps/web/src/wsTransport.tspackages/contracts/src/git.test.tspackages/contracts/src/git.tspackages/contracts/src/ipc.tspackages/contracts/src/ws.test.tspackages/contracts/src/ws.tsscripts/benchmark-analyze.tsscripts/claude-fast-mode-probe.tsscripts/claude-haiku-thinking-probe.tsscripts/harness-dry-run.tsscripts/package.jsonscripts/stress-test-exception.tsscripts/stress-test-memory-leak.tsscripts/stress-test-real-claude.tsscripts/stress-test-real-subagent.tsscripts/stress-test-scale50.tsscripts/test-harness-connection.tsscripts/test-harness-mapping.tsscripts/test-harness-prompt.tsscripts/tsconfig.json
💤 Files with no reviewable changes (6)
- apps/server/src/provider/Layers/ClaudeSdkFastMode.probe.test.ts
- apps/server/src/git/Layers/GitService.ts
- apps/server/src/git/Services/GitService.ts
- apps/server/src/git/Layers/GitService.test.ts
- scripts/claude-fast-mode-probe.ts
- scripts/claude-haiku-thinking-probe.ts
✅ Files skipped from review due to trivial changes (17)
- apps/server/src/attachmentPaths.test.ts
- .gitignore
- apps/web/package.json
- apps/server/package.json
- apps/web/src/lib/terminalFocus.ts
- apps/server/src/provider/Layers/HarnessClientAdapter.ts
- apps/server/src/main.ts
- scripts/tsconfig.json
- scripts/test-harness-prompt.ts
- packages/contracts/src/ws.test.ts
- scripts/stress-test-scale50.ts
- apps/web/src/composerDraftStore.test.ts
- packages/contracts/src/git.test.ts
- scripts/stress-test-exception.ts
- apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.test.ts
- apps/web/src/routes/_chat.tsx
- apps/server/integration/OrchestrationEngineHarness.integration.ts
🚧 Files skipped from review as they are similar to previous changes (19)
- apps/web/src/wsTransport.test.ts
- apps/web/src/lib/gitReactQuery.ts
- apps/web/src/appSettings.test.ts
- apps/server/src/orchestration/Layers/CheckpointReactor.test.ts
- apps/web/src/projectScripts.ts
- apps/web/src/components/Sidebar.tsx
- apps/web/src/components/chat/ChatHeader.tsx
- packages/contracts/src/ipc.ts
- apps/web/src/lib/terminalFocus.test.ts
- apps/server/src/git/Services/GitManager.ts
- apps/web/src/components/ui/select.tsx
- packages/contracts/src/ws.ts
- apps/server/src/git/Layers/GitManager.test.ts
- apps/server/src/git/Services/GitCore.ts
- apps/server/src/git/Layers/GitManager.ts
- packages/contracts/src/git.ts
- apps/server/src/git/Layers/GitCore.test.ts
- .github/workflows/pr-size.yml
- apps/web/src/projectScripts.test.ts
Comment on lines
+740
to
+748
| return executeGit( | ||
| "GitCore.fetchUpstreamRefForStatus", | ||
| cwd, | ||
| ["fetch", "--quiet", "--no-tags", upstream.remoteName, refspec], | ||
| { | ||
| allowNonZeroExit: true, | ||
| timeoutMs: Duration.toMillis(STATUS_UPSTREAM_REFRESH_TIMEOUT), | ||
| }, | ||
| ).pipe(Effect.asVoid); |
There was a problem hiding this comment.
Let failed status refreshes fail.
The caller already ignores refresh failures. With allowNonZeroExit: true, a non-zero git fetch is cached as a success here, so stale upstream data stays warm for the full TTL after auth/network errors.
🩹 Suggested fix
const fetchUpstreamRefForStatus = (
cwd: string,
upstream: { upstreamRef: string; remoteName: string; upstreamBranch: string },
): Effect.Effect<void, GitCommandError> => {
const refspec = `+refs/heads/${upstream.upstreamBranch}:refs/remotes/${upstream.upstreamRef}`;
return executeGit(
"GitCore.fetchUpstreamRefForStatus",
cwd,
["fetch", "--quiet", "--no-tags", upstream.remoteName, refspec],
{
- allowNonZeroExit: true,
timeoutMs: Duration.toMillis(STATUS_UPSTREAM_REFRESH_TIMEOUT),
},
).pipe(Effect.asVoid);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return executeGit( | |
| "GitCore.fetchUpstreamRefForStatus", | |
| cwd, | |
| ["fetch", "--quiet", "--no-tags", upstream.remoteName, refspec], | |
| { | |
| allowNonZeroExit: true, | |
| timeoutMs: Duration.toMillis(STATUS_UPSTREAM_REFRESH_TIMEOUT), | |
| }, | |
| ).pipe(Effect.asVoid); | |
| return executeGit( | |
| "GitCore.fetchUpstreamRefForStatus", | |
| cwd, | |
| ["fetch", "--quiet", "--no-tags", upstream.remoteName, refspec], | |
| { | |
| timeoutMs: Duration.toMillis(STATUS_UPSTREAM_REFRESH_TIMEOUT), | |
| }, | |
| ).pipe(Effect.asVoid); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/git/Layers/GitCore.ts` around lines 740 - 748, The git fetch
call in GitCore.fetchUpstreamRefForStatus is suppressing failures by using
allowNonZeroExit: true so transient auth/network errors are cached as success;
change the executeGit options to not allow non-zero exits (remove
allowNonZeroExit or set it to false) so a failing "git fetch" will surface an
error (preserving the existing
timeoutMs/Duration.toMillis(STATUS_UPSTREAM_REFRESH_TIMEOUT) option) and let
callers handle/ignore refresh failures as intended.
Comment on lines
+1240
to
+1261
| const comparableBaseBranch = yield* resolveBaseBranchForNoUpstream(cwd, branch).pipe( | ||
| Effect.catch(() => Effect.succeed(null)), | ||
| ); | ||
| if (hasRemoteBranch) { | ||
| return { | ||
| status: "skipped_up_to_date" as const, | ||
| branch, | ||
| }; | ||
| if (comparableBaseBranch) { | ||
| const publishRemoteName = yield* resolvePushRemoteName(cwd, branch).pipe( | ||
| Effect.catch(() => Effect.succeed(null)), | ||
| ); | ||
| if (!publishRemoteName) { | ||
| return { | ||
| status: "skipped_up_to_date" as const, | ||
| branch, | ||
| }; | ||
| } | ||
|
|
||
| const hasRemoteBranch = yield* remoteBranchExists(cwd, publishRemoteName, branch).pipe( | ||
| Effect.catch(() => Effect.succeed(false)), | ||
| ); | ||
| if (hasRemoteBranch) { | ||
| return { | ||
| status: "skipped_up_to_date" as const, | ||
| branch, | ||
| }; |
There was a problem hiding this comment.
Don't short-circuit the no-upstream path as skipped_up_to_date.
This block can return skipped_up_to_date for a brand-new branch with no remote configured, and it can also skip the existing git push -u path for a same-named remote branch that actually diverges. That prevents the real push error from surfacing and never sets upstream.
🩹 Suggested fix
- const comparableBaseBranch = yield* resolveBaseBranchForNoUpstream(cwd, branch).pipe(
- Effect.catch(() => Effect.succeed(null)),
- );
- if (comparableBaseBranch) {
- const publishRemoteName = yield* resolvePushRemoteName(cwd, branch).pipe(
- Effect.catch(() => Effect.succeed(null)),
- );
- if (!publishRemoteName) {
- return {
- status: "skipped_up_to_date" as const,
- branch,
- };
- }
-
- const hasRemoteBranch = yield* remoteBranchExists(cwd, publishRemoteName, branch).pipe(
- Effect.catch(() => Effect.succeed(false)),
- );
- if (hasRemoteBranch) {
- return {
- status: "skipped_up_to_date" as const,
- branch,
- };
- }
- }
+ // Fall through: the no-upstream branch below should either set
+ // upstream with `git push -u` or surface the real push error.
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/git/Layers/GitCore.ts` around lines 1240 - 1261, The current
no-upstream path wrongly returns skipped_up_to_date too early; instead, remove
the early return when publishRemoteName is null so branches with no configured
remote continue to the push -u path, and when remoteBranchExists(cwd,
publishRemoteName, branch) returns true do not immediately return skipped—add a
proper equality check (e.g., implement or call an isBranchUpToDate /
compareCommits helper that compares local HEAD and remote ref via git rev-parse
/ merge-base) and only return {status: "skipped_up_to_date", branch} when the
local and remote commits are identical; otherwise proceed with the
push/upstream-setting flow so divergent remote branches surface push errors and
upstream gets set. Ensure to reference resolveBaseBranchForNoUpstream,
resolvePushRemoteName, remoteBranchExists, and add/use isBranchUpToDate or
compareCommits for the check.
Comment on lines
+1755
to
+1763
| const checkoutArgs = localInputExists | ||
| ? ["checkout", input.branch] | ||
| : remoteExists && !localTrackingBranch | ||
| ? ["checkout", "--track", input.branch] | ||
| : remoteExists && localTrackingBranch | ||
| ? ["checkout", localTrackingBranch] | ||
| : ["checkout", input.branch]; | ||
| : remoteExists && !localTrackingBranch && localTrackedBranchTargetExists | ||
| ? ["checkout", input.branch] | ||
| : remoteExists && !localTrackingBranch | ||
| ? ["checkout", "--track", input.branch] | ||
| : remoteExists && localTrackingBranch | ||
| ? ["checkout", localTrackingBranch] | ||
| : ["checkout", input.branch]; |
There was a problem hiding this comment.
Use the existing local branch here.
When input.branch is a remote ref like origin/feature and local feature already exists, git checkout origin/feature detaches HEAD. This branch should checkout localTrackedBranchCandidate instead.
🩹 Suggested fix
const checkoutArgs = localInputExists
? ["checkout", input.branch]
: remoteExists && !localTrackingBranch && localTrackedBranchTargetExists
- ? ["checkout", input.branch]
+ ? ["checkout", localTrackedBranchCandidate!]
: remoteExists && !localTrackingBranch
? ["checkout", "--track", input.branch]
: remoteExists && localTrackingBranch
? ["checkout", localTrackingBranch]
: ["checkout", input.branch];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const checkoutArgs = localInputExists | |
| ? ["checkout", input.branch] | |
| : remoteExists && !localTrackingBranch | |
| ? ["checkout", "--track", input.branch] | |
| : remoteExists && localTrackingBranch | |
| ? ["checkout", localTrackingBranch] | |
| : ["checkout", input.branch]; | |
| : remoteExists && !localTrackingBranch && localTrackedBranchTargetExists | |
| ? ["checkout", input.branch] | |
| : remoteExists && !localTrackingBranch | |
| ? ["checkout", "--track", input.branch] | |
| : remoteExists && localTrackingBranch | |
| ? ["checkout", localTrackingBranch] | |
| : ["checkout", input.branch]; | |
| const checkoutArgs = localInputExists | |
| ? ["checkout", input.branch] | |
| : remoteExists && !localTrackingBranch && localTrackedBranchTargetExists | |
| ? ["checkout", localTrackedBranchCandidate!] | |
| : remoteExists && !localTrackingBranch | |
| ? ["checkout", "--track", input.branch] | |
| : remoteExists && localTrackingBranch | |
| ? ["checkout", localTrackingBranch] | |
| : ["checkout", input.branch]; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/git/Layers/GitCore.ts` around lines 1755 - 1763, The checkout
logic currently uses input.branch for remote refs which can detach HEAD; update
the conditional that builds checkoutArgs so when input.branch is a remote ref
(e.g. "origin/feature") and a corresponding local branch exists you use
localTrackedBranchCandidate (or localTrackingBranchCandidate) instead of
input.branch; specifically modify the branch of the ternary that now returns
["checkout", input.branch] for remoteExists && !localTrackingBranch &&
localTrackedBranchTargetExists to return ["checkout",
localTrackedBranchCandidate] (and ensure any other branches that handle
remoteExists prefer localTrackingBranch or localTrackedBranchCandidate before
falling back to input.branch).
Comment on lines
+275
to
+296
| export function getProviderStartOptions( | ||
| settings: Pick<AppSettings, "claudeBinaryPath" | "codexBinaryPath" | "codexHomePath">, | ||
| ): ProviderStartOptions | undefined { | ||
| const providerOptions: ProviderStartOptions = { | ||
| ...(settings.codexBinaryPath || settings.codexHomePath | ||
| ? { | ||
| codex: { | ||
| ...(settings.codexBinaryPath ? { binaryPath: settings.codexBinaryPath } : {}), | ||
| ...(settings.codexHomePath ? { homePath: settings.codexHomePath } : {}), | ||
| }, | ||
| } | ||
| : {}), | ||
| ...(settings.claudeBinaryPath | ||
| ? { | ||
| claudeAgent: { | ||
| binaryPath: settings.claudeBinaryPath, | ||
| }, | ||
| } | ||
| : {}), | ||
| }; | ||
|
|
||
| return Object.keys(providerOptions).length > 0 ? providerOptions : undefined; |
There was a problem hiding this comment.
Trim and drop blank provider override paths before dispatch.
ProviderStartOptions expects trimmed non-empty strings, but this helper forwards raw settings values as-is. A copied path with trailing whitespace, or a whitespace-only input, will currently produce an invalid providerOptions payload and can block provider startup.
Suggested fix
export function getProviderStartOptions(
settings: Pick<AppSettings, "claudeBinaryPath" | "codexBinaryPath" | "codexHomePath">,
): ProviderStartOptions | undefined {
+ const codexBinaryPath = settings.codexBinaryPath.trim();
+ const codexHomePath = settings.codexHomePath.trim();
+ const claudeBinaryPath = settings.claudeBinaryPath.trim();
+
const providerOptions: ProviderStartOptions = {
- ...(settings.codexBinaryPath || settings.codexHomePath
+ ...(codexBinaryPath || codexHomePath
? {
codex: {
- ...(settings.codexBinaryPath ? { binaryPath: settings.codexBinaryPath } : {}),
- ...(settings.codexHomePath ? { homePath: settings.codexHomePath } : {}),
+ ...(codexBinaryPath ? { binaryPath: codexBinaryPath } : {}),
+ ...(codexHomePath ? { homePath: codexHomePath } : {}),
},
}
: {}),
- ...(settings.claudeBinaryPath
+ ...(claudeBinaryPath
? {
claudeAgent: {
- binaryPath: settings.claudeBinaryPath,
+ binaryPath: claudeBinaryPath,
},
}
: {}),
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function getProviderStartOptions( | |
| settings: Pick<AppSettings, "claudeBinaryPath" | "codexBinaryPath" | "codexHomePath">, | |
| ): ProviderStartOptions | undefined { | |
| const providerOptions: ProviderStartOptions = { | |
| ...(settings.codexBinaryPath || settings.codexHomePath | |
| ? { | |
| codex: { | |
| ...(settings.codexBinaryPath ? { binaryPath: settings.codexBinaryPath } : {}), | |
| ...(settings.codexHomePath ? { homePath: settings.codexHomePath } : {}), | |
| }, | |
| } | |
| : {}), | |
| ...(settings.claudeBinaryPath | |
| ? { | |
| claudeAgent: { | |
| binaryPath: settings.claudeBinaryPath, | |
| }, | |
| } | |
| : {}), | |
| }; | |
| return Object.keys(providerOptions).length > 0 ? providerOptions : undefined; | |
| export function getProviderStartOptions( | |
| settings: Pick<AppSettings, "claudeBinaryPath" | "codexBinaryPath" | "codexHomePath">, | |
| ): ProviderStartOptions | undefined { | |
| const codexBinaryPath = settings.codexBinaryPath?.trim() || ''; | |
| const codexHomePath = settings.codexHomePath?.trim() || ''; | |
| const claudeBinaryPath = settings.claudeBinaryPath?.trim() || ''; | |
| const providerOptions: ProviderStartOptions = { | |
| ...(codexBinaryPath || codexHomePath | |
| ? { | |
| codex: { | |
| ...(codexBinaryPath ? { binaryPath: codexBinaryPath } : {}), | |
| ...(codexHomePath ? { homePath: codexHomePath } : {}), | |
| }, | |
| } | |
| : {}), | |
| ...(claudeBinaryPath | |
| ? { | |
| claudeAgent: { | |
| binaryPath: claudeBinaryPath, | |
| }, | |
| } | |
| : {}), | |
| }; | |
| return Object.keys(providerOptions).length > 0 ? providerOptions : undefined; | |
| } |
🤖 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 275 - 296, getProviderStartOptions
is forwarding raw settings values which may contain whitespace-only or trailing
spaces; trim settings.codexBinaryPath, settings.codexHomePath and
settings.claudeBinaryPath, drop any that become empty after trimming, and only
build the ProviderStartOptions entries using the trimmed non-empty strings so
codex and claudeAgent keys are omitted when their trimmed paths are blank;
ensure you reference the getProviderStartOptions function and the
ProviderStartOptions shape when applying this change.
Comment on lines
+854
to
+856
| function verifyPersistedAttachments( | ||
| threadId: ThreadId, | ||
| attachments: PersistedComposerImageAttachment[], |
There was a problem hiding this comment.
Use the latest staged attachments inside the deferred verifier.
verifyPersistedAttachments() runs in a later microtask. If syncPersistedAttachments() is called again before that verifier runs, the older verifier filters against its captured attachments array and can overwrite newer staged attachments; the next flush then persists that truncated set.
Suggested fix
function verifyPersistedAttachments(
threadId: ThreadId,
- attachments: PersistedComposerImageAttachment[],
set: (
partial:
| ComposerDraftStoreState
@@
set((state) => {
const current = state.draftsByThreadId[threadId];
if (!current) {
return state;
}
const imageIdSet = new Set(current.images.map((image) => image.id));
- const persistedAttachments = attachments.filter(
+ const persistedAttachments = current.persistedAttachments.filter(
(attachment) => imageIdSet.has(attachment.id) && persistedIdSet.has(attachment.id),
);
@@
Promise.resolve().then(() => {
- verifyPersistedAttachments(threadId, attachments, set);
+ verifyPersistedAttachments(threadId, set);
});Also applies to: 880-882, 1726-1728
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/composerDraftStore.ts` around lines 854 - 856,
verifyPersistedAttachments currently closes over the passed-in attachments array
which can become stale if syncPersistedAttachments is called again before the
deferred verifier runs; update the verifier to re-read the latest staged
attachments for the thread (e.g., from the same stagedAttachments store or a
getStagedAttachments(threadId) accessor) inside the deferred callback before
filtering/persisting so it always operates on the most recent set; apply the
same change to the other deferred verifiers referenced (the similar closures
around lines 880-882 and 1726-1728) so none of them use a captured attachments
array.
Comment on lines
+867
to
+873
| let persistedIdSet = new Set<string>(); | ||
| try { | ||
| composerDebouncedStorage.flush(); | ||
| persistedIdSet = new Set(readPersistedAttachmentIdsFromStorage(threadId)); | ||
| } catch { | ||
| persistedIdSet = new Set(); | ||
| } |
There was a problem hiding this comment.
Keep reading the last persisted attachment snapshot after a failed flush.
If composerDebouncedStorage.flush() throws, this falls straight back to an empty set. Previously persisted attachments then get marked as non-persisted, and the next successful write can erase them from storage.
Suggested fix
- let persistedIdSet = new Set<string>();
- try {
- composerDebouncedStorage.flush();
- persistedIdSet = new Set(readPersistedAttachmentIdsFromStorage(threadId));
- } catch {
- persistedIdSet = new Set();
- }
+ try {
+ composerDebouncedStorage.flush();
+ } catch {
+ // Preserve the last successfully persisted snapshot if the newest write fails.
+ }
+ const persistedIdSet = new Set(readPersistedAttachmentIdsFromStorage(threadId));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/composerDraftStore.ts` around lines 867 - 873, If
composerDebouncedStorage.flush() throws, the current code abandons reading the
last persisted snapshot and resets persistedIdSet to empty, which marks
previously persisted attachments as non-persisted; fix by still calling
readPersistedAttachmentIdsFromStorage(threadId) in the catch path and using its
result to initialize persistedIdSet (optionally log the flush error for
diagnostics) so previously persisted attachment IDs are preserved even when
flush fails.
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
Cherry-picks 9 of 11 upstream commits from pingdotgg/t3code (2 already in our main).
Plus: fix SVG namespace typo in select.tsx (
1500/svg→2000/svg).Test plan
mix compile— Elixir compiles cleantsc --noEmit— TypeScript compiles (1 pre-existing error)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Bug Fixes