From a1f296999e7a581b7676871e24d5f55a7eb0e406 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Sun, 6 Sep 2026 12:33:19 -0400 Subject: [PATCH 1/5] feat(cli): advance a forced version on request, report it otherwise MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `plan_fixes` skipped every `overrides` / `resolutions` entry with an early `continue` placed before the has-update filter and before target selection. A forced version with a newer release waiting was therefore indistinguishable from one with nothing to do: it never reached `declined`, so nothing was reported, and a manifest whose only outdated entry was an override was answered with "Everything is already up to date." over the top of what `check` had just said. Move the decision after target selection and express it as a `Declined` with a new `DeclineReason::ForcedVersion`, so the existing reporting machinery carries it: `report_declined_fixes` prints the note and the summary counts it, unchanged. The note names a concrete available version, which is what makes the question it puts to the author — has this pin outlived its reason? — answerable. `fix --overrides` is how they answer yes. The default is still never to write over a forced version, `--all` included; the flag composes with `--all` on the axis `--all` already means, advancing within the declared constraint on its own and beyond it together. Constraint guards still fire first, so an override carrying a wildcard reports the wildcard rather than a flag that would leave it exactly where it is. A `$name` override reference stays unwritable and unreported: the parser records a zero-width span for it, so it fails `is_rewritable` before any of this. Closes #111 --- crates/dependable/src/cli.rs | 5 + crates/dependable/src/fix.rs | 336 +++++++++++++++++++++++++---- crates/dependable/src/runner.rs | 2 +- crates/dependable/tests/cli_fix.rs | 163 ++++++++++++++ 4 files changed, 469 insertions(+), 37 deletions(-) diff --git a/crates/dependable/src/cli.rs b/crates/dependable/src/cli.rs index 15d2d6d..19b0adb 100644 --- a/crates/dependable/src/cli.rs +++ b/crates/dependable/src/cli.rs @@ -221,6 +221,11 @@ pub struct FixArgs { /// Update all, including beyond the declared constraint. #[arg(long)] pub all: bool, + /// Also rewrite `overrides` / `resolutions` entries — versions this manifest + /// forces onto the resolved tree, often to hold a transitive dependency above + /// a vulnerable release. + #[arg(long)] + pub overrides: bool, /// Print what would change without writing. #[arg(long)] pub dry_run: bool, diff --git a/crates/dependable/src/fix.rs b/crates/dependable/src/fix.rs index 56a33e9..b5d6315 100644 --- a/crates/dependable/src/fix.rs +++ b/crates/dependable/src/fix.rs @@ -29,8 +29,12 @@ pub struct FixRecord { pub to: String, } -/// Why [`rewrite_constraint`] would not substitute a new version into a -/// constraint. +/// Why `fix` left an available update unwritten. +/// +/// Most reasons are a property of the *constraint*: [`rewrite_constraint`] would +/// not substitute a new version into it without changing what it admits. +/// [`ForcedVersion`](Self::ForcedVersion) is the one that is not — the constraint +/// there would take the rewrite, and it is the *entry* that declines it. /// /// 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 @@ -67,15 +71,23 @@ pub enum DeclineReason { /// A partial version, which is an X-range wherever a bare version is exact: /// npm's `"react": "16"`. PartialVersion, + /// Not the constraint at all: the entry is an `overrides` / `resolutions` + /// value, a version the manifest forces onto the resolved tree. `fix` moves + /// one only when `--overrides` asks it to. + ForcedVersion, } 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. + /// Every reason says what the entry *is*, not that a rule fired — the point + /// of the note is to let the author decide what to do about it, and a rule + /// name would not help them do that. For a constraint reason that means + /// deciding whether to widen the constraint by hand; for + /// [`ForcedVersion`](Self::ForcedVersion) the decision is whether the pin has + /// outlived its reason, so that clause names the flag that acts on the + /// answer. #[must_use] pub fn explain(self) -> &'static str { match self { @@ -108,13 +120,19 @@ impl DeclineReason { Self::PartialVersion => { "a partial version is an X-range that already tracks new releases" } + Self::ForcedVersion => { + "an override forces this version onto the resolved tree; pass --overrides to \ + advance it" + } } } } -/// An update `check` reports that `fix` will not write. +/// An update `check` reports that `fix` will not write: either a constraint +/// refused the rewrite, or the entry is a version this manifest forces onto the +/// resolved tree and no `--overrides` was given. /// -/// The whole point of recording it: without one, a declined constraint and a +/// The whole point of recording it: without one, a declined update 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. @@ -163,7 +181,8 @@ pub struct PlannedFix { /// /// Pinned (`=x.y.z`) deps are skipped unless `all` is set; multi-constraint forms /// (containing `,`) are skipped because they can't be rewritten to a single -/// version. +/// version. An `overrides` / `resolutions` entry is left alone — and reported — +/// unless `overrides` is set. /// /// Planning is separated from writing so a multi-manifest run can compute every rewrite /// before it writes any. Writing as it went left the tree half-rewritten when the third @@ -172,7 +191,12 @@ pub struct PlannedFix { /// # Errors /// Returns an error if the manifest cannot be read, or if a recorded span no longer /// holds the constraint it was planned against. -pub fn plan(manifest: &Path, results: &[CheckResult], all: bool) -> anyhow::Result { +pub fn plan( + manifest: &Path, + results: &[CheckResult], + all: bool, + overrides: bool, +) -> anyhow::Result { let content = std::fs::read_to_string(manifest) .with_context(|| format!("reading {}", manifest.display()))?; // What a rewritten constraint *means* is an ecosystem question, and the file @@ -180,7 +204,7 @@ 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, declined) = plan_fixes(&content, results, all, ecosystem) + let (updated, records, declined) = plan_fixes(&content, results, all, overrides, ecosystem) .with_context(|| format!("rewriting {}", manifest.display()))?; Ok(PlannedFix { path: manifest.to_path_buf(), @@ -242,16 +266,20 @@ pub fn commit(planned: &PlannedFix) -> anyhow::Result<()> { /// manifest kind was not recognized, which is treated as the most restrictive /// answer rather than as permission. /// +/// `overrides` opts into rewriting an `overrides` / `resolutions` entry, which is +/// otherwise reported and left in place; see the guard in the loop below. +/// /// 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. +/// every dependency with an update available that this loop did not write. 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, + overrides: bool, ecosystem: Option, ) -> anyhow::Result<(String, Vec, Vec)> { let mut edits: Vec = Vec::new(); @@ -266,20 +294,6 @@ fn plan_fixes( if !item.is_rewritable() { continue; } - // An override is a version this manifest deliberately forces onto the resolved - // tree — very often a security pin holding a transitive dependency above a - // vulnerable release. Reporting that a newer version exists is useful; rewriting - // the pin to it defeats the reason the entry was written, so `fix` declines the - // whole kind rather than trying to guess which overrides are safe to move. - // - // Skipped outright rather than recorded in `declined`: that list is for a - // rewrite a *constraint* refused, which the author could act on by widening it. - // An override is not rewritable by this tool at all, whatever it says, so a - // note offering to explain the refusal would be describing a decision the - // author cannot change and did not make. - if item.kind == DependencyKind::Override { - continue; - } if !result.status.has_update() || (item.is_pinned() && !all) { continue; } @@ -290,8 +304,26 @@ fn plan_fixes( result.latest_compatible.as_ref() }; let Some(target) = target else { continue }; + // An override is a version this manifest deliberately forces onto the resolved + // tree — very often a security pin holding a transitive dependency above a + // vulnerable release. Rewriting it to the newest release defeats the reason the + // entry was written, so the default is still never to write over one; `fix` + // moves it only when `--overrides` asks for it by name. + // + // Decided *here*, after target selection, and not as an early `continue` at the + // top of the loop: the note this produces names a concrete version that is + // actually available, so the question it puts to the author — has this pin + // outlived its reason? — is one they can answer. Skipped before that, an + // override with an update waiting was indistinguishable from one with nothing + // to do, which is the same contradiction with `check` that `declined` exists to + // remove. + let forced = item.kind == DependencyKind::Override && !overrides; let new_constraint = match rewrite_constraint(&item.version_constraint, target, ecosystem) { Ok(new_constraint) => new_constraint, + // Before the `forced` guard, deliberately: a constraint that refuses the + // rewrite refuses it whether or not `--overrides` was passed, so an override + // carrying a wildcard reports the wildcard — the reason that would still + // stand with the flag turned on — rather than a flag that would not help. Err(reason) => { declined.push(Declined { name: item.name.clone(), @@ -307,6 +339,15 @@ fn plan_fixes( if new_constraint == item.version_constraint { continue; } + if forced { + declined.push(Declined { + name: item.name.clone(), + constraint: item.version_constraint.clone(), + target: target.clone(), + reason: DeclineReason::ForcedVersion, + }); + continue; + } edits.push(Edit { line: item.version_line, @@ -590,6 +631,7 @@ mod tests { content, &results, true, + false, Some(ManifestKind::PackageJson.ecosystem()), ) .expect("the plan applies"); @@ -608,18 +650,228 @@ mod tests { updated.contains(r#""minimist": "1.2.6""#), "the override was rewritten: {updated}" ); - // And it is skipped *silently*. A `Declined` says a constraint refused a - // rewrite the author could permit by widening it; an override refuses for a - // reason that has nothing to do with its constraint and that no edit to the - // constraint would change, so reporting one here would tell the author to go - // fix a string that is not the problem. + // And it is left alone *out loud* (#111). It used to be skipped before the + // update filter ever ran, which made an override with a newer release waiting + // indistinguishable from one with nothing to do — so `fix` answered "everything + // is already up to date" over a version `check` had just reported. The note is + // what the author needs to decide whether the pin has outlived its reason. assert_eq!( declined, - [], - "the override was reported as a declined update" + [Declined { + name: "minimist".to_string(), + constraint: "1.2.6".to_string(), + target: "1.2.8".to_string(), + reason: DeclineReason::ForcedVersion, + }], + "the forced version was not reported" + ); + } + + /// The note claims a version is available. An override already at the newest + /// release has none, so it must not produce one — otherwise every pinned + /// override in the tree reports itself on every run. + #[test] + fn an_override_with_nothing_available_is_not_a_decline() { + let content = r#"{ + "overrides": { + "minimist": "1.2.6" + } +} +"#; + // The same version the override already forces: `plan_fixes` reaches the + // rewrite, produces the constraint that is already there, and stops. + let results = results_for(ManifestKind::PackageJson, content, &[("minimist", "1.2.6")]); + assert_eq!(results.len(), 1, "the fixture must produce one item"); + + let (updated, records, declined) = plan_fixes( + content, + &results, + true, + false, + Some(ManifestKind::PackageJson.ecosystem()), + ) + .expect("the plan applies"); + assert_eq!(updated, content); + assert!(records.is_empty(), "{records:?}"); + assert!(declined.is_empty(), "{declined:?}"); + } + + /// The ordering rule: a constraint that refuses the rewrite refuses it with or + /// without `--overrides`, so it — and not the flag — is what the note names. + /// Reporting `ForcedVersion` here would point the author at a flag that would + /// leave the wildcard exactly where it is. + #[test] + fn an_override_whose_constraint_also_refuses_reports_the_constraint() { + let content = r#"{ + "resolutions": { + "lodash": "1.x" + } +} +"#; + let results = results_for(ManifestKind::PackageJson, content, &[("lodash", "1.9.0")]); + assert_eq!(results.len(), 1, "the fixture must produce one item"); + assert_eq!(results[0].item.kind, DependencyKind::Override); + + let (_, records, declined) = plan_fixes( + content, + &results, + false, + false, + Some(ManifestKind::PackageJson.ecosystem()), + ) + .expect("the plan applies"); + assert!(records.is_empty(), "{records:?}"); + assert_eq!( + declined.iter().map(|item| item.reason).collect::>(), + [DeclineReason::WildcardPins], + "{declined:?}" ); } + /// …and it still holds when the flag *is* set: `--overrides` lifts the kind + /// guard, not the constraint guards. + #[test] + fn a_requested_override_still_obeys_its_constraint() { + let content = r#"{ + "resolutions": { + "lodash": "1.x" + } +} +"#; + let results = results_for(ManifestKind::PackageJson, content, &[("lodash", "1.9.0")]); + + let (updated, records, declined) = plan_fixes( + content, + &results, + false, + true, + Some(ManifestKind::PackageJson.ecosystem()), + ) + .expect("the plan applies"); + assert_eq!(updated, content, "the wildcard was pinned"); + assert!(records.is_empty(), "{records:?}"); + assert_eq!( + declined.iter().map(|item| item.reason).collect::>(), + [DeclineReason::WildcardPins], + "{declined:?}" + ); + } + + /// A `$name` override is a reference to another entry's constraint, and the + /// parser records a zero-width span for it precisely so nothing can splice a + /// version over the reference. It is therefore not a decline either: `fix` + /// declining to advance it would offer a flag that still could not write it. + #[test] + fn a_dollar_reference_override_is_never_a_decline_and_never_rewritten() { + let content = r#"{ + "dependencies": { + "semver": "^7.5.0" + }, + "overrides": { + "semver": "$semver" + } +} +"#; + let results = results_for(ManifestKind::PackageJson, content, &[("semver", "7.6.0")]); + assert!( + results + .iter() + .any(|r| r.item.kind == DependencyKind::Override && !r.item.is_rewritable()), + "the fixture must produce an unrewritable override" + ); + + let (updated, records, declined) = plan_fixes( + content, + &results, + true, + true, + Some(ManifestKind::PackageJson.ecosystem()), + ) + .expect("the plan applies"); + assert!( + updated.contains(r#""semver": "$semver""#), + "the reference was overwritten: {updated}" + ); + assert_eq!( + records + .iter() + .map(|record| record.name.as_str()) + .collect::>(), + ["semver"], + "only the declaration the reference points at is rewritable: {records:?}" + ); + assert!(declined.is_empty(), "{declined:?}"); + } + + /// The whole point of the flag: asked for by name, the forced version moves. + #[test] + fn an_override_is_rewritten_when_overrides_are_requested() { + let content = r#"{ + "dependencies": { + "monolog": "^2.0" + }, + "overrides": { + "minimist": "1.2.6" + } +} +"#; + let results = results_for( + ManifestKind::PackageJson, + content, + &[("minimist", "1.2.8"), ("monolog", "2.9.1")], + ); + + let (updated, records, declined) = plan_fixes( + content, + &results, + true, + true, + Some(ManifestKind::PackageJson.ecosystem()), + ) + .expect("the plan applies"); + assert!( + updated.contains(r#""minimist": "1.2.8""#), + "the override was not advanced: {updated}" + ); + let mut names: Vec<&str> = records.iter().map(|record| record.name.as_str()).collect(); + names.sort_unstable(); + assert_eq!(names, ["minimist", "monolog"], "{records:?}"); + assert!(declined.is_empty(), "{declined:?}"); + } + + /// A pnpm key scopes an override to the parent that pulls the package in: + /// `"foo@2>bar"` forces a version onto **bar**. The rewrite must land on that + /// entry's own value span — the defect the kind guard originally hid was a + /// rewrite aimed at an unrelated package's newest release. + #[test] + fn a_scoped_pnpm_override_rewrites_the_package_it_names() { + let content = r#"{ + "pnpm": { + "overrides": { + "foo@2>bar": "1.0.0" + } + } +} +"#; + let results = results_for(ManifestKind::PackageJson, content, &[("bar", "1.5.0")]); + assert_eq!(results.len(), 1, "the fixture must produce one item"); + assert_eq!(results[0].item.name, "bar"); + + let (updated, records, _declined) = plan_fixes( + content, + &results, + true, + true, + Some(ManifestKind::PackageJson.ecosystem()), + ) + .expect("the plan applies"); + assert!( + updated.contains(r#""foo@2>bar": "1.5.0""#), + "the value span was not the one rewritten: {updated}" + ); + assert_eq!(records.len(), 1, "{records:?}"); + } + /// Every ecosystem, so a guard that is ecosystem-independent is asserted /// against all of them rather than against a convenient one — and so a new /// variant cannot quietly opt out of a claim made here. @@ -1112,6 +1364,7 @@ mod tests { content, &results, false, + false, Some(ManifestKind::PackageJson.ecosystem()), ) .expect("the plan applies"); @@ -1150,6 +1403,7 @@ mod tests { content, &results_for(ManifestKind::PackageJson, content, &[]), false, + false, Some(ManifestKind::PackageJson.ecosystem()), ) .expect("the plan applies"); @@ -1249,6 +1503,7 @@ mod tests { content, &results, false, + false, Some(ManifestKind::PackageJson.ecosystem()), ) .expect("the plan applies"); @@ -1293,6 +1548,7 @@ mod tests { content, &results, false, + false, Some(ManifestKind::ComposerJson.ecosystem()), ) .expect("the plan applies"); @@ -1325,6 +1581,7 @@ mod tests { content, &results, false, + false, Some(ManifestKind::PubspecYaml.ecosystem()), ) .expect("the plan applies"); @@ -1375,6 +1632,7 @@ mod tests { member, &results, false, + false, Some(ManifestKind::CargoToml.ecosystem()), ) .expect("the plan applies"); @@ -1410,6 +1668,7 @@ mod tests { root, &results, false, + false, Some(ManifestKind::CargoToml.ecosystem()), ) .expect("the plan applies"); @@ -1492,6 +1751,7 @@ mod tests { content, &results, false, + false, Some(ManifestKind::PackageJson.ecosystem()), ) .expect("the plan applies"); @@ -1534,6 +1794,7 @@ mod tests { content, &results, true, + false, Some(ManifestKind::ComposerJson.ecosystem()), ) .expect("the plan applies"); @@ -1585,6 +1846,7 @@ mod tests { content, &results, false, + false, Some(ManifestKind::CargoToml.ecosystem()), ) .expect("the plan applies"); @@ -1617,6 +1879,7 @@ mod tests { content, &results, false, + false, Some(ManifestKind::PackageJson.ecosystem()), ) .expect("the plan applies"); @@ -1645,6 +1908,7 @@ mod tests { content, &results, false, + false, Some(ManifestKind::PackageJson.ecosystem()), ) .expect("the plan applies"); diff --git a/crates/dependable/src/runner.rs b/crates/dependable/src/runner.rs index 117f929..1318f98 100644 --- a/crates/dependable/src/runner.rs +++ b/crates/dependable/src/runner.rs @@ -915,7 +915,7 @@ pub async fn run_fix(args: FixArgs) -> anyhow::Result { continue; }; report_inherited_skips(manifest, &report); - let plan = fix::plan(manifest, &report.results, args.all)?; + let plan = fix::plan(manifest, &report.results, args.all, args.overrides)?; report_declined_fixes(manifest, &plan.declined); planned.push(plan); } diff --git a/crates/dependable/tests/cli_fix.rs b/crates/dependable/tests/cli_fix.rs index ada35eb..0c6cd6b 100644 --- a/crates/dependable/tests/cli_fix.rs +++ b/crates/dependable/tests/cli_fix.rs @@ -434,3 +434,166 @@ fn a_run_with_no_declines_still_says_everything_is_up_to_date() { ); assert!(!stderr.contains("note: left"), "stderr: {stderr}"); } + +// --------------------------------------------------------------------------- +// Forced versions (issue #111) +// +// An `overrides` / `resolutions` entry forces a version onto the resolved tree. +// `fix` skipped the whole kind before it ever looked at whether an update was +// waiting, so a stale forced version was indistinguishable from one with nothing +// to do — `check` reported it and `fix` answered "Everything is already up to +// date." The default is still never to write over one; `--overrides` is how the +// author asks. +// --------------------------------------------------------------------------- + +/// A `package.json` forcing `lodash` to `1.0.0`, beside an ordinary dependency +/// that is already current so it contributes nothing to the counts below. +const FORCED_VERSION_MANIFEST: &str = "{\n \"name\": \"app\",\n \"dependencies\": {\n \ + \"react\": \"^18.0.0\"\n },\n \"overrides\": {\n \ + \"lodash\": \"1.0.0\"\n }\n}\n"; + +/// `lodash` with a newer patch line and a newer major, and a `react` that has +/// only the release already declared. Written out rather than built by +/// [`packument`], which names `lodash` in every entry it writes. +fn forced_version_routes() -> Vec<(String, String)> { + vec![ + ( + "/lodash".to_string(), + packument(&["1.0.0", "1.9.0", "2.0.0"], "2.0.0"), + ), + ( + "/react".to_string(), + "{\"name\":\"react\",\"dist-tags\":{\"latest\":\"18.0.0\"},\"versions\":\ + {\"18.0.0\":{\"name\":\"react\",\"version\":\"18.0.0\"}}}" + .to_string(), + ), + ] +} + +/// The defect as reported: an override with a newer release available produced no +/// output of any kind, and the run claimed to be up to date over the top of it. +#[test] +fn a_forced_version_is_reported_instead_of_silently_skipped() { + let dir = workdir("fix_forced_version_reported"); + let base = registry(forced_version_routes()); + let config = write_config(&dir, &base); + let manifest = dir.join("package.json"); + fs::write(&manifest, FORCED_VERSION_MANIFEST).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.0.0 alone in {}", + manifest.display() + )) && stderr.contains( + "1.9.0 is available, but an override forces this version onto the resolved \ + tree; pass --overrides to advance it" + ), + "no note for the forced version.\nstdout: {stdout}\nstderr: {stderr}" + ); + assert!( + !stdout.contains("Everything is already up to date."), + "fix claimed everything was up to date over a forced version it left alone:\n{stdout}" + ); + assert!( + stdout.contains("Nothing to rewrite. 1 available update left alone"), + "stdout: {stdout}" + ); + // Reporting is not rewriting: the pin the author wrote is still exactly there. + assert_eq!( + fs::read_to_string(&manifest).unwrap(), + FORCED_VERSION_MANIFEST + ); +} + +/// `--all` reaches beyond the declared constraint, and must still stop at a forced +/// version: it is the flag most likely to be aimed at a tree full of security pins. +#[test] +fn fix_all_still_leaves_a_forced_version_alone() { + let dir = workdir("fix_forced_version_all"); + let base = registry(forced_version_routes()); + let config = write_config(&dir, &base); + let manifest = dir.join("package.json"); + fs::write(&manifest, FORCED_VERSION_MANIFEST).unwrap(); + + let output = run_with_config(&dir, &config, &["--all"]); + 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.0.0 alone in ") + && stderr.contains("2.0.0 is available, but an override forces this version"), + "stdout: {stdout}\nstderr: {stderr}" + ); + assert_eq!( + fs::read_to_string(&manifest).unwrap(), + FORCED_VERSION_MANIFEST, + "--all rewrote a forced version" + ); +} + +/// Asked for by name, the forced version moves — and only then. +#[test] +fn overrides_are_rewritten_when_asked_for() { + let dir = workdir("fix_forced_version_requested"); + let base = registry(forced_version_routes()); + let config = write_config(&dir, &base); + let manifest = dir.join("package.json"); + fs::write(&manifest, FORCED_VERSION_MANIFEST).unwrap(); + + let output = run_with_config(&dir, &config, &["--overrides", "--all"]); + 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}"); + + let written = fs::read_to_string(&manifest).unwrap(); + assert!( + written.contains("\"lodash\": \"2.0.0\""), + "the forced version was not advanced: {written}" + ); + // One span, in place: the neighbouring dependency and the formatting are not + // this command's to touch. + assert_eq!( + written, + FORCED_VERSION_MANIFEST.replace("\"lodash\": \"1.0.0\"", "\"lodash\": \"2.0.0\""), + "more than the override's value span changed" + ); + assert!(stdout.contains("Updated 1 dependency."), "stdout: {stdout}"); + assert!( + !stderr.contains("note: left lodash"), + "a rewritten override was also reported as left alone: {stderr}" + ); +} + +/// `--overrides` is the destructive flag in this command, so the mode people use +/// to find out what it would do must still write nothing. +#[test] +fn a_requested_override_rewrite_honours_dry_run() { + let dir = workdir("fix_forced_version_dry_run"); + let base = registry(forced_version_routes()); + let config = write_config(&dir, &base); + let manifest = dir.join("package.json"); + fs::write(&manifest, FORCED_VERSION_MANIFEST).unwrap(); + let before = fs::metadata(&manifest).unwrap().modified().unwrap(); + + let output = run_with_config(&dir, &config, &["--overrides", "--all", "--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!( + stdout.contains("lodash 1.0.0 → 2.0.0"), + "the dry run did not say what it would do: {stdout}" + ); + assert_eq!( + fs::read_to_string(&manifest).unwrap(), + FORCED_VERSION_MANIFEST, + "a dry run rewrote a forced version" + ); + assert_eq!(fs::metadata(&manifest).unwrap().modified().unwrap(), before); +} From f956f6521c7e65ac3e38fae03371275aa48228e4 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Sun, 6 Sep 2026 12:34:45 -0400 Subject: [PATCH 2/5] docs: document forced versions and `fix --overrides` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `overrides` / `resolutions` maps had no entry in the README at all, so neither the default — `fix` never writes over a forced version, `--all` included — nor the note it now prints instead was written down anywhere a user would look. Says which maps count, that they are npm-family `package.json` only, how `--overrides` composes with `--all` and `--dry-run`, and why to find out what a pin is for before advancing it. --- README.md | 45 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/README.md b/README.md index 3b5bda3..6453570 100644 --- a/README.md +++ b/README.md @@ -333,6 +333,51 @@ by walking up rather than by anything the caller spelled, and a relative answer relative to a directory the caller never named. A dependency the root turns out not to declare gets no attribution at all, and a warning saying so. +### Forced versions (overrides and resolutions) + +An npm-family `package.json` can force a version onto the *resolved* tree, past +whatever the packages in it asked for: npm's `overrides`, Yarn's `resolutions`, +and `pnpm.overrides` — including npm's nested form (`"overrides": { "parent": +{ "child": "…" } }`) and pnpm's scoped keys (`"foo@2>bar"`, which forces a version +onto **bar**). These are the only maps `dependable` treats this way; nothing in +Cargo, Go, Python, or the rest declares one, and `pnpm-workspace.yaml` overrides +are not read at all. + +A forced version is usually there for a reason — most often a security pin, +holding a transitive dependency above a vulnerable release — and the tool cannot +tell that from a compatibility pin that has outlived its cause. So **`fix` never +rewrites one by default**, `--all` included. It says so instead: + +``` +$ dependable fix . +note: left lodash = 1.0.0 alone in package.json: 1.9.0 is available, but an override + forces this version onto the resolved tree; pass --overrides to advance it +Nothing to rewrite. 1 available update left alone; see the notes above. +``` + +That note is the point: a stale pin used to be skipped in silence, so a manifest +whose only outdated entry was an override was reported by `check` and then +answered by `fix` with "Everything is already up to date." + +`--overrides` is how you say yes: + +```bash +dependable fix . --overrides # advance forced versions within their constraint +dependable fix . --overrides --all # …and beyond it, like --all everywhere else +dependable fix . --overrides --dry-run # see it first; nothing is written +``` + +Before you reach for it, check *why* each pin is there — advancing a security pin +past the release it was holding the tree above puts the vulnerability back. +`--dry-run` prints every rewrite it would make without touching a file. + +Constraint rules still apply on top: `--overrides` lifts the rule about the kind +of entry, not the rules about what a constraint means. An override written as a +wildcard (`"resolutions": { "lodash": "1.x" }`) is still left alone and still +reported, because pinning an npm wildcard to one release changes what the entry +admits. An override written as a `$name` reference to another entry is never +rewritten either — the version it names lives in the entry it points at. + ## Project inventory (`list`) `dependable list` answers "what lives in this repository" — every manifest it From 55220b8a489f912b26a9be56706155520031edd1 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Sun, 6 Sep 2026 13:51:04 -0400 Subject: [PATCH 3/5] docs: narrow the forced-version guarantee to the manifests it covers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The forced-versions section claimed npm-family `overrides` / `resolutions` were "the only maps `dependable` treats this way; nothing in Cargo, Go, Python, or the rest declares one", and that `fix` never rewrites one by default. The second half is false outside `package.json`. A Gradle version catalog's rich versions are read for their version string, and `strictly` — Gradle's pinning form, and the usual shape of a JVM security pin — is recorded with `DependencyKind::Normal` and a real span. The `forced` guard tests `kind == Override`, which only the `package.json` parser ever sets, so a plain `fix` with no flags advances a `strictly` pin. That defect is issue #147 and is not fixed here; the README now says so instead of promising otherwise. Cargo's `[patch]` / `[replace]`, Composer's `replace` / `conflict`, and `pnpm-workspace.yaml`'s `overrides:` are named separately, because they are safe for a different reason: no parser reads them at all. Implying a deliberate guard where there is only an absence would misdescribe what protects them. Two more corrections in the same section. The safety warning stated the inverse of the hazard — an override holds a dependency *above* a vulnerable release, so advancing it further above is not how the vulnerability comes back. It now names the four things the flag cannot work out: which release the pin was chosen for, whether the target is any safer (`all_vulnerabilities` is declared and populated nowhere, so advisories are only known for the version already declared), which direction the pin points, and whether its upper bound was the point. And `--overrides` is no longer described as advancing "within their constraint": the requirement is built with `VersionReq`, which reads a bare `1.0.0` as a caret while npm — and `Ecosystem::bare_version` — read it as exact, so the honest bound is the range the tool reads. That disagreement is issue #118 and belongs there, since it moves ordinary dependencies too. --- README.md | 57 +++++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 49 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 0101e07..f3d3663 100644 --- a/README.md +++ b/README.md @@ -339,14 +339,29 @@ An npm-family `package.json` can force a version onto the *resolved* tree, past whatever the packages in it asked for: npm's `overrides`, Yarn's `resolutions`, and `pnpm.overrides` — including npm's nested form (`"overrides": { "parent": { "child": "…" } }`) and pnpm's scoped keys (`"foo@2>bar"`, which forces a version -onto **bar**). These are the only maps `dependable` treats this way; nothing in -Cargo, Go, Python, or the rest declares one, and `pnpm-workspace.yaml` overrides -are not read at all. +onto **bar**). Those maps are the only ones any parser tags as a forced version, +so **`package.json` is the only manifest this section's guarantee covers.** + +Other ecosystems have forcing mechanisms of their own. None of them is covered, +and the two reasons for that are not the same — which is the part worth knowing: + +- **Never read, so nothing can rewrite them.** Cargo's `[patch]` and `[replace]`, + Composer's `replace` and `conflict`, and `pnpm-workspace.yaml`'s `overrides:` + (only its `catalog:` and `catalogs:` maps are read) are absent from every + parser. They are safe because nothing looks at them, not because anything + protects them. +- **Read, but not yet recognised as forced.** A Gradle version catalog's rich + versions are read for their version string, and `strictly` — Gradle's pinning + form, and the usual shape of a JVM security pin — is recorded as an ordinary + constraint. A plain `dependable fix`, with no flags, will advance one. That is + [issue #147](https://github.com/getkono/dependable/issues/147), and until it is + fixed a Gradle `strictly` pin gets none of the protection described below. A forced version is usually there for a reason — most often a security pin, holding a transitive dependency above a vulnerable release — and the tool cannot -tell that from a compatibility pin that has outlived its cause. So **`fix` never -rewrites one by default**, `--all` included. It says so instead: +tell that from a compatibility pin that has outlived its cause. So for the +entries it does recognise, **`fix` never rewrites one by default**, `--all` +included. It says so instead: ``` $ dependable fix . @@ -362,13 +377,39 @@ answered by `fix` with "Everything is already up to date." `--overrides` is how you say yes: ```bash -dependable fix . --overrides # advance forced versions within their constraint +dependable fix . --overrides # advance forced versions within the range the tool reads dependable fix . --overrides --all # …and beyond it, like --all everywhere else dependable fix . --overrides --dry-run # see it first; nothing is written ``` -Before you reach for it, check *why* each pin is there — advancing a security pin -past the release it was holding the tree above puts the vulnerability back. +"Within the range the tool reads" is the honest boundary, and for npm it is not +always the range npm reads. The requirement is built with Cargo's `VersionReq`, +which takes a bare `1.0.0` as `^1.0.0` — so `"overrides": { "lodash": "1.0.0" }` +is advanced to `1.9.0` by `--overrides` alone, while npm reads that same string as +exactly `1.0.0`, which is what `Ecosystem::bare_version` records for it. The two +readings disagree, and closing that gap is +[issue #118](https://github.com/getkono/dependable/issues/118) — it changes how +every bare version is read, not just a forced one, so it is not settled here. A +forced version spelled as a range (`"^1.0.0"`) or as an explicit pin (`"=1.0.0"`) +carries no such ambiguity: the first advances within the range, the second needs +`--all` like any other pin. + +Before you reach for it, check *why* each pin is there. `--overrides` is the +destructive flag in this command, and here is what it cannot work out for you: + +- **Which release the pin was chosen for.** A forced version carries no record of + its reason, so whether the pin has outlived it is a question only the author can + answer. +- **Whether the release it advances to is any safer.** + `CheckResult::all_vulnerabilities` is declared but nothing populates it yet, so + advisories are only known for the version currently declared. A pin can be moved + from one vulnerable release to another with no signal at all. +- **Which direction the pin points.** A pnpm override can hold a package *below* a + release — a regression, a breaking change — and advancing that one walks straight + into what it was written to avoid. +- **Whether the upper bound was the point.** For a compatibility pin it usually is, + and raising it is the whole of the damage. + `--dry-run` prints every rewrite it would make without touching a file. Constraint rules still apply on top: `--overrides` lifts the rule about the kind From b6a522d9d8bcb49be5fd423391a03836af644569 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Sun, 6 Sep 2026 13:51:12 -0400 Subject: [PATCH 4/5] docs(cli): say the pin note names the next flag, not one that acts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `Pinned` guard's comment claimed that for an override written as an explicit pin, "`--all` is the flag that would move it" and "every note names a flag that acts". The test directly beneath it, `an_override_that_is_also_a_pin_reports_the_pin`, asserts the opposite: with `--all` alone the pin guard passes and the `forced` guard reports a second note naming `--overrides`. Neither flag on its own moves that entry. The ordering the comment explains is right; only the claim about it was wrong. Each note names the next flag that has to be lifted, and the two are disclosed one at a time — which is what makes each note true at the moment it is printed. --- crates/dependable/src/fix.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/crates/dependable/src/fix.rs b/crates/dependable/src/fix.rs index 2add654..9486a37 100644 --- a/crates/dependable/src/fix.rs +++ b/crates/dependable/src/fix.rs @@ -367,9 +367,14 @@ fn plan_fixes( // action the note points at. // // Ahead of the `forced` guard below, and so this is also the reason an - // `overrides` entry written as an explicit pin (`=1.2.3`) reports: `--all` - // is the flag that would move it, and `--overrides` on its own would still - // leave it exactly where it is. Every note names a flag that acts. + // `overrides` entry written as an explicit pin (`=1.2.3`) reports the pin: + // `--overrides` on its own would still leave it exactly where it is, so + // the flag worth naming is the next one that has to be lifted, not one + // that finishes the job. For that entry it takes both, and the notes + // disclose them one at a time — with `--all` the pin guard passes and the + // `forced` guard below names `--overrides` in turn. Each note is true at + // the moment it is printed, which is what + // `an_override_that_is_also_a_pin_reports_the_pin` walks through. if item.is_pinned() && !all { declined.push(Declined { name: item.name.clone(), From 718a4ec01ff7e5940f3da83c6e93f5ce3435c806 Mon Sep 17 00:00:00 2001 From: Justin Chung Date: Sun, 6 Sep 2026 13:51:22 -0400 Subject: [PATCH 5/5] test(cli): cover advancing an override without `--all` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--overrides` on its own is the invocation the README recommends first, and no test exercised it producing a rewrite. Every case that produced a `FixRecord` passed `all = true`; the two that passed `all = false, overrides = true` both ended in a decline, so the write path for the recommended combination was unpinned. The fixture is a range-form override, `"^1.0.0"`, which is the honest shape for "advances within its constraint": the range admits `1.9.0` and refuses `2.0.0`. `latest_available` is set past `latest_compatible` on purpose — with the two equal, a path that ignored the compatible target would pass unchanged — and the test asserts both that the rewrite lands as `^1.9.0` and that nothing reached `2.0.0`. --- crates/dependable/src/fix.rs | 61 ++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/crates/dependable/src/fix.rs b/crates/dependable/src/fix.rs index 9486a37..015906a 100644 --- a/crates/dependable/src/fix.rs +++ b/crates/dependable/src/fix.rs @@ -1147,6 +1147,67 @@ mod tests { assert!(declined.is_empty(), "{declined:?}"); } + /// `--overrides` *without* `--all`, which is the combination the README + /// recommends first and the one every other test here reaches only to watch + /// it decline. The write path for it was unpinned: each of the two cases + /// passing `all = false, overrides = true` ends in a decline, and every case + /// that produces a record passes `all = true`. + /// + /// A range-form override is the honest fixture for "advances within its + /// constraint": `^1.0.0` admits `1.9.0` and refuses `2.0.0`, so the target + /// this run picks is visible in the result rather than assumed. Written with + /// `latest_available` deliberately *past* `latest_compatible` — with the two + /// equal, an `--all` path that ignored the compatible target would pass this + /// test unchanged. + #[test] + fn an_override_advances_within_its_range_without_all() { + let content = r#"{ + "overrides": { + "lodash": "^1.0.0" + } +} +"#; + let mut results = results_for(ManifestKind::PackageJson, content, &[("lodash", "1.9.0")]); + assert_eq!(results.len(), 1, "the fixture must produce one item"); + assert_eq!(results[0].item.kind, DependencyKind::Override); + assert!( + !results[0].item.is_pinned(), + "a range-form override must not be a pin, or the pin guard answers first" + ); + results[0].latest_available = Some("2.0.0".to_string()); + + let (updated, records, declined) = plan_fixes( + content, + &results, + false, + true, + Some(ManifestKind::PackageJson.ecosystem()), + ) + .expect("the plan applies"); + + assert!( + updated.contains(r#""lodash": "^1.9.0""#), + "the override was not advanced without `--all`: {updated}" + ); + assert!( + !updated.contains("2.0.0"), + "`--overrides` alone reached past the constraint: {updated}" + ); + assert_eq!( + records + .iter() + .map(|record| ( + record.name.as_str(), + record.from.as_str(), + record.to.as_str() + )) + .collect::>(), + [("lodash", "^1.0.0", "^1.9.0")], + "{records:?}" + ); + assert!(declined.is_empty(), "{declined:?}"); + } + /// A pnpm key scopes an override to the parent that pulls the package in: /// `"foo@2>bar"` forces a version onto **bar**. The rewrite must land on that /// entry's own value span — the defect the kind guard originally hid was a