Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions scripts/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@

Local checks and tooling this repo runs by hand, with the deterministic ones also gating CI. Each one exists because the CI linters pass on the failure it catches: `markdownlint`, `cspell`, `actionlint`, and `editorconfig-checker` all report clean on prose that breaks a documented [`GOVERNANCE.md`][governance] rule. Doc linters stay out of the pre-commit hook, which runs language formatting only so it stays fast.

**Hub-only, not carried.** These are not declared in [`spec/files.json`][files], so the audit does not expect a downstream repo to ship them - the same footing as `spec/audit.py`. Promoting one to fleet-carried is a deliberate act: declare it in the baseline and vendor it, per [`spec/section-model.md`][section-model].
**Hub-only, not carried.** These are not declared in [`spec/files.json`][files], so the audit does not expect a downstream repo to ship them, the same footing as `spec/audit.py`. Promoting one to fleet-carried is a deliberate act: declare it in the baseline and vendor it, per [`spec/section-model.md`][section-model].

Python only, standard library only, no third-party packages. Every script is read-only and exits non-zero on a finding.

Each script has a `test_<script>.py` beside it, driving its gates against input they must reject, because a gate nobody has watched fail is a gate nobody knows works. Where a case covers a table it reads the live table rather than restating it, and each one asserts a floor on what a healthy run reaches - a check whose scan matches nothing reports zero findings and reads exactly like a pass.
Each script has a `test_<script>.py` beside it, driving its gates against input they must reject, because a gate nobody has watched fail is a gate nobody knows works. Where a case covers a table it reads the live table rather than restating it, and each one asserts a floor on what a healthy run reaches, since a check whose scan matches nothing reports zero findings and reads exactly like a pass.

```sh
python3 scripts/test_prose_lint.py
Expand Down Expand Up @@ -48,9 +48,9 @@ The `comment-wrap` rule covers comments in every syntax the fleet's project type

JSON is treated as JSONC, because that is what ships: VS Code tasks, launch, devcontainer and workspace files all carry comments under a plain `.json` name. A marker inside a string literal is not a comment, so each line is scanned with quoted spans blanked first, and Python uses `tokenize` so a trailing comment is seen exactly. A documentation comment (`///`, `/**`, a docstring) is left to CODESTYLE, which permits the paragraphs this rule forbids.

Each syntax also declares how its strings escape: the escape character, the quotes it works inside, and whether it works outside one, all read independently of whether a string embeds its delimiter by doubling it. Neither property implies the other - PowerShell's double-quoted string is escaped by a backtick and doubling at once, while a C# verbatim string is doubling and not escaped. Reading an escape a string does not have consumes its closing quote and blanks the rest of the line, and missing one it does have ends the string early on the escaped quote.
Each syntax also declares how its strings escape: the escape character, the quotes it works inside, and whether it works outside one, all read independently of whether a string embeds its delimiter by doubling it. Neither property implies the other: PowerShell's double-quoted string is escaped by a backtick and doubling at once, while a C# verbatim string is doubling and not escaped. Reading an escape a string does not have consumes its closing quote and blanks the rest of the line, and missing one it does have ends the string early on the escaped quote.

A string that spans lines carries its state onto the lines it covers, so a marker inside one is data rather than a comment. Each syntax declares the forms it carries: the C# verbatim string, an ordinary quoted string in shell and in PowerShell, a PowerShell here-string, a shell heredoc, and a YAML block scalar. A YAML `run:` scalar is deliberately not one of them, because it holds a script whose `#` lines are exactly the comments this rule governs. A form no syntax declares stays scanned a line at a time, which is where a false positive is still possible: a TOML triple-quoted string is the open case. The reverse direction is guarded too, since a form that carried where the language has none would blank the rest of the file and report nothing - a YAML plain scalar's apostrophe is not a string, so an ordinary quote does not carry there.
A string that spans lines carries its state onto the lines it covers, so a marker inside one is data rather than a comment. Each syntax declares the forms it carries: the C# verbatim string, an ordinary quoted string in shell and in PowerShell, a PowerShell here-string, a shell heredoc, and a YAML block scalar. A YAML `run:` scalar is deliberately not one of them, because it holds a script whose `#` lines are exactly the comments this rule governs. A form no syntax declares stays scanned a line at a time, which is where a false positive is still possible: a TOML triple-quoted string is the open case. The reverse direction is guarded too, since a form that carried where the language has none would blank the rest of the file and report nothing: a YAML plain scalar's apostrophe is not a string, so an ordinary quote does not carry there.

