From 600695d860d6c7bac478eb49e12c9ae4e66d997b Mon Sep 17 00:00:00 2001 From: Qwynn Marcelle Date: Sun, 26 Jul 2026 22:34:13 -0400 Subject: [PATCH] fix(cli): exit 0 from --help and --version; repair the post-publish check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 0.5.0 published correctly — provenance and all — but the release run went red at the final step, and chasing that surfaced two real defects plus one false claim of mine. **`--help` and `--version` exited 1.** The CLI uses commander's `.exitOverride()`, which throws instead of exiting, including on its success paths: both raise a CommanderError carrying `exitCode: 0`. The handler read that with `Number(error.exitCode) || 1`, and `0 || 1` is `1`. The output was always right; only the status was wrong, so it reported failure to every shell, script and CI job that checked it. Inherited from `agents-audit`, where the same expression sits and — that package being frozen at 0.4.4 — will remain. Regression tests assert the exit code for `--help`, `--version` and the `version` subcommand, plus a guard that an unknown option still exits non-zero. Watched red against the pre-fix source: 2 failed, "expected 1 to be +0", exactly the two success paths. **The post-publish check could never pass.** It ran ` --help`, so the bug above meant a non-zero result no matter how healthy the release. I previously reported this verifier as passing end-to-end for agents-audit, including in PR #14's checklist. It did not. I read `${PIPESTATUS[0]}` after piping through `tail`, which reports tail's status, and took visible help output as success without checking for the `Verified registry install...` line. It now runs `version` — a normal action returning 0, and a better check besides: it executes the installed entry point and prints the version the artifact actually carries, rather than text commander emits before any package code runs. Confirmed exit 0 against the live 0.5.0. **The propagation window was too short.** Measured on 0.5.0: the version endpoint answered at once, but the aggregated packument — what `npm install` resolves against — took ~4m30s. The old window summed to 75s. Now linear backoff with a 60s ceiling, ~6m30s total. A first publish is the slow case; there is no existing packument to update. Cut as 0.5.1 through the changeset flow, which this exercises for the first time: changeset added, `pnpm changeset version` bumped the manifest and wrote the changelog together. Also dated 0.5.0, which was still labelled Unreleased. Verified: architecture guard, typecheck, 108 tests, release:verify-packs on both tarballs, and verify-published.mjs exiting 0 against the published 0.5.0. --- .changeset/adopt-agents-audit-version-line.md | 11 ---- packages/cli/CHANGELOG.md | 33 +++++++++--- packages/cli/package.json | 2 +- packages/cli/src/cli.exit-code.test.ts | 50 +++++++++++++++++++ packages/cli/src/cli.ts | 12 ++++- scripts/verify-published.mjs | 31 ++++++++++-- 6 files changed, 114 insertions(+), 25 deletions(-) delete mode 100644 .changeset/adopt-agents-audit-version-line.md create mode 100644 packages/cli/src/cli.exit-code.test.ts diff --git a/.changeset/adopt-agents-audit-version-line.md b/.changeset/adopt-agents-audit-version-line.md deleted file mode 100644 index 2d8647b..0000000 --- a/.changeset/adopt-agents-audit-version-line.md +++ /dev/null @@ -1,11 +0,0 @@ ---- ---- - -`@workspacejson/cli` adopts `agents-audit`'s version line at `0.5.0`, set -directly in `packages/cli/package.json` rather than derived from a changeset — -changesets bumps relative to the current version and cannot jump `0.1.0` to -`0.5.0`. This changeset is intentionally empty so `changeset status` stays clean -for a release whose number was chosen, not computed. - -From `0.5.0` onward the normal flow applies: add a changeset, run -`pnpm changeset version`, tag `cli-vX.Y.Z` to match. diff --git a/packages/cli/CHANGELOG.md b/packages/cli/CHANGELOG.md index 63f4053..d611f9c 100644 --- a/packages/cli/CHANGELOG.md +++ b/packages/cli/CHANGELOG.md @@ -1,6 +1,25 @@ # Changelog — `@workspacejson/cli` -## [0.5.0] - Unreleased +## 0.5.1 + +### Patch Changes + +- **Fixed:** `workspacejson --help` and `workspacejson --version` exited `1` + instead of `0`. + + The CLI uses commander's `.exitOverride()`, which throws rather than exiting — + including on the success paths, where `--help` and `--version` raise a + `CommanderError` carrying `exitCode: 0`. The handler read that with + `Number(error.exitCode) || 1`, and `0 || 1` is `1`, so both commands reported + failure to every shell, script and CI job that checked their status. The output + was always correct; only the exit code was wrong. + + Inherited from `agents-audit`, where the same expression is present and, since + that package is frozen at `0.4.4`, will remain. Regression tests assert the exit + code for `--help`, `--version` and the `version` subcommand, plus a guard that an + unknown option still exits non-zero — verified red against the pre-fix source. + +## [0.5.0] - 2026-07-27 First release under the `workspacejson` name, and the continuation of `agents-audit`'s version line. That package is frozen at `0.4.4` — locked for @@ -8,7 +27,7 @@ hackathon judging — and all forward development happens here, so the numbering carries over rather than restarting. **Why `0.5.0` and not `0.4.5`.** Measured against `agents-audit@0.4.4`, this -binary *removes* surface: there is no `scan` command, no config file (see +binary _removes_ surface: there is no `scan` command, no config file (see Notes), and the binary itself is renamed from `agents-audit` to `workspacejson`. A patch bump would promise a drop-in successor, and the first thing a migrating user would hit is a missing command. Under 0.x, breaking changes go in the minor @@ -16,11 +35,11 @@ slot. Migrating from `agents-audit@0.4.4`: -| Before | After | -| -- | -- | -| `npx agents-audit generate` | `npx workspacejson generate` | -| `npx agents-audit scan` | no equivalent — the producer does not audit | -| `.agentsauditrc` | not read (see Notes) | +| Before | After | +| --------------------------- | ------------------------------------------- | +| `npx agents-audit generate` | `npx workspacejson generate` | +| `npx agents-audit scan` | no equivalent — the producer does not audit | +| `.agentsauditrc` | not read (see Notes) | ### Changed diff --git a/packages/cli/package.json b/packages/cli/package.json index cd2164f..903247c 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -1,6 +1,6 @@ { "name": "@workspacejson/cli", - "version": "0.5.0", + "version": "0.5.1", "description": "The workspace.json producer — scans a repository and generates .agents/workspace.json deterministically, preserving human-authored manual evidence.", "license": "Apache-2.0", "author": "workspace.json contributors", diff --git a/packages/cli/src/cli.exit-code.test.ts b/packages/cli/src/cli.exit-code.test.ts new file mode 100644 index 0000000..5670e1d --- /dev/null +++ b/packages/cli/src/cli.exit-code.test.ts @@ -0,0 +1,50 @@ +import { describe, expect, it, vi } from 'vitest'; +import { runCli } from './cli.js'; + +// Regression cover for the 0.5.0 defect: `.exitOverride()` makes commander throw +// on its *success* paths too — `--help` and `--version` raise a CommanderError +// carrying `exitCode: 0`. The handler read that with `Number(...) || 1`, so the +// legitimate zero became 1 and `workspacejson --help` reported failure to every +// shell and CI job that checked it. These assert the exit code, not the output, +// because the output was always correct — only the status was wrong. +const ARGV = (...args: string[]) => ['node', 'workspacejson', ...args]; + +describe('runCli exit codes', () => { + it('exits 0 for --help', async () => { + const stdout = vi.spyOn(process.stdout, 'write').mockReturnValue(true); + try { + await expect(runCli(ARGV('--help'))).resolves.toBe(0); + } finally { + stdout.mockRestore(); + } + }); + + it('exits 0 for --version', async () => { + const stdout = vi.spyOn(process.stdout, 'write').mockReturnValue(true); + try { + await expect(runCli(ARGV('--version'))).resolves.toBe(0); + } finally { + stdout.mockRestore(); + } + }); + + it('exits 0 for the version subcommand', async () => { + const log = vi.spyOn(console, 'log').mockImplementation(() => undefined); + try { + await expect(runCli(ARGV('version'))).resolves.toBe(0); + } finally { + log.mockRestore(); + } + }); + + // The fix must not turn every failure into a success: an unparseable option + // still has to report non-zero. + it('exits non-zero for an unknown option', async () => { + const stderr = vi.spyOn(process.stderr, 'write').mockReturnValue(true); + try { + await expect(runCli(ARGV('--no-such-flag'))).resolves.not.toBe(0); + } finally { + stderr.mockRestore(); + } + }); +}); diff --git a/packages/cli/src/cli.ts b/packages/cli/src/cli.ts index f455988..ea8e638 100644 --- a/packages/cli/src/cli.ts +++ b/packages/cli/src/cli.ts @@ -49,7 +49,17 @@ export async function runCli(argv: string[] = process.argv): Promise { try { await program.parseAsync(argv); } catch (error) { - exitCode = typeof error === 'object' && error && 'exitCode' in error ? Number((error as { exitCode?: number }).exitCode) || 1 : 1; + // `.exitOverride()` makes commander throw instead of exiting, including on + // the success paths: `--help` and `--version` raise a CommanderError whose + // exitCode is 0. The previous `Number(...) || 1` collapsed that legitimate + // zero into 1, so `workspacejson --help` reported failure to every shell and + // CI job that checked it. Distinguish "no usable exitCode" from "exitCode is + // zero" instead of leaning on falsiness. + const reported = + typeof error === 'object' && error !== null && 'exitCode' in error + ? Number((error as { exitCode?: number }).exitCode) + : Number.NaN; + exitCode = Number.isInteger(reported) ? reported : 1; } return exitCode; diff --git a/scripts/verify-published.mjs b/scripts/verify-published.mjs index fa359b0..7db408e 100644 --- a/scripts/verify-published.mjs +++ b/scripts/verify-published.mjs @@ -17,14 +17,25 @@ import { spawnSync } from "node:child_process"; // `cli-v*` release must not be reported green because a previously published // sibling still installs. With no argument it verifies every publishable // package, which is only meaningful when their versions genuinely agree. +// The health check runs `version`, not `--help`. Both CLIs use commander's +// `.exitOverride()`, which throws a CommanderError with exitCode 0 on the +// `--help` success path; until the fix landing alongside this, that zero was +// coerced to 1, so `--help` reported failure and this verifier could never pass +// for either package. `version` is a normal action that returns 0, and it proves +// more anyway — it executes the installed entry point and prints the version the +// artifact actually carries, rather than text commander produces before any of +// the package's own code runs. +// +// `agents-audit` is frozen at 0.4.4, so its `--help` keeps the old exit code +// permanently. That alone makes `version` the only check that can work for both. const RELEASABLE = { "@workspacejson/cli": { manifest: "../packages/cli/package.json", - check: ["npx", "--no-install", "workspacejson", "--help"], + check: ["npx", "--no-install", "workspacejson", "version"], }, "agents-audit": { manifest: "../packages/agents-audit-compat/package.json", - check: ["npx", "--no-install", "agents-audit", "--help"], + check: ["npx", "--no-install", "agents-audit", "version"], }, }; @@ -57,8 +68,18 @@ if (requested.length !== 1 && process.env.WORKSPACEJSON_RELEASE_VERSION) { // immediate post-publish check has no way to tell "not actually published" // apart from "not visible here yet" and fails the Release workflow either // way — training everyone to ignore red, which is worse than no gate at all. -const REGISTRY_PROPAGATION_RETRIES = 6; -const REGISTRY_PROPAGATION_BASE_DELAY_MS = 5000; +// +// Measured on the 0.5.0 release (2026-07-27): the version endpoint answered +// almost immediately, but the aggregated packument — which is what `npm install` +// resolves against — took ~4m30s to appear. The previous window summed to 75s, +// so it reported a red failure for a release that had in fact published +// correctly, provenance and all. A first-ever publish is the slow case: there is +// no existing packument to update, so budget for minutes, not seconds. +// +// Linear backoff with a ceiling: 10+20+30+40+50+60+60+60+60 ≈ 6m30s total. +const REGISTRY_PROPAGATION_RETRIES = 10; +const REGISTRY_PROPAGATION_BASE_DELAY_MS = 10_000; +const REGISTRY_PROPAGATION_MAX_DELAY_MS = 60_000; const isRegistryPropagationLag = (stderr) => /\bE(TARGET|404)\b|No matching version found/.test(stderr ?? ""); for (const pkg of packages) { @@ -90,7 +111,7 @@ async function installWithRetry(pkg, directory) { process.stderr.write(result.stderr); process.exit(result.status ?? 1); } - const delayMs = REGISTRY_PROPAGATION_BASE_DELAY_MS * attempt; + const delayMs = Math.min(REGISTRY_PROPAGATION_BASE_DELAY_MS * attempt, REGISTRY_PROPAGATION_MAX_DELAY_MS); console.log(`${pkg.name}@${pkg.version} not yet visible on the registry (attempt ${attempt}/${REGISTRY_PROPAGATION_RETRIES}) — retrying in ${delayMs}ms`); await new Promise((resolve) => setTimeout(resolve, delayMs)); }