fix(rigs): device-state prep — configure, do not refuse; and do not degrade toward the risky write - #225
Open
JC-000 wants to merge 4 commits into
Open
fix(rigs): device-state prep — configure, do not refuse; and do not degrade toward the risky write#225JC-000 wants to merge 4 commits into
JC-000 wants to merge 4 commits into
Conversation
…ing toward the risky write (#197, #187, #212) Three defects of one shape. Device config on an Ultimate is runtime-only: a re-flash restores factory defaults and we never write flash, so `RAM Expansion Unit: Disabled` and a 1 MHz clock are the DEFAULT state, not another lane's leftovers. The working pattern is that a run restores baseline and then sets what it needs. #197 -- four of the five `preflight_reu` callers had nothing for it to back up. `bench_ecdsa_u64e.py`, `rig_https_local.py`, `rig_https_bad_finished.py` and `rig_https_live.py` read the REU state and exited 4 on a device that was simply at its default (observed on a U64E at fw 3.15 while validating #181). Only `rig_https_wiki.py` configured it. The preflight is the backstop for a rig that forgot; it is not a substitute for setup. #187 -- all four HTTPS rigs probed `CPU Speed` / `Turbo Control` in order to SKIP a redundant write, because the write itself glitches the UCI bridge and loses the next pushed command ($88). On a failed probe they printed "writing anyway" -- performing the exact action the probe exists to avoid, with an intermittent $88 as the symptom on a project with a documented history of blaming that on firmware. #212 (second half) -- no run recorded the device state it ran against. New `tools/uci/_device_prep.py::prepare_device()`, called under the DeviceLock after `enable_uci` and BEFORE `preflight_reu` at all five sites. It configures the REU the linked profile needs, sets turbo before the reset, and prints the before- and after-state (also written to `$UCI_DEBUG_DIR/device_state.json`). The failure policy is per item, not per module, because both degrades are wrong somewhere: * REU probe unreadable -> WRITE ANYWAY. The write is the configuration the run needs; a wasted one costs a PUT. This is `ensure_reu_16mb`'s shape, generalised rather than deleted -- #197 names it as the correct asymmetry. * Turbo probe unreadable -> ABORT, after one retry. Writing risks the $88 the probe was added to avoid; skipping runs the rig at the previous lane's clock, which invalidates every wall-clock number and is how a 1 MHz comb boot got read as a code defect. Two unsafe degrades means stopping is the honest answer, with `C64_FORCE_TURBO_WRITE=1` as the named way back. `preflight_reu` is unchanged in behaviour and keeps its #179 fail-closed guarantee; only its superseded "it never enables the REU for you" policy prose is corrected to say which half now does. Every device read still goes through its `_read_config_value`, so the harness-shape tolerance has one home to audit, not two. Harness APIs audited against the sibling editable tree (c64-test-harness @ 1219fe1): `get_config_value` (raises rather than returning None -- the ambiguity #187 describes disappears at the read), `get_config_item`, `get_config_category` (unchanged, still bare values per item), `set_reu` (probes the Cartridge preset; C64U-safe), `set_turbo_mhz`, `cpu_speed_enum`, and `apply_factory_baseline` / `BASELINE_NEVER_TOUCH` -- neither REU store is in the five never-touch stores, and this module resets nothing regardless: it PUTs exactly the four items it names. Red-green, hardware-free, `tools/test_device_prep.py` (21 cases, faked client, on `test_reu_preflight.py`'s model): * before, against the unmodified rigs: 18 passed, 3 failed -- `test_every_crypto_rig_prepares_before_it_checks`, `test_no_rig_still_degrades_toward_the_turbo_write` (4 rigs) and `test_rigs_do_not_hand_roll_the_turbo_probe` (4 rigs). * after: 21 passed, 0 failed. Both halves of the asymmetry are mutation-checked, because a uniform policy in either direction passes half the suite: * MUTANT A (reinstate the "writing anyway" turbo degrade): 18 passed, 3 failed -- the two abort cases plus the None-is-not-a-mismatch case, each reporting `turbo writes: [48]`. * MUTANT B (extend the fail-closed to the REU): 19 passed, 2 failed -- `test_unreadable_reu_writes_anyway` and `test_unreadable_items_are_named_in_the_report`. `tools/test_reu_preflight.py` stays green (PASS). Bare `pytest` has the same 16 pre-existing failures as master (build-dependent and pin suites), and one fewer: `test_pytest_boundary` goes green with the new suite listed in `testpaths`. NOT covered: no hardware run. The U64E was actively held by a peer lane throughout, and queuing behind it would have blocked the session's other hardware lanes for a plumbing change that is fully covered hardware-free. The prep's own REST calls are therefore proven against a faked client only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CLAUDE.md's rig section said crypto-path rigs "call preflight_reu(); it never writes device config", which was the whole of the story and is now half of it. It names the prep, the call order (prep first, preflight as the backstop -- asserted at all five sites by `tools/test_device_prep.py`), the two opposite degrade directions and their overrides. `preflight_reu` itself still writes nothing, and that sentence survives unchanged. `tools/uci/README.md` gets the same in table form, plus `_device_prep.py` in the helper-module list. `docs/engineering-notes.md` closes the gap its #197 entry opened, and keeps the part worth carrying forward: the right degrade is per item, against a named cost, and a uniform policy in either direction is wrong in one of the two places -- with the mutation counts that show it. No memory-margin figure is touched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…d close four review findings Adversarial review of the previous commit blocked on two findings; this addresses both, plus four smaller ones. BLOCKER 1 -- the REU write was unwrapped, so `prepare_device` broke its own documented "never raises for the REU" contract in the worst possible direction. The vicious part is the correlation: the condition that makes the probe unreadable is frequently the same one that makes the write fail. With REST refusing, the harness's `_cartridge_preset_supported` probe is also inconclusive, `set_reu` therefore INCLUDES a `Cartridge: "REU"` PUT (ultimate64_helpers.py:483-485) which a C64 Ultimate rejects HTTP 400, and `set_config_items` catches nothing per item. All five call sites catch only `DevicePrepError`, so that escaped `main()`: master reported such a device as exit 4 with the writemem-wedge ladder, and this branch would have reported exit 1 and a raw traceback with `outcome=UNKNOWN` -- on the one path this project has a documented habit of misdiagnosing as firmware corruption. The write is now wrapped into a `DevicePrepError` carrying its own ladder (distinct from the preflight's "your device has no REU" text: nothing here says the REU is missing), and it aborts before the turbo write -- a device refusing config is not a device to keep writing to. The docstring is corrected too: `set_reu` can PUT a FIFTH item, so "this module PUTs exactly the four items it names" was wrong. BLOCKER 2 -- `device_state.json` was written on zero default runs. No call site passed `artifact_dir`, and every rig defaults `UCI_DEBUG_DIR` in Python, so the env fallback was normally unset; when set it resolved to the rigs' BASE dir, not the per-run timestamped dir, so it would be overwritten by the next run. `bench_ecdsa_u64e.py` has no `UCI_DEBUG_DIR` concept at all. All five now pass the run's own dir; `rig_https_local.py` and `bench_ecdsa_u64e.py` create one when DEBUG_CAPTURE left none, since the run that most needs the record is the one nobody enabled capture for. The test passed for the wrong reason (it passed `artifact_dir` explicitly while `_run` popped the env var), so both directions are now covered. 3 -- `C64_SKIP_DEVICE_PREP=1` is the dangerous hatch, not `C64_FORCE_TURBO_WRITE=1`. It skips the turbo write entirely, so the run inherits the previous lane's clock: the exact outcome the turbo policy argues is worth aborting over. Its banner said only that the preflight still fails closed -- true, and irrelevant to the clock. It now warns. 4 -- `PREP_CALL_SITES` was a hardcoded five-tuple claiming to be "every rig that runs a crypto-path PRG", which by construction could not find a sixth. Replaced by discovery: a module that calls `client.run_prg` AND knows about the comb profile. That selects the five plus `rig_https_banner.py` -- which boots the PRG through the menu on a 75 s budget while printing "comb boot precompute", writes neither turbo nor REU, and is therefore #212's documented failure verbatim (a comb boot at 1 MHz needs ~36 min; `tests/rig_ip65_rrnet_hw.py` writes turbo at stock 1 MHz, so "RR-Net run, then banner run" reaches it live). That file is owned by another lane and is NOT edited here: it is listed in `KNOWN_UNPREPPED` with the reason, and a second test fails if the rig is fixed and the entry is left behind. 5 -- the two skip flags parsed their values differently: `C64_SKIP_REU_PREFLIGHT=false` SKIPPED while `C64_SKIP_DEVICE_PREP=false` did not, with both documented on one line. Unified on one parser (`_reu_preflight.env_flag_enabled`) in the fail-closed direction, so `=false` now leaves every guard ON. 6 -- correcting the previous commit message rather than amending it: it claimed "one fewer failure: test_pytest_boundary goes green". Measured, the failure SET is identical on master and this branch and `test_pytest_boundary` passes on both. Master is 16 failed / 137 passed (reviewer-measured); this branch is 16 failed / 167 passed -- the delta is 30 new passing cases, not a fixed failure. Also: the retry delay comment no longer implies a measurement behind the 1.0 s (there is none), and the notes record the accepted side effect that four more rigs leave the device REU-Enabled after a REU-profile run, so a later on-chip run on the same device is no longer free evidence of REU-less operation. Red-green, hardware-free. New cases against the PREVIOUS COMMIT's sources (`git checkout 110e36f -- tools/uci/*.py`), 25 passed / 5 failed: ERROR test_a_refused_reu_write_does_not_reach_the_turbo_write: _HarnessError: HTTP 500 ERROR test_a_refused_reu_write_is_exit_4_with_a_ladder_not_a_traceback: _HarnessError: HTTP 400: 'REU' is not a valid choice for Cartridge FAIL test_every_prepping_rig_hands_over_its_run_artifact_dir: bench_ecdsa_u64e.py:471 calls prepare_device without artifact_dir ... FAIL test_skipping_prep_warns_that_the_clock_is_unmanaged: skipping prep must warn about the unmanaged clock ERROR test_the_two_skip_flags_parse_env_values_identically: AttributeError: module '_reu_preflight_parity' has no attribute 'env_flag_enabled' Note the first two arrive as raw `_HarnessError`, which IS the finding: they escape as uncaught exceptions rather than as `DevicePrepError`. After: 30 passed, 0 failed. The discovery rule has teeth -- emptying `KNOWN_UNPREPPED` gives 29 passed / 1 failed, naming `rig_https_banner.py`. Both asymmetry mutants still fail on the amended module (A: 27/3, B: 27/3; B now also fails the refused-write case, since a uniform fail-closed swallows that distinction). `tools/test_reu_preflight.py` PASS, `test_pytest_boundary` PASS, all five rigs import and compile. Hardware, from the independent review run (not mine), U64E fw 3.15 commit `4011c97c` / fpga 125, via `acquire_device_lock()`: device found in exactly #212's leftover state (`Manual` / `48` / REU `Disabled`); `_turbo_matches` -> True so the turbo write was correctly SKIPPED on real firmware; `set_reu(True, size="16 MB")` succeeded and read back with the `Cartridge` write correctly omitted on the 3.15 shape; a second `prepare_device` gave `wrote=[] skipped_write=['reu','turbo']`, i.e. idempotent with zero PUTs. State restored, lock released. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Prose only. No behaviour change; `tools/test_device_prep.py` stays 30/0, `test_reu_preflight.py` PASS, `test_pytest_boundary` PASS, all five rigs import. A (must fix) -- "so a sixth rig cannot slip past" was a false mechanism claim. The discovery rule is a text match (`client.run_prg(` plus a mention of the comb profile), so it finds a sixth rig written to the same SHAPE and misses one that is not: the reviewer built two rigs that boot the real PRG and hold `preflight_reu` with no prep -- #197's exact defect -- one naming its client `u64`, the other never saying "comb", and both pass the suite 30/0. `test_the_discovery_rule_finds_the_known_rigs` cannot see them either, because a module the rule misses never enters the compared set. **Under-selection is the silent direction; over-selection is loud.** Both are now stated where the rule lives, together with the instruction that follows from it: on a false positive, add a KNOWN_UNPREPPED exemption -- do not weaken the rule, which trades a loud false positive for a silent false negative. The discovery is still what found `rig_https_banner.py`, which an allowlist could not; only the absoluteness was wrong. C (must fix) -- "one parser for every `C64_*` switch these rigs read" was an overclaim. Three flags were unified (`C64_SKIP_REU_PREFLIGHT`, `C64_SKIP_DEVICE_PREP`, `C64_FORCE_TURBO_WRITE`). `C64_SKIP_TEMP_GC` still tests `== "1"` / `!= "1"`, inconsistently between its two call sites, and `KEEP_DEBUG_ON_PASS` is spelled three ways across the rigs. The `_ENV_FALSE` comment now scopes itself to the three and names the two it does NOT cover, so nobody reads it as already done. E -- "an on-chip build makes no REU call at all" is true of `_reu_preflight` (literally zero REST calls) and false of `prepare_device`, which still issues the four state reads for the #212 record. Same words, two meanings, one module apart; the prep's docstring now says what it actually means -- no REU is ever configured for an on-chip image, reads excepted, because reads are not the hazard. F -- the side-effect paragraph named REU-Enabled but not REU Size, which these rigs also force to 16 MB from the flash default. Per the harness's own `get_reu_config` docstring that RAM-vs-flash divergence is the exact pair that confused a previous reporter: we never call `save_config_to_flash`, so the device reports 16 MB while flash holds the default, and a reboot silently restores the smaller one mid-session. G -- the refused-write ladder labelled the pre-attempt snapshot "state". `set_config_items` is one PUT per item and explicitly non-atomic ("earlier writes are left in place"), so on a firmware that writes `Cartridge` first, a rejected `RAM Expansion Unit` leaves the device half-configured while the message implies otherwise. Relabelled "state before the attempted write", with the non-atomicity and a re-read instruction spelled out. H -- "fifth item" (module docstring) versus "third item" (CLAUDE.md, README, ladder) read as a contradiction. Both frames are now named in one place: `Cartridge` makes it a THREE-item PUT batch and the FIFTH distinct config item this module is responsible for. Also documented at the call sites: `rig_https_local` and `bench_ecdsa_u64e` now prune on capture-OFF runs where they previously pruned nothing, so such a run can rotate away the oldest of the kept 5 capture dirs; and a PASS with capture off leaves the prep dir behind holding only `device_state.json`, since the cleanup block is nested under `if run_dir is not None`. Both accepted -- unbounded prep dirs would be worse -- and the printed line now says the directory holds the record only. B was raised as a note and needs no code: over-selection is brittle (a helper containing `client.run_prg(` and the word "combines" earns two reds, one demanding that a helper call `prepare_device`) but fails loud. Captured in the guard's docstring so the next person under time pressure adds an exemption instead of loosening the rule. Correcting my own numbers again: the "16 failed / 137 passed vs 16 failed / 167 passed" figures in commit 12527fb were measured on an UNBUILT tree, where build-dependent suites fail for reasons unrelated to this branch. On a clean built tree the reviewer measured master 153 passed / 0 failed and this branch 183 passed / 0 failed -- delta exactly +30, failure sets diff-identical (both empty). The +30 and identical-failure-set claims hold; the framing did not. No hardware this round, deliberately: the U64E was held by a live, progressing holder (PID 73138 running `rig_https_banner.py` at FETCH_TIMEOUT=900, queue depth 0), checked read-only, not queued. Round 1's hardware evidence still carries because the success path is byte-for-byte unchanged since -- the write only gained a try/except. Fittingly, the lane holding the device is the #210 lane that KNOWN_UNPREPPED names as banner's owner. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Closes #197, closes #187, closes the second half of #212.
Three defects of one shape. Device config on an Ultimate is runtime-only, so REU-Disabled at 1 MHz is the factory default, not another lane's mess: a run restores baseline and then sets what it needs. Four of five
preflight_reucallers had nothing for the preflight to back up (#197); all four HTTPS rigs degraded a failed turbo probe intowriting anyway— the exact$88-causing write the probe exists to skip (#187); and no run recorded the device state it ran against (#212).New
tools/uci/_device_prep.py::prepare_device(), called under the DeviceLock afterenable_uciand beforepreflight_reuat all five crypto-path sites.rig_https_wiki.py'sensure_reu_16mbwas generalised into it, not deleted. The record lands asdevice_state.jsonin the run's own artifact dir.The failure policy is per item, deliberately asymmetric
An unreadable REU state writes anyway — the write is the safe action. An unreadable turbo state aborts after one retry — the write is the hazard, and skipping it runs the rig at the previous lane's clock, invalidating every wall-clock number and reproducing #212's misdiagnosed comb boot. Overrides:
C64_FORCE_TURBO_WRITE=1,C64_SKIP_DEVICE_PREP=1.Both directions are mutation-pinned: reinstating the old turbo degrade fails 3 cases, and extending fail-closed to the REU fails 3 different ones. A uniform policy either way fails half the suite, which is what makes the asymmetry load-bearing rather than a rationalisation.
Review sharpened the first half. "Write anyway is safe" only holds because the write is now wrapped, and the code comment says so at the wrap site. With REST refusing, the harness's
Cartridgepreset probe is also inconclusive, soset_reuPUTsCartridge: "REU", a C64U answers HTTP 400 — and unwrapped that escaped every call site, downgrading master's exit-4-plus-wedge-ladder into exit 1 and a traceback, on the path this project most often misreads as firmware corruption. The condition that triggers "write anyway" is the same condition that makes the write fail.Rig coverage is discovered, not listed
PREP_CALL_SITESis gone; rigs are found byclient.run_prg(plus a mention of the comb profile. That foundrig_https_banner.py, which an allowlist never could — it is #212's failure verbatim (menu boot, 75 s budget, "comb boot precompute", no turbo or REU write) and sits inKNOWN_UNPREPPEDwith a reason and an owner, never edited here. Emptying that list goes red naming it.The rule is a text match, and its limits are documented rather than claimed away. Review wrote two rigs with #197's exact defect that pass 30/0 — one naming its client
u64, one never containing the word "comb". Under-selection is silent; over-selection is loud, because a module the rule misses never enters the compared set. The guard's docstring says so, and says what to do about a false positive: add an exemption, never weaken the rule, since that trades a loud false positive for a silent false negative.Also fixed in passing
cpu_speed_enum(1)returns' 1'with a leading space, and master's unstripped comparison meant the turbo skip could never fire atTURBO_MHZ=1— every 1 MHz run performed a redundant,$88-risking turbo write.C64_SKIP_REU_PREFLIGHT=falseused to skip the guard whileC64_SKIP_DEVICE_PREP=falsedid not. One parser now, fail-closed: measured across 13 values,=0and=1unchanged, and every behaviour change moves a guard from OFF to ON. No value exists where a previously-ON guard is now OFF. Scoped honestly —C64_SKIP_TEMP_GCandKEEP_DEBUG_ON_PASSare not unified, and the comment says so and says the work is worth doing.Red-green
tools/test_device_prep.py, 30 cases, faked client, no hardware. Call-site contract red on 4 rigs + bench pre-fix. The five amendment cases red against the first commit's sources — two arriving as raw uncaught_HarnessErrorrather than FAIL, which is the blocker manifesting, confirmed by review as genuine escapes rather than test bugs.Clean-tree pytest: master 153 passed / 0 failed, branch 183 passed / 0 failed, failure sets diff-identical, delta exactly +30.
Harness audited at
1219fe1Verified claim by claim by the reviewer against the sibling tree, not against docs, since it is an editable install and their merges are our regressions:
get_config_value(raises rather than returningNone),get_config_item,get_config_category(unchanged, still bare values),set_reu(C64UCartridgepreset probe),set_turbo_mhz,cpu_speed_enum,apply_factory_baseline+BASELINE_NEVER_TOUCH. No never-touch store is read or written;apply_factory_baselineis deliberately never called, since it resetsU64 Specific Settingsand would perform exactly the unconditional turbo write #187 is about.Hardware
Independent review run, U64E fw 3.15 / commit
4011c97c/ fpga 125, viaacquire_device_lock(). Device found in exactly #212's leftover state (Turbo Control='Manual',CPU Speed='48',RAM Expansion Unit='Disabled'). The turbo write was correctly skipped;set_reu(True, "16 MB")succeeded and read back, with theCartridgewrite correctly omitted on the 3.15 shape; a secondprepare_devicegavewrote=[] skipped_write=['reu','turbo']— idempotent, zero PUTs. State restored, lock released.That evidence still describes the tip: the success path is byte-for-byte unchanged since, the write only having gained a
try/except. Round 2 did not re-run, because the device was held by a live, progressing holder — PID 73138 runningrig_https_banner.pyatFETCH_TIMEOUT=900, i.e. the #210 lane thatKNOWN_UNPREPPEDnames as banner's owner. The exemption's stated ownership is corroborated by the device queue.Still hardware-unproven, and stated as such: the REU-write-refusal path (now a Python-level wrapper, low residual risk) and the C64U
CartridgeHTTP 400 itself, which is unprovable on a U64E at all and rests on the harness's live-verified claim.Accepted side effects
Four more rigs leave the device REU-Enabled and REU Size at 16 MB, up from the flash default. We never call
save_config_to_flash, so the device reports 16 MB while flash holds the smaller default, and a reboot silently restores it mid-session — perget_reu_config's own docstring, that RAM/flash divergence is the pair that confused a previous reporter. An on-chip build is never given an REU, though its four state reads still happen, because the record is the point and reads are not the hazard.A later on-chip run on the same device is therefore no longer free evidence of REU-less operation; that now needs a deliberately REU-disabled device or
C64_VICE_NO_REU=1.Follow-up owed
rig_https_banner.pyneedsprepare_device(client, LABELS_PATH, turbo_mhz=..., artifact_dir=run_dir)afterenable_uci. It has noTURBO_MHZconcept today, so its owner has to decide the clock it should assert. Sequenced after #210.🤖 Generated with Claude Code