chore(master): release 0.6.0 - #337
github-actions[bot] wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a16d88367d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
a16d883 to
abdd12b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abdd12b850
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…#341) * fix(release): bump the security policy with the release instead of by hand Codex on #337, P1: `SECURITY.md` declared the `0.5.x` series while that PR sets package.json to 0.6.0, and `tests/docs/public-scope-copy.test.ts` derives the allowed series from package.json. `release.yml` runs the suite *before* `Run Release Please`, so merging #337 as-is fails the suite on master and the workflow stops before creating the tag or any release asset. Reproduced by setting the version to 0.6.0 locally: AssertionError: SECURITY.md names a version series the package is not on (package.json is 0.6.0): expected [ '0.5' ] to deeply equal [] The release PR would have been unmergeable in the only sense that matters: it could not have produced a release. Root cause is the shape this repository keeps hitting -- a hand-maintained duplicate of a fact with a canonical source. `release-please-config.json` already bumps `src/extension/version.ts`, `README.md` and `docs/PUBLICATION_READINESS.md` through `extra-files`; SECURITY.md restated the version and was not on that list, so it could only stay current by someone remembering, and the guard fired during the release rather than before it. SECURITY.md now names the exact release with the same `<!-- x-release-please-version -->` annotation the other three use, and joins `extra-files`. The exact form is also more accurate than the series it replaces: `0.5.x` reads as covering 0.5.0, which the row below it ("Previous releases | Not supported; upgrade to the current one") denies. Verified by simulating the release: bumping package.json and SECURITY.md together passes, and bumping package.json alone still fails with the drift message, so the guard keeps its purpose without blocking the release. * docs: distinguish the Store package from the release binary for GSTR-2B Codex on #337: both files said the GSTR-2B workbook builder "postdates v0.5.1", is not in a released binary, and that a user "cannot produce it yet". That is false for one of the two distributions Pack ships. Verified before changing the copy: - `src/entrypoints/offscreen/main.ts` imports `buildFiledReturnsGstr2bWorkbook` and branches on `gstr2bWorkbookApplicable` in the production offscreen entrypoint, so the builder is reachable in a built extension. - #337's generated changelog lists "add GSTR-2B invoice workbook (#196)" and the ITC summary sheet (#199) under 0.6.0, so the GitHub release binary carries it. - The Chrome Web Store publication is v0.5.0 and predates it, so a Store user genuinely cannot produce one. Both statements were true of different distributions and the copy collapsed them into one. It now says which is which, and keeps the Store restriction explicit rather than softening it. Both files also hard-coded `v0.5.1` on unannotated lines, two lines away from ones release-please bumps automatically -- the same duplicate that broke SECURITY.md. The corrected text names the Store publication instead, which moves only when a submission is recorded rather than with every release. This adds a positive claim about the GitHub release binary, so it is called out here for review rather than buried: it is evidenced by the entrypoint import above, not inferred from the changelog alone. * test(docs): fail when public copy hard-codes the current version Three files named `v0.5.1` by hand beside lines release-please already bumps, and each was wrong the moment a release landed -- SECURITY.md blocked the 0.6.0 release outright, README and PUBLICATION_READINESS told readers a shipped feature was in no binary. Nothing caught the class; the security guard caught one instance, and only during the release it then blocked. Adds a guard that fails when any of the three names the *current* package version without the release annotation. It fires when the hard-coding is written rather than one release later when it has become false. An older version stays legal on purpose -- the Store publication record is one, and it does not move when this package does. The security assertion now binds the exact version rather than a `0.N.x` series, and a second assertion states that SECURITY.md is an `extra-files` entry carrying the annotation, so the binding is automatic rather than remembered. Mutation-tested, each caught with the offending file and line: - current version hard-coded in README -> "README.md hard-codes v0.5.1 ... ['README.md:503']" - annotation stripped from SECURITY.md -> "SECURITY.md hard-codes v0.5.1 ... ['SECURITY.md:10']" - SECURITY.md removed from extra-files -> 1 of 10 fails * docs: stop asserting which release binary carries the GSTR-2B workbook Codex on #341, P1: the previous commit overcorrected. The current published release is `v0.5.1`, and `ceb6ada` introduced the builder after it -- the v0.5.1 tree contains zero occurrences of `buildFiledReturnsGstr2bWorkbook`. So the copy saying the GitHub release binary carries it was false for anyone downloading the current release. The entrypoint import proves source availability, not release availability, and I read one as the other. This is the second time this paragraph has been wrong, in opposite directions: first denying every distribution while the source already carried the builder, then asserting the release binary carried it before any release did. A sentence that names which release has a feature is wrong on one side of the release that changes the answer, and this file merges before that release. So it no longer restates the answer. It states the Store restriction, which is fixed and does not move with this package, and directs the reader to compare `CHANGELOG.md` against the release recorded above for the GitHub side. README points at this document rather than carrying a second copy of the status. Worth recording that the previous commit flagged this exact risk in its own message -- "this adds a positive claim ... called out here for review" -- and made the error anyway. Flagging a claim is not the same as verifying it against the artifact it describes. * test(docs): distinguish release-bound versions from fixed distribution records Codex on #341: the guard flagged every occurrence of the current version, including ones that legitimately record another distribution. When the Chrome Web Store catches up to the source version, the Store publication record in `docs/PUBLICATION_READINESS.md` names the same number, and the guard's advice -- add the release annotation -- would make release-please rewrite that record on the next source release even though no new submission happened. The guard would have instructed the maintainer to corrupt a historical fact, and it would have contradicted the distribution policy the same PR introduced. A line naming the current version now declares which kind it is: the release annotation when it tracks this package's release, or `<!-- pack-fixed-distribution-version -->` when it records a version that must not be rewritten. The marker is an escape hatch and no file needs one today, so it is exercised directly rather than left as untested affordance -- tested-but-never-reached is how a guard rots. The scan is now a pure function over text so both paths can be driven from synthetic input. The guard also caught the prose added in the previous commit, which named the current version while explaining why not to name it. That line was rewritten rather than marked. Mutation-tested: - fixed-distribution line without its marker -> flagged with file and line - marker no longer honoured in the helper -> "expected [ 1, 3 ] to deeply equal [ 1 ]"
abdd12b to
f59d877
Compare
Codex on #337: the notes credited #304 "simplify selection and completion paths", but `11cc788` is literally `Revert "fix(filed-returns): simplify selection and completion paths (#304)"` and its body records why -- a live authenticated run stalled on the first period of the year, for a period months past its filing date, where the same taxpayer and financial year had captured April through July before the change. release-please did not drop the entry because the revert's subject carries no conventional type prefix, so it never entered the changelog to cancel it. The notes therefore advertised behaviour that does not exist in this release. Marked rather than deleted: the attempt is real history, and #312 above is the reworked behaviour that survived, so a reader comparing the two entries needs both. Deleting would leave #312's "re-land" reading as a re-land of nothing. Scanned every other 0.6.0 entry for the same shape; this is the only one. The first scan searched full 40-character SHAs and found nothing, which was wrong -- revert bodies name the 7-character short SHA. Re-run against the known case first, then across the section. This edit lives on the release branch because the 0.6.0 section is generated and exists nowhere else; it survives only until the next master push regenerates it.
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. |
Codex on #337: the Features entry "add all-supported fiscal-year runner (#243)" reads as a capability this release ships. It is not one. The panel enables the flow only under `MODE === "source-surfaces"`: panel-surface.tsx:60 fullYearFlowAvailable = isPackSourceSurfaceBuildMode(import.meta.env.MODE) panel-guided-scope.tsx:91 sourceSurfacesEnabled = import.meta.env.MODE === "source-surfaces" and the comment above the second records that the literal comparison is deliberate so Vite constant-folds it and dead-code-eliminates the JSX from a packaged build. `README.md:68` already says the flow will not be available in the next production build. Release notes advertising it contradict the README and the standing prohibition on claiming durable full-year support. Ten entries in this section describe that flow, not one, so qualifying only the line Codex cited would leave nine reading the same way. A single note under the release heading states it once and covers the class; ten inline qualifications would be nine more copies of a fact to drift. Kept as a qualification rather than a removal. The work happened and the entries are accurate about source builds -- what was missing is which build they describe, which is the same distinction #341 drew for the GSTR-2B workbook.
d91447b to
79161c9
Compare
… 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.
dfeb9d1 to
d4e30c8
Compare
…carded 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>
…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>
15e3ec5 to
4fa6c09
Compare
4fa6c09 to
a3ee2e2
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…352) * fix(release): record the head each regeneration discards 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> * fix(release): make an interrupted rewrite record recoverable 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> * fix(release): read every page, and refuse a list entry that cannot be 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. * refactor(release): keep who may write a marker in one place 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. * fix(release): refuse three readings that let a rewrite go unrecorded 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. * test(release): pin the three readings a rewrite record depends on 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. * fix(release): name the right pull request, and the head the rewrite created **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. * test(release): pin which pull request, and which head, a record names 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. * fix(release): retain rewrite marker identity * fix(release): compare generated branch rewrites * fix(release): fail closed on ambiguous rewrites * fix(release): preserve original rewrite heads * fix(release): close rewrites from CAS receipts * fix(release): guard first generated branch update * fix(release): fail no-release regeneration errors * fix(release): close confirmed rewrite receipts * fix(release): hold unrecorded retained rewrites * fix(release): record retained-branch rewrites instead of refusing them A retained generated branch whose release pull request has already merged is the ordinary state of any repository that does not delete merged head branches. `openBranchRewriteRecords` refused it, so the wrapper rejected Release Please's reuse of that branch as unrecorded and no further release pull request could be created until someone deleted the branch by hand -- on a workflow that runs on every push to master. Record against the pull request whose head that branch was. A closed release PR is a durable home for the marker: it still names the head about to be discarded, which is what #350 needs, while claiming nothing about continuity for a review that is already closed. Only a branch with no pull request at all, open or closed, is still held -- nothing anywhere could hold its marker. That reuse opens a second window, and it is closed here rather than left: if a pull request appears after the record was written to a closed one, the exact-head CAS would still succeed and force-push that new review, whose discarded head is named nowhere. The absence is now re-checked immediately before the mutation, which is the last instant the answer still matters, and a rewrite that cannot be checked at all is refused rather than assumed safe. Every closed-home rewrite still produces a confirmed receipt, so no path reaches `updateRefs` without the discarded head being named. Addresses both P1 review threads on #352. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(release): declare the rewrite-CAS checker in the hand-written types CI's type-check rejected the new fifth argument as "Expected 4 arguments, but got 5" while a local `tsc --noEmit` on a stale tree reported clean. The cause is `scripts/run-release-please.d.mts`: a hand-written declaration that TypeScript resolves in preference to the implementation, so the `.mjs` signature and its JSDoc were never consulted. The declaration is a second source of truth for a fact the implementation already states -- the shape this repository keeps being bitten by -- and it was not updated with the parameter. Declares `assertNoPullRequestOpened`, and extracts `BranchRewriteRecord` so `recordHomeIsOpen` is stated once rather than spelled into three inline shapes. The parameter is optional. Omitting it is safe by construction rather than by convention: a closed-home rewrite with no checker is refused, not waved through, so the eight existing call sites that never produce a closed-home record need no change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
No description provided. |
fb4e7bf to
fc75aea
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
No description provided. |
fc75aea to
c88034a
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
No description provided. |
c88034a to
a853c4e
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
No description provided. |
a853c4e to
bbf8ab8
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
No description provided. |
bbf8ab8 to
b6acdb6
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
No description provided. |
b6acdb6 to
c8ee9e3
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
No description provided. |
c8ee9e3 to
6f97c4a
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
No description provided. |
6f97c4a to
1cee4af
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
No description provided. |
1cee4af to
832ae54
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
No description provided. |
832ae54 to
00e248c
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
🤖 I have created a release beep boop
0.6.0 (2026-09-23)
Features
Fixes
Documentation
Tests
Maintenance
Pack Workflow Preflight
pnpm workflow:preflightruns in Review findings gate.Privacy And Data-Flow Impact
Sensitive Surface Review
Verification
PR Review Follow-Up