diff --git a/.github/groom/README.md b/.github/groom/README.md index 4389845..060e05f 100644 --- a/.github/groom/README.md +++ b/.github/groom/README.md @@ -1,4 +1,4 @@ -# Groom — two-phase code-cleanup briefs +# Groom — two-phase code-cleanup briefs + durable rejection ledger Version-controlled, co-ownable **prompts** for the agent-work *groom* workflow: a periodic, org-wide sweep that proposes high-value refactors (duplication, @@ -81,3 +81,75 @@ a template + substitution reproduces the previous inline prompt with no change t review panel's safety rails — the `security` flag as an explicit placeholder, and a read-only + untrusted-input boundary on both phases — which harden behavior without changing the findings themselves. + +## `ledger.py` — the durable dedup / rejection ledger (BE-3874) + +A **stateless CI run** starts fresh every time — with no durable memory it would +re-file findings that were already filed OR already human-rejected on every +scheduled run. That is the fastest way to make the shared groom capability +annoying and get it disabled. The roundtable was explicit: *dedup must remember +REJECTIONS — don't re-raise a rejected finding next week.* + +`ledger.py` uses **GitHub issue state itself** as the durable store — the +GitHub-native option that needs **no net-new secret** (the run's `GITHUB_TOKEN` +already reads issues) and is fully **auditable** (the record is the issues you +can see). No separate database, cache, or committed state file. + +Keyed on `(repo, finding_signature) → {filed | rejected | superseded}`: + +| Live GitHub state | Ledger status | Re-file? | +|---|---|---| +| Open `groom` issue for the signature | `filed` | no | +| Closed as **completed** | `filed` | no (already handled) | +| Closed as **not planned** (GitHub "close as wontfix") | `rejected` | **no — durable** | +| Carries the `groom-rejected` label (open or closed) | `rejected` | **no — durable** | +| Carries the `groom-superseded` label | `superseded` | no | +| No `groom` issue carries the signature | `unknown` | **yes** | + +Only an `unknown` signature is filed. Human rejection — close-as-not-planned or +the `groom-rejected` label — suppresses that signature forever. + +### The filing contract (load-bearing) + +This module consumes the verifier's stable dedup `signature` (see above) as an +opaque string on each finding. For the memory to survive, the step that OPENS an +issue for a `to_file` finding **must**: + +1. apply the **`groom`** label (how the next run finds our issues), and +2. append `signature_marker(finding["signature"])` to the issue body — an + invisible HTML comment (``) the next run recovers. + +Skip either and the next run cannot recognize the issue and will re-file it. + +The dedup decision is a point-in-time snapshot of GitHub issue state read +*before* filing, and issue creation happens in a later step. Two overlapping +groom runs could therefore both classify the same signature as `unknown` and +file duplicates (a TOCTOU race). The caller workflow (not yet written — epic +BE-3870) **must serialize groom runs with a `concurrency:` group** so at most +one run reads-then-files at a time. + +### CLI (called right before the groomer files) + +```bash +python3 .github/groom/ledger.py \ + --repo owner/name --candidates findings.json --out decision.json +``` + +`findings.json` is a JSON array of findings, each with a `signature`. +`decision.json` receives `{to_file, suppressed, invalid, ledger_size}` — open +issues only for `to_file`. `invalid` = findings with no usable signature; they +are **not** filed (filing an un-dedupable finding would risk the exact +duplicate-spam this ledger prevents) and should be surfaced as a producer error. + +Single-signature probe (exit 0 = should file, 1 = suppressed): + +```bash +python3 .github/groom/ledger.py --repo owner/name --check "" +``` + +- **`tests/`** — `unittest` suite, run by + [`test-groom-scripts.yml`](../workflows/test-groom-scripts.yml). + +```bash +python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' -v +``` diff --git a/.github/groom/ledger.py b/.github/groom/ledger.py new file mode 100644 index 0000000..9abb313 --- /dev/null +++ b/.github/groom/ledger.py @@ -0,0 +1,422 @@ +#!/usr/bin/env python3 +"""Durable rejection-memory (dedup ledger) for the stateless groom CI run. + +Studio groom keeps its dedup + rejection ledger on the local filesystem +(`.groom-state/`). A **stateless CI run** starts fresh every time, so without a +durable memory it would re-file findings that were already filed OR already +human-rejected on every scheduled run — the fastest way to make the shared +capability annoying and get it disabled. + +This module gives the CI groomer that durable memory **using GitHub issue state +itself as the store** — the GitHub-native option that needs no net-new secret +and is fully auditable (the record is the issues themselves): + +- When the groomer files a finding it opens a `groom`-labeled issue whose body + carries the verifier's stable dedup signature as an HTML-comment marker + (`signature_marker()`), invisible to human readers but machine-recoverable. +- Before filing on the NEXT run, the groomer lists every `groom`-labeled issue + (`state=all`), recovers each signature, and classifies it (`build_ledger()`). + A signature that already has an issue — open, resolved, or rejected — is + **known** and is not re-filed. +- **Human rejection is remembered natively**: an issue closed as `not_planned` + (GitHub's "close as not planned" == wontfix) OR carrying the + `groom-rejected` label maps to `REJECTED`, so that signature is suppressed + forever, exactly as the roundtable required ("dedup must remember + REJECTIONS — don't re-raise a rejected finding next week"). + +The pure logic (signature marker round-trip, per-issue classification, ledger +build, candidate partition) is separated from the one thin `gh` I/O shell +(`fetch_groom_issues`) so it is fully unit-testable with no network. + +The signature is OWNED by the verifier ("keyed on the verifier's stable dedup +signature") — this module consumes whatever opaque string the verifier emits on +each finding's `signature` field; it never invents one. It only trims +surrounding whitespace so a stray newline in a marker can't split one signature +into two ledger keys. + +CLI (what the groom workflow calls right before it files): + + python3 .github/groom/ledger.py \ + --repo owner/name --candidates findings.json --out decision.json + +`findings.json` is a JSON array of findings, each with a `signature` field; +`decision.json` receives {"to_file": [...], "suppressed": [...], +"invalid": [...], "ledger_size": N}. Only `to_file` should be opened as issues; +each `to_file` finding must have `signature_marker(finding["signature"])` +appended to its issue body and the `groom` label applied, or the NEXT run will +re-file it. +""" + +import argparse +import base64 +import binascii +import json +import re +import subprocess +import sys + +# Label every groom-filed issue carries. It is how the ledger identifies "our" +# issues (list-by-label, not a full-text search — deterministic and free of the +# search index's indexing lag, so a finding filed in run N is reliably seen in +# run N+1). +GROOM_LABEL = "groom" + +# Human-rejection label. Applying it to a groom issue (open OR closed) durably +# suppresses that signature — the label path exists alongside "close as not +# planned" so a maintainer can reject without necessarily closing. +REJECTED_LABEL = "groom-rejected" + +# A finding replaced by another (e.g. folded into a broader finding, or its +# location drifted and the verifier re-keyed it). Suppresses re-filing without +# implying a human said "no". +SUPERSEDED_LABEL = "groom-superseded" + +# The signature marker embedded in a filed issue's body. An HTML comment so it +# renders invisibly, and a stable prefix so it round-trips through the API's raw +# body. The opaque signature is URL-safe-base64 encoded before embedding: the +# raw signature could contain `-->` (which would close the comment early and +# truncate the recovered key, re-filing the finding forever) or arbitrary +# markdown/HTML (which would be injected into the public issue body). base64url +# is a delimiter-safe, injection-proof alphabet — `[A-Za-z0-9_=-]` — that can +# never contain `-->`, so the payload group is bounded to that alphabet. The +# surrounding whitespace groups are POSSESSIVE (`\s*+`): with a plain `\s*` on +# both sides of an empty-matchable payload, a body carrying the prefix followed +# by a long whitespace run and no terminator backtracks in O(n^2) (the reported +# ReDoS). Possessive quantifiers make the whitespace non-giving, so a +# non-matching body fails in linear time. (Requires Python 3.11+; CI runs 3.12.) +_MARKER_PREFIX = "groom-signature:" +_MARKER_RE = re.compile(r"") + +# Ledger statuses. UNKNOWN is the only one that permits filing. +FILED = "filed" +REJECTED = "rejected" +SUPERSEDED = "superseded" +UNKNOWN = "unknown" + +# Partition-time-only status: a signature that is UNKNOWN in the live ledger but +# has ALREADY been routed to `to_file` earlier in THIS candidate batch. The +# second-and-later findings that share it are suppressed under this status so a +# single run cannot open two issues for one signature before GitHub state is +# refreshed — the exact duplicate spam the ledger exists to prevent. Never a +# live ledger status (it is not a GitHub issue state), so it is not in +# `_PRECEDENCE`. +PENDING = "pending" + +# Precedence when several issues share one signature (shouldn't happen, but be +# robust): surface the most decision-bearing status. Rejection is the stickiest +# human signal, so it wins; a superseded marker beats a plain filed one. The +# dedup DECISION doesn't depend on this ordering — every non-UNKNOWN status +# suppresses filing equally — only the reported status does. +_PRECEDENCE = {REJECTED: 3, SUPERSEDED: 2, FILED: 1} + + +def signature_marker(signature: str) -> str: + """The HTML-comment marker the filing step must append to an issue body. + + Round-trips with `extract_signature`. The signature is URL-safe-base64 + encoded so it can carry any opaque bytes (including `-->`, newlines, or + markdown) without closing the comment early or injecting into the public + issue body. The filing step owns applying this (and the `groom` label); if + it doesn't, the next run cannot recognize the issue and will re-file the + finding. + """ + encoded = base64.urlsafe_b64encode(normalize_signature(signature).encode("utf-8")).decode("ascii") + return f"" + + +def normalize_signature(signature) -> str: + """Canonicalize a signature for use as a ledger key. + + Only strips surrounding whitespace — the signature is an opaque, + case-sensitive token owned by the verifier, so we must not lowercase or + otherwise rewrite it (that could collide two distinct findings). A missing + or non-string signature returns "" (never the literal "None"), so a + malformed candidate is routed to `invalid` rather than filed under a + bogus shared key. + """ + if not isinstance(signature, str): + return "" + return signature.strip() + + +def extract_signature(body): + """Recover the embedded signature from an issue body, or None. + + Tolerant of the surrounding markdown/prose an issue body carries. Returns + the signature from the LAST marker, not the first: the filing contract + appends the authoritative marker after the finding text, so a marker-shaped + comment planted earlier in an attacker-controlled quoted snippet cannot + shadow the genuine one. The base64 payload is decoded back to the original + opaque signature; a marker whose payload is not valid base64/UTF-8 is + ignored (returns None) rather than poisoning a ledger key. + """ + if not body: + return None + matches = _MARKER_RE.findall(body) + if not matches: + return None + try: + decoded = base64.urlsafe_b64decode(matches[-1].encode("ascii")).decode("utf-8") + except (binascii.Error, ValueError): + return None + signature = normalize_signature(decoded) + return signature or None + + +def _labels(issue) -> set: + """Lowercased set of an issue's label names (tolerant of shapes/None).""" + names = set() + for label in issue.get("labels") or []: + name = label.get("name") if isinstance(label, dict) else label + if isinstance(name, str): + names.add(name.lower()) + return names + + +def classify_issue(issue) -> str: + """Map one groom issue to a ledger status (never UNKNOWN — it exists). + + Rejection is recognized two ways, either of which is durable: + * the `groom-rejected` label (open or closed), or + * closed as `not_planned` — GitHub's "Close as not planned" == wontfix. + A `groom-superseded` label marks a replaced finding. Everything else + (open, or closed as completed/fixed) is FILED: already handled, don't + re-file. + """ + labels = _labels(issue) + closed_not_planned = ( + issue.get("state") == "closed" and issue.get("state_reason") == "not_planned" + ) + if REJECTED_LABEL in labels or closed_not_planned: + return REJECTED + if SUPERSEDED_LABEL in labels: + return SUPERSEDED + return FILED + + +def build_ledger(issues) -> dict: + """Build a {signature -> status} map from a list of groom issues. + + Issues without a recoverable signature marker are skipped: a `groom`-labeled + issue a human opened by hand (no marker) is not one of ours and must not + poison a signature key. Pull requests (the `/issues` endpoint returns them + too) are skipped. When two issues share a signature, the higher-precedence + status wins (`_PRECEDENCE`). + """ + statuses: dict = {} + for issue in issues: + if issue.get("pull_request"): + continue + signature = extract_signature(issue.get("body")) + if not signature: + continue + status = classify_issue(issue) + current = statuses.get(signature) + if current is None or _PRECEDENCE[status] > _PRECEDENCE[current]: + statuses[signature] = status + return statuses + + +class Ledger: + """A signature -> status view with the dedup decision baked in.""" + + def __init__(self, statuses: dict): + self._statuses = dict(statuses) + + def __len__(self) -> int: + return len(self._statuses) + + def status(self, signature) -> str: + """Ledger status for a signature (UNKNOWN if never filed/rejected).""" + return self._statuses.get(normalize_signature(signature), UNKNOWN) + + def is_known(self, signature) -> bool: + return self.status(signature) != UNKNOWN + + def should_file(self, signature) -> bool: + """A finding is filed only if its signature is genuinely new. + + A missing/blank/non-string signature is NOT filable: it has no + recoverable marker, so filing it would re-file on every subsequent run. + This mirrors `partition`, which routes such a candidate to `invalid` + rather than `to_file`. + """ + return normalize_signature(signature) != "" and self.status(signature) == UNKNOWN + + def partition(self, findings): + """Split candidate findings into to_file / suppressed / invalid. + + `to_file` — signature is UNKNOWN *and* first-seen in this batch: open + an issue (remember to embed the marker + apply the `groom` + label). + `suppressed` — signature is known (filed/rejected/superseded) OR was + already routed to `to_file` earlier in this same batch + (`pending`): each annotated with `ledger_status` for + auditable reporting. + `invalid` — no usable signature: cannot be deduped, so it is NOT filed + (filing it would risk the exact duplicate-spam this ledger + exists to prevent). The workflow should surface these as a + producer error rather than silently dropping or spamming. + + Intra-batch dedup: two candidates that share one new signature must not + both be filed — the ledger is only refreshed from GitHub between runs, + so the first opens the issue and later duplicates are suppressed as + `pending` (not falsely labeled `filed`, which they are not yet). + """ + to_file, suppressed, invalid = [], [], [] + filed_this_batch = set() + for finding in findings: + signature = normalize_signature(finding.get("signature")) if isinstance(finding, dict) else "" + if not signature: + invalid.append(finding) + continue + status = self.status(signature) + if status != UNKNOWN: + suppressed.append({**finding, "ledger_status": status}) + elif signature in filed_this_batch: + suppressed.append({**finding, "ledger_status": PENDING}) + else: + filed_this_batch.add(signature) + to_file.append(finding) + return to_file, suppressed, invalid + + +# A repo must be exactly `owner/name` — no extra path segments or URL +# metacharacters (`?`, `&`, `#`) that could override the `labels=…&state=all` +# query or redirect the endpoint and silently return the wrong issue set. +_REPO_RE = re.compile(r"^[A-Za-z0-9._-]+/[A-Za-z0-9._-]+$") + +# Bound the `gh api` call so a stalled network/API connection can't block the +# groom run until the coarse Actions job timeout, wasting runner minutes. +_FETCH_TIMEOUT_SECONDS = 60 + + +def fetch_groom_issues(repo: str, run=subprocess.run): + """List every `groom`-labeled issue in `repo` (state=all) via `gh api`. + + Paginated so a repo with many groom issues is fully covered. `run` is + injectable so tests can stub the subprocess. Raises on a non-zero exit or a + timeout — a failure to read the ledger must fail loudly, never silently + degrade to an empty ledger (which would re-file everything). + """ + if not _REPO_RE.match(repo or ""): + raise ValueError(f"invalid repo {repo!r}: expected owner/name") + try: + result = run( + [ + "gh", + "api", + "--paginate", + f"/repos/{repo}/issues?labels={GROOM_LABEL}&state=all&per_page=100", + ], + text=True, + capture_output=True, + timeout=_FETCH_TIMEOUT_SECONDS, + ) + except subprocess.TimeoutExpired as exc: + raise RuntimeError( + f"gh api timed out after {_FETCH_TIMEOUT_SECONDS}s listing groom issues" + ) from exc + if result.returncode != 0: + raise RuntimeError(f"gh api failed to list groom issues: {result.stderr.strip()}") + return _parse_paginated_json(result.stdout) + + +def _parse_paginated_json(stdout: str): + """Parse `gh api --paginate` output into one flat list of issues. + + `--paginate` concatenates each page's JSON array. `gh` normally stitches + them into one array, but be tolerant of the concatenated-arrays shape too + (multiple top-level `[...]` values) so a gh behavior change can't silently + truncate the ledger to page one. + """ + text = stdout.strip() + if not text: + return [] + decoder = json.JSONDecoder() + issues, idx, n = [], 0, len(text) + while idx < n: + while idx < n and text[idx].isspace(): + idx += 1 + if idx >= n: + break + value, end = decoder.raw_decode(text, idx) + if isinstance(value, list): + issues.extend(value) + elif isinstance(value, dict): + issues.append(value) + idx = end + return issues + + +def load_ledger(repo: str, run=subprocess.run) -> Ledger: + """Read live GitHub issue state and return the dedup Ledger.""" + return Ledger(build_ledger(fetch_groom_issues(repo, run=run))) + + +def main(argv=None): + parser = argparse.ArgumentParser( + description="Durable groom dedup/rejection ledger backed by GitHub issue state." + ) + parser.add_argument("--repo", required=True, help="owner/name of the target repo") + parser.add_argument( + "--candidates", + help="Path to a JSON array of candidate findings (each with a 'signature').", + ) + parser.add_argument( + "--out", + help="Path to write the {to_file, suppressed, invalid, ledger_size} decision JSON.", + ) + parser.add_argument( + "--check", + metavar="SIGNATURE", + help="Print the ledger status of one signature and exit 0 if it should be filed, 1 if suppressed.", + ) + args = parser.parse_args(argv) + + if args.check is None and not args.candidates: + parser.error("one of --candidates or --check is required") + + ledger = load_ledger(args.repo) + + if args.check is not None: + # A blank/unusable signature is `invalid`, not filable — mirror + # `partition` (which routes it to `invalid`) instead of reporting + # `unknown` and exiting 0, which would file an un-dedupable issue. + if normalize_signature(args.check) == "": + print("invalid") + return 1 + status = ledger.status(args.check) + print(status) + return 0 if ledger.should_file(args.check) else 1 + + with open(args.candidates, encoding="utf-8") as f: + findings = json.load(f) + if not isinstance(findings, list): + parser.error("--candidates must be a JSON array of findings") + + to_file, suppressed, invalid = ledger.partition(findings) + decision = { + "to_file": to_file, + "suppressed": suppressed, + "invalid": invalid, + "ledger_size": len(ledger), + } + + payload = json.dumps(decision, indent=2) + if args.out: + with open(args.out, "w", encoding="utf-8") as f: + f.write(payload + "\n") + else: + print(payload) + + print( + f"ledger: {len(ledger)} known signature(s); " + f"{len(to_file)} to file, {len(suppressed)} suppressed, {len(invalid)} invalid.", + file=sys.stderr, + ) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/.github/groom/tests/test_ledger.py b/.github/groom/tests/test_ledger.py new file mode 100644 index 0000000..386f690 --- /dev/null +++ b/.github/groom/tests/test_ledger.py @@ -0,0 +1,314 @@ +#!/usr/bin/env python3 +"""Tests for the groom dedup/rejection ledger (BE-3874). + +The core property the ledger must hold: a finding filed OR human-rejected in +run N is never re-filed in run N+1 (same signature), and a rejection is durable. +These tests drive the pure logic (marker round-trip, classification, ledger +build, partition) with no network, plus a stubbed `gh` fetch. + +Run: python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' -v +""" + +import importlib.util +import json +import os +import unittest + +_MODULE_PATH = os.path.join(os.path.dirname(__file__), "..", "ledger.py") +_spec = importlib.util.spec_from_file_location("groom_ledger", _MODULE_PATH) +ledger = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(ledger) + + +def issue(signature=None, *, state="open", state_reason=None, labels=("groom",), body=None, pr=False): + """Build a minimal GitHub-issue dict, embedding a marker unless body given.""" + if body is None: + body = "Some finding text.\n\n" + ledger.signature_marker(signature) if signature else "no marker" + d = { + "state": state, + "state_reason": state_reason, + "labels": [{"name": n} for n in labels], + "body": body, + } + if pr: + d["pull_request"] = {"url": "http://x"} + return d + + +class MarkerRoundTripTest(unittest.TestCase): + def test_round_trip(self): + sig = "sha256:abcdef123" + self.assertEqual(ledger.extract_signature(ledger.signature_marker(sig)), sig) + + def test_marker_embedded_in_prose(self): + sig = "repo:rule-x:path/to/file.go:func" + body = f"# A groom finding\n\nBlah blah.\n\n{ledger.signature_marker(sig)}\n\nmore text" + self.assertEqual(ledger.extract_signature(body), sig) + + def test_no_marker_returns_none(self): + self.assertIsNone(ledger.extract_signature("a plain human-written issue")) + self.assertIsNone(ledger.extract_signature("")) + self.assertIsNone(ledger.extract_signature(None)) + + def test_normalize_trims_whitespace(self): + self.assertEqual(ledger.normalize_signature(" sig \n"), "sig") + + def test_signature_is_case_sensitive(self): + # Opaque token — must NOT be lowercased (would collide distinct hashes). + self.assertEqual(ledger.extract_signature(ledger.signature_marker("AbC")), "AbC") + + def test_round_trip_signature_with_comment_terminator(self): + # A signature containing `-->` must not close the HTML comment early and + # truncate the recovered key (which would re-file the finding forever). + sig = "rule:x-->y:path/file.go:func" + self.assertEqual(ledger.extract_signature(ledger.signature_marker(sig)), sig) + + def test_round_trip_signature_with_newlines_and_markup(self): + sig = "line1\nline2 markup & " + self.assertEqual(ledger.extract_signature(ledger.signature_marker(sig)), sig) + + def test_last_marker_wins_over_planted_shadow(self): + # An attacker-controlled finding snippet can embed a marker-shaped + # comment; the authoritative marker the filing step appends comes LAST + # and must win, so the genuine signature is the one recovered. + planted = ledger.signature_marker("forged-suppression-target") + genuine = ledger.signature_marker("genuine-sig") + body = f"Quoted code:\n\n{planted}\n\nfinding text\n\n{genuine}" + self.assertEqual(ledger.extract_signature(body), "genuine-sig") + + def test_invalid_base64_payload_ignored(self): + # A marker whose payload is not valid base64 must not poison a key — + # both when out-of-alphabet chars stop the regex and when the payload is + # in-alphabet but undecodable (bad length). + self.assertIsNone(ledger.extract_signature("")) + self.assertIsNone(ledger.extract_signature("")) + + +class ClassifyIssueTest(unittest.TestCase): + def test_open_issue_is_filed(self): + self.assertEqual(ledger.classify_issue(issue("s", state="open")), ledger.FILED) + + def test_closed_completed_is_filed(self): + # Fixed & closed → already handled, still suppressed (don't re-file). + self.assertEqual( + ledger.classify_issue(issue("s", state="closed", state_reason="completed")), + ledger.FILED, + ) + + def test_closed_not_planned_is_rejected(self): + # GitHub "Close as not planned" == wontfix → durable rejection. + self.assertEqual( + ledger.classify_issue(issue("s", state="closed", state_reason="not_planned")), + ledger.REJECTED, + ) + + def test_rejected_label_open_is_rejected(self): + # Label rejection works even without closing the issue. + self.assertEqual( + ledger.classify_issue(issue("s", state="open", labels=("groom", "groom-rejected"))), + ledger.REJECTED, + ) + + def test_superseded_label(self): + self.assertEqual( + ledger.classify_issue(issue("s", labels=("groom", "groom-superseded"))), + ledger.SUPERSEDED, + ) + + def test_rejected_label_beats_superseded(self): + self.assertEqual( + ledger.classify_issue(issue("s", labels=("groom", "groom-superseded", "groom-rejected"))), + ledger.REJECTED, + ) + + +class BuildLedgerTest(unittest.TestCase): + def test_skips_issues_without_marker(self): + # A human-opened groom issue with no marker must not create a key. + led = ledger.build_ledger([issue(body="human wrote this, no marker")]) + self.assertEqual(len(led), 0) + + def test_skips_pull_requests(self): + led = ledger.build_ledger([issue("s", pr=True)]) + self.assertEqual(len(led), 0) + + def test_rejection_wins_when_duplicate_signatures(self): + # Same signature on a filed AND a rejected issue → rejected surfaces. + led = ledger.build_ledger( + [ + issue("dup", state="open"), + issue("dup", state="closed", state_reason="not_planned"), + ] + ) + self.assertEqual(led["dup"], ledger.REJECTED) + + def test_mixed_repo(self): + led = ledger.build_ledger( + [ + issue("filed-sig", state="open"), + issue("rejected-sig", state="closed", state_reason="not_planned"), + issue("super-sig", labels=("groom", "groom-superseded")), + issue(body="no marker human issue"), + ] + ) + self.assertEqual(led, { + "filed-sig": ledger.FILED, + "rejected-sig": ledger.REJECTED, + "super-sig": ledger.SUPERSEDED, + }) + + +class LedgerDecisionTest(unittest.TestCase): + def setUp(self): + self.led = ledger.Ledger({ + "filed": ledger.FILED, + "rejected": ledger.REJECTED, + "super": ledger.SUPERSEDED, + }) + + def test_unknown_should_file(self): + self.assertTrue(self.led.should_file("brand-new")) + self.assertFalse(self.led.is_known("brand-new")) + self.assertEqual(self.led.status("brand-new"), ledger.UNKNOWN) + + def test_blank_signature_is_not_filable(self): + # Mirrors partition's `invalid` routing: an empty/missing/non-string + # signature has no recoverable marker, so it must NOT be filed (else it + # re-files every run). Guards the single-signature `should_file`/`--check` + # path against disagreeing with `partition`. + self.assertFalse(self.led.should_file("")) + self.assertFalse(self.led.should_file(" ")) + self.assertFalse(self.led.should_file(None)) + self.assertFalse(self.led.should_file(123)) + + def test_filed_suppressed(self): + self.assertFalse(self.led.should_file("filed")) + + def test_rejected_suppressed(self): + # The load-bearing acceptance case: a human rejection stays suppressed. + self.assertFalse(self.led.should_file("rejected")) + self.assertTrue(self.led.is_known("rejected")) + + def test_superseded_suppressed(self): + self.assertFalse(self.led.should_file("super")) + + def test_status_lookup_normalizes(self): + self.assertEqual(self.led.status(" filed \n"), ledger.FILED) + + def test_partition(self): + findings = [ + {"signature": "brand-new", "title": "A"}, + {"signature": "filed", "title": "B"}, + {"signature": "rejected", "title": "C"}, + {"signature": "", "title": "D no sig"}, + {"title": "E missing sig key"}, + "not even a dict", + ] + to_file, suppressed, invalid = self.led.partition(findings) + self.assertEqual([f["title"] for f in to_file], ["A"]) + self.assertEqual({f["title"]: f["ledger_status"] for f in suppressed}, + {"B": ledger.FILED, "C": ledger.REJECTED}) + self.assertEqual(len(invalid), 3) + + def test_partition_dedups_within_batch(self): + # Two findings sharing ONE new signature must not both be filed in a + # single run — the ledger only refreshes from GitHub between runs, so a + # second issue would be the exact duplicate spam this exists to prevent. + findings = [ + {"signature": "new-dup", "title": "first"}, + {"signature": "new-dup", "title": "second"}, + {"signature": " new-dup \n", "title": "third-whitespace-variant"}, + ] + to_file, suppressed, invalid = self.led.partition(findings) + self.assertEqual([f["title"] for f in to_file], ["first"]) + self.assertEqual( + [(f["title"], f["ledger_status"]) for f in suppressed], + [("second", ledger.PENDING), ("third-whitespace-variant", ledger.PENDING)], + ) + self.assertEqual(invalid, []) + + +class AcceptanceScenarioTest(unittest.TestCase): + """End-to-end run N -> run N+1 using the ledger built from prior issues.""" + + def test_filed_then_not_refiled(self): + # Run N filed signature "x" (an open issue now exists). Run N+1: + led = ledger.Ledger(ledger.build_ledger([issue("x", state="open")])) + _, suppressed, _ = led.partition([{"signature": "x"}]) + self.assertEqual(len(suppressed), 1) + + def test_human_rejection_durably_suppresses(self): + # Run N filed "y"; a human closed it as not planned. Run N+1 must NOT re-file. + led = ledger.Ledger( + ledger.build_ledger([issue("y", state="closed", state_reason="not_planned")]) + ) + to_file, suppressed, _ = led.partition([{"signature": "y"}]) + self.assertEqual(to_file, []) + self.assertEqual(suppressed[0]["ledger_status"], ledger.REJECTED) + + def test_new_finding_still_files(self): + led = ledger.Ledger(ledger.build_ledger([issue("y", state="open")])) + to_file, _, _ = led.partition([{"signature": "z-new"}]) + self.assertEqual(len(to_file), 1) + + +class FetchTest(unittest.TestCase): + """Stub `gh api` to exercise the I/O shell without network.""" + + class _Result: + def __init__(self, returncode, stdout="", stderr=""): + self.returncode, self.stdout, self.stderr = returncode, stdout, stderr + + def test_fetch_parses_single_array(self): + payload = json.dumps([issue("a"), issue("b")]) + run = lambda *a, **k: self._Result(0, stdout=payload) + issues = ledger.fetch_groom_issues("o/r", run=run) + self.assertEqual(len(issues), 2) + + def test_fetch_parses_concatenated_pages(self): + # --paginate can emit concatenated top-level arrays; must not truncate. + payload = json.dumps([issue("a")]) + "\n" + json.dumps([issue("b"), issue("c")]) + run = lambda *a, **k: self._Result(0, stdout=payload) + self.assertEqual(len(ledger.fetch_groom_issues("o/r", run=run)), 3) + + def test_fetch_raises_on_error(self): + run = lambda *a, **k: self._Result(1, stderr="boom") + with self.assertRaises(RuntimeError): + ledger.fetch_groom_issues("o/r", run=run) + + def test_fetch_rejects_malformed_repo(self): + # A repo with URL metacharacters / extra path segments could override + # the labels/state query or redirect the endpoint — reject before the + # gh call so it can never corrupt the issue set (never calls `run`). + never = lambda *a, **k: self.fail("run must not be called for a bad repo") + for bad in ("o/r?labels=other", "o/r/extra", "o r", "", "justname"): + with self.assertRaises(ValueError): + ledger.fetch_groom_issues(bad, run=never) + + def test_fetch_raises_on_timeout(self): + import subprocess as _sp + + def run(*a, **k): + raise _sp.TimeoutExpired(cmd="gh", timeout=k.get("timeout", 0)) + + with self.assertRaises(RuntimeError): + ledger.fetch_groom_issues("o/r", run=run) + + def test_empty_output(self): + run = lambda *a, **k: self._Result(0, stdout="") + self.assertEqual(ledger.fetch_groom_issues("o/r", run=run), []) + + def test_load_ledger_end_to_end(self): + payload = json.dumps([ + issue("open-one", state="open"), + issue("rejected-one", state="closed", state_reason="not_planned"), + ]) + run = lambda *a, **k: self._Result(0, stdout=payload) + led = ledger.load_ledger("o/r", run=run) + self.assertTrue(led.should_file("something-new")) + self.assertFalse(led.should_file("open-one")) + self.assertFalse(led.should_file("rejected-one")) + + +if __name__ == "__main__": + unittest.main() diff --git a/.github/workflows/test-groom-scripts.yml b/.github/workflows/test-groom-scripts.yml new file mode 100644 index 0000000..3cc9ae9 --- /dev/null +++ b/.github/workflows/test-groom-scripts.yml @@ -0,0 +1,39 @@ +name: Test groom scripts + +# Runs the unit tests for the groom dedup/rejection ledger (ledger.py). The +# ledger is the durable memory that stops the stateless groom CI run from +# re-filing already-filed or human-rejected findings every run, so a regression +# here would silently re-open the spam it exists to prevent — cheap to guard +# with a unit run on change. + +on: + pull_request: + paths: + - '.github/groom/**' + - '.github/workflows/test-groom-scripts.yml' + push: + branches: [main] + paths: + - '.github/groom/**' + - '.github/workflows/test-groom-scripts.yml' + +permissions: + contents: read + +jobs: + test: + name: unittest + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 + with: + persist-credentials: false + + - name: Set up Python + uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6 + with: + python-version: '3.12' + + - name: Run groom ledger unit tests + run: python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' -v diff --git a/AGENTS.md b/AGENTS.md index aca9a33..d101fb1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -20,6 +20,9 @@ python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py' -v # agents-md-integrity checker tests python3 -m unittest discover -s .github/agents-md-integrity/tests -p 'test_*.py' -v +# groom dedup/rejection ledger tests +python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' -v + # bump-callers shell tests + lint (gh is stubbed; no network) shellcheck -x .github/bump-callers/bump-callers.sh .github/bump-callers/tests/test_bump_callers.sh bash .github/bump-callers/tests/test_bump_callers.sh @@ -43,6 +46,11 @@ tests — run the matching command above for whatever you touched. time, never copied into consumers. Tests in `tests/`. - `.github/agents-md-integrity/` — `check_agents_md.py`, the checker behind `agents-md-integrity.yml` (enforces this AGENTS.md standard). Tests in `tests/`. +- `.github/groom/` — building blocks for the reusable **groom** code-cleanup + workflow (epic BE-3870). Currently `ledger.py`, the durable dedup/rejection + memory that stops the stateless groom CI run from re-filing already-filed or + human-rejected findings (it uses GitHub issue state as the store — no new + secret). Tests in `tests/`. - `.github/bump-callers/` — `bump-callers.sh`, the ONE fleet-agnostic script that opens SHA-bump PRs in consumer repos when a reusable workflow changes. Tests in `tests/`.