hive-forge: let pr status take the PR number positionally

Every other PR-scoped verb takes the number as a positional — `pr show
42`, `pr comments 42`, `pr assign-reviewer 42 argus`. `pr status` alone
required `--pr 42`, so whichever form you learn first is wrong for the
other, and clap's error for the mistake suggests `-- --pr`, which would
pass the literal string on as the next positional.

The number is positional here too now. `--pr` stays, because it was the
only spelling this verb had; `--sha` keeps its flag because the two are
alternatives rather than one required argument.

Two error strings in this file also named `pr-status`, a form that has
refused to run since the subcommand rename. One of them is the message
you get for passing neither argument — i.e. exactly when you are already
unsure what the verb is called.

The four new cases cover both halves: that clap accepts each spelling,
and that something reads it. `target_pr` is named rather than inlined
for exactly that reason — dropping the positional from the selection
passed every parse-only case while leaving `pr status 42` reporting
"pass a PR number". The conflict case earns its keep the same way: clap
accepting an argument and clap ignoring it are indistinguishable from a
passing parse, so `42 --sha abc` and `42 --pr 42` both have to be
rejected.

Closes #4182.

hive-forge/src/verbs/pr_status.rs

@@ -1,12 +1,12 @@
-//! `pr status --pr <n>` — one-stop PR health view: mergeable state, CI
+//! `pr status <n>` — one-stop PR health view: mergeable state, CI
 //! checks, requested reviewers + review verdicts, and the last-comment
 //! timestamp. `--sha <sha>` is a CI-only fast path for a raw commit.
 //! Removes the need for raw `curl` to the statuses / reviews endpoints,
 //! keeping forge access on the single `hive-forge` tool.
 //!
-//! Exit code is a merge-readiness verdict for `--pr`: 0 only when CI is
+//! Exit code is a merge-readiness verdict when given a PR: 0 only when
-//! green AND the PR is mergeable AND no review requests changes — so it
+//! CI is green AND the PR is mergeable AND no review requests changes —
-//! composes (`hive-forge pr status --pr 42 && …`). `--sha` mirrors the
+//! so it composes (`hive-forge pr status 42 && …`). `--sha` mirrors the
 //! CI verdict alone (0 = success).
 
 use anyhow::{Context, Result, bail};
@@ -21,26 +21,42 @@ use crate::verbs::{print_json, rfc3339};
 pub struct Args {
     /// PR number — full health view (mergeable, CI, reviews, last
     /// comment). Mutually exclusive with `--sha`.
+    #[arg(value_name = "NUMBER", conflicts_with_all = ["pr", "sha"])]
+    number: Option<u64>,
+    /// Same as passing the number positionally. Kept because this verb
+    /// accepted only this spelling before the positional existed.
     #[arg(long, conflicts_with = "sha")]
     pr: Option<u64>,
     /// Explicit commit sha (or ref) — CI-only fast path. Mutually
-    /// exclusive with `--pr`.
+    /// exclusive with a PR number.
     #[arg(long)]
     sha: Option<String>,
 }
 
+impl Args {
+    /// The PR to inspect, whichever spelling asked for it.
+    ///
+    /// Named rather than inlined so the two spellings can be shown to
+    /// select the same PR: a test that only parses proves clap accepted
+    /// the positional, never that anything reads it.
+    fn target_pr(&self) -> Option<u64> {
+        self.number.or(self.pr)
+    }
+}
+
 /// # Errors
 ///
-/// Returns an error if neither `--pr` nor `--sha` is given, or any forge
+/// Returns an error if neither a PR number nor `--sha` is given, or any
-/// GET fails (PR lookup, combined status, reviews, comments). A
+/// forge GET fails (PR lookup, combined status, reviews, comments). A
 /// not-ready verdict is NOT an error — it's reported and reflected in
 /// the process exit code instead.
 pub fn run(client: &Client, args: Args) -> Result<()> {
     let repo = client.repo()?;
-    match (args.pr, args.sha) {
+    let target = args.target_pr();
+    match (target, args.sha) {
         (Some(pr), _) => pr_status(client, repo, pr),
         (None, Some(sha)) => sha_status(client, &sha),
-        (None, None) => bail!("pr-status: pass one of --pr <n> or --sha <sha>"),
+        (None, None) => bail!("pr status: pass a PR number (`pr status 42`) or --sha <sha>"),
     }
 }
 
@@ -104,7 +120,7 @@ fn pr_status(client: &Client, repo: &str, pr: u64) -> Result<()> {
         .head
         .as_ref()
         .and_then(|h| h.sha.clone())
-        .with_context(|| format!("pr-status: PR #{pr} has no head.sha"))?;
+        .with_context(|| format!("pr status: PR #{pr} has no head.sha"))?;
 
     let requested: Vec<String> = pull
         .requested_reviewers
@@ -389,6 +405,59 @@ fn combined_json(sha: &str, state: &str, statuses: &[Value]) -> Value {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use clap::Parser;
+
+    /// `Args` is a flattened arg group, not a `Parser`, so parsing it
+    /// standalone needs a wrapper. Without one these cases could only be
+    /// written against the whole binary's clap tree.
+    #[derive(Parser)]
+    struct Wrap {
+        #[command(flatten)]
+        args: Args,
+    }
+
+    fn parse(argv: &[&str]) -> Result<Args, clap::Error> {
+        Wrap::try_parse_from(argv).map(|w| w.args)
+    }
+
+    /// The property: the number is accepted the way every other
+    /// `pr <verb>` takes it, and the older flag still works.
+    #[test]
+    fn the_pr_number_is_accepted_positionally_and_as_a_flag() {
+        assert_eq!(parse(&["x", "42"]).unwrap().number, Some(42));
+        assert_eq!(parse(&["x", "--pr", "42"]).unwrap().pr, Some(42));
+    }
+
+    /// And something reads it. Parsing only proves clap accepted the
+    /// positional — dropping it from the selection leaves every other
+    /// case here green while `pr status 42` reports "pass a PR number".
+    #[test]
+    fn both_spellings_select_the_same_pr_and_sha_selects_none() {
+        assert_eq!(parse(&["x", "42"]).unwrap().target_pr(), Some(42));
+        assert_eq!(parse(&["x", "--pr", "42"]).unwrap().target_pr(), Some(42));
+        assert_eq!(parse(&["x", "--sha", "abc"]).unwrap().target_pr(), None);
+    }
+
+    /// Control for the case above. Clap accepting an argument and clap
+    /// ignoring it look identical from a passing parse, so the conflicts
+    /// have to be shown to bite.
+    #[test]
+    fn a_pr_number_conflicts_with_sha_and_with_the_flag() {
+        assert!(parse(&["x", "42", "--sha", "abc"]).is_err());
+        assert!(parse(&["x", "42", "--pr", "42"]).is_err());
+    }
+
+    /// Giving neither is a runtime error from `run`, not a parse error —
+    /// so `--sha` alone has to keep parsing.
+    #[test]
+    fn neither_form_parses_and_sha_alone_still_does() {
+        let empty = parse(&["x"]).unwrap();
+        assert!(empty.number.is_none() && empty.pr.is_none() && empty.sha.is_none());
+        assert_eq!(
+            parse(&["x", "--sha", "abc"]).unwrap().sha.as_deref(),
+            Some("abc")
+        );
+    }
 
     #[test]
     fn short_sha_truncates() {