golem15/msd-core
4
0
Fork 0
Code Issues 1 Pull Requests Actions 2 Packages Projects Releases Wiki Activity

fix: recover silently-excluded test dirs + test-architecture audit hardening (#1195)

Browse Source 
* fix: recurse test discovery so subdir test suites actually run

scripts/run-tests.cjs discovered tests with a flat readdirSync(testDir),
silently excluding tests/observability/ (4 files), tests/dispatch/ (1) and
tests/installer-migrations/ (1) — 94 passing tests — from `npm test` and all
CI lanes. Walk the tree recursively (relative subpaths preserved), classify
suites by basename, and add a fail-on-zero-executed guard for suite/default
runs (escape hatch GSD_ALLOW_EMPTY_SUITE=1) while preserving the empty
--files/--files-from path the CI inert lane relies on.

Unit suite 735 -> 741 files; surfaces ADR-227's observability/dispatch seam.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* test: retire 5 verified-worthless tests

Adversarial verification confirmed these 5 prove nothing — their coverage is
provided more strictly elsewhere:
- enh-2790 'has a name: field' spot-checks (command-contract enforces /^gsd[:-]/)
- command-routing-hub duplicate construct + duplicate ERROR_KINDS assertions
- no-cjs-sdk-handsync-tooling (guarded files that never existed on main; bug-190
  covers the real retired SDK artifacts)
- runtime-artifact-layout cline edge case (subsumed by the explicit-global test
  and bug-782-cline-skills-emission)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* test: add ADR-218 release version-validation coverage

ADR-218 (reject leading-zero versions like 1.01.0; npm duplicate pre-check) had
zero tests — the logic lived only in release.yml bash. Add a test that extracts
the actual rejection regexes from the workflow and exercises them against a
boundary table (leading-zero/malformed rejected, valid accepted) plus structural
wiring assertions. Goes red if the regex is reverted to [0-9]+.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* test: redesign weak tests into behavioral, deterministic assertions

Per the ADR test audit, rewrite 27 weak test files (test-only, no source
changes) so each can go red for the defect it guards:
- kill pass-always assert.ok(true) placeholders (research-cli, worktree-baseref,
  bug-260 security guard, eslint-rules x24, clusters '|| true')
- replace source-text grep with behavioral calls (install Kilo, sh-hook-paths,
  plan-review-convergence) and add a repo-layout governance test
- de-flake real-clock/Math.random coupling (phase last_updated, bug-3707 mtime,
  context-utilization property, feat-3594)
- fix independence/shared-state violations (bug-492 singleton, issue-844 tmpRoot,
  core reapStaleTempFiles, active-workstream TTY, feat-488 GSD_HOME)
- strengthen property/shape-only tests (research-provider/store classification +
  collision) and unconditional plugin.json schema validation (issue-766)

Verified: all 28 files run together 1220 pass / 0 fail / 1 skip.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* chore: add no-tautological-assert lint rule, error in test suite

New custom ESLint rule (eslint-rules/no-tautological-assert.cjs) bans asserts
that can never fail: assert(true)/assert.ok(<always-truthy literal>),
'cond || true' inside an assert, and equality asserts comparing two identical
literals. Wired as error on tests/**; full sweep confirmed zero existing
violations so the suite stays green. Prevents the placeholder-assert regressions
the audit redesigns just removed. RuleTester coverage added (6 valid, 8 invalid).

Note: no-only-tests was already enforced via eslint-plugin-no-only-tests, so no
duplicate rule was added.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* chore: gate new allow-test-rule exemptions to require an issue ref

ADR-456 requires any allow-test-rule exemption added after the ADR to carry a
tracking issue number, but nothing enforced it. New ratchet gate
(scripts/lint-allow-test-rule-refs.cjs, wired into lint:ci) fails when a NEW
allow-test-rule comment lacks a #NNN/URL reference; the 323 existing untracked
exemptions are grandfathered in an allowlist that ratchets down as they gain
refs. Red-green verified (novel untracked offender fails; compliant passes).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* docs: add ADR test-audit evidence report (#1192)

Full risk-first qa-test-architect audit of the ADR portfolio (37 ADRs + 4
platform lenses, adversarial verification of retire verdicts) that drove the
P0 discovery fix, ADR-218 coverage, 5 retires, 27 redesigns, and the two new
lint gates. Filed as point-in-time evidence under docs/issueevidence/, named
for tracking issue #1192.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* test: replace pre-existing raw NUL byte with escape in feat-3594 fixture

feat-3594's null-byte parser fixture contained a literal NUL byte (pre-existing
on next at b10e5681 — confirmed: base blob has 1 NUL, this fix has 0), which
made git treat the file as binary and would break grep/editors. Switch to the
\x00 escape; the runtime string value (a real NUL in the parser input) is
unchanged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* test: address adversarial-review findings

Codex adversarial pass over the branch:
- capability-registry drift test no longer mutates the committed generated
  capability-registry.cjs in place (concurrency hazard) — uses in-memory
  checkPipeline comparison instead.
- allow-test-rule ratchet now detects exemptions in ALL comment forms (block
  /* */ too, matching no-source-grep) so a block comment can't bypass it;
  one newly-surfaced pre-existing offender grandfathered (323->324).
