Skip to content

Improve --suggest-fix: Better adherence to restrictions - #10

Merged
timweri merged 34 commits into
mainfrom
improve-suggest
Sep 19, 2026
Merged

timweri merged 34 commits into
mainfrom
improve-suggest

Conversation

@timweri

@timweri timweri commented Sep 7, 2026 •

Copy link
Copy Markdown
Owner

Make --suggest-fix emit downgrade commands that satisfy the active requirements in Cargo.lock and workspace manifests, on a best-effort basis.

  • Read dependent requirements from the sparse crates.io index and local manifests.
  • Select the newest eligible, non-yanked version that satisfies every known requirement.
  • Report blocked packages with the requirement that prevents a downgrade, and annotate suggestions when a
    requirement could not be verified.
  • Preserve package source identity so local and registry packages with the same name and version do not
    affect each other.
  • Add prerelease support, cache sparse-index records, and cover the behavior with unit and fixture tests.

Package now carries its dependency edges (resolved to concrete
versions by cargo_lock) and an is_registry flag. Intake stops
filtering non-registry packages so workspace members and other
lockfile entries are available as dependents for the requirement
graph the suggest-fix flow will build; the registry filter moves to
the point of use in main's check loop.

Adds semver, glob, and cargo_toml as dependencies for the
requirement-matching and manifest-reading work to follow.
fetch_index_record retrieves the newline-delimited JSON index record
for a crate from index.crates.io, one line per published version with
that version's own dependency requirements — the source the suggest
flow will use to check whether a candidate downgrade satisfies a
transitive dependent. It reuses the existing retry and 404-as-empty
handling via a new fetch_body helper shared with fetch_json, but skips
the inter-request pacing delay since the sparse index isn't subject to
the crates.io API's rate limit.

Index records are cached under a third map in the response cache,
defaulted on deserialisation so cache files from earlier releases
still load.
load_direct_requirements answers "what do the user's manifests require
of a given registry crate": it reads the root Cargo.toml beside the
lockfile, expands workspace members from glob patterns with exclusions
applied, follows path dependencies one level so members not listed
under workspace.members are still read, and walks normal, dev, build,
and target-specific dependency tables. Renames resolve to the real
crate name and workspace-inherited requirements are resolved by
cargo_toml itself.

A missing or unparseable manifest, or an entry whose requirement can't
be determined, produces a warning and is treated as unconstrained
rather than aborting the run — the bare-lockfile workflow must keep
working.
…ests

Replaces the newest-by-age-only suggestion with a pipeline: gather
every version requirement currently placed on a "too new" package,
from lockfile-recorded dependents (via the crates.io sparse index for
registry dependents, honouring renames and excluding dev-kind edges)
and from the user's own manifests; then walk candidates newest to
oldest, within the same caret-compatible zone as the locked version,
for the first one every requirement accepts.

Candidate filtering deliberately doesn't build a literal `^<locked>`
requirement to bound the search — that only ever matches versions at
or above locked, which would rule out every downgrade. Bounding uses
same_compatible_zone instead: cargo's own symmetric notion of "same
leading nonzero component" (major, or minor/patch when major is 0).

Each violation now produces one Outcome — Suggest, Blocked (naming the
blocking package/manifest and its requirement, and whether the blocker
itself has a pending suggestion), or NoCompliantVersion — so reporting
can't silently drop a case. A dependent whose requirement couldn't be
read degrades the suggestion to an "unverified" annotation rather than
losing it. The report gains a "no compatible version" section and
drops the old blanket disclaimer and cargo-tree-i pointer, both made
obsolete by the check the tool now performs; a closing note states
what is and isn't verified.

Adds --include-prerelease (requires --suggest-fix, per clap's existing
requires="min_age_days" pattern for --suggest-fix itself), with help
text noting that semver rarely lets a prerelease satisfy a normal
requirement, so an empty result under the flag is expected.
Drives lockfile intake, manifest reading, and generate_suggestions
together over a committed fixture (a one-member workspace and a
hand-written Cargo.lock), asserting all four outcome kinds in one
pass: a suggestion, a package blocked by a manifest requirement, one
blocked by a transitive dependent's index requirement, and one with
nothing old enough in its compatible range.

README documents --suggest-fix's actual guarantee and adds
--include-prerelease.
- also_suggested was set whenever the blocker's name appeared in the
  too-new set, regardless of whether the blocker's own walk actually
  resolved to a suggestion. It's now computed in a second pass once
  every outcome is known, against the set of packages that actually
  got Outcome::Suggest.
- Manifest-derived constraints were matched by crate name alone, so a
  workspace member's requirement on a crate leaked into another
  member's unrelated locked version of a same-named crate. Direct
  requirements now carry the manifest's own declaring_package, and
  gather_constraints scopes each dependent to its own manifest.
