feat(core): operator-driven alias map + audited alias events (RFC0001.12-.16) - #153
Merged
Conversation
…(RFC0001.12-.16)
Implements SLICE A of the RFC 0001 §6.7 alias-index write path:
- `AuditPayload::AliasAsserted` / `AliasRetracted` on the existing §6.4
audit stream, carrying `representative_id`, `member_ids`, the new
`ActorId` newtype (validated non-empty — aliasing is never anonymous,
§3.1), and a ≤256 B `reason`. Routed through `AuditSink` exactly like
the Template / Compaction payloads; new stable event_kind/event_type
ordinals (4/5). Alias events do not count as `merges_total`.
- `alias::AliasMap`: the per-tenant equivalence-class projection (§3.7
isolation). Operator API `assert` / `retract` validates, emits the
audited event, and folds it in (union-on-overlap on assert,
remove-and-resplit on retract). Canonical = `min(members)`, derived.
`resolves` returns the whole class, `{id}` for a non-aliased id. The
map is foldable from the durable event log (`apply` / `from_events`);
the physical on-disk artifact is the RFC 0005 split (#147 sibling),
out of scope here.
- `alias_assertions_total` / `alias_retractions_total` OTel-API counters
on `global::meter("ourios.miner")` with the `tenant_id` attribute
(§6.8 telemetry table), seeded at init for collect-on-read.
Flips RFC0001.12-.16 from ignored red-gate stubs to real AAA tests.
The audit-Parquet writer has no columns for alias payloads yet (that
schema extension is the RFC 0005 split), so it rejects alias events via
a new `AuditBatchError::AliasEventNotYetPersistable` rather than
inventing columns or dropping them silently — alias events stay durable
via the alias event log meanwhile.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 53 minutes and 21 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ 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 implements RFC0001 alias functionality by adding an audited, per-tenant equivalence-class index ( ChangesAlias Functionality
Sequence Diagram(s)sequenceDiagram
participant Caller
participant AliasMap
participant AuditSink
participant MetricsCounter
Caller->>AliasMap: assert(sink, tenant, representative, members, operator)
AliasMap->>AliasMap: validate_reason length
AliasMap->>AliasMap: merge overlapping equivalence classes
AliasMap->>AuditSink: emit AliasAsserted event
AliasMap->>MetricsCounter: increment assertions_counter with tenant_id
AliasMap-->>Caller: Ok(())
Caller->>AliasMap: retract(sink, tenant, id, operator)
AliasMap->>AliasMap: remove id from classes
AliasMap->>AuditSink: emit AliasRetracted event
AliasMap->>MetricsCounter: increment retractions_counter with tenant_id
AliasMap-->>Caller: Ok(())
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Pull request overview
Implements the RFC 0001 §6.7 operator-driven alias write path in ourios-core, including audited alias events on the existing audit stream, an in-memory per-tenant alias projection, and telemetry counters; and turns RFC0001.12–.16 from ignored stubs into real acceptance tests. It also updates the audit-Parquet writer to explicitly reject alias events until the RFC 0005 schema split lands.
Changes:
- Added
AuditPayload::AliasAsserted/AliasRetractedwith stableevent_kind/event_typemappings and merge-count semantics. - Introduced
AliasMap(per-tenant alias-class projection) + operator API (assert/retract) with audit emission and OTel counters. - Flipped RFC0001.12–.16 tests to validate alias durability, projection semantics, tenant isolation, and retraction behavior; added a Parquet-writer rejection test for alias events.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/ourios-parquet/src/audit_record_batch.rs | Rejects alias audit events at the Parquet writer boundary (schema not yet extended), adds error variant + test. |
| crates/ourios-core/tests/rfc0001_alias.rs | Converts RFC0001.12–.16 from ignored stubs to real acceptance tests for alias write-path behavior. |
| crates/ourios-core/src/lib.rs | Exposes the new alias module publicly. |
| crates/ourios-core/src/audit.rs | Adds audited alias payload variants plus stable kind/type constants and merge-count logic updates. |
| crates/ourios-core/src/alias.rs | Implements ActorId, Operator, and AliasMap projection + audited operator API + metrics instrumentation + unit tests. |
| crates/ourios-core/Cargo.toml | Adds opentelemetry API dependency and test-only deps for in-memory metrics export verification. |
| Cargo.lock | Locks new dependency edges from the ourios-core dependency updates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…r seed Review: canonical() now reads min via BTreeSet::first on the stored class (no resolves() clone); removed the attribute-less add(0,&[]) counter seeds that created a spurious tenant_id-less timeseries (the counters carry tenant_id and materialize per-tenant on first increment). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/ourios-parquet/src/audit_record_batch.rs (1)
667-691: ⚡ Quick winAdd test coverage for
AliasRetractedvariant.The test validates that
AliasAssertedevents are rejected, but the rejection logic at line 307 also handlesAliasRetracted. Adding a second test case forAliasRetractedwould ensure both alias variants are covered and guard against future regressions if the match arms diverge.🧪 Suggested additional test case
#[test] fn alias_retracted_events_are_rejected_pending_the_rfc_0005_split() { let retracted = AuditEvent { tenant_id: TenantId::new("acme"), timestamp: ts(1_775_127_600), payload: AuditPayload::AliasRetracted { representative_id: 1, member_ids: vec![2], actor: ourios_core::alias::ActorId::new("op").expect("non-empty actor"), reason: String::new(), }, }; let err = audit_events_to_batch(std::slice::from_ref(&retracted)) .expect_err("alias events are not yet persistable"); assert!(matches!( err, AuditBatchError::AliasEventNotYetPersistable { event_type: "alias_retracted" } )); }🤖 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 667 - 691, Add a new unit test mirroring alias_events_are_rejected_pending_the_rfc_0005_split but for the AliasRetracted variant: construct an AuditEvent with payload AuditPayload::AliasRetracted (using TenantId::new, ts(...), representative_id/member_ids/actor/reason as in the existing test), call audit_events_to_batch on a slice containing it, expect an error, and assert it matches AuditBatchError::AliasEventNotYetPersistable with event_type "alias_retracted" to ensure both alias variants are covered.crates/ourios-core/src/audit.rs (1)
205-223: ⚡ Quick winPin the new alias wire mapping in this module’s tests.
crates/ourios-parquet/src/audit_record_batch.rspersistsevent_kind()andevent_type()directly, butevent_kind_and_type_map_per_rfc_0005_3_7()still only exercises template + compaction. Addingalias_asserted/alias_retractedcoverage here would make accidental renumbering, renaming, orcounts_as_merge()drift fail inourios-corebefore it turns into a cross-crate compatibility break.Also applies to: 239-258, 270-276
🤖 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-core/src/audit.rs` around lines 205 - 223, Add test cases to pin the alias wire mapping by extending event_kind_and_type_map_per_rfc_0005_3_7()’s test coverage to include the new alias constants: ensure EVENT_KIND_ALIAS_ASSERTED and EVENT_KIND_ALIAS_RETRACTED map to EVENT_TYPE_ALIAS_ASSERTED and EVENT_TYPE_ALIAS_RETRACTED respectively; update or add assertions around counts_as_merge() and any mapping lookups that currently only validate template_widened/compaction to prevent accidental renumbering/renaming drift across crates.crates/ourios-core/src/alias.rs (1)
270-286: ⚡ Quick winSingle-source the live and replay mutation path.
assert()/retract()updateclassesseparately fromapply(), even though this module’s main contract is that live operator actions and replayed audit events produce the same projection. Reusingapply()for the just-emittedAuditEventwould keep that behavior pinned in one place and reduce drift risk the next time alias semantics change.♻️ Possible refactor
let event = AuditEvent { tenant_id: tenant.clone(), timestamp: by.timestamp, payload: AuditPayload::AliasAsserted { representative_id, member_ids, actor: by.actor, reason: by.reason, }, }; - sink.emit(event); - - self.union_in(tenant, &asserted); + sink.emit(event.clone()); + self.apply(&event); self.assertions_total.add( 1, &[KeyValue::new(ATTR_TENANT_ID, tenant.as_str().to_owned())], ); Ok(()) @@ let event = AuditEvent { tenant_id: tenant.clone(), timestamp: by.timestamp, payload: AuditPayload::AliasRetracted { representative_id: id, member_ids: Vec::new(), actor: by.actor, reason: by.reason, }, }; - sink.emit(event); - - self.remove_id(tenant, id); + sink.emit(event.clone()); + self.apply(&event); self.retractions_total.add( 1, &[KeyValue::new(ATTR_TENANT_ID, tenant.as_str().to_owned())], ); Ok(())Also applies to: 318-334, 376-404
🤖 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-core/src/alias.rs` around lines 270 - 286, The live mutation path should reuse the replay handler instead of duplicating updates: after constructing and sink.emit(event) in functions like assert() and retract(), stop calling self.union_in(...) and self.assertions_total.add(...) (and their equivalents at the other sites) and instead call the module's replay handler (apply) with the same AuditEvent you just emitted (e.g., pass the AuditEvent instance to self.apply(...) or the appropriate method that handles AuditEvent replays). Ensure AuditEvent::AliasAsserted / AliasRetracted is what apply expects and that apply covers both updating classes/sets and updating metrics so live and replayed events stay single-sourced. Adjust other duplicated blocks (the regions mentioned) to follow the same pattern.
🤖 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-core/src/alias.rs`:
- Around line 270-286: The live mutation path should reuse the replay handler
instead of duplicating updates: after constructing and sink.emit(event) in
functions like assert() and retract(), stop calling self.union_in(...) and
self.assertions_total.add(...) (and their equivalents at the other sites) and
instead call the module's replay handler (apply) with the same AuditEvent you
just emitted (e.g., pass the AuditEvent instance to self.apply(...) or the
appropriate method that handles AuditEvent replays). Ensure
AuditEvent::AliasAsserted / AliasRetracted is what apply expects and that apply
covers both updating classes/sets and updating metrics so live and replayed
events stay single-sourced. Adjust other duplicated blocks (the regions
mentioned) to follow the same pattern.
In `@crates/ourios-core/src/audit.rs`:
- Around line 205-223: Add test cases to pin the alias wire mapping by extending
event_kind_and_type_map_per_rfc_0005_3_7()’s test coverage to include the new
alias constants: ensure EVENT_KIND_ALIAS_ASSERTED and EVENT_KIND_ALIAS_RETRACTED
map to EVENT_TYPE_ALIAS_ASSERTED and EVENT_TYPE_ALIAS_RETRACTED respectively;
update or add assertions around counts_as_merge() and any mapping lookups that
currently only validate template_widened/compaction to prevent accidental
renumbering/renaming drift across crates.
In `@crates/ourios-parquet/src/audit_record_batch.rs`:
- Around line 667-691: Add a new unit test mirroring
alias_events_are_rejected_pending_the_rfc_0005_split but for the AliasRetracted
variant: construct an AuditEvent with payload AuditPayload::AliasRetracted
(using TenantId::new, ts(...), representative_id/member_ids/actor/reason as in
the existing test), call audit_events_to_batch on a slice containing it, expect
an error, and assert it matches AuditBatchError::AliasEventNotYetPersistable
with event_type "alias_retracted" to ensure both alias variants are covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c5f5b90-da18-43cb-ba50-1daf85ad56f0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
crates/ourios-core/Cargo.tomlcrates/ourios-core/src/alias.rscrates/ourios-core/src/audit.rscrates/ourios-core/src/lib.rscrates/ourios-core/tests/rfc0001_alias.rscrates/ourios-parquet/src/audit_record_batch.rs
Review pass 2: AliasMap::new doc no longer claims a zero-seed (removed); the RFC0001.12/.15 audit-payload assertions match on &events[0].payload (ergonomics) instead of a place + ref binding, matching the repo idiom. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Comment on lines
+263
to
+279
| let asserted: BTreeSet<u64> = std::iter::once(representative_id) | ||
| .chain(member_ids.iter().copied()) | ||
| .collect(); | ||
| if asserted.len() < 2 { | ||
| return Err(AliasError::DegenerateAssertion); | ||
| } | ||
|
|
||
| let event = AuditEvent { | ||
| tenant_id: tenant.clone(), | ||
| timestamp: by.timestamp, | ||
| payload: AuditPayload::AliasAsserted { | ||
| representative_id, | ||
| member_ids, | ||
| actor: by.actor, | ||
| reason: by.reason, | ||
| }, | ||
| }; |
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.
Summary
Implements SLICE A of the RFC 0001 §6.7 alias-index write path (the merged 2026-06-07 amendment) in
ourios-core, and flips the five red-gate stubs RFC0001.12–.16 from ignoredunimplemented!()to real AAA tests.The alias index is operator-driven, audited, reversible — never silently inferred (§3.1).
What landed
Audited alias events (
src/audit.rs)AuditPayload::AliasAsserted/AuditPayload::AliasRetractedvariants on the existing §6.4 audit stream, routed throughAuditSinkexactly likeTemplate/Compaction. They inherit the §3.4 WAL-before-ack durability contract (the alias event log is the source of truth).representative_id: u64(the operator's anchor id — carries no contract weight beyond naming the assertion),member_ids: Vec<u64>, the newActorIdnewtype (validated non-empty — aliasing is never anonymous), and areason: Stringvalidated ≤ 256 B (mirroring the §6.4 triggering-line-sample cap).event_kind/event_typeordinals (4/5,alias_asserted/alias_retracted). Alias events do not count towardmerges_total(that stays reserved for the two structural widenings).Per-tenant alias map (
src/alias.rs)AliasMap: the per-tenant equivalence-class projection (§3.7 isolation — an alias in tenant A never touches tenant B).assert(...)/retract(...)takes anOperator { actor, reason, timestamp }context, (a) validates, (b) emits the audited event through an injectedAuditSink, (c) updates the in-memory classes: union-on-overlap on assert (assertions sharing any member merge into one class, order-independent), remove-and-resplit on retract.{representative_id} ∪ member_ids. The canonical representative is derived asmin(members)— a display/identity convenience, re-derived on every membership change, not what defines membership. Retraction is representative-independent (retracting the canonical re-derives it from the remainder); a class that drops below two members is no longer an alias set.resolves(tenant, id)returns the whole class containingid, or{id}for a non-aliased id.apply/from_eventsrebuild the same classes by replaying the durable stream (a unit test asserts replay == the live projection). The physical on-disk map artifact (serialization format + snapshot cadence) is explicitly out of scope — that is the RFC 0005 storage split (sibling to RFC 0005 amendment: queryable/pruneable attribute columns (service.name + attr predicates) #147). No new on-disk write plane is added.Counters (§6.8 telemetry table)
alias_assertions_total/alias_retractions_totalinstrumented via the OpenTelemetry meter API (global::meter("ourios.miner")),tenant_idattribute, seeded at init for collect-on-read — matching the compaction-metrics pattern.ourios-corenow depends on the lightweightopentelemetryAPI crate only (the SDK/exporter stays inourios-telemetry, per the §6.8 API/SDK split). The un-namespaced metric names are kept per §6.8 ("deliberately does not rename the metrics … pending the dotted-semconv redesign"), so the weaver registry is untouched.Stubs flipped
RFC0001.12 (assert is durably recorded + in the map), .13 (
resolvessymmetry; non-member → itself), .14 (per-tenant isolation), .15 (retraction removes a member incl. the canonical, re-derives, audited, counter increments), .16 (never-aliased id resolves to itself). Greppable §2.3 doc-comment headers retained.Alias events × the audit Parquet writer
The audit-Parquet §3.7 schema has no columns for the alias payload (
representative_id,member_ids,actor,reason). Adding them is the RFC 0005 split (#147 sibling), out of scope here. Rather than invent columns or drop events silently, the writer rejects alias events with a newAuditBatchError::AliasEventNotYetPersistable(covered by a test). The audit reader already had another =>catch-all on unknownevent_kindordinals, so it needed no change. Alias events stay durable via the alias event log meanwhile.Invariants touched
TenantId; cross-tenant isolation is RFC0001.14.Verification (run locally in the worktree)
cargo test --all-features— 0 failures workspace-wide;tests/rfc0001_alias.rs: 5 passed; 0 failed; 0 ignored (was 5 ignored);ourios-corelib: 43 passed (incl. the new telemetry export test).cargo fmt --all --check— clean.cargo clippy --all-targets --all-features -- -D warnings— clean.Design note for the maintainer
assertvalidates that the asserted set names ≥ 2 distinct ids (AliasError::DegenerateAssertion) — a one-id assertion is a no-op the caller almost certainly didn't intend, and the spec says a single-member class isn't an alias set. The replay path (apply) tolerates a degenerate event in the log without panicking (folds to nothing) so a hand-edited / future-writer log can't crash a rebuild. Flag if you'd ratherassertsilently accept and drop a one-id set instead.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests