fix(logging-export): CSV escaping, delimiter validation, ticks bounds, no-op progress by cptkoolbeenz · Pull Request #194 · daqifi/daqifi-core

cptkoolbeenz · 2026-05-12T01:34:41Z

Summary

Three follow-ups to PR #167, all flagged by Qodo on the parallel daqifi-python-core port (PR #103) and verified equivalent in Python before being ported here.

Closes fix(csv-export): always finalize progress on no-op export (PR #167 follow-up) #191 — finalize progress on no-op (empty-channels) export.
Closes fix(csv-export): FormatTimestamp marks DateTime.MinValue (ticks=0) as INVALID (PR #167 follow-up) #192 — FormatTimestamp no longer rejects ticks=0 (DateTime.MinValue) as INVALID; only negative ticks are.
Closes fix(logging-export): escape CSV header fields against delimiter, quote, newline, and formula injection #193 — RFC 4180 escaping with EscapeCsvField helper applied to header AND data rows; delimiter validation; formula-injection mitigation scoped to header fields only (so legit negative numbers don't get clobbered).

Test plan

13 new tests across header escaping (delimiter / quote / formula-leading char / whitespace-prefixed formula), data-row escaping (: quotes ISO timestamp, . quotes relative time + values), negative-value regression, invalid-delimiter Theory (empty / multi-char / CR / LF / "), and no-op progress finalization.
2 prior tests updated to assert the new correct behavior for ticks=0.
Full suite: 905 pass, 0 fail, 2 skipped on both net9.0 and net10.0 (was 890 + 2 failing pre-fix).

Refs

…, no-op progress Bundles three follow-ups to PR #167 — all flagged by Qodo on the parallel daqifi-python-core port (PR #103) and verified equivalent in Python before being ported here. Closes #191 (no-op progress finalization): - Empty-channel early return now calls progress?.Report(100) so callers (UI progress bars) don't stall at <100% on no-op exports. Closes #192 (DateTime.MinValue marked INVALID): - FormatTimestamp's `ticks <= 0` check rejected ticks=0 even though it's a legal DateTime value (DateTime.MinValue = 0001-01-01 00:00:00). Tightened to `ticks < 0`. Existing test Export_ZeroTicks_WritesInvalidToken updated to assert the new correct format ("0001-01-01T00:00:00..."); the relative-time invalid-tick test switched to a negative tick (-1) since 0 now formats as "0.000". Closes #193 (CSV escaping + delimiter validation, 5-part fix): 1. New EscapeCsvField helper with formulaSafe parameter — RFC 4180 quoting for fields containing the delimiter / `"` / CR / LF, plus formula-injection mitigation that prefixes `'` to fields whose first non-whitespace character is `=`/`+`/`-`/`@`. 2. Delimiter validation rejects empty / multi-char / CR / LF / double-quote (`"` is reserved as the quoting char so allowing it as a delimiter would produce ambiguous CSV). 3. Header writes use formulaSafe=true (channel keys are user- controllable; spreadsheet apps would evaluate "=DevA:..." as a formula on open). 4. Whitespace-prefixed formula chars caught via TrimStart before the first-char check (" =SUM(A1)" still evaluates in spreadsheets, so naive value[0] checks bypass). 5. Data-row writes (timestamps, values) pass formulaSafe=false: internal output, so RFC 4180 quoting still applies (necessary when ':' or '.' is the delimiter and timestamps/floats contain them) but formula mitigation is OFF — otherwise legit negative numbers like "-1.5" would be clobbered into "'-1.5". 13 new tests cover header escaping (delimiter, quote, formula-leading char, whitespace-prefixed formula), data-row escaping (':' delimiter quotes ISO timestamp, '.' delimiter quotes relative time + value), negative-value regression (no leading- apostrophe), invalid-delimiter validation (5 cases parametrized via Theory), and no-op progress finalization. 905 total tests pass on net9.0 + net10.0 (was 892 with 2 failing on the prior buggy assertions).

qodo-code-review · 2026-05-12T01:34:58Z

Review Summary by Qodo

Fix CSV export escaping, delimiter validation, ticks bounds, and progress finalization

🐞 Bug fix ✨ Enhancement 🧪 Tests

Walkthroughs

Description

• Fix DateTime.MinValue (ticks=0) incorrectly marked as INVALID; only negative ticks are invalid
• Implement RFC 4180 CSV escaping with formula-injection mitigation for headers
• Add delimiter validation to reject empty, multi-char, CR, LF, and double-quote characters
• Finalize progress reporting (100%) on no-op exports to prevent UI stalls
• Add 13 new tests covering CSV escaping, delimiter validation, and edge cases

Diagram

flowchart LR
  A["CSV Export"] --> B["Delimiter Validation"]
  A --> C["Timestamp Formatting"]
  A --> D["Field Escaping"]
  B --> E["Reject Invalid Delimiters"]
  C --> F["Handle ticks=0 as Valid"]
  D --> G["RFC 4180 Quoting"]
  D --> H["Formula Injection Mitigation"]
  H --> I["Headers: formulaSafe=true"]
  H --> J["Data: formulaSafe=false"]
  A --> K["Progress Reporting"]
  K --> L["Report 100% on No-op"]

File Changes

1. src/Daqifi.Core.Tests/Logging/Export/CsvExporterTests.cs 🧪 Tests +133/-5

Add comprehensive CSV escaping and bounds tests

• Updated Export_ZeroTicks_WritesInvalidToken test to verify ticks=0 formats as DateTime.MinValue
 (0001-01-01T00:00:00) instead of INVALID
• Renamed and modified Export_RelativeTime_InvalidTicks_StillWritesInvalidToken to use negative
 ticks (-1) instead of 0
• Added 13 new tests covering: no-op progress finalization, header escaping (delimiter/quote/formula
 chars/whitespace), data-row escaping (timestamps/values), negative value regression, and invalid
 delimiter validation (empty/multi-char/CR/LF/double-quote)

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

2. src/Daqifi.Core/Logging/Export/CsvExporter.cs Bug fix, enhancement +83/-8

Implement CSV escaping, validation, and progress fixes

• Added delimiter validation in ExportAsync to reject empty, multi-character, CR, LF, and
 double-quote delimiters
• Fixed FormatTimestamp condition from ticks <= 0 to ticks < 0 to treat DateTime.MinValue
 (ticks=0) as valid
• Implemented new EscapeCsvField helper method with RFC 4180 quoting and optional
 formula-injection mitigation via apostrophe prefix
• Applied EscapeCsvField to header fields (channel keys) with formulaSafe=true to neutralize
 formula-leading characters
• Applied EscapeCsvField to data fields (timestamps and values) with formulaSafe=false to
 preserve legitimate negative numbers
• Added progress reporting (100%) on no-op exports when channel list is empty

src/Daqifi.Core/Logging/Export/CsvExporter.cs

qodo-code-review · 2026-05-12T01:34:59Z

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

1. ~~Data fields skip formula mitigation~~ ✓ Resolved 📎 Requirement gap ⛨ Security

Description

EscapeCsvField is invoked with formulaSafe: false for timestamp and numeric data fields, so
values beginning with =, +, -, or @ would not be prefixed with a leading apostrophe. This
fails the checklist requirement to mitigate CSV formula injection on every header and data field.

Code

src/Daqifi.Core/Logging/Export/CsvExporter.cs[R139-147]

+        // Data fields use formulaSafe=false: timestamps and numeric values are
+        // internally generated; their leading '-' is a sign on negative numbers,
+        // not a formula char.
+        sb.Append(EscapeCsvField(
+            FormatTimestamp(ticks, firstTicks, options.UseRelativeTime),
+            options.Delimiter, formulaSafe: false));

        var lookup = new Dictionary<string, double>(bucket.Count);
        foreach (var row in bucket)

Evidence
PR Compliance ID 2 requires formula-injection mitigation (leading apostrophe) for every header and
data field. In the modified exporter, data fields explicitly pass formulaSafe: false when escaping
the timestamp and numeric values, disabling the required mitigation on row fields.
CsvExporter must escape all CSV fields (headers and rows) for delimiter/quotes/newlines and mitigate formula injection
src/Daqifi.Core/Logging/Export/CsvExporter.cs[139-156]

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 compliance checklist requires formula-injection mitigation for *every* CSV field (headers and data rows). The new code disables this mitigation for data fields by calling `EscapeCsvField(..., formulaSafe: false)` for timestamps and numeric values.

## Issue Context
Rule PR Compliance ID 2 requires prefixing a leading apostrophe for any field starting with `=`, `+`, `-`, or `@`, even in data rows.

## Fix Focus Areas
- src/Daqifi.Core/Logging/Export/CsvExporter.cs[136-156]
- src/Daqifi.Core/Logging/Export/CsvExporter.cs[228-239]

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

2. ~~Delimiter error detail misleading~~ ✓ Resolved 🐞 Bug ◔ Observability

Description

For invalid multi-character delimiters, the exception detail string reports only the first character
code point (e.g., ",," becomes "U+002C"), which can mislead debugging/troubleshooting. This happens
because got is derived from options.Delimiter[0] regardless of delimiter length > 1.

Code

src/Daqifi.Core/Logging/Export/CsvExporter.cs[R53-57]

+            var got = options.Delimiter == null
+                ? "null"
+                : options.Delimiter.Length == 0
+                    ? "empty"
+                    : $"U+{(int)options.Delimiter[0]:X4}";

Evidence
The got computation uses only options.Delimiter[0], so any invalid delimiter with length > 1
will be reported as the code point of its first character only; the test suite includes a
multi-character delimiter (",") in the invalid-delimiter theory, which would trigger this path.
src/Daqifi.Core/Logging/Export/CsvExporter.cs[44-60]
src/Daqifi.Core.Tests/Logging/Export/CsvExporterTests.cs[674-687]

Agent prompt

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

### Issue description
`CsvExporter.ExportAsync` throws an `ArgumentException` for invalid delimiters, but the `(got ...)` diagnostic only reports the first UTF-16 code unit (`options.Delimiter[0]`). For multi-character delimiters, this can misrepresent what was actually provided (e.g., a two-character string).

### Issue Context
The code intentionally avoids embedding raw delimiter text in the exception message (to prevent CR/LF from breaking logs), which is good. The diagnostic can still be made accurate while remaining log-safe by including the delimiter length and/or listing all characters as code points.

### Fix Focus Areas
- src/Daqifi.Core/Logging/Export/CsvExporter.cs[53-57]
- src/Daqifi.Core/Logging/Export/CsvExporter.cs[59-59]

### Suggested fix shape
- If `options.Delimiter` is `null`/empty: keep current `null`/`empty` handling.
- Else: include `Length` and all characters as `U+XXXX` (or `U+XXXXX` for scalars if you choose to enumerate runes), e.g. `len=2 [U+002C U+002C]`.
- Update the exception message to use the improved diagnostic string.

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

cptkoolbeenz · 2026-05-12T02:04:07Z

Qodo finding "Data fields skip formula mitigation" — this is intentional and is the resolution of fifth follow-up on #193 from the Python port (daqifi-python-core PR #105 pass 8).

Rationale: applying formula mitigation to data fields would clobber legitimate negative numeric values. `FormatValue(-1.5)` returns `"-1.5"`; with `formulaSafe: true` that would become `"'-1.5"` in the CSV — breaking any consumer that expects numeric tokens.

The split is header = formulaSafe=true (channel keys are user-controllable strings like `=DevA:...` that Excel would evaluate); data = formulaSafe=false (timestamps from `FormatTimestamp` and numbers from `G` formatting are internally generated; their leading `-` is a sign, not a formula char).

The new test `Export_NegativeValue_NotApostrophePrefixed` is the regression coverage for this — pre-fix, my data-row escape applied formulaSafe by default and broke negative numbers. The Qodo /agentic_review on the parallel Python PR caught it; documented as the fifth follow-up on #193.

RFC 4180 quoting (delimiter / `"` / CR / LF) IS still applied to data fields — so timestamps containing `:` or `.` correctly get quoted when those characters are picked as delimiters (`Export_ColonDelimiter_QuotesIsoTimestamp` and `Export_DotDelimiter_QuotesRelativeTimestampAndValue` cover that). Only the formula-injection prefix is scoped header-only.

Code stays as-is.

cptkoolbeenz · 2026-05-12T02:39:22Z

/improve

cptkoolbeenz · 2026-05-12T02:39:23Z

/agentic_review

qodo-code-review · 2026-05-12T02:39:29Z

Persistent review updated to latest commit 8a0df25

qodo-code-review · 2026-05-12T02:40:19Z

PR Code Suggestions ✨

Latest suggestions up to 2f77284

Warning

/improve is deprecated. Use /agentic_review instead (removal date not yet scheduled).

Category Suggestion Impact

Possible issue

Prevent null-related escaping crashes

Update EscapeCsvField to handle null values and empty delimiters safely. This
prevents potential exceptions if the method is ever called from paths lacking
prior validation.

src/Daqifi.Core/Logging/Export/CsvExporter.cs [136-184]

-private static string EscapeCsvField(string value, string delimiter, bool formulaSafe = true)
+private static string EscapeCsvField(string? value, string delimiter, bool formulaSafe = true)
 {
-    if (formulaSafe && !string.IsNullOrEmpty(value))
+    value ??= string.Empty;
+
+    if (delimiter.Length == 0)
+        return value;
+
+    if (formulaSafe && value.Length > 0)
     {
         // Skip ALL Unicode whitespace, not just ' ' and '\t'. CSV
         // formula-injection PoCs use NBSP (U+00A0), thin spaces, line
         // separator (U+2028), etc. before '=' to evade trim-based
         // checks; spreadsheets still treat the resulting cell as a
         // formula. char.IsWhiteSpace covers the full Unicode set.
         var i = 0;
         while (i < value.Length && char.IsWhiteSpace(value[i]))
             i++;
         if (i < value.Length && "=+-@".IndexOf(value[i]) >= 0)
         {
             value = "'" + value;
         }
     }
 
     var delimChar = delimiter[0];
     var mustQuote = false;
 
     // Quote fields with leading or trailing whitespace — many CSV
     // parsers (Excel, Google Sheets, pandas with default options) trim
     // unquoted whitespace and silently lose it. Quoting preserves the
     // exact value through round-trip.
     if (value.Length > 0 && (char.IsWhiteSpace(value[0]) || char.IsWhiteSpace(value[^1])))
     {
         mustQuote = true;
     }
 
     if (!mustQuote)
     {
         for (var i = 0; i < value.Length; i++)
         {
             var c = value[i];
             if (c == delimChar || c == '"' || c == '\r' || c == '\n')
             {
                 mustQuote = true;
                 break;
             }
         }
     }
 
     if (mustQuote)
     {
         return "\"" + value.Replace("\"", "\"\"") + "\"";
     }
     return value;
 }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 3

__

Why: While adding null-safety is a reasonable defensive practice, this is a private method where inputs are already strictly controlled and validated earlier in the flow, making the practical impact of this change marginal.

Low

More

Previous suggestions

Suggestions up to commit 6da7050

Category Suggestion Impact

Possible issue

Prevent null field escaping crashes

Update EscapeCsvField to accept a nullable string and treat null as an empty
string.

src/Daqifi.Core/Logging/Export/CsvExporter.cs [118-166]

-private static string EscapeCsvField(string value, string delimiter, bool formulaSafe = true)
+private static string EscapeCsvField(string? value, string delimiter, bool formulaSafe = true)
 {
-    if (formulaSafe && !string.IsNullOrEmpty(value))
+    value ??= string.Empty;
+
+    if (formulaSafe && value.Length != 0)
     {
         // Skip ALL Unicode whitespace, not just ' ' and '\t'. CSV
         // formula-injection PoCs use NBSP (U+00A0), thin spaces, line
         // separator (U+2028), etc. before '=' to evade trim-based
         // checks; spreadsheets still treat the resulting cell as a
         // formula. char.IsWhiteSpace covers the full Unicode set.
         var i = 0;
         while (i < value.Length && char.IsWhiteSpace(value[i]))
             i++;
         if (i < value.Length && "=+-@".IndexOf(value[i]) >= 0)
         {
             value = "'" + value;
         }
     }
 
     var delimChar = delimiter[0];
     var mustQuote = false;
 
     // Quote fields with leading or trailing whitespace — many CSV
     // parsers (Excel, Google Sheets, pandas with default options) trim
     // unquoted whitespace and silently lose it. Quoting preserves the
     // exact value through round-trip.
     if (value.Length > 0 && (char.IsWhiteSpace(value[0]) || char.IsWhiteSpace(value[^1])))
     {
         mustQuote = true;
     }
 
     if (!mustQuote)
     {
         for (var i = 0; i < value.Length; i++)
         {
             var c = value[i];
             if (c == delimChar || c == '"' || c == '\r' || c == '\n')
             {
                 mustQuote = true;
                 break;
             }
         }
     }
 
     if (mustQuote)
     {
         return "\"" + value.Replace("\"", "\"\"") + "\"";
     }
     return value;
 }

Suggestion importance[1-10]: 5

__

Why: While all current callers of this private method pass non-null strings, adding null coalescing provides defensive programming and prevents potential future NullReferenceExceptions.

Low

Suggestions up to commit 904746b

Category	Suggestion	Impact
General	Make delimiter errors diagnosable Format the invalid delimiter as a Unicode code point or descriptive text in the exception message rather than interpolating it directly. src/Daqifi.Core/Logging/Export/CsvExporter.cs [8-17] if (string.IsNullOrEmpty(options.Delimiter) \|\| options.Delimiter.Length != 1 \|\| options.Delimiter == "\"" \|\| options.Delimiter == "\r" \|\| options.Delimiter == "\n") { + var got = options.Delimiter == null + ? "null" + : options.Delimiter.Length == 0 + ? "empty" + : $"U+{(int)options.Delimiter[0]:X4}"; + throw new ArgumentException( - $"Delimiter must be a single character that is not a newline or double-quote (got '{options.Delimiter}').", + $"Delimiter must be a single character that is not a newline or double-quote (got {got}).", $"{nameof(options)}.{nameof(CsvExportOptions.Delimiter)}"); } Suggestion importance[1-10]: 4 __ Why: Escaping control characters in the exception message prevents misleading or multi-line logs, making it easier to diagnose invalid delimiters.	Low
Possible issue	Prevent null-related runtime crashes Add null-checking for the `value` parameter and validate the `delimiter` parameter defensively inside the `EscapeCsvField` method. src/Daqifi.Core/Logging/Export/CsvExporter.cs [110-158] -private static string EscapeCsvField(string value, string delimiter, bool formulaSafe = true) +private static string EscapeCsvField(string? value, string delimiter, bool formulaSafe = true) { - if (formulaSafe && !string.IsNullOrEmpty(value)) + if (string.IsNullOrEmpty(delimiter) \|\| delimiter.Length != 1) + throw new ArgumentException("Delimiter must be a single character.", nameof(delimiter)); + + value ??= string.Empty; + + if (formulaSafe && value.Length != 0) { - ... + var i = 0; + while (i < value.Length && char.IsWhiteSpace(value[i])) + i++; + if (i < value.Length && "=+-@".IndexOf(value[i]) >= 0) + { + value = "'" + value; + } } var delimChar = delimiter[0]; var mustQuote = false; - // Quote fields with leading or trailing whitespace — many CSV - // parsers (Excel, Google Sheets, pandas with default options) trim - // unquoted whitespace and silently lose it. Quoting preserves the - // exact value through round-trip. if (value.Length > 0 && (char.IsWhiteSpace(value[0]) \|\| char.IsWhiteSpace(value[^1]))) { mustQuote = true; } if (!mustQuote) { for (var i = 0; i < value.Length; i++) { var c = value[i]; if (c == delimChar \|\| c == '"' \|\| c == '\r' \|\| c == '\n') { mustQuote = true; break; } } } - if (mustQuote) - { - return "\"" + value.Replace("\"", "\"\"") + "\""; - } - return value; + return mustQuote ? "\"" + value.Replace("\"", "\"\"") + "\"" : value; } Suggestion importance[1-10]: 3 __ Why: While defensively preventing null references is good practice, the `EscapeCsvField` method is private, its current callers guarantee non-null values, and the delimiter is already validated at the public entry point.	Low

✅ Suggestions up to commit e656b8c

Category Suggestion Impact

General

Preserve leading/trailing whitespace safely

Update the mustQuote logic to also quote fields that start or end with
whitespace characters.

src/Daqifi.Core/Logging/Export/CsvExporter.cs [129-143]

 var mustQuote = false;
-for (var i = 0; i < value.Length; i++)
+
+if (value.Length > 0 && (char.IsWhiteSpace(value[0]) || char.IsWhiteSpace(value[^1])))
+    mustQuote = true;
+
+if (!mustQuote)
 {
-    var c = value[i];
-    if (c == delimChar || c == '"' || c == '\r' || c == '\n')
+    for (var i = 0; i < value.Length; i++)
     {
-        mustQuote = true;
-        break;
+        var c = value[i];
+        if (c == delimChar || c == '"' || c == '\r' || c == '\n')
+        {
+            mustQuote = true;
+            break;
+        }
     }
 }
+
 if (mustQuote)
 {
     return "\"" + value.Replace("\"", "\"\"") + "\"";
 }
 return value;

Suggestion importance[1-10]: 5

__

Why: Quoting fields with leading or trailing whitespace improves the robustness of the CSV export, as some external parsers may unintentionally trim unquoted whitespace.

Low

✅ Suggestions up to commit e656b8c

Category	Suggestion	Impact
Possible issue	✅ ~~Guard against null and invalid inputs~~ Suggestion Impact: Instead of adding delimiter validation/null-handling inside EscapeCsvField, the commit strengthened handling of invalid delimiter inputs at the public validation point by formatting the bad delimiter safely (including null/empty/multi-char and code points) in the exception message. It also adjusted CSV quoting behavior for leading/trailing whitespace, but did not implement the suggested null/delimiter checks inside EscapeCsvField. code diff: @@ -47,8 +48,29 @@ \|\| options.Delimiter == "\r" \|\| options.Delimiter == "\n") { + // Format the bad value as code points so a stray '\r' or '\n' + // in the delimiter doesn't break the exception message into + // multiple log lines. Enumerate ALL chars (not just [0]) so + // multi-character delimiters like ",," aren't misreported as a + // single character. + string got; + if (options.Delimiter == null) + { + got = "null"; + } + else if (options.Delimiter.Length == 0) + { + got = "empty"; + } + else + { + var codePoints = string.Join( + " ", + options.Delimiter.Select(c => $"U+{(int)c:X4}")); + got = $"len={options.Delimiter.Length} [{codePoints}]"; + } throw new ArgumentException( - $"Delimiter must be a single character that is not a newline or double-quote (got '{options.Delimiter}').", + $"Delimiter must be a single character that is not a newline or double-quote (got {got}).", $"{nameof(options)}.{nameof(CsvExportOptions.Delimiter)}"); } @@ -293,15 +315,29 @@ var delimChar = delimiter[0]; var mustQuote = false; - for (var i = 0; i < value.Length; i++) - { - var c = value[i]; - if (c == delimChar \|\| c == '"' \|\| c == '\r' \|\| c == '\n') - { - mustQuote = true; - break; - } - } + + // Quote fields with leading or trailing whitespace — many CSV + // parsers (Excel, Google Sheets, pandas with default options) trim + // unquoted whitespace and silently lose it. Quoting preserves the + // exact value through round-trip. + if (value.Length > 0 && (char.IsWhiteSpace(value[0]) \|\| char.IsWhiteSpace(value[^1]))) + { + mustQuote = true; + } + + if (!mustQuote) + { + for (var i = 0; i < value.Length; i++) + { + var c = value[i]; + if (c == delimChar \|\| c == '"' \|\| c == '\r' \|\| c == '\n') + { + mustQuote = true; + break; + } + } + } + if (mustQuote) { return "\"" + value.Replace("\"", "\"\"") + "\""; Add null checks for `value` and length validation for `delimiter` in `EscapeCsvField`. src/Daqifi.Core/Logging/Export/CsvExporter.cs [110-144] -private static string EscapeCsvField(string value, string delimiter, bool formulaSafe = true) +private static string EscapeCsvField(string? value, string delimiter, bool formulaSafe = true) { - if (formulaSafe && !string.IsNullOrEmpty(value)) + value ??= string.Empty; + + if (string.IsNullOrEmpty(delimiter) \|\| delimiter.Length != 1) + throw new ArgumentException("Delimiter must be a single character.", nameof(delimiter)); + + if (formulaSafe && value.Length > 0) { - ... + var i = 0; + while (i < value.Length && char.IsWhiteSpace(value[i])) + i++; + if (i < value.Length && "=+-@".IndexOf(value[i]) >= 0) + { + value = "'" + value; + } } var delimChar = delimiter[0]; var mustQuote = false; for (var i = 0; i < value.Length; i++) { var c = value[i]; if (c == delimChar \|\| c == '"' \|\| c == '\r' \|\| c == '\n') { mustQuote = true; break; } } - if (mustQuote) - { - return "\"" + value.Replace("\"", "\"\"") + "\""; - } - return value; + + return mustQuote + ? "\"" + value.Replace("\"", "\"\"") + "\"" + : value; } Suggestion importance[1-10]: 2 __ Why: The `delimiter` is already strictly validated in the public entry point `ExportAsync`, making redundant checks in this `private` helper unnecessary.	Low
Possible issue	Avoid reporting completion after cancellation Add a cancellation token check right before reporting 100% progress for empty channels. src/Daqifi.Core/Logging/Export/CsvExporter.cs [21-28] var channels = source.GetChannels(); if (channels.Count == 0) { // Always finalize progress so callers (e.g. UI progress bars) don't // stall at <100% when the export is a no-op. + cancellationToken.ThrowIfCancellationRequested(); progress?.Report(100); return; } Suggestion importance[1-10]: 2 __ Why: The execution window between the initial cancellation check and this block is negligible, making an additional check overly pedantic and practically unnecessary.	Low

✅ Suggestions up to commit 8a0df25

Category	Suggestion	Impact
Security	✅ ~~Harden spreadsheet formula neutralization~~ Suggestion Impact: Replaced TrimStart(' ', '\t') with a loop that skips all Unicode whitespace using char.IsWhiteSpace before checking for "=+-@" to prevent CSV formula injection bypasses. code diff: if (formulaSafe && !string.IsNullOrEmpty(value)) { - var leading = value.TrimStart(' ', '\t'); - if (leading.Length > 0 && "=+-@".IndexOf(leading[0]) >= 0) + // Skip ALL Unicode whitespace, not just ' ' and '\t'. CSV + // formula-injection PoCs use NBSP (U+00A0), thin spaces, line + // separator (U+2028), etc. before '=' to evade trim-based + // checks; spreadsheets still treat the resulting cell as a + // formula. char.IsWhiteSpace covers the full Unicode set. + var i = 0; + while (i < value.Length && char.IsWhiteSpace(value[i])) + i++; + if (i < value.Length && "=+-@".IndexOf(value[i]) >= 0) { value = "'" + value; } Enhance CSV formula neutralization by skipping all Unicode whitespace characters instead of just spaces and tabs. src/Daqifi.Core/Logging/Export/CsvExporter.cs [112-119] -if (formulaSafe && !string.IsNullOrEmpty(value)) +if (formulaSafe && value.Length > 0) { - var leading = value.TrimStart(' ', '\t'); - if (leading.Length > 0 && "=+-@".IndexOf(leading[0]) >= 0) + var i = 0; + while (i < value.Length && char.IsWhiteSpace(value[i])) + i++; + + if (i < value.Length && "=+-@".IndexOf(value[i]) >= 0) { value = "'" + value; } } Suggestion importance[1-10]: 7 __ Why: Using `char.IsWhiteSpace` hardens the application against CSV formula injection vectors that exploit non-standard whitespace characters.	Medium
Possible issue	Handle null CSV field values Gracefully handle null values in `EscapeCsvField` by coalescing them to an empty string. src/Daqifi.Core/Logging/Export/CsvExporter.cs [110-137] -private static string EscapeCsvField(string value, string delimiter, bool formulaSafe = true) +private static string EscapeCsvField(string? value, string delimiter, bool formulaSafe = true) { - if (formulaSafe && !string.IsNullOrEmpty(value)) + value ??= string.Empty; + + if (formulaSafe && value.Length > 0) { var leading = value.TrimStart(' ', '\t'); if (leading.Length > 0 && "=+-@".IndexOf(leading[0]) >= 0) { value = "'" + value; } } var delimChar = delimiter[0]; var mustQuote = false; for (var i = 0; i < value.Length; i++) { var c = value[i]; if (c == delimChar \|\| c == '"' \|\| c == '\r' \|\| c == '\n') { mustQuote = true; break; } } if (mustQuote) { return "\"" + value.Replace("\"", "\"\"") + "\""; } return value; } Suggestion importance[1-10]: 6 __ Why: While current callers do not pass null, the method's initial partial null-check suggests ambiguity; explicitly handling nulls improves the exporter's resilience against future regressions.	Low

…against Unicode whitespace Qodo flagged that the formula-injection check trimmed only ' ' and '\t' before testing for the leading '=', '+', '-', '@'. CSV formula-injection PoCs use NBSP (U+00A0), thin spaces, and Unicode line separators (U+2028) ahead of '=' to evade trim-based checks — spreadsheets still treat the resulting cell as a formula because they normalize / ignore the leading whitespace. Switched the skip loop to char.IsWhiteSpace, which covers the full Unicode whitespace set, closing that bypass class. Skipped a second suggestion to coalesce null inputs to empty — EscapeCsvField is private and current callers always pass strings; adding null tolerance for an impossible case violates the "only validate at boundaries" rule.

cptkoolbeenz · 2026-05-12T02:42:08Z

/improve

cptkoolbeenz · 2026-05-12T02:42:09Z

/agentic_review

qodo-code-review · 2026-05-12T02:42:15Z

Persistent review updated to latest commit b1e27f2

qodo-code-review · 2026-05-12T02:43:13Z

Persistent suggestions updated to latest commit b1e27f2

#194 follow-up) Locks in the char.IsWhiteSpace upgrade from the previous commit. NBSP (U+00A0) and EM SPACE (U+2003) before '=' previously slipped past the trim-only check; the regression test confirms both now get the leading apostrophe. U+2028 LINE SEPARATOR — also covered by the runtime fix — is omitted from the test data because C# treats raw U+2028 as a source-file newline even inside string literals; the runtime behavior is identical to the other whitespace cases since char.IsWhiteSpace covers U+2028.

cptkoolbeenz · 2026-05-12T02:47:56Z

/improve

cptkoolbeenz · 2026-05-12T02:47:57Z

/agentic_review

qodo-code-review · 2026-05-12T02:48:09Z

Persistent review updated to latest commit e656b8c

qodo-code-review · 2026-05-12T02:48:48Z

Persistent suggestions updated to latest commit e656b8c

cptkoolbeenz · 2026-05-12T02:51:12Z

Convergence summary (Qodo /agentic_review pass 3):

/improve: 2 new suggestions both importance 2/10 (Qodo's own rationale: "redundant" / "overly pedantic"). Both add defensive validation against impossible cases (null inputs to a private helper, redundant cancellation check 2 statements after the existing one). Skipped per the project's no-defensive-validation rule.
/agentic_review: counter row 0 / 0 / 1. The remaining gap is "Data fields skip formula mitigation" — the intentional formulaSafe: false for internally-generated numeric / timestamp fields. Resolution path: ticket fix(logging-export): escape CSV header fields against delimiter, quote, newline, and formula injection #193 has been edited with an explicit scope carve-out documenting why double.ToString("G") output (digits, sign, exponent, decimal point) cannot contain attacker-controlled formula prefixes, and why mitigating leading - would corrupt negative numbers. Qodo doesn't appear to re-fetch the ticket text on subsequent reviews, so the finding persists in the bot output despite the resolution. Documented for the human reviewer.

Test gate: 905 pass / 2 skipped (real-hardware) / 0 fail; CI build green.

Ready for human review.

cptkoolbeenz · 2026-05-12T07:10:47Z

/improve

cptkoolbeenz · 2026-05-12T07:10:49Z

/agentic_review

qodo-code-review · 2026-05-12T07:10:56Z

Persistent review updated to latest commit e656b8c

qodo-code-review · 2026-05-12T07:11:57Z

Persistent suggestions updated to latest commit e656b8c

Excel, Google Sheets, and pandas (with default options) trim unquoted leading/trailing whitespace in CSV fields, silently losing the exact value through round-trip. Detect and quote those fields up front so the round-trip preserves the value verbatim.

cptkoolbeenz · 2026-05-12T07:15:16Z

/improve

cptkoolbeenz · 2026-05-12T07:15:17Z

/agentic_review

qodo-code-review · 2026-05-12T07:15:24Z

Persistent review updated to latest commit 904746b

qodo-code-review · 2026-05-12T07:16:22Z

Persistent suggestions updated to latest commit 904746b

When the delimiter is '\r' or '\n', interpolating it raw into the ArgumentException message produced a multi-line, hard-to-grep error log. Format the bad value as 'null' / 'empty' / 'U+XXXX' so the message stays a single line and unambiguously identifies the bad character.

cptkoolbeenz · 2026-05-12T07:18:06Z

/improve

cptkoolbeenz · 2026-05-12T07:18:07Z

/agentic_review

qodo-code-review · 2026-05-12T07:18:14Z

Persistent review updated to latest commit 6da7050

qodo-code-review · 2026-05-12T07:18:46Z

Persistent suggestions updated to latest commit 6da7050

Enumerate every code point of the invalid delimiter (with its length) in the ArgumentException detail string instead of reporting only the first character. Multi-character delimiters like ",," were misreported as a single "U+002C" — the new format reads len=2 [U+002C U+002C] and stays log-safe (no raw CR/LF interpolated).

cptkoolbeenz · 2026-05-12T19:04:46Z

/improve

cptkoolbeenz · 2026-05-12T19:04:47Z

/agentic_review

qodo-code-review · 2026-05-12T19:04:54Z

Persistent review updated to latest commit 2f77284

qodo-code-review · 2026-05-12T19:05:38Z

Persistent suggestions updated to latest commit 2f77284

tylerkron · 2026-05-13T14:31:13Z

Live-device smoke test

Built the core example app against this PR (project reference, not the 0.20.0 NuGet) and exercised the end-to-end SD download → SdCardFileParser → CsvExporter path against a real Nq1 (FW 3.4.4, SN 9090539562006014104) over serial at /dev/cu.usbmodem101.

Setup

dotnet build Daqifi.Core.Cli.csproj \
  -p:DaqifiCoreProjectPath=.../Daqifi.Core.csproj -c Release

# Step 1: download an existing protobuf log from SD
dotnet run ... -- --serial /dev/cu.usbmodem101 \
  --sd-download log_20260502_125152.bin

# Step 2: parse + export via Daqifi.Core.CsvExporter
dotnet run ... -- --sd-parse sd_log.bin \
  --sd-export-csv out.csv

Result

Export succeeded: 3,908 incoming samples → 1,955 CSV lines (1 header + 1,954 timestamp-bucketed rows × 2 channel columns), 64,390 bytes in 0.01s — 313K rows/s, 4.9 MB/s.

Time,Daqifi:unknown:AI0,Daqifi:unknown:DIO
2026-05-13T14:29:46.9229570,0,0
2026-05-13T14:29:46.9279591,0,0
...

Verifications

Check	Result	Notes
Header unquoted	✓	`:` in channel keys doesn't trigger escaping under default `,` delimiter — confirms `EscapeCsvField` only fires when needed
`INVALID` tokens	0	Real device timestamps don't trigger the bounds check; ticks=0 fix doesn't false-positive
Lines with `'` formula prefix	0	Channel keys (`Daqifi:unknown:AI0`) don't start with `=/+/-/@`, security prefix correctly stays dormant
Spurious quoting	0	Per-field escape path is allocation-free on real data
Throughput	313K rows/s	The per-field `EscapeCsvField` overhead is empirically a non-issue

What this didn't exercise

Couldn't directly hit these on real-device data — they're properly covered by the 13 new unit tests in this PR:

Negative-value regression — captured dataset has values 0.0 → 0.887, no negatives to confirm -1.5 doesn't become '-1.5.
Header escaping for special chars — real firmware doesn't emit channel names containing , " CR LF or formula chars.
Delimiter validation — example CLI hard-codes ,.

Bottom line

PR #194 does not regress the live SD → CSV export path on real hardware. LGTM.

cptkoolbeenz requested a review from a team as a code owner May 12, 2026 01:34

qodo-code-review Bot reviewed May 12, 2026

View reviewed changes

Comment thread src/Daqifi.Core/Logging/Export/CsvExporter.cs

tylerkron merged commit 4f3766f into main May 13, 2026
1 check passed

tylerkron deleted the fix/csv-export-escaping-and-bounds branch May 13, 2026 14:45

Conversation

cptkoolbeenz commented May 12, 2026

Summary

Test plan

Refs

Uh oh!

qodo-code-review Bot commented May 12, 2026

Review Summary by Qodo

Walkthroughs

File Changes

Uh oh!

qodo-code-review Bot commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

Uh oh!

cptkoolbeenz commented May 12, 2026

Uh oh!

cptkoolbeenz commented May 12, 2026

Uh oh!

cptkoolbeenz commented May 12, 2026

Uh oh!

qodo-code-review Bot commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qodo-code-review Bot commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

cptkoolbeenz commented May 12, 2026

Uh oh!

cptkoolbeenz commented May 12, 2026

Uh oh!

qodo-code-review Bot commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qodo-code-review Bot commented May 12, 2026

Uh oh!

cptkoolbeenz commented May 12, 2026

Uh oh!

cptkoolbeenz commented May 12, 2026

Uh oh!

qodo-code-review Bot commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qodo-code-review Bot commented May 12, 2026

Uh oh!

cptkoolbeenz commented May 12, 2026

Uh oh!

cptkoolbeenz commented May 12, 2026

Uh oh!

cptkoolbeenz commented May 12, 2026

Uh oh!

qodo-code-review Bot commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qodo-code-review Bot commented May 12, 2026

Uh oh!

cptkoolbeenz commented May 12, 2026

Uh oh!

cptkoolbeenz commented May 12, 2026

Uh oh!

qodo-code-review Bot commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qodo-code-review Bot commented May 12, 2026

Uh oh!

cptkoolbeenz commented May 12, 2026

Uh oh!

cptkoolbeenz commented May 12, 2026

Uh oh!

qodo-code-review Bot commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qodo-code-review Bot commented May 12, 2026

Uh oh!

cptkoolbeenz commented May 12, 2026

Uh oh!

cptkoolbeenz commented May 12, 2026

qodo-code-review Bot commented May 12, 2026 •

edited

Loading

qodo-code-review Bot commented May 12, 2026 •

edited

Loading

qodo-code-review Bot commented May 12, 2026 •

edited

Loading

qodo-code-review Bot commented May 12, 2026 •

edited

Loading

qodo-code-review Bot commented May 12, 2026 •

edited

Loading

qodo-code-review Bot commented May 12, 2026 •

edited

Loading

qodo-code-review Bot commented May 12, 2026 •

edited

Loading

qodo-code-review Bot commented May 12, 2026 •

edited

Loading

qodo-code-review Bot commented May 12, 2026 •

edited

Loading