- print_suggestions's "no compliant versions found" branch fired
  whenever there were zero Suggest outcomes, even when Blocked
  outcomes existed with real compliant versions — printing that
  contradictory message right above the accurate "no compatible
  compliant version" section. Restructured around has_suggestion; the
  closing "suggestions satisfy..." footer only prints when there is at
  least one suggestion to make a claim about.
- gather_constraints rescanned every lockfile package per violation;
  a dependents index is now built once per run.
- main.rs's ad hoc lockfile-path resolution duplicated logic already
  in lockfile::load; both now share lockfile::resolve_path.
- Restored a doc comment on fast_client that an earlier edit dropped.

Two new regression tests cover the also_suggested and manifest-scoping
fixes directly; both failed before the fix and pass after.
gather_constraints now counts a dependent as verified only when one of
its recorded requirements actually matches the locked version. An alias
resolving to a different major, or an edge whose requirement no longer
covers what is locked, is reported as unverified instead of silently
treated as confirmation. The registry branch moves out into
registry_dependent_constraints.

also_suggested now keys on both name and locked version. A suggestion
for foo 1.5.0 previously annotated a dependency blocked by foo 2.5.0,
telling the reader to apply an unrelated downgrade. The message names
the blocker's version and no longer claims applying it will work, since
nothing checks whether the older version relaxes its requirement.

Trim the --include-prerelease help to one line. clap prints the whole
doc comment in -h, where a paragraph on semver prerelease matching sat
next to one-line entries for every other flag, and the README already
explains it.
Widen lockfile intake to carry each package's and dependency edge's
source (crates.io, alternate registry, git, or none for a local path),
and key the dependents index by (name, version, source) instead of
just (name, version). Previously, a dependent's requirement could leak
across packages that merely shared a name and version but came from
different origins, wrongly blocking a valid downgrade.

Dependency edges that omit a source in the lockfile are resolved
against the package list: an edge without a source always targets a
path package sharing the name and version when one exists (cargo only
omits the source when the true target has none), and only falls back
to treating every same-name/same-version candidate as a possible match
when no such path package exists. This stops a path consumer's
requirement from blocking a same-name/same-version crates.io downgrade.

Also fix a related false-ambiguity bug surfaced during review: the
index lists one dependency entry per target table, so a requirement
repeated identically across, say, cfg(unix) and cfg(windows) produced
multiple matching entries and was incorrectly treated as ambiguous
(demoted to "unverified" instead of enforced as a blocker). Distinct
requirement text is still ambiguous; identical repeats are not.
registry_dependent_constraints now classifies each matching
declaration as mandatory (unconditional, non-optional — always
active) or uncertain (target-gated or optional). Every mandatory
requirement is enforced as a definite blocker regardless of how many
other declarations also match, instead of the previous all-or-nothing
rule that let a single ambiguous extra declaration erase a known
blocker. A leftover uncertain declaration next to an enforced
mandatory one still marks the dependent unverified, since its own
applicability is still unresolved. With no mandatory declaration, a
lone uncertain one remains the unique explanation for the lockfile
edge and is still enforced, as before.

Add regression tests for two mandatory declarations of different
kinds both blocking a downgrade, and for a mandatory requirement
still blocking despite a co-occurring uncertain declaration.
@timweri
timweri requested a balanced review from Copilot September 8, 2026 05:29
@timweri timweri changed the title Improve suggest Improve --suggest-fix: Better adherence to restrictions Sep 8, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Candidate filtering and source handling can produce incorrect or unusable downgrade commands.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Improves --suggest-fix by validating downgrade candidates against lockfile and manifest requirements.

Changes:

  • Adds source-aware dependency and manifest constraint collection.
  • Supports prereleases, blocked outcomes, and sparse-index caching.
  • Expands unit, fixture, and end-to-end coverage.
File summaries
File Description
Cargo.toml Adds manifest, semver, and glob dependencies.
Cargo.lock Locks the new dependency graph.
README.md Documents enhanced suggestions and prereleases.
src/api.rs Fetches and parses sparse-index records.
src/cache.rs Caches sparse-index responses.
src/lockfile.rs Preserves dependency edges and source identity.
src/main.rs Integrates manifest constraints and prerelease handling.
src/manifest.rs Extracts requirements from workspace manifests.
src/policy.rs Updates test package construction.
src/report.rs Reports suggestions, blockers, and unverified requirements.
src/suggest.rs Implements constraint-aware candidate selection.
tests/fixtures/suggest_fix_e2e/Cargo.toml Defines the fixture workspace.
tests/fixtures/suggest_fix_e2e/Cargo.lock Supplies the fixture dependency graph.
tests/fixtures/suggest_fix_e2e/app/Cargo.toml Supplies fixture manifest requirements.
Review details

Suppressed comments (2)

