diff --git a/.github/workflows/dev-version-bump.yml b/.github/workflows/dev-version-bump.yml new file mode 100644 index 0000000000..45ecd4ae3f --- /dev/null +++ b/.github/workflows/dev-version-bump.yml @@ -0,0 +1,170 @@ +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. + # + # 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 + + # 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} 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 + + 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/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..b3b51de7d3 --- /dev/null +++ b/devlog/_plan/260830_dev_version_line_bump_pr/020_post_release_bump.md @@ -0,0 +1,202 @@ +# 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`. + +## 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. + +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. 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. 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", diff --git a/scripts/bump-dev-version.ts b/scripts/bump-dev-version.ts new file mode 100644 index 0000000000..e57d02e032 --- /dev/null +++ b/scripts/bump-dev-version.ts @@ -0,0 +1,205 @@ +#!/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 { existsSync, renameSync, unlinkSync, writeFileSync } from "node:fs"; + +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); + } + // 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. + 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..914861fd96 --- /dev/null +++ b/tests/bump-dev-version.test.ts @@ -0,0 +1,163 @@ +import { describe, expect, test } from "bun:test"; +import { chmodSync, mkdtempSync, readFileSync, readdirSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { dirname, 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 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); + }); + + // 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"); + 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"); + const proc = Bun.spawnSync(["bun", CLI, "nonsense", path]); + expect(proc.exitCode).not.toBe(0); + expect(readFileSync(path, "utf8")).toBe(before); + }); +});