Repository navigation
fix(codex): write the provenance ledger, bounded (#2622) - #2626
Conversation
The writer appends three entries per admitted transaction, and a 'present' baseline carries the artifact's exact bytes as base64 - a 25 KB config.toml is ~34 KB per entry, so roughly 100 KB per transaction. Unbounded, a machine that syncs on every start grows integrations/codex.json forever, and since the record is re-read and re-serialized on every append the cost is quadratic rather than merely large. A ledger is evidence, not an archive. The window keeps whole transactions: trimming by entry count would cut one in half and leave a record claiming a transaction touched two artifacts when it touched three, which reads as complete and is worse than dropping it. Also spreads the existing provenance object so an unknown ledger-level key from a newer writer survives the append, which the record contract requires. Falsified: removing the window reddens the trimming test and leaves the within-window identity case green.
|
✅ Deterministic PR hygiene checks passed. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughCodex apply and restore transactions now record bounded provenance for native artifacts. The writer stores pre-image metadata, post-image hashes, transaction IDs, and timestamps, preserves unknown integration fields, and handles malformed records without changing the admitted transition. ChangesCodex provenance recording
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CodexWritePath
participant withCodexWriteLock
participant recordCodexNativeTransactionProvenance
participant IntegrationRecord
CodexWritePath->>withCodexWriteLock: Commit apply or restore transaction
withCodexWriteLock-->>CodexWritePath: Return preImages and currentTxId
CodexWritePath->>recordCodexNativeTransactionProvenance: Record committed transaction
recordCodexNativeTransactionProvenance->>IntegrationRecord: Update provenance ledger
Suggested reviewers: ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2df876f193
ℹ️ 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".
| recordCodexNativeTransactionProvenance( | ||
| coordinated.value.preImages, | ||
| coordinated.value.receipt.currentTxId, | ||
| ); |
There was a problem hiding this comment.
When two processes update the same Codex home, this call runs only after withCodexWriteLock has committed and released N, so a second transaction can replace any of the three files before provenancePostImage() reads them. The first transaction's ledger entries can therefore contain the second transaction's hashes, allowing later provenance-based restoration to mistake another writer's state for bytes owned by the first transaction. Compute and return the post-image hashes inside the locked callback, then append those captured values only after the transaction commits.
Useful? React with 👍 / 👎.
| return { | ||
| kind: "present", | ||
| sha256: createHash("sha256").update(bytes).digest("hex"), | ||
| bytesBase64: Buffer.from(bytes).toString("base64"), |
There was a problem hiding this comment.
When config.toml contains credentials, this serializes its exact contents as reversible base64 into integrations/codex.json; the same module already acknowledges that config bytes can carry credentials. Unlike the transient journal, the bounded ledger retains up to 16 historical snapshots, including rotated secrets, expanding both their lifetime and storage surface. Store only non-reversible evidence in this record, or place the required restoration material behind an appropriate credential-storage boundary rather than embedding it in the ledger.
AGENTS.md reference: AGENTS.md:L266-L272
Useful? React with 👍 / 👎.
…dge-jun#2626) * fix(codex): write admitted transaction provenance * fix(provenance): bound the ledger to the newest 16 transactions The writer appends three entries per admitted transaction, and a 'present' baseline carries the artifact's exact bytes as base64 - a 25 KB config.toml is ~34 KB per entry, so roughly 100 KB per transaction. Unbounded, a machine that syncs on every start grows integrations/codex.json forever, and since the record is re-read and re-serialized on every append the cost is quadratic rather than merely large. A ledger is evidence, not an archive. The window keeps whole transactions: trimming by entry count would cut one in half and leave a record claiming a transaction touched two artifacts when it touched three, which reads as complete and is worse than dropping it. Also spreads the existing provenance object so an unknown ledger-level key from a newer writer survives the append, which the record contract requires. Falsified: removing the window reddens the trimming test and leaves the within-window identity case green.
…dge-jun#2626) * fix(codex): write admitted transaction provenance * fix(provenance): bound the ledger to the newest 16 transactions The writer appends three entries per admitted transaction, and a 'present' baseline carries the artifact's exact bytes as base64 - a 25 KB config.toml is ~34 KB per entry, so roughly 100 KB per transaction. Unbounded, a machine that syncs on every start grows integrations/codex.json forever, and since the record is re-read and re-serialized on every append the cost is quadratic rather than merely large. A ledger is evidence, not an archive. The window keeps whole transactions: trimming by entry count would cut one in half and leave a record claiming a transaction touched two artifacts when it touched three, which reads as complete and is worse than dropping it. Also spreads the existing provenance object so an unknown ledger-level key from a newer writer survives the append, which the record contract requires. Falsified: removing the window reddens the trimming test and leaves the within-window identity case green.
1 participant
Summary
Closes #2622.
updateIntegrationRecordhad no production caller, so the Codex provenance ledger was never written — a tested, exported, documented writer that nothing invoked, which reads as implemented. The apply and remove paths now append evidence for the artifacts a native transaction may mutate:config,generated-profile, andinjection-journal.Ordering is the load-bearing decision. The append runs after
withCodexWriteLockreturns an acquired transaction, so the native files and the coordinator row have already committed. A crash between the two leaves an admitted transaction with no ledger entry — safe, because absence grants no restore authority and existing acceptance already treats absence as unavailable evidence. Writing before the commit would be the unsafe direction: it would leave provenance for a transaction that never happened. The append is best-effort for the same reason; a non-CAS JSON failure must not turn a committed native transaction into a reported failure.Bound added on review
As written the ledger was unbounded, and that is not a small leak. Each transaction appends three entries, and a
presentbaseline embeds the artifact's exact bytes as base64 — a 25 KBconfig.tomlmeasures ~34 KB per entry, so roughly 100 KB per transaction. A machine that syncs on every start growsintegrations/codex.jsonwithout limit, and because the record is re-read and re-serialized on every append, the cost is quadratic rather than merely large.The window keeps the newest 16 transactions, whole. Trimming by entry count would cut a transaction in half and leave a record claiming it touched two artifacts when it touched three — a partial record still reads as complete, which is worse than dropping it outright.
The append also now spreads the existing
provenanceobject, so an unknown ledger-level key from a newer writer survives — the record contract requires that, and the original append dropped it.Verification
Falsified both ways: removing the production writer leaves an admitted transaction with
[]matching artifacts instead of the three expected ones; removing the window reddens the trimming test while the within-window identity case stays green — which pins the bound as a bound rather than an unconditional filter.Checklist
devdevlog/_plan/260826_provenance_writer/010_design.mdSummary by CodeRabbit
New Features
Bug Fixes