Skip to content

fix(log-parser): validate configured delimiter - #4455

Open
pbreton wants to merge 2 commits into
NVIDIA:mainfrom
pbreton:codex/issue-4401-log-parser-delimiter
Open

fix(log-parser): validate configured delimiter#4455
pbreton wants to merge 2 commits into
NVIDIA:mainfrom
pbreton:codex/issue-4401-log-parser-delimiter

Conversation

@pbreton

@pbreton pbreton commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Summary

Validate the log parser's configured delimiter before processing log files.

The delimiter must contain exactly one byte. Invalid configurations now return a descriptive error instead of panicking or silently using only the first byte.

Fixes #4401.

Root cause

process_log_file_events accessed delim.as_bytes()[0] without validating its length:

  • An empty delimiter caused an index-out-of-bounds panic.
  • A multi-byte delimiter was silently truncated.
  • A non-ASCII delimiter used only the first byte of its UTF-8 encoding.

Changes

  • Validate delimiters while loading event definitions.
  • Preserve newline as the default when the delimiter is omitted.
  • Accept single-byte delimiters.
  • Reject empty, multi-byte ASCII, and multi-byte UTF-8 delimiters.
  • Reuse the validated conversion when processing log files.
  • Add table-driven regression coverage.

Validation

  • cargo test -p carbide-log-parser
  • cargo clippy -p carbide-log-parser --all-targets -- -D warnings
  • cargo fmt --all -- --check
  • git diff --check

The regression test was confirmed to fail before the fix and pass afterward.

Note

This is an attempt at fully automated issue resolution using Codex with Sol 5.6 Medium.

Signed-off-by: Patrice Breton <pbreton@nvidia.com>
@pbreton
pbreton requested a review from a team as a code owner July 31, 2026 19:29
@coderabbitai

coderabbitai Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Summary by CodeRabbit

  • New Features

    • Added configurable log delimiters, defaulting to newline.
    • Supports single-byte delimiters for log processing.
    • Log events are now split consistently using the selected delimiter.
  • Bug Fixes

    • Invalid delimiters, including empty, multi-byte ASCII, and multi-byte UTF-8 values, are now rejected with validation errors.
    • Incomplete trailing log content is preserved until a complete delimited event is available.
  • Tests

    • Added coverage for omitted, valid, invalid, and custom delimiter configurations.

Walkthrough

The log parser validates delimiters through Configuration::delimiter_byte. It uses the validated delimiter for log boundaries and event-definition loading. Tests cover valid, invalid, and configured-delimiter processing cases.

Changes

Delimiter validation

Layer / File(s) Summary
Delimiter contract and integration
crates/log-parser/src/main.rs
Configuration::delimiter_byte accepts omitted and single-byte delimiters and rejects empty or multi-byte values. Log processing and event-definition loading use the validated delimiter.
Delimiter validation and processing tests
crates/log-parser/src/tests.rs
Tests cover delimiter validation and confirm that configured delimiters complete records while leaving unfinished suffixes unread.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely identifies delimiter validation as the primary change.
Description check ✅ Passed The description directly explains the delimiter validation fix, affected failure modes, implementation, and regression tests.
Linked Issues check ✅ Passed The changes satisfy issue #4401 by rejecting invalid delimiters, preserving the newline default, and preventing truncation and panics.
Out of Scope Changes check ✅ Passed The changes are limited to delimiter validation, delimiter handling, and related regression tests.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/log-parser/src/main.rs`:
- Line 297: Update the record-processing path around delimiter_byte to use the
delimiter variable for both record detection and splitting instead of hard-coded
newline bytes, while preserving the existing seek calculation. Add a regression
case covering a non-newline delimiter such as "|" with input that lacks a
trailing newline, verifying the record is processed and its offset advances
correctly.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: a373eed0-76e5-4279-a5d9-71198a213d7c

📥 Commits

Reviewing files that changed from the base of the PR and between 0021e8c and 444d861.

📒 Files selected for processing (2)
  • crates/log-parser/src/main.rs
  • crates/log-parser/src/tests.rs

Comment thread crates/log-parser/src/main.rs
Signed-off-by: Patrice Breton <pbreton@nvidia.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/log-parser/src/tests.rs (1)

943-1015: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert the delimiter value and diagnostic. inspect_delimiter currently discards the parsed Configuration and maps every error to (). Assert that omitted and "|" delimiters produce b'\n' and b'|', respectively, and that invalid delimiters return the delimiter-specific diagnostic.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/log-parser/src/tests.rs` around lines 943 - 1015, Update
inspect_delimiter to retain the parsed Configuration and return or assert its
delimiter value, verifying omitted input becomes b'\n' and "|" becomes b'|'.
Preserve the existing test cases while matching invalid inputs against the
delimiter-specific diagnostic instead of mapping all errors to ().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@crates/log-parser/src/tests.rs`:
- Around line 943-1015: Update inspect_delimiter to retain the parsed
Configuration and return or assert its delimiter value, verifying omitted input
becomes b'\n' and "|" becomes b'|'. Preserve the existing test cases while
matching invalid inputs against the delimiter-specific diagnostic instead of
mapping all errors to ().

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 2828b677-4d52-4586-b152-3b7afb5f1c78

📥 Commits

Reviewing files that changed from the base of the PR and between 444d861 and 43d41c0.

📒 Files selected for processing (2)
  • crates/log-parser/src/main.rs
  • crates/log-parser/src/tests.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/log-parser/src/main.rs

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.

log-parser: delimiter config assumed to be exactly one byte, empty value panics, multi-byte value silently truncated

1 participant