Run workspace grep through the trusted Git executable - #396
Open
beardthelion wants to merge 7 commits into
Open
Conversation
beardthelion
marked this pull request as ready for review
August 24, 2026 19:27
Workspace file discovery refuses to resolve git from PATH. trustedGitExecutable stats a fixed list of absolute paths and returns null rather than fall back, so a git planted earlier on PATH cannot stand in for the real one. Grep did the opposite: check-ignore, git grep, and git grep --count all passed a bare "git" as argv0, with cwd set to the search root. The three sites now take the executable the discovery layer selects, and grep falls back to the Zig scanner when no trusted Git exists, which is the same shape discoverWithStop already uses for the null case. A planted git first on PATH is executed twice per grep on the old code, once for check-ignore and once for git grep. It is never reached through workspace discovery, which is what makes this an inconsistency between the two rather than a property of the tree. A file named git inside the workspace is not affected either way, since resolution is PATH-only, so the plant has to land in a PATH directory. Each of the three sites is covered by a test that fails when that site alone reverts to the bare name, and the null path is covered by a test that fails when its guard is removed. The unused-parameter error catches the same revert at compile time if the parameter is left in place.
Review of the previous commit found the guard's own tests were mostly not load-bearing. Four problems, each confirmed by mutation. The spawn tests asserted an exact tally of two runs of the injected executable. A tally cannot tell a missing spawn from one that resolved its own name through PATH and ran the real Git, so adding a bare-name spawn kept them green while threading the executable to one more site turned them red. They now assert the argv the selection was asked to run. The no-trusted-Git test asserted only that the scanner still returned the match. Substituting a name for the missing executable lands in the same scanner, so the test passed with the guard removed. It now asserts the trace line that only the refusal emits. The bare-name test wrapped its single assertion in an if-let over trustedGitExecutable, so it asserted nothing where no allowlisted Git exists, and it checked a workspace_files property rather than anything grep does. It is now a source scan over this file plus a skip-guarded absolute-path check. The fake Git interpolated the workspace path into an sh redirect unquoted, so both spawn tests failed on any checkout path containing a space. runGitForTest resolves Git the same way production does. That is what lets the source scan key on the bare literal rather than on an argv shape; a scan keyed on the shape misses a future spawn that skips the usual flags, which is the case the previous version of this guard let through.
The wrappers are the only place the trusted executable is looked up, and nothing asserted they do it. Passing null there degrades every grep to the Zig scanner while still returning the same matches, so the whole suite stayed green with the git backend disabled for every caller. The new test drives the public collect entry and asserts the refusal trace is absent, which is the one signal that separates resolving the executable from never asking for it. It fails with either wrapper nulled, not just both.
Everything guarding the trusted-Git change so far runs at the grep_search seam under zig test. Nothing exercised the hop from the grep_files tool through to the spawn, so a regression in that layer would not have been caught by any test. This drives the built binary against a fake gateway with a git planted first on PATH. The grep_files call must return its match without the plant recording a run. The terminal control is the load-bearing half. It runs `git --version` through the same planted PATH and asserts the plant did record a run, so an override that never reached the child cannot be read as a guard that held. Reintroducing the defect in gitIgnoresRoot makes the grep half fail with the recorded argv, which is how the pair was checked. tests/e2e files need a shard weight or the planner rejects the manifest.
CI validates that every tests/e2e file is either a corpus scenario or an intentional exclusion, and the new suite was neither, so the check failed before the build ran. It exercises fx runtime behavior through the built binary, so it is a scenario rather than an exclusion. A new E2E file needs two registrations, this and the shard weight; there are no others.
The corpus scenario alone was not enough. The driver test pins the training file tuple and the scenario counts, so classifying the suite without updating them failed CI a second time, after the build had already passed. A new tests/e2e file needs three registrations: the shard weight, the corpus scenario, and this. Fixing the tuple on its own still fails on the counts.
The control planted a `git` on PATH and asserted the terminal tool ran it. That holds on Linux and cannot hold on macOS: a login zsh runs path_helper, which rebuilds PATH with the system directories first, so /usr/bin/git wins wherever the plant sits. The PGSO training shard runs on macOS arm64, where the control failed and took the whole test with it before the grep half ran. The control now plants a second recorder under a name that exists nowhere else, so it resolves through PATH regardless of ordering. That is the property the control actually needs: proof the override reaches the child. The git plant stays exactly as it was, and the control additionally asserts nothing has touched it yet. Checked both directions: dropping the PATH override fails the control, and reintroducing the bare argv0 in gitIgnoresRoot fails the guard with the recorded argv.
beardthelion
force-pushed
the
fix/grep-trusted-git-executable
branch
from
September 7, 2026 14:58
7b84d2d to
bae6937
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #395.
Workspace file discovery refuses to resolve
gitfrom PATH:trustedGitExecutablestats a fixed list of absolute paths and returns null rather than fall back. Grep did the opposite, passing a bare"git"as argv0 atgitIgnoresRoot,gitGrepTrackedMatches, andgitGrepTrackedCounts, withcwdset to the search root. Onegrep_filescall ran a planted PATHgittwice; the same workspace searched through discovery never touched it.The three sites now take the executable discovery already selects, and grep skips the git backend when none is available, which is the
orelse break :gitshapediscoverWithStopuses for its null case. Semantics are otherwise unchanged:force_fallbackstill suppresses the check-ignore spawn, and an ignored root still reaches the same fallback.trustedGitExecutablebecomespub.runGitForTestin this file resolves Git the same way, which is what lets the argv guard key on the bare literal rather than on an argv shape.Verification
Each guard was run against the mutation it exists to catch.
git grep -nargv0 -> bare namegit grep --countargv0 -> bare nameorelse "git"The end-to-end test drives the built binary against a fake gateway with a
gitplanted first on PATH.grep_filesreturns its match while the plant records nothing. Theterminalcontrol runsgit --versionthrough the same PATH and asserts the plant did record a run, so an override that never reached the child cannot read as a guard that held. Reintroducing the defect makes the grep half fail with the recorded argv.Three of the four commits are test work because the first round of tests was not load-bearing: the spawn assertion counted runs, so adding a bare-name spawn kept it green while threading the executable to one more site turned it red, and the null-guard test passed with the guard replaced by a name.
zig build teston88cb3da8: 8403/8458 passing on base, 8409/8464 on the branch, with the same 33 failing test names and the same 7 crashes on both. The +6 accounts for the added tests.zig fmt --checkclean.Scope
Two pre-existing gaps this does not close, both noted in #395:
src/core/github/git_context.zig:37andsrc/builtins/skills.zig:253still build argv from a bare"git", andtrustedGitExecutablestats withfollow_symlinks = trueand checks only that the candidate is a file, so an allowlisted path that is user-writable or a symlink out of the allowlist is accepted. Pinning argv0 also does not stop the workspace's own.git/configfrom running a program throughcore.fsmonitor; that is unchanged by this PR, andcommand_effect.ziganddirect_command.zigalready harden their own git spawns.The allowlist misses some real install locations (Linuxbrew,
/snap/bin, asdf/mise shims). On those machines grep now uses the Zig scanner rather than the git backend. Results are equivalent, since the untracked pass already fell back there, but it is slower on a large repo.The e2e test covers the collect path; the count path is covered at the module seam only. Both spawn tests skip on Windows, which the allowlist supports but CI does not run.