fix: Make the dependency audit skill scalable, make all calls through the cjs script#51
fix: Make the dependency audit skill scalable, make all calls through the cjs script#51Cesar-M-Diaz wants to merge 6 commits into
Conversation
|
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:
WalkthroughAdds a bundled NCM dependency audit CLI with dependency collection, batched queries, retries, recovery, remediation verification, validated rendering, secure report persistence, updated skill instructions, and extensive unit coverage. ChangesDependency audit
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Skill
participant audit-dependencies.cjs
participant collectDependencies
participant NCM
participant renderAuditReport
participant audit-report-output
Skill->>audit-dependencies.cjs: run audit command
audit-dependencies.cjs->>collectDependencies: collect dependency tree
audit-dependencies.cjs->>NCM: fetch dependency and remediation scores
NCM-->>audit-dependencies.cjs: scores, failures, or omitted responses
audit-dependencies.cjs->>renderAuditReport: validate and render summary
renderAuditReport->>audit-report-output: save report and executive summary
audit-report-output-->>Skill: linked authoritative summary
Possibly related issues
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@skills/ns-audit-dependencies/audit-dependencies.cjs`:
- Around line 719-723: Update the score handling in activeVulnerabilities so
non-array or missing pkg.scores is treated as an invalid-response and the
package remains unchecked, rather than defaulting to an empty clean result.
Preserve filtering and compactVulnerability mapping only for valid score arrays.
- Around line 200-206: Bound all nested NCM-provided finding content, not just
the package count controlled by MAX_FINDINGS. Extend compactStringList and the
finding-processing paths to cap collection sizes and individual string lengths
for vulnerabilities, assessments, ranges, and related fields, while tracking and
reporting how many items or characters were truncated. Apply the same limits
consistently across the other finding aggregation and output paths.
In `@skills/ns-audit-dependencies/SKILL.md`:
- Line 13: Update the fenced command block in SKILL.md to specify the Bash
language by changing its opening fence to ```bash, while preserving the block’s
existing contents and closing fence.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 32b3bbbc-59a9-4908-a6d2-b3f6d40420a5
📒 Files selected for processing (4)
packages/core/test/unit/skills/audit-dependencies-script.test.tsskills/ns-audit-dependencies/SKILL.mdskills/ns-audit-dependencies/audit-dependencies.cjsskills/ns-audit-dependencies/collect-dependencies.cjs
| function compactStringList (value) { | ||
| const values = Array.isArray(value) ? value : typeof value === 'string' ? [value] : [] | ||
| return Array.from(new Set(values | ||
| .filter(item => typeof item === 'string') | ||
| .map(item => item.trim()) | ||
| .filter(Boolean))) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Bound each finding’s nested content.
MAX_FINDINGS limits package count only; NCM-provided vulnerabilities, assessments, ranges, and strings remain unbounded. A verbose response can still generate arbitrarily large stdout and agent context. Cap collection sizes/string lengths and report truncation counts.
Also applies to: 274-309, 724-739
🤖 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 `@skills/ns-audit-dependencies/audit-dependencies.cjs` around lines 200 - 206,
Bound all nested NCM-provided finding content, not just the package count
controlled by MAX_FINDINGS. Extend compactStringList and the finding-processing
paths to cap collection sizes and individual string lengths for vulnerabilities,
assessments, ranges, and related fields, while tracking and reporting how many
items or characters were truncated. Apply the same limits consistently across
the other finding aggregation and output paths.
- remove the 50-package findings limit - bound oversized NCM strings with truncation accounting - treat malformed score responses as unchecked - match resolved versions returned for latest requests - support SemVer build metadata in remediation checks - update audit instructions and regression coverage
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@skills/ns-audit-dependencies/render-audit-report.cjs`:
- Around line 30-67: Extend validateAuditSummary to validate every nested object
and collection required by validation and rendering—including vulnerabilities,
packages, batchFailures, and each finding.moduleRisks—before any property
dereference or iteration. For every missing or malformed structure, throw
integrityError so malformed summaries consistently preserve the
AUDIT_REPORT_INTEGRITY_ERROR contract; keep valid-summary counting behavior
unchanged.
- Around line 51-67: Update the summary validation in render-audit-report.cjs to
validate batchFailures and remediation.failures counters as non-negative values
and assert each total matches the sum represented by its byReason breakdown,
preventing contradictory rendered totals. Extend the existing integrity-error
regression tests with cases covering both mismatch types, while preserving the
current validation 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e348be1c-f3fc-4eb7-95d0-6a57c0e0d4c4
📒 Files selected for processing (5)
packages/core/test/unit/skills/audit-dependencies-renderer.test.tspackages/core/test/unit/skills/audit-dependencies-script.test.tsskills/ns-audit-dependencies/SKILL.mdskills/ns-audit-dependencies/audit-dependencies.cjsskills/ns-audit-dependencies/render-audit-report.cjs
🚧 Files skipped from review as they are similar to previous changes (2)
- skills/ns-audit-dependencies/audit-dependencies.cjs
- packages/core/test/unit/skills/audit-dependencies-script.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@skills/ns-audit-dependencies/audit-report-output.cjs`:
- Around line 42-45: In the report-generation flow, capture an immutable
snapshot of summary before the awaited saveAuditReport call, then pass that
snapshot to both renderAuditReport and renderAuditSummary so persistence-time
mutations cannot change either output. Add a regression test covering mutation
of the original summary during save and verify the returned report and
executiveSummary remain 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 73806ee3-86ad-4e3b-94bb-407ec1d68600
📒 Files selected for processing (7)
packages/core/test/unit/skills/audit-dependencies-output.test.tspackages/core/test/unit/skills/audit-dependencies-renderer.test.tspackages/core/test/unit/skills/audit-dependencies-script.test.tsskills/ns-audit-dependencies/SKILL.mdskills/ns-audit-dependencies/audit-dependencies.cjsskills/ns-audit-dependencies/audit-report-output.cjsskills/ns-audit-dependencies/render-audit-report.cjs
🚧 Files skipped from review as they are similar to previous changes (4)
- skills/ns-audit-dependencies/SKILL.md
- skills/ns-audit-dependencies/render-audit-report.cjs
- skills/ns-audit-dependencies/audit-dependencies.cjs
- packages/core/test/unit/skills/audit-dependencies-script.test.ts
484b31f to
a3f47bb
Compare
| if (result) { | ||
| state.hadResponse = true | ||
| const active = activeVulnerabilities(result) | ||
| const resultVersion = truncateNcmString(result.version) |
There was a problem hiding this comment.
Line 741 calls truncateNcmString(result.version) without the truncation counter, then line 745 calls it again with ncmContentTruncation. If result.version exceeds 4096, the first call truncates without counting. The second call gets a string ≤ 4096 and won't truncate, so the truncation of this version goes uncounted.
| if (!uncheckedByKey.has(key)) { | ||
| uncheckedByKey.set(key, { | ||
| name: requestedPackage.name, | ||
| version: pkg && pkg.version && requestedPackage.version === 'latest' ? pkg.version : requestedPackage.version, |
There was a problem hiding this comment.
If a request was made with version latest and the batch fails before getting a response, recordUnchecked stores "latest" as the version string (since pkg.version is "latest" and requestedPackage.version === "latest" is true). The unchecked-packages report will show version: "latest", which is unhelpful — it does not tell the operator which version was actually installed and unchecked.
|
|
||
| for (const vulnerability of finding.vulnerabilities.filter(item => !item.withdrawn)) { | ||
| for (const range of vulnerability.patched || []) { | ||
| const match = range.match(/(?:^|[\s|,(])>=?\s*v?(\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?)/) |
There was a problem hiding this comment.
The patched-range regex (line 295) uses range.match(...) with no while loop, so it only captures the first boundary. The vulnerable-range regex (line 299-301) uses a while loop with /g flag to capture all. If an advisory specifies multiple patched ranges (e.g., >= 1.0.0 || >= 2.0.0), the second boundary is silently dropped.
2 participants
Summary by CodeRabbit