fix(cli): pair flags by name first, and let a value hint reach fig

Three from CodeRabbit's review of #1171:

- **A moved spelling could claim a flag whose name still matched.** Pairing was
  one pass, so `--bar` migrating from the flag named `foo` to the flag named
  `bar` let `foo` claim it by spelling first; the old `bar` then matched the
  same flag by name and was compared to it a second time, while its own
  `--baz` was never reported as removed. Pairing is now two passes — every
  name match claims its flag, then what is left is offered to moved spellings —
  so one new flag is compared exactly once.
- **`display_order` was documented as a metadata finding and never compared.**
  It is compared now, for commands, flags and arguments, rather than the
  documentation being trimmed to match: it is a declaration somebody made
  deliberately, and "moves from 10 to declaration order in help" is worth a
  line.
- **`diff`'s two arguments offered no filesystem completion in the generated
  Fig spec.** They are spec files, so they now say so with
  `value_hint = ValueHint::FilePath`. That alone was not enough, and the reason
  was a gap in the Fig generator rather than in the declaration: it inferred
  templates from an argument's *name* (`out_file` gets paths because it
  contains "file"), applied only the root spec's `complete` nodes so a
  `complete` inside a `cmd` block never reached an argument at all, and turned
  a typed completer into a generator whose script was the empty string. A
  typed completer now becomes the template Fig has a name for, and each
  command's completers are applied at its own level. Regenerating changes two
  lines of `cli/assets/fig.ts` and nothing else.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Author: jdx

cli/assets/fig.ts

@@ -191,10 +191,12 @@ const completionSpec: Fig.Spec = {
           name: "old",
           description:
             'The spec as it was, typically the released one, use "-" to read from stdin',
+          template: "filepaths",
         },
         {
           name: "new",
           description: 'The spec as it is now, use "-" to read from stdin',
+          template: "filepaths",
         },
       ],
     },

cli/src/cli/diff.rs