A comment sentence also has to start with a capital, which `comment-case` checks. A lowercase opening reads as the continuation of the line above it, so the two rules are read together: a wrapped sentence reports as `comment-wrap`, and a lowercase opening that is not a continuation reports as `comment-case`. Where the first word is a tool whose own casing is lowercase, the fix is to restructure rather than to capitalize the name against CODESTYLE's tooling-casing rule.

Expand All @@ -60,8 +60,8 @@ A comment sentence also has to start with a capital, which `comment-case` checks

Two deterministic checks:

- `sha-pin` - every workflow `uses:` is a 40-hex commit SHA, with the one documented `dotnet/nbgv@master` exception allowed.
- `eol` - every path pinned LF in [`.gitattributes`][gitattributes] has the matching [`.editorconfig`][editorconfig] override the line-ending rule requires, with EditorConfig brace syntax expanded. One direction only: an `.editorconfig` LF glob with no git pin is legitimate, since `.editorconfig` governs what the editor writes where git enforces a class it must not guess at.
- `sha-pin`: every workflow `uses:` is a 40-hex commit SHA, with the one documented `dotnet/nbgv@master` exception allowed.
- `eol`: every path pinned LF in [`.gitattributes`][gitattributes] has the matching [`.editorconfig`][editorconfig] override the line-ending rule requires, with EditorConfig brace syntax expanded. One direction only: an `.editorconfig` LF glob with no git pin is legitimate, since `.editorconfig` governs what the editor writes where git enforces a class it must not guess at.

