fix(review-gate): accept regeneration rewrites on the generated release branch - #343
lamemustafa wants to merge 2 commits into
Conversation
…se branch Release Please updates its release pull request by force-pushing the branch, and GitHub records those rewrites with no `before_commit_id`. #318 made an untraceable rewrite fail closed, which is right for a human branch: a force-push there can replace reviewed code without a trace. A generated release branch has no such state to protect. The bot rebuilds it from the base branch on every upstream merge, so a rewrite discards only content the next run reproduces. Failing closed there makes a release pull request unmergeable as soon as anything lands on the base branch. That is not theoretical: it blocked v0.6.0 outright (#342), with every other check green, and the discontinuity was created by an unrelated merge rather than by anything done to the release branch. Timeline: v0.5.1 released 2026-08-17, #318 landed 2026-09-07, and v0.6.0 is the first release attempt since. The exemption is evidence-based rather than name-based, because a branch name is chosen by whoever pushes. Both must hold: the head branch carries release-please's generated name for *this* pull request's own base, and the pull request is authored by a bot. A human branch wearing the generated name fails the second; a bot pull request from an ordinary branch, or one naming a base it does not target, fails the first. The acceptance is logged rather than silent. An exemption nobody can see is one nobody can audit, and this one weakens a gate that was settled deliberately. Closes #342.
Fixtures now carry `head.ref`, `base.ref` and `user.type`, matching the shape release-please actually produces (confirmed against #337: `release-please--branches--master--components--pack`, `github-actions[bot]`). Four cases: the generated release pull request accepts an untraceable rewrite and publishes success while announcing the exemption; a human-authored branch wearing the generated name, a bot pull request from an ordinary branch, and a bot branch naming a base it does not target are all still refused. Mutation-tested, each caught: - bot-author requirement dropped -> human branch wearing the name is accepted - base ref ignored, bare prefix -> wrong-base branch is accepted - guard removed entirely -> 7 of 34 fail The three refusal cases are the point. An exemption is only as good as what it declines, and the acceptance case alone would pass against a guard that exempts everything.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0800a5d8fc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const headRef = pr?.head?.ref; | ||
| if (typeof baseRef !== "string" || baseRef.length === 0) return false; | ||
| if (typeof headRef !== "string") return false; | ||
| if (String(pr?.user?.type ?? "").toLowerCase() !== "bot") return false; |
There was a problem hiding this comment.
Verify the rewrite actor instead of the PR creator
pr.user.type identifies who originally opened the pull request, not who performed a later force-push. Once Release Please creates this PR, a maintainer or unrelated bot can rewrite its generated-named branch and this predicate still grants the exemption; when GitHub omits before_commit_id, the gate can then seed from the rewritten history and lose an orphaned durable finding. Authenticate the rewrite as trusted Release Please automation rather than relying on the PR creator's persistent type.
AGENTS.md reference: AGENTS.md:L90-L92
Useful? React with 👍 / 👎.
| // across a null `before_commit_id` cannot be proved, so no reachable state is trustworthy here. | ||
| if (hasUntraceableRewrite) throw untraceableRewriteError(); | ||
| if (hasUntraceableRewrite) { | ||
| if (!isGeneratedReleasePullRequest(pr)) throw untraceableRewriteError(); |
There was a problem hiding this comment.
Preserve findings across legitimate release regeneration
Even when Release Please legitimately performs this rewrite, exempting it discards the exact state the continuity guard protects: if the scheduled check persisted an open finding on the release PR and that comment is subsequently deleted, the null before_commit_id leaves the old check unreachable, this branch proceeds using only the regenerated history, and loadLatestDurableReviewState can seed an empty state and publish success. Regeneration does not prove the prior ask was fixed, stale, or otherwise dispositioned, so generated PRs need a continuity mechanism rather than bypassing the terminal rejection.
AGENTS.md reference: AGENTS.md:L94-L97
Useful? React with 👍 / 👎.
|
Withdrawn without merging. The approach was unsound, and review established that before it landed. What it attempted. Exempt the generated release branch from the untraceable-rewrite check, so a release pull request stops being unmergeable whenever anything lands on The premise, stated in the code comment and the PR body: a bot-regenerated branch has no reviewed history for a rewrite to smuggle past. I also wrote, before review: "If that claim is wrong, the exemption is wrong." It is wrong, in two independent ways:
The second one is not fixable by tightening the exemption, because it is an argument against exempting at all. Generated pull requests need a continuity mechanism, not a bypass — which is close to the continuity-override protocol #318 deliberately descoped after seven rounds. Consequence: release pull requests remain blocked (#342), and #337 with them. That is worse than merging this would have felt, and better than a gate that reports success while dropping a finding. Recorded in #342 so the next attempt starts from what this one established rather than from its premise. |
… record Release-please regenerates its release branch by force-pushing, GitHub records those events with a null `before_commit_id`, and the gate refused every one as an untraceable rewrite. That blocked release pull requests outright: #337 sits with every check green, a clean review at its head, and all threads resolved, and the last release was v0.5.1 on 2026-08-17. The rewrites were never untraceable. The same timeline event carries `commit_id`, the head the push created, and durable review state lives in check runs addressable by SHA. `loadForcePushedPriorShas` read `before_commit_id`, set `hasUntraceableRewrite`, and discarded the event -- including the usable SHA in it. Verified on #337, where all three regenerations look like: before_commit_id: null commit_id: abdd12b… github-actions[bot] before_commit_id: null commit_id: f59d877… github-actions[bot] before_commit_id: null commit_id: 79161c9… github-actions[bot] Both fields are now treated as candidate heads to search. A rewrite naming neither is still untraceable and still fails closed. This recovers continuity rather than waiving it, which is the distinction that sank the earlier attempt (#343, withdrawn). That change exempted generated branches from the check; review showed the durable state it protects is real, and that exempting could publish success while dropping an observed finding. Here the discarded head is searched, so such a finding is found. State reached through a recovered head is still required to belong to the pull request -- dropping that validation fails two tests. Ordering moved from SHAs to events. The ambiguity check threw when two prior heads shared a timestamp, and one event now contributes two SHAs that necessarily share one. Their order is known -- the created head is newer than the discarded one -- so only a tie between distinct events is ambiguous. Left alone, every recovered rewrite would have thrown "ambiguous chronological ordering": a fix that fails differently. Mutation-tested: reverting the widening, never marking untraceable, and dropping the pull-request ownership check each fail. Closes #342.
… record (#349) * test(review-gate): pin the untraceable-rewrite premise before fixing it #342 blocks every release pull request: release-please force-pushes its branch, GitHub records the event with a null `before_commit_id`, and the gate fails closed on an untraceable rewrite. Two withdrawn attempts (#343, and the exemption argument behind it) tried to bypass that check. This pins the fact that makes a real fix possible instead. The same timeline event carries `commit_id` -- the head *after* the push -- even when `before_commit_id` is null. Verified against #337, where all three release-please force-pushes look like: before_commit_id: null commit_id: abdd12b… github-actions[bot] before_commit_id: null commit_id: f59d877… github-actions[bot] before_commit_id: null commit_id: 79161c9… github-actions[bot] So the orphaned heads are named in the timeline. Durable review state lives in check runs addressable by SHA, and those SHAs are exactly the ones being discarded: `loadForcePushedPriorShas` reads `before_commit_id`, sets `hasUntraceableRewrite`, and drops the event including the usable SHA in it. Two tests, both asserting current behaviour so a fix has something to flip: - state on a head named only by `commit_id` is never queried, and the run is refused. This is the premise; it passes today, which is the point. - a rewrite with neither field usable stays untraceable. Any widening must keep this failing closed. No production change here. Written first deliberately: two designs of mine on this gate have already been withdrawn for resting on unverified models, and this is the assertion that decides whether widening discovery is the right direction or whether #342 needs a storage redesign instead. * fix(review-gate): recover prior heads a force-push names but does not record Release-please regenerates its release branch by force-pushing, GitHub records those events with a null `before_commit_id`, and the gate refused every one as an untraceable rewrite. That blocked release pull requests outright: #337 sits with every check green, a clean review at its head, and all threads resolved, and the last release was v0.5.1 on 2026-08-17. The rewrites were never untraceable. The same timeline event carries `commit_id`, the head the push created, and durable review state lives in check runs addressable by SHA. `loadForcePushedPriorShas` read `before_commit_id`, set `hasUntraceableRewrite`, and discarded the event -- including the usable SHA in it. Verified on #337, where all three regenerations look like: before_commit_id: null commit_id: abdd12b… github-actions[bot] before_commit_id: null commit_id: f59d877… github-actions[bot] before_commit_id: null commit_id: 79161c9… github-actions[bot] Both fields are now treated as candidate heads to search. A rewrite naming neither is still untraceable and still fails closed. This recovers continuity rather than waiving it, which is the distinction that sank the earlier attempt (#343, withdrawn). That change exempted generated branches from the check; review showed the durable state it protects is real, and that exempting could publish success while dropping an observed finding. Here the discarded head is searched, so such a finding is found. State reached through a recovered head is still required to belong to the pull request -- dropping that validation fails two tests. Ordering moved from SHAs to events. The ambiguity check threw when two prior heads shared a timestamp, and one event now contributes two SHAs that necessarily share one. Their order is known -- the created head is newer than the discarded one -- so only a tie between distinct events is ambiguous. Left alone, every recovered rewrite would have thrown "ambiguous chronological ordering": a fix that fails differently. Mutation-tested: reverting the widening, never marking untraceable, and dropping the pull-request ownership check each fail. Closes #342.
…rry it (#351) * fix(review-gate): scope the durable-state search to heads that can carry it The scheduled gate refused #337 with "durable force-push history exceeded the safe lookup bound". #349 made the discarded heads discoverable; the search that then ran walked their ancestry into master and exhausted a 20-node budget before reading anything useful. Durable state is only ever published against a pull request head -- `publishCheck` is always called with `pr.head.sha` -- so base-branch history below the branch point was never a head here and a lookup there can only miss. A test pinned that first: it asserted a commit two levels below the branch point is not queried, and it failed on the pre-fix code. Each discarded head is now expanded through `compare(base...head)` to the commits unique to that head, which stops at the branch point. Scoping alone would still have refused #337, and the reason turned out to falsify what #342 and #349 both recorded. #337 does carry durable state: a16d883 Review gate (scheduled) failure review-gate-state/v1 {"version":1,"prNumber":337,"findings":[]} That head is the one the first regeneration discarded. Its rewrite recorded no `before_commit_id`, and `compare` confirms it is not an ancestor of any recovered head -- each regeneration replaces the single commit outright -- so no force-push event names it and no ancestry reaches it. The gate was refusing a pull request for want of state it already had. Reviews name it. Each review records the `commit_id` it was submitted against, so `/pulls/N/reviews` enumerates heads the timeline does not, and `a16d883` is reachable through the Codex review submitted on it. Recovered heads are merged with the force-push heads and searched newest first, because the newest recorded state is the one that wins. What is deliberately unchanged: a force-push that leaves no reachable state is still refused. An earlier draft concluded the opposite -- that a completed search finding nothing proved nothing was ever recorded -- and `a16d883` is the counter-example that killed it. A rewrite recording no `before_commit_id` never names what it discarded, so the search cannot be proved complete, and had that state carried an open finding the draft would have dropped it. That is the defect #343 was rejected for. The refusal stays; the search got better instead. Two guards on the comparison, both tested: a commit list GitHub truncated, and a list whose tip is not the head it was asked about, are each a narrower search wearing the shape of a complete one, so both refuse rather than search. Residual gap, recorded rather than implied: a head that was never reviewed and whose rewrite named no `before_commit_id` is still unreachable. Nothing in the pull request's record names it. That is narrower than before this change and is filed as a follow-up. Mutation-tested; every one is caught: walk into base-branch history again -> 2 tests fail drop reviewed-head recovery -> 1 test fails remove the fail-closed refusal -> 1 test fails drop newest-first ordering -> 3 tests fail drop the comparison truncation check -> 1 test fails drop the PR-ownership check on found state -> 2 tests fail Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(review-gate): rank recovered state by when it was recorded Dispositions all three findings from the Codex review of a8f61bf. P1, head order inferred from review time. A review submitted after a force-push still records the commit it was started against, so `submitted_at` could rank a discarded head above the head that replaced it. With the older head clean and the newer one holding a deleted finding, the gate would have published success and lost the ask -- the failure this whole path exists to prevent. No timestamp attached to a head is safe to rank by, so ranking no longer uses one. Precedence comes from `completed_at` on the durable check run: when the gate actually recorded that state. Every candidate is read and the newest recorded state wins, rather than stopping at the first hit in an inferred order. Two different states recorded at the same instant are refused as ambiguous rather than guessed, matching how ambiguous force-push ordering is already handled. The early return survives only where commit order settles precedence on its own: a linear history with no force-push, where a commit stops being the head the moment the next one is pushed, so nothing can land on an older commit afterwards. P2, the bound did not bound the work. It was checked after expansion, so an oversized history still cost one comparison request per candidate head before anything refused, which can exhaust the run that was supposed to publish the fail-closed check. Moved inside the loop. P2, a recovered head the base branch has caught up to. When the base contains the discarded head, the comparison legitimately has no head-only commits, `at(-1)` is undefined, and the tip guard rejected a head that can still carry durable state. The named head is now always a candidate; the tip guard applies only when the comparison does contribute commits. This one was a legitimate empty result being read as corruption. Mutation-tested; every one is caught: rank prior heads by review/event time again -> 3 tests fail check the bound after expansion, not during -> 1 test fails refuse a head the base has caught up to -> 1 test fails drop the same-instant ambiguity refusal -> 1 test fails Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(review-gate): refuse a tie across the whole cohort, not its first pair Dispositions both findings from the Codex review of 1e19c0e. P1, the tie check read only the first two states. Three states sharing the newest `completed_at` whose first two agree would return that agreed text while a third recorded a different unresolved finding -- publishing a clean state over an ask, which is the outcome the refusal exists to prevent. The check now spans every state tied at the newest timestamp. P2, the comparison request preceded the already-covered check. A review attached to a commit still on the current line was added as a candidate head, compared, and its results then discarded by `seen`. Because those results never grow the candidate list, the bound could not stop the redundant requests, so a heavily-reviewed pull request could still issue hundreds of comparisons. The check moved ahead of the request. A head already covered contributes only commits that are themselves already covered, so nothing reachable is lost. The first mutation of the cohort fix did not fail: candidates are ordered newest force-push first, so a differing state on the newest head sat at index 0 and a pair-only check tripped over it anyway. The fixture now puts the differing state on the oldest head, where it lands third in the cohort, which is the only arrangement that tells the two implementations apart. Recorded because the first version of that test would have passed against the defect it was written for. Mutation-tested; both are caught: compare only the first two of the tied cohort -> 1 test fails skip the covered head after comparing, not before -> 1 test fails Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(review-gate): refuse state older than a rewrite that named no discarded head Dispositions all three findings from the Codex review of 19a131b. P1, and this one is a regression this pull request introduced. Before it, a force-push with nothing reachable was refused. Widening recovery to review heads means the search now finds something, and returning the newest reachable state as the baseline is wrong when a rewrite did not name what it discarded: that head can hold a superseding state, and accepting an older one publishes success over an ask recorded only there. `commit_id` names the head a rewrite created, never the head it destroyed. Rewrites that leave the discarded head unnamed are now recorded with their timestamp, and a state recorded before the newest of them is refused. Verified that the discarded head is not otherwise derivable: `committed` timeline events track only the current line, so an ordinary push later rewritten away leaves no record at all. On #337 that is `dfeb9d1`, which is named by no event and no review. P2, heads named only by a clean top-level review. A clean Codex review is a comment carrying a `Reviewed commit` marker rather than a review object, so `/pulls/N/reviews` never names the head it reviewed, and the evaluator already trusts that marker. Those markers are now candidate heads, with their prefixes resolved to full SHAs. The pattern moved to `scripts/lib/codex-review-markers.mjs` so the evaluator and the publisher read one definition rather than two copies that can disagree about which commits were reviewed. P1, rejections that did not name themselves. Six boundaries threw plain errors that `runEvaluationOperation` collapsed into "could not retrieve durable review state", against AGENTS.md's rule that a rejection names its own reason. Each now carries a bounded static reason into the published check. The cost was concrete: diagnosing the bound rejection on #337 required reading workflow logs because the published check did not say which boundary fired. Mutation-tested; every one is caught: accept state that predates an unnamed discard -> 1 test fails drop top-level review marker heads -> 1 test fails collapse a rejection into the generic reason -> 1 test fails Known consequence, stated rather than discovered later: #337 stays refused. Its newest recoverable state is `a16d883` at 2026-09-08T18:10:12Z and its newest unnamed discard is at 2026-09-09T20:25:48Z, so the state predates the rewrite. No other head carries state -- every one publishes `action_required` with no text -- and #337 has no clean top-level review marker. That is the correct answer under the continuity guarantee, and closing it needs a decision about generated branches rather than a weaker guard. Tracked in #350. One test run failed once during this work and has not reproduced across six subsequent runs of the file. Recorded because it is unexplained, not because it is understood. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * style(review-gate): apply prettier to the durable-state search Formatting only; no behaviour change. Prettier flagged both files after the round-4 edits. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(review-gate): fail closed when state ties an unidentified discard Dispositions the finding from the Codex review of 497c3d3. The predates-discard guard used a strict `<`, so a state recorded at the same instant as a rewrite that named no discarded head was accepted. GitHub timestamps share an instant often enough for that to matter, and an equal timestamp is an unknown order, not a safe one -- treating it as safe is the "could not determine means matches" mistake the repository rules name directly. Now `<=`. That change exposed a real interaction between this guard and the recovery this branch added, so the fixtures were corrected rather than the guard loosened. A discarded head's durable state is always written before the rewrite that discarded it, so when that rewrite names no `before_commit_id` the recovered state is refused by construction. Recovery through review heads and clean top-level review markers is therefore usable only where GitHub did record the discarded head. Both recovery tests now model that: an identified rewrite, with the state on an earlier head reachable only through a review or a marker. Their previous fixtures described a discarded head whose state was written after its own rewrite, which cannot happen. Mutation-tested: treat the timestamp tie as safe again -> 1 test fails Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Summary
Accept regeneration rewrites on the generated release branch, so a release pull request stops
being unmergeable as soon as anything lands on
master.Closes #342. Unblocks #337.
Root Cause / Decision Record
Release Please updates its release pull request by force-pushing the branch, and GitHub records
those rewrites with no
before_commit_id. #318 (4527569) made an untraceable rewrite fail closed.That is correct for a human branch, where a force-push can replace reviewed code without a trace.
It is wrong for a branch a bot rebuilds from the base branch on every upstream merge: the rewrite
discards only content the next run reproduces, so there is no prior reviewed state for it to
smuggle anything past — which is the property the check exists to protect.
This was not theoretical. #337 sat
BLOCKEDwith every other check green and zero unresolvedthreads, on
Review gate (scheduled)=action_required:Verified the mechanism rather than inferring it:
The discontinuity was created when #341 merged and Release Please regenerated the branch — not
by anything done to the release branch itself. So every release pull request is blocked, recurring
whenever anything lands on
master. Timeline agrees: v0.5.1 released 2026-08-17, #318 landed2026-09-07, and v0.6.0 is the first release attempt since.
The exemption is evidence-based, not name-based, because a branch name is chosen by whoever
pushes. Both must hold:
The base ref is interpolated from the pull request itself, so a bot branch naming a base it does not
target is refused as well. Confirmed against the real shape of #337: head
release-please--branches--master--components--pack, authorgithub-actions[bot](typeBot),against #341's
tapish-codex/...and typeUser.The acceptance is logged, never silent. This weakens a gate settled deliberately over seven
rounds in #318, so every run that uses it says so:
Considered and rejected: verifying that a rewritten head is "regenerable" from the base branch.
More precise in principle, but it means modelling what release-please would produce — inferring a
grammar this repository does not own, which has cost it repeatedly (#197, #331). Re-creating the
branch per #299 was also rejected: it works once, and the next regeneration reproduces the block.
Scope
src/is touched; nothing reaches a taxpayer's browser.scripts/publish-review-gate-check.mjs— one predicate and one guarded branch.tests/scripts/publish-review-gate-check.test.ts.two such commits and they are lost on the next regeneration. Raised in Review gate blocks every Release Please PR: regeneration force-pushes, which fails the continuity check #342 and left open there
rather than solved here.
Pack Workflow Preflight
pnpm workflow:preflightwas run before editing/push, or the skip reason is documented.Sanchika Adoption Gate
@sanchika/*packages or copied Sanchika guidance, Iread
sanchika/docs/adoption-pack.mdin the coordinated parent worktree.and records the Sanchika commit or copied guidance used.
../sanchika,sanchika/packages/*/src, or parentsource paths.
Privacy And Data-Flow Impact
Sensitive Surface Review
Chrome Web Store Impact
docs/PUBLICATION_READINESS.mdis checked.Verification
pnpm install --frozen-lockfilepnpm audit --audit-level highpnpm exec wxt preparepnpm exec prettier --check .pnpm exec eslint . --max-warnings 0pnpm exec tsc --noEmitpnpm exec vitest runpnpm exec wxt buildnode scripts/verify-extension-package.mjs .output/chrome-mv3pnpm exec wxt zipnode scripts/verify-extension-zip.mjsnode scripts/write-release-provenance.mjsnode scripts/verify-github-release-assets.mjs --tag <tag> --zip <zip> --checksum <sha256> --provenance <json>when release assets existnode scripts/publish-chrome-web-store.mjs --zip .output/<zip> --provenance .output/pack-release-provenance.v1.json --publisher-id <id> --dry-run truegit diff --checkpnpm review:gate -- --strict-head-review --wait-head-review-ms 180000before merge/readiness claim; a missing Codex review blocks readiness:Unchecked boxes are release-only steps; this PR ships no artifact and touches no runtime source.
The three refusal cases are the point, not the acceptance. An exemption is only as good as what
it declines, and the acceptance case alone would pass against a guard that exempts everything.
Mutation-tested, each caught:
Artifact Evidence
0800a5d.PR Review Follow-Up
cdb225epredicate,0800a5dtests; 3 mutations caughtScreenshots
None — CI tooling change.