fix(review-gate): recover prior heads a force-push names but does not record - #349
Merged
Merged
Conversation
#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.
… 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.
lamemustafa
marked this pull request as ready for review
September 9, 2026 16:46
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. |
lamemustafa
added a commit
that referenced
this pull request
Sep 9, 2026
Third time. Release Please regenerated this branch when #349 merged and discarded both corrections again, restoring the defects they fixed. Verified against the prior head `dfeb9d1`: the reverted #304 entry was back unmarked and the source-build-only qualifier was gone. Re-applied, byte-identical to `dfeb9d1`: - #304 "simplify selection and completion paths" is marked as reverted by #307 before this release. `11cc788` is literally `Revert "fix(filed-returns): simplify selection and completion paths (#304)"`, after a live authenticated run stalled on the first period of the year. - The full-year and all-supported entries are qualified as source-build only. The panel enables that flow solely under `MODE === "source-surfaces"` and Vite removes the JSX from a packaged build, so the released extension does not offer it. Re-ran the reverted-entry scan over `v0.5.1..master`, short-SHA aware because revert bodies name the 7-character form, with the known case as a control: `11cc788` is found and is still the only revert in the range, so `b78b13d` remains the only entry needing the annotation. Diffing this tree against `dfeb9d1` leaves exactly one changelog line: the #349 entry the regeneration correctly added. Nothing else moved. Without this, v0.6.0 ships notes crediting a reverted fix and advertising a full-year flow the packaged build removes -- a claim AGENTS.md prohibits. Nothing detects the loss automatically. Tracked in #342's thread, which is about where corrections to generated release notes should live; this commit is the workaround, not the answer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 9, 2026
lamemustafa
added a commit
that referenced
this pull request
Sep 10, 2026
…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>
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
1 participant
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.
Summary
Recover the prior heads a force-push names but does not record, so release pull requests stop being
refused as untraceable rewrites.
Closes #342. Unblocks #337.
Root Cause / Decision 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. The cost is not theoretical: #337 sitswith every check green, a clean Codex review at its head, and all review threads resolved, and
cannot merge. 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 thepush created — and durable review state lives in check runs addressable by SHA. Verified against
#337, where all three regenerations look like:
loadForcePushedPriorShasreadbefore_commit_id, sethasUntraceableRewrite, and discarded theevent — including the usable SHA inside it. Both fields are now candidate heads to search. A rewrite
naming neither is still untraceable and still fails closed.
Why this is not the withdrawn attempt. #343 exempted generated branches from the check. Review
found two P1s: the actor test was the pull request's creator rather than the rewrite's actor, and —
decisively — the durable state the check protects is real, so exempting could publish success while
dropping an observed finding. This change does the opposite: it searches the discarded head, so
such a finding is found rather than lost. State reached through a recovered head is still required
to belong to this pull request; dropping that validation fails two tests.
A second discovery, recorded because it explains the shape of the bug. #337 has never carried
durable state on any head —
output.textis null on everyReview gate (scheduled)run acrossa16d8836,abdd12b,f59d877,79161c9,dfeb9d1. #318 publishes the terminal error withoutstate, so a generated pull request hits the error on its first regeneration and can never publish
state afterwards. The gate was failing closed to protect state its own failure prevented existing.
That is an argument for fixing discovery, not for exempting.
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 rather than works.
Scope
src/; nothing reaches a taxpayer's browser.scripts/publish-review-gate-check.mjs— one loop and the ordering it feeds.tests/scripts/publish-review-gate-check.test.ts.two such commits and a regeneration triggered by an unrelated merge destroyed both, which is
demonstrated in that PR's history. Still open on Review gate blocks every Release Please PR: regeneration force-pushes, which fails the continuity check #342's thread.
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
queried against endpoints already in use.
Sensitive Surface Review
at stake, and it is strengthened: a finding on a discarded head is now found rather than lost.
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 premise was pinned before the fix existed (
d5a08b7), as a test asserting the orphaned headwas not consulted. It passed, which is what made the direction evidence-backed rather than
plausible. The fix flips it (
c50b3de), and the counterpart — a rewrite naming neither field staysuntraceable — passed before and after.
Mutation-tested:
The third is the one that matters: it confirms recovered state is still validated as belonging to
this pull request, so continuity is recovered rather than waived.
Artifact Evidence
c50b3de.PR Review Follow-Up
d5a08b7premise,c50b3defix; 3 mutations caughtcommit_idsemantics rest on three samplesTwo things I could not settle, stated rather than buried
1.
commit_idsemantics are inferred, not documented. I read it as the head after the push,from three samples on one pull request, corroborated by ordering: the
reviewedevent immediatelyfollowing the first force-push sits at
abdd12b. If it is ever the before head instead, thisstill searches a real head of this pull request and the ownership check still gates what is
accepted — so the failure mode is a wasted query, not a wrong result. But I would rather a reviewer
confirm than take my reading.
2. Recovery is proven by fixture, not by live data. #337 has no durable state on any head, so I
cannot demonstrate a real finding being recovered. The unit tests construct the state and prove the
walk reaches it. That is weaker than a live case, and I could not manufacture one without
publishing state to a real pull request.
Screenshots
None — CI tooling change.