Skip to content

fix(server): name the cause of a failed git command - #8645

Open
walid-baharwal wants to merge 1 commit into
pingdotgg:mainfrom
walid-baharwal:fix/git-error-stderr-excerpt
Open

fix(server): name the cause of a failed git command#8645
walid-baharwal wants to merge 1 commit into
pingdotgg:mainfrom
walid-baharwal:fix/git-error-stderr-excerpt

Conversation

@walid-baharwal

@walid-baharwal walid-baharwal commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

What Changed

GitCommandError gains an optional reason: a closed set of tags for well-known git
failures, recognized from stderr inside the driver. Each tag renders a fixed sentence that
the error's message getter appends after the existing detail.

Before and after, for the same failing command (real git, captured from the driver):

Git command failed in GitVcsDriver.createWorktree (/tmp/…/repo): Git command exited with a non-zero status.
Git command failed in GitVcsDriver.createWorktree (/tmp/…/repo): Git command exited with a non-zero status. That branch is already checked out in another worktree.

Raw stderr still never leaves the driver.

Why

Fixes #4380. Every git precondition failure collapsed into one string, so a tag conflict, an
auth failure and a branch already checked out elsewhere were indistinguishable without
re-running the command by hand.

The issue offers two options. The first — carrying a bounded stderrExcerpt — is the one I
did not take: apps/server/src/vcs/GitVcsDriverCore.test.ts already asserts
notProperty(error, "stderr") and that a secret passed in argv never reaches
error.message, so dropping stderr is deliberate, and shipping an excerpt would mean
deleting a security test. This is the issue's second option, the parsed reason, which keeps
that guarantee intact: the tags are a closed literal union, the sentences are static, and
nothing matched from stderr is ever interpolated. The existing redaction test is extended
with notProperty(error, "reason").

Classification happens at the two shared funnels every git call routes through
(executeGit and the non-zero-exit branch of the raw executor), which covers
fetchRemote and createWorktree — the two operations named in the issue.

Two limits, stated rather than hidden:

  • Classification is English-only. executeGit inherits the process locale, so under a
    non-English LANG git's wording does not match and the error simply keeps today's
    behavior — no reason, no regression. Forcing LC_ALL=C for every git command would fix
    that, but it changes the environment of every call in the driver and belongs in its own
    change. The new tests pin LC_ALL: "C" so they do not depend on the runner's locale.
  • Five other GitCommandError constructions hold stderr in hand and stay unclassified.
    Adding them is one line each and deliberately left out to keep this to one concern.

UI Changes

None. Server-side error metadata; no rendered change. The improved text surfaces wherever
an existing git error message is already shown.

Verification

vp test run apps/server/src/vcs/GitVcsDriverCore.test.ts   # 59 passed
cd packages/contracts && tsgo --noEmit                     # clean
cd apps/server && tsgo --noEmit                            # clean
vp lint apps/server/src/vcs/GitVcsDriverCore.ts apps/server/src/vcs/GitVcsDriverCore.test.ts packages/contracts/src/git.ts
vp format --check <same three files>
git diff --check

The three new tests drive real git through the real driver: a branch already checked out in
another worktree, a command outside a repository, and a tag collision that must stay
unclassified. All three fail without the source change.

Checklist

  • One concern
  • Focused tests, failing before the fix
  • Typecheck, lint and format run on the changed packages
  • No new dependencies, no committed artifacts
  • Additive optional contract field; web, mobile and desktop need no change (no reference
    to GitCommandError fields exists in any client)

Model: Claude Opus 5 (1M context). Harness: Claude Code.


Note

Low Risk
Additive optional contract field and localized error enrichment; stderr redaction behavior is preserved and covered by tests.

Overview
GitCommandError now carries an optional reason tag (closed union in contracts) for common git failures. The VCS driver classifies non-zero exits by matching stderr against ordered regex patterns inside GitVcsDriverCore only—raw stderr still never leaves the driver.

