Skip to content

fix(parse): don't split unknown --key=value into two positional args#628

Open
srnnkls wants to merge 1 commit into
jdx:mainfrom
srnnkls:fix-long-flag-eq-duplicates-positional
Open

fix(parse): don't split unknown --key=value into two positional args#628
srnnkls wants to merge 1 commit into
jdx:mainfrom
srnnkls:fix-long-flag-eq-duplicates-positional

Conversation

@srnnkls
Copy link
Copy Markdown

@srnnkls srnnkls commented May 10, 2026

Summary

When --key=value is parsed against a spec that doesn't declare --key as a flag (e.g. a passthrough wrapper using only arg "[args]..." var=#true), the value is duplicated: both --key=value and value end up in the variadic.

Minimal repro (mise task; same shape against the parser directly):

[tasks.minimal]
usage = '''
arg "[args]..." var=#true
'''
run = 'echo "RAW=<$usage_args>"'
$ mise run minimal -- --depth=3
RAW=<'--depth=3' 3>

Cause

lib/src/parse.rs:454-459 (pre-fix):

if enable_flags && w.starts_with("--") {
    grouped_flag = false;
    let (word, val) = w.split_once('=').unwrap_or_else(|| (&w, ""));
    if !val.is_empty() {
        input.push_front(val.to_string());   // pushed before checking
    }
    if let Some(f) = out.available_flags.get(word) {
        ...
        continue;
    }
    // falls through to positional parsing if --key isn't known
}

For --depth=3 against a spec that only declares [args]...:

  1. Split into word=--depth, val=3.
  2. 3 is unconditionally pushed to the front of the input queue.
  3. --depth is not in available_flags — the continue doesn't fire.
  4. The original --depth=3 token falls through and is consumed as a positional.
  5. Next iteration pops 3 — also consumed as a positional.

The short-flag branch already gets this right: it only pushes the grouped-remainder back into the queue inside the if let Some(f) = ... block (line 489).

Fix

Move the push_front inside the lookup block, and only push when the flag actually takes a value (f.arg.is_some()). Unknown --key=value tokens now pass through to positional parsing as a single token. Mirrors the short-flag pattern.

Test plan

  • New regression test test_arg_var_true_unknown_long_eq_flag_not_split covering the variadic-only case
  • cargo test — all tests pass (177 lib + integration suites)
  • End-to-end mise verification (requires rebuilding mise with patched usage) — not done; happy to do it if a maintainer wants the extra confirmation

When a `--key=value` token is encountered and `--key` is not a declared
flag, the value was pre-pushed to the input queue before the lookup, so
the original `--key=value` token fell through to be captured as a
positional argument and the pushed value was then consumed as a second
positional. For a variadic-only spec, `--depth=3` produced both
`--depth=3` and `3`.

Gate the push on the flag being known and actually taking a value, the
same way short flags already handle their remainder push (line 489).
Unknown `--key=value` and value-less long flags now pass through to
positional parsing as a single token.
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 modifies the parsing logic for long flags containing an equals sign (e.g., --flag=value) to ensure unknown flags are treated as single positional arguments, and includes a regression test for this behavior. Feedback was provided regarding a potential regression where values assigned to known flags that do not accept arguments (such as boolean flags) would be silently ignored; a suggestion was made to move the value-pushing logic to maintain consistency with existing short-flag handling.

Comment thread lib/src/parse.rs
Comment on lines 458 to 462
if f.arg.is_some() {
if !val.is_empty() {
input.push_front(val.to_string());
}
out.flag_awaiting_value.push(Arc::clone(f));
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.

medium

The current implementation moves the push_front logic inside the f.arg.is_some() block. While this correctly fixes the issue for unknown flags, it introduces a change in behavior for known flags that do not take arguments (e.g., boolean flags).

Previously, a value provided via = to a boolean flag (e.g., --verbose=true) would be pushed back and treated as a subsequent positional argument. With this change, that value is silently swallowed.

To maintain consistency with the short-flag parsing logic (lines 488-494), which pushes the remainder regardless of whether the flag takes an argument, consider moving the push_front call outside the f.arg.is_some() check but still within the if let Some(f) block. This ensures that unknown flags are still treated as single positional arguments while preserving existing behavior for known flags.

                if !val.is_empty() {
                    input.push_front(val.to_string());
                }
                if f.arg.is_some() {
                    out.flag_awaiting_value.push(Arc::clone(f));

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 10, 2026

Greptile Summary

This PR fixes a bug where an unknown long flag with an inline value (e.g. --flag=val) was incorrectly split during parsing: the inline value was unconditionally pushed back into the input queue and then consumed as a second positional argument. The fix moves that push_front inside the flag-lookup block, mirroring the existing short-flag pattern.

  • Core fix (lib/src/parse.rs): push_front is now guarded by both the flag-lookup hit and f.arg.is_some(), so unknown long-flag tokens with inline values pass through to positional parsing as a single atomic token.
  • Regression test: constructs a variadic-only spec and asserts that an unknown long flag with an inline value resolves to a single MultiString entry rather than two separate entries.

Confidence Score: 4/5

Safe to merge; the fix is narrowly scoped and the primary bug is well-tested. There is a subtle secondary behavior change for known boolean/count flags that receive an inline value, which is not covered by a test.

The change correctly fixes the split-token bug and the new test validates the targeted scenario. The only concern is that moving push_front inside f.arg.is_some() also silently drops the value portion when a known boolean or count flag is written with an inline assignment — a behavior change on an existing code path that has no test coverage and no inline comment acknowledging the intent.

lib/src/parse.rs — the else if f.count and else branches inside the long-flag lookup block are now affected by the placement of push_front and would benefit from a test or comment clarifying the intended behavior.

Important Files Changed

Filename Overview
lib/src/parse.rs Fixes premature push_front of =value portion for unknown long flags; adds regression test. Subtle behavioral change: =value on known boolean/count flags is now silently dropped instead of becoming the next positional token.

Comments Outside Diff (1)

  1. lib/src/parse.rs, line 458-476 (link)

    P2 Silent drop of =value on known boolean/count flags

    Moving push_front inside the f.arg.is_some() branch is correct for the reported bug, but it also silently discards the value portion when a user writes --bool-flag=foo or --count-flag=foo against a known flag that does not accept a value. Previously that token was re-queued and treated as the next positional; now it disappears without an error. If a caller accidentally writes --verbose=1 and --verbose is a known boolean flag, the 1 will be silently ignored rather than surfaced as a positional argument or a validation error. This is probably fine for most CLI semantics, but it is a behavior change on an existing code path and worth a deliberate test or comment to document the intent.

Reviews (1): Last reviewed commit: "fix(parse): don't split unknown long --k..." | Re-trigger Greptile

@codecov
Copy link
Copy Markdown

codecov Bot commented May 10, 2026

Codecov Report

❌ Patch coverage is 85.71429% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.90%. Comparing base (386df55) to head (e9cb436).

Files with missing lines Patch % Lines
lib/src/parse.rs 85.71% 1 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #628      +/-   ##
==========================================
- Coverage   78.94%   78.90%   -0.04%     
==========================================
  Files          49       49              
  Lines        7284     7309      +25     
  Branches     7284     7309      +25     
==========================================
+ Hits         5750     5767      +17     
- Misses       1147     1148       +1     
- Partials      387      394       +7     

☔ 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.

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