- install.test Kilo case asserts on what install(false,'kilo') actually writes
  rather than manually calling configureKiloPermissions (masked the call site).
- issue-766 drops the undeclared transitive ajv dep for explicit structural
  assertions from the schema fixture.
- adr-218 test notes the hotfix leading-zero gap is tracked in #1186.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix: address code-review findings (subdir discovery, rule + test gaps)

xhigh code review surfaced 15 confirmed issues, all fixed:
- run-tests.cjs --files now resolves subdir tests by bare basename + handles
  Windows backslash paths (ambiguous basenames error clearly).
- affected-tests-lib.cjs listTestFiles made recursive — the targeted CI lane was
  silently dropping changed subdir tests (same false-green class the audit fixed).
- no-tautological-assert now catches 'true || cond' and empty []/{}  equality.
- verify-test-quality: restore provenance-classification coverage, tighten the
  writeFile circular-detection check, guard the module-level file read.
- sh-hook-paths: cover the global-install .sh delegation branch (#2045 guard).
- active-workstream null-guard runs deterministically (no longer skipped on TTY).
- adr-218 structural guards tightened (major/minor leading-zero; needs: membership).
- repo-layout AGENTS.md guard no longer false-alarms on equivalent refactors.
- cross-ai ordering guard fails red when the step is missing.
- issue-766 parses required fields from the schema fixture (auto-enforced).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* test: stub USERPROFILE alongside HOME in feat-488 (Windows parity)

The feat-488 redesign stubbed process.env.HOME but not USERPROFILE; os.homedir()
resolves from USERPROFILE on Windows, so the home stub was not hermetic there —
caught by windows-test-parity-guard (stubsHomeNoUserProfile). Save/set/restore
USERPROFILE symmetrically with HOME (delete-if-originally-undefined).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* chore: reconcile allow-test-rule allowlist after rebase onto next

Rebasing onto current next pulled in merged PR #1170, which added
inventory-headings-countfree.test.cjs (a baseline allow-test-rule exemption) and
deleted inventory-counts.test.cjs. Grandfather the former and prune the latter so
the ratchet matches the merged tree. No new debt from this PR.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
2026-06-13 23:35:08 -04:00
committed by GitHub
parent 3ebd13c45d
commit 5fa4dcd78c
44 changed files with 3958 additions and 757 deletions
Download Patch File Download Diff File Expand all files Collapse all files

193
tests/sh-hook-paths.test.cjs
View File

@@ -24,15 +24,8 @@
const { describe, test } = require('node:test');
const assert = require('node:assert/strict');
const fs = require('fs');
const path = require('path');
const INSTALL_SRC = path.join(__dirname, '..', 'bin', 'install.js');
// ADR-857 phase 5f-1b: the sh-hook command-construction code moved from install.js
// into applySettingsJsonHooks in src/runtime-hooks-surface.cts. Source-scan checks
// that reference commandVar patterns must target the new canonical source.
const HOOKS_SURFACE_SRC = path.join(__dirname, '..', 'src', 'runtime-hooks-surface.cts');
// buildHookCommand was extracted to gsd-core/bin/lib/runtime-hooks-surface.cjs
// (ADR-857 phase 5f-1) and re-exported via install.js. Import through install.js
// so the test exercises the same public surface that the rest of the codebase uses.
@@ -40,9 +33,9 @@ const INSTALL = require(path.join(__dirname, '..', 'bin', 'install.js'));
const { buildHookCommand } = INSTALL;
const SH_HOOKS = [
{ name: 'gsd-validate-commit.sh', commandVar: 'validateCommitCommand' },
{ name: 'gsd-session-state.sh', commandVar: 'sessionStateCommand' },
{ name: 'gsd-phase-boundary.sh', commandVar: 'phaseBoundaryCommand' },
{ name: 'gsd-validate-commit.sh' },
{ name: 'gsd-session-state.sh' },
{ name: 'gsd-phase-boundary.sh' },
];
// Use a fixed configDir that is unambiguously absolute so the assertions below
@@ -53,18 +46,6 @@ const TEST_CONFIG_DIR = '/test-home/.claude';
const HOOK_OPTS = { platform: 'linux', runtime: 'claude' };
describe('bugs #2045 #2046: .sh hook paths must be absolute and quoted', () => {
let src;
try {
// ADR-857 phase 5f-1b: command-construction code moved to runtime-hooks-surface.cts.
// Concatenate both sources so structural assertions find patterns in either file.
const installSrc = fs.readFileSync(INSTALL_SRC, 'utf-8');
const hooksSurfaceSrc = fs.readFileSync(HOOKS_SURFACE_SRC, 'utf-8');
src = installSrc + '\n' + hooksSurfaceSrc;
} catch {
src = '';
}
// ── Test 1: buildHookCommand supports .sh files (BEHAVIORAL) ─────────────
describe('buildHookCommand', () => {
test('returns a bash command for .sh hookName', () => {
@@ -115,81 +96,125 @@ describe('bugs #2045 #2046: .sh hook paths must be absolute and quoted', () => {
});
});
// ── Test 2: each .sh command variable uses a quoted path ─────────────────
for (const { name, commandVar } of SH_HOOKS) {
describe(`${name} command`, () => {
test(`${commandVar} uses double-quoted path (fixes #2045 Windows spaces)`, () => {
const varIdx = src.indexOf(commandVar);
assert.ok(varIdx !== -1, `${commandVar} not found in install.js`);
// ── Tests 2-4: behavioral buildHookCommand checks for each .sh hook ────────
// These replace the former source-grep variable-name scans. We call
// buildHookCommand directly for each .sh hook on both linux and win32 and
// assert three properties that the bugs required:
// (a) the returned command is non-empty
// (b) a bash runner appears in the command (linux path; win32 uses the
// path directly as the invocation, so the check is conditioned on OS)
// (c) the configDir is embedded as an absolute, double-quoted prefix
// Extract the assignment block (~300 chars should cover a single declaration)
const blockEnd = Math.min(src.length, varIdx + 400);
const block = src.slice(varIdx, blockEnd);
// Absolute configDir with a space in it exercises the #2045 quoting bug.
const SPACED_CONFIG_DIR = '/home/first last/.claude';
// The command string for the global branch must contain a quoted path:
// bash "..." — the path must be wrapped in double quotes.
for (const { name } of SH_HOOKS) {
describe(`${name} — buildHookCommand output`, () => {
// ── Test 2: non-empty command on linux ─────────────────────────────────
test(`linux: returns non-empty command (fixes #2046 relative-path crash)`, () => {
const cmd = buildHookCommand(TEST_CONFIG_DIR, name, { platform: 'linux', runtime: 'claude' });
assert.ok(
block.includes('bash "') || block.includes("bash '") || block.includes('buildHookCommand'),
`${commandVar} must use buildHookCommand() (which quotes the path) or manually ` +
`quote the path. Found: ${block.slice(0, 200)}`
typeof cmd === 'string' && cmd.length > 0,
`buildHookCommand must return a non-empty string for ${name} on linux. Got: ${String(cmd)}`
);
});
test(`${commandVar} does not use bare localPrefix without quoting (fixes #2046 relative path)`, () => {
const varIdx = src.indexOf(commandVar);
assert.ok(varIdx !== -1, `${commandVar} not found in install.js`);
const blockEnd = Math.min(src.length, varIdx + 400);
const block = src.slice(varIdx, blockEnd);
// The old bad pattern was: 'bash ' + localPrefix + '/hooks/...'
// where localPrefix === '.claude' (relative, no quotes).
// The fix routes through buildHookCommand which emits bash "absolutePath".
// So the raw string '.claude/hooks' must NOT appear unquoted in this block.
const hasBareRelativePath = /bash ['"]?\.claude\/hooks/.test(block);
// ── Test 3: bash runner present on linux ───────────────────────────────
test(`linux: command starts with bash runner (fixes #2046 sh dispatch)`, () => {
const cmd = buildHookCommand(TEST_CONFIG_DIR, name, { platform: 'linux', runtime: 'claude' });
// Acceptable forms: "bash <path>", "/usr/bin/bash <path>", etc.
assert.ok(
!hasBareRelativePath,
`${commandVar} must not use a bare relative path ".claude/hooks". ` +
`Use buildHookCommand() so the path is absolute and quoted.`
/\bbash\b/.test(cmd),
`buildHookCommand must include "bash" runner for ${name} on linux. Got: ${cmd}`
);
});
// ── Test 4: absolute, double-quoted configDir on both platforms ─────────
// Uses a configDir containing a space to prove quoting is not incidental.
for (const platform of ['linux', 'win32']) {
test(`${platform}: configDir is absolute and double-quoted (fixes #2045 spaces)`, () => {
const cmd = buildHookCommand(SPACED_CONFIG_DIR, name, { platform, runtime: 'claude' });
// The configDir must appear verbatim inside double quotes in the command.
// e.g. bash "/home/first last/.claude/hooks/gsd-validate-commit.sh"
// or "/home/first last/.claude/hooks/gsd-validate-commit.sh"
assert.ok(
cmd.includes(`"${SPACED_CONFIG_DIR}`),
`buildHookCommand must embed configDir inside double quotes for ${name} on ${platform}. ` +
`Got: ${cmd}`
);
// Confirm the path is absolute (starts with / or drive letter) — not ".claude/..."
const quotedPath = cmd.match(/"([^"]+)"/)?.[1] ?? '';
assert.ok(
path.isAbsolute(quotedPath),
`The quoted path in buildHookCommand output must be absolute for ${name} on ${platform}. ` +
`Got quoted segment: "${quotedPath}" in: ${cmd}`
);
});
}
});
}
// ── Test 3: global .sh hooks must not use unquoted manual concatenation ───
test('global .sh hook commands use buildHookCommand, not unquoted string concat', () => {
// Old bad pattern for global installs:
// 'bash ' + targetDir.replace(/\\/g, '/') + '/hooks/gsd-*.sh'
// This left the absolute path unquoted, breaking paths with spaces (#2045).
// The fix routes all global .sh hooks through buildHookCommand() which
// wraps the path in double quotes: bash "/absolute/path/hooks/gsd-*.sh"
const oldGlobalPattern = /'bash ' \+ targetDir/g;
const globalMatches = src.match(oldGlobalPattern) || [];
// ── Tests 5-7: GLOBAL-install branch (isGlobal=true) for each .sh hook ─────
// The #2045 path-with-spaces bug originally lived in the global-install branch
// of hook registration. These tests call buildHookCommand with isGlobal:true
// for each .sh hook on both linux and win32 and assert:
// (a) the command is non-empty
// (b) bash is used as the runner on linux (not bare concatenation)
// (c) the configDir with spaces is wrapped in double quotes
// (d) the quoted path is absolute (not a relative ".claude/..." fragment)
//
// A regression that reintroduces bare string concatenation on the isGlobal
// branch will produce e.g. `bash /home/first last/.claude/hooks/...` (no
// quotes), which fails assertion (c) and makes these tests go RED.
assert.strictEqual(
globalMatches.length, 0,
`Found ${globalMatches.length} occurrence(s) of unquoted global .sh path construction ` +
`('bash ' + targetDir). Use buildHookCommand(targetDir, 'gsd-*.sh') instead.`
);
});
for (const { name } of SH_HOOKS) {
describe(`${name} — buildHookCommand isGlobal=true`, () => {
// ── Test 5: non-empty command on linux (global) ────────────────────────
test(`linux isGlobal: returns non-empty command`, () => {
const cmd = buildHookCommand(TEST_CONFIG_DIR, name, {
platform: 'linux', runtime: 'claude', isGlobal: true,
});
assert.ok(
typeof cmd === 'string' && cmd.length > 0,
`buildHookCommand(isGlobal=true) must return a non-empty string for ${name} on linux. Got: ${String(cmd)}`
);
});
// ── Test 4: global .sh hook commands contain double-quoted absolute paths ─
test('global .sh hook commands in source use bash with double-quoted path', () => {
// After the fix, buildHookCommand produces: bash "/abs/path/hooks/gsd-*.sh"
// Verify each hook's command variable is assigned via buildHookCommand for the global branch.
for (const { commandVar } of SH_HOOKS) {
const varIdx = src.indexOf(commandVar);
assert.ok(varIdx !== -1, `${commandVar} not found in install.js`);
// ── Test 6: bash runner present on linux (global) ─────────────────────
test(`linux isGlobal: command delegates to bash runner (not bare concatenation)`, () => {
const cmd = buildHookCommand(TEST_CONFIG_DIR, name, {
platform: 'linux', runtime: 'claude', isGlobal: true,
});
// Must contain 'bash' — bare concatenation produces "bash /path with space/..."
// which crashes the shell; the fix puts the path in quotes.
assert.ok(
/\bbash\b/.test(cmd),
`buildHookCommand(isGlobal=true) must include "bash" runner for ${name} on linux. Got: ${cmd}`
);
});
// The ternary assignment: const xCommand = isGlobal ? buildHookCommand(...) : ...
const blockEnd = Math.min(src.length, varIdx + 300);
const block = src.slice(varIdx, blockEnd);
assert.ok(
block.includes('buildHookCommand'),
`${commandVar} global branch must use buildHookCommand() to produce a quoted absolute path. ` +
`Found: ${block.slice(0, 150)}`
);
}
});
// ── Test 7: absolute, double-quoted configDir on both platforms (global) ─
// Uses SPACED_CONFIG_DIR (contains a space) to ensure the test goes RED
// when bare concatenation is reintroduced: `bash /home/first last/...`
// fails the `cmd.includes('"' + SPACED_CONFIG_DIR)` check.
for (const platform of ['linux', 'win32']) {
test(`${platform} isGlobal: configDir is absolute and double-quoted (guards #2045 global path)`, () => {
const cmd = buildHookCommand(SPACED_CONFIG_DIR, name, {
platform, runtime: 'claude', isGlobal: true,
});
assert.ok(
cmd.includes(`"${SPACED_CONFIG_DIR}`),
`buildHookCommand(isGlobal=true) must embed configDir inside double quotes for ${name} on ${platform}. ` +
`Got: ${cmd}`
);
const quotedPath = cmd.match(/"([^"]+)"/)?.[1] ?? '';
assert.ok(
path.isAbsolute(quotedPath),
`The quoted path in buildHookCommand(isGlobal=true) output must be absolute for ${name} on ${platform}. ` +
`Got quoted segment: "${quotedPath}" in: ${cmd}`
);
});
}
});
}
});