fix(scan): audit graph draws immediate dependencies, not the soldeer closure - #177
Conversation
…closure Soldeer does not resolve transitive dependencies, so org manifests declare the FULL closure and graph_edges() drew every entry as a direct edge. An edge now survives only when the consumer's own code (outside vendored trees) imports the dependency's package prefix, read from the same shallow clone the scan already takes per Foundry repo — zero extra fetches. - graph::imported_prefixes / import_package_matches / immediate_deps: AST-parsed import extraction (lexer fallback for grammar-rejected files), versioned-prefix matching that never splits package names on hyphens, and the closure filter with an explicit deps-unknown state. - A readable manifest with an unreadable tree mirrors the unparseable- manifest state: depsKnown=false, no silent keep-all/drop-all, logged. - Blockers are reachability over the dep set and closure entries are by definition reachable through direct ones, so blocker sets are unchanged; asserted in tests rather than assumed. - Stale-pin data (#79) on surviving edges still comes from the manifest. - site/audit.html: the depsKnown=false label now names both causes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…'s semicolon; empty and absolute import targets name no package Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe scanner now extracts Solidity imports, filters them to own non-vendored files, and builds graph edges only for immediate manifest dependencies. It preserves unknown states for unreadable manifests or trees and updates tests and audit messaging. ChangesDependency graph resolution
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The change narrows audit-graph edges to direct package usage, with the supplied verification checks passing. No actionable merge-blocking risk remains beyond normal review. Sequence Diagram(s)sequenceDiagram
participant ShallowCloneWalk
participant resolve_node_deps
participant imported_prefixes
participant RepoResult
ShallowCloneWalk->>imported_prefixes: collect Solidity import prefixes
ShallowCloneWalk->>resolve_node_deps: provide manifest state and import prefixes
resolve_node_deps->>RepoResult: store immediate dependencies and deps_known
resolve_node_deps->>ShallowCloneWalk: emit warning for unreadable trees
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Master took #176 ([external.package] table), #177 (immediate deps in the audit graph) and #178 (package names resolve manifest-first with a fallback to the release workflow's `soldeer-package:` input; unresolvable renders UNKNOWN via packageKnown, rainix#335) since the last sync. Resolutions: - main.rs `fetch_inputs`: both sides kept — master's shape (build RepoInputs first, then one registry lookup keyed on `inputs.package()`, the manifest-or-workflow resolution) composed with this branch's typed `foundry.lock`/`.gitmodules` reads (`RepoFile`), which stale-foundry-lock needs so a failed fetch cannot read as an absence. - untested.rs: this branch's hoisted `pub const VENDOR_DIRS` (shared with consumers) kept, master's doc addition about `graph::imported_prefixes` kept above it, master's now-duplicate function-local const dropped. - SKILL.md findings table: master's #178 rewording of `soldeer-unpublished` (name from the release-metadata table OR the release workflow) plus this branch's `stale-foundry-lock` row appended; denofmt realigned the columns. - site/health.json: master's side — the hourly scan cron owns that file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The defect
graph_edges()drew one edge per[dependencies]entry. Soldeer does notresolve transitive dependencies, so an org manifest declares the FULL closure —
a consumer must declare every package its dependencies' own internal
rain-solmem-0.1.3/...imports resolve through, whether or not its own codeever imports them. The audit graph therefore drew transitive edges as if they
were direct: rain.verify showed 16 first-party outgoing edges when its own code
imports 6 first-party packages; edge count tracked manifest size, not usage.
The fix — immediacy from data, never from graph shape
An edge
from → tosurvives iff a file infrom's OWN tree importsto'spackage prefix.
untested::is_vendored):everything outside top-level
lib/,dependencies/,node_modules/,out/and
cache/—src/,script/andtest/in the org's layout, withoutdropping own code that lives elsewhere. A vendored file's imports are its own
package's dependencies resolving through the consumer's remappings — counting
them would re-admit the exact closure this cuts.
solang_parser::parse), so a commented-outimport or an import-shaped string literal is not a use. A file the grammar
rejects (solang can lag solc syntax) still lexes; a token-stream fallback
reads
import… one string literal …;rather than silently dropping areal direct edge.
<package>-+ aleading-digit version — never hyphen-splitting, so
rain-math-float-0.0.1is not a use of
rain-math, while pre-releaserain-math-float-0.0.1-alpha.5is one ofrain-math-float. Deliberatelyversion-agnostic: an import pinning an older path than the manifest is still
a direct use, and version/stale data on surviving edges keeps coming from
the manifest exactly as before (Dependency graph ignores pinned versions — a stale pin's transitive closure can differ from what's drawn #79 semantics untouched).
reachable through another dependency keeps its edge (everything imports
solmem); a test asserts precisely that. Immediacy is decided from the imports
that exist, never from the shape of the graph.
the same
deps_known: falsestate an unparseable manifest yields (typedDepsResolution,::warningin the scan log), never a silent keep-all ordrop-all. A manifest declaring nothing needs no tree to confirm it. The site
label now says which ("deps unknown — manifest or tree unreadable") — the
only site change; the data shape is unchanged.
same shallow-clone walk
untested::analyzealready does. No coverage boundwas added anywhere; nothing is skipped silently.
Verification: two full 4-org scans, same token, same minute
The hourly cron's committed
site/health.jsonis NOT a fair baseline here: itruns with a token that sees only public repos, while a local scan sees the
private ones too (st0x.atomic-bridge, st0x.bebop, st0x.univ4.hook, dvin.poc
have no node in it at all). So the baseline below is master's own binary
built and run locally (same token, same
ORGS="rainlanguage cyclofinance S01-Issuer ST0x-Technology"), 11 minutes apart from the branch scan.Node sets: identical, 47 Solidity repos.
Edges: 190 → 115 (−75). Strict subset: zero edges in AFTER that are
not in BEFORE.
33 of 47 repos unchanged — a repo already declaring only what it imports
(e.g. rain.deploy) is untouched. The 14 that changed, all decreases:
Ground truth, checked against fresh clones rather than the scan's own output:
(
rain-factory,rain-verify-interface,rain-deploy,rain-interpreter-interface,rain-solmem,rain-sol-codegen). Five mapto graph nodes = the 5 kept edges. The sixth,
rain-interpreter-interface,is the LEGACY name of today's
rainlang-interface(reporainlang.interface) — no node publishes the old name, so it drew no edge on
master either. Pre-existing, unchanged by this PR, noted for follow-up.
rain.erc4626.words imports 5 → exactly its 5. st0x.atomic-bridge
declares 7 first-party packages and imports exactly one
(
rain-math-float-0.1.1) → its 1 kept edge; the other 6 are entries noimport justifies.
blockedBy— identical for 45 of 47 nodes, and the two diffs are the fix workingReachability over direct edges equals reachability over closure edges whenever
every closure entry really sits beneath some direct dependency — a unit test
asserts blocker sets are unchanged in that case. The two live diffs are repos
whose manifests carry dead entries (declared, imported nowhere, and beneath
no kept dependency either):
rainlang = "0.1.2"and imports itnowhere. The 8 lost blockers are exactly
rainlangplus rainlang's ownunaudited subtree (
rain.datacontract,rain.extrospection,rain.lib.memkv,rain.math.float,rain.metadata,rain.string,rain.tofu.erc20-decimals) minus what stays reachable through kept edges(
rain.deploy,rain.sol.codegen,rain.solmemall remain).are imported nowhere in its own code. What remains is precisely
rain.math.float+ rain.math.float's subtree (rain.datacontract,rain.deploy,rain.sol.codegen,rain.solmem,rain.string) —rain.math.float's manifest and imports confirm that subtree exactly.
Master asserted these repos stand on ground their code does not touch. The
manifests still carry the dead entries (the scan is not a linter for them);
the graph just stops drawing them as dependencies.
Before / after renders
Same page (
site/audit.html), same scan universe, only the binary differs.The closure thicket collapses to the direct graph; the headline "11 of 47 …
clear ground" is unchanged; most-inherited moves only where the blockedBy
shrinks predict (rain.factory 7 → 6 inherit; rain.solmem 28 and
rain.sol.codegen 25 unchanged); per-repo stale-deps lists shrink in step with
their dropped edges (rain.merkle 8 → 2).
Before (master binary, fair local baseline):
After (this branch):
QA
acceptance case: closure entry dropped, direct entry kept,
direct-AND-transitively-reachable entry kept (NOT-transitive-reduction),
unknown-tree repo handled (typed, flagged), blockers unchanged vs closure
edges, AST-vs-comment discrimination, grammar-rejected file fallback,
version-vs-name-extension prefix matching, and the two survivor-killers
below.
sol_import_paths,imported_prefixes,import_package_matches,immediate_deps,resolve_node_deps) — one behavior each. Probe pass 1: 16 killed, 2survived. Survivors got a discriminating test each and were re-probed to
KILLED; final matrix 18/18 killed, 0 no-run, 0 harness errors, with the
suite's own tally (233 baseline / 235 after the new tests) proving every run
ran:
;) — killed bythe_lexer_fallback_stops_at_the_imports_semicolon: a grammar-rejectedfixture with
import X;followed later by the string"phantom-9.9.9/x.sol", which must NOT be collected.an_empty_or_absolute_import_contributes_no_prefix:import "";andimport "/abs/X.sol";contribute nothing.Attribution is by construction: the 233-test suite let both live; the only
delta in the killing suite is the two new tests.
import … from "<package>-<version>/…"in the consumer's own files) andlive repos, not the implementation: rain.verify / rain.merkle /
rain.erc4626.words / st0x.atomic-bridge clones grepped independently, plus
the fair master-binary scan for the graph-level deltas above.
the named knock-ons checked — blockers (test + live proof above),
standsOn()tap-highlight (walks the sameg.edgestransitively; unchangedargument, verified against the render), stale-pin Dependency graph ignores pinned versions — a stale pin's transitive closure can differ from what's drawn #79 semantics on surviving
edges (manifest remains the source; visible in the renders' stale lists),
and the unreadable-tree state (typed, mirrored from the unparseable-manifest
state, mutants M12/M16/M17/M18). Transitive reduction explicitly rejected
and tested against.
Suite:
nix develop -c cargo test -p roh-scan→ 235 passed / 0 failed.pre-commit run --all-filesclean (rainix static toolchain = CI).nix run .#dashboard-test→ 264 passed / 0 failed.Summary by CodeRabbit