From cfe881f3b42e6ce91e3dd34c186efd754306f039 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sun, 30 Aug 2026 23:39:42 +0900 Subject: [PATCH 1/7] docs(devlog): plan the dev version-line bump PR after four audit rounds dev's package.json is 2.36.0 while tag v2.36.0 names c7d8407d2 on main, so tests/release-version-line.test.ts fails on dev and on every PR against it. The same defect has been repaired by hand four times (32529c2b2, e4a85d134, 076ad3036, befcac3e1) because nothing in scripts/release.ts or release.yml advances dev after a publish. Records the cause, the rejected options, and the shipped design: a separate release-triggered workflow that opens a version-bump PR against dev, plus a pure decision script that imports compareReleaseTags from release-notes.ts so scripts/release.ts stays untouched. Three audit rounds failed this plan before it passed, and each FAIL changed the design rather than the prose: a printed notice was rejected because the existing test is already louder than a printout; the first workflow could not have run (release events resolve from the default branch) or imported its comparator (module-scope process.exit); and the +minor bump rule contradicted befcac3e1, which moved dev to 2.36.0 on a preview-first publish. All four verdicts are recorded in the unit. --- .../000_cause_and_roadmap.md | 134 +++++++++++++++ .../010_version_repair.md | 55 ++++++ .../020_post_release_bump.md | 160 ++++++++++++++++++ .../030_ship.md | 73 ++++++++ 4 files changed, 422 insertions(+) create mode 100644 devlog/_plan/260830_dev_version_line_bump_pr/000_cause_and_roadmap.md create mode 100644 devlog/_plan/260830_dev_version_line_bump_pr/010_version_repair.md create mode 100644 devlog/_plan/260830_dev_version_line_bump_pr/020_post_release_bump.md create mode 100644 devlog/_plan/260830_dev_version_line_bump_pr/030_ship.md diff --git a/devlog/_plan/260830_dev_version_line_bump_pr/000_cause_and_roadmap.md b/devlog/_plan/260830_dev_version_line_bump_pr/000_cause_and_roadmap.md new file mode 100644 index 0000000000..32b1a7efd6 --- /dev/null +++ b/devlog/_plan/260830_dev_version_line_bump_pr/000_cause_and_roadmap.md @@ -0,0 +1,134 @@ +# dev version line: stop repairing it by hand + +Unit: `devlog/_plan/260830_dev_version_line_bump_pr/` + +Named for what it ships: a version-bump PULL REQUEST opened when a release publishes. +The unit was briefly called `..._autobump`, which an audit correctly rejected — the +workflow prepares the change and a human merges it, so nothing is automatic end to +end. +Goalplan: `repair-the-dev-version-line-and-add-a-post-relea` + +## The symptom, today + +`dev` head `df8b3882f` carries `package.json` version `2.36.0`. Tag `v2.36.0` +names `c7d8407d2`, which is `origin/main`. So the tree claims a version that is +already published from a different commit, and +`tests/release-version-line.test.ts` reports exactly that: + +``` +(fail) release version line > the in-tree version is never behind a released one +error: package.json version 2.36.0 equals release tag v2.36.0, but this commit is +not the one that tag names. The tree claims an already-published version: +publishing is refused as a duplicate. Bump package.json. +``` + +This fails CI jobs `test 2/4` and `macos` on `dev` itself (run 33312566315, cut at +`c2778ca3a` — `dev` has since advanced to `df8b3882f` and the failure still +reproduces there) and therefore on every PR opened against it. PR #3007 inherited +the same two red jobs for a two-file GUI change, and branch protection refused the +merge until it was overridden. + +## Why a one-line bump is not the fix + +The same defect has been repaired by hand FOUR times: + +| commit | what it did | +|---|---| +| `32529c2b2` | `2.24.2` -> `2.27.0`, after dev trailed the published channel by two releases | +| `e4a85d134` | `2.32.1-preview.20260825` -> `2.34.0`; also ADDED `release-version-line.test.ts` | +| `076ad3036` | `2.34.0` -> `2.35.0`, right after v2.34.0 shipped | +| `befcac3e1` | `2.35.0` -> `2.36.0`, after v2.36.0-preview.20260829 shipped | + +Note the second row: the detector was added DURING this sequence, and two more +hand-repairs followed it. Visibility was never the missing piece — that is the +finding that decided the design in `020`. + +Four repairs of one cause is a missing actor, not four accidents. The cause is +structural and visible in `scripts/release.ts`: the release runs only on `main` or +`preview` (`allowedBranches = ["main", "preview"]`, line 496), bumps +`package.json` there, commits `release: v`, pushes THAT branch, and +dispatches `release.yml`. The workflow ends at "Create GitHub release" — tag plus +GitHub release, nothing more. No step in either file ever advances `dev`. The +workflow declares `permissions: {}` at the top (line 32) and grants each job only +`contents: read` or `contents: write`, which is what makes an added `dev` write +there a security-review problem rather than a convenience. + +So the version line on `dev` goes stale the moment a release publishes, and stays +stale until a human notices red CI on an unrelated PR. The cost lands on +contributors: inherited red they did not cause and cannot fix from their own diff. + +What this unit can and cannot promise: it moves the repair from "someone eventually +remembers" to "a reviewable PR is waiting." It does not make the red impossible, +because the bump still needs a human merge — `Protect dev` requires an approving +review and code-owner sign-off, which a bot cannot supply. Claiming more than that +was the defect an audit caught in the first two drafts of `020`. + +## Constraint that shapes the design + +`dev` carries the NEXT STABLE version; the preview train adds its own suffix at +release time. That is the precedent `befcac3e1` states explicitly and the three +earlier repairs followed. A mechanism must preserve it — bumping dev to a preview +string would contradict every prior repair. + +The existing test is already the right detector. It reads the local tag set, needs +no network, and distinguishes "equal on the release commit" (legal) from "equal +anywhere else" (duplicate). Nothing about the detector needs changing. What is +missing is anything that PREPARES the repair: today the detector reports the problem +to whoever happens to open the next PR, and the fix is left to memory. + +## Phase map + +Each decade doc below is one full PABCD cycle. Dependency-ordered: the version +repair lands first because it unblocks CI for everything else, then the actor that +prepares the next repair as a reviewable PR, then the ship. + +- `010_version_repair.md` — move `dev` off the consumed `2.36.0` (wp2). +- `020_post_release_bump.md` — open the dev bump as a PR when a release publishes (wp3). + Note: that workflow only runs once it reaches `main`, the default branch. Merging it + to `dev` does not activate it. +- `030_ship.md` — PR against `dev`, CI evidence, merge (wp4). + +## Audit record + +TWO drafts of this roadmap were FAILED by an independent reviewer, and both verdicts +changed the design rather than the wording. + +Round 1: `020` chose a printed notice inside the release script and called it an +autobump. The reviewer showed the existing test is already louder than any printout, +and that two hand-repairs happened AFTER it landed. It also caught a wrong +"highest tag" claim in `010` and a test plan citing a `--dry-run` flag and reusable +shim helpers that do not exist. + +Round 2: the replacement PR-workflow design could not have worked. A `release` event +runs the workflow from the DEFAULT branch (`main`), which the scope forbade touching; +the named comparator `compareReleaseVersions` sits behind a module-scope +`process.exit` in `scripts/release.ts` and cannot be imported; and the "+minor" bump +rule contradicted `befcac3e1`, which moved `dev` to `2.36.0` on a +`v2.36.0-preview.*` publish. All three are fixed in the third draft, which imports +`compareReleaseTags` from `scripts/release-notes.ts` instead, records the `main` +promotion as a named follow-up in `030`, and replaces "+minor" with the two-branch +rule in `020`. The unit was also renamed. + +Round 3 caught the sequel to that last fix: "lowest unused stable" is not a pure +function of the script's two inputs, because "unused" is a property of the tag set and +the registry. The rule is now split — shape arithmetic in the script, freeness in the +tag-aware detector that already exists. It also caught that the out-of-scope list +below forbade the very promotion `030` depends on. + +Every rejected option and its reason stay in `020` so the decision is auditable. + +## Out of scope + +No publish, tag, or Release dispatch. No `main`/`preview` change IN THIS UNIT. No +merge of `main` back into `dev` to "sync" the version: `010_wp2_version_line.md` +names that as the trap that lands the consumed string on top of newer commits. + +That `main` exclusion is a scope boundary, not a claim that `main` is irrelevant. The +workflow in `020` cannot run until an ordinary maintainer-controlled promotion carries +it to the default branch; `030` records that as the named follow-up. Two consequences +worth stating plainly: + +- Merging this unit into `dev` fixes the red CI immediately (that is `010`) but arms + nothing (that is `020`, dormant until promotion). +- The next release cut from the CURRENT `main` will still strand `dev` one last time. + The loop closes on the release AFTER the workflow reaches `main`. diff --git a/devlog/_plan/260830_dev_version_line_bump_pr/010_version_repair.md b/devlog/_plan/260830_dev_version_line_bump_pr/010_version_repair.md new file mode 100644 index 0000000000..f74141e12c --- /dev/null +++ b/devlog/_plan/260830_dev_version_line_bump_pr/010_version_repair.md @@ -0,0 +1,55 @@ +# 010 — move dev off the consumed 2.36.0 (wp2) + +One line. `package.json` `version`: `2.36.0` -> `2.37.0`. + +## Why 2.37.0 + +Verified against the real state, not read off a pattern: + +| candidate | verdict | +|---|---| +| `2.36.0` (current) | tag `v2.36.0` names `c7d8407d2`, not dev's head; npm `latest` = 2.36.0. Consumed. | +| `2.36.1` | mechanically legal but labels the range a patch, against the `befcac3e1` precedent | +| `2.36.1-preview.*` | contradicts "dev carries the next STABLE version" | +| `2.37.0` | `npm view @bitkyc08/opencodex@2.37.0` -> E404; no `v2.37.0` in the tag set; forward of every tag | + +Highest existing tag by the repository's own ordering is `v2.36.0` — NOT the +later-dated `v2.36.0-preview.20260830`. Sorting all 218 `v*` tags with +`compareReleaseTags` puts the stable release above its own prerelease, which is +correct SemVer precedence and the reason the failing message names `v2.36.0`: + +``` +top 5: v2.34.0 v2.35.0 v2.36.0-preview.20260829 v2.36.0-preview.20260830 v2.36.0 +HIGHEST = v2.36.0 +compareReleaseTags("v2.37.0", "v2.36.0") -> 1 +``` + +The first draft of this doc asserted the preview was highest while claiming to have +run the comparator. It had not. Run it. + +npm dist-tags at the time of writing: `latest` = 2.36.0, `preview` = +2.36.0-preview.20260830. + +## The diff + +```json +- "version": "2.36.0", ++ "version": "2.37.0", +``` + +No other file carries the product version. `gui/package.json` is `0.0.0`, +`docs-site/package.json` is `0.0.1`, and `src/generated/*` hold catalog hashes. +Re-verify with a repo-wide search excluding `node_modules`, `.tmp`, `devlog`, +`gui/dist` before claiming the line is unique. + +## Verification + +- `bun test tests/release-version-line.test.ts` — all three tests pass, including + "the in-tree version is never behind a released one" which currently fails. +- Re-run the freeness checks (`npm view`, `git tag --list`) immediately before + committing: another release landing mid-cycle would consume the candidate. + +## What this does not do + +It does not publish, tag, or promote, and it does not stop the next release from +stranding dev again. That is `020`. diff --git a/devlog/_plan/260830_dev_version_line_bump_pr/020_post_release_bump.md b/devlog/_plan/260830_dev_version_line_bump_pr/020_post_release_bump.md new file mode 100644 index 0000000000..5732582596 --- /dev/null +++ b/devlog/_plan/260830_dev_version_line_bump_pr/020_post_release_bump.md @@ -0,0 +1,160 @@ +# 020 — open the dev bump as a PR when a release publishes (wp3) + +Third draft. Two independent audit rounds failed the first two; both verdicts and the +reasons are recorded below, because each one changed the design rather than the prose. + +## Round 1 rejected a printed notice (option C) + +> The detector already exists and is louder than a notice. +> `tests/release-version-line.test.ts` fails CI on every unrelated PR, and TWO +> hand-repairs happened after it landed — `e4a85d134` added it. A printout is a +> reminder, not a mechanism. + +Also verified: `dryRun = !args.includes("--publish")` (`scripts/release.ts:492`) makes +the default invocation a rehearsal, so a notice fires on every dry run until it is +trained away; and `release.yml` is `workflow_dispatch`, so an Actions-tab release +never runs `scripts/release.ts` at all. + +## Round 2 rejected the first PR-workflow design + +Three blockers, each confirmed against the repository: + +1. **It would never fire.** A `release` event runs the workflow file from the + DEFAULT branch. `gh repo view` reports `main`. This repository already documents + the identical trap in `cleanup-closed-pr-branches.yml:8-10` for scheduled + workflows. Landing the file on `dev` alone starts nothing. +2. **It could not import its comparator.** `compareReleaseVersions` is exported from + `scripts/release.ts:303`, but that file parses `process.argv` and calls + `process.exit(1)` at module scope (lines 487-491) with no `import.meta.main` + guard. `tests/release-version-line.test.ts:27-29` already records that importing + it kills the runner. +3. **The bump rule was wrong for preview-first releases.** `befcac3e1` moved `dev` + from `2.35.0` to `2.36.0` when the published tag was + `v2.36.0-preview.20260829`. "Increment the released core's minor" would have said + `2.37.0` and skipped a stable version that had not shipped. + +## Chosen design + +A separate workflow that opens a PULL REQUEST against `dev`, plus a pure script that +decides the version. + +**Honest scope.** This does not silently repair `dev`; it converts a forgotten chore +into a review-queue item that a human merges. Until that merge, +`release-version-line` stays red on `dev`. That is a real improvement over today — +a PR is durable where a printout is not, and it lands in the same place +`MAINTAINERS.md` already requires every `dev` change to land — but it is not an +autobump, and this unit should not be described as one. + +## The version rule + +Not "+minor" — that contradicts `befcac3e1`. But not "lowest unused stable" either, +phrased as if the script could evaluate it: "unused" is a property of the TAG SET and +the npm registry, and a pure function cannot see either. Stating the rule that way +would have made the doc unimplementable in exactly the manner the previous two drafts +were. + +Split the rule by who can answer it: + +**The script decides the CANDIDATE from the published version's SHAPE alone.** + +| published | candidate | precedent | +|---|---|---| +| `X.Y.Z-preview.*` (a prerelease of an unreleased core) | `X.Y.Z` | `befcac3e1`: 2.35.0 -> 2.36.0 on v2.36.0-preview.20260829 | +| `X.Y.Z` (stable) | `X.(Y+1).0` | `e4a85d134` 2.33.0 -> 2.34.0; `076ad3036` 2.34.0 -> 2.35.0; `32529c2b2` tip 2.26.0 -> 2.27.0 | + +Both rows are pure string arithmetic on the published version, and both are pinned by +tests. The prerelease row is the one that matters: the stable core of a +preview-first release has NOT shipped, so `dev` should carry it rather than skip it. + +**Freeness is verified where the tag set is visible.** The candidate is passed to the +existing detector, not re-derived: after the bump the workflow runs +`bun test tests/release-version-line.test.ts` in the `dev` checkout, which sorts the +real local tags with `compareReleaseTags` and fails if the candidate is at or behind +any published version. If that test fails, the workflow opens NO PR and the job goes +red — a visible request for a human decision, not a wrong PR. + +This is the honest division: shape arithmetic in the pure function, set membership in +the tag-aware gate that already exists. The script additionally refuses to emit a +candidate that `compareReleaseTags` does not rank strictly ahead of both `dev`'s +current version and the published one, which is the part it CAN check without I/O. + +## Files + +**`scripts/bump-dev-version.ts`** — pure decision logic, no git and no network. + +- Imports `compareReleaseTags` from `scripts/release-notes.ts`, NOT + `compareReleaseVersions` from `scripts/release.ts`. `release-notes.ts` guards its + CLI behind `import.meta.main` (line 1231) and already exports the comparator at + line 66, which is exactly why `release-version-line.test.ts` imports from there. + This avoids editing `scripts/release.ts` at all, keeping the release authority and + its security review surface untouched. +- Takes the released version and an explicit `package.json` path, so a test can + operate on a temp copy and the script is genuinely pure with respect to the + checkout. +- Emits a MACHINE CONTRACT, not prose: writes `changed=true|false` and `version=` + to `$GITHUB_OUTPUT` when set, and prints the same as JSON otherwise. Round 2 was + right that "print the chosen version" mixed with "print that nothing is needed" is + not an interface. +- `dev` already ahead -> `changed=false`, file untouched, exit 0. +- Malformed released version -> non-zero exit, file untouched. + +**`.github/workflows/dev-version-bump.yml`** — the actor. + +- Trigger: `release: [published]` only. No `workflow_dispatch`: round 2 correctly + noted that a branch-selected manual run executes THAT branch's body with + `contents: write`, which is the pattern this repository's own workflow comments + refuse. A missed run is re-driven by running the script by hand and opening the PR + normally. +- `permissions: {}` at the top; the single job takes `contents: write` (to push a new + `codex/dev-version-` branch — ruleset `Protect dev` covers only + `refs/heads/dev`, so the new branch is unprotected) and `pull-requests: write` (to + open the PR). Not `issues: write`, not `id-token: write`. +- `actions/checkout` with `ref: dev` AND `fetch-depth: 0` (or `fetch-tags: true`). A + `release` checkout defaults to the tag on `main`/`preview`, which is the wrong tree + to bump — and the tags are not optional decoration: `release-version-line.test.ts` + returns early on an empty tag set (line 93), so a shallow checkout would make the + freeness gate below silently vacuous rather than failing loudly. +- Do NOT copy `persist-credentials: false` from the repository's read-only workflows. + This job has to push its bump branch. +- Set up Bun and run `bun install` before the freeness gate: that gate is a + `bun test` invocation, not a shell comparison. +- Idempotent: if `codex/dev-version-` or its PR already exists, log and exit 0 + rather than failing the push. A second publish must not error. +- The workflow file must reach `main` to ever run. That is a promotion, not a + `dev`-only change, and `030` records it as an explicit follow-up rather than + pretending the merge to `dev` activates it. + +**Known limitation, stated not hidden:** a PR opened with `GITHUB_TOKEN` does not +start `pull_request` workflows, so the bump PR arrives without CI. `Protect dev` +additionally requires an approving review and code-owner review, and +`.github/CODEOWNERS` assigns `/.github/` and `/package.json` to human owners. A bot +cannot satisfy those. The PR is therefore a prepared, reviewable change — which is +the honest ceiling for automation here, and the reason the "autobump" framing is +dropped. + +## Test + +`tests/bump-dev-version.test.ts`, against temp copies of `package.json`. No shim +harness: the script is pure and takes a path. + +- dev `2.36.0`, released stable `2.36.0` -> `2.37.0`, `changed=true`. +- dev `2.35.0`, released stable `2.36.0` -> `2.37.0` (behind, not merely equal). +- dev `2.35.0`, released `2.36.0-preview.20260829` -> `2.36.0`. Pins `befcac3e1`, and + fails under a naive "+minor" rule, which is what makes it the load-bearing case. +- dev `2.36.0`, released `2.36.0-preview.20260830` -> `changed=false`: dev already + carries the prerelease's stable core, so there is nothing to do. +- dev `2.37.0`, released `2.36.0` -> `changed=false`, file BYTE-IDENTICAL, exit 0. +- dev `2.37.0-preview.1`, released `2.36.0` -> `changed=false`; a preview of a future + core is ahead, which `release-version-line.test.ts` already pins. +- released version malformed -> non-zero exit, file untouched. + +Red-first for a CLI is a behavioral red, not an import error: assert the chosen +version and the untouched-file invariant, and confirm each assertion fails against a +deliberately wrong rule (e.g. always `+minor`, which breaks the preview case) before +committing. + +## Also + +`MAINTAINERS.md`: after a release publishes, a `dev` version-bump PR is opened +automatically; merging it is part of closing out the release. Note that the workflow +only runs once it is on `main`. diff --git a/devlog/_plan/260830_dev_version_line_bump_pr/030_ship.md b/devlog/_plan/260830_dev_version_line_bump_pr/030_ship.md new file mode 100644 index 0000000000..5d1bb35545 --- /dev/null +++ b/devlog/_plan/260830_dev_version_line_bump_pr/030_ship.md @@ -0,0 +1,73 @@ +# 030 — ship it (wp4) + +## Branch and commits + +Branch `codex/dev-version-line-bump-pr` off `origin/dev`. Two commits, matching +the two implementation phases: + +1. `fix(release): move dev's version line past the published 2.36.0` +2. `feat(release): open the dev version bump as a PR when a release publishes` + +Plus the devlog unit. Push with `--no-verify` as the user directed. + +## PR + +Against `dev`, filling every `.github/PULL_REQUEST_TEMPLATE.md` section: Summary, +Verification, Checklist. No screenshot section is required — this touches no GUI. + +The description must state the four prior hand-repairs, because that history is the +argument for the mechanism. Reviewers who see only the version bump will read it as +routine maintenance. + +Release-tooling changes require explicit security review per `scripts/AGENTS.md` and +`MAINTAINERS.md`. Call that out in the description rather than leaving a reviewer to +discover it, and be precise about what the new workflow can do: it takes +`contents: write` to push a NEW unprotected bump branch and `pull-requests: write` to +open the PR. It does not use the release deploy key, does not write to protected +`dev` directly, and is a separate file from `release.yml` so the publish job's +permissions are unchanged. + +## Evidence required before the merge claim + +Local: +- `bun test tests/release-version-line.test.ts` — pass. +- `bun test tests/bump-dev-version.test.ts` — pass, including the NOOP case that must + leave `package.json` byte-identical. +- `bun test tests/release-helper.test.ts` — pass, proving the existing release + contract is unbroken. `scripts/release.ts` is deliberately NOT modified by this + unit, so this suite is a regression check rather than coverage of new behavior. +- `actionlint` on the new workflow if available; otherwise state that the YAML was + not machine-validated. +- `bun x tsc --noEmit` — clean. +- `bun run privacy:scan` — clean. +- `bun run prepush` — required by `scripts/AGENTS.md` for release-tooling changes. +- Red-then-green transcript for each new assertion. + +Remote: +- `gh pr checks` for the PR head showing `test 2/4` and `macos` GREEN. This is the + specific flip that proves the fix: those two jobs are red on `dev` today for this + exact test. + +The full local root suite is prohibited by the user. State that boundary in the PR +and rely on CI for whole-suite coverage. + +## Merge + +Merge into `dev` once the two previously-red jobs are green. If some unrelated job +is red, check whether it is also red on `dev` at `c2778ca3a` before deciding — the +point of this unit is to stop inheriting someone else's red, not to add to it. + +## Required follow-up, not part of this PR + +`.github/workflows/dev-version-bump.yml` DOES NOT RUN until it reaches `main`. A +`release` event resolves the workflow file from the repository default branch, which +`gh repo view` reports as `main`; this repository documents the same trap for +scheduled workflows in `cleanup-closed-pr-branches.yml:8-10`. + +So merging this PR into `dev` installs the file but arms nothing. The workflow first +fires after the next ordinary `dev` -> `main` promotion carries it there. That +promotion is maintainer-controlled (`MAINTAINERS.md`) and explicitly out of scope +here: this unit must not touch `main`. + +State this in the PR description. A reviewer who assumes the merge activates the +automation will believe the loop is closed a release earlier than it is. From 71fdea240fa1ccca6d2e25a621899b2e362a1131 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sun, 30 Aug 2026 23:47:03 +0900 Subject: [PATCH 2/7] fix(release): move dev's version line past the published 2.36.0 dev carried 2.36.0 while tag v2.36.0 names c7d8407d2 on main, so the tree claimed an already-published version from a different commit: release version line > the in-tree version is never behind a released one package.json version 2.36.0 equals release tag v2.36.0, but this commit is not the one that tag names. The tree claims an already-published version: publishing is refused as a duplicate. That failed test 2/4 and macos on dev itself (run 33312566315) and therefore on every PR opened against it, including #3007, whose own diff was two GUI files. 2.37.0 rather than 2.36.1 follows the precedent of all four prior repairs: dev carries the next stable version and the preview train adds its own suffix at release time. Freeness was verified live rather than assumed - no v2.37* tag, npm view @bitkyc08/opencodex@2.37.0 is E404, gh release view v2.37.0 is not found - and compareReleaseTags ranks v2.37.0 ahead of the highest tag v2.36.0. Note the highest tag is v2.36.0, not the later-dated v2.36.0-preview.20260830: sorting all 218 tags with the repository's own comparator puts a stable release above its own prerelease, which is why the failure message names v2.36.0. Verification: tests/release-version-line.test.ts goes 2 pass/1 fail -> 3 pass/0 fail. 260 pass / 0 fail across release-version-line, release-helper, release-notes, cli-version-skew, and service - the five suites that read package.json or assert on versions. test:changed selects nothing here because package.json is read as data, not imported, so those files were run explicitly. --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 03911d978a..b0f39a14aa 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@bitkyc08/opencodex", - "version": "2.36.0", + "version": "2.37.0", "description": "Universal provider proxy for OpenAI Codex & Claude Code — use any LLM with Codex CLI/App/SDK and Claude Code", "type": "module", "main": "./bin/package-main.mjs", From 8db0c794aac0ce7390eb76010fb6af628e81f79d Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sun, 30 Aug 2026 23:55:03 +0900 Subject: [PATCH 3/7] feat(release): open the dev version bump as a PR when a release publishes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit dev's version line goes stale the moment a release publishes, because scripts/release.ts runs only on main/preview and release.yml ends at "Create GitHub release". Nothing advances dev, so release-version-line.test.ts fails on dev and on every PR opened against it. That was repaired by hand four times (32529c2b2, e4a85d134, 076ad3036, befcac3e1). The second of those ADDED the detector and two more repairs followed it, so more visibility was never the missing piece. What this ships: - scripts/bump-dev-version.ts decides the version. Pure: no git, no network, so it is unit-testable and the credentials stay in the workflow. - .github/workflows/dev-version-bump.yml opens the PR on release: published. permissions {} at the top; the one job takes contents: write to push an unprotected codex/dev-version-* branch and pull-requests: write to open the PR. It never writes to dev and never uses the release deploy key, so release.yml and its review surface are untouched. The rule is not "increment the released minor" — that contradicts befcac3e1, which moved dev to 2.36.0 when v2.36.0-preview.20260829 published, because the stable 2.36.0 had not shipped. It keys off the published version's SHAPE: a prerelease of X.Y.Z means dev carries X.Y.Z; a stable X.Y.Z means dev moves to X.(Y+1).0. Freeness is not guessed either — the workflow runs release-version-line.test.ts against the rewritten tree and opens no PR if the candidate collides. Deliberate limits, stated rather than implied: a release event resolves the workflow from the DEFAULT branch, so this only fires once promoted to main; there is no workflow_dispatch, because a branch-selected manual run would execute that branch's body with contents: write; and a GITHUB_TOKEN PR does not start pull_request workflows, so the bump PR arrives without CI and a human merges it. This prepares the repair; it does not perform it. Verification: tests/bump-dev-version.test.ts 8 pass / 0 fail. Two real bugs were caught by those tests before commit — an ahead-check against the candidate instead of the released version, which would have downgraded a legitimate 2.37.0-preview.1 line, and a double "vv" prefix when handed the release tag_name the workflow actually passes. Each new rule was driven red: naive +minor fails 2 tests, the candidate-based guard fails 1, dropping the prefix normalisation fails 1. actionlint clean; every run block passes bash -n; the parsed YAML was asserted for permissions, trigger, and step list. --- .github/workflows/dev-version-bump.yml | 139 +++++++++++++++++++ MAINTAINERS.md | 15 ++ scripts/bump-dev-version.ts | 185 +++++++++++++++++++++++++ tests/bump-dev-version.test.ts | 120 ++++++++++++++++ 4 files changed, 459 insertions(+) create mode 100644 .github/workflows/dev-version-bump.yml create mode 100644 scripts/bump-dev-version.ts create mode 100644 tests/bump-dev-version.test.ts diff --git a/.github/workflows/dev-version-bump.yml b/.github/workflows/dev-version-bump.yml new file mode 100644 index 0000000000..5f0a1f42ef --- /dev/null +++ b/.github/workflows/dev-version-bump.yml @@ -0,0 +1,139 @@ +name: Dev version bump + +# When a release publishes, open a pull request that moves `dev` past the published +# version. Without this, `dev` keeps carrying a version that is at or behind a released +# one, and `tests/release-version-line.test.ts` fails on `dev` and on every pull request +# opened against it - inherited red a contributor cannot fix from their own diff. +# +# That has been repaired by hand four times: 32529c2b2, e4a85d134, 076ad3036, befcac3e1. +# The second of those ADDED the detector and two more repairs followed it, so more +# visibility was never the missing piece; a prepared change was. +# +# WHAT THIS DOES NOT DO. It does not push to `dev`. It opens a pull request and a human +# merges it, because ruleset `Protect dev` requires an approving review and code-owner +# sign-off that a bot cannot supply. Until that merge the red persists. This converts a +# forgotten chore into a queued, reviewable change - not into an automatic repair. +# +# A `release` event resolves this workflow file from the repository DEFAULT branch +# (`main`), not from `dev` - the same trap documented in cleanup-closed-pr-branches.yml. +# So merging this file to `dev` installs it but arms nothing; it first fires after an +# ordinary dev -> main promotion carries it there. +# +# There is deliberately no `workflow_dispatch`: a branch-selected manual run executes +# THAT branch body with `contents: write`. Re-drive a missed run by running +# `bun scripts/bump-dev-version.ts package.json` locally and opening the pull +# request normally. +on: + release: + types: [published] + +permissions: {} + +concurrency: + group: dev-version-bump + cancel-in-progress: false + +jobs: + open-bump-pr: + runs-on: ubuntu-latest + permissions: + # Push the new codex/dev-version-* branch. Ruleset `Protect dev` covers only + # refs/heads/dev, so the bump branch is unprotected and this token cannot + # bypass dev review. It is the ruleset that keeps this job off dev, not the + # permission name. + contents: write + # Open the pull request. + pull-requests: write + steps: + - name: Checkout dev + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 + with: + ref: dev + # Tags are load-bearing, not decoration: the freeness gate below is a bun + # test that reads the local tag set, and release-version-line.test.ts + # returns EARLY on an empty set. A shallow checkout would make that gate + # silently vacuous instead of failing loudly. + fetch-depth: 0 + # Do NOT set persist-credentials: false here as the read-only workflows do. + # This job has to push its bump branch. + + # The repository-owned composite action, not a hand-pinned setup-bun SHA: it + # resolves the Bun version from package.json so the runtime SOT stays in one + # place. An independently pinned action here would drift from every other job. + - name: Setup project Bun + uses: ./.github/actions/setup-project-bun + + - name: Install dependencies + run: bun install --frozen-lockfile + + - name: Decide the version dev should carry + id: decide + env: + RELEASED_VERSION: ${{ github.event.release.tag_name }} + run: | + set -euo pipefail + bun scripts/bump-dev-version.ts "${RELEASED_VERSION}" package.json + + - name: Prove the chosen version is unused + if: ${{ steps.decide.outputs.changed == 'true' }} + # The script decides the candidate from the released version SHAPE, which is all + # a pure function can see. Whether that candidate is actually FREE is a property + # of the tag set, so it is settled here by the detector that already owns the + # question. If this fails, no pull request is opened and the job goes red asking + # for a human decision - which is the correct outcome, not a fallback. + run: bun test tests/release-version-line.test.ts + + - name: Open the bump pull request + if: ${{ steps.decide.outputs.changed == 'true' }} + env: + GH_TOKEN: ${{ github.token }} + NEXT_VERSION: ${{ steps.decide.outputs.version }} + RELEASED_VERSION: ${{ github.event.release.tag_name }} + run: | + set -euo pipefail + + branch="codex/dev-version-${NEXT_VERSION}" + + # Idempotent: a second publish, a re-run, or a manual repair must not turn a + # successful release into a red job. + if git ls-remote --exit-code --heads origin "${branch}" >/dev/null 2>&1; then + echo "::notice::${branch} already exists; nothing to do" + exit 0 + fi + + git config user.name "github-actions[bot]" + git config user.email "41898282+github-actions[bot]@users.noreply.github.com" + git checkout -b "${branch}" + git add package.json + git commit -m "fix(release): move dev to ${NEXT_VERSION} after ${RELEASED_VERSION}" + git push origin "${branch}" + + gh pr create \ + --base dev \ + --head "${branch}" \ + --title "fix(release): move dev to ${NEXT_VERSION} after ${RELEASED_VERSION}" \ + --body "$(cat < package.json`, + then open the pull request normally. ## The retired `dev2-go` line diff --git a/scripts/bump-dev-version.ts b/scripts/bump-dev-version.ts new file mode 100644 index 0000000000..c5b61d11df --- /dev/null +++ b/scripts/bump-dev-version.ts @@ -0,0 +1,185 @@ +#!/usr/bin/env bun +/** + * Decide the version `dev` should carry after a release publishes. + * + * WHY THIS EXISTS + * + * `scripts/release.ts` runs only on `main` or `preview` (`allowedBranches`), bumps + * `package.json` there, and pushes that branch. `release.yml` ends at "Create GitHub + * release". Nothing advances `dev`. So the moment a release publishes, `dev` carries a + * version that is at or behind a published one, and + * `tests/release-version-line.test.ts` fails on `dev` and on every pull request opened + * against it — inherited red that a contributor cannot fix from their own diff. + * + * That has been repaired by hand four times: `32529c2b2`, `e4a85d134`, `076ad3036`, + * `befcac3e1`. The second of those ADDED the detector, and two more repairs followed + * it, which is the evidence that visibility was never the missing piece. + * + * WHAT THIS IS AND IS NOT + * + * This decides a version. It does no git and no network work, which is what makes it + * unit-testable and what keeps the credential surface in the workflow that calls it. + * It does not merge anything: `.github/workflows/dev-version-bump.yml` uses the + * output to open a pull request, and a human still merges that. Until they do, the + * red persists. This is a prepared repair, not an automatic one. + * + * THE RULE + * + * Not "increment the minor". That contradicts `befcac3e1`, which moved `dev` from + * `2.35.0` to `2.36.0` when the published tag was `v2.36.0-preview.20260829`: + * incrementing the released core's minor would have skipped the stable `2.36.0` that + * had not shipped yet. + * + * Not "lowest unused stable" either, however natural that sounds. "Unused" is a + * property of the tag set and the npm registry, and a function with no I/O cannot + * evaluate it. Stating the rule that way would make this file unimplementable as + * specified. + * + * The rule is therefore about the published version's SHAPE, which is the only thing + * this function can see: + * + * published `X.Y.Z-preview.*` -> dev becomes `X.Y.Z` (befcac3e1) + * published `X.Y.Z` (stable) -> dev becomes `X.(Y+1).0` (e4a85d134, 076ad3036, 32529c2b2) + * + * A prerelease means the stable core has not shipped, so `dev` should carry it. A + * stable release means that core is consumed, so `dev` moves to the next minor. + * + * Freeness is then checked where the tag set IS visible: the workflow runs + * `tests/release-version-line.test.ts` in the `dev` checkout after the rewrite. If the + * candidate collides with something published, that test fails, no pull request is + * opened, and the job goes red asking for a human decision. This file only enforces + * what it can prove without I/O — that the candidate ranks strictly ahead of both + * inputs by the repository's own comparator. + */ + +import { compareReleaseTags } from "./release-notes"; + +/** + * `compareReleaseTags` wants a tag. The workflow supplies `github.event.release.tag_name` + * (`v2.36.0`) while `package.json` holds a bare version (`2.36.0`), so prefixing blindly + * produces `vv2.36.0` and every comparison against it silently misorders. + * + * That is not hypothetical: it made the first version of this script reject a correct + * candidate with "candidate 2.37.0 does not rank ahead of released v2.36.0" when handed + * the tag the workflow actually passes. + */ +function asTag(version: string): string { + return version.startsWith("v") ? version : `v${version}`; +} + +/** + * `parseReleaseTag` in `release-notes.ts` is not exported, so parse here rather than + * widen that module's surface for one caller. Same shape, optional `v` prefix. + */ +function parseVersion(raw: string): { major: number; minor: number; patch: number; prerelease: string | null } | null { + const match = /^v?(\d+)\.(\d+)\.(\d+)(?:-(.+))?$/.exec(raw.trim()); + if (!match) return null; + return { + major: Number(match[1]), + minor: Number(match[2]), + patch: Number(match[3]), + prerelease: match[4] ?? null, + }; +} + +export interface BumpDecision { + changed: boolean; + /** The version dev should carry. Equals `current` when `changed` is false. */ + version: string; + reason: string; +} + +/** + * Pure decision. `released` is the version just published; `current` is what `dev` + * carries now. + * + * @throws when either input is not a parseable version. A malformed input must not + * silently produce a plausible-looking bump. + */ +export function decideDevVersion(released: string, current: string): BumpDecision { + const rel = parseVersion(released); + if (!rel) throw new Error(`released version is not parseable: ${JSON.stringify(released)}`); + if (!parseVersion(current)) throw new Error(`current version is not parseable: ${JSON.stringify(current)}`); + + const candidate = rel.prerelease === null + ? `${rel.major}.${rel.minor + 1}.0` + : `${rel.major}.${rel.minor}.${rel.patch}`; + + // Nothing to do when dev is already clear of the RELEASED version. That is the real + // question — the detector in tests/release-version-line.test.ts compares dev against + // published tags, not against this candidate. + // + // Comparing against the candidate instead is wrong, and a test caught it: dev at + // `2.37.0-preview.1` with `2.36.0` published is genuinely ahead of the release, but it + // is BEHIND the candidate `2.37.0`, so a candidate-based guard would "fix" a tree that + // was never broken and downgrade a legitimate prerelease line. release-version-line + // already pins that a prerelease of a future core outranks a published stable; these + // two must not disagree. + if (compareReleaseTags(asTag(current), asTag(released)) > 0) { + return { + changed: false, + version: current, + reason: `dev already carries ${current}, which is ahead of the published ${released}`, + }; + } + + // The candidate must beat the published version too. With the rule above this holds + // by construction, so a failure here means the rule and the comparator disagree — + // refuse rather than emit a version the detector would reject. + if (compareReleaseTags(asTag(candidate), asTag(released)) <= 0) { + throw new Error(`candidate ${candidate} does not rank ahead of released ${released}`); + } + + return { + changed: true, + version: candidate, + reason: rel.prerelease === null + ? `${released} is a stable release, so dev moves to the next minor ${candidate}` + : `${released} is a prerelease of an unshipped ${candidate}, so dev carries that core`, + }; +} + +if (import.meta.main) { + const [released, packageJsonPath] = process.argv.slice(2); + if (!released || !packageJsonPath) { + console.error("Usage: bun scripts/bump-dev-version.ts "); + process.exit(1); + } + + const file = Bun.file(packageJsonPath); + const raw = await file.text(); + const parsed = JSON.parse(raw) as { version?: unknown }; + if (typeof parsed.version !== "string") { + console.error(`${packageJsonPath} has no string version`); + process.exit(1); + } + + let decision: BumpDecision; + try { + decision = decideDevVersion(released, parsed.version); + } catch (err) { + console.error(`✗ ${err instanceof Error ? err.message : String(err)}`); + process.exit(1); + } + + if (decision.changed) { + // Rewrite only the version line. A full JSON round-trip would reformat the file + // and turn a one-line bump into an unreviewable diff. + const rewritten = raw.replace( + /("version"\s*:\s*")[^"]+(")/, + (_match, open: string, close: string) => `${open}${decision.version}${close}`, + ); + if (rewritten === raw) { + console.error("✗ could not locate the version line to rewrite"); + process.exit(1); + } + await Bun.write(packageJsonPath, rewritten); + } + + // A machine contract, not prose: the workflow branches on these values. + const output = process.env.GITHUB_OUTPUT; + if (output) { + await Bun.write(output, `changed=${decision.changed}\nversion=${decision.version}\n`); + } + console.log(JSON.stringify(decision)); +} diff --git a/tests/bump-dev-version.test.ts b/tests/bump-dev-version.test.ts new file mode 100644 index 0000000000..5234b3b079 --- /dev/null +++ b/tests/bump-dev-version.test.ts @@ -0,0 +1,120 @@ +import { describe, expect, test } from "bun:test"; +import { mkdtempSync, readFileSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { decideDevVersion } from "../scripts/bump-dev-version"; + +/** + * The bump rule that keeps dev off an already-published version. + * + * Every case here is a real repair this repository performed by hand. The rule was got + * wrong once during design - "increment the released minor" - and befcac3e1 is the + * case that disproves it, so that row is load-bearing rather than an edge case. + */ + +const CLI = new URL("../scripts/bump-dev-version.ts", import.meta.url).pathname; + +function tempPackageJson(version: string): string { + const dir = mkdtempSync(join(tmpdir(), "ocx-bump-")); + const path = join(dir, "package.json"); + // Two neighbouring keys and specific spacing on purpose: the CLI rewrites only the + // version line, and this fixture is what proves the rest stays byte-identical. + const body = [ + "{", + ' "name": "@bitkyc08/opencodex",', + ' "version": "' + version + '",', + ' "private": false', + "}", + "", + ].join("\n"); + writeFileSync(path, body, "utf8"); + return path; +} + +describe("dev version bump rule", () => { + test("a stable release moves dev to the next minor", () => { + // e4a85d134 (2.33.0 -> 2.34.0) and 076ad3036 (2.34.0 -> 2.35.0). + expect(decideDevVersion("2.36.0", "2.36.0")).toMatchObject({ changed: true, version: "2.37.0" }); + expect(decideDevVersion("2.36.0", "2.35.0")).toMatchObject({ changed: true, version: "2.37.0" }); + expect(decideDevVersion("2.33.0", "2.32.1-preview.20260825")).toMatchObject({ changed: true, version: "2.34.0" }); + }); + + test("a prerelease moves dev to that prereleases own stable core", () => { + // befcac3e1: published v2.36.0-preview.20260829, dev went to 2.36.0 - NOT 2.37.0. + // An "increment the released minor" rule returns 2.37.0 here and skips a stable + // version that has not shipped. This assertion is the whole reason the rule keys + // off the published version shape. + expect(decideDevVersion("2.36.0-preview.20260829", "2.35.0")).toMatchObject({ + changed: true, + version: "2.36.0", + }); + expect(decideDevVersion("2.36.0-preview.20260829", "2.35.0").version).not.toBe("2.37.0"); + }); + + test("dev already ahead is a no-op, not a downgrade", () => { + expect(decideDevVersion("2.36.0", "2.37.0")).toMatchObject({ changed: false, version: "2.37.0" }); + // A prerelease of a FUTURE core is ahead of a published stable. This is the same + // ordering release-version-line.test.ts pins, so the two must not disagree. + expect(decideDevVersion("2.36.0", "2.37.0-preview.1")).toMatchObject({ changed: false }); + // dev already carries the prerelease stable core. + expect(decideDevVersion("2.36.0-preview.20260830", "2.36.0")).toMatchObject({ changed: false }); + }); + + test("a v-prefixed release tag is accepted, not double-prefixed", () => { + // The workflow passes github.event.release.tag_name, which is "v2.36.0", while + // package.json holds a bare "2.36.0". Prefixing blindly built "vv2.36.0" and the + // comparison silently misordered, so the script rejected a correct candidate with + // "candidate 2.37.0 does not rank ahead of released v2.36.0". Both forms must agree. + expect(decideDevVersion("v2.36.0", "2.35.0")).toMatchObject({ changed: true, version: "2.37.0" }); + // Compare the DECISION, not the reason text: reason echoes the input verbatim, so it + // legitimately differs between the two forms while the outcome must not. + const tagged = decideDevVersion("v2.36.0", "2.35.0"); + const bare = decideDevVersion("2.36.0", "2.35.0"); + expect({ changed: tagged.changed, version: tagged.version }) + .toEqual({ changed: bare.changed, version: bare.version }); + expect(decideDevVersion("v2.36.0-preview.20260829", "2.35.0")).toMatchObject({ + changed: true, + version: "2.36.0", + }); + // And a v-prefixed dev version must not fool the ahead-check either. + expect(decideDevVersion("v2.36.0", "v2.37.0")).toMatchObject({ changed: false }); + }); + + test("a malformed version is refused rather than guessed at", () => { + expect(() => decideDevVersion("not-a-version", "2.36.0")).toThrow(/not parseable/); + expect(() => decideDevVersion("2.36", "2.36.0")).toThrow(/not parseable/); + expect(() => decideDevVersion("2.36.0", "garbage")).toThrow(/not parseable/); + }); + + test("the CLI rewrites only the version line", () => { + const path = tempPackageJson("2.36.0"); + const before = readFileSync(path, "utf8"); + const proc = Bun.spawnSync(["bun", CLI, "2.36.0", path]); + expect(proc.exitCode).toBe(0); + const after = readFileSync(path, "utf8"); + expect(after).toContain('"version": "2.37.0"'); + // Everything else survives. A JSON round-trip would reformat the file and turn a + // one-line bump into an unreviewable diff, so assert the inverse substitution + // reproduces the original exactly. + expect(after.replace('"version": "2.37.0"', '"version": "2.36.0"')).toBe(before); + }); + + test("the CLI leaves the file byte-identical when nothing is needed", () => { + const path = tempPackageJson("2.37.0"); + const before = readFileSync(path, "utf8"); + const proc = Bun.spawnSync(["bun", CLI, "2.36.0", path]); + expect(proc.exitCode).toBe(0); + // Byte-identical, not merely "still parses": a no-op run that reformats the file + // would open a pull request with a diff and no version change. + expect(readFileSync(path, "utf8")).toBe(before); + expect(new TextDecoder().decode(proc.stdout)).toContain('"changed":false'); + }); + + test("the CLI fails without writing when the released version is malformed", () => { + const path = tempPackageJson("2.36.0"); + const before = readFileSync(path, "utf8"); + const proc = Bun.spawnSync(["bun", CLI, "nonsense", path]); + expect(proc.exitCode).not.toBe(0); + expect(readFileSync(path, "utf8")).toBe(before); + }); +}); From afc24749a4c4c35da9b192da7141e696727f41cd Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sun, 30 Aug 2026 23:58:58 +0900 Subject: [PATCH 4/7] docs(devlog): record how the dev bump workflow differed from its plan Three deviations forced by the tree, not chosen: the composite setup-project-bun action instead of a hand-pinned setup-bun SHA, a local shape parse because parseReleaseTag is not exported, and a v-prefix normaliser because the workflow passes release.tag_name while package.json holds a bare version. Also records the ahead-check defect the tests caught: comparing dev against the candidate rather than the released version would have downgraded a legitimate 2.37.0-preview.1 line. --- .../020_post_release_bump.md | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/devlog/_plan/260830_dev_version_line_bump_pr/020_post_release_bump.md b/devlog/_plan/260830_dev_version_line_bump_pr/020_post_release_bump.md index 5732582596..c8da19e880 100644 --- a/devlog/_plan/260830_dev_version_line_bump_pr/020_post_release_bump.md +++ b/devlog/_plan/260830_dev_version_line_bump_pr/020_post_release_bump.md @@ -158,3 +158,29 @@ committing. `MAINTAINERS.md`: after a release publishes, a `dev` version-bump PR is opened automatically; merging it is part of closing out the release. Note that the workflow only runs once it is on `main`. + +## As implemented + +Shipped in `075a33be8`. Three deviations from the sketch above, recorded because each +was forced by the tree rather than chosen: + +1. **Bun setup uses the repository's composite action**, `./.github/actions/setup-project-bun`, + not a hand-pinned `oven-sh/setup-bun` SHA. That action resolves the version from + `package.json` so the runtime source of truth stays in one place; an independently + pinned SHA here would have drifted from every other job. The first draft of the + workflow pinned its own and disagreed with the one already in the tree. +2. **`parseReleaseTag` is not exported** from `release-notes.ts`, so the script does its + own shape parse rather than widening that module's surface for one caller. Only + `compareReleaseTags` is imported. +3. **A `v`-prefix normaliser was required.** The workflow passes + `github.event.release.tag_name` (`v2.36.0`) while `package.json` holds a bare version, + so prefixing blindly built `vv2.36.0` and every comparison against it misordered. It + surfaced as the script rejecting a correct candidate: "candidate 2.37.0 does not rank + ahead of released v2.36.0". Now pinned by a test. + +The tests also caught a defect the plan did not anticipate. The ahead-check originally +compared `dev` against the CANDIDATE, which is the wrong question: a `dev` at +`2.37.0-preview.1` with `2.36.0` published is genuinely ahead of the release but behind +the candidate `2.37.0`, so the script would have "repaired" a healthy tree and +downgraded a legitimate prerelease line. It now compares against the released version, +which is the same question `release-version-line.test.ts` asks. From 481e59933bf5606ecac4de2a360012844a1fcfd7 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 31 Aug 2026 00:23:14 +0900 Subject: [PATCH 5/7] fix(release): check for an open bump PR, not just the branch A security review of the workflow found the idempotency guard incomplete. It checked only whether codex/dev-version- existed as a branch, so an open bump PR whose head branch had been deleted left the check passing: the job would recreate the branch and then fail on gh pr create with "already exists", turning a successful release red for a repair that was already queued. Now checks for an open PR against dev first, then the branch. GH_TOKEN is already in scope for that step, so no new permission is needed. Also records the two residual gaps the review accepted rather than fixed: the GITHUB_OUTPUT write truncates rather than appends (equivalent today, not append-safe later), and no test exercises that output path. --- .github/workflows/dev-version-bump.yml | 12 ++++++++++++ .../020_post_release_bump.md | 16 ++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/.github/workflows/dev-version-bump.yml b/.github/workflows/dev-version-bump.yml index 5f0a1f42ef..5aa172f129 100644 --- a/.github/workflows/dev-version-bump.yml +++ b/.github/workflows/dev-version-bump.yml @@ -96,6 +96,18 @@ jobs: # Idempotent: a second publish, a re-run, or a manual repair must not turn a # successful release into a red job. + # + # Check the PULL REQUEST as well as the branch, not just the branch. A security + # review caught that: an open bump pull request whose head branch was deleted + # leaves the branch check passing, so the job would recreate the branch and then + # fail on `gh pr create` with "already exists" — turning a successful release red + # for a repair that was already queued. + open_prs="$(gh pr list --base dev --head "${branch}" --state open --json number --jq 'length')" + if [ "${open_prs}" != "0" ]; then + echo "::notice::a bump pull request for ${branch} is already open; nothing to do" + exit 0 + fi + if git ls-remote --exit-code --heads origin "${branch}" >/dev/null 2>&1; then echo "::notice::${branch} already exists; nothing to do" exit 0 diff --git a/devlog/_plan/260830_dev_version_line_bump_pr/020_post_release_bump.md b/devlog/_plan/260830_dev_version_line_bump_pr/020_post_release_bump.md index c8da19e880..b3b51de7d3 100644 --- a/devlog/_plan/260830_dev_version_line_bump_pr/020_post_release_bump.md +++ b/devlog/_plan/260830_dev_version_line_bump_pr/020_post_release_bump.md @@ -184,3 +184,19 @@ compared `dev` against the CANDIDATE, which is the wrong question: a `dev` at the candidate `2.37.0`, so the script would have "repaired" a healthy tree and downgraded a legitimate prerelease line. It now compares against the released version, which is the same question `release-version-line.test.ts` asks. + +A security review of the shipped workflow also found one gap worth recording. The +idempotency guard originally checked only whether the bump BRANCH existed. An open bump +pull request whose head branch had been deleted leaves that check passing, so the job +would recreate the branch and then fail on `gh pr create` with "already exists" - turning +a successful release red for a repair that was already queued. It now checks for an open +pull request first, then the branch. + +Two residual gaps are accepted rather than fixed, and named so a later reader does not +mistake them for oversights: + +- `Bun.write` to `$GITHUB_OUTPUT` truncates rather than appends. That is equivalent to a + first write today because the step emits nothing else, but it is not append-safe if a + later edit adds a second output in the same step. +- There is no test that exercises the `$GITHUB_OUTPUT` path itself; the tests cover the + decision and the file rewrite. From b2129b3211c0a4811d9d05151709f59a0a977513 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 31 Aug 2026 00:49:12 +0900 Subject: [PATCH 6/7] fix(release): reuse an orphaned bump branch and write package.json atomically Two review findings from the maintainer on #3013. An existing branch was treated as terminal success: if a prior run pushed the branch and then failed at pull-request creation, every rerun exited 0 with no pull request, leaving the repair permanently unqueued. The job now fetches the branch, asserts it carries exactly the one-line package.json bump to the expected version, fails closed on anything else, and resumes pull-request creation. The rewrite used a direct write of package.json. scripts/AGENTS.md requires atomic replacement for package metadata, and this script is also the manual recovery path, so an interrupt mid-write would strand an uninstallable checkout. It now writes a sibling temp file, renames it into place, and removes the temp on failure. Two regressions cover it: no debris after a successful rewrite, and a byte-identical original when the write fails. --- .github/workflows/dev-version-bump.yml | 37 +++++++++++++++++------ scripts/bump-dev-version.ts | 22 +++++++++++++- tests/bump-dev-version.test.ts | 42 ++++++++++++++++++++++++-- 3 files changed, 89 insertions(+), 12 deletions(-) diff --git a/.github/workflows/dev-version-bump.yml b/.github/workflows/dev-version-bump.yml index 5aa172f129..45ecd4ae3f 100644 --- a/.github/workflows/dev-version-bump.yml +++ b/.github/workflows/dev-version-bump.yml @@ -108,18 +108,37 @@ jobs: exit 0 fi + # An existing branch is NOT terminal. If a previous run pushed the branch and then + # failed at `gh pr create`, exiting here would leave the repair permanently unqueued + # while every rerun reports success - the exact failure mode a reviewer caught. So + # reuse the branch and fall through to pull-request creation instead. if git ls-remote --exit-code --heads origin "${branch}" >/dev/null 2>&1; then - echo "::notice::${branch} already exists; nothing to do" - exit 0 + echo "::notice::${branch} exists without an open pull request; validating it" + git fetch origin "${branch}" + + # Fail closed on unexpected content. The branch carries the bot's own one-line + # bump, so anything else on it means a human or another job is using that name and + # this job must not push to it or open a pull request from it. + changed_files="$(git diff --name-only "origin/dev...origin/${branch}")" + if [ "${changed_files}" != "package.json" ]; then + echo "::error::${branch} touches unexpected files: ${changed_files:-}" + exit 1 + fi + branch_version="$(git show "origin/${branch}:package.json" | node -p "JSON.parse(require('fs').readFileSync(0,'utf8')).version")" + if [ "${branch_version}" != "${NEXT_VERSION}" ]; then + echo "::error::${branch} carries ${branch_version}, expected ${NEXT_VERSION}" + exit 1 + fi + git checkout -B "${branch}" "origin/${branch}" + else + git config user.name "github-actions[bot]" + git config user.email "41898282+github-actions[bot]@users.noreply.github.com" + git checkout -b "${branch}" + git add package.json + git commit -m "fix(release): move dev to ${NEXT_VERSION} after ${RELEASED_VERSION}" + git push origin "${branch}" fi - git config user.name "github-actions[bot]" - git config user.email "41898282+github-actions[bot]@users.noreply.github.com" - git checkout -b "${branch}" - git add package.json - git commit -m "fix(release): move dev to ${NEXT_VERSION} after ${RELEASED_VERSION}" - git push origin "${branch}" - gh pr create \ --base dev \ --head "${branch}" \ diff --git a/scripts/bump-dev-version.ts b/scripts/bump-dev-version.ts index c5b61d11df..e57d02e032 100644 --- a/scripts/bump-dev-version.ts +++ b/scripts/bump-dev-version.ts @@ -52,6 +52,8 @@ * inputs by the repository's own comparator. */ +import { existsSync, renameSync, unlinkSync, writeFileSync } from "node:fs"; + import { compareReleaseTags } from "./release-notes"; /** @@ -173,7 +175,25 @@ if (import.meta.main) { console.error("✗ could not locate the version line to rewrite"); process.exit(1); } - await Bun.write(packageJsonPath, rewritten); + // Atomic replacement, per scripts/AGENTS.md: package metadata is exactly the class of + // file whose partial write corrupts a checkout. This script is also the documented + // manual recovery path, so it can run on a developer machine where an interrupt or a + // full disk mid-write would leave a truncated package.json and no way to install. + // Write a sibling temp file, rename it into place (atomic within one filesystem), and + // remove the temp on any failure so a crash leaves no debris. + const temp = `${packageJsonPath}.tmp-${process.pid}`; + try { + writeFileSync(temp, rewritten, "utf8"); + renameSync(temp, packageJsonPath); + } catch (err) { + try { + if (existsSync(temp)) unlinkSync(temp); + } catch { + // Nothing more to do: the original file is untouched, which is the point. + } + console.error(`✗ could not write ${packageJsonPath}: ${err instanceof Error ? err.message : String(err)}`); + process.exit(1); + } } // A machine contract, not prose: the workflow branches on these values. diff --git a/tests/bump-dev-version.test.ts b/tests/bump-dev-version.test.ts index 5234b3b079..1daf00e047 100644 --- a/tests/bump-dev-version.test.ts +++ b/tests/bump-dev-version.test.ts @@ -1,7 +1,7 @@ import { describe, expect, test } from "bun:test"; -import { mkdtempSync, readFileSync, writeFileSync } from "node:fs"; +import { chmodSync, mkdtempSync, readFileSync, readdirSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { dirname, join } from "node:path"; import { decideDevVersion } from "../scripts/bump-dev-version"; /** @@ -110,6 +110,44 @@ describe("dev version bump rule", () => { expect(new TextDecoder().decode(proc.stdout)).toContain('"changed":false'); }); + test("the rewrite is atomic and leaves no debris", () => { + // package.json is package metadata, so a partial write corrupts a checkout and this + // script is also the documented manual recovery path - it runs on developer machines + // where an interrupt or a full disk mid-write would strand an unusable file. + // scripts/AGENTS.md requires atomic replacement for exactly this class of file. + const path = tempPackageJson("2.36.0"); + const dir = dirname(path); + const proc = Bun.spawnSync(["bun", CLI, "2.36.0", path]); + expect(proc.exitCode).toBe(0); + // The temp sibling must be gone: a leftover .tmp- means the rename never + // happened and the write was not atomic. + expect(readdirSync(dir).filter(f => f.includes(".tmp-"))).toEqual([]); + expect(readdirSync(dir)).toEqual(["package.json"]); + // And the surviving file is complete, not truncated. + const after = readFileSync(path, "utf8"); + expect(JSON.parse(after).version).toBe("2.37.0"); + expect(JSON.parse(after).name).toBe("@bitkyc08/opencodex"); + expect(after.endsWith("}\n")).toBe(true); + }); + + test("an unwritable target fails closed with the original intact", () => { + // The atomic path must not destroy the original when the write itself fails. A + // read-only directory makes both the temp write and the rename impossible. + const path = tempPackageJson("2.36.0"); + const before = readFileSync(path, "utf8"); + const dir = dirname(path); + chmodSync(dir, 0o500); + try { + const proc = Bun.spawnSync(["bun", CLI, "2.36.0", path]); + expect(proc.exitCode).not.toBe(0); + // Byte-identical: the failure path must leave the checkout installable. + expect(readFileSync(path, "utf8")).toBe(before); + expect(readdirSync(dir).filter(f => f.includes(".tmp-"))).toEqual([]); + } finally { + chmodSync(dir, 0o700); + } + }); + test("the CLI fails without writing when the released version is malformed", () => { const path = tempPackageJson("2.36.0"); const before = readFileSync(path, "utf8"); From 0c897d589aec58f56ca37e580f14a2d3bb0cc537 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 31 Aug 2026 01:02:22 +0900 Subject: [PATCH 7/7] test(release): skip the unwritable-target case on Windows The read-only-directory test proves the atomic write fails closed, but chmod 0500 is not access control on Windows: the temp write would succeed there and the test would go red for a reason unrelated to the behavior under test. This file is a general suite member, so the Windows shards run it. Guarded with the same process.platform === win32 skip that tests/codex-native-residue.test.ts already uses for its EACCES case. The POSIX runners keep the coverage. --- tests/bump-dev-version.test.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/bump-dev-version.test.ts b/tests/bump-dev-version.test.ts index 1daf00e047..914861fd96 100644 --- a/tests/bump-dev-version.test.ts +++ b/tests/bump-dev-version.test.ts @@ -130,7 +130,12 @@ describe("dev version bump rule", () => { expect(after.endsWith("}\n")).toBe(true); }); - test("an unwritable target fails closed with the original intact", () => { + // Skipped on Windows: `chmod 0500` is not access control there, so the write would + // succeed and this test would fail red for a reason that has nothing to do with the + // behavior under test. Same guard as tests/codex-native-residue.test.ts uses for its + // EACCES case. The POSIX runners still cover the failure path. + const unwritableTest = process.platform === "win32" ? test.skip : test; + unwritableTest("an unwritable target fails closed with the original intact", () => { // The atomic path must not destroy the original when the write itself fails. A // read-only directory makes both the temp write and the rename impossible. const path = tempPackageJson("2.36.0");