Skip to content

fix(review-gate): accept regeneration rewrites on the generated release branch - #343

Closed
lamemustafa wants to merge 2 commits into
masterfrom
tapish-codex/release-branch-continuity
Closed

lamemustafa wants to merge 2 commits into
masterfrom
tapish-codex/release-branch-continuity

Conversation

@lamemustafa

Copy link
Copy Markdown
Owner

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 BLOCKED with every other check green and zero unresolved
threads, on Review gate (scheduled) = action_required:

Review gate GitHub did not record the prior head, so review continuity cannot be
verified across that rewrite. Re-create the branch as described in #299...

Verified the mechanism rather than inferring it:

$ git merge-base --is-ancestor abdd12b d91447b     # previous #337 head
  -> not an ancestor; release-please rewrote the branch

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 landed
2026-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:

headRef.startsWith(`release-please--branches--${baseRef}--components--`) && pr.user.type === "Bot"

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, author github-actions[bot] (type Bot),
against #341's tapish-codex/... and type User.

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:

Accepting an untraceable rewrite on generated release branch <ref>: its contents are
regenerated from <base> rather than carried across review.

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

Pack Workflow Preflight

  • pnpm workflow:preflight was run before editing/push, or the skip reason is documented.
  • This PR was opened from a Pack branch, not master.
  • I checked latest master Pack AGENTS guidance or recorded the stale-guidance warning.
  • PR body keeps the required Pack privacy/review/verification checklist visible.

Sanchika Adoption Gate

  • If this PR consumes @sanchika/* packages or copied Sanchika guidance, I
    read sanchika/docs/adoption-pack.md in the coordinated parent worktree.
  • If this PR consumes Sanchika, it links ComplyEaze and Axal completion evidence
    and records the Sanchika commit or copied guidance used.
  • This PR does not import ../sanchika, sanchika/packages/*/src, or parent
    source paths.

Privacy And Data-Flow Impact

  • No new browser permissions.
  • No new host permissions.
  • No new network calls.
  • No analytics, telemetry, ads, or session replay.
  • No credential, OTP, CAPTCHA, cookie, token, GST file, or taxpayer-data capture.
  • Public copy and privacy declarations are updated if behaviour changed. (CI tooling only.)

Sensitive Surface Review

  • Current tab / portal target binding is preserved or intentionally changed. (Untouched.)
  • Download completion remains evidence-backed and fail-closed. (Untouched.)
  • Ambiguous side-effect delivery cannot be reported as confirmed success. (Untouched.)
  • Service-worker durability impact is understood and documented. (None — CI tooling.)
  • Real taxpayer data, local paths, raw URLs/referrers, and portal HTML are absent from the diff.

Chrome Web Store Impact

  • This PR does not expand beyond the existing Chrome Web Store V0 listing unless every gate in docs/PUBLICATION_READINESS.md is checked.
  • Full fiscal year remains source-only and excluded from the next packaged Store build until its evidence gates are recorded.
  • Store copy, README status, Privacy QA, and reviewer instructions were reviewed if user-facing behavior changed. (No user-facing change.)
  • CI ZIP creation, provenance, and protected publishing are treated as release evidence, not manual store-submission sign-off.
  • PR title uses Conventional Commits so Release Please can bump Pack after merge.

Verification

  • pnpm install --frozen-lockfile
  • pnpm audit --audit-level high
  • pnpm exec wxt prepare
  • pnpm exec prettier --check .
  • pnpm exec eslint . --max-warnings 0
  • pnpm exec tsc --noEmit
  • pnpm exec vitest run
  • pnpm exec wxt build
  • node scripts/verify-extension-package.mjs .output/chrome-mv3
  • pnpm exec wxt zip
  • node scripts/verify-extension-zip.mjs
  • node scripts/write-release-provenance.mjs
  • node scripts/verify-github-release-assets.mjs --tag <tag> --zip <zip> --checksum <sha256> --provenance <json> when release assets exist
  • node scripts/publish-chrome-web-store.mjs --zip .output/<zip> --provenance .output/pack-release-provenance.v1.json --publisher-id <id> --dry-run true
  • git diff --check
  • pnpm review:gate -- --strict-head-review --wait-head-review-ms 180000 before 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.

Pack workflow preflight passed for tapish-codex/release-branch-continuity against master.
 Test Files  175 passed (175)
      Tests  3294 passed (3294)
   Duration  255.00s (tests 94%, import 5%, transform 1%)

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:

bot-author requirement dropped   -> human branch wearing the generated name is accepted
base ref ignored (bare prefix)   -> branch naming a base it does not target is accepted
guard removed entirely           -> 7 of 34 fail

Artifact Evidence

  • CI run: pending on this head.
  • ZIP artifact: n/a — CI tooling and tests only.
  • ZIP SHA-256: n/a.
  • Clean source/tag or head SHA: 0800a5d.

PR Review Follow-Up

  • GitHub Actions completed.
  • Autogenerated Codex/bot review comments inspected after checks completed for the latest head SHA.
  • Inline review threads are resolved, outdated, or answered with evidence.
  • No commits were pushed after the last required human/bot review without re-review.
  • Any follow-up PRs or issues are listed here instead of being left implicit.
Thread/comment Disposition Commit or evidence
none yet cdb225e predicate, 0800a5d tests; 3 mutations caught
Where corrections to generated release notes should live follow-up #342 — left open; #337's two correction commits die on the next regeneration

Screenshots

None — CI tooling change.

…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.
@lamemustafa
lamemustafa marked this pull request as ready for review September 9, 2026 00:11
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-09T00:17:28.336479Z 0800a5d Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

@lamemustafa

Copy link
Copy Markdown
Owner Author

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 master (#342).

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:

  1. The actor check was the wrong actor. pr.user.type identifies who opened the pull request, not who performed a later force-push. Once Release Please creates the PR, anyone with push access can rewrite its generated-named branch and still receive the exemption. I demonstrated the shape of this myself on chore(master): release 0.6.0 #337 by pushing hand commits to that branch — had I force-pushed, my own exemption would have accepted it. Fixable by reading actor from the head_ref_force_pushed timeline event, which does carry it.

  2. The durable review state is the reviewed history. Even a legitimate regeneration discards it: if the scheduled check recorded an open finding against a prior head and that comment is later deleted, the null before_commit_id leaves the old state unreachable, so the gate seeds empty and publishes success — losing the ask. Regeneration does not prove a prior finding was fixed, stale, or dispositioned.

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.

@lamemustafa lamemustafa closed this Sep 9, 2026
lamemustafa added a commit that referenced this pull request Sep 9, 2026
… 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 added a commit that referenced this pull request Sep 9, 2026
… 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.
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Review gate blocks every Release Please PR: regeneration force-pushes, which fails the continuity check

1 participant