90e60b414595f510d6f519e5087ce7b7943ae38c
2 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Tom Boucher
|
639e4d603a |
refactor(shell-projection): migrate all subprocess call sites to exec*/probeTty seam (Phase 2, #3466) (#3476)
* refactor(shell-projection): migrate planning-workspace.cjs tty probe to probeTty seam (#3466) Replaces direct execFileSync('tty') with probeTty() from the shell-projection seam. Removes try/catch — probeTty() returns null on error/non-tty/win32. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor(shell-projection): migrate commands.cjs to execGit seam (#3466) - Replaces execSync('git diff --cached --name-only') with execGit array call - Migrates 14 existing execGit(cwd, args) callers from core.cjs's local wrapper to the seam's execGit(args, { cwd }) signature - Drops execGit from the core.cjs destructure to resolve naming collision Drops try/catch around git diff — execGit returns exitCode without throwing, so the no-staged-files / not-a-git-repo case is detected by exitCode !== 0. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor(shell-projection): migrate check-latest-version.cjs to execNpm seam (#3466) Routes the default-spawn path through execNpm — execNpm owns the win32 shell-flag policy. The injection point remains spawnSync-shaped for test compatibility; an internal adapter translates { exitCode } → { status }. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor(shell-projection): migrate init.cjs git calls to execGit seam (#3466) Replaces 3 execSync calls with execGit array-args: - detectChildRepos: git status --porcelain - cmdInitNewWorkspace: git --version (worktree availability probe) - cmdRemoveWorkspace: git status --porcelain Drops 3 try/catch blocks — execGit returns exitCode without throwing, so best-effort handling becomes a clean exitCode === 0 check. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor(shell-projection): migrate core.cjs to execGit seam delegation (#3466) - Removes direct require('child_process') from core.cjs - Replaces execFileSync('git check-ignore') with seam's execGit - Local execGit wrapper now a thin adapter delegating to seam — keeps the legacy (cwd, args) positional signature and derived timedOut field for the verify.cjs and worktree-safety.cjs consumers that are out of Phase 2 scope (the wrapper proper would only be removed once those consumers migrate, tracked separately) Extends the seam's _spawnResult to expose signal and error fields so callers can compute timedOut without bypassing the seam. The Phase 1 test suite asserts on required field presence only, so the extension is backward-compatible. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor(shell-projection): migrate graphify.cjs to execTool seam (#3466) - execGraphify: spawnSync('graphify', ...) → execTool with env passthrough, preserving the ENOENT/TIMEOUT/EXIT_NONZERO typed reason mapping using the seam's signal/error fields - checkGraphifyInstalled: spawnSync('graphify', ['--help']) → execTool - checkGraphifyVersion strategy 1: graphify --version via execTool - checkGraphifyVersion strategy 2: python3 importlib.metadata via execTool Adds env option to execTool — graphify needs PYTHONUNBUFFERED=1 to drain buffered stdout on long-running operations. Changes seam internals to access spawnSync/execFileSync via the non-destructured childProcess module reference. Destructured imports capture references at load time and are un-mockable by mock.method(childProcess, 'spawnSync', ...) — which breaks all the graphify subprocess tests. Non-destructured access restores mockability without changing public behavior. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * feat(shell-projection): execGit defaults to non-interactive git env (#3466) Bakes GIT_TERMINAL_PROMPT=0 and GCM_INTERACTIVE=never into execGit's default env. Without these, a credential prompt or terminal-input probe blocks the git subprocess indefinitely until our 10s timeout kills it — surfacing as a generic timeout instead of the actual auth-prompt cause. These were previously set ad-hoc in worktree-safety.cjs's local execGitDefault wrapper. Moving them to the seam makes them the consistent default for every git call across the codebase. Callers can override via opts.env. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor(shell-projection): remove core.cjs local execGit wrapper; migrate verify + worktree-safety (#3466) Completes the Phase 2 "remove local execGit wrapper" criterion. All callers now use the shell-projection seam's execGit(args, opts) signature directly. - core.cjs: delete the local execGit wrapper and the execGit export. The isGitIgnored seam check now calls execGit from the seam. Worktree-safety function calls drop their execGit DI passthrough — worktree-safety's internal execGitDefault now delegates to the seam and adds timedOut. - verify.cjs: 6 callers migrate from execGit(cwd, args) to execGit(args, { cwd }). Imports execGit from the seam directly. The inspectWorktreeHealth DI passes the seam's execGit (worktree-safety now matches that shape). - worktree-safety.cjs: local execGitDefault becomes a thin adapter over the seam — no more direct spawnSync. 11 internal callers migrate to the new shape. DI contract for tests changes from (cwd, args) → (args, opts). - graphify.cjs: 2 remaining execGit callers migrate from core.cjs (now removed) to the seam directly. - test mocks updated in 3 worktree-safety test files to match the new (args, opts) DI shape — most mocks were shape-agnostic and required no changes; only those that destructured cwd/args needed updates. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore(changeset): add entry for shell-projection Phase 2 migration (#3466) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(pr3476): address CodeRabbit review findings --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> |
||
|
Tom Boucher
|
cde793f1f0 |
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). |