From 81234491022b658c61a9d5c1d52a9f7462bc50ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Thu, 6 Aug 2026 12:41:52 +0200 Subject: [PATCH 1/4] perf(cli): route command aliases through the help fast path bin.ts's `--help` fast path resolved aliases through a hand-written two-entry table that had drifted out of sync with the real CLI_COMMAND_ALIASES registry (five entries). `tap`, `launch`, and `relaunch` missed the table and silently fell through to a full runCli() bootstrap just to print static help text (~150-165ms vs ~45-50ms for aliases already in the table). Delegate to the shared normalizeCliCommandAlias registry instead of the stale local table, so every alias the registry knows about gets the fast path automatically. --- src/bin.ts | 10 +-- .../cli-help-alias-fast-path.test.ts | 66 +++++++++++++++++++ 2 files changed, 69 insertions(+), 7 deletions(-) create mode 100644 src/cli/parser/__tests__/cli-help-alias-fast-path.test.ts 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); +}); From 44e49412c3a08ee212f54467e9d402e8fe9bca00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Thu, 6 Aug 2026 15:30:13 +0200 Subject: [PATCH 2/4] test(cli): add R12 layering guard for bin.ts's alias delegation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The unit test added for the alias fast-path fix (cli-help-alias-fast-path.test.ts) calls normalizeCliCommandAlias directly, so it stays green even if bin.ts itself reverts to a hand-rolled table — it pins the registry composition, not bin.ts's own wiring, and bin.ts cannot be safely unit-imported (it runs unguarded top-level dispatch on import and is deliberately excluded from coverage). Add an AST-based structural guard instead, in the style already established by scripts/layering/session-state.ts, facade-exports.ts, and zero-dep-jobs.ts (oxc-parser's module/program records, not a line scan, so a fixture's string literal can't produce a false hit). R12 asserts two facts about src/bin.ts: it holds a value import of normalizeCliCommandAlias from commands/cli-command-aliases.ts, and it contains none of the registry's own alias tokens as string literals. The token list is read out of the registry's own source (CLI_COMMAND_ALIASES's `alias:` property values), not hard-coded, so a future sixth alias is covered automatically. Both facts were false on the pre-fix bin.ts, verified by reverting locally and capturing the failure before restoring the fix. Wired into the existing check:layering chain (already part of check:tooling), next to R7's session-state ownership rule, which pins the same "delegate to your single owner" shape. --- package.json | 2 +- scripts/layering/bin-alias-fast-path.test.ts | 126 +++++++++++++++++++ scripts/layering/bin-alias-fast-path.ts | 113 +++++++++++++++++ scripts/layering/check.ts | 64 +++++++++- 4 files changed, 303 insertions(+), 2 deletions(-) create mode 100644 scripts/layering/bin-alias-fast-path.test.ts create mode 100644 scripts/layering/bin-alias-fast-path.ts 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..a05d5675e --- /dev/null +++ b/scripts/layering/bin-alias-fast-path.test.ts @@ -0,0 +1,126 @@ +// 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, + BIN_FILE, + importsAliasResolver, + localAliasLiterals, + registryAliasTokens, +} from './bin-alias-fast-path.ts'; + +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('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, 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']); + assert.equal(importsAliasResolver(binSource), true); + 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..9d1419742 --- /dev/null +++ b/scripts/layering/bin-alias-fast-path.ts @@ -0,0 +1,113 @@ +// 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. +// +// Two 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 +// (fact 1 alone would still pass if bin.ts imported the function and never called it, or +// called it beside a leftover local table; fact 2 is what makes the pair exhaustive). +// +// Both were false on the pre-fix bin.ts (no import; both 'long-press' and 'metrics' present as +// literals), so the pair 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(); +} + +/** + * Whether `binSource` holds a VALUE (not type-only) import of `normalizeCliCommandAlias` from + * the alias registry. 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. + */ +export function importsAliasResolver(binSource: string): boolean { + const parsed = parseSync(BIN_FILE, binSource); + return parsed.module.staticImports.some((entry) => { + if (entry.moduleRequest.value !== ALIAS_REGISTRY_SPECIFIER) return false; + return entry.entries.some( + (specifier) => + !specifier.isType && + specifier.importName.kind === 'Name' && + specifier.importName.name === ALIAS_RESOLVER_EXPORT, + ); + }); +} + +/** + * 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..0b568dae3 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,13 @@ import { STORE_OWNED_SESSION_STATE_FIELDS, } from './session-state.ts'; import { uninstallableImports, zeroDepClosureFiles, zeroDepJobs } from './zero-dep-jobs.ts'; +import { + ALIAS_REGISTRY_FILE, + BIN_FILE, + importsAliasResolver, + localAliasLiterals, + registryAliasTokens, +} from './bin-alias-fast-path.ts'; import { backEdgePair, findValueImportCycles, @@ -366,6 +376,55 @@ 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 + * two facts below, together, are what closes the gap the original drift exploited. + */ +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[] = []; + if (!importsAliasResolver(binSource)) { + 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.', + }); + } + + 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 +500,9 @@ 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 delegates command-alias resolution ` + + `to the registry with no local alias literals (R12).\n`, ); return 0; } @@ -482,6 +543,7 @@ export function main(): number { ...checkSessionStateOwnership(sources), ...checkDaemonModularityRatchets(edges, typeCycleMembers), ...checkZeroDepJobs(), + ...checkBinAliasFastPath(sources), ...checkPackageBoundaries( repoRoot, zeroDepClosureFiles(repoZeroDepJobs(), readSourceOrNull, fileExists), From d2c3b1be561162cfde4129d834e14565ec0829fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Thu, 6 Aug 2026 17:20:58 +0200 Subject: [PATCH 3/4] test(cli): pin the alias-resolver call into buildCommandUsageText (R12 P2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Maintainer review of R12 (PR #1641): import-presence and literal-absence alone let bin.ts regress to buildCommandUsageText(helpTarget) while the normalizeCliCommandAlias import stays in place, used harmlessly elsewhere (or not at all) — the real-tree gate stayed green through that exact regression. Add a third fact: bin.ts's call to buildCommandUsageText must receive, as its argument, a call to the LOCAL binding the resolver was imported as (aliasResolverLocalName + usageTextCallsResolver, both AST-based). Binding by local name rather than the literal export name means a renamed import (`as resolveAlias`) still verifies, and an unrelated same-named local cannot be mistaken for it. Verified by reverting locally to exactly the missed regression — import left in place, call reverted to buildCommandUsageText(helpTarget) — and confirming R12 now fails where the two-fact version passed; restored after. Two negative fixtures pin the scenario going forward: import present but unused, and import present but used only unrelated to the call. --- scripts/layering/bin-alias-fast-path.test.ts | 86 +++++++++++++++++- scripts/layering/bin-alias-fast-path.ts | 91 +++++++++++++++----- scripts/layering/check.ts | 28 ++++-- 3 files changed, 178 insertions(+), 27 deletions(-) diff --git a/scripts/layering/bin-alias-fast-path.test.ts b/scripts/layering/bin-alias-fast-path.test.ts index a05d5675e..c7d86a3a3 100644 --- a/scripts/layering/bin-alias-fast-path.test.ts +++ b/scripts/layering/bin-alias-fast-path.test.ts @@ -7,10 +7,12 @@ import path from 'node:path'; import { test } from 'node:test'; import { ALIAS_REGISTRY_FILE, + aliasResolverLocalName, BIN_FILE, importsAliasResolver, localAliasLiterals, registryAliasTokens, + usageTextCallsResolver, } from './bin-alias-fast-path.ts'; const REGISTRY_FIXTURE = ` @@ -83,6 +85,84 @@ test('importsAliasResolver is false when the import is missing or from the wrong ); }); +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('usageTextCallsResolver is true for the real composition, by local name', () => { + assert.equal( + usageTextCallsResolver( + 'const commandHelp = buildCommandUsageText(normalizeCliCommandAlias(helpTarget));\n', + 'normalizeCliCommandAlias', + ), + true, + ); + // Binds by whatever LOCAL name the caller passes — 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. + assert.equal( + usageTextCallsResolver( + 'const commandHelp = buildCommandUsageText(resolveAlias(helpTarget));\n', + 'resolveAlias', + ), + true, + ); +}); + +test('usageTextCallsResolver is false for a raw call, with no wrapping resolver call', () => { + assert.equal( + usageTextCallsResolver( + 'const commandHelp = buildCommandUsageText(helpTarget);\n', + 'normalizeCliCommandAlias', + ), + false, + ); +}); + +// #P2 (maintainer review of the original 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. These two fixtures are +// exactly that regression — the import is real and even "used", but never as the argument +// buildCommandUsageText receives — and usageTextCallsResolver must reject both. +test('usageTextCallsResolver rejects a present-but-unused import', () => { + const source = ` +import { normalizeCliCommandAlias } from './commands/cli-command-aliases.ts'; +const commandHelp = buildCommandUsageText(helpTarget); +`; + assert.equal(importsAliasResolver(source), true); + assert.equal(usageTextCallsResolver(source, aliasResolverLocalName(source)!), false); +}); + +test('usageTextCallsResolver rejects an import used only unrelated to buildCommandUsageText', () => { + const source = ` +import { normalizeCliCommandAlias } from './commands/cli-command-aliases.ts'; +void normalizeCliCommandAlias; +const commandHelp = buildCommandUsageText(helpTarget); +`; + assert.equal(importsAliasResolver(source), true); + assert.equal(usageTextCallsResolver(source, aliasResolverLocalName(source)!), false); +}); + 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 = ` @@ -116,11 +196,13 @@ const commandHelp = buildCommandUsageText(normalizeCliCommandAlias(helpTarget)); const repoRoot = path.resolve(import.meta.dirname, '../..'); -test('the real tree imports the resolver, holds no local alias literals, and passes R12', () => { +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']); - assert.equal(importsAliasResolver(binSource), true); + const localName = aliasResolverLocalName(binSource); + assert.equal(localName, 'normalizeCliCommandAlias'); + assert.equal(usageTextCallsResolver(binSource, localName!), true); assert.deepEqual(localAliasLiterals(binSource, tokens), []); }); diff --git a/scripts/layering/bin-alias-fast-path.ts b/scripts/layering/bin-alias-fast-path.ts index 9d1419742..98244ad1a 100644 --- a/scripts/layering/bin-alias-fast-path.ts +++ b/scripts/layering/bin-alias-fast-path.ts @@ -10,17 +10,25 @@ // "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. // -// Two structural facts, read from bin.ts's source text rather than by importing and running it, -// close the gap without needing to import it: +// 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 -// (fact 1 alone would still pass if bin.ts imported the function and never called it, or -// called it beside a leftover local table; fact 2 is what makes the pair exhaustive). +// so it cannot be re-declaring a parallel mapping instead of actually calling the import. +// 3. bin.ts's call to `buildCommandUsageText` receives, as an argument, a call to the LOCAL +// binding fact 1 imported — 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 binds by the import's LOCAL name, following any +// `as` alias, so `import { normalizeCliCommandAlias as resolveAlias }` still passes and an +// unrelated same-named local does not. // -// Both were false on the pre-fix bin.ts (no import; both 'long-press' and 'metrics' present as -// literals), so the pair is a real regression pin, not just a description of intent. +// 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 @@ -75,24 +83,67 @@ export function registryAliasTokens(registrySource: string): string[] { } /** - * Whether `binSource` holds a VALUE (not type-only) import of `normalizeCliCommandAlias` from - * the alias registry. 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. + * 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 `usageTextCallsResolver` below bind + * by the name bin.ts actually calls, so a renamed import (`... as resolveAlias`) still verifies, + * while a same-named unrelated local elsewhere in the file cannot be mistaken for it. */ -export function importsAliasResolver(binSource: string): boolean { +export function aliasResolverLocalName(binSource: string): string | null { const parsed = parseSync(BIN_FILE, binSource); - return parsed.module.staticImports.some((entry) => { - if (entry.moduleRequest.value !== ALIAS_REGISTRY_SPECIFIER) return false; - return entry.entries.some( - (specifier) => + 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, - ); + 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; +} + +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; +} + +/** + * Whether `binSource` calls `buildCommandUsageText` with an argument that is ITSELF a call to + * `resolverLocalName` — the composition the `--help` fast path actually needs + * (`buildCommandUsageText(normalizeCliCommandAlias(helpTarget))`), not merely both names + * appearing somewhere in the file. Import presence and literal absence (facts 1 and 2 above) + * both still hold if bin.ts imports the resolver and never calls it, or calls it on something + * unrelated while `buildCommandUsageText(helpTarget)` runs raw — this is the fact that closes + * that gap: it inspects the actual argument expression at the actual call site, not just whether + * both identifiers occur in the source. + */ +export function usageTextCallsResolver(binSource: string, resolverLocalName: string): boolean { + const parsed = parseSync(BIN_FILE, binSource); + let found = false; + visit(parsed.program, (record) => { + if (found || !isCallTo(record, 'buildCommandUsageText')) return; + const args = record['arguments']; + if (!Array.isArray(args)) return; + found = args.some((arg) => isCallTo(arg, resolverLocalName)); }); + return found; } /** diff --git a/scripts/layering/check.ts b/scripts/layering/check.ts index 0b568dae3..a11d8cbba 100644 --- a/scripts/layering/check.ts +++ b/scripts/layering/check.ts @@ -50,10 +50,11 @@ import { import { uninstallableImports, zeroDepClosureFiles, zeroDepJobs } from './zero-dep-jobs.ts'; import { ALIAS_REGISTRY_FILE, + aliasResolverLocalName, BIN_FILE, - importsAliasResolver, localAliasLiterals, registryAliasTokens, + usageTextCallsResolver, } from './bin-alias-fast-path.ts'; import { backEdgePair, @@ -379,7 +380,11 @@ function checkSessionStateOwnership(sources: ReadonlyMap): Layer /** * 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 - * two facts below, together, are what closes the gap the original drift exploited. + * 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: it + * requires the actual composition at the actual call site, bound by the import's local name. */ function checkBinAliasFastPath(sources: ReadonlyMap): LayeringViolation[] { const registrySource = sources.get(ALIAS_REGISTRY_FILE); @@ -397,7 +402,8 @@ function checkBinAliasFastPath(sources: ReadonlyMap): LayeringVi } const violations: LayeringViolation[] = []; - if (!importsAliasResolver(binSource)) { + const resolverLocalName = aliasResolverLocalName(binSource); + if (resolverLocalName === null) { violations.push({ rule: 'R12 bin-alias-fast-path', file: BIN_FILE, @@ -407,6 +413,17 @@ function checkBinAliasFastPath(sources: ReadonlyMap): LayeringVi `${ALIAS_REGISTRY_FILE} — the --help fast path cannot delegate alias resolution to the ` + 'registry without it.', }); + } else if (!usageTextCallsResolver(binSource, resolverLocalName)) { + violations.push({ + rule: 'R12 bin-alias-fast-path', + file: BIN_FILE, + line: 1, + message: + `imports normalizeCliCommandAlias (locally ${resolverLocalName}) but never passes ` + + `${resolverLocalName}(...) as the argument to buildCommandUsageText — the import alone ` + + 'does not prove the --help fast path actually delegates; call ' + + `buildCommandUsageText(${resolverLocalName}(helpTarget)) at the fast-path call site.`, + }); } const localLiterals = localAliasLiterals(binSource, registryAliasTokens(registrySource)); @@ -501,8 +518,9 @@ function report( `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()}; ` + - `${packageBoundariesSummary(repoRoot)}; and bin.ts delegates command-alias resolution ` + - `to the registry with no local alias literals (R12).\n`, + `${packageBoundariesSummary(repoRoot)}; and bin.ts imports normalizeCliCommandAlias, ` + + `actually passes it into buildCommandUsageText, and holds no local alias literals ` + + `(R12).\n`, ); return 0; } From f469d6c00c7949cc0c24ecbcb61bfaf44821d808 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 18:39:44 +0000 Subject: [PATCH 4/4] test(cli): make R12's delegation fact universal and value-bound MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous fact 3 asked whether *any* `buildCommandUsageText(resolver(...))` existed in bin.ts. That quantifier is satisfied by a decoy call while the line that actually ships resolves nothing: void buildCommandUsageText(normalizeCliCommandAlias('open')); const commandHelp = buildCommandUsageText(helpTarget); Fact 3 now requires EVERY `buildCommandUsageText` call to receive the imported resolver applied to the fast path's own help-target binding, which rejects both lines above independently. The help-target name is read from bin.ts (the variable initialized by `resolveSimpleHelpTarget`), so renaming it re-points the guard instead of disarming it. Because fact 3 claims binding identity by name, it also now rejects a local shadow of the resolver and an ambiguous second help-target declaration — a same-named local would otherwise let the composition read as delegation while calling something that resolves nothing. The predicate returns the reason rather than a boolean, so the gate names which of the several distinct failures happened. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017Rva4YGtSCAKJqH5PbpcCU --- scripts/layering/bin-alias-fast-path.test.ts | 182 +++++++++++---- scripts/layering/bin-alias-fast-path.ts | 227 ++++++++++++++++--- scripts/layering/check.ts | 32 +-- 3 files changed, 363 insertions(+), 78 deletions(-) diff --git a/scripts/layering/bin-alias-fast-path.test.ts b/scripts/layering/bin-alias-fast-path.test.ts index c7d86a3a3..78e801907 100644 --- a/scripts/layering/bin-alias-fast-path.test.ts +++ b/scripts/layering/bin-alias-fast-path.test.ts @@ -9,12 +9,36 @@ import { ALIAS_REGISTRY_FILE, aliasResolverLocalName, BIN_FILE, + countLocalBindings, + helpTargetBindingName, importsAliasResolver, localAliasLiterals, registryAliasTokens, - usageTextCallsResolver, + 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 = [ @@ -110,57 +134,138 @@ test('aliasResolverLocalName is null when there is no matching value import', () ); }); -test('usageTextCallsResolver is true for the real composition, by local name', () => { +test('helpTargetBindingName reads the binding resolveSimpleHelpTarget produces', () => { assert.equal( - usageTextCallsResolver( - 'const commandHelp = buildCommandUsageText(normalizeCliCommandAlias(helpTarget));\n', - 'normalizeCliCommandAlias', - ), - true, + helpTargetBindingName(binFixture(' buildCommandUsageText(normalizeCliCommandAlias(x));')), + 'helpTarget', ); - // Binds by whatever LOCAL name the caller passes — 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. + // 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( - usageTextCallsResolver( - 'const commandHelp = buildCommandUsageText(resolveAlias(helpTarget));\n', - 'resolveAlias', + delegationFailure( + binFixture( + ' const commandHelp = buildCommandUsageText(normalizeCliCommandAlias(helpTarget));', + ), ), - true, + 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('usageTextCallsResolver is false for a raw call, with no wrapping resolver call', () => { - assert.equal( - usageTextCallsResolver( - 'const commandHelp = buildCommandUsageText(helpTarget);\n', - 'normalizeCliCommandAlias', - ), - false, +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 (maintainer review of the original 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. These two fixtures are -// exactly that regression — the import is real and even "used", but never as the argument -// buildCommandUsageText receives — and usageTextCallsResolver must reject both. -test('usageTextCallsResolver rejects a present-but-unused import', () => { - const source = ` -import { normalizeCliCommandAlias } from './commands/cli-command-aliases.ts'; -const commandHelp = 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.equal(usageTextCallsResolver(source, aliasResolverLocalName(source)!), false); + assert.notEqual(delegationFailure(source), null); }); -test('usageTextCallsResolver rejects an import used only unrelated to buildCommandUsageText', () => { - const source = ` +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'; -void normalizeCliCommandAlias; -const commandHelp = buildCommandUsageText(helpTarget); +const commandHelp = buildCommandUsageText(normalizeCliCommandAlias(argv[1])); `; - assert.equal(importsAliasResolver(source), true); - assert.equal(usageTextCallsResolver(source, aliasResolverLocalName(source)!), false); + assert.match(delegationFailure(untraceable) ?? '', /has no variable initialized by/); }); test('localAliasLiterals reports every requested token present as a string literal', () => { @@ -203,6 +308,7 @@ test('the real tree imports the resolver, calls it into buildCommandUsageText, h assert.deepEqual(tokens, ['launch', 'long-press', 'metrics', 'relaunch', 'tap']); const localName = aliasResolverLocalName(binSource); assert.equal(localName, 'normalizeCliCommandAlias'); - assert.equal(usageTextCallsResolver(binSource, localName!), true); + 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 index 98244ad1a..e3296538f 100644 --- a/scripts/layering/bin-alias-fast-path.ts +++ b/scripts/layering/bin-alias-fast-path.ts @@ -16,15 +16,34 @@ // 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. bin.ts's call to `buildCommandUsageText` receives, as an argument, a call to the LOCAL -// binding fact 1 imported — 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 +// 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 binds by the import's LOCAL name, following any -// `as` alias, so `import { normalizeCliCommandAlias as resolveAlias }` still passes and an -// unrelated same-named local does not. +// (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 @@ -90,9 +109,10 @@ export function registryAliasTokens(registrySource: string): string[] { * `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 `usageTextCallsResolver` below bind - * by the name bin.ts actually calls, so a renamed import (`... as resolveAlias`) still verifies, - * while a same-named unrelated local elsewhere in the file cannot be mistaken for it. + * 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); @@ -116,6 +136,9 @@ 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; @@ -124,26 +147,178 @@ function isCallTo(node: unknown, calleeName: string): boolean { 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'])}>`; +} + /** - * Whether `binSource` calls `buildCommandUsageText` with an argument that is ITSELF a call to - * `resolverLocalName` — the composition the `--help` fast path actually needs - * (`buildCommandUsageText(normalizeCliCommandAlias(helpTarget))`), not merely both names - * appearing somewhere in the file. Import presence and literal absence (facts 1 and 2 above) - * both still hold if bin.ts imports the resolver and never calls it, or calls it on something - * unrelated while `buildCommandUsageText(helpTarget)` runs raw — this is the fact that closes - * that gap: it inspects the actual argument expression at the actual call site, not just whether - * both identifiers occur in the source. + * 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 usageTextCallsResolver(binSource: string, resolverLocalName: string): boolean { +export function helpTargetBindingName(binSource: string): string | null { const parsed = parseSync(BIN_FILE, binSource); - let found = false; + let name: string | null = null; visit(parsed.program, (record) => { - if (found || !isCallTo(record, 'buildCommandUsageText')) return; - const args = record['arguments']; - if (!Array.isArray(args)) return; - found = args.some((arg) => isCallTo(arg, resolverLocalName)); + 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 found; + 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; } /** diff --git a/scripts/layering/check.ts b/scripts/layering/check.ts index a11d8cbba..00eb72ae3 100644 --- a/scripts/layering/check.ts +++ b/scripts/layering/check.ts @@ -54,7 +54,7 @@ import { BIN_FILE, localAliasLiterals, registryAliasTokens, - usageTextCallsResolver, + usageTextDelegationFailure, } from './bin-alias-fast-path.ts'; import { backEdgePair, @@ -383,8 +383,11 @@ function checkSessionStateOwnership(sources: ReadonlyMap): Layer * 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: it - * requires the actual composition at the actual call site, bound by the import's local name. + * 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); @@ -413,17 +416,18 @@ function checkBinAliasFastPath(sources: ReadonlyMap): LayeringVi `${ALIAS_REGISTRY_FILE} — the --help fast path cannot delegate alias resolution to the ` + 'registry without it.', }); - } else if (!usageTextCallsResolver(binSource, resolverLocalName)) { - violations.push({ - rule: 'R12 bin-alias-fast-path', - file: BIN_FILE, - line: 1, - message: - `imports normalizeCliCommandAlias (locally ${resolverLocalName}) but never passes ` + - `${resolverLocalName}(...) as the argument to buildCommandUsageText — the import alone ` + - 'does not prove the --help fast path actually delegates; call ' + - `buildCommandUsageText(${resolverLocalName}(helpTarget)) at the fast-path call site.`, - }); + } 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));