color: ansi-16, ansi-256 and hsl/lab all produced unusable output - #33328
Conversation
WalkthroughThis PR fixes ansi-16 escape generation so the color index is written as decimal digits, updates related hsl/lab formatting, refreshes docs and type examples, and adds tests for ANSI output, round-trips, and input parsing. ChangesColor output formatting
Sequence Diagram(s)flowchart TD
js_function_color --> output_buffer
output_buffer --> return_string
ansi256_get16 --> js_function_color
Possibly related issues
Related PRs: None referenced. Suggested labels: bug, docs, tests Suggested reviewers: None determinable from provided data. Poem 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Good catch on #22161, and it pushed the fix further than I had it. My first pass only stopped So it now emits real 16-color SGR parameters, $ bun -e 'for (const c of ["red","blue","lime","white"]) console.log(c, JSON.stringify(Bun.color(c, "ansi-16")))'
red "\u001b[91m"
blue "\u001b[94m"
lime "\u001b[92m"
white "\u001b[97m"which is exactly the table in the issue. Added On the docs URL: fixed, |
3e5e02c to
5454d26
Compare
|
That is a real bug and a good one, thanks. Fixed in f992406 along with the test that should have caught it. Confirmed, and a bit wider than the analysis suggests: 115 of the 216 colors with $ bun -e 'console.log(JSON.stringify(Bun.color("#020202", "ansi-256")))'
"\u001b[38;5;429496961m" # was
"\u001b[38;5;16m" # now
The sharpest part of this is about my own test: 961 tests, 0 fail with the fix. |
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 `@src/css_jsc/color_js.rs`:
- Around line 490-509: The 16-color SGR logic in the color conversion path is
fine, but the explanatory comment above the `sgr` calculation is too long and
exceeds the 3-line comment guideline. Trim the multi-line comment in
`color_js.rs` near the `ansi256::get16` and `sgr` computation so it stays
concise while still making the 30..=37/90..=97 mapping clear.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dca4f3e8-ee95-4b18-996f-aca442a2303d
⛔ Files ignored due to path filters (1)
test/js/bun/css/__snapshots__/color.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
docs/runtime/color.mdxsrc/css_jsc/color_js.rstest/js/bun/css/color.test.ts
Bun.color(x, "ansi-16") built the escape as \x1b[38;5;{index}m and wrote the
palette index straight into the string as a single byte, so index 9 came out as
a tab and index 12 as a form feed. No terminal renders \x1b[38;5;<TAB>m, which
means ansi-16 has never produced usable output.
Both halves were wrong. The index now goes in as decimal digits, and it goes in
as a 16-color SGR parameter (30..=37, or 90..=97 for the bright variants) rather
than the 38;5;{index} form that only a 256-color terminal understands. Red is
now \x1b[91m. Emitting a 256-color escape from ansi-16 defeated the one case the
format exists for: Bun.color(x, "ansi") picks ansi-16 exactly when the terminal
cannot do 256 colors.
Nothing can have depended on the old output, since it was not a valid sequence.
The old output was corrupting the test snapshot too: index 13 is a carriage
return, which the snapshot writer normalized to a newline, so the stored value
for magenta claimed index 10.
Adds assertion-based tests for all three ansi formats, a round-trip property for
the css string formats, and the input forms. Two formats do not round-trip and
are marked test.failing: hsl prints saturation and lightness as 0..1 rather than
percentages, and lab prints lightness as 0..1 rather than 0..100, so Bun cannot
parse back the strings it just produced.
ansi256::get is a port of tmux's colour_find_rgb, which computes the grey index as (grey_avg - 3) / 10 in signed int, so an average below 3 truncates to 0. The port does it on u32 with wrapping_sub, so "#20202" produced index 429496961: $ bun -e 'console.log(JSON.stringify(Bun.color("#20202", "ansi-256")))' "\u001b[38;5;429496961m" 115 of the 216 colors with r,g,b < 6 were affected. get16 masks the index with 0xff, so ansi-16 turned near-black into bright blue. The SGR regex the new tests use accepts any digits, so it would have passed a 429-million index happily. They now assert the index stays inside the palette, sweeping the grey ramp where the arithmetic underflows.
f992406 to
a2651db
Compare
There was a problem hiding this comment.
LGTM — the old ansi-16 output was never a valid escape sequence, so this is a pure fix with no compat surface, and both follow-ups from earlier review are in.
Extended reasoning...
Overview
Fixes Bun.color(input, "ansi-16"), which previously wrote the 0–15 palette index into the escape sequence as a raw byte (so index 9 became a literal tab). The fix maps the index to a real 16-color SGR parameter (30..=37 / 90..=97) and formats it as decimal, mirroring the ansi-256 arm's approach. A second one-line fix changes wrapping_sub(3) → saturating_sub(3) in ansi256::get() so near-black inputs no longer produce a ~429M palette index (raised in my earlier inline comment; now fixed and tested). Docs and the snapshot are regenerated to match, and ~140 lines of contract-based tests are added.
Security risks
None. This is pure string formatting of a color value into an ANSI escape sequence — no I/O, no parsing of untrusted structured data beyond what the existing CSS color parser already handled, no auth/crypto/permissions.
Level of scrutiny
Low-to-moderate. Bun.color is a leaf utility API; the change is confined to one match arm plus a one-token arithmetic fix in a helper it calls. The output-format change (\x1b[91m instead of \x1b[38;5;9m) is technically a design choice, but the previous output contained raw control bytes and was unusable by any terminal, so there is no compatibility surface to break. The chosen form matches what issue #22161 explicitly requested and is the only form a 16-color-only terminal actually understands, which is the whole reason ansi-16 exists.
Other factors
- Both of my earlier inline comments (docs URL,
saturating_subunderflow) were addressed in follow-up commits and are marked resolved. - Buffer sizing is safe:
get16returns 0–15, sosgris 30–97 (2 digits), and"\x1b[" + "NN" + "m"= 5 bytes into an 8-byte buffer. - New tests assert the documented contract (SGR regex, palette bounds, exact values from the linked issue,
Bun.stringWidthzero-width) rather than re-snapshotting; the author verified that reverting each fix independently fails specific tests. - Two
test.failingcases document pre-existinghsl/labround-trip bugs without changing their output — appropriate restraint since those touch the documented.d.tsexamples. - No CODEOWNERS on these paths; bug-hunter found nothing.
Bun.color(x, "hsl") printed saturation and lightness as the raw 0..1 values it
stores them as, and Bun.color(x, "lab") printed lightness the same way with
comma separators. Neither is CSS, and Bun's own parser rejects both, so the
output of Bun.color could not be fed back into Bun.color:
$ bun -e 'console.log(Bun.color("red", "hsl"))'
hsl(0, 1, 0.5)
$ bun -e 'console.log(Bun.color("hsl(0, 1, 0.5)", "hex"))'
null
hsl() takes percentages for saturation and lightness, and lab() takes lightness
as a percentage and is space-separated, which is also what the CSS printer in
src/css emits. An achromatic color has no hue, so a grey was printing
hsl(NaN, ...); it now prints a zero hue.
hsl now round-trips across a sweep of the color cube. lab parses back but still
loses saturated blues, because the lab <-> sRGB conversion is inaccurate there
independently of the formatting; that one stays marked test.failing with the
repro.
|
Took the The output format is not a judgement call:
So The one piece I am still not claiming: $ bun -e 'console.log(Bun.color(Bun.color("#0000f8", "lab"), "hex"))'
#002be3That is not formatting. 963 pass, 0 fail. |
There was a problem hiding this comment.
Both of my earlier points are addressed and I didn't find anything new — this looks correct to me, but since it changes the user-visible output of three Bun.color formats (ansi-16, hsl, lab) and rewrites the full snapshot, a maintainer should sign off on the format choices.
Extended reasoning...
Overview
This PR fixes three broken output formats in Bun.color:
ansi-16was emitting a raw control byte as the palette index and using the 256-color38;5;Nform; it now emits real 16-color SGR parameters (\x1b[30-37m/\x1b[90-97m).ansi-256underflowed the grey-ramp index for near-black inputs viawrapping_subonu32; nowsaturating_sub, matching the tmux reference.hslandlabprinted raw 0..1 floats instead of CSS percentages/space-separated form, so their output wasn't valid CSS and couldn't be re-parsed byBun.coloritself. Achromatic hue is now 0 instead of NaN.
It also updates docs/runtime/color.mdx, the .d.ts @example lines, regenerates the snapshot (all 432 entries, since the writer now escapes control chars), and adds ~160 lines of new assertions in test/js/bun/css/color.test.ts including a test.failing documenting a pre-existing lab↔sRGB conversion inaccuracy.
Security risks
None. This is pure output-string formatting for a color utility — no parsing of untrusted input was changed, no auth/crypto/filesystem/network paths touched.
Level of scrutiny
Medium. The individual fixes are small and well-reasoned (the saturating_sub change is one word; the ansi-16 SGR mapping 30+i / 82+i for i<8 / i>=8 is correct; the buffer sizing is safe). However, this changes user-visible output for three documented formats. The PR argues convincingly that all three were unusable as-is (invalid escape sequences, not-CSS strings the docs already contradicted), so nothing could have depended on the exact old output — but that's a maintainer judgment call, not something I should shadow-approve.
Other factors
- Both of my prior inline comments (docs URL nit,
wrapping_subunderflow) were addressed in follow-up commits and the threads are resolved. - The snapshot file is rewritten wholesale (binary diff), which the PR description explains but I can't independently verify entry-by-entry.
- A
test.failingis added for a separate pre-existing lab conversion bug — reasonable as documentation, but a maintainer may prefer it filed as an issue instead. - No human reviewer has looked at this yet.
… direction The forward sRGB -> Lab conversion matches the CIELAB D50 reference values exactly, which is what locates the bug in the inverse. Filed as #33331.
|
Both fair. Addressed in 174c902. On the snapshot being unverifiable: you're right that "trust me, only 108 changed" is not good enough for a binary diff. The description now carries the script that proves it, so anyone can run it instead of squinting: On the lab Chasing it down, the bug is not confined to $ echo 'a { color: lab(29.568% 68.287 -112.029); }' > lab.css # that is #0000ff
$ bun build lab.css --minify
a{color:#002cea;color:color(display-p3 .00000346541 -.00000245467 .95958);color:lab(...)}So a browser that falls back to the hex renders pure blue as a washed-out The direction is now pinned too. The forward conversion is exact, matching the CIELAB D50 reference to four decimals for all three primaries; feeding the exactly-correct Lab for blue back in still returns On maintainer sign-off for the format choices: right instinct, and I am not trying to shadow-land it. But I would push back on calling them choices. In all three cases the old output was not usable by anything:
So there is no compat surface to weigh, and the format is whatever the docs and the |
There was a problem hiding this comment.
LGTM — each fix is small and verifiable, the old outputs were objectively unusable, and the new ones match what the docs already promised.
Extended reasoning...
Overview
Four files touched: src/css_jsc/color_js.rs (~40 lines across four output arms of Bun.color), docs/runtime/color.mdx (5 example lines), packages/bun-types/bun.d.ts (2 JSDoc @example lines), and test/js/bun/css/color.test.ts (+170 lines of assertion-based tests replacing/supplementing snapshots). The Rust changes are: wrapping_sub → saturating_sub in the tmux grey-ramp port; the ansi-16 arm rewritten to emit real 16-color SGR parameters (30..37/90..97) as decimal digits instead of a raw palette-index byte; the hsl arm now emits percentages and guards NaN hue; the lab arm now emits space-separated with percentage lightness.
Security risks
None. This is pure output-string formatting of already-parsed color values — no new parsing of untrusted input, no I/O, no auth, no allocation driven by user-controlled sizes. The one buffer write ([0u8; 8]) is bounded: 2-byte prefix + at most "97m" = 5 bytes, and get16() provably returns 0–15 via TABLE_256.
Level of scrutiny
Moderate — it changes the string output of a public API in three formats. But the old outputs were not something anyone could depend on: ansi-16 embedded a raw control byte (tab, form-feed) mid-escape; ansi-256 emitted a 9-digit palette index for near-black; hsl/lab emitted strings Bun's own parser rejects and that never matched docs/runtime/color.mdx. This is the implementation catching up to its documented contract, not a contract change. I verified the SGR arithmetic (82 + index for index ≥ 8 gives 90–97), the saturating_sub matches tmux's signed-int truncation, and the * 100.0 scaling matches how s/l/L are stored.
Other factors
Test coverage is strong: exact-value pins for #22161, property sweeps over the color cube and grey ramp, round-trip checks for every CSS-string format, a Bun.stringWidth zero-column check, and a test.failing pinning the separate pre-existing lab↔sRGB conversion inaccuracy to its own issue. Both of my earlier inline comments (docs URL, wrapping_sub underflow) and CodeRabbit's comment-length nit are addressed and resolved. The one item flagged this run — none in non-hue hsl/lab components still surfaces as NaN — is explicitly pre-existing, niche (CSS Color 4 none keyword), and strictly no worse than before; it does not block.
CSS Color 4's none keyword stores a missing component as NaN, and the hue of an
achromatic color is NaN too. The guard I added only covered the hue, so
Bun.color("hsl(120 none 50%)", "hsl") still emitted hsl(120, NaN%, 50%), which
no CSS parser accepts. A missing component is a zero value outside of
interpolation, so all of them are now guarded.
Also stop asserting the lab reference values as exact strings. The conversion
goes through cbrt, so the last f32 digit differs by platform: Windows and darwin
aarch64 print 69.891 where linux x64 prints 69.89099. Compared numerically now.
f94c494 to
05f9f24
Compare
|
Good flag, and it caught a second thing on the way in. Both in 05f9f24. The $ bun -e 'console.log(Bun.color("hsl(120 none 50%)", "hsl"))'
hsl(120, NaN%, 50%)
$ bun -e 'console.log(Bun.color("lab(none 40 30)", "lab"))'
lab(NaN% 40 30)Now all five components go through one $ bun -e 'console.log(Bun.color("hsl(120 none 50%)", "hsl"))'
hsl(120, 0%, 50%)
$ bun -e 'console.log(Bun.color(Bun.color("hsl(120 none 50%)", "hsl"), "hex"))'
#808080And the CI failure on the last push was my own doing, worth recording. The commit before this one added: expect(color("#ff0000", "lab")).toBe("lab(54.290546% 80.80492 69.89099)");Windows and darwin aarch64 print I swept the rest of my new assertions for the same smell and relaxed one more ( 968 pass, 0 fail. |
#33328 landed the same hsl()/lab() output-format fix, so the conflicting arms now use main's `zero_if_none` formatting; this branch keeps the null-for-unconvertible guard it adds in front of them and leaves main's ansi-16 and ansi-256 fixes untouched.
Fixes #22161
Bun.colorhad three output formats that produced strings nothing could use. A round-trip property test (feed each output back intoBun.color) found all of them.On the three-in-one scope: the bot keeps flagging it. They are three arms of the same
matchin the same function, all found by the same test, and all of the same shape (the output string is not something any consumer can parse). Splitting them would mean three PRs regenerating the same snapshot in sequence. Happy to split if a maintainer prefers.1.
ansi-16emitted a control byte instead of an SGR codesrc/css_jsc/color_js.rswrote the palette index into the string as a single byte rather than as decimal digits:Both halves of that sequence were wrong. The index now goes in as decimal digits, and as a 16-color SGR parameter (
30..=37,90..=97for the bright variants) rather than38;5;{index}, which is the 256-color form. Red is now\x1b[91m, matching #22161.That second half matters:
Bun.color(x, "ansi")selectsansi-16exactly when the terminal reports it cannot do 256 colors, and such a terminal does not understand38;5;N. The format was emitting an escape that the one terminal it exists to serve cannot read. (The old code comment even said "5 is the 16-color mode", which is not what38;5;Nmeans.)Nothing can have depended on the old output, since it was not a valid escape sequence.
2.
ansi-256underflowed the grey ramp for near-black colorsansi256::getports tmux'scolour_find_rgb, which computes the grey index as(grey_avg - 3) / 10in signedint, so an average below 3 truncates to0. The port does it onu32withwrapping_sub. 115 of the 216 colors withr,g,b < 6were affected, and throughget16's& 0xffmaskansi-16rendered near-black as bright blue. Nowsaturating_sub, which matches tmux.3.
hslandlabemitted strings that are not CSSSaturation, lightness and L* are stored as
0..1and were printed raw.hsl()takes percentages andlab()takes lightness as a percentage and is space-separated. Bun's own CSS parser rejects both of the strings above, soBun.color's output could not be fed back intoBun.color.This is not a judgement call, it is what the repo already says:
docs/runtime/color.mdxhas always documented the"hsl"output as"hsl(120, 50%, 50%)". The implementation never matched its own docs.src/cssprintslab(54.29% 80.8 69.89)for the same color.Bun.coloritself only parseshsl(h, s%, l%)andlab(L% a b).So:
hsl(0, 100%, 50%)andlab(54.29% 80.80492 69.89099). Also, an achromatic color has no hue, so a grey used to printhsl(NaN, 0%, 50%); it now prints a zero hue.hslround-trips across a sweep of the color cube.The
.d.ts@examplelines documented the old broken output and are updated.Tests
Snapshots pin whatever the implementation happens to emit, which is how all of this survived. The snapshot was also corrupt: index 13 (magenta) is a carriage return, which the snapshot writer normalized to a newline, so magenta's entry recorded index 10.
Assertions now, not snapshots:
ansi-16output matches/^\x1b\[(3[0-7]|9[0-7])m$/, swept across the color cube, so a 256-color escape can never come back.ansi-256never emits an index outside the 256-entry palette, swept along the grey ramp where the arithmetic underflowed. A valid-looking38;5;429496961mis all digits, so the SGR regex alone would have let it through.91, green92, blue94, white97.Bun.stringWidth(Bun.color("red", format) + "hello") === 5, since a terminal skips the escape.nullfor unparseable input.Reverting any one of the three fixes turns specific tests red.
Regenerating the snapshot rewrites all 432 entries because the current writer escapes control characters as
\x1Binstead of embedding them raw, not because 432 values changed. Only the 108ansi-16values changed in meaning. Rather than take my word for it on a binary diff, this normalizes both escapings and compares entry by entry:verify the snapshot
One thing left, with the repro
laboutput now parses, but it does not round-trip for saturated blues:Filed separately as #33331, and the
test.failingnow points at it. It is worse than aBun.colorproblem: the same conversion feeds the CSS bundler's sRGB fallback forlab()colors, solab(29.568% 68.287 -112.029)(pure blue) compiles to#002cea.The direction is pinned. The forward conversion is exact, matching the CIELAB D50 reference to four decimals:
#ff0000lab(54.290546% 80.80492 69.89099)#0000fflab(29.5683% 68.287384 -112.02972)Feeding that exactly-correct Lab back in returns
#002cea, so the inverse is not the inverse. Not fixed here: that is colour-science to be done against reference vectors rather than guessed at, and it is a different bug from "the string is not CSS". This PR adds a passing test for the forward direction so whoever picks up #33331 knows which half to look at.Verification