fix: Make the dependency audit skill scalable, make all calls through the cjs script#51
Open
Cesar-M-Diaz wants to merge 4 commits into
Open
fix: Make the dependency audit skill scalable, make all calls through the cjs script#51Cesar-M-Diaz wants to merge 4 commits into
Cesar-M-Diaz wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a bundled NCM dependency audit CLI with dependency collection, batched GraphQL queries, retries, recovery, vulnerability extraction, remediation verification, validated Markdown rendering, 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
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: render validated summary
renderAuditReport-->>Skill: authoritative Markdown report
Possibly related issues
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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
Comment on lines
+200
to
+206
| 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
Comment on lines
+51
to
+67
| assertEqual(summary.findings.length, summary.vulnerabilities.affectedPackages, 'affected package versions') | ||
| assertEqual(vulnerabilityTotal, summary.vulnerabilities.total, 'vulnerability records') | ||
| assertCounts(vulnerabilityCounts, summary.vulnerabilities.bySeverity, 'severity') | ||
| assertCounts(remediationCounts, { | ||
| verified: summary.remediation.verified, | ||
| unresolved: summary.remediation.unresolved, | ||
| verificationFailed: summary.remediation.verificationFailed, | ||
| notRequired: summary.remediation.notRequired | ||
| }, 'remediation') | ||
|
|
||
| const uncheckedCounts = {} | ||
| for (const pkg of summary.uncheckedPackages) { | ||
| uncheckedCounts[pkg.reason] = (uncheckedCounts[pkg.reason] || 0) + 1 | ||
| } | ||
| assertEqual(summary.packages.checked + summary.packages.unchecked, summary.packages.total, 'package coverage') | ||
| assertEqual(summary.uncheckedPackages.length, summary.packages.unchecked, 'unchecked package versions') | ||
| assertCounts(stableCounts(uncheckedCounts), summary.packages.uncheckedByReason, 'unchecked reason') |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reconcile the failure totals that the report prints.
batchFailures.total and remediation.failures.total are rendered on Lines 171–172 but never validated against byReason. A summary such as { total: 1, byReason: {} } passes validation and reports “1 (none),” creating a contradictory audit result. Validate non-negative counters and assert each total equals its reason breakdown.
Proposed direction
+function sumCounts (counts, label) {
+ return Object.entries(counts || {}).reduce((total, [, count]) => {
+ if (!Number.isSafeInteger(count) || count < 0) {
+ throw integrityError(`${label} contains an invalid count`)
+ }
+ return total + count
+ }, 0)
+}
+
function validateAuditSummary (summary) {
// existing checks
+ assertEqual(
+ summary.batchFailures.total,
+ sumCounts(summary.batchFailures.byReason, 'batch failure reasons'),
+ 'terminal batch failures'
+ )
+ assertEqual(
+ summary.remediation.failures.total,
+ sumCounts(summary.remediation.failures.byReason, 'remediation failure reasons'),
+ 'remediation candidate failures'
+ )Add regression cases alongside the existing integrity-error test for both mismatches.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| assertEqual(summary.findings.length, summary.vulnerabilities.affectedPackages, 'affected package versions') | |
| assertEqual(vulnerabilityTotal, summary.vulnerabilities.total, 'vulnerability records') | |
| assertCounts(vulnerabilityCounts, summary.vulnerabilities.bySeverity, 'severity') | |
| assertCounts(remediationCounts, { | |
| verified: summary.remediation.verified, | |
| unresolved: summary.remediation.unresolved, | |
| verificationFailed: summary.remediation.verificationFailed, | |
| notRequired: summary.remediation.notRequired | |
| }, 'remediation') | |
| const uncheckedCounts = {} | |
| for (const pkg of summary.uncheckedPackages) { | |
| uncheckedCounts[pkg.reason] = (uncheckedCounts[pkg.reason] || 0) + 1 | |
| } | |
| assertEqual(summary.packages.checked + summary.packages.unchecked, summary.packages.total, 'package coverage') | |
| assertEqual(summary.uncheckedPackages.length, summary.packages.unchecked, 'unchecked package versions') | |
| assertCounts(stableCounts(uncheckedCounts), summary.packages.uncheckedByReason, 'unchecked reason') | |
| function sumCounts (counts, label) { | |
| return Object.entries(counts || {}).reduce((total, [, count]) => { | |
| if (!Number.isSafeInteger(count) || count < 0) { | |
| throw integrityError(`${label} contains an invalid count`) | |
| } | |
| return total + count | |
| }, 0) | |
| } | |
| assertEqual(summary.findings.length, summary.vulnerabilities.affectedPackages, 'affected package versions') | |
| assertEqual(vulnerabilityTotal, summary.vulnerabilities.total, 'vulnerability records') | |
| assertCounts(vulnerabilityCounts, summary.vulnerabilities.bySeverity, 'severity') | |
| assertCounts(remediationCounts, { | |
| verified: summary.remediation.verified, | |
| unresolved: summary.remediation.unresolved, | |
| verificationFailed: summary.remediation.verificationFailed, | |
| notRequired: summary.remediation.notRequired | |
| }, 'remediation') | |
| assertEqual( | |
| summary.batchFailures.total, | |
| sumCounts(summary.batchFailures.byReason, 'batch failure reasons'), | |
| 'terminal batch failures' | |
| ) | |
| assertEqual( | |
| summary.remediation.failures.total, | |
| sumCounts(summary.remediation.failures.byReason, 'remediation failure reasons'), | |
| 'remediation candidate failures' | |
| ) | |
| const uncheckedCounts = {} | |
| for (const pkg of summary.uncheckedPackages) { | |
| uncheckedCounts[pkg.reason] = (uncheckedCounts[pkg.reason] || 0) + 1 | |
| } | |
| assertEqual(summary.packages.checked + summary.packages.unchecked, summary.packages.total, 'package coverage') | |
| assertEqual(summary.uncheckedPackages.length, summary.packages.unchecked, 'unchecked package versions') | |
| assertCounts(stableCounts(uncheckedCounts), summary.packages.uncheckedByReason, 'unchecked reason') |
🤖 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/render-audit-report.cjs` around lines 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.
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 by CodeRabbit