Skip to content

fix(parse): dedupe required flag validation errors#685

Merged
jdx merged 1 commit into
mainfrom
codex/dedupe-required-flag-errors
Jun 17, 2026
Merged

fix(parse): dedupe required flag validation errors#685
jdx merged 1 commit into
mainfrom
codex/dedupe-required-flag-errors

Conversation

@jdx

@jdx jdx commented Jun 17, 2026

Copy link
Copy Markdown
Owner

Summary

  • Deduplicate logical flags before required-flag validation
  • Add a regression test for a required flag with both short and long aliases

Root cause

available_flags stores one entry per lookup token, such as -n and --name, and both entries point at the same SpecFlag. Required-flag validation iterated the map values directly, so a missing required flag with multiple aliases emitted the same error once per alias.

Validation

  • cargo fmt --all -- --check
  • cargo test -p usage-lib
  • cargo test --all --all-features

This PR was generated by an AI coding assistant.


Note

Low Risk
Small, localized change to validation error reporting in parse; no change to successful parse behavior or auth/data paths.

Overview
Fixes duplicate Missing required flag errors when a required flag registers both short and long aliases in available_flags (each alias points at the same Arc<SpecFlag>).

Adds unique_flags, which walks flag values once per logical flag via Arc pointer identity, and uses it in the partial-parse required-flag check instead of iterating every map entry.

Adds regression coverage for flag "-n --name <name>" required=#true so a missing value yields a single error message.

Reviewed by Cursor Bugbot for commit 5450e16. Bugbot is set up for automated code reviews on this repo. Configure here.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed redundant validation of required flags with aliases, improving validation efficiency and preventing unnecessary repeated checks.

@coderabbitai

coderabbitai Bot commented Jun 17, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: 0dae6e01-abf3-4d4b-b69d-a395ddb3e097

📥 Commits

Reviewing files that changed from the base of the PR and between d5734f0 and 5450e16.

📒 Files selected for processing (2)
  • lib/src/parse.rs
  • lib/tests/parse.rs

📝 Walkthrough

Walkthrough

lib/src/parse.rs imports HashSet and introduces a unique_flags helper that deduplicates Arc<SpecFlag> values by raw pointer identity. The required-flag validation loop now calls this helper so flags registered under multiple keys (e.g., -n and --name) are validated only once. A new test confirms the correct single error for a required flag with a short alias.

Changes

Required-flag deduplication for aliased flags

Layer / File(s) Summary
unique_flags helper and required-flag validation fix
lib/src/parse.rs, lib/tests/parse.rs
Adds HashSet import and a private unique_flags iterator that filters &Arc<SpecFlag> by pointer identity; the required-flag validation loop now calls unique_flags(out.available_flags.values()) instead of iterating raw values. A new required_flag_with_short_alias test asserts that a flag declared with both -n and --name emits exactly one missing-required error.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐇 A flag with two names, both short and long,
Once checked itself twice — that was clearly wrong!
A HashSet of pointers now guards the gate,
Each Arc seen but once, no duplicate fate.
One missing flag error, tidy and true —
The rabbit hops forward with less déjà vu! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and concisely describes the main change: deduplicating validation errors for required flags by using a helper function to eliminate duplicates before validation.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps

greptile-apps Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a bug where a required flag with multiple aliases (e.g., -n --name) would emit a duplicate "Missing required flag" error once per alias, because available_flags stores one entry per lookup token and the validation loop iterated all values directly.

  • Adds a unique_flags helper that deduplicates Arc<SpecFlag> values by raw pointer identity before validation, correctly handling regular flags, merged/inherited global flags, and negate tokens.
  • Adds a required_flag_with_short_alias regression test that confirms a single error is emitted for a flag declared with both a short and long alias.

Confidence Score: 5/5

Safe to merge — the change is minimal, correctly scoped, and the deduplication logic aligns with how merged/inherited flags are already handled elsewhere in the same file.

The fix is a single well-understood iterator adapter. Pointer identity via Arc::as_ptr is the right key because available_flags always stores clones of the same Arc for each alias of the same logical flag, and the existing merged_cache already relies on the same guarantee. The regression test exercises exactly the broken case and would have caught the original bug.

No files require special attention.

Important Files Changed

Filename Overview
lib/src/parse.rs Adds unique_flags helper using Arc::as_ptr-based deduplication and applies it to the required-flag validation loop to eliminate duplicate errors for flags with multiple aliases.
lib/tests/parse.rs Adds required_flag_with_short_alias regression test verifying that a flag with both -n and --name aliases emits exactly one missing-required-flag error.

Reviews (1): Last reviewed commit: "fix(parse): dedupe required flag validat..." | Re-trigger Greptile

@jdx jdx merged commit 30faca6 into main Jun 17, 2026
6 checks passed
@jdx jdx deleted the codex/dedupe-required-flag-errors branch June 17, 2026 01:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant