Conversation
Adds docs/rfc/contract-ledger.md capturing the five fresh decisions agreed in the proposal comment on Q00#513 plus two decisions inherited from the Mesh and Disposable Memory RFCs. The ledger is a projection over the existing event_store rather than a new authoritative store; ac_tree (static AC decomposition) and a new contract.dependency.recorded event stream (dynamic, additive edge_type vocabulary) live as two separate graphs with separate accessors; raw control.directive.emitted events stay 1:1 while the projector exposes per-contract aggregates; the replay API is layered into replay_events, replay_state, and replay_timeline, all reading from the same journal, and replay never re-executes LLM calls or re-forks sub-agents; the migration plan is five additive steps with opt-in backfill, so 1.0 ships without anyone running a destructive migration. The ledger explicitly does not absorb Q00#338's CheckpointStore. Docs-only; zero runtime impact. Closes Q00#513. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![ouroboros-agent[bot]](https://avatars.githubusercontent.com/in/3091302?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
4527aa2for PR #522
Review record:
47f5e232-7c76-44fe-95a7-79763305ef31
Blocking Findings
| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|-----------|----------|---------|
| 1 | docs/rfc/contract-ledger.md:34 | BLOCKING | The RFC says events/control.py is the “authoritative source” for a target_type vocabulary of lineage / execution / session / agent_process, but the actual event contract is intentionally open-ended: src/ouroboros/events/control.py states the field is a free-form string, and tests/unit/events/test_control_events.py locks in forward-compatibility for new target types without schema changes. Freezing a closed vocabulary here would mislead ledger/projector implementers into validating or hard-coding an enum that the underlying event schema explicitly does not guarantee. |
| 2 | docs/rfc/contract-ledger.md:32 | BLOCKING | This “Accepted” RFC treats mesh.md D2/D6 and disposable-memory.md C2/C5 as normative inherited constraints for contract_id, artifact_ref, and replay semantics, but those referenced RFC files are not present in this snapshot at all (docs/rfc/ contains only contract-ledger.md). That leaves the accepted spec incomplete inside the repository: contributors cannot verify the inherited contract the document tells them to implement, and several cross-references in the body/checklist are broken as written. |
Non-blocking Suggestions
None.
Design Notes
The projection-first direction is coherent with the existing event-store architecture, and separating AC structure from runtime contract dependencies is a good boundary. The main problem is spec hygiene: this RFC currently overstates one existing contract (target_type) and depends on missing upstream RFCs for key invariants.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Constraint: Addressed current blocking PR review findings without widening the feature scope. Confidence: high Scope-risk: narrow Directive: Preserve the new regression tests when modifying this contract. Tested: Documentation-only; grep confirmed no dead relative RFC links remain under docs/rfc. Not-tested: Full repository matrix; CI remains authoritative.
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
bae07affor PR #522
Review record:
4319baf1-c3bf-40b0-bf9b-05b53bed139b
Blocking Findings
| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|-----------|----------|---------|
| 1 | docs/rfc/contract-ledger.md:126 | BLOCKING | L7 says adding the new contract.* event family requires an event_version bump, but the current event contract says the opposite: new event types are additive and only payload removals/renames should bump event_version ([docs/events.md:105-117] and [src/ouroboros/events/base.py:97-110]). If implementers follow this RFC, unchanged event types will start deserializing as a new global version and consumers are explicitly told to fail on unsupported versions, creating an avoidable compatibility break. |
| 2 | docs/rfc/contract-ledger.md:177 | BLOCKING | The ER diagram models event_version as a top-level EVENT_STORE field, but the actual store keeps it inside the JSON payload, not as a database column ([docs/events.md:25-39], [src/ouroboros/events/base.py:99-117]). That is not just cosmetic: it points implementers toward a schema change the RFC elsewhere says should be unnecessary/additive-only. |
| 3 | docs/rfc/contract-ledger.md:214 | BLOCKING | The RFC promises contract-rooted replay/audit APIs (ledger.replay_events(contract_id), contract_audit_trail(contract_id)), yet its own lifecycle example still writes control.directive.emitted to target=lineage/<id>. In the current implementation, directive events are aggregated by (target_type, target_id) ([src/ouroboros/events/control.py:13-18,76-80]). Without an explicit rule that directives for a contract must target contract_id (or another deterministic join contract), the projector cannot reliably recover a complete per-contract directive stream and will silently omit parts of the audit trail. |
Non-blocking Suggestions
| 1 | docs/rfc/contract-ledger.md:121 | Documentation | Step 1 says the projector “works against existing data immediately,” while the L7 goal says historical data becomes Ledger-aware only after opt-in backfill. Clarifying whether Step 1 supports only partial read-only projections, or whether pre-backfill history is intentionally unsupported, would remove avoidable ambiguity for implementers. |
Design Notes
The projection-first shape is directionally sound and stays aligned with the existing event-sourced architecture, but the RFC currently has two schema-contract mismatches and one unresolved identity/join problem around directive aggregation. Those need to be made internally consistent before this can safely guide implementation.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
Autopilot follow-up: pushed review-addressing commits and confirmed the GitHub check suite is green on the latest head. Current merge state is clean; remaining CHANGES_REQUESTED status appears to be awaiting fresh bot/human re-review rather than failing CI. |
Constraint: RFC guidance must match additive event-version semantics and contract-rooted replay requirements. Rejected: Treating new contract.* events as an event_version bump | new event families are additive and do not change existing payload contracts. Confidence: high Scope-risk: narrow Directive: Emit contract lifecycle directives with contract targets when they must appear in contract replay. Tested: git diff --check; grep -n "event_version bump\|target=lineage\|target=contract" docs/rfc/contract-ledger.md Not-tested: Markdown render preview; CI remains authoritative.
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
2fe9620for PR #522
Review record:
bb9231d9-e199-4ae5-873b-df408610706f
Blocking Findings
| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|-----------|----------|---------|
| 1 | docs/rfc/contract-ledger.md:125 | BLOCKING | L7’s rollout steps only add the contract.* event family, but L5/L6 require contract-rooted replay to be built from control.directive.emitted events targeted at the contract, and the sequence section makes that a MUST. As written, an implementation can follow Step 2 exactly and still emit directives only at lineage/session scope, which means ledger.replay_timeline(contract_id) will be incomplete for every new contract from day one. The migration plan needs an explicit contract-targeting change for directive emission, not just new contract.* factories. |
| 2 | docs/rfc/contract-ledger.md:151 | BLOCKING | The backfill mapping is internally inconsistent about contract identity. One row says “one contract per lineage generation”, the next says “one contract per execution”, and the execution mapping uses execution_id + ULID seed, which is not deterministic. For historical events that carry both fields, different implementations can synthesize different contract boundaries; rerunning backfill can also mint new contract_ids for the same history. That is a data-correctness problem for an append-only journal and needs a single deterministic precedence/derivation rule. |
Non-blocking Suggestions
None.
Design Notes
The projection-first direction is coherent and stays aligned with the journal-as-source-of-truth model. The main weakness is that the migration/backfill section does not yet define a deterministic event-to-contract mapping tightly enough to be safely implemented.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Constraint: Contract ledger migration guidance must produce complete contract-rooted replay from day one and idempotent historical backfill. Rejected: Fresh ULID-seeded synthetic contract IDs | rerunning backfill could mint different identities for the same journal history. Confidence: high Scope-risk: narrow Directive: Use contract-targeted directive emissions and deterministic synthetic contract keys for backfill. Tested: git diff --check; grep -n "contract-targeted\|target_type=contract\|uuid5\|legacy:lineage\|backfill:ambiguous" docs/rfc/contract-ledger.md Not-tested: Markdown render preview; CI remains authoritative.
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
663fbb0for PR #522
Review record:
4c7c0791-cba0-44fb-8a49-1264189d035f
Blocking Findings
| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|-----------|----------|---------|
| 1 | docs/rfc/contract-ledger.md:160 | BLOCKING | The backfill section changes contract_id format from the RFC’s inherited ULID contract to uuid5(...). That is an API contract break inside the same document: any projector, CLI, or storage keyed on contract_id = ULID will either reject backfilled records or lose the stated sortable/log-friendly semantics. The RFC needs a backfill identity that still satisfies the ULID contract, or it needs to define a distinct legacy key field instead of overloading contract_id. |
Non-blocking Suggestions
| 1 | docs/rfc/contract-ledger.md:34 | documentation | The “known current producers” list includes agent_process, but the referenced source still describes that target type as forward-compatible rather than current. Tightening that wording would avoid implementers inferring existing producers that are not wired yet. |
Design Notes
The overall direction is coherent: projection-over-journal, separate AC and contract graphs, and opt-in backfill all fit the existing event-sourcing model. The main issue is that the migration section currently violates the document’s own identifier contract, which would force downstream implementations into incompatible behavior.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Constraint: Contract ledger backfill must be deterministic without violating the inherited contract_id equals ULID API. Rejected: uuid5 contract identifiers | they break the same RFC's sortable ULID contract. Confidence: high Scope-risk: narrow Directive: Store human-readable legacy keys separately from ULID contract_id values. Tested: git diff --check; grep -n "deterministic ULID\|extra.legacy_key\|forward-compatible targets" docs/rfc/contract-ledger.md Not-tested: Markdown render preview; CI remains authoritative.
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
dd865f9for PR #522
Review record:
c1227608-5342-4990-a8cf-b810314da777
Blocking Findings
| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|-----------|----------|---------|
| 1 | docs/rfc/contract-ledger.md:95 | BLOCKING | L6 promises ledger.replay_events(contract_id) as a raw contract event stream, but the migration plan only introduces contract.* envelope events and retargeted control.directive.emitted events. The rest of the runtime stream is still shown as tool.call.* / llm.call.* with no contract_id association, and the current EventStore replay API only reads a single (aggregate_type, aggregate_id) pair. As written, implementers cannot actually return the full raw event history for a contract, so this RFC would lead to an incomplete or misleading replay contract unless it also requires contract correlation on those runtime event families or narrows Layer 0 to a projected view. |
Non-blocking Suggestions
None.
Design Notes
The projection-first direction is coherent and stays aligned with the existing journal-as-source-of-truth model. The main design gap is that the replay API claims a stronger contract-rooted raw stream than the specified event model can currently support.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Constraint: Raw contract replay must be implementable over the existing append-only EventStore without pretending all events share one aggregate. Rejected: Mapping replay_events directly to EventStore.replay(contract, id) | it would omit runtime I/O events outside the contract aggregate. Confidence: high Scope-risk: narrow Directive: Carry contract correlation on runtime I/O events before promising them in contract replay. Tested: git diff --check; grep -n "Layer 0 query rule\|contract_id/correlation_id\|contract-correlated" docs/rfc/contract-ledger.md Not-tested: Markdown render preview; CI remains authoritative.
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
a6d263efor PR #522
Review record:
4e2a4382-4bae-480f-8e38-0559106bb302
Blocking Findings
| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|-----------|----------|---------|
| 1 | docs/rfc/contract-ledger.md:106 | BLOCKING | Layer 0 promises that ledger.replay_events(contract_id) returns cross-aggregate events “in journal order”, but the current EventStore API only exposes timestamp-based ordering (replay() orders by timestamp, id in src/ouroboros/persistence/event_store.py:340) and has no stable append sequence for merging contract.*, directive, and runtime events from different aggregates. That means two same-timestamp or skewed events can be replayed in the wrong causal order, producing an incorrect contract timeline. The RFC needs to define a real journal cursor/ordering primitive, or weaken the guarantee. |
| 2 | docs/rfc/contract-ledger.md:82 | BLOCKING | L5 says control.directive.emitted stays raw 1:1, but L7 Step 2 later says lineage/session targets “may still be emitted as secondary views” (line 138). The existing event factory aggregates each directive event by exactly one (target_type, target_id) pair (src/ouroboros/events/control.py:13, src/ouroboros/events/control.py:76), so supporting both contract and lineage/session views would require duplicating rows for the same decision. That breaks the document’s own 1:1 audit invariant and will mislead implementers about whether duplicate directive events are acceptable. |
Non-blocking Suggestions
None.
Design Notes
The RFC’s overall direction is coherent: projection-over-journal, separate AC vs contract graphs, and opt-in backfill all fit the existing architecture. The blocking issue is that two core behavioral contracts are still underspecified or internally inconsistent, so the document is not yet safe as accepted implementation guidance.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Constraint: Contract ledger replay must not promise causal cross-aggregate ordering until the EventStore has a journal cursor that supports it. Rejected: Duplicating directive rows for secondary lineage views | it violates the raw 1:1 audit invariant. Confidence: high Scope-risk: narrow Directive: Use extra correlation for secondary context and reserve causal ordering for projected replay timelines. Tested: git diff --check; grep -n "canonical order\|not a strict causal\|written once" docs/rfc/contract-ledger.md Not-tested: Markdown render preview; CI remains authoritative.
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
7d2c712for PR #522
Review record:
b256acae-551c-444d-b96e-a5e037743966
Blocking Findings
| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|-----------|----------|---------|
| 1 | docs/rfc/contract-ledger.md:137 | BLOCKING | This RFC changes a documented event contract in a way that would regress existing aggregate-based replay. It requires contract-serving control.directive.emitted rows to target only contract/<contract_id> and pushes lineage/session association into extra, but the current control-event design explicitly relies on aggregate_(type,id) = (target_type,target_id) so lineage/execution/session projectors naturally see directives during ordinary aggregate replay (src/ouroboros/events/control.py:13-18, 76-80). Unless the RFC also defines how non-contract timelines keep working from those aggregates, future implementations following this spec will make existing `EventStore.replay("lineage" |
Non-blocking Suggestions
| 1 | docs/rfc/contract-ledger.md:112 | Documentation | “Canonical order (timestamp, then row id)” does not match the current wording in EventStore.replay, which orders by timestamp then the event id column (src/ouroboros/persistence/event_store.py:367-369). Clarifying that this is the event UUID column, not SQLite rowid, would avoid an implementation mismatch. |
Design Notes
The RFC is mostly disciplined about keeping the ledger as a projection over event_store and separating AC structure from runtime dependency edges. The main unresolved design problem is that the contract-rooted directive targeting rule conflicts with the existing aggregate-oriented replay model instead of extending it cleanly.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Constraint: Contract replay must extend existing aggregate replay instead of retargeting directive rows away from lineage/session timelines. Rejected: Emitting secondary directive rows for contract views | duplicates violate the raw 1:1 audit invariant. Confidence: high Scope-risk: narrow Directive: Put contract correlation in extra unless a directive is natively contract-scoped. Tested: git diff --check; grep -n "extra.contract_id\|event UUID\|natural aggregate" docs/rfc/contract-ledger.md Not-tested: Markdown render preview; CI remains authoritative.
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
d4ca657for PR #522
Review record:
3f0cabe7-bb3b-4716-ac13-a9ab44ce8f7b
Blocking Findings
| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|-----------|----------|---------|
| 1 | docs/rfc/contract-ledger.md:106 | BLOCKING | Layer 0 replay is defined to return only rows that already carry contract_id/correlation_id, but the backfill plan in L7 only appends synthetic contract.* envelope events for historical data. That means historical directive/runtime rows that have only lineage_id/execution_id remain invisible to ledger.replay_events(contract_id) even after backfill, so the RFC’s “historical event_store data becomes Ledger-aware” goal is not actually met for raw replay. As written, an implementation can follow the spec exactly and still return incomplete contract histories for migrated data. |
| 2 | docs/rfc/contract-ledger.md:167 | BLOCKING | The RFC claims rerunning backfill is idempotent, but it only makes the derived contract_id deterministic; it does not define deterministic synthetic event IDs or any dedupe/upsert rule for the appended contract.* rows. In the current model, each persisted event gets a fresh UUID and the journal is append-only, so a second backfill run would append duplicate synthetic contract.created/completed/dependency events for the same contract and change replay results. This needs an explicit deduplication mechanism before the migration contract is safe. |
Non-blocking Suggestions
None.
Design Notes
The overall direction is coherent: keeping the ledger as a projection over event_store preserves the existing source-of-truth model and cleanly separates AC structure from runtime contract dependencies. The main problem is that the migration section currently overpromises replay correctness/idempotence relative to the append-only event model.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Constraint: Backfilled contract replay must include historical source rows and avoid duplicate synthetic envelopes on repeated runs. Rejected: Relying only on synthetic contract envelope events | historical directive and runtime rows would remain invisible to raw replay. Confidence: high Scope-risk: narrow Directive: Query historical rows through deterministic backfill keys and dedupe synthetic events by type, contract_id, and backfill_key. Tested: git diff --check Not-tested: Markdown render preview; CI remains authoritative.
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
35d427bfor PR #522
Review record:
04f90cc8-e5bd-4746-a48b-aa8b2e04e345
Blocking Findings
| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|-----------|----------|---------|
| 1 | docs/rfc/contract-ledger.md:177 | BLOCKING | L6 defines ledger.replay_events() ordering by the stored event row timestamp and UUID, but the backfill spec only makes the synthetic contract_id deterministic; it never specifies the persisted timestamps for the synthetic contract.* rows. If an implementation appends those rows with backfill-time timestamps, contract.created / contract.completed will replay after the historical directive and tool rows they are supposed to bracket, producing an incorrect raw audit slice for every backfilled contract. The RFC needs to require historical timestamps for synthetic envelope rows (or otherwise define how Layer 0 excludes/reorders them). |
| 2 | docs/rfc/contract-ledger.md:150 | BLOCKING | Step 3 says backfill should infer boundaries from (lineage_id, execution_id), but the authoritative rule immediately below keys lineage-backed synthetic contracts by (lineage_id, generation_number) and the table says there is “one contract per lineage generation.” Those are different partitioning rules: following the Step 3 text would merge multiple generations from the same lineage into one synthetic contract, corrupting replay and audit boundaries for historical data. The step text needs to match the generation-aware rule. |
Non-blocking Suggestions
None.
Design Notes
The RFC’s overall shape is coherent: keeping the ledger as a projection over event_store preserves the existing source-of-truth model and the AC-tree vs runtime-dependency split is well articulated. The main problem is that the migration section still leaves two implementation-defining details inconsistent, which is risky because L7 is effectively the contract future code will follow.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Constraint: Backfilled contract envelopes must bracket historical rows in raw replay and use the same generation-aware boundary rule as the mapping table. Rejected: Backfill-time timestamps | they would replay synthetic envelopes after the history they summarize. Confidence: high Scope-risk: narrow Directive: Use historical boundary timestamps and generation-aware precedence for contract backfill. Tested: git diff --check Not-tested: Markdown render preview; CI remains authoritative.
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
e5ebe22for PR #522
Review record:
aa4b5fd1-e09e-4ee8-adf2-069ad24ec9da
Blocking Findings
| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|-----------|----------|---------|
| 1 | docs/rfc/contract-ledger.md:174 | BLOCKING | The backfill rule collapses every historical row with the same (lineage_id, generation_number) into one synthetic contract. That is lossy against this RFC’s own model, which allows multiple runtime contracts under a generation (parent_ac sub-contracts, retries, parallel child work). Replaying or auditing historical data after this backfill would silently merge distinct contracts into one ledger entry and produce incorrect dependency/timeline results. |
| 2 | docs/rfc/contract-ledger.md:175 | BLOCKING | The fallback execution_id -> legacy:execution:<execution_id> rule is even broader: for any older data missing lineage metadata, it merges an entire execution’s contract activity into one synthetic contract. That makes Layer 0/1 replay materially wrong for historical runs, because separate contract boundaries, terminal states, and dependency edges are no longer recoverable once everything is grouped at execution scope. |
Non-blocking Suggestions
| 1 | docs/rfc/contract-ledger.md:290 | Documentation | The checklist says ambiguous boundaries use provenance="backfill", but the spec body uses provenance="backfill:ambiguous". That should be normalized so implementers and reviewers are checking the same contract. |
| 2 | docs/rfc/contract-ledger.md:9 | Documentation | The summary says the Ledger is built “over the existing event_store, CheckpointStore, and ac_tree”, but later sections repeatedly say the Ledger does not absorb CheckpointStore and all replay layers read from event_store. Tightening that opening sentence would remove avoidable ambiguity. |
Design Notes
The RFC’s projection-first direction is coherent, and the separation between AC structure and runtime contract dependencies is well stated. The main design gap is the historical backfill identity rule: it chooses convenience boundaries (generation, then execution) that are too coarse for the contract model the rest of the RFC defines.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Constraint: Historical ledger backfill must not merge multiple runtime contracts under one broad lineage generation or execution key. Rejected: Generation-only and execution-only synthetic contracts | they silently collapse distinct historical contract activity. Confidence: high Scope-risk: narrow Directive: Require stable invocation evidence for synthetic contracts; otherwise use event-level ambiguous markers. Tested: git diff --check Not-tested: Markdown render preview; CI remains authoritative.
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
2f4eb65for PR #522
Review record:
445f89d7-f835-4470-ab98-c26043bc79a3
Blocking Findings
| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|-----------|----------|---------|
| 1 | docs/rfc/contract-ledger.md:151 | BLOCKING | L7 gives two incompatible backfill precedence rules. Step 3 says to infer boundaries using (lineage_id, generation_number) before execution_id, but the authoritative numbered rule immediately below does the opposite and prefers execution_id + stable invocation id before lineage/generation. An implementer has to choose one, which changes which historical events get grouped into a synthetic contract. |
| 2 | docs/rfc/contract-ledger.md:180 | BLOCKING | The backfill section promises synthetic envelope events can be appended later with historical timestamps without reordering the history they summarize, but the current EventStore replay order is timestamp, id and event IDs are random UUID4s. If a synthetic event reuses the same timestamp as source rows, replay can interleave it before or between those rows arbitrarily, so the RFC is specifying an ordering guarantee the existing store cannot provide. |
| 3 | docs/rfc/contract-ledger.md:111 | BLOCKING | L6 and L7 disagree on what a generation-only backfilled contract contains. replay_events() says Layer 0 includes historical rows whose (lineage_id, generation_number) maps to the same synthetic contract key, but L7 later says that when only lineage/generation evidence exists, the system must “map only the synthetic envelope marker” and must not merge the generation’s rows into one contract. Those two rules produce different replay results for the same historical data. |
Non-blocking Suggestions
| 1 | docs/rfc/contract-ledger.md:9 | documentation | The summary says the ledger combines views over event_store, CheckpointStore, and ac_tree, but the rest of the RFC treats CheckpointStore as explicitly out of scope and not part of the ledger backing. Tightening that sentence would make the boundary clearer for future implementers. |
Design Notes
The RFC is directionally coherent: keeping the ledger as a projection over event_store and separating AC structure from runtime dependency edges are both solid choices. The main issue is that the migration/replay sections still contain contradictory rules, so the document is not yet precise enough to serve as an implementation contract.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Constraint: Backfill guidance must not promise generation-wide raw replay or historical row insertion that EventStore cannot provide. Rejected: Historical synthetic timestamps as ordering guarantees | timestamp plus random UUID cannot deterministically bracket old rows. Confidence: high Scope-risk: narrow Directive: Use stable invocation evidence for synthetic replay and carry historical interval fields for projectors. Tested: git diff --check Not-tested: Markdown render preview; CI remains authoritative.
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
31b251dfor PR #522
Review record:
2c0f1591-4896-401b-95b0-be9209a1d3a0
Blocking Findings
| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|-----------|----------|---------|
| 1 | docs/rfc/contract-ledger.md:109 | BLOCKING | The RFC says contract-serving control.directive.emitted rows should usually keep their existing lineage/session/execution aggregate target, but the current control-event contract is the opposite: directives are aggregated by the object the decision is about (src/ouroboros/events/control.py). For a decision about a contract, storing it under some other aggregate makes the aggregate key semantically wrong and breaks the invariant existing projectors rely on. This needs to be reconciled in the RFC before it becomes the implementation contract. |
| 2 | docs/rfc/contract-ledger.md:110 | BLOCKING | Layer 0 is defined as replay_events(contract_id), but Step 2 allows runtime I/O producers to emit contract_id/correlation_id on those rows. If an event carries only correlation_id, the RFC never defines how native contract replay maps that back to a contract_id, so an implementation can satisfy Step 2 and still silently omit tool/LLM events from contract replay. The spec needs to require contract_id on replayable runtime rows or define the missing correlation-to-contract mapping. |
Non-blocking Suggestions
| 1 | docs/rfc/contract-ledger.md:9 | documentation | The summary says the ledger is built “over the existing event_store, CheckpointStore, and ac_tree”, but later sections repeatedly say the ledger does not absorb CheckpointStore and do not otherwise use it as a projection source. Rewording the summary would avoid implying a tighter CheckpointStore dependency than the RFC actually specifies. |
Design Notes
The RFC has a coherent overall shape: event-store-as-source, projector/materialized-view split, and explicit separation between AC structure and runtime contract dependencies. The main issue is that two core replay rules currently under-specify or contradict existing event contracts, which would make the implementation semantics drift before any code is written.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Constraint: Contract directive events must align aggregate target with the object the decision is about while preserving secondary lineage context without duplicates. Rejected: Using correlation_id as a contract replay substitute | replayable runtime rows need explicit contract_id. Confidence: high Scope-risk: narrow Directive: Target contract decisions at contract aggregates and put lineage/session context in extra correlation fields. Tested: git diff --check Not-tested: Markdown render preview; CI remains authoritative.
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
52b5e8ffor PR #522
Review record:
198389b7-8458-4010-a86f-c9da209bf7ee
Blocking Findings
| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|-----------|----------|---------|
| 1 | docs/rfc/contract-ledger.md:136 | BLOCKING | Step 2 retargets contract-serving control.directive.emitted rows to aggregate_type=\"contract\", but Step 5 later promises that existing direct event_store readers remain valid with “no flag day.” Those two claims are incompatible with the current control-event contract: today projectors discover directives by replaying the target aggregate stream, so lineage/execution/session readers will silently stop seeing contract decisions once those rows move to contract/<contract_id>. This needs a compatibility strategy (for example dual-read support or a phased migration plan) before the RFC can be treated as accepted. |
Follow-up Findings
docs/rfc/contract-ledger.md:66[warning] The L3 accessor is specified asledger.ac_tree(contract_id) -> ACNode, which conflicts with both the existingACTreemodel and the rest of this RFC’s language about exposing the AC tree as a graph/state. As written, implementers cannot tell whether the ledger should return the full decomposition tree, the root node, or the current node for a contract. Because this RFC is an accepted API/spec document, that ambiguity is likely to produce incompatible implementations.
Non-blocking Suggestions
None.
Design Notes
The overall direction is coherent: keeping the ledger as a projection over event_store is consistent with the existing architecture, and the separation between AC structure and runtime contract dependencies is the right boundary. The main gap is migration compatibility: the spec currently changes how directive events are aggregated without preserving the replay contract existing consumers rely on.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Constraint: Contract-targeted directive rows must not imply existing aggregate-only readers see new contract decisions without a migration path. Rejected: Duplicating directive rows for legacy views | it would violate the raw 1:1 audit invariant. Confidence: high Scope-risk: narrow Directive: Use Ledger dual-read helpers for lineage/session views that include contract-targeted decisions. Tested: git diff --check Not-tested: Markdown render preview; CI remains authoritative.
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
c34d39efor PR #522
Review record:
82306832-39a4-4b32-8074-a0aef5834f77
Blocking Findings
| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|-----------|----------|---------|
| 1 | docs/rfc/contract-ledger.md:136 | BLOCKING | L7 changes directive aggregation to contract/<contract_id> in Step 2, but explicitly postpones consumer migration until Step 5. That creates a rollout window where any lineage/session projector or tool still replaying direct event_store aggregates will silently stop seeing those decisions, yielding incomplete audit timelines rather than an obvious failure. Because the current control.directive.emitted contract is built around aggregate-local replay, this needs a stronger migration invariant (for example, dual-read support landing everywhere before emitters switch targets) instead of "slow consumer migration". |
| 2 | docs/rfc/contract-ledger.md:176 | BLOCKING | The backfill key rule is not actually deterministic as written: for execution-scoped history it allows execution_id plus correlation_id, call_id, "or another stable invocation id" without defining precedence when more than one field is present. Two compliant implementations can choose different identifiers for the same historical row set, producing different synthetic contract keys/ULIDs and fragmenting one contract into multiple backfilled histories. Since idempotent backfill is a core guarantee of L7, the RFC needs an exact field-priority rule here. |
Non-blocking Suggestions
None.
Design Notes
The overall direction is coherent: keeping the ledger as a projection over event_store avoids introducing a second source of truth, and the separation between AC structure and runtime contract dependencies is well stated. The main weakness is rollout precision: the migration/backfill sections still leave enough ambiguity to cause silent history gaps or divergent synthetic identities.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Constraint: Bot review required eliminating silent rollout gaps and divergent backfill keys. Rejected: Leaving consumer migration asynchronous | directive audit readers would miss contract-targeted decisions during rollout. Confidence: high Scope-risk: narrow Directive: Keep directive-observing readers dual-read before switching directive emitters to contract aggregates. Tested: Documentation-only change reviewed by diff. Not-tested: Runtime tests not run for RFC text-only edit. Co-authored-by: OmX <omx@oh-my-codex.dev>
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: APPROVE
Reviewing commit
5e51db2for PR #522
Review record:
59e5327e-5b52-405b-8225-8ef3f7f00aa5
Blocking Findings
No in-scope blocking findings remained after policy filtering.
Follow-up Findings
docs/rfc/contract-ledger.md:109[warning] The RFC repeatedly says lineage/session/execution correlation for contract-targetedcontrol.directive.emittedrows should live inextra(:109,:141-147,:254,:268), but the current control-event contract defines those as first-class payload fields (session_id,execution_id,lineage_id,generation_number,phase) rather thanextra-nested data in src/ouroboros/events/control.py. If implementers follow this RFC literally, existing readers and the proposed dual-read helpers will miss correlation unless they are rewritten around a new undocumented shape, which breaks the migration story this RFC is trying to lock down.docs/rfc/contract-ledger.md:110[warning] Layer 0 and the rollout steps key runtime I/O capture offtool.call.*/llm.call.*events (:110,:143-144,:269), but the current runtime event family in-tree isorchestrator.tool.called(src/ouroboros/orchestrator/events.py, src/ouroboros/orchestrator/events.py). Because the RFC presents these names as the concrete implementation hook for Ledger replay, this mismatch is likely to send the follow-up implementation at nonexistent emitters and leave present-day tool activity out of contract replay unless someone catches it later.
Non-blocking Suggestions
None.
Design Notes
The overall direction is coherent: keeping the ledger as a projection over event_store preserves the existing source-of-truth invariant and the separation between AC structure, runtime dependencies, and checkpointing is well argued. The main issue is contract drift in the concrete event-shape details; this RFC needs those payload and event-name references tightened before it becomes a reliable implementation target.
Policy Notes
- No in-scope blocking findings remained after policy filtering; downgraded verdict accordingly.
Recovery Notes
First recoverable review artifact generated from codex analysis log.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Constraint: Bot approval still flagged concrete drift from current control and runtime event shapes. Rejected: Leaving warnings for implementation follow-up | RFC readers could target nonexistent emitters or wrong correlation fields. Confidence: high Scope-risk: narrow Directive: Use first-class control correlation fields before extra; include current orchestrator.tool.called in runtime I/O capture. Tested: Documentation-only diff inspection. Not-tested: Runtime tests not run for RFC text-only edit. Co-authored-by: OmX <omx@oh-my-codex.dev>
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
fb3b2ddfor PR #522
Review record:
3b2d11be-6e28-4ab7-93ed-cc9f9bf01a91
Blocking Findings
| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|-----------|----------|---------|
| 1 | docs/rfc/contract-ledger.md:190 | BLOCKING | The backfill idempotency key is underspecified in a way that breaks the dependency graph. Step 3 stores extra.backfill_key=<synthetic_contract_key> per synthetic contract, then line 190 says to skip any existing synthetic contract.* event with the same type, contract_id, and extra.backfill_key. That key is unique per contract, not per dependency edge, so a contract with multiple synthetic contract.dependency.recorded edges will collide on reruns and only one edge can survive the dedupe rule. Since L3/L6 rely on reconstructing the full dependency graph, this needs an edge-specific idempotency key or additional dedupe fields such as to_contract_id/edge_type/source timestamp. |
Non-blocking Suggestions
None.
Design Notes
The RFC is generally coherent: it keeps event_store authoritative, separates AC structure from runtime contract dependencies, and makes the migration path additive. The main gap is that the backfill dedupe rule is not precise enough to preserve multi-edge dependency history.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
Autopilot follow-up: addressed the latest bot blocker. Commit |
Constraint: Latest bot review found boundary-level backfill keys could collapse multiple synthetic dependency edges.\nRejected: Reusing extra.backfill_key for all synthetic events | dependency events need event-specific identity to preserve multi-edge histories.\nConfidence: high\nScope-risk: narrow\nDirective: Keep backfill_event_key event-specific; never dedupe contract.dependency.recorded by boundary key alone.\nTested: git diff --check; targeted grep for backfill_event_key, dependency edge key fields, provenance spelling, and stale extra lineage wording.\nNot-tested: Full CI; docs-only change and existing PR checks are green.
|
Autopilot follow-up: addressed the latest bot blocker on backfill idempotency. Commit |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
89636fffor PR #522
Review record:
6cebcf69-934e-482d-a9e6-0a7a8711bec3
Blocking Findings
| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|-----------|----------|---------|
| 1 | docs/rfc/contract-ledger.md:186 | BLOCKING | The backfill key selection is defined per event, not per invocation boundary. If one historical call emits some rows with call_id and other related rows with only correlation_id or request_id, this rule will assign those rows to different synthetic contract keys, splitting one real contract into multiple synthetic contracts. That would make replay_events(contract_id) silently incomplete for backfilled data and corrupt the audit/dependency view the RFC is trying to preserve. The mapping needs a boundary-level reconciliation rule instead of choosing the highest-priority field independently on each row. |
Non-blocking Suggestions
None.
Design Notes
The RFC is directionally sound: keeping the ledger as a projection over event_store preserves the existing source-of-truth model, and the separation between AC structure and runtime contract dependencies is clear. The main weakness is the historical backfill algorithm, which is currently under-specified at the exact place where determinism and correctness matter most.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Q00
left a comment
Q00
left a comment
There was a problem hiding this comment.
Requesting changes because the backfill idempotency key is still not edge-specific enough.
The RFC currently says synthetic contract events are deduped by type, contract_id, and extra.backfill_key. That key identifies the synthetic contract boundary, not a specific dependency edge. If a generation-derived synthetic contract emits multiple dependency edges, multiple events can share the same type/contract_id/backfill_key and be incorrectly skipped as duplicates.
Please include edge-specific identity in the synthetic dependency event key, such as from_contract_id, to_contract_id, edge_type, and a source event/timestamp/evidence key, or introduce a dedicated extra.backfill_event_key.
If I am missing a later invariant that makes only one synthetic edge possible per contract boundary, please reply and I will re-check.
The latest reviews identified two remaining ambiguity risks in the Contract Ledger RFC: stable invocation evidence was selected per row instead of per reconstructed boundary, and dependency backfill dedupe could still be read as boundary-key based. Tighten the RFC so backfill first reconciles invocation clusters, assigns one synthetic contract key per cluster, and gives every dependency event an edge-specific backfill_event_key carrying edge identity plus source evidence. Constraint: PR Q00#522 is docs-only and must preserve the event_store-as-source projection design. Rejected: Choosing the highest-priority invocation field independently on each historical row | it can split one real invocation across multiple synthetic contracts. Rejected: Dedupe contract.dependency.recorded by extra.backfill_key | the key identifies a boundary, not an edge. Confidence: high Scope-risk: narrow Directive: Backfill implementations must cluster historical invocation evidence before assigning synthetic contract keys, and must dedupe dependency events by edge-specific backfill_event_key. Tested: git diff --check; grep -n invocation-boundary docs/rfc/contract-ledger.md; grep -n backfill_event_key docs/rfc/contract-ledger.md Not-tested: Markdown render preview; docs-only change
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: APPROVE
Reviewing commit
fd0ce2ffor PR #522
Review record:
a5f771ee-3282-4af3-92bd-de518f1bf0a3
Blocking Findings
No in-scope blocking findings remained after policy filtering.
Follow-up Findings
docs/rfc/contract-ledger.md:183[warning] The L7 backfill algorithm keys invocation boundaries oncall_id, but the existing runtime/projection code already usestool_call_idas the concrete persisted field name (for example src/ouroboros/orchestrator/runtime_message_projection.py:316 and src/ouroboros/orchestrator/mcp_tools.py:1481). If an implementer follows this RFC literally, historical tool-call rows carrying onlytool_call_idwill miss rule 2, fall through to ambiguous/event-level backfill, and produce incomplete or fragmented contract histories. The precedence list needs to nametool_call_idexplicitly (or definecall_idas an alias that includes it).
Non-blocking Suggestions
| 1 | docs/rfc/contract-ledger.md:300 | Documentation | The RFC is marked Accepted, but the embedded pre-merge checklist still contains unchecked gating items such as maintainer approvals and sub-thread resolution. That reads like stale process state once merged; consider removing or converting these to historical notes. |
Design Notes
The RFC is generally well-structured: it keeps the ledger as a projection over event_store, separates AC structure from runtime dependency edges, and makes rollout/backfill constraints explicit. The main weakness is field-name precision in L7, where one mismatched identifier can materially change how history is reconstructed.
Policy Notes
- No in-scope blocking findings remained after policy filtering; downgraded verdict accordingly.
Recovery Notes
First recoverable review artifact generated from codex analysis log.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
2 participants
Summary
Adds
docs/rfc/contract-ledger.mdcapturing the schema strategy (event_store source + projection + optional materialized view), dependency-edge representation, audit composition, three-layer replay API, and additive five-step migration plan agreed in the proposal comment on #513. Docs-only; zero runtime impact.Decisions captured
event_store(unchanged); ledger =ContractLedgerProjectorprojection layer; optionalcontract_summariesmaterialized view recoverable from the journal.ac_tree(static AC decomposition, immutable per generation) and a newcontract.dependency.recordedevent (runtime, additiveedge_type:result_input,parent_ac) live as two separate graphs with separate accessors.control.directive.emittedevents stay 1:1 (M2 fidelity); the projector exposescontract_audit_trail(contract_id)aggregates.replay_events(raw),replay_state(projector),replay_timeline(TUI-ready); all read from the sameevent_storesource; replay never re-executes LLM calls or re-forks sub-agents.ouroboros ledger backfill [--apply]); 1.0 ships without anyone running a migration; first-draft backfill mapping table included.Inherited:
contract_id = ULID(mesh.mdD2);artifact_ref = "sha256:..."(disposable-memory.mdC2).Cross-references
ac_tree), feat(orchestrator): wire checkpoint store + public API context #338 (CheckpointStore), feat(events): add event_version to BaseEvent payload #436 (event_version), [RFC] Agent OS runtime contract: capabilities, policy, directives, and replayable agent processes #476 M2 + S4 (journal as source of truth, additive-only schema)docs/rfc/mesh.md([RFC] MCP Mesh wire format & Coordinator handshake (sub-RFC of #476) #511),docs/rfc/disposable-memory.md([RFC] Disposable Memory process model & artifact_ref (sub-RFC of #476) #512)Pre-merge checklist
mesh.mdD2 +disposable-memory.mdC2synthetic=true+provenance="backfill")replay()semantics matchmesh.mdD6 (no LLM re-execution) anddisposable-memory.mdC5 (read artifact default)Post-merge checklist
docs/rfc/contract-ledger.mdreachable from the docs site (or the README index when it lands)ouroboros ledger backfillandouroboros ledger verifycommands listed in the user-facing CLI docsRollback
Docs-only PR with zero runtime impact. Rollback = revert this PR.
Closes #513.