From f2c1c4d50439c4864f649028fc973a4a7a15875e Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 3 Sep 2026 01:03:12 +0000 Subject: [PATCH 1/9] fix(hooks): resolve check-pr-skill-audit-disclosure.sh's python3 deterministically hooks/check-pr-skill-audit-disclosure.sh's tier-1 block invoked its precondition probe and the skill-audit-disclosure gate script as bare python3, which resolves from the calling PreToolUse hook's own ambient PATH rather than this checkout's uv-managed .venv. When that PATH lacks the venv (confirmed live in this environment), the probe reports a genuinely-installed third-party package (pydantic) as missing and false-denies create_pull_request/update_pull_request. Route both invocations through uv run --frozen python3 (this block only ever runs inside a dev checkout, guarded by .github/ presence). Also route the tier-2 (SKILL.md-only) check_script invocation the same way, with a command -v uv + lockfile-gated fallback to bare python3, since that path also runs in a consumer plugin install with no uv toolchain. Also default is_importable()'s own python argument to sys.executable instead of a second, independent PATH lookup, so the probe always reflects whichever interpreter actually launched it rather than re-deriving a possibly-different one from PATH. Live-reproduced via a git worktree at the pre-fix commit: under this session's own ambient PATH (uv present, .venv/bin absent), the original code denies (exit 2, "python3 cannot import: pydantic") and the fixed code allows (exit 0). Refs #1697, #1581 Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01BGHXhvkGRDFxGBARLbmh81 --- hooks/check-pr-skill-audit-disclosure.sh | 39 +++++++++++++-- hooks/gitapex_check_python_precondition.py | 34 ++++++++----- .../test_gitapex_check_python_precondition.py | 49 +++++++++++++++++++ 3 files changed, 106 insertions(+), 16 deletions(-) diff --git a/hooks/check-pr-skill-audit-disclosure.sh b/hooks/check-pr-skill-audit-disclosure.sh index e3a094a9..7f2f79b3 100755 --- a/hooks/check-pr-skill-audit-disclosure.sh +++ b/hooks/check-pr-skill-audit-disclosure.sh @@ -252,7 +252,20 @@ if [ "$base_is_explicit" = "yes" ] && [ -n "$repo_root" ] && [ -f "$full_gate" ] # precondition script exit 0 (its own --help path) or reject with a # usage error, either of which this block would otherwise read as "no # missing packages" and silently skip the new deny path. - precondition_json=$(python3 "$precondition_script" -- "${required_packages[@]}" 2>/dev/null) || true + # + # Issue #1697: `uv run --frozen python3`, not a bare `python3` -- this + # whole block only ever runs once $full_gate/$flag_module (both under + # .github/scripts/, never deployed with the plugin -- see tier + # docstring above) are confirmed present, i.e. only inside this + # repository's own dev checkout, where uv and its .venv are always + # available (same precondition .pre-commit-config.yaml's own + # local-preflight entry already relies on, issue #1485/PR #1486). A + # bare `python3` here resolves from the calling PreToolUse hook's own + # ambient PATH, which may have no access to this checkout's + # uv-managed .venv -- the exact false-deny #1697 reports live (a + # PATH lacking pydantic reports it "missing" even though `uv sync` + # installed it). + precondition_json=$(uv run --frozen python3 "$precondition_script" -- "${required_packages[@]}" 2>/dev/null) || true # The filter must PROVE the output is the expected `{"missing": [...]}` # object, not merely fail to contradict it. Found by issue #1566's own # step-8 adversarial review: the previous `jq -r '.missing // [] | @@ -298,7 +311,10 @@ if [ "$base_is_explicit" = "yes" ] && [ -n "$repo_root" ] && [ -f "$full_gate" ] # disclosure.sh` crashed with mktemp's own exit 1, not falling through. if body_file=$(mktemp 2>/dev/null); then printf '%s' "$body" >"$body_file" - if full_output=$(cd "$repo_root" && python3 "$full_gate" \ + # Issue #1697: `uv run --frozen python3`, same rationale as the + # precondition-probe invocation above -- this call is also inside the + # dev-checkout-only tier-1 block. + if full_output=$(cd "$repo_root" && uv run --frozen python3 "$full_gate" \ --check-diff "$merge_base" HEAD --body-file "$body_file" 2>&1); then full_exit=0 else @@ -326,7 +342,7 @@ if [ "$base_is_explicit" = "yes" ] && [ -n "$repo_root" ] && [ -f "$full_gate" ] # the latter anyway. deny "Blocked by hooks/check-pr-skill-audit-disclosure.sh: this PR's diff requires skill-audit disclosure evidence its body does not carry. This is the same verdict .github/workflows/skill-audit-gate.yml will report, computed locally before the push. Fix the '## Skill audit evidence' section, then re-check with: - python3 .github/scripts/gitapex_gate_skill_audit_disclosure.py --check-diff ${merge_base} HEAD --body-file + uv run --frozen python3 .github/scripts/gitapex_gate_skill_audit_disclosure.py --check-diff ${merge_base} HEAD --body-file $full_output" fi @@ -347,6 +363,21 @@ fi # would otherwise crash past this hook's own "skip the local pre-check, # CI remains authoritative" fallback with mktemp's own exit code, an # exit Claude Code's PreToolUse contract treats as non-blocking. +# +# Issue #1697/#1581: prefer this checkout's own uv-managed .venv (uv on +# PATH plus a pyproject.toml/uv.lock at repo_root) over a bare `python3` +# resolved from the calling shell's own ambient PATH. Unlike tier 1 +# above, this block also runs in a consumer plugin install -- only +# skills/ and hooks/ are ever deployed there (docs/repository-layout.md), +# so no uv toolchain or lockfile exists to prefer -- an unconditional `uv +# run` would newly break every such install. $check_script is stdlib-only +# by design, so a bare python3 has always been a correct answer there; +# this fallback keeps that unchanged. +python3_cmd=(python3) +if command -v uv >/dev/null 2>&1 && [ -f "${repo_root}/pyproject.toml" ] && [ -f "${repo_root}/uv.lock" ]; then + python3_cmd=(uv run --frozen --directory "$repo_root" python3) +fi + if ! diff_error=$(mktemp 2>/dev/null); then echo "Warning: hooks/check-pr-skill-audit-disclosure.sh could not create a temp file for git-diff error capture (mktemp failed); skipping the local pre-check (CI's skill-audit-gate.yml will still catch this)." >&2 exit 0 @@ -363,7 +394,7 @@ if [ -z "$changed" ]; then exit 0 fi -if check_output=$(printf '%s' "$body" | python3 "$check_script" 2>&1); then +if check_output=$(printf '%s' "$body" | "${python3_cmd[@]}" "$check_script" 2>&1); then check_exit=0 else check_exit=$? diff --git a/hooks/gitapex_check_python_precondition.py b/hooks/gitapex_check_python_precondition.py index 43332523..f0ad01bf 100755 --- a/hooks/gitapex_check_python_precondition.py +++ b/hooks/gitapex_check_python_precondition.py @@ -72,22 +72,31 @@ PROBE_TIMEOUT_SECONDS = 10.0 -def is_importable(module: str, *, python: str = "python3", timeout: float = PROBE_TIMEOUT_SECONDS) -> bool: +def is_importable(module: str, *, python: str | None = None, timeout: float = PROBE_TIMEOUT_SECONDS) -> bool: """Return True iff `module` is importable by a separate `python` subprocess within `timeout` seconds. + `python` defaults to `sys.executable` (this process's own interpreter) + rather than a fresh `python3` PATH lookup (issue #1697): a PreToolUse + hook's own shell context can resolve a bare `python3` from a different + PATH than the one that actually launched this checker (e.g. via `uv + run --frozen python3`), so a second, independent PATH lookup can + answer a different question than "can the interpreter that is running + right now import this module" -- the one callers actually ask. Falls + back to the literal string "python3" only in the rare case + `sys.executable` itself is empty (documented as possible for an + embedded interpreter). + Never imports `module` in this process: a missing module must not be able to crash this checker itself. """ + if python is None: + python = sys.executable or "python3" try: - # S603 waived: a fixed argv list with no shell, and `python` - # (default "python3") is intentionally resolved from PATH -- this - # probe exists specifically to check what that same PATH-resolved - # interpreter can import, so pinning an absolute path here would - # answer a different question than the one callers actually ask. - # The module name is data (sys.argv[1] inside the probe source, - # never spliced into it), so this is not untrusted-input execution - # in the sense S603 warns about. + # S603 waived: a fixed argv list with no shell. The module name is + # data (sys.argv[1] inside the probe source, never spliced into + # it), so this is not untrusted-input execution in the sense S603 + # warns about. # # `-I` (isolated mode) is load-bearing, not cosmetic: without it, # `python3 -c` prepends the process's own current working directory @@ -134,10 +143,11 @@ def is_importable(module: str, *, python: str = "python3", timeout: float = PROB def find_missing_modules( - modules: list[str], *, python: str = "python3", timeout: float = PROBE_TIMEOUT_SECONDS + modules: list[str], *, python: str | None = None, timeout: float = PROBE_TIMEOUT_SECONDS ) -> list[str]: - """Return the subsequence of `modules` not importable by `python`, in - order. `timeout` bounds each module's own probe independently.""" + """Return the subsequence of `modules` not importable by `python` + (defaults to `sys.executable`, see `is_importable`'s own docstring), + in order. `timeout` bounds each module's own probe independently.""" return [module for module in modules if not is_importable(module, python=python, timeout=timeout)] diff --git a/hooks/test_gitapex_check_python_precondition.py b/hooks/test_gitapex_check_python_precondition.py index 95fdc1e0..ff6920db 100644 --- a/hooks/test_gitapex_check_python_precondition.py +++ b/hooks/test_gitapex_check_python_precondition.py @@ -64,6 +64,55 @@ def test_is_importable_false_for_a_fake_module_name() -> None: assert checker.is_importable(_FAKE_MODULE) is False +def test_is_importable_default_python_is_this_processs_own_interpreter( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Regression (issue #1697): the default `python` must be + `sys.executable`, not a fresh `python3` PATH lookup -- a PreToolUse + hook's own shell can resolve a bare `python3` from a PATH lacking the + uv-managed .venv this checker itself was launched from, causing a + false "cannot import" even though the venv genuinely has the + package. Asserted by intercepting `subprocess.run`'s own argv rather + than the return value, so this fails loudly if a future edit + reintroduces a literal `"python3"` default.""" + captured: list[list[str]] = [] + + def _fake_run(argv: list[str], **_kwargs: object) -> subprocess.CompletedProcess[bytes]: + captured.append(argv) + return subprocess.CompletedProcess(argv, returncode=0) + + monkeypatch.setattr(subprocess, "run", _fake_run) + checker.is_importable("json") + + assert captured[0][0] == sys.executable + + +def test_is_importable_default_falls_back_to_python3_when_sys_executable_is_empty( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """`sys.executable` can be an empty string for an embedded interpreter + (documented CPython behavior) -- that must fall back to the literal + "python3" rather than launching argv[0] == "" (which OSErrors).""" + captured: list[list[str]] = [] + + def _fake_run(argv: list[str], **_kwargs: object) -> subprocess.CompletedProcess[bytes]: + captured.append(argv) + return subprocess.CompletedProcess(argv, returncode=0) + + monkeypatch.setattr(sys, "executable", "") + monkeypatch.setattr(subprocess, "run", _fake_run) + checker.is_importable("json") + + assert captured[0][0] == "python3" + + +def test_is_importable_explicit_python_overrides_the_default() -> None: + """An explicitly passed `python` (e.g. a caller that already resolved + a specific interpreter) must still win over the `sys.executable` + default.""" + assert checker.is_importable("json", python=sys.executable) is True + + def test_is_importable_probes_in_a_subprocess_not_this_process() -> None: """A missing module must not be able to crash this checker itself -- the whole reason for the subprocess-probe design. If is_importable() From 1117176a7de7a00d3178b75c8579befbe08dfce6 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 3 Sep 2026 01:04:37 +0000 Subject: [PATCH 2/9] fix(hooks): route remaining bare python3 call sites through uv, with fallback Extends #1697's fix pattern to the nine other hooks/*.sh bare-python3 call sites (check-bash-safety.sh x2, check-issue-acm-disclosure.sh, check-post-review-obligation-tracker.sh, check-post-write-provenance.sh, check-pr-duplicate-issue.sh, check-pr-issue-acm-disclosure.sh, check-pr-title-convention.sh, check-stop-review-obligation.sh) plus check-pr-skill-audit-disclosure.sh's own tier-2 (SKILL.md-only) call site, for consistency with the same PATH-nondeterminism class #1697's own precondition-probe fix closes. All ten scripts these call sites invoke are stdlib-only today (verified by inspecting every import statement), so this is a preventive or consistency fix rather than a currently-observable-failure fix for these nine plus tier-2. Each call site prefers uv run --frozen python3 when uv is on PATH and this checkout has its own pyproject.toml/uv.lock, falling back to a bare python3 otherwise. hooks/ is deployed to consumer plugin installs (see docs/repository-layout.md), which carry neither a uv toolchain nor a lockfile, so an unconditional uv run would have newly broken every such install; this fallback keeps the pre-existing behavior unchanged there. Also closes out #1581's own residual scope on this fix pattern (its stated target, .pre-commit-config.yaml's local-preflight entry, was already fixed by PR #1486/#1485 -- see the comment on #1581 for the live-verified timeline). Refs #1697, #1581 Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01BGHXhvkGRDFxGBARLbmh81 --- hooks/check-bash-safety.sh | 26 +++++++++++++++++-- hooks/check-issue-acm-disclosure.sh | 17 +++++++++++- hooks/check-post-review-obligation-tracker.sh | 17 ++++++++++-- hooks/check-post-write-provenance.sh | 17 +++++++++++- hooks/check-pr-duplicate-issue.sh | 17 +++++++++++- hooks/check-pr-issue-acm-disclosure.sh | 17 +++++++++++- hooks/check-pr-title-convention.sh | 17 +++++++++++- hooks/check-stop-review-obligation.sh | 21 ++++++++++++++- 8 files changed, 139 insertions(+), 10 deletions(-) diff --git a/hooks/check-bash-safety.sh b/hooks/check-bash-safety.sh index ad6ac481..29d9f1ba 100755 --- a/hooks/check-bash-safety.sh +++ b/hooks/check-bash-safety.sh @@ -117,6 +117,21 @@ if [ ! -f "$classifier" ]; then deny "Blocked by hooks/check-bash-safety.sh: gitapex_check_bash_safety.py was not found at $classifier (corrupted or incomplete plugin bundle). Failing closed." fi +# Issue #1697/#1581: prefer this checkout's own uv-managed .venv (uv on +# PATH plus a pyproject.toml/uv.lock at plugin_root) over a bare `python3` +# resolved from the calling shell's own ambient PATH -- closes the same +# PATH-nondeterminism class hooks/check-pr-skill-audit-disclosure.sh's own +# precondition probe hit. Falls back to a bare `python3` for a consumer +# plugin install (only skills/ and hooks/ are ever deployed there -- +# docs/repository-layout.md), where no uv toolchain/lockfile exists -- +# $classifier is stdlib-only, so a bare python3 has always been a correct +# answer there; this fallback keeps that unchanged. +plugin_root="$(dirname "$script_dir")" +python3_cmd=(python3) +if command -v uv >/dev/null 2>&1 && [ -f "$plugin_root/pyproject.toml" ] && [ -f "$plugin_root/uv.lock" ]; then + python3_cmd=(uv run --frozen --directory "$plugin_root" python3) +fi + # $input is piped on stdin the whole way through, never re-passed as a # command-line argument -- same ARG_MAX rationale as deny()/warn() above. # The classifier re-validates tool_input/tool_input.command's own shape @@ -130,7 +145,7 @@ classifier_exit=0 # leak into this hook's own stderr channel -- deny()'s JSON envelope below # is the only thing this hook itself ever writes there, and a stray extra # line ahead of it would break Claude Code's own JSON parse of that stream. -classifier_output=$(printf '%s' "$input" | python3 "$classifier" 2>/dev/null) || classifier_exit=$? +classifier_output=$(printf '%s' "$input" | "${python3_cmd[@]}" "$classifier" 2>/dev/null) || classifier_exit=$? if [ "$classifier_exit" -ne 0 ]; then deny "Blocked by hooks/check-bash-safety.sh: gitapex_check_bash_safety.py exited non-zero ($classifier_exit) instead of returning a decision. Failing closed." fi @@ -160,6 +175,13 @@ if [ "$is_git_push" = "true" ]; then deny "Blocked by hooks/check-bash-safety.sh: git push requires the outward-artifact-preflight scan, but gitapex_scan_provenance.py was not found at $scan_script." fi + # Same uv-preferred/bare-python3-fallback rationale as $classifier above, + # keyed on project_dir (this call's own root) rather than plugin_root. + scan_python3_cmd=(python3) + if command -v uv >/dev/null 2>&1 && [ -f "$project_dir/pyproject.toml" ] && [ -f "$project_dir/uv.lock" ]; then + scan_python3_cmd=(uv run --frozen --directory "$project_dir" python3) + fi + # Determine the commit range being pushed. With an upstream, @{u}..HEAD is # exact. On a first push (`git push -u origin newbranch`) there is no # upstream, so @{u} errors and the range is empty. Fall back to the @@ -187,7 +209,7 @@ if [ "$is_git_push" = "true" ]; then fi scan_exit=0 - scan_output=$(printf '%s' "$content" | python3 "$scan_script" 2>&1) || scan_exit=$? + scan_output=$(printf '%s' "$content" | "${scan_python3_cmd[@]}" "$scan_script" 2>&1) || scan_exit=$? # gitapex_scan_provenance.py's own docstring says it "surfaces candidates, it does # not decide" -- a hard deny here would make this mechanical regex the diff --git a/hooks/check-issue-acm-disclosure.sh b/hooks/check-issue-acm-disclosure.sh index fa1f656d..f7c438d5 100755 --- a/hooks/check-issue-acm-disclosure.sh +++ b/hooks/check-issue-acm-disclosure.sh @@ -41,6 +41,21 @@ body=$(printf '%s' "$input" | jq -r '.tool_input.body // empty') script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" check_script="$script_dir/gitapex_check_acm_present_or_waiver.py" +# Issue #1697/#1581: prefer this checkout's own uv-managed .venv over a +# bare `python3` resolved from the calling shell's own ambient PATH -- +# see hooks/check-pr-skill-audit-disclosure.sh's own precondition-probe +# fix for the PATH-nondeterminism class this closes. Falls back to a bare +# `python3` for a consumer plugin install (only skills/ and hooks/ are +# ever deployed there -- docs/repository-layout.md), where no uv +# toolchain/lockfile exists -- $check_script is stdlib-only, so a bare +# python3 has always been a correct answer there; this fallback keeps +# that unchanged. +plugin_root="$(dirname "$script_dir")" +python3_cmd=(python3) +if command -v uv >/dev/null 2>&1 && [ -f "$plugin_root/pyproject.toml" ] && [ -f "$plugin_root/uv.lock" ]; then + python3_cmd=(uv run --frozen --directory "$plugin_root" python3) +fi + deny() { local reason="$1" jq -n --arg msg "$reason" \ @@ -52,7 +67,7 @@ if [ ! -f "$check_script" ]; then deny "Blocked by hooks/check-issue-acm-disclosure.sh: cannot verify ACM disclosure -- gitapex_check_acm_present_or_waiver.py was not found at $check_script (corrupted or incomplete plugin bundle)." fi -if printf '%s' "$body" | python3 "$check_script" >/dev/null 2>&1; then +if printf '%s' "$body" | "${python3_cmd[@]}" "$check_script" >/dev/null 2>&1; then exit 0 fi diff --git a/hooks/check-post-review-obligation-tracker.sh b/hooks/check-post-review-obligation-tracker.sh index 60a5a962..80d34c77 100755 --- a/hooks/check-post-review-obligation-tracker.sh +++ b/hooks/check-post-review-obligation-tracker.sh @@ -44,7 +44,20 @@ if [ ! -f "$tracker_script" ]; then exit 0 fi -if ! command -v python3 >/dev/null 2>&1; then +# Issue #1697/#1581: prefer this checkout's own uv-managed .venv over a +# bare `python3` resolved from the calling shell's own ambient PATH -- +# see hooks/check-pr-skill-audit-disclosure.sh's own precondition-probe +# fix for the PATH-nondeterminism class this closes. Falls back to a bare +# `python3` for a consumer plugin install (only skills/ and hooks/ are +# ever deployed there -- docs/repository-layout.md), where no uv +# toolchain/lockfile exists -- $tracker_script is stdlib-only, so a bare +# python3 has always been a correct answer there; this fallback keeps +# that unchanged. +plugin_root="$(dirname "$script_dir")" +python3_cmd=(python3) +if command -v uv >/dev/null 2>&1 && [ -f "$plugin_root/pyproject.toml" ] && [ -f "$plugin_root/uv.lock" ]; then + python3_cmd=(uv run --frozen --directory "$plugin_root" python3) +elif ! command -v python3 >/dev/null 2>&1; then printf '%s\n' "{\"systemMessage\": \"hooks/check-post-review-obligation-tracker.sh: python3 is not available on PATH. Skipping this cycle's obligation tracking.\"}" exit 0 fi @@ -53,7 +66,7 @@ fi # no ARG_MAX concern either. Payload-shape validation, tool_name dispatch, # and any systemMessage worth emitting for a malformed/irrelevant payload # all happen inside the tracker script itself (see header above). -if ! python3 "$tracker_script" 2>/dev/null; then +if ! "${python3_cmd[@]}" "$tracker_script" 2>/dev/null; then printf '%s\n' "{\"systemMessage\": \"hooks/check-post-review-obligation-tracker.sh: gitapex_check_post_review_obligation_tracker.py exited non-zero. Review-thread-resolution/mergeable_state tracking for this turn may be incomplete.\"}" fi diff --git a/hooks/check-post-write-provenance.sh b/hooks/check-post-write-provenance.sh index e8268606..c1f89cd5 100755 --- a/hooks/check-post-write-provenance.sh +++ b/hooks/check-post-write-provenance.sh @@ -99,10 +99,25 @@ if [ ! -f "$check_script" ]; then report "hooks/check-post-write-provenance.sh could not verify the stored PR/issue body: gitapex_check_post_write_provenance.py was not found at $check_script (corrupted or incomplete plugin bundle). The artifact this call just published is UNVERIFIED." fi +# Issue #1697/#1581: prefer this checkout's own uv-managed .venv over a +# bare `python3` resolved from the calling shell's own ambient PATH -- +# see hooks/check-pr-skill-audit-disclosure.sh's own precondition-probe +# fix for the PATH-nondeterminism class this closes. Falls back to a bare +# `python3` for a consumer plugin install (only skills/ and hooks/ are +# ever deployed there -- docs/repository-layout.md), where no uv +# toolchain/lockfile exists -- $check_script is stdlib-only, so a bare +# python3 has always been a correct answer there; this fallback keeps +# that unchanged. +plugin_root="$(dirname "$script_dir")" +python3_cmd=(python3) +if command -v uv >/dev/null 2>&1 && [ -f "$plugin_root/pyproject.toml" ] && [ -f "$plugin_root/uv.lock" ]; then + python3_cmd=(uv run --frozen --directory "$plugin_root" python3) +fi + # $input is piped on stdin the whole way through, never re-passed as a # command-line argument -- same ARG_MAX rationale as report() above, and # the same reason a tool-controlled title/body never reaches an argv slot. -if check_output=$(printf '%s' "$input" | python3 "$check_script" 2>&1); then +if check_output=$(printf '%s' "$input" | "${python3_cmd[@]}" "$check_script" 2>&1); then exit 0 fi diff --git a/hooks/check-pr-duplicate-issue.sh b/hooks/check-pr-duplicate-issue.sh index 195d304a..70db57b0 100755 --- a/hooks/check-pr-duplicate-issue.sh +++ b/hooks/check-pr-duplicate-issue.sh @@ -117,10 +117,25 @@ if [ ! -f "$check_script" ]; then deny "Blocked by hooks/check-pr-duplicate-issue.sh: cannot verify duplicate-PR status -- gitapex_check_pr_duplicate_issue.py was not found at $check_script (corrupted or incomplete plugin bundle). Failing closed." fi +# Issue #1697/#1581: prefer this checkout's own uv-managed .venv over a +# bare `python3` resolved from the calling shell's own ambient PATH -- +# see hooks/check-pr-skill-audit-disclosure.sh's own precondition-probe +# fix for the PATH-nondeterminism class this closes. Falls back to a bare +# `python3` for a consumer plugin install (only skills/ and hooks/ are +# ever deployed there -- docs/repository-layout.md), where no uv +# toolchain/lockfile exists -- $check_script is stdlib-only, so a bare +# python3 has always been a correct answer there; this fallback keeps +# that unchanged. +plugin_root="$(dirname "$script_dir")" +python3_cmd=(python3) +if command -v uv >/dev/null 2>&1 && [ -f "$plugin_root/pyproject.toml" ] && [ -f "$plugin_root/uv.lock" ]; then + python3_cmd=(uv run --frozen --directory "$plugin_root" python3) +fi + payload=$(printf '%s' "$input" | jq -c \ '{owner: (.tool_input.owner // ""), repo: (.tool_input.repo // ""), title: (.tool_input.title // ""), body: (.tool_input.body // "")}') -if check_output=$(printf '%s' "$payload" | python3 "$check_script" 2>&1); then +if check_output=$(printf '%s' "$payload" | "${python3_cmd[@]}" "$check_script" 2>&1); then check_exit=0 else check_exit=$? diff --git a/hooks/check-pr-issue-acm-disclosure.sh b/hooks/check-pr-issue-acm-disclosure.sh index 9dbb8790..63e8c729 100755 --- a/hooks/check-pr-issue-acm-disclosure.sh +++ b/hooks/check-pr-issue-acm-disclosure.sh @@ -130,6 +130,21 @@ if [ ! -f "$check_script" ]; then deny "Blocked by hooks/check-pr-issue-acm-disclosure.sh: cannot verify the cited issue's ACM/waiver disclosure -- gitapex_check_pr_issue_acm_disclosure.py was not found at $check_script (corrupted or incomplete plugin bundle). Failing closed." fi +# Issue #1697/#1581: prefer this checkout's own uv-managed .venv over a +# bare `python3` resolved from the calling shell's own ambient PATH -- +# see hooks/check-pr-skill-audit-disclosure.sh's own precondition-probe +# fix for the PATH-nondeterminism class this closes. Falls back to a bare +# `python3` for a consumer plugin install (only skills/ and hooks/ are +# ever deployed there -- docs/repository-layout.md), where no uv +# toolchain/lockfile exists -- $check_script is stdlib-only, so a bare +# python3 has always been a correct answer there; this fallback keeps +# that unchanged. +plugin_root="$(dirname "$script_dir")" +python3_cmd=(python3) +if command -v uv >/dev/null 2>&1 && [ -f "$plugin_root/pyproject.toml" ] && [ -f "$plugin_root/uv.lock" ]; then + python3_cmd=(uv run --frozen --directory "$plugin_root" python3) +fi + # Extracts owner/repo/title/body directly from $input in one jq call and # re-shapes them into the payload the Python checker expects -- $input is # read via stdin the whole way through, never re-passed as a `--arg` @@ -142,7 +157,7 @@ fi payload=$(printf '%s' "$input" | jq -c \ '{owner: (.tool_input.owner // ""), repo: (.tool_input.repo // ""), title: (.tool_input.title // ""), body: (.tool_input.body // "")}') -if check_output=$(printf '%s' "$payload" | python3 "$check_script" 2>&1); then +if check_output=$(printf '%s' "$payload" | "${python3_cmd[@]}" "$check_script" 2>&1); then check_exit=0 else check_exit=$? diff --git a/hooks/check-pr-title-convention.sh b/hooks/check-pr-title-convention.sh index b757e8a9..5b483768 100755 --- a/hooks/check-pr-title-convention.sh +++ b/hooks/check-pr-title-convention.sh @@ -110,6 +110,21 @@ if [ ! -f "$check_script" ]; then deny "Blocked by hooks/check-pr-title-convention.sh: cannot verify the PR title's Conventional Commits format -- gitapex_check_pr_title_convention.py was not found at $check_script (corrupted or incomplete plugin bundle). Failing closed." fi +# Issue #1697/#1581: prefer this checkout's own uv-managed .venv over a +# bare `python3` resolved from the calling shell's own ambient PATH -- +# see hooks/check-pr-skill-audit-disclosure.sh's own precondition-probe +# fix for the PATH-nondeterminism class this closes. Falls back to a bare +# `python3` for a consumer plugin install (only skills/ and hooks/ are +# ever deployed there -- docs/repository-layout.md), where no uv +# toolchain/lockfile exists -- $check_script is stdlib-only, so a bare +# python3 has always been a correct answer there; this fallback keeps +# that unchanged. +plugin_root="$(dirname "$script_dir")" +python3_cmd=(python3) +if command -v uv >/dev/null 2>&1 && [ -f "$plugin_root/pyproject.toml" ] && [ -f "$plugin_root/uv.lock" ]; then + python3_cmd=(uv run --frozen --directory "$plugin_root" python3) +fi + # jq's output is piped straight into python3, never captured into a shell # variable first: `title=$(...)` command substitution unconditionally # strips every trailing newline, which silently defeats the very @@ -122,7 +137,7 @@ fi # extra `\n` and false-reject every otherwise-valid title. `-j` emits the # string with no added newline, so only a newline genuinely present in # the title's own JSON value reaches the checker. -if printf '%s' "$input" | jq -j '.tool_input.title // ""' | python3 "$check_script" >/dev/null 2>&1; then +if printf '%s' "$input" | jq -j '.tool_input.title // ""' | "${python3_cmd[@]}" "$check_script" >/dev/null 2>&1; then exit 0 fi diff --git a/hooks/check-stop-review-obligation.sh b/hooks/check-stop-review-obligation.sh index b83544d3..82245851 100755 --- a/hooks/check-stop-review-obligation.sh +++ b/hooks/check-stop-review-obligation.sh @@ -74,11 +74,30 @@ if [ ! -f "$check_script" ]; then deny "Blocked by hooks/check-stop-review-obligation.sh: gitapex_check_stop_review_obligation.py was not found at $check_script (corrupted or incomplete plugin bundle). Failing closed." fi +# Issue #1697/#1581: prefer this checkout's own uv-managed .venv over a +# bare `python3` resolved from the calling shell's own ambient PATH -- +# see hooks/check-pr-skill-audit-disclosure.sh's own precondition-probe +# fix for the PATH-nondeterminism class this closes. Falls back to a bare +# `python3` for a consumer plugin install (only skills/ and hooks/ are +# ever deployed there -- docs/repository-layout.md), where no uv +# toolchain/lockfile exists -- $check_script is stdlib-only, so a bare +# python3 has always been a correct answer there; this fallback keeps +# that unchanged. deny()'s own inline `python3 -c` above is left as a +# bare interpreter deliberately: it only ever needs the stdlib json +# module and must stay reachable even when this block's own uv/pyproject +# lookup below has not run yet (it can fire before this point, from the +# command -v python3 guard at the very top of this file). +plugin_root="$(dirname "$script_dir")" +python3_cmd=(python3) +if command -v uv >/dev/null 2>&1 && [ -f "$plugin_root/pyproject.toml" ] && [ -f "$plugin_root/uv.lock" ]; then + python3_cmd=(uv run --frozen --directory "$plugin_root" python3) +fi + # Payload-shape validation (malformed JSON, non-object payload) happens # entirely inside gitapex_check_stop_review_obligation.py's own main() -- # see that module's docstring -- so this wrapper does none of its own. check_exit=0 -check_output=$(python3 "$check_script" 2>&1) || check_exit=$? +check_output=$("${python3_cmd[@]}" "$check_script" 2>&1) || check_exit=$? if [ "$check_exit" -eq 0 ]; then exit 0 From 1b9d35ed2bae5f4336d444829640b78cfa55242c Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 3 Sep 2026 01:05:05 +0000 Subject: [PATCH 3/9] feat(gates): hard-fail bare python3 of a third-party-dependent hooks/*.py target gitapex_gate_bare_python3_invocation.py previously only WARNED (report-only, never affected exit code) on a hooks/*.sh bare-python3 invocation of a .github/scripts/*.py target, and never inspected hooks/*.py targets at all -- so it could not have caught #1697's own root cause, a bare invocation of hooks/gitapex_check_python_precondition.py. Promotes the .github/scripts/*.py case from WARNING to HARD-FAIL, and adds a new HARD-FAIL case for a hooks/*.py file registered in .gitapex/ssot.json under a gate whose own preconditions.requires_python_packages is non-empty (a hooks/*.py file that genuinely needs a third-party-dependent, deterministically-resolved interpreter, not every hooks/*.py sibling indiscriminately -- an unregistered hooks/*.py target stays bare-invoked by design, per docs/repository-layout.md). Also registers hooks/gitapex_check_python_precondition.py under the skill-audit-disclosure gate's own script list in .gitapex/ssot.json, since that gate is the one declaring the pydantic precondition this script exists to probe, and it was missing from that list. Live-verified this gate would have caught #1697's own defect before merge: run against a git-worktree checkout of the pre-fix commit's hooks/ directory with the fixed ssot.json, it correctly flags all three original bare invocations in check-pr-skill-audit-disclosure.sh (precondition_script, full_gate, and check_script). Refs #1697 Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01BGHXhvkGRDFxGBARLbmh81 --- .gitapex/ssot.json | 1 + .../gitapex_gate_bare_python3_invocation.py | 199 +++++++++--- ...st_gitapex_gate_bare_python3_invocation.py | 285 +++++++++++++++++- 3 files changed, 434 insertions(+), 51 deletions(-) diff --git a/.gitapex/ssot.json b/.gitapex/ssot.json index dbec52ca..be62a177 100644 --- a/.gitapex/ssot.json +++ b/.gitapex/ssot.json @@ -218,6 +218,7 @@ "script": [ "hooks/check-pr-skill-audit-disclosure.sh", "hooks/gitapex_check_skill_audit_disclosure_or_waiver.py", + "hooks/gitapex_check_python_precondition.py", ".github/scripts/gitapex_gate_skill_audit_disclosure.py", ".github/scripts/gitapex_compute_skill_audit_flags.py", ".github/scripts/gitapex_skill_description_diff.py", diff --git a/.github/scripts/gitapex_gate_bare_python3_invocation.py b/.github/scripts/gitapex_gate_bare_python3_invocation.py index 0ae25191..b43ccd32 100644 --- a/.github/scripts/gitapex_gate_bare_python3_invocation.py +++ b/.github/scripts/gitapex_gate_bare_python3_invocation.py @@ -73,42 +73,64 @@ shell parsing. Usage: - uv run --frozen python3 .github/scripts/gitapex_gate_bare_python3_invocation.py [workflows_dir] [hooks_dir] + uv run --frozen python3 .github/scripts/gitapex_gate_bare_python3_invocation.py [workflows_dir] [hooks_dir] [ssot_path] Exit codes: 0 every `.github/scripts/*.py` invocation found in a `run:` step uses - `uv run`. - 1 a bare `python3 .github/scripts/*.py` invocation was found, OR the - scan could not be performed (missing/unreadable directory, no - workflow files, a file that will not decode, or a file whose YAML - does not parse to the expected `jobs: {...: {steps: [...]}}` - shape) -- "nothing was scanned" and "everything scanned was clean" - are different claims, and only one of them is ever true, so the - former is reported as a finding rather than sharing the latter's - exit code (matching `gitapex_scan_unpinned_actions.py`'s own - fail-closed rationale, issue #848). - -Issue #1446 Item 2 -- WARNING-tier addition: `hooks/*.sh` almost never -invokes a `.github/scripts/*.py` gate directly on the same line the way a -workflow `run:` step does. It instead assigns the path to a shell -variable on one line and invokes `python3 "$var"` several lines later -(e.g. `hooks/check-pr-skill-audit-disclosure.sh`'s `full_gate` variable), -which `find_bare_invocations`'s same-line regex cannot see. + `uv run`, and every `hooks/*.sh` shell-variable-indirected + invocation (see below) is likewise clean. + 1 a bare `python3 .github/scripts/*.py` invocation was found (in a + workflow `run:` step or, indirected through a shell variable, in a + `hooks/*.sh` file), a bare `hooks/*.sh` invocation of a registered + `hooks/*.py` target whose own gate requires a third-party Python + package was found, OR the scan could not be performed + (missing/unreadable directory, no workflow files, a file that will + not decode, or a file whose YAML does not parse to the expected + `jobs: {...: {steps: [...]}}` shape) -- "nothing was scanned" and + "everything scanned was clean" are different claims, and only one + of them is ever true, so the former is reported as a finding + rather than sharing the latter's exit code (matching + `gitapex_scan_unpinned_actions.py`'s own fail-closed rationale, + issue #848). + +Issue #1446 Item 2 (original introduction) / issue #1697 (HARD-FAIL +promotion): `hooks/*.sh` almost never invokes a `.github/scripts/*.py` +gate directly on the same line the way a workflow `run:` step does. It +instead assigns the path to a shell variable on one line and invokes +`python3 "$var"` several lines later (e.g. +`hooks/check-pr-skill-audit-disclosure.sh`'s `full_gate` variable), which +`find_bare_invocations`'s same-line regex cannot see. `find_hooks_shell_indirected_invocations` closes that blind spot with a two-step static scan of `hooks/*.sh`, scoped to the direct single- assignment-then-invocation shape this repository's real `hooks/*.sh` files actually use (no aliasing, no string concatenation, no multi-hop -reassignment tracing). Its findings are report-only: `main()` prints them -but they never flip the exit code, unlike `find_bare_invocations`'s own -workflow-scan findings above, which remain a hard fail. This is -deliberate, not an oversight -- a `hooks/*.py` sibling script is -stdlib-only, self-contained, and bare-invoked by design -(`docs/repository-layout.md`), and a variable assigned from one is never -tracked, so it can never be flagged. +reassignment tracing). + +Originally WARNING-tier (report-only, never flipped the exit code) on the +theory that a `hooks/*.py` sibling script is stdlib-only, self-contained, +and bare-invoked by design (`docs/repository-layout.md`). Issue #1697 +found the gap in that theory live: `hooks/gitapex_check_python_precondition.py` +is itself stdlib-only, but its own job is probing whether a *third-party* +package (pydantic, for the `skill-audit-disclosure` gate) is importable -- +and `hooks/check-pr-skill-audit-disclosure.sh`'s own bare `python3 +"$precondition_script"` invocation of it inherited the exact PATH- +dependent false-deny this whole gate exists to prevent, invisible to this +scan because it targeted a `hooks/*.py` file, a class this scan +categorically never tracked. `load_python_dependent_hook_script_names` +closes that gap by reading `.gitapex/ssot.json` directly: a `hooks/*.py` +path is tracked (and now hard-fails, same as `.github/scripts/*.py`) if +and only if it is registered under a gate whose own +`preconditions.requires_python_packages` is non-empty -- i.e. a `hooks/*.py` +file that genuinely needs a third-party-dependent, deterministically- +resolved interpreter, not every `hooks/*.py` sibling indiscriminately. A +`hooks/*.py` variable NOT so registered is still never tracked, so it can +never be flagged -- the original stdlib-only-and-bare-by-design theory +still holds for every such file. """ from __future__ import annotations +import json import pathlib import re import sys @@ -117,6 +139,7 @@ WORKFLOWS_DIR = pathlib.Path(".github/workflows") HOOKS_DIR = pathlib.Path("hooks") +SSOT_PATH = pathlib.Path(".gitapex/ssot.json") # `uv run`, optionally followed by long-form flags (`--frozen`, # `--flag=value` -- the only shapes this repository's real call sites use; @@ -261,37 +284,106 @@ def _scan_workflow(workflow: pathlib.Path) -> list[tuple[str, int, str]]: return findings -def find_hooks_shell_indirected_invocations(hooks_dir: pathlib.Path = HOOKS_DIR) -> list[tuple[str, int, str]]: +def load_python_dependent_hook_script_names(ssot_path: pathlib.Path = SSOT_PATH) -> frozenset[str]: + """Return the basenames of every `hooks/*.py` file registered in + `.gitapex/ssot.json` under a gate whose own + `preconditions.requires_python_packages` is non-empty (issue #1697): a + `hooks/*.sh` bare-`python3` invocation of one of these risks the exact + PATH-dependent false-deny #1697 found (the calling shell's own ambient + PATH may not resolve an interpreter that can import the third-party + package that gate needs), the same reason a bare `python3 + .github/scripts/*.py` invocation is already a hard failure below. + + Degrades to an empty result (never raises) when the registry is + missing, unreadable, or does not parse to the expected shape -- the + caller then simply has no additional `hooks/*.py` targets to widen its + existing `.github/scripts/*.py`-only scope with, rather than crashing + this whole gate on an unreadable registry.""" + try: + data = json.loads(ssot_path.read_text(encoding="utf-8")) + except (OSError, UnicodeDecodeError, json.JSONDecodeError): + return frozenset() + if not isinstance(data, dict): + return frozenset() + gates = data.get("gates") + if not isinstance(gates, list): + return frozenset() + + names: set[str] = set() + for gate in gates: + if not isinstance(gate, dict): + continue + preconditions = gate.get("preconditions") + packages = preconditions.get("requires_python_packages") if isinstance(preconditions, dict) else None + if not isinstance(packages, list) or not packages: + continue + scripts = gate.get("script") + if not isinstance(scripts, list): + continue + for script in scripts: + if isinstance(script, str) and script.startswith("hooks/") and script.endswith(".py"): + names.add(pathlib.PurePosixPath(script).name) + return frozenset(names) + + +def find_hooks_shell_indirected_invocations( + hooks_dir: pathlib.Path = HOOKS_DIR, + hard_fail_hooks_py_names: frozenset[str] = frozenset(), +) -> list[tuple[str, int, str]]: """Return (file, line_number, line) for each `hooks/*.sh` bare `python3 "$var"` invocation (quoted or unquoted, `${var}` brace form - or bare `$var`) of a shell variable whose own assignment targets a - `.github/scripts/*.py` path. WARNING tier (report-only, see - module docstring): unlike `find_bare_invocations`, a missing or - unreadable `hooks_dir` is simply nothing to warn about, not a - fail-closed finding -- there is no exit-code contract here to protect. - A variable assigned from a `hooks/*.py` path is never tracked, so it - can never appear in the result (those are bare-invoked by design).""" + or bare `$var`) of a shell variable whose own assignment targets + either a `.github/scripts/*.py` path, or a `hooks/*.py` file named in + `hard_fail_hooks_py_names` (issue #1697: a gate-registered `hooks/*.py` + whose own gate declares a non-empty + `preconditions.requires_python_packages` -- see + `load_python_dependent_hook_script_names`). HARD-FAIL tier (issue + #1697; formerly WARNING-only, see issue #1446's own original + introduction): unlike `find_bare_invocations`, a missing or unreadable + `hooks_dir` is simply nothing to check, not a fail-closed finding -- + there is no exit-code contract to protect on a directory this gate's + own caller may legitimately not have. A `hooks/*.py` variable NOT + named in `hard_fail_hooks_py_names` is still never tracked (those + remain bare-invoked by design, per docs/repository-layout.md).""" findings: list[tuple[str, int, str]] = [] if not hooks_dir.is_dir(): return findings for hook in sorted(hooks_dir.glob("*.sh")): - findings.extend(_scan_hook(hook)) + findings.extend(_scan_hook(hook, hard_fail_hooks_py_names)) return findings -def _scan_hook(hook: pathlib.Path) -> list[tuple[str, int, str]]: +def _scan_hook( + hook: pathlib.Path, hard_fail_hooks_py_names: frozenset[str] = frozenset() +) -> list[tuple[str, int, str]]: try: lines = hook.read_text(encoding="utf-8").splitlines() except (OSError, UnicodeDecodeError): - # WARNING tier: nothing to fail closed on -- a hook that cannot be - # read has no reportable finding, not a hard failure. + # No exit-code contract to protect here (see this function's own + # caller docstring) -- a hook that cannot be read has no + # reportable finding, not a hard failure of the scan itself. return [] - # Pass 1: which variables get assigned a `.github/scripts/*.py` path - # anywhere in this file. Whole-file rather than "only assignments - # before the invocation line" -- the real shape this repository uses - # always assigns before invoking, and tracking that ordering would add - # dataflow analysis this scan deliberately does not attempt. + # A registered hooks/*.py target's own basename (e.g. + # "gitapex_check_python_precondition.py") at the very end of an + # assignment's right-hand side, after trailing-comment stripping and + # quote trimming -- the real shape this repository's own hooks/*.sh + # files use is always a `$script_dir/.py`-style path, never a + # literal `hooks/.py` substring (that literal-substring shape is + # what `_GITHUB_SCRIPTS_PATH_RE` matches for `.github/scripts/*.py` + # instead, since those are always written relative to repo_root). + hooks_py_target_re = None + if hard_fail_hooks_py_names: + hooks_py_target_re = re.compile( + r"(?:" + "|".join(re.escape(name) for name in sorted(hard_fail_hooks_py_names)) + r")$" + ) + + # Pass 1: which variables get assigned a `.github/scripts/*.py` path, + # or a registered `hooks/*.py` target, anywhere in this file. Whole- + # file rather than "only assignments before the invocation line" -- + # the real shape this repository uses always assigns before invoking, + # and tracking that ordering would add dataflow analysis this scan + # deliberately does not attempt. tracked_vars: set[str] = set() for line in lines: if line.lstrip().startswith("#"): @@ -303,7 +395,8 @@ def _scan_hook(hook: pathlib.Path) -> list[tuple[str, int, str]]: comment = _TRAILING_COMMENT_RE.search(rhs) if comment: rhs = rhs[: comment.start()] - if _GITHUB_SCRIPTS_PATH_RE.search(rhs): + rhs_trimmed = rhs.strip().strip('"').strip("'") + if _GITHUB_SCRIPTS_PATH_RE.search(rhs) or (hooks_py_target_re and hooks_py_target_re.search(rhs_trimmed)): tracked_vars.add(assignment.group(1)) if not tracked_vars: @@ -341,6 +434,7 @@ def _scan_hook(hook: pathlib.Path) -> list[tuple[str, int, str]]: def main() -> int: workflows_dir = pathlib.Path(sys.argv[1]) if len(sys.argv) > 1 else WORKFLOWS_DIR hooks_dir = pathlib.Path(sys.argv[2]) if len(sys.argv) > 2 else HOOKS_DIR + ssot_path = pathlib.Path(sys.argv[3]) if len(sys.argv) > 3 else SSOT_PATH findings = find_bare_invocations(workflows_dir) if findings: @@ -352,17 +446,24 @@ def main() -> int: print("No bare `python3 .github/scripts/*.py` invocations found; every call site uses `uv run`.") exit_code = 0 - # WARNING tier (issue #1446 Item 2): report-only, never contributes to - # exit_code -- see find_hooks_shell_indirected_invocations's own - # docstring and this module's docstring for why. - hooks_findings = find_hooks_shell_indirected_invocations(hooks_dir) + # HARD-FAIL tier (issue #1697; formerly WARNING-only under issue + # #1446 Item 2 -- see find_hooks_shell_indirected_invocations's own + # docstring for why that changed): a bare `python3 "$var"` of a + # `.github/scripts/*.py` target, or of a registered `hooks/*.py` + # target whose own gate declares a non-empty + # `preconditions.requires_python_packages`, now fails this gate the + # same way a workflow-level bare invocation always has. + hard_fail_hooks_py_names = load_python_dependent_hook_script_names(ssot_path) + hooks_findings = find_hooks_shell_indirected_invocations(hooks_dir, hard_fail_hooks_py_names) if hooks_findings: print( - "WARNING (non-blocking): hooks/*.sh shell-variable-indirected bare " - '`python3 "$var"` invocations of a `.github/scripts/*.py` target:' + 'Bare `python3 "$var"` invocations (hooks/*.sh) of a `.github/scripts/*.py` ' + "target, or of a hooks/*.py target whose own gate requires a third-party " + "Python package:" ) for path, lineno, line in hooks_findings: print(f" {path}:{lineno}: {line}") + exit_code = 1 else: print("No hooks/*.sh shell-variable-indirected bare invocations found.") diff --git a/tests/test_gitapex_gate_bare_python3_invocation.py b/tests/test_gitapex_gate_bare_python3_invocation.py index ff686af2..c38beb5f 100644 --- a/tests/test_gitapex_gate_bare_python3_invocation.py +++ b/tests/test_gitapex_gate_bare_python3_invocation.py @@ -6,6 +6,7 @@ from __future__ import annotations +import json import pathlib import time @@ -569,9 +570,17 @@ def test_hooks_shell_indirected_missing_dir_returns_empty(tmp_path: pathlib.Path # change find_bare_invocations's existing hard-fail exit-code behavior --- -def test_main_exit_code_stays_zero_with_only_a_hooks_warning_finding( +def test_main_exit_code_becomes_one_with_a_hooks_shell_indirected_finding( tmp_path: pathlib.Path, capsys: pytest.CaptureFixture[str], monkeypatch: pytest.MonkeyPatch ) -> None: + """Issue #1697: a hooks/*.sh shell-variable-indirected bare invocation + of a `.github/scripts/*.py` target is now a HARD FAIL, not a + report-only WARNING (formerly issue #1446 Item 2's own WARNING-tier + addition) -- see this repository's own live incident (a bare + `python3` invocation of exactly this shape, inside + hooks/check-pr-skill-audit-disclosure.sh, false-denied + create_pull_request/update_pull_request under an ambient PATH lacking + this checkout's own uv-managed .venv).""" workflows_dir = _write( tmp_path, "clean.yml", @@ -583,7 +592,7 @@ def test_main_exit_code_stays_zero_with_only_a_hooks_warning_finding( '#!/bin/bash\nfull_gate="${repo_root}/.github/scripts/gitapex_gate_foo.py"\npython3 "$full_gate"\n', ) monkeypatch.setattr("sys.argv", ["prog", str(workflows_dir), str(hooks_dir)]) - assert gate.main() == 0 + assert gate.main() == 1 out = capsys.readouterr().out assert "full_gate" in out @@ -683,3 +692,275 @@ def test_scan_hook_ignores_a_github_scripts_path_mentioned_only_in_a_trailing_co 'python3 "$other_var"\n', ) assert gate.find_hooks_shell_indirected_invocations(hooks_dir) == [] + + +# --- load_python_dependent_hook_script_names() (issue #1697) --- + + +def _write_ssot(tmp_path: pathlib.Path, gates: list[dict[str, object]]) -> pathlib.Path: + ssot_path = tmp_path / "ssot.json" + ssot_path.write_text(json.dumps({"gates": gates}), encoding="utf-8") + return ssot_path + + +def test_load_python_dependent_hook_script_names_returns_hooks_py_basenames(tmp_path: pathlib.Path) -> None: + ssot_path = _write_ssot( + tmp_path, + [ + { + "id": "skill-audit-disclosure", + "script": [ + "hooks/check-pr-skill-audit-disclosure.sh", + "hooks/gitapex_check_python_precondition.py", + ".github/scripts/gitapex_gate_skill_audit_disclosure.py", + ], + "preconditions": {"requires_python_packages": ["pydantic"]}, + } + ], + ) + assert gate.load_python_dependent_hook_script_names(ssot_path) == frozenset( + {"gitapex_check_python_precondition.py"} + ) + + +def test_load_python_dependent_hook_script_names_ignores_gates_with_no_preconditions(tmp_path: pathlib.Path) -> None: + ssot_path = _write_ssot( + tmp_path, + [ + { + "id": "some-other-gate", + "script": ["hooks/gitapex_check_something.py"], + } + ], + ) + assert gate.load_python_dependent_hook_script_names(ssot_path) == frozenset() + + +def test_load_python_dependent_hook_script_names_ignores_empty_requires_python_packages( + tmp_path: pathlib.Path, +) -> None: + ssot_path = _write_ssot( + tmp_path, + [ + { + "id": "some-other-gate", + "script": ["hooks/gitapex_check_something.py"], + "preconditions": {"requires_python_packages": []}, + } + ], + ) + assert gate.load_python_dependent_hook_script_names(ssot_path) == frozenset() + + +def test_load_python_dependent_hook_script_names_ignores_non_hooks_py_scripts(tmp_path: pathlib.Path) -> None: + ssot_path = _write_ssot( + tmp_path, + [ + { + "id": "skill-audit-disclosure", + "script": [ + "hooks/check-pr-skill-audit-disclosure.sh", + ".github/scripts/gitapex_gate_skill_audit_disclosure.py", + ], + "preconditions": {"requires_python_packages": ["pydantic"]}, + } + ], + ) + assert gate.load_python_dependent_hook_script_names(ssot_path) == frozenset() + + +def test_load_python_dependent_hook_script_names_missing_file_returns_empty(tmp_path: pathlib.Path) -> None: + assert gate.load_python_dependent_hook_script_names(tmp_path / "does-not-exist.json") == frozenset() + + +def test_load_python_dependent_hook_script_names_invalid_json_returns_empty(tmp_path: pathlib.Path) -> None: + bad = tmp_path / "bad.json" + bad.write_text("not valid json{", encoding="utf-8") + assert gate.load_python_dependent_hook_script_names(bad) == frozenset() + + +def test_load_python_dependent_hook_script_names_non_mapping_top_level_returns_empty(tmp_path: pathlib.Path) -> None: + weird = tmp_path / "weird.json" + weird.write_text("[1, 2, 3]", encoding="utf-8") + assert gate.load_python_dependent_hook_script_names(weird) == frozenset() + + +def test_load_python_dependent_hook_script_names_malformed_gate_entries_are_skipped(tmp_path: pathlib.Path) -> None: + """Defeat case: a `gates` array containing shapes this loader must not + crash on -- a non-mapping entry, a non-list `script`, a non-mapping + `preconditions`, and a non-list `requires_python_packages` -- none of + which should raise or contribute a name.""" + ssot_path = _write_ssot( + tmp_path, + [ + "not-a-mapping", # type: ignore[list-item] + {"id": "a", "script": "not-a-list", "preconditions": {"requires_python_packages": ["x"]}}, + {"id": "b", "script": ["hooks/foo.py"], "preconditions": "not-a-mapping"}, + {"id": "c", "script": ["hooks/bar.py"], "preconditions": {"requires_python_packages": "not-a-list"}}, + ], + ) + assert gate.load_python_dependent_hook_script_names(ssot_path) == frozenset() + + +# --- HARD-FAIL promotion of hooks/*.py targets (issue #1697) --- + + +def test_hooks_shell_indirected_registered_hooks_py_target_is_flagged(tmp_path: pathlib.Path) -> None: + """Reproduces hooks/check-pr-skill-audit-disclosure.sh's own real + defect (issue #1697): a bare `python3 "$precondition_script"` + invocation of a `hooks/*.py` file registered under a gate that + declares a non-empty `preconditions.requires_python_packages` is now + flagged, closing the exact blind spot that let this defect ship.""" + hooks_dir = _write_hook( + tmp_path, + "check-pr-skill-audit-disclosure.sh", + "#!/bin/bash\n" + 'script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"\n' + 'precondition_script="$script_dir/gitapex_check_python_precondition.py"\n' + 'precondition_json=$(python3 "$precondition_script" -- pydantic)\n', + ) + findings = gate.find_hooks_shell_indirected_invocations( + hooks_dir, frozenset({"gitapex_check_python_precondition.py"}) + ) + assert len(findings) == 1 + assert "precondition_script" in findings[0][2] + + +def test_hooks_shell_indirected_unregistered_hooks_py_target_is_still_not_flagged(tmp_path: pathlib.Path) -> None: + """A hooks/*.py target NOT named in hard_fail_hooks_py_names stays + exactly as before (bare-invoked by design, per + docs/repository-layout.md) -- the promotion is scoped to registered, + third-party-dependent targets only, not every hooks/*.py sibling.""" + hooks_dir = _write_hook( + tmp_path, + "check-bash-safety.sh", + "#!/bin/bash\n" + 'classifier="${repo_root}/hooks/gitapex_check_bash_safety.py"\n' + 'classifier_output=$(printf %s "$input" | python3 "$classifier" 2>/dev/null)\n', + ) + findings = gate.find_hooks_shell_indirected_invocations( + hooks_dir, frozenset({"gitapex_check_python_precondition.py"}) + ) + assert findings == [] + + +def test_hooks_shell_indirected_registered_hooks_py_target_uv_wrapped_is_not_flagged(tmp_path: pathlib.Path) -> None: + hooks_dir = _write_hook( + tmp_path, + "fixed.sh", + "#!/bin/bash\n" + 'precondition_script="$script_dir/gitapex_check_python_precondition.py"\n' + 'uv run --frozen python3 "$precondition_script" -- pydantic\n', + ) + findings = gate.find_hooks_shell_indirected_invocations( + hooks_dir, frozenset({"gitapex_check_python_precondition.py"}) + ) + assert findings == [] + + +def test_hooks_shell_indirected_registered_hooks_py_target_via_python3_cmd_array_is_not_flagged( + tmp_path: pathlib.Path, +) -> None: + """The actual fix shape this repository's own hooks/*.sh files now + use (a `python3_cmd` array resolved to either `uv run --frozen + python3` or a bare `python3` fallback) must not itself be + misdetected as a NEW bare invocation -- "python3" only ever appears + here as a substring of the array variable's own name + (`python3_cmd`), never as a standalone token immediately followed by + the target variable.""" + hooks_dir = _write_hook( + tmp_path, + "fixed.sh", + "#!/bin/bash\n" + 'precondition_script="$script_dir/gitapex_check_python_precondition.py"\n' + "python3_cmd=(python3)\n" + "if command -v uv >/dev/null 2>&1; then python3_cmd=(uv run --frozen python3); fi\n" + '"${python3_cmd[@]}" "$precondition_script" -- pydantic\n', + ) + findings = gate.find_hooks_shell_indirected_invocations( + hooks_dir, frozenset({"gitapex_check_python_precondition.py"}) + ) + assert findings == [] + + +def test_hooks_shell_indirected_hooks_py_name_substring_does_not_cross_match(tmp_path: pathlib.Path) -> None: + """Defeat case: a decoy file `gitapex_check_python_precondition_extra.py` + must not be treated as a match for the registered + `gitapex_check_python_precondition.py` merely because one name is a + substring of the other's own prefix -- the trailing `$` anchor in the + generated pattern requires the registered name to end the (quote- + trimmed) right-hand side exactly.""" + hooks_dir = _write_hook( + tmp_path, + "decoy.sh", + '#!/bin/bash\ndecoy_script="$script_dir/gitapex_check_python_precondition_extra.py"\npython3 "$decoy_script"\n', + ) + findings = gate.find_hooks_shell_indirected_invocations( + hooks_dir, frozenset({"gitapex_check_python_precondition.py"}) + ) + assert findings == [] + + +def test_main_flags_a_registered_hooks_py_target_and_exits_one( + tmp_path: pathlib.Path, capsys: pytest.CaptureFixture[str], monkeypatch: pytest.MonkeyPatch +) -> None: + workflows_dir = _write( + tmp_path, + "clean.yml", + "jobs:\n a:\n steps:\n - name: run\n run: uv run --frozen python3 .github/scripts/x.py\n", + ) + hooks_dir = _write_hook( + tmp_path, + "check-pr-skill-audit-disclosure.sh", + "#!/bin/bash\n" + 'precondition_script="$script_dir/gitapex_check_python_precondition.py"\n' + 'python3 "$precondition_script" -- pydantic\n', + ) + ssot_path = _write_ssot( + tmp_path, + [ + { + "id": "skill-audit-disclosure", + "script": ["hooks/gitapex_check_python_precondition.py"], + "preconditions": {"requires_python_packages": ["pydantic"]}, + } + ], + ) + monkeypatch.setattr("sys.argv", ["prog", str(workflows_dir), str(hooks_dir), str(ssot_path)]) + assert gate.main() == 1 + out = capsys.readouterr().out + assert "precondition_script" in out + + +def test_main_returns_zero_when_no_gate_requires_a_python_package( + tmp_path: pathlib.Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A hooks/*.py target invoked bare stays clean when no registered + gate declares a `requires_python_packages` precondition for it -- + the pre-#1697 behavior for every ordinary, stdlib-only hooks/*.py + sibling.""" + workflows_dir = _write( + tmp_path, + "clean.yml", + "jobs:\n a:\n steps:\n - name: run\n run: uv run --frozen python3 .github/scripts/x.py\n", + ) + hooks_dir = _write_hook( + tmp_path, + "ordinary.sh", + '#!/bin/bash\nclassifier="$script_dir/gitapex_check_bash_safety.py"\npython3 "$classifier"\n', + ) + ssot_path = _write_ssot(tmp_path, []) + monkeypatch.setattr("sys.argv", ["prog", str(workflows_dir), str(hooks_dir), str(ssot_path)]) + assert gate.main() == 0 + + +# --- live proof against this repository's own real ssot.json + hooks/ --- + + +def test_this_repositorys_own_hooks_have_no_hard_fail_indirected_invocation() -> None: + """After issue #1697's own fix lands, this is the actual regression + backstop: scan this repository's REAL hooks/ directory against its + REAL ssot.json, exactly as CI/local-preflight will.""" + hard_fail_names = gate.load_python_dependent_hook_script_names(REPO_ROOT / ".gitapex" / "ssot.json") + findings = gate.find_hooks_shell_indirected_invocations(REPO_ROOT / "hooks", hard_fail_names) + assert findings == [], findings From 47c3e9c177c7533fc886c79876b48c137b95e4ec Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 3 Sep 2026 01:07:43 +0000 Subject: [PATCH 4/9] test(gates): cover load_python_dependent_hook_script_names and _scan_hook directly local-preflight's detection-logic-property-coverage and function-body-test-coverage gates flagged the previous commit's own gitapex_gate_bare_python3_invocation.py changes: the new load_python_dependent_hook_script_names string-comparison call site had no hypothesis @given property test, and the changed _scan_hook body had no test in the same diff mentioning it by name (existing tests only called it through find_hooks_shell_indirected_invocations's wrapper). Adds direct _scan_hook unit tests for a registered vs. unregistered hooks/*.py target, plus hypothesis property tests for load_python_dependent_hook_script_names covering: a required-package gate contributes its own hooks/*.py basename (never a sibling .github/scripts/*.py entry on the same gate), a gate with no preconditions never contributes one, and an empty requires_python_packages list is treated the same as absent. Refs #1697 Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01BGHXhvkGRDFxGBARLbmh81 --- ...st_gitapex_gate_bare_python3_invocation.py | 31 ++++++++ ...gate_bare_python3_invocation_properties.py | 77 +++++++++++++++++++ 2 files changed, 108 insertions(+) diff --git a/tests/test_gitapex_gate_bare_python3_invocation.py b/tests/test_gitapex_gate_bare_python3_invocation.py index c38beb5f..8807860b 100644 --- a/tests/test_gitapex_gate_bare_python3_invocation.py +++ b/tests/test_gitapex_gate_bare_python3_invocation.py @@ -805,6 +805,37 @@ def test_load_python_dependent_hook_script_names_malformed_gate_entries_are_skip # --- HARD-FAIL promotion of hooks/*.py targets (issue #1697) --- +def test_scan_hook_directly_flags_a_registered_hooks_py_target(tmp_path: pathlib.Path) -> None: + """Calls `_scan_hook` directly (not through + `find_hooks_shell_indirected_invocations`'s directory-level wrapper) + to cover its own new `hard_fail_hooks_py_names` parameter -- the same + real defect shape as `test_hooks_shell_indirected_registered_hooks_py_target_is_flagged` + below, exercised at the function this repository's own diff actually + changed.""" + hook = tmp_path / "check-pr-skill-audit-disclosure.sh" + hook.write_text( + "#!/bin/bash\n" + 'script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"\n' + 'precondition_script="$script_dir/gitapex_check_python_precondition.py"\n' + 'precondition_json=$(python3 "$precondition_script" -- pydantic)\n', + encoding="utf-8", + ) + findings = gate._scan_hook(hook, frozenset({"gitapex_check_python_precondition.py"})) + assert len(findings) == 1 + assert "precondition_script" in findings[0][2] + + +def test_scan_hook_directly_leaves_an_unregistered_hooks_py_target_unflagged(tmp_path: pathlib.Path) -> None: + hook = tmp_path / "check-bash-safety.sh" + hook.write_text( + "#!/bin/bash\n" + 'classifier="${repo_root}/hooks/gitapex_check_bash_safety.py"\n' + 'classifier_output=$(printf %s "$input" | python3 "$classifier" 2>/dev/null)\n', + encoding="utf-8", + ) + assert gate._scan_hook(hook, frozenset({"gitapex_check_python_precondition.py"})) == [] + + def test_hooks_shell_indirected_registered_hooks_py_target_is_flagged(tmp_path: pathlib.Path) -> None: """Reproduces hooks/check-pr-skill-audit-disclosure.sh's own real defect (issue #1697): a bare `python3 "$precondition_script"` diff --git a/tests/test_gitapex_gate_bare_python3_invocation_properties.py b/tests/test_gitapex_gate_bare_python3_invocation_properties.py index 64ce7b54..0644c14a 100644 --- a/tests/test_gitapex_gate_bare_python3_invocation_properties.py +++ b/tests/test_gitapex_gate_bare_python3_invocation_properties.py @@ -30,6 +30,7 @@ from __future__ import annotations +import json import pathlib import tempfile @@ -42,6 +43,8 @@ _VARNAMES = st.from_regex(r"[A-Za-z_][A-Za-z0-9_]{0,12}", fullmatch=True) _SCRIPT_STEMS = st.from_regex(r"[a-z][a-z0-9_]{0,16}", fullmatch=True) _PREFIXES = st.sampled_from(["", "${repo_root}", "$repo_root", "/abs/path"]) +_GATE_IDS = st.from_regex(r"[a-z][a-z0-9-]{0,20}", fullmatch=True) +_PACKAGE_NAMES = st.from_regex(r"[a-z][a-z0-9_-]{0,16}", fullmatch=True) @_PROPERTIES @@ -111,6 +114,80 @@ def test_commented_out_assignment_and_invocation_are_ignored(varname: str, stem: assert findings == [] +@_PROPERTIES +@given(gate_id=_GATE_IDS, stem=_SCRIPT_STEMS, package=_PACKAGE_NAMES) +def test_load_python_dependent_hook_script_names_extracts_registered_hooks_py( + gate_id: str, stem: str, package: str +) -> None: + """**Model-based property (issue #1697):** for ANY gate id, + hooks/*.py stem, and required-package name, a gate declaring a + non-empty `preconditions.requires_python_packages` must contribute + its own `hooks/.py` script's basename to the result -- the + exact shape `hooks/gitapex_check_python_precondition.py`'s own + missing `skill-audit-disclosure` registry entry needed to be caught + by this loader. A sibling `.github/scripts/*.py` entry on the same + gate must never itself be returned (this loader only ever tracks + `hooks/*.py` targets, since `.github/scripts/*.py` is already covered + by `find_bare_invocations`'s own workflow-level scan).""" + ssot = { + "gates": [ + { + "id": gate_id, + "script": [f"hooks/{stem}.py", ".github/scripts/unrelated.py"], + "preconditions": {"requires_python_packages": [package]}, + } + ] + } + with tempfile.TemporaryDirectory() as tmp: + ssot_path = pathlib.Path(tmp) / "ssot.json" + ssot_path.write_text(json.dumps(ssot), encoding="utf-8") + result = gate.load_python_dependent_hook_script_names(ssot_path) + assert result == frozenset({f"{stem}.py"}) + + +@_PROPERTIES +@given(gate_id=_GATE_IDS, stem=_SCRIPT_STEMS) +def test_load_python_dependent_hook_script_names_ignores_gates_without_required_packages( + gate_id: str, stem: str +) -> None: + """The mirror-image property: for ANY gate id/stem, a gate with NO + `preconditions` key at all must never contribute its own `hooks/*.py` + script -- only a gate that actually declares a non-empty + `requires_python_packages` widens this scan's scope.""" + ssot = {"gates": [{"id": gate_id, "script": [f"hooks/{stem}.py"]}]} + with tempfile.TemporaryDirectory() as tmp: + ssot_path = pathlib.Path(tmp) / "ssot.json" + ssot_path.write_text(json.dumps(ssot), encoding="utf-8") + result = gate.load_python_dependent_hook_script_names(ssot_path) + assert result == frozenset() + + +@_PROPERTIES +@given(gate_id=_GATE_IDS, stem=_SCRIPT_STEMS, package=_PACKAGE_NAMES) +def test_load_python_dependent_hook_script_names_ignores_empty_requires_python_packages( + gate_id: str, stem: str, package: str +) -> None: + """A gate whose `requires_python_packages` is present but empty must + be treated the same as one with no `preconditions` at all, for ANY + generated gate id/stem -- `package` here only pins the strategy type, + it is never actually placed in the empty list.""" + del package + ssot = { + "gates": [ + { + "id": gate_id, + "script": [f"hooks/{stem}.py"], + "preconditions": {"requires_python_packages": []}, + } + ] + } + with tempfile.TemporaryDirectory() as tmp: + ssot_path = pathlib.Path(tmp) / "ssot.json" + ssot_path.write_text(json.dumps(ssot), encoding="utf-8") + result = gate.load_python_dependent_hook_script_names(ssot_path) + assert result == frozenset() + + @_PROPERTIES @given(text=st.text(max_size=500)) def test_scan_hook_never_raises_and_is_deterministic(text: str) -> None: From faab3c1acf57e64862550b15f4a80bc8522795c3 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 3 Sep 2026 01:21:57 +0000 Subject: [PATCH 5/9] test(gates): cover the gates-key-not-a-list branch in the ssot loader codecov/patch flagged PR #1701's head commit at 98.36% patch coverage (target 99.66%), missing gitapex_gate_bare_python3_invocation.py:310 -- load_python_dependent_hook_script_names's own early-return when .gitapex/ssot.json's "gates" key is present but not a list (a malformed registry shape distinct from the already-covered missing-file, undecodable, and non-mapping-top-level cases). Refs #1697 Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01BGHXhvkGRDFxGBARLbmh81 --- tests/test_gitapex_gate_bare_python3_invocation.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/test_gitapex_gate_bare_python3_invocation.py b/tests/test_gitapex_gate_bare_python3_invocation.py index 8807860b..39602497 100644 --- a/tests/test_gitapex_gate_bare_python3_invocation.py +++ b/tests/test_gitapex_gate_bare_python3_invocation.py @@ -769,6 +769,12 @@ def test_load_python_dependent_hook_script_names_ignores_non_hooks_py_scripts(tm assert gate.load_python_dependent_hook_script_names(ssot_path) == frozenset() +def test_load_python_dependent_hook_script_names_gates_key_not_a_list_returns_empty(tmp_path: pathlib.Path) -> None: + ssot_path = tmp_path / "ssot.json" + ssot_path.write_text(json.dumps({"gates": "not-a-list"}), encoding="utf-8") + assert gate.load_python_dependent_hook_script_names(ssot_path) == frozenset() + + def test_load_python_dependent_hook_script_names_missing_file_returns_empty(tmp_path: pathlib.Path) -> None: assert gate.load_python_dependent_hook_script_names(tmp_path / "does-not-exist.json") == frozenset() From f6e97a70b3cafc686cb1fb35a184d691d79aa98e Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 3 Sep 2026 01:46:44 +0000 Subject: [PATCH 6/9] fix(gates): close 2 findings from an independent deterministic-gate review An independent evaluating-deterministic-gate-quality review of PR #1701 (dispatched via a fresh, isolated subagent per that skill's own Subagent-dispatch rule) live-confirmed two real defects in this PR's own gitapex_gate_bare_python3_invocation.py changes: 1. Dimension-15 (fail-closed) violation: load_python_dependent_hook_script_names silently degraded to an empty frozenset on a missing/unreadable/malformed .gitapex/ssot.json, which find_hooks_shell_indirected_invocations then read as "nothing registered" -- so a crafted malformed ssot.json alongside a real bare python3 invocation of a registered hooks/*.py target produced a false "No ... bare invocations found" and exit 0. This broke the same file's own established convention (find_bare_invocations/_scan_workflow already treat an unreadable input as a "cannot verify" finding forcing exit_code=1). Fixed: load_python_dependent_hook_script_names now returns None (not an empty frozenset) on any unreadable/malformed registry; main() treats None as a hard failure while still running the .github/scripts/*.py scan against an empty hooks/*.py scope, matching find_bare_invocations's own "still scan what you can, but report the inability to verify" precedent. Live-verified: a malformed ssot.json now exits 1 with "Could not read or parse ..." instead of the prior false-clean exit 0. 2. Missing path-boundary anchor: the registered-hooks/*.py-target regex anchored only on a trailing "$", so an unregistered file whose own name merely ENDS with a registered name as a substring (e.g. "my_other_gitapex_check_python_precondition.py" against registered "gitapex_check_python_precondition.py") would false-positive. Fixed by anchoring on "(?:^|/)" before the name, requiring a real path-component boundary. Live-verified the decoy no longer flags. Both fixes covered by new unit tests (including the exact decoy/malformed inputs above) plus updated existing tests whose expected return value changed from frozenset() to None for the four genuinely-malformed-registry cases (a well-formed-but-empty registry, or one with no matching gates, still correctly returns frozenset()). Refs #1697 Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01BGHXhvkGRDFxGBARLbmh81 --- .../gitapex_gate_bare_python3_invocation.py | 46 ++++++++--- ...st_gitapex_gate_bare_python3_invocation.py | 82 +++++++++++++++++-- 2 files changed, 110 insertions(+), 18 deletions(-) diff --git a/.github/scripts/gitapex_gate_bare_python3_invocation.py b/.github/scripts/gitapex_gate_bare_python3_invocation.py index b43ccd32..4055604d 100644 --- a/.github/scripts/gitapex_gate_bare_python3_invocation.py +++ b/.github/scripts/gitapex_gate_bare_python3_invocation.py @@ -284,7 +284,7 @@ def _scan_workflow(workflow: pathlib.Path) -> list[tuple[str, int, str]]: return findings -def load_python_dependent_hook_script_names(ssot_path: pathlib.Path = SSOT_PATH) -> frozenset[str]: +def load_python_dependent_hook_script_names(ssot_path: pathlib.Path = SSOT_PATH) -> frozenset[str] | None: """Return the basenames of every `hooks/*.py` file registered in `.gitapex/ssot.json` under a gate whose own `preconditions.requires_python_packages` is non-empty (issue #1697): a @@ -294,20 +294,24 @@ def load_python_dependent_hook_script_names(ssot_path: pathlib.Path = SSOT_PATH) package that gate needs), the same reason a bare `python3 .github/scripts/*.py` invocation is already a hard failure below. - Degrades to an empty result (never raises) when the registry is - missing, unreadable, or does not parse to the expected shape -- the - caller then simply has no additional `hooks/*.py` targets to widen its - existing `.github/scripts/*.py`-only scope with, rather than crashing - this whole gate on an unreadable registry.""" + Returns `None` (never an empty result masquerading as "nothing + registered") when the registry is missing, unreadable, or does not + parse to the expected shape: the caller must treat that the same + fail-closed way `find_bare_invocations`/`_scan_workflow` already treat + an unreadable workflow file -- a "cannot verify" finding, not a + silent "no additional targets" pass (adversarial-review finding, + issue #1697: a malformed `.gitapex/ssot.json` alongside a real bare + invocation of a registered `hooks/*.py` target previously made this + whole gate report "No ... bare invocations found" and exit 0).""" try: data = json.loads(ssot_path.read_text(encoding="utf-8")) except (OSError, UnicodeDecodeError, json.JSONDecodeError): - return frozenset() + return None if not isinstance(data, dict): - return frozenset() + return None gates = data.get("gates") if not isinstance(gates, list): - return frozenset() + return None names: set[str] = set() for gate in gates: @@ -372,10 +376,17 @@ def _scan_hook( # literal `hooks/.py` substring (that literal-substring shape is # what `_GITHUB_SCRIPTS_PATH_RE` matches for `.github/scripts/*.py` # instead, since those are always written relative to repo_root). + # `(?:^|/)` anchors the match to a real path-component boundary -- + # without it, an unregistered file whose own name merely ENDS with a + # registered name as a substring (e.g. + # "my_other_gitapex_check_python_precondition.py" against registered + # "gitapex_check_python_precondition.py") would false-positive, since + # a bare trailing `$` allows any preceding character (adversarial- + # review finding, issue #1697). hooks_py_target_re = None if hard_fail_hooks_py_names: hooks_py_target_re = re.compile( - r"(?:" + "|".join(re.escape(name) for name in sorted(hard_fail_hooks_py_names)) + r")$" + r"(?:^|/)(?:" + "|".join(re.escape(name) for name in sorted(hard_fail_hooks_py_names)) + r")$" ) # Pass 1: which variables get assigned a `.github/scripts/*.py` path, @@ -453,7 +464,22 @@ def main() -> int: # target whose own gate declares a non-empty # `preconditions.requires_python_packages`, now fails this gate the # same way a workflow-level bare invocation always has. + # + # `None` (registry missing/unreadable/malformed) is itself a hard + # failure here, not a silent "nothing extra to widen with" pass -- + # adversarial-review finding, issue #1697: without this, a malformed + # .gitapex/ssot.json made this whole gate report a false "clean" + # verdict even with a real bare invocation of a registered target + # present, the exact silent-degrade class this gate exists to close. + # The `.github/scripts/*.py`-only scan below still runs against an + # empty hooks/*.py scope rather than being skipped entirely, matching + # find_bare_invocations's own "still scan what you can, but report the + # inability to verify" precedent. hard_fail_hooks_py_names = load_python_dependent_hook_script_names(ssot_path) + if hard_fail_hooks_py_names is None: + print(f"Could not read or parse {ssot_path} to determine hooks/*.py third-party-dependent targets.") + exit_code = 1 + hard_fail_hooks_py_names = frozenset() hooks_findings = find_hooks_shell_indirected_invocations(hooks_dir, hard_fail_hooks_py_names) if hooks_findings: print( diff --git a/tests/test_gitapex_gate_bare_python3_invocation.py b/tests/test_gitapex_gate_bare_python3_invocation.py index 39602497..6c7982aa 100644 --- a/tests/test_gitapex_gate_bare_python3_invocation.py +++ b/tests/test_gitapex_gate_bare_python3_invocation.py @@ -769,26 +769,31 @@ def test_load_python_dependent_hook_script_names_ignores_non_hooks_py_scripts(tm assert gate.load_python_dependent_hook_script_names(ssot_path) == frozenset() -def test_load_python_dependent_hook_script_names_gates_key_not_a_list_returns_empty(tmp_path: pathlib.Path) -> None: +def test_load_python_dependent_hook_script_names_gates_key_not_a_list_returns_none(tmp_path: pathlib.Path) -> None: + """A malformed registry returns None (a "cannot verify" signal), never + an empty frozenset that a caller could mistake for "nothing + registered" -- issue #1697 adversarial-review finding: the latter + shape let a malformed .gitapex/ssot.json silently mask a real bare + invocation of a registered hooks/*.py target.""" ssot_path = tmp_path / "ssot.json" ssot_path.write_text(json.dumps({"gates": "not-a-list"}), encoding="utf-8") - assert gate.load_python_dependent_hook_script_names(ssot_path) == frozenset() + assert gate.load_python_dependent_hook_script_names(ssot_path) is None -def test_load_python_dependent_hook_script_names_missing_file_returns_empty(tmp_path: pathlib.Path) -> None: - assert gate.load_python_dependent_hook_script_names(tmp_path / "does-not-exist.json") == frozenset() +def test_load_python_dependent_hook_script_names_missing_file_returns_none(tmp_path: pathlib.Path) -> None: + assert gate.load_python_dependent_hook_script_names(tmp_path / "does-not-exist.json") is None -def test_load_python_dependent_hook_script_names_invalid_json_returns_empty(tmp_path: pathlib.Path) -> None: +def test_load_python_dependent_hook_script_names_invalid_json_returns_none(tmp_path: pathlib.Path) -> None: bad = tmp_path / "bad.json" bad.write_text("not valid json{", encoding="utf-8") - assert gate.load_python_dependent_hook_script_names(bad) == frozenset() + assert gate.load_python_dependent_hook_script_names(bad) is None -def test_load_python_dependent_hook_script_names_non_mapping_top_level_returns_empty(tmp_path: pathlib.Path) -> None: +def test_load_python_dependent_hook_script_names_non_mapping_top_level_returns_none(tmp_path: pathlib.Path) -> None: weird = tmp_path / "weird.json" weird.write_text("[1, 2, 3]", encoding="utf-8") - assert gate.load_python_dependent_hook_script_names(weird) == frozenset() + assert gate.load_python_dependent_hook_script_names(weird) is None def test_load_python_dependent_hook_script_names_malformed_gate_entries_are_skipped(tmp_path: pathlib.Path) -> None: @@ -938,6 +943,31 @@ def test_hooks_shell_indirected_hooks_py_name_substring_does_not_cross_match(tmp assert findings == [] +def test_hooks_shell_indirected_unregistered_name_ending_in_a_registered_name_is_not_flagged( + tmp_path: pathlib.Path, +) -> None: + """Mirror-image defeat case (adversarial-review finding, issue + #1697): a decoy file `my_other_gitapex_check_python_precondition.py` + must not be treated as a match merely because the registered name + `gitapex_check_python_precondition.py` is a SUFFIX of the decoy's own + name -- the `(?:^|/)` boundary requires the registered name to start + right after a path separator (or the start of the value), not merely + end the value. Without that boundary, a bare trailing `$` anchor + alone would wrongly flag this decoy, over-blocking a target that was + never registered.""" + hooks_dir = _write_hook( + tmp_path, + "decoy2.sh", + "#!/bin/bash\n" + 'decoy_script="$script_dir/my_other_gitapex_check_python_precondition.py"\n' + 'python3 "$decoy_script"\n', + ) + findings = gate.find_hooks_shell_indirected_invocations( + hooks_dir, frozenset({"gitapex_check_python_precondition.py"}) + ) + assert findings == [] + + def test_main_flags_a_registered_hooks_py_target_and_exits_one( tmp_path: pathlib.Path, capsys: pytest.CaptureFixture[str], monkeypatch: pytest.MonkeyPatch ) -> None: @@ -991,6 +1021,41 @@ def test_main_returns_zero_when_no_gate_requires_a_python_package( assert gate.main() == 0 +def test_main_hard_fails_on_an_unreadable_ssot_registry_even_with_no_other_findings( + tmp_path: pathlib.Path, capsys: pytest.CaptureFixture[str], monkeypatch: pytest.MonkeyPatch +) -> None: + """Regression for the issue #1697 adversarial-review finding: before + this fix, a malformed .gitapex/ssot.json made + load_python_dependent_hook_script_names degrade to an empty + frozenset, which find_hooks_shell_indirected_invocations then read as + "nothing registered" -- so a real bare invocation of a registered + hooks/*.py target went completely undetected AND main() printed a + false "No ... bare invocations found" while exiting 0. This asserts + main() now hard-fails on the unreadable registry itself, matching + find_bare_invocations's own established "cannot verify" convention, + even though the hooks/*.sh scan below it finds nothing (since it has + no registered names to check against).""" + workflows_dir = _write( + tmp_path, + "clean.yml", + "jobs:\n a:\n steps:\n - name: run\n run: uv run --frozen python3 .github/scripts/x.py\n", + ) + hooks_dir = _write_hook( + tmp_path, + "check-pr-skill-audit-disclosure.sh", + "#!/bin/bash\n" + 'precondition_script="$script_dir/gitapex_check_python_precondition.py"\n' + 'python3 "$precondition_script" -- pydantic\n', + ) + bad_ssot_path = tmp_path / "bad_ssot.json" + bad_ssot_path.write_text("not valid json{", encoding="utf-8") + monkeypatch.setattr("sys.argv", ["prog", str(workflows_dir), str(hooks_dir), str(bad_ssot_path)]) + assert gate.main() == 1 + out = capsys.readouterr().out + assert "Could not read or parse" in out + assert str(bad_ssot_path) in out + + # --- live proof against this repository's own real ssot.json + hooks/ --- @@ -999,5 +1064,6 @@ def test_this_repositorys_own_hooks_have_no_hard_fail_indirected_invocation() -> backstop: scan this repository's REAL hooks/ directory against its REAL ssot.json, exactly as CI/local-preflight will.""" hard_fail_names = gate.load_python_dependent_hook_script_names(REPO_ROOT / ".gitapex" / "ssot.json") + assert hard_fail_names is not None, "this repository's own .gitapex/ssot.json must be readable" findings = gate.find_hooks_shell_indirected_invocations(REPO_ROOT / "hooks", hard_fail_names) assert findings == [], findings From 129abc45f8d7a027175488cce719d9b9ac73c8c1 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 3 Sep 2026 03:16:28 +0000 Subject: [PATCH 7/9] docs(adr): record the uv-preferred python3 resolution decision Retrofit ADR for PR #1701's own decision (hooks/*.sh companion-script invocations prefer `uv run --frozen python3` when this checkout owns a uv toolchain and lockfile, falling back to the pre-existing bare `python3` otherwise) that fixed issue #1697's PATH-dependent false-deny and closed out issue #1581's residual scope on the same defect class. Approved by tvna, 2026-09-03. Refs #1697, #1581 Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01BGHXhvkGRDFxGBARLbmh81 --- ...er-uv-resolved-python3-in-hooks-scripts.md | 123 ++++++++++++++++++ 1 file changed, 123 insertions(+) create mode 100644 docs/adr/0003-prefer-uv-resolved-python3-in-hooks-scripts.md diff --git a/docs/adr/0003-prefer-uv-resolved-python3-in-hooks-scripts.md b/docs/adr/0003-prefer-uv-resolved-python3-in-hooks-scripts.md new file mode 100644 index 00000000..20c05d67 --- /dev/null +++ b/docs/adr/0003-prefer-uv-resolved-python3-in-hooks-scripts.md @@ -0,0 +1,123 @@ +# Prefer uv-resolved python3 in hooks/*.sh, with a bare-python3 fallback + +## Status + +Accepted (approved by tvna, 2026-09-03) + +## Context and Problem Statement + +This decision is already implemented, on branch `claude/fix-python-path-resolution-q2e3pv` +(PR #1701, not yet merged as of this writing) -- this is a retrofit +record, written after the change, not before it. + +`hooks/*.sh` files are this repository's own agent-harness hook +subprocesses (PreToolUse/Stop, etc.). Several of them invoke a companion +Python checker script. Before this change, that invocation was a bare +`python3 "$script"` call, which resolves the `python3` binary from +whatever `PATH` the *calling* hook context happens to have at +invocation time -- not necessarily this repository's own uv-managed +`.venv`. + +Issue #1697 reported the resulting defect directly: +`hooks/check-pr-skill-audit-disclosure.sh`'s precondition probe denied a +real `create_pull_request` call with `"python3 cannot import: pydantic"`, +even though `uv sync --group dev` had already installed pydantic into +this checkout's own `.venv` -- because the ambient `PATH` at hook- +invocation time resolved a different `python3` that could not see that +`.venv`. Issue #1581 raised the same PATH-dependent-interpreter defect +class against a different call site. + +A conflicting constraint narrows the fix: per `docs/repository-layout.md`, +only `skills/` and `hooks/` are ever deployed to a *consumer* plugin +install of this repository, and a consumer install carries no `uv` +toolchain or lockfile of its own. An unconditional switch to `uv run` in +every `hooks/*.sh` file would resolve this repository's own dev-checkout +bug but break every consumer install outright. + +## Considered Options + +- Leave every `hooks/*.sh` bare-`python3` call site unchanged (do + nothing). +- Switch every `hooks/*.sh` companion-script invocation to `uv run + --frozen python3` unconditionally. +- Resolve the interpreter through a `python3_cmd` array: prefer `uv run + --frozen [--directory "$plugin_root"] python3` when `command -v uv` + succeeds AND the checkout actually owns `pyproject.toml`/`uv.lock`, + falling back to bare `python3` otherwise. + +## Decision Outcome + +We will resolve each `hooks/*.sh` companion-Python-script invocation +through the third option: a `command -v uv`-and-lockfile-gated +`python3_cmd` array that prefers `uv run --frozen python3` when this +checkout is a uv-managed dev checkout, and falls back to the pre-existing +bare `python3` otherwise -- because it closes the PATH-dependent +false-deny issue #1697 and #1581 both describe, in exactly the dev +checkout where it can occur, without changing behavior at all for a +consumer plugin install that has no `uv` toolchain to invoke. + +The one exception is `hooks/check-pr-skill-audit-disclosure.sh`'s own +tier-1 block, which only ever runs when `.github/scripts/` is present +(i.e., only in this repository's own dev checkout, never a consumer +install) -- there, the invocation uses `uv run --frozen python3` +unconditionally, since the gating condition the other nine call sites +need is already guaranteed by that block's own existing +`.github/scripts/`-presence check. + +As a durable enforcement mechanism for this decision, we also promoted +`.github/scripts/gitapex_gate_bare_python3_invocation.py`'s own +`hooks/*.sh` shell-variable-indirected scan from WARNING-only (report +only, CI never failed) to HARD-FAIL: a `hooks/*.sh` bare `python3 +"$var"` invocation of a `.github/scripts/*.py` target, or of a `hooks/*.py` +target registered in `.gitapex/ssot.json` under a gate whose own +`preconditions.requires_python_packages` is non-empty, now fails CI and +local-preflight. + +## Consequences + +Good, because the exact PATH-dependent false-deny issue #1697 reported +no longer reproduces in a uv-managed dev checkout, while a consumer +plugin install's own behavior (bare `python3`, unchanged) is completely +unaffected. + +Good, because the promoted hard-fail gate makes this decision durable +going forward: a future `hooks/*.sh` call site that reintroduces a bare +`python3 "$var"` of a third-party-dependent target now fails CI, rather +than silently reintroducing this defect class the way the original +regression (#1697, itself a regression from #1566/PR #1675) went +undetected. + +Bad, because the `command -v uv` + lockfile-gated `python3_cmd`-array +resolution snippet (~5 lines) is duplicated verbatim across all ten +`hooks/*.sh` call sites (nine files, one of them -- `check-bash-safety.sh` +-- with two call sites), with no automated check that the ten copies stay +in sync. `hooks/` is entirely on the deployed side of the plugin- +redistribution boundary per `docs/repository-layout.md`, so consolidating +this into one sourced helper is not blocked by that boundary; it simply +has not been done yet. + +Bad, because the hard-fail gate's own `load_python_dependent_hook_script_names` +helper (which reads `.gitapex/ssot.json` to learn which `hooks/*.py` +targets carry a third-party-package precondition) fails *open* -- +returns an empty result rather than a hard failure -- when an individual +`gates` entry's own `preconditions` field is present but malformed +(e.g., a string instead of a mapping), even though the same entry's +`script` list does name a `hooks/*.py` target. This is narrower than, but +the same class as, the whole-file-unreadable case the gate does already +treat as a hard failure; an independent review of this branch confirmed +it live (a well-formed `ssot.json` with one corrupted `preconditions` +field, alongside a real bare invocation of the registered target, +produced a false "clean" exit 0). Not yet fixed as of this ADR's writing. + +Unknown, pending a follow-up fix: whether the per-gate fail-open gap +above gets closed by hard-failing on any malformed `gates` entry, or by +some narrower per-field rule -- this ADR records the uv-preference +*decision itself*, not that follow-up's own resolution. + +## Confirmation + +`.github/scripts/gitapex_gate_bare_python3_invocation.py`'s HARD-FAIL +`hooks/*.sh` shell-variable-indirected scan, run in CI and as part of +local-preflight: a new bare `python3 "$var"` invocation of a +`.github/scripts/*.py` target, or of a registered third-party-dependent +`hooks/*.py` target, fails the check. From f6bed2776689802f4798da9e2d9f674e6c73023a Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 3 Sep 2026 03:21:13 +0000 Subject: [PATCH 8/9] fix(gates): fail closed on a single malformed gate entry too Second independent review of this branch found a narrower mirror of the already-fixed whole-file fail-open bug: load_python_dependent_hook_script_names returned an empty frozenset (a legitimate-looking "no additional targets") rather than failing closed when a single `gates` entry DID name a `hooks/*.py` script but its own `preconditions` or `requires_python_packages` field was present and malformed (e.g. a string instead of a mapping/list) -- live-reproduced: a well-formed ssot.json with one such corrupted entry, alongside a real bare invocation of the registered target, produced a false "clean" exit 0. Fixed by checking each gate's own `script` list for a hooks/*.py target first; only once one is present does a malformed `preconditions`/ `requires_python_packages` shape return None (fail closed) instead of being skipped. A gate that legitimately omits preconditions, or that names no hooks/*.py script at all, is unaffected and still contributes nothing, as before. Refs #1697 Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01BGHXhvkGRDFxGBARLbmh81 --- .../gitapex_gate_bare_python3_invocation.py | 44 +++++++++++++------ ...st_gitapex_gate_bare_python3_invocation.py | 44 ++++++++++++++++--- 2 files changed, 68 insertions(+), 20 deletions(-) diff --git a/.github/scripts/gitapex_gate_bare_python3_invocation.py b/.github/scripts/gitapex_gate_bare_python3_invocation.py index 4055604d..0d48eedd 100644 --- a/.github/scripts/gitapex_gate_bare_python3_invocation.py +++ b/.github/scripts/gitapex_gate_bare_python3_invocation.py @@ -296,13 +296,21 @@ def load_python_dependent_hook_script_names(ssot_path: pathlib.Path = SSOT_PATH) Returns `None` (never an empty result masquerading as "nothing registered") when the registry is missing, unreadable, or does not - parse to the expected shape: the caller must treat that the same - fail-closed way `find_bare_invocations`/`_scan_workflow` already treat - an unreadable workflow file -- a "cannot verify" finding, not a - silent "no additional targets" pass (adversarial-review finding, - issue #1697: a malformed `.gitapex/ssot.json` alongside a real bare - invocation of a registered `hooks/*.py` target previously made this - whole gate report "No ... bare invocations found" and exit 0).""" + parse to the expected shape -- whole-file, or scoped to one `gates` + entry that does name a `hooks/*.py` script but whose own + `preconditions`/`requires_python_packages` shape is malformed: the + caller must treat either the same fail-closed way + `find_bare_invocations`/`_scan_workflow` already treat an unreadable + workflow file -- a "cannot verify" finding, not a silent "no + additional targets" pass (adversarial-review finding, issue #1697: a + malformed `.gitapex/ssot.json`, or a single gate entry's own malformed + `preconditions`, alongside a real bare invocation of a registered + `hooks/*.py` target previously made this whole gate report "No ... + bare invocations found" and exit 0 either way). A gate entry that + names no `hooks/*.py` script at all, or that genuinely omits + `preconditions`/`requires_python_packages` or leaves the latter + empty, is not malformed -- it legitimately contributes nothing and is + skipped, not fail-closed.""" try: data = json.loads(ssot_path.read_text(encoding="utf-8")) except (OSError, UnicodeDecodeError, json.JSONDecodeError): @@ -317,16 +325,24 @@ def load_python_dependent_hook_script_names(ssot_path: pathlib.Path = SSOT_PATH) for gate in gates: if not isinstance(gate, dict): continue - preconditions = gate.get("preconditions") - packages = preconditions.get("requires_python_packages") if isinstance(preconditions, dict) else None - if not isinstance(packages, list) or not packages: - continue scripts = gate.get("script") if not isinstance(scripts, list): continue - for script in scripts: - if isinstance(script, str) and script.startswith("hooks/") and script.endswith(".py"): - names.add(pathlib.PurePosixPath(script).name) + hooks_py_scripts = [s for s in scripts if isinstance(s, str) and s.startswith("hooks/") and s.endswith(".py")] + if not hooks_py_scripts: + continue + preconditions = gate.get("preconditions") + if preconditions is None: + continue + if not isinstance(preconditions, dict): + return None + packages = preconditions.get("requires_python_packages") + if packages is None or packages == []: + continue + if not isinstance(packages, list): + return None + for script in hooks_py_scripts: + names.add(pathlib.PurePosixPath(script).name) return frozenset(names) diff --git a/tests/test_gitapex_gate_bare_python3_invocation.py b/tests/test_gitapex_gate_bare_python3_invocation.py index 6c7982aa..ce857663 100644 --- a/tests/test_gitapex_gate_bare_python3_invocation.py +++ b/tests/test_gitapex_gate_bare_python3_invocation.py @@ -796,23 +796,55 @@ def test_load_python_dependent_hook_script_names_non_mapping_top_level_returns_n assert gate.load_python_dependent_hook_script_names(weird) is None -def test_load_python_dependent_hook_script_names_malformed_gate_entries_are_skipped(tmp_path: pathlib.Path) -> None: +def test_load_python_dependent_hook_script_names_malformed_gate_entries_without_hooks_py_are_skipped( + tmp_path: pathlib.Path, +) -> None: """Defeat case: a `gates` array containing shapes this loader must not - crash on -- a non-mapping entry, a non-list `script`, a non-mapping - `preconditions`, and a non-list `requires_python_packages` -- none of - which should raise or contribute a name.""" + crash on -- a non-mapping entry and a non-list `script` -- neither of + which names a `hooks/*.py` target, so neither is "cannot verify": + there is nothing here this scan needs to widen its scope with.""" ssot_path = _write_ssot( tmp_path, [ "not-a-mapping", # type: ignore[list-item] {"id": "a", "script": "not-a-list", "preconditions": {"requires_python_packages": ["x"]}}, - {"id": "b", "script": ["hooks/foo.py"], "preconditions": "not-a-mapping"}, - {"id": "c", "script": ["hooks/bar.py"], "preconditions": {"requires_python_packages": "not-a-list"}}, ], ) assert gate.load_python_dependent_hook_script_names(ssot_path) == frozenset() +def test_load_python_dependent_hook_script_names_hooks_py_gate_with_malformed_preconditions_returns_none( + tmp_path: pathlib.Path, +) -> None: + """Regression, adversarial-review finding (issue #1697): a `gates` + entry that DOES name a `hooks/*.py` script but whose own + `preconditions` is present and malformed (not a mapping) must fail + closed (`None`), not silently skip -- this is the exact per-gate, + narrower-trigger mirror of the whole-file fail-open bug this loader + was already fixed for once. Skipping this case previously let a + single corrupted `preconditions` field mask a real bare invocation + of the registered hooks/*.py target while still reporting a false + "clean" exit 0.""" + ssot_path = _write_ssot( + tmp_path, + [{"id": "b", "script": ["hooks/foo.py"], "preconditions": "not-a-mapping"}], + ) + assert gate.load_python_dependent_hook_script_names(ssot_path) is None + + +def test_load_python_dependent_hook_script_names_hooks_py_gate_with_malformed_requires_packages_returns_none( + tmp_path: pathlib.Path, +) -> None: + """Same regression as above, one field narrower: `preconditions` is a + mapping, but its own `requires_python_packages` is present and + malformed (not a list) -- also fails closed (`None`), not skipped.""" + ssot_path = _write_ssot( + tmp_path, + [{"id": "c", "script": ["hooks/bar.py"], "preconditions": {"requires_python_packages": "not-a-list"}}], + ) + assert gate.load_python_dependent_hook_script_names(ssot_path) is None + + # --- HARD-FAIL promotion of hooks/*.py targets (issue #1697) --- From 99ed17d268bc58cacffb9d083e4568720c87d714 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 3 Sep 2026 03:31:34 +0000 Subject: [PATCH 9/9] docs(adr): correct factual errors found by independent review A second independent review of PR #1701 (dispatched to re-verify the per-gate fail-open fix, commit f6bed277) also checked ADR 0003 for factual accuracy against the actual code/docs and found three errors: 1. "Not yet fixed as of this ADR's writing" / "Unknown, pending a follow-up fix" (the per-gate fail-open Consequence) was already stale -- f6bed277, committed on this same branch shortly after the ADR was first drafted, had already fixed it. Updated to state the fix and cite the commit; the reviewer live re-verified the fix holds. 2. Decision Outcome said "the other nine call sites" where Consequences correctly said "ten call sites (nine files, check-bash-safety.sh twice)" -- an internal inconsistency. Independently re-counted via `grep -c 'python3_cmd\[@\]' hooks/*.sh`: 10 occurrences across 9 files, confirming "ten" is correct. Decision Outcome corrected to match. 3. The ADR stated hooks/ deployment to a consumer plugin install as current fact ("only skills/ and hooks/ are ever deployed... hooks/ is entirely on the deployed side"), but docs/repository-layout.md (the doc it cites) describes hooks/ deployment as future/planned ("and, in the future, hooks from hooks/"; "and, later, hooks"), not yet shipped. Corrected Context and the duplication Consequence to describe hooks/ deployment as planned rather than current, and Context's own risk framing to "as soon as hooks/ deployment ships" rather than an already-present break. No change to the Decision Outcome (uv-preferred python3 resolution with a bare-python3 fallback) or Status -- these are factual corrections to an already-Accepted record, not a re-litigation of the decision itself. Refs #1697 Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01BGHXhvkGRDFxGBARLbmh81 --- ...er-uv-resolved-python3-in-hooks-scripts.md | 55 ++++++++++--------- 1 file changed, 28 insertions(+), 27 deletions(-) diff --git a/docs/adr/0003-prefer-uv-resolved-python3-in-hooks-scripts.md b/docs/adr/0003-prefer-uv-resolved-python3-in-hooks-scripts.md index 20c05d67..3b029018 100644 --- a/docs/adr/0003-prefer-uv-resolved-python3-in-hooks-scripts.md +++ b/docs/adr/0003-prefer-uv-resolved-python3-in-hooks-scripts.md @@ -28,11 +28,13 @@ invocation time resolved a different `python3` that could not see that class against a different call site. A conflicting constraint narrows the fix: per `docs/repository-layout.md`, -only `skills/` and `hooks/` are ever deployed to a *consumer* plugin -install of this repository, and a consumer install carries no `uv` -toolchain or lockfile of its own. An unconditional switch to `uv run` in -every `hooks/*.sh` file would resolve this repository's own dev-checkout -bug but break every consumer install outright. +only `skills/` is currently deployed to a *consumer* plugin install of +this repository, with `hooks/` deployment stated there as planned for a +future release -- and a consumer install carries no `uv` toolchain or +lockfile of its own. An unconditional switch to `uv run` in every +`hooks/*.sh` file would resolve this repository's own dev-checkout bug +today, but would break every consumer install outright as soon as +`hooks/` deployment ships. ## Considered Options @@ -60,9 +62,9 @@ The one exception is `hooks/check-pr-skill-audit-disclosure.sh`'s own tier-1 block, which only ever runs when `.github/scripts/` is present (i.e., only in this repository's own dev checkout, never a consumer install) -- there, the invocation uses `uv run --frozen python3` -unconditionally, since the gating condition the other nine call sites -need is already guaranteed by that block's own existing -`.github/scripts/`-presence check. +unconditionally, since the gating condition the other nine files' ten +call sites (`check-bash-safety.sh` has two) need is already guaranteed +by that block's own existing `.github/scripts/`-presence check. As a durable enforcement mechanism for this decision, we also promoted `.github/scripts/gitapex_gate_bare_python3_invocation.py`'s own @@ -91,28 +93,27 @@ Bad, because the `command -v uv` + lockfile-gated `python3_cmd`-array resolution snippet (~5 lines) is duplicated verbatim across all ten `hooks/*.sh` call sites (nine files, one of them -- `check-bash-safety.sh` -- with two call sites), with no automated check that the ten copies stay -in sync. `hooks/` is entirely on the deployed side of the plugin- -redistribution boundary per `docs/repository-layout.md`, so consolidating -this into one sourced helper is not blocked by that boundary; it simply -has not been done yet. +in sync. `hooks/` is planned to join `skills/` on the deployed side of +the plugin-redistribution boundary per `docs/repository-layout.md`, so +consolidating this into one sourced helper is not blocked by that +boundary; it simply has not been done yet. Bad, because the hard-fail gate's own `load_python_dependent_hook_script_names` helper (which reads `.gitapex/ssot.json` to learn which `hooks/*.py` -targets carry a third-party-package precondition) fails *open* -- -returns an empty result rather than a hard failure -- when an individual -`gates` entry's own `preconditions` field is present but malformed -(e.g., a string instead of a mapping), even though the same entry's -`script` list does name a `hooks/*.py` target. This is narrower than, but -the same class as, the whole-file-unreadable case the gate does already -treat as a hard failure; an independent review of this branch confirmed -it live (a well-formed `ssot.json` with one corrupted `preconditions` -field, alongside a real bare invocation of the registered target, -produced a false "clean" exit 0). Not yet fixed as of this ADR's writing. - -Unknown, pending a follow-up fix: whether the per-gate fail-open gap -above gets closed by hard-failing on any malformed `gates` entry, or by -some narrower per-field rule -- this ADR records the uv-preference -*decision itself*, not that follow-up's own resolution. +targets carry a third-party-package precondition) initially failed +*open* -- returned an empty result rather than a hard failure -- when an +individual `gates` entry's own `preconditions` field was present but +malformed (e.g., a string instead of a mapping), even though the same +entry's `script` list did name a `hooks/*.py` target. This was narrower +than, but the same class as, the whole-file-unreadable case the gate +already treated as a hard failure; an independent review of this branch +confirmed it live (a well-formed `ssot.json` with one corrupted +`preconditions` field, alongside a real bare invocation of the +registered target, produced a false "clean" exit 0). Fixed in a +follow-up commit (`f6bed277`) on this same branch, shortly after this +ADR was first drafted, by extending the same fail-closed treatment to a +malformed per-gate shape, not only a malformed whole-file one -- live +re-verified by a second independent review pass after the fix. ## Confirmation