fix(cli): report the updates fix declined instead of claiming none exist - #108
Open
justin13888 wants to merge 7 commits into
Open
fix(cli): report the updates fix declined instead of claiming none exist#108justin13888 wants to merge 7 commits into
justin13888 wants to merge 7 commits into
Conversation
`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.
`rewrite_constraint` returned `Option<String>`, 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<String, DeclineReason>` 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.
`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
…solute `/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.
…t-declined-updates
Brings forward both the ecosystem-aware wildcard work this branch sits on and,
through it, the ten commits that repaired its base — nine of them fixes for
defects an adversarial review confirmed.
`crates/dependable/src/fix.rs` was the only conflict, in two hunks:
- The imports. The base needs `DependencyKind` for the override guard; this
branch had already dropped `DependencyStatus` when `has_update()` replaced
the inline `matches!`. Kept as `{CheckResult, DependencyKind}` — the test
module imports `DependencyStatus` for itself.
- The `plan_fixes` guards. The base added an override skip and this branch
replaced the `updatable` `matches!` with `status.has_update()`; the two
changes are independent and both are kept. The override skip stays first, so
an override never reaches `rewrite_constraint` at all.
The override is skipped without recording a `Declined`, and
`fix_all_leaves_an_override_alone` now asserts that emptiness alongside its
existing claims. A `Declined` reports a constraint that refused a rewrite, and
its note invites the author to widen that constraint. An override refuses for
a reason its constraint has no part in and that no edit to the constraint
would change, so a note there would point the author at a string that is not
the problem.
This reverts commit 9ced38d. 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.
justin13888
force-pushed
the
fix/93-report-declined-updates
branch
from
September 1, 2026 22:04
7bd51d9 to
7c8f3ab
Compare
…card' into fix/93-report-declined-updates
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
dependable checkreported an available update to"lodash": "1.x"whiledependable fixprintedEverything is already up to date.for the same manifest, andfix --dry-runprinted nothing at all.rewrite_constraintdeclined the wildcard,plan_fixesemitted noFixRecord, andrun_fixhad only two outcomes — records printed, or the flat "already up to date" line — so a declined-but-updatable item was indistinguishable from nothing to do.Closes #93
Stacked
Base is
refactor/92-ecosystem-aware-wildcard(#106), which is itself stacked onfix/stabilization-pass(#99). Neither is merged. Do not merge this before #99 and #106; retarget tomasteronce they land.#106 is the direct prerequisite: it made the wildcard decline conditional on the ecosystem (a Cargo
serde = "1.*"is rewritten, an npm"lodash": "1.x"is not), so a note that states "the real reason" has to vary by ecosystem, and theOption<Ecosystem>it threads intorewrite_constraintis what lets it.The shape chosen for carrying declines out
rewrite_constraintnow returnsResult<String, DeclineReason>instead ofOption<String>, andplan_fixesreturns(String, Vec<FixRecord>, Vec<Declined>).The
Optionwas the lossy boundary. Which guard fired is the only thing that makes a note actionable, that fact is live exactly at thecontinuethat discarded it, and it is not recoverable afterwards:run_fixwould have to duplicateis_rewritable, the has-an-update predicate,is_pinned, and target selection and every guard insiderewrite_constraint— a second copy kept in step with the first by hope. Returning it costs one enum.PlannedFixgains adeclined: Vec<Declined>(name, constraint, target version, reason). Declines are sorted and deduped insideplan_fixes, so the same crate under[dependencies]and[dev-dependencies]produces one note, not two.The decline reasons and their exact wording
Notes go to stderr, in
report_inherited_skips's register, from a siblingreport_declined_fixescalled immediately afterfix::planinrun_fix. The line is:CommaRange>=1.0, <2.0MultiClause>=1.0.0 <2.0.0,^1 || ^2||-separated range has more than one clause and one version cannot carry them allQualifier@dev,^1.0@beta,npm:pkg@1.0.0@qualifier — a stability flag or an alias — describes the range, not the versionDistTaglatest,nextWildcardOperator^1.x,=1.*,==1.*WildcardPins1.x; unrecognized manifestsWildcardUnbounds1.*, Gradle1.+WildcardShape*,1.2.*,1.+PartialVersion"16","1.0"So the issue's reproduction now prints:
The closing line changes only when something was left alone:
Everything is already up to date.survives verbatim for a run with no declines, which has its own test.Where the
updatablepredicate livesDependencyStatus::has_update()independable-core. There were three copies of the samematches!, not two:plan_fixes(fix.rs),report_inherited_skips(runner.rs), andManifestCheck::outdated(dependable-fetch/src/check.rs). All three now call it. They have to agree — the whole defect is two commands disagreeing about what counts as an update — so the agreement is now structural rather than transcribed.What falsifies the issue's reproduction
crates/dependable/tests/cli_fix.rs,a_declined_wildcard_is_reported_instead_of_claimed_up_to_date: apackage.jsonwhose only dependency is"lodash": "1.x", a registry serving1.0.0/1.9.0/2.0.0, and three assertions — the note appears on stderr with its exact text, stdout does not containEverything is already up to date., and the manifest is byte-identical afterwards.a_dry_run_reports_a_declined_wildcard_toocovers the "printed nothing at all" half.Also added:
a_declined_dist_tag_is_reported(npm"latest"with a lockfile holding1.0.0),a_declined_comma_range_is_reported(requirements.txtwithrequests>=1.0,<2.0),a_run_with_no_declines_still_says_everything_is_up_to_date, and, infix.rs,a_decline_names_the_guard_that_refused_it(one assertion per reason variant) plusa_declined_constraint_leaves_a_record_of_what_was_not_doneandan_up_to_date_dependency_is_not_a_decline.These need a registry that actually offers a newer release, so
cli_fix.rsgains a ~40-line loopback HTTP server built onstd::net::TcpListener. No dev-dependency is added (dependablehas none), the tests stay hermetic and un-#[ignore]d, and they exercise the real fetch path rather than a stub of it.Judgement calls
latest, notnext. Onlylatestreaches the fix layer:check_versiontreats it as*, whilenextandbetafail to parse and are reported as unreadable constraints, never as available updates.latestalone still needs a lockfile pinning an older release, or it resolves to the newest and is up to date. Reversed by teaching the checker more dist-tags.>=1.0.0 <2.0.0) or||alternation is an unparseable constraint to the checker and never reachesrewrite_constraint, soMultiClauseis currently unreachable end to end; a PEP 440 comma range parses and does reach it.MultiClauseis kept and unit-tested, because the guard is real and a parser change would make it live. Reversed by teachingto_version_reqnpm-native range dialects.ORed — only which one gets to explain itself. An operator has to answer first or^1.xwould be blamed on pinning when the rewrite (^2.0.0) pins nothing; the reading answers before the shape because it names a concrete harm and the shape does not. Reversed by reordering the match.BareVersionreading added later declines underWildcardShape. The wildcard match has a_arm (the enum is#[non_exhaustive]) that declines, as every non-caret reading already did, under the reason that names no particular harm. Inventing a harm for a reading this code has never seen would be worse than saying only that the shapes do not correspond.fixchanged, so piping stdout must neither swallow it nor mix it into that record.new_constraint == item.version_constraintstillcontinues silently: the constraint would have accepted the rewrite, so there is nothing to explain.Note on PR #97
#97 (a different unmerged stack) changes the same closing line for the uncheckable case to
Nothing to rewrite. N dependencies could not be checked for a newer version…. That is a different condition from this one; the wording here —N available update(s) left alone— is deliberately distinct so the two read as separate facts if both land. The two will conflict textually inrun_fixand need a human merge.An unrelated Windows failure this PR had to absorb
The first CI run failed two
dependable-reportSARIF tests onwindows-latest, in a crate this change otherwise does not touch. They are not caused by this change — they are deterministic failures of commit0248ef8("fix(report): emit SARIF artifact URIs a consumer can actually resolve"), which lives onfix/stabilization-pass(#99), two levels down this stack.uri_is_relative_to_report_root_and_slash_joinedandspaces_are_encoded_in_relative_and_absolute_urisfeeduri_forpaths like/elsewhere/Cargo.tomland assert it returnsfile:///elsewhere/Cargo.toml. On Windows that path is not absolute —Path::is_absolutethere wants a drive prefix — souri_fortakes its relative branch and returnselsewhere/Cargo.toml. The assertion tests nothing on Windows and cannot pass.It went green on #99 and #106 because their Windows jobs reused a cached
dependable_reporttest binary built before0248ef8. This branch touchesdependable-core, whichdependable-reportdepends on, so the test binary was rebuilt and the assertion ran for the first time. Every currently-green PR in the repo is based on a branch that does not contain0248ef8.The last commit here builds the fixture path and its expected URI per platform, so the claim is made on Windows rather than gated off it.
uri_foris unchanged, and the Windows-specific behaviour it does have is already covered bya_windows_path_keeps_its_drive_and_encodes_its_segments.This fix belongs in #99, not here. It is one self-contained commit precisely so it can be moved: if #99 fixes it at the source, drop
test(report): give the SARIF uri fixtures a path Windows agrees is absolutefrom this branch. Until it is fixed somewhere,fix/stabilization-passandrefactor/92-ecosystem-aware-wildcardwill go red on Windows the moment their cache turns over — including after they merge tomaster.Gates
Baseline on the base branch was 917 passed / 0 failed / 20 ignored; the 8 new tests are the difference. No test was deleted or weakened.
Restacked on the repaired base
refactor/92-ecosystem-aware-wildcard— itself just brought forward onto the tenstabilization commits it was missing — is merged in at
fd84779. A merge, nota rebase, so the pushed history is unchanged.
Conflicts
crates/dependable/src/fix.rswas the only conflicted file, in two hunks:DependencyKindfor the override guard; this branch had already droppedDependencyStatuswhenhas_update()replaced the inlinematches!. Kept as{CheckResult, DependencyKind}— the test module importsDependencyStatusfor itself.plan_fixesguardsupdatablematches!withstatus.has_update(). Independent changes, both kept, with the override skip first so an override never reachesrewrite_constraint.An override is skipped without recording a
Declined. ADeclinedsays aconstraint refused a rewrite, and its note invites the author to widen that
constraint. An override refuses for a reason its constraint has no part in and
that no edit to the constraint would change, so a note there would point the
author at a string that is not the problem.
fix_all_leaves_an_override_alonenow asserts that emptiness alongside itsexisting claims.
Everything else merged cleanly and was checked to compose rather than merely to
compile:
check_version_for's translation-failure detection routes anuntranslatable constraint to
Undetermined, whichhas_update()excludes, sosuch a dependency is neither reported up to date nor rewritten. All three
has_update()call sites survive,ManifestCheck::outdatedamong them, as doregistry_unreachableingate_is_answerableandreport_declined_fixes.9ced38dreverted9ced38d("give the SARIF uri fixtures a path Windows agrees is absolute") isreverted in
7bd51d9. It worked around a Windows failure that787480dhassince fixed at its cause —
uri_fornow asksPath::has_rootrather thanPath::is_absolute, and the gap between those two predicates was the entirereason
/elsewhere/Cargo.tomltook the relative branch on Windows whileasserting the absolute branch's answer.
Keeping it would have cost coverage rather than added it. On Windows its
outside_roothelper substitutes a drive-absolute path, so therooted-but-drive-less case — precisely the one
787480drepaired — would nolonger be exercised on the only platform where it was ever broken, and the
drive-absolute form it substitutes is already covered by
a_windows_path_keeps_its_drive_and_encodes_its_segments. Its doc comment hadalso become false and contradicted an assertion
787480dadded a few hundredlines below in the same file.
crates/dependable-report/src/sarif.rsis now byte-identical to its state onfix/stabilization-pass. The SARIF tests were re-read for self-consistency: theone
#[cfg(windows)]test is gated in full, and every ungateduri_forassertion resolves the same way on both platforms under
has_root.Gates
Both the base (925) and this branch's pre-merge count (925) are exceeded, so no
test was lost in the resolution.
Repairs verified end to end
Run against the built binary, not inspected:
fix --allon{"pnpm":{"overrides":{"foo@2>bar":"3.0.0"}}}leaves the overrideuntouched and prints no decline note. The registry resolves
bar— the last arrowsegment — to an unrelated package at
0.1.2, which is the downgrade the guardprevents. As a control, an npm
"lodash": "1.x"in the same build does stillproduce
note: left lodash = 1.x alone … a bare version here would pin it to one,so the silence above is the override guard and not broken reporting.
errorand exits 0 under--fail-on vulnerable, withnote: 1 dependency was not found in its registry, so it is not gated on.version = "[4.0,4.9"reports undetermined, notup to date."overrides": {"semver": "$semver"}resolves as a reference to thesemverconstraint and exits 0.Restacked on the second repair round
fix/stabilization-passreceived ten further commits after this branch was last brought forward — repairs for four defects that its own first round of repairs had introduced. This branch now contains them: Go's410 Gonecounted as not-found alongside404; anErrorOrigin(NotFound/Unanswered/Local) carried onCheckResultfrom the typed fetch error, splittingScanIntegrityintounresolved(exempt from the gate, reported) andunevaluated(unanswerable); an override key carrying a version range no longer split on the>inside that range; a bare*in PEP 440 and Poetry translating to*rather than reading as a failed translation; a stderr note for undetermined dependencies;-qhonoured on both notes;PackageSource::Unresolvedgaining its own"unresolved"list token; andcrates/dependable/tests/cli_gate.rs, a stub HTTP registry that can answer with a chosen status code and content type.The merge was clean — no conflicted files.
Everything load-bearing across the stack survived, verified in the merged tree rather than assumed: the
DependencyKind::Overrideexclusion inplan_fixes(which deliberately records no decline note, since an override is not a refused rewrite but a version the author forced), all threehas_update()call sites,report_declined_fixesand theErrorOrigin-based gate coexisting inrunner.rs,Ecosystem::bare_version(), and translation-failure detection reachingUndetermined.Validation
The behaviours the stack must not lose are each covered by a test that ran green in the merged tree, rather than by a claim: