Repository navigation
Improve supply chain attack scan reporting and failure handling - #32
Merged
JosephMaynard merged 6 commits intoMay 12, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds packaging and local execution signal detection, candidate-limited npm registry metadata enrichment, compare-mode "new-*" policy evaluation, and end-to-end integration across types, aggregator, runners, CLI, UI, findings, tests, and README docs. ChangesSupply-Chain Signal Detection and Compare-Mode Framework
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli.ts (1)
1352-1355:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate
--fail-onhelp to advertise the new compare-mode rules.
parseFailOnRules()now accepts thenew-*delta rules, but the CLI help still lists only the old set. That makes the new feature hard to discover from the terminal.Suggested fix
--open Open the generated report using the system default application --fail-on <rules> Fail with exit code 1 when selected rules are violated - Supported: reachable-vuln, production-vuln, high-severity-vuln, - licence-mismatch, copyleft-detected, unknown-licence, - supply-chain-source + Supported: ${SUPPORTED_FAIL_ON_RULES.join(', ')}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli.ts` around lines 1352 - 1355, The --fail-on option help text is out of date and doesn't advertise the new compare-mode "new-*" delta rules accepted by parseFailOnRules(); update the CLI option help string (the --fail-on description in src/cli.ts where the option is declared) to list the corresponding new-* rules (e.g., new-production-vuln, new-high-severity-vuln, new-licence-mismatch, new-copyleft-detected, new-unknown-licence, new-supply-chain-source) alongside the existing rules so terminal help reflects parseFailOnRules() supported values and formatting remains consistent with the existing multi-line description.
🧹 Nitpick comments (2)
src/runners/npmRegistryMetadata.ts (1)
269-271: ⚡ Quick winRegistry lookups are fully sequential; this will stretch scan time unnecessarily.
Running fetches one-by-one makes total latency roughly sum of each request. A small bounded parallelism would keep scan time predictable.
Suggested patch
- for (const candidate of candidates) { - results.set(candidate.name, await fetcher(candidate.name)); - } + await Promise.all( + candidates.map(async (candidate) => { + results.set(candidate.name, await fetcher(candidate.name)); + }) + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runners/npmRegistryMetadata.ts` around lines 269 - 271, The current loop performs registry fetches sequentially by awaiting fetcher(candidate.name) inside the for-loop, which slows scans; change this to bounded parallelism: create promises for candidates using fetcher(candidate.name) but run them with a concurrency limit (e.g., use a batching loop, Promise.allSettled on slices, or a p-limit helper) and then populate results.set(candidate.name, ...) from the settled results; reference the existing identifiers candidates, fetcher, and results and ensure errors from failed promises are handled/logged and do not block other results.README.md (1)
39-39: ⚡ Quick winConsider breaking down the long feature description.
Line 39 packs many distinct signal types into a single sentence (>200 characters), which may be harder for users to parse. The phrase "capped registry metadata heuristics for already suspicious packages" could also be clearer.
📝 Suggested simplification
-- **Local and targeted supply-chain review signals** — flags git/local/tarball sources, missing integrity, unexpected registry hosts, install-time behavior, bounded local execution capability signals, packaging signals, optional npm signature/provenance verification, and capped registry metadata heuristics for already suspicious packages +- **Local and targeted supply-chain review signals** — flags git/local/tarball sources, missing integrity, unexpected registry hosts, install-time behavior (lifecycle hooks), local execution capability signals, packaging signals, optional npm signature/provenance verification, and targeted registry metadata enrichment for suspicious packages (capped to 10 lookups)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` at line 39, The long bullet starting with "**Local and targeted supply-chain review signals**" is overloaded and hard to parse; split it into 3–4 shorter bullets or sentences each describing one signal group (e.g., source flags like git/local/tarball; integrity/registry and host anomalies; install-time and bounded local execution signals; packaging and optional npm signature/provenance verification). Replace the phrase "capped registry metadata heuristics for already suspicious packages" with a clearer wording such as "limited registry-metadata heuristics applied only to packages already flagged as suspicious" and ensure each new line is concise and parallel in structure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/aggregator.ts`:
- Around line 1923-1943: isInspectableSourceFile currently rejects extensionless
entry/bin targets so readInspectablePackageFile skips them; update
isInspectableSourceFile (and any checks in readInspectablePackageFile and
looksMinified usage) to treat extensionless paths as inspectable when the target
file exists and is a text/javascript-like file: detect lack of extension via
path.extname(filePath) === '' and then stat the resolvedPath, read a small head
of the file, ensure it's not binary (check for null bytes or use a simple text
heuristic) and that looksMinified(text) is false before returning true; ensure
readInspectablePackageFile no longer filters out extensionless filePath values
but still enforces the resolvedPath sandbox check and maxBytes limits; apply the
same change to the other similar checksite referenced in the review.
In `@src/cli.test.ts`:
- Around line 598-618: The test's fake npm executable (created at fakeNpmPath)
lacks a Windows wrapper so spawn('npm', ...) fails on Windows; add creation of
an npm.cmd file in the same binDir alongside the fakeNpmPath that forwards all
arguments to the Node script (the fake npm file) so PATHEXT resolution finds it
on Windows; ensure the wrapper is written to path.join(binDir, 'npm.cmd') and
made executable/accessible before writing package.json so tests invoke the fake
npm on Windows as well as UNIX.
In `@src/cli.ts`:
- Around line 2128-2140: The targeted registry enrichment call
enrichAggregatedWithRegistryMetadata inside the opts.outdated branch can throw
and currently will abort executeAnalysis; wrap that call in a try/catch so
failures are non-fatal: on error log or spinner.warn a concise message including
the error, set a safe default (e.g. registryEnrichment with succeeded:0 and
attempted:0) and continue so buildDependencyFindings and the rest of
executeAnalysis still run; ensure you still call buildDependencyFindings only
when registryEnrichment.attempted > 0 as before.
In `@src/runners/npmRegistryMetadata.ts`:
- Around line 189-193: The current candidate capping sorts by package name using
byName.entries(), which lets alphabetical ordering exclude higher-risk packages;
change the sort to prioritize suspiciousness (e.g., sort entries by reasons.size
or a computed risk score descending) and only use name.localeCompare as a
tie-breaker—update the pipeline that starts with Array.from(byName.entries())
and the subsequent .sort(...) so .slice(0, limit) keeps the top N most
suspicious packages before mapping to { name, reasons }.
---
Outside diff comments:
In `@src/cli.ts`:
- Around line 1352-1355: The --fail-on option help text is out of date and
doesn't advertise the new compare-mode "new-*" delta rules accepted by
parseFailOnRules(); update the CLI option help string (the --fail-on description
in src/cli.ts where the option is declared) to list the corresponding new-*
rules (e.g., new-production-vuln, new-high-severity-vuln, new-licence-mismatch,
new-copyleft-detected, new-unknown-licence, new-supply-chain-source) alongside
the existing rules so terminal help reflects parseFailOnRules() supported values
and formatting remains consistent with the existing multi-line description.
---
Nitpick comments:
In `@README.md`:
- Line 39: The long bullet starting with "**Local and targeted supply-chain
review signals**" is overloaded and hard to parse; split it into 3–4 shorter
bullets or sentences each describing one signal group (e.g., source flags like
git/local/tarball; integrity/registry and host anomalies; install-time and
bounded local execution signals; packaging and optional npm signature/provenance
verification). Replace the phrase "capped registry metadata heuristics for
already suspicious packages" with a clearer wording such as "limited
registry-metadata heuristics applied only to packages already flagged as
suspicious" and ensure each new line is concise and parallel in structure.
In `@src/runners/npmRegistryMetadata.ts`:
- Around line 269-271: The current loop performs registry fetches sequentially
by awaiting fetcher(candidate.name) inside the for-loop, which slows scans;
change this to bounded parallelism: create promises for candidates using
fetcher(candidate.name) but run them with a concurrency limit (e.g., use a
batching loop, Promise.allSettled on slices, or a p-limit helper) and then
populate results.set(candidate.name, ...) from the settled results; reference
the existing identifiers candidates, fetcher, and results and ensure errors from
failed promises are handled/logged and do not block other results.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d534c3c4-3dd3-4398-a6c9-9af92e3c084c
📒 Files selected for processing (15)
README.mdreport-ui/main.tsreport-ui/types.tssrc/aggregator.test.tssrc/aggregator.tssrc/cli.test.tssrc/cli.tssrc/explain.tssrc/failOn.test.tssrc/failOn.tssrc/findings.tssrc/report-assets.tssrc/runners/npmRegistryMetadata.test.tssrc/runners/npmRegistryMetadata.tssrc/types.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
108-108:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClarify
--offlinebehavior in the options tableLine 108 is now incomplete relative to the rest of this README: it says
--offlineskips onlynpm audit/npm outdated, but later sections also state signature checks and targeted registry enrichment are skipped.Suggested doc fix
-| `--offline` | Skip `npm audit` and `npm outdated` (useful for offline/air-gapped scans) | +| `--offline` | Skip registry-dependent checks (`npm audit`, `npm outdated`, optional signature checks, and targeted registry enrichment) (useful for offline/air-gapped scans) |🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` at line 108, Update the README options table entry for `--offline` to reflect its full behavior: change the short description to say it skips npm audit and npm outdated as well as signature verification checks and targeted registry enrichment (used for registry lookups), and ensure wording matches later sections by using the exact flag name `--offline` and mentioning "signature checks" and "targeted registry enrichment" so the table and detailed docs are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/aggregator.ts`:
- Around line 1563-1589: normalizeBundledDependencies currently only handles
array forms and misses the case where pkg.bundleDependencies or
pkg.bundledDependencies is the boolean true (meaning “bundle all deps”); update
normalizeBundledDependencies to detect when either field is exactly true and
return a sentinel entry (e.g. ['*'] or another agreed-upon string) so
derivePackagingInfo will still add the 'bundled-dependencies' signal; keep the
existing trimming/uniq/sort behavior for array inputs and ensure
derivePackagingInfo continues to add bundledDependencies when the returned array
has length > 0.
- Around line 1884-1904: packageEntryTargets currently only handles string and
one-level object exports and ignores arrays and deeper nesting; update
packageEntryTargets to recursively traverse pkg.exports (and any nested
objects/arrays) to collect all string export paths: implement a helper (or make
add accept arrays/objects) that, given a value, if it's a string calls
normalizePackageRelativePath and adds it, if it's an array iterates and
recurses, and if it's an object iterates Object.values and recurses; keep using
normalizePackageRelativePath and the existing fallback to add 'index.js' when no
targets found.
---
Outside diff comments:
In `@README.md`:
- Line 108: Update the README options table entry for `--offline` to reflect its
full behavior: change the short description to say it skips npm audit and npm
outdated as well as signature verification checks and targeted registry
enrichment (used for registry lookups), and ensure wording matches later
sections by using the exact flag name `--offline` and mentioning "signature
checks" and "targeted registry enrichment" so the table and detailed docs are
consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d2aca15b-bd15-4775-acd4-b6bacb1d9fa8
📒 Files selected for processing (7)
README.mdsrc/aggregator.test.tssrc/aggregator.tssrc/cli.test.tssrc/cli.tssrc/runners/npmRegistryMetadata.test.tssrc/runners/npmRegistryMetadata.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/runners/npmRegistryMetadata.test.ts
- src/aggregator.test.ts
- src/runners/npmRegistryMetadata.ts
- src/cli.ts
- src/cli.test.ts
Contributor
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @JosephMaynard. The following files were modified: * `report-ui/main.ts` * `src/aggregator.ts` * `src/cli.ts` * `src/explain.ts` * `src/failOn.ts` * `src/findings.ts` * `src/runners/npmRegistryMetadata.ts` These files were ignored: * `src/aggregator.test.ts` * `src/cli.test.ts` * `src/failOn.test.ts` * `src/runners/npmRegistryMetadata.test.ts` These file types are not supported: * `README.md`
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/findings.ts (1)
184-199: ⚡ Quick winDeduplicate registry signals before emitting findings.
If
dep.supplyChain.registry.signalscontains duplicates, this loop emits duplicate findings with the sameid. Deduping avoids noisy output and potential downstream key collisions.♻️ Proposed fix
- for (const signal of dep.supplyChain?.registry?.signals || []) { + for (const signal of new Set(dep.supplyChain?.registry?.signals || [])) { const registry = dep.supplyChain?.registry; findings.push(baseFinding(dep, `registry-${signal}`, {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/findings.ts` around lines 184 - 199, The loop over dep.supplyChain?.registry?.signals can emit duplicate findings; deduplicate the signals before creating findings to avoid duplicate ids. Replace iterating directly over dep.supplyChain?.registry?.signals with a de-duplicated list (e.g., Array.from(new Set(...)) or other stable-ordered dedupe) and then iterate that list when calling baseFinding and pushing to findings; keep using REGISTRY_SIGNAL_TITLES, baseFinding, registry variable, and the same evidence/recommendation logic so behavior is unchanged except duplicates are removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/findings.ts`:
- Around line 184-199: The loop over dep.supplyChain?.registry?.signals can emit
duplicate findings; deduplicate the signals before creating findings to avoid
duplicate ids. Replace iterating directly over
dep.supplyChain?.registry?.signals with a de-duplicated list (e.g.,
Array.from(new Set(...)) or other stable-ordered dedupe) and then iterate that
list when calling baseFinding and pushing to findings; keep using
REGISTRY_SIGNAL_TITLES, baseFinding, registry variable, and the same
evidence/recommendation logic so behavior is unchanged except duplicates are
removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fe325d47-836b-4249-bb54-312d42d4cc6c
📒 Files selected for processing (7)
report-ui/main.tssrc/aggregator.tssrc/cli.tssrc/explain.tssrc/failOn.tssrc/findings.tssrc/runners/npmRegistryMetadata.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/explain.ts
- src/cli.ts
- src/failOn.ts
- report-ui/main.ts
- src/aggregator.ts
- src/runners/npmRegistryMetadata.ts
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
1 participant
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.
Summary
Testing
Summary by CodeRabbit
New Features
Documentation
Tests