fix(server): name the cause of a failed git command - #8645
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Warning Your free Security trial is over. An organization admin can activate Security or dismiss this notice. Comment |
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This PR changes shared production Git error handling and automatically classifies SSH authentication and host-key failures, with new metadata and message output visible to existing callers. The authentication-sensitive runtime behavior warrants human validation despite the additive contract and comprehensive integration tests. You can add or adjust custom eligibility rules. Learn more. |
64e5038 to
dc39834
Compare
dc39834 to
87e512e
Compare
87e512e to
59c8b8b
Compare
There was a problem hiding this comment.
One convention finding on the new GitCommandError.reason discriminator. Details inline.
Posted via Macroscope — Effect Service Conventions
59c8b8b to
3a9c452
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3a9c452. Configure here.
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 diagnostic tags, appended to the message the way EnvironmentInternalError does it. The tag names the cause; it selects no text, so nothing matched from stderr is ever quoted and the existing redaction guarantee is unchanged. Only git's own diagnostic lines are classified, plus the refusals ssh prints. Hooks write to the same stream unprefixed, so a pre-push hook echoing "authentication failed" would otherwise be reported as a credential failure. ssh states its refusal unprefixed too, naming whichever methods it tried, and git adds only a generic "could not read from remote repository" afterwards — so every refusal shape is matched, and an untrusted host key gets its own tag rather than being blamed on credentials. Fixes pingdotgg#4380
3a9c452 to
61b8981
Compare

What Changed
GitCommandErrorgains an optionalreason: a closed set of diagnostic tags for well-knowngit failures, recognized from stderr inside the driver and appended to the message the way
EnvironmentInternalErrordoes it. The tag names the cause; it selects no text.Before and after, for the same failing command (real git, captured from the driver):
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 Idid not take:
apps/server/src/vcs/GitVcsDriverCore.test.tsalready assertsnotProperty(error, "stderr")and that a secret passed in argv never reacheserror.message, so dropping stderr is deliberate, and shipping an excerpt would meandeleting 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
(
executeGitand the non-zero-exit branch of the raw executor), which coversfetchRemoteandcreateWorktree— the two operations named in the issue.Two limits, stated rather than hidden:
remote:is deliberately excluded: git prefixes every byte the server sends that way, remote hook
output included, so trusting it would reintroduce the same false positive from the far end.
Every source that can reach the classifier is covered by a test — local hook output, remote
hook output, five ssh refusal shapes, and an OS-level permission error that must not read as
ssh auth. The one accepted residue is that a local hook echoing an ssh refusal verbatim would
still be classified; that text is narrow enough that the tag would arguably still be right. Hooks write to the same stream
unprefixed, so a
pre-pushhook echoing "authentication failed" would otherwise be reportedas a credential failure (reproduced, and now covered by a test that pushes through a failing
hook). ssh states a key refusal unprefixed as well, so those specific lines stay eligible —
otherwise git's generic "could not read from remote repository" would be read as an
unreachable remote when the real cause is credentials.
executeGitinherits the process locale, so under anon-English
LANGgit's wording does not match and the error simply keeps today'sbehavior — no reason, no regression. Forcing
LC_ALL=Cfor every git command would fixthat, 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.GitCommandErrorconstructions 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
The new tests drive real git through the real driver: a branch already checked out in
another worktree, a command outside a repository, a tag collision that must stay unclassified, a failing
pre-push hook whose output must not be classified, and a fallback detail that must not run
into the reason sentence. Each fails without the source change.
Checklist
to
GitCommandErrorfields exists in any client)Model: Claude Opus 5 (1M context). Harness: Claude Code.
Note
Medium Risk
Changes how git failures are surfaced (new optional field and message suffix) and classifies auth/SSH-related stderr, though patterns are conservative and stderr still never leaves the driver.
Overview
GitCommandErrornow carries an optionalreasonfrom a closedGitCommandFailureReasonunion in contracts, and the error message appends(reason)when classification succeeds—without ever exposing raw stderr.The git VCS driver adds
classifyGitFailure, which pattern-matches filtered diagnostic lines (plus specific SSH refusal lines) at the shared non-zero-exit paths inexecuteRawandexecuteGit. Callers can distinguish worktree branch conflicts, missing repos, auth vs host-key failures, and similar cases while the redaction guarantee stays intact (nostderr, no matched text in messages). Classification deliberately ignores hook/remote:noise and leaves ambiguous failures (e.g. duplicate tags) untagged; matching is English-only unless git runs underLC_ALL=C.Integration tests exercise real git scenarios and extend the secret-leak test to assert
reasonis not set when stderr would be unsafe to interpret.Reviewed by Cursor Bugbot for commit 61b8981. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add
reasontag toGitCommandErrorfor classified git failuresGitCommandFailureReasonschema in git.ts with literal tags likeauthentication_failed,not_a_repository,branch_checked_out_in_worktree, andhost_key_unverified.classifyGitFailure(stderr)in GitVcsDriverCore.ts that matches stderr lines against ordered regex patterns to produce a reason tag, returningnullwhen no pattern matches.executeRawandexecuteGitWithStableDiagnosticsnow callclassifyGitFailureon non-zero exits and attach the reason to the thrownGitCommandError.GitCommandError.messagenow appends(reason)when a reason is present, so callers that parse error messages see a new suffix.GitCommandErrormessages or asserts exact error text will break due to the appended(reason)suffix; thereasonfield is optional so existing.reasonchecks are unaffected.Macroscope summarized 61b8981.