From fc6fe5e5df759f02a1aa1644a2786e10731e0037 Mon Sep 17 00:00:00 2001 From: Juan Garibay Date: Thu, 17 Sep 2026 15:06:49 -0400 Subject: [PATCH 1/9] fix(hooks): pre-push skipped every Python project that uses a virtualenv The Python block gates on `command -v pytest`, so it only runs when pytest is on PATH. Installing a project's tools into a virtualenv is the norm rather than the exception, so in practice the hook printed [ECC pre-push] Python project detected but pytest is not installed. Skipping. while standing in a directory with `.venv/bin/pytest` in it, and pushed. The failure mode is worse than not having the hook. A skip line reads like a pass: the push succeeds, the output looks healthy, and nothing indicates the gate declined to gate. A repository can sit behind it for months believing its tests run on every push. Found on a project with 893 tests, none of which the hook had ever executed. `resolve_pytest` now looks, in order, at `ECC_PYTEST_CMD`, `$VIRTUAL_ENV`, `.venv`, `venv`, `env`, `uv run` when a `uv.lock` is present, `poetry run` when a `poetry.lock` is, and finally PATH. Each candidate is confirmed by importing pytest rather than by the path existing, so a half-built venv falls through to the next one instead of failing the push. Two deliberate choices: The log line names the command it resolved -- `Running: .venv/bin/python -m pytest -q` -- so which interpreter ran is visible in the push output rather than inferred. When nothing resolves, the message says where it looked and names `ECC_PYTEST_CMD`, instead of asserting pytest is not installed when it may well be. `uv run` passes `--no-sync` so the hook cannot mutate the developer's environment on its way to running the tests. Behaviour change worth flagging for the release note: on any Python project with a working virtualenv this hook now actually runs the suite, and will block a push whose tests fail. That is the intent, but it is new behaviour for every such repository, and `ECC_SKIP_PREPUSH=1` remains the escape. Verified on two real repositories: a uv/venv Python project (resolves `.venv/bin/python -m pytest`, 893 tests, exits 0; exits 1 when the suite fails) and a Node project (unchanged, still runs lint/typecheck/test/build). --- scripts/codex-git-hooks/pre-push | 53 +++++++++++++++++++++++++++++--- 1 file changed, 49 insertions(+), 4 deletions(-) diff --git a/scripts/codex-git-hooks/pre-push b/scripts/codex-git-hooks/pre-push index 2ee23c7f4..472c2f194 100755 --- a/scripts/codex-git-hooks/pre-push +++ b/scripts/codex-git-hooks/pre-push @@ -117,16 +117,61 @@ if [[ -f "go.mod" ]] && command -v go >/dev/null 2>&1; then go test ./... || fail "go test failed" fi -if [[ -f "pyproject.toml" || -f "requirements.txt" ]]; then +# Resolve how this project runs pytest. +# +# Looking only for `pytest` on PATH meant the hook skipped every project that keeps +# its tools in a virtualenv -- which is most of them -- and reported "pytest is not +# installed" while sitting next to a .venv with pytest in it. A gate that silently +# declines to gate is worse than no gate, because the skip line reads like a pass. +# +# Echoes the command it will run, so the reason for a skip is always visible. +resolve_pytest() { + if [[ -n "${ECC_PYTEST_CMD:-}" ]]; then + echo "$ECC_PYTEST_CMD" + return 0 + fi + local venv + for venv in "${VIRTUAL_ENV:-}" .venv venv env; do + if [[ -n "$venv" && -x "$venv/bin/python" ]]; then + if "$venv/bin/python" -c "import pytest" >/dev/null 2>&1; then + echo "$venv/bin/python -m pytest" + return 0 + fi + fi + done + if [[ -f "uv.lock" ]] && command -v uv >/dev/null 2>&1; then + if uv run --no-sync python -c "import pytest" >/dev/null 2>&1; then + echo "uv run --no-sync pytest" + return 0 + fi + fi + if [[ -f "poetry.lock" ]] && command -v poetry >/dev/null 2>&1; then + if poetry run python -c "import pytest" >/dev/null 2>&1; then + echo "poetry run pytest" + return 0 + fi + fi if command -v pytest >/dev/null 2>&1; then + echo "pytest" + return 0 + fi + return 1 +} + +if [[ -f "pyproject.toml" || -f "requirements.txt" ]]; then + if pytest_cmd="$(resolve_pytest)"; then ran_any_check=1 - log "Python project detected. Running: pytest -q" - pytest -q || fail "pytest failed" + log "Python project detected. Running: $pytest_cmd -q" + # Unquoted on purpose: the resolver returns a command with arguments. + # shellcheck disable=SC2086 + $pytest_cmd -q || fail "pytest failed" else - log "Python project detected but pytest is not installed. Skipping." + log "Python project detected but no pytest found (checked \$VIRTUAL_ENV, .venv," + log " venv, env, uv, poetry, PATH). Set ECC_PYTEST_CMD to point at it." fi fi + if [[ "$ran_any_check" -eq 0 ]]; then log "No supported checks found in this repository. Skipping." else From 5cbe78c22b4e35e64fcd9ed40a9e6493400d8a80 Mon Sep 17 00:00:00 2001 From: Juan Garibay Date: Thu, 17 Sep 2026 15:57:09 -0400 Subject: [PATCH 2/9] fix(hooks): keep venv paths intact and check every pytest candidate Two holes in the resolver this branch added, both found in review. A virtualenv path may contain spaces. `resolve_pytest` returned one string and the caller expanded it unquoted, so `/home/me/my env/bin/python -m pytest` split into `/home/me/my` and `env/bin/python`. The probe that accepted the candidate was correctly quoted, so the hook reported the venv as usable and then failed to run anything in it -- rejecting the push for a reason with nothing to do with the code being pushed. It now builds an argv array and runs `"${PYTEST_CMD[@]}"`. The resolver's contract is that every candidate is confirmed to be pytest, and two of them were not. `ECC_PYTEST_CMD` was returned unchecked, so `ECC_PYTEST_CMD=true` made the hook run `true -q`, exit 0 and report a Python project verified by nothing. The PATH branch used `command -v pytest`, which proves only that a file of that name exists. Both now go through `is_pytest`, which runs `--version` and requires the output to name pytest -- `--version` alone is not evidence, since `true --version` also exits 0. A bad `ECC_PYTEST_CMD` fails the push rather than falling through to the next candidate. An operator who set it asked for that command, and silently running a different one hides the misconfiguration -- which is the same silent-gate failure this branch exists to remove, one level along. Three regression tests cover the three paths: a venv whose directory name contains a space, an override that is not pytest, and an override that is. --- scripts/codex-git-hooks/pre-push | 52 +++++++++++++---- tests/scripts/codex-hooks.test.js | 92 +++++++++++++++++++++++++++++++ 2 files changed, 132 insertions(+), 12 deletions(-) diff --git a/scripts/codex-git-hooks/pre-push b/scripts/codex-git-hooks/pre-push index 472c2f194..806d616cd 100755 --- a/scripts/codex-git-hooks/pre-push +++ b/scripts/codex-git-hooks/pre-push @@ -117,54 +117,82 @@ if [[ -f "go.mod" ]] && command -v go >/dev/null 2>&1; then go test ./... || fail "go test failed" fi -# Resolve how this project runs pytest. +# Resolve how this project runs pytest, into PYTEST_CMD as an argv array. # # Looking only for `pytest` on PATH meant the hook skipped every project that keeps # its tools in a virtualenv -- which is most of them -- and reported "pytest is not # installed" while sitting next to a .venv with pytest in it. A gate that silently # declines to gate is worse than no gate, because the skip line reads like a pass. # +# An array rather than one string, because a virtualenv path may contain spaces: +# a scalar command splits `/home/me/my env/bin/python` into two paths that do not +# exist, and the hook then rejects the push for a reason that has nothing to do +# with the code being pushed. +# # Echoes the command it will run, so the reason for a skip is always visible. +PYTEST_CMD=() + +# Does this command actually run pytest? Accepting `--version` is not evidence -- +# plenty of programs take it and exit 0 -- so the output has to name pytest. The +# version is captured rather than piped: under `set -o pipefail` a `| grep -q` can +# report the SIGPIPE of the program it just matched. +is_pytest() { + local version + version="$("$@" --version 2>&1)" || return 1 + grep -qiE 'pytest[[:space:]]+(version[[:space:]]+)?[0-9]' <<<"$version" +} + resolve_pytest() { if [[ -n "${ECC_PYTEST_CMD:-}" ]]; then - echo "$ECC_PYTEST_CMD" + # Word-split, so the override names a command on PATH or an interpreter whose + # path has no spaces; a venv with spaces in its name is found by the loop below. + read -r -a PYTEST_CMD <<<"$ECC_PYTEST_CMD" || true + # Checked like every other candidate, and fatally rather than by falling + # through: an operator who set this asked for that command, and quietly running + # a different one would hide the misconfiguration. `ECC_PYTEST_CMD=true` would + # otherwise run `true -q`, pass, and report a Python project verified by + # nothing -- the same silent gate this resolver exists to remove. + if [[ ${#PYTEST_CMD[@]} -eq 0 ]] || ! is_pytest "${PYTEST_CMD[@]}"; then + fail "ECC_PYTEST_CMD is set to '$ECC_PYTEST_CMD', which does not run pytest" + fi return 0 fi local venv for venv in "${VIRTUAL_ENV:-}" .venv venv env; do if [[ -n "$venv" && -x "$venv/bin/python" ]]; then if "$venv/bin/python" -c "import pytest" >/dev/null 2>&1; then - echo "$venv/bin/python -m pytest" + PYTEST_CMD=("$venv/bin/python" -m pytest) return 0 fi fi done if [[ -f "uv.lock" ]] && command -v uv >/dev/null 2>&1; then if uv run --no-sync python -c "import pytest" >/dev/null 2>&1; then - echo "uv run --no-sync pytest" + PYTEST_CMD=(uv run --no-sync pytest) return 0 fi fi if [[ -f "poetry.lock" ]] && command -v poetry >/dev/null 2>&1; then if poetry run python -c "import pytest" >/dev/null 2>&1; then - echo "poetry run pytest" + PYTEST_CMD=(poetry run pytest) return 0 fi fi - if command -v pytest >/dev/null 2>&1; then - echo "pytest" + # `command -v` proves only that a file of that name exists on PATH, which is why + # this candidate is confirmed too before it is accepted. + if command -v pytest >/dev/null 2>&1 && is_pytest pytest; then + PYTEST_CMD=(pytest) return 0 fi + PYTEST_CMD=() return 1 } if [[ -f "pyproject.toml" || -f "requirements.txt" ]]; then - if pytest_cmd="$(resolve_pytest)"; then + if resolve_pytest; then ran_any_check=1 - log "Python project detected. Running: $pytest_cmd -q" - # Unquoted on purpose: the resolver returns a command with arguments. - # shellcheck disable=SC2086 - $pytest_cmd -q || fail "pytest failed" + log "Python project detected. Running: ${PYTEST_CMD[*]} -q" + "${PYTEST_CMD[@]}" -q || fail "pytest failed" else log "Python project detected but no pytest found (checked \$VIRTUAL_ENV, .venv," log " venv, env, uv, poetry, PATH). Set ECC_PYTEST_CMD to point at it." diff --git a/tests/scripts/codex-hooks.test.js b/tests/scripts/codex-hooks.test.js index 0dfe1d2f9..a6f9f3fd6 100644 --- a/tests/scripts/codex-hooks.test.js +++ b/tests/scripts/codex-hooks.test.js @@ -306,6 +306,98 @@ if ( passed++; else failed++; +function writeExecutable(filePath, body) { + fs.mkdirSync(path.dirname(filePath), { recursive: true }); + fs.writeFileSync(filePath, body); + fs.chmodSync(filePath, 0o755); +} + +// The Python arm of the hook, exercised without a real interpreter: the stubs +// record the argv they were handed, which is what the virtualenv-path regression +// is actually about. +function runHermeticPythonPrePush({ + venvName = null, + pytestCmd = null, + overrideVersionLine = null, +} = {}) { + const tempDir = createTempDir('codex-pre-push-py-'); + const projectDir = path.join(tempDir, 'project'); + const callsPath = path.join(tempDir, 'calls.txt'); + fs.mkdirSync(projectDir); + fs.writeFileSync(path.join(projectDir, 'pyproject.toml'), '[project]\nname = "demo"\n'); + const initialized = spawnSync('git', ['init', '--quiet'], { cwd: projectDir }); + assert.strictEqual(initialized.status, 0, initialized.stderr?.toString()); + + const env = { + ECC_SKIP_GIT_HOOKS: '0', + ECC_SKIP_PREPUSH: '0', + MSYS_NO_PATHCONV: '1', + }; + + let venvPython = null; + if (venvName) { + venvPython = path.join(tempDir, venvName, 'bin', 'python'); + writeExecutable(venvPython, `#!/bin/sh\nprintf '%s\\n' "$0|$*" >> "${toBashPath(callsPath)}"\nexit 0\n`); + env.VIRTUAL_ENV = toBashPath(path.join(tempDir, venvName)); + } + + if (overrideVersionLine !== null) { + const stub = path.join(tempDir, 'bin', 'fake-pytest'); + writeExecutable(stub, `#!/bin/sh\nif [ "$1" = "--version" ]; then printf '%s\\n' '${overrideVersionLine}'; exit 0; fi\nprintf '%s\\n' "$0|$*" >> "${toBashPath(callsPath)}"\nexit 0\n`); + env.ECC_PYTEST_CMD = toBashPath(stub); + } else if (pytestCmd !== null) { + env.ECC_PYTEST_CMD = pytestCmd; + } + + const result = runBash(prePushHook, { + env, + cwd: projectDir, + input: Buffer.from('refs/heads/main 1111111111111111111111111111111111111111 refs/heads/main 0000000000000000000000000000000000000000\n'), + }); + const calls = fs.existsSync(callsPath) + ? fs.readFileSync(callsPath, 'utf8').trim().split(/\r?\n/).filter(Boolean) + : []; + cleanup(tempDir); + return { result, calls, venvPython }; +} + +if ( + test('pre-push runs pytest from a virtualenv whose path contains spaces', () => { + const { result, calls, venvPython } = runHermeticPythonPrePush({ venvName: 'my venv' }); + const python = toBashPath(venvPython); + assert.strictEqual(result.status, 0, `${result.stdout}\n${result.stderr}`); + assert.deepStrictEqual(calls, [ + `${python}|-c import pytest`, + `${python}|-m pytest -q`, + ], JSON.stringify({ calls, python, stdout: result.stdout, stderr: result.stderr }, null, 2)); + }) +) + passed++; +else failed++; + +if ( + test('pre-push rejects an ECC_PYTEST_CMD that does not run pytest', () => { + const { result, calls } = runHermeticPythonPrePush({ pytestCmd: 'true' }); + assert.notStrictEqual(result.status, 0, `${result.stdout}\n${result.stderr}`); + assert.match(result.stderr, /ECC_PYTEST_CMD is set to 'true', which does not run pytest/); + assert.deepStrictEqual(calls, []); + assert.doesNotMatch(result.stdout, /Verification checks passed/); + }) +) + passed++; +else failed++; + +if ( + test('pre-push runs an ECC_PYTEST_CMD override that identifies itself as pytest', () => { + const { result, calls } = runHermeticPythonPrePush({ overrideVersionLine: 'pytest 8.0.0' }); + assert.strictEqual(result.status, 0, `${result.stdout}\n${result.stderr}`); + assert.strictEqual(calls.length, 1, JSON.stringify(calls)); + assert.match(calls[0], /\|-q$/); + }) +) + passed++; +else failed++; + if ( test('check-plugin-cache fails when the installed cache is missing manifest-referenced files', () => { const homeDir = createTempDir('codex-plugin-cache-home-'); From 1f5cd2af737c0b319ced41c021663da082e40e7e Mon Sep 17 00:00:00 2001 From: Juan Garibay Date: Thu, 17 Sep 2026 16:04:41 -0400 Subject: [PATCH 3/9] test(hooks): build the pre-push python fixture env without mutation AGENTS.md makes immutability mandatory and the helper built `env` by assigning into it. Rather than reassigning a `let` through spreads, the two stub paths are now resolved before the object exists, so `env` is a single `const` built in one expression with the conditional keys spread in. Nothing to mutate and nothing to rebind. --- tests/scripts/codex-hooks.test.js | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/tests/scripts/codex-hooks.test.js b/tests/scripts/codex-hooks.test.js index a6f9f3fd6..366f98f0c 100644 --- a/tests/scripts/codex-hooks.test.js +++ b/tests/scripts/codex-hooks.test.js @@ -328,27 +328,28 @@ function runHermeticPythonPrePush({ const initialized = spawnSync('git', ['init', '--quiet'], { cwd: projectDir }); assert.strictEqual(initialized.status, 0, initialized.stderr?.toString()); + const venvDir = venvName === null ? null : path.join(tempDir, venvName); + const venvPython = venvDir === null ? null : path.join(venvDir, 'bin', 'python'); + if (venvPython !== null) { + writeExecutable(venvPython, `#!/bin/sh\nprintf '%s\\n' "$0|$*" >> "${toBashPath(callsPath)}"\nexit 0\n`); + } + + const overrideStub = overrideVersionLine === null + ? null + : path.join(tempDir, 'bin', 'fake-pytest'); + if (overrideStub !== null) { + writeExecutable(overrideStub, `#!/bin/sh\nif [ "$1" = "--version" ]; then printf '%s\\n' '${overrideVersionLine}'; exit 0; fi\nprintf '%s\\n' "$0|$*" >> "${toBashPath(callsPath)}"\nexit 0\n`); + } + + const override = overrideStub === null ? pytestCmd : toBashPath(overrideStub); const env = { ECC_SKIP_GIT_HOOKS: '0', ECC_SKIP_PREPUSH: '0', MSYS_NO_PATHCONV: '1', + ...(venvDir === null ? {} : { VIRTUAL_ENV: toBashPath(venvDir) }), + ...(override === null ? {} : { ECC_PYTEST_CMD: override }), }; - let venvPython = null; - if (venvName) { - venvPython = path.join(tempDir, venvName, 'bin', 'python'); - writeExecutable(venvPython, `#!/bin/sh\nprintf '%s\\n' "$0|$*" >> "${toBashPath(callsPath)}"\nexit 0\n`); - env.VIRTUAL_ENV = toBashPath(path.join(tempDir, venvName)); - } - - if (overrideVersionLine !== null) { - const stub = path.join(tempDir, 'bin', 'fake-pytest'); - writeExecutable(stub, `#!/bin/sh\nif [ "$1" = "--version" ]; then printf '%s\\n' '${overrideVersionLine}'; exit 0; fi\nprintf '%s\\n' "$0|$*" >> "${toBashPath(callsPath)}"\nexit 0\n`); - env.ECC_PYTEST_CMD = toBashPath(stub); - } else if (pytestCmd !== null) { - env.ECC_PYTEST_CMD = pytestCmd; - } - const result = runBash(prePushHook, { env, cwd: projectDir, From c6195edb2f99de947338292b7ef5142407fa7ffd Mon Sep 17 00:00:00 2001 From: Juan Garibay Date: Thu, 17 Sep 2026 16:19:57 -0400 Subject: [PATCH 4/9] fix(hooks): stop probing the pytest override, and stop failing on exit 5 Three defects, found by reviewing this branch against a running pytest rather than by reading it. Exit 5 is not a failure. pytest reserves it for NO_TESTS_COLLECTED, and `|| fail "pytest failed"` collapsed it into a blocked push. The `|| fail` predates this branch, but this branch is what makes it reachable: a repository whose pyproject.toml only configures ruff or black, with pytest in its venv and no test files, used to hit the "pytest is not installed" skip and now gets gated. $VIRTUAL_ENV is the first candidate, so merely having a venv activated in the pushing shell drags any requirements.txt repository into this path, and the hook is installed globally. Reproduced with pytest 9.1.1. Exit 5 is now non-blocking but loud -- a bad rootdir, testpaths or an unimportable conftest also collects nothing, and swallowing that silently would reopen the hole this resolver exists to close. Other non-zero codes now carry the code, because 1 (tests failed) and 4 (usage error) call for different responses. The ECC_PYTEST_CMD probe ran the operator's command. Validating the override with `--version` assumed it would answer like pytest. A wrapper that sets an environment variable and execs pytest ignores the flag and runs the whole suite, so the probe executed the tests, then rejected the command for not printing a version, then blocked the push -- with the suite green. That is worse than the silent gate the probe was added to close, so the override is taken as given again: it is a deliberate setting, the hook cannot inspect it without running it, and pointing it at something that is not pytest is the operator's call. `is_pytest` still guards the PATH candidate, which this script composes itself, where `pytest --version` is harmless. An empty override still fails closed. The tests inherited the ambient environment. `runHermeticPythonPrePush` passed process.env through, so an exported ECC_PYTEST_CMD or an activated virtualenv resolved a pytest the fixture never created and the venv test failed for anyone who runs the suite that way. Both variables are now neutralised in the base env. Coverage: the gate had no test proving it blocks. Changing the run line to `|| true` left all three previous tests green. Seven now cover a spaced venv path, a red suite, exit 5, an override invoked exactly once with no probe, an empty override, and the PATH candidate in both directions. --- scripts/codex-git-hooks/pre-push | 47 +++++++++++---- tests/scripts/codex-hooks.test.js | 98 ++++++++++++++++++++++++++----- 2 files changed, 117 insertions(+), 28 deletions(-) diff --git a/scripts/codex-git-hooks/pre-push b/scripts/codex-git-hooks/pre-push index 806d616cd..726f3916d 100755 --- a/scripts/codex-git-hooks/pre-push +++ b/scripts/codex-git-hooks/pre-push @@ -136,6 +136,11 @@ PYTEST_CMD=() # plenty of programs take it and exit 0 -- so the output has to name pytest. The # version is captured rather than piped: under `set -o pipefail` a `| grep -q` can # report the SIGPIPE of the program it just matched. +# +# Only ever called on a command this script composed itself. Probing an arbitrary +# operator-supplied command is not safe: a wrapper that ignores `--version` and +# execs pytest runs the entire suite during the probe, and is then rejected for +# not having printed a version. is_pytest() { local version version="$("$@" --version 2>&1)" || return 1 @@ -144,17 +149,15 @@ is_pytest() { resolve_pytest() { if [[ -n "${ECC_PYTEST_CMD:-}" ]]; then - # Word-split, so the override names a command on PATH or an interpreter whose - # path has no spaces; a venv with spaces in its name is found by the loop below. + # Taken as given. This is a deliberate override, and the hook cannot inspect it + # without running it -- a wrapper script may ignore `--version` and run the + # suite, so probing costs a duplicate test run and then blocks the push anyway. + # Pointing this at something that is not pytest turns the gate off, and that is + # the operator's call to make, not a misconfiguration for the hook to second + # guess. Word-split, so the command names something on PATH or an interpreter + # whose path has no spaces; a venv with spaces is found by the loop below. read -r -a PYTEST_CMD <<<"$ECC_PYTEST_CMD" || true - # Checked like every other candidate, and fatally rather than by falling - # through: an operator who set this asked for that command, and quietly running - # a different one would hide the misconfiguration. `ECC_PYTEST_CMD=true` would - # otherwise run `true -q`, pass, and report a Python project verified by - # nothing -- the same silent gate this resolver exists to remove. - if [[ ${#PYTEST_CMD[@]} -eq 0 ]] || ! is_pytest "${PYTEST_CMD[@]}"; then - fail "ECC_PYTEST_CMD is set to '$ECC_PYTEST_CMD', which does not run pytest" - fi + [[ ${#PYTEST_CMD[@]} -gt 0 ]] || fail "ECC_PYTEST_CMD is set but empty" return 0 fi local venv @@ -178,8 +181,8 @@ resolve_pytest() { return 0 fi fi - # `command -v` proves only that a file of that name exists on PATH, which is why - # this candidate is confirmed too before it is accepted. + # `command -v` proves only that a file of that name exists on PATH. This one the + # script composed itself, so confirming it costs a harmless `pytest --version`. if command -v pytest >/dev/null 2>&1 && is_pytest pytest; then PYTEST_CMD=(pytest) return 0 @@ -192,7 +195,25 @@ if [[ -f "pyproject.toml" || -f "requirements.txt" ]]; then if resolve_pytest; then ran_any_check=1 log "Python project detected. Running: ${PYTEST_CMD[*]} -q" - "${PYTEST_CMD[@]}" -q || fail "pytest failed" + pytest_status=0 + "${PYTEST_CMD[@]}" -q || pytest_status=$? + case "$pytest_status" in + 0) ;; + # pytest reserves 5 for NO_TESTS_COLLECTED, which is not a red suite. A + # pyproject.toml that only configures ruff or black is still a Python project + # by this hook's test, and blocking those pushes would make the gate something + # people switch off. Never silent, though: a bad rootdir, testpaths or a + # conftest that fails to import also collects nothing, and swallowing that is + # the same skip-reads-like-a-pass hole this resolver exists to close. + 5) + log "pytest collected no tests (exit 5). Not gating this push." + log " If this repository is supposed to have tests, that is the bug:" + log " check rootdir, testpaths, and conftest.py import errors." + ;; + # The code is in the message because 1 (tests failed) and 4 (usage error) + # need different responses, and "pytest failed" alone cannot tell them apart. + *) fail "pytest failed (exit $pytest_status)" ;; + esac else log "Python project detected but no pytest found (checked \$VIRTUAL_ENV, .venv," log " venv, env, uv, poetry, PATH). Set ECC_PYTEST_CMD to point at it." diff --git a/tests/scripts/codex-hooks.test.js b/tests/scripts/codex-hooks.test.js index 366f98f0c..fd11fd691 100644 --- a/tests/scripts/codex-hooks.test.js +++ b/tests/scripts/codex-hooks.test.js @@ -317,8 +317,10 @@ function writeExecutable(filePath, body) { // is actually about. function runHermeticPythonPrePush({ venvName = null, + venvExit = 0, pytestCmd = null, - overrideVersionLine = null, + overrideStub = false, + pathPytestVersionLine = null, } = {}) { const tempDir = createTempDir('codex-pre-push-py-'); const projectDir = path.join(tempDir, 'project'); @@ -328,26 +330,48 @@ function runHermeticPythonPrePush({ const initialized = spawnSync('git', ['init', '--quiet'], { cwd: projectDir }); assert.strictEqual(initialized.status, 0, initialized.stderr?.toString()); + // Every stub records the argv it was handed. That record is the assertion: it is + // how a test tells a preserved path from a split one, and a command that was run + // once from one the hook probed first. + const record = `printf '%s\\n' "$0|$*" >> "${toBashPath(callsPath)}"`; + const venvDir = venvName === null ? null : path.join(tempDir, venvName); const venvPython = venvDir === null ? null : path.join(venvDir, 'bin', 'python'); if (venvPython !== null) { - writeExecutable(venvPython, `#!/bin/sh\nprintf '%s\\n' "$0|$*" >> "${toBashPath(callsPath)}"\nexit 0\n`); + writeExecutable(venvPython, `#!/bin/sh\n${record}\nif [ "$1" = "-c" ]; then exit 0; fi\nexit ${venvExit}\n`); } - const overrideStub = overrideVersionLine === null - ? null - : path.join(tempDir, 'bin', 'fake-pytest'); - if (overrideStub !== null) { - writeExecutable(overrideStub, `#!/bin/sh\nif [ "$1" = "--version" ]; then printf '%s\\n' '${overrideVersionLine}'; exit 0; fi\nprintf '%s\\n' "$0|$*" >> "${toBashPath(callsPath)}"\nexit 0\n`); + // Deliberately does NOT special-case --version: an operator's wrapper would not + // either, and the recorded calls are what prove the hook never probed it. + const overrideStubPath = overrideStub ? path.join(tempDir, 'bin', 'wrapper') : null; + if (overrideStubPath !== null) { + writeExecutable(overrideStubPath, `#!/bin/sh\n${record}\nexit 0\n`); } - const override = overrideStub === null ? pytestCmd : toBashPath(overrideStub); + const pathBin = pathPytestVersionLine === null ? null : path.join(tempDir, 'pathbin'); + if (pathBin !== null) { + writeExecutable( + path.join(pathBin, 'pytest'), + `#!/bin/sh\nif [ "$1" = "--version" ]; then printf '%s\\n' '${pathPytestVersionLine}'; exit 0; fi\n${record}\nexit 0\n`, + ); + } + + const override = overrideStubPath === null ? pytestCmd : toBashPath(overrideStubPath); const env = { + // The hook reads both of these from the ambient environment. Inherited, a + // developer running this suite inside an activated virtualenv, or with an + // ECC_PYTEST_CMD exported, would resolve a pytest the fixture never created, + // and these tests would pass or fail depending on whose shell ran them. + VIRTUAL_ENV: '', + ECC_PYTEST_CMD: '', ECC_SKIP_GIT_HOOKS: '0', ECC_SKIP_PREPUSH: '0', MSYS_NO_PATHCONV: '1', ...(venvDir === null ? {} : { VIRTUAL_ENV: toBashPath(venvDir) }), ...(override === null ? {} : { ECC_PYTEST_CMD: override }), + ...(pathBin === null + ? {} + : { PATH: `${toBashPath(pathBin)}${path.delimiter}${process.env.PATH}` }), }; const result = runBash(prePushHook, { @@ -359,7 +383,7 @@ function runHermeticPythonPrePush({ ? fs.readFileSync(callsPath, 'utf8').trim().split(/\r?\n/).filter(Boolean) : []; cleanup(tempDir); - return { result, calls, venvPython }; + return { result, calls, venvPython, overrideStubPath }; } if ( @@ -377,11 +401,10 @@ if ( else failed++; if ( - test('pre-push rejects an ECC_PYTEST_CMD that does not run pytest', () => { - const { result, calls } = runHermeticPythonPrePush({ pytestCmd: 'true' }); + test('pre-push blocks the push when the resolved pytest fails', () => { + const { result } = runHermeticPythonPrePush({ venvName: 'venv-red', venvExit: 1 }); assert.notStrictEqual(result.status, 0, `${result.stdout}\n${result.stderr}`); - assert.match(result.stderr, /ECC_PYTEST_CMD is set to 'true', which does not run pytest/); - assert.deepStrictEqual(calls, []); + assert.match(result.stderr, /pytest failed \(exit 1\)/); assert.doesNotMatch(result.stdout, /Verification checks passed/); }) ) @@ -389,8 +412,52 @@ if ( else failed++; if ( - test('pre-push runs an ECC_PYTEST_CMD override that identifies itself as pytest', () => { - const { result, calls } = runHermeticPythonPrePush({ overrideVersionLine: 'pytest 8.0.0' }); + test('pre-push does not block when pytest collected no tests (exit 5)', () => { + const { result } = runHermeticPythonPrePush({ venvName: 'venv-empty', venvExit: 5 }); + assert.strictEqual(result.status, 0, `${result.stdout}\n${result.stderr}`); + assert.match(result.stdout, /collected no tests \(exit 5\)/); + assert.match(result.stdout, /rootdir, testpaths, and conftest\.py/); + }) +) + passed++; +else failed++; + +if ( + test('pre-push runs an ECC_PYTEST_CMD override exactly once, without probing it', () => { + const { result, calls, overrideStubPath } = runHermeticPythonPrePush({ overrideStub: true }); + assert.strictEqual(result.status, 0, `${result.stdout}\n${result.stderr}`); + assert.deepStrictEqual(calls, [`${toBashPath(overrideStubPath)}|-q`], JSON.stringify(calls)); + }) +) + passed++; +else failed++; + +if ( + test('pre-push fails closed when ECC_PYTEST_CMD is set to whitespace', () => { + const { result } = runHermeticPythonPrePush({ pytestCmd: ' ' }); + assert.notStrictEqual(result.status, 0, `${result.stdout}\n${result.stderr}`); + assert.match(result.stderr, /ECC_PYTEST_CMD is set but empty/); + }) +) + passed++; +else failed++; + +if ( + test('pre-push rejects a PATH pytest that does not identify itself as pytest', () => { + const { result, calls } = runHermeticPythonPrePush({ + pathPytestVersionLine: 'true (GNU coreutils) 9.0', + }); + assert.strictEqual(result.status, 0, `${result.stdout}\n${result.stderr}`); + assert.match(result.stdout, /no pytest found/); + assert.deepStrictEqual(calls, []); + }) +) + passed++; +else failed++; + +if ( + test('pre-push accepts a PATH pytest that reports a pytest version', () => { + const { result, calls } = runHermeticPythonPrePush({ pathPytestVersionLine: 'pytest 8.0.0' }); assert.strictEqual(result.status, 0, `${result.stdout}\n${result.stderr}`); assert.strictEqual(calls.length, 1, JSON.stringify(calls)); assert.match(calls[0], /\|-q$/); @@ -399,6 +466,7 @@ if ( passed++; else failed++; + if ( test('check-plugin-cache fails when the installed cache is missing manifest-referenced files', () => { const homeDir = createTempDir('codex-plugin-cache-home-'); From 08b173f12f41fff38e8b5c6367b17295b30044cb Mon Sep 17 00:00:00 2001 From: Juan Garibay Date: Thu, 17 Sep 2026 16:33:37 -0400 Subject: [PATCH 5/9] fix(hooks): a blank ECC_PYTEST_CMD is an override, and say when one is in use `[[ -n "${ECC_PYTEST_CMD:-}" ]]` asked whether the variable had a value, not whether it was set, so `ECC_PYTEST_CMD=` fell through to virtualenv discovery while `ECC_PYTEST_CMD=" "` failed the push. Two spellings of the same mistake, two behaviours. Falling through is the wrong one: an override that evaluated to nothing -- a command substitution that found no pytest, say -- then silently ran a different runner than the operator named, which is exactly the substitution this resolver refuses to make anywhere else. Both now fail closed. `${ECC_PYTEST_CMD+set}` rather than `[[ -v ECC_PYTEST_CMD ]]`, because `-v` is bash 4.2 and a stock macOS /bin/bash is 3.2, where it is not a false but a syntax error. The hook runs under whatever `env bash` resolves to. The override is still not probed -- probing runs the operator's command, and a wrapper that ignores `--version` executes the whole suite and is then rejected for not printing a version. What the gate can honestly do about a stale override is refuse to be quiet about it, so a push that uses one now says so, every time, and says the hook has not checked that it is pytest. A bypass that announces itself is not the silent gate this resolver exists to prevent. The fixture env is built from nothing instead of inheriting process.env with two keys blanked. Blanking is no longer neutral: a blanked ECC_PYTEST_CMD is now an override, and every one of these tests would have taken that branch. --- scripts/codex-git-hooks/pre-push | 23 ++++++++++++-- tests/scripts/codex-hooks.test.js | 50 ++++++++++++++++++++----------- 2 files changed, 53 insertions(+), 20 deletions(-) diff --git a/scripts/codex-git-hooks/pre-push b/scripts/codex-git-hooks/pre-push index 726f3916d..82ed82194 100755 --- a/scripts/codex-git-hooks/pre-push +++ b/scripts/codex-git-hooks/pre-push @@ -148,7 +148,18 @@ is_pytest() { } resolve_pytest() { - if [[ -n "${ECC_PYTEST_CMD:-}" ]]; then + # `${VAR+set}` rather than `-n "${VAR:-}"`, so that a variable set to nothing is + # still an override: `ECC_PYTEST_CMD=` and `ECC_PYTEST_CMD=" "` now behave + # alike, where the first used to fall through to discovery and the second failed + # the push. Falling through is the wrong half of that pair -- an override that + # evaluated empty (a command substitution that found nothing, say) would silently + # run a different runner than the operator asked for, which is the substitution + # this resolver refuses to make anywhere else. + # + # Not `[[ -v ECC_PYTEST_CMD ]]`: that is bash 4.2, and a stock macOS `/bin/bash` + # is 3.2, where it is a syntax error rather than a false. This hook ships to + # whatever `env bash` finds. + if [[ -n "${ECC_PYTEST_CMD+set}" ]]; then # Taken as given. This is a deliberate override, and the hook cannot inspect it # without running it -- a wrapper script may ignore `--version` and run the # suite, so probing costs a duplicate test run and then blocks the push anyway. @@ -157,7 +168,7 @@ resolve_pytest() { # guess. Word-split, so the command names something on PATH or an interpreter # whose path has no spaces; a venv with spaces is found by the loop below. read -r -a PYTEST_CMD <<<"$ECC_PYTEST_CMD" || true - [[ ${#PYTEST_CMD[@]} -gt 0 ]] || fail "ECC_PYTEST_CMD is set but empty" + [[ ${#PYTEST_CMD[@]} -gt 0 ]] || fail "ECC_PYTEST_CMD is set but names no command" return 0 fi local venv @@ -195,6 +206,14 @@ if [[ -f "pyproject.toml" || -f "requirements.txt" ]]; then if resolve_pytest; then ran_any_check=1 log "Python project detected. Running: ${PYTEST_CMD[*]} -q" + if [[ -n "${ECC_PYTEST_CMD+set}" ]]; then + # resolve_pytest deliberately does not verify the override is pytest, because + # probing it can run the operator's suite. What this gate can honestly do + # about a stale override is refuse to be quiet about it: a bypass announced + # on every push is not the silent gate this resolver exists to prevent. + log " via ECC_PYTEST_CMD -- the hook runs what you pointed it at, and does" + log " not check that it is pytest. Unset it to gate on the real suite." + fi pytest_status=0 "${PYTEST_CMD[@]}" -q || pytest_status=$? case "$pytest_status" in diff --git a/tests/scripts/codex-hooks.test.js b/tests/scripts/codex-hooks.test.js index fd11fd691..2e3cd9648 100644 --- a/tests/scripts/codex-hooks.test.js +++ b/tests/scripts/codex-hooks.test.js @@ -357,26 +357,28 @@ function runHermeticPythonPrePush({ } const override = overrideStubPath === null ? pytestCmd : toBashPath(overrideStubPath); + // Built from nothing rather than from process.env. The hook reads VIRTUAL_ENV and + // ECC_PYTEST_CMD from the ambient environment, so a developer running this suite + // inside an activated virtualenv, or with ECC_PYTEST_CMD exported, would resolve a + // pytest the fixture never created. Omitted, not blanked: now that a variable set + // to nothing is itself an override, blanking it here would make every one of these + // tests take that branch. const env = { - // The hook reads both of these from the ambient environment. Inherited, a - // developer running this suite inside an activated virtualenv, or with an - // ECC_PYTEST_CMD exported, would resolve a pytest the fixture never created, - // and these tests would pass or fail depending on whose shell ran them. - VIRTUAL_ENV: '', - ECC_PYTEST_CMD: '', + PATH: pathBin === null + ? process.env.PATH + : `${toBashPath(pathBin)}${path.delimiter}${process.env.PATH}`, + HOME: process.env.HOME ?? '', ECC_SKIP_GIT_HOOKS: '0', ECC_SKIP_PREPUSH: '0', MSYS_NO_PATHCONV: '1', ...(venvDir === null ? {} : { VIRTUAL_ENV: toBashPath(venvDir) }), ...(override === null ? {} : { ECC_PYTEST_CMD: override }), - ...(pathBin === null - ? {} - : { PATH: `${toBashPath(pathBin)}${path.delimiter}${process.env.PATH}` }), }; const result = runBash(prePushHook, { env, cwd: projectDir, + preservePath: false, input: Buffer.from('refs/heads/main 1111111111111111111111111111111111111111 refs/heads/main 0000000000000000000000000000000000000000\n'), }); const calls = fs.existsSync(callsPath) @@ -427,20 +429,32 @@ if ( const { result, calls, overrideStubPath } = runHermeticPythonPrePush({ overrideStub: true }); assert.strictEqual(result.status, 0, `${result.stdout}\n${result.stderr}`); assert.deepStrictEqual(calls, [`${toBashPath(overrideStubPath)}|-q`], JSON.stringify(calls)); + // The override is not verified to be pytest, so it must at least be loud. + assert.match(result.stdout, /via ECC_PYTEST_CMD/); + assert.match(result.stdout, /does\n?.*not check that it is pytest/s); }) ) passed++; else failed++; -if ( - test('pre-push fails closed when ECC_PYTEST_CMD is set to whitespace', () => { - const { result } = runHermeticPythonPrePush({ pytestCmd: ' ' }); - assert.notStrictEqual(result.status, 0, `${result.stdout}\n${result.stderr}`); - assert.match(result.stderr, /ECC_PYTEST_CMD is set but empty/); - }) -) - passed++; -else failed++; +// Both blank forms, because they used to disagree: an unquoted empty value fell +// through to discovery while whitespace failed the push. A venv is present so a +// fall-through would be visible as a pass rather than as an absence. +for (const [label, blank] of [['empty', ''], ['whitespace', ' ']]) { + if ( + test(`pre-push fails closed when ECC_PYTEST_CMD is set to ${label}`, () => { + const { result, calls } = runHermeticPythonPrePush({ + venvName: 'venv-blank', + pytestCmd: blank, + }); + assert.notStrictEqual(result.status, 0, `${result.stdout}\n${result.stderr}`); + assert.match(result.stderr, /ECC_PYTEST_CMD is set but names no command/); + assert.deepStrictEqual(calls, [], JSON.stringify(calls)); + }) + ) + passed++; + else failed++; +} if ( test('pre-push rejects a PATH pytest that does not identify itself as pytest', () => { From 9cdc40e6d1aadc4f6833a59a235acf46d6212ad4 Mon Sep 17 00:00:00 2001 From: Juan Garibay Date: Thu, 17 Sep 2026 16:44:33 -0400 Subject: [PATCH 6/9] fix(hooks): do not run a virtualenv interpreter the repository ships This branch taught the hook to run `.venv/bin/python`, and that is a binary the repository can supply. On main the Python arm only ever ran `pytest` from PATH -- the developer's own -- and on a machine without one it ran nothing at all, which is exactly the machine this branch was written for. So the exposure is new, and it arrived with the fix. The hook is installed globally through core.hooksPath. Cloning a hostile repository, committing nothing, and pushing it to your own fork is enough: the pre-push hook finds the committed `.venv/bin/python`, runs it once to probe for pytest and again to run the suite. Reproduced -- the planted executable logged two invocations under the previous commit and none under this one. A virtualenv is never committed. It is platform-specific binaries and every Python project gitignores it, so `git ls-files --error-unmatch` separates the two cases exactly: a developer's own venv is untracked and still resolves, a tracked one is skipped with the reason printed. An absolute $VIRTUAL_ENV outside the worktree reads as untracked, as it should. Not addressed here, and worth a maintainer's view: `uv run` and `poetry run` resolve from the repository's own lockfile, so they carry the same shape of trust in a form this check cannot see. They are gated behind a lockfile being present, and changing their semantics is a larger decision than this fix. --- scripts/codex-git-hooks/pre-push | 14 ++++++++++++++ tests/scripts/codex-hooks.test.js | 24 ++++++++++++++++++++++-- 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/scripts/codex-git-hooks/pre-push b/scripts/codex-git-hooks/pre-push index 82ed82194..6a3d46152 100755 --- a/scripts/codex-git-hooks/pre-push +++ b/scripts/codex-git-hooks/pre-push @@ -174,6 +174,20 @@ resolve_pytest() { local venv for venv in "${VIRTUAL_ENV:-}" .venv venv env; do if [[ -n "$venv" && -x "$venv/bin/python" ]]; then + # A virtualenv is never committed -- it is platform-specific binaries, and + # every Python project gitignores it. One that IS tracked is the repository + # handing this hook an executable and asking it to run. The hook is installed + # globally, so cloning a hostile repository and pushing it to your own fork + # would be enough, and on a machine with no pytest on PATH this arm is the + # only thing that would run at all. Skipping costs nothing legitimate, + # because a developer's own venv is untracked -- and it says why rather than + # going quiet about it. + if git ls-files --error-unmatch -- "$venv/bin/python" >/dev/null 2>&1; then + log "Ignoring $venv/bin/python: it is tracked in this repository." + log " A committed virtualenv is an executable the repository controls, and" + log " this hook runs on every push in every repository." + continue + fi if "$venv/bin/python" -c "import pytest" >/dev/null 2>&1; then PYTEST_CMD=("$venv/bin/python" -m pytest) return 0 diff --git a/tests/scripts/codex-hooks.test.js b/tests/scripts/codex-hooks.test.js index 2e3cd9648..42bdbda68 100644 --- a/tests/scripts/codex-hooks.test.js +++ b/tests/scripts/codex-hooks.test.js @@ -318,6 +318,7 @@ function writeExecutable(filePath, body) { function runHermeticPythonPrePush({ venvName = null, venvExit = 0, + trackVenv = false, pytestCmd = null, overrideStub = false, pathPytestVersionLine = null, @@ -335,10 +336,18 @@ function runHermeticPythonPrePush({ // once from one the hook probed first. const record = `printf '%s\\n' "$0|$*" >> "${toBashPath(callsPath)}"`; - const venvDir = venvName === null ? null : path.join(tempDir, venvName); + // A tracked venv has to live inside the repository to be trackable at all, and is + // found by directory-name discovery rather than by VIRTUAL_ENV. + const venvDir = venvName === null ? null : path.join(trackVenv ? projectDir : tempDir, venvName); const venvPython = venvDir === null ? null : path.join(venvDir, 'bin', 'python'); if (venvPython !== null) { writeExecutable(venvPython, `#!/bin/sh\n${record}\nif [ "$1" = "-c" ]; then exit 0; fi\nexit ${venvExit}\n`); + if (trackVenv) { + // Staged, not committed: `git ls-files` reads the index, so this is enough to + // make the file repository-controlled without needing a committer identity. + const added = spawnSync('git', ['add', '-f', '--', venvPython], { cwd: projectDir }); + assert.strictEqual(added.status, 0, added.stderr?.toString()); + } } // Deliberately does NOT special-case --version: an operator's wrapper would not @@ -371,7 +380,7 @@ function runHermeticPythonPrePush({ ECC_SKIP_GIT_HOOKS: '0', ECC_SKIP_PREPUSH: '0', MSYS_NO_PATHCONV: '1', - ...(venvDir === null ? {} : { VIRTUAL_ENV: toBashPath(venvDir) }), + ...(venvDir === null || trackVenv ? {} : { VIRTUAL_ENV: toBashPath(venvDir) }), ...(override === null ? {} : { ECC_PYTEST_CMD: override }), }; @@ -402,6 +411,17 @@ if ( passed++; else failed++; +if ( + test('pre-push refuses to run a virtualenv python that the repository tracks', () => { + const { result, calls } = runHermeticPythonPrePush({ venvName: '.venv', trackVenv: true }); + assert.strictEqual(result.status, 0, `${result.stdout}\n${result.stderr}`); + assert.deepStrictEqual(calls, [], JSON.stringify(calls)); + assert.match(result.stdout, /it is tracked in this repository/); + }) +) + passed++; +else failed++; + if ( test('pre-push blocks the push when the resolved pytest fails', () => { const { result } = runHermeticPythonPrePush({ venvName: 'venv-red', venvExit: 1 }); From ca1a5ad8bc612979059b4ad88d9db5030e778de9 Mon Sep 17 00:00:00 2001 From: Juan Garibay Date: Thu, 17 Sep 2026 16:51:43 -0400 Subject: [PATCH 7/9] fix(hooks): name the remedy in the blank-override failure The hook is global and this message blocks a push, so "ECC_PYTEST_CMD is set but names no command" left the operator holding a refusal with no next step. It now says to point the variable at a runner or unset it to fall back to discovery, which is the same advice the no-pytest-found branch already gives from the other direction. --- scripts/codex-git-hooks/pre-push | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/scripts/codex-git-hooks/pre-push b/scripts/codex-git-hooks/pre-push index 6a3d46152..6f04c2680 100755 --- a/scripts/codex-git-hooks/pre-push +++ b/scripts/codex-git-hooks/pre-push @@ -168,7 +168,8 @@ resolve_pytest() { # guess. Word-split, so the command names something on PATH or an interpreter # whose path has no spaces; a venv with spaces is found by the loop below. read -r -a PYTEST_CMD <<<"$ECC_PYTEST_CMD" || true - [[ ${#PYTEST_CMD[@]} -gt 0 ]] || fail "ECC_PYTEST_CMD is set but names no command" + [[ ${#PYTEST_CMD[@]} -gt 0 ]] || fail "ECC_PYTEST_CMD is set but names no command.\ + Point it at your test runner, or unset it to fall back to discovery." return 0 fi local venv From 3c317470163b62345935125aecdeb037cba88dd8 Mon Sep 17 00:00:00 2001 From: Juan Garibay Date: Thu, 17 Sep 2026 16:55:11 -0400 Subject: [PATCH 8/9] fix(hooks): resolve the venv path before asking git whether it is tracked The guard added in 9cdc40e6 was incomplete. `git ls-files` reports paths as they are indexed and does not follow symlinks, so a repository that commits `.venv` as a symlink to its own root alongside a tracked `bin/python` gets asked about `.venv/bin/python` -- a path git has never heard of -- and the answer is "untracked". The interpreter then runs. Measured on that shape: the planted executable logged two invocations against 9cdc40e6 and none against this commit. `repo_ships_interpreter` now resolves the bin directory with `cd -P`/`pwd -P`, resolves the worktree root the same way, and asks git about the resolved path relative to it. The three cases that matter all hold: a plainly committed venv is still refused, the symlink shape is now refused, and a developer's own untracked venv still resolves and runs. `cd -P`/`pwd -P` rather than `realpath` or `readlink -f`, because neither is portable to a stock macOS. --- scripts/codex-git-hooks/pre-push | 37 ++++++++++++++++++++++--------- tests/scripts/codex-hooks.test.js | 24 +++++++++++++++++++- 2 files changed, 50 insertions(+), 11 deletions(-) diff --git a/scripts/codex-git-hooks/pre-push b/scripts/codex-git-hooks/pre-push index 6f04c2680..dfeeab4fd 100755 --- a/scripts/codex-git-hooks/pre-push +++ b/scripts/codex-git-hooks/pre-push @@ -147,6 +147,31 @@ is_pytest() { grep -qiE 'pytest[[:space:]]+(version[[:space:]]+)?[0-9]' <<<"$version" } +# Does the repository itself ship this interpreter? +# +# A virtualenv is never committed -- it is platform-specific binaries, and every +# Python project gitignores it. One that IS tracked is the repository handing this +# hook an executable and asking it to run. The hook is installed globally, so +# cloning a hostile repository and pushing it to your own fork would be enough, +# and on a machine with no pytest on PATH this arm is the only thing that would +# run at all. A developer's own venv is untracked, so nothing legitimate is lost. +# +# The path is resolved through symlinks before git is asked, because `git ls-files` +# reports paths as indexed and does not follow links. A repository that commits +# `.venv` as a symlink to `.` next to a tracked `bin/python` would otherwise be +# queried for `.venv/bin/python`, a path git has never heard of, and the answer +# would be "untracked". Measured: that shape ran the planted binary twice. +repo_ships_interpreter() { + local bindir real top + bindir="$(cd -P -- "$1" 2>/dev/null && pwd -P)" || return 1 + [[ -n "$bindir" ]] || return 1 + real="$bindir/python" + top="$(git rev-parse --show-toplevel 2>/dev/null)" || return 1 + top="$(cd -P -- "$top" 2>/dev/null && pwd -P)" || return 1 + [[ -n "$top" && "$real" == "$top/"* ]] || return 1 + git ls-files --error-unmatch -- "${real#"$top"/}" >/dev/null 2>&1 +} + resolve_pytest() { # `${VAR+set}` rather than `-n "${VAR:-}"`, so that a variable set to nothing is # still an override: `ECC_PYTEST_CMD=` and `ECC_PYTEST_CMD=" "` now behave @@ -175,16 +200,8 @@ resolve_pytest() { local venv for venv in "${VIRTUAL_ENV:-}" .venv venv env; do if [[ -n "$venv" && -x "$venv/bin/python" ]]; then - # A virtualenv is never committed -- it is platform-specific binaries, and - # every Python project gitignores it. One that IS tracked is the repository - # handing this hook an executable and asking it to run. The hook is installed - # globally, so cloning a hostile repository and pushing it to your own fork - # would be enough, and on a machine with no pytest on PATH this arm is the - # only thing that would run at all. Skipping costs nothing legitimate, - # because a developer's own venv is untracked -- and it says why rather than - # going quiet about it. - if git ls-files --error-unmatch -- "$venv/bin/python" >/dev/null 2>&1; then - log "Ignoring $venv/bin/python: it is tracked in this repository." + if repo_ships_interpreter "$venv/bin"; then + log "Ignoring $venv/bin/python: the repository ships it." log " A committed virtualenv is an executable the repository controls, and" log " this hook runs on every push in every repository." continue diff --git a/tests/scripts/codex-hooks.test.js b/tests/scripts/codex-hooks.test.js index 42bdbda68..11f8d0115 100644 --- a/tests/scripts/codex-hooks.test.js +++ b/tests/scripts/codex-hooks.test.js @@ -319,6 +319,7 @@ function runHermeticPythonPrePush({ venvName = null, venvExit = 0, trackVenv = false, + trackedSymlinkVenv = false, pytestCmd = null, overrideStub = false, pathPytestVersionLine = null, @@ -350,6 +351,16 @@ function runHermeticPythonPrePush({ } } + // The shape that defeats a naive `git ls-files -- .venv/bin/python` check: the + // repository commits `.venv` as a symlink to its own root plus a tracked + // `bin/python`, so git is asked about a path it has never indexed. + if (trackedSymlinkVenv) { + writeExecutable(path.join(projectDir, 'bin', 'python'), `#!/bin/sh\n${record}\nexit 0\n`); + fs.symlinkSync('.', path.join(projectDir, '.venv')); + const added = spawnSync('git', ['add', '-f', '--', 'bin/python', '.venv'], { cwd: projectDir }); + assert.strictEqual(added.status, 0, added.stderr?.toString()); + } + // Deliberately does NOT special-case --version: an operator's wrapper would not // either, and the recorded calls are what prove the hook never probed it. const overrideStubPath = overrideStub ? path.join(tempDir, 'bin', 'wrapper') : null; @@ -416,7 +427,18 @@ if ( const { result, calls } = runHermeticPythonPrePush({ venvName: '.venv', trackVenv: true }); assert.strictEqual(result.status, 0, `${result.stdout}\n${result.stderr}`); assert.deepStrictEqual(calls, [], JSON.stringify(calls)); - assert.match(result.stdout, /it is tracked in this repository/); + assert.match(result.stdout, /the repository ships it/); + }) +) + passed++; +else failed++; + +if ( + test('pre-push refuses a tracked interpreter reached through a committed symlink', () => { + const { result, calls } = runHermeticPythonPrePush({ trackedSymlinkVenv: true }); + assert.strictEqual(result.status, 0, `${result.stdout}\n${result.stderr}`); + assert.deepStrictEqual(calls, [], JSON.stringify(calls)); + assert.match(result.stdout, /the repository ships it/); }) ) passed++; From 4869db30c45c983000d7e4beff5853af3468dd53 Mon Sep 17 00:00:00 2001 From: Juan Garibay Date: Thu, 17 Sep 2026 17:17:19 -0400 Subject: [PATCH 9/9] fix(hooks): match the index case-insensitively, and isolate the pytest probe Red-teaming the guard from 3c317470 found two more ways to get a repository's own code executed. Both are demonstrated by a planted binary that appends to a witness file, counted before and after. Case folding. git matches index pathspecs case-sensitively even where core.ignorecase is set, but APFS does not -- so a repository that commits `.venv/bin/Python` gets `$venv/bin/python` opening and running that file while the guard's lowercase query finds nothing in the index and reports it untracked. The witness logged two invocations. It applies to `venv` and `env` as well, and to any folding of the name. The query now uses a `:(icase)` pathspec; all nine directory-by-spelling combinations are refused, and an untracked venv still runs. Module shadowing. `python -c "import pytest"` puts the working directory first on sys.path, so a repository that commits a `pytest.py` in its root has that file imported, and executed, by a check whose only job is to answer whether pytest is installed. The probe is now `python -I -c "import pytest"` on the virtualenv, uv and poetry paths alike. Isolation does not hide a real pytest -- it lives in the interpreter's own site-packages, confirmed against a venv holding pytest 9.1.1. Still true, and not something this hook can fix: running the repository's declared suite runs the repository's code. `pytest` imports conftest.py, and the Node arm runs package.json scripts. That is what a pre-push verification hook is for. The line this guard draws is narrower and worth keeping -- a capability probe, and the choice of which interpreter to trust, should not be things the pushed repository gets to decide. --- scripts/codex-git-hooks/pre-push | 19 +++++++++++++++---- tests/scripts/codex-hooks.test.js | 29 ++++++++++++++++++++++++++--- 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/scripts/codex-git-hooks/pre-push b/scripts/codex-git-hooks/pre-push index dfeeab4fd..ff66512a1 100755 --- a/scripts/codex-git-hooks/pre-push +++ b/scripts/codex-git-hooks/pre-push @@ -169,9 +169,20 @@ repo_ships_interpreter() { top="$(git rev-parse --show-toplevel 2>/dev/null)" || return 1 top="$(cd -P -- "$top" 2>/dev/null && pwd -P)" || return 1 [[ -n "$top" && "$real" == "$top/"* ]] || return 1 - git ls-files --error-unmatch -- "${real#"$top"/}" >/dev/null 2>&1 + # `:(icase)` because git matches index pathspecs case-sensitively even where + # core.ignorecase is set, while the filesystem underneath does not. On macOS's + # APFS -- the platform this hook most often runs on -- a committed + # `.venv/bin/Python` is what `$venv/bin/python` opens and executes, but a + # case-sensitive query for the lowercase name finds nothing in the index and the + # guard waves it through. Measured: that spelling ran the planted binary twice. + git ls-files --error-unmatch -- ":(icase)${real#"$top"/}" >/dev/null 2>&1 } +# `-I` isolates the probe: without it Python puts the working directory first on +# sys.path, so a repository that commits a `pytest.py` in its root gets that file +# imported -- and executed -- by a check whose only job is to answer whether pytest +# exists. Measured: a committed pytest.py ran during the probe. Isolation does not +# hide a real pytest, which lives in the interpreter's own site-packages. resolve_pytest() { # `${VAR+set}` rather than `-n "${VAR:-}"`, so that a variable set to nothing is # still an override: `ECC_PYTEST_CMD=` and `ECC_PYTEST_CMD=" "` now behave @@ -206,20 +217,20 @@ resolve_pytest() { log " this hook runs on every push in every repository." continue fi - if "$venv/bin/python" -c "import pytest" >/dev/null 2>&1; then + if "$venv/bin/python" -I -c "import pytest" >/dev/null 2>&1; then PYTEST_CMD=("$venv/bin/python" -m pytest) return 0 fi fi done if [[ -f "uv.lock" ]] && command -v uv >/dev/null 2>&1; then - if uv run --no-sync python -c "import pytest" >/dev/null 2>&1; then + if uv run --no-sync python -I -c "import pytest" >/dev/null 2>&1; then PYTEST_CMD=(uv run --no-sync pytest) return 0 fi fi if [[ -f "poetry.lock" ]] && command -v poetry >/dev/null 2>&1; then - if poetry run python -c "import pytest" >/dev/null 2>&1; then + if poetry run python -I -c "import pytest" >/dev/null 2>&1; then PYTEST_CMD=(poetry run pytest) return 0 fi diff --git a/tests/scripts/codex-hooks.test.js b/tests/scripts/codex-hooks.test.js index 11f8d0115..1171de52c 100644 --- a/tests/scripts/codex-hooks.test.js +++ b/tests/scripts/codex-hooks.test.js @@ -319,6 +319,7 @@ function runHermeticPythonPrePush({ venvName = null, venvExit = 0, trackVenv = false, + trackedVenvBasename = 'python', trackedSymlinkVenv = false, pytestCmd = null, overrideStub = false, @@ -340,9 +341,11 @@ function runHermeticPythonPrePush({ // A tracked venv has to live inside the repository to be trackable at all, and is // found by directory-name discovery rather than by VIRTUAL_ENV. const venvDir = venvName === null ? null : path.join(trackVenv ? projectDir : tempDir, venvName); - const venvPython = venvDir === null ? null : path.join(venvDir, 'bin', 'python'); + const venvPython = venvDir === null + ? null + : path.join(venvDir, 'bin', trackVenv ? trackedVenvBasename : 'python'); if (venvPython !== null) { - writeExecutable(venvPython, `#!/bin/sh\n${record}\nif [ "$1" = "-c" ]; then exit 0; fi\nexit ${venvExit}\n`); + writeExecutable(venvPython, `#!/bin/sh\n${record}\ncase " $* " in *" -c "*) exit 0 ;; esac\nexit ${venvExit}\n`); if (trackVenv) { // Staged, not committed: `git ls-files` reads the index, so this is enough to // make the file repository-controlled without needing a committer identity. @@ -414,7 +417,7 @@ if ( const python = toBashPath(venvPython); assert.strictEqual(result.status, 0, `${result.stdout}\n${result.stderr}`); assert.deepStrictEqual(calls, [ - `${python}|-c import pytest`, + `${python}|-I -c import pytest`, `${python}|-m pytest -q`, ], JSON.stringify({ calls, python, stdout: result.stdout, stderr: result.stderr }, null, 2)); }) @@ -433,6 +436,26 @@ if ( passed++; else failed++; +// A case-folded spelling, because macOS resolves `$venv/bin/python` to a committed +// `Python` while git matches index pathspecs case-sensitively. Skipped where the +// filesystem is case-sensitive and the two names cannot collide. +if (fs.existsSync(__filename.toUpperCase()) || fs.existsSync(__filename.toLowerCase())) { + if ( + test('pre-push refuses a tracked interpreter committed under a folded case', () => { + const { result, calls } = runHermeticPythonPrePush({ + venvName: '.venv', + trackVenv: true, + trackedVenvBasename: 'Python', + }); + assert.strictEqual(result.status, 0, `${result.stdout}\n${result.stderr}`); + assert.deepStrictEqual(calls, [], JSON.stringify(calls)); + assert.match(result.stdout, /the repository ships it/); + }) + ) + passed++; + else failed++; +} + if ( test('pre-push refuses a tracked interpreter reached through a committed symlink', () => { const { result, calls } = runHermeticPythonPrePush({ trackedSymlinkVenv: true });