fix(spur-cli): sort squeue output and honor -S/--sort (#524)

* fix(spur-cli): sort squeue output and honor -S/--sort

squeue returned jobs in controller HashMap order and silently ignored
-S/--sort. Sort client-side (as Slurm does): parse comma-separated sort
fields with +/- direction prefixes, apply Slurm's default P,t,-p (plus a
jobid tiebreak) when -S is absent, and allow -S -i to parse. State sort
follows Slurm's base state order; unknown sort fields are rejected.

* fix(spur-cli): add TIME_LEFT sort comparator and avoid per-compare string clones

squeue -S L was accepted but sorted as a no-op because compare_field had no
'L' arm and resolve_job_field returns "?" for it. Add a time-left comparator
(time_limit - run_time, unlimited last). Also compare the common string specs
(partition/user/name/account/qos) by borrowing the proto fields directly
instead of cloning through resolve_job_field on every comparison.

* fix(spur-cli): hoist squeue sort parse and share state sort-rank via spur-core

Parse the -S/--sort spec before any network I/O so an invalid spec reports
"Invalid sort specification" immediately instead of a downstream connect
failure, and a typo no longer costs a full get_jobs fetch.

Move the job-state sort ordering into JobState::sort_rank() in spur-core next to
code()/display() so squeue no longer keeps a fourth hand-maintained copy of the
state list. The rank stays an explicit match because the enum's declaration
order differs from Slurm's sort order (SUSPENDED after RUNNING); a spur-core
test guards that.

Also from review: extract the single-char sort spec via an explicit match
instead of a side-effecting filter, add disable_help_flag so squeue's -h stays
--noheader and a parser test exercises allow_hyphen_values (-S -i), drop a
duplicated call-site comment, and correct the TIME_LEFT helper doc comment.

* fix(spur-cli): restore squeue --help as long-only flag

disable_help_flag dropped both -h and --help; re-add --help via
ArgAction::Help so -h stays --noheader while --help still prints help.

Author: yansun1996

crates/spur-cli/src/squeue.rs

@@ -9,7 +9,13 @@ use crate::format_engine;
 
 /// View information about jobs in the scheduling queue.
 #[derive(Parser, Debug)]
-#[command(name = "squeue", about = "View the job queue")]
+// -h is squeue's --noheader (Slurm convention), so disable clap's auto -h and
+// re-add --help below as long-only.
+#[command(
+    name = "squeue",
+    about = "View the job queue",
+    disable_help_flag = true
+)]
 pub struct SqueueArgs {
     /// Show only jobs for this user
     #[arg(short = 'u', long)]
@@ -47,8 +53,12 @@ pub struct SqueueArgs {
     #[arg(short = 'h', long)]
     pub noheader: bool,
 
-    /// Sort by field
-    #[arg(short = 'S', long)]
+    /// Print help
+    #[arg(long, action = clap::ArgAction::Help)]
+    pub help: Option<bool>,
+
+    /// Sort by field(s), comma-separated, each optionally prefixed with + (asc) or - (desc)
+    #[arg(short = 'S', long, allow_hyphen_values = true)]
     pub sort: Option<String>,
 
     /// Controller address
@@ -84,6 +94,13 @@ pub async fn main_with_args(args: Vec<String>) -> Result<()> {
         None => default_squeue_states(),
     };
 
+    // Parse the sort spec before any network I/O so an invalid -S surfaces its own error
+    // rather than a downstream connect failure.
+    let sort_keys = match args.sort.as_deref() {
+        Some(s) => parse_sort_arg(s)?,
+        None => default_sort_keys(),
+    };
+
     // Parse job ID filter
     let job_ids = args
         .jobs
@@ -113,7 +130,8 @@ pub async fn main_with_args(args: Vec<String>) -> Result<()> {
         .await
         .context("failed to get jobs")?;
 
-    let jobs = response.into_inner().jobs;
+    let mut jobs = response.into_inner().jobs;
+    sort_jobs(&mut jobs, &sort_keys);
 
     // Print header
     if !args.noheader {
@@ -173,6 +191,132 @@ fn resolve_job_field(job: &spur_proto::proto::JobInfo, spec: char) -> String {
     }
 }
 
+/// One field in a sort spec, with direction.
+#[derive(Debug, Clone, Copy, PartialEq)]
+struct SortKey {
+    spec: char,
+    descending: bool,
+}
+
+/// Slurm's documented default job sort: partition asc, state asc, priority desc.
+/// A trailing jobid asc keeps equal-priority jobs deterministically ordered.
+fn default_sort_keys() -> Vec<SortKey> {
+    vec![
+        SortKey {
+            spec: 'P',
+            descending: false,
+        },
+        SortKey {
+            spec: 't',
+            descending: false,
+        },
+        SortKey {
+            spec: 'p',
+            descending: true,
+        },
+        SortKey {
+            spec: 'i',
+            descending: false,
+        },
+    ]
+}
+
+/// Parse `-S` / `--sort`: comma-separated fields, each optionally prefixed with
+/// `+` (asc, default) or `-` (desc). Reuses the format-spec letters.
+fn parse_sort_arg(s: &str) -> Result<Vec<SortKey>> {
+    let mut keys = Vec::new();
+    for token in s.split(',').map(str::trim).filter(|t| !t.is_empty()) {
+        let (descending, rest) = match token.strip_prefix('-') {
+            Some(r) => (true, r),
+            None => (false, token.strip_prefix('+').unwrap_or(token)),
+        };
+        let mut chars = rest.chars();
+        let spec = match (chars.next(), chars.next()) {
+            (Some(c), None) => c,
+            _ => anyhow::bail!("Invalid sort specification: {token}"),
+        };
+        if format_engine::squeue_header(spec) == "?" {
+            anyhow::bail!("Invalid sort specification: {token}");
+        }
+        keys.push(SortKey { spec, descending });
+    }
+    if keys.is_empty() {
+        anyhow::bail!("Invalid sort specification: (empty)");
+    }
+    Ok(keys)
+}
+
+fn sort_jobs(jobs: &mut [spur_proto::proto::JobInfo], keys: &[SortKey]) {
+    jobs.sort_by(|a, b| {
+        for key in keys {
+            let ord = compare_field(a, b, key.spec);
+            let ord = if key.descending { ord.reverse() } else { ord };
+            if ord != std::cmp::Ordering::Equal {
+                return ord;
+            }
+        }
+        std::cmp::Ordering::Equal
+    });
+}
+
+/// Compare two jobs on a single field. Numeric fields compare numerically;
+/// timestamps by epoch seconds; everything else lexically on the rendered value.
+fn compare_field(
+    a: &spur_proto::proto::JobInfo,
+    b: &spur_proto::proto::JobInfo,
+    spec: char,
+) -> std::cmp::Ordering {
+    match spec {
+        'i' | 'A' => a.job_id.cmp(&b.job_id),
+        'p' => a.priority.cmp(&b.priority),
+        'D' => a.num_nodes.cmp(&b.num_nodes),
+        'C' => a.cpus_per_task.cmp(&b.cpus_per_task),
+        't' | 'T' => state_sort_rank(a.state).cmp(&state_sort_rank(b.state)),
+        'M' => run_time_secs(a).cmp(&run_time_secs(b)),
+        'l' => time_limit_secs(a).cmp(&time_limit_secs(b)),
+        'L' => time_left_secs(a).cmp(&time_left_secs(b)),
+        'S' => ts_secs(a.start_time.as_ref()).cmp(&ts_secs(b.start_time.as_ref())),
+        'V' => ts_secs(a.submit_time.as_ref()).cmp(&ts_secs(b.submit_time.as_ref())),
+        'e' => ts_secs(a.end_time.as_ref()).cmp(&ts_secs(b.end_time.as_ref())),
+        'P' => a.partition.cmp(&b.partition),
+        'u' => a.user.cmp(&b.user),
+        'j' | 'n' => a.name.cmp(&b.name),
+        'a' => a.account.cmp(&b.account),
+        'q' => a.qos.cmp(&b.qos),
+        _ => resolve_job_field(a, spec).cmp(&resolve_job_field(b, spec)),
+    }
+}
+
+fn state_sort_rank(state: i32) -> u8 {
+    spur_core::job::JobState::from_proto_i32(state)
+        .map(|s| s.sort_rank())
+        .unwrap_or(u8::MAX)
+}
+
+fn run_time_secs(job: &spur_proto::proto::JobInfo) -> i64 {
+    job.run_time.as_ref().map(|d| d.seconds).unwrap_or(0)
+}
+
+fn time_limit_secs(job: &spur_proto::proto::JobInfo) -> i64 {
+    job.time_limit
+        .as_ref()
+        .map(|d| d.seconds)
+        .unwrap_or(i64::MAX)
+}
+
+fn ts_secs(ts: Option<&prost_types::Timestamp>) -> i64 {
+    ts.map(|t| t.seconds).unwrap_or(0)
+}
+
+/// Unlimited time limit sorts last, matching `-S L`.
+fn time_left_secs(job: &spur_proto::proto::JobInfo) -> i64 {
+    let limit = time_limit_secs(job);
+    if limit == i64::MAX {
+        return i64::MAX;
+    }
+    limit.saturating_sub(run_time_secs(job))
+}
+
 fn state_code(state: i32) -> String {
     spur_core::job::JobState::from_proto_i32(state)
         .map(|s| s.code().to_string())
@@ -273,6 +417,26 @@ mod tests {
     use super::*;
     use spur_proto::proto::JobState as P;
 
+    #[test]
+    fn sort_flag_accepts_hyphen_prefixed_value() {
+        // -S -i must parse as a descending sort spec, not a flag (allow_hyphen_values).
+        let args = SqueueArgs::try_parse_from(["squeue", "-S", "-i"]).unwrap();
+        assert_eq!(args.sort.as_deref(), Some("-i"));
+    }
+
+    #[test]
+    fn long_help_flag_is_preserved() {
+        // -h is reclaimed for --noheader, but --help must still print help.
+        let err = SqueueArgs::try_parse_from(["squeue", "--help"]).unwrap_err();
+        assert_eq!(err.kind(), clap::error::ErrorKind::DisplayHelp);
+    }
+
+    #[test]
+    fn short_h_is_noheader_not_help() {
+        let args = SqueueArgs::try_parse_from(["squeue", "-h"]).unwrap();
+        assert!(args.noheader);
+    }
+
     #[test]
     fn default_squeue_states_includes_completing() {
         let states = default_squeue_states();
@@ -311,4 +475,181 @@ mod tests {
         assert!(parse_states_arg("").is_err());
         assert!(parse_states_arg("  ,  ").is_err());
     }
+
+    fn job(id: u32, partition: &str, state: P, priority: u32) -> spur_proto::proto::JobInfo {
+        spur_proto::proto::JobInfo {
+            job_id: id,
+            partition: partition.into(),
+            state: state as i32,
+            priority,
+            ..Default::default()
+        }
+    }
+
+    fn ids(jobs: &[spur_proto::proto::JobInfo]) -> Vec<u32> {
+        jobs.iter().map(|j| j.job_id).collect()
+    }
+
+    #[test]
+    fn parse_sort_arg_directions() {
+        let keys = parse_sort_arg("P,-p,+i").unwrap();
+        assert_eq!(keys.len(), 3);
+        assert_eq!(
+            keys[0],
+            SortKey {
+                spec: 'P',
+                descending: false
+            }
+        );
+        assert_eq!(
+            keys[1],
+            SortKey {
+                spec: 'p',
+                descending: true
+            }
+        );
+        assert_eq!(
+            keys[2],
+            SortKey {
+                spec: 'i',
+                descending: false
+            }
+        );
+    }
+
+    #[test]
+    fn parse_sort_arg_rejects_bad_tokens() {
+        assert!(parse_sort_arg("").is_err());
+        assert!(parse_sort_arg("ii").is_err());
+        assert!(parse_sort_arg("-").is_err());
+    }
+
+    #[test]
+    fn sort_ascending_and_descending_jobid() {
+        let mut jobs = vec![
+            job(70, "default", P::JobRunning, 100),
+            job(72, "default", P::JobRunning, 100),
+            job(71, "default", P::JobRunning, 100),
+        ];
+        sort_jobs(&mut jobs, &parse_sort_arg("i").unwrap());
+        assert_eq!(ids(&jobs), vec![70, 71, 72]);
+        sort_jobs(&mut jobs, &parse_sort_arg("-i").unwrap());
+        assert_eq!(ids(&jobs), vec![72, 71, 70]);
+    }
+
+    #[test]
+    fn default_sort_matches_slurm_p_t_negp() {
+        // partition asc, then state asc (PD<R), then priority desc, then jobid asc
+        let mut jobs = vec![
+            job(10, "b", P::JobRunning, 100),
+            job(11, "a", P::JobRunning, 50),
+            job(12, "a", P::JobPending, 200),
+            job(13, "a", P::JobRunning, 50),
+        ];
+        sort_jobs(&mut jobs, &default_sort_keys());
+        assert_eq!(ids(&jobs), vec![12, 11, 13, 10]);
+    }
+
+    #[test]
+    fn sort_by_priority_desc_then_jobid_tiebreak() {
+        let mut jobs = vec![
+            job(70, "default", P::JobPending, 100),
+            job(71, "default", P::JobPending, 300),
+            job(72, "default", P::JobPending, 300),
+        ];
+        sort_jobs(&mut jobs, &parse_sort_arg("-p,i").unwrap());
+        assert_eq!(ids(&jobs), vec![71, 72, 70]);
+    }
+
+    #[test]
+    fn parse_sort_arg_rejects_unknown_spec() {
+        let err = parse_sort_arg("x").unwrap_err();
+        assert!(err.to_string().contains("Invalid sort specification"));
+        assert!(parse_sort_arg("-z").is_err());
+        assert!(parse_sort_arg("i,x").is_err());
+    }
+
+    #[test]
+    fn state_sort_places_suspended_after_running() {
+        let mut jobs = vec![
+            job(1, "default", P::JobFailed, 0),
+            job(2, "default", P::JobSuspended, 0),
+            job(3, "default", P::JobRunning, 0),
+            job(4, "default", P::JobPending, 0),
+        ];
+        sort_jobs(&mut jobs, &parse_sort_arg("t").unwrap());
+        assert_eq!(ids(&jobs), vec![4, 3, 2, 1]);
+    }
+
+    #[test]
+    fn sort_by_timelimit_puts_unlimited_last() {
+        let mut a = job(1, "default", P::JobRunning, 0);
+        a.time_limit = Some(prost_types::Duration {
+            seconds: 600,
+            nanos: 0,
+        });
+        let b = job(2, "default", P::JobRunning, 0);
+        let mut c = job(3, "default", P::JobRunning, 0);
+        c.time_limit = Some(prost_types::Duration {
+            seconds: 60,
+            nanos: 0,
+        });
+        let mut jobs = vec![a, b, c];
+        sort_jobs(&mut jobs, &parse_sort_arg("l").unwrap());
+        assert_eq!(ids(&jobs), vec![3, 1, 2]);
+    }
+
+    #[test]
+    fn sort_by_submit_time_ascending() {
+        let mut a = job(1, "default", P::JobPending, 0);
+        a.submit_time = Some(prost_types::Timestamp {
+            seconds: 300,
+            nanos: 0,
+        });
+        let mut b = job(2, "default", P::JobPending, 0);
+        b.submit_time = Some(prost_types::Timestamp {
+            seconds: 100,
+            nanos: 0,
+        });
+        let mut jobs = vec![a, b];
+        sort_jobs(&mut jobs, &parse_sort_arg("V").unwrap());
+        assert_eq!(ids(&jobs), vec![2, 1]);
+    }
+
+    #[test]
+    fn sort_by_partition_orders_lexically() {
+        let mut jobs = vec![
+            job(1, "gamma", P::JobRunning, 0),
+            job(2, "alpha", P::JobRunning, 0),
+            job(3, "beta", P::JobRunning, 0),
+        ];
+        sort_jobs(&mut jobs, &parse_sort_arg("P").unwrap());
+        assert_eq!(ids(&jobs), vec![2, 3, 1]);
+    }
+
+    #[test]
+    fn sort_by_time_left_puts_unlimited_last() {
+        let mut a = job(1, "default", P::JobRunning, 0);
+        a.time_limit = Some(prost_types::Duration {
+            seconds: 600,
+            nanos: 0,
+        });
+        a.run_time = Some(prost_types::Duration {
+            seconds: 60,
+            nanos: 0,
+        });
+        let b = job(2, "default", P::JobRunning, 0);
+        let mut c = job(3, "default", P::JobRunning, 0);
+        c.time_limit = Some(prost_types::Duration {
+            seconds: 600,
+            nanos: 0,
+        });
+        c.run_time = Some(prost_types::Duration {
+            seconds: 500,
+            nanos: 0,
+        });
+        let mut jobs = vec![a, b, c];
+        sort_jobs(&mut jobs, &parse_sort_arg("L").unwrap());
+        assert_eq!(ids(&jobs), vec![3, 1, 2]);
+    }
 }