diff --git a/.gitapex/ssot.json b/.gitapex/ssot.json index be62a177..fe774a4b 100644 --- a/.gitapex/ssot.json +++ b/.gitapex/ssot.json @@ -1909,6 +1909,36 @@ {"kind": "workflow-event", "ref": "retrospective-gate-drift.yml:schedule"}, {"kind": "workflow-event", "ref": "retrospective-gate-drift.yml:workflow_dispatch"} ] + }, + { + "id": "unguarded-shell-pipe-in-docs", + "kind": "script", + "script": ".github/scripts/gitapex_gate_unguarded_shell_pipe_in_docs.py", + "rule": "No skills/*/SKILL.md, skills/*/references/*.md, or checker/gate script's own module docstring may carry an unguarded `cmd1 | cmd2`-shaped shell pipe example (a real command token, a single `|`, and one of a fixed shell-consumer vocabulary such as python3/bash/uv/jq) with no nearby `pipefail` disclosure -- either `pipefail` mentioned inside the same fenced block (Markdown) or the same module docstring (Python), or an explicit `` marker directly above the fence (Markdown) or the flagged line (Python docstring). A single-backtick or double-backtick-quoted inline example in a docstring is never in scope. Exits 2, never a silent pass, when no in-scope file is discovered in either category or a file cannot be read/decoded as UTF-8 or parsed as Python.", + "planes": ["ci", "local"], + "local_invocation": ["uv", "run", "--frozen", "python3", ".github/scripts/gitapex_gate_unguarded_shell_pipe_in_docs.py"], + "trigger": ".github/workflows/unguarded-shell-pipe-in-docs-gate.yml on pull_request and workflow_dispatch, plus tests/test_gitapex_gate_unguarded_shell_pipe_in_docs.py and tests/test_gitapex_gate_unguarded_shell_pipe_in_docs_properties.py inside the pytest step of .github/workflows/test.yml", + "policy_refs": [], + "cluster": "repo-hygiene", + "tracking_issue": [1531, 1567], + "status": "active", + "supersedes": null, + "bypass_review_status": "not-yet-reviewed", + "preconditions": { + "requires_python_packages": ["pydantic"] + }, + "target": [ + {"kind": "file-glob", "ref": "skills/*/SKILL.md"}, + {"kind": "file-glob", "ref": "skills/*/references/*.md"}, + {"kind": "file-glob", "ref": ".github/scripts/*.py"}, + {"kind": "file-glob", "ref": "skills/*/scripts/*.py"}, + {"kind": "file-glob", "ref": "evals/scripts/*.py"}, + {"kind": "file-glob", "ref": "hooks/*.py"}, + {"kind": "workflow-event", "ref": "unguarded-shell-pipe-in-docs-gate.yml:pull_request"}, + {"kind": "workflow-event", "ref": "unguarded-shell-pipe-in-docs-gate.yml:workflow_dispatch"}, + {"kind": "workflow-event", "ref": "test.yml:pull_request"}, + {"kind": "workflow-event", "ref": "test.yml:push"} + ] } ], "clusters": { diff --git a/.github/scripts/gitapex_detect_changed_gate_scripts.py b/.github/scripts/gitapex_detect_changed_gate_scripts.py index 6a169b35..b88c9fc3 100644 --- a/.github/scripts/gitapex_detect_changed_gate_scripts.py +++ b/.github/scripts/gitapex_detect_changed_gate_scripts.py @@ -121,6 +121,12 @@ git diff --name-status BASE...HEAD | uv run --frozen python3 gitapex_detect_changed_gate_scripts.py +A bare pipe here masks `git diff`'s own exit status in a non-`pipefail` +shell (issue #1531): add `set -o pipefail` first, or check `git diff`'s own +exit code separately, if the caller must detect an upstream failure rather +than silently scanning whatever partial `--name-status` output reached +stdin. + Reads `--name-status` lines on stdin, writes the comma-joined selection to stdout (empty line when nothing matched) and diagnostics to stderr, so the machine-read channel carries only the payload (dimension 14). diff --git a/.github/scripts/gitapex_detect_touched_eval_skills.py b/.github/scripts/gitapex_detect_touched_eval_skills.py index c36f7a78..5adcac08 100644 --- a/.github/scripts/gitapex_detect_touched_eval_skills.py +++ b/.github/scripts/gitapex_detect_touched_eval_skills.py @@ -125,6 +125,12 @@ python3 .github/scripts/gitapex_detect_touched_eval_skills.py \\ evals/foo/tasks/x.yaml evals/bar/eval.yaml +A bare pipe in the first form masks `git diff`'s own exit status in a +non-`pipefail` shell (issue #1531): add `set -o pipefail` first, or check +`git diff`'s own exit code separately, if the caller must detect an +upstream failure rather than silently classifying whatever partial path +list reached stdin. + `--nul`/`-0` (matching the `-0` convention of `xargs -0`/`grep -z`) reads raw NUL-delimited bytes from `sys.stdin.buffer`, decoding each piece with `os.fsdecode` (POSIX-safe, round-trips non-UTF-8-but-valid path bytes via diff --git a/.github/scripts/gitapex_extract_diff_added_lines.py b/.github/scripts/gitapex_extract_diff_added_lines.py index e0d2c0b0..1cfa97e0 100644 --- a/.github/scripts/gitapex_extract_diff_added_lines.py +++ b/.github/scripts/gitapex_extract_diff_added_lines.py @@ -83,6 +83,11 @@ git diff -U "$BASE_SHA...$HEAD_SHA" -- \\ | python3 .github/scripts/gitapex_extract_diff_added_lines.py > added_lines.txt + +A bare pipe here masks `git diff`'s own exit status in a non-`pipefail` +shell (issue #1531): add `set -o pipefail` first, or check `git diff`'s +own exit code separately, if the caller must detect an upstream failure +rather than silently extracting whatever partial diff reached stdin. """ from __future__ import annotations diff --git a/.github/scripts/gitapex_gate_acm_issue_disclosure.py b/.github/scripts/gitapex_gate_acm_issue_disclosure.py index a5ee835f..46850eef 100644 --- a/.github/scripts/gitapex_gate_acm_issue_disclosure.py +++ b/.github/scripts/gitapex_gate_acm_issue_disclosure.py @@ -80,6 +80,11 @@ uv run --frozen python3 .github/scripts/gitapex_gate_acm_issue_disclosure.py --check-only --body printf '%s' "$ISSUE_BODY" | uv run --frozen python3 .github/scripts/gitapex_gate_acm_issue_disclosure.py --check-only +A bare pipe here masks `printf`'s own exit status in a non-`pipefail` shell +(issue #1531) -- harmless for a literal `printf` producer, which cannot +itself fail in ordinary use, but add `set -o pipefail` first if this +recipe's producer is ever swapped for a command that can. + Usage (full run: check, then label/comment as needed):: uv run --frozen python3 .github/scripts/gitapex_gate_acm_issue_disclosure.py \\ diff --git a/.github/scripts/gitapex_gate_design_doc_pattern_dryrun.py b/.github/scripts/gitapex_gate_design_doc_pattern_dryrun.py index 2e973561..5ba0363f 100644 --- a/.github/scripts/gitapex_gate_design_doc_pattern_dryrun.py +++ b/.github/scripts/gitapex_gate_design_doc_pattern_dryrun.py @@ -119,6 +119,12 @@ class the module-level Residual risk paragraph above already commits --diff-added added_lines.txt [--diff-added ...] [--body PR_BODY.txt] \\ [--repo-root .] [--corpus-glob 'skills/**/*.md'] +A bare pipe in the first line masks `git diff`'s own exit status in a +non-`pipefail` shell (issue #1531): add `set -o pipefail` first, or check +`git diff`'s own exit code separately, if the caller must detect an +upstream failure rather than silently extracting whatever partial diff +reached stdin. + Exit codes: 0 No stated literal-text-search pattern found, every stated pattern has at least one live corpus match, or a disclosure marker is diff --git a/.github/scripts/gitapex_gate_detection_logic_property_coverage.py b/.github/scripts/gitapex_gate_detection_logic_property_coverage.py index c3071d06..dcd4da84 100644 --- a/.github/scripts/gitapex_gate_detection_logic_property_coverage.py +++ b/.github/scripts/gitapex_gate_detection_logic_property_coverage.py @@ -337,6 +337,11 @@ "$MERGE_BASE" "$HEAD_SHA" -- '*.py' \\ | uv run --frozen python3 .github/scripts/gitapex_gate_detection_logic_property_coverage.py +A bare pipe here masks `git diff`'s own exit status in a non-`pipefail` +shell (issue #1531): add `set -o pipefail` first, or check `git diff`'s +own exit code separately, if the caller must detect an upstream failure +rather than silently grading whatever partial diff reached stdin. + Both flags are load-bearing for the same reason they are in ``gitapex_gate_exception_handler_gaps.py``: rename detection would hide a file newly promoted into a graded directory behind a zero-added-line header, and diff --git a/.github/scripts/gitapex_gate_exception_handler_gaps.py b/.github/scripts/gitapex_gate_exception_handler_gaps.py index d16ca4ee..c2138ab4 100644 --- a/.github/scripts/gitapex_gate_exception_handler_gaps.py +++ b/.github/scripts/gitapex_gate_exception_handler_gaps.py @@ -231,6 +231,11 @@ class this gate was measured against: a read from `sys.stdin`, a write-mode "$MERGE_BASE" "$HEAD_SHA" -- '*.py' \\ | uv run --frozen python3 .github/scripts/gitapex_gate_exception_handler_gaps.py +A bare pipe here masks `git diff`'s own exit status in a non-`pipefail` +shell (issue #1531): add `set -o pipefail` first, or check `git diff`'s +own exit code separately, if the caller must detect an upstream failure +rather than silently grading whatever partial diff reached stdin. + Both flags are load-bearing, not tidiness: rename detection hides a file promoted into a graded directory behind a zero-added-line header, and `core.quotePath` renders a non-ASCII path as an escaped string this gate diff --git a/.github/scripts/gitapex_gate_function_body_test_coverage.py b/.github/scripts/gitapex_gate_function_body_test_coverage.py index 27ffd112..f0683ce3 100644 --- a/.github/scripts/gitapex_gate_function_body_test_coverage.py +++ b/.github/scripts/gitapex_gate_function_body_test_coverage.py @@ -201,6 +201,11 @@ "$MERGE_BASE" "$HEAD_SHA" -- '*.py' \\ | uv run --frozen python3 .github/scripts/gitapex_gate_function_body_test_coverage.py +A bare pipe here masks `git diff`'s own exit status in a non-`pipefail` +shell (issue #1531): add `set -o pipefail` first, or check `git diff`'s +own exit code separately, if the caller must detect an upstream failure +rather than silently grading whatever partial diff reached stdin. + Both flags are load-bearing for the same reason they are in both sibling gates: rename detection would hide a file newly promoted into a graded directory behind a zero-added-line header, and ``core.quotePath`` renders a diff --git a/.github/scripts/gitapex_gate_independent_review_pending.py b/.github/scripts/gitapex_gate_independent_review_pending.py index b5fa3bc7..972ab805 100644 --- a/.github/scripts/gitapex_gate_independent_review_pending.py +++ b/.github/scripts/gitapex_gate_independent_review_pending.py @@ -85,6 +85,11 @@ --body PR_BODY.txt --head-sha printf '%s' "$PR_BODY" | python3 .github/scripts/gitapex_gate_independent_review_pending.py --head-sha +A bare pipe here masks `printf`'s own exit status in a non-`pipefail` shell +(issue #1531) -- harmless for a literal `printf` producer, which cannot +itself fail in ordinary use, but add `set -o pipefail` first if this +recipe's producer is ever swapped for a command that can. + Exit codes: 0 A Verdict: CLEAN verdict naming the given head SHA is present. 1 No verdict section, an incomplete one, a non-CLEAN verdict, a diff --git a/.github/scripts/gitapex_gate_local_preflight.py b/.github/scripts/gitapex_gate_local_preflight.py index dd42364c..a7ae4487 100644 --- a/.github/scripts/gitapex_gate_local_preflight.py +++ b/.github/scripts/gitapex_gate_local_preflight.py @@ -107,7 +107,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 42 of the 44 wired gates. ``behind-base`` (issue #985) and + for 43 of the 45 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* @@ -119,7 +119,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 44 wired + installed crashed the whole runner on import before any of the 45 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 @@ -207,9 +207,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 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 +# call in the other direction. For scale: a warm run of all 45 wired gates +# combined measures roughly 14 s end to end (the +# prior 44-gate set measured roughly 15 s, the 43-gate set before that +# 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 diff --git a/.github/scripts/gitapex_gate_pr_title_convention.py b/.github/scripts/gitapex_gate_pr_title_convention.py index 159cec46..32fdaa53 100644 --- a/.github/scripts/gitapex_gate_pr_title_convention.py +++ b/.github/scripts/gitapex_gate_pr_title_convention.py @@ -55,6 +55,11 @@ printf '%s' "$PR_TITLE" | uv run --frozen python3 \\ .github/scripts/gitapex_gate_pr_title_convention.py +A bare pipe here masks `printf`'s own exit status in a non-`pipefail` shell +(issue #1531) -- harmless for a literal `printf` producer, which cannot +itself fail in ordinary use, but add `set -o pipefail` first if this +recipe's producer is ever swapped for a command that can. + Exit codes: 0 the title matches; 1 it does not. """ diff --git a/.github/scripts/gitapex_gate_provenance_disclosure.py b/.github/scripts/gitapex_gate_provenance_disclosure.py index 773c4c9b..f7b864c4 100644 --- a/.github/scripts/gitapex_gate_provenance_disclosure.py +++ b/.github/scripts/gitapex_gate_provenance_disclosure.py @@ -94,6 +94,11 @@ --body PR_BODY.txt --diff-added ADDED_LINES.txt [--diff-added ...] printf '%s' "$PR_BODY" | python3 .github/scripts/gitapex_gate_provenance_disclosure.py +A bare pipe here masks `printf`'s own exit status in a non-`pipefail` shell +(issue #1531) -- harmless for a literal `printf` producer, which cannot +itself fail in ordinary use, but add `set -o pipefail` first if this +recipe's producer is ever swapped for a command that can. + Exit codes: 0 No offending paragraph in the combined corpus, or a disclosure marker is present. diff --git a/.github/scripts/gitapex_gate_stdlib_only_claim_drift.py b/.github/scripts/gitapex_gate_stdlib_only_claim_drift.py index d979de5f..80eecfbd 100644 --- a/.github/scripts/gitapex_gate_stdlib_only_claim_drift.py +++ b/.github/scripts/gitapex_gate_stdlib_only_claim_drift.py @@ -60,6 +60,11 @@ git diff -U0 "$BASE_SHA...$HEAD_SHA" -- '.github/scripts/*.py' 'evals/scripts/*.py' \\ | uv run --frozen python3 .github/scripts/gitapex_gate_stdlib_only_claim_drift.py +A bare pipe here masks `git diff`'s own exit status in a non-`pipefail` +shell (issue #1531): add `set -o pipefail` first, or check `git diff`'s +own exit code separately, if the caller must detect an upstream failure +rather than silently grading whatever partial diff reached stdin. + Exit codes: 0 clean (including "no file in this diff gained a third-party import" -- a legitimate pass, not an error), 1 stale claim(s) found, 2 the scan could not be trusted (a malformed diff, an unreadable file, or a diff --git a/.github/scripts/gitapex_gate_unguarded_shell_pipe_in_docs.py b/.github/scripts/gitapex_gate_unguarded_shell_pipe_in_docs.py new file mode 100644 index 00000000..457cb674 --- /dev/null +++ b/.github/scripts/gitapex_gate_unguarded_shell_pipe_in_docs.py @@ -0,0 +1,540 @@ +#!/usr/bin/env python3 +"""CI gate: flag an unguarded `cmd1 | cmd2`-shaped shell pipe example, with no +nearby `pipefail` disclosure, in a `skills/*/SKILL.md`, a +`skills/*/references/*.md` file, or a checker/gate script's own module +docstring. + +Issue #1531 (refs #1567, gate-proposal-umbrella: local-hook fail-open +remediation). The documented invocation `git log ... | python3 +gitapex_check_task_commit_provenance.py` piped two commands directly +together; a bare shell pipeline's own exit status is the RIGHT-hand +command's, not the LEFT's, so an upstream `git log` failure (a +stale/unresolvable BASE ref, a rebase, a shallow worktree) would silently +report a clean "PASS: no commits in range" instead of a blocked merge -- +discovered only by an adversarial security-focused review, not by any +deterministic check (that specific script's own docstring now documents the +two-step, never-piped invocation instead; this gate is the durable check +that a *future* documented recipe does not reintroduce the same shape +elsewhere). + +A documentation-lint sibling to `gitapex_gate_no_raw_gh_cli_in_docs.py`, not +a runtime enforcement: it cannot verify what an agent actually types into a +shell, only flag an unsafe *documented* example for a human/agent to +notice before copying it (this issue's own stated residual risk). + +Scope +----- +Two file kinds, discovered via `git ls-files` (tracked files only, matching +`gitapex_gate_no_raw_gh_cli_in_docs.py`'s own tracked-file rationale): + +* Markdown: `skills/*/SKILL.md` and `skills/*/references/*.md`. Only text + inside a fenced code block (``` or ~~~) is scanned, using the same + CommonMark run-length fence-pairing `gitapex_gate_no_raw_gh_cli_in_docs.py` + already established (a fence closes only on a bare run of the same marker + character at least as long as the one that opened it) -- re-implemented + here rather than imported, matching this repository's own existing + precedent of one fence-pairing copy per gate (`gitapex_gate_split_fixture_ + coverage.py`, `gitapex_gate_skill_branch_fixture_coverage.py`, + `gitapex_gate_independent_review_pending.py` each already carry their own). +* Python: the module docstring only (never the rest of the file -- a CLI + help string or an inline comment quoting the same shape is out of scope, + deliberately; see "Known gaps" below) of every tracked + `.github/scripts/*.py`, `skills/*/scripts/*.py`, `evals/scripts/*.py` and + `hooks/*.py` file -- the three globs match + `gitapex_compute_skill_audit_flags.py`'s own `_CHECKER_SCRIPT_PATHSPECS` + exactly, widened here to also include `hooks/*.py` (that flag-computation + module tracks `hooks/**` separately, as a gate-membership signal rather + than a checker-script one; this gate folds both into one disclosure- + bearing script surface). + +Detection +--------- +A line matches when it carries a single `|` (not `||`) with a real command +token before it and one of a fixed, explicit consumer vocabulary +(`_PIPE_CONSUMERS` -- `python3`, `bash`, `uv`, `jq`, `grep`, `sed`, ... -- +the same never-grow-it-ad-hoc discipline `_GH_SUBCOMMANDS` uses in the +sibling gate) directly after it. This is deliberately narrower than "any +line with a pipe character": a bare non-whitespace-pipe-non-whitespace match also fires on a Python +type-hint quoted in prose (`` `list[str] | None` ``, a real, common shape in +this repository's own docstrings -- confirmed live during this gate's own +authoring, not assumed) and on an ordinary Markdown table row. Requiring a +recognized shell-consumer token on the right closes both false-positive +classes for every real instance found in this repository at authoring +time, without needing real shell tokenization -- but not in general: a +table cell (or a type-hint-like phrase) whose own literal value happens to +equal one of `_PIPE_CONSUMERS` (e.g. a table row documenting the `jq` +tool, `| Parser | jq |`) still matches, live-confirmed, since nothing here +distinguishes a table's `|` column separator from a shell pipe. Closing +that residual case needs table-syntax awareness this gate does not +implement; see "Known gaps" below. + +A match is a violation unless "nearby disclosure" is found, meaning either: + +1. `pipefail` (case-insensitive, matching the literal `pipefail`/`set -o + pipefail` vocabulary #1531 names) appears -- for Markdown, anywhere + within the *same fenced block* the match sits in (i.e. the documented + recipe already shows the guard); for a Python docstring, anywhere in + that *same module docstring* (a prose caveat elsewhere in the docstring, + not necessarily inside a code-like line, still counts -- docstrings have + no fence to scope a match to more tightly); or +2. an explicit `` + marker sits on the line directly above (no blank line in between) -- + for Markdown, the fence's own opening marker line; for a Python + docstring, the flagged line itself -- the same strict, + regex-anchored, non-empty-reason-required marker style + `gitapex_gate_no_raw_gh_cli_in_docs.py`'s own `gitapex-allow-raw-gh-cli` + marker already uses, under a distinct token so an author waiving one + check does not silently waive the other. + +Known gaps, disclosed rather than claimed closed +------------------------------------------------- +* A Python docstring match is skipped outright when the matched pipe + expression sits inside a backtick span (single `` ` `` or double `` `` ``) + on that same line -- deliberately, mirroring the sibling gate's own + distinction between an inline code span that *discusses* a pattern and a + fenced block that *instructs* running one: a docstring has no fence to + draw that line at, so a backtick-quoted illustrative example (a warning + discussing what NOT to do, or an unrelated dangerous-pattern discussion + entirely unrelated to a merge gate) is excluded the same way a Markdown + inline code span already is. A standalone, unquoted `Usage::`-style + recipe line -- the shape #1531's own motivating defect took, and the one + this gate exists to catch -- is never backtick-wrapped in this + repository's existing docstring convention, confirmed live against every + in-scope script at authoring time (20 real matches -- 7 found by the + single-line match alone, 13 more found only once `_effective_line`'s own + shell-line-continuation join was added -- all standalone; every + backtick-wrapped candidate was illustrative prose, none a documented + recipe). +* Only the *module* docstring is scanned for a Python file -- an argparse + `description=` string, an inline comment, or a nested function's own + docstring is out of scope. A CLI's own `--help` text can therefore still + carry the same unguarded shape undetected; narrowing to the module + docstring is a deliberate scope limit (issue #1531's own text says + "a Python script's own module docstring"), not an oversight. +* No real shell tokenization: a `|` inside a quoted string + (`echo "a|b" | python3 x.py`) is graded the same as a real pipeline + boundary would be, and a consumer token appearing for an unrelated reason + (a comment, a variable name) immediately after a `|` is graded as a match + regardless of context. Both are the same class of imprecision + `gitapex_gate_no_raw_gh_cli_in_docs.py` already accepts for its own + fence-scoped regex scan. +* `||` (logical OR) is deliberately excluded -- it does not mask an upstream + exit status the way a single `|` does, so it carries none of this gate's + own risk class. +* A documentation-only lint cannot verify the actual runtime invocation an + agent performs matches the documented one (issue #1531's own stated + residual risk) -- it can only flag an unsafe documented example for a + human/agent to notice. +* Every exemption below is scoped to the whole enclosing unit, not to the + individual matched line: `pipefail` disclosure and the allow marker both + clear every match in the same fenced block (Markdown) or the same module + docstring (Python), not only the one match adjacent to the disclosure. + A block/docstring carrying two unrelated pipe examples, only one of them + genuinely covered by the stated disclosure or marker reason, silently + clears both -- this gate does not check that a marker's own reason text + actually corresponds to every match it exempts. The backtick exclusion + for a Python docstring line is likewise a whole-line check (`` "`" in + line ``), not position-aware: an unrelated backtick-quoted term earlier + on the same line as a real, unquoted recipe would exempt that recipe + too. Closing either gap needs a position-aware span check this gate does + not implement; a human reviewer reading the stated reason against the + block's actual content remains the backstop, the same trust this + gate's own sibling already places in `gitapex-allow-raw-gh-cli`'s + reason text. + +Exit codes: 0 clean, 1 violation(s) found, 2 the scan could not be trusted +(no in-scope file discovered in either category, or a file could not be +read/decoded as UTF-8 or parsed as Python) -- the same 0/1/2 split +`gitapex_gate_no_raw_gh_cli_in_docs.py` uses. + +Run via `uv run` (needed for the pydantic import) or via the pytest gate in +tests/test_gitapex_gate_unguarded_shell_pipe_in_docs.py. +""" + +from __future__ import annotations + +import argparse +import ast +import pathlib +import re +import subprocess +import sys +from dataclasses import dataclass + +from pydantic import BaseModel, ValidationError, field_validator + +REPO_ROOT = pathlib.Path(__file__).resolve().parents[2] + +# Fixed, explicit, never grown ad hoc -- the same discipline `_GH_SUBCOMMANDS` +# uses in gitapex_gate_no_raw_gh_cli_in_docs.py. Chosen to catch the real +# motivating shape (a data-producing command feeding an interpreter or text +# tool) while excluding a Python type hint (`list[str] | None`) and an +# ordinary Markdown table cell, neither of which is ever followed by one of +# these tokens. +_PIPE_CONSUMERS = frozenset( + { + "python3", + "python", + "bash", + "sh", + "zsh", + "uv", + "jq", + "grep", + "sed", + "awk", + "sort", + "xargs", + "head", + "tail", + "wc", + "tee", + "perl", + "ruby", + "node", + "cut", + "tr", + } +) + +# `\S` before the pipe requires a real token on the left; `(?= 3 of +# the same marker character and closes only on a bare run of that same +# character at least as long. +_FENCE_OPEN_RE = re.compile(r"^(`{3,}|~{3,})") +_FENCE_CLOSE_RE = re.compile(r"^(`{3,}|~{3,})$") + +_PIPEFAIL_RE = re.compile(r"pipefail", re.IGNORECASE) + +_ALLOW_MARKER_RE = re.compile(r"^[ \t]*[ \t]*$") + +_MARKDOWN_PATHSPECS = (":(glob)skills/*/SKILL.md", ":(glob)skills/*/references/*.md") +_PYTHON_PATHSPECS = ( + ":(glob).github/scripts/*.py", + ":(glob)skills/*/scripts/*.py", + ":(glob)evals/scripts/*.py", + ":(glob)hooks/*.py", +) + + +class ScanError(Exception): + """The scan could not be trusted -- exit 2, never a silent pass.""" + + +@dataclass(frozen=True) +class Violation: + path: str + line: int + matched: str + location: str # "fenced code block" or "module docstring" + + def describe(self) -> str: + return ( + f"{self.path}:{self.line}: unguarded shell pipe `{self.matched}` in a {self.location}, " + "with no nearby pipefail disclosure" + ) + + +def _pipe_match(line: str) -> re.Match[str] | None: + """The first `cmd | `-shaped match on `line`, or None.""" + return _PIPE_RE.search(line) + + +def _effective_line(lines: list[str], index: int) -> str: + """`lines[index]` (0-indexed), prefixed with the previous line's own + content when that previous line ends in a shell line-continuation + backslash. + + This repository's own `Usage::` convention commonly wraps a long + producer command onto its own line ending in `\\`, with the `| + ` continuation starting the next line -- `_pipe_match`'s own + `\\S` requirement before the pipe would otherwise never see a pipe that + is the first token on its own line. Joining the two lines here, the + same way a shell itself joins a backslash-continued command before + executing it, lets the existing single-line match still find it. A + literal `\\\\` (an escaped backslash, not a continuation) is + deliberately excluded. + """ + if index == 0: + return lines[index] + previous = lines[index - 1].rstrip() + if previous.endswith("\\") and not previous.endswith("\\\\"): + return previous[:-1] + " " + lines[index].lstrip() + return lines[index] + + +def _has_pipefail_disclosure(text: str) -> bool: + """True iff `text` mentions `pipefail` (case-insensitive) anywhere.""" + return bool(_PIPEFAIL_RE.search(text)) + + +def _has_allow_marker(lines: list[str], marker_line: int) -> bool: + """True iff the line directly above `marker_line` (1-indexed, no blank + line in between) is a valid `gitapex-allow-unguarded-shell-pipe` marker. + + Reused for both surfaces this gate scans: `marker_line` is a fence's own + opening marker line for Markdown, or the flagged line itself for a + Python docstring (which has no fence to anchor the marker's position + to). + """ + if marker_line < 2: + return False + return bool(_ALLOW_MARKER_RE.match(lines[marker_line - 2])) + + +def _fenced_line_ranges(lines: list[str]) -> list[tuple[int, int]]: + """Return `(open_marker_line, scan_end_line)` pairs, 1-indexed, for each + fenced block in `lines` -- identical contract to + `gitapex_gate_no_raw_gh_cli_in_docs.py`'s own `_fenced_line_ranges`: an + unclosed fence's `scan_end_line` is `len(lines) + 1`, one past the last + real line, so that line is still scanned even with no trailing newline. + """ + ranges: list[tuple[int, int]] = [] + open_run: str | None = None + open_line = 0 + for i, line in enumerate(lines, start=1): + stripped = line.strip() + if open_run is None: + opening = _FENCE_OPEN_RE.match(stripped) + if opening: + open_run = opening.group(1) + open_line = i + continue + closing = _FENCE_CLOSE_RE.match(stripped) + if closing and closing.group(1)[0] == open_run[0] and len(closing.group(1)) >= len(open_run): + ranges.append((open_line, i)) + open_run = None + if open_run is not None: + ranges.append((open_line, len(lines) + 1)) + return ranges + + +def markdown_violations_in_text(text: str) -> list[tuple[int, str]]: + """Return `(line, matched)` for every unguarded shell pipe found inside a + fenced code block in `text`, 1-indexed, skipping a fence either exempted + by a directly-preceding allow marker or already disclosing `pipefail` + somewhere inside itself.""" + lines = text.split("\n") + found: list[tuple[int, str]] = [] + for open_line, close_line in _fenced_line_ranges(lines): + if _has_allow_marker(lines, open_line): + continue + block_text = "\n".join(lines[open_line : close_line - 1]) + if _has_pipefail_disclosure(block_text): + continue + for lineno in range(open_line + 1, close_line): + if _pipe_match(_effective_line(lines, lineno - 1)): + found.append((lineno, lines[lineno - 1].strip())) + return found + + +def docstring_violations_in_text(doc_text: str) -> list[tuple[int, str]]: + """Return `(line, matched)` -- 1-indexed within `doc_text` -- for every + unguarded, non-backtick-quoted shell pipe found in a module docstring's + raw text, unless `doc_text` discloses `pipefail` anywhere (in which case + the whole docstring is treated as covered) or the flagged line is + directly preceded by a valid allow marker.""" + if _has_pipefail_disclosure(doc_text): + return [] + lines = doc_text.split("\n") + found: list[tuple[int, str]] = [] + for i, line in enumerate(lines, start=1): + if not _pipe_match(_effective_line(lines, i - 1)): + continue + if "`" in line: + continue + if _has_allow_marker(lines, i): + continue + found.append((i, line.strip())) + return found + + +def _tracked_files(root: pathlib.Path, pathspecs: tuple[str, ...]) -> list[pathlib.Path]: + try: + # S603/S607 waived: a fixed argv list with no shell, and `git` is + # intentionally resolved from PATH -- same rationale as + # gitapex_gate_no_raw_gh_cli_in_docs.py's own discover(). + result = subprocess.run( # noqa: S603 + ["git", "-C", str(root), "ls-files", "-z", "--", *pathspecs], # noqa: S607 + capture_output=True, + text=True, + check=False, + ) + except OSError as error: + raise ScanError(f"cannot run git to list tracked files: {error}") from error + if result.returncode != 0: + raise ScanError(f"{root}: git ls-files failed: {result.stderr.strip()}") + return sorted(root / name for name in result.stdout.split("\0") if name) + + +def discover_markdown(root: pathlib.Path) -> list[pathlib.Path]: + """Every tracked `skills/*/SKILL.md` and `skills/*/references/*.md` file.""" + return _tracked_files(root, _MARKDOWN_PATHSPECS) + + +def discover_python(root: pathlib.Path) -> list[pathlib.Path]: + """Every tracked checker/gate-script `.py` file in this gate's scope.""" + return _tracked_files(root, _PYTHON_PATHSPECS) + + +def _read_text(path: pathlib.Path) -> str: + try: + return path.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError) as error: + raise ScanError(f"{path}: cannot be read as UTF-8 text: {error}") from error + + +def violations_in_markdown_file(path: pathlib.Path, root: pathlib.Path) -> list[Violation]: + text = _read_text(path) + relative = str(path.relative_to(root)) # detection-logic-property-coverage: WAIVED: plain relativization only + return [ + Violation(path=relative, line=line, matched=matched, location="fenced code block") + for line, matched in markdown_violations_in_text(text) + ] + + +def _module_docstring_with_start_line(text: str, path: pathlib.Path) -> tuple[str, int] | None: + """The module docstring's raw text (not dedented/cleaned -- this gate's + own detection does not need that) and the file line its own first + character sits on, or None when the file has no module docstring. + + Raises `ScanError` on a syntax error rather than skipping the file: an + unparseable checker/gate script hides whether it carries the exact + shape this gate exists to catch. + """ + try: + tree = ast.parse(text) + except SyntaxError as error: + raise ScanError(f"{path}: cannot be parsed as Python: {error}") from error + if not tree.body: + return None + first_stmt = tree.body[0] + if not isinstance(first_stmt, ast.Expr): + return None + doc_expr = first_stmt.value + if not isinstance(doc_expr, ast.Constant) or not isinstance(doc_expr.value, str): + return None + return doc_expr.value, first_stmt.lineno + + +def violations_in_python_file(path: pathlib.Path, root: pathlib.Path) -> list[Violation]: + text = _read_text(path) + relative = str(path.relative_to(root)) # detection-logic-property-coverage: WAIVED: plain relativization only + docstring = _module_docstring_with_start_line(text, path) + if docstring is None: + return [] + doc_text, start_line = docstring + return [ + Violation(path=relative, line=start_line + line - 1, matched=matched, location="module docstring") + for line, matched in docstring_violations_in_text(doc_text) + ] + + +def find_violations(root: pathlib.Path = REPO_ROOT) -> list[Violation]: + """Scan every in-scope tracked file under `root` and return all + violations. Raises `ScanError` when neither corpus can be trusted to + have been checked -- an empty combined match set most plausibly means + the scan ran against the wrong root, and this gate would otherwise pass + while checking nothing. + + Deliberately AND-gated, not per-corpus: a real checkout always matches + at least this gate's own script under `.github/scripts/*.py`, so a + hypothetical future typo narrowing `_MARKDOWN_PATHSPECS` to zero + matches would not raise here even under a per-corpus check's own + intent -- and gating on either corpus alone breaks every test fixture + below that legitimately populates only one category to isolate what it + tests (confirmed live: 30 of this file's own tests failed against a + per-corpus version of this check, tried and reverted during this + gate's own authoring). `test_repository_scan_reaches_a_real_tracked_set` + is this repository's own real backstop for that regression instead -- + it asserts a real file-count floor for each corpus against the actual + checkout, so a pathspec narrowed to empty fails CI immediately rather + than silently passing this gate. + """ + markdown_paths = discover_markdown(root) + python_paths = discover_python(root) + if not markdown_paths and not python_paths: + raise ScanError( + f"{root}: no tracked skills/*/SKILL.md, skills/*/references/*.md, .github/scripts/*.py, " + "skills/*/scripts/*.py, evals/scripts/*.py, or hooks/*.py files found. An empty match set " + "most plausibly means the scan ran against the wrong root -- either way this gate would " + "otherwise pass while checking nothing." + ) + violations: list[Violation] = [] + for path in markdown_paths: + violations.extend(violations_in_markdown_file(path, root)) + for path in python_paths: + violations.extend(violations_in_python_file(path, root)) + return violations + + +class GateUnguardedShellPipeInDocsArgs(BaseModel): + """Typed view of `main`'s parsed CLI namespace.""" + + 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 + + +def main(argv: list[str] | None = None) -> int: + """CLI: 0 clean, 1 violation(s) found, 2 the scan could not be trusted.""" + parser = argparse.ArgumentParser( + description="Check that no skills/*/SKILL.md, skills/*/references/*.md, or checker/gate " + "script's own module docstring carries an unguarded `cmd1 | cmd2`-shaped shell pipe example " + "with no nearby pipefail disclosure." + ) + parser.add_argument( + "--root", + type=pathlib.Path, + default=REPO_ROOT, + help="Repository root to scan (defaults to this checkout).", + ) + args = parser.parse_args(argv) + + try: + validated = GateUnguardedShellPipeInDocsArgs(root=args.root) + except ValidationError: + print(f"{args.root}: --root must be an existing directory", file=sys.stderr) + return 2 + + try: + violations = find_violations(validated.root) + except ScanError as error: + print(f"{error}", file=sys.stderr) + return 2 + + if violations: + for violation in violations: + print(violation.describe(), file=sys.stderr) + print( + f"\n{len(violations)} unguarded shell pipe example(s) found with no nearby pipefail " + "disclosure. Add `set -o pipefail` (or an equivalent caveat mentioning `pipefail`) inside " + "the same fenced block (Markdown) or the same module docstring (Python), or if the pipe is " + "illustrative prose rather than a documented recipe, add " + "`` directly above the fence (Markdown) " + "or the flagged line (Python docstring) (issue #1531, refs #1567).", + file=sys.stderr, + ) + return 1 + + print( + f"OK: {len(discover_markdown(validated.root))} Markdown file(s) and " + f"{len(discover_python(validated.root))} Python file(s) carry no unguarded shell pipe examples." + ) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/scripts/gitapex_scan_contract_discipline_drift.py b/.github/scripts/gitapex_scan_contract_discipline_drift.py index 011794fb..15ef4289 100644 --- a/.github/scripts/gitapex_scan_contract_discipline_drift.py +++ b/.github/scripts/gitapex_scan_contract_discipline_drift.py @@ -47,6 +47,12 @@ 'skills/drafting-a-skill/references/contract-structure.md' \\ | uv run --frozen python3 .github/scripts/gitapex_scan_contract_discipline_drift.py --diff - +A bare pipe in the second form masks `git diff`'s own exit status in a +non-`pipefail` shell (issue #1531): add `set -o pipefail` first, or check +`git diff`'s own exit code separately, if the caller must detect an +upstream failure rather than silently scanning whatever partial diff +reached stdin. + Exit codes: 0 Content lock holds; diff (if any) shows no unacknowledged drift. 1 A lock drifted, or the diff shows the section changed without the diff --git a/.github/workflows/unguarded-shell-pipe-in-docs-gate.yml b/.github/workflows/unguarded-shell-pipe-in-docs-gate.yml new file mode 100644 index 00000000..b2582592 --- /dev/null +++ b/.github/workflows/unguarded-shell-pipe-in-docs-gate.yml @@ -0,0 +1,48 @@ +# Issue #1531 (refs #1567, gate-proposal-umbrella: local-hook fail-open +# remediation): the documented invocation `git log ... | python3 +# gitapex_check_task_commit_provenance.py` piped two commands directly +# together; a bare shell pipeline's own exit status is the RIGHT-hand +# command's, not the LEFT's, so an upstream `git log` failure silently +# reported a clean result instead of a blocked merge -- discovered only by +# an adversarial security-focused review, not by any deterministic check. +# No lint checked documentation/script-docstring files for this pattern +# before this gate. +# +# Deliberately no `paths:` filter, following no-raw-gh-cli-in-docs-gate.yml's +# own rationale: a workflow that never fires for a given PR leaves a +# required status check Pending forever, whereas a job that runs and passes +# is safe to promote to required later. The scan (a real `pydantic` import, +# same as that sibling gate) runs unconditionally over every in-scope +# tracked file and still costs only a few seconds. +name: Unguarded shell pipe in docs gate + +on: + pull_request: {} + workflow_dispatch: {} + +permissions: + contents: read + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +jobs: + unguarded-shell-pipe-in-docs: + runs-on: ubuntu-latest + timeout-minutes: 5 + permissions: + contents: read + steps: + - name: Harden runner + checkout + uses: tvna/gitapex/.github/actions/harden-checkout@2f62b5648552a0f800b1b85e75ec108a7016dd02 + + - name: Install uv + uses: astral-sh/setup-uv@c771a70e6277c0a99b617c7a806ffedaca235ff9 # v9.0.0 + with: + enable-cache: true + + - name: Check for unguarded shell pipe examples in docs/script docstrings + run: | + set -euo pipefail + uv run --frozen python3 .github/scripts/gitapex_gate_unguarded_shell_pipe_in_docs.py diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index e8038d85..1525de00 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -158,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 44 wired gates it is too slow to sit on every + # warm end to end for all 45 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 cec8ee30..5b066536 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -125,9 +125,10 @@ 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 -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 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 +one pass, before the push leaves your machine. A warm run of all 45 wired +gates measures roughly 14 seconds end to end (the +prior 44-gate set measured roughly 15 seconds, the 43-gate set before that +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 @@ -163,7 +164,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 44 wired gates (the same `uv run` pins CI uses). Without `uv` +and so do all 45 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/hooks/gitapex_check_post_review_obligation_tracker.py b/hooks/gitapex_check_post_review_obligation_tracker.py index 943fb84b..c5d5c4f0 100644 --- a/hooks/gitapex_check_post_review_obligation_tracker.py +++ b/hooks/gitapex_check_post_review_obligation_tracker.py @@ -185,6 +185,11 @@ printf '%s' '{"session_id":"abc","tool_name":"Bash","tool_input":{"command":"git push"}}' \\ | python3 hooks/gitapex_check_post_review_obligation_tracker.py + +A bare pipe here masks `printf`'s own exit status in a non-`pipefail` +shell (issue #1531) -- harmless for a literal `printf` producer, which +cannot itself fail in ordinary use, but add `set -o pipefail` first if +this recipe's producer is ever swapped for a command that can. """ from __future__ import annotations diff --git a/hooks/gitapex_check_post_write_provenance.py b/hooks/gitapex_check_post_write_provenance.py index e5a94c88..acf56fd4 100644 --- a/hooks/gitapex_check_post_write_provenance.py +++ b/hooks/gitapex_check_post_write_provenance.py @@ -142,6 +142,11 @@ "tool_input":{"owner":"tvna","repo":"gitapex","pullNumber":1}}' \\ | python3 hooks/gitapex_check_post_write_provenance.py +A bare pipe here masks `printf`'s own exit status in a non-`pipefail` +shell (issue #1531) -- harmless for a literal `printf` producer, which +cannot itself fail in ordinary use, but add `set -o pipefail` first if +this recipe's producer is ever swapped for a command that can. + Exit codes: 0 PASS (the stored body scanned clean), or SKIP (the payload names a tool this gate does not cover -- self-revalidation, dimension 3). diff --git a/hooks/gitapex_check_pr_duplicate_issue.py b/hooks/gitapex_check_pr_duplicate_issue.py index fa952a32..66a6198f 100644 --- a/hooks/gitapex_check_pr_duplicate_issue.py +++ b/hooks/gitapex_check_pr_duplicate_issue.py @@ -81,6 +81,11 @@ printf '%s' '{"owner":"tvna","repo":"gitapex","title":"...","body":"Closes #1"}' \\ | python3 hooks/gitapex_check_pr_duplicate_issue.py +A bare pipe here masks `printf`'s own exit status in a non-`pipefail` +shell (issue #1531) -- harmless for a literal `printf` producer, which +cannot itself fail in ordinary use, but add `set -o pipefail` first if +this recipe's producer is ever swapped for a command that can. + Exit codes: 0 Allow -- no resolving citation on the new PR, a waiver is present, or no other open PR cites the same issue(s). diff --git a/hooks/gitapex_check_pr_issue_acm_disclosure.py b/hooks/gitapex_check_pr_issue_acm_disclosure.py index 6c8251b7..9a480323 100644 --- a/hooks/gitapex_check_pr_issue_acm_disclosure.py +++ b/hooks/gitapex_check_pr_issue_acm_disclosure.py @@ -116,6 +116,11 @@ printf '%s' '{"owner":"tvna","repo":"gitapex","title":"...","body":"Closes #1"}' \\ | python3 hooks/gitapex_check_pr_issue_acm_disclosure.py +A bare pipe here masks `printf`'s own exit status in a non-`pipefail` +shell (issue #1531) -- harmless for a literal `printf` producer, which +cannot itself fail in ordinary use, but add `set -o pipefail` first if +this recipe's producer is ever swapped for a command that can. + Exit codes: 0 Allow -- every resolving-cited issue passed, or none was cited but some other citation form (Refs/#N) was. diff --git a/hooks/gitapex_check_stop_review_obligation.py b/hooks/gitapex_check_stop_review_obligation.py index 0646d8be..86660321 100644 --- a/hooks/gitapex_check_stop_review_obligation.py +++ b/hooks/gitapex_check_stop_review_obligation.py @@ -80,6 +80,11 @@ printf '%s' '{"session_id":"abc"}' \\ | python3 hooks/gitapex_check_stop_review_obligation.py + +A bare pipe here masks `printf`'s own exit status in a non-`pipefail` +shell (issue #1531) -- harmless for a literal `printf` producer, which +cannot itself fail in ordinary use, but add `set -o pipefail` first if +this recipe's producer is ever swapped for a command that can. """ from __future__ import annotations diff --git a/skills/evaluating-deterministic-gate-quality/metadata/gitapex.yaml b/skills/evaluating-deterministic-gate-quality/metadata/gitapex.yaml index d6ea26d1..6d05e39a 100644 --- a/skills/evaluating-deterministic-gate-quality/metadata/gitapex.yaml +++ b/skills/evaluating-deterministic-gate-quality/metadata/gitapex.yaml @@ -820,6 +820,11 @@ spec: summary: "Added the missing second `[elided]` marker for the dropped ACM-disclosure-family clause, and generalized the quotation's own preamble from 'one elided span' to 'two elided spans' to match. Re-run confirmed 61/61 shape checks unchanged." outcome: lines: "branch-final, measured against the working tree at the final commit: gitapex-worked-examples.md 595->598 (second elision marker), dimensions.md unchanged at 638" + - kind: correction + anchor: "https://github.com/tvna/gitapex/issues/1531" + summary: "Unrelated docs-hygiene fix: added a `` exception marker to gitapex-worked-examples.md's own live-measurement timing transcript, one line, so a new documentation-lint gate (https://github.com/tvna/gitapex/issues/1531) does not misread that transcript's illustrative pipe as an undisclosed merge-gate invocation risk. Recorded here, not by editing the prior branch-final entry above, since this change is not part of this skill's own audit history." + outcome: + lines: "gitapex-worked-examples.md 598->599 (one exception-marker line added)" skillDependencies: requires: - evaluating-skill-quality diff --git a/skills/evaluating-deterministic-gate-quality/references/gitapex-worked-examples.md b/skills/evaluating-deterministic-gate-quality/references/gitapex-worked-examples.md index d97a5bb5..67bfe165 100644 --- a/skills/evaluating-deterministic-gate-quality/references/gitapex-worked-examples.md +++ b/skills/evaluating-deterministic-gate-quality/references/gitapex-worked-examples.md @@ -374,6 +374,7 @@ real script with bash's own `time` builtin (copy-pasteable and reproducible as written; substitute a different repository checkout's own path if re-running elsewhere): + ``` $ payload='{"tool_name":"mcp__github__issue_write","tool_input":{"method":"create","body":"ACM: not-applicable (docs): example"}}' $ for i in 1 2 3 4 5; do diff --git a/skills/executing-a-branch-plan/scripts/gitapex_check_canonical_governance_paths.py b/skills/executing-a-branch-plan/scripts/gitapex_check_canonical_governance_paths.py index 5825c3c1..e76a77dd 100644 --- a/skills/executing-a-branch-plan/scripts/gitapex_check_canonical_governance_paths.py +++ b/skills/executing-a-branch-plan/scripts/gitapex_check_canonical_governance_paths.py @@ -31,6 +31,11 @@ python3 gitapex_check_canonical_governance_paths.py --files git diff --name-only BASE HEAD | python3 gitapex_check_canonical_governance_paths.py +A bare pipe here masks `git diff`'s own exit status in a non-`pipefail` +shell (issue #1531): add `set -o pipefail` first, or check `git diff`'s own +exit code separately, if the caller must detect an upstream failure rather +than silently classifying whatever partial path list reached stdin. + Input: one file path per line (a file, via --files, or stdin). Exit code: 0 on a successful run (this is an informational classifier, diff --git a/skills/executing-a-branch-plan/scripts/gitapex_check_file_ownership_conflicts.py b/skills/executing-a-branch-plan/scripts/gitapex_check_file_ownership_conflicts.py index d335d36c..56884644 100644 --- a/skills/executing-a-branch-plan/scripts/gitapex_check_file_ownership_conflicts.py +++ b/skills/executing-a-branch-plan/scripts/gitapex_check_file_ownership_conflicts.py @@ -33,6 +33,11 @@ python3 gitapex_check_file_ownership_conflicts.py --input echo '{"task-a": ["a.py"], "task-b": ["a.py"]}' | python3 gitapex_check_file_ownership_conflicts.py +A bare pipe here masks `echo`'s own exit status in a non-`pipefail` shell +(issue #1531) -- harmless for a literal `echo` producer, which cannot +itself fail in ordinary use, but add `set -o pipefail` first if this +recipe's producer is ever swapped for a command that can. + Input JSON shape: an object mapping each task ID (string) to a list of file paths (strings) that task will write. diff --git a/tests/test_gitapex_gate_local_preflight.py b/tests/test_gitapex_gate_local_preflight.py index df0ac559..d8a369e8 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 44 wired gates. Issue + second with no dependence on this repository's real 45 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" -- diff --git a/tests/test_gitapex_gate_unguarded_shell_pipe_in_docs.py b/tests/test_gitapex_gate_unguarded_shell_pipe_in_docs.py new file mode 100644 index 00000000..86c6c99d --- /dev/null +++ b/tests/test_gitapex_gate_unguarded_shell_pipe_in_docs.py @@ -0,0 +1,619 @@ +"""Tests for the unguarded-shell-pipe-in-docs gate +(.github/scripts/gitapex_gate_unguarded_shell_pipe_in_docs.py). + +Issue #1531 (refs #1567, gate-proposal-umbrella: local-hook fail-open +remediation). The documented invocation `git log ... | python3 +gitapex_check_task_commit_provenance.py` piped two commands directly +together, silently masking an upstream `git log` failure. This gate exists +so a future documented recipe cannot reintroduce the same shape +undetected. +""" + +from __future__ import annotations + +import pathlib +import subprocess + +import gitapex_gate_unguarded_shell_pipe_in_docs as gate +import pytest + +REPO_ROOT = pathlib.Path(__file__).resolve().parents[1] + + +def _repo(tmp_path: pathlib.Path) -> pathlib.Path: + subprocess.run(["git", "init", "-q", str(tmp_path)], check=True) + return tmp_path + + +def _write(root: pathlib.Path, relative: str, content: str, *, track: bool = True) -> pathlib.Path: + path = root / relative + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(content, encoding="utf-8") + if track: + subprocess.run(["git", "-C", str(root), "add", "--", relative], check=True) + return path + + +# --- the real repository ------------------------------------------------- + + +def test_repository_has_no_unguarded_shell_pipe_violations() -> None: + """The real checkout passes clean -- every real pre-existing instance + this gate's own authoring found was fixed (a pipefail disclosure added) + in the same change that adds this gate, matching + gitapex_gate_no_raw_gh_cli_in_docs.py's own historical-grandfathering + precedent.""" + assert gate.find_violations(REPO_ROOT) == [] + + +def test_repository_scan_reaches_a_real_tracked_set() -> None: + """Without this, a discovery bug that found nothing would make the test + above pass for the wrong reason.""" + assert len(gate.discover_markdown(REPO_ROOT)) > 20 + assert len(gate.discover_python(REPO_ROOT)) > 20 + + +# --- reintroducing the original #1531 defect ------------------------------ + + +def test_reintroducing_the_original_defect_shape_is_caught(tmp_path: pathlib.Path) -> None: + """Test-first proof: the exact original defect shape -- a bare + `git log ... | python3 ...` recipe with no pipefail disclosure -- is + caught when reintroduced into a checker script's own module docstring, + the same shape issue #1531 itself reports against + gitapex_check_task_commit_provenance.py.""" + root = _repo(tmp_path) + _write( + root, + "hooks/gitapex_check_example.py", + '"""Usage -- piped directly together for convenience::\n\n' + " git log --format=%B -z BASE..HEAD | python3 gitapex_check_example.py\n\n" + 'Exit codes:\n 0 PASS\n"""\n', + ) + violations = gate.find_violations(root) + assert len(violations) == 1 + assert violations[0].path == "hooks/gitapex_check_example.py" + assert "git log" in violations[0].matched + + +def test_the_current_real_fixed_docstring_is_clean() -> None: + """The real, already-fixed file the original defect names discloses + `pipefail` elsewhere in its own module docstring, so this gate must not + re-flag it.""" + path = REPO_ROOT / "skills/executing-a-branch-plan/scripts/gitapex_check_task_commit_provenance.py" + text = path.read_text(encoding="utf-8") + docstring = gate._module_docstring_with_start_line(text, path) + assert docstring is not None + doc, _ = docstring + assert gate.docstring_violations_in_text(doc) == [] + + +# --- Markdown: violations -------------------------------------------------- + + +def test_pipe_in_a_fenced_block_with_no_pipefail_disclosure_is_a_violation(tmp_path: pathlib.Path) -> None: + root = _repo(tmp_path) + _write( + root, + "skills/foo/SKILL.md", + "---\nname: foo\n---\n\n## Usage\n\n```bash\ngit log --oneline | python3 check.py\n```\n", + ) + violations = gate.find_violations(root) + assert [v.path for v in violations] == ["skills/foo/SKILL.md"] + assert violations[0].location == "fenced code block" + + +def test_tilde_fence_is_also_scanned(tmp_path: pathlib.Path) -> None: + root = _repo(tmp_path) + _write(root, "skills/foo/references/notes.md", "~~~bash\ngit log | python3 x.py\n~~~\n") + assert len(gate.find_violations(root)) == 1 + + +def test_fence_nested_in_a_longer_fence_is_still_scanned(tmp_path: pathlib.Path) -> None: + """CommonMark run-length pairing, same as the sibling gate: a three- + backtick fence nested inside a four-backtick one does not close the + outer block.""" + root = _repo(tmp_path) + _write( + root, + "skills/foo/SKILL.md", + "````markdown\n## Example\n\n```bash\ngit log | python3 x.py\n```\n````\n", + ) + violations = gate.find_violations(root) + assert len(violations) == 1 + assert violations[0].line == 5 + + +# --- Markdown: must not fire ------------------------------------------------ + + +def test_pipe_outside_any_fence_is_not_scanned(tmp_path: pathlib.Path) -> None: + root = _repo(tmp_path) + _write(root, "skills/foo/SKILL.md", "prose mentioning `git log | python3 x.py` inline, not fenced\n") + assert gate.find_violations(root) == [] + + +def test_markdown_table_row_is_not_a_violation(tmp_path: pathlib.Path) -> None: + root = _repo(tmp_path) + _write(root, "skills/foo/SKILL.md", "| Situation | Skill |\n|---|---|\n| a | b |\n") + assert gate.find_violations(root) == [] + + +def test_python_type_hint_shape_is_not_a_violation(tmp_path: pathlib.Path) -> None: + """The false-positive class this gate's own docstring names: a bare + `\\S \\| \\S` match would fire on a quoted Python type hint.""" + root = _repo(tmp_path) + _write(root, "skills/foo/SKILL.md", "```python\ndef main(argv: list[str] | None = None) -> int: ...\n```\n") + assert gate.find_violations(root) == [] + + +def test_logical_or_double_pipe_is_not_a_violation(tmp_path: pathlib.Path) -> None: + root = _repo(tmp_path) + _write(root, "skills/foo/SKILL.md", "```bash\ncommand1 || command2\n```\n") + assert gate.find_violations(root) == [] + + +def test_pipefail_disclosed_inside_the_same_fence_exempts_it(tmp_path: pathlib.Path) -> None: + root = _repo(tmp_path) + _write( + root, + "skills/foo/SKILL.md", + "```bash\nset -o pipefail\ngit log --oneline | python3 check.py\n```\n", + ) + assert gate.find_violations(root) == [] + + +def test_allow_marker_directly_above_the_fence_exempts_it(tmp_path: pathlib.Path) -> None: + root = _repo(tmp_path) + _write( + root, + "skills/foo/SKILL.md", + "\n```bash\ngit log | python3 x.py\n```\n", + ) + assert gate.find_violations(root) == [] + + +def test_allow_marker_separated_by_a_blank_line_does_not_exempt(tmp_path: pathlib.Path) -> None: + root = _repo(tmp_path) + _write( + root, + "skills/foo/SKILL.md", + "\n\n```bash\ngit log | python3 x.py\n```\n", + ) + assert len(gate.find_violations(root)) == 1 + + +def test_malformed_allow_marker_with_no_reason_does_not_exempt(tmp_path: pathlib.Path) -> None: + root = _repo(tmp_path) + _write( + root, + "skills/foo/SKILL.md", + "\n```bash\ngit log | python3 x.py\n```\n", + ) + assert len(gate.find_violations(root)) == 1 + + +def test_untracked_markdown_file_is_not_scanned(tmp_path: pathlib.Path) -> None: + root = _repo(tmp_path) + _write(root, "skills/foo/SKILL.md", "clean\n") + _write(root, "skills/foo/references/dirty.md", "```bash\ngit log | python3 x.py\n```\n", track=False) + assert gate.find_violations(root) == [] + + +def test_non_reference_markdown_under_skills_is_not_scanned(tmp_path: pathlib.Path) -> None: + """Scope is skills/*/SKILL.md and skills/*/references/*.md only -- a + stray markdown file elsewhere under a skill directory is out of scope.""" + root = _repo(tmp_path) + _write(root, "skills/foo/SKILL.md", "clean\n") + _write(root, "skills/foo/notes/extra.md", "```bash\ngit log | python3 x.py\n```\n") + _write(root, "docs/plan.md", "```bash\ngit log | python3 x.py\n```\n") + assert gate.find_violations(root) == [] + + +def test_doubly_nested_skill_md_is_not_scanned(tmp_path: pathlib.Path) -> None: + """`skills/*/SKILL.md` must not let `*` cross a `/` -- a SKILL.md nested + two levels deep is not `skills//SKILL.md` and is out of scope. + Without the `:(glob)` pathspec magic, git's own default pathspec + matching lets a bare `*` span `/`, live-confirmed to otherwise match + this exact shape.""" + root = _repo(tmp_path) + _write(root, "hooks/gitapex_check_placeholder.py", '"""ok\n"""\n') + _write(root, "skills/foo/nested/SKILL.md", "```bash\ngit log | python3 x.py\n```\n") + assert gate.find_violations(root) == [] + + +def test_doubly_nested_reference_file_is_not_scanned(tmp_path: pathlib.Path) -> None: + """`skills/*/references/*.md` must likewise not let `*` cross a `/` -- + a file nested one level deeper than skills//references/ is out of + scope, the same class of gap the SKILL.md case above pins.""" + root = _repo(tmp_path) + _write(root, "hooks/gitapex_check_placeholder.py", '"""ok\n"""\n') + _write(root, "skills/foo/references/sub/deep.md", "```bash\ngit log | python3 x.py\n```\n") + assert gate.find_violations(root) == [] + + +# --- Python docstrings: violations ----------------------------------------- + + +def test_standalone_recipe_line_in_module_docstring_is_a_violation(tmp_path: pathlib.Path) -> None: + root = _repo(tmp_path) + _write( + root, + ".github/scripts/gitapex_gate_example.py", + '"""Usage::\n\n git diff --name-only BASE HEAD | python3 gitapex_gate_example.py\n"""\n', + ) + violations = gate.find_violations(root) + assert len(violations) == 1 + assert violations[0].location == "module docstring" + assert violations[0].line == 3 + + +def test_violation_reports_the_real_file_line_not_a_docstring_relative_one(tmp_path: pathlib.Path) -> None: + """The reported line is the absolute file line, computed from the + docstring AST node's own `lineno` -- not an offset relative to the + (possibly dedented) docstring text alone.""" + root = _repo(tmp_path) + _write( + root, + "hooks/gitapex_check_padded.py", + "#!/usr/bin/env python3\n" + '"""One line of preamble before the recipe.\n\n' + " git diff --name-only BASE HEAD | python3 gitapex_check_padded.py\n" + '"""\n', + ) + violations = gate.find_violations(root) + assert len(violations) == 1 + assert violations[0].line == 4 + + +# --- Python docstrings: must not fire -------------------------------------- + + +def test_backtick_quoted_inline_example_in_a_docstring_is_not_a_violation(tmp_path: pathlib.Path) -> None: + """The false-positive class this gate's own docstring names: a + backtick-quoted illustrative warning, not a standalone recipe.""" + root = _repo(tmp_path) + _write( + root, + "hooks/gitapex_check_example.py", + '"""Never invoke as `git log ... | python3 x.py` in an ordinary shell.\n"""\n', + ) + assert gate.find_violations(root) == [] + + +def test_double_backtick_quoted_inline_example_is_not_a_violation(tmp_path: pathlib.Path) -> None: + root = _repo(tmp_path) + _write(root, "hooks/gitapex_check_example.py", '"""See ``git show : | wc -l`` for comparison.\n"""\n') + assert gate.find_violations(root) == [] + + +def test_pipefail_disclosed_anywhere_in_the_docstring_exempts_the_whole_file(tmp_path: pathlib.Path) -> None: + root = _repo(tmp_path) + _write( + root, + "hooks/gitapex_check_example.py", + '"""Usage::\n\n git diff --name-only BASE HEAD | python3 gitapex_check_example.py\n\n' + 'Never invoke this as a bare pipe in a non-pipefail shell.\n"""\n', + ) + assert gate.find_violations(root) == [] + + +def test_allow_marker_directly_above_the_flagged_line_exempts_it(tmp_path: pathlib.Path) -> None: + root = _repo(tmp_path) + _write( + root, + "hooks/gitapex_check_example.py", + '"""Usage::\n\n\n' + ' git diff --name-only BASE HEAD | python3 gitapex_check_example.py\n"""\n', + ) + assert gate.find_violations(root) == [] + + +def test_no_module_docstring_is_not_a_violation(tmp_path: pathlib.Path) -> None: + root = _repo(tmp_path) + _write(root, "hooks/gitapex_check_example.py", "import sys\nsys.exit(0)\n") + assert gate.find_violations(root) == [] + + +def test_empty_python_file_has_no_module_docstring(tmp_path: pathlib.Path) -> None: + """An empty file has no statements at all -- `tree.body` is empty, a + distinct case from a file whose first statement merely isn't a + docstring.""" + root = _repo(tmp_path) + text = "" + assert gate._module_docstring_with_start_line(text, pathlib.Path("empty.py")) is None + _write(root, "hooks/gitapex_check_empty.py", text) + assert gate.find_violations(root) == [] + + +def test_non_string_first_statement_has_no_module_docstring(tmp_path: pathlib.Path) -> None: + """The first statement can be a bare expression that is not a string + constant (here, a bare integer literal) -- distinct from `import sys` + (not an `Expr` at all) and from a real docstring (an `Expr` wrapping a + string `Constant`).""" + root = _repo(tmp_path) + text = "42\n" + assert gate._module_docstring_with_start_line(text, pathlib.Path("x.py")) is None + _write(root, "hooks/gitapex_check_non_string_first.py", text) + assert gate.find_violations(root) == [] + + +def test_python_file_outside_the_checker_gate_scope_is_not_scanned(tmp_path: pathlib.Path) -> None: + root = _repo(tmp_path) + _write(root, "hooks/gitapex_check_example.py", '"""ok\n"""\n') + _write( + root, + "src/not_in_scope.py", + '"""Usage::\n\n git diff --name-only BASE HEAD | python3 not_in_scope.py\n"""\n', + ) + assert gate.find_violations(root) == [] + + +def test_non_gh_style_consumer_is_not_a_violation(tmp_path: pathlib.Path) -> None: + """A pipe with no recognized consumer token on the right is not flagged + -- this gate requires a real shell-consumer vocabulary match, not a bare + pipe character.""" + root = _repo(tmp_path) + _write( + root, + "hooks/gitapex_check_example.py", + '"""Usage::\n\n some_value | not_a_real_consumer_tool\n"""\n', + ) + assert gate.find_violations(root) == [] + + +# --- fail closed (exit 2) ------------------------------------------------ + + +def test_discovering_nothing_in_either_category_is_an_error_not_a_pass( + tmp_path: pathlib.Path, capsys: pytest.CaptureFixture[str] +) -> None: + root = _repo(tmp_path) + assert gate.main(["--root", str(root)]) == 2 + assert "checking nothing" in capsys.readouterr().err + + +def test_non_utf8_markdown_file_fails_closed_naming_the_file( + tmp_path: pathlib.Path, capsys: pytest.CaptureFixture[str] +) -> None: + root = _repo(tmp_path) + path = _write(root, "skills/foo/SKILL.md", "clean\n") + path.write_bytes(b"\xff\xfe not valid utf-8") + subprocess.run(["git", "-C", str(root), "add", "--", "skills/foo/SKILL.md"], check=True) + assert gate.main(["--root", str(root)]) == 2 + stderr = capsys.readouterr().err + assert "skills/foo/SKILL.md" in stderr + assert "cannot be read as UTF-8" in stderr + + +def test_invalid_python_syntax_fails_closed_naming_the_file( + tmp_path: pathlib.Path, capsys: pytest.CaptureFixture[str] +) -> None: + root = _repo(tmp_path) + _write(root, "hooks/gitapex_check_broken.py", "def f(:\n pass\n") + assert gate.main(["--root", str(root)]) == 2 + stderr = capsys.readouterr().err + assert "gitapex_check_broken.py" in stderr + assert "cannot be parsed as Python" in stderr + + +def test_a_non_repository_root_fails_closed(tmp_path: pathlib.Path, capsys: pytest.CaptureFixture[str]) -> None: + assert gate.main(["--root", str(tmp_path)]) == 2 + assert "git ls-files failed" in capsys.readouterr().err + + +def test_git_missing_entirely_fails_closed( + tmp_path: pathlib.Path, capsys: pytest.CaptureFixture[str], monkeypatch: pytest.MonkeyPatch +) -> None: + def _no_git(*args: object, **kwargs: object) -> subprocess.CompletedProcess[str]: + raise OSError("No such file or directory: 'git'") + + monkeypatch.setattr(gate.subprocess, "run", _no_git) + assert gate.main(["--root", str(tmp_path)]) == 2 + assert "cannot run git" in capsys.readouterr().err + + +def test_main_exits_2_on_a_root_that_does_not_exist(tmp_path: pathlib.Path, capsys: pytest.CaptureFixture[str]) -> None: + missing = tmp_path / "does-not-exist" + assert gate.main(["--root", str(missing)]) == 2 + assert "must be an existing directory" in capsys.readouterr().err + + +def test_main_exits_2_on_a_root_that_is_a_file(tmp_path: pathlib.Path, capsys: pytest.CaptureFixture[str]) -> None: + a_file = tmp_path / "not-a-directory" + a_file.write_text("x", encoding="utf-8") + assert gate.main(["--root", str(a_file)]) == 2 + assert "must be an existing directory" in capsys.readouterr().err + + +# --- CLI ------------------------------------------------------------------- + + +def test_main_returns_zero_on_the_real_repository(capsys: pytest.CaptureFixture[str]) -> None: + assert gate.main(["--root", str(REPO_ROOT)]) == 0 + assert "OK:" in capsys.readouterr().out + + +def test_main_returns_one_and_explains_the_failure(tmp_path: pathlib.Path, capsys: pytest.CaptureFixture[str]) -> None: + root = _repo(tmp_path) + _write(root, "skills/foo/SKILL.md", "```bash\ngit log | python3 x.py\n```\n") + assert gate.main(["--root", str(root)]) == 1 + stderr = capsys.readouterr().err + assert "skills/foo/SKILL.md:2" in stderr + assert "gitapex-allow-unguarded-shell-pipe" in stderr + assert "#1531" in stderr + + +# --- GateUnguardedShellPipeInDocsArgs validation -------------------------- + + +def test_args_reject_a_root_that_does_not_exist(tmp_path: pathlib.Path) -> None: + with pytest.raises(ValueError, match="must be an existing directory"): + gate.GateUnguardedShellPipeInDocsArgs(root=tmp_path / "does-not-exist") + + +def test_args_root_must_exist_accepts_a_real_directory(tmp_path: pathlib.Path) -> None: + """Calls `_root_must_exist` directly on both its accepting and its + rejecting path, not only through the constructor above.""" + assert gate.GateUnguardedShellPipeInDocsArgs._root_must_exist(tmp_path) == tmp_path + with pytest.raises(ValueError, match="must be an existing directory"): + gate.GateUnguardedShellPipeInDocsArgs._root_must_exist(tmp_path / "does-not-exist") + + +# --- internal helpers, called directly ------------------------------------ + + +def test_violation_describe_names_the_path_line_and_location() -> None: + violation = gate.Violation( + path="skills/foo/SKILL.md", line=7, matched="git log | python3 x.py", location="fenced code block" + ) + described = violation.describe() + assert "skills/foo/SKILL.md:7" in described + assert "fenced code block" in described + assert "git log | python3 x.py" in described + + +def test_markdown_violations_in_text_called_directly(tmp_path: pathlib.Path) -> None: + text = "```bash\ngit log | python3 x.py\n```\n" + assert gate.markdown_violations_in_text(text) == [(2, "git log | python3 x.py")] + + +def test_violations_in_markdown_file_called_directly(tmp_path: pathlib.Path) -> None: + root = _repo(tmp_path) + path = _write(root, "skills/foo/SKILL.md", "```bash\ngit log | python3 x.py\n```\n") + violations = gate.violations_in_markdown_file(path, root) + assert [v.path for v in violations] == ["skills/foo/SKILL.md"] + + +def test_violations_in_python_file_called_directly(tmp_path: pathlib.Path) -> None: + root = _repo(tmp_path) + path = _write( + root, + "hooks/gitapex_check_direct.py", + '"""Usage::\n\n git diff --name-only BASE HEAD | python3 gitapex_check_direct.py\n"""\n', + ) + violations = gate.violations_in_python_file(path, root) + assert len(violations) == 1 + assert violations[0].location == "module docstring" + + +def test_tracked_files_called_directly(tmp_path: pathlib.Path) -> None: + root = _repo(tmp_path) + _write(root, "skills/foo/SKILL.md", "clean\n") + found = gate._tracked_files(root, ("skills/*/SKILL.md",)) + assert [p.relative_to(root).as_posix() for p in found] == ["skills/foo/SKILL.md"] + + +def test_read_text_called_directly(tmp_path: pathlib.Path) -> None: + path = tmp_path / "a.md" + path.write_text("hello\n", encoding="utf-8") + assert gate._read_text(path) == "hello\n" + + +# --- shell line-continuation (issue #1531 blast-radius review finding) ---- + + +def test_pipe_split_across_a_line_continuation_is_caught_in_a_docstring(tmp_path: pathlib.Path) -> None: + """The exact live gap a blast-radius review found: this repository's + own `Usage::` convention commonly wraps a long producer command onto + its own line ending in a backslash, with `| ` starting the + next line -- `_pipe_match`'s own same-line `\\S` requirement would + otherwise never see this. Confirmed against real, previously- + undetected instances in this repository (e.g. + `.github/scripts/gitapex_extract_diff_added_lines.py`) before this + fix, all now disclosed via a pipefail caveat in the same change.""" + root = _repo(tmp_path) + _write( + root, + "hooks/gitapex_check_continuation.py", + '"""Usage::\n\n git diff --name-only BASE HEAD \\\\\n | python3 gitapex_check_continuation.py\n"""\n', + ) + violations = gate.find_violations(root) + assert len(violations) == 1 + assert violations[0].line == 4 + + +def test_pipe_split_across_a_line_continuation_is_caught_in_a_fence(tmp_path: pathlib.Path) -> None: + root = _repo(tmp_path) + _write( + root, + "skills/foo/SKILL.md", + "```bash\ngit diff --name-only BASE HEAD \\\n | python3 check.py\n```\n", + ) + violations = gate.find_violations(root) + assert len(violations) == 1 + assert violations[0].line == 3 + + +def test_escaped_backslash_at_line_end_is_not_a_continuation(tmp_path: pathlib.Path) -> None: + """A literal `\\\\` (two backslashes) at end of line is an escaped + backslash, not a shell line-continuation -- the following line's own + leading pipe must not be joined to it.""" + root = _repo(tmp_path) + _write( + root, + "hooks/gitapex_check_escaped.py", + '"""Usage::\n\n echo "literal backslash: \\\\\\\\"\n | python3 gitapex_check_escaped.py\n"""\n', + ) + assert gate.find_violations(root) == [] + + +def test_effective_line_called_directly_on_a_continuation() -> None: + """The join keeps whatever whitespace already preceded the backslash + (here, a real space) and adds one more before the next line's own + (left-stripped) content -- `_PIPE_RE`'s own `[ \\t]*` tolerates either + count, so this is a formatting detail, not a correctness requirement.""" + lines = ["git diff --name-only BASE HEAD \\", " | python3 x.py"] + assert gate._effective_line(lines, 1) == "git diff --name-only BASE HEAD | python3 x.py" + + +def test_effective_line_called_directly_on_the_first_line() -> None: + """Index 0 has no predecessor to join with -- returned unchanged.""" + lines = ["| python3 x.py"] + assert gate._effective_line(lines, 0) == "| python3 x.py" + + +# --- disclosed known gaps, pinned rather than silently left untested ----- + + +def test_a_table_cell_equal_to_a_consumer_token_is_a_disclosed_over_report(tmp_path: pathlib.Path) -> None: + """Pins a limitation this gate's own docstring discloses rather than + claims closed: a Markdown table row whose own cell value happens to + equal a `_PIPE_CONSUMERS` token still matches, since nothing here + distinguishes a table's `|` column separator from a shell pipe.""" + root = _repo(tmp_path) + _write(root, "skills/foo/SKILL.md", "```text\n| Parser | jq |\n```\n") + assert len(gate.find_violations(root)) == 1 + + +def test_backtick_exclusion_is_whole_line_not_span_aware_disclosed_gap(tmp_path: pathlib.Path) -> None: + """Pins another disclosed limitation: the backtick exclusion for a + Python docstring checks whether the LINE carries any backtick at all, + not whether the matched pipe itself sits inside one -- an unrelated + backtick-quoted term earlier on the same line as a real, unquoted + recipe silently exempts that recipe too.""" + root = _repo(tmp_path) + _write( + root, + "hooks/gitapex_check_mixed_line.py", + '"""Usage: run `gitapex_check_mixed_line.py` via: ' + 'git diff --name-only BASE HEAD | python3 gitapex_check_mixed_line.py\n"""\n', + ) + assert gate.find_violations(root) == [] + + +def test_marker_exempts_every_match_in_the_block_disclosed_gap(tmp_path: pathlib.Path) -> None: + """Pins the disclosed whole-block exemption scope: one allow marker, + with a reason describing only the first match, also silently clears a + second, unrelated match in the same fenced block.""" + root = _repo(tmp_path) + _write( + root, + "skills/foo/SKILL.md", + "\n" + "```bash\n" + "echo x | sed 's/a/b/'\n" + "git log --oneline | python3 unrelated_check.py\n" + "```\n", + ) + assert gate.find_violations(root) == [] diff --git a/tests/test_gitapex_gate_unguarded_shell_pipe_in_docs_properties.py b/tests/test_gitapex_gate_unguarded_shell_pipe_in_docs_properties.py new file mode 100644 index 00000000..11e1ecfe --- /dev/null +++ b/tests/test_gitapex_gate_unguarded_shell_pipe_in_docs_properties.py @@ -0,0 +1,489 @@ +"""Hypothesis property-based layer for +``.github/scripts/gitapex_gate_unguarded_shell_pipe_in_docs.py`` (issue +#1531's own gate), added because issue #1178's +``detection-logic-property-coverage`` gate requires one for the regex-based +detection logic that gate introduces. + +Five properties, one per trigger-bearing helper function -- the example +suite next door (``tests/test_gitapex_gate_unguarded_shell_pipe_in_docs.py``) +enumerates specific input shapes by hand; these properties instead +generate the shape space each helper's own regex is meant to accept or +reject, so a boundary condition no hand-written example happens to hit still +gets exercised. Deliberately no test count cited here -- a hardcoded count +drifts the moment either file gains a test, exactly the staleness this same +review found in the sibling ``tests/test_gitapex_gate_no_raw_gh_cli_in_docs_ +properties.py``'s own "32 tests" claim (real count higher). + +Which properties are model-based +--------------------------------- +* :func:`test_fenced_line_ranges_pairs_fences_by_commonmark_run_length` -- + **model-based**. Adapted from the identical generative model + ``tests/test_gitapex_gate_no_raw_gh_cli_in_docs_properties.py`` already + validated for the sibling gate's own ``_fenced_line_ranges`` (byte-for-byte + the same function, copied rather than imported -- see this gate's own + module docstring for why one copy per gate is this repository's existing + convention). The generator holds the intended block structure + independently of the function under test, so a fence-pairing regression + (closing a longer fence on a shorter nested marker of the same character) + fails against it. +* :func:`test_pipe_match_requires_a_recognized_consumer_token_after_a_single_pipe` + -- **model-based**. The generator draws separately from + ``gate._PIPE_CONSUMERS`` (must match) and a disjoint pool of non-consumer + words (must not match), so a boundary defect in either direction fails. + Confirmed live against an injected defect: dropping the trailing ``\\b`` + makes a non-consumer word that merely *starts* with a consumer token + (``pythonic``) match, and this property catches it. +* :func:`test_has_pipefail_disclosure_finds_a_planted_case_insensitive_occurrence` + and :func:`test_has_pipefail_disclosure_is_false_with_no_planted_occurrence` + -- **model-based**. The generator plants (or withholds) ``pipefail`` in a + randomly-cased spelling at a random position in otherwise random text, so + the oracle (whether it planted the substring) is independent of the + regex under test. +* :func:`test_has_allow_marker_accepts_only_a_valid_marker_directly_above` -- + **model-based**, adapted from the sibling gate's own identical property + (same marker grammar, different token name). The generator knows which + line is a well-formed marker and where it placed it, so both an + over-permissive regex and an off-by-one in the inspected line fail. +* :func:`test_effective_line_joins_a_real_continuation` and + :func:`test_effective_line_does_not_join_a_non_continuation` -- + **model-based**. Added after a blast-radius review found this gate's + own regex could not see a pipe split across a shell line-continuation + (live-confirmed against 13 real, previously-undetected instances of + issue #1531's own defect class already in this repository). The + generator holds which previous-line shape is a genuine continuation and + which is not (plain text, empty, an escaped ``\\\\``), independently of + `_effective_line`'s own join logic. + +Reproducibility: ``derandomize=True`` with an explicit ``max_examples`` and +``deadline=None``, applied per property rather than as a global Hypothesis +profile -- the same rationale +``tests/test_gitapex_gate_no_raw_gh_cli_in_docs_properties.py``'s own module +docstring gives (this repository runs pytest under ``-n auto``, where a +randomly-seeded generator turns a latent failure into an intermittently red +suite, and a wall-clock deadline measures CI scheduling noise rather than +the code under test). +""" + +from __future__ import annotations + +from typing import NamedTuple + +import gitapex_gate_unguarded_shell_pipe_in_docs as gate +from hypothesis import given, settings +from hypothesis import strategies as st + +_PROPERTIES = settings(derandomize=True, max_examples=200, deadline=None) + + +# ========================================================================== +# `_fenced_line_ranges` -- model-based, adapted from the sibling gate's own +# already-validated generative model. +# ========================================================================== + +_PROSE = ( + "", + "Ordinary prose line.", + "## A heading", + "- a list item", + "text mentioning ``` inline, not at line start", + " indented prose", + "~~ two tildes only", + "`` two backticks only", +) + +_INFO_STRINGS = ("", "bash", "markdown", "text", "json", "console") +_INDENTS = ("", " ", " ") +_CLOSE_TRAILING = ("", " ", "\t", " ") + + +class _FenceBlock(NamedTuple): + char: str + open_len: int + close_extra: int + info: str + indent: str + close_trailing: str + decoys: tuple[int, ...] + + +def _to_fence_block(raw: tuple[bool, int, int, int, int, int, list[int]]) -> _FenceBlock: + backtick, open_len, close_extra, info_index, indent_index, trailing_index, decoys = raw + return _FenceBlock( + char="`" if backtick else "~", + open_len=open_len, + close_extra=close_extra, + info=_INFO_STRINGS[info_index % len(_INFO_STRINGS)], + indent=_INDENTS[indent_index % len(_INDENTS)], + close_trailing=_CLOSE_TRAILING[trailing_index % len(_CLOSE_TRAILING)], + decoys=tuple(decoys), + ) + + +def _decoy_pool(char: str, open_len: int) -> tuple[str, ...]: + other = "~" if char == "`" else "`" + pool = [ + "content line inside the block", + f"echo {char * 2}", + other * 3, + other * (open_len + 2), + f"{char * open_len} not a bare run", + f"{char * (open_len + 3)}info-string", + f" {other * open_len} ", + "", + ] + pool.extend(char * shorter for shorter in range(3, open_len)) + return tuple(pool) + + +def _render_fence_document( + sections: list[tuple[list[int], _FenceBlock]], tail: list[int], unclosed: bool +) -> tuple[list[str], list[tuple[int, int]]]: + lines: list[str] = [] + expected: list[tuple[int, int]] = [] + for index, (prose_choices, block) in enumerate(sections): + lines.extend(_PROSE[choice % len(_PROSE)] for choice in prose_choices) + open_line = len(lines) + 1 + lines.append(f"{block.indent}{block.char * block.open_len}{block.info}") + pool = _decoy_pool(block.char, block.open_len) + lines.extend(pool[choice % len(pool)] for choice in block.decoys) + if unclosed and index == len(sections) - 1: + expected.append((open_line, 0)) + else: + close_len = block.open_len + block.close_extra + lines.append(f"{block.indent}{block.char * close_len}{block.close_trailing}") + expected.append((open_line, len(lines))) + lines.extend(_PROSE[choice % len(_PROSE)] for choice in tail) + if expected and expected[-1][1] == 0: + expected[-1] = (expected[-1][0], len(lines) + 1) + return lines, expected + + +_FENCE_BLOCKS = st.tuples( + st.booleans(), + st.integers(min_value=3, max_value=9), + st.integers(min_value=0, max_value=3), + st.integers(min_value=0, max_value=99), + st.integers(min_value=0, max_value=99), + st.integers(min_value=0, max_value=99), + st.lists(st.integers(min_value=0, max_value=99), max_size=5), +).map(_to_fence_block) + +_FENCE_SECTIONS = st.tuples(st.lists(st.integers(min_value=0, max_value=99), max_size=3), _FENCE_BLOCKS) + +_NESTING_BLOCKS = st.tuples( + st.just(True), + st.integers(min_value=4, max_value=7), + st.integers(min_value=0, max_value=3), + st.integers(min_value=0, max_value=99), + st.integers(min_value=0, max_value=99), + st.integers(min_value=0, max_value=99), + st.lists(st.integers(min_value=0, max_value=99), max_size=3), +).map(_to_fence_block) + + +def _with_forced_nesting(block: _FenceBlock) -> _FenceBlock: + pool = _decoy_pool(block.char, block.open_len) + shorter = (pool.index(block.char * 3), pool.index(block.char * (block.open_len - 1))) + return block._replace(decoys=shorter + block.decoys) + + +def _bare_run_length(stripped: str, char: str) -> int: + return len(stripped) if stripped and set(stripped) == {char} else 0 + + +@_PROPERTIES +@given( + sections=st.lists(_FENCE_SECTIONS, max_size=3), + nesting=st.tuples(st.lists(st.integers(min_value=0, max_value=99), max_size=3), _NESTING_BLOCKS), + position=st.integers(min_value=0, max_value=99), + tail=st.lists(st.integers(min_value=0, max_value=99), max_size=3), + unclosed=st.booleans(), +) +def test_fenced_line_ranges_pairs_fences_by_commonmark_run_length( + sections: list[tuple[list[int], _FenceBlock]], + nesting: tuple[list[int], _FenceBlock], + position: int, + tail: list[int], + unclosed: bool, +) -> None: + """Fence pairing follows CommonMark's run-length rule for every generated + nesting/sibling structure: a fence closes only on a bare run of the same + marker character at least as long as the one that opened it. + + Every generated example forces at least one block opened with four to + seven backticks whose body carries a bare three-backtick line -- the + shape a naive `startswith` toggle would mistake for a close. Flip-tested + against that exact defect: reverting `_fenced_line_ranges` to a naive + `stripped.startswith(char * 3)` toggle fails this property; the current + run-length-aware implementation passes. + """ + prose_choices, block = nesting + ordered = list(sections) + ordered.insert(position % (len(ordered) + 1), (prose_choices, _with_forced_nesting(block))) + lines, expected = _render_fence_document(ordered, tail, unclosed) + + ranges = gate._fenced_line_ranges(lines) + + assert ranges == expected + + previous_close = 0 + for open_line, close_line in ranges: + assert open_line > previous_close + assert close_line >= open_line + previous_close = close_line + opened = lines[open_line - 1].strip() + marker = opened[0] + open_run = len(opened) - len(opened.lstrip(marker)) + if close_line == len(lines) + 1: + continue + closed = lines[close_line - 1].strip() + assert _bare_run_length(closed, marker) >= open_run + + +# ========================================================================== +# `_pipe_match` -- model-based. +# ========================================================================== + +_CONSUMERS = tuple(sorted(gate._PIPE_CONSUMERS)) + +# Words that must never match: each is either not in `_PIPE_CONSUMERS` at +# all, or a near-miss on the trailing `\b` (a consumer token as a strict +# prefix of a longer word). +_NON_CONSUMERS = ( + "None", + "int", + "str", + "cat", + "curl", + "echo", + "pythonic", + "basher", + "unix", + "notaconsumer", +) + +_LEFT_TOKENS = ("cmd", "git", "$(cmd)", "1", "value", "a-b_c.d") +_SEPARATORS = (" ", " ", "\t", "") +_SUFFIXES = ("", " arg", " --flag value", " x.py") + + +@_PROPERTIES +@given( + left=st.sampled_from(_LEFT_TOKENS), + left_sep=st.sampled_from(_SEPARATORS), + right_sep=st.sampled_from(_SEPARATORS), + consumer=st.sampled_from(_CONSUMERS), + suffix=st.sampled_from(_SUFFIXES), +) +def test_pipe_match_requires_a_recognized_consumer_token_after_a_single_pipe( + left: str, left_sep: str, right_sep: str, consumer: str, suffix: str +) -> None: + """Every generated ` | ` line matches, for every + consumer token the gate's own vocabulary declares and every whitespace + variant around the pipe. + + **Model-based**: `consumer` is drawn directly from `gate._PIPE_CONSUMERS` + (the gate's own vocabulary), so this does not detect a *stale* + vocabulary (a real shell tool missing from it) -- the same disclosed + residual risk the sibling gate's own subcommand-vocabulary property + carries. It does detect a regex regression that stops matching a + registered token, or that stops matching one of the generated whitespace + variants. + """ + line = f"{left}{left_sep}|{right_sep}{consumer}{suffix}" + match = gate._pipe_match(line) + assert match is not None + assert match.group(0).endswith(consumer) or consumer in match.group(0) + + +@_PROPERTIES +@given( + left=st.sampled_from(_LEFT_TOKENS), + left_sep=st.sampled_from(_SEPARATORS), + right_sep=st.sampled_from(_SEPARATORS), + non_consumer=st.sampled_from(_NON_CONSUMERS), + suffix=st.sampled_from(_SUFFIXES), +) +def test_pipe_match_rejects_a_non_consumer_token_after_a_single_pipe( + left: str, left_sep: str, right_sep: str, non_consumer: str, suffix: str +) -> None: + """The mirror property: a ` | ` line -- a + Python type hint (`list[str] | None`), a table cell, or a near-miss on + the trailing word boundary (`pythonic`, `basher`) -- never matches. + + Confirmed live: dropping the trailing `\\b` from `_PIPE_RE` makes + `pythonic`/`basher` (both a registered consumer as a strict prefix) + match, and this property catches it. + """ + line = f"{left}{left_sep}|{right_sep}{non_consumer}{suffix}" + assert gate._pipe_match(line) is None + + +@_PROPERTIES +@given(left=st.sampled_from(_LEFT_TOKENS), consumer=st.sampled_from(_CONSUMERS)) +def test_pipe_match_rejects_a_double_pipe(left: str, consumer: str) -> None: + """`||` (logical OR) never matches, regardless of what follows it -- + this gate's own risk class is specific to a single unguarded pipe.""" + assert gate._pipe_match(f"{left} || {consumer}") is None + + +# ========================================================================== +# `_has_pipefail_disclosure` -- model-based. +# ========================================================================== + +_PIPEFAIL_SPELLINGS = ("pipefail", "PIPEFAIL", "PipeFail", "pIpEfAil") +_FILLER = ("", "some prose ", "set -o ", "line one\nline two\n") + + +@_PROPERTIES +@given( + before=st.sampled_from(_FILLER), + spelling=st.sampled_from(_PIPEFAIL_SPELLINGS), + after=st.sampled_from(_FILLER), +) +def test_has_pipefail_disclosure_finds_a_planted_case_insensitive_occurrence( + before: str, spelling: str, after: str +) -> None: + """`pipefail`, in any letter-casing, planted anywhere in the text, is + always found.""" + assert gate._has_pipefail_disclosure(f"{before}{spelling}{after}") is True + + +@_PROPERTIES +@given(text=st.sampled_from((*_FILLER, "no risky word here", "PIPE FAIL (split, not planted)"))) +def test_has_pipefail_disclosure_is_false_with_no_planted_occurrence(text: str) -> None: + """The mirror property: text with no `pipefail` substring planted (the + filler pool itself, plus a split near-miss) is never reported as + disclosed.""" + assert gate._has_pipefail_disclosure(text) is False + + +# ========================================================================== +# `_has_allow_marker` -- model-based, adapted from the sibling gate's own +# identical property (same marker grammar, different token name). +# ========================================================================== + +_REASONS = ("historical", "illustrative prose, not an instruction to run", "x", "a --> b", "issue #1531") + +_VALID_MARKER_SPACINGS = ( + ("", " ", "", " ", ""), + (" ", "", " ", "", ""), + ("\t", " ", " ", " ", " "), + ("", "", "", "", ""), + (" ", "\t", "\t", "\t", "\t"), +) + +_INVALID_MARKERS = ( + "", + "", + "", + "", + "", + " tail", + "", + "", +) + +_MARKER_PROSE = ("", "Ordinary prose above a fence.", "```bash", "") + + +def _valid_marker_line(reason: str, spacing: tuple[str, str, str, str, str]) -> str: + indent, after_open, before_colon, after_colon, trailing = spacing + return ( + f"{indent}{trailing}" + ) + + +_MARKER_LINES = st.one_of( + st.builds(_valid_marker_line, st.sampled_from(_REASONS), st.sampled_from(_VALID_MARKER_SPACINGS)).map( + lambda text: (True, text) + ), + st.sampled_from(_INVALID_MARKERS).map(lambda text: (False, text)), + st.sampled_from(_MARKER_PROSE).map(lambda text: (False, text)), +) + + +@_PROPERTIES +@given(candidates=st.lists(_MARKER_LINES, min_size=1, max_size=6), open_choice=st.integers(min_value=0, max_value=99)) +def test_has_allow_marker_accepts_only_a_valid_marker_directly_above( + candidates: list[tuple[bool, str]], open_choice: int +) -> None: + """The exemption holds exactly when the line directly above the given + index is a syntactically valid `gitapex-allow-unguarded-shell-pipe` + marker -- not one line higher, not the line itself, and not a marker + missing its reason, its colon, its closing `-->`, or its line + anchoring. + + **Model-based** on two axes at once: validity (each generated line is + drawn from a pool that knows whether it is well-formed) and position + (the inspected index is drawn independently of where the valid marker + landed). + """ + lines = [text for _, text in candidates] + index = 1 + open_choice % len(lines) + expected = index >= 2 and candidates[index - 2][0] + + assert gate._has_allow_marker(lines, index) is expected + + +# ========================================================================== +# `_effective_line` -- model-based. +# ========================================================================== + +_CONTINUATION_TRAILING = ("\\", "\\ ", "\\\t") +_NON_CONTINUATION_LINES = ("plain line", "", "ends in backslash-backslash \\\\", "trailing space \\ x") +# Letters/digits/spaces only (no backslash, no leading/trailing whitespace of +# their own) so the join arithmetic below is unambiguous -- an alphabet wide +# enough to include internal whitespace would make "where did next_body's own +# leading space go" ambiguous between the two lines, a question about the +# generator's own construction, not about `_effective_line`. +_WORD_TEXT = st.text(alphabet=st.characters(whitelist_categories=("Ll", "Lu", "Nd")), max_size=12) + + +@_PROPERTIES +@given( + prefix=_WORD_TEXT, + trailing=st.sampled_from(_CONTINUATION_TRAILING), + next_leading=st.sampled_from(("", " ", " ", "\t")), + next_body=_WORD_TEXT, +) +def test_effective_line_joins_a_real_continuation( + prefix: str, trailing: str, next_leading: str, next_body: str +) -> None: + """A previous line ending in a bare backslash (optionally followed by + only whitespace) is joined with the next line's own left-stripped + content -- the same join a shell performs before executing a + backslash-continued command. + + **Model-based**: the generator holds the intended previous/next split + and the expected joined form independently of `_effective_line` + itself, so a regression that failed to strip the trailing backslash, + or that failed to strip the next line's own leading whitespace, fails + against it. + """ + lines = [prefix + trailing, next_leading + next_body] + expected = prefix + " " + next_body + assert gate._effective_line(lines, 1) == expected + + +@_PROPERTIES +@given(non_continuation=st.sampled_from(_NON_CONTINUATION_LINES), next_line=_WORD_TEXT) +def test_effective_line_does_not_join_a_non_continuation(non_continuation: str, next_line: str) -> None: + """The mirror property: a previous line that does not end in a bare + backslash -- plain text, empty, an escaped `\\\\`, or a backslash + followed by non-whitespace -- leaves the next line unchanged. + + Confirmed live: dropping the `not previous.endswith("\\\\\\\\")` guard + makes an escaped-backslash line (`ends in backslash-backslash \\\\`) + wrongly join with its successor, and this property catches it. + """ + lines = [non_continuation, next_line] + assert gate._effective_line(lines, 1) == next_line + + +@_PROPERTIES +@given(line=_WORD_TEXT) +def test_effective_line_returns_the_first_line_unchanged(line: str) -> None: + """Index 0 has no predecessor to join with.""" + assert gate._effective_line([line], 0) == line