From 9d52043f50052aeb78cd2867125bc005653bfad2 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 25 Jun 2026 21:49:22 -0400 Subject: [PATCH] feat(#1733): normalize-path-in-content production AST rule + fix Windows agent-skills content leak (Phase 5) (#1736) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(#1733): normalize-path-in-content production AST rule (Phase 5) ADR-1703 Phase 5 — the first production-code rule. local/normalize-path-in-content (src/**/*.cts, @typescript-eslint/parser): flags a path-returning fn result (path.basename excluded — returns a separator-less filename) interpolated into an @-reference / config-dir markdown body without .replace(/\\/g,'/') normalization, per RULESET.CONTENT-PATH-NORMALIZATION / DEFECT.WINDOWS-PATH-LEAK-IN-MARKDOWN-CONTENT. Build-and-assess found the canonical defect site (computePathPrefix) already compliant and only 1 src/ hit — a false positive (path.basename in a status message) — eliminated by narrowing (exclude basename; require a real @-ref/ config-dir marker, not bare .md). 0 src/ violations: clean forward-prevention. The out-of-band disable-ban now scans src/**/*.cts too (typescript-estree) so the production rule also cannot be eslint-disabled. Registered (error) + PROTECTED_RULES; CONTEXT.md predicates + how-to doc updated. - RuleTester suite (26 cases) Co-Authored-By: Claude Opus 4.8 * docs(#1733): add changeset for Windows agent-skills path-leak fix Co-Authored-By: Claude Opus 4.8 * fix: harden mutation-matrix.cjs stdin read against EAGAIN on non-blocking pipe scripts/mutation-matrix.cjs read piped stdin via readFileSync(process.stdin.fd). On macOS libuv marks the stdin pipe fd non-blocking, so a synchronous read can throw EAGAIN before the writer fills the pipe — intermittently, under heavy CI shard load — aborting the script (status 2) and flaking mutation-matrix-ratchet. Replace with readStdinSync(): an fs.readSync loop that retries on EAGAIN (1ms synchronous Atomics.wait yield), stops on 0-byte/EOF, and rethrows other errors. Deterministic regression test injects EAGAIN via an fs.readSync monkeypatch. Co-Authored-By: Claude Opus 4.8 * ci: re-run golden-install-parity on src/lib + installer changes (close drift guard) golden-install-parity hashes every installed bin/lib/*.cjs per runtime, so it must re-run whenever the built lib could change. ci-test-scope selected it for neither src/** nor installer changes, so a source-only edit (e.g. #1691's milestone.cts/roadmap.cts) recompiled bin/lib and silently drifted the golden fixtures past the scoped lane. Add golden-install-parity.test.cjs to both the 'TS runtime sources' and 'installer and package layout' selection rules, with behavioral regression tests for each. Co-Authored-By: Claude Opus 4.8 --------- Co-authored-by: Claude Opus 4.8 Co-authored-by: review-bot --- .../1733-windows-agent-skills-path-leak.md | 5 + CONTEXT.md | 6 +- .../cross-platform-portability-rules.md | 16 +- eslint-rules/normalize-path-in-content.cjs | 479 ++++++++++++++++ eslint.config.mjs | 8 + scripts/ci-test-scope.cjs | 2 + scripts/mutation-matrix.cjs | 48 +- src/init.cts | 2 +- tests/ci-test-scope.test.cjs | 28 + tests/mutation-matrix-stdin-eagain.test.cjs | 93 +++ tests/normalize-path-in-content.rule.test.cjs | 541 ++++++++++++++++++ tests/portability-rule-disable-ban.test.cjs | 89 ++- 12 files changed, 1297 insertions(+), 20 deletions(-) create mode 100644 .changeset/1733-windows-agent-skills-path-leak.md create mode 100644 eslint-rules/normalize-path-in-content.cjs create mode 100644 tests/mutation-matrix-stdin-eagain.test.cjs create mode 100644 tests/normalize-path-in-content.rule.test.cjs diff --git a/.changeset/1733-windows-agent-skills-path-leak.md b/.changeset/1733-windows-agent-skills-path-leak.md new file mode 100644 index 000000000..6c40bb8bc --- /dev/null +++ b/.changeset/1733-windows-agent-skills-path-leak.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 1736 +--- +The `` block emitted by `gsd init` no longer leaks backslash paths into `@`-reference skill paths on Windows. The global skill directory (a native `path.join` result) was interpolated into the generated markdown without POSIX normalization, producing references like `@C:\…\skills\name/SKILL.md`; the reference is now normalized at the emit site so skill references use forward slashes on every platform. diff --git a/CONTEXT.md b/CONTEXT.md index 9621ee747..11db8e83e 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -770,11 +770,11 @@ The prompt-level data/instruction isolation seam for untrusted web/document ingr `DEFECT.WINDOWS-PATH-LEAK-IN-MARKDOWN-CONTENT.symptom=path.join() result on Windows (backslashes) substituted verbatim into markdown body (@-references, workflow files, generated docs); content gains mixed separators; cross-platform substring assertions fail on windows-latest CI lane only; macOS/Linux CI green so defect ships undetected` `DEFECT.WINDOWS-PATH-LEAK-IN-MARKDOWN-CONTENT.examples=PR #1622 computePathPrefix returned ${resolvedTarget}/ verbatim — rewrites of @~/.claude/gsd-core/commands/gsd/X.md wrote @C:\...\gsd-ial-windsurf-XXX\gsd-core/commands/gsd/help.md (trailing forward slashes from the original literal survived, prefix backslashes did not); tests/install-runtime-artifacts.test.cjs:318 + tests/install.test.cjs:1323 failed on windows-latest only` -`DEFECT.WINDOWS-PATH-LEAK-IN-MARKDOWN-CONTENT.detect=any function returning a filesystem path that flows into markdown/text body substitution; grep for path.join/raw resolvedTarget/${configDir}/ in code paths writing workflow .md, agent .md, or generated docs; smoke pattern is ${resolvedTarget}/ or ${configDir}/... templates that bypass normalization` +`DEFECT.WINDOWS-PATH-LEAK-IN-MARKDOWN-CONTENT.detect=any function returning a filesystem path that flows into markdown/text body substitution; grep for path.join/raw resolvedTarget/${configDir}/ in code paths writing workflow .md, agent .md, or generated docs; smoke pattern is ${resolvedTarget}/ or ${configDir}/... templates that bypass normalization; NOW enforced at write-time + CI by local/normalize-path-in-content (eslint, error, src/**/*.cts; ADR-1703 Phase 5 #1733) — flags a path-returning fn result (path.basename excluded — returns a separator-less filename) interpolated DIRECTLY into @-reference content (shape a: @~/, @$, @/) or into a template immediately followed by a /…\.md or /…\.json quasi (shape b); INDIRECT data-flow (path stored in a variable/object field then interpolated, e.g. ${entry.ref}) is NOT detected by the rule — normalize at the assignment source or at the emit site; one known indirect leak (src/init.cts cmdAgentSkills entry.ref) fixed in PR #1733 by normalizing at emit; zero opt-out (the out-of-band disable-ban scans src/**/*.cts too)` `DEFECT.WINDOWS-PATH-LEAK-IN-MARKDOWN-CONTENT.fix-forward=normalize at the SOURCE not the test: posixTarget=String(resolvedTarget).replace(/\\/g,'/'), posixHome=homeDir?String(homeDir).replace(/\\/g,'/'):homeDir; markdown body is POSIX-only; .replace(/\\/g,'/') is idempotent on POSIX (no backslashes present) so safe to apply unconditionally; isWindowsHost arg is a no-op tripwire (enh-1511) — do NOT branch on it, normalize always` -`DEFECT.WINDOWS-PATH-LEAK-IN-MARKDOWN-CONTENT.prevention=RULESET.CONTENT-PATH-NORMALIZATION; tests are downstream signal, never the fix; ref DEFECT.WINDOWS-TEST-PORTABILITY for test-side parity (normalize expected substrings too: ${configDir}/foo.replace(/\\/g,'/'))` +`DEFECT.WINDOWS-PATH-LEAK-IN-MARKDOWN-CONTENT.prevention=enforced by local/normalize-path-in-content (eslint, error; ADR-1703 Phase 5 #1733) per RULESET.CONTENT-PATH-NORMALIZATION; tests are downstream signal, never the fix; ref DEFECT.WINDOWS-TEST-PORTABILITY for test-side parity (normalize expected substrings too: ${configDir}/foo.replace(/\\/g,'/'))` -`RULESET.CONTENT-PATH-NORMALIZATION=filesystem paths substituted into markdown body text (@-references, workflow .md, agent .md, generated docs, command bodies) MUST be normalized to POSIX forward slashes via .replace(/\\/g,'/') at the production source BEFORE substitution; never push normalization to tests; cross-platform content is POSIX-only; applies to: computePathPrefix output, install-path rewrites, generated shim paths emitted into .md bodies; idempotent on POSIX so unconditional` +`RULESET.CONTENT-PATH-NORMALIZATION=filesystem paths substituted into markdown body text (@-references, workflow .md, agent .md, generated docs, command bodies) MUST be normalized to POSIX forward slashes via .replace(/\\/g,'/') at the production source BEFORE substitution; never push normalization to tests; cross-platform content is POSIX-only; applies to: computePathPrefix output, install-path rewrites, generated shim paths emitted into .md bodies; idempotent on POSIX so unconditional; mechanically enforced by local/normalize-path-in-content (eslint, src/**/*.cts; #1733)` `DEFECT.WINDOWS-PATH-LITERAL-IN-ASSERT.symptom=an assertion compares the return value of a path-returning function (resolveAgentDir, path.join, path.resolve, getPathX, computePathPrefix, etc.) to a HARDCODED forward-slash string literal like '/H/.config/opencode/agent' or 'C:/Users/...' — passes on POSIX (macOS/linux/ubuntu CI incl. gsd-test docker mirror, where path.join emits forward slashes so literal == actual), FAILS on windows-latest CI lane where path.join emits backslashes so literal != actual` `DEFECT.WINDOWS-PATH-LITERAL-IN-ASSERT.examples=PR #1692 tests/stale-bake-guard.test.cjs resolveAgentDir suite: assert.equal(resolveAgentDir('opencode',{homedir:()=>'/H'}), '/H/.config/opencode/agent') — green on macOS+ubuntu (docker gate PASS 21101/21101), red on test (windows-latest,24) + full test (windows-latest,22, shard 2/3); same root cause as DEFECT.WINDOWS-PATH-LEAK-IN-MARKDOWN-CONTENT but on the TEST side against a function return, not the production-markdown side` diff --git a/docs/contributing/cross-platform-portability-rules.md b/docs/contributing/cross-platform-portability-rules.md index 80290c134..13e2c7bda 100644 --- a/docs/contributing/cross-platform-portability-rules.md +++ b/docs/contributing/cross-platform-portability-rules.md @@ -21,8 +21,9 @@ running outside ESLint, fails the build if you try). Legitimately platform-speci | `local/no-unguarded-nonportable-exec` | A file that **both** sets a chmod exec-bit (`chmod`/`chmodSync` with `0oNNN & 0o111 !== 0`) **and** invokes `sh`/`bash` with a `-c` flag (`execFileSync`/`spawnSync`/`spawn`/`exec`/`execSync`) without a Windows platform guard — Windows Git Bash ignores the exec bit for extension-less PATH-executed scripts. | `tests/**/*.test.cjs` | | `local/no-crlf-fragile-split` | A `.split('\n')` or `.split("\n")` call on `readFileSync` content, **or** a regex literal containing a bare `\n` used against `readFileSync` content — Windows `git-autocrlf` yields `\r\n` line endings so a literal `\n` split or regex will mismatch. | `tests/**/*.test.cjs` | | `local/no-hardcoded-tmp` | A hardcoded `/tmp/` string passed as the first argument to an `fs.*` function or `path.join` — `/tmp` does not exist on Windows. Use `os.tmpdir()` instead. | `tests/**/*.test.cjs` | -| `local/no-bare-npm-exec` | An `execFileSync`/`spawnSync`/`spawn`/`exec`/`execSync` call with `"npm"` as the command and no `{ shell: true }` option (or a platform-guarded equivalent) — `npm` is a `.cmd` batch wrapper on Windows and will not be found without a shell. | `tests/**/*.test.cjs` | -| `local/require-userprofile-with-home` | A `process.env.HOME = ` assignment in a test file with no corresponding `process.env.USERPROFILE` reference anywhere in the file — Windows uses `USERPROFILE` as the home directory environment variable, not `HOME`. | `tests/**/*.test.cjs` | +| `local/no-bare-npm-exec` | An `execFileSync`/`spawnSync`/`spawn` call with `"npm"` as the command and no `{ shell: true }` option (or a platform-guarded equivalent) — `npm` is a `.cmd` batch wrapper on Windows and is not found without a shell. (`execSync`/`exec` already run via a shell, so they are not flagged.) | `tests/**/*.test.cjs` | +| `local/require-userprofile-with-home` | A `process.env.HOME = ` assignment in a test file with no corresponding `process.env.USERPROFILE` **assignment** — Windows uses `USERPROFILE` as the home directory environment variable, not `HOME`. | `tests/**/*.test.cjs` | +| `local/normalize-path-in-content` | A path-returning fn result (excluding `path.basename`, which returns a separator-less filename) interpolated **directly** into content without `.replace(/\\/g,'/')` normalization — backslash paths leak into generated content on Windows (`RULESET.CONTENT-PATH-NORMALIZATION`). Two content shapes are detected: (a) the template/string contains an `@`-reference marker (`@~/`, `@$`, `@/`), `$HOME`, or `~/`; (b) the quasi immediately following the interpolation starts with `/…\.md` or `/…\.json`. **Indirect data-flow** (path stored in a variable/field then interpolated) is not detected — normalize at source. Fix: `String(resolvedTarget).replace(/\\/g, '/')`. | `src/**/*.cts` | (See ADR-1703's catalog and [epic #1702](https://github.com/open-gsd/gsd-core/issues/1702) for the full phase history.) @@ -237,3 +238,14 @@ The rule matches by spelling and inspects the direct operand (or a `String().replace(/\\/g,'/')` before interpolating into content. + * + * ── Known boundaries ──────────────────────────────────────────────────────── + * + * (a) Name-based matching only. `path`, `os`, and the project resolver names in + * PATH_RETURNING_FNS are recognized by spelling. A local variable that + * shadows one of these names is out of scope. + * + * (b) Shallow expression inspection. Only the direct expression inside `${ }` + * (or a concatenation operand) is checked, plus one level of String() cast. + * Deeper wrapping (e.g. `.toLowerCase()` after a path call) is not detected + * as a path-returning expression and will not trigger the rule. + * + * (c) Content heuristic — two shapes are recognized: + * + * Shape (a) — quasis contain an @-reference or home-dir prefix marker: + * `@~/`, `@$`, `@/`, `$HOME`, `~/` anywhere in the template's static + * parts. A bare `@` that is NOT immediately followed by `~`, `$`, or `/` + * (e.g. an email address or attribution line) does NOT qualify. + * + * Shape (b) — per-expression: quasis[i+1].raw starts with a forward slash + * and contains `.md` or `.json` at the end of a path component. This + * catches `${computePathPrefix(t)}/commands/gsd/x.md` and + * `@${getGlobalConfigDir()}/agents/foo.md` without requiring config-dir + * markers in CONTENT_MARKERS. + * + * Config-dir substrings (e.g. `/.claude`, `/commands`, `/skills`, etc.) + * are NOT content markers — they appeared in log/error/diagnostic strings + * too often and generated false positives. Shape (b) covers the genuine + * content-emit cases without those FPs. + * + * Bare `.md`/`.json` tokens in plain prose (e.g. "see PROJECT.md") do NOT + * qualify — they carry no separator-bearing path context that could be + * tainted by backslashes. Pure log messages, filesystem paths passed to + * fs.* functions, and Error messages that lack these markers are NOT flagged. + * + * (d) Suppression by call context. A path expression inside a `fs.*` call + * argument (readFileSync, writeFileSync, join, resolve, etc.), a + * `console.*` call, `new Error(...)`, a bare `Error(...)` / `TypeError(...)` + * / `RangeError(...)` etc. (any CallExpression whose callee is an Identifier + * whose name ends in `Error`), or a `require(...)` is not flagged — these + * are real FS paths or diagnostics, not content. + * + * (e) Indirect data-flow is NOT tracked. If a path-returning call result is + * stored in a variable or object field and that variable is later + * interpolated into a content template (e.g. `${globalSkillDir}/SKILL.md` + * → `@${entry.ref}` as in src/init.cts), the rule DOES NOT detect the + * violation — it only flags direct path-returning call expressions inside + * `${ }`. Indirect content-path-leaks rely on + * RULESET.CONTENT-PATH-NORMALIZATION discipline (normalize at source) and + * code review. The one known indirect leak (src/init.cts cmdAgentSkills + * `entry.ref` building) is fixed by normalizing at the content-emit site. + * + * (f) path.basename is excluded from PATH_RETURNING_FNS for this rule. + * path.basename() returns only the final filename component — it cannot + * contain directory separators, so it is safe to interpolate into content + * without normalization. Only calls that produce separator-bearing paths + * (path.join, path.resolve, path.dirname, path.relative, path.normalize, + * os.homedir, os.tmpdir, and the project resolver functions) are flagged. + */ + +const { + PATH_RETURNING_FNS, + isPosixNormalizerCall, + unwrapString, +} = require('./lib/portability-vocab.cjs'); + +// ── Rule-local path-fn set: PATH_RETURNING_FNS minus path.basename ───────── +// +// path.basename() returns a filename with no directory separators, so it +// cannot leak backslashes into content. All other entries in PATH_RETURNING_FNS +// DO produce separator-bearing paths and ARE checked by this rule. +// +// Note: toPosixPath remains in this set intentionally (it IS a path-returning +// function), but isContentPathReturningCall is never reached for a toPosixPath +// call because isPosixNormalizerCall short-circuits first in isUnnormalizedPathExpression. +const CONTENT_PATH_FNS = new Set(PATH_RETURNING_FNS.filter(fn => fn !== 'path.basename')); + +/** + * Returns true when `node` (a CallExpression) is a call to one of the + * CONTENT_PATH_FNS entries (PATH_RETURNING_FNS minus path.basename). + * + * @param {import('eslint').Rule.Node} node + * @returns {boolean} + */ +function isContentPathReturningCall(node) { + if (!node || node.type !== 'CallExpression') return false; + const callee = node.callee; + + // Dotted call: path.join, os.homedir, etc. + if ( + callee.type === 'MemberExpression' && + !callee.computed && + callee.object.type === 'Identifier' && + callee.property.type === 'Identifier' + ) { + const dotted = `${callee.object.name}.${callee.property.name}`; + if (CONTENT_PATH_FNS.has(dotted)) return true; + } + + // Bare call: getGlobalConfigDir(), resolveKimiGlobalDir(), etc. + if (callee.type === 'Identifier') { + if (CONTENT_PATH_FNS.has(callee.name)) return true; + } + + return false; +} + +// ── Content-heuristic markers ────────────────────────────────────────────── +// +// A template/string is considered "content" (markdown @-references, workflow +// bodies, generated documentation) when its STATIC parts (quasis) contain at +// least one genuine reference marker. Two shapes are recognized: +// +// Shape (a) — explicit @-reference / home-dir prefix in quasis: +// '@~' → @~/.claude/ reference (home-dir @-reference form) +// '@$' → @${prefix}/commands/... reference (interpolated @-reference) +// '@/' → @/path/... reference (root-relative @-reference form) +// '$HOME' → $HOME/.cursor/... in generated workflow content +// '~/' → ~/. shorthand for home-dir references in content +// +// NOTE: bare '@' is deliberately excluded — it is too broad and would +// match email addresses and attribution lines (@author), causing false +// positives. Only the genuine @-reference shapes (@~, @$, @/) are matched. +// +// Shape (b) — path-returning interpolation immediately before a .md/.json +// file reference (per-expression quasi check, not template-wide): +// quasis[i+1].raw matches /^\/[^\s`]*\.(md|json)(\b|$|\/)/ — the text +// immediately following expression `i` starts with `/...path.md` or +// `/...path.json`, indicating the expression is a path prefix for a +// content file reference. This catches `${computePathPrefix(t)}/commands/ +// gsd/x.md` and `@${getGlobalConfigDir('claude')}/commands/gsd/help.md` +// without requiring config-dir markers in CONTENT_MARKERS. +// +// Deliberately excluded from CONTENT_MARKERS (were Tier 2 / Tier 3): +// Config-dir substrings (`/.claude`, `/.cursor`, `/.gemini`, `/.config`, +// etc.) and artifact-path segments (`/commands`, `/agents`, `/skills`, +// `/workflows`, `/rules`, `/gsd`) — these are too broad as standalone +// markers and generate false positives when interpolated into log/error/ +// diagnostic strings that mention config-dir paths. Shape (b) above covers +// the genuine content-emit cases without the FP risk. +// +// '@' — too broad; matches email addresses and @author attributions. +// Only the genuine @-reference prefixes (@~, @$, @/) are kept. +// '.md' — too broad; appears in plain prose ("see PROJECT.md") with no +// separator-bearing path context. +// '.json' — same rationale as '.md'. +const CONTENT_MARKERS = [ + // Shape (a) — @-reference / home-dir prefix markers + '@~', // @~/.claude/ home-dir @-reference form + '@$', // @${prefix}/... interpolated @-reference form + '@/', // @/path/... root-relative @-reference form + '$HOME', // $HOME/.cursor/ path prefix in content + '~/', // ~/. shorthand in content +]; + +// ── Shape (b): per-expression quasi marker ─────────────────────────────────── +// +// Applied per-expression in TemplateLiteral: quasis[i+1].raw must start with +// a forward slash and contain `.md` or `.json` before the next whitespace or +// end of the quasi string. This matches `/commands/gsd/x.md`, +// `/skills/foo/SKILL.md`, `/help.json`, etc. without requiring a config-dir +// marker in CONTENT_MARKERS. +// +// The check is: /^\/[^\s`]*\.(md|json)(\b|\/|$)/ against the raw quasi text. +// The `\b` / `\/` / end-of-string ensures the extension is a terminal component +// (not a `.md` substring in the middle of a word). +const QUASI_MD_JSON_RE = /^\/[^\s`]*\.(md|json)(\b|\/|$)/; + +// ── FS-call suppression: callee names that indicate a real filesystem path ─ +const FS_OBJECT_NAMES = new Set(['fs', 'path', 'os']); +const FS_METHOD_NAMES = new Set([ + 'readFileSync', 'writeFileSync', 'existsSync', 'statSync', + 'mkdirSync', 'mkdtempSync', 'readdirSync', 'unlinkSync', + 'copyFileSync', 'renameSync', 'lstatSync', 'accessSync', + 'readFile', 'writeFile', 'mkdir', 'mkdtemp', 'stat', 'access', + 'cpSync', 'rmSync', 'openSync', 'fstatSync', 'realpathSync', + 'join', 'resolve', 'dirname', 'basename', 'relative', 'normalize', + 'homedir', 'tmpdir', +]); +const FS_BARE_NAMES = new Set(['require']); +const LOG_OBJECT_NAMES = new Set(['console']); +const LOG_METHOD_NAMES = new Set(['log', 'warn', 'error', 'info', 'debug', 'trace']); + +/** + * Returns true if any quasis in the TemplateLiteral contains a content marker. + */ +function isContentTemplate(templateLiteralNode) { + const quasis = templateLiteralNode.quasis || []; + for (const quasi of quasis) { + const raw = quasi.value?.raw ?? quasi.value?.cooked ?? ''; + for (const marker of CONTENT_MARKERS) { + if (raw.includes(marker)) return true; + } + } + return false; +} + +/** + * Returns true if `node` (a CallExpression) is a context where template + * literals are real FS paths or diagnostic messages — NOT content. + * + * Checks: + * - fs.method(templateLiteral, ...) + * - path.method(templateLiteral, ...) + * - console.method(...) + * - new Error(...) + * - require(...) + */ +function isInSuppressedCallContext(expressionNode) { + const parent = expressionNode.parent; + if (!parent) return false; + + // Direct argument to a call expression + if (parent.type === 'CallExpression') { + const callee = parent.callee; + + // fs.*, path.*, os.* calls → FS paths + if ( + callee.type === 'MemberExpression' && + !callee.computed && + callee.object.type === 'Identifier' && + callee.property.type === 'Identifier' && + FS_OBJECT_NAMES.has(callee.object.name) && + FS_METHOD_NAMES.has(callee.property.name) + ) { + return true; + } + + // console.log/warn/error → diagnostic + if ( + callee.type === 'MemberExpression' && + !callee.computed && + callee.object.type === 'Identifier' && + callee.property.type === 'Identifier' && + LOG_OBJECT_NAMES.has(callee.object.name) && + LOG_METHOD_NAMES.has(callee.property.name) + ) { + return true; + } + + // require(...) → not content + if (callee.type === 'Identifier' && FS_BARE_NAMES.has(callee.name)) { + return true; + } + + // W2: bare Error(...) / TypeError(...) / RangeError(...) etc. → diagnostic. + // Handles the call-expression form (as opposed to `new Error(...)` which is + // a NewExpression). Any Identifier callee whose name ends in 'Error' is + // treated as a diagnostic constructor, not content production. + if (callee.type === 'Identifier' && callee.name.endsWith('Error')) { + return true; + } + } + + // new Error(...) → diagnostic + if (parent.type === 'NewExpression') { + const callee = parent.callee; + if (callee.type === 'Identifier' && callee.name.endsWith('Error')) { + return true; + } + } + + // throw statement containing the template → diagnostic + if (parent.type === 'ThrowStatement') { + return true; + } + + return false; +} + +/** + * Returns true if `expressionNode` (the expression inside `${ }`) is a + * content-path-returning call (PATH_RETURNING_FNS minus path.basename) that + * is NOT POSIX-normalized. + * + * Checks: + * 1. If it is a POSIX normalizer call → NOT a violation. + * 2. Unwrap String() cast → check if inner is a content path call. + * 3. Direct content path-returning call. + * + * Returns false if the expression has been POSIX-normalized. + */ +function isUnnormalizedPathExpression(exprNode) { + if (!exprNode) return false; + + // If it's already POSIX-normalized → not a violation + if (isPosixNormalizerCall(exprNode)) return false; + + // Unwrap String() cast + const inner = unwrapString(exprNode); + + // If the unwrapped inner is POSIX-normalized → not a violation + if (isPosixNormalizerCall(inner)) return false; + + // Direct content path-returning call (possibly wrapped in String()) + // Note: path.basename is excluded from CONTENT_PATH_FNS — it returns a + // filename with no directory separators, so it cannot leak backslashes. + if (isContentPathReturningCall(inner)) return true; + + return false; +} + +/** @type {import('eslint').Rule.RuleModule} */ +const rule = { + meta: { + type: 'problem', + docs: { + description: + 'Disallow path-returning calls interpolated into content (markdown/workflow) template ' + + 'literals without POSIX normalization (DEFECT.WINDOWS-PATH-LEAK-IN-MARKDOWN-CONTENT)', + category: 'Portability', + }, + schema: [], + messages: { + pathInContent: + 'Path-returning call interpolated into content template literal without POSIX normalization ' + + '(RULESET.CONTENT-PATH-NORMALIZATION). ' + + "Normalize at source: String().replace(/\\\\\\\\/g, '/') before interpolating into content. " + + 'See DEFECT.WINDOWS-PATH-LEAK-IN-MARKDOWN-CONTENT in CONTEXT.md.', + }, + }, + + create(context) { + return { + /** + * Check TemplateLiteral expressions: `...${}...` + * + * For each expression inside the template, if: + * 1. The template contains a content marker in its static parts + * 2. The expression is a path-returning call without POSIX normalization + * 3. The template is NOT in a suppressed call context (fs.*, console.*, Error) + * → report a violation. + */ + TemplateLiteral(node) { + // Check if the entire template is in a suppressed context + if (isInSuppressedCallContext(node)) return; + + const quasis = node.quasis || []; + const exprs = node.expressions || []; + + // Shape (a): any quasi contains a CONTENT_MARKERS marker → check all + // expressions in this template for unnormalized path calls. + if (isContentTemplate(node)) { + for (const expr of exprs) { + if (isUnnormalizedPathExpression(expr)) { + context.report({ node: expr, messageId: 'pathInContent' }); + } + } + return; + } + + // Shape (b): per-expression quasi check. For expression at index i, + // quasis[i+1].raw starts with a forward slash followed by a path that + // terminates in .md or .json — the expression is a path prefix being + // interpolated directly before a content file reference. This catches + // `${computePathPrefix(t)}/commands/gsd/x.md` and + // `@${getGlobalConfigDir('claude')}/commands/gsd/help.md` without + // requiring config-dir markers in CONTENT_MARKERS. + for (let i = 0; i < exprs.length; i++) { + const nextQuasi = quasis[i + 1]; + if (!nextQuasi) continue; + const raw = nextQuasi.value?.raw ?? nextQuasi.value?.cooked ?? ''; + if (QUASI_MD_JSON_RE.test(raw) && isUnnormalizedPathExpression(exprs[i])) { + context.report({ node: exprs[i], messageId: 'pathInContent' }); + } + } + }, + + /** + * Check BinaryExpression string concatenation: + "/foo.md" + * + * For `left + right` or `right + left` where one side is a string + * literal containing a content marker and the other is a path-returning + * call without POSIX normalization. + * + * W3 (right-deep FN): when a content marker is present anywhere in the + * concat tree, search the ENTIRE tree recursively for any unnormalized + * path-returning call — not just the immediate sibling. This catches + * '@~/' + (name + path.join(home, '.claude')) + * where the path call is nested inside a right-side BinaryExpression. + */ + BinaryExpression(node) { + if (node.operator !== '+') return; + + const { left, right } = node; + + // isContentString: recursively check if a node (or its concat + // sub-tree) contains a Literal/TemplateLiteral quasi with a + // content marker. Descends into nested BinaryExpression `+` chains. + function isContentString(n) { + if (!n) return false; + // Plain string literal + if (n.type === 'Literal' && typeof n.value === 'string') { + return CONTENT_MARKERS.some((m) => n.value.includes(m)); + } + // TemplateLiteral — check quasis (static parts) + if (n.type === 'TemplateLiteral') { + for (const quasi of (n.quasis || [])) { + const raw = quasi.value?.raw ?? quasi.value?.cooked ?? ''; + if (CONTENT_MARKERS.some((m) => raw.includes(m))) return true; + } + } + // Descend into nested + concatenations + if (n.type === 'BinaryExpression' && n.operator === '+') { + return isContentString(n.left) || isContentString(n.right); + } + return false; + } + + const treeHasContent = isContentString(left) || isContentString(right); + + // Suppress if the whole concatenation is in a suppressed context + if (isInSuppressedCallContext(node)) return; + + if (!treeHasContent) return; + + // Only report at the TOP-LEVEL BinaryExpression for this concat chain + // (i.e. when the parent is NOT also a `+` BinaryExpression) to avoid + // duplicate reports on every node of a chained concatenation. + const parentNode = node.parent; + if ( + parentNode && + parentNode.type === 'BinaryExpression' && + parentNode.operator === '+' + ) { + return; + } + + // Recursively scan the full concat tree for unnormalized path calls + // and report each one found. + function scanAndReport(n) { + if (!n) return; + if (n.type === 'BinaryExpression' && n.operator === '+') { + // Check left + if (isUnnormalizedPathExpression(n.left)) { + context.report({ node: n.left, messageId: 'pathInContent' }); + } else { + scanAndReport(n.left); + } + // Check right + if (isUnnormalizedPathExpression(n.right)) { + context.report({ node: n.right, messageId: 'pathInContent' }); + } else { + scanAndReport(n.right); + } + } + } + + scanAndReport(node); + }, + }; + }, +}; + +module.exports = rule; diff --git a/eslint.config.mjs b/eslint.config.mjs index 04da69e47..34d1aaf08 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -22,6 +22,7 @@ import noCrlfFragileSplit from './eslint-rules/no-crlf-fragile-split.cjs'; import noHardcodedTmp from './eslint-rules/no-hardcoded-tmp.cjs'; import noBareNpmExec from './eslint-rules/no-bare-npm-exec.cjs'; import requireUserprofileWithHome from './eslint-rules/require-userprofile-with-home.cjs'; +import normalizePathInContent from './eslint-rules/normalize-path-in-content.cjs'; const localPlugin = { rules: { @@ -38,6 +39,7 @@ const localPlugin = { 'no-hardcoded-tmp': noHardcodedTmp, 'no-bare-npm-exec': noBareNpmExec, 'require-userprofile-with-home': requireUserprofileWithHome, + 'normalize-path-in-content': normalizePathInContent, }, }; @@ -216,6 +218,12 @@ export default tseslint.config( // ADR-1372 T7: enforce use of the markdown-sectionizer seam; grandfather // pre-migration sites with // allow-adhoc-markdown: 'local/no-adhoc-markdown-parsing': 'error', + // ADR-1703 Phase 5: flag path-returning calls interpolated into content + // (markdown @-references, workflow files, generated docs) without POSIX + // normalization. Promoted to 'error' after precision review (path.basename + // excluded; content heuristic tightened to genuine reference/config-dir + // markers). See RULESET.CONTENT-PATH-NORMALIZATION in CONTEXT.md. + 'local/normalize-path-in-content': 'error', }, }, diff --git a/scripts/ci-test-scope.cjs b/scripts/ci-test-scope.cjs index fa78344b4..53a2f0cde 100644 --- a/scripts/ci-test-scope.cjs +++ b/scripts/ci-test-scope.cjs @@ -118,6 +118,7 @@ const RULES = [ tests: [ 'tests/semver-compare.test.cjs', 'tests/bug-10-semver-policy-consolidation.test.cjs', + 'tests/golden-install-parity.test.cjs', // any src/installer change can alter emitted install artifacts → re-verify golden install parity (drift guard) ], }, { @@ -134,6 +135,7 @@ const RULES = [ 'tests/install-path-detection.test.cjs', 'tests/release-tarball-smoke.install.test.cjs', 'tests/runtime-artifact-layout.test.cjs', + 'tests/golden-install-parity.test.cjs', // any src/installer change can alter emitted install artifacts → re-verify golden install parity (drift guard) ], }, { diff --git a/scripts/mutation-matrix.cjs b/scripts/mutation-matrix.cjs index 18dad2b96..2f012d493 100644 --- a/scripts/mutation-matrix.cjs +++ b/scripts/mutation-matrix.cjs @@ -30,10 +30,52 @@ */ const { execFileSync } = require('child_process'); -const { readFileSync } = require('fs'); +const fs = require('fs'); const { ExitError, runMain } = require('./lib/cli-exit.cjs'); +// ── Resilient stdin reader ──────────────────────────────────────────────────── +// On macOS, libuv sets the stdin pipe fd to non-blocking mode. A synchronous +// readFileSync(process.stdin.fd) can therefore throw EAGAIN ("resource +// temporarily unavailable") when the writer hasn't yet filled the pipe — this +// is intermittent under heavy CI shard load and causes a spurious status 2 +// exit. We work around it by calling fs.readSync in a loop and retrying on +// EAGAIN with a 1 ms synchronous pause (Atomics.wait on a fresh SharedArrayBuffer +// — no hot spin, no real-clock dependency, works under --experimental-vm-modules). +/** + * Read all of stdin synchronously, retrying on EAGAIN. + * + * @returns {string} UTF-8 decoded full stdin content. + */ +function readStdinSync() { + const BUF_SIZE = 64 * 1024; // 64 KB chunks + const buf = Buffer.allocUnsafe(BUF_SIZE); + const chunks = []; + + for (;;) { + let bytesRead; + try { + bytesRead = fs.readSync(process.stdin.fd, buf, 0, BUF_SIZE, null); + } catch (err) { + if (err.code === 'EAGAIN') { + // Non-blocking pipe not yet ready — yield for ~1 ms then retry. + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 1); + continue; + } + if (err.code === 'EOF') { + break; + } + throw err; + } + if (bytesRead === 0) { + break; // Clean EOF + } + chunks.push(Buffer.from(buf.slice(0, bytesRead))); + } + + return Buffer.concat(chunks).toString('utf8'); +} + // ── Per-module mutation score ratchet ───────────────────────────────────────── // ADR-456 / issue #1187: every covered module declares a minScore floor. // @@ -195,7 +237,7 @@ function resolveChangedFiles(args) { // When --base is absent AND stdin is not a TTY (isTTY is falsy / undefined), // read a newline-delimited file list from stdin. if (!args.base && process.stdin.isTTY !== true) { - const raw = readFileSync(process.stdin.fd, 'utf8'); + const raw = readStdinSync(); return raw.split('\n').map(l => l.trim()).filter(Boolean); } @@ -318,6 +360,6 @@ function resolveMutationBreak(raw) { // Export internals for programmatic use (tests/mutation-matrix-ratchet.test.cjs). // The require.main guard prevents main() from running when this file is require()d. -module.exports = { COVERED, TARGET_MUTATION_SCORE, resolveMutationBreak }; +module.exports = { COVERED, TARGET_MUTATION_SCORE, resolveMutationBreak, readStdinSync }; if (require.main === module) runMain(main); diff --git a/src/init.cts b/src/init.cts index 00e3da153..6c12e0a18 100644 --- a/src/init.cts +++ b/src/init.cts @@ -2183,7 +2183,7 @@ function buildAgentSkillsBlock( if (entry.kind === 'directive') { return `- Load the \`${entry.name}\` skill via the Skill tool before proceeding (plugin-provided).`; } - return `- @${entry.ref}`; + return `- @${String(entry.ref).replace(/\\/g, '/')}`; }).join('\n'); return `\nRead these user-configured skills:\n${lines}\n`; } diff --git a/tests/ci-test-scope.test.cjs b/tests/ci-test-scope.test.cjs index a41dd3318..57fd32344 100644 --- a/tests/ci-test-scope.test.cjs +++ b/tests/ci-test-scope.test.cjs @@ -547,6 +547,34 @@ describe('test-full shard matrix parity (#1212)', () => { }); }); +describe('golden-install-parity selection (#1691 drift guard)', () => { + // Regression: a src/*.cts-only edit recompiles bin/lib/*.cjs (changing installed + // hashes), but the scoped CI lane was not re-running golden-install-parity — + // causing golden fixtures to silently drift (#1691 milestone/roadmap cts change). + // Both the 'TS runtime sources' and 'installer and package layout' rules must now + // select tests/golden-install-parity.test.cjs. + + test('src/*.cts change selects golden-install-parity (TS runtime sources rule)', () => { + const result = scopeFor(['src/milestone.cts']); + assert.strictEqual(result.code_changed, true, + `expected code_changed=true for src/ change, got: ${JSON.stringify(result)}`); + assert.ok( + result.targeted_tests.includes('tests/golden-install-parity.test.cjs'), + `expected golden-install-parity in targeted_tests for src/*.cts change, got: ${JSON.stringify(result.targeted_tests)}`, + ); + }); + + test('bin/install.js change selects golden-install-parity (installer and package layout rule)', () => { + const result = scopeFor(['bin/install.js']); + assert.strictEqual(result.code_changed, true, + `expected code_changed=true for bin/ change, got: ${JSON.stringify(result)}`); + assert.ok( + result.targeted_tests.includes('tests/golden-install-parity.test.cjs'), + `expected golden-install-parity in targeted_tests for bin/install.js change, got: ${JSON.stringify(result.targeted_tests)}`, + ); + }); +}); + describe('code_changed=false implies clean output invariant', () => { // Fix 1: when code_changed is false, full_matrix, targeted_tests, windows_tests // must ALL be empty/false — even if a docs path coincidentally diff --git a/tests/mutation-matrix-stdin-eagain.test.cjs b/tests/mutation-matrix-stdin-eagain.test.cjs new file mode 100644 index 000000000..ff54797ff --- /dev/null +++ b/tests/mutation-matrix-stdin-eagain.test.cjs @@ -0,0 +1,93 @@ +'use strict'; + +/** + * tests/mutation-matrix-stdin-eagain.test.cjs + * + * Regression tests for the EAGAIN-resilient readStdinSync() helper added to + * scripts/mutation-matrix.cjs (issue #1733). + * + * Background: On macOS, libuv sets a piped stdin fd to non-blocking mode. + * Under heavy CI shard load a synchronous fs.readFileSync(process.stdin.fd) + * can throw EAGAIN before the writer has filled the pipe, aborting the script + * with status 2. readStdinSync() retries on EAGAIN; these tests verify that + * contract deterministically by monkeypatching fs.readSync (never via chmod / + * permission tricks — see cross-platform IO-failure-injection convention). + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); + +const matrix = require(path.resolve(__dirname, '../scripts/mutation-matrix.cjs')); + +describe('readStdinSync: EAGAIN resilience', () => { + test('exports readStdinSync as a function', () => { + assert.strictEqual( + typeof matrix.readStdinSync, + 'function', + 'mutation-matrix.cjs must export readStdinSync' + ); + }); + + test('retries on EAGAIN then returns the full payload', () => { + // Arrange: stub fs.readSync driven by a closure counter. + // Call 1 → throw EAGAIN (simulates non-blocking pipe not ready) + // Call 2 → write payload into buffer, return byte length + // Call 3+ → return 0 (clean EOF) + const payload = 'src/core-utils.cts\nsrc/adr-parser.cts\n'; + const payloadBuf = Buffer.from(payload, 'utf8'); + let callCount = 0; + + const origReadSync = fs.readSync; + try { + fs.readSync = (fd, buf, offset, _length, _position) => { + callCount++; + if (callCount === 1) { + throw Object.assign(new Error('EAGAIN: resource temporarily unavailable'), { code: 'EAGAIN' }); + } + if (callCount === 2) { + payloadBuf.copy(buf, offset, 0, payloadBuf.length); + return payloadBuf.length; + } + // Call 3+: EOF + return 0; + }; + + const result = matrix.readStdinSync(); + + assert.strictEqual( + result, + payload, + 'readStdinSync must return the full payload after retrying the EAGAIN' + ); + assert.ok( + callCount >= 3, + `expected at least 3 fs.readSync calls (EAGAIN + data + EOF), got ${callCount}` + ); + } finally { + fs.readSync = origReadSync; + } + }); + + test('non-EAGAIN errors propagate (are not swallowed)', () => { + // Arrange: stub fs.readSync to throw a non-retryable error. + const origReadSync = fs.readSync; + try { + fs.readSync = () => { + throw Object.assign(new Error('EACCES: permission denied'), { code: 'EACCES' }); + }; + + assert.throws( + () => matrix.readStdinSync(), + (err) => { + assert.strictEqual(err.code, 'EACCES'); + return true; + }, + 'readStdinSync must rethrow non-EAGAIN errors' + ); + } finally { + fs.readSync = origReadSync; + } + }); +}); diff --git a/tests/normalize-path-in-content.rule.test.cjs b/tests/normalize-path-in-content.rule.test.cjs new file mode 100644 index 000000000..ebfffa784 --- /dev/null +++ b/tests/normalize-path-in-content.rule.test.cjs @@ -0,0 +1,541 @@ +'use strict'; + +/** + * normalize-path-in-content.rule.test.cjs + * + * RuleTester unit tests for the local/normalize-path-in-content ESLint rule. + * + * Rule: flag a path-returning fn result (PATH_RETURNING_FNS minus path.basename) + * interpolated into a content template literal (`${ … }`) without POSIX + * normalization. "Content" is identified by two shapes: + * + * Shape (a) — quasis contain an @-reference or home-dir prefix marker: + * `@~/`, `@$`, `@/`, `$HOME`, `~/` in the template's static parts. + * A bare `@` not followed by `~`, `$`, or `/` is NOT a marker. + * + * Shape (b) — per-expression quasi check: quasis[i+1].raw starts with `/` + * and contains `.md` or `.json` at the end of a path component. Catches + * `${computePathPrefix(t)}/commands/gsd/x.md` without config-dir markers. + * + * Config-dir substrings (`/.claude`, `/commands`, `/skills`, etc.) are NOT + * content markers — removed to eliminate diagnostic/log FPs. + * + * DEFECT category: DEFECT.WINDOWS-PATH-LEAK-IN-MARKDOWN-CONTENT + * RULESET: RULESET.CONTENT-PATH-NORMALIZATION + * + * INVALID (violation expected): + * - path.join(home, '.claude') interpolated into a template containing @~/ + * - computePathPrefix(...) interpolated into a template with /commands/gsd/x.md + * (shape b: quasi immediately after starts with /…\.md) + * - getGlobalConfigDir() in @${fn()}/commands/gsd/help.md (shape b) + * + * VALID (no violation): + * - path.basename(p) in any content template — basename cannot contain separators (N1). + * - Path normalized via .replace(/\\/g, '/') before interpolation + * - Path normalized via String(...).replace(...) before interpolation + * - Path in console.log() — log message, not content + * - Path in fs.writeFileSync() first arg — real FS path, not content + * - Path in new Error() — diagnostic, not content + * - throw Error(...) — bare Error() call is a diagnostic, not content (W2) + * - Template literal with NO content markers — e.g. just a log string + * - path.basename(outputPath) in a status message with bare .md prose — N2 + * - Template with a bare `@` that is not an @-reference (@-narrowing precision) + * - path.resolve(configDir) in `${fn()}/.claude/x` — no .md/.json and no + * @-ref markers → not flagged (narrowed from Tier 2 / config-dir markers) + * - os.homedir() in `${fn()}/.claude/gsd-core/commands` — same: no .md/.json + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const { RuleTester } = require('eslint'); + +const normalizePathInContent = require('../eslint-rules/normalize-path-in-content.cjs'); + +const ruleTester = new RuleTester({ + languageOptions: { + ecmaVersion: 2022, + sourceType: 'commonjs', + }, +}); + +// ─── module shape ───────────────────────────────────────────────────────────── + +describe('normalize-path-in-content rule module', () => { + test('exports meta and create', () => { + assert.strictEqual(typeof normalizePathInContent.meta, 'object'); + assert.strictEqual(typeof normalizePathInContent.create, 'function'); + assert.strictEqual(normalizePathInContent.meta.type, 'problem'); + assert.ok(normalizePathInContent.meta.messages.pathInContent); + }); +}); + +// ─── INVALID cases (violation expected) ─────────────────────────────────────── + +describe('normalize-path-in-content invalid cases', () => { + test('invalid: path.join(home, ".claude") in @~/ home-dir reference template', () => { + ruleTester.run('normalize-path-in-content', normalizePathInContent, { + valid: [], + invalid: [ + { + // Canonical defect shape: path.join result into a markdown @~/ reference + // without normalization. On Windows, path.join emits backslashes. + // The '@~/' quasi prefix is the genuine @-reference marker (not bare '@'). + code: "const ref = `@~/${path.join(home, '.claude')}/x.md`;", + errors: [{ messageId: 'pathInContent' }], + }, + ], + }); + }); + + test('invalid: computePathPrefix result in content template containing .md reference', () => { + ruleTester.run('normalize-path-in-content', normalizePathInContent, { + valid: [], + invalid: [ + { + // computePathPrefix is a PATH_RETURNING_FNS entry; its result used + // directly in a template with a .md path reference is flagged. + code: `const body = \`\${computePathPrefix(t)}/commands/gsd/x.md\`;`, + errors: [{ messageId: 'pathInContent' }], + }, + ], + }); + }); + + test('valid (narrowed): path.resolve(configDir) in template with /.claude/x — no .md/.json after expr and no @-ref marker → NOT flagged after Tier 2 marker removal', () => { + // /.claude was a Tier 2 marker but was removed to eliminate diagnostic FPs. + // Shape (b) requires the quasi immediately after the expression to start with + // /…\.md or /…\.json — /.claude/x does NOT end in .md/.json so this is now VALID. + ruleTester.run('normalize-path-in-content', normalizePathInContent, { + valid: [ + { + code: `const prefix = \`\${path.resolve(configDir)}/.claude/x\`;`, + }, + ], + invalid: [], + }); + }); + + test('invalid: getGlobalConfigDir("claude") in @${fn()}/commands/gsd/help.md — shape (b) fires: quasi after expr starts with /commands/gsd/help.md', () => { + ruleTester.run('normalize-path-in-content', normalizePathInContent, { + valid: [], + invalid: [ + { + // Shape (b): quasi[1] = '/commands/gsd/help.md' starts with '/' and ends with '.md' + // → QUASI_MD_JSON_RE matches → path call is flagged regardless of config-dir markers. + // Note: bare '@' in quasi[0] is NOT a content marker (only @~, @$, @/ are); + // this case is detected by shape (b) alone. + code: `const ref = \`@\${getGlobalConfigDir('claude')}/commands/gsd/help.md\`;`, + errors: [{ messageId: 'pathInContent' }], + }, + ], + }); + }); + + test('valid (narrowed): os.homedir() in template /.claude/gsd-core/commands — no .md/.json and no @-ref → NOT flagged after Tier 2 marker removal', () => { + // /.claude was a Tier 2 marker but was removed to eliminate diagnostic FPs. + // Shape (b) requires /…\.md or /…\.json immediately after the expression. + // /.claude/gsd-core/commands has no .md/.json → VALID. + ruleTester.run('normalize-path-in-content', normalizePathInContent, { + valid: [ + { + code: `const body = \`\${os.homedir()}/.claude/gsd-core/commands\`;`, + }, + ], + invalid: [], + }); + }); + + test('invalid: String() wrapping without normalization is still flagged', () => { + ruleTester.run('normalize-path-in-content', normalizePathInContent, { + valid: [], + invalid: [ + { + // String() alone is not a POSIX normalizer — the replace() is still needed. + // Uses @~/ marker so the content heuristic fires (bare '@' is no longer a marker). + code: "const ref = `@~/${String(path.join(home, '.claude'))}/x.md`;", + errors: [{ messageId: 'pathInContent' }], + }, + ], + }); + }); +}); + +// ─── VALID cases (no violation) ─────────────────────────────────────────────── + +describe('normalize-path-in-content valid cases', () => { + test('valid: path normalized with .replace(/\\\\/g, "/") before interpolation', () => { + ruleTester.run('normalize-path-in-content', normalizePathInContent, { + valid: [ + { + // Correct POSIX normalization via .replace before interpolation into @~/ reference. + // Uses '@~/' marker so the content heuristic fires, then confirms no violation. + code: "const ref = `@~/${String(path.join(home, '.claude')).replace(/\\\\/g, '/')}/x.md`;", + }, + ], + invalid: [], + }); + }); + + test('valid: toPosixPath() wrapper before interpolation', () => { + ruleTester.run('normalize-path-in-content', normalizePathInContent, { + valid: [ + { + code: `const body = \`\${toPosixPath(path.resolve(configDir))}/commands/gsd/x.md\`;`, + }, + ], + invalid: [], + }); + }); + + test('valid: console.log with path — log line, not content (no content markers = no flag)', () => { + ruleTester.run('normalize-path-in-content', normalizePathInContent, { + valid: [ + { + // Log line: no @, $HOME, .md, /gsd etc. in the template quasis + code: `console.log(\`built \${path.join(a, b)}\`);`, + }, + ], + invalid: [], + }); + }); + + test('valid: fs.writeFileSync with path join as first arg — real FS path, not content', () => { + ruleTester.run('normalize-path-in-content', normalizePathInContent, { + valid: [ + { + // The ENTIRE template is an arg to fs.writeFileSync → suppressed + // as a real filesystem path argument, not markdown content + code: `fs.writeFileSync(\`\${path.join(a, b)}/foo\`, data);`, + }, + ], + invalid: [], + }); + }); + + test('valid: new Error with path — diagnostic, not content', () => { + ruleTester.run('normalize-path-in-content', normalizePathInContent, { + valid: [ + { + // Diagnostic / error message: not markdown content + code: `throw new Error(\`File not found: \${path.join(dir, file)}\`);`, + }, + ], + invalid: [], + }); + }); + + test('valid: template literal with no content markers does not flag path', () => { + ruleTester.run('normalize-path-in-content', normalizePathInContent, { + valid: [ + { + // No @, $HOME, ~/., .md, /gsd, etc. → not considered content + code: `const msg = \`Processing \${path.join(a, b)} now\`;`, + }, + ], + invalid: [], + }); + }); + + test('valid: String() + .replace() chain normalized before interpolation', () => { + ruleTester.run('normalize-path-in-content', normalizePathInContent, { + valid: [ + { + // The real fix used in runtime-artifact-conversion.cts line 2193 + code: `const posixTarget = String(resolvedTarget).replace(/\\\\/g, '/'); +const ref = \`@\${posixTarget}/.claude/x.md\`;`, + }, + ], + invalid: [], + }); + }); + + test('valid: resolveAgentDir result already POSIX-normalized via toPosixPath', () => { + ruleTester.run('normalize-path-in-content', normalizePathInContent, { + valid: [ + { + code: `const body = \`@\${toPosixPath(resolveAgentDir('opencode', { homedir: () => home }))}/agents/gsd.md\`;`, + }, + ], + invalid: [], + }); + }); + + test('valid: path used in require() — module load, not content', () => { + ruleTester.run('normalize-path-in-content', normalizePathInContent, { + valid: [ + { + // require() is a suppressed context + code: `const m = require(\`\${path.join(libDir, 'helpers')}\`);`, + }, + ], + invalid: [], + }); + }); + + // ── N1: path.basename exclusion ───────────────────────────────────────────── + // + // path.basename() returns only the final filename component (no directory + // separators), so it cannot leak backslashes into content regardless of OS. + // It is excluded from CONTENT_PATH_FNS for this rule. + + test('valid (N1): path.basename(p) in a template with @~/ reference marker — no flag because basename cannot contain separators', () => { + ruleTester.run('normalize-path-in-content', normalizePathInContent, { + valid: [ + { + // path.basename returns a filename with no directory separators; + // interpolating it into @~/ reference content is safe on all platforms. + // Uses '@~/' marker so the content heuristic fires, then confirms no + // violation because path.basename is excluded from CONTENT_PATH_FNS. + code: "const label = `@~/${path.basename(outputPath)}/x.md`;", + }, + ], + invalid: [], + }); + }); + + test('valid (N1): path.basename(p) in a template with /commands/ marker — not flagged', () => { + ruleTester.run('normalize-path-in-content', normalizePathInContent, { + valid: [ + { + code: `const ref = \`/commands/\${path.basename(outputPath)}/help.md\`;`, + }, + ], + invalid: [], + }); + }); + + // ── N2: tightened content heuristic ───────────────────────────────────────── + // + // A bare `.md` token in plain prose (no @-ref, ~/., $HOME, config-dir, or + // artifact-path segment) must NOT qualify as "content". This test mirrors + // the false-positive from src/profile-output.cts:1205. + + test('valid (N2): path.basename(outputPath) in a status-message template with bare .md prose — not flagged', () => { + ruleTester.run('normalize-path-in-content', normalizePathInContent, { + valid: [ + { + // Mirrors the src/profile-output.cts:1205 false positive. The template + // contains "PROJECT.md / REQUIREMENTS.md" (bare prose), but the quasi + // after the expression does NOT start with '/' → shape (b) does not fire. + // And no @-ref / $HOME / ~/ markers → shape (a) does not fire. + code: `const msg = \`Left existing \${path.basename(outputPath)} untouched (no GSD markers found). Broad project context lives in PROJECT.md / REQUIREMENTS.md; pass --force to overwrite.\`;`, + }, + ], + invalid: [], + }); + }); + + test('valid (N2): path.join result in prose with bare .md mention — quasi after expr does not start with / → not flagged', () => { + ruleTester.run('normalize-path-in-content', normalizePathInContent, { + valid: [ + { + // path.join IS in CONTENT_PATH_FNS, but the quasi after the expression is + // ': see README.md for details.' which does NOT start with '/' → shape (b) + // does not fire. No @-ref / $HOME / ~/ markers either. + code: `const note = \`Generated \${path.join(dir, 'output')}: see README.md for details.\`;`, + }, + ], + invalid: [], + }); + }); + + // ── N4: canonical INVALID shapes still flag after N1/N2 changes ───────────── + + test('invalid (N4 confirm): @~/${path.join(home,".claude")}/x.md — still flagged after N1/N2/@-narrowing', () => { + ruleTester.run('normalize-path-in-content', normalizePathInContent, { + valid: [], + invalid: [ + { + // @~/ prefix marker in quasis → genuine @-reference content → path.join flagged. + // Confirms that narrowing '@' to '@~'/'@$'/'@/' leaves the canonical case working. + code: "const ref = `@~/${path.join(home, '.claude')}/x.md`;", + errors: [{ messageId: 'pathInContent' }], + }, + ], + }); + }); + + test('invalid (N4 confirm): ${computePathPrefix(t)}/commands/gsd/x.md — still flagged after N1/N2 via shape (b)', () => { + ruleTester.run('normalize-path-in-content', normalizePathInContent, { + valid: [], + invalid: [ + { + // Shape (b): quasi[1] = '/commands/gsd/x.md' starts with '/' and ends with '.md' + // → QUASI_MD_JSON_RE matches → flagged. (/commands/ is no longer in CONTENT_MARKERS + // but shape (b) per-expression check catches this case.) + code: `const body = \`\${computePathPrefix(t)}/commands/gsd/x.md\`;`, + errors: [{ messageId: 'pathInContent' }], + }, + ], + }); + }); + + // ── W2: bare Error(...) / TypeError(...) suppression ───────────────────────── + // + // A bare Error(...) call (CallExpression, not NewExpression) with a path inside + // must be suppressed — it is a diagnostic, not content. + + test('valid (W2): throw Error(`...${path.join(...)}`) — Error() is diagnostic; also /.claude is no longer a content marker', () => { + ruleTester.run('normalize-path-in-content', normalizePathInContent, { + valid: [ + { + // W2: bare Error() call (CallExpression). Additionally, /.claude is no + // longer in CONTENT_MARKERS (removed to eliminate diagnostic FPs), so this + // template has no content markers at all — not flagged on either ground. + code: "throw Error(`Cannot install to /.claude: ${path.join(a, b)}`);", + }, + ], + invalid: [], + }); + }); + + test('valid (W2): throw TypeError(`...${path.join(...)}`) — TypeError() is suppressed; also no content markers', () => { + ruleTester.run('normalize-path-in-content', normalizePathInContent, { + valid: [ + { + // W2: /.claude no longer a content marker; also in an Error() call context. + code: "throw TypeError(`Bad path /.claude: ${path.join(a, b)}`);", + }, + ], + invalid: [], + }); + }); + + // ── W3: chained and right-deep concatenation with nested BinaryExpression ────── + // + // The BinaryExpression check recursively scans the FULL concat tree for both: + // (1) a content marker anywhere in the tree (via isContentString recursion) + // (2) an unnormalized path call anywhere in the non-content subtree + // (via scanAndReport recursion — the "right-deep FN" fix) + // + // Cases: + // '@~/' + name + path.join(home, '.claude') + // → parse tree: ('@~/' + name) + path.join(...) + // → outer left is BinaryExpression → isContentString descends → finds '@~/' + // → outer right is path.join → isUnnormalizedPathExpression → flagged + // + // '@~/' + (name + path.join(home, '.claude')) + // → parse tree: '@~/' + (name + path.join(...)) + // → left is '@~/' → content marker found + // → right is BinaryExpression; scanAndReport descends → finds path.join → flagged + + test('invalid (W3): "@~/" + name + path.join(home, ".claude") — chained concat flagged via recursive scan', () => { + ruleTester.run('normalize-path-in-content', normalizePathInContent, { + valid: [], + invalid: [ + { + // W3 left-deep: left side is '@~/' + name (nested BinaryExpression). + // Without recursive descent, the marker '@~/' in the inner literal + // is invisible to the outer + node and the violation is missed. + code: "const s = '@~/' + name + path.join(home, '.claude');", + errors: [{ messageId: 'pathInContent' }], + }, + ], + }); + }); + + test('invalid (W3-right-deep): "@~/" + (name + path.join(home, ".claude")) — right-deep path call flagged', () => { + ruleTester.run('normalize-path-in-content', normalizePathInContent, { + valid: [], + invalid: [ + { + // W3 right-deep: content marker is in the left literal '@~/' and the path + // call is nested inside the right-side BinaryExpression (name + path.join). + // The recursive scanAndReport must descend into the right subtree to find + // path.join and report it. + code: "const s = '@~/' + (name + path.join(home, '.claude'));", + errors: [{ messageId: 'pathInContent' }], + }, + ], + }); + }); + + test('valid (W3): "log " + path.join(a, b) — no content marker in any sub-literal → not flagged', () => { + ruleTester.run('normalize-path-in-content', normalizePathInContent, { + valid: [ + { + // No content marker anywhere in the concat tree → not flagged. + code: "const s = 'log ' + path.join(a, b);", + }, + ], + invalid: [], + }); + }); + + // ── '@' narrowing precision ──────────────────────────────────────────────────── + // + // A bare '@' that is not @~, @$, or @/ must NOT trigger the content heuristic + // (e.g. email addresses, @author attributions in comments / strings). + + test('valid (@-narrowing): bare "@" in string (e.g. email) is not an @-reference marker', () => { + ruleTester.run('normalize-path-in-content', normalizePathInContent, { + valid: [ + { + // The string contains '@' but no @~, @$, @/ and no ~/., $HOME markers. + // Bare '@' is not in CONTENT_MARKERS (only @~, @$, @/ are). + // Also: quasi after expression is ';' — no /…\.md shape (b) match. + code: "const msg = `Contact author@example.com for ${path.join(a, b)}`;", + }, + ], + invalid: [], + }); + }); + + test('valid (@-narrowing): @~/${path.join(home, ".claude")}/x.md WITH normalization — not flagged', () => { + ruleTester.run('normalize-path-in-content', normalizePathInContent, { + valid: [ + { + // Confirms that after @-narrowing, a proper @~/ reference WITH POSIX + // normalization is still correctly NOT flagged. + code: "const ref = `@~/${String(path.join(home, '.claude')).replace(/\\\\/g, '/')}/x.md`;", + }, + ], + invalid: [], + }); + }); +}); + +// ── TS-parser RuleTester ─────────────────────────────────────────────────────── +// +// Run representative fixtures through the @typescript-eslint/parser to confirm +// the rule works under the production parser (used on src/**/*.cts). This +// catches any AST-shape differences between espree and ts-estree. + +const { RuleTester: TSRuleTester } = require('eslint'); +const tsParser = require('@typescript-eslint/parser'); + +const tsRuleTester = new TSRuleTester({ + languageOptions: { + parser: tsParser, + ecmaVersion: 2022, + sourceType: 'commonjs', + }, +}); + +describe('normalize-path-in-content — @typescript-eslint/parser (TS syntax fixtures)', () => { + test('TS-parser INVALID: @~/${path.join(home as string, ".claude")}/x.md — flagged under TS parser', () => { + tsRuleTester.run('normalize-path-in-content', normalizePathInContent, { + valid: [], + invalid: [ + { + // TS-specific syntax: `home as string` type assertion inside path.join arg. + // The expression inside ${ } is still path.join (a CallExpression) → flagged. + code: "const ref = `@~/${path.join(home as string, '.claude')}/x.md`;", + errors: [{ messageId: 'pathInContent' }], + }, + ], + }); + }); + + test('TS-parser VALID: @~/${toPosixPath(path.join(home as string, ".claude"))}/x.md — not flagged', () => { + tsRuleTester.run('normalize-path-in-content', normalizePathInContent, { + valid: [ + { + // Same TS syntax but POSIX-normalized via toPosixPath — must NOT be flagged. + code: "const ref = `@~/${toPosixPath(path.join(home as string, '.claude'))}/x.md`;", + }, + ], + invalid: [], + }); + }); +}); diff --git a/tests/portability-rule-disable-ban.test.cjs b/tests/portability-rule-disable-ban.test.cjs index aee17ba98..29c6c68e1 100644 --- a/tests/portability-rule-disable-ban.test.cjs +++ b/tests/portability-rule-disable-ban.test.cjs @@ -26,6 +26,7 @@ const assert = require('node:assert/strict'); const fs = require('fs'); const path = require('path'); const espree = require('espree'); +const tsEstree = require('@typescript-eslint/typescript-estree'); const { globSync } = require('glob'); // ── Protected portability rules (grows with each ADR-1703 phase) ────────────── @@ -38,6 +39,8 @@ const PROTECTED_RULES = [ 'no-hardcoded-tmp', 'no-bare-npm-exec', 'require-userprofile-with-home', + // ADR-1703 Phase 5 rule (issue #1733) — applies to src/**/*.cts (production sources) + 'normalize-path-in-content', ]; // ── Detect disable directives via the comment text ─────────────────────────── @@ -81,14 +84,25 @@ function classifyComment(commentValue) { return null; } -// ── Collect test files ──────────────────────────────────────────────────────── +// ── Collect scanned files ───────────────────────────────────────────────────── +// +// The disable-ban covers: +// - tests/**/*.test.cjs — test sources (phase 1–4 scope) +// - src/**/*.cts — production TypeScript sources (extended in phase 5 to +// protect normalize-path-in-content, which applies to +// src/**/*.cts; an eslint-disable there would bypass the +// production rule entirely) const SELF_ABS = __filename; function collectTestFiles() { - return globSync('tests/**/*.test.cjs', { cwd: path.join(__dirname, '..') }) - .map(rel => path.join(__dirname, '..', rel)) + const root = path.join(__dirname, '..'); + const testFiles = globSync('tests/**/*.test.cjs', { cwd: root }) + .map(rel => path.join(root, rel)) .filter(absPath => absPath !== SELF_ABS); + const srcFiles = globSync('src/**/*.cts', { cwd: root }) + .map(rel => path.join(root, rel)); + return [...testFiles, ...srcFiles]; } // ── Scan ────────────────────────────────────────────────────────────────────── @@ -101,15 +115,23 @@ function scanFile(absPath) { throw new Error(`Could not read ${absPath}: ${err.message}`); } + // .cts files use TypeScript syntax — use @typescript-eslint/typescript-estree. + // .cjs files use plain JS — use espree (the original parser). + const isCts = absPath.endsWith('.cts'); + let ast; try { - ast = espree.parse(src, { - comment: true, - ecmaVersion: 2022, - loc: true, - range: true, - tolerant: true, - }); + if (isCts) { + ast = tsEstree.parse(src, { comment: true, loc: true, range: true }); + } else { + ast = espree.parse(src, { + comment: true, + ecmaVersion: 2022, + loc: true, + range: true, + tolerant: true, + }); + } } catch (parseErr) { // C5: fail CLOSED on parse error — a file that fails to parse must FAIL the // test with its path, not be silently skipped. Silent skip is a false-green: @@ -135,7 +157,7 @@ function scanFile(absPath) { // ── C5: parse-error fail-closed ─────────────────────────────────────────────── describe('C5 — scanFile fails closed on parse error', () => { - test('C5: scanFile throws on parse error instead of silently returning empty result', () => { + test('C5a: scanFile throws on parse error instead of silently returning empty result (.cjs path, espree)', () => { // Inject a parse error deterministically by monkeypatching espree.parse. // This is the cross-platform approach (works under root/Docker too). const origParse = espree.parse; @@ -154,6 +176,51 @@ describe('C5 — scanFile fails closed on parse error', () => { espree.parse = origParse; } }); + + test('W1/C5b: scanFile throws on parse error for .cts path (tsEstree path — fail-closed)', () => { + // W1: The existing C5a test only exercises the espree (.cjs) path. This test + // exercises the tsEstree (.cts) path by monkeypatching tsEstree.parse and + // pointing scanFile at a synthetic .cts-suffixed path. + // + // Cross-platform approach: monkeypatch the module method, not chmod/permissions + // (chmod 0o000 is bypassed by root in Docker and behaves differently per OS). + // + // tsEstree exports 'parse' via a configurable getter (no setter), so we use + // Object.defineProperty to inject a throwing stub, then restore the original + // descriptor in the finally block. + const tsEstreeModule = require('@typescript-eslint/typescript-estree'); + const origDescriptor = Object.getOwnPropertyDescriptor(tsEstreeModule, 'parse'); + const injected = () => { throw new SyntaxError('injected tsEstree parse error for W1/C5b test'); }; + Object.defineProperty(tsEstreeModule, 'parse', { + value: injected, + writable: true, + configurable: true, + enumerable: true, + }); + + // Also monkeypatch fs.readFileSync to return dummy content for the fake .cts + // path, so the .cts branch in scanFile runs without needing a real file. + const origReadFileSync = fs.readFileSync; + fs.readFileSync = (p, enc) => { + if (typeof p === 'string' && p.endsWith('.cts')) return '// dummy cts content'; + return origReadFileSync.call(fs, p, enc); + }; + + try { + assert.throws( + () => scanFile(path.join(__dirname, 'dummy-fixture.cts')), + (err) => { + return err instanceof Error && + err.message.includes('injected tsEstree parse error for W1/C5b test'); + }, + 'scanFile must throw on tsEstree parse error for .cts files (fail-closed)' + ); + } finally { + fs.readFileSync = origReadFileSync; + // Restore original descriptor (getter-only) + Object.defineProperty(tsEstreeModule, 'parse', origDescriptor); + } + }); }); // ── Tests ─────────────────────────────────────────────────────────────────────