```sh
python3 scripts/repo_gate.py
Expand All @@ -81,6 +81,10 @@ python3 scripts/pr_review.py wait 452 --timeout 2700

`wait` exits `30` when the review is still pending at the timeout, which is pending rather than failed. Its failure mode is a wrong answer rather than a crash, so the cases feed crafted GraphQL payloads: a review attributed to the wrong login, a review counted against a stale head, a maintainer's own thread read as a finding, and a wait that returns success while nothing landed. One case reads the reviewer login out of the runbook rather than restating it, since GraphQL drops the `[bot]` suffix REST carries, and another asserts no mutation has crept into a read-only script.

The digest also reports the **suppressed findings** a review body collapses into a `<details>` 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 the reviews on the current head only, so a finding answered before a push does not re-open after it. 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.

The match is on the block's heading rather than anywhere in the body, and on the runbook's alternation rather than on one phrasing, since the wording has already appeared two ways. A case asserts the script's pattern is the one the runbook publishes rather than a copy of it that can drift. Reading the whole body was the first implementation and its own review caught it: a review whose overview prose discusses suppressed findings carries none, and reporting that as a finding trains the reader to skim the field. A heading outside any `<details>` wrapper is still read, because reporting zero when the markup moves is the same false clean one level up, and that fallback takes a count so ordinary prose does not become one.

<!-- Internal -->

[copilot-instructions]: ../.github/copilot-instructions.md
Expand Down
57 changes: 54 additions & 3 deletions scripts/pr_review.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
Discipline" for the rule this implements.

Subcommands
status One digest line plus any unresolved threads. Read-only.
status One digest line, any unresolved threads, and any suppressed findings. Read-only.
wait Poll until Copilot's review lands on the current head, then print the digest.
The loop runs in-process, so a 45-minute wait costs one agent turn, not 90.
Exit 0 = review present, 30 = still pending at timeout (pending is not failure).
Expand All @@ -19,10 +19,20 @@
.github/copilot-instructions.md for the mutation runbook.
"""
from __future__ import annotations
import argparse, json, subprocess, sys, time
import argparse, json, re, subprocess, sys, time

REVIEWER = 'copilot-pull-request-reviewer'

# A review body can carry a collapsed block of findings withheld from the inline threads.
# Those appear nowhere in `reviewThreads`, so polling threads alone reports a clean pass while
# they stand. The alternation is the runbook's, because the heading wording has changed once
# already, and matching one phrasing alone reports zero on a review that has them.
SUPPRESSED = re.compile(r'Suppressed comments|low confidence', re.IGNORECASE)
DETAILS = re.compile(r'<details>(.*?)</details>', re.DOTALL | re.IGNORECASE)
SUMMARY = re.compile(r'<summary>(.*?)</summary>', re.DOTALL | re.IGNORECASE)
TAGS = re.compile(r'</?(?:details|summary)>', re.IGNORECASE)
COUNT = re.compile(r'\((\d+)\)')

# Liveness query: two scalars only, no comment bodies.
# A liveness check does not need the finding text, and re-fetching bodies was 76% of polls.
Q_LIVE = """
Expand All @@ -38,7 +48,7 @@
query($o:String!,$r:String!,$n:Int!){
repository(owner:$o,name:$r){ pullRequest(number:$n){
headRefOid mergeable mergeStateStatus
reviews(last:20){ nodes{ author{login} state commit{oid} submittedAt } }
reviews(last:20){ nodes{ author{login} state commit{oid} submittedAt body } }
reviewThreads(first:100){ nodes{ id isResolved
comments(first:1){ nodes{ author{login} path line body } } }}
}}}
Expand Down Expand Up @@ -66,6 +76,37 @@ def live_state(owner: str, repo: str, num: int) -> tuple[str, bool]:
return head, done


def heading_of(block: str) -> str:
"""The block's `<summary>`, or its opening where the wrapper carries none."""
m = SUMMARY.search(block)
return m.group(1) if m else block[:200]


def suppressed_blocks(body: str) -> list[str]:
"""Return the review body's low-confidence sections, matched on their heading.

The heading carries the match rather than the body text, since a review whose prose discusses
suppressed findings is not itself carrying any. The fallback covers the day the `<details>`
wrapper moves, and takes a heading with a count so ordinary prose is not read as one.
"""
if not body:
return []
blocks = [b for b in DETAILS.findall(body) if SUPPRESSED.search(heading_of(b))]
if blocks:
return blocks
outside = DETAILS.sub('', body).splitlines()
for i, line in enumerate(outside):
if SUPPRESSED.search(line) and COUNT.search(line):
return ['\n'.join(outside[i:])]
return []


def finding_count(block: str) -> int:
"""The heading's `(N)`, floored at one, since a block reported as zero reads as a clean pass."""
m = COUNT.search(heading_of(block))
return max(int(m.group(1)), 1) if m else 1


def digest(owner: str, repo: str, num: int, seen: set[str] | None = None) -> tuple[str, int]:
pr = gql(Q_FULL, owner, repo, num)
head = pr['headRefOid']
Expand All @@ -78,10 +119,14 @@ def digest(owner: str, repo: str, num: int, seen: set[str] | None = None) -> tup
and ((t.get('comments') or {}).get('nodes') or [{}])[0]
.get('author', {}).get('login') == REVIEWER]

# Scoped to the head, so a finding answered in an earlier round does not re-open.
blocks = [b for n in on_head for b in suppressed_blocks(n.get('body') or '')]

lines = [
f'pr={num} head={head[:8]} rounds={len(revs)} '
f'review_on_head={"yes" if on_head else "NO"} '
f'threads={len(threads)} unresolved={len(unresolved)} '
f'suppressed={sum(finding_count(b) for b in blocks)} '
f'merge={pr.get("mergeStateStatus")}'
Comment thread
ptr727 marked this conversation as resolved.
]
new = 0
Expand All @@ -97,6 +142,12 @@ def digest(owner: str, repo: str, num: int, seen: set[str] | None = None) -> tup
new += 1
body = ' '.join((c.get('body') or '').split())
lines.append(f' {mark}{tid} {c.get("path")}:{c.get("line")} {body[:160]}')
for b in blocks:
# Printed whole where a thread body is truncated: a thread can be re-read at its id,
# while a suppressed finding has no thread, so this digest is the only place it appears.
lines.append(' SUPPRESSED: no thread to resolve, answer it in the PR conversation')
# Indentation is kept, since a block carries fenced code a flattened line would garble.
lines += [f' {ln.rstrip()}' for ln in TAGS.sub('', b).splitlines() if ln.strip()]
if seen is not None:
lines[0] += f' new={new}'
return '\n'.join(lines), len(unresolved)
Expand Down
Loading
Loading