diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e05ad6b..7b900624 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,15 @@ once it reaches a published 0.1.0 release. ## [Unreleased] +### Added + +- `--allow-root` lets mandible run as root; without it, the TUI, `--doctor`, and `--report` refuse before probing anything when effective uid is 0. + +### Fixed + +- `xtask sweep-diff` now reports positional-count gains and losses per tool, separately from flags and subcommands, so a positional turning into a flag (or vice versa) no longer passes as a clean sweep; the field-level report also names each positional added or removed instead of folding it into the flag list. +- The existence-fabrication oracle now attests a contiguous multi-word operand from a synopsis line's own slots (S-154's shape, `mknod`'s `MAJOR MINOR`), instead of only single tokens, so `mknod`, `accessdb`, `gdk-pixbuf-thumbnailer`, `systemd-sysusers` and `systemd-tmpfiles` no longer report a fabricated positional they genuinely document. + ## [0.8.0] - 2026-09-14 ### Changed diff --git a/Cargo.lock b/Cargo.lock index d74e11f0..cfe1e6a7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -917,6 +917,7 @@ dependencies = [ "mandible-core", "mandible-extract", "mandible-tui", + "nix", "ratatui", "rayon", "tempfile", diff --git a/README.md b/README.md index 6e837e8f..c57707be 100644 --- a/README.md +++ b/README.md @@ -253,6 +253,13 @@ contributed, and how much of the tool it understood. It turns "mandible is wrong about tool X" into "the cobra grammar mishandles Y", which is a bug someone can actually fix. +### Running as root + +mandible refuses to start as root, printing the flag that lifts the refusal. +A probe like `fail2ban-client start --help` reaches a daemon socket, and +under root that is an action, not a question. Pass `--allow-root` to proceed +anyway; mandible never asks for or gains privileges on its own. + ## Documentation diff --git a/docs/design.md b/docs/design.md index 8329540b..4cf89d16 100644 --- a/docs/design.md +++ b/docs/design.md @@ -1129,6 +1129,16 @@ allowlist below. `COLUMNS` this policy sets cannot be matched; the scratch prefix is kept short to make that rare. +10. **Refuse to start at all under uid 0.** The binary checks its effective + uid before the TUI, `--doctor`, or `--report` resolves a tool, and + before any probe spawns. Refusal names the flag that lifts it, + `--allow-root`. + + A subcommand probe such as `fail2ban-client start --help` reaches a + daemon socket, and under root that is an action rather than a question. + mandible never asks for or gains privileges itself, so this rule is + about the privilege the user already brought, not one mandible seeks. + **A convention-discovered node (§5.4's `-` children, named by a file on `PATH`) adds no argv shape and no exemption.** It is probed as its own binary's root `--help`, never as a subcommand word, so it needs no @@ -1168,7 +1178,9 @@ Neither was a bad shape; both were a right shape sent to the wrong program. A per-tool list of who may be probed would be §1's forbidden knowledge wearing a safety label, so rule 1a requires evidence instead. -**Implemented in.** `mandible-extract/src/exec/`. +**Implemented in.** `mandible-extract/src/exec/`. Rule 10 is implemented in +`mandible/src/root_guard.rs`, ahead of the exec chokepoint, since it governs +whether mandible starts at all rather than one probe's argv. --- --- diff --git a/docs/instruments.md b/docs/instruments.md index de4d8d4e..99917114 100644 --- a/docs/instruments.md +++ b/docs/instruments.md @@ -30,10 +30,14 @@ own lifecycle rules, never because the tool it names became inconvenient. ## SweepDiff (`xtask sweep-diff`) SweepDiff compares two rendered coverage scoreboards and reports which -tools gained or lost flags, which gained or lost subcommands, and which -changed parse status, without netting gains against losses. A subcommand -gain is named, never scored: it can be a real recovery or an invented row, -and only a human reading the rendered screen can tell which. It answers +tools gained or lost flags, which gained or lost subcommands, which gained +or lost positionals, and which changed parse status, without netting gains +against losses. A subcommand or positional gain is named, never scored: it +can be a real recovery or an invented row, and only a human reading the +rendered screen can tell which. The positional column is read off each +scoreboard's `#fp2` entity ids (S-154), so it cannot be derived from a V1 +`#fp` scoreboard or a pair missing a fingerprint footer entirely, reported +honestly as unmeasured rather than as "no positional change". It answers "which specific tools did this change touch", the question the fleet-wide aggregate cannot answer because a four-tool regression moves it by hundredths of a percent. It diff --git a/docs/shapes.md b/docs/shapes.md index 50a04393..171c5cca 100644 --- a/docs/shapes.md +++ b/docs/shapes.md @@ -3100,7 +3100,9 @@ entry's `tools` field and nothing else. It does not get a new entry. operand per bracket pair, the inner ones optional (`bare_bracket_group_is_flat`, `mandible-extract/src/help_text/sections/ multiword.rs`). The sweep that admitted the rule could not see this, - because sweep-diff compares flags and subcommands and never positionals. + because sweep-diff compared flags and subcommands and never positionals at + the time; it now reports a positional-count gain/loss column and names + each positional added or removed per tool. - fleet: `trailing-bracket-group-multiword-operand` reads 0 tools/0 findings post-fix on a full-`PATH` sweep of 2323 tools, 2026-09-13, and is ratcheted there. `nested-bracket-group-fused-operand` diff --git a/mandible/Cargo.toml b/mandible/Cargo.toml index be0d1ec5..7bc882c2 100644 --- a/mandible/Cargo.toml +++ b/mandible/Cargo.toml @@ -23,6 +23,7 @@ mandible-tui.workspace = true clap.workspace = true clap_complete.workspace = true anyhow.workspace = true +nix = { version = "0.29", default-features = false, features = ["user"] } tracing.workspace = true tracing-subscriber.workspace = true crossterm.workspace = true diff --git a/mandible/src/cli.rs b/mandible/src/cli.rs index d1a2880a..8d283622 100644 --- a/mandible/src/cli.rs +++ b/mandible/src/cli.rs @@ -89,6 +89,12 @@ pub struct Cli { /// Mirrors `xtask audit`'s own `--dir` default. #[arg(long, default_value = "audit")] pub audit_dir: PathBuf, + + /// Proceed when running as root (uid 0) instead of refusing. mandible + /// never asks for or gains privileges itself; this only lifts the + /// refusal on privilege the user already brought. See spec §6 rule 10. + #[arg(long)] + pub allow_root: bool, } impl Cli { diff --git a/mandible/src/main.rs b/mandible/src/main.rs index 37618d53..3a07536a 100644 --- a/mandible/src/main.rs +++ b/mandible/src/main.rs @@ -15,6 +15,7 @@ mod discovery; mod doctor; mod pipeline; mod report; +mod root_guard; mod shell_init; use clap::{CommandFactory, Parser}; @@ -43,6 +44,13 @@ fn main() -> anyhow::Result<()> { return Ok(()); } + // Before any probe spawns (spec §6 rule 10): the TUI, `--doctor`, + // `--report` and `--review` all pass through here first, and nothing + // above this point resolves or runs a tool. + if let Some(refusal) = root_guard::refusal(nix::unistd::Uid::effective(), cli.allow_root) { + anyhow::bail!(refusal); + } + if let Some(seed) = cli.review { if !Sink::Stdout.is_tty() { anyhow::bail!( diff --git a/mandible/src/root_guard.rs b/mandible/src/root_guard.rs new file mode 100644 index 00000000..bd77d50b --- /dev/null +++ b/mandible/src/root_guard.rs @@ -0,0 +1,36 @@ +//! Refuses to run as root before any probe spawns (spec §6 rule 10). +//! Checked once in `main`, before any tool resolves. + +use nix::unistd::Uid; + +/// One-line refusal `main` prints and exits on; names `--allow-root`. +pub const REFUSAL: &str = "mandible refuses to run as root (uid 0); pass --allow-root to proceed."; + +pub fn refusal(uid: Uid, allow_root: bool) -> Option<&'static str> { + if uid.is_root() && !allow_root { + Some(REFUSAL) + } else { + None + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn root_without_the_flag_is_refused() { + assert_eq!(refusal(Uid::from_raw(0), false), Some(REFUSAL)); + } + + #[test] + fn root_with_the_flag_proceeds() { + assert_eq!(refusal(Uid::from_raw(0), true), None); + } + + #[test] + fn a_normal_uid_proceeds_without_the_flag() { + assert_eq!(refusal(Uid::from_raw(1000), false), None); + assert_eq!(refusal(Uid::from_raw(1000), true), None); + } +} diff --git a/mandible/tests/root_refusal.rs b/mandible/tests/root_refusal.rs new file mode 100644 index 00000000..f21e9663 --- /dev/null +++ b/mandible/tests/root_refusal.rs @@ -0,0 +1,76 @@ +//! Spec §6 rule 10: refuses before any probe when effective uid is 0. +//! +//! Proven against a real uid 0 via a user namespace (AGENTS.md's own +//! `unshare --user --map-root-user`), not a shim, per §3.5. Skips cleanly +//! where unprivileged user namespaces are unavailable, since that is an +//! environment limit, not a claim about the code. + +use std::process::Command; + +fn mandible() -> Command { + Command::new(env!("CARGO_BIN_EXE_mandible")) +} + +fn user_namespaces_available() -> bool { + Command::new("unshare") + .args(["--user", "--map-root-user", "true"]) + .status() + .map(|s| s.success()) + .unwrap_or(false) +} + +#[test] +fn root_is_refused_before_any_probe() { + if !user_namespaces_available() { + eprintln!("unshare --user --map-root-user unavailable; skipping"); + return; + } + let out = Command::new("unshare") + .args(["--user", "--map-root-user"]) + .arg(env!("CARGO_BIN_EXE_mandible")) + .args(["--doctor", "git"]) + .output() + .expect("failed to run mandible under unshare"); + + assert!(!out.status.success(), "root must be refused"); + assert_eq!(out.stdout, b"", "no probe output belongs on stdout"); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!( + stderr.contains("--allow-root"), + "the refusal must name the flag: {stderr:?}" + ); +} + +#[test] +fn allow_root_lets_root_proceed() { + if !user_namespaces_available() { + eprintln!("unshare --user --map-root-user unavailable; skipping"); + return; + } + let out = Command::new("unshare") + .args(["--user", "--map-root-user"]) + .arg(env!("CARGO_BIN_EXE_mandible")) + .args(["--allow-root", "--doctor", "git"]) + .output() + .expect("failed to run mandible under unshare"); + + let stderr = String::from_utf8_lossy(&out.stderr); + assert!( + !stderr.contains("--allow-root"), + "the refusal must not fire: {stderr:?}" + ); +} + +/// A non-root uid never sees the refusal, regardless of the flag. +#[test] +fn non_root_never_refused() { + let out = mandible() + .args(["--doctor", "git"]) + .output() + .expect("failed to run mandible"); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!( + !stderr.contains("--allow-root"), + "a non-root run must not print the root refusal: {stderr:?}" + ); +} diff --git a/packaging/mandible.1 b/packaging/mandible.1 index 655948f2..f5517908 100644 --- a/packaging/mandible.1 +++ b/packaging/mandible.1 @@ -155,6 +155,15 @@ eval "$(mandible \-\-shell\-init bash)" .fi .RE .TP +.B \-\-allow\-root +Proceed when running as root (uid 0) instead of refusing. +.B mandible +refuses to start as root before probing anything, and names this flag in +the refusal. +.B mandible +never asks for or gains privileges itself; this only lifts the refusal on +privilege you already brought. +.TP .B \-h\fR,\fB \-\-help Print help and exit. .TP diff --git a/xtask/src/existence.rs b/xtask/src/existence.rs index f1dc0de6..8102268c 100644 --- a/xtask/src/existence.rs +++ b/xtask/src/existence.rs @@ -494,28 +494,51 @@ fn option_list_slot<'a>(slots: &[(&'a str, bool)], has_literal_flag: bool) -> Ha placeholders } +/// Longest contiguous run this module joins into one attested operand name +/// (S-154's shape). Every real run measured so far is two words; four +/// leaves headroom without letting an arbitrary sentence become an alibi. +const MAX_OPERAND_RUN: usize = 4; + /// Every position in `raw` at which a genuine positional operand is -/// attested — the operand half of what [`attested_name_positions`] is for -/// subcommand names. +/// attested — the operand half of [`attested_name_positions`]. /// -/// Two sources: an operand slot of a synopsis line, minus the option-list -/// slot ([`option_list_slot`]); and a line's first token -/// ([`line_start_words`]), an entry in a declared operand block (argparse's -/// `positional arguments:`). The option-list subtraction applies only to -/// the synopsis set, not to `line_start_words`, to avoid an unmeasured -/// false-alarm class. -fn attested_operand_positions<'a>(raw: &'a str, root_name: &str) -> HashSet<&'a str> { - let mut out = HashSet::new(); +/// Three sources: a single operand slot of a synopsis line, minus the +/// option-list slot; a contiguous run of two or more such slots joined by +/// one space in source order ([`MAX_OPERAND_RUN`], S-154), never spanning +/// the placeholder; and a line's first token ([`line_start_words`]). `Cow` +/// since a joined run is owned but a single slot stays borrowed — the same +/// shape [`attested_name_positions`] already uses. +fn attested_operand_positions<'a>(raw: &'a str, root_name: &str) -> HashSet> { + let mut out: HashSet> = HashSet::new(); for line in synopsis_lines(raw, root_name) { let (slots, has_literal_flag) = usage_operands(&line, root_name); let placeholders = option_list_slot(&slots, has_literal_flag); - for (word, _) in &slots { + let words: Vec<&str> = slots.iter().map(|(word, _)| *word).collect(); + for word in &words { if !placeholders.contains(word) { - out.insert(*word); + out.insert(Cow::Borrowed(*word)); + } + } + // Runs stay within one line's own slot order — no skip, no reorder. + for start in 0..words.len() { + if placeholders.contains(&words[start]) { + continue; + } + let mut run = words[start].to_string(); + for len in 2..=MAX_OPERAND_RUN { + let Some(end) = start.checked_add(len - 1) else { + break; + }; + if end >= words.len() || placeholders.contains(&words[end]) { + break; + } + run.push(' '); + run.push_str(words[end]); + out.insert(Cow::Owned(run.clone())); } } } - out.extend(line_start_words(raw)); + out.extend(line_start_words(raw).into_iter().map(Cow::Borrowed)); out } @@ -707,7 +730,7 @@ fn check_flags(node: &CommandNode, path: &str, raw: &str, out: &mut Vec, + operands: &HashSet>, out: &mut Vec, ) { for positional in node.positionals() { @@ -729,7 +752,7 @@ fn walk( path: &str, raw: &str, attested: &HashSet>, - operands: &HashSet<&str>, + operands: &HashSet>, out: &mut Vec, ) { check_flags(node, path, raw, out); @@ -1811,11 +1834,11 @@ mod tests { for (raw, placeholder, operand) in PLACEHOLDER_PAIRS { let attested = attested_operand_positions(raw, ""); assert!( - !attested.contains(placeholder), + !attested.contains(*placeholder), "{placeholder:?} must not be attested by {raw:?}: {attested:?}" ); assert!( - attested.contains(operand), + attested.contains(*operand), "{operand:?} must be attested by {raw:?}: {attested:?}" ); } @@ -2201,4 +2224,57 @@ mod tests { assert!(!attested.contains("is"), "{attested:?}"); assert!(!attested.contains("tool"), "{attested:?}"); } + + // --- H2: a multi-word positional in the synopsis ------------------- + + /// `mknod`'s real shape: `MAJOR MINOR` is one contiguous run inside a + /// flat bracket group, attested whole, and the parser's positional + /// `primary_name()` for it is the run joined by one space (S-154). + #[test] + fn a_genuine_multi_word_operand_is_attested_as_one_run() { + let raw = "Usage: mknod [OPTION]... NAME TYPE [MAJOR MINOR]\n"; + let attested = attested_operand_positions(raw, "mknod"); + assert!(attested.contains("MAJOR MINOR"), "{attested:?}"); + let mut root = help_text_node("mknod"); + root.entities.push(help_text_positional("NAME")); + root.entities.push(help_text_positional("TYPE")); + root.entities.push(help_text_positional("MAJOR MINOR")); + assert_eq!(detect(raw, &root).fabrication_count(), 0); + } + + /// `accessdb`'s real shape: a second, independent two-word operand. + #[test] + fn detect_does_not_flag_accessdbs_two_word_operand() { + let raw = "Usage: accessdb [OPTION...] [MAN DATABASE]\n"; + let mut root = help_text_node("accessdb"); + root.entities.push(help_text_positional("MAN DATABASE")); + assert_eq!(detect(raw, &root).fabrication_count(), 0); + } + + /// A name whose words are real but not adjacent in the synopsis is + /// still fabricated: widening only ever covers a genuine contiguous + /// run, never a reordering or a skip. + #[test] + fn a_non_contiguous_word_pair_is_still_flagged() { + let raw = "Usage: cmd [OPTION]... FIRST [FLAG] SECOND\n"; + let attested = attested_operand_positions(raw, "cmd"); + assert!(!attested.contains("FIRST SECOND"), "{attested:?}"); + let mut root = help_text_node("cmd"); + root.entities.push(help_text_positional("FIRST SECOND")); + assert_eq!(detect(raw, &root).fabrication_count(), 1); + } + + /// A name whose words come from two different synopsis lines is still + /// flagged: a run never crosses a line boundary. + #[test] + fn words_from_two_different_lines_are_still_flagged() { + let raw = "Usage: cmd [OPTION]... FIRST\nUsage: cmd SECOND [OTHER]\n"; + let attested = attested_operand_positions(raw, "cmd"); + assert!(attested.contains("FIRST"), "{attested:?}"); + assert!(attested.contains("SECOND"), "{attested:?}"); + assert!(!attested.contains("FIRST SECOND"), "{attested:?}"); + let mut root = help_text_node("cmd"); + root.entities.push(help_text_positional("FIRST SECOND")); + assert_eq!(detect(raw, &root).fabrication_count(), 1); + } } diff --git a/xtask/src/transition/diff.rs b/xtask/src/transition/diff.rs index db98c1a8..dacd7f08 100644 --- a/xtask/src/transition/diff.rs +++ b/xtask/src/transition/diff.rs @@ -3,8 +3,8 @@ //! ([`field_diff`]) — rendered by [`super::render_text`]/ //! [`super::render_markdown`], never disagreeing about what changed. -use super::fingerprint::{ParsedFingerprint, EMPTY_FINGERPRINT}; -use super::ParsedScoreboard; +use super::fingerprint::{is_positional_id, ParsedFingerprint, EMPTY_FINGERPRINT}; +use super::{FingerprintFormat, ParsedScoreboard}; /// One matched tool's flag-count comparison. Kept as a signed delta /// alongside both raw counts — never reduced to a single "net" number, per @@ -39,6 +39,33 @@ impl SubcommandDelta<'_> { } } +/// One matched tool's positional-count comparison, mirroring [`FlagDelta`] +/// and [`SubcommandDelta`]: gains and losses stay two separate totals, +/// never netted. Unlike [`SubcommandDelta`], no scoreboard column carries +/// this count — it is derived from the `#fp2` entity ids themselves +/// ([`positional_count`]), so it only exists when both scoreboards carry +/// [`FingerprintFormat::V2`] fingerprints (`H1`'s own doc comment on +/// [`diff`]). +pub(super) struct PositionalDelta<'a> { + pub(super) tool: &'a str, + pub(super) before: usize, + pub(super) after: usize, +} + +impl PositionalDelta<'_> { + pub(super) fn delta(&self) -> i64 { + self.after as i64 - self.before as i64 + } +} + +/// Count of `Positional`-kind entity ids in a parsed fingerprint — the +/// per-tool positional count [`diff`] compares, read straight off the +/// `#fp2` entity ids ([`is_positional_id`]) rather than any scoreboard +/// column. +fn positional_count(fp: &ParsedFingerprint) -> usize { + fp.flags.keys().filter(|id| is_positional_id(id)).count() +} + /// `ParsedRow::nodes` counts the root plus every subcommand /// (`count_nodes`'s own doc comment); subtract the root to read a per-tool /// subcommand count off a column every scoreboard already carries. @@ -61,8 +88,20 @@ pub(super) struct StatusTransition<'a> { /// this task exists to close. pub(super) struct FieldDiff<'a> { pub(super) tool: &'a str, + /// Never a `Positional`-kind id — split into [`positionals_added`] + /// instead ([`is_positional_id`]), so a reader sees a positional + /// appearing as a positional, not folded into the flag list. + /// + /// [`positionals_added`]: FieldDiff::positionals_added pub(super) flags_added: Vec<&'a str>, + /// Never a `Positional`-kind id — see [`flags_added`]'s doc comment. + /// + /// [`flags_added`]: FieldDiff::flags_added pub(super) flags_removed: Vec<&'a str>, + /// A positional entity id present on only the "after" side. + pub(super) positionals_added: Vec<&'a str>, + /// A positional entity id present on only the "before" side. + pub(super) positionals_removed: Vec<&'a str>, /// Flags present on both sides whose description's presence or hash /// differs — catches both "text deleted" (`has_description` flips) and /// "text changed to something else" (hash differs, presence unchanged). @@ -88,6 +127,8 @@ impl FieldDiff<'_> { fn is_empty(&self) -> bool { self.flags_added.is_empty() && self.flags_removed.is_empty() + && self.positionals_added.is_empty() + && self.positionals_removed.is_empty() && self.description_changed.is_empty() && self.choices_changed.is_empty() && self.value_name_changed.is_empty() @@ -117,6 +158,24 @@ pub struct Transition<'a> { /// scoreboard format change. pub(super) subcommand_gains: Vec>, pub(super) subcommand_losses: Vec>, + /// Positional-count gains and losses, reported the same way flags and + /// subcommands are: two separate totals, never netted. Derived from + /// `#fp2` entity ids ([`positional_count`]), so only ever populated + /// when [`positional_diff_unmeasured`] is 0 for the tools involved — + /// see [`diff`]'s own doc comment on why a V1 or missing fingerprint + /// makes this dimension unmeasurable rather than "no change." + /// + /// [`positional_diff_unmeasured`]: Transition::positional_diff_unmeasured + pub(super) positional_gains: Vec>, + pub(super) positional_losses: Vec>, + /// Matched, non-near-cap tools whose positional count could not be + /// derived: either scoreboard isn't [`FingerprintFormat::V2`], or this + /// one tool has no `#fp` entry on either side. Mirrors + /// [`field_diff_unmeasured`]'s "not measured" wording — never folded + /// into "no positional change." + /// + /// [`field_diff_unmeasured`]: Transition::field_diff_unmeasured + pub(super) positional_diff_unmeasured: usize, /// Per-tool field-level diffs — only tools with at least one change /// ([`FieldDiff::is_empty`] false), sorted by tool name. Empty (not /// absent) when neither side's scoreboard carries a `#fp` footer at @@ -150,10 +209,99 @@ impl Transition<'_> { && self.flag_losses.is_empty() && self.subcommand_gains.is_empty() && self.subcommand_losses.is_empty() + && self.positional_gains.is_empty() + && self.positional_losses.is_empty() && self.field_diffs.is_empty() } } +/// What [`per_tool_fingerprint_diff`] found for one matched tool — split +/// out of [`diff`]'s own loop body (ratchet: `clippy::too_many_lines`) so +/// the loop stays a sequence of "handle this dimension" calls. +struct PerToolFingerprintResult<'a> { + field_diff: Option>, + field_unmeasured: bool, + positional_delta: Option>, + positional_unmeasured: bool, +} + +/// The `#fp`/`#fp2` half of one matched tool's comparison: field-level +/// diff plus positional-count delta, both read off the same before/after +/// fingerprint entries. +/// +/// Three states, not two: a line absent on *both* sides means comparison +/// is impossible (a genuinely legacy scoreboard pair); absent on *one* +/// side only means "no record for this side," read as empty +/// (`EMPTY_FINGERPRINT`'s own doc comment) — `coverage::fingerprint_lines` +/// used to skip an empty row and fall into the impossible case instead, +/// silently hiding a total flag loss. +fn per_tool_fingerprint_diff<'a>( + tool: &'a str, + before: &'a ParsedScoreboard, + after: &'a ParsedScoreboard, + positional_format_ok: bool, + tier_changed: Option<(&'a str, &'a str)>, + framework_changed: Option<(&'a str, &'a str)>, +) -> PerToolFingerprintResult<'a> { + match (before.fingerprints.get(tool), after.fingerprints.get(tool)) { + (None, None) => { + // Neither side has a `#fp` entry for this tool: field-level and + // positional-count comparison are both impossible, not "nothing + // changed" (`ParsedScoreboard::fingerprints`'s doc comment) — + // there is no entity id to classify by kind either way, + // regardless of `positional_format_ok`. Still surface a + // tier/framework change if one was found from the ordinary + // columns, which every scoreboard shape carries. + let field_diff = + (tier_changed.is_some() || framework_changed.is_some()).then(|| FieldDiff { + tool, + flags_added: Vec::new(), + flags_removed: Vec::new(), + positionals_added: Vec::new(), + positionals_removed: Vec::new(), + description_changed: Vec::new(), + choices_changed: Vec::new(), + value_name_changed: Vec::new(), + subcommands_added: Vec::new(), + subcommands_removed: Vec::new(), + tier_changed, + framework_changed, + }); + PerToolFingerprintResult { + field_unmeasured: field_diff.is_none(), + field_diff, + positional_delta: None, + positional_unmeasured: true, + } + } + (bfp, afp) => { + // At least one side has a real entry — diff it against the + // other side's entry, or against `EMPTY_FINGERPRINT` when the + // other side has none. Covers both the ordinary both-measured + // case and the deletion/mixed-vintage case. + let bfp = bfp.unwrap_or(&EMPTY_FINGERPRINT); + let afp = afp.unwrap_or(&EMPTY_FINGERPRINT); + let fd = field_diff(tool, bfp, afp, tier_changed, framework_changed); + let positional_delta = positional_format_ok + .then(|| { + let (bpos, apos) = (positional_count(bfp), positional_count(afp)); + (bpos != apos).then_some(PositionalDelta { + tool, + before: bpos, + after: apos, + }) + }) + .flatten(); + PerToolFingerprintResult { + field_unmeasured: false, + field_diff: (!fd.is_empty()).then_some(fd), + positional_unmeasured: !positional_format_ok, + positional_delta, + } + } + } +} + /// Compute the transition between two parsed scoreboards. /// /// Tools whose `ms` is [`near_timeout_cap`] on *either* side are excluded @@ -172,9 +320,19 @@ pub fn diff<'a>(before: &'a ParsedScoreboard, after: &'a ParsedScoreboard) -> Tr let mut flag_losses = Vec::new(); let mut subcommand_gains = Vec::new(); let mut subcommand_losses = Vec::new(); + let mut positional_gains = Vec::new(); + let mut positional_losses = Vec::new(); + let mut positional_diff_unmeasured = 0usize; let mut field_diffs = Vec::new(); let mut field_diff_unmeasured = 0usize; + // A positional count is derived from `#fp2` entity ids, which carry no + // `EntityKind` tag on a V1 line at all — H1's requirement 5. Checked + // once for the whole pair, not per tool: the format is a property of + // which xtask wrote the scoreboard, not of any one row. + let positional_format_ok = before.fingerprint_format == Some(FingerprintFormat::V2) + && after.fingerprint_format == Some(FingerprintFormat::V2); + for (tool, after_row) in &after.rows { let Some(before_row) = before.rows.get(tool) else { appeared.push(tool.as_str()); @@ -224,55 +382,28 @@ pub fn diff<'a>(before: &'a ParsedScoreboard, after: &'a ParsedScoreboard) -> Tr let framework_changed = (before_row.framework != after_row.framework) .then_some((before_row.framework.as_str(), after_row.framework.as_str())); - // Three states, not two (the defect this match used to have: - // `coverage::fingerprint_lines` used to skip a row with no flags and - // no subcommands, so a tool that lost every flag produced a line on - // the "before" side and none on the "after" side, and fell into the - // catch-all below — "unmeasured" — instead of reporting the total - // loss it actually was). Now that every row gets a `#fp` line - // unconditionally, a line is absent on *both* sides only for a - // genuinely legacy scoreboard pair; absent on *one* side only means - // "no record for this side," read as empty (`EMPTY_FINGERPRINT`'s - // own doc comment) so the diff still reports the present side's - // flags/subcommands as added or removed rather than staying silent. - match (before.fingerprints.get(tool), after.fingerprints.get(tool)) { - (None, None) => { - // Neither side has a `#fp` entry for this tool — the - // genuine legacy case (this scoreboard pair predates the - // footer entirely, or — vanishingly rarely — this one row's - // line failed to parse on both sides). Field-level - // comparison is impossible, not "nothing changed" - // (`ParsedScoreboard::fingerprints`'s doc comment). Still - // surface a tier/framework change if one was found from the - // ordinary columns, which every scoreboard shape carries. - if tier_changed.is_some() || framework_changed.is_some() { - field_diffs.push(FieldDiff { - tool, - flags_added: Vec::new(), - flags_removed: Vec::new(), - description_changed: Vec::new(), - choices_changed: Vec::new(), - value_name_changed: Vec::new(), - subcommands_added: Vec::new(), - subcommands_removed: Vec::new(), - tier_changed, - framework_changed, - }); - } else { - field_diff_unmeasured += 1; - } - } - (bfp, afp) => { - // At least one side has a real entry — diff it against the - // other side's entry, or against `EMPTY_FINGERPRINT` when - // the other side has none. Covers both the ordinary - // both-measured case and the deletion/mixed-vintage case. - let bfp = bfp.unwrap_or(&EMPTY_FINGERPRINT); - let afp = afp.unwrap_or(&EMPTY_FINGERPRINT); - let fd = field_diff(tool, bfp, afp, tier_changed, framework_changed); - if !fd.is_empty() { - field_diffs.push(fd); - } + let r = per_tool_fingerprint_diff( + tool, + before, + after, + positional_format_ok, + tier_changed, + framework_changed, + ); + if r.field_unmeasured { + field_diff_unmeasured += 1; + } + if let Some(fd) = r.field_diff { + field_diffs.push(fd); + } + if r.positional_unmeasured { + positional_diff_unmeasured += 1; + } + if let Some(d) = r.positional_delta { + if d.delta() > 0 { + positional_gains.push(d); + } else { + positional_losses.push(d); } } } @@ -292,6 +423,8 @@ pub fn diff<'a>(before: &'a ParsedScoreboard, after: &'a ParsedScoreboard) -> Tr flag_gains.sort_by_key(|d| (std::cmp::Reverse(d.delta()), d.tool.to_string())); subcommand_losses.sort_by_key(|d| (d.delta(), d.tool.to_string())); subcommand_gains.sort_by_key(|d| (std::cmp::Reverse(d.delta()), d.tool.to_string())); + positional_losses.sort_by_key(|d| (d.delta(), d.tool.to_string())); + positional_gains.sort_by_key(|d| (std::cmp::Reverse(d.delta()), d.tool.to_string())); status_transitions.sort_by_key(|t| t.tool.to_string()); field_diffs.sort_by_key(|d| d.tool.to_string()); @@ -306,6 +439,9 @@ pub fn diff<'a>(before: &'a ParsedScoreboard, after: &'a ParsedScoreboard) -> Tr flag_losses, subcommand_gains, subcommand_losses, + positional_gains, + positional_losses, + positional_diff_unmeasured, field_diffs, field_diff_unmeasured, } @@ -325,13 +461,29 @@ pub(super) fn field_diff<'a>( ) -> FieldDiff<'a> { let mut flags_added = Vec::new(); let mut flags_removed = Vec::new(); + let mut positionals_added = Vec::new(); + let mut positionals_removed = Vec::new(); let mut description_changed = Vec::new(); let mut choices_changed = Vec::new(); let mut value_name_changed = Vec::new(); + // Description/choices/value_name changes stay on the shared lists + // above rather than getting their own positional-only split: those + // three already mix every `EntityKind` (flag, positional, modifier, + // env-var) together with no per-kind separation at all, so splitting + // only positionals out of them would be inconsistent rather than + // cheap. Only add/remove — H1's requirement 2 — is split, because that + // is the one case an entity lands in the wrong-shaped bucket entirely + // (a positional read as a flag) rather than merely un-labelled. for (id, after_f) in &after.flags { match before.flags.get(id) { - None => flags_added.push(id.as_str()), + None => { + if is_positional_id(id) { + positionals_added.push(id.as_str()); + } else { + flags_added.push(id.as_str()); + } + } Some(before_f) => { if before_f.has_description != after_f.has_description || before_f.description_hash != after_f.description_hash @@ -349,7 +501,11 @@ pub(super) fn field_diff<'a>( } for id in before.flags.keys() { if !after.flags.contains_key(id) { - flags_removed.push(id.as_str()); + if is_positional_id(id) { + positionals_removed.push(id.as_str()); + } else { + flags_removed.push(id.as_str()); + } } } @@ -368,6 +524,8 @@ pub(super) fn field_diff<'a>( flags_added.sort_unstable(); flags_removed.sort_unstable(); + positionals_added.sort_unstable(); + positionals_removed.sort_unstable(); description_changed.sort_unstable(); choices_changed.sort_unstable(); value_name_changed.sort_unstable(); @@ -376,6 +534,8 @@ pub(super) fn field_diff<'a>( tool, flags_added, flags_removed, + positionals_added, + positionals_removed, description_changed, choices_changed, value_name_changed, diff --git a/xtask/src/transition/fingerprint.rs b/xtask/src/transition/fingerprint.rs index 706704dd..b8561a4d 100644 --- a/xtask/src/transition/fingerprint.rs +++ b/xtask/src/transition/fingerprint.rs @@ -164,6 +164,21 @@ const FP_ID_SEP: char = '='; /// [`FP_FIELD_SEP`] above. const FP_ENTRY_SEP: char = ':'; +/// True when a V2 entity id's own `::::` segment reads +/// `Positional` — `mandible_core::EntityKind`'s `{:?}` spelling, the same +/// string `coverage::fingerprint.rs`'s `entity_identity` embeds. The id +/// shape is fixed as `::::` and `` never itself +/// contains `::` (it's dot-joined subcommand names), so the segment +/// between the first and second `::` is always the kind, however many +/// times `::` may appear later inside ``. +/// +/// A V1 id (no kind tag at all) never matches: `split` never finds a +/// second `::`, so `nth(1)` on a V1 id is its whole tail rather than a bare +/// kind name. +pub(super) fn is_positional_id(id: &str) -> bool { + id.split("::").nth(1) == Some("Positional") +} + /// The pre-generalization `#fp` line prefix — flags only, entity ids with /// no `EntityKind` tag. Still read, never written: kept so a scoreboard /// produced by any earlier xtask still loads, tagged diff --git a/xtask/src/transition/mod.rs b/xtask/src/transition/mod.rs index 8dfeaa59..b6f1290e 100644 --- a/xtask/src/transition/mod.rs +++ b/xtask/src/transition/mod.rs @@ -901,6 +901,134 @@ mod tests { ); } + // --- H1: the positional column ------------------------------------- + // + // `build_fingerprint`/`fingerprint_lines` (`coverage::fingerprint`) are + // `pub(super)`, reachable only from `crate::coverage` itself, so these + // tests drive the *parser* half of the real `#fp2` wire format + // (`parse_scoreboard`, on hand-written text in the exact shape + // `fingerprint_lines` emits — the same technique this file's own awk + // and old-format tests above already use) rather than the emitter half. + + /// A positional appearing on the "after" side is a positional-count + /// gain, named as a positional in `positionals_added`, never mixed into + /// `flags_added` (the defect `h1-sweepdiff-before-change.txt` shows on + /// `uniq`: `(root)::Positional::OUTPUT` reported as a flag). + #[test] + fn positional_gained_is_a_named_positional_gain_not_a_flag() { + let row = row_line("uniqx", "ok", 1, 20); + let before_text = format!( + "{}#fp2 uniqx\t\t(root)::Flag::--version=0:-:-:-\n", + sample_text(&[&row]) + ); + let after_text = format!( + "{}#fp2 uniqx\t\t(root)::Flag::--version=0:-:-:-|(root)::Positional::FILE=0:-:-:-\n", + sample_text(&[&row]) + ); + let before = parse_scoreboard(&before_text); + let after = parse_scoreboard(&after_text); + assert_eq!(before.fingerprint_format, Some(FingerprintFormat::V2)); + assert_eq!(after.fingerprint_format, Some(FingerprintFormat::V2)); + + let t = diff(&before, &after); + assert_eq!(t.positional_diff_unmeasured, 0); + assert_eq!(t.positional_gains.len(), 1); + assert_eq!( + (t.positional_gains[0].before, t.positional_gains[0].after), + (0, 1) + ); + assert!(t.positional_losses.is_empty()); + assert_eq!(t.field_diffs.len(), 1); + assert_eq!( + t.field_diffs[0].positionals_added, + vec!["(root)::Positional::FILE"] + ); + assert!( + t.field_diffs[0].flags_added.is_empty(), + "a gained positional must never land in flags_added" + ); + } + + /// The loss direction of the same defect: a positional present only on + /// the "before" side. + #[test] + fn positional_lost_is_a_named_positional_loss_not_a_flag() { + let row = row_line("uniqx", "ok", 1, 20); + let before_text = format!( + "{}#fp2 uniqx\t\t(root)::Positional::FILE=0:-:-:-\n", + sample_text(&[&row]) + ); + let after_text = format!("{}#fp2 uniqx\t\t\n", sample_text(&[&row])); + let before = parse_scoreboard(&before_text); + let after = parse_scoreboard(&after_text); + + let t = diff(&before, &after); + assert_eq!(t.positional_diff_unmeasured, 0); + assert_eq!(t.positional_losses.len(), 1); + assert_eq!( + (t.positional_losses[0].before, t.positional_losses[0].after), + (1, 0) + ); + assert!(t.positional_gains.is_empty()); + assert_eq!( + t.field_diffs[0].positionals_removed, + vec!["(root)::Positional::FILE"] + ); + assert!(t.field_diffs[0].flags_removed.is_empty()); + } + + /// A renamed positional is one loss plus one gain — **never** a net + /// zero: the total count is unchanged (1 before, 1 after), so it must + /// not appear in `positional_gains`/`positional_losses` at all, but the + /// field-level lists must still name both the old and the new spelling. + #[test] + fn positional_renamed_is_one_loss_and_one_gain_never_a_net_zero() { + let row = row_line("uniqx", "ok", 1, 20); + let before_text = format!( + "{}#fp2 uniqx\t\t(root)::Positional::OLDNAME=0:-:-:-\n", + sample_text(&[&row]) + ); + let after_text = format!( + "{}#fp2 uniqx\t\t(root)::Positional::NEWNAME=0:-:-:-\n", + sample_text(&[&row]) + ); + let before = parse_scoreboard(&before_text); + let after = parse_scoreboard(&after_text); + + let t = diff(&before, &after); + assert!( + t.positional_gains.is_empty() && t.positional_losses.is_empty(), + "an unchanged total count must never appear in the gain/loss lists" + ); + assert_eq!(t.field_diffs.len(), 1); + assert_eq!( + t.field_diffs[0].positionals_added, + vec!["(root)::Positional::NEWNAME"] + ); + assert_eq!( + t.field_diffs[0].positionals_removed, + vec!["(root)::Positional::OLDNAME"] + ); + assert!(!t.is_identical(), "a renamed positional is a real change"); + } + + /// H1 requirement 5: a positional count cannot be derived from a V1 + /// `#fp` scoreboard at all (no `EntityKind` tag to read), so it is + /// reported unmeasured, never as "no positional change." + #[test] + fn v1_fingerprint_pair_reports_positionals_unmeasured_not_clean() { + let row = row_line("t", "ok", 1, 10); + let text = format!("{}#fp t\t\t(root)::--flag=0:-:-:-\n", sample_text(&[&row])); + let before = parse_scoreboard(&text); + let after = parse_scoreboard(&text); + assert_eq!(before.fingerprint_format, Some(FingerprintFormat::V1)); + + let t = diff(&before, &after); + assert_eq!(t.positional_diff_unmeasured, 1); + assert!(t.positional_gains.is_empty()); + assert!(t.positional_losses.is_empty()); + } + // The end-to-end render→parse round trip used to live here, driven by a // real `grep --help` probe, and asserted "at least one flag carries a // description" — a fact about the *host's* grep (GNU grep documents its diff --git a/xtask/src/transition/render_markdown.rs b/xtask/src/transition/render_markdown.rs index 2ed46186..0c899ec3 100644 --- a/xtask/src/transition/render_markdown.rs +++ b/xtask/src/transition/render_markdown.rs @@ -27,6 +27,8 @@ pub fn render_markdown(t: &Transition) -> String { out.push_str(&md_flag_gains_section(t)); out.push_str(&md_subcommand_losses_section(t)); out.push_str(&md_subcommand_gains_section(t)); + out.push_str(&md_positional_losses_section(t)); + out.push_str(&md_positional_gains_section(t)); out.push_str(&md_field_level_section(t)); out.push_str(&md_appeared_disappeared_section(t)); out.push_str(&md_near_cap_section(t)); @@ -264,6 +266,88 @@ fn md_subcommand_gains_section(t: &Transition) -> String { out } +/// The "### Positional-count losses" section — mirrors +/// [`md_subcommand_losses_section`] over the positional-count dimension +/// (H1: derived from `#fp2` entity ids, never a scoreboard column). +fn md_positional_losses_section(t: &Transition) -> String { + let mut out = String::new(); + let total_lost: i64 = t.positional_losses.iter().map(|d| -d.delta()).sum(); + out.push_str("### Positional-count losses (never netted against gains)\n\n"); + if t.positional_losses.is_empty() { + out.push_str("No matched tool lost positionals.\n\n"); + } else { + out.push_str(&format!( + "**{total_lost} positional(s) lost across {n} tool(s).** A gain elsewhere never \ + offsets this.\n\n", + n = t.positional_losses.len(), + )); + out.push_str("| tool | before | after | lost |\n|---|---|---|---|\n"); + for d in t.positional_losses.iter().take(TABLE_ROW_LIMIT) { + out.push_str(&format!( + "| {} | {} | {} | {} |\n", + escape_md(d.tool), + d.before, + d.after, + -d.delta(), + )); + } + if t.positional_losses.len() > TABLE_ROW_LIMIT { + out.push_str(&format!( + "\n_{} more not shown._\n", + t.positional_losses.len() - TABLE_ROW_LIMIT + )); + } + out.push('\n'); + } + if t.positional_diff_unmeasured > 0 { + out.push_str(&format!( + "> [!NOTE]\n> {} matched tool(s) could not have their positional count compared — \ + needs a `#fp2` fingerprint on both sides. A V1 entity id carries no `EntityKind` \ + tag, so it cannot be classified as a positional at all.\n\n", + t.positional_diff_unmeasured, + )); + } + out +} + +/// The "### Positional-count gains" section — mirrors +/// [`md_subcommand_gains_section`]. A gain is named, not scored: only a +/// human reading the rendered screen can tell a real recovery from an +/// invented positional. +fn md_positional_gains_section(t: &Transition) -> String { + let mut out = String::new(); + let total_gained: i64 = t.positional_gains.iter().map(|d| d.delta()).sum(); + out.push_str("### Positional-count gains\n\n"); + if t.positional_gains.is_empty() { + out.push_str("No matched tool gained positionals.\n\n"); + } else { + out.push_str(&format!( + "**{total_gained} positional(s) gained across {n} tool(s).** A gain is not \ + automatically a fix: verify each named tool against its rendered screen before \ + trusting the count.\n\n", + n = t.positional_gains.len(), + )); + out.push_str("| tool | before | after | gained |\n|---|---|---|---|\n"); + for d in t.positional_gains.iter().take(TABLE_ROW_LIMIT) { + out.push_str(&format!( + "| {} | {} | {} | {} |\n", + escape_md(d.tool), + d.before, + d.after, + d.delta(), + )); + } + if t.positional_gains.len() > TABLE_ROW_LIMIT { + out.push_str(&format!( + "\n_{} more not shown._\n", + t.positional_gains.len() - TABLE_ROW_LIMIT + )); + } + out.push('\n'); + } + out +} + /// The "### Field-level changes" section — split out of [`render_markdown`] /// (ratchet: `clippy::too_many_lines`/`clippy::cognitive_complexity`). fn md_field_level_section(t: &Transition) -> String { @@ -289,6 +373,18 @@ fn md_field_level_section(t: &Transition) -> String { if !fd.flags_removed.is_empty() { parts.push(format!("flags removed: {}", capped_join(&fd.flags_removed))); } + if !fd.positionals_added.is_empty() { + parts.push(format!( + "positionals added: {}", + capped_join(&fd.positionals_added) + )); + } + if !fd.positionals_removed.is_empty() { + parts.push(format!( + "positionals removed: {}", + capped_join(&fd.positionals_removed) + )); + } if !fd.description_changed.is_empty() { parts.push(format!( "description changed: {}", diff --git a/xtask/src/transition/render_text.rs b/xtask/src/transition/render_text.rs index 0f817408..f7e07ea0 100644 --- a/xtask/src/transition/render_text.rs +++ b/xtask/src/transition/render_text.rs @@ -33,6 +33,8 @@ pub fn render_text(t: &Transition) -> String { out.push_str(&text_flag_gains_section(t)); out.push_str(&text_subcommand_losses_section(t)); out.push_str(&text_subcommand_gains_section(t)); + out.push_str(&text_positional_losses_section(t)); + out.push_str(&text_positional_gains_section(t)); out.push_str(&text_field_level_section(t)); out.push_str(&text_appeared_disappeared_section(t)); out.push_str(&text_near_cap_section(t)); @@ -190,6 +192,59 @@ fn text_subcommand_gains_section(t: &Transition) -> String { out } +/// The `# positional-count losses` section — mirrors +/// [`text_subcommand_losses_section`] over the positional-count dimension +/// (H1: derived from `#fp2` entity ids, never a scoreboard column). +fn text_positional_losses_section(t: &Transition) -> String { + let mut out = String::new(); + let total_lost: i64 = t.positional_losses.iter().map(|d| -d.delta()).sum(); + out.push_str(&format!( + "# positional-count losses (never netted): {total_lost} lost across {} tool(s)\n", + t.positional_losses.len() + )); + for d in &t.positional_losses { + out.push_str(&format!( + " {}: {} -> {} ({})\n", + d.tool, + d.before, + d.after, + d.delta() + )); + } + if t.positional_diff_unmeasured > 0 { + out.push_str(&format!( + "# positional count unavailable for {} matched tool(s) — needs a #fp2 fingerprint on both sides (a V1 entity id carries no EntityKind tag)\n", + t.positional_diff_unmeasured + )); + } + out.push('\n'); + out +} + +/// The `# positional-count gains` section — mirrors +/// [`text_subcommand_gains_section`]. A gain is named, not scored, exactly +/// like a subcommand gain: only a human reading the rendered screen can +/// tell a real recovery from an invented one. +fn text_positional_gains_section(t: &Transition) -> String { + let mut out = String::new(); + let total_gained: i64 = t.positional_gains.iter().map(|d| d.delta()).sum(); + out.push_str(&format!( + "# positional-count gains (verify against the rendered screen, not this count alone): {total_gained} gained across {} tool(s)\n", + t.positional_gains.len() + )); + for d in &t.positional_gains { + out.push_str(&format!( + " {}: {} -> {} (+{})\n", + d.tool, + d.before, + d.after, + d.delta() + )); + } + out.push('\n'); + out +} + /// The `# field-level changes` section — split out of [`render_text`] /// (ratchet: `clippy::too_many_lines`). fn text_field_level_section(t: &Transition) -> String { @@ -206,6 +261,18 @@ fn text_field_level_section(t: &Transition) -> String { if !fd.flags_removed.is_empty() { parts.push(format!("flags removed: {}", fd.flags_removed.join(", "))); } + if !fd.positionals_added.is_empty() { + parts.push(format!( + "positionals added: {}", + fd.positionals_added.join(", ") + )); + } + if !fd.positionals_removed.is_empty() { + parts.push(format!( + "positionals removed: {}", + fd.positionals_removed.join(", ") + )); + } if !fd.description_changed.is_empty() { parts.push(format!( "description changed: {}",