Skip to content

fix(parse): keep inherited global flags when a subcommand re-declares them as non-global#649

Merged
jdx merged 1 commit into
jdx:mainfrom
JamBalaya56562:fix-parse-inherited-global-flags
May 31, 2026
Merged

fix(parse): keep inherited global flags when a subcommand re-declares them as non-global#649
jdx merged 1 commit into
jdx:mainfrom
JamBalaya56562:fix-parse-inherited-global-flags

Conversation

@JamBalaya56562
Copy link
Copy Markdown
Contributor

Summary

Fixes a parse_partial bug where a value-taking global flag placed before a (mounted) subcommand breaks completion/parsing when the subcommand re-declares that same flag as non-global.

This fixes the parser-side root cause referenced by jdx/mise#10069.

Reproduction

mise emits a usage spec where run mounts tasks dynamically, a task can have a choices positional arg, and run re-declares the global -C/--cd flag as non-global. Completing such a task with the global flag in front:

mycli -C /tmp run sample:run <TAB>
→ Error: × Invalid choice for arg profile: -C, expected one of alpha, beta, gamma

Expected: alpha beta gamma.

Root cause

During phase 1 (subcommand scan / global-flag gathering), each descent did:

out.available_flags.retain(|_, f| f.global);
out.available_flags.extend(gather_flags(&subcommand));

When a child subcommand re-declares a parent's global flag (same -C/--cd key) as non-global, extend overwrites the inherited global entry with the non-global one, and the next descent's retain(|_, f| f.global) then drops it entirely.

With the global flag no longer in available_flags, phase 2 no longer recognizes the leftover -C token and feeds it to the task's choices positional arg → validate_choices bails.

Fix

Replace retain + extend with a small merge_subcommand_flags helper that merges with global precedence: a subcommand's non-global re-declaration no longer shadows an inherited global flag. The global flag stays recognized, so phase 2 consumes it as a flag (and records it in as_env(), preserving usage_* env vars for normal execution and for mount scripts) instead of mistaking it for a positional.

Note: the prefix global flag is intentionally left in input so phase 2 re-parses it into out.flags; that is how its value reaches as_env(). The fix is purely about keeping the flag recognized across descents.

Tests

  • Unit (lib/src/parse.rs): test_prefix_global_flag_does_not_pollute_choices builds the post-mount structure directly (root global -C/--cd, run re-declaring it as non-global, mounted sample:run with a choices arg) and asserts: no false "Invalid choice" bail, the inherited global flag survives descent, the flag value still reaches as_env() (usage_cd), valid choices still parse, and genuinely invalid choices are still rejected.
  • Integration (cli/tests/complete_word.rs + examples/mounted-global-flags-choices.sh): a new fixture whose mounted choices vary by usage_cd, so the test also confirms the global flag value propagates through the mount despite the non-global re-declaration.

Downstream note

mise currently has a temporary workaround (promoting the conflicting run/tasks run flags to global=true in its completion spec generation). Once this lands, that workaround becomes a no-op and can be removed; I'll follow up on jdx/mise#10069.

🤖 Generated with Claude Code

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request fixes a parser bug where a value-taking global flag placed before a mounted subcommand is dropped from recognized flags if the subcommand re-declares the same flag as non-global, leading to parsing errors. To resolve this, a new helper function merge_subcommand_flags is introduced to prevent non-global re-declarations from shadowing inherited global flags. The PR also adds comprehensive integration and unit tests to prevent regressions. The reviewer suggested a minor optimization in merge_subcommand_flags to simplify the redundancy check to available.contains_key(&key), as all remaining flags in the map are already guaranteed to be global.

Comment thread lib/src/parse.rs
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 31, 2026

Greptile Summary

This PR fixes a parse_partial bug in the usage crate where a value-taking global flag placed before a mounted subcommand was silently dropped from available_flags when the subcommand re-declared the same flag as non-global. The fix replaces the inline retain + extend pattern with a new merge_subcommand_flags helper that gives precedence to inherited global flags.

  • Core fix (lib/src/parse.rs): merge_subcommand_flags performs the same retain(global) step but skips inserting a subcommand's non-global re-declaration when an inherited global entry for the same key already exists, preventing the inherited global flag from being dropped on the next descent.
  • Unit test (lib/src/parse.rs): hermetic test builds the post-mount spec structure directly and asserts the global flag survives descent, doesn't leak into choices validation, and still reaches as_env().
  • Integration test + fixture (cli/tests/complete_word.rs, examples/mounted-global-flags-choices.sh): end-to-end fixture with choices that vary by usage_cd value, covering short, long, and embedded-value flag forms.

Confidence Score: 5/5

Safe to merge — the change is narrowly scoped to flag-merge logic during subcommand descent and does not alter any existing behaviour when subcommands do not re-declare inherited global flags.

The new merge_subcommand_flags helper is a minimal, well-reasoned replacement for two lines of inline logic. The invariant it relies on ('after retain, every remaining key is a global flag') holds by construction, and the skip condition correctly handles only the non-global re-declaration case. Both descent call sites are updated consistently, unit and integration tests cover the regression scenario end-to-end (including short, long, and embedded-value flag forms), and the fix does not change behaviour for specs that do not have this flag re-declaration pattern.

No files require special attention.

Important Files Changed

Filename Overview
lib/src/parse.rs Introduces merge_subcommand_flags helper that preserves inherited global flags when a subcommand re-declares them as non-global; applied at both subcommand-descent call sites. Logic and invariants look correct.
cli/tests/complete_word.rs Adds integration test covering short, long, and embedded-value global flag forms before a mounted subcommand with choices; follows the same PATH-mutation pattern already used by existing tests.
examples/mounted-global-flags-choices.sh New test fixture shell script that emits different choices based on usage_cd, proving the global flag value propagates to the mount even though run re-declares it as non-global.

Reviews (3): Last reviewed commit: "fix(parse): keep inherited global flags ..." | Re-trigger Greptile

… them as non-global

When descending into a subcommand (including mounted ones), a global flag that
the child re-declared as non-global would shadow and then be dropped from
`available_flags` on the next descent. A global flag placed before the
subcommand (e.g. `-C dir run task`) then stopped being recognized in phase 2 and
was mis-validated against the subcommand's `choices` positional arg, producing
"Invalid choice for arg ...".

Merge subcommand flags with global precedence so inherited global flags survive
the descent and are still consumed as flags (and recorded in `as_env()`). This
fixes the parser-side root cause referenced by jdx/mise#10069.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@JamBalaya56562 JamBalaya56562 force-pushed the fix-parse-inherited-global-flags branch from 1c5311e to 8f11e99 Compare May 31, 2026 01:49
@JamBalaya56562
Copy link
Copy Markdown
Contributor Author

Thanks for the review! Applied the suggestion — since �vailable.retain(|_, f| f.global) runs first and the merged
ew_flags keys are unique, any present key is guaranteed to be an inherited global flag, so the check is now simply �vailable.contains_key(&key) (added a comment noting the invariant). Pushed as a force-update to the branch.

@JamBalaya56562 JamBalaya56562 force-pushed the fix-parse-inherited-global-flags branch from 8f11e99 to 30f983e Compare May 31, 2026 02:49
@codecov
Copy link
Copy Markdown

codecov Bot commented May 31, 2026

Codecov Report

❌ Patch coverage is 98.91304% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 79.58%. Comparing base (3edff27) to head (30f983e).

Files with missing lines Patch % Lines
lib/src/parse.rs 98.91% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #649      +/-   ##
==========================================
+ Coverage   79.33%   79.58%   +0.24%     
==========================================
  Files          49       49              
  Lines        7342     7430      +88     
  Branches     7342     7430      +88     
==========================================
+ Hits         5825     5913      +88     
- Misses       1141     1142       +1     
+ Partials      376      375       -1     

☔ View full report in Codecov by Sentry.
📢 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.

@jdx jdx merged commit 8ca4f7f into jdx:main May 31, 2026
6 checks passed
@JamBalaya56562 JamBalaya56562 deleted the fix-parse-inherited-global-flags branch May 31, 2026 16:26
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.

2 participants