Skip to content

fix(cli): wire the contexts command into the CLI program - #4369

Merged
diegosouzapw merged 1 commit into
release/v3.8.30from
fix/cli-contexts-wiring
Jun 20, 2026
Merged

diegosouzapw merged 1 commit into
release/v3.8.30from
fix/cli-contexts-wiring

Conversation

@diegosouzapw

Copy link
Copy Markdown
Owner

Found by E2E testing remote mode

omniroute contexts list/current/use fell through to serve (too many arguments for 'serve'). Worse, connect itself prints "Switch back to local with: omniroute contexts use default" — pointing users at a command that didn't exist.

Root cause

bin/cli/commands/contexts.mjs fully implements registerContexts (subcommands: list/add/use/current/show/remove/rename/export/import) and had a unit test — but that test used a fake program object. registry.mjs never imported or called registerContexts, so the command was never wired into the real CLI. The isolated fake-program test passed regardless, hiding the gap.

Fix

Add the import + registerContexts(program) call alongside the other remote-mode commands (connect/tokens/configure). One-line wiring; the command's implementation was already complete.

Validation

  • TDD: a new test builds the real program via createProgram() and asserts contexts/connect/tokens/configure are top-level commands and that contexts exposes list/use/current. It fails before the fix, passes after — exactly the class the old fake-program test could not catch.
  • Verified live: omniroute contexts list / contexts current now render the context table instead of erroring; contexts use is available to switch back to local.

Client-side only — no server change, no redeploy. Completes the remote-mode CLI UX surfaced during the item-5 E2E.

Found by end-to-end testing of remote mode: `omniroute contexts list/current/use`
fell through to `serve` ("too many arguments for 'serve'"). `connect` even tells
users "Switch back to local with: omniroute contexts use default" — a dead command.

Root cause: `bin/cli/commands/contexts.mjs` implements `registerContexts` (with
list/add/use/current/show/remove/rename/export/import), and it had an isolated unit
test using a FAKE program — but `registry.mjs` never imported or called it, so the
command was never wired into the real CLI. Add the import + registration alongside
the other remote-mode commands (connect/tokens/configure).

Regression test: build the REAL program via createProgram() and assert the
top-level contexts/connect/tokens/configure commands exist and that `contexts`
exposes its list/use/current subcommands (RED before, GREEN after). The previous
isolated fake-program test could not catch the missing wiring.
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request registers the missing contexts command in the CLI registry and adds a regression test to verify that remote-mode commands and their subcommands are correctly wired. The feedback recommends adding an assertion to ensure the contexts command is defined before accessing its properties in the test, preventing potential TypeError exceptions.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +256 to +257
const contexts = program.commands.find((c: any) => c.name() === "contexts");
const subs = contexts.commands.map((c: any) => c.name());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The find method can return undefined if the 'contexts' command is not found. To prevent a TypeError when accessing contexts.commands and to provide a clearer test failure message, assert that contexts is defined before accessing its properties.

  const contexts = program.commands.find((c: any) => c.name() === "contexts");
  assert.ok(contexts, "expected 'contexts' command to be registered");
  const subs = contexts.commands.map((c: any) => c.name());

@diegosouzapw
diegosouzapw merged commit 1a1ef10 into release/v3.8.30 Jun 20, 2026
4 checks passed
@diegosouzapw
diegosouzapw deleted the fix/cli-contexts-wiring branch June 20, 2026 09:38
@diegosouzapw diegosouzapw mentioned this pull request Jun 20, 2026
tkgo11 pushed a commit to tkgo11/OmniRoute that referenced this pull request Sep 23, 2026
…apw#4369)

Found by end-to-end testing of remote mode: `omniroute contexts list/current/use`
fell through to `serve` ("too many arguments for 'serve'"). `connect` even tells
users "Switch back to local with: omniroute contexts use default" — a dead command.

Root cause: `bin/cli/commands/contexts.mjs` implements `registerContexts` (with
list/add/use/current/show/remove/rename/export/import), and it had an isolated unit
test using a FAKE program — but `registry.mjs` never imported or called it, so the
command was never wired into the real CLI. Add the import + registration alongside
the other remote-mode commands (connect/tokens/configure).

Regression test: build the REAL program via createProgram() and assert the
top-level contexts/connect/tokens/configure commands exist and that `contexts`
exposes its list/use/current subcommands (RED before, GREEN after). The previous
isolated fake-program test could not catch the missing wiring.
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