Reject flag-like values for option arguments (#103) - #342
Conversation
`jss start --single-user-name --idp` previously had Commander silently
consume `--idp` as the username, which left --idp unset, broke the IdP
config (no /.well-known/openid-configuration), and surfaced as a
confusing downstream error: "issuer has no registration endpoint" —
plus a banner line of "Single-user: --idp (registration disabled)".
Adds a `program.hook('preAction', ...)` validator that runs before
every subcommand action and:
- Errors with a clear "looks like a flag, not a value" message for
any string option whose value starts with `--`.
- Errors with "got a non-numeric value (parsed as NaN)" for numeric
options like `--port` whose value-coercer was fed a flag.
Both messages include a "Hint:" line with the canonical kebab-case
form of the flag and an example value.
New `test/cli-flag-like-values.test.js` spawns the bin and asserts
the four cases: string-option flag value, unknown-follow-up flag,
numeric-option flag value, and a clean run that proves no
false-positive on legitimate values.
Full suite: 537/537.
There was a problem hiding this comment.
Pull request overview
Adds a CLI guard to prevent Commander from treating the next flag token as an option value (e.g. --single-user-name --idp), improving error clarity and preventing downstream misconfiguration (notably IdP setup).
Changes:
- Add a
program.hook('preAction', …)validator to reject string option values that start with--and numeric options parsed asNaN. - Add regression tests that spawn
bin/jss.jsand assert non-zero exits + helpful stderr messages for flag-like values.
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| bin/jss.js | Adds a preAction hook to detect and error on flag-like option values / NaN numeric coercions before executing subcommand actions. |
| test/cli-flag-like-values.test.js | Adds CLI-level regression coverage for the flag-swallowing behavior and verifies no false positives on a valid --print-config run. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const r = runCli(['start', | ||
| '--port', '4582', | ||
| '--root', '/tmp/jss-103-sanity-doesnotneedtoexist', | ||
| '--single-user-name', 'alice', |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // value, which is the whole reason the bug exists. We use a flag | ||
| // commander doesn't know about ("--unknown-flag") so commander | ||
| // doesn't reroute it through its own argument-count error. | ||
| const r = runCli(['start', '--idp-issuer', '--unknown-flag']); | ||
| assert.notStrictEqual(r.status, 0); | ||
| assert.match(r.stderr, /--idp-issuer value "--unknown-flag" looks like a flag/); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| function runCli(args) { | ||
| return spawnSync(process.execPath, [BIN, ...args], { encoding: 'utf8' }); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
jss start --single-user-name --idphad Commander silently consume--idpas the username. The IdP flag was lost,/.well-known/openid-configurationreturned 404, and the error surface was a confusing "issuer has no registration endpoint" plus a banner lineSingle-user: --idp (registration disabled).This PR adds a single
program.hook('preAction', …)validator inbin/jss.jsthat runs before every subcommand action and:value "--foo" looks like a flag, not a valuemessage when the value starts with--.got a non-numeric value (parsed as NaN)when the value coercer (e.g.parseIntfor--port) was fed a flag.Hint:line with the canonical kebab-case flag name and an example value.Test plan
test/cli-flag-like-values.test.jsspawns the bin viachild_process.spawnSyncand asserts:--single-user-name --idpexits non-zero with the right error and hint.--idp-issuer --unknown-flagexits non-zero with the analogous message.--port --idpexits non-zero with the NaN-flavoured error.--print-configexits 0 (no false positives on legitimate values).Fixes #103.