diff --git a/.github/workflows/npm-publish.yml b/.github/workflows/npm-publish.yml index 67451f6523..bb1ff2a6a1 100644 --- a/.github/workflows/npm-publish.yml +++ b/.github/workflows/npm-publish.yml @@ -7,11 +7,27 @@ name: Publish npm packages # that never left the repo, so every `commonly` install and every seat on this # laptop ran code the repo had already replaced. # -# Trigger: a push to main that touches a published package. For each package, -# compare package.json's version with what the registry serves: -# equal → nothing to do (most pushes) -# repo > registry → publish, then read it back from the registry -# repo < registry → fail loudly; the repo walked backwards (see the guard) +# Since TASK-466 (2026-10-24) PRs never touch a package's version line. Every +# cli/mcp PR used to bump it, so concurrent PRs conflicted on that one line and +# the guard refused duplicate claims (13 open PRs claimed 11 cli versions, three +# of them 0.1.148). The version is now assigned HERE, at publish time: +# +# Trigger: a push to main that touches a published package. For each package: +# 1. pack this tree and compare it with the registry's latest tarball, +# ignoring package.json "version" (scripts/npm-artifact-diff.sh) +# identical → nothing to do +# 2. otherwise version = next patch above max(registry, repo package.json), +# stamped into the working copy only (npm version --no-git-tag-version; +# commonly-mcp's server.json too), never committed +# 3. publish, read it back, push tag cli-v / mcp-v on the commit +# that was built, then assert tag → this SHA and registry → this version. +# package-version-guard.yml re-checks each publish independently: the tag's +# commit, packed and stamped, must equal the tarball npm serves. One version, +# one commit (#979 / #1017). +# +# The repo's package.json version is therefore a floor, not the release. A +# `commonly --version` / the MCP handshake still report the stamped version, +# because both read the package.json that ships inside the tarball. # # Auth: npm trusted publishing (OIDC). No token is stored anywhere. Each package # on npmjs.com names this repo and this workflow file as its trusted publisher, @@ -45,7 +61,9 @@ on: permissions: contents: read - id-token: write # npm trusted publishing (OIDC) + provenance + +# Granted on the job, not here, so a job added to this file later starts +# read-only instead of inheriting tag-push rights. concurrency: group: npm-publish @@ -55,6 +73,9 @@ jobs: publish: name: ${{ matrix.pkg }} runs-on: ubuntu-latest + permissions: + contents: write # push the cli-v / mcp-v release tag + id-token: write # npm trusted publishing (OIDC) + provenance strategy: fail-fast: false matrix: @@ -69,6 +90,7 @@ jobs: env: PKG: ${{ matrix.pkg }} PICK: ${{ github.event.inputs.package || 'all' }} + TAG_PREFIX: ${{ matrix.pkg == 'cli' && 'cli-v' || 'mcp-v' }} steps: - uses: actions/checkout@v4 @@ -81,15 +103,14 @@ jobs: with: node-version: '22' - - name: Compare repo version with the registry + - name: Decide whether this tree holds anything unpublished id: cmp shell: bash run: | set -euo pipefail name=$(node -p "require('./package.json').name") repo_v=$(node -p "require('./package.json').version") - echo "name=$name" >> "$GITHUB_OUTPUT" - echo "repo_v=$repo_v" >> "$GITHUB_OUTPUT" + echo "name=$name" >> "$GITHUB_OUTPUT" if [ "$PICK" != "all" ] && [ "$PICK" != "$PKG" ]; then echo "· $name: not selected (package=$PICK)" @@ -97,6 +118,17 @@ jobs: exit 0 fi + # A dispatch can name any branch. Publishing or tagging from one + # would release code main does not hold. This catches a mistaken + # dispatch; it is not a security boundary, because a branch's own + # copy of this file can drop it. The boundary is the trusted-publisher + # binding plus who can dispatch; a protected main-only environment is + # the planned hardening. + if [ "$GITHUB_REF" != "refs/heads/main" ]; then + echo "::error::$name: this workflow publishes only from main (got $GITHUB_REF)" + exit 1 + fi + # "Never published" (E404) is the first-release path and maps to # 0.0.0. Anything else — outage, auth, rate limit — must stop the # run: a registry we cannot read is not a registry at 0.0.0. @@ -108,27 +140,75 @@ jobs: echo "::error::$name: npm view failed for a reason other than E404:"; cat /tmp/npm-view.err exit 1 fi - echo "reg_v=$reg_v" >> "$GITHUB_OUTPUT" - if [ "$repo_v" = "$reg_v" ]; then - echo "· $name: registry already at $reg_v" - echo "action=skip" >> "$GITHUB_OUTPUT" - exit 0 - fi - # Prerelease ordering is beyond `sort -V` (same limit the version - # guard documents). Checked AFTER equality so a repo and registry - # parked on the same prerelease is a no-op, not a failure. + # Prerelease ordering is beyond `sort -V` (same limit the guard + # documents), and "next patch" is undefined for one. case "$repo_v$reg_v" in *-*) - echo "::error::$name: prerelease in $reg_v → $repo_v; this workflow does not order prereleases. Publish by hand." + echo "::error::$name: prerelease in $reg_v / $repo_v; this workflow does not order prereleases. Publish by hand." exit 1;; esac - if [ "$(printf '%s\n%s\n' "$reg_v" "$repo_v" | sort -V | tail -1)" = "$repo_v" ]; then - echo "✓ $name: $reg_v → $repo_v, publishing" - echo "action=publish" >> "$GITHUB_OUTPUT" - else - echo "::error::$name: repo $repo_v is BELOW registry $reg_v. main walked the version backwards; fix the repo, do not republish." - exit 1 + + if [ "$reg_v" != "0.0.0" ]; then + set +e; bash "$GITHUB_WORKSPACE/scripts/npm-artifact-diff.sh" . "$reg_v"; d=$?; set -e + # The diff step ran prepublishOnly; put the tree back as checked out. + git checkout -- . && git clean -fdq -- . + case "$d" in + # Already published: nothing for npm, but the MCP Registry step + # still lists that version, so a failed listing can be retried. + # The version's release tag must exist too: a run whose publish + # landed and whose tag push failed is re-run into this branch. + 0) echo "version=$reg_v" >> "$GITHUB_OUTPUT" + set +e; git ls-remote --exit-code --tags origin "refs/tags/$TAG_PREFIX$reg_v" >/dev/null; t=$?; set -e + case "$t" in + 0) echo "action=skip" >> "$GITHUB_OUTPUT" ;; + # This commit packs exactly what npm serves for $reg_v, which is + # the invariant the tag asserts, so it may carry the missing + # tag. That holds for a version published before TASK-466 + # too, so the first run tags it and there is no bootstrap + # case where a missing tag is left missing. + 2) echo "· $name: $reg_v is published but $TAG_PREFIX$reg_v is missing; tagging $GITHUB_SHA, which packs the same files" + echo "action=retag" >> "$GITHUB_OUTPUT" ;; + *) echo "::error::$name: could not read tag $TAG_PREFIX$reg_v (git ls-remote exit $t)"; exit 1 ;; + esac + exit 0 ;; + 1) ;; + *) echo "::error::$name: could not compare with $reg_v; not publishing blind"; exit 1 ;; + esac + fi + + floor=$(printf '%s\n%s\n' "$reg_v" "$repo_v" | sort -V | tail -1) + next=$(echo "$floor" | awk -F. '{printf "%d.%d.%d", $1, $2, $3+1}') + # --exit-code: 0 found, 2 no such ref. Anything else is a lookup that + # failed, which proves nothing about the tag, so only 2 may publish. + set +e; git ls-remote --exit-code --tags origin "refs/tags/$TAG_PREFIX$next" >/dev/null; t=$?; set -e + case "$t" in + 2) ;; + 0) echo "::error::$name: tag $TAG_PREFIX$next already exists but npm serves $reg_v. A publish half-finished; reconcile by hand." + exit 1 ;; + *) echo "::error::$name: could not read tag $TAG_PREFIX$next (git ls-remote exit $t); not publishing blind" + exit 1 ;; + esac + echo "✓ $name: registry $reg_v, repo floor $repo_v → publishing $next" + echo "next=$next" >> "$GITHUB_OUTPUT" + echo "version=$next" >> "$GITHUB_OUTPUT" + echo "action=publish" >> "$GITHUB_OUTPUT" + + # The version exists only in this job's working copy and so in the + # tarball; nothing is committed. server.json goes with it, because the + # MCP Registry step below lists the version server.json names and checks + # it against package.json. It also runs when the tree is already + # published (version = what npm serves), so a re-run after a failed + # registry listing lists that version, not the repo's stale floor. + - name: Stamp the version into the working copy + if: steps.cmp.outputs.version != '' + env: + VERSION: ${{ steps.cmp.outputs.version }} + run: | + npm version "$VERSION" --no-git-tag-version --allow-same-version + if [ -f server.json ]; then + node -e "const f='server.json',j=require('./'+f);j.version=process.env.VERSION;(j.packages||[]).forEach(p=>p.version=process.env.VERSION);require('fs').writeFileSync(f,JSON.stringify(j,null,2)+'\n')" fi + node -p "'stamped ' + require('./package.json').version" # Trusted publishing needs npm >= 11.5.1; Node 22 ships npm 10. - name: Use an npm that supports trusted publishing @@ -171,9 +251,35 @@ jobs: shell: bash env: NAME: ${{ steps.cmp.outputs.name }} - WANT: ${{ steps.cmp.outputs.repo_v }} + WANT: ${{ steps.cmp.outputs.next }} run: bash "$GITHUB_WORKSPACE/scripts/verify-npm-publish-readback.sh" + # The tag is the record of which commit a version was built from. Pushed + # only after npm serves the version, so a tag never names a release that + # does not exist; a publish whose tag push failed is caught by the next + # run (it refuses to reuse the version) and by package-version-guard. + - name: Tag the commit that was built + if: steps.cmp.outputs.action == 'publish' || steps.cmp.outputs.action == 'retag' + env: + TAG: ${{ env.TAG_PREFIX }}${{ steps.cmp.outputs.version }} + run: | + git tag "$TAG" "$GITHUB_SHA" + git push origin "refs/tags/$TAG" + + - name: Assert tag → this commit and registry → this version + if: steps.cmp.outputs.action == 'publish' || steps.cmp.outputs.action == 'retag' + env: + NAME: ${{ steps.cmp.outputs.name }} + NEXT: ${{ steps.cmp.outputs.version }} + TAG: ${{ env.TAG_PREFIX }}${{ steps.cmp.outputs.version }} + run: | + set -euo pipefail + tag_sha=$(git ls-remote origin "refs/tags/$TAG" | cut -f1) + [ "$tag_sha" = "$GITHUB_SHA" ] || { echo "::error::$TAG points at '$tag_sha', not the built commit $GITHUB_SHA"; exit 1; } + got=$(npm view "$NAME@$NEXT" version) + [ "$got" = "$NEXT" ] || { echo "::error::npm serves '$got' for $NAME@$NEXT"; exit 1; } + echo "✓ $NAME $NEXT = $TAG = $GITHUB_SHA" + # The MCP Registry lists commonly-mcp from its server.json, after npm # serves the version. Runs whether or not this push published (it is a # no-op when the version is already listed), so a registry failure can be diff --git a/.github/workflows/package-version-guard.yml b/.github/workflows/package-version-guard.yml index fee115e173..5cf9ca7c97 100644 --- a/.github/workflows/package-version-guard.yml +++ b/.github/workflows/package-version-guard.yml @@ -1,8 +1,8 @@ name: Package Version Guard # A published package's version is the ONLY check available from outside this -# repo. When source ships without a version bump, that check silently passes -# while the artifact and the repo disagree — and nobody can tell. +# repo. When two different artifacts carry one version, that check silently +# passes while the artifact and the repo disagree — and nobody can tell. # # It has happened twice: # #979 @commonlyai/mcp npm 0.3.0 and main 0.3.0 were different code; the @@ -11,183 +11,123 @@ name: Package Version Guard # SEVEN source commits since the bump — including #995, the quota # misclassification that had seats probing a dead provider every 5s. # -# Both were found by hand, months and hours late respectively. This makes the -# third one go red instead. +# Until TASK-466 (2026-10-24) this guard made every PR that touched cli/src or +# commonly-mcp/src bump the version, and refused a version an older open PR had +# claimed. That kept the invariant, at a cost: every such PR edited the same +# line, so concurrent PRs conflicted on it (13 open PRs claimed 11 cli versions +# on 10-24, three of them 0.1.148; #2589 needed three bumps). +# +# Now npm-publish.yml assigns the version at publish time and tags the commit it +# built (cli-v / mcp-v). This guard checks that record after each +# publish, independently of the job that wrote it: the tagged commit, packed, +# must be file-for-file the tarball npm serves. That is the #979/#1017 +# guarantee — one version, one commit — checked on the artifact itself. +# +# The PR-side half is now advisory only: a PR that still edits a version line +# gets a warning, because that edit is what made PRs conflict, not because it +# can break a release (the publish takes max(registry, repo) + 1 either way). on: + workflow_run: + workflows: [ "Publish npm packages" ] + types: [ completed ] + branches: [ main ] + workflow_dispatch: pull_request: branches: [ main ] - # `edited` catches base-branch retargeting — see the note in - # pr-base-freshness.yml. A stacked PR retargeted to main after its parent - # merges enters this guard's population without any event in the list - # below ever firing for it. types: [opened, synchronize, reopened, ready_for_review, edited] - # Only PRs that can change this guard's answer run it. Every other PR - # paid a runner for a no-op, and on a rebase wave those no-ops queue - # ahead of the merge-group build (10-24: 165 runs queued, merge - # group waited 16+ min). Not a required check, so a skip never blocks. paths: - - 'cli/**' - - 'commonly-mcp/**' - - '.github/workflows/package-version-guard.yml' + - 'cli/package.json' + - 'commonly-mcp/package.json' + - 'commonly-mcp/server.json' permissions: contents: read - pull-requests: read concurrency: - group: ${{ github.workflow }}-${{ github.event.pull_request.number }} - cancel-in-progress: true + group: ${{ github.workflow }}-${{ github.event.pull_request.number || 'published' }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} jobs: - version-guard: - name: Source changed ⇒ version bumped + pr-leaves-version-alone: + name: PR leaves the version line alone + if: github.event_name == 'pull_request' runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 with: fetch-depth: 0 - - - name: Every published package whose source moved must bump its version + - name: Warn on a version edit shell: bash run: | set -euo pipefail BASE="origin/${{ github.event.pull_request.base.ref }}" - - # NO --depth here. `checkout` above already fetched full history - # (fetch-depth: 0); a --depth=1 fetch of the base UNDOES that by - # shallowing the ref, and once the base is one commit deep the fork - # point is unreachable. `git diff BASE...HEAD` then dies with - # "fatal: ...HEAD: no merge base" — which reads like a problem - # with the PR's diff and is entirely a problem with this fetch. - # - # It only fires once the base has advanced past the fork point, which - # is why the guard worked for weeks and then failed on #1113. Locally - # reproducible: full clone, advance base 12 commits past a feature - # branch, `git fetch --depth=1 origin `, and the very next - # three-dot diff fails. - # - # Also dropped the `|| true`. A base we cannot fetch means every - # comparison below is meaningless, so it must stop rather than - # silently compare against whatever ref happens to be on disk — the - # failure mode this whole guard exists to prevent. git fetch --no-tags origin "${{ github.event.pull_request.base.ref }}" - - # Prove the base is usable before trusting any diff against it. A - # guard that cannot see its own baseline must say so, not pass. - if ! git merge-base "$BASE" HEAD >/dev/null 2>&1; then - echo "::error::No merge base between $BASE and HEAD — the checkout is too shallow for this guard to compare anything. Not passing on an unusable baseline." - exit 1 - fi - - fail=0 - # pkg_dir : the published package root (holds package.json) - for pkg in cli commonly-mcp; do - # Did any SOURCE file move? Docs and tests do not require a release. - changed=$(git diff --name-only "$BASE"...HEAD -- "$pkg/src" | wc -l | tr -d ' ') - if [ "$changed" = "0" ]; then - echo "· $pkg: no src changes" - continue - fi - - base_v=$(git show "$BASE:$pkg/package.json" 2>/dev/null | sed -n 's/.*"version": "\([^"]*\)".*/\1/p' | head -1) - head_v=$(sed -n 's/.*"version": "\([^"]*\)".*/\1/p' "$pkg/package.json" | head -1) - - # `sort -V` is not semver. It orders 1.0.0 BEFORE 1.0.0-beta.1, - # reading a prerelease as NEWER than its own release, and it - # inverts in both directions: base=1.0.0 head=1.0.0-beta.1 would - # PASS — the exact backwards walk this check exists to stop — and - # the legitimate promotion 1.0.0-beta.1 → 1.0.0 would FAIL. - # No version ever committed to either package carries a - # prerelease, so this is latent today. It fails OPEN in the - # direction that matters, so the guard refuses to judge rather - # than guessing, on the same principle as the merge-base check - # above: a guard that cannot compare its inputs must say so. - case "$base_v$head_v" in - *-*) - echo "::error file=$pkg/package.json::$pkg version comparison involves a prerelease ($base_v → $head_v). \`sort -V\` orders a prerelease as NEWER than its release, so this guard reaches the wrong answer in BOTH directions. Teach it semver before landing a prerelease; do not merge on this check alone." - fail=1 - continue - ;; - esac - - # An INCREASE, not merely a difference. A long-lived branch that - # bumped while the base moved further ahead leaves head_v BELOW - # base_v — different, so an equality test passes it, and merging - # then walks the version backwards on the base. Observed on two - # open PRs at once (base 0.1.21, heads 0.1.20 and 0.1.19), both - # showing this check green. - newest=$(printf '%s\n%s\n' "$base_v" "$head_v" | sort -V | tail -1) - if [ "$base_v" = "$head_v" ]; then - echo "::error file=$pkg/package.json::$pkg/src changed ($changed file(s)) but version is still $head_v. A published version that maps to two different artifacts defeats the only check available from outside this repo — bump it, or move the change out of $pkg/src." - fail=1 - elif [ "$newest" != "$head_v" ]; then - echo "::error file=$pkg/package.json::$pkg/src changed ($changed file(s)) and $head_v is BELOW the base's $base_v. Merging would walk the published version backwards. Rebase and bump above $base_v." - fail=1 + for f in cli/package.json commonly-mcp/package.json commonly-mcp/server.json; do + if git diff "$BASE"...HEAD -- "$f" | grep -qE '^[+-][^+-].*"version":'; then + echo "::warning file=$f::This PR edits the version in $f. Since TASK-466 the version is assigned when npm-publish runs; revert this line so the PR stops conflicting with every other cli/mcp PR." else - echo "✓ $pkg: src changed and version rose $base_v → $head_v" + echo "· $f: version untouched" fi done - exit $fail - - # The check above compares this PR against the base. It cannot see a - # SECOND open PR bumping the same package to the same version, because - # neither branch contains the other's commit — and that pair merges - # CLEAN. Measured, not assumed: two branches both taking cli 0.1.30 to - # 0.1.31 while touching different files under cli/src merge with no - # conflict, and the result is one 0.1.31 holding both PRs' source. Each - # PR's guard was green the whole time. npm then has a version that maps - # to an artifact neither PR alone produced, which is the exact defect - # #979 and #1017 were. - # - # Older PR keeps the version, newer picks the next one — so it is always - # fixable by one author alone. Failing both would be a deadlock. - - name: No older open PR is taking this package to the same version - env: - GH_TOKEN: ${{ github.token }} - PR: ${{ github.event.pull_request.number }} - REPO: ${{ github.repository }} + published-matches-tag: + name: Published version = its tagged commit + if: github.event_name != 'pull_request' && (github.event_name == 'workflow_dispatch' || github.event.workflow_run.conclusion != 'cancelled') + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + include: + # untagged: the last version published before TASK-466, the only + # one allowed on npm without a tag (npm-publish's next run tags it). + - pkg: cli + prefix: cli-v + untagged: 0.1.155 + - pkg: commonly-mcp + prefix: mcp-v + untagged: 0.3.16 + steps: + # workflow_run runs with the default branch's context, so this checks out + # main (never a PR ref), and the only other commit it runs code from is a + # release tag proven to be on main first. Read-only token throughout. + - uses: actions/checkout@v4 + with: + ref: main + fetch-depth: 0 # the tagged commit must be reachable, and checked against main + fetch-tags: true + persist-credentials: false + - uses: actions/setup-node@v4 + with: + node-version: '22' + - name: Tagged commit packs to exactly what npm serves shell: bash + env: + PKG: ${{ matrix.pkg }} + PREFIX: ${{ matrix.prefix }} + UNTAGGED: ${{ matrix.untagged }} run: | set -euo pipefail - - # Every gh call is checked. An unchecked failure yields an empty - # version list, which reads as "this PR bumps nothing" and PASSES — - # the guard at its most reassuring exactly when it has gone blind. - # Same reason the `|| true` came off the fetch above. - proposed() { - local pr="$1" pkg="$2" patch - if ! patch=$(gh api "repos/$REPO/pulls/$pr/files" --paginate \ - --jq ".[] | select(.filename == \"$pkg/package.json\") | .patch"); then - echo "::error::gh api failed reading PR #$pr. Not passing on a check that could not run." >&2 - return 1 + name=$(node -p "require('./$PKG/package.json').name") + reg_v=$(npm view "$name" version) + tag="$PREFIX$reg_v" + + if ! git rev-parse -q --verify "refs/tags/$tag" >/dev/null; then + # Only the last version published before TASK-466 may be untagged. + # Any other missing tag is a publish whose tag push failed, even + # the first one (no tags at all is not evidence of an old version). + if [ "$reg_v" = "$UNTAGGED" ]; then + echo "· $name $reg_v predates release tags (TASK-466); npm-publish's next run tags it" + exit 0 fi - # Only an ADDED version line counts. A PR that merely carries an - # old package.json (every stale branch does) proposes nothing. - printf '%s\n' "$patch" | sed -n 's/^+.*"version": "\([^"]*\)".*/\1/p' | head -1 - } - - if ! older=$(gh api "repos/$REPO/pulls?state=open&per_page=100" --paginate \ - --jq ".[] | select(.number < $PR) | .number"); then - echo "::error::gh api failed listing open PRs. Not passing on a blind check." + echo "::error::$name $reg_v is on npm but tag $tag does not exist. Releases after TASK-466 are tagged; find the npm-publish run that published $reg_v." exit 1 fi - fail=0 - for pkg in cli commonly-mcp; do - mine=$(proposed "$PR" "$pkg") - [ -z "$mine" ] && { echo "· $pkg: this PR proposes no version"; continue; } - echo "· $pkg: this PR proposes $mine" - for other in $older; do - theirs=$(proposed "$other" "$pkg") - [ "$theirs" = "$mine" ] || continue - echo "::error file=$pkg/package.json::$pkg $mine is already claimed by the older open PR #$other. Both bumps merge clean — git sees the same line changed the same way — so npm would carry one $mine built from both PRs' source. #$other keeps $mine; pick the next version here." - fail=1 - done - done - - if [ "$fail" = "0" ]; then - echo "✓ no older open PR claims these versions" - fi - exit $fail + sha=$(git rev-parse "refs/tags/$tag^{commit}") + git merge-base --is-ancestor "$sha" origin/main \ + || { echo "::error::$tag points at $sha, which is not on main"; exit 1; } + git checkout -q --detach "$sha" + bash scripts/npm-artifact-diff.sh "$PKG" "$reg_v" \ + || { echo "::error::$name $reg_v does not match the commit its tag names ($tag → $sha)"; exit 1; } + echo "✓ $name $reg_v = $tag = $sha" diff --git a/backend/__tests__/unit/scripts/npmArtifactDiff.test.js b/backend/__tests__/unit/scripts/npmArtifactDiff.test.js new file mode 100644 index 0000000000..29a895e6d4 --- /dev/null +++ b/backend/__tests__/unit/scripts/npmArtifactDiff.test.js @@ -0,0 +1,165 @@ +/** + * Guards scripts/npm-artifact-diff.sh, which decides whether main holds + * anything npm does not (TASK-466). npm-publish.yml reads exit 1 as permission + * to publish, so exit 1 must mean ONLY "compared, and the files differ". Sable's + * HOLD on #2610 reproduced the opposite: a failing prepublishOnly and a diff + * I/O error both came out as 1, and the decision step published on a + * comparison that never finished. Every failure before the verdict is a 2. + * + * `npm`, `diff` and `mktemp` are stubs on PATH, driven by env, so each case + * states the condition rather than mocking the script. `tar` and `node` are real. + */ + +const { spawnSync } = require('child_process'); +const fs = require('fs'); +const os = require('os'); +const path = require('path'); + +const REPO_ROOT = path.resolve(__dirname, '..', '..', '..', '..'); +const SCRIPT = path.join(REPO_ROOT, 'scripts', 'npm-artifact-diff.sh'); +const WORKFLOW = path.join(REPO_ROOT, '.github', 'workflows', 'npm-publish.yml'); +const GUARD = path.join(REPO_ROOT, '.github', 'workflows', 'package-version-guard.yml'); +const REAL_MKTEMP = fs.existsSync('/usr/bin/mktemp') ? '/usr/bin/mktemp' : 'mktemp'; +const REAL_DIFF = fs.existsSync('/usr/bin/diff') ? '/usr/bin/diff' : 'diff'; + +// `npm pack` from the package dir packs it; `npm pack @` "fetches" the +// registry fixture. Either can be made to fail, as can `npm run`. +const npmStub = [ + '#!/usr/bin/env bash', + 'if [ "$1" = run ]; then [ -n "${STUB_PREPUBLISH_FAIL:-}" ] && exit 1; exit 0; fi', + 'if [ "$1" != pack ]; then echo "stub: unexpected npm $*" >&2; exit 1; fi', + 'dest=""; spec=""; shift', + 'while [ $# -gt 0 ]; do case "$1" in --pack-destination) dest="$2"; shift 2;; --silent) shift;; *) spec="$1"; shift;; esac; done', + 'if [ -z "$spec" ]; then [ -n "${STUB_PACK_FAIL:-}" ] && exit 1; src="$PWD"', + 'else [ -n "${STUB_FETCH_FAIL:-}" ] && exit 1; src="$STUB_REGISTRY_DIR"; fi', + 't=$(mktemp -d); mkdir "$t/package"; cp -R "$src"/. "$t/package/"', + 'tar -czf "$dest/pkg.tgz" -C "$t" package', +].join('\n'); + +const diffStub = [ + '#!/usr/bin/env bash', + '[ -n "${STUB_DIFF_EXIT:-}" ] && exit "$STUB_DIFF_EXIT"', + `exec ${REAL_DIFF} "$@"`, +].join('\n'); + +// STUB_DIFF_OUT_BLOCKED makes diff.txt unopenable (a directory) before the +// script writes it, so its redirect fails without diff ever running. +const mktempStub = [ + '#!/usr/bin/env bash', + `d=$(${REAL_MKTEMP} "$@") || exit $?`, + '[ -n "${STUB_DIFF_OUT_BLOCKED:-}" ] && mkdir "$d/diff.txt"', + 'echo "$d"', +].join('\n'); + +function setup({ localVersion = '0.1.4', registryVersion = '0.1.3', registryText = 'same\n' } = {}) { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'npm-artifact-diff-')); + const bin = path.join(root, 'bin'); + const pkg = path.join(root, 'pkg'); + const reg = path.join(root, 'reg'); + for (const d of [bin, pkg, reg]) fs.mkdirSync(d); + fs.writeFileSync(path.join(bin, 'npm'), npmStub, { mode: 0o755 }); + fs.writeFileSync(path.join(bin, 'diff'), diffStub, { mode: 0o755 }); + fs.writeFileSync(path.join(bin, 'mktemp'), mktempStub, { mode: 0o755 }); + const manifest = (version) => JSON.stringify({ name: '@x/fixture', version }, null, 2); + fs.writeFileSync(path.join(pkg, 'package.json'), manifest(localVersion)); + fs.writeFileSync(path.join(pkg, 'index.js'), 'same\n'); + fs.writeFileSync(path.join(reg, 'package.json'), manifest(registryVersion)); + fs.writeFileSync(path.join(reg, 'index.js'), registryText); + return { root, bin, pkg, reg }; +} + +function run(fx, env = {}) { + const r = spawnSync('bash', [SCRIPT, fx.pkg, '0.1.3'], { + encoding: 'utf8', + env: { ...process.env, PATH: `${fx.bin}:${process.env.PATH}`, STUB_REGISTRY_DIR: fx.reg, ...env }, + }); + fs.rmSync(fx.root, { recursive: true, force: true }); + return r; +} + +describe('npm-artifact-diff.sh', () => { + test('identical files apart from the version exit 0', () => { + expect(run(setup()).status).toBe(0); + }); + + test('a real difference exits 1 and names the file', () => { + const r = run(setup({ registryText: 'other\n' })); + expect(r.status).toBe(1); + expect(r.stdout).toMatch(/index\.js/); + }); + + test.each([ + ['prepublishOnly fails', { STUB_PREPUBLISH_FAIL: '1' }], + ['local npm pack fails', { STUB_PACK_FAIL: '1' }], + ['the registry tarball cannot be fetched', { STUB_FETCH_FAIL: '1' }], + ['diff hits an I/O error (exit 2)', { STUB_DIFF_EXIT: '2' }], + ['diff exits with anything else', { STUB_DIFF_EXIT: '3' }], + // Sable, #2610 at bc515eea: the redirect failing returns 1 without diff. + ['the diff output cannot be opened', { STUB_DIFF_OUT_BLOCKED: '1' }], + ])('%s: exit 2, never 1', (_label, env) => { + expect(run(setup(), env).status).toBe(2); + }); + + test('missing arguments are a 2, not a difference', () => { + const r = spawnSync('bash', [SCRIPT], { encoding: 'utf8' }); + expect(r.status).toBe(2); + }); +}); + +describe('npm-publish.yml decision step', () => { + const body = () => fs.readFileSync(WORKFLOW, 'utf8'); + + test('only a completed difference (1) may continue to publish', () => { + const step = body(); + expect(step).toMatch(/0\) echo "version=\$reg_v"/); + expect(step).toMatch(/\n\s+1\) ;;\n/); + expect(step).toMatch(/\*\) echo "::error::\$name: could not compare/); + }); + + test('only a confirmed-absent tag (ls-remote exit 2) may publish', () => { + const step = body(); + expect(step).toMatch(/git ls-remote --exit-code --tags origin "refs\/tags\/\$TAG_PREFIX\$next" >\/dev\/null; t=\$\?/); + expect(step).toMatch(/case "\$t" in\n\s+2\) ;;/); + expect(step).toMatch(/could not read tag/); + }); + + test('the stamp step also runs on the already-published path, for the registry retry', () => { + const step = body(); + expect(step).toContain("if: steps.cmp.outputs.version != ''"); + expect(step).toContain('VERSION: ${{ steps.cmp.outputs.version }}'); + }); + + test('an already-published version missing its tag is retagged, not skipped', () => { + const step = body(); + expect(step).toMatch(/git ls-remote --exit-code --tags origin "refs\/tags\/\$TAG_PREFIX\$reg_v" >\/dev\/null; t=\$\?/); + expect(step).toContain('echo "action=retag" >> "$GITHUB_OUTPUT"'); + expect(step).toMatch(/could not read tag \$TAG_PREFIX\$reg_v/); + // The tag and assert steps run on a retag and name the published version. + expect(step).toContain("if: steps.cmp.outputs.action == 'publish' || steps.cmp.outputs.action == 'retag'"); + expect(step).toContain('TAG: ${{ env.TAG_PREFIX }}${{ steps.cmp.outputs.version }}'); + }); + + test('a missing tag is retagged even when no tag with that prefix exists yet', () => { + // Sable, #2610 at bc515eea: "no tags at all" is also the state after the + // first publish whose tag push failed, so it must not mean "skip". + const step = body(); + expect(step).toMatch(/case "\$t" in\n\s+0\) echo "action=skip" >> "\$GITHUB_OUTPUT" ;;\n(\s+#.*\n)*\s+2\) echo "· \$name: \$reg_v is published but/); + expect(step).not.toContain('git ls-remote --tags origin "refs/tags/$TAG_PREFIX*"'); + }); + + test('publishes and tags only from main', () => { + expect(body()).toContain('if [ "$GITHUB_REF" != "refs/heads/main" ]; then'); + }); +}); + +describe('package-version-guard.yml', () => { + const body = () => fs.readFileSync(GUARD, 'utf8'); + + test('only the pinned last pre-TASK-466 version may be untagged', () => { + const guard = body(); + expect(guard).toContain('untagged: 0.1.155'); + expect(guard).toContain('untagged: 0.3.16'); + expect(guard).toContain('if [ "$reg_v" = "$UNTAGGED" ]; then'); + expect(guard).not.toContain('git tag --list "$PREFIX*"'); + }); +}); diff --git a/backend/__tests__/unit/scripts/npmPublishReadback.test.js b/backend/__tests__/unit/scripts/npmPublishReadback.test.js index fdcb10dc48..fbe7adcb32 100644 --- a/backend/__tests__/unit/scripts/npmPublishReadback.test.js +++ b/backend/__tests__/unit/scripts/npmPublishReadback.test.js @@ -661,9 +661,10 @@ describe('the workflow calls that script under the publish gate', () => { expect(step).toContain('scripts/verify-npm-publish-readback.sh'); expect(step).toContain("if: steps.cmp.outputs.action == 'publish'"); - // The values the script polls for come from the compare step, not literals. + // The values the script polls for come from the compare step, not literals; + // WANT is the version that step assigned (TASK-466), not the repo floor. expect(step).toContain('NAME: ${{ steps.cmp.outputs.name }}'); - expect(step).toContain('WANT: ${{ steps.cmp.outputs.repo_v }}'); + expect(step).toContain('WANT: ${{ steps.cmp.outputs.next }}'); // The interpreter is named: the file's exec bit does not survive every // checkout or archive path, and a bare path would fail there. expect(step).toMatch(/run: bash "\$GITHUB_WORKSPACE/); diff --git a/docs/development/review-checklist.md b/docs/development/review-checklist.md index a647da9f74..dc1dc2b77e 100644 --- a/docs/development/review-checklist.md +++ b/docs/development/review-checklist.md @@ -115,7 +115,7 @@ And then note where the fix landed. Someone hit the audience gap, felt it, and b ## Published packages and the version guard -35. **A change under `cli/src` or `commonly-mcp/src` must bump that package's `package.json` version, and "bumped" means ROSE ABOVE the base's — not merely differs from the branch it started on.** A published version number is the only check available from outside this repo, so when source ships without one the repo and the artifact disagree and nothing anywhere says so: the two times it happened, `@commonlyai/mcp` 0.3.0 and `@commonlyai/cli` 0.1.9 each named two different artifacts, the CLI case with seven source commits behind one number, and both were found by hand. `package-version-guard.yml` makes the third go red, and it fails a PR in three distinct ways a reviewer should be able to predict before CI speaks: (a) **no bump** — src moved, version equals base; (b) **a bump below base** — a long-lived branch that bumped while main moved further ahead, which a bare equality test passes and which walks the published version *backwards* on merge (observed on two open PRs at once, both green); (c) **a version an older open PR has already claimed for the same package** — two branches both taking `cli` 0.1.30 → 0.1.31 merge with no conflict and npm ends up with one 0.1.31 built from both PRs' source, so the older PR keeps the number and the newer picks the next; failing both would be a deadlock. What it deliberately does **not** require: a bump for tests, docs, or anything outside `$pkg/src` — a backend-test-only change needs none, and the guard says so by name (`· cli: no src changes`). Two operator facts worth knowing before reading a red run: an input pair involving a **prerelease** makes the guard REFUSE rather than guess (`sort -V` orders `1.0.0` before `1.0.0-beta.1`, inverting in both directions — so the promotion `0.1.0-beta.1` → `0.1.0` would be rejected), and every `gh` call in its second check step — `No older open PR is taking this package to the same version`; the workflow has a single job, `Source changed ⇒ version bumped` — is fatal on failure on purpose, because an unchecked failure yields an empty version list that reads as "this PR bumps nothing" and passes. *(Earned: 2026-09-25, #1876 — a fix to `commonly-mcp/src/tools.js` passed its suites and went red on this guard alone; the fix was one commit raising 0.3.12 → 0.3.13, and the guard's message named the package, the count of changed src files and the version it was still on (`commonly-mcp/src changed (1 file(s)) but version is still 0.3.12`), annotated on `commonly-mcp/package.json` rather than on the source file that moved — a no-bump failure has only one version to name. The reviewer's rule: a red version guard is a one-line author fix, not a design question — and a PR that touches a published package's source without one is not "waiting for the guard", it is incomplete.)* +35. **A change under `cli/src` or `commonly-mcp/src` must bump that package's `package.json` version, and "bumped" means ROSE ABOVE the base's — not merely differs from the branch it started on.** A published version number is the only check available from outside this repo, so when source ships without one the repo and the artifact disagree and nothing anywhere says so: the two times it happened, `@commonlyai/mcp` 0.3.0 and `@commonlyai/cli` 0.1.9 each named two different artifacts, the CLI case with seven source commits behind one number, and both were found by hand. `package-version-guard.yml` makes the third go red, and it fails a PR in three distinct ways a reviewer should be able to predict before CI speaks: (a) **no bump** — src moved, version equals base; (b) **a bump below base** — a long-lived branch that bumped while main moved further ahead, which a bare equality test passes and which walks the published version *backwards* on merge (observed on two open PRs at once, both green); (c) **a version an older open PR has already claimed for the same package** — two branches both taking `cli` 0.1.30 → 0.1.31 merge with no conflict and npm ends up with one 0.1.31 built from both PRs' source, so the older PR keeps the number and the newer picks the next; failing both would be a deadlock. What it deliberately does **not** require: a bump for tests, docs, or anything outside `$pkg/src` — a backend-test-only change needs none, and the guard says so by name (`· cli: no src changes`). Two operator facts worth knowing before reading a red run: an input pair involving a **prerelease** makes the guard REFUSE rather than guess (`sort -V` orders `1.0.0` before `1.0.0-beta.1`, inverting in both directions — so the promotion `0.1.0-beta.1` → `0.1.0` would be rejected), and every `gh` call in its second check step — `No older open PR is taking this package to the same version`; the workflow has a single job, `Source changed ⇒ version bumped` — is fatal on failure on purpose, because an unchecked failure yields an empty version list that reads as "this PR bumps nothing" and passes. *(Earned: 2026-09-25, #1876 — a fix to `commonly-mcp/src/tools.js` passed its suites and went red on this guard alone; the fix was one commit raising 0.3.12 → 0.3.13, and the guard's message named the package, the count of changed src files and the version it was still on (`commonly-mcp/src changed (1 file(s)) but version is still 0.3.12`), annotated on `commonly-mcp/package.json` rather than on the source file that moved — a no-bump failure has only one version to name. The reviewer's rule: a red version guard is a one-line author fix, not a design question — and a PR that touches a published package's source without one is not "waiting for the guard", it is incomplete.)* **Superseded 2026-10-24 by rule 57 (TASK-466): PRs no longer bump versions; the PR-side guard described here is gone.** ## Acceptance instruments @@ -242,3 +242,7 @@ And then note where the fix landed. Someone hit the audience gap, felt it, and b ## What a scan covered 56. **A scan that reports nothing found has proved nothing until it reports what it covered, and a count of zero is a failure, not a pass.** "No leaks found", "0 findings" and a green tick read the same whether the scanner read 2,581 commits or none, so the result a reviewer sees cannot tell a clean input from an empty one. Read the coverage line (commits scanned, files checked, rows matched) before the result. When you own the check, make an empty input fail it. The usual shape is a scan that works on one trigger and not another: the trigger you watch is real, and it vouches for the one you don't. *(Earned: #2389, 2026-10-21. Secret Scan passed every pull request from its first day, #84 on 2026-04-03, with "0 commits scanned … no leaks found". Its `--remotes=origin/main` is a git glob for `origin/main/*` and matched nothing, and a pull request checks out a detached merge commit with no local branch for `--branches` to find. Pushes to main scanned through the checked-out branch, so main's scan was real and nobody read the pull-request one. #2213's fake-JWT fixture passed its pull request and then kept main red for five days. A one-time scan of every branch and pull-request head, 8,883 commits in #2390, found no real secret. The workflow now names its commits and fails before scanning when there are none.)* + +## Published packages, after TASK-466 + +57. **Do not edit the version line of `cli/package.json`, `commonly-mcp/package.json` or `commonly-mcp/server.json` — the version is assigned at publish time, and a PR that bumps it is the PR that conflicts.** Since TASK-466 (2026-10-24), `npm-publish.yml` packs `main` after each merge touching a published package, compares it file-for-file with the tarball npm serves (ignoring the version field, `scripts/npm-artifact-diff.sh`), and when they differ publishes `max(registry, repo) + 1` stamped into the tarball only, then tags the built commit `cli-v` / `mcp-v`. The repo's version is a floor, never the release; `commonly --version` and the MCP handshake still report the stamped version because both read the `package.json` inside the tarball. The invariant the old PR-side guard protected is unchanged — **one version, one commit**: `@commonlyai/mcp` 0.3.0 and `@commonlyai/cli` 0.1.9 each once named two different artifacts (#979, #1017), and `package-version-guard.yml` now checks it where it can actually fail, after every publish: the tagged commit, packed, must equal what npm serves, and once any tag exists an untagged registry version is red. What a reviewer checks instead: nothing about versions on the PR (the guard only *warns* if a version line moved), and on a red `Published version = its tagged commit` run, which publish produced the mismatch — it is a release-path defect, not an author fix. A consequence worth knowing: a merge that changes any shipped file (README included, not just `src`) now publishes, because the artifact moved. *(Earned twice. First 2026-09-25, #1876: under the PR-side rule a one-file fix went red on the guard alone. Then 2026-10-24: 13 open PRs claimed 11 cli versions, three of them 0.1.148, every cli/mcp PR conflicted on the same line, and #2589 needed three bumps before it could merge. The PR-side rule had turned a release invariant into a merge-ordering problem; moving the assignment to the publish job keeps the invariant and removes the conflict.)* diff --git a/scripts/npm-artifact-diff.sh b/scripts/npm-artifact-diff.sh new file mode 100755 index 0000000000..ed63e64809 --- /dev/null +++ b/scripts/npm-artifact-diff.sh @@ -0,0 +1,54 @@ +#!/usr/bin/env bash +# Does the tarball this tree would publish differ from one already on npm? +# +# scripts/npm-artifact-diff.sh +# +# Exit 0: identical apart from package.json's "version". Exit 1: they differ +# (the differing paths are printed). Exit 2: it could not compare. The version +# field is set aside because, since TASK-466, the repo's package.json version is +# only a floor and npm-publish.yml stamps the real one into the tarball. +# +# Used twice: by npm-publish.yml to decide whether main holds anything +# unpublished, and by package-version-guard.yml to prove a published version +# was built from the commit its tag names (#979 / #1017: one version, two +# artifacts). +set -uo pipefail + +# Exit 1 means ONLY "compared, and the files differ": the publish job reads it +# as permission to publish. Every step that fails before the comparison is +# finished is a 2, so a broken compare can never pass for a difference. +cannot() { echo "::error::npm-artifact-diff: $*"; exit 2; } + +dir=${1:-}; want=${2:-} +[ -n "$dir" ] && [ -n "$want" ] || cannot "usage: npm-artifact-diff.sh " +cd "$dir" || cannot "no such package dir: $dir" +name=$(node -p "require('./package.json').name") || cannot "cannot read $dir/package.json" +work=$(mktemp -d) || cannot "mktemp failed" +trap 'rm -rf -- "${work:?}"' EXIT +mkdir -p "$work/local" "$work/registry" "$work/tgz-local" "$work/tgz-registry" || cannot "cannot create work dirs" + +# The cli copies its skill in at prepublishOnly, which `npm pack` does not run. +npm run --silent --if-present prepublishOnly >/dev/null || cannot "prepublishOnly failed in $dir" +npm pack --silent --pack-destination "$work/tgz-local" >/dev/null || cannot "npm pack failed in $dir" +( cd "$work" && npm pack --silent "$name@$want" --pack-destination "$work/tgz-registry" >/dev/null ) \ + || cannot "could not fetch $name@$want from npm" +tar -xzf "$work"/tgz-local/*.tgz -C "$work/local" || cannot "cannot extract the local tarball" +tar -xzf "$work"/tgz-registry/*.tgz -C "$work/registry" || cannot "cannot extract the registry tarball" +for side in local registry; do + node -e "const f=process.argv[1],j=require(f);j.version='0.0.0';require('fs').writeFileSync(f,JSON.stringify(j,null,2))" "$work/$side/package/package.json" \ + || cannot "cannot normalize the $side package.json" +done +# diff: 0 same, 1 different, anything else trouble (2 = I/O error). Its status +# is taken inside the group, so a redirect that cannot open diff.txt (which +# also returns 1, without running diff) is caught here and never read as +# "different". +rc= +{ diff -rq "$work/local/package" "$work/registry/package"; rc=$?; } >"$work/diff.txt" \ + || cannot "cannot write the diff output" +case "$rc" in + 0) echo "· $name: this tree packs the same files as $want"; exit 0 ;; + 1) echo "· $name: this tree differs from $want:" + sed 's|'"$work"'/||g' "$work/diff.txt" + exit 1 ;; + *) cannot "diff failed (exit $rc) comparing with $want" ;; +esac