From e0c2a0a148b29750232fad436611891b1cf48875 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Thu, 6 Aug 2026 06:34:07 -0700 Subject: [PATCH 1/5] Read a Copilot refusal as a refusal, not as coverage (#584) Copilot answers a pull request it will not take on with a formal review, `state: COMMENTED`, carrying the correct `commit.oid` and zero inline threads, whose whole body is a refusal. `pr_review.py` treated a review carrying the head as coverage, so the digest read pr=108 head=9f56a472 rounds=1 review_on_head=yes threads=0 unresolved=0 suppressed=0 answered_outside_review=no requested=no merge=CLEAN which is the clean pass byte for byte, and `wait` exited 0. Observed on a pull request of 301 changed files, one over the reviewer's limit of 300, which was one command from merging on a round that never ran. The existing guard does not reach it. Exit 40 rests on a comment carrying no commit, so it satisfies no coverage check whatever it says. This carries the right commit. Only the delivery mechanism differs, and the mechanism is what the guard keys on. ## The reading A refusal is not coverage. `reviewed_head` and the digest's `review_on_head` both exclude it, so the state reports as NO, and the summary line carries a `refusal=` field of its own, because `rounds=1 review_on_head=NO` is equally what a stale round looks like and the two want opposite responses. `wait` exits 41 and the body prints whole, since the wording is the only thing separating a file-count refusal, cleared by splitting the pull request, from a quota one, cleared by waiting. The script reads neither cause. Head-scoped, unlike a suppressed finding: a refusal is a statement about one commit that a push retires, and a genuine review of the same head outranks it. Both surfaces spend it on that coverage, the exit code and the field alike, or the line reads `review_on_head=yes refusal=YES` and tells a reader to split a pull request the reviewer has just reviewed. The liveness query still carries no bodies, so a refusal reads there as ordinary coverage, which ends the wait, and the full read every wait finishes with is what tells the two apart. No exit code comes from the cheaper reading, so the per-poll cost is unchanged. ## The unit is the body's opening line Matching anywhere in the body reports this pull request as a refusal of itself, the same false positive the suppressed matcher already had once. Two lines is not enough either: a review's first line is its heading and its second is the overview prose, which is exactly where a review describing this check states the wording. A case pins that, and it is the case that failed while every other one passed. The cost runs the other way, that a refusal introduced by a heading would be missed. The alternation is the runbook's, and a case asserts the script's pattern is the published one rather than a copy that can drift. The dot spans both spellings of the apostrophe and keeps the published `--jq` filter usable inside single quotes, which neither spelling survives. ## Docs The same guarantee was stated on four surfaces, so each moves: the runbook's clean-pass rule and its coverage-confirmation paragraph, and `GOVERNANCE.md` merge-gate condition 2 and review-loop step 3, which now say the matched review is read rather than only counted. The runbook also records the reviewer-side fact an agent hitting this needs, that the limit is 300 changed files and the remedy is to split the pull request. `GOVERNANCE.md` is verbatim-carried, so a fleet re-vendor is owed. The issue's second point, `--repo` defaulting to this repository, was fixed in #575 and needs nothing here. Fixes #581. Co-authored-by: Claude Opus 5 (1M context) --- .github/copilot-instructions.md | 14 +++- GOVERNANCE.md | 4 +- scripts/README.md | 2 + scripts/pr_review.py | 93 +++++++++++++++++++++- scripts/test_pr_review.py | 137 ++++++++++++++++++++++++++++++++ 5 files changed, 244 insertions(+), 6 deletions(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index cf856d7c..fc8ba252 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -37,6 +37,18 @@ Auto-review on push is configured (via the branch ruleset's `copilot_code_review **A review with no inline comments is still a completed review, not a failure, and not a reason to ask the maintainer to re-trigger.** Copilot very often posts a single formal review (GraphQL `state: COMMENTED`) whose body ends with "...reviewed N of N changed files ... and generated no comments" and adds **zero** inline threads. That review carries the head `commit.oid` and fully satisfies the loop, and it is the clean-pass success case. Never read "no inline comments" as "the review didn't run," and never re-request or escalate to the maintainer because comments are absent. +**The one exception is a review that says it did not review, and it is delivered in exactly that shape.** Copilot answers a pull request it will not take on with a formal review, `state: COMMENTED`, carrying the correct `commit.oid` and **zero** inline threads, whose whole body is a refusal: "Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again." Every coverage check passes, the rule above says an empty review is the clean pass, and the two together read a round that never happened as a round that found nothing. Observed on a pull request of 301 changed files, one over the limit, which was one command from merging on it. **The limit is 300 changed files and the remedy is to split the pull request**, since re-requesting the same head repeats the refusal: the file count is what it declined on and re-requesting does not change it. A repository committing binary or generated data alongside code crosses that line easily. Match the refusal on the body's **opening line** rather than anywhere in it, because a review discussing the wording is not one carrying it, and one line rather than two, because a review's first line is its heading and its second is the overview prose where such a description sits. Match an alternation for the same reason the suppressed heading takes one: + +```sh +# A review whose opening line declines the round. That line is the unit, since a refusal is +# the whole body and a match further down is a review quoting the wording rather than refusing. +# The dot spans both spellings of the apostrophe, the typographic one Copilot writes and the +# ASCII one, and it also keeps this filter usable inside single quotes, which neither survives. +gh api repos///pulls//reviews --jq \ + '.[] | select([(.body // "") | split("\n")[] | select(. != "")][0] // "" + | test("wasn.t able to review|was not able to review|unable to review")) | {commit_id, body}' +``` + **Read the low-confidence findings, which are not inline threads.** A review body can carry a collapsed `
` block of findings Copilot withheld from the inline threads, and those findings appear nowhere in `reviewThreads`, so a loop that polls threads alone never sees them and reports a clean pass. **Match the block on more than one phrasing.** Its heading has appeared both as `Suppressed comments (N)` and as "Comments suppressed due to low confidence", so a filter keyed on either one alone silently reports zero suppressed findings on a review that has them, the same false clean this rule exists to prevent, one level up in the detection. **The section moves as well as it is worded, so match the heading wherever it sits.** It has appeared as its own `
` wrapper with a matching ``, as a bare heading in the body, and as a Markdown heading nested inside the `Review details` wrapper, whose `` names the wrapper and not the section. A filter reading a wrapper's `` reports zero on the nested shape, and the count it needs is the heading's own `(N)` rather than the wrapper's. They have been right repeatedly, including a rule stated more broadly than its check enforced and a check that skipped fenced blocks in every rule but one. Read the body of every review, investigate each suppressed finding on the same footing as an inline one, and answer it in the PR conversation, since a suppressed finding has no thread to reply on or resolve. ```sh @@ -141,7 +153,7 @@ gh api repos///issues//comments --jq \ '[.[] | select(.user.login=="copilot-pull-request-reviewer[bot]")] | last | {created_at, body: .body[:200]}' ``` -Coverage is confirmed when (1) exits 0, and **a formal review with no inline comments still satisfies path (1)**, because coverage is about the head SHA, not the comment count. For issue comments (path 2), body content is the only reliable signal, and `created_at` is not: `git log -1 --format=%cI` is the **commit** timestamp, not the push timestamp, so amended or rebased commits can have an earlier timestamp and an older Copilot comment could satisfy a time check even though Copilot never saw the current head. Treat path (2) as confirmed only when the comment body explicitly refers to the current changes. +Coverage is confirmed when (1) exits 0, and **a formal review with no inline comments still satisfies path (1)**, because coverage is about the head SHA, not the comment count. The exception is the refusal above, which is a formal review on the head with no inline comments and covers nothing, so path (1) exits 0 over a round that never ran. Read the body of the review the SHA matched, not only the SHA. For issue comments (path 2), body content is the only reliable signal, and `created_at` is not: `git log -1 --format=%cI` is the **commit** timestamp, not the push timestamp, so amended or rebased commits can have an earlier timestamp and an older Copilot comment could satisfy a time check even though Copilot never saw the current head. Treat path (2) as confirmed only when the comment body explicitly refers to the current changes. ### Bounded Retry Workflow diff --git a/GOVERNANCE.md b/GOVERNANCE.md index 29c665fa..8e743ab5 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -246,7 +246,7 @@ The repo runs a review loop on every PR: local agent iteration plus remote autom **Do not merge, and do not enable auto-merge, unless ALL of these hold:** 1. Required status checks are green (`mergeStateStatus: CLEAN`), **and** -2. A Copilot review is confirmed on the **current head SHA** by matching the review's commit SHA to the head, not an earlier push, because a push makes required checks go green **before** the re-review lands, so a green merge-state can precede the current-head review and never signals readiness on its own, **and** +2. A Copilot review is confirmed on the **current head SHA** by matching the review's commit SHA to the head, not an earlier push, because a push makes required checks go green **before** the re-review lands, so a green merge-state can precede the current-head review and never signals readiness on its own, and the matched review is **read** rather than only counted, because Copilot declines a pull request it will not take on with a formal review carrying that same head SHA and no findings, which matches the SHA and covers nothing, **and** 3. **Every** Copilot finding on that head SHA is closed out, with all review threads resolved, **and** any issue-level Copilot comments (which have no resolve action) triaged and replied to, **and** the low-confidence findings collapsed in the review body investigated and answered, since those appear in no thread and a loop that polls threads alone reports a clean pass while they stand, so zero outstanding findings remain, **and** 4. The maintainer has given **explicit** permission to merge. @@ -258,7 +258,7 @@ The repo runs a review loop on every PR: local agent iteration plus remote autom 1. Push changes to the PR branch. 2. Re-request a review for the **current head SHA**. Auto-trigger is unreliable, so request it explicitly via the `requestReviews` GraphQL mutation (reliable end-to-end, per the runbook). The UI is only a fallback. -3. Wait for review activity on that head. A completed review that raises **no findings** is a valid terminal outcome for that head, so proceed. Do not re-trigger it or treat the absence of comments as a missing review. +3. Wait for review activity on that head. A completed review that raises **no findings** is a valid terminal outcome for that head, so proceed. Do not re-trigger it or treat the absence of comments as a missing review. A review whose body says it did not review is the one exception, and it is terminal in the other direction: nothing follows it, re-requesting the same head repeats it, and the body names what has to change first. 4. Triage findings. 5. Apply fixes or write a rationale for declines. 6. Reply to each thread and resolve what was addressed. diff --git a/scripts/README.md b/scripts/README.md index c1c8a9b9..a5272633 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -101,6 +101,8 @@ python3 scripts/pr_review.py wait 452 --repo ptr727/ProjectTemplate --timeout 27 Everything else is decidable and says so. One of the reviewer's own nodes in view, even a **spent** one, settles the question, because nodes arrive in creation order, so anything behind the window is older than everything inside it. A window holding every node the pull request has is settled too, which is why the guard reads `pageInfo.hasPreviousPage` rather than the node count: a full window and a complete one are the same length, so length alone would report a gap where none exists. Cases hold `WINDOW` equal across all four windows and hold all four to asking for `hasPreviousPage`, since a connection that stops asking reports `no` instead of `unknown`, the silent narrowing one level up. `wait` exits `50` when the reviewer sits in the pending request set and no `copilot_work_started` follows the newest request, meaning nothing is acting on it and waiting on will not start it. That state is invisible from the reviews alone and indistinguishable from patience: one request sat thirteen and a half hours while the pull request read as waiting on the reviewer. Elapsed time cannot separate it from a slow round either, so the pickup event decides. It is the one thing here read over REST, since no GraphQL timeline item carries it, and it runs on its own interval rather than per poll: the first read comes after `--pickup-grace` (default five minutes), because inside that window a pending request is simply a review being worked on, and each later read waits another interval. One reading settles the request in front of it, and the next covers a request a push raises mid-wait, so a long wait costs a handful of REST calls instead of one per poll. The pickup is checked **before** the timeout, so the stall reports as itself instead of as `PENDING` once the clock runs out. Recovery stays out of this script, which holds its no-mutation contract: the digest names the state and the runbook carries the two mutations that clear and re-raise the request. The pending set is read through GraphQL rather than `gh pr view --json reviewRequests`, which omits a Bot reviewer outright and reports an empty set while Copilot sits in it. +`wait` exits `41` when the review carrying the head is a **refusal**, meaning its body opens by saying it did not review. That answer arrives as a formal review, `state: COMMENTED`, with the correct commit and zero inline threads, so it satisfies every coverage check a clean pass does and renders a digest byte for byte identical to one. The `40` reasoning does not reach it, because that reasoning rests on a comment carrying no commit, and this carries the right one. A pull request of 301 changed files, one over the reviewer's limit of 300, read as `rounds=1 review_on_head=yes threads=0 unresolved=0 merge=CLEAN` and was one command from merging on a review that never ran. A refusal is therefore not coverage: `review_on_head` reports `NO`, the summary line carries a `refusal=YES` of its own, since `rounds=1 review_on_head=NO` is equally what a stale round looks like and the two want opposite responses, and the body prints whole because its wording is the only thing separating a file-count refusal, cleared by splitting the pull request, from a quota one, cleared by waiting. The script reads neither cause, only that the round declined. The match is on the body's **opening line**, since a refusal is the whole body where a review that merely quotes the wording carries it below its own overview, and this script and this file are exactly that quotation. One line rather than two, because a review's first line is its heading and its second is the overview prose: reading two passed every case except the review describing this check, which reported itself as a refusal of itself. The cost is the other direction, that a refusal introduced by a heading would sit below the opening and be missed, and answering that shape means telling a refusal from an overview rather than reading one line further. It is an alternation over the runbook's phrasings for the same reason the suppressed heading is, and a case asserts the script's pattern is the one the runbook publishes. The reading is **head-scoped**, unlike a suppressed finding, because a refusal is a statement about one commit that a push retires, and a genuine review of that same head outranks it, coverage that landed being coverage. The field is spent by that coverage as well as the exit code is, or the summary line reads `review_on_head=yes refusal=YES` and tells a reader to split a pull request the reviewer has just reviewed. The liveness query carries no bodies, so a refusal reads there as ordinary coverage. That is deliberate: it ends the wait, which is what a terminal outcome should do, and the full read every wait finishes with is what tells the two apart, so no exit code comes from the cheaper reading. + The timeout path prints the full digest for the same reason, as a bare `PENDING` line reports a slow reviewer and a broken poll identically, which is the reading that turns a stalled watcher into a watcher nobody notices is stalled. The digest also reports the **suppressed findings** a review body collapses into a `
` block. Those reach no review thread, so a loop that polls threads alone reports a clean pass while they stand, and the [merge gate][governance] counts them as outstanding findings either way. `suppressed=N` counts findings rather than blocks, reading the `(N)` the heading carries, since one body holds one block per round and counting blocks reports two findings as one. It covers **every** round rather than the current head, because a suppressed finding has no resolved state for a push to retire: head-scoping read "superseded by a push" as "answered", and a finding nobody replied to left the digest the moment the branch moved, so the run reported zero. That is how four rounds went unanswered across three pull requests in one day, each found by the maintainer rather than by this script. The summary line splits the count as `suppressed=N (on_head=N earlier=N)` and each block is marked with the round that raised it, since a finding on an older round may since be moot and deciding that is the reader's call rather than one the count should make for them. Each block prints whole where a thread body truncates, because a thread can be re-read at its id and a suppressed finding cannot, and it prints under a marker naming what closing it takes: no thread exists to reply on or resolve, so the answer goes in the PR conversation. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index b0faa377..95161235 100644 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -15,6 +15,7 @@ 40 = Copilot answered outside a formal review, so read the printed body. 40 reports the shape of that answer and reads nothing of its cause: an answer carrying no commit covers no head, so the wait ends and the reader decides. + 41 = the review carrying the head says it did not review, so it covers nothing. 50 = the request is pending and nothing picked it up, which no amount of waiting changes. Recovery is two mutations, and they stay in the runbook. @@ -33,6 +34,14 @@ # The alternation is the runbook's, since the heading wording has changed once already. # Matching one phrasing alone reports zero on a review that has them. SUPPRESSED = re.compile(r'Suppressed comments|low confidence', re.IGNORECASE) +# A refusal declines the round as a formal review carrying the head and no threads. +# That is the clean pass byte for byte, so every coverage check passes over a round that never ran. +# The alternation is the runbook's for the same reason the one above is. +# One phrasing is one rewording away from reading a refusal as a review. +# The dot covers the apostrophe in the typographic spelling and the ASCII one alike. +# It also keeps the published filter usable inside single quotes, which neither survives. +REFUSAL = re.compile(r'wasn.t able to review|was not able to review|unable to review', + re.IGNORECASE) DETAILS = re.compile(r'
(.*?)
', re.DOTALL | re.IGNORECASE) SUMMARY = re.compile(r'(.*?)', re.DOTALL | re.IGNORECASE) TAGS = re.compile(r'', re.IGNORECASE) @@ -197,10 +206,60 @@ def stall_of(owner: str, repo: str, num: int, pr: dict) -> str: return never_picked_up(timeline(owner, repo, num)) +def refusal_of(node: dict) -> str: + """The review's body where its opening line says the reviewer did not review, otherwise empty. + + Read over the opening rather than the whole body, because a refusal replaces the review and + is the only thing the body carries, where a review that merely quotes the wording carries it + below its own overview. This script and its documentation are that quotation, so a body-wide + match would report the pull request adding this check as a refusal, which is the false + positive the suppressed-block matcher already had once. + + The opening is one line rather than two, and the second line is where the cost of widening + it shows: a review's first line is its heading and its second is the overview prose, which + is exactly where a review describing this check states the wording. Reading two lines passed + every case here except that one, which is the case that matters. A refusal introduced by a + heading would sit below the opening and be missed, and answering that shape means telling it + from an overview rather than reading one line further. + """ + body = node.get('body') or '' + opening = next((ln for ln in body.splitlines() if ln.strip()), '') + return body if REFUSAL.search(opening) else '' + + +def refusing_review(pr: dict) -> dict | None: + """The reviewer's newest refusal carrying the current head, where one is there. + + Head-scoped, unlike a suppressed finding, because a refusal is a statement about one commit: + a push retires it, and the round the push raises either reviews that head or refuses it in + its own right. + + A refusal alongside a genuine review of the same head is spent too, and that is the caller's + reading rather than this one's, since what spends it is coverage this cannot see from a + refusal alone. Both callers hold it: `main` returns 0 on `reviewed_head` before reaching + here, and the digest reports the field only where nothing covers the head. + """ + head = pr['headRefOid'] + refusals = [n for n in reviewer_nodes(pr, 'reviews') + if (n.get('commit') or {}).get('oid') == head and refusal_of(n)] + return max(refusals, key=lambda n: n.get('submittedAt') or '') if refusals else None + + def reviewed_head(pr: dict) -> bool: - """True where one of the reviewer's own reviews carries the current head's commit.""" + """True where one of the reviewer's own reviews covers the current head's commit. + + A refusal is not coverage. It is a formal review, `state: COMMENTED`, carrying the head's + commit and raising no threads, so it satisfies every check a clean pass does and renders a + digest identical to one. That is how a pull request of 301 changed files, one over the + reviewer's limit, sat one command from merging on a review that never ran. + + The liveness query carries no bodies, so a refusal reads there as ordinary coverage. That is + deliberate rather than a gap: it ends the wait, which is what a terminal outcome should do, + and the full read every wait finishes with is what tells the two apart. No exit code and no + merge decision is taken from the liveness reading. + """ head = pr['headRefOid'] - return any((n.get('commit') or {}).get('oid') == head + return any((n.get('commit') or {}).get('oid') == head and not refusal_of(n) for n in reviewer_nodes(pr, 'reviews')) @@ -270,7 +329,10 @@ def digest(owner: str, repo: str, num: int, seen: set[str] | None = None, stalled = stall_of(owner, repo, num, pr) if stalled is None else stalled head = pr['headRefOid'] revs = reviewer_nodes(pr, 'reviews') - on_head = [n for n in revs if (n.get('commit') or {}).get('oid') == head] + # A refusal carries the head and covers nothing, so it counts as a round and not as coverage. + # Reading it as coverage prints `review_on_head=yes` over a review that says it did not run. + on_head = [n for n in revs + if (n.get('commit') or {}).get('oid') == head and not refusal_of(n)] threads = pr['reviewThreads']['nodes'] # A deleted account leaves `author` present and null, which `.get('author', {})` returns as # None rather than as the default, so the chained lookup crashes the whole digest. @@ -291,6 +353,10 @@ def digest(owner: str, repo: str, num: int, seen: set[str] | None = None, finding_count(b) for b in on_head_blocks) answer = answered_outside_review(pr) + # Spent where coverage of the same head landed, the precedence the exit codes already hold. + # Reported regardless, it prints `review_on_head=yes refusal=YES` over a reviewed head. + # That tells a reader to split a pull request the reviewer has just reviewed. + refusal = None if on_head else refusing_review(pr) blind = [f for f in ('reviews', 'comments') if window_blind(pr, f)] answered = 'yes' if answer else ('unknown' if blind else 'no') lines = [ @@ -298,6 +364,10 @@ def digest(owner: str, repo: str, num: int, seen: set[str] | None = None, # A digest of the wrong pull request is well-formed, so naming it is what shows the miss. f'repo={owner}/{repo} pr={num} head={head[:8]} rounds={len(revs)} ' f'review_on_head={"yes" if on_head else "NO"} ' + # A field of its own, since `rounds=1 review_on_head=NO` is also what a stale round is. + # The two want opposite responses, one a re-request and the other a split pull request. + # Upper-case for the reason `NO` is, as a state that blocks a merge is not one to skim. + f'refusal={"YES" if refusal else "no"} ' f'threads={len(threads)} unresolved={len(unresolved)} ' f'suppressed={sum(finding_count(b) for n, b in blocks)} ' f'(on_head={sum(finding_count(b) for b in on_head_blocks)} earlier={stale}) ' @@ -305,6 +375,15 @@ def digest(owner: str, repo: str, num: int, seen: set[str] | None = None, f'requested={"yes" if reviewer_requested(pr) else "no"} ' f'merge={pr.get("mergeStateStatus")}' ] + if refusal: + # Printed whole for the reason the comment below is, as the wording carries the remedy. + # A file-count refusal is cleared by splitting the pull request and a quota one by waiting. + # This reads neither cause, only that the round declined. + lines.append(' COPILOT REFUSED THIS ROUND: the review carrying the head says it did ' + 'not review, so it covers nothing and re-requesting the same head repeats ' + 'it, and the body below is what says which remedy applies') + lines += [f' {ln.rstrip()}' for ln in (refusal.get('body') or '').splitlines() + if ln.strip()] if stalled: lines.append(f' REQUEST NOT PICKED UP (requested {stalled}, no copilot_work_started ' 'since): clear the request and re-request, per the runbook') @@ -424,6 +503,14 @@ def main(argv: list[str] | None = None) -> int: print(f'waited={int(time.monotonic()-start)}s') if reviewed_head(final): return 0 + # A refusal before an answer, since it names the round that declined where 40 names none. + # The digest prints both bodies regardless, so the narrower code costs the reader nothing. + if refusing_review(final): + print('status=REVIEW_IS_A_REFUSAL the review carrying the head says it did not review, ' + 'so it covers nothing and no further review follows it: read the body above, ' + 'since a file-count refusal is cleared by splitting the pull request and a quota ' + 'one by waiting, and re-requesting this head clears neither') + return 41 # An answer before a stall, because the reviewer saying something outranks it saying nothing. if answered_outside_review(final): print('status=ANSWERED_OUTSIDE_REVIEW the reviewer answered without reviewing, ' diff --git a/scripts/test_pr_review.py b/scripts/test_pr_review.py index 392f4463..e9758651 100644 --- a/scripts/test_pr_review.py +++ b/scripts/test_pr_review.py @@ -60,6 +60,11 @@ def nested(heading: str = '### Suppressed comments (2)', '- **Review effort level:** Lite\n
\n') +REFUSED = ("Copilot wasn't able to review this pull request because it exceeds the maximum " + 'number of files (300). Try reducing the number of changed files and requesting a ' + 'review from Copilot again.') + + def thread(tid: str, resolved: bool = False, login: str = pr_review.REVIEWER, body: str = 'A finding.', path: str = 'a.py', line: int = 1) -> dict: return {'id': tid, 'isResolved': resolved, @@ -424,6 +429,106 @@ def test_a_human_review_carrying_the_phrase_is_not_a_copilot_finding(self) -> No self.assertIn('suppressed=0', out) +class TestRefusal(GqlCase): + """The review that says it did not review, which carries the head and covers nothing. + + It is a formal review with the correct commit and no threads, so every check a clean pass + satisfies it satisfies too, and the digest it renders is the clean pass byte for byte. The + pull request it was observed on had 301 changed files, one over the reviewer's limit, and was + one command from merging on a round that never ran. + """ + + def test_a_refusal_on_the_head_is_not_coverage(self) -> None: + self.answer(payload([review(body=REFUSED)])) + out, _ = pr_review.digest('o', 'r', 7) + self.assertIn('review_on_head=NO', out) + self.assertIn('refusal=YES', out) + # It happened, so it is still a round. + # What it is not is a review of anything. + self.assertIn('rounds=1', out) + self.assertFalse(pr_review.reviewed_head(payload([review(body=REFUSED)]))) + + def test_the_body_prints_whole_under_a_marker_naming_the_remedy(self) -> None: + """Its wording is what separates a file-count refusal from a quota one, so it is not cut.""" + self.answer(payload([review(body=REFUSED)])) + out, _ = pr_review.digest('o', 'r', 7) + self.assertIn('COPILOT REFUSED THIS ROUND', out) + self.assertIn(REFUSED, out) + + def test_each_documented_phrasing_counts_including_the_typographic_apostrophe(self) -> None: + """One phrasing alone is one rewording away from reporting a refusal as a review.""" + # The typographic apostrophe is an escape, since the charset rule governs this file too. + # The case is about the byte the reviewer sends rather than the character on screen. + for body in ("Copilot wasn't able to review this pull request because it is too large.", + 'Copilot wasn\u2019t able to review this pull request.', + 'Copilot was not able to review this pull request.', + 'Copilot is unable to review this pull request right now.'): + with self.subTest(body=body): + self.assertEqual(body, pr_review.refusal_of({'body': body})) + + def test_a_clean_pass_and_an_ordinary_review_are_not_refusals(self) -> None: + for body in ('Reviewed 3 of 3 changed files and generated no comments.', + collapsed(), nested(), ''): + with self.subTest(body=body[:40]): + self.assertEqual('', pr_review.refusal_of({'body': body})) + + def test_a_review_quoting_the_wording_below_its_overview_is_not_a_refusal(self) -> None: + """This pull request's own review body is that quotation, and the shape has bitten once. + + The suppressed matcher read the whole body and reported the review that discussed + suppressed findings as carrying them. The opening is the unit for that reason: a refusal + is the whole body, so a match further down is a review describing the wording. + + This is also what fixes the unit at one line. The overview prose is the second line of + every review body, and reading two lines reports this review as a refusal of itself. + """ + body = ('## Pull request overview\n\nThis PR treats a review that says Copilot ' + "wasn't able to review a pull request as a terminal state rather than " + 'as coverage.\n\n- The digest now reports `unable to review` separately.\n') + self.assertEqual('', pr_review.refusal_of({'body': body})) + self.answer(payload([review(body=body)])) + out, _ = pr_review.digest('o', 'r', 7) + self.assertIn('review_on_head=yes', out) + self.assertIn('refusal=no', out) + + def test_a_refusal_from_an_earlier_round_is_spent(self) -> None: + """A refusal is a statement about one commit, so the push that changed it retires it.""" + self.answer(payload([review(oid=OLD, body=REFUSED), review()])) + out, _ = pr_review.digest('o', 'r', 7) + self.assertIn('review_on_head=yes', out) + self.assertIn('refusal=no', out) + + def test_a_genuine_review_of_the_same_head_outranks_a_refusal_of_it(self) -> None: + """Coverage that landed is coverage, whatever an earlier round of the same head said. + + The digest has to agree with the exit codes here, which return 0 on that coverage and + never reach 41. Reported unconditionally the field read `review_on_head=yes refusal=YES` + on one line, which tells a reader to split a pull request that has just been reviewed, + and tells an automated one that a state it treats as actionable is outstanding. + """ + pr = payload([review(body=REFUSED, at=EARLY), review(at=LATE)]) + self.assertTrue(pr_review.reviewed_head(pr)) + self.answer(pr) + out, _ = pr_review.digest('o', 'r', 7) + self.assertIn('review_on_head=yes', out) + self.assertIn('refusal=no', out) + self.assertNotIn('COPILOT REFUSED THIS ROUND', out) + self.assertNotIn(REFUSED, out) + + def test_the_wait_returns_zero_where_coverage_followed_a_refusal_of_the_same_head(self) -> None: + """The digest and the exit code read one payload, so neither may spend it differently.""" + self.answer(payload([review(body=REFUSED, at=EARLY), review(at=LATE)])) + with contextlib.redirect_stdout(io.StringIO()) as out, \ + mock.patch.object(pr_review.time, 'sleep'): + self.assertEqual(0, pr_review.main(['wait', '7', '--repo', 'o/r'])) + self.assertNotIn('status=REVIEW_IS_A_REFUSAL', out.getvalue()) + + def test_a_human_review_carrying_the_wording_is_not_the_reviewer_refusing(self) -> None: + self.answer(payload([review(login='ptr727', body=REFUSED), review(oid=OLD)])) + out, _ = pr_review.digest('o', 'r', 7) + self.assertIn('refusal=no', out) + + class TestDigestReportsTheAnswer(GqlCase): def test_the_comment_prints_whole_under_a_marker_naming_it_terminal(self) -> None: """Its wording is what separates a refusal from a remark, so it is not truncated.""" @@ -527,6 +632,31 @@ def test_wait_ends_on_an_answer_outside_a_review_instead_of_waiting_it_out(self) self.assertIn('status=ANSWERED_OUTSIDE_REVIEW', out) self.assertIn('quota', out) + def test_wait_exits_forty_one_on_a_review_that_says_it_did_not_review(self) -> None: + """Returning zero here is the failure: the digest is the clean pass byte for byte.""" + self.answer(payload([review(body=REFUSED)])) + with mock.patch.object(pr_review.time, 'sleep') as slept: + self.assertEqual(41, self.cli(['wait', '7', '--timeout', '0'])) + # Terminal, so it ends the wait rather than polling out the timeout against it. + slept.assert_not_called() + out = self.out.getvalue() + self.assertIn('status=REVIEW_IS_A_REFUSAL', out) + self.assertIn('review_on_head=NO', out) + self.assertIn(REFUSED, out) + + def test_the_liveness_reading_ends_the_wait_and_the_full_read_refuses_it(self) -> None: + """The liveness query carries no bodies, so a refusal reads there as ordinary coverage. + + That is what ends the loop, and nothing decides on it: the full read that follows every + wait carries the body and is where the exit code comes from. + """ + bodyless = {k: v for k, v in review().items() if k != 'body'} + self.answer(payload([bodyless]), payload([review(body=REFUSED)])) + with mock.patch.object(pr_review.time, 'sleep') as slept: + self.assertEqual(41, self.cli(['wait', '7', '--timeout', '600'])) + slept.assert_not_called() + self.assertIn('status=REVIEW_IS_A_REFUSAL', self.out.getvalue()) + def test_a_landed_review_wins_over_an_older_answer(self) -> None: """Coverage is the success case, and a spent comment does not downgrade it to 40.""" self.answer(payload([review(at=LATE)], comments=[comment(at=EARLY)])) @@ -681,6 +811,13 @@ def test_the_suppressed_pattern_is_the_runbook_alternation(self) -> None: text = RUNBOOK.read_text(encoding='utf-8') self.assertIn(f'test("{pr_review.SUPPRESSED.pattern}")', text) + def test_the_refusal_pattern_is_the_runbook_alternation(self) -> None: + """A refusal reworded once is a refusal read as coverage, so the pattern is not a memory.""" + text = RUNBOOK.read_text(encoding='utf-8') + self.assertIn(f'test("{pr_review.REFUSAL.pattern}")', text) + # The published filter is single-quoted, which no spelling of the apostrophe survives. + self.assertNotIn("'", pr_review.REFUSAL.pattern) + def test_no_mutation_reaches_this_script(self) -> None: """Mutations stay as explicit `gh` calls so the write-guard hook and review still see them.""" source = (REPO / 'scripts' / 'pr_review.py').read_text(encoding='utf-8') From e848a4da832d3628e14550ffdcc04be1905572bd Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Thu, 6 Aug 2026 06:53:03 -0700 Subject: [PATCH 2/5] Name the collation for the reference-definition sort order (#586) "Alphabetized by reference name" is ambiguous wherever one label is a prefix of another, and the two readings disagree on that pair alone. Sorting the bare label puts `[governance]` first, while sorting the whole definition line puts `[governance-branching-model]` first, because `-` (0x2D) precedes `]` (0x5D). A block ordered either way is internally consistent, so a reviewer reads the other one as a defect and asks for a re-sort on every pull request that touches a definition block. ## The reading is already in the tree Every reference-definition group this repository carries, twelve of them across ten files, sorts by the bare label. `[governance]` above `[governance-branching-model]`, `[repo-config]` above `[repo-config-settings]`, `[vscode-tasks]` above `[vscode-tasks-python]`. None sorts by the line. The rule's own wording says "by reference name", so the bare label is both what the sentence says and what the corpus does, and the line reading would mean re-sorting `README.md`, `AUDIT.md`, `CODESTYLE.md`, `STANDUP.md`, `TODO.md`, and five more against a rule that never asked for it. `GOVERNANCE.md` now names the key rather than leaving it inferred: the text inside the brackets, never the whole line, so a shorter name sorts above one it prefixes. It also records the trap, which is that a plain `sort -c` over the block passes on the inverted order, since that command compares the line. A block can therefore be mechanically "sorted" and still be wrong, which is how a consistent block reaches review. `spec/readme-structure.md` restates the same rule for a repository that reads the spec without the governance file, so it carries the key too. Leaving it at "alphabetized within each group" would hand the same guess to exactly the reader who has nothing else to check against. ## Two blocks were unsorted under either reading `WORKFLOW.md` and `docs/repo-config-carry.md` each carry a `` group ordered by neither collation. Both are fixed here, so the canonical satisfies the rule it states. ## Verification `prose_lint.py --diff`, `repo_gate.py`, and `markdownlint-cli2` are clean over the change, and a scan of every definition group in the tree reports zero violating the stated order. `GOVERNANCE.md` is verbatim-carried, so a fleet re-vendor is owed. Fixes #583. Co-authored-by: Claude Opus 5 (1M context) --- GOVERNANCE.md | 2 +- WORKFLOW.md | 2 +- docs/repo-config-carry.md | 2 +- spec/readme-structure.md | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/GOVERNANCE.md b/GOVERNANCE.md index 8e743ab5..9d10545a 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -145,7 +145,7 @@ Clarify devcontainer setup steps in README ### Markdown -- **Reference-style links in human-facing docs.** Every Markdown file uses reference-style links only, **except** the four agent-instruction files that keep inline links: `AGENTS.md`, `GOVERNANCE.md`, `OPERATIONS.md`, and `.github/copilot-instructions.md`. Those are read one section at a time rather than end to end, so an inline target resolves where it is read while a definition at the bottom of the file is never reached. The exception is that closed list rather than a category to argue from, so every other Markdown file follows the rule whatever its audience. Under the rule: every URI (internal path, anchor, external URL, or shield image) is defined at the **bottom of the file**, split into groups by type under an HTML-comment header (e.g. ``, ``, ``, ``) with each group's definitions alphabetized by reference name. **Reference names are contextual and encode the target and its group**: `foo-shield` for a shield image, `foo-link` for an external URL, and a bare `foo` for a local path or anchor (e.g. `[license-shield]`, `[releases-link]`, `[repo-config]`), never numeric (`[1]`) or opaque. No inline `[text](uri)` targets in prose. **A URL inside a fenced code block stays inline**, because reference links do not resolve in code blocks, so do not extract it, and exclude fenced code from any link-integrity check (bracket literals like `["a", "b"]` otherwise read as undefined references). **Removing a link also removes its reference definition**, since an orphaned definition fails the no-unused-defs rule. The one exception is the Table of Contents, whose entries stay inline anchor links (see Table of Contents below). +- **Reference-style links in human-facing docs.** Every Markdown file uses reference-style links only, **except** the four agent-instruction files that keep inline links: `AGENTS.md`, `GOVERNANCE.md`, `OPERATIONS.md`, and `.github/copilot-instructions.md`. Those are read one section at a time rather than end to end, so an inline target resolves where it is read while a definition at the bottom of the file is never reached. The exception is that closed list rather than a category to argue from, so every other Markdown file follows the rule whatever its audience. Under the rule: every URI (internal path, anchor, external URL, or shield image) is defined at the **bottom of the file**, split into groups by type under an HTML-comment header (e.g. ``, ``, ``, ``) with each group's definitions alphabetized by reference name. **The sort key is the reference name alone**, the text inside the brackets, never the whole definition line: where one name is a prefix of another, the shorter one sorts first (`[governance]` above `[governance-branching-model]`, `[repo-config]` above `[repo-config-settings]`). Sorting the line instead inverts every such pair, because `-` precedes `]` in byte order, so the two readings disagree on exactly the names a reader looks up together, and a plain `sort -c` over the block passes on the inverted order. **Reference names are contextual and encode the target and its group**: `foo-shield` for a shield image, `foo-link` for an external URL, and a bare `foo` for a local path or anchor (e.g. `[license-shield]`, `[releases-link]`, `[repo-config]`), never numeric (`[1]`) or opaque. No inline `[text](uri)` targets in prose. **A URL inside a fenced code block stays inline**, because reference links do not resolve in code blocks, so do not extract it, and exclude fenced code from any link-integrity check (bracket literals like `["a", "b"]` otherwise read as undefined references). **Removing a link also removes its reference definition**, since an orphaned definition fails the no-unused-defs rule. The one exception is the Table of Contents, whose entries stay inline anchor links (see Table of Contents below). - **Table of Contents.** Generate it with the Markdown All in One extension, which fills and auto-updates the list on save, so leave the `## Table of Contents` heading for the extension to populate and never hand-author or hand-edit the entries. Exclude a heading with an inline `` marker on it (the badge/build header block and the `## Table of Contents` heading itself carry it). The workspace sets which heading levels appear. - One logical paragraph per line, with no hard-wrap line-length limit. For an intentional hard line break within a block (stacked badges, status, or license lines), end the line with a trailing backslash (`\`). This explicit form is preferred over trailing whitespace and is not treated as a paragraph split. - Headings follow the title-case-with-short-bind-words rule from the PR-title section. diff --git a/WORKFLOW.md b/WORKFLOW.md index dcc1e97e..4e9b810e 100644 --- a/WORKFLOW.md +++ b/WORKFLOW.md @@ -293,6 +293,6 @@ Each type maps the *applicable* S-scenarios onto its targets. The differences ar -[governance-branching-model]: ./GOVERNANCE.md#branching-model [codestyle]: ./CODESTYLE.md +[governance-branching-model]: ./GOVERNANCE.md#branching-model [repo-config-readme]: ./repo-config/README.md diff --git a/docs/repo-config-carry.md b/docs/repo-config-carry.md index 9f19afc8..5ae4662f 100644 --- a/docs/repo-config-carry.md +++ b/docs/repo-config-carry.md @@ -54,7 +54,7 @@ done +[conformance-matrix]: ../reports/conformance-matrix.md [governance-documentation-style]: ../GOVERNANCE.md#documentation-style-conventions [governance-git-and-commit-rules]: ../GOVERNANCE.md#git-and-commit-rules -[conformance-matrix]: ../reports/conformance-matrix.md [repo-config-readme]: ../repo-config/README.md diff --git a/spec/readme-structure.md b/spec/readme-structure.md index 43ad9ea4..c8cd568a 100644 --- a/spec/readme-structure.md +++ b/spec/readme-structure.md @@ -34,7 +34,7 @@ Shields are not a top-level section. They live under **Build and Distribution** - US English and straight quotes. Judge a non-ASCII character against the three tiers in GOVERNANCE.md "Character Set", and restructure a sentence rather than joining its clauses with an em dash or with a spaced hyphen, which that section bans in the same terms. - Verify every quantitative claim (counts, versions, supported platforms) against current code. - A project README describes only that project, with no cross-repo references and no template or inheritance framing. -- Reference-style links only: every URI is a reference link defined at the bottom of the file, grouped by type under an HTML-comment header (``, ``, ``, ``) and alphabetized within each group. The auto-generated Table of Contents is the one exception, keeping inline anchor links. +- Reference-style links only: every URI is a reference link defined at the bottom of the file, grouped by type under an HTML-comment header (``, ``, ``, ``) and alphabetized within each group by the reference name alone, the text inside the brackets, so a shorter name sorts above one it prefixes (`[governance]` above `[governance-branching-model]`) rather than below it as sorting the whole definition line would put it. The auto-generated Table of Contents is the one exception, keeping inline anchor links. ## TODO.md From 14c4fe3aa4801a793c0773a54fe5390e0e9a3e42 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Thu, 6 Aug 2026 07:19:44 -0700 Subject: [PATCH 3/5] Run CI on a pull request into an operational develop (#587) The operational lint workflow's prescribed trigger set was push to develop plus pull_request to main, so a pull request opened against develop matched no trigger at all: no validation job, no aggregator report, and a CLEAN mergeable state with an empty check list. That is the exact output D1.2 forbids on any pull request, so the two halves of WORKFLOW.md disagreed. Direct commit to develop is an allowance rather than a substitute for review. The ruleset drops the pull-request requirement, it does not withdraw the pull request, and on the pull-request path the change has not landed yet, which is where validation is worth the most. Widen the prescription to pull_request: branches: [ main, develop ], keeping the push trigger, and record the duplicate run a merged pull request now costs, stated without assuming a squash and with the concurrency key given as the workflow name plus the ref. Add branch.operational.prtriggers as a letter check, since the trigger set is now concrete enough to assert, and note the four downstream operational repos that still need the one-line change. Refs #585 Co-Authored-By: Claude Opus 5 (1M context) --- GOVERNANCE.md | 1 + TODO.md | 2 ++ WORKFLOW.md | 5 ++++- repo-config/README.md | 2 +- spec/project-types.json | 3 ++- 5 files changed, 10 insertions(+), 3 deletions(-) diff --git a/GOVERNANCE.md b/GOVERNANCE.md index 9d10545a..6f252857 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -101,6 +101,7 @@ The registry `workflowModel` field is `release` (the default) or `operational`. **Operational** repos track a live service's running state rather than shipping versioned units of delivery: live-service config such as Home Assistant, ESPHome, Vantage, and home automation. - **Commit configuration directly to `develop`.** There is no feature branch, and the maintainer commits straight to `develop` and *occasionally* opens a `develop -> main` PR to bless a known-good snapshot. The `develop` ruleset drops the PR and status-check gate, so direct signed pushes are allowed (force-push, deletion, and unsigned commits are still blocked) and CI runs on the push as **advisory** feedback that never rejects a commit. +- **A PR into `develop` stays available, and CI runs on it.** Dropping the requirement permits the direct push, it does not withdraw the pull request, so a change worth reviewing takes one and both paths into `develop` are legitimate. The lint workflow's `pull_request` trigger therefore names `develop` alongside `main`, since a trigger set naming `main` alone matches nothing on a `develop` PR and that PR would merge with no validation at all, which is the one output [WORKFLOW.md](./WORKFLOW.md) D1.2 forbids on *any* PR. The result is **reported and not required** there, because a required status check on `develop` would gate the direct push too and dissolve the allowance the model is built on. The asymmetry is the point: on the direct-commit path CI can only be advisory after the fact, while on the PR path the change has not landed yet, so validating it is both pre-merge and actionable. - **The `main` promotion gate is unchanged.** The [`main` ruleset](./repo-config/main.json) is shared with `release` repos, so the `develop -> main` PR still **enforces** the required `Check pull request workflow status job`. For an operational repo that check is lint/validation only, meaning editorconfig/EOL plus domain linters (a Home Assistant or ESPHome config validation, a firmware build) and never unit tests, so `develop` stays the live surface and a broken config can never reach `main`. - **Release only by manual dispatch.** Operational repos carry `releaseTrigger: dispatch-only` and run no codegen or auto-publish bots, so they publish **only** on a manual `workflow_dispatch`, the source-only release the publisher already supports (tag + source zip + README + LICENSE, NBGV-versioned), never automatically. The `develop -> main` promotion just blesses a known-good snapshot, and a release is a separate, deliberate dispatch. - **Fleet sync still applies.** Dependabot's dual-target sync and the App-signed merge-bot run on **every** tier, operational included, so both branches stay in sync and a promotion stays a clean forward merge. diff --git a/TODO.md b/TODO.md index 2ac4df94..97581231 100644 --- a/TODO.md +++ b/TODO.md @@ -80,6 +80,7 @@ Everything here changes a file the fleet holds a copy of, so each entry costs a - Try local defensive-review subagents as a first pass before the remote reviewer, and measure what the pass is worth. The remote loop is thorough and it is also where most of a session's tokens and wall-clock go, and it delivers its findings one round at a time, which is the slowest available way to learn that a change had five problems. A local pass prompted adversarially, one agent per lens rather than one general reviewer, would surface the mechanical half before the branch is even pushed. Scope the experiment to what it can actually prove: run both for a stretch, record which findings the local pass caught that the remote reviewer then raised anyway, and keep it only where that overlap is large enough to shorten the remote loop rather than to add a step in front of it. Note the trap that makes this worth designing carefully rather than just trying. A local pass that finds nothing reads exactly like a clean change, and the next inference from there is that the remote review can be skipped, which is the one outcome the review contract exists to prevent, so the local pass is an input to the loop and never a substitute for the round the merge gate requires. - Record a disproved review finding somewhere the next round will read it, so the same proof is not built twice. [`.github/copilot-instructions.md`][copilot-instructions] already says the reviewer is sometimes factually wrong, already requires a decline to carry evidence, and already keeps a list of known non-working request paths, which is the shape this wants. What it has nowhere to put is the finding itself, so a claim disproved against a downloaded binary in one session is argued from scratch in the next. The worked example is the claim that `keys_unsorted` needs jq 1.6, disproved against a 1.5 binary on which `keys_unsorted` works and `walk/1` does not. Note the reason this is not simply a list to append to, which is that a disproof is true of one tree at one revision, so an entry outliving the code it was proved against becomes a reason not to check, and that is strictly worse than re-proving it. Each entry therefore records what was tested and against which revision, and an entry whose subject changes is deleted rather than edited. - Refresh the README, which has gone stale, and evaluate a lower-maintenance structure, for example a per-section index that points into each doc with a one-line description, keeping the README as the adoption and audit-instruction entry point with pointers to the other docs. A per-section index trades brevity for a sync obligation, since it must track what the docs contain. +- Apply the operational `pull_request` trigger fix from [#585][issue-585] to the four operational repos, `HomeAutomation-Config`, `ESPHome-Config`, `HomeAssistant-Config` and `Vantage-Config`, each of which currently triggers its lint workflow on `pull_request` to `main` only and therefore runs nothing at all on a PR into `develop`. The hub change is prose and spec, so it fixes no downstream repo by itself, and the downstream edit is one line per repo, widening the trigger to `branches: [ main, develop ]`. Two things to check per repo rather than assume, since the prescription is prose and each repo owns its YAML. Confirm the workflow really does trigger on `main` alone before editing, because a repo that already names both is conformant and needs no PR. And leave the ruleset alone: the required check stays on `main`, so nothing is added to `repo-config/operational/develop.json`. The evidence that this is not hypothetical is `HomeAutomation-Config` PR #34, which merged into `develop` with an empty check list and a `CLEAN` mergeable state. Cross-repo work, so it needs the maintainer to name the repos. - Adopt the OCI annotation keys (`org.opencontainers.image.*`) for Docker image metadata across the Docker repos, replacing the ad-hoc and `org.label-schema.*` labels (from [#363][issue-363]). - Sweep `ManagePackageVersionsCentrally` placement to `Directory.Packages.props` fleet-wide, since PlexCleaner sets it in `Directory.Build.props`, off the CODESTYLE canonical. - Consider renaming this repo to reflect the audit-catalog identity, which updates badge and link URLs across the fleet. @@ -108,6 +109,7 @@ Everything here changes a file the fleet holds a copy of, so each entry costs a [issue-578]: https://github.com/ptr727/ProjectTemplate/issues/578 [issue-579]: https://github.com/ptr727/ProjectTemplate/issues/579 [issue-580]: https://github.com/ptr727/ProjectTemplate/issues/580 +[issue-585]: https://github.com/ptr727/ProjectTemplate/issues/585 diff --git a/WORKFLOW.md b/WORKFLOW.md index 4e9b810e..b44b4516 100644 --- a/WORKFLOW.md +++ b/WORKFLOW.md @@ -58,9 +58,12 @@ flowchart LR ```mermaid flowchart LR edit[direct signed commit] -->|advisory CI| develop + pr[pull request] -->|lint CI, reported not required| develop develop -->|merge commit, enforced lint CI| main ``` +The direct commit is an **allowance, not a substitute for review**. The ruleset drops the pull-request *requirement*, which permits a direct push without withdrawing the pull request, so a change worth reviewing still takes one and both paths reach `develop` legally. What differs is when validation lands. On the direct-commit path the commit is already on the branch, so CI can only be advisory after the fact, and that is the accepted cost of the model. On the pull-request path the change has not landed, so validation is pre-merge and actionable, which is the moment it is worth the most, and the lint workflow's `pull_request` trigger therefore names `develop` alongside `main` (Section 6). That is what makes **D1.2** hold here, since its input is *any* PR and the operational model is no exception. The check is reported on a `develop` PR rather than required, because a required status check on `develop` binds the direct push too and would dissolve the allowance the model is built on. + Their CI is lint/validation only (editorconfig/EOL plus domain linters such as Home Assistant or ESPHome config validation or a firmware build, but **no unit tests**), so the D-guarantees below that assume a build/test pipeline are **N/A** exactly as for `source-only` (Section 6). What binds: the promotion gate, where the `develop -> main` PR must pass the required `Check pull request workflow status job`, and the source-only release on manual dispatch (`releaseTrigger: dispatch-only`; tag + source zip). Branch-model rulesets are specified in [GOVERNANCE.md "Branching Model"][governance-branching-model] and [repo-config/README.md][repo-config-readme], not here. ### Two Layers: Orchestration vs Build @@ -285,7 +288,7 @@ Each type maps the *applicable* S-scenarios onto its targets. The differences ar - **Data / asset library.** A single new leaf: validate -> zip -> upload `release-asset--library` (`retention-days: 1`, upload gated `!smoke`, mirroring the nugetlibrary leaf's shape). Because no such leaf ships, you **add a target** (D6.4): a new `enable_library` input + `build-library` job + `github-release` `needs:` entry in the release task, and a `library` paths-filter entry + `changes` output + `smoke-build` enable-forward in the PR workflow (without it, D1.1 never smoke-builds the library). Keep `expect_release_assets: true` (it has a file target, unlike Docker). The .NET `unit-test` job is replaced by a type-appropriate validator with the aggregator **and** `smoke-build` both re-pointed to it (D1.2/D1.5). `version.json` + the NBGV `get-version` step are retained (they own the tag). Test: S1 smoke runs validate+zip and uploads nothing; S7 attaches the zip, prerelease on the non-default leg; S9 on a *scheduled* re-run release-create + asset-delete skip (the existing zip is untouched, no registry push), while a `workflow_dispatch` re-run **refreshes** the release and re-runs the asset-delete (the asset is re-uploaded then re-deleted). N/A: the nuget/pypi/docker/executable 5A addenda and their scenario clauses. - **Source-only / no build.** There is no `build-release-task.yml` (its `appliesTo` excludes source-only) and no package/image leaf, so nothing is edited down. The release is a standalone dispatch-only `publish-release.yml` that inlines NBGV for the tag and `action-gh-release` for the release: tag + source zip + README + LICENSE, with no reusable release task and no asset download. With no target the paths-filter matches nothing, so `smoke-build` is **structurally always skipped**, and validation is carried solely by the (replaced, non-.NET) validation job that the aggregator and `smoke-build`'s own `needs:` must both point at (D1.2; or drop the never-running `smoke-build` job). NBGV and `version.json` are still retained (they own the tag). Its publish job gates on the repo's reusable validation task (`needs:` the same `workflow_call` job the PR workflow runs), so a dispatch cannot release a ref that fails validation. Applicable scenarios: S1 (validation only), S5/S6 (publish gating), S7 (tag-only release), S8 (dispatch guard), S9 (no-op republish), S10 (classification gate). N/A: S2-S4 (assume a smoke-built target), the artifact-lifecycle and registry clauses of S7/S9, the D5/D6 artifact items, and all per-type 5A addenda, all recorded N/A, not failed. - **Static site deployed to a host the project owns.** Two independent surfaces, and keeping them apart is the point. The **release** is the source-only shape above, unchanged: a dispatch-only `publish-release.yml` where NBGV and `version.json` own the tag, producing tag + source zip + README + LICENSE. The **deploy** is its own `workflow_dispatch` carrying an `environment` choice input, so redeploying an unchanged commit mints no tag, which matters because redeploying is routine. It runs a ref gate **first**, before anything is installed or written (production from the default branch only; any ref may reach a non-production environment, since proving a branch before it merges is what that environment is for), then the **same** reusable validation task the PR gate runs, so a dispatch cannot deploy a ref that fails validation, then calls `deploy-site-task.yml` with `secrets: inherit`. Concurrency is keyed on the environment with `cancel-in-progress: false`, because a cancelled deploy leaves a release uploaded and unflipped. The task pins the generator by version and checksum, and re-asserts the environment name in a job of its own, because the `environment:` binding resolves before any step runs and a `workflow_call` caller is not bound by the dispatch choice list a human sees. Its environment-bound job then: checks out full history (a shallow clone silently changes page metadata), installs the verified generator, derives the release id **once** and exports it (deriving it twice yields ids seconds apart, and the live check then asserts a version nothing installed), builds the tree, installs the deploy credential from the environment, uploads into a per-release directory hard-linked against the current release and carrying **no** delete flag (at an environment root a delete removes the rollback targets), flips the pointer as a separate atomic step so a failed transfer cannot half-publish, and finally checks the running host (D4.6). Retention (D5.6) is bounded by a declared count with one side recorded as owning it: a deploy whose credential can observe the destination prunes and asserts the count here, while a credential confined **write-only** can neither delete nor read back, so there the prune is a host-side timer and the repo's runbook records that ownership. Widening the credential to bring the prune in-pipeline would trade a real confinement boundary for a check, and is the wrong trade; what the guarantee rejects is neither side owning it. One thing the pipeline cannot assert and the server config must: a non-public environment serving a byte-identical copy must not be indexed, and that default belongs on the side that is harmless in production, since a non-public container missing the value is still behind its gate while a production container inheriting it deindexes the site silently. Applicable scenarios: S1 (validation), the source-only release set S7/S8/S9/S10, and S12/S13 (the deploy dispatch). N/A: S2-S4, every registry scenario, and D5.1-D5.4 (the pipeline uploads no workflow artifact at all, so D5.6 is what applies in their place), all recorded N/A, not failed. -- **Operational (workflow model, not a build target).** A `workflowModel: operational` repo layers a direct-commit `develop` onto the **source-only** release shape (above). Two workflows: (1) a **lint/validation** PR workflow feeding the required `Check pull request workflow status job`, built from the generic linters (editorconfig/EOL, markdownlint, cspell, actionlint) plus a domain validator (Home Assistant `hass --script check_config`, `esphome config`, a firmware build), with **no unit tests**; its triggers differ from the `release` model: `push` to `develop` (advisory feedback on direct commits) plus `pull_request` to `main` (the enforced promotion gate) plus `workflow_dispatch`. (2) the standard **source-only publisher** on `workflow_dispatch` only (`releaseTrigger: dispatch-only`): NBGV + `version.json` own the tag, and a manual dispatch cuts a GitHub release (tag + source zip + README + LICENSE, via the standalone publisher's inlined `action-gh-release`). Applicable scenarios: S1 (validation) on the promotion PR, plus the source-only release set: S7 (tag-only release), S8 (dispatch guard), S9 (no-op republish), S10 (classification). N/A: the auto-publish paths (S5/S6 bot-push and schedule, neither of which an operational repo has) and every build/registry scenario. See the branch-model note in Section 3 and [GOVERNANCE.md "Branching Model"][governance-branching-model]. +- **Operational (workflow model, not a build target).** A `workflowModel: operational` repo layers a direct-commit `develop` onto the **source-only** release shape (above). Two workflows: (1) a **lint/validation** PR workflow feeding the required `Check pull request workflow status job`, built from the generic linters (editorconfig/EOL, markdownlint, cspell, actionlint) plus a domain validator (Home Assistant `hass --script check_config`, `esphome config`, a firmware build), with **no unit tests**; its triggers differ from the `release` model: `push` to `develop` (advisory feedback on the direct-commit path) plus `pull_request` to `branches: [ main, develop ]` (enforced on `main` as the promotion gate, reported but not required on `develop`) plus `workflow_dispatch`. (2) the standard **source-only publisher** on `workflow_dispatch` only (`releaseTrigger: dispatch-only`): NBGV + `version.json` own the tag, and a manual dispatch cuts a GitHub release (tag + source zip + README + LICENSE, via the standalone publisher's inlined `action-gh-release`). **The PR trigger names both branches, and naming `main` alone is a defect.** A set that omits `develop` matches nothing when a PR opens against `develop`, so the validation job never starts, the aggregator never reports, and the PR shows a clean mergeable state with an empty check list, which is the exact output D1.2 forbids. Naming both costs a duplicate run, since a PR-merged change validates once on the PR and again on the push when the merge lands, whatever merge method the repo allows (the operational `develop` ruleset prescribes none). The concurrency group is keyed on the workflow name plus `${{ github.ref }}` (Section 2), and a `pull_request` run's ref is `refs/pull//merge` where the push run's is `refs/heads/develop`, so the two occupy different groups and neither cancels the other. Pay it. On a lint-only gate that is a couple of runner-minutes, where the alternative, a condition that suppresses the push run, has to tell a merge commit from a direct commit and reintroduces exactly the reasoning the trigger set removes. Applicable scenarios: S1 (validation) on every PR, the promotion PR and a PR into `develop` alike, plus the source-only release set: S7 (tag-only release), S8 (dispatch guard), S9 (no-op republish), S10 (classification). N/A: the auto-publish paths (S5/S6 bot-push and schedule, neither of which an operational repo has) and every build/registry scenario. See the branch-model note in Section 3 and [GOVERNANCE.md "Branching Model"][governance-branching-model]. diff --git a/repo-config/README.md b/repo-config/README.md index 71fed176..1826f060 100644 --- a/repo-config/README.md +++ b/repo-config/README.md @@ -11,7 +11,7 @@ Repository and branch configuration held as committed files, kept out of `.githu Two workflow models share `main.json` but differ on `develop` (registry `workflowModel`, default `release`): - **`release`** (`develop.json`): `develop` requires squash merges with linear history and a PR, the feature-branch pipeline. -- **`operational`** (`operational/develop.json`): `develop` takes **direct signed pushes**, carrying only `deletion`, `non_fast_forward`, and `required_signatures`; no PR, no status-check, no Copilot-on-push. CI runs on the push as advisory feedback. This is for live-service config repos that edit `develop` directly and promote a known-good snapshot to `main` via an occasional PR (see [GOVERNANCE.md "Branching Model"][governance-branching-model]). +- **`operational`** (`operational/develop.json`): `develop` takes **direct signed pushes**, carrying only `deletion`, `non_fast_forward`, and `required_signatures`; no PR, no status-check, no Copilot-on-push. CI runs on the push as advisory feedback. Read the dropped rules as an allowance rather than a prohibition, since a PR into `develop` remains legal and the lint workflow triggers on it, with its result reported and not required (a required check here would gate the direct push as well). This is for live-service config repos that edit `develop` directly and promote a known-good snapshot to `main` via an occasional PR (see [GOVERNANCE.md "Branching Model"][governance-branching-model]). `main` (both models) requires merge-commit merges (no linear-history rule), signed commits, a passing `Check pull request workflow status job`, resolved review threads, and Copilot review, and blocks force-pushes and deletion, so a `develop -> main` promotion is always gated even when `develop` takes direct commits. Every ruleset intentionally leaves "Require branches to be up to date before merging" **off**, per [GOVERNANCE.md "Branching Model"][governance-branching-model]. diff --git a/spec/project-types.json b/spec/project-types.json index 652e4aaa..6afac0c0 100644 --- a/spec/project-types.json +++ b/spec/project-types.json @@ -143,7 +143,8 @@ { "id": "branch.both-protected", "verdict": "letter", "assert": "main and develop both exist and are protected.", "intentRef": "GOVERNANCE.md#branching-model" }, { "id": "branch.ruleset.develop", "verdict": "letter", "assert": "The live develop ruleset matches the expected payload for the repo's workflowModel (normalized diff): release -> repo-config/develop.json (PR-gated), operational -> repo-config/operational/develop.json (direct signed pushes; deletion + non_fast_forward + required_signatures only).", "intentRef": "repo-config/README.md" }, { "id": "branch.ruleset.main", "verdict": "letter", "assert": "The live main ruleset matches repo-config/main.json (normalized diff); this ruleset is shared by both workflow models.", "intentRef": "repo-config/README.md" }, - { "id": "branch.operational.lintci", "verdict": "intent", "assert": "An operational (workflowModel) repo runs a lint/validation CI (editorconfig/EOL plus domain linters, e.g. Home Assistant or ESPHome config validation or a firmware build; no unit testing) feeding the required Check pull request workflow status job, so the develop -> main promotion PR is gated even though develop takes direct commits. N/A for release repos.", "intentRef": "GOVERNANCE.md#branching-model" } + { "id": "branch.operational.lintci", "verdict": "intent", "assert": "An operational (workflowModel) repo runs a lint/validation CI (editorconfig/EOL plus domain linters, e.g. Home Assistant or ESPHome config validation or a firmware build; no unit testing) feeding the required Check pull request workflow status job, so the develop -> main promotion PR is gated even though develop takes direct commits. N/A for release repos.", "intentRef": "GOVERNANCE.md#branching-model" }, + { "id": "branch.operational.prtriggers", "verdict": "letter", "assert": "The operational repo's lint/validation workflow triggers on pull_request with branches: [ main, develop ], plus push to develop and workflow_dispatch. A pull_request set naming main alone leaves a PR into develop matching no trigger, so it merges with no validation at all (WORKFLOW.md D1.2). The develop result is reported, not required: the required status check stays on main only, since requiring it on develop would gate the direct push the model allows. N/A for release repos.", "workflowRef": "WORKFLOW.md#6-per-project-type-test-walkthroughs" } ] }, "repo-setup": { From 4e28ddb6b830d4d749d8f081adca4c18a326eed9 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Thu, 6 Aug 2026 08:17:55 -0700 Subject: [PATCH 4/5] Host the fleet tooling in the hub instead of carrying copies (#588) GOVERNANCE.md "PR Review Etiquette" told a downstream reader to run scripts/pr_review.py, and scripts/ appears in no spec/files.json baseline entry, so the path is absent from every repository that carries the rule. The cost is measured rather than anticipated: several onboarding sessions built their own worse versions of pr_review.py and prose_lint.py and had to be redirected by hand. A pointer that resolves nowhere teaches the reader that a pointer in carried text is decorative. #580 offered two resolutions, carrying the script or dropping its name, and named a third from TODO.md. Take the third. Add GOVERNANCE.md "Hub-Hosted Tooling" as a verbatim carried section settling the three questions that held the model open. A repository carries what it is audited against and what describes it, and reaches machinery holding nothing per-repo. A tool is named by its path in a hub checkout read at main and fetched immediately before use, its target is named explicitly rather than defaulted, and the working directory stays the tool's own contract. CI reaches the same rules as a composite action pinned to a commit SHA, which is the action-pinning rule unchanged and which .github/actions/prose-gate already implements. An unreachable hub means the tool did not run, reported as not run rather than answered with a reconstruction. The rule that blocked this was "Carried files carry no coordination references", which conflated where a file came from with what its reader runs. Give it a second exception for a hub-hosted tool the reader executes. The audit's template-reference check needs no widening, since the new section defers to AGENTS.md "Fleet Bootstrap" for the destination and GOVERNANCE.md still holds no occurrence of the hub name. De-vendor repo-config/configure.sh, the fleet's only carried script, which holds nothing per-repo and whose last hub fix immediately became a re-vendor owed by every repository carrying it. It resolves every payload path against its own directory, so a run from a hub checkout compares a repo against the hub's payloads and never opens the carried copies. Those copies stay, being what a repo is audited against and what its own AUDIT.md diffs the live rulesets against. A downstream copy is now retired rather than converged, which the divergence ledger says with a new retire disposition. Require the workflow model as an argument wherever the script is invoked, since a run from a hub checkout finds the registry and resolves an unregistered repo through defaults.workflowModel to release, which would apply the release develop ruleset to an operational repo. Repoint the two CODESTYLE.md definitions into hub-only catalog/, dead in every downstream copy since catalog/ became hub-only, and separate the two files they conflated: a repo carries the .NET task definitions in its own .vscode/tasks.json, and the hub snippet is the canonical it is written against. Refs #580 Refs #559 Co-Authored-By: Claude Opus 5 (1M context) --- AGENTS.md | 1 + CODESTYLE.md | 12 ++++++------ GOVERNANCE.md | 22 ++++++++++++++++++---- OPERATIONS.md | 12 ++++++------ STANDUP.md | 2 +- TODO.md | 11 ++++++----- docs/repo-config-carry.md | 10 +++++----- repo-config/README.md | 3 ++- scripts/README.md | 7 ++++--- spec/divergences.json | 6 +++--- spec/divergences.schema.json | 4 ++-- spec/fidelity_honesty.py | 2 +- spec/files.json | 3 +-- spec/section-model.md | 1 + spec/validate.py | 2 +- 15 files changed, 58 insertions(+), 40 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index a192ae47..0b6459c1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -75,6 +75,7 @@ Every rule below is a level-two section of [`GOVERNANCE.md`](./GOVERNANCE.md). R | Releasing, version bumps, publishing | `Release Model` | | A live config repo rather than a code repo | `Operational Repositories` | | Onboarding a repo or running a conformance sweep | `Repository Onboarding and Conformance` (hub only, not carried) | +| Running a fleet gate, the review digest, or the config script | `Hub-Hosted Tooling` | | Writing a commit message or pull request title | `Pull Request Title and Commit Message Conventions` | | Any prose, comment, doc, or line-ending change | `Documentation Style Conventions` | | Proving work actually happened | `Verification Discipline` | diff --git a/CODESTYLE.md b/CODESTYLE.md index 2414a1ea..892d579d 100644 --- a/CODESTYLE.md +++ b/CODESTYLE.md @@ -52,8 +52,8 @@ This is the style guide for any **.NET projects** in this repo. **CRITICAL**: All builds must complete without warnings. The project enforces this through: 1. **The `.NET Format` clean-compile task** (see [Clean-Compile Verification][clean-compile-verification]) - - The .NET clean-compile is the **`.NET Format`** VS Code task, which chains `CSharpier Format` -> `.NET Build` -> `dotnet format style --verify-no-changes`. These three task definitions are carried verbatim in [`.vscode/tasks.json`][vscode-tasks]. - - After any code change it must pass before commit. Run the `.NET Format` task. To run it natively instead, reproduce that task chain from [`.vscode/tasks.json`][vscode-tasks] exactly (`CSharpier Format`, then `.NET Build`, then the `dotnet format style --verify-no-changes --severity=info ...` verify) without dropping or loosening any argument (tasks.json is the canonical command spec). Bare `dotnet format` alone, skipping CSharpier or the build, is not sufficient. + - The .NET clean-compile is the **`.NET Format`** VS Code task, which chains `CSharpier Format` -> `.NET Build` -> `dotnet format style --verify-no-changes`. A repo carries those three definitions in its own `.vscode/tasks.json`, matching the canonical in [`vscode-tasks.json`][vscode-tasks-link]. + - After any code change it must pass before commit. Run the `.NET Format` task. To run it natively instead, reproduce that task chain exactly (`CSharpier Format`, then `.NET Build`, then the `dotnet format style --verify-no-changes --severity=info ...` verify) without dropping or loosening any argument, reading it from [`vscode-tasks.json`][vscode-tasks-link], which is the canonical command spec a repo's own `tasks.json` is written against. Bare `dotnet format` alone, skipping CSharpier or the build, is not sufficient. 2. **Analyzer configuration** - `true` with `latest-all` and `All` (full analyzer set enabled) @@ -361,7 +361,7 @@ This is the style guide for any **Python project(s)** in this repo. - **Dependency declaration** - `[dependency-groups]`, or PEP 621 `[project.optional-dependencies]` (dev tools installed with `uv sync --extra `). - **Versioning / publishing** - a published package (`_version.py` + a version source + `uv build` + a PyPI publish step), or a **source-only** repo with a static `version` and no publish step (see [Versioning][versioning-section]). - **Disabled markdownlint rules** - repo-specific. `.markdownlint-cli2.jsonc` at the repo root is the source of truth, not any example rule named here. -- **VS Code config home** - editor **settings/extensions** may live in `.vscode/*.json` **or** the `.code-workspace`, while **tasks / launch / debug** configs can only be external `.vscode/*.json` (they cannot live in the workspace file). A `[vscode-tasks]` reference must point wherever the repo actually keeps `tasks.json`. +- **VS Code config home** - editor **settings/extensions** may live in `.vscode/*.json` **or** the `.code-workspace`, while **tasks / launch / debug** configs can only be external `.vscode/*.json` (they cannot live in the workspace file). The repo's own `tasks.json` sits wherever it keeps it, and the canonical task definitions it is written against are the hub snippet the [`vscode-tasks.json`][vscode-tasks-link] reference names, which resolves the same way from every repo. **Two profiles.** A repo's Python is one of two shapes, declared as the `build` or `lint-only` profile and validated against the `pyproject.toml` shape. The rest of this section (uv project, `uv.lock`, `uv run`, `src` layout, pytest coverage) describes the **Project** shape (the `build` profile). The two differ by whether the Python has **third-party runtime dependencies**, which shows up structurally in `pyproject.toml`, so the audit reads the shape there (`python.profile.detect`): @@ -398,7 +398,7 @@ uv run pytest # run tests uv build # produce wheel + sdist in ./dist (published packages only) ``` -The Python clean-compile (see [Clean-Compile Verification][clean-compile-verification]) is `uv run ruff format` + `uv run ruff check` + the repo's type checker: `uv run pyright`, or `uv run mypy src` where mypy is the CI checker, or both where the repo runs both (see Type checking above); run it (plus `uv run pytest`) before committing. These are documented commands; an optional VS Code tasks mirror (all `type: process`, no `&&` shell chaining, so it runs the same on any task shell) is in [`vscode-tasks-python.json`][vscode-tasks-python]. CI runs the same clean-compile commands as the authoritative backstop. Git hooks are opt-in; wire `pre-commit` for `ruff` and the type checker yourself if you want local enforcement. +The Python clean-compile (see [Clean-Compile Verification][clean-compile-verification]) is `uv run ruff format` + `uv run ruff check` + the repo's type checker: `uv run pyright`, or `uv run mypy src` where mypy is the CI checker, or both where the repo runs both (see Type checking above). Run it, plus `uv run pytest`, before committing. These are documented commands, and an optional VS Code tasks mirror (all `type: process`, no `&&` shell chaining, so it runs the same on any task shell) is in [`vscode-tasks-python.json`][vscode-tasks-python-link]. CI runs the same clean-compile commands as the authoritative backstop. Git hooks are opt-in, so wire `pre-commit` for `ruff` and the type checker yourself if you want local enforcement. ### Layout @@ -502,8 +502,6 @@ Before pushing or opening a PR: [readme]: ./README.md [root]: ./.editorconfig [versioning-section]: #versioning -[vscode-tasks]: ./catalog/snippets/configs/vscode-tasks.json -[vscode-tasks-python]: ./catalog/snippets/configs/vscode-tasks-python.json @@ -513,4 +511,6 @@ Before pushing or opening a PR: [pep-0257-link]: https://peps.python.org/pep-0257/ [pyright-link]: https://microsoft.github.io/pyright/ [ruff-link]: https://docs.astral.sh/ruff/ +[vscode-tasks-link]: https://github.com/ptr727/ProjectTemplate/blob/main/catalog/snippets/configs/vscode-tasks.json +[vscode-tasks-python-link]: https://github.com/ptr727/ProjectTemplate/blob/main/catalog/snippets/configs/vscode-tasks-python.json [uv-link]: https://docs.astral.sh/uv/ diff --git a/GOVERNANCE.md b/GOVERNANCE.md index 6f252857..e6d37680 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -116,6 +116,20 @@ Every fleet repo is a standard-style repo the hub audits **downward** against it - **The registry is ground truth about reality, not intent.** After any conformance change, reconcile the repo's `registry/repos.json` entry (`status`, `types`, `releaseTrigger`, `workflowModel`, `driftNotes`) to what the repo actually is. `spec/validate.py` proves the catalog is self-consistent. It does **not** prove the catalog matches the live repo. A `validate.py`-clean entry can still be false, and a stale entry is a finding. - **Prove the docs suffice.** The onboarding docs are sufficient only when a context-free agent stands each supported repo shape (project type(s) plus workflow model) up from them alone. The cold-start self-test in `STANDUP.md` and the conformance matrix track that. A shape that cannot be stood up cold is a documentation defect, fixed in the hub, not worked around per repo. +## Hub-Hosted Tooling + +The fleet's tooling lives in the hub once and a repository runs it from there rather than holding a copy. A carried script is current only until the next fix to it, and a repository that misses the sweep does not fail loudly, it audits itself with an older gate while reporting the same command in its output. Removing the copy removes the sweep, the stale-copy detection, and the disposition each stale copy earns, all at once. The hub is the repository [`AGENTS.md`](./AGENTS.md) "Fleet Bootstrap" names, and that section is the entry point whenever nothing else present says where it is. + +**What a repository carries and what it reaches is decided by what the content is.** It carries the content it is audited against and the configuration that describes it, meaning its rule text, the files the manifest declares, and the ruleset payloads its own audit diffs the live configuration against. It reaches machinery whose content is identical in every repository, meaning the prose and repository gates, the review digest, and the configuration script, because a file holding no per-repo content is a copy whose only future is to go stale. A tool named in a carried rule is therefore named as the hub's, since the alternative is a pointer to a path the reader does not have, and a pointer that resolves nowhere teaches the reader that a pointer in carried text is decorative. + +**Reaching it is a checkout of the hub rather than a copy of one file.** A tool reads the payloads, tables, and sibling modules beside it, so a single file lifted out of the tree runs against whatever the caller happens to have, which is the copy problem again in a shorter loop. Read `main`, the promoted and gated state, and fetch immediately before running, because a clone is whatever it last fetched rather than the branch it names, and a stale clone answers confidently instead of failing. Name the tool by its path in that checkout and name the target explicitly, since a tool that defaults to the current directory or the current repository resolves somewhere either way, and a result computed against the wrong repository is well-formed. Which directory the command runs in is the tool's own contract rather than a rule here, so a gate reading a working tree runs in the repository under test while a tool taking its target as an argument runs anywhere and is given one. What the rule forbids is letting a default decide which repository the answer is about. + +**A report or finding a hub tool produces names the hub commit it ran from.** The tool moves independently of the repository it measures, so a verdict carrying no hub commit cannot be re-run, and two runs that disagree cannot be attributed to the tree or to the tool. The obligation is the runner's rather than the tool's, since a tool reports on the repository it measures rather than on itself, so the commit is read from the hub checkout and written into the report beside the verdict. This is the same requirement "Verification Discipline" places on any claim that gets acted on. + +**CI reaches the same tooling as a pinned action.** A runner holds no hub checkout, so a workflow consumes the hub's composite action and pins it to a commit SHA, per the action-pinning rule under "Workflow YAML Conventions". The pin is what makes a released repository's gate reproducible, since an unpinned consume lets a later hub commit fail a re-run of a change that already passed. Branch-dependent behavior belongs inside the consumed action, because `uses:` takes no expressions and a per-branch ref therefore cannot be selected in the workflow file. + +**An unreachable hub means the tool did not run, and that is the result reported.** A carried copy still works offline and a reached one does not, which is the cost this model trades away and the reason to state the failure rather than route around it. A check that cannot run reports itself as not run, never as clean, which is the silent-narrowing failure "Verification Discipline" names. A hand-rolled substitute is not the tool either: a reconstructed gate encodes its author's reading of the rule rather than the rule, agrees with no other repository, and is the duplicated effort this model exists to end, so an agent that cannot reach the hub says so and stops. + ## Pull Request Title and Commit Message Conventions ### Format @@ -142,7 +156,7 @@ Clarify devcontainer setup steps in README ## Documentation Style Conventions -- **Carried files carry no coordination references.** In the files the fleet carries (`AGENTS.md`, `GOVERNANCE.md`, `CODESTYLE.md`, `WORKFLOW.md`, `.github/copilot-instructions.md`, the `repo-config/` and `spec/` files, the carried `AUDIT.md`), two things are banned. **Any reference to the template repo**, in prose or in a link: the coordination flow is machinery a consumer of that repo should never have to see, and naming where a file came from is the derived-from framing the present-tense rule below independently forbids. Where a carried file must express a template-level behavior ("report a rule discrepancy upstream"), state the behavior rather than the destination. **The one exception is a verbatim section, and `AGENTS.md` "Fleet Bootstrap" is why it exists.** That section's whole function is to name where the canonical rules live, for an agent in a repository whose carried copies are stale, partial, or absent, which is exactly when no other file present can say it. Its bytes are fixed fleet-wide, so a repository cannot edit the reference out without failing the verbatim check instead, and a rule banning it would be unsatisfiable rather than merely strict. The exception is scoped to the verbatim region and never leaks past it: the same document's own prose is governed normally, so a repository that names the destination in a paragraph it owns is still in breach. A reference that reaches a verbatim section is a defect in the canonical, fixed once at the source rather than reported against every repository carrying it. The maintainer supplies the destination out of band. And **a sibling fleet repo named as an illustrative example** ("repo X does it this way", "see repo Y's adoption"), which couples the repos and rots as they diverge. To point at a current good example, name it in the onboarding or conformance issue, never in a carried doc. **A contextually relevant link to a related project is not a coordination reference, and is expected.** Where another repo is part of this repo's subject matter (the image that consumes this config, the builder that generates this hardware, a library this depends on), link it normally. The test is whether the link serves a reader of *this* repo's content, not whether the target happens to be in the fleet. This rule governs carried template content. A repo's own `README.md` and topical docs are its own content, not carried verbatim, and it does not reach them. This pairs with the present-tense rule below: state the current shape, not a history of which repo it came from. +- **Carried files carry no coordination references.** In the files the fleet carries (`AGENTS.md`, `GOVERNANCE.md`, `CODESTYLE.md`, `WORKFLOW.md`, `.github/copilot-instructions.md`, the `repo-config/` and `spec/` files, the carried `AUDIT.md`), two things are banned. **Any reference to the template repo**, in prose or in a link: the coordination flow is machinery a consumer of that repo should never have to see, and naming where a file came from is the derived-from framing the present-tense rule below independently forbids. Where a carried file must express a template-level behavior ("report a rule discrepancy upstream"), state the behavior rather than the destination. **The first exception is a verbatim section, and `AGENTS.md` "Fleet Bootstrap" is why it exists.** That section's whole function is to name where the canonical rules live, for an agent in a repository whose carried copies are stale, partial, or absent, which is exactly when no other file present can say it. Its bytes are fixed fleet-wide, so a repository cannot edit the reference out without failing the verbatim check instead, and a rule banning it would be unsatisfiable rather than merely strict. The exception is scoped to the verbatim region and never leaks past it: the same document's own prose is governed normally, so a repository that names the destination in a paragraph it owns is still in breach. A reference that reaches a verbatim section is a defect in the canonical, fixed once at the source rather than reported against every repository carrying it. The maintainer supplies the destination out of band. **The second exception is a hub-hosted tool the reader is told to run, which is a different kind of reference.** A rule naming a gate, a script, or a reference snippet the reader executes or copies states an instruction rather than a provenance, and an instruction with no destination is unfollowable, which is precisely how a pointer in carried text comes to read as decorative. The test is whether the reference is something the reader does or something that happened to this file, so where the content came from stays out and what the reader runs stays in. Such a pointer names the hub's canonical rather than this repository's provenance, so it is the hub's to keep resolving and never a repository's to edit out or to re-point at a local path, which is what left the reference resolving differently in every copy before. In `AGENTS.md` and `GOVERNANCE.md` it belongs in verbatim rule text, the same region the first exception already covers, so the whole fleet reads one wording and no repository is asked to answer for a reference it did not write. What is reached rather than carried, and how, is in [Hub-Hosted Tooling](#hub-hosted-tooling). And **a sibling fleet repo named as an illustrative example** ("repo X does it this way", "see repo Y's adoption"), which couples the repos and rots as they diverge. To point at a current good example, name it in the onboarding or conformance issue, never in a carried doc. **A contextually relevant link to a related project is not a coordination reference, and is expected.** Where another repo is part of this repo's subject matter (the image that consumes this config, the builder that generates this hardware, a library this depends on), link it normally. The test is whether the link serves a reader of *this* repo's content, not whether the target happens to be in the fleet. This rule governs carried template content. A repo's own `README.md` and topical docs are its own content, not carried verbatim, and it does not reach them. This pairs with the present-tense rule below: state the current shape, not a history of which repo it came from. ### Markdown @@ -299,7 +313,7 @@ After the final push on a PR, sweep older threads from earlier rounds whose code **Answering a suppressed finding is a different act from replying in a thread, and it carries its own pairing.** A threaded reply sits under the comment it answers and the UI shows whether it is resolved. A suppressed finding has neither, so an answer that does not carry its own context is unverifiable: the maintainer cannot tell that it was seen, which finding it addresses, or whether any were skipped, and has to ask. An answer therefore **quotes the finding** in a blockquote, with its `file:line` anchor and enough of Copilot's own words to identify it, **carries one bold verdict per finding** (`Fixed in `, `Disproven`, or `No change needed`) so the outcomes are scannable without reading prose, **states the `(N)` count** the block heading gives so N answers can be checked against N findings, and **links the review** that raised them, since a PR accumulates rounds and an unlinked answer is ambiguous about which one it closes. One comment per review round keeps the answers together. -**Read every round, not only the head.** A suppressed finding has no resolved state, so a push does not retire it: the finding simply stops appearing in a head-scoped query while remaining unanswered. Treating "superseded by a push" as "answered" is how rounds of findings go unanswered. `scripts/pr_review.py status --repo /` reports every round and marks which are from earlier ones, and it names the repository because a pull request number resolves in every repository and a digest of the wrong one is well-formed. +**Read every round, not only the head.** A suppressed finding has no resolved state, so a push does not retire it: the finding simply stops appearing in a head-scoped query while remaining unanswered. Treating "superseded by a push" as "answered" is how rounds of findings go unanswered. The hub's `scripts/pr_review.py status --repo /` reports every round and marks which are from earlier ones, and it names the repository because a pull request number resolves in every repository and a digest of the wrong one is well-formed. That script is hub-hosted rather than carried, so no repository holds a copy to run and it is invoked from a hub checkout per "Hub-Hosted Tooling" above. **The review's own overview cannot be trusted to say whether findings exist.** A body that reads "Copilot reviewed N out of N changed files and generated no new comments" routinely carries a collapsed block of suppressed findings directly beneath that sentence. Read the body for the block rather than the summary line, because the summary line and `reviewDecision` and an empty unresolved-thread list all agree that a review with four outstanding findings is clean. @@ -412,9 +426,9 @@ Every repo's GitHub repository details (the About panel) follow a fixed conventi - [`GOVERNANCE.md`](./GOVERNANCE.md), [`CODESTYLE.md`](./CODESTYLE.md), [`WORKFLOW.md`](./WORKFLOW.md), [`AUDIT.md`](./AUDIT.md): the governance and audit docs. This file is the cross-cutting-rules authority. - [`spec/`](./spec/): the machine-readable ground truth, covering project-type requirements, the file/section baseline, secrets, and the preferred README structure. - [`registry/repos.json`](./registry/repos.json): the fleet registry, holding every project, its type(s), and its status. -- [`repo-config/`](./repo-config/): branch rulesets and the apply script, kept out of `.github/` (which is Actions-owned). +- [`repo-config/`](./repo-config/): branch rulesets, the fleet settings, and the apply script every repo is configured by, kept out of `.github/` (which is Actions-owned). The payloads carry to the fleet and the script is reached here. - [`catalog/`](./catalog/): reusable reference snippets (workflows, configs, devcontainers) the audit compares implementations against. -- [`scripts/`](./scripts/): local lint and review tooling this repo runs by hand, with the deterministic checks also gating CI, for the documented rules no CI linter checks. Hub-only, not carried. +- [`scripts/`](./scripts/): the fleet's lint and review tooling, with the deterministic checks also gating CI, for the documented rules no CI linter checks. Hosted here and reached rather than carried, per "Hub-Hosted Tooling". - [`docs/`](./docs/): hub-only procedure and measurement docs (host setup, config carry, signing, agent token cost), none of them carried downstream. - [`reports/`](./reports/): per-repo audit output. - [`.github/workflows/`](./.github/workflows/): this repo's own lint-only CI and source-only release, the workflow contract it dogfoods. diff --git a/OPERATIONS.md b/OPERATIONS.md index 84a98154..6e74419c 100644 --- a/OPERATIONS.md +++ b/OPERATIONS.md @@ -1,6 +1,6 @@ # Operations -How this repository is run. It ships no application code, so its operations are the fleet audit, the local gates that mirror CI, and the script that applies repository configuration. +How this repository is run. It ships no application code, so its operations are the fleet audit, the gates that mirror CI, and the script that applies repository configuration. Those gates and that script serve the whole fleet from this checkout rather than being carried into each repository, per [GOVERNANCE.md "Hub-Hosted Tooling"](./GOVERNANCE.md#hub-hosted-tooling), so every run below is a run from here against a repository named on the command line. ## Runbooks @@ -48,11 +48,11 @@ Findings are a point-in-time snapshot read live over the API. Re-run before acti ### Apply or verify repository configuration ```sh -repo-config/configure.sh check [owner/repo] [release|operational] -repo-config/configure.sh apply [owner/repo] [release|operational] +repo-config/configure.sh check owner/repo release|operational +repo-config/configure.sh apply owner/repo release|operational ``` -**Always pass the command.** A bare `repo-config/configure.sh` with no arguments defaults to `apply` against the current repo, so an invocation meant to test whether the script runs performs a live write instead. Never run it without a command. +**Always pass the command, the repository, and the model.** A bare `repo-config/configure.sh` with no arguments defaults to `apply` against the current repo, so an invocation meant to test whether the script runs performs a live write instead. Never run it without a command. The repository argument matters for the same reason now that the fleet runs this copy rather than its own: an omitted target resolves to this repository, and applying the fleet configuration to the hub while meaning to configure a downstream repo is a well-formed write to the wrong place. The model is the third argument for the same reason. This checkout has the registry beside the script, so a repo the registry does not yet name resolves through `defaults.workflowModel` to `release` rather than aborting, and an operational repo then takes the release `develop` ruleset. `check` is read-only and exits non-zero on drift. `apply` is idempotent and drives entirely from the committed payloads, so it is a no-op on a conformant repo. @@ -100,7 +100,7 @@ Two `gh` limitations on the current host, both worked around rather than fixed: - [spec/](./spec/) is the machine-readable ground truth, holding project types, the file and section baseline, and required or forbidden secrets. - [registry/repos.json](./registry/repos.json) is the fleet registry, naming every project with its types, publish mechanism, and status. -- [repo-config/](./repo-config/) holds the branch rulesets and the apply script. It sits outside `.github/`, which is Actions-owned. +- [repo-config/](./repo-config/) holds the branch rulesets, the fleet settings, and the apply script. The payloads carry to the fleet and the script is reached here. It sits outside `.github/`, which is Actions-owned. - [catalog/](./catalog/) holds reference snippets the audit compares implementations against. - [reports/](./reports/) holds per-repo audit output. -- [scripts/](./scripts/) holds the gates that run in CI and locally. +- [scripts/](./scripts/) holds the gates that run in CI and locally, and that every fleet repository reaches rather than carries. diff --git a/STANDUP.md b/STANDUP.md index 209c16ad..5e978e26 100644 --- a/STANDUP.md +++ b/STANDUP.md @@ -119,7 +119,7 @@ Three conditions fail here, and the two commands together are what separate them Each is step 0A's escalation rather than something to work around. -Run `repo-config/configure.sh apply [owner/repo] [release|operational]` (the repo defaults to the current one, the model to the registry lookup or, absent a registry, to the carried payload) to apply the fleet settings, the Dependabot security features, and the two rulesets idempotently (import the JSON, never hand-build it, per [`docs/repo-config-carry.md`][repo-config-carry]), then `repo-config/configure.sh check [owner/repo] [release|operational]` to validate the repo and exit non-zero on any drift. Configure every required secret per [`spec/secrets.json`][secrets] (the registry `requiredSecrets[]` list plus the implicit baseline) in the right store(s), meaning Actions plus Dependabot where the mechanism needs it, and confirm no forbidden secret is present. The required check binds by name (`Check pull request workflow status job`) and turns green only after the PR workflow has run once, which is why this step follows step 3 rather than preceding it. A ruleset requiring a name no run has ever reported leaves the first pull request waiting on a status nothing produces, and on an operational repo the `develop -> main` promotion is a pull request too, so the same wait applies there. +Run `repo-config/configure.sh apply owner/repo release|operational` from a hub checkout, naming the repo being stood up and its model, to apply the fleet settings, the Dependabot security features, and the two rulesets idempotently (import the JSON, never hand-build it, per [`docs/repo-config-carry.md`][repo-config-carry]), then `repo-config/configure.sh check owner/repo release|operational` to validate the repo and exit non-zero on any drift. The script is hub-hosted rather than carried, so the repo being stood up holds no copy of it and never needs one, and naming the target is what keeps the write off the checkout the command runs in. Pass the model explicitly here rather than relying on the lookup. Run from a hub checkout the registry is present, so a repo not yet registered resolves through `defaults.workflowModel` to `release` and applies the wrong `develop` ruleset to an operational repo, and a repo being stood up is exactly the one the registry has not got yet. Reconcile its registry entry in step 6 either way. Configure every required secret per [`spec/secrets.json`][secrets] (the registry `requiredSecrets[]` list plus the implicit baseline) in the right store(s), meaning Actions plus Dependabot where the mechanism needs it, and confirm no forbidden secret is present. The required check binds by name (`Check pull request workflow status job`) and turns green only after the PR workflow has run once, which is why this step follows step 3 rather than preceding it. A ruleset requiring a name no run has ever reported leaves the first pull request waiting on a status nothing produces, and on an operational repo the `develop -> main` promotion is a pull request too, so the same wait applies there. ## 5. Verify: Run the Audit diff --git a/TODO.md b/TODO.md index 97581231..c510d827 100644 --- a/TODO.md +++ b/TODO.md @@ -24,8 +24,8 @@ The gates in [`scripts/`][scripts] are hub-only and are the fleet's main defense - Teach the `sha-pin` check in [`repo_gate.py`][repo-gate] to verify a pin **resolves**, not merely that it is shaped like a SHA. Forty hex characters is a format any fabricated string satisfies, and an agent hand-writing a plausible SHA into a workflow is a real failure mode rather than a hypothetical one. A resolvability check also catches the neighboring case, a pin whose commit was reachable only from a branch that has since been squashed and deleted, which breaks a downstream gate long after the change that caused it. Scope the network call to same-owner repositories, where the fleet's own actions live, and skip rather than fail when the host is offline so the local gate stays usable. Note that the existing `gh-write-guard` hook cannot cover this, since it watches Bash and an editor tool writing the same string into a file never reaches it. - Add a check that a pull request's **description** does not contradict its own branch. Three stale descriptions in one session generated six review findings between them, each one a reviewer noticing that the body named a commit, a branch, or a behavior the branch no longer carried. The cheap and precise form is to extract SHAs and `uses:` refs quoted in the body and confirm each still appears in the head tree, since those are the claims that go stale silently and the ones a reviewer actually catches. Prose claims are out of scope, and deliberately so: judging those needs a similarity heuristic, which [`spec/section-model.md`][section-model] already rejects for exactly the reason it would fail here. - Reconsider whether the pre-commit hook should run the doc gates now that they are diff-scoped. [`scripts/README.md`][scripts] records the current decision and its reason, that doc linters stay out of the hook so it stays fast, and that reason was sound when the only mode was a whole-tree sweep. A `--diff` run reads the lines one commit touches and finishes in about a second, so the trade has moved. The failure it would prevent is the most repeated one on record: comment sentences wrapped across lines, caught by CI or by a reviewer after the commit rather than before it, over and over within a single session. Weigh it against the standing preference for a fast hook, and against the risk of a hook that runs the gate from the wrong directory, which is its own false clean. -- Audit the fleet's shell surface by size and branching, and decide per script whether Python with unit tests is the cheaper form. The evidence is the review record rather than a language preference: a non-trivial shell script earns findings round after round, while every gate under [`scripts/`][scripts] carries a `test_