Skip to content

fix: Cleanup pass — 7 audit findings#136

Merged
strawgate merged 1 commit into
masterfrom
fix/cleanup-pass
Mar 30, 2026
Merged

fix: Cleanup pass — 7 audit findings#136
strawgate merged 1 commit into
masterfrom
fix/cleanup-pass

Conversation

@strawgate
Copy link
Copy Markdown
Owner

Summary

Fixes 7 findings from the reference-doc-guided codebase audit. All verified against current master.

Issue Fix Severity
#125 StreamingBuilder: move has_int/has_float after parse success HIGH — ghost null columns
#128 Add 30s HTTP timeout to OtlpSink and JsonLinesSink MEDIUM — hung endpoint blocks pipeline
#129 FanOut: try all sinks before returning error MEDIUM — first failure was skipping remaining sinks
#130 Store and reuse ureq::Agent for connection pooling MEDIUM — was creating new TCP connections per batch
#132 RawParser: escape control chars 0x00-0x1F per RFC 8259 LOW — invalid JSON output
#134 Replace assert! with Result in add_enrichment_table LOW — runtime panic on bad config

Not in this PR (bigger changes):

Test plan

  • cargo clippy -- -D warnings clean
  • cargo test all pass
  • cargo fmt --check clean

Closes #125, #128, #129, #130, #132, #134.

🤖 Generated with Claude Code

Fixes from the reference-doc-guided codebase audit:

- #125: StreamingBuilder ghost columns — move has_int/has_float flags
  after parse success (was creating all-null columns on parse failure)
- #128: Add 30s HTTP timeout to OtlpSink and JsonLinesSink via
  ureq Agent with timeout_global
- #129: FanOut tries ALL sinks before returning error (was short-
  circuiting on first failure, skipping remaining sinks)
- #130: Reuse ureq::Agent for HTTP connection pooling (was creating
  throwaway connections per request)
- #132: RawParser escapes JSON control chars 0x00-0x1F per RFC 8259
  (was passing them through raw, producing invalid JSON)
- #134: Replace assert! panics in add_enrichment_table with Result
  (was runtime panic on bad config instead of error propagation)

Closes #125, #128, #129, #130, #132, #134.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 30, 2026

Caution

Review failed

Pull request was closed or merged during review

Walkthrough

Changes span multiple crates addressing encoding, HTTP efficiency, error handling, and validation. JSON output now escapes newlines and control characters per RFC 8259. Numeric parsing flags are set only after successful validation, preventing ghost null columns. HTTP sinks now reuse persistent agents with 30-second timeouts rather than creating requests through global constructors. Multi-sink error handling defers failures to complete all operations before returning the first error. Enrichment table validation transitions from panics to explicit Result returns with checks for reserved names and duplicates.

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed PR addresses #125 by moving has_int/has_float flag assignment to after parse success, preventing ghost null columns.
Out of Scope Changes check ✅ Passed Changes in fanout.rs, json_lines.rs, otlp_sink.rs, format.rs, and lib.rs address audit findings #128#134 as documented.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@strawgate strawgate merged commit 675bcc4 into master Mar 30, 2026
8 of 9 checks passed
@strawgate strawgate deleted the fix/cleanup-pass branch March 30, 2026 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: StreamingBuilder creates ghost null columns on parse failure

1 participant