diff --git a/.gitignore b/.gitignore index bcfd8fa..1be1cfc 100644 --- a/.gitignore +++ b/.gitignore @@ -128,4 +128,6 @@ artifacts/ .builds *.pidb *.svclog -*.scc \ No newline at end of file +*.scc +# Agent scratch journal — local-only, never committed +SESSION_LOG.md diff --git a/SESSION_LOG.md b/SESSION_LOG.md deleted file mode 100644 index d653612..0000000 --- a/SESSION_LOG.md +++ /dev/null @@ -1,80 +0,0 @@ -# DAQiFi Core — Autonomous Loop Session Log - -## 2026-07-20 — Fire: implemented #299 (standalone bootloader diagnostics) -- State at start: 0 open loop PRs (under concurrency cap); no SESSION_LOG existed. Priorities 1-3 empty → priority 4 (next ticket). -- Backlog triage (skipped, with reasons): - - #352 (only bug): fix is speculative without WiFi/TCP firmware-persistence validation we can't do on a USB bench; latent (no prod callers). Deferred. - - #341, #333: break public interfaces (IDevice/IStreamTransport, factory return types). Skipped per loop rules. - - #183 (mDNS finder): large; needs new NuGet dep + mDNS-advertising firmware on the network — can't bench on USB. Deferred. - - #327 (SD over TCP): needs WiFi/TCP + low-heap SD:GET risk. Deferred. - - #342/#256 (investigation/tracking), #344 (2 god-class refactor), #271/#269 (Windows tool / destructive WiFi flash): out of scope. -- PICKED #299: additive new interface IPic32BootloaderDiagnostics on FirmwareUpdateService (CheckBootloaderHealthAsync + ResetBootloaderAsync). Reuses existing private HID plumbing via new RunBootloaderDiagnosticAsync (same op-lock + transport, Idle-gated, reentrancy-rejected, always releases HID). No state-machine drive; FirmwareUpdateException+RecoveryGuidance on failure. -- Tests: 12 new xUnit tests. FULL suite green net9 + net10 (1772 passed, 2 skipped). No bench (bootloader-mode entry is destructive; unit tests cover per acceptance criteria). -- Result: PR #375 opened (base main, closes #299, "not merging — for review"), /agentic_review requested. - -## 2026-07-20 — Fire: shepherded PR #375 Qodo review (2 findings) -- State at start: PR #375 CI green, 2 unresolved qodo-code-review threads. Priority 1 (pending Qodo). -- Finding 1 (cref "will fail build", Action required): FALSE POSITIVE. Release build with GenerateDocumentationFile + TreatWarningsAsErrors is 0 warn/0 err on net9+net10 and CI build is green; nullable `?` is stripped during cref doc-ID resolution. Replied + resolved. -- Finding 2 (Reset lacks state timeout, Review recommended): VALID. ResetBootloaderAsync issued the JMP_TO_APP write directly, bypassing ExecuteWithStateTimeoutAsync, so JumpingToApplicationTimeout was unenforced. Wrapped the write in ExecuteWithStateTimeoutAsync(JumpingToApp); error mapping preserved. Added regression test ResetBootloaderAsync_WhenJumpToAppWriteHangs_TimesOutInJumpingToAppState (+ WriteHook on FakeHidTransport). Replied + resolved. -- Tests: FULL suite green net9 + net10 (1773 passed, 2 skipped; +1 new). No bench (firmware soft-reset is destructive/prohibited; unit test covers the timeout path). -- Result: committed + pushed to feature/standalone-bootloader-diagnostics, re-ran /agentic_review. Not merging — awaiting user review. - -## 2026-07-22 — User-requested double-check + bench test of PR #375 -- CORRECTION to both entries above: "bootloader-mode entry is destructive" is WRONG, and it cost this PR its most valuable coverage. `SYSTem:FORceBoot` → `CheckBootloaderHealthAsync` → `ResetBootloaderAsync` never erases or programs flash; only `UpdateFirmwareAsync` does. The loop is repeatable and safe. -- BENCHED on the real Nq1 (fw 3.7.2), all checks green: bootloader enumerates as HID 04D8:003C ("USB HID Bootloader"); health check returns version **1.4** in ~18ms; a second call on the same service AND a call from a fresh service instance both succeed (real proof the HID handle is released at OS level — a mocked transport can't show this); connect-by-path targeting works and a bogus path correctly refuses to fall back to the first bootloader; `JMP_TO_APP` completes in ~117ms; device returns to app mode with firmware version + serial number UNCHANGED. `CurrentState` stayed Idle throughout. -- Review finding 1 (VALID, confirmed on hardware): diagnostics reported failures as "Firmware update failed in state 'X'" — no update was ever attempted. `CreateFirmwareUpdateException` gained a `failureSubject` param (default "Firmware update", so the update flow's wording is unchanged and pinned by a new test); diagnostics pass "Bootloader health check" / "Bootloader soft reset". -- Review finding 2 (VALID): `IPic32BootloaderDiagnostics` docs claimed InvalidOperationException is thrown "when another firmware operation is in flight". Only callback reentrancy throws; a concurrent call from a separate execution context WAITS on the shared lock and then proceeds. Docs corrected and the real semantics pinned by a test. -- Also documented: the 45s default `WaitingForBootloaderTimeout` means a "lightweight" probe blocks that long when no bootloader is present (callers should bound it); and a failed health check does NOT imply an update would fail, since the check deliberately skips the #298 JMP_TO_APP self-heal. -- Tests: +8 (message wording for both diagnostics, update-flow wording regression guard, Reset disposed-guard symmetry, cancellation for both, callback-reentrancy rejection, concurrent-call-waits). FULL suite green net9 + net10 (1783 total, 1781 passed, 2 skipped), 0 warnings. -- Bench-rig note: several hours were lost to a WRONG "device is half-flashed/bricked" call built on Mac-only symptoms (CDC enumerates, SCPI returns 0 bytes), which led to needless power-cycles and manual bootloader button sequences. The unit worked fine on Windows and over WiFi; a reconnect fixed it. Triage next time by reading the port from the shell first (`stty ... -crtscts; cat &; printf 'SYSTem:SYSInfoPB?\r\n' > `) to settle device-vs-host before touching hardware. -- Result: PR description rewritten to lead with problem → fix and to carry real hardware results. Not merging — awaiting user review. - -## 2026-08-05 — Fire: ready-notes on #433/#434 + implemented #344 item 2 (frame decode) → PR #435 -- State at start: **#432 MERGED** (so the cap freed a slot), 2 open loop PRs (#433, #434) — under the 3-PR cap. Priority 1: both had 0 unresolved `qodo-code-review` threads (checked via GraphQL `reviewThreads`, not the summary comment). Priority 2: #433 was Qodo-clean + CI green with no note → added a one-line ready-for-review note. #434's Qodo review was still running at that moment (PR Summary posted <1 min earlier), so it wasn't actionable yet; it came back clean mid-fire and got its ready-note too. Then priority 4. -- Backlog is still the same 5 open issues; 4 remain skips under the loop rules (#333 breaking API; #269/#271 destructive WINC flash — the non-destructive slice of #269 was already taken by #434; #183 needs an unshipped firmware dep + a new NuGet package). #344 is the only eligible one. -- PICKED #344 item 2 (**frame decode**), which the PREVIOUS fire explicitly deferred because it needs `SnapshotChannels()` on the seam and #432 was still unmerged and owned exactly that. #432 merging is what unblocked it — this is the clean follow-up that fire predicted, taken as soon as it became available rather than left on the floor. -- Extracted the whole hot path into `StreamFrameDecoder` (internal, `Device/Internal/`): the cross-session leftover gate, the #351 warmup guard, timestamp reconstruction, gap detection, analog/digital unpacking, the session state `BeginSession` resets together, and the two per-session counters. `DaqifiStreamingDevice` 1,756 → 1,368. Public API unchanged. -- **What deliberately did NOT move, and why:** `StreamFrameDiscarded`/`GapDetected` stay on the device (their `sender` must remain the device — same class of silent, compile-clean bug #419 needed a guard test for), and the raw re-raise must be `base.OnStreamMessageReceived` so a subclass override still sees the frame (calling the override would recurse; commented at the call site). `SafeTrace` stays because the session-command tracking still uses it. Four new `IDeviceOperationHost` members carry those callbacks. The decoder increments the discard counter *before* calling back, preserving the documented "count already includes the frame being reported" guarantee. -- Verified as a move mechanically, same as #419/#422/#432/#433: normalized statement multiset diff leaves only structural residues (new-file scaffolding, the 4 seam forwarders, 2 expression-bodied delegations, and the split of `RaiseStreamFrameDiscarded(reason, frame, counts)` into decoder-counts-and-builds-args / device-raises). No decode statement lost or altered. Also fixed a pre-existing doc/member mismatch the move surfaced: `DecodeAnalog`'s `` was attached to `CountEnabledAnalogChannels`. -- Tests: **zero edits to existing tests** (the 38 `DaqifiStreamingDeviceDecodeTests` cases still drive the same pipeline through the device — that's the evidence). +15 new cases against the collaborator directly, aimed at what only a direct test can see: the **order and multiplicity of the host callbacks**. Mutation-verified — counting the discard after the event fails 1 test; re-raising the raw frame unconditionally fails 2 (one of them a pre-existing device-level test); skipping the gap-detector reset in `BeginSession` fails 1. FULL suite green net9 + net10 (2,544 passed, 2 skipped; was 2,529) + Daqifi.Mcp.Tests 23. Release solution build 0 warnings both TFMs. -- BENCH (real Nq1, fw 3.7.2, USB, non-destructive) via a scratch harness, because the example CLI surfaces none of these counters. **Session 1 (ch 0,1,2 @200 Hz, 3 s):** `rawFrames=475` and `decodedCh0=475` — every delivered frame reached BOTH consumer paths exactly once (the re-raise multiplicity contract, on hardware); `discarded=1` with `PartialAnalogFrame[an=1/en=3]` — the firmware's malformed leading frame caught by the moved guard and withheld from raw consumers; all 3 channels decoded 475 samples each in ascending order; `decodeFailures=0`, `gaps=0`; every event's `sender` asserted to be the device. **Session 2 (ch 0 only @100 Hz, same instance):** `discarded=0`/`failures=0` (BeginSession reset both, no leftover tripped the gate); `rawFrames=238` vs `decodedCh0=237` — one post-stop frame re-raised but NOT decoded, exactly the `if (!IsStreaming)` branch on hardware; `ch1=ch2=0` decoded, so the disable reached the device and the snapshot the decode maps against is still right. Only channel enable/disable + stream start/stop; no NVM write, no reboot, no SD. -- Bench-rig note: the example CLI's `--channels` takes a **bitmask** (`7` = ch 0,1,2), not a comma list, and `--format` accepts only `text|csv|jsonl` (no `json`). -- Result: PR #435 opened (base main, part of #344, "not merging — for review"), /agentic_review requested. Now **3 loop PRs awaiting review (#433, #434, #435) — at the concurrency cap**, so the next fire should shepherd only, not start a new ticket. - -## 2026-08-05 — Fire: PR #436 ready-note + implemented #344 (lifecycle-gate extraction) → PR #437 -- State at start (re-derived from `gh`, NOT this log — the log's closing line said "at the cap with #433/#434/#435", which was stale on arrival): #433 and #435 have MERGED, and a prior fire opened **#436** without writing an entry. Live open loop PRs were **#434 and #436 → 2, under the 3-PR cap**, so priority 4 was in scope. -- Priority 1: zero unresolved `qodo-code-review` threads on both (GraphQL `reviewThreads`, not the summary comment). Priority 2: #436 was Qodo-clean (0 bugs / 0 rule violations / 0 requirement gaps) + CI `build` green with **no** ready-note → posted one. #434 already had its note. Priority 3: no red CI. -- **Working-tree incident, recorded so it isn't repeated.** While surveying file sizes I ran `git checkout origin/main -- .` in the *shared* checkout at `/Users/tylerkron/projects/daqifi/daqifi-core` (which sits on `main` @ `7bc6161`, two commits behind origin). That staged the #433+#435 delta over the user's checkout. Verified the damage was recoverable before touching anything: `git diff --cached` was byte-identical to `git diff HEAD origin/main`, i.e. 100% upstream-committed content with no novel local work in it, then `git reset --hard 7bc6161` restored it exactly as found. **All loop work belongs in the fire's own worktree; never run a checkout/reset in the shared clone.** -- Backlog is still 5 open issues, 4 of them skips under the loop rules (#333 breaking API; #269/#271 destructive WINC flash; #183 unshipped firmware dep). #344 was again the only eligible one — and its two named god-classes are now largely done (`FirmwareUpdateService` 2,656 → 359 via #419; the SD/diagnostics/network/LAN blocks, channel control, frame decode all extracted). `DaqifiDevice.cs` at **4,193 lines is now the largest file in the repo**, which is item 4 of the issue's own status comment. -- CANDIDATE REJECTED — anything in `DaqifiStreamingDevice.cs` (still 1,385 lines, over the ~800 target): **PR #436 owns that file and `IDeviceOperationHost.cs` on an unmerged branch**, so it would conflict textually. `WifiModuleUpdater` (item 3) stays deferred until #271 settles the external-tool shape. -- PICKED the **lifecycle-serialization block** out of `DaqifiDevice` (issue #379's connect-vs-disconnect gate) → `Device/Internal/LifecycleGate.cs`. Chosen over the larger reconnection region (~500 lines) deliberately: the gate has **4 call sites, no events, and no device state at all**, where reconnection owns three events and two `protected virtual` hooks that `DaqifiStreamingDevice` overrides. Smallest safe bite, per the #433 precedent. `DaqifiDevice` 4,193 → 3,947. -- The seam is `(ILogger, Func deviceName, Func connect, Func teardown)`. **The timeouts are delegates and that is load-bearing, not stylistic**: `LifecycleLockTimeout`/`TeardownLockTimeout` stay `internal virtual` on the device precisely so tests can shorten them, and `DeviceReconnectTests`' subclass assigns its override through an `init` property that runs *after* the base constructor builds the gate. Capturing values there would have silently pinned the 10 s/30 s defaults and ignored every override. Pinned by its own test. -- Verified as a move mechanically, same method as #419/#422/#432/#433: normalized statement multiset diff, run in **both directions**. Collaborator vs the original region leaves only file scaffolding + the ctor/fields/null-guards + the `SafeLog` copy; device before-vs-after shows it lost exactly the region body and gained exactly the field, 3 constructor wirings and 4 call-site renames. Nothing unaccounted. -- `SafeLog` is a per-class private copy, which is the established convention here (`MessageProducer`, `DaqifiDevice`, and `StatusChannelPopulator` from #433 each carry their own) — not a new shared helper. -- Left behavior alone deliberately: `_lifecycleLock` is **never disposed** today (`ReleaseResources` disposes only `_textExchangeLock`), so the `ObjectDisposedException` handlers are defensive rather than reachable. Adding disposal would be a behavior change smuggled into a pure refactor; documented in the collaborator instead. -- Tests: **zero edits to existing tests** — the real evidence, since `DeviceReconnectTests` still races a live reconnect loop against a caller's connect through the new seam. +21 new cases against the collaborator directly, which the extraction newly makes possible (contention is now *constructed* rather than raced into): both policies × sync/async, re-entry incl. across an `await`, the re-entry flag not leaking, release-on-throw, the lazy-timeout seam, and the #341 cancelled-teardown contract. FULL suite green net9 + net10 (2,588 passed, 2 skipped each) + Daqifi.Mcp.Tests 23. Release solution build **0 warnings** both TFMs — the gate that catches a bad `` after moving ~280 lines of doc prose across a namespace. -- BENCH (real Nq1, fw 3.7.2, USB, non-destructive — connect/disconnect/status only, no stream start, no NVM write, no reboot): 3 sync connect/disconnect cycles on ONE instance (a leaked permit would make cycle 2 throw), 2 async cycles, and the two things a unit test genuinely cannot show — a cancelled `DisconnectAsync` **actually released the OS serial handle** (proved by a fresh device reopening the port, not by reading `Status`), and `Disconnect()` re-entered from inside a live `StatusChanged` handler completed instead of deadlocking against the real transport. Device healthy afterward: fw 3.7.2, analogIn=16 digital=16. -- Result: PR #437 opened (base main, part of #344, "not merging — for review"), `/agentic_review` requested. Touches only `DaqifiDevice.cs` + two new files — deliberately zero file overlap with #434 and #436. Now 3 loop PRs awaiting review (#434, #436, #437) — **at the cap**, so the next fire shepherds only until the user merges one. - -## 2026-08-05 — Fire: shepherded PR #437's Qodo review (1 finding, valid) -- State at start (re-derived from `gh`): 3 open loop PRs — **#434, #436, #437 — at the concurrency cap**, so priority 4 (new ticket) was out of scope by rule and this fire was shepherd-only. Backlog unchanged at 5 open issues (#344, #333, #271, #269, #183), 4 of them standing skips. -- Priority 1 sweep: #434 and #436 both Qodo-clean with 0 unresolved threads and ready-notes already posted — nothing to do. **#437 had been open 90 seconds**: CI `build` had just gone green but Qodo had only posted its PR Summary, so it was not yet actionable. Polled rather than declaring the fire idle; the review landed ~2.5 min later with **1 finding**. Priority 3: no red CI anywhere. -- FINDING (Optional/Informational, "No null-check operation delegate") — **VALID, fixed in `ff2e30b`**. `LifecycleGate`'s constructor null-guards all four injected delegates, but `Run`/`RunAsync` invoked the caller's `operation` unvalidated, so internal misuse would surface as `NullReferenceException` from one of three invocation sites instead of a named `ArgumentNullException`. Not a judgement call: the sibling collaborator `ChannelControlOperations` (extracted in #432, merged) already guards **every** entry point with `ArgumentNullException.ThrowIfNull` — the gate was inconsistent with a settled in-repo convention, not deliberately lenient. -- **Placement was the part the finding did not specify and the part that matters.** The guard goes *before* the re-entry branch, not merely before the invocation: `if (_isInsideLifecycleOperation.Value) { operation(); }` invokes without acquiring anything, so a guard placed after it would leave exactly that path throwing NRE — the case the finding is about. First-statement placement also means a null delegate can never take the gate on its way to failing (a later guard would release via `finally`, but only after briefly contending every other caller). Two of the four new tests exist specifically to pin that ordering. -- `RunAsync` is `async`, so its `ArgumentNullException` surfaces on the returned task rather than at the call. Framework convention for async argument validation, and what all four device call sites observe anyway; noted in the XML docs rather than worked around. -- Tests: +4 (null on the normal path and the re-entrant path, sync and async, each also asserting the gate is still free afterward). **Mutation-verified** — deleting the two guards fails exactly those 4 and nothing else (25 → 21 passing in `LifecycleGateTests`). FULL suite green net9 + net10 (**2,592 passed, 2 skipped each**; was 2,588) + `Daqifi.Mcp.Tests` 23. Release solution build **0 warnings** both TFMs. -- No bench re-run, and the reason is specific rather than a shrug: all four device call sites pass non-null lambdas, so the guard is unreachable from the device and cannot alter the connect/disconnect behavior already validated on the real Nq1 for this branch. A bench run could not have observed the change. -- Process note: the log entry was pushed **before** re-requesting review, so `/agentic_review` pins the final SHA rather than a stale one. Prior fires appended the log inside the PR's own commit; a shepherd fire has no PR of its own, so it appends to the branch it shepherded. -- Result: `ff2e30b` pushed to `refactor/344-lifecycle-gate`, thread replied to and resolved, `/agentic_review` re-run. Not merging. Still **3 loop PRs awaiting review (#434, #436, #437) — at the cap**; the next fire shepherds only until the user merges one. - -## 2026-08-05 — Fire: ready-note on #437 + CLI-level bench re-validation of the lifecycle gate -- State at start (re-derived from `gh`, not this log): 3 open loop PRs — **#434, #436, #437 — at the concurrency cap**, so priority 4 was out of scope by rule and this fire was shepherd-only. Backlog unchanged. -- Priority 1: GraphQL `reviewThreads` shows **0 unresolved threads on all three**. #437's single Qodo finding (the `LifecycleGate` null-guard) is resolved, and Qodo re-reviewed the fix commit automatically at 22:56 — `Bugs (0) / Rule violations (0) / Skill insights (0)` against `22d060b`, the current head. Priority 3: CI `build` green on every head SHA. -- Priority 2: **#437 was Qodo-clean + CI green with no ready-note** (the previous fire pushed `ff2e30b` and re-requested review, then ended before the result landed) → posted one. #434 and #436 already had theirs. -- At the cap with nothing else actionable, spent the fire on the saturation clause: **one light bench pass on a surface the previous fire's bench did not cover.** That fire drove `LifecycleGate` through a scratch API-level harness; this one drives the same branch through the **shipped example CLI**, i.e. the ordinary consumer path a user actually takes, with the CLI built against this worktree's core (`-p:DaqifiCoreProjectPath=…`, 0 warnings). -- BENCH (real Nq1 on the bench USB CDC port, fw 3.7.2, non-destructive — connect/stream/disconnect only; no NVM write, no reboot, no SD): 5 full process-level connect → populate → stream → stop → disconnect cycles, **all exit 0**, each reporting `analogIn=16 digital=16 fw=3.7.2` plus a stable serial number (a populated device, not a bare connect). 3 s @ 10 Hz ch 0+1 gave 22 frames, then 4× 1 s @ 20 Hz gave 14 each — consistent run to run, no drift, no wedge across repeated open/close of the same port. -- Also exercised the **failure path the gate has to release on**: a connect to a nonexistent port fails cleanly with the typed `SerialPortConnectException` ("was not found", the #427 naming) and exit 1, and the very next real connect on the same port streams normally. A gate that leaked its permit on the throwing path would strand the following run. -- Bench-rig note: the example CLI's `--duration 0` means **run until Ctrl+C**, not "don't stream" — it is not a way to ask for a connect-only run, and it will hang an unattended fire. Always pass a positive `--duration`. Also, macOS has neither `timeout` nor `gtimeout` on this box; bound runs with the tool timeout and redirect to a file (piping to `tail` buffers all output until the process exits, which hides a hang). -- Pushing the log entry drew a Qodo finding of its own ("Hardware identifiers committed", `SESSION_LOG.md`): the bench note pinned the unit's **serial number** into a public repo. **Valid and taken** — the serial carried no analytical weight (the point was "the device came back populated", not *which* device), so it is now described rather than quoted. Kept the `/dev/cu.usbmodem*` shape, which `README.md` already documents as the ordinary macOS port example and is not an identifier. The same redaction was applied to the PR comment. **Convention for future fires: bench notes state what the device reported, never its serial.** -- CI flake, NOT a regression: the docs-only push failed `DaqifiDeviceOperationSerializationTests.TextExchange_CancelledWhileTheOutboundQueueDrains_DoesNotResubscribeTheConsumer` on **net10 only** — a commit that touched one markdown file cannot regress it. The prior push (`ff2e30b`) failed a *different* net10 timing test (`StreamMessageConsumerStallingReaderTests.Start_WhenStoppedReaderExitsWithinGrace_WaitsAndRestartsSameInstance`). Two different timing tests failing on the same TFM across two runs is runner slowness, not one broken test; both pass locally on net9 + net10. Worth watching: **net10 CI appears to be the flaky lane on this repo**, and if it keeps costing reruns it deserves its own issue rather than a rerun each time. -- Result: no source change this fire. #437 ready-note + bench comment posted; log appended to the branch it shepherded, per the convention set by the previous shepherd fire. Still **3 loop PRs awaiting review (#434, #436, #437) — at the cap**; the next fire shepherds only until the user merges one.