src/manifest.rs:196

  • Alternate-registry dependencies are retained here but their registry identity is discarded in DirectRequirement. If one local package depends on crates.io foo and an aliased private-registry foo at the same version, the private requirement is enforced against the crates.io package and can incorrectly block its downgrade. Preserve and match registry/source identity for direct requirements, or treat unsupported alternate-registry requirements as unverified.
    // Path and git dependencies aren't registry-versioned.
    if let Some(detail) = dep.detail()
        && (detail.path.is_some() || detail.git.is_some())
    {
        return;

src/report.rs:184

  • This guarantee is false when a suggestion has unverified_dependents: the code deliberately emits the command without knowing those requirements. Qualify the statement so users do not interpret a best-effort suggestion as resolver-safe.
  Suggestions satisfy every version requirement in Cargo.lock and your manifests.
  Source compatibility is not verified: build after applying.
  • Files reviewed: 12/14 changed files
  • Comments generated: 4
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/suggest.rs
Comment thread src/manifest.rs
Comment thread src/report.rs
Comment thread README.md Outdated
filter_candidates only bounded candidates to the caret-compatible
zone of the locked version, sorted by publish date. A version
published earlier than the locked one but higher in semantic
precedence (or equal, including build-metadata-only differences, or
a stable release above a locked prerelease) could slip through and
get suggested as a "downgrade".

Reject any candidate whose semver precedence is >= locked before
sorting. Version's Ord already ignores build metadata and orders
prereleases below the release they precede, so this covers all of
those cases in one check.

Signed-off-by: Duc Thanh Nguyen <ng.duc.tahn@gmail.com>
When a registry package shares a name and locked version with a path
or git package, cargo rejects the abbreviated `name@version` pkgid
spec as ambiguous. Suggestions now compute a source-qualified spec
(`{source}#{name}@{version}`) whenever such a collision exists, and
the printed cargo update command uses it.

Verified against a hermetic local-registry fixture: the abbreviated
spec is confirmed ambiguous to a real cargo invocation, and the
qualified spec resolves without error.
Precompute a single (name, version) -> packages index once per
generate_suggestions call, and use it for both the target-source lookup
and the ambiguity check that feeds build_package_spec, instead of
scanning all_packages twice per violation. Mirrors the existing
build_dependents_index pattern.

Also drops an unneeded package_spec.clone() now that the value isn't
read again after being moved into the outcome.
README and CLI previously claimed suggestions satisfy every requirement
and that Cargo accepts the resulting command unconditionally, even though
unverified requirements are a supported path. Both now describe best-effort
verification and call out that unverified requirements may still be
rejected by Cargo.
…guard

Version's Ord breaks precedence ties on build metadata, so a locked
version like 1.3.0+build.2 let both 1.3.0 and 1.3.0+build.1 pass the
>= guard despite having equal semantic precedence. Use cmp_precedence,
which ignores build metadata per the semver spec, and require Less.
A manifest requirement declared against an explicit alternate registry
(via `registry` or `registry-index`) was previously indistinguishable
from an ordinary crates.io requirement once loaded: both compared only
on crate name, declaring package, and locked version. When one manifest
declared crates.io foo and a renamed alternate-registry foo at the same
locked version, the alternate-registry requirement could wrongly block
(or wrongly count as verifying) a crates.io suggestion it was never
written for.

DirectRequirement now carries a RequirementSource recording whether the
declaration is an ordinary crates.io dependency or names an alternate
registry, resolved from the dependency detail after workspace
inheritance. Only CratesIo requirements are enforced (or count as
verification) in the crates.io suggestion flow; a Registry(_)
declaration is excluded outright rather than compared against a
lockfile source string, since a registry alias and a lockfile source
URL are different representations with no mapping available here.
Cargo reserves the name `crates-io` for the default registry, and a
`registry-index` naming crates.io's own git or sparse index URL
directly is the same registry under its literal address. The prior fix
treated any `registry`/`registry-index` field as an alternate
registry unconditionally, so a manifest requirement written as
`registry = "crates-io"` was wrongly excluded from the crates.io
suggestion flow instead of being enforced as an ordinary crates.io
constraint.

requirement_source now recognizes these identities and reports
RequirementSource::CratesIo for them, alongside a bare (unqualified)
dependency.
…nt's identity

Local manifest requirements were scoped by declaring-package name alone,
so a git or alternate-registry dependent could inherit an unrelated
local package's requirement just because they shared a name (and, with
no version check, even without sharing a version). Restrict manifest
matching to dependents whose lockfile source is absent (a git or
alternate-registry dependent's requirements genuinely can't be read),
and match on the declaring package's version as well as its name,
carrying that version — including workspace-inherited versions —
through requirement collection.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Workspace exclusion globs and blocker attribution can currently produce incorrect downgrade diagnostics.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/manifest.rs:194

  • workspace.exclude supports glob patterns, but Path::starts_with treats each entry literally. For example, exclude = ["crates/*-internal"] never matches and those packages' requirements are loaded, so an excluded workspace member can incorrectly block a downgrade. Match the relative path with glob::Pattern instead.
