fix(sdcard): a blank file-derived string is a gap, not an answer, in the SD card config merge - #627
Conversation
… merge
A truncated SD card log header writes a field's label with no value after
it ("# Serial Number:"), which parses to an empty string rather than null.
SdCardConfigurationMerge treated that as a stated value, so it shadowed the
connected device's real serial or part number and the session reported "".
Blank now counts as a gap, matching what the numeric fields already do with
zero and what the binary parser's own merge already does with an empty part
number or firmware revision. The list fields keep the plain null test.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/agentic_review |
PR Summary by QodoFix SD card config merge: treat blank strings as missing metadata
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Qodo review: the merge normalized blanks only on the parsed side, so a blank on the override side still counted as a stated value and could leak "" into the merged config. That side is if anything the more common one -- DeviceMetadata initializes its strings to string.Empty, so a device that has not reported a firmware revision hands FromDevice an empty one. Both sides now go through Stated(), and the both-blank test sets blanks on the override rather than nulls, which is what a real snapshot looks like. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Round 1 finding "Blank override not normalized" is correct and is fixed in fff5b62. |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit fff5b62 |
|
Round 2 triage on fff5b62 — finding 1 resolved as noted. Pushing back on both new ones. 2. "Blank gap ignored w/o override" — declining. The gap rules here are about precedence, not about normalizing parser output, and the numeric rule this PR was written to match behaves identically: with a null override, a parsed It also breaks a deliberate contract: On the "two absence representations" point: the PR body's line about one absence to check describes the merged-with-override result, which is the path #618 is about. Normalizing a blank when there is nothing to fill it with is the issue's own stated alternative — "normalize empty metadata to 3. "Unresolved cref breaks build" — factually incorrect. C# cref resolution walks enclosing namespaces, so |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit fff5b62 |
|
Qodo-clean (settle re-check confirmed), CI green — ready for review. Re-checked both surfaces on |
AsSampleSource coalesced the log's serial on null alone, so a log whose status message carries an empty or whitespace serial shadowed a perfectly good serial from the connected device and every channel key came out as Daqifi:unknown:*. Blank is a gap here exactly as it is in the SD-card configuration merge (#627), so the fallback now applies to both null and blank. Found by Qodo on PR #668. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
What was wrong
If an SD card log's header line states a field's label but no value —
# Serial Number:with nothing after it, from a truncated or partially written file — the CSV parser produces an empty string for that field, notnull.SdCardConfigurationMergeonly treatednullas "the file didn't say", so the empty string counted as a real answer and won over the override.The visible effect: parse that log with a connected device's snapshot in
SdCardParseOptions.ConfigurationOverride(fromSdCardDeviceConfiguration.FromDevice, which knows the real serial), andsession.DeviceConfig.DeviceSerialNumberstill comes back as"". The device's answer was available and was thrown away. Same forDevicePartNumbervia# Device:.The merge already had the right instinct for numbers —
AnalogPortCountuses> 0, so a zero port count is a gap the device fills. The strings just used??, so onlynullwas a gap. That is the inconsistency.How it was fixed
A blank string now counts as a gap, on the same footing as a non-positive number.
DeviceSerialNumber,DevicePartNumberandFirmwareRevisionroute through a smallStated()helper that maps null-or-whitespace tonull, on both sides of the??.Two judgement calls a reviewer should weigh:
Whitespace, not just empty.
string.IsNullOrWhiteSpace, so" "is a gap too. The CSV parser trims, so only""is reachable today; whitespace is included because a label with nothing but spaces after it says exactly as much as one with nothing at all. Note this is slightly wider than the neighbouring precedent — the binary parser'sMergeConfigurationsusesIsNullOrEmptyfor the same two fields. That precedent is the reason I'm confident "empty means absent" is the intended reading here; I took the whitespace-tolerant version of it rather than copying it exactly.A blank with nothing to fall back on now reports
null. If neither side states the field, the result isnullwhere it used to be"". That is the one behaviour change beyond the bug itself, and it applies to the override side too — which is the more common source of blanks, sinceDeviceMetadatainitializesSerialNumber,PartNumberandFirmwareVersiontostring.Empty, soFromDeviceon a device that hasn't reported a firmware revision yet carries an empty one. Normalizing onnullmatches what the record documents as absent (string? DeviceSerialNumber— "Device serial number, if present"), so callers have one absence to check instead of two.(This second half — normalizing the override side — came out of the first Qodo round, which correctly spotted that the original push normalized only the parsed side while a test comment claimed otherwise.)
Scope kept narrow on the lists. The issue also suggests giving
CalibrationValues/PortRange/InternalScaleMaCount > 0test. I left them on??. Both text parsers pass those in as literalnull, so an empty-but-non-null list cannot reach this merge — the change would be untriggerable, and "an explicitly empty channel list" is a different question from "a blank label" that's better answered when a parser can actually produce one. The reasoning is recorded in the method's<remarks>rather than left implicit.Verification
Five new assertions, all confirmed failing against the pre-fix implementation (stashed the source change, re-ran: 5 failed; restored, 17 passed):
SdCardConfigurationMergeTests— blank strings (""," ","\t") let the override through; blank-on-both-sides (with real blanks on the override, the shapeFromDeviceactually produces) reportsnull; a blank on the override side does not clobber a real file value.SdCardCsvFileParserTests.ParseAsync_ConfigurationOverride_FillsHeaderLabelsThatStateNoValue— the end-to-end shape from the issue: a CSV whose# Device:and# Serial Number:lines are label-only, parsed with a device override, now reports the device's metadata.Full suite green on net9.0 and net10.0: 3772 passed, 2 skipped, 0 failed on each.
closes #618
Not merging — for review.
🤖 Generated with Claude Code