From 4c97f8182c0502d69f4f3a9ba4554542b45e17f1 Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Tue, 18 Aug 2026 20:54:51 +0000 Subject: [PATCH 1/7] =?UTF-8?q?feat:=20implement=20issue=20#1493=20?= =?UTF-8?q?=E2=80=94=20[Phase=202=20follow-up]=20Complete=20the=20#1184=20?= =?UTF-8?q?legacy=E2=86=92major-scoped=20pin=20migration=20in=20the=20reus?= =?UTF-8?q?able-pin=20compliance=20check=20(split=20from=20#687=20ACs=207?= =?UTF-8?q?=E2=80=9313)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .github/workflows/test-dev-lead.yml | 4 +- AGENTS.md | 9 +- docs/aw/ci-failure-analyst.md | 2 +- templates/ci-failure-analyst.yml | 2 +- .../test_reusable_pin_compliance.py | 201 +++++++++++++++--- ...test_reusable_pin_compliance_classifier.py | 194 +++++++++++++++++ tests/test_seed_repo_template.bats | 20 ++ 7 files changed, 397 insertions(+), 35 deletions(-) create mode 100644 tests/dev-lead/integration/test_reusable_pin_compliance_classifier.py diff --git a/.github/workflows/test-dev-lead.yml b/.github/workflows/test-dev-lead.yml index 2460a1bdb..05e2d5ebb 100644 --- a/.github/workflows/test-dev-lead.yml +++ b/.github/workflows/test-dev-lead.yml @@ -214,7 +214,9 @@ jobs: persist-credentials: false - name: Install PyYAML run: python3 -m pip install --quiet --only-binary ":all:" pyyaml==6.0.3 - - name: Reject SHA / off-channel pins on any first-party reusable + - name: Classifier semantics (canonical / DEPRECATED-legacy / violation) — #1493 + run: python3 tests/dev-lead/integration/test_reusable_pin_compliance_classifier.py + - name: Reject SHA / off-channel pins; inventory legacy → major-scoped migration run: python3 tests/dev-lead/integration/test_reusable_pin_compliance.py spec-drift-workflow: diff --git a/AGENTS.md b/AGENTS.md index d133978fb..646d1f3b0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -431,7 +431,14 @@ all of them.** Two kinds of tag exist per reusable: - **Immutable releases** `/vX.Y.Z` — never moved or deleted; the audit trail and rollback targets. - **Moving channel tags** `/v-` (major-scoped, #1184; tiers `stable`, and where live - `next`/`ring0`/`ring1`) — what callers pin to; advanced on promotion by **moving** the tag. + `next`/`ring0`/`ring1`) — the **canonical** form callers pin to; advanced on promotion by **moving** the tag. + Examples: `@dev-lead/v1-stable`, `@pr-review/v1-next`, `@ci-failure-analyst/v1-stable`. +- **Legacy bare-tier channel** `/` (e.g. `@pr-review/next`) — the pre-#1184 form, **DEPRECATED**. + It is still the sanctioned mutable-ref exception (never flagged as an "unpinned action"), but it is being + migrated to the major-scoped canonical form. The `reusable-pin-compliance` check + (`tests/dev-lead/integration/test_reusable_pin_compliance.py`) reports a bare-tier pin as DEPRECATED with the + major-scoped ref it should become; during the migration window this is a **warning, not a failure** + (#687 AC #8), so `main` stays green until every legacy pin is repinned. **Scoped exception to the SHA-pin standard.** The org standard requires SHA-pinning actions to avoid mutable-ref supply-chain risk. That rule targets **third-party** actions. The channel tags above are diff --git a/docs/aw/ci-failure-analyst.md b/docs/aw/ci-failure-analyst.md index ec8beeda2..0546479f1 100644 --- a/docs/aw/ci-failure-analyst.md +++ b/docs/aw/ci-failure-analyst.md @@ -22,7 +22,7 @@ The workflow uses the **reusable workflow** (`workflow_call`) pattern — the sa ``` repo-x (check_run: failure) └─ .github/workflows/ci-failure-analyst.yml ← ~20-line stub (no logic) - └─ uses: petry-projects/.github-private/.github/workflows/ci-failure-analyst-reusable.yml@ci-failure-analyst/stable + └─ uses: petry-projects/.github-private/.github/workflows/ci-failure-analyst-reusable.yml@ci-failure-analyst/v1-stable ├─ resolves PR (non-fork only) ├─ idempotency check () ├─ installs Claude Code diff --git a/templates/ci-failure-analyst.yml b/templates/ci-failure-analyst.yml index b4574e4aa..1a622e263 100644 --- a/templates/ci-failure-analyst.yml +++ b/templates/ci-failure-analyst.yml @@ -41,6 +41,6 @@ jobs: if: >- github.event.check_run.conclusion == 'failure' && !startsWith(github.event.check_run.name, 'CI Failure Analyst') - uses: petry-projects/.github-private/.github/workflows/ci-failure-analyst-reusable.yml@ci-failure-analyst/stable # NOSONAR(githubactions:S7637) first-party channel ref + uses: petry-projects/.github-private/.github/workflows/ci-failure-analyst-reusable.yml@ci-failure-analyst/v1-stable # NOSONAR(githubactions:S7637) first-party channel ref secrets: CLAUDE_CODE_OAUTH_TOKEN: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} diff --git a/tests/dev-lead/integration/test_reusable_pin_compliance.py b/tests/dev-lead/integration/test_reusable_pin_compliance.py index cde40def1..3fdb8c099 100644 --- a/tests/dev-lead/integration/test_reusable_pin_compliance.py +++ b/tests/dev-lead/integration/test_reusable_pin_compliance.py @@ -17,9 +17,14 @@ `@ref` (GitHub uses the caller's own commit) and are out of scope. Third-party actions still follow the SHA-pin standard and are untouched. -Sanctioned pins (AC #3): a channel tag `/{stable,next,ring}` or its -major-scoped form `/v-{stable,next,ring}`, an immutable -`/vMAJOR.MINOR.PATCH` release tag, or a bare `@vN`. +Canonical pins (#687 AC #7, #1184): the *canonical* channel is the major-scoped +form `/v-{stable,next,ring}`, an immutable +`/vMAJOR.MINOR.PATCH` release tag, or a bare `@vN`. A bare +`/{stable,next,ring}` channel is the pre-#1184 **legacy** form: it is +reported as DEPRECATED (with file, line, and the major-scoped ref it should +become) but, during the migration window, is a WARNING — not a failure — so a +repo carrying a not-yet-migrated legacy pin keeps `main` green (#687 AC #8). The +enforcement flip to a hard failure is a separate, later change (#687 AC #12). Usage: python3 tests/dev-lead/integration/test_reusable_pin_compliance.py [FILE ...] @@ -28,6 +33,7 @@ """ from __future__ import annotations +import os import re import sys from pathlib import Path @@ -52,7 +58,10 @@ ) SHA_RE = re.compile(r"^[0-9a-fA-F]{40}$") -CHANNEL_RE = re.compile(r"^[a-z0-9][a-z0-9-]*/(?:stable|next|ring\d+)$") +# Legacy (pre-#1184) bare-tier channel — DEPRECATED, not canonical (#687 AC #7/#8). +CHANNEL_LEGACY_RE = re.compile( + r"^(?P[a-z0-9][a-z0-9-]*)/(?Pstable|next|ring\d+)$" +) CHANNEL_MAJOR_RE = re.compile(r"^[a-z0-9][a-z0-9-]*/v\d+-(?:stable|next|ring\d+)$") SEMVER_RE = re.compile(r"^[a-z0-9][a-z0-9-]*/v\d+\.\d+\.\d+$") BARE_VN_RE = re.compile(r"^v\d+$") @@ -74,32 +83,47 @@ def is_first_party_reusable(path: str) -> bool: ) -def is_sanctioned_tag(ref: str) -> bool: +def is_canonical_tag(ref: str) -> bool: + """A canonical pin: major-scoped channel, immutable semver, or a bare @vN (#1184).""" return bool( - CHANNEL_RE.match(ref) - or CHANNEL_MAJOR_RE.match(ref) + CHANNEL_MAJOR_RE.match(ref) or SEMVER_RE.match(ref) or BARE_VN_RE.match(ref) ) -def classify_ref(ref: str) -> str | None: - """Return a violation reason, or None when the ref is a sanctioned pin.""" +def classify_ref(ref: str) -> tuple[str, str | None]: + """Classify a first-party reusable `@ref`. + + Returns ``(status, detail)`` where ``status`` is one of: + - ``"ok"`` — a canonical pin; ``detail`` is ``None``. + - ``"deprecated"`` — a legacy bare `/` channel (#1184); ``detail`` + is the major-scoped ref it should become + (``/v-``). A WARNING, not a failure. + - ``"violation"`` — a hard violation (SHA / off-channel / unsanctioned / + missing ref); ``detail`` is the human-readable reason. + """ if ref == "": - return "no @ref — a first-party reusable must pin a sanctioned channel/version tag" + return "violation", "no @ref — a first-party reusable must pin a canonical channel/version tag" if SHA_RE.match(ref): - return ( + return "violation", ( f"SHA-pinned (@{ref}) — first-party reusables must pin a channel/version " "tag, not a 40-hex SHA" ) if ref in ("main", "master") or ref.endswith("/main") or ref.endswith("/master"): - return f"off-channel pin (@{ref}) — must pin a sanctioned channel/version tag, not main/master" - if not is_sanctioned_tag(ref): - return ( - f"unsanctioned ref (@{ref}) — expected /{{stable,next,ring}}, " - "/v-, /vX.Y.Z, or @vN" + return "violation", ( + f"off-channel pin (@{ref}) — must pin a canonical channel/version tag, not main/master" + ) + m = CHANNEL_LEGACY_RE.match(ref) + if m: + suggestion = f"{m.group('name')}/v-{m.group('tier')}" + return "deprecated", suggestion + if not is_canonical_tag(ref): + return "violation", ( + f"unsanctioned ref (@{ref}) — expected /v-, " + "/vX.Y.Z, or @vN" ) - return None + return "ok", None def off_channel_comment(comment: str | None) -> bool: @@ -135,22 +159,59 @@ def raw_comment_map(text: str) -> dict[tuple[str, str], str]: return out -def scan_file(path: Path) -> tuple[list[str], int]: - """Return (violations, first_party_refs_checked) for one workflow/template file.""" +def uses_line_map(text: str) -> dict[tuple[str, str], int]: + """Map (job_id, uses_val) to the 1-based line number of its `uses:` line.""" + out: dict[tuple[str, str], int] = {} + current_job: str | None = None + in_jobs = False + for i, line in enumerate(text.splitlines(), start=1): + if line.strip() == "jobs:": + in_jobs = True + continue + if in_jobs: + m_job = re.match(r"^ (?P[A-Za-z0-9_-]+):\s*(?:#.*)?$", line) + if m_job: + current_job = m_job.group("job_id") + continue + elif re.match(r"^[^#\s]", line): + in_jobs = False + current_job = None + continue + if current_job: + m_uses = USES_LINE_RE.match(line) + if m_uses: + val = m_uses.group("val").strip().strip('"').strip("'") + out.setdefault((current_job, val), i) + return out + + +def scan_file(path: Path) -> tuple[list[str], list[dict], int, int]: + """Scan one workflow/template file for first-party reusable pins. + + Returns ``(violations, deprecations, checked, canonical)``: + - ``violations`` — hard-failure strings (SHA/off-channel/unsanctioned/…). + - ``deprecations`` — legacy bare-tier pins, each a dict with keys + ``path``, ``job``, ``line``, ``ref``, ``suggestion`` (#687 AC #8/#9). + - ``checked`` — count of first-party reusable refs inspected. + - ``canonical`` — count of those that are canonical (major-scoped/semver/@vN). + """ text = path.read_text(encoding="utf-8") try: doc = yaml.safe_load(text) except yaml.YAMLError as err: - return [f"{path}: not valid YAML: {err}"], 0 + return [f"{path}: not valid YAML: {err}"], [], 0, 0 if not isinstance(doc, dict): - return [], 0 + return [], [], 0, 0 jobs = doc.get("jobs") if not isinstance(jobs, dict): - return [], 0 + return [], [], 0, 0 comments = raw_comment_map(text) + lines = uses_line_map(text) violations: list[str] = [] + deprecations: list[dict] = [] checked = 0 + canonical = 0 for job_id, job in jobs.items(): if not isinstance(job, dict): @@ -162,16 +223,28 @@ def scan_file(path: Path) -> tuple[list[str], int]: if not is_first_party_reusable(job_path): continue checked += 1 - reason = classify_ref(ref) - if reason: - violations.append(f"{path}: job '{job_id}': {reason}") + status, detail = classify_ref(ref) + if status == "violation": + violations.append(f"{path}: job '{job_id}': {detail}") + elif status == "deprecated": + deprecations.append( + { + "path": str(path), + "job": job_id, + "line": lines.get((job_id, uses), 0), + "ref": ref, + "suggestion": detail, + } + ) + else: + canonical += 1 comment = comments.get((job_id, uses)) if off_channel_comment(comment): violations.append( f"{path}: job '{job_id}': off-channel `{comment.strip()}` annotation " "on a first-party reusable pin" ) - return violations, checked + return violations, deprecations, checked, canonical def collect_files(argv: list[str]) -> tuple[list[Path], bool]: @@ -188,25 +261,80 @@ def collect_files(argv: list[str]) -> tuple[list[Path], bool]: return files, False +def emit_inventory(deprecations: list[dict], checked: int, canonical: int) -> None: + """Print the legacy→major-scoped inventory + counts, and append them to the + GitHub run summary when `$GITHUB_STEP_SUMMARY` is set (#687 AC #9).""" + legacy = len(deprecations) + print( + f"\nPin inventory (#1184 migration): {canonical} canonical (major-scoped) / " + f"{legacy} legacy (DEPRECATED) of {checked} first-party pin(s) checked." + ) + for d in deprecations: + print( + f" DEPRECATED: {d['path']}:{d['line']} job '{d['job']}' pins " + f"@{d['ref']} — repin to @{d['suggestion']} (bare / is the " + "pre-#1184 legacy form)." + ) + + summary_path = os.environ.get("GITHUB_STEP_SUMMARY") + if not summary_path: + return + rows = [ + "### Reusable-pin migration inventory (#1184)", + "", + f"- **canonical (major-scoped)**: {canonical}", + f"- **legacy (DEPRECATED)**: {legacy}", + f"- **checked**: {checked}", + "", + ] + if deprecations: + rows.append("| file | line | job | current (legacy) | should become |") + rows.append("|---|---|---|---|---|") + for d in deprecations: + rows.append( + f"| `{d['path']}` | {d['line']} | `{d['job']}` | " + f"`@{d['ref']}` | `@{d['suggestion']}` |" + ) + rows.append("") + rows.append( + "> Legacy pins are a **warning** during the #1184 migration window; " + "enforcement flips to a failure only after the migration is complete " + "(#687 AC #12)." + ) + else: + rows.append("No legacy bare-tier pins remain. ✅") + rows.append("") + with open(summary_path, "a", encoding="utf-8") as fh: + fh.write("\n".join(rows)) + + def main(argv: list[str]) -> int: files, explicit = collect_files(argv[1:]) violations: list[str] = [] + deprecations: list[dict] = [] checked = 0 + canonical = 0 for f in files: if not f.is_file(): print(f"FAIL: {f} not found") return 1 - file_violations, file_checked = scan_file(f) + file_violations, file_deprecations, file_checked, file_canonical = scan_file(f) violations.extend(file_violations) + deprecations.extend(file_deprecations) checked += file_checked + canonical += file_canonical + + # Inventory + counts are always emitted (green or not) so the run summary is a + # complete, machine-readable census of the migration (#687 AC #9). + emit_inventory(deprecations, checked, canonical) if violations: - print(f"FAIL: {len(violations)} first-party reusable pin compliance violation(s):") + print(f"\nFAIL: {len(violations)} first-party reusable pin compliance violation(s):") for v in violations: print(f" {v}") print( - "\n Fix: pin first-party reusables to a sanctioned channel/version tag " - "(/stable, /v-, /vX.Y.Z, or @vN).\n" + "\n Fix: pin first-party reusables to a canonical channel/version tag " + "(/v-, /vX.Y.Z, or @vN).\n" " See AGENTS.md 'Release channel tags & the mutable-ref exception'." ) return 1 @@ -218,7 +346,18 @@ def main(argv: list[str]) -> int: ) return 1 - print(f"PASS: {checked} first-party reusable pin(s) across {len(files)} file(s) compliant") + if deprecations: + # A warning, not a failure: `main` stays green while the migration proceeds + # (#687 AC #8). The enforcement flip is a separate change (#687 AC #12). + print( + f"\nPASS (with warnings): {canonical} canonical pin(s) compliant; " + f"{len(deprecations)} legacy pin(s) DEPRECATED and pending repin." + ) + else: + print( + f"\nPASS: {checked} first-party reusable pin(s) across {len(files)} " + "file(s) compliant (all canonical)." + ) return 0 diff --git a/tests/dev-lead/integration/test_reusable_pin_compliance_classifier.py b/tests/dev-lead/integration/test_reusable_pin_compliance_classifier.py new file mode 100644 index 000000000..99046d53f --- /dev/null +++ b/tests/dev-lead/integration/test_reusable_pin_compliance_classifier.py @@ -0,0 +1,194 @@ +#!/usr/bin/env python3 +"""Classifier unit/behavior tests for the reusable-pin compliance scanner (#1493). + +Exercises the #1184 legacy→major-scoped migration semantics that #687 ACs 7–9 add +on top of the original #687 scanner: + + - AC 7: the *canonical* form is the major-scoped channel `/v-` + (or an immutable `/vX.Y.Z`); a bare `/` is NOT canonical. + - AC 8: a bare `/` is classified DEPRECATED — reported with file, line, + and the major-scoped ref it should become — and during the migration window this + is a WARNING, not a failure (exit 0 so `main` stays green). + - AC 9: every legacy pin is inventoried with counts. + +Standalone runner (repo convention: `python3 `, exit 0 = pass). No pytest. +""" +from __future__ import annotations + +import importlib.util +import os +import subprocess +import sys +import tempfile +from pathlib import Path + +HERE = Path(__file__).resolve().parent +SCANNER = HERE / "test_reusable_pin_compliance.py" + +_spec = importlib.util.spec_from_file_location("pin_scanner", SCANNER) +assert _spec and _spec.loader +mod = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(mod) # type: ignore[union-attr] + +_failures: list[str] = [] + + +def check(cond: bool, msg: str) -> None: + if not cond: + _failures.append(msg) + print(f" FAIL: {msg}") + else: + print(f" ok: {msg}") + + +def test_classify_ref_canonical() -> None: + print("test_classify_ref_canonical") + for ref in ( + "dev-lead/v1-stable", + "pr-review/v2-next", + "agent-shield/v2-ring0", + "ci-failure-analyst/v0-stable", + "dev-lead/v1.2.3", + "v1", + ): + status, detail = mod.classify_ref(ref) + check(status == "ok" and detail is None, f"{ref!r} is canonical (ok) -> {status,detail}") + + +def test_classify_ref_deprecated() -> None: + print("test_classify_ref_deprecated") + cases = { + "pr-review/next": "pr-review/v-next", + "ci-failure-analyst/stable": "ci-failure-analyst/v-stable", + "dev-lead/ring1": "dev-lead/v-ring1", + } + for ref, want in cases.items(): + status, detail = mod.classify_ref(ref) + check( + status == "deprecated" and detail == want, + f"{ref!r} is DEPRECATED with suggestion {want!r} -> {status,detail}", + ) + + +def test_classify_ref_violation() -> None: + print("test_classify_ref_violation") + for ref in ( + "", + "a" * 40, # SHA + "main", + "dev-lead/main", + "not-a-tag", + ): + status, _ = mod.classify_ref(ref) + check(status == "violation", f"{ref!r} is a violation -> {status}") + + +def _write(tmp: Path, name: str, body: str) -> Path: + p = tmp / name + p.write_text(body, encoding="utf-8") + return p + + +def test_scan_file_captures_line_and_suggestion() -> None: + print("test_scan_file_captures_line_and_suggestion") + body = ( + "name: x\n" + "on: [push]\n" + "jobs:\n" + " review:\n" + " uses: petry-projects/.github-private/.github/workflows/pr-review.yml@pr-review/next\n" + ) + with tempfile.TemporaryDirectory() as d: + p = _write(Path(d), "wf.yml", body) + violations, deprecations, checked, canonical = mod.scan_file(p) + check(not violations, f"no hard violations -> {violations}") + check(checked == 1, f"one first-party ref checked -> {checked}") + check(canonical == 0, f"zero canonical -> {canonical}") + check(len(deprecations) == 1, f"one deprecation -> {len(deprecations)}") + if deprecations: + dep = deprecations[0] + check(dep["line"] == 5, f"deprecation at line 5 -> {dep.get('line')}") + check(dep["ref"] == "pr-review/next", f"ref recorded -> {dep.get('ref')}") + check( + dep["suggestion"] == "pr-review/v-next", + f"suggestion recorded -> {dep.get('suggestion')}", + ) + + +def test_scan_file_canonical_passes_clean() -> None: + print("test_scan_file_canonical_passes_clean") + body = ( + "jobs:\n" + " dl:\n" + " uses: petry-projects/.github-private/.github/workflows/dev-lead-reusable.yml@dev-lead/v1-stable\n" + ) + with tempfile.TemporaryDirectory() as d: + p = _write(Path(d), "wf.yml", body) + violations, deprecations, checked, canonical = mod.scan_file(p) + check(not violations and not deprecations, "canonical pin is clean") + check(canonical == 1, f"one canonical -> {canonical}") + + +def _run_cli(files: list[str], env_extra: dict | None = None) -> subprocess.CompletedProcess: + env = dict(os.environ) + if env_extra: + env.update(env_extra) + return subprocess.run( + [sys.executable, str(SCANNER), *files], + capture_output=True, + text=True, + env=env, + ) + + +def test_cli_deprecated_is_warning_not_failure() -> None: + print("test_cli_deprecated_is_warning_not_failure") + body = ( + "jobs:\n" + " review:\n" + " uses: petry-projects/.github-private/.github/workflows/pr-review.yml@pr-review/next\n" + ) + with tempfile.TemporaryDirectory() as d: + summary = Path(d) / "summary.md" + p = _write(Path(d), "wf.yml", body) + proc = _run_cli([str(p)], {"GITHUB_STEP_SUMMARY": str(summary)}) + check(proc.returncode == 0, f"DEPRECATED-only exits 0 (main stays green) -> {proc.returncode}\n{proc.stdout}") + check("DEPRECATED" in proc.stdout, "output labels the pin DEPRECATED") + check("pr-review/v-next" in proc.stdout, "output shows the suggested major-scoped ref") + # AC 9: counts land in the run summary. + text = summary.read_text() if summary.exists() else "" + check("legacy" in text.lower() and "1" in text, f"run summary carries the legacy count -> {text!r}") + + +def test_cli_sha_still_fails() -> None: + print("test_cli_sha_still_fails") + body = ( + "jobs:\n" + " dl:\n" + " uses: petry-projects/.github-private/.github/workflows/dev-lead-reusable.yml@" + + "a" * 40 + + "\n" + ) + with tempfile.TemporaryDirectory() as d: + p = _write(Path(d), "wf.yml", body) + proc = _run_cli([str(p)]) + check(proc.returncode == 1, f"SHA pin still fails -> {proc.returncode}") + + +def main() -> int: + test_classify_ref_canonical() + test_classify_ref_deprecated() + test_classify_ref_violation() + test_scan_file_captures_line_and_suggestion() + test_scan_file_canonical_passes_clean() + test_cli_deprecated_is_warning_not_failure() + test_cli_sha_still_fails() + if _failures: + print(f"\nFAIL: {len(_failures)} classifier assertion(s) failed") + return 1 + print("\nPASS: reusable-pin classifier migration semantics (#1493)") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tests/test_seed_repo_template.bats b/tests/test_seed_repo_template.bats index b09d307dc..e202e79f4 100644 --- a/tests/test_seed_repo_template.bats +++ b/tests/test_seed_repo_template.bats @@ -243,6 +243,26 @@ EOF [ "$got" = "pr-review-mention/v7-stable" ] } +# AC #13 (#1493): a canonical stub ALREADY pinned @/v-stable must round-trip +# through the generator UNCHANGED — the repin is idempotent and can never reintroduce a +# legacy bare-tier /stable pin. Supersedes #1436 AC #5 (the guard #1448 references). +@test "repin round-trip: a canonical @/v-stable stub is unchanged (#1493 AC 13)" { + for name in "${CALLER_STUBS[@]}"; do + # Multi-line stub so the round-trip covers surrounding context, not just the uses: line. + host="petry-projects/.github" + [ "$name" = "dev-lead" ] && host="petry-projects/.github-private" + src=$'jobs:\n call:\n uses: '"${host}/.github/workflows/${name}-reusable.yml@${name}/v1-stable"$'\n with:\n agent_ref: '"${name}/v1-stable" + run bash -c 'printf "%s" "$1" | bash "$0" --repin "$2"' "$SEED" "$src" "$name" + [ "$status" -eq 0 ] + # Byte-for-byte identical: canonical in → canonical out (idempotent). + [ "$output" = "$src" ] \ + || { echo "$name: canonical stub was mutated by round-trip" >&2; printf 'IN:\n%s\nOUT:\n%s\n' "$src" "$output" >&2; false; } + # And it certainly must not have become the pre-#1184 legacy bare-tier pin. + [[ "$output" != *"@${name}/stable"* ]] \ + || { echo "$name: round-trip emitted legacy bare-tier channel" >&2; false; } + done +} + @test "_derive_stable_channel: FAILS LOUD on a legacy non-major-scoped ref (no v to preserve)" { # A canonical stub that is NOT major-scoped is itself a standards violation # (#1184); the derivation must error rather than silently emit a legacy pin. From 9a3d10ddd1c3dc2c1c81f58af7a36c63dd9668ef Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Tue, 18 Aug 2026 20:58:17 +0000 Subject: [PATCH 2/7] fix(reviews): address review comments [skip ci-relay] --- .../integration/test_reusable_pin_compliance_classifier.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/dev-lead/integration/test_reusable_pin_compliance_classifier.py b/tests/dev-lead/integration/test_reusable_pin_compliance_classifier.py index 99046d53f..a25022702 100644 --- a/tests/dev-lead/integration/test_reusable_pin_compliance_classifier.py +++ b/tests/dev-lead/integration/test_reusable_pin_compliance_classifier.py @@ -156,7 +156,7 @@ def test_cli_deprecated_is_warning_not_failure() -> None: check("DEPRECATED" in proc.stdout, "output labels the pin DEPRECATED") check("pr-review/v-next" in proc.stdout, "output shows the suggested major-scoped ref") # AC 9: counts land in the run summary. - text = summary.read_text() if summary.exists() else "" + text = summary.read_text(encoding="utf-8") if summary.exists() else "" check("legacy" in text.lower() and "1" in text, f"run summary carries the legacy count -> {text!r}") From 46dd9a4fb40b8d03ef275981f96fdddb05921d05 Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Tue, 18 Aug 2026 21:03:15 +0000 Subject: [PATCH 3/7] chore: dev-lead update (review-changes) [skip ci-relay] --- .../test_reusable_pin_compliance.py | 32 +++++++++++++++ ...test_reusable_pin_compliance_classifier.py | 40 +++++++++++++++++++ 2 files changed, 72 insertions(+) diff --git a/tests/dev-lead/integration/test_reusable_pin_compliance.py b/tests/dev-lead/integration/test_reusable_pin_compliance.py index 3fdb8c099..eef87b36f 100644 --- a/tests/dev-lead/integration/test_reusable_pin_compliance.py +++ b/tests/dev-lead/integration/test_reusable_pin_compliance.py @@ -126,6 +126,25 @@ def classify_ref(ref: str) -> tuple[str, str | None]: return "ok", None +def _expected_channel_name(job_path: str) -> str | None: + """Return the expected ref name-prefix for a first-party reusable path, or None. + + Derives the channel-name segment from the reusable filename: + dev-lead-reusable.yml → 'dev-lead' + pr-review.yml → 'pr-review' + Returns None when the path is not a recognised first-party reusable. + """ + m = FIRST_PARTY_PATH_RE.match(job_path) + if not m: + return None + filename = m.group("file") + for suffix in ("-reusable.yml", "-reusable.yaml"): + if filename.endswith(suffix): + return filename[: -len(suffix)] + # pr-review.yml / pr-review.yaml (special allowlisted non-reusable name) + return filename.rsplit(".", 1)[0] + + def off_channel_comment(comment: str | None) -> bool: if not comment: return False @@ -224,6 +243,19 @@ def scan_file(path: Path) -> tuple[list[str], list[dict], int, int]: continue checked += 1 status, detail = classify_ref(ref) + # Reject refs whose name prefix doesn't match the reusable filename's channel name. + # A ref like 'pr-review/v1-stable' on 'dev-lead-reusable.yml' would otherwise + # pass as canonical even though it points to the wrong reusable's channel. + if status != "violation" and "/" in ref: + ref_name = ref.split("/")[0] + expected_name = _expected_channel_name(job_path) + if expected_name and ref_name != expected_name: + filename = job_path.rsplit("/", 1)[-1] + status = "violation" + detail = ( + f"ref name mismatch: '@{ref}' does not belong to '{filename}' " + f"(expected prefix '{expected_name}/')" + ) if status == "violation": violations.append(f"{path}: job '{job_id}': {detail}") elif status == "deprecated": diff --git a/tests/dev-lead/integration/test_reusable_pin_compliance_classifier.py b/tests/dev-lead/integration/test_reusable_pin_compliance_classifier.py index a25022702..5e96cbad3 100644 --- a/tests/dev-lead/integration/test_reusable_pin_compliance_classifier.py +++ b/tests/dev-lead/integration/test_reusable_pin_compliance_classifier.py @@ -175,12 +175,52 @@ def test_cli_sha_still_fails() -> None: check(proc.returncode == 1, f"SHA pin still fails -> {proc.returncode}") +def test_scan_file_rejects_canonical_ref_with_mismatched_name() -> None: + """A syntactically canonical ref whose name prefix does not match the reusable is a violation.""" + print("test_scan_file_rejects_canonical_ref_with_mismatched_name") + # dev-lead-reusable.yml expects 'dev-lead/' prefix; 'pr-review/' belongs to another reusable. + body = ( + "jobs:\n" + " dl:\n" + " uses: petry-projects/.github-private/.github/workflows/dev-lead-reusable.yml" + "@pr-review/v1-stable\n" + ) + with tempfile.TemporaryDirectory() as d: + p = _write(Path(d), "wf.yml", body) + violations, deprecations, checked, canonical = mod.scan_file(p) + check(len(violations) == 1, f"mismatched canonical name is a violation -> {violations}") + check( + any("mismatch" in v for v in violations), + f"violation message mentions mismatch -> {violations}", + ) + check(not deprecations, f"not classified as deprecation -> {deprecations}") + + +def test_scan_file_rejects_legacy_ref_with_mismatched_name() -> None: + """A syntactically deprecated ref whose name prefix does not match the reusable is a violation, not a warning.""" + print("test_scan_file_rejects_legacy_ref_with_mismatched_name") + # dev-lead-reusable.yml expects 'dev-lead/' prefix; 'other-name/' is wrong. + body = ( + "jobs:\n" + " dl:\n" + " uses: petry-projects/.github-private/.github/workflows/dev-lead-reusable.yml" + "@other-name/next\n" + ) + with tempfile.TemporaryDirectory() as d: + p = _write(Path(d), "wf.yml", body) + violations, deprecations, checked, canonical = mod.scan_file(p) + check(len(violations) == 1, f"mismatched legacy name is a violation -> {violations}") + check(not deprecations, f"not classified as deprecation -> {deprecations}") + + def main() -> int: test_classify_ref_canonical() test_classify_ref_deprecated() test_classify_ref_violation() test_scan_file_captures_line_and_suggestion() test_scan_file_canonical_passes_clean() + test_scan_file_rejects_canonical_ref_with_mismatched_name() + test_scan_file_rejects_legacy_ref_with_mismatched_name() test_cli_deprecated_is_warning_not_failure() test_cli_sha_still_fails() if _failures: From 91041cb37e7ed10fbf546885e357f2e3db9c04b2 Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Tue, 18 Aug 2026 21:11:31 +0000 Subject: [PATCH 4/7] chore: dev-lead update (review-changes) [skip ci-relay] --- .github/workflows/test-dev-lead.yml | 2 +- .../test_reusable_pin_compliance.py | 46 +++++++++---- ...test_reusable_pin_compliance_classifier.py | 69 ++++++++++++++++++- 3 files changed, 102 insertions(+), 15 deletions(-) diff --git a/.github/workflows/test-dev-lead.yml b/.github/workflows/test-dev-lead.yml index 05e2d5ebb..9a54a08a4 100644 --- a/.github/workflows/test-dev-lead.yml +++ b/.github/workflows/test-dev-lead.yml @@ -214,7 +214,7 @@ jobs: persist-credentials: false - name: Install PyYAML run: python3 -m pip install --quiet --only-binary ":all:" pyyaml==6.0.3 - - name: Classifier semantics (canonical / DEPRECATED-legacy / violation) — #1493 + - name: "Classifier semantics (canonical / DEPRECATED-legacy / violation) — #1493" run: python3 tests/dev-lead/integration/test_reusable_pin_compliance_classifier.py - name: Reject SHA / off-channel pins; inventory legacy → major-scoped migration run: python3 tests/dev-lead/integration/test_reusable_pin_compliance.py diff --git a/tests/dev-lead/integration/test_reusable_pin_compliance.py b/tests/dev-lead/integration/test_reusable_pin_compliance.py index eef87b36f..c81b14e93 100644 --- a/tests/dev-lead/integration/test_reusable_pin_compliance.py +++ b/tests/dev-lead/integration/test_reusable_pin_compliance.py @@ -47,6 +47,11 @@ DEFAULT_GLOBS = ( ".github/workflows/*.yml", ".github/workflows/*.yaml", + # Recursive patterns cover files in subdirectories of .github/workflows/. + # Python < 3.12 requires ** to match at least one component, so the + # non-recursive *.yml lines above remain to catch top-level files. + ".github/workflows/**/*.yml", + ".github/workflows/**/*.yaml", "templates/**/*.yml", "templates/**/*.yaml", ) @@ -71,6 +76,20 @@ # is the classic pin-with-branch-note that this standard forbids for first-party. OFF_CHANNEL_COMMENT_RE = re.compile(r"(?[A-Za-z0-9_-]+)|["'](?P[A-Za-z0-9_-]+)["']):\s*(?:#.*)?$""" +) + + +def _parse_job_id(line: str) -> str | None: + """Return the job ID from a top-level job-key line, or None if not a job key.""" + m = _JOB_KEY_RE.match(line) + if not m: + return None + return m.group("bare") or m.group("quoted") + def is_first_party_reusable(path: str) -> bool: m = FIRST_PARTY_PATH_RE.match(path) @@ -162,9 +181,9 @@ def raw_comment_map(text: str) -> dict[tuple[str, str], str]: in_jobs = True continue if in_jobs: - m_job = re.match(r"^ (?P[A-Za-z0-9_-]+):\s*(?:#.*)?$", line) - if m_job: - current_job = m_job.group("job_id") + job_id = _parse_job_id(line) + if job_id is not None: + current_job = job_id continue elif re.match(r"^[^#\s]", line): in_jobs = False @@ -188,9 +207,9 @@ def uses_line_map(text: str) -> dict[tuple[str, str], int]: in_jobs = True continue if in_jobs: - m_job = re.match(r"^ (?P[A-Za-z0-9_-]+):\s*(?:#.*)?$", line) - if m_job: - current_job = m_job.group("job_id") + job_id = _parse_job_id(line) + if job_id is not None: + current_job = job_id continue elif re.match(r"^[^#\s]", line): in_jobs = False @@ -270,12 +289,15 @@ def scan_file(path: Path) -> tuple[list[str], list[dict], int, int]: ) else: canonical += 1 - comment = comments.get((job_id, uses)) - if off_channel_comment(comment): - violations.append( - f"{path}: job '{job_id}': off-channel `{comment.strip()}` annotation " - "on a first-party reusable pin" - ) + # Only check for off-channel comments on canonical pins. A deprecated pin + # already carries a migration warning; escalating it to a failure via a + # comment would violate the AC #8 grace-period intent (#687). + comment = comments.get((job_id, uses)) + if off_channel_comment(comment): + violations.append( + f"{path}: job '{job_id}': off-channel `{comment.strip()}` annotation " + "on a first-party reusable pin" + ) return violations, deprecations, checked, canonical diff --git a/tests/dev-lead/integration/test_reusable_pin_compliance_classifier.py b/tests/dev-lead/integration/test_reusable_pin_compliance_classifier.py index 5e96cbad3..42a0260f7 100644 --- a/tests/dev-lead/integration/test_reusable_pin_compliance_classifier.py +++ b/tests/dev-lead/integration/test_reusable_pin_compliance_classifier.py @@ -155,9 +155,11 @@ def test_cli_deprecated_is_warning_not_failure() -> None: check(proc.returncode == 0, f"DEPRECATED-only exits 0 (main stays green) -> {proc.returncode}\n{proc.stdout}") check("DEPRECATED" in proc.stdout, "output labels the pin DEPRECATED") check("pr-review/v-next" in proc.stdout, "output shows the suggested major-scoped ref") - # AC 9: counts land in the run summary. + # AC 9: exact counts land in the run summary. text = summary.read_text(encoding="utf-8") if summary.exists() else "" - check("legacy" in text.lower() and "1" in text, f"run summary carries the legacy count -> {text!r}") + check("- **canonical (major-scoped)**: 0" in text, f"run summary carries canonical count -> {text!r}") + check("- **legacy (DEPRECATED)**: 1" in text, f"run summary carries legacy count -> {text!r}") + check("- **checked**: 1" in text, f"run summary carries checked count -> {text!r}") def test_cli_sha_still_fails() -> None: @@ -213,6 +215,66 @@ def test_scan_file_rejects_legacy_ref_with_mismatched_name() -> None: check(not deprecations, f"not classified as deprecation -> {deprecations}") +def test_scan_file_quoted_job_id_line_mapping() -> None: + """Quoted job keys like '"review":' must be tracked with the correct line number.""" + print("test_scan_file_quoted_job_id_line_mapping") + body = ( + 'name: x\n' + 'on: [push]\n' + 'jobs:\n' + ' "review":\n' + ' uses: petry-projects/.github-private/.github/workflows/pr-review.yml@pr-review/next\n' + ) + with tempfile.TemporaryDirectory() as d: + p = _write(Path(d), "wf.yml", body) + violations, deprecations, checked, canonical = mod.scan_file(p) + check(not violations, f"no hard violations for quoted job key -> {violations}") + check(checked == 1, f"one first-party ref checked -> {checked}") + check(len(deprecations) == 1, f"one deprecation -> {len(deprecations)}") + if deprecations: + dep = deprecations[0] + check(dep["line"] == 5, f"correct line 5 (not 0) for quoted job -> {dep.get('line')}") + + +def test_scan_file_deprecated_with_off_channel_comment_stays_warning() -> None: + """A deprecated legacy pin with an off-channel comment must not escalate to a failure.""" + print("test_scan_file_deprecated_with_off_channel_comment_stays_warning") + body = ( + "jobs:\n" + " review:\n" + " uses: petry-projects/.github-private/.github/workflows/pr-review.yml" + "@pr-review/next # main\n" + ) + with tempfile.TemporaryDirectory() as d: + p = _write(Path(d), "wf.yml", body) + violations, deprecations, checked, canonical = mod.scan_file(p) + check(not violations, f"deprecated + off-channel comment stays warning, not violation -> {violations}") + check(len(deprecations) == 1, f"one deprecation -> {len(deprecations)}") + + +def test_nested_workflow_included_in_inventory() -> None: + """DEFAULT_GLOBS must inventory first-party pins in .github/workflows/ subdirectories.""" + print("test_nested_workflow_included_in_inventory") + body = ( + "jobs:\n" + " dl:\n" + " uses: petry-projects/.github-private/.github/workflows/dev-lead-reusable.yml@dev-lead/next\n" + ) + with tempfile.TemporaryDirectory() as d: + tmp = Path(d) + nested_dir = tmp / ".github" / "workflows" / "subdir" + nested_dir.mkdir(parents=True) + (nested_dir / "nested.yml").write_text(body, encoding="utf-8") + proc = subprocess.run( + [sys.executable, str(SCANNER)], + capture_output=True, + text=True, + cwd=d, + ) + check(proc.returncode == 0, f"nested legacy pin exits 0 (warning not failure) -> {proc.returncode}\n{proc.stdout}") + check("DEPRECATED" in proc.stdout, f"nested pin inventoried as DEPRECATED -> {proc.stdout}") + + def main() -> int: test_classify_ref_canonical() test_classify_ref_deprecated() @@ -223,6 +285,9 @@ def main() -> int: test_scan_file_rejects_legacy_ref_with_mismatched_name() test_cli_deprecated_is_warning_not_failure() test_cli_sha_still_fails() + test_scan_file_quoted_job_id_line_mapping() + test_scan_file_deprecated_with_off_channel_comment_stays_warning() + test_nested_workflow_included_in_inventory() if _failures: print(f"\nFAIL: {len(_failures)} classifier assertion(s) failed") return 1 From 695c8eec09ba8507e3d902ec7a83e4989792d843 Mon Sep 17 00:00:00 2001 From: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Date: Tue, 18 Aug 2026 22:44:50 +0000 Subject: [PATCH 5/7] fix(reviews): address review comments [skip ci-relay] --- .../test_reusable_pin_compliance.py | 37 +++++++++++++--- ...test_reusable_pin_compliance_classifier.py | 44 +++++++++++++++++++ 2 files changed, 74 insertions(+), 7 deletions(-) diff --git a/tests/dev-lead/integration/test_reusable_pin_compliance.py b/tests/dev-lead/integration/test_reusable_pin_compliance.py index c81b14e93..423fdd561 100644 --- a/tests/dev-lead/integration/test_reusable_pin_compliance.py +++ b/tests/dev-lead/integration/test_reusable_pin_compliance.py @@ -76,18 +76,31 @@ # is the classic pin-with-branch-note that this standard forbids for first-party. OFF_CHANNEL_COMMENT_RE = re.compile(r"(?[A-Za-z0-9_-]+)|["'](?P[A-Za-z0-9_-]+)["']):\s*(?:#.*)?$""" + r"""^(?P +)(?:(?P[A-Za-z0-9_-]+)|["'](?P[A-Za-z0-9_-]+)["']):\s*(?:#.*)?$""" ) -def _parse_job_id(line: str) -> str | None: - """Return the job ID from a top-level job-key line, or None if not a job key.""" +def _detect_job_indent(line: str) -> str | None: + """Return the leading-space string if `line` looks like a job key, else None.""" + m = _JOB_KEY_RE.match(line) + return m.group("indent") if m else None + + +def _parse_job_id(line: str, expected_indent: str | None = None) -> str | None: + """Return the job ID if `line` is a job-key line at `expected_indent`, else None. + + When `expected_indent` is None any leading-space indent is accepted (only safe + before the first job key has been seen and the indent level is known). + """ m = _JOB_KEY_RE.match(line) if not m: return None + if expected_indent is not None and m.group("indent") != expected_indent: + return None return m.group("bare") or m.group("quoted") @@ -176,18 +189,23 @@ def raw_comment_map(text: str) -> dict[tuple[str, str], str]: out: dict[tuple[str, str], str] = {} current_job: str | None = None in_jobs = False + job_indent: str | None = None for line in text.splitlines(): if line.strip() == "jobs:": in_jobs = True + job_indent = None continue if in_jobs: - job_id = _parse_job_id(line) + if job_indent is None: + job_indent = _detect_job_indent(line) + job_id = _parse_job_id(line, job_indent) if job_id is not None: current_job = job_id continue elif re.match(r"^[^#\s]", line): in_jobs = False current_job = None + job_indent = None continue if current_job: m_uses = USES_LINE_RE.match(line) @@ -202,18 +220,23 @@ def uses_line_map(text: str) -> dict[tuple[str, str], int]: out: dict[tuple[str, str], int] = {} current_job: str | None = None in_jobs = False + job_indent: str | None = None for i, line in enumerate(text.splitlines(), start=1): if line.strip() == "jobs:": in_jobs = True + job_indent = None continue if in_jobs: - job_id = _parse_job_id(line) + if job_indent is None: + job_indent = _detect_job_indent(line) + job_id = _parse_job_id(line, job_indent) if job_id is not None: current_job = job_id continue elif re.match(r"^[^#\s]", line): in_jobs = False current_job = None + job_indent = None continue if current_job: m_uses = USES_LINE_RE.match(line) diff --git a/tests/dev-lead/integration/test_reusable_pin_compliance_classifier.py b/tests/dev-lead/integration/test_reusable_pin_compliance_classifier.py index 42a0260f7..007e3b0f6 100644 --- a/tests/dev-lead/integration/test_reusable_pin_compliance_classifier.py +++ b/tests/dev-lead/integration/test_reusable_pin_compliance_classifier.py @@ -275,6 +275,49 @@ def test_nested_workflow_included_in_inventory() -> None: check("DEPRECATED" in proc.stdout, f"nested pin inventoried as DEPRECATED -> {proc.stdout}") +def test_scan_file_four_space_job_indent_line_mapping_and_comment() -> None: + """Four-space job-key indentation: line mapping and comment detection must both work.""" + print("test_scan_file_four_space_job_indent_line_mapping_and_comment") + # Deprecated legacy pin at four-space indent — line must be captured (not 0). + body_deprecated = ( + "name: x\n" + "on: [push]\n" + "jobs:\n" + " review:\n" + " uses: petry-projects/.github-private/.github/workflows/pr-review.yml" + "@pr-review/next\n" + ) + with tempfile.TemporaryDirectory() as d: + p = _write(Path(d), "wf.yml", body_deprecated) + violations, deprecations, checked, canonical = mod.scan_file(p) + check(not violations, f"no violations for four-space job indent (deprecated) -> {violations}") + check(len(deprecations) == 1, f"one deprecation at four-space indent -> {len(deprecations)}") + if deprecations: + check( + deprecations[0]["line"] == 5, + f"correct line 5 (not 0) for four-space job indent -> {deprecations[0].get('line')}", + ) + + # Canonical pin with off-channel comment at four-space indent — raw_comment_map() + # must capture the comment so the violation is reported (not silently bypassed). + body_canonical_comment = ( + "name: x\n" + "on: [push]\n" + "jobs:\n" + " review:\n" + " uses: petry-projects/.github-private/.github/workflows/pr-review.yml" + "@pr-review/v1-stable # main\n" + ) + with tempfile.TemporaryDirectory() as d: + p = _write(Path(d), "wf.yml", body_canonical_comment) + violations, deprecations, checked, canonical = mod.scan_file(p) + check( + len(violations) == 1, + f"canonical + off-channel comment is a violation at four-space indent -> {violations}", + ) + check(not deprecations, f"no deprecations for canonical pin -> {deprecations}") + + def main() -> int: test_classify_ref_canonical() test_classify_ref_deprecated() @@ -288,6 +331,7 @@ def main() -> int: test_scan_file_quoted_job_id_line_mapping() test_scan_file_deprecated_with_off_channel_comment_stays_warning() test_nested_workflow_included_in_inventory() + test_scan_file_four_space_job_indent_line_mapping_and_comment() if _failures: print(f"\nFAIL: {len(_failures)} classifier assertion(s) failed") return 1 From 3bc26c3277ecc90afcf20e761bd3599af11b7ba9 Mon Sep 17 00:00:00 2001 From: Don Petry <36422719+don-petry@users.noreply.github.com> Date: Wed, 19 Aug 2026 08:59:51 -0500 Subject: [PATCH 6/7] =?UTF-8?q?chore:=20empty=20commit=20to=20retrigger=20?= =?UTF-8?q?review=20=E2=80=94=20cycle-1=20blocking=20finding=20was=20metad?= =?UTF-8?q?ata-only=20('Closes'=20->=20'Part=20of'=20#1493)=20and=20is=20f?= =?UTF-8?q?ixed=20in=20the=20PR=20body;=20no=20content=20change?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From e655425f5eb8a8fd4127c8b1863270741993248b Mon Sep 17 00:00:00 2001 From: don-petry <36422719+don-petry@users.noreply.github.com> Date: Wed, 19 Aug 2026 14:08:13 +0000 Subject: [PATCH 7/7] fix(reviews): address review comments [skip ci-relay] --- .../test_reusable_pin_compliance.py | 15 +++- ...test_reusable_pin_compliance_classifier.py | 85 +++++++++++++++++++ 2 files changed, 98 insertions(+), 2 deletions(-) diff --git a/tests/dev-lead/integration/test_reusable_pin_compliance.py b/tests/dev-lead/integration/test_reusable_pin_compliance.py index 423fdd561..cc5e85428 100644 --- a/tests/dev-lead/integration/test_reusable_pin_compliance.py +++ b/tests/dev-lead/integration/test_reusable_pin_compliance.py @@ -83,6 +83,17 @@ r"""^(?P +)(?:(?P[A-Za-z0-9_-]+)|["'](?P[A-Za-z0-9_-]+)["']):\s*(?:#.*)?$""" ) +# Matches the root `jobs:` mapping key. YAML permits the key to be quoted +# (`"jobs":` / `'jobs':`) and to carry a trailing comment (`jobs: # …`); a plain +# `line.strip() == "jobs:"` misses those forms, leaving the source maps empty so +# deprecated pins report line 0 and canonical pins bypass off-channel validation. +_ROOT_JOBS_KEY_RE = re.compile(r"""^(?:jobs|["']jobs["']):\s*(?:#.*)?$""") + + +def _is_root_jobs_key(line: str) -> bool: + """Return True if `line` is the root `jobs:` key (quoted or comment-trailed).""" + return bool(_ROOT_JOBS_KEY_RE.match(line)) + def _detect_job_indent(line: str) -> str | None: """Return the leading-space string if `line` looks like a job key, else None.""" @@ -191,7 +202,7 @@ def raw_comment_map(text: str) -> dict[tuple[str, str], str]: in_jobs = False job_indent: str | None = None for line in text.splitlines(): - if line.strip() == "jobs:": + if _is_root_jobs_key(line): in_jobs = True job_indent = None continue @@ -222,7 +233,7 @@ def uses_line_map(text: str) -> dict[tuple[str, str], int]: in_jobs = False job_indent: str | None = None for i, line in enumerate(text.splitlines(), start=1): - if line.strip() == "jobs:": + if _is_root_jobs_key(line): in_jobs = True job_indent = None continue diff --git a/tests/dev-lead/integration/test_reusable_pin_compliance_classifier.py b/tests/dev-lead/integration/test_reusable_pin_compliance_classifier.py index 007e3b0f6..c77954247 100644 --- a/tests/dev-lead/integration/test_reusable_pin_compliance_classifier.py +++ b/tests/dev-lead/integration/test_reusable_pin_compliance_classifier.py @@ -318,6 +318,89 @@ def test_scan_file_four_space_job_indent_line_mapping_and_comment() -> None: check(not deprecations, f"no deprecations for canonical pin -> {deprecations}") +def test_scan_file_quoted_root_jobs_key_line_and_comment() -> None: + """A quoted root key (`"jobs":`) must still map lines and off-channel comments.""" + print("test_scan_file_quoted_root_jobs_key_line_and_comment") + # Deprecated legacy pin under a quoted root jobs key — line must be captured (not 0). + body_deprecated = ( + 'name: x\n' + 'on: [push]\n' + '"jobs":\n' + ' review:\n' + ' uses: petry-projects/.github-private/.github/workflows/pr-review.yml' + '@pr-review/next\n' + ) + with tempfile.TemporaryDirectory() as d: + p = _write(Path(d), "wf.yml", body_deprecated) + violations, deprecations, checked, canonical = mod.scan_file(p) + check(not violations, f"no violations under quoted root jobs key -> {violations}") + check(len(deprecations) == 1, f"one deprecation under quoted root jobs key -> {len(deprecations)}") + if deprecations: + check( + deprecations[0]["line"] == 5, + f"correct line 5 (not 0) under quoted root jobs key -> {deprecations[0].get('line')}", + ) + + # Canonical pin with off-channel comment under a quoted root jobs key — the + # comment must be captured so the violation is reported (not silently bypassed). + body_canonical_comment = ( + 'name: x\n' + 'on: [push]\n' + '"jobs":\n' + ' review:\n' + ' uses: petry-projects/.github-private/.github/workflows/pr-review.yml' + '@pr-review/v1-stable # main\n' + ) + with tempfile.TemporaryDirectory() as d: + p = _write(Path(d), "wf.yml", body_canonical_comment) + violations, deprecations, checked, canonical = mod.scan_file(p) + check( + len(violations) == 1, + f"canonical + off-channel comment is a violation under quoted root jobs key -> {violations}", + ) + + +def test_scan_file_commented_root_jobs_key_line_and_comment() -> None: + """A root key with a trailing comment (`jobs: # …`) must still map lines and comments.""" + print("test_scan_file_commented_root_jobs_key_line_and_comment") + # Deprecated legacy pin under a comment-trailed root jobs key — line captured (not 0). + body_deprecated = ( + "name: x\n" + "on: [push]\n" + "jobs: # source-map test\n" + " review:\n" + " uses: petry-projects/.github-private/.github/workflows/pr-review.yml" + "@pr-review/next\n" + ) + with tempfile.TemporaryDirectory() as d: + p = _write(Path(d), "wf.yml", body_deprecated) + violations, deprecations, checked, canonical = mod.scan_file(p) + check(not violations, f"no violations under commented root jobs key -> {violations}") + check(len(deprecations) == 1, f"one deprecation under commented root jobs key -> {len(deprecations)}") + if deprecations: + check( + deprecations[0]["line"] == 5, + f"correct line 5 (not 0) under commented root jobs key -> {deprecations[0].get('line')}", + ) + + # Canonical pin with off-channel comment under a comment-trailed root jobs key. + body_canonical_comment = ( + "name: x\n" + "on: [push]\n" + "jobs: # source-map test\n" + " review:\n" + " uses: petry-projects/.github-private/.github/workflows/pr-review.yml" + "@pr-review/v1-stable # main\n" + ) + with tempfile.TemporaryDirectory() as d: + p = _write(Path(d), "wf.yml", body_canonical_comment) + violations, deprecations, checked, canonical = mod.scan_file(p) + check( + len(violations) == 1, + f"canonical + off-channel comment is a violation under commented root jobs key -> {violations}", + ) + + def main() -> int: test_classify_ref_canonical() test_classify_ref_deprecated() @@ -332,6 +415,8 @@ def main() -> int: test_scan_file_deprecated_with_off_channel_comment_stays_warning() test_nested_workflow_included_in_inventory() test_scan_file_four_space_job_indent_line_mapping_and_comment() + test_scan_file_quoted_root_jobs_key_line_and_comment() + test_scan_file_commented_root_jobs_key_line_and_comment() if _failures: print(f"\nFAIL: {len(_failures)} classifier assertion(s) failed") return 1