Integrate 8 upstream commits (security, Claude git text gen, UI fixes)#35
Merged
aaditagrawal merged 11 commits intomainfrom Mar 26, 2026
Merged
Integrate 8 upstream commits (security, Claude git text gen, UI fixes)#35aaditagrawal merged 11 commits intomainfrom
aaditagrawal merged 11 commits intomainfrom
Conversation
Co-authored-by: Jakob Guldberg Aaes <jakob1379@gmali.com>
Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: cursor[bot] <206951365+cursor[bot]@users.noreply.github.com>
pingdotgg#1423) Co-authored-by: morgan ye <morganye@morgans-MacBook-Pro.local>
Port upstream sideOffset={4} fix and submenu gap test to fork's
multi-provider picker structure.
Co-authored-by: keyzou <keyzou@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Julius Marminge <julius0216@outlook.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a one-shot FD-based bootstrap envelope mechanism for startup config, integrates per-request ModelSelection routing between Codex and Claude providers, centralizes prompt/schema utilities, threads modelSelection through server/web/orchestration layers, and updates desktop spawn/stdio to deliver the bootstrap envelope. Changes
Sequence Diagram(s)sequenceDiagram
participant Parent as Desktop Main
participant OS as Operating System
participant Child as Backend Process
Parent->>OS: spawn backend with FD 3 included in stdio
Note right of OS: FD 3 inherited by Child
Parent->>Child: wait for child ready
Parent->>OS: write one-line JSON envelope to FD 3 and close
OS->>Child: deliver envelope on FD 3
Child->>Child: read line, decode schema, apply bootstrap config
alt write to FD fails or FD closed
Child->>Parent: exit / no bootstrap -> Parent schedules restart
end
sequenceDiagram
participant Client as Requester (service)
participant Router as RoutingTextGeneration
participant Codex as CodexTextGeneration
participant Claude as ClaudeTextGeneration
Client->>Router: generateCommitMessage(input with modelSelection)
Router->>Router: inspect input.modelSelection.provider
alt provider == "claudeAgent"
Router->>Claude: forward request (propagate modelSelection)
Claude->>Router: structured JSON response
else
Router->>Codex: forward request (propagate modelSelection)
Codex->>Router: structured JSON response
end
Router->>Client: validate, sanitize, return output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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
vouch:trusted
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/server/src/serverLayers.ts (1)
111-156:⚠️ Potential issue | 🟠 MajorDon't wire this router globally until it can actually dispatch all supported providers.
git.runStackedActionnow carriesmodelSelectionfor everyProviderKind, and the web client forwards the active provider. Since this router only special-casesclaudeAgentand falls back to Codex for everything else, requests from Cursor/Copilot/Gemini/Amp/Kilo will either hit Codex with unsupported model IDs or silently lose provider-specific options.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/serverLayers.ts` around lines 111 - 156, The global wiring of the Git router should be gated until it can route by ProviderKind and honor modelSelection; stop registering the router globally in GitManagerLive / gitManagerLayer (and any initialization that exposes git.runStackedAction) and instead only wire it behind a feature-flag or provider-aware facade. Update the code paths that currently special-case claudeAgent and fallback to Codex so they instead validate ProviderKind + modelSelection and either dispatch to a provider-specific handler or return a clear unsupported-provider error; locate symbols GitManagerLive, gitManagerLayer, and any usages of git.runStackedAction to implement the guard and defer global registration until all ProviderKinds are supported. Ensure the web client’s forwarded provider is used to select the correct handler rather than defaulting to Codex.apps/server/src/git/Layers/GitManager.ts (1)
1276-1289:⚠️ Potential issue | 🟡 MinorFix the CI formatting failure here.
Line 1287 is still a single long
runPrStep(...)call, and CI is already failingoxfmt --checkon this file. Please reflow this block before merge.🤖 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 1276 - 1289, The long single-line call to runPrStep inside the pr assignment breaks the oxfmt layout; reflow the nested Effect.flatMap/Effect.gen block so the runPrStep call and its arguments are split across multiple lines to satisfy formatter rules. Locate the pr construction (the wantsPr ? yield* progress.emit(...).pipe(Effect.flatMap(() => Effect.gen(function* () { currentPhase = "pr"; return yield* runPrStep(input.modelSelection, input.cwd, currentBranch, input.provider, input.model); }), ...)) and reformat the inner generator to place currentPhase = "pr"; on its own line and the return yield* runPrStep(...) with each argument on its own line (or otherwise wrapped) so oxfmt --check passes. Ensure you only change formatting, preserving the use of runPrStep, progress.emit, Effect.flatMap, and Effect.gen.apps/server/src/git/Layers/CodexTextGeneration.ts (1)
142-159:⚠️ Potential issue | 🟠 MajorUse the normalized reasoning effort here.
Lines 142-159 compute
normalizedOptions, butreasoningEffortstill comes from rawmodelSelection.options. If the normalizer downgrades or strips unsupported effort values for a given model, this code will still emit the invalid one and make Codex requests fail.♻️ Proposed fix
const normalizedOptions = normalizeCodexModelOptions( modelSelection.model, modelSelection.options, ); const reasoningEffort = - modelSelection.options?.reasoningEffort ?? CODEX_GIT_TEXT_GENERATION_REASONING_EFFORT; + normalizedOptions?.reasoningEffort ?? CODEX_GIT_TEXT_GENERATION_REASONING_EFFORT; const command = ChildProcess.make(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/git/Layers/CodexTextGeneration.ts` around lines 142 - 159, The code computes normalizedOptions via normalizeCodexModelOptions but still reads reasoningEffort from modelSelection.options; update the reasoningEffort assignment to use the normalized value (e.g., normalizedOptions?.reasoningEffort ?? CODEX_GIT_TEXT_GENERATION_REASONING_EFFORT) so the ChildProcess.make call (the `command` construction) emits the validated/normalized effort; keep the existing fallback constant and leave the fastMode handling (normalizedOptions?.fastMode) unchanged.
🧹 Nitpick comments (4)
apps/web/src/components/ui/toast.tsx (1)
309-317: Consider extracting the repeated toast-header + copy-button block.The same header/copy logic appears in both non-anchored and anchored branches. A small shared component/helper would reduce duplication and future drift risk.
Also applies to: 403-411
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/ui/toast.tsx` around lines 309 - 317, Extract the duplicated header+copy-button markup into a small reusable component (e.g., ToastHeader or ToastHeaderWithCopy) that renders the container div, Toast.Title and conditionally the CopyErrorButton when toast.type === "error" and typeof toast.description === "string"; replace the two occurrences (the block containing Toast.Title and CopyErrorButton in the non-anchored and anchored branches) with this new component and pass the toast object or the minimal props (title slot and description/type) to keep behavior identical; ensure the new component uses the same classes ("flex items-center justify-between gap-1" and "min-w-0 break-words font-medium") and exports from the same file so imports remain straightforward.apps/web/src/components/chat/TraitsPicker.tsx (1)
121-140: Keep trigger-only props offTraitsMenuContent's public API.
triggerVariantandtriggerClassNameare added toTraitsMenuContentProps, butTraitsMenuContentnever reads them. Since the component is exported, this widens the API with no-op props and also lets those fields leak into the...persistencerest object. I'd split picker trigger props into a separateTraitsPickerPropstype.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/chat/TraitsPicker.tsx` around lines 121 - 140, The public props interface TraitsMenuContentProps incorrectly includes trigger-only props (triggerVariant, triggerClassName) that TraitsMenuContent does not consume and which leak into the rest/persistence object; remove these two props from TraitsMenuContentProps and instead define a separate TraitsPickerProps (or extend an existing picker prop type) that owns triggerVariant and triggerClassName, update any call sites to pass trigger props to the picker component rather than to TraitsMenuContent, and ensure TraitsMenuContentImpl only receives the intended props (provider, model, prompt, onPromptChange, modelOptions, allowPromptInjectedEffort and TraitsPersistence) so the ...persistence rest no longer contains triggerVariant/triggerClassName.apps/server/src/orchestration/Layers/ProviderCommandReactor.ts (1)
462-465: Use provider-aware model selection for worktree branch generation.Hardcoding Codex here bypasses the thread’s chosen model/provider. Prefer using thread selection when supported, with Codex as fallback.
Suggested refactor
+ const branchNameModelSelection = + thread.modelSelection.provider === "claudeAgent" + ? thread.modelSelection + : { + provider: "codex", + model: DEFAULT_GIT_TEXT_GENERATION_MODEL_BY_PROVIDER.codex, + }; + yield* textGeneration .generateBranchName({ cwd, message: input.messageText, ...(attachments.length > 0 ? { attachments } : {}), - modelSelection: { - provider: "codex", - model: DEFAULT_GIT_TEXT_GENERATION_MODEL_BY_PROVIDER.codex, - }, + modelSelection: branchNameModelSelection, })🤖 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 462 - 465, The modelSelection block currently hardcodes provider "codex" and DEFAULT_GIT_TEXT_GENERATION_MODEL_BY_PROVIDER.codex; change it to prefer the thread's chosen model/provider when available (use the thread or context modelSelection object used elsewhere in ProviderCommandReactor or the worktree branch generation code) and only fall back to DEFAULT_GIT_TEXT_GENERATION_MODEL_BY_PROVIDER.codex if the thread selection is missing or unsupported; update references to modelSelection so provider and model are derived from the thread's selection first, then fallback to the codex defaults.packages/contracts/src/git.ts (1)
80-85: Avoid keeping two sources of truth for the model selection.Now that
modelSelectionis required, leaving top-levelproviderandmodelin the same payload lets callers send contradictory values.apps/web/src/components/GitActionsControl.tsxalready sends both forms, so this contract is ambiguous until one of them is removed or clearly deprecated.🤖 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 80 - 85, The schema currently exposes two sources of truth—provider and model plus modelSelection—which allows contradictory payloads; remove the top-level provider and model fields from the exported schema and require callers to use modelSelection only (update any callers like GitActionsControl.tsx to stop sending provider/model), or if you need a transitional step, add a validation in the schema (in the file where provider, model, modelSelection are defined) that rejects payloads where provider/model are present alongside modelSelection or enforces they match modelSelection; update the Schema definitions for provider, model, and modelSelection accordingly and update callers to emit only modelSelection.
🤖 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/desktop/src/preload.ts`:
- Around line 22-25: The getWsUrl implementation currently returns empty string
from ipcRenderer.sendSync(GET_WS_URL_CHANNEL) as-is, which should be treated as
the “not ready” sentinel; update getWsUrl (in the preload export for
GET_WS_URL_CHANNEL) to normalize the result by returning null when result is not
a non-empty string (e.g., if typeof result !== "string" or result.trim() ===
""), otherwise return the string—this ensures callers expecting null for “not
ready yet” will receive null instead of "".
In `@apps/server/src/bootstrap.test.ts`:
- Around line 50-95: The two fd-related tests in bootstrap.test.ts that call
NFS.openSync("/dev/null"), execFileSync("mkfifo"), spawn("sh", ...) and use
readBootstrapEnvelope/TestEnvelopeSchema should be skipped on Windows; add a
platform guard at the start of each test body (check process.platform ===
"win32") and return an immediate Effect (e.g. Effect.unit) when on Windows so
the test is effectively no-op there, leaving the rest of the POSIX logic
(mkfifo, spawn, TestClock.adjust, Fiber.join) unchanged.
In `@apps/server/src/bootstrap.ts`:
- Around line 21-22: When a bootstrap FD was requested but unreadable the code
currently returns Option.none(), making an explicit bootstrap failure
indistinguishable from “no bootstrap requested”; change the behavior in the
isFdReady(fd) check (the fdReady/Option.none() branch) so that if a bootstrap FD
was explicitly provided (detect via the bootstrap FD flag / presence of fd) and
isFdReady(fd) returns false you fail closed (throw an Error or exit with
non‑zero) and log the failure, instead of returning Option.none(); apply the
same change to the other branches that collapse failures into Option.none() (the
blocks referencing Option.none() around the bootstrap FD handling).
In `@apps/server/src/git/Layers/ClaudeTextGeneration.test.ts`:
- Around line 19-60: The generated fake Claude shim in makeFakeClaudeBinary is
currently a POSIX shell script requiring chmod and using ':' as PATH delimiter
which breaks on Windows; replace it with a cross-platform Node-based shim (or
create both a Unix executable and a Windows .cmd wrapper) that reads env vars,
stdin and writes stdout/stderr/exit code accordingly, write the shim using the
existing FileSystem/FileSystem and Path/Path handles, only call fs.chmod for
non-Windows platforms, and compute the PATH delimiter via path.delimiter (or
Node's Path.path.delimiter) instead of hardcoding ':' so the tests run on
Windows and Unix.
In `@apps/server/src/git/Layers/ClaudeTextGeneration.ts`:
- Around line 89-103: The runClaudeCommand call constructs the Claude CLI args
incorrectly: ChildProcess.make currently passes JSON.stringify(settings) to the
--settings flag (which expects a file path) and uses
--dangerously-skip-permissions (too-broad). Fix by persisting settings to a temp
file and passing that filepath to --settings (or drop the flag if settings is
empty/not needed) and replace --dangerously-skip-permissions with explicit tool
restrictions e.g. add --allowedTools "Read" and --disallowedTools
"Edit,Bash,Write" when building the args in runClaudeCommand/ChildProcess.make
so the CLI receives a settings filepath and least-privilege permissions.
In `@apps/server/src/git/Layers/GitManager.test.ts`:
- Around line 37-40: DEFAULT_TEST_MODEL_SELECTION is forcing Codex
modelSelection into tests and masking provider/model routing regressions; update
the test helpers so runStackedAction does not unconditionally inject
DEFAULT_TEST_MODEL_SELECTION. Specifically, modify runStackedAction to accept an
optional modelSelection parameter and merge it so any modelSelection passed by a
test overrides the default (or remove the default entirely), update usages that
relied on the constant (e.g., places referencing DEFAULT_TEST_MODEL_SELECTION)
so tests explicitly pass the intended provider/model when needed, and ensure
test fixtures in GitManager.test.ts and the other affected block now assert
behavior with the actual modelSelection they intend to exercise.
In `@apps/server/src/git/Prompts.ts`:
- Around line 123-125: The attachment lines currently embed attachment.name and
attachment.mimeType directly in the prompt (see attachmentLines and
input.attachments), which can include control characters or newlines; add a
sanitizer that normalizes each string to a single-line safe representation
(e.g., replace all control characters and newlines with spaces, collapse
multiple whitespace to a single space, trim) and use it when building the list
(sanitize attachment.name and attachment.mimeType before interpolation);
optionally convert sizeBytes to a safe number/string format as well and
centralize this logic in a small helper like sanitizeSingleLine used by the map.
- Around line 148-150: The outputSchema currently allows any string for branch;
update the Schema.Struct definition for outputSchema to enforce a git-safe
branch format by either (1) replacing Schema.String for the branch field with a
refined/regex string that matches allowed git branch names, or (2) apply an
in-schema transform that runs the existing sanitizeFeatureBranchName() or
sanitizeBranchFragment() on the parsed value so branch is always normalized;
target the outputSchema symbol and ensure callers can rely on branch being
git-safe without additional sanitization.
In `@apps/server/src/main.test.ts`:
- Around line 158-164: openBootstrapFd currently opens a raw file descriptor via
FileSystem.FileSystem -> makeTempFileScoped -> fs.open and returns fd without
closing it, leaking FDs; change it to use Effect.acquireRelease (or an
equivalent acquire/release pattern) so the acquisition opens the fd and the
release calls fs.close(fd) (or fs.closeFile) to always close the descriptor
after runCli finishes, or alternatively ensure callers of openBootstrapFd call
fs.close(fd) in a finally/release step; reference openBootstrapFd,
FileSystem.FileSystem, makeTempFileScoped, fs.open, and fs.close when
implementing the acquire/release.
In `@apps/server/src/main.ts`:
- Around line 125-128: The bootstrapFd config currently accepts any integer (via
Config.int("T3CODE_BOOTSTRAP_FD").pipe(Config.option,
Config.map(Option.getOrUndefined)), which allows negatives and stdio fds);
change the config boundary to validate the FD (e.g., require non-negative or >=3
depending on allowed pipes) by refining the Config.int schema before turning it
into an option so invalid values are rejected early; update the same pattern
used elsewhere (the other bootstrapFd occurrence) so both use the validated
schema while keeping the Option.getOrUndefined mapping and the
T3CODE_BOOTSTRAP_FD symbol.
- Around line 107-112: The current CliEnvConfig mode mapping
(Config.string("T3CODE_MODE") -> Option.map((value) => (value === "desktop" ?
"desktop" : "web"))) incorrectly coerces any non-"desktop" string to "web";
change the pipeline for mode in Config.all so only the allowed literals are
accepted (e.g., use Option.filter or an explicit check that returns "desktop"
for "desktop", "web" for "web", and otherwise maps to Option.none/undefined)
instead of defaulting unknown values to "web" — update the
Config.string("T3CODE_MODE") -> pipe so invalid values are filtered out like the
bootstrap validation does.
In `@apps/web/src/components/chat/ProviderModelPicker.tsx`:
- Around line 9-12: The file ProviderModelPicker.tsx is failing the oxfmt check;
run the formatter (oxfmt) on
apps/web/src/components/chat/ProviderModelPicker.tsx to reformat the imports and
JSX (affecting the import block and the JSX regions around lines 255-287 and
343-429), then stage and commit the reformatted file so CI passes; ensure no
semantic changes to ProviderModelPicker, ProviderPickerKind, or any referenced
components like Button/buttonVariants while formatting.
In `@apps/web/src/components/ChatView.tsx`:
- Around line 131-135: Run the project's formatter on this file to satisfy CI:
run oxfmt (or the project's formatting command) against the ChatView component
and commit the changes so the import block and entire file conform to oxfmt
style; no code logic changes needed, just reformat the file that imports
getProviderStartOptions, useAppSettings, getCustomModelsByProvider, and
resolveAppModelSelection (and also reformat the other reported ranges) before
merging.
In `@apps/web/src/components/ui/toast.tsx`:
- Around line 45-49: The icon-only copy button lacks an explicit accessible
name; update the button element that calls copyToClipboard(text) in the toast
component to include an aria-label (e.g., aria-label="Copy error") alongside the
existing title so screen readers announce it reliably; ensure you add the
attribute on the same button element that has onClick={() =>
copyToClipboard(text)} in the toast UI component.
In `@REMOTE.md`:
- Around line 25-26: The phrase "command line arguments" in the REMOTE.md note
should be hyphenated; update the sentence that mentions `--bootstrap-fd <fd>` to
read "command-line arguments" (replace the unhyphenated phrase) so the note now
says the auth token can be delivered without putting it in process environment
or command-line arguments.
In `@scripts/dev-runner.ts`:
- Around line 156-174: The forwarded env object output (created from baseEnv in
scripts/dev-runner.ts) may unintentionally pass a parent T3CODE_BOOTSTRAP_FD
into the child; remove that descriptor from output after cloning by deleting the
T3CODE_BOOTSTRAP_FD key (e.g. add delete output.T3CODE_BOOTSTRAP_FD) so it never
survives into the turbo child regardless of isDesktopMode branch handling; apply
this near where output is constructed (alongside the existing deletes for
T3CODE_* in the else branch) to ensure the child process cannot inherit the FD.
---
Outside diff comments:
In `@apps/server/src/git/Layers/CodexTextGeneration.ts`:
- Around line 142-159: The code computes normalizedOptions via
normalizeCodexModelOptions but still reads reasoningEffort from
modelSelection.options; update the reasoningEffort assignment to use the
normalized value (e.g., normalizedOptions?.reasoningEffort ??
CODEX_GIT_TEXT_GENERATION_REASONING_EFFORT) so the ChildProcess.make call (the
`command` construction) emits the validated/normalized effort; keep the existing
fallback constant and leave the fastMode handling (normalizedOptions?.fastMode)
unchanged.
In `@apps/server/src/git/Layers/GitManager.ts`:
- Around line 1276-1289: The long single-line call to runPrStep inside the pr
assignment breaks the oxfmt layout; reflow the nested Effect.flatMap/Effect.gen
block so the runPrStep call and its arguments are split across multiple lines to
satisfy formatter rules. Locate the pr construction (the wantsPr ? yield*
progress.emit(...).pipe(Effect.flatMap(() => Effect.gen(function* () {
currentPhase = "pr"; return yield* runPrStep(input.modelSelection, input.cwd,
currentBranch, input.provider, input.model); }), ...)) and reformat the inner
generator to place currentPhase = "pr"; on its own line and the return yield*
runPrStep(...) with each argument on its own line (or otherwise wrapped) so
oxfmt --check passes. Ensure you only change formatting, preserving the use of
runPrStep, progress.emit, Effect.flatMap, and Effect.gen.
In `@apps/server/src/serverLayers.ts`:
- Around line 111-156: The global wiring of the Git router should be gated until
it can route by ProviderKind and honor modelSelection; stop registering the
router globally in GitManagerLive / gitManagerLayer (and any initialization that
exposes git.runStackedAction) and instead only wire it behind a feature-flag or
provider-aware facade. Update the code paths that currently special-case
claudeAgent and fallback to Codex so they instead validate ProviderKind +
modelSelection and either dispatch to a provider-specific handler or return a
clear unsupported-provider error; locate symbols GitManagerLive,
gitManagerLayer, and any usages of git.runStackedAction to implement the guard
and defer global registration until all ProviderKinds are supported. Ensure the
web client’s forwarded provider is used to select the correct handler rather
than defaulting to Codex.
---
Nitpick comments:
In `@apps/server/src/orchestration/Layers/ProviderCommandReactor.ts`:
- Around line 462-465: The modelSelection block currently hardcodes provider
"codex" and DEFAULT_GIT_TEXT_GENERATION_MODEL_BY_PROVIDER.codex; change it to
prefer the thread's chosen model/provider when available (use the thread or
context modelSelection object used elsewhere in ProviderCommandReactor or the
worktree branch generation code) and only fall back to
DEFAULT_GIT_TEXT_GENERATION_MODEL_BY_PROVIDER.codex if the thread selection is
missing or unsupported; update references to modelSelection so provider and
model are derived from the thread's selection first, then fallback to the codex
defaults.
In `@apps/web/src/components/chat/TraitsPicker.tsx`:
- Around line 121-140: The public props interface TraitsMenuContentProps
incorrectly includes trigger-only props (triggerVariant, triggerClassName) that
TraitsMenuContent does not consume and which leak into the rest/persistence
object; remove these two props from TraitsMenuContentProps and instead define a
separate TraitsPickerProps (or extend an existing picker prop type) that owns
triggerVariant and triggerClassName, update any call sites to pass trigger props
to the picker component rather than to TraitsMenuContent, and ensure
TraitsMenuContentImpl only receives the intended props (provider, model, prompt,
onPromptChange, modelOptions, allowPromptInjectedEffort and TraitsPersistence)
so the ...persistence rest no longer contains triggerVariant/triggerClassName.
In `@apps/web/src/components/ui/toast.tsx`:
- Around line 309-317: Extract the duplicated header+copy-button markup into a
small reusable component (e.g., ToastHeader or ToastHeaderWithCopy) that renders
the container div, Toast.Title and conditionally the CopyErrorButton when
toast.type === "error" and typeof toast.description === "string"; replace the
two occurrences (the block containing Toast.Title and CopyErrorButton in the
non-anchored and anchored branches) with this new component and pass the toast
object or the minimal props (title slot and description/type) to keep behavior
identical; ensure the new component uses the same classes ("flex items-center
justify-between gap-1" and "min-w-0 break-words font-medium") and exports from
the same file so imports remain straightforward.
In `@packages/contracts/src/git.ts`:
- Around line 80-85: The schema currently exposes two sources of truth—provider
and model plus modelSelection—which allows contradictory payloads; remove the
top-level provider and model fields from the exported schema and require callers
to use modelSelection only (update any callers like GitActionsControl.tsx to
stop sending provider/model), or if you need a transitional step, add a
validation in the schema (in the file where provider, model, modelSelection are
defined) that rejects payloads where provider/model are present alongside
modelSelection or enforces they match modelSelection; update the Schema
definitions for provider, model, and modelSelection accordingly and update
callers to emit only modelSelection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c70641d9-5f54-4ea5-8535-55c1af11060d
📒 Files selected for processing (44)
CLAUDE.mdREMOTE.mdapps/desktop/src/main.tsapps/desktop/src/preload.tsapps/server/src/bootstrap.test.tsapps/server/src/bootstrap.tsapps/server/src/git/Layers/ClaudeTextGeneration.test.tsapps/server/src/git/Layers/ClaudeTextGeneration.tsapps/server/src/git/Layers/CodexTextGeneration.test.tsapps/server/src/git/Layers/CodexTextGeneration.tsapps/server/src/git/Layers/GitManager.test.tsapps/server/src/git/Layers/GitManager.tsapps/server/src/git/Layers/RoutingTextGeneration.tsapps/server/src/git/Prompts.test.tsapps/server/src/git/Prompts.tsapps/server/src/git/Services/TextGeneration.tsapps/server/src/git/Utils.tsapps/server/src/git/isRepo.tsapps/server/src/main.test.tsapps/server/src/main.tsapps/server/src/orchestration/Layers/CheckpointReactor.tsapps/server/src/orchestration/Layers/ProviderCommandReactor.tsapps/server/src/orchestration/Layers/ProviderRuntimeIngestion.tsapps/server/src/serverLayers.tsapps/server/src/wsServer.test.tsapps/web/src/appSettings.test.tsapps/web/src/appSettings.tsapps/web/src/components/ChatView.tsxapps/web/src/components/GitActionsControl.tsxapps/web/src/components/chat/ProviderModelPicker.browser.tsxapps/web/src/components/chat/ProviderModelPicker.tsxapps/web/src/components/chat/TraitsPicker.browser.tsxapps/web/src/components/chat/TraitsPicker.tsxapps/web/src/components/ui/toast.tsxapps/web/src/composerDraftStore.tsapps/web/src/lib/gitReactQuery.test.tsapps/web/src/lib/gitReactQuery.tsapps/web/src/modelSelection.tsapps/web/src/wsNativeApi.test.tspackages/contracts/src/git.test.tspackages/contracts/src/git.tspackages/contracts/src/model.tsscripts/dev-runner.test.tsscripts/dev-runner.ts
💤 Files with no reviewable changes (2)
- apps/server/src/git/isRepo.ts
- apps/web/src/appSettings.ts
Comment on lines
+50
to
+95
| it.effect("returns none when the fd is unavailable", () => | ||
| Effect.gen(function* () { | ||
| const fd = NFS.openSync("/dev/null", "r"); | ||
| NFS.closeSync(fd); | ||
|
|
||
| const payload = yield* readBootstrapEnvelope(TestEnvelopeSchema, fd, { timeoutMs: 100 }); | ||
| assertNone(payload); | ||
| }), | ||
| ); | ||
|
|
||
| it.effect("returns none when the bootstrap read times out before any value arrives", () => | ||
| Effect.gen(function* () { | ||
| const fs = yield* FileSystem.FileSystem; | ||
| const tempDir = yield* fs.makeTempDirectoryScoped({ prefix: "t3-bootstrap-" }); | ||
| const fifoPath = path.join(tempDir, "bootstrap.pipe"); | ||
|
|
||
| yield* Effect.sync(() => execFileSync("mkfifo", [fifoPath])); | ||
|
|
||
| const _writer = yield* Effect.acquireRelease( | ||
| Effect.sync(() => | ||
| spawn("sh", ["-c", 'exec 3>"$1"; sleep 60', "sh", fifoPath], { | ||
| stdio: ["ignore", "ignore", "ignore"], | ||
| }), | ||
| ), | ||
| (writer) => | ||
| Effect.sync(() => { | ||
| writer.kill("SIGKILL"); | ||
| }), | ||
| ); | ||
|
|
||
| const fd = yield* Effect.acquireRelease( | ||
| Effect.sync(() => NFS.openSync(fifoPath, "r")), | ||
| (fd) => Effect.sync(() => NFS.closeSync(fd)), | ||
| ); | ||
|
|
||
| const fiber = yield* readBootstrapEnvelope(TestEnvelopeSchema, fd, { | ||
| timeoutMs: 100, | ||
| }).pipe(Effect.forkScoped); | ||
|
|
||
| yield* Effect.yieldNow; | ||
| yield* TestClock.adjust(Duration.millis(100)); | ||
|
|
||
| const payload = yield* Fiber.join(fiber); | ||
| assertNone(payload); | ||
| }).pipe(Effect.provide(TestClock.layer())), | ||
| ); |
There was a problem hiding this comment.
Guard these fd tests on Windows.
These cases use /dev/null, mkfifo, and sh, so they will fail on win32 even though Lines 20-22 already acknowledge that fd-path handling is platform-specific there. Please skip or branch these tests for non-POSIX platforms.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/bootstrap.test.ts` around lines 50 - 95, The two fd-related
tests in bootstrap.test.ts that call NFS.openSync("/dev/null"),
execFileSync("mkfifo"), spawn("sh", ...) and use
readBootstrapEnvelope/TestEnvelopeSchema should be skipped on Windows; add a
platform guard at the start of each test body (check process.platform ===
"win32") and return an immediate Effect (e.g. Effect.unit) when on Windows so
the test is effectively no-op there, leaving the rest of the POSIX logic
(mkfifo, spawn, TestClock.adjust, Fiber.join) unchanged.
Comment on lines
+21
to
+22
| const fdReady = yield* isFdReady(fd); | ||
| if (!fdReady) return Option.none(); |
There was a problem hiding this comment.
Fail closed when --bootstrap-fd is configured but unreadable.
Lines 21-22, 73-75, and 82 all collapse “bootstrap FD missing/closed/timed out” into Option.none(). In the new desktop flow, that makes “an explicit bootstrap request failed” indistinguishable from “no bootstrap requested”, so the child can silently fall back to web-mode defaults on a different port with no auth token instead of aborting startup.
Also applies to: 73-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/bootstrap.ts` around lines 21 - 22, When a bootstrap FD was
requested but unreadable the code currently returns Option.none(), making an
explicit bootstrap failure indistinguishable from “no bootstrap requested”;
change the behavior in the isFdReady(fd) check (the fdReady/Option.none()
branch) so that if a bootstrap FD was explicitly provided (detect via the
bootstrap FD flag / presence of fd) and isFdReady(fd) returns false you fail
closed (throw an Error or exit with non‑zero) and log the failure, instead of
returning Option.none(); apply the same change to the other branches that
collapse failures into Option.none() (the blocks referencing Option.none()
around the bootstrap FD handling).
Comment on lines
+19
to
+60
| function makeFakeClaudeBinary(dir: string) { | ||
| return Effect.gen(function* () { | ||
| const fs = yield* FileSystem.FileSystem; | ||
| const path = yield* Path.Path; | ||
| const binDir = path.join(dir, "bin"); | ||
| const claudePath = path.join(binDir, "claude"); | ||
| yield* fs.makeDirectory(binDir, { recursive: true }); | ||
|
|
||
| yield* fs.writeFileString( | ||
| claudePath, | ||
| [ | ||
| "#!/bin/sh", | ||
| 'args="$*"', | ||
| 'stdin_content="$(cat)"', | ||
| 'if [ -n "$T3_FAKE_CLAUDE_ARGS_MUST_CONTAIN" ]; then', | ||
| ' printf "%s" "$args" | grep -F -- "$T3_FAKE_CLAUDE_ARGS_MUST_CONTAIN" >/dev/null || {', | ||
| ' printf "%s\\n" "args missing expected content" >&2', | ||
| " exit 2", | ||
| " }", | ||
| "fi", | ||
| 'if [ -n "$T3_FAKE_CLAUDE_ARGS_MUST_NOT_CONTAIN" ]; then', | ||
| ' if printf "%s" "$args" | grep -F -- "$T3_FAKE_CLAUDE_ARGS_MUST_NOT_CONTAIN" >/dev/null; then', | ||
| ' printf "%s\\n" "args contained forbidden content" >&2', | ||
| " exit 3", | ||
| " fi", | ||
| "fi", | ||
| 'if [ -n "$T3_FAKE_CLAUDE_STDIN_MUST_CONTAIN" ]; then', | ||
| ' printf "%s" "$stdin_content" | grep -F -- "$T3_FAKE_CLAUDE_STDIN_MUST_CONTAIN" >/dev/null || {', | ||
| ' printf "%s\\n" "stdin missing expected content" >&2', | ||
| " exit 4", | ||
| " }", | ||
| "fi", | ||
| 'if [ -n "$T3_FAKE_CLAUDE_STDERR" ]; then', | ||
| ' printf "%s\\n" "$T3_FAKE_CLAUDE_STDERR" >&2', | ||
| "fi", | ||
| 'printf "%s" "$T3_FAKE_CLAUDE_OUTPUT"', | ||
| 'exit "${T3_FAKE_CLAUDE_EXIT_CODE:-0}"', | ||
| "", | ||
| ].join("\n"), | ||
| ); | ||
| yield* fs.chmod(claudePath, 0o755); | ||
| return binDir; |
There was a problem hiding this comment.
Make the fake Claude shim portable.
Lines 30-55 generate a POSIX shell script, Line 59 relies on chmod, and Line 89 hardcodes : as the PATH delimiter. That works on Unix, but it will not execute on Windows. Please switch this to a Node-based shim (or add a .cmd wrapper) and use the platform path delimiter.
Also applies to: 88-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/git/Layers/ClaudeTextGeneration.test.ts` around lines 19 -
60, The generated fake Claude shim in makeFakeClaudeBinary is currently a POSIX
shell script requiring chmod and using ':' as PATH delimiter which breaks on
Windows; replace it with a cross-platform Node-based shim (or create both a Unix
executable and a Windows .cmd wrapper) that reads env vars, stdin and writes
stdout/stderr/exit code accordingly, write the shim using the existing
FileSystem/FileSystem and Path/Path handles, only call fs.chmod for non-Windows
platforms, and compute the PATH delimiter via path.delimiter (or Node's
Path.path.delimiter) instead of hardcoding ':' so the tests run on Windows and
Unix.
Comment on lines
+89
to
+103
| const runClaudeCommand = Effect.gen(function* () { | ||
| const command = ChildProcess.make( | ||
| "claude", | ||
| [ | ||
| "-p", | ||
| "--output-format", | ||
| "json", | ||
| "--json-schema", | ||
| jsonSchemaStr, | ||
| "--model", | ||
| modelSelection.model, | ||
| ...(normalizedOptions?.effort ? ["--effort", normalizedOptions.effort] : []), | ||
| ...(Object.keys(settings).length > 0 ? ["--settings", JSON.stringify(settings)] : []), | ||
| "--dangerously-skip-permissions", | ||
| ], |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Official Claude CLI documentation for: claude -p, --output-format json, --json-schema, --settings, --effort, and --dangerously-skip-permissions. Does --json-schema accept inline JSON text, and what least-privilege non-interactive mode is recommended for read-only automation?
💡 Result:
Official Claude Code CLI (from Anthropic) documentation for the specified flags, based on code.claude.com/docs and docs.anthropic.com/en/docs/claude-code: - claude -p (or --print): Runs a single non-interactive query via the Agent SDK and exits (print/headless mode). Ideal for scripting, CI/CD, and automation. Example: claude -p "explain this function". All other CLI flags work with -p. Recommended with --bare for faster startup in scripts by skipping auto-discovery. - --output-format json: Specifies JSON output format for print mode (options: text (default), json, stream-json). Returns structured JSON with result, session ID, and metadata. Example: claude -p "query" --output-format json. - --json-schema: Validates JSON output against a provided JSON Schema in print mode (requires --output-format json). Accepts inline JSON schema text as a CLI argument. Example: claude -p --json-schema '{"type":"object","properties":{"functions":{"type":"array","items":{"type":"string"}}},"required":["functions"]}' "Extract function names". The agent completes its workflow, then returns validated JSON matching the schema. - --settings: Path to a settings JSON file to override configuration for the session. Example: claude --settings ./my-settings.json. - --effort: Sets session effort level (low, medium, high, max for Opus 4.6 only). Session-scoped, does not persist. Example: claude --effort high. - --dangerously-skip-permissions: Skips all permission prompts, allowing unrestricted tool use (file edits, Bash commands, etc.). Use with extreme caution; intended for sandboxed environments like Docker/CI. Does not bypass all checks (e.g., protected directories). Related: --allow-dangerously-skip-permissions enables it as an option for composing with --permission-mode. For read-only automation (least-privilege non-interactive mode): Use claude -p (or -p --bare for speed) combined with --allowedTools "Read" (or "Read,Grep") to auto-approve only read tools, and --disallowedTools "Edit,Bash,Write" (etc.) to block mutations. Limits Claude to analysis without file changes or command execution. Add --output-format json for parseable output. Example: claude -p --allowedTools "Read" --disallowedTools "Edit,Bash" --output-format json "Analyze the codebase for security issues". Use --max-turns N and --max-budget-usd X for cost/safety limits. --bare recommended for reproducible scripted runs.
Citations:
- 1: https://docs.anthropic.com/en/docs/claude-code/cli-usage
- 2: https://code.claude.com/docs/en/cli-reference
- 3: https://code.claude.com/docs/en/headless.md
- 4: https://code.claude.com/docs/en/permissions
Fix two CLI contract violations: --settings format and overly broad permissions.
Line 96: --settings expects a file path, not inline JSON. The current code passes JSON.stringify(settings) which will fail at runtime. Either write settings to a temp file first or remove this flag if not actually needed.
Line 99: --dangerously-skip-permissions grants unrestricted tool access (file edits, Bash commands, etc.) and is not appropriate for read-only git text generation. Per the official Claude CLI docs, use --allowedTools "Read" and --disallowedTools "Edit,Bash,Write" instead to enforce least-privilege access.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/git/Layers/ClaudeTextGeneration.ts` around lines 89 - 103,
The runClaudeCommand call constructs the Claude CLI args incorrectly:
ChildProcess.make currently passes JSON.stringify(settings) to the --settings
flag (which expects a file path) and uses --dangerously-skip-permissions
(too-broad). Fix by persisting settings to a temp file and passing that filepath
to --settings (or drop the flag if settings is empty/not needed) and replace
--dangerously-skip-permissions with explicit tool restrictions e.g. add
--allowedTools "Read" and --disallowedTools "Edit,Bash,Write" when building the
args in runClaudeCommand/ChildProcess.make so the CLI receives a settings
filepath and least-privilege permissions.
- RoutingTextGeneration: explicitly document fallback behavior for providers without dedicated CLI text-gen layers, add supported provider set for clarity - preload.ts: normalize empty backendWsUrl string to null during pre-bootstrap window - toast.tsx: add aria-label to icon-only copy button for accessibility - REMOTE.md: hyphenate "command-line arguments" - dev-runner.ts: strip T3CODE_BOOTSTRAP_FD from forwarded env to prevent child processes reading stale/closed descriptors - Run oxfmt on ProviderModelPicker.tsx and ChatView.tsx
There was a problem hiding this comment.
♻️ Duplicate comments (1)
scripts/dev-runner.ts (1)
156-175:⚠️ Potential issue | 🟠 MajorStrip
T3CODE_BOOTSTRAP_FDbefore branching on mode.
outputis cloned frombaseEnvat Line 156, sodev,dev:web, anddev:serverstill inherit any ambientT3CODE_BOOTSTRAP_FD. That leaves turbo descendants able to read a stale or unrelated descriptor; this cleanup needs to be unconditional, not desktop-only.Suggested fix
const output: NodeJS.ProcessEnv = { ...baseEnv, PORT: String(webPort), ELECTRON_RENDERER_PORT: String(webPort), VITE_DEV_SERVER_URL: devUrl?.toString() ?? `http://localhost:${webPort}`, T3CODE_HOME: resolvedBaseDir, }; + delete output.T3CODE_BOOTSTRAP_FD; if (!isDesktopMode) { output.T3CODE_PORT = String(serverPort); output.VITE_WS_URL = `ws://localhost:${serverPort}`; } else { delete output.T3CODE_PORT; delete output.VITE_WS_URL; delete output.T3CODE_AUTH_TOKEN; delete output.T3CODE_MODE; delete output.T3CODE_NO_BROWSER; delete output.T3CODE_HOST; - delete output.T3CODE_BOOTSTRAP_FD; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/dev-runner.ts` around lines 156 - 175, The cloned environment object output still inherits T3CODE_BOOTSTRAP_FD from baseEnv, so remove any T3CODE_BOOTSTRAP_FD entry unconditionally immediately after creating output (before the isDesktopMode branch) to prevent turbo descendants reading a stale descriptor; update the code near the output creation (where output is built from baseEnv and webPort) to delete output.T3CODE_BOOTSTRAP_FD right after that assignment, and keep the existing deletions inside the isDesktopMode branch for other desktop-only keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@scripts/dev-runner.ts`:
- Around line 156-175: The cloned environment object output still inherits
T3CODE_BOOTSTRAP_FD from baseEnv, so remove any T3CODE_BOOTSTRAP_FD entry
unconditionally immediately after creating output (before the isDesktopMode
branch) to prevent turbo descendants reading a stale descriptor; update the code
near the output creation (where output is built from baseEnv and webPort) to
delete output.T3CODE_BOOTSTRAP_FD right after that assignment, and keep the
existing deletions inside the isDesktopMode branch for other desktop-only keys.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 44a04ff7-ac08-46a7-9b6d-9b529de99db8
📒 Files selected for processing (7)
REMOTE.mdapps/desktop/src/preload.tsapps/server/src/git/Layers/RoutingTextGeneration.tsapps/web/src/components/ChatView.tsxapps/web/src/components/chat/ProviderModelPicker.tsxapps/web/src/components/ui/toast.tsxscripts/dev-runner.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/desktop/src/preload.ts
- apps/web/src/components/ui/toast.tsx
- apps/server/src/git/Layers/RoutingTextGeneration.ts
- REMOTE.md
- apps/web/src/components/ChatView.tsx
Move the delete above the mode branch so it applies to all dev modes, not just desktop. Prevents turbo children from inheriting a stale fd.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/dev-runner.ts (1)
208-220: Consider consolidatingT3CODE_DESKTOP_WS_URLdeletion.
T3CODE_DESKTOP_WS_URLis deleted across all four modes (dev, dev:server, dev:web, dev:desktop) in three separate conditionals. Similar to theT3CODE_BOOTSTRAP_FDfix, this could be consolidated into a single unconditional deletion for clarity.♻️ Suggested consolidation
// Always strip bootstrap fd — it refers to the parent's descriptor and // must never leak into turbo/child processes regardless of mode. delete output.T3CODE_BOOTSTRAP_FD; + // Desktop WS URL is always cleared — desktop computes it dynamically. + delete output.T3CODE_DESKTOP_WS_URL; if (!isDesktopMode) {Then remove the individual
delete output.T3CODE_DESKTOP_WS_URL;statements from lines 210, 215, and 219.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/dev-runner.ts` around lines 208 - 220, The deletion of output.T3CODE_DESKTOP_WS_URL is repeated in multiple conditionals; consolidate by adding a single unconditional delete output.T3CODE_DESKTOP_WS_URL after the mode/isDesktopMode logic and remove the repeated delete statements inside the blocks that set output.T3CODE_MODE (the occurrences referencing output.T3CODE_DESKTOP_WS_URL in the branches checking mode === "dev", mode === "dev:server" || mode === "dev:web", and isDesktopMode). Ensure the existing logic that sets output.T3CODE_MODE (e.g., the blocks using mode and isDesktopMode) remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/dev-runner.ts`:
- Around line 208-220: The deletion of output.T3CODE_DESKTOP_WS_URL is repeated
in multiple conditionals; consolidate by adding a single unconditional delete
output.T3CODE_DESKTOP_WS_URL after the mode/isDesktopMode logic and remove the
repeated delete statements inside the blocks that set output.T3CODE_MODE (the
occurrences referencing output.T3CODE_DESKTOP_WS_URL in the branches checking
mode === "dev", mode === "dev:server" || mode === "dev:web", and isDesktopMode).
Ensure the existing logic that sets output.T3CODE_MODE (e.g., the blocks using
mode and isDesktopMode) remains unchanged.
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pingdotgg/t3codemain into the forkUpstream commits integrated
sideOffset={4}to fork's multi-provider picker + added upstream testmodelSelection.tsre-exports fromappSettings.ts; widenedRoutingTextGenerationrouting to accept allProviderKindvaluesTest plan
bun typecheckpasses (0 errors across all 7 packages)bun lintpasses (0 errors)claudeCLISummary by CodeRabbit
New Features
UI
Documentation
Tests