diff --git a/.editorconfig b/.editorconfig index aa6be5c3..08aeec34 100644 --- a/.editorconfig +++ b/.editorconfig @@ -45,6 +45,11 @@ indent_size = 2 [.github/workflows/*.{yml,yaml}] end_of_line = lf +# Composite action metadata is Actions-owned like the workflows above, and Dependabot rewrites the +# `uses:` pins inside it with LF exactly as it does theirs, so declaring LF keeps it consistent. +[.github/actions/**/*.{yml,yaml}] +end_of_line = lf + # Catalog snippet workflows mirror the real workflow files pinned LF above. # Keep the snippets LF, so a copied snippet lands compliant instead of needing conversion. [catalog/snippets/workflows/*.{yml,yaml}] diff --git a/.github/actions/prose-gate/action.yml b/.github/actions/prose-gate/action.yml new file mode 100644 index 00000000..5cef1faf --- /dev/null +++ b/.github/actions/prose-gate/action.yml @@ -0,0 +1,97 @@ +# Fleet prose gate, consumed by downstream repos so the rules live here rather than in 20 copies. +# Pin this action to a commit SHA per GOVERNANCE.md "Action pinning". +# The pin is one literal, identical on both branches. +# `uses:` takes no expressions, so a per-branch ref would diverge at every promotion. +# Branch-dependent behavior therefore lives here, where expressions are legal. +name: Fleet prose gate +description: Check the lines a change touches against the fleet prose rules. + +inputs: + base: + description: Ref to diff against, so only lines the change touches are reported. + required: true + paths: + description: Paths to scan. + required: false + default: . + rules-ref: + description: >- + Hub ref supplying the rules. + Empty selects develop on every branch except main, which takes the pinned action version. + required: false + default: '' + +runs: + using: composite + steps: + # A develop-targeted run reads the rules from hub develop. + # That exercises an unpromoted rule change fleet-wide before it reaches main. + # Every other run uses the copy bundled at this action's pinned SHA. + # A released repo's gate is then reproducible, and a hub commit cannot fail a re-run. + - name: Resolve the rules source step + id: rules + shell: bash + env: + REQUESTED: ${{ inputs.rules-ref }} + # `base_ref` is set only on a pull_request event, and a fleet repo gates on push instead. + # The branch name is the reliable signal, so it is read whenever `base_ref` is empty. + TARGET: ${{ github.base_ref || github.ref_name }} + run: | + set -Eeuo pipefail + ref="${REQUESTED:-}" + # Only main is pinned, since it is the released tier and a re-run of it must not change. + # Every other branch merges into develop, so it tracks develop and exercises rules early. + if [ -z "$ref" ] && [ "$TARGET" != "main" ]; then ref="develop"; fi + if [ -z "$ref" ]; then + # Three levels up from the action directory is the repository root of this pinned checkout. + bundled="$GITHUB_ACTION_PATH/../../../scripts/prose_lint.py" + if [ ! -f "$bundled" ]; then + echo "::error::Bundled prose_lint.py not found at $bundled" >&2 + exit 1 + fi + echo "script=$bundled" >>"$GITHUB_OUTPUT" + echo "Rules source: bundled at this action's pinned version" + else + dst="$RUNNER_TEMP/prose_lint.py" + url="https://raw.githubusercontent.com/ptr727/ProjectTemplate/$ref/scripts/prose_lint.py" + # Retry a transient network failure, since it would otherwise fail a correct change. + # A non-200 still fails the step, because -f keeps the trust model unchanged. + # Fail loudly rather than skipping the gate, since a silent skip reports a clean pass. + curl -fsSL --retry 3 --retry-delay 2 --retry-all-errors "$url" -o "$dst" + echo "script=$dst" >>"$GITHUB_OUTPUT" + echo "Rules source: hub $ref" + fi + + # The caller checks out with fetch-depth 0, because a diff against the base needs its history. + - name: Check prose step + shell: bash + env: + SCRIPT: ${{ steps.rules.outputs.script }} + BASE: ${{ inputs.base }} + PATHS: ${{ inputs.paths }} + run: | + set -Eeuo pipefail + # Check the base resolves before scanning, so an empty or absent ref fails naming itself. + # Unresolvable, the run would report the repository's whole backlog against this change. + # A shallow checkout is the usual cause, so the caller fetches full history. + if ! git rev-parse --verify --quiet "$BASE^{commit}" >/dev/null; then + echo "::error::Diff base '$BASE' does not resolve in this checkout." >&2 + echo "::error::Check the ref name and that the job checks out with fetch-depth 0." >&2 + exit 1 + fi + # A caller may write paths as a multi-line YAML block, and `read -ra` stops at a newline. + # Fold newlines into spaces first, so every path is read. + # Left unfolded it scans only the first path and reports the rest clean. + # That is a silent under-scan, the one failure a gate must never have. + # Splitting on whitespace keeps several paths as several arguments. + # Passing the array quoted keeps each element literal, where an unquoted one would glob. + read -ra scan <<<"$(printf '%s' "$PATHS" | tr '\n' ' ')" + if [ "${#scan[@]}" -eq 0 ]; then + echo "::error::No paths to scan after parsing the paths input." >&2 + exit 1 + fi + echo "Scanning ${#scan[@]} path(s): ${scan[*]}" + # Options first, then `--`, so every remaining token is read as a literal path. + # A path-shaped token beginning with a dash would otherwise parse as an option, and + # `--list-files` in that position turns the gate into a file listing that exits 0. + python3 "$SCRIPT" --diff "$BASE" -- "${scan[@]}" diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 18848560..4c4a8e68 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -44,10 +44,17 @@ Auto-review on push is configured (via the branch ruleset's `copilot_code_review gh api repos///pulls//reviews --jq \ '.[] | select(.body | test("Suppressed comments|low confidence")) | .body' -# Scope it to the current head, so an answered finding from an earlier round does not re-open. +# Read every round, not only the head. A suppressed finding has no resolved state, so a push +# does not retire it: it simply stops appearing in a head-scoped query while still unanswered. +# Head-scoping this query is how four rounds went unanswered across three pull requests in a day. +gh api repos///pulls//reviews --jq \ + '[.[] | select(.body | test("Suppressed comments|low confidence"))] | length' + +# Mark which round each came from, since a finding on an older round may since be moot. PR_HEAD=$(gh pr view --json headRefOid --jq '.headRefOid') gh api repos///pulls//reviews --jq \ - "[.[] | select(.commit_id==\"$PR_HEAD\") | select(.body | test(\"Suppressed comments|low confidence\"))] | length" + "[.[] | select(.body | test(\"Suppressed comments|low confidence\")) + | {round: (if .commit_id == \"$PR_HEAD\" then \"head\" else \"earlier\" end), id}]" ``` **Round 1 is normally auto-seeded, so poll for it before trying to self-trigger.** Auto-review-on-open supplies the first review with no `botIds` call needed, but it can lag one to three minutes. After opening a PR (or the first push), **poll** for a Copilot review on the head SHA (see [Verify Review Covered Current Head](#verify-review-covered-current-head)) before concluding none ran. The `requestReviews` mutation below is for **re-requesting on later pushes** (a new head SHA). By then a prior review exists, so its bot node id is readable. A missing bot node id on round 1 therefore means "the auto-review has not landed yet - wait and poll," **not** "ask the maintainer to kick it off." diff --git a/GOVERNANCE.md b/GOVERNANCE.md index 980741df..16cb53e1 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -29,6 +29,7 @@ A state-changing GitHub call is the highest-blast-radius thing an agent does her ## Git and Commit Rules - **Default to staging, not committing.** Stage changes with `git add` and leave `git commit` to the developer unless the developer has explicitly authorized the agent to commit for the current ask ("commit this", "open a PR", etc.). Authorization is scope-bound: it covers the commits needed for that specific task, not a blanket commit license for the rest of the session. +- **"Commit" means commit and push.** An authorization to commit carries the push to the feature branch the work belongs on, because nothing reviews a local commit. The Copilot review loop, the required status checks, and the maintainer all read the remote, so work that stops at `git commit` leaves the review unstarted and the branch's state private to one machine, which reads as progress while none of the gates have run. Push to the feature branch, never to a protected branch (see the Branching Model), and never with `--force`. Holding a commit locally is the narrower case, so it happens when the developer asks for it rather than by default. - **Check the working tree for the maintainer's own uncommitted edits before committing.** The maintainer hand-edits files live (often `README.md`/`HISTORY.md`, sometimes with the editor's LF->CRLF flip on top). Review `git status` first. If there are changes you did not make, ask whether to include them rather than bundling half-finished work or stranding it in an unrelated commit. - **All commits must be cryptographically signed (SSH or GPG).** Branch protection enforces this on both branches, and unsigned commits are rejected on push. Signing depends on environment configuration: `git config commit.gpgsign true`, a configured `user.signingkey`, and a working signing agent (loaded `ssh-agent` for SSH, or `gpg-agent` for GPG). If signing is not configured in the environment, **do not commit**. Surface the missing config to the developer and stop at `git add`. Verify before any agent-authored commit (`git config --get commit.gpgsign && ssh-add -L` or the GPG equivalent). **Signing must be live before the *first* commit, not retrofitted.** Turning on `Require signed commits` against a branch that already has unsigned commits forces a rewrite of that entire history to re-sign it, changing every commit SHA and making whoever does the rewrite the committer and signer of every commit (a rebase preserves the `author` field but not the original signatures, and you cannot sign another contributor's commits for them). During new-repo setup, never create commits until signing is verified. - **Commit under the committing account's own GitHub `noreply` identity, never a private, personal, or invented address.** The `author` and `committer` on every agent-authored commit are the GitHub `noreply` address of the account whose key signs the commit (above). GitHub issues these in a `username@users.noreply.github.com` or `ID+username@users.noreply.github.com` form, and for this single-maintainer fleet it is the owner's `ptr727@users.noreply.github.com`. Do not set `user.name`/`user.email` to a fabricated persona, bot name, or product name, and do not commit under whatever identity the environment happens to carry: verify `git config --get user.email` is that GitHub `noreply` address before committing. **Verify it, do not set it.** The identity is host configuration, set globally once, so a repo-local `user.email` is redundant where the global is right and a wrong identity where it is not, and it silently shadows the global it overrides. A mismatch is a host fault to surface to the maintainer rather than to patch per repo, because a local override hides a broken host that then commits under the wrong identity in every other repo on that machine. A wrong identity is not cosmetic: a private email trips GitHub's email-privacy push protection (GH007), and an unrecognized or invented author pollutes history. Identity is separate from signing: a wrong author does not by itself fail the signature rule, but the ad-hoc identities that produce it are typically also unsigned, which the signing rule above then rejects on push. @@ -258,6 +259,8 @@ For provider-specific mechanics (how to request review, query review state, post ### Triaging Review Comments +**A low-confidence finding is not a low-value one.** Copilot collapses the findings it is least sure of into the review body instead of raising a thread, and in this fleet's experience those are right the large majority of the time. Judge each one against the code, never against its confidence label. They are also the easiest to lose, because they appear in no thread, so a loop that polls threads alone reports a clean pass while they stand (see the Merge Gate, condition 3). + For each comment, classify before responding: - **Bug** - wrong behavior, missing test coverage, or a real divergence between code and docs. Fix it. Reply with the fixing commit SHA when done. @@ -268,10 +271,16 @@ For each comment, classify before responding: ### Responding and Resolution Expectations -Reply inline with either the fixing commit SHA (for accepted issues) or a concise rationale (for declines). Resolve review threads when addressed or intentionally declined with rationale. Issue-level comments (those at `repos/.../issues//comments` rather than tied to a specific line) have no resolution action, so acknowledge with a reply if needed and move on. +Reply inline with either the fixing commit SHA (for accepted issues) or the evidence that disproves it (for declines). **A decline carries proof rather than an assertion**, meaning the command and its output, the code path that makes the concern impossible, or the rule that governs it. "This is fine" is not a reply, and disagreeing without evidence is not addressing a finding, so a thread is not resolved on one. Resolve review threads when addressed, or when declined with that evidence recorded in the thread. Issue-level comments (those at `repos/.../issues//comments` rather than tied to a specific line) have no resolution action, so acknowledge with a reply if needed and move on. After the final push on a PR, sweep older threads from earlier rounds whose code paths no longer exist, or stale unresolved markers remain in the review UI. +**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 ` reports every round and marks which are from earlier ones. + +**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. + ### Escalating to the User Bring the user in when: diff --git a/TODO.md b/TODO.md index 47b192ea..12301bda 100644 --- a/TODO.md +++ b/TODO.md @@ -18,11 +18,13 @@ Running backlog for this repo, kept in a committed file so the guidance survives - Decide whether the canonical README section order follows PlexCleaner, which is a separate question from the four divergences above and affects every repo plus the `readme-structure` audit. PlexCleaner places **Questions or Issues** immediately after the Table of Contents, where the spec orders it ninth, and it carries sections the spec names nowhere, including Performance Considerations, Runtime Metrics, Custom Plugins, Testing, Development Tooling, Feature Ideas, and Sample Media Files. Under the recurrence rule in [`spec/section-model.md`][section-model] those last ones are correctly repo-specific and stay undeclared, so the open question is only the position of the sections the spec already names. - Declare locally-required secrets the way GitHub-stored ones are already declared, and make a gitignored `secrets/` directory the fleet standard that holds them. [`spec/secrets.json`][secrets] covers only the Actions and Dependabot stores, so a repo that deploys somewhere has no declared way to say what it needs at runtime, and the required set is discoverable only by reading the deploy. The pattern already runs in the fleet in two shapes: HomeAutomation-Config keeps a gitignored secrets directory of env files and Docker secret files, and ESPHome-Config keeps a gitignored `secrets.yaml` beside a committed `_secrets.yaml`. The committed file carries the required names with dummy values, so the shape of the requirement is in git while the values never are, which is the same split the GitHub side already gets from `requiredSecrets[]`. Blog needs it immediately, since it deploys on the proxmox host through HomeAutomation-Config's Docker Compose stack and carries the copy destinations and the internal URI. The hub carries neither the directory nor a `.gitignore` entry for one today, so adopting it here comes first. - Re-vendor `repo-config/configure.sh` across the fleet. The hub swept it to one sentence per line, and it is carried `verbatim` with `appliesTo: "*"`, so every repo already holding a copy is byte-mismatched against the hub until it takes the new one. +- Investigate replacing copy-pasted workflow content with cross-repo reuse, now that this repo is public. A public repository's composite actions and reusable workflows can be consumed by any other repository regardless of owner type, so the organization account this pattern was assumed to require is not needed, and the constraint that shaped the current vendor-everything model no longer holds. The catalog under [`catalog/snippets/workflows/`][workflows] is copied into each repo today, which means a fix to a shared job is a sweep across the fleet rather than one edit here, and it is the mechanism by which a defect in a snippet seeds itself into every repo that adopted it. Scope the investigation to which jobs are genuinely identical across repos against which only look similar, since a reusable workflow that needs a long input list to cover per-repo variation is worse than the copy it replaces. Settle the ref policy in the same pass, because consuming hub code at CI time is a floating dependency unless it is pinned, and [`GOVERNANCE.md`][governance] "Action pinning" requires a commit SHA for every action with one documented exception. Note that `uses:` does not accept expressions, so a per-branch ref cannot be selected in the workflow file and any branch-dependent behavior belongs inside the consumed action instead. [audit]: ./spec/audit.py [audit-doc]: ./AUDIT.md +[governance]: ./GOVERNANCE.md [matrix]: ./reports/conformance-matrix.md [readme-structure]: ./spec/readme-structure.md [reports]: ./reports/ @@ -30,3 +32,4 @@ Running backlog for this repo, kept in a committed file so the guidance survives [secrets]: ./spec/secrets.json [section-model]: ./spec/section-model.md [standup]: ./STANDUP.md +[workflows]: ./catalog/snippets/workflows/ diff --git a/host-setup/agent-safety/claude-md-safety.md b/host-setup/agent-safety/claude-md-safety.md index 8908294f..ac75d865 100644 --- a/host-setup/agent-safety/claude-md-safety.md +++ b/host-setup/agent-safety/claude-md-safety.md @@ -1,9 +1,20 @@ ## GitHub Write Safety (Any Project, Every Session) -A `gh` / GitHub API write runs under the logged-in identity, so a mis-targeted write acts publicly as that account on someone else's repository - outward-facing and hard to reverse. These rules bound every write (a git push, an API mutation, a comment, a label, a merge) in every session on this machine, including ad-hoc work outside any project. Reads are unrestricted. A committed repo's `GOVERNANCE.md` "Repository Boundaries and Write Safety" states the same rules for its fleet, and the two are kept in sync deliberately, because this file also covers sessions that `AGENTS.md` never reaches. The `gh-write-guard` PreToolUse hook enforces the mechanical half. +A `gh` / GitHub API write runs under the logged-in identity, so a mis-targeted write acts publicly as that account on someone else's repository, an outward-facing and hard-to-reverse act. These rules bound every write (a git push, an API mutation, a comment, a label, a merge) in every session on this machine, including ad-hoc work outside any project. Reads are unrestricted. A committed repo's `GOVERNANCE.md` "Repository Boundaries and Write Safety" states the same rules for its fleet, and the two are kept in sync deliberately, because this file also covers sessions that `AGENTS.md` never reaches. The `gh-write-guard` PreToolUse hook enforces the mechanical half. - **Write only within the owner of the current checkout's repository.** Every state-changing call targets this checkout's `origin` or a sibling repository under the same owner. A broad or logged-in identity is capability, not permission. A repository under a **different owner** needs explicit human permission naming it, set in `GH_WRITE_GUARD_ALLOW` before the session starts rather than granted by the agent to itself, and a "harmless test" write is still a write. -- **Never fabricate, guess, or reuse an identifier passed to a write.** Every id a write consumes (a node id, a numeric id, a thread or comment id) is captured from a live query in the same session into a variable and passed from there. Ids resolve globally, so a wrong-but-valid id does not fail - it writes to the wrong target in another repository. If a query returns no id, stop rather than invent one. -- **A write is never a probe, and a write's output is never suppressed.** Never fire a state-changing call to see whether it works, and never append an output-discarding or force-success tail (for example `>/dev/null`, `2>/dev/null`, `&>/dev/null`, `|| true`, `|| :`, `|| echo`) to a mutation. A write that appears to fail is verified, not assumed harmless - it may have succeeded on the server. +- **Never fabricate, guess, or reuse an identifier passed to a write.** Every id a write consumes (a node id, a numeric id, a thread or comment id) is captured from a live query in the same session into a variable and passed from there. Ids resolve globally, so a wrong-but-valid id does not fail. It writes to the wrong target, in another repository. If a query returns no id, stop rather than invent one. +- **A write is never a probe, and a write's output is never suppressed.** Never fire a state-changing call to see whether it works, and never append an output-discarding or force-success tail (for example `>/dev/null`, `2>/dev/null`, `&>/dev/null`, `|| true`, `|| :`, `|| echo`) to a mutation. A write that appears to fail is verified, not assumed harmless, because it may have succeeded on the server. + +## Authorization Scope and Memory Hygiene (Any Project, Every Session) + +A permission the maintainer grants is bounded by what he said, where he said it, and when. Memory is how those bounds get lost, because a note outlives the session that justified it and a grant given for one repository is later read as a mode. This section is a backstop on how an authorization is recorded and read, and it holds in every project because the failure is in the mechanism rather than in any one project. It is a restriction and never a grant, which is why it belongs in this host-wide file: only restrictions live here, so nothing in this file can widen a permission. + +- **A committed rule outranks a memory, always.** The rules in a repository's own `AGENTS.md`, `GOVERNANCE.md`, `CODESTYLE.md`, or equivalent are the law. A memory is a private note about a past session, and it never supersedes, retires, or relaxes one. Where a memory and a committed rule disagree, follow the rule, say the memory looks stale, and ask. A memory asserting that a documented default has been "retired" or "superseded" is the specific claim to distrust. +- **Record a grant with its scope and its lifetime, both explicit.** Scope is the narrowest of repository, project, or fleet that he actually named. Lifetime defaults to **this session only**. Write a grant as standing solely when he said it stands, and quote the words that said so. An unscoped or undated authorization in memory is read as expired rather than as broad. +- **Storage location is not scope.** A note in one project's memory directory can still assert authority over other repositories, and the directory holding it does not bound what it claims. State the scope in the text. A project-scoped file claiming fleet-wide authority is a defect to fix on sight rather than a convenience to rely on. +- **Never widen a grant by inference.** Permission for one repository does not carry to a sibling, permission for one pull request does not carry to the next, and permission for one task does not become a mode for the session. Similarity is not authorization. Re-ask instead, because re-asking is cheap and an unwanted write is not. +- **Never grant to yourself.** Do not record an authorization he did not give, infer one from a tool's capability, or restore one he narrowed or withdrew. Capability is not permission. +- **The irreversible step stays his.** Merging, publishing, releasing, force-pushing, deleting, and changing branch protection each stop for an explicit and current go-ahead, however green the work is and whatever a memory says about a past session. diff --git a/scripts/README.md b/scripts/README.md index 1a68bf97..1ee2f87c 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -32,6 +32,12 @@ python3 scripts/prose_lint.py . --diff origin/develop Whole-tree (`python3 scripts/prose_lint.py .`) reports the legacy backlog as well, which is informational rather than a gate. `charset`, `dupword` and `spelling` are clean tree-wide, so CI gates those three and reports the rest warn-only. +The default rule set covers comment shape (`comment-wrap` and `comment-case`) alongside the prose rules. It did not, which meant a run nobody parameterized reported clean on a wrapped comment while the rule read as enforced, and comment shape is the most frequently regressed rule in agent-authored work. Reading the backlog it exposes needs no flag now, and gating it still needs `--diff`, because the tree carries several hundred of them. + +A wide scan skips the trees this repo generates rather than authors, currently `reports/`, which [`spec/audit.py`][audit] writes. A finding there is the audit engine's phrasing rather than an author's, so no edit to that tree can fix it, and leaving them in made the repo's own number mostly generated output. Naming such a path directly still reads it (`prose_lint.py reports`), so nothing becomes uncheckable. + +In markdown an HTML comment carrying no sentence punctuation is treated as a structural marker rather than commentary, so it takes neither a capital nor a sentence split. The reference-link group headers, the ToC-omit directive, and the `agent-safety` install markers are each matched verbatim by a tool, so rewriting one to satisfy the rule breaks whatever reads it. A markdown comment that does punctuate a sentence is prose and is judged as prose. + The `spelling` rule covers the US English convention where cspell does not reach. That gate reads README and HISTORY only, deliberately, because gating every markdown file would mean endlessly padding `cspell.json` with technical terms, so a British spelling anywhere else in the tree had nothing checking it. The banned words are generated from stems rather than listed one by one, since an inflected spelling is as wrong as its base and a hand-listed family drifts as soon as one form is added without the others. Two words are deliberately absent: `analyses` is the US plural of `analysis` as much as it is a British verb form, and `cancelled` is a GitHub Actions job status rather than prose. **Outside markdown `spelling` and `dupword` read the comments, not the source lines**, reusing the extraction the `comment-wrap` rule already does. An identifier, a string literal, or a lookup table is code, and judging it as prose would make this script report its own table of banned words. Each comment on a line is judged on its own rather than joined with its neighbors, because two comments are two sentences and joining them reads the second's opening word as a repeat of the first's last. @@ -87,12 +93,13 @@ 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 `
` 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 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. 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 `
` 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. +[audit]: ../spec/audit.py [copilot-instructions]: ../.github/copilot-instructions.md [editorconfig]: ../.editorconfig [files]: ../spec/files.json diff --git a/scripts/pr_review.py b/scripts/pr_review.py index ec0ed133..067d4afa 100644 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -119,14 +119,23 @@ 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 '')] + # Every round, not just the head, because a suppressed finding has no resolved state to read. + # Head-scoping treated "superseded by a push" as "answered", and the two are not the same. + # A finding nobody replied to left the digest the moment the branch moved, reporting zero. + # That is how four rounds went unanswered across three pull requests in one day. + # The head is still marked per block, since a finding on an older round may be moot. + # Deciding that is the reader's call rather than one the count makes for them. + blocks = [(n, b) for n in revs for b in suppressed_blocks(n.get('body') or '')] + on_head_blocks = [b for n, b in blocks if (n.get('commit') or {}).get('oid') == head] + stale = sum(finding_count(b) for n, b in blocks) - sum( + finding_count(b) for b in on_head_blocks) 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'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}) ' f'merge={pr.get("mergeStateStatus")}' ] new = 0 @@ -142,10 +151,20 @@ 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: + for n, 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') + # GraphQL returns a null commit for a pending or partial review. + # An empty sha rendered as "raised on , earlier round", losing what traces the finding. + sha = ((n.get('commit') or {}).get('oid') or '')[:8] + if not sha: + where = 'commit unknown, treat as outstanding' + elif sha == head[:8]: + where = 'on head' + else: + where = f'raised on {sha}, earlier round' + lines.append(f' SUPPRESSED ({where}): no thread to resolve, ' + 'answer it in the PR conversation quoting the finding') # 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: diff --git a/scripts/prose_lint.py b/scripts/prose_lint.py index 4c11d86b..24b1c03f 100644 --- a/scripts/prose_lint.py +++ b/scripts/prose_lint.py @@ -33,7 +33,14 @@ 'spelling': 'a British spelling where the repo convention is US English', } DEFAULT_RULES = frozenset({'charset', 'charset-unknown', 'semicolon', 'dash', 'dupword', - 'spelling'}) + 'spelling', 'comment-wrap', 'comment-case'}) + +# Trees this repo generates rather than authors, skipped when a wider scan expands into them. +# The gate then measures hand-written prose. +# `spec/audit.py` writes `reports/`, so a finding there is the engine's phrasing, not an author's. +# No edit to that tree can fix one. +# Naming one of these paths directly still reads it, so nothing becomes uncheckable. +GENERATED_TREES = frozenset({'reports'}) # Produced rather than authored trees, consulted only on the no-git fallback path. # Where git can answer, its own ignore rules are the better answer. @@ -79,6 +86,32 @@ def changed_lines(base: str) -> dict[str, set[int]] | None: return out +def repo_prefix(root: Path) -> str: + """Where `root` sits inside its repository, as a posix prefix, or '' when git cannot say. + + The generated-tree decision has to be made against the repository-relative path. Reading the + filesystem path instead lets a directory *above* the checkout decide it, so a repository + cloned under a parent named `reports` had its own `reports/` tree scanned as authored. + """ + try: + r = subprocess.run(['git', '-C', str(root), 'rev-parse', '--show-prefix'], + capture_output=True, text=True) + except (OSError, ValueError): + return '' + return r.stdout.strip() if r.returncode == 0 else '' + + +def repo_root(path: Path) -> str: + """The repository top level containing `path`, or '' when git cannot say.""" + start = path if path.is_dir() else path.parent + try: + r = subprocess.run(['git', '-C', str(start), 'rev-parse', '--show-toplevel'], + capture_output=True, text=True) + except (OSError, ValueError): + return '' + return r.stdout.strip() if r.returncode == 0 else '' + + def tracked_paths(root: Path) -> list[Path] | None: """Paths git tracks under `root`, or None when git cannot answer. @@ -136,7 +169,24 @@ def discover(paths: list[str], excludes: tuple[str, ...] = ()) -> list[Path]: print(f'warning: git cannot describe {root}, falling back to a filesystem walk', file=sys.stderr) tracked = walk_paths(root) - found.extend(tracked) + # Judge against the repository-relative path, never the filesystem one. + # A directory above the checkout must not decide whether a file is generated. + # An absolute argument otherwise carried its whole parent chain into the test. + prefix = repo_prefix(root) + for q in tracked: + try: + inside = Path(prefix) / q.relative_to(root) + except ValueError: + # Unreachable while both come from the same root, and kept safe rather than tidy. + # With no repository-relative path there is nothing to judge, so scan the file. + # Skipping on doubt is how a gate reports clean over what it never read. + found.append(q) + continue + if GENERATED_TREES.isdisjoint(inside.parts): + found.append(q) + elif not GENERATED_TREES.isdisjoint(Path(prefix).parts): + # The root named is itself inside a generated tree, so it was asked for. + found.append(q) keep = [p for p in found if not any(x in rel(p) for x in excludes) and p.is_file() and is_text(p)] return sorted(set(keep)) @@ -766,6 +816,14 @@ def comment_wrap_findings(path: Path, raw: str, lines: list[str]) -> list[tuple[ if not body or NOT_PROSE.search(body) or BARE_URI.match(body.strip()): prev_body = '' continue + # An unpunctuated markdown HTML comment is a structural marker, not commentary. + # It is a label, so it takes neither a capital nor a sentence split. + # A tool matches each one verbatim, so rewriting it breaks whatever reads it. + # Group headers, the ToC-omit directive, and the agent-safety markers are the cases. + # A comment that does punctuate a sentence is prose and is judged as prose. + if path.suffix == '.md' and not SENT_END.search(body): + prev_body = '' + continue if RUN_ON.search(strip_inline_code(body)): out.append((n, 'comment-wrap', 'two sentences on one comment line -> split them')) # A continuation is the very next line: two comments with code between them are separate. @@ -893,6 +951,29 @@ def main(argv: list[str] | None = None) -> int: a = ap.parse_args(argv) rules = set(a.checks or DEFAULT_RULES) + + # Checked before discovery, which reads every tracked file to classify it as text. + # A run this rejects would otherwise pay that cost and throw the result away. + # `--list-files` is exempt, since it reports the scan scope and never consults the diff. + if a.diff and not a.list_files: + # `git diff` runs in the current directory while the paths may name another checkout. + # Scanning one repository and diffing another intersects to nothing. + # The run then reports clean, which is the false clean this gate exists to prevent. + # It cost a real verification once, where a branch read zero from the wrong directory. + # A path under no repository at all fails the same way, and more quietly. + # Discovery walks the filesystem, then every absolute key misses the repo-relative ones. + # Requiring the same root covers both, where testing for a different one did not. + here = repo_root(Path('.')) + for raw in (a.paths or ['.']): + there = repo_root(Path(raw)) + if here and there != here: + where = there or 'no git repository' + print(f'error: --diff resolves against {here}, but {raw} is in {where}. ' + 'Run the gate from the repository being scanned, since a diff taken ' + 'elsewhere scopes every finding away and reports a false clean.', + file=sys.stderr) + return 2 + files = discover(a.paths or ['.'], tuple(a.exclude)) if a.list_files: @@ -902,7 +983,15 @@ def main(argv: list[str] | None = None) -> int: scope = changed_lines(a.diff) if a.diff else None if a.diff and scope is None: - print('warning: git diff failed; falling back to whole-tree scan', file=sys.stderr) + # Widening to the whole tree answers a different question, and answers it silently. + # A caller scoping to a change gets the backlog reported as though the change made it. + # A CI adoption hits this first, where an unresolvable base walls off the first run. + # Scoping to nothing instead would report a false clean, so neither default is honest. + print(f'error: cannot diff against {a.diff!r}, so the run cannot be scoped to changed ' + 'lines. Refusing to scan the whole tree instead, since that reports the existing ' + 'backlog as though this change introduced it. Check the ref exists and that the ' + 'checkout carries its history.', file=sys.stderr) + return 2 if scope is not None: files = [f for f in files if rel(f) in scope] diff --git a/scripts/test_pr_review.py b/scripts/test_pr_review.py index 79f4e79d..5f722356 100644 --- a/scripts/test_pr_review.py +++ b/scripts/test_pr_review.py @@ -148,19 +148,42 @@ def test_either_documented_heading_counts_and_a_clean_body_does_not(self) -> Non out, _ = pr_review.digest('o', 'r', 7) self.assertIn(f'suppressed={want}', out) - def test_a_block_on_a_stale_round_is_not_reported_again(self) -> None: - """Scoped to the head, so a finding answered before a push does not re-open after it.""" + def test_a_block_on_a_review_with_no_commit_names_that_rather_than_an_empty_sha(self) -> None: + """GraphQL returns a null commit for a pending review, and the sha is what traces it. + + Rendered from an empty string it read "raised on , earlier round", which loses the round + and reads as a formatting glitch rather than as a finding that still needs an answer. + """ + self.answer(payload([review(body=collapsed()) | {'commit': None}])) + out, _ = pr_review.digest('o', 'r', 7) + self.assertIn('commit unknown, treat as outstanding', out) + self.assertNotIn('raised on ,', out) + # It still counts, since an unknown round is not a reason to drop a finding. + self.assertIn('suppressed=1', out) + + def test_a_block_on_an_earlier_round_is_still_reported(self) -> None: + """A suppressed finding has no resolved state, so a push must not retire it. + + Head-scoping read "superseded by a push" as "answered", and the two are not the same: + a finding nobody replied to left the digest the moment the branch moved, and the run + then reported zero. Four rounds of findings went unanswered across three pull requests + that way in a single day, each one discovered by the maintainer rather than the gate. + + The round is marked instead, since a finding on an older round may be moot and deciding + that is the reader's judgment rather than something the count should make for them. + """ self.answer(payload([review(oid=OLD, body=collapsed()), review()])) out, _ = pr_review.digest('o', 'r', 7) - self.assertIn('suppressed=0', out) - self.assertNotIn('SUPPRESSED', out) + self.assertIn('suppressed=1', out) + self.assertIn('earlier=1', out) + self.assertIn('earlier round', out) def test_the_finding_prints_whole_under_a_marker_naming_the_answer(self) -> None: """A thread can be re-read at its id and truncates for that reason, and this cannot.""" finding = 'a.py:12 ' + ('the same clause repeated. ' * 20).strip() self.answer(payload([review(body=collapsed(finding=finding))])) out, _ = pr_review.digest('o', 'r', 7) - self.assertIn('SUPPRESSED: no thread to resolve, answer it in the PR conversation', out) + self.assertIn('no thread to resolve, answer it in the PR conversation', out) self.assertIn(finding, out) # The `
` wrapper is markup around the finding, not part of it. self.assertNotIn('', out) diff --git a/scripts/test_prose_lint.py b/scripts/test_prose_lint.py index 9fc8c983..dc37645e 100644 --- a/scripts/test_prose_lint.py +++ b/scripts/test_prose_lint.py @@ -580,6 +580,26 @@ def test_a_comment_inside_a_fenced_block_is_skipped(self) -> None: self.assertEqual(['comment-wrap'], self.flag('a.md', 'Prose.\n\n\n')) + def test_an_unpunctuated_markdown_marker_is_a_label_not_a_sentence(self) -> None: + """A tool matches these verbatim, so a capital or a split would break what reads them. + + The reference-link group headers, the ToC-omit directive, and the `agent-safety` install + markers all open lowercase or sit adjacent, which reads as a sentence that failed to + start or as one wrapping into the next. + """ + for marker in ('', '', + ''): + with self.subTest(marker=marker): + self.assertEqual([], self.flag('a.md', f'Prose.\n\n{marker}\n')) + # Adjacent markers must not read as one sentence wrapping into the next. + self.assertEqual([], self.flag( + 'a.md', 'Prose.\n\n\n\n')) + # A punctuated HTML comment is commentary, so it stays judged as prose. + self.assertEqual(['comment-wrap'], + self.flag('a.md', 'Prose.\n\n\n')) + # Outside markdown the carve-out does not apply, since there the marker case does not arise. + self.assertEqual(['comment-case'], self.flag('a.py', '# lowercase opening\n')) + def test_a_block_opener_inside_a_line_comment_is_text(self) -> None: """Read as a real opener it opens a block, and the code lines below are linted as prose. @@ -1015,6 +1035,46 @@ def test_an_excluded_path_is_dropped(self) -> None: found = prose_lint.discover([str(self.tmp)], ('drop.md',)) self.assertEqual(['keep.md'], [p.name for p in found]) + def test_a_generated_tree_is_skipped_when_a_wider_scan_expands_into_it(self) -> None: + """Its prose is the generator's, so a finding there names no edit an author can make.""" + (self.tmp / 'authored.md').write_text('fine\n', encoding='utf-8') + generated = self.tmp / 'reports' + generated.mkdir() + (generated / 'audit.md').write_text('fine\n', encoding='utf-8') + with mock.patch.object(prose_lint, 'tracked_paths', return_value=None), \ + contextlib.redirect_stderr(io.StringIO()): + found = prose_lint.discover([str(self.tmp)]) + self.assertEqual(['authored.md'], [p.name for p in found]) + + def test_a_parent_directory_above_the_checkout_does_not_decide_generated(self) -> None: + """The decision is repository-relative, so the filesystem path above it cannot leak in. + + Judged on the absolute path, a checkout under a parent named `reports` carried that + parent into every file's parts, which read the whole scan as deliberately requested and + put the repository's own generated tree back into an ordinary sweep. + """ + repo = self.tmp / 'reports' / 'checkout' + (repo / 'reports').mkdir(parents=True) + (repo / 'authored.md').write_text('fine\n', encoding='utf-8') + (repo / 'reports' / 'audit.md').write_text('fine\n', encoding='utf-8') + with mock.patch.object(prose_lint, 'tracked_paths', return_value=None), \ + mock.patch.object(prose_lint, 'repo_prefix', return_value=''), \ + contextlib.redirect_stderr(io.StringIO()): + found = prose_lint.discover([str(repo)]) + self.assertEqual(['authored.md'], [p.name for p in found]) + + def test_naming_a_generated_tree_directly_still_reads_it(self) -> None: + """The skip keeps a wide scan honest, and must not make the tree uncheckable.""" + generated = self.tmp / 'reports' + generated.mkdir() + (generated / 'audit.md').write_text('fine\n', encoding='utf-8') + with mock.patch.object(prose_lint, 'tracked_paths', return_value=None), \ + contextlib.redirect_stderr(io.StringIO()): + found = prose_lint.discover([str(generated)]) + self.assertEqual(['audit.md'], [p.name for p in found]) + loose = generated / 'audit.md' + self.assertEqual([loose], prose_lint.discover([str(loose)])) + def test_an_unreadable_root_is_not_a_file_set(self) -> None: """`tracked_paths` answers None on the error paths, never an empty list read as clean.""" with mock.patch.object(prose_lint.subprocess, 'run', side_effect=OSError): @@ -1289,6 +1349,66 @@ def test_every_rule_name_is_offered_by_the_cli(self) -> None: def test_default_rules_are_a_subset_of_the_declared_rules(self) -> None: self.assertLessEqual(set(prose_lint.DEFAULT_RULES), set(prose_lint.RULES)) + def test_a_bare_run_checks_comment_shape(self) -> None: + """Comment shape is the most regressed rule, so a run nobody parameterized must catch it. + + It sat outside DEFAULT_RULES, so `prose_lint.py .` reported clean on a wrapped comment + and the rule read as enforced while nothing ran it. + """ + for rule in ('comment-wrap', 'comment-case'): + with self.subTest(rule=rule): + self.assertIn(rule, prose_lint.DEFAULT_RULES) + bait = self.tmp / 'bait.py' + bait.write_text('# A sentence that wraps\n# across two comment lines.\n', + encoding='utf-8') + with mock.patch.object(prose_lint, 'discover', return_value=[bait]): + self.assertEqual(1, prose_lint.main([])) + + def test_diffing_one_repository_while_scanning_another_is_refused(self) -> None: + """The intersection is empty, so it reports a clean run over an unchecked tree. + + `git diff` runs in the current directory while the paths may name another checkout. + A PhotoCleaner branch reported zero findings when the gate was run from the hub's + directory and three when run from its own, and the zero was believed. + """ + with mock.patch.object(prose_lint, 'repo_root', + side_effect=lambda p: '/hub' if str(p) == '.' else '/other'), \ + mock.patch.object(prose_lint, 'discover') as disc: + self.assertEqual(2, prose_lint.main(['--diff', 'HEAD', '/other/tree'])) + # Refused before discovery, which reads every tracked file to classify it as text. + disc.assert_not_called() + + def test_a_path_under_no_repository_is_refused_too(self) -> None: + """It fails the same way as a different repository, and more quietly. + + Discovery falls back to a filesystem walk, then every absolute key misses the diff's + repository-relative ones, so the scope drops every file and the run exits 0. Testing for + a *different* root missed this, because there is no root to differ from. + """ + with mock.patch.object(prose_lint, 'repo_root', + side_effect=lambda p: '/hub' if str(p) == '.' else ''), \ + contextlib.redirect_stderr(io.StringIO()) as err: + self.assertEqual(2, prose_lint.main(['--diff', 'HEAD', '/tmp/loose'])) + self.assertIn('no git repository', err.getvalue()) + + def test_list_files_still_reports_scope_across_repositories(self) -> None: + """It reports the scan scope and never consults the diff, so the guard must not stop it.""" + clean = self.tmp / 'clean.md' + clean.write_text('fine\n', encoding='utf-8') + with mock.patch.object(prose_lint, 'repo_root', + side_effect=lambda p: '/hub' if str(p) == '.' else '/other'), \ + mock.patch.object(prose_lint, 'discover', return_value=[clean]): + self.assertEqual(0, prose_lint.main(['--list-files', '--diff', 'HEAD', '/other/tree'])) + + def test_a_matching_repository_is_not_refused(self) -> None: + """The guard must not reject the ordinary case it sits in front of.""" + clean = self.tmp / 'clean.md' + clean.write_text('Nothing here breaks a rule.\n', encoding='utf-8') + with mock.patch.object(prose_lint, 'repo_root', return_value='/hub'), \ + mock.patch.object(prose_lint, 'discover', return_value=[clean]), \ + mock.patch.object(prose_lint, 'changed_lines', return_value={}): + self.assertEqual(0, prose_lint.main(['--check', 'dupword', '--diff', 'HEAD'])) + def test_diff_scope_reports_only_the_changed_lines(self) -> None: """A finding on an untouched line is the backlog, which the diff run must not attribute.""" bait = self.tmp / 'bait.md' @@ -1309,13 +1429,22 @@ def test_a_file_outside_the_diff_is_dropped_entirely(self) -> None: mock.patch.object(prose_lint, 'changed_lines', return_value={'other.md': {1}}): self.assertEqual(0, prose_lint.main(['--check', 'dupword', '--diff', 'HEAD'])) - def test_a_failed_diff_falls_back_to_the_whole_tree(self) -> None: - """Scoping to nothing would report a clean run, so an unusable diff widens instead.""" + def test_a_failed_diff_is_an_error_rather_than_a_wider_or_narrower_scan(self) -> None: + """An unusable diff has three answers, and only one of them is honest. + + Scoping to nothing reports a clean run, which is a false pass. Widening to the whole tree + reports the existing backlog as though this change introduced it, which is what a CI + adoption hits first: an unresolvable base turned PhotoCleaner's first run into 420 + findings its branch never touched. Failing names the cause and asserts neither. + + The exit code is distinct from a findings exit, so a caller can tell "the gate could not + run" from "the gate ran and found something". + """ bait = self.tmp / 'bait.md' bait.write_text(f'{DUP} thing\n', encoding='utf-8') with mock.patch.object(prose_lint, 'discover', return_value=[bait]), \ mock.patch.object(prose_lint, 'changed_lines', return_value=None): - self.assertEqual(1, prose_lint.main(['--check', 'dupword', '--diff', 'HEAD'])) + self.assertEqual(2, prose_lint.main(['--check', 'dupword', '--diff', 'HEAD'])) def test_list_files_prints_the_scope_and_reports_nothing(self) -> None: """The audit path for the sweep scope exits 0 even on a tree full of findings."""