fix: close the ungated idealized-palette hole and the remaining audit findings#65
Merged
Conversation
… of truth The `PALETTES` record in packages/javascript/src/palettes.ts was hand-written and completely ungated. In the ColorScheme path, ditherImage() gets palette *indices* from WASM (computed against Rust's PALETTE_* statics) but returns the *colors* from that TS object, where key order IS the palette index -- and nothing tied the two together. Swapping yellow and red in PALETTES[BWRY] left vitest, check_enum_parity.py and gen_ts_palettes --check all green while shipping red where the hardware puts yellow. Commit c53441a established "generate TypeScript from the Rust source of truth" for the eight measured palettes; this extends the same generator to the nine idealized ones, emitting SCHEME_PALETTES from ColorScheme::palette() + color_names() so the existing --check CI gate covers both. palettes.ts now narrows the generated record instead of duplicating it; getPalette/getColorCount/fromValue/ColorPalette and every import path are unchanged, and the generated values are byte-equivalent in data (key order, RGB, accent) to the hand-written table they replace. Also adds a vitest table pinning key order and literal RGB values for all nine schemes, written by hand rather than derived from the code under test -- the existing tests pinned key order for only two schemes and checked RGB only for 0 <= c <= 255, so a typo passed everywhere.
measured_palettes_follow_canonical_color_order compared only names, lengths and an accent_idx bound -- it never read entry.palette.colors. Swapping the yellow and red rows inside BWRY_3_97 passed it, then propagated to Python (which derives from CATALOG via FFI) and to the generated TypeScript: wrong inks on a real 3.97" panel. Adds a per-name hue assertion over every CATALOG entry, plus an exact accent_idx match against the scheme's canonical palette (previously only bounds-checked). Thresholds are deliberately loose because these are photographed values, not pure sRGB: they detect a misplaced row, they do not pin calibration. An unrecognised color name panics so a new ink cannot slip through unchecked.
It covers unknown-discriminant conversions only; composite::InvalidRgbaLength is a deliberately separate typed struct carrying the offending len.
- Both READMEs documented BWRY as "black, white, red, yellow". The wire order everywhere in code is black, white, YELLOW, red (PALETTE_BWRY; firmware BBEP_YELLOW=2, BBEP_RED=3). Since the same READMEs get BWGBRY right, the BWRY line read as authoritative rather than as a typo -- and py-opendisplay still needs encode_image branches written for these schemes. - packages/javascript/README.md advertised "8 Color Schemes ... GRAYSCALE_4/8/16": GRAYSCALE_8 no longer exists, the count is 9, and SEVEN_COLOR and BWGBRY_SPLIT were missing. Now matches the root README. - The demo scheme pickers (docs/index.html, demo.html, dev.html) listed eight schemes and omitted BWGBRY_SPLIT. Every other documented ink order in both READMEs was checked against the palette definitions and is correct.
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
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.
Follow-up to the 7-PR audit remediation (
6970ecc), closing the six findings from the whole-set review. No dithered output changes: the three visual-regression fixture tests pass unchanged and noColorSchemevalue, palette order, or RGB triple was modified.1. CRITICAL — idealized per-scheme palettes were completely ungated
PALETTESinpackages/javascript/src/palettes.tswas hand-written. In theColorSchemepath,ditherImage()gets palette indices from WASM (computed against Rust'sPALETTE_*statics) but returns the colors from that TS object, where key order is the palette index — and nothing tied the two together. The reviewer proved it: swappingyellowandredinPALETTES[BWRY]left vitest 48/48 green,check_enum_parity.pygreen (it parses enum names and integers only), andgen_ts_palettes --checkgreen (it covered only the measured palettes). A JS consumer would get index 2, readresult.palette[2], and render/encode red where the hardware puts yellow.gen_ts_palettes.rsnow also emitsSCHEME_PALETTESfromColorScheme::palette()+ColorScheme::color_names(), so the existing--checkCI gate covers the idealized palettes too.palettes.tsnarrows the generated record instead of duplicating it.getPalette,getColorCount,fromValue,ColorPalette, and every import path.SCHEMESarray +CATALOGorder, no timestamps, no absolute paths); two consecutive--stdoutruns are byte-identical.assert_schemes_complete()aborts generation if a new enum variant is added but forgotten inSCHEMES.SEVEN_COLORandBWGBRY_SPLIThad key order pinned and RGB was only range-checked0 <= c <= 255.Acceptance test — repeating the reviewer's swap at the new source of truth (
PALETTE_BWRYinpalettes.rs) now fails three ways:cargo test→color_names_match_expected_rgb_valuesBwry: palette().colors[2] (yellow) RGB mismatch, left: [255, 0, 0]gen_ts_palettes -- --checkfirst differing line: 56 / committed: yellow: { r: 255, g: 255, b: 0 } / generated: yellow: { r: 255, g: 0, b: 0 }vitest(after regenerating, so the swap reaches TS)ColorScheme > scheme 3 has exact palette key order and RGB values2. IMPORTANT — measured RGB triples were not tied to their color names
measured_palettes_follow_canonical_color_ordercompared names, lengths and anaccent_idxbound, but never readentry.palette.colors. Swapping the yellow and red rows insideBWRY_3_97passed, then propagated to Python (which derives fromCATALOGvia FFI) and to the generated TypeScript.New
measured_colors_match_their_namesasserts a loose hue sanity check per color name over everyCATALOGentry, andaccent_idxis now matched exactly against the scheme's canonical accent rather than only bounds-checked. Thresholds are swap detectors, not calibration pins — these are photographed values (measured yellow is[172, 128, 0], measured white[168, 180, 182]), so a re-measurement should never need to touch them. An unrecognised color name panics.Proof: swapping yellow/red in
BWRY_3_97→BWRY_3_97: yellow = [85, 24, 14]: yellow must have R and G high.3. IMPORTANT — stale
GRAYSCALE_8shipping to npmpackages/javascript/README.mdadvertised8 Color Schemes: ... GRAYSCALE\_4/8/16.GRAYSCALE_8was removed in protocol v2, the count is 9, andSEVEN_COLOR/BWGBRY_SPLITwere missing. Now matches the root README.4. IMPORTANT — both READMEs documented the BWRY ink order backwards
packages/javascript/README.mdsaidBWRY = 3, // Black, White, Red, Yellowandpackages/python/README.mdsaidBlack, white, red, yellow. The wire order everywhere in code is black, white, yellow, red (PALETTE_BWRY; firmwareBBEP_YELLOW=2,BBEP_RED=3). Both fixed. Every other documented ink order in both READMEs was checked against the palette definitions and is correct.5. MINOR — error type clarification
One-line doc note on
DitherErrorstating it is not the crate-wide error type.composite::InvalidRgbaLengthis deliberately left separate, per the reviewer's explicit recommendation.6. TRIVIAL —
BWGBRY_SPLITmissing from the demosAdded to the scheme picker and
SCHEMESmap indocs/index.html,packages/javascript/demo.htmlandpackages/javascript/dev.html.Verification
WASM was rebuilt before the JS runs.