Skip to content

Capture clone subprocess output to match rest of file #18

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jamesgol wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Capture clone subprocess output to match rest of file #18

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
29a2a06
Capture clone subprocess output to match rest of file
jamesgol Jul 25, 2026
8b2af81
Address PR #18 review feedback: surface redacted stderr in clone errors
jamesgol Aug 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions vulnhunter-agent/agent/clone.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,12 +111,13 @@ def shallow_clone(
if _GIT_EXECUTABLE is None:
raise RuntimeError("git not on PATH; cannot clone")
try:
# nosec B603 — argv is statically constructed ("clone --progress
# --depth 1 --"); effective_url comes from the agent's own URL
# nosec B603 — argv is statically constructed ("clone --depth 1
# --"); effective_url comes from the agent's own URL
# validation + token injection; target is a Path the agent
# owns. Absolute git path resolved at module load (kills B607).
result = subprocess.run( # nosec B603
[_GIT_EXECUTABLE, "clone", "--progress", "--depth", "1", "--", effective_url, str(target)],
[_GIT_EXECUTABLE, "clone", "--depth", "1", "--", effective_url, str(target)],
capture_output=True,

@schenksj schenksj Aug 1, 2026

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.

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.

text=True,

@schenksj schenksj Aug 1, 2026

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.

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.

timeout=timeout_seconds,
env=env,
Expand All @@ -132,8 +133,8 @@ def shallow_clone(
if target.exists():
shutil.rmtree(target, ignore_errors=True)
raise RuntimeError(
f"git clone failed (exit {result.returncode}) for {redact(repo_url)}; "
"see git output above"
f"git clone failed (exit {result.returncode}) for {redact(repo_url)}: "
f"{redact(result.stderr.strip())}"
)

# Strip the token from the remote URL stored in .git/config. Without
Expand Down
41 changes: 35 additions & 6 deletions vulnhunter-agent/tests/test_clone.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,10 @@ def test_derive(self, url: str, expected: str) -> None:


class _FakeCompleted:
def __init__(self, returncode: int = 0) -> None:
def __init__(self, returncode: int = 0, stderr: str = "") -> None:
self.returncode = returncode
self.stdout = ""
self.stderr = ""
self.stderr = stderr


@pytest.fixture
Expand Down Expand Up @@ -142,11 +142,15 @@ def test_subprocess_nonzero_cleanup_and_runtime_error(
def fake_run(cmd: list[str], **kwargs: Any) -> _FakeCompleted:
target = Path(cmd[-1])
target.mkdir(parents=True, exist_ok=True)
return _FakeCompleted(returncode=128)
return _FakeCompleted(
returncode=128,
stderr="fatal: repository 'https://github.com/org/myrepo' not found",
)

monkeypatch.setattr(clone_mod.subprocess, "run", fake_run)
with pytest.raises(RuntimeError, match="git clone failed"):
with pytest.raises(RuntimeError, match="git clone failed") as exc:
shallow_clone("https://github.com/org/myrepo", tmp_path)
assert "repository" in str(exc.value)
assert not (tmp_path / "myrepo").exists()

def test_git_terminal_prompt_env_set(
Expand Down Expand Up @@ -244,6 +248,29 @@ def fake_run(cmd: list[str], **kwargs: Any) -> _FakeCompleted:
assert "secret" not in msg
assert "***@github.com" in msg

def test_stderr_redacted_in_error_message(
self,
tmp_path: Path,
monkeypatch: pytest.MonkeyPatch,
) -> None:
def fake_run(cmd: list[str], **kwargs: Any) -> _FakeCompleted:
target = Path(cmd[-1])
target.mkdir(parents=True, exist_ok=True)
return _FakeCompleted(
returncode=128,
stderr="fatal: Authentication failed for 'https://x-access-token:ghp_SECRET@github.com/org/repo'",
)

monkeypatch.setattr(clone_mod.subprocess, "run", fake_run)
with pytest.raises(RuntimeError) as exc:
shallow_clone(
"https://github.com/org/repo",
tmp_path,
)
msg = str(exc.value)
assert "ghp_SECRET" not in msg
assert "Authentication failed" in msg

def test_url_redacted_in_timeout_error_message(
self,
tmp_path: Path,
Expand Down Expand Up @@ -306,5 +333,7 @@ def fake_run(cmd: list[str], **kwargs: Any) -> _FakeCompleted:
"https://github.com/org/myrepo",
tmp_path,
)
assert captured[0][0] == "/usr/bin/git"
assert captured[0][1] == "clone"
assert captured[0] == [
"/usr/bin/git", "clone", "--depth", "1",
"--", "https://github.com/org/myrepo", str(tmp_path / "myrepo"),
]