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..55f8dade 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -44,15 +44,22 @@ 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." -> **The reviewer login differs by API.** In **GraphQL** (`gh api graphql` and `gh pr view --json reviews`, which is GraphQL-backed) the `Bot.login` is `copilot-pull-request-reviewer`, with **no `[bot]` suffix**. In the **REST** API (`gh api repos/.../issues|pulls/...`) the same account's `user.login` is `copilot-pull-request-reviewer[bot]`, **with** the suffix. Each query below uses the correct form for its API, so match the API, not a single spelling, when adapting them. +> **The reviewer login differs by API, in three forms rather than two.** In **GraphQL** (`gh api graphql` and `gh pr view --json reviews`, which is GraphQL-backed) the `Bot.login` is `copilot-pull-request-reviewer`, with **no `[bot]` suffix**. In the **REST** API (`gh api repos/.../issues|pulls/...`) the same account's `user.login` is `copilot-pull-request-reviewer[bot]`, **with** the suffix. In a REST **timeline** `review_requested` event the `requested_reviewer` is a third spelling again, login `Copilot` with `type` `Bot`, so a filter written against either of the other two selects nothing there and reports a pull request with requests as having none. Match on the type plus a loose login test rather than on any one spelling, and each query below uses the correct form for its API. ```sh # 1. PR node id + the Copilot reviewer's bot node id (read from any existing @@ -109,7 +116,8 @@ Known non-working request paths (don't rely on them, and use the `requestReviews - `copilot-pull-request-reviewer` as a requested reviewer slug returns 422. - `requestReviews` with the reviewer's bot node id in **`userIds`** fails with `Could not resolve to User node`, because the Copilot reviewer is a **Bot**, so its node id goes in **`botIds`** (as in the mutation above), never `userIds`. - `suggestedActors(capabilities: [CAN_BE_ASSIGNED])` lists `copilot-swe-agent` (the coding agent), not `copilot-pull-request-reviewer`, so do not source the reviewer's bot node id there. Read it from an existing review per step 1 above. -- There is no `removePullRequestFromReviewRequest` mutation, and removing the reviewer to force a fresh pass is unnecessary anyway, since `requestReviews` with `union: true` re-fires the review on the current head. +- There is no `removePullRequestFromReviewRequest` mutation, but removal is not therefore impossible: `requestReviews` **replaces** the reviewer set when `union` is false (the schema describes `union` as "add users to the set rather than replace"), so an empty `botIds` with `union: false` removes the pending request. Reach for it only in the stuck case below, since `union: true` re-fires a review on the current head without it. +- `gh pr view --json reviewRequests` **omits a Bot reviewer entirely**, reporting an empty set while Copilot sits in it. Read the pending set through GraphQL `reviewRequests`, which returns the `Bot` node, because the REST-backed projection makes a pending request read as no request at all. ### Verify Review Covered Current Head @@ -141,6 +149,46 @@ This path is only for a **genuinely missing** review, meaning no Copilot review **A slow review is pending, not missing, so poll with backoff and never escalate on a timeout alone.** Copilot can lag far beyond the usual one-to-three minutes when it has been re-requested many times in quick succession, because it throttles under load, and a re-review landing tens of minutes after the request is normal. A poll that times out is therefore evidence only that the review has not landed *yet*, not that Copilot is done or unresponsive. Report the status as "review still pending" and keep polling on a widening interval (for example 20s steps, then a few minutes) rather than stopping. Enter the escalation step below only when the `requestReviews` mutation itself no-ops or errors, or after a genuinely long wait with the request confirmed accepted, never merely because one fixed poll window elapsed. +**Bound each wait, and read what Copilot actually posted before opening another one.** A poll that widens forever is indistinguishable from a poll that has stopped, and "still pending" is the honest report for exactly as long as evidence supports it. Two readings decide whether waiting again is warranted. Compare the request's timestamp against the newest Copilot activity of **any** kind on the pull request, since a reviewer that has already answered on a later head, or that posted an issue comment instead of a formal review, is not a reviewer running late, and a wait that keeps reporting "pending" against a landed review is a broken wait rather than a slow reviewer. Then read that newest response, because a Copilot answer naming a quota or a rate limit is a **terminal** outcome rather than a pending one: no formal review will land, so path (1) never matches the head and path (2) is correctly never confirmed, both paths behave exactly as specified, and the agent waits for something that is not coming. The fix is account-side and re-requesting does not change it, so report it to the maintainer and stop waiting. Where the newest response is neither a review nor a refusal you recognize, that too goes to the maintainer with its text, rather than being waited through. + +**A pending request nothing picked up is a third state, and it is the one that looks most like patience.** Copilot raises a `copilot_work_started` timeline event within about half a minute of accepting a request, and submits its review a few minutes later. A request that never draws one is not a slow review, it is a request nothing is acting on, and it stays that way indefinitely: one sat for thirteen and a half hours while the pull request read as waiting on the reviewer. Elapsed time cannot tell the two apart, since a genuinely slow round also shows no review, so read the event rather than the clock. `copilot_work_started` appears in the REST timeline only, and no GraphQL timeline item carries it: + +```sh +# The pending set (GraphQL, since the `gh pr view` projection cannot see a Bot reviewer). +gh api graphql -f query=' +{ repository(owner:"",name:""){ pullRequest(number:){ + reviewRequests(first:10){ totalCount + nodes{ requestedReviewer{ __typename ... on Bot{login} ... on User{login} } } } } } }' + +# The request and pickup events, newest last. A `review_requested` with no later +# `copilot_work_started` is the stuck state. Requests are filtered to the reviewer's own, +# since a human requested afterwards is a different request and reading it as this one +# reports a picked-up review as never picked up. `per_page` is the pagination cost. +gh api --paginate 'repos///issues//timeline?per_page=100' \ + --jq '.[] | select(.event == "copilot_work_started" or (.event == "review_requested" + and .requested_reviewer.type == "Bot" + and ((.requested_reviewer.login // "") | ascii_downcase | test("copilot")))) + | "\(.event) \(.created_at)"' +``` + +**Recover it by clearing the request and requesting again**, because the pull request UI offers no re-request control while a request is pending, and `requestReviews` with `union: true` adds a reviewer already in the set, which changes nothing. Read the pending set first, since `union: false` replaces the whole set and would drop a human reviewer requested alongside the bot. Where the clear-and-request does not draw a `copilot_work_started` within a minute or so, push a commit instead, since a new head raises a fresh request rather than poking a stale one. + +```sh +PR_NODE=$(gh pr view --json id --jq '.id') +# 1. Clear. `union: false` replaces the set, so an empty botIds removes the pending request. +gh api graphql -f query=' +mutation($pr: ID!) { + requestReviews(input: { pullRequestId: $pr, botIds: [], union: false }) { + pullRequest { reviewRequests(first: 10) { totalCount } } } +}' -F pr="$PR_NODE" +# 2. Request again, against a now-empty set, with $BOT_ID read as in "Triggering and Polling". +gh api graphql -f query=' +mutation($pr: ID!, $bot: ID!) { + requestReviews(input: { pullRequestId: $pr, botIds: [$bot], union: true }) { + pullRequest { reviewRequests(first: 10) { totalCount } } } +}' -F pr="$PR_NODE" -F bot="$BOT_ID" +``` + If a review did not run on the current head, retry: 1. Wait briefly and check head-SHA coverage (see above). diff --git a/AGENTS.md b/AGENTS.md index 13d20700..3afe6f43 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -13,7 +13,7 @@ An agent session is billed on the context it carries, not the work it does. Ever ### Session Scope - **One deliverable, one session.** A session covers one branch and one deliverable, and ends when that work merges. A multi-step task is one deliverable and stays in one session. Two unrelated tasks are two sessions even when they run back to back. -- **End a session at any of these, without being asked:** the branch changes, the pull request merges, the next task is unrelated to the last, or a third review round opens on the same pull request. +- **End a session at any of these, without being asked:** the branch changes, the pull request merges, or the next task is unrelated to the last. A review round is none of them. A loop still producing findings is the deliverable in progress, and a round count is not a reason to leave one open. - **Hand off in a file, never in context.** Close a session by writing at most 2 KB to a scratch file: branch, pull request link, what is done, the next command. A summary held in context is re-billed until the session ends, and a summary on disk is read once by whoever needs it. - **Re-derive state, do not carry it.** "This session already has the context" is the signal to split, not to continue. Context that has gone stale is worse than absent, because a file read hundreds of requests ago no longer describes the file. - **Compaction is a fallback, not the strategy.** It restarts context from a floor and climbs again, where a fresh session starts from zero. @@ -45,6 +45,7 @@ If a rule you were given does not cover what you find, stop and report it. Do no ``` - **Wait in a background process, not in a poll loop.** A review or CI wait is a sequence of near-identical requests, each billed for whatever context it happens to carry. Run the wait as one backgrounded command that returns when the condition is met. +- **A wait separates three outcomes, and says which one it reached.** The condition was met, it has not been met yet, and the wait cannot reach it at all are three different results, and a backgrounded wait that emits nothing renders all three identically. Run the command once in the foreground and read its output before backgrounding it, because a wait is only as good as the command inside it, and an unsupported flag on the installed tool version exits non-zero with an empty stdout that every naive test reads as "nothing yet". Never let a fallback stand in for a failed command, since `|| echo '[]'`, `|| true`, and `2>/dev/null` convert an error into that same reading, which is the suppression the write-safety rules already forbid on a mutation. Make the wait emit on failure as loudly as on success, so silence means "still running" and nothing else, and bound it, so a condition that is never coming ends in a report rather than in another wait. ## Where the Rules Live diff --git a/GOVERNANCE.md b/GOVERNANCE.md index 980741df..5c35b9bb 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. @@ -220,6 +221,7 @@ The checks that separate work actually done from work that merely reports succes - **Never edit source through a shell heredoc when the text carries backslash escapes.** The shell consumes the escape and writes an invisible control character in its place, so a `\b` inside a regex becomes a backspace and the pattern silently matches nothing while every test still passes. Use a file-editing tool for such text. When a check inspects text for control characters, use `str.isprintable()` rather than a codepoint floor, since DEL and the Unicode format characters sit above 32 and are equally invisible in a diff. - **Never edit an active `.code-workspace` file.** A workspace file rewritten on disk can make VS Code reload the window, and a reload destroys the running agent session's context, so the work in flight is lost with nothing to catch it, and the trigger is not fully characterized (an agent's edit has caused the reload where a human's identical edit did not). Surface the needed change for the maintainer to apply by hand. - **A green check is not evidence the work happened.** A skipped job and a passing job are indistinguishable in the aggregated required check. When a job exists to exercise something, confirm from its log that it ran and produced the output it promises. +- **A launched process is not a result, and a cause nobody observed is not a diagnosis.** "The watcher is armed" names a process rather than a finding, so what gets reported is the output that process produced, and where it produced none, that absence is the report. The failure it prevents is an agent standing still on a condition that was met half an hour earlier, having announced the wait and never read it. Naming an external cause for such a stall afterwards, a throttle or a quota that appears nowhere in the record, turns a local defect into a story about someone else and closes the investigation on the wrong party, so read the record for the cause before naming one, and where the record does not carry it, report the cause as unknown. - **A workflow change is only fully exercised by CI.** Extracting a `run:` block and executing it locally validates the script and nothing else, because `secrets: inherit`, `permissions:`, `needs:` wiring, and reusable-workflow inputs resolve only in a real run. - **A review flags an instance, so fix the class.** When a reviewer cites one stale claim, one silent-narrowing pattern, or one mis-worded contract, sweep for its siblings before replying. Reviewers sample rather than enumerate. @@ -256,28 +258,47 @@ Drive the loop to green, meaning a review confirmed on the latest head SHA and e For provider-specific mechanics (how to request review, query review state, post replies, resolve threads), see the **GitHub Copilot Review Runbook** in [.github/copilot-instructions.md](./.github/copilot-instructions.md). This file owns the contract, and that file owns the mechanics. +### Every Finding Ends in an Action + +**A finding is closed by one of five outcomes, and a round count is never one of them.** The loop runs until no finding stands, however many rounds that takes, because the number of rounds measures how much was found rather than whether the work is done. A finding parked, waited out, or superseded by a push is still open. + +1. **It is real, so fix it.** Reply with the fixing commit SHA. +2. **It is not real, so disprove it in the thread**, with the command and its output, the code path that makes it impossible, or the rule that governs it. The proof is addressed to the reviewer as much as to the maintainer, since a decline it can read is what stops it raising the same thing next round. An assertion is not a proof and does not close a finding. +3. **It is real and deliberately not being fixed, which is the maintainer's call and not the agent's.** Say what the finding is, why the fix is unwanted, and get an explicit answer. Never suppress one by silence, by resolving the thread, or by an answer that reads as a decline while conceding the point. +4. **It is real and worth doing later, so file the issue first and reply with its link.** A deferral recorded only in a thread is lost the moment the pull request merges, so the issue is what carries it and the link is what proves it exists rather than being intended. This is for work the change did not create: an adjacent defect the reviewer noticed in passing, or a fix too large to ride along. It does not cover a defect in the change under review, because filing an issue about a bug you are about to merge is outcome 3 in other clothes, and that one is the maintainer's to decide. +5. **It keeps coming back, so fix the class rather than the instance.** A finding raised repeatedly against correct code is a defect in what the code communicates, not in the reviewer. Give it what it lacks: the non-obvious *why* as a comment where the code cannot state it, a clearer name, a narrower interface, or the rule change where the rule is what is wrong. A comment written for this earns its place under the comment rules like any other, so it states the why, stays short, and never cites a rule or addresses the reviewer. Making the noise stop is worth doing well, because a reviewer that repeats itself trains the reader to skim it, and skimming is how a real finding gets missed. + ### 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. - **Style/convention** - the comment cites a rule from this file or a language-specific style guide. Two cases: - The cited rule matches what the existing codebase already does -> fix the offending code. - - The cited rule contradicts what's in the tree, or industry norm -> **update the rule instead of the code**. The rule is wrong, not the code. Bouncing the same code across rounds is the symptom of a wrong rule. Heuristic: three rounds on the same style category means the rule needs adjusting and the user should authorize the rule change. + - The cited rule contradicts what's in the tree, or industry norm -> **update the rule instead of the code**. The rule is wrong, not the code. Bouncing the same code across rounds is the symptom of a wrong rule, so treat the recurrence itself as the finding and take it to the user for the rule change (outcome 5 above), rather than counting rounds until some threshold licenses it. - **Architectural opinion** - the comment proposes a different design ("constrain this to disabled-by-default", "move it elsewhere", "add a runtime guardrail"). This is judgment, not a bug. Surface it to the user with a recommendation, and don't apply it unilaterally. ### 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: - **Genuine design trade-off** surfaces (fail-open vs fail-closed, narrow vs broad refactor scope, "should we add a guardrail or trust the docstring"). Triage, recommend, ask. -- **Repeated friction** across rounds without convergence, which is the rule-needs-updating signal. Stop, summarize the pattern, and let the user authorize the rule change. +- **A recurring finding** the code keeps attracting, which is the fix-the-class signal. Summarize the pattern and bring the remedy, whether that is the rule change or what the code has to say differently to stop earning it. +- **A finding you judge real but do not want fixed**, which is outcome 3 above and is never the agent's call to make quietly. - **Architectural redesign** is requested rather than a bug fix. Surface with a recommendation, and never apply it unilaterally. Anti-pattern: don't keep flipping the code on the same style point. Flip the rule once and stick to the rule. diff --git a/TODO.md b/TODO.md index 47b192ea..ec6212f6 100644 --- a/TODO.md +++ b/TODO.md @@ -2,6 +2,7 @@ Running backlog for this repo, kept in a committed file so the guidance survives across environments where agent memory does not. +- Make agent-authored text use representative data rather than data observed in the maintainer's environment, and state it as a rule in [`GOVERNANCE.md`][governance] rather than leaving it to judgment. An agent illustrating a review finding on a downstream pull request quoted real paths from the maintainer's own filesystem, which carried family members' names, into a comment on a **public** repository. Nothing about the finding needed them. The rule to write covers every surface an agent authors, meaning pull request and issue comments, commit messages, code, tests, fixtures, and docs, and it reads roughly: illustrate with data you constructed, never with data you observed here. Three points the wording has to carry. **Synthetic evidence is better evidence**, not a weaker substitute, since a reader can re-run it: a filename constructed to contain a newline demonstrates the newline defect exactly, while a real photo library proves the same thing and can never be re-run by anyone else. **The exposure is one-way**, because a public comment is fetched, cached, and indexed the moment it posts, so an edit afterwards is mitigation rather than a fix, and the maintainer decides what to do about one that has already landed. And **no checker closes this.** A pattern can find an absolute home path or a drive letter, and that subset is worth gating in [`prose_lint.py`][prose-lint] as a floor, but the data that actually leaked here was name-shaped, and a name is not pattern-detectable. A grep over the offending pull request for path-shaped strings returns nothing while the names sit in plain sight, so the gate has to be understood as catching the easy half only. Scope this fleet-wide rather than to the hub, since every repo in the fleet is public and each one is worked by agents that read the same carried rules. - Populate [reports/][reports] for the cataloged repos that still have no audit, since a registry `status` of `cataloged` asserts a result that only a committed report evidences. Eight repos have one. This is paced by maintainer capacity rather than blocked on anything, since repos are brought up to spec as they are worked on, so the entry records the outstanding set rather than a defect. - Revisit automating the audit, which was explored and deliberately deferred, recorded here so the reasoning is not re-derived from scratch. Three shapes were considered: a scheduled hub-driven audit publishing each report as a workflow artifact, the same thing committing the report back the way the codegen bot updates its own files, and a pull-request hook in each downstream repo that audits itself against the current hub. Three things blocked all of them. Until the fleet reaches stasis with every repo onboarded, a scheduled run reports mostly noise, since a repo mid-onboarding is expected to be non-conformant. The hub has to be stable before downstreams can audit against it, because a hub change lands as fleet-wide findings the same day. And the downstream half is a catch-22, since a self-auditing pull-request hook is CI instrumentation the repos that most need it do not yet carry. The agreed outcome was the on-demand audit that [`AUDIT.md`][audit-doc] describes today. Worth reopening once the fleet is onboarded and the hub goes a stretch without carried-content changes, and the artifact shape is the one to try first, since it produces evidence without committing anything and so cannot generate review load while the noise level is still unknown. - Canonicalize Python linter-config placement on `pyproject.toml` (one cataloged repo uses standalone `.ruff.toml` + `pyrightconfig.json`), track as a drift finding, fix downstream. @@ -17,16 +18,28 @@ Running backlog for this repo, kept in a committed file so the guidance survives - Rework [`spec/readme-structure.md`][readme-structure] to match the hand-crafted PlexCleaner README, which is the shape the maintainer wants, and make the result auditable rather than advisory. Four concrete divergences are already identified, measured against PlexCleaner `README.md`, this repo's `README.md`, and the current spec. First, the distribution bullet is labeled by deliverable: PlexCleaner ships executables and calls the channel **Binary Releases**, while the spec fixes the label as **Versioned Releases** for every repo, so the label belongs in a per-channel table rather than as one string. Second, the license shield sits in the top **Build Status** block here and at the very bottom of PlexCleaner, inside a closing `## License` section that reads `Licensed under the [MIT License]` followed by the shield, immediately before the link definitions. Third, the Release Notes section closes with `See Release History for complete release notes and older versions.` in PlexCleaner against `See Release History for the full history.` here, and the PlexCleaner form is the wanted one. Note that PlexCleaner writes that link inline, which the reference-style rule forbids, so adopt the wording and keep the reference form. Fourth, the channel bullets and their shields vary by deliverable, meaning GitHub binaries, Docker Hub, NuGet, and PyPI each carry a different bullet label and a different shield set, which is what a per-type table has to encode for the `readme-structure` audit dimension to check a repo against its own declared types. - 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. +- Re-vendor the changed `verbatim` content across the fleet, which is one sweep covering three files. `repo-config/configure.sh` is carried `verbatim` with `appliesTo: "*"` and the hub swept it to one sentence per line. `AGENTS.md` "Context and Delegation Discipline" carries the wait rule's failure clause, and `GOVERNANCE.md` "Verification Discipline" carries the rule that a launched process is not a result. Every repo already holding a copy is byte-mismatched against the hub until it takes the new one, which the audit reports as stale rather than modified. +- Measure review rounds against pull request size, and decide what the number licenses. The recent loops suggest a large change earns a different finding every round while a small one converges in one or two, which would make change size the lever on review cost rather than the reviewer's thoroughness, and would argue for splitting a change before review rather than discovering it through five rounds of findings. The data needs no new instrumentation, since the review history already carries it: for each recent pull request, record the diff size in files and lines, the number of rounds, and the findings per round, counting suppressed findings alongside threaded ones because they are the majority of what these loops produce. The outcome worth having is a threshold [`GOVERNANCE.md`][governance] can state in the branching or review guidance, expressed as the size at which a change is split rather than as advice to keep changes small. Note two confounds before drawing a line from the numbers. A large change is usually also a novel one, so size and unfamiliarity move together and the record should note what kind of change each was. And a round that finds something new is not evidence of a problem by itself, since a round that finds something new is the reviewer working, so the metric to watch is findings that a smaller first cut would have surfaced earlier rather than findings per round on its own. +- Decide where a carried file may name hub-only machinery, since `GOVERNANCE.md` "PR Review Etiquette" points at `scripts/pr_review.py` and the fleet carries the section but not the script. A downstream reader follows that pointer to a path their repo does not have. Either the script joins the carried set, or the rule states the behavior and drops the tool name the way the coordination-reference rule already requires for the template repo itself. +- Make [`prose_lint.py`][prose-lint] assert a floor on its own scope, applying to itself the rule [`GOVERNANCE.md`][governance] already states: a gate that finds nothing is indistinguishable from a gate with nothing to find. A `--diff` run that resolves a non-empty diff and then matches **zero** files has almost certainly failed to scope rather than found a clean change, so it should say so instead of exiting 0. One session produced four separate routes to that same false clean: an unresolvable base widening to a whole-tree scan, a multi-line `paths` input read only to its first newline, a diff taken in one repository while scanning another, and a path under no repository at all. Each was fixed with its own guard, which is the wrong shape, because the fifth route will need a fifth guard and will be found the same way the first four were, by a reviewer rather than by the gate. A floor assertion covers the family. Note the honest limit before building it: a change touching only files the gate does not read (an image, a lock file) legitimately scopes to zero, so the assertion compares against the diff's own file list rather than against zero alone. +- 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. +- 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 +[prose-lint]: ./scripts/prose_lint.py [readme-structure]: ./spec/readme-structure.md +[repo-gate]: ./scripts/repo_gate.py +[scripts]: ./scripts/README.md [reports]: ./reports/ [repos]: ./registry/repos.json [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..dd555d23 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,19 @@ 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. +`wait` exits `40` when Copilot answers the request with a plain comment rather than a review, meaning a comment of its own that postdates its newest review on the pull request. The test is the **shape** of that answer and not its cause, which the script reads nothing of: a comment carries no commit, so it satisfies no coverage check whatever it says, and a wait reading formal reviews alone treats it as an unmet condition and then polls out its whole timeout against an answer that already arrived. A refusal is the case that makes this worth catching, a quota or rate-limit message among them, and `40` neither asserts nor detects one. The comment prints whole because its wording is the only thing separating a refusal, which is terminal since no review follows it and re-requesting does not clear it, from an ordinary remark that is not, so `40` ends the wait and hands the text to the reader who can tell them apart. A comment **older** than the newest review is spent rather than terminal, because the review it preceded did land. Every connection reads the newest `WINDOW` nodes rather than the reviewer's own, since GraphQL offers no author filter, so ordinary traffic is what pushes theirs out of reach. `window_blind` is the one guard over both sides, and each side fails differently. Blind on **comments** means an answer could be back there unseen, which reads as `answered_outside_review=unknown` rather than `no`. Blind on **reviews** is worse, because the newest review in view is then not the newest there is, and an empty baseline dates every comment as newer so each one reads as an answer: a false `40` that stops the loop on a pull request whose review actually landed. That case reports nothing and lets the wait keep polling, since a wait that runs on is visible where a wrong terminal is not. + +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. + +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. 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..09ef7390 100644 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -11,7 +11,12 @@ 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). + Exit 0 = review present, 30 = still pending at timeout (pending is not failure), + 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. + 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. Read-only by design. Mutations (re-request review, reply, resolve thread) are deliberately NOT implemented here - they are state-changing calls that must stay @@ -24,22 +29,43 @@ 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. +# Those appear nowhere in `reviewThreads`, so polling threads alone reports a clean pass. +# 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) DETAILS = re.compile(r'
(.*?)
', re.DOTALL | re.IGNORECASE) SUMMARY = re.compile(r'(.*?)', re.DOTALL | re.IGNORECASE) TAGS = re.compile(r'', re.IGNORECASE) COUNT = re.compile(r'\((\d+)\)') -# Liveness query: two scalars only, no comment bodies. +# How many of the newest reviews and comments both queries read. +# A narrow window drops the reviewer's answer behind ordinary discussion, reporting no answer. +# A test holds this equal to the number the queries carry, since a drift between them reads clean. +WINDOW = 100 + +# The timeline spells the reviewer a third way, as login `Copilot` with type `Bot`. +# GraphQL says `copilot-pull-request-reviewer`, and REST user objects add a `[bot]` suffix. +# The predicate is the type plus a loose login match rather than any one spelling. +# Requests are the reviewer's own, since a human requested later is a different request. +# Reading one as the newest reports a picked-up review as never picked up. +TIMELINE_JQ = ( + '.[] | select(.event == "copilot_work_started" or (.event == "review_requested"' + ' and .requested_reviewer.type == "Bot"' + ' and ((.requested_reviewer.login // "") | ascii_downcase | test("copilot"))))' + ' | "\\(.event) \\(.created_at)"' +) + +# Liveness query: timestamps and ids only, no comment or review bodies. # A liveness check does not need the finding text, and re-fetching bodies was 76% of polls. +# It does need the reviewer's non-review answers. +# A wait reading formal reviews alone treats a refusal as an unmet condition. Q_LIVE = """ query($o:String!,$r:String!,$n:Int!){ repository(owner:$o,name:$r){ pullRequest(number:$n){ headRefOid - reviews(last:20){ nodes{ author{login} state commit{oid} } } + reviews(last:100){ nodes{ author{login} state commit{oid} submittedAt } pageInfo{ hasPreviousPage } } + comments(last:100){ nodes{ author{login} createdAt } pageInfo{ hasPreviousPage } } + reviewRequests(first:10){ nodes{ requestedReviewer{ __typename ... on Bot{login} ... on User{login} } } } }}} """ @@ -48,9 +74,11 @@ 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 body } } + reviews(last:100){ nodes{ author{login} state commit{oid} submittedAt body } pageInfo{ hasPreviousPage } } reviewThreads(first:100){ nodes{ id isResolved comments(first:1){ nodes{ author{login} path line body } } }} + comments(last:100){ nodes{ author{login} createdAt body } pageInfo{ hasPreviousPage } } + reviewRequests(first:10){ nodes{ requestedReviewer{ __typename ... on Bot{login} ... on User{login} } } } }}} """ @@ -66,14 +94,116 @@ def gql(query: str, owner: str, repo: str, num: int) -> dict: return json.loads(r.stdout)['data']['repository']['pullRequest'] -def live_state(owner: str, repo: str, num: int) -> tuple[str, bool]: - """Return (head_sha, copilot_reviewed_current_head).""" - pr = gql(Q_LIVE, owner, repo, num) +def timeline(owner: str, repo: str, num: int) -> list[tuple[str, str]]: + """The request and pickup events, oldest first, as (event, timestamp). + + GraphQL carries no `copilot_work_started`, so this is the one REST reader here. + `--jq` projects inside gh rather than after it, since `--paginate` without one emits a + concatenated array per page that is not valid JSON on every gh a fleet machine may carry. + `per_page` is the page size the pagination actually costs, and the default of 30 turns a + long-running pull request into six requests a reading where the maximum makes it two. + """ + r = subprocess.run( + ['gh', 'api', '--paginate', f'repos/{owner}/{repo}/issues/{num}/timeline?per_page=100', + '--jq', TIMELINE_JQ], + capture_output=True, text=True) + if r.returncode != 0: + sys.stderr.write(r.stderr[:800]) + raise SystemExit(f'gh timeline failed rc={r.returncode}') + return [(ln.split(' ', 1)[0], ln.split(' ', 1)[1]) + for ln in r.stdout.splitlines() if ' ' in ln] + + +def never_picked_up(events: list[tuple[str, str]]) -> str: + """The newest request's timestamp where no pickup followed it, otherwise the empty string. + + A request the reviewer accepts raises `copilot_work_started` within about half a minute, so + a request with no pickup after it is not a slow review, it is a request nothing is acting on. + The two states look identical from the reviews alone, which is how one sat for thirteen hours + reading as pending. Elapsed time cannot separate them either, since a genuinely slow round + also produces no review, and only the pickup event says whether anything is working. + """ + requested = [t for e, t in events if e == 'review_requested'] + started = [t for e, t in events if e == 'copilot_work_started'] + if not requested: + return '' + newest = max(requested) + return '' if any(t >= newest for t in started) else newest + + +def reviewer_requested(pr: dict) -> bool: + """True where the reviewer sits in the pending request set. + + Read from GraphQL rather than `gh pr view --json reviewRequests`, which omits a Bot + reviewer entirely and reports an empty set while the reviewer is sitting in it. + """ + return any((n.get('requestedReviewer') or {}).get('login') == REVIEWER + for n in ((pr.get('reviewRequests') or {}).get('nodes') or [])) + + +def reviewer_nodes(pr: dict, field: str) -> list[dict]: + """The reviewer's own nodes under `field`, oldest first as the API returns them.""" + return [n for n in ((pr.get(field) or {}).get('nodes') or []) + if (n.get('author') or {}).get('login') == REVIEWER] + + +def answered_outside_review(pr: dict) -> dict | None: + """The reviewer's newest plain comment, where it postdates its newest formal review. + + The test is the shape of the answer rather than its cause, which this reads nothing of: + a comment carries no commit, so it satisfies no coverage check whatever it says. + Treating that shape as an unmet condition is what leaves a wait with nothing at its end. + A comment older than the newest review is spent, since the review it preceded did land. + """ + comments = reviewer_nodes(pr, 'comments') + # A blind review window leaves no honest baseline to date a comment against. + # An empty one dates every comment as newer, so each reads as an answer. + # Reporting nothing keeps the wait polling, where a wrong answer ends it outright. + if not comments or window_blind(pr, 'reviews'): + return None + newest = max(comments, key=lambda n: n.get('createdAt') or '') + reviews = reviewer_nodes(pr, 'reviews') + latest_review = max((n.get('submittedAt') or '' for n in reviews), default='') + return newest if (newest.get('createdAt') or '') > latest_review else None + + +def window_blind(pr: dict, field: str) -> bool: + """True where the reviewer's own nodes can sit behind the window, so the view cannot decide. + + Each query reads the newest nodes rather than the reviewer's, so ordinary traffic is what + pushes theirs out of reach. Nodes arrive in creation order, so anything behind the window is + older than everything inside it: one of the reviewer's in view bounds every hidden one as + older still, which settles the question rather than leaving it open. `hasPreviousPage` is + what says anything is back there at all, since a full window and a window holding the lot + are the same length. + """ + older = ((pr.get(field) or {}).get('pageInfo') or {}).get('hasPreviousPage') + return bool(older) and not reviewer_nodes(pr, field) + + +def stall_of(owner: str, repo: str, num: int, pr: dict) -> str: + """The stalled request's timestamp for this payload, or the empty string where none. + + Derived from the payload it is reported beside, since a stall read earlier describes a + pull request that has since moved: a request picked up after the reading still reports as + picked up by nothing. A covered head or no pending request settles it without a REST call. + """ + if reviewed_head(pr) or not reviewer_requested(pr): + return '' + return never_picked_up(timeline(owner, repo, num)) + + +def reviewed_head(pr: dict) -> bool: + """True where one of the reviewer's own reviews carries the current head's commit.""" head = pr['headRefOid'] - done = any((n.get('author') or {}).get('login') == REVIEWER - and (n.get('commit') or {}).get('oid') == head - for n in pr['reviews']['nodes']) - return head, done + return any((n.get('commit') or {}).get('oid') == head + for n in reviewer_nodes(pr, 'reviews')) + + +def live_state(owner: str, repo: str, num: int) -> tuple[str, bool, dict | None]: + """Return (head_sha, copilot_reviewed_current_head, copilot_answer_outside_a_review).""" + pr = gql(Q_LIVE, owner, repo, num) + return pr['headRefOid'], reviewed_head(pr), answered_outside_review(pr) def heading_of(block: str) -> str: @@ -107,28 +237,66 @@ def finding_count(block: str) -> int: 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) +def digest(owner: str, repo: str, num: int, seen: set[str] | None = None, + pr: dict | None = None, stalled: str | None = None) -> tuple[str, int]: + """Render the digest, from a caller's payload and stall reading where those are given. + + The caller passes its own readings when the exit code has to agree with what was printed, + since a review landing between two reads makes a fresh fetch describe a different pull + request than the one the code was decided from. Passing the stall also spends one REST + call between the caller and the digest rather than one each. + """ + pr = gql(Q_FULL, owner, repo, num) if pr is None else pr + stalled = stall_of(owner, repo, num, pr) if stalled is None else stalled head = pr['headRefOid'] - revs = [n for n in pr['reviews']['nodes'] - if (n.get('author') or {}).get('login') == REVIEWER] + revs = reviewer_nodes(pr, 'reviews') on_head = [n for n in revs if (n.get('commit') or {}).get('oid') == head] 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. unresolved = [t for t in threads if not t['isResolved'] - and ((t.get('comments') or {}).get('nodes') or [{}])[0] - .get('author', {}).get('login') == REVIEWER] + and ((((t.get('comments') or {}).get('nodes') or [{}])[0] + .get('author') or {}).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) + answer = answered_outside_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 = [ 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'answered_outside_review={answered} ' + f'requested={"yes" if reviewer_requested(pr) else "no"} ' f'merge={pr.get("mergeStateStatus")}' ] + 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') + if blind: + lines.append(f' BEHIND THE WINDOW ({" and ".join(blind)}): the newest {WINDOW} carry ' + 'none from the reviewer and older ones exist, so this cannot decide') + if answer: + # Printed whole for the same reason a suppressed finding is, since it reaches no thread. + # Its wording is the only thing separating a refusal from an ordinary remark. + lines.append(f' COPILOT COMMENT ({answer.get("createdAt")}, newer than any review): ' + 'the reviewer answered without reviewing, so read the body below and ' + 'decide, since a refusal is terminal and a remark is not') + lines += [f' {ln.rstrip()}' for ln in (answer.get('body') or '').splitlines() + if ln.strip()] new = 0 for t in unresolved: c = (t.get('comments') or {}).get('nodes', [{}])[0] @@ -142,10 +310,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: - # 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') + for n, b in blocks: + # Printed whole where a thread body is truncated, since a thread can be re-read at its id. + # A suppressed finding has none, so this digest is the only place it appears. + # 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: @@ -159,7 +337,13 @@ def main(argv: list[str] | None = None) -> int: ap.add_argument('number', type=int) ap.add_argument('--repo', default='ptr727/ProjectTemplate') ap.add_argument('--timeout', type=int, default=2700, help='seconds (default 45m)') + ap.add_argument('--pickup-grace', type=int, default=300, + help='seconds before the first pickup read, and between reads (default 5m)') a = ap.parse_args(argv) + # A negative grace leaves the next reading permanently behind the clock. + # That is the per-poll REST pattern the interval exists to prevent. + if a.pickup_grace < 0: + ap.error('--pickup-grace cannot be negative') owner, repo = a.repo.split('/', 1) if a.cmd == 'status': @@ -170,21 +354,57 @@ def main(argv: list[str] | None = None) -> int: # In-process backoff, so the whole wait costs one agent turn. delays = [15, 20, 30, 45, 60, 120] start = time.monotonic() - head0, done = live_state(owner, repo, a.number) + pr = gql(Q_LIVE, owner, repo, a.number) + done, answer = reviewed_head(pr), answered_outside_review(pr) + stalled = '' i = 0 - while not done: - if time.monotonic() - start > a.timeout: - print(f'pr={a.number} head={head0[:8]} review_on_head=NO ' - f'status=PENDING waited={int(time.monotonic()-start)}s') - return 30 + next_pickup = a.pickup_grace + while not done and not answer: + elapsed = time.monotonic() - start + # Read the pickup before the clock, so a request nothing acted on reports as itself. + # Running the clock out instead would report it exactly as a slow reviewer. + # The read costs a second call over REST, so it runs on its own interval, not per poll. + # One reading settles the current request, and the next covers a request a push raises. + if elapsed > next_pickup and reviewer_requested(pr): + next_pickup = elapsed + a.pickup_grace + stalled = never_picked_up(timeline(owner, repo, a.number)) + if stalled: + break + if elapsed > a.timeout: + break time.sleep(delays[min(i, len(delays) - 1)]) i += 1 # Re-read head each iteration: a push during the wait moves it. - head0, done = live_state(owner, repo, a.number) - out, _ = digest(owner, repo, a.number) + pr = gql(Q_LIVE, owner, repo, a.number) + done, answer = reviewed_head(pr), answered_outside_review(pr) + + # One payload decides the digest and the exit code together. + # Read separately, a review landing between them prints coverage and returns a stalled code. + # A reader resolves that by believing the code, dropping the review it was just shown. + # The digest also earns its call at the timeout. + # A bare PENDING line reports a broken wait and a slow reviewer identically. + final = gql(Q_FULL, owner, repo, a.number) + # The stall is re-read here rather than carried out of the loop. + # A request picked up since that reading would still report as picked up by nothing. + stalled = stall_of(owner, repo, a.number, final) + out, _ = digest(owner, repo, a.number, pr=final, stalled=stalled) print(out) print(f'waited={int(time.monotonic()-start)}s') - return 0 + if reviewed_head(final): + return 0 + # 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, ' + 'so read the comment above and decide, since where it declines or names a limit ' + 'no review follows and re-requesting does not clear it') + return 40 + if stalled: + print(f'status=REQUEST_NOT_PICKED_UP requested {stalled} and no copilot_work_started ' + 'followed it, so nothing is working on this and waiting on will not start it: ' + 'clear the request and re-request, per the runbook') + return 50 + print('status=PENDING') + return 30 if __name__ == '__main__': 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..8e563e28 100644 --- a/scripts/test_pr_review.py +++ b/scripts/test_pr_review.py @@ -9,7 +9,8 @@ Run as `python3 scripts/test_pr_review.py`, or under `python3 -m unittest discover -s scripts`. """ from __future__ import annotations -import contextlib, io, json, subprocess, sys, unittest +import contextlib, io, json, re, subprocess, sys, unittest +from itertools import count from pathlib import Path from unittest import mock @@ -22,9 +23,19 @@ OLD = 'b' * 40 -def review(login: str = pr_review.REVIEWER, oid: str = HEAD, body: str = '') -> dict: +EARLY = '2026-08-02T10:00:00Z' +LATE = '2026-08-02T11:00:00Z' + + +def review(login: str = pr_review.REVIEWER, oid: str = HEAD, body: str = '', + at: str = EARLY) -> dict: return {'author': {'login': login}, 'state': 'COMMENTED', 'commit': {'oid': oid}, - 'body': body} + 'body': body, 'submittedAt': at} + + +def comment(login: str = pr_review.REVIEWER, at: str = LATE, + body: str = 'I have reached my quota limit and cannot review this now.') -> dict: + return {'author': {'login': login}, 'createdAt': at, 'body': body} def collapsed(heading: str = 'Comments suppressed due to low confidence (1)', @@ -41,9 +52,15 @@ def thread(tid: str, resolved: bool = False, login: str = pr_review.REVIEWER, def payload(reviews: list[dict], threads: list[dict] | None = None, - merge: str = 'CLEAN') -> dict: + merge: str = 'CLEAN', comments: list[dict] | None = None, + older: bool = False, older_reviews: bool = False, pending: bool = False) -> dict: + requested = ([{'requestedReviewer': {'__typename': 'Bot', 'login': pr_review.REVIEWER}}] + if pending else []) return {'headRefOid': HEAD, 'mergeable': 'MERGEABLE', 'mergeStateStatus': merge, - 'reviews': {'nodes': reviews}, 'reviewThreads': {'nodes': threads or []}} + 'reviews': {'nodes': reviews, 'pageInfo': {'hasPreviousPage': older_reviews}}, + 'reviewThreads': {'nodes': threads or []}, + 'comments': {'nodes': comments or [], 'pageInfo': {'hasPreviousPage': older}}, + 'reviewRequests': {'nodes': requested}} class GqlCase(unittest.TestCase): @@ -71,12 +88,124 @@ def test_the_review_must_be_the_reviewer_and_on_the_current_head(self) -> None: ): with self.subTest(case=label): self.answer(payload(reviews)) - self.assertEqual((HEAD, want), pr_review.live_state('o', 'r', 1)) + self.assertEqual((HEAD, want, None), pr_review.live_state('o', 'r', 1)) def test_a_null_author_or_commit_does_not_raise(self) -> None: """GraphQL returns null for a deleted account, and a crash there stalls the whole wait.""" self.answer(payload([{'author': None, 'state': 'COMMENTED', 'commit': None}])) - self.assertEqual((HEAD, False), pr_review.live_state('o', 'r', 1)) + self.assertEqual((HEAD, False, None), pr_review.live_state('o', 'r', 1)) + + +class TestAnsweredOutsideReview(unittest.TestCase): + """A refusal answers the request without covering the head, so a wait cannot read it as pending.""" + + def test_a_reviewer_comment_newer_than_every_review_is_the_answer(self) -> None: + answer = pr_review.answered_outside_review( + payload([review(oid=OLD, at=EARLY)], comments=[comment(at=LATE)])) + self.assertIsNotNone(answer) + self.assertEqual(LATE, (answer or {}).get('createdAt')) + + def test_an_answer_the_reviewer_then_superseded_is_spent(self) -> None: + """The review it preceded did land, so the comment is history rather than a stop signal.""" + self.assertIsNone(pr_review.answered_outside_review( + payload([review(at=LATE)], comments=[comment(at=EARLY)]))) + + def test_another_account_s_comment_is_not_the_reviewer_answering(self) -> None: + """A maintainer note and a codecov post both postdate the review and mean nothing here.""" + for login in ('ptr727', 'codecov[bot]', 'copilot-swe-agent'): + with self.subTest(login=login): + self.assertIsNone(pr_review.answered_outside_review( + payload([review(oid=OLD)], comments=[comment(login=login)]))) + + def test_no_comments_at_all_reads_as_no_answer(self) -> None: + self.assertIsNone(pr_review.answered_outside_review(payload([review(oid=OLD)]))) + + def test_ordinary_discussion_does_not_push_the_answer_out_of_the_window(self) -> None: + """The window reads the newest comments, not the reviewer's, so others crowd it.""" + chatter = [comment(login='ptr727', at=LATE) for _ in range(pr_review.WINDOW - 1)] + found = pr_review.answered_outside_review( + payload([review(oid=OLD, at=EARLY)], comments=[comment(at=LATE)] + chatter)) + self.assertIsNotNone(found) + + def test_comments_behind_the_window_are_unknown_rather_than_no_answer(self) -> None: + """Finding nothing and having nothing to find are one reading once an answer can hide.""" + full = [comment(login='ptr727') for _ in range(pr_review.WINDOW)] + self.assertTrue( + pr_review.window_blind(payload([review()], comments=full, older=True), 'comments')) + + def test_a_window_holding_every_comment_is_not_a_gap(self) -> None: + """A full window and a window holding the lot are the same length, so length cannot say.""" + full = [comment(login='ptr727') for _ in range(pr_review.WINDOW)] + self.assertFalse( + pr_review.window_blind(payload([review()], comments=full, older=False), 'comments')) + + def test_reviews_behind_the_window_report_nothing_rather_than_a_false_answer(self) -> None: + """No reviewer review in view dates every comment as newer, so each reads as an answer. + + Reporting nothing keeps the wait polling, where a wrong answer ends it outright on a + pull request whose review landed and simply sits behind a busier review history. + """ + pr = payload([review(login='ptr727') for _ in range(pr_review.WINDOW)], + comments=[comment(at=LATE)], older_reviews=True) + self.assertTrue(pr_review.window_blind(pr, 'reviews')) + self.assertIsNone(pr_review.answered_outside_review(pr)) + + def test_one_reviewer_review_in_view_is_a_baseline_the_answer_can_be_dated_against(self) -> None: + """Reviews arrive in creation order too, so a hidden one is older than the one in view.""" + pr = payload([review(at=EARLY, oid=OLD)] + + [review(login='ptr727') for _ in range(pr_review.WINDOW - 1)], + comments=[comment(at=LATE)], older_reviews=True) + self.assertFalse(pr_review.window_blind(pr, 'reviews')) + self.assertIsNotNone(pr_review.answered_outside_review(pr)) + + def test_one_spent_reviewer_comment_in_view_settles_the_question(self) -> None: + """Comments arrive in creation order, so a hidden one is older than the spent one in view.""" + full = ([comment(at=EARLY)] + + [comment(login='ptr727') for _ in range(pr_review.WINDOW - 1)]) + pr = payload([review(at=LATE)], comments=full, older=True) + self.assertIsNone(pr_review.answered_outside_review(pr)) + self.assertFalse(pr_review.window_blind(pr, 'comments')) + + +class TestPickup(unittest.TestCase): + """A request nothing acted on and a review being worked on are one reading from the reviews.""" + + def test_a_request_with_no_pickup_after_it_is_named_by_its_timestamp(self) -> None: + """The shape that sat thirteen hours reading as pending: requested, never started.""" + events = [('review_requested', '2026-08-02T22:58:15Z'), + ('copilot_work_started', '2026-08-02T22:58:45Z'), + ('review_requested', '2026-08-03T00:15:00Z')] + self.assertEqual('2026-08-03T00:15:00Z', pr_review.never_picked_up(events)) + + def test_a_request_the_reviewer_took_up_is_not_stalled(self) -> None: + """Slow is not stuck, and only the pickup event tells them apart.""" + events = [('review_requested', '2026-08-03T13:09:19Z'), + ('copilot_work_started', '2026-08-03T13:09:54Z')] + self.assertEqual('', pr_review.never_picked_up(events)) + + def test_an_earlier_pickup_does_not_cover_a_later_request(self) -> None: + """Answering the last request is not answering this one, and order is what says so.""" + events = [('copilot_work_started', '2026-08-02T22:58:45Z'), + ('review_requested', '2026-08-02T23:31:44Z')] + self.assertEqual('2026-08-02T23:31:44Z', pr_review.never_picked_up(events)) + + def test_no_request_at_all_is_not_a_stall(self) -> None: + self.assertEqual('', pr_review.never_picked_up( + [('copilot_work_started', '2026-08-02T22:58:45Z')])) + self.assertEqual('', pr_review.never_picked_up([])) + + def test_the_pending_set_is_read_where_a_bot_reviewer_is_visible(self) -> None: + """`gh pr view --json reviewRequests` omits a Bot outright and reports an empty set.""" + pending = {'reviewRequests': {'nodes': [ + {'requestedReviewer': {'__typename': 'Bot', 'login': pr_review.REVIEWER}}]}} + self.assertTrue(pr_review.reviewer_requested(pending)) + human = {'reviewRequests': {'nodes': [ + {'requestedReviewer': {'__typename': 'User', 'login': 'ptr727'}}]}} + self.assertFalse(pr_review.reviewer_requested(human)) + self.assertFalse(pr_review.reviewer_requested({'reviewRequests': {'nodes': []}})) + # A null reviewer is what a deleted account leaves behind, and it must not raise. + self.assertFalse(pr_review.reviewer_requested( + {'reviewRequests': {'nodes': [{'requestedReviewer': None}]}})) class TestDigest(GqlCase): @@ -99,6 +228,15 @@ def test_review_on_head_reports_no_when_every_round_is_stale(self) -> None: out, _ = pr_review.digest('o', 'r', 7) self.assertIn('review_on_head=NO', out) + def test_a_thread_from_a_deleted_account_does_not_crash_the_digest(self) -> None: + """GraphQL sends `author` present and null, which a defaulted lookup returns as None.""" + orphan = thread('T1') + orphan['comments']['nodes'][0]['author'] = None + self.answer(payload([review()], [orphan, thread('T2')])) + out, unresolved = pr_review.digest('o', 'r', 7) + self.assertEqual(1, unresolved) + self.assertIn('T2', out) + def test_only_the_reviewer_s_own_unresolved_threads_are_listed(self) -> None: """A maintainer's own open thread is not a review finding to answer.""" self.answer(payload([review()], [thread('T1', login='ptr727'), thread('T2')])) @@ -148,19 +286,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) @@ -206,6 +367,32 @@ def test_a_human_review_carrying_the_phrase_is_not_a_copilot_finding(self) -> No self.assertIn('suppressed=0', 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.""" + text = 'Copilot has reached its quota limit.\nTry again after the window resets.' + self.answer(payload([review(oid=OLD)], comments=[comment(body=text)])) + out, _ = pr_review.digest('o', 'r', 7) + self.assertIn('answered_outside_review=yes', out) + self.assertIn('COPILOT COMMENT', out) + for line in text.splitlines(): + self.assertIn(line, out) + + def test_a_pull_request_with_no_such_answer_says_so_rather_than_staying_silent(self) -> None: + self.answer(payload([review()])) + out, _ = pr_review.digest('o', 'r', 7) + self.assertIn('answered_outside_review=no', out) + + def test_an_unreadable_window_reports_unknown_and_names_why(self) -> None: + """Reporting `no` off a window an answer can hide behind is the false clean to avoid.""" + self.answer(payload([review()], older=True, + comments=[comment(login='ptr727') + for _ in range(pr_review.WINDOW)])) + out, _ = pr_review.digest('o', 'r', 7) + self.assertIn('answered_outside_review=unknown', out) + self.assertIn('BEHIND THE WINDOW (comments)', out) + + class TestGqlTransport(unittest.TestCase): def test_a_failed_call_raises_rather_than_returning_an_empty_reading(self) -> None: """Returning nothing on failure would read as a PR with no reviews and no threads.""" @@ -256,6 +443,134 @@ def test_wait_exits_thirty_at_the_timeout_rather_than_reporting_success(self) -> self.assertEqual(30, pr_review.main(['wait', '7', '--timeout', '0'])) self.assertIn('status=PENDING', self.out.getvalue()) + def test_the_timeout_carries_the_digest_rather_than_a_bare_pending_line(self) -> None: + """A wait that ends with no evidence reports a slow reviewer and a broken poll alike.""" + self.answer(payload([review(oid=OLD)], [thread('T1')])) + with mock.patch.object(pr_review.time, 'sleep'): + self.assertEqual(30, pr_review.main(['wait', '7', '--timeout', '0'])) + out = self.out.getvalue() + self.assertIn('review_on_head=NO', out) + self.assertIn('unresolved=1', out) + + def test_wait_ends_on_an_answer_outside_a_review_instead_of_waiting_it_out(self) -> None: + """A refusal covers no head, so polling on for the timeout waits for nothing. + + The zero timeout is what this case fails on rather than hangs on: an answer read as + pending spins the loop for the whole default wait, and a case that hangs gates nothing. + """ + self.answer(payload([review(oid=OLD)], comments=[comment()])) + with mock.patch.object(pr_review.time, 'sleep') as slept: + self.assertEqual(40, pr_review.main(['wait', '7', '--timeout', '0'])) + slept.assert_not_called() + out = self.out.getvalue() + self.assertIn('status=ANSWERED_OUTSIDE_REVIEW', out) + self.assertIn('quota', out) + + 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)])) + with mock.patch.object(pr_review.time, 'sleep'): + self.assertEqual(0, pr_review.main(['wait', '7'])) + + def test_wait_stops_on_a_request_nothing_picked_up(self) -> None: + """Waiting on cannot start a request nothing is acting on, so the wait says so and ends. + + The zero timeout is what this fails on rather than hangs on, and it also pins the order: + the pickup is read before the clock, so the stall reports as itself instead of as PENDING. + """ + self.answer(payload([review(oid=OLD)], pending=True)) + with mock.patch.object(pr_review, 'timeline', + return_value=[('review_requested', LATE)]), \ + mock.patch.object(pr_review.time, 'sleep'): + self.assertEqual(50, pr_review.main( + ['wait', '7', '--pickup-grace', '0', '--timeout', '0'])) + out = self.out.getvalue() + self.assertIn('status=REQUEST_NOT_PICKED_UP', out) + self.assertIn(LATE, out) + + def test_a_request_being_worked_on_is_not_stopped_on(self) -> None: + """A slow round is the case the grace exists for, and stopping on it loses the review.""" + self.answer(payload([review(oid=OLD)], pending=True), payload([review()], pending=True)) + with mock.patch.object(pr_review, 'timeline', + return_value=[('review_requested', EARLY), + ('copilot_work_started', LATE)]), \ + mock.patch.object(pr_review.time, 'sleep'): + self.assertEqual(0, pr_review.main( + ['wait', '7', '--pickup-grace', '0', '--timeout', '600'])) + + def test_the_pickup_read_waits_out_the_grace_rather_than_running_per_poll(self) -> None: + """It costs a second call, and inside the grace a pending request is just work in flight.""" + self.answer(payload([review(oid=OLD)], pending=True), payload([review()], pending=True)) + with mock.patch.object(pr_review, 'timeline', return_value=[]) as seen, \ + mock.patch.object(pr_review.time, 'sleep'): + self.assertEqual(0, pr_review.main( + ['wait', '7', '--pickup-grace', '9999', '--timeout', '600'])) + seen.assert_not_called() + + def test_the_pickup_read_runs_on_its_own_interval_once_the_grace_is_out(self) -> None: + """Every poll past the grace is what the comment ruled out and the code did anyway. + + The clock advances a fixed step per reading, so the interval is counted rather than + waited: a long wait must not turn one REST reader into one per poll. + """ + picked_up = [('review_requested', EARLY), ('copilot_work_started', LATE)] + self.answer(payload([review(oid=OLD)], pending=True)) + with mock.patch.object(pr_review.time, 'monotonic', side_effect=count(0, 30)), \ + mock.patch.object(pr_review, 'timeline', return_value=picked_up) as seen, \ + mock.patch.object(pr_review.time, 'sleep'): + self.assertEqual(30, pr_review.main( + ['wait', '7', '--pickup-grace', '300', '--timeout', '1200'])) + # Roughly one read per grace interval over the wait, never one per poll. + self.assertGreaterEqual(seen.call_count, 1) + self.assertLessEqual(seen.call_count, 1200 // 300 + 1) + + def test_a_review_landing_during_the_last_read_wins_over_the_stalled_code(self) -> None: + """The digest and the exit code come from one payload, or they describe different PRs. + + An automated reader resolves a digest saying covered against a code saying stalled by + believing the code, so the review it just printed is the thing that gets dropped. + """ + self.answer(payload([review(oid=OLD)], pending=True), payload([review()], pending=True)) + with mock.patch.object(pr_review, 'timeline', + return_value=[('review_requested', LATE)]), \ + mock.patch.object(pr_review.time, 'sleep'): + self.assertEqual(0, pr_review.main( + ['wait', '7', '--pickup-grace', '0', '--timeout', '0'])) + out = self.out.getvalue() + self.assertIn('review_on_head=yes', out) + self.assertNotIn('status=REQUEST_NOT_PICKED_UP', out) + + def test_a_review_landing_during_the_last_read_wins_over_the_timeout(self) -> None: + """Same disagreement at the other exit: printing coverage and returning PENDING.""" + self.answer(payload([review(oid=OLD)]), payload([review()])) + with mock.patch.object(pr_review.time, 'sleep'): + self.assertEqual(0, pr_review.main(['wait', '7', '--timeout', '0'])) + out = self.out.getvalue() + self.assertIn('review_on_head=yes', out) + self.assertNotIn('status=PENDING', out) + + def test_a_request_picked_up_after_the_loop_read_it_is_not_reported_as_stalled(self) -> None: + """The stall is re-read at the end, or a request taken up since still reports as dead.""" + self.answer(payload([review(oid=OLD)], pending=True)) + picked_up = [('review_requested', EARLY), ('copilot_work_started', LATE)] + with mock.patch.object(pr_review, 'timeline', + side_effect=[[('review_requested', LATE)], picked_up]), \ + mock.patch.object(pr_review.time, 'sleep'): + self.assertEqual(30, pr_review.main( + ['wait', '7', '--pickup-grace', '0', '--timeout', '0'])) + out = self.out.getvalue() + self.assertNotIn('status=REQUEST_NOT_PICKED_UP', out) + self.assertNotIn('REQUEST NOT PICKED UP', out) + + def test_an_answer_outranks_a_stall_when_both_are_true(self) -> None: + """The reviewer saying something outranks it saying nothing, and the digest shows both.""" + self.answer(payload([review(oid=OLD)], comments=[comment()], pending=True)) + with mock.patch.object(pr_review, 'timeline', + return_value=[('review_requested', LATE)]), \ + mock.patch.object(pr_review.time, 'sleep'): + self.assertEqual(40, pr_review.main( + ['wait', '7', '--pickup-grace', '0', '--timeout', '0'])) + def test_the_repo_argument_splits_into_owner_and_name(self) -> None: self.answer(payload([review()])) with mock.patch.object(pr_review, 'digest', return_value=('x', 0)) as dig: @@ -287,6 +602,72 @@ def test_no_mutation_reaches_this_script(self) -> None: with self.subTest(verb=verb): self.assertFalse(verb in source, f'{verb!r} is a state-changing call in a read-only script') + def test_the_guard_tests_the_window_the_queries_actually_read(self) -> None: + """A guard measuring one number while the query fetches another reads clean on drift.""" + source = (REPO / 'scripts' / 'pr_review.py').read_text(encoding='utf-8') + windows = set(re.findall(r'(?:comments|reviews)\(last:(\d+)\)', source)) + self.assertEqual({str(pr_review.WINDOW)}, windows) + # The guard reads `hasPreviousPage`, so a connection that stops asking reports no. + # That is the silent narrowing this holds every window against. + # Four: reviews and comments, in each of the two queries. + self.assertEqual(4, source.count('pageInfo{ hasPreviousPage }')) + self.assertEqual(4, len(re.findall(r'(?:comments|reviews)\(last:\d+\)', source))) + + def test_the_timeline_reader_asks_for_the_largest_page(self) -> None: + """The page size is what pagination costs, and the default of 30 triples the requests.""" + done = subprocess.CompletedProcess(args=[], returncode=0, stdout='', stderr='') + with mock.patch.object(pr_review.subprocess, 'run', return_value=done) as run: + pr_review.timeline('o', 'r', 7) + argv = run.call_args.args[0] + self.assertIn('repos/o/r/issues/7/timeline?per_page=100', argv) + self.assertIn('--paginate', argv) + # A read, and the guard against a write creeping into the one REST call here. + self.assertEqual(['gh', 'api'], argv[:2]) + self.assertFalse({'-X', '--method'} & set(argv)) + + def test_the_timeline_filter_takes_the_reviewer_s_own_requests_only(self) -> None: + """A human requested later is not this request, and reading it as one reports a stall. + + The filter runs inside gh, so this drives the real `jq` over a crafted timeline rather + than asserting on the filter's text, which would pass on a filter that matches nothing. + The timeline spells the reviewer `Copilot` with type `Bot`, a third form after GraphQL's + `copilot-pull-request-reviewer` and REST's `[bot]` suffix on that, so a filter keyed to + either of those two selects nothing here and the whole state reads as no request at all. + """ + events = [ + {'event': 'review_requested', 'created_at': '01', 'requested_reviewer': + {'login': 'Copilot', 'type': 'Bot'}}, + {'event': 'copilot_work_started', 'created_at': '02'}, + {'event': 'review_requested', 'created_at': '03', 'requested_reviewer': + {'login': 'ptr727', 'type': 'User'}}, + {'event': 'review_requested', 'created_at': '04', 'requested_reviewer': + {'login': 'some-other-bot', 'type': 'Bot'}}, + {'event': 'commented', 'created_at': '05'}, + ] + run = subprocess.run(['jq', '-r', pr_review.TIMELINE_JQ], + input=json.dumps(events), capture_output=True, text=True) + self.assertEqual(0, run.returncode, run.stderr) + self.assertEqual(['review_requested 01', 'copilot_work_started 02'], + run.stdout.split('\n')[:-1]) + # The reading that matters: the human request must not become the newest request. + parsed = [(ln.split(' ', 1)[0], ln.split(' ', 1)[1]) for ln in run.stdout.splitlines()] + self.assertEqual('', pr_review.never_picked_up(parsed)) + + def test_a_negative_pickup_grace_is_rejected_rather_than_read_as_every_poll(self) -> None: + """It leaves the next reading behind the clock, which is the per-poll pattern returning.""" + with contextlib.redirect_stderr(io.StringIO()): + with self.assertRaises(SystemExit): + pr_review.main(['wait', '7', '--pickup-grace', '-1']) + + def test_a_failed_timeline_read_raises_rather_than_reading_as_no_events(self) -> None: + """An empty list reads as no request pending, which is the false clean one level up.""" + failed = subprocess.CompletedProcess(args=[], returncode=1, stdout='', stderr='boom') + with mock.patch.object(pr_review.subprocess, 'run', return_value=failed), \ + contextlib.redirect_stderr(io.StringIO()) as err: + with self.assertRaises(SystemExit): + pr_review.timeline('o', 'r', 7) + self.assertIn('boom', err.getvalue()) + def test_the_backoff_is_bounded_and_non_decreasing(self) -> None: """A wait that sleeps zero seconds is a busy loop, and one that shrinks polls harder later.""" source = (REPO / 'scripts' / 'pr_review.py').read_text(encoding='utf-8') 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."""