Skip to content

Fit review cards again at publish so a stale PR head can't error the review - #22386

Merged
CamSoper merged 1 commit into
masterfrom
camsoper/amazing-hawking-aqo20s
Oct 20, 2026
Merged

CamSoper merged 1 commit into
masterfrom
camsoper/amazing-hawking-aqo20s

Conversation

@CamSoper

Copy link
Copy Markdown
Contributor

Proposed changes

#22287's pre-merge review errored after a clean model pass. The publish job's pinned-comment.sh refused the author card: 110,327 bytes against the 60,000-byte budget.

Root cause: that branch was cut on 2026-10-12, a day before the card budget landed in #22269. The initial review lane runs build-evidence.py from the PR head, so the head's old copy built the card and never measured it. The publish job uses the default branch's pinned-comment.sh, which enforces the budget, but it never re-fit the card. (The update lane already runs default-branch tooling, so it doesn't have this gap.) This is the same "workflow from master, scripts from the PR head" problem the workflow already works around for --pr-diff, --approval-scope, and ack-request.sh.

Changes:

  • build-evidence.py gets a --fit AUTHOR BRIEF mode. It fits two finished cards in place with the existing fit_author_card / fit_brief_card and exits. A card that already fits comes back byte-identical.
  • The publish job runs it from its default-branch checkout right before pinned-comment.sh. It's best-effort: if the fit itself fails, the size check is still the backstop, so a publish that would have worked before still works.
  • New regression test in test_card_budget.py: it builds an unfitted card the way an older head would, runs the CLI on it, checks the card fits, keeps every row, and passes validate-pinned.py, and checks the no-op case. The test fails against the current build-evidence.py (unrecognized arguments: --fit).
  • README section and the pinned-comment.sh error hint now mention the publish-time fit.

Replayed against #22287's real handoff artifact: the card went from 110,267 to 35,262 bytes (❓ links and ❓ Do-this blocks shed). All 4 🚨 and 114 ❓ rows are kept, and the fitted card passes validate-pinned.py. Nothing is lost: the old head's build() already copied every Do-this block into evidence detail, and the evidence page shows them.

make test-review-pipeline passes. make lint doesn't cover these files (.github/ and *.md are prettier-ignored, and the markdown lint only checks content/).

Note for #22287 itself: its review will still run its own stale composer until the branch merges master. With this change the card publishes anyway, but merging master into that branch would also pick up the current card wording (the old card still tells authors to use the retired @claude … #update-review form).

Related issues (optional)


Generated by Claude Code

…review

#22287's review errored after a clean model pass: the author card
came in at 110 KB and pinned-comment.sh refused it against its 60,000-byte
budget. The branch was cut on 2026-10-12, before the card budget landed
(#22269), and the initial lane runs build-evidence.py from the PR head, so
the card was never measured. The update lane already uses the default
branch's tooling; the initial lane's publish job didn't re-check.

- build-evidence.py grows `--fit AUTHOR BRIEF`: fit two finished cards in
  place with fit_author_card / fit_brief_card and exit. A card that already
  fits comes back byte-identical.
- The publish job runs it (default-branch checkout) right before
  pinned-comment.sh. Best-effort: the size check is still the backstop.
- Regression test reproduces the shape (an unfitted card from an older
  head) and covers the CLI and its no-op case.

Replayed against #22287's real handoff: 110,267 -> 35,262 bytes, all 118
rows kept (the shed Do-this blocks are already in evidence `detail`), and
the fitted card passes validate-pinned.py.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01GWRsY6CwDJHhNhNFUnswxJ
@github-actions

github-actions Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Sentinel — merge gate status

2 of 5 gates need attention before this can merge.

Gate What it needs
🔴 G1 review-ran No current review for 0bee4d8de — comment /update-review <what changed> to refresh it, or /new-review for a fresh one.
➖ G2 findings-answered Doesn't apply to this PR: no review present — see G1.
🔴 G3 right-approver Needs an approval from anyone with write access to this repo — no qualifying human approval at this head (bot approvals count only on the automated lanes bot_approvers lists).
➖ 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 0bee4d8de.


Need help, or think a gate got it wrong? Ask in #docs on Slack.

@pulumi-bot

Copy link
Copy Markdown
Collaborator

@CamSoper
CamSoper marked this pull request as ready for review October 20, 2026 18:51
@github-actions github-actions Bot added the review:triaging Claude Triage is currently classifying the PR label Oct 20, 2026
@CamSoper
CamSoper merged commit 76786d7 into master Oct 20, 2026
17 checks passed
@CamSoper
CamSoper deleted the camsoper/amazing-hawking-aqo20s branch October 20, 2026 18:51
@github-actions github-actions Bot added the domain:infra PR touches workflows, scripts, infra, Makefile, or build config label Oct 20, 2026
@pulumi-bot
pulumi-bot requested a review from a team October 20, 2026 18:52
@github-actions github-actions Bot removed the review:triaging Claude Triage is currently classifying the PR label Oct 20, 2026

This branch was successfully deployed

1 active and 1 inactive deployments
testing — 0bee4d8d Deployed Oct 20, 2026 by CamSoper via Do cleanup #14833
production — 0bee4d8d Deployed Oct 20, 2026 by CamSoper via Record review outcome #14833
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants