Skip to content

fix(cli): make session close accept --session and be idempotent - #420

Open
ankitranjan7 wants to merge 1 commit into
mainfrom
fix/cli-session-grammar
Open

fix(cli): make session close accept --session and be idempotent#420
ankitranjan7 wants to merge 1 commit into
mainfrom
fix/cli-session-grammar

Conversation

@ankitranjan7

Copy link
Copy Markdown
Contributor

webcmd session close --session session_abc told you to run webcmd --session session_abc session close — and that command then failed with missing required argument 'session-id'. The suggestion was impossible to satisfy: --session is a root selector everywhere in the CLI except session close, where the id is a positional. A contestant in a recent eval burned three turns on that loop and never closed its session.

What changed

  1. session close takes [session-id] (optional) and falls back to the root --session selector. With neither, it errors naming both forms.
  2. rejectMisplacedSessionSelectorArgv is command-aware: for session close it rewrites --session <id> into the positional instead of throwing a suggestion that cannot work. Every other command keeps the existing SESSION_SELECTOR_POSITION error.
  3. session close is idempotent — an unknown / already-reaped / already-closed Session returns { ok: true, closed: false, alreadyClosed: true, session } and exit 0. InvalidSessionSelectorError (badid) still exits 2; every other failure still propagates.
  4. src/daemon/server.ts kept no error code on unhandled command errors, so SESSION_NOT_FOUND arrived at the client as a generic BROWSER_COMMAND with only a message. The code is now preserved on the wire, which is what the idempotency check branches on.
  5. --force is untouched.

Before / After

Before (on main):

$ node dist/src/main.js session close --session session_abc
error: SESSION_SELECTOR_POSITION: --session must appear before the command.
       Use: webcmd --session session_abc session close
$ node dist/src/main.js --session session_abc session close
error: missing required argument 'session-id'

After (real output, isolated HOME):

=== webcmd session close --session session_abc
ok: true
closed: false
alreadyClosed: true
session: session_abc
exit=0

=== webcmd --session session_abc session close
ok: true
closed: false
alreadyClosed: true
session: session_abc
exit=0

=== webcmd session close session_00000000-0000-0000-0000-000000000000
ok: true
closed: false
alreadyClosed: true
session: session_00000000-0000-0000-0000-000000000000
exit=0

=== webcmd session close badid
ok: false
error:
  code: INVALID_SESSION_SELECTOR
  message: 'Session selector must be an opaque Session ID: badid'
  help: Run `webcmd session create` and pass the returned `session_...` ID.
  exitCode: 2
exit=2

=== webcmd session close            (no id at all)
ok: false
error:
  code: ARGUMENT
  message: Missing Session ID.
  help: Use `webcmd session close <session-id>` or `webcmd --session <session-id> session close`.
  exitCode: 2
exit=2

Real Session, closed twice, then with --force — all exit 0:

=== close once  → closed: false / alreadyIdle: true   exit=0
=== close twice → closed: false / alreadyIdle: true   exit=0
=== close --force → closed: false / alreadyIdle: true / displaced: null   exit=0

Tests

  • src/cli-argv-preprocess.test.ts: --session, --session=, and --profile x prefixed forms all rewrite to session close <id>; a valueless --session is dropped so the command reports both accepted forms.
  • src/cli.test.ts: missing Session closes idempotently; root --session selector is accepted; badid still throws INVALID_SESSION_SELECTOR exit 2.
  • tests/e2e/management.test.ts: five tests that RUN the CLI — the suggestion round-trip (both spellings, asserting no usage error), unknown id idempotent, real Session closed twice, badid exit 2, and the no-id error text.
  • npm run typecheck, npx vitest run --project unit (154 files, 2707 passed), npm run build all green. The new e2e tests pass; management.test.ts > list shows all registered commands fails in this worktree on main too (fixture-plugin install), unrelated to this change.

🤖 Generated with Claude Code

`webcmd session close --session <id>` errored with SESSION_SELECTOR_POSITION
and suggested `webcmd --session <id> session close`, which then failed with
`missing required argument 'session-id'` — the suggested command could not
work, because `session close` takes the id as a positional.

- `session close` now takes `[session-id]` and falls back to the root
  `--session` selector; with neither, it names both forms.
- The argv preprocessor rewrites `--session <id>` into the positional for
  `session close` instead of throwing a broken suggestion.
- Closing an unknown / already-reaped Session returns
  `{ ok: true, closed: false, alreadyClosed: true, session }` with exit 0.
  A malformed selector stays INVALID_SESSION_SELECTOR / exit 2.
- The daemon now keeps the CliError code on unhandled command errors, so
  SESSION_NOT_FOUND survives the wire instead of arriving as BROWSER_COMMAND.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

🟠 Maintainer review suggested — low confidence

The automated review could not reach a fully supported conclusion.

This review is advisory and does not block merging.

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.

1 participant