fix(release): record confirmed branch rewrites with compare-and-swap - #352
fix(release): record confirmed branch rewrites with compare-and-swap#352lamemustafa wants to merge 18 commits into
Conversation
Closes the loop #342 opened and #350 tracked. Release Please regenerates its branch by force-pushing, GitHub records no `before_commit_id` for it, and the discarded head is then unnameable: an ordinary push later rewritten away leaves no timeline entry, and a head nobody reviewed is named by no review. The review gate cannot prove a finding recorded only on that head was carried forward, so it refuses -- correctly, and permanently. Confirmed live on #337 after #351 landed: conclusion=action_required summary=Review gate a rewrite did not record the head it discarded, and the newest recoverable review state predates that rewrite, so a finding recorded only on the discarded head cannot be ruled out. The workflow performing the rewrite is ours, so it can record what GitHub does not. `run-release-please.mjs` reads the release branch heads immediately before the regeneration, while the head about to be discarded is still addressable, and posts a marker naming both heads when one actually moved. The gate reads that marker and treats the rewrite as identified. This supplies a name, not permission. A head named by a marker is still searched for durable state belonging to that pull request, and a marker pointing at a head with no state leaves the gate exactly as unconvinced as before. That is the distinction from #343, which exempted generated branches from the check outright and was withdrawn for it. Three boundaries, each tested: - Only `github-actions[bot]` markers count. Anyone who can comment can write the text, so the author is the whole of its authority, and no human can post under that login. - A branch created rather than rewritten, or one whose head did not move, discarded nothing and gets no marker. - Two markers naming different discarded heads for one created head are two claims about one rewrite with no basis for preferring either, so the gate refuses instead of choosing. The wrapper's "without contacting GitHub" test asserted a guarantee this feature ends. Rather than delete it, it now stubs `fetch` and asserts exactly which calls happen: two branch-head reads bracketing the regeneration, and no writes when nothing was rewritten. Mutation-tested; every one is caught: accept a rewrite record from any author -> 1 test fails ignore recorded discards entirely -> 1 test fails record a marker when the head did not move -> 1 test fails accept conflicting rewrite records -> 1 test fails Does not retroactively unblock #337: its four rewrites already happened unrecorded. That needs its branch re-created, or the corrections carried on a fresh regeneration once this is live. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1262bb8477
ℹ️ 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".
Dispositions all three findings from the Codex review of 1262bb8. Two of them interlock, so this is a redesign of the recording rather than three patches. P1, an interrupted run lost the discarded head permanently. Writing the record after the force-push meant a cancellation in between destroyed the only remaining name for that commit, and no later run could reconstruct it. Recording is now two stages: the discarded head is written while it is still the branch head, and the created head is added once the rewrite has produced one. A record stopped in between names a discard with no replacement, identifies nothing, and is ignored by the gate -- and the next run completes it from the branch head it finds, which is exactly what that rewrite created, because nothing else rewrites the branch and it has not run since. P1, bookkeeping could strand a published release. The head snapshot ran after `createReleases()`, so a failure there aborted the wrapper before it wrote its outputs, and the workflow's prerelease-marking, asset-upload and asset-verify steps never ran. Everything fallible now runs before `createReleases()`, where a failure costs a re-run rather than a release without assets. The closing step must run afterwards, so it no longer throws: it logs and leaves the record open. That is only safe because of the fix above -- the open record is recoverable, and the cost of not throwing is a refusal the gate was already making. P2, a malformed head list was read as "no branches". An indeterminate response would have let a rewrite proceed with nothing recording what it discarded, which is the state this mechanism exists to prevent and which nothing downstream could detect. It now throws before regeneration. Fetching the pull request's comments twice -- once for rewrite records, once for clean top-level review markers -- was a duplicate of a fact the pull request states once, and each `gh` invocation spawns a process. Read once and shared. A single run of the publisher tests went from exceeding a 120-second budget to about 41 seconds, which also accounts for tests that had begun failing intermittently in full-file runs while passing alone. Stated as the likely cause rather than a proven one: no before/after timing was captured deliberately. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3bc069214
ℹ️ 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".
… read Two findings on the previous head, both correct, and both the same class one level below the fix that prompted them. A response that is an array can still carry an entry with no usable SHA. Skipping it made that branch look absent, and an absent branch is force-pushed with no record of the head it discarded -- the exact state this mechanism exists to prevent. An entry that cannot be read is the same claim as a response that cannot be read, so it is now the same refusal. A release pull request outlives a hundred comments, and the marker just written is the newest one, so a first-page read is precisely what drops it. The record then cannot be closed, and every later run reads the same truncated page, leaving the force-push permanently unpaired. Three call sites each had their own single-page request and their own array check. One reader now pages to exhaustion for all three, and throws rather than returning what it collected when it runs out -- a short answer here is indistinguishable from 'no such branch', and that reading is the failure.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bba8115a41
ℹ️ 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".
The marker parser told callers to check the comment's author themselves, and named no way to do it. One of the two readers did, with its own constant and its own login normaliser; the other did not, and that omission is a finding on this pull request. An instruction in a comment is not a mechanism. `isTrustedRewriteRecord` now lives beside the parser it qualifies, so both readers ask the same question of a comment, and a third reader has something to reach for rather than a note saying it ought to. The login normaliser moves with it, since the gate's review-author check wants the same normalisation and was the only reason a second copy existed.
Three list reads in this script decide whether a force-push is recorded, and each accepted an answer it should not have. **A marker from anyone.** Records read back were parsed without asking who wrote them. Anyone who can comment on a release pull request could have their comment taken for this workflow's own open record -- and completing a record rewrites the comment in place, so an unrelated comment would be overwritten, or an uneditable one would abort the run before any release work began. The workflow posts with `github.token`, so the recorder it trusts is the author it writes as. **An unreadable pull request entry read as "no pull request".** The caller then skips opening a record while the regeneration force-pushes the branch anyway. An empty list and an entry that cannot be read are different answers; only the first means there is nothing to record. **A head snapshot too old to say what is being discarded.** The record is opened before `createReleases()` so a failure there costs a re-run rather than a published release with no assets, which leaves a gap: anything reaching the branch between then and the force-push is discarded while the record still names the older head, and the gate accepts that pair and never searches the head that was lost. The record is now brought up to the branch's current head immediately before the rewrite. If it cannot be, the rewrite does not happen -- throwing would strand a published release without its assets, and a pull request that waits for the next run is recoverable where a discarded head is not. The refresh and the close walk the same path and differ only in what they write, so they share it.
Each fails with its guard removed. The comment fixtures now carry an author, which real ones always do -- that they did not is why the gap was invisible.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 261f140913
ℹ️ 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".
…reated **The lookup matched on head alone.** A generated branch can carry open pull requests against more than one base, and only the one targeting this run's branch is the release pull request being regenerated. Matched on head, the record could be opened and closed on a different pull request while the force-push rewrote this one, leaving the rewrite that mattered unrecorded. **The close read the branch, not the rewrite.** The review gate looks a record up by the head the force-push *created* -- it keys on the timeline event's `commit_id`. An ordinary commit landing on the branch between the rewrite and this read would have the record name a head no event mentions, and the gate would treat the discard as unidentified: exactly as if nothing had recorded it. I checked whether ancestry saved this, since an ordinary commit leaves the created head as its ancestor. It does not: `loadRecordedRewriteDiscards` keys the map by `marker.after` and the lookup is an exact SHA match, not a walk. The close now reads the created head from the force-push event itself. When no event names it, the record stays open -- which is how this module answers not knowing everywhere else: the gate ignores an open record and the next run completes it. Closing with an uncorroborated head would publish a claim about a rewrite nothing backs.
Both fail when their guard is reverted: closing from the branch head fails the two head cases, and dropping the base filter fails the lookup case.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 290757054c
ℹ️ 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".
lamemustafa
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa6d5640a0
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a07395a2d8
ℹ️ 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 record = expected.get(branch); | ||
| const createdHead = createdBranches.get(branch); | ||
| if (!record && !createdHead) { | ||
| throw new Error(`Release Please tried to rewrite unrecorded generated branch ${branch}.`); |
There was a problem hiding this comment.
When a merged release PR's generated head branch is retained, the next master-push run finds the ref but no open PR, so openBranchRewriteRecords() omits it from expected; Release Please then reuses that standard generated branch for the next release PR, and this check rejects its update as unrecorded. Because .github/workflows/release.yml invokes this wrapper on every master push, repositories that do not automatically delete merged head branches become unable to create another release PR until someone manually deletes the stale branch; capture that ref for a CAS that does not claim continuity for the closed PR, rather than rejecting all reuse.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2ec72e7. A retained generated ref with no open PR is captured only as a local exact-head snapshot. Its next update uses that head as the CAS precondition and emits no marker closing or closed-PR continuity claim. Refresh refuses head advances and newly appeared PRs; unrelated unrecorded refs remain unauthorized. Reuse, race and wrong-ref regressions pass. Full disposition is recorded in the PR body.
There was a problem hiding this comment.
HELD in 4bf1130. The marker-free retained-branch CAS has been removed; the wrapper now refuses a retained generated ref without an open release PR before release creation. This removes the unrecorded rewrite path, but normal retained-branch reuse remains unresolved. The existing ref CAS cannot atomically bind PR absence or create its PR-scoped marker, and a repeated absence lookup would leave the same race. This PR remains draft and must not merge until a durable recording design closes that functional gap. The linked #350 and PR body record the hold; no historical recovery is claimed.
|
@codex review Please review current head 2ec72e7. Confirmed receipts are now closed in finally before later regeneration errors propagate; retained generated refs after a closed PR use only an exact observed-head CAS, without closed-PR continuity claims. The new lifecycle/race cases pass in the 37-test focused suite. All original and subsequent findings are dispositioned in the body, with final full verification still pending. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ec72e72cc
ℹ️ 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".
| branch, | ||
| targetBranch, | ||
| }); | ||
| if (pullRequestNumber !== null) { |
There was a problem hiding this comment.
When a retained ref has no PR at this lookup but an automation or collaborator opens or reopens one before updateRefs, the exact-head CAS still succeeds and force-pushes that now-open PR. Fresh evidence beyond the closed-PR finding is that the new recordId: null path then creates no confirmed receipt at line 310, so the discarded head has no marker and review findings present only there can be lost. Couple the absence check to the rewrite, or detect and bind a PR appearing during this window before treating the CAS as marker-free.
AGENTS.md reference: AGENTS.md:L90-L92
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
HELD in 4bf1130. The marker-free retained-branch CAS has been removed; the wrapper now refuses a retained generated ref without an open release PR before release creation. This removes the unrecorded rewrite path, but normal retained-branch reuse remains unresolved. The existing ref CAS cannot atomically bind PR absence or create its PR-scoped marker, and a repeated absence lookup would leave the same race. This PR remains draft and must not merge until a durable recording design closes that functional gap. The linked #350 and PR body record the hold; no historical recovery is claimed.
|
@codex review Please inspect current head 4bf1130. This is explicitly a held draft: marker-free retained-branch rewrites are refused, so normal no-open-PR branch reuse remains unresolved and blocks merging. Receipt-closing failures now propagate when no release exists while all confirmed closes are attempted. The body contains all eighteen finding dispositions and final local evidence. |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Summary
Record generated release-branch rewrites through confirmed compare-and-swap receipts so the review gate can discover the exact discarded head. A concurrent advance prevents regeneration instead of silently replacing review history. Interrupted records without proof remain held.
Addresses #350 for future generated-branch rewrites; previously unnamed history remains outside this repair.
Root Cause / Decision Record
A separate head snapshot and unconditional ref update could disagree when another commit arrived between them. Inferring the created head from the current ref or a nearby timeline event could also attach the record to the wrong rewrite.
The wrapper intercepts the installed Release Please ref-update path and uses GraphQL
updateRefswithbeforeOid. It closes a record only from a confirmed before/after receipt. A newly created generated branch uses its confirmed creation head for the first conditional update. A retained generated branch without an open PR is explicitly refused before release creation: the ref CAS cannot atomically guarantee PR absence or create its review marker. Normal retained-branch reuse remains unresolved, so this is a safe draft fallback, not a merge-ready fix. Unknown outcomes retain an open record rather than guessing. No new state store or review-gate exemption is introduced.Scope
Pack Workflow Preflight
Preflight passed on the isolated branch; current master
c5df53634ac8a1479779d53bdeb5c65507c5209fwas integrated by a merge commit, preserving pushed head continuity.pnpm workflow:preflightwas run before editing/push, or the skip reason is documented.Sanchika Adoption Gate
Not applicable: no Sanchika adoption.
@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
No extension permissions, portal data flow or sensitive storage change. Tooling uses its existing GitHub authorization and repository API boundary.
Sensitive Surface Review
The ref mutation is conditional on the recorded head; ambiguous mutation results cannot produce a confirmed rewrite record. Already-created release outputs remain available if subsequent manifest setup, PR regeneration or closing bookkeeping fails. Without an actual release, those failures remain terminal; every confirmed receipt gets a closing attempt.
Chrome Web Store Impact
No Store scope or publication change. This is review-continuity tooling, not release or Store qualification.
docs/PUBLICATION_READINESS.mdis checked.Verification
Candidate
4bf1130ae6d808d1e138d9a77c5567f2b3ce2850passed all fourteen final local verification commands on a clean tree under Node 26.3.0 and pnpm 11.1.2: 176 files / 3344 tests, locked install/audit, prepare, formatting, lint, types, build, package and ZIP verification, preflight and diff checks. Independent source review confirms it is safe to publish as draft only. This candidate intentionally remains draft / HELD: the unsafe marker-free retained-branch CAS is removed, but normal reuse of a retained branch with no open release PR still needs a durable recording design. Existing GitHub ref CAS does not atomically bind PR absence or marker creation. The PR must not be merged while this functional blocker remains.Release publication/provenance/Store commands below are not applicable: this change creates no release. No new browser permissions or extension data flow; the existing GitHub API boundary gains a conditional ref mutation.
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:Artifact Evidence
4bf1130. Current-head Codex review reported no major issues. The strict gate still fails on the documented open retained-branch reuse finding, so the draft remains held.7327a439cb9f477139148f1d7976fe66ec744aa3e3af0ff5caa10be77caa0397(local validation only).4bf1130ae6d808d1e138d9a77c5567f2b3ce2850.PR Review Follow-Up
All automated asks are dispositioned below. The retained-branch reuse findings remain open and held; this PR is not merge-ready. #350 retains the forward-design and historical continuity gaps.
fa6d564: Open a trusted before-head marker before release creation or regeneration; open-record tests.fa6d564: Reject malformed matching-ref responses before mutation; malformed-head-list test.fa6d564: Regeneration/closing failures retain previously created release outputs; output-preservation regression.fa6d564: Validate each matching-ref entry and exact head before use; unreadable-entry regression.fa6d564: Paginate marker comments and fail on malformed pages; multi-page marker tests.fa6d564: Admit trusted workflow-authored markers only; forged-marker regression.fa6d564: Validate PR-match shape before regeneration; malformed-PR regression.fa6d564: Revalidate exact marker identity, then use its head as atomic updateRefs.beforeOid; ordinary/force-advance regressions.fa6d564: Match release PR by head and target base; wrong-base regression.fa6d564: Close using the confirmed CAS after-head, independent of any later ref advance; receipt regression.fa6d564: Replace generated updateRef with atomic updateRefs beforeOid CAS. A race is refused; confirmed receipts alone authorize closing.fa6d564: Timestamp/current-ref recovery cannot uniquely prove a rewrite when ordinary advances or unrelated force events intervene. Removed the inference; interrupted changed-head records explicitly remain held. Confirmed in-process receipts close only their actual before/after pair. No claim of automatic historical recovery; #350 retains that limitation.fa6d564: Retain exact opened record IDs and reject missing/changed records before regeneration; missing-marker regression.a07395a: without an actual created release, regeneration errors or failed record refresh are terminal; confirmed release outputs retain the asset-continuation behavior. Regression covers no-release failure, alongside existing created-release preservation.4bf1130: captures regeneration errors, attempts every confirmed marker-backed receipt, then propagates the named failures when no actual release exists. Regression performs CAS, fails the later PR request and observes exact receipt closing before rejection.4bf1130: prior normal-reuse implementation is withdrawn because PR appearance can race its final absence check. The safe fallback refuses retained refs with no open PR before release creation. Functional reuse remains unresolved and blocks merging.4bf1130: removes marker-free retained-ref mutation. The current marker reader reads the current PR; ref CAS alone cannot atomically bind that PR context. A durable recording design is required before normal retained-branch reuse can be restored safely.4bf1130: every confirmed receipt gets a closing attempt; no-release regeneration/closing failures propagate together with their named causes. Regression covers successful CAS followed by a later API failure and marker PATCH failure. Actual-release outputs remain available.Screenshots
Not applicable: tooling-only change. Tests use synthetic repositories/heads and stubbed GitHub calls.
Final candidate full-suite output, final three non-empty lines verbatim: