Repository navigation
feat(reborn): durable event/audit substrate - #2993
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces several core crates for the IronClaw Reborn architecture: ironclaw_host_api for shared authority contracts, ironclaw_resources for resource reservation and quota management, ironclaw_events for runtime auditing and durable logging, and ironclaw_architecture for enforcing workspace dependency boundaries. These additions provide the foundational vocabulary and enforcement mechanisms for identity, authorization, and observability. A security concern was raised regarding the canonical_json function in ironclaw_host_api, which uses unbounded recursion and could be susceptible to stack overflow attacks from deeply nested JSON inputs.
| fn canonical_json(value: &serde_json::Value) -> serde_json::Value { | ||
| match value { | ||
| serde_json::Value::Array(items) => { | ||
| serde_json::Value::Array(items.iter().map(canonical_json).collect()) | ||
| } | ||
| serde_json::Value::Object(map) => { | ||
| let mut entries = map.iter().collect::<Vec<_>>(); | ||
| entries.sort_by_key(|(key, _)| *key); | ||
| let mut canonical = serde_json::Map::new(); | ||
| for (key, value) in entries { | ||
| canonical.insert(key.clone(), canonical_json(value)); | ||
| } | ||
| serde_json::Value::Object(canonical) | ||
| } | ||
| _ => value.clone(), | ||
| } | ||
| } |
There was a problem hiding this comment.
The canonical_json function uses recursion to process serde_json::Value. In security-sensitive contexts like generating invocation fingerprints for approvals, this could be exploited by providing a deeply nested JSON object to cause a stack overflow, leading to a Denial of Service (DoS). Consider implementing this iteratively or adding a recursion depth limit to prevent unbounded resource usage, similar to the requirement of capping string length and entries when interning untrusted sources.
References
- When processing data from untrusted sources, cap the length and total number of entries to prevent unbounded memory leaks or resource exhaustion.
Address contract-level issues raised in the Codex code review of #2993: 1. Silent replay loss on future cursor. read_after_cursor previously echoed back any caller-supplied cursor for a stream whose head was lower (or for an absent stream), losing every event 1..cursor when those events later landed. The contract requires explicit ReplayGap signaling so consumers fetch a snapshot/rebase rather than silently miss history. StreamState::read_after now returns ReplayGap when after > head, and InMemoryDurableEventLog/InMemoryDurableAuditLog return ReplayGap on non-origin cursors against absent streams. The earlier test that pinned the wrong behavior is split into the correct empty-stream case (origin/None) and two new tests that prove gap signaling on a future cursor against an empty stream and against a populated stream. 2. RuntimeEvent deserialize bypassed sanitize_error_kind. Public error_kind: Option<String> with derived Deserialize meant a hand-rolled or replayed JSONL record could put a raw secret into the field without going through the typed constructors. Adds a custom deserialize_with hook that re-runs sanitize_error_kind on inbound values so the redaction guard fires on every code path, including JSONL replay. New caller-level test deserialize_runtime_event_resanitizes_error_kind proves it. 3. sanitize_error_kind allowed token-shaped secrets. Previous allowlist [a-z0-9_-.:] up to 128 bytes accepted real-world API key shapes like sk_live_<32 alphanumerics>. Tightens to require leading lowercase letter, drops dash from the body alphabet, caps overall length at 64 bytes, caps each .:-separated segment at 24 bytes, and requires every segment to start with a lowercase letter. Test sanitize_error_kind_collapses_long_or_unsafe_input expanded to cover token shapes, leading digits, leading underscores, mixed-case identifiers, and oversized random segments. 4. Cursor overflow on append. StreamState::append now uses checked_add and returns EventError::DurableLog on u64 exhaustion rather than panicking on wrap. Append signature is now Result<EventLogEntry<T>, EventError>. 5. ReplayGap path was untested. Added pub truncate_before_or_at(stream, cursor) on both in-memory logs that drops entries up to and including the supplied cursor and advances earliest_retained, matching the retention model the contract describes. New test replay_gap_after_truncation_forces_snapshot_rebase exercises the gap-after-retention path end to end. 6. Audit redaction lacked a serialization assertion. New test approval_audit_envelope_serialization_excludes_raw_reason_and_fingerprint builds an ApprovalRequest with a unique sentinel `reason`, runs it through AuditEnvelope::approval_resolved, and asserts the JSON serialization does not contain the sentinel or an invocation_fingerprint field. 7. Serde derives on EventStreamKey, EventLogEntry<T>, EventReplay<T> for gateway/SSE consumers. 8. EventSink/AuditSink doc comments now state the best-effort contract loudly: callers must not `?`-propagate sink errors and must continue with their original outcome. Type-level enforcement is flagged as a follow-up. Test count: 18 -> 23. All pass; cargo fmt, clippy with -D warnings, RUSTDOCFLAGS=-D warnings cargo doc, and ironclaw_architecture boundary test all green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Updated in response to a Codex code review. The review flagged four contract-level concerns; all four are now addressed in commit 072a26e. Fixed
Added in response to recommendations
Documented; deferred
Verification
Heads up on the force-push: the original commit had a test fixture string |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 072a26e673
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let next_cursor = entries | ||
| .last() | ||
| .map(|entry| entry.cursor) | ||
| .unwrap_or_else(|| EventCursor::new(after.max(current_cursor))); |
There was a problem hiding this comment.
Reject replay cursors beyond JSONL head
When replay_jsonl produces no entries, it sets next_cursor to max(after, current_cursor), so a cursor that is ahead of the file head is silently echoed instead of being rejected. In a JSONL-backed durable log, that lets a wrong/stale cursor (for example from another stream or store reset) appear valid and can hide earlier records once new lines are appended, which breaks the replay-gap semantics this crate enforces in InMemoryDurableEventLog.
Useful? React with 👍 / 👎.
| fn from_estimate(estimate: &ResourceEstimate) -> Self { | ||
| Self { | ||
| usd: estimate.usd.unwrap_or_default(), | ||
| input_tokens: estimate.input_tokens.unwrap_or_default(), |
There was a problem hiding this comment.
Disallow negative USD estimates in reservations
ResourceTally::from_estimate accepts signed Decimal values unchanged, so a negative estimate.usd flows into reservation checks and reserved balances. That allows callers to lower active_reserved with a negative reservation and then admit additional positive reservations that should be denied by budget limits, undermining enforcement wherever estimates are caller-controlled or computed from untrusted inputs.
Useful? React with 👍 / 👎.
| /// | ||
| /// Used by JSONL-backed durable log adapters in later grouped Reborn PRs. | ||
| /// The cursor is the 1-based line index of the last consumed record. | ||
| pub fn replay_jsonl<T>( |
There was a problem hiding this comment.
High Severity — replay_jsonl accepts future/foreign cursors and can silently skip history.
The in-memory durable log correctly treats a cursor beyond the stream head as a ReplayGap, but this helper does not. If a JSONL log has 2 records and a caller asks for after = EventCursor::new(99), the loop finishes with current_cursor = 2 and returns Ok(empty, next_cursor = 99). A future filesystem JSONL backend built on this helper would then silently accept a cursor this stream never issued, which is exactly the replay-loss case the durable-log contract is trying to prevent.
Concrete fix: after scanning, if after > current_cursor, return EventError::ReplayGap { requested: EventCursor::new(after), earliest: EventCursor::new(current_cursor) } (or a documented head/earliest value), and add a replay_jsonl_with_future_cursor_returns_replay_gap test.
There was a problem hiding this comment.
Fixed in 901dba3. replay_jsonl now matches InMemoryDurableEventLog semantics: after scanning, if requested > current_cursor, returns EventError::ReplayGap { requested, earliest: current_cursor }. Added replay_jsonl_with_future_cursor_returns_replay_gap test.
| pub process_id: Option<ProcessId>, | ||
| pub output_bytes: Option<u64>, | ||
| #[serde(default, deserialize_with = "deserialize_error_kind")] | ||
| pub error_kind: Option<String>, |
There was a problem hiding this comment.
High Severity — public RuntimeEvent fields let direct construction bypass error redaction.
The docs say a hand-built record cannot smuggle raw error text because deserialization re-runs sanitize_error_kind, but that only protects inbound JSON. Since all fields are public and Serialize is derived, any in-process caller can construct:
RuntimeEvent { error_kind: Some("/Users/alice/token=secret".into()), ... }and then append/serialize it without hitting sanitize_error_kind. That violates the event redaction invariant before durable backends are added.
Concrete fix: make RuntimeEvent fields private and expose constructors/getters, introduce a sanitized ErrorKind newtype for the field, or implement custom Serialize that re-sanitizes error_kind before writing. Please also add a test that manually constructs an unsafe event and verifies serialization/append redacts it.
There was a problem hiding this comment.
Fixed in 901dba3. Custom Serialize impl now re-runs sanitize_error_kind on the way out, symmetric with the existing Deserialize hook. Direct construction RuntimeEvent { error_kind: Some(raw), .. } followed by serde_json/append still emits "error_kind":"Unclassified". Test direct_construction_serialize_path_resanitizes_error_kind builds a path-shaped raw value and asserts the wire payload is sanitized.
| pub trait DurableEventLog: Send + Sync { | ||
| async fn append(&self, event: RuntimeEvent) -> Result<EventLogEntry<RuntimeEvent>, EventError>; | ||
|
|
||
| async fn read_after_cursor( |
There was a problem hiding this comment.
Medium Severity — replay authorization is easy to get wrong because stream keys omit project/mission/thread scope.
EventStreamKey partitions only by (tenant, user, agent), while the comment says project/mission/thread/process/invocation filtering is a read-side filter. However, read_after_cursor takes only stream, after, and limit, so the durable-log trait itself returns every record in that user/agent stream and gives implementers no typed authorized-scope/filter input to enforce deeper isolation.
A project-scoped consumer for project A could receive project B events for the same tenant/user/agent if the caller forgets to post-filter. Given this is the substrate future SSE/projection callers will build on, relying on every caller to remember that filter is a risky authority boundary.
Concrete fix: either include project_id in the v1 stream key, or add an explicit authorized scope/filter parameter to read_after_cursor and make implementations enforce it. Please add a cross-project isolation test that appends two same-user/same-agent events with different project_ids and proves a project-scoped read cannot see the other project.
There was a problem hiding this comment.
Fixed in 901dba3. Added a typed ReadScope { project_id, mission_id, thread_id, process_id } filter and made it a required argument on DurableEventLog::read_after_cursor and DurableAuditLog::read_after_cursor. Implementations enforce the filter; cursors advance through filtered-out records so consumers can resume. Filter is a tightening — a record's None field cannot match a filter that asks for Some(...). Test read_scope_filter_isolates_project_within_same_stream appends three same-stream events across two project_ids and asserts a project-A-scoped read sees only project A; read_scope_filter_excludes_records_with_none_field_when_filter_is_some pins the tightening semantics. ReadScope::any() exists for tests/admin paths but production callers must construct a tightened filter, removing the post-filter caller-discipline foot gun.
Address Firat's review on #2993 plus the codex-bot P1 finding on replay_jsonl. All three issues turn on the same theme: the substrate must enforce its invariants on every wire/cursor crossing, not rely on caller discipline. 1. replay_jsonl accepted future cursors (Codex P1 / Firat HIGH). The byte-level helper had the same silent-replay-loss bug as the in-memory log: a cursor beyond the file head returned an empty replay with the cursor echoed back, hiding records 1..head once they appeared. A future filesystem JSONL backend built on this helper would inherit the bug. Now mirrors InMemoryDurableEventLog: after scanning, if requested > current_cursor, return ReplayGap. New test replay_jsonl_with_future_cursor_returns_replay_gap pins it. 2. RuntimeEvent direct-construction Serialize path bypassed redaction (Firat HIGH). The previous commit added a Deserialize hook that re-runs sanitize_error_kind on inbound payloads, but the public fields plus derived Serialize meant any in-process caller could build RuntimeEvent { error_kind: Some(raw_path), .. } directly and persist the raw value through any to_json/append path. Custom Serialize impl now re-runs sanitize_error_kind on the way out, symmetric with the Deserialize hook. Both wire directions are enforced. Test direct_construction_serialize_path_resanitizes_error_kind builds a RuntimeEvent with a path-shaped error_kind directly and asserts the serialized JSON contains "Unclassified" rather than the raw value. 3. EventStreamKey omitted project/mission/thread/process scope, leaving read authorization to caller discipline (Firat MEDIUM). EventStreamKey continues to partition cursors by (tenant, user, agent) per the durable-log path contract. But within a stream, multiple projects/missions/threads/processes co-exist, and a project-scoped consumer must not see other projects' events. The previous trait relied on every caller to remember to post-filter — exactly the kind of authority boundary that produces leaks when a new caller forgets. Adds a typed ReadScope filter required by DurableEventLog and DurableAuditLog read_after_cursor: async fn read_after_cursor( &self, stream: &EventStreamKey, filter: &ReadScope, after: Option<EventCursor>, limit: usize, ) -> Result<EventReplay<...>, EventError>; ReadScope { project_id, mission_id, thread_id, process_id }, all optional. ReadScope::any() disables filtering for tests/admin paths. Filter is a tightening: a record's None field cannot match a filter that asks for Some(...). Implementations enforce the filter; cursors advance through filtered-out records so consumers resume cleanly. Tests: - read_scope_filter_isolates_project_within_same_stream appends three same-stream events across two project_ids and asserts a project-A-scoped read sees only project A and the cursor advances past the project-B record. - read_scope_filter_excludes_records_with_none_field_when_filter_is_some pins the tightening semantics. Test count: 23 -> 27. fmt, clippy with -D warnings, RUSTDOCFLAGS=-D warnings cargo doc, and ironclaw_architecture boundary test all green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Updated in response to the latest review (Firat HIGH/HIGH/MEDIUM + Codex P1). All four addressed in 901dba3; replied to each inline thread above. Fixed
Verification
Note on the two PR1 findingsThe review thread also surfaced two findings that originate in PR1 (#2988) rather than this PR — |
| Forbidden dependencies (enforced by `ironclaw_architecture`): authorization, | ||
| approvals, capabilities, dispatcher, extensions, host_runtime, secrets, | ||
| network, mcp, processes, resources, run_state, scripts, wasm, filesystem, | ||
| memory. |
There was a problem hiding this comment.
Medium Severity — CLAUDE.md vs BoundaryRule drift on the forbidden-deps list.
This file lists filesystem and memory as forbidden deps for ironclaw_events. The actual BoundaryRule for ironclaw_events in crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs does not include either:
crate_name: "ironclaw_events",
forbidden: vec![
"ironclaw_authorization", "ironclaw_approvals", "ironclaw_capabilities",
"ironclaw_dispatcher", "ironclaw_extensions", "ironclaw_host_runtime",
"ironclaw_secrets", "ironclaw_network", "ironclaw_mcp", "ironclaw_processes",
"ironclaw_resources", "ironclaw_run_state", "ironclaw_scripts", "ironclaw_wasm",
],The PR description's "Open coordination questions" item 3 explicitly says omitting ironclaw_filesystem is intentional ("PR2's JSONL sink will need that dep, so leaving it out preserves PR2's freedom"). So the doc is the wrong file — it's overly restrictive vs the actual enforcement.
Two ways this misleads future contributors:
- A reader trusting the doc would believe adding
ironclaw_filesystemwill be rejected by the boundary test. It won't — the test passes regardless. - If someone fixes the doc by tightening the rule (the natural reading of "doc says forbidden, code says allowed → make code match doc") they'd break PR2's premise.
Either:
- Remove
filesystemandmemoryfrom this list and add a note explainingfilesystemis deliberately allowed for PR2's JSONL sink (matching the open question), or - Add
ironclaw_filesystemandironclaw_memoryto theBoundaryRuleif the team prefers tightening over PR2 freedom (which is the discussion the PR description is asking for).
Either way the doc and the rule should agree.
There was a problem hiding this comment.
Fixed in 3ed34b7. Removed filesystem and memory from the forbidden-deps list in crates/ironclaw_events/CLAUDE.md so the doc matches the actual BoundaryRule in crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs. Added a note that ironclaw_filesystem is deliberately allowed for PR2's JSONL sink (per the PR description's open-coordination question) and that the test is the authoritative source if the two ever drift again.
| } | ||
| self.entries.retain(|entry| entry.cursor.as_u64() > bound); | ||
| if bound >= self.earliest_retained { | ||
| self.earliest_retained = bound + 1; |
There was a problem hiding this comment.
Medium Severity — truncate_before_or_at doesn't validate cursor <= next_cursor, leaving the stream permanently bricked on misuse.
If a caller passes a cursor beyond the current head:
fn truncate_before_or_at(&mut self, cursor: EventCursor) {
let bound = cursor.as_u64();
if bound == 0 { return; }
self.entries.retain(|entry| entry.cursor.as_u64() > bound);
if bound >= self.earliest_retained {
self.earliest_retained = bound + 1;
}
}Trace with next_cursor = 5, caller passes bound = 99:
- All 5 existing entries are retained-out (none have cursor > 99).
earliest_retained = 100.- Stream state:
next_cursor = 5,earliest_retained = 100,entries = [].
Now the caller appends a fresh event:
next_cursor = 6, entry at cursor 6.
The caller (or any other consumer) reads with after = 5 (or any cursor in [0, 99]):
read_after:5 > next_cursor (6)→ false;earliest_retained > 0 (100 > 0) && after < earliest_retained - 1 (5 < 99)→ true →ReplayGap { requested: 5, earliest: 100 }.
But cursor 100 doesn't exist! The caller follows the gap protocol, requests a snapshot, gets back cursor 6 from the snapshot, and reads with after = 6:
6 > 6→ false;6 < 99→ true → stillReplayGap { earliest: 100 }.
The stream is permanently bricked until ~94 more appends bring next_cursor up to earliest_retained.
Trigger requires misuse — a retention policy that picks a cursor by inspecting an existing entry won't hit this. But:
- The function is
pubonInMemoryDurableEventLogandInMemoryDurableAuditLog. - The docstring just says "Discard entries whose cursor is
<=the supplied cursor and advanceearliest_retained" with no precondition. - A "drop everything older than 24h" retention policy implemented in calendar time (not cursor time) could easily produce a bound > head if the stream is quiet.
- Production backends (PostgreSQL, libSQL) will likely inherit this contract from the in-memory reference impl. The reference impl is what callers will read first to learn the semantics.
Suggest one of:
// Option A: clamp
let bound = cursor.as_u64().min(self.next_cursor);
// Option B: reject (less surprising for retention policies)
if bound > self.next_cursor {
return Err(EventError::InvalidReplayRequest {
reason: format!(
"truncation cursor {bound} exceeds stream head {head}",
head = self.next_cursor,
),
});
}
// Option C: document the precondition loudly
/// **Caller must ensure `cursor <= current head cursor`.** Passing a cursor
/// beyond the head leaves the stream in a permanently-gapped state until
/// appends catch up.
Option B is the cleanest type-level enforcement; it requires changing the signature to return Result. Option A is a one-line fix that preserves the existing signature.
There was a problem hiding this comment.
Fixed in 3ed34b7. Took Option B: StreamState::truncate_before_or_at now returns Result<(), EventError> and rejects cursors past next_cursor with InvalidReplayRequest { reason: "truncation cursor {bound} exceeds stream head {head}" }. The public InMemoryDurableEventLog::truncate_before_or_at and InMemoryDurableAuditLog::truncate_before_or_at already returned Result, so the surface is unchanged — the error now propagates instead of silently bricking the stream. Added regression test truncate_beyond_head_is_rejected that asserts the rejection and that the stream remains usable for subsequent reads. Docstring on the public method also updated to call out the new precondition.
| /// Replay a JSONL byte slice after a cursor with a bounded limit. | ||
| /// | ||
| /// Used by JSONL-backed durable log adapters in later grouped Reborn PRs. | ||
| /// The cursor is the 1-based line index of the last consumed record. |
There was a problem hiding this comment.
Low Severity — Docstring promises "1-based line index of the last consumed record" but doesn't warn about the compaction case.
The helper assigns EventCursor::new(current_cursor) where current_cursor is incremented per non-empty line. This works cleanly when the JSONL file is append-only and never compacted.
But future filesystem JSONL backends will likely implement retention/compaction (drop old entries). After compaction, line 1 of the file may correspond to cursor 1234 in the logical stream, not cursor 1. Calling replay_jsonl(bytes, after=1233, limit) on the compacted file would:
- Increment
current_cursorfrom 1 to 5 (the line count) instead of 1234 to 1238. - Falsely return
ReplayGap { requested: 1233, earliest: 5 }— a meaninglessearliestvalue.
A backend that compacts would need to:
- Store the cursor inline in the JSONL line (e.g.
{"cursor": 1234, "record": {...}}) and use a different parser, OR - Maintain an out-of-band mapping of file-offset → cursor, OR
- Never compact — keep using line-index = cursor.
The current docstring leaves a future implementer to discover this on their own. Suggest one extra sentence:
/// Replay a JSONL byte slice after a cursor with a bounded limit.
///
/// Used by JSONL-backed durable log adapters in later grouped Reborn PRs.
/// The cursor is the 1-based line index of the last consumed record.
///
/// **Assumes uncompacted JSONL.** Backends that compact entries (drop old
/// records to reclaim disk) must not use this helper directly — line index
/// will desynchronize from the logical cursor. Such backends should either
/// store the cursor inline in each record or maintain an out-of-band offset
/// map.
Pre-emptive concern; the JSONL-backed sinks are deferred to a later grouped PR per the carve note. Worth pinning the assumption now while the helper is the only public API for cursor extraction.
There was a problem hiding this comment.
Fixed in 3ed34b7. Added the suggested compaction warning to the replay_jsonl docstring: backends that compact entries must not use this helper directly because line index will desynchronize from the logical cursor and ReplayGap.earliest will be meaningless. The doc now spells out the two acceptable strategies (cursor inline per record, or out-of-band offset→cursor map) so a future implementer doesn't rediscover this on their own.
Paranoid review summary — durable event/audit substrateThis PR has been through two prior review rounds (Codex P1 + my earlier review on the typed substrate). Both rounds' concerns are now addressed in the latest commits — Three new findings posted inline:
Cross-PR pattern observation (informational)Asymmetric redaction posture between
This PR's CLAUDE.md asserts the redaction contract for both event and audit envelopes, but the test suite ( Stale-base noteThe diff against v2 Engine impactNot related — substrate-only PR, no v1 modules touched, exposure checklist confirms zero What's solid
LGTM with the three suggestions above; finding #2 (truncation footgun) is the most concrete bug worth landing before downstream backends inherit the contract. |
|
Addressed the three review comments from @serrrfirat at #2993 (review), #2993 (review), and #2993 (review) in 3ed34b7:
|
Adds ironclaw_events as a Reborn substrate crate carved from PR2 to land ahead of the filesystem grouped slice, per the integration plan in #2987 and the prior tracking comment. Includes: - RuntimeEvent / RuntimeEventKind with redaction-aware constructors and sanitize_error_kind helper that collapses unsafe error detail into the Unclassified token. Approval-specific event kinds are deliberately absent; approval resolution is recorded as a control-plane AuditEnvelope per events.md. - EventSink and AuditSink (best-effort delivery) async traits whose failures must not alter runtime/control-plane outcomes. - DurableEventLog and DurableAuditLog (explicit-error append + scoped read_after_cursor) async traits. Append failures are propagated; a cursor older than the earliest retained record yields EventError::ReplayGap so transports request a snapshot/rebase rather than silently lose history. - EventStreamKey = (tenant_id, user_id, agent_id) partition; per-stream monotonic EventCursor; EventLogEntry / EventReplay shapes; explicit zero-limit rejection. - InMemoryEventSink / InMemoryAuditSink for best-effort delivery. - InMemoryDurableEventLog / InMemoryDurableAuditLog for caller-level tests across PR3/PR4/PR6 substrate work. - parse_jsonl / replay_jsonl byte-level helpers exposed for downstream filesystem-backed JSONL backends in PR2. The crate has zero ironclaw_filesystem dependency; the JSONL backend is deliberately deferred to the filesystem grouped slice. PostgreSQL/libSQL backends follow with the database substrates. The byte-level helpers let those later backends share redaction and replay invariants. Caller-level tests cover full append -> cursor -> read_after_cursor loops, per-stream partitioning across (tenant, user, agent) tuples, limit/resume semantics, redaction guarantees on dispatch_failed, audit envelope replay across stream keys, JSONL round-trip, and explicit malformed-line and zero-limit rejection. Architecture boundary tests for ironclaw_events were already present at crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs and remain green with the new crate in the workspace. Verification: cargo fmt --check cargo test -p ironclaw_events cargo test -p ironclaw_architecture cargo clippy -p ironclaw_events --all-targets -- -D warnings RUSTDOCFLAGS='-D warnings' cargo doc -p ironclaw_events --no-deps cargo tree -p ironclaw_events -e normal All pass; the only ironclaw_ normal dependency is ironclaw_host_api. No production wiring; feature-gated free; src/ untouched. Tracking issue #2987. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address contract-level issues raised in the Codex code review of #2993: 1. Silent replay loss on future cursor. read_after_cursor previously echoed back any caller-supplied cursor for a stream whose head was lower (or for an absent stream), losing every event 1..cursor when those events later landed. The contract requires explicit ReplayGap signaling so consumers fetch a snapshot/rebase rather than silently miss history. StreamState::read_after now returns ReplayGap when after > head, and InMemoryDurableEventLog/InMemoryDurableAuditLog return ReplayGap on non-origin cursors against absent streams. The earlier test that pinned the wrong behavior is split into the correct empty-stream case (origin/None) and two new tests that prove gap signaling on a future cursor against an empty stream and against a populated stream. 2. RuntimeEvent deserialize bypassed sanitize_error_kind. Public error_kind: Option<String> with derived Deserialize meant a hand-rolled or replayed JSONL record could put a raw secret into the field without going through the typed constructors. Adds a custom deserialize_with hook that re-runs sanitize_error_kind on inbound values so the redaction guard fires on every code path, including JSONL replay. New caller-level test deserialize_runtime_event_resanitizes_error_kind proves it. 3. sanitize_error_kind allowed token-shaped secrets. Previous allowlist [a-z0-9_-.:] up to 128 bytes accepted real-world API key shapes like sk_live_<32 alphanumerics>. Tightens to require leading lowercase letter, drops dash from the body alphabet, caps overall length at 64 bytes, caps each .:-separated segment at 24 bytes, and requires every segment to start with a lowercase letter. Test sanitize_error_kind_collapses_long_or_unsafe_input expanded to cover token shapes, leading digits, leading underscores, mixed-case identifiers, and oversized random segments. 4. Cursor overflow on append. StreamState::append now uses checked_add and returns EventError::DurableLog on u64 exhaustion rather than panicking on wrap. Append signature is now Result<EventLogEntry<T>, EventError>. 5. ReplayGap path was untested. Added pub truncate_before_or_at(stream, cursor) on both in-memory logs that drops entries up to and including the supplied cursor and advances earliest_retained, matching the retention model the contract describes. New test replay_gap_after_truncation_forces_snapshot_rebase exercises the gap-after-retention path end to end. 6. Audit redaction lacked a serialization assertion. New test approval_audit_envelope_serialization_excludes_raw_reason_and_fingerprint builds an ApprovalRequest with a unique sentinel `reason`, runs it through AuditEnvelope::approval_resolved, and asserts the JSON serialization does not contain the sentinel or an invocation_fingerprint field. 7. Serde derives on EventStreamKey, EventLogEntry<T>, EventReplay<T> for gateway/SSE consumers. 8. EventSink/AuditSink doc comments now state the best-effort contract loudly: callers must not `?`-propagate sink errors and must continue with their original outcome. Type-level enforcement is flagged as a follow-up. Test count: 18 -> 23. All pass; cargo fmt, clippy with -D warnings, RUSTDOCFLAGS=-D warnings cargo doc, and ironclaw_architecture boundary test all green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address Firat's review on #2993 plus the codex-bot P1 finding on replay_jsonl. All three issues turn on the same theme: the substrate must enforce its invariants on every wire/cursor crossing, not rely on caller discipline. 1. replay_jsonl accepted future cursors (Codex P1 / Firat HIGH). The byte-level helper had the same silent-replay-loss bug as the in-memory log: a cursor beyond the file head returned an empty replay with the cursor echoed back, hiding records 1..head once they appeared. A future filesystem JSONL backend built on this helper would inherit the bug. Now mirrors InMemoryDurableEventLog: after scanning, if requested > current_cursor, return ReplayGap. New test replay_jsonl_with_future_cursor_returns_replay_gap pins it. 2. RuntimeEvent direct-construction Serialize path bypassed redaction (Firat HIGH). The previous commit added a Deserialize hook that re-runs sanitize_error_kind on inbound payloads, but the public fields plus derived Serialize meant any in-process caller could build RuntimeEvent { error_kind: Some(raw_path), .. } directly and persist the raw value through any to_json/append path. Custom Serialize impl now re-runs sanitize_error_kind on the way out, symmetric with the Deserialize hook. Both wire directions are enforced. Test direct_construction_serialize_path_resanitizes_error_kind builds a RuntimeEvent with a path-shaped error_kind directly and asserts the serialized JSON contains "Unclassified" rather than the raw value. 3. EventStreamKey omitted project/mission/thread/process scope, leaving read authorization to caller discipline (Firat MEDIUM). EventStreamKey continues to partition cursors by (tenant, user, agent) per the durable-log path contract. But within a stream, multiple projects/missions/threads/processes co-exist, and a project-scoped consumer must not see other projects' events. The previous trait relied on every caller to remember to post-filter — exactly the kind of authority boundary that produces leaks when a new caller forgets. Adds a typed ReadScope filter required by DurableEventLog and DurableAuditLog read_after_cursor: async fn read_after_cursor( &self, stream: &EventStreamKey, filter: &ReadScope, after: Option<EventCursor>, limit: usize, ) -> Result<EventReplay<...>, EventError>; ReadScope { project_id, mission_id, thread_id, process_id }, all optional. ReadScope::any() disables filtering for tests/admin paths. Filter is a tightening: a record's None field cannot match a filter that asks for Some(...). Implementations enforce the filter; cursors advance through filtered-out records so consumers resume cleanly. Tests: - read_scope_filter_isolates_project_within_same_stream appends three same-stream events across two project_ids and asserts a project-A-scoped read sees only project A and the cursor advances past the project-B record. - read_scope_filter_excludes_records_with_none_field_when_filter_is_some pins the tightening semantics. Test count: 23 -> 27. fmt, clippy with -D warnings, RUSTDOCFLAGS=-D warnings cargo doc, and ironclaw_architecture boundary test all green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…oc drift - truncate_before_or_at: reject cursors past stream head with InvalidReplayRequest. Without the guard, a misuse (e.g. calendar-time retention on a quiet stream) could push earliest_retained > next_cursor and brick the stream until enough appends caught up. - replay_jsonl: document the uncompacted-JSONL assumption so future filesystem backends do not silently desync line index from logical cursor after compaction. - ironclaw_events/CLAUDE.md: drop filesystem and memory from the forbidden-deps list to match the actual BoundaryRule, and call out filesystem as deliberately-allowed for PR2's JSONL sink.
3ed34b7 to
686390c
Compare
* feat(reborn): durable event/audit substrate Adds ironclaw_events as a Reborn substrate crate carved from PR2 to land ahead of the filesystem grouped slice, per the integration plan in nearai#2987 and the prior tracking comment. Includes: - RuntimeEvent / RuntimeEventKind with redaction-aware constructors and sanitize_error_kind helper that collapses unsafe error detail into the Unclassified token. Approval-specific event kinds are deliberately absent; approval resolution is recorded as a control-plane AuditEnvelope per events.md. - EventSink and AuditSink (best-effort delivery) async traits whose failures must not alter runtime/control-plane outcomes. - DurableEventLog and DurableAuditLog (explicit-error append + scoped read_after_cursor) async traits. Append failures are propagated; a cursor older than the earliest retained record yields EventError::ReplayGap so transports request a snapshot/rebase rather than silently lose history. - EventStreamKey = (tenant_id, user_id, agent_id) partition; per-stream monotonic EventCursor; EventLogEntry / EventReplay shapes; explicit zero-limit rejection. - InMemoryEventSink / InMemoryAuditSink for best-effort delivery. - InMemoryDurableEventLog / InMemoryDurableAuditLog for caller-level tests across PR3/PR4/PR6 substrate work. - parse_jsonl / replay_jsonl byte-level helpers exposed for downstream filesystem-backed JSONL backends in PR2. The crate has zero ironclaw_filesystem dependency; the JSONL backend is deliberately deferred to the filesystem grouped slice. PostgreSQL/libSQL backends follow with the database substrates. The byte-level helpers let those later backends share redaction and replay invariants. Caller-level tests cover full append -> cursor -> read_after_cursor loops, per-stream partitioning across (tenant, user, agent) tuples, limit/resume semantics, redaction guarantees on dispatch_failed, audit envelope replay across stream keys, JSONL round-trip, and explicit malformed-line and zero-limit rejection. Architecture boundary tests for ironclaw_events were already present at crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs and remain green with the new crate in the workspace. Verification: cargo fmt --check cargo test -p ironclaw_events cargo test -p ironclaw_architecture cargo clippy -p ironclaw_events --all-targets -- -D warnings RUSTDOCFLAGS='-D warnings' cargo doc -p ironclaw_events --no-deps cargo tree -p ironclaw_events -e normal All pass; the only ironclaw_ normal dependency is ironclaw_host_api. No production wiring; feature-gated free; src/ untouched. Tracking issue nearai#2987. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(reborn): close redaction and replay-gap holes flagged in code review Address contract-level issues raised in the Codex code review of nearai#2993: 1. Silent replay loss on future cursor. read_after_cursor previously echoed back any caller-supplied cursor for a stream whose head was lower (or for an absent stream), losing every event 1..cursor when those events later landed. The contract requires explicit ReplayGap signaling so consumers fetch a snapshot/rebase rather than silently miss history. StreamState::read_after now returns ReplayGap when after > head, and InMemoryDurableEventLog/InMemoryDurableAuditLog return ReplayGap on non-origin cursors against absent streams. The earlier test that pinned the wrong behavior is split into the correct empty-stream case (origin/None) and two new tests that prove gap signaling on a future cursor against an empty stream and against a populated stream. 2. RuntimeEvent deserialize bypassed sanitize_error_kind. Public error_kind: Option<String> with derived Deserialize meant a hand-rolled or replayed JSONL record could put a raw secret into the field without going through the typed constructors. Adds a custom deserialize_with hook that re-runs sanitize_error_kind on inbound values so the redaction guard fires on every code path, including JSONL replay. New caller-level test deserialize_runtime_event_resanitizes_error_kind proves it. 3. sanitize_error_kind allowed token-shaped secrets. Previous allowlist [a-z0-9_-.:] up to 128 bytes accepted real-world API key shapes like sk_live_<32 alphanumerics>. Tightens to require leading lowercase letter, drops dash from the body alphabet, caps overall length at 64 bytes, caps each .:-separated segment at 24 bytes, and requires every segment to start with a lowercase letter. Test sanitize_error_kind_collapses_long_or_unsafe_input expanded to cover token shapes, leading digits, leading underscores, mixed-case identifiers, and oversized random segments. 4. Cursor overflow on append. StreamState::append now uses checked_add and returns EventError::DurableLog on u64 exhaustion rather than panicking on wrap. Append signature is now Result<EventLogEntry<T>, EventError>. 5. ReplayGap path was untested. Added pub truncate_before_or_at(stream, cursor) on both in-memory logs that drops entries up to and including the supplied cursor and advances earliest_retained, matching the retention model the contract describes. New test replay_gap_after_truncation_forces_snapshot_rebase exercises the gap-after-retention path end to end. 6. Audit redaction lacked a serialization assertion. New test approval_audit_envelope_serialization_excludes_raw_reason_and_fingerprint builds an ApprovalRequest with a unique sentinel `reason`, runs it through AuditEnvelope::approval_resolved, and asserts the JSON serialization does not contain the sentinel or an invocation_fingerprint field. 7. Serde derives on EventStreamKey, EventLogEntry<T>, EventReplay<T> for gateway/SSE consumers. 8. EventSink/AuditSink doc comments now state the best-effort contract loudly: callers must not `?`-propagate sink errors and must continue with their original outcome. Type-level enforcement is flagged as a follow-up. Test count: 18 -> 23. All pass; cargo fmt, clippy with -D warnings, RUSTDOCFLAGS=-D warnings cargo doc, and ironclaw_architecture boundary test all green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(reborn): close remaining redaction and replay leaks from PR review Address Firat's review on nearai#2993 plus the codex-bot P1 finding on replay_jsonl. All three issues turn on the same theme: the substrate must enforce its invariants on every wire/cursor crossing, not rely on caller discipline. 1. replay_jsonl accepted future cursors (Codex P1 / Firat HIGH). The byte-level helper had the same silent-replay-loss bug as the in-memory log: a cursor beyond the file head returned an empty replay with the cursor echoed back, hiding records 1..head once they appeared. A future filesystem JSONL backend built on this helper would inherit the bug. Now mirrors InMemoryDurableEventLog: after scanning, if requested > current_cursor, return ReplayGap. New test replay_jsonl_with_future_cursor_returns_replay_gap pins it. 2. RuntimeEvent direct-construction Serialize path bypassed redaction (Firat HIGH). The previous commit added a Deserialize hook that re-runs sanitize_error_kind on inbound payloads, but the public fields plus derived Serialize meant any in-process caller could build RuntimeEvent { error_kind: Some(raw_path), .. } directly and persist the raw value through any to_json/append path. Custom Serialize impl now re-runs sanitize_error_kind on the way out, symmetric with the Deserialize hook. Both wire directions are enforced. Test direct_construction_serialize_path_resanitizes_error_kind builds a RuntimeEvent with a path-shaped error_kind directly and asserts the serialized JSON contains "Unclassified" rather than the raw value. 3. EventStreamKey omitted project/mission/thread/process scope, leaving read authorization to caller discipline (Firat MEDIUM). EventStreamKey continues to partition cursors by (tenant, user, agent) per the durable-log path contract. But within a stream, multiple projects/missions/threads/processes co-exist, and a project-scoped consumer must not see other projects' events. The previous trait relied on every caller to remember to post-filter — exactly the kind of authority boundary that produces leaks when a new caller forgets. Adds a typed ReadScope filter required by DurableEventLog and DurableAuditLog read_after_cursor: async fn read_after_cursor( &self, stream: &EventStreamKey, filter: &ReadScope, after: Option<EventCursor>, limit: usize, ) -> Result<EventReplay<...>, EventError>; ReadScope { project_id, mission_id, thread_id, process_id }, all optional. ReadScope::any() disables filtering for tests/admin paths. Filter is a tightening: a record's None field cannot match a filter that asks for Some(...). Implementations enforce the filter; cursors advance through filtered-out records so consumers resume cleanly. Tests: - read_scope_filter_isolates_project_within_same_stream appends three same-stream events across two project_ids and asserts a project-A-scoped read sees only project A and the cursor advances past the project-B record. - read_scope_filter_excludes_records_with_none_field_when_filter_is_some pins the tightening semantics. Test count: 23 -> 27. fmt, clippy with -D warnings, RUSTDOCFLAGS=-D warnings cargo doc, and ironclaw_architecture boundary test all green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(reborn): address PR2993 review — truncate guard, jsonl warning, doc drift - truncate_before_or_at: reject cursors past stream head with InvalidReplayRequest. Without the guard, a misuse (e.g. calendar-time retention on a quiet stream) could push earliest_retained > next_cursor and brick the stream until enough appends caught up. - replay_jsonl: document the uncompacted-JSONL assumption so future filesystem backends do not silently desync line index from logical cursor after compaction. - ironclaw_events/CLAUDE.md: drop filesystem and memory from the forbidden-deps list to match the actual BoundaryRule, and call out filesystem as deliberately-allowed for PR2's JSONL sink. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Adds
ironclaw_eventsas a Reborn substrate crate carved from PR2 to land ahead of the filesystem grouped slice, per the integration plan in #2987 and the prior carve proposal in #issuecomment-4329311786.This is the durable-log substrate the host runtime, dispatcher, process manager, and approval resolver will depend on for caller-level tests in PR3/PR4/PR6, and the foundation the JSONL/PostgreSQL/libSQL backends in later grouped PRs build on.
Tracking issue:
Target branch:
Stacked on:
This PR is opened as draft because it depends on PR1 merging into
reborn-integrationfirst; the diff againstreborn-integrationwill shrink to just theironclaw_eventscrate plus workspace wiring once PR1 lands.Carve note
The
reborn-predispatch-obligationsstack already had a substantialironclaw_eventsslice; this PR carves the contract substrate out of that work into a filesystem-free crate so it can land in PR2's first half and unblock downstream substrate work without the filesystem-backed JSONL sink. The byte-levelparse_jsonl/replay_jsonlhelpers are keptpubso PR2'sJsonlEventSink<F: RootFilesystem>can build on the same redaction and replay invariants without duplicating logic.The two responsibilities the existing stack bundled — best-effort sinks (
EventSink/AuditSink) and explicit-error durable logs — are split here. The freeze packet's two-tier reliability model (sink failures best-effort vs. durable append failures explicit) becomes a type-level distinction rather than a per-callsite convention.Included
ironclaw_eventsRuntimeEvent/RuntimeEventKind(8 kinds:DispatchRequested,RuntimeSelected,DispatchSucceeded,DispatchFailed,ProcessStarted,ProcessCompleted,ProcessFailed,ProcessKilled).sanitize_error_kindcollapsing any unsafe value into the stableUnclassifiedtoken.AuditEnvelopeperevents.md§1.EventSinkandAuditSinkasync traits (best-effort delivery) — failures must not alter runtime/control-plane outcomes.DurableEventLogandDurableAuditLogasync traits (explicit-error append + scopedread_after_cursor).EventError::ReplayGapso transports request a snapshot/rebase rather than silently lose history (perevents-projections.md§6 and the freeze addendum).EventStreamKey = (tenant_id, user_id, agent_id)partition; per-stream monotonicEventCursor;EventLogEntry/EventReplayshapes.EventError::InvalidReplayRequest).InMemoryEventSink/InMemoryAuditSink(best-effort) andInMemoryDurableEventLog/InMemoryDurableAuditLog(durable, per-stream cursors) for caller-level tests.parse_jsonl/replay_jsonlbyte-level helpers for downstream JSONL backends.Out of scope
JsonlEventSink<F: RootFilesystem>andJsonlAuditSink<F: RootFilesystem>— deferred to PR2's filesystem half.scoped_runtime_event_log_path/scoped_audit_log_pathfilesystem-aware path helpers — those compose withRootFilesystemand belong in PR2.Exposure checklist
src/runtime behavior.src/writers exist for these types yet).src/until later grouped PRs wire it in.Architecture boundary
ironclaw_eventsalready had aBoundaryRuleincrates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs(lines 116-134) forbidding deps on auth/approvals/capabilities/dispatcher/extensions/host_runtime/secrets/network/mcp/processes/resources/run_state/scripts/wasm. That rule is now active with the crate present, andcargo test -p ironclaw_architecturepasses.cargo tree -p ironclaw_events -e normalconfirms the onlyironclaw_*normal dependency isironclaw_host_api.Acceptance tests (caller-level)
Per the freeze packet's "test through the caller, not just the helper" rule, all tests drive the public trait surfaces, not internal helpers:
durable_event_log_appends_and_replays_in_order— full append → read_after_cursor loop with cursor monotonicity.read_after_next_cursor_returns_empty_replay— cursor-resume idempotence.replay_respects_limit_and_resumes_cleanly— bounded replay across multiple resumes.streams_partition_by_tenant_user_agent— per-stream cursor isolation acrossEventStreamKeytuples.read_empty_stream_returns_echoed_cursor— empty stream returns echoed cursor for clean resume.replay_with_zero_limit_is_rejected/replay_jsonl_with_zero_limit_is_rejected— explicit invalid-request rejection.dispatch_failed_redacts_unsafe_error_kind— raw error message containing paths/secrets collapses toUnclassified.dispatch_failed_preserves_safe_classification_token— short stable tokens survive sanitization.sanitize_error_kind_collapses_long_or_unsafe_input— direct sanitizer pinning.appended_event_payload_omits_raw_payloads_by_construction— wire-shape pinning.best_effort_event_sink_records_emit_calls/best_effort_audit_sink_captures_records— best-effort delivery.durable_audit_log_appends_and_replays—AuditEnvelope::denied(...)round-trip.approval_audit_records_partition_by_stream_key— approval-resolution audit cross-stream isolation.parse_jsonl_round_trips_runtime_events— JSONL round-trip.parse_jsonl_rejects_malformed_line_rather_than_silently_skipping— explicit error rather than silent empty history (events.md§5).replay_jsonl_advances_cursor_with_limit— JSONL cursor semantics.18 tests, all passing.
Verification
All pass.
Open coordination questions
These don't block this PR but are flagged for the team:
EventCursor(u64)+ per-stream monotonic + explicitReplayGapsignaling). Treat this PR as the ratification proposal; tighten in review.(tenant_id, user_id, agent_id)matching the existingscoped_runtime_event_log_pathconvention. Deeper scope filtering (project/mission/thread/process/invocation) is intentionally a read-side filter rather than a separate cursor.ironclaw_filesystemboundary rule. The existingBoundaryRuleforironclaw_eventsdoes not includeironclaw_filesystem. PR2's JSONL sink will need that dep, so leaving it out preserves PR2's freedom; if the team prefers a separateironclaw_events_jsonlcrate, the boundary can tighten then.rebornfeature should be defined in PR1 and applied uniformly when wiring begins.🤖 Generated with Claude Code