Skip to content

feat(parquet): audit-stream writer/reader (PR-G) - #46

Merged
jensholdgaard merged 7 commits into
mainfrom
feat/ourios-parquet-audit
May 22, 2026
Merged

feat(parquet): audit-stream writer/reader (PR-G)#46
jensholdgaard merged 7 commits into
mainfrom
feat/ourios-parquet-audit

Conversation

@jensholdgaard

@jensholdgaard jensholdgaard commented May 21, 2026

Copy link
Copy Markdown
Owner

Summary

Lands the audit half of RFC 0005 §3.10's crate-shape plan, closing out the Parquet crate's audit-event file series.

  • AuditWriter (production) — opens an audit Parquet file under partition.audit_path() (one axis coarser than the data path: stops at day=DD/), appends AuditEvents, atomically publishes on close. Same .parquet.tmp + rename-on-close + Drop cleanup story as the data writer.
  • AuditReader::open_partition / open_file / read_all — §3.9 schema-evolution contract on the audit side. Row-vs-path validation compares tenant + year/month/day (the hour field is ignored on the audit axis since the audit path has no hour segment).
  • audit_events_to_batch — builds the §3.7 column set per the normative mapping table. Per-variant column population:
    • TemplateWidened → ordinal 0, positions populated, slots empty
    • TemplateTypeExpanded → ordinal 1, positions empty, slots populated
    • TemplateWideningRejectedDegenerate → ordinal 2; the in-memory variant's would_be_template / would_be_positions ride in the §3.7 reason column as a JSON object. The §3.7 text frames reason as "the degenerate-template guard's diagnostic string"; encoding it as JSON preserves the in-memory shape without amending the schema.
  • §3.7 encoding policy: ZSTD-3 codec, dictionary off on timestamp / old_template / new_template / triggering_line_hash / triggering_line_sample, page-index downgraded to chunk on every column except the §3.7 Page index = yes four (timestamp, event_kind, event_type, template_id).
  • Adds serde_json as a ourios-parquet dependency for the rejection-variant reason payload encoder/decoder.

RFC criteria closed

  • RFC0005.7 — audit-event stream is a separate file series; every §3.7 row-level column round-trips for each of the three variants.
  • RFC0005.11 (audit axis) — row-vs-path validation on tenant / year / month / day; hour mismatches on the same day validate cleanly (the §3.4 audit-stops-at-day rule).

Tests added

  • tests/audit_round_trip.rs — one event of each variant through AuditWriterAuditReader::open_partition with full struct equality; sub-tests pin the audit-path layout (no hour= segment) and the rejection variant's would_be_* survival.
  • tests/audit_row_vs_path_validation.rs — tenant mismatch / day mismatch surface PartitionMismatch; same-day-different-hour validates cleanly; open_file skips validation.
  • Inline #[cfg(test)] mod tests in src/audit_record_batch.rs for the batch builder + the rejection-reason JSON round-trip + the pre-epoch timestamp rejection.

Known gap

Unknown event_kind ordinals surface as a hard AuditReaderError::UnknownEventKind rather than an AuditEventKind::Unknown variant — there is no analog of ParamType::Unknown on the audit-event enum yet. Deferred until a real new variant lands via a §3.8 amendment, which will either extend AuditEventKind directly or introduce the Unknown(u8) catch-all.

Test plan

  • cargo fmt --all --check
  • cargo clippy --all-targets --all-features -- -D warnings
  • cargo test --all-features (32 + 116 + 9 + 7 + 4 + 7 + 26 + 3 + 4 + 2 + 4 + 2 + 4 + 2 = all green; RFC0005.7 + audit RFC0005.11 pass)
  • Phase 3 wiring (DataFusion table provider, ourios-ingester's audit sink → AuditWriter) is out of scope for this PR.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Audit-stream Parquet reader with optional row-vs-path partition validation (day granularity) and a diagnostic open mode that skips validation.
    • Audit-stream Parquet writer with atomic finalization, per-event partition checks, and JSON-encoded rejection diagnostics in the reason column.
  • Bug Fixes / Reliability

    • Writer now marks failed writes as poisoned: failing files are not published and temp artifacts are preserved for diagnosis.
  • Tests

    • Integration tests for audit round-trip and partition validation; unit tests for timestamps and event-kind/error handling.

Review Change Stack

Lands the audit half of RFC 0005 §3.10's crate-shape plan. Closes
RFC0005.7 (audit-event stream is a separate file series, every §3.7
row-level column round-trips) and the audit-axis half of RFC0005.11
(row-vs-path validation on partition mismatch).

- `AuditWriter::open` / `append_events` / `close` mirroring the data
  writer's atomic publish (`.parquet.tmp` + rename-on-close + `Drop`
  cleanup). Writes to `partition.audit_path()`, which stops at
  `day=DD/` per §3.4 — one axis coarser than the data partitioning.
- `AuditReader::open_partition` / `open_file` / `read_all` enforcing
  the §3.9 schema-evolution contract on the audit side. Row-vs-path
  validation compares only tenant + year/month/day; the hour field
  is populated on the shared `PartitionKey` shape but ignored on the
  audit axis.
- `audit_events_to_batch` building the §3.7 columns from a slice of
  `AuditEvent`. Per-variant column population follows the §3.7
  mapping table:
  * `TemplateWidened` → ordinal 0, positions populated, slots empty
  * `TemplateTypeExpanded` → ordinal 1, positions empty, slots populated
  * `TemplateWideningRejectedDegenerate` → ordinal 2, both empty,
    `would_be_template` + `would_be_positions` JSON-encoded into the
    `reason` column (§3.7's "diagnostic string" framing accommodates
    the structured payload without amending the schema).
- §3.7 encoding policy: ZSTD-3 codec, dict-off on `timestamp` /
  `old_template` / `new_template` / `triggering_line_hash` /
  `triggering_line_sample`, page index downgraded to chunk-only on
  every column except the §3.7 `Page index = yes` four
  (`timestamp`, `event_kind`, `event_type`, `template_id`).

Adds `serde_json` as a dep for the rejection-variant reason payload.

Tests:
- `tests/audit_round_trip.rs` — RFC0005.7 full round-trip of one of
  each variant; sub-tests pinning the audit-path-stops-at-day layout
  and the rejection-variant `would_be_*` survival via `reason`.
- `tests/audit_row_vs_path_validation.rs` — RFC0005.11 audit-side
  mirror: tenant mismatch, day mismatch, same-day-different-hour
  validates cleanly, `open_file` skips validation.

Known gap: unknown `event_kind` ordinals surface as a hard
`AuditReaderError::UnknownEventKind` rather than an
`AuditEventKind::Unknown` variant (no analog of `ParamType::Unknown`
on the audit-event enum yet). Deferred until a real new variant
lands via a §3.8 amendment.

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

coderabbitai Bot commented May 21, 2026

Copy link
Copy Markdown

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds audit-stream Parquet I/O per RFC 0005: event-to-batch serialization (JSON-encoded rejection diagnostics), an atomic-file Parquet writer with partition validation, a reader that decodes rows and enforces per-row partition checks, and integration tests validating round-trip and partition behavior.

Changes

Audit Stream Parquet I/O

