From 8a6d87538f878e3b047eb29b72a0136fe591d11d Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 14 Aug 2026 15:57:25 -0400 Subject: [PATCH] test(#3466): replace 8 source-grep assertions with behavioral tests (#3500) Phase 2 of #3464, following #3465. Rewrites every assertion that read a shipped .cjs/.js file and text-searched it, so the file no longer needs an allow-test-rule exemption. 6 of the 8 files are now marker-free; the ceiling drops 285 -> 278 against a measured 277. A source-grep passes when a STRING is present, not when the code WORKS. It survives a refactor that keeps the string but breaks the behavior, and breaks on a refactor that keeps the behavior but renames the string. Both failure modes are silent about the thing the test claims to protect. That is the anti-pattern ADR-456 and local/no-source-grep exist to prevent. Rewrites, each against the real exported seam: - discuss-mode: calls cmdInitPlanPhase() against a fixture whose config sets workflow.text_mode, asserts the value propagates to its emitted JSON. - effort-surface-axis: runs review-lane invoke against a real project with a fake `claude` PATH shim, asserts the resolved --effort actually lands in the shim's captured argv. - install-minimal-hooks: calls applySettingsJsonHooks() with a hook source missing, asserts it is neither registered nor silently registers wholesale, with sibling present hooks as the positive control. - install: calls the exported inferPreferredRuntime({fs, env, preferredConfigDir}) via its injected fs seam, asserting 'kilo' from both the config-marker and env-var paths. - opencode-permissions: spawns the real installer with a custom config dir, asserts the written opencode.json permission paths are anchored on it. - repo-layout: spawns the installer for copilot local vs global, asserting AGENTS.md is written only in the local case. - runtime-config-adapter-registry: stubs resolveInstallPlan and force-reloads install.js, asserting the runtime's artifact stops being written -- proving install.js genuinely routes through the registry. - runtime-homes-descriptor-drive: calls buildAgentSkillsBlock() for cursor and claude against real fixture SKILL.md files, asserting each runtime's refs land under its own config dir and never the other's. Every rewrite was mutation-checked before its marker was dropped. Because node --test is not runnable locally in this repo, each assertion was replicated in a standalone probe that requires the same module: run green against the real file, then red against a deliberately broken one (text_mode propagation deleted, existsSync guard removed, config dir hardcoded, !isGlobal guard dropped, kilo branch removed, effort resolution bypassed, skills base hardcoded back to .claude), then the production file restored and confirmed byte-identical. An assertion that could not be made to fail would not have shipped -- a behavioral test that passes regardless of correctness is strictly worse than the source-grep it replaces, because it looks rigorous while asserting nothing. Three claims were checked rather than trusted. All three were wrong: - repo-layout's own marker cited #1188 asserting the `!isGlobal` lexical scope was "unprovable at runtime". It is provable: the guard decides whether AGENTS.md is written, which is directly observable. Both directions verified. - The triage for runtime-config-adapter-registry claimed its source-grep was redundant with the file's EXPECTED_TABLE tests, so deletion would be safe. Those tests only exercise resolveInstallPlan() directly and never load bin/install.js, so they do not cover it. A real behavioral assertion was written instead of deleting coverage. - An earlier revision of this change dropped runtime-config-adapter-registry's two markers on the grounds that ESLint stayed silent without them. An adversarial review caught that this was wrong, and it is restored here. The file still genuinely source-greps bin/install.js at two sites; ESLint is silent only because no-source-grep's TEXT_METHODS omits matchAll. Dropping a marker because the linter cannot see the violation is exploiting the blind spot, not resolving it -- and it would go red the moment the rule is widened. Those two assertions are also irreducible: they assert that EVERY inline `runtime === '...'` branch in install.js names a registry-known runtime, and a branch naming an unregistered runtime would simply never execute, so no runtime observation can prove its absence. The markers now say so explicitly. Two coverage gaps in no-source-grep surfaced while doing this, recorded on #3464 rather than fixed here, since widening the rule is its own change with its own blast radius: - TEXT_METHODS omits matchAll, so a matchAll source-grep never trips the rule (the case above). - The path test requires a literal quoted bin/lib/gsd-core/src segment and tracks the binding one hop, so a read through a dynamic path or an intermediate variable evades it. install-minimal-hooks' genuinely load-bearing read at line 975 is itself unmarked for a related reason. install-minimal-hooks therefore keeps its markers too: its remaining real source-grep of bin/install.js (the Codex legacy gsd-update-check migration check, line 975) is outside this issue's 8 sites. It is the last blocker for that file and is a clean follow-up. Closes #3466 Co-authored-by: sim Co-authored-by: Claude Opus 5 --- .../lint-allow-test-rule-refs.allowlist.json | 4 - .../lint-allow-test-rule-refs.ceiling.json | 2 +- tests/discuss-mode.test.cjs | 58 +++- tests/effort-surface-axis.test.cjs | 115 +++++-- tests/install-minimal-hooks.test.cjs | 286 ++++++++++-------- tests/install.test.cjs | 59 ++-- tests/opencode-permissions.test.cjs | 53 +++- tests/repo-layout.test.cjs | 125 ++++---- .../runtime-config-adapter-registry.test.cjs | 77 ++++- tests/runtime-homes-descriptor-drive.test.cjs | 120 +++++--- 10 files changed, 588 insertions(+), 311 deletions(-) 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}`); + }); + }); }); }); });