fix: alias launch/relaunch to open and suggest canonical commands for unknown names#1166
Conversation
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
thymikee
left a comment
There was a problem hiding this comment.
Adversarial review — fix/unknown-command-suggestions
Verified against a scratch worktree of origin/fix/unknown-command-suggestions (pnpm install --frozen-lockfile, tsc, oxlint, scripts/layering/check.ts, vitest run --project unit-core) plus gh pr checks / gh run view --log.
Blocking
1. Fallow Code Quality CI check fails — real dead export, not a flake.
getNearestCommandNames (src/cli/parser/command-suggestions.ts:49) is exported but has zero external consumers. It's only called internally by suggestCommandFor (line 99), and the new test file never imports it (only listCommandAliasSuggestionEntries and suggestCommandFor are imported). Confirmed with a repo-wide grep — the only two hits are the definition and the one internal call site. The CI job (Fallow Code Quality, run 29009894774/job 86090589214) fails on exactly this:
● Unused exports (1)
src/cli/parser/command-suggestions.ts
:49 getNearestCommandNames
Fix: drop the export keyword (make it module-private) unless there's a reason to expose it for direct testing.
Worth addressing (non-blocking)
2. Case-insensitivity gap — curated map and fallback both miss exact case-variant matches.
suggestCommandFor does a case-sensitive lookup into COMMAND_ALIAS_SUGGESTIONS, and the Levenshtein fallback's threshold is tight enough that a single case difference on a short token blows the budget. Verified directly:
"RELAUNCH" -> undefined
"Relaunch" -> undefined
"TAP" -> undefined
"Touch" -> undefined
Notably, the PR's own suggestCommandFor never throws for arbitrary input test (lines 149–154) already includes 'RELAUNCH' and 'Relaunch' in its input list — but only asserts doesNotThrow, not that a suggestion comes back. Given the target audience (LLM agents / users who plausibly type RELAUNCH, Touch, etc.), this silently drops exactly the cases the curated map exists to catch. Cheap fix: lower-case both sides of the curated lookup.
3. Levenshtein fallback can bundle an unrelated match in with a good one.
No gap/confidence cutoff separates a strong top match from a weak tied/near-tied one when nearest.length > 1. Reproduced:
suggestCommandFor('clos') -> 'one of: close, logs'
close (distance 1, clearly the intended target) and logs (distance 2, coincidentally within the length-4 threshold of 2 but semantically unrelated) both clear the same static threshold, so the message becomes "Did you mean one of: close, logs?" — confusing when one candidate is clearly better. Similarly suggestCommandFor('ls') -> 'is' (distance 1 within the length<=3 threshold of 1) — a plausible false positive since users typing the common ls almost certainly don't mean is. Not dangerous (suggestion-only, no auto-exec) but a real precision gap; consider only keeping ties within 0–1 of the best distance, or dropping non-prefix matches at distance ≥ len-1 for very short inputs.
4. Minor DRY/drift risk: the unknown-flag hint's example string is hand-duplicated, not registry-checked.
formatUnknownFlagMessage (line 350) hardcodes "open <app> --relaunch" independently of COMMAND_ALIAS_SUGGESTIONS.relaunch.example (line 247) etc. The "alias suggestion target resolves to a registered command" test (command-suggestions.test.ts:49-61) does not cover this literal, so if open's --relaunch flag is ever renamed, this string can drift silently while the curated-map guard still passes. Low priority since it's the same string today, but worth a comment or a shared constant given the PR explicitly sells registry-drift protection as a feature.
5. Test-strength nit: "registry validation" only checks the top-level command token, not the full multi-word shape.
listCommandAliasSuggestionEntries test only asserts isKnownCliCommandName(suggestion.command) — e.g. that keyboard and open exist — not that dismiss is a legal keyboard action or --relaunch a legal open flag. I independently verified both are correct today (src/commands/system/index.ts KEYBOARD_METADATA_ACTION_VALUES includes 'dismiss'; src/cli/parser/cli-flags.ts:1006-1009 registers --relaunch for open), so there's no live bug — but the guarantee is weaker than "validated against the live registry" implies. A future typo in an example field's trailing subcommand/flag (e.g. keyboard dismis) would not be caught by this test.
Verified sound
tapremoval (the stated reason for this PR):normalizeCommandAlias('tap')→'press'runs at every pointparseRawArgsassignscommand(src/cli/parser/args.ts, both branches at lines 59/70), sotapnever reaches theisKnownCliCommandNamecheck infinalizeParsedArgs.usageForCommand(used by thehelp <target>path incli.ts) also normalizes internally before building help text, soagent-device help tapresolves to press's help rather than falling intoformatUnknownHelpTargetMessage. The MCP path (src/mcp/router.ts→commandToolExecutor.execute) is driven by a fixed, pre-enumerated tool list unrelated to CLI alias parsing —tapisn't a concept there at all. Existing regression tests (cli-help.test.ts: "tap dispatches as press...", "tap with a bare ref gets the @ref hint, not an unknown-command error") pass.- No internal/hidden command leakage:
listCliCommandNames()returns onlyPUBLIC_COMMANDS+ local-cli commands, explicitly excludingINTERNAL_COMMANDS— consistent withisKnownCliCommandNameand the exposure-list pattern from #1137 (which is wherelistCliCommandNamesitself was introduced). - Unknown-flag hint is narrowly scoped: grepped the full test suite for
"Unknown flag"— only the new test file references it, so no existing assertion on the old bare message breaks. None of the curatedPOSITIONAL_APP_FLAG_GUESSES(--bundle-id,--bundleid,--bundle,--package,--package-name,--packagename,--app-id,--appid,--pkg) is a real registered flag anywhere incli-flags.ts, so it can't misfire on a legitimately-supported flag (also covered by the "unrelated unknown flags are unaffected" test). - Error code / no auto-exec:
AppError('INVALID_ARGS', ...)preserved everywhere; every new code path only builds message text. - Message-style convention: embedding "Did you mean X?" directly in the error message (rather than the separate
details.hintfield) matches the pre-existing convention insrc/core/interaction-positionals.ts(Did you mean "@${ref}"? Snapshot refs need the @ prefix.). - Layering:
node --experimental-strip-types scripts/layering/check.ts→ "OK — 730 source files satisfy the import-direction DAG"; the new file's only cross-module import islistCliCommandNamesfromcommand-catalog.ts, same dependencyargs.tsalready had. - Local verification: typecheck, lint, layering guard,
command-suggestions.test.ts(14/14), and the fullsrc/cli/parser/__tests__/*+src/__tests__/cli-help.test.tssuite (178 tests total) all pass in a scratch worktree. Fullunit-coreproject run shows only pre-existing/unrelated failures: the knownrunner-xctestrun.test.tspackageVersion0.19.0vs0.19.1stale-fixture mismatch (reproduces onmainatff7fc5b, and is the same failure behind CI'sCoveragejob) plus a handful of apple-runner-client test timeouts under local CPU contention. Nothing incli/parser/command-suggestionsfailed.
CI status at review time
- Failing:
Coverage(pre-existing/unrelated, see above),Fallow Code Quality(real, PR-caused — finding #1). - Passing: Typecheck, Lint & Format, Layering Guard, Integration Tests, Bundle Size, Packaged CLI Node 22.12, No test-only DI seams, Swift Runner Unit Compile, iOS Runner Swift Compatibility, Web Platform Smoke, CodeQL, Analyze(actions/java-kotlin/js-ts/python), and one of four Smoke Tests jobs (others still pending at review time).
Verdict
Needs changes. The Fallow dead-export failure is a one-line fix and should block merge as-is since it's a hard CI gate. Findings #2–#5 are real but non-blocking quality gaps in an advisory-only feature — worth a follow-up if not folded into this PR.
|
Addressed the review in 07d0a0b: Blocking — Fallow dead export (#1): dropped the Case-insensitivity (#2): The former doesNotThrow-only cases are now positive assertions ("curated suggestions are case-insensitive", "known command names in the wrong case suggest their lowercase form"). Levenshtein tie-bundling (#3): three precision rules in Both covered by new tests. DRY example string (#4): the unknown-flag hint now references the shared Multi-word shape validation (#5): new "every curated alias suggestion example parses as a valid invocation" test tokenizes each example (placeholders substituted) and runs it through Evidence: new test file 20/20; all |
Agents commonly guess command names that don't exist, e.g. relaunch/launch instead of `open <app> --relaunch`, burning turns on Unknown command errors that only say "run --help". Add a curated alias-to-canonical-shape map for the most common guesses (launch/relaunch/start/restart, touch, input/ settext/entertext, screencap/capture, dismiss), backed by a nearest-name edit-distance fallback derived from the live command registry so suggestions can't drift. Also hint that `open` takes the app/bundle id as a positional when an unknown flag looks like a bundle-id guess (e.g. --bundle-id), and apply the same suggestion to `help <unknown>`. Suggestions are display-only; nothing auto-executes and the error code stays INVALID_ARGS.
…ter nearest-name matching - Drop the export on getNearestCommandNames (module-private; only suggestCommandFor uses it) to satisfy the Fallow unused-export gate. - Lowercase the input token before both the curated-map lookup and the nearest-name pass, so RELAUNCH/Relaunch/TAP/Touch get the same hint as their lowercase forms. Added a curated `tap` entry: lowercase `tap` is normalized to press before the unknown-command check, so the entry only catches case variants like TAP. - Tighten the nearest-name fallback: exact prefix matches win outright (`clos` now suggests only `close`, not "one of: close, logs"), otherwise only ties at the minimum edit distance are kept, and 1-2 character tokens never get a suggestion (`ls` no longer suggests `is`). - Share the "open <app> --relaunch" example string between the curated map and the unknown-flag hint, and extend the registry-drift tests to parse each curated example end-to-end (validates open --relaunch as a registered flag) plus assert keyboard dismiss is a real keyboard action.
07d0a0b to
96778f3
Compare
Follow the tap -> press precedent: `relaunch <app>` now runs `open <app>` with --relaunch injected, and `launch <app>` runs a plain `open <app>` (no forced restart — that would silently destroy app state). Both are normalized in normalizeCommandAlias before parsing, so command identity stays `open` for daemon requests and telemetry, all other args/flags pass through to open's normal validation (URL targets still get the daemon's existing --relaunch guidance), and an explicit --relaunch stays idempotent. Alias matching is now case-insensitive (TAP, RELAUNCH, Launch), so the curated tap suggestion entry is dead and removed along with launch/relaunch; start/restart and the rest of the map stay suggestion-only since start is genuinely ambiguous.
|
Design change per maintainer feedback, in 0d0b823:
PR body updated to reflect the alias + suggestion split. Evidence: parser/help/suggestion suites green (46+23+19), full |
thymikee
left a comment
There was a problem hiding this comment.
Re-review addendum — commit 0d0b823 (launch/relaunch as true aliases)
Scoped to the new commit only. Verified in a scratch worktree at 0d0b823a4 (pnpm frozen-lockfile install, tsc, oxlint, oxfmt --check, fallow audit vs merge-base 477c684cf, vitest on all touched suites) plus direct parser probes via node --experimental-strip-types.
Verdict: APPROVE-worthy
No blocking findings. I tried to break the flag-injection, case-folding, and alternate-dispatch-path surfaces; everything held. Details below.
Adversarial probes — all sound
1. applyAliasImpliedFlags cannot clobber or be clobbered.
- There is no negation path to lose: boolean flags reject inline values (
parseFlagValuethrowsFlag --relaunch does not take a value.for--relaunch=false, verified empirically) and no--no-relaunchflag exists. Injection is set-true-only, so it is idempotent with explicit--relaunchin any position (relaunch app --relaunch→ singlerelaunch: true, verified). - Injection cannot mask flag validation: it writes
flags.relaunchdirectly without touchingprovidedFlags, and both unsupported-flag checks infinalizeParsedArgsiterateprovidedFlags(user-typed tokens only). Verified:relaunch app --not-a-flagstill throwsUnknown flag: --not-a-flag;relaunch app --count 3still throwsFlag --count is not supported for command open. - The injected flag survives defaults:
mergeDefinedFlags(defaults→flags, then parsed.flags)infinalizeParsedArgslets parsed flags win — verified withdefaultFlags: { relaunch: false }, injectedtruesurvives. - Minor observation, no action needed: the unsupported-flag error for an aliased invocation says
for command open(notrelaunch) — consistent with the tap precedent and arguably more honest.
2. Case-insensitive normalizeCommandAlias is collision-free and casing-preserving.
- Every registered command name (
listCliCommandNames()+ internal + special) is lowercase, and none oflaunch/relaunch/tap/metrics/long-pressis a registered command, so case-folded alias matching cannot shadow a real command. - Non-alias tokens keep original casing in errors, as claimed:
OPEN app→Unknown command: OPEN. Did you mean open?(verified;normalizeCommandAliasreturns the original string on no-match). help RELAUNCH→ open's usage text (usageForCommandnormalizes; verified, output startsagent-device open [appOrUrl] ...). Same forhelp launch/help relaunch.- Note the lowering also silently extends case-insensitivity to the pre-existing
TAP/METRICS/LONG-PRESSaliases — a benign behavior expansion, covered forTAPby the new test atargs-parse-session.test.ts("command aliases normalize case-insensitively").
3. Ordering and alternate dispatch paths.
- Both
parseRawArgscommand-assignment sites (post---branch and the normal-token branch,args.ts:59-71/:79-84) now capturerawCommandand normalize in lockstep;applyAliasImpliedFlagsruns once at the end of the loop, beforefinalizeParsedArgs's unknown-command check, sorelaunchnever reaches the suggestion path. Flag-before-command ordering works (--json relaunch appverified). The third assignment site insideshouldTreatUnknownDashTokenAsPositionalskips normalization, but only fires for dash-leading tokens — not alias-relevant, pre-existing. parseRawArgs's only production consumers aresrc/cli.tsandsrc/utils/cli-options.ts(grepped); MCP dispatch (src/mcp/router.ts→ fixed tool list) has no command-name aliasing surface at all.- Two paths dispatch command names WITHOUT alias normalization, both pre-existing and shared with the
tapalias, so not regressions from this commit (informational): (a) replay scripts —parseReplayScriptLine(src/replay/script.ts:188) matches command tokens verbatim, so a hand-writtenrelaunch <app>line would not normalize (the recorder only ever writes canonicalopen); (b) batch steps —readStructuredBatchCommandName(src/batch-policy.ts:57) validates against descriptor-derived names and rejectsrelaunch(andtap) with a clearnot available through command batchINVALID_ARGS. If true aliases keep accumulating, a shared alias-normalization helper for these surfaces might be worth a follow-up, but nothing is wrong today.
4. URL target case (polish, non-blocking). relaunch rne://url parses to open + relaunch: true (verified) and the daemon guidance exists on both validation paths (validateResolvedOpenRequest and validatePreResolvedOpenRequest, src/daemon/handlers/session-open-prepare.ts:70/93): INVALID_ARGS: open --relaunch does not support URL targets. The message references --relaunch, which an alias user never typed — comprehensible given the alias semantics, but a small nod ("relaunch is an alias for open --relaunch; open URLs with plain open") would read better for agents. Same applies to bare relaunch with no app arg (open --relaunch requires an app argument, session-open.ts). Polish only.
5. Docs nit (non-blocking). The agent-facing help (src/cli/parser/cli-help.ts:201) documents "tap is an alias for press" but nothing documents the new launch/relaunch aliases — agents are still only taught open <app> --relaunch. Behaviorally fine (both spellings now work), but the alias is undiscoverable from help; a one-line mention would complete the feature.
Local verification
- Touched suites + full parser suite:
cli-help.test.ts(23),args-parse-session.test.ts(46),command-suggestions.test.ts(19), plusargs-validation/args-parse-interaction/cli-help-command-usage/cli-help-topics— 190/190 pass. fallow audit --base 477c684cf(merge-base): No issues in 6 changed files — the previous dead-export blocker stays fixed.format:check,typecheck,lint: all clean.
CI (fresh push, run 29034329622)
All green including the two previously-failing gates: Coverage pass (the 0.19.0/0.19.1 fixture fix #1170 is in the rebase), Fallow Code Quality pass. Typecheck, Lint & Format, Layering Guard, Integration Tests, Bundle Size, Packaged CLI, No test-only DI seams, Swift Runner Unit Compile, iOS Runner Swift Compatibility, Web Platform Smoke, CodeQL, all Analyze jobs, 3/4 Smoke Tests pass; 1 Smoke Tests job still pending at review time.
|
Three exported-and-unit-tested-but-unreferenced-in-production incidents this week (#1166 getNearestCommandNames, #1167 buildSettleTail, #1199 clearMetroSessionHints) — the first two were caught by fallow's dead-code check because they had zero importers anywhere; #1199 was missed because a test file imports the export, and fallow's default reachability graph counts a test import as "used". Adds a second, stricter pass reusing fallow's own --production mode (entry.exclude test/story/dev files) via scripts/test-only-exports/check.ts: an export alive in fallow's default graph but dead in its production graph, with no other reference anywhere in its own file, has no production call site — exactly the #1199 shape. Ratchets against a checked-in baseline (scripts/test-only-exports-baseline.json, 77 entries); new findings fail `pnpm check:test-only-exports` (wired into CI's Fallow job and check:tooling). A `// test-seam: <reason>` comment above an export is the escape hatch for intentional test seams. Also extends .fallowrc.json's ignoreExports for seven daemon route handlers (src/daemon/handlers/*.ts) that are genuinely production-reachable through request-handler-chain.ts's `typeof import()` lazy-load pattern, which fallow's static import graph can't trace as a named-export consumer — without this they were false positives in the production-mode pass.
* ci: ratchet against test-only exports Three exported-and-unit-tested-but-unreferenced-in-production incidents this week (#1166 getNearestCommandNames, #1167 buildSettleTail, #1199 clearMetroSessionHints) — the first two were caught by fallow's dead-code check because they had zero importers anywhere; #1199 was missed because a test file imports the export, and fallow's default reachability graph counts a test import as "used". Adds a second, stricter pass reusing fallow's own --production mode (entry.exclude test/story/dev files) via scripts/test-only-exports/check.ts: an export alive in fallow's default graph but dead in its production graph, with no other reference anywhere in its own file, has no production call site — exactly the #1199 shape. Ratchets against a checked-in baseline (scripts/test-only-exports-baseline.json, 77 entries); new findings fail `pnpm check:test-only-exports` (wired into CI's Fallow job and check:tooling). A `// test-seam: <reason>` comment above an export is the escape hatch for intentional test seams. Also extends .fallowrc.json's ignoreExports for seven daemon route handlers (src/daemon/handlers/*.ts) that are genuinely production-reachable through request-handler-chain.ts's `typeof import()` lazy-load pattern, which fallow's static import graph can't trace as a named-export consumer — without this they were false positives in the production-mode pass. * fix: harden test-only-exports ratchet per review Addresses the two should-fixes and all five minors from the independent review of #1202: - Replace the regex own-file occurrence count with an oxc-parser AST walk (typescript@7 ships no JS scanner API, so the review's fallback tool suggestion is the primary): identifiers are counted as AST nodes deduped by source span, so mentions in JSDoc/block comments, strings, and template-literal text no longer masquerade as call sites (review finding 1, both constructed cases re-verified fixed), and a `//` inside a string no longer hides real usages (finding 6). Span dedupe keeps barrel re-exports (`export { x } from`) counting once. The sharper count surfaced one organic false negative on main: `selector` in src/commands/index.ts was previously exempted because the regex matched "selector" inside the './...selector-read.ts' import path string; it is now baselined alongside its sibling `ref` (same re-export line). - Make the baseline shrink-only (finding 2): --update-baseline refuses new findings with the same wire/delete/annotate message, so the `// test-seam:` annotation in the reviewed source diff is the only acceptance path; CONTRIBUTING no longer documents baseline regeneration as an acceptance option and now describes baseline growth as a deliberate manual edit. - Stale baseline entries now emit a `::warning` CI annotation (finding 3). - Commit a re-runnable fixture test (finding 4): check.test.ts mirrors scripts/layering/model.test.ts, builds a synthetic package with a clearMetroSessionHints-shaped export (JSDoc self-mention included), asserts it is flagged, and asserts the annotated twin passes; wired before the check in pnpm check:test-only-exports. - Mark the unreadable/unparseable-file fallbacks CONSERVATIVE: per CONTRIBUTING's convention (finding 5). - Document the dynamic property access (obj[name]) blind spot in the script header and CONTRIBUTING (finding 7). * fix: harden test-only export ratchet * refactor: use native Fallow export gate * chore: refresh production export baseline
Problem
In a July 2026 benchmark, claude-haiku burned 5 turns across runs trying to
relaunch an installed app:
launch --bundle-id <app>— the QA task said "Relaunch the installed app"relaunch <app>help relaunchThe correct shape is
open <bundleId> --relaunch, but the CLI's errors gaveno hint toward it:
Approach
Two layers: true aliases for the two crisp names, and suggestions as
fallback for the fuzzy rest.
True aliases:
launch/relaunch→openFollowing the existing
tap→pressprecedent (normalized innormalizeCommandAliasbefore parsing, hidden — not added to help or thecommand catalog):
relaunch <app>→open <app>with--relaunchinjected. Injection isidempotent with an explicit
--relaunch, and only the command token isnormalized — everything else passes through to open's normal validation
(e.g.
relaunch rne://urlsurfaces the daemon's existing"open --relaunch does not support URL targets" guidance).
launch <app>→ plainopen <app>(no--relaunch— aliasing launchto a force-restart would silently destroy app state).
TAP,RELAUNCH,Launchallnormalize), so agent-typed case variants work too.
openin daemonrequests and telemetry — asserted at the daemon boundary by dispatch-level
tests (
calls[0].command === 'open',flags.relaunch === true).help relaunch/help launchnow show open's usage instead of erroring.Suggestions for the rest (
src/cli/parser/command-suggestions.ts)Wired into the "Unknown command" check in
src/cli/parser/args.tsand thehelp <unknown>path insrc/cli.ts:start/restart→open <app> --relaunch(startisgenuinely ambiguous, so a hint beats silently guessing — kept
suggestion-only by design),
touch→press,input/settext/entertext→fill,screencap/capture→screenshot,dismiss→keyboard dismiss. Registry-drift tests assert every target resolves to aregistered command and every example parses as a valid invocation
(placeholders substituted), so a renamed
--relaunchflag or droppedkeyboard action fails the suite. A companion test asserts true aliases
(
launch/relaunch/tap) are not in the map — a stale entry therewould be dead code masking the alias.
listCliCommandNames()(derived from the command descriptor registry,never hardcoded). Precision rules: exact prefix matches win outright
(
clos→ onlyclose), otherwise only ties at the minimum distance arekept, and 1-2 character tokens never get a suggestion (
lsdoesn'tsuggest
is). Case-insensitive; falls back to the plain error whennothing is close — never guesses wildly, never crashes.
guesses (
--bundle-id,--package,--app-id, ...) gets a hint that theapp/bundle id is a positional on
open.Suggestions are strictly display text on the existing
INVALID_ARGSAppError— nothing auto-executes; aliases only normalize to a command theuser unambiguously meant.
Before / after
All three of the benchmark's wasted-turn shapes now resolve on the first try.
Gaps / deferred
Unknown flagfix is intentionally narrow (a curated flag-name set),not a generic fuzzy matcher — flag validation is sequenced ahead of
command validation, so making it command-aware felt riskier than the
payoff warranted.
tap): batch steps and MCP toolnames are separate surfaces and unchanged.
Test plan
src/cli/parser/__tests__/args-parse-session.test.ts:relaunch→ openrelaunch: true;launch→ open without it; explicit--relaunchidempotent;
RELAUNCH/Launch/TAPcase variants; URL pass-through.src/__tests__/cli-help.test.ts: dispatch-level daemon-call identity forboth aliases (command records
open, neverrelaunch/launch).src/cli/parser/__tests__/command-suggestions.test.ts(19 tests):registry parity + example-parses drift guards, aliases-not-in-map guard,
start/restart/touch/dismiss/fill/screenshot suggestions,case-insensitivity, prefix-beats-ties (
clos), short-token cutoff (ls),nonsense fallback,
--bundle-idhint, unrelated flags unaffected.pnpm typecheck,pnpm lint, layering guard,pnpm build,pnpm format:check,pnpm fallow(audit vs merge-base: no issues), andthe full
pnpm test:unitsuite — 379 files, 3447/3447 tests pass(the previously failing
runner-xctestruntest is fixed by fix: read runner cache package version from project root #1170, whichthis branch is rebased on).