fix(scripts/dep-audit): correctness fixes from PR #6353 review, follow-up - #6358
Conversation
…reporting The dependency audit scripts now handle lockfiles more carefully by recording whether each target's `Cargo.lock` existed before analysis and restoring that exact state, including removing newly created lockfiles and refusing to analyze targets with symlinked lockfiles. The unused dependency check now scans for the correct identifier (the manifest alias for renamed dependencies) and searches individual target files rather than entire directories to avoid false positives from unrelated sibling tests. The heavy dependency ranking excludes development-only dependencies from the shipped-build weight table, and the drift report now correctly serializes the patch-only count. The `tinyanalyzer.toml` configuration empties the `ignore_unused` list since the re-check already handles the attribute-based false positive that previously required global suppression. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Re-ran the dependency audit with the latest tinyanalyzer, which now detects additional unused dependencies and reports more accurate "name only" classifications. The updated report reflects changes in dependency counts, unused flags, and duplicate version tables across multiple targets, including the removal of several unused crates such as `tempfile` from root and `thiserror` from tinyskills. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Comment |
Tiny Sweeper reviewTiny Sweeper reviewed this change across 6 lane(s) and found 7 active actionable finding(s). Detailed lane evidence and any incomplete work are listed below. State: Reviewing pending checks Review snapshot
Completeness: Complete What changedThe review could not produce a supported behavioral summary; inspect the cited changed surface and lane details below. FeaturesNone identified with supported citations. TestsNo supported feature-to-test mapping was produced. Test execution is not inferred. Findings
Resolved this pass
Pending checks: Rust E2E (mock backend), Build Playwright E2E Artifact, E2E (Playwright / web lane), Desktop E2E (full suite, 3 OS) Before merge
How this fits togetherflowchart LR
n0["driftAcross<br/>changed<br/>5 findings"]:::flagged
n1["packageDir<br/>changed<br/>5 findings"]:::flagged
n2["packageSourceDirs<br/>changed<br/>5 findings"]:::flagged
n3["renderMarkdown<br/>changed<br/>5 findings"]:::flagged
n4["unusedFor<br/>changed<br/>5 findings"]:::flagged
n5["data"]:::impacted
n6["p"]:::impacted
n7["summary"]:::impacted
n8["a"]:::impacted
n9["m"]:::impacted
n10["pulledInVia"]:::impacted
n0 -->|uses| n5
n0 -->|uses| n6
n0 -->|uses| n8
n0 -->|uses| n9
n1 -->|uses| n5
n1 -->|uses| n9
n2 -->|uses| n9
n3 -->|uses| n7
n4 -->|uses| n5
n4 -->|uses| n8
n7 -->|calls| n4
n7 -->|uses| n5
n7 -->|uses| n6
n7 -->|uses| n8
n7 -->|calls| n10
n9 -->|uses| n6
n10 -->|uses| n5
n10 -->|uses| n6
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Agent review detailscritique
security
tests
commits
description
e2e
Evidence and run details
|
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0343 · 720,043 in / 34,528 out · 78,034 cached (11%) · ladder/vectors, gpt-5.6-luna, deepseek-v4-flash · 1,128 embedded
critique: $0.0199 · 372,397 in / 15,728 out · 36,209 cached (10%) · gpt-5.6-luna, deepseek-v4-flash
security: $0.0118 · 204,765 in / 6,885 out · 11,105 cached (5%) · gpt-5.6-luna
tests: $0.0008 · 39,734 in / 3,683 out · 1,024 cached (3%) · deepseek-v4-flash
description: $0.0007 · 31,272 in / 4,358 out · 1,024 cached (3%) · deepseek-v4-flash
e2e: $0.0008 · 44,030 in / 839 out · 1,024 cached (2%) · deepseek-v4-flash
| @@ -99,7 +99,12 @@ fi | |||
| declare -A seen_by_repo_sha=() | |||
There was a problem hiding this comment.
Reserve built-in target names before discovery
root and openhuman-app are written to targets.tsv before this map is initialized, and neither name is inserted into it. If a recursive submodule directory is named root or openhuman-app and contains a Cargo.toml, discovery emits a second target with the same name; its JSON and log paths then overwrite the built-in target's artifacts, so the final report can associate the wrong analysis with that target. Seed the collision-tracking map with the built-in names, or assign unique report names before writing each target.
[RULE] report-file-collision ·
| * dependency to "keep". | ||
| */ | ||
| function packageSourceDirs(dir) { | ||
| const dirs = new Set([dir]); |
There was a problem hiding this comment.
Exclude unrelated sibling files from source-use scans
dirs still contains the whole package directory, and grepAny recursively scans every *.rs file under it. Adding explicit target paths to files therefore does not prevent an unrelated sibling such as another integration test, example, or workspace Rust file from matching the dependency name and changing an actually-unused dependency to keep. The new documentation explicitly promises that each explicit target is scanned as an exact file; either stop recursively scanning files that are not package sources or otherwise exclude unrelated siblings from the directory targets.
[RULE] overbroad-source-scan ·
| const head = /^\[([a-zA-Z0-9_.-]+)\]/.exec(line); | ||
| if (head) { | ||
| section = head[1]; | ||
| const table = /^(dependencies|dev-dependencies|build-dependencies)\.([A-Za-z0-9_-]+)$/.exec(section); |
There was a problem hiding this comment.
Recognize renamed dependencies in target-specific tables
This only recognizes aliases in plain [dependencies], [dev-dependencies], and [build-dependencies] tables. A valid declaration such as [target.'cfg(windows)'.dependencies] with foo = { package = "bar", ... } is skipped, so dependencyAliasMap returns no foo -> bar mapping. The later graph lookups use foo even though the resolved package is named bar, producing incorrect graph-win and dependent information for target-specific dependencies. Match dependency table suffixes as well as top-level tables.
| const table = /^(dependencies|dev-dependencies|build-dependencies)\.([A-Za-z0-9_-]+)$/.exec(section); | |
| const table = /(?:^|\.)(dependencies|dev-dependencies|build-dependencies)\.([A-Za-z0-9_-]+)$/.exec(section); |
[RULE] target-specific-dependency-alias ·
| let tableDepKey = null; | ||
| for (const raw of toml.split("\n")) { | ||
| const line = raw.trim(); | ||
| const head = /^\[([a-zA-Z0-9_.-]+)\]/.exec(line); |
There was a problem hiding this comment.
Handle renamed dependencies in target-specific tables
Cargo permits renamed dependencies under tables such as [target.'cfg(unix)'.dependencies.foo] and [target.'cfg(unix)'.dependencies]. This parser only recognizes top-level dependency sections, so those aliases are absent from the map. The later graph lookup then uses the manifest alias instead of the resolved package name, causing valid dependencies to be reported with incorrect graph-win or unused results. Parse target-specific dependency tables, including quoted target expressions, or use a TOML parser.
[RULE] parse-target-specific-dependencies ·
| continue; | ||
| } | ||
| if (!/^(dependencies|dev-dependencies|build-dependencies)$/.test(section)) continue; | ||
| const inline = /^([A-Za-z0-9_-]+)\s*=\s*\{([^}]*)\}/.exec(line); |
There was a problem hiding this comment.
Parse multiline renamed dependency declarations
This only recognizes inline dependency tables whose package = "..." attribute appears on the same line as the dependency key and closing brace. A valid multiline declaration such as foo = { followed by package = "real-foo" is left unmapped, so resolvedDependency and otherDependents look up the alias instead of the resolved crate name and produce incorrect unused-dependency or graph-win results. Parse the TOML structure rather than matching one line at a time, or retain the current dependency table until its closing brace and inspect its fields.
[RULE] incomplete-manifest-parser ·
| * imports), the latter is the real crate name (what the resolved package is | ||
| * called), so callers matching a dependency against the graph need this map. | ||
| */ | ||
| function dependencyAliasMap(dir) { |
There was a problem hiding this comment.
Add unit tests for the dependency alias map parser
dependencyAliasMap parses Cargo.toml to map manifest dependency aliases to real crate names. This mapping is used in the unused-dependency check; if it misparses a form (e.g., workspace-level dependencies, dependencies.rename.workspace = true, or double-quoted inline tables), the verdict for renamed dependencies will be wrong. The function has no unit test. Add a test case under scripts/dep-audit/__tests__/ that exercises inline { package = ".." } and table-form [dependencies.alias] renames along with missing-file and empty-Cargo.toml paths.
[RULE] untested-function ·
tinysweeper
Bot
added
the
priority: p2
1 participant
Summary
#6353 (the dep-audit tool itself) was merged before its CodeRabbit/tinysweeper review threads were fully addressed and pushed (a race with the merge). This PR carries the fixes I made in response to that review, rebased onto current
main(post #6353/#6355/#6356). All 28 threads on #6353 were replied to and resolved citing this work; this is that work landing.Fixes (scripts/dep-audit/report.mjs, run.sh, tinyanalyzer.toml, README.md)
compatKeyno longer collapses distinct0.0.zversions into one bucket (0.0.1and0.0.2are semver-incompatible; only0.xwithx>0collapses by minor).driftAcross/summary.json:patch_onlyis now a real field ({ rows, patch_only }) instead of a property tacked onto an array, whichJSON.stringifysilently dropped.packageSourceDirsnow includes a package'sbuild = "..."script and tracks[[test]]/[[example]]/[[bench]]/[[bin]]targets as exact files rather than their parent directory, so a sibling file in a shared directory (e.g. the root crate'stests/) can no longer flip an unrelated package's unused-dependency verdict.dependencyAliasMapso a dependency declared withpackage = "real-name"resolves its real crate name before graph lookups (previously reported a null version/graph-win).packageDir:decodeURIComponents thepath+file://component (a checkout path with a space/percent-encoded character previously failed existence checks and forced a falseremove).heavyFor: excludes dependencies whose only edge kind isdevelopment— those never link into the shipped build and don't belong in a shipped-build weight ranking.run.sh: fails loudly (instead of silently skipping) when a listed submodule is not initialized; rejects a symlinked/non-regularCargo.lockbefore backing it up; tracks whether a lockfile existed before the run and restores that exact state (including removing a newly-created one) via anEXITtrap, so an interrupted run cannot leave edits behind.tinyanalyzer.toml: removed the globalignore_unused = ["thiserror"]suppression —report.mjs's own textual re-check already handles the one false positive it existed for, and the global suppression was hiding genuinely-unusedthiserroroccurrences everywhere.crate::…, which is misleading —crate::means something else in Rust; now describes the actualdep_name::…pattern), corrected the "every optional feature" claim (verified against tinyanalyzer's source: defaultcargo metadata, no--all-features), and documented the lockfile-absence/symlink behavior.docs/dep-audit/2026-09-19.mdis regenerated from the fixed generator against currentmain.Validation
node --check scripts/dep-audit/report.mjs,bash -n scripts/dep-audit/run.sh.bash scripts/dep-audit/run.sh --snapshot— full 24-target sweep, exit 0, lockfiles restored (verifiedgit statusclean afterward).thiserrorfindings appear now that the global suppression is gone (e.g.tinyskills→ remove),summary.json'sdrift.patch_onlyis present and non-zero.Submission Checklist
scripts/, exercised by running the full sweep.app/srcor product Rust changes.## Related— N/A.pnpm --filter openhuman-app format:check— N/A, noapp/changes.pnpm typecheck— N/A, no TypeScript changes.Related
Follow-up to #6353.