prevent creating a release when the pinned js sdk version is differen… - #53
Conversation
…t from the one in the play repo
|
Claude finished @The64thRealm's task in 3m 5s —— View job Code review
Nice shape overall — gating at tag time rather than at PR time is the right call, the App-token approach beats a PAT in a public repo, and the security trade-off is written down honestly in
#1 is the one worth fixing before merge. The workflow only fires on Smaller things I checked and found fine: One thing I couldn't verify from this environment (no network for the GitHub API): whether Note: I only reviewed here, no code changes pushed. Also flagging that the checkout in this job has |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @franzwarning's task in 5m 37s —— View job Code review
The shell itself is in good shape — Every finding is about how the gate meets the workflow's trigger and play's release semantics, not the script. Four, posted inline:
#1 is the one worth fixing before merge. One doc nit not worth its own inline comment: the new Releases bullet says the source of truth is play's I only reviewed here — no code changes pushed. Note I can't edit |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @franzwarning's task in 4m 38s —— View job Code review
The shell is solid: Four findings, posted inline:
#2 is new since the last round and the easiest to miss. #1 is still unaddressed from the previous two rounds. I only reviewed here — no code changes pushed. I can't edit |
…e check Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @franzwarning's task in 4m 13s —— View job Code review
The latest commit is a genuine improvement: gating on Five findings, posted inline:
#1 is new in this round and the easiest to miss. #2 is carried over from both prior rounds and is still live. The trigger is still One thing folded into #1 rather than its own comment: the reverted text was the only place I only reviewed here — no code changes pushed. I can't edit |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @franzwarning's task in 4m 30s —— View job Code review
#1 is the one I'd want resolved before merge. Reverting Still live from the earlier rounds (unchanged code, so I didn't re-post these inline — the existing threads stand):
Things I checked that hold up: I only reviewed here — no code changes pushed. I can't edit |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @franzwarning's task in 6m 36s —— View job Code review
Two of the carried-over findings are fixed at this head: The core of the gate holds up. It's fail-closed, it runs before the tag push rather than after, Five findings, posted inline:
#1 is new in this round and is the one I'd fix before merge. The #3 and #5 are worth a decision, not necessarily a change. #3 (the Also worth noting: Things I checked and found fine: the tag format ( I only reviewed here — no code changes pushed. I can't edit |
| GH_TOKEN: ${{ secrets.PLAY_READ_PAT }} | ||
| run: | |
There was a problem hiding this comment.
PLAY_READ_PAT is now documented nowhere, and the failure mode it will eventually hit is undiagnosable.
Two things landed together across d9e029e → e2999ba: the ## Releases prose that explained this secret was reverted (correctly — CLAUDE.md should be clean against main), and 1d410df dropped the empty-GH_TOKEN pre-check that d9e029e had added. Net result:
$ grep -rn PLAY_READ . # excluding .git
./.github/workflows/auto-tag.yml:41
One hit, in the workflow itself. Nothing records that it must be a fine-grained PAT with Contents: read on wvdsh/play, that fine-grained PATs expire (so this will break on a date nobody has written down), or that it's bound to the account that minted it (so it dies when that person's access changes).
The failure is worse than just undocumented, because both variants die before the ::error:: on line 52 can fire:
- Secret unset or deleted →
${{ secrets.PLAY_READ_PAT }}expands to the empty string,ghtreats emptyGH_TOKENas unset, and line 46 dies withgh: To use GitHub CLI in a GitHub Actions workflow, set the GH_TOKEN environment variable.Whoever reads that looks at line 41, seesGH_TOKENis set, and is stuck. - PAT expired → bare
gh: Bad credentials (HTTP 401), no annotation, no mention ofPLAY_READ_PAT.
Either way every release is hard-blocked, on a step named "Verify the pinned sdk-js matches the version play ships" — which reads like play is ahead, not like the token lapsed.
Restoring the pre-check is three lines and puts the secret's name and requirements in the one place someone will definitely be looking:
| GH_TOKEN: ${{ secrets.PLAY_READ_PAT }} | |
| run: | | |
| set -euo pipefail | |
| if [ -z "${GH_TOKEN:-}" ]; then | |
| echo "::error::PLAY_READ_PAT is unset. The release gate needs a fine-grained PAT with Contents:read on wvdsh/play, set as the PLAY_READ_PAT repo secret. Fine-grained PATs expire — if this used to work, reissue it." | |
| exit 1 | |
| fi | |
| pinned=$(tr -d '[:space:]' < src/dev/sdk-js-version) |
That doesn't cover the 401 case, and it isn't a substitute for writing the runbook down somewhere — but "somewhere" needn't be CLAUDE.md; the secret's own description field or a line in the repo README would do.
| shipped=$(gh api -H "Accept: application/vnd.github.raw" \ | ||
| "repos/wvdsh/play/contents/package-lock.json?ref=$tag" \ | ||
| | jq -er '.packages["node_modules/@wvdsh/sdk-js"].version') | ||
| echo "cli pins $pinned; play $tag ships $shipped" |
There was a problem hiding this comment.
The remediation command in this message doesn't run as written — the script isn't executable.
$ git ls-files -s scripts/bump-sdk-js.sh
100644 cd62a35... 0 scripts/bump-sdk-js.sh
Mode 100644, not 100755. So the two ways someone would read "Run scripts/bump-sdk-js.sh 1.3.44" both fail:
./scripts/bump-sdk-js.sh 1.3.44→bash: ./scripts/bump-sdk-js.sh: Permission deniedscripts/bump-sdk-js.sh 1.3.44→ same (andcommand not foundif they drop the path)
Only bash scripts/bump-sdk-js.sh 1.3.44 works. The missing bit is pre-existing (the file landed non-executable in #52, and CLAUDE.md has the same ./scripts/bump-sdk-js.sh wording), but this PR is what makes it load-bearing: this string is the only instruction a maintainer gets at the moment a release is blocked, and it's the first thing they'll paste.
Cheapest fix that makes both this message and CLAUDE.md true is git update-index --chmod=+x scripts/bump-sdk-js.sh in a separate commit. Failing that, spell out the interpreter here:
| echo "cli pins $pinned; play $tag ships $shipped" | |
| echo "::error::wavedash dev would inject @wvdsh/sdk-js@$pinned but play $tag ships $shipped. Run bash scripts/bump-sdk-js.sh $shipped, verify a game boots with wavedash dev, then push the bump to retry the release." |
(Separately, "then push the bump to retry the release" is still not true — see the trigger-paths thread carried over from the earlier rounds.)
| id: tagged | ||
| run: | | ||
| if git ls-remote --exit-code origin "refs/tags/${{ steps.get_version.outputs.version }}" > /dev/null; then | ||
| echo "exists=true" >> $GITHUB_OUTPUT | ||
| else |
There was a problem hiding this comment.
Dropping || echo "Tag already exists" made the tag push non-idempotent, and this guard doesn't fully replace it.
Gating on steps.tagged is the right call and it covers the common case. But the old || fallback was also absorbing two cases this if doesn't, and both now surface as a red X on main for a push that would previously have been a silent no-op:
1. ls-remote conflates "no such ref" with "couldn't reach the remote." Per the docs, --exit-code returns 2 when no matching ref is found; a transport or auth failure is a generic fatal, 128. Both are non-zero, so both take the else branch and set exists=false. On a transient blip against an already-tagged version, the job then runs the gate (which can itself hard-fail on a pin that drifted after that version shipped — an error about a release that already happened), creates the tag locally (actions/checkout defaults to fetch-depth: 1 with no tags, so nothing collides), and dies on git push origin <tag> with rejected — already exists.
2. There's no concurrency: key, so two runs can both see exists=false. Merge a version bump, then merge a dependency-only Cargo.toml edit 30 seconds later — both jobs read the same pre-tag state, both pass the guard, and the loser's push is rejected. Narrow, but this workflow's trigger makes back-to-back Cargo.toml pushes normal.
Neither is dangerous — nothing double-releases — but each is a failure notification on a no-op, which is exactly the noise 1d410df set out to remove.
Distinguishing 2 from 128 fixes (1) and makes the intent explicit:
| id: tagged | |
| run: | | |
| if git ls-remote --exit-code origin "refs/tags/${{ steps.get_version.outputs.version }}" > /dev/null; then | |
| echo "exists=true" >> $GITHUB_OUTPUT | |
| else | |
| rc=0 | |
| git ls-remote --exit-code origin "refs/tags/${{ steps.get_version.outputs.version }}" > /dev/null || rc=$? | |
| if [ "$rc" -eq 0 ]; then | |
| echo "exists=true" >> $GITHUB_OUTPUT | |
| elif [ "$rc" -eq 2 ]; then | |
| echo "exists=false" >> $GITHUB_OUTPUT | |
| else | |
| echo "::error::Could not list tags on origin (git ls-remote exited $rc)" | |
| exit 1 | |
| fi |
For (2), a top-level concurrency: {group: auto-tag, cancel-in-progress: false} serialises the runs so the second one observes the first one's tag.
| name: Auto Tag on Version Change | ||
|
|
||
| on: | ||
| workflow_dispatch: |
There was a problem hiding this comment.
workflow_dispatch with no branch guard lets this job cut a release tag from any branch. Dispatching the workflow with "Use workflow from branch: some-feature-branch" checks out that branch, reads its Cargo.toml version, and — if that version isn't tagged yet — pushes the tag, which fires release.yml (tag pattern **[0-9]+.[0-9]+.[0-9]+*) and publishes binaries plus the Homebrew formula from non-main code. Before this change only pushes to main could produce a tag. Worth pinning the job to main:
| workflow_dispatch: | |
| workflow_dispatch: |
plus a job-level guard, e.g. if: github.ref == 'refs/heads/main'.
| set -euo pipefail | ||
| pinned=$(tr -d '[:space:]' < src/dev/sdk-js-version) | ||
| # play deploys prod on `release: published`, so its latest release is what's live | ||
| tag=$(gh api repos/wvdsh/play/releases/latest --jq .tag_name) |
There was a problem hiding this comment.
releases/latest returns the most recent non-prerelease, non-draft release, ordered by the tag's created_at — which doesn't match the comment above it. release: published fires for prereleases too, so if play deploys prod from a prerelease (or from a release whose tag was created earlier than another tag, e.g. a backport cut later), this reads a different release than the one that is live. Concrete failure: play publishes prerelease v2.0.0-rc1 bundling sdk 1.4.0 and prod deploys it; a dev correctly bumps the CLI pin to 1.4.0; this step reads the older full release shipping 1.3.43, blocks the release, and tells the dev to downgrade the pin to a version prod no longer runs.
| shipped=$(gh api -H "Accept: application/vnd.github.raw" \ | ||
| "repos/wvdsh/play/contents/package-lock.json?ref=$tag" \ | ||
| | jq -er '.packages["node_modules/@wvdsh/sdk-js"].version') |
There was a problem hiding this comment.
This is a fail-closed gate keyed on three unverified assumptions about a repo this one can't see: that play has a package-lock.json (npm, not pnpm/yarn/bun), that it's lockfileVersion 2/3 (v1 has no .packages), and that the sdk is hoisted to the top-level node_modules/@wvdsh/sdk-js key rather than a workspace path like apps/web/node_modules/@wvdsh/sdk-js. If any holds false, jq -er exits non-zero (or the contents call 404s) and every release is permanently blocked — with a bare jq/gh error, not the actionable ::error:: message below, and with a remediation ("bump the pin") that cannot possibly fix it. Note also that CLAUDE.md and scripts/bump-sdk-js.sh both name play/package.json as the source of truth, so the gate reads a different file than the docs. Consider failing with an explicit "could not determine the sdk version play ships" message when the lookup itself fails, so an unreadable play repo is distinguishable from a genuine mismatch.
| - name: Check if version is already tagged | ||
| id: tagged | ||
| run: | | ||
| if git ls-remote --exit-code origin "refs/tags/${{ steps.get_version.outputs.version }}" > /dev/null; then |
There was a problem hiding this comment.
git ls-remote --exit-code returns 2 for "no matching ref" but 128 for a genuine failure (network blip, expired HOMEBREW_TAP_TOKEN, DNS), and both land in the else branch as exists=false. So an auth/transport failure is reported as "this version isn't tagged yet": the sdk gate runs, and the job then dies at git push origin <tag> with a confusing "tag already exists" rejection instead of surfacing the real cause. Distinguishing the codes would make the failure self-explanatory:
| if git ls-remote --exit-code origin "refs/tags/${{ steps.get_version.outputs.version }}" > /dev/null; then | |
| if git ls-remote --exit-code origin "refs/tags/${{ steps.get_version.outputs.version }}" > /dev/null; then | |
| echo "exists=true" >> $GITHUB_OUTPUT | |
| elif [ $? -eq 2 ]; then | |
| echo "exists=false" >> $GITHUB_OUTPUT | |
| else | |
| echo "::error::git ls-remote failed; cannot tell whether ${{ steps.get_version.outputs.version }} is already tagged" | |
| exit 1 | |
| fi |
| git tag -a "${{ steps.get_version.outputs.version }}" -m "Release ${{ steps.get_version.outputs.version }}" | ||
| git push origin "${{ steps.get_version.outputs.version }}" |
There was a problem hiding this comment.
Dropping the || echo "Tag already exists" / || echo "Tag already pushed" fallbacks makes the check-then-push non-atomic and there's no concurrency: group on the workflow, so a benign duplicate now fails the job red. This is more likely after adding src/dev/sdk-js-version to the path filter, since two runs can now be in flight for the same Cargo.toml version: push A bumps the version to 0.1.93, push B (a pin bump, seconds later) contains that same version, both runs pass ls-remote before either reaches this step — the gate step adds several seconds of API latency in between — A pushes the tag and B dies on "tag already exists". A concurrency: { group: auto-tag, cancel-in-progress: false } block, or re-checking git ls-remote immediately before the push, would close the window.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @franzwarning's task in 2m 34s —— View job Code review
The gate itself continues to hold up: fail-closed, runs before the tag push, Three findings, posted inline. Two are consequences of this round's two commits.
#1 is the one I'd fix before merge. #2 is a scope question more than a defect. #3 is genuinely new because of On the threads you closed as won't-fix: the reasoning holds, and I'm not re-litigating them. Verifying play's lockfile is One loose end from the previous round that no commit has touched: I only reviewed here — no code changes pushed. I can't edit |
| name: Auto Tag on Version Change | ||
|
|
||
| on: | ||
| workflow_dispatch: |
There was a problem hiding this comment.
workflow_dispatch with no branch guard turns "cut a release" into a button anyone with write access can press on any branch. (Raised in the previous round and by Devin; a214795 added the trigger without the guard, so re-flagging once.)
For workflow_dispatch, actions/checkout@v4 checks out the ref picked in the UI. The job then reads version from that branch's Cargo.toml (line 27), finds no matching remote tag, and pushes an annotated tag at that branch's HEAD (line 63-64) using HOMEBREW_TAP_TOKEN. release.yml fires on version-shaped tag pushes and publishes binaries plus the Homebrew formula. Before a214795, only a push to main could produce a tag.
The sdk gate is no help here — it reads the pin from the same unreviewed branch, so a branch that bumps both files sails through.
Concretely: open a branch, set version = "9.9.9", dispatch Auto Tag on it → 9.9.9 is tagged at unreviewed code and shipped to the tap.
A job-level guard keeps the manual retry path (dispatch on main) while restoring the main-only invariant:
jobs:
tag:
runs-on: ubuntu-latest
if: github.ref == 'refs/heads/main'Worth deciding whether you want workflow_dispatch at all now that src/dev/sdk-js-version is in paths — the pin-only push covers the documented retry on its own, and dropping the trigger closes this without a guard.
| - Environment variables are managed by Doppler. Always use `doppler run --` as a prefix when running cargo commands (build, check, clippy, run, test, etc.). For example: `doppler run -- cargo check`, `doppler run -- cargo clippy`. | ||
|
|
||
| ## JS SDK | ||
| - `wavedash dev` injects `@wvdsh/sdk-js` from jsdelivr into the boot shell. The version is pinned in `src/dev/sdk-js-version` (one line, `include_str!`'d by `src/dev/server.rs`) — never `@latest`, so an SDK release can't change dev behaviour underfoot. Bump it with `./scripts/bump-sdk-js.sh` (no args = current npm `latest`), and keep it in step with the version play bundles in prod (`play/package.json`). |
There was a problem hiding this comment.
2c24236 deletes CLAUDE.md outright — new this round, and it goes further than the e2999ba restore it replaced. The PR now removes the whole file, including the ## Development Doppler rule and ## Project Purpose, neither of which this PR touches.
If that's deliberate, it's a separate call from "gate the release on the sdk pin" and reads better as its own PR. Two consequences worth being deliberate about:
- The gate's own error message points at docs this commit deletes. Line 54 tells a blocked maintainer to
Run scripts/bump-sdk-js.sh $shipped, verify a game boots with wavedash dev. After this merges,grep -rn "bump-sdk-js"finds only the script itself and this workflow — nothing explains that the pin must never be@latest, that it'sinclude_str!'d bysrc/dev/server.rs, or that the script takes no args to mean npmlatest. The one moment someone needs that context is the moment this gate fires. .github/workflows/claude.ymlandclaude-code-review.ymlreadCLAUDE.mdimplicitly. Deleting it means every future Claude run in this repo loses thedoppler run --instruction and will suggest barecargo check.
Restoring the file (i.e. dropping 2c24236) makes this PR the single-file change it was at a214795.
| - main | ||
| paths: | ||
| - 'Cargo.toml' | ||
| - 'src/dev/sdk-js-version' |
There was a problem hiding this comment.
Adding this path is the right fix for the retry, but it also widens the check-then-push race that 1d410df opened, and there's still no concurrency: key on the workflow.
Two runs can now be in flight for the same Cargo.toml version, which wasn't possible before this line:
- A version bump
0.1.92 → 0.1.93merges. Run 1:ls-remotesays untagged → gate runs (twogh apiround-trips to play, several seconds). - A pin bump merges 20s later — which this line now makes a trigger. Run 2 reads the same
0.1.93,ls-remotestill says untagged (run 1 hasn't pushed yet) → gate runs. - Run 1 pushes
0.1.93. Run 2 reaches line 64 and dies onrejected — already exists.
That used to be a green no-op twice over: the pin push queued no run at all, and || echo "Tag already pushed" absorbed the collision. Now it's a red X and a failure notification on main for a commit that was correct.
Nothing double-releases, so this is noise rather than danger — but it's the same class of noise 1d410df set out to remove, and it's one key at the top of the file:
concurrency:
group: auto-tag
cancel-in-progress: falseThat serialises the runs so the second one's ls-remote observes the first one's tag and skips cleanly.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @franzwarning's task in 2m 34s —— View job Code review
The gate itself is unchanged since One finding this round, posted inline — everything else is a carried-over thread that's still open against unchanged code, so I didn't duplicate it.
On #1. I raised the deletion last round as a scope question; this is the concrete consequence. There's no replacement in the tree — no Still open from earlier rounds (unchanged code — the existing threads stand, not re-posting):
Things I re-checked and found fine: an empty I only reviewed here — no code changes pushed. I can't edit |
| - Environment variables are managed by Doppler. Always use `doppler run --` as a prefix when running cargo commands (build, check, clippy, run, test, etc.). For example: `doppler run -- cargo check`, `doppler run -- cargo clippy`. | ||
|
|
||
| ## JS SDK | ||
| - `wavedash dev` injects `@wvdsh/sdk-js` from jsdelivr into the boot shell. The version is pinned in `src/dev/sdk-js-version` (one line, `include_str!`'d by `src/dev/server.rs`) — never `@latest`, so an SDK release can't change dev behaviour underfoot. Bump it with `./scripts/bump-sdk-js.sh` (no args = current npm `latest`), and keep it in step with the version play bundles in prod (`play/package.json`). |
There was a problem hiding this comment.
Deleting this file leaves scripts/bump-sdk-js.sh --help as the only surviving documentation of the pin — and it names a different file than the gate reads.
I flagged the deletion last round as a scope question; this is the concrete consequence, and it's sharper than "docs got thinner."
After 2c24236 there is no replacement — no AGENTS.md, and .claude/settings.json carries settings, not instructions. So the last written description of the pin is the script's own usage text:
Keep the pin in step with the `@wvdsh/sdk-js` version play bundles in prod
(play/package.json), and verify a game boots with `wavedash dev` after bumping.
But the gate compares against play's package-lock.json (line 49), specifically because — as the removed comment on the previous revision of this line noted — package.json only carries a caret range. A maintainer whose release just got blocked follows the one instruction they're given (Run scripts/bump-sdk-js.sh $shipped), reads --help for the source of truth, opens play's package.json, sees "^1.3.0", and has nothing to pin to. The $shipped value in the error message is what actually saves them — the docs actively point away from it.
Two smaller knock-ons from the same commit:
## Developmentand## Project Purposearen't yours to drop here.claude.ymlandclaude-code-review.ymlpick upCLAUDE.mdimplicitly, so every future Claude run in this repo loses thedoppler run --prefix rule and will suggest barecargo check— whichbuild.rsneeds env vars for. That's unrelated to gating the release.PLAY_READ_PATis now documented nowhere at all.grep -rn PLAY_READhits exactly one line (auto-tag.yml:42). Nothing records that it needs Contents:read onwvdsh/play, that fine-grained PATs expire, or whose account it's bound to.
If dropping the file is deliberate, that's a fine call — but it's a different PR than "gate the release on the sdk pin," and the two doc pointers the gate depends on (bump-sdk-js.sh --help naming the lockfile, and the PAT runbook) need to land somewhere first. Reverting 2c24236 makes this the single-file change it was at a214795.
…t from the one in the play repo