From f7cfe3d71022237117fc0c4912832a4ada8a244c Mon Sep 17 00:00:00 2001 From: Aadharsh Pannirselvam <19518661+oddharsh@users.noreply.github.com> Date: Fri, 14 Aug 2026 12:39:53 -0400 Subject: [PATCH] chore(tools): declare the system binaries, and check them Four external binaries were required by scripts in this repo and documented nowhere: cwebp, ffmpeg, ssimulacra2 and butteraugli_main. On a clean machine each one arrived as a raw shell error partway through a run. They stayed hidden because most of these preconditions are written `for cmd in sips exiftool`, so the binary name exists only as a loop word and a grep for the name finds nothing. That is gotcha 29's blind spot in different clothes: a command assembled from list elements is invisible to a search for the assembled form. config/tools.json declares all 13, and `pnpm run tools:check` verifies them. It sits outside infra.json because that file declares Cloudflare and GitHub state and diffs it against those APIs, while nothing here is remote. Tiered the way infra:check is. The DECLARATION tier reads source text, needs no binary, and gates in CI: every guard loop, every literal `command -v`, and every `brew install` hint must have an entry, and both CLAUDE.md and MAINTENANCE.md must name it. The PRESENCE tier probes the machine and stays advisory in CI, since a hosted runner carries none of the photo toolchain. Each scanner carries a floor and fails if its match count collapses, because a scanner that matches nothing otherwise reports a pass while checking nothing. Six controls, all measured rather than assumed: an undeclared literal probe fails, an undeclared loop word fails, dropping a tool from both docs fails, a stale required_by path fails, CI with the tools absent passes, and a workstation with them absent fails. Two accuracy fixes fell out of the sweep. ssimulacra2 and butteraugli_main are libjxl tools that `brew install jpeg-xl` does not ship, so the error message in export-for-instagram.sh names the wrong thing. And cjpeg is documented as mozjpeg's while gen-encoding-grids.sh calls whichever copy PATH resolves first. --- .github/workflows/ci.yml | 7 ++ CLAUDE.md | 35 ++++++ config/tools.json | 140 +++++++++++++++++++++++ docs/MAINTENANCE.md | 21 +++- package.json | 1 + scripts/check-tools.mjs | 240 +++++++++++++++++++++++++++++++++++++++ 6 files changed, 443 insertions(+), 1 deletion(-) create mode 100644 config/tools.json create mode 100644 scripts/check-tools.mjs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 049f75726..a5bd18330 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -123,6 +123,13 @@ jobs: - name: Validate LWE and Garage page contracts run: pnpm run pages:check + # Only the DECLARATION tier gates here. It reads shell source and needs no + # binary present, so it runs anywhere. The presence tier prints what is + # absent and passes, because a hosted runner carries none of the photo + # toolchain and never runs the pipeline that wants it. + - name: Check system tool declaration + run: pnpm run tools:check + - name: Test coffee Worker run: pnpm --filter cal-aadhar-sh test diff --git a/CLAUDE.md b/CLAUDE.md index c7059730b..75cb16c7d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -805,6 +805,41 @@ Two encoders + one transform tool, all built from source: this pipeline. The 64-bin RGB/luminance bake moved into `zenc histogram` on 2026-08-14, so nothing in add-photos.sh or extract-photo-metadata.sh needs it. +The four below serve the STUDY pages rather than the photo pipeline, and every +one of them was undocumented until `tools:check` went looking (2026-08-14): + +- **webp** (`brew install webp`) — `cwebp` draws the WebP point on both + `/garage/encoding` grids. +- **ffmpeg** (`brew install ffmpeg`) — one PNG-to-PPM conversion in + `gen-encoding-grids.sh`, because `cjpeg` will not read PNG and sips' BMP + output confuses it. +- **ssimulacra2** and **butteraugli_main** — the two perceptual metrics + `export-for-instagram.sh` searches quality against. These are libjxl TOOLS, + built with `-DJPEGXL_ENABLE_TOOLS=ON`, and **`brew install jpeg-xl` does not + ship them**, which is why the script falls back to `/opt/zerobrew/prefix/bin`. + The error message in that script names the formula rather than the tools and + is misleading; the binaries are what to go find. + +> **The whole list is DECLARED in [`config/tools.json`](config/tools.json) and +> `pnpm run tools:check` fails on drift.** It sits outside `infra.json` on +> purpose: that file declares Cloudflare and GitHub state and diffs it against +> those APIs, while nothing here is remote and nothing here has an API. +> +> The check is tiered the same way `infra:check` is. Its DECLARATION tier reads +> source text, needs no binary, and runs on every PR: every `for cmd in …` guard, +> every literal `command -v`, and every `brew install` hint in the shell scripts +> must have an entry, and both this file and MAINTENANCE.md must name it. Its +> PRESENCE tier probes the machine and is ADVISORY in CI, because a hosted runner +> has none of these and is not meant to. +> +> The guard scanner is the load-bearing part, and it is why four prerequisites +> could stay undocumented. Most of these preconditions are written `for cmd in +> sips exiftool`, so the binary name exists only as a loop word and a grep for the +> name finds nothing. That is gotcha 29's blind spot in different clothes: a +> command assembled from list elements is invisible to a search for the assembled +> form. Each scanner also carries a FLOOR and fails if its match count collapses, +> since a scanner that matches nothing otherwise reports a pass. + ### `` + content-addressed thumbnails Photo thumbnails are dual-encoded AVIF + JPG, served via `` from diff --git a/config/tools.json b/config/tools.json new file mode 100644 index 000000000..680951ae2 --- /dev/null +++ b/config/tools.json @@ -0,0 +1,140 @@ +{ + "$comment": [ + "The external binaries the photo and encoding pipelines shell out to, declared", + "once so `pnpm run tools:check` can verify them. NOT in infra.json on purpose:", + "that file declares Cloudflare and GitHub state and `infra:check` diffs it", + "against those APIs, while nothing here is remote and nothing here has an API.", + "Mixing them would give one file two unrelated contracts.", + "", + "This exists because the list used to live in three places that disagreed:", + "CLAUDE.md's encoder section, MAINTENANCE.md's brew line, and inline guards in", + "the scripts. ffmpeg, cwebp, ssimulacra2 and butteraugli_main were required and", + "documented nowhere, so a clean machine met them as a raw shell error.", + "", + "`optional` means a script degrades rather than exits: avifenc falls back to", + "sips in add-photos.sh and reencode-thumbnails.sh, and is REQUIRED by the two", + "encoding-grid scripts, so it is listed optional and its hard callers are named", + "in `required_by` anyway. A tool that is optional everywhere is still declared,", + "because the point is a complete list rather than a blocking one.", + "", + "`path` is for a binary a script resolves by absolute location rather than", + "PATH. jpegtran is the only one: mozjpeg is keg-only, so `command -v jpegtran`", + "can find libjpeg-turbo's copy while the scripts use mozjpeg's, and probing", + "PATH would report a pass on the wrong binary." + ], + + "tools": [ + { + "bin": "exiftool", + "install": "brew install exiftool", + "why": "EXIF, orientation, and the Fujifilm recipe card", + "required_by": [ + "www/scripts/extract-photo-metadata.sh", + "www/scripts/add-photos.sh", + "www/scripts/download-remote-photos.sh", + "www/scripts/export-for-instagram.sh", + "www/scripts/gen-encoding-samples.sh", + "www/scripts/reencode-thumbnails.sh" + ] + }, + { + "bin": "jq", + "install": "brew install jq", + "why": "JSON assembly in the metadata shell scripts", + "required_by": [ + "www/scripts/extract-photo-metadata.sh", + "www/scripts/download-remote-photos.sh" + ] + }, + { + "bin": "sips", + "install": "ships with macOS", + "why": "resize, HEIF decode, and the AVIF fallback encoder", + "required_by": [ + "www/scripts/add-photos.sh", + "www/scripts/export-for-instagram.sh", + "www/scripts/gen-encoding-grids.sh", + "www/scripts/gen-encoding-samples.sh", + "www/scripts/reencode-thumbnails.sh" + ] + }, + { + "bin": "curl", + "install": "ships with macOS", + "why": "pulls the R2 originals back down", + "required_by": ["www/scripts/download-remote-photos.sh"] + }, + { + "bin": "cargo", + "install": "https://rustup.rs", + "why": "builds zenc, the shipped JPEG encoder, on first run", + "required_by": [ + "www/scripts/add-car-photo.sh", + "www/scripts/add-photos.sh", + "www/scripts/export-for-instagram.sh", + "www/scripts/gen-encoding-grids.sh", + "www/scripts/gen-encoding-samples.sh", + "www/scripts/reencode-thumbnails.sh" + ] + }, + { + "bin": "jpegtran", + "path": "/opt/homebrew/opt/mozjpeg/bin/jpegtran", + "install": "brew install mozjpeg", + "why": "lossless EXIF-orientation rotation before recompression", + "required_by": [ + "www/scripts/add-photos.sh", + "www/scripts/reencode-thumbnails.sh" + ] + }, + { + "bin": "cjpeg", + "install": "brew install mozjpeg", + "why": "the chroma-subsampling row of the /garage/encoding grids", + "required_by": ["www/scripts/gen-encoding-grids.sh"] + }, + { + "bin": "avifenc", + "install": "brew install libavif", + "optional": true, + "why": "primary 10-bit AVIF tier; add-photos and reencode-thumbnails fall back to sips, the grid scripts do not", + "required_by": [ + "www/scripts/gen-encoding-grids.sh", + "www/scripts/gen-encoding-samples.sh" + ] + }, + { + "bin": "cwebp", + "install": "brew install webp", + "why": "the WebP point on the /garage/encoding format grid", + "required_by": [ + "www/scripts/gen-encoding-grids.sh", + "www/scripts/gen-encoding-samples.sh" + ] + }, + { + "bin": "ffmpeg", + "install": "brew install ffmpeg", + "why": "PNG to PPM, because cjpeg will not read PNG and sips BMP confuses it", + "required_by": ["www/scripts/gen-encoding-grids.sh"] + }, + { + "bin": "ssimulacra2", + "install": "build libjxl with -DJPEGXL_ENABLE_TOOLS=ON; brew's jpeg-xl does not ship it", + "why": "the perceptual metric the Instagram export searches quality against", + "required_by": ["www/scripts/export-for-instagram.sh"] + }, + { + "bin": "butteraugli_main", + "install": "build libjxl with -DJPEGXL_ENABLE_TOOLS=ON; brew's jpeg-xl does not ship it", + "why": "the second metric bounding that same search", + "required_by": ["www/scripts/export-for-instagram.sh"] + }, + { + "bin": "python3", + "install": "brew install python", + "why": "alt text in add-photos.sh, plus build-recipes.py and gen-pixel-peeper.py; only the last of those wants Pillow, from www/scripts/requirements.txt", + "required_by": ["www/scripts/add-photos.sh"] + } + ] +} diff --git a/docs/MAINTENANCE.md b/docs/MAINTENANCE.md index a00ad3f44..761a052fc 100644 --- a/docs/MAINTENANCE.md +++ b/docs/MAINTENANCE.md @@ -1115,15 +1115,34 @@ workflow that hand-tracked the from-source jpegli commit. Only Homebrew formulas ## Local fallback setup ```bash -brew install exiftool jq mozjpeg libavif # mozjpeg = jpegtran; libavif = avifenc (optional, sips falls back) +# the photo pipeline +brew install exiftool jq mozjpeg libavif # mozjpeg = jpegtran + cjpeg; libavif = avifenc (optional, sips falls back) python3 -m pip install -r www/scripts/requirements.txt # Pillow, for gen-pixel-peeper.py only # the JPEG encoder (zenc) builds itself on first pipeline run; needs rust (rustup.rs) cargo build --release --manifest-path www/scripts/zenc/Cargo.toml wrangler login # Cloudflare auth (deploys + KV + R2 all use it) + +# the study pages, which are NOT needed to add a photo +brew install webp ffmpeg # cwebp for the encoding grids; ffmpeg for their PNG -> PPM step ``` This is an emergency fallback only. sips is macOS-native (no install), and the normal path is the remote workflow above. +`export-for-instagram.sh` additionally wants **ssimulacra2** and +**butteraugli_main**, the two perceptual metrics it searches quality against. +Those are libjxl tools built with `-DJPEGXL_ENABLE_TOOLS=ON`, and Homebrew's +`jpeg-xl` formula does not ship them, so there is no brew line for this row. The +script falls back to `/opt/zerobrew/prefix/bin`, which is where this workstation's +source build put them. + +**`pnpm run tools:check` is the check on all of it**, declared in +[`config/tools.json`](../config/tools.json). Its declaration tier runs on every +PR and needs no binary; its presence tier probes this machine and is advisory in +CI. Run it before a pipeline session on a fresh machine and it names what is +missing and how to get it, rather than letting a script exit on a raw shell +error four steps in. Four of the tools above were required and documented nowhere +until it was written. + --- ## Add photos (local fallback only) diff --git a/package.json b/package.json index b6288a325..3c26cd6eb 100644 --- a/package.json +++ b/package.json @@ -28,6 +28,7 @@ "infra:check": "node scripts/check-infra.mjs", "infra:apply": "node scripts/apply-infra.mjs", "pages:check": "node scripts/check-page-contracts.mjs", + "tools:check": "node scripts/check-tools.mjs", "test": "node --test scripts/contract-tests.mjs", "lint": "oxlint --type-aware", "typecheck": "tsc -p config/tsconfig.json && tsc -p config/tsconfig.browser.json", diff --git a/scripts/check-tools.mjs b/scripts/check-tools.mjs new file mode 100644 index 000000000..f9cfb7722 --- /dev/null +++ b/scripts/check-tools.mjs @@ -0,0 +1,240 @@ +// The external binaries the photo and encoding pipelines shell out to. +// +// Two tiers, split on what the environment can reach, the same way infra:check +// splits on what its credential can reach: +// +// 1. DECLARATION, from source text alone. No binary has to exist, so this runs +// on every PR and on a fresh Linux runner. It is the tier that catches the +// bug this script was written for: a script acquiring a prerequisite that +// no documentation mentions. +// 2. PRESENCE, by probing the machine. Only a workstation can pass this, so it +// degrades to a report in CI rather than a failure. +// +// The scanners in tier 1 read shell source, which is exactly the fragile thing +// CLAUDE.md keeps warning about, so two rules apply to every one of them. They +// match a BOUNDED set of shapes rather than trying to understand shell. And each +// one carries a floor: a scanner that suddenly matches nothing reports a pass +// while checking nothing, which is how the Markdown-twin test went green for +// months on the wrong field names. +import { readFile, readdir, access } from "node:fs/promises"; +import { constants as fsConstants } from "node:fs"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; + +const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); +const argv = process.argv.slice(2); +const STRICT = argv.includes("--strict"); +const IN_CI = Boolean(process.env.CI) && !STRICT; + +// Floors. Each is comfortably under today's count and above zero, so ordinary +// edits never trip them and a scanner that stops matching always does. +const FLOOR_GUARD_LISTS = 4; +const FLOOR_BREW_HINTS = 4; + +const errors = []; + +const declaration = JSON.parse(await readFile(path.join(ROOT, "config/tools.json"), "utf8")); +const tools = declaration.tools ?? []; +if (tools.length === 0) { + console.error("config/tools.json declares no tools"); + process.exit(1); +} + +const byBin = new Map(tools.map((t) => [t.bin, t])); +const declaredBins = new Set(byBin.keys()); +const declaredInstalls = new Set(tools.map((t) => t.install).filter(Boolean)); + +// wrangler is guarded like a system binary and is not one: it comes from the +// workspace install, is pinned exactly at the root, and check-wrangler.mjs +// already asserts that. Declaring it here would put one version pin in two +// files, which is the drift this repo keeps writing gotchas about. +const NOT_SYSTEM = new Set(["wrangler"]); + +// ── the shell corpus ───────────────────────────────────────────────────────── +async function shellScripts() { + const dirs = ["www/scripts", "scripts"]; + const found = []; + for (const dir of dirs) { + let entries = []; + try { + entries = await readdir(path.join(ROOT, dir)); + } catch { + continue; + } + for (const entry of entries) { + if (entry.endsWith(".sh")) found.push(`${dir}/${entry}`); + } + } + return found.sort(); +} + +const scripts = await shellScripts(); +const source = new Map(); +for (const rel of scripts) source.set(rel, await readFile(path.join(ROOT, rel), "utf8")); + +// ── tier 1a: every declared file exists and names its tool ─────────────────── +// +// Catches a declaration that has gone stale, which is the failure mode a list +// like this acquires once the scripts move on without it. +for (const tool of tools) { + for (const rel of tool.required_by ?? []) { + let text = source.get(rel); + if (text === undefined) { + try { + text = await readFile(path.join(ROOT, rel), "utf8"); + } catch { + errors.push(`config/tools.json: ${tool.bin} claims ${rel}, which does not exist`); + continue; + } + } + if (!text.includes(tool.bin)) { + errors.push(`config/tools.json: ${tool.bin} claims ${rel}, which never mentions it`); + } + } +} + +// ── tier 1b: every `for cmd in …` guard list is declared ───────────────────── +// +// The shape is `for in ; do … command -v "$" …`, which is how +// most of these scripts check their preconditions. A grep for the binary NAME +// cannot see these, because the name only exists as a loop word. That blind spot +// is gotcha 29's, in a different costume. +let guardLists = 0; +const guardPattern = /^[ \t]*for[ \t]+(\w+)[ \t]+in[ \t]+([^;\n]+);?[ \t]*(?:do)?[ \t]*$/gm; +for (const [rel, text] of source) { + for (const match of text.matchAll(guardPattern)) { + const [, variable, wordsRaw] = match; + // Only a loop whose body probes the loop variable is a precondition guard. + const after = text.slice(match.index, match.index + 400); + if (!after.includes(`command -v "$${variable}"`) && !after.includes(`-x "$${variable}"`)) continue; + guardLists += 1; + for (const word of wordsRaw.trim().split(/\s+/)) { + // Words that are themselves variables hold a path resolved earlier; the + // binary they point at is declared under its own name instead. + if (word.includes("$")) continue; + const bin = word.replace(/^["']|["']$/g, ""); + if (!bin || declaredBins.has(bin) || NOT_SYSTEM.has(bin)) continue; + errors.push(`${rel}: guards on \`${bin}\`, which config/tools.json does not declare`); + } + } +} +if (guardLists < FLOOR_GUARD_LISTS) { + errors.push( + `guard-list scanner matched ${guardLists} loops, below the floor of ${FLOOR_GUARD_LISTS}. ` + + `The shell changed shape and this scanner is now checking nothing.`, + ); +} + +// ── tier 1c: every `command -v ` is declared ──────────────────────── +let literalProbes = 0; +for (const [rel, text] of source) { + for (const match of text.matchAll(/command -v[ \t]+"?([A-Za-z][\w.-]*)"?/g)) { + const bin = match[1]; + if (bin.startsWith("$")) continue; + literalProbes += 1; + if (declaredBins.has(bin) || NOT_SYSTEM.has(bin)) continue; + errors.push(`${rel}: probes \`${bin}\`, which config/tools.json does not declare`); + } +} + +// ── tier 1d: every `brew install …` hint maps to a declaration ─────────────── +// +// A formula is not a binary (mozjpeg gives jpegtran and cjpeg, libavif gives +// avifenc, webp gives cwebp), so this matches on the install STRING rather than +// trying to translate. A new hint that nothing declares is a new prerequisite. +let brewHints = 0; +for (const [rel, text] of source) { + for (const match of text.matchAll(/brew install ([a-z0-9][a-z0-9 -]*)/g)) { + const hint = `brew install ${match[1].trim()}`; + brewHints += 1; + if (declaredInstalls.has(hint)) continue; + // A multi-formula hint is satisfied when every formula in it is declared + // somewhere, since the docs write them as one line. + const formulae = match[1].trim().split(/\s+/); + const covered = formulae.every((f) => [...declaredInstalls].some((i) => i.split(/\s+/).includes(f))); + if (!covered) { + errors.push(`${rel}: suggests \`${hint}\`, which config/tools.json does not account for`); + } + } +} +if (brewHints < FLOOR_BREW_HINTS) { + errors.push( + `brew-hint scanner matched ${brewHints} hints, below the floor of ${FLOOR_BREW_HINTS}. ` + + `The install hints moved and this scanner is now checking nothing.`, + ); +} + +// ── tier 1e: the docs name every tool ──────────────────────────────────────── +// +// The whole reason this file exists is that a prerequisite can be real and +// undocumented. Asserting the docs mention it is what keeps that from recurring. +const DOCS = ["CLAUDE.md", "docs/MAINTENANCE.md"]; +const docText = (await Promise.all(DOCS.map((d) => readFile(path.join(ROOT, d), "utf8")))).join("\n"); +for (const tool of tools) { + if (!docText.includes(tool.bin)) { + errors.push(`neither ${DOCS.join(" nor ")} mentions \`${tool.bin}\`; an undocumented prerequisite is the bug this check exists for`); + } +} + +// ── tier 2: presence ───────────────────────────────────────────────────────── +// Walk PATH directly rather than shelling out to `command -v`. Passing an +// argument array with `shell: true` concatenates instead of escaping, which +// node 26 deprecates (DEP0190), and a lookup needs no shell anyway. +const PATH_DIRS = (process.env.PATH ?? "").split(path.delimiter).filter(Boolean); + +async function present(tool) { + const candidates = tool.path ? [tool.path] : PATH_DIRS.map((dir) => path.join(dir, tool.bin)); + for (const candidate of candidates) { + try { + await access(candidate, fsConstants.X_OK); + return candidate; + } catch { + // next + } + } + return null; +} + +const missing = []; +const found = []; +for (const tool of tools) { + const where = await present(tool); + if (where) found.push([tool.bin, where]); + else missing.push(tool); +} + +// ── report ─────────────────────────────────────────────────────────────────── +console.log(`tools: ${tools.length} declared across ${scripts.length} shell scripts`); +console.log(`declaration: ${guardLists} guard loops, ${literalProbes} literal probes, ${brewHints} brew hints scanned`); + +if (errors.length) { + console.error("\ndeclaration check FAILED:"); + for (const e of errors) console.error(` - ${e}`); + process.exit(1); +} +console.log("declaration: every guarded binary is declared, and every declared binary is documented"); + +if (missing.length === 0) { + console.log(`presence: all ${found.length} present`); + process.exit(0); +} + +const blocking = missing.filter((t) => !t.optional); +const lines = missing.map((t) => ` - ${t.bin}${t.optional ? " (optional)" : ""} — ${t.install}\n ${t.why}`); + +if (IN_CI) { + // A hosted runner has none of these and is not supposed to. Reporting keeps + // the tier visible in the log; failing would make every PR red for a machine + // that never runs the photo pipeline. + console.log(`\npresence: ${missing.length} not on this machine (advisory in CI)`); + for (const line of lines) console.log(line); + process.exit(0); +} + +console.log(`\npresence: ${found.length} present, ${missing.length} missing`); +for (const line of lines) console.log(line); +if (blocking.length) { + console.error(`\n${blocking.length} required tool(s) missing. The pipelines that need them will exit.`); + process.exit(1); +} +process.exit(0);