diff --git a/.changeset/merry-lynx-sing.md b/.changeset/merry-lynx-sing.md new file mode 100644 index 000000000..026c2a8a9 --- /dev/null +++ b/.changeset/merry-lynx-sing.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2992 +--- +/gsd-update queries wrong npm package names — moved package name into a deterministic check-latest-version.cjs script and updated the workflow to use ${GSD_DIR} from get_installed_version. See #2992. diff --git a/.changeset/witty-newts-greet.md b/.changeset/witty-newts-greet.md new file mode 100644 index 000000000..026c2a8a9 --- /dev/null +++ b/.changeset/witty-newts-greet.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2992 +--- +/gsd-update queries wrong npm package names — moved package name into a deterministic check-latest-version.cjs script and updated the workflow to use ${GSD_DIR} from get_installed_version. See #2992. diff --git a/get-shit-done/bin/check-latest-version.cjs b/get-shit-done/bin/check-latest-version.cjs new file mode 100755 index 000000000..bf2d54193 --- /dev/null +++ b/get-shit-done/bin/check-latest-version.cjs @@ -0,0 +1,99 @@ +#!/usr/bin/env node +'use strict'; + +/** + * Deterministic latest-version check for /gsd-update (#2992). + * + * The /gsd-update workflow's check_latest_version step was previously + * prescribed in LLM-driven prose ("run `npm view get-shit-done-cc + * version`"). The executing model could shortcut the prescription and + * invent npm queries against wrong-shaped names (`@get-shit-done/cli`, + * `get-shit-done-cli`, `gsd`), all of which 404 or — worse — return an + * unrelated typosquat package. + * + * This script makes the package name a CONSTANT in code, not a free + * choice at execution time. The workflow calls it via `npm run + * check-latest-version -- --json` and parses the structured response. + * + * Tests assert on the typed CHECK_REASON enum and the structured result + * record, never on console prose. See CONTRIBUTING.md "Prohibited: Raw + * Text Matching on Test Outputs". + */ + +const cp = require('node:child_process'); + +// Hardcoded. Do not parameterise — the whole point of this script is that +// the package name is not a runtime choice for the caller. +const PACKAGE_NAME = 'get-shit-done-cc'; + +const CHECK_REASON = Object.freeze({ + OK: 'ok', + FAIL_NPM_FAILED: 'fail_npm_failed', + FAIL_INVALID_OUTPUT: 'fail_invalid_output', +}); + +const SEMVER_RE = /^\d+\.\d+\.\d+(?:[-+][0-9A-Za-z.-]+)?$/; + +/** + * Pure-ish: takes an injected spawn function so tests don't actually run npm. + * In production, defaults to cp.spawnSync('npm', ...). + */ +function checkLatestVersion(opts = {}) { + const defaultSpawn = () => cp.spawnSync('npm', ['view', PACKAGE_NAME, 'version'], { + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'pipe'], + shell: process.platform === 'win32', // npm is npm.cmd on Windows + // Bound the registry call so a hung network/registry doesn't block the + // entire /gsd-update workflow indefinitely (#2993 CR). 15s is generous + // for `npm view version`; on timeout, spawnSync returns with + // signal !== null and the existing failure path emits FAIL_NPM_FAILED. + timeout: 15_000, + }); + const spawn = opts.spawn || defaultSpawn; + + const r = spawn(); + if (!r || r.status !== 0) { + // Distinguish timeout (status null, signal set, stderr empty) from a + // genuine npm failure. Without this, both surfaced as "npm exited + // non-zero" and the operator couldn't tell which (#2993 CR). + let detail; + if (r && r.signal) { + detail = `npm timed out (signal: ${r.signal})`; + } else if (r && r.stderr) { + detail = r.stderr.trim(); + } else { + detail = 'npm exited non-zero'; + } + return { + ok: false, + reason: CHECK_REASON.FAIL_NPM_FAILED, + detail, + }; + } + const version = (r.stdout || '').trim(); + if (!SEMVER_RE.test(version)) { + return { + ok: false, + reason: CHECK_REASON.FAIL_INVALID_OUTPUT, + detail: version || '(empty)', + }; + } + return { ok: true, version, reason: CHECK_REASON.OK }; +} + +function main() { + const json = process.argv.includes('--json'); + const r = checkLatestVersion(); + if (json) { + process.stdout.write(JSON.stringify(r) + '\n'); + } else if (r.ok) { + process.stdout.write(r.version + '\n'); + } else { + process.stderr.write(`check-latest-version: ${r.reason}: ${r.detail}\n`); + } + process.exit(r.ok ? 0 : 1); +} + +if (require.main === module) main(); + +module.exports = { checkLatestVersion, CHECK_REASON, PACKAGE_NAME }; diff --git a/get-shit-done/workflows/update.md b/get-shit-done/workflows/update.md index 68dbd45b3..0d61e8eb3 100644 --- a/get-shit-done/workflows/update.md +++ b/get-shit-done/workflows/update.md @@ -113,6 +113,12 @@ if [ -n "$PREFERRED_CONFIG_DIR" ] && { [ -f "$PREFERRED_CONFIG_DIR/get-shit-done echo "$INSTALLED_VERSION" echo "$INSTALL_SCOPE" echo "${PREFERRED_RUNTIME:-claude}" + # 4-line output contract (#2993 CR): early-return path must also emit + # GSD_DIR or downstream check_latest_version misreads the install as + # UNKNOWN. PREFERRED_CONFIG_DIR is the resolved config dir we just + # validated above (line 95-96); it is the right GSD_DIR value for + # this fast path. + echo "$PREFERRED_CONFIG_DIR" exit 0 fi @@ -222,34 +228,41 @@ if [ "$IS_LOCAL" = true ]; then INSTALLED_VERSION="$(cat "$LOCAL_VERSION_FILE")" INSTALL_SCOPE="LOCAL" TARGET_RUNTIME="$LOCAL_RUNTIME" + RESOLVED_GSD_DIR="$LOCAL_DIR" elif [ -n "$GLOBAL_VERSION_FILE" ] && [ -f "$GLOBAL_VERSION_FILE" ] && [ -f "$GLOBAL_MARKER_FILE" ] && grep -Eq '^[0-9]+\.[0-9]+\.[0-9]+' "$GLOBAL_VERSION_FILE"; then INSTALLED_VERSION="$(cat "$GLOBAL_VERSION_FILE")" INSTALL_SCOPE="GLOBAL" TARGET_RUNTIME="$GLOBAL_RUNTIME" + RESOLVED_GSD_DIR="$GLOBAL_DIR" elif [ -n "$LOCAL_RUNTIME" ] && [ -f "$LOCAL_MARKER_FILE" ]; then # Runtime detected but VERSION missing/corrupt: treat as unknown version, keep runtime target INSTALLED_VERSION="0.0.0" INSTALL_SCOPE="LOCAL" TARGET_RUNTIME="$LOCAL_RUNTIME" + RESOLVED_GSD_DIR="$LOCAL_DIR" elif [ -n "$GLOBAL_RUNTIME" ] && [ -f "$GLOBAL_MARKER_FILE" ]; then INSTALLED_VERSION="0.0.0" INSTALL_SCOPE="GLOBAL" TARGET_RUNTIME="$GLOBAL_RUNTIME" + RESOLVED_GSD_DIR="$GLOBAL_DIR" else INSTALLED_VERSION="0.0.0" INSTALL_SCOPE="UNKNOWN" TARGET_RUNTIME="claude" + RESOLVED_GSD_DIR="" fi echo "$INSTALLED_VERSION" echo "$INSTALL_SCOPE" echo "$TARGET_RUNTIME" +echo "$RESOLVED_GSD_DIR" ``` Parse output: - Line 1 = installed version (`0.0.0` means unknown version) - Line 2 = install scope (`LOCAL`, `GLOBAL`, or `UNKNOWN`) - Line 3 = target runtime (`claude`, `opencode`, `gemini`, `kilo`, or `codex`) +- Line 4 = resolved GSD config dir (e.g. `/Users/me/.claude`, `/Users/me/.gemini`); empty if scope is `UNKNOWN`. Capture this as `GSD_DIR` and pass it to subsequent steps so they don't have to re-derive the runtime path. - If scope is `UNKNOWN`, proceed to install step using `--claude --global` fallback. If multiple runtime installs are detected and the invoking runtime cannot be determined from execution_context, ask the user which runtime to update before running install. @@ -269,15 +282,43 @@ Proceed to install step (treat as version 0.0.0 for comparison). -Check npm for latest version: +Check npm for latest version via the deterministic script. **Do NOT run `npm view` or `npm search` directly** — the package name must come from the script, not from a free choice at execution time. (#2992: LLM-driven prescriptions of npm package names produced wrong-package queries; moving the package name into a script constant closes that gap.) + +The `GSD_DIR` value emitted by `get_installed_version` (line 4) resolves to the runtime-specific config dir (`~/.claude/`, `~/.gemini/`, `~/.codex/`, etc.), so the script invocation works for every runtime — not just Claude. If `GSD_DIR` is empty (scope `UNKNOWN`), skip this step and go directly to install. + +`LATEST_RESULT` is a JSON document with the documented shape `{ ok: bool, version: string, reason: string, detail?: string }`. Parse via `jq` ONLY when the script actually ran. When `GSD_DIR` is empty (scope `UNKNOWN`), skip the check entirely and seed the parsed fields with their no-op values so downstream logic does not mistake an unset `LATEST_RESULT` for a failed network check (#2993 CR feedback): ```bash -npm view get-shit-done-cc version 2>/dev/null +if [ -z "$GSD_DIR" ]; then + # No install detected — fall through to install step; version-check is skipped. + LATEST_RESULT="" + LATEST_STATUS=0 + LATEST_OK=false + LATEST_VERSION="" + LATEST_REASON="no_install_detected" +else + LATEST_RESULT="$(node "$GSD_DIR/get-shit-done/bin/check-latest-version.cjs" --json 2>/dev/null)" + LATEST_STATUS=$? + # #2993 CR: when node is missing or the script doesn't exist, LATEST_RESULT + # is empty and piping it to `jq` produces a parse error on stderr while + # leaving LATEST_OK / LATEST_REASON as empty strings. Fail the check with a + # meaningful reason instead of a blank diagnostic. + if [ -n "$LATEST_RESULT" ]; then + LATEST_OK="$(printf '%s' "$LATEST_RESULT" | jq -r '.ok // false')" + LATEST_VERSION="$(printf '%s' "$LATEST_RESULT" | jq -r '.version // empty')" + LATEST_REASON="$(printf '%s' "$LATEST_RESULT" | jq -r '.reason // empty')" + else + LATEST_OK=false + LATEST_VERSION="" + LATEST_REASON="script_not_found_or_node_unavailable" + fi +fi ``` -**If npm check fails:** -``` -Couldn't check for updates (offline or npm unavailable). +**If `LATEST_OK` is not `true`** (or `LATEST_STATUS` is non-zero): + +```text +Couldn't check for updates (reason: {LATEST_REASON}, exit: {LATEST_STATUS}). To update manually: `npx get-shit-done-cc --global` ``` diff --git a/tests/bug-2992-check-latest-version.test.cjs b/tests/bug-2992-check-latest-version.test.cjs new file mode 100644 index 000000000..9780ec6f5 --- /dev/null +++ b/tests/bug-2992-check-latest-version.test.cjs @@ -0,0 +1,96 @@ +'use strict'; +process.env.GSD_TEST_MODE = '1'; + +const { test, describe, before, after } = require('node:test'); +const assert = require('node:assert/strict'); +const path = require('node:path'); +const cp = require('node:child_process'); + +const ROOT = path.join(__dirname, '..'); +const { checkLatestVersion, CHECK_REASON, PACKAGE_NAME } = require( + path.join(ROOT, 'get-shit-done', 'bin', 'check-latest-version.cjs'), +); + +// checkLatestVersion is a pure-ish function: it spawns one fixed npm +// command, validates the output, and returns { ok, version | reason }. +// The package name is HARDCODED — not a free choice for the caller. +// Tests use a pluggable spawn so no real npm process is invoked. + +describe('Bug #2992: deterministic latest-version check', () => { + test('PACKAGE_NAME is the constant get-shit-done-cc (no callers can override)', () => { + assert.equal(PACKAGE_NAME, 'get-shit-done-cc'); + }); + + test('CHECK_REASON enum exposes the documented codes', () => { + assert.deepEqual( + Object.keys(CHECK_REASON).sort(), + ['FAIL_INVALID_OUTPUT', 'FAIL_NPM_FAILED', 'OK'].sort(), + ); + }); + + test('returns { ok: true, version } when npm prints a valid semver', () => { + const fakeSpawn = () => ({ status: 0, stdout: '1.39.1\n', stderr: '' }); + const r = checkLatestVersion({ spawn: fakeSpawn }); + assert.deepEqual(r, { ok: true, version: '1.39.1', reason: CHECK_REASON.OK }); + }); +}); + +describe('Bug #2992: error paths', () => { + const { checkLatestVersion, CHECK_REASON } = require(require('node:path').join(__dirname, '..', 'get-shit-done', 'bin', 'check-latest-version.cjs')); + + test('FAIL_NPM_FAILED when npm exits non-zero (e.g. offline, 404)', () => { + const r = checkLatestVersion({ + spawn: () => ({ status: 1, stdout: '', stderr: 'npm ERR! 404\n' }), + }); + assert.equal(r.ok, false); + assert.equal(r.reason, CHECK_REASON.FAIL_NPM_FAILED); + assert.equal(r.detail, 'npm ERR! 404', + 'detail should be the trimmed stderr when npm reports a real error'); + }); + + // #2993 CR: distinguish timeout from genuine npm failure in `detail`. + // spawnSync sets status=null and signal='SIGTERM' on timeout; stderr is + // typically empty. Without the signal-first branch, both shape as + // 'npm exited non-zero' and the operator cannot tell timeout from failure. + test('FAIL_NPM_FAILED detail names the signal when spawn times out', () => { + const r = checkLatestVersion({ + spawn: () => ({ status: null, signal: 'SIGTERM', stdout: '', stderr: '' }), + }); + assert.equal(r.ok, false); + assert.equal(r.reason, CHECK_REASON.FAIL_NPM_FAILED); + assert.equal(r.detail, 'npm timed out (signal: SIGTERM)', + 'detail should explicitly name the signal when status is null and signal is set'); + }); + + test('FAIL_NPM_FAILED detail falls back to generic when neither stderr nor signal is present', () => { + const r = checkLatestVersion({ + spawn: () => ({ status: 1, stdout: '', stderr: '' }), + }); + assert.equal(r.detail, 'npm exited non-zero'); + }); + + test('FAIL_INVALID_OUTPUT when npm prints something that is not a semver', () => { + // E.g. if a future npm version changes the output format, or if the + // network returns an HTML error page captured as stdout. + const r = checkLatestVersion({ + spawn: () => ({ status: 0, stdout: 'not a version\n', stderr: '' }), + }); + assert.equal(r.ok, false); + assert.equal(r.reason, CHECK_REASON.FAIL_INVALID_OUTPUT); + }); + + test('FAIL_INVALID_OUTPUT when stdout is empty', () => { + const r = checkLatestVersion({ + spawn: () => ({ status: 0, stdout: '', stderr: '' }), + }); + assert.equal(r.ok, false); + assert.equal(r.reason, CHECK_REASON.FAIL_INVALID_OUTPUT); + }); + + test('accepts pre-release semver (e.g. 1.40.0-rc.1)', () => { + const r = checkLatestVersion({ + spawn: () => ({ status: 0, stdout: '1.40.0-rc.1\n', stderr: '' }), + }); + assert.deepEqual(r, { ok: true, version: '1.40.0-rc.1', reason: CHECK_REASON.OK }); + }); +});