Skip to content

Conversation

@jfeingold35
Copy link
Contributor

@jfeingold35 jfeingold35 commented Jan 30, 2026

…e hook

The preparse hook was using ?? (nullish coalescing) instead of || (logical OR) when checking if a CLI flag was already present. This caused a bug where CLI arguments would not override flags-dir values in certain scenarios.

Bug introduced in: c83f81a (2025-04-22, SF CLI 2.100.0) The change added a necessary guard (flagOptions.char &&) to prevent checking for undefined short chars, but incorrectly used ?? instead of || to chain the conditions. Since ?? only evaluates the right side when left is null/undefined, and false is neither, the subsequent checks were skipped.

Affected scenarios (5 of 14 systematic test cases):

  • String flag with char defined, CLI uses long form (--test-level vs -l)
  • Boolean flag with char defined, CLI uses long form (--dry-run vs -d)
  • Boolean allowNo flag, CLI uses --no- form to override file
  • Boolean allowNo with char, CLI uses -- to override no- file
  • Boolean allowNo with char, CLI uses --no- to override file

Example: Flag defined as Flags.string({ char: 'l' }) with flags-dir containing a 'test-level' file. Running sf cmd --test-level NoTestRun should override the file value, but with ?? it was erroneously combined (conflict error).

This fix restores the documented behavior from PR #1536: "Flags/values provided by the user will override any flag/values found by --flags-dir -- unless the flag supports multiple, in which case they will all be combined."

The eslint-disable is intentional because the lint rule incorrectly suggests ?? when || is actually required for proper boolean short-circuit evaluation.

What does this PR do?

Describe the feature with use cases or bug details. Interesting/complex design choices or weird workarounds should be documented in the code but can be included here instead if there is a valid reason it shouldn't be in the code.

Acceptance Criteria

How do you know this change is successful? What is the scope of this change? What tests test the criteria (or why no tests)?

The Acceptance Criteria can be copied from the work item if they exist there but it is useful to have on the PR when reviewing the code.

Testing Notes

Anything additional to note about testing this PR. How did you test it? Special setup? Additional manual test cases? Things not tested?

Checklist

  • This change has been approved by the CLI Review Board or approval isn't required. Anything that adds/changes/deletes commands, flags, output, or json output has to be reviewed.
  • Does this follow the deprecation policy?

What issues does this PR fix or reference?

@W-20991305@

…e hook

The preparse hook was using ?? (nullish coalescing) instead of || (logical OR)
when checking if a CLI flag was already present. This caused a bug where CLI
arguments would not override flags-dir values in certain scenarios.

Bug introduced in: c83f81a (2025-04-22, SF CLI 2.100.0)
The change added a necessary guard (flagOptions.char &&) to prevent checking
for undefined short chars, but incorrectly used ?? instead of || to chain the
conditions. Since ?? only evaluates the right side when left is null/undefined,
and false is neither, the subsequent checks were skipped.

Affected scenarios (5 of 14 systematic test cases):
- String flag with char defined, CLI uses long form (--test-level vs -l)
- Boolean flag with char defined, CLI uses long form (--dry-run vs -d)
- Boolean allowNo flag, CLI uses --no-<flag> form to override <flag> file
- Boolean allowNo with char, CLI uses --<flag> to override no-<flag> file
- Boolean allowNo with char, CLI uses --no-<flag> to override <flag> file

Example: Flag defined as Flags.string({ char: 'l' }) with flags-dir containing
a 'test-level' file. Running `sf cmd --test-level NoTestRun` should override
the file value, but with ?? it was erroneously combined (conflict error).

This fix restores the documented behavior from PR #1536:
"Flags/values provided by the user will override any flag/values found by
--flags-dir -- unless the flag supports `multiple`, in which case they will
all be combined."

The eslint-disable is intentional because the lint rule incorrectly suggests ??
when || is actually required for proper boolean short-circuit evaluation.
@jfeingold35 jfeingold35 merged commit 80ca406 into main Jan 30, 2026
34 of 36 checks passed
@jfeingold35 jfeingold35 deleted the qa/pr-2554-b branch January 30, 2026 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants