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', () => {