Skip to content

fix(publish): make the npm release idempotent and verify it installs - #5633

Merged
georgi merged 2 commits into
mainfrom
claude/nodetool-cli-npm-setup-surm52
Sep 6, 2026
Merged

fix(publish): make the npm release idempotent and verify it installs#5633
georgi merged 2 commits into
mainfrom
claude/nodetool-cli-npm-setup-surm52

Conversation

@georgi

@georgi georgi commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

What changed

npm i @nodetool-ai/cli installs 0.7.0-rc.11 — the version the latest dist-tag still points at. The registry stops at rc.23 while this repo is at rc.40, because every publish run since rc.24 failed; the last three died on 404 Not Found - PUT .../@nodetool-ai%2fprotocol, an unauthorized npm token. rc.11 predates the validate, debug, node and harness commands, so what users get is missing half the CLI, and 35 of the 59 public packages (including 9 direct CLI dependencies) have never been published at all. Nothing in CI noticed for five months. Fixing the token is outside this repo; this diff stops the same failure from being silent and unrecoverable. scripts/publish-packages.mjs replaces npm publish --workspaces, which aborts on the first failing package — protocol is first in the workspace list, so one 404 there meant the other 58 were never attempted, and any package that did publish then returned EPUBLISHCONFLICT and killed the retry. The new script attempts every package, counts an already-published version as success, and exits non-zero at the end, so re-running after a partial publish finishes the job. A smoke job then installs the just-published CLI from the registry and asserts it reports the released version and lists its commands. Alongside that: nodetool --version no longer returns a hardcoded 0.1.0; nightly tags no longer match the v* publish trigger; engines is widened to >=22.0.0 so the CLI installs on Node 23+ without --force; vectorstore ships the sql/ migration its own error message tells users to run; and blender-nodes and reliability-harness get the README.md they already declared in files[].

