Repository navigation
test(firmware): pin what the firmware-update state machine refuses to do - #718
Conversation
The transition table in FirmwareUpdateContext is only ever walked forwards by the two update flows, so nothing today asks it to say no. An edge added to the table — letting Programming report Complete without verifying, or letting Failed resume mid-flow instead of restarting — breaks no existing test. ResetIfTerminalState has the same shape: the flows only call it when the machine is already terminal, so its guard could vanish unnoticed. Adds direct tests that assert the machine's properties (every phase can still report failure; no state is stranded away from Idle) and the specific edges that must not exist, plus the self-transition shortcut, the rejection path leaving nothing half-applied, and PreviousState on the public event args, which nothing asserted. The graph is discovered by probing a real context rather than read out of the production dictionary, so a mutated table produces a mutated graph. Tests only; no production code changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/agentic_review |
PR Summary by QodoPin firmware update state machine rejection contracts
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…erties Qodo round 1, both findings conceded. A state that lost every inbound edge fell out of the discovered graph, so both graph properties stopped covering it instead of failing — the coverage vanished with the state. The properties now walk the declared enum, and a new case asserts every declared state is still reachable from Idle, so disconnecting one fails loudly. The non-terminal reset theory was also missing Connecting, JumpingToApp and ReconnectingAfterFlash, all live phases: treating any of them as terminal would rewind an in-flight update to Idle with the guard test still green. Mutations rerun (19, all red, baseline green after each restore). Three are new: disconnecting JumpingToApp now kills 4 cases where it previously killed none, and treating Connecting or ReconnectingAfterFlash as terminal each kill their new row. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Both round-1 findings conceded and fixed in 1. Unreachable states escape properties — correct, and it was the more interesting of the two. 2. Reset guard skips active states — also correct. Suite is 27 cases now. Mutation set rerun end to end: 19 mutations, all 19 red, baseline green after each restore, union of kills covering all 27 cases. Full solution suite under a test lock with /agentic_review |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit e5802a5 |
|
Qodo-clean, CI green — ready for review. (Round 2 on |
Produced by the coverage-gap maintenance routine (run 9). Tests only — no production code is touched, so it cannot change behaviour. Not merging — for review.
What was wrong
FirmwareUpdateServicedrives a state machine, and consumers see it through two pieces of public API:FirmwareUpdateState CurrentStateand theStateChangedevent. The machine's legal-transition table lives in an internal collaborator, and today it is only ever walked forwards by the two update flows (PIC32 and WiFi) along the transitions they already make. Nothing has ever asked it to say no.That leaves the whole denial half of the contract unpinned. Someone could add an edge letting
Programmingjump straight toComplete— reporting a successful update that was never verified — or lettingFailedgo back toPreparingDeviceso a retry resumes mid-flow instead of restarting, and every test in the suite would still be green.ResetIfTerminalStatehas the same shape: the flows only ever call it when the machine is already terminal, so its guard could be deleted entirely and nothing would notice, even though without it the call the service makes before starting work would silently rewind an in-flight update toIdle.Separately,
FirmwareUpdateStateChangedEventArgs.PreviousStateis shipped public API and was asserted nowhere at all — the two states could have been reported the wrong way round and only a consumer's progress display would have shown it.How it is fixed
One new test file,
FirmwareUpdateStateMachineTests.cs(23 cases). Rather than restating the table row by row, it asserts properties of the machine — every phase can still reachFailed, and no state is stranded away fromIdleso the service stays reusable — plus the specific edges that must not exist, the self-transition shortcut that renames the current operation without announcing a change, and a rejected transition leaving state, operation and subscribers untouched.The graph the property tests reason over is discovered by probing a real context, not read out of the production dictionary, so a mutated table produces a mutated graph and the tests cannot quietly agree with it.
Mutation evidence
Every case was proved able to fail. Sixteen mutations were applied one at a time to
FirmwareUpdateContext.csfrom a pristine copy, each re-running the new file; all sixteen went red and the baseline was green again after each restore. The union of what they killed is all 23 cases.They cover: dropping a phase's
Failededge; strandingRecoveredwith no route home; six added edges that skip a flash step (Programming/ErasingFlash→Complete,ErasingFlash→Verifying,Connecting→Programming,WaitingForBootloader→ErasingFlash,ReconnectingAfterFlash→CleaningUp); lettingFailedandCompleteresume mid-flow; removing or hollowing out the self-transition shortcut; reporting the transition backwards; mutatingCurrentStatebefore the legality guard; resetting unconditionally; and droppingRecoveredfrom the terminal set.Verification
Full solution suite under a test lock,
-warnaserror: net9.0 and net10.0 both 4171 passed / 3 skipped / 0 failed,Daqifi.Mcp.Tests217 passed on both. Zero warnings.git diff --statagainstmaintouches nothing undersrc/Daqifi.Core/.🤖 Generated with Claude Code