@@ -47,9 +47,11 @@ use usage::spec::unknown_flags::UnknownFlags;
 #[usage(effect = "read", verbatim_doc_comment)]
 pub struct Diff {
     /// The spec as it was, typically the released one, use "-" to read from stdin
+    #[usage(value_hint = usage_rs::ValueHint::FilePath)]
     old: PathBuf,
 
     /// The spec as it is now, use "-" to read from stdin
+    #[usage(value_hint = usage_rs::ValueHint::FilePath)]
     new: PathBuf,
 
     /// Output format
@@ -542,54 +544,95 @@ fn diff_command_props(old: &SpecCommand, new: &SpecCommand, path: &str, c: &mut
     {
         c.metadata("help-changed", path, "help text changed".to_string());
     }
+
+    diff_display_order(old.display_order, new.display_order, path, "command", c);
 }
 
+/// Pair the two flag lists up, then report.
+///
+/// Two passes, because the two rules are not equals. A matching internal name is the
+/// primary pairing and claims its flag first; only then may a flag whose spellings
+/// moved onto a differently-named declaration claim what is left. Interleaving them let
+/// a spelling take a flag whose name still matched some later old flag, which then
+/// paired with it a second time and reported one comparison twice while losing another
+/// flag's removal entirely.
 fn diff_flags(old: &SpecCommand, new: &SpecCommand, path: &str, c: &mut Changes) {
-    // Which new flags an old one has already been paired with, so a rename is not also
-    // reported as an addition.
-    let mut paired: Vec<bool> = vec![false; new.flags.len()];
+    /// What became of one old flag.
+    enum Pairing {
+        /// Paired by internal name, which is what the derive and the KDL both key on.
+        Named(usize),
+        /// Paired by a shared spelling. The name is not something a caller can type, so
+        /// this is a rename rather than a removal — reported, and then compared in full,
+        /// so a spelling that really did disappear is still reported.
+        Renamed(usize),
+        Gone,
+    }
+
+    let mut claimed: Vec<bool> = vec![false; new.flags.len()];
+    let mut pairings: Vec<Pairing> = Vec::with_capacity(old.flags.len());
 
     for was in &old.flags {
-        // By internal name first, which is what the derive and the KDL both key on.
-        if let Some(position) = new.flags.iter().position(|f| f.name == was.name) {
-            paired[position] = true;
-            diff_flag(was, &new.flags[position], path, c);
+        match new
+            .flags
+            .iter()
+            .enumerate()
+            .find(|(position, f)| !claimed[*position] && f.name == was.name)
+            .map(|(position, _)| position)
+        {
+            Some(position) => {
+                claimed[position] = true;
+                pairings.push(Pairing::Named(position));
+            }
+            None => pairings.push(Pairing::Gone),
+        }
+    }
+
+    for (index, was) in old.flags.iter().enumerate() {
+        if !matches!(pairings[index], Pairing::Gone) {
             continue;
         }
-        // Then by spelling: the name is not something a caller can type, so a flag whose
-        // spellings moved onto a differently-named declaration was renamed rather than
-        // removed. Compared in full afterwards, so a spelling that really did disappear
-        // is still reported.
         let spellings = flag_spellings(was);
-        let renamed = new
+        if let Some(position) = new
             .flags
             .iter()
             .enumerate()
             .find(|(position, f)| {
-                !paired[*position] && flag_spellings(f).iter().any(|s| spellings.contains(s))
+                !claimed[*position] && flag_spellings(f).iter().any(|s| spellings.contains(s))
             })
-            .map(|(position, _)| position);
-        match renamed {
-            Some(position) => {
-                paired[position] = true;
-                let now = &new.flags[position];
+            .map(|(position, _)| position)
+        {
+            claimed[position] = true;
+            pairings[index] = Pairing::Renamed(position);
+        }
+    }
+
+    // Reported in the old spec's declaration order, whichever pass did the pairing.
+    for (was, pairing) in old.flags.iter().zip(&pairings) {
+        match pairing {
+            Pairing::Named(position) => diff_flag(was, &new.flags[*position], path, c),
+            Pairing::Renamed(position) => {
+                let now = &new.flags[*position];
                 c.metadata(
                     "flag-renamed",
                     path,
                     format!("flag '{}' was renamed to '{}'", was.name, now.name),
                 );
                 diff_flag(was, now, path, c);
             }
-            _ => c.breaking(
+            Pairing::Gone => c.breaking(
                 "flag-removed",
                 path,
-                format!("flag '{}' ({}) was removed", was.name, spellings.join(", ")),
+                format!(
+                    "flag '{}' ({}) was removed",
+                    was.name,
+                    flag_spellings(was).join(", ")
+                ),
             ),
         }
     }
 
     for (position, now) in new.flags.iter().enumerate() {
-        if paired[position] {
+        if claimed[position] {
             continue;
         }
         let spellings = flag_spellings(now);
@@ -968,6 +1011,8 @@ fn diff_flag(old: &SpecFlag, new: &SpecFlag, path: &str, c: &mut Changes) {
     {
         c.metadata("help-changed", path, format!("{subject} help text changed"));
     }
+
+    diff_display_order(old.display_order, new.display_order, path, &subject, c);
 }
 
 fn diff_args(old: &[SpecArg], new: &[SpecArg], path: &str, c: &mut Changes) {
@@ -1218,6 +1263,8 @@ fn diff_arg(old: &SpecArg, new: &SpecArg, path: &str, subject: &str, c: &mut Cha
     {
         c.metadata("help-changed", path, format!("{subject} help text changed"));
     }
+
+    diff_display_order(old.display_order, new.display_order, path, subject, c);
 }
 
 fn diff_choices(
@@ -1879,6 +1926,34 @@ fn diff_unknown_flags(
     }
 }
 
+/// Where a flag, argument or command sits in help output.
+///
+/// Presentation only — nothing about parsing moves — but it is a declaration somebody
+/// made deliberately, and the documentation lists it among the things a metadata finding
+/// covers, so it is compared rather than promised.
+fn diff_display_order(
+    old: Option<usize>,
+    new: Option<usize>,
+    path: &str,
+    subject: &str,
+    c: &mut Changes,
+) {
+    if old == new {
+        return;
+    }
+    let describe =
+        |order: Option<usize>| order.map_or("declaration order".to_string(), |o| o.to_string());
+    c.metadata(
+        "display-order-changed",
+        path,
+        format!(
+            "{subject} moves from {} to {} in help",
+            describe(old),
+            describe(new)
+        ),
+    );
+}
+
 /// How many values are required. Flags carry this as well as arguments do, and a flag
 /// that needs two values where it needed one rejects a command line that used to work.
 fn diff_var_min(
@@ -2719,4 +2794,63 @@ cmd "add" help="install" {
         assert_eq!(removed.location, "ex install");
         assert_eq!(removed.category, Category::Breaking);
     }
+
+    #[test]
+    fn a_name_match_claims_its_flag_before_a_moved_spelling_can() {
+        // `--bar` moves from the flag named `foo` to the flag named `bar`, whose own
+        // `--baz` goes away. Pairing in one pass let `foo` claim the new `bar` by
+        // spelling, after which the old `bar` matched the same flag by name and was
+        // compared to it a second time — so `--baz` was never reported as removed.
+        let old = r#"
+name "ex"
+bin "ex"
+flag "foo: --bar" help="one"
+flag "bar: --baz" help="two"
+        "#;
+        let new = r#"
+name "ex"
+bin "ex"
+flag "bar: --bar" help="two"
+        "#;
+        let found = changes(old, new);
+        // One comparison per new flag: `bar` pairs with `bar` by name, and `foo` is gone.
+        let removed: Vec<&str> = found
+            .iter()
+            .filter(|c| c.code == "flag-removed")
+            .map(|c| c.message.as_str())
+            .collect();
+        assert_eq!(removed, ["flag 'foo' (--bar) was removed"]);
+        assert_eq!(
+            find(&found, "flag-spelling-removed").message,
+            "flag '--bar' no longer answers to '--baz'"
+        );
+        assert!(!found.iter().any(|c| c.code == "flag-renamed"), "{found:?}");
+    }
+
+    #[test]
+    fn display_order_is_compared_because_the_docs_say_it_is() {
+        let old = r#"
+name "ex"
+bin "ex"
+flag "--verbose" help="verbose" display_order=10
+        "#;
+        let new = r#"
+name "ex"
+bin "ex"
+flag "--verbose" help="verbose" display_order=20
+        "#;
+        let found = changes(old, new);
+        let moved = find(&found, "display-order-changed");
+        assert_eq!(moved.category, Category::Metadata);
+        assert_eq!(
+            moved.message,
+            "flag '--verbose' moves from 10 to 20 in help"
+        );
+        // And dropping it entirely says what it falls back to.
+        let none = "name \"ex\"\nbin \"ex\"\nflag \"--verbose\" help=\"verbose\"\n";
+        assert_eq!(
+            find(&changes(old, none), "display-order-changed").message,
+            "flag '--verbose' moves from 10 to declaration order in help"
+        );
+    }
 }

cli/src/cli/generate/fig.rs

@@ -178,6 +178,19 @@ impl FigGenerator {
     }
 }
 
+/// The Fig template for a portable `complete … type=`, where there is one.
+///
+/// `executable` is filesystem completion in Fig's vocabulary too; the narrower kinds
+/// (`command`, `user`, `host`, `none`) have no template and are left alone rather than
+/// approximated with the wrong one.
+fn template_for_type(type_: &str) -> Option<&'static str> {
+    match type_ {
+        "path" | "file" | "executable" => Some("filepaths"),
+        "dir" => Some("folders"),
+        _ => None,
+    }
+}
+
 impl FigArg {
     fn get_template(name: &str) -> Option<String> {
         name.to_lowercase()
@@ -234,10 +247,25 @@ impl FigArg {
     pub fn update_from_complete(&mut self, spec: SpecComplete) {
         let name = spec.name;
 
+        // A typed completer is a policy Fig has its own name for, so it becomes a
+        // template rather than a generator. `get_template` above guesses from the
+        // argument's name — `out_file` gets paths because it contains "file" — which was
+        // all there was before `type=` existed.
+        if let Some(template) = spec.type_.as_deref().and_then(template_for_type) {
+            self.template = Some(template.to_string());
+            return;
+        }
+
+        // Only a completer with something to run becomes a generator. The rest — `none`,
+        // and the kinds a shell answers for itself — have no command to post-process, and
+        // building one anyway emitted a generator whose script was the empty string.
+        let Some(run) = spec.run else {
+            return;
+        };
         self.generators = self.generators.clone().or_else(|| {
             Some(FigGenerator {
                 type_: GeneratorType::Complete,
-                post_process: spec.run.unwrap_or("".to_string()),
+                post_process: run,
                 template_str: format!("${name}$"),
             })
         })
@@ -305,6 +333,18 @@ impl FigCommand {
         [subcmds, vec![self.clone()]].concat()
     }
 
+    /// This command's own arguments — its positionals and its flags' values — without
+    /// descending into subcommands.
+    ///
+    /// What a `complete` node inside a `cmd` block is about. [`Self::get_args`] gathers
+    /// the whole subtree, which is right for the root spec's completers because those are
+    /// inherited, and wrong for a command's own.
+    pub fn get_own_args(&mut self) -> Vec<&mut FigArg> {
+        let mut own: Vec<&mut FigArg> = self.options.iter_mut().map(|o| o.get_args()).concat();
+        own.extend(self.args.iter_mut());
+        own
+    }
+
     pub fn get_args(&mut self) -> Vec<&mut FigArg> {
         let opt_args = self.options.iter_mut().map(|o| o.get_args()).concat();
         let sub_args = self.subcommands.iter_mut().map(|c| c.get_args()).concat();
@@ -318,6 +358,15 @@ impl FigCommand {
     }
 
     pub fn parse_from_spec(cmd: &SpecCommand) -> Option<Self> {
+        let mut command = Self::parse_declarations(cmd)?;
+        // Each command's own completers, at its own level. Only the root spec's `complete`
+        // nodes used to reach any argument, so a `complete` inside a `cmd` block — which
+        // is where a typed value hint on a subcommand's argument lands — was dropped.
+        Fig::fill_args_complete(command.get_own_args(), cmd.complete.clone());
+        Some(command)
+    }
+
+    fn parse_declarations(cmd: &SpecCommand) -> Option<Self> {
         (!cmd.hide).then(|| Self {
             name: FigCommand::get_names(cmd),
             description: cmd.help.clone(),

cli/usage.usage.kdl

@@ -53,6 +53,8 @@ cmd diff help="Compare two usage specs and report what changed about the interfa
     flag --exit-zero help="Exit 0 even when there are breaking changes"
     arg <OLD> help="The spec as it was, typically the released one, use \"-\" to read from stdin"
     arg <NEW> help="The spec as it is now, use \"-\" to read from stdin"
+    complete old type=path
+    complete new type=path
 }
 cmd exec help="Execute a script, parsing args and exposing them as environment variables" unknown_flags=value {
     alias x

docs/cli/reference/commands.json

@@ -249,7 +249,17 @@
         "name": "diff",
         "aliases": [],
         "hidden_aliases": [],
-        "examples": []
+        "examples": [],
+        "complete": {
+          "old": {
+            "name": "old",
+            "type_": "path"
+          },
+          "new": {
+            "name": "new",
+            "type_": "path"
+          }
+        }
       },
       "exec": {
         "full_cmd": ["exec"],