When a reason is set, GitCommandError.message appends a fixed user-facing sentence from gitCommandFailureReasonMessage; callers can also branch on reason (e.g. worktree branch conflict, not a repository, auth, remote unreachable). Unmatched cases (including duplicate tag creation) stay unclassified so tag/ref collisions are not mislabeled as path conflicts.

Integration tests cover worktree add conflicts, commands outside a repo, tag collisions, and extend the redaction test to assert reason is absent when classification does not apply.

Reviewed by Cursor Bugbot for commit 64e5038. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Add reason field to GitCommandError to classify common git failures

  • Introduces GitCommandFailureReason in git.ts: a closed set of reasons (e.g. not_a_repository, branch_checked_out_in_worktree, remote_unreachable) with a user-facing message map.
  • Adds classifyGitFailure in GitVcsDriverCore.ts that scans git stderr against ordered GIT_FAILURE_REASON_PATTERNS and attaches a reason to GitCommandError on non-zero exits.
  • GitCommandError.message now appends an explanatory sentence when reason is set; unmatched errors remain unchanged.
  • Behavioral Change: GitCommandError.message gains an extra sentence for classified failures, and instances now carry an optional reason field that was absent before.
📊 Macroscope summarized 64e5038. 2 files reviewed, 1 issue evaluated, 0 issues filtered, 1 comment posted

🗂️ Filtered Issues

Git states why a command failed on stderr, but stderr is deliberately kept
off GitCommandError — it echoes argv and remote URLs, which can carry
credentials. Callers were left with "git fetch origin failed" and no way to
tell a tag conflict from an auth failure without re-running git by hand.

Match stderr at the driver against a fixed set of well-known failures and
carry the result as a closed set of tags. The message each tag renders is a
static sentence that quotes none of the matched text, so the existing
redaction guarantee is unchanged: stderr still never leaves the driver.

Fixes pingdotgg#4380
@coderabbitai

coderabbitai Bot commented Aug 29, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: d8da3cfe-6b79-4a96-8703-975a15e17367

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Aug 29, 2026
if (options.allowNonZeroExit || result.exitCode === 0) {
return Effect.succeed(result);
}
const reason = classifyGitFailure(result.stderr);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium vcs/GitVcsDriverCore.ts:930

classifyGitFailure(result.stderr) labels hook-generated text as a Git failure reason, so a failing pre-push hook that prints authentication failed receives reason: "authentication_failed" even though no credentials were attempted. Restrict classification to Git diagnostic lines or exclude hook output before assigning a remediation reason.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/vcs/GitVcsDriverCore.ts around line 930:

`classifyGitFailure(result.stderr)` labels hook-generated text as a Git failure reason, so a failing `pre-push` hook that prints `authentication failed` receives `reason: "authentication_failed"` even though no credentials were attempted. Restrict classification to Git diagnostic lines or exclude hook output before assigning a remediation reason.

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 64e5038. Configure here.

return `Git command failed in ${this.operation} (${this.cwd}): ${this.detail}`;
const reason =
this.reason === undefined ? "" : ` ${gitCommandFailureReasonMessage(this.reason)}`;
return `Git command failed in ${this.operation} (${this.cwd}): ${this.detail}${reason}`;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reason text runs into detail

Low Severity

The message getter appends the reason sentence with only a leading space, so it reads as one run-on clause whenever detail does not already end with a period. createWorktree and fetchRemote use fallback details like git worktree add failed and git fetch origin failed, which produces text such as git worktree add failed That branch is already checked out in another worktree.

Additional Locations (2)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 64e5038. Configure here.

@macroscopeapp

macroscopeapp Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Would Approve

Macroscope's review found this PR approvable — This is a localized Git error-enrichment fix with an optional, backward-compatible reason field, fixed safe messages, and focused integration tests; successful command behavior remains unchanged. A remaining risk is that hook-generated stderr may occasionally be mistaken for Git diagnostics, while the separate fallback-message formatting issue is minor.

Not approved because:

  • 1 blocking correctness issue found at or above your repo's Minimum Blocking Severity

Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Git command errors discard stderr, making failures opaque to callers

1 participant