fix(#2992): deterministic latest-version check — package name is a constant, not LLM choice (#2993)

* fix(#2992): deterministic latest-version check — package name is a constant, not LLM choice

The /gsd-update workflow's check_latest_version step was prescribed in
LLM-driven prose: "run `npm view get-shit-done-cc version`". The
executing model could and did shortcut the prescription and invent
npm queries against name-shaped guesses — `@get-shit-done/cli`,
`get-shit-done-cli`, `gsd` — all of which 404 or, worse, return an
unrelated typosquat (the 2016 `get-shit-done` timer package). Same
architectural anti-pattern as #2969 (Hunk Verification Gate where
the LLM filled `verified: yes` without checking).

Implementation built TDD per #2992:

  get-shit-done/bin/check-latest-version.cjs
    - PACKAGE_NAME = 'get-shit-done-cc' as a module constant; not
      parameterised, not exposed for override.
    - checkLatestVersion({ spawn? }) returns
      { ok: bool, version?: string, reason: CHECK_REASON.X, detail? }
      via a frozen enum: OK / FAIL_NPM_FAILED / FAIL_INVALID_OUTPUT.
    - --json mode emits the structured record on stdout for the
      workflow to parse via jq.
    - Windows-aware: uses { shell: process.platform === 'win32' }
      since npm is npm.cmd on Windows (same lesson as #2962).
    - Stored under get-shit-done/bin/ (not top-level scripts/) because
      that path IS in the user's installed config dir; top-level
      scripts/ ships in the npm tarball but is not copied into
      ~/.claude/ at install time.

  tests/bug-2992-check-latest-version.test.cjs
    - 7 tests, all assertions on the typed CHECK_REASON enum + the
      structured record. Injectable spawn function so no real npm
      process is invoked. Covers OK, npm-non-zero, invalid-output,
      empty-output, pre-release semver, PACKAGE_NAME constant lock,
      enum-shape lock.

  get-shit-done/workflows/update.md
    - check_latest_version step rewritten to call the script via
      `node "${GSD_HOME}/get-shit-done/bin/check-latest-version.cjs"
      --json` and parse the structured response with jq. Explicit
      "Do NOT run `npm view` or `npm search` directly" guidance
      cites #2992 so future contributors understand why.

Closes #2992

* fix(#2992): trailing slash on GSD_HOME default to satisfy bare-path lint

The bug-2470 regression test scans update.md for bare `$HOME/.claude`
references (no trailing slash). The PR added one in the new
check_latest_version step. Fix: trailing slash on the default value
(`${GSD_HOME:-$HOME/.claude/}`). Bash POSIX collapses the resulting
double slash; the lint pattern's negative lookahead is now satisfied.

* fix(#2992): emit GSD_DIR from get_installed_version, use it in check_latest_version

Addresses CodeRabbit feedback: the previous `${GSD_HOME:-$HOME/.claude/}`
fallback hardcoded the Claude runtime path, which silently breaks for
non-Claude runtimes (gemini, codex, opencode, kilo).

Fix:
- get_installed_version now emits a 4th line with the resolved config
  dir ($LOCAL_DIR or $GLOBAL_DIR), captured by callers as GSD_DIR.
- check_latest_version uses $GSD_DIR/get-shit-done/bin/check-latest-version.cjs.
  Empty GSD_DIR (UNKNOWN scope) skips the version check and falls
  through to fresh-install path.

This keeps the package name deterministic (#2992) AND respects the
detected runtime, instead of assuming Claude.

* chore(#2992): add changeset fragment for PR #2993

* chore(#2992): add changeset fragment for PR #2993

* fix(#2992): consolidate LATEST_RESULT parsing inside the GSD_DIR guard

CodeRabbit on PR #2993: the previous structure separated the GSD_DIR
guard from the jq parsing, so when GSD_DIR was empty the parsing block
ran against an unset LATEST_RESULT and produced misleading 'couldn't
check for updates' diagnostics instead of clean 'no_install_detected'.

Move all field assignments inside the conditional so the skip path
seeds LATEST_OK=false, LATEST_VERSION='', LATEST_REASON='no_install_detected',
and LATEST_STATUS=0 atomically.

* fix(#2992): emit GSD_DIR in early-return; add code-block lang and spawnSync timeout (CR)

CodeRabbit on PR #2993 caught three issues:

1. (Major) The early-return path in get_installed_version (PREFERRED_CONFIG_DIR
   fast path) only echoed 3 lines, but PR #2993 changed the contract to 4
   (GSD_DIR is now line 4). Downstream check_latest_version misread valid
   installs as UNKNOWN. Added `echo "$PREFERRED_CONFIG_DIR"` before exit 0.

2. (Minor) Markdown MD040: fenced code block at line 310 was missing a
   language identifier. Added ```text.

3. (Quick win) spawnSync('npm view ...') had no timeout, so a hung network
   could block /gsd-update indefinitely. Added 15s timeout; on timeout
   spawnSync returns with signal !== null and the existing failure path
   emits FAIL_NPM_FAILED.

* fix(#3008): kill cross-process race in install-minimal:307 mid-copy test

Old shape compared listTmpStageDirs() snapshots before/after the
mid-copy throw. Under scripts/run-tests.cjs --test-concurrency=4,
tests/install-minimal-all-runtimes.test.cjs runs in a parallel
subprocess and also creates gsd-minimal-skills-* dirs in shared
os.tmpdir(). The parallel process's create/remove activity between
this test's two snapshots caused deterministic failure when timing
aligned -- presented as 'flaky' but is a real race.

CI failure data (PR #2993 run 25238555786):
  expected (before): ['gsd-minimal-skills-km1O1O']
  actual   (after):  []

Both processes behaved correctly in isolation. The test was wrong:
it observed a shared filesystem state across processes.

Fix: stub fs.mkdtempSync inside this test to record THIS call's
stage dir path. After the throw, assert fs.existsSync(stagedDir)
=== false. Direct observation of the function's own behavior; no
global tmpdir scan; no parallel-process interference.

Closes #3008

* fix(#2992): distinguish timeout from npm failure; guard empty LATEST_RESULT (CR)

CodeRabbit on PR #2993 (post-fix-up review) caught two improvements:

1. (Low value) check-latest-version.cjs:55-61 — when spawnSync times
   out, r.status is null and r.signal is set (e.g. 'SIGTERM'), but
   r.stderr is empty. Without the signal-first branch, both timeouts
   and genuine npm failures shaped as 'npm exited non-zero' in detail,
   making logs ambiguous. Added explicit signal-first branch:
   'npm timed out (signal: SIGTERM)'.

2. (Quick win) update.md:284-315 — when node is missing or the script
   doesn't exist, LATEST_RESULT is empty. Piping empty to jq parses
   without error but leaves LATEST_OK / LATEST_REASON as empty
   strings, producing the user-visible diagnostic
   'Couldn\'t check for updates (reason: , exit: N)' with a blank
   reason. Added an explicit guard that sets LATEST_REASON to
   'script_not_found_or_node_unavailable' when LATEST_RESULT is empty,
   so operators see a meaningful failure message.

Tests: bug-2992 grows by 2 cases (timeout signal detail + empty
stderr fallback).
This commit is contained in:
Tom Boucher
2026-05-02 00:29:31 -04:00
committed by GitHub
parent ffeeb92c14
commit cde793f1f0
5 changed files with 251 additions and 5 deletions

View File

@@ -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.

View File

@@ -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.

View File

@@ -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 <pkg> 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 };

View File

@@ -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).
</step>
<step name="check_latest_version">
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`
```

View File

@@ -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: '<html>not a version</html>\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 });
});
});