From 80d2236ca50f7f1611ebb7a9d290865915858e66 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 1 Sep 2026 13:03:33 -0400 Subject: [PATCH] fix(#4112): drop redundant $(( subshell wrapping in pause-work.md Context Detection (#4140) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#4112): add failing-first regression coverage for pause-work.md Context Detection Extracts and executes the shipped Context Detection bash block to prove the $(( arithmetic-context misparse (SC1102/SC1106/SC2205) as a hard syntax error, plus boundary/independence coverage for the phase/spike/sketch/ deliberation resolution the fix must preserve. * test(#4112): fix false-green regression test — assert dash/sh failure, not bash -n The prior version asserted bash -n syntax validity and wrapped bash -c execution, neither of which detects this bug: bash/zsh silently fall back to a working command-substitution reading of the ambiguous $(( construct, and bash -n never evaluates arithmetic-context content at parse time. A gsd-test run against the still-broken file passed 41594/41594 with this test in place — a false green. POSIX sh/dash does not implement that fallback and throws a real "Syntax error: Missing '))'", which is what this version asserts against. * fix(#4112): remove leaked tool-call tags corrupting the regression test file A subagent's Write introduced trailing / markup at the end of tests/pause-work-context-detection.test.cjs. gsd-test's own tests/portability-rule-disable-ban.test.cjs (which parses every test file) correctly caught this as a parse error. Strips the garbage lines; no behavioral change. * fix(#4112): drop redundant $(( subshell wrapping in pause-work.md Context Detection phase=/spike=/sketch= opened with $((, which POSIX sh/dash parses as arithmetic expansion and rejects (Syntax error: Missing '))') since the enclosed text isn't valid arithmetic. bash/zsh silently retry it as command substitution, which masked the defect there. Dropping the inner grouping parens (matching the deliberation= line already in the same block) makes the construct valid under bash, zsh, and dash alike, with identical fallback-to-empty-string behavior when nothing matches. Once the surrounding syntax parses, ShellCheck can now also analyze the inner ls -lt calls it previously couldn't see past the parse failure, newly surfacing the same pre-existing SC2012 ("use find instead of ls") suggestion already baselined for the file's other ls usage. Baseline updated to reflect the true current count; no ls-vs-find behavior change made, as that is a separate, pre-existing, out-of-scope question. * fix(#4112): verify shell discrimination instead of trusting the binary name Code review finding: falling back from dash to sh could silently produce a non-discriminating test on a host where /bin/sh is bash-compatible (e.g. macOS) — such a shell never throws on the broken construct either, so the test would pass whether the bug were present or not. resolvePosixShell now verifies the candidate actually rejects a known-ambiguous $(( snippet before using it, and skips with an explicit reason when none does. The gsd-test Linux bench (dash as /bin/sh) is unaffected either way. * fix(#4112): route regression test through process-seam, splitLines, cleanup npm run lint:ci flagged 7 violations gsd-test's node:test run doesn't check: local/no-adhoc-markdown-parsing (single fence-spanning regex), local/no-crlf-fragile-split (bare \n split), local/no-unbounded-spawn (x4, hand-rolled execFileSync with no timeout), and local/no-raw-rmsync-in-tests. Rewrites the test to route every subprocess call through tests/helpers/process-seam.cjs's runHook() (bounded by construction), tests/helpers.cjs's cleanup() for temp-dir removal, and a line-by-line fence scan using the text-lines.cts splitLines() seam, matching this repo's established extraction idiom (tests/no-hardcoded-home-gsd-tools.test.cjs). No behavioral change to what is asserted. * docs(#4112): add changeset for the pause-work Context Detection fix * docs(#4112): backfill changeset PR number (pr:0 -> pr:4140) --------- Co-authored-by: sim --- .changeset/sunny-voles-snooze.md | 5 + gsd-core/workflows/pause-work.md | 6 +- .../lint-workflow-shellcheck-baseline.json | 42 +--- tests/pause-work-context-detection.test.cjs | 206 ++++++++++++++++++ 4 files changed, 220 insertions(+), 39 deletions(-) create mode 100644 .changeset/sunny-voles-snooze.md create mode 100644 tests/pause-work-context-detection.test.cjs diff --git a/.changeset/sunny-voles-snooze.md b/.changeset/sunny-voles-snooze.md new file mode 100644 index 000000000..a060ad7f0 --- /dev/null +++ b/.changeset/sunny-voles-snooze.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4140 +--- +**`/gsd-pause-work` no longer fails on its first step** — the Context Detection step's phase/spike/sketch lookups used a $(( construct that POSIX `sh`/dash rejects as a hard syntax error (bash/zsh happened to tolerate it via an undocumented fallback). (#4112) diff --git a/gsd-core/workflows/pause-work.md b/gsd-core/workflows/pause-work.md index 8d940356d..e44283e86 100644 --- a/gsd-core/workflows/pause-work.md +++ b/gsd-core/workflows/pause-work.md @@ -15,13 +15,13 @@ Determine what kind of work is being paused and set the handoff destination acco ```bash # Check for active phase -phase=$(( ls -lt .planning/phases/*/PLAN.md 2>/dev/null || true ) | head -1 | grep -oP 'phases/\K[^/]+' || true) +phase=$(ls -lt .planning/phases/*/PLAN.md 2>/dev/null | head -1 | grep -oP 'phases/\K[^/]+' || true) # Check for active spike -spike=$(( ls -lt .planning/spikes/*/SPIKE.md .planning/spikes/*/DESIGN.md .planning/spikes/*/README.md 2>/dev/null || true ) | head -1 | grep -oP 'spikes/\K[^/]+' || true) +spike=$(ls -lt .planning/spikes/*/SPIKE.md .planning/spikes/*/DESIGN.md .planning/spikes/*/README.md 2>/dev/null | head -1 | grep -oP 'spikes/\K[^/]+' || true) # Check for active sketch -sketch=$(( ls -lt .planning/sketches/*/README.md .planning/sketches/*/index.html 2>/dev/null || true ) | head -1 | grep -oP 'sketches/\K[^/]+' || true) +sketch=$(ls -lt .planning/sketches/*/README.md .planning/sketches/*/index.html 2>/dev/null | head -1 | grep -oP 'sketches/\K[^/]+' || true) # Check for active deliberation deliberation=$(ls .planning/deliberations/*.md 2>/dev/null | head -1 || true) diff --git a/scripts/lint-workflow-shellcheck-baseline.json b/scripts/lint-workflow-shellcheck-baseline.json index 778dbeb4d..9352c61ad 100644 --- a/scripts/lint-workflow-shellcheck-baseline.json +++ b/scripts/lint-workflow-shellcheck-baseline.json @@ -491,33 +491,18 @@ }, { "file": "gsd-core/workflows/pause-work.md", - "code": "1102", - "message": "Shells disambiguate $(( differently or not at all. For $(command substitution), add space after $( . For $((arithmetics)), fix parsing errors." + "code": "2012", + "message": "Use find instead of ls to better handle non-alphanumeric filenames." }, { "file": "gsd-core/workflows/pause-work.md", - "code": "1102", - "message": "Shells disambiguate $(( differently or not at all. For $(command substitution), add space after $( . For $((arithmetics)), fix parsing errors." + "code": "2012", + "message": "Use find instead of ls to better handle non-alphanumeric filenames." }, { "file": "gsd-core/workflows/pause-work.md", - "code": "1102", - "message": "Shells disambiguate $(( differently or not at all. For $(command substitution), add space after $( . For $((arithmetics)), fix parsing errors." - }, - { - "file": "gsd-core/workflows/pause-work.md", - "code": "1106", - "message": "In arithmetic contexts, use < instead of -lt" - }, - { - "file": "gsd-core/workflows/pause-work.md", - "code": "1106", - "message": "In arithmetic contexts, use < instead of -lt" - }, - { - "file": "gsd-core/workflows/pause-work.md", - "code": "1106", - "message": "In arithmetic contexts, use < instead of -lt" + "code": "2012", + "message": "Use find instead of ls to better handle non-alphanumeric filenames." }, { "file": "gsd-core/workflows/pause-work.md", @@ -529,21 +514,6 @@ "code": "2102", "message": "Ranges can only match single chars (mentioned due to duplicates)." }, - { - "file": "gsd-core/workflows/pause-work.md", - "code": "2205", - "message": "(..) is a subshell. Did you mean [ .. ], a test expression?" - }, - { - "file": "gsd-core/workflows/pause-work.md", - "code": "2205", - "message": "(..) is a subshell. Did you mean [ .. ], a test expression?" - }, - { - "file": "gsd-core/workflows/pause-work.md", - "code": "2205", - "message": "(..) is a subshell. Did you mean [ .. ], a test expression?" - }, { "file": "gsd-core/workflows/plan-phase.md", "code": "2010", diff --git a/tests/pause-work-context-detection.test.cjs b/tests/pause-work-context-detection.test.cjs new file mode 100644 index 000000000..df0b1402d --- /dev/null +++ b/tests/pause-work-context-detection.test.cjs @@ -0,0 +1,206 @@ +'use strict'; + +/** + * pause-work-context-detection.test.cjs — regression coverage for #4112. + * + * gsd-core/workflows/pause-work.md's "Context Detection" step opens its + * phase/spike/sketch assignments with `$((`. That is genuinely ambiguous in + * POSIX-family shell grammar between arithmetic expansion and a command + * substitution wrapping a subshell. bash and zsh silently fall back to the + * command-substitution reading when the arithmetic parse fails (undocumented + * but real — verified empirically), so this construct does NOT throw under + * bash/zsh. It DOES throw a hard "Syntax error: Missing '))'" under POSIX + * `sh` (dash), which does not implement that fallback and is the default + * `/bin/sh` on Debian/Ubuntu (including the CI/bench hosts this repo tests + * on, and a common default shell for `child_process.exec()`-style spawns + * with no explicit `shell:` override). + * + * An earlier version of this test asserted `bash -n` syntax validity and + * wrapped a plain `bash -c` execution in assert.doesNotThrow(). Both were + * wrong: `bash -n` never evaluates arithmetic-context content at parse time, + * and bash's runtime fallback means a bare `bash -c` execution never throws + * either. That version passed a full `gsd-test` run (41594/41594, 0 + * failures) against the UNFIXED file — a false green. This version fixes + * that by asserting on the real, verified failure mode: execution under + * `dash`/`sh`. + * + * A code review of an intermediate version also found that falling back + * from `dash` to plain `sh` without verifying discrimination could silently + * produce a non-discriminating test on a host where `/bin/sh` is + * bash-compatible (macOS, for example) — `resolvePosixShell` below verifies + * each shell candidate actually rejects a known-ambiguous `$((` snippet + * before trusting it, rather than assuming that from the binary name. + * + * These tests execute the real fenced bash block extracted from the shipped + * workflow file against real temp-directory filesystem states — not a mock, + * not a source-text grep of the fix. Extracting the fence line-by-line (not + * a single fence-spanning regex — matches this repo's established + * extraction idiom, e.g. tests/no-hardcoded-home-gsd-tools.test.cjs) reads + * workflow markdown, not source code; `local/no-source-grep` targets + * readFileSync of .cjs/.cts/.js/.mjs/.mts/.ts SOURCE files, which this is + * not. All subprocess calls route through tests/helpers/process-seam.cjs + * (bounded by construction, never a hand-rolled execFileSync/spawnSync); + * temp-directory removal goes through tests/helpers.cjs's `cleanup()`. + */ + +const { test, describe, before } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); +const { runHook, OUTCOME } = require('./helpers/process-seam.cjs'); +const { cleanup } = require('./helpers.cjs'); +const { splitLines } = require('../gsd-core/bin/lib/text-lines.cjs'); + +const REPO_ROOT = path.join(__dirname, '..'); +const WORKFLOW_PATH = path.join(REPO_ROOT, 'gsd-core', 'workflows', 'pause-work.md'); + +/** + * Extract the fenced ```bash block that immediately follows the given + * heading, scanning line-by-line rather than matching a single + * fence-spanning regex (the shape `local/no-adhoc-markdown-parsing` flags). + */ +function extractBashBlockAfterHeading(markdown, heading) { + const headingIdx = markdown.indexOf(heading); + assert.ok(headingIdx !== -1, `heading "${heading}" not found in ${WORKFLOW_PATH}`); + const lines = splitLines(markdown.slice(headingIdx)); + const blockLines = []; + let inBash = false; + for (const line of lines) { + if (!inBash) { + if (/^```bash\s*$/.test(line)) inBash = true; + } else if (/^```\s*$/.test(line)) { + break; + } else { + blockLines.push(line); + } + } + assert.ok(blockLines.length > 0, `no \`\`\`bash fence found after heading "${heading}"`); + return `${blockLines.join('\n')}\n`; +} + +const KNOWN_AMBIGUOUS_ARITHMETIC_SNIPPET = + 'x=$(( ls -lt /tmp 2>/dev/null || true ) | head -1 || true)'; + +/** True if `shell` is present and spawnable at all (independent of exit code). */ +function shellIsUsable(shell) { + const result = runHook('-c', ['true'], { interpreter: shell }); + return result.outcome === OUTCOME.EXITED; +} + +/** + * True if `shell` rejects the known-ambiguous `$((` construct above (i.e. it + * does NOT implement bash's permissive arithmetic-expansion-or-subshell + * fallback, so it can actually tell broken from fixed for this bug). + */ +function shellRejectsAmbiguousArithmetic(shell) { + const result = runHook('-c', [KNOWN_AMBIGUOUS_ARITHMETIC_SNIPPET], { interpreter: shell }); + return result.outcome !== OUTCOME.EXITED || result.exitCode !== 0; +} + +/** + * Resolve a POSIX shell binary that does NOT implement bash's permissive + * `$((` -> command-substitution fallback, so it can actually distinguish + * broken from fixed. Plain `/bin/sh` is unreliable for this: on macOS it is + * bash-compatible and does not reproduce the defect. Prefer `dash` (the + * default `/bin/sh` on Debian/Ubuntu, including this repo's gsd-test Linux + * bench); fall back to `sh` only if `dash` is not on PATH — and even then, + * only if `sh` actually discriminates the two cases (verified, not assumed + * from the binary name). + */ +function resolvePosixShell() { + for (const candidate of ['dash', 'sh']) { + if (!shellIsUsable(candidate)) continue; + if (shellRejectsAmbiguousArithmetic(candidate)) return candidate; + } + return null; +} + +let contextDetectionBlock; +let posixShell; + +before(() => { + const markdown = fs.readFileSync(WORKFLOW_PATH, 'utf8'); + contextDetectionBlock = extractBashBlockAfterHeading(markdown, '## Context Detection'); + posixShell = resolvePosixShell(); +}); + +/** Run the block in a fresh temp cwd under bash, returning parsed var assignments. */ +function runBlock(setup) { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'pause-work-context-')); + try { + if (setup) setup(tmpDir); + const probe = `${contextDetectionBlock}\n` + + 'printf \'phase=%s\\nspike=%s\\nsketch=%s\\ndeliberation=%s\\n\' ' + + '"$phase" "$spike" "$sketch" "$deliberation"\n'; + const result = runHook('-c', [probe], { interpreter: 'bash', cwd: tmpDir }); + assert.equal(result.outcome, OUTCOME.EXITED, `bash exited abnormally: ${result.stderr}`); + assert.equal(result.exitCode, 0, `bash exited ${result.exitCode}: ${result.stderr}`); + const out = {}; + for (const line of result.stdout.trim().split('\n')) { + const eq = line.indexOf('='); + out[line.slice(0, eq)] = line.slice(eq + 1); + } + return out; + } finally { + cleanup(tmpDir); + } +} + +describe('regression #4112: pause-work.md Context Detection block', () => { + test('does not throw a "Missing \'))\'" syntax error under POSIX sh/dash', (t) => { + if (!posixShell) { + t.skip('no POSIX shell on this host actually rejects the ambiguous $(( construct ' + + '(dash unavailable and the resolved /bin/sh is bash-compatible) — cannot prove the ' + + 'regression here; this is still proven on the gsd-test Linux bench, where /bin/sh is dash'); + return; + } + const result = runHook('-c', [contextDetectionBlock], { interpreter: posixShell }); + assert.equal(result.outcome, OUTCOME.EXITED, `${posixShell} exited abnormally: ${result.stderr}`); + assert.equal(result.exitCode, 0, `${posixShell} rejected the fixed block: ${result.stderr}`); + }); + + test('no active phase/spike/sketch/deliberation resolves all four vars to empty string, no abort', () => { + const result = runBlock(); + assert.equal(result.phase, ''); + assert.equal(result.spike, ''); + assert.equal(result.sketch, ''); + assert.equal(result.deliberation, ''); + }); + + test('single active phase resolves phase to its directory name', () => { + const result = runBlock((tmpDir) => { + const dir = path.join(tmpDir, '.planning', 'phases', '03-foo'); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync(path.join(dir, 'PLAN.md'), '# plan\n'); + }); + assert.equal(result.phase, '03-foo'); + assert.equal(result.spike, ''); + assert.equal(result.sketch, ''); + }); + + test('spike/sketch resolution is independent of phase (no active phase present)', () => { + const result = runBlock((tmpDir) => { + const spikeDir = path.join(tmpDir, '.planning', 'spikes', 'SPIKE-002'); + fs.mkdirSync(spikeDir, { recursive: true }); + fs.writeFileSync(path.join(spikeDir, 'SPIKE.md'), '# spike\n'); + + const sketchDir = path.join(tmpDir, '.planning', 'sketches', 'my-sketch'); + fs.mkdirSync(sketchDir, { recursive: true }); + fs.writeFileSync(path.join(sketchDir, 'README.md'), '# sketch\n'); + }); + assert.equal(result.phase, ''); + assert.equal(result.spike, 'SPIKE-002'); + assert.equal(result.sketch, 'my-sketch'); + }); + + test('deliberation (the already-correct pattern on line 27) is unaffected by the fix', () => { + const result = runBlock((tmpDir) => { + const delibDir = path.join(tmpDir, '.planning', 'deliberations'); + fs.mkdirSync(delibDir, { recursive: true }); + fs.writeFileSync(path.join(delibDir, 'topic.md'), '# deliberation\n'); + }); + assert.equal(result.phase, ''); + assert.match(result.deliberation, /\.planning\/deliberations\/topic\.md$/); + }); +});