From 9e11c327e8a6c6895f7f1eff41a6a0578462e349 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 5 Sep 2026 08:37:34 +0200 Subject: [PATCH 1/8] refactor(cli): let cli-help resolve the --help alias itself bin.ts's --help fast path composed buildCommandUsageText(normalizeCliCommandAlias(helpTarget)) inline, which let a future edit call buildCommandUsageText raw without anyone noticing until an alias's help silently dropped back to a full CLI bootstrap (the regression #1641 fixed). Move the composition into cli-schema/cli-help.ts as resolveHelpTargetUsageText, so bin.ts just calls one function that owns its own alias normalization; bin.ts no longer imports the alias registry at all. Retargets cli-help-alias-fast-path.test.ts at the new function (same three cases) and adds a process-level smoke test asserting `tap --help`/`launch --help` stdout is byte-identical to `press --help`/`open --help`. Seen red by temporarily removing the `tap` alias from CLI_COMMAND_ALIASES (both fast and slow paths lose the alias, producing an "Unknown command: tap" mismatch); green again after restoring it. Verified manually: `node --experimental-strip-types src/bin.ts tap --help` stays byte-identical to `press --help`, and `launch --help` to `open --help`; `rotate --help` still falls through to the retired-command error. --- src/bin.ts | 6 +-- .../cli-help-alias-fast-path.test.ts | 39 +++++++++---------- src/cli-schema/cli-help.ts | 13 +++++++ test/integration/smoke-cli.test.ts | 14 +++++++ 4 files changed, 47 insertions(+), 25 deletions(-) diff --git a/src/bin.ts b/src/bin.ts index 1be4250272..a6fd860624 100644 --- a/src/bin.ts +++ b/src/bin.ts @@ -1,5 +1,3 @@ -import { normalizeCliCommandAlias } from './commands/cli-command-aliases.ts'; - const argv = process.argv.slice(2); declare const __AGENT_DEVICE_VERSION__: string; @@ -51,12 +49,12 @@ function runHelpFastPath(argv: string[]): boolean { if (helpTarget === undefined) return false; import('./cli-schema/cli-help.ts') - .then(({ buildCommandUsageText, buildUsageText }) => { + .then(({ resolveHelpTargetUsageText, buildUsageText }) => { if (helpTarget === null) { process.stdout.write(`${buildUsageText()}\n`); return; } - const commandHelp = buildCommandUsageText(normalizeCliCommandAlias(helpTarget)); + const commandHelp = resolveHelpTargetUsageText(helpTarget); if (commandHelp) { process.stdout.write(commandHelp); return; diff --git a/src/cli-schema/cli-help-alias-fast-path.test.ts b/src/cli-schema/cli-help-alias-fast-path.test.ts index 891bafcd77..a7e2cff054 100644 --- a/src/cli-schema/cli-help-alias-fast-path.test.ts +++ b/src/cli-schema/cli-help-alias-fast-path.test.ts @@ -1,21 +1,18 @@ -// 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). +// Pins `resolveHelpTargetUsageText`, the function `bin.ts`'s `--help` fast path calls to +// resolve a help target through alias normalization before rendering usage text. Before this +// fix, `bin.ts` used its own hand-written two-entry table instead of the shared alias registry, +// 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 function 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 +// function (see test/integration/smoke-cli.test.ts for the process-level proof: `tap --help` +// spawned as a real process is byte-identical to `press --help`). 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 { resolveHelpTargetUsageText } from './cli-help.ts'; +import { cliAliasesForCommand } from '../commands/cli-command-aliases.ts'; import { listCliCommandNames } from '../command-catalog.ts'; test('alias help output matches its canonical command', () => { @@ -26,8 +23,8 @@ test('alias help output matches its canonical command', () => { ['long-press', 'longpress'], ]; for (const [alias, canonical] of cases) { - const aliasHelp = buildCommandUsageText(normalizeCliCommandAlias(alias)); - const canonicalHelp = buildCommandUsageText(canonical); + const aliasHelp = resolveHelpTargetUsageText(alias); + const canonicalHelp = resolveHelpTargetUsageText(canonical); assert.notEqual(aliasHelp, null, `expected help text for alias "${alias}"`); assert.equal( aliasHelp, @@ -47,8 +44,8 @@ test('every CLI alias resolves to a command with help text', () => { ); 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}"`); + const help = resolveHelpTargetUsageText(alias); + assert.notEqual(help, null, `expected resolveHelpTargetUsageText to resolve alias "${alias}"`); } }); @@ -57,6 +54,6 @@ test('rotate still has no fast-path help', () => { // 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')); + const help = resolveHelpTargetUsageText('rotate'); assert.equal(help, null); }); diff --git a/src/cli-schema/cli-help.ts b/src/cli-schema/cli-help.ts index 8f075211dc..a0c51ec7d3 100644 --- a/src/cli-schema/cli-help.ts +++ b/src/cli-schema/cli-help.ts @@ -4,6 +4,7 @@ import { MAESTRO_COMPATIBILITY_ADR_URL, MAESTRO_COMPATIBILITY_ISSUE_URL, } from '@agent-device/maestro'; +import { normalizeCliCommandAlias } from '../commands/cli-command-aliases.ts'; import { helpBody } from '../commands/command-text.ts'; import { DEVICE_SELECTION_FLAG_KEYS, @@ -1212,6 +1213,18 @@ ${helpBody(schema.text)}${flagsSections} `; } +/** + * `bin.ts`'s `--help` fast path calls this instead of composing + * `buildCommandUsageText(normalizeCliCommandAlias(...))` itself, so an alias like `tap` or + * `launch` cannot silently miss alias resolution by calling `buildCommandUsageText` raw. A + * command name the alias registry does not recognize (including `rotate`, which is retired + * rather than aliased) passes through unchanged and falls back to `null` exactly as + * `buildCommandUsageText` would. + */ +export function resolveHelpTargetUsageText(helpTarget: string): string | null { + return buildCommandUsageText(normalizeCliCommandAlias(helpTarget)); +} + /** * Topic-id registry view for conformance tooling: the help benchmark's topic * coverage gate enumerates this instead of a hand-maintained list, so adding a diff --git a/test/integration/smoke-cli.test.ts b/test/integration/smoke-cli.test.ts index 8d98b19bc2..bba6aaf1be 100644 --- a/test/integration/smoke-cli.test.ts +++ b/test/integration/smoke-cli.test.ts @@ -39,3 +39,17 @@ test('cli without command prints usage and exits 1', () => { assert.equal(result.status, 1, result.stderr); assert.match(result.stdout, /agent-device /i); }); + +test('alias --help fast path is byte-identical to its canonical command', () => { + const tap = runCli(['tap', '--help']); + const press = runCli(['press', '--help']); + assert.equal(tap.status, 0, tap.stderr); + assert.equal(press.status, 0, press.stderr); + assert.equal(tap.stdout, press.stdout); + + const launch = runCli(['launch', '--help']); + const open = runCli(['open', '--help']); + assert.equal(launch.status, 0, launch.stderr); + assert.equal(open.status, 0, open.stderr); + assert.equal(launch.stdout, open.stdout); +}); From 50e002da1add048eff5705fcd3dfc591eb64574e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 5 Sep 2026 08:37:50 +0200 Subject: [PATCH 2/8] chore(gates): retire R12 now that cli-help owns its own alias resolution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit bin.ts can no longer compose buildCommandUsageText and normalizeCliCommandAlias incorrectly because it doesn't hold either import any more — resolveHelpTargetUsageText in cli-schema/cli-help.ts is the only call site, and cli-help-alias-fast-path.test.ts plus the new smoke-cli process test pin it. The static R12 checker existed only to prove that composition from source text; delete it along with its rule wiring in check.ts (rule function, import, LAYERING_RULE_IDS/LAYERING_RULES entries, header comment, summary string). Drops scripts/layering/bin-alias-fast-path.ts (352 lines) and its test (311 lines). Updates the two stale references left behind: record-runtime-mechanics-policy.ts's comparison to R12's "delegate to your single owner" shape, and check-wiring.test.ts's header, which named bin-alias-fast-path.test.ts as the seam it protects. rule-ids.ts discovers rule ids by scanning source text rather than a hand-maintained list, so no entry there needed updating. Verified: pnpm check:layering green (175/175), including check-wiring.test.ts and rule-ids.test.ts; pnpm check:quick (lint + typecheck) clean; scripts/__tests__/eager-closure-budgets.test.ts (418/418) unaffected, since neither bin.ts nor cli-help.ts sits in any HUB_ENTRY_FILES or facade closure — both files reach cli-help.ts only through a dynamic import. --- scripts/layering/bin-alias-fast-path.test.ts | 311 ---------------- scripts/layering/bin-alias-fast-path.ts | 352 ------------------ scripts/layering/check-wiring.test.ts | 9 +- scripts/layering/check.ts | 89 +---- .../record-runtime-mechanics-policy.ts | 4 +- 5 files changed, 9 insertions(+), 756 deletions(-) delete mode 100644 scripts/layering/bin-alias-fast-path.test.ts delete mode 100644 scripts/layering/bin-alias-fast-path.ts diff --git a/scripts/layering/bin-alias-fast-path.test.ts b/scripts/layering/bin-alias-fast-path.test.ts deleted file mode 100644 index f8fb38bcd2..0000000000 --- a/scripts/layering/bin-alias-fast-path.test.ts +++ /dev/null @@ -1,311 +0,0 @@ -// 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: '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', - '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', () => { - // A stale bin.ts shape with a hand-written alias table. - const preFixBinSource = ` -function normalizeHelpTarget(command) { - if (command === 'long-press') return 'longpress'; - return command; -} -`; - assert.deepEqual( - localAliasLiterals(preFixBinSource, ['long-press', 'tap', 'launch', 'relaunch']), - ['long-press'], - ); -}); - -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', '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', '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 deleted file mode 100644 index 7a3793ddfb..0000000000 --- a/scripts/layering/bin-alias-fast-path.ts +++ /dev/null @@ -1,352 +0,0 @@ -// Catches: bin.ts's --help fast path re-declaring its own alias table instead of calling the -// real registry — the exact silent-drift bug #1618-adjacent produced, where tap/launch/ -// relaunch fell out of a hand-written table and paid a full CLI bootstrap for static help -// text. bin.ts runs unconditionally on import, so no unit test can import and exercise it -// directly; only reading its source text structurally can catch a regression. -// Evidence: d85072d935 (#1641) routed command aliases through the help fast path; 74a70f1764 -// (#2046) removed next-major compatibility surfaces bin.ts once carried alongside it. -// Cost: 650 LOC (339 rule + 311 test). -// Kill criterion: none enforced today; retire only by maintainer decision that bin.ts's --help -// fast path no longer needs to delegate to the real alias registry — moot once the fast path -// is deleted or its resolution is inlined into commands/cli-command-aliases.ts, leaving no -// second call site. -// -// 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`, `launch`) 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 'launch' 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, contracts-implementation-policy.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 `contracts-implementation-policy.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-wiring.test.ts b/scripts/layering/check-wiring.test.ts index 7658768b49..e8d90489da 100644 --- a/scripts/layering/check-wiring.test.ts +++ b/scripts/layering/check-wiring.test.ts @@ -1,7 +1,8 @@ -// The seam bin-alias-fast-path.test.ts calls out as untested — "the check.ts wiring that turns it -// into a violation." The registry makes duplicate registration unrepresentable on its own (an -// object holds a key once, and oxlint's no-dupe-keys rejects the attempt), so what is left to check -// is the other direction: that the catalog and the registry still describe the same set of rules. +// A rule policy module can compute violations correctly and still never run, if its entry in +// LAYERING_RULES is missing or its id is missing from LAYERING_RULE_IDS. The registry makes +// duplicate registration unrepresentable on its own (an object holds a key once, and oxlint's +// no-dupe-keys rejects the attempt), so what is left to check is the other direction: that the +// catalog and the registry still describe the same set of rules. // scripts/ is outside tsconfig.json's `include`, so the Record's exhaustiveness is an editor // signal, not a CI gate — this test is what fails the build when wiring goes missing. diff --git a/scripts/layering/check.ts b/scripts/layering/check.ts index c9a95d40e1..ebbaef4e14 100644 --- a/scripts/layering/check.ts +++ b/scripts/layering/check.ts @@ -31,9 +31,6 @@ // 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. // - Over PLATFORM PACKAGE COMPOSITION: six private metadata façades meet at the exact root // composition file; premature implementation loading and forbidden cross-boundary edges fail (R13). // - Over REQUEST-BOUND RUNTIME EXECUTION: facts remain the only admission authority and daemon @@ -56,14 +53,6 @@ import { SESSION_STATE_FIELD_OWNERS, STORE_OWNED_SESSION_STATE_FIELDS, } from './session-state.ts'; -import { - ALIAS_REGISTRY_FILE, - aliasResolverLocalName, - BIN_FILE, - localAliasLiterals, - registryAliasTokens, - usageTextDelegationFailure, -} from './bin-alias-fast-path.ts'; import { backEdgePair, findValueImportCycles, @@ -447,75 +436,6 @@ 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; -} - function report( files: readonly string[], violations: readonly LayeringViolation[], @@ -532,11 +452,8 @@ function report( `(R9); ${daemonModularitySummary()}; ` + `${packageBoundariesSummary(repoRoot)}; ${platformPackagePolicySummary()}; ` + `runtime facts remain the only device-command admission authority and daemon code cannot ` + - `manufacture narrowed runtime proof (R66); R65 keeps production src/daemon free of concrete ` + - `platform imports in every executable and type-only form; and bin.ts imports ` + - `normalizeCliCommandAlias, ` + - `actually passes it into buildCommandUsageText, and holds no local alias literals ` + - `(R12).\n`, + `manufacture narrowed runtime proof (R66); and R65 keeps production src/daemon free of ` + + `concrete platform imports in every executable and type-only form.\n`, ); return 0; } @@ -598,7 +515,6 @@ export const LAYERING_RULE_IDS = [ 'session-state-ownership', 'daemon-modularity-ratchets', 'daemon-platform-boundary', - 'bin-alias-fast-path', 'package-boundaries', 'platform-package-policy', 'retired-platforms-zone', @@ -640,7 +556,6 @@ export const LAYERING_RULES: Readonly> = { ], 'daemon-platform-boundary': (context) => checkDaemonPlatformBoundary([...context.sources].map(([path, source]) => ({ path, source }))), - 'bin-alias-fast-path': (context) => checkBinAliasFastPath(context.sources), 'package-boundaries': () => checkPackageBoundaries(repoRoot), 'platform-package-policy': (context) => checkPlatformPackagePolicy( diff --git a/scripts/layering/record-runtime-mechanics-policy.ts b/scripts/layering/record-runtime-mechanics-policy.ts index 44e1e2ad35..cf80728d1a 100644 --- a/scripts/layering/record-runtime-mechanics-policy.ts +++ b/scripts/layering/record-runtime-mechanics-policy.ts @@ -1,6 +1,6 @@ // Catches: the daemon record owner reaching past its declared coordinators to spawn or poll a -// native process/timer directly — the same "delegate to your single owner" shape as R7 and -// R12, applied to record's runtime mechanics; a type check cannot see this because runCmd and +// native process/timer directly — the same "delegate to your single owner" shape as R7, +// applied to record's runtime mechanics; a type check cannot see this because runCmd and // setInterval are both fully typed, legal calls from anywhere. // Evidence: 1b2e786128 (#1724) moved screen recording onto the platform runtime, the migration // this ownership boundary protects against regressing. From 0753531992887fa8b715a0a38a76848b6e061602 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 5 Sep 2026 08:56:24 +0200 Subject: [PATCH 3/8] test(cli): pin the alias help fast path with a coverage-based oracle The byte-identical stdout test cannot fail when the fast path is bypassed: src/cli.ts's slow path resolves the same alias and writes the identical string, so a reintroduced hand-written table in bin.ts (the exact shape of #1641) would still pass it. Add a second process-level test that runs `tap`/`launch --help` and `rotate --help` with NODE_V8_COVERAGE set and reads the subprocess's own coverage report for src/cli/process-entry.ts, the one module runCli's slow path loads and the fast path never does. Seen red: forcing the fast path to always fall through to runCli (simulating the reintroduced-table bug) failed this test (bootstrappedFullCli true where false was expected) while the byte-identical test stayed green; reverted and confirmed both green. --- test/integration/smoke-cli.test.ts | 58 ++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/test/integration/smoke-cli.test.ts b/test/integration/smoke-cli.test.ts index bba6aaf1be..9a11264232 100644 --- a/test/integration/smoke-cli.test.ts +++ b/test/integration/smoke-cli.test.ts @@ -1,5 +1,8 @@ import test from 'node:test'; import assert from 'node:assert/strict'; +import { mkdtempSync, readFileSync, readdirSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; import { runCmdSync } from '@agent-device/host-kit/command'; function runCli(args: string[]): { status: number; stdout: string; stderr: string } { @@ -11,6 +14,45 @@ function runCli(args: string[]): { status: number; stdout: string; stderr: strin return { status: result.exitCode, stdout: result.stdout ?? '', stderr: result.stderr ?? '' }; } +// `runCli` (src/bin.ts) reaches the full CLI bootstrap only through one dynamic import: +// `runCli(argv)` -> `import('./cli/process-entry.ts')`. The --help fast path never reaches +// it. That makes this file's own coverage report (collected via NODE_V8_COVERAGE on the +// subprocess) an independent, byte-output-blind signal for which path actually ran: a +// reintroduced hand-written alias table in bin.ts, or a `resolveHelpTargetUsageText` that +// returns null for a real alias, falls through to `runCli` and flips this from false to +// true even though stdout would stay byte-identical to the canonical command's --help. +const PROCESS_ENTRY_MARKER = 'src/cli/process-entry.ts'; + +function runCliTrackingCoverage(args: string[]): { + status: number; + stdout: string; + stderr: string; + bootstrappedFullCli: boolean; +} { + const coverageDir = mkdtempSync(join(tmpdir(), 'agent-device-cli-coverage-')); + try { + const result = runCmdSync( + process.execPath, + ['--experimental-strip-types', 'src/bin.ts', ...args], + { allowFailure: true, env: { ...process.env, NODE_V8_COVERAGE: coverageDir } }, + ); + const bootstrappedFullCli = readdirSync(coverageDir).some((file) => { + const report = JSON.parse(readFileSync(join(coverageDir, file), 'utf8')) as { + result: Array<{ url: string }>; + }; + return report.result.some((entry) => entry.url.includes(PROCESS_ENTRY_MARKER)); + }); + return { + status: result.exitCode, + stdout: result.stdout ?? '', + stderr: result.stderr ?? '', + bootstrappedFullCli, + }; + } finally { + rmSync(coverageDir, { recursive: true, force: true }); + } +} + test('cli --help returns usage', () => { const result = runCli(['--help']); assert.equal(result.status, 0, result.stderr); @@ -53,3 +95,19 @@ test('alias --help fast path is byte-identical to its canonical command', () => assert.equal(open.status, 0, open.stderr); assert.equal(launch.stdout, open.stdout); }); + +test('alias --help fast path bypasses the full CLI bootstrap', () => { + const tap = runCliTrackingCoverage(['tap', '--help']); + assert.equal(tap.status, 0, tap.stderr); + assert.equal(tap.bootstrappedFullCli, false); + + const launch = runCliTrackingCoverage(['launch', '--help']); + assert.equal(launch.status, 0, launch.stderr); + assert.equal(launch.bootstrappedFullCli, false); + + // Control: a retired/unmapped command must still fall through to the full CLI + // bootstrap, proving the coverage signal actually distinguishes the two paths + // instead of reading false unconditionally. + const rotate = runCliTrackingCoverage(['rotate', '--help']); + assert.equal(rotate.bootstrappedFullCli, true); +}); From 1dab979e2ed041ccb24bb11850251bb528772254 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 5 Sep 2026 08:56:32 +0200 Subject: [PATCH 4/8] test(cli): restore an independent oracle for alias help parity The canonical side of "alias help output matches its canonical command" also called resolveHelpTargetUsageText, so the assertion became self-consistency: a degenerate normalizer that maps every input to one canonical command would make aliasHelp and canonicalHelp equal for every case. Compare resolveHelpTargetUsageText(alias) against buildCommandUsageText(canonical) (no alias normalization on the canonical side) instead, restoring the original two-source oracle. Seen red: pointing resolveHelpTargetUsageText at a degenerate `return buildCommandUsageText('press')` failed this test ("launch --help" no longer byte-identical to "open --help"); reverted and confirmed green. --- src/cli-schema/cli-help-alias-fast-path.test.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/cli-schema/cli-help-alias-fast-path.test.ts b/src/cli-schema/cli-help-alias-fast-path.test.ts index a7e2cff054..d411f06f2c 100644 --- a/src/cli-schema/cli-help-alias-fast-path.test.ts +++ b/src/cli-schema/cli-help-alias-fast-path.test.ts @@ -11,7 +11,7 @@ // spawned as a real process is byte-identical to `press --help`). import { test } from 'vitest'; import assert from 'node:assert/strict'; -import { resolveHelpTargetUsageText } from './cli-help.ts'; +import { buildCommandUsageText, resolveHelpTargetUsageText } from './cli-help.ts'; import { cliAliasesForCommand } from '../commands/cli-command-aliases.ts'; import { listCliCommandNames } from '../command-catalog.ts'; @@ -24,7 +24,12 @@ test('alias help output matches its canonical command', () => { ]; for (const [alias, canonical] of cases) { const aliasHelp = resolveHelpTargetUsageText(alias); - const canonicalHelp = resolveHelpTargetUsageText(canonical); + // The canonical side deliberately calls `buildCommandUsageText` directly (no alias + // normalization), not `resolveHelpTargetUsageText`, so this keeps two independent + // sources instead of comparing one composition against itself. If both sides went + // through `resolveHelpTargetUsageText`, a degenerate normalizer that maps every + // input to one canonical command would still make every case pass. + const canonicalHelp = buildCommandUsageText(canonical); assert.notEqual(aliasHelp, null, `expected help text for alias "${alias}"`); assert.equal( aliasHelp, From 01b4d9b17ca8d4f8c262b5be1cc98abe79588593 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 5 Sep 2026 08:56:39 +0200 Subject: [PATCH 5/8] refactor(mcp): route the help tool through resolveHelpTargetUsageText server-guide.ts's help tool composed buildCommandUsageText(normalizeCliCommandAlias(topic)) inline, the same composition bin.ts held before this PR moved it into cli-help.ts. That left a second hand-written call site the R12 gate's own kill criterion said had to be gone before retirement was moot. Call resolveHelpTargetUsageText(topic) instead; behavior is unchanged (manually confirmed tap/press and rotate topics still match) since it's the same composition, and no closure/layering change since server-guide.ts already imports cli-help.ts statically. --- src/mcp/server-guide.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/mcp/server-guide.ts b/src/mcp/server-guide.ts index 8e81c4f8ef..00a8320082 100644 --- a/src/mcp/server-guide.ts +++ b/src/mcp/server-guide.ts @@ -1,7 +1,6 @@ import type { JsonSchema } from '../commands/command-contract.ts'; -import { buildCommandUsageText, buildUsageText, helpTopicIds } from '../cli-schema/cli-help.ts'; +import { buildUsageText, helpTopicIds, resolveHelpTargetUsageText } from '../cli-schema/cli-help.ts'; import { listCliCommandNames } from '../command-catalog.ts'; -import { normalizeCliCommandAlias } from '../commands/cli-command-aliases.ts'; import { listMcpExposedCommandNames } from '../core/command-descriptor/registry.ts'; import type { ToolResult } from './command-tools.ts'; @@ -62,7 +61,7 @@ export function callHelpTool(input: Record): ToolResult { if (topic !== undefined && typeof topic !== 'string') { return textResult('Expected topic to be a string.', true); } - const text = topic ? buildCommandUsageText(normalizeCliCommandAlias(topic)) : buildUsageText(); + const text = topic ? resolveHelpTargetUsageText(topic) : buildUsageText(); if (text === null) { return textResult(`Unknown help topic: ${topic}. ${HELP_TOOL.description}`, true); } From 0b83a38c3b83537117a00f4ce7ad8ed5e24d72e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 5 Sep 2026 08:57:58 +0200 Subject: [PATCH 6/8] style: apply oxfmt --- src/mcp/server-guide.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/mcp/server-guide.ts b/src/mcp/server-guide.ts index 00a8320082..6db7eb21a2 100644 --- a/src/mcp/server-guide.ts +++ b/src/mcp/server-guide.ts @@ -1,5 +1,9 @@ import type { JsonSchema } from '../commands/command-contract.ts'; -import { buildUsageText, helpTopicIds, resolveHelpTargetUsageText } from '../cli-schema/cli-help.ts'; +import { + buildUsageText, + helpTopicIds, + resolveHelpTargetUsageText, +} from '../cli-schema/cli-help.ts'; import { listCliCommandNames } from '../command-catalog.ts'; import { listMcpExposedCommandNames } from '../core/command-descriptor/registry.ts'; import type { ToolResult } from './command-tools.ts'; From 313574982031a289d84dc9993d912521760fff12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 5 Sep 2026 18:42:20 +0200 Subject: [PATCH 7/8] test(cli): prove the help fast path for every registered alias --- test/integration/smoke-cli.test.ts | 61 +++++++++++++++++++++--------- 1 file changed, 43 insertions(+), 18 deletions(-) diff --git a/test/integration/smoke-cli.test.ts b/test/integration/smoke-cli.test.ts index 9a11264232..70f7d90e52 100644 --- a/test/integration/smoke-cli.test.ts +++ b/test/integration/smoke-cli.test.ts @@ -4,6 +4,15 @@ import { mkdtempSync, readFileSync, readdirSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { runCmdSync } from '@agent-device/host-kit/command'; +import { cliAliasesForCommand } from '../../src/commands/cli-command-aliases.ts'; +import { listCliCommandNames } from '../../src/command-catalog.ts'; + +// Derived from the alias registry itself (not hard-coded) so a future alias is +// exercised automatically, and so a partial hand-written mapping that covers only +// today's aliases cannot silently regain a fast-path gap for an alias added later. +const KNOWN_ALIASES: ReadonlyArray = listCliCommandNames().flatMap( + (command) => cliAliasesForCommand(command).map((entry) => [entry.alias, command] as const), +); function runCli(args: string[]): { status: number; stdout: string; stderr: string } { const result = runCmdSync( @@ -82,28 +91,44 @@ test('cli without command prints usage and exits 1', () => { assert.match(result.stdout, /agent-device /i); }); -test('alias --help fast path is byte-identical to its canonical command', () => { - const tap = runCli(['tap', '--help']); - const press = runCli(['press', '--help']); - assert.equal(tap.status, 0, tap.stderr); - assert.equal(press.status, 0, press.stderr); - assert.equal(tap.stdout, press.stdout); +test('known alias registry is non-empty and includes every known alias', () => { + // Belt-and-braces pin: if the registry were ever emptied by mistake, the two + // tests below would vacuously pass (an empty `for` loop asserts nothing). Pin + // both the non-empty condition and the concrete alias set so that failure is + // loud instead of silent. + assert.ok(KNOWN_ALIASES.length > 0, 'expected at least one alias to exercise this test'); + const aliasNames = KNOWN_ALIASES.map(([alias]) => alias).sort(); + assert.deepEqual(aliasNames, ['launch', 'long-press', 'relaunch', 'tap'].sort()); +}); - const launch = runCli(['launch', '--help']); - const open = runCli(['open', '--help']); - assert.equal(launch.status, 0, launch.stderr); - assert.equal(open.status, 0, open.stderr); - assert.equal(launch.stdout, open.stdout); +test('alias --help fast path is byte-identical to its canonical command', () => { + // The canonical side is always a direct invocation of the canonical command name + // (never routed through the alias resolver), so this stays an independent oracle: + // a degenerate resolver that maps every alias to one fixed command would still + // fail here for the aliases whose canonical command differs. + for (const [alias, canonical] of KNOWN_ALIASES) { + const aliasResult = runCli([alias, '--help']); + const canonicalResult = runCli([canonical, '--help']); + assert.equal(aliasResult.status, 0, `${alias} --help: ${aliasResult.stderr}`); + assert.equal(canonicalResult.status, 0, `${canonical} --help: ${canonicalResult.stderr}`); + assert.equal( + aliasResult.stdout, + canonicalResult.stdout, + `expected "${alias} --help" to be byte-identical to "${canonical} --help"`, + ); + } }); test('alias --help fast path bypasses the full CLI bootstrap', () => { - const tap = runCliTrackingCoverage(['tap', '--help']); - assert.equal(tap.status, 0, tap.stderr); - assert.equal(tap.bootstrappedFullCli, false); - - const launch = runCliTrackingCoverage(['launch', '--help']); - assert.equal(launch.status, 0, launch.stderr); - assert.equal(launch.bootstrappedFullCli, false); + for (const [alias] of KNOWN_ALIASES) { + const result = runCliTrackingCoverage([alias, '--help']); + assert.equal(result.status, 0, `${alias} --help: ${result.stderr}`); + assert.equal( + result.bootstrappedFullCli, + false, + `expected "${alias} --help" to bypass ${PROCESS_ENTRY_MARKER}`, + ); + } // Control: a retired/unmapped command must still fall through to the full CLI // bootstrap, proving the coverage signal actually distinguishes the two paths From 08031734734b6558b2b0125416fc259de11f7ad7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Sat, 5 Sep 2026 19:41:13 +0200 Subject: [PATCH 8/8] refactor(cli): make the process entry importable and test it directly bin.ts ran its dispatch at import time, so the only way to prove that an alias --help never loads the full CLI was to spawn the process under NODE_V8_COVERAGE and grep the report for process-entry.ts. That oracle needed a paragraph to justify; the code was wrong, not the comment. The dispatch now lives in src/cli/entry.ts as runEntry(argv, modules, io), with the five lazy imports injected by bin.ts. entry.test.ts drives it with recording loaders and the real help module: every registry alias prints its canonical help with only the help module loaded, an unknown topic falls through to the CLI loader, --version, bare usage, mcp, and startup failures each have one case. The subprocess coverage machinery, the alias table pin, and the multi-line comments are gone; the smoke test keeps one registry-derived byte-identical alias --help check against the real bin.ts. Seen red: hand-routing long-press and relaunch to the CLI loader inside entry.ts failed "every registered alias prints its canonical help without loading the CLI"; restored. --- src/bin.ts | 142 +++--------------- .../cli-help-alias-fast-path.test.ts | 62 ++------ src/cli-schema/cli-help.ts | 9 +- src/cli/entry.test.ts | 111 ++++++++++++++ src/cli/entry.ts | 86 +++++++++++ test/integration/smoke-cli.test.ts | 101 ++----------- 6 files changed, 238 insertions(+), 273 deletions(-) create mode 100644 src/cli/entry.test.ts create mode 100644 src/cli/entry.ts diff --git a/src/bin.ts b/src/bin.ts index a6fd860624..34d30c9f19 100644 --- a/src/bin.ts +++ b/src/bin.ts @@ -1,124 +1,24 @@ -const argv = process.argv.slice(2); +import { runEntry } from './cli/entry.ts'; declare const __AGENT_DEVICE_VERSION__: string; -if (runFastPath(argv)) { - // Fast path owns process output and exit behavior. -} else if (argv[0] === 'mcp' && !argv.includes('--help') && !argv.includes('-h')) { - import('./mcp/server.ts') - .then(({ runAgentDeviceMcpServer }) => runAgentDeviceMcpServer()) - .catch(handleStartupError); -} else { - runCli(argv); -} - -function runFastPath(argv: string[]): boolean { - return runVersionFastPath(argv) || runNoCommandFastPath(argv) || runHelpFastPath(argv); -} - -function runVersionFastPath(argv: string[]): boolean { - if (argv.length !== 1 || !isVersionFlag(argv[0])) return false; - if (typeof __AGENT_DEVICE_VERSION__ === 'string') { - process.stdout.write(`${__AGENT_DEVICE_VERSION__}\n`); - return true; - } - import('@agent-device/host-kit/version') - .then(({ readVersion }) => { - process.stdout.write(`${readVersion()}\n`); - }) - .catch(handleStartupError); - return true; -} - -function runNoCommandFastPath(argv: string[]): boolean { - if (argv.length !== 0) return false; - import('./cli-schema/cli-help.ts') - .then(async ({ buildUsageText }) => { - process.stdout.write(`${buildUsageText()}\n`); - // #1596: exitAfterFlush (not a bare process.exit) so the full usage - // text reaches a piped caller before the process terminates. - const { exitAfterFlush } = await import('./cli/process-exit.ts'); - await exitAfterFlush(1); - }) - .catch(handleStartupError); - return true; -} - -function runHelpFastPath(argv: string[]): boolean { - const helpTarget = resolveSimpleHelpTarget(argv); - if (helpTarget === undefined) return false; - - import('./cli-schema/cli-help.ts') - .then(({ resolveHelpTargetUsageText, buildUsageText }) => { - if (helpTarget === null) { - process.stdout.write(`${buildUsageText()}\n`); - return; - } - const commandHelp = resolveHelpTargetUsageText(helpTarget); - if (commandHelp) { - process.stdout.write(commandHelp); - return; - } - // Unknown help topics still need full CLI parsing for the normal error path. - runCli(argv); - }) - .catch(handleStartupError); - return true; -} - -function resolveSimpleHelpTarget(argv: string[]): string | null | undefined { - switch (argv.length) { - case 1: - return resolveSingleArgHelpTarget(argv[0]); - case 2: - return resolveTwoArgHelpTarget(argv[0], argv[1]); - default: - return undefined; - } -} - -function resolveSingleArgHelpTarget(arg: string | undefined): null | undefined { - if (arg === 'help') return null; - return isHelpFlag(arg) ? null : undefined; -} - -function resolveTwoArgHelpTarget( - command: string | undefined, - helpArg: string | undefined, -): string | undefined { - if (isHelpCommand(command)) return helpArg; - return resolveTrailingHelpTarget(command, helpArg); -} - -function resolveTrailingHelpTarget( - command: string | undefined, - helpArg: string | undefined, -): string | undefined { - return isHelpFlag(helpArg) ? command : undefined; -} - -function isHelpCommand(command: string | undefined): boolean { - return command === 'help'; -} - -function isHelpFlag(arg: string | undefined): boolean { - return arg === '--help' || arg === '-h'; -} - -function isVersionFlag(arg: string | undefined): boolean { - return arg === '--version' || arg === '-V'; -} - -function runCli(argv: string[]): void { - import('./cli/process-entry.ts') - .then(({ runCliProcess }) => runCliProcess(argv)) - .catch(handleStartupError); -} - -function handleStartupError(error: unknown): void { - process.stderr.write(`${error instanceof Error ? error.message : String(error)}\n`); - // #1596: exitAfterFlush so the message above isn't dropped on a piped stderr. - import('./cli/process-exit.ts') - .then(({ exitAfterFlush }) => exitAfterFlush(1)) - .catch(() => process.exit(1)); -} +runEntry( + process.argv.slice(2), + { + help: () => import('./cli-schema/cli-help.ts'), + cli: () => import('./cli/process-entry.ts'), + mcp: () => import('./mcp/server.ts'), + version: () => import('@agent-device/host-kit/version'), + processExit: () => import('./cli/process-exit.ts'), + }, + { + bundledVersion: + typeof __AGENT_DEVICE_VERSION__ === 'string' ? __AGENT_DEVICE_VERSION__ : undefined, + stdout: (text) => { + process.stdout.write(text); + }, + stderr: (text) => { + process.stderr.write(text); + }, + }, +).catch(() => process.exit(1)); diff --git a/src/cli-schema/cli-help-alias-fast-path.test.ts b/src/cli-schema/cli-help-alias-fast-path.test.ts index d411f06f2c..b723686488 100644 --- a/src/cli-schema/cli-help-alias-fast-path.test.ts +++ b/src/cli-schema/cli-help-alias-fast-path.test.ts @@ -1,64 +1,22 @@ -// Pins `resolveHelpTargetUsageText`, the function `bin.ts`'s `--help` fast path calls to -// resolve a help target through alias normalization before rendering usage text. Before this -// fix, `bin.ts` used its own hand-written two-entry table instead of the shared alias registry, -// 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 function 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 -// function (see test/integration/smoke-cli.test.ts for the process-level proof: `tap --help` -// spawned as a real process is byte-identical to `press --help`). import { test } from 'vitest'; import assert from 'node:assert/strict'; import { buildCommandUsageText, resolveHelpTargetUsageText } from './cli-help.ts'; import { cliAliasesForCommand } 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'], - ]; - for (const [alias, canonical] of cases) { - const aliasHelp = resolveHelpTargetUsageText(alias); - // The canonical side deliberately calls `buildCommandUsageText` directly (no alias - // normalization), not `resolveHelpTargetUsageText`, so this keeps two independent - // sources instead of comparing one composition against itself. If both sides went - // through `resolveHelpTargetUsageText`, a degenerate normalizer that maps every - // input to one canonical command would still make every case pass. - 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"`, - ); - } -}); +const ALIASES = listCliCommandNames().flatMap((command) => + cliAliasesForCommand(command).map((entry) => [entry.alias, command] as const), +); -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) { +test('every registered alias resolves to its canonical command help', () => { + assert.ok(ALIASES.length > 0); + for (const [alias, canonical] of ALIASES) { const help = resolveHelpTargetUsageText(alias); - assert.notEqual(help, null, `expected resolveHelpTargetUsageText to resolve alias "${alias}"`); + assert.notEqual(help, null, alias); + assert.equal(help, buildCommandUsageText(canonical), 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 = resolveHelpTargetUsageText('rotate'); - assert.equal(help, null); +test('a retired command has no help text and is left to the full CLI', () => { + assert.equal(resolveHelpTargetUsageText('rotate'), null); }); diff --git a/src/cli-schema/cli-help.ts b/src/cli-schema/cli-help.ts index a0c51ec7d3..da5d3c00b1 100644 --- a/src/cli-schema/cli-help.ts +++ b/src/cli-schema/cli-help.ts @@ -1213,14 +1213,7 @@ ${helpBody(schema.text)}${flagsSections} `; } -/** - * `bin.ts`'s `--help` fast path calls this instead of composing - * `buildCommandUsageText(normalizeCliCommandAlias(...))` itself, so an alias like `tap` or - * `launch` cannot silently miss alias resolution by calling `buildCommandUsageText` raw. A - * command name the alias registry does not recognize (including `rotate`, which is retired - * rather than aliased) passes through unchanged and falls back to `null` exactly as - * `buildCommandUsageText` would. - */ +/** `--help` text for a command name or one of its aliases; `null` when neither has any. */ export function resolveHelpTargetUsageText(helpTarget: string): string | null { return buildCommandUsageText(normalizeCliCommandAlias(helpTarget)); } diff --git a/src/cli/entry.test.ts b/src/cli/entry.test.ts new file mode 100644 index 0000000000..eba5688fc1 --- /dev/null +++ b/src/cli/entry.test.ts @@ -0,0 +1,111 @@ +import assert from 'node:assert/strict'; +import { test } from 'vitest'; +import * as cliHelp from '../cli-schema/cli-help.ts'; +import { listCliCommandNames } from '../command-catalog.ts'; +import { cliAliasesForCommand } from '../commands/cli-command-aliases.ts'; +import { runEntry, type EntryModules } from './entry.ts'; + +const ALIASES = listCliCommandNames().flatMap((command) => + cliAliasesForCommand(command).map((entry) => [entry.alias, command] as const), +); + +function harness(options: { bundledVersion?: string; cli?: EntryModules['cli'] } = {}) { + const loaded: string[] = []; + const stdout: string[] = []; + const stderr: string[] = []; + const cliArgv: string[][] = []; + const exits: number[] = []; + const modules: EntryModules = { + help: async () => { + loaded.push('help'); + return cliHelp; + }, + cli: + options.cli ?? + (async () => { + loaded.push('cli'); + return { + runCliProcess: async (argv) => { + cliArgv.push(argv); + }, + }; + }), + mcp: async () => { + loaded.push('mcp'); + return { runAgentDeviceMcpServer: async () => {} }; + }, + version: async () => { + loaded.push('version'); + return { readVersion: () => '9.9.9-source' }; + }, + processExit: async () => ({ + exitAfterFlush: async (code) => { + exits.push(code); + }, + }), + }; + const run = (argv: string[]) => + runEntry(argv, modules, { + bundledVersion: options.bundledVersion, + stdout: (text) => stdout.push(text), + stderr: (text) => stderr.push(text), + }); + return { run, loaded, stdout, stderr, cliArgv, exits }; +} + +test('every registered alias prints its canonical help without loading the CLI', async () => { + assert.ok(ALIASES.length > 0); + for (const [alias, canonical] of ALIASES) { + const entry = harness(); + await entry.run([alias, '--help']); + assert.equal(entry.stdout.join(''), cliHelp.buildCommandUsageText(canonical), alias); + assert.deepEqual(entry.loaded, ['help'], alias); + } +}); + +test('an unknown help topic falls through to the full CLI', async () => { + const entry = harness(); + await entry.run(['rotate', '--help']); + assert.deepEqual(entry.stdout, []); + assert.deepEqual(entry.loaded, ['help', 'cli']); + assert.deepEqual(entry.cliArgv, [['rotate', '--help']]); +}); + +test('--version prints the bundled version, or reads it when running from source', async () => { + const bundled = harness({ bundledVersion: '1.2.3' }); + await bundled.run(['--version']); + assert.deepEqual([bundled.stdout, bundled.loaded], [['1.2.3\n'], []]); + + const source = harness(); + await source.run(['-V']); + assert.deepEqual([source.stdout, source.loaded], [['9.9.9-source\n'], ['version']]); +}); + +test('no command prints usage and exits 1 after flushing', async () => { + const entry = harness(); + await entry.run([]); + assert.equal(entry.stdout.join(''), `${cliHelp.buildUsageText()}\n`); + assert.deepEqual(entry.exits, [1]); +}); + +test('mcp starts the server unless help is requested', async () => { + const server = harness(); + await server.run(['mcp']); + assert.deepEqual(server.loaded, ['mcp']); + + const help = harness(); + await help.run(['mcp', '--help']); + assert.deepEqual(help.loaded, ['help']); + assert.equal(help.stdout.join(''), cliHelp.buildCommandUsageText('mcp')); +}); + +test('a startup failure is reported on stderr and exits 1', async () => { + const entry = harness({ + cli: async () => { + throw new Error('boom'); + }, + }); + await entry.run(['press', 'Sign in']); + assert.deepEqual(entry.stderr, ['boom\n']); + assert.deepEqual(entry.exits, [1]); +}); diff --git a/src/cli/entry.ts b/src/cli/entry.ts new file mode 100644 index 0000000000..71f00154e0 --- /dev/null +++ b/src/cli/entry.ts @@ -0,0 +1,86 @@ +/** The lazily loaded halves of the process; `bin.ts` supplies the real `import()` calls. */ +export type EntryModules = Readonly<{ + help: () => Promise<{ + buildUsageText(): string; + resolveHelpTargetUsageText(target: string): string | null; + }>; + cli: () => Promise<{ runCliProcess(argv: string[]): Promise }>; + mcp: () => Promise<{ runAgentDeviceMcpServer(): Promise }>; + version: () => Promise<{ readVersion(): string }>; + processExit: () => Promise<{ exitAfterFlush(code: number): Promise }>; +}>; + +export type EntryIo = Readonly<{ + /** The version the bundler baked in; absent when running from source. */ + bundledVersion: string | undefined; + stdout: (text: string) => void; + stderr: (text: string) => void; +}>; + +/** Process entry: the three help/version fast paths, else the MCP server or the full CLI. */ +export async function runEntry(argv: string[], modules: EntryModules, io: EntryIo): Promise { + try { + await dispatch(argv, modules, io); + } catch (error) { + io.stderr(`${error instanceof Error ? error.message : String(error)}\n`); + // #1596: flush before exiting so a piped caller sees the message. + await (await modules.processExit()).exitAfterFlush(1); + } +} + +async function dispatch(argv: string[], modules: EntryModules, io: EntryIo): Promise { + if (argv.length === 0) return await printUsageAndExit(modules, io); + if (isVersionRequest(argv)) return await printVersion(modules, io); + const helpTarget = simpleHelpTarget(argv); + if (helpTarget !== undefined) return await printHelp(helpTarget, argv, modules, io); + if (isMcpServerRequest(argv)) return await (await modules.mcp()).runAgentDeviceMcpServer(); + await runCli(argv, modules); +} + +async function printUsageAndExit(modules: EntryModules, io: EntryIo): Promise { + io.stdout(`${(await modules.help()).buildUsageText()}\n`); + await (await modules.processExit()).exitAfterFlush(1); +} + +async function printVersion(modules: EntryModules, io: EntryIo): Promise { + io.stdout(`${io.bundledVersion ?? (await modules.version()).readVersion()}\n`); +} + +/** An unknown help topic is left to the full CLI, which owns the error path. */ +async function printHelp( + target: string | null, + argv: string[], + modules: EntryModules, + io: EntryIo, +): Promise { + const help = await modules.help(); + const text = + target === null ? `${help.buildUsageText()}\n` : help.resolveHelpTargetUsageText(target); + if (text === null) return await runCli(argv, modules); + io.stdout(text); +} + +async function runCli(argv: string[], modules: EntryModules): Promise { + await (await modules.cli()).runCliProcess(argv); +} + +function isVersionRequest(argv: string[]): boolean { + return argv.length === 1 && (argv[0] === '--version' || argv[0] === '-V'); +} + +function isMcpServerRequest(argv: string[]): boolean { + return argv[0] === 'mcp' && !argv.some(isHelpFlag); +} + +/** `null` = general usage, a string = one command's help, `undefined` = not a help request. */ +function simpleHelpTarget(argv: string[]): string | null | undefined { + const [first, second] = argv; + if (argv.length === 1) return first === 'help' || isHelpFlag(first) ? null : undefined; + if (argv.length !== 2) return undefined; + if (first === 'help') return second; + return isHelpFlag(second) ? first : undefined; +} + +function isHelpFlag(arg: string | undefined): boolean { + return arg === '--help' || arg === '-h'; +} diff --git a/test/integration/smoke-cli.test.ts b/test/integration/smoke-cli.test.ts index 70f7d90e52..7d4710cb05 100644 --- a/test/integration/smoke-cli.test.ts +++ b/test/integration/smoke-cli.test.ts @@ -1,19 +1,9 @@ import test from 'node:test'; import assert from 'node:assert/strict'; -import { mkdtempSync, readFileSync, readdirSync, rmSync } from 'node:fs'; -import { tmpdir } from 'node:os'; -import { join } from 'node:path'; import { runCmdSync } from '@agent-device/host-kit/command'; import { cliAliasesForCommand } from '../../src/commands/cli-command-aliases.ts'; import { listCliCommandNames } from '../../src/command-catalog.ts'; -// Derived from the alias registry itself (not hard-coded) so a future alias is -// exercised automatically, and so a partial hand-written mapping that covers only -// today's aliases cannot silently regain a fast-path gap for an alias added later. -const KNOWN_ALIASES: ReadonlyArray = listCliCommandNames().flatMap( - (command) => cliAliasesForCommand(command).map((entry) => [entry.alias, command] as const), -); - function runCli(args: string[]): { status: number; stdout: string; stderr: string } { const result = runCmdSync( process.execPath, @@ -23,45 +13,6 @@ function runCli(args: string[]): { status: number; stdout: string; stderr: strin return { status: result.exitCode, stdout: result.stdout ?? '', stderr: result.stderr ?? '' }; } -// `runCli` (src/bin.ts) reaches the full CLI bootstrap only through one dynamic import: -// `runCli(argv)` -> `import('./cli/process-entry.ts')`. The --help fast path never reaches -// it. That makes this file's own coverage report (collected via NODE_V8_COVERAGE on the -// subprocess) an independent, byte-output-blind signal for which path actually ran: a -// reintroduced hand-written alias table in bin.ts, or a `resolveHelpTargetUsageText` that -// returns null for a real alias, falls through to `runCli` and flips this from false to -// true even though stdout would stay byte-identical to the canonical command's --help. -const PROCESS_ENTRY_MARKER = 'src/cli/process-entry.ts'; - -function runCliTrackingCoverage(args: string[]): { - status: number; - stdout: string; - stderr: string; - bootstrappedFullCli: boolean; -} { - const coverageDir = mkdtempSync(join(tmpdir(), 'agent-device-cli-coverage-')); - try { - const result = runCmdSync( - process.execPath, - ['--experimental-strip-types', 'src/bin.ts', ...args], - { allowFailure: true, env: { ...process.env, NODE_V8_COVERAGE: coverageDir } }, - ); - const bootstrappedFullCli = readdirSync(coverageDir).some((file) => { - const report = JSON.parse(readFileSync(join(coverageDir, file), 'utf8')) as { - result: Array<{ url: string }>; - }; - return report.result.some((entry) => entry.url.includes(PROCESS_ENTRY_MARKER)); - }); - return { - status: result.exitCode, - stdout: result.stdout ?? '', - stderr: result.stderr ?? '', - bootstrappedFullCli, - }; - } finally { - rmSync(coverageDir, { recursive: true, force: true }); - } -} - test('cli --help returns usage', () => { const result = runCli(['--help']); assert.equal(result.status, 0, result.stderr); @@ -91,48 +42,14 @@ test('cli without command prints usage and exits 1', () => { assert.match(result.stdout, /agent-device /i); }); -test('known alias registry is non-empty and includes every known alias', () => { - // Belt-and-braces pin: if the registry were ever emptied by mistake, the two - // tests below would vacuously pass (an empty `for` loop asserts nothing). Pin - // both the non-empty condition and the concrete alias set so that failure is - // loud instead of silent. - assert.ok(KNOWN_ALIASES.length > 0, 'expected at least one alias to exercise this test'); - const aliasNames = KNOWN_ALIASES.map(([alias]) => alias).sort(); - assert.deepEqual(aliasNames, ['launch', 'long-press', 'relaunch', 'tap'].sort()); -}); - -test('alias --help fast path is byte-identical to its canonical command', () => { - // The canonical side is always a direct invocation of the canonical command name - // (never routed through the alias resolver), so this stays an independent oracle: - // a degenerate resolver that maps every alias to one fixed command would still - // fail here for the aliases whose canonical command differs. - for (const [alias, canonical] of KNOWN_ALIASES) { - const aliasResult = runCli([alias, '--help']); - const canonicalResult = runCli([canonical, '--help']); - assert.equal(aliasResult.status, 0, `${alias} --help: ${aliasResult.stderr}`); - assert.equal(canonicalResult.status, 0, `${canonical} --help: ${canonicalResult.stderr}`); - assert.equal( - aliasResult.stdout, - canonicalResult.stdout, - `expected "${alias} --help" to be byte-identical to "${canonical} --help"`, - ); - } -}); - -test('alias --help fast path bypasses the full CLI bootstrap', () => { - for (const [alias] of KNOWN_ALIASES) { - const result = runCliTrackingCoverage([alias, '--help']); - assert.equal(result.status, 0, `${alias} --help: ${result.stderr}`); - assert.equal( - result.bootstrappedFullCli, - false, - `expected "${alias} --help" to bypass ${PROCESS_ENTRY_MARKER}`, - ); +test('alias --help matches the canonical command help', () => { + const aliases = listCliCommandNames().flatMap((command) => + cliAliasesForCommand(command).map((entry) => [entry.alias, command] as const), + ); + assert.ok(aliases.length > 0); + for (const [alias, canonical] of aliases) { + const result = runCli([alias, '--help']); + assert.equal(result.status, 0, result.stderr); + assert.equal(result.stdout, runCli([canonical, '--help']).stdout, alias); } - - // Control: a retired/unmapped command must still fall through to the full CLI - // bootstrap, proving the coverage signal actually distinguishes the two paths - // instead of reading false unconditionally. - const rotate = runCliTrackingCoverage(['rotate', '--help']); - assert.equal(rotate.bootstrappedFullCli, true); });