diff --git a/scripts/codex-git-hooks/pre-push b/scripts/codex-git-hooks/pre-push index 2ee23c7f4..ff66512a1 100755 --- a/scripts/codex-git-hooks/pre-push +++ b/scripts/codex-git-hooks/pre-push @@ -117,16 +117,172 @@ 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, 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. +# +# 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 + 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 + # `:(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 + # 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. + # 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 + [[ ${#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 + for venv in "${VIRTUAL_ENV:-}" .venv venv env; do + if [[ -n "$venv" && -x "$venv/bin/python" ]]; then + 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 + fi + 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 -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 -I -c "import pytest" >/dev/null 2>&1; then + PYTEST_CMD=(poetry run pytest) + return 0 + fi + fi + # `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 + fi + PYTEST_CMD=() + return 1 +} + if [[ -f "pyproject.toml" || -f "requirements.txt" ]]; then - if command -v pytest >/dev/null 2>&1; then + if 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" + 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 + 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 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 diff --git a/tests/scripts/codex-hooks.test.js b/tests/scripts/codex-hooks.test.js index 0dfe1d2f9..1171de52c 100644 --- a/tests/scripts/codex-hooks.test.js +++ b/tests/scripts/codex-hooks.test.js @@ -306,6 +306,246 @@ 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, + venvExit = 0, + trackVenv = false, + trackedVenvBasename = 'python', + trackedSymlinkVenv = false, + pytestCmd = null, + overrideStub = false, + pathPytestVersionLine = 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()); + + // 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)}"`; + + // 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', trackVenv ? trackedVenvBasename : 'python'); + if (venvPython !== null) { + 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. + const added = spawnSync('git', ['add', '-f', '--', venvPython], { cwd: projectDir }); + assert.strictEqual(added.status, 0, added.stderr?.toString()); + } + } + + // 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; + if (overrideStubPath !== null) { + writeExecutable(overrideStubPath, `#!/bin/sh\n${record}\nexit 0\n`); + } + + 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); + // 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 = { + 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 || trackVenv ? {} : { VIRTUAL_ENV: toBashPath(venvDir) }), + ...(override === null ? {} : { ECC_PYTEST_CMD: override }), + }; + + 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) + ? fs.readFileSync(callsPath, 'utf8').trim().split(/\r?\n/).filter(Boolean) + : []; + cleanup(tempDir); + return { result, calls, venvPython, overrideStubPath }; +} + +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}|-I -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 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, /the repository ships it/); + }) +) + 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 }); + 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 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, /pytest failed \(exit 1\)/); + assert.doesNotMatch(result.stdout, /Verification checks passed/); + }) +) + passed++; +else failed++; + +if ( + 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)); + // 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++; + +// 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', () => { + 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$/); + }) +) + 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-');