diff --git a/PR_2862_REVIEW.md b/PR_2862_REVIEW.md new file mode 100644 index 000000000..24230d147 --- /dev/null +++ b/PR_2862_REVIEW.md @@ -0,0 +1,218 @@ +# PR #2862 Review: Fix bug where output gets truncated when too long + +## Summary +✅ **RECOMMENDATION: APPROVE AND MERGE** + +This PR fixes a legitimate bug where large SARIF output (~1MB+) gets truncated when piped to stdout. The fix is minimal, correct, and well-tested. + +## Problem Analysis + +### The Bug +When Spectral CLI outputs large results (e.g., SARIF format with ~200 paths = ~1MB output), the output gets truncated because: + +1. `process.stdout.write()` is a non-blocking operation +2. When writing large amounts of data, Node.js may apply backpressure +3. The original code didn't wait for the write to complete or handle backpressure +4. The function returns immediately while data is still being written +5. When the process exits, any unflushed data in the buffer is lost + +### Root Cause +```typescript +// Original code - PROBLEMATIC +export async function writeOutput(outputStr: string, outputFile: string): Promise { + if (outputFile !== '') { + await fs.writeFile(outputFile, outputStr); + } else { + process.stdout.write(outputStr); // Returns immediately, doesn't wait for flush + } +} +``` + +The issue occurs when: +- Large output is written to stdout +- The write operation returns `false` (indicating backpressure) +- The function returns immediately without waiting +- Process exits before all data is flushed + +## Solution Analysis + +### The Fix +```typescript +// Fixed code - CORRECT +export async function writeOutput(outputStr: string, outputFile: string): Promise { + if (outputFile !== '') { + await fs.writeFile(outputFile, outputStr); + } else { + // Handle backpressure by using the callback parameter + // The callback is invoked when the data is flushed (or an error occurs) + return new Promise((resolve, reject) => { + process.stdout.write(outputStr, err => { + if (err) reject(err); + else resolve(); + }); + }); + } +} +``` + +### Why This Works +1. **Callback-based approach**: Uses `process.stdout.write()`'s callback parameter +2. **Promisification**: Wraps the callback in a Promise to maintain async/await compatibility +3. **Proper error handling**: Rejects on errors, resolves on success +4. **Backpressure handling**: The callback is only invoked after data is flushed +5. **Consistent with fs.writeFile**: Both branches now properly await completion + +## Code Quality Assessment + +### ✅ Strengths +1. **Minimal change**: Only changes what's necessary +2. **Well-commented**: Explains why the callback is needed +3. **Proper error handling**: Catches and propagates errors +4. **Test coverage**: Updated test to mock the callback properly +5. **Consistent API**: Maintains async/await pattern throughout +6. **No breaking changes**: Function signature and behavior remain the same for callers + +### Test Coverage +The PR updates the test to properly mock `process.stdout.write` with callback support: + +```typescript +const writeMock = jest.spyOn(process.stdout, 'write').mockImplementation((chunk: any, callback?: any) => { + if (typeof callback === 'function') { + callback(); + } + return true; +}); + +expect(await writeOutput(output, '')).toBeUndefined(); +expect(writeMock).toHaveBeenCalledWith(output, expect.any(Function)); + +writeMock.mockRestore(); +``` + +This test: +- ✅ Properly mocks the callback parameter +- ✅ Verifies the callback is passed +- ✅ Cleans up the mock after the test +- ✅ Tests the async behavior correctly + +## Verification + +### Manual Testing Results +I tested both the broken and fixed versions: + +**Without fix (develop branch)**: +```bash +❌ FAIL: SARIF output truncated +``` + +**With fix (PR branch)**: +```bash +✅ PASS: SARIF output pipes correctly (979508 bytes) +``` + +The fix successfully handles ~1MB of output without truncation. + +### Automated Testing +All existing tests pass: +``` +PASS @stoplight/spectral-cli packages/cli/src/services/__tests__/output.test.ts +Test Suites: 1 passed, 1 total +Tests: 5 passed, 5 total +``` + +## Performance Considerations + +### Impact: Minimal to None +- **Small outputs**: No measurable difference (callback overhead is negligible) +- **Large outputs**: Slightly slower but correct (must wait for flush) +- **Files**: No change (already used await with fs.writeFile) + +The performance impact is acceptable because: +1. Correctness > Speed (truncated output is useless) +2. The overhead is minimal for typical use cases +3. Large outputs already had performance constraints + +## Security Considerations + +### No Security Issues +- ✅ No new dependencies +- ✅ No external inputs +- ✅ Proper error handling +- ✅ No code execution paths +- ✅ No data leakage + +## Compatibility + +### Backward Compatible +- ✅ No breaking changes +- ✅ Function signature unchanged +- ✅ Behavior unchanged for small outputs +- ✅ Fixes behavior for large outputs (previously broken) + +## Documentation + +### Adequate Documentation +- ✅ In-code comments explain the fix +- ✅ PR description references the issue +- ✅ No user-facing docs needed (internal fix) + +## Recommendations + +### Primary Recommendation: ✅ MERGE +This PR should be merged because: +1. **Fixes a real bug**: Demonstrated with reproduction case +2. **Minimal risk**: Small, focused change +3. **Well-tested**: Updated tests, all passing +4. **No breaking changes**: Fully backward compatible +5. **Good code quality**: Clear, commented, follows best practices + +### Optional: Future Enhancements +While not necessary for this PR, future improvements could include: +1. **Chunked writing**: For extremely large outputs (>10MB), could write in chunks +2. **Progress indication**: For very large files, show progress +3. **Stream-based API**: Replace string concatenation with streaming for memory efficiency + +However, these are NOT blockers for this PR and would be separate enhancements. + +## Conclusion + +**APPROVE AND MERGE** ✅ + +This PR successfully fixes a legitimate bug where large Spectral output gets truncated. The fix is: +- Minimal and focused +- Technically correct +- Well-tested +- Backward compatible +- Properly documented + +There are no blocking issues or concerns. The PR is ready to merge. + +--- + +## Testing Evidence + +### Test Run: develop (without fix) +``` +❌ FAIL: SARIF output truncated +``` + +### Test Run: pr-2862 (with fix) +``` +✅ PASS: SARIF output pipes correctly (979508 bytes) +``` + +### Unit Tests +``` +PASS @stoplight/spectral-cli packages/cli/src/services/__tests__/output.test.ts + Output service + formatOutput + ✓ calls stylish formatter with given result + ✓ calls json formatter with given result + ✓ calls junit formatter with given result + writeOutput + ✓ given outputFile, writes output to a specified path + ✓ given , print output to console + +Test Suites: 1 passed, 1 total +Tests: 5 passed, 5 total +```