From 62dbf329fa975601b2a95e0c3712298a9ff60fc6 Mon Sep 17 00:00:00 2001 From: rldyourmnd Date: Wed, 26 Aug 2026 18:49:40 +0500 Subject: [PATCH 1/3] feat: a surface you can ask, and the rollback the vision named Two things a caller could not do. ` --help` answered `--help has no value`, and every missing argument surfaced singly, so learning `plan-operation` cost seven invocations. One table is now read by both `--help` and the parser, bound by a test that removes each flag it calls required and requires the refusal to name it. And `rollback` existed in the vision and in no code. Installing a new version leaves the old tree in place and moves only the exposed command, so the bytes were always there and nothing pointed back at them. `software --prefix` reads the program directory; `rollback --to --prefix` points the command at one of the versions in it. Both need no network, which is why they are commands you type rather than the three-phase exchange install and update must be. The version is named, never inferred: there is no record of which was previous, only what is on disk, and these strings do not order reliably. Rendered from NDDev-it-com/setup-systems@9180548. Claude-Session: https://claude.ai/code/session_012UZkeU7yQhyZqKAaDaQEjA --- crates/harness-runtime/src/facts.rs | 18 ++ crates/harness-runtime/src/human.rs | 298 ++++++++++++++++++++++++- crates/harness-runtime/src/lib.rs | 39 +++- crates/harness-runtime/src/wire.rs | 7 +- crates/provider-v3/src/argv.rs | 331 +++++++++++++++++++++++++++- crates/setup-core/src/software.rs | 167 ++++++++++++++ 6 files changed, 845 insertions(+), 15 deletions(-) diff --git a/crates/harness-runtime/src/facts.rs b/crates/harness-runtime/src/facts.rs index 4f69ab3..77c78f4 100644 --- a/crates/harness-runtime/src/facts.rs +++ b/crates/harness-runtime/src/facts.rs @@ -136,6 +136,24 @@ impl Harness { }) } + /// Whether this build can put a program on disk from bytes it can verify. + /// + /// Not the same question as "does this harness have a `software` field". + /// Pi's is `Some`, and its delivery is a package manager -- the product is + /// installable, just not by fetching an artifact whose digest was fixed in + /// advance. Offering it `software` and `rollback` would be offering commands + /// that can only refuse. + #[must_use] + pub const fn installs_a_program(&self) -> bool { + matches!( + self.software, + Some(Software { + delivery: Delivery::Artifacts(_), + .. + }) + ) + } + /// The projection this provider owns, which is what its identity is over. /// /// Exactly `native_namespaces`. It is a method rather than a field access so diff --git a/crates/harness-runtime/src/human.rs b/crates/harness-runtime/src/human.rs index d445fb4..8f4a1e7 100644 --- a/crates/harness-runtime/src/human.rs +++ b/crates/harness-runtime/src/human.rs @@ -54,6 +54,18 @@ pub enum Command { /// The setup to apply. setup: String, }, + /// Report which versions of the product are installed, and which is exposed. + Software { + /// The program directory to read. + prefix: PathBuf, + }, + /// Point the exposed command back at a version already on disk. + Rollback { + /// The program directory to change. + prefix: PathBuf, + /// The version to expose. Named, never inferred. + to: Option, + }, /// Re-apply whatever setup is already recorded, repairing drift. Reinstall { /// The directory to repair. @@ -109,6 +121,8 @@ pub fn is_human_command(name: &str) -> bool { | "remove" | "adopt" | "diff" + | "software" + | "rollback" ) } @@ -135,6 +149,8 @@ where struct Arguments { target: Option, backup: Option, + prefix: Option, + to: Option, positional: Vec, } @@ -144,29 +160,46 @@ impl Arguments { let mut parsed = Self { target: None, backup: None, + prefix: None, + to: None, positional: Vec::new(), }; let mut index = 0; while index < rest.len() { let Some(token) = rest.get(index) else { break }; match token.as_str() { - "--target" | "--backup" => { + "--target" | "--backup" | "--prefix" | "--to" => { let Some(value) = rest.get(index + 1) else { return Err(local(format!("{token} has no value"))); }; if value.starts_with("--") { return Err(local(format!("{token} has no value"))); } - if token == "--target" { - if parsed.target.is_some() { - return Err(local("--target was given twice")); + match token.as_str() { + "--target" => { + if parsed.target.is_some() { + return Err(local("--target was given twice")); + } + parsed.target = Some(PathBuf::from(value)); + } + "--prefix" => { + if parsed.prefix.is_some() { + return Err(local("--prefix was given twice")); + } + parsed.prefix = Some(PathBuf::from(value)); + } + "--to" => { + if parsed.to.is_some() { + return Err(local("--to was given twice")); + } + parsed.to = Some(value.clone()); } - parsed.target = Some(PathBuf::from(value)); - } else { - if parsed.backup.is_some() { - return Err(local("--backup was given twice")); + _ => { + if parsed.backup.is_some() { + return Err(local("--backup was given twice")); + } + parsed.backup = Some(value.clone()); } - parsed.backup = Some(value.clone()); } index += 2; } @@ -188,6 +221,12 @@ impl Arguments { .ok_or_else(|| local(format!("{name} requires --target "))) } + fn prefix(&self, name: &str) -> Result { + self.prefix + .clone() + .ok_or_else(|| local(format!("{name} requires --prefix "))) + } + fn setup(&self, name: &str) -> Result { match self.positional.as_slice() { [only] => Ok(only.clone()), @@ -210,11 +249,30 @@ impl Arguments { if self.backup.is_some() && name != "restore" { return Err(local(format!("--backup is not an argument of {name}"))); } + if self.prefix.is_some() && !matches!(name, "software" | "rollback") { + return Err(local(format!("--prefix is not an argument of {name}"))); + } + if self.to.is_some() && name != "rollback" { + return Err(local(format!("--to is not an argument of {name}"))); + } match name { "list" => { self.no_setup(name)?; Ok(Command::List) } + "software" => { + self.no_setup(name)?; + Ok(Command::Software { + prefix: self.prefix(name)?, + }) + } + "rollback" => { + self.no_setup(name)?; + Ok(Command::Rollback { + prefix: self.prefix(name)?, + to: self.to, + }) + } "status" => { self.no_setup(name)?; Ok(Command::Status { @@ -293,7 +351,133 @@ pub fn run(harness: &Harness, command: Command) -> Result<()> { Command::Restore { target, backup } => restore(harness, &target, backup), Command::Adopt { target } => adopt_target(harness, &target), Command::Remove { target } => remove(harness, &target), + Command::Software { prefix } => software(harness, &prefix), + Command::Rollback { prefix, to } => rollback(harness, &prefix, to.as_deref()), + } +} + +/// What the program directory holds, and which version answers to the command. +/// +/// Install and update are not here, and their absence is the design rather than +/// an omission: both need bytes fetched over a network this program never +/// touches, so they are the three-phase exchange the wire already carries. +/// Rollback and this reading need no network at all, which is exactly why they +/// can be commands someone types. +fn software(harness: &Harness, prefix: &Path) -> Result<()> { + let declared = declared_software(harness)?; + let present = setup_core::software::Present::under(prefix, declared.command); + + if present.versions.is_empty() { + println!( + "No version of {} is installed under {}.", + declared.command, + prefix.display() + ); + return Ok(()); + } + + println!("{} under {}:", declared.command, prefix.display()); + println!(); + for version in &present.versions { + let mark = if present.exposed.as_ref() == Some(version) { + "* " + } else { + " " + }; + println!(" {mark}{version}"); + } + println!(); + match &present.exposed { + Some(version) => println!( + " {}/bin/{} runs {version}", + prefix.display(), + declared.command + ), + // Worth saying rather than leaving blank: the versions are on disk and + // nothing answers to the command, which is a different situation from + // having nothing installed. + None => println!( + " Nothing is exposed: {}/bin/{} names no installed version.", + prefix.display(), + declared.command + ), + } + if present.versions.len() > 1 { + println!(); + println!( + " Change it with: rollback --to --prefix {}", + prefix.display() + ); + } + Ok(()) +} + +/// Point the exposed command back at a version that is already on disk. +fn rollback(harness: &Harness, prefix: &Path, to: Option<&str>) -> Result<()> { + let declared = declared_software(harness)?; + let present = setup_core::software::Present::under(prefix, declared.command); + + // Named, never inferred. There is no record of what was previous -- only + // what is on disk -- and these version strings do not order reliably: + // cursor's `2026.08.11-e8db854` sorts by string, not by release. Choosing + // "the one before" would invent an ordering the vendor never promised. + let Some(version) = to else { + return Err(local(if present.versions.is_empty() { + format!( + "rollback requires --to , and {} holds none", + prefix.display() + ) + } else { + format!( + "rollback requires --to ; {} holds {}", + prefix.display(), + present.versions.join(", ") + ) + })); + }; + + if present.exposed.as_deref() == Some(version) { + println!( + "{} already runs {version}; nothing to do.", + declared.command + ); + return Ok(()); + } + + let rolled = setup_core::software::rollback(&declared, prefix, version)?; + println!( + "{} now runs {}.", + rolled.executable.display(), + rolled.version + ); + if let Some(previous) = present.exposed { + println!(" it ran {previous} before this, and that tree is still here"); } + Ok(()) +} + +/// The software this build installs, or the reason it installs none. +/// +/// Two different absences, and they are worth separating. A harness with no +/// `software` at all does not offer the lifecycle. A harness whose delivery is a +/// package manager does -- the product is installable, just not by fetching +/// bytes whose digest was fixed in advance -- and answering "nothing is +/// installed under this prefix" would suggest this build could put something +/// there. It cannot, and pi is the one that would have been told so. +fn declared_software(harness: &Harness) -> Result { + let declared = harness.software.ok_or_else(|| { + local(format!( + "{} configures {} and does not install it", + harness.provider_id, harness.product + )) + })?; + if let setup_core::software::Delivery::Manager { tool, reason } = declared.delivery { + return Err(local(format!( + "{} is delivered by {tool}: {reason}", + declared.command + ))); + } + Ok(declared) } fn local(detail: impl Into) -> Error { @@ -783,6 +967,102 @@ fn short(digest: &str) -> String { mod tests { #![allow(clippy::unwrap_used, clippy::panic)] + /// `rollback` names its version and never infers one. + /// + /// There is no record of what was previous -- only what is on disk -- and + /// these version strings do not order reliably: cursor's + /// `2026.08.11-e8db854` sorts by string, not by release. Choosing "the one + /// before" would invent an ordering the vendor never promised, and pointing + /// a command at the wrong build is the class of mistake this program + /// refuses everywhere else. + #[test] + fn rollback_requires_the_version_it_is_going_to() { + let parsed = parse(["rollback", "--prefix", "/tmp/prefix"]).unwrap(); + assert_eq!( + parsed, + Command::Rollback { + prefix: std::path::PathBuf::from("/tmp/prefix"), + to: None + } + ); + let with = parse(["rollback", "--to", "1.2.2", "--prefix", "/tmp/prefix"]).unwrap(); + assert_eq!( + with, + Command::Rollback { + prefix: std::path::PathBuf::from("/tmp/prefix"), + to: Some("1.2.2".to_owned()) + } + ); + } + + /// The two new flags belong to the two new commands and nowhere else. + #[test] + fn prefix_and_to_are_refused_on_commands_that_do_not_take_them() { + for tokens in [ + vec![ + "install", "baseline", "--target", "/tmp/t", "--prefix", "/tmp/p", + ], + vec!["restore", "--target", "/tmp/t", "--to", "1.2.2"], + vec!["status", "--target", "/tmp/t", "--to", "1.2.2"], + ] { + let error = parse(tokens.clone()).unwrap_err(); + assert!( + error.detail().contains("is not an argument of"), + "{tokens:?} was accepted: {}", + error.detail() + ); + } + } + + /// Two different absences, and the wrong one is misleading. + /// + /// Pi's `software` is `Some` and its delivery is npm, so before this was + /// separated `software --prefix` answered *no version of pi is installed + /// under /tmp/x* -- which reads as an invitation to install one. This build + /// cannot, and says so in the same words `plan-operation` uses. + #[test] + fn a_product_delivered_by_a_package_manager_says_so_rather_than_looking_empty() { + let mut managed = crate::wire::tests_support::TEST; + managed.software = Some(setup_core::software::Software { + version: "1.0.0", + command: "managed", + delivery: setup_core::software::Delivery::Manager { + tool: "npm", + reason: "its closure is resolved at install time", + }, + unsupported: &[], + }); + assert!(!managed.installs_a_program()); + + let error = declared_software(&managed).unwrap_err(); + assert!(error.detail().contains("npm"), "{}", error.detail()); + assert!( + !error.detail().contains("no version"), + "it read as an empty prefix: {}", + error.detail() + ); + + // And a build that installs nothing at all is a third answer again. + let mut none = crate::wire::tests_support::TEST; + none.software = None; + assert!(!none.installs_a_program()); + assert!( + declared_software(&none) + .unwrap_err() + .detail() + .contains("does not install it") + ); + } + + /// Both new commands take a program directory, and neither guesses one. + #[test] + fn software_and_rollback_require_a_prefix() { + for name in ["software", "rollback"] { + let error = parse([name]).unwrap_err(); + assert!(error.detail().contains("--prefix"), "{}", error.detail()); + } + } + use crate::wire::tests_support::TEST; use super::*; diff --git a/crates/harness-runtime/src/lib.rs b/crates/harness-runtime/src/lib.rs index e5991ff..de51fb4 100644 --- a/crates/harness-runtime/src/lib.rs +++ b/crates/harness-runtime/src/lib.rs @@ -85,6 +85,26 @@ pub fn run(harness: &Harness, arguments: Vec) -> ExitCode { }; } + // ` --help` used to answer `--help has no value`, because the flag + // parser reads every `--flag` as taking one. A caller could not ask what a + // command takes, and each missing argument surfaced singly -- seven + // invocations to learn `plan-operation`, measured by a peer who already + // knew the shape. + // + // Read before parsing, and only outside a passthrough: everything after a + // bare `--` belongs to the product `launch` starts, where `--help` means + // something else entirely and is not ours to intercept. + if let Some(command) = provider_v3::vocabulary::Command::parse(first) { + let mine = arguments + .iter() + .take_while(|token| *token != "--") + .any(|token| token == "--help"); + if mine { + print!("{}", provider_v3::argv::render_usage(command)); + return ExitCode::SUCCESS; + } + } + let invocation = match provider_v3::argv::parse(arguments) { Ok(invocation) => invocation, Err(error) => { @@ -182,7 +202,19 @@ fn print_help(harness: &Harness) { if !harness.predecessor_state_file.is_empty() { println!(" adopt --target "); } + if harness.installs_a_program() { + println!(" software --prefix "); + println!(" rollback --to --prefix "); + } println!(); + if harness.installs_a_program() { + println!("`software` reads the program directory and `rollback` points the"); + println!("command back at a version already in it -- installing a new one leaves"); + println!("the old tree in place and moves only the command. Both need no network,"); + println!("which is why they are commands you type; install and update need bytes"); + println!("fetched between planning and applying, so they stay on the wire above."); + println!(); + } if !harness.predecessor_state_file.is_empty() { println!( "`adopt` takes over a target still carrying {},", @@ -192,8 +224,11 @@ fn print_help(harness: &Harness) { println!("type, never something install does behind you, and it deletes nothing."); println!(); } - println!("Every one takes an explicit --target. There is no default: a change"); - println!("aimed at a guessed path is a change aimed at someone else's state."); + println!("Every one takes an explicit --target, and the two program commands an"); + println!("explicit --prefix. There is no default: a change aimed at a guessed"); + println!("path is a change aimed at someone else's state. `rollback` names its"); + println!("version for the same reason -- there is no record of which was"); + println!("previous, only what is on disk."); println!(); println!("A backup is captured before every change, so `restore` always has"); println!("something to return to. Over the wire, install and replace arrive as"); diff --git a/crates/harness-runtime/src/wire.rs b/crates/harness-runtime/src/wire.rs index c3a2666..3c8bb02 100644 --- a/crates/harness-runtime/src/wire.rs +++ b/crates/harness-runtime/src/wire.rs @@ -2415,9 +2415,14 @@ mod tests { #[test] fn launch_without_a_prefix_says_where_a_program_lives() { + // Refused at the argv layer now rather than in dispatch: `--prefix` is + // one of the three arguments `launch` is defined by, so the parser + // names it before a target is ever opened. `launch --help` lists the + // same three, from the same table. let target = seeded("launch-noprefix"); - let error = refuse(args("launch", &target, &[])); + let error = argv::parse(args("launch", &target, &[])).unwrap_err(); assert!(error.detail().contains("--prefix"), "{}", error.detail()); + assert!(error.detail().contains("--help"), "{}", error.detail()); } #[test] diff --git a/crates/provider-v3/src/argv.rs b/crates/provider-v3/src/argv.rs index abfe379..b1b877f 100644 --- a/crates/provider-v3/src/argv.rs +++ b/crates/provider-v3/src/argv.rs @@ -50,6 +50,157 @@ const BUNDLE_FLAGS: &[&str] = &[ "--bundle-size", ]; +/// What one command requires and what it accepts, stated once as data. +/// +/// This table exists because of a measurement, not a preference. A peer +/// building the consumer half spent five round-trips discovering that +/// `plan-operation` takes seven required arguments — and they already knew the +/// shape from their own conformance code. Each missing flag surfaced singly, +/// and `--help` answered `--help has no value`, because it was parsed as a flag +/// that takes one. The one question a caller could ask was met with a complaint +/// about its grammar. +/// +/// The refusals themselves were right, and they are unchanged. What was missing +/// was any way to ask. +/// +/// It is one table read by two callers — [`usage`] renders it and [`parse`] +/// checks against it — because two lists of the same requirement eventually +/// disagree. A test binds it to the parser by removing each named flag from a +/// complete invocation and requiring the refusal to name it. +pub struct Usage { + /// The command this describes. + pub command: Command, + /// Flags without which the command cannot run. + pub required: &'static [&'static str], + /// Flags the command accepts, each with why a caller would pass it. + pub optional: &'static [(&'static str, &'static str)], + /// One line on what the command does with them. + pub note: &'static str, +} + +/// The arguments one command takes. +#[must_use] +pub const fn usage(command: Command) -> Usage { + match command { + Command::ProviderInfo => Usage { + command, + required: &[], + optional: &[], + note: "Report capabilities. Takes no arguments at all, not even --json.", + }, + Command::Status => Usage { + command, + required: &["--target", "--json"], + optional: &[], + note: "Report the target's current state. Never changes it.", + }, + Command::RecoverOperation => Usage { + command, + required: &["--target", "--json"], + optional: &[], + note: "Resolve an interrupted operation. Reads the journal to know what it is resolving.", + }, + Command::ValidateBundle => Usage { + command, + required: &[ + "--target", + "--json", + "--bundle", + "--bundle-format", + "--bundle-digest", + "--artifact-digest", + "--bundle-size", + ], + optional: &[], + note: "Check a bundle against the exact claim that named it. Touches nothing.", + }, + Command::PlanOperation => Usage { + command, + required: &[ + "--target", + "--json", + "--operation", + "--provider-release-digest", + "--operation-id", + "--expires-at", + ], + optional: &[ + ( + "--prefix", + "where a program lives; required by every software_* operation", + ), + ("--backup-ref", "which slot a restore returns to"), + ("--permission-profile", "a profile this build declares"), + ( + "--software-version", + "exactly one pinned version, when not the current one", + ), + ( + "--bundle …", + "the five bundle flags, for install and replace", + ), + ], + note: "Produce a plan. Always pure: reads the target and the local disk, opens no socket.", + }, + Command::ApplyOperation => Usage { + command, + required: &[ + "--target", + "--json", + "--plan", + "--plan-digest", + "--provider-release-digest", + ], + optional: &[ + ( + "--prefix", + "where a program lives; required by every software_* operation", + ), + ( + "--software-artifact", + "one per software_artifacts entry, in the plan's order", + ), + ( + "--bundle …", + "the five bundle flags, for install and replace", + ), + ], + note: "Apply one exact plan under the target lock. --plan is the plan object, \ + written canonically -- not the envelope the planner printed around it.", + }, + Command::Launch => Usage { + command, + required: &["--target", "--json", "--prefix"], + optional: &[( + "-- ", + "everything after a bare -- goes to the product verbatim", + )], + note: "Start the exact executable a software install placed. Never a name found on PATH.", + }, + } +} + +/// Render one command's arguments for a caller who asked. +#[must_use] +pub fn render_usage(command: Command) -> String { + use std::fmt::Write as _; + let shape = usage(command); + let mut out = format!("{command}\n\n {}\n", shape.note); + if !shape.required.is_empty() { + out.push_str("\nRequired:\n"); + for flag in shape.required { + let _ = writeln!(out, " {flag}"); + } + } + if !shape.optional.is_empty() { + out.push_str("\nOptional:\n"); + for (flag, why) in shape.optional { + let _ = writeln!(out, " {flag:<22} {why}"); + } + } + out +} + /// One parsed invocation. #[derive(Debug, Clone, PartialEq, Eq)] pub enum Invocation { @@ -226,6 +377,23 @@ where // leftovers the same way it always would. let (mine, passthrough) = Flags::split_passthrough(rest); let mut flags = Flags::parse(&mine)?; + + // Every missing argument at once, rather than the first one alphabetically. + // Learning a command used to cost one invocation per argument, and the + // count is seven for two of these. + let missing: Vec<&str> = usage(command) + .required + .iter() + .copied() + .filter(|flag| !flags.holds(flag)) + .collect(); + if !missing.is_empty() { + return Err(local(format!( + "{command} is missing {}; run `{command} --help` for what it takes", + missing.join(", ") + ))); + } + let target = PathBuf::from(flags.take_required("--target")?); if !flags.take_switch("--json") { return Err(local(format!("{command} requires --json"))); @@ -359,6 +527,18 @@ impl Flags { Ok(Self { values, switches }) } + /// Whether a flag was given, without consuming it. + /// + /// `--json` is a switch and lives in its own list; asking about it here + /// keeps the completeness check able to name it beside the others rather + /// than leaving one required argument to a separate refusal further down. + fn holds(&self, name: &str) -> bool { + if name == "--json" { + return self.switches.iter().any(|switch| switch == name); + } + self.values.contains_key(name) + } + fn take_required(&mut self, name: &str) -> Result { self.take_optional(name) .ok_or_else(|| local(format!("{name} is required"))) @@ -491,6 +671,117 @@ mod tests { tokens } + /// A complete, well-formed invocation of one command. + fn complete(command: Command) -> Vec { + let mut tokens = vec![command.as_str().to_owned()]; + for flag in usage(command).required { + tokens.push((*flag).to_owned()); + if *flag == "--json" { + continue; + } + tokens.push( + match *flag { + "--target" => "/tmp/target", + "--operation" => "install", + "--bundle" | "--plan" => "/tmp/file", + "--bundle-format" => "ai-stp-bundle/1", + "--bundle-size" => "4096", + "--operation-id" => "operation_00000000000000000000000", + "--expires-at" => "2027-01-01T00:00:00.000Z", + "--prefix" => "/tmp/prefix", + _ => DIGEST, + } + .to_owned(), + ); + } + // `install` arrives as a bundle, so a complete plan for it carries one. + if command == Command::PlanOperation { + tokens.extend(bundle_flags()); + } + tokens + } + + /// The table and the parser must demand the same set. + /// + /// [`usage`] is read by `--help` and by the completeness refusal, and it + /// would be worth nothing if it drifted from what `parse` actually enforces + /// -- a caller would be told the truth about a command that then refused + /// something else. So every flag the table calls required is removed from a + /// complete invocation, one at a time, and the refusal must name it. + #[test] + fn every_flag_the_table_calls_required_is_one_the_parser_demands() { + for command in Command::ALL.iter().copied().filter(|c| c.takes_target()) { + let whole = complete(command); + parse(whole.clone()) + .unwrap_or_else(|e| panic!("{command}: a complete invocation was refused: {e}")); + + for flag in usage(command).required { + let mut without = Vec::new(); + let mut skip = false; + for token in &whole { + if skip { + skip = false; + continue; + } + if token == flag { + skip = *flag != "--json"; + continue; + } + without.push(token.clone()); + } + let error = parse(without) + .err() + .unwrap_or_else(|| panic!("{command} was accepted without {flag}")); + assert!( + error.detail().contains(flag), + "{command} without {flag} refused without naming it: {}", + error.detail() + ); + } + } + } + + /// The count is the point: learning a command used to cost one invocation + /// per argument. + #[test] + fn one_refusal_names_every_missing_argument() { + let error = parse(["plan-operation", "--target", "/tmp/target"]).unwrap_err(); + for flag in [ + "--json", + "--operation", + "--provider-release-digest", + "--operation-id", + "--expires-at", + ] { + assert!( + error.detail().contains(flag), + "the refusal did not name {flag}: {}", + error.detail() + ); + } + assert!( + error.detail().contains("--help"), + "it does not say how to ask" + ); + } + + /// Every command can be asked what it takes, and the answer names the same + /// flags the refusal would. + #[test] + fn every_command_can_be_asked_what_it_takes() { + for command in Command::ALL { + let rendered = render_usage(*command); + assert!(rendered.contains(command.as_str())); + assert!(!usage(*command).note.is_empty()); + for flag in usage(*command).required { + assert!( + rendered.contains(flag), + "{command} help omits its own required {flag}" + ); + } + } + } + #[test] fn provider_info_takes_neither_target_nor_json() { assert_eq!(parse(["provider-info"]).unwrap(), Invocation::ProviderInfo); @@ -634,9 +925,43 @@ mod tests { fn a_partial_bundle_is_refused_rather_than_completed() { let mut partial = bundle_flags(); partial.truncate(partial.len() - 2); // drop --bundle-size and its value - let tokens = [with_target("validate-bundle", &[]), partial].concat(); - let error = parse(tokens).unwrap_err(); - assert!(error.detail().contains("all five")); + + // On `validate-bundle` the bundle *is* the command, so the five flags + // are required and the completeness check names the missing one before + // the bundle reader is reached. Naming the exact flag is the better + // answer of the two, and it is the one a caller gets here. + let error = + parse([with_target("validate-bundle", &[]), partial.clone()].concat()).unwrap_err(); + assert!( + error.detail().contains("--bundle-size"), + "{}", + error.detail() + ); + + // On `plan-operation` a bundle is optional, so the invariant that still + // has to be stated is all-five-or-none. This is the refusal that would + // otherwise have been lost when the completeness check went in. + let plan = parse( + [ + with_target( + "plan-operation", + &[ + "--operation", + "install", + "--provider-release-digest", + DIGEST, + "--operation-id", + "operation_00000000000000000000000", + "--expires-at", + "2027-01-01T00:00:00.000Z", + ], + ), + partial, + ] + .concat(), + ) + .unwrap_err(); + assert!(plan.detail().contains("all five"), "{}", plan.detail()); } #[test] diff --git a/crates/setup-core/src/software.rs b/crates/setup-core/src/software.rs index e63cdc7..e7dd694 100644 --- a/crates/setup-core/src/software.rs +++ b/crates/setup-core/src/software.rs @@ -173,6 +173,22 @@ pub struct Installed { } impl Software { + /// Where this build's own artifacts put the executable inside their tree. + /// + /// A *hint*, and named one deliberately: it is right for a version this + /// build installed and is only a first guess for a tree an older build + /// wrote. [`rollback`] tries it, then the flat shape, and refuses naming + /// both rather than pointing a command at a path it did not verify. + #[must_use] + pub fn member_hint(&self) -> &'static str { + match self.delivery { + Delivery::Artifacts(artifacts) => { + artifacts.first().map_or(self.command, |entry| entry.member) + } + Delivery::Manager { .. } => self.command, + } + } + /// The artifact for one platform, or the reason there is not one. /// /// # Errors @@ -395,6 +411,85 @@ pub fn remove(software: &Software, root: &Path) -> Result { Ok(true) } +/// Point the exposed command back at a version that is already on disk. +/// +/// Installing 1.0.6 leaves 1.0.5 in its own directory and moves only the +/// exposed command, so the bytes to go back to are already there. Until this +/// existed nothing pointed at them: the owner named rollback in the same +/// sentence as install, reinstall and select, and three of those four were +/// reachable. +/// +/// This is the one part of the software lifecycle that needs no network at all, +/// which is why it can be a command someone types rather than the three-phase +/// exchange install and update have to be. +/// +/// **The version is named, never inferred.** There is no record of what was +/// previous -- only what is on disk -- and these version strings do not order +/// reliably: `2026.08.11-e8db854` sorts by string, not by release. Picking "the +/// one before" would mean inventing an ordering the vendor never promised, and +/// pointing a command at the wrong build is exactly the class of mistake this +/// program refuses everywhere else. A caller who omits it is told what is here. +/// +/// # Errors +/// +/// Refuses a version that is not installed, naming the ones that are, and a +/// version tree that holds no executable this build can find. +pub fn rollback(software: &Software, root: &Path, to: &str) -> Result { + let present = Present::under(root, software.command); + if !present.versions.iter().any(|found| found == to) { + return Err(Error::new( + ReasonCode::InvalidTarget, + if present.versions.is_empty() { + format!( + "{} holds no installed version of {}", + root.display(), + software.command + ) + } else { + format!( + "{to} is not installed under {}; it holds {}", + root.display(), + present.versions.join(", ") + ) + }, + )); + } + + let version_root = root.join(to); + // Looked for rather than assumed. This build pins one version and knows + // where *its* executable sits inside the archive; an older tree was written + // by an older build, whose artifact table this one does not carry. Both + // shapes it could have used are tried, and neither is guessed at: if the + // file is not there, the refusal says where it looked. + let candidates = [ + version_root.join(software.member_hint()), + version_root.join(software.command), + ]; + let Some(executable) = candidates.iter().find(|path| path.is_file()) else { + return Err(Error::new( + ReasonCode::StateUnavailable, + format!( + "the {to} tree holds no {} executable; looked at {}", + software.command, + candidates + .iter() + .map(|path| path.display().to_string()) + .collect::>() + .join(" and ") + ), + )); + }; + + let exposed = root.join("bin").join(software.command); + expose(executable, &exposed)?; + Ok(Installed { + version: to.to_owned(), + root: version_root, + executable: exposed, + files: 0, + }) +} + /// Point one stable path at the executable inside a versioned tree. /// /// The member is left where the archive put it. Codex's binary needs the `rg` @@ -471,6 +566,78 @@ mod tests { } } + /// The bytes to go back to are already on disk: installing a new version + /// leaves the old tree in place and moves only the exposed command. Until + /// `rollback` existed nothing pointed back at them, and the owner named + /// rollback in the same sentence as install, reinstall and select. + #[test] + fn rollback_points_the_command_at_a_version_already_on_disk() { + let (at, artifact) = staged("rollback", b"#!/bin/sh\necho new\n", CODEX_MEMBER); + let root = at.join("prefix"); + let installed = install(&software(), &artifact, &at.join("artifact.tgz"), &root).unwrap(); + assert_eq!(installed.version, "1.2.3"); + + // What an update leaves behind: the previous tree, untouched. + let older = root.join("1.2.2"); + fs::create_dir_all(older.join("package/vendor/x86_64-unknown-linux-musl/bin")).unwrap(); + fs::write(older.join(CODEX_MEMBER), b"#!/bin/sh\necho old\n").unwrap(); + + let rolled = rollback(&software(), &root, "1.2.2").unwrap(); + assert_eq!(rolled.version, "1.2.2"); + + let present = Present::under(&root, "codex"); + assert_eq!(present.exposed.as_deref(), Some("1.2.2")); + assert_eq!(present.versions, vec!["1.2.2", "1.2.3"]); + + // And the version it came from is still there to go forward to. + assert!(root.join("1.2.3").join(CODEX_MEMBER).is_file()); + assert_eq!( + rollback(&software(), &root, "1.2.3").unwrap().version, + "1.2.3" + ); + assert_eq!( + Present::under(&root, "codex").exposed.as_deref(), + Some("1.2.3") + ); + } + + /// A version that is not there is refused, and the refusal says what is -- + /// otherwise a caller's only way to find out is to guess again. + #[test] + fn rollback_to_a_version_that_is_not_installed_names_the_ones_that_are() { + let (at, artifact) = staged("rollback-missing", b"x", CODEX_MEMBER); + let root = at.join("prefix"); + install(&software(), &artifact, &at.join("artifact.tgz"), &root).unwrap(); + + let error = rollback(&software(), &root, "9.9.9").unwrap_err(); + assert!(error.detail().contains("9.9.9"), "{}", error.detail()); + assert!(error.detail().contains("1.2.3"), "{}", error.detail()); + // Nothing moved. + assert_eq!( + Present::under(&root, "codex").exposed.as_deref(), + Some("1.2.3") + ); + } + + /// A tree an older build wrote may not put the executable where this + /// build's artifacts do. Both shapes are tried and neither is guessed at: + /// if the file is not there, the refusal says where it looked. + #[test] + fn a_version_tree_with_no_executable_is_refused_naming_where_it_looked() { + let (at, artifact) = staged("rollback-empty", b"x", CODEX_MEMBER); + let root = at.join("prefix"); + install(&software(), &artifact, &at.join("artifact.tgz"), &root).unwrap(); + fs::create_dir_all(root.join("1.2.2")).unwrap(); + + let error = rollback(&software(), &root, "1.2.2").unwrap_err(); + assert!(error.detail().contains("1.2.2"), "{}", error.detail()); + assert!(error.detail().contains("looked at"), "{}", error.detail()); + assert_eq!( + Present::under(&root, "codex").exposed.as_deref(), + Some("1.2.3") + ); + } + fn scratch(name: &str) -> PathBuf { let path = std::env::temp_dir().join(format!("setup-core-software-{name}-{}", std::process::id())); From 04c70f37d52eb3fa0a539d7b37fde95ad7992317 Mon Sep 17 00:00:00 2001 From: rldyourmnd Date: Wed, 26 Aug 2026 19:02:47 +0500 Subject: [PATCH 2/3] fix(test): a fixture that was only absolute on one of the three systems On Windows a rooted path is not an absolute path: Path::new("/tmp") answers is_absolute() == false, because absolute means a drive letter or a UNC prefix. The new argv table test built its complete invocations from /tmp, so launch -- whose --prefix is checked for absoluteness at parse time -- refused the fixture before it reached its assertion. The product code is right. The fixture proved the right thing on two systems and nothing at all on the third. Rendered from NDDev-it-com/setup-systems@d233a90. Claude-Session: https://claude.ai/code/session_012UZkeU7yQhyZqKAaDaQEjA --- crates/provider-v3/src/argv.rs | 40 ++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/crates/provider-v3/src/argv.rs b/crates/provider-v3/src/argv.rs index b1b877f..5b7a86d 100644 --- a/crates/provider-v3/src/argv.rs +++ b/crates/provider-v3/src/argv.rs @@ -672,27 +672,39 @@ mod tests { } /// A complete, well-formed invocation of one command. + /// + /// Paths come from `std::env::temp_dir()` rather than `/tmp`, because **on + /// Windows a rooted path is not an absolute path**: `Path::new("/tmp")` + /// answers `is_absolute() == false` there, since absolute means a drive or + /// a UNC prefix. This project has met that once before -- a fixture passing + /// `/tmp` made a test prove the right thing on two systems and nothing at + /// all on the third -- and this test found it again on the first Windows run + /// after it was written, with `--prefix "/tmp/prefix" is not an absolute + /// path`. The product code was right both times. fn complete(command: Command) -> Vec { + let temporary = |name: &str| { + std::env::temp_dir() + .join(name) + .to_string_lossy() + .into_owned() + }; let mut tokens = vec![command.as_str().to_owned()]; for flag in usage(command).required { tokens.push((*flag).to_owned()); if *flag == "--json" { continue; } - tokens.push( - match *flag { - "--target" => "/tmp/target", - "--operation" => "install", - "--bundle" | "--plan" => "/tmp/file", - "--bundle-format" => "ai-stp-bundle/1", - "--bundle-size" => "4096", - "--operation-id" => "operation_00000000000000000000000", - "--expires-at" => "2027-01-01T00:00:00.000Z", - "--prefix" => "/tmp/prefix", - _ => DIGEST, - } - .to_owned(), - ); + tokens.push(match *flag { + "--target" => temporary("target"), + "--operation" => "install".to_owned(), + "--bundle" | "--plan" => temporary("file"), + "--bundle-format" => "ai-stp-bundle/1".to_owned(), + "--bundle-size" => "4096".to_owned(), + "--operation-id" => "operation_00000000000000000000000".to_owned(), + "--expires-at" => "2027-01-01T00:00:00.000Z".to_owned(), + "--prefix" => temporary("prefix"), + _ => DIGEST.to_owned(), + }); } // `install` arrives as a bundle, so a complete plan for it carries one. if command == Command::PlanOperation { From fa2d4799fe574f49bb0f0c80ddf4754f3cafa0f1 Mon Sep 17 00:00:00 2001 From: rldyourmnd Date: Wed, 26 Aug 2026 19:17:46 +0500 Subject: [PATCH 3/3] fix: on Windows nothing was ever exposed, so no update was ever an update Present::exposed answered by resolving bin/. Windows reserves symlink creation for privileged processes, so expose writes a hard link or a copy there and canonicalizing one returns its own path -- the answer was always "no version is exposed" on a prefix where one plainly was. Not cosmetic: that reading is what separates an install from an update, so every software_update on Windows refused as an update of nothing while the version it would have updated sat right there. The version is recorded beside the command and read back now. A dangling link still exposes nothing, whatever the record says. Rendered from NDDev-it-com/setup-systems@fa233ff. Claude-Session: https://claude.ai/code/session_012UZkeU7yQhyZqKAaDaQEjA --- crates/setup-core/src/software.rs | 128 ++++++++++++++++++++++++++---- 1 file changed, 112 insertions(+), 16 deletions(-) diff --git a/crates/setup-core/src/software.rs b/crates/setup-core/src/software.rs index e7dd694..d2f81db 100644 --- a/crates/setup-core/src/software.rs +++ b/crates/setup-core/src/software.rs @@ -108,6 +108,15 @@ pub struct Present { } impl Present { + /// Where the exposed version is recorded, beside the command it describes. + /// + /// Dotted so it is hidden on Unix, and under `bin/` so it can never be read + /// as a version directory. + #[must_use] + fn marker(root: &Path, command: &str) -> PathBuf { + root.join("bin").join(format!(".{command}.version")) + } + /// Whether this build's pinned version is one of the ones already there. #[must_use] pub fn holds(&self, version: &str) -> bool { @@ -142,18 +151,50 @@ impl Present { // entry point names it. Resolving costs one syscall and is right for // both. A dangling link resolves to nothing, which is also correct: // nothing usable is exposed. - let link = root.join("bin").join(command); - let exposed = fs::canonicalize(&link) + // Recorded, not inferred -- and the reason is Windows. + // + // Resolving the link was the only reading here, and it cannot work on a + // system where `expose` does not make a link. Windows reserves symlink + // creation for privileged processes, so the exposed command is a hard + // link or a copy: canonicalizing it returns its own path, its first + // component under the root is `bin`, and the answer became "no version + // is exposed" on a prefix where one plainly was. + // + // That was not only cosmetic. `Present::exposed` is what separates an + // install from an update, so on Windows every `software_update` saw an + // empty prefix and refused as an update of nothing -- while the version + // it would have updated sat right there. Found by the three-OS matrix + // on the first Windows run of the rollback tests. + // A record is only ever believed about a command that resolves. A + // dangling link exposes nothing whatever the record says -- the record + // remembers what `expose` last pointed at, and someone can break that + // by hand afterwards. `metadata` follows links, so this is false for a + // dangling one and true for both a real file and a live link. + let usable = fs::metadata(root.join("bin").join(command)).is_ok(); + let marker = Self::marker(root, command); + let exposed = fs::read_to_string(&marker) .ok() - .zip(fs::canonicalize(root).ok()) - .and_then(|(to, base)| { - to.strip_prefix(&base).ok().and_then(|rest| { - rest.components() - .next() - .map(|first| first.as_os_str().to_string_lossy().into_owned()) - }) - }) - .filter(|name| versions.contains(name)); + .filter(|_| usable) + .map(|held| held.trim().to_owned()) + // A hand-edited or half-written marker must not name a version that + // is not there. + .filter(|name| versions.contains(name)) + .or_else(|| { + // Nothing recorded: a prefix written before this existed, or one + // someone arranged themselves. Where a real link is what is + // there, reading it is still the truth. + fs::canonicalize(root.join("bin").join(command)) + .ok() + .zip(fs::canonicalize(root).ok()) + .and_then(|(to, base)| { + to.strip_prefix(&base).ok().and_then(|rest| { + rest.components() + .next() + .map(|first| first.as_os_str().to_string_lossy().into_owned()) + }) + }) + .filter(|name| versions.contains(name)) + }); Self { versions, exposed } } @@ -368,7 +409,7 @@ pub fn install( }; let exposed = root.join("bin").join(software.command); - expose(&executable, &exposed)?; + expose(&executable, &exposed, software.version, software.command)?; Ok(Installed { version: software.version.to_owned(), @@ -408,6 +449,9 @@ pub fn remove(software: &Software, root: &Path) -> Result { .with_source(error) })?; } + // The record goes with the command it described. A marker outliving it + // would name a version nothing runs. + let _ = fs::remove_file(Present::marker(root, software.command)); Ok(true) } @@ -481,7 +525,7 @@ pub fn rollback(software: &Software, root: &Path, to: &str) -> Result }; let exposed = root.join("bin").join(software.command); - expose(executable, &exposed)?; + expose(executable, &exposed, to, software.command)?; Ok(Installed { version: to.to_owned(), root: version_root, @@ -496,7 +540,7 @@ pub fn rollback(software: &Software, root: &Path, to: &str) -> Result /// and `bwrap` beside it and cursor's launcher needs its bundled `node`, so /// moving the executable out of its tree would produce a file that runs on the /// machine it was built on and nowhere else. -fn expose(executable: &Path, exposed: &Path) -> Result<()> { +fn expose(executable: &Path, exposed: &Path, version: &str, command: &str) -> Result<()> { let fail = |error: std::io::Error| { Error::new( ReasonCode::StateUnavailable, @@ -514,7 +558,7 @@ fn expose(executable: &Path, exposed: &Path) -> Result<()> { #[cfg(unix)] { - std::os::unix::fs::symlink(executable, exposed).map_err(fail) + std::os::unix::fs::symlink(executable, exposed).map_err(fail)?; } #[cfg(not(unix))] { @@ -523,8 +567,17 @@ fn expose(executable: &Path, exposed: &Path) -> Result<()> { // resort and costs a second copy of a large binary. fs::hard_link(executable, exposed) .or_else(|_| fs::copy(executable, exposed).map(|_| ())) - .map_err(fail) + .map_err(fail)?; } + + // Which version this now runs, recorded rather than left to be inferred + // from a link that two of the three systems do not make. Written after the + // command is in place, so a marker never names a version that is not + // exposed yet. + if let Some(root) = exposed.parent().and_then(Path::parent) { + fs::write(Present::marker(root, command), version).map_err(fail)?; + } + Ok(()) } #[cfg(test)] @@ -601,6 +654,49 @@ mod tests { ); } + /// The exposed version is recorded, so it is readable where no link exists. + /// + /// This is the Windows defect written as a test that fails on Linux too. + /// `expose` makes a symlink on Unix and a hard link or a copy on Windows, + /// and the old reading resolved the link -- so on Windows the answer was + /// always "nothing is exposed", on a prefix where something plainly was. + /// + /// It was not cosmetic: `Present::exposed` is what separates an install + /// from an update, so every `software_update` on Windows saw an empty + /// prefix and refused as an update of nothing. + #[test] + fn the_exposed_version_is_readable_without_a_link_to_resolve() { + let (at, artifact) = staged("exposed-marker", b"#!/bin/sh\necho hi\n", CODEX_MEMBER); + let root = at.join("prefix"); + install(&software(), &artifact, &at.join("artifact.tgz"), &root).unwrap(); + assert_eq!( + Present::under(&root, "codex").exposed.as_deref(), + Some("1.2.3") + ); + + // Exactly what Windows leaves behind: a real file where Unix has a + // link. Nothing to resolve, and the answer must not change. + let exposed = root.join("bin").join("codex"); + let bytes = fs::read(root.join("1.2.3").join(CODEX_MEMBER)).unwrap(); + fs::remove_file(&exposed).unwrap(); + fs::write(&exposed, &bytes).unwrap(); + assert!(!exposed.symlink_metadata().unwrap().is_symlink()); + assert_eq!( + Present::under(&root, "codex").exposed.as_deref(), + Some("1.2.3"), + "the exposed version was unreadable without a link to resolve" + ); + + // A record naming a version that is not there is not believed. + fs::write(root.join("bin").join(".codex.version"), "9.9.9").unwrap(); + assert_eq!(Present::under(&root, "codex").exposed, None); + + // And removing takes the record with the command it described. + fs::write(root.join("bin").join(".codex.version"), "1.2.3").unwrap(); + remove(&software(), &root).unwrap(); + assert!(!root.join("bin").join(".codex.version").exists()); + } + /// A version that is not there is refused, and the refusal says what is -- /// otherwise a caller's only way to find out is to guess again. #[test]