review-trial: record three operational lessons from the preview trials - #306
Merged
Conversation
… lessons from the preview trials Three things the last two trials taught the hard way, none of which the skill said: - Never force-push a trial branch between rounds. A round that cannot anchor on a prior fingerprint reports `stampSource: null` and executes `depth: full` with `staging: whole-diff` even where ROUTING says `scoped`, which looks identical to a genuine carrier gap and voids the scoped-cost sample. The carrier itself is content-hashed and rebase stable (`rereview-mode.ts`) and its `commitSha` is never read back, so the thing that actually loses the anchor is losing the record; a force-pushed round is just where that goes unnoticed. Read `stampSource` out of the plan artifact every round instead of inferring it. - Give each trial's compiled workflow a fresh lock filename. GitHub keys the workflow display name to the file path and keeps the stale one, so two trials that reused `review-preview.lock.yml` both listed as "PR Reviewer" while their files declared "PR Reviewer Preview v1.8.0" and "...v1.9.0". Concurrency was unaffected, but the run list invites the "did production just run?" scare the naming rule exists to prevent. - A lifecycle plan that means to test open-thread suppression has to leave one finding unfixed; if the fixer repairs everything the reconciler resolves every thread, `threads.json` is empty, and the round proves nothing either way.
🦋 Changeset detectedLatest commit: ac523fd The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
khan-actions-bot
requested review from
a team,
jeresig and
kevinb-khan
and removed request for
a team
July 31, 2026 16:51
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
jeresig
approved these changes
Jul 31, 2026
…t; make `stampSource` the authoritative anchoring signal The Lints job failed for want of a changeset. Skill-doc changes take a `review` patch, matching #291. Review feedback, both non-blocking: - The recap carried the force-push ban but not the rule the section says actually matters, leaving a skimming operator unprotected against record loss, which is the real anchor-loss cause. Adds the `stampSource` line. - The carrier internals (hunk-hash tolerance, `commitSha` written but never read back) are accurate today but are incidental implementation properties, not contracts. They now sit under the `stampSource`-is-authoritative rule, dated, with a note saying which half goes stale if a later change starts validating `commitSha` on restore.
Contributor
There was a problem hiding this comment.
Approved — no blocking issues found.
Re-review: both prior review threads are now resolved.
- The recap (
stampSourceline) was added, addressing the earlier suggestion. - The carrier-internals paragraph is now dated (
As of 2026-07:) and framed as incidental implementation properties rather than contracts, with a note on which half goes stale ifcommitSharestore-validation is ever added — addressing the earlier note.
Both factual claims in the diff check out against the current source: the content-hashed hunk signature is force-push/rebase stable (workflows/review/lib/rereview-mode.ts), and commitSha is written at workflows/review/lib/cache-record.ts:401 but has no read site in the repo. Docs-only change; no code paths touched.
…runs by events, keep injections out of the diff From the 2026-07-31 suppression trial on webapp#41204. Counting runs by events, not pushes. A closed PR fires no pull_request event, so pushes onto it are free; `reopened` is in the reviewer's trigger list, so a reopen fires exactly one run; pushing to an open PR fires `synchronize`, one more each time. Push-while-closed then reopen therefore buys a round for one run. The larger saving is not building the round at all: a closed PR whose last round's comments are still unresolved already IS a partially-fixed round, and reopening webapp#41204 produced the round that made open-thread suppression fire in production for the first time (run 30650642317) against a two-run budget. Unresolving a thread is a GraphQL mutation an agent's tooling may refuse, so it belongs to the operator. Keeping fault injections out of the diff under review. Mis-staging to trip a fail-open reporter works (run 30654454047 reported unusableThreads: 9), but injecting through the prompt or lock file on the trial branch makes that edit the newly-changed code: all five of that run's candidates landed on the prompt file and none on the Go fixture, so the duplicate re-posting the reporter warns about went unobserved, and the round was spent correctly flagging the injected prose as agent-directing instructions. Cleanup gains the corollary the closed-PR shortcut needs. Keeping a resumable branch is fine, but it is reverted to a safe state in the pass that closes the PR, with the resumable details (unresolved threads, run IDs, what a reopen fires) in the closing comment: a surviving branch that still carries a planted panic is a trap, and the reopen reviews immediately.
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "review": patch | |||
Contributor
There was a problem hiding this comment.
question (non-blocking): Does a doc outside workflows/review/ warrant a review: patch bump? The skill lives at repo-level .claude/skills/review-trial/, outside the review package, and the changeset's own closing line says "No change to the shipped review workflow" — yet this bump cuts review-v1.9.1 and rewrites the pinned review-v<semver> literal in review.md. If the changeset gate only needs a file to exist, pnpm changeset --empty lands the lessons without minting a contentless release; keep the patch bump only if trial docs are deliberately versioned with the package.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three lessons the last two preview trials taught the hard way, none of which the skill said.
1. Never force-push a trial branch between rounds (Step 3)
A re-review round that cannot anchor on a prior fingerprint reports
stampSource: nullinout/rereview-plan.jsonand executesdepth: fullwithstaging: whole-diffeven where ROUTING saysscoped. That is visually indistinguishable from a genuine carrier gap, and it silently voids the scoped-cost sample the round was supposed to produce.The skill now also names what the carrier does and does not tolerate, because it is easy to assume the wrong one:
workflows/review/lib/rereview-mode.ts:63-70,88-92; asserted byrereview-mode.test.ts:64);commitShais written (workflows/review/lib/cache-record.ts:401) and read by nothing in the repo.So a rebase alone should not refuse the carrier. What loses the anchor is losing the record (cache eviction, or a credit-capped run that dies before Step 9 writes it, which the skill already warns about); a force-pushed round is simply the round where that goes unnoticed and gets blamed on the rebase. Guidance: forward-only pushes once a round has been reviewed, rebase the stack before the first review, read
stampSourceout of the plan artifact every round instead of inferring it, and discard scoped-cost samples from any round that did not anchor.2. Fresh lock filename per trial (Step 1)
GitHub's workflow registry keys the display name to the file path and keeps the stale one. Two consecutive trials that reused
review-preview.lock.ymlboth appeared in the Actions list as "PR Reviewer" while the files on the branches declared "PR Reviewer Preview v1.8.0" and "...v1.9.0". Concurrency was unaffected (distinct groups, and the production workflow recorded zero runs on either branch), but a run list that names every arm after the production reviewer is unreadable and invites exactly the "did production just run?" scare the existing naming rule exists to prevent.3. Suppression tests need an unfixed finding (Required input 4)
A lifecycle push where the fixer repairs everything lets the reconciler resolve every thread, so
threads.jsoncomes back empty and open-thread suppression has nothing to suppress. A plan that means to exercise suppression must leave at least one finding unfixed.Plus two lines in the Guardrails recap.
Docs only; no code paths touched.