Repository navigation
Add supply-chain signals, schema output, and workspace filtering - #27
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds SARIF/CycloneDX/SPDX outputs, Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI
participant LockfileSig as LockfileSignals
participant Aggregator as Aggregator
participant Findings as FindingsBuilder
participant Renderer as OutputRenderer
participant FS as Filesystem
CLI->>LockfileSig: runLockfileSupplyChainSignals(projectPath,tempDir,opts)
LockfileSig->>LockfileSig: parse lockfiles (npm/pnpm/yarn/bun), emit signals, run signature audit?
LockfileSig-->>CLI: ToolResult { signals[], signatureAudit }
CLI->>Aggregator: aggregateData(scanData, supplyChainResult, targetNodeMajor)
Aggregator->>Aggregator: normalize supplyChain, set schemaVersion "1.4", compute targetNodeCompatible
Aggregator->>Findings: buildDependencyFindings(aggregated, {targetNodeMajor})
Findings-->>Aggregator: DependencyFinding[]
Aggregator->>Renderer: render(format, aggregatedData)
Renderer->>FS: write output file(s) (SARIF/CycloneDX/SPDX/JSON/HTML)
Renderer-->>CLI: file paths / stdout
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 13
🧹 Nitpick comments (7)
src/runners/importGraphRunner.ts (1)
7-20: Trim redundant dot-prefixed ignore entries (or make the check order intentional).At Line 85, dot-prefixed entries are skipped before directory filtering, so entries like
.git,.yarn,.pnpm-store,.next,.nuxt,.svelte-kit, and.dependency-radarinIGNORED_DIRSare currently redundant. Consider simplifying the set to avoid drift.Also applies to: 85-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runners/importGraphRunner.ts` around lines 7 - 20, The IGNORED_DIRS set contains redundant dot-prefixed entries that are already excluded earlier by the hidden-entry check; update IGNORED_DIRS in importGraphRunner.ts to remove dot-prefixed names (e.g., '.git', '.yarn', '.pnpm-store', '.next', '.nuxt', '.svelte-kit', '.dependency-radar') or alternatively adjust the traversal logic so the dot-prefix check runs after the set membership test; modify the IGNORED_DIRS declaration (symbol: IGNORED_DIRS) to only include non-dot directory names or reorder the hidden-entry filter used by the directory scanning function so there is no duplication.src/runners/npmLs.test.ts (1)
264-275: Add real-worldbun.lockfixtures with JSONC features to catch parser gaps.The fixture at line 264 is strict JSON created with
JSON.stringify(). The official Bun v1.2+bun.lockformat is JSONC, supporting comments and trailing commas. While the code strips comments viastripJsonComments(), it does not handle trailing commas—a feature that real Bun lockfiles can contain. The current test only validates that strict JSON fixtures work, leaving the parser's gap with trailing commas undetected. Add at least one fixture with trailing commas or pull an actualbun installoutput to ensure the parser handles real-world Bun lockfiles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runners/npmLs.test.ts` around lines 264 - 275, The test writes a strict JSON bun.lock via JSON.stringify which misses real-world JSONC features; update the test in npmLs.test.ts that writes to path.join(projectPath, 'bun.lock') to use a raw string fixture containing JSONC features (comments and trailing commas) instead of JSON.stringify, or add a new bun.lock fixture with comments and trailing commas (and optionally a real bun install output) so the parser path exercised by stripJsonComments() plus the bun lock parsing logic handles trailing commas; locate the write in the test (the fs.writeFile call creating 'bun.lock') and replace or add the fixture string accordingly.src/outputFormats.ts (2)
60-67: SARIF locations use hardcodedpackage.jsonwhich may not reflect actual finding location.All findings point to
package.jsonline 1, regardless of where the issue originates (e.g., lockfile, transitive dependency). While SARIF allows this, it reduces the actionability of findings in IDE integrations.This is acceptable for v1 but consider enriching locations in future iterations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/outputFormats.ts` around lines 60 - 67, The SARIF output currently hardcodes every finding's location to package.json line 1 (see the locations -> physicalLocation -> artifactLocation block), which makes IDE integrations non-actionable; update the code that builds SARIF locations to use the actual source file and line (for example derive artifactLocation.uri and region.startLine from the finding metadata such as sourceFile/sourcePath and startLine in the finding object) and fall back to package.json line 1 only when no precise location is available so existing consumers keep working.
110-116: CycloneDX dependencies mapping has complex type casting that may silently produce incorrect output.The chain
.flatMap(...).map((entry) => entry[1])assumesSubDependencyEntryis[string, string | null], but the intermediateObject.values(group || {})returnsunknown[]due to theas Array<...>cast. If the actual shape differs, this will silently produce incorrect data.Consider adding validation or simplifying:
♻️ Safer extraction
dependencies: dependencies.map((dep) => ({ ref: dep.package.id, - dependsOn: Object.values(dep.graph.subDeps || {}) - .flatMap((group) => Object.values(group || {}) as Array<[string, string | null]>) - .map((entry) => entry[1]) - .filter((resolved): resolved is string => Boolean(resolved)) + dependsOn: Object.values(dep.graph.subDeps || {}) + .flatMap((group) => Object.values(group || {})) + .map((entry) => Array.isArray(entry) ? entry[1] : null) + .filter((resolved): resolved is string => typeof resolved === 'string' && resolved.length > 0) }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/outputFormats.ts` around lines 110 - 116, The CycloneDX dependencies mapping in dependencies: dependencies.map(...) uses an unsafe cast to Array<[string, string | null]> when reading dep.graph.subDeps which can silently produce wrong refs; update the extraction to validate shapes instead of casting: iterate Object.values(dep.graph.subDeps || {}), then for each group iterate its Object.values entries and use a type guard (e.g., check Array.isArray(entry) && entry.length >= 2 && (entry[1] === null || typeof entry[1] === 'string')) before mapping entry[1], then filter to keep only strings; apply this validation where dep.graph.subDeps is referenced (the dependencies mapping block) to ensure only bona fide string refs are emitted.src/runners/lockfileSignals.ts (3)
240-266: MoveReturnTypePlaceholderdefinition before its first usage.
ReturnTypePlaceholderis defined at lines 259-266 but first referenced at line 240. While TypeScript allows forward references to type aliases, placing the type definition after its usage reduces readability.♻️ Move type definition before runNpmAuditSignatures
+type SignatureAuditResult = { + attempted: boolean; + ok: boolean; + output?: string; + error?: string; +}; + -async function runNpmAuditSignatures(projectPath: string): Promise<NonNullable<ReturnTypePlaceholder['signatureAudit']>> { +async function runNpmAuditSignatures(projectPath: string): Promise<SignatureAuditResult> { try { ... } } -type ReturnTypePlaceholder = { - signatureAudit?: { - attempted: boolean; - ok: boolean; - output?: string; - error?: string; - }; -};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runners/lockfileSignals.ts` around lines 240 - 266, Move the type alias ReturnTypePlaceholder so it is declared before its first usage in runNpmAuditSignatures; specifically, cut the ReturnTypePlaceholder definition and paste it above the async function runNpmAuditSignatures so the signatureAudit type is defined prior to being referenced, which improves readability and keeps type declarations organized.
15-19: Comment-stripping regex may incorrectly strip content within strings.The regex on line 18
/(^|[^:])\/\/.*$/gmattempts to preserve://in URLs but will still incorrectly strip content in strings like"foo // bar"→"foo. Sincebun.lockcan contain string values with arbitrary content, this could corrupt JSON parsing.Consider using a proper JSON5/JSONC parser or only stripping comments outside of string literals.
♻️ Safer approach: only strip if bun.lock format guarantees no inline comments in strings
function stripJsonComments(raw: string): string { - return raw - .replace(/\/\*[\s\S]*?\*\//g, '') - .replace(/(^|[^:])\/\/.*$/gm, '$1'); + // Bun lockfiles use block comments only at the top; line comments appear + // outside string values. A full JSONC parser would be safer if format evolves. + return raw + .replace(/\/\*[\s\S]*?\*\//g, '') + .replace(/^(\s*)\/\/.*$/gm, '$1'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runners/lockfileSignals.ts` around lines 15 - 19, stripJsonComments currently uses a regex (in function stripJsonComments) that can remove content inside string literals (e.g., "foo // bar"); replace this with a safe approach: either (a) switch to a proper JSONC/JSON5 parser or comment-stripping library (e.g., jsonc-parser or strip-json-comments) and call that from stripJsonComments, or (b) implement a simple state-machine in stripJsonComments that walks the input and only removes // and /* */ comments when not inside string literals (handling escapes and both single/double quotes). Update the function to use the chosen safe method so bun.lock strings are preserved and JSON parsing is not corrupted.
112-116: Consider adding a minimal type for the lockfile object parameter.Using
anyforobjbypasses type checking. A minimal interface would catch typos and improve maintainability.♻️ Add minimal type definition
+interface NpmLockfileShape { + packages?: Record<string, { resolved?: string; integrity?: string; version?: string }>; + dependencies?: Record<string, { resolved?: string; integrity?: string; version?: string }>; +} + function inspectNpmLockObject( - obj: any, + obj: NpmLockfileShape | null | undefined, sourceFile: string, expectedHosts: Set<string> ): SupplyChainSignal[] {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runners/lockfileSignals.ts` around lines 112 - 116, The parameter obj in inspectNpmLockObject is typed as any; define a minimal interface (e.g., NpmLockfile or NpmLockObject) that includes only the properties the function uses (for example packages?: Record<string, { resolved?: string; integrity?: string }>, dependencies?: Record<string, string>, and any index signatures you access), replace the obj: any signature with obj: NpmLockObject, and update any property access within inspectNpmLockObject to match the new type (adjust optional chaining or null checks as needed) so TypeScript catches mismatches and typos.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 310: The Bun table row currently has two blank cells which are ambiguous;
update the row that starts with "Bun" (the cell mentioning "Text `bun.lock`
parsing; binary `bun.lockb` is reported with a migration hint") and replace the
empty Audit and Outdated cells with explicit values (e.g., set Audit to "N/A" if
auditing isn't supported and set Outdated to "⚠️" or "❌" as appropriate) so the
table shows clear, non-empty statuses.
In `@src/aggregator.ts`:
- Around line 625-647: The current isNodeEngineTargetCompatible implementation
incorrectly interprets comparator-only clauses (e.g., "<20", ">18") by
extracting literal majors instead of evaluating comparators; update
isNodeEngineTargetCompatible to use a comparator-aware check (preferably
leveraging semver.Range/semver.satisfies) by converting targetMajor into a full
semver string (e.g., `${targetMajor}.0.0`) and evaluating the original range
string with semver.satisfies(range, version) (or implement equivalent comparator
parsing that honors <, <=, >, >=, =, ~, ^, hyphen and || semantics) so
comparator-only ranges behave correctly for the range and targetMajor
parameters.
In `@src/cli.test.ts`:
- Around line 469-480: Replace the synthetic JSON generation that writes
bun.lock using JSON.stringify with a real bun.lock fixture string that contains
JSONC-style trailing commas (and optional comments) so the test exercises the
actual parser behavior; update the fs.writeFile call in src/cli.test.ts (where
projectPath is used to write 'bun.lock') to write a raw bun.lock sample (copied
from a real Bun example) that includes trailing commas, ensuring the code paths
that call stripJsonComments() and the bun lockfile parser are exercised and will
fail if trailing-comma handling is incorrect.
In `@src/compare.ts`:
- Around line 40-47: The newFindings and resolvedFindings arrays are not sorted,
causing nondeterministic output; update the return in compare.ts to sort both
(e.g., by finding.id — use (a.id || '').localeCompare(b.id) or numeric compare
if ids are numeric) after filtering so the function returns deterministically
ordered newFindings and resolvedFindings; reference the existing
previousFindingIds, currentFindingIds, newFindings, and resolvedFindings
variables when applying the sort.
In `@src/findings.ts`:
- Around line 17-43: parseSupportedNodeMajors is misclassifying comparator-only
ranges (e.g. ">=18") as only the exact major and diverges from
upgrade.targetNodeCompatible; instead of maintaining two parsers, update
supportsTargetNode/parseSupportedNodeMajors to reuse the single compatibility
check used in src/aggregator.ts (upgrade.targetNodeCompatible) or extract that
logic into a shared function and import it here; replace the current
parseSupportedNodeMajors usage in supportsTargetNode with a call to the shared
target-compatibility function so that comparator-only ranges (">=x", "<y",
open-ended bounds) are handled consistently and return the correct
boolean/undefined results.
- Around line 70-74: The title string passed into baseFinding currently builds
"vulnerability" + (vulnCount === 1 ? '' : 'ies') which produces
"vulnerabilityies" for counts >1; update the title expression in the call to
baseFinding (the title argument near vulnCount) to use a correct pluralization,
e.g. choose "vulnerability" when vulnCount === 1 and "vulnerabilities"
otherwise, ensuring the message still uses vulnCount at the front (e.g.
`${vulnCount} ${vulnCount === 1 ? 'vulnerability' : 'vulnerabilities'}`).
In `@src/outputFormats.ts`:
- Around line 5-14: The purl function currently strips the leading '@' from
scoped npm package names which violates the purl spec; update purl (function
purl, parameter DependencyRecord) to percent-encode the '@' as part of the
namespace so scoped names become "%40scope/name". Concretely, when
dep.package.name.startsWith('@'), prepend "%40" instead of removing the '@'
before splitting and encoding the remaining segments, then join and append the
encoded version as before so the returned string follows the purl npm spec.
- Line 150: The documentNamespace currently uses Date.now(), producing
non-deterministic SPDX output; replace that with a stable, input-derived
identifier by computing a deterministic hash (e.g., SHA-256) of stable inputs
such as project name, version, and generatedAt (or other canonicalized input
data) and assign documentNamespace to a fixed URI like
`https://www.dependency-radar.com/spdx/${hash}`; implement the hash computation
where SPDX output is assembled (referencing documentNamespace in
src/outputFormats.ts) and ensure a deterministic canonicalization order and a
sensible fallback when any input fields are missing.
In `@src/report.ts`:
- Around line 247-250: The workspace filter select with id "workspace-filter" is
missing a programmatic label; replace the visual-only <span
class="filter-label">Workspace</span> with a proper <label
for="workspace-filter">Workspace</label> (preserving the "filter-label" class
and placement inside the element with class "filter-group
workspace-filter-group") so screen readers correctly associate the label with
the select element.
In `@src/runners/lockfileGraph.ts`:
- Around line 128-136: buildBunJsonResolvedTree/buildBunJsonNode currently drop
the requested selector (name@ref) and memoize/track by bare package name,
causing findBunPackageEntry to resolve the wrong version when bun.lock has
multiple versions; change the recursion to carry the full selector (e.g.,
"name@specifier") into buildBunJsonNode, use that selector as the key for memo
and stack (instead of dep name), and use that selector when looking up entries
in findBunPackageEntry so the exact requested ref is resolved and the correct
subtree attached; update the loop in buildBunJsonResolvedTree to call
buildBunJsonNode with the full spec from collectPackageJsonDependencySpecs and
ensure dependencies are indexed by the resolved node.name but
memoization/cycle-detection use the selector.
In `@src/runners/lockfileSignals.ts`:
- Around line 283-287: The current conditional sets signatureAudit to {
attempted: false, ok: true, error: 'skipped (--offline)' } when
options.auditSignatures is true but options.offline is true, which incorrectly
signals success; change the skipped branch in the signatureAudit assignment (the
ternary that uses runNpmAuditSignatures, options.auditSignatures and
options.offline) so the returned object clearly indicates a skipped state (for
example set ok: false or add status: 'skipped' and attempted: false) instead of
ok: true, and update any consumers of signatureAudit that rely on ok to handle
the explicit skipped state consistently; reference runNpmAuditSignatures,
signatureAudit, options.auditSignatures and options.offline when making the
change.
In `@src/schema.ts`:
- Around line 42-50: The schema's SupplyChainSignal.type enum incorrectly
includes the signature-audit values; remove "signature-verification-failed" and
"signature-verification-unavailable" from the signals[].type enum in
src/schema.ts so that signals[].type only contains supply-chain signal values
(e.g., 'git-dependency', 'file-dependency', 'non-registry-tarball',
'missing-integrity', 'unexpected-registry-host'), and ensure any signature
verification data remains modeled under supplyChain.signatureAudit (leave
supplyChain.signatureAudit untouched).
In `@src/utils.ts`:
- Around line 43-60: The current collect function enforces maxOutputBytes
separately for stdout and stderr, allowing combined output to exceed the limit;
modify the logic to track and enforce a single combined byte counter (e.g., add
totalBytes variable) and have collect accept/update that shared total (or
compute total = stdoutBytes + stderrBytes before pushing) so both stdout and
stderr event handlers call collect with and update the shared totalBytes; ensure
outputExceeded is set and child.kill('SIGTERM') is invoked when the shared total
reaches maxOutputBytes, and continue pushing only the remaining bytes from the
current chunk into stdoutChunks or stderrChunks as appropriate.
---
Nitpick comments:
In `@src/outputFormats.ts`:
- Around line 60-67: The SARIF output currently hardcodes every finding's
location to package.json line 1 (see the locations -> physicalLocation ->
artifactLocation block), which makes IDE integrations non-actionable; update the
code that builds SARIF locations to use the actual source file and line (for
example derive artifactLocation.uri and region.startLine from the finding
metadata such as sourceFile/sourcePath and startLine in the finding object) and
fall back to package.json line 1 only when no precise location is available so
existing consumers keep working.
- Around line 110-116: The CycloneDX dependencies mapping in dependencies:
dependencies.map(...) uses an unsafe cast to Array<[string, string | null]> when
reading dep.graph.subDeps which can silently produce wrong refs; update the
extraction to validate shapes instead of casting: iterate
Object.values(dep.graph.subDeps || {}), then for each group iterate its
Object.values entries and use a type guard (e.g., check Array.isArray(entry) &&
entry.length >= 2 && (entry[1] === null || typeof entry[1] === 'string')) before
mapping entry[1], then filter to keep only strings; apply this validation where
dep.graph.subDeps is referenced (the dependencies mapping block) to ensure only
bona fide string refs are emitted.
In `@src/runners/importGraphRunner.ts`:
- Around line 7-20: The IGNORED_DIRS set contains redundant dot-prefixed entries
that are already excluded earlier by the hidden-entry check; update IGNORED_DIRS
in importGraphRunner.ts to remove dot-prefixed names (e.g., '.git', '.yarn',
'.pnpm-store', '.next', '.nuxt', '.svelte-kit', '.dependency-radar') or
alternatively adjust the traversal logic so the dot-prefix check runs after the
set membership test; modify the IGNORED_DIRS declaration (symbol: IGNORED_DIRS)
to only include non-dot directory names or reorder the hidden-entry filter used
by the directory scanning function so there is no duplication.
In `@src/runners/lockfileSignals.ts`:
- Around line 240-266: Move the type alias ReturnTypePlaceholder so it is
declared before its first usage in runNpmAuditSignatures; specifically, cut the
ReturnTypePlaceholder definition and paste it above the async function
runNpmAuditSignatures so the signatureAudit type is defined prior to being
referenced, which improves readability and keeps type declarations organized.
- Around line 15-19: stripJsonComments currently uses a regex (in function
stripJsonComments) that can remove content inside string literals (e.g., "foo //
bar"); replace this with a safe approach: either (a) switch to a proper
JSONC/JSON5 parser or comment-stripping library (e.g., jsonc-parser or
strip-json-comments) and call that from stripJsonComments, or (b) implement a
simple state-machine in stripJsonComments that walks the input and only removes
// and /* */ comments when not inside string literals (handling escapes and both
single/double quotes). Update the function to use the chosen safe method so
bun.lock strings are preserved and JSON parsing is not corrupted.
- Around line 112-116: The parameter obj in inspectNpmLockObject is typed as
any; define a minimal interface (e.g., NpmLockfile or NpmLockObject) that
includes only the properties the function uses (for example packages?:
Record<string, { resolved?: string; integrity?: string }>, dependencies?:
Record<string, string>, and any index signatures you access), replace the obj:
any signature with obj: NpmLockObject, and update any property access within
inspectNpmLockObject to match the new type (adjust optional chaining or null
checks as needed) so TypeScript catches mismatches and typos.
In `@src/runners/npmLs.test.ts`:
- Around line 264-275: The test writes a strict JSON bun.lock via JSON.stringify
which misses real-world JSONC features; update the test in npmLs.test.ts that
writes to path.join(projectPath, 'bun.lock') to use a raw string fixture
containing JSONC features (comments and trailing commas) instead of
JSON.stringify, or add a new bun.lock fixture with comments and trailing commas
(and optionally a real bun install output) so the parser path exercised by
stripJsonComments() plus the bun lock parsing logic handles trailing commas;
locate the write in the test (the fs.writeFile call creating 'bun.lock') and
replace or add the fixture string accordingly.
🪄 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: b7eb08ff-4770-49fd-a5d6-a190f082e30e
⛔ Files ignored due to path filters (23)
dist/aggregator.jsis excluded by!**/dist/**dist/cli.jsis excluded by!**/dist/**dist/compare.jsis excluded by!**/dist/**dist/failOn.jsis excluded by!**/dist/**dist/findings.jsis excluded by!**/dist/**dist/generated/spdx.jsis excluded by!**/dist/**,!**/generated/**dist/outputFormats.jsis excluded by!**/dist/**dist/report-assets.jsis excluded by!**/dist/**dist/report.jsis excluded by!**/dist/**dist/runners/depcheckRunner.jsis excluded by!**/dist/**dist/runners/importGraphRunner.jsis excluded by!**/dist/**dist/runners/licenseChecker.jsis excluded by!**/dist/**dist/runners/lockfileGraph.jsis excluded by!**/dist/**dist/runners/lockfileSignals.jsis excluded by!**/dist/**dist/runners/madgeRunner.jsis excluded by!**/dist/**dist/runners/npmLs.jsis excluded by!**/dist/**dist/schema.jsis excluded by!**/dist/**dist/utils.jsis excluded by!**/dist/**dist/why.jsis excluded by!**/dist/**dist/workspaceFilter.jsis excluded by!**/dist/**report-ui/dist/report.cssis excluded by!**/dist/**report-ui/dist/report.iife.jsis excluded by!**/dist/**src/generated/spdx.tsis excluded by!**/generated/**
📒 Files selected for processing (29)
README.mdpackage.jsonreport-ui/main.tsreport-ui/style.cssreport-ui/types.tssrc/aggregator.tssrc/cli.test.tssrc/cli.tssrc/compare.tssrc/failOn.test.tssrc/failOn.tssrc/findings.test.tssrc/findings.tssrc/outputFormats.tssrc/packageManifest.test.tssrc/report-assets.tssrc/report.tssrc/runners/importGraphRunner.tssrc/runners/lockfileGraph.tssrc/runners/lockfileSignals.test.tssrc/runners/lockfileSignals.tssrc/runners/npmLs.test.tssrc/runners/npmLs.tssrc/schema.tssrc/types.tssrc/utils.tssrc/why.tssrc/workspaceFilter.test.tssrc/workspaceFilter.ts
Comment on lines
+625
to
+647
| function isNodeEngineTargetCompatible(range: string, targetMajor: number): boolean { | ||
| const normalized = range.trim(); | ||
| if (!normalized || normalized === '*' || normalized.toLowerCase() === 'x') return true; | ||
| const clauses = normalized.split('||').map((clause) => clause.trim()).filter(Boolean); | ||
| if (clauses.length === 0) return true; | ||
| return clauses.some((clause) => { | ||
| const lower = clause.match(/>=\s*v?(\d+)/); | ||
| const upper = clause.match(/<\s*v?(\d+)/); | ||
| if (lower && upper) { | ||
| const min = Number.parseInt(lower[1], 10); | ||
| const max = Number.parseInt(upper[1], 10); | ||
| return targetMajor >= min && targetMajor < max; | ||
| } | ||
| const majors = Array.from(clause.matchAll(/(?:^|[\s>=<~^])v?(\d+)/g)) | ||
| .map((match) => Number.parseInt(match[1], 10)) | ||
| .filter((major) => Number.isFinite(major)); | ||
| if (majors.length === 0) return true; | ||
| if (/^\s*[~^]?\s*v?\d+/.test(clause) && !/[<>]/.test(clause)) { | ||
| return majors.includes(targetMajor); | ||
| } | ||
| if (lower && !upper) return targetMajor >= Number.parseInt(lower[1], 10); | ||
| return majors.includes(targetMajor); | ||
| }); |
Contributor
There was a problem hiding this comment.
Comparator-only engine ranges are evaluated incorrectly here.
<20 currently marks Node 18 as incompatible and Node 20 as compatible, and >18 marks Node 19 as incompatible. That makes upgrade.targetNodeCompatible unreliable for common engines.node syntax. This needs a shared comparator-aware evaluator rather than extracting literal majors from the clause.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/aggregator.ts` around lines 625 - 647, The current
isNodeEngineTargetCompatible implementation incorrectly interprets
comparator-only clauses (e.g., "<20", ">18") by extracting literal majors
instead of evaluating comparators; update isNodeEngineTargetCompatible to use a
comparator-aware check (preferably leveraging semver.Range/semver.satisfies) by
converting targetMajor into a full semver string (e.g., `${targetMajor}.0.0`)
and evaluating the original range string with semver.satisfies(range, version)
(or implement equivalent comparator parsing that honors <, <=, >, >=, =, ~, ^,
hyphen and || semantics) so comparator-only ranges behave correctly for the
range and targetMajor parameters.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
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)
537-558:⚠️ Potential issue | 🟠 Major
detectScanManager()still prefers stale Yarn/npm lockfiles over Bun.A repo with
packageManager: "bun"plus a leftoveryarn.lockorpackage-lock.jsonwill never reach the Bun path here, because the earlier lockfile checks win first. That produces the wrong dependency graph for the migration/stale-lockfile case this PR is explicitly trying to support. Prefer the inferred manager when its lockfile exists, or at least check Bun before unrelated lockfiles whenfallback === "bun".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli.ts` around lines 537 - 558, detectScanManager currently returns "yarn" or "npm" when stale yarn/npm lockfiles exist even if the project is actually using Bun; reorder or add conditional checks so Bun lockfiles are considered before unrelated lockfiles when the inferred manager should be Bun. Specifically, inside detectScanManager check for Bun lockfiles (bun.lock or bun.lockb) and the bun node_modules marker early (or add an explicit branch: if (fallback === "bun") then check bun lockfiles first) so that functions referencing detectScanManager will return "bun" when Bun artifacts are present; update the logic around pathExists checks for bun.lock/bun.lockb and node_modules/.pnpm/.yarn-state.yml accordingly to ensure Bun is preferred when appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli.ts`:
- Around line 1670-1673: The code currently treats a user-supplied opts.out
equal to "dependency-radar.html" as if it were not provided and overwrites it
with defaultOutputName(opts.format); instead, detect whether --out was passed
explicitly (e.g., via the parsed options metadata or by checking argv) and only
replace the output when the user did not provide --out; update the logic around
opts.out, outputPath and the defaultOutputName(...) call to use this
explicit-flag check rather than comparing against the magic filename, and apply
the same explicit-presence fix to the new --schema path handling so
user-provided schema paths are never overwritten by a filename comparison.
In `@src/nodeEngine.ts`:
- Around line 48-57: isNodeEngineTargetCompatible currently tests only a single
concrete version [targetMajor,0,0], causing false negatives; change the logic to
treat the "target" as the interval [targetMajor.0.0, (targetMajor+1).0.0) and
test whether the parsed range overlaps that interval. Concretely, in
isNodeEngineTargetCompatible replace the single target tuple with minTarget =
[targetMajor,0,0] and maxTarget = [targetMajor+1,0,0], then for each clause use
expandToken to build comparators and determine if those comparators allow any
version in the interval (e.g., implement a small helper that, given comparators
from expandToken and the interval boundaries, returns true if the comparator set
does not exclude the whole interval — you can reuse satisfiesComparator by
checking it against the interval endpoints and/or reasoning about <=/>= bounds).
Update the final .some(...) check to use that interval-overlap helper instead of
satisfiesComparator(target, ...).
In `@src/outputFormats.ts`:
- Around line 144-148: The current license resolution uses declared?.valid to
pick declared.spdxId but doesn't ensure declared.spdxId is truthy; update the
logic around declared, inferred, and license (the variables derived from
dep.compliance.license) to require a non-empty declared.spdxId when
declared.valid is true, e.g. only use declared.spdxId if declared?.valid &&
declared.spdxId is truthy, otherwise fall back to inferred?.spdxId and then
'NOASSERTION'.
In `@src/runners/lockfileGraph.ts`:
- Around line 133-140: The code falls back to Yarn parsing when
readBunJsonLock(raw) fails, which masks Bun JSON parse errors; modify the logic
in the function around the readBunJsonLock call so that if the raw lock content
(raw) trimmed begins with '{' and readBunJsonLock returned falsy, you
immediately return undefined (do not attempt parseYarnV1/parseYarnV2); otherwise
continue to try the Yarn parsers and call buildYarnResolvedTree as before.
Reference: readBunJsonLock, parseYarnV1, parseYarnV2, buildBunJsonResolvedTree,
buildYarnResolvedTree (and leave runNpmLs to handle Bub JSON parse errors).
In `@src/utils.ts`:
- Around line 18-19: The timeoutMs and maxOutputBytes values are being set
directly from options or Number(process.env...) without validating the parsed
numeric result, which allows NaN to disable guards; update the initialization
logic for timeoutMs and maxOutputBytes in utils.ts to: parse the env value (or
options value if provided), check Number.isFinite(value) and value > 0 (or >= 0
if zero allowed), and only use it if valid—otherwise fall back to the existing
default constants (120000 and 50*1024*1024). Ensure you validate both the
options-provided values and the env-parsed values so timeoutMs and
maxOutputBytes are never NaN or non-finite; reference the timeoutMs and
maxOutputBytes variables when implementing these checks.
---
Outside diff comments:
In `@src/cli.ts`:
- Around line 537-558: detectScanManager currently returns "yarn" or "npm" when
stale yarn/npm lockfiles exist even if the project is actually using Bun;
reorder or add conditional checks so Bun lockfiles are considered before
unrelated lockfiles when the inferred manager should be Bun. Specifically,
inside detectScanManager check for Bun lockfiles (bun.lock or bun.lockb) and the
bun node_modules marker early (or add an explicit branch: if (fallback ===
"bun") then check bun lockfiles first) so that functions referencing
detectScanManager will return "bun" when Bun artifacts are present; update the
logic around pathExists checks for bun.lock/bun.lockb and
node_modules/.pnpm/.yarn-state.yml accordingly to ensure Bun is preferred when
appropriate.
🪄 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: c885592e-7072-4ae2-a0eb-4e289b71741d
⛔ Files ignored due to path filters (12)
dist/aggregator.jsis excluded by!**/dist/**dist/cli.jsis excluded by!**/dist/**dist/compare.jsis excluded by!**/dist/**dist/findings.jsis excluded by!**/dist/**dist/nodeEngine.jsis excluded by!**/dist/**dist/outputFormats.jsis excluded by!**/dist/**dist/report.jsis excluded by!**/dist/**dist/runners/importGraphRunner.jsis excluded by!**/dist/**dist/runners/lockfileGraph.jsis excluded by!**/dist/**dist/runners/lockfileSignals.jsis excluded by!**/dist/**dist/schema.jsis excluded by!**/dist/**dist/utils.jsis excluded by!**/dist/**
📒 Files selected for processing (19)
README.mdreport-ui/types.tssrc/aggregator.tssrc/cli.test.tssrc/cli.tssrc/compare.tssrc/findings.tssrc/nodeEngine.test.tssrc/nodeEngine.tssrc/outputFormats.tssrc/report.tssrc/runners/importGraphRunner.tssrc/runners/lockfileGraph.tssrc/runners/lockfileSignals.test.tssrc/runners/lockfileSignals.tssrc/runners/npmLs.test.tssrc/schema.tssrc/types.tssrc/utils.ts
✅ Files skipped from review due to trivial changes (1)
- src/aggregator.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/report.ts
- src/runners/importGraphRunner.ts
- src/runners/npmLs.test.ts
- src/runners/lockfileSignals.ts
- src/runners/lockfileSignals.test.ts
- src/cli.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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)
1827-1838:⚠️ Potential issue | 🟡 MinorSkipped registry collectors are reported as failures for Bun scans.
supportsRegistryCollectors(scanManager)correctly skipsrunPackageAudit/runPackageOutdatedfor Bun, but the later status checks still treat the resultingundefinedentries asunavailable. That makes Bun scans print failure-style audit/outdated messages even when nothing actually failed. Please track support/attempted state separately and report these as skipped instead.Also applies to: 1867-1901
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli.ts` around lines 1827 - 1838, The code path that gates runPackageAudit/runPackageOutdated with supportsRegistryCollectors(scanManager) returns undefined when collectors are unsupported, but downstream status checks treat undefined as an unavailable failure; adjust the flow so skipped collectors are explicitly represented and not treated as failures. Change the conditional that currently returns Promise.resolve(undefined) to instead return a ToolResult (or similar result shape used elsewhere) indicating a skipped status (e.g., { ok: true/false as your contract expects, status: 'skipped', error: undefined }) or a distinct marker object, and update the downstream status-check logic (the code that inspects these results around the audit/outdated reporting) to recognize this skipped marker rather than treating undefined as unavailable; use the existing functions/objects supportsRegistryCollectors, runPackageAudit, runPackageOutdated and ToolResult names to locate and update both the branch that creates the promise and the later reporting logic so Bun scans show "skipped" instead of failure-style messages.
♻️ Duplicate comments (1)
src/nodeEngine.ts (1)
40-47:⚠️ Potential issue | 🟠 MajorThe overlap check still uses two sample points instead of the full major interval.
This still produces false negatives for valid mid-major windows. For example,
>=18.500.0 <18.600.0should be compatible with target major18, but neither18.0.0nor18.999.999satisfies both comparators, so this returnsfalse. Please compute interval intersection directly from the comparator bounds instead of probing sentinel versions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nodeEngine.ts` around lines 40 - 47, comparatorsOverlapTargetMajor currently probes two sentinel versions and misses valid mid-major ranges; fix it by computing each comparator's effective lower/upper bounds and intersecting them with the target major interval instead of using satisfiesComparator on sample points. Update comparatorsOverlapTargetMajor to parse/combine bounds from each comparator (reuse/combine logic in comparatorAllowsTargetMajor or extract a helper that returns numeric lower/upper VersionTuple bounds for a comparator), intersect all comparator bounds to produce a final interval, and return true if that final interval intersects [targetMajor.0.0, targetMajor+1.0.0) (use inclusive/exclusive semantics consistent with existing comparator parsing). Ensure you still use comparatorAllowsTargetMajor, satisfiesComparator only as helpers if needed but base the decision on interval arithmetic rather than sentinel samples.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli.ts`:
- Around line 1676-1678: The default output path is currently resolved against
the caller CWD using path.resolve(opts.out) when no --out is provided; change it
so when opts.outProvided is false (and you're setting opts.out via
defaultOutputName(opts.format)) you resolve that default against projectPath
instead of the shell CWD. Concretely, in the branch that checks
!opts.outProvided && opts.format !== "html", keep assigning opts.out =
defaultOutputName(opts.format) but set outputPath = path.resolve(projectPath,
opts.out) (and ensure any downstream uses of outputPath/or opts.out expect this
repo-root anchored path). Use the existing symbols opts.outProvided,
opts.format, defaultOutputName, outputPath, projectPath to locate and update the
code.
---
Outside diff comments:
In `@src/cli.ts`:
- Around line 1827-1838: The code path that gates
runPackageAudit/runPackageOutdated with supportsRegistryCollectors(scanManager)
returns undefined when collectors are unsupported, but downstream status checks
treat undefined as an unavailable failure; adjust the flow so skipped collectors
are explicitly represented and not treated as failures. Change the conditional
that currently returns Promise.resolve(undefined) to instead return a ToolResult
(or similar result shape used elsewhere) indicating a skipped status (e.g., {
ok: true/false as your contract expects, status: 'skipped', error: undefined })
or a distinct marker object, and update the downstream status-check logic (the
code that inspects these results around the audit/outdated reporting) to
recognize this skipped marker rather than treating undefined as unavailable; use
the existing functions/objects supportsRegistryCollectors, runPackageAudit,
runPackageOutdated and ToolResult names to locate and update both the branch
that creates the promise and the later reporting logic so Bun scans show
"skipped" instead of failure-style messages.
---
Duplicate comments:
In `@src/nodeEngine.ts`:
- Around line 40-47: comparatorsOverlapTargetMajor currently probes two sentinel
versions and misses valid mid-major ranges; fix it by computing each
comparator's effective lower/upper bounds and intersecting them with the target
major interval instead of using satisfiesComparator on sample points. Update
comparatorsOverlapTargetMajor to parse/combine bounds from each comparator
(reuse/combine logic in comparatorAllowsTargetMajor or extract a helper that
returns numeric lower/upper VersionTuple bounds for a comparator), intersect all
comparator bounds to produce a final interval, and return true if that final
interval intersects [targetMajor.0.0, targetMajor+1.0.0) (use
inclusive/exclusive semantics consistent with existing comparator parsing).
Ensure you still use comparatorAllowsTargetMajor, satisfiesComparator only as
helpers if needed but base the decision on interval arithmetic rather than
sentinel samples.
🪄 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: 34e8b03e-2d74-4fa2-beae-948e94e490de
⛔ Files ignored due to path filters (5)
dist/cli.jsis excluded by!**/dist/**dist/nodeEngine.jsis excluded by!**/dist/**dist/outputFormats.jsis excluded by!**/dist/**dist/runners/lockfileGraph.jsis excluded by!**/dist/**dist/utils.jsis excluded by!**/dist/**
📒 Files selected for processing (6)
src/cli.tssrc/nodeEngine.test.tssrc/nodeEngine.tssrc/outputFormats.tssrc/runners/lockfileGraph.tssrc/utils.ts
✅ Files skipped from review due to trivial changes (1)
- src/runners/lockfileGraph.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/nodeEngine.test.ts
- src/utils.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/cli.ts (1)
1670-1684:⚠️ Potential issue | 🟠 MajorResolve the default output against
projectPath, not the caller CWD.
outputPathstill starts frompath.resolve(opts.out), soscan --project /other/repowrites the default HTML report in the shell's working directory instead of the scanned project root. That regresses the repo-root default behavior.Suggested fix
- let outputPath = path.resolve(opts.out); + let outputPath = opts.outProvided + ? path.resolve(opts.out) + : path.resolve(projectPath, opts.out);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli.ts` around lines 1670 - 1684, The code currently sets outputPath = path.resolve(opts.out) which causes the default report filename to be resolved against the caller CWD instead of the scanned project root; update the logic so when you assign the default (in the block checking !opts.outProvided && opts.format !== "html") you also set outputPath = path.resolve(projectPath, opts.out) (i.e., resolve opts.out against projectPath), and ensure the initial outputPath assignment uses opts.out only if opts.outProvided (or move the initial path.resolve(opts.out) after projectPath is available) so that projectPath, outputPath, opts.outProvided, opts.format and defaultOutputName are consistent.
🧹 Nitpick comments (1)
src/nodeEngine.test.ts (1)
4-20: Add a regression for malformed ranges.This suite covers the happy path, but it doesn't lock in the new fail-closed behavior for bad comparator tokens. A case like
>=18fooshould stay rejected once the parser is tightened.Suggested test
describe('isNodeEngineTargetCompatible', () => { + it('rejects malformed comparator tokens', () => { + expect(isNodeEngineTargetCompatible('>=18foo', 18)).toBe(false); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nodeEngine.test.ts` around lines 4 - 20, Add a regression test to ensure malformed comparator tokens are rejected: update the isNodeEngineTargetCompatible test suite to include a case such as isNodeEngineTargetCompatible('>=18foo', 18) and assert it returns false (or otherwise fails closed). Locate the tests around the existing describe('isNodeEngineTargetCompatible', ...) block and add an it() that verifies malformed ranges like '>=18foo' (and optionally other malformed strings) do not pass; reference the isNodeEngineTargetCompatible function in the assertion so the new test protects the tightened parser behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/nodeEngine.ts`:
- Around line 13-25: satisfiesComparator currently uses a regex that only
matches a prefix, allowing malformed strings like ">=18foo" or "18.0.0-beta" to
be treated as valid; update the comparator parsing in satisfiesComparator to
require a full-string match (anchor the pattern with ^...$) and tighten the
version capture to only allow numeric major/minor/patch parts (no trailing
characters or prerelease tags) so parseVersion/compare are only invoked for
fully-matched, well-formed comparators; apply the same fix to the other similar
parsing blocks referenced (the ones around lines 39-50 and 125-135) so all
comparator parsing is anchored and rejects malformed inputs before proceeding.
---
Duplicate comments:
In `@src/cli.ts`:
- Around line 1670-1684: The code currently sets outputPath =
path.resolve(opts.out) which causes the default report filename to be resolved
against the caller CWD instead of the scanned project root; update the logic so
when you assign the default (in the block checking !opts.outProvided &&
opts.format !== "html") you also set outputPath = path.resolve(projectPath,
opts.out) (i.e., resolve opts.out against projectPath), and ensure the initial
outputPath assignment uses opts.out only if opts.outProvided (or move the
initial path.resolve(opts.out) after projectPath is available) so that
projectPath, outputPath, opts.outProvided, opts.format and defaultOutputName are
consistent.
---
Nitpick comments:
In `@src/nodeEngine.test.ts`:
- Around line 4-20: Add a regression test to ensure malformed comparator tokens
are rejected: update the isNodeEngineTargetCompatible test suite to include a
case such as isNodeEngineTargetCompatible('>=18foo', 18) and assert it returns
false (or otherwise fails closed). Locate the tests around the existing
describe('isNodeEngineTargetCompatible', ...) block and add an it() that
verifies malformed ranges like '>=18foo' (and optionally other malformed
strings) do not pass; reference the isNodeEngineTargetCompatible function in the
assertion so the new test protects the tightened parser behavior.
🪄 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: 130753ba-a8ff-4518-87ee-2d9f43aeed99
⛔ Files ignored due to path filters (2)
dist/cli.jsis excluded by!**/dist/**dist/nodeEngine.jsis excluded by!**/dist/**
📒 Files selected for processing (4)
src/cli.tssrc/nodeEngine.test.tssrc/nodeEngine.tssrc/types.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/cli.ts (4)
546-563: Redundant Bun lockfile check at Line 558.The check at Line 558 duplicates the check at Line 549. Once
bun.lock/bun.lockbis checked on Line 549, reaching Line 558 implies that check already failed, so the second check will always be false.🔧 Suggested fix: remove redundant check
if (await pathExists(path.join(projectPath, "node_modules", ".pnpm"))) return "pnpm"; - if ((await pathExists(path.join(projectPath, "bun.lock"))) || (await pathExists(path.join(projectPath, "bun.lockb")))) return "bun"; if ( await pathExists(path.join(projectPath, "node_modules", ".yarn-state.yml")) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli.ts` around lines 546 - 563, Remove the duplicated bun lockfile existence check that repeats "(await pathExists(path.join(projectPath, 'bun.lock'))) || (await pathExists(path.join(projectPath, 'bun.lockb')))" (the second occurrence after the pnpm/yarn checks); leave the initial bun check that returns "bun" when fallback === "bun" and the earlier one immediately after, and ensure all other package manager checks (pnpm, yarn, npm, node_modules checks) remain unchanged so control flow and returned fallback logic are preserved.
2208-2215: Consider validating the previous report structure.The JSON is parsed and cast to
AggregatedDatawithout schema validation. If the file is from an incompatible schema version or is malformed beyond JSON syntax errors, downstream code may fail with unclear errors. A lightweight version check (e.g., verifyingprevious.schemamatches expected version) would improve robustness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli.ts` around lines 2208 - 2215, The parsed `previous` report is cast to AggregatedData without validating its schema; after parsing the JSON from `previousPath` (the try block that assigns `previous`), verify that the resulting object has the expected schema/version field (e.g., `previous.schema === EXPECTED_SCHEMA_VERSION`) and required top-level properties, and if the check fails log a clear error via `console.error` including the mismatched or missing schema value and then `process.exit(1)`; update the try/catch to separate JSON parse errors from schema validation failures so downstream code using `previous` (the AggregatedData variable) only runs when the report matches the expected structure.
2237-2252:--schematakes precedence over all commands.When
--schemais passed, it outputs the schema regardless of the command (e.g.,dependency-radar explain foo --schemaoutputs schema, not explanation). This is similar to how--helpworks but may surprise users. If this is intentional, consider documenting it in the help text.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli.ts` around lines 2237 - 2252, The current logic makes opts.schema take precedence over any command; change it so runSchemaCommand is only executed when no other command is specified or when the command explicitly requests schema (e.g., opts.command === "schema"), so replace the top-level check if (opts.schema) { await runSchemaCommand(opts); return; } with a guarded condition such as if (opts.schema && !opts.command) || opts.command === "schema" then call runSchemaCommand; leave calls to runExplainCommand, runWhyCommand, runCompareCommand untouched so commands like "explain"/"why"/"compare" run even if --schema is present (or alternatively, if you prefer keeping current behavior, update help text to document that opts.schema supersedes commands).
1889-1896: Use the structuredstatusfield instead of magic string comparison.The
SignatureAuditResulttype already provides astatus?: 'verified' | 'failed' | 'skipped'field. Replaceaudit?.error === "skipped (--offline)"withaudit?.status === 'skipped'to avoid fragility if the error message changes and to rely on the type system rather than string matching.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli.ts` around lines 1889 - 1896, Replace the fragile magic-string check for skipped audits with the structured status field on SignatureAuditResult: in the block that reads the audit from supplyChainResult.data?.signatureAudit (used when opts.auditSignatures is true) change the condition audit?.error === "skipped (--offline)" to audit?.status === 'skipped' and leave the spinner.log/statusLine behavior unchanged so the "skipped" case uses the structured audit.status instead of matching audit.error text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/nodeEngine.ts`:
- Around line 103-121: expandToken currently mishandles x-ranges, tilde
semantics, and bare versions; update expandToken to first add an x-range handler
(matching forms like 18.x, 18.* or 18.5.x) before the caret/tilde logic and
treat them as ranged tokens (e.g., 18.x => >=18.0.0 <19.0.0, 18.5.x => >=18.5.0
<18.6.0), then change the tilde branch (function/regex around variable tilde) to
distinguish ~18 (treat like 18.x => major range) versus ~18.5 (treat as >=18.5.0
<18.6.0), and finally update the bare-version branch (the parseVersion usage) to
emit exact match when a patch is present (18.5.1 => =18.5.1), a minor range when
only major+minor are present (18.5 => >=18.5.0 <18.6.0), and a major range when
only major present (18 => >=18.0.0 <19.0.0); keep references to expandToken, the
caret/tilde handlers, and parseVersion to locate the changes.
---
Nitpick comments:
In `@src/cli.ts`:
- Around line 546-563: Remove the duplicated bun lockfile existence check that
repeats "(await pathExists(path.join(projectPath, 'bun.lock'))) || (await
pathExists(path.join(projectPath, 'bun.lockb')))" (the second occurrence after
the pnpm/yarn checks); leave the initial bun check that returns "bun" when
fallback === "bun" and the earlier one immediately after, and ensure all other
package manager checks (pnpm, yarn, npm, node_modules checks) remain unchanged
so control flow and returned fallback logic are preserved.
- Around line 2208-2215: The parsed `previous` report is cast to AggregatedData
without validating its schema; after parsing the JSON from `previousPath` (the
try block that assigns `previous`), verify that the resulting object has the
expected schema/version field (e.g., `previous.schema ===
EXPECTED_SCHEMA_VERSION`) and required top-level properties, and if the check
fails log a clear error via `console.error` including the mismatched or missing
schema value and then `process.exit(1)`; update the try/catch to separate JSON
parse errors from schema validation failures so downstream code using `previous`
(the AggregatedData variable) only runs when the report matches the expected
structure.
- Around line 2237-2252: The current logic makes opts.schema take precedence
over any command; change it so runSchemaCommand is only executed when no other
command is specified or when the command explicitly requests schema (e.g.,
opts.command === "schema"), so replace the top-level check if (opts.schema) {
await runSchemaCommand(opts); return; } with a guarded condition such as if
(opts.schema && !opts.command) || opts.command === "schema" then call
runSchemaCommand; leave calls to runExplainCommand, runWhyCommand,
runCompareCommand untouched so commands like "explain"/"why"/"compare" run even
if --schema is present (or alternatively, if you prefer keeping current
behavior, update help text to document that opts.schema supersedes commands).
- Around line 1889-1896: Replace the fragile magic-string check for skipped
audits with the structured status field on SignatureAuditResult: in the block
that reads the audit from supplyChainResult.data?.signatureAudit (used when
opts.auditSignatures is true) change the condition audit?.error === "skipped
(--offline)" to audit?.status === 'skipped' and leave the spinner.log/statusLine
behavior unchanged so the "skipped" case uses the structured audit.status
instead of matching audit.error text.
🪄 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: 35705b5d-dbc5-43ec-9040-a6aaab0a80c2
⛔ Files ignored due to path filters (2)
dist/cli.jsis excluded by!**/dist/**dist/nodeEngine.jsis excluded by!**/dist/**
📒 Files selected for processing (3)
src/cli.tssrc/nodeEngine.test.tssrc/nodeEngine.ts
✅ Files skipped from review due to trivial changes (1)
- src/nodeEngine.test.ts
Merged
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
--fail-on supply-chain-sourcepolicy--schemasupport plus a versioned JSON schema for CI consumersbun.lockbguidance, and Yarn PnP-aware behaviorTesting
--schema,--audit-signatures,--fail-on supply-chain-source, Bun scans, Yarn PnP scans, and JSON output fieldsSummary by CodeRabbit
New Features
whyandcomparecommands; multi-format exports (SARIF, CycloneDX, SPDX, JSON/HTML),--schema/--format/--sbom, optional npm signature checks (--audit-signatures),--target-nodecompatibility checks, workspace filter in the report UI, and lockfile supply-chain signal collection.Documentation
supply-chain-sourcefail rule.Chores
Tests