fix(cli): an adapter arg named json must not crash every command (#441) - #442
Open
Agnik47 wants to merge 1 commit into
Open
fix(cli): an adapter arg named json must not crash every command (#441)#442Agnik47 wants to merge 1 commit into
json must not crash every command (#441)#442Agnik47 wants to merge 1 commit into
Conversation
…ntrhq#441) `configureCommandSurface` registers an adapter's own arguments first, then added the shared options unconditionally. Commander throws on a duplicate flag, and this runs while the CLI is being built, so a single adapter argument named `json` aborted startup for *every* command — `list`, `doctor`, and the `plugin uninstall` needed to remove the offending plugin, leaving no recovery path through the CLI. The official LinkedIn plugin ships such an adapter (`thread-snapshot`), so installing it bricked webcmd. Guard every shared option the adapter path registers — `--format`, `--json`, `--trace`, `-v/--verbose`, and the browser trio — the way `ensureOutputFormatOptions` in the same file already guarded its own, and reuse one helper for both so the two paths cannot drift again. An adapter that names a flag keeps it; webcmd drops its own rather than refusing to run. Format resolution has to agree about who owns a shadowed `--json`, or the flag would silently do two things: set the adapter's argument *and* switch the output format. Argv preprocessing already resolves this collision in the adapter's favour, so `outputFormatIsExplicit`/`requestedOutputFormat` now honour `--json` as the format alias only on commands where webcmd registered it. `-f json` is unaffected and remains the way to ask for JSON output there. Help follows the same rule: a shared option the adapter shadows is no longer listed under "Common options", in both the text and structured renderings, since advertising it would name a flag that is not registered and show the same flag twice with two different meanings.
Contributor
🟠 Maintainer review suggested — low confidenceThe automated review could not reach a fully supported conclusion. Limitations
This review is advisory and does not block merging. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #441.
The bug
configureCommandSurfaceregisters an adapter's own arguments first, thenadded the shared options unconditionally:
Commander throws on a duplicate flag, and this runs inside
createProgram()— while the CLI is still being built — so one adapter argument named
jsonaborted startup for every command. The official LinkedIn plugin ships such
an adapter (
plugins/linkedin/thread-snapshot.js:162), so installing itbricked webcmd. Verified on a clean
mainworktree at37036a6(v0.7.6):webcmd --versionwebcmd listwebcmd doctorwebcmd plugin listwebcmd plugin uninstall linkedinplugin uninstallcrashing is what made it a dead end: the only way out wasdeleting
~/.webcmd/plugins/linkedinby hand. The failure was also anunhandled exception — a raw Node stack trace instead of a structured webcmd
error, and the process still exited
0.The fix
Guard every shared option the adapter path registers, the way
ensureOutputFormatOptionsin the same file already guarded its own. Bothpaths now go through one helper, so they cannot drift apart again:
An adapter that names a flag keeps it; webcmd drops its own rather than
refusing to run. The guard covers
--format,--json,--trace,-v/--verbose, and for browser commands--window,--site-session,--keep-tab—thread-snapshotis the only adapter in the repo that collidestoday, but any of those names was equally fatal.
Who owns a shadowed
--jsonRegistering nothing is not sufficient on its own.
outputFormatIsExplicitandrequestedOutputFormatkeyed offgetOptionValueSource('json'), which doesnot care who registered the option — so
--jsononthread-snapshotwouldhave set the adapter's argument and switched the output format.
Argv preprocessing already resolves this collision in the adapter's favour —
knownCommandOptionsseeds the shared flags, then lets adapter args overwritethem — so format resolution now agrees:
--jsonis read as--format jsononly on commands where webcmd actually registered the alias.
-f jsonisuntouched and remains the way to ask for JSON output on such a command.
Help
Help follows the same rule, or it advertises a flag that is not registered and
lists the same flag twice with two different meanings. Before, on this branch,
thread-snapshot --helpstill printed the alias under Common options; now:A command that shadows nothing is unchanged and still lists
--json Alias of --format json. The same filtering is applied to thestructured (
--help -f yaml) rendering.Tests
16 new tests, all failing before this change and passing after (verified by
reverting only
src/command-surface.tsandsrc/command-presentation.tsandre-running):
src/commanderAdapter.test.ts(3) — registering an adapter with ajsonargument does not throw, the adapter keeps the flag, and a sibling command
that shadows nothing still gets the alias. This is the reported crash at the
level it actually occurred.
src/command-surface.test.ts(7) — registration succeeds for an argumentnamed
json,format,trace,verbose,window,site-session, orkeep-tab; every shared option is still registered when nothing collides;a shadowed
--jsonreaches the adapter and does not change the outputformat;
-f jsonstill does.src/command-presentation.test.ts(4) — shadowed flag listed once with theadapter's meaning in text and structured help; other shared options still
listed; a non-shadowing command unchanged.
vitest run --project unit --project plugin: 46 pre-existing failures onmain(WindowsEPERMonfs.symlinkSync, plus hosted/site-memory), 45 onthis branch with no failure that is not also on
main. One browser-runnertest appeared in a first run and not a second, and passes in isolation twice —
load-flaky, unrelated to this change.
tsc --noEmitclean.Left for a follow-up
Whether an adapter should be allowed to declare an argument that shadows a
reserved flag at all. This PR makes the collision survivable and gives the
adapter the flag; making
webcmd validatereject such names up front is aseparate call, and I did not want to break an adapter that already ships one
in the same change that stops it crashing the CLI.