Skip to content

refactor(browser): replace --session flag with <sessionname> positional#1505

Merged
jackwener merged 4 commits into
mainfrom
refactor/browser-sessionname-positional
May 12, 2026
Merged

refactor(browser): replace --session flag with <sessionname> positional#1505
jackwener merged 4 commits into
mainfrom
refactor/browser-sessionname-positional

Conversation

@jackwener
Copy link
Copy Markdown
Owner

@jackwener jackwener commented May 12, 2026

Summary

--session <name> was required-but-flag, which is a UX anti-pattern. Required-flag = "this is required but I'm dressing it up as optional". Session is OpenCLI's operation-target identifier; the natural form is positional, like docker exec <container> <cmd> or git checkout <branch>.

New surface:

opencli browser <sessionname> open https://x.com
opencli browser <sessionname> click 12
opencli browser <sessionname> bind
opencli browser <sessionname> unbind

Implementation

Commander 14 cannot natively combine a parent positional with subcommand dispatch — the parent's positional is shadowed by the subcommand matcher. Workaround: src/main.ts pre-processes argv before program.parse(). When the token after browser is non-flag and not one of the 33 reserved subcommand names, it's treated as <sessionname> and rewritten into the internal --session <name> flag form. Help text on the browser command is overridden via .usage('<sessionname> <command> [options]') so users see the positional form.

Reserved subcommand list lives in src/cli-argv-preprocess.ts and is checked against actual cli.ts registrations via an invariant test.

Files

  • src/cli-argv-preprocess.ts — new module, the rewriter
  • src/cli-argv-preprocess.test.ts — 10 targeted tests
  • src/main.ts — call rewriter before runCli()
  • src/cli.ts — <sessionname> usage override, drop requiredOption UX-facing wording
  • tests/e2e/browser-tabs.test.ts — drop --session from browserArgs helper
  • README.md / README.zh-CN.md — examples
  • docs/guide/browser-bridge.md / docs/zh/guide/browser-bridge.md
  • skills/opencli-browser/SKILL.md — bind/unbind, examples, table
  • skills/opencli-usage/SKILL.md
  • CHANGELOG.md — Unreleased BREAKING entry

Test plan

  • npx tsc --noEmit pass
  • npx vitest run --project unit → 1073/1074 (1 unrelated skip)
  • npx vitest run --project extension → 61/61
  • npm run check:typed-error-lint baseline 189
  • npm run check:silent-column-drop baseline 103
  • grep --session zero in user-facing docs (README / skills / docs)
  • manual smoke: opencli browser foo state parses, opencli browser --help shows positional Usage line

Out of scope (follow-up tracked)

  • Deep adapter-author skill references (skills/opencli-adapter-author/references/*) have many opencli browser eval ... examples lacking session — separate sweep PR
  • Agent runtime adapters (consumers of OpenCLI from external agents) need to migrate; coordinate after merge

jackwener added 4 commits May 12, 2026 20:08
@jackwener
refactor(browser): replace --session flag with <sessionname> positional
f047ff8 
The `--session <name>` flag was semantically required but syntactically
optional, which is an anti-pattern. Required + flag is a contradiction:
flag form implies "optional", required is a runtime patch on top. Session
is OpenCLI's "operation target" identifier — the natural form for that is
a positional argument, like `docker exec <container> <cmd>` or
`git checkout <branch>`.

New surface:

  opencli browser <sessionname> open https://x.com
  opencli browser <sessionname> click 12
  opencli browser <sessionname> bind
  opencli browser <sessionname> unbind

Commander 14 cannot natively combine a parent positional with subcommand
dispatch — the parent's positional is shadowed by subcommand matching. To
bridge that, main.ts now pre-processes argv: when the token after `browser`
is non-flag and not a known subcommand name, it is treated as the
sessionname and rewritten to the internal `--session <name>` flag form
before commander parses it. Help text on the `browser` command is
overridden via `.usage('<sessionname> <command> [options]')` so users see
the positional form.

Reserved subcommand names (33) are listed in cli-argv-preprocess.ts and
tested for parity with cli.ts subcommand registrations. If a future
subcommand is added, the test fails loudly.

Synced surfaces:
  - README.md / README.zh-CN.md — all examples
  - docs/guide/browser-bridge.md (+ zh)
  - skills/opencli-browser/SKILL.md (bind/unbind, examples, table)
  - skills/opencli-usage/SKILL.md
  - tests/e2e/browser-tabs.test.ts
  - CHANGELOG.md (Unreleased BREAKING)

The internal `--session` flag and the unit tests calling
`program.parseAsync(['...', 'browser', '--session', 'foo', ...])` are
preserved as a stable internal API: tests bypass main.ts pre-processing
and exercise commander directly. The pre-processor has its own targeted
test file (cli-argv-preprocess.test.ts, 10 tests, all green).

Verification:
  - npx tsc --noEmit — pass
  - npx vitest run --project unit — 1073/1074 pass (1 unrelated skip)
  - npx vitest run --project extension — 61/61 pass
  - npm run check:typed-error-lint — baseline 189
  - npm run check:silent-column-drop — baseline 103
@jackwener
fix(cli-argv): only rewrite when browser is the root command
d43111e 
The preprocessor was looping through every argv slot and would mis-rewrite
occurrences of the literal word `browser` deeper in argv (e.g. `opencli
adapter init browser/x` or arg values containing `browser`).

Now the preprocessor walks past leading root flags + their values to
identify the root command token, and only acts when that token is
`browser`. The full set of root value-consuming flags
(`ROOT_VALUE_FLAGS`) is documented inline and kept in sync with the
`program.option()` calls in cli.ts.

Adds regression tests:
  - `opencli adapter init browser x` not rewritten
  - URL/path values containing `browser` not rewritten
  - `list browser state` (different root command) not rewritten
  - `--profile work browser foo state` correctly identifies `foo` as
    sessionname (not as --profile's value)
  - `--profile=work` long-form-with-equals consumes one slot only
  - boolean flags (`-v`) don't consume the next value

12/12 preprocessor tests pass.
@jackwener
fix(cli-argv): hide --session flag, fail-fast on retired form, rename…
b1e0bf1 
… to <session>

Three blockers in #1505 review:

1. `--session` flag was still visible in `opencli browser --help` and could
   be used as a public entrance, contradicting "positional only" UX.
   Fix: switch from `.requiredOption()` to `.addOption(new Option(...).hideHelp())`.
   The flag is preserved as an internal API for the daemon protocol and direct
   `program.parseAsync` callers (tests), but is no longer documented or
   surfaced in structured help.

2. `opencli browser --session foo state` still succeeded. Now the argv
   preprocessor throws `BrowserSessionArgvError` when root `browser` is
   followed by `--session`, and main.ts catches it and exits with a
   user-facing usage error pointing to the positional form.

3. Missing-session error message exposed the internal flag:
   `required option '--session <name>' not specified`. Now `getBrowserSession()`
   in the action body throws `<session> is a required positional argument:
   opencli browser <session> <command>`, and commander no longer guards the
   hidden option.

Also (per @WAWQAQ) rename placeholder `<sessionname>` -> `<session>` everywhere
user-facing — shorter, matches CLI convention. The help text "<session> is a
required positional: pass the name of the browser session..." carries the
"name" semantics in description, not in the placeholder itself.

Sync surfaces:
  - src/cli.ts — usage line, addOption with hideHelp, descriptions
  - src/cli-argv-preprocess.ts — throw on --session form
  - src/cli-argv-preprocess.test.ts — refusal test for old form
  - src/cli.test.ts — assertions updated for hidden option + new error path
  - src/help.ts — read `_usage` private field to respect `.usage()` override
    (commander's `.usage()` getter returns auto-generated form if not set,
    which would otherwise pollute every namespace's usage string)
  - src/main.ts — catch BrowserSessionArgvError, stderr + exit
  - README.md / README.zh-CN.md
  - docs/guide/browser-bridge.md / docs/zh/guide/browser-bridge.md
  - skills/opencli-browser/SKILL.md / skills/opencli-usage/SKILL.md
  - CHANGELOG.md

Manual smoke tests (against built dist):
  - `opencli browser --help` shows `Usage: opencli browser <session> <command> [options]`
  - `opencli browser --help` Options block does NOT show `--session`
  - `opencli browser --session foo state` → friendly error, no commander stacktrace
  - `opencli browser state` → `<session> is a required positional argument: opencli browser <session> <command>`
  - `opencli browser foo state` → parses correctly
@jackwener
fix: inject <session> into subcommand help paths and drop stale sessi…
e2eca71 
…ons ref

Two follow-up blockers from #1505 review:

1. Subcommand help and structured help still rendered the command path
   without the parent's positional. `opencli browser foo state --help`
   showed `Usage: opencli browser state [options]`, which would lead
   users (and agents reading structured help) to think
   `opencli browser state` was a valid invocation. Now:

   - `commanderPath()` injects an ancestor's leading-positional placeholder
     (extracted from its `.usage()` override) between the ancestor's name
     and the next path segment when building paths upward.
   - `commandPathFromRoot()` strips placeholder segments (e.g. `<session>`)
     from the relative `name` field so agents can still address subcommands
     by their leaf name; placeholders remain in the `command` / `usage`
     display paths.
   - `program.configureHelp({ commandUsage: ... })` is applied recursively
     to every descendant of `browser`, because commander does NOT inherit
     `configureHelp` into subcommands.

   Result:
     opencli browser <session> click --help
     -> Usage: opencli browser <session> click [target] [options]

   Daemon, plugin, adapter, profile namespaces (no `.usage()` override)
   are unaffected.

2. `skills/opencli-browser/SKILL.md` still referenced
   `opencli browser sessions`, which was removed in #1470. Replaced the
   sentence with the underlying invariant ("Bound sessions have no
   OpenCLI idle-close timer; the binding lasts until `unbind`, tab close,
   window close, or daemon restart") without mentioning the deleted
   command.

Tests:
  - cli.test.ts: structured help expectations updated to include
    `<session>` in command/usage paths (3 tests)
  - cli-argv-preprocess.test.ts: 12 tests still green
  - 1136/1137 unit+extension green (1 unrelated skip)
  - typed-error-lint baseline 189
  - silent-column-drop baseline 103
@jackwener jackwener merged commit 0e168d5 into main May 12, 2026
13 checks passed
@jackwener jackwener mentioned this pull request May 13, 2026
Merged
4 tasks
jackwener added a commit that referenced this pull request May 13, 2026
@jackwener
ci(e2e): drop e2e-headed from pull_request trigger (#1521)
fd438c2 
Per-PR e2e-headed Chrome was the dominant PR-time wait (~10-15 min on
two platforms) and on fork PRs blocks behind maintainer approval, while
the actually-blocking failures it caught in the last 30 days were all
e2e-test migrations missed by the authoring PR (#1461 / #1505 workspace
->session) rather than real regressions the unit/typecheck tier missed.

PR feedback path is now:
  - typecheck / unit / lint / adapter / build  ← `pull_request` (ci.yml)
  - extension typecheck / build                ← `pull_request` (build-extension.yml)
  - docs build                                  ← `pull_request` (doc-check.yml)
  - security audit                              ← `pull_request` (security.yml)

E2E-headed Chrome guards:
  - push to main / dev (watched paths)
  - push v* tag (release)
  - nightly cron 08:00 UTC (added: catches Chrome version drift / flake
    drift even when no commits touch watched paths)
  - workflow_dispatch (manual when a PR really wants e2e signal)

smoke-test was already gated on `schedule || workflow_dispatch` only
(ci.yml), so no change needed there.
@jackwener jackwener mentioned this pull request May 13, 2026
Merged
5 tasks
@tkwong tkwong mentioned this pull request May 14, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@jackwener