fix(vcs): surface git's stderr on GitCommandError - #5620
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:
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 |
There was a problem hiding this comment.
Effect service conventions: the new stderrTail field puts raw git command output into a direct error attribute and into the caller-visible message. Convention requires direct error attributes and messages to stay safe and bounded (normalized categories plus lengths/counts), with the exact underlying value preserved only as cause. This also reintroduces data that #3253 deliberately removed and that GitVcsDriverCore.test.ts still asserts against. Details inline.
Posted via Macroscope — Effect Service Conventions
|
Thanks — all three are fair, and the second one is the important correction. I've dropped the original approach entirely rather than patch around it.
Specifically on each point:
The remaining change is the smallest thing that makes the failure knowable: with the error keeping only lengths and clone failures returned to the caller rather than logged, git's reason currently exists nowhere. Debug level felt like the right default given the content can carry credentials — happy to move it behind an explicit diagnostics flag instead if you'd prefer it off by default. |
There was a problem hiding this comment.
One convention issue: the raw git stdout/stderr that #3253 removed from GitCommandError is now re-attached beside the sanitized error as a debug log payload. Convention requires log payloads to be as safe and bounded as the error attributes themselves.
Posted via Macroscope — Effect Service Conventions
^ this was claude lol |
|
Fixed — the log payload is now a normalized category plus lengths, and the exact output moved to The annotation reuses the vocabulary
Net effect is now three lines of behaviour: a debug log with a category and counts at each non-zero-exit site, and the output on |
ApprovabilityVerdict: Approved dc88184 This PR improves git error classification and messaging without changing core git operation behavior. The schema change is additive (optional You can customize Macroscope's approvability policy. Learn more. |
A failing git command records only `stdoutLength` and `stderrLength` — how much git wrote, never what it wrote. The text is captured and then dropped, so the reason exists nowhere: not in the error, not in the RPC response, and not in the server log (clone failures are returned to the caller rather than logged). In practice this turns an ordinary, self-explanatory git failure into an opaque one. A clone whose remote refuses the key surfaces as "The source control operation could not be completed", with git's own "Permission denied (publickey)" discarded a few frames earlier. Add an optional `stderrTail` carrying the end of stderr, and include it in the error message. Truncated from the end, since git puts the reason on its last lines behind a long transfer log, and credential-bearing URLs are redacted because git echoes back the remote it was handed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses review: drop `stderrTail` and the message change entirely, and log the output at the failure site instead. The first attempt put git's stderr on `GitCommandError` as a bounded, redacted attribute. That was wrong on three counts, all correctly flagged: it reintroduces raw command output into a value that crosses RPC, UI and persistence boundaries; it makes `message` unbounded rather than derived from stable structural attributes; and the redaction only covered `//user:pass@` remotes, missing single-token URLs, query-string tokens, and anything git echoes back from an argument or hook. It also regressed the existing test asserting a secret passed as a git argument never reaches `error.message`, and effectively reverted pingdotgg#3253, which removed stderr from this error for exactly these reasons. The error's public shape is now untouched. Both non-zero-exit sites call `Effect.logDebug` with the command context and its stdout/stderr, so the diagnostic is reachable when someone goes looking without becoming part of the serialized error. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses the remaining review finding: a log payload has to be as safe and bounded as a direct error attribute, and the previous revision copied raw stdout/stderr into one. Git echoes back its arguments and hook output, and the buffers are capped only by `maxOutputBytes` — megabytes at some call sites — so that payload was neither safe nor bounded. The log annotation is now a normalized category plus lengths, reusing the vocabulary `VcsProcess.classifyNonZeroExit` already established (authentication / not-found / command-failed). The exact text is preserved on the error's `cause`, as the convention prescribes. `GitCommandError`'s direct attributes are unchanged, so the existing assertion that a secret passed as a git argument reaches neither `error.message` nor an `stderr` property still holds, and `isMissingGitCwdError` is unaffected — it guards on `cause instanceof PlatformError`, which a string fails exactly as the previous `undefined` did. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Attaching stderr to `GitCommandError.cause` put unredacted git output on a field that is part of the error's RPC schema, so it reached clients on all six `WsVcs*` methods that declare `GitCommandError`. Git echoes back the remote it was handed and any hook output, so that text can carry a token. Take the approach `VcsProcessExitError.fromProcessExit` already uses for the same problem: classify stderr into a bounded `failureKind`, turn that into a fixed caller-facing `detail`, and let the text go. An authentication failure now says so in `detail` — which was the point of the change — while nothing git wrote crosses the boundary. `GitVcsDriverCore.test.ts` now asserts `cause` is free of a secret passed as a git argument; the existing case only covered `message` and `detail`, which is why this regressed unnoticed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`classifyGitFailure` matched a bare "permission denied", which git also
writes for local filesystem errors — `git init` into an unwritable directory
reaches it too. That was harmless while the classification only annotated a
log line, but it now drives the caller-facing `detail`, so an unwritable
directory would have been answered with advice about remote credentials.
Match the forms that are specific to a remote instead: ssh names the methods
it tried ("Permission denied (publickey)."), and GitHub over https writes
"remote: Permission to owner/repo.git denied to user".
Also widens the not-found sentence, which promised the missing thing was a
repository while the match is broad enough to catch `path 'x' does not exist
in 'HEAD'`.
The two new cases were asserting against real git rather than the stub —
they sit outside `it.layer(TestLayer)` now, and assert on recorded spawns so
a bypassed stub fails instead of silently passing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Bugbot named `.git/index.lock` and `FETCH_HEAD` specifically. Both already classify as command-failed, but only the clone-target phrasing was covered, so nothing pinned the other two. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
b40a853 to
b68ec7f
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b68ec7f. Configure here.
GitHub/GitLab SSH failures for a missing repo include both "not found" and the "access rights" footer; matching the footer first mislabeled them as auth. Also force LC_ALL=C on paths that classify failures so translated stderr cannot miss the English heuristics. Co-authored-by: Cursor <cursoragent@cursor.com>
Dismissing prior approval to re-evaluate dc88184
|
Note 🤖 GPT-6 Astra (preview) responding on behalf of Theo This was closed as part of an automated cleanup pass. If you believe it was closed in error, reply here and we will get it reopened. Closing in favor of #8645. It gives specific failure reasons for Git commands and excludes hook output from classification while keeping raw stderr private. This branch now uses broader categories, despite the original title. Review continues on the retained PR, with the classification-only locale behavior and its tests recorded there. |

Problem
When a git command exits non-zero,
GitCommandErrorrecordsstdoutLengthandstderrLength— how much git wrote, never what it wrote. The text is captured by the process runner and then dropped.That leaves the reason nowhere to be found:
SourceControlRepositoryServicereturns clone failures to the caller rather than logging them, soboot-service.logstays empty for the entire failure.The result is that an ordinary, self-explanatory git failure becomes opaque.
What it looks like today
Cloning onto a headless server whose host has no key registered with the remote. The RPC response, in full:
{"_tag":"SourceControlRepositoryError","provider":"unknown","operation":"cloneRepository", "detail":"The source control operation could not be completed.", "cause":{"name":"GitCommandError", "message":"Git command failed in SourceControlRepositoryService.cloneRepository (/root): Git command exited with a non-zero status."}}Git had already said exactly what was wrong. Recovering it required patching
dist/bin.mjson the server toconsole.errorthe discarded text:One line, and the problem is obvious. Getting to it took an afternoon.
Change
Add an optional
stderrTailtoGitCommandErrorand populate it at both non-zero-exit sites inGitVcsDriverCore, then include it in the error'smessageso it reaches anywhere the error is already rendered.Two details that seemed worth getting right rather than dumping the buffer:
//user:token@host→//user:***@host). Git echoes back the remote it was handed, which may embed a token, and this value now travels into logs and RPC responses.The 2000-character cap is a judgement call; happy to change it, or to gate the whole field behind a debug flag if you'd rather it not be on by default.
Notes
stderrTailisSchema.optional, so existing clients and any serialized errors are unaffected.vp/pnpm setup I did not want to guess at. The change is confined to one new helper plus two field additions, and I have parse-checked both files. Happy to fix whatever CI says.SourceControlRepositoryServicenever logs clone failures, and the web client renders a generic string in place of thedetailthe server sends. Either would be a good follow-up; this one is the smallest change that makes the failure knowable at all.Note
Medium Risk
Changes how git failures are classified and what clients see on RPC errors (auth/remote semantics), though the design intentionally avoids leaking stderr or credentials.
Overview
Non-zero git exits in
GitVcsDriverCoreare now classified from stderr (authentication,not-found, orcommand-failed) and surfaced onGitCommandErrorvia an optionalfailureKind, withdetailreplaced by fixed, non-secret copy instead of a generic exit message or raw git output.Classification runs only when failures are turned into errors (not when
allowNonZeroExitreturns raw output). Those paths setLC_ALL=Cso English heuristics stay reliable;allowNonZeroExitkeeps the caller’s locale.Failed commands log bounded debug metadata (kinds and stdout/stderr lengths), not command output. Tests add a stubbed failing-git layer and cover auth vs local “permission denied”, not-found vs “access rights”, locale behavior, and that tokens never appear in
detail,message, orcause.Reviewed by Cursor Bugbot for commit dc88184. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Surface git stderr in
GitCommandErrorwith failure classificationclassifyGitFailureinGitVcsDriverCore.tswhich inspects lowercased stderr to categorize failures as'authentication','not-found', or'command-failed'.failureKindfield onGitCommandError(ingit.ts) and sets a stable, classification-deriveddetailstring instead of a generic non-zero exit message.LC_ALL=Cwhen git output will be classified to ensure consistent stderr text for heuristic matching; preserves caller locale whenallowNonZeroExitis true.failureKind) without logging raw output to avoid leaking secrets.GitCommandError.detailnow reflects the failure class (e.g. credential advice) rather than a raw non-zero exit message when classification applies.Macroscope summarized dc88184.