Review loop: fix dropped forced reviews, stale labels, REVIEW_STATE gaps, and id reuse

[CamSoper]

Proposed changes

This PR fixes the review-loop rough edges from the 2026-09-28 survey of open PRs. I root-caused each item from run logs, debug artifacts, and the PR timelines before fixing it. Every fix has a regression test, and two of the tests replay captured drafts from failed production runs. make test-review-pipeline and make lint both pass.

Session: https://claude.ai/code/session_01GtKzf3eR8238vqCLcc1qZt

#	Item	Root cause (evidence)	Fix	Test
1a	#21936: workprentice's #new-review never landed	claude-new.yml had no trusted-bot allowlist, although the other three lanes had one. The collaborator API has nothing on a GitHub App, so run 36421951663 logged ✗ User workprentice[bot] has access and ended green having done nothing. The unquoted [bot] in the curl URL is also a glob, so the permission came back empty. The "Pre-merge Review skipped" at 12:27:23 was the unrelated triage-chained run skipping on review:oversized.	Added the same allowlist, read permissions with gh api, and posted a reply on the PR when a request is declined.	test_trusted_bot_allowlist_is_identical_in_every_access_check fails if the four copies drift.
1b	#21760: four triggers before a card appeared	Every run succeeded (claude-update 36200936469 and 36202602015; review runs 36201223550 and 36202816129). #update-review edits the cards in place, far up the timeline, so the only visible signal is a 🚀 on the request.	None: working as designed.	n/a
1c	#21885: the first review errored	v3 validation refused a draft whose model edit deleted the empty ❓ section and the ⚠️ finding F9 (v3-section-order, bucket-split-faithful). v3 has no repair step.	New normalize-v3-draft.py rules: rule 8 restores a deleted 🚨/❓ section in its empty form, and rule 9 re-appends a deleted composed finding verbatim from the evidence base.	The captured #21885 draft is a new fixture: refused, then normalized, then accepted, then idempotent.
2	#21840: no refresh after a push that answered a reviewer; G1 red while G2 green	By design, a synchronize event never runs a review, and auto-refresh only fires off outstanding-issues. G2 scored the stale card ("every finding answered") while G1 rejected it. G1's "push to refresh" advice was wrong for a clean review.	G2 skips with "not current, see G1" unless the stale card still has undecided findings, which stay red. G1 now names #update-review / #new-review. WORKPRENTICE.md tells the bot to comment #update-review after pushing a reviewer's change. I didn't add an auto-refresh for clean reviews: the unattended lane can't raise new findings, so it would only re-stamp a substantive push as reviewed, at the cost of a model call per push.	test_g2_does_not_call_a_stale_card_answered
3a	#21673: review:stale never clears after a merge from master	mark-stale staled a base-only merge, and nothing ever un-staled a label.	New restamp-base-merge.py handles pushes where every commit since the reviewed head is a merge and the PR's +/- lines are unchanged (the same test collect.py already uses). It moves the v3 card's head markers and nothing else, so the composition stamp and the stale-publish guard are unaffected. It re-reads the card right before writing and backs off if a refresh landed meanwhile, and fails closed on a large or binary file whose blob changed. mark-stale and the reconcile cron use it (on a stacked PR whose base predates the script, the step no-ops to the old stale transition), and the cron gains an un-stale pass: a current card gets its terminal label back from count-buckets. The cron re-dispatches the Sentinel after either repair.	test_restamp_base_merge.py (8 cases, including a conflict resolution that changes the PR's lines, a card republished mid-decision, and a changed patchless file)
3b	#21066: the label contradicts the Sentinel; two #update-review requests didn't stick	The update lane did set the label. The reconcile cron then re-staled it every 2 h from the first CLAUDE_REVIEW 1/ comment, which was a legacy monolith from August (run 36206993300). pinned-comment.sh last-reviewed-sha tied the two 1/1 comments the same way.	list_pinned_comments now returns only the v3 author card when one exists, and the cron reads through it. New prune-legacy subcommand, which the v3 publish calls, so the two surfaces never coexist. clear still sweeps everything.	4 new test_pinned_comment.py cases
4	REVIEW_STATE omits conceded findings (#21790 {}; 21801 F1, 21799 F2, 21798 F3, 21796 F1, 21793 F2, 21828 F2)	concede wrote no disposition, so the ✅ concede: annotation was the only record, while the card's note says an id absent from the block is OPEN. Sentinel G2, the worklist, and sla-sweep read row position, so they agreed with each other but not with the card, the README, or review-admin.	concede now records not-applicable (actor update-lane, note conceded: <reason>). That keeps the scraper's "conceded" classification and triggers no closed-set or schema change. Every refresh backfills a disposition for any ✅ row that lacks one, so older cards heal on their next update. The README now states the rule.	test_concede_records_…, test_resolved_rows_without_a_disposition_are_backfilled…
5a	Review errored on #21614 (×6), #21892, #21083	On #21614, five runs were refused by the validator's promote-only rule; master already fixed that (v3_may_demote). The sixth was a concurrent-refresh race ("F3 is already resolved"). #21892: the model wrote a #### F? · Do this block. #21083: PULUMI_BOT_TOKEN hit its rate limit.	New normalizer rule 7 drops an F? detail block. apply-update treats resolve/concede on an already-resolved id as a no-op.	The captured #21892 draft is a new fixture. I replayed all five captured failing drafts against the new normalizer and validator, and all pass.
5b	claude-update "Publish v3 refresh" contract breaks (runs 35626815279, 35650017269, 36193992403)	(1) The model used the card's own word for the bucket: bucket: "blocking". (2) It acted on an id it invented for an untracked ⚠️ bullet. (3) Version skew: the publish chain ran #21908's own half-finished validate-pinned.py against cards composed on master.	(1) Bucket aliases are normalized, and the prompt now lists the allowed values. (2) Actions on unknown ids are dropped with a warning; the finding they failed to name stays open. (3) The deterministic publish chain now runs from a default-branch checkout (.review-tools/), so the PR under review also no longer supplies the scripts that run with the bot token and AWS credentials.	test_add_bucket_uses_the_cards_words, test_an_invented_id_is_dropped…, test_resolve_and_concede_on_a_resolved_finding_are_no_ops
5c	Sentinel 422 No commit found for SHA:	Runs 35875114067 and 35913662488: GitHub fills workflow_run.pull_requests with [2, 71] for runs on master, so every workflow_run-triggered Sentinel evaluated #2 (and posted a status comment there). Run 35938827723 was a single HTTP 500. In both cases an empty head_sha reached the check-run POST.	RUN_PR is trusted only for a pull_request run at that PR's head. The lanes already poke the Sentinel with the right number. Reads retry a 5xx twice (writes never retry). The publish step refuses an empty SHA with a clear error.	test_a_transient_5xx_on_a_read_is_retried
5d	G2 can't read two-part legacy reviews (#21210, #21487, #21149)	Page 2 was posted by pulumi-bot, because the v2 update lane runs on PULUMI_BOT_TOKEN, but G2 read only github-actions[bot] pages.	Legacy pages are accepted from either identity. The v3 role cards stay github-actions[bot]-only.	test_a_legacy_page_the_v2_update_lane_posted_as_pulumi_bot_is_read
6a	Stale briefs after a rework (#21801, #21798, #21799, #21828) and #21790's summary	The brief's "What this PR changes" bullets, its confidence notes, and the card summary are composed once and carried verbatim by every refresh.	Each refresh now drops change bullets naming a repo file (a path or a name with an extension, not a package or site URL) that left the PR, only inside the "What this PR changes" block (it gets --pr-files from the paginated API, renamed files' old paths included); replaces confidence notes that point at settled findings or an empty ⚠️ list with a "settled" note; and, when nothing blocks, removes a summary that asks the author about a counted set of items ("these four need a source from you"), unless it's negated. Repairs only ever remove or neutralize text the card itself contradicts.	test_brief_drift_after_a_rework…, test_a_summary_naming_open_work…, test_change_bullets_that_do_not_name_a_file_are_kept, test_a_summary_that_reports_settled_checks_is_kept
6b	Ids renumber between reviews (#21798)	compose_v3 always started at F1. Worse, record-evidence.merge_dispositions then grafted the old F3's fixed record onto an unrelated new F3.	Ids continue above the prior high_water, read from the live card. #new-review reads it before clearing the cards and passes it through as prior_high_water; the redispatch job forwards it. A forced run that errors or times out before publishing writes <!-- REVIEW_HIGH_WATER n --> on its failure notice, and the next review reads the larger of the card and those bot notices.	test_a_rereview_continues_ids_above_the_prior_high_water, test_redispatch_forwards_every_new_review_input, test_failure_notices_carry_the_prior_high_water, test_review_state.py
6c	#21889's brief describes the pre-rework layout	Partly addressed. Bullet pruning works by filename, and #21889's moved files kept their basenames, so the prose ("new /authors/ grid") survives. Rewriting it needs a model pass, which the update lane doesn't do.	Not fixed deterministically. A rework that big warrants #new-review.	n/a
7a	Leftover "Review errored" comments (#21920, #21787)	Success paths deleted only their own run's spinner.	Both lanes delete earlier bot failure notices once a review lands.	Exercised via the cleanup script's tests.
7b	Triage said "100 files" on #21936	gh pr view --json files returns one 100-file GraphQL page. That also made the classifier's 150-file oversized axis unreachable and cut domain/routing classification off at 100 paths.	Triage and routing swap in the paginated REST list (100 per page) through with-pr-files.sh (temp file + --slurpfile, never argv: a ~1,000-file list overflows the 128 KiB argument cap), and the comment uses changedFiles. pr_file_count takes the larger of the two.	test_oversized_threshold (1,156 changedFiles behind a 100-file page); test_with_pr_files.py (1,200 files through the helper and on into the classifier)
7c	review:oversized while G5 says "not oversized"	The Sentinel read only the label. The opened evaluation beat triage, and a label written with GITHUB_TOKEN fires no event to re-run it.	The label stays the Sentinel's only source of truth. Triage now re-dispatches the Sentinel when it adds review:oversized or moves review:trivial. Now that the 150-file axis is reachable, a push only newly flags a PR by the line axis, as before; the file axis applies on open / ready / reopen. So no open PR that was reviewed under the normal gates gets flipped (G1/G2 skipped, G5 newly red) by this change.	test_oversized_is_the_label_not_the_size, test_triage_re_evaluates_the_sentinel_when_it_moves_a_label_the_gate_reads, test_a_push_never_newly_flags_a_pr_as_oversized_by_file_count
7d	Two "Sentinel"s in the checks list	The Actions job is sentinel and always succeeds.	Not changed. A display name on the job made /pr-review's _is_sentinel (act.py, analyze.py) count it as an ordinary pending check, stalling every stamp preflight; reverted, with a test pinning the default name.	test_the_sentinel_job_keeps_its_default_name

Adversarial review follow-up (6b32a93). An independent review of daddce5 found five confirmed bugs and three plausible ones. All are fixed, each with a test that fails on the old code; the rows above reflect the fixes, and the PR comment answering that review has the per-item detail.

Blast-radius review (e366396, after rebasing onto master). A second independent pass looked for effects on unrelated PRs. It confirmed five: the Sentinel job rename (7d), a push newly flagging >150-file open PRs as oversized (7c), brief repairs dropping renamed-file and site-URL bullets and two correct summaries (6a), 30-per-page file listings, and a red step on stacked PRs whose base predates the restamp script (3a). All five are fixed with tests that fail on the previous head.

Not fixed, and why:

• Add blog post: How to provision a Kubernetes cluster with infrastructure as code #21083 was a PULUMI_BOT_TOKEN rate limit hit inside claude-code-action's startup. It's a quota problem on a shared token, not a pipeline bug.
• I didn't add a v3 model repair pass for validation failures. The deterministic normalizer now handles every captured failure, and a model pass costs a call per failure.

One-time cleanup (I didn't touch anyone's PR). scripts/review-v3/cleanup-review-leftovers.py --repo pulumi/docs --extra-pr 2 is a dry run that lists what the old loop left behind:

• stale labels on reviews that are actually current;
• "Review errored" notices from before the live card was composed;
• legacy pages beside a v3 card (fix(seo): correct unsourceable claims and stale enumerations across blog #21066's monolith, comment 5376850348);
• the Sentinel status comment on Update in the wake of renamepocalypse #2 (comment 5781587676).

Add --apply to perform the writes.

Shared-code note: triage-classify.py (one additive output key, oversized_by_lines) and apply-update.py (new optional --pr-files) are used outside this lane only through the functions changed above. I didn't touch the content-review, glow-up, or digest code.

Related issues (optional)

#21936, #21760, #21885, #21840, #21673, #21066, #21790, #21614, #21892, #21083, #21908, #21761, #21785, #21210, #21487, #21149, #21889, #21801, #21798, #21920, #21787

🤖 Generated with Claude Code

https://claude.ai/code/session_01GtKzf3eR8238vqCLcc1qZt

[github-actions]

Sentinel — merge gate status (preview)

Warning

⚠️ Preview mode — this check is NOT blocking your merge

The Sentinel is in preview (report-only) mode: its check-run concludes neutral whatever the gates below say, so nothing here gates this PR. The rows are informational and safe to ignore for now. It would have concluded failure.

This will be enforced in the near future. Once it is, every row below has to be green before this PR can merge. The rows are the sign-offs a merge needs — some yours, some a reviewer's or a deploy's. Tell us in #docs if one looks wrong.

1 of 5 gates would block this merge once the Sentinel is enforced.

	Gate	What it needs
✅	G1 review-ran	Nothing to do: review current at e36639644.
✅	G2 findings-answered	Nothing to do: every finding answered.
🔴	G3 right-approver	Needs approval from a member of any review team (pulumi/docs-blog-review, pulumi/docs-guild, pulumi/docs-marketing-review, pulumi/docs-tools) or a repository administrator — no qualifying human approval at this head (bot approvals never count).
➖	G4 infra-evidence	Doesn't apply to this PR: no changed path affects the deploy.
➖	G5 oversized-ack	Doesn't apply to this PR: not oversized.

Break glass: a member of a routing team (.github/review-routing.yml teams:) can apply review:waived, which is logged.

_{Evaluated at head e36639644.}

[pulumi-bot]

Your site preview for commit e366396 is ready! 🎉

http://www-testing-pulumi-docs-origin-pr-21948-e3663964.s3-website.us-west-2.amazonaws.com

[github-actions]

Reviewer's guide v5 — not for the author

Tip

Check the ⚠️ items, then approve. Your approval covers only those; links, shortcodes, metadata, and verified claims were checked by machine (receipts on the evidence page). Code samples are read, not compiled.

PR author: nothing here is yours. Your to-do list is the "Author action guide" comment.

Approval needed from: @pulumi/docs-tools — any member's approval satisfies the merge gate.

Note

What this PR changes:

• Review workflows: forced #new-review / #update-review runs are no longer dropped.
• review-label-reconcile.yml and review-sentinel.yml: stale review:* labels reconciled against live card state.
• apply-update.py, normalize-v3-draft.py, review_state.py: REVIEW_STATE gaps filled; finding-id high-water respected so ids aren't reused.
• New cleanup-review-leftovers.py and restamp-base-merge.py scripts, with tests and README entries.
• Normalizer test fixtures captured from PRs Add UnrecognizedClientException coverage and internal links to AWS CLI credential-error page #21885 and Refresh secrets management tools guide: fact corrections, comparison table, FAQ schema #21892.

The blocking risk is a workflow or script regression that drops, mislabels, or mis-gates reviews on every PR. The infra criteria and the fact-check pipeline ran; the only extracted claims came from test fixtures.

Review confidence:

Dimension	Level	Notes
mechanics	HIGH
facts	HIGH
code correctness	MEDIUM	Workflow and script logic read, not executed end to end

⚠️ Check these before approving

Nothing needs a human eye beyond the rubber-stamp list below.

✅ What you can rubber-stamp

• Facts: 18 factual claims checked — 16 verified clean, 2 settled — see the evidence page.
• Mechanics: no internal links added.
• Style: 0 advisory suggestions left with the author; never blocking.

Pre-existing issues in touched files: 0 — details on the evidence page.

Full evidence: verification trail, investigation log, review history.

_{Review v5 · updated 2026-09-28T22:31:43Z · head commit e366396}

---

For the reviewer: something look wrong? Comment @claude <your point> #update-review and the review re-checks with your input.

[github-actions]

Author action guide v5 — nothing blocks merge

Note

Nothing here needs an answer from you. A human reviewer still approves the merge.

This PR hardens the v3 review loop (forced-review dispatch, label reconciliation, REVIEW_STATE handling, finding-id reuse); the review checked the workflow and script changes against the infra criteria.

Full evidence: verification trail, investigation log, review history.

_{Review v5 · updated 2026-09-28T22:31:43Z · head commit e366396}

---

Please don't edit or delete this comment; it's the review's record.

[CamSoper]

@claude pushed daddce5: build-evidence.py now collapses a ⚠️ table whose rows were all filed off even when the "Not your area?" hand-off line sits beside it -- this PR's own v1 brief showed exactly that header-only table. New test in test_compose_v3.py. No other changes. #update-review

---

Generated by Claude Code

[CamSoper]

Independent adversarial review of head daddce5, from the C&C session. make test-review-pipeline passes. Items 1–5 were traced and confirmed. Please fix them or refute each one before this goes back for approval.

Confirmed

1. Triage crashes at about 1,000+ files, and oversized PRs then get a full Opus review. This is a regression on exactly the seo: add canonical, per-page description, and JSON-LD to Node.js TypeDoc reference pages #21936 case. At claude-triage.yml:246-250 and :270-273, the paginated file list goes to jq as a single argv (jq --argjson f "$ALL_FILES"). Linux caps one argument at 128 KiB (MAX_ARG_STRLEN). Reproduced with 1,156 files: 153 KB, Argument list too long, rc 126. Because of set -euo pipefail plus continue-on-error, triage still ends green but applies no review:oversized and posts no TRIAGE_OVERSIZED comment. claude-code-review.yml (about L559) skips only on that label, so the review runs until it times out. The routing step at L270 fails the same way. Fix: write the list to a temp file and use --slurpfile, or merge on stdin (jq -s '.[0] + {files: .[1]}' <(echo "$PR_DATA") files.json).
2. Finding ids are still reused when a #new-review is re-dispatched. The redispatch job at claude-code-review.yml:2691-2697 forwards force, mention_author and ack_target, but not prior_high_water. If claude-new clears the cards and the head moves mid-run, the handoff is superseded, the re-run finds no card and no input, and it starts with PRIOR_HW=0 at F1 again. That's the Add Pulumi HCL tabs to operations and CI pages (MA-594 cluster F) #21798 bug. Fix: forward -f prior_high_water="${{ github.event.inputs.prior_high_water || '0' }}". Also, clear deletes the card before the new review publishes, so an errored or timed-out forced run loses the high-water mark for good. Consider falling back to high_water in S3 latest.json.
3. The reconcile cron restamps or un-stales without re-running Sentinel. review-label-reconcile.yml loops 1 and 3 move the card head or the label but never dispatch review-sentinel.yml. The job has no actions: write, and GITHUB_TOKEN label edits fire no pull_request_target. When mark-stale's restamp fails (fork token, a compare error), the cron repairs the card but G1 stays red: the Refresh SST/CDK post: correct Ion status, fix self-referencing TOC links, cross-link the SST-on-Pulumi piece #21673 symptom. Fix: add actions: write and dispatch Sentinel after each restamp or un-stale.
4. prune_changes_bullets (apply-update.py) drops change bullets that don't name a file. Any > - `X` — … line is removed unless some PR path equals or ends with X. Probed with `restamp_body()`, `review:stale` and `content/docs/iac/`: all were dropped although the PR still changes them. Fix: prune only when X looks like a path (contains / or has an extension), and only inside the "What this PR changes" block.
5. _OPEN_WORK_RE deletes correct summaries on cards where nothing blocks. "the review checked two claims … and confirmed both hold" matches, and so does "a setting your stacks need; the review confirmed …". Either gets silently removed on every refresh. Fix: tighten the regex, or have the model send a replacement summary instead of the line being deleted.

Plausible: fix or justify in the PR body

1. Behavior change: Sentinel's _size_oversized now decides "oversized" by size, not only by label. Open PRs over 150 files that were reviewed before (the old triage count capped at 100) would now skip G1 and G2, so unanswered 🚨 findings stop blocking. They'd also fail G5 until an approval carries sentinel:oversized-ack, which flips PRs that are already approved. A forced #new-review can no longer put an oversized PR back under the normal gates. If this is intended, spell out the migration impact for open PRs. If it isn't, keep the label as the source of truth and make triage label reliably (see item 1).
2. restamp-base-merge.py can overwrite a concurrent publish. It reads the card, makes about 4 API calls, then PATCHes the whole stale body, bypassing pinned-comment.sh's stale-publish guard. An #update-review that lands in that window gets reverted, and the old card is stamped current. Fix: re-fetch and compare immediately before the PATCH.
3. Low severity.
   • same_diff treats a missing patch (large or binary files) as equal, so a conflict edit inside a large file passes as a base-only merge.
   • cleanup-review-leftovers.py decides a notice is old by comparing it to the card's updated_at. Banner stamps and restamps bump that too, so a notice from a real failed later #update-review can get deleted.

Checked and fine:

• No untrusted text reaches run: without going through env:.
• mark-stale and the claude-update publish chain run scripts from the base or default branch.
• No workflow loops, and loops 1 and 3 converge without label thrash.
• Existing cards still parse, and high-water defaults to 0.
• prune-legacy never touches the author card or brief.
• The cleanup script defaults to dry-run, touches open PRs only, and deletes only bot comments.

---

Generated by Claude Code

[CamSoper]

@claude I rebased onto master (b743c3b) and pushed e366396 with the fixes from a blast-radius review of the effects on unrelated PRs. Each new test fails on b743c3b.

1. Sentinel job rename reverted. /pr-review's _is_sentinel (act.py, analyze.py) filters the job by the name "sentinel". With the display name, every stamp preflight waited on it as a pending check. Row 7d now says this.
2. Oversized flip on open PRs. Now that the 150-file axis is reachable, a push that newly labels a PR does it on the line axis alone, the same as before. The file axis applies on open, ready, and reopen. The classifier reports oversized_by_lines, which is an additive key.
3. Brief repairs. A bullet whose lead is a site URL (/docs/…) or a renamed file's old path is now kept; --pr-files includes previous_filename. The summary drop now also needs a count of items and no negation, so "nothing more is needed from you" and "steps that need your input from the IdP console" survive.
4. Pagination. The PR file listings use per_page=100.
5. Stacked PRs. On a base branch that predates restamp-base-merge.py, mark-stale's restamp step now falls back to the old stale transition instead of erroring.

Plausible but not changed: claude-new posts a "needs write access" reply when the permission API fails transiently. That's cosmetic, and it's better than the old silent drop.

make test-review-pipeline and lint pass. #update-review

---

Generated by Claude Code