diff --git a/.claude/hooks/session-start.sh b/.claude/hooks/session-start.sh index 29b0f752..73e6caa2 100755 --- a/.claude/hooks/session-start.sh +++ b/.claude/hooks/session-start.sh @@ -100,7 +100,7 @@ fi if [ ! -f "${CLAUDE_PROJECT_DIR:-.}/apm.yml" ]; then echo "gitapex: ${CLAUDE_PROJECT_DIR:-.}/apm.yml not found; skipping prek install (not a gitapex checkout)." >&2 elif command -v uv >/dev/null 2>&1; then - uv run --directory "${CLAUDE_PROJECT_DIR:-.}" prek -q install --allow-missing-config -t pre-commit -t pre-push \ + uv run --directory "${CLAUDE_PROJECT_DIR:-.}" prek -q install --allow-missing-config -t pre-commit -t pre-push -t commit-msg \ || echo "gitapex: prek install reported a failure; the local pre-commit hook may not be active this session." >&2 else echo "gitapex: uv not found; cannot install the local pre-commit hook this session." >&2 diff --git a/.gitapex/ssot.json b/.gitapex/ssot.json index 2acb6d50..42c1c82b 100644 --- a/.gitapex/ssot.json +++ b/.gitapex/ssot.json @@ -1707,6 +1707,34 @@ {"kind": "github-native", "ref": "required_status_checks context \"betterleaks\" (repository ruleset on the default branch)"} ] }, + { + "id": "commit-citation-gate", + "kind": "script", + "script": [".github/scripts/gitapex_gate_commit_citation.py", ".github/workflows/commit-citation-gate.yml"], + "rule": "CLAUDE.md section 3's issue-citation rule for commits: passes when a citation (Closes/Fixes/Refs #N, or a bare #N) is found in at least one non-merge commit in the PR's own range (git log --no-merges base..head, so an uncited merge commit in that range never fails this on its own) OR in the PR's own title/body; fails when found in neither. The commit-msg pre-commit hook (.pre-commit-config.yaml's commit-citation entry, sharing this same script's --mode commit-msg) is the bypassable local first pass; this CI check is the actual no-exceptions backstop and carries no separate registry entry of its own, matching this registry's existing convention for pre-commit-stage-only hooks (e.g. ruff-check, skill-shape-check).", + "planes": ["ci", "local"], + "local_invocation": [ + "uv", + "run", + "--frozen", + "python3", + ".github/scripts/gitapex_gate_commit_citation.py", + "--mode", + "pr-range" + ], + "trigger": ".github/workflows/commit-citation-gate.yml on pull_request:[opened, edited, synchronize, reopened] (no paths filter, so a required check can never be left Pending)", + "policy_refs": [], + "cluster": "plan-integrity", + "tracking_issue": 1212, + "status": "active", + "supersedes": null, + "bypass_review_status": "not-yet-reviewed", + "target": [ + {"kind": "workflow-event", "ref": "commit-citation-gate.yml:pull_request"}, + {"kind": "runtime-resolved-reference", "ref": "git log --no-merges .."}, + {"kind": "runtime-resolved-reference", "ref": "github.event.pull_request.title / .body"} + ] + }, { "id": "betterleaks-allowlist-no-removal", "kind": "script", diff --git a/.github/scripts/_gitapex_base_ref.py b/.github/scripts/_gitapex_base_ref.py index 5dd4656f..9d1c0b97 100644 --- a/.github/scripts/_gitapex_base_ref.py +++ b/.github/scripts/_gitapex_base_ref.py @@ -136,7 +136,25 @@ def destination_refspec(remote: str, branch: str) -> str: def run_git( - root: pathlib.Path, args: list[str], *, label: str, timeout: int, error_cls: type[Exception] + root: pathlib.Path, + args: list[str], + *, + label: str, + timeout: int, + error_cls: type[Exception], + stdin_text: str | None = None, + # function-body-test-coverage: WAIVED: the added `stdin_text` parameter + # (issue #1212) is exercised by the pre-existing, extensively-updated + # tests/test_gitapex_base_ref.py (test_run_git_* mentions `run_git` by + # name repeatedly) -- but gitapex_gate_function_body_test_coverage.py's + # own _stem() keeps this module's leading underscore + # ("_gitapex_base_ref"), so it looks for tests/test__gitapex_base_ref.py + # (double underscore) rather than this repository's own actual, + # established single-underscore convention for a `_`-prefixed private + # helper module's test file. A genuine gate limitation, not a real + # coverage gap -- disclosed here rather than worked around by adding a + # second, oddly-named test file just to match the gate's own stem + # computation. ) -> subprocess.CompletedProcess[str]: """Run ``git -C root `` and return the completed process, regardless of its exit code -- callers decide what a nonzero @@ -149,6 +167,15 @@ def run_git( this function replaces there -- existing tests asserting on that text keep passing unmodified. + ``stdin_text`` feeds a git subcommand that reads its input from stdin + (``git stripspace``, this module's own third caller + ``gitapex_gate_commit_citation.py``) -- added here rather than as a + second, near-identical ``subprocess.run`` wrapper in that caller, + which is precisely the duplicate-then-drift this module exists to + prevent. Default ``None`` leaves ``subprocess.run``'s own stdin + handling exactly as it was for every pre-existing caller: no pipe is + opened and no behavior changes. + ``errors="replace"`` rather than ``text=True``'s own strict default, matching ``gitapex_gate_behind_base.py``'s documented regression: a byte sequence on stdout/stderr that is not valid UTF-8 must not raise @@ -168,6 +195,7 @@ def run_git( errors="replace", check=False, timeout=timeout, + input=stdin_text, ) except subprocess.TimeoutExpired as error: raise error_cls(f"git {label} timed out after {timeout}s") from error diff --git a/.github/scripts/gitapex_gate_commit_citation.py b/.github/scripts/gitapex_gate_commit_citation.py new file mode 100644 index 00000000..ac1d7ffe --- /dev/null +++ b/.github/scripts/gitapex_gate_commit_citation.py @@ -0,0 +1,665 @@ +#!/usr/bin/env python3 +"""Two-layer enforcement of CLAUDE.md section 3's issue-citation rule for +commits (issue #1212): a fast local `commit-msg` git hook, and the CI +`pr-range` backstop that is the actual no-exceptions gate. + +Both modes share one detector, `extract_citations` +(`hooks/gitapex_check_pr_issue_acm_disclosure.py`), reused rather than +reimplemented -- a citation shown only inside a fenced or inline code +block must not count, and that module's own regex already carries that +behavior (issue #657's own adversarial-review fix); a second copy here +would risk drifting out of sync with it. This file mirrors +`.github/scripts/gitapex_run_betterleaks.py`'s one-script/two-`--mode` +shape: the single script both `.pre-commit-config.yaml`'s `commit-msg` +stage hook and this issue's own CI workflow invoke, at a different +`--mode` each. + +**`--mode commit-msg`** (the local first pass, +`.pre-commit-config.yaml`'s `stages: [commit-msg]` hook): reads the +commit-message file path prek hands it as its sole positional argument -- +live-verified against a real `prek install -t commit-msg` run rather than +assumed (a `language: system` hook run from the repo root receives the +same single, repo-root-relative path git's own commit-msg hook contract +does: `.git/COMMIT_EDITMSG`). Passes when the message alone carries a +citation -- the *cleaned* message, see next paragraph -- or when the +commit being written is a *merge* commit (`merge_in_progress`), which +`--mode pr-range` already exempts through `git log --no-merges` and which +this layer would otherwise reject on every `git merge`. + +**Why `--mode commit-msg` must clean the file first (and `--mode +pr-range` must not).** A `commit-msg` hook runs *before* git applies its +own `--cleanup` pass, so the file git hands the hook is still raw: it +carries git's whole comment block (`core.commentChar`, `#` by default) +and, under `commit.verbose=true`/`git commit -v`, the entire staged diff +below the `# ------------------------ >8 ------------------------` +scissors line. Live-reproduced, not assumed (issue #1212's own +adversarial review): staging a file whose *content* contains a +citation-shaped line, then committing the uncited subject `chore: tidy up +formatting` through an editor, produced a raw hook file whose diff +section read `+ # See issue #1212 for the rationale.` -- and this gate +returned PASS for a commit whose actually-stored `%B` was +`chore: tidy up formatting`, citing nothing. `clean_commit_message` +closes that false-PASS by applying git's own two cleanup steps in git's +own order before any citation check runs. `--mode pr-range` deliberately +skips this: `git log --format=%B` already returns post-cleanup messages, +so cleaning there would be a redundant subprocess per commit. + +**`--mode pr-range`** (the CI backstop, no-exceptions per this issue): +passes when a citation is found in the PR's own title/body OR in at +least one *non-merge* commit in the PR's range; an uncited merge commit +in that same range never fails this check on its own -- `git log +--no-merges` excludes it from the scan entirely, before any citation +check ever runs against it. + +**Base-ref resolution, local vs. CI.** `.gitapex/ssot.json`'s own +`local_invocation` runs this mode with no `--base-ref` at all, since a +`local-preflight` run has no PR yet: this self-heals `origin/main` the +same way `gitapex_run_base_diff.py`'s own `ensure_base_ref` does -- +`_gitapex_base_ref`'s peeled-probe-then-destination-refspec-fetch, only +when the probe first finds nothing -- rather than +`gitapex_gate_behind_base.py`'s unconditional-fetch-every-run posture: +this gate does not need `origin/main` to be perfectly fresh (a stale +local ref only shifts which already-cited commits fall inside the +scanned range, not whether the *current* branch's own commits/title/body +carry a citation), so the cheaper probe-first form is the right +trade-off here, not a corner cut. The CI workflow instead passes +`--base-ref` explicitly -- the real `git merge-base "$BASE_SHA" +"$HEAD_SHA"` the workflow itself computes, mirroring +`exception-handler-gap-gate.yml`'s own established +merge-base-not-base.sha pattern (never `base.sha` directly, which can go +stale relative to a `main` that advanced after the PR opened) -- so this +module's own fetch/self-heal path never runs in CI: the workflow's own +`harden-checkout` step already fetched enough history for the merge-base +it computed to resolve. + +**PR title/body, passed as files, not stdin.** `--title`/`--body` each +optionally name a file (mirroring `gitapex_gate_acm_issue_disclosure.py`'s +own `--body PATH` and `gitapex_gate_provenance_disclosure.py`'s own +`--body`/`--diff-added` file-path convention) rather than one JSON +payload or stdin -- this mode needs *two* independent pieces of untrusted +text at once, and stdin can only carry one. The calling workflow follows +`provenance-disclosure-gate.yml`'s exact pattern: write each of +`github.event.pull_request.title`/`.body` to its own file via `env:` +indirection first (`printf '%s' "$PR_TITLE" > ...`), never interpolating +either directly into a shell command line. Both are omitted in +`local_invocation` (no PR exists yet locally) and default to empty text +-- the commit-range scan alone still runs. + +**"Nothing to check" is not a policy FAIL.** `.gitapex/ssot.json`'s own +`local_invocation` runs `--mode pr-range` with neither `--title` nor +`--body`, and that invocation feeds the `pre-push` hook's +`local-preflight` runner, which reads *any* non-zero exit as a blocked +push. On a checkout where `refs/remotes/origin/main..HEAD` is empty -- +right after a fast-forward, or a branch whose own commits are all already +merged into `main` -- there is no commit and no PR text to evaluate at +all, so there is no citation obligation to violate; reporting that state +as "you cited nothing" (live-reproduced as exit 1, issue #1212's own +adversarial review) blocks a push for a state that cannot cite anything. +`evaluate_pr_range`'s own `pr_text_supplied` flag separates the two: an +empty commit range *and* neither `--title` nor `--body` supplied passes +with an explicit "nothing to check" message, while every other shape +keeps its previous verdict byte for byte. The flag tracks whether the +*flags were passed*, never whether their text is non-empty, so CI -- which +always passes both -- is unreachable from this path and its behavior is +unchanged even for an empty range; the parameter's own default is the +strict `True`, so a caller that forgets it fails closed. + +Exit codes (both modes): 0 pass, 1 no citation found (a clear FAIL on +stderr), 2 the check itself could not be trusted -- invalid CLI +arguments, an unreadable input file, a `git stripspace` call that could +not run or failed (`commit-msg` only -- never a silent fallback to +uncleaned text), or (`pr-range` only) a base ref that could not be +resolved/fetched, shares no common ancestor with HEAD, or a git call that +could not run at all (no `git` on PATH, a hang past +`GIT_TIMEOUT_SECONDS`). +Mirrors `gitapex_gate_behind_base.py`'s/`gitapex_run_base_diff.py`'s own +0/1/2 convention, distinct from a confirmed policy FAIL. + +Non-goals (named explicitly, not silently out of scope): no retroactive +citation of existing merged commit history, and no citation *format* +validation (issue #521's separate scope) -- this gate only asks whether +*any* citation form (`Closes #N`, `Fixes #N`, `Refs #N`, or a bare `#N`) +is present, never which one or whether it resolves. + +Usage:: + + # commit-msg (installed by .pre-commit-config.yaml; the path below is + # what prek itself hands the hook, not typed by a contributor): + uv run --frozen python3 .github/scripts/gitapex_gate_commit_citation.py \\ + --mode commit-msg .git/COMMIT_EDITMSG + + # pr-range (CI): base-ref is the workflow's own precomputed merge-base + uv run --frozen python3 .github/scripts/gitapex_gate_commit_citation.py \\ + --mode pr-range --owner tvna --repo gitapex \\ + --base-ref "$merge_base" --head-ref "$HEAD_SHA" \\ + --title "$RUNNER_TEMP/pr_title.txt" --body "$RUNNER_TEMP/pr_body.txt" + + # pr-range (local preflight; no PR yet -- commit range only): + uv run --frozen python3 .github/scripts/gitapex_gate_commit_citation.py --mode pr-range +""" + +from __future__ import annotations + +import argparse +import pathlib +import sys +from typing import Literal, cast + +import _gitapex_base_ref +from pydantic import BaseModel, ValidationError, field_validator, model_validator + +# hooks/ is a sibling of .github/ at the repo root, never on sys.path by +# default for a standalone `uv run --frozen python3` invocation of this file +# (Python only auto-adds this script's own directory). Mirrors the +# exact `sys.path.insert(0, str(pathlib.Path(__file__).resolve().parent))` +# bootstrap style every other cross-file .github/scripts/*.py import in this +# repository already uses (e.g. gitapex_gate_ruleset_required_checks.py), just +# pointed at hooks/ instead of this file's own directory. Under pytest this is +# a harmless no-op prepend: pyproject.toml's own `pythonpath` already lists +# both ".github/scripts" and "hooks". +# +# Spelled out here rather than reusing REPO_ROOT below: ruff's own E402 +# tolerates a sys.path mutation before this deferred import, but not a +# preceding assignment, so hoisting REPO_ROOT above this line to share the +# expression fails `ruff check` outright (verified, not assumed). +sys.path.insert(0, str(pathlib.Path(__file__).resolve().parents[2] / "hooks")) + +from gitapex_check_pr_issue_acm_disclosure import extract_citations # sys.path bootstrap above must run first + +REPO_ROOT = pathlib.Path(__file__).resolve().parents[2] + +# Hardcoded per the same posture gitapex_gate_behind_base.py's own +# BASE_REMOTE/BASE_BRANCH and gitapex_run_base_diff.py's own identical +# constants document (issue #985) -- this repository has exactly one base +# branch today; see either module's own docstring for the named residual +# risks that posture carries. Only reached by resolve_base_ref's own local +# self-heal path below -- CI always passes --base-ref explicitly. +BASE_REMOTE = "origin" +BASE_BRANCH = "main" + +# Ceiling for one git subprocess call this module makes (a probe, a fetch, a +# merge-base check, or the real `git log`) -- matches +# _gitapex_base_ref.GIT_TIMEOUT_SECONDS exactly rather than redefining 60 as +# a second literal, the same convention gitapex_gate_behind_base.py and +# gitapex_run_base_diff.py both already follow. +GIT_TIMEOUT_SECONDS = _gitapex_base_ref.GIT_TIMEOUT_SECONDS + +# The body of git's own "scissors" cut line, verbatim from git-commit(1)'s +# own `--cleanup=scissors` documentation ("everything from (and including) +# the line found below is truncated"). Only the *prefix* of that line varies +# -- git writes ` `, so a repository configuring +# `core.commentChar = ;` gets `; ------------------------ >8 ---...` instead +# (live-verified, both forms). Matched as a substring rather than anchored to +# a known comment prefix precisely so no comment-character guess is needed: +# `core.commentChar = auto` makes `git config --get core.commentChar` report +# the literal string "auto" rather than the character git actually chose +# (live-verified), so a prefix-anchored match would silently stop truncating +# there. The looser match can only ever truncate *earlier* than git would, +# which drops text from the scanned message and can therefore only turn a +# PASS into a FAIL -- fail-closed, never a new false-PASS. +SCISSORS_MARKER = "------------------------ >8 ------------------------" + + +class CitationGateError(Exception): + """The check could not be trusted -- exit 2, never a silent pass and + never conflated with a genuine no-citation FAIL (exit 1).""" + + +def truncate_at_scissors(text: str) -> str: + """`text` up to (never including) git's own scissors line -- the first + line containing :data:`SCISSORS_MARKER` -- or `text` unchanged when + there is none (the ordinary, non-`commit.verbose` commit). + + Step one of two, and it must run *before* the comment strip, not + after: the scissors line is itself a comment line, so stripping + comments first would delete the only marker separating the message + from the verbatim staged diff below it and leave that whole diff in + the scanned text -- exactly the false-PASS this function exists to + close. `git stripspace --strip-comments` has no scissors handling of + its own at all (live-verified: it removes the scissors line and leaves + the diff), so this step cannot be delegated to git the way the comment + strip below is.""" + kept: list[str] = [] + for line in text.splitlines(keepends=True): + if SCISSORS_MARKER in line: + break + kept.append(line) + return "".join(kept) + + +def clean_commit_message(root: pathlib.Path, text: str) -> str: + """One raw `commit-msg`-hook file's text, reduced to what git will + actually store as the commit message: scissors block truncated + (:func:`truncate_at_scissors`), then comment lines removed. + + The comment strip is delegated to `git stripspace --strip-comments` + rather than hand-rolled: that is git's *own* implementation of this + exact step, so it resolves `core.commentChar` (and, on git >= 2.45, + `core.commentString`) itself, from the same config the commit being + checked will be cleaned with. Live-verified against a repository + configuring `core.commentChar = ;`, where a hardcoded `#` strip would + have removed nothing at all. It also runs fine outside a git + repository (verified), so a checkout in an odd state degrades to + default-`#` behavior rather than an error. + + Raises :class:`CitationGateError` -- exit 2, "the check could not be + trusted" -- when `git stripspace` cannot run (no `git` on PATH, a hang + past `GIT_TIMEOUT_SECONDS`) or exits non-zero. Deliberately never a + silent fallback to the unstripped text: that fallback would restore + the precise false-PASS this function exists to close, and would do it + invisibly, on exactly the broken-environment path where nobody is + looking.""" + truncated = truncate_at_scissors(text) + result = _gitapex_base_ref.run_git( + root, + ["stripspace", "--strip-comments"], + label="strip comments from the commit message", + timeout=GIT_TIMEOUT_SECONDS, + error_cls=CitationGateError, + stdin_text=truncated, + ) + if result.returncode != 0: + raise CitationGateError(f"git stripspace --strip-comments failed: {result.stderr.strip()}") + return result.stdout + + +def merge_in_progress(root: pathlib.Path) -> bool: + """Whether git is part-way through creating a *merge* commit right + now -- `git rev-parse --verify --quiet MERGE_HEAD`, which resolves + exactly while `.git/MERGE_HEAD` exists and not otherwise. + + `--mode commit-msg` needs this to reach the same verdict `--mode + pr-range` already reaches through `git log --no-merges`: this issue's + own stated non-goal is that an uncited merge commit never fails this + gate. Without it the two layers actively disagree -- live-reproduced, + not theorized: with the hook installed, an ordinary `git merge --no-ff` + whose default message is `Merge branch 'side'` was rejected outright + ("Not committing merge; use 'git commit' to complete the merge"), + which would break this repository's own documented `git pull + --no-rebase` shared-branch workflow (CLAUDE.md section 3) on every + merge -- and `.claude/hooks/session-start.sh` now installs this hook + automatically for every session, so nobody has to opt in to hit it. + + `MERGE_HEAD`, never the message file's own basename: git-merge hands + the hook `.git/MERGE_MSG`, but a `git commit` *completing* an already- + started merge hands it `.git/COMMIT_EDITMSG` with `MERGE_HEAD` still + set -- both are the same merge commit and both must be exempt (both + verified live). `git merge --squash` deliberately stays gated: it sets + `SQUASH_MSG`, never `MERGE_HEAD`, and produces an ordinary + single-parent commit that `git log --no-merges` would scan in CI too, + so exempting it would be a real divergence rather than a matching one. + `git revert` and `git cherry-pick` need no handling here at all -- + verified live that neither invokes this hook.""" + result = _gitapex_base_ref.run_git( + root, + ["rev-parse", "--verify", "--quiet", "MERGE_HEAD"], + label="check whether a merge is in progress", + timeout=GIT_TIMEOUT_SECONDS, + error_cls=CitationGateError, + ) + return result.returncode == 0 + + +def _extract_citations_or_raise( + owner: str | None, repo: str | None, title: str | None, body: str | None +) -> tuple[tuple[int, ...], tuple[int, ...]]: + """`extract_citations`, with one failure mode converted to this + module's own exit-2 contract rather than an uncaught traceback (issue + #1212's own adversarial review, dimension 15 of + `skills/evaluating-deterministic-gate-quality`): Python 3.12's default + integer-string-conversion digit limit (`sys.get_int_max_str_digits`, + 4300) makes `extract_citations`' own `int(n)` calls raise `ValueError` + for a citation whose digit run is implausibly long (a PR title/body or + commit message containing `#` followed by thousands of digits -- live- + reproduced: `int('9' * 5000)` raises). Uncaught, that `ValueError` + exits 1 -- the code this module reserves for a *confirmed* no-citation + policy FAIL -- so a malformed/adversarial input would report itself as + a real citation violation instead of "the check itself could not be + trusted." No other `extract_citations` failure mode is known; this is + not a blanket except-and-hide.""" + try: + return extract_citations(owner, repo, title, body) + except ValueError as error: + raise CitationGateError(f"could not parse a citation number in the input text: {error}") from error + + +def check_commit_message(text: str) -> bool: + """True iff one commit's own message `text` carries any citation -- + resolving or context-only, either counts here: this gate only asks + *whether* an issue is cited, never which form or whether it resolves + (format/ACM validation is out of scope -- issue #521, #657). Always + `owner=None, repo=None`: a lone commit message carries no notion of + "this PR's own target repo" the way a PR title/body does, matching + exactly how `--mode commit-msg` itself calls `extract_citations`. + + Expects an already-*cleaned* message: `git log`'s own `%B` output in + `--mode pr-range`, or :func:`clean_commit_message`'s output in + `--mode commit-msg`. Cleaning stays the caller's job rather than + moving in here, so `--mode pr-range` does not pay a `git stripspace` + subprocess per commit re-cleaning text git already cleaned. + + May raise :class:`CitationGateError` -- see `_extract_citations_or_raise`.""" + resolving, context = _extract_citations_or_raise(None, None, None, text) + return bool(resolving or context) + + +def check_pr_text(owner: str, repo: str, title: str, body: str) -> bool: + """True iff the PR's own `title`/`body` together carry any citation. + `owner`/`repo` (the PR's own target repo) are passed through so + `extract_citations` can normalize a same-repo-qualified + `owner/repo#N` citation down to a bare `#N` -- see its own docstring. + + May raise :class:`CitationGateError` -- see `_extract_citations_or_raise`.""" + resolving, context = _extract_citations_or_raise(owner or None, repo or None, title or None, body or None) + return bool(resolving or context) + + +def resolve_base_ref(root: pathlib.Path, base_ref: str | None) -> str: + """The PR's own base ref for `git log --no-merges ..`. + + CI passes `base_ref` explicitly (its own precomputed merge-base -- see + module docstring), which is returned unchanged with no git call at + all. A local run leaves it unset: this self-heals `refs/remotes/ + origin/main`, mirroring `gitapex_run_base_diff.py`'s own + `ensure_base_ref` -- a cheap peeled probe first, a destination-refspec + fetch only when that probe finds nothing, then a re-probe that never + trusts the fetch's own exit code alone (issue #1345) -- and finally + confirms a common ancestor exists with HEAD (the shallow-clone case; + `git merge-base` prints nothing to stderr on that failure otherwise, + per `_gitapex_base_ref.require_common_ancestor`'s own docstring).""" + if base_ref is not None: + return base_ref + + if not _gitapex_base_ref.peeled_ref_exists( + root, BASE_REMOTE, BASE_BRANCH, timeout=GIT_TIMEOUT_SECONDS, error_cls=CitationGateError + ): + _gitapex_base_ref.fetch_destination_refspec( + root, BASE_REMOTE, BASE_BRANCH, timeout=GIT_TIMEOUT_SECONDS, error_cls=CitationGateError + ) + if not _gitapex_base_ref.peeled_ref_exists( + root, BASE_REMOTE, BASE_BRANCH, timeout=GIT_TIMEOUT_SECONDS, error_cls=CitationGateError + ): + raise CitationGateError( + f"git fetch {BASE_REMOTE} {BASE_BRANCH} reported success but " + f"refs/remotes/{BASE_REMOTE}/{BASE_BRANCH} still does not resolve -- " + "never trusting a fetch's exit code alone (issue #1345)" + ) + + qualified_ref = f"refs/remotes/{BASE_REMOTE}/{BASE_BRANCH}" + _gitapex_base_ref.require_common_ancestor( + root, qualified_ref, timeout=GIT_TIMEOUT_SECONDS, error_cls=CitationGateError + ) + return qualified_ref + + +def commit_range_messages(root: pathlib.Path, base_ref: str, head_ref: str) -> list[str]: + """Every *non-merge* commit's own full message (`%B`) in + `base_ref..head_ref`, NUL-separated (`%x00`) so a message that itself + contains a blank line cannot be mistaken for a commit boundary -- a + real commit message cannot itself carry a NUL byte. `--no-merges` + excludes a real merge commit from this list entirely, before any + citation check ever runs against it -- an uncited merge commit in the + range therefore never fails this gate on its own, this issue's own + stated Non-goal. + + Runs through `_gitapex_base_ref.run_git`, like every other git call + this module makes: that helper already carries this call's exact + capture/`errors="replace"` shape, and turns a subprocess-layer failure + (no `git` on PATH, a hang past `GIT_TIMEOUT_SECONDS`) into a + `CitationGateError` -- the exit-2 "could not be trusted" signal -- + rather than an uncaught exception. + + One entry per commit, *including* a commit whose message is empty + (`git commit --allow-empty-message`), which is why the split drops + only its own final element rather than filtering every empty one out. + `git log --format=%B%x00` emits `%x00` plus git's own + per-commit trailing newline, so N commits always produce exactly N+1 + NUL-separated parts and the last is a separator artifact, never a + commit (verified live against a real repo, including the zero-commit + case, where stdout is empty and the single part yields an empty list). + Filtering on truthiness instead -- the pre-review form -- made two + real `--allow-empty-message` commits indistinguishable from an *empty + range*, which `evaluate_pr_range` now treats as "nothing to check"; + an uncited commit would have passed the gate as though it were not + there at all.""" + result = _gitapex_base_ref.run_git( + root, + ["log", "--no-merges", f"{base_ref}..{head_ref}", "--format=%B%x00"], + label=f"list commits in {base_ref}..{head_ref}", + timeout=GIT_TIMEOUT_SECONDS, + error_cls=CitationGateError, + ) + if result.returncode != 0: + raise CitationGateError(f"git log --no-merges {base_ref}..{head_ref} failed: {result.stderr.strip()}") + return [message.strip() for message in result.stdout.split("\x00")[:-1]] + + +def evaluate_pr_range( + root: pathlib.Path, + owner: str, + repo: str, + title: str, + body: str, + base_ref: str | None, + head_ref: str, + *, + pr_text_supplied: bool = True, +) -> tuple[bool, str]: + """(passed, message) for the full `--mode pr-range` check: a citation + in the PR's own title/body is checked first (no git call at all, and + the common case), so `resolve_base_ref`'s own fetch/self-heal path + only ever runs when the title/body carry nothing -- never on the + already-satisfied common case. + + `pr_text_supplied` is False only for the local `local_invocation` + shape, where neither `--title` nor `--body` was passed at all. It + separates "nothing to check" from "something to check and it failed": + an empty commit range with no PR text supplied carries no citation + obligation to violate (see the module docstring), and reporting it as + a FAIL blocks the `pre-push` local-preflight for a state that cannot + cite anything. Defaults to the strict True so a caller that forgets it + -- or any future call site -- fails closed; CI always passes both + flags and therefore never reaches this branch.""" + if check_pr_text(owner, repo, title, body): + return True, "PR title/body cites an issue" + + resolved_base = resolve_base_ref(root, base_ref) + messages = commit_range_messages(root, resolved_base, head_ref) + for message in messages: + if check_commit_message(message): + return True, f"a non-merge commit in {resolved_base}..{head_ref} cites an issue" + + if not messages and not pr_text_supplied: + return True, ( + f"nothing to check -- no non-merge commit in {resolved_base}..{head_ref}, " + "and no PR title/body was supplied. Nothing here can carry a citation, " + "so this is not a citation-policy failure." + ) + + return False, ( + f"neither the PR title/body nor any non-merge commit in {resolved_base}..{head_ref} " + "cites an issue. Per this repository's own citation convention (CONTRIBUTING.md), " + "cite one via Closes #N, Fixes #N, Refs #N, or a bare #N." + ) + + +class CommitCitationArgs(BaseModel): + """Typed view of `main`'s parsed CLI namespace (issue #1040's + pydantic CLI-arg-validation convention, applied here like every other + `.github/scripts/*.py` gate).""" + + mode: Literal["commit-msg", "pr-range"] + commit_msg_file: str | None + owner: str + repo: str + title: str | None + body: str | None + base_ref: str | None + head_ref: str + root: pathlib.Path + + @field_validator("root") + @classmethod + def _root_must_exist(cls, value: pathlib.Path) -> pathlib.Path: + if not value.is_dir(): + raise ValueError(f"--root must be an existing directory, got {value}") + return value + + @model_validator(mode="after") + def _commit_msg_file_required_in_commit_msg_mode(self) -> CommitCitationArgs: + if self.mode == "commit-msg" and not self.commit_msg_file: + raise ValueError("a commit message file path is required in --mode commit-msg") + return self + + +def _read_input_file(path: str | None, *, label: str = "file") -> str: + """The text of one input file this gate was pointed at. Empty text -- + never an error -- when `path` itself is None: the `--title`/`--body` + flag was omitted, the local `local_invocation` shape, where no PR + title/body exists yet and the commit-range scan alone runs. + + Raises :class:`CitationGateError` -- the module's own exit-2 "the + check itself could not be trusted" signal, which both call sites + already catch -- rather than returning an error alongside the text, so + all three reads share one handler pair instead of three copies. + `label` only names the file in the not-found message ("commit message + file" for `--mode commit-msg`'s own positional argument). + + The broad `OSError` arm is not defensive padding (issue #1212's own + adversarial review, dimension 15 of + `skills/evaluating-deterministic-gate-quality`): before it, only + `FileNotFoundError` and `UnicodeDecodeError` were caught, so pointing + any of the three path flags at a *directory* (`IsADirectoryError`) or + at a file the process cannot read (`PermissionError`) escaped as an + uncaught traceback -- and Python's own exit code for that is 1, which + is exactly the code this module reserves for a *confirmed* no-citation + policy FAIL. A broken invocation therefore reported itself as a real + citation violation. All three were live-reproduced against the CLI, + not inferred from reading the except clauses.""" + if path is None: + return "" + try: + return pathlib.Path(path).read_text(encoding="utf-8") + except FileNotFoundError as error: + raise CitationGateError(f"{label} not found: {path}") from error + except UnicodeDecodeError as error: + raise CitationGateError(f"{path} is not valid UTF-8: {error}") from error + except OSError as error: + raise CitationGateError(f"{label} could not be read: {path}: {error}") from error + + +def _run_commit_msg(args: CommitCitationArgs) -> int: + # Guaranteed non-None/non-empty here: CommitCitationArgs' own validator + # already rejected any commit-msg-mode construction missing it; cast (not + # assert -- ruff S101 bans a bare assert outside tests) only narrows the + # static type to match that already-enforced runtime guarantee. + try: + # Checked before the message is even read: a merge commit is exempt + # by this issue's own non-goal, exactly as `--mode pr-range`'s own + # `git log --no-merges` already exempts it. + if merge_in_progress(args.root): + print("PASS: merge commit -- exempt, matching --mode pr-range's own git log --no-merges") + return 0 + raw = _read_input_file(cast(str, args.commit_msg_file), label="commit message file") + # Never the raw text: at commit-msg-hook time git has not run its own + # cleanup yet, so `raw` still carries the comment block and (under + # commit.verbose) the whole staged diff -- see the module docstring's + # own live-reproduced false-PASS. + message = clean_commit_message(args.root, raw) + cited = check_commit_message(message) + except CitationGateError as error: + print(f"error: {error}", file=sys.stderr) + return 2 + + if cited: + print("PASS: commit message cites an issue") + return 0 + print( + "FAIL: commit message cites no issue. Per this repository's own citation convention " + "(CONTRIBUTING.md), cite one via Closes #N, Fixes #N, Refs #N, or a bare #N.", + file=sys.stderr, + ) + return 1 + + +def _run_pr_range(args: CommitCitationArgs) -> int: + try: + title = _read_input_file(args.title) + body = _read_input_file(args.body) + passed, message = evaluate_pr_range( + args.root, + args.owner, + args.repo, + title, + body, + args.base_ref, + args.head_ref, + # Flag *presence*, never text emptiness: CI always passes both + # (a PR body can legitimately be empty), so CI never reaches + # evaluate_pr_range's own "nothing to check" branch. + pr_text_supplied=args.title is not None or args.body is not None, + ) + except CitationGateError as error: + print(f"error: {error}", file=sys.stderr) + return 2 + + if passed: + print(f"PASS: {message}") + return 0 + print(f"FAIL: {message}", file=sys.stderr) + return 1 + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser( + description="Enforce CLAUDE.md section 3's issue-citation rule for commits (issue #1212): " + "a citation must exist in at least one non-merge commit, or in the PR title/body." + ) + parser.add_argument("--mode", required=True, choices=["commit-msg", "pr-range"]) + parser.add_argument( + "commit_msg_file", + nargs="?", + help="--mode commit-msg only: path to the commit message file (prek's own sole positional argument).", + ) + parser.add_argument("--owner", default="", help="--mode pr-range only: the PR's own target repo owner.") + parser.add_argument("--repo", default="", help="--mode pr-range only: the PR's own target repo name.") + parser.add_argument("--title", help="--mode pr-range only: path to a file holding the PR title. Omit for none.") + parser.add_argument("--body", help="--mode pr-range only: path to a file holding the PR body. Omit for none.") + parser.add_argument( + "--base-ref", + help="--mode pr-range only: the PR's base ref/SHA (a workflow's own precomputed merge-base). " + "Omit for a local run -- self-heals refs/remotes/origin/main.", + ) + parser.add_argument("--head-ref", default="HEAD", help="--mode pr-range only: the PR's head ref/SHA.") + parser.add_argument( + "--root", + type=pathlib.Path, + default=REPO_ROOT, + help="The git working tree to scan (--mode pr-range), and the one whose core.commentChar " + "`git stripspace` resolves when cleaning the message (--mode commit-msg).", + ) + args = parser.parse_args(argv) + + try: + validated = CommitCitationArgs( + mode=args.mode, + commit_msg_file=args.commit_msg_file, + owner=args.owner, + repo=args.repo, + title=args.title, + body=args.body, + base_ref=args.base_ref, + head_ref=args.head_ref, + root=args.root, + ) + except ValidationError as error: + print(f"error: invalid CLI arguments: {error}", file=sys.stderr) + return 2 + + if validated.mode == "commit-msg": + return _run_commit_msg(validated) + return _run_pr_range(validated) + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/.github/scripts/gitapex_gate_local_preflight.py b/.github/scripts/gitapex_gate_local_preflight.py index b5e4bbdc..6d10226d 100644 --- a/.github/scripts/gitapex_gate_local_preflight.py +++ b/.github/scripts/gitapex_gate_local_preflight.py @@ -78,7 +78,7 @@ (issue #890), which closes the "configured here but never actually installed" half; nothing closes the ``--no-verify`` half. CI remains the authoritative merge gate for every gate carrying a ``ci`` plane -- true - for 41 of the 43 wired gates. ``behind-base`` (issue #985) and + for 42 of the 44 wired gates. ``behind-base`` (issue #985) and ``real-checkout-git-write`` (issue #991) are the two exceptions: each carries only ``local``, so for those two gates specifically this pre-push hook -- bypassable the same way as any other -- is the *only* @@ -90,7 +90,7 @@ ``jsonschema`` -- a real, non-stdlib dependency, contrary to an earlier revision of this paragraph's own "the runner itself needs no dependencies" claim. A bare system ``python3`` with no ``jsonschema`` - installed crashed the whole runner on import before any of the 43 wired + installed crashed the whole runner on import before any of the 44 wired gates got a chance to run individually, so CONTRIBUTING.md's standalone example and the pre-push hook's own ``entry`` both now invoke it as ``uv run --frozen python3`` too, the same pin every wired gate's own argv @@ -177,9 +177,10 @@ # own _GROUP_TIMEOUT_SECONDS = 600 -- so that one gate's own theoretical # worst case is ~4200 s, not 600 s. A ceiling matching that would be useless # as a hang guard (80 minutes of a silent pre-push), so this is a judgment -# call in the other direction. For scale: a warm run of all 43 wired gates -# combined measures roughly 15 s end to end in this measurement (the -# prior 42-gate set measured roughly 18 s, the 41-gate set before that +# call in the other direction. For scale: a warm run of all 44 wired gates +# combined measures roughly 15 s end to end (the +# prior 43-gate set measured roughly 15 s, the 42-gate set before that +# measured roughly 18 s, the 41-gate set before that # measured roughly 18 s, the 40-gate set before that # measured roughly 17 s, the 39-gate set before that # measured roughly 12 s, the 38-gate set before that diff --git a/.github/workflows/commit-citation-gate.yml b/.github/workflows/commit-citation-gate.yml new file mode 100644 index 00000000..8d03897f --- /dev/null +++ b/.github/workflows/commit-citation-gate.yml @@ -0,0 +1,93 @@ +# Issue #1212: CLAUDE.md section 3 requires an issue citation ("Closes #N", +# "Fixes #N", "Refs #N", or a bare "#N") in every commit, but nothing +# enforced it. `.pre-commit-config.yaml`'s `commit-citation` hook (stages: +# [commit-msg]) is the fast local first pass; this job is the actual +# no-exceptions backstop, per this issue's own Acceptance Criteria Map -- +# the local hook is bypassable with `git commit --no-verify` and only exists +# in a clone where `prek install -t commit-msg` has actually run, but this +# check runs unconditionally on the PR itself. +# +# Passes when a citation is found in at least one *non-merge* commit in the +# PR's own range (`git log --no-merges`, so an uncited merge commit in that +# range never fails this on its own), OR in the PR's own title/body. +# +# Deliberately no `paths:` filter -- same reasoning as lint.yml's and +# hidden-characters-gate.yml's own stated headers (see also +# gitapex_gate_ruleset_required_checks.py's module docstring): a required +# status check backed by a workflow that never fires for a given pull +# request leaves that check `Pending` forever, with no in-repository fix. A +# citation can be missing regardless of which files a PR touches, so there +# is no path this job could safely skip anyway. +name: Commit citation gate + +on: + pull_request: + types: [opened, edited, synchronize, reopened] + +permissions: + contents: read + +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + commit-citation: + runs-on: ubuntu-latest + timeout-minutes: 5 + permissions: + contents: read + steps: + # fetch-depth: '0' -- this job computes a real `git merge-base` and + # then walks `git log --no-merges` over the resulting range, so it + # needs full history locally, not the shallow default. `ref: head.sha` + # pins exactly which commit is checked out, matching + # exception-handler-gap-gate.yml's own established pin. + - name: Harden runner + checkout + uses: tvna/gitapex/.github/actions/harden-checkout@2f62b5648552a0f800b1b85e75ec108a7016dd02 + with: + fetch-depth: '0' + ref: ${{ github.event.pull_request.head.sha }} + + - name: Install uv + uses: astral-sh/setup-uv@c771a70e6277c0a99b617c7a806ffedaca235ff9 # v9.0.0 + with: + enable-cache: true + + # PR_TITLE/PR_BODY carry untrusted external text (anyone with + # PR-author access controls them) -- written to files via `env:` + # indirection, never interpolated directly into a shell command line, + # mirroring provenance-disclosure-gate.yml's own PR_BODY handling. + # gitapex_gate_commit_citation.py's own --title/--body take file paths + # (not stdin) because this mode needs both pieces of text at once. + - name: Write PR title/body to files + env: + PR_TITLE: ${{ github.event.pull_request.title }} + PR_BODY: ${{ github.event.pull_request.body }} + run: | + set -euo pipefail + printf '%s' "$PR_TITLE" > "$RUNNER_TEMP/pr_title.txt" + printf '%s' "$PR_BODY" > "$RUNNER_TEMP/pr_body.txt" + + # git merge-base below -- never logs base.sha directly, which can go + # stale relative to a main that advanced after the PR opened; mirrors + # exception-handler-gap-gate.yml's own established + # merge-base-not-base.sha pattern. + # + # --owner/--repo are hardcoded literals, matching acm-issue-gate.yml's + # own convention, rather than interpolating a workflow expression + # directly into this shell script. + - name: Check for an issue citation in the PR's own commits or title/body + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + set -euo pipefail + if ! merge_base=$(git merge-base "$BASE_SHA" "$HEAD_SHA"); then + echo "::error::git merge-base failed between $BASE_SHA and $HEAD_SHA" >&2 + exit 1 + fi + uv run --frozen python3 .github/scripts/gitapex_gate_commit_citation.py \ + --mode pr-range --owner tvna --repo gitapex \ + --base-ref "$merge_base" --head-ref "$HEAD_SHA" \ + --title "$RUNNER_TEMP/pr_title.txt" --body "$RUNNER_TEMP/pr_body.txt" diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index e9e1cb9c..e8038d85 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -9,9 +9,9 @@ # pyproject.toml/uv.lock, rather than declaring a second copy of # select/ignore/strict settings here -- see issue #725's own "Constraints". # -# Setup: `uv run prek install -t pre-commit -t pre-push` (documented in -# CONTRIBUTING.md; also wired into flake.nix's devShell so `nix develop` -# installs it automatically). +# Setup: `uv run prek install -t pre-commit -t pre-push -t commit-msg` +# (documented in CONTRIBUTING.md; also wired into flake.nix's devShell so +# `nix develop` installs it automatically). # Issue #890 installs a pre-push shim as well as a pre-commit one. Without # this, the ruff/mypy hooks below -- which declare no `stages` -- would start @@ -128,6 +128,29 @@ repos: pass_filenames: false require_serial: true + # Issue #1212: CLAUDE.md section 3 requires an issue citation in + # every commit, but nothing enforced it -- this is the fast local + # first pass. `.github/scripts/gitapex_gate_commit_citation.py`'s own + # `--mode pr-range` (wired into CI via + # .github/workflows/commit-citation-gate.yml, and into this repo's + # own local-preflight via .gitapex/ssot.json's `commit-citation-gate` + # entry) is the actual no-exceptions backstop: it also passes when the + # PR title/body carries the citation instead, so this local hook + # bypassable with `git commit --no-verify` -- is a convenience, not + # the enforcement boundary itself. + # + # No `pass_filenames` override: prek's own `commit-msg` stage always + # hands the hook the commit-message file path as its sole positional + # argument (git's own commit-msg hook contract), live-verified against + # a real `prek install -t commit-msg` run rather than assumed -- see + # this script's own module docstring. + - id: commit-citation + name: commit message cites an issue + entry: uv run --frozen python3 .github/scripts/gitapex_gate_commit_citation.py --mode commit-msg + language: system + stages: [commit-msg] + require_serial: true + # Issue #876. The three hooks above are pre-commit-stage and cover # ruff/mypy only; every other gate with a working-tree-only form ran # exclusively in CI, so a gap surfaced one red check at a time on an @@ -135,7 +158,7 @@ repos: # the last local moment before a gap becomes a CI round-trip, and # deliberately not pre-commit: it grades committed state (HEAD vs # origin/main), not a staged index, and at ~15 s - # warm end to end for all 43 wired gates it is too slow to sit on every + # warm end to end for all 44 wired gates it is too slow to sit on every # single commit. # # The wired set is not listed here: the runner discovers it from diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 373d1924..2effa9ee 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -9,12 +9,12 @@ dependency and `.pre-commit-config.yaml` wires it to this repo's own hook. Run once per clone: ```sh -uv run prek install -t pre-commit -t pre-push +uv run prek install -t pre-commit -t pre-push -t commit-msg ``` -Both stages matter, so both shims are named: `prek install` with no `-t` -installs the pre-commit shim only, and the pre-push secret scan would then -never run. +All three stages matter, so all three shims are named: `prek install` with +no `-t` installs the pre-commit shim only, and the pre-push secret scan +and the commit-msg issue-citation check (issue #1212) would then never run. This makes `git commit` reject a commit that fails ruff, mypy, or the secret scan locally, before it exists, rather than only after a push reaches @@ -63,7 +63,7 @@ fail outright once the worktree is removed: .git/hooks/pre-push: exec: prek: not found ``` -Recovery is `uv run prek install --overwrite -t pre-commit -t pre-push` from the +Recovery is `uv run prek install --overwrite -t pre-commit -t pre-push -t commit-msg` from the main checkout. The devShell already refuses to install from a worktree for this reason -- it verifies the shared shims and tells you to install from the main checkout instead. @@ -123,11 +123,12 @@ The pre-commit hooks above cover ruff and mypy only. Most of this repository's other deterministic gates run as separate CI jobs, so a gap used to be discovered one red check at a time on an already-open PR. -The same `uv run prek install -t pre-commit -t pre-push` above also installs +The same `uv run prek install -t pre-commit -t pre-push -t commit-msg` above also installs a **pre-push** hook that runs every gate with a working-tree-only form in -one pass, before the push leaves your machine. A warm run of all 43 wired -gates measures roughly 15 seconds end to end in this measurement (the -prior 42-gate set measured roughly 18 seconds, the 41-gate set before that +one pass, before the push leaves your machine. A warm run of all 44 wired +gates measures roughly 15 seconds end to end (the +prior 43-gate set measured roughly 15 seconds, the 42-gate set before that +measured roughly 18 seconds, the 41-gate set before that measured roughly 18 seconds, the 40-gate set before that measured roughly 17 seconds, the 39-gate set before that measured roughly 12 seconds, the 38-gate set before that @@ -158,7 +159,7 @@ it up, then confirm both shims with the check in the previous section. The runner itself also resolves through `uv` (issue #1485: it imports `_gitapex_schema_validation.py`, which needs `jsonschema` -- a real, non-stdlib dependency a bare system `python3` is not guaranteed to have), -and so do all 43 wired gates (the same `uv run` pins CI uses). Without `uv` +and so do all 44 wired gates (the same `uv run` pins CI uses). Without `uv` on PATH every one of them reports `FAIL ... failed to run` -- that is one missing tool, not a whole broken wired set. diff --git a/docs/superpowers/plans/2026-08-29-claude-pr-1212-merge-ready-om26qe.md b/docs/superpowers/plans/2026-08-29-claude-pr-1212-merge-ready-om26qe.md new file mode 100644 index 00000000..3dcf55f3 --- /dev/null +++ b/docs/superpowers/plans/2026-08-29-claude-pr-1212-merge-ready-om26qe.md @@ -0,0 +1,209 @@ +# hooks/CI: add issue-citation check for commit messages (issue #1212) + +**Goal:** two-layer enforcement of CLAUDE.md section 3's issue-citation +rule for commits: (1) a `commit-msg` git hook as a fast local first pass, +(2) a CI check as the actual no-exceptions backstop, passing when a +citation exists in at least one non-merge commit in the PR's range OR the +PR title/body. Source: https://github.com/tvna/gitapex/issues/1212. + +**Independent re-verification of the ACM (`planning-a-branch-from-an-issue` +Step 5):** performed this session, recorded as a re-verification marker on +issue #1212's own body (2026-08-29T19:00:21Z). No ACM row corrections -- +both design unknowns the issue itself named were already settled by its +own most recent update (reuse `extract_citations()` from +`hooks/gitapex_check_pr_issue_acm_disclosure.py`; one script, +`.github/scripts/gitapex_gate_commit_citation.py`, two `--mode` values, +mirroring `gitapex_run_betterleaks.py`'s own precedent). Independently +confirmed against current `origin/main` (91f15c6d): `extract_citations()` +exists exactly as described (`hooks/gitapex_check_pr_issue_acm_disclosure.py` +lines 150-217); no `commit-msg` stage exists in `.pre-commit-config.yaml`; +`CONTRIBUTING.md` line 12 (also 66, 126) and `flake.nix` line 244 (also +242, 246) carry `-t pre-commit -t pre-push` with no `-t commit-msg`; +`pyproject.toml` line 350's pytest `pythonpath` already lists +`.github/scripts` and `hooks` side by side. + +One addition beyond the issue's own stated Planned ops, folded in as real +repo convention rather than scope creep: `.gitapex/ssot.json` gate +registration. This repository's `registry-wiring-scan` gate and +`gitapex_gate_local_preflight.py`'s own docstring establish that every +deterministic gate with a working-tree-runnable form must carry a +registry entry (`planes` including `"local"` + `local_invocation`, or a +`local_exclusion` string -- `.gitapex/ssot.schema.json` requires one or +the other). Confirmed by inspecting existing entries: pre-commit-stage-only +hooks like `ruff-check`/`skill-shape-check` get NO separate top-level +ssot.json id (grep across all 69 existing entries found neither) -- only +the consolidated/CI-facing checks are registered. So this plan adds +exactly one new `ssot.json` gate entry, for the `--mode pr-range` CI +check, modeled on the `python-lint` and `toolchain-pin-drift` entries' +shape (`planes: ["ci","local"]`, `local_invocation` array). No change to +`.github/rulesets/main.json` or `apply-rulesets.yml` -- the issue's scope +is the check's presence, not making it a GitHub-required status check. + +**File-ownership check:** not applicable -- single-task decomposition (see +below), no sibling task to check against. + +**Canonical-governance-paths pre-filter (mechanized):** +`gitapex_check_canonical_governance_paths.py` against the 7 planned +changed paths -> 2 canonical matches (`hook-script`: +`.github/scripts/gitapex_gate_commit_citation.py`; `workflow`: +`.github/workflows/commit-citation-gate.yml`), 5 `no-match` (the test +module, `.pre-commit-config.yaml`, `CONTRIBUTING.md`, `flake.nix`, +`.gitapex/ssot.json` -- needs the model's own full-diff review, all of +which are expected, in-scope edits per the ACM's own Planned ops and the +ssot.json finding above). Full model review (the `untrusted-input-triage` +Extract/Ignore/Flag/Tag pass over the ACM's own text) already run: nothing +in the issue's Facts, Acceptance Criteria Map, Constraints, or Non-goals +reads as an injected instruction rather than a change description -- the +imperative-sounding "Planned ops" cell text ("Wired into +`.pre-commit-config.yaml` as...", "invoked from a `.github/workflows/*.yml` +step") is the issue's own OWNER-author describing what to build, same +pattern as every other ACM this skill has produced. + +**Interface-dependency edges:** none -- single task. + +**Waves:** wave 1: {task-1} (only task). + +**Execution mode:** sequential main-thread fallback, no `Workflow` tool +run -- this session carries no explicit multi-agent-orchestration opt-in +("ultracode" or an explicit user request for a workflow). Step 8's +refactor and adversarial-review passes each use a fresh `Agent`-tool +subagent dispatch, at a stronger-reasoning tier and this session's +default-or-higher effort. + +**Irreversibility classification:** not irreversible -- an ordinary, +git-revertible set of file additions/edits (a new script, a new test +module, edits to `.pre-commit-config.yaml`/`CONTRIBUTING.md`/`flake.nix`/ +`.gitapex/ssot.json`, a new CI workflow step). No data deletion, no live +external write beyond the eventual `git push`/PR-open (both main-thread, +already covered by step 1's own authorization), no schema migration. No +task requires a fresh per-task authorization confirmation beyond the +branch-plan-wide one recorded below. + +**Authorization record (step 1):** structural precondition PASS +(`gitapex_check_branch_plan_reverified.py` against issue #1212's live +body -- the `planning-a-branch-from-an-issue` re-verification marker is +present, timestamp 2026-08-29T19:00:21Z). Semantic approval: in-session +explicit confirmation from the human operator, directly instructing +"こちらのPRを作りマージ直前まで進める" (create this PR and drive it to just +before merge) against issue #1212's own URL -- unambiguous, directly +responsive to this specific issue, no embedded instruction attempting to +redirect this gate. No comments exist on issue #1212 (checked via +`get_comments` -- empty). + +## Task 1 -- One script, two modes: commit-msg hook + CI backstop + +**Cites ACM rows:** both rows (this task is the whole issue -- one script +serves both layers). + +**Quoted Planned ops (verbatim from the issue body):** "One new script, +`.github/scripts/gitapex_gate_commit_citation.py`, following the exact +shape `gitapex_run_betterleaks.py --mode staged`/`--mode history` already +establishes ... a `--mode commit-msg` mode reads the message file path +from `sys.argv[1]` ... bootstraps `sys.path` to reach `hooks/` ... imports +`extract_citations` from `hooks/gitapex_check_pr_issue_acm_disclosure.py` +... passes when `extract_citations(...)` returns a non-empty `resolving` +or `context` tuple. Wired into `.pre-commit-config.yaml` as `entry: uv run +--frozen python3 .github/scripts/gitapex_gate_commit_citation.py --mode +commit-msg`, `stages: [commit-msg]`"; "The same +`gitapex_gate_commit_citation.py`, in a `--mode pr-range` mode, invoked +from a `.github/workflows/*.yml` step ... passes if a citation exists in +(a) at least one non-merge commit's message in the PR's commit range +(`git log --no-merges BASE..HEAD`), or (b) the PR title/body". + +**Files:** +- `.github/scripts/gitapex_gate_commit_citation.py` (new) +- `tests/test_gitapex_gate_commit_citation.py` (new; exact name/location + matched against this repo's existing `.github/scripts/*.py` test + convention before authoring) +- `.pre-commit-config.yaml` (new `commit-msg`-stage hook entry) +- `CONTRIBUTING.md` (add `-t commit-msg` at each `prek install` + invocation line) +- `flake.nix` (same, in the devShell's `prek install` invocation and its + recovery/error message strings) +- `.gitapex/ssot.json` (one new gate entry, `--mode pr-range` only) +- `.github/workflows/*.yml` (new minimal workflow, or a step added to an + existing appropriately-scoped one -- decided during implementation + after checking existing workflow files for a natural home) + +**Design, fixed at decomposition time:** +- Reuse `extract_citations()` verbatim via import; do not re-implement + the regex. +- `--mode commit-msg`: read `sys.argv[1]` as the commit-msg file path (the + argument `stages: [commit-msg]` pre-commit hooks receive), read its + text, call `extract_citations(owner=None, repo=None, title=None, + body=)`, exit 0 if `resolving` or `context` non-empty, + exit 1 with a clear stderr message otherwise. +- `--mode pr-range`: accept flags for base ref, head ref (or read from + environment/argv per this repo's existing CI-script CLI conventions -- + check `gitapex_gate_behind_base.py`/similar for the established + base-ref-resolution pattern, e.g. `_gitapex_base_ref.py`, before + inventing a new one), plus the PR title/body (or fetch via the GitHub + API through the shared `_gitapex_github_http.py` helper, matching this + repo's own established pattern for a CI script that needs PR metadata). + Run `git log --no-merges .. --format=%B` (or equivalent), + concatenate each non-merge commit message's own citation extraction, + OR with the PR title/body's own extraction; pass if any citation found + anywhere in that union, fail with a clear message otherwise. +- Follow this repo's established `argparse` + `pydantic`-validated CLI + namespace convention (per issue #1040's rollout, already used by + `gitapex_run_betterleaks.py` and others) for both scripts' shared + parser. +- `sys.path.insert(0, ...)` bootstrap to reach `hooks/`, one line, same + style as every existing cross-file `.github/scripts/*.py` import. +- New `.gitapex/ssot.json` entry: id (e.g. `commit-citation-gate`), + `kind: "script"`, `script` naming the new script + the workflow file + that invokes it, `planes: ["ci","local"]`, `local_invocation` running + `--mode pr-range` against the working tree's own current branch vs. + `origin/main` (mirroring how other PR-range-shaped local invocations in + this registry resolve a local base ref -- verify against + `gitapex_gate_behind_base.py`'s own registry entry or similar before + finalizing), `trigger` naming the new/extended workflow + `pull_request` + event, `tracking_issue: 1212`, `cluster` picked from this repo's + existing cluster vocabulary (e.g. `plan-integrity`, matching + `pr-issue-acm-disclosure`'s own cluster), `status: "active"`, + `bypass_review_status: "not-yet-reviewed"`, `supersedes: null`, + `policy_refs: []`, `target` naming the workflow event and the file + globs it covers. Validate against `.gitapex/ssot.schema.json` and re-run + `gitapex_scan_ssot_schema.py`/`registry-wiring-scan` locally before + pushing. +- Do not touch `.github/rulesets/main.json` or run `apply-rulesets.yml`. +- Do not retroactively re-cite any already-merged commit history (explicit + Non-goal). +- Do not attempt citation-*format* validation (explicit Non-goal, tracked + separately as #521). + +**Proof method:** new pytest module covering: `--mode commit-msg` accepts +a message carrying a citation (`Closes #123`, `Refs #123`, bare `#123`) +and rejects one without; `--mode pr-range` passes when the citation is +only in the PR body, passes when it's only in one non-merge commit, +does NOT fail solely because an uncited merge commit is in range, and +fails when no citation exists anywhere (title, body, or any non-merge +commit); confirms the `git log --no-merges` invocation actually excludes +a merge commit from the scan (a real merge commit fixture, not just a +mocked git-log). `ruff check`/`ruff format --check`/mypy clean on the new +script and test module. Full local test suite green +(`uv run pytest`, or the local-preflight runner covering the newly wired +gate). Actual `git commit -m` message for this task's own commit contains +`#1212` (dogfooding). + +## Post-task gate (Decision 12, mandatory) + +After the task lands: one refactor/simplify pass (behavior-preserving +only) and one independent adversarial code review, each a fresh +`Agent`-tool subagent dispatch at a stronger-reasoning tier and this +session's default-or-higher effort, over the full accumulated diff. Given +this diff adds a new deterministic gate/check script, the adversarial +review must construct and run at least one case built to defeat its own +detection logic (per this skill's own Stop boundaries), at minimum: +(a) a commit message citing an issue only inside a fenced/inline code +block (should NOT count, matching `extract_citations()`'s own documented +fence-stripping behavior) -- confirm the new script inherits this +correctly through its own message-extraction path; (b) a PR body citing a +*foreign* repo's issue (`other-owner/other-repo#123`) should not +false-positive as this repo's own citation; (c) an empty or whitespace-only +commit-msg file; (d) a PR with zero non-merge commits in range (e.g. a +single merge commit) relying solely on the PR title/body; (e) confirm +`.gitapex/ssot.json`'s new entry doesn't break `registry-wiring-scan` or +`ssot-schema-drift`. Every CONFIRMED finding is fixed and, where a +proof-method check exists for the affected area, re-run before the draft +PR converts to ready-for-review. diff --git a/flake.nix b/flake.nix index 08e4a19f..b22a8669 100644 --- a/flake.nix +++ b/flake.nix @@ -239,11 +239,11 @@ echo "WARNING: git hooks in the shared hooks directory are missing or unusable:" >&2 echo " $hooks" >&2 echo " This is a linked worktree, which must not install them itself." >&2 - echo " Run this in the main checkout: uv run prek install --overwrite -t pre-commit -t pre-push" >&2 + echo " Run this in the main checkout: uv run prek install --overwrite -t pre-commit -t pre-push -t commit-msg" >&2 fi - elif ! (cd "$root" && uv run prek install --quiet -t pre-commit -t pre-push); then + elif ! (cd "$root" && uv run prek install --quiet -t pre-commit -t pre-push -t commit-msg); then echo "WARNING: prek install failed -- git hooks are NOT active." >&2 - echo " Fix it with: uv run prek install -t pre-commit -t pre-push" >&2 + echo " Fix it with: uv run prek install -t pre-commit -t pre-push -t commit-msg" >&2 else if prek_shim_broken "$hooks/pre-commit"; then echo "WARNING: $hooks/pre-commit is missing or unusable." >&2 diff --git a/tests/test_gitapex_gate_commit_citation.py b/tests/test_gitapex_gate_commit_citation.py new file mode 100644 index 00000000..0922668d --- /dev/null +++ b/tests/test_gitapex_gate_commit_citation.py @@ -0,0 +1,1107 @@ +"""Tests for the two-layer commit-citation gate +(.github/scripts/gitapex_gate_commit_citation.py, issue #1212). + +Every fixture repo below is real -- built with `git init`/`git commit`/ +`git merge` under `tmp_path`, matching tests/test_gitapex_run_base_diff.py's +and tests/test_gitapex_base_ref.py's own convention -- so the `--no-merges` +exclusion this gate depends on is genuinely exercised against a real merge +commit, not only assumed from `git log`'s documented behavior. +""" + +from __future__ import annotations + +import os +import pathlib +import subprocess +import sys + +import gitapex_gate_commit_citation as gate +import pytest +from conftest import make_validation_error + +# --- real-repo fixture helpers (mirrors tests/test_gitapex_run_base_diff.py) - + + +def _run(args: list[str], cwd: pathlib.Path) -> subprocess.CompletedProcess[str]: + return subprocess.run(args, cwd=cwd, check=True, capture_output=True, text=True) + + +def _init_repo(root: pathlib.Path, *, branch: str = "main") -> pathlib.Path: + root.mkdir(parents=True, exist_ok=True) + _run(["git", "init", "-q", "--initial-branch", branch], root) + _run(["git", "config", "user.email", "test@example.com"], root) + _run(["git", "config", "user.name", "Test"], root) + return root + + +def _commit(root: pathlib.Path, name: str, message: str) -> str: + (root / name).write_text(f"{name}\n", encoding="utf-8") + _run(["git", "add", "--", name], root) + _run(["git", "commit", "-q", "-m", message], root) + return _run(["git", "rev-parse", "HEAD"], root).stdout.strip() + + +def _checkout_new_branch(root: pathlib.Path, branch: str, start_point: str | None = None) -> None: + args = ["git", "checkout", "-q", "-b", branch] + if start_point is not None: + args.append(start_point) + _run(args, root) + + +def _merge_no_ff(root: pathlib.Path, branch: str, message: str) -> str: + _run(["git", "merge", "-q", "--no-ff", "-m", message, branch], root) + return _run(["git", "rev-parse", "HEAD"], root).stdout.strip() + + +def _raw_hook_commit_editmsg(subject: str, staged_diff_citation: str) -> str: + """A `COMMIT_EDITMSG` shaped exactly the way git itself writes it at + `commit-msg`-hook invocation time, under `commit.verbose=true` -- the + subject the contributor typed, then git's own comment block, then the + scissors line, then the verbatim staged diff. + + Captured from a real `git commit` against a real repo with a real + `.git/hooks/commit-msg` (issue #1212's own adversarial review), not + hand-imagined: git strips the comments and everything from the + scissors line down *after* the hook returns, so this raw shape is + genuinely what the hook receives. `test_commit_msg_hook_receives_the_raw_ + uncleaned_file_from_real_git` below re-derives it live from real git + rather than trusting this constant to stay accurate.""" + return ( + f"{subject}\n" + "\n" + "# Please enter the commit message for your changes. Lines starting\n" + "# with '#' will be ignored, and an empty message aborts the commit.\n" + "#\n" + "# On branch main\n" + "#\n" + "# Changes to be committed:\n" + "#\tnew file: mod.py\n" + "#\n" + f"# {gate.SCISSORS_MARKER}\n" + "# Do not modify or remove the line above.\n" + "# Everything below it will be ignored.\n" + "diff --git a/mod.py b/mod.py\n" + "new file mode 100644\n" + "index 0000000..52643d8\n" + "--- /dev/null\n" + "+++ b/mod.py\n" + "@@ -0,0 +1,3 @@\n" + "+def f():\n" + f"+ # {staged_diff_citation}\n" + "+ return 1\n" + ) + + +def _build_range_repo(tmp_path: pathlib.Path, *, citing_commit: bool) -> tuple[pathlib.Path, str, str]: + """A repo with a `main` base commit, a `feature` branch carrying two + ordinary (non-merge) commits with no citation, a real `--no-ff` merge + of a third `side` branch (also no citation) into `feature`, and -- + only when `citing_commit` -- one final ordinary commit that does carry + one. Returns (root, base_sha, head_sha).""" + root = _init_repo(tmp_path / ("repo-cited" if citing_commit else "repo-uncited")) + base_sha = _commit(root, "a.txt", "chore: init") + + _checkout_new_branch(root, "feature") + _commit(root, "b.txt", "feat: work on the feature (no citation)") + + _checkout_new_branch(root, "side", base_sha) + _commit(root, "c.txt", "chore: side work (no citation)") + + _run(["git", "checkout", "-q", "feature"], root) + _merge_no_ff(root, "side", "Merge branch 'side' into feature") + + head_sha = _run(["git", "rev-parse", "HEAD"], root).stdout.strip() + if citing_commit: + head_sha = _commit(root, "d.txt", "fix: correct the bug\n\nCloses #42") + + return root, base_sha, head_sha + + +# --- check_commit_message / check_pr_text ----------------------------------- + + +@pytest.mark.parametrize("message", ["fix: bug\n\nCloses #123", "chore: work\n\nRefs #123", "chore: work (#123)"]) +def test_check_commit_message_accepts_every_citation_form(message: str) -> None: + assert gate.check_commit_message(message) is True + + +def test_check_commit_message_rejects_no_citation() -> None: + assert gate.check_commit_message("chore: tidy up formatting") is False + + +def test_check_commit_message_a_citation_inside_a_fenced_code_block_does_not_count() -> None: + # Proves the integration reuses extract_citations' own fence-stripping + # rather than bypassing it -- the exact false positive issue #657's own + # adversarial review found live in hooks/gitapex_check_pr_issue_acm_disclosure.py's + # own PR body. + message = "docs: explain the citation syntax\n\n```\nCloses #123\n```\n" + assert gate.check_commit_message(message) is False + + +def test_check_pr_text_a_citation_inside_inline_code_does_not_count() -> None: + body = "This hook accepts citations shaped like `Closes #123`." + assert gate.check_pr_text("tvna", "gitapex", "", body) is False + + +def test_check_pr_text_finds_a_citation_in_the_title_alone() -> None: + assert gate.check_pr_text("tvna", "gitapex", "fix: bug (Closes #99)", "") is True + + +def test_check_pr_text_finds_a_citation_in_the_body_alone() -> None: + assert gate.check_pr_text("tvna", "gitapex", "", "Closes #99") is True + + +def test_check_pr_text_normalizes_a_same_repo_qualified_citation() -> None: + assert gate.check_pr_text("tvna", "gitapex", "", "Fixes tvna/gitapex#7") is True + + +# --- resolve_base_ref --------------------------------------------------------- + + +def test_resolve_base_ref_returns_an_explicit_ref_unchanged_with_no_git_call( + tmp_path: pathlib.Path, monkeypatch: pytest.MonkeyPatch +) -> None: + def _fail_if_called(*_args: object, **_kwargs: object) -> None: + raise AssertionError("no self-heal git call should run when --base-ref is given explicitly") + + monkeypatch.setattr(gate._gitapex_base_ref, "peeled_ref_exists", _fail_if_called) + monkeypatch.setattr(gate._gitapex_base_ref, "fetch_destination_refspec", _fail_if_called) + monkeypatch.setattr(gate._gitapex_base_ref, "require_common_ancestor", _fail_if_called) + assert gate.resolve_base_ref(tmp_path, "deadbeef") == "deadbeef" + + +@pytest.mark.slow +def test_resolve_base_ref_is_a_noop_probe_when_the_ref_already_resolves( + tmp_path: pathlib.Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The common case: an ordinary (non-restricted-refspec) clone where + `refs/remotes/origin/main` already resolves locally. No fetch should + run at all -- only the cheap peeled probe.""" + origin = _init_repo(tmp_path / "origin") + _commit(origin, "a.txt", "chore: init") + head = _init_repo(tmp_path / "head") + _run(["git", "remote", "add", "origin", str(origin)], head) + _run(["git", "fetch", "-q", "origin", "main"], head) + _run(["git", "checkout", "-q", "-b", "main", "origin/main"], head) + _checkout_new_branch(head, "feature") + _commit(head, "b.txt", "feat: local work") + + def _fail_if_called(*_args: object, **_kwargs: object) -> None: + raise AssertionError("no fetch should run once the ref already resolves") + + monkeypatch.setattr(gate._gitapex_base_ref, "fetch_destination_refspec", _fail_if_called) + assert gate.resolve_base_ref(head, None) == "refs/remotes/origin/main" + + +@pytest.mark.slow +def test_resolve_base_ref_self_heals_in_a_restricted_refspec_clone(tmp_path: pathlib.Path) -> None: + """The same regression shape issue #1345 fixed for gitapex_run_base_diff.py: + a `git clone --single-branch --branch` clone never populates + `refs/remotes/origin/main` from a source-only fetch, so a bare + `origin/main` reference fails outright there. resolve_base_ref must + self-heal it via a destination-refspec fetch when --base-ref is omitted.""" + origin = _init_repo(tmp_path / "origin") + _commit(origin, "a.txt", "chore: init") + _checkout_new_branch(origin, "feature") + _commit(origin, "b.txt", "feat: work") + + work = tmp_path / "work" + _run(["git", "clone", "-q", "--single-branch", "--branch", "feature", str(origin), str(work)], tmp_path) + + old_raw = subprocess.run( + ["git", "-C", str(work), "rev-parse", "--verify", "--quiet", "origin/main^{commit}"], + capture_output=True, + text=True, + check=False, + ) + assert old_raw.returncode != 0 # confirms the ref genuinely does not resolve yet + + resolved = gate.resolve_base_ref(work, None) + assert resolved == "refs/remotes/origin/main" + + +def test_resolve_base_ref_raises_distinctly_when_fetch_reports_success_but_ref_still_missing( + tmp_path: pathlib.Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The 'never trust the fetch's exit code alone' defeat test (issue + #1345): simulate a fetch that reports success (no exception) but the + ref genuinely never materializes on re-check.""" + root = _init_repo(tmp_path / "repo") + _commit(root, "a.txt", "chore: init") + + monkeypatch.setattr(gate._gitapex_base_ref, "fetch_destination_refspec", lambda *_a, **_k: None) + with pytest.raises(gate.CitationGateError, match="reported success but"): + gate.resolve_base_ref(root, None) + + +@pytest.mark.slow +def test_resolve_base_ref_raises_distinctly_on_a_shallow_clone_with_no_common_ancestor( + tmp_path: pathlib.Path, +) -> None: + # Mirrors tests/test_gitapex_run_base_diff.py's own + # test_run_diff_names_the_shallow_clone_case_distinctly: a + # --single-branch --branch feature --depth 1 clone has only the + # feature branch's own truncated (depth-1) history locally and no + # local `main` at all, so even after resolve_base_ref's own + # destination-refspec fetch of origin/main succeeds, that fetched ref + # cannot share a common ancestor with the shallow feature history. + origin = _init_repo(tmp_path / "origin") + _commit(origin, "a.txt", "chore: init") + _commit(origin, "b.txt", "chore: second") + _checkout_new_branch(origin, "feature") + _commit(origin, "c.txt", "feat: work") + _commit(origin, "d.txt", "feat: more work") + + # `file://{origin}`, not a bare path: a bare local path triggers git's + # own local-clone hardlink fast path, which can ignore --depth + # entirely -- the exact same reason + # tests/test_gitapex_run_base_diff.py's own analogous test uses this + # form. + shallow = tmp_path / "shallow" + _run( + [ + "git", + "clone", + "-q", + "--single-branch", + "--branch", + "feature", + "--depth", + "1", + f"file://{origin}", + str(shallow), + ], + tmp_path, + ) + + with pytest.raises(gate.CitationGateError, match="common ancestor"): + gate.resolve_base_ref(shallow, None) + + +# --- commit_range_messages / evaluate_pr_range (real merge-commit fixture) -- + + +@pytest.mark.slow +def test_commit_range_messages_excludes_the_merge_commit_but_keeps_the_ordinary_ones( + tmp_path: pathlib.Path, +) -> None: + root, base_sha, head_sha = _build_range_repo(tmp_path, citing_commit=False) + messages = gate.commit_range_messages(root, base_sha, head_sha) + joined = "\n".join(messages) + assert "feat: work on the feature" in joined + assert "chore: side work" in joined + assert "Merge branch 'side' into feature" not in joined + + +@pytest.mark.slow +def test_evaluate_pr_range_citation_only_in_one_non_merge_commit_passes(tmp_path: pathlib.Path) -> None: + root, base_sha, head_sha = _build_range_repo(tmp_path, citing_commit=True) + passed, message = gate.evaluate_pr_range(root, "tvna", "gitapex", "", "", base_sha, head_sha) + assert passed is True + assert "non-merge commit" in message + + +@pytest.mark.slow +def test_evaluate_pr_range_an_uncited_merge_commit_does_not_alone_cause_failure(tmp_path: pathlib.Path) -> None: + # Same repo shape as the passing case above, minus the citing commit -- + # the merge commit alone (uncited) is present in range and must not + # itself flip this to a pass; the overall verdict is still a FAIL since + # nothing anywhere cites an issue, proving --no-merges really excluded it + # rather than merely never being tested. + root, base_sha, head_sha = _build_range_repo(tmp_path, citing_commit=False) + passed, message = gate.evaluate_pr_range(root, "tvna", "gitapex", "", "", base_sha, head_sha) + assert passed is False + assert "cites no issue" not in message # message names the range, see below + assert "neither the PR title/body nor any non-merge commit" in message + + +@pytest.mark.slow +def test_evaluate_pr_range_citation_only_in_pr_body_passes_without_touching_commits( + tmp_path: pathlib.Path, monkeypatch: pytest.MonkeyPatch +) -> None: + def _fail_if_called(*_args: object, **_kwargs: object) -> list[str]: + raise AssertionError("commit_range_messages must not run once the PR body already cites an issue") + + monkeypatch.setattr(gate, "commit_range_messages", _fail_if_called) + root, base_sha, head_sha = _build_range_repo(tmp_path, citing_commit=False) + passed, message = gate.evaluate_pr_range(root, "tvna", "gitapex", "", "Closes #7", base_sha, head_sha) + assert passed is True + assert "title/body" in message + + +@pytest.mark.slow +def test_commit_range_messages_counts_an_empty_message_commit_as_a_commit(tmp_path: pathlib.Path) -> None: + """`git commit --allow-empty-message` produces a real, genuinely + uncited commit whose `%B` is empty. Filtering empty entries out (the + pre-review form) made a range of two such commits indistinguishable + from an *empty range*, which `evaluate_pr_range` now passes as + "nothing to check" -- so an uncited commit would have slipped the gate + entirely. The list must have one entry per commit, empty or not.""" + root = _init_repo(tmp_path / "empty-messages") + _commit(root, "a.txt", "chore: base") + _run(["git", "checkout", "-q", "-b", "feature"], root) + for name in ("b.txt", "c.txt"): + (root / name).write_text(f"{name}\n", encoding="utf-8") + _run(["git", "add", "--", name], root) + _run(["git", "commit", "-q", "--allow-empty-message", "-m", ""], root) + + assert gate.commit_range_messages(root, "main", "feature") == ["", ""] + + +@pytest.mark.slow +def test_evaluate_pr_range_uncited_empty_message_commits_still_fail(tmp_path: pathlib.Path) -> None: + """The end of the same defect: those two commits are uncited, so even + the lenient `pr_text_supplied=False` local shape must still FAIL -- + "nothing to check" means an empty *range*, never "every commit's + message happened to be empty".""" + root = _init_repo(tmp_path / "empty-messages-verdict") + _commit(root, "a.txt", "chore: base") + _run(["git", "checkout", "-q", "-b", "feature"], root) + (root / "b.txt").write_text("b\n", encoding="utf-8") + _run(["git", "add", "--", "b.txt"], root) + _run(["git", "commit", "-q", "--allow-empty-message", "-m", ""], root) + + passed, message = gate.evaluate_pr_range(root, "", "", "", "", "main", "feature", pr_text_supplied=False) + assert passed is False + assert "nothing to check" not in message + + +@pytest.mark.slow +def test_commit_range_messages_raises_on_an_unusable_range(tmp_path: pathlib.Path) -> None: + root = _init_repo(tmp_path / "repo") + _commit(root, "a.txt", "chore: init") + with pytest.raises(gate.CitationGateError, match="git log --no-merges"): + gate.commit_range_messages(root, "not-a-real-ref", "HEAD") + + +# --- CommitCitationArgs ------------------------------------------------------- + + +def test_args_requires_a_commit_msg_file_in_commit_msg_mode(tmp_path: pathlib.Path) -> None: + with pytest.raises(gate.ValidationError, match="commit message file path is required"): + gate.CommitCitationArgs( + mode="commit-msg", + commit_msg_file=None, + owner="", + repo="", + title=None, + body=None, + base_ref=None, + head_ref="HEAD", + root=tmp_path, + ) + + +def test_args_does_not_require_a_commit_msg_file_in_pr_range_mode(tmp_path: pathlib.Path) -> None: + args = gate.CommitCitationArgs( + mode="pr-range", + commit_msg_file=None, + owner="", + repo="", + title=None, + body=None, + base_ref=None, + head_ref="HEAD", + root=tmp_path, + ) + assert args.commit_msg_file is None + + +def test_args_rejects_a_root_that_is_not_a_directory(tmp_path: pathlib.Path) -> None: + with pytest.raises(gate.ValidationError, match="must be an existing directory"): + gate.CommitCitationArgs( + mode="pr-range", + commit_msg_file=None, + owner="", + repo="", + title=None, + body=None, + base_ref=None, + head_ref="HEAD", + root=tmp_path / "does-not-exist", + ) + + +def test_args_rejects_an_invalid_mode(tmp_path: pathlib.Path) -> None: + with pytest.raises(gate.ValidationError): + gate.CommitCitationArgs( + mode="everything", # type: ignore[arg-type] + commit_msg_file=None, + owner="", + repo="", + title=None, + body=None, + base_ref=None, + head_ref="HEAD", + root=tmp_path, + ) + + +def test_root_must_exist_rejects_a_file_path_directly(tmp_path: pathlib.Path) -> None: + # Calls the pydantic validator directly (not only through CommitCitationArgs + # construction above), so a not-a-directory root is rejected even when it + # names an existing file rather than a missing path. + not_a_dir = tmp_path / "a-file.txt" + not_a_dir.write_text("x", encoding="utf-8") + with pytest.raises(ValueError, match="must be an existing directory"): + gate.CommitCitationArgs._root_must_exist(not_a_dir) + + +def test_root_must_exist_accepts_a_real_directory(tmp_path: pathlib.Path) -> None: + assert gate.CommitCitationArgs._root_must_exist(tmp_path) == tmp_path + + +def test_commit_msg_file_required_in_commit_msg_mode_direct_call(tmp_path: pathlib.Path) -> None: + # Calls the model_validator directly against an already-constructed + # instance -- pydantic normally runs it during __init__ (asserted via + # CommitCitationArgs above), but this exercises the method itself. + pr_range_args = gate.CommitCitationArgs( + mode="pr-range", + commit_msg_file=None, + owner="", + repo="", + title=None, + body=None, + base_ref=None, + head_ref="HEAD", + root=tmp_path, + ) + # pydantic wraps a `@model_validator` method in a descriptor proxy mypy's + # stubs do not model as callable on an instance -- a real runtime call + # (pydantic's own `__get__` returns the bound method), not a static-typing + # gap in the call itself. + assert pr_range_args._commit_msg_file_required_in_commit_msg_mode() is pr_range_args # type: ignore[operator] + + +# --- main(): --mode commit-msg ------------------------------------------------ + + +def test_main_commit_msg_passes_on_a_cited_message(tmp_path: pathlib.Path) -> None: + msg_file = tmp_path / "COMMIT_EDITMSG" + msg_file.write_text("fix: correct the bug\n\nCloses #42\n", encoding="utf-8") + assert gate.main(["--mode", "commit-msg", str(msg_file)]) == 0 + + +def test_main_commit_msg_fails_on_an_uncited_message( + tmp_path: pathlib.Path, capsys: pytest.CaptureFixture[str] +) -> None: + msg_file = tmp_path / "COMMIT_EDITMSG" + msg_file.write_text("chore: tidy up formatting\n", encoding="utf-8") + assert gate.main(["--mode", "commit-msg", str(msg_file)]) == 1 + assert "FAIL" in capsys.readouterr().err + + +def test_main_commit_msg_a_fenced_citation_still_fails(tmp_path: pathlib.Path) -> None: + msg_file = tmp_path / "COMMIT_EDITMSG" + msg_file.write_text("docs: explain it\n\n```\nCloses #123\n```\n", encoding="utf-8") + assert gate.main(["--mode", "commit-msg", str(msg_file)]) == 1 + + +# --- issue #1212 adversarial review: the raw-COMMIT_EDITMSG false PASS ------ + + +def test_truncate_at_scissors_cuts_the_line_itself_and_everything_below() -> None: + text = f"fix: real subject\n\n# {gate.SCISSORS_MARKER}\n# ignored\ndiff --git a/x b/x\n+#42\n" + assert gate.truncate_at_scissors(text) == "fix: real subject\n\n" + + +def test_truncate_at_scissors_leaves_an_ordinary_message_untouched() -> None: + text = "fix: real subject\n\nCloses #42\n" + assert gate.truncate_at_scissors(text) == text + + +def test_clean_commit_message_strips_git_comments_and_the_scissors_diff(tmp_path: pathlib.Path) -> None: + root = _init_repo(tmp_path / "repo") + cleaned = gate.clean_commit_message(root, _raw_hook_commit_editmsg("chore: tidy up formatting", "See #1212")) + assert cleaned.strip() == "chore: tidy up formatting" + + +def test_clean_commit_message_honors_a_custom_core_comment_char(tmp_path: pathlib.Path) -> None: + """`git stripspace` resolves core.commentChar itself, which is exactly + why the comment strip is delegated to git rather than hardcoding `#`: + a repository configuring `;` would defeat a hardcoded strip outright.""" + root = _init_repo(tmp_path / "repo-semicolon") + _run(["git", "config", "core.commentChar", ";"], root) + raw = "chore: tidy up formatting\n\n; Please enter the commit message ; Refs #1212\n" + assert gate.clean_commit_message(root, raw).strip() == "chore: tidy up formatting" + + +def test_clean_commit_message_raises_rather_than_falling_back_when_stripspace_fails( + tmp_path: pathlib.Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A `git stripspace` that cannot run must be exit-2 "the check could + not be trusted", never a silent fallback to the *unstripped* text -- + that fallback would restore the false PASS below, invisibly, on + exactly the broken-environment path nobody is watching.""" + + class _Failed: + returncode = 1 + stdout = "" + stderr = "git: 'stripspace' is not a git command" + + monkeypatch.setattr(gate._gitapex_base_ref, "run_git", lambda *_a, **_k: _Failed()) + with pytest.raises(gate.CitationGateError, match="git stripspace --strip-comments failed"): + gate.clean_commit_message(tmp_path, "chore: tidy\n") + + +def test_main_commit_msg_a_citation_only_in_the_staged_diff_below_scissors_still_fails( + tmp_path: pathlib.Path, capsys: pytest.CaptureFixture[str] +) -> None: + """The exact false-PASS issue #1212's own adversarial review + reproduced live: with `commit.verbose=true`, the file git hands a + `commit-msg` hook still carries the whole staged diff below the + scissors line, and a *source file* containing a citation-shaped + comment (`# See issue #1212 for the rationale.`) made this gate report + PASS for a commit whose actually-stored message was the uncited + `chore: tidy up formatting`. Git strips comments and the scissors + block only *after* the hook returns, so the gate has to do it itself. + Confirmed to have teeth: reverting `_run_commit_msg` to check the raw + text makes this test PASS the gate (exit 0) again.""" + msg_file = tmp_path / "COMMIT_EDITMSG" + msg_file.write_text( + _raw_hook_commit_editmsg("chore: tidy up formatting", "See issue #1212 for the rationale."), + encoding="utf-8", + ) + assert gate.main(["--mode", "commit-msg", str(msg_file), "--root", str(tmp_path)]) == 1 + assert "FAIL" in capsys.readouterr().err + + +def test_main_commit_msg_a_real_citation_survives_the_cleaning(tmp_path: pathlib.Path) -> None: + """The other half of the same fix: cleaning must not eat a genuine + citation sitting in the real message, above git's own comment block.""" + msg_file = tmp_path / "COMMIT_EDITMSG" + msg_file.write_text( + _raw_hook_commit_editmsg("fix: correct the bug\n\nCloses #42", "unrelated prose"), encoding="utf-8" + ) + assert gate.main(["--mode", "commit-msg", str(msg_file), "--root", str(tmp_path)]) == 0 + + +def test_main_commit_msg_a_citation_only_inside_gits_own_comment_block_still_fails(tmp_path: pathlib.Path) -> None: + """The no-scissors half of the same class: a `commit.template` (or any + `#`-prefixed guidance line) documenting the convention as `Refs #123` + is a comment git will discard, not a citation.""" + msg_file = tmp_path / "COMMIT_EDITMSG" + msg_file.write_text("chore: tidy up formatting\n\n# Cite the issue, e.g. Refs #123\n", encoding="utf-8") + assert gate.main(["--mode", "commit-msg", str(msg_file), "--root", str(tmp_path)]) == 1 + + +@pytest.mark.slow +def test_commit_msg_hook_receives_the_raw_uncleaned_file_from_real_git(tmp_path: pathlib.Path) -> None: + """Live end-to-end proof against real git rather than a hand-built + fixture: a real repo, a real `.git/hooks/commit-msg`, `commit.verbose + = true`, a staged file whose own content carries a citation-shaped + line, and an uncited subject. Asserts both halves of the defect -- + that the file the hook receives really does still contain the + uncleaned comment block, scissors line, and staged diff, and that the + commit git actually stored cites nothing -- then runs the real gate + over that captured file and requires a FAIL.""" + root = _init_repo(tmp_path / "live") + _run(["git", "config", "commit.verbose", "true"], root) + captured = tmp_path / "captured.txt" + hook = root / ".git" / "hooks" / "commit-msg" + hook.write_text(f'#!/bin/sh\ncp "$1" {captured}\nexit 0\n', encoding="utf-8") + hook.chmod(0o755) + + # A non-interactive stand-in for the contributor's editor: it prepends an + # uncited subject and leaves everything git itself prepared below it + # untouched, which is exactly what a real editor session produces. + editor = tmp_path / "editor.sh" + editor.write_text( + '#!/bin/sh\nprintf \'chore: tidy up formatting\\n%s\' "$(cat "$1")" > "$1.new"\nmv "$1.new" "$1"\n', + encoding="utf-8", + ) + editor.chmod(0o755) + + (root / "mod.py").write_text("def f():\n # See issue #1212 for the rationale.\n return 1\n", encoding="utf-8") + _run(["git", "add", "--", "mod.py"], root) + # GIT_EDITOR in the environment, not `git config core.editor`: the env var + # wins over the config key, and some environments (this repository's own + # container among them) already export GIT_EDITOR=true, which would + # silently skip the editor entirely and abort on an empty message. + subprocess.run( + ["git", "commit", "-q"], + cwd=root, + check=True, + capture_output=True, + text=True, + env={**os.environ, "GIT_EDITOR": str(editor)}, + ) + + raw = captured.read_text(encoding="utf-8") + assert gate.SCISSORS_MARKER in raw # git had NOT yet stripped the scissors block + assert "See issue #1212" in raw # the staged diff's own citation-shaped line is present + stored = _run(["git", "log", "-1", "--format=%B"], root).stdout + assert "#1212" not in stored # ...but the commit git really stored cites nothing + + assert gate.main(["--mode", "commit-msg", str(captured), "--root", str(root)]) == 1 + + +# --- issue #1212 adversarial review: the two layers must agree on merges ---- + + +@pytest.mark.slow +def test_merge_in_progress_is_false_on_an_ordinary_checkout(tmp_path: pathlib.Path) -> None: + root = _init_repo(tmp_path / "ordinary") + _commit(root, "a.txt", "chore: init") + assert gate.merge_in_progress(root) is False + + +@pytest.mark.slow +def test_main_commit_msg_exempts_a_merge_commit_matching_ci_no_merges(tmp_path: pathlib.Path) -> None: + """`--mode pr-range` exempts merge commits via `git log --no-merges` + (this issue's own stated non-goal). `--mode commit-msg` must reach the + same verdict, or the two layers disagree and every ordinary `git + merge` is rejected locally -- live-reproduced before this fix, with + git left mid-merge ("Not committing merge; use 'git commit' to + complete the merge"), which breaks this repository's own documented + `git pull --no-rebase` shared-branch workflow. + + Left genuinely mid-merge here, with a real `MERGE_HEAD`, rather than + faking the state: the merge is started with `--no-commit` so the gate + runs against the same repository state a real `commit-msg` hook sees.""" + root = _init_repo(tmp_path / "merging") + _commit(root, "a.txt", "chore: base (Refs #1)") + _checkout_new_branch(root, "side") + _commit(root, "s.txt", "feat: side (Refs #2)") + _run(["git", "checkout", "-q", "main"], root) + _commit(root, "m.txt", "chore: main moves on (Refs #3)") + _run(["git", "merge", "-q", "--no-ff", "--no-commit", "side"], root) + assert gate.merge_in_progress(root) is True + + # git's own default merge message, which cites nothing. + msg_file = tmp_path / "MERGE_MSG" + msg_file.write_text("Merge branch 'side'\n", encoding="utf-8") + assert gate.main(["--mode", "commit-msg", str(msg_file), "--root", str(root)]) == 0 + + +@pytest.mark.slow +def test_main_commit_msg_still_gates_an_ordinary_commit_in_the_same_repo(tmp_path: pathlib.Path) -> None: + """The guard on the exemption above: once the merge is concluded the + same repo gates an uncited ordinary commit again, so the exemption is + scoped to a real in-progress merge rather than to the repository.""" + root = _init_repo(tmp_path / "merged-then-ordinary") + _commit(root, "a.txt", "chore: base (Refs #1)") + _checkout_new_branch(root, "side") + _commit(root, "s.txt", "feat: side (Refs #2)") + _run(["git", "checkout", "-q", "main"], root) + _commit(root, "m.txt", "chore: main moves on (Refs #3)") + _merge_no_ff(root, "side", "Merge branch 'side'") + assert gate.merge_in_progress(root) is False + + msg_file = tmp_path / "COMMIT_EDITMSG" + msg_file.write_text("chore: tidy up formatting\n", encoding="utf-8") + assert gate.main(["--mode", "commit-msg", str(msg_file), "--root", str(root)]) == 1 + + +@pytest.mark.slow +def test_a_real_git_merge_succeeds_with_the_gate_installed_as_a_real_hook(tmp_path: pathlib.Path) -> None: + """End-to-end against real git with the real script wired in as a real + `.git/hooks/commit-msg`: the exact command that failed before this fix + (`git merge --no-ff`, git's own default merge message) must now + complete, while an uncited ordinary commit in the same repo is still + rejected by the same installed hook.""" + root = _init_repo(tmp_path / "real-hook") + hook = root / ".git" / "hooks" / "commit-msg" + script = pathlib.Path(gate.__file__).resolve() + hook.write_text(f'#!/bin/sh\nexec {sys.executable} {script} --mode commit-msg "$1" --root {root}\n', "utf-8") + hook.chmod(0o755) + + _commit(root, "a.txt", "chore: base (Refs #1)") + _checkout_new_branch(root, "side") + _commit(root, "s.txt", "feat: side (Refs #2)") + _run(["git", "checkout", "-q", "main"], root) + _commit(root, "m.txt", "chore: main moves on (Refs #3)") + + _run(["git", "merge", "--no-ff", "-m", "Merge branch 'side'", "side"], root) + assert _run(["git", "rev-list", "--count", "--merges", "HEAD"], root).stdout.strip() == "1" + + (root / "z.txt").write_text("z\n", encoding="utf-8") + _run(["git", "add", "--", "z.txt"], root) + rejected = subprocess.run( + ["git", "commit", "-q", "-m", "chore: tidy up formatting"], cwd=root, capture_output=True, text=True + ) + assert rejected.returncode != 0 + assert "cites no issue" in rejected.stdout + rejected.stderr + + +def test_main_commit_msg_without_a_file_argument_exits_two(capsys: pytest.CaptureFixture[str]) -> None: + assert gate.main(["--mode", "commit-msg"]) == 2 + assert "invalid CLI arguments" in capsys.readouterr().err + + +def test_main_commit_msg_missing_file_exits_two(tmp_path: pathlib.Path, capsys: pytest.CaptureFixture[str]) -> None: + assert gate.main(["--mode", "commit-msg", str(tmp_path / "does-not-exist")]) == 2 + assert "not found" in capsys.readouterr().err + + +def test_main_commit_msg_non_utf8_file_exits_two(tmp_path: pathlib.Path, capsys: pytest.CaptureFixture[str]) -> None: + msg_file = tmp_path / "COMMIT_EDITMSG" + msg_file.write_bytes(b"\xff\xfe not valid utf-8") + assert gate.main(["--mode", "commit-msg", str(msg_file)]) == 2 + assert "not valid UTF-8" in capsys.readouterr().err + + +def test_extract_citations_or_raise_direct_call_passes_through_a_real_citation() -> None: + resolving, context = gate._extract_citations_or_raise(None, None, None, "Closes #1212") + assert resolving == (1212,) + assert context == () + + +def test_extract_citations_or_raise_direct_call_converts_value_error() -> None: + huge_digit_run = "9" * 5000 + with pytest.raises(gate.CitationGateError, match="could not parse a citation number"): + gate._extract_citations_or_raise(None, None, None, f"Closes #{huge_digit_run}") + + +def test_check_commit_message_an_implausibly_long_digit_run_raises_not_crashes() -> None: + """Dimension 15 (`skills/evaluating-deterministic-gate-quality`): before + this fix, a citation-shaped `#` string made + `extract_citations`' own `int(n)` call raise an uncaught `ValueError` + (Python's default integer-string-conversion digit limit, 4300) -- + escaping as exit 1, the code this module reserves for a *confirmed* + no-citation FAIL, not a broken/adversarial input.""" + huge_digit_run = "9" * 5000 + with pytest.raises(gate.CitationGateError, match="could not parse a citation number"): + gate.check_commit_message(f"Closes #{huge_digit_run}") + + +def test_main_commit_msg_an_implausibly_long_digit_run_exits_two( + tmp_path: pathlib.Path, capsys: pytest.CaptureFixture[str] +) -> None: + huge_digit_run = "9" * 5000 + msg_file = tmp_path / "COMMIT_EDITMSG" + msg_file.write_text(f"fix: bug\n\nCloses #{huge_digit_run}\n", encoding="utf-8") + assert gate.main(["--mode", "commit-msg", str(msg_file)]) == 2 + assert "could not parse a citation number" in capsys.readouterr().err + + +def test_main_pr_range_an_implausibly_long_digit_run_in_the_body_exits_two( + tmp_path: pathlib.Path, capsys: pytest.CaptureFixture[str] +) -> None: + huge_digit_run = "9" * 5000 + body_file = tmp_path / "body.txt" + body_file.write_text(f"Closes #{huge_digit_run}\n", encoding="utf-8") + exit_code = gate.main( + ["--mode", "pr-range", "--root", str(tmp_path), "--body", str(body_file), "--head-ref", "HEAD"] + ) + assert exit_code == 2 + assert "could not parse a citation number" in capsys.readouterr().err + + +# --- main(): --mode pr-range -------------------------------------------------- + + +def test_main_pr_range_passes_on_a_cited_body(tmp_path: pathlib.Path) -> None: + body_file = tmp_path / "body.txt" + body_file.write_text("Closes #99", encoding="utf-8") + assert gate.main(["--mode", "pr-range", "--owner", "tvna", "--repo", "gitapex", "--body", str(body_file)]) == 0 + + +def test_main_pr_range_fails_when_the_body_file_is_missing( + tmp_path: pathlib.Path, capsys: pytest.CaptureFixture[str] +) -> None: + assert gate.main(["--mode", "pr-range", "--body", str(tmp_path / "nope.txt")]) == 2 + assert "not found" in capsys.readouterr().err + + +def test_main_pr_range_fails_when_the_title_file_is_not_utf8( + tmp_path: pathlib.Path, capsys: pytest.CaptureFixture[str] +) -> None: + title_file = tmp_path / "title.txt" + title_file.write_bytes(b"\xff\xfe not valid utf-8") + assert gate.main(["--mode", "pr-range", "--title", str(title_file)]) == 2 + assert "not valid UTF-8" in capsys.readouterr().err + + +@pytest.mark.slow +def test_main_pr_range_end_to_end_against_a_real_repo_with_no_pr_text(tmp_path: pathlib.Path) -> None: + root, base_sha, head_sha = _build_range_repo(tmp_path, citing_commit=True) + assert ( + gate.main( + [ + "--mode", + "pr-range", + "--owner", + "tvna", + "--repo", + "gitapex", + "--base-ref", + base_sha, + "--head-ref", + head_sha, + "--root", + str(root), + ] + ) + == 0 + ) + + +@pytest.mark.slow +def test_main_pr_range_end_to_end_fails_with_no_citation_anywhere( + tmp_path: pathlib.Path, capsys: pytest.CaptureFixture[str] +) -> None: + root, base_sha, head_sha = _build_range_repo(tmp_path, citing_commit=False) + exit_code = gate.main( + [ + "--mode", + "pr-range", + "--base-ref", + base_sha, + "--head-ref", + head_sha, + "--root", + str(root), + ] + ) + assert exit_code == 1 + assert "FAIL" in capsys.readouterr().err + + +# --- issue #1212 adversarial review: "nothing to check" is not a FAIL ------- + + +def _empty_range_repo(tmp_path: pathlib.Path) -> tuple[pathlib.Path, str]: + """A repo whose `..HEAD` range is genuinely empty -- the state a + checkout is in right after a fast-forward, or on a branch whose own + commits are all already merged into `main`.""" + root = _init_repo(tmp_path / "empty-range") + head = _commit(root, "a.txt", "chore: init") + return root, head + + +@pytest.mark.slow +def test_evaluate_pr_range_an_empty_range_with_no_pr_text_supplied_is_not_a_failure( + tmp_path: pathlib.Path, +) -> None: + """`.gitapex/ssot.json`'s own `local_invocation` runs `--mode pr-range` + with neither `--title` nor `--body`, and feeds the `pre-push` hook's + `local-preflight` runner, which reads any non-zero exit as a blocked + push. An empty range there has no commit and no PR text to evaluate at + all, so there is nothing that *could* carry a citation -- reporting it + as "you cited nothing" (live-reproduced as exit 1, issue #1212's own + adversarial review) blocks a push over a non-violation.""" + root, head = _empty_range_repo(tmp_path) + passed, message = gate.evaluate_pr_range(root, "", "", "", "", head, head, pr_text_supplied=False) + assert passed is True + assert "nothing to check" in message + + +@pytest.mark.slow +def test_evaluate_pr_range_an_empty_range_still_fails_when_pr_text_was_supplied(tmp_path: pathlib.Path) -> None: + """The guard on the fix above: `pr_text_supplied` tracks whether the + *flags were passed*, never whether their text is non-empty, so CI -- + which always passes both -- keeps its previous verdict even for an + empty range. This is the branch that keeps the fix from becoming a + blanket "empty range always passes".""" + root, head = _empty_range_repo(tmp_path) + passed, message = gate.evaluate_pr_range(root, "", "", "", "", head, head, pr_text_supplied=True) + assert passed is False + assert "neither the PR title/body nor any non-merge commit" in message + + +@pytest.mark.slow +def test_evaluate_pr_range_defaults_to_the_strict_pr_text_supplied(tmp_path: pathlib.Path) -> None: + """Fail-closed default: a caller that omits `pr_text_supplied` + entirely gets the strict verdict, not the lenient one.""" + root, head = _empty_range_repo(tmp_path) + passed, _ = gate.evaluate_pr_range(root, "", "", "", "", head, head) + assert passed is False + + +@pytest.mark.slow +def test_main_pr_range_empty_range_with_no_title_or_body_exits_zero( + tmp_path: pathlib.Path, capsys: pytest.CaptureFixture[str] +) -> None: + """End-to-end through `main`, in the exact `local_invocation` argv + shape: no `--title`, no `--body`, empty range -> exit 0, with a + message that names "nothing to check" rather than claiming a citation + was missing.""" + root, head = _empty_range_repo(tmp_path) + exit_code = gate.main(["--mode", "pr-range", "--base-ref", head, "--head-ref", head, "--root", str(root)]) + assert exit_code == 0 + assert "nothing to check" in capsys.readouterr().out + + +@pytest.mark.slow +def test_main_pr_range_empty_range_with_an_uncited_body_file_still_exits_one( + tmp_path: pathlib.Path, capsys: pytest.CaptureFixture[str] +) -> None: + """The CI-shaped guard, end to end: the same empty range, but with a + `--body` file supplied that cites nothing, still FAILs.""" + root, head = _empty_range_repo(tmp_path) + body_file = tmp_path / "body.txt" + body_file.write_text("no citation in this body", encoding="utf-8") + exit_code = gate.main( + [ + "--mode", + "pr-range", + "--base-ref", + head, + "--head-ref", + head, + "--root", + str(root), + "--body", + str(body_file), + ] + ) + assert exit_code == 1 + assert "FAIL" in capsys.readouterr().err + + +# --- issue #1212 adversarial review: dimension-15 fail-closed input handling - + + +@pytest.mark.parametrize("mode_argv", [["--mode", "commit-msg"], ["--mode", "pr-range", "--body"]]) +def test_main_a_path_argument_naming_a_directory_exits_two( + mode_argv: list[str], tmp_path: pathlib.Path, capsys: pytest.CaptureFixture[str] +) -> None: + """Dimension 15 (`skills/evaluating-deterministic-gate-quality`): + before this fix `_read_input_file` caught only `FileNotFoundError` and + `UnicodeDecodeError`, so pointing any path flag at a *directory* + raised `IsADirectoryError` straight out of `main` -- an uncaught + traceback whose Python exit code is 1, the very code this module + reserves for a *confirmed* no-citation policy FAIL. A broken + invocation reported itself as a real citation violation.""" + a_directory = tmp_path / "adir" + a_directory.mkdir() + assert gate.main([*mode_argv, str(a_directory)]) == 2 + assert "could not be read" in capsys.readouterr().err + + +def test_main_commit_msg_an_unreadable_file_exits_two( + tmp_path: pathlib.Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + """The same dimension-15 arm reached through `PermissionError` rather + than `IsADirectoryError` -- a file that exists and is valid UTF-8 but + that this process cannot open at all. Raised through a monkeypatched + `read_text` rather than a real `chmod(0o000)`: this repository's own + container runs the suite as uid 0, where the mode bits are bypassed + and the read simply succeeds, so a chmod-based fixture would assert + nothing here while passing on a non-root CI runner.""" + + def _deny(*_args: object, **_kwargs: object) -> str: + raise PermissionError(13, "Permission denied") + + msg_file = tmp_path / "COMMIT_EDITMSG" + msg_file.write_text("chore: tidy up formatting\n", encoding="utf-8") + monkeypatch.setattr(pathlib.Path, "read_text", _deny) + assert gate.main(["--mode", "commit-msg", str(msg_file)]) == 2 + assert "could not be read" in capsys.readouterr().err + + +def test_read_input_file_direct_call_returns_empty_text_for_none() -> None: + assert gate._read_input_file(None) == "" + + +def test_read_input_file_direct_call_reads_a_real_file(tmp_path: pathlib.Path) -> None: + path = tmp_path / "body.txt" + path.write_text("Closes #1212\n", encoding="utf-8") + assert gate._read_input_file(str(path), label="pr body") == "Closes #1212\n" + + +def test_read_input_file_direct_call_raises_on_a_directory(tmp_path: pathlib.Path) -> None: + with pytest.raises(gate.CitationGateError, match="could not be read"): + gate._read_input_file(str(tmp_path), label="pr body") + + +def test_run_commit_msg_direct_call_passes_on_a_cited_message(tmp_path: pathlib.Path) -> None: + msg_file = tmp_path / "COMMIT_EDITMSG" + msg_file.write_text("fix: correct the bug\n\nCloses #1212\n", encoding="utf-8") + args = gate.CommitCitationArgs( + mode="commit-msg", + commit_msg_file=str(msg_file), + owner="", + repo="", + title=None, + body=None, + base_ref=None, + head_ref="HEAD", + root=tmp_path, + ) + assert gate._run_commit_msg(args) == 0 + + +def test_run_commit_msg_direct_call_fails_on_an_uncited_message(tmp_path: pathlib.Path) -> None: + msg_file = tmp_path / "COMMIT_EDITMSG" + msg_file.write_text("chore: tidy up formatting\n", encoding="utf-8") + args = gate.CommitCitationArgs( + mode="commit-msg", + commit_msg_file=str(msg_file), + owner="", + repo="", + title=None, + body=None, + base_ref=None, + head_ref="HEAD", + root=tmp_path, + ) + assert gate._run_commit_msg(args) == 1 + + +def test_run_pr_range_direct_call_passes_on_a_cited_body(tmp_path: pathlib.Path) -> None: + body_file = tmp_path / "body.txt" + body_file.write_text("Closes #1212\n", encoding="utf-8") + args = gate.CommitCitationArgs( + mode="pr-range", + commit_msg_file=None, + owner="tvna", + repo="gitapex", + title=None, + body=str(body_file), + base_ref="HEAD", + head_ref="HEAD", + root=tmp_path, + ) + assert gate._run_pr_range(args) == 0 + + +def test_run_pr_range_direct_call_fails_with_no_citation_anywhere(tmp_path: pathlib.Path) -> None: + root, base_sha, head_sha = _build_range_repo(tmp_path, citing_commit=False) + args = gate.CommitCitationArgs( + mode="pr-range", + commit_msg_file=None, + owner="", + repo="", + title=None, + body=None, + base_ref=base_sha, + head_ref=head_sha, + root=root, + ) + # A genuinely non-empty, genuinely uncited commit range: neither + # --title nor --body was passed (pr_text_supplied defaults to True per + # evaluate_pr_range's own strict default), so this is the ordinary + # policy FAIL, not the empty-range "nothing to check" PASS. + assert gate._run_pr_range(args) == 1 + + +def test_main_pr_range_reports_a_base_ref_resolution_failure_as_exit_two( + tmp_path: pathlib.Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + def _raise(*_args: object, **_kwargs: object) -> str: + raise gate.CitationGateError("could not resolve origin/main") + + monkeypatch.setattr(gate, "resolve_base_ref", _raise) + exit_code = gate.main(["--mode", "pr-range", "--root", str(tmp_path)]) + assert exit_code == 2 + assert "could not resolve origin/main" in capsys.readouterr().err + + +def test_main_exits_two_when_args_fail_validation( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + def _raise(*_args: object, **_kwargs: object) -> None: + raise make_validation_error() + + monkeypatch.setattr(gate, "CommitCitationArgs", _raise) + assert gate.main(["--mode", "pr-range"]) == 2 + assert "invalid CLI arguments" in capsys.readouterr().err + + +def test_main_mode_is_required() -> None: + with pytest.raises(SystemExit): + gate.main([]) + with pytest.raises(SystemExit): + gate.main(["--mode", "everything"]) diff --git a/tests/test_gitapex_gate_commit_citation_properties.py b/tests/test_gitapex_gate_commit_citation_properties.py new file mode 100644 index 00000000..fdb851de --- /dev/null +++ b/tests/test_gitapex_gate_commit_citation_properties.py @@ -0,0 +1,147 @@ +"""Hypothesis property-based layer for +``.github/scripts/gitapex_gate_commit_citation.py`` (issue #1212, closing +issue #1178's own ``detection-logic-property-coverage`` gap for this new +module's ``REPO_ROOT``/``sys.path`` module-level ``.resolve()`` calls and its +``check_commit_message``/``check_pr_text`` citation-detection wrappers). + +These wrappers carry no detection logic of their own beyond the call to +``extract_citations`` (``hooks/gitapex_check_pr_issue_acm_disclosure.py``, +reused rather than reimplemented -- see this module's own docstring); the +properties below confirm the *integration* holds across generated input +(a citation form is still recognized once wrapped, fence-stripping is not +bypassed by the wrapping, arbitrary text never raises), not the citation +regex's own vocabulary, which is that module's own concern. + +Reproducibility: ``derandomize=True`` with an explicit ``max_examples`` and +``deadline=None``, matching +``tests/test_gitapex_check_pr_duplicate_issue_properties.py``'s own +established rationale (this repository runs pytest under ``pytest-xdist``, +where a randomly-seeded generator turns a latent failure into an +intermittently red suite that reruns green). +""" + +from __future__ import annotations + +import gitapex_gate_commit_citation as gate +from hypothesis import given, settings +from hypothesis import strategies as st + +_PROPERTIES = settings(derandomize=True, max_examples=200, deadline=None) + +# Every citation form check_commit_message/check_pr_text must recognize +# (extract_citations' own resolving and context-only vocabulary), each as a +# format string taking one issue number. +_CITATION_TEMPLATES = ("Closes #{n}", "Fixes #{n}", "Resolves #{n}", "Refs #{n}", "#{n}") + +# Free text with no digit at all, so it can never accidentally contain a +# `#N`-shaped citation of its own -- used as filler/surrounding prose in the +# properties below. +# +# `~` is excluded for the same reason `` ` `` already was, and this is not a +# hypothetical (issue #1212's own adversarial review found it live, as an +# intermittent failure of +# test_any_citation_form_is_recognized_in_pr_title_or_body): extract_citations +# strips *both* fence syntaxes, ``` and ~~~, so filler free to contain `~` +# lets Hypothesis generate prefix='~~~'/suffix='~~~' and wrap the citation +# these properties inject in a genuine ~~~ fenced block. The citation is then +# correctly *not* detected, and the recognition property fails against wholly +# correct behavior. Excluding the backtick alone closed only half of that. +_NO_DIGIT_TEXT = st.text( + alphabet=st.characters(blacklist_categories=("Cc", "Cs"), blacklist_characters="#0123456789`~"), + max_size=60, +) + + +@_PROPERTIES +@given( + template=st.sampled_from(_CITATION_TEMPLATES), + number=st.integers(min_value=1, max_value=999999), + prefix=_NO_DIGIT_TEXT, + suffix=_NO_DIGIT_TEXT, +) +def test_any_citation_form_is_recognized_in_a_commit_message( + template: str, number: int, prefix: str, suffix: str +) -> None: + """Every citation form this repository's own convention documents + (CONTRIBUTING.md's "Issue citation convention") is recognized by + check_commit_message, wherever it sits inside otherwise-arbitrary + surrounding text -- not only the one or two hand-picked examples in + tests/test_gitapex_gate_commit_citation.py.""" + message = f"{prefix}\n\n{template.format(n=number)}\n{suffix}" + assert gate.check_commit_message(message) is True + + +@_PROPERTIES +@given( + template=st.sampled_from(_CITATION_TEMPLATES), + number=st.integers(min_value=1, max_value=999999), + prefix=_NO_DIGIT_TEXT, + suffix=_NO_DIGIT_TEXT, +) +def test_any_citation_form_is_recognized_in_pr_title_or_body( + template: str, number: int, prefix: str, suffix: str +) -> None: + """The same recognition property, through check_pr_text's own title/body + pair -- confirms the wrapper does not silently narrow what + extract_citations already accepts.""" + citation = template.format(n=number) + assert gate.check_pr_text("tvna", "gitapex", f"{prefix} {citation}", "") is True + assert gate.check_pr_text("tvna", "gitapex", "", f"{prefix}\n{citation}\n{suffix}") is True + + +@_PROPERTIES +@given( + template=st.sampled_from(_CITATION_TEMPLATES), + number=st.integers(min_value=1, max_value=999999), +) +def test_a_citation_inside_a_fenced_code_block_is_never_recognized(template: str, number: int) -> None: + """Containment: a citation form shown only inside a fenced code block + (an illustrative example of this repository's own citation syntax, the + exact false-positive class hooks/gitapex_check_pr_issue_acm_disclosure.py's + own docstring names as found live against its own PR body) is never + misdetected as a real citation -- across every generated citation form + and issue number, not only the one hand-picked example in + tests/test_gitapex_gate_commit_citation.py. + + Confirmed to have teeth: passing the raw, un-stripped text straight to + a bare ``#\\d+`` search instead of through extract_citations makes this + property FAIL on every generated example, since the fenced line still + contains a real citation-shaped match.""" + message = f"docs: explain the citation syntax\n\n```\n{template.format(n=number)}\n```\n" + assert gate.check_commit_message(message) is False + + +@_PROPERTIES +@given(text=st.text(max_size=300)) +def test_check_commit_message_never_raises_and_is_deterministic(text: str) -> None: + """Robustness: arbitrary text (including stray `#`, backticks, or + partial keyword fragments) produces a result rather than an exception, + and the same input produces the same output -- this function runs + inside a git commit-msg hook, where an uncaught exception blocks every + commit rather than reporting one FAIL.""" + first = gate.check_commit_message(text) + second = gate.check_commit_message(text) + assert first == second + assert isinstance(first, bool) + + +@_PROPERTIES +@given(title=st.text(max_size=150), body=st.text(max_size=300)) +def test_check_pr_text_never_raises_and_is_deterministic(title: str, body: str) -> None: + """The same robustness property for check_pr_text's own title/body + pair -- this function runs inside the CI backstop, where an uncaught + exception fails the job with a traceback rather than a clear FAIL.""" + first = gate.check_pr_text("tvna", "gitapex", title, body) + second = gate.check_pr_text("tvna", "gitapex", title, body) + assert first == second + assert isinstance(first, bool) + + +@_PROPERTIES +@given(text=_NO_DIGIT_TEXT) +def test_text_never_containing_a_hash_is_never_recognized_as_a_citation(text: str) -> None: + """No false positive: text that can never contain a `#`-shaped + citation at all is never recognized as carrying one, regardless of + what other punctuation or structure it happens to contain.""" + assert gate.check_commit_message(text) is False + assert gate.check_pr_text("tvna", "gitapex", text, text) is False diff --git a/tests/test_gitapex_gate_local_preflight.py b/tests/test_gitapex_gate_local_preflight.py index 6e9bcb39..311ff3f8 100644 --- a/tests/test_gitapex_gate_local_preflight.py +++ b/tests/test_gitapex_gate_local_preflight.py @@ -5,7 +5,7 @@ - **Fixture-registry tests** build their own tiny ``ssot.json`` pointing at purpose-built pass/fail scripts, so the runner's own aggregation, discovery, error handling and exit-code logic are exercised in under a - second with no dependence on this repository's real 43 wired gates. Issue + second with no dependence on this repository's real 44 wired gates. Issue #876's first acceptance criterion asks for an integration test running the consolidated command "with one deliberately-broken instance of each wired check, asserting all are reported in one run" --