Skip to content

fix(git): explain pull failures without exposing remote output - #18083

Open
TinBane wants to merge 1 commit into
pingdotgg:mainfrom
TinBane:fix/pull-failure-detail-v2
Open

TinBane wants to merge 1 commit into
pingdotgg:mainfrom
TinBane:fix/pull-failure-detail-v2

Conversation

@TinBane

@TinBane TinBane commented Oct 11, 2026

Copy link
Copy Markdown

Supersedes #14702, which was closed because it did not name the model and harness used. This is a fresh rebuild on current main by the model and harness listed at the end, reviewed by two others.

What Changed

pullCurrentBranch now explains a failed git pull --ff-only with the fixed diagnoses #12485 added for fetch. When Git's output matches a known case the detail says "Git could not authenticate with the remote…", "Git could not reach the remote…", "Git could not access the remote repository…" or the reference-lock message. Anything else keeps "git pull failed". The reason tag from #8645 is kept as it is.

Raw Git output still never enters the error; only the fixed messages do. To match them on any server, git pull now runs with LC_ALL=C, like the fetch path. The output cap guard for large fast-forwards (appendTruncationMarker) is unchanged.

Two small changes to the shared diagnosis, both found in review: the network case now also recognises macOS and unroutable ssh wording (Operation timed out, No route to host, Host is down), so an unreachable host on the main desktop platform is named; and the lock message says a lock "may be blocking Git" rather than "the fetch", since it now also covers pull's index.lock.

Why

The triage of #11872 names this as its secondary problem: when a desktop SSH remote loses its forwarded agent, Pull fails and the Permission denied (publickey) that explains it never reaches the UI. #12485 fixed that for fetches; the Pull action was left on the generic message. Since #14702 was opened, #8645 added a reason tag, so main now shows a code rather than nothing, but still not an explanation:

Message for a Pull rejected for a missing SSH key
main Git command failed in GitVcsDriver.pullCurrentBranch.pull (<repo>): git pull failed (authentication_failed)
This PR Git command failed in GitVcsDriver.pullCurrentBranch.pull (<repo>): Git could not authenticate with the remote. Check Git credentials or SSH access on the server, then retry. (authentication_failed)

A small, focused fix for that defect: one call site, reusing the existing helper, with no change to what Pull does or to any default. It does not fix the agent lifetime bug in #11872 itself.

UI Changes

The toast text changes as in the table above. Screenshots from #14702, taken before #8645 added the trailing reason tag (so today's "before" also ends in (authentication_failed)):

Before:

Before, on the SSH remote: Pull failed, git pull failed

After:

After, on the SSH remote: Pull failed, Git could not authenticate with the remote

These were captured against a Linux host over SSH in the state #11872 describes; see #14702 for that setup.

Verification

  • vp test run src/vcs/GitVcsDriverCore.test.ts from apps/server: 144 passed. New, all with real Git:
    • a stand-in for ssh prints OpenSSH's Permission denied (publickey) line only under the C locale, so the test also proves pull fixes the locale; the error has the authentication detail and reason: "authentication_failed", and a marker printed alongside never appears in the message;
    • a branch tracks a local bare remote whose URL is then pointed at a missing path; the pull fails with "could not access the remote repository" and reason: "remote_unreachable";
    • the existing "still fails a pull that Git rejects" test now also asserts that a non-fast-forward keeps exactly "git pull failed";
    • two table rows for the macOS and unroutable ssh wording.
  • Red before green: with the source file from main and the new tests kept, both new pull tests fail (expected 'git pull failed' to include 'could not access the remote repository', … to include 'could not authenticate'). With the fix but without LC_ALL=C on pull, the SSH test fails the same way.
  • vp exec tsc --noEmit -p . in apps/server: exit 0. vp lint and vp fmt --check on the two changed files: clean apart from two existing warnings on lines this PR does not touch.

Not checked in this rebuild: a fresh in-app run and the Linux/Windows test runs; #14702 did both for the same call-site change.

Review notes not acted on, deliberately out of scope: LC_ALL=C also reaches hooks run during the pull (post-merge), as it already does for fetch; and background auto-pull still runs without GIT_TERMINAL_PROMPT=0.

Models

  • Authored by Claude Opus 5.5, in Claude Code (run from T3 Code).
  • Reviewed by GPT-6.1 Sol (Codex CLI, high reasoning) and Claude Opus 5.5 (separate Claude Code review agent). Their findings were applied as described above.

A failed Pull reported "git pull failed", tagged with a reason at best,
so a dead SSH agent or a missing remote was named only by a code. Fetch
failures already map authentication, network, missing-repository and
reference-lock errors to fixed messages; pull now uses the same
diagnoses and runs Git with LC_ALL=C so they match on any server locale.
The reason tag is kept, raw Git output still never enters the error, and
unrecognised failures keep the generic message.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@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 Oct 11, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 11, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This focused fix changes production handling of Git authentication failures and potentially credential-bearing remote stderr while adding locale-dependent diagnosis behavior. Although the existing pull flow is largely preserved and redaction is tested, the sensitive-data and authentication implications warrant human review.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 11, 2026

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: e64a5c0d-d3f3-453d-ab0c-1dc9e4b822f2

📥 Commits

Reviewing files that changed from the base of the PR and between 75fca7d and db300c3.


📒 Files selected for processing (2)
  • apps/server/src/vcs/GitVcsDriverCore.test.ts
  • apps/server/src/vcs/GitVcsDriverCore.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.



📝 Walkthrough

Walkthrough

The Git VCS driver recognizes more SSH connection failures and classifies errors from pullCurrentBranch. Pull failures use a recognized diagnosis when available, or the generic detail git pull failed otherwise. Tests cover the diagnoses and check that sensitive output is not exposed.

Changes

Git failure diagnostics

Layer / File(s) Summary
Remote error classification
apps/server/src/vcs/GitVcsDriverCore.ts, apps/server/src/vcs/GitVcsDriverCore.test.ts
The classifier recognizes SSH connection timeouts and unroutable hosts. Local-reference guidance now refers to another Git operation, not only fetch.
Pull failure handling
apps/server/src/vcs/GitVcsDriverCore.ts, apps/server/src/vcs/GitVcsDriverCore.test.ts
pullCurrentBranch captures nonzero results and uses a recognized diagnosis or git pull failed. Tests cover missing remotes, rejected SSH keys, and checks against exposing paths or secret output.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~15 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge


Merge Risk | ⚪ Minimal · up to db300

Merge Risk: ⚪ Minimal · up to db300

The pull diagnostics change is mergeable after normal checks; no actionable merge-blocking risk remains.

Security Architecture Review

Security architecture risk: 🔵 Low · up to db300

Pull errors continue to use fixed explanations rather than remote output, and existing access paths remain unchanged. No introduced security issue was established. Compatibility with locale-sensitive repository hooks remains unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • observed — The affected failure reporting remains reachable through existing interactive and automatic pull callers. No new external caller or expanded error interface was identified in the base-to-head comparison.

Trust Boundaries and Controls

  • observed — Potentially remote-controlled stderr is used to select a predefined diagnosis, not interpolated into the new error. The nonzero-exit branch stores output lengths rather than stdout or stderr contents and supplies no cause. Unrecognized diagnostics retain the generic detail, preserving the control against remote-output disclosure at this producer boundary.

Pre-merge checks | Passed 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: explaining Git pull failures without exposing remote output. It uses an appropriate conventional commit format.
Description check Passed The description is mostly complete. It explains the problem, the implementation, scope, verification results, limitations, screenshots, and the model and harness used. The content maps to the required…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

This branch has not been deployed

No deployments
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.

1 participant