From c6195edb2f99de947338292b7ef5142407fa7ffd Mon Sep 17 00:00:00 2001 From: Juan Garibay Date: Thu, 17 Sep 2026 16:19:57 -0400 Subject: [PATCH] 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-');