Skip to content

Glow-ups that come out clean: pre-verify edited claims, bounded post-open autofix, receipts - #21949

Merged
CamSoper merged 8 commits into
masterfrom
claude/glowup-preverify
Sep 29, 2026
Merged

CamSoper merged 8 commits into
masterfrom
claude/glowup-preverify

Conversation

@CamSoper

@CamSoper CamSoper commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Proposed changes

Glow-up PRs keep shipping errors the pre-merge review then catches, and those findings deadlock because the author card asks pulumi-bot to answer. This makes a glow-up PR either open already clean or get fixed after it opens, with receipts, and gives whatever's left a human owner.

1. Pre-verification (primary path) -- scripts/content-review/preverify-glowup.py

After the glow-up model step, the workflow runs the pre-merge review's own claim pipeline (URL fetch, regex + atomic + holistic extraction at standard scrutiny, merge, verify-claims.py) over the pristine→edited patch. At standard scrutiny the extractor reads only + lines, so this checks the edited spans and nothing else: the same claims the review will check, at a cost proportional to the edit.

  • A claim is must-address when the review would block on it: contradicted / mismatch / flagged (🚨), unverifiable (❓), or a framing-drift the edit introduced. Each gets a provenance against the pristine page's verdicts (introduced vs carried).
  • Rounds: verify → narrow repair turn → verify → repair → verify, i.e. at most three verify rounds, the lane's existing fixpoint bound. The repair turn follows a new ### Pre-verification repair section in the skill: correct the claim to what the cited evidence states outright, or revert the edit to the pristine text. Later rounds reuse verdicts for unchanged claim text, so a repair round pays only for what it changed.
  • Deterministic backstop: after the last round, every edited block still carrying a must-address claim is restored to the pristine text (revert). The diff only shrinks, so nothing new needs verifying. verify-glowup-scope.py still bounds the result at publish. A Backlog executed row whose line was reverted moves to Backlog declined, and its id moves in the verdict. Rows with no known line are listed in the receipts for a human to check.
  • Trust: the model steps have Bash, so nothing trusts the workspace except the article under review. Each round, and the backstop, runs from a fresh checkout of the workflow's own commit (.preverify-code, deleted before every checkout so no planted hook or core.fsmonitor survives) with only the edited article copied in, and the composite action (.github/actions/glowup-preverify-round) is loaded from that checkout. Round files and pre-repair snapshots are uploaded as artifacts as soon as they are written, then read back from fresh downloads into cleared directories.
  • Settle: each round records the article's hash. If the article no longer matches the last round (a repair failed, or the round after it crashed), the backstop first restores the pre-repair snapshot that has that hash. If there isn't one, it restores the pristine page and withholds the verdict sentinel, so nothing publishes and the page is recorded incomplete and retried. A receipt can't claim "clean" for text no round verified.
  • The fix lane is untouched: only the glow-up lane iterates, per AGENTS.md (the fix lane's scope gate matches pre-fix ranges from the immutable snapshot).
  • Kill switch: repo variable GLOWUP_PREVERIFY=0.

2. Receipts

A new ## Pre-verification section in every glow-up PR body (composer placeholder, filled by the workflow and never by the model) lists every claim on an edited line with its verdict, source, and outcome (kept / repaired in round N / reverted), plus the reverted blocks and token cost. A machine marker (<!-- PREVERIFY {...} -->) carries clean/degraded/counts. The full JSON goes to the run's preverify artifact (30 days).

3. Bounded post-open autofix (fallback, opt-in) -- content-review-glowup-autofix.yml + scripts/content-review/glowup-autofix.py

This is off until the repo variable GLOWUP_AUTOFIX is '1', so merging turns on no bot pushes. When enabled, it runs when any pre-merge review lane completes (plus a 6-hourly backstop and manual dispatch) and sweeps open pulumi-bot content-review/glowup-* PRs:

  • select (deterministic):
    • Reads review cards from github-actions[bot] comments only.
    • Acts only when the card is current at the head and has open 🚨/❓ items.
    • Stands down for good on any PR a person has pushed to or answered a finding on (@claude F3: … #update-review). A bare @claude #update-review refresh doesn't count, since the Sentinel and the brief tell people to post exactly that.
    • Runs at most one pass per answered head, and at most two per PR. The marker records both the head the pass answered and the head it pushed, and only pulumi-bot's markers count, so deleting one is the only way to reset the budget.
    • After a refute-only pass (no push), escalates once the card refreshes with findings still open.
  • fix (model, unprivileged): the tools are Read,Write,Edit,Glob,Grep plus the brand MCP, with no Bash and no web (--disallowed-tools overrides any allow rule in the PR head's settings), because it reads review text that commenters can influence while holding an API key. For each finding it records fixed / reverted / refuted (with a source from the checkout) / unresolved. A bot can't accept or defer. Nothing runs on that runner after the model except the upload of two files by path.
  • postcheck (deterministic, fresh runner): re-verifies the change with the same pre-verify code, from a clean master checkout with only the article overlaid, builds the patch, and fails closed. Nothing the model could have planted (git config, hooks, Python startup files) exists on this runner. The change leaves the job only when the model step and a non-degraded post-check both succeeded. Otherwise the patch is empty, and every fixed/reverted is recorded unresolved.
  • publish (deterministic, credentialed):
    • Bounds the change to one file, runs verify-glowup-scope.py against master, and runs make lint.
    • Pushes with a plain push (never force), and aborts if the head moved.
    • Posts one @claude F1: … F2: … #update-review comment as pulumi-bot, and accumulates dispositions in a ## Post-open review findings table in the PR body.
  • Human owner: a pass escalates only what it left unresolved. A PR whose refreshed card is still open after the budget is spent escalates through select, which lists what's actually open. Either way it happens once. The routed team (from route-pr.py / review-routing.yml) is requested, a comment names the team and the open ids, and the PR gets content-review/needs-human.

4. The author card no longer addresses pulumi-bot

Cards for content-review/* branches carry <!-- CLAUDE_REVIEW_AUTOMATED_AUTHOR -->, and the callout addresses the requested reviewer. Only content-review/glowup-* cards (marker … autofix) mention the autofix, and only "where enabled"; fix- and retire-lane cards don't. build-evidence._fix_header keeps that audience when it re-stamps the callout.

5. Correction: the survey's "no requested reviewer" was wrong, but the PR body's claim was also wrong

All three open glow-ups do have a team review request (#21939 and #21938 → docs-guild; #21897 → docs-marketing-review via the get-started override). Triage made them. The survey read requested_reviewers, which lists users only; teams are in requested_teams. The glow-up notice's "the PR-review sweep assigns the reviewers" was still false, because no sweep does that. The notice, the skill, and the worker prompt now say what happens: triage requests the routed team, and the autofix escalates to it by name.

6. Found along the way: whole-page claim extraction silently truncated on dense pages

Both LLM extraction passes over the 770-line YAML reference failed with response truncated at max_tokens=8192 (twice) on every glow-up/fix run in September. The page was verified on the 34 regex claims alone, which is part of why the glow-up's number edits there went unchecked. extract-claims-llm.py now retries a truncated call as two halves split at the nearest H2 (two levels deep), and keeps the spent tokens in the usage totals. This touches a shared review script, but the change is minimal, and without it pre-verify would degrade the same way on a large glow-up. Cost change for every lane that uses the extractor: a split happens only on a call that already truncated twice, which previously meant the pass returned nothing. The worst case is 1+2+4 = 7 calls per truncated body per pass, so 14 per file across both passes. H2 detection works in both the whole-file and the standard-scope hunk formats.

Measurement: replay on #21939 and #21897

I had no API key in this session, so instead of re-running the (non-deterministic) glow-ups I replayed pre-verify's real verify / revert / receipts code against the actual verdicts. The pre-merge review ran the same scripts over the same pristine→head diff; its review-claims artifact, plus the glow-up runs' review-snapshot pristine copy and handoff patch, is exactly what round 1 would have produced (preverify-glowup.py verify --verdicts-file).

#21939 (YAML language reference) #21897 (AWS modify-program)
Blocking findings the review raised 7 (F1-F2 🚨, F3-F7 ❓) 2 (F1-F2 🚨)
Flagged by pre-verify round 1 6: F1, F2, F3, F5, F6, F7 2: F1, F2
Not flagged F4: L48 is a diff context line, identical on master; the extractor pulled a claim from it and the review called it "added by this PR". That's post-open fallback territory (refute with master as the source). none
Worst case (repair does nothing; backstop reverts) 7 blocks reverted; 132 of 150 changed lines kept; every flagged span gone from the diff 2 blocks reverted; 65 of 69 changed lines kept
Expected with a repair turn F2's evidence states the count (the source declares five properties), so it can be fixed rather than reverted. The rest are unverifiable restyles or additions, so revert. The evidence states the correct output outright ("2 unchanged"), so it's fixed, leaving the page more correct than master (which says 4 changes. 1 unchanged).

Cost (Opus 5.5 $4/$20/MTok, cache-read $0.20; Sonnet 5 $2/$10):

#21939 #21897
Pre-verify round 1 (extraction + verification) $0.15 + $5.97 (976k in / 80k out / 2.3M cache-read; 40 of 53 claims went to web search) $0.04 + $1.09
Existing glow-up run (model + pristine verification + readthrough) about $3.50 about $2.70
Rounds 2-3 Only changed claim text is re-verified Same

So round 1 costs about what the pre-merge review already spends verifying the same diff: +40% on a typical glow-up and roughly 2.7× on a dense reference page. I prototyped reusing this run's pristine verdicts for claims whose assertion didn't change. The replay showed zero matches on both PRs, because the extractor restates claims differently each run, so I removed it rather than ship dead complexity. A cheaper design would let the pre-merge review reuse pre-verify's verdicts for a PR whose diff is unchanged since pre-verify ran. That trades away the review's independence and belongs to the review pipeline, so I left it out; say if you want it.

Validation

  • make test-review-pipeline: every suite passes, including new self-tests (preverify-glowup.py, glowup-autofix.py), new pytest suites (test_extract_claims_split.py, automated-author tests in test_compose_v3.py), and the existing workflow-sanity / MCP-config checks on the new model steps.
  • make lint: passes. Prettier is clean on the new YAML.
  • preverify-glowup.py end-to-end in --dry-run against the Content glow-up: content/docs/iac/get-started/aws/modify-program.md #21897 article (every pipeline stage invoked, receipts rendered), and in replay mode as above.
  • glowup-autofix.py select against live Content glow-up: content/docs/iac/languages-sdks/yaml/yaml-language-reference.md #21939: fix — 7 open blocking finding(s); pass 1/2.
  • Two rounds of independent adversarial review (see the comment thread). The first, on b7f6561, found four blockers and four should-fixes, fixed in c91cec9f. The follow-up on c91cec9f found two partial fixes and four new items, fixed in edf9fda3. Both rounds added lifecycle self-tests (select → answer → select at the new head, refute-only escalation, human stand-down, card author filter, settle's three outcomes, row moves).
  • Not exercised live: the new workflow steps have not run in Actions yet. The first real glow-up after merge is the integration test for pre-verify (GLOWUP_PREVERIFY=0 turns it off). The autofix stays off until GLOWUP_AUTOFIX=1.

Out of lane, noted

  • author_staleness (review-routing.yml) will close a bot PR at 21 days even though it has no author to go stale. That's the other review-loop session's area; flagging it here only.

Related issues (optional)

#21939, #21897, #21938

🤖 Generated with Claude Code

https://claude.ai/code/session_01KAxPaBAhPHjsfgkAXuuXvq


Generated by Claude Code

@github-actions

github-actions Bot commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

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 78e1d787c.
✅ 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 78e1d787c.

@pulumi-bot

pulumi-bot commented Sep 28, 2026 •

Copy link
Copy Markdown
Collaborator

@CamSoper
CamSoper marked this pull request as ready for review September 28, 2026 19:07
@github-actions github-actions Bot added review:triaging Claude Triage is currently classifying the PR domain:infra PR touches workflows, scripts, infra, Makefile, or build config labels Sep 28, 2026
@pulumi-bot
pulumi-bot requested a review from a team September 28, 2026 19:08
@github-actions github-actions Bot added review:in-progress Claude review is currently running and removed review:triaging Claude Triage is currently classifying the PR labels Sep 28, 2026
@github-actions

github-actions Bot commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Reviewer's guide v7 — 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:

  • preverify-glowup.py and the glowup-preverify-round action: verify claims a glow-up edited before the PR opens.
  • content-review-glowup-autofix.yml and glowup-autofix.py: scheduled, bounded autofix of open glow-up PRs.
  • extract-claims-llm.py: split and retry claim extraction that hits max_tokens, with tests.
  • compose-review.py and compose-pr-body.py: render pre-verify receipts, with tests.
  • review-existing-content/SKILL.md and content-review-article.yml: wire the new round into the glow-up lane.

A regression here would ship glow-up PRs with unverified or wrongly rewritten claims, or let the autofix loop push past its bounds with the bot token. The infra pass read the new workflows for permission scope and input injection (inputs pass through env:) and the scripts for control flow; no content claims were in scope.

Review confidence:

Dimension Level Notes
mechanics HIGH
facts HIGH
code correctness MEDIUM Scripts and workflows were read, not run; the autofix loop only runs on its schedule after merge.

⚠️ Check these before approving

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

✅ What you can rubber-stamp

  • Facts: no factual claims found in the changed lines.
  • 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 v7 · updated 2026-09-29T01:43:31Z · head commit 78e1d78


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

@github-actions

github-actions Bot commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Author action guide v7 — nothing blocks merge

Note

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

This PR adds a pre-verify round for glow-up edits, a bounded post-open autofix workflow, and receipts in the PR body. The review read the new workflows and scripts against the infra criteria.

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

Review v7 · updated 2026-09-29T01:43:31Z · head commit 78e1d78


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

@github-actions github-actions Bot added review:no-blockers Claude review completed cleanly; outstanding is empty and removed review:in-progress Claude review is currently running labels Sep 28, 2026

Copy link
Copy Markdown
Contributor Author

Independent adversarial review of head b7f6561, run from the C&C session. make test-review-pipeline passes, but the lifecycle and trust-boundary paths aren't covered by it. Items 1–4 need fixing before this is ready. The rest should be fixed or answered.

Blocking

  1. The pass bound never escalates (confirmed by simulation). glowup-autofix.py's render_answer / render_pass_marker_only write head={commit}, and publish passes --commit "$COMMIT", which is the new pushed head. When the review refreshes at that head, select() sees "a pass already answered this head" and returns none forever. Pass 2 and the budget-spent escalation never run. A refute-only pass (no push) stalls the same way. Fix: record the answered head (sel["head_sha"]) in the marker, keep the pushed commit in a separate field, and add a self-test for select → answer → select-at-new-head.
  2. Prompt injection into a job that holds secrets. The "Stage inputs" step in content-review-glowup-autofix.yml builds card.md from any issue comment containing <!-- CLAUDE_REVIEW_AUTHOR -->, with no author filter (review-worklist.py / pinned-comment.sh don't filter either). The fix job gives the model Bash, WebFetch, WebSearch, ANTHROPIC_API_KEY in env, id-token: write, and open egress. Anyone who can comment could post a fake card with a matching CLAUDE_REVIEW_HEAD and instructions. Fix: accept the card only from the bot login that posts it, and drop Bash/WebFetch in this job or cut its egress.
  3. The autofix post-check fails open. Export is if: always(), and the post-check runs only when steps.claude.outcome == 'success'. When Claude fails (max-turns or error), partial edits are exported unverified and then pushed. The post-check is also continue-on-error. When verify-claims returns no verdicts, run_pipeline reports degraded with must_address=0, and nothing reads degraded, so fold() keeps fixed. Fix: export an empty patch unless Claude succeeded, the post-check succeeded, and the round isn't degraded.
  4. Pre-verify can ship unverified edits with a "clean" receipt (content-review-article.yml). A failed repairN (continue-on-error) skips pv(N+1). The backstop revert then runs with the stale round-N file against an article that the repair already changed, so the line ranges drift: it reverts the wrong block or reports it unmapped. summarize() can still say clean: true. Fix: snapshot the article before each repair and restore it if the repair fails, or always run a final verify round before revert.

Should fix

  1. The "automation opened this PR" card is too broad. AUTOMATED_AUTHOR_BRANCH_PREFIXES = ("content-review/",) also matches fix-lane and retire-lane branches. Their cards then promise that the autofix answers each item, but the autofix only selects content-review/glowup-. Narrow the prefix, or vary the copy per lane.
  2. The autofix can fight a human (plausible). select() doesn't look for human commits or human #update-review answers, so a reverted disposition can overwrite a human's edit on the branch. Skip any PR with non-bot commits or human answers.
  3. Default-on. An unset GLOWUP_AUTOFIX means the lane is enabled, so merging turns on bot pushes with no opt-in. Make the default off and gate on == '1' (the pattern the SLA sweep uses).
  4. Records drift after a revert. revert_blocks restores a whole difflib opcode block but doesn't update executed_ids or the "Backlog executed" table, so reverted work is still recorded as executed.

Minor

  • After a pass-1 escalation, later passes escalate again without checking escalated().
  • split_user_text never finds H2s in the hunk format (N\t+ ## …) used at standard scrutiny, so it always splits at the midpoint.
  • The split/retry path is shared by every lane that uses extract-claims-llm.py. Worst case it's about 14 calls per file. Note that cost change in the PR body.
  • The pass budget lives in PR comments, so deleting a marker resets it. Acceptable if the author filter from item 2 lands, but say so.
  • Verifier scripts and actions run from workspace dot-paths that model steps with Bash could write, and dot-path changes don't appear in the exported patch. The pre-merge re-check limits the impact. Run the verifiers from a pristine checkout if that's cheap.

No ${{ }} injection of untrusted text in run:: only the numeric PR number is interpolated.


Generated by Claude Code

Copy link
Copy Markdown
Contributor Author

Thanks. All of these were real. All fixed in c91cec9; here's each one.

Blocking

  1. The pass bound never escalates. Fixed. The marker now records answered=<head the pass read the card at> pushed=<head it left>. Only answered decides "already had its pass", so a card refreshed at the pushed head gets pass 2, then escalation. For a refute-only pass (nothing pushed, head unchanged), select waits for the card to refresh after the answer. If findings are still open after that, it escalates once. New self-tests cover select → answer → select at the pushed head, the refute-only wait-then-escalate path, and a marker round trip.

  2. Prompt injection into the fix job. Fixed. A new glowup-autofix.py cards builds the author card and brief from github-actions[bot] comments only. select feeds those to review-worklist.py --body-file/--brief-file (it no longer fetches the card itself), and the fix job gets the same filtered card.md. The fix job's model is now Read,Write,Edit,Glob,Grep plus the brand MCP: no Bash, no WebFetch, no WebSearch. Refutes cite sources in the checkout. Self-test: a fake card from another login is ignored.

  3. The autofix post-check fails open. Fixed. Export ships a patch only when the model step succeeded and the post-check reports ok=true, which it does only for a non-degraded round. Otherwise the patch and status are empty. fold() now takes changed: a fixed/reverted with nothing pushed is unresolved ("no verified change was pushed"). The post-check also now runs inside a fresh master checkout with only the article overlaid, and head.md/base.md are rebuilt after the model step from git show HEAD: and that checkout, because the model's Write tool could reach the old copies.

  4. Pre-verify ships unverified edits with a clean receipt. Fixed. Each round records article_sha256. Before each repair, the article, PR body, and verdict are snapshotted and uploaded as preverify-prerepair-N. A new settle step runs before the backstop:

    • The article matches the last round's hash: proceed as before.
    • A fresh-downloaded pre-repair snapshot has that hash: restore the article, body, and verdict from it, then revert.
    • Neither: restore the pristine page. The receipts then say clean: false, and the publish gate refuses an empty glow-up, so the page is recorded incomplete and retried.

    Self-tests cover all three cases, including a tampered snapshot.

Should fix

  1. The card copy is too broad. Fixed. There are now two markers: CLAUDE_REVIEW_AUTOMATED_AUTHOR for every content-review/* branch, and CLAUDE_REVIEW_AUTOMATED_AUTHOR autofix for content-review/glowup-* only. Only the glow-up copy mentions the autofix, and it says "where enabled". build-evidence._fix_header re-stamps from the marker, and a test pins that fix-lane cards don't mention the autofix.
  2. The autofix can fight a human. Fixed. select stands down, permanently for that PR, when any commit author isn't automation, or when anyone who isn't automation has commented #update-review. pr.json now carries commits. Self-tests cover both.
  3. Default-on. Fixed. The lane now requires vars.GLOWUP_AUTOFIX == '1', so merging doesn't turn on bot pushes. The skill and the glow-up notice say "where enabled". I left GLOWUP_PREVERIFY default-on (!= '0'): it's the primary path you approved, and it only changes what the worker does before a PR opens. Say if you want it opt-in too.
  4. Records drift after a revert. Fixed where the line is known. revert now takes --backlog/--verdict/--body. A Backlog executed row whose line (this run's pristine numbering, from the stub's line_range or fresh_verdict.line_range) overlaps a reverted block ±2 moves to Backlog declined with a "reverted by pre-verification" reason, and its id moves from executed_ids to declined_ids. Rows banked with no line can't be matched, so the receipts list them in a warning for the reviewer to check against the reverted blocks. I'd rather flag those than guess.

Minor

  • Repeat escalation: fixed. select passes escalated, and answer escalates only if it hasn't happened yet.
  • split_user_text H2s: fixed. It strips the + / hunk marker before the H2 test, and falls back to a hunk boundary before the midpoint. New test with standard-scope hunk lines.
  • Split/retry cost: added to the PR body. Worst case is 1+2+4 = 7 calls per truncated body per pass, so 14 per file across both passes. It fires only on a call that has already truncated twice, which before this meant the whole pass returned nothing.
  • Budget in PR comments: noted in the PR body. Only pulumi-bot's markers count, so deleting one is the only way to reset it, and that takes write access to pulumi-bot's comments.
  • Verifiers from writable dot-paths: fixed for both lanes. Each pre-verify round checks out this workflow's own commit into .preverify-code, loads the composite action from that checkout (not the workspace, where the main model's Bash could have edited action.yml), and verifies there with only the article overlaid. .preverify-trusted and .preverify-final are cleared before every download, so a planted round file can't join the merged set. The autofix re-checks out master before its post-check.

Validated: make test-review-pipeline, prettier on the three YAML files, and make lint.


Generated by Claude Code

@github-actions github-actions Bot added review:stale New commits since last Claude review; refresh on next ready-transition or @claude mention and removed review:no-blockers Claude review completed cleanly; outstanding is empty labels Sep 28, 2026

Copy link
Copy Markdown
Contributor Author

@claude pushed c91cec9, which fixes everything in the adversarial review above (pass-marker lifecycle, card author filter, no Bash/web in the autofix model, fail-closed export, settle before the backstop, trusted checkouts, opt-in autofix, row moves). Please re-review the new commit. #update-review


Generated by Claude Code

@github-actions github-actions Bot added review:in-progress Claude review is currently running and removed review:stale New commits since last Claude review; refresh on next ready-transition or @claude mention labels Sep 28, 2026

Copy link
Copy Markdown
Contributor Author

I verified c91cec9f against the earlier adversarial review. make test-review-pipeline passes. I simulated select → answer → select across heads A→B→C: pass 1 at A, pass 2 at B, escalation at C. Items 1, 3–8, repeated escalation, and hunk-format H2 splitting are fixed. What's left:

Partial

  • Item 2, tool restriction. --allowed-tools drops Bash and web access, but no --disallowed-tools is set, and the job runs in the PR-head checkout, whose .claude/settings.json allows Bash(git commit:*). If claude-code-action loads project settings, that narrow Bash path stays open. Write could also plant core.fsmonitor in .git/config, which the later git diff/git status in Export would then run while the step holds ANTHROPIC_API_KEY, the OIDC request token, and open egress. Fix: add --disallowed-tools "Bash,WebFetch,WebSearch", which overrides project allow rules.
  • Verifiers run from paths the model can write. actions/checkout into an existing .preverify-code or .autofix-master keeps that directory's .git. A step with Bash access (the glow-up model before round 1, or repair1/2) can plant .git/hooks/post-checkout or core.fsmonitor, and the next "trusted" checkout runs it. That can rewrite preverify-glowup.py or action.yml. Fix: rm -rf both directories before every checkout, as you already do for .preverify-trusted.

New

  1. Confirmed (predates this commit). Every pass-2 answer escalates (pass >= MAX_PASSES, around glowup-autofix.py:386), even when every finding came back fixed. With no unresolved rows, render_escalation falls back to sel.open, requests the routed team, adds needs-human, and lists findings that are already fixed as "yours to decide". Reproduced in simulation. Escalate only when unresolved rows remain after the pass.
  2. Design. Any bare human @claude #update-review permanently stands the autofix down (:143). But Sentinel and the brief both tell people to post exactly that to refresh a review, and jkodroff's refresh comment on Content glow-up: content/docs/iac/languages-sdks/yaml/yaml-language-reference.md #21939 would already trip it. Count only human answers to findings (or human commits), not bare refresh requests.
  3. Minor. On restored-pristine, settle restores only the article, so the PR body and verdict still describe the executed items. If the glow-up also changed non-markdown files in the bundle, the patch is non-empty and the publish gate lets it through with a misleading body (it's clean:false). Either restore the whole bundle, or regenerate the body from the settled state.
  4. Nit. The cost note in the PR body contradicts itself: "at most 4 calls per body", then "1+2+4 = 7".

Generated by Claude Code

@github-actions github-actions Bot added review:no-blockers Claude review completed cleanly; outstanding is empty and removed review:in-progress Claude review is currently running labels Sep 28, 2026
@github-actions github-actions Bot added review:stale New commits since last Claude review; refresh on next ready-transition or @claude mention and removed review:no-blockers Claude review completed cleanly; outstanding is empty labels Sep 28, 2026
pulumi-bot opens content-review PRs and never reads the author card, so a
card telling 'the PR author' to answer addressed nobody. Cards for
content-review/* branches carry CLAUDE_REVIEW_AUTOMATED_AUTHOR and address
the requested reviewer; build-evidence keeps that audience when it
re-stamps the callout.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KAxPaBAhPHjsfgkAXuuXvq
When the pre-merge review still blocks a glow-up after it opens, one
pass per head (at most two per PR) fixes, refutes, or reverts each open
finding, re-verifies its own change with preverify-glowup.py, pushes as
pulumi-bot, answers the card with one #update-review mention, and
records the dispositions in the PR body. Whatever it can't settle goes
to the routed team by name, once. Kill switch: GLOWUP_AUTOFIX=0.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KAxPaBAhPHjsfgkAXuuXvq
Whole-page extraction on dense pages truncated at max_tokens=8192 on both
passes (the Pulumi YAML reference, every run in September 2026), so the
content-review lanes verified those pages on the regex floor alone. A
truncated call now retries as two halves split at the nearest H2, two
levels deep, and the spent tokens stay on the books.

Also: GLOWUP_PREVERIFY=0 kill switch for the pre-verification rounds;
replay-mode (--verdicts-file) for measuring pre-verify on past PRs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KAxPaBAhPHjsfgkAXuuXvq
… checks

- Autofix pass markers record the answered head separately from the
  pushed head, so a refreshed card gets pass 2 and escalation; a
  refute-only pass escalates once the card refreshes with findings open.
- Review cards come from github-actions[bot] only; the fix job's model
  has no Bash or web tools.
- The autofix post-check runs from a fresh master checkout on inputs
  rebuilt from git, and the change is withheld unless the model and a
  non-degraded post-check both succeeded.
- Pre-verify rounds record the article hash; settle restores the
  pre-repair snapshot (or the pristine page) before the backstop reverts.
  Rounds run from a trusted checkout of the workflow's own commit.
- Reverted blocks move matched Backlog executed rows to declined.
- The autofix stands down on human commits or answers and is opt-in
  (GLOWUP_AUTOFIX == '1'); the card promises the autofix only on glow-up
  branches.
- split_user_text finds H2s in standard-scope hunks.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KAxPaBAhPHjsfgkAXuuXvq
…lation

- The autofix post-check and patch build move to their own job on a
  fresh runner; nothing runs after the model on its runner except a
  two-file upload. The model also gets --disallowed-tools for Bash and
  web, which overrides project allow rules.
- The trusted pre-verify checkout is deleted before every checkout, so a
  planted hook or core.fsmonitor can't run.
- A pass escalates only what it left unresolved; a budget-spent PR
  escalates through select with what is actually open.
- A bare human '@claude #update-review' refresh no longer stands the
  autofix down; only answers to findings (or human commits) do.
- settle's restored-pristine outcome also withholds the verdict, so a
  bundle-asset change can't publish under a stale body.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KAxPaBAhPHjsfgkAXuuXvq
- settle's restored-pristine case now writes .content-review-incomplete.json,
  carried in the review handoff; record-review.py --incomplete-marker records
  the no-sentinel run incomplete (retried) instead of derived clean.
- glowup-autofix: neutralize HTML comments in model-supplied note/source
  (and rendered summaries), and read the pass/escalation markers only as the
  comment's first line, so a note can't forge or suppress either marker.
- human_activity: a commit author with no linked login is a person unless its
  email is one of our automation identities (pulumi-bot, the Claude
  co-author trailer, *[bot] noreply).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FHDG7o9KCZvdnEdBZtW6qq
@CamSoper
CamSoper force-pushed the claude/glowup-preverify branch from 78d194b to bc86ed7 Compare September 28, 2026 23:03

Copy link
Copy Markdown
Contributor Author

@claude rebased onto master (now bc86ed71, on top of #21948's review-loop fixes). The only conflict was in test_compose_v3.py, where both sides appended tests; I kept both. No other changes. make test-review-pipeline and make lint pass. Please refresh the review at the new head. #update-review


Generated by Claude Code

@github-actions github-actions Bot added review:stale New commits since last Claude review; refresh on next ready-transition or @claude mention review:in-progress Claude review is currently running review:no-blockers Claude review completed cleanly; outstanding is empty and removed review:no-blockers Claude review completed cleanly; outstanding is empty review:stale New commits since last Claude review; refresh on next ready-transition or @claude mention review:in-progress Claude review is currently running labels Sep 28, 2026
CamSoper pushed a commit to CamSoper/pulumi.docs that referenced this pull request Sep 29, 2026
…oke hooks

Never PR upstream. Fork sandbox for smoke-testing pulumi#21949.
The three pre-verify round gates compared `vars.GLOWUP_PREVERIFY != '0'`
bare. An unset variable is null, and GitHub coerces null-vs-'0' to numbers
(0 != 0 is false), so unset meant OFF: every round was skipped and the
receipts said "did not run". A fork smoke run caught it. Wrap the variable
in format('{0}', ...) like every other kill switch in the repo, and add a
test that fails on any bare `vars.X ==/!= '0'` in a workflow.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FHDG7o9KCZvdnEdBZtW6qq
@github-actions github-actions Bot added review:stale New commits since last Claude review; refresh on next ready-transition or @claude mention and removed review:no-blockers Claude review completed cleanly; outstanding is empty labels Sep 29, 2026
CamSoper pushed a commit to CamSoper/pulumi.docs that referenced this pull request Sep 29, 2026
…oke hooks

Never PR upstream. Fork sandbox for smoke-testing pulumi#21949.

Copy link
Copy Markdown
Contributor Author

Fork smoke test found a real bug, fixed in 78e1d78.

The three pre-verify round gates used a bare vars.GLOWUP_PREVERIFY != '0'. When the variable is unset it's null, and GitHub coerces null vs '0' to numbers (0 != 0 is false), so unset meant off: every round got skipped and the receipts said "Pre-verification did not run". That's the same trap review-existing-content.yml / claims-reverify.yml / blog-review-index.yml already document. Two fork glow-ups (CamSoper/pulumi.docs runs 36506585325 / 36506587608) reproduced it: rounds skipped, a PR opened with the "did not run" warning. The fix wraps the variable in format('{0}', ...) like the other kill switches, and the new scripts/content-review/test_workflow_var_gates.py fails on any bare vars.X ==/!= '0' in a workflow (it fails on the old gate and passes now).

Re-running the fork smoke on the fixed head to check the rounds and the restored-pristine → incomplete ledger path end to end.

@claude pushed 78e1d78: pre-verify gate null-coercion fix plus a workflow guard test #update-review


Generated by Claude Code

@github-actions github-actions Bot added review:in-progress Claude review is currently running review:no-blockers Claude review completed cleanly; outstanding is empty and removed review:stale New commits since last Claude review; refresh on next ready-transition or @claude mention review:in-progress Claude review is currently running labels Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

Fork smoke test on the fixed head (78e1d78, plus a fork-only ESC bypass on CamSoper/pulumi.docs) passed end to end:

  • Normal glow-up (options/protect.md, run 36508906546): round 1 ran, 7 claims on 8 edited lines, 0 must-address, settle: consistent, receipts clean=True. The PR opened with the full receipts table and the ledger reads status: glowup.
  • Forced restored-pristine (options/ignorechanges.md, run 36508908774; the smoke hook perturbs the article and drops the pre-repair snapshots before settle): preverify settle: restored-pristine, the verdict was withheld, the gate printed no verdict sentinel; nothing to publish, and no PR opened. .content-review-incomplete.json made it through the handoff, and the ledger reads status: incomplete, attempts: 1, note verdict withheld: pre-verification restored the pristine page; .... That exercises fix Switch to using typedoc for TypeScript documentation. #1 from the verification round in Actions, not just in unit tests.

Not covered: the autofix workflow (it's still off behind GLOWUP_AUTOFIX) and the real S3 ledger write (the fork has no OIDC trust, so the record was written locally and uploaded as an artifact).

Minor, not fixed: on the restored-pristine path the receipts keep the generic "Every claim on an edited line is verified…" line under the CAUTION. It only shows in the preverify artifact because no PR body is posted on that path.

The fork's master is back to where it was, and the smoke PRs are closed.


Generated by Claude Code

@CamSoper
CamSoper merged commit adca0a5 into master Sep 29, 2026
19 checks passed
@CamSoper
CamSoper deleted the claude/glowup-preverify branch September 29, 2026 17:38

This branch was successfully deployed

1 active deployment
testing — 78e1d787 Deployed Sep 29, 2026 by CamSoper via Do cleanup #14495
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain:infra PR touches workflows, scripts, infra, Makefile, or build config review:no-blockers Claude review completed cleanly; outstanding is empty

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants