refactor(doctor): drop --no-live / --sessions + dead protocol chain#1470
Merged
Conversation
Doctor's job is browser-bridge health diagnosis. The `--no-live` flag let users skip the connectivity probe (= the core diagnostic), and `--sessions` listed automation sessions (a separate concern not part of health). Both flags accreted features that violated the command's first-principles purpose. Cleanup chain (removing dead code surfaced by the flag removal): - `--no-live` / `--sessions` flags removed from `opencli doctor` - `DoctorOptions.live` / `DoctorOptions.sessions` removed - `DoctorReport.sessions` removed - `[SKIP] Connectivity` render branch removed (always-live now) - `listSessions()` removed (only consumer was doctor) - `'sessions'` action removed from daemon-client protocol type - `BrowserSessionInfo` type removed (no remaining consumers) - extension `handleSessions` action handler removed (1.0.12) - extension test "reports sessions per session" removed - `OPENCLI_BROWSER_IDLE_TIMEOUT` test rewired to 'cookies' action Verification: - root typecheck + extension typecheck pass - doctor.test.ts 17/17 pass - extension/background.test.ts 49/49 pass - typed-error-lint 189/189 baseline - silent-column-drop 103/103 baseline - build + extension build green
jackwener
commented
May 11, 2026
jackwener
left a comment
jackwener
left a comment
Owner
Author
There was a problem hiding this comment.
LGTM. 深审过，scope 准 + 语义保留 + 验证齐。
Dead-chain 完整性 (5 层)：
- CLI flags
--no-live/--sessions→ 删 DoctorOptions.live/.sessions/DoctorReport.sessions→ 删listSessions()daemon-client → 删（grep 全 repo 唯一 consumer 是 doctor）'sessions'在DaemonCommand.actionunion → 删（src/browser/daemon-client.ts:24 已不含）- extension
handleSessions+BrowserSessionInfo公共类型 → 删
repo-wide grep 验证（不含 dist/ 构建产物 + CHANGELOG 说明）：
listSessions/BrowserSessionInfo/handleSessions/--no-live/'sessions'/"sessions"在 src/extension/tests/docs/skills/clis 全 0 remnant ✓
语义保留：
- 老
live: false分支的 minimal auto-start (bridge.connect timeout 5s → close) 不再需要——新流程checkConnectivity()永远跑，bridge.connect()本身就是 auto-start 路径。closeWindow+bridge.close在 finally 链清理 ✓ daemonFlaky/extensionFlaky判定从connectivity?.ok && ...简化为connectivity.ok && ...（connectivity 现在必有），逻辑一致
Test rewiring 抽查：
OPENCLI_BROWSER_IDLE_TIMEOUT从action:'sessions'改action:'cookies'是有效 coverage——handleCommand入口 dispatch 内idleTimeout提取逻辑 action-agnosticdoctor.test.ts把mockConnect.mockResolvedValue(...)提到 beforeEach 当 happy default，failure-path 测试用mockRejectedValueOnce显式驱动，覆盖等价于老live:true/false矩阵
Version 一致性：
extension/manifest.json+extension/package.json同步 1.0.11 → 1.0.12 ✓compatRange: '>=1.7.0'未改是对的——extension 删旧 action 不破老 CLI（老 CLI 即使发'sessions'也只是 extension 返 unknown action，CLI 已不会再发）
本地复跑：
npx vitest run --project unit src/doctor.test.ts→ 17/17 ✓npx vitest run --project extension extension/src/background.test.ts→ 49/49 ✓
Skill doc 一致：skills/opencli-usage/SKILL.md 同步 always-live + 只剩 -v flag 一致 ✓
CI 红点确认非 PR 引起：douyin/_shared/creation-id.test.js 4-char random ID 100 个偶尔碰撞——pre-existing flake，作为单独 cleanup（增加熵 / N-1 tolerance）跟踪即可，不阻塞本 PR。
Nice-to-have (不阻塞)：删掉的 skips auto-start in no-live mode when daemon is already running 测试覆盖的是「daemon already running 时不重复 bridge.connect」的优化路径。新代码 checkConnectivity 内 bridge.connect 的 fast-path（health.state ready 时直接 attach 不 spawn）继续生效，覆盖被 fast-path 自身吸收，无 coverage gap 但失去显式 regression gate。如果之后 fast-path 回归，最先发现的会是 doctor live probe 在 daemon-healthy 场景下的 latency tail。可选 follow-up：加一条 fast-path 计数 assertion。
合入授权我没有（pr-manager routing rule），但 review 这条通过。你按 WAWQAQ 在 msg=05809474 的 "review pass 才 merge" 收尾即可。
jackwener
added a commit
that referenced
this pull request
May 12, 2026
…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
added a commit
that referenced
this pull request
May 12, 2026
…al (#1505) * refactor(browser): replace --session flag with <sessionname> positional 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 * fix(cli-argv): only rewrite when `browser` is the root command 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. * fix(cli-argv): hide --session flag, fail-fast on retired form, rename 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 * fix: inject <session> into subcommand help paths and drop stale sessions 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
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
1 participant
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.
Summary
@WAWQAQdirective:opencli doctorshouldn't need--no-liveor--sessions. First-principles: doctor diagnoses browser-bridge health — the live connectivity probe is the core diagnostic, and session enumeration is a separate concern. Both flags are accretion.Removing the flags surfaces a dead chain that goes with them:
--no-live/--sessionsflags → removedDoctorOptions.live/.sessions→ removedDoctorReport.sessions→ removed[SKIP] Connectivity (--no-live)render branch → removed (always-live now)listSessions()in daemon-client → removed (only consumer was doctor)'sessions'action in protocol type → removedBrowserSessionInfopublic type → removed (no remaining consumers)handleSessionsaction handler → removed (extension 1.0.12)reports sessions per session→ removedOPENCLI_BROWSER_IDLE_TIMEOUTtest rewired to a still-valid action (cookies)Net: -222 lines, 12 files. Extension bumped 1.0.11 → 1.0.12.
Test plan
npx tsc --noEmitroot passnpm run typecheck --prefix extensionpassnpx vitest run --project unit src/doctor.test.ts→ 17/17npx vitest run --project extension extension/src/background.test.ts→ 49/49npm run check:typed-error-lintbaseline 189/189npm run check:silent-column-dropbaseline 103/103npm run build+ extension build pass--no-live/--sessions/BrowserSessionInfo/listSessions/handleSessions/'sessions'action grep remnants