Skip to content

fix(cli): restore auto-open in sh1pt login — --no-browser default disables it permanently - #972

Merged
ralyodio merged 1 commit into
profullstack:masterfrom
Larslllllll:fix/login-no-browser-default
Aug 19, 2026
Merged

fix(cli): restore auto-open in sh1pt login — --no-browser default disables it permanently#972
ralyodio merged 1 commit into
profullstack:masterfrom
Larslllllll:fix/login-no-browser-default

Conversation

@Larslllllll

Copy link
Copy Markdown
Contributor

Problem

sh1pt login never opens the verification URL, and --no-browser does nothing.

packages/cli/src/commands/login.ts declares the flag like this:

.option('--no-browser', 'do not auto-open the verification URL', false)

Commander derives a boolean option named browser from --no-browser and defaults it to true.
Supplying an explicit third argument overrides that default, so opts.browser resolves to false
even when the user never passed the flag. The action guard is:

if (opts.browser !== false) tryOpenBrowser(url);

With the default forced to false, that condition never holds. Auto-open is therefore disabled for
every user, and --no-browser is a no-op because the value was already false.

The comment above the command documents the intended behaviour ("attempts to open the URL in the
user's browser"), so this is a defect rather than a deliberate default.

Verification

Reproduced against commander@12 with the exact option declarations:

declaration argv opts.browser opens browser?
.option('--no-browser', ..., false) (current) (none) false no — bug
.option('--no-browser', ..., false) (current) --no-browser false no
.option('--no-browser', ...) (fixed) (none) true yes — intended
.option('--no-browser', ...) (fixed) --no-browser false no

Fix

Remove the explicit false default so Commander's negated-boolean semantics apply:

no flag        -> opts.browser === true   (browser opens, as documented)
--no-browser   -> opts.browser === false  (browser stays closed)

The action guard is left untouched, so behaviour is unchanged for anyone who does pass --no-browser.

Test

Adds packages/cli/src/commands/login.test.ts (vitest, matches the existing
packages/**/src/**/*.test.ts pattern). It asserts both directions and guards against the explicit
default being reintroduced. parseOptions is used so the command's action never runs — the test
touches no network, consistent with the 5s contract-test timeout in vitest.config.ts.

Scope

One-line source change plus a focused test. No dependency, build, or public API changes.


Separate issue noticed while verifying (happy to send a follow-up PR)

The three published packages — @profullstack/sh1pt, -core, -policy — declare no engines
field, but the CLI relies on global fetch (login.ts, build.ts, cloud-vault.ts,
core/setup-helpers.ts), which requires Node >= 18. mise.toml pins Node 22 and the workflows run
22 (threatcrush uses 20). A user on Node 16 installs successfully and then hits
ReferenceError: fetch is not defined at runtime instead of an install-time engine warning.
Adding "engines": { "node": ">=20" } (or >=22 to match mise/CI exactly) would surface that at
install time. Say the word and I will open it as its own PR.

…fault on --no-browser

`.option('--no-browser', ..., false)` overrides the default Commander assigns to a
negated boolean flag. `opts.browser` therefore resolved to `false` even when the flag
was absent, so `opts.browser !== false` never held and the verification URL was never
opened. That silently disabled auto-open for every user and made `--no-browser` a no-op.

Removing the explicit default restores Commander's intended semantics:
  no flag        -> opts.browser === true   (browser opens, as documented)
  --no-browser   -> opts.browser === false  (browser stays closed)

Adds packages/cli/src/commands/login.test.ts covering both directions plus a guard
against the explicit default returning.
@ralyodio
ralyodio merged commit 0d4a0af into profullstack:master Aug 19, 2026
6 checks passed
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