diff --git a/.claude/commands/address-review/SKILL.md b/.claude/commands/address-review/SKILL.md new file mode 100644 index 000000000000..037324b30440 --- /dev/null +++ b/.claude/commands/address-review/SKILL.md @@ -0,0 +1,166 @@ +--- +name: address-review +description: "Work a PR's pre-merge review to zero. Watches for the pinned review to land after you ship, then walks every finding β€” blockers, low-confidence, and style suggestions β€” with you until each one is fixed, refuted, deferred, or explicitly accepted. Use when the user types /address-review, ships a PR to pulumi/docs, asks to watch or monitor a PR for review results, says the review came back / what did the review say / address the review feedback, or is about to merge a PR that still has open findings." +argument-hint: "[] [--watch] [--no-watch] [--resume]" +user-invocable: true +--- + +# `/address-review` β€” work a pre-merge review to zero + +**The rule this skill exists to enforce:** a PR is not finished when it is pushed. It is finished when **every** item the pre-merge review raised has been *fixed*, *refuted*, *deferred to a filed issue*, or *explicitly accepted with a stated reason* β€” 🚨 blockers, ⚠️ low-confidence findings, and ✏️ style suggestions alike. + +The review pipeline is good at finding things and has no way to make anyone look at them. The measured failure mode is real: `scrape-review-outcomes.py` tracks an `ignored_low_confidence` outcome precisely because authors clear 🚨 and stop reading. This skill is the counterweight. + +**Related skills:** `/docs-review` runs the same criteria locally *before* you push (cheaper). `/shipit` creates the PR and hands off here. `/pr-review` is the *maintainer* adjudication layer β€” it decides approve/merge; this one is the *author* side and runs first. + +--- + +## Usage + +`/address-review [] [--watch|--no-watch] [--resume]` + +- **PR** β€” optional; inferred from the current branch when omitted. +- `--watch` β€” skip the offer and start watching for the review immediately. +- `--no-watch` β€” the review is already posted; go straight to the worklist. +- `--resume` β€” reload the saved worklist state and continue where the last session stopped. + +Worklist state lives in `.review-worklist-.json` at the repo root (gitignored). It survives context loss: a fresh session with `--resume` picks up every disposition already recorded. + +--- + +## Offer this without being asked + +**Whenever you open or push to a PR in this repo, the review loop is part of the job.** Do not wait to be asked. + +1. **On PR creation (draft).** Say in one line that automated review fires when the PR goes ready-for-review, and that you'll work the findings when they land. Don't offer to watch yet β€” a draft gets no review. +1. **On ready-for-review.** Offer to watch, with `AskUserQuestion`: *Watch for the review* (recommended) / *Ping me when I ask* / *Skip β€” I'll merge without it*. If watching, follow `address-review:references:watching`. +1. **When the review lands.** Announce the bucket counts and start Step 3. Do not summarize the review and stop β€” a summary is not a disposition. +1. **Whenever the user moves to merge with items still open.** Say so plainly, once, with the count and the shortest path to clearing it: *"3 findings still open (1 blocker, 2 style). Want me to work them now β€” about 5 minutes β€” or record why we're merging over them?"* Recording a reason is a legitimate outcome; skipping the question is not. + +**Be pushy, not obstructive.** Raise it once per decision point, concretely, then do what the user says. Never block a merge the user has decided on, never re-litigate a finding they already dispositioned, and never nag about items that are already dispositioned. If the user says "just merge it," record `accepted` with their reason on the open items so the ledger tells the truth, then get out of the way. + +--- + +## Process + +Steps 1-3 are mostly silent. Step 4 is the skill. + +### Step 1 β€” Resolve the PR and classify the review state + +```bash +PR=$(gh pr view --json number --jq .number) # when no argument was given +gh pr view "$PR" --json isDraft,mergeStateStatus,labels,headRefOid,url,title +``` + +Classify from the labels β€” the five state labels are mutually exclusive (`set-review-label.sh` owns them): + +| Label / signal | Meaning | What to do | +|---|---|---| +| PR is a draft | Review doesn't run on drafts | Offer to mark ready-for-review; that is what fires the review | +| `review:in-progress` | Workflow running now | Step 2 (watch) | +| `review:outstanding-issues` | Review posted, 🚨 > 0 | Step 3 | +| `review:no-blockers` | Review posted, 🚨 == 0 | Step 3 β€” ⚠️ and ✏️ items still need dispositions | +| `review:stale` | Pushed since the review ran | Refresh first (see Step 6), then Step 3 | +| `review:error` | Workflow failed before publishing | Check the Actions run; `@claude #update-review` to retry | +| `review:trivial` / `review:frontmatter-only` / `review:oversized` | Full review short-circuited | No pinned comment. If `review:prose-flagged` is also set, triage's advisory comment **is** the worklist β€” walk it the same way | + +Verify freshness even on a `CURRENT`-looking label: pushes made with `GITHUB_TOKEN` or by a coding agent don't fire `synchronize`, so `review:stale` can be missing on a review that predates the head commit. + +```bash +HEAD_SHA=$(gh pr view "$PR" --json headRefOid --jq .headRefOid) +REVIEWED_SHA=$(bash .claude/commands/docs-review/scripts/pinned-comment.sh fetch --pr "$PR" \ + | grep -oE '' | tail -1 | grep -oE '[0-9a-f]{7,40}') +``` + +A `REVIEWED_SHA` that isn't a prefix of `HEAD_SHA` means stale regardless of labels β€” say so, and refresh before working the list. Working a stale review wastes the user's time on findings that may already be fixed. + +### Step 2 β€” Watch for the review (only when it hasn't landed) + +Follow `address-review:references:watching`. It covers both environments (event subscription where available, bounded polling otherwise), what to do while waiting, and when to give up and hand back. + +### Step 3 β€” Build the worklist + +```bash +python3 .claude/commands/docs-review/scripts/review-worklist.py --pr "$PR" --format json \ + --state ".review-worklist-$PR.json" +``` + +The script enumerates every item needing a disposition and assigns each a stable id (`outstanding:L40-50`, `low:L12`, `style:content/docs/a.md:L88`, `pre-existing:L7`), merges in the inline one-click suggestions posted on the Files-changed tab, and reports what is still undecided. On `--resume`, the same command reloads prior dispositions. + +**If `parse_confidence` comes back `low`, do not proceed as if the list were complete** β€” read the pinned comment yourself and work from it, saying that the enumerator couldn't parse it. + +Present the counts before working: `4 items: 1 blocker, 1 low-confidence, 2 style (1 one-click). Pre-existing: 1 (optional).` Then start. + +### Step 4 β€” Walk the worklist with the user + +**One item at a time, in bucket order:** 🚨 Outstanding β†’ ⚠️ Low-confidence β†’ ✏️ Style β†’ πŸ’‘ Pre-existing (optional; ask once whether to include them at all, default no). + +For each item, present a compact block β€” never a wall: + +```text +[1/4] 🚨 outstanding:L40 content/docs/ai/skills/index.md +Finding: "Pulumi supports 9 languages" β€” the docs say six. +Evidence: ❌ contradicted (source: content/docs/iac/languages-sdks/) +Proposal: change "nine" β†’ "six" on line 40. +``` + +Then `AskUserQuestion` with the dispositions that plausibly apply to *this* item, drawn from the closed set in `address-review:references:dispositions`: **Fix it** / **Refute it** / **Defer to an issue** / **Accept as-is** / **Not applicable**. The tool takes at most four options, so offer the three or four that fit β€” your recommendation first, marked `(Recommended)` β€” and let the rest arrive through "Other". Whatever the user picks, map it back to one of the five before recording it. + +Rules for the walk: + +- **Have a proposal before you ask.** Read the file, work out the actual change, and show it. "What do you want to do about this?" with no proposal makes the user do the work twice. +- **Verify the finding before proposing a fix.** The review can be wrong; a fix applied to a false finding is worse than the finding. When you believe it's wrong, recommend *Refute* and bring the evidence β€” that's what the dispute path is for. +- **Batch only what is genuinely identical.** Style suggestions on the same rewrite across several files can be one question ("apply all 6 Vale suggestions?"). Substantive findings get their own question each. +- **Record every decision immediately** into `.review-worklist-.json` β€” `{"items": {"": {"disposition": "fixed", "note": "..."}}}` β€” so a lost session resumes instead of restarting. `deferred`, `accepted`, and `not-applicable` require a note; the enumerator treats a missing one as still-open. +- **Apply fixes to the working tree as you go, but don't push mid-walk.** One push at the end keeps the auto-refresh gate's small-diff shape intact. +- **Never silently drop an item.** If the user doesn't answer one, it stays open and shows up in the Step 7 report. + +### Step 5 β€” Execute the batch + +1. Run the repo's own checks on what you changed: `make lint`, plus `ONLY_TEST="" ./scripts/programs/test.sh` when a `static/programs/` example moved. Never push a review fix that breaks the build. +1. Commit with a message naming the review round (`Address pre-merge review: language count, 6 Vale suggestions`), keeping the `Co-Authored-By: Claude ...` trailer. +1. Push: `git push -u origin `. +1. **One-click style suggestions**: applying them in the GitHub UI and pushing a fix for the same line collide. Pick one lane per item and say which β€” either the user clicks **Add suggestion to batch** on the Files-changed tab (nothing for you to commit), or you edit the line locally and the button goes stale. Don't do both. + +### Step 6 β€” Refresh the review and verify convergence + +A push marks the review stale. Refreshing is not optional β€” an unrefreshed review is a permanent record that the findings were never addressed. + +- **Small fix-push** (≀80 changed lines, every hunk on a flagged line): `auto-refresh-gate.py` fires the scoped refresh on its own. Wait for it rather than double-posting. +- **Anything else**: comment `@claude #update-review` and say what you did. Put fix-responses and disputes in the *same* mention β€” the update path handles both: + + ```text + @claude #update-review + + Fixed: the language count on L40 (now "six"), and the 6 advisory Vale + suggestions. + + Disputing L12: "teams often" is sourced from the 2026 state-of-IaC + survey, cited two paragraphs down. Please re-check with that in view. + ``` + +- Then re-run Step 3's command and confirm the fixed items moved into βœ… Resolved and the disputed ones were adjudicated (conceded, or held with a reason). **A finding the model holds after a dispute is still open** β€” take it back into Step 4 with the model's reasoning in hand. + +Loop Steps 4-6 until `--require-clean` passes: + +```bash +python3 .claude/commands/docs-review/scripts/review-worklist.py --pr "$PR" \ + --state ".review-worklist-$PR.json" --require-clean +``` + +### Step 7 β€” Report and hand off + +Report in one block: what was fixed (with the commit), what was refuted (and how the model adjudicated), what was deferred (with issue links), what was accepted (with reasons), and anything still open. Then say what's next: + +- **Clean** β€” the PR is ready for a maintainer. Mention `/pr-review ` for the adjudication pass. +- **Not clean** β€” name exactly what's left and offer to keep going. Don't call a PR ready while the exit code says otherwise. + +--- + +## Non-negotiables + +- **Never** mark an item resolved because it looks minor. Style suggestions get a disposition like everything else β€” `accepted` with "house voice, leaving it" is a fine answer; silence is not. +- **Never** delete, hide, or resolve the pinned `` comment. Hiding it makes later refreshes edit a comment nobody can see. Use the review's own βœ… Resolved section as the tracker. +- **Never** push a fix without re-running the review afterward. +- **Never** invent a finding's resolution in the PR thread that the diff doesn't support. The pinned comment is scraped after merge into the `#docs-ops` digest; a false "fixed" corrupts the tuning data the review's severity rules are built from. +- **Never** hold the user hostage. Pushy means asking once, clearly, with the cost stated. It does not mean refusing to proceed. diff --git a/.claude/commands/address-review/references/dispositions.md b/.claude/commands/address-review/references/dispositions.md new file mode 100644 index 000000000000..c9107558ce87 --- /dev/null +++ b/.claude/commands/address-review/references/dispositions.md @@ -0,0 +1,88 @@ +--- +user-invocable: false +description: The closed set of dispositions for a review finding β€” what each means, when it's allowed, how to execute it, and how it's recorded. +--- + +# Dispositions + +Every item on the worklist ends in exactly one of five states. There is no sixth state, and "we talked about it" is not one of them. + +| Disposition | Means | Note required | Evidence that it happened | +|---|---|:---:|---| +| `fixed` | The diff changed; the finding no longer applies | no | The commit | +| `refuted` | Disputed with evidence; the model conceded | no | The `#update-review` mention + the βœ… Resolved `concede:` annotation | +| `deferred` | Real, but out of scope for this PR | **yes** | A filed issue, linked in the note | +| `accepted` | Knowingly shipping as-is | **yes** | The note (and, for a blocker, a PR comment) | +| `not-applicable` | The finding misreads the change; nothing to do and nothing to argue | **yes** | The note | + +`fixed` and `refuted` evidence themselves. The other three are judgment calls someone has to own, so `review-worklist.py --require-clean` treats a missing note as an open item. + +--- + +## `fixed` + +The ordinary path. Make the change, keep it minimal, and keep it to what the finding actually asks for β€” a review fix is not an invitation to rewrite the section. + +- Apply to the working tree during the walk; push once at the end (Step 5). A single small fix-push that lands only on flagged lines is what `auto-refresh-gate.py` recognizes, and it refreshes the review with no mention needed. +- For a `[style-blocker]` bullet in 🚨 (wrong product name, banned term, misspelling): fix it. These come from Vale's blocker allowlist, they are deterministic, and they are not worth disputing. +- For an inline ✏️ one-click suggestion: either the user clicks it in the Files-changed tab **or** you edit the line locally. Never both β€” the second one conflicts with the first. + +## `refuted` + +Use when the finding is wrong, not when it's inconvenient. Refuting well is a service: it tunes the pipeline. Refuting lazily poisons the outcome telemetry. + +Dispute in the same `@claude #update-review` mention as the fixes, saying which finding and why. The update path classifies the dispute three ways, and what counts as evidence differs: + +- **Domain-knowledge** ("this pattern is intentional; the team decided it") β€” the model defaults to conceding, and maintainer write access is itself sufficient evidence for design intent. Say plainly that it's a design decision. +- **Verifiable claim** ("that was added in v3.0", "the docs already say this elsewhere") β€” author authority proves nothing here. Bring the link, the file:line, or the command output, or the model will hold. +- **Reframing** ("you misread the sentence; the qualifier bounds it") β€” quote the sentence and the reading you intend. + +Then check the outcome. A concede moves the finding to βœ… Resolved with a `concede: ` annotation. **If the model holds** β€” a `πŸ›‘οΈ Disputed by … model held.` line β€” the item is *not* resolved. Take it back into the walk with the model's cited evidence in hand and pick a different disposition. Don't record `refuted` on a finding that was held. + +## `deferred` + +Real finding, wrong PR. Legitimate for a pre-existing problem the change merely brushed past, or a fix that would balloon the diff past what a reviewer can read. + +- File the issue **now**, in the same session, and put its URL in the note. A deferral without an issue is an acceptance wearing a disguise. +- Give the issue enough context to act on cold: the finding text, the file, the line, and why it was out of scope here. +- Say it in the PR thread too, so the maintainer isn't left wondering. One line: "L88 heading case is pre-existing β€” filed #20456." + +## `accepted` + +Knowingly shipping with the finding standing. Always available, never free. + +- The note must say *why*, in terms someone reading the PR later can evaluate: "house voice β€” we say 'simply' in tutorials deliberately", not "won't fix". +- For a 🚨 blocker, also post the reason as a PR comment. A blocker accepted silently reads to the scraper as `ignored_outstanding`, and to a maintainer as an oversight. +- This is the disposition to use when the user says "just merge it." Record it, with their reason, on each open item. That is the honest ledger entry, and it takes ten seconds. + +## `not-applicable` + +The finding is about something the change doesn't do β€” the reviewer matched the wrong line, or the finding describes code the PR deletes. Distinct from `refuted`: there's no factual dispute to adjudicate, just a mis-anchor. + +- The note says what the finding actually points at and why nothing follows from it. +- If you find yourself reaching for this more than once or twice in a review, the review probably went stale against a newer head. Refresh it and re-read (skill Step 1) rather than dismissing item after item. + +--- + +## Bucket-specific rules + +- **🚨 Outstanding** β€” `fixed` or `refuted` are the expected outcomes. `deferred` and `accepted` are allowed but must be visible in the PR thread, not only in the local state file. Never leave one undecided. +- **⚠️ Low-confidence** β€” these don't block the PR and they still get a disposition. Most are author questions ("can you cite this?"); the answer is usually a one-line `fixed` or a `refuted` with the citation. +- **✏️ Style** β€” advisory. Apply, or `accepted` with a reason. Batch identical rewrites into one question; don't ask six times about "simply". +- **πŸ’‘ Pre-existing** β€” optional by construction: not introduced by this PR and not the author's debt. Ask once whether to include them, default no, and `deferred` with an issue is the good outcome when the user says yes. + +## Recording + +State file, `.review-worklist-.json` at the repo root (gitignored): + +```json +{ + "items": { + "outstanding:L40": { "disposition": "fixed", "note": "" }, + "low:L12": { "disposition": "refuted", "note": "cited two paragraphs down; model conceded" }, + "style:content/docs/a.md:L91": { "disposition": "accepted", "note": "term of art on this page" } + } +} +``` + +Write it as each decision is made, not at the end. The file is what makes `--resume` work after a lost session, and what `--require-clean` reads to answer the only question that matters at merge time: **is anything still undecided?** diff --git a/.claude/commands/address-review/references/watching.md b/.claude/commands/address-review/references/watching.md new file mode 100644 index 000000000000..45933c4f7949 --- /dev/null +++ b/.claude/commands/address-review/references/watching.md @@ -0,0 +1,71 @@ +--- +user-invocable: false +description: How to wait for a pinned pre-merge review to land β€” event subscription, bounded polling, and when to hand back. +--- + +# Watching for the review + +The pre-merge review is a GitHub Actions job, not something you can block on. `claude-code-review.yml` gives the job a 40-minute ceiling and the model step 18 minutes; in practice a review posts in **5-15 minutes** from the ready-for-review transition. Anything past ~40 minutes without a pinned comment is a failure, not a slow run. + +## First: is a review even coming? + +Don't watch a PR that will never post one. + +| Situation | Signal | What to say | +|---|---|---| +| PR is a draft | `isDraft: true` | "Review fires when this goes ready-for-review β€” want me to mark it ready?" | +| Trivial short-circuit | `review:trivial` | No pinned comment is coming. If `review:prose-flagged` is also set, triage posted an advisory comment β€” walk that instead. | +| Frontmatter-only | `review:frontmatter-only` | Same as above. | +| Oversized | `review:oversized` | Triage posted a `` advisory suggesting a split. Offer to split the hand-written source into its own PR β€” that PR gets a real review. | +| Bot-authored PR | author is `pulumi-bot` / `dependabot[bot]` | Review skips bot PRs. | +| `review:error` | Workflow failed before publishing | Watching won't help. Read the Actions log; `@claude #new-review` reruns from scratch. | + +## Preferred: subscribe to PR events + +When the session has PR activity subscription available (Claude Code on the web and other remote sessions expose `subscribe_pr_activity`), use it: + +- Subscribe once with the repo and PR number, then **end the turn**. Review completion, CI results, and comments arrive as wake events; the session resumes on its own. +- Do not also poll. A subscription plus a polling loop wakes twice per event and burns the session for nothing. +- On the wake event: re-fetch the pinned comment, then go to the skill's Step 3. The event tells you *that* something happened, never *what the review says*. +- Unsubscribe when the PR merges or closes, or when the user says to stop. + +## Fallback: bounded polling + +In a local CLI session there is no event stream. Poll on a **bounded** loop, and tell the user the shape of it before starting ("checking every 2 minutes for up to 30"). + +```bash +for i in $(seq 1 15); do + LABELS=$(gh pr view "$PR" --json labels --jq '[.labels[].name] | join(",")') + case "$LABELS" in + *review:outstanding-issues*|*review:no-blockers*) echo "review posted"; break ;; + *review:error*) echo "review errored"; break ;; + esac + sleep 120 +done +``` + +Rules for the fallback: + +- **Cap it.** 30 minutes of polling, then stop and report β€” never an unbounded loop. +- **Stay quiet while waiting.** One line at the start, one when it lands. No per-iteration narration. +- **Offer the alternative first.** Polling occupies the session; many users would rather do something else and run `/address-review ` later. Ask, and make "ping me later" a real option rather than a formality. +- **Watch the run, not just the label,** when the user wants detail: `gh run watch` on the `Pre-merge Review (main)` workflow run for the head SHA. + +## While waiting + +Waiting time is not dead time. Useful things to offer: + +- Run `/docs-review` locally on the same branch β€” same criteria, no GitHub round-trip. Findings you fix now can land in the same fix-push later. +- Pre-read the diff for the things the review reliably flags: missing aliases on moved files, internal links to pages that don't exist, frontmatter `meta_desc` length, heading case. + +Don't start speculative edits while waiting unless the user asks. A push during the review run makes the review stale the moment it posts. + +## When it doesn't land + +Past the 40-minute ceiling with no pinned comment and no `review:error`: + +1. Check the workflow run for the head SHA β€” a cancelled or timed-out job leaves no comment. +1. Check whether the PR is still marked ready (a draft transition mid-run kills it). +1. Re-trigger with `@claude #new-review`, which bypasses the skip paths, or transition draft β†’ ready. + +Report what happened rather than waiting again. Two silent 40-minute waits is worse than one clear "the review job timed out; want me to retrigger it?" diff --git a/.claude/commands/docs-review/SKILL.md b/.claude/commands/docs-review/SKILL.md index 31c96e0bee01..ef3c227f0bc0 100644 --- a/.claude/commands/docs-review/SKILL.md +++ b/.claude/commands/docs-review/SKILL.md @@ -8,6 +8,8 @@ user-invocable: true Output goes into the conversation. This skill never posts to GitHub. +This is the *pre-push* pass. Once a PR is open and CI has posted its pinned review, `/address-review` is the skill that works those findings to zero with the author. + ## Usage `/docs-review [PR_NUMBER]` diff --git a/.claude/commands/docs-review/scripts/review-worklist.py b/.claude/commands/docs-review/scripts/review-worklist.py new file mode 100755 index 000000000000..5b5bf1174fcf --- /dev/null +++ b/.claude/commands/docs-review/scripts/review-worklist.py @@ -0,0 +1,781 @@ +#!/usr/bin/env python3 +"""review-worklist.py β€” turn a pinned pre-merge review into an author worklist. + +The pinned `` comment is written for reading, not for +working: findings live in four H3 buckets plus an H4 style block, the one-click +suggestions live in a separate review-comment thread, and nothing in either +place tracks whether the author actually did something about an item. An author +(or an agent working with one) who clears 🚨 and stops reading leaves the rest +silently unaddressed β€” which is exactly what the `ignored_low_confidence` column +in scrape-review-outcomes.py keeps measuring. + +This script enumerates every item that needs a disposition, assigns each a +stable id, and β€” given a state file recording what was decided β€” reports what is +left. It is the deterministic half of the `/address-review` skill: the model +decides *what* to do with a finding, this decides *whether anything is still +undecided*. Nothing here writes to GitHub. + +Disposition vocabulary (closed set, see `address-review:references:dispositions`): + + fixed β€” the diff changed; the finding no longer applies + refuted β€” disputed with evidence via `@claude #update-review` + deferred β€” real but out of scope; tracked in a filed issue (note required) + accepted β€” knowingly shipping as-is (note required) + not-applicable β€” the finding misreads the change; nothing to do (note required) + +`fixed` and `refuted` are self-evidencing β€” the diff or the dispute comment is +the record. The other three are judgment calls someone has to own, so they carry +a mandatory note that `--require-clean` enforces. + +Usage: + review-worklist.py --pr 20123 [--repo owner/repo] + Fetch the pinned review + inline style suggestions; print a markdown + checklist. + review-worklist.py --pr 20123 --format json + Same, as JSON (what the skill parses). + review-worklist.py --body-file pinned.md [--suggestions-file comments.json] + Offline: parse a body already on disk. `--suggestions-file` takes the raw + `gh api .../pulls/N/comments` JSON array. + review-worklist.py --pr 20123 --state .review-state.json --require-clean + Exit 1 when any non-optional item has no disposition (or a note-requiring + disposition has no note). This is the "is the PR actually done?" check. + review-worklist.py --self-test + Run embedded parse checks (no network). + +Parsing reuses validate-pinned.py's body helpers (find_section, +extract_count_table_row, extract_trail_records, extract_bullet_prefix) and its +exported FINDING_START_RE, so the comment-format contract keeps exactly one +parser β€” same import-by-path pattern as scrape-review-outcomes.py. The one +deliberate extension is `_bullet_blocks`: extract_bucket_bullets returns a +bullet's first line, which is right for counting and useless for working, so +this module walks the same sections with the same rule and keeps each bullet's +continuation lines. Same recognition rule, wider capture β€” not a second parser. + +Fail-open on inputs, fail-closed on completeness. Three things independently +block a "clean" verdict, so --require-clean can only pass when the whole list +was actually seen: an unparseable body (`parse_confidence: "low"`), a failed +inline-suggestions fetch (`suggestions_ok: false` β€” distinct from a PR that +genuinely has none), and any parsed item without a recorded disposition. +""" + +from __future__ import annotations + +import argparse +import importlib.util +import json +import re +import subprocess +import sys +import tempfile +from pathlib import Path + +HERE = Path(__file__).resolve().parent + +# Single source of truth for pinned-body parsing. validate-pinned.py's name is +# hyphenated, so import by path; its main() is __main__-guarded, so importing +# has no side effects. +_spec = importlib.util.spec_from_file_location("validate_pinned", HERE / "validate-pinned.py") +_vp = importlib.util.module_from_spec(_spec) +# Register before exec: validate-pinned.py defines dataclasses, and the +# dataclass machinery resolves the defining module through sys.modules. +sys.modules["validate_pinned"] = _vp +_spec.loader.exec_module(_vp) + +DEFAULT_REPO = "pulumi/docs" +PAGE_DELIMITER = "----- PINNED-COMMENT-DELIMITER -----" +MARKER_RE = re.compile(r"^\s*$", re.M) +HEAD_SENTINEL_RE = re.compile(r"") +# Inline one-click suggestions are posted by post-style-suggestions.py; every +# comment body it writes starts with this marker. +SUGGESTION_MARKER = "" +STYLE_BULLET_RE = re.compile(r"^\s*-\s+\*\*line (\d+):?\*\*\s*(.*)$") +STYLE_FILE_HEADING_RE = re.compile(r"^#####\s+`?([^`\s]+)`?\s*$") +# The bullet-recognition rule comes from the shared parser, never a local +# copy β€” _bullet_blocks extends what extract_bucket_bullets does with that +# rule, so the two must agree by construction rather than by vigilance. +FINDING_START_RE = _vp.FINDING_START_RE + +DISPOSITIONS = ("fixed", "refuted", "deferred", "accepted", "not-applicable") +# Dispositions that are a judgment call rather than a change in the diff. The +# review record can't evidence these on its own, so a human-readable reason is +# mandatory β€” otherwise "accepted" becomes an unaudited way to close the loop. +NOTE_REQUIRED = ("deferred", "accepted", "not-applicable") + +# Every bucket's configuration, read by both extraction paths β€” the plain-H3 +# loop and the ⚠️ split. `low` and `style` share a heading because the style +# block is an H4 *inside* the ⚠️ Low-confidence H3; _split_low_confidence +# divides them. Nothing hard-codes a heading string outside this table. +BUCKETS = { + # id prefix heading substring blocking optional + "outstanding": ("🚨 Outstanding", True, False), + "low": ("⚠️ Low-confidence", False, False), + "style": ("⚠️ Low-confidence", False, False), + "pre-existing": ("πŸ’‘ Pre-existing", False, True), +} +# Buckets whose bullets sit directly under their own H3, with no H4 split. +PLAIN_BUCKETS = ("outstanding", "pre-existing") + + +def log(msg: str) -> None: + print(f"review-worklist: {msg}", file=sys.stderr) + + +# ---- gh access --------------------------------------------------------------- + + +def run(args: list[str]) -> str: + """Run a command; return stdout, or "" on failure (logged to stderr).""" + try: + proc = subprocess.run(args, capture_output=True, text=True, check=True) + return proc.stdout + except (subprocess.CalledProcessError, FileNotFoundError) as exc: + detail = exc.stderr.strip() if getattr(exc, "stderr", "") else str(exc) + log(f"warning: {' '.join(args[:4])}... failed: {detail}") + return "" + + +def fetch_pinned_body(repo: str, pr: int) -> str: + return run(["bash", str(HERE / "pinned-comment.sh"), "fetch", "--pr", str(pr), "--repo", repo]) + + +def fetch_inline_suggestions(repo: str, pr: int) -> tuple[list[dict], bool]: + """Return (suggestions, ok). `ok=False` means the fetch failed. + + The distinction is the whole point: "this PR has no ✏️ suggestions" and "I + couldn't ask" both produce an empty list, and only one of them means the + worklist is complete. `run()` swallows every gh failure (auth, rate limit, + network) into "", so without this flag a --require-clean run could answer + "clean" for a PR whose style items were never enumerated. + """ + out = run([ + "gh", "api", f"repos/{repo}/pulls/{pr}/comments", + "--paginate", + "--jq", f'[.[] | select(.body | startswith("{SUGGESTION_MARKER}"))]', + ]) + # `--jq '[...]'` prints at least `[]` per page on success, so silence is failure. + if not out.strip(): + return [], False + # --paginate --jq emits one JSON document per page; concatenate the arrays. + # Every page must decode: a partial failure (page 1 parses, page 2 is a gh + # error object or truncated JSON) yields a genuinely short suggestion set, + # which is the same signal loss as a total failure β€” just quieter. + merged: list[dict] = [] + decoded_any = False + failed_any = False + for chunk in out.strip().splitlines(): + try: + parsed = json.loads(chunk) + except json.JSONDecodeError: + failed_any = True + continue + if isinstance(parsed, list): + decoded_any = True + merged.extend(x for x in parsed if isinstance(x, dict)) + else: + # Valid JSON that isn't a page of results β€” an error object, say. + failed_any = True + return merged, decoded_any and not failed_any + + +# ---- body normalization ------------------------------------------------------ + + +def join_pages(raw: str) -> str: + """Fold a multi-comment fetch into one logical body. + + `pinned-comment.sh fetch` prints every page separated by a delimiter line, + each still carrying its `` marker. Sections can + straddle a page boundary, so the parser needs them joined back together. + """ + pages = raw.split(PAGE_DELIMITER) + return MARKER_RE.sub("", "\n".join(p.strip("\n") for p in pages if p.strip())) + + +# ---- item extraction --------------------------------------------------------- + + +def _split_low_confidence(body: str) -> tuple[list[str], list[str]]: + """Return (low-confidence bullets, style-block lines) from the ⚠️ section. + + The `#### Style suggestions` H4 lives *inside* the ⚠️ Low-confidence H3, and + its bullets are uncounted advisory polish rather than reviewer burden β€” so + they are their own bucket here, not low-confidence findings. + + Returns (low-confidence bullet blocks, raw style-block lines). + """ + span = _vp.find_section(body, BUCKETS["low"][0]) + if span is None: + return [], [] + start, end = span + lines = body.splitlines()[start:end] + style_idx = None + for i, line in enumerate(lines): + if line.strip() in _vp.STYLE_HEADINGS: + style_idx = i + break + head = lines[:style_idx] if style_idx is not None else lines + style = lines[style_idx:] if style_idx is not None else [] + blocks = [b for b in _bullet_blocks(head) if not STYLE_BULLET_RE.match(b[0])] + return blocks, style + + +def _bullet_blocks(lines: list[str]) -> list[tuple[str, str]]: + """Group each column-0 finding bullet with its continuation lines. + + `extract_bucket_bullets` returns first lines only ("Sub-bullets (indented) + and continuation paragraphs (no leading `**`) are not counted"), which is + right for counting and wrong for working: a 🚨 bullet's fix prose and + suggested patch all live below its first line, and that is the part the + author has to act on. One block per bullet, so the count still matches. + """ + blocks: list[list[str]] = [] + current: list[str] | None = None + for line in lines: + if FINDING_START_RE.match(line): + if current is not None: + blocks.append(current) + current = [line] + elif current is not None: + if line.startswith("#"): # a nested heading ends the block + blocks.append(current) + current = None + else: + current.append(line) + if current is not None: + blocks.append(current) + return [(b[0], "\n".join(b).strip()) for b in blocks] + + +def _first_sentence(bullet: str, limit: int = 160) -> str: + """A one-line summary for the checklist view; `text` carries the whole bullet.""" + text = re.sub(r"^\s*-\s+", "", bullet).strip() + text = re.sub(r"\s+", " ", text) + return text if len(text) <= limit else text[: limit - 1].rstrip() + "…" + + +def extract_items(body: str, suggestions: list[dict]) -> list[dict]: + trail = {} + for rec in _vp.extract_trail_records(body): + for ref in rec.get("line_refs") or []: + trail.setdefault(ref, rec.get("raw", "").strip()) + + items: list[dict] = [] + seen: set[str] = set() + + def add(item: dict) -> None: + # Two findings can share a line anchor (a contradicted claim and a + # readthrough flag on the same range). Suffix rather than collapse: + # dropping one would under-report the worklist. + base = item["id"] + n = 2 + while item["id"] in seen: + item["id"] = f"{base}#{n}" + n += 1 + seen.add(item["id"]) + items.append(item) + + for prefix in PLAIN_BUCKETS: + heading, blocking, optional = BUCKETS[prefix] + span = _vp.find_section(body, heading) + section = body.splitlines()[span[0]:span[1]] if span else [] + for first, full in _bullet_blocks(section): + anchor = _vp.extract_bullet_prefix(first) or "L?" + add({ + "id": f"{prefix}:{anchor}", + "bucket": prefix, + "anchor": anchor, + "blocking": blocking, + "optional": optional, + "summary": _first_sentence(first), + "text": full, + "trail": trail.get(anchor, ""), + }) + + low_blocks, style_lines = _split_low_confidence(body) + _, low_blocking, low_optional = BUCKETS["low"] + for first, full in low_blocks: + anchor = _vp.extract_bullet_prefix(first) or "L?" + add({ + "id": f"low:{anchor}", + "bucket": "low", + "anchor": anchor, + "blocking": low_blocking, + "optional": low_optional, + "summary": _first_sentence(first), + "text": full, + "trail": trail.get(anchor, ""), + }) + + # Style bullets are grouped under an `##### ` H5 per file, so this + # walk tracks the heading rather than reusing _bullet_blocks wholesale; + # continuation lines still fold into the bullet they belong to. + _, style_blocking, style_optional = BUCKETS["style"] + current_file = "" + last: dict | None = None + for line in style_lines: + heading = STYLE_FILE_HEADING_RE.match(line.strip()) + if heading: + current_file = heading.group(1) + last = None + continue + m = STYLE_BULLET_RE.match(line) + if m: + last = { + "id": f"style:{current_file or '?'}:L{m.group(1)}", + "bucket": "style", + "anchor": f"L{m.group(1)}", + "file": current_file, + "blocking": style_blocking, + "optional": style_optional, + "one_click": False, + "summary": _first_sentence(line), + "text": line.strip(), + "trail": "", + } + add(last) + continue + if last is not None and line.strip() and not line.startswith("#"): + last["text"] += "\n" + line.rstrip() + + # Mark the style items that have a live one-click button, and surface any + # posted suggestion the pinned block doesn't carry (a stale ✏️ annotation + # would otherwise hide it). + by_key = {it["id"]: it for it in items} + for sug in suggestions: + path = str(sug.get("path") or "") + line_no = sug.get("line") or sug.get("original_line") + if not path or not line_no: + continue + key = f"style:{path}:L{line_no}" + target = by_key.get(key) + if target is not None: + target["one_click"] = True + target["comment_id"] = sug.get("id") + continue + add({ + "id": key, + "bucket": "style", + "anchor": f"L{line_no}", + "file": path, + "blocking": False, + "optional": False, + "one_click": True, + "comment_id": sug.get("id"), + "summary": _first_sentence(str(sug.get("body") or "").replace(SUGGESTION_MARKER, "")), + "text": str(sug.get("body") or ""), + "trail": "", + }) + + return items + + +# ---- state ------------------------------------------------------------------- + + +def load_state(path: Path) -> dict[str, dict]: + """Read a disposition state file. + + Accepts both the full form (`{"items": {id: {disposition, note}}}`) and the + shorthand a hand-edit tends to produce (`{id: "fixed"}`). + """ + # A missing file is the first run, not an error: the skill passes --state on + # every invocation, including the one that builds the list to begin with. + if not path.exists(): + return {} + try: + data = json.loads(path.read_text(encoding="utf-8")) + except (OSError, json.JSONDecodeError) as exc: + raise SystemExit(f"review-worklist: cannot read state file {path}: {exc}") + if isinstance(data, dict) and isinstance(data.get("items"), dict): + data = data["items"] + if not isinstance(data, dict): + raise SystemExit(f"review-worklist: state file {path} is not an object") + out: dict[str, dict] = {} + for key, value in data.items(): + if isinstance(value, str): + value = {"disposition": value} + if not isinstance(value, dict): + raise SystemExit(f"review-worklist: state entry {key!r} is not an object or string") + disp = value.get("disposition") + if disp not in DISPOSITIONS: + raise SystemExit( + f"review-worklist: state entry {key!r} has disposition {disp!r}; " + f"expected one of {', '.join(DISPOSITIONS)}" + ) + out[key] = {"disposition": disp, "note": str(value.get("note") or "").strip()} + return out + + +def apply_state(items: list[dict], state: dict[str, dict]) -> list[dict]: + """Attach dispositions to items and flag the ones that don't hold up.""" + for item in items: + rec = state.get(item["id"]) + item["disposition"] = rec["disposition"] if rec else None + item["note"] = rec["note"] if rec else "" + if rec and rec["disposition"] in NOTE_REQUIRED and not rec["note"]: + item["problem"] = f"disposition `{rec['disposition']}` requires a note" + else: + item.pop("problem", None) + known = {it["id"] for it in items} + stale = [ + {"id": key, "disposition": rec["disposition"], "note": rec["note"]} + for key, rec in state.items() + if key not in known + ] + return stale + + +def summarize(items: list[dict], parse_confidence: str, suggestions_ok: bool = True) -> dict: + remaining = [ + it for it in items + if not it.get("optional") and (not it.get("disposition") or it.get("problem")) + ] + counts: dict[str, int] = {} + for it in items: + counts[it["bucket"]] = counts.get(it["bucket"], 0) + 1 + return { + "counts": counts, + "total": len(items), + "resolved": sum(1 for it in items if it.get("disposition") and not it.get("problem")), + "remaining": len(remaining), + "remaining_ids": [it["id"] for it in remaining], + # Clean means "everything was seen and everything was decided". A body + # that didn't parse or a suggestions fetch that failed means the first + # half is unproven, whatever the dispositions say. + "clean": not remaining and parse_confidence == "high" and suggestions_ok, + } + + +# ---- rendering --------------------------------------------------------------- + +BUCKET_LABEL = { + "outstanding": "🚨 Outstanding β€” must be fixed or refuted before merge", + "low": "⚠️ Low-confidence β€” each needs a decision, none block the PR", + "style": "✏️ Style suggestions β€” advisory; ✏️ marks a one-click apply", + "pre-existing": "πŸ’‘ Pre-existing β€” optional; not introduced by this PR", +} + + +def render_markdown(report: dict) -> str: + out: list[str] = [] + s = report["summary"] + header = f"# Review worklist β€” {report['remaining_label']}" + out.append(header) + out.append("") + if report["parse_confidence"] != "high": + out.append("> **Parse confidence: low.** The pinned review did not parse into buckets; " + "work from the comment itself and treat this list as incomplete.") + out.append("") + if not report.get("suggestions_ok", True): + out.append("> **Inline suggestions could not be fetched.** Any ✏️ one-click suggestions on " + "the Files-changed tab are missing from this list β€” check them by hand.") + out.append("") + out.append(f"{s['resolved']} of {s['total']} items dispositioned Β· " + f"{s['remaining']} still needing a decision") + out.append("") + for bucket in ("outstanding", "low", "style", "pre-existing"): + rows = [it for it in report["items"] if it["bucket"] == bucket] + if not rows: + continue + out.append(f"## {BUCKET_LABEL[bucket]}") + out.append("") + for it in rows: + box = "x" if it.get("disposition") and not it.get("problem") else " " + tail = "" + if it.get("disposition"): + tail = f" β€” **{it['disposition']}**" + if it.get("note"): + tail += f" ({it['note']})" + if it.get("problem"): + tail += f" ⚠️ {it['problem']}" + if it.get("one_click"): + tail += " ✏️" + out.append(f"- [{box}] `{it['id']}` {it['summary']}{tail}") + out.append("") + if report["stale_state"]: + out.append("## Recorded against findings no longer in the review") + out.append("") + for row in report["stale_state"]: + out.append(f"- `{row['id']}` β€” {row['disposition']} (gone from the current review)") + out.append("") + return "\n".join(out).rstrip() + "\n" + + +def _remaining_label(summary: dict, items: list[dict]) -> str: + if not summary["clean"]: + if not summary["remaining"]: + # Everything listed was decided, but the list itself isn't trustworthy. + return "every listed item decided β€” but the list is incomplete" + return f"{summary['remaining']} item(s) still open" + untouched_optional = sum(1 for it in items if it.get("optional") and not it.get("disposition")) + if untouched_optional: + return f"all required items dispositioned βœ… ({untouched_optional} optional left alone)" + return "all items dispositioned βœ…" + + +def build_report(body: str, suggestions: list[dict], state: dict[str, dict], pr: int | None, + repo: str, suggestions_ok: bool = True) -> dict: + parse_confidence = "high" if _vp.extract_count_table_row(body) else "low" + items = extract_items(body, suggestions) + stale = apply_state(items, state) + summary = summarize(items, parse_confidence, suggestions_ok) + head = HEAD_SENTINEL_RE.search(body) + return { + "pr": pr, + "repo": repo, + "reviewed_sha": head.group(1) if head else None, + "parse_confidence": parse_confidence, + "suggestions_ok": suggestions_ok, + "counts_table": _vp.extract_count_table_row(body), + "items": items, + "stale_state": stale, + "summary": summary, + "remaining_label": _remaining_label(summary, items), + } + + +# ---- self-test --------------------------------------------------------------- + +_FIXTURE = """ +## Pre-merge Review β€” Last updated 2026-08-20T10:00:00Z + + +| 🚨 Outstanding | ⚠️ Low-confidence | πŸ’‘ Pre-existing | βœ… Resolved | +| :---: | :---: | :---: | :---: | +| **1** | **1** | **1** | **0** | + +### πŸ” Verification trail + +
+2 claims extracted + +- L40 in `content/docs/a.md` "Pulumi supports 9 languages." β†’ ❌ contradicted (evidence: six) +- L12 in `content/docs/a.md` "Teams often do X." β†’ 🀷 unverifiable (evidence: none) + +
+ +### 🚨 Outstanding in this PR + +*These must be resolved or refuted before merging.* + +- **[L40]** The language count is wrong; the docs say six. + + Suggested fix: + + ```diff + - nine languages + + six languages + ``` + +### ⚠️ Low-confidence + +*Review each and resolve as appropriate β€” these don't block the PR.* + +- **[L12]** Unattributed "teams often" claim β€” can you cite it? + +#### Style suggestions + +*Pattern-based linting; advisory.* + +##### `content/docs/a.md` + +- **line 88:** "Simply run" β†’ "Run" ✏️ +- **line 91:** "utilize" β†’ "use" + +### πŸ’‘ Pre-existing issues in touched files + +- **[L7]** Heading is title case; house style is sentence case. + +### πŸ“œ Review history + +- 2026-08-20T10:00:00Z β€” initial review (abc1234) +""" + + +def self_test() -> int: + failures: list[str] = [] + + def check(label: str, cond: bool) -> None: + if not cond: + failures.append(label) + + body = join_pages(_FIXTURE) + check("marker stripped", "" not in body) + + items = extract_items(body, []) + ids = [it["id"] for it in items] + check("outstanding parsed", "outstanding:L40" in ids) + check("low-confidence parsed", "low:L12" in ids) + check("style bullets parsed", "style:content/docs/a.md:L88" in ids) + check("second style bullet parsed", "style:content/docs/a.md:L91" in ids) + check("pre-existing parsed", "pre-existing:L7" in ids) + check("style not counted as low-confidence", sum(1 for i in items if i["bucket"] == "low") == 1) + check("four buckets, five items", len(items) == 5) + + outstanding = next(i for i in items if i["id"] == "outstanding:L40") + check("trail attached", "contradicted" in outstanding["trail"]) + check("outstanding blocks", outstanding["blocking"] is True) + # The fix prose and patch below a bullet's first line are the part the + # author has to act on β€” they must survive into `text`. + check("multi-line bullet captured", "+ six languages" in outstanding["text"]) + check("summary stays one line", "\n" not in outstanding["summary"]) + check("block stops at the next bullet", + "Unattributed" not in outstanding["text"]) + check("pre-existing optional", + next(i for i in items if i["bucket"] == "pre-existing")["optional"] is True) + + # Inline suggestion marks its bullet and adds an unlisted one. + # The exported bullet rule is the shared one, not a look-alike copy. + check("bullet rule is the shared one", FINDING_START_RE is _vp.FINDING_START_RE) + + sugs = [ + {"id": 1, "path": "content/docs/a.md", "line": 88, "body": SUGGESTION_MARKER + "\nx"}, + {"id": 2, "path": "content/docs/b.md", "line": 5, "body": SUGGESTION_MARKER + "\ny"}, + ] + items2 = extract_items(body, sugs) + marked = next(i for i in items2 if i["id"] == "style:content/docs/a.md:L88") + check("one-click marked", marked["one_click"] is True and marked["comment_id"] == 1) + check("orphan suggestion surfaced", "style:content/docs/b.md:L5" in [i["id"] for i in items2]) + + # Nothing dispositioned β†’ nothing clean; optional items don't hold it back. + r0 = build_report(body, [], {}, 20123, DEFAULT_REPO) + check("all open initially", r0["summary"]["remaining"] == 4) + check("not clean when open", r0["summary"]["clean"] is False) + check("sha sentinel read", r0["reviewed_sha"] == "abc1234") + + state = { + "outstanding:L40": {"disposition": "fixed", "note": ""}, + "low:L12": {"disposition": "refuted", "note": ""}, + "style:content/docs/a.md:L88": {"disposition": "fixed", "note": ""}, + "style:content/docs/a.md:L91": {"disposition": "accepted", "note": ""}, + } + r1 = build_report(body, [], state, 20123, DEFAULT_REPO) + check("note-required flagged", + r1["summary"]["remaining_ids"] == ["style:content/docs/a.md:L91"]) + state["style:content/docs/a.md:L91"]["note"] = "term of art in this page" + r2 = build_report(body, [], state, 20123, DEFAULT_REPO) + check("clean once noted", r2["summary"]["clean"] is True) + check("optional item stays optional", r2["summary"]["remaining"] == 0) + + state["outstanding:L999"] = {"disposition": "fixed", "note": ""} + r3 = build_report(body, [], state, 20123, DEFAULT_REPO) + check("stale state reported", [s["id"] for s in r3["stale_state"]] == ["outstanding:L999"]) + + # The suggestions fetch reports completeness, not just contents. Stub `run` + # rather than gh: fetch_inline_suggestions resolves it from module globals. + global run + real_run = run + try: + run = lambda _args: '[]\n[]\n' + check("every page decodes β†’ ok", fetch_inline_suggestions("o/r", 1) == ([], True)) + run = lambda _args: '' + check("silent gh failure fails closed", fetch_inline_suggestions("o/r", 1)[1] is False) + run = lambda _args: '{"message": "Not Found"}\n' + check("error object fails closed", fetch_inline_suggestions("o/r", 1)[1] is False) + # The quiet one: page 1 parses, page 2 doesn't. A short list that reads + # as complete is the same defect as an empty one that reads as complete. + run = lambda _args: '[{"id": 1, "path": "a.md", "line": 3}]\nnot json\n' + partial, partial_ok = fetch_inline_suggestions("o/r", 1) + check("partial decode failure fails closed", partial_ok is False) + check("partial decode still returns what it got", len(partial) == 1) + finally: + run = real_run + + # State-file handling: absent is a first run, shorthand parses, junk is loud. + check("missing state file is empty", load_state(HERE / "no-such-state-file.json") == {}) + with tempfile.TemporaryDirectory() as tmp: + shorthand = Path(tmp) / "s.json" + shorthand.write_text('{"outstanding:L40": "fixed"}', encoding="utf-8") + check("shorthand state parses", + load_state(shorthand) == {"outstanding:L40": {"disposition": "fixed", "note": ""}}) + bad = Path(tmp) / "bad.json" + bad.write_text('{"outstanding:L40": "wontfix"}', encoding="utf-8") + try: + load_state(bad) + check("invalid disposition rejected", False) + except SystemExit: + check("invalid disposition rejected", True) + + # An unparseable body must never read as an all-clear. + r4 = build_report("nothing to see here", [], {}, 20123, DEFAULT_REPO) + check("low parse confidence", r4["parse_confidence"] == "low") + check("low parse is never clean", r4["summary"]["clean"] is False) + + # Nor may a failed suggestions fetch: every item decided, list unproven. + r5 = build_report(body, [], state, 20123, DEFAULT_REPO, suggestions_ok=False) + check("failed suggestions fetch blocks clean", r5["summary"]["clean"] is False) + check("failed fetch reported", r5["suggestions_ok"] is False) + check("label says incomplete, not open", + "incomplete" in r5["remaining_label"] and "still open" not in r5["remaining_label"]) + check("markdown warns about the gap", "could not be fetched" in render_markdown(r5)) + + check("markdown renders", "🚨 Outstanding" in render_markdown(r1)) + + for f in failures: + print(f"FAIL: {f}", file=sys.stderr) + print(f"review-worklist self-test: {'FAILED' if failures else 'passed'}", file=sys.stderr) + return 1 if failures else 0 + + +# ---- entry point ------------------------------------------------------------- + + +def main() -> int: + ap = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + ap.add_argument("--pr", type=int, help="PR number to fetch the pinned review from") + ap.add_argument("--repo", default=DEFAULT_REPO) + ap.add_argument("--body-file", help="parse this pinned body instead of fetching") + ap.add_argument("--suggestions-file", + help="raw `gh api .../pulls/N/comments` JSON array (offline mode)") + ap.add_argument("--state", help="JSON file of recorded dispositions") + ap.add_argument("--format", choices=("markdown", "json"), default="markdown") + ap.add_argument("--require-clean", action="store_true", + help="exit 1 when any non-optional item is still undecided") + ap.add_argument("--self-test", action="store_true") + args = ap.parse_args() + + if args.self_test: + return self_test() + if not args.pr and not args.body_file: + ap.error("one of --pr, --body-file, --self-test is required") + + if args.body_file: + raw = Path(args.body_file).read_text(encoding="utf-8") + else: + raw = fetch_pinned_body(args.repo, args.pr) + if not raw.strip(): + log("no pinned review found β€” is the PR still a draft, or did review short-circuit?") + return 1 if args.require_clean else 0 + body = join_pages(raw) + + if args.suggestions_file: + try: + loaded = json.loads(Path(args.suggestions_file).read_text(encoding="utf-8")) + except (OSError, json.JSONDecodeError) as exc: + raise SystemExit(f"review-worklist: cannot read {args.suggestions_file}: {exc}") + suggestions = [x for x in loaded if isinstance(x, dict)] if isinstance(loaded, list) else [] + suggestions_ok = isinstance(loaded, list) + elif args.pr: + suggestions, suggestions_ok = fetch_inline_suggestions(args.repo, args.pr) + if not suggestions_ok: + log("warning: could not fetch inline style suggestions β€” worklist is incomplete") + else: + # --body-file with no --pr: the caller opted out of the suggestions lane + # deliberately, so completeness is theirs to assert, not ours to doubt. + suggestions, suggestions_ok = [], True + + state = load_state(Path(args.state)) if args.state else {} + report = build_report(body, suggestions, state, args.pr, args.repo, suggestions_ok) + + if args.format == "json": + print(json.dumps(report, indent=2, ensure_ascii=False)) + else: + print(render_markdown(report), end="") + + if args.require_clean and not report["summary"]["clean"]: + return 1 + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.claude/commands/docs-review/scripts/scrape-review-outcomes.py b/.claude/commands/docs-review/scripts/scrape-review-outcomes.py index 4298ab7dc765..55fc69062576 100644 --- a/.claude/commands/docs-review/scripts/scrape-review-outcomes.py +++ b/.claude/commands/docs-review/scripts/scrape-review-outcomes.py @@ -101,8 +101,8 @@ # so the SHA shares its parens with prose. Requiring at least one a-f letter # keeps pure-digit runs (issue numbers, dates) from matching. HISTORY_SHA_RE = re.compile(r"\b(?=[0-9a-f]*[a-f])[0-9a-f]{7,40}\b") -# Column-0 finding-paragraph start, mirroring extract_bucket_bullets. -FINDING_START_RE = re.compile(r"^(?:- )?\*\*\S") +# Column-0 finding-paragraph start β€” the shared rule, not a mirror of it. +FINDING_START_RE = _vp.FINDING_START_RE # Branch prefixes of the repo's own automation; their PRs' review outcomes are # reported separately from human-authored PRs. BOT_BRANCH_PREFIXES = ("content-review/", "fix-broken-links") diff --git a/.claude/commands/docs-review/scripts/validate-pinned.py b/.claude/commands/docs-review/scripts/validate-pinned.py index cc71fa871753..575011cb434f 100755 --- a/.claude/commands/docs-review/scripts/validate-pinned.py +++ b/.claude/commands/docs-review/scripts/validate-pinned.py @@ -199,6 +199,14 @@ # re-entrant review merging a pre-rename body still validates. compose-review.py # emits only the current spelling (its STYLE_HEADING) β€” keep these in sync. STYLE_HEADINGS = ("#### Style suggestions", "#### Style findings") +# What a bucket finding looks like: a column-0 line starting with `**` (with an +# optional `- ` prefix). This is THE bullet-recognition rule for the pinned +# format, exported rather than kept local because every consumer needs the same +# answer to "is this line a new finding?" β€” extract_bucket_bullets and +# extract_finding_paragraphs below, scrape-review-outcomes.py's paragraph walk, +# and review-worklist.py's _bullet_blocks. Four private copies had already +# drifted into existence; one definition is the point. +FINDING_START_RE = re.compile(r"^(?:- )?\*\*\S") EXPECTED_TRAIL_EMOJI = { "verified": "βœ…", "matches": "🀝", @@ -400,10 +408,8 @@ def extract_bucket_bullets(body: str, heading_substring: str) -> list[str]: return [] start, end = span bullets = [] - # Match any column-0 line starting with `**` (with optional `- ` prefix). - finding_re = re.compile(r"^(?:- )?\*\*\S") for line in body.splitlines()[start:end]: - if finding_re.match(line): + if FINDING_START_RE.match(line): bullets.append(line) return bullets @@ -2388,13 +2394,12 @@ def _finding_paragraphs(ctx: Context, heading_substring: str) -> list[tuple[int, if span is None: return [] start, end = span - finding_re = re.compile(r"^(?:- )?\*\*\S") paragraphs: list[tuple[int, str]] = [] current_start = None current: list[str] = [] for i in range(start + 1, end): line = ctx.body_lines[i] - if finding_re.match(line): + if FINDING_START_RE.match(line): if current: paragraphs.append((current_start + 1, "\n".join(current))) current_start = i diff --git a/.claude/commands/pr-review/SKILL.md b/.claude/commands/pr-review/SKILL.md index d281abd88111..b0d509875cf7 100644 --- a/.claude/commands/pr-review/SKILL.md +++ b/.claude/commands/pr-review/SKILL.md @@ -177,6 +177,19 @@ This is the **first big user-facing output**. Render in this order, top to botto Render the whole package in one message. +#### Unresolved-findings check (part of Step 6's output) + +Approving is the moment the review's findings stop being actionable, so say what is about to be merged over. Run the enumerator against the pinned comment: + +```bash +python3 .claude/commands/docs-review/scripts/review-worklist.py --pr "$PR_NUMBER" --format json +``` + +Anything it lists that the PR thread shows no outcome for β€” no fix in the diff, no dispute, no filed issue, no stated reason β€” is being merged over silently. Report it as one line in the Step 6 package (`Merging over: 2 ⚠️ low-confidence, 4 ✏️ style suggestions`) and carry it into the Step 7 recommendation. Two rules: + +- **The maintainer decides.** This is disclosure, not a gate. Merging over advisory findings is a legitimate call and πŸ’‘ Pre-existing never counts against a PR. +- **On your own PR, work them first.** When the PR is the maintainer's own (or one you authored in this session), `/address-review $PR_NUMBER` is the right move before adjudication β€” dispositioning the findings there produces the outcome record the post-merge scrape reads. + ### Step 7: Present action menu Use AskUserQuestion. Adaptive-scenario selection (which menu fires for which finding shape) and per-scenario options live in `pr-review:references:action-menus`. The Step 7 menu chooses *what* to do; auto-merge is decided in Step 8 via the merge toggle, never as a Step 7 option. diff --git a/.claude/commands/shipit/SKILL.md b/.claude/commands/shipit/SKILL.md index 15d790c2be14..c85504786046 100644 --- a/.claude/commands/shipit/SKILL.md +++ b/.claude/commands/shipit/SKILL.md @@ -26,9 +26,9 @@ Finalizes your current work by running quality checks, committing changes, pushi ## Process -**CRITICAL SUCCESS CRITERIA**: Complete all 8 steps in sequence. Every step is mandatory and serves a critical purpose in the workflow. **DO NOT SKIP ANY STEP OR END THE WORKFLOW PREMATURELY!** +**CRITICAL SUCCESS CRITERIA**: Complete all 9 steps in sequence. Every step is mandatory and serves a critical purpose in the workflow. **DO NOT SKIP ANY STEP OR END THE WORKFLOW PREMATURELY!** -**Step Counter**: Display progress before each step as: **[Step X/8]** followed by the step heading. This helps users track progress through the workflow. +**Step Counter**: Display progress before each step as: **[Step X/9]** followed by the step heading. This helps users track progress through the workflow. **References**: This skill uses detailed reference files. Always follow the detailed instructions in these referenced documents when applicable: - `shipit:references:quality-checks` - Quality check procedures and code testing @@ -36,7 +36,7 @@ Finalizes your current work by running quality checks, committing changes, pushi --- -## **[Step 1/8] Context Gathering** +## **[Step 1/9] Context Gathering** **Purpose**: Understand what's changed and check for next steps from previous work. @@ -56,7 +56,7 @@ Finalizes your current work by running quality checks, committing changes, pushi --- -## **[Step 2/8] Quality Checks** +## **[Step 2/9] Quality Checks** Run context-aware quality checks. Scan conversation history to skip redundant checks (lint/build), test inline code snippets in markdown files, and run full tests for program examples. @@ -70,7 +70,7 @@ Display summary and ask user to proceed with `AskUserQuestion`. --- -## **[Step 3/8] Branch Verification** +## **[Step 3/9] Branch Verification** **Purpose**: Prevent accidental commits to master branch. @@ -92,14 +92,14 @@ Display summary and ask user to proceed with `AskUserQuestion`. - Exits the skill 3. **If on feature branch**: - - Display: "[Step 3/8] Skipped - already on feature branch `{branch-name}`" + - Display: "[Step 3/9] Skipped - already on feature branch `{branch-name}`" - Continue to Step 4 **Safety**: Always preview destructive operations (like reset --hard) before executing. --- -## **[Step 4/8] Commit Preparation** +## **[Step 4/9] Commit Preparation** Generate 3 meaningful commit message suggestions based on: - `git diff --stat` output @@ -115,7 +115,7 @@ All messages include: `Co-Authored-By: Claude Sonnet 4.5 --- -## **[Step 5/8] Commit Preview** +## **[Step 5/9] Commit Preview** **Purpose**: Preview exactly what will be committed before execution. @@ -163,7 +163,7 @@ Commands that will run: --- -## **[Step 6/8] Push Changes** +## **[Step 6/9] Push Changes** **Purpose**: Commit and push changes to remote. @@ -219,7 +219,7 @@ Commands that will run: --- -## **[Step 7/8] Create Pull Request** +## **[Step 7/9] Create Pull Request** **Purpose**: Generate and create a pull request with appropriate description. @@ -276,12 +276,14 @@ Commands that will run: 5. **If "Create PR"**: ```bash - gh pr create --title "{title}" --body "$(cat <<'EOF' + gh pr create --draft --title "{title}" --body "$(cat <<'EOF' {body} EOF )" ``` + **Draft, always** β€” per `CONTRIBUTING.md` Β§Draft-first pull requests. Automated review fires on the ready-for-review transition, so opening ready means the review runs against whatever is pushed at that moment. Step 9 offers to mark it ready once the user is done iterating. The exception CONTRIBUTING allows β€” a genuinely trivial typo fix opened straight to ready β€” is the user's call to make, not a default to assume. + **If PR creation fails**: - Display error (auth issues, network, etc.) - Note that changes are still committed and pushed @@ -290,7 +292,7 @@ Commands that will run: --- -## **[Step 8/8] Completion Report** +## **[Step 8/9] Completion Report** **Purpose**: Confirm successful completion and provide next steps. @@ -321,20 +323,47 @@ Commands that will run: 4. **Celebrate**: ``` - 🐿️ Ship it! Your changes are ready for review. + 🐿️ Ship it! Your changes are pushed and the PR is open as a draft. ``` +--- + +## **[Step 9/9] Hand off to the review loop** + +**Purpose**: Shipping isn't finished when the PR exists β€” it's finished when the pre-merge review's findings are. This step makes sure the PR doesn't get opened and forgotten. **Never skip it**, and never end the skill at Step 8. + +**Actions**: + +1. **If the PR is still a draft** (the normal case): state in one line that automated review fires on the ready-for-review transition, and offer to mark it ready now. A draft gets no review, so there is nothing to watch yet. + +2. **Once the PR is ready for review**, offer to watch, with `AskUserQuestion`: + + 1. **Watch for the review** (Recommended) β€” invoke `/address-review {PR} --watch`. Reviews typically post in 5-15 minutes. + 2. **I'll come back to it** β€” tell the user the exact command for later: `/address-review {PR}`. + 3. **Skip the review** β€” accepted, but say once what it costs: findings that are never dispositioned scrape as ignored and tune the pipeline toward flagging more. + +3. **If the user picks watching**, hand control to `/address-review` and follow that skill from its Step 1. Do not re-implement watching or triage here. + +4. **Display the closing line**: + ``` + Next: the pre-merge review. I'll work every finding with you β€” blockers, + low-confidence, and style β€” until each one is fixed, refuted, or explicitly + accepted. That's what "done" means on this repo. + ``` + +**Rules**: ask once, take the answer, don't nag. See `AGENTS.md` Β§PR Lifecycle for AI-Assisted Contributions and `CONTRIBUTING.md` Β§Working the review to zero for the expectation this step implements. + **End of skill** --- ## Critical Workflow Rules -1. **Progress Display**: Always display step number "[Step X/8]" before each heading +1. **Progress Display**: Always display step number "[Step X/9]" before each heading 2. **Sequential Execution**: Never skip ahead - complete each step before moving to next 3. **User Approval**: Get explicit approval before destructive actions (Steps 3, 5, 7) 4. **Error Handling**: If a step fails, don't proceed - offer retry or cancel -5. **Skip Display**: If skipping a step, show "[Step X/8] Skipped - {reason}" +5. **Skip Display**: If skipping a step, show "[Step X/9] Skipped - {reason}" β€” Step 9 is never skippable 6. **Context Preservation**: Store decisions from previous steps to avoid re-asking ## Notes diff --git a/.gitignore b/.gitignore index 13f12cd7975e..49742a5cb422 100644 --- a/.gitignore +++ b/.gitignore @@ -138,6 +138,12 @@ _vendor/ /.applied.paths /.fix-scope-report.json /.review-snapshot/ +# Author-side worklist state for `/address-review` β€” one file per PR, +# recording the disposition (fixed / refuted / deferred / accepted / +# not-applicable) chosen for every finding in that PR's pinned review. Local +# working state for the walkthrough, never a commit. +/.review-worklist-*.json + # Glow-up lane equivalents (`.glowup-backlog.json`, the composed work list, # and `.glowup-scope-report.json`, the scope gate's report). Both are written # to the repo root before the `make lint` re-gate, so an unignored one makes diff --git a/AGENTS.md b/AGENTS.md index 65c3bf9786d3..899c5d2d0cfa 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -269,6 +269,10 @@ Before starting any documentation task, check `.claude/commands/` for a relevant ## PR Lifecycle for AI-Assisted Contributions -Open as draft, mark ready when done. Each ready-transition fires one full review; thrashing draft β†’ ready β†’ draft burns budget. Leave AI authoring trailers in commits (`Co-Authored-By: Claude ...`) β€” stripping them is bad form and changes nothing about which review runs. Don't delete `` comments β€” the re-entrant pipeline edits them in place. A small push that only touches lines carrying outstanding findings refreshes the stale review automatically; otherwise, mention `@claude #update-review` (fix-response / dispute / re-verify) or transition through draft and back to ready. Bare `@claude` (no hashtag) is for ad-hoc help, +Open as draft, mark ready when done. Each ready-transition fires one full review; thrashing draft β†’ ready β†’ draft burns budget. Leave AI authoring trailers in commits (`Co-Authored-By: Claude ...`) β€” stripping them is bad form and changes nothing about which review runs. Don't delete `` comments β€” the re-entrant pipeline edits them in place. A small push that only touches lines carrying outstanding findings refreshes the stale review automatically; otherwise, mention `@claude #update-review` (fix-response / dispute / re-verify) or transition through draft and back to ready. Bare `@claude` (no hashtag) is for ad-hoc help, not for refreshing the review. + +**Shipping a PR includes finishing its review.** A PR is done when every finding the pre-merge review raised β€” 🚨 blockers, ⚠️ low-confidence findings, and ✏️ style suggestions alike β€” has been fixed, refuted, deferred to a filed issue, or explicitly accepted with a stated reason. Clearing 🚨 and stopping is not a shortcut; it is the failure mode the pipeline's own outcome telemetry measures (`ignored_low_confidence` in `scrape-review-outcomes.py`). + +Agents: treat this as expected behavior on every PR you open here, asked for or not. Use the **`/address-review`** skill (`.claude/commands/address-review/SKILL.md`) β€” offer to watch for the review when the PR goes ready for review, walk **every** finding with the user when it lands, and say so plainly (once, then drop it) when a merge would leave items undecided. `python3 .claude/commands/docs-review/scripts/review-worklist.py --pr --state .review-worklist-.json --require-clean` is the machine answer to "is anything still open?" For the full mechanics β€” refresh-pattern details, short-circuit thresholds, classifier internals β€” see `CONTRIBUTING.md` Β§AI-assisted contributions. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 35911230d970..cd6c7639f1d3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -48,6 +48,29 @@ A pinned review goes **stale** when you push new commits after it ran. One case Rare. Use when the pinned-review state is corrupted (the 1/M comment was manually deleted, the comment sequence is malformed, the review is stuck in a wrong state that `#update-review` can't reconcile). Clears every existing `` comment and dispatches a fresh initial review from scratch β€” same workflow that fires on ready-for-review, just bypassing the trivial / frontmatter-only / draft / bot-author skips. Don't use it for routine refreshes; `#update-review` is the right tool for those. +### Working the review to zero + +A review is finished when every finding it raised has an outcome β€” not when the 🚨 count hits zero. The pinned comment carries three actionable buckets and they are all yours: **🚨 Outstanding** (must be resolved or refuted before merge), **⚠️ Low-confidence** (doesn't block, still needs a decision), and the **✏️ style suggestions** posted inline on the Files-changed tab. πŸ’‘ Pre-existing is the one optional bucket β€” that's not debt this PR created. + +Five outcomes count as done, and no others: + +| Outcome | When | What it takes | +|---|---|---| +| **Fixed** | The finding is right | Push the change. A small push that lands only on flagged lines refreshes the review by itself | +| **Refuted** | The finding is wrong | Dispute it in a `@claude #update-review` mention, with evidence. The model concedes cleanly or explains why it's holding | +| **Deferred** | Real, but out of scope here | File an issue and link it in the PR thread | +| **Accepted** | Shipping as-is on purpose | Say why, in the PR. An accepted blocker that nobody explained reads as an oversight | +| **Not applicable** | The finding mis-anchored | Say what it actually points at. Several of these in one review usually means the review went stale β€” refresh it | + +This matters past your own PR. After a PR closes, `scrape-review-outcomes.py` derives what happened to each finding and aggregates it into the Monday `#docs-ops` digest, which is how the review's severity rules get tuned. A finding you fixed but never refreshed scrapes as ignored; a finding you disagreed with but never disputed scrapes as ignored too. Both push the pipeline toward flagging *more*, not less. + +Working the list by hand is fine. If you'd rather not, **`/address-review`** does it with you: it watches for the review to land, enumerates every item β€” inline style suggestions included β€” into one checklist, walks them a finding at a time with a proposed fix for each, batches the fixes into a single push, and writes the `#update-review` mention. It won't call the PR done while anything is undecided. The same check standalone: + +```bash +python3 .claude/commands/docs-review/scripts/review-worklist.py --pr \ + --state .review-worklist-.json --require-clean +``` + ### Don't fight the pinned comment The `` comments are managed by the pipeline. Don't delete them β€” the re-entrant skill expects to find and edit them in place. If you accidentally delete the 1/M summary, the next run posts fresh at the bottom of the timeline; recoverable but ugly.