diff --git a/docs/adr/1703-portability-enforcement-architecture.md b/docs/adr/1703-portability-enforcement-architecture.md index 4ae15608e..986a5c73c 100644 --- a/docs/adr/1703-portability-enforcement-architecture.md +++ b/docs/adr/1703-portability-enforcement-architecture.md @@ -212,6 +212,28 @@ as-built deviations from the Phase 0 catalog, both within this ADR's precision d The forward "how to add a portability rule" recipe delivered by this phase lives at [`docs/contributing/adding-a-portability-rule.md`](../contributing/adding-a-portability-rule.md). +Two further as-built deviations from the Phase 0 text, reconciled in the post-merge +coverage audit (#1749): + +- **Disable-ban mechanism — meta-rule → out-of-band test.** §"Strictness" specified a + `local/no-portability-disable` ESLint meta-rule to ban inline disables of portability + rules. What shipped is [`tests/portability-rule-disable-ban.test.cjs`](../../tests/portability-rule-disable-ban.test.cjs) + — a `node:test` that scans files for disable directives **outside ESLint**, so it cannot + itself be eslint-disabled (an advantage over an in-process meta-rule, which the ADR noted + as the motivating risk). The substitution is at least as strong; recorded here so the + ADR's written mechanism matches the as-built one. + +- **Drift-guard `bin/install.js` scope.** §"Architecture" said the drift guard parses + `src/runtime-homes.cts` *and the relevant `bin/install.js` exports*. The shipped + [`tests/portability-vocab-drift.test.cjs`](../../tests/portability-vocab-drift.test.cjs) + originally covered only `runtime-homes.cts`; the audit extended it to `bin/install.js` + with a SOUND shape only (a top-level function that directly `return path.*(...)` must be + registered; plus a curated two-way existence lock on the installer path helpers). The + looser body-contains heuristic used for `runtime-homes.cts` is unsound for the generated + 12k-line installer (~33 false positives), so a new installer resolver that builds a path + via a temp variable relies on review — documented as a boundary in the test. The active + resolver module (`runtime-homes.cts`) remains fully drift-guarded by the looser heuristic. + ## Alternatives considered 1. **Keep extending the regex lint.** Rejected — the adversarial review proved it is diff --git a/scripts/ci-test-scope.cjs b/scripts/ci-test-scope.cjs index 9c47399b1..d4b2ac6cf 100644 --- a/scripts/ci-test-scope.cjs +++ b/scripts/ci-test-scope.cjs @@ -238,8 +238,17 @@ const RULES = [ tests: [ 'tests/portability-rule-disable-ban.test.cjs', 'tests/portability-vocab-drift.test.cjs', - 'tests/require-fs-op-fallback.rule.test.cjs', + // All nine RuleTester suites (P1–P6) — editing any rule / the shared + // vocab+guard helpers / the eslint config re-runs the full rule family. + 'tests/no-path-literal-in-assert.rule.test.cjs', + 'tests/no-posix-mode-bit-assert.rule.test.cjs', + 'tests/no-unguarded-nonportable-exec.rule.test.cjs', + 'tests/no-crlf-fragile-split.rule.test.cjs', + 'tests/no-hardcoded-tmp.rule.test.cjs', + 'tests/no-bare-npm-exec.rule.test.cjs', + 'tests/require-userprofile-with-home.rule.test.cjs', 'tests/normalize-path-in-content.rule.test.cjs', + 'tests/require-fs-op-fallback.rule.test.cjs', ], }, ]; @@ -349,7 +358,7 @@ function classify(files) { // Determine if this file is product/pipeline code. // docs/ and root-level .md files are intentionally excluded. if ( - ['bin/', 'src/', 'gsd-core/', 'agents/', 'commands/', 'hooks/', 'tests/', 'scripts/'].some(p => file.startsWith(p)) || + ['bin/', 'src/', 'gsd-core/', 'agents/', 'commands/', 'hooks/', 'tests/', 'scripts/', 'eslint-rules/'].some(p => file.startsWith(p)) || file === 'package.json' || file === 'package-lock.json' || (file.startsWith('tsconfig') && file.endsWith('.json')) || file.startsWith('.github/rulesets/') diff --git a/tests/portability-vocab-drift.test.cjs b/tests/portability-vocab-drift.test.cjs index bf93ab1bf..d95872352 100644 --- a/tests/portability-vocab-drift.test.cjs +++ b/tests/portability-vocab-drift.test.cjs @@ -21,6 +21,7 @@ const assert = require('node:assert/strict'); const fs = require('fs'); const path = require('path'); const tsParser = require('@typescript-eslint/parser'); +const espree = require('espree'); const { PATH_RETURNING_FNS } = require('../eslint-rules/lib/portability-vocab.cjs'); @@ -33,6 +34,21 @@ const IGNORED_NON_PATH_EXPORTS = new Set([ 'detectAntigravityDirAmbiguity', ]); +// bin/install.js path-returning helpers that are registered in PATH_RETURNING_FNS. +// This is the curated set named by ADR-1703 L114-119 ("the relevant bin/install.js +// exports"). The companion test below locks it two ways: each must still be DEFINED +// in the generated installer (catches a rename/removal making the vocab entry stale), +// AND this list must equal the PATH_RETURNING_FNS bare-names that are defined in +// bin/install.js (so the curation cannot drift silently out of sync with the vocab). +const INSTALL_JS_PATH_HELPERS = [ + 'getConfigDirFromHome', + 'getGlobalDir', + 'resolveKiloConfigPath', + 'resolveOpencodeConfigPath', + 'computePathPrefix', + 'normalizeInstallRelativePath', +]; + describe('portability-vocab drift guard', () => { test('PATH_RETURNING_FNS is a non-empty array', () => { assert.ok(Array.isArray(PATH_RETURNING_FNS)); @@ -336,4 +352,134 @@ describe('portability-vocab drift guard', () => { 'mySpecifierResolver should not be in PATH_RETURNING_FNS (it is a test fixture name)', ); }); + + // ─── ADR-1703 L114-119: the drift guard also covers "the relevant bin/install.js + // exports". The generated installer is a path-heavy 12k-line CommonJS file where + // the loose body-contains heuristic (used for runtime-homes.cts) is UNSOUND — it + // produces ~33 false positives because nearly every function uses path.join. The + // two checks below use SOUND shapes only, and document the residual boundary. + test('bin/install.js: every function that DIRECTLY returns a path.* call is in PATH_RETURNING_FNS', () => { + const srcPath = path.join(__dirname, '..', 'bin', 'install.js'); + const raw = fs.readFileSync(srcPath, 'utf8'); + // bin/install.js starts with a shebang espree cannot parse — rewrite #! -> //. + const src = raw.startsWith('#!') ? '//' + raw.slice(2) : raw; + const ast = espree.parse(src, { ecmaVersion: 2022, loc: true, range: true, tolerant: true }); + + // SOUND shape: a top-level function whose body contains a ReturnStatement + // whose argument is a CallExpression to path.join/resolve/dirname/normalize/ + // relative. This is tight (0 false positives on the current installer) and + // catches the canonical resolver shape (resolveOpencodeConfigPath, + // resolveKiloConfigPath). It does NOT catch a resolver that builds a path + // into a temp variable then returns the temp — see the boundary note below. + function returnsPathCall(funcBody) { + let found = false; + function walk(n) { + if (found || !n || typeof n !== 'object') return; + if (n.type === 'ReturnStatement' && n.argument && n.argument.type === 'CallExpression') { + const callee = n.argument.callee; + if ( + callee.type === 'MemberExpression' && + !callee.computed && + callee.object.type === 'Identifier' && + callee.object.name === 'path' && + callee.property.type === 'Identifier' && + ['join', 'resolve', 'dirname', 'normalize', 'relative', 'basename'].includes(callee.property.name) + ) { + found = true; + return; + } + } + // Do NOT descend into nested function expressions/declarations — a path + // return inside a nested callback does not make the outer fn path-returning. + if (n.type === 'FunctionExpression' || n.type === 'ArrowFunctionExpression' || n.type === 'FunctionDeclaration') return; + for (const key of Object.keys(n)) { + if (key === 'parent' || key === 'tokens' || key === 'comments') continue; + const child = n[key]; + if (Array.isArray(child)) { + for (const item of child) { + if (item && typeof item === 'object' && item.type) walk(item); + } + } else if (child && typeof child === 'object' && child.type) { + walk(child); + } + } + } + walk(funcBody); + return found; + } + + const pathReturning = []; + for (const node of ast.body) { + if (node.type === 'FunctionDeclaration' && node.id && node.body) { + if (returnsPathCall(node.body)) pathReturning.push(node.id.name); + } + } + + const vocabSet = new Set(PATH_RETURNING_FNS); + const missing = pathReturning.filter((n) => !vocabSet.has(n)); + assert.deepStrictEqual( + missing, + [], + `These bin/install.js functions return a path.* call but are missing from PATH_RETURNING_FNS:\n ${missing.join('\n ')}\n\nRegister them in eslint-rules/lib/portability-vocab.cjs (or, if they do not return a string path that flows into content/assertions, document why).`, + ); + }); + + test('bin/install.js: the curated INSTALL_JS_PATH_HELPERS still exist (no stale vocab entries after a rename)', () => { + const srcPath = path.join(__dirname, '..', 'bin', 'install.js'); + const raw = fs.readFileSync(srcPath, 'utf8'); + const src = raw.startsWith('#!') ? '//' + raw.slice(2) : raw; + const ast = espree.parse(src, { ecmaVersion: 2022, loc: true, range: true, tolerant: true }); + + // Collect every top-level function-declaration AND const/let/var name defined + // in the installer (path helpers may be `function foo(){}` OR a const import + // like `const computePathPrefix = runtimeArtifactConversion._computePathPrefix`). + const defined = new Set(); + for (const node of ast.body) { + if (node.type === 'FunctionDeclaration' && node.id) defined.add(node.id.name); + if (node.type === 'VariableDeclaration') { + for (const decl of node.declarations) { + if (decl.type === 'VariableDeclarator' && decl.id && decl.id.type === 'Identifier') { + defined.add(decl.id.name); + } + } + } + } + + // (a) Each curated helper must still be defined — catches a rename/removal + // that would leave a stale entry in PATH_RETURNING_FNS (the rule would + // silently stop matching the renamed resolver). + const missing = INSTALL_JS_PATH_HELPERS.filter((n) => !defined.has(n)); + assert.deepStrictEqual( + missing, + [], + `These curated bin/install.js path helpers are no longer defined in bin/install.js — their PATH_RETURNING_FNS entries are now stale:\n ${missing.join('\n ')}\n\nRename them in eslint-rules/lib/portability-vocab.cjs PATH_RETURNING_FNS and in INSTALL_JS_PATH_HELPERS here.`, + ); + + // (b) The curated list must equal the PATH_RETURNING_FNS bare-names that are + // defined in bin/install.js — so the curation cannot drift out of sync + // with the vocab. If a new install.js helper is added to PATH_RETURNING_FNS, + // it must also be added to INSTALL_JS_PATH_HELPERS (and vice versa). + const vocabSet = new Set(PATH_RETURNING_FNS); + const vocabHelpersDefinedInInstallJs = [...vocabSet].filter((n) => defined.has(n) && !n.includes('.')).sort(); + assert.deepStrictEqual( + [...INSTALL_JS_PATH_HELPERS].sort(), + vocabHelpersDefinedInInstallJs, + `INSTALL_JS_PATH_HELPERS is out of sync with PATH_RETURNING_FNS entries defined in bin/install.js.\n` + + ` curated list: [${[...INSTALL_JS_PATH_HELPERS].sort().join(', ')}]\n` + + ` vocab ∩ install.js: [${vocabHelpersDefinedInInstallJs.join(', ')}]\n` + + `Reconcile the two lists.`, + ); + }); }); + +// ─── Known boundary (documented, not enforced) ───────────────────────────────── +// +// A NEW path-returning resolver added to bin/install.js that builds its path via +// a temp variable (`const p = path.join(...); return p;`) or by delegating to +// another helper (`return getGlobalDir(...)`) is NOT caught by the tight +// return-path.* check above (which requires `return path.join(...)` directly). +// The looser body-contains heuristic is unsound here (33 FPs on the current +// installer). The installer's path API is a small, stable, curated set; a new +// resolver there is caught at code review (the active resolver module, +// src/runtime-homes.cts, IS fully drift-guarded by the looser heuristic above, +// which is sound for that focused module).