Validate conflicting short flags in command signatures (#400) - #401
Validate conflicting short flags in command signatures (#400)#401warp-factories[bot] wants to merge 4 commits into
Conversation
|
This PR was generated with Warp. Comment |
|
Spec approved. This specification covers issue #400, and the spec stage is now complete. Implementation remains gated: a maintainer can apply Responding as Warp for OSS: Open session · View run |
Adds a shared validator in warp-completion-metadata that detects two or more distinct option entries directly authored on the same command or subcommand node claiming the same normalized short flag. Option names are HTML-unescaped with the same helper used by `From<CommandOption> for Opt` before classification, so encoded and literal spellings (e.g. `-h` and `-h`) are recognized as the same flag; diagnostics show the normalized flag alongside the raw authored name arrays. A repository invariant test in warp-command-signatures scans only the top-level handwritten command-signatures/json/*.json files (excluding autogenerated/ and overrides/) and compares collected conflicts against a temporary, exact baseline allowlist of the 45 pre-existing conflicts across 13 files. The comparison is against complete records -- (file, command path, short flag) plus the ordered claimant list of source indices and raw name arrays -- so adding, removing, reordering, or swapping a claimant fails even when the key itself is unchanged. Regression tests cover both the third-claimant and swapped-claimant cases directly against the comparison function. Fixes #400. Follow-up: #402 tracks reducing the baseline to zero.
diff_against_baseline previously formatted failures with {:?} on the
raw (command_path, claimants) tuples, producing zero-based debug
tuples instead of the one-based, source-actionable diagnostic the
issue and spec require. Reconstruct a ShortFlagConflict from each
record and reuse its describe() formatter for all three failure
modes (new conflict, changed claimant identity, stale baseline
entry), and pin the exact emitted text with a new regression test.
There was a problem hiding this comment.
Overview
Adds a short-flag conflict validator for handwritten command signatures, wired into the existing invariant tests that script/presubmit and CI already run, with a frozen 45-record baseline so the current offenders do not break main while new conflicts fail immediately. Net position: accepted.
Verdict
Verdict: accepted
Checks: build pass (author-run; cargo unavailable in the review sandbox), tests pass (author-run: 183 tests in warp-command-signatures, 12 unit + 13 integration in warp-completion-metadata), CI green with Rust CodeQL still in progress and nothing failing, visual proof n/a
Found: 0 critical, 0 important, 0 suggestions, 0 nits
Two earlier passes found and closed three issues: the baseline originally compared bare keys and so missed a claimant added under an existing conflict; validation originally ran on raw names, missing that From<CommandOption> for Opt unescapes before is_short_hand_flag classifies, so -h and -h would not have collided; and the invariant's own failure output rendered zero-based debug tuples instead of the actionable diagnostic. All three are fixed. All 45 baseline records were independently cross-checked against the handwritten JSON — no missing keys, no stale keys, no claimant mismatches — and namespace isolation for siblings, parent/child, and separate roots was verified.
Responding as Warp for OSS: Open session · View run



Summary
Implements #400: detect duplicate short flags claimed by two or more distinct option entries directly authored on the same handwritten command or subcommand.
warp_completion_metadata::validation::find_short_flag_conflicts, a recursive validator overfig_types::Command(before conversion intoSignature, since conversion loses source option boundaries and copies persistent options into descendants). Each command/subcommand node is an independent namespace, so sibling subcommands and a parent/child may reuse a short flag.html_unescapeused byFrom<CommandOption> for Optbefore being classified by the existing (nowpub(crate))is_short_hand_flag, so an authored-hand a literal-hare recognized as the same flag. Diagnostics show the normalized flag alongside the raw authorednamearrays and one-based positions, e.g.:command-signatures/json/flutter.json: command "flutter assemble": duplicate short flag "-d" is used by options #1 ["-d", "--device-id"] and #3 ["-d", "--define"]warp-command-signatures(command-signatures/src/lib.rs) that scans only the top-level handwrittencommand-signatures/json/*.jsonfiles (excludingautogenerated/andoverrides/) and compares the collected conflicts against a temporary, exact baseline allowlist of the 45 pre-existing conflicts across 13 files.(file, canonical command path, normalized short flag)plus the ordered claimant list of source indices and rawnamearrays — not just the key. Adding a claimant, removing one, reordering options, or swapping a claimant fails even when(file, command path, flag)is unchanged. Two dedicated regression tests exercise the comparison function directly with a synthetic third-claimant case and a synthetic swapped-claimant case.completion-metadata/tests/fixtures/duplicate_short_flags/and is exercised bycompletion-metadata/tests/duplicate_short_flags.rs, covering all the valid/invalid cases from the spec (distinct flags, identical name arrays, three claimants, nested subcommands, internal repetition, HTML-escaped vs. literal spellings, parent/child and sibling reuse, unrelated top-level fixtures, short+long alias on one option, single-dash long names, and multi-alias command namespaces).Does not touch any of the 45 existing offending command specs; cleanup is tracked separately in #402 per the approved spec's rollout plan.
Fixes #400.
Validation
Ran with a locally installed Rust toolchain (this sandbox does not ship one by default) and Node/npm for the prettier check:
script/presubmit— passed (npm format:check,cargo fmt --check,cargo clippy -p warp-command-signatures -p warp-completion-metadata --all-targets --all-features -- -D warnings,cargo test --verbose, all green; 182 tests inwarp-command-signaturesincluding the new invariant/regression tests, 12 + 13 tests inwarp-completion-metadata)cargo build --workspacecargo test --workspacecargo fmt --checkcargo clippy -p warp-command-signatures -p warp-completion-metadata --all-targets --all-features -- -D warningsThe re-derived conflict inventory matches the spec's expected baseline exactly: 45 conflicts across 13 files (brew 2, copilot 27, flutter 2, kubecolor 1, kubectl 1, oc 1, pscale 2, pulumi 1, rsync 1, sfdx 1, stripe 4, yarn 1, zapier 1).