From 933087100d8607413283d528f706f4d92a7351ff Mon Sep 17 00:00:00 2001 From: Minsu Lee Date: Wed, 16 Sep 2026 23:29:01 +0900 Subject: [PATCH] fix(release): classify a superseded tag as complete before it can publish MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `classifyRelease` asked whether a `+rebuild.N` had superseded this tag only after establishing that the release exists. A tag whose run failed before `gh release create` therefore returned `publish` no matter what the catalog named, and `release.yml` gates registration on `mode != 'complete'` — so `publish` runs both phases. Re-running an interrupted `+rebuild.1` after `+rebuild.2` had completed published a stale archive and then opened a catalog pull request repointing the entry from `rebuild.2` back to `rebuild.1`, dragging `released_at` backwards under a routine `chore(catalog): record …` title. The sequence was one re-run away on 2026-09-16: of the 47 Boot rebuilds cut that day, four runs — boot-3.3.6, boot-3.3.7, boot-3.5.6 and boot-3.5.7, all `+rebuild.1` — were cancelled at the Build step's 30-minute timeout, before any release existed. Cutting a `+rebuild.2` for any of them instead of re-running was all it would have taken. Supersession is now decided before publication is considered, so the two superseded rows of the decision table answer the same way. The check stays in `classifyRelease` rather than moving to `applyEntry`: the catalog records the rebuild last published, not the highest one, and that permissiveness is deliberate — republishing a known-good `rebuild.1` over a bad `rebuild.2` must remain expressible. What is wrong is not the write but a run making it by accident, which is a fact about what the run owes. The `complete` notice no longer claims `$TAG is published`, since the new row is reached precisely when it is not. ADR-0005 records the decision and the three options weighed; ADR-0003 keeps its original text and gains forward pointers from the row and Negative bullet this amends. Closes #9 --- .claude/skills/release-pipeline/SKILL.md | 6 + .github/workflows/release.yml | 13 +- .../0003-idempotent-release-recovery.md | 8 +- .../0005-supersession-on-the-publish-path.md | 142 ++++++++++++++++++ .please/docs/decisions/index.md | 1 + ARCHITECTURE.md | 2 +- scripts/lib/release-state.ts | 28 ++-- tests/unit/release-state.test.ts | 20 +++ 8 files changed, 202 insertions(+), 18 deletions(-) create mode 100644 .please/docs/decisions/0005-supersession-on-the-publish-path.md diff --git a/.claude/skills/release-pipeline/SKILL.md b/.claude/skills/release-pipeline/SKILL.md index b780d6fb..314db893 100644 --- a/.claude/skills/release-pipeline/SKILL.md +++ b/.claude/skills/release-pipeline/SKILL.md @@ -201,6 +201,12 @@ archive's sha256 to match the published one — the archive is reproducible by c (sorted entries, pinned timestamps, `gzip -n`), so a mismatch means the published bytes came from different input and must not be indexed. Publish a `+rebuild.N` tag in that case. +`complete` also covers a tag a later rebuild has taken over, including one whose run failed +before it published anything — re-running it would otherwise publish a stale archive and open a +catalog pull request moving the entry backwards (ADR-0005). The notice names the tag consumers +resolve to now. If you cut a `+rebuild.N` instead of re-running a failed job, the abandoned tag +stays abandoned; re-running it is a safe no-op. + ## Invariants not to break - GA versions only; no pre-releases. diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index b8006459..07d2862e 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -133,13 +133,16 @@ jobs: echo "mode=$mode" >> "$GITHUB_OUTPUT" case "$mode" in complete) - # Name the entry instead of asserting it is this tag. `complete` - # also covers a tag a later rebuild has superseded, for which - # "recorded in catalog.json" would be false and the operator's - # real question is which tag consumers resolve to now. + # Name the entry rather than asserting anything about this tag. + # `complete` also covers a tag a later rebuild has superseded, and + # such a tag need not be published at all — its run may have failed + # before `gh release create` (ADR-0005) — so neither "recorded in + # catalog.json" nor "is published" holds for every case reaching + # here. What the operator actually needs is the tag consumers + # resolve to now. current="$(jq -r --arg p "$PROJECT" --arg v "$VERSION" \ '.projects[$p][$v].tag' "$RUNNER_TEMP/catalog.json")" - echo "::notice::$TAG is published and catalog.json resolves $PROJECT $VERSION to $current; nothing to do. Correcting its archive needs a +rebuild.N tag." ;; + echo "::notice::catalog.json resolves $PROJECT $VERSION to $current; $TAG has nothing left to do. Correcting its archive needs a +rebuild.N tag." ;; register) echo "::notice::$TAG is published but catalog.json does not record it; completing registration only." ;; esac diff --git a/.please/docs/decisions/0003-idempotent-release-recovery.md b/.please/docs/decisions/0003-idempotent-release-recovery.md index aa54ac02..15aa073c 100644 --- a/.please/docs/decisions/0003-idempotent-release-recovery.md +++ b/.please/docs/decisions/0003-idempotent-release-recovery.md @@ -67,7 +67,8 @@ still outstanding and completes only those. | no | a newer rebuild of the pair | `publish`* | | no | this tag | refuse | - \* The one row this decision leaves as it found it — see Negative. + \* The one row this decision leaves as it found it — see Negative. **Amended by ADR-0005**, + which classifies that row `complete` and removes the asterisk. `publish` runs both phases. `register` skips publication and runs registration only. `complete` is a successful no-op. The last row cannot arise from an interrupted run — @@ -149,7 +150,8 @@ still outstanding and completes only those. it then registers it and moves the entry backwards. That path predates this decision — the guard it replaced keyed on the release existing, so it never fired here either — and closing it means deciding whether a superseded tag should still be publishable at all, which is a - separate question from recovery. Tracked as a follow-up. + separate question from recovery. Tracked as a follow-up. **Closed by ADR-0005** (issue #9), + which answers that question no and classifies the row `complete`. ### Neutral @@ -202,4 +204,6 @@ still outstanding and completes only those. - `scripts/release-mode.ts` — the entry point the workflow calls. - `scripts/lib/catalog-update.ts` — the tag-immutability rules this decision leaves intact. - `ARCHITECTURE.md` § "Release Invariants" — updated by this decision. +- ADR-0005 (`0005-supersession-on-the-publish-path.md`) — amends the decision table's fifth + row and closes the Negative bullet above. - Issue #6, and the Greptile review thread on PR #5 that raised it. diff --git a/.please/docs/decisions/0005-supersession-on-the-publish-path.md b/.please/docs/decisions/0005-supersession-on-the-publish-path.md new file mode 100644 index 00000000..69ab1490 --- /dev/null +++ b/.please/docs/decisions/0005-supersession-on-the-publish-path.md @@ -0,0 +1,142 @@ +# ADR-0005: Classify a Superseded Tag as `complete` Whether or Not It Was Published + +## Status + +Accepted — 2026-09-16 + +## Context + +ADR-0003 derives what a release run still owes from two observable facts — whether the GitHub +Release exists, and which tag `catalog.json` records for this `(project, version)` on the +default branch — and folds one more case into them: a catalog that has moved past this tag to a +*newer* `+rebuild.N` owes nothing, because registering the older tag is a legal catalog write +(`catalog-update.ts` repoints an entry at any rebuild of its own pair, deliberately recording +the rebuild last published rather than the highest one) that walks consumers backwards onto a +superseded archive. + +That check sat inside the branch for a release that already exists. Row 5 of the decision +table — release absent, catalog naming a newer rebuild — was therefore left as `publish*`, with +the asterisk pointing at a Negative bullet explaining why: closing it means answering a question +recovery did not raise, namely whether a superseded tag should still be publishable at all. + +The row is reachable, and `publish` is the damaging answer. `release.yml` gates registration on +`mode != 'complete'`, so `publish` runs *both* phases: + +1. `boot-4.1.1+rebuild.1` is tagged; its run fails during build, so no release is created. +2. The operator cuts `boot-4.1.1+rebuild.2` rather than re-running; it publishes and registers. +3. Someone re-runs the failed `rebuild.1` job, or re-pushes its tag. +4. `releaseExists === false` → `publish` → `rebuild.1` is published, and the catalog pull + request that follows repoints the entry from `rebuild.2` back to `rebuild.1`, dragging + `released_at` backwards with it. + +Publication itself is harmless — `rebuild.1` is a tag of its own and no published asset is +replaced. The catalog write is the damaging half, and it lands as a pull request titled +`chore(catalog): record boot-4.1.1+rebuild.1`, which reads as routine to whoever merges it. + +Step 1 is not hypothetical. Of the 47 Boot rebuild releases cut on 2026-09-16, four runs — +`boot-3.3.6`, `boot-3.3.7`, `boot-3.5.6` and `boot-3.5.7`, all `+rebuild.1` — were cancelled at +the Build step's 30-minute timeout, before any release existed. Had a `+rebuild.2` been cut for +any of them instead of a re-run, step 4 was one stale job re-run away. + +This supersedes ADR-0003's row 5 and closes the Negative bullet that recorded it; the rest of +ADR-0003 stands. + +## Decision + +**Supersession is decided before publication is considered.** `classifyRelease` asks whether the +catalog has moved past this tag first, and only then splits on whether the release exists: + +| Release exists | Catalog entry for this `(project, version)` | Mode | +| -------------- | ------------------------------------------- | ---------- | +| no | absent, or a tag this one supersedes | `publish` | +| yes | absent, or a tag this one supersedes | `register` | +| yes | this tag | `complete` | +| yes | a newer rebuild of the pair | `complete` | +| no | a newer rebuild of the pair | `complete` | +| no | this tag | refuse | + +Only the fifth row changes. `register` still proves the published bytes before indexing them, +`publish` still runs both phases, and the refusal for a catalog entry whose release is gone is +unchanged — it is checked first, since "the catalog names *this* tag" and "the catalog names a +*newer* rebuild" cannot both hold. + +**It stays in `classifyRelease`, not in `applyEntry`.** `applyEntry`'s permissiveness about +suffix ordering is the deliberate design ADR-0003 relied on and this decision leaves intact: the +catalog records the rebuild last published, so a genuine republication of `rebuild.1` after +`rebuild.2` — an operator correcting a bad `rebuild.2` by re-publishing the known-good +predecessor — must remain expressible. What is wrong is not the write; it is a *run* making it +by accident. Ordering is a fact about which run owes what, so it belongs where the other two +facts are already weighed. + +**No fourth mode.** The modes name what a run still owes (ADR-0003), and a run the catalog has +moved past owes nothing whichever phases it has left. The workflow's `complete` notice names +the tag the catalog actually resolves to, so an operator can still tell the cases apart, and it +no longer claims `$TAG is published` — which this row makes false. + +## Consequences + +### Positive + +- **The last backwards-catalog path is closed.** Both halves of the hazard ADR-0003 identified + now answer the same way, so no sequence of failed runs, rebuilds and re-runs can open a + pull request moving an entry onto a superseded archive. +- **Re-running a stale failed job stays a no-op rather than becoming an error.** The idempotence + ADR-0003 chose — re-running the tag is always the right instinct — now holds on this row too. +- **The rule got simpler, not larger.** Supersession was already implemented; moving it ahead of + the `releaseExists` split removed a branch rather than adding one, and the decision table + loses its footnote. + +### Negative + +- **A superseded tag can no longer be published at all.** Back-filling an older rebuild as an + archived artifact — publishing `rebuild.1`'s bytes for provenance after `rebuild.2` has taken + over the entry — is now a `complete` no-op. Nothing in the pipeline needs this today: the + catalog is the only way consumers discover a release (ARCHITECTURE.md), so an archive it does + not name is not reachable, and every one of the 155 published versions is the entry's own tag. + If it ever matters, the fourth mode rejected below is what to add. +- **Two facts no longer fully separate the modes.** `releaseExists` is now ignored on the + superseded rows, so the mode is not a function of the two booleans alone but of the tag + ordinals as well. That was already true for row 4; this makes it true for a second row. + +### Neutral + +- **The `complete` notice changed wording** to stop asserting that `$TAG` is published, since + the new row is reached precisely when it is not. +- **No workflow gating changed.** `publish` and `register` still key on the same step + conditions; the only difference is which runs reach them. + +## Alternatives Considered + +- **Publish but do not register — a fourth mode running phase 1 and skipping phase 2.** The only + option that preserves publishing a superseded tag, and the one to revisit if the Negative + above ever bites. Rejected now: it adds a mode whose name has to explain itself against the + "name what the run owes" rule, plus a third `if:` shape in `release.yml`, to serve a use case + that has never arisen in 155 releases. Speculative structure on the release path is the code + least likely to be correct when it finally runs. + +- **Refuse, naming the newer rebuild.** Loudest, and it would catch an operator re-pushing a tag + by mistake. Rejected: it makes a re-run of a stale failed job an error rather than a no-op, + which is exactly the reflex ADR-0003 set out to make safe. The `complete` notice already names + the tag the catalog resolves to, so the operator gets the same information without a red run. + +- **Enforce suffix ordering in `applyEntry`.** It would block the bad write at the last possible + moment, covering any future caller rather than this one path. Rejected: it removes a + capability the catalog's design states on purpose — the entry records the rebuild last + published, not the highest — and it would fail the run *after* a release has been published, + leaving the half-done state this whole area exists to avoid. + +- **Do nothing; rely on the pull-request merge gate.** Rejected: the gate is real but weak. The + pull request is titled `chore(catalog): record ` like every other one, and its diff shows + a tag and a timestamp changing, which is what a legitimate rebuild registration also looks + like. + +## Related + +- ADR-0003 (`0003-idempotent-release-recovery.md`) — the recovery decision this amends; its + decision table row 5 and the matching § Consequences → Negative bullet are superseded here. +- `scripts/lib/release-state.ts` — `classifyRelease`, the pure decision. +- `scripts/release-mode.ts` — the entry point `release.yml` calls. +- `scripts/lib/catalog-update.ts` — `applyEntry`, the permissive repoint this decision + deliberately leaves alone. +- `.github/workflows/release.yml` — the `complete` notice this reworded. +- Issue #9 — where the gap was filed, with the three options weighed above. diff --git a/.please/docs/decisions/index.md b/.please/docs/decisions/index.md index 9295e490..57b02820 100644 --- a/.please/docs/decisions/index.md +++ b/.please/docs/decisions/index.md @@ -8,3 +8,4 @@ | [0002](./0002-antora-as-a-library.md) | Drive Antora as a Library and Emit Markdown from the Asciidoctor AST | 2026-09-11 | Accepted | | [0003](./0003-idempotent-release-recovery.md) | Recover a Published-but-Unregistered Release by Re-running the Tag | 2026-09-12 | Accepted | | [0004](./0004-synthesize-3x-component.md) | Reconstruct the Generated Component for Spring Boot 3.x | 2026-09-12 | Accepted | +| [0005](./0005-supersession-on-the-publish-path.md) | Classify a Superseded Tag as `complete` Whether or Not It Was Published | 2026-09-16 | Accepted | diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 507da8ae..209b5eae 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -64,7 +64,7 @@ Conversion pipeline. Each top-level file is an executable Bun/TypeScript script | `convert.ts` | Pipeline entry — drive Antora's pipeline modules over a fetched tree and emit one Markdown file per page plus `_index.md`. | | `package-release.ts` | Pipeline entry — produce a reproducible `tar.gz` + `manifest.json` + SHA-256 checksum. | | `update-catalog.ts` | Pipeline entry — record a published `(project, version) → tag` in `catalog.json`. | -| `release-mode.ts` | Pipeline entry — report whether a tag still owes publication, registration, or nothing (ADR-0003). | +| `release-mode.ts` | Pipeline entry — report whether a tag still owes publication, registration, or nothing (ADR-0003, ADR-0005). | | `promote-markdown.ts` | Pipeline entry — copy a converted tree into the committed `markdown///`. | | `detect-upstream-versions.ts` | Tooling entry — list GA versions upstream has released that the catalog does not carry. Read-only; feeds the nightly issues and the build matrix. | | `lib/catalog-schema.ts` | zod schema for `catalog.json`. Owns the public catalog shape; changes require an ADR. | diff --git a/scripts/lib/release-state.ts b/scripts/lib/release-state.ts index bc71e308..50a1ab2b 100644 --- a/scripts/lib/release-state.ts +++ b/scripts/lib/release-state.ts @@ -21,6 +21,13 @@ * own pair — that walks consumers backwards onto a superseded archive. A run * whose tag the catalog has moved past owes nothing. * + * That last verdict does not depend on publication having happened. A tag whose + * run failed *before* `gh release create` is superseded the same way — by the + * rebuild cut in its place — and re-running it publishes an archive nobody + * asked for and then makes exactly that backwards catalog write. So supersession + * is decided before the two facts are split, not on the recovery path alone + * (ADR-0005). + * * Pure — the caller observes both facts and applies the verdict. */ @@ -32,7 +39,8 @@ * skip publication and record it, once the published bytes are shown to * match this run's rebuild. * - `complete` — the run is a no-op, because it owes nothing: both phases are - * done, or the catalog has already moved past this tag to a newer rebuild. + * done, or the catalog has already moved past this tag to a newer rebuild — + * whether or not this tag was ever published. * The modes are named after what a run still owes rather than after the state * it found (ADR-0003), so both cases are the same instruction to the caller. */ @@ -98,24 +106,24 @@ function isSupersededBy(tag: string, catalogTag: string): boolean { export function classifyRelease(state: ReleaseState): ReleaseMode { const registered = state.catalogTag === state.tag - if (!state.releaseExists) { - if (registered) { + if (registered) { + if (!state.releaseExists) { throw new Error( `catalog.json records tag "${state.tag}" but no such release exists. ` + 'A release is never recorded before it is published, so the release was ' + 'deleted or the catalog was edited by hand; repair the catalog before re-running.', ) } - return 'publish' - } - - if (registered) return 'complete' + } - // Published, unregistered, and the catalog has already moved on: the entry - // this run would write is the one a later rebuild replaced on purpose. + // The catalog has already moved on: the entry this run would write is the one + // a later rebuild replaced on purpose. Asked before publication is considered, + // because publication is not what makes the write wrong — both phases run + // together on the `publish` path, so a superseded tag that never published + // reaches the same backwards catalog write by a longer route. if (state.catalogTag !== null && isSupersededBy(state.tag, state.catalogTag)) return 'complete' - return 'register' + return state.releaseExists ? 'register' : 'publish' } diff --git a/tests/unit/release-state.test.ts b/tests/unit/release-state.test.ts index b961ab82..9d6d117a 100644 --- a/tests/unit/release-state.test.ts +++ b/tests/unit/release-state.test.ts @@ -74,6 +74,17 @@ const CASES: readonly Case[] = [ catalogTag: 'boot-4.1.1+rebuild.2', expected: 'complete', }, + { + name: 'is complete when a newer rebuild superseded a tag that was never published', + why: 'The rebuild.1 run failed before `gh release create`, so rebuild.2 was cut and ' + + 'completed both phases instead. Re-running the stale rebuild.1 job owes nothing: ' + + 'publishing it would be harmless on its own, but the catalog write that follows ' + + 'repoints the entry back onto it and drags `released_at` backwards with it.', + tag: 'boot-4.1.1+rebuild.1', + releaseExists: false, + catalogTag: 'boot-4.1.1+rebuild.2', + expected: 'complete', + }, { name: 'is complete when a rebuild has superseded the base tag', why: 'The same supersession seen from ordinal 0: the base tag is published and ' @@ -83,6 +94,15 @@ const CASES: readonly Case[] = [ catalogTag: 'boot-4.1.1+rebuild.1', expected: 'complete', }, + { + name: 'is complete when a rebuild superseded a base tag that was never published', + why: 'The same supersession at ordinal 0: the original tag never got a release, and ' + + 'the rebuild cut in its place is what consumers resolve to now.', + tag: 'boot-4.1.1', + releaseExists: false, + catalogTag: 'boot-4.1.1+rebuild.1', + expected: 'complete', + }, { name: 'orders rebuild suffixes numerically, not lexicographically', why: '"10" < "9" as text, which would read the catalog as being behind and '