diff --git a/package.json b/package.json index 4bd7df22e..880bcbc9e 100644 --- a/package.json +++ b/package.json @@ -130,7 +130,7 @@ "check:affected:test": "node --experimental-strip-types --test scripts/check-affected/model.test.ts scripts/check-affected/run.test.ts", "check:coverage-changed": "node --experimental-strip-types scripts/coverage-changed/run.ts", "check:coverage-changed:test": "node --experimental-strip-types --test scripts/coverage-changed/model.test.ts scripts/coverage-changed/run.test.ts", - "check:layering": "node --experimental-strip-types --test scripts/layering/model.test.ts scripts/layering/zone-policy.test.ts scripts/layering/daemon-modularity.test.ts scripts/layering/package-boundaries.test.ts scripts/layering/facade-exports.test.ts && node --experimental-strip-types scripts/layering/check.ts", + "check:layering": "node --experimental-strip-types --test scripts/layering/model.test.ts scripts/layering/zone-policy.test.ts scripts/layering/daemon-modularity.test.ts scripts/layering/package-boundaries.test.ts scripts/layering/facade-exports.test.ts scripts/layering/bin-alias-fast-path.test.ts && node --experimental-strip-types scripts/layering/check.ts", "depgraph": "node --experimental-strip-types scripts/depgraph/build.ts", "depgraph:test": "node --experimental-strip-types --test scripts/depgraph/model.test.ts scripts/depgraph/affected.test.ts", "check:production-exports": "fallow dead-code --config fallow-production-exports.json --production --unused-exports --fail-on-issues", diff --git a/scripts/layering/bin-alias-fast-path.test.ts b/scripts/layering/bin-alias-fast-path.test.ts new file mode 100644 index 000000000..78e801907 --- /dev/null +++ b/scripts/layering/bin-alias-fast-path.test.ts @@ -0,0 +1,314 @@ +// R12 bin-alias-fast-path, tested directly: what each pure function reports for a fixture, +// independently of the check.ts wiring that turns it into a violation. + +import assert from 'node:assert/strict'; +import { readFileSync } from 'node:fs'; +import path from 'node:path'; +import { test } from 'node:test'; +import { + ALIAS_REGISTRY_FILE, + aliasResolverLocalName, + BIN_FILE, + countLocalBindings, + helpTargetBindingName, + importsAliasResolver, + localAliasLiterals, + registryAliasTokens, + usageTextDelegationFailure, +} from './bin-alias-fast-path.ts'; + +/** + * The shape of bin.ts's real `--help` fast path, minus everything R12 does not read. Fixtures + * below vary one thing against this baseline, so a test's subject is the line it changed. + */ +function binFixture(fastPathBody: string, prelude = ''): string { + return ` +import { normalizeCliCommandAlias } from './commands/cli-command-aliases.ts'; +${prelude} +function runHelpFastPath(argv) { + const helpTarget = resolveSimpleHelpTarget(argv); + if (helpTarget === undefined) return false; +${fastPathBody} + return true; +} +`; +} + +/** `usageTextDelegationFailure` for a fixture, resolving the local name the way check.ts does. */ +function delegationFailure(source: string): string | null { + return usageTextDelegationFailure(source, aliasResolverLocalName(source)!); +} + +const REGISTRY_FIXTURE = ` +import type { CliFlags } from '@agent-device/contracts/command'; +const CLI_COMMAND_ALIASES = [ + { alias: 'long-press', command: 'longpress' }, + { alias: 'metrics', command: 'perf' }, + { alias: 'tap', command: 'press' }, + { alias: 'launch', command: 'open' }, + { alias: 'relaunch', command: 'open', impliedFlags: ['relaunch'] }, +]; +export function normalizeCliCommandAlias(command) { return command; } +`; + +test('registryAliasTokens reads every alias property value out of the registry source', () => { + assert.deepEqual(registryAliasTokens(REGISTRY_FIXTURE), [ + 'launch', + 'long-press', + 'metrics', + 'relaunch', + 'tap', + ]); +}); + +test('registryAliasTokens is not fooled by an unrelated `alias` string elsewhere in the file', () => { + // Only a `{ alias: '' }` object-property VALUE counts. A same-named local variable, or + // the word appearing inside a comment, must not contribute a token. + const source = "const alias = 'not-a-token';\n// alias: also not a token\n"; + assert.deepEqual(registryAliasTokens(source), []); +}); + +test('importsAliasResolver is true only for a real VALUE import of the resolver', () => { + assert.equal( + importsAliasResolver( + "import { normalizeCliCommandAlias } from './commands/cli-command-aliases.ts';\n", + ), + true, + ); + // A renamed local binding still delegates to the real function — the registry specifier and + // the imported name are what matter, not what the caller calls it locally. + assert.equal( + importsAliasResolver( + "import { normalizeCliCommandAlias as resolve } from './commands/cli-command-aliases.ts';\n", + ), + true, + ); +}); + +test('importsAliasResolver is false for a type-only import', () => { + // Erased at compile time — no runtime delegation at all, which is exactly the STOP condition + // the original plan called out: importing the registry as a type only would look wired + // without actually being wired. + assert.equal( + importsAliasResolver( + "import type { normalizeCliCommandAlias } from './commands/cli-command-aliases.ts';\n", + ), + false, + ); +}); + +test('importsAliasResolver is false when the import is missing or from the wrong module', () => { + assert.equal(importsAliasResolver('const x = 1;\n'), false); + assert.equal( + importsAliasResolver("import { normalizeCliCommandAlias } from './wrong-file.ts';\n"), + false, + ); + assert.equal( + importsAliasResolver("import { somethingElse } from './commands/cli-command-aliases.ts';\n"), + false, + ); +}); + +test('aliasResolverLocalName resolves the LOCAL binding, following an `as` alias', () => { + assert.equal( + aliasResolverLocalName( + "import { normalizeCliCommandAlias } from './commands/cli-command-aliases.ts';\n", + ), + 'normalizeCliCommandAlias', + ); + assert.equal( + aliasResolverLocalName( + "import { normalizeCliCommandAlias as resolveAlias } from './commands/cli-command-aliases.ts';\n", + ), + 'resolveAlias', + ); +}); + +test('aliasResolverLocalName is null when there is no matching value import', () => { + assert.equal(aliasResolverLocalName('const x = 1;\n'), null); + assert.equal( + aliasResolverLocalName( + "import type { normalizeCliCommandAlias } from './commands/cli-command-aliases.ts';\n", + ), + null, + ); +}); + +test('helpTargetBindingName reads the binding resolveSimpleHelpTarget produces', () => { + assert.equal( + helpTargetBindingName(binFixture(' buildCommandUsageText(normalizeCliCommandAlias(x));')), + 'helpTarget', + ); + // Renaming the local re-points the guard rather than disarming it — the name is never assumed. + const renamed = ` +function runHelpFastPath(argv) { + const target = resolveSimpleHelpTarget(argv); +} +`; + assert.equal(helpTargetBindingName(renamed), 'target'); +}); + +test('helpTargetBindingName is null when the fast path no longer produces one', () => { + assert.equal(helpTargetBindingName('const helpTarget = argv[0];\n'), null); +}); + +test('countLocalBindings counts value declarations only, not the import or type positions', () => { + const source = ` +import { normalizeCliCommandAlias } from './commands/cli-command-aliases.ts'; +function f(helpTarget: normalizeCliCommandAlias) { const other = 1; } +`; + // The import itself is not a shadow, and a type annotation naming the resolver binds nothing. + assert.equal(countLocalBindings(source, 'normalizeCliCommandAlias'), 0); + assert.equal(countLocalBindings(source, 'helpTarget'), 1); + assert.equal(countLocalBindings('const x = 1;\nfunction x() {}\n', 'x'), 2); +}); + +test('usageTextDelegationFailure accepts the real composition, by local name', () => { + assert.equal( + delegationFailure( + binFixture( + ' const commandHelp = buildCommandUsageText(normalizeCliCommandAlias(helpTarget));', + ), + ), + null, + ); + // Binds by whatever LOCAL name the import resolved to — an aliased import's local name must + // still be found at the call site, since that is the only name available to call it by. + const aliased = ` +import { normalizeCliCommandAlias as resolveAlias } from './commands/cli-command-aliases.ts'; +function runHelpFastPath(argv) { + const helpTarget = resolveSimpleHelpTarget(argv); + const commandHelp = buildCommandUsageText(resolveAlias(helpTarget)); +} +`; + assert.equal(delegationFailure(aliased), null); +}); + +test('usageTextDelegationFailure rejects a raw call, with no wrapping resolver call', () => { + const failure = delegationFailure( + binFixture(' const commandHelp = buildCommandUsageText(helpTarget);'), + ); + assert.match(failure ?? '', /buildCommandUsageText\(helpTarget\)/); +}); + +// #P2 (first maintainer review of R12): import presence and literal absence both still pass a +// bin.ts that imports the resolver and never calls it, or calls it on something unrelated, while +// buildCommandUsageText runs on the raw, unresolved helpTarget. +test('usageTextDelegationFailure rejects a present-but-unused import', () => { + const source = binFixture(' const commandHelp = buildCommandUsageText(helpTarget);'); + assert.equal(importsAliasResolver(source), true); + assert.notEqual(delegationFailure(source), null); +}); + +test('usageTextDelegationFailure rejects an import used only unrelated to buildCommandUsageText', () => { + const source = binFixture( + ' const commandHelp = buildCommandUsageText(helpTarget);', + 'void normalizeCliCommandAlias;', + ); + assert.equal(importsAliasResolver(source), true); + assert.notEqual(delegationFailure(source), null); +}); + +// #P2 (second maintainer review of R12): the fixture below is the reviewer's own, verbatim in +// shape. An EXISTENTIAL fact 3 — "some buildCommandUsageText call somewhere wraps the resolver" — +// accepts it, because the decoy on the first line satisfies the quantifier while the line that +// actually ships resolves nothing. This is the regression that motivated making fact 3 universal +// and value-bound, and it must be rejected for BOTH reasons independently. +test('usageTextDelegationFailure rejects a decoy wrapped call beside a raw shipped call', () => { + const source = binFixture( + ` void buildCommandUsageText(normalizeCliCommandAlias('open')); + const commandHelp = buildCommandUsageText(helpTarget);`, + ); + assert.equal(importsAliasResolver(source), true); + const failure = delegationFailure(source); + assert.match(failure ?? '', /every buildCommandUsageText call must receive/); +}); + +test('usageTextDelegationFailure rejects the resolver applied to anything but the help target', () => { + // The decoy alone, with no raw call at all: the only usage-text call in the file wraps the + // resolver, so a universal-but-not-value-bound fact 3 would still pass it. + const source = binFixture( + " const commandHelp = buildCommandUsageText(normalizeCliCommandAlias('open'));", + ); + assert.match(delegationFailure(source) ?? '', /normalizeCliCommandAlias\("open"\)/); +}); + +test('usageTextDelegationFailure rejects a local shadow of the imported resolver', () => { + // Fact 3 binds by NAME, so a same-named local would otherwise let the composition read as + // delegation while calling something that resolves nothing. + const source = binFixture( + ' const commandHelp = buildCommandUsageText(normalizeCliCommandAlias(helpTarget));', + 'const normalizeCliCommandAlias = (command) => command;', + ); + assert.match(delegationFailure(source) ?? '', /shadowing the imported resolver/); +}); + +test('usageTextDelegationFailure rejects an ambiguous second help-target binding', () => { + const source = binFixture( + ` const helpTarget = 'open'; + const commandHelp = buildCommandUsageText(normalizeCliCommandAlias(helpTarget));`, + ); + assert.match(delegationFailure(source) ?? '', /declares helpTarget more than once/); +}); + +test('usageTextDelegationFailure reports a fast path that no longer builds usage text at all', () => { + const gone = ` +import { normalizeCliCommandAlias } from './commands/cli-command-aliases.ts'; +function runHelpFastPath(argv) { + const helpTarget = resolveSimpleHelpTarget(argv); + void normalizeCliCommandAlias(helpTarget); +} +`; + assert.match(delegationFailure(gone) ?? '', /never calls buildCommandUsageText/); + // …and one whose help-target producer is gone, so the guard says so instead of passing blind. + const untraceable = ` +import { normalizeCliCommandAlias } from './commands/cli-command-aliases.ts'; +const commandHelp = buildCommandUsageText(normalizeCliCommandAlias(argv[1])); +`; + assert.match(delegationFailure(untraceable) ?? '', /has no variable initialized by/); +}); + +test('localAliasLiterals reports every requested token present as a string literal', () => { + // The pre-fix bin.ts shape: a hand-written table re-declaring two of the five tokens. + const preFixBinSource = ` +function normalizeHelpTarget(command) { + if (command === 'long-press') return 'longpress'; + if (command === 'metrics') return 'perf'; + return command; +} +`; + assert.deepEqual( + localAliasLiterals(preFixBinSource, ['long-press', 'metrics', 'tap', 'launch', 'relaunch']), + ['long-press', 'metrics'], + ); +}); + +test('localAliasLiterals ignores tokens that only appear as identifiers, not string literals', () => { + const source = 'const tap = 1;\nfunction launch() {}\n'; + assert.deepEqual(localAliasLiterals(source, ['tap', 'launch']), []); +}); + +test('localAliasLiterals reports nothing when the fixed bin.ts delegates and holds no literals', () => { + const fixedBinSource = ` +import { normalizeCliCommandAlias } from './commands/cli-command-aliases.ts'; +const commandHelp = buildCommandUsageText(normalizeCliCommandAlias(helpTarget)); +`; + assert.deepEqual( + localAliasLiterals(fixedBinSource, ['long-press', 'metrics', 'tap', 'launch', 'relaunch']), + [], + ); +}); + +const repoRoot = path.resolve(import.meta.dirname, '../..'); + +test('the real tree imports the resolver, calls it into buildCommandUsageText, holds no local alias literals, and passes R12', () => { + const registrySource = readFileSync(path.join(repoRoot, ALIAS_REGISTRY_FILE), 'utf8'); + const binSource = readFileSync(path.join(repoRoot, BIN_FILE), 'utf8'); + const tokens = registryAliasTokens(registrySource); + assert.deepEqual(tokens, ['launch', 'long-press', 'metrics', 'relaunch', 'tap']); + const localName = aliasResolverLocalName(binSource); + assert.equal(localName, 'normalizeCliCommandAlias'); + assert.equal(helpTargetBindingName(binSource), 'helpTarget'); + assert.equal(usageTextDelegationFailure(binSource, localName!), null); + assert.deepEqual(localAliasLiterals(binSource, tokens), []); +}); diff --git a/scripts/layering/bin-alias-fast-path.ts b/scripts/layering/bin-alias-fast-path.ts new file mode 100644 index 000000000..e3296538f --- /dev/null +++ b/scripts/layering/bin-alias-fast-path.ts @@ -0,0 +1,339 @@ +// R12 bin-alias-fast-path. +// +// `bin.ts`'s `--help` fast path resolves a command alias (`tap`, `launch`, …) to its canonical +// command before looking up static help text. #1618-adjacent: bin.ts once carried its own +// hand-written two-entry table (`long-press`, `metrics`) instead of calling the real alias +// registry, `commands/cli-command-aliases.ts` (five entries). The table silently fell out of +// sync — `tap`, `launch`, `relaunch` missed the fast path entirely and paid a full CLI bootstrap +// just to print static help text — and nothing failed, because bin.ts's own top-level dispatch +// runs unconditionally on import (see the module comment on `check.ts`'s R7 for the same +// "cannot safely unit-import this file" constraint) and is deliberately excluded from coverage +// (`vitest.config.ts`), so no unit test can call into it directly. +// +// Three structural facts, read from bin.ts's source text rather than by importing and running +// it, close the gap without needing to import it: +// 1. bin.ts holds a VALUE import of `normalizeCliCommandAlias` from the registry — so it is +// wired to delegate. +// 2. bin.ts never itself contains one of the registry's OWN alias tokens as a string literal — +// so it cannot be re-declaring a parallel mapping instead of actually calling the import. +// 3. EVERY call to `buildCommandUsageText` in bin.ts receives `()` — the +// LOCAL binding fact 1 imported, applied to the binding the fast path's own +// `resolveSimpleHelpTarget(...)` produced. This is the actual composition the fast path +// needs (`buildCommandUsageText(normalizeCliCommandAlias(helpTarget))`), not merely the +// import's presence. Facts 1 and 2 alone still pass if bin.ts imports the resolver and never +// calls it, or calls it on something unrelated (`void normalizeCliCommandAlias`) while +// `buildCommandUsageText(helpTarget)` runs raw — a real gap a maintainer review caught +// (the guard's own P2 follow-up). +// +// Fact 3 is deliberately UNIVERSAL and VALUE-BOUND, not existential, which is the second P2 from +// the same review. An "is there any `buildCommandUsageText(resolver(...))` somewhere" phrasing is +// satisfied by a decoy that never runs on the help target: +// +// void buildCommandUsageText(normalizeCliCommandAlias('press')); // decoy, satisfies ∃ +// const commandHelp = buildCommandUsageText(helpTarget); // what actually ships +// +// Requiring every usage-text call to receive the resolver applied to the help-target binding +// rejects both lines: the decoy resolves a literal rather than the fast path's own value, and the +// shipped call is raw. The help-target binding is discovered from bin.ts's source (the variable +// initialized by `resolveSimpleHelpTarget(...)`) rather than hard-coded, so renaming the local +// does not silently disarm the guard — it re-points it. +// +// Fact 3 binds by the import's LOCAL name, following any `as` alias, so +// `import { normalizeCliCommandAlias as resolveAlias }` still passes. Because that is a +// name-based claim about binding identity, fact 3 additionally requires that no local +// declaration in bin.ts SHADOWS either name: a local `const normalizeCliCommandAlias = (c) => c` +// would otherwise let the composition read correctly while calling something else entirely, and +// a second `helpTarget` declaration would let the resolver run on an unrelated value. +// +// All three were false on the pre-fix bin.ts (no import; both 'long-press' and 'metrics' present +// as literals; no composition to find), so the set is a real regression pin, not just a +// description of intent. +// +// AST-based (`oxc-parser`, the standing precedent in this directory — session-state.ts, +// facade-exports.ts, zero-dep-jobs.ts), not a line scan: a line scan reading raw text for +// "'tap'" would mistake this very comment, or a fixture string in a test file, for the real +// thing — precisely the false-positive failure mode that turned this directory to +// `parseSync(...).module`/`.program` in the first place. + +import { parseSync } from 'oxc-parser'; + +export const BIN_FILE = 'src/bin.ts'; +export const ALIAS_REGISTRY_FILE = 'src/commands/cli-command-aliases.ts'; +// The specifier bin.ts must use to reach the registry — relative to BIN_FILE's own directory +// (src/), not to the repo root, since that is how bin.ts's own import statement writes it. +const ALIAS_REGISTRY_SPECIFIER = './commands/cli-command-aliases.ts'; +const ALIAS_RESOLVER_EXPORT = 'normalizeCliCommandAlias'; + +/** Depth-first walk over an oxc-parser AST subtree (or `.module` entry list). */ +function visit(node: unknown, onNode: (record: Record) => void): void { + if (node === null || typeof node !== 'object') return; + if (Array.isArray(node)) { + for (const child of node) visit(child, onNode); + return; + } + const record = node as Record; + onNode(record); + for (const key of Object.keys(record)) visit(record[key], onNode); +} + +/** + * The alias tokens the registry declares — `CLI_COMMAND_ALIASES`'s `alias:` property values, + * read out of the registry's own source text rather than imported and executed. Every other + * gate in this directory treats its target as data to parse, not a module to run (session-state + * .ts reads `daemon/types.ts` the same way); staying consistent means R12 needs no `pnpm build` + * and cannot be fooled by import side effects. `CLI_COMMAND_ALIASES` itself is deliberately + * unexported (a façade names only what it means to share) — this reads its literal values + * directly out of the array-literal declaration instead, so a future sixth alias is picked up + * automatically and this list never needs hand-maintaining in a second place. + */ +export function registryAliasTokens(registrySource: string): string[] { + const parsed = parseSync(ALIAS_REGISTRY_FILE, registrySource); + const tokens = new Set(); + visit(parsed.program, (record) => { + if (record['type'] !== 'Property') return; + const key = record['key'] as Record | undefined; + if (key?.['type'] !== 'Identifier' || key['name'] !== 'alias') return; + const value = record['value'] as Record | undefined; + if (value?.['type'] === 'Literal' && typeof value['value'] === 'string') { + tokens.add(value['value'] as string); + } + }); + return [...tokens].sort(); +} + +/** + * The LOCAL binding name `binSource` imports `normalizeCliCommandAlias` as — following any `as` + * alias — for a VALUE (not type-only) import from the alias registry, or `null` if there is no + * such import. Reads `oxc-parser`'s own resolved import-entry table (`module.staticImports`), + * the same source `zero-dep-jobs.ts`'s `moduleSpecifiers` uses — not a regex, so + * `import type { normalizeCliCommandAlias as x }` (erased at compile time, no runtime delegation + * at all) cannot pass as a real import the way a line match on the specifier text would. + * + * Reporting the LOCAL name (not just a boolean) is what lets `usageTextDelegationFailure` below + * bind by the name bin.ts actually calls, so a renamed import (`... as resolveAlias`) still + * verifies, while a same-named unrelated local cannot be mistaken for it (that one is enforced, + * not assumed — see the shadow check there). + */ +export function aliasResolverLocalName(binSource: string): string | null { + const parsed = parseSync(BIN_FILE, binSource); + for (const entry of parsed.module.staticImports) { + if (entry.moduleRequest.value !== ALIAS_REGISTRY_SPECIFIER) continue; + for (const specifier of entry.entries) { + if ( + !specifier.isType && + specifier.importName.kind === 'Name' && + specifier.importName.name === ALIAS_RESOLVER_EXPORT + ) { + return specifier.localName.value; + } + } + } + return null; +} + +/** Whether `binSource` holds a VALUE import of `normalizeCliCommandAlias` from the registry. */ +export function importsAliasResolver(binSource: string): boolean { + return aliasResolverLocalName(binSource) !== null; +} + +const USAGE_TEXT_CALLEE = 'buildCommandUsageText'; +const HELP_TARGET_PRODUCER = 'resolveSimpleHelpTarget'; + +function isCallTo(node: unknown, calleeName: string): boolean { + if (node === null || typeof node !== 'object') return false; + const record = node as Record; + if (record['type'] !== 'CallExpression') return false; + const callee = record['callee'] as Record | undefined; + return callee?.['type'] === 'Identifier' && callee['name'] === calleeName; +} + +function isIdentifierNamed(node: unknown, name: string): boolean { + if (node === null || typeof node !== 'object') return false; + const record = node as Record; + return record['type'] === 'Identifier' && record['name'] === name; +} + +/** A short, quotable rendering of an argument expression, for the violation message. */ +function describeArgument(node: unknown): string { + if (node === null || typeof node !== 'object') return String(node); + const record = node as Record; + if (record['type'] === 'Identifier') return String(record['name']); + if (record['type'] === 'Literal') return JSON.stringify(record['value']); + if (record['type'] === 'CallExpression') { + const callee = record['callee'] as Record | undefined; + const calleeName = callee?.['type'] === 'Identifier' ? String(callee['name']) : ''; + const args = Array.isArray(record['arguments']) ? record['arguments'] : []; + return `${calleeName}(${args.map(describeArgument).join(', ')})`; + } + return `<${String(record['type'])}>`; +} + +/** + * The LOCAL name of the fast path's help-target binding — the variable initialized by + * `resolveSimpleHelpTarget(...)` — or `null` if bin.ts no longer produces one that way. + * + * Read from the source rather than hard-coded so that renaming the local re-points the guard + * instead of disarming it, and so `helpTarget` never has to be maintained as a magic string in + * two places. + */ +export function helpTargetBindingName(binSource: string): string | null { + const parsed = parseSync(BIN_FILE, binSource); + let name: string | null = null; + visit(parsed.program, (record) => { + if (name !== null || record['type'] !== 'VariableDeclarator') return; + if (!isCallTo(record['init'], HELP_TARGET_PRODUCER)) return; + const id = record['id'] as Record | undefined; + if (id?.['type'] === 'Identifier') name = String(id['name']); + }); + return name; +} + +/** + * Every VALUE binding `binSource` declares locally under `name` — variable declarators, function + * and class declarations, function parameters, and catch clauses. + * + * This is what makes fact 3's binding-identity claim real rather than nominal: the composition + * `buildCommandUsageText(normalizeCliCommandAlias(helpTarget))` reads as delegation whether the + * callee is the import or a local shadow that happens to share its name, and only a declaration + * scan can tell those apart. Over-collection is the safe direction here — a false positive on + * these two specific names fails the gate loudly rather than passing a shadowed call silently — + * so patterns are walked whole, with type annotations skipped (a type named `helpTarget` binds + * nothing at runtime and must not count as a shadow). + */ +export function countLocalBindings(binSource: string, name: string): number { + const parsed = parseSync(BIN_FILE, binSource); + let count = 0; + const scanPattern = (node: unknown): void => { + visitSkippingTypes(node, (record) => { + if (isIdentifierNamed(record, name)) count += 1; + }); + }; + visit(parsed.program, (record) => { + switch (record['type']) { + case 'VariableDeclarator': + scanPattern(record['id']); + return; + case 'FunctionDeclaration': + case 'FunctionExpression': + case 'ArrowFunctionExpression': + case 'ClassDeclaration': + case 'ClassExpression': + if (isIdentifierNamed(record['id'], name)) count += 1; + scanPattern(record['params']); + return; + case 'CatchClause': + scanPattern(record['param']); + return; + default: + } + }); + return count; +} + +/** `visit`, minus type-position subtrees — type names bind nothing at runtime. */ +function visitSkippingTypes( + node: unknown, + onNode: (record: Record) => void, +): void { + if (node === null || typeof node !== 'object') return; + if (Array.isArray(node)) { + for (const child of node) visitSkippingTypes(child, onNode); + return; + } + const record = node as Record; + onNode(record); + for (const key of Object.keys(record)) { + if (key === 'typeAnnotation' || key === 'returnType' || key === 'typeParameters') continue; + visitSkippingTypes(record[key], onNode); + } +} + +/** + * Why `binSource` fails fact 3, or `null` if it holds. + * + * Fact 3 is universal and value-bound: EVERY `buildCommandUsageText(...)` call in bin.ts must + * receive `resolverLocalName()`, where `` is the binding + * `resolveSimpleHelpTarget(...)` produced. The existential phrasing this replaces ("some call + * somewhere wraps the resolver") is satisfied by a decoy that resolves an unrelated value while + * the shipped call runs raw — see the module comment for that exact fixture. + * + * Returning the reason rather than a boolean lets the gate say which of the several distinct + * ways to fail actually happened; a bare `false` sent a maintainer back to re-derive it. + */ +export function usageTextDelegationFailure( + binSource: string, + resolverLocalName: string, +): string | null { + if (countLocalBindings(binSource, resolverLocalName) > 0) { + return ( + `declares a local binding named ${resolverLocalName}, shadowing the imported resolver — ` + + `a call to ${resolverLocalName}(...) then proves nothing about delegating to ` + + `${ALIAS_REGISTRY_FILE}. Remove the shadow (or import the resolver under a different name).` + ); + } + + const helpTarget = helpTargetBindingName(binSource); + if (helpTarget === null) { + return ( + `has no variable initialized by ${HELP_TARGET_PRODUCER}(...), so the --help fast path's ` + + 'help-target binding cannot be located and its delegation cannot be checked. Keep the ' + + 'fast path resolving its target through that helper, or re-point this rule at its ' + + 'replacement.' + ); + } + if (countLocalBindings(binSource, helpTarget) > 1) { + return ( + `declares ${helpTarget} more than once, so "${USAGE_TEXT_CALLEE}(${resolverLocalName}(` + + `${helpTarget}))" no longer names one value — the resolver could be running on an ` + + 'unrelated binding that shares the name.' + ); + } + + const parsed = parseSync(BIN_FILE, binSource); + const calls: Record[] = []; + visit(parsed.program, (record) => { + if (isCallTo(record, USAGE_TEXT_CALLEE)) calls.push(record); + }); + + if (calls.length === 0) { + return ( + `never calls ${USAGE_TEXT_CALLEE} — the --help fast path that alias resolution exists to ` + + 'serve is gone, so this rule is checking nothing. Restore the fast path or retire R12.' + ); + } + + for (const call of calls) { + const args = Array.isArray(call['arguments']) ? call['arguments'] : []; + const first = args[0]; + const wraps = isCallTo(first, resolverLocalName); + const resolverArgs = + wraps && Array.isArray((first as Record)['arguments']) + ? ((first as Record)['arguments'] as unknown[]) + : []; + if (wraps && isIdentifierNamed(resolverArgs[0], helpTarget)) continue; + return ( + `calls ${USAGE_TEXT_CALLEE}(${describeArgument(first)}) — every ${USAGE_TEXT_CALLEE} call ` + + `must receive ${resolverLocalName}(${helpTarget}), the imported resolver applied to the ` + + 'fast path’s own help target. A call that resolves something else (or nothing) leaves ' + + 'the shipped path un-delegated while looking wired.' + ); + } + return null; +} + +/** + * Which of `tokens` appear as a string-literal VALUE anywhere in `binSource` — not a substring + * match on the raw text, so a token that only shows up inside an unrelated identifier or this + * module's own doc comment does not count. + */ +export function localAliasLiterals(binSource: string, tokens: readonly string[]): string[] { + const wanted = new Set(tokens); + const parsed = parseSync(BIN_FILE, binSource); + const found = new Set(); + visit(parsed.program, (record) => { + if (record['type'] !== 'Literal') return; + const value = record['value']; + if (typeof value === 'string' && wanted.has(value)) found.add(value); + }); + return [...found].sort(); +} diff --git a/scripts/layering/check.ts b/scripts/layering/check.ts index a40aa8145..00eb72ae3 100644 --- a/scripts/layering/check.ts +++ b/scripts/layering/check.ts @@ -28,6 +28,9 @@ // engine files, and planned logical modules start with zero forbidden/internal imports (R10). // - Over the WORKSPACE PACKAGES: no root back-imports, no relative tunnelling past // an exports map, and every workspace specifier declared + exports-named (R11). +// - Over BIN.TS'S ALIAS RESOLUTION: it must delegate to the one alias registry instead of +// re-declaring a parallel mapping of its own (R12) — the same "delegate to your single +// owner" shape as R7's SessionState ownership, applied to bin.ts's `--help` fast path. // Only `(root)` is unranked among src/ zones (see `UNRANKED_ZONES` in model.ts): // it holds entrypoints and composition roots. Extracted workspace package zones // are classified separately and held behind R11 instead of the src folder spine. @@ -45,6 +48,14 @@ import { STORE_OWNED_SESSION_STATE_FIELDS, } from './session-state.ts'; import { uninstallableImports, zeroDepClosureFiles, zeroDepJobs } from './zero-dep-jobs.ts'; +import { + ALIAS_REGISTRY_FILE, + aliasResolverLocalName, + BIN_FILE, + localAliasLiterals, + registryAliasTokens, + usageTextDelegationFailure, +} from './bin-alias-fast-path.ts'; import { backEdgePair, findValueImportCycles, @@ -366,6 +377,75 @@ function checkSessionStateOwnership(sources: ReadonlyMap): Layer return violations; } +/** + * R12: bin.ts's `--help` fast path must delegate command-alias resolution to the one alias + * registry instead of re-declaring its own mapping. See bin-alias-fast-path.ts for why the + * three facts below, together, are what closes the gap the original drift exploited — import + * presence and literal absence alone still pass a bin.ts that imports the resolver and never + * calls it (or calls it on something unrelated) while `buildCommandUsageText(helpTarget)` runs + * raw, which is exactly the P2 a maintainer review caught. Fact 3 is what closes that: EVERY + * `buildCommandUsageText` call must receive the imported resolver applied to the fast path's own + * help-target binding, with neither name shadowed by a local declaration. The universal + * quantifier is the follow-up P2 — an existential one is satisfied by a decoy call that resolves + * an unrelated literal while the shipped call still runs raw. + */ +function checkBinAliasFastPath(sources: ReadonlyMap): LayeringViolation[] { + const registrySource = sources.get(ALIAS_REGISTRY_FILE); + const binSource = sources.get(BIN_FILE); + if (!registrySource || !binSource) { + const missing = !registrySource ? ALIAS_REGISTRY_FILE : BIN_FILE; + return [ + { + rule: 'R12 bin-alias-fast-path', + file: missing, + line: 1, + message: `${missing} is missing, so bin.ts's alias delegation cannot be checked.`, + }, + ]; + } + + const violations: LayeringViolation[] = []; + const resolverLocalName = aliasResolverLocalName(binSource); + if (resolverLocalName === null) { + violations.push({ + rule: 'R12 bin-alias-fast-path', + file: BIN_FILE, + line: 1, + message: + 'does not hold a value import of normalizeCliCommandAlias from ' + + `${ALIAS_REGISTRY_FILE} — the --help fast path cannot delegate alias resolution to the ` + + 'registry without it.', + }); + } else { + const delegationFailure = usageTextDelegationFailure(binSource, resolverLocalName); + if (delegationFailure !== null) { + violations.push({ + rule: 'R12 bin-alias-fast-path', + file: BIN_FILE, + line: 1, + message: + `imports normalizeCliCommandAlias (locally ${resolverLocalName}) but ` + + `${delegationFailure}`, + }); + } + } + + const localLiterals = localAliasLiterals(binSource, registryAliasTokens(registrySource)); + if (localLiterals.length > 0) { + violations.push({ + rule: 'R12 bin-alias-fast-path', + file: BIN_FILE, + line: 1, + message: + `contains the registry's own alias token(s) (${localLiterals.join(', ')}) as string ` + + 'literals — a local alias-mapping table, hand-rolled instead of delegated to ' + + `${ALIAS_REGISTRY_FILE}. Delegate through normalizeCliCommandAlias instead of ` + + 're-declaring the mapping.', + }); + } + return violations; +} + // R8: a CI job that runs with `install-deps: false` has no `node_modules`, so every script it // reaches must import only Node builtins and other repo files. Locally the opposite is true — // `node_modules` is always present — which is why this needs a gate rather than a convention. @@ -441,7 +521,10 @@ function report( `inversions match the R6 ratchet (${Object.values(TYPE_INVERSION_BASELINE).reduce((sum, count) => sum + count, 0)} remaining); ` + `all ${sessionStateFieldCount()} SessionState fields are classified and every write is ` + `inside its declared owner (R7); every zero-dep CI job resolves without ` + - `node_modules (R8); ${typeCycleNote(typeCycle)}; ${daemonModularitySummary()}; and ${packageBoundariesSummary(repoRoot)}.\n`, + `node_modules (R8); ${typeCycleNote(typeCycle)}; ${daemonModularitySummary()}; ` + + `${packageBoundariesSummary(repoRoot)}; and bin.ts imports normalizeCliCommandAlias, ` + + `actually passes it into buildCommandUsageText, and holds no local alias literals ` + + `(R12).\n`, ); return 0; } @@ -482,6 +565,7 @@ export function main(): number { ...checkSessionStateOwnership(sources), ...checkDaemonModularityRatchets(edges, typeCycleMembers), ...checkZeroDepJobs(), + ...checkBinAliasFastPath(sources), ...checkPackageBoundaries( repoRoot, zeroDepClosureFiles(repoZeroDepJobs(), readSourceOrNull, fileExists), diff --git a/src/bin.ts b/src/bin.ts index 18b0a2006..4afd45ebb 100644 --- a/src/bin.ts +++ b/src/bin.ts @@ -1,3 +1,5 @@ +import { normalizeCliCommandAlias } from './commands/cli-command-aliases.ts'; + const argv = process.argv.slice(2); declare const __AGENT_DEVICE_VERSION__: string; @@ -54,7 +56,7 @@ function runHelpFastPath(argv: string[]): boolean { process.stdout.write(`${buildUsageText()}\n`); return; } - const commandHelp = buildCommandUsageText(normalizeHelpTarget(helpTarget)); + const commandHelp = buildCommandUsageText(normalizeCliCommandAlias(helpTarget)); if (commandHelp) { process.stdout.write(commandHelp); return; @@ -97,12 +99,6 @@ function resolveTrailingHelpTarget( return isHelpFlag(helpArg) ? command : undefined; } -function normalizeHelpTarget(command: string): string { - if (command === 'long-press') return 'longpress'; - if (command === 'metrics') return 'perf'; - return command; -} - function isHelpCommand(command: string | undefined): boolean { return command === 'help'; } diff --git a/src/cli/parser/__tests__/cli-help-alias-fast-path.test.ts b/src/cli/parser/__tests__/cli-help-alias-fast-path.test.ts new file mode 100644 index 000000000..1e164223d --- /dev/null +++ b/src/cli/parser/__tests__/cli-help-alias-fast-path.test.ts @@ -0,0 +1,66 @@ +// Pins the exact composition `bin.ts`'s `--help` fast path relies on: +// `buildCommandUsageText(normalizeCliCommandAlias(helpTarget))`. Before this +// fix, `bin.ts` used its own hand-written two-entry table instead of this +// composition, so `tap`, `launch`, and `relaunch` silently missed the fast +// path and fell through to a full CLI bootstrap just to print static help +// text. `bin.ts` runs unguarded top-level dispatch on import (and is +// deliberately excluded from coverage — see vitest.config.ts), so it cannot +// be imported directly in a test; these tests instead pin the registry +// composition it calls. That makes them a real regression pin for a *future* +// alias missing help text (test 2 is durable for that), but not a substitute +// for the manual proof, run outside this suite, that bin.ts itself calls +// this composition (see the plan's execution report for the red/green +// evidence: with the stale table, `tap --help` loads `src/cli.ts`; with this +// fix, it does not). +import { test } from 'vitest'; +import assert from 'node:assert/strict'; +import { buildCommandUsageText } from '../cli-help.ts'; +import { + cliAliasesForCommand, + normalizeCliCommandAlias, +} from '../../../commands/cli-command-aliases.ts'; +import { listCliCommandNames } from '../../../command-catalog.ts'; + +test('alias help output matches its canonical command', () => { + const cases: ReadonlyArray = [ + ['tap', 'press'], + ['launch', 'open'], + ['relaunch', 'open'], + ['long-press', 'longpress'], + ['metrics', 'perf'], + ]; + for (const [alias, canonical] of cases) { + const aliasHelp = buildCommandUsageText(normalizeCliCommandAlias(alias)); + const canonicalHelp = buildCommandUsageText(canonical); + assert.notEqual(aliasHelp, null, `expected help text for alias "${alias}"`); + assert.equal( + aliasHelp, + canonicalHelp, + `expected "${alias} --help" to be byte-identical to "${canonical} --help"`, + ); + } +}); + +test('every CLI alias resolves to a command with help text', () => { + // Derive the alias list from the registry itself (via the canonical + // commands it targets) rather than hard-coding the five current alias + // names — a hard-coded list would silently stop covering a future sixth + // alias, reintroducing exactly the drift this test exists to catch. + const aliases = listCliCommandNames().flatMap((command) => + cliAliasesForCommand(command).map((entry) => entry.alias), + ); + assert.ok(aliases.length > 0, 'expected at least one alias to exercise this test'); + for (const alias of aliases) { + const help = buildCommandUsageText(normalizeCliCommandAlias(alias)); + assert.notEqual(help, null, `expected buildCommandUsageText to resolve alias "${alias}"`); + } +}); + +test('rotate still has no fast-path help', () => { + // `rotate` is not in the alias registry, so it must fall through to the + // slow path (`src/cli/parser/args.ts`'s `normalizeCommandAlias`), which is + // where the "renamed to orientation" migration error is raised. The fast + // path must never special-case `rotate` itself. + const help = buildCommandUsageText(normalizeCliCommandAlias('rotate')); + assert.equal(help, null); +});