Skip to content

fix(handlers): standardize mutual flag parsing behavior - #2263

Merged
Hweinstock merged 2 commits into
aws:refactorfrom
Hweinstock:feat/assert-mutually-exclusive-flags
Sep 10, 2026
Merged

fix(handlers): standardize mutual flag parsing behavior#2263
Hweinstock merged 2 commits into
aws:refactorfrom
Hweinstock:feat/assert-mutually-exclusive-flags

Conversation

@Hweinstock

@Hweinstock Hweinstock commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

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

  • Replace assertMutuallyExclusiveInputs with assertMutuallyExclusiveFlags that is general enough to handle all of the mutually exclusive checks we make today.
  • Migrate handlers with mutually-exclusive checks onto the shared utility.

Verification

  • All handler tests green; changed lines have explicit coverage.

manually tested a migrated case:

$ agentcore  identity api-key-credential-provider create --name x --region us-west-2
Error: specify exactly one of --api-key, --api-key-secret-reference

@github-actions github-actions Bot added the size/m PR size: M label Sep 9, 2026
@agentcore-devx-automation agentcore-devx-automation Bot added agentcore-harness-reviewing AgentCore Harness review in progress claude-security-reviewing Claude Code /security-review in progress labels Sep 9, 2026
@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

Claude Security Review: no high-confidence findings. (run)

@agentcore-devx-automation agentcore-devx-automation Bot removed the claude-security-reviewing Claude Code /security-review in progress label Sep 9, 2026

@agentcore-devx-automation agentcore-devx-automation Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 assertMutuallyExclusiveInputs were manually converting false booleans to undefined (e.g., flags["clear-description"] || undefined). The new helper treats both undefined and false as "not set", so the behavior matches without callers needing to normalize.
  • The exactlyOne: true option correctly replaces the previous "both required OR both forbidden" branches (e.g., in online-eval/create, online-insight/create, api-key/oauth2 create+update, sessionSource, harness export, payment-connector, gateway-target add). 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/hasDataSource left behind after the removed branches are still used downstream — no dead code.
  • Because the exactlyOne branch fires whenever definedFlags.length !== 1, the "mutually exclusive" branch is unreachable when exactlyOne is set. That's the intended design (produces the specify exactly one message in both the 0-set and >1-set cases), and the unit tests in utils.test.tsx pin 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.

@agentcore-devx-automation agentcore-devx-automation Bot removed the agentcore-harness-reviewing AgentCore Harness review in progress label Sep 9, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.05%. Comparing base (4fe2c10) to head (8edd7cb).
⚠️ Report is 1 commits behind head on refactor.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Hweinstock
Hweinstock marked this pull request as ready for review September 10, 2026 16:50
@github-actions github-actions Bot added size/m PR size: M and removed size/m PR size: M labels Sep 10, 2026

@nborges-aws nborges-aws left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM clean PR

@Hweinstock
Hweinstock merged commit 551ab96 into aws:refactor Sep 10, 2026
22 of 26 checks passed
@Hweinstock
Hweinstock deleted the feat/assert-mutually-exclusive-flags branch September 10, 2026 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/m PR size: M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants