Repository navigation
fix(desktop): classify pull/sync conflicts and rejected pushes as typed non-500s - #6259
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughGit operations now classify expected merge, rebase, push, and upstream failures. Pull and sync convert recognized failures into typed ChangesGit error handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Sync
participant GitOperations
participant Git
participant TRPC
Sync->>GitOperations: Execute pull and push
GitOperations->>Git: Run Git operation
Git-->>GitOperations: Return success or expected failure
GitOperations->>TRPC: Convert expected failure to PRECONDITION_FAILED
GitOperations->>Sync: Refresh branch status and clear caches
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/changes/git-operations.ts`:
- Around line 65-70: In the isUpstreamMissingError(message) branch, keep kind
set to "UPSTREAM_MISSING" but return the original message value unchanged
instead of replacing it with fixed text, so Git’s message remains available to
the UI.
🪄 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: ba5b42c1-a601-4c80-b16e-673a1d237287
📒 Files selected for processing (3)
apps/desktop/src/lib/trpc/routers/changes/git-operations.test.tsapps/desktop/src/lib/trpc/routers/changes/git-operations.tsapps/desktop/src/lib/trpc/routers/changes/git-utils.ts
Comment on lines
+65
to
+70
| if (isUpstreamMissingError(message)) { | ||
| return { | ||
| kind: "UPSTREAM_MISSING", | ||
| message: | ||
| "No upstream branch to pull from. The remote branch may have been deleted.", | ||
| }; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the original upstream error message.
Line 69 replaces Git's message with a fixed message. A direct pull with a missing upstream will no longer expose Git's original message to the UI. Keep message unchanged and use kind: "UPSTREAM_MISSING" for classification.
Based on PR objectives: “Git’s original message is preserved for the UI.”
Proposed fix
if (isUpstreamMissingError(message)) {
return {
kind: "UPSTREAM_MISSING",
- message:
- "No upstream branch to pull from. The remote branch may have been deleted.",
+ message,
};
}📝 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
| if (isUpstreamMissingError(message)) { | |
| return { | |
| kind: "UPSTREAM_MISSING", | |
| message: | |
| "No upstream branch to pull from. The remote branch may have been deleted.", | |
| }; | |
| if (isUpstreamMissingError(message)) { | |
| return { | |
| kind: "UPSTREAM_MISSING", | |
| message, | |
| }; |
🤖 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/changes/git-operations.ts` around lines 65
- 70, In the isUpstreamMissingError(message) branch, keep kind set to
"UPSTREAM_MISSING" but return the original message value unchanged instead of
replacing it with fixed text, so Git’s message remains available to the UI.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 3 files
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/git-operations.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/changes/git-operations.ts:68">
P2: The UPSTREAM_MISSING classification in `classifyExpectedGitFailure` substitutes a hardcoded pull-specific message ("No upstream branch to pull from...") and drops the original git error text, and `rethrowClassifiedGitError` is now also applied to the sync push legs (`pushCurrentBranch` and the `pushWithResolvedUpstream` fallback). When a push fails with an upstream-missing signature (e.g. "couldn't find remote ref" / "cannot be resolved to branch" when the remote ref is gone), the user performing a push is shown a message that says "pull from" — the wrong operation — and the specific ref/branch context from the original error is discarded. Previously such push failures were rethrown with their raw git message. Consider using an operation-aware message for push legs (or preserving the original message/context, e.g. `cause: { kind, originalError/message }`) so the classification doesn't mislead users or hide which ref is missing.</violation>
<violation number="2" location="apps/desktop/src/lib/trpc/routers/changes/git-operations.ts:88">
P2: The new typed causes are dropped by the desktop changes error contract, so the renderer cannot obtain a typed conflict, rejection, or missing-upstream state from these errors. Extending `GitChangesErrorCause` and `GIT_CHANGES_CAUSE_KINDS` (plus the corresponding UI copy handling) would keep this classification usable.</violation>
</file>
Architecture diagram
sequenceDiagram
participant UI as Desktop UI
participant TRPC as TRPC Router
participant GitOp as git-operations.ts
participant GitUtils as git-utils.ts
participant SimpleGit as simple-git
Note over UI,GitOp: Pull/Sync Flow with Error Classification
UI->>TRPC: pull() or sync()
TRPC->>GitOp: call handler
alt Pull flow
GitOp->>SimpleGit: git.pull(["--rebase"])
alt Pull succeeds
SimpleGit-->>GitOp: success
GitOp-->>TRPC: { success: true }
TRPC-->>UI: success response
else Pull fails
SimpleGit-->>GitOp: error message
GitOp->>GitUtils: isMergeConflictError(message)
alt Merge conflict detected
GitUtils-->>GitOp: true
GitOp->>GitOp: rethrowClassifiedGitError()
GitOp-->>TRPC: TRPCError PRECONDITION_FAILED (MERGE_CONFLICT)
TRPC-->>UI: typed error, not 500
else Not merge conflict
GitOp->>GitUtils: isNonFastForwardPushError(message)
alt Push rejected
GitUtils-->>GitOp: true
GitOp->>GitOp: rethrowClassifiedGitError()
GitOp-->>TRPC: TRPCError PRECONDITION_FAILED (PUSH_REJECTED)
TRPC-->>UI: typed error, not 500
else Not push rejected
GitOp->>GitUtils: isUpstreamMissingError(message)
alt Upstream missing
GitUtils-->>GitOp: true
GitOp->>GitOp: rethrowClassifiedGitError()
GitOp-->>TRPC: TRPCError PRECONDITION_FAILED (UPSTREAM_MISSING)
TRPC-->>UI: typed error, not 500
else Unknown git error
GitOp->>GitOp: throw error (unclassified)
GitOp-->>TRPC: 500 Internal Server Error
TRPC-->>UI: 500 error (reported to Sentry)
end
end
end
end
else Sync flow
GitOp->>SimpleGit: pushCurrentBranch()
alt Push succeeds
SimpleGit-->>GitOp: success
GitOp->>SimpleGit: fetchCurrentBranch()
GitOp-->>TRPC: { success: true }
else Push fails
SimpleGit-->>GitOp: error message
GitOp->>GitUtils: isUpstreamMissingError(message)
alt Upstream missing
GitUtils-->>GitOp: true
GitOp->>SimpleGit: pushWithResolvedUpstream()
alt Push succeeds
SimpleGit-->>GitOp: success
GitOp->>SimpleGit: fetchCurrentBranch()
GitOp-->>TRPC: { success: true }
else Push fails
SimpleGit-->>GitOp: error message
GitOp->>GitOp: rethrowClassifiedGitError()
GitOp-->>TRPC: TRPCError PRECONDITION_FAILED
TRPC-->>UI: typed error, not 500
end
else Other error
GitOp->>GitOp: rethrowClassifiedGitError()
alt Classified as PUSH_REJECTED
GitOp-->>TRPC: TRPCError PRECONDITION_FAILED
TRPC-->>UI: typed error, not 500
else Unclassified
GitOp-->>TRPC: 500 Internal Server Error
TRPC-->>UI: 500 error (reported to Sentry)
end
end
end
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| throw new TRPCError({ | ||
| code: "PRECONDITION_FAILED", | ||
| message: expected.message, | ||
| cause: { kind: expected.kind }, |
Contributor
There was a problem hiding this comment.
P2: The new typed causes are dropped by the desktop changes error contract, so the renderer cannot obtain a typed conflict, rejection, or missing-upstream state from these errors. Extending GitChangesErrorCause and GIT_CHANGES_CAUSE_KINDS (plus the corresponding UI copy handling) would keep this classification usable.
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/git-operations.ts, line 88:
<comment>The new typed causes are dropped by the desktop changes error contract, so the renderer cannot obtain a typed conflict, rejection, or missing-upstream state from these errors. Extending `GitChangesErrorCause` and `GIT_CHANGES_CAUSE_KINDS` (plus the corresponding UI copy handling) would keep this classification usable.</comment>
<file context>
@@ -47,6 +48,49 @@ async function getLocalBranchOrThrow({
+ throw new TRPCError({
+ code: "PRECONDITION_FAILED",
+ message: expected.message,
+ cause: { kind: expected.kind },
+ });
+ }
</file context>
| if (isUpstreamMissingError(message)) { | ||
| return { | ||
| kind: "UPSTREAM_MISSING", | ||
| message: |
Contributor
There was a problem hiding this comment.
P2: The UPSTREAM_MISSING classification in classifyExpectedGitFailure substitutes a hardcoded pull-specific message ("No upstream branch to pull from...") and drops the original git error text, and rethrowClassifiedGitError is now also applied to the sync push legs (pushCurrentBranch and the pushWithResolvedUpstream fallback). When a push fails with an upstream-missing signature (e.g. "couldn't find remote ref" / "cannot be resolved to branch" when the remote ref is gone), the user performing a push is shown a message that says "pull from" — the wrong operation — and the specific ref/branch context from the original error is discarded. Previously such push failures were rethrown with their raw git message. Consider using an operation-aware message for push legs (or preserving the original message/context, e.g. cause: { kind, originalError/message }) so the classification doesn't mislead users or hide which ref is missing.
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/git-operations.ts, line 68:
<comment>The UPSTREAM_MISSING classification in `classifyExpectedGitFailure` substitutes a hardcoded pull-specific message ("No upstream branch to pull from...") and drops the original git error text, and `rethrowClassifiedGitError` is now also applied to the sync push legs (`pushCurrentBranch` and the `pushWithResolvedUpstream` fallback). When a push fails with an upstream-missing signature (e.g. "couldn't find remote ref" / "cannot be resolved to branch" when the remote ref is gone), the user performing a push is shown a message that says "pull from" — the wrong operation — and the specific ref/branch context from the original error is discarded. Previously such push failures were rethrown with their raw git message. Consider using an operation-aware message for push legs (or preserving the original message/context, e.g. `cause: { kind, originalError/message }`) so the classification doesn't mislead users or hide which ref is missing.</comment>
<file context>
@@ -47,6 +48,49 @@ async function getLocalBranchOrThrow({
+ if (isUpstreamMissingError(message)) {
+ return {
+ kind: "UPSTREAM_MISSING",
+ message:
+ "No upstream branch to pull from. The remote branch may have been deleted.",
+ };
</file context>
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
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
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.
Problem
Three Sentry issues (1 event each) from the desktop changes router: DESKTOP-M7 (
changes.pull, 1.16.0), DESKTOP-MG (changes.syncpull leg, 1.15.0), DESKTOP-JK (changes.syncpush leg, 1.15.0). All are expected user-repo outcomes — a rebase merge conflict duringgit pull --rebase, and a push rejected by the remote (non-fast-forward / repository rules) — escaping asINTERNAL_SERVER_ERRORand getting reported. Because the raw simple-git error message is git's full stderr transcript, the issue titles also embed remote branch listings and third-party repository URLs.Root cause
changes.pushgot a classification catch in #6190, butchanges.pull's catch only handled the upstream-missing case (and rethrew it as a plainError, itself a reportable 500), rethrowing everything else raw.changes.synccalledpushCurrentBranch/pushWithResolvedUpstreamentirely outside any try, and its pull leg rethrew non-upstream-missing errors raw.Fix
Classification only — no behavior change, no Sentry-side filtering (per the #6164 contract, classify at throw sites):
isMergeConflictErrormatcher ingit-utils.tsalongside the existing matchers.rethrowClassifiedGitErrorhelper ingit-operations.ts: passesTRPCErrorthrough, wraps expected git outcomes inTRPCErrorPRECONDITION_FAILEDwith a plain-object cause and git's original message text preserved for the UI, and rethrows anything unexpected raw so genuine bugs still report as 500s.cause: { kind: "MERGE_CONFLICT" }isNonFastForwardPushError) →cause: { kind: "PUSH_REJECTED" }cause: { kind: "UPSTREAM_MISSING" }, keeping pull's existing "No upstream branch to pull from…" message (previously a plainError)pullroutes its catch through the helper;syncroutes its pull leg and both push paths (pushCurrentBranchand the upstream-missingpushWithResolvedUpstreamfallback) through it.Matchers are message-based (worker boundary strips prototypes, so no
instanceofon git errors). Added unit tests forisMergeConflictError.Verification
bunx biome checkon the three changed files:cd apps/desktop && bun run typecheck:bun test apps/desktop/src/lib/trpc/routers/changes/git-operations.test.ts:Refs DESKTOP-M7
Refs DESKTOP-MG
Refs DESKTOP-JK
https://claude.ai/code/session_0d966f97-3e8a-4239-ae1b-b1c2e539b359
Summary by cubic
Classifies expected git outcomes in the desktop changes router (merge conflicts, rejected pushes, missing upstream) as typed non-500
TRPCErrors to stop false 500s and avoid leaking git stderr details. No functional behavior changes to pull/push/sync behavior.isMergeConflictErroringit-utils.tswith unit tests.rethrowClassifiedGitErrorto map expected failures toPRECONDITION_FAILEDwithcausekinds:MERGE_CONFLICT,PUSH_REJECTED,UPSTREAM_MISSING.pull,syncpull leg,pushCurrentBranch, andpushWithResolvedUpstreamthrough the classifier; unexpected errors still surface as 500s.Written for commit 7437c9c. Summary will update on new commits.