Skip to content

fix: detect a negated option's positive pair by attribute, not flag string - #2557

Closed
spokodev wants to merge 1 commit into
tj:masterfrom
spokodev:fix/negated-option-default-by-attribute
Closed

fix: detect a negated option's positive pair by attribute, not flag string#2557
spokodev wants to merge 1 commit into
tj:masterfrom
spokodev:fix/negated-option-default-by-attribute

Conversation

@spokodev

Copy link
Copy Markdown

Problem

When only a negated option is defined, Commander gives it an implicit true
default; when a positive counterpart is also defined, it must not, because
the pair shares a single value (per the v15 behavior from #2405).

The "is there a positive counterpart?" check reconstructs the --foo long flag
from --no-foo and looks it up by flag string:

const positiveLongFlag = option.long.replace(/^--no-/, '--');
if (!this._findOption(positiveLongFlag)) { /* treat as lone → default true */ }

So it misses a positive option that shares the same attribute but is spelled
differently — a short-only -c, or a camelCase --fooBar paired with a
kebab-case --no-foo-bar. Those --no-* options are then wrongly defaulted to
true when unused, instead of left undefined, silently flipping program logic:

const p = new Command();
p.option('-c', 'add cheese').option('--no-c', 'remove cheese');
p.parse([], { from: 'user' });        // neither supplied
p.opts();  // { c: true }   ← expected {} (undefined)

The equivalent all-long dual (--cheese + --no-cheese) already behaves
correctly ({}), so the two spellings of the same construct diverge.

Fix

Detect the positive counterpart by attributeName() — the same way the internal
DualOptions helper already pairs positive/negative options — instead of
reconstructing and matching the flag string. Any spelling of the positive option
is then recognised as the pair. A genuinely lone --no-* option is unaffected
and still defaults to true.

Test

Added cases in tests/options.bool.test.js for a short-only positive (-c +
--no-c) and a camelCase/kebab mismatch (--fooBar + --no-foo-bar), asserting
the value stays undefined when unused, plus a lone --no-sauce asserting the
true default is preserved. The two mismatch cases fail on main; all pass with
the fix, and the full suite is green (1375 pass, 0 fail).

…tring

When only a negated option is defined, Commander gives it an implicit
`true` default; when a positive counterpart is also defined, it must not
(the pair shares one value). That "is there a positive counterpart?"
check reconstructed the `--foo` long flag from `--no-foo` and looked it
up by flag string, so it missed a positive option spelled differently but
sharing the same attribute — a short-only `-c`, or `--fooBar` paired with
`--no-foo-bar`. Those `--no-*` options were then wrongly defaulted to
`true` when unused instead of left `undefined`.

Match the positive counterpart by `attributeName()`, the same way the
internal `DualOptions` helper already pairs them, so any spelling of the
positive option is recognised. A genuinely lone `--no-*` option is
unaffected and still defaults to `true`.
@shadowspawn

Copy link
Copy Markdown
Collaborator

AI slop. Account has created 245 Pull Requests in July.

@spokodev

Copy link
Copy Markdown
Author

Understood — but I wanted to flag that the underlying bug here is real and reproducible:

const p = new Command();
p.option('-c', 'add cheese').option('--no-c', 'remove cheese');
p.parse([], { from: 'user' });  // neither supplied
p.opts();                       // { c: true } — expected {}

When a --no-x option's positive counterpart is spelled differently (a short-only -c, or --fooBar vs --no-foo-bar), the unused negated option is defaulted to true instead of left undefined, silently flipping program logic — while the all-long dual (--cheese/--no-cheese) correctly returns {}. The fix pairs them by attributeName(), the same way the internal DualOptions helper already does, and the full test suite stays green.

No worries if you'd still rather not take it — just didn't want a genuine correctness bug to slip by.

@shadowspawn

shadowspawn commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

This is a low value fix probably completely generated by AI. (And hence much less effort spent creating the Pull Request than by this human reviewer.)

This is a correctness fix for an edge case which is outside the intended usage.

The Pull Request does not follow the contributing guidelines or the Pull Request template.

Fail. Fail. Fail.

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