Take the new shared vectors, and stop padding BULK transfers #62
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/shared-bulk-vectors"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
The firmware branch has published vectors for
SYS_WIFI_CONTROL,SYS_WIFI_STATUSandCLASS_BULK, all of which this repository transcribed before the firmware implemented any of it. This refreshestests/vectors/frames.jsonfrom that branch and takes the specification's side wherever it and our transcription disagreed.Where a vector disagreed with us, all of it in wire field names nothing had pinned before:
SYS_WIFI_CONTROL:timeoutMinistimeoutMinutes,updateServerIpv4isserverAddress,updateServerPortisserverPort.SYS_WIFI_STATUS:ipv4isaddress.BULK_BEGIN,BULK_DATAandBULK_END:transferIdistransfer, andcrc32iscrc.BULK_DATAcarries alengthalongside its payload, because a short final frame is now canonical.Our own names for those fields are unchanged, so nothing above this layer moves.
Behaviour that changed, from sections 8.9 and 11:
BULK_ENDresult 4, content not usable, is a refusal with a name rather than an unknown code.SYS_WIFI_STATUSstate 6 is named. Nothing acts on it yet; CCS-UHA-15 and CCS-UHA-17 build on the name existing.Our CRC32 already matched the polynomial the specification now names, and a test pins it against the vectors rather than a hand-copied constant.
Vectors and CI. The new cases exist only on the firmware's
feat/firmware-updatesbranch, and the CI step rantools/refresh-vectorswith no argument, which defaults tomain. It refetched main's file and read the difference as drift, so the first run failed.tests/vectors/SOURCEnow records the branch and the script reads it, so CI refreshes against the same branch the committed vectors came from. That alone would have been a trap: a merged branch is kept rather than deleted, so its vectors freeze, and a build would go on matching that snapshot while the firmware'smainmoved on unwatched. Sotools/refresh-vectorsalso refuses to run when it is building formainandSOURCEsays anything else, withci.ymlpassing the target branch inVECTORS_TARGET.tests/test_refresh_vectors.pycovers the guard, the argument still winning, and that the variable sits on the step that runs the script rather than anywhere in the file.The net effect is that
SOURCEgoing back tomainis enforced rather than remembered.Not changed, deliberately:
UpdateSession._watchends a session on state 5 and not state 6, so a node reporting a failed fetch is watched until the ten minute timeout and reported asLOST. That is CCS-UHA-15's.Tested with
tools/check: 1009 passed, 3 skipped, ruff and mypy clean.feat/firmware-updatesmerged in at15cbb2e; pipeline green on the merged head (run #1402 on76ad114). Refs CCS-UHA-14.The layout, the short-frame encode in both directions, and the session's refused/ignored/too-long paths all check out against section 8.9. Six things:
tests/test_bench_agrees_with_the_protocol.py:75— nothing ties the catalogue'scountedto the bench shortening the frame. Both sides now have two such fields (SYS_ENDPOINT_LABEL.text,BULK_DATA.payload) and they agree today, but a future counted field added on one side only is exactly the "one side sending eight bytes where the other expects two" drift that file exists to catch. Addassert (isinstance(mine, TextField) or (isinstance(mine, RawField) and mine.trailing)) == (field.counted is not None)totest_the_field_layout_agrees.updater/session.py:396— state 5 now reportsdetail="the node reported the update failed", which is what state 6 means. Section 11: state 5 is a node that never started a join (no credentials, or its own radio refused). Reword to say the join never started.tests/test_updater_session.py:671—assert zlib.crc32(b"van-up\0sekrit\0") == 0xDAE34469hardcodes the content and CRC of thebulk_begin_session_credentialsvector. If the firmware republishes that vector with different content, this still passes and silently stops checking anything shared. Read the expected value out offrames.jsoninstead.updater/session.py:247— the cap checks only the 98-byte total, not the 32/64 split it is derived from. A 40-byte SSID with a 50-byte key is 92 bytes and passes, and no node can hold it. Check the two parts.tools/frame_bench/protocol.py:203—describe()emitstrailingand nothing reads it:app.jsbuilds the same text control either way and still shows"00".repeat(width)as the placeholder. Either drop it fromdescribe()or have the browser use it.tools/frame_bench/examples/send-the-lighting-node-for-its-firmware.json— the credentials are 24 bytes, so the one committed example of aBULKtransfer divides exactly by six and never shows the short final frame this branch makes canonical. Change the passphrase so the example demonstrates it.All six taken, and the bench example now pins the short final frame end to end through
test_a_session_sends_what_the_bench_example_sends— that is a better test than what I asked for. Two small things left, neither blocking:tests/test_updater_session.py:690— the content is reassembled from everyBULK_DATAcase in file order, with no tie back to theBULK_BEGINcase's transfer. A refresh that adds aBULK_DATAvector from a second transfer, or in a different order, fails this test on the length assertion with nothing saying why. Filter oncase["fields"]["transfer"] == declared["transfer"]and sort bysequence.tests/test_updater_session.py:26—from tests.test_vectors import CASES as VECTORSworks and nothing else intests/does it, but it makes an updater test fail at collection ifframes.jsoncannot be read, and it is the only reasontest_vectorsis importable as a data source. Avectors()helper intests/fakes.pythat both modules call would keep the data shared without one test module importing another. Your call; I would not hold the merge for it.Both points from the second review, and the CI failure they arrived alongside.
Taken: the CRC test now picks its
BULK_DATAcases by theBULK_BEGINcase's transfer and reassembles them by sequence, with an assertion that the sequences run from zero with no gap. Reading the file in order was exactly as fragile as you said.Not taken: moving the vectors into a
vectors()helper intests/fakes.py. You are right that an unreadableframes.jsonnow breaks collection of an updater test as well as the vector tests, but that is a build that is already failing for the right reason, and a shared helper is a second place the vectors are reached from. Worth revisiting if a third test module wants them.CI. The first run failed for a reason neither of us was looking at: the vectors step runs
tools/refresh-vectorswith no argument, which defaulted tomain, so it refetched main's file over the branch's and read the difference as drift. The branch is now recorded intests/vectors/SOURCE, the script reads it when no argument is given, andtests/test_refresh_vectors.pycovers both that and an argument still winning.SOURCEhas to go back tomainonce the firmware branch lands, which the script, the README and the PR body all say.Green on the merged head: run #1399 on
8390850.The stub-curl test does prove what it claims — putting
${1:-main}back failstest_it_fetches_from_the_branch_the_vectors_came_from, and the argument-wins case is real too. The CRC tightening is right in both directions: a second transfer's vectors are now ignored, and removing sequence 1 fails with "the transfer has a gap in it" rather than a bare length mismatch.One thing to fix before this lands, and it is the trap you asked about:
.forgejo/workflows/ci.yml:43/tests/vectors/SOURCE— the silent failure mode is real, and it is the likely one. This PR merges into the HA repo'sfeat/firmware-updates, soSOURCEcontainingfeat/firmware-updatesrides into HAmainwith it. Merged branches are kept in these repos —fix/manifest-for-the-specandfix/stop-a-download-on-shutdownare both merged and still on the remote — so the firmware'sfeat/firmware-updateswill survive its own merge and sit there frozen. CI then keeps fetching a snapshot that has stopped tracking the firmware'smain:cmpmatches, the step passes, and the build stays green while the wire format moves. That is the drift CLAUDE.md names as the risk that defines this repository, and before this commit it could not happen, because CI always checkedmain.The mechanism itself is fine; what is missing is that nothing checks
SOURCE. The README paragraph is the only guard and a README is not a check. Add a mechanical one to the vectors step: when the branch under test ismain, fail ifSOURCEis notmain. A feature branch may point wherever it likes;mainmust point atmain. That also makestest_the_committed_vectors_say_where_they_came_fromsufficient as it stands, since the relationship is then enforced where it can be.Nothing else. The deleted-branch case is a 404 and fails loudly, which is fine.
Taken, in
b516505. You were right that a README is not a check, and the failure mode you described is worse than the one I was fixing: green for the wrong reason, for ever, with nothing to notice it.tools/refresh-vectorsnow refuses to run whenVECTORS_TARGETismainandSOURCEsays anything else, and the vectors step inci.ymlsetsVECTORS_TARGETto${{ github.base_ref || github.ref_name }}so a pull request is judged on what it merges into. Four tests: the guard firing,mainfrommainstill working, a feature branch still free to take a firmware branch, and one readingci.ymlto check CI actually passes the variable — without which the guard never fires and the other three prove nothing.Confirmed by hand on this worktree:
VECTORS_TARGET=main tools/refresh-vectorsexits 1 namingSOURCE, andVECTORS_TARGET=feat/firmware-updatesfetches and matches.1009 passed, 3 skipped, ruff and mypy clean.
The guard itself is right. By hand in the worktree:
VECTORS_TARGET=mainexits 1 namingSOURCE,VECTORS_TARGET=feat/firmware-updatesfetches the live firmware branch and reports the vectors current, and replacing the condition withif falsefailstest_main_will_not_take_its_vectors_from_a_feature_branch.One way round it, which is the ci.yml assertion you asked about:
tests/test_refresh_vectors.py:127— the assertion is a substring match against the whole file, so it does not check theenv:sits on the step that runs the script. I moved the entire block to theLintstep: all seven tests stay green, the vectors step never seesVECTORS_TARGET, and the guard can never fire. That is exactly the failure the test's docstring says it prevents. PyYAML 6.0.3 is already in the dev requirements — parse the workflow and assert the variable is on the step whoseruncontainstools/refresh-vectors.On
github.base_ref, which I could not verify on this runner: it is populated only forpull_requestandpull_request_target, and empty for push, dispatch and schedule. The workflow fires onpush: branches: [main]andpull_requestonly, so a push to main givesref_name: mainand the guard fires — that half is solid, and line 93 already trusts the push context the same way. If Forgejo does not populatebase_ref, a pull request into main falls back toref_name, which on apull_requestevent is<n>/merge, and the guard would not fire before the merge. Thepush: [main]trigger catches it on the very next run, so the worst case is main going red just after the merge rather than the PR going red just before — degraded, not silent. This PR's own run cannot tell you which it is, since it merges intofeat/firmware-updatesand is green either way. Addecho "Vectors target: ${VECTORS_TARGET:-unset}"to the step and the next run settles it permanently.Nothing else. Note
b516505is not on the remote yet —origin/feat/shared-bulk-vectorsis still at8390850.Taken, in
76ad114. The substring match was worth nothing, exactly as you showed.test_ci_tells_the_script_which_branch_it_is_building_fornow parsesci.yml, finds the one step of thequalityjob whoseruncalls the script, and assertsVECTORS_TARGETis in that step'senv. Reproduced your mutation first: moving the block to theLintstep fails the test and nothing else, and restoring it passes. PyYAML and its stubs are now named inrequirements-dev.txtrather than borrowed from Home Assistant, since the test imports them directly.On
base_ref: addedecho "Vectors target: ${VECTORS_TARGET:-unset}"to the step, so the next run says which way the runner behaves rather than leaving it to be worked out again. Your degraded-not-silent reading is the one I have taken —push: branches: [main]catches it either way.Also: the worktree was left on a detached HEAD, which is how
b516505came to be off the branch. Recovered withgit checkout -B; nothing lost, and both commits are on the remote now.CI green on
76ad114, run #1402.