Skip to content

fix(browser): accept --window after the leaf subcommand (#1850) - #1899

Open
Chen17-sq wants to merge 1 commit into
jackwener:mainfrom
Chen17-sq:fix/1850-browser-window-leaf-subcommand
Open

fix(browser): accept --window after the leaf subcommand (#1850)#1899
Chen17-sq wants to merge 1 commit into
jackwener:mainfrom
Chen17-sq:fix/1850-browser-window-leaf-subcommand

Conversation

@Chen17-sq

@Chen17-sq Chen17-sq commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

fix(browser): accept --window after page-interaction leaf subcommands (#1850)

Problem

opencli browser <session> open <url> --window background — the natural agent-facing form with the flag in the trailing position — failed with error: unknown option '--window', and browser open --help omitted --window, making it look unsupported on the leaf. The original fix made it work but went too far: it registered --window on EVERY browser descendant, including commands where the flag is inert and misleading in --help.

Root cause

  • src/cli.ts--window <mode> was declared only on the parent browser command. With enablePositionalOptions(), a parent option is only accepted before the leaf subcommand token, so the trailing-position form was rejected.
  • The first-pass fix (addBrowserWindowOptionRecursively) blindly added --window to every descendant: the session/scaffold commands bind, unbind, init, verify, close (none route through browserAction/getBrowserWindowMode, so the flag does nothing) and the group nodes get/tab/dialog (not real commands).

Fix

  • --window is a namespace option. It stays declared on the parent browser command (line ~878) so it parses in the pre-subcommand position and is surfaced once under namespace_options in structured help.
  • Registered on page leaves only. addBrowserWindowOptionRecursively(command, isBrowserRoot) now (a) skips the session set {bind,unbind,init,verify,close} but ONLY when isBrowserRoot === true — gating on isBrowserRoot so the unrelated leaf browser tab close (a real page leaf under the tab group, which DOES route through browserAction) is not wrongly skipped by the name collision; (b) recurses into group nodes (get/tab/dialog) with isBrowserRoot=false without adding --window to the group node itself; (c) adds --window only to true leaves (sub.commands.length === 0); (d) keeps the existing "already has --window" guard. Verified that eval/extract (and open/click/state/get url/tab close) route through browserActiongetBrowserWindowMode, so they correctly keep --window; init/verify use plain .action() and correctly lose it.
  • Deduped out of structured command_options. Because --window is now redeclared on each page leaf, src/help.ts compactCommanderCommand filters options whose long matches the namespace root's own options out of each leaf's command_options. This is correct and retained — without it --window would appear twice (once under namespace_options, once per leaf). The comment was clarified to state it removes namespace-inherited options from a leaf's own list.

Design summary

--window is a browser-namespace option. To accept it in the trailing position it is mirrored onto every page-interaction leaf (so commander parses it wherever it binds; getBrowserWindowMode/getCommandOption walk the parent chain to read the value). It is deduped out of structured command_options so it shows once at the namespace level. Non-page commands — init/verify/bind/unbind/close and the get/tab/dialog group nodes — are excluded so --help stays honest.

Tests

  • src/cli.test.ts: trailing-position parse asserts windowMode: 'background' reaches the connect call; the breadth test asserts --window IS on click/open/eval and nested get url, is NOT on init/verify, and is NOT on the get group node.
  • src/help.test.ts: a direct test builds the program, gets the browser eval leaf, calls commanderCommandHelpData(browser, leaf, {globalCommand}), and asserts command_options excludes window/session but includes the leaf's own frame, while namespace_options still includes window; a second test confirms the non-browser auth status leaf's command_options is unchanged (sanity that the dedup only removes inherited options).
  • Gates: npx tsc --noEmit clean; npx vitest run src/cli.test.ts src/help.test.ts → 175 passed; full suite npm test → 464 files / 5065 tests passed, 1 skipped, 0 failed.

…jackwener#1850)

`--window <mode>` was declared only on the parent `browser` command, but
enablePositionalOptions() means a parent option is only accepted *before* the
leaf subcommand token. The natural agent-facing form

    opencli browser <session> open <url> --window background

failed with `error: unknown option '--window'`, and `browser open --help`
omitted `--window` so it looked unsupported on the leaf.

Register `--window` on the browser's page-interaction leaves so it is also
accepted in the trailing position. getBrowserWindowMode()/getCommandOption()
already walk the parent chain, so the pre-subcommand form keeps working and the
flag resolves wherever it binds; `--window` now also shows in each page leaf's
`--help`.

Scope it precisely: only true leaves (sub.commands.length === 0) get the flag.
Group nodes (get/tab/dialog) are recursed into but never carry `--window`
themselves, and the session/scaffold commands {bind,unbind,init,verify,close}
are skipped at the browser root because they never route through
browserAction()/getBrowserWindowMode() — declaring it there would be inert and
misleading in `--help`. The skip is gated on isBrowserRoot so the unrelated
leaf `browser tab close` (a real page leaf under the `tab` group) still gets
`--window`.

In structured help, options inherited from the namespace root are de-duplicated
out of a command's own `command_options`, so `--window` continues to surface
once under `namespace_options` rather than being repeated on every leaf.
@jackwener

Copy link
Copy Markdown
Owner

Thanks for the broader analysis and tests. I compared this with #1926 because both PRs address #1850.

I opened a replacement PR here: #1963

The replacement keeps your important UX point: users and agents naturally put --window after the browser leaf command, and the accepted form should be documented/tested. I used the argv-preprocessor approach from #1926 as the lower-blast implementation, then added broader coverage for nested browser commands and a browser help example.

I did not use the leaf-option mirroring approach because it is easier to get subtly wrong as the browser command tree changes. One concrete example: this PR skips root browser close as a session/scaffold command, but browser close actually routes through browserAction, unlike init/verify.

You are credited in the replacement commit as a co-author:

Co-authored-by: Chen17-sq <109075336+Chen17-sq@users.noreply.github.com>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants