From 5a1096268e8922585151b40bd82d5e3d489106d8 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Tue, 1 Sep 2026 17:28:43 -0400 Subject: [PATCH 1/5] refactor(core): give the has-an-update predicate one home `fix` planned rewrites for a status set, `report_inherited_skips` reported skips for the same set, and `ManifestCheck::outdated` iterated it again -- three hand-written copies of one `matches!`. They have to agree or the commands built on them contradict each other, so make the agreement structural: `DependencyStatus::has_update`, called from all three. --- crates/dependable-core/src/result.rs | 18 ++++++++++++++++++ crates/dependable-fetch/src/check.rs | 10 +--------- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/crates/dependable-core/src/result.rs b/crates/dependable-core/src/result.rs index 732032a..3c2bcff 100644 --- a/crates/dependable-core/src/result.rs +++ b/crates/dependable-core/src/result.rs @@ -140,6 +140,24 @@ impl DependencyStatus { } } + /// Whether a newer release exists that this dependency could move to. + /// + /// The one predicate behind every "there is something to do here" decision: + /// which rows `fix` plans a rewrite for, and which of the rows it cannot + /// rewrite are worth saying so about. Those two answers must be the same set + /// or the two commands contradict each other, which is exactly the defect + /// this replaced three hand-written copies of the same `matches!` to prevent. + /// + /// [`Vulnerable`](Self::Vulnerable) counts: a vulnerable-but-current + /// dependency is the one most worth upgrading. + #[must_use] + pub fn has_update(&self) -> bool { + matches!( + self, + Self::PatchAvailable | Self::UpdateAvailable | Self::Outdated | Self::Vulnerable + ) + } + /// A stable uppercase token for machine-readable output. #[must_use] pub fn token(&self) -> &'static str { diff --git a/crates/dependable-fetch/src/check.rs b/crates/dependable-fetch/src/check.rs index f1aceff..883a7cc 100644 --- a/crates/dependable-fetch/src/check.rs +++ b/crates/dependable-fetch/src/check.rs @@ -124,15 +124,7 @@ pub struct ManifestCheck { impl ManifestCheck { /// Results that represent an available upgrade (patch/update/outdated/vulnerable). pub fn outdated(&self) -> impl Iterator { - self.results.iter().filter(|r| { - matches!( - r.status, - DependencyStatus::PatchAvailable - | DependencyStatus::UpdateAvailable - | DependencyStatus::Outdated - | DependencyStatus::Vulnerable - ) - }) + self.results.iter().filter(|r| r.status.has_update()) } /// Results with known advisories on the current version. From 0399a0a146d25891e1a61783d5ba3be4b0dd9bc5 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Tue, 1 Sep 2026 17:28:49 -0400 Subject: [PATCH 2/5] refactor(fix): make a declined rewrite say which guard refused it `rewrite_constraint` returned `Option`, so `plan_fixes` learned that a constraint could not be rewritten and immediately threw away why. That is the lossy boundary: the reason is live only at the guard that fires, and recovering it later would mean a second copy of every guard. Return `Result` instead and carry the declined items out of `plan_fixes` alongside the records. The wildcard guard's three conditions are now checked in the order that makes the best explanation -- an operator answers whatever the ecosystem reads a bare version as, then the ecosystem's reading, then the wildcard's shape -- declining exactly the same set as the single boolean it replaces. --- crates/dependable/src/fix.rs | 544 +++++++++++++++++++++++++---------- 1 file changed, 397 insertions(+), 147 deletions(-) diff --git a/crates/dependable/src/fix.rs b/crates/dependable/src/fix.rs index f7ef157..82ad33b 100644 --- a/crates/dependable/src/fix.rs +++ b/crates/dependable/src/fix.rs @@ -18,8 +18,8 @@ use std::io::Write as _; use std::path::Path; use anyhow::Context; +use dependable_fetch::CheckResult; use dependable_fetch::core::{BareVersion, Ecosystem, ManifestKind}; -use dependable_fetch::{CheckResult, DependencyStatus}; /// A single applied (or would-be-applied) version change. #[derive(Debug, Clone)] @@ -29,6 +29,107 @@ pub struct FixRecord { pub to: String, } +/// Why [`rewrite_constraint`] would not substitute a new version into a +/// constraint. +/// +/// Carried out of the planner rather than recomputed, because the answer is only +/// live at the point the guard fires: reconstructing it later would mean a second +/// copy of every guard below, kept in step with this one by hope. It reaches the +/// user as the second half of a `note:` line — see [`DeclineReason::explain`] — +/// so a dependency `check` reports an update for and `fix` leaves alone says so +/// instead of vanishing into "everything is already up to date". +/// +/// `#[non_exhaustive]`: match with a wildcard arm so a new guard is additive. +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] +#[non_exhaustive] +pub enum DeclineReason { + /// A comma-separated range: Cargo's `>=1.0, <2.0`. + CommaRange, + /// A space- or `|`-separated range: `>=1.0.0 <2.0.0`, `^1 || ^2`. + MultiClause, + /// An `@` qualifier: a Composer stability flag (`@dev`, `^1.0@beta`) or an + /// npm alias (`npm:pkg@1.0.0`). + Qualifier, + /// A dist-tag or channel name: `latest`, `next`. + DistTag, + /// A wildcard behind an operator (`^1.x`, `=1.*`), whose rewrite would not be + /// a bare version at all. + WildcardOperator, + /// A wildcard in an ecosystem that reads a bare version as one release: + /// npm's `"lodash": "1.x"`. + WildcardPins, + /// A wildcard in an ecosystem that reads a bare version as a minimum: + /// NuGet's `1.*`, Gradle's `1.+`. + WildcardUnbounds, + /// A wildcard whose shape no bare version reproduces even where the bare + /// reading is a caret: `*`, `1.2.*`, `1.+`. + WildcardShape, + /// A partial version, which is an X-range wherever a bare version is exact: + /// npm's `"react": "16"`. + PartialVersion, +} + +impl DeclineReason { + /// The clause that completes a `note:` line, reading on from + /// "… is available, but ". + /// + /// Every reason says what the constraint *is*, not that a rule fired — the + /// point of the note is to let the author decide whether to widen the + /// constraint by hand, and a rule name would not help them do that. + #[must_use] + pub fn explain(self) -> &'static str { + match self { + Self::CommaRange => { + "a comma-separated range has two bounds and one version cannot carry both" + } + Self::MultiClause => { + "a space- or `||`-separated range has more than one clause and one version \ + cannot carry them all" + } + Self::Qualifier => { + "an `@` qualifier — a stability flag or an alias — describes the range, not the \ + version" + } + Self::DistTag => "a dist-tag names a release channel, not a version", + Self::WildcardOperator => { + "an operator in front of a wildcard is a range the new version would not reproduce" + } + Self::WildcardPins => { + "a wildcard already tracks new releases, and a bare version here would pin it to \ + one" + } + Self::WildcardUnbounds => { + "a wildcard already tracks new releases, and a bare version here would drop its \ + upper bound" + } + Self::WildcardShape => { + "a wildcard already tracks new releases, and no bare version covers the same range" + } + Self::PartialVersion => { + "a partial version is an X-range that already tracks new releases" + } + } + } +} + +/// An update `check` reports that `fix` will not write. +/// +/// The whole point of recording it: without one, a declined constraint and a +/// dependency with nothing to do are the same empty result, and `fix` answers +/// "everything is already up to date" to a manifest `check` just said had an +/// update waiting. +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] +pub struct Declined { + /// The dependency's name. + pub name: String, + /// The constraint left in place, verbatim. + pub constraint: String, + /// The version that would have been written had the constraint allowed it. + pub target: String, + /// Why it was not. + pub reason: DeclineReason, +} + /// A byte-range replacement within one line of the manifest. struct Edit { line: usize, @@ -53,6 +154,9 @@ pub struct PlannedFix { updated: String, /// What changed, for reporting. pub records: Vec, + /// The updates this rewrite declined to make, and why — reported so `check` + /// and `fix` do not appear to contradict each other. + pub declined: Vec, } /// Compute the rewrite for `manifest` without touching it. @@ -76,12 +180,13 @@ pub fn plan(manifest: &Path, results: &[CheckResult], all: bool) -> anyhow::Resu // `detect` does not recognize — is not an error here: the rewrite still runs, // under the reading that declines the most (see [`rewrite_constraint`]). let ecosystem = ManifestKind::detect(manifest).map(ManifestKind::ecosystem); - let (updated, records) = plan_fixes(&content, results, all, ecosystem) + let (updated, records, declined) = plan_fixes(&content, results, all, ecosystem) .with_context(|| format!("rewriting {}", manifest.display()))?; Ok(PlannedFix { path: manifest.to_path_buf(), updated, records, + declined, }) } @@ -136,14 +241,22 @@ pub fn commit(planned: &PlannedFix) -> anyhow::Result<()> { /// [`rewrite_constraint`] explains what turns on it — and `None` means the /// manifest kind was not recognized, which is treated as the most restrictive /// answer rather than as permission. +/// +/// Returns the rewritten content, the changes made, and the changes *not* made: +/// every dependency with an update available whose constraint +/// [`rewrite_constraint`] declined. The third list exists because it cannot be +/// recovered afterwards — the caller would have to redo the rewritability, +/// pinning, and target selection above *and* every guard inside +/// [`rewrite_constraint`] to learn what this loop already knew and threw away. fn plan_fixes( content: &str, results: &[CheckResult], all: bool, ecosystem: Option, -) -> anyhow::Result<(String, Vec)> { +) -> anyhow::Result<(String, Vec, Vec)> { let mut edits: Vec = Vec::new(); let mut records = Vec::new(); + let mut declined = Vec::new(); for result in results { let item = &result.item; // `is_rewritable` and not `is_checkable`: a workspace member inheriting a @@ -153,14 +266,7 @@ fn plan_fixes( if !item.is_rewritable() { continue; } - let updatable = matches!( - result.status, - DependencyStatus::PatchAvailable - | DependencyStatus::UpdateAvailable - | DependencyStatus::Outdated - | DependencyStatus::Vulnerable - ); - if !updatable || (item.is_pinned() && !all) { + if !result.status.has_update() || (item.is_pinned() && !all) { continue; } @@ -170,10 +276,20 @@ fn plan_fixes( result.latest_compatible.as_ref() }; let Some(target) = target else { continue }; - let Some(new_constraint) = rewrite_constraint(&item.version_constraint, target, ecosystem) - else { - continue; + let new_constraint = match rewrite_constraint(&item.version_constraint, target, ecosystem) { + Ok(new_constraint) => new_constraint, + Err(reason) => { + declined.push(Declined { + name: item.name.clone(), + constraint: item.version_constraint.clone(), + target: target.clone(), + reason, + }); + continue; + } }; + // Already at the target: nothing to write and nothing to say. Not a + // decline — the constraint would have accepted the rewrite. if new_constraint == item.version_constraint { continue; } @@ -197,11 +313,15 @@ fn plan_fixes( } else { apply_edits(content, &edits)? }; - Ok((updated, records)) + // One note per distinct decline: the same crate under `[dependencies]` and + // `[dev-dependencies]` is one fact about one constraint, not two. + declined.sort(); + declined.dedup(); + Ok((updated, records, declined)) } /// Build a new constraint from `original`, preserving its leading operator/`v` -/// prefix and substituting `new_version`. Returns `None` for the forms that +/// prefix and substituting `new_version`. Returns [`Err`] for the forms that /// can't be rewritten without changing their meaning: a comma-separated range /// (Cargo `>=1.0, <2.0`), a space-separated range (npm/pubspec `>=1.0.0 <2.0.0`), /// a `||` alternation (`^1 || ^2`), a dist-tag (`latest`), anything carrying an @@ -209,6 +329,11 @@ fn plan_fixes( /// such as `npm:pkg@1.0.0`), and — depending on `ecosystem` — a wildcard (`*`, /// `1.x`, `1.*`) or a partial version (npm `"16"`). /// +/// The error is a [`DeclineReason`] and not a bare `None`, because *which* guard +/// fired is the only thing that makes the resulting note actionable, and this is +/// the sole place that knows it. An `Option` return threw that away at the one +/// boundary where it was still free. +/// /// `ecosystem` is what the wildcard and partial-version guards turn on, because /// both ask the same question: the rewrite writes the new version back bare, so /// is a bare version in this ecosystem still the range the author had? `None` @@ -216,14 +341,17 @@ fn plan_fixes( /// [`BareVersion::Exact`], which declines strictly more than either other /// reading, so an unknown manifest is never rewritten into something a known one /// would have refused. +/// +/// # Errors +/// Returns the [`DeclineReason`] for the first guard that refuses the rewrite. fn rewrite_constraint( original: &str, new_version: &str, ecosystem: Option, -) -> Option { +) -> Result { let trimmed = original.trim(); if trimmed.contains(',') { - return None; + return Err(DeclineReason::CommaRange); } const OP_CHARS: &[char] = &['^', '~', '>', '<', '=', '!', 'v', 'V', ' ', '\t']; let prefix: String = trimmed @@ -234,7 +362,7 @@ fn rewrite_constraint( // clause (range upper bound or alternative) we'd silently drop — leave it be. let rest = &trimmed[prefix.len()..]; if rest.contains([' ', '\t', '|']) { - return None; + return Err(DeclineReason::MultiClause); } // An `@` never belongs to a version: it introduces a Composer stability flag // (`@dev`, `2.8.*@dev`, `^1.0@beta`) or an npm alias target (`npm:pkg@1.0.0`). @@ -244,13 +372,13 @@ fn rewrite_constraint( // pin. That is the harm of #87, so take the same call already taken for a // dist-tag and decline. if rest.contains('@') { - return None; + return Err(DeclineReason::Qualifier); } // A dist-tag / channel name (`latest`, `next`, `beta`, …) starts with a letter // once any operator prefix is removed — it names a channel, not a version // range, so it must never be pinned to a concrete version (npm D8). if rest.starts_with(|c: char| c.is_ascii_alphabetic()) { - return None; + return Err(DeclineReason::DistTag); } let bare = ecosystem.map_or(BareVersion::Exact, Ecosystem::bare_version); @@ -281,8 +409,27 @@ fn rewrite_constraint( // - An operator in front (`^1.x`, `=1.*`, Python's `==1.*`) means the // result is not a bare version at all, so the caret reading that // justifies the rewrite does not apply to it. - if bare != BareVersion::Caret || !prefix.is_empty() || !is_minor_wildcard(rest) { - return None; + // + // The three conditions are checked in order of what the note should say, + // not in the order they were written: an operator answers for the whole + // constraint whatever the ecosystem reads a bare version as, and the + // ecosystem's reading answers before the wildcard's shape because it is + // the more specific harm. + if !prefix.is_empty() { + return Err(DeclineReason::WildcardOperator); + } + match bare { + BareVersion::Exact => return Err(DeclineReason::WildcardPins), + BareVersion::Minimum => return Err(DeclineReason::WildcardUnbounds), + BareVersion::Caret => {} + // A reading added since this was written. Decline, as every non-caret + // reading already does, under the reason that names no particular + // harm — inventing one for a reading this code has never seen would + // be worse than saying only that the shapes do not correspond. + _ => return Err(DeclineReason::WildcardShape), + } + if !is_minor_wildcard(rest) { + return Err(DeclineReason::WildcardShape); } } else if bare == BareVersion::Exact && prefix.is_empty() && is_partial_version(rest) { // The same harm one wildcard character away. npm treats a partial version @@ -297,9 +444,9 @@ fn rewrite_constraint( // constraint that already pins while a wrong `true` costs the author their // range. Cargo and NuGet keep the rewrite: `1.0` is `^1.0` and `>=1.0` // respectively, and raising the floor of either is exactly what `fix` is. - return None; + return Err(DeclineReason::PartialVersion); } - Some(format!("{prefix}{new_version}")) + Ok(format!("{prefix}{new_version}")) } /// Whether `rest` — a wildcard constraint with its operator prefix already @@ -423,49 +570,51 @@ mod tests { let it = Some(ecosystem); assert_eq!( rewrite_constraint("^1.0", "1.5.0", it).as_deref(), - Some("^1.5.0"), + Ok("^1.5.0"), "{ecosystem:?}" ); assert_eq!( rewrite_constraint("~1.0", "1.5.0", it).as_deref(), - Some("~1.5.0"), + Ok("~1.5.0"), "{ecosystem:?}" ); assert_eq!( rewrite_constraint(">=1.0", "1.5.0", it).as_deref(), - Some(">=1.5.0"), + Ok(">=1.5.0"), "{ecosystem:?}" ); assert_eq!( rewrite_constraint("v1.2.3", "1.5.0", it).as_deref(), - Some("v1.5.0"), + Ok("v1.5.0"), "{ecosystem:?}" ); // A full bare version names one release under every reading, and // moving it forward is what `fix` is for. assert_eq!( rewrite_constraint("1.0.0", "1.5.0", it).as_deref(), - Some("1.5.0"), + Ok("1.5.0"), "{ecosystem:?}" ); assert_eq!( rewrite_constraint("=1.2.0", "1.5.0", it).as_deref(), - Some("=1.5.0"), + Ok("=1.5.0"), "{ecosystem:?}" ); // The bare wildcard `*` is a range, not a version — see // `rewrite_never_narrows_a_wildcard_to_a_pin`. Declined everywhere, // Cargo included: `*` admits every major and a caret admits one. - assert_eq!(rewrite_constraint("*", "1.5.0", it), None, "{ecosystem:?}"); + assert!( + rewrite_constraint("*", "1.5.0", it).is_err(), + "{ecosystem:?}" + ); } } #[test] fn rewrite_skips_multi_constraint() { for ecosystem in EVERY_ECOSYSTEM { - assert_eq!( - rewrite_constraint(">=1.0,<2.0", "1.5.0", Some(ecosystem)), - None, + assert!( + rewrite_constraint(">=1.0,<2.0", "1.5.0", Some(ecosystem)).is_err(), "{ecosystem:?}" ); } @@ -479,24 +628,24 @@ mod tests { // channel name means the same thing wherever one is written. for ecosystem in EVERY_ECOSYSTEM { let it = Some(ecosystem); - assert_eq!( - rewrite_constraint("latest", "2.3.0", it), - None, + assert!( + rewrite_constraint("latest", "2.3.0", it).is_err(), "{ecosystem:?}" ); - assert_eq!( - rewrite_constraint("next", "2.3.0", it), - None, + assert!( + rewrite_constraint("next", "2.3.0", it).is_err(), "{ecosystem:?}" ); - assert_eq!( - rewrite_constraint("beta", "2.3.0", it), - None, + assert!( + rewrite_constraint("beta", "2.3.0", it).is_err(), "{ecosystem:?}" ); // The bare wildcard `*` is declined for the same reason: it is a range // the author chose, and pinning it would narrow their manifest (#87). - assert_eq!(rewrite_constraint("*", "2.3.0", it), None, "{ecosystem:?}"); + assert!( + rewrite_constraint("*", "2.3.0", it).is_err(), + "{ecosystem:?}" + ); } } @@ -519,40 +668,37 @@ mod tests { continue; } let it = Some(ecosystem); - assert_eq!( - rewrite_constraint("1.x", "2.0.0", it), - None, + assert!( + rewrite_constraint("1.x", "2.0.0", it).is_err(), "{ecosystem:?}" ); - assert_eq!( - rewrite_constraint("1.*", "2.0.0", it), - None, + assert!( + rewrite_constraint("1.*", "2.0.0", it).is_err(), "{ecosystem:?}" ); - assert_eq!( - rewrite_constraint("1.X", "2.0.0", it), - None, + assert!( + rewrite_constraint("1.X", "2.0.0", it).is_err(), "{ecosystem:?}" ); // Gradle's dynamic version has the same shape (issue #87), and NuGet's // floating `1.*` resolves differently from a bare `2.0.0`. - assert_eq!( - rewrite_constraint("1.+", "2.0.0", it), - None, + assert!( + rewrite_constraint("1.+", "2.0.0", it).is_err(), "{ecosystem:?}" ); - assert_eq!( - rewrite_constraint("^1.x", "2.0.0", it), - None, + assert!( + rewrite_constraint("^1.x", "2.0.0", it).is_err(), "{ecosystem:?}" ); - assert_eq!( - rewrite_constraint("1.2.x", "2.0.0", it), - None, + assert!( + rewrite_constraint("1.2.x", "2.0.0", it).is_err(), "{ecosystem:?}" ); // The bare wildcard is the same kind of thing. - assert_eq!(rewrite_constraint("*", "2.0.0", it), None, "{ecosystem:?}"); + assert!( + rewrite_constraint("*", "2.0.0", it).is_err(), + "{ecosystem:?}" + ); } // Cargo reads a bare version as a caret, which reproduces exactly one @@ -560,16 +706,16 @@ mod tests { let cargo = Some(Ecosystem::Rust); // Gradle's `+` is a prefix range with its own resolution rules, and no // caret-reading ecosystem accepts it as a wildcard at all. - assert_eq!(rewrite_constraint("1.+", "2.0.0", cargo), None); + assert!(rewrite_constraint("1.+", "2.0.0", cargo).is_err()); // An operator means what gets written back is not a bare version, so the // caret reading that would justify the rewrite does not apply to it. - assert_eq!(rewrite_constraint("^1.x", "2.0.0", cargo), None); - assert_eq!(rewrite_constraint("=1.*", "2.0.0", cargo), None); + assert!(rewrite_constraint("^1.x", "2.0.0", cargo).is_err()); + assert!(rewrite_constraint("=1.*", "2.0.0", cargo).is_err()); // `1.2.*` is `>=1.2.0, <1.3.0`; a caret over any 1.2.z release reaches to // `<2.0.0`, so substituting *widens* what the author admitted. - assert_eq!(rewrite_constraint("1.2.x", "2.0.0", cargo), None); + assert!(rewrite_constraint("1.2.x", "2.0.0", cargo).is_err()); // `*` is every version; any concrete release confines it to one major. - assert_eq!(rewrite_constraint("*", "2.0.0", cargo), None); + assert!(rewrite_constraint("*", "2.0.0", cargo).is_err()); } /// The other side of issue #92: declining every wildcard was conservatism, not @@ -583,15 +729,15 @@ mod tests { let cargo = Some(Ecosystem::Rust); assert_eq!( rewrite_constraint("1.*", "1.0.219", cargo).as_deref(), - Some("1.0.219") + Ok("1.0.219") ); assert_eq!( rewrite_constraint("1.x", "1.0.219", cargo).as_deref(), - Some("1.0.219") + Ok("1.0.219") ); assert_eq!( rewrite_constraint("1.X", "1.0.219", cargo).as_deref(), - Some("1.0.219") + Ok("1.0.219") ); // The same input is declined for every ecosystem that reads a bare version // any other way — the whole point of asking which one this is. @@ -599,15 +745,14 @@ mod tests { if ecosystem == Ecosystem::Rust { continue; } - assert_eq!( - rewrite_constraint("1.*", "1.0.219", Some(ecosystem)), - None, + assert!( + rewrite_constraint("1.*", "1.0.219", Some(ecosystem)).is_err(), "{ecosystem:?}" ); } // And a manifest whose kind was not recognized gets the reading that // declines the most, never the one that permits the most. - assert_eq!(rewrite_constraint("1.*", "1.0.219", None), None); + assert!(rewrite_constraint("1.*", "1.0.219", None).is_err()); } /// Issue #92's second gap, and the one with no `*` in it. npm reads a partial @@ -619,33 +764,33 @@ mod tests { #[test] fn rewrite_declines_a_partial_version_where_a_bare_version_is_exact() { let npm = Some(Ecosystem::Npm); - assert_eq!(rewrite_constraint("16", "16.14.0", npm), None); - assert_eq!(rewrite_constraint("1.0", "1.5.0", npm), None); + assert!(rewrite_constraint("16", "16.14.0", npm).is_err()); + assert!(rewrite_constraint("1.0", "1.5.0", npm).is_err()); // Cargo's `1.0` is `^1.0` and `1.5.0` is `^1.5.0`: the floor rises and the // upper bound holds, which is what every other `fix` rewrite does. assert_eq!( rewrite_constraint("1.0", "1.5.0", Some(Ecosystem::Rust)).as_deref(), - Some("1.5.0") + Ok("1.5.0") ); // NuGet's `1.0` is `>= 1.0` and `1.5.0` is `>= 1.5.0` — a raised floor too. assert_eq!( rewrite_constraint("1.0", "1.5.0", Some(Ecosystem::CSharp)).as_deref(), - Some("1.5.0") + Ok("1.5.0") ); // Only the *partial* form is a range. A full bare version is a pin, and // moving a pin forward is exactly what `fix` is asked to do. assert_eq!( rewrite_constraint("16.0.0", "16.14.0", npm).as_deref(), - Some("16.14.0") + Ok("16.14.0") ); // An operator makes it a range in its own right, npm included: `^16` is a // caret range and `^16.14.0` is that range with a raised floor. assert_eq!( rewrite_constraint("^16", "16.14.0", npm).as_deref(), - Some("^16.14.0") + Ok("^16.14.0") ); // An unrecognized manifest declines, like every exact reading. - assert_eq!(rewrite_constraint("16", "16.14.0", None), None); + assert!(rewrite_constraint("16", "16.14.0", None).is_err()); } /// A wildcard segment is not always the whole dot-segment. Composer allows a @@ -662,29 +807,24 @@ mod tests { fn rewrite_declines_a_wildcard_wearing_a_stability_flag() { for ecosystem in EVERY_ECOSYSTEM { let it = Some(ecosystem); - assert_eq!( - rewrite_constraint("2.8.*@dev", "7.0.0", it), - None, + assert!( + rewrite_constraint("2.8.*@dev", "7.0.0", it).is_err(), "{ecosystem:?}" ); - assert_eq!( - rewrite_constraint("2.8.x@dev", "7.0.0", it), - None, + assert!( + rewrite_constraint("2.8.x@dev", "7.0.0", it).is_err(), "{ecosystem:?}" ); - assert_eq!( - rewrite_constraint("1.*@stable", "7.0.0", it), - None, + assert!( + rewrite_constraint("1.*@stable", "7.0.0", it).is_err(), "{ecosystem:?}" ); - assert_eq!( - rewrite_constraint("*@dev", "7.0.0", it), - None, + assert!( + rewrite_constraint("*@dev", "7.0.0", it).is_err(), "{ecosystem:?}" ); - assert_eq!( - rewrite_constraint("^2.8.*@dev", "7.0.0", it), - None, + assert!( + rewrite_constraint("^2.8.*@dev", "7.0.0", it).is_err(), "{ecosystem:?}" ); } @@ -703,33 +843,28 @@ mod tests { for ecosystem in EVERY_ECOSYSTEM { let it = Some(ecosystem); // The bare flag: a range over every version, collapsed to a pin. - assert_eq!( - rewrite_constraint("@dev", "7.0.0", it), - None, + assert!( + rewrite_constraint("@dev", "7.0.0", it).is_err(), "{ecosystem:?}" ); // Flag on an operator-led constraint, and on a bare version. - assert_eq!( - rewrite_constraint(">=2.8@dev", "7.0.0", it), - None, + assert!( + rewrite_constraint(">=2.8@dev", "7.0.0", it).is_err(), "{ecosystem:?}" ); - assert_eq!( - rewrite_constraint("2.8@dev", "7.0.0", it), - None, + assert!( + rewrite_constraint("2.8@dev", "7.0.0", it).is_err(), "{ecosystem:?}" ); - assert_eq!( - rewrite_constraint("^1.0@beta", "7.0.0", it), - None, + assert!( + rewrite_constraint("^1.0@beta", "7.0.0", it).is_err(), "{ecosystem:?}" ); // npm's alias form carries an `@` too. The dist-tag guard caught it only // incidentally, because `npm:` happens to start with a letter; now it is // declined for the reason that actually applies. - assert_eq!( - rewrite_constraint("npm:pkg@1.0.0", "7.0.0", it), - None, + assert!( + rewrite_constraint("npm:pkg@1.0.0", "7.0.0", it).is_err(), "{ecosystem:?}" ); } @@ -757,54 +892,54 @@ mod tests { // Go: a pseudo-version and the `+incompatible` marker. assert_eq!( rewrite_constraint("v0.0.0-20191109021931-daa7c04131f5", "1.5.0", go).as_deref(), - Some("v1.5.0") + Ok("v1.5.0") ); assert_eq!( rewrite_constraint("v2.0.0+incompatible", "1.5.0", go).as_deref(), - Some("v1.5.0") + Ok("v1.5.0") ); // Semver build metadata and prereleases — note the dotted identifiers, // which a leading-character test for `x` would have to survive. assert_eq!( rewrite_constraint("1.2.3+build.5", "1.5.0", rust).as_deref(), - Some("1.5.0") + Ok("1.5.0") ); assert_eq!( rewrite_constraint("1.0.0-alpha+exp.sha.5114f85", "1.5.0", rust).as_deref(), - Some("1.5.0") + Ok("1.5.0") ); // From the semver spec itself: a prerelease whose identifiers include `x`. assert_eq!( rewrite_constraint("1.0.0-x.7.z.92", "1.5.0", rust).as_deref(), - Some("1.5.0") + Ok("1.5.0") ); // NuGet's four-part version — four numeric segments, which the // partial-version guard must not mistake for a truncated one. assert_eq!( rewrite_constraint("1.0.0.4", "1.5.0", nuget).as_deref(), - Some("1.5.0") + Ok("1.5.0") ); // Python epochs and compatible-release operators. assert_eq!( rewrite_constraint("1!2.0", "1.5.0", python).as_deref(), - Some("1.5.0") + Ok("1.5.0") ); assert_eq!( rewrite_constraint("~=1.4", "1.5.0", python).as_deref(), - Some("~=1.5.0") + Ok("~=1.5.0") ); // Hex's `~>`, whose space belongs to the operator prefix. assert_eq!( rewrite_constraint("~> 1.0", "1.5.0", hex).as_deref(), - Some("~> 1.5.0") + Ok("~> 1.5.0") ); // Declined already, and for a different reason: NuGet's bracketed range // holds a comma. The wildcard guard must not change that verdict. - assert_eq!(rewrite_constraint("[1.0,2.0)", "1.5.0", nuget), None); + assert!(rewrite_constraint("[1.0,2.0)", "1.5.0", nuget).is_err()); // Python's `==1.*` is a wildcard, and stays declined — twice over: Python // reads a bare version exactly, and the `==` means the rewrite would not // have produced a bare version anyway. - assert_eq!(rewrite_constraint("==1.*", "1.5.0", python), None); + assert!(rewrite_constraint("==1.*", "1.5.0", python).is_err()); } #[test] @@ -814,25 +949,139 @@ mod tests { // Dropping a clause is a loss in every ecosystem, so assert it in all nine. for ecosystem in EVERY_ECOSYSTEM { let it = Some(ecosystem); - assert_eq!( - rewrite_constraint(">=1.0.0 <2.0.0", "1.5.0", it), - None, + assert!( + rewrite_constraint(">=1.0.0 <2.0.0", "1.5.0", it).is_err(), "{ecosystem:?}" ); - assert_eq!( - rewrite_constraint("^1.0.0 || ^2.0.0", "1.5.0", it), - None, + assert!( + rewrite_constraint("^1.0.0 || ^2.0.0", "1.5.0", it).is_err(), "{ecosystem:?}" ); // A single constraint that merely spaces its operator is still rewritten. assert_eq!( rewrite_constraint(">= 1.0.0", "1.5.0", it).as_deref(), - Some(">= 1.5.0"), + Ok(">= 1.5.0"), "{ecosystem:?}" ); } } + /// A decline is only worth carrying out of the planner if it says which + /// guard fired, because that is the whole content of the note the user sees. + /// One assertion per variant, so a guard that starts answering under another + /// reason changes a test rather than quietly changing what `fix` tells people. + #[test] + fn a_decline_names_the_guard_that_refused_it() { + let cargo = Some(Ecosystem::Rust); + let npm = Some(Ecosystem::Npm); + let nuget = Some(Ecosystem::CSharp); + + let reason = |original, ecosystem| rewrite_constraint(original, "2.0.0", ecosystem).err(); + + assert_eq!(reason(">=1.0,<2.0", cargo), Some(DeclineReason::CommaRange)); + assert_eq!( + reason(">=1.0.0 <2.0.0", npm), + Some(DeclineReason::MultiClause) + ); + assert_eq!( + reason("^1.0.0 || ^2.0.0", npm), + Some(DeclineReason::MultiClause) + ); + assert_eq!(reason("2.8.*@dev", cargo), Some(DeclineReason::Qualifier)); + assert_eq!(reason("latest", npm), Some(DeclineReason::DistTag)); + + // The wildcard family, whose reason is the point of #92: the same three + // characters are declined for three different harms. + // + // An operator answers first, and for every ecosystem — with one in front, + // what gets written back is not a bare version at all, so what a bare + // version *means* here cannot be the reason. + assert_eq!(reason("^1.x", cargo), Some(DeclineReason::WildcardOperator)); + assert_eq!(reason("^1.x", npm), Some(DeclineReason::WildcardOperator)); + // Then the ecosystem's reading, which is the more specific harm than the + // shape: npm pins, NuGet loses the upper bound. + assert_eq!(reason("1.x", npm), Some(DeclineReason::WildcardPins)); + assert_eq!(reason("1.*", nuget), Some(DeclineReason::WildcardUnbounds)); + // And only where a bare version is already a caret does the shape get to + // be the reason — there the reading is fine and the wildcard is not. + assert_eq!(reason("1.2.*", cargo), Some(DeclineReason::WildcardShape)); + assert_eq!(reason("*", cargo), Some(DeclineReason::WildcardShape)); + + assert_eq!(reason("16", npm), Some(DeclineReason::PartialVersion)); + + // An unrecognized manifest reads a bare version exactly, so it declines + // under that reading rather than under a reason of its own. + assert_eq!(reason("1.x", None), Some(DeclineReason::WildcardPins)); + } + + /// The issue #93 defect at the planner's own boundary: `plan_fixes` used to + /// return an empty record list for a wildcard it declined, which is the same + /// answer it returns for a manifest with nothing to do. The declined list is + /// what tells those two apart. + #[test] + fn a_declined_constraint_leaves_a_record_of_what_was_not_done() { + let content = r#"{ + "name": "demo", + "dependencies": { + "lodash": "1.x", + "react": "^18.0.0" + } +} +"#; + let results = results_for( + ManifestKind::PackageJson, + content, + &[("lodash", "1.9.0"), ("react", "18.2.0")], + ); + let (updated, records, declined) = plan_fixes( + content, + &results, + false, + Some(ManifestKind::PackageJson.ecosystem()), + ) + .expect("the plan applies"); + + // The wildcard is untouched and the ordinary caret is rewritten: the + // decline is a record, not a refusal to plan the rest of the manifest. + assert!(updated.contains(r#""lodash": "1.x""#)); + assert!(updated.contains(r#""react": "^18.2.0""#)); + assert_eq!(records.len(), 1); + assert_eq!( + declined, + vec![Declined { + name: "lodash".to_string(), + constraint: "1.x".to_string(), + target: "1.9.0".to_string(), + reason: DeclineReason::WildcardPins, + }] + ); + } + + /// A dependency with nothing available must not be reported as left alone — + /// the note claims `check` had something to say, so anything up to date has + /// to be filtered out before the constraint is ever consulted. + #[test] + fn an_up_to_date_dependency_is_not_a_decline() { + let content = r#"{ + "name": "demo", + "dependencies": { + "lodash": "1.x" + } +} +"#; + // `results_for` marks its targets `UpdateAvailable`; naming none leaves + // the manifest's only dependency with no result at all. + let (_, records, declined) = plan_fixes( + content, + &results_for(ManifestKind::PackageJson, content, &[]), + false, + Some(ManifestKind::PackageJson.ecosystem()), + ) + .expect("the plan applies"); + assert!(records.is_empty()); + assert!(declined.is_empty()); + } + #[test] fn apply_edits_replaces_recorded_span() { // `serde = "^1.0"` — replace the `^1.0` span (bytes 9..13) on line 1. @@ -872,6 +1121,7 @@ mod tests { assert_eq!(out, "a=1.9 b=2.9\n"); } + use dependable_fetch::DependencyStatus; use dependable_fetch::core::{DependencyKind, parse, resolve_workspace_inheritance}; /// Parse `content`, then build an `UpdateAvailable` result with the given @@ -920,7 +1170,7 @@ mod tests { content, &[("react", "18.2.0"), ("typescript", "5.4.5")], ); - let (updated, records) = plan_fixes( + let (updated, records, _declined) = plan_fixes( content, &results, false, @@ -964,7 +1214,7 @@ mod tests { content, &[("monolog/monolog", "2.9.1")], ); - let (updated, records) = plan_fixes( + let (updated, records, _declined) = plan_fixes( content, &results, false, @@ -996,7 +1246,7 @@ mod tests { content, &[("http", "1.2.0"), ("provider", "6.1.0")], ); - let (updated, records) = plan_fixes( + let (updated, records, _declined) = plan_fixes( content, &results, false, @@ -1046,7 +1296,7 @@ mod tests { "the old guards would both have passed" ); - let (updated, records) = plan_fixes( + let (updated, records, _declined) = plan_fixes( member, &results, false, @@ -1081,7 +1331,7 @@ mod tests { ); assert_eq!(declaration.version_line, 1, "and the span points at it"); - let (updated, records) = plan_fixes( + let (updated, records, _declined) = plan_fixes( root, &results, false, @@ -1163,7 +1413,7 @@ mod tests { "the fixture must produce a checkable item" ); - let (updated, records) = plan_fixes( + let (updated, records, _declined) = plan_fixes( content, &results, false, @@ -1205,7 +1455,7 @@ mod tests { "the `--all` branch reads `latest_available`, so the fixture must set it" ); - let (updated, records) = plan_fixes( + let (updated, records, _declined) = plan_fixes( content, &results, true, @@ -1256,7 +1506,7 @@ mod tests { ); assert_eq!(results.len(), 2, "the fixture must produce two items"); - let (updated, records) = plan_fixes( + let (updated, records, _declined) = plan_fixes( content, &results, false, @@ -1288,7 +1538,7 @@ mod tests { let results = results_for(ManifestKind::PackageJson, content, &[("lodash", "1.9.0")]); assert_eq!(results.len(), 1, "the fixture must produce one item"); - let (updated, records) = plan_fixes( + let (updated, records, _declined) = plan_fixes( content, &results, false, @@ -1316,7 +1566,7 @@ mod tests { ); assert_eq!(results.len(), 2, "the fixture must produce two items"); - let (updated, records) = plan_fixes( + let (updated, records, _declined) = plan_fixes( content, &results, false, @@ -1345,19 +1595,19 @@ mod tests { /// scoped to the forms whose meaning depends on the ecosystem. #[test] fn an_unrecognized_manifest_kind_declines_every_ecosystem_dependent_form() { - assert_eq!(rewrite_constraint("1.*", "1.5.0", None), None); - assert_eq!(rewrite_constraint("1.x", "1.5.0", None), None); - assert_eq!(rewrite_constraint("1.0", "1.5.0", None), None); - assert_eq!(rewrite_constraint("16", "16.14.0", None), None); + assert!(rewrite_constraint("1.*", "1.5.0", None).is_err()); + assert!(rewrite_constraint("1.x", "1.5.0", None).is_err()); + assert!(rewrite_constraint("1.0", "1.5.0", None).is_err()); + assert!(rewrite_constraint("16", "16.14.0", None).is_err()); // Not ecosystem-dependent: an operator-led range and a full bare version // mean the same thing everywhere, so they are still rewritten. assert_eq!( rewrite_constraint("^1.0", "1.5.0", None).as_deref(), - Some("^1.5.0") + Ok("^1.5.0") ); assert_eq!( rewrite_constraint("1.0.0", "1.5.0", None).as_deref(), - Some("1.5.0") + Ok("1.5.0") ); } } From 7846a65b11311db7f0725e2f348a722928957012 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Tue, 1 Sep 2026 17:28:55 -0400 Subject: [PATCH 3/5] fix(cli): report the updates fix declined instead of claiming none exist `dependable check` reported an update to "lodash": "1.x" and `dependable fix` answered "Everything is already up to date." for the same manifest, because a declined constraint and a manifest with nothing to do produced the same empty record list. `--dry-run` printed nothing at all. Emit a note per declined update, in the register and on the stream `report_inherited_skips` already uses for the sibling case, and give the closing line a count of what was left alone so it cannot claim otherwise. Every silent decline is covered, not just the wildcard #89 widened the set with: dist-tags and compound ranges have been silent for longer. Closes #93 --- crates/dependable/src/runner.rs | 51 ++++- crates/dependable/tests/cli_fix.rs | 287 +++++++++++++++++++++++++++++ 2 files changed, 328 insertions(+), 10 deletions(-) diff --git a/crates/dependable/src/runner.rs b/crates/dependable/src/runner.rs index 624f810..010fc66 100644 --- a/crates/dependable/src/runner.rs +++ b/crates/dependable/src/runner.rs @@ -900,7 +900,9 @@ pub async fn run_fix(args: FixArgs) -> anyhow::Result { continue; }; report_inherited_skips(manifest, &report); - planned.push(fix::plan(manifest, &report.results, args.all)?); + let plan = fix::plan(manifest, &report.results, args.all)?; + report_declined_fixes(manifest, &plan.declined); + planned.push(plan); } let mut total = 0; @@ -921,8 +923,21 @@ pub async fn run_fix(args: FixArgs) -> anyhow::Result { total += 1; } } + let declined: usize = planned.iter().map(|plan| plan.declined.len()).sum(); if total == 0 { - println!("Everything is already up to date."); + // "Everything is already up to date" is only true when nothing was left + // behind. Saying it over a declined update is the contradiction with + // `check` that this whole path exists to remove, so the count of what was + // left alone takes over the line and points at the notes that explain it. + if declined == 0 { + println!("Everything is already up to date."); + } else { + println!( + "Nothing to rewrite. {declined} available update{} left alone; \ + see the notes above.", + if declined == 1 { "" } else { "s" } + ); + } } else if !args.dry_run { println!( "\nUpdated {total} dependenc{}.", @@ -983,14 +998,7 @@ fn report_inherited_skips(manifest: &Path, report: &ManifestReport) { .results .iter() .filter(|result| { - result.item.source == PackageSource::Inherited - && matches!( - result.status, - DependencyStatus::PatchAvailable - | DependencyStatus::UpdateAvailable - | DependencyStatus::Outdated - | DependencyStatus::Vulnerable - ) + result.item.source == PackageSource::Inherited && result.status.has_update() }) .map(|result| result.item.name.as_str()) .collect(); @@ -1008,6 +1016,29 @@ fn report_inherited_skips(manifest: &Path, report: &ManifestReport) { ); } +/// Say which available updates this manifest's own constraints refused, and why. +/// +/// The sibling of [`report_inherited_skips`], for the other way `fix` can decline +/// an upgrade `check` just reported: there the version string lives in another +/// file, here it lives in a constraint that a concrete version would not +/// reproduce — a wildcard, a dist-tag, a two-bound range. Both are silent skips, +/// and silence is what makes the two commands look like they disagree. +/// +/// stderr, like its sibling: a note is not part of the record of what `fix` +/// changed, and piping stdout must not swallow it or mix it into that record. +fn report_declined_fixes(manifest: &Path, declined: &[fix::Declined]) { + for item in declined { + eprintln!( + "note: left {} = {} alone in {}: {} is available, but {}", + item.name, + item.constraint, + manifest.display(), + item.target, + item.reason.explain() + ); + } +} + /// Read whole-template overrides from `/dependable-templates/`. /// /// The directory is taken literally — no ancestor search, no `$XDG_CONFIG_HOME`, diff --git a/crates/dependable/tests/cli_fix.rs b/crates/dependable/tests/cli_fix.rs index 5cdc7d8..ada35eb 100644 --- a/crates/dependable/tests/cli_fix.rs +++ b/crates/dependable/tests/cli_fix.rs @@ -147,3 +147,290 @@ fn a_read_only_manifest_is_not_destroyed() { "a read-only manifest was truncated" ); } + +// --------------------------------------------------------------------------- +// Declined updates (issue #93) +// +// `check` reports an update, `fix` cannot rewrite the constraint that carries it, +// and until now `fix` said "Everything is already up to date." — the contradiction +// this section falsifies. Proving it needs a registry that actually offers a newer +// release, so these run against a throwaway HTTP server on loopback: hermetic, no +// dependency added, and the real fetch path rather than a stub of it. +// --------------------------------------------------------------------------- + +/// A single-shot registry: a path-to-JSON-body table served on loopback. +/// +/// Deliberately minimal rather than a mock-server crate. `dependable` has no +/// dev-dependencies at all, and the two registries these tests need — an npm +/// packument and a PyPI release map — are each one GET returning one document. +/// Every response closes the connection, so no keep-alive state has to be modelled. +fn registry(routes: Vec<(String, String)>) -> String { + use std::io::{BufRead as _, BufReader, Write as _}; + use std::net::TcpListener; + + let listener = TcpListener::bind("127.0.0.1:0").expect("bind a loopback port"); + let addr = listener.local_addr().expect("read the bound port"); + std::thread::spawn(move || { + for stream in listener.incoming() { + let Ok(mut stream) = stream else { continue }; + let routes = routes.clone(); + std::thread::spawn(move || { + let mut reader = BufReader::new(stream.try_clone().expect("clone the socket")); + let mut request = String::new(); + if reader.read_line(&mut request).is_err() { + return; + } + // Drain the headers so the client is never left writing into a + // socket nobody is reading, which some stacks report as a reset + // rather than as the response we are about to send. + let mut line = String::new(); + while reader.read_line(&mut line).is_ok_and(|n| n > 2) { + line.clear(); + } + let path = request.split_whitespace().nth(1).unwrap_or("").to_string(); + let body = routes + .iter() + .find(|(route, _)| *route == path) + .map(|(_, body)| body.clone()); + let response = match body { + Some(body) => format!( + "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: \ + {}\r\nConnection: close\r\n\r\n{body}", + body.len() + ), + None => "HTTP/1.1 404 Not Found\r\nContent-Length: 0\r\nConnection: \ + close\r\n\r\n" + .to_string(), + }; + let _ = stream.write_all(response.as_bytes()); + let _ = stream.flush(); + }); + } + }); + format!("http://{addr}") +} + +/// An npm abbreviated packument: the version keys and the `latest` dist-tag are +/// all the version checker reads. +fn packument(versions: &[&str], latest: &str) -> String { + let entries: Vec = versions + .iter() + .map(|v| format!("\"{v}\":{{\"name\":\"lodash\",\"version\":\"{v}\"}}")) + .collect(); + format!( + "{{\"name\":\"lodash\",\"dist-tags\":{{\"latest\":\"{latest}\"}},\"versions\":{{{}}}}}", + entries.join(",") + ) +} + +/// Point the ecosystem fetchers at `base` and switch OSV off, so a run touches +/// nothing but the loopback registry. +fn write_config(dir: &Path, base: &str) -> PathBuf { + let config = dir.join(".dependable.toml"); + fs::write( + &config, + format!( + "[npm]\nregistry = \"{base}\"\n\n[python]\nregistry = \"{base}/pypi\"\n\n\ + [vulnerability]\nenabled = false\n" + ), + ) + .unwrap(); + config +} + +fn run_with_config(dir: &Path, config: &Path, args: &[&str]) -> Output { + let mut command = Command::new(env!("CARGO_BIN_EXE_dependable")); + command + .arg("fix") + .arg(dir) + .arg("--config") + .arg(config) + .arg("--no-cache") + .arg("--no-vuln") + .args(args); + command.env_remove("DEPENDABLE_FAIL_ON"); + // A user `.npmrc` would override the configured registry and send the run at + // the real npm. + command.env("HOME", dir); + command.current_dir(dir); + command.output().expect("run dependable fix") +} + +/// Issue #93, exactly as reported: `"lodash": "1.x"` with `1.9.0` in range and +/// `2.0.0` published. `check` reports the update; `fix` cannot write it, because a +/// bare version in npm is one release and the author asked for a line of them. +/// Before this, the run printed "Everything is already up to date." over the top +/// of it and `--dry-run` printed nothing at all. +#[test] +fn a_declined_wildcard_is_reported_instead_of_claimed_up_to_date() { + let dir = workdir("fix_declined_wildcard"); + let base = registry(vec![( + "/lodash".to_string(), + packument(&["1.0.0", "1.9.0", "2.0.0"], "2.0.0"), + )]); + let config = write_config(&dir, &base); + let manifest = dir.join("package.json"); + let original = + "{\n \"name\": \"app\",\n \"dependencies\": {\n \"lodash\": \"1.x\"\n }\n}\n"; + fs::write(&manifest, original).unwrap(); + + let output = run_with_config(&dir, &config, &[]); + let stdout = String::from_utf8_lossy(&output.stdout).into_owned(); + let stderr = String::from_utf8_lossy(&output.stderr).into_owned(); + assert!(output.status.success(), "{stderr}"); + + assert!( + stderr.contains(&format!( + "note: left lodash = 1.x alone in {}: 1.9.0 is available, but a wildcard already \ + tracks new releases, and a bare version here would pin it to one", + manifest.display() + )), + "no note for the declined wildcard.\nstdout: {stdout}\nstderr: {stderr}" + ); + assert!( + !stdout.contains("Everything is already up to date."), + "fix claimed everything was up to date over an update it declined:\n{stdout}" + ); + assert!( + stdout.contains("Nothing to rewrite. 1 available update left alone"), + "stdout: {stdout}" + ); + // Declining is still declining: the constraint is untouched. + assert_eq!(fs::read_to_string(&manifest).unwrap(), original); +} + +/// `--dry-run` printed nothing whatsoever for the same manifest — the worst form +/// of the defect, because it is the mode people use to find out whether there is +/// anything to do. +#[test] +fn a_dry_run_reports_a_declined_wildcard_too() { + let dir = workdir("fix_declined_wildcard_dry"); + let base = registry(vec![( + "/lodash".to_string(), + packument(&["1.0.0", "1.9.0", "2.0.0"], "2.0.0"), + )]); + let config = write_config(&dir, &base); + fs::write( + dir.join("package.json"), + "{\n \"name\": \"app\",\n \"dependencies\": {\n \"lodash\": \"1.x\"\n }\n}\n", + ) + .unwrap(); + + let output = run_with_config(&dir, &config, &["--dry-run"]); + let stdout = String::from_utf8_lossy(&output.stdout).into_owned(); + let stderr = String::from_utf8_lossy(&output.stderr).into_owned(); + assert!(output.status.success(), "{stderr}"); + assert!( + stderr.contains("note: left lodash = 1.x alone in ") + && stderr.contains("package.json: 1.9.0 is available, but a wildcard"), + "stderr: {stderr}" + ); + assert!( + stdout.contains("Nothing to rewrite. 1 available update left alone"), + "stdout: {stdout}" + ); +} + +/// A dist-tag has been silent since long before the wildcard was. `"latest"` +/// resolves to the newest release, so the update only shows once a lockfile holds +/// an older one — and then `fix` must say why it will not pin the channel. +#[test] +fn a_declined_dist_tag_is_reported() { + let dir = workdir("fix_declined_dist_tag"); + let base = registry(vec![( + "/lodash".to_string(), + packument(&["1.0.0", "2.0.0"], "2.0.0"), + )]); + let config = write_config(&dir, &base); + fs::write( + dir.join("package.json"), + "{\n \"name\": \"app\",\n \"dependencies\": {\n \"lodash\": \"latest\"\n }\n}\n", + ) + .unwrap(); + fs::write( + dir.join("package-lock.json"), + "{\n \"lockfileVersion\": 3,\n \"packages\": {\n \"node_modules/lodash\": {\n \ + \"version\": \"1.0.0\"\n }\n }\n}\n", + ) + .unwrap(); + + let output = run_with_config(&dir, &config, &[]); + let stdout = String::from_utf8_lossy(&output.stdout).into_owned(); + let stderr = String::from_utf8_lossy(&output.stderr).into_owned(); + assert!(output.status.success(), "{stderr}"); + assert!( + stderr.contains("note: left lodash = latest alone in ") + && stderr.contains( + "package.json: 2.0.0 is available, but a dist-tag names a release channel, not \ + a version" + ), + "stdout: {stdout}\nstderr: {stderr}" + ); + assert!( + !stdout.contains("Everything is already up to date."), + "{stdout}" + ); +} + +/// A compound range: two bounds one version cannot carry. Python, because a +/// comma-separated range is the compound form the checker actually parses — an +/// npm space range is reported as an unreadable constraint and never reaches the +/// rewrite at all. +#[test] +fn a_declined_comma_range_is_reported() { + let dir = workdir("fix_declined_comma_range"); + let base = registry(vec![( + "/pypi/requests/json".to_string(), + "{\"releases\":{\"1.0.0\":[],\"1.9.0\":[],\"2.0.0\":[]}}".to_string(), + )]); + let config = write_config(&dir, &base); + let manifest = dir.join("requirements.txt"); + let original = "requests>=1.0,<2.0\n"; + fs::write(&manifest, original).unwrap(); + + let output = run_with_config(&dir, &config, &[]); + let stdout = String::from_utf8_lossy(&output.stdout).into_owned(); + let stderr = String::from_utf8_lossy(&output.stderr).into_owned(); + assert!(output.status.success(), "{stderr}"); + assert!( + stderr.contains("note: left requests = >=1.0,<2.0 alone in ") + && stderr.contains( + "requirements.txt: 1.9.0 is available, but a comma-separated range has two \ + bounds and one version cannot carry both" + ), + "stdout: {stdout}\nstderr: {stderr}" + ); + assert!( + !stdout.contains("Everything is already up to date."), + "{stdout}" + ); + assert_eq!(fs::read_to_string(&manifest).unwrap(), original); +} + +/// The line `fix` prints when there is genuinely nothing to do must survive: a +/// note-driven summary that fired on an ordinary up-to-date run would trade one +/// wrong message for another. +#[test] +fn a_run_with_no_declines_still_says_everything_is_up_to_date() { + let dir = workdir("fix_no_declines"); + let base = registry(vec![( + "/lodash".to_string(), + packument(&["1.0.0"], "1.0.0"), + )]); + let config = write_config(&dir, &base); + fs::write( + dir.join("package.json"), + "{\n \"name\": \"app\",\n \"dependencies\": {\n \"lodash\": \"1.0.0\"\n }\n}\n", + ) + .unwrap(); + + let output = run_with_config(&dir, &config, &[]); + let stdout = String::from_utf8_lossy(&output.stdout).into_owned(); + let stderr = String::from_utf8_lossy(&output.stderr).into_owned(); + assert!(output.status.success(), "{stderr}"); + assert!( + stdout.contains("Everything is already up to date."), + "stdout: {stdout}\nstderr: {stderr}" + ); + assert!(!stderr.contains("note: left"), "stderr: {stderr}"); +} From 9ced38d57f26207fd96c108519ec59b5e7f61f34 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Tue, 1 Sep 2026 17:45:11 -0400 Subject: [PATCH 4/5] test(report): give the SARIF uri fixtures a path Windows agrees is absolute `/elsewhere/Cargo.toml` is absolute on Unix and is not on Windows, where `Path::is_absolute` wants a drive prefix. So on Windows the two fixtures asserting the `file:` URI branch of `uri_for` were taking its *relative* branch and asserting the absolute branch's answer -- a deterministic failure that a stale cached test binary had been hiding on this stack, and that surfaced here only because touching `dependable-core` forced `dependable-report` to rebuild. Build the fixture path and its expected URI per platform instead, so the claim is made on both rather than gated off one. `uri_for` itself is unchanged: a real Windows path outside the root carries a drive, and `a_windows_path_keeps_its_drive_and_encodes_its_segments` already covers it. --- crates/dependable-report/src/sarif.rs | 32 +++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/crates/dependable-report/src/sarif.rs b/crates/dependable-report/src/sarif.rs index db1bb4a..9153ee3 100644 --- a/crates/dependable-report/src/sarif.rs +++ b/crates/dependable-report/src/sarif.rs @@ -964,6 +964,30 @@ mod tests { // -- A.4 ---------------------------------------------------------------- + /// A path that is absolute on the platform the test is running on. + /// + /// `/elsewhere/Cargo.toml` is absolute on Unix and *not* on Windows, where + /// [`Path::is_absolute`] wants a drive prefix. A fixture written that way + /// takes `uri_for`'s relative branch on Windows while asserting the absolute + /// branch's answer, so the assertion tests nothing there and fails. + fn outside_root(rest: &str) -> PathBuf { + if cfg!(windows) { + PathBuf::from(format!("C:\\{}", rest.replace('/', "\\"))) + } else { + PathBuf::from(format!("/{rest}")) + } + } + + /// The `file:` URI [`outside_root`] renders to, whose drive prefix Windows + /// keeps and Unix has none of. + fn outside_root_uri(rest: &str) -> String { + if cfg!(windows) { + format!("file:///C:/{rest}") + } else { + format!("file:///{rest}") + } + } + #[test] fn uri_is_relative_to_report_root_and_slash_joined() { let log = rendered(&report_at( @@ -982,12 +1006,12 @@ mod tests { // `uriBaseId`, which this log deliberately omits. let outside = rendered(&report_at( PathBuf::from("/repo"), - PathBuf::from("/elsewhere/Cargo.toml"), + outside_root("elsewhere/Cargo.toml"), vec![outdated()], )); assert_eq!( results_of(&outside)[0]["locations"][0]["physicalLocation"]["artifactLocation"]["uri"], - "file:///elsewhere/Cargo.toml" + outside_root_uri("elsewhere/Cargo.toml").as_str() ); // `.` components are normalized away even when the prefix does not strip. @@ -1325,8 +1349,8 @@ mod tests { "my%20app/Cargo.toml" ); assert_eq!( - uri_for(Path::new("/repo"), Path::new("/other dir/Cargo.toml")), - "file:///other%20dir/Cargo.toml" + uri_for(Path::new("/repo"), &outside_root("other dir/Cargo.toml")), + outside_root_uri("other%20dir/Cargo.toml") ); } From 7c8f3ab2ef3ccf5129f57d7a9eebd03654b7e83d Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Tue, 1 Sep 2026 17:59:52 -0400 Subject: [PATCH 5/5] test(report): restore the shared SARIF uri fixtures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 9ced38d57f26207fd96c108519ec59b5e7f61f34. The Windows failure it worked around has since been fixed at its cause. This branch was cut before `787480d`, which changed `uri_for` to ask `Path::has_root` rather than `Path::is_absolute` — and the difference between those two predicates is the entire reason `/elsewhere/Cargo.toml` took the relative branch on Windows while asserting the absolute branch's answer. It now takes the absolute branch on every platform, so the original fixtures make one claim that holds everywhere. Keeping the per-platform helper would cost coverage rather than add it. On Windows `outside_root` substitutes `C:\elsewhere\Cargo.toml`, a drive-absolute path, which means the rooted-but-drive-less case — precisely the case `787480d` repaired — would no longer be exercised on the one platform where it was ever broken. The drive-absolute form it substitutes instead is already asserted by `a_windows_path_keeps_its_drive_and_encodes_its_segments`. The helper's doc comment had also become false, and contradicted an assertion in the same file: it says `/elsewhere/Cargo.toml` "takes `uri_for`'s relative branch on Windows", while `787480d` added `uri_for(r"D:\repo", "/elsewhere/Cargo.toml") == "file:///elsewhere/Cargo.toml"` a few hundred lines below. Two contradictory statements about one input is worse than either alone. `crates/dependable-report/src/sarif.rs` is now identical to its state on the repaired base. --- crates/dependable-report/src/sarif.rs | 32 ++++----------------------- 1 file changed, 4 insertions(+), 28 deletions(-) diff --git a/crates/dependable-report/src/sarif.rs b/crates/dependable-report/src/sarif.rs index 0fab3b8..048bbfa 100644 --- a/crates/dependable-report/src/sarif.rs +++ b/crates/dependable-report/src/sarif.rs @@ -1010,30 +1010,6 @@ mod tests { // -- A.4 ---------------------------------------------------------------- - /// A path that is absolute on the platform the test is running on. - /// - /// `/elsewhere/Cargo.toml` is absolute on Unix and *not* on Windows, where - /// [`Path::is_absolute`] wants a drive prefix. A fixture written that way - /// takes `uri_for`'s relative branch on Windows while asserting the absolute - /// branch's answer, so the assertion tests nothing there and fails. - fn outside_root(rest: &str) -> PathBuf { - if cfg!(windows) { - PathBuf::from(format!("C:\\{}", rest.replace('/', "\\"))) - } else { - PathBuf::from(format!("/{rest}")) - } - } - - /// The `file:` URI [`outside_root`] renders to, whose drive prefix Windows - /// keeps and Unix has none of. - fn outside_root_uri(rest: &str) -> String { - if cfg!(windows) { - format!("file:///C:/{rest}") - } else { - format!("file:///{rest}") - } - } - #[test] fn uri_is_relative_to_report_root_and_slash_joined() { let log = rendered(&report_at( @@ -1052,12 +1028,12 @@ mod tests { // `uriBaseId`, which this log deliberately omits. let outside = rendered(&report_at( PathBuf::from("/repo"), - outside_root("elsewhere/Cargo.toml"), + PathBuf::from("/elsewhere/Cargo.toml"), vec![outdated()], )); assert_eq!( results_of(&outside)[0]["locations"][0]["physicalLocation"]["artifactLocation"]["uri"], - outside_root_uri("elsewhere/Cargo.toml").as_str() + "file:///elsewhere/Cargo.toml" ); // `.` components are normalized away even when the prefix does not strip. @@ -1489,8 +1465,8 @@ mod tests { "my%20app/Cargo.toml" ); assert_eq!( - uri_for(Path::new("/repo"), &outside_root("other dir/Cargo.toml")), - outside_root_uri("other%20dir/Cargo.toml") + uri_for(Path::new("/repo"), Path::new("/other dir/Cargo.toml")), + "file:///other%20dir/Cargo.toml" ); }