test(export): census the range guards in Logging/Export so they cannot drift apart

[tylerkron]

What was wrong

When Core rejects an out-of-range argument, what the caller actually gets back is inconsistent. Some guards throw the two-argument ArgumentOutOfRangeException, which leaves ActualValue null — so the value that was rejected simply isn't reported — while others use the three-argument form and do report it. Nobody notices, because nothing checks. ScpiMessageProducer got a census in #660 that locks its nine guards into one shape, but that's one file, and the new guards aren't landing there: #657 added two in Logging/Export, and #668 added another while this branch was open.

So the region that's guaranteed uniform is not the region that's growing, and a caller writing catch (ArgumentOutOfRangeException ex) can't rely on ex.ActualValue being populated.

How it was fixed

A companion census over Logging/Export — the folder where the recent guards actually landed, four sites today. It invokes each guard and asserts the exact ParamName, ActualValue and message the caller sees, and that the message is a sentence ending in a period. Then it reads the folder's source and asserts the table matches every ArgumentOutOfRangeException site it finds, per file, so a guard added without a census entry fails loudly instead of being quietly uncovered.

The thing a reviewer is most likely to want to argue with: CsvExporter composes its ParamName as "options.AverageWindow" where everything else uses a plain nameof. That's the one real divergence in scope, and I pinned it as-is rather than normalising it, with a comment saying why — ParamName is observable, a caller can filter on it, and the composed form says which option was rejected where nameof(options) wouldn't. To stop that becoming a loophole, the census resolves every ParamName structurally: the first segment must name a real parameter of the throwing member and the tail a real public member of that parameter's type, so a rename that leaves the string behind fails.

The divergent guards outside this folder (DigitalChannel, AnalogChannel, HidLibraryTransport, TimestampProcessor, the two SD-card files) are out of scope and stay recorded in #664.

Verification

The census can fail, and I checked, by mutating production code and reverting:

• Two-argument constructor on LiveCsvRecording's bufferCapacity guard — i.e. drifting it into the shape six other guards in the repo already have — turns the census red on ActualValue, while every other test in the export test classes stays green, including LiveCsvRecordingTests' own bufferCapacity test. That's the point: the existing per-site tests assert type and ParamName only, so this drift is invisible to them.
• An extra uncensused guard in CsvExporter turns the source scan red, naming the file.
• It also catches a whole new file, not just new guards in existing ones — which is not hypothetical: feat(export): ship the SD-card log adapter Core made everyone write themselves #668 landed SdCardLogSampleSource with a constructor guard while this branch was open, the scan went red naming it, and 59211a6 adds the row (widening GuardSite from MethodInfo to MethodBase so a constructor censuses on the same terms).

Full suite green on net9.0 and net10.0 under -warnaserror: 3982 passing, 0 failing, plus Daqifi.Mcp.Tests. Bench validation doesn't apply — test-only change, no production code touched.

closes #664

Not merging — for review.

🤖 Generated with Claude Code

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Add a guard census for Logging/Export range checks to prevent drift

🧪 Tests 🕐 20-40 Minutes

AI Description

• Add a structural census test covering all range guards in Logging/Export.
• Assert uniform exception shape (ParamName, ActualValue, message sentence/period).
• Scan source files to ensure every throw site is represented in the census table.

Diagram

graph TD
  A["ExportGuardCensusTests"] --> B["Census table (3 sites)"] --> C["Invoke guards w/ bad args"] --> D["Assert ex shape (ParamName/ActualValue/message)"]
  D --> E["Reflection: ParamName resolves"]
  A --> F["Scan \"src/Daqifi.Core/Logging/Export/*.cs\""] --> G["Count ArgumentOutOfRangeException sites"] --> H["Assert census completeness"]
  C --> I["CsvExporter.ExportAsync"]
  C --> J["LiveCsvRecordingExtensions.RecordLiveSamplesToCsvAsync"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Centralize range guards into shared helper methods

• ➕ Eliminates per-call-site constructor drift by construction
• ➕ Reduces need for reflective/source-scanning tests
• ➖ Requires production refactor across multiple call sites
• ➖ May reduce guard message specificity unless carefully designed

2. Add a Roslyn analyzer/code fix to enforce exception constructor/shape

• ➕ Scales repo-wide without expanding runtime tests
• ➕ Can enforce rules at build time (ParamName, constructor usage)
• ➖ Higher up-front implementation cost
• ➖ Adds analyzer maintenance burden and build complexity

3. Expand the existing SCPI census to cover the whole repository

• ➕ Single, unified catalog of all range guards
• ➕ Consistent policy enforcement everywhere
• ➖ Large initial scope (many throw sites)
• ➖ High churn/maintenance as guards are added across unrelated areas

Recommendation: The folder-scoped census + source scan is a good incremental strategy: it targets the area where new guards are being added while preventing silent drift and uncensused additions. If guard proliferation continues beyond a few hotspots, consider graduating to a shared guard helper or a Roslyn analyzer for repo-wide enforcement.

Files changed (1) +281 / -0

Tests (1) +281 / -0

ExportGuardCensusTests.csAdd export range-guard census with source-scan completeness checks +281/-0

Add export range-guard census with source-scan completeness checks

• Introduces a table-driven census asserting that every Logging/Export range guard throws ArgumentOutOfRangeException with identical, pinned observable shape (ParamName, ActualValue, and sentence-style message). Adds reflection-based validation that ParamName (including the composed options.AverageWindow) still refers to real parameters/members. Scans the Export source directory to ensure all throw sites are represented so new guards cannot be added without updating the census.

src/Daqifi.Core.Tests/Logging/Export/ExportGuardCensusTests.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. Guard scan matches non-throws ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

ThrowSitesInExportSource() is intended to count real ArgumentOutOfRangeException range-guard throw
sites in Logging/Export, but it currently counts any line containing the string
"ArgumentOutOfRangeException" and only partially skips comments. As a result, non-guard references
(e.g., catch blocks, typeof, other mentions) and block comments like /* ... */ that mention the type
can be miscounted as guard sites, causing spurious census failures and breaking the
guard-completeness invariant.

Code

src/Daqifi.Core.Tests/Logging/Export/ExportGuardCensusTests.cs[R253-256]

+                if (trimmed.Contains(nameof(ArgumentOutOfRangeException), StringComparison.Ordinal))
+                {
+                    sites.Add((Path.GetFileName(path), i + 1));
+                }

Relevance

●●● Strong

Scanner contradicts documented intent by counting non-throw mentions; recent folder reviews favor
tightening test correctness.

PR-#657

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Although the method/test conceptually deals with “ThrowSites,” the implementation adds a site for
any line that contains the type name regardless of whether the line is actually a guard/throw
statement, which means unrelated references (like catch clauses or typeof) would be treated as guard
sites. Additionally, the scan claims to ignore doc comments, but it only skips lines starting with
"//" or "*"; it does not properly exclude /* ... */-style block comments (or interior block-comment
lines not beginning with "*"), so a mention of ArgumentOutOfRangeException inside such comments
would still be added to the sites set and counted as a guard site, contradicting the documented
intent and creating false-positive failures.

src/Daqifi.Core.Tests/Logging/Export/ExportGuardCensusTests.cs[231-257]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The completeness test should only require census updates when actual *ArgumentOutOfRangeException range guards* are added/changed in `src/Daqifi.Core/Logging/Export`. `ThrowSitesInExportSource()` currently (1) counts any occurrence of the string `ArgumentOutOfRangeException` even when it’s not a guard/throw (e.g., `catch (ArgumentOutOfRangeException)`, `typeof(ArgumentOutOfRangeException)`, or other mentions) and (2) claims to ignore doc/comments but only skips `//` and `*`-prefixed lines, so `/* ... */` block comments (and non-`*` interior lines) can be miscounted as guard sites.

## Issue Context
Right now the folder happens to contain only `throw new ArgumentOutOfRangeException(...)` plus XML-doc mentions, so the scan “works,” but future edits can easily introduce non-guard references or block-comment mentions that create noisy, misleading test failures. This is a drift-prevention test; false positives will train developers to distrust/disable it.

## Fix Focus Areas
- src/Daqifi.Core.Tests/Logging/Export/ExportGuardCensusTests.cs[231-257]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Reflection lookup brittle ✓ Resolved 🐞 Bug ☼ Reliability

Description

Census() uses Type.GetMethod(name) without parameter types; if an overload is added later, GetMethod
can throw AmbiguousMatchException or return an unintended overload, making the census fail in a
non-actionable way or validate the wrong signature.

Code

src/Daqifi.Core.Tests/Logging/Export/ExportGuardCensusTests.cs[R67-70]

+        var export = typeof(CsvExporter).GetMethod(nameof(CsvExporter.ExportAsync))!;
+        var record = typeof(LiveCsvRecordingExtensions)
+            .GetMethod(nameof(LiveCsvRecordingExtensions.RecordLiveSamplesToCsvAsync))!;
+

Relevance

●●● Strong

Recent same-folder history accepts test-robustness fixes; overload-safe reflection avoids
AmbiguousMatchException risk.

PR-#657

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new test uses name-only reflection lookups for the throwing methods, which is inherently
ambiguous once overloads exist.

src/Daqifi.Core.Tests/Logging/Export/ExportGuardCensusTests.cs[65-70]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`Census()` retrieves methods by name only. This is fragile if `CsvExporter.ExportAsync` or `LiveCsvRecordingExtensions.RecordLiveSamplesToCsvAsync` gain overloads.

### Issue Context
This test is intended to be a stable contract lock; ambiguous reflection failures will be confusing and slow to diagnose.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Logging/Export/ExportGuardCensusTests.cs[67-70]

### Suggested fix
Use `GetMethod(string, Type[])` (or `GetMethods().Single(m => ...)`) with an explicit parameter type list that matches the intended overload, e.g.:
- For `ExportAsync`: `(ISampleSource, TextWriter, CsvExportOptions, IProgress<int>, CancellationToken)`
- For `RecordLiveSamplesToCsvAsync`: `(IStreamingDevice, TextWriter, CsvExportOptions, TimeSpan?, int?, CancellationToken)`
This ensures the ParamName validation is always bound to the correct signature.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context sources

Review mode: ⚖️ Balanced

Tip of the day

💡 Did you know, you can ask Qodo to dismiss a finding you disagree with, with your reason on record

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[tylerkron]

Both findings taken, fixed in e4ab807.

1. Guard scan matches non-throws — agreed, and it was the more serious of the two. The scan counted any line mentioning ArgumentOutOfRangeException outside a // or * line, so a catch, a typeof, or a mention inside a /* */ block would have failed the census for a change that added no guard. A drift test that cries wolf is one people switch off. It now matches throw new ArgumentOutOfRangeException and the ArgumentOutOfRangeException.ThrowIf* helpers only (keeping helper-form coverage, which was the reason the match was loose), over comment-stripped lines that carry block-comment state across the file. Re-ran the mutation that adds an uncensused guard to CsvExporter against the tightened scan: still red, same message.

2. Reflection lookup brittle — agreed. Type.GetMethod(name) would throw AmbiguousMatchException on the first overload added, which tells the reader nothing. Resolution is now by enumeration with an assertion whose message says what to do: the census must name which overload its guard lives in.

Full suite green on net9.0 and net10.0 under -warnaserror after the change (3905 passing, plus Daqifi.Mcp.Tests).

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit e4ab807

[tylerkron]

Qodo-clean, CI green — ready for review.

Independently re-verified at head e4ab807 (already contains current main c546abd, so the test-merge is a fast-forward):

• Both claimed mutations reproduce. Two-arg constructor on the bufferCapacity guard → census red on ActualValue (Expected (…, "bufferCapacity", 0) / Actual (…, "bufferCapacity", null)), with all 75 other tests in the three export test classes green, including LiveCsvRecordingTests' own bufferCapacity test. An extra uncensused guard in CsvExporter → source scan red (CsvExporter.cs: 1 vs 2).
• Round 1's scan fix is correctly calibrated — not overcorrected. A file containing only non-guard mentions (<exception cref> doc tag, catch, typeof, is, and a block comment literally spelling out throw new ArgumentOutOfRangeException) yields zero sites: no false positives. The one blind spot is a throw new dangling on its own line before the type name; zero of the repo's throw sites are written that way, so it is theoretical rather than live. The scan is also non-recursive, which matches a folder that has no subdirectories.
• The scan notices a new file, not just new guards. Dropping a new .cs file with a conventional guard into the folder turns the scan red and names the file.
• Divergence documentation checked. The "options.AverageWindow" note states why it is pinned (ParamName is observable contract; the composed path says which option was rejected) and the structural resolve keeps it honest. It reads as a decision, not an oversight.
• Full suite under -warnaserror: net9.0 3905 / net10.0 3905 / Daqifi.Mcp.Tests 217, 0 failures. CI build success on e4ab807.
• Qodo round 2 confirmed on both surfaces at this head: Code Review by Qodo at Bugs (0) / rule violations (0) / skill insights (0) with both round-1 findings struck through, and an unfiltered reviewThreads dump returning 2 threads, both isResolved: true — a non-empty result, so the query is proven live rather than silently filtering. Settled 15 minutes; unchanged.

One thing reviewers should weigh before merging: the merge order with #668 has a real cost. Confirmed against #668 at head 2c1de9b — it adds SdCardLogSampleSource.cs to this very folder with an analogChannelCount guard. The census will go red for whichever of the two merges second; it does not silently ignore the new guard. The row cannot be pre-added here (typeof(SdCardLogSampleSource) does not compile until #668 lands), and loosening the scan to tolerate a known-pending file is exactly what would make the census vacuous, so it is deliberately not papered over. Recommended order and the exact two-part follow-up — the new guard sits in a constructor, so GuardSite.Method needs widening from MethodInfo to MethodBase as well as the new row — are now written into the PR body.

Not merging.

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 59211a6

[tylerkron]

Qodo-clean, CI green — ready for review, now at head 59211a6.

The earlier ready note was for e4ab807, before the merge-order follow-up landed. Re-verified against the current head:

• Qodo round on 59211a6: Bugs (0), rule violations (0), both round-1 findings still struck through as resolved, and zero unresolved inline threads. Re-checked ~4 minutes after the summary settled — both surfaces still empty.
• Full suite green locally on net9.0 and net10.0 under -warnaserror: 3982 passing, 2 skipped, 0 failing, plus Daqifi.Mcp.Tests (217). CI green on all four jobs for this head.
• The merge-order caveat is now closed. feat(export): ship the SD-card log adapter Core made everyone write themselves #668 merged first, as recommended, and 59211a6 adds the SdCardLogSampleSource census row — so the branch is a single new test file against current main, with nothing left pending. I've rewritten the PR description accordingly; the "this WILL go red, by design" section it used to carry no longer applies and was removed rather than left to confuse a reviewer.

No production code touched, so bench validation doesn't apply.

[qodo-code-review]

Qodo-clean, CI green — ready for review, now at head 59211a6.

The earlier ready note was for e4ab807, before the merge-order follow-up landed. Re-verified against the current head:

• Qodo round on 59211a6: Bugs (0), rule violations (0), both round-1 findings still struck through as resolved, and zero unresolved inline threads. Re-checked ~4 minutes after the summary settled — both surfaces still empty.
• Full suite green locally on net9.0 and net10.0 under -warnaserror: 3982 passing, 2 skipped, 0 failing, plus Daqifi.Mcp.Tests (217). CI green on all four jobs for this head.
• The merge-order caveat is now closed. feat(export): ship the SD-card log adapter Core made everyone write themselves #668 merged first, as recommended, and 59211a6 adds the SdCardLogSampleSource census row — so the branch is a single new test file against current main, with nothing left pending. I've rewritten the PR description accordingly; the "this WILL go red, by design" section it used to carry no longer applies and was removed rather than left to confuse a reviewer.

No production code touched, so bench validation doesn't apply.

The current head looks ready to merge from the review perspective. The diff is test-only and the census now covers the four export range-guard sites, including SdCardLogSampleSource(analogChannelCount); the source scan also checks that the census cannot silently become incomplete. Both prior findings—finding 1 and finding 2—are implemented, with no active findings or unresolved inline threads. Based on the reported green CI and full-suite results, I have no further review concerns.