Harden never-throw parity and tighten test rigor (audit follow-up)

[khaines]

What & why

Follow-up to the red-team / security / quality audit of the codebase. Addresses the actionable findings that don't change the wire format (control-char escaping is a separate PR). No behavior change except never-throw hardening.

Never-throw completeness (S3)

• Logger.Log(severity, msg, object[]) now contains a throwing value ToString() as a [VALUE ERROR: …] placeholder — parity with the MEL adapter. Null values still render as null.
• ConsoleLogExporter reads exception messages via a throw-safe helper and wraps per-record export so one malformed record can't fail the whole batch.

Docs (S2, G2)

• Document ts/level/msg as reserved fields (a colliding user key produces a duplicate; logfmt permits duplicate keys and typical consumers take the last value).
• Explain why the provider intentionally doesn't dispose its stdout-backed inner loggers (immediate flush → no data loss; closing stdout is undesirable).

Test gaps (G1, G3)

• Quoted value ending in a lone backslash at EOF (parser must not read past the buffer).
• Exporter throwing-Message and null-StackTrace paths.

Vacuous / weak tests fixed (V1, V2, V3)

• TestConsoleLogExporterDispose now asserts the inner stream is closed (was zero assertions).
• Runtime level-lowering test parses the exact msg (was an over-broad Contains("after") that a payload mutation survived).
• TestBeginScopeReturnsDisposable asserts the shared singleton (was NotNull on a never-null value).

Validation

• 195/195 tests on net8.0 and net10.0; dotnet build -c Release → 0 warnings.
• Every new/tightened test is mutation-verified: reverting each production hunk (value try/catch, exporter SafeExceptionMessage, ?? string.Empty stack fallback, parser pos+1<len bound, _loggers.Clear(), exporter _logger.Dispose()) turns its test RED.

Self-authored → informational review; AI does not merge.

[khaines]

🤖 Review-Fix Loop — Final Report

PR: #89 — Harden never-throw parity and tighten test rigor (audit follow-up)
Branch: fix/robustness-and-test-rigor
HEAD SHA: d774c7d
Rounds completed: 3
Final rating: 5/5 ⭐ — Exceptional
Gate: ✅ PASS — unanimous 5/5 across all voting seats + decorrelated red-team NO-MISS-CERTIFIED
Termination reason: Target achieved (PASS gate)

Council run via the deltasharp review-pr / review-fix-loop skills against this repo (logfmt canon substituted for Delta/Spark/K8s). Self-authored PR → informational COMMENT review; AI never merges. This PR itself implements the non-wire-format findings from a red-team/security/quality audit of the library.

Progression

Round	Rating	🔴 Critical	🟠 High	🟡 Medium	🔵 Low	ℹ️ Info	Fixed
R1 (a9f02a8)	3/5 · ❌ red-team MISS	0	0	3	1	0	—
R2 (1441b9e)	5/5 · ❌ red-team MISS	0	0	1	0	1	5
R3 (d774c7d)	5/5 · ✅ NO-MISS	0	0	0	0	0	2

Council Composition Audit

Round	Slot	agent_type	model	Dispatch HEAD	Verification
R1	Architect	general-purpose	claude-opus-4.8	a9f02a8	✓ 4/5
R1	Balanced	general-purpose	claude-opus-4.8	a9f02a8	✓ 5/5
R1	Security	general-purpose	claude-opus-4.8	a9f02a8	✓ 5/5
R1	Quality	general-purpose	gemini-3.1-pro-preview	a9f02a8	✓ 3/5
R1	Red-Team	general-purpose	gpt-5.6-sol	a9f02a8	✓ MISS-FOUND (decorrelated)
R2	Architect	general-purpose	claude-opus-4.8	1441b9e	✓ 5/5
R2	Balanced	general-purpose	claude-opus-4.8	1441b9e	✓ 5/5
R2	Security	general-purpose	claude-opus-4.8	1441b9e	✓ 5/5
R2	Quality	general-purpose	gemini-3.1-pro-preview	1441b9e	✓ 5/5
R2	Red-Team	general-purpose	gpt-5.6-sol	1441b9e	✓ MISS-FOUND (decorrelated)
R3	Quality	general-purpose	gemini-3.1-pro-preview	d774c7d	✓ 5/5
R3	Red-Team	general-purpose	gpt-5.6-sol	d774c7d	✓ NO-MISS-CERTIFIED (decorrelated)