Layer / File(s) Summary
Writer poisoning & chunking
crates/ourios-parquet/src/writer.rs
Adds poisoned state, append_chunks helper, and preserves .parquet.tmp on Parquet write failures (WriterError::Poisoned).
Audit event serialization and batch building
crates/ourios-parquet/src/audit_record_batch.rs
Builds Arrow RecordBatches from AuditEvent slices with RFC 0005 §3.7 column mapping, stable event-kind ordinals, nested LIST/STRUCT handling, timestamp→i64 nanos conversion, and JSON encoding of rejection diagnostics via encode_rejection_reason. Includes AuditBatchError and unit tests.
Audit Parquet writer with atomic lifecycle
crates/ourios-parquet/src/audit_writer.rs
AuditWriter writes UUID-temp .parquet.tmp files, validates per-event derived partition (tenant/year/month/day), batches writes in sub-chunks, finalizes footer and atomically renames to .parquet. Configures ZSTD-3, dictionary opt-outs, and chunk-level stats for nested columns; exposes AuditWrittenFile and AuditWriterError.
Audit Parquet reader with partition validation
crates/ourios-parquet/src/audit_reader.rs
AuditReader::open_partition and open_file create batch readers; read_all decodes rows to AuditEvent with strict required/optional column handling, nested LIST/STRUCT decoding into typed vectors, JSON parsing of rejection reason, timestamp decoding, and optional per-row PartitionKey validation (hour ignored). Defines AuditReaderError.
Public API exports and round-trip/validation tests
crates/ourios-parquet/src/lib.rs, crates/ourios-parquet/Cargo.toml, crates/ourios-parquet/tests/*
Exposes audit modules and types via lib.rs re-exports, adds serde_json dependency, and integration tests that write/read three event variants (round-trip equality), assert file path stops at day= segment, confirm rejection would_be_* fields survive via JSON reason, and validate row-vs-path partition mismatch behavior.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant AuditWriter
  participant ArrowWriter
  participant Filesystem
  Client->>AuditWriter: open(partition)
  Client->>AuditWriter: append_events(events)
  AuditWriter->>ArrowWriter: write RecordBatch
  ArrowWriter->>Filesystem: emit .parquet.tmp
  Client->>AuditWriter: close()
  AuditWriter->>Filesystem: rename .parquet.tmp -> .parquet
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • jensholdgaard/ourios#45: Adds ParamType::Unknown(i32) support and ordinal round-tripping; related because reader decodes unrecognized slots_expanded tags into ParamType::Unknown.
  • jensholdgaard/ourios#44: Prior writer/atomic-publish implementation; related to the new poisoning/preservation behavior in Writer and AuditWriter.

Poem

"I'm a rabbit in the parquet patch, hopping bytes with glee,
I stash JSON carrots for rejected rows and guard each timestamp tree,
I write to .parquet.tmp then rename with care, read every row in line,
Partitions checked to day (not hour) — neat files and order fine,
Hooray for audits, round-trip safe, I hop and hum in time! 🐇"

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: implementing audit-stream writer/reader functionality for the Parquet crate, matching the primary objective of this PR.
Description check ✅ Passed The description follows the repository template with all required sections (Summary, Related, Checklist) completed. It provides comprehensive detail on the audit implementation, RFC criteria closed, tests added, and test plan verification.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/ourios-parquet-audit

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

Copilot AI 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.

Pull request overview

Implements the audit-event Parquet file series described in RFC 0005 by adding a dedicated audit writer/reader pair plus an Arrow RecordBatch builder and round-trip/validation tests, completing the Parquet crate’s audit-stream functionality.

Changes:

  • Added AuditWriter to write audit-event Parquet files under the day-partitioned audit/…/day=DD/ path with atomic publish-on-close semantics.
  • Added AuditReader to read audit-event Parquet files with RFC §3.9 schema-evolution behavior and audit-axis row-vs-path validation (tenant/year/month/day; hour ignored).
  • Added audit-stream batch construction (audit_events_to_batch) and end-to-end tests, plus serde_json for encoding/decoding the rejection variant’s reason payload.

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
crates/ourios-parquet/src/lib.rs Exposes new audit modules and re-exports audit reader/writer/batch APIs.
crates/ourios-parquet/src/audit_writer.rs Implements audit Parquet writer with RFC §3.7 encoding policy and audit-axis partition validation.
crates/ourios-parquet/src/audit_reader.rs Implements audit Parquet reader with schema evolution rules and row-vs-path validation.
crates/ourios-parquet/src/audit_record_batch.rs Builds audit Arrow RecordBatch rows per RFC §3.7 mapping, including rejection reason JSON encoding.
crates/ourios-parquet/tests/audit_round_trip.rs Adds end-to-end audit writer→reader round-trip tests and audit path layout assertions.
crates/ourios-parquet/tests/audit_row_vs_path_validation.rs Adds row-vs-path validation tests for tenant/day mismatch and same-day different-hour behavior.
crates/ourios-parquet/Cargo.toml Adds serde_json dependency to support rejection-variant reason encoding/decoding.
Cargo.lock Updates lockfile for the new dependency.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/ourios-parquet/src/audit_reader.rs Outdated
Comment thread crates/ourios-parquet/src/audit_reader.rs
Comment thread crates/ourios-parquet/src/audit_record_batch.rs Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
crates/ourios-parquet/src/audit_writer.rs (1)

319-347: 💤 Low value

Consider adding colocated unit tests for partition helpers.

derive_audit_partition and audit_partition_matches contain non-trivial logic (timestamp conversion, partition comparison ignoring hour). While integration tests cover the end-to-end flow, colocated unit tests would provide faster feedback and clearer coverage of edge cases like DST boundaries, leap years, or tenant ID comparisons.

As per coding guidelines: "Unit tests must be colocated next to the code for anything non-trivial."

🤖 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/ourios-parquet/src/audit_writer.rs` around lines 319 - 347, Add
colocated unit tests next to derive_audit_partition and audit_partition_matches
that exercise timestamp conversion and comparison logic: create AuditEvent
instances to assert derive_audit_partition returns expected PartitionKey for
normal timestamps (including leap day and DST-transition times), asserts
handling of pre-epoch timestamps produces
AuditWriterError::Batch(AuditBatchError::PreEpochTimestamp), tests timestamp
overflow path producing
AuditWriterError::Batch(AuditBatchError::TimestampOverflow), and verify
audit_partition_matches ignores hour by asserting equality for same
tenant/year/month/day with different hours and inequality for differing tenant,
year, month, or day; reference the functions derive_audit_partition,
audit_partition_matches and the error types AuditWriterError/AuditBatchError
when adding these unit tests.
🤖 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.

Nitpick comments:
In `@crates/ourios-parquet/src/audit_writer.rs`:
- Around line 319-347: Add colocated unit tests next to derive_audit_partition
and audit_partition_matches that exercise timestamp conversion and comparison
logic: create AuditEvent instances to assert derive_audit_partition returns
expected PartitionKey for normal timestamps (including leap day and
DST-transition times), asserts handling of pre-epoch timestamps produces
AuditWriterError::Batch(AuditBatchError::PreEpochTimestamp), tests timestamp
overflow path producing
AuditWriterError::Batch(AuditBatchError::TimestampOverflow), and verify
audit_partition_matches ignores hour by asserting equality for same
tenant/year/month/day with different hours and inequality for differing tenant,
year, month, or day; reference the functions derive_audit_partition,
audit_partition_matches and the error types AuditWriterError/AuditBatchError
when adding these unit tests.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 910d68b5-6d44-405a-a486-a49623f6fe44

📥 Commits

Reviewing files that changed from the base of the PR and between 59e127a and 4bc788f.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (7)
  • crates/ourios-parquet/Cargo.toml
  • crates/ourios-parquet/src/audit_reader.rs
  • crates/ourios-parquet/src/audit_record_batch.rs
  • crates/ourios-parquet/src/audit_writer.rs
  • crates/ourios-parquet/src/lib.rs
  • crates/ourios-parquet/tests/audit_round_trip.rs
  • crates/ourios-parquet/tests/audit_row_vs_path_validation.rs

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 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/ourios-parquet/src/audit_reader.rs`:
- Line 128: The batch-to-file-global row indexing change is incomplete: call
sites that still produce batch-local "row {i}" must receive and use the
file-global offset. Update batch_to_audit_events usage so it passes row_offset
into the downstream helpers (or change the helpers to accept row_offset):
required_string, required_timestamp, required_fixed_bytes16, and
materialize_required_primitive should accept a row_offset parameter (or a single
normalized function that computes file_row = row_offset + i) and use file_row in
Conversion/error messages; propagate that parameter through the call chain for
the other affected spots (around the blocks you changed at batch_to_audit_events
and the ranges referenced) so all Conversion errors report file-global rows
consistently.
- Around line 288-297: Add colocated unit tests in the same file that directly
exercise batch_to_audit_events: (1) construct a multi-batch scenario (multiple
RecordBatch inputs with a non-first-row error) and assert the returned
AuditReaderError contains a file-global row index matching row_offset +
local_row_index (verifying the multi-batch error-index path and
PartitionMismatch behavior), and (2) craft a RecordBatch with a timestamp value
that triggers overflow in the checked_add path and assert the function returns
the specific overflow AuditReaderError (the checked_add failure). Use the
existing AuditEvent, AuditReaderError, RecordBatch types and the
batch_to_audit_events helper to drive these tests and place them adjacent to the
function (also cover the analogous cases noted around the other related
functions at 379-389).
🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: c0883e86-5d34-449f-99b6-94193309eb48

📥 Commits

Reviewing files that changed from the base of the PR and between 4bc788f and 6ba7e19.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (3)
  • crates/ourios-parquet/Cargo.toml
  • crates/ourios-parquet/src/audit_reader.rs
  • crates/ourios-parquet/src/audit_record_batch.rs

Comment thread crates/ourios-parquet/src/audit_reader.rs
Comment thread crates/ourios-parquet/src/audit_reader.rs

Copilot AI 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.

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.

Comment thread crates/ourios-parquet/tests/audit_row_vs_path_validation.rs Outdated
Comment thread crates/ourios-parquet/tests/audit_round_trip.rs Outdated
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot AI 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.

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.

Comment thread crates/ourios-parquet/src/audit_writer.rs Outdated
Comment thread crates/ourios-parquet/src/audit_writer.rs
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

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

⚠️ Outside diff range comments (1)
crates/ourios-parquet/src/audit_writer.rs (1)

80-261: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add colocated unit coverage for this module’s invariants.

This is a non-trivial writer module, but there are no inline tests here. Please add #[cfg(test)] coverage next to the code for the audit partition matching rules and the temp-file lifecycle on close/error paths.

As per coding guidelines, "Unit tests must be colocated next to the code for anything non-trivial".

Also applies to: 357-464

🤖 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/ourios-parquet/src/audit_writer.rs` around lines 80 - 261, Add
colocated #[cfg(test)] unit tests in this module that (1) exercise the
partition-matching invariants by calling derive_audit_partition on synthesized
AuditEvent structs and asserting audit_partition_matches against PartitionKey
instances used to open an AuditWriter (use AuditWriter::open to get a real
partition path where appropriate) and (2) cover temp-file lifecycle by creating
an AuditWriter (AuditWriter::open), writing some events with append_events, then
forcing both a successful close (AuditWriter::close) to assert the .parquet.tmp
is renamed to final_path and a failure path (e.g., mock or induce
ArrowWriter::try_new/write/close to fail or simulate an IO rename error) to
assert the .parquet.tmp remains on disk after the error; reference the symbols
AuditWriter::open, append_events, close, audit_partition_matches,
derive_audit_partition, final_path and temp_path in the tests so they exercise
the actual module invariants.
🤖 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/ourios-parquet/src/audit_writer.rs`:
- Around line 184-195: The loop handling Parquet writes must mark the writer as
poisoned when inner.flush() or inner.write() fails so close() won't publish a
partial file; modify the implementation (e.g., in the method containing the
shown loop, likely append_events) to set a boolean flag on the AuditWriter
instance (name it poisoned or similar) before returning an Err when
inner.flush() or inner.write() returns an error, and update AuditWriter::close
to check that poisoned flag and refuse to rename/publish the temp file
(returning an error) if set; ensure the error returned from the failing Parquet
call is preserved when setting the flag and propagated.

---

Outside diff comments:
In `@crates/ourios-parquet/src/audit_writer.rs`:
- Around line 80-261: Add colocated #[cfg(test)] unit tests in this module that
(1) exercise the partition-matching invariants by calling derive_audit_partition
on synthesized AuditEvent structs and asserting audit_partition_matches against
PartitionKey instances used to open an AuditWriter (use AuditWriter::open to get
a real partition path where appropriate) and (2) cover temp-file lifecycle by
creating an AuditWriter (AuditWriter::open), writing some events with
append_events, then forcing both a successful close (AuditWriter::close) to
assert the .parquet.tmp is renamed to final_path and a failure path (e.g., mock
or induce ArrowWriter::try_new/write/close to fail or simulate an IO rename
error) to assert the .parquet.tmp remains on disk after the error; reference the
symbols AuditWriter::open, append_events, close, audit_partition_matches,
derive_audit_partition, final_path and temp_path in the tests so they exercise
the actual module invariants.
🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 302b8ad1-74e4-4d0f-a5d7-c990df34350c

📥 Commits

Reviewing files that changed from the base of the PR and between 4d32383 and bb01e86.

📒 Files selected for processing (1)
  • crates/ourios-parquet/src/audit_writer.rs

Comment thread crates/ourios-parquet/src/audit_writer.rs Outdated

Copilot AI 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.

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.

Comment thread crates/ourios-parquet/src/audit_record_batch.rs
Comment thread crates/ourios-parquet/src/audit_reader.rs Outdated
Comment thread crates/ourios-parquet/src/audit_reader.rs Outdated
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
crates/ourios-parquet/src/audit_record_batch.rs (1)

498-536: 💤 Low value

Consider adding a unit test for the TemplateMustNotChange error path.

The new invariant check for TemplateTypeExpanded (lines 299-305) is non-trivial validation logic. A colocated test that constructs a TemplateTypeExpanded event with divergent old_template / new_template and asserts the expected AuditBatchError::TemplateMustNotChange error would pin this behavior.

💡 Suggested test
#[test]
fn template_type_expanded_rejects_divergent_templates() {
    let e = AuditEvent {
        kind: AuditEventKind::TemplateTypeExpanded {
            old_version: 2,
            new_version: 3,
            old_template: "[\"user\",\"<*>\"]".to_string(),
            new_template: "[\"different\",\"<*>\"]".to_string(), // divergent
            slots_expanded: vec![],
        },
        tenant_id: TenantId::new("acme"),
        template_id: 7,
        triggering_line_hash: hash_triggering_line(b"trigger"),
        triggering_line_sample: None,
        timestamp: ts(1_775_127_480),
    };
    let err = audit_events_to_batch(std::slice::from_ref(&e))
        .expect_err("divergent templates must error");
    assert!(matches!(err, AuditBatchError::TemplateMustNotChange { .. }));
}

As per coding guidelines, crates/**/src/**/*.rs: Unit tests must be colocated next to the code for anything non-trivial.

