Upstream sync batch 3: flatten git layer, resizable sidebar, PTY runtime loading#31
Merged
aaditagrawal merged 8 commits intomainfrom Mar 24, 2026
Merged
Upstream sync batch 3: flatten git layer, resizable sidebar, PTY runtime loading#31aaditagrawal merged 8 commits intomainfrom
aaditagrawal merged 8 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughThis PR refactors Git command execution by moving it from Changes
Sequence DiagramssequenceDiagram
participant Client
participant GitCore as GitCore Layer
participant CPS as ChildProcessSpawner
participant Git as Git Process
Client->>GitCore: execute(ExecuteGitInput)
activate GitCore
GitCore->>GitCore: Apply timeout wrapper<br/>(Effect.timeoutOption)
GitCore->>CPS: spawn git process
activate CPS
CPS->>Git: spawn child process
activate Git
Git-->>CPS: stdout/stderr streams
CPS->>CPS: Stream & decode output<br/>(bounded by maxOutputBytes)
Git-->>CPS: exit code
deactivate Git
CPS-->>GitCore: ExecuteGitResult<br/>(code, stdout, stderr)
deactivate CPS
GitCore->>GitCore: Map spawn/exit errors<br/>to GitCommandError
GitCore-->>Client: Effect<ExecuteGitResult,<br/>GitCommandError>
deactivate GitCore
sequenceDiagram
participant App as Application Startup
participant ServerLayers as serverLayers Module
participant Checkpoint as CheckpointStore Layer
participant GitCore as GitCore Layer
participant Orchestration as Orchestration Runtime
App->>ServerLayers: initialize layers
activate ServerLayers
ServerLayers->>GitCore: create GitCoreLive
activate GitCore
GitCore-->>ServerLayers: GitCore layer ready
deactivate GitCore
ServerLayers->>Checkpoint: create CheckpointStoreLive
activate Checkpoint
Checkpoint->>Checkpoint: pipe Layer.provide(GitCoreLive)
Checkpoint-->>ServerLayers: composed checkpoint layer
deactivate Checkpoint
ServerLayers->>Orchestration: wire all layers<br/>(checkpoint + git + runtime)
activate Orchestration
Orchestration-->>ServerLayers: orchestration ready
deactivate Orchestration
ServerLayers-->>App: all layers initialized
deactivate ServerLayers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
apps/web/src/composerDraftStore.ts (1)
868-913: Well-structured extraction with correct flush-before-read pattern.The extraction of
verifyPersistedAttachmentsimproves readability and correctly addresses the false persistence warning issue by flushing debounced writes before reading. The defensive error handling with fallback to an empty Set is appropriate.Minor observation: the initialization
let persistedIdSet = new Set<string>()on line 881 is technically redundant since it's always reassigned in either thetryorcatchblock. Consider simplifying:♻️ Optional simplification
- let persistedIdSet = new Set<string>(); - try { - composerDebouncedStorage.flush(); - persistedIdSet = new Set(readPersistedAttachmentIdsFromStorage(threadId)); - } catch { - persistedIdSet = new Set(); - } + composerDebouncedStorage.flush(); + const persistedIdSet = new Set(readPersistedAttachmentIdsFromStorage(threadId));Note:
readPersistedAttachmentIdsFromStoragealready has internal try-catch that returns[]on failure (lines 852-865), so the outer try-catch only guards against potentialflush()errors. If you want to keep the outer catch for defense againstflush()failures, consider wrapping only that call.🤖 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 868 - 913, The local variable persistedIdSet in verifyPersistedAttachments is redundantly initialized and the outer try-catch currently spans both composerDebouncedStorage.flush() and readPersistedAttachmentIdsFromStorage(threadId); simplify by removing the initial new Set<string>() and only wrap composerDebouncedStorage.flush() in a try-catch (calling readPersistedAttachmentIdsFromStorage(threadId) unguarded afterward since it already handles errors), then assign persistedIdSet = new Set(readPersistedAttachmentIdsFromStorage(threadId)) so the logic is clearer and the outer catch only protects the flush() call.apps/server/src/git/Layers/GitManager.test.ts (1)
539-542: Share the test server config wiring.
GitManagerTestLayernow builds oneGitCoreLive, whilemakeManager()builds another with the sameServerConfig.layerTest(...). Pulling that common test setup into one helper would keep the repo helpers and the SUT on identical config and reduce drift.🤖 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 539 - 542, The test constructs duplicate ServerConfig layers: GitManagerTestLayer builds a GitCoreLive with ServerConfig.layerTest(...) while makeManager() creates another with the same config causing config drift; extract the shared ServerConfig.layerTest(process.cwd(), { prefix: "t3-git-manager-test-" }) into a single helper Layer (e.g., TestServerConfigLayer) and have both GitManagerTestLayer and makeManager() consume that same helper (using Layer.provide / Layer.provideMerge) so they use the identical ServerConfig instance; update references to GitCoreLive, GitManagerTestLayer and makeManager() to use the new TestServerConfigLayer.apps/server/src/serverLayers.ts (1)
44-59: Add a smoke test for the runtime PTY selector.This is now the single point that binds the
./terminal/Layers/*PTYimport paths to the renamedlayerexport, so a small test here would catch a broken import/export rename before it turns into a startup-only failure.🤖 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 44 - 59, Add a small smoke test that imports makeRuntimePtyAdapterLayer (or calls the loader via runtimePtyAdapterLoaders) and asserts it successfully resolves the expected layer shape: create a test that stubs process.versions.bun to both truthy and undefined, invokes the appropriate loader from runtimePtyAdapterLoaders (or calls makeRuntimePtyAdapterLayer inside an async test), awaits the Promise, and verifies the returned object matches the RuntimePtyAdapterLoader shape (has a .layer export that is an Effect Layer/has expected methods) to catch any import/export renames of the ./terminal/Layers/*PTY modules.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/pr-size.yml:
- Around line 182-186: The truncation check (files.length >= 3000) currently
only logs a warning but still allows the workflow to compute and apply a size
label from a partial list; update the logic that runs after pulls.listFiles to
treat truncation as a special case: when files.length >= 3000 either set the
computed size label to "size:XXL" or skip changing the existing size label
entirely instead of applying the normal label flow, and ensure this change is
applied where the code computes/applies labels (the block that checks
files.length and the subsequent label assignment/patching logic).
In `@apps/server/src/git/Layers/GitCore.ts`:
- Around line 526-569: The cache is treating non-zero-exit fetches as successes
because fetchUpstreamRefForStatus passes allowNonZeroExit: true to executeGit,
so failures get cached by statusUpstreamRefreshCache; remove/disable
allowNonZeroExit in fetchUpstreamRefForStatus (i.e., call executeGit without
allowNonZeroExit or set it to false) so executeGit will fail on non-zero exits,
let the Effect fail (preserving the GitCommandError), and thus the cache's
Exit.isSuccess logic will not keep failed refreshes warm; keep the timeout
option as-is.
- Around line 944-958: The current prepareCommitContext treats an explicit empty
array (filePaths = []) as falsy and falls into the "add all" branch; change the
branching so that only undefined/null filePaths triggers "git add -A" (stage
everything), while an explicit empty array means "no files selected" and should
do nothing (no add). Update prepareCommitContext to first check whether
filePaths is null/undefined (e.g., filePaths == null) and only call
runGit("GitCore.prepareCommitContext.addAll", ...) in that case; if filePaths is
an array then if length>0 run the reset + add selected via
runGit("GitCore.prepareCommitContext.reset", ...) and
runGit("GitCore.prepareCommitContext.addSelected", ...), and if length===0 skip
adding entirely. Ensure you update the condition around runGit invocations
accordingly.
- Around line 630-651: The fallback remote selection is arbitrary because
listRemoteNames reverses order and resolvePrimaryRemoteName picks the first
non-origin; change behavior so we pick a meaningful default: stop reordering
remotes in listRemoteNames (remove the toReversed()) so order from git is
preserved, and update resolvePrimaryRemoteName to prefer, in order, (1)
"origin", (2) a configured branch remote from git config (use runGitStdout to
read "git config --get branch.<currentBranch>.remote" or check
"remote.pushDefault"), (3) a remote named "upstream", and only then (4) fall
back to the first entry from listRemoteNames; keep existing error path via
createGitCommandError and reuse originRemoteExists, runGitStdout,
parseRemoteNames to implement these checks.
---
Nitpick comments:
In `@apps/server/src/git/Layers/GitManager.test.ts`:
- Around line 539-542: The test constructs duplicate ServerConfig layers:
GitManagerTestLayer builds a GitCoreLive with ServerConfig.layerTest(...) while
makeManager() creates another with the same config causing config drift; extract
the shared ServerConfig.layerTest(process.cwd(), { prefix:
"t3-git-manager-test-" }) into a single helper Layer (e.g.,
TestServerConfigLayer) and have both GitManagerTestLayer and makeManager()
consume that same helper (using Layer.provide / Layer.provideMerge) so they use
the identical ServerConfig instance; update references to GitCoreLive,
GitManagerTestLayer and makeManager() to use the new TestServerConfigLayer.
In `@apps/server/src/serverLayers.ts`:
- Around line 44-59: Add a small smoke test that imports
makeRuntimePtyAdapterLayer (or calls the loader via runtimePtyAdapterLoaders)
and asserts it successfully resolves the expected layer shape: create a test
that stubs process.versions.bun to both truthy and undefined, invokes the
appropriate loader from runtimePtyAdapterLoaders (or calls
makeRuntimePtyAdapterLayer inside an async test), awaits the Promise, and
verifies the returned object matches the RuntimePtyAdapterLoader shape (has a
.layer export that is an Effect Layer/has expected methods) to catch any
import/export renames of the ./terminal/Layers/*PTY modules.
In `@apps/web/src/composerDraftStore.ts`:
- Around line 868-913: The local variable persistedIdSet in
verifyPersistedAttachments is redundantly initialized and the outer try-catch
currently spans both composerDebouncedStorage.flush() and
readPersistedAttachmentIdsFromStorage(threadId); simplify by removing the
initial new Set<string>() and only wrap composerDebouncedStorage.flush() in a
try-catch (calling readPersistedAttachmentIdsFromStorage(threadId) unguarded
afterward since it already handles errors), then assign persistedIdSet = new
Set(readPersistedAttachmentIdsFromStorage(threadId)) so the logic is clearer and
the outer catch only protects the flush() call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e3f43b6-b869-42db-8eed-110dc11346c4
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
.github/workflows/pr-size.ymlapps/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/GitService.test.tsapps/server/src/git/Layers/GitService.tsapps/server/src/git/Services/GitCore.tsapps/server/src/git/Services/GitService.tsapps/server/src/orchestration/Layers/CheckpointReactor.test.tsapps/server/src/provider/Layers/ClaudeAdapter.test.tsapps/server/src/provider/Layers/ClaudeAdapter.tsapps/server/src/provider/Layers/ClaudeSdkFastMode.probe.test.tsapps/server/src/provider/claude-agent-sdk.d.tsapps/server/src/serverLayers.tsapps/server/src/terminal/Layers/BunPTY.tsapps/server/src/terminal/Layers/NodePTY.tsapps/web/package.jsonapps/web/src/composerDraftStore.tsapps/web/src/routes/_chat.tsxscripts/claude-fast-mode-probe.tsscripts/claude-haiku-thinking-probe.ts
💤 Files with no reviewable changes (6)
- apps/server/src/provider/Layers/ClaudeSdkFastMode.probe.test.ts
- apps/server/src/git/Layers/GitService.test.ts
- scripts/claude-fast-mode-probe.ts
- scripts/claude-haiku-thinking-probe.ts
- apps/server/src/git/Layers/GitService.ts
- apps/server/src/git/Services/GitService.ts
Comment on lines
+182
to
+186
| if (files.length >= 3000) { | ||
| core.warning( | ||
| "The GitHub pull request files API may truncate results at 3,000 files; PR size may be undercounted.", | ||
| ); | ||
| } |
There was a problem hiding this comment.
Don't relabel from a truncated file list.
If pulls.listFiles hits GitHub's 3,000-file cap, the workflow still classifies the PR from a partial total. That can attach a smaller size label than the PR actually deserves. Once truncation is detected, this should either force size:XXL or leave the existing size label untouched.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/pr-size.yml around lines 182 - 186, The truncation check
(files.length >= 3000) currently only logs a warning but still allows the
workflow to compute and apply a size label from a partial list; update the logic
that runs after pulls.listFiles to treat truncation as a special case: when
files.length >= 3000 either set the computed size label to "size:XXL" or skip
changing the existing size label entirely instead of applying the normal label
flow, and ensure this change is applied where the code computes/applies labels
(the block that checks files.length and the subsequent label assignment/patching
logic).
Comment on lines
+526
to
569
| const fetchUpstreamRef = ( | ||
| cwd: string, | ||
| upstream: { upstreamRef: string; remoteName: string; upstreamBranch: string }, | ||
| ): Effect.Effect<void, GitCommandError> => { | ||
| const refspec = `+refs/heads/${upstream.upstreamBranch}:refs/remotes/${upstream.upstreamRef}`; | ||
| return runGit( | ||
| "GitCore.fetchUpstreamRef", | ||
| cwd, | ||
| ["config", "--get", `branch.${branch}.gh-merge-base`], | ||
| ["fetch", "--quiet", "--no-tags", upstream.remoteName, refspec], | ||
| true, | ||
| ).pipe(Effect.map((stdout) => stdout.trim())); | ||
|
|
||
| const primaryRemoteName = yield* resolvePrimaryRemoteName(cwd).pipe( | ||
| Effect.catch(() => Effect.succeed(null)), | ||
| ); | ||
| const defaultBranch = | ||
| primaryRemoteName === null ? null : yield* resolveDefaultBranchName(cwd, primaryRemoteName); | ||
| const candidates = [ | ||
| configuredBaseBranch.length > 0 ? configuredBaseBranch : null, | ||
| defaultBranch, | ||
| ...DEFAULT_BASE_BRANCH_CANDIDATES, | ||
| ]; | ||
|
|
||
| for (const candidate of candidates) { | ||
| if (!candidate) { | ||
| continue; | ||
| } | ||
|
|
||
| const remotePrefix = | ||
| primaryRemoteName && primaryRemoteName !== "origin" ? `${primaryRemoteName}/` : null; | ||
| const normalizedCandidate = candidate.startsWith("origin/") | ||
| ? candidate.slice("origin/".length) | ||
| : remotePrefix && candidate.startsWith(remotePrefix) | ||
| ? candidate.slice(remotePrefix.length) | ||
| : candidate; | ||
| if (normalizedCandidate.length === 0 || normalizedCandidate === branch) { | ||
| continue; | ||
| } | ||
|
|
||
| if (yield* branchExists(cwd, normalizedCandidate)) { | ||
| return normalizedCandidate; | ||
| } | ||
| }; | ||
|
|
||
| if ( | ||
| primaryRemoteName && | ||
| (yield* remoteBranchExists(cwd, primaryRemoteName, normalizedCandidate)) | ||
| ) { | ||
| return `${primaryRemoteName}/${normalizedCandidate}`; | ||
| } | ||
| } | ||
| 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); | ||
| }; | ||
|
|
||
| return null; | ||
| const statusUpstreamRefreshCache = yield* Cache.makeWith({ | ||
| capacity: STATUS_UPSTREAM_REFRESH_CACHE_CAPACITY, | ||
| lookup: (cacheKey: StatusUpstreamRefreshCacheKey) => | ||
| Effect.gen(function* () { | ||
| yield* fetchUpstreamRefForStatus(cacheKey.cwd, { | ||
| upstreamRef: cacheKey.upstreamRef, | ||
| remoteName: cacheKey.remoteName, | ||
| upstreamBranch: cacheKey.upstreamBranch, | ||
| }); | ||
| return true as const; | ||
| }), | ||
| // Keep successful refreshes warm; drop failures immediately so next request can retry. | ||
| timeToLive: (exit) => | ||
| Exit.isSuccess(exit) ? STATUS_UPSTREAM_REFRESH_INTERVAL : Duration.zero, | ||
| }); |
There was a problem hiding this comment.
Let upstream refresh fetches fail on non-zero exit.
These helpers opt into allowNonZeroExit, so auth/network/ref errors are treated as successful refreshes. fetchUpstreamRefForStatus() then keeps that failed refresh warm in the cache for 15 seconds, and the checkout background refresh never logs a real fetch failure. That leaves statusDetails() and pushCurrentBranch() running against stale upstream state.
🔧 Suggested fix
const fetchUpstreamRef = (
cwd: string,
upstream: { upstreamRef: string; remoteName: string; upstreamBranch: string },
): Effect.Effect<void, GitCommandError> => {
const refspec = `+refs/heads/${upstream.upstreamBranch}:refs/remotes/${upstream.upstreamRef}`;
return runGit(
"GitCore.fetchUpstreamRef",
cwd,
["fetch", "--quiet", "--no-tags", upstream.remoteName, refspec],
- true,
);
};
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);
};🤖 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 526 - 569, The cache is
treating non-zero-exit fetches as successes because fetchUpstreamRefForStatus
passes allowNonZeroExit: true to executeGit, so failures get cached by
statusUpstreamRefreshCache; remove/disable allowNonZeroExit in
fetchUpstreamRefForStatus (i.e., call executeGit without allowNonZeroExit or set
it to false) so executeGit will fail on non-zero exits, let the Effect fail
(preserving the GitCommandError), and thus the cache's Exit.isSuccess logic will
not keep failed refreshes warm; keep the timeout option as-is.
Comment on lines
+630
to
+651
| const listRemoteNames = (cwd: string): Effect.Effect<ReadonlyArray<string>, GitCommandError> => | ||
| runGitStdout("GitCore.listRemoteNames", cwd, ["remote"]).pipe( | ||
| Effect.map((stdout) => parseRemoteNames(stdout).toReversed()), | ||
| ); | ||
|
|
||
| const branchLastCommit = new Map<string, number>(); | ||
| if (branchRecency.code !== 0) { | ||
| return branchLastCommit; | ||
| } | ||
| const resolvePrimaryRemoteName = (cwd: string): Effect.Effect<string, GitCommandError> => | ||
| Effect.gen(function* () { | ||
| if (yield* originRemoteExists(cwd)) { | ||
| return "origin"; | ||
| } | ||
| const remotes = yield* listRemoteNames(cwd); | ||
| const [firstRemote] = remotes; | ||
| if (firstRemote) { | ||
| return firstRemote; | ||
| } | ||
| return yield* createGitCommandError( | ||
| "GitCore.resolvePrimaryRemoteName", | ||
| cwd, | ||
| ["remote"], | ||
| "No git remote is configured for this repository.", | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Fallback remote selection is currently arbitrary.
listRemoteNames() rewrites the remote order and resolvePrimaryRemoteName() blindly takes the first entry when origin is missing. In a repo with multiple non-origin remotes, publish/default-branch/PR-fetch paths can target whichever remote happens to sort shortest, not the configured default.
🔧 Suggested fix
const listRemoteNames = (cwd: string): Effect.Effect<ReadonlyArray<string>, GitCommandError> =>
runGitStdout("GitCore.listRemoteNames", cwd, ["remote"]).pipe(
- Effect.map((stdout) => parseRemoteNames(stdout).toReversed()),
+ Effect.map((stdout) =>
+ stdout
+ .split("\n")
+ .map((line) => line.trim())
+ .filter((line) => line.length > 0),
+ ),
);
const resolvePrimaryRemoteName = (cwd: string): Effect.Effect<string, GitCommandError> =>
Effect.gen(function* () {
if (yield* originRemoteExists(cwd)) {
return "origin";
}
const remotes = yield* listRemoteNames(cwd);
- const [firstRemote] = remotes;
- if (firstRemote) {
- return firstRemote;
+ if (remotes.length === 1) {
+ return remotes[0]!;
+ }
+ if (remotes.length > 1) {
+ return yield* createGitCommandError(
+ "GitCore.resolvePrimaryRemoteName",
+ cwd,
+ ["remote"],
+ "Multiple remotes are configured. Set remote.pushDefault or branch.<name>.pushRemote.",
+ );
}
return yield* createGitCommandError(
"GitCore.resolvePrimaryRemoteName",
cwd,
["remote"],📝 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 listRemoteNames = (cwd: string): Effect.Effect<ReadonlyArray<string>, GitCommandError> => | |
| runGitStdout("GitCore.listRemoteNames", cwd, ["remote"]).pipe( | |
| Effect.map((stdout) => parseRemoteNames(stdout).toReversed()), | |
| ); | |
| const branchLastCommit = new Map<string, number>(); | |
| if (branchRecency.code !== 0) { | |
| return branchLastCommit; | |
| } | |
| const resolvePrimaryRemoteName = (cwd: string): Effect.Effect<string, GitCommandError> => | |
| Effect.gen(function* () { | |
| if (yield* originRemoteExists(cwd)) { | |
| return "origin"; | |
| } | |
| const remotes = yield* listRemoteNames(cwd); | |
| const [firstRemote] = remotes; | |
| if (firstRemote) { | |
| return firstRemote; | |
| } | |
| return yield* createGitCommandError( | |
| "GitCore.resolvePrimaryRemoteName", | |
| cwd, | |
| ["remote"], | |
| "No git remote is configured for this repository.", | |
| ); | |
| }); | |
| const listRemoteNames = (cwd: string): Effect.Effect<ReadonlyArray<string>, GitCommandError> => | |
| runGitStdout("GitCore.listRemoteNames", cwd, ["remote"]).pipe( | |
| Effect.map((stdout) => | |
| stdout | |
| .split("\n") | |
| .map((line) => line.trim()) | |
| .filter((line) => line.length > 0), | |
| ), | |
| ); | |
| const resolvePrimaryRemoteName = (cwd: string): Effect.Effect<string, GitCommandError> => | |
| Effect.gen(function* () { | |
| if (yield* originRemoteExists(cwd)) { | |
| return "origin"; | |
| } | |
| const remotes = yield* listRemoteNames(cwd); | |
| if (remotes.length === 1) { | |
| return remotes[0]!; | |
| } | |
| if (remotes.length > 1) { | |
| return yield* createGitCommandError( | |
| "GitCore.resolvePrimaryRemoteName", | |
| cwd, | |
| ["remote"], | |
| "Multiple remotes are configured. Set remote.pushDefault or branch.<name>.pushRemote.", | |
| ); | |
| } | |
| return yield* createGitCommandError( | |
| "GitCore.resolvePrimaryRemoteName", | |
| cwd, | |
| ["remote"], | |
| "No git remote is configured for this repository.", | |
| ); | |
| }); |
🤖 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 630 - 651, The fallback
remote selection is arbitrary because listRemoteNames reverses order and
resolvePrimaryRemoteName picks the first non-origin; change behavior so we pick
a meaningful default: stop reordering remotes in listRemoteNames (remove the
toReversed()) so order from git is preserved, and update
resolvePrimaryRemoteName to prefer, in order, (1) "origin", (2) a configured
branch remote from git config (use runGitStdout to read "git config --get
branch.<currentBranch>.remote" or check "remote.pushDefault"), (3) a remote
named "upstream", and only then (4) fall back to the first entry from
listRemoteNames; keep existing error path via createGitCommandError and reuse
originRemoteExists, runGitStdout, parseRemoteNames to implement these checks.
Comment on lines
+944
to
+958
| const prepareCommitContext: GitCoreShape["prepareCommitContext"] = (cwd, filePaths) => | ||
| Effect.gen(function* () { | ||
| if (filePaths && filePaths.length > 0) { | ||
| yield* runGit("GitCore.prepareCommitContext.reset", cwd, ["reset"]).pipe( | ||
| Effect.catch(() => Effect.void), | ||
| ); | ||
| yield* runGit("GitCore.prepareCommitContext.addSelected", cwd, [ | ||
| "add", | ||
| "-A", | ||
| "--", | ||
| ...filePaths, | ||
| ]); | ||
| } else { | ||
| yield* runGit("GitCore.prepareCommitContext.addAll", cwd, ["add", "-A"]); | ||
| } |
There was a problem hiding this comment.
Handle [] as “no files selected”, not “stage everything”.
When filePaths is an explicit empty array, this falls through to git add -A and stages the entire repo. A caller that selects zero files will generate a commit context for all changes instead of none.
🔧 Suggested fix
const prepareCommitContext: GitCoreShape["prepareCommitContext"] = (cwd, filePaths) =>
Effect.gen(function* () {
- if (filePaths && filePaths.length > 0) {
+ if (filePaths !== undefined) {
yield* runGit("GitCore.prepareCommitContext.reset", cwd, ["reset"]).pipe(
Effect.catch(() => Effect.void),
);
+ if (filePaths.length === 0) {
+ return null;
+ }
yield* runGit("GitCore.prepareCommitContext.addSelected", cwd, [
"add",
"-A",
"--",
...filePaths,
]);
} else {
yield* runGit("GitCore.prepareCommitContext.addAll", cwd, ["add", "-A"]);
}🤖 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 944 - 958, The current
prepareCommitContext treats an explicit empty array (filePaths = []) as falsy
and falls into the "add all" branch; change the branching so that only
undefined/null filePaths triggers "git add -A" (stage everything), while an
explicit empty array means "no files selected" and should do nothing (no add).
Update prepareCommitContext to first check whether filePaths is null/undefined
(e.g., filePaths == null) and only call
runGit("GitCore.prepareCommitContext.addAll", ...) in that case; if filePaths is
an array then if length>0 run the reset + add selected via
runGit("GitCore.prepareCommitContext.reset", ...) and
runGit("GitCore.prepareCommitContext.addSelected", ...), and if length===0 skip
adding entirely. Ensure you update the condition around runGit invocations
accordingly.
4 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
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
pingdotgg/t3codeSessionTextGeneration(multi-provider text generation)SettingSourceandsettingSourcesUpstream commits included
Test plan
bun typecheckpasses across all 7 packagesSummary by CodeRabbit
Release Notes
New Features
Improvements
Chores