From 9bd7297757300d00d45971bd79791ae1f56d36e9 Mon Sep 17 00:00:00 2001 From: Sylpheed port agent Date: Sat, 29 Aug 2026 14:10:16 +0000 Subject: [PATCH] port: the P1 regression harness could not have run since the monorepo merge verify-screen resolves its reference binary to a path build-reference-cli stopped being able to produce: that script greps Cargo.toml for a `Syplheed-Reborn.git", rev = "..."` pin, and 9fbb352 replaced it with a path dependency. The grep returns nothing, the script exits 1, and the binary left behind predates the merge -- three hours older than the sources, from a revision nothing points at. The consumer only checked that the file EXISTED. DECISIONS already carries "the reference renderer was stale for three diff runs" from P2. This would have been the fourth, by a different mechanism: not a forgotten rebuild but a build step that could no longer succeed. The fix is a deletion. Revision-keying solved a two-repo problem -- /reborn's target/ was a live mount that moved mid-run -- and the monorepo removed that problem by construction. verify-screen now builds sylpheed-cli from the workspace, which is the same decoder the exporter and the port read. BASELINE, all 16 screens, after four milestones of renderer change: no new drift. The only substantial disagreement is title_jp (max 155, 20498 px), which is the same one P1 recorded and left open -- ptlogo_eff2 at 125 % scale, where blit samples the destination pixel's corner and a GPU samples its centre. The port still does not match it, because matching means reproducing a half-pixel bias on purpose to make a number smaller. `max` alone could not tell 2 pixels from 25 000: main_menu and title_jp produced the same DIFFERS verdict. The script now also reports how many pixels are over the bar. The bar is NOT raised -- tuning a threshold until things match is the failure its own header warns about. Stated plainly because sixteen OK rows look reassuring: the harness renders --pose=rest, so NONE of the last four milestones' visible work is under test -- not the spinning ring, not the plate composite, not any timeline behaviour, not audio. REFUTATION: the RE agent's answer naming "builds 0/1 and 10/11" as the loading screen is false in the index space this export uses -- entries 10 and 11 are palogo_sqex and palogo_gamearts, the splashes, which the same agent identified as such answering my ask 1. Over the twelve is_build bundles, ordinals 10/11 ARE entries 12/15, so the finding is right and the enumeration is not mine. authored/screen_names.json is keyed by ENTRY, and someone writing keys "10" and "11" from that message would name the publisher wordmark and the developer logos as loading screens, and everything would still validate and run. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WM5XL4HfrHuxz8RiMWdCMC --- docs/port/DECISIONS.md | 127 +++++++++++++++++++++++++++++++++++++++ tools/port/verify-screen | 48 +++++++++++++-- 2 files changed, 169 insertions(+), 6 deletions(-) diff --git a/docs/port/DECISIONS.md b/docs/port/DECISIONS.md index 56bd351b..58e38e3c 100644 --- a/docs/port/DECISIONS.md +++ b/docs/port/DECISIONS.md @@ -2238,3 +2238,130 @@ and taking it here would be inconsistent, and it would trade an inaudible **A peak reading is not a clipping measurement.** One sample at 0 dBFS and two seconds of square wave give the same number, and I reported the first as though it were the second — twice, in red, in two milestones' write-ups. + +## The P1 regression harness had been broken since the monorepo merge, 2026-08-29 + +`tools/port/verify-screen` is the P1 gate's regression detector: Godot's drawing +of a screen against `sylpheed-cli screen render` of the same build. It had not +been run since P1, across four milestones that changed the renderer — rotation, +the focus record, the spinning ring, two builds composited at once. + +It could not have been run. **It resolves its reference binary to a path that +`build-reference-cli` stopped being able to produce.** That script greps +`crates/sylpheed-export/Cargo.toml` for + +``` +sylpheed-formats = { git = "…Syplheed-Reborn.git", rev = "…" } +``` + +and the monorepo merge (`65cefa7`) replaced that line with +`{ path = "../sylpheed-formats" }`. The grep returns nothing, the script exits 1, +and the binary left at `reference-cli/sylpheed-cli` is whatever predated the +merge — here, **three hours older than the sources** and built from a revision +nothing in the tree points at any more. + +Running the diff against it would have compared the port to a decoder from +another era and called the result a regression check. `DECISIONS.md` already +carries *"The reference renderer was stale for three diff runs"* from P2. This +would have been the fourth, and the mechanism was different: not a forgotten +rebuild, but a **build step that could no longer succeed and a consumer that +only checked whether the file existed**. + +### The fix is a deletion, not a repair + +The revision-keying solved a two-repo problem: `/reborn`'s `target/` was a live +mount of the other agent's checkout and moved mid-run, so a pixel disagreement +against it had a free variable in it. **The monorepo removed that problem by +construction** — the exporter, the reference and the port now read one decoder, +the working tree's. So `verify-screen` builds `sylpheed-cli` from the workspace. +`SYLPHEED_CLI` still overrides for anyone who wants to pin one deliberately. + +### The baseline, all 16 screens + +``` +build_00/01 max 3 over3 0 OK +press_start(_jp) max 1 over3 0 OK +title max 6 over3 790 DIFFERS +main_menu(_jp) max 4 over3 0 DIFFERS +extras(_jp) max 3 over3 0 OK +publisher_logo(_r) max 1-2 over3 0 OK +developer_logos(_r) max 2 over3 0 OK +title_jp max 155 over3 20498 DIFFERS +build_12/15 max 0 over3 0 OK +``` + +**No new drift.** Four milestones of renderer change and the only screen with a +substantial disagreement is `title_jp` — which is the *same* one P1 recorded and +left open: `ptlogo_eff2` is the single drawn element in the whole export at a +scale that is not a whole multiple of 100 % (125 %), and the two renderers pick +different source texels there. `ui_layout::blit` samples at the destination +pixel's top-left corner, a GPU at its centre. **The port has still not changed to +match**, because matching would mean reproducing a half-pixel bias on purpose to +make a number smaller. Only an oracle capture settles it. + +`title`'s 790 pixels at ≤ 6/255 are the same class, one texel wide, on the logo's +scaled edges. `main_menu` and `main_menu_jp` say DIFFERS on a max of 4 with +**zero** pixels over the bar — a couple of pixels differing in a single channel. + +### `max` alone could not tell 2 pixels from 25 000 + +The script reported only the largest difference anywhere in the frame, so +`main_menu` (two pixels) and `title_jp` (2.8 % of the frame) produced the same +verdict. It now also reports how many pixels are over the bar. + +**The bar itself is not raised.** Tuning a threshold until things match is the +failure the script's own header warns about; adding a second number is +information, not a loosened bound. ⚠️ The count is thresholded on **greyscale +luma** while `max` is a per-channel maximum, so they are not two views of one +measurement — a per-channel check counts 957 on `title` where the luma count +says 790. + +### What this harness cannot see, stated because the OK rows look reassuring + +It renders `--pose=rest`. That is deliberate — it holds both renderers to the +same declared pose so the test is *port vs reference* and not *rest vs timeline* +— but it means **none of this iteration's or the last four's visible work is +under test**: not the spinning focus ring, not the plate composited over the +title, not any timeline behaviour, not audio. Sixteen OK rows are a statement +about the resting composite and nothing else. + +And it remains what its header says: a consistency check between two renderers +that share their assumptions. Both have been wrong together three times — +`pteff05`, scale-0, `rest()` — and each time only a capture caught it. + +## Refutation — "builds 0/1 and 10/11 are the loading screen" is false in the index space this export uses + +**The claim**, from the RE agent 2026-08-29, answering the port's ask to name +`GP_TITLE`'s unnamed bundles: *"builds 0/1 and 10/11 are the loading screen, +decoded from their own `pgloading_*` element names."* + +**In this export, entries 10 and 11 are the splash screens**, and it is not close: + +| entry | elements | +|---|---| +| 10 | `palogo_eff0`, **`palogo_sqex`**, `palogo_sqex_eff` | +| 11 | `palogo_eff0`, **`palogo_gamearts`**, `palogo_seta`, `palogo_anima` … | +| 12 / 15 | `pgloading_eff00`, `pgloading_loop1`, `pgloading_str` … | + +Entry 10 is the **SQUARE ENIX** wordmark and 11 the developer logos — which the +same agent identified, in the answer to the port's ask 1, as *"entries 10/13 are +the SQUARE ENIX publisher wordmark, the first thing the boot shows"*. + +**Verdict: the finding is almost certainly right and the index space is wrong.** +Over the twelve bundles `is_build` accepts — entries 0,1,2,3,4,5,6,7,8,9,12,15 — +ordinals 10 and 11 are entries **12 and 15**, which are exactly the two dressed +loading variants. So "0/1 and 10/11" is the `is_build` ordinal, and this export +addresses by **pak entry index**. + +**Why this is worth a section rather than a shrug.** `authored/screen_names.json` +is keyed by entry index, and the exporter's own comment says why: *"keyed by +ENTRY, not by the enumeration ordinal — widening the enumeration to reach the +splash renumbers the ordinals, and a name that moves when the rule changes is not +a name."* Someone reading that message and writing keys `"10"` and `"11"` would +**name the publisher wordmark and the developer logos as loading screens**, and +the export would validate, and the boot would still run. + +Two enumerations of the same archive differ by exactly the four bundles the port +had to add an allow-list to reach. That is the sharpest possible demonstration of +why the exporter switched, and it has now nearly caused the error it switched to +prevent. Reported; the names are still the RE agent's to give. diff --git a/tools/port/verify-screen b/tools/port/verify-screen index f1ef2ada..03be9d38 100755 --- a/tools/port/verify-screen +++ b/tools/port/verify-screen @@ -53,15 +53,39 @@ set -euo pipefail cd "${PROJECT_DIR:-/work}" -# `reference-cli/`, not `release/`: the reference binary is built per pinned -# revision so a pin change cannot silently reuse the previous revision's build. -# See docker/bin/build-reference-cli. -CLI="${SYLPHEED_CLI:-${CARGO_TARGET_DIR:-/sylph-home/port/target-container}/reference-cli/sylpheed-cli}" +# THE REFERENCE IS THE WORKSPACE'S OWN `sylpheed-cli`, and that is a change. +# +# It used to be a binary built per PINNED REVISION into `reference-cli//`, +# because `sylpheed-formats` was a git dependency and /reborn's target/ was a +# live mount of the other agent's checkout that moved mid-run. A pixel +# disagreement against a moving decoder has a free variable in it. +# +# The monorepo merge (`65cefa7`) removed that problem by construction: +# `crates/sylpheed-export/Cargo.toml` now says +# `sylpheed-formats = { path = "../sylpheed-formats" }`, so the exporter, this +# reference and the port all read ONE decoder -- the working tree's. +# +# 🔴 It also silently broke the old machinery, and this script did not notice. +# `build-reference-cli` greps Cargo.toml for `Syplheed-Reborn.git", rev = "..."`; +# that line no longer exists, so the script exits 1 and the binary at +# `reference-cli/sylpheed-cli` is whatever was last built before the merge -- +# here, three hours older than the sources and from a revision nothing points +# at any more. Running the diff against it would have compared the port to a +# decoder from another era and called the result a regression check. This +# corpus has already been bitten by a stale reference renderer three times. +# +# So: build it from the workspace. `SYLPHEED_CLI` still overrides, for anyone +# who does want to pin one deliberately. +CLI="${SYLPHEED_CLI:-}" +if [ -z "$CLI" ]; then + CLI="${CARGO_TARGET_DIR:-/sylph-home/port/target-container}/release/sylpheed-cli" + cargo build --release -p sylpheed-cli >/dev/null 2>&1 || true +fi DISC="${SYLPHEED_DISC:-/disc}" OUT="${OUT:-${TMPDIR:-/tmp}/verify-screen}" export DISPLAY="${DISPLAY:-:97}" -[ -x "$CLI" ] || { echo "no reference CLI at $CLI -- run build-reference-cli" >&2; exit 2; } +[ -x "$CLI" ] || { echo "no reference CLI at $CLI -- \`cargo build --release -p sylpheed-cli\` failed?" >&2; exit 2; } [ -f export/manifest.json ] || { echo "no export/manifest.json -- run build-export --run" >&2; exit 2; } mkdir -p "$OUT" @@ -98,12 +122,24 @@ print(json.load(open("export/"+f))["source"]["build"])' "$name") read -r max mean <<<"$(convert "$OUT/$name.godot.png" "$OUT/$name.ref.png" \ -compose difference -composite -format "%[fx:maxima*255] %[fx:mean*255]" info:)" + # HOW MANY pixels are over the bar, not just how far the worst one is. A + # single `max` cannot tell 2 pixels from 25 444, and this run produced both: + # `main_menu` trips the threshold on TWO pixels out of 921 600 while + # `title_jp` trips it on 2.8 % of the frame. Reporting only the max made those + # the same verdict, which is how a real disagreement hides behind a rounding + # one. The bar itself is NOT raised -- tuning a threshold until things match + # is the failure this script's own header warns about. + over=$(convert "$OUT/$name.godot.png" "$OUT/$name.ref.png" \ + -compose difference -composite -colorspace Gray -threshold $((3*65535/255)) \ + -format "%[fx:int(mean*w*h)]" info:) + # 3/255 is what integer-truncating compositing in the CLI and float rounding # in a GPU differ by. Anything above that is a placement, order or colour # disagreement and needs a reason, not a threshold. verdict=OK awk "BEGIN{exit !($max > 3)}" && { verdict=DIFFERS; status=1; } - printf '%-16s build %-3s max %-5s mean %-8s %s\n' "$name" "$build" "$max" "${mean:0:6}" "$verdict" + printf '%-17s build %-3s max %-5s mean %-8s over3 %-7s %s\n' \ + "$name" "$build" "$max" "${mean:0:6}" "$over" "$verdict" done echo "artifacts in $OUT" exit $status