-
Notifications
You must be signed in to change notification settings - Fork 83
Description
Summary
When using --flags-dir, command line arguments should override values from the flags directory for single-value flags (as documented in PR #1536). This works correctly in most cases, but fails when:
- A flag has a short/char form defined (e.g.,
-ofor--target-org) - The user specifies the long form on the command line (e.g.,
--target-org my-org) - A file exists in the flags-dir for that flag
In this scenario, the flags-dir value is incorrectly appended instead of being overridden.
Steps To Reproduce
See the automated tests in the accompanying PR: salesforcecli/cli#2554
The PR includes 14 test scenarios covering all combinations of flag types and short/char usage. The 5 scenarios that fail when the bug is present are:
| # | Scenario |
|---|---|
| 3 | String (single), has short/char, command line uses long form --flagname |
| 9 | Boolean, has short/char, command line uses long form --flagname |
| 11 | Boolean (allowNo), no short/char, command line uses --no-flagname |
| 13 | Boolean (allowNo), has short/char, command line uses long form --flagname |
| 14 | Boolean (allowNo), has short/char, command line uses long form --no-flagname |
Simple manual reproduction:
-
Create a flags directory:
mkdir my-flags echo "default-org" > my-flags/target-org
-
Run a command using the long form of a flag that has a short form:
sf org display --flags-dir ./my-flags --target-org other-org
-
Expected:
other-orgis used (command line value overrides flags-dir value) -
Actual: Both values are passed to the command, causing "flag can only be specified once" errors or unexpected behavior
Note: The bug does NOT occur if you use the short form (-o other-org) on the command line - only the long form (--target-org other-org) is affected.
Expected Result
Command line arguments should always override --flags-dir values for single-value flags, regardless of whether the user types the short form (-o) or long form (--target-org).
Actual Result
When a flag has a short/char form defined but the user types the long form on the command line, the override check fails and both the command line value and the flags-dir value are passed to the command.
Root Cause
In commit c83f81a ("chore: bump wireit, fix types"), the logical OR operators (||) in src/hooks/preparse.ts were changed to nullish coalescing operators (??). This was likely an unintended side effect of accepting a linter auto-fix suggestion from the @typescript-eslint/prefer-nullish-coalescing rule.
This change is incorrect because argv.includes() returns false (not null/undefined) when the flag isn't found. With ??, the false return value doesn't trigger evaluation of subsequent conditions, so the long-form check is skipped when a short/char form exists for the flag.
Before (correct):
(flagOptions.char && argv.includes(`-${flagOptions.char}`)) ||
argv.includes(`--${flagName}`) ||
(flagOptions.type === 'boolean' && flagOptions.allowNo && argv.includes(`--no-${flagName}`))After c83f81a (buggy):
(flagOptions.char && argv.includes(`-${flagOptions.char}`)) ??
argv.includes(`--${flagName}`) ??
(flagOptions.type === 'boolean' && flagOptions.allowNo && argv.includes(`--no-${flagName}`))Affected Scenarios
The bug affects two categories of cases:
1. Flag has short/char defined, but user types long form on command line:
| Flag Type | short/char? | Command Line Usage | Affected? |
|---|---|---|---|
| String (single) | yes | --flagname value |
Yes (#3) |
| Boolean | yes | --flagname |
Yes (#9) |
| Boolean (allowNo) | yes | --flagname |
Yes (#13) |
| Boolean (allowNo) | yes | --no-flagname |
Yes (#14) |
2. Boolean (allowNo) using --no-flagname form (third condition never evaluated):
| Flag Type | short/char? | Command Line Usage | Affected? |
|---|---|---|---|
| Boolean (allowNo) | no | --no-flagname |
Yes (#11) |
Not affected:
- Flags without short/char using long form (first condition is falsy,
??works correctly) - Flags where user types the short form (
-f) on the command line - Multiple-value flags (combine as expected)
Additional Information
The original --flags-dir feature was added in PR #1536, which documents this expected behavior:
"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."
Proposed Fix
Restore the || operators with an eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing comment explaining why ?? is incorrect here. This will prevent the linter from "fixing" it back to ?? in the future.
A PR with the fix and comprehensive tests will be submitted to salesforcecli/cli.
System Information
- Affected versions: Any version containing commit c83f81a (April 2025 onwards)
- OS: All platforms