fix(handlers): standardize mutual flag parsing behavior - #2263
Conversation
|
Claude Security Review: no high-confidence findings. (run) |
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Looks good
Nice consolidation of scattered "at most/exactly one of these flags" checks into a single assertMutuallyExclusiveFlags helper. I traced each converted call site and the semantics are preserved:
- The old callers of
assertMutuallyExclusiveInputswere manually convertingfalsebooleans toundefined(e.g.,flags["clear-description"] || undefined). The new helper treats bothundefinedandfalseas "not set", so the behavior matches without callers needing to normalize. - The
exactlyOne: trueoption correctly replaces the previous "both required OR both forbidden" branches (e.g., inonline-eval/create,online-insight/create, api-key/oauth2 create+update,sessionSource,harnessexport,payment-connector,gateway-targetadd). The "required" side is now folded into the same error path (specify exactly one of ...), which is a small user-visible message change already reflected in the updated test regexes. - The variables
hasApiKey/hasSecretRef/hasClientSecret/hasAgent/hasDataSourceleft behind after the removed branches are still used downstream — no dead code. - Because the
exactlyOnebranch fires wheneverdefinedFlags.length !== 1, the "mutually exclusive" branch is unreachable whenexactlyOneis set. That's the intended design (produces thespecify exactly onemessage in both the 0-set and >1-set cases), and the unit tests inutils.test.tsxpin that behavior.
The direct unit tests for the helper cover the interesting cases (empty, single set, false-boolean, exactlyOne with 0/1/many, listing only the offending flags in the "mutually exclusive" message). No new telemetry is warranted for a pure refactor.
No changes required.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## refactor #2263 +/- ##
============================================
+ Coverage 97.04% 97.05% +0.01%
============================================
Files 566 566
Lines 39353 39228 -125
============================================
- Hits 38190 38073 -117
+ Misses 1163 1155 -8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Problem
CLI handlers hand-roll "mutually exclusive" and "specify exactly one of" flag checks with at least three message formats, so identical failures read differently across commands.
Solution
assertMutuallyExclusiveInputswithassertMutuallyExclusiveFlagsthat is general enough to handle all of the mutually exclusive checks we make today.Verification
manually tested a migrated case: