Skip to content

zlib: accept zlib flush constants in brotli set_flush instead of panicking - #32358

Closed
robobun wants to merge 5 commits into
mainfrom
farm/4f40f9b6/brotli-zlib-flush-panic
Closed

zlib: accept zlib flush constants in brotli set_flush instead of panicking#32358
robobun wants to merge 5 commits into
mainfrom
farm/4f40f9b6/brotli-zlib-flush-panic

zlib: accept zlib flush constants in brotli set_flush instead of pani…

14bfabe
Select commit
Loading
Failed to load commit list.
Claude / Claude Code Review completed Jun 15, 2026 in 12m 39s

Code review found 1 potential issue

Found 5 candidates, confirmed 1. See review comments for details.

Details

Severity Count
🔴 Important 0
🟡 Nit 1
🟣 Pre-existing 0
Severity File:Line Issue
🟡 Nit test/js/node/zlib/zlib.test.js:332-335 Tests cover BrotliDecompress only, not BrotliCompress

Annotations

Check warning on line 335 in test/js/node/zlib/zlib.test.js

See this annotation in the file changed.

@claude claude / Claude Code Review

Tests cover BrotliDecompress only, not BrotliCompress

nit: `createBrotliCompress().flush(Z_FINISH)` hit the same `unreachable!()` before this fix (set_flush is shared between encode and decode), and the new code comment explicitly justifies encoder semantics — but the tests only exercise `createBrotliDecompress()`. Per CLAUDE.md §Tests ("Cover the variant matrix, not just the repro. Every sibling entry point receiving the same fix"), consider adding a `BrotliCompress` row to this `it.each`.