🤖 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/ourios-parquet/src/audit_record_batch.rs` around lines 498 - 536, Add
a colocated unit test that constructs an AuditEvent with kind
AuditEventKind::TemplateTypeExpanded where old_template and new_template are
different, calls audit_events_to_batch(std::slice::from_ref(&e)), and asserts it
returns an Err matching AuditBatchError::TemplateMustNotChange; specifically
reference the TemplateTypeExpanded variant, the audit_events_to_batch function,
and the AuditBatchError::TemplateMustNotChange pattern to locate the code to
test and ensure the divergent templates trigger the expected error.
🤖 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.

Nitpick comments:
In `@crates/ourios-parquet/src/audit_record_batch.rs`:
- Around line 498-536: Add a colocated unit test that constructs an AuditEvent
with kind AuditEventKind::TemplateTypeExpanded where old_template and
new_template are different, calls
audit_events_to_batch(std::slice::from_ref(&e)), and asserts it returns an Err
matching AuditBatchError::TemplateMustNotChange; specifically reference the
TemplateTypeExpanded variant, the audit_events_to_batch function, and the
AuditBatchError::TemplateMustNotChange pattern to locate the code to test and
ensure the divergent templates trigger the expected error.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 2b1797d2-20ad-4319-976a-dbdc575b7319

📥 Commits

Reviewing files that changed from the base of the PR and between bb01e86 and f1ff078.

📒 Files selected for processing (4)
  • crates/ourios-parquet/src/audit_reader.rs
  • crates/ourios-parquet/src/audit_record_batch.rs
  • crates/ourios-parquet/src/audit_writer.rs
  • crates/ourios-parquet/src/writer.rs

Copilot AI 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.

Pull request overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (2)

crates/ourios-parquet/src/writer.rs:254

  • Writer::append_records can still be called after a prior Parquet error has set self.poisoned = true. Since the ArrowWriter buffer is documented as undefined after a failed write/flush, subsequent appends should refuse immediately (e.g., return WriterError::Poisoned) rather than attempting more writes into a potentially-corrupted in-memory state.
    pub fn append_records(&mut self, records: &[MinedRecord]) -> Result<(), WriterError> {
        if records.is_empty() {
            return Ok(());
        }
        for (idx, r) in records.iter().enumerate() {
            let derived = PartitionKey::derive(r).map_err(|e| WriterError::Batch(e.into()))?;
            if derived != self.partition {
                return Err(WriterError::PartitionMismatch {
                    row_index: idx,
                    expected: self.partition.clone(),
                    actual: derived,
                });
            }
        }
        let inner = self
            .inner
            .as_mut()
            .expect("inner ArrowWriter is Some until Writer::close is called");
        // Run the Parquet-touching loop in a helper that takes a
        // `&mut ArrowWriter<File>` so the outer `self.poisoned =
        // true` assignment can run after the borrow on `self.inner`
        // ends. Poison only on Parquet errors — `Batch` errors
        // come from `mined_records_to_batch` BEFORE any
        // `inner.write` touches the buffer, so the inner writer
        // is still in a clean state and a follow-up
        // `append_records` is safe.
        let result = append_chunks(inner, records);
        if matches!(result, Err(WriterError::Parquet(_))) {
            self.poisoned = true;
        }
        result
    }

crates/ourios-parquet/src/audit_writer.rs:217

  • AuditWriter::append_events does not check self.poisoned on entry, so callers can keep appending after a Parquet write/flush error even though the ArrowWriter buffer is documented as undefined. Consider returning AuditWriterError::Poisoned immediately when poisoned is set to prevent further writes into a potentially-corrupted state.
    pub fn append_events(&mut self, events: &[AuditEvent]) -> Result<(), AuditWriterError> {
        if events.is_empty() {
            return Ok(());
        }
        for (idx, e) in events.iter().enumerate() {
            let derived = derive_audit_partition(e)?;
            if !audit_partition_matches(&derived, &self.partition) {
                return Err(AuditWriterError::PartitionMismatch {
                    row_index: idx,
                    expected: self.partition.clone(),
                    actual: derived,
                });
            }
        }
        let inner = self
            .inner
            .as_mut()
            .expect("inner ArrowWriter is Some until AuditWriter::close is called");
        // Run the Parquet-touching loop in a helper that takes a
        // `&mut ArrowWriter<File>` so the outer `self.poisoned =
        // true` assignment can run after the borrow on `self.inner`
        // ends. Poison only on Parquet errors — `Batch` errors
        // come from `audit_events_to_batch` BEFORE any `inner.write`
        // touches the buffer, so the inner writer is still in a
        // clean state and a follow-up `append_events` is safe.
        let result = append_chunks(inner, events);
        if matches!(result, Err(AuditWriterError::Parquet(_))) {
            self.poisoned = true;
        }
        result
    }

Comment thread crates/ourios-parquet/src/writer.rs Outdated
Comment thread crates/ourios-parquet/src/audit_writer.rs Outdated
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot AI 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.

Pull request overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.

Comment thread crates/ourios-parquet/src/writer.rs
Comment thread crates/ourios-parquet/src/audit_writer.rs
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot AI 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.

Pull request overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.

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.

2 participants