fix: use || instead of ?? for CLI flag override precedence in preparse hook @W-20991305@#2554
Merged
jfeingold35 merged 1 commit intosalesforcecli:mainfrom Jan 30, 2026
Conversation
Open
Contributor
|
@jon-freed , I've done some local testing of these changes, and they seem solid. Great find, and great fix! I'll make sure it gets into |
jfeingold35
approved these changes
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-<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 salesforcecli#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.
c2faa0f to
d7ec648
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes a bug where CLI arguments fail to override
--flags-dirvalues in certain scenarios. The bug is in the tpreparse hook where it is using??(nullish coalescing) instead of||(logical OR) when checking if a CLI flag was already present.Bug Details
Introduced in: Commit c83f81a (2025-04-22, "chore: bump wireit, fix types")
Affected versions: SF CLI 2.100.0 through 2.120.x
The change added a necessary guard
(flagOptions.char &&)to prevent checking for undefined short chars, but incorrectly used??instead of||to chain conditions:Because
??only evaluates the right side when left isnull/undefined, andfalseis neither, the long-form check was skipped when the short-char check returnedfalse.Affected Scenarios
The bug affects scenarios where:
{ char: 'l' }for--test-level)--test-levelinstead of-l)--flags-dirExample:
5 specific scenarios are affected (out of 14 tested):
--no-<flag>form--<flag>long form--no-<flag>long formRoot Cause
The ESLint rule
@typescript-eslint/prefer-nullish-coalescinglikely suggested the change from||to??, but the rule's advice was incorrect in this context. The fix includes aneslint-disablecomment to prevent this from recurring.Acceptance Criteria
--flags-dirvalues as documented in PR feat: add preparse hook for --flags-dir #1536Testing Notes
Added systematic tests organized by:
The tests verify the documented behavior from PR #1536:
Checklist
What issues does this PR fix or reference?
Fixes forcedotcom/cli#3486
Fixes regression introduced in c83f81a. Restores the behavior documented in PR #1536.
(edit by @jfeingold35 , this PR addresses work item @W-20991305@)