Preserve dashed-literal contract while fail-closing on recognized-option-as-value (#184) - #455
Merged
Merged
Conversation
QueryCommandRunner.ParseArgs previously let a value-taking option consume the next recognized CLI flag (e.g. --json, --lang) as its value, silently dropping the swallowed flag and surfacing a confusing numeric-parse error. Add an IsRecognizedOptionToken guard in TryReadRawOptionValue and TryReadStringOptionValue so the missing value is reported cleanly with "<option> requires a value.". --query is exempted because its design intentionally accepts dashed literals (use --query=<value> to disambiguate if needed). Also warn on repeat occurrences of non-repeatable value-taking options (--db, --limit/--top, --lang, --query, --kind, --since, --start, --end, --before, --after, --focus-line, --focus-column, --focus-length, --snippet-lines, --max-line-width, --depth) so silent take-last overrides become visible. Repeatable --path/--exclude-path/--name are unchanged. Add regression locks for both the missing-value guard and duplicate warnings, and update the three pre-existing missing-value tests whose error message shifted from the downstream "requires a positive integer" to the earlier "requires a value" short-circuit. Closes #184 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…184) Codex iteration 2 flagged that the new IsRecognizedOptionToken guard in TryReadStringOptionValue short-circuited every option before the existing IsRejectedSeparatedStringValue step, which silently broke the pre-existing contract for options that legitimately accept separated dash-prefixed literal values. `cdidx search foo --db --json` used to return "--db requires a value. Hint: if the literal value starts with `--`, pass it as `--db=<value>`." Now it must continue to return that hinted error rather than the plain "requires a value" short-circuit. Change TryReadStringOptionValue to apply the recognized-option guard only when the option does NOT accept separated dash-prefixed literals. For flags with allowSeparatedDashPrefixedLiteralValue=true (`--db`, `--path`, `--exclude-path`, `--query`), the guard is skipped and the downstream IsRejectedSeparatedStringValue flow still emits the inline-form hint for double-dash values. The numeric TryReadRawOptionValue guard is unchanged because numeric options never accept dashed literals. Add regression tests: - --db / --path / --exclude-path followed by --json must fail closed with the inline-form hint. - --query followed by --json must accept --json as literal query text. - --focus-column and --max-line-width must reject zero and non-numeric values with "requires a positive integer" (covers the positive-integer contract that the existing missing-value tests no longer prove after the short-circuit change). Update the English and Japanese CHANGELOG entries to describe the new contract: the guard is unconditional in TryReadRawOptionValue, but in TryReadStringOptionValue it is gated on allowSeparatedDashPrefixedLiteralValue so `--db --json` / `--path --json` / `--exclude-path --json` continue to fail closed with the inline-form hint, and `--query --json` continues to accept `--json` as literal query text. All 2150 tests pass (2 skipped: long-running performance tests).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cdidx search hello --limit --lang rustnow reports "--limit requires a value" cleanly instead of consuming--langas the--limitvalue and failing numeric validation with a confusing "got '--lang'" message). The newIsRecognizedOptionTokenguard fires inTryReadRawOptionValueunconditionally, and inTryReadStringOptionValueonly when the option does NOT accept separated dash-prefixed literal values.--db,--path,--exclude-path, and--query:--db --json/--path --json/--exclude-path --jsonstill fail closed with the inline-form hint (pass it as \--db=`), and--query --jsonstill accepts--jsonas literal query text. Use--query=` to disambiguate if a flag name collides with a literal query.Warning: <option> specified more than once; using the last value '<v>'.on every repeat of a non-repeatable value-taking option so scripted or copy-pasted duplicates are visible instead of silent take-last.--topis canonicalized to--limit, so--limit 5 --top 10also warns. Repeatable options (--path,--exclude-path,--name) are deliberately excluded.search,symbols,impact, …), duplicate-warning behavior for non-repeatable and repeatable options, dashed-literal hint preservation for--db/--path/--exclude-path,--querydashed-literal acceptance, and positive-integer validation for--focus-column/--max-line-width(zero and non-numeric fail closed). Updated bilingual CHANGELOG entries.Closes #184.
Test plan
dotnet build(main project + tests project both clean)dotnet test tests/CodeIndex.Tests/CodeIndex.Tests.csproj— 2150 passed, 2 skipped (long-running perf tests), 0 failedcdidx search hello --limit --lang rust→--limit requires a valuecdidx search foo --db --json→--db requires a value. Hint: if the literal value starts with --, pass it as --db=<value>cdidx find --query --json --path src/** --db <path> --json→ parses--jsonas literal query text (no "requires a value" error)cdidx search foo --limit 5 --limit 10→Warning: --limit specified more than once; using the last value '10'.