fix(version): read the version from a package.json, and accept 3-digit semver - #2501
Open
YiftahR wants to merge 1 commit into
Open
fix(version): read the version from a package.json, and accept 3-digit semver#2501YiftahR wants to merge 1 commit into
YiftahR wants to merge 1 commit into
Conversation
…t semver
The version-path pin (--version-path / .gstack/version-path) already let a repo point
the version tooling anywhere, but two real-world shapes still failed — and both failed
CLOSED, which silently disabled /ship's queue-collision check rather than erroring:
1. A package.json as the version source. The readers treated the pinned path as raw
text, so a JSON file was whitespace-stripped into
'{"name":"frontend","version":"0.99.2",...' — which parseVersion rejected, so every
read fell through to the 0.0.0.0 default. This bites any repo whose version lives in
a package.json rather than a plain VERSION file, at the root or not.
2. 3-digit semver. parseVersion required exactly four components, so gstack-next-version
exited 2 on every invocation in such a repo. That CLI *is* the queue-collision check,
so /ship took its documented offline path — naive local patch arithmetic. Two branches
cut from the same base then pick the same version, and git merges that WITHOUT a
conflict because both sides set one line to identical text. The duplicate slot ships
silently: two PRs land as one version, and only one gets a CHANGELOG entry. We hit
this six times in one repo before working out why.
lib/version-source.ts now holds the semantics so both CLIs agree by construction.
Detection is by shape rather than new configuration: a version-path ending in .json is
read (and written) as JSON via .version; a version string with three components stays
three components through bumping and formatting. A repo with a root VERSION file and
4-digit versions sees no behaviour change.
Details worth reviewing:
- MICRO on a 3-digit version is carried out as a PATCH, with a warning in the output.
/ship auto-picks MICRO by default, so erroring would make it unusable in every 3-digit
repo; a silent no-op would be worse, since the caller would write back the version it
started with and claim a slot already taken.
- When the version-path IS a package.json, that file is the single source of truth: it is
the only file written, and the DRIFT_* states cannot arise (there is no second file to
drift from), so classify returns only FRESH / ALREADY_BUMPED and repair is a no-op.
Also syncing a root package.json there would be a guess about which of two JSON files
the repo publishes from.
- Fixes a pre-existing bug in gstack-version-bump: versionRel was derived from the CLI
flag alone, ignoring the .gstack/version-path pin, so a pinned repo compared its local
version against the BASE's root VERSION — two different files. On a repo with no root
VERSION the base then always read as 0.0.0.0 and every branch looked FRESH.
Two existing assertions encoded the old 4-digit-only contract (parseVersion('1.2.3')
is null; VERSION_RE rejects 3-digit). Both are updated with the reasoning inline, and
the garbage-rejection cases are kept and extended.
ship/SKILL.md is deliberately untouched: documenting the new shapes there also requires
regenerating the three host-variant copies and three golden fixtures, which looks like a
release chore rather than something to guess at from outside. Happy to add the prose in a
follow-up if you tell me the right way to regenerate those.
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
--version-path/.gstack/version-pathalready let a repo point the version tooling anywhere, but two shapes still failed — and both failed closed, so instead of erroring they silently disabled/ship's queue-collision check.In a repo whose version lives in
frontend/package.jsonas 3-digit semver:Two independent causes:
package.jsonversion source was read as raw text. The pinned path was whitespace-stripped, so a JSON file became{"name":"frontend","version":"0.99.2",...— whichparseVersionrejected, and every read fell back to the0.0.0.0default. This also silently discarded rival PRs' claims: the GitHub Contents API path base64-decodes the file and then parses it, so every competing claim was dropped as "malformed".parseVersionrequired exactly four components, sogstack-next-versionexited 2 on every invocation in a 3-digit repo.The second one is the damaging half, because
gstack-next-versionis the queue-collision check. With it erroring,/shiptakes its documented offline path — naive local patch arithmetic. Two branches cut from the same base then pick the same version, and git merges that without a conflict, because both sides set one line to identical text. The second PR's bump silently evaporates: two PRs ship as one version, and only one gets a CHANGELOG entry.We hit that six times in one repo before working out why — the tests pass, the diff looks right in isolation, and
git mergereports success, so there's nothing to notice.The fix
lib/version-source.tsholds the semantics so both CLIs agree by construction. Detection is by shape, not new configuration — no new flags or config keys:.jsonis read and written as JSON via.versionA repo with a root
VERSIONand 4-digit versions sees no behaviour change.After:
Decisions I'd most like a second opinion on
MICROon a 3-digit version is carried out as aPATCH, with a warning in the output./shipauto-picks MICRO by default, so erroring would make it unusable in every 3-digit repo — and a silent no-op would be worse, since the caller would write back the version it started with and claim a slot already taken. A hard error with a "pin a 4-digit VERSION file instead" message is the other defensible option.package.json, it's treated as the single source of truth: the only file written, and theDRIFT_*states can't arise (no second file to drift from), soclassifyreturns onlyFRESH/ALREADY_BUMPEDandrepairis a no-op. Also syncing a rootpackage.jsonthere would be guessing which of two JSON files the repo publishes from.Also fixes a pre-existing bug
gstack-version-bumpderivedversionRelfrom the CLI flag alone, ignoring the.gstack/version-pathpin — so a pinned repo compared its local (pinned) version against the base's rootVERSION: two different files. On a repo with no rootVERSIONthe base then always read0.0.0.0, making every branch lookFRESH.Tests
test/gstack-next-version.test.tsandtest/gstack-version-bump.test.ts, in a clean clone of this branch:New coverage: 3-digit parsing/width, formatting narrowing, the MICRO→PATCH carry, slot-picking within a width,
extractVersionfor JSON vs plain-text paths, and an end-to-end temp-repo suite drivingclassify→write→classify→repairagainst a pinnedfrontend/package.json— asserting the values that used to be wrong (baseVersionwas0.0.0.0,pkgExistswasfalse).I also ran the wider static tier. It has failures I could not attribute to this change, and I checked rather than assumed — stashing the change and re-running gives the same result:
swift build invariants > XCTest suite for StateServerfails identically on unmodifiedmainhere (toolchain), and the 5gstack-gbrain-detectfailures pass in isolation and reference none of the changed modules (they look like cross-file interference overHOME/GSTACK_HOMEin a concurrent full run). Happy to dig into either if they're not already known.Two existing assertions encoded the old contract and are updated with the reasoning inline —
parseVersion('1.2.3')was asserted null, andVERSION_REwas asserted to reject 3-digit. The garbage-rejection cases are kept and extended (1.2,1.2.3.4.5,v1.2.3.4,1.2.3.x).Deliberately not included
ship/SKILL.mdprose. Documenting the new shapes there breaks the byte-for-byte golden test, and fixing it properly means regenerating three host-variant copies and three golden fixtures — that reads like a release chore, and refreshing the goldens myself would make that test tautological. Happy to add the prose in a follow-up if you point me at the right regeneration path.CHANGELOG.md/VERSION. Those look maintainer-owned per the wave process in CONTRIBUTING.md. Say the word and I'll add an entry.