Repository navigation
fix(desktop,host-service): translate expected errors at throw sites, keep the Sentry boundary dumb - #6164
Conversation
…keep the Sentry boundary dumb Expected user-environment states (deleted worktrees, pathological repos, vanished files, path-validation rejections) were thrown as generic Errors, escaped tRPC procedures as INTERNAL_SERVER_ERROR, and buried real bugs under hundreds of thousands of Sentry events. The contract now enforced: - Adapters translate third-party/OS errors once (simple-git construct, getStatusNoLock, worker timeouts -> GitEnvironmentError; workspace-fs path guards -> WorkspaceFsPathError). - Routers translate expected states into non-500 TRPCErrors with kind-discriminated causes (path-validation, worktree-on-disk pre-check before spawning git tasks, errno mapping across the filesystem routers, the Project/Workspace-not-found families, per-site judgment in git.ts). - An errorFormatter serializes plain-object causes to the renderer, which now surfaces degraded states (missing worktree / not a repo / git can't read) instead of "Unable to load changes", backs polling off to 30s on deterministic failures, branches on FS_ERRNO instead of message sniffing, and drops renderer-side TRPCClientError duplicates. - The Sentry middlewares (desktop + new host-service parity) report every remaining 500 with zero allowlists: a 500 now means a missed translation at the throw site, never a filter to add. Host-service also gains unhandledRejection capture and typed Workspace/ProjectNotFoundError instead of string-prefix matching. Claude-Session: https://claude.ai/code/session_012u6VZtfQgoTh8qS4NqCb7e
📝 WalkthroughWalkthroughThe change standardizes error handling across desktop and host-service tRPC routers. It adds structured causes, typed Git and filesystem errors, Sentry reporting updates, adaptive Git-status polling, and renderer messages for unavailable workspace changes. ChangesTyped error propagation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
Greptile SummaryThis PR moves expected desktop and host-service failures into typed non-500 tRPC errors, serializes structured causes to renderer clients, and adds degraded changes-pane states with polling backoff.
Confidence Score: 4/5The getBranches degraded path needs to be surfaced before merging so branch-data failures do not leave the changes UI silently incomplete. The status and branches requests can fail independently, but the renderer only exposes the status request's error, causing branch enumeration failures to remove branch-dependent data without displaying the degraded state promised by this change. Files Needing Attention: apps/desktop/src/renderer/screens/main/hooks/useGitChangesStatus/useGitChangesStatus.ts
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/screens/main/hooks/useGitChangesStatus/useGitChangesStatus.ts | Adds typed degraded-state rendering and polling backoff, but drops the independent branches-query error. |
| apps/desktop/src/lib/trpc/routers/changes/utils/translate-git-errors.ts | Introduces on-disk worktree checks and translates named worker errors into typed tRPC failures. |
| apps/desktop/src/lib/trpc/index.ts | Serializes structured causes and simplifies Sentry reporting to all remaining internal errors. |
| apps/desktop/src/lib/trpc/utils/errno-trpc.ts | Centralizes desktop filesystem errno and path-validation translation. |
| packages/host-service/src/trpc/index.ts | Applies internal-error Sentry reporting consistently to public and protected procedures. |
| packages/host-service/src/trpc/router/filesystem/filesystem.ts | Adds typed not-found handling and filesystem error translation across host-service operations. |
| packages/workspace-fs/src/fs.ts | Changes workspace-root escape failures to the shared typed path error. |
Sequence Diagram
sequenceDiagram
participant UI as Renderer
participant RPC as tRPC Router
participant Git as Git Worker
participant Sentry
UI->>RPC: getStatus / getBranches
RPC->>Git: run typed git task
alt Expected domain or environment failure
Git-->>RPC: named/typed error
RPC-->>UI: non-500 TRPCError + structured cause
UI-->>UI: render degraded state and back off polling
else Unexpected failure
Git-->>RPC: unhandled error
RPC->>Sentry: capture INTERNAL_SERVER_ERROR
RPC-->>UI: 500 response
end
Comments Outside Diff (1)
-
apps/desktop/src/renderer/screens/main/hooks/useGitChangesStatus/useGitChangesStatus.ts, line 78 (link)Branch failures remain invisible
When
getBranchesfails independently whilegetStatussucceeds, the hook discards the branch error and deriveserrorCauseonly from the status query, causing branch-dependent controls to lose data or silently use fallback branch information without showing the degraded state.Knowledge Base Used: Desktop App (Electron main + renderer)
Prompt To Fix With AI
This is a comment left during a code review. Path: apps/desktop/src/renderer/screens/main/hooks/useGitChangesStatus/useGitChangesStatus.ts Line: 78 Comment: **Branch failures remain invisible** When `getBranches` fails independently while `getStatus` succeeds, the hook discards the branch error and derives `errorCause` only from the status query, causing branch-dependent controls to lose data or silently use fallback branch information without showing the degraded state. **Knowledge Base Used:** [Desktop App (Electron main + renderer)](https://app.greptile.com/personal-org-6810/-/custom-context/knowledge-base/superset-sh/superset/-/docs/desktop-app.md) --- For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Prompt To Fix All With AI
### Issue 1
apps/desktop/src/renderer/screens/main/hooks/useGitChangesStatus/useGitChangesStatus.ts:78
**Branch failures remain invisible**
When `getBranches` fails independently while `getStatus` succeeds, the hook discards the branch error and derives `errorCause` only from the status query, causing branch-dependent controls to lose data or silently use fallback branch information without showing the degraded state.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(desktop,host-service): translate exp..." | Re-trigger Greptile
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
…dure bodies
Kills the parallel error-translation subsystem the first pass introduced:
- translate-git-errors.ts and the shared errno-trpc modules are gone;
each procedure body catches domain errors and maps them to TRPCError
inline, so a procedure's full error behavior reads in one place.
- Utils throw domain errors only (git.ts's TRPCError throws reverted to
GitEnvironmentError), which also gets @trpc/server out of the
git-task-worker bundle.
- runGitTask rehydrates NotGitRepoError/GitEnvironmentError from the
worker's serialized {name, message}, so procedure bodies use instanceof
everywhere — the name-string checks are confined to the serialization
boundary where they belong.
- delete.ts needed nothing: its existing body-level catches already
degrade unreadable repos to canDelete:false; create.ts gained inline
arms around safeCheckoutBranch/getPrInfo/handleNewWorktree.
Claude-Session: https://claude.ai/code/session_012u6VZtfQgoTh8qS4NqCb7e
![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
🧹 Nitpick comments (1)
apps/desktop/src/renderer/lib/sentry.ts (1)
22-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the Sentry
TRPCClientErrorfilter to server-returned tRPC errors.The renderer link uses
TRPCClientErrorfor IPC responses, butname === "TRPCClientError"also captures transport, deserialization, and client-link failures. Those should not be dropped unless the error is a procedure result response, for examplehint.originalException?.data?.code === "INTERNAL_SERVER_ERROR". Add coverage for rejected client-link operations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/lib/sentry.ts` around lines 22 - 31, Update the beforeSend filter in the Sentry configuration to ignore only TRPCClientError instances whose originalException data indicates a server-returned procedure error, such as code INTERNAL_SERVER_ERROR. Continue reporting transport, deserialization, and other client-link failures, and add coverage for rejected client-link operations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts`:
- Around line 878-887: The worktree validation currently accepts Git porcelain
metadata even when the linked worktree is missing on disk. Update worktreeExists
and the OPEN_WORKSPACE/openExternalWorktree flows in
apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts:878-887 and
apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-creation.ts:339-348
to require the worktree path and its linked gitdir location to exist, or prune
stale metadata before proceeding; otherwise consistently reject with
WORKTREE_MISSING.
In `@packages/host-service/src/trpc/errno-trpc.ts`:
- Around line 49-56: Replace the inherited-key-sensitive errno mapping check in
the translator around ERRNO_TO_TRPC with an own-property check, preserving the
existing mapping and TRPCError behavior for valid errno codes. Apply the same
change in packages/host-service/src/trpc/errno-trpc.ts lines 49-56 and
apps/desktop/src/lib/trpc/utils/errno-trpc.ts lines 37-44.
---
Nitpick comments:
In `@apps/desktop/src/renderer/lib/sentry.ts`:
- Around line 22-31: Update the beforeSend filter in the Sentry configuration to
ignore only TRPCClientError instances whose originalException data indicates a
server-returned procedure error, such as code INTERNAL_SERVER_ERROR. Continue
reporting transport, deserialization, and other client-link failures, and add
coverage for rejected client-link operations.
🪄 Autofix
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 Plus
Run ID: 944de3a6-4483-440e-9cf3-866ac62d7d4b
📒 Files selected for processing (41)
apps/desktop/src/lib/trpc/index.tsapps/desktop/src/lib/trpc/routers/browser-history/index.tsapps/desktop/src/lib/trpc/routers/changes/branches.tsapps/desktop/src/lib/trpc/routers/changes/security/git-commands.tsapps/desktop/src/lib/trpc/routers/changes/security/path-validation.tsapps/desktop/src/lib/trpc/routers/changes/status.tsapps/desktop/src/lib/trpc/routers/changes/utils/translate-git-errors.tsapps/desktop/src/lib/trpc/routers/changes/workers/git-task-runner.tsapps/desktop/src/lib/trpc/routers/config/config.tsapps/desktop/src/lib/trpc/routers/filesystem/index.tsapps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/lib/trpc/routers/ui-state/index.tsapps/desktop/src/lib/trpc/routers/workspace-fs-service.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/create.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/generate-branch-name.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/init.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/sections.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/git-client.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/git-errors.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/git.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/usability.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-creation.tsapps/desktop/src/lib/trpc/utils/errno-trpc.tsapps/desktop/src/renderer/lib/sentry.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/ChangesContent.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/hooks/useFileContent/useFileContent.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsxapps/desktop/src/renderer/screens/main/hooks/useGitChangesStatus/index.tsapps/desktop/src/renderer/screens/main/hooks/useGitChangesStatus/useGitChangesStatus.test.tsapps/desktop/src/renderer/screens/main/hooks/useGitChangesStatus/useGitChangesStatus.tsapps/desktop/src/shared/changes-types.tsapps/desktop/src/shared/fs-error-types.tsapps/desktop/src/shared/workspace-error-types.tspackages/host-service/src/runtime/filesystem/filesystem.tspackages/host-service/src/safety.tspackages/host-service/src/trpc/errno-trpc.tspackages/host-service/src/trpc/index.tspackages/host-service/src/trpc/router/filesystem/filesystem.tspackages/workspace-fs/src/fs.ts
| const exists = await worktreeExists( | ||
| project.mainRepoPath, | ||
| worktree.path, | ||
| ); | ||
| if (!exists) { | ||
| throw new Error("Worktree no longer exists on disk"); | ||
| throw new TRPCError({ | ||
| code: "NOT_FOUND", | ||
| message: "Worktree no longer exists on disk", | ||
| cause: { kind: "WORKTREE_MISSING", worktreePath: worktree.path }, | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
tmp_dir="$(mktemp -d)"
trap 'rm -rf "$tmp_dir"' EXIT
git init -q "$tmp_dir/main"
git -C "$tmp_dir/main" config user.email "review@example.invalid"
git -C "$tmp_dir/main" config user.name "Review"
touch "$tmp_dir/main/file"
git -C "$tmp_dir/main" add file
git -C "$tmp_dir/main" commit -qm initial
git -C "$tmp_dir/main" worktree add -q -b removed "$tmp_dir/removed"
rm -rf "$tmp_dir/removed"
output="$(git -C "$tmp_dir/main" worktree list --porcelain)"
printf '%s\n' "$output"
grep -Fx "worktree $tmp_dir/removed" <<<"$output"
grep -q '^prunable' <<<"$output"
test ! -e "$tmp_dir/removed"Repository: superset-sh/superset
Length of output: 463
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "Candidate files:"
git ls-files | rg 'apps/desktop/src/lib/trpc/routers/workspaces/(procedures/create\.ts|utils/workspace-creation\.ts)$' || true
echo
echo "Outline create.ts around workspace/worktree handling:"
ast-grep outline apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts --view condensed 2>/dev/null | sed -n '1,220p' || true
echo
echo "Outline workspace-creation.ts around listExternalWorktrees:"
ast-grep outline apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-creation.ts --view condensed 2>/dev/null | sed -n '1,220p' || true
echo
echo "Relevant create.ts lines:"
nl -ba apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts | sed -n '830,905p'
echo
echo "Relevant workspace-creation.ts lines:"
nl -ba apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-creation.ts | sed -n '300,370p'
echo
echo "Search for worktreeExists/listExternalWorktrees definitions/usages:"
rg -n "function worktreeExists|const worktreeExists|worktreeExists|function listExternalWorktrees|const listExternalWorktrees|listExternalWorktrees|prunable|no-worktree|WORKTREE_MISSING" apps/desktop/src/lib/trpc/routers/workspaces -SRepository: superset-sh/superset
Length of output: 498
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "Candidate files:"
git ls-files | grep -E 'apps/desktop/src/lib/trpc/routers/workspaces/(procedures/create\.ts|utils/workspace-creation\.ts)$' || true
echo
echo "Relevant create.ts lines:"
awk '{printf "%6d\t%s\n", NR, $0}' apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts | sed -n '830,905p'
echo
echo "Relevant workspace-creation.ts lines:"
awk '{printf "%6d\t%s\n", NR, $0}' apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-creation.ts | sed -n '300,370p'
echo
echo "Definitions/usages in tracked files:"
grep -RInE "worktreeExists|listExternalWorktrees|WORKTREE_MISSING|no-worktree|prunable|gitdir|non-existent location" apps/desktop/src/lib/trpc/routers/workspaces || true
echo
echo "Definitions of Git helpers (tracked shell/ts files, if any):"
grep -RInE "function worktreeExists|const worktreeExists|worktreeExists|function listExternalWorktrees|const listExternalWorktrees|listExternalWorktrees" . || trueRepository: superset-sh/superset
Length of output: 16220
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "git.ts helpers:"
awk '{printf "%6d\t%s\n", NR, $0}' apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts | sed -n '820,905p'
echo
echo "listExternalWorktrees tests:"
awk '{printf "%6d\t%s\n", NR, $0}' apps/desktop/src/lib/trpc/routers/workspaces/utils/select-external-worktrees-for-import.integration.test.ts | sed -n '70,115p'
awk '{printf "%6d\t%s\n", NR, $0}' apps/desktop/src/lib/trpc/routers/workspaces/procedures/external-worktree-import.test.ts | sed -n '105,132p'
echo
echo "Open external worktree tests/calls:"
rg -n "openExternalWorktree|does not exist|deleted|removed" apps/desktop/src/lib/trpc/routers/workspaces apps/desktop/src/renderer apps/desktop/src -g '*.ts' -g '*.tsx' | sed -n '1,220p'
echo
echo "Search in apps/desktop TypeScript files only:"
find apps/desktop/src -type f \( -name '*.ts' -o -name '*.tsx' \) -print0 | xargs -0 grep -In "listExternalWorktrees\|worktreeExists\|worktree no longer exists|WORKTREE_MISSING|no-worktree|remove --prune|git worktree prune" | sed -n '1,260p'Repository: superset-sh/superset
Length of output: 39993
Reject missing linked worktrees before accepting Git metadata.
worktreeExists, OPEN_WORKSPACE, and openExternalWorktree currently treat a worktree <path> porcelain entry as live even when gitdir file points to non-existent location follows it. That lets a manually deleted worktree bypass WORKTREE_MISSING; require disk existence or git worktree prune before opening/importing the worktree.
📍 Affects 2 files
apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts#L878-L887(this comment)apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-creation.ts#L339-L348
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts` around
lines 878 - 887, The worktree validation currently accepts Git porcelain
metadata even when the linked worktree is missing on disk. Update worktreeExists
and the OPEN_WORKSPACE/openExternalWorktree flows in
apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts:878-887 and
apps/desktop/src/lib/trpc/routers/workspaces/utils/workspace-creation.ts:339-348
to require the worktree path and its linked gitdir location to exist, or prune
stale metadata before proceeding; otherwise consistently reject with
WORKTREE_MISSING.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (1)
226-239: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep parser failures as internal errors.
parsePorcelainStatusV2(stdout)runs in the sametryblock as Git execution. If parsing fails, Line 237 converts the failure toGitEnvironmentError.createStatusRouterthen returnsPRECONDITION_FAILED, so Sentry does not receive the parser failure asINTERNAL_SERVER_ERROR.Limit
GitEnvironmentErrorconversion toexecGitWithShellPathfailures. Parsestdoutafter thetryblock.Proposed fix
export async function getStatusNoLock(repoPath: string): Promise<StatusResult> { const env = await getGitEnv(); + let stdout: string; try { - const { stdout } = await execGitWithShellPath( + ({ stdout } = await execGitWithShellPath( [ "--no-optional-locks", "-C", repoPath, @@ ], { env, timeout: 30_000, maxBuffer: 10 * 1024 * 1024 }, - ); - - return parsePorcelainStatusV2(stdout); + )); } catch (error) { // Existing Git execution classification. } + + return parsePorcelainStatusV2(stdout); }Based on PR objectives, remaining internal failures must reach Sentry middleware.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts` around lines 226 - 239, Limit the GitEnvironmentError handling in the status retrieval flow to failures from execGitWithShellPath only. Move parsePorcelainStatusV2(stdout) outside the try/catch so parser failures propagate unchanged as internal errors, while preserving the existing GitEnvironmentError and NotGitRepoError handling for execution failures.
🧹 Nitpick comments (3)
apps/desktop/src/lib/trpc/routers/ui-state/index.ts (1)
12-21: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAttach the structured
FsErrnoCauseto these errors.The filesystem routers attach
{ kind: "FS_ERRNO", errno }so the renderer classifies failures by errno instead of message text.writeAppStatesets onlycodeandmessage. A renderer that needs to distinguish a full disk from a permission wall on these two mutations must then match message text, which this PR removes elsewhere.Add the same cause here.
♻️ Proposed change to emit a typed cause
+import type { FsErrnoCause } from "shared/fs-error-types";} catch (error) { const errno = (error as NodeJS.ErrnoException | null)?.code; if (errno === "ENOSPC" || errno === "EACCES" || errno === "EPERM") { + const cause: FsErrnoCause = { kind: "FS_ERRNO", errno }; throw new TRPCError({ code: errno === "ENOSPC" ? "PRECONDITION_FAILED" : "FORBIDDEN", message: error instanceof Error ? error.message : String(error), + cause, }); } throw error; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/lib/trpc/routers/ui-state/index.ts` around lines 12 - 21, Update the error handling in the affected UI-state mutation around the shown catch block to include a structured FsErrnoCause containing kind "FS_ERRNO" and the captured errno when constructing TRPCError. Preserve the existing errno-to-TRPC-code mapping and message behavior, and continue rethrowing unrelated errors unchanged.packages/host-service/src/trpc/router/filesystem/filesystem.ts (1)
14-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe errno→TRPC code mapping is duplicated in three places. The same six errno codes and the same target tRPC codes are hand-written at each site, and host-service also re-declares
FsErrnoCodeandFsErrnoCause. The renderer classifies these failures byerrno, so any divergence between copies turns a classified failure back into an unclassified 500. This PR intentionally removed the previous shared translation modules, so treat this as a follow-up rather than a blocker.
packages/host-service/src/trpc/router/filesystem/filesystem.ts#L14-L42: importFsErrnoCode,FsErrnoCause, and the mapping tables from a package both host-service and desktop can depend on, such as@superset/workspace-fs, instead of re-declaring them.apps/desktop/src/lib/trpc/routers/filesystem/index.ts#L12-L27: import the same shared tables instead of declaring local copies.apps/desktop/src/lib/trpc/routers/ui-state/index.ts#L9-L22: derive the tRPC code from the shared errno map rather than the hand-writtenENOSPC/EACCES/EPERMternary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/host-service/src/trpc/router/filesystem/filesystem.ts` around lines 14 - 42, Centralize the errno types, causes, and errno-to-tRPC/path-error mappings in a shared package consumable by host-service and desktop. In packages/host-service/src/trpc/router/filesystem/filesystem.ts:14-42, import and reuse the shared definitions instead of redeclaring them; in apps/desktop/src/lib/trpc/routers/filesystem/index.ts:12-27, replace the local copies with the shared tables; and in apps/desktop/src/lib/trpc/routers/ui-state/index.ts:9-22, derive the tRPC code from the shared errno map instead of the hand-written ENOSPC/EACCES/EPERM ternary.apps/desktop/src/lib/trpc/routers/filesystem/index.ts (1)
36-42: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueUse the shared path validation cause type here.
The runtime
PATH_VALIDATIONshape is already captured byapps/desktop/src/shared/changes-types.ts; declarewithFsErrorTranslation’s cause with that shared type like theFsErrnoCausebranch. Keepworkspace-fs-service.ts:94on the same shared type if these failure codes travel over the same contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/lib/trpc/routers/filesystem/index.ts` around lines 36 - 42, Update withFsErrorTranslation to type its PATH_VALIDATION cause using the shared path-validation cause type from changes-types.ts, matching the existing FsErrnoCause branch. Ensure the corresponding cause declaration in workspace-fs-service.ts remains aligned with that same shared type for failures crossing the contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts`:
- Around line 226-239: Limit the GitEnvironmentError handling in the status
retrieval flow to failures from execGitWithShellPath only. Move
parsePorcelainStatusV2(stdout) outside the try/catch so parser failures
propagate unchanged as internal errors, while preserving the existing
GitEnvironmentError and NotGitRepoError handling for execution failures.
---
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/filesystem/index.ts`:
- Around line 36-42: Update withFsErrorTranslation to type its PATH_VALIDATION
cause using the shared path-validation cause type from changes-types.ts,
matching the existing FsErrnoCause branch. Ensure the corresponding cause
declaration in workspace-fs-service.ts remains aligned with that same shared
type for failures crossing the contract.
In `@apps/desktop/src/lib/trpc/routers/ui-state/index.ts`:
- Around line 12-21: Update the error handling in the affected UI-state mutation
around the shown catch block to include a structured FsErrnoCause containing
kind "FS_ERRNO" and the captured errno when constructing TRPCError. Preserve the
existing errno-to-TRPC-code mapping and message behavior, and continue
rethrowing unrelated errors unchanged.
In `@packages/host-service/src/trpc/router/filesystem/filesystem.ts`:
- Around line 14-42: Centralize the errno types, causes, and
errno-to-tRPC/path-error mappings in a shared package consumable by host-service
and desktop. In
packages/host-service/src/trpc/router/filesystem/filesystem.ts:14-42, import and
reuse the shared definitions instead of redeclaring them; in
apps/desktop/src/lib/trpc/routers/filesystem/index.ts:12-27, replace the local
copies with the shared tables; and in
apps/desktop/src/lib/trpc/routers/ui-state/index.ts:9-22, derive the tRPC code
from the shared errno map instead of the hand-written ENOSPC/EACCES/EPERM
ternary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ddf2a47f-bd8a-42e2-888a-73522dc1f886
📒 Files selected for processing (8)
apps/desktop/src/lib/trpc/routers/changes/branches.tsapps/desktop/src/lib/trpc/routers/changes/status.tsapps/desktop/src/lib/trpc/routers/changes/workers/git-task-runner.tsapps/desktop/src/lib/trpc/routers/filesystem/index.tsapps/desktop/src/lib/trpc/routers/ui-state/index.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/create.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/git.tspackages/host-service/src/trpc/router/filesystem/filesystem.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/lib/trpc/routers/changes/branches.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/lib/trpc/routers/config/config.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/config/config.ts:447">
P3: The new NOT_FOUND TRPCError message is a generic "Project not found" and drops the `projectId` that is in scope at the throw site. In a multi-project app that makes a failing config call hard to attribute to a specific workspace during debugging; consider including the id, e.g. `Project not found: ${input.projectId}` (applies to both getSetupOnboardingDefaults and updateConfig).</violation>
</file>
<file name="apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts:226">
P2: The broad catch in getStatusNoLock now converts every non-ENOENT/non-not-a-repo failure into GitEnvironmentError, which translateGitTaskError turns into a PRECONDITION_FAILED (412) and which the Sentry middleware skips by name. That means an unexpected internal error raised anywhere inside the try (a programming bug, not an environment condition) is also reclassified as a routine environment error and stops being reported to Sentry, contradicting the PR's stated goal that unknowns should stay 500. Consider narrowing the environment classification to the explicitly-recognized cases (maxBuffer overflow, timeout, permission wall) or keeping the distinguishing condition (e.g. an isExecFileException / timeout flag) on the GitEnvironmentError so genuine unexpected errors still propagate as 500s.</violation>
</file>
<file name="apps/desktop/src/lib/trpc/routers/workspace-fs-service.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/workspace-fs-service.ts:35">
P3: The two NOT_FOUND errors here share the same `{ kind: "WORKSPACE_NOT_FOUND" }` cause even though they represent different states: one means the workspace record is missing, the other means the workspace exists but its root path is empty. Since the rest of this PR is specifically about giving the renderer typed, branchable error states, collapsing these two into one kind prevents UI from distinguishing them. Consider a dedicated kind for the path-resolution failure (e.g., `kind: "WORKSPACE_PATH_NOT_FOUND"`) so the two states stay distinguishable.</violation>
<violation number="2" location="apps/desktop/src/lib/trpc/routers/workspace-fs-service.ts:94">
P3: This inlines the same `PATH_VALIDATION` cause construction already used in security/path-validation.ts and utils/errno-trpc.ts. Export a shared helper (e.g. `pathValidationError`) or a single constructor for the `{ kind: "PATH_VALIDATION", code }` cause and reuse it, so the renderer-facing contract (shared/changes-types.ts) doesn't drift across three sites.</violation>
</file>
<file name="packages/host-service/src/trpc/index.ts">
<violation number="1" location="packages/host-service/src/trpc/index.ts:79">
P2: Workspace deletion with a failed teardown script is an expected, recoverable user flow, but this unconditional `INTERNAL_SERVER_ERROR` check reports each one to Sentry and can recreate the non-bug 500 noise this PR is meant to remove. Assign `TEARDOWN_FAILED` a non-500 TRPC code at its throw site while preserving its structured cause for the renderer.</violation>
</file>
<file name="apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsx">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsx:675">
P2: When the status query errors while stale-but-valid status data is still cached (react-query keeps `data` on refetch failure), this early return replaces the usable changes list with a static error message for the full 30s backoff, even for a one-off transient failure. Consider gating the error screen on no usable data (e.g. `if (errorCause && !status)`), falling back to showing the stale list so recoverable hiccups don't blank the panel.</violation>
</file>
<file name="apps/desktop/src/shared/changes-types.ts">
<violation number="1" location="apps/desktop/src/shared/changes-types.ts:85">
P2: Malformed serialized causes are accepted as fully typed causes, so invalid `PATH_VALIDATION` payloads can reach the renderer and produce incorrect UI classification instead of being treated as unknown. Validating each variant's required fields in this guard would keep the runtime narrowing sound.</violation>
</file>
<file name="apps/desktop/src/lib/trpc/routers/projects/projects.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/projects/projects.ts:779">
P2: The new `.catch` on `git.branch` converts every failure to `PRECONDITION_FAILED` and drops the original error as a `cause`. Elsewhere in this same change-set (translate-git-errors.ts) a not-a-git-repo failure is typed as `BAD_REQUEST` with a `{kind:'NOT_GIT_REPO'}` cause and a git-environment failure as `PRECONDITION_FAILED` with a `{kind:'GIT_ENVIRONMENT'}` cause, so the renderer/Sentry can branch on the serialized kind. By collapsing all failures into one un-caused 412 here, a repo that has stopped being a git repo (or whose worktree is missing) surfaces as the wrong code and with no cause, making it indistinguishable from a real environment problem. Consider routing through the same translation so expected states get correct codes and a serialized `cause` for diagnostics.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if (isExecFileException(error)) { | ||
| if (error.code === "ENOENT") { | ||
| throw new Error("Git is not installed or not found in PATH"); | ||
| throw new GitEnvironmentError( |
There was a problem hiding this comment.
P2: The broad catch in getStatusNoLock now converts every non-ENOENT/non-not-a-repo failure into GitEnvironmentError, which translateGitTaskError turns into a PRECONDITION_FAILED (412) and which the Sentry middleware skips by name. That means an unexpected internal error raised anywhere inside the try (a programming bug, not an environment condition) is also reclassified as a routine environment error and stops being reported to Sentry, contradicting the PR's stated goal that unknowns should stay 500. Consider narrowing the environment classification to the explicitly-recognized cases (maxBuffer overflow, timeout, permission wall) or keeping the distinguishing condition (e.g. an isExecFileException / timeout flag) on the GitEnvironmentError so genuine unexpected errors still propagate as 500s.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts, line 228:
<comment>The broad catch in getStatusNoLock now converts every non-ENOENT/non-not-a-repo failure into GitEnvironmentError, which translateGitTaskError turns into a PRECONDITION_FAILED (412) and which the Sentry middleware skips by name. That means an unexpected internal error raised anywhere inside the try (a programming bug, not an environment condition) is also reclassified as a routine environment error and stops being reported to Sentry, contradicting the PR's stated goal that unknowns should stay 500. Consider narrowing the environment classification to the explicitly-recognized cases (maxBuffer overflow, timeout, permission wall) or keeping the distinguishing condition (e.g. an isExecFileException / timeout flag) on the GitEnvironmentError so genuine unexpected errors still propagate as 500s.</comment>
<file context>
@@ -222,14 +225,18 @@ export async function getStatusNoLock(repoPath: string): Promise<StatusResult> {
if (isExecFileException(error)) {
if (error.code === "ENOENT") {
- throw new Error("Git is not installed or not found in PATH");
+ throw new GitEnvironmentError(
+ "Git is not installed or not found in PATH",
+ );
</file context>
| const sentryMiddleware = t.middleware(async ({ next, path, type }) => { | ||
| const result = await next(); | ||
|
|
||
| if (!result.ok && result.error.code === "INTERNAL_SERVER_ERROR") { |
There was a problem hiding this comment.
P2: Workspace deletion with a failed teardown script is an expected, recoverable user flow, but this unconditional INTERNAL_SERVER_ERROR check reports each one to Sentry and can recreate the non-bug 500 noise this PR is meant to remove. Assign TEARDOWN_FAILED a non-500 TRPC code at its throw site while preserving its structured cause for the renderer.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/host-service/src/trpc/index.ts, line 79:
<comment>Workspace deletion with a failed teardown script is an expected, recoverable user flow, but this unconditional `INTERNAL_SERVER_ERROR` check reports each one to Sentry and can recreate the non-bug 500 noise this PR is meant to remove. Assign `TEARDOWN_FAILED` a non-500 TRPC code at its throw site while preserving its structured cause for the renderer.</comment>
<file context>
@@ -64,10 +65,42 @@ const t = initTRPC
+const sentryMiddleware = t.middleware(async ({ next, path, type }) => {
+ const result = await next();
+
+ if (!result.ok && result.error.code === "INTERNAL_SERVER_ERROR") {
+ const error = result.error;
+ const originalError = error.cause instanceof Error ? error.cause : error;
</file context>
| ); | ||
| } | ||
|
|
||
| if (errorCause) { |
There was a problem hiding this comment.
P2: When the status query errors while stale-but-valid status data is still cached (react-query keeps data on refetch failure), this early return replaces the usable changes list with a static error message for the full 30s backoff, even for a one-off transient failure. Consider gating the error screen on no usable data (e.g. if (errorCause && !status)), falling back to showing the stale list so recoverable hiccups don't blank the panel.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsx, line 675:
<comment>When the status query errors while stale-but-valid status data is still cached (react-query keeps `data` on refetch failure), this early return replaces the usable changes list with a static error message for the full 30s backoff, even for a one-off transient failure. Consider gating the error screen on no usable data (e.g. `if (errorCause && !status)`), falling back to showing the stale list so recoverable hiccups don't blank the panel.</comment>
<file context>
@@ -663,6 +672,14 @@ export function ChangesView({
);
}
+ if (errorCause) {
+ return (
+ <div className="flex-1 flex select-text cursor-text items-center justify-center text-muted-foreground text-sm p-4">
</file context>
| if (errorCause) { | |
| if (errorCause && !status) { |
| "PATH_VALIDATION", | ||
| ]); | ||
|
|
||
| export function isGitChangesErrorCause( |
There was a problem hiding this comment.
P2: Malformed serialized causes are accepted as fully typed causes, so invalid PATH_VALIDATION payloads can reach the renderer and produce incorrect UI classification instead of being treated as unknown. Validating each variant's required fields in this guard would keep the runtime narrowing sound.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/shared/changes-types.ts, line 85:
<comment>Malformed serialized causes are accepted as fully typed causes, so invalid `PATH_VALIDATION` payloads can reach the renderer and produce incorrect UI classification instead of being treated as unknown. Validating each variant's required fields in this guard would keep the runtime narrowing sound.</comment>
<file context>
@@ -56,6 +56,43 @@ export interface GitChangesStatus {
+ "PATH_VALIDATION",
+]);
+
+export function isGitChangesErrorCause(
+ value: unknown,
+): value is GitChangesErrorCause {
</file context>
| .branch(["-a"]) | ||
| .catch((error: unknown) => { | ||
| throw new TRPCError({ | ||
| code: "PRECONDITION_FAILED", |
There was a problem hiding this comment.
P2: The new .catch on git.branch converts every failure to PRECONDITION_FAILED and drops the original error as a cause. Elsewhere in this same change-set (translate-git-errors.ts) a not-a-git-repo failure is typed as BAD_REQUEST with a {kind:'NOT_GIT_REPO'} cause and a git-environment failure as PRECONDITION_FAILED with a {kind:'GIT_ENVIRONMENT'} cause, so the renderer/Sentry can branch on the serialized kind. By collapsing all failures into one un-caused 412 here, a repo that has stopped being a git repo (or whose worktree is missing) surfaces as the wrong code and with no cause, making it indistinguishable from a real environment problem. Consider routing through the same translation so expected states get correct codes and a serialized cause for diagnostics.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/lib/trpc/routers/projects/projects.ts, line 779:
<comment>The new `.catch` on `git.branch` converts every failure to `PRECONDITION_FAILED` and drops the original error as a `cause`. Elsewhere in this same change-set (translate-git-errors.ts) a not-a-git-repo failure is typed as `BAD_REQUEST` with a `{kind:'NOT_GIT_REPO'}` cause and a git-environment failure as `PRECONDITION_FAILED` with a `{kind:'GIT_ENVIRONMENT'}` cause, so the renderer/Sentry can branch on the serialized kind. By collapsing all failures into one un-caused 412 here, a repo that has stopped being a git repo (or whose worktree is missing) surfaces as the wrong code and with no cause, making it indistinguishable from a real environment problem. Consider routing through the same translation so expected states get correct codes and a serialized `cause` for diagnostics.</comment>
<file context>
@@ -766,7 +772,14 @@ export const createProjectsRouter = (getWindow: () => BrowserWindow | null) => {
+ .branch(["-a"])
+ .catch((error: unknown) => {
+ throw new TRPCError({
+ code: "PRECONDITION_FAILED",
+ message: error instanceof Error ? error.message : String(error),
+ });
</file context>
| .get(); | ||
| if (!project) { | ||
| throw new Error("Project not found"); | ||
| throw new TRPCError({ |
There was a problem hiding this comment.
P3: The new NOT_FOUND TRPCError message is a generic "Project not found" and drops the projectId that is in scope at the throw site. In a multi-project app that makes a failing config call hard to attribute to a specific workspace during debugging; consider including the id, e.g. Project not found: ${input.projectId} (applies to both getSetupOnboardingDefaults and updateConfig).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/lib/trpc/routers/config/config.ts, line 447:
<comment>The new NOT_FOUND TRPCError message is a generic "Project not found" and drops the `projectId` that is in scope at the throw site. In a multi-project app that makes a failing config call hard to attribute to a specific workspace during debugging; consider including the id, e.g. `Project not found: ${input.projectId}` (applies to both getSetupOnboardingDefaults and updateConfig).</comment>
<file context>
@@ -443,7 +444,10 @@ export const createConfigRouter = () => {
.get();
if (!project) {
- throw new Error("Project not found");
+ throw new TRPCError({
+ code: "NOT_FOUND",
+ message: "Project not found",
</file context>
| throw new TRPCError({ | ||
| code: "BAD_REQUEST", | ||
| message: `Path is outside worktree: ${absolutePath}`, | ||
| cause: { kind: "PATH_VALIDATION", code: "INVALID_TARGET" }, |
There was a problem hiding this comment.
P3: This inlines the same PATH_VALIDATION cause construction already used in security/path-validation.ts and utils/errno-trpc.ts. Export a shared helper (e.g. pathValidationError) or a single constructor for the { kind: "PATH_VALIDATION", code } cause and reuse it, so the renderer-facing contract (shared/changes-types.ts) doesn't drift across three sites.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/lib/trpc/routers/workspace-fs-service.ts, line 94:
<comment>This inlines the same `PATH_VALIDATION` cause construction already used in security/path-validation.ts and utils/errno-trpc.ts. Export a shared helper (e.g. `pathValidationError`) or a single constructor for the `{ kind: "PATH_VALIDATION", code }` cause and reuse it, so the renderer-facing contract (shared/changes-types.ts) doesn't drift across three sites.</comment>
<file context>
@@ -79,7 +88,11 @@ export function toRegisteredWorktreeRelativePath(
+ throw new TRPCError({
+ code: "BAD_REQUEST",
+ message: `Path is outside worktree: ${absolutePath}`,
+ cause: { kind: "PATH_VALIDATION", code: "INVALID_TARGET" },
+ });
}
</file context>
| const workspace = getWorkspace(workspaceId); | ||
| if (!workspace) { | ||
| throw new Error(`Workspace not found: ${workspaceId}`); | ||
| throw new TRPCError({ |
There was a problem hiding this comment.
P3: The two NOT_FOUND errors here share the same { kind: "WORKSPACE_NOT_FOUND" } cause even though they represent different states: one means the workspace record is missing, the other means the workspace exists but its root path is empty. Since the rest of this PR is specifically about giving the renderer typed, branchable error states, collapsing these two into one kind prevents UI from distinguishing them. Consider a dedicated kind for the path-resolution failure (e.g., kind: "WORKSPACE_PATH_NOT_FOUND") so the two states stay distinguishable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/lib/trpc/routers/workspace-fs-service.ts, line 35:
<comment>The two NOT_FOUND errors here share the same `{ kind: "WORKSPACE_NOT_FOUND" }` cause even though they represent different states: one means the workspace record is missing, the other means the workspace exists but its root path is empty. Since the rest of this PR is specifically about giving the renderer typed, branchable error states, collapsing these two into one kind prevents UI from distinguishing them. Consider a dedicated kind for the path-resolution failure (e.g., `kind: "WORKSPACE_PATH_NOT_FOUND"`) so the two states stay distinguishable.</comment>
<file context>
@@ -31,12 +32,20 @@ const sharedHostServiceOptions = {
const workspace = getWorkspace(workspaceId);
if (!workspace) {
- throw new Error(`Workspace not found: ${workspaceId}`);
+ throw new TRPCError({
+ code: "NOT_FOUND",
+ message: `Workspace not found: ${workspaceId}`,
</file context>
There was a problem hiding this comment.
6 issues found across 11 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/lib/trpc/routers/changes/status.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/changes/status.ts:81">
P3: The `if (error instanceof NotGitRepoError) → BAD_REQUEST` / `if (error instanceof GitEnvironmentError) → PRECONDITION_FAILED` translation is copy-pasted across status.ts (twice), branches.ts, and create.ts (three times), replacing what was previously a single shared `translateGitTaskError` helper. Keeping the mapping logic in one place (e.g., a small `translateGitTaskError(error)` that rethrows typed TRPCErrors) would make the not-found / environment mapping consistent and easier to evolve without touching six call sites.</violation>
<violation number="2" location="apps/desktop/src/lib/trpc/routers/changes/status.ts:95">
P1: Expanding a commit after its registered worktree is deleted, or when the registered path is not a repository, still produces a 500 and Sentry event: `getCommitFiles` invokes raw `simple-git` without translation and this fallback rethrows its `GitError`. Translating that worker failure (or applying the same existence/repository pre-check before the task) would preserve the intended NOT_FOUND/BAD_REQUEST behavior.</violation>
</file>
<file name="apps/desktop/src/lib/trpc/routers/changes/branches.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/changes/branches.ts:76">
P1: Non-repository `changes.getBranches` calls still surface as 500s instead of the intended `BAD_REQUEST`/`NOT_GIT_REPO` response. `computeBranches` fails in its initial `git.branch(["-a"])` call before `getCurrentBranch` can run, and that simple-git error is not translated into `NotGitRepoError`; translating that failure in the task handler would make this mapping effective.</violation>
</file>
<file name="apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts:761">
P2: Selecting a branch that no longer exists still returns a 500 because `safeCheckoutBranch` leaves the checkout rejection as a plain error and this catch rethrows it; wrapping that known checkout failure in a domain error (or translating the specific missing-branch case) would keep this workflow out of the Sentry 500 path while preserving unknown errors.</violation>
</file>
<file name="packages/host-service/src/trpc/router/filesystem/filesystem.ts">
<violation number="1" location="packages/host-service/src/trpc/router/filesystem/filesystem.ts:49">
P3: Filesystem error translation is now maintained independently in the desktop and host-service routers, making fixes and newly supported errno/path codes prone to diverging. A shared host-compatible translator or centralized error-mapping contract would avoid this behavior drift.</violation>
</file>
<file name="apps/desktop/src/lib/trpc/routers/ui-state/index.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/ui-state/index.ts:17">
P2: UI-state filesystem failures lose their structured errno when translated, so the renderer cannot distinguish the specific environment failure through the documented `FS_ERRNO` cause contract. Preserving `cause: { kind: "FS_ERRNO", errno }` here would keep this translation consistent with the filesystem router and retain actionable diagnostics.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| cause: { kind: "GIT_ENVIRONMENT" }, | ||
| }); | ||
| } | ||
| throw error; |
There was a problem hiding this comment.
P1: Expanding a commit after its registered worktree is deleted, or when the registered path is not a repository, still produces a 500 and Sentry event: getCommitFiles invokes raw simple-git without translation and this fallback rethrows its GitError. Translating that worker failure (or applying the same existence/repository pre-check before the task) would preserve the intended NOT_FOUND/BAD_REQUEST behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/lib/trpc/routers/changes/status.ts, line 95:
<comment>Expanding a commit after its registered worktree is deleted, or when the registered path is not a repository, still produces a 500 and Sentry event: `getCommitFiles` invokes raw `simple-git` without translation and this fallback rethrows its `GitError`. Translating that worker failure (or applying the same existence/repository pre-check before the task) would preserve the intended NOT_FOUND/BAD_REQUEST behavior.</comment>
<file context>
@@ -67,7 +78,21 @@ export const createStatusRouter = () => {
+ cause: { kind: "GIT_ENVIRONMENT" },
+ });
+ }
+ throw error;
}
})();
</file context>
| }, | ||
| ); | ||
| } catch (error) { | ||
| if (error instanceof NotGitRepoError) { |
There was a problem hiding this comment.
P1: Non-repository changes.getBranches calls still surface as 500s instead of the intended BAD_REQUEST/NOT_GIT_REPO response. computeBranches fails in its initial git.branch(["-a"]) call before getCurrentBranch can run, and that simple-git error is not translated into NotGitRepoError; translating that failure in the task handler would make this mapping effective.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/lib/trpc/routers/changes/branches.ts, line 76:
<comment>Non-repository `changes.getBranches` calls still surface as 500s instead of the intended `BAD_REQUEST`/`NOT_GIT_REPO` response. `computeBranches` fails in its initial `git.branch(["-a"])` call before `getCurrentBranch` can run, and that simple-git error is not translated into `NotGitRepoError`; translating that failure in the task handler would make this mapping effective.</comment>
<file context>
@@ -65,7 +73,21 @@ export const createBranchesRouter = () => {
);
} catch (error) {
- translateGitTaskError(error);
+ if (error instanceof NotGitRepoError) {
+ throw new TRPCError({
+ code: "BAD_REQUEST",
</file context>
| message: error.message, | ||
| }); | ||
| } | ||
| throw error; |
There was a problem hiding this comment.
P2: Selecting a branch that no longer exists still returns a 500 because safeCheckoutBranch leaves the checkout rejection as a plain error and this catch rethrows it; wrapping that known checkout failure in a domain error (or translating the specific missing-branch case) would keep this workflow out of the Sentry 500 path while preserving unknown errors.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts, line 761:
<comment>Selecting a branch that no longer exists still returns a 500 because `safeCheckoutBranch` leaves the checkout rejection as a plain error and this catch rethrows it; wrapping that known checkout failure in a domain error (or translating the specific missing-branch case) would keep this workflow out of the Sentry 500 path while preserving unknown errors.</comment>
<file context>
@@ -748,7 +749,17 @@ export const createCreateProcedures = () => {
+ message: error.message,
+ });
+ }
+ throw error;
+ }
}
</file context>
| if (errno === "ENOSPC" || errno === "EACCES" || errno === "EPERM") { | ||
| throw new TRPCError({ | ||
| code: errno === "ENOSPC" ? "PRECONDITION_FAILED" : "FORBIDDEN", | ||
| message: error instanceof Error ? error.message : String(error), |
There was a problem hiding this comment.
P2: UI-state filesystem failures lose their structured errno when translated, so the renderer cannot distinguish the specific environment failure through the documented FS_ERRNO cause contract. Preserving cause: { kind: "FS_ERRNO", errno } here would keep this translation consistent with the filesystem router and retain actionable diagnostics.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/lib/trpc/routers/ui-state/index.ts, line 17:
<comment>UI-state filesystem failures lose their structured errno when translated, so the renderer cannot distinguish the specific environment failure through the documented `FS_ERRNO` cause contract. Preserving `cause: { kind: "FS_ERRNO", errno }` here would keep this translation consistent with the filesystem router and retain actionable diagnostics.</comment>
<file context>
@@ -1,8 +1,25 @@
+ if (errno === "ENOSPC" || errno === "EACCES" || errno === "EPERM") {
+ throw new TRPCError({
+ code: errno === "ENOSPC" ? "PRECONDITION_FAILED" : "FORBIDDEN",
+ message: error instanceof Error ? error.message : String(error),
+ });
+ }
</file context>
| message: error instanceof Error ? error.message : String(error), | |
| message: error instanceof Error ? error.message : String(error), | |
| cause: { kind: "FS_ERRNO", errno }, |
| return result; | ||
| } catch (error) { | ||
| if (error instanceof Error && error.name === "NotGitRepoError") { | ||
| if (error instanceof NotGitRepoError) { |
There was a problem hiding this comment.
P3: The if (error instanceof NotGitRepoError) → BAD_REQUEST / if (error instanceof GitEnvironmentError) → PRECONDITION_FAILED translation is copy-pasted across status.ts (twice), branches.ts, and create.ts (three times), replacing what was previously a single shared translateGitTaskError helper. Keeping the mapping logic in one place (e.g., a small translateGitTaskError(error) that rethrows typed TRPCErrors) would make the not-found / environment mapping consistent and easier to evolve without touching six call sites.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/lib/trpc/routers/changes/status.ts, line 81:
<comment>The `if (error instanceof NotGitRepoError) → BAD_REQUEST` / `if (error instanceof GitEnvironmentError) → PRECONDITION_FAILED` translation is copy-pasted across status.ts (twice), branches.ts, and create.ts (three times), replacing what was previously a single shared `translateGitTaskError` helper. Keeping the mapping logic in one place (e.g., a small `translateGitTaskError(error)` that rethrows typed TRPCErrors) would make the not-found / environment mapping consistent and easier to evolve without touching six call sites.</comment>
<file context>
@@ -67,7 +78,21 @@ export const createStatusRouter = () => {
return result;
} catch (error) {
- translateGitTaskError(error);
+ if (error instanceof NotGitRepoError) {
+ throw new TRPCError({
+ code: "BAD_REQUEST",
</file context>
| * TRPCErrors (message preserved verbatim); anything unrecognized rethrows and | ||
| * stays a reported 500. | ||
| */ | ||
| function translateFsError(error: unknown): never { |
There was a problem hiding this comment.
P3: Filesystem error translation is now maintained independently in the desktop and host-service routers, making fixes and newly supported errno/path codes prone to diverging. A shared host-compatible translator or centralized error-mapping contract would avoid this behavior drift.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/host-service/src/trpc/router/filesystem/filesystem.ts, line 49:
<comment>Filesystem error translation is now maintained independently in the desktop and host-service routers, making fixes and newly supported errno/path codes prone to diverging. A shared host-compatible translator or centralized error-mapping contract would avoid this behavior drift.</comment>
<file context>
@@ -1,16 +1,84 @@
+ * TRPCErrors (message preserved verbatim); anything unrecognized rethrows and
+ * stays a reported 500.
+ */
+function translateFsError(error: unknown): never {
+ if (error instanceof WorkspaceFsPathError) {
+ throw new TRPCError({
</file context>
- worktreeExists no longer counts git's stale `prunable` metadata entries as live worktrees, so open paths can't bypass the missing-worktree handling after a directory is deleted out from under git. - getStatusNoLock only classifies the git execution as environment; porcelain parsing moved outside the catch so parser bugs surface as reported 500s instead of hiding behind GitEnvironmentError. - Host-service teardown-script failure is expected and force-retryable: now PRECONDITION_FAILED with the same TEARDOWN_FAILED cause, so the new Sentry middleware doesn't re-create known-500 noise. The indeterminate fail-closed paths on destroy stay 500 deliberately. - Invalid PR URL input is BAD_REQUEST, message unchanged. Claude-Session: https://claude.ai/code/session_012u6VZtfQgoTh8qS4NqCb7e
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts`:
- Around line 854-865: The worktreeExists path-matching logic currently relies
on newline-delimited porcelain records, so paths containing newlines are parsed
incorrectly. Update the Git worktree listing command and parsing in
worktreeExists to use NUL-delimited output with the -z option, preserving the
existing live-versus-prunable filtering and exact path matching.
- Around line 241-243: Update parsePorcelainStatusV2 to throw when a known
porcelain v2 record type (1, 2, or u) is malformed or unmatched, allowing
getStatusNoLock to surface the failure. Preserve -z pathnames verbatim, and
parse 2 rename records using the NUL separator between target and original paths
rather than a trailing field after the current path. Add tests covering
malformed records and filenames containing newlines.
🪄 Autofix
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 Plus
Run ID: 14901db6-e521-478b-b876-8f1635208ee1
📒 Files selected for processing (3)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/git.tspackages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts
| // Parsing failures are our bugs, not the environment's — keep them outside | ||
| // the catch so they surface as reported 500s. | ||
| return parsePorcelainStatusV2(stdout); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg '^apps/desktop/src/lib/trpc/routers/workspaces/utils/git\.ts$' || true
echo "== outline =="
ast-grep outline apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts --view compact || true
echo "== relevant parser section =="
sed -n '200,320p' apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts
echo "== references to parsePorcelainStatusV2 =="
rg -n "parsePorcelainStatusV2|parse.*Porcelain|PorcelainStatus|status.*porcelain|getStatusNoLock" apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts apps/desktop/src -g '*.ts' -g '*.tsx' | head -200
echo "== deterministic JS behavior probe for relevant parser candidates =="
tmpdir=$(mktemp -d)
python3 - <<'PY' > "$tmpdir/inspects.py"
import re, pathlib
p = pathlib.Path("apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts")
text = p.read_text()
for needle in ["(.*?)", "(.+)", "(?: |\\Z)", "records\\.", "console\\.", "throw new Error"]:
print(needle)
PY
python3 "$tmpdir/inspects.py"Repository: superset-sh/superset
Length of output: 6785
🌐 Web query:
Git git-status porcelain format 2 output status --porcelain=v2 NUL terminated pathnames
💡 Result:
When running git status --porcelain=v2, you can use the -z flag to enable NUL-terminated pathnames [1]. In the v2 porcelain format, when the -z flag is used, pathnames are printed as-is without quoting, and pathnames are separated by a NUL (ASCII 0x00) byte [2][3]. For entries involving renames (e.g., copied or renamed files), the two pathnames are separated by a NUL byte [2][4]. Additionally, in this mode, each entry line is terminated with a NUL byte [2][3]. This differs from the default behavior without -z, where pathnames with unusual characters may be quoted according to the core.quotePath configuration, and fields/lines are separated by spaces or newlines [2][3]. Note that for reliable machine parsing, you may also want to ensure consistent path behavior regardless of the user's current directory by setting the status.relativePaths configuration to false, as porcelain v2 is affected by this configuration variable [5]. You can do this by running: git -c status.relativepaths=no status --porcelain=v2 -z [5]
Citations:
- 1: https://public-inbox.org/git/1471016692-35828-10-git-send-email-git@jeffhostetler.com/
- 2: https://git-scm.com/docs/git-status
- 3: https://man7.org/linux/man-pages/man1/git-status.1.html
- 4: https://git.github.io/htmldocs/git-status.html
- 5: http://public-inbox.org/git/CAHCc5YV2Bn5NOfZvdSu6c=nERizmKrPbmGoFWkB=T58BjQ=9UQ@mail.gmail.com/T/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== full parsePorcelainStatusV2 section =="
sed -n '250,430p' apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts
echo "== relevant tests =="
sed -n '630,830p' apps/desktop/src/lib/trpc/routers/workspaces/utils/git.test.ts
echo "== focused parser patterns =="
python3 - <<'PY'
import re, pathlib
p = pathlib.Path("apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts")
text = p.read_text()
start = text.index("export function parsePorcelainStatusV2")
end = text.index("const worktreePath", start)
body = text[start:end]
patterns = [
r"\n\s*(1|2|u)\s+",
r"const match = .*?matches\(([^\)]+)\)",
r"for .*?name.*?:.*?matches",
r"records\.",
r"console\.",
r"throw new Error",
]
for pat in patterns:
print(f"\n--- {pat!r} ---")
for sec in ("header", body):
if re.search(pat, sec, re.S):
print(f"FOUND in {('body' if sec==body else 'full')}")
print(re.search(pat, sec, re.S).group(0).replace("\n", "\n")[:500])
PY
echo "== node parser behavior probe =="
node - <<'JS'
function parseRecord(entry) {
if (/^(1|2|u)\s/.test(entry)) {
const match = entry.match(/^(1|2|u)\s+(.*?)\s+(.*?)\s+\((.+)\)/);
if (match) return { matched: true, fields: match.slice(1) };
// Current code silently skips with return here? Use placeholder.
return { skipped: true, recordType: entry.charAt(0), length: entry.length };
}
return { skipped: true };
}
const cases = [
["no newline", "1 M M \0a/b/file.txt\0"],
["newline unquoted", "1 M M \0a/b/file\nnewline.txt\0"],
["malformed line enough data", "1 M M \0a/b/file.txt\0"],
];
for (const [name, input] of cases) {
for (const entry of input.split("\0").filter(Boolean)) {
console.log(name, JSON.stringify(entry), JSON.stringify(parseRecord(entry)));
}
}
JSRepository: superset-sh/superset
Length of output: 10656
🌐 Web query:
Git status porcelain v2 format reference unmerged records line 2 rename records line 1 fields
💡 Result:
The Git status porcelain v2 format provides a stable, machine-readable output for scripts [1][2]. Unmerged Records Unmerged entries are identified by the character "u" as the first field [3][4]. The format for an unmerged record is: u
[3][4] Fields for unmerged records: - : A 2-character field describing the conflict type [3][4]. - : A 4-character field describing the submodule state [3][4]. - , , : Octal file modes for stages 1, 2, and 3 [3][4]. - : Octal file mode in the worktree [3][4]. - ,
,
: Object names (SHA-1) for stages 1, 2, and 3 [3][4]. - : The pathname [3][4]. Rename/Copy Records Renamed or copied entries are identified by the character "2" as the first field [1][5]. The format for a rename/copy record is: 2 [1][5] Fields for rename/copy records (specifically the rename/copy fields): - : A 2-character field containing the staged and unstaged XY values; unchanged values are indicated by a "." instead of a space [1][3]. - : A 4-character field describing the submodule state (e.g., commit changed, tracked changes, untracked changes) [1][6]. - , , : Octal file modes for HEAD, the index, and the worktree [7][6]. - , : Object names for HEAD and the index [7][6]. - : A field indicating the type of change (R for rename, C for copy) followed by the percentage of similarity (e.g., "R100", "C75") [1][7]. - : The current pathname [1][7]. - : A separator character (TAB by default, or NUL if the -z option is used) [1][8]. - : The original pathname in the commit at HEAD [1][7].
Citations:
- 1: https://git-scm.com/docs/git-status
- 2: https://git-scm.com/docs/git-status/2.44.0
- 3: https://mirrors.edge.kernel.org/pub/software/scm/git/docs/git-status.html
- 4: git/git@1cd828d
- 5: https://github.com/git/git/blob/b271a3034fc54a7c84ae5803d72f14a29b172c78/Documentation/git-status.txt
- 6: https://public-inbox.org/git/1470434434-64283-8-git-send-email-git@jeffhostetler.com/
- 7: https://github.com/git/git/blob/5e83cca0b83b4a915f9ed246c2bcfeeb5a58b86b/Documentation/git-status.txt
- 8: https://www.kernel.org/pub/software/scm/git/docs/git-status.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== node parser behavior probe =="
node - <<'JS'
function currentParseRecord(entry, i, entries) {
if (entry.startsWith("1 ")) {
const match = entry.match(/^1 (\S{2}) \S+ \S+ \S+ \S+ \S+ \S+ (.+)$/);
return { matched: !!match, fields: match ? match.slice(1) : undefined };
}
if (entry.startsWith("2 ")) {
const match = entry.match(/^2 (\S{2}) \S+ \S+ \S+ \S+ \S+ \S+ \S+ (.+)$/);
const from = entries[i + 1];
return { matched: !!match, fields: match ? match.slice(1) : undefined, skip: from ? 2 : 1 };
}
if (entry.startsWith("u ")) {
const match = entry.match(/^u (\S{2}) \S+ \S+ \S+ \S+ \S+ \S+ \S+ \S+ (.+)$/);
return { matched: !!match, fields: match ? match.slice(1) : undefined };
}
return { matched: false };
}
const cases = [
["no newline", "1\tXY\tsub\tmH\tmI\tmW\thH\thI\tscore\tpath.txt"],
["newline current path before score", "2\tXY\tsub\tmH\tmI\tmW\thH\thI\tR100\tpath\nnewline.txt\to\0"],
["malformed enough line", "1 M M \0a\tc\tmW\thH\thI\tscore\tb.txt\to\0"],
];
for (const [name, input] of cases) {
const entries = input.split("\0").filter(Boolean);
for (let k=0; k<entries.length; k++) {
const entry = entries[k];
try {
console.log(name, entry.length, JSON.stringify(entry.replace(/\n/g, "\\n").replace(/\t/g, "\\t")), JSON.stringify(currentParseRecord(entry, k, entries)));
} catch (e) {
console.log(name, "ERROR", e.message);
}
}
}
JS
Repository: superset-sh/superset
Length of output: 547
🌐 Web query:
git status porcelain v2 "2 " record format path sep origPath NUL
💡 Result:
In Git, the --porcelain=v2 output format for renamed or copied entries includes a field to distinguish between the path and the original path [1][2]. The format for these entries is: 2 [2][3] The field behaves according to the following rules: 1. If the -z option is used, is a NUL (ASCII 0x00) byte [1][2]. 2. If the -z option is NOT used, is a TAB (ASCII 0x09) byte [1][2]. The represents the current target path, while represents the path in the commit at HEAD where the contents originated [2][4]. When -z is present, pathnames are printed without quoting, and lines are terminated with a NUL (ASCII 0x00) byte [2][5]. Without -z, pathnames containing special characters (tab, linefeed, double quote, or backslash) are "C-quoted" [2][3].
Citations:
- 1: https://git-scm.com/docs/git-status
- 2: git/git@1cd828d
- 3: https://public-inbox.org/git/1470434434-64283-8-git-send-email-git@jeffhostetler.com/
- 4: https://github.com/git/git/blob/cada7308ade0b9a018f9c54ea3f226e2e9c39ee5/Documentation/git-status.txt
- 5: https://mirrors.edge.kernel.org/pub/software/scm/git/docs/git-status.html
Citations:
- 1: https://git-scm.com/docs/git-status
- 2: https://git-scm.com/docs/git-status/2.44.0
- 3: https://mirrors.edge.kernel.org/pub/software/scm/git/docs/git-status.html
- 4: git/git@1cd828d
- 5: https://github.com/git/git/blob/b271a3034fc54a7c84ae5803d72f14a29b172c78/Documentation/git-status.txt
- 6: https://public-inbox.org/git/1470434434-64283-8-git-send-email-git@jeffhostetler.com/
- 7: https://github.com/git/git/blob/5e83cca0b83b4a915f9ed246c2bcfeeb5a58b86b/Documentation/git-status.txt
- 8: https://www.kernel.org/pub/software/scm/git/docs/git-status.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== node parser behavior probe =="
node - <<'JS'
function currentParseRecord(entry, i, entries) {
if (entry.startsWith("1 ")) {
const match = entry.match(/^1 (\S{2}) \S+ \S+ \S+ \S+ \S+ \S+ (.+)$/);
return { matched: !!match, fields: match ? match.slice(1) : undefined };
}
if (entry.startsWith("2 ")) {
const match = entry.match(/^2 (\S{2}) \S+ \S+ \S+ \S+ \S+ \S+ \S+ (.+)$/);
const from = entries[i + 1];
return { matched: !!match, fields: match ? match.slice(1) : undefined, skip: from ? 2 : 1 };
}
if (entry.startsWith("u ")) {
const match = entry.match(/^u (\S{2}) \S+ \S+ \S+ \S+ \S+ \S+ \S+ \S+ (.+)$/);
return { matched: !!match, fields: match ? match.slice(1) : undefined };
}
return { matched: false };
}
const cases = [
["no newline", "1\tXY\tsub\tmH\tmI\tmW\thH\thI\tscore\tpath.txt"],
["newline current path before score", "2\tXY\tsub\tmH\tmI\tmW\thH\thI\tR100\tpath\nnewline.txt\to\0"],
["malformed enough line", "1 M M \0a\tc\tmW\thH\thI\tscore\tb.txt\to\0"],
];
for (const [name, input] of cases) {
const entries = input.split("\0").filter(Boolean);
for (let k=0; k<entries.length; k++) {
const entry = entries[k];
try {
console.log(name, entry.length, JSON.stringify(entry.replace(/\n/g, "\\n").replace(/\t/g, "\\t")), JSON.stringify(currentParseRecord(entry, k, entries)));
} catch (e) {
console.log(name, "ERROR", e.message);
}
}
}
JSRepository: superset-sh/superset
Length of output: 547
🌐 Web query:
git status porcelain v2 "2 " record format path sep origPath NUL
💡 Result:
In Git, the --porcelain=v2 output format for renamed or copied entries includes a field to distinguish between the path and the original path [1][2]. The format for these entries is: 2 [2][3] The field behaves according to the following rules: 1. If the -z option is used, is a NUL (ASCII 0x00) byte [1][2]. 2. If the -z option is NOT used, is a TAB (ASCII 0x09) byte [1][2]. The represents the current target path, while represents the path in the commit at HEAD where the contents originated [2][4]. When -z is present, pathnames are printed without quoting, and lines are terminated with a NUL (ASCII 0x00) byte [2][5]. Without -z, pathnames containing special characters (tab, linefeed, double quote, or backslash) are "C-quoted" [2][3].
Citations:
- 1: https://git-scm.com/docs/git-status
- 2: git/git@1cd828d
- 3: https://public-inbox.org/git/1470434434-64283-8-git-send-email-git@jeffhostetler.com/
- 4: https://github.com/git/git/blob/cada7308ade0b9a018f9c54ea3f226e2e9c39ee5/Documentation/git-status.txt
- 5: https://mirrors.edge.kernel.org/pub/software/scm/git/docs/git-status.html
Fail closed when parsePorcelainStatusV2 cannot parse a known porcelain record.
A malformed 1, 2, or u entry currently is skipped, while getStatusNoLock parses outside try with the intent to report 500s. Throw for unmatched known records. Also keep this parser aligned with Git: -z pathnames are emitted verbatim, and 2 rename records use a NUL separator between target and original path, not a final field after the current path. Add tests for malformed records and newline-containing filenames.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile, spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts` around lines 241 -
243, Update parsePorcelainStatusV2 to throw when a known porcelain v2 record
type (1, 2, or u) is malformed or unmatched, allowing getStatusNoLock to surface
the failure. Preserve -z pathnames verbatim, and parse 2 rename records using
the NUL separator between target and original paths rather than a trailing field
after the current path. Add tests covering malformed records and filenames
containing newlines.
| // Git keeps deleted worktrees in its metadata as `prunable` entries, so a | ||
| // listed path is not proof the worktree is live on disk. | ||
| const worktreePrefix = `worktree ${worktreePath}`; | ||
| return lines.some((line) => line.trim() === worktreePrefix); | ||
| return worktrees.split("\n\n").some((block) => { | ||
| const lines = block.split("\n").map((line) => line.trim()); | ||
| return ( | ||
| lines.some((line) => line === worktreePrefix) && | ||
| !lines.some( | ||
| (line) => line === "prunable" || line.startsWith("prunable "), | ||
| ) | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== Locate file =="
git ls-files | rg 'apps/desktop/src/lib/trpc/routers/workspaces/utils/git\.ts$' || true
echo "== Inspect relevant function and calls =="
sed -n '800,890p' apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts
echo "== Search worktreeExists and worktree list invocations =="
rg -n "worktreeExists|worktree list|worktree.*porcelain|Worktree" apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts apps/desktop/src/lib/trpc/routers/workspaces -S || true
echo "== Deterministic probe for newline worktree path logic =="
python3 - <<'PY'
text = """# worktree /tmp/wt\n
path /tmp/wt
prunable
# worktree /tmp/wt
\n
path /tmp/wt\n
"""
def worktree_exists(worktrees, worktree_path):
worktreePrefix = f"worktree {worktree_path}"
return any(
any(line == worktreePrefix for line in block.split("\n").map(str.strip))
and not any(line == "prunable" or line.startswith("prunable ") for line in block.split("\n").map(str.strip))
for block in worktrees.split("\n\n")
if block
)
live_with_embedded = "# worktree /tmp/wt\n\npath /tmp/weird\n\nnew line\n"
print("live_with_newline_path_parsed_by_current_code:")
try:
print(worktree_exists(live_with_embedded, "/tmp/wt"))
except Exception as e:
print(type(e).__name__, str(e))
PYRepository: superset-sh/superset
Length of output: 50376
🌐 Web query:
Git worktree list --porcelain -z paths with newlines documentation
💡 Result:
When using git worktree list --porcelain, you can handle paths (or other attributes) that contain newline characters by using the -z option [1][2]. By default, the --porcelain output terminates each line with a newline character [1][3]. If a path or a lock reason contains unusual characters like a newline, it may be escaped and quoted according to the core.quotePath configuration [1][4]. To ensure the output is reliably machine-readable regardless of the characters within the paths, you should use: git worktree list --porcelain -z When -z is specified, each line of the output is terminated with a NUL character (ASCII 0) instead of a newline [1][2]. This allows scripts to correctly parse worktree attributes even if they contain newlines [1][2].
Citations:
- 1: https://git-scm.com/docs/git-worktree
- 2: https://www.kernel.org/pub/software/scm/git/docs/git-worktree.html
- 3: https://github.com/git/git/blob/d0a1cbccab54d828be2d83acc73f38c162962966/Documentation/git-worktree.txt
- 4: https://github.com/git/git/blob/13fa77b6899d0068303761c449d9a7e01dd85eb8/Documentation/git-worktree.txt
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== inspect related worktree parsing functions =="
sed -n '100,165p' apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts
sed -n '879,960p' apps/desktop/src/lib/trpc/routers workspaces/utils/git.ts 2>/dev/null || sed -n '879,960p' apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts
echo "== deterministic newline-split parser behavior =="
python3 - <<'PY'
def worktree_exists(worktrees, worktree_path):
worktreePrefix = f"worktree {worktree_path}"
blocks = worktrees.split("\n\n")
print("blocks:", repr(blocks))
result = any(
any(line == worktreePrefix for line in (line.strip() for line in block.split("\n")))
and not any(line == "prunable" or line.startswith("prunable ") for line in (line.strip() for line in block.split("\n")))
for block in blocks
)
print("record matching:", repr(worktreePrefix), "found=", result)
return result
live_with_embedded_newline = "# worktree /tmp/wt\na\n\npath /tmp/path with\nnewline\n\nbranch HEAD\n\n"
print(worktree_exists(live_with_embedded_newline, "/tmp/wt"))
PYRepository: superset-sh/superset
Length of output: 4341
Use Git NUL-delimited worktree output for path matching.
worktreeExists parses git worktree list --porcelain with split("\n\n"), so a live worktree path containing a newline can split across record lines and fail to match worktreePrefix. Run git worktree list --porcelain -z and parse NUL-delimited records, or reject worktree paths containing newlines before this check.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile, spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts` around lines 854 -
865, The worktreeExists path-matching logic currently relies on
newline-delimited porcelain records, so paths containing newlines are parsed
incorrectly. Update the Git worktree listing command and parsing in
worktreeExists to use NUL-delimited output with the -z option, preserving the
existing live-versus-prunable filtering and exact path matching.
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts:243">
P2: parsePorcelainStatusV2 should throw on unmatched/malformed porcelain records rather than silently skipping them. Since parsing was just moved outside the try/catch specifically so parsing bugs surface as 500s, a parser that swallows unrecognized entries defeats that intent and can mask real status-parsing failures.</violation>
<violation number="2" location="apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts:857">
P2: worktreeExists parses `git worktree list --porcelain` output by splitting on blank lines (`split("\n\n")`), but worktree paths containing embedded newlines can span across the double-newline record boundary and fail to match `worktreePrefix`, causing a false negative for an existing worktree. Consider using `git worktree list --porcelain -z` and parsing NUL-delimited records instead.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
|
||
| // Parsing failures are our bugs, not the environment's — keep them outside | ||
| // the catch so they surface as reported 500s. | ||
| return parsePorcelainStatusV2(stdout); |
There was a problem hiding this comment.
P2: parsePorcelainStatusV2 should throw on unmatched/malformed porcelain records rather than silently skipping them. Since parsing was just moved outside the try/catch specifically so parsing bugs surface as 500s, a parser that swallows unrecognized entries defeats that intent and can mask real status-parsing failures.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts, line 243:
<comment>parsePorcelainStatusV2 should throw on unmatched/malformed porcelain records rather than silently skipping them. Since parsing was just moved outside the try/catch specifically so parsing bugs surface as 500s, a parser that swallows unrecognized entries defeats that intent and can mask real status-parsing failures.</comment>
<file context>
@@ -238,6 +237,10 @@ export async function getStatusNoLock(repoPath: string): Promise<StatusResult> {
+
+ // Parsing failures are our bugs, not the environment's — keep them outside
+ // the catch so they surface as reported 500s.
+ return parsePorcelainStatusV2(stdout);
}
</file context>
| // listed path is not proof the worktree is live on disk. | ||
| const worktreePrefix = `worktree ${worktreePath}`; | ||
| return lines.some((line) => line.trim() === worktreePrefix); | ||
| return worktrees.split("\n\n").some((block) => { |
There was a problem hiding this comment.
P2: worktreeExists parses git worktree list --porcelain output by splitting on blank lines (split("\n\n")), but worktree paths containing embedded newlines can span across the double-newline record boundary and fail to match worktreePrefix, causing a false negative for an existing worktree. Consider using git worktree list --porcelain -z and parsing NUL-delimited records instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts, line 857:
<comment>worktreeExists parses `git worktree list --porcelain` output by splitting on blank lines (`split("\n\n")`), but worktree paths containing embedded newlines can span across the double-newline record boundary and fail to match `worktreePrefix`, causing a false negative for an existing worktree. Consider using `git worktree list --porcelain -z` and parsing NUL-delimited records instead.</comment>
<file context>
@@ -848,9 +851,18 @@ export async function worktreeExists(
+ // listed path is not proof the worktree is live on disk.
const worktreePrefix = `worktree ${worktreePath}`;
- return lines.some((line) => line.trim() === worktreePrefix);
+ return worktrees.split("\n\n").some((block) => {
+ const lines = block.split("\n").map((line) => line.trim());
+ return (
</file context>
Why
Re-enabling the desktop Sentry key exposed ~140 events/hour that were never bugs: deleted worktrees, home-directories-registered-as-repos, vanished files — all thrown as generic
Errors, escaping tRPC asINTERNAL_SERVER_ERROR, and reported. The top issue alone accumulated 359k events, and it was hiding a real product bug: the changes pane polls a permanently-gone worktree every 2.5s forever, spawning doomed git processes. An earlier attempt to fix this with name/errno allowlists in the Sentry middleware was rejected as classification-at-a-distance.The contract
TRPCErrors translated where context lives. Third-party/OS errors get translated once, at the adapter that calls them.INTERNAL_SERVER_ERROR⇒ report, zero allowlists. A 500 now means a missed translation at a throw site — fix the router, never add a filter.What changed
Desktop main
getStatusNoLockfailures (maxBuffer/permissions/timeouts), and git worker timeouts becomeGitEnvironmentError(name-checked across the worker boundary);ensureWithinRootthrowsWorkspaceFsPathErrorlike its sibling guards.path-validation.tsthrows properly-codedTRPCErrors directly (UNREGISTERED_WORKTREE→404 etc.) — kills the deleted-workspace 500s at all ~33 call sites.changes.getStatus/getBranchespre-check worktree existence (pathExistsCached) before spawning git tasks — no more doomed git spawns — and translateNotGitRepoError/GitEnvironmentErrorwith causes.ENOENT→404,EISDIR→400,EACCES→403,ENOSPC→412; messages verbatim).NOT_FOUND; git lock contention, branch-already-checked-out, gh-not-installed/authed →PRECONDITION_FAILED; input validation →BAD_REQUEST; ui-state ENOSPC + browser-historySQLITE_FULLtyped. Catch-all "unknown failure" wrappers deliberately stay 500.errorFormatterserializes plain-object{kind: ...}causes to the renderer (host-service pattern; verified the cause survives tRPC'sUnknownCauseErrorwrapping).Renderer
useGitChangesStatussurfaceserror/errorCause; polling backs off to 30s on deterministic codes (400/403/404/412) and 10s on transient failures —refetchOnWindowFocusstill gives instant recovery. Changes pane renders "Workspace folder no longer exists on disk" / "not a git repository" / "Git can't read this workspace" instead of a catch-all.useFileContentbranches on theFS_ERRNOcause instead ofmessage.includes("EISDIR").TRPCClientErrorevents — the main-process middleware is the reporting source of truth.Host-service (parity — previously reported nothing)
unhandledRejectionnow captured; errno translation in its filesystem router; typedWorkspaceNotFoundError/ProjectNotFoundErrorreplace string-prefix matching.Verification
bun run lintclean; typecheck clean (desktop, host-service, workspace-fs).useGitChangesStatus.test.ts), 321 host-service, 58 workspace-fs — all pass.After ship (ops)
Takes effect as the fleet auto-updates to the next desktop release. Watch the desktop Sentry project: getStatus/getBranches clusters should go to ~0; host-service will report real 500s for the first time (expect a week-one triage wave). Any 500 carrying
NotGitRepoError/GitEnvironmentError/WorkspaceFsPathError= missed translation arm to fix at the source.https://claude.ai/code/session_012u6VZtfQgoTh8qS4NqCb7e
Summary by cubic
Translate expected desktop and host-service errors at throw sites into typed non-500
TRPCErrors with machine-readable causes, and keep the Sentry boundary “dumb” so only real 500s are reported. This cuts noisy events, stops endless git spawns on missing worktrees, and shows clear UI with smarter polling backoff.Bug Fixes
status/branches(and nowgetCommitFiles) and mapNotGitRepoError/GitEnvironmentErrorto 400/412; worker now translates raw simple-git “not a git repository” intoNotGitRepoErrorbefore crossing the boundary.worktreeExistsignores gitprunableentries;getStatusNoLockclassifies exec/timeout/maxBuffer as environment while porcelain parse failures surface as true 500s.ENOENT/EISDIR/ENOTDIR/EACCES/EPERM/ENOSPCand path-guard failures to non-500s withFS_ERRNO/PATH_VALIDATIONcauses; browser historySQLITE_FULLand UI-state ENOSPC/EACCES/EPERM return typed errors.unhandledRejectionis captured; Sentry middleware reports only 500s.FS_ERRNO; renderer Sentry drops duplicateTRPCClientErrors.Refactors
instanceofchecks. Added anerrorFormatterto serialize kind-discriminated causes to the renderer.Written for commit 0c2ba78. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Performance
Monitoring