diff --git a/scripts/lint-allow-test-rule-refs.allowlist.json b/scripts/lint-allow-test-rule-refs.allowlist.json index 1db215e85..9f0bbd3ec 100644 --- a/scripts/lint-allow-test-rule-refs.allowlist.json +++ b/scripts/lint-allow-test-rule-refs.allowlist.json @@ -36,7 +36,6 @@ "tests/cursor-reviewer.test.cjs :: source-text-is-the-product", "tests/debug-session-management.test.cjs :: source-text-is-the-product", "tests/discuss-checkpoint.test.cjs :: source-text-is-the-product", - "tests/discuss-mode.test.cjs :: structural-implementation-guard", "tests/discuss-phase-power.test.cjs :: source-text-is-the-product", "tests/docs-parity-live-registry.test.cjs :: source-text-is-the-product", "tests/drift-detection.test.cjs :: source-text-is-the-product", @@ -65,10 +64,8 @@ "tests/import-command.test.cjs :: source-text-is-the-product", "tests/ingest-docs.test.cjs :: source-text-is-the-product", "tests/inline-plan-threshold.test.cjs :: source-text-is-the-product", - "tests/install-minimal-hooks.test.cjs :: source-text-is-the-product", "tests/install-nested-layout.test.cjs :: source-text-is-the-product", "tests/install-runtime-artifacts.test.cjs :: source-text-is-the-product", - "tests/install.test.cjs :: source-text-is-the-product", "tests/intel.test.cjs :: source-text-is-the-product", "tests/inventory-headings-countfree.test.cjs :: source-text-is-the-product", "tests/ios-scaffold-safety.test.cjs :: source-text-is-the-product", @@ -80,7 +77,6 @@ "tests/next-safety-gates.test.cjs :: source-text-is-the-product", "tests/next-up-clear-order.test.cjs :: source-text-is-the-product", "tests/no-hardcoded-home-gsd-tools.test.cjs :: source-text-is-the-product", - "tests/opencode-permissions.test.cjs :: architectural-invariant", "tests/orphaned-hooks.test.cjs :: structural-regression-guard", "tests/package-legitimacy-gate.test.cjs :: source-text-is-the-product", "tests/parallel-dependent-plans.test.cjs :: source-text-is-the-product", diff --git a/scripts/lint-allow-test-rule-refs.ceiling.json b/scripts/lint-allow-test-rule-refs.ceiling.json index f2371f869..8b97e48ff 100644 --- a/scripts/lint-allow-test-rule-refs.ceiling.json +++ b/scripts/lint-allow-test-rule-refs.ceiling.json @@ -1,4 +1,4 @@ { - "maxFiles": 285, + "maxFiles": 278, "grace": 3 } diff --git a/tests/discuss-mode.test.cjs b/tests/discuss-mode.test.cjs index 42a212d51..846279d42 100644 --- a/tests/discuss-mode.test.cjs +++ b/tests/discuss-mode.test.cjs @@ -1,9 +1,3 @@ -// allow-test-rule: structural-implementation-guard -// init.cjs cmdInitPlanPhase must expose text_mode in its returned flags object. -// The behavioral alternative (run plan-phase init and inspect JSON output) is -// fragile across runtime variations. Structural inspection guards the contract -// until a stable behavioral API test is in place. - /** * Discuss Mode Config Tests * @@ -14,6 +8,7 @@ const { test, describe } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('fs'); const path = require('path'); +const { createTempProject, cleanup } = require('./helpers.cjs'); describe('workflow.discuss_mode config', () => { test('config template includes discuss_mode default', () => { @@ -140,13 +135,51 @@ describe('workflow.discuss_mode config', () => { assert.ok(command.includes('--text'), 'argument-hint should include --text flag'); }); - test('plan-phase init exposes text_mode in workflow flags', () => { - const initSrc = fs.readFileSync( - path.join(__dirname, '..', 'gsd-core', 'bin', 'lib', 'init.cjs'), 'utf8' + test('plan-phase init propagates config.workflow.text_mode into its result', () => { + // Behavioral replacement for a source-grep assertion (#3466): calls + // cmdInitPlanPhase directly against a real project fixture whose + // config.json sets workflow.text_mode, and asserts the value actually + // reaches the JSON result cmdInitPlanPhase emits — rather than grepping + // init.cjs's source text for the propagation line. + const { cmdInitPlanPhase } = require( + path.join(__dirname, '..', 'gsd-core', 'bin', 'lib', 'init.cjs') ); - // The cmdInitPlanPhase result object must include text_mode - const planPhaseBlock = initSrc.slice(initSrc.indexOf('function cmdInitPlanPhase')); - assert.ok(planPhaseBlock.includes('text_mode: config.text_mode'), 'init plan-phase must expose text_mode'); + const cwd = createTempProject('gsd-text-mode-propagation-'); + try { + fs.writeFileSync( + path.join(cwd, '.planning', 'config.json'), + JSON.stringify({ workflow: { text_mode: true } }, null, 2) + ); + + // cmdInitPlanPhase writes its JSON result directly to fd 1 via + // io.cjs's writeAllSync (bypasses console.log — captureConsole() + // cannot observe it). Monkeypatch fs.writeSync and restore it in a + // finally, the project's standard IO-capture seam. + const orig = fs.writeSync; + let captured = Buffer.alloc(0); + fs.writeSync = (fd, ...rest) => { + if (fd !== 1) return orig.call(fs, fd, ...rest); + const [data, offset = 0, length] = rest; + const chunk = Buffer.isBuffer(data) + ? data.subarray(offset, offset + (length ?? data.length - offset)) + : Buffer.from(String(data), 'utf8'); + captured = Buffer.concat([captured, chunk]); + return chunk.length; + }; + let result; + try { + cmdInitPlanPhase(cwd, 'does-not-exist', false, {}); + result = JSON.parse(captured.toString('utf8')); + } finally { + fs.writeSync = orig; + } + assert.strictEqual( + result.text_mode, true, + 'cmdInitPlanPhase result must propagate config.workflow.text_mode' + ); + } finally { + cleanup(cwd); + } }); test('progress workflow references discuss_mode', () => { @@ -195,7 +228,6 @@ describe('workflow.discuss_mode config', () => { { const { describe: __foldDescribe } = require('node:test'); __foldDescribe("folded:bug-2549-2550-2552-discuss-phase-context (consolidation epic #1969 B4 #1973)", () => { -// allow-test-rule: source-text-is-the-product (see #2549) // Workflow .md / agent .md / command .md / reference .md files — their text // IS what the runtime loads. Testing text content tests the deployed contract. // Per CONTRIBUTING.md exception matrix. diff --git a/tests/effort-surface-axis.test.cjs b/tests/effort-surface-axis.test.cjs index 3f847f154..76ab48e80 100644 --- a/tests/effort-surface-axis.test.cjs +++ b/tests/effort-surface-axis.test.cjs @@ -1,12 +1,8 @@ -// allow-test-rule: source-text-is-the-product (see #2481, #2615) -// The final describe block asserts on gsd-core/workflows/review.md's text. A -// workflow .md IS what the runtime loads — its literal command lines are the -// deployed contract, and there is no runtime seam that executes review.md here. -// The #2615 matrix-parity block below is the same kind of contract assertion: -// docs/reference/host-integration-capability-matrix.md IS the cited source of -// truth for every descriptor axis (ADR-1239), so asserting a shipped axis -// value appears there and matches is a contract assertion, not a source grep. -// Every other block in this file is behavioral (CLI + module surface). +// #2615 the matrix-parity block below (the file's final describe block) is a +// contract assertion, not a source grep: docs/reference/host-integration-capability-matrix.md +// IS the cited source of truth for every descriptor axis (ADR-1239), so asserting a shipped +// axis value appears there and matches is a contract assertion. Every other block in this file +// is behavioral (CLI + module surface). /** * #2481 — ADR-1239 `effortSurface` axis + ADR-443 path (a). @@ -23,6 +19,8 @@ const { describe, test } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('node:fs'); const path = require('node:path'); +const os = require('node:os'); +const cp = require('node:child_process'); const fc = require('fast-check'); const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs'); @@ -418,18 +416,95 @@ describe('#2481 — ADR-443 mechanism callers, as they actually exist', () => { }); describe('#2481 review workflow resolves effort per reviewer', () => { - test('shipped orchestration invokes resolve-execution — the grep ADR-443 said returned zero hits', () => { + test('shipped orchestration: the live claude lane genuinely receives --effort in its spawned argv', () => { // Phase 5b (#2799) moved the call out of review.md's per-lane bash and into the review-lane - // route, which resolves effort once per selected lane through the SAME surface. ADR-443's - // invariant is about shipped orchestration calling resolve-execution at all, not about which - // file it lives in — so the assertion follows the call rather than pinning the old location. - const toolsSrc = fs.readFileSync( - path.join(__dirname, '..', 'gsd-core', 'bin', 'gsd-tools.cjs'), 'utf-8', - ); - assert.ok( - toolsSrc.includes('resolve-execution'), - 'ADR-443 blocks on no shipped orchestration calling resolve-execution', - ); + // route's `effortFor()`, which SPAWNS `query resolve-execution … --pick effort_argv_string` + // once per selected lane and folds the result into that lane's argv template. A text grep for + // the string "resolve-execution" in gsd-tools.cjs would pass even if effortFor's result were + // silently dropped before reaching the spawned reviewer, or if the call were dead code. This + // drives the REAL `review-lane invoke` route end-to-end — real cp.spawnSync, a real project + // config, a real claude-shaped shim on PATH — and inspects the argv the shim actually received, + // which is the only way to prove the resolved effort reaches the invocation rather than merely + // that some file mentions the command name. + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2481-orchestration-e2e-')); + const projectDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2481-orchestration-project-')); + try { + const bin = path.join(dir, 'bin'); + const runDir = path.join(dir, 'run'); + fs.mkdirSync(bin); + fs.mkdirSync(runDir); + fs.writeFileSync(path.join(runDir, 'gsd-review-prompt.md'), 'prompt'); + + const seenArgv = path.join(dir, 'argv.txt'); + fs.writeFileSync( + path.join(bin, 'claude'), + '#!/usr/bin/env bash\n' + + 'cat >/dev/null\n' + + `printf '%s\\n' "$@" > "${seenArgv}"\n` + + 'echo "a review body long enough to clear the empty-output guard."\n', + { mode: 0o755 }, + ); + + // An extensionless file with a POSIX shebang is not executable on Windows: + // CreateProcess resolves a bare `claude` command against PATHEXT + // (.COM;.EXE;.BAT;.CMD;...), and a shebang-only file matches none of them, + // so the shim above is invisible there. Ship a second shim recognized by + // PATHEXT that writes the SAME newline-per-argv capture format the + // assertion below parses. Delegating the actual argv capture to a small + // Node script (invoked via `%*`) rather than parsing `%*` in batch avoids + // cmd.exe's fragile re-splitting of quoted/spaced arguments — Node parses + // the raw Windows command line itself, the same way the real `claude` + // binary's argv would be parsed. + if (process.platform === 'win32') { + const captureScript = path.join(bin, '_claude-capture.cjs'); + fs.writeFileSync( + captureScript, + 'const fs = require("fs");\n' + + 'process.stdin.resume();\n' + + 'process.stdin.on("end", () => {\n' + + ` fs.writeFileSync(${JSON.stringify(seenArgv)}, process.argv.slice(2).join("\\n") + "\\n");\n` + + ' console.log("a review body long enough to clear the empty-output guard.");\n' + + '});\n', + ); + fs.writeFileSync( + path.join(bin, 'claude.cmd'), + `@echo off\r\n"${process.execPath}" "${captureScript}" %*\r\n`, + ); + } + + fs.mkdirSync(path.join(projectDir, '.planning'), { recursive: true }); + fs.writeFileSync( + path.join(projectDir, '.planning', 'config.json'), + JSON.stringify({ effort: { default: 'xhigh' } }, null, 2), + ); + + const r = cp.spawnSync( + process.execPath, + [ + path.join(REPO_ROOT, 'gsd-core', 'bin', 'gsd-tools.cjs'), + 'review-lane', 'invoke', '--slug', 'claude', + '--run-dir', runDir, '--repo-root', REPO_ROOT, '--json', + ], + { + cwd: projectDir, + encoding: 'utf8', + timeout: 60000, + killSignal: 'SIGKILL', + env: { ...process.env, PATH: `${bin}${path.delimiter}${process.env.PATH}` }, + }, + ); + assert.equal(r.status, 0, `review-lane invoke failed: ${r.stderr}`); + assert.ok(fs.existsSync(seenArgv), `the claude shim never ran; stdout was: ${r.stdout}`); + + const argv = fs.readFileSync(seenArgv, 'utf8').trim().split(/\r?\n/); + assert.ok( + argv.includes('--effort') && argv.includes('xhigh'), + `resolved effort ("xhigh") did not reach the spawned claude reviewer's argv: ${JSON.stringify(argv)}`, + ); + } finally { + cleanup(dir); + cleanup(projectDir); + } }); test('each argv-effort reviewer places effort in its resolved command line', () => { diff --git a/tests/install-minimal-hooks.test.cjs b/tests/install-minimal-hooks.test.cjs index 2c31132fe..a0b7e6cf5 100644 --- a/tests/install-minimal-hooks.test.cjs +++ b/tests/install-minimal-hooks.test.cjs @@ -1,7 +1,3 @@ -// allow-test-rule: source-text-is-the-product -// Reads .md/.json/.yml product files whose deployed text IS what the -// runtime loads — testing text content tests the deployed contract. - /** * Installer Module — Sections 9–11 + 13. * @@ -1021,104 +1017,110 @@ describe('Codex legacy gsd-update-check migration', () => { * * The .sh hooks already had fs.existsSync() guards (added in #1817). This * test verifies the same defensive pattern exists for all .js hooks. + * + * Behavioral (#3466): drives the real `applySettingsJsonHooks` (the exported + * function `bin/install.js` calls at finishInstall time) against a temp + * target dir, rather than grepping install.js's source text for + * `fs.existsSync`. A missing hook file must produce NO settings.json entry + * plus a skip warning; a present hook file (positive control) must be + * registered — proving the guard discriminates per-file, not wholesale. */ 'use strict'; -const { describe, test, before } = require('node:test'); +const { describe, test, beforeEach, afterEach } = 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: settings-json hook registration moved to runtime-hooks-surface.cts. -const HOOKS_SURFACE_SRC = path.join(__dirname, '..', 'src', 'runtime-hooks-surface.cts'); +const { createTempDir, cleanup, captureConsole } = require('./helpers.cjs'); +const { applySettingsJsonHooks } = require('../bin/install.js'); const JS_HOOKS = [ - { name: 'gsd-check-update.js', registrationAnchor: 'hasGsdUpdateHook' }, - { name: 'gsd-context-monitor.js', registrationAnchor: 'hasContextMonitorHook' }, - { name: 'gsd-prompt-guard.js', registrationAnchor: 'hasPromptGuardHook' }, - { name: 'gsd-read-guard.js', registrationAnchor: 'hasReadGuardHook' }, - { name: 'gsd-workflow-guard.js', registrationAnchor: 'hasWorkflowGuardHook' }, - { name: 'gsd-worktree-path-guard.js', registrationAnchor: 'hasWorktreePathGuardHook' }, - { name: 'gsd-write-guard.js', registrationAnchor: 'hasWriteGuardHook' }, + 'gsd-check-update.js', + 'gsd-context-monitor.js', + 'gsd-prompt-guard.js', + 'gsd-read-guard.js', + 'gsd-workflow-guard.js', + 'gsd-worktree-path-guard.js', + 'gsd-write-guard.js', ]; -describe('bug #1754: .js hook registration guards', () => { - let src; - - before(() => { - // ADR-857 phase 5f-1b: hook registration 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'); - let hooksSurfaceSrc = ''; - try { hooksSurfaceSrc = fs.readFileSync(HOOKS_SURFACE_SRC, 'utf-8'); } catch { /* ok */ } - src = installSrc + '\n' + hooksSurfaceSrc; +// Drives the real guarded registration function directly (local-install +// shape: isGlobal=false routes every *Command through the supplied +// localCmd/localShellCmd, so no real node/bash-path resolution is needed). +// `presentHooks` controls which hook basenames actually exist on disk under +// targetDir/hooks/ before the call — every other referenced hook file is +// left absent, exercising the fs.existsSync guard for that hook. +function runApplySettingsJsonHooks(targetDir, presentHooks) { + fs.mkdirSync(path.join(targetDir, 'hooks'), { recursive: true }); + for (const hook of presentHooks) { + fs.writeFileSync(path.join(targetDir, 'hooks', hook), '// stub\n'); + } + const settings = {}; + const localCmd = (hookFile) => `node ${path.join(targetDir, 'hooks', hookFile)}`; + const localShellCmd = (hookFile) => `bash ${path.join(targetDir, 'hooks', hookFile)}`; + const { stdout, stderr } = captureConsole(() => { + applySettingsJsonHooks(settings, { + runtime: 'claude', + isGlobal: false, + targetDir, + postToolEvent: 'PostToolUse', + hookEvents: 'claude', + extendedHookEvents: [], + hooksSurface: 'settings-json', + updateCheckCommand: localCmd('gsd-check-update.js'), + contextMonitorCommand: localCmd('gsd-context-monitor.js'), + promptGuardCommand: localCmd('gsd-prompt-guard.js'), + readGuardCommand: localCmd('gsd-read-guard.js'), + readInjectionScannerCommand: localCmd('gsd-read-injection-scanner.js'), + configReloadCommand: null, + hookOpts: { portableHooks: false, runtime: 'claude' }, + localCmd, + localShellCmd, + }); }); + return { settings, stdout, stderr }; +} - for (const { name, registrationAnchor } of JS_HOOKS) { - describe(`${name} registration`, () => { - test(`install.js checks file existence before registering ${name}`, () => { - // Find the registration block by locating the "has...Hook" variable - const anchorIdx = src.indexOf(registrationAnchor); - assert.ok( - anchorIdx !== -1, - `${registrationAnchor} variable not found in install.js` +function settingsReferencesHook(settings, hookBaseName) { + const events = Object.values(settings.hooks || {}); + return events.some((entries) => + Array.isArray(entries) && entries.some((entry) => + Array.isArray(entry.hooks) && entry.hooks.some((h) => h.command && h.command.includes(hookBaseName)) + ) + ); +} + +describe('bug #1754: .js hook registration guards', () => { + let targetDir; + beforeEach(() => { targetDir = createTempDir('gsd-hook-guard-js-'); }); + afterEach(() => { cleanup(targetDir); }); + + for (const hookName of JS_HOOKS) { + test(`${hookName} is NOT registered in settings.json when its file is missing at the target path`, () => { + // Every OTHER JS hook is present (positive control keeps the guard + // honest — a wholesale skip of all hooks would falsely satisfy the + // negative assertion below). + const present = JS_HOOKS.filter((h) => h !== hookName); + const { settings, stderr } = runApplySettingsJsonHooks(targetDir, present); + + assert.equal( + settingsReferencesHook(settings, hookName), false, + `settings.json must NOT register ${hookName} when its file was never copied (root cause of #1754)`, + ); + assert.ok( + stderr.includes(hookName.replace('.js', '')), + `install must emit a skip warning naming ${hookName} when it is missing (stderr: ${stderr})`, + ); + + for (const otherHook of present) { + assert.equal( + settingsReferencesHook(settings, otherHook), true, + `${otherHook} (present on disk) must still be registered — the guard must be per-file, not wholesale`, ); - - // Extract a window around the registration block to find the guard - const blockStart = anchorIdx; - const blockEnd = Math.min(src.length, anchorIdx + 1200); - const block = src.slice(blockStart, blockEnd); - - // The block must contain an fs.existsSync check for the hook file - assert.ok( - block.includes('fs.existsSync') || block.includes('existsSync'), - `install.js must call fs.existsSync on the target path before registering ${name} ` + - `in settings.json. Without this guard, hooks are registered even when the .js file ` + - `was never copied (the root cause of #1754).` - ); - }); - - test(`install.js emits a warning when ${name} is missing`, () => { - // The hook file name (without extension) should appear in a warning message - const hookBaseName = name.replace('.js', ''); - const warnPattern = `Skipped`; - const anchorIdx = src.indexOf(registrationAnchor); - const block = src.slice(anchorIdx, Math.min(src.length, anchorIdx + 1200)); - - assert.ok( - block.includes(warnPattern) && block.includes(hookBaseName), - `install.js must emit a skip warning when ${name} is not found at the target path` - ); - }); + } }); } - - test('all .js hooks use the same guard pattern as .sh hooks', () => { - // Count existsSync calls in the hook registration section. - // There should be guards for all JS hooks plus the existing SH hooks. - // This test ensures new hooks added in the future follow the same pattern. - // ADR-857 phase 5f-1b: registration moved to runtime-hooks-surface.cts so scan the - // full concatenated source (install.js + runtime-hooks-surface.cts) rather than slicing. - const registrationSection = src; - - // Count unique hook file existence checks (pattern: path.join(targetDir, 'hooks', 'gsd-*.js')) - const jsGuards = (registrationSection.match(/gsd-[\w-]+\.js.*not found at target/g) || []); - const shGuards = (registrationSection.match(/gsd-[\w-]+\.sh.*not found at target/g) || []); - - assert.ok( - jsGuards.length >= JS_HOOKS.length, - `Expected at least ${JS_HOOKS.length} .js hook guards, found ${jsGuards.length}. ` + - `Every .js hook registration must check file existence before registering.` - ); - - assert.ok( - shGuards.length >= 3, - `Expected at least 3 .sh hook guards (validate-commit, session-state, phase-boundary), ` + - `found ${shGuards.length}.` - ); - }); }); }); } @@ -1142,61 +1144,95 @@ describe('bug #1754: .js hook registration guards', () => { * Defensive guard: before registering each .sh hook in settings.json, * install.js must verify the target file exists. If it doesn't, skip * registration and emit a warning. + * + * Behavioral (#3466): see the bug-1754 block above — same + * `applySettingsJsonHooks` seam, applied to the three opt-in `.sh` hooks. */ 'use strict'; -const { describe, test } = require('node:test'); +const { describe, test, beforeEach, afterEach } = 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: settings-json hook registration moved to runtime-hooks-surface.cts. -const HOOKS_SURFACE_SRC = path.join(__dirname, '..', 'src', 'runtime-hooks-surface.cts'); +const { createTempDir, cleanup, captureConsole } = require('./helpers.cjs'); +const { applySettingsJsonHooks } = require('../bin/install.js'); const SH_HOOKS = [ - { name: 'gsd-validate-commit.sh', settingsVar: 'validateCommitCommand' }, - { name: 'gsd-session-state.sh', settingsVar: 'sessionStateCommand' }, - { name: 'gsd-phase-boundary.sh', settingsVar: 'phaseBoundaryCommand' }, + 'gsd-validate-commit.sh', + 'gsd-session-state.sh', + 'gsd-phase-boundary.sh', ]; -describe('bug #1817: .sh hook registration guards', () => { - let src; - - // Read once — all tests in this suite share the same source snapshot. - // ADR-857 phase 5f-1b: hook registration moved to runtime-hooks-surface.cts. - // Concatenate both sources so structural assertions find patterns in either file. - try { - const installSrc = fs.readFileSync(INSTALL_SRC, 'utf-8'); - let hooksSurfaceSrc = ''; - try { hooksSurfaceSrc = fs.readFileSync(HOOKS_SURFACE_SRC, 'utf-8'); } catch { /* ok */ } - src = installSrc + '\n' + hooksSurfaceSrc; - } catch { - src = ''; +// Same seam as the bug-1754 block above, duplicated locally rather than +// imported across the fold boundary — each folded block is a standalone +// module scope (see the __foldDescribe wrapper), matching this file's +// existing folding convention. +function runApplySettingsJsonHooksForSh(targetDir, presentHooks) { + fs.mkdirSync(path.join(targetDir, 'hooks'), { recursive: true }); + for (const hook of presentHooks) { + fs.writeFileSync(path.join(targetDir, 'hooks', hook), '#!/bin/sh\n'); } + const settings = {}; + const localCmd = (hookFile) => `node ${path.join(targetDir, 'hooks', hookFile)}`; + const localShellCmd = (hookFile) => `bash ${path.join(targetDir, 'hooks', hookFile)}`; + const { stdout, stderr } = captureConsole(() => { + applySettingsJsonHooks(settings, { + runtime: 'claude', + isGlobal: false, + targetDir, + postToolEvent: 'PostToolUse', + hookEvents: 'claude', + extendedHookEvents: [], + hooksSurface: 'settings-json', + updateCheckCommand: localCmd('gsd-check-update.js'), + contextMonitorCommand: localCmd('gsd-context-monitor.js'), + promptGuardCommand: localCmd('gsd-prompt-guard.js'), + readGuardCommand: localCmd('gsd-read-guard.js'), + readInjectionScannerCommand: localCmd('gsd-read-injection-scanner.js'), + configReloadCommand: null, + hookOpts: { portableHooks: false, runtime: 'claude' }, + localCmd, + localShellCmd, + }); + }); + return { settings, stdout, stderr }; +} - for (const { name, settingsVar } of SH_HOOKS) { - describe(`${name} registration`, () => { - test(`install.js checks file existence before registering ${name}`, () => { - // Find the block where this .sh hook is registered. - // Each registration block is preceded by the command variable declaration - // and followed by the next hook or end of registration section. - const varIdx = src.indexOf(settingsVar); - assert.ok(varIdx !== -1, `${settingsVar} variable not found in install.js`); +function shSettingsReferencesHook(settings, hookBaseName) { + const events = Object.values(settings.hooks || {}); + return events.some((entries) => + Array.isArray(entries) && entries.some((entry) => + Array.isArray(entry.hooks) && entry.hooks.some((h) => h.command && h.command.includes(hookBaseName)) + ) + ); +} - // Extract ~900 chars around the variable to find the registration block - const blockStart = Math.max(0, varIdx - 50); - const blockEnd = Math.min(src.length, varIdx + 900); - const block = src.slice(blockStart, blockEnd); +describe('bug #1817: .sh hook registration guards', () => { + let targetDir; + beforeEach(() => { targetDir = createTempDir('gsd-hook-guard-sh-'); }); + afterEach(() => { cleanup(targetDir); }); - assert.ok( - block.includes('fs.existsSync') || block.includes('existsSync'), - `install.js must call fs.existsSync on the target path before registering ${name} in settings.json. ` + - `Without this guard, hooks are registered even when the .sh file was never copied ` + - `(the root cause of #1817).` + for (const hookName of SH_HOOKS) { + test(`${hookName} is NOT registered in settings.json when its file is missing at the target path`, () => { + const present = SH_HOOKS.filter((h) => h !== hookName); + const { settings, stderr } = runApplySettingsJsonHooksForSh(targetDir, present); + + assert.equal( + shSettingsReferencesHook(settings, hookName), false, + `settings.json must NOT register ${hookName} when its file was never copied (root cause of #1817)`, + ); + assert.ok( + stderr.includes(hookName.replace('.sh', '')), + `install must emit a skip warning naming ${hookName} when it is missing (stderr: ${stderr})`, + ); + + for (const otherHook of present) { + assert.equal( + shSettingsReferencesHook(settings, otherHook), true, + `${otherHook} (present on disk) must still be registered — the guard must be per-file, not wholesale`, ); - }); + } }); } }); diff --git a/tests/install.test.cjs b/tests/install.test.cjs index 893427fe5..8793f939b 100644 --- a/tests/install.test.cjs +++ b/tests/install.test.cjs @@ -1,6 +1,6 @@ -// allow-test-rule: source-text-is-the-product // Reads .md/.json/.yml product files whose deployed text IS what the -// runtime loads — testing text content tests the deployed contract. +// runtime loads — testing text content tests the deployed contract. (No +// .cjs/.js/.ts source-grep remains in this file — see #3466.) /** * Installer Module — Sections 1–5. @@ -736,15 +736,16 @@ describe('configureKiloPermissions', () => { }); describe('Kilo integration — install/uninstall behaviour', () => { - // Product-text reads for test 6 only — update.md and update-context.cjs - // are deployed artifacts whose text IS the runtime contract (allow-test-rule). + // update.md IS the deployed workflow contract — its literal command lines are + // what the runtime loads, and there is no runtime seam that executes update.md + // here, so this .md read stays a text assertion (does not trigger no-source-grep). const updateWorkflowSrc = fs.readFileSync( path.join(__dirname, '..', 'gsd-core', 'workflows', 'update.md'), 'utf8'); // #498: update.md's runtime/scope/config-dir resolution moved into the tested - // projection gsd-core/bin/lib/update-context.cjs. Custom-config-dir - // detection (kilo.jsonc, KILO_CONFIG) is now asserted there. - const updateContextSrc = fs.readFileSync( - path.join(__dirname, '..', 'gsd-core', 'bin', 'lib', 'update-context.cjs'), 'utf8'); + // projection gsd-core/bin/lib/update-context.cjs. Custom-config-dir detection + // (kilo.jsonc, KILO_CONFIG) is asserted behaviorally below via + // inferPreferredRuntime() itself, not via a source grep on update-context.cjs. + const { inferPreferredRuntime } = require('../gsd-core/bin/lib/update-context.cjs'); let tmpDir; let previousCwd; @@ -882,10 +883,35 @@ describe('Kilo integration — install/uninstall behaviour', () => { test('update workflow checks preferred custom config dirs', () => { // update.md still derives the preferred config dir from execution_context… assert.ok(updateWorkflowSrc.includes('PREFERRED_CONFIG_DIR')); - // …and the custom-dir detection (kilo.jsonc config marker, KILO_CONFIG env) - // now lives in the tested update-context projection (#498). - assert.ok(updateContextSrc.includes('kilo.jsonc')); - assert.ok(updateContextSrc.includes('KILO_CONFIG')); + }); + + test('inferPreferredRuntime infers "kilo" from a kilo.jsonc marker in preferredConfigDir', () => { + // Behavioural replacement for the update-context.cjs source grep (#3466): + // the custom-dir detection (kilo.jsonc config marker) lives in this exact + // projection (#498) — calling it directly, with an injected fs seam, proves + // the kilo branch actually resolves rather than merely that the string + // "kilo.jsonc" appears in the file. + const fakeFs = { + exists: (p) => String(p).endsWith('kilo.jsonc'), + }; + const runtime = inferPreferredRuntime({ + fs: fakeFs, + env: {}, + preferredConfigDir: '/fake/kilo-config-dir', + }); + assert.strictEqual(runtime, 'kilo'); + }); + + test('inferPreferredRuntime infers "kilo" from KILO_CONFIG_DIR / KILO_CONFIG env when no config-dir marker is present', () => { + const fakeFs = { exists: () => false }; + assert.strictEqual( + inferPreferredRuntime({ fs: fakeFs, env: { KILO_CONFIG_DIR: '/custom/kilo' }, preferredConfigDir: '' }), + 'kilo', + ); + assert.strictEqual( + inferPreferredRuntime({ fs: fakeFs, env: { KILO_CONFIG: '/custom/kilo/kilo.jsonc' }, preferredConfigDir: '' }), + 'kilo', + ); }); }); @@ -1156,7 +1182,6 @@ describe('readCmdNames() — tolerates missing commands/gsd directory (#1223)', }); // ─── Section N: Antigravity .agents canonical workspace dir (#791) ───────────── -// allow-test-rule: source-text-is-the-product // Reads deployed agent .md files whose text IS the product surface the // Antigravity runtime loads at startup (path references, command names). @@ -1292,7 +1317,6 @@ describe('install — --devin-desktop CLI flag routes to windsurf runtime (#792) }); }); // ─── Section N: Windsurf workflow slash-command install (#1615) ───────────── -// allow-test-rule: source-text-is-the-product // Reads deployed workflow .md files whose text IS the product surface the // Windsurf runtime loads at startup (path references, command names). @@ -5985,7 +6009,6 @@ test('install.js tier-defaults object has exactly the same keys as manifest effo { const { describe: __foldDescribe } = require('node:test'); __foldDescribe("folded:bug-1367-claude-local-flat-command-layout (consolidation epic #1969 B6 #1975)", () => { -// allow-test-rule: source-text-is-the-product #1367 // Installed command `.md` files — their on-disk path determines the slash-command // namespace registered by Claude Code. Asserting the layout (flat vs. subdirectory) // IS a behavioral test of the deploy contract, not source-grep theater. @@ -6189,8 +6212,7 @@ describe('bug #1367 — Claude local install uses flat gsd-.md command layo __gtmAfter(() => { if (__savedGsdTestMode === undefined) delete process.env.GSD_TEST_MODE; else process.env.GSD_TEST_MODE = __savedGsdTestMode; }); 'use strict'; -// allow-test-rule: source-text-is-the-product (see #2380) -// Reads .md/.json/.yml product files whose deployed text IS what the +// (see #2380) Reads .md/.json/.yml product files whose deployed text IS what the // runtime loads — testing text content tests the deployed contract. /** @@ -7453,8 +7475,7 @@ describe('#3184: scripts/lib/ and scripts/changeset/ install/uninstall parity', { const { describe: __foldDescribe } = require('node:test'); __foldDescribe("folded:issue-607-installer-dry-run (test-hygiene sweep #3336 H3 Wave 4)", () => { -// allow-test-rule: integration-test-input (#607) -// Test-created temp dirs are the only filesystem reads here — not repo source files. +// (#607) Test-created temp dirs are the only filesystem reads here — not repo source files. // This is an integration test that seeds fixture files in OS temp dirs and // asserts that the installer correctly handles --dry-run and the // cleanupLegacyGsdCc exported helper. diff --git a/tests/opencode-permissions.test.cjs b/tests/opencode-permissions.test.cjs index 55ddd3886..5b3408f76 100644 --- a/tests/opencode-permissions.test.cjs +++ b/tests/opencode-permissions.test.cjs @@ -1,9 +1,3 @@ -// allow-test-rule: architectural-invariant -// The finishInstall test asserts the call-site passes configDir (not a hardcoded -// path) — a load-bearing wiring invariant. All other tests call the exported -// configureOpencodePermissions function directly and assert on typed config state. -// Migrated from pending-migration-to-typed-ir per #455. - /** * Regression tests for OpenCode permission config handling. * @@ -22,8 +16,9 @@ const path = require('node:path'); const { createTempDir, cleanup } = require('./helpers.cjs'); const { configureOpencodePermissions } = require('../bin/install.js'); const { PACKAGE_NAME } = require('../gsd-core/bin/lib/package-identity.cjs'); - -const installSrc = fs.readFileSync(path.join(__dirname, '..', 'bin', 'install.js'), 'utf8'); +const { runNode } = require('./helpers/process-seam.cjs'); +const { INSTALL_SCRIPT, installerEnv } = require('./helpers/install-shared.cjs'); +const { INSTALL_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); const envKeys = ['OPENCODE_CONFIG_DIR', 'OPENCODE_CONFIG', 'XDG_CONFIG_HOME']; const originalEnv = Object.fromEntries(envKeys.map((key) => [key, process.env[key]])); @@ -107,10 +102,46 @@ describe('configureOpencodePermissions', () => { assert.deepEqual(config.mcp.gsd, userMcp); }); - test('finishInstall passes the actual config dir to OpenCode permissions', () => { + test('finishInstall passes the REAL resolved config dir to OpenCode permissions (not a hardcoded path)', () => { + // Behavioral replacement for a source-grep assertion (#3466): spawns the + // real installer CLI end-to-end (`bin/install.js --opencode --global + // --config-dir `) — the only code path that actually reaches + // finishInstall's `configureOpencodePermissions(isGlobal, configDir);` + // call site (the exported `install()` function alone does NOT call + // finishInstall — only the CLI/installAllRuntimes flow does, and that + // flow is also where the GSD_TEST_MODE gate on this writer is normally + // active, so a real spawn — whose env deliberately excludes + // GSD_TEST_MODE via installerEnv() — is required to exercise it) — + // then asserts the resulting opencode.json's permission paths are + // anchored on THAT exact config dir. A hardcoded or stale configDir at + // the finishInstall call site would write permission paths anchored on + // the wrong directory (or fail to find/produce opencode.json at all), + // so this can only pass if the real, per-call configDir reaches + // configureOpencodePermissions. + const result = runNode( + [INSTALL_SCRIPT, '--opencode', '--global', '--config-dir', configDir], + { env: installerEnv(), timeoutMs: INSTALL_TIMEOUT_MS }, + ); + assert.strictEqual( + result.exitCode, 0, + `installer must exit 0 for --opencode --global\nstdout: ${result.stdout}\nstderr: ${result.stderr}` + ); + + const configPath = path.join(configDir, 'opencode.json'); assert.ok( - installSrc.includes('configureOpencodePermissions(isGlobal, configDir);'), - 'OpenCode permission config uses actual install dir' + fs.existsSync(configPath), + 'finishInstall must write opencode.json into the resolved config dir' + ); + const config = JSON.parse(fs.readFileSync(configPath, 'utf8')); + const gsdPath = `${configDir.replace(/\\/g, '/')}/gsd-core/*`; + assert.strictEqual( + config.permission?.read?.[gsdPath], 'allow', + 'finishInstall must call configureOpencodePermissions with the REAL resolved configDir, ' + + `not a hardcoded path — expected a read permission entry for ${gsdPath}` + ); + assert.strictEqual( + config.permission?.external_directory?.[gsdPath], 'allow', + `expected an external_directory permission entry for ${gsdPath}` ); }); }); diff --git a/tests/repo-layout.test.cjs b/tests/repo-layout.test.cjs index 1bb3240b0..0e6ae424c 100644 --- a/tests/repo-layout.test.cjs +++ b/tests/repo-layout.test.cjs @@ -1,8 +1,4 @@ 'use strict'; -// allow-test-rule: structural-regression-guard (#1188) -// Guard-placement verification in bin/install.js requires source-text -// analysis; install.js is a non-exportable CLI script and the guard must be -// in a specific lexical scope which require()+behavior cannot verify. /** * Governance tests for the gsd-core repository root layout. @@ -23,8 +19,10 @@ const test = require('node:test'); const assert = require('node:assert/strict'); const fs = require('fs'); const path = require('path'); -const { runGit } = require('./helpers/process-seam.cjs'); -const { GIT_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); +const { runGit, runNode } = require('./helpers/process-seam.cjs'); +const { GIT_TIMEOUT_MS, INSTALL_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); +const { INSTALL_SCRIPT, installerEnv } = require('./helpers/install-shared.cjs'); +const { createTempDir, cleanup } = require('./helpers.cjs'); const ROOT = path.resolve(__dirname, '..'); @@ -53,70 +51,61 @@ test('repo-layout: root AGENTS.md is not git-tracked — no ad-hoc AI instructio ); }); -test('repo-layout: installer writes AGENTS.md only for local Copilot scope (not global), confirming the commit risk is scoped', () => { - // Verify the installer source encodes the "!isGlobal" guard that restricts - // AGENTS.md emission to local installs. If that guard were removed, the file - // could be silently created in any directory the installer runs from, - // including the repo root during development. This test is a static read of - // the install source — it does not execute the installer. - // - // Why structural rather than a fixed-window regex: a 200-char sliding-window - // regex between `if (!isGlobal)` and the assignment produces false failures - // on semantically-equivalent refactors (early-return guards, added comments, - // interposed conditions that push the tokens apart). The structural approach - // instead verifies that the agentsMdPath assignment appears INSIDE the body - // of the `if (!isGlobal)` block in the copilot-instructions surface handler - // — which is the invariant that actually matters. - const installJs = fs.readFileSync(path.join(ROOT, 'bin', 'install.js'), 'utf8'); - - // Step 1: Locate the copilot-instructions surface block. - const copilotBlockStart = installJs.indexOf("plan.installSurface === 'copilot-instructions'"); - assert.ok( - copilotBlockStart !== -1, - "bin/install.js must contain a 'copilot-instructions' surface handler; " + - "the Copilot AGENTS.md guard lives inside it.", - ); - - // Step 2: Slice to the next installSurface branch so we don't accidentally - // match tokens from a sibling surface handler. - const nextSurface = installJs.indexOf('plan.installSurface ===', copilotBlockStart + 1); - const copilotBlock = installJs.substring( - copilotBlockStart, - nextSurface > copilotBlockStart ? nextSurface : copilotBlockStart + 5000, - ); - - // Step 3: Find the `if (!isGlobal)` guard inside the copilot block. - const guardIdx = copilotBlock.indexOf('if (!isGlobal)'); - assert.ok( - guardIdx !== -1, - 'bin/install.js copilot-instructions surface handler must contain an `if (!isGlobal)` guard; ' + - 'removing that guard would allow a local Copilot install to silently create AGENTS.md ' + - 'in any working directory, including this repo checkout.', - ); - - // Step 4: Walk the brace tree to extract the body of the `if (!isGlobal)` block. - const openBrace = copilotBlock.indexOf('{', guardIdx); - assert.ok(openBrace !== -1, 'if (!isGlobal) guard must have an opening brace'); - let depth = 0; - let i = openBrace; - while (i < copilotBlock.length) { - if (copilotBlock[i] === '{') depth++; - else if (copilotBlock[i] === '}') { - depth--; - if (depth === 0) break; - } - i++; +test('repo-layout: installer writes AGENTS.md for a LOCAL Copilot install (cwd), confirming the artifact is scoped', () => { + // Behavioral replacement for a source-grep/brace-walk assertion (#3466): + // runs the REAL installer (`bin/install.js --copilot --local`) with its cwd + // pointed at an isolated temp dir, and asserts AGENTS.md is actually written + // there. This directly observes the file-output side effect the guard + // controls, rather than parsing install.js's lexical structure. + const localRoot = createTempDir('gsd-repo-layout-copilot-local-'); + try { + const result = runNode( + [INSTALL_SCRIPT, '--copilot', '--local'], + { cwd: localRoot, env: installerEnv({ HOME: localRoot, USERPROFILE: localRoot }), timeoutMs: INSTALL_TIMEOUT_MS }, + ); + assert.strictEqual( + result.exitCode, 0, + `local Copilot install must exit 0\nstdout: ${result.stdout}\nstderr: ${result.stderr}` + ); + assert.ok( + fs.existsSync(path.join(localRoot, 'AGENTS.md')), + 'a LOCAL Copilot install (issue #786) must write AGENTS.md to its cwd — ' + + 'if this ever stops happening, Copilot CLI loses its primary repo-root instructions file', + ); + } finally { + cleanup(localRoot); } - const guardBody = copilotBlock.substring(openBrace, i + 1); +}); - // Step 5: Assert the repo-root AGENTS.md write site lives inside the guard body. - assert.ok( - guardBody.includes('agentsMdPath = path.join(process.cwd()'), - 'bin/install.js must assign `agentsMdPath = path.join(process.cwd(), ...)` INSIDE the ' + - '`if (!isGlobal)` block in the copilot-instructions surface handler. ' + - 'If this assignment moves outside that block the installer would unconditionally create ' + - 'AGENTS.md in the working directory on every Copilot install, including repo-root runs.', - ); +test('repo-layout: installer does NOT write AGENTS.md for a GLOBAL Copilot install, confirming the commit risk is scoped', () => { + // Companion to the local-install test above (#3466): a GLOBAL Copilot + // install is already covered by ~/.copilot/copilot-instructions.md (per + // the comment at the `if (!isGlobal)` guard's call site in bin/install.js), + // so it must NOT also write a repo-root AGENTS.md. Both the cwd AND the + // global --config-dir are isolated temp dirs distinct from this checkout, + // so even if the guard under test were broken, this run cannot pollute the + // real repository root. + const globalCwd = createTempDir('gsd-repo-layout-copilot-global-cwd-'); + const globalConfigDir = createTempDir('gsd-repo-layout-copilot-global-config-'); + try { + const result = runNode( + [INSTALL_SCRIPT, '--copilot', '--global', '--config-dir', globalConfigDir], + { cwd: globalCwd, env: installerEnv({ HOME: globalCwd, USERPROFILE: globalCwd }), timeoutMs: INSTALL_TIMEOUT_MS }, + ); + assert.strictEqual( + result.exitCode, 0, + `global Copilot install must exit 0\nstdout: ${result.stdout}\nstderr: ${result.stderr}` + ); + assert.equal( + fs.existsSync(path.join(globalCwd, 'AGENTS.md')), false, + 'a GLOBAL Copilot install must NOT write AGENTS.md to the working directory — ' + + 'that artifact is scoped to local installs only (issue #786); a global install is ' + + 'already covered by ~/.copilot/copilot-instructions.md', + ); + } finally { + cleanup(globalCwd); + cleanup(globalConfigDir); + } }); diff --git a/tests/runtime-config-adapter-registry.test.cjs b/tests/runtime-config-adapter-registry.test.cjs index 8834e81d1..6f210b0eb 100644 --- a/tests/runtime-config-adapter-registry.test.cjs +++ b/tests/runtime-config-adapter-registry.test.cjs @@ -35,6 +35,7 @@ const { resolveRuntimeArtifactLayout } = require( path.join(ROOT, 'gsd-core', 'bin', 'lib', 'runtime-artifact-layout.cjs'), ); const { getGlobalConfigDir } = require(path.join(ROOT, 'gsd-core', 'bin', 'lib', 'runtime-homes.cjs')); +const { createTempDir, cleanup } = require('./helpers.cjs'); const sorted = (iterable) => [...iterable].sort(); @@ -492,7 +493,8 @@ describe('issue-57 AC2 — config-mutation dispatch is closed over the explicit // structural guard over bin/install.js source. Behavioral assertions // cannot observe inline `runtime === '...'` config branching, so this enforces that // every inline per-runtime branch references a runtime the adapter registry knows - // about — a NEW branch against an unregistered runtime name fails here. + // about — a NEW branch against an unregistered runtime name fails here. ESLint cannot + // currently see this grep because no-source-grep's TEXT_METHODS omits matchAll (#3464). test('every inline `runtime === "..."` branch references a registry-known runtime', () => { const src = fs.readFileSync(path.join(ROOT, 'bin', 'install.js'), 'utf8'); const literals = new Set( @@ -514,7 +516,8 @@ describe('issue-57 AC2 — config-mutation dispatch is closed over the explicit // allow-test-rule: structural-regression-guard (#2103) // VS Code is a registry runtime but is NEVER CLI-installed (Marketplace/VSIX // extension); it must stay fully descriptor-driven — bin/install.js must - // never special-case it by name. + // never special-case it by name. ESLint cannot currently see this grep because + // no-source-grep's TEXT_METHODS omits matchAll (#3464). test('#2103: bin/install.js has ZERO runtime === "vscode" / isVscode branches (vscode stays fully descriptor-driven)', () => { const src = fs.readFileSync(path.join(ROOT, 'bin', 'install.js'), 'utf8'); const runtimeComparisons = [...src.matchAll(/runtime === (?:'vscode'|"vscode")/g)]; @@ -534,19 +537,63 @@ describe('issue-57 AC2 — config-mutation dispatch is closed over the explicit ); }); - // allow-test-rule: structural-regression-guard (#3336) - // delegation-presence guard. Catches wholesale removal of the registry - // dispatch (a regression to scattered per-runtime config branching). - test('bin/install.js requires the config adapter registry and dispatches through it', () => { - const src = fs.readFileSync(path.join(ROOT, 'bin', 'install.js'), 'utf8'); - assert.ok( - src.includes('runtime-config-adapter-registry'), - 'bin/install.js no longer requires the runtime config adapter registry', - ); - assert.ok( - src.includes('resolveInstallPlan('), - 'bin/install.js no longer dispatches config through resolveInstallPlan', - ); + // Behavioral replacement for the delegation-presence source grep (#3466). + // + // The EXPECTED_TABLE / resolveInstallPlan projection-contract tests above + // (`resolveInstallPlan — descriptor-projection contract`) prove resolveInstallPlan + // ITSELF maps every registry descriptor correctly — but they call the registry + // function directly and never touch bin/install.js, so they cannot by themselves + // prove install.js actually CONSULTS it rather than reimplementing an equivalent + // per-runtime branch. This test closes that gap: it stubs the registry's + // resolveInstallPlan (the SAME module object bin/install.js requires and + // destructures) so it reports a different installSurface for every runtime, + // re-requires a fresh bin/install.js so its destructured reference picks up the + // stub, and asserts a REAL install(false, 'copilot') run STOPS producing the + // copilot-instructions.md artifact that installSurface === 'copilot-instructions' + // gates directly inside install() (bin/install.js:~12164 — no finishInstall/CLI + // orchestration layer involved, so this is reachable from install() alone). If + // install.js ever reverts to a `runtime === 'copilot'` inline branch instead of + // reading resolveInstallPlan(runtime).installSurface, the stub has no effect on + // that branch and the artifact keeps getting written — which is exactly the + // divergence this test would then catch (verified by mutating install.js to that + // exact inline form during authoring: the assertion below goes red). + test('bin/install.js dispatches config through the REAL resolveInstallPlan (stubbing it changes install() output)', () => { + const installPath = require.resolve('../bin/install.js'); + const registryPath = require.resolve('../gsd-core/bin/lib/runtime-config-adapter-registry.cjs'); + const registryModule = require(registryPath); + const originalResolveInstallPlan = registryModule.resolveInstallPlan; + + registryModule.resolveInstallPlan = (runtime) => ({ + ...originalResolveInstallPlan(runtime), + // Force every gate bin/install.js checks against + // resolveInstallPlan(runtime).installSurface to read as "nothing special for + // this runtime" — including copilot's own surface, which the real descriptor + // sets to 'copilot-instructions'. + installSurface: 'settings-json', + }); + + delete require.cache[installPath]; + const stubbedInstaller = require(installPath); + + const tmpDir = createTempDir('gsd-3466-delegation-guard-'); + const previousCwd = process.cwd(); + process.chdir(tmpDir); + try { + const result = stubbedInstaller.install(false, 'copilot'); + const instructionsPath = path.join(result.configDir, 'copilot-instructions.md'); + assert.equal( + fs.existsSync(instructionsPath), false, + 'with resolveInstallPlan stubbed to report installSurface: "settings-json" for every ' + + 'runtime, install(\'copilot\') must NOT write copilot-instructions.md — if it still ' + + 'does, bin/install.js is not actually gating on resolveInstallPlan(runtime) for this ' + + 'decision (a regression to scattered per-runtime branching)', + ); + } finally { + process.chdir(previousCwd); + cleanup(tmpDir); + registryModule.resolveInstallPlan = originalResolveInstallPlan; + delete require.cache[installPath]; + } }); }); }); diff --git a/tests/runtime-homes-descriptor-drive.test.cjs b/tests/runtime-homes-descriptor-drive.test.cjs index 12275e8a4..904bb2a4e 100644 --- a/tests/runtime-homes-descriptor-drive.test.cjs +++ b/tests/runtime-homes-descriptor-drive.test.cjs @@ -1170,10 +1170,6 @@ describe('descriptor-driven parity: 13 non-probe registry runtimes × no-env-var const { describe: __foldDescribe } = require('node:test'); __foldDescribe("folded:bug-3126-global-skills-base-runtime-path (consolidation epic #1969 B3 #1972)", () => { 'use strict'; -// allow-test-rule: structural-implementation-guard (see #3126) -// Last three tests read init.cjs source to verify delegation contract to -// runtime-homes.cjs — no behavioral IR exposed yet for this wiring point. - // Regression guard for bug #3126. // // buildAgentSkillsBlock() in init.cjs hardcoded `globalSkillsBase` to @@ -1481,39 +1477,93 @@ describe('getGlobalConfigDir — explicitDir override and opencode/kilo file-pat }); }); -describe('bug #3126: init.cjs uses runtime-homes not hardcoded .claude', () => { - test('init.cjs has no hardcoded globalSkillsBase assignment to ~/.claude/skills', () => { - const fs = require('node:fs'); - const src = fs.readFileSync( - path.join(ROOT, 'gsd-core', 'bin', 'lib', 'init.cjs'), - 'utf8', - ); - assert.ok( - !src.includes("const globalSkillsBase = path.join(os.homedir(), '.claude', 'skills')"), - 'init.cjs still assigns globalSkillsBase to hardcoded ~/.claude/skills — fix not applied', - ); +describe('bug #3126: buildAgentSkillsBlock resolves the agent-skills path per runtime (not hardcoded .claude)', () => { + // Behavioral replacement (#3466) for the three init.cjs source-grep assertions + // ("no hardcoded ~/.claude/skills assignment", "requires runtime-homes", + // "warning message no longer hardcodes ~/.claude/skills"). Those proved a + // STRING was absent/present in init.cjs's text; they would pass even if + // buildAgentSkillsBlock resolved the WRONG path for a non-claude runtime, as + // long as the literal old hardcoded expression didn't reappear verbatim. This + // drives buildAgentSkillsBlock() itself — the real exported function bug + // #3126 fixed — for two DIFFERENT runtimes with real fixture skill files + // under real per-runtime config dirs, and asserts each resolves under ITS + // OWN runtime's skills dir and never falls back to (or leaks into) the + // other's. + const fs = require('node:fs'); + const { buildAgentSkillsBlock } = require(path.join(ROOT, 'gsd-core', 'bin', 'lib', 'init.cjs')); + + /** + * Creates a temp config dir with a real `skills//SKILL.md` fixture, + * points `configDirEnvKey` at it for the duration of `fn`, and cleans up + * (including restoring the env var) afterward. + */ + function withSkillFixture(configDirEnvKey, skillName, fn) { + const tmpConfigDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3126-skills-')); + const skillDir = path.join(tmpConfigDir, 'skills', skillName); + fs.mkdirSync(skillDir, { recursive: true }); + fs.writeFileSync(path.join(skillDir, 'SKILL.md'), '# fixture skill\n'); + const saved = process.env[configDirEnvKey]; + process.env[configDirEnvKey] = tmpConfigDir; + try { + return fn(tmpConfigDir); + } finally { + if (saved === undefined) delete process.env[configDirEnvKey]; + else process.env[configDirEnvKey] = saved; + cleanup(tmpConfigDir); + } + } + + test('cursor: resolves under CURSOR_CONFIG_DIR/skills, never falls back to .claude/skills', () => { + withSkillFixture('CURSOR_CONFIG_DIR', 'gsd-executor', (tmpConfigDir) => { + const diagnostics = { warnings: [] }; + const block = buildAgentSkillsBlock( + { runtime: 'cursor', agent_skills: { 'gsd-executor': 'global:gsd-executor' } }, + 'gsd-executor', + tmpConfigDir, + diagnostics, + ); + const expectedRef = path.join(tmpConfigDir, 'skills', 'gsd-executor', 'SKILL.md').replace(/\\/g, '/'); + assert.ok(block.includes(expectedRef), `expected block to include ${expectedRef}, got: ${block}`); + assert.ok(!block.includes('.claude/skills'), `cursor resolution must not fall back to .claude/skills, got: ${block}`); + assert.deepEqual(diagnostics.warnings, [], `expected no warnings, got: ${JSON.stringify(diagnostics.warnings)}`); + }); }); - test('init.cjs requires runtime-homes', () => { - const fs = require('node:fs'); - const src = fs.readFileSync( - path.join(ROOT, 'gsd-core', 'bin', 'lib', 'init.cjs'), - 'utf8', - ); - assert.ok( - src.includes('runtime-homes'), - 'init.cjs does not require runtime-homes.cjs', - ); + + test('claude: resolves under CLAUDE_CONFIG_DIR/skills, never leaks into .cursor/skills', () => { + withSkillFixture('CLAUDE_CONFIG_DIR', 'gsd-executor', (tmpConfigDir) => { + const diagnostics = { warnings: [] }; + const block = buildAgentSkillsBlock( + { runtime: 'claude', agent_skills: { 'gsd-executor': 'global:gsd-executor' } }, + 'gsd-executor', + tmpConfigDir, + diagnostics, + ); + const expectedRef = path.join(tmpConfigDir, 'skills', 'gsd-executor', 'SKILL.md').replace(/\\/g, '/'); + assert.ok(block.includes(expectedRef), `expected block to include ${expectedRef}, got: ${block}`); + assert.ok(!block.includes('.cursor/skills'), `claude resolution must not use .cursor/skills, got: ${block}`); + assert.deepEqual(diagnostics.warnings, [], `expected no warnings, got: ${JSON.stringify(diagnostics.warnings)}`); + }); }); - test('init.cjs warning message no longer hardcodes ~/.claude/skills', () => { - const fs = require('node:fs'); - const src = fs.readFileSync( - path.join(ROOT, 'gsd-core', 'bin', 'lib', 'init.cjs'), - 'utf8', - ); - assert.ok( - !src.includes("~/.claude/skills/${skillName}/SKILL.md"), - 'init.cjs warning message still hardcodes ~/.claude/skills path', - ); + + test('per-runtime resolution: two different runtimes in the same process each resolve into THEIR OWN config dir, never the other\'s', () => { + // Proves this isn't a single special-cased runtime — cursor and claude, + // driven back-to-back, must never cross-resolve into each other's fixture dir. + withSkillFixture('CURSOR_CONFIG_DIR', 'gsd-executor', (cursorDir) => { + withSkillFixture('CLAUDE_CONFIG_DIR', 'gsd-executor', (claudeDir) => { + const cursorBlock = buildAgentSkillsBlock( + { runtime: 'cursor', agent_skills: { x: 'global:gsd-executor' } }, 'x', cursorDir, { warnings: [] }, + ); + const claudeBlock = buildAgentSkillsBlock( + { runtime: 'claude', agent_skills: { x: 'global:gsd-executor' } }, 'x', claudeDir, { warnings: [] }, + ); + const cursorPosix = cursorDir.replace(/\\/g, '/'); + const claudePosix = claudeDir.replace(/\\/g, '/'); + assert.ok(cursorBlock.includes(cursorPosix), `cursor block must reference its own config dir, got: ${cursorBlock}`); + assert.ok(!cursorBlock.includes(claudePosix), `cursor block must not reference claude's config dir, got: ${cursorBlock}`); + assert.ok(claudeBlock.includes(claudePosix), `claude block must reference its own config dir, got: ${claudeBlock}`); + assert.ok(!claudeBlock.includes(cursorPosix), `claude block must not reference cursor's config dir, got: ${claudeBlock}`); + }); + }); }); }); });