R3 diff is test-only + a corrected comment (no production logic change), so the R2 Architect/Balanced/Security 5/5 carry forward on the unchanged production; the test-rigor Quality seat and the gate-keeping red-team both re-ran at R3 HEAD.

What the PR does

Implements the audit's non-wire-format findings: never-throw parity for the core Log(severity, msg, object[]) overload and the OpenTelemetry ConsoleLogExporter (a throwing ToString()/Exception.Message is contained as a [VALUE ERROR]/[EXPORT ERROR] marker; one bad record can't fail the batch), documents the ts/level/msg reserved-field collision behavior, and closes test gaps + fixes vacuous/weak tests.

Findings Addressed

Round 1 → Round 2

• [Medium, R1 Red-Team] Never-throw hole on a disposed exporter. Export wrote to Console.Error.WriteLine when _isDisposed; a hostile/redirected stderr made it throw. Fixed: the disposed path now just returns ExportResult.Failure (no I/O). Security re-reproduced it closed (hostile stderr → Failure, no throw).
• [Medium, R1 Quality] The [EXPORT ERROR] batch-isolation path had no test. Fixed: added TestOpenTelemetryHostileRecordDoesNotAbortBatch.
• [Medium, R1 Quality] The documented reserved-field duplicate (S2) had no test. Fixed: added UserKeyCollidingWithReservedFieldProducesDuplicate (asserts two msg fields).
• [Low, R1 Architect] SafeExceptionMessage duplicated. Fixed: ExtensionLogger now delegates to the internal Logger.SafeExceptionMessage; the private copy is deleted (mutation-proven still covering the never-throw fallback).
• [Low, R1 Quality] Vacuous/weak tests. TestConsoleLogExporterDefaultConstructor/WithCustomLogger (asserted only NotNull) now assert the Export Success/Failure contract; TestOpenTelemetryIntegration/DifferentLogLevels now parse exact fields (ParseFields) instead of substring Contains.

Round 2 → Round 3

• [Medium, R2 Red-Team] The batch-isolation test used SimpleLogRecordExportProcessor (one record per Export), so it didn't test true single-batch isolation — a mutation returning early after the per-record catch stayed green. Fixed: rewrote it with BatchLogRecordExportProcessor so all three records reach one Export call (read via MemoryStream.ToArray() since flush-on-dispose closes the stream). The early-return mutation now turns it RED on both TFMs.
• [Info, R2 Architect] Inaccurate comment ("ExtractAttributes is already throw-safe" — a hostile Exception.StackTrace can still throw). Fixed: comment corrected to state the outer catch is what upholds the contract.

Findings Dismissed / Deferred / Inherent

• S1 (control-char / Unicode-separator escaping) — deliberately out of scope for this PR; it changes the wire format and lands as a separate follow-up PR.
• [Info, Security] record.Exception.StackTrace is the one exception member not wrapped in a Safe helper — intentionally left to the per-record Export try/catch (now tested), which contains it as [EXPORT ERROR]. Documented, not a hole.

Validation

• Tests: dotnet test 197/197 on net8.0 and 197/197 on net10.0; dotnet build -c Release → 0 warnings (StyleCop-as-lint clean). CI: Analyze C#, CodeQL, build, dependency-review all green.
• Mutation coverage (C7), each reverted hunk → named test RED, verified by multiple seats + red-team: value try/catch → LogWithThrowingTypedValueDoesNotThrow; exporter SafeExceptionMessage → throwing-message test; ?? string.Empty stack → null-stack test; parser pos+1<len → EOF-backslash test; provider _loggers.Clear() → provider-dispose test; exporter _logger.Dispose() → dispose test; disposed check → constructor test; exporter level map → DifferentLogLevels; early-return after per-record catch → batch-isolation test (R3); reserved-key dedup → duplicate-key test.
• Security probes (executed): disposed hostile-stderr → Failure no throw; batch with a throwing-StackTrace middle record → 3 lines (first, [EXPORT ERROR], third); hostile core key/value ToString contained; injection/CR-LF posture unchanged (S1 separate).
• C6 hygiene: final diff is the intended files only; no scratch artifacts; dotnet format correctly dismissed (StyleCop is the canonical lint). No issue closed by this PR (audit follow-up).

Commits

• a9f02a8 — Harden never-throw parity and tighten test rigor (audit follow-up)
• 1441b9e — Review fix (R2): dedup SafeExceptionMessage, close disposed-export never-throw, add batch/dup-key tests
• d774c7d — Review fix (R3): use a real batch processor for the isolation test; correct Export comment

Recommendation: APPROVE (5/5). AI does not merge — awaiting your review/merge.