ci: fail when a test declares screens it never captures - #487
Merged
Conversation
The screenshot gate was `total PNG count > 0`. On the rc30 artifact that gate passed with 345 PNGs while EVERY suite 7.14.2 changed captured zero: the rendering evidence for a release whose whole subject is what reaches the screen did not exist, and nothing in CI said so. msg_ethereum_erc20_0x_signtx ABSENT #444/#468/#472 msg_display_disclosure ABSENT the byte-exact pager msg_binance_sign_tx ABSENT #430 test_verify_typed_data ABSENT EIP-712 The 33 suites that ARE captured are the stable ones nobody touched. This is the same silent-absence class as the Stage-1 skip that ci-gate closed in #474 and the deadlock that #477 made legible: the check reported on the wrong thing, so its green meant less than it appeared to. Adds a per-test audit after the count gate. Any SECTIONS entry that DECLARED screens must have captured some; skipped tests are excluded because a version-gated test cannot draw. Run against the rc30 artifact it names exactly the eight tests that declared screens and captured none. Repins python-keepkey to 1a6e785, which carries the SECTIONS entries, the --screenshot-audit implementation, and the fix for test_msg_display_disclosure answering ButtonRequests through its own callback -- which bypassed the capture hook, so the one suite written to police what the screen shows was the one suite whose screens nobody could look at. Also lands the rc30 hardware evidence (T1-T12), including the retraction of the #484 false finding and the T8 leg C result: AdvancedMode PERSISTS across a power cycle, contradicting what efa18ad and b53b63b both assert and build their risk analysis on.
The emulator test script detected the firmware version with:
FW_VERSION=$(sed -n '/^project/,/)/p' CMakeLists.txt \
| grep -oP '\d+\.\d+\.\d+' || echo "7.14.0")
`grep -oP` is a GNU extension. This container's grep is BusyBox, which has no
-P, so that command ALWAYS failed and the fallback silently supplied 7.14.0 --
on the 7.14.2 release branch. CI has been reporting
"Detected FW_VERSION=7.14.0 from CMakeLists.txt" while CMakeLists says 7.14.2.
Everything downstream keys off that number. SECTIONS entries are selected by
ver_ge(fw_version, min_fw), so at 7.14.0 every test gated to 7.14.1 or later was
excluded from BOTH the screenshot filter and validate_junit. That is the root
cause of the gap #487 set out to fix: the suites this release changed captured
no screens because the report system did not believe those tests existed yet.
It also means the per-test screenshot audit added in #487 passed vacuously --
with no 7.14.2 sections active, nothing declared screens, so nothing could be
missing. The audit was correct; it was being asked about the wrong release.
Two changes:
- read VERSION with sed alone, portable to BusyBox
- FAIL if it cannot be read, rather than defaulting. A wrong version here is
invisible and silently narrows what CI checks -- exactly the failure mode
ci-gate (#474) and the screenshot audit were added to prevent.
Same class as those two: the check ran, went green, and was measuring something
other than what its name implied.
This was referenced Aug 20, 2026
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
1 participant
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.
The gap
CI captures 345 OLED PNGs and the screenshot phase reports healthy. On the rc30
artifact, every suite 7.14.2 changed captured zero of them:
msg_ethereum_erc20_0x_signtxmsg_display_disclosuremsg_binance_sign_txtest_verify_typed_dataThe 33 suites that are captured —
msg_signtx,msg_getaddress_segwit,msg_changepin— are the stable ones nobody touched.So the rendering evidence for a release whose entire subject is what reaches the
screen did not exist, and nothing said so.
Three causes, all silent
SECTIONSdeclared no screenshot expectations for the 0x, Binance andtyped-data suites, so
screenshot_filter()never selected them.test_msg_display_disclosureWAS selected and still produced nothing. Itanswers ButtonRequests through its own recording callback in order to read the
layout, which bypasses the client's capture hook. The one suite written
specifically to police what the screen shows was the one suite whose screens
nobody could look at.
The gate is
total PNG count > 0. A single captured suite satisfies it.It cannot distinguish "captured everything" from "captured something".
That third one is the same class as the Stage-1 skip
ci-gateclosed in #474 andthe deadlock #477 made legible: the check reported on the wrong thing, so its
green meant less than it appeared to.
The change
Adds
--screenshot-auditand runs it after the count gate. AnySECTIONSentrythat declared screens must have captured some. Skipped tests are excluded —
a version-gated test cannot draw.
Against the rc30 artifact it names exactly the eight tests that declared screens
and captured none.
Two entries deliberately carry an empty screenshot list, and that is the
assertion rather than an omission:
test_eip1559_requires_chain_id— the refusal happens before the firstconfirm(), so no screen is drawn and none should be expected.test_contract_handler_streamed_calldata_signs_full_data— every test intest_msg_ethereum_signing_guardscurrently SKIPs underrequires_firmware.Declaring a screen nothing can satisfy would turn the new audit into noise.
Repin
python-keepkey →
1a6e785, pushed to the existing open PR for this release(keepkey/python-keepkey#214) per the dress-rehearsal SOP rather than opening a
second one.
Also lands the rc30 hardware evidence
docs/release/rc30-evidence/, T1–T12. Two things worth reading:efa18ad77and
b53b63b0astate it is session state and build their risk analysis onthat. The blast radius is "until revoked", not "until unplugged". Release-note
correction.
confirmed with no press"; the tester was pressing. The refuting control — a
single untouched screen — took one minute and belonged before the issue was
filed.
Honest note on the round: most of the wire-level checks duplicated what the
emulator already asserts in CI. The screens were the part only hardware could
show, and they are exactly the part no automation was looking at. This PR moves
that evidence into CI so no future release depends on someone photographing an
OLED.
Closes #467. The FW_VERSION fix here implements that issue rather than
discovering it — #467 already documented the BusyBox 1.28.4 / Alpine 3.8 root
cause. What is new in this PR is the per-test screenshot audit and the SECTIONS
entries; the version fix is the prerequisite that makes them non-vacuous.
Related: #469 (PRs targeting release/** get no pull_request CI) is the reason
this PR's checks only run via the fix/** push trigger. Not fixed here.