fn is_excluded(root_dir: &Path, member_dir: &Path, exclude: &[String]) -> bool {
    let Ok(relative) = member_dir.strip_prefix(root_dir) else {
        return false;
    };
    exclude.iter().any(|pattern| relative.starts_with(pattern))
  • Files reviewed: 11/14 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread src/suggest.rs Outdated
… gaps

Deduplicate unverified parent labels in encounter order, document that
--include-prerelease does not change SemVer matching, and assert the
unverified label and guidance in the CLI test.

Use dependency sources already resolved by cargo-lock instead of
re-inferring them; drop the unreachable edge-ambiguity check and add
loader tests proving the invariant. Consolidate local-manifest and
registry-index requirement attribution behind one private policy.

Collapse repeated test setup behind two small helpers.
@timweri
timweri marked this pull request as ready for review September 16, 2026 19:02
@timweri
timweri requested a balanced review from Copilot September 16, 2026 19:02

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Candidate filtering, local optional dependencies, and workspace exclusion globs can produce incorrect outcomes.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

src/manifest.rs:148

  • workspace.exclude entries use glob patterns, but Path::starts_with treats *, ?, and character classes literally. Thus an exclusion such as exclude = ["crates/experimental-*"] is ignored; the excluded package is loaded as a workspace member and its dev-dependencies can become false downgrade blockers. Match each root-relative path with Cargo-compatible glob semantics instead.
    exclude.iter().any(|pattern| relative.starts_with(pattern))
  • Files reviewed: 12/15 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread src/suggest.rs
Comment thread src/suggest.rs

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Candidate filtering, manifest attribution, exclusion matching, and symlink handling can produce incorrect suggestions.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

src/suggest.rs:84

  • This unconditional caret-zone filter drops older versions even when every gathered requirement accepts them—for example, a lock at 2.0.0 with >=1,<3 can legally move to an old-enough 1.9.0, but this path reports no compliant version. Let the collected requirements determine eligibility instead of imposing an undocumented compatible-zone restriction.
            if !same_compatible_zone(locked, &parsed) {
                return None;
            }

src/suggest.rs:307

  • Every local declaration is treated as mandatory, although load_direct_requirements collects optional and target-specific declarations and currently discards that metadata. A permissive direct dependency plus a restrictive optional alias can therefore block a downgrade instead of marking the dependent unverified, unlike the registry-index path and the documented best-effort behavior. Preserve the declaration metadata in DirectRequirement and derive mandatory from it here.
                .map(|r| NormalizedDeclaration {
                    req: r.req.clone(),
                    mandatory: true,

src/manifest.rs:148

  • workspace.exclude supports glob patterns, but starts_with treats metacharacters literally and also overmatches plain prefixes (for example, excluding crates/foo also excludes crates/foo-extra). This can load requirements from excluded packages or omit active members. Match the normalized relative member path using glob patterns instead.
    exclude.iter().any(|pattern| relative.starts_with(pattern))
  • Files reviewed: 21/23 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread src/main.rs Outdated
…rting

- Treat target-specific index dependencies as mandatory, since Cargo
  resolves every target table regardless of host platform.
- Ignore alternate-registry index declarations when collecting
  crates.io constraints.
- Warn instead of silently skipping violations whose locked version
  fails to parse.
- Only load manifest requirements when there is a too-new violation,
  avoiding a spurious missing-Cargo.toml warning on clean runs.
- Reword the empty-suggestions message to say packages could not be
  checked rather than claiming no compliant versions exist.
…h date

A later-published backport (e.g. 1.3.9) could outrank a higher compliant
version (e.g. 1.4.0) and be suggested or reported as "newest compliant".
Sort by semver precedence descending, using publish date only to break
ties.
…t version

A cached index record list that predates the locked dependent version
caused the lookup to miss, marking the dependent unverified and dropping
its requirement. Treat such a cache entry as a miss and refetch.

Also align the README with target-specific registry requirements now
being enforced rather than annotated as unverified.
Match the convention used elsewhere in this file rather than placing
the comment between #[test] and the function signature.
Cache every successful sparse-index response, including empty results
from a 404 or whitespace-only body, so they are reused until normal
expiry instead of being refetched on every lookup.
…ty results

Replace the durable empty-result cache entry with a per-run memo of
fetched index records. A persisted cache entry is now a hit only when
it is fresh and contains the needed version, so a transient miss is
retried on the next run instead of persisting for the configured
cache expiry.
@timweri
timweri merged commit e488575 into main Sep 19, 2026
5 checks passed
@timweri
timweri deleted the improve-suggest branch September 19, 2026 00:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants