Chore/stream scan responses #125
Conversation
WalkthroughReplaces in-memory result aggregation with NDJSON write streams and a converter; adds CLI streaming JSON transform using stream-json; introduces Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI
participant Scanner as Scanner
participant ND as NDJSON Streams
participant Converter as NDJSON→JSON Converter
participant Parser as stream-json Parser
participant Crypto as Cryptography logic
participant Output as File/Stdout
CLI->>Scanner: start scan (format)
Scanner->>ND: open wfp & result write streams
loop emit results
Scanner->>ND: write NDJSON line per result
end
Scanner->>Scanner: close write streams
Scanner->>Converter: convertNDJSONToJSON()
Converter->>Converter: read NDJSON, assemble final JSON
alt format == JSON
CLI->>Parser: streamAndTransformResults(input JSON)
Parser->>Crypto: emit component objects (includes requirement)
Parser->>Output: stream transformed JSON to file/stdout
else format == HTML
CLI->>Converter: load full JSON into memory (guard large files)
CLI->>Output: generate HTML report from in-memory JSON
end
Output-->>CLI: finished
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sdk/Cryptography/Helper/ResultCollector/Component/ComponentCryptographyResultColletor.ts (1)
48-55: Inconsistentrequirementhandling between algorithm and hint collection.
collectAlgorithmResultsskips components with a falsyrequirement(line 50), butcollectHintResults(line 63) processes all components regardless ofrequirementvalue. This could lead to data inconsistency where hints are collected for components that have no corresponding algorithm entries due to missing requirements.Consider applying the same guard to
collectHintResults, or document why the asymmetric behavior is intentional.🔎 Proposed fix to add consistent guard
public collectHintResults(hintResults: HintsInRangeResponse):void { hintResults.components.forEach((c) => { + if (c.requirement) { const result = this.getOrCreateResult(c.purl,c.version,c.requirement); result.hints = c.hints; + } }); }
🧹 Nitpick comments (5)
src/cli/commands/scan.ts (3)
306-319: Duplicate streaming pipeline code should be extracted to a helper.The streaming logic to load scanner results appears twice with nearly identical code (lines 306-319 and 333-346). Extract this into a reusable helper function to improve maintainability.
🔎 Proposed helper function
async function loadScannerResultsFromStream(resultPath: string): Promise<Record<string, any>> { return new Promise((resolve, reject) => { const pipeline = fs.createReadStream(resultPath) .pipe(parser()) .pipe(streamObject()); const scannerResults: Record<string, any> = {}; pipeline.on('data', (data: { key: string; value: any }) => { scannerResults[data.key] = data.value; }); pipeline.on('end', () => resolve(scannerResults)); pipeline.on('error', reject); }); }Then replace both occurrences:
const scannerData = await loadScannerResultsFromStream(scannerResultPath);Also applies to: 333-346
62-65: OS-specific EOL may cause inconsistent JSON output across platforms.Using
EOLfromosmodule means the JSON output will have\r\non Windows and\non Unix. This could cause issues if files are shared across platforms or compared for equality. Consider using\nconsistently for JSON output.🔎 Proposed fix
+const JSON_EOL = '\n'; // Use consistent line endings for JSON output const indentLines = (jsonStr: string, spaces: number): string => { const indent = ' '.repeat(spaces); - return jsonStr.split(EOL).map((line, idx) => idx === 0 ? line : indent + line).join(EOL); + return jsonStr.split('\n').map((line, idx) => idx === 0 ? line : indent + line).join(JSON_EOL); };And replace all
EOLusages instreamAndTransformResultswithJSON_EOL.
111-116: Pipeline error may leave partial output file.When a pipeline error occurs, the write stream is destroyed but the partial output file may remain on disk. Consider unlinking the partial file on error.
🔎 Proposed fix
pipeline.on('error', (error) => { if (outputPath && writeStream !== process.stdout) { (writeStream as fs.WriteStream).destroy(); + fs.unlink(outputPath, () => {}); // Best-effort cleanup } reject(error); });src/sdk/scanner/Scanner.ts (2)
607-638: Consider usingPromise.allfor cleaner stream closing.The current implementation works but the
rejectparameter is unused. Sincestream.end()callbacks don't receive errors, this is functionally fine, but you could simplify usingPromise.all.🔎 Proposed simplified implementation
- private closeWriteStreams(): Promise<void> { - return new Promise((resolve, reject) => { - let wfpClosed = false; - let resultClosed = false; - - const checkBothClosed = () => { - if (wfpClosed && resultClosed) { - resolve(); - } - }; - - if (this.wfpWriteStream) { - this.wfpWriteStream.end(() => { - wfpClosed = true; - checkBothClosed(); - }); - } else { - wfpClosed = true; - } - - if (this.resultWriteStream) { - this.resultWriteStream.end(() => { - resultClosed = true; - checkBothClosed(); - }); - } else { - resultClosed = true; - } - - checkBothClosed(); - }); + private closeWriteStreams(): Promise<void> { + const closeStream = (stream: fs.WriteStream | undefined): Promise<void> => + stream ? new Promise(resolve => stream.end(resolve)) : Promise.resolve(); + + return Promise.all([ + closeStream(this.wfpWriteStream), + closeStream(this.resultWriteStream) + ]).then(() => {}); }
602-605: Consider adding error listeners to write streams.The write streams are created without error listeners. If a write fails (e.g., disk full), the error would be unhandled and could crash the process or silently fail.
🔎 Proposed fix to add error listeners
private initializeWriteStreams() { this.wfpWriteStream = fs.createWriteStream(this.wfpFilePath, { flags: 'a' }); this.resultWriteStream = fs.createWriteStream(this.resultFilePath, { flags: 'a' }); + + this.wfpWriteStream.on('error', (err) => { + logger.error(`[ SCANNER ]: WFP write stream error: ${err.message}`); + this.errorHandler(err, ScannerEvents.MODULE_WINNOWER); + }); + + this.resultWriteStream.on('error', (err) => { + logger.error(`[ SCANNER ]: Result write stream error: ${err.message}`); + this.errorHandler(err, ScannerEvents.MODULE_DISPATCHER); + }); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
CHANGELOG.md(2 hunks)package.json(3 hunks)src/cli/commands/scan.ts(5 hunks)src/sdk/Clients/Cryptography/ICryptographyClient.ts(1 hunks)src/sdk/Cryptography/Helper/ResultCollector/Component/ComponentCryptographyResultColletor.ts(4 hunks)src/sdk/scanner/Scanner.ts(9 hunks)src/sdk/scanner/ScannerCfg.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/cli/commands/scan.ts (3)
src/sdk/Clients/Dependency/IDependencyClient.ts (1)
DependencyResponse(32-35)src/sdk/Clients/Grpc/scanoss/api/dependencies/v2/scanoss-dependencies_pb.d.ts (1)
DependencyResponse(83-102)src/sdk/Cryptography/CryptographyTypes.ts (2)
LocalCryptography(65-69)CryptographyResponse(75-80)
src/sdk/scanner/Scanner.ts (3)
src/index.ts (1)
logger(64-64)src/sdk/scanner/ScannerTypes.ts (1)
ScannerResults(70-70)src/sdk/scanner/ScannnerResultPostProcessor/rules/rule-factory.ts (1)
ScannerResultsRuleFactory(6-22)
🔇 Additional comments (8)
src/sdk/Clients/Cryptography/ICryptographyClient.ts (1)
23-26: LGTM!The
requirementfield addition toComponentAlgorithmaligns with the existingComponentHintResponseinterface pattern (line 40), maintaining consistency across component-related types.src/sdk/Cryptography/Helper/ResultCollector/Component/ComponentCryptographyResultColletor.ts (1)
30-31: Verify that keying byrequirementinstead ofversionis intentional.The key changed from
${purl}@${version}to${purl}@${requirement}. If the same purl has different versions but the same requirement (e.g.,^1.0.0), they will collide and the later entry will overwrite algorithms/hints from the earlier one.src/cli/commands/scan.ts (1)
42-51: LGTM - Well-structured streaming implementation.The
streamAndTransformResultsfunction provides a clean streaming approach for JSON output that avoids holding the entire scanner result in memory during transformation.package.json (1)
40-40: No security issues detected for stream-json ^1.9.1.The dependency addition aligns with streaming implementation and has no known vulnerabilities.
src/sdk/scanner/ScannerCfg.ts (1)
30-30: LGTM!Reducing the buffer size from 300 to 20 aligns well with the new streaming approach, minimizing peak memory usage at the cost of more frequent (but efficient) stream writes.
src/sdk/scanner/Scanner.ts (3)
85-87: LGTM!The new write stream properties properly support the streaming architecture for both WFP and result files.
708-715: LGTM!Using
end()for cleanup ensures buffered data is flushed before closing, which is appropriate for abort scenarios. Not awaiting completion is acceptable here since it's an abort path.
471-479: LGTM!The finish sequence correctly ensures the buffer is flushed, streams are closed, and then NDJSON is converted to JSON. This properly coordinates the streaming pipeline.
agustingroh
force-pushed
the
chore/stream-scan-responses
branch
2 times, most recently
from
dc6fa26 to
98f759e
Compare
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 (2)
src/sdk/Cryptography/Helper/ResultCollector/Component/ComponentCryptographyResultColletor.ts (2)
30-42: Inconsistent null/undefined handling betweencollectAlgorithmResultsandcollectHintResultsfor the requirement parameter.
collectAlgorithmResults(line 50) guards against missing requirement withif (c.requirement), butcollectHintResults(line 63) does not. Ifc.requirementis undefined or null incollectHintResults, the key becomes${purl}@undefinedor${purl}@null, which could cause data integrity issues or unexpected collisions.Additionally, if the
requirementstring contains the@character, it could lead to key collisions (e.g.,npm/pkg+ requirementfoo@barproduces the same key asnpm/pkg@foo+ requirementbar). Consider either escaping the requirement value or using a different delimiter for the map key.
48-55: Guard check incollectAlgorithmResultsis inconsistent withcollectHintResults.The
collectAlgorithmResultsmethod guards against falsyrequirementvalues withif (c.requirement), butcollectHintResults(line 61-65) lacks this guard and directly usesc.requirementon all components. Althoughrequirementis typed as required (string, notstring?), this inconsistency creates a potential issue: algorithms may be skipped for components with falsy requirements, while hints are always collected. This could lead to incomplete or misaligned results where a component has hints but no algorithms recorded.Consider:
- Applying the same guard to
collectHintResultsfor consistency, or- Removing the guard from
collectAlgorithmResultsifrequirementis guaranteed to be present, or- Documenting why the two methods handle
requirementdifferently.
🧹 Nitpick comments (2)
src/sdk/scanner/Scanner.ts (2)
556-567: Handle potential ENOENT in cleanup.
fs.rmSync(tempFilePath)will throw if the file doesn't exist (e.g., if both streams error in quick succession). Consider using{ force: true }to silently ignore missing files.🔎 Proposed fix
readStream.on('error', (error) => { writeStream.destroy(); - fs.rmSync(tempFilePath); + fs.rmSync(tempFilePath, { force: true }); reject(error); }); writeStream.on('error', (error) => { rl.close(); readStream.destroy(); - fs.rmSync(tempFilePath); + fs.rmSync(tempFilePath, { force: true }); reject(error); });
609-640: Consider error handling in stream closure.The
stream.end()callback may receive an error argument that's currently ignored. While uncommon, stream close errors could indicate data wasn't fully flushed.🔎 Optional improvement with error propagation
private closeWriteStreams(): Promise<void> { - return new Promise((resolve, reject) => { - let wfpClosed = false; - let resultClosed = false; - - const checkBothClosed = () => { - if (wfpClosed && resultClosed) { - resolve(); - } - }; - - if (this.wfpWriteStream) { - this.wfpWriteStream.end(() => { - wfpClosed = true; - checkBothClosed(); - }); - } else { - wfpClosed = true; - } - - if (this.resultWriteStream) { - this.resultWriteStream.end(() => { - resultClosed = true; - checkBothClosed(); - }); - } else { - resultClosed = true; - } - - checkBothClosed(); - }); + const closeStream = (stream: fs.WriteStream | undefined): Promise<void> => { + if (!stream) return Promise.resolve(); + return new Promise((resolve, reject) => { + stream.end((err?: Error) => { + if (err) reject(err); + else resolve(); + }); + }); + }; + return Promise.all([ + closeStream(this.wfpWriteStream), + closeStream(this.resultWriteStream) + ]).then(() => {}); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
CHANGELOG.mdpackage.jsonsrc/cli/commands/scan.tssrc/sdk/Clients/Cryptography/ICryptographyClient.tssrc/sdk/Cryptography/Helper/ResultCollector/Component/ComponentCryptographyResultColletor.tssrc/sdk/scanner/Scanner.tssrc/sdk/scanner/ScannerCfg.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
🧰 Additional context used
🧬 Code graph analysis (2)
src/cli/commands/scan.ts (3)
src/sdk/Clients/Dependency/IDependencyClient.ts (1)
DependencyResponse(32-35)src/sdk/Clients/Grpc/scanoss/api/dependencies/v2/scanoss-dependencies_pb.d.ts (1)
DependencyResponse(83-102)src/sdk/Cryptography/CryptographyTypes.ts (2)
LocalCryptography(65-69)CryptographyResponse(75-80)
src/sdk/scanner/Scanner.ts (3)
src/index.ts (1)
logger(64-64)src/sdk/scanner/ScannerTypes.ts (1)
ScannerResults(70-70)src/sdk/scanner/ScannnerResultPostProcessor/rules/rule-factory.ts (1)
ScannerResultsRuleFactory(6-22)
🔇 Additional comments (10)
src/sdk/scanner/ScannerCfg.ts (1)
30-30: LGTM - buffer reduction aligns with streaming approach.Reducing from 300 to 20 responses makes sense for the new streaming NDJSON output. More frequent flushes reduce memory pressure at the cost of additional I/O, which is the expected trade-off for streaming workflows.
src/sdk/scanner/Scanner.ts (3)
85-88: LGTM - stream member declarations.The write stream members are properly typed and will be managed through the new lifecycle methods.
646-672: LGTM - NDJSON streaming output.The streaming approach with deobfuscation and rule application is well-structured. While
write()doesn't handle backpressure explicitly, the reduced buffer size (20 responses) keeps individual batch writes small.
710-717: LGTM - abort cleanup.Using
end()for cleanup during abort is appropriate, allowing buffered data to flush before closing.src/cli/commands/scan.ts (4)
29-31: LGTM - streaming imports.The
stream-jsonimports enable efficient streaming JSON parsing without loading the entire file into memory.
302-326: LGTM - streaming component data loading.The streaming approach for loading scanner results when building the component list is memory-efficient. This addresses the earlier concern about unconditionally loading scanner results into memory.
330-343: LGTM - HTML format size safeguard.The 2GB limit check is a sensible safeguard before loading the entire file into memory. The error message clearly guides users to use JSON format for large scans.
366-375: LGTM - JSON streaming output.The JSON format path now uses streaming transformation, avoiding the need to load the entire scanner results file into memory.
src/sdk/Clients/Cryptography/ICryptographyClient.ts (1)
23-26: LGTM -requirementfield addition.The
requirementfield enables proper grouping of components by their version requirement (e.g.,purl@requirement), aligning with the cryptography result collection changes.Verify that all implementations populate the
requirementfield:#!/bin/bash # Search for implementations that create ComponentAlgorithm or ComponentHintResponse objects rg -n "ComponentAlgorithm|ComponentHintResponse" --type ts -A 5 | head -100Also applies to: 38-41
package.json (1)
3-3: LGTM - version bump and streaming dependencies align well.The latest version of stream-json is 1.9.1, and the dependency constraint
^1.9.1is appropriate. With 380 dependents on npm, stream-json is a well-established micro-library for creating custom JSON processing pipelines with minimal dependencies and memory footprint. The @types/stream-json version 1.7.8 is current and used by 17 other projects. The version bump to 0.29.0 appropriately reflects the new streaming functionality.
src/sdk/Cryptography/Helper/ResultCollector/Component/ComponentCryptographyResultColletor.ts
Show resolved
Hide resolved
agustingroh
force-pushed
the
chore/stream-scan-responses
branch
from
98f759e to
e4f9653
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/sdk/Cryptography/Helper/ResultCollector/Component/ComponentCryptographyResultColletor.ts (1)
61-66: Critical: Missing guard forc.requirementcauses inconsistent behavior.Unlike
collectAlgorithmResults(line 50), this method does not check whetherc.requirementexists before using it. If a component lacks arequirementfield, the key at line 31 will become${purl}@undefined, leading to:
- Unexpected map keys and potential data corruption
- Inconsistent behavior between algorithm and hint collection
Apply the same guard pattern used in
collectAlgorithmResults:🔎 Proposed fix
public collectHintResults(hintResults: HintsInRangeResponse):void { hintResults.components.forEach((c) => { + if (c.requirement) { const result = this.getOrCreateResult(c.purl,c.version,c.requirement); result.hints = c.hints; + } }); }
🧹 Nitpick comments (1)
src/cli/commands/scan.ts (1)
112-124: Missing error handler for stdout writes could leave promise hanging.When writing to
stdout(nooutputPath), the code doesn't attach an error handler towriteStream. If stdout encounters an error (e.g., broken pipe when piped to a closed process), the promise may never reject.🔎 Proposed fix
pipeline.on('error', (error) => { if (outputPath && writeStream !== process.stdout) { (writeStream as fs.WriteStream).destroy(); } reject(error); }); - if (outputPath) { - writeStream.on('error', (error) => { - pipeline.destroy(); - reject(error); - }); - } + writeStream.on('error', (error) => { + pipeline.destroy(); + if (outputPath && writeStream !== process.stdout) { + (writeStream as fs.WriteStream).destroy(); + } + reject(error); + });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
CHANGELOG.mdpackage.jsonsrc/cli/commands/scan.tssrc/sdk/Clients/Cryptography/ICryptographyClient.tssrc/sdk/Cryptography/Helper/ResultCollector/Component/ComponentCryptographyResultColletor.tssrc/sdk/scanner/Scanner.tssrc/sdk/scanner/ScannerCfg.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/sdk/scanner/ScannerCfg.ts
- package.json
- src/sdk/Clients/Cryptography/ICryptographyClient.ts
- CHANGELOG.md
🧰 Additional context used
🧬 Code graph analysis (2)
src/cli/commands/scan.ts (3)
src/sdk/Clients/Dependency/IDependencyClient.ts (1)
DependencyResponse(32-35)src/sdk/Clients/Grpc/scanoss/api/dependencies/v2/scanoss-dependencies_pb.d.ts (1)
DependencyResponse(83-102)src/sdk/Cryptography/CryptographyTypes.ts (2)
LocalCryptography(65-69)CryptographyResponse(75-80)
src/sdk/scanner/Scanner.ts (3)
src/index.ts (1)
logger(64-64)src/sdk/scanner/ScannerTypes.ts (1)
ScannerResults(70-70)src/sdk/scanner/ScannnerResultPostProcessor/rules/rule-factory.ts (1)
ScannerResultsRuleFactory(6-22)
🔇 Additional comments (14)
src/sdk/Cryptography/Helper/ResultCollector/Component/ComponentCryptographyResultColletor.ts (1)
48-55: Good defensive guard for undefined requirement.The guard at line 50 properly prevents processing components without a
requirementfield, avoiding the creation of invalid map keys.src/cli/commands/scan.ts (5)
29-31: LGTM!Imports are correctly added for streaming JSON parsing and platform-specific line endings.
62-66: LGTM - addressed previous feedback.The
indentLineshelper now correctly splits on'\n'and joins withEOLfor platform consistency, addressing the previous EOL mismatch concern.
330-344: Good implementation of format-specific memory management.The 2GB file size check before loading for HTML format is a sensible guard. The streaming path for JSON format avoids memory issues for large files. This addresses the previous feedback about deferring scanner result loading.
367-376: LGTM!The JSON format path now uses streaming transformation, avoiding loading the entire scanner result file into memory.
306-319: Missing error handler on streaming pipeline.If the read stream or parser encounters an error, the promise will hang indefinitely since no
'error'event handler is attached topipeline.🔎 Proposed fix
const pipeline = fs.createReadStream(scannerResultPath) .pipe(parser()) .pipe(streamObject()); const scannerResults: any = {}; pipeline.on('data', (data: { key: string; value: any }) => { scannerResults[data.key] = data.value; }); pipeline.on('end', () => resolve(scannerResults)); - pipeline.on('error', reject); + pipeline.on('error', (error) => { + reject(new Error(`Failed to parse scanner results: ${error.message}`)); + });Likely an incorrect or invalid review comment.
src/sdk/scanner/Scanner.ts (8)
85-87: LGTM!New write stream fields properly declared as private class members.
533-535: Addressed: Error logging now in place.Previous review comment about silent error handling has been addressed - parse errors are now logged via
this.reportLog.
604-640: LGTM!Stream lifecycle management is well implemented. The
closeWriteStreamsmethod correctly handles cases where streams may not be initialized and uses a proper completion pattern.
659-673: LGTM - Streaming NDJSON writes.Good use of streaming writes with NDJSON format. The rule processing chain correctly reassigns
processedResponsebefore iterating to write entries.
710-717: LGTM!Good addition of stream cleanup in the abort path to prevent resource leaks.
474-478: LGTM!Correct sequencing - streams are closed before the NDJSON conversion reads from the file.
413-414: LGTM!Useful debug logging for monitoring buffer behavior.
588-602: LGTM!Output files are properly initialized as empty before write streams are created with append mode.
src/sdk/Cryptography/Helper/ResultCollector/Component/ComponentCryptographyResultColletor.ts
Show resolved
Hide resolved
agustingroh
force-pushed
the
chore/stream-scan-responses
branch
from
e4f9653 to
d9a2517
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/sdk/Cryptography/Helper/ResultCollector/Component/ComponentCryptographyResultColletor.ts (1)
61-66: Missing guard forc.requirementincollectHintResults- inconsistent withcollectAlgorithmResults.The
collectAlgorithmResultsmethod (line 50) checksif (c.requirement)before processing, butcollectHintResultsdoes not. WhileComponentHintResponse.requirementis typed as requiredstring, the guard incollectAlgorithmResultssuggests the runtime data may not always include this field. For consistency and defensive coding, apply the same guard pattern here.This was flagged in a previous review but appears unaddressed.
🔎 Proposed fix
public collectHintResults(hintResults: HintsInRangeResponse):void { hintResults.components.forEach((c) => { + if (c.requirement) { const result = this.getOrCreateResult(c.purl,c.version,c.requirement); result.hints = c.hints; + } }); }
🧹 Nitpick comments (2)
src/cli/commands/scan.ts (1)
62-66: Mixed line endings could cause inconsistent output.Splitting on
'\n'but joining withEOLcreates mixed line endings whenJSON.stringifyoutput (which uses\n) is joined with platform-specificEOL(which is\r\non Windows). The output will have\nwithin JSON values but\r\nbetween lines.Consider using consistent line endings throughout:
🔎 Proposed fix for consistent line endings
// Helper to indent JSON output - // Note: JSON.stringify always uses \n, so we split on \n but join with EOL for platform consistency const indentLines = (jsonStr: string, spaces: number): string => { const indent = ' '.repeat(spaces); - return jsonStr.split('\n').map((line, idx) => idx === 0 ? line : indent + line).join(EOL); + return jsonStr.split('\n').map((line, idx) => idx === 0 ? line : indent + line).join('\n'); };Then replace all
EOLusage with'\n'for consistent JSON output, or normalize the entire output at the end.src/sdk/scanner/Scanner.ts (1)
526-532: Potential memory concern with large individual results.Line 530 calls
JSON.stringify(result, null, 2).replace(/\n/g, '\n ')which creates multiple string copies for each result entry. For files with very large result objects, this could consume significant memory. Consider whether individual entries are expected to be large.This is acceptable if individual file results are expected to remain reasonably sized, but worth noting for future optimization if large results become common.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
CHANGELOG.mdpackage.jsonsrc/cli/commands/scan.tssrc/sdk/Clients/Cryptography/ICryptographyClient.tssrc/sdk/Cryptography/Helper/ResultCollector/Component/ComponentCryptographyResultColletor.tssrc/sdk/scanner/Scanner.tssrc/sdk/scanner/ScannerCfg.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/sdk/scanner/ScannerCfg.ts
- package.json
🧰 Additional context used
🧬 Code graph analysis (2)
src/cli/commands/scan.ts (4)
src/sdk/Clients/Dependency/IDependencyClient.ts (1)
DependencyResponse(32-35)src/sdk/Cryptography/CryptographyTypes.ts (2)
LocalCryptography(65-69)CryptographyResponse(75-80)src/sdk/Logger/ScanossLogger.ts (1)
error(19-21)src/sdk/Logger/Logger.ts (1)
error(33-37)
src/sdk/scanner/Scanner.ts (3)
src/index.ts (1)
logger(64-64)src/sdk/scanner/ScannerTypes.ts (1)
ScannerResults(70-70)src/sdk/scanner/ScannnerResultPostProcessor/rules/rule-factory.ts (1)
ScannerResultsRuleFactory(6-22)
🔇 Additional comments (13)
src/sdk/Clients/Cryptography/ICryptographyClient.ts (1)
23-26: LGTM!The addition of
requirementtoComponentAlgorithmcreates symmetry withComponentHintResponse(line 38-41) and aligns with the updated result collection keying inComponentCryptographyResultColletor.CHANGELOG.md (2)
5-11: LGTM!The changelog entries accurately document the streaming changes and the
requirementfield addition. The previous issues with the empty list item and compare URL have been addressed.
199-199: Verified: Compare URL is now correct.The compare URL correctly references
v0.28.1as the base version for the0.29.0release.src/cli/commands/scan.ts (4)
42-126: Well-structured streaming transformation.The
streamAndTransformResultsfunction correctly streams scanner results while constructing the output structure. Good error handling with proper cleanup of write streams on pipeline errors.
303-327: Streaming for cryptography component list extraction.Good use of streaming to extract component data for cryptography scanning when the API key is provided. This avoids loading the entire file into memory for the main JSON output path while still supporting the cryptography feature.
330-344: Sensible 2GB limit for HTML format.Good safeguard to prevent OOM errors for HTML generation, with a clear error message guiding users to use JSON format instead.
367-376: JSON output now fully streams.The JSON format path correctly uses
streamAndTransformResultsto avoid loading the entire scanner result file into memory.src/sdk/Cryptography/Helper/ResultCollector/Component/ComponentCryptographyResultColletor.ts (1)
30-31: Verify keying strategy change fromversiontorequirement.The map key changed from
${purl}@${version}to${purl}@${requirement}. Components with the samepurlandrequirementbut differentversionvalues will now share the same result entry, potentially overwriting each other. Confirm this is the intended behavior for the cryptography result aggregation.src/sdk/scanner/Scanner.ts (5)
85-87: Good addition of write stream fields for resource management.Adding dedicated write stream fields enables proper lifecycle management and cleanup, which is essential for the streaming approach.
501-577: Well-implemented NDJSON to JSON conversion with proper error handling.The streaming conversion handles the NDJSON format correctly. Previous review concerns have been addressed:
- Parse errors are now logged (line 534) instead of silently skipped
fs.rmSynccalls are wrapped in try/catch (lines 558-563, 569-573) to prevent masking the original error
612-648: Robust stream lifecycle management.The
initializeWriteStreamsandcloseWriteStreamsmethods properly manage stream resources. ThecloseWriteStreamscorrectly waits for both streams to finish before resolving.
650-681: NDJSON output with proper processing pipeline.The
appendOutputFilesmethod correctly:
- Writes WFP content via stream
- Applies deobfuscation when configured
- Applies settings rules when present
- Writes each result as a separate NDJSON line
This maintains the streaming benefit while preserving existing functionality.
718-725: Proper stream cleanup in abort path.Calling
end()on write streams during abort prevents resource leaks. This is important for error scenarios and manual scan termination.
What's Changed
Changed
requirementin component crypto algorithm responseSummary by CodeRabbit
New Features
Bug Fixes / Reliability
Chores
✏️ Tip: You can customize this high-level summary in your review settings.