Capture clone subprocess output to match rest of file

[jamesgol]

Every other subprocess call in clone.py (scrub, fetch, checkout) uses capture_output=True, but the main git clone call in shallow_clone() does not, its stderr goes straight to the parent process's file descriptors. This adds capture_output=True to match the rest of the file.

[schenksj]

Thanks for this — the premise is correct and the direction is right. scrub, fetch, and checkout all capture; only clone inherits the parent's fds. Capturing is also the safer default: git can echo the token-bearing URL in some error paths, and inheriting fds writes it to the operator's terminal outside redact().

Requesting changes for one blocking issue: as written, the captured stderr is dropped on the floor and the failure branch below still says "see git output above" — so clone failures now produce zero diagnostics.

Verified locally against a nonexistent repo.

Before (main):

remote: Repository not found.
fatal: repository 'https://github.com/capitalone/definitely-not-a-repo-xyz/' not found
RAISED: git clone failed (exit 128) for ...; see git output above

After (this PR):

RAISED: git clone failed (exit 128) for ...; see git output above

Auth failures, GIT_TERMINAL_PROMPT=0 refusals, DNS/proxy errors, repo-not-found — all collapse to a bare exit code. That's a real regression for the private-repo and proxy cases this agent is most likely to hit. The consistency argument actually cuts the other way: fetch and checkout capture and fold redact(...stderr.strip()) into the RuntimeError. Clone should match that, not just the capture_output=True half of it.

Details inline. Summary of what I'd like before merge:

Blocking

1. Surface redact(result.stderr) in the returncode != 0 branch (clone.py:131-137) and drop the now-false "see git output above".

Test updates (please include with the fix)
tests/test_clone.py currently can't catch this — _FakeCompleted hardcodes stderr = "", so the regression passes CI silently. Concretely:

• Give _FakeCompleted.__init__ an optional stderr: str = "" parameter.
• Extend the existing test_clone_failure_raises (around test_clone.py:145-149) or add a sibling: _FakeCompleted(returncode=128, stderr="fatal: repository 'https://github.com/org/myrepo' not found") → assert the raised RuntimeError message contains "repository" / the git text, not just "git clone failed".
• Add a redaction test: stderr containing a ghp_-prefixed token (or a https://x-access-token:ghp_...@github.com/... URL) → assert "ghp_" secret body does not appear in the exception message and *** does. This is the one that matters most, since capturing stderr is precisely what puts token-bearing git output on a path toward logs.
• If you drop --progress (see inline), the existing argv assertion at test_clone.py:305-310 should be tightened to assert the full expected argv rather than just cmd[:2], so future argv drift is caught.

Non-blocking — see inline notes on --progress and the timeout path.

[schenksj]

Blocking. capture_output=True routes git's stderr into result.stderr, but nothing ever reads it — and the failure branch at lines 131-137 still tells the operator to "see git output above", which after this change points at nothing.

GitHub won't let me anchor a comment on line 131 (outside the diff), so the suggested edit for that branch:

    if result.returncode != 0:
        if target.exists():
            shutil.rmtree(target, ignore_errors=True)
        raise RuntimeError(
            f"git clone failed (exit {result.returncode}) for {redact(repo_url)}: "
            f"{redact(result.stderr.strip())}"
        )

This mirrors the fetch (line 266-270) and checkout (line 283-287) handlers exactly, which is the consistency this PR is going for.

redact() is mandatory here, not stylistic — same reasoning as the scrub warning at line 163. git will happily print the tokenized remote URL in some failure messages, and this exception propagates up to __main__.py:902 where it lands in operator-visible output.

Please also add the stderr test coverage described in the review body — _FakeCompleted currently pins stderr = "", so neither the diagnostics loss nor a redaction miss would fail CI today.

[schenksj]

Non-blocking. With capture_output=True on the next line, --progress becomes dead weight. Its only job is forcing per-object progress rendering when stderr isn't a tty; now the other end is a pipe nobody reads, so it just buffers counter spam into memory and dilutes the result.stderr you'll want to put in the error message.

Suggested change

	[_GIT_EXECUTABLE, "clone", "--progress", "--depth", "1", "--", effective_url, str(target)],
	[_GIT_EXECUTABLE, "clone", "--depth", "1", "--", effective_url, str(target)],

Keep it deliberately if streaming clone progress to the UI is on the roadmap — but then it needs a reader, not just a pipe. Either way, if the argv changes, please tighten the assertion at tests/test_clone.py:305-310 from captured[0][1] == "clone" to a full expected-argv comparison so this doesn't drift again unnoticed.

[schenksj]

Non-blocking / FYI. Attaching pipes slightly weakens this timeout. On TimeoutExpired, subprocess.run kills the direct child and then re-enters communicate() with no timeout of its own; a surviving git-remote-https grandchild that inherited the stderr pipe can keep it open and stall the call past timeout_seconds. With no pipes (today's behavior) the timeout path was clean.

Low likelihood, and I wouldn't hold the PR for it — but this timeout exists specifically as a hang guard, so it's worth knowing the failure mode changed. If you want to close it properly, subprocess.Popen + communicate(timeout=...) + kill() + a second bounded communicate() is the usual shape.

[schenksj]

@jamesgol - Do you expect to have an opportunity to address the feedback on this one?

[jamesgol]

Like the other PR, changes were already pushed

…

On Tue, Aug 18, 2026, 12:47 PM Scott Schenkein ***@***.***> wrote: *schenksj* left a comment (capitalone/VulnHunter#18) <#18 (comment)> @jamesgol <https://github.com/jamesgol> - Do you expect to have an opportunity to address the feedback on this one? — Reply to this email directly, view it on GitHub <#18?email_source=notifications&email_token=ABTZLJPEDBM4TQ5H6W7JOID5KSQFLA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMZTGI2TQMBXGI32M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-5332580727>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABTZLJPASXWC3J2D622XJ4L5KSQFLAVCNFSNUABGKJSXA33TNF2G64TZHMYTEOJSG42TMMBVGM5US43TOVSTWNBZG42DCMJYGQYDFILWAI> . Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS <https://github.com/notifications/mobile/ios/ABTZLJMKAH2ZCUFOLCW66TL5KSQFLA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMZTGI2TQMBXGI32M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y> and Android <https://github.com/notifications/mobile/android/ABTZLJOHK472TDNFTYWTG2L5KSQFLA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMZTGI2TQMBXGI32M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>. Download it today! You are receiving this because you were mentioned.Message ID: ***@***.***>