Verification

  • npm run test:affected1 failure, load-induced, not from this diff. blender-nodes tests/settings.test.ts hit Test timed out in 5000ms while 44 vitest workers ran concurrently (that run's import phase took 229s). In isolation it passes in 3.86s, and the full package suite is green:
    npx vitest run tests/settings.test.ts --root packages/blender-nodes
      Test Files  1 passed (1)    Tests  2 passed (2)    Duration 3.86s
    npm run test --workspace=packages/blender-nodes
      Test Files  14 passed | 7 skipped (21)    Tests  88 passed | 30 skipped (118)
    
  • npm run typecheck — web and electron pass; the mobile leg fails with 36 errors, caused by the stale latest dist-tag this PR is about (mechanism below). Not introduced by this diff — confirmed identical on clean origin/main in a separate worktree, same exit code and same 36 errors:
    cd /tmp/nt-base/mobile && npx tsc --noEmit   # worktree at origin/main
      36 errors, MAIN_MOBILE_TC_EXIT=2
    cd mobile && npx tsc --noEmit                # this branch
      36 errors
    
    This diff touches no mobile file and no type-bearing source outside packages/cli/src/nodetool.ts; git diff origin/main -- packages/protocol/src packages/app-runtime/src is empty.
  • npm run lintLINT_EXIT=0
  • npm run dev:nodetool -- harness gate --base origin/mainGATE_EXIT=0, Gate: 15/15 selfchecks passed (it selects the new repo-scripts suite)
  • npm run build:packagesBUILD_EXIT=0, 62/62 tasks
  • npm run check:lockfile, check:deps, check:circular, check:agents-docs — all pass

Behavior verified against a real CLI rather than assumed:

npm run dev:nodetool -- --version        →  0.7.0-rc.40   (was 0.1.0)

Why the mobile typecheck fails, and why it is this bug

mobile/ is not a workspace, so npm ci there resolves @nodetool-ai/protocol: "*" from the registry, not from source. "*" resolves to the latest dist-tag, and latest is rc.11 — stale since April because publishing has been broken:

npm view @nodetool-ai/protocol dist-tags
  { "latest": "0.7.0-rc.11", "rc": "0.7.0-rc.23" }

node -p '…mobile/node_modules/@nodetool-ai/protocol/package.json'.version
  0.7.0-rc.11            # while packages/protocol is 0.7.0-rc.40

mobile/tsconfig.json maps @nodetool-ai/app-runtime (and two deep protocol paths) to local source, but not bare @nodetool-ai/protocol. So mobile compiles current-source app-runtime against five-month-old protocol types. The 36 errors are that skew:

rc.11  dist/api-types.d.ts:473   sync_mode: string;              # required
HEAD   src/api-schemas/…v1.ts:21 sync_mode: z.string().optional()  # optional

mobile/package-lock.json pins rc.11 as well, so CI's cd mobile && npm ci reproduces it. Fixing it properly means pinning mobile's protocol dependency or adding a paths entry for the bare specifier — a mobile-build change, deliberately out of scope here — but it resolves itself for new installs once latest points at a current release.

Agent capabilities

No capability added, and no capability's declared contract changed.

New checks

  • The classification test was inverted once and observed failing. Changing return "skipped" to return "failed" in classifyPublishResult:
    FAIL scripts/__tests__/publish-packages.test.mjs > classifyPublishResult >
         counts an already-published version as skipped, not failed
    Tests  1 failed | 4 passed (5)
    
    Restored: Tests 5 passed (5).
  • The publish-skip predicate was shown to discriminate, not to return one value:
    protocol@0.7.0-rc.23 (exists)          -> true
    protocol@0.7.0-rc.40 (missing)         -> false
    compute@0.7.0-rc.23 (never published)  -> false
    
  • The smoke assertion was shown to fail on the version it must catch. The obvious form, nodetool validate --help, is not a usable check: commander's --help short-circuits before unknown-command handling, so on rc.11 — which has no validate command — it prints top-level help and exits 0. Replaced with a check that the command is listed, which correctly rejects rc.11 and accepts the current build:
    rc.11:   MISSING validate / debug / node / harness   (check fails — correct)
    source:  HAS validate / debug / node / harness       (check passes)
    
  • The publish script enumerates its targets and was shown to find them — 59 public packages, with the 3 private codegen packages excluded — so it cannot pass by matching nothing.

Notes for the reviewer

Two things this diff deliberately does not fix:

  1. The latest dist-tag stays at rc.11 until someone moves it. Prereleases only move the rc tag, so even a green publish leaves npm i @nodetool-ai/cli on rc.11 — and, as above, leaves mobile compiling against rc.11 types. That needs npm dist-tag add per package, or a stable (non-prerelease) release.
  2. The existing "Verify dry-run" step is a check that cannot fail. It runs prepare-publish.mjs --dry-run after the real prepare-publish.mjs, so every "*" dep is already rewritten and it always reports "No workspace * dependencies found to resolve." Same class of bug as the rest of this PR; left alone to keep the diff scoped — happy to fix it here if preferred.

Unblocking the release still needs the npm side: a token authorized for the @nodetool-ai scope, or Trusted Publishing (OIDC) for this repo, which the workflow already has id-token: write for.

🤖 Generated with Claude Code

https://claude.ai/code/session_01HWxheb5NMNMeUDgeQwykjk

Every publish run since 0.7.0-rc.24 failed, so npm has stopped at rc.23 while
the repo is at rc.40, and the "latest" dist-tag still points at rc.11 — the
version a plain `npm i @nodetool-ai/cli` installs. rc.11 predates the validate,
debug, node and harness commands, so the published CLI is missing half its
surface. Nothing in CI noticed for five months.

The immediate cause is an unauthorized npm token (404 on PUT for a package that
exists), which is fixed outside this repo. These changes stop the same failure
from being silent and unrecoverable:

- scripts/publish-packages.mjs replaces `npm publish --workspaces`, which aborts
  on the first failing package. protocol is first in the workspace list, so one
  404 there meant the other 58 packages were never attempted. The script tries
  every package, treats an already-published version as success, and exits
  non-zero at the end — so a re-run after a partial publish finishes the job
  instead of dying on EPUBLISHCONFLICT.
- A smoke job installs the just-published CLI from the registry and asserts it
  reports the released version and lists its commands. `nodetool validate
  --help` is not usable as that check: commander's --help short-circuits before
  unknown-command handling, so it prints top-level help and exits 0 even on
  rc.11, where the command does not exist.
- nodetool --version read a hardcoded "0.1.0" that never moved across any
  0.7.0-rc release. It now comes from package.json.
- Nightly tags match the workflow's "v*" trigger and would publish to npm daily;
  excluded.
- engines widened from ">=22.0.0 <23.0.0" to ">=22.0.0" so the CLI installs on
  Node 23+ without --force.
- vectorstore ships sql/, which its own error message tells users to run.
- blender-nodes and reliability-harness listed README.md in files[] without
  having one.

georgi commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator Author

Two checks went red on 07b4aa1. Neither is this PR's, and I've merged current main in (003b4687) so CI re-runs against an up-to-date base. Details and evidence for each:

1. Quality Gate (npm run check) / test-app — broken on main, no fix exists yet

web/src/components/timeline/Inspector/__tests__/TimelineInspector.motionControls.test.tsx — 4 failed, 10 passed:

> 323 |       screen.getByText(/not an easing this build reads; plays linear/i)
        Unable to find an element with the text: /not an easing this build reads; plays linear/i

That file does not exist on this PR's branch point; it arrived on main in aeda2232b feat(web): give every motion field an inspector control, and CI tests the merge commit. Reproduced on pure origin/main with no part of this diff applied:

git checkout origin/main          # dbc39dad4
cd web && npx jest src/components/timeline/Inspector/__tests__/TimelineInspector.motionControls.test.tsx
  Tests: 4 failed, 10 passed, 14 total     MAIN_JEST_EXIT=1

Identical counts and identical assertion to the CI run, so the base branch is red here on its own. This PR touches no file under web/ — the diff is the publish workflow, packages/cli/src/nodetool.ts, two READMEs, scripts/, and engines/files fields in package.json.

The test asserts the inspector captions an unreadable easing ("wobble(3)") with "not an easing this build reads; plays linear". The component renders the Animate section without that caption, so either the caption was never wired up or its copy differs from the test's regex. Searching open PRs for a fix returns none, so there is nothing to port. The fix belongs in ClipAnimation/inspector code that this PR has no business touching — widening it to chase an unrelated web feature would be worse than leaving it. Flagging it here so the main breakage is visible and someone who owns that feature can decide whether the caption or the test is wrong.

2. Quality Gate (npm run check) / docker — Docker Hub 502

#3 resolve image config for docker-image://docker.io/docker/dockerfile:1-labs
#3 ERROR: unexpected status from HEAD request to
   https://registry-1.docker.io/v2/docker/dockerfile/manifests/1-labs: 502 Bad Gateway

The build never reached a Dockerfile step — it could not resolve the syntax frontend image — and the smoke container was never created (No such container: nodetool-smoke). This names a service the diff does not touch, and this PR changes no Dockerfile or container config. I tried to re-run it and got 403 This workflow is already running; the push above starts a fresh run, which serves as that single re-run. If it fails a second time I will treat it as real and root-cause it.


Generated by Claude Code

@georgi georgi left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the full diff. The idempotency logic is sound:

  • classifyPublishResult correctly distinguishes EPUBLISHCONFLICT (skip) from genuine auth/network errors (fail) by inspecting output after checking status === 0.
  • alreadyPublished pre-check via npm view avoids unnecessary publish attempts.
  • Post-publish smoke job installs from the public registry and asserts version string and key commands.
  • --provenance and --ignore-scripts preserved. Nightly tag exclusion correct.
  • CLI version now reads from package.json via createRequire instead of hardcoded "0.1.0".
  • Engine field widened to >=22.0.0 is reasonable.
  • Unit tests cover all classifyPublishResult branches.

No issues found. Ready to merge once CI is green.


Generated by Claude Code

@georgi
georgi merged commit 528e60a into main Sep 6, 2026
24 of 26 checks passed
@georgi
georgi deleted the claude/nodetool-cli-npm-setup-surm52 branch September 6, 2026 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants