docs(rfc-0005): alias events in the audit stream + v1 reader-side map derivation - #183
Merged
Conversation
… derivation RFC 0001 §6.7's alias_asserted / alias_retracted events had no home in the RFC 0005 §3.7 audit schema, so nothing persisted and the querier could not load the per-tenant alias map from storage (issue #148). Pin kinds 4 / 5 with OPTIONAL alias_* columns (§3.8 rule 1), the unknown-event_kind tolerance rule, and the v1 read path: the querier derives the tenant's map at query-compile time by folding the audit stream (§3.7.1). The cached per-tenant artifact stays deferred behind the RFC 0009 §3.4 manifest fork. Adds scenario RFC0005.14; cross-refs recorded in RFC 0001 §6.7 / §9. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
|
Warning Review limit reached
More reviews will be available in 19 minutes and 4 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR amends RFC 0005 (Parquet storage) with alias audit event support (event_kind 4–5), specifying new schema columns, encoding policy, and v1 reader semantics for deriving per-tenant alias maps at query compile time. RFC 0001 is concurrently updated to reflect these finalized decisions. ChangesAlias Audit Events and Derivation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR amends the storage RFCs to give RFC 0001’s operator-driven alias events a durable home in the RFC 0005 audit Parquet stream, and to pin the v1 querier read-path for deriving a per-tenant alias map from those stored events.
Changes:
- Extend RFC 0005 §3.7 audit schema with
event_kindordinals 4–5 (alias_asserted/alias_retracted) and new OPTIONALalias_*columns. - Pin an “unknown
event_kindtolerance” rule (readers must not fail files on unknown ordinals; surface envelope-only opaque events). - Specify v1 querier behavior: derive the requesting tenant’s alias map at query-compile time by scanning/folding audit rows of kinds 4–5; update RFC 0001 to reflect this v1 storage decision.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| docs/rfcs/0005-parquet-storage.md | Adds alias audit kinds/columns, pins unknown-kind tolerance, and specifies the v1 reader-side alias-map derivation and a new scenario/test plan. |
| docs/rfcs/0001-template-miner.md | Adds dated amendment notes linking the alias write-path to RFC 0005’s persisted audit stream and v1 querier derivation approach. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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)
docs/rfcs/0001-template-miner.md (1)
1833-1857:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the stale “persisted alias map” wording.
This section still says the folded result is persisted as a per-tenant artifact and that the querier reads it at compile time, but the v1 amendment below says there is no persisted alias-map artifact yet and the audit stream is the source of truth. Please rewrite both paragraphs so they describe compile-time derivation from the audit log, with persistence deferred to the RFC 0009 cache.
🛠️ Suggested rewrite
- The folded result is persisted as a per-tenant artifact (one map per tenant, not per partition) that the querier reads at compile time. + The querier derives the requesting tenant's alias map at query-compile time by scanning the tenant's audit stream; any persisted alias-map artifact remains deferred to RFC 0009.Also applies to: 1915-1924
🤖 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 `@docs/rfcs/0001-template-miner.md` around lines 1833 - 1857, Update the "Materialization and storage." paragraphs to remove references to a persisted per-tenant alias-map artifact and instead describe that the querier derives alias mappings at compile time directly from the audit event log (the `alias_asserted` / `alias_retracted` streams) by folding per-tenant; keep the behavior details (unioning `{representative_id} ∪ member_ids`, merging overlapping classes, `min(members)` as canonical representative, single-member collapse per RFC0001.16) but explicitly state that persistence of a cached alias map is deferred to RFC0009 rather than being presently persisted; make the same textual change in the later duplicated passage that currently repeats the "persisted alias map" wording.
🤖 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 `@docs/rfcs/0005-parquet-storage.md`:
- Around line 785-797: The spec must pin a deterministic total order for folding
alias events so same-nanosecond ties across audit file flushes are reproducible:
update the text around the folding step that references `timestamp` order and
`alias_asserted`/`alias_retracted` semantics (and the
`ourios-core::alias::AliasMap` reference) to state events are ordered by
(timestamp, flush UUID, row ordinal) — i.e., primary key `timestamp`, tie-break
by the flush (file) UUID used when the audit file was written, and final
tie-break by the event's row ordinal within that flush — and note that this
total order is used when applying RFC 0001 §6.7 projection semantics so folding
is deterministic across files.
---
Outside diff comments:
In `@docs/rfcs/0001-template-miner.md`:
- Around line 1833-1857: Update the "Materialization and storage." paragraphs to
remove references to a persisted per-tenant alias-map artifact and instead
describe that the querier derives alias mappings at compile time directly from
the audit event log (the `alias_asserted` / `alias_retracted` streams) by
folding per-tenant; keep the behavior details (unioning `{representative_id} ∪
member_ids`, merging overlapping classes, `min(members)` as canonical
representative, single-member collapse per RFC0001.16) but explicitly state that
persistence of a cached alias map is deferred to RFC0009 rather than being
presently persisted; make the same textual change in the later duplicated
passage that currently repeats the "persisted alias map" wording.
🪄 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: 9f4296bf-7bec-48de-9bc1-2206ed3b1115
📒 Files selected for processing (2)
docs/rfcs/0001-template-miner.mddocs/rfcs/0005-parquet-storage.md
The alias fold order is now (timestamp, file path lexicographic, within-file row index) — deterministic across files, not just within one; the member-id list uses the §3.7 unsigned INTEGER(64) notation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e, v1 artifact note Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jensholdgaard
added a commit
that referenced
this pull request
Jun 12, 2026
…e envelope (RFC 0005 §3.7) The reader rebuilds AliasAsserted / AliasRetracted payloads from the alias_* columns (empty member list != NULL; NULL reason decodes to the in-memory empty string; an empty actor is a writer-invariant error — aliasing is never anonymous, RFC 0001 §6.7). The UnknownEventKind hard error is replaced by the now-pinned §3.7 tolerance rule: an ordinal above the known range surfaces as the new envelope-only AuditPayload::Unknown variant (the ParamType::Unknown discipline applied to the kind enum), preserved verbatim on read-then-write; folds over named kinds (AliasMap::apply, the drift filter) ignore it by construction. The forged-ordinal reader test flips from expect-error to expect-opaque-event per the RFC-gated contract change (#183, CLAUDE.md §6.2), with a round-trip test alongside. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jensholdgaard
added a commit
that referenced
this pull request
Jun 12, 2026
* feat(parquet): alias_* audit columns + writer mapping for kinds 4/5 (RFC 0005 §3.7, #148) Adds the three OPTIONAL alias columns the 2026-06-12 amendment pins (alias_representative_id, alias_member_ids LIST<UInt64> with the empty-list-vs-NULL distinction, alias_actor), maps AliasAsserted / AliasRetracted to event_kind 4 / 5 with member_ids stored verbatim and the "" <-> NULL reason rule, applies the §3.7 encoding-policy rows for the new columns, and retires the writer's interim AliasEventNotYetPersistable rejection. Schema change is §3.8 rule 1 (additive OPTIONAL); the schema-pin fixture moves in lockstep. The colocated expect-error test flips per the RFC-gated contract change (CLAUDE.md §6.2), citing the amendment in its doc comment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(parquet,core): decode alias kinds; unknown kinds become an opaque envelope (RFC 0005 §3.7) The reader rebuilds AliasAsserted / AliasRetracted payloads from the alias_* columns (empty member list != NULL; NULL reason decodes to the in-memory empty string; an empty actor is a writer-invariant error — aliasing is never anonymous, RFC 0001 §6.7). The UnknownEventKind hard error is replaced by the now-pinned §3.7 tolerance rule: an ordinal above the known range surfaces as the new envelope-only AuditPayload::Unknown variant (the ParamType::Unknown discipline applied to the kind enum), preserved verbatim on read-then-write; folds over named kinds (AliasMap::apply, the drift filter) ignore it by construction. The forged-ordinal reader test flips from expect-error to expect-opaque-event per the RFC-gated contract change (#183, CLAUDE.md §6.2), with a round-trip test alongside. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(querier): extract the shared audit partition walk into audit_scan Pure relocation of drift.rs's tenant-rooted audit-tree walk (canonical escape backstop, canonical-path de-dup, conservative day prune) into a shared module, with the day window now optional and the resolved file set sorted lexicographically — the file-path component of the RFC 0005 §3.7.1 total fold order the alias-map derivation needs next. The two prune unit tests move with the helpers, unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(querier): derive the tenant alias map from the audit stream (RFC 0005 §3.7.1, #148) run_query's alias parameter becomes Option<&AliasMap>: None — the production default — folds the requesting tenant's map from its audit/ partition at compile time (alias_store::derive_alias_map), reading kinds 4/5 via AuditReader in the §3.7.1 total order (timestamp, file path lexicographic, within-file row index — the shared walk's sorted file set + in-file row order under a stable sort by timestamp) and handing the events to ourios-core::alias::AliasMap::from_events, whose RFC 0001 §6.7 semantics this does not restate. Some(map) stays the test/operator override, bypassing storage. Queries with no resolves_to skip the audit scan entirely. A row claiming another tenant under the tenant's partition root is a loud Storage error, not a silent drop (CLAUDE.md §3.7). Test updates are mechanical Option-wrapping at the call sites; no assertion changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: land RFC0005.14 + the storage-backed RFC0002.9 (#148 step 3) (a) round-trip through the real audit writer/reader: member set verbatim (order + duplicate preserved), empty-list retraction != NULL, actor, the "" <-> NULL reason rule, and raw-column NULL discipline per kind (§3.8 rule 6). (b) the §3.7.1 derived fold: assert-then-retract by event time, plus both directions of the cross-file same-timestamp tiebreak — one event per file, files renamed into a crafted lexicographic order so the outcome is decided by the file-path component of the total order and nothing else. (c) tenant isolation: a second tenant's stored alias events never fold into the requesting tenant's derived map (CLAUDE.md §3.7). (d) storage-backed RFC0002.9: the assertion is written through ParquetAuditSink and run_query(None) derives the map end-to-end — resolves_to(A) returns A ∪ {B} while template_id == A stays exactly A; the injected-map RFC0002.9 test stays alongside, unweakened. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(querier): bucket-root trust anchor for the audit scan; validate before alias derivation Review round 1: - a symlinked audit/tenant_id=... root is now rejected outright (the per-file starts_with backstop trusted the symlink's own resolution) - run_query validates the query before paying the alias-derivation IO, so compile errors precede Storage errors deterministically - tenant-mismatch error renders tenant IDs with Display, not Debug - tests: symlinked-root rejection, error precedence, and the T2 isolation test now proves its own map was derived (resolves_to == 2) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(querier,parquet): precise error-precedence comment; batch-local row index in messages Copilot round 2, wording only: the run_query comment no longer implies all invalid-query errors precede the audit scan (predicate compilation needs the map), and the non-nullable-element conversion messages say "batch row" — the index is batch-local, not file-global. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jun 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The gap (issue #148)
RFC 0001 §6.7 (amendment 2026-06-07) pins the alias model and write path: operator-driven, audited
alias_asserted/alias_retractedevents on the §6.4 audit stream, folded into a per-tenant alias map (ourios-core::alias::AliasMap, RFC0001.12–.16 green) that RFC 0002'sresolves_toconsumes (RFC0002.9 green). RFC 0001 §9 hands the storage half to "the RFC 0005 line" — but the RFC 0005 §3.7 audit schema only knows kinds 0–3, so alias events have no persistent home (AuditBatchError::AliasEventNotYetPersistableis the writer's interim rejection) and the querier cannot load the map from storage.What this amendment pins (doc-only)
alias_asserted(4) andalias_retracted(5), matching the ordinalsourios-core::auditalready pins. Payload lands in new OPTIONAL kind-prefixed columns per the compaction-amendment pattern:alias_representative_id(INT64),alias_member_ids(LIST<INT64>),alias_actor(STRING); the existingreasoncolumn is reused for the operator justification ("" ↔ NULL). The canonical-JSONUtf8encoding formember_idswas considered and rejected on the same grounds the 2026-06-03 amendment rejected a structuredreason: aLISTof ids is first-class queryable where a JSON blob is opaque, andLISTis already the §3.7 precedent for flat set-valued payload fields (positions_widened,compaction_input_files).event_kindtolerance rule pinned. Today'sAuditReaderhard-errors on unknown ordinals, with a documented deferral "until a real new variant lands" — kinds 4–5 are that variant. The pinned rule: readers surface unknown ordinals as opaque envelope-only events rather than failing the file (theParamType::Unknown/ §3.9 discipline applied to the kind enum). Already-deployed readers are bounded by §3.8 rule 6's version-together argument.event_kind/event_typedict/page-index columns; alias events are rare operator actions, so the scan is small) and folding in event-time order via the RFC 0001 §6.7 projection semantics — referenced, not duplicated. Consistency bound: the map reflects events durably written and flushed at scan time, the same eventual-consistency stance §6.7 already takes.resolves_toreflects the folded assert/retract state), and tenant isolation. RFC 0001 §6.7 / §9 get matching dated amendment notes; nostatus:field changes. RFC 0002 needed no edit — it already defers to "the alias-set membership of RFC 0001 §6.7" without implying an operator-supplied-only map.Invariants / hazards touched (CLAUDE.md §4)
None; no data migration. New kinds follow the established "§3.8 additive amendment" path for the mapping table.Verification
mdbook buildclean (no new diagrams added).docs/rfcs/0001-template-miner.md,docs/rfcs/0005-parquet-storage.md. Cargo checks not run — no code changed.Closes nothing yet — #148's implementation slice (writer columns, reader ordinals 4–5 + tolerance rule, querier derivation) lands against this spec.
🤖 Generated with Claude Code
Summary by CodeRabbit