git: replace the 17 Run* variants with one call that takes options #527

New issue

Open

opened 2026-09-16 17:07:36 +00:00 by hexajon · 1 comment

hexajon commented

2026-09-16 17:07:36 +00:00

Owner

What happens

internal/git exports 17 functions that run git. Each one is a fixed mix of three options: a working directory or not, forge auth or not, and a way to capture output (stdout only, stdout and stderr, stderr only, or passthrough). Examples: RunGit, RunGitDir, RunGitWithStderr, RunGitDirWithStderr, RunGitAuth, RunGitAuthWithCredentials, RunGitAuthDirWithCredentials, RunGitAuthPassthrough, RunGitAuthCaptureStderr, RunGitAuthDirCaptureStderr, RunGitAuthDirCaptureStderrOnly, RunGitAuthDirWithStderr, RunGitAuthWithStderr, and RunGitCloneUnauthenticated.

A new need means a new function name, and a caller must find the right one of 17.

Callers also repeat a test seam per file, about 30 times, for example:

commitRunGitDir      = libgit.RunGitDir
stashRunGitWithStderr = gitlib.RunGitWithStderr

Depends on

Issue 524 adds one shared function that starts every git process (it sets LC_ALL=C and checks the minimum git version) and removes the direct exec.Command("git", ...) calls. This issue builds on that function.

Ask

One exported call that takes the options, for example git.Run(ctx, git.Opts{Dir: dir, Auth: true, Capture: git.Stderr}, args...). The exact shape is part of the work.
Every current Run* caller moves to it, and the 17 variants go.
One test seam replaces the per-file seam variables, for example a package-level runner that tests swap once.
The auth paths keep their current security properties: host-scoped GIT_ASKPASS, GIT_TERMINAL_PROMPT=0, and no credential on argv.

Acceptance

internal/git exports one run call, plus any helper that is not a run variant.
No per-file xxxRunGit* = libgit.RunGit* seam variables remain.
The auth tests pass unchanged, or changed only for the new call shape.
The gate passes.

## What happens `internal/git` exports 17 functions that run git. Each one is a fixed mix of three options: a working directory or not, forge auth or not, and a way to capture output (stdout only, stdout and stderr, stderr only, or passthrough). Examples: `RunGit`, `RunGitDir`, `RunGitWithStderr`, `RunGitDirWithStderr`, `RunGitAuth`, `RunGitAuthWithCredentials`, `RunGitAuthDirWithCredentials`, `RunGitAuthPassthrough`, `RunGitAuthCaptureStderr`, `RunGitAuthDirCaptureStderr`, `RunGitAuthDirCaptureStderrOnly`, `RunGitAuthDirWithStderr`, `RunGitAuthWithStderr`, and `RunGitCloneUnauthenticated`. A new need means a new function name, and a caller must find the right one of 17. Callers also repeat a test seam per file, about 30 times, for example: ```go commitRunGitDir = libgit.RunGitDir stashRunGitWithStderr = gitlib.RunGitWithStderr ``` ## Depends on Issue 524 adds one shared function that starts every git process (it sets `LC_ALL=C` and checks the minimum git version) and removes the direct `exec.Command("git", ...)` calls. This issue builds on that function. ## Ask - One exported call that takes the options, for example `git.Run(ctx, git.Opts{Dir: dir, Auth: true, Capture: git.Stderr}, args...)`. The exact shape is part of the work. - Every current `Run*` caller moves to it, and the 17 variants go. - One test seam replaces the per-file seam variables, for example a package-level runner that tests swap once. - The auth paths keep their current security properties: host-scoped `GIT_ASKPASS`, `GIT_TERMINAL_PROMPT=0`, and no credential on argv. ## Acceptance - `internal/git` exports one run call, plus any helper that is not a run variant. - No per-file `xxxRunGit* = libgit.RunGit*` seam variables remain. - The auth tests pass unchanged, or changed only for the new call shape. - The gate passes.

hexajon added the

enhancement

label

2026-09-16 17:07:36 +00:00

hexajon added a new dependency

2026-09-16 17:07:40 +00:00

#524 git: pin the git output contract and declare a minimum git version

hexajon commented

2026-09-16 21:14:22 +00:00

Author

Owner

The enabling trick is that the 15 variants become one-line wrappers over the new call first. Nothing breaks, so the callers move in small batches afterwards and every step compiles and gates green on its own.

Step	Scope	Rough size
1	`internal/git`: add `Opts` and `Run`, the 15 variants delegate to it	~300 lines
2	One package-level test seam; drop the per-file seam variables	~200 lines
3	Migrate the `internal/cmd/git` callers	~150 lines
4	Migrate the `internal/service/git` callers	~200 lines
5	Migrate `internal/mcp` and the remaining callers	~150 lines
6	Delete the 15 wrappers, add a guard test against new variants	~80 lines

Step 1 is the only one that needs hard review. The rest are sweeps.

What step 1 has to get right

The issue text describes three options: a directory, auth, and a capture mode. Reading the source, the variants actually differ on six axes, and three of them are security-relevant. A three-axis collapse would silently change behaviour:

Dir - empty, meaning inherit the process working directory, or explicit.
Auth mode - none, required, best-effort with an ErrNotConfigured fallback to the unguarded plain runner, or explicit credentials passed in.
Capture shape - stdout only, both buffered, stdout streamed with stderr teed, stdout streamed with stderr buffered and not teed, or full passthrough.
remoteVerbGuard - applied by RunGitDir and RunGitDirWithStderr only. Not by RunGitCloneUnauthenticated (a deliberate exemption, #163), not by any auth variant, not by the unauthenticated fallback.
SSH transport dispatch - happens before credential resolution in RunGitAuthPassthrough, RunGitAuthDirCaptureStderr and RunGitAuthDirWithStderr. Not in RunGitAuthDirCaptureStderrOnly, not in RunGitAuthDirWithCredentials.
GIT_TERMINAL_PROMPT=0 - set by RunGitCloneUnauthenticated and by setupAuthCmd.

Step 1 carries a table with one row per variant naming its value on all six axes, taken from the source rather than inferred from the name, plus table tests that lock each axis. Axes 4 and 5 are the ones a careless collapse gets wrong, and remoteVerbGuard is a non-overridable guard under ADR 0009, so Opts must not expose a way to turn it off where it is on today.

RunSSHAGitPush in agit.go is out of scope throughout. It is a composed AGit push operation, not a generic run variant.

Every PR references this issue; the last one closes it.

Plan: this lands as six PRs, not one. A single diff here would be over a thousand lines and would mix a security-sensitive API design with a mechanical caller sweep, which is not reviewable in one read. The enabling trick is that the 15 variants become one-line wrappers over the new call first. Nothing breaks, so the callers move in small batches afterwards and every step compiles and gates green on its own. | Step | Scope | Rough size | |---|---|---| | 1 | `internal/git`: add `Opts` and `Run`, the 15 variants delegate to it | ~300 lines | | 2 | One package-level test seam; drop the per-file seam variables | ~200 lines | | 3 | Migrate the `internal/cmd/git` callers | ~150 lines | | 4 | Migrate the `internal/service/git` callers | ~200 lines | | 5 | Migrate `internal/mcp` and the remaining callers | ~150 lines | | 6 | Delete the 15 wrappers, add a guard test against new variants | ~80 lines | Step 1 is the only one that needs hard review. The rest are sweeps. ## What step 1 has to get right The issue text describes three options: a directory, auth, and a capture mode. Reading the source, the variants actually differ on six axes, and three of them are security-relevant. A three-axis collapse would silently change behaviour: 1. **Dir** - empty, meaning inherit the process working directory, or explicit. 2. **Auth mode** - none, required, best-effort with an `ErrNotConfigured` fallback to the unguarded plain runner, or explicit credentials passed in. 3. **Capture shape** - stdout only, both buffered, stdout streamed with stderr teed, stdout streamed with stderr buffered and not teed, or full passthrough. 4. **`remoteVerbGuard`** - applied by `RunGitDir` and `RunGitDirWithStderr` only. Not by `RunGitCloneUnauthenticated` (a deliberate exemption, #163), not by any auth variant, not by the unauthenticated fallback. 5. **SSH transport dispatch** - happens before credential resolution in `RunGitAuthPassthrough`, `RunGitAuthDirCaptureStderr` and `RunGitAuthDirWithStderr`. Not in `RunGitAuthDirCaptureStderrOnly`, not in `RunGitAuthDirWithCredentials`. 6. **`GIT_TERMINAL_PROMPT=0`** - set by `RunGitCloneUnauthenticated` and by `setupAuthCmd`. Step 1 carries a table with one row per variant naming its value on all six axes, taken from the source rather than inferred from the name, plus table tests that lock each axis. Axes 4 and 5 are the ones a careless collapse gets wrong, and `remoteVerbGuard` is a non-overridable guard under ADR 0009, so `Opts` must not expose a way to turn it off where it is on today. `RunSSHAGitPush` in `agit.go` is out of scope throughout. It is a composed AGit push operation, not a generic run variant. Every PR references this issue; the last one closes it.

hexajon referenced this issue

2026-09-17 00:23:17 +00:00

git: ssh and ssh-only mode are ignored by six credential-bearing git legs #535

hexajon added a new dependency

2026-09-17 00:23:34 +00:00

#535 git: ssh and ssh-only mode are ignored by six credential-bearing git legs

hexajon referenced this issue

2026-10-20 03:29:52 +00:00

audit: find every concern with more than one implementation, and every bypass of an existing chokepoint #834

hexajon added a new dependency

2026-10-20 03:30:00 +00:00

#834 audit: find every concern with more than one implementation, and every bypass of an existing chokepoint