feat(store): freeze kernel schema - #112
Conversation
|
Warning Review limit reachedNext included review available in 45 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 108 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
Comment |
|
You have reached your Codex usage limits for security reviews. Please try again later. |
![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: b958c38893
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| use sha2::{Digest, Sha256}; | ||
| use std::collections::BTreeSet; | ||
|
|
||
| pub const KERNEL_APPLICATION_ID: u32 = 0x4D43_4B52; |
There was a problem hiding this comment.
Every newly bootstrapped core.sqlite is marked with 0x4D434B52 (MCKR), but the shared direct-format identity vocabulary in sqlite_runtime.rs defines 0x4D435458 (MCTX), which this kernel is intended to reuse. An opener or repair tool applying the existing pristine-or-exact identity check will therefore classify a freshly created kernel database as mismatched and quarantine or rebuild it; reuse MC_APPLICATION_ID here, or update the entire shared identity contract together.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — you're right, and I was initially going to push back on this. Fixed in fb248d7.
I'd reasoned that a distinct format should carry a distinct application_id, so the header alone discriminates. The plan disagrees explicitly. KTD8:
Identity manifest strengthened to normalized schema-SQL digest plus exact non-internal object inventory, in new kernel-specific fixture with kernel-specific marker table (
mc_kernel_format_marker) so kernel identity decidable against legacy files (KTD3). MCTXapplication_idkept per restructure R6.
And R1 lists "MCTX application_id" as part of the identity core.sqlite stamps. So the shared id is the decided design, and the marker table name is the discriminator, not the header — which is what makes R2's foreign outcome safe (a non-pristine family lacking the kernel marker is refused, never quarantined).
KERNEL_APPLICATION_ID now aliases MC_APPLICATION_ID rather than redeclaring a value, so there is one definition to drift from. kernel_stamps_the_shared_direct_format_application_id asserts the alias and the stamped pragma.
| ), | ||
| ( | ||
| "predicate_schemas", | ||
| r#"CREATE TABLE predicate_schemas(predicate_schema_id TEXT PRIMARY KEY,object_id TEXT NOT NULL UNIQUE REFERENCES object_registry(object_id),domain_id TEXT NOT NULL REFERENCES domains(domain_id),predicate_name TEXT NOT NULL,value_schema BLOB NOT NULL,freshness_class TEXT NOT NULL,created_commit_seq INTEGER NOT NULL REFERENCES commit_log(commit_seq),invalidated_commit_seq INTEGER REFERENCES commit_log(commit_seq),superseded_by TEXT REFERENCES object_registry(object_id),sensitivity_class TEXT NOT NULL,UNIQUE(domain_id,predicate_name)) STRICT; CREATE INDEX idx_predicate_domain_fk ON predicate_schemas(domain_id); CREATE INDEX idx_predicate_known_as_of ON predicate_schemas(created_commit_seq,invalidated_commit_seq,predicate_schema_id);"#, |
There was a problem hiding this comment.
When a predicate's value schema or freshness policy is corrected, the append-only model requires retaining the invalidated row and inserting a replacement with a new predicate_schema_id, but UNIQUE(domain_id,predicate_name) still covers the invalidated row and rejects that replacement. Updating or deleting the old row instead would destroy known_as_of history, so uniqueness needs to apply only to active rows, such as through a partial unique index on rows whose invalidated_commit_seq is null; the analogous permanent name constraints on domains and relations need the same treatment.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 902aa20.
R8 retains the invalidated row and U4 appends a successor, so UNIQUE(domain_id,predicate_name) made a predicate schema permanently uncorrectable: the replacement collided with the very row it was replacing. Replaced with a partial unique index over active rows:
CREATE UNIQUE INDEX idx_predicate_active_name
ON predicate_schemas(domain_id,predicate_name) WHERE invalidated_commit_seq IS NULL;You were right that domains.name and relation_registry.relation_name had the identical defect; both got the same treatment.
One consequence worth recording: 4f4904a dropped idx_predicate_domain_fk on the grounds that UNIQUE(domain_id,predicate_name) covered it. A partial index only serves queries whose WHERE implies its predicate, so that index is restored alongside this change.
Covered by superseded_predicate_schema_replacement_reuses_its_name and superseded_domain_and_relation_names_are_reusable_once_invalidated, which also assert exactly one active holder of a name at a time.
| |row| row.get::<_, String>(0), | ||
| ) | ||
| .unwrap(), | ||
| kernel_schema_digest(&conn).unwrap() |
There was a problem hiding this comment.
If a column, index, or constraint is accidentally changed, bootstrap stores the digest of that changed schema and this assertion immediately recomputes the digest from the same changed database, so both sides change together and the test still passes; the separate inventory assertion only checks table names. The claimed frozen shape is therefore not pinned, allowing incompatible schema changes to ship without an epoch change or quarantine signal. Compare against a checked-in golden digest or an expected digest derived independently from the database under test.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already pinned — six lines below the assertion you flagged:
const PINNED_SCHEMA_DIGEST: &str = "...";
#[test]
fn kernel_schema_digest_is_pinned_to_the_frozen_v1_shape() { ... }That constant landed in 4f4904a, before this comment. Your comment is anchored to b958c388, which is no longer in the branch.
The line-70 assertion proves a different property — that the marker stores the digest kernel_schema_digest computes, i.e. bootstrap stamps the shape it actually built. The golden constant is what catches shape drift, and it did its job here: the schema changes in 902aa20 and fb248d7 both failed it and required an explicit repin.
| ), | ||
| ( | ||
| "admission_decisions", | ||
| r#"CREATE TABLE admission_decisions(admission_decision_id TEXT PRIMARY KEY,candidate_id TEXT REFERENCES candidates(candidate_id) ON DELETE SET NULL,subject_object_id TEXT REFERENCES object_registry(object_id),source_kind TEXT NOT NULL,source_id TEXT NOT NULL,source_revision INTEGER NOT NULL,source_class TEXT NOT NULL,taint_class TEXT NOT NULL,maturity TEXT NOT NULL,disposition TEXT NOT NULL,visibility TEXT NOT NULL,policy_revision INTEGER NOT NULL,reason TEXT NOT NULL,evidence_id TEXT REFERENCES evidence_meta(evidence_id) ON DELETE SET NULL,approval_object_id TEXT REFERENCES object_registry(object_id) ON DELETE SET NULL,decided_at INTEGER NOT NULL) STRICT; CREATE INDEX idx_admission_candidate_fk ON admission_decisions(candidate_id); CREATE INDEX idx_admission_subject_fk ON admission_decisions(subject_object_id); CREATE INDEX idx_admission_evidence_fk ON admission_decisions(evidence_id); CREATE INDEX idx_admission_source ON admission_decisions(source_kind,source_id,source_revision,decided_at);"#, |
There was a problem hiding this comment.
When an admitted object's maturity, visibility, or disposition changes—particularly when evidence deletion propagates a status change—the new admission decision must be ordered at a specific commit_seq so known_as_of reads can exclude it before that commit and replay can apply it atomically. This table records only wall-clock decided_at; subject_object_id can at most imply the object's initial creation commit and cannot identify later policy decisions. Add a commit-log reference for decisions that affect canonical state rather than relying on timestamps.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 902aa20. Added commit_seq INTEGER REFERENCES commit_log(commit_seq) ON DELETE RESTRICT plus idx_admission_commit_fk.
Nullable rather than NOT NULL, deliberately: an accepted decision mutates canonical state and therefore rides an envelope commit, but a rejection produces no canonical mutation, and R4 scopes commit_seq to canonical mutation commits. Forcing NOT NULL would mean fabricating a commit for every rejected candidate. NULL now means "this decision produced no canonical commit", and the index orders the ones that did.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| ), | ||
| ( | ||
| "outbox", | ||
| r#"CREATE TABLE outbox(outbox_position INTEGER PRIMARY KEY AUTOINCREMENT,commit_seq INTEGER NOT NULL REFERENCES commit_log(commit_seq) ON DELETE RESTRICT,ordinal INTEGER NOT NULL,object_id TEXT NOT NULL,object_kind TEXT NOT NULL,source_kind TEXT NOT NULL,source_id TEXT NOT NULL,source_revision INTEGER NOT NULL,sensitivity_class TEXT NOT NULL,payload BLOB NOT NULL,created_at INTEGER NOT NULL,published_at INTEGER,UNIQUE(commit_seq,ordinal)) STRICT; CREATE INDEX idx_outbox_poll ON outbox(published_at,outbox_position); CREATE INDEX idx_outbox_prune ON outbox(published_at,created_at,outbox_position); CREATE INDEX idx_outbox_commit_fk ON outbox(commit_seq);"#, |
There was a problem hiding this comment.
SUGGESTION: idx_outbox_commit_fk on outbox is redundant with UNIQUE(commit_seq, ordinal)
outbox defines UNIQUE(commit_seq, ordinal), which creates a composite unique index with commit_seq as the leading column. SQLite's query planner and foreign key enforcement automatically use that prefix, making CREATE INDEX idx_outbox_commit_fk ON outbox(commit_seq); redundant (compare with change_event, which omits the single-column index).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Already dropped in 4f4904a, for exactly the reason you give — that commit message cites the UNIQUE(commit_seq,ordinal) autoindex prefix and calls the extra index pure write amplification. Your comment is anchored to b958c388, which predates it.
| ), | ||
| ( | ||
| "writer_fence", | ||
| r#"CREATE TABLE writer_fence(id INTEGER PRIMARY KEY CHECK(id=0),writer_epoch INTEGER) STRICT;"#, |
There was a problem hiding this comment.
WARNING: writer_fence.writer_epoch is nullable and seeded with NULL
writer_fence is defined with writer_epoch INTEGER, and apply_schema initializes it with INSERT INTO writer_fence(id) VALUES(0), leaving writer_epoch as NULL. Everywhere else in the kernel (commit_log, capture_pins), writer_epoch is INTEGER NOT NULL.
Consider defining writer_epoch INTEGER NOT NULL DEFAULT 0 so fence readers do not require optional handling on an initialized database.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Keeping it nullable. KTD11 makes the unstamped state load-bearing in a way NOT NULL DEFAULT 0 would erase:
Dedicated one-row
writer_fencetable (declared in bootstrap DDL, inside manifest digest) stamped with acquiring lease epoch at open [...]commit_log.writer_epochstay per-commit audit, never comparison source: column-max fence unstamped between successor's open and first commit; restored database whose column max exceed fresh lease root epoch would fence new writer out permanently
Bootstrap declares the row; the open path stamps it with the acquiring lease epoch (U3, a later unit). NULL is the distinguishable "declared, not yet stamped" state. DEFAULT 0 turns that into a valid-looking epoch 0 that compares successfully against any lease, which is the fence-epoch confusion KTD11 is specifically guarding against — and it would only be sound under an unstated guarantee that lease epochs begin at 1.
The asymmetry with commit_log.writer_epoch and capture_pins.writer_epoch is principled rather than accidental: those record an epoch that is already known at write time, whereas the fence row exists before any lease has been acquired.
| ), | ||
| ( | ||
| "asserted_edges", | ||
| r#"CREATE TABLE asserted_edges(edge_id TEXT PRIMARY KEY,object_id TEXT NOT NULL UNIQUE REFERENCES object_registry(object_id),relation_id TEXT NOT NULL REFERENCES relation_registry(relation_id),source_object_id TEXT NOT NULL REFERENCES object_registry(object_id),target_object_id TEXT NOT NULL REFERENCES object_registry(object_id),scope_id TEXT REFERENCES scopes(scope_id),anchor_id TEXT REFERENCES anchors(anchor_id),evidence_id TEXT REFERENCES evidence_meta(evidence_id) ON DELETE SET NULL,edge_payload BLOB,created_commit_seq INTEGER NOT NULL REFERENCES commit_log(commit_seq),invalidated_commit_seq INTEGER REFERENCES commit_log(commit_seq),superseded_by TEXT REFERENCES object_registry(object_id),sensitivity_class TEXT NOT NULL) STRICT; CREATE INDEX idx_edges_relation_fk ON asserted_edges(relation_id); CREATE INDEX idx_edges_source_fk ON asserted_edges(source_object_id); CREATE INDEX idx_edges_target_fk ON asserted_edges(target_object_id); CREATE INDEX idx_edges_evidence_fk ON asserted_edges(evidence_id); CREATE INDEX idx_edges_known_as_of ON asserted_edges(created_commit_seq,invalidated_commit_seq,edge_id);"#, |
There was a problem hiding this comment.
WARNING: Missing foreign key indexes on asserted_edges for scope_id and anchor_id
asserted_edges defines foreign keys scope_id REFERENCES scopes(scope_id) and anchor_id REFERENCES anchors(anchor_id). Unlike propositions (which defines idx_prop_scope_fk and idx_prop_anchor_fk), asserted_edges omits indexes on both columns.
Filtering edges by scope or anchor, or validating parent table mutations under PRAGMA foreign_keys = ON, will require full table scans. Add idx_edges_scope_fk and idx_edges_anchor_fk.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in 902aa20 — added idx_edges_scope_fk and idx_edges_anchor_fk, matching what propositions already declares. Also added idx_edges_superseded_fk, which was missing for the same reason.
One correction to the rationale: under R8 canonical rows are never deleted, so the parent-mutation validation scan you describe should not occur in practice. The reason to add these is the query path plus internal consistency — propositions declaring scope and anchor indexes while asserted_edges omitted them was the actual defect, and in a frozen schema that inconsistency is expensive to correct later.
| ), | ||
| ( | ||
| "decisions", | ||
| r#"CREATE TABLE decisions(decision_id TEXT PRIMARY KEY,object_id TEXT NOT NULL UNIQUE REFERENCES object_registry(object_id),proposition_id TEXT REFERENCES propositions(proposition_id),scope_id TEXT REFERENCES scopes(scope_id),anchor_id TEXT REFERENCES anchors(anchor_id),evidence_id TEXT REFERENCES evidence_meta(evidence_id) ON DELETE SET NULL,decision_kind TEXT NOT NULL,decision_payload BLOB NOT NULL,created_commit_seq INTEGER NOT NULL REFERENCES commit_log(commit_seq),invalidated_commit_seq INTEGER REFERENCES commit_log(commit_seq),superseded_by TEXT REFERENCES object_registry(object_id),sensitivity_class TEXT NOT NULL) STRICT; CREATE INDEX idx_decisions_known_as_of ON decisions(created_commit_seq,invalidated_commit_seq,decision_id); CREATE INDEX idx_decisions_evidence_fk ON decisions(evidence_id);"#, |
There was a problem hiding this comment.
WARNING: decisions lacks foreign key index coverage for proposition_id, scope_id, anchor_id, and superseded_by
decisions declares foreign keys referencing propositions(proposition_id), scopes(scope_id), anchors(anchor_id), and object_registry(object_id) (superseded_by), but only defines idx_decisions_known_as_of and idx_decisions_evidence_fk.
Lookups resolving decisions by proposition, scope dimension, or anchor reachability will require full table scans of decisions, and parent table mutations will trigger unindexed foreign key validation scans.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in 902aa20 — added idx_decisions_proposition_fk, idx_decisions_scope_fk, idx_decisions_anchor_fk, and idx_decisions_superseded_fk.
Same caveat as on asserted_edges: R8 forbids canonical deletion, so the unindexed-FK-validation half of the argument does not really apply. These land for the resolution paths you name and for consistency with propositions.
| ), | ||
| ( | ||
| "observations", | ||
| r#"CREATE TABLE observations(observation_id TEXT PRIMARY KEY,object_id TEXT NOT NULL UNIQUE REFERENCES object_registry(object_id),proposition_id TEXT REFERENCES propositions(proposition_id),scope_id TEXT REFERENCES scopes(scope_id),anchor_id TEXT REFERENCES anchors(anchor_id),evidence_id TEXT REFERENCES evidence_meta(evidence_id) ON DELETE SET NULL,observation_kind TEXT NOT NULL,observation_payload BLOB NOT NULL,observed_at INTEGER NOT NULL,created_commit_seq INTEGER NOT NULL REFERENCES commit_log(commit_seq),invalidated_commit_seq INTEGER REFERENCES commit_log(commit_seq),superseded_by TEXT REFERENCES object_registry(object_id),sensitivity_class TEXT NOT NULL) STRICT; CREATE INDEX idx_observations_known_as_of ON observations(created_commit_seq,invalidated_commit_seq,observation_id); CREATE INDEX idx_observations_evidence_fk ON observations(evidence_id);"#, |
There was a problem hiding this comment.
WARNING: observations lacks foreign key index coverage for proposition_id, scope_id, anchor_id, and superseded_by
observations references propositions(proposition_id), scopes(scope_id), anchors(anchor_id), and object_registry(object_id) (superseded_by) without corresponding indexes.
Resolving observations effective for a proposition or within a scope/anchor context will require full table scans of observations.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in 902aa20 — added idx_observations_proposition_fk, idx_observations_scope_fk, idx_observations_anchor_fk, and idx_observations_superseded_fk, matching the treatment decisions got.
| created_at: i64, | ||
| hook: F, | ||
| ) -> rusqlite::Result<()> { | ||
| let tx = conn.transaction()?; |
There was a problem hiding this comment.
WARNING: conn.transaction() uses DEFERRED locking and risks lock upgrade contention
conn.transaction()? creates a DEFERRED transaction (BEGIN DEFERRED). If concurrent connections attempt to bootstrap or open the database, both acquire shared read locks and fail with SQLITE_BUSY when upgrading to write locks during DDL execution instead of queueing up front for busy_timeout.
Use conn.transaction_with_behavior(rusqlite::TransactionBehavior::Immediate)? to acquire the write lock immediately and respect busy_timeout.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Confirmed and fixed in 902aa20. This also contradicted the stated contract in two places, which makes it more than a tuning preference:
- U4: "Mutation coordinator over single write connection (
BEGIN IMMEDIATE)" docs/migration-version-lanes.md: "A pristine family is bootstrapped underBEGIN IMMEDIATE"
Now conn.transaction_with_behavior(TransactionBehavior::Immediate).
| ) -> rusqlite::Result<()> { | ||
| let tx = conn.transaction()?; | ||
| tx.pragma_update(None, "application_id", KERNEL_APPLICATION_ID)?; | ||
| for ((name, sql), expected) in COMPONENTS.iter().zip(KERNEL_SCHEMA_COMPONENT_NAMES) { |
There was a problem hiding this comment.
WARNING: zip with debug_assert_eq! silently ignores schema components in release builds
apply_schema iterates over components using COMPONENTS.iter().zip(KERNEL_SCHEMA_COMPONENT_NAMES). Because Iterator::zip terminates as soon as the shorter iterator is exhausted, and debug_assert_eq! is a no-op in release builds, any future mismatch in slice lengths will silently truncate table creation without raising an error.
Replace the debug assertion with an explicit length check or iterate directly over COMPONENTS.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Already fixed in 4f4904a, and more strongly than suggested. apply_schema iterates COMPONENTS directly:
for (_, sql) in COMPONENTS {and the length-and-name agreement is a compile-time assertion, not a debug one:
const _: () = assert!(component_names_match());That fails the build in every profile on drift, so it strictly dominates the runtime length check. Your comment is anchored to b958c388, which predates the change.
| } | ||
| let synchronous: i64 = conn.query_row("PRAGMA synchronous", [], |row| row.get(0))?; | ||
| if !(1..=3).contains(&synchronous) { | ||
| if !(2..=3).contains(&synchronous) { |
There was a problem hiding this comment.
WARNING: verify_sqlite_connection_contract disallows synchronous = NORMAL and enforces trusted_schema = OFF
Tightening synchronous to !(2..=3) (FULL / EXTRA) and requiring trusted_schema == 0 causes contract verification to diverge from packages/plugin/src/shared/sqlite.ts (which defaults allowedSynchronous to [1, 2, 3] and does not verify trusted_schema). Non-kernel or TypeScript connections evaluated against this function will fail closed unless their profiles and contracts are updated in tandem.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Already resolved in 72afb13, which is the commit this comment is anchored ahead of. verify_sqlite_connection_contract still accepts synchronous in [1, 2, 3] and does not read trusted_schema at all (sqlite_runtime.rs:138-143).
The tighter requirements live in verify_kernel_connection_contract, which calls the shared contract first and then appends the kernel-only checks. Non-kernel and TypeScript connections evaluated against the shared function are unaffected, and packages/plugin/src/shared/sqlite.ts needs no change. normal_synchronous_mode_fails_kernel_verification and trusted_schema_on_fails_kernel_verification pin the kernel side.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous Review Summaries (2 snapshots, latest commit cabd3e4)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit cabd3e4)Status: 12 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (6 files)
Fix these issues in Kilo Cloud Previous review (commit b958c38)Status: 12 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (6 files)
Reviewed by gemini-3.7-flash · Input: 109.9K · Output: 11.5K · Cached: 641.4K |
Create the complete core.sqlite shape in one transaction and verify its SQLite safety profile. The fixed inventory includes downstream-required control facts without adding migration machinery.
b958c38 to
059a03a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 059a03a91a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ), | ||
| ( | ||
| "mc_kernel_format_marker", | ||
| r#"CREATE TABLE mc_kernel_format_marker(singleton INTEGER PRIMARY KEY CHECK(singleton=1),format_epoch INTEGER NOT NULL,database_incarnation_id TEXT NOT NULL,schema_digest TEXT NOT NULL,created_at INTEGER NOT NULL) STRICT;"#, |
There was a problem hiding this comment.
After bootstrap, any connection can UPDATE or DELETE this marker row because the schema installs neither the no-update/no-delete triggers used by the shared direct-format marker nor an equivalent guard. An accidental maintenance write can therefore change the incarnation, epoch, or stored digest—or remove the marker entirely—causing a valid database to be quarantined or allowing altered schema identity to be restamped; enforce immutability at the database boundary.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 902aa20.
The contract already called the row immutable — docs/migration-version-lanes.md says "one immutable mc_format_marker row" — and the TypeScript side enforces it with triggers (storage-format-epoch.ts:143-151). The kernel marker had no equivalent, so the precedent was right there and unmatched. Mirrored both:
CREATE TRIGGER mc_kernel_format_marker_no_update BEFORE UPDATE ON mc_kernel_format_marker
BEGIN SELECT RAISE(ABORT,'mc_kernel_format_marker is immutable'); END;
CREATE TRIGGER mc_kernel_format_marker_no_delete BEFORE DELETE ON mc_kernel_format_marker
BEGIN SELECT RAISE(ABORT,'mc_kernel_format_marker is immutable'); END;format_marker_rejects_update_and_delete asserts both abort and the row survives. Following the same reasoning, commit_log got the same guard in fb248d7 — see your later comment on that table.
| ), | ||
| ( | ||
| "consumer_abandonments", | ||
| r#"CREATE TABLE consumer_abandonments(abandonment_id TEXT PRIMARY KEY,consumer_id TEXT NOT NULL,operator_id TEXT NOT NULL,last_checkpoint_outbox_position INTEGER NOT NULL,reason TEXT NOT NULL,abandoned_at INTEGER NOT NULL) STRICT; CREATE INDEX idx_abandonments_consumer ON consumer_abandonments(consumer_id,abandoned_at);"#, |
There was a problem hiding this comment.
When a required consumer is abandoned after canonical commit N, this row records only wall-clock time and has no reference to commit_log. Consequently a replay or known_as_of=N reconstruction cannot determine atomically whether the consumer still participated in the minimum checkpoint, so pruning can be computed from the wrong membership set; store the commit sequence that performed the authorized abandonment.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 902aa20. R9 is explicit on both halves of your point:
prune boundary = minimum required-consumer checkpoint [...] pending deregistration refuse unless operator-authorized abandonment record audit fields in commit log
and KTD10 adds that "Registration, deregistration, abandonment = rare auditable events, do go through envelope (policy R3 require abandonment in commit log)".
Added commit_seq INTEGER NOT NULL REFERENCES commit_log(commit_seq) ON DELETE RESTRICT plus idx_abandonments_commit_fk. NOT NULL here, unlike admission_decisions, because an authorized abandonment is itself a canonical control fact and always rides an authorizing commit.
| ), | ||
| ( | ||
| "entities", | ||
| r#"CREATE TABLE entities(entity_id TEXT PRIMARY KEY,object_id TEXT NOT NULL UNIQUE REFERENCES object_registry(object_id),domain_id TEXT NOT NULL REFERENCES domains(domain_id),entity_kind TEXT NOT NULL,canonical_name TEXT NOT NULL,created_commit_seq INTEGER NOT NULL REFERENCES commit_log(commit_seq),invalidated_commit_seq INTEGER REFERENCES commit_log(commit_seq),superseded_by TEXT REFERENCES object_registry(object_id),sensitivity_class TEXT NOT NULL) STRICT; CREATE INDEX idx_entities_domain_fk ON entities(domain_id,entity_id); CREATE INDEX idx_entities_known_as_of ON entities(created_commit_seq,invalidated_commit_seq,entity_id); CREATE INDEX idx_entities_superseded_fk ON entities(superseded_by);"#, |
There was a problem hiding this comment.
When a writer or replay supplies inconsistent duplicated metadata, this foreign key checks only that object_id exists: it does not require the entity and registry rows to share their domain, creation/invalidation commits, sensitivity, or even an entity object kind. For example, a registry row created at commit 10 can back an entity marked as created at commit 5, causing a typed known_as_of=7 query to expose an object that does not yet exist in the registry; bind the duplicated fields and discriminator to the registry row or make one representation authoritative.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. Deferring to the commit envelope with a tracking task rather than fixing it in the schema.
The reasoning: SQLite cannot express cross-row agreement as a CHECK, and triggers across the eleven typed canonical tables would be materially worse than one enforced write path. U4 already routes canonical writes through version-bound helpers, which is where the duplicated columns should be derived from the registry row rather than accepted from the caller — that closes the known_as_of=7-exposes-a-commit-10-object skew you describe by construction instead of by validation.
The task also covers the same shape on asserted_edges endpoint kinds and relation_registry.cardinality, which you raised separately: all three are "the foreign key proves the row exists but not that its discriminator agrees".
Two related things did land in the schema, since they were expressible declaratively: the validity interval check (invalidated > created) in 902aa20, and the append-only guard on commit_log in fb248d7.
| ), | ||
| ( | ||
| "asserted_edges", | ||
| r#"CREATE TABLE asserted_edges(edge_id TEXT PRIMARY KEY,object_id TEXT NOT NULL UNIQUE REFERENCES object_registry(object_id),relation_id TEXT NOT NULL REFERENCES relation_registry(relation_id),source_object_id TEXT NOT NULL REFERENCES object_registry(object_id),target_object_id TEXT NOT NULL REFERENCES object_registry(object_id),scope_id TEXT REFERENCES scopes(scope_id),anchor_id TEXT REFERENCES anchors(anchor_id),evidence_id TEXT REFERENCES evidence_meta(evidence_id) ON DELETE SET NULL,edge_payload BLOB,created_commit_seq INTEGER NOT NULL REFERENCES commit_log(commit_seq),invalidated_commit_seq INTEGER REFERENCES commit_log(commit_seq),superseded_by TEXT REFERENCES object_registry(object_id),sensitivity_class TEXT NOT NULL) STRICT; CREATE INDEX idx_edges_relation_fk ON asserted_edges(relation_id); CREATE INDEX idx_edges_source_fk ON asserted_edges(source_object_id); CREATE INDEX idx_edges_target_fk ON asserted_edges(target_object_id); CREATE INDEX idx_edges_evidence_fk ON asserted_edges(evidence_id); CREATE INDEX idx_edges_known_as_of ON asserted_edges(created_commit_seq,invalidated_commit_seq,edge_id);"#, |
There was a problem hiding this comment.
When evidence is deleted or purged, ON DELETE SET NULL rewrites every referencing asserted edge in place. Because that nullification has no validity interval or commit-sequence column of its own, a later known_as_of read for a commit before the purge can no longer recover which evidence supported the edge, and a canonical backup/replay loses the same historical association; retain tombstoned evidence metadata or invalidate and append a replacement edge at the deletion commit instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and fixed more simply than proposed, in 902aa20.
Following the R8 reading through: evidence_meta is canonical, and R8 says canonical rows are content-append-only with "DELETE never". So ON DELETE SET NULL was not merely lossy — it permitted a deletion the model forbids. Rather than adding tombstones or replacement edges to recover from a delete that should be impossible, the fix is to make it impossible at the boundary.
All six references to canonical parents are now ON DELETE RESTRICT: asserted_edges.evidence_id, decisions.evidence_id, decision_events.evidence_id, observations.evidence_id, admission_decisions.evidence_id, and admission_decisions.approval_object_id.
admission_decisions.candidate_id deliberately keeps SET NULL: candidates is staging and genuinely does expire under R10's 30-day TTL while the decision survives, so there the nullification is the intended semantics.
canonical_evidence_delete_is_refused_while_referenced covers it.
SQLite review findings on the frozen kernel schema: - drop idx_outbox_commit_fk: UNIQUE(commit_seq,ordinal) autoindex already covers the leftmost prefix, so the index was pure write amplification - drop idx_predicate_domain_fk: covered by UNIQUE(domain_id,predicate_name) - compile-time assert COMPONENTS and KERNEL_SCHEMA_COMPONENT_NAMES lengths match; zip truncated silently on drift in release builds - derive the version-floor gate message from MIN_SUPPORTED_SQLITE_VERSION instead of a hardcoded string Cargo.lock picks up the commons bump (cortexkit-lease 0.1.1, fs2 removed).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f4904af37
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ), | ||
| ( | ||
| "object_registry", | ||
| r#"CREATE TABLE object_registry(object_id TEXT PRIMARY KEY,object_kind TEXT NOT NULL,domain_id TEXT NOT NULL REFERENCES domains(domain_id) DEFERRABLE INITIALLY DEFERRED,source_kind TEXT NOT NULL,source_id TEXT NOT NULL,source_revision INTEGER NOT NULL,created_commit_seq INTEGER NOT NULL REFERENCES commit_log(commit_seq) ON DELETE RESTRICT,invalidated_commit_seq INTEGER REFERENCES commit_log(commit_seq) ON DELETE RESTRICT,superseded_by TEXT REFERENCES object_registry(object_id) DEFERRABLE INITIALLY DEFERRED,sensitivity_class TEXT NOT NULL,UNIQUE(source_kind,source_id,source_revision,object_kind)) STRICT; CREATE INDEX idx_objects_known_as_of ON object_registry(created_commit_seq,invalidated_commit_seq,object_id); CREATE INDEX idx_objects_domain_fk ON object_registry(domain_id,object_id); CREATE INDEX idx_objects_superseded_fk ON object_registry(superseded_by);"#, |
There was a problem hiding this comment.
When one source revision yields two objects of the same kind—for example, an extraction run producing multiple proposition candidates—the UNIQUE(source_kind,source_id,source_revision,object_kind) constraint rejects the second canonical object even though candidates explicitly permits many results per run. This makes ordinary multi-fact admission impossible unless callers fabricate a different source identity for each result; use an object-level idempotency identity instead of treating the source revision and kind as unique.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 902aa20. Demoted to a plain lookup index:
CREATE INDEX idx_objects_source
ON object_registry(source_kind,source_id,source_revision,object_kind);Your reading of the candidates contradiction is right: candidates explicitly permits many results per extraction run, so an extraction over one file revision yielding two propositions is ordinary, and the constraint rejected the second.
It was also redundant. Idempotency identity is owned elsewhere — operation_receipts(producer,operation_key) plus change_event.idempotency_key, per U4 step 2 — so the registry-level constraint was a second, coarser mechanism that happened to be wrong about the shape of the thing it was constraining.
one_source_revision_admits_many_objects_of_one_kind covers the multi-proposition case.
| ), | ||
| ( | ||
| "asserted_edges", | ||
| r#"CREATE TABLE asserted_edges(edge_id TEXT PRIMARY KEY,object_id TEXT NOT NULL UNIQUE REFERENCES object_registry(object_id),relation_id TEXT NOT NULL REFERENCES relation_registry(relation_id),source_object_id TEXT NOT NULL REFERENCES object_registry(object_id),target_object_id TEXT NOT NULL REFERENCES object_registry(object_id),scope_id TEXT REFERENCES scopes(scope_id),anchor_id TEXT REFERENCES anchors(anchor_id),evidence_id TEXT REFERENCES evidence_meta(evidence_id) ON DELETE SET NULL,edge_payload BLOB,created_commit_seq INTEGER NOT NULL REFERENCES commit_log(commit_seq),invalidated_commit_seq INTEGER REFERENCES commit_log(commit_seq),superseded_by TEXT REFERENCES object_registry(object_id),sensitivity_class TEXT NOT NULL) STRICT; CREATE INDEX idx_edges_relation_fk ON asserted_edges(relation_id); CREATE INDEX idx_edges_source_fk ON asserted_edges(source_object_id); CREATE INDEX idx_edges_target_fk ON asserted_edges(target_object_id); CREATE INDEX idx_edges_evidence_fk ON asserted_edges(evidence_id); CREATE INDEX idx_edges_known_as_of ON asserted_edges(created_commit_seq,invalidated_commit_seq,edge_id);"#, |
There was a problem hiding this comment.
When a writer or replay supplies endpoints of the wrong kinds, these foreign keys verify only that the relation and objects exist; they never require the source and target objects' object_kind values to match the relation registry's source_kind and target_kind. Such an edge is therefore accepted as canonical even though consumers rely on the registry to describe its valid endpoint types, so bind those discriminators with an enforceable constraint or equivalent database guard.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. Folded into the same tracking task as the registry-agreement gap you raised on entities, since both are the same defect: the foreign key proves the referenced row exists but never that its discriminator agrees.
Not fixed in the schema because enforcing source_object_id.object_kind = relation_registry.source_kind requires reading two other tables, which SQLite can only do from a trigger. U4's version-bound write helpers are the right layer, and putting it there covers the endpoint kinds, the registry field agreement, and the declared cardinality with one enforced path instead of three sets of triggers.
| ), | ||
| ( | ||
| "object_registry", | ||
| r#"CREATE TABLE object_registry(object_id TEXT PRIMARY KEY,object_kind TEXT NOT NULL,domain_id TEXT NOT NULL REFERENCES domains(domain_id) DEFERRABLE INITIALLY DEFERRED,source_kind TEXT NOT NULL,source_id TEXT NOT NULL,source_revision INTEGER NOT NULL,created_commit_seq INTEGER NOT NULL REFERENCES commit_log(commit_seq) ON DELETE RESTRICT,invalidated_commit_seq INTEGER REFERENCES commit_log(commit_seq) ON DELETE RESTRICT,superseded_by TEXT REFERENCES object_registry(object_id) DEFERRABLE INITIALLY DEFERRED,sensitivity_class TEXT NOT NULL,UNIQUE(source_kind,source_id,source_revision,object_kind)) STRICT; CREATE INDEX idx_objects_known_as_of ON object_registry(created_commit_seq,invalidated_commit_seq,object_id); CREATE INDEX idx_objects_domain_fk ON object_registry(domain_id,object_id); CREATE INDEX idx_objects_superseded_fk ON object_registry(superseded_by);"#, |
There was a problem hiding this comment.
When a writer or replay supplies an invalidated_commit_seq that is equal to or earlier than created_commit_seq, both foreign keys succeed and the canonical row is accepted even though it is absent from every normal known_as_of interval. The same unconstrained pair is repeated across the typed canonical tables, so malformed corrections can silently disappear from snapshots; add an interval check requiring invalidation to be null or later than creation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 902aa20. Added to every canonical table:
CHECK(invalidated_commit_seq IS NULL OR invalidated_commit_seq>created_commit_seq)Worth noting it rejects equality as well as inversion, which is slightly stronger than "later than creation" implies. KTD6 filters created <= N < invalidated, so invalidated == created yields an interval that is empty in every snapshot — the same disappearing-correction failure, just at the boundary.
inverted_and_empty_validity_intervals_are_refused covers both cases plus the legal one.
| ), | ||
| ( | ||
| "evidence_meta", | ||
| r#"CREATE TABLE evidence_meta(evidence_id TEXT PRIMARY KEY,object_id TEXT NOT NULL UNIQUE REFERENCES object_registry(object_id),artifact_reference TEXT NOT NULL,artifact_digest TEXT NOT NULL,byte_length INTEGER NOT NULL,media_type TEXT NOT NULL,retention_class TEXT NOT NULL,retain_until INTEGER,detector_kind TEXT,detector_version TEXT,detector_metadata BLOB,detector_id TEXT,secret_type TEXT,utf8_offset INTEGER,utf8_length INTEGER,provider_egress_class TEXT NOT NULL,redaction_metadata BLOB NOT NULL,created_commit_seq INTEGER NOT NULL REFERENCES commit_log(commit_seq),invalidated_commit_seq INTEGER REFERENCES commit_log(commit_seq),superseded_by TEXT REFERENCES object_registry(object_id),sensitivity_class TEXT NOT NULL) STRICT; CREATE INDEX idx_evidence_retention ON evidence_meta(retain_until,evidence_id); CREATE INDEX idx_evidence_known_as_of ON evidence_meta(created_commit_seq,invalidated_commit_seq,evidence_id);"#, |
There was a problem hiding this comment.
When an evidence row represents a detected secret, the unrestricted detector_metadata blob can durably retain arbitrary detector output—including the matched secret itself—alongside the sanitized detector ID, secret type, offset, and length fields. That bypasses the ingestion rule that secret bytes are redacted before any durable write; either remove this free-form field for secret detections or enforce that such rows can store only the bounded sanitized metadata.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. Deferring to the redaction unit with a tracking task, because the fix is a write-path property rather than a schema one.
The plan backs you up. U6 step 4 requires detection metadata to carry "detector id, secret type, offset/length; never matched bytes", and R11 says vocabulary-covered secret bytes never reach any durable write. evidence_meta does carry that sanitized set in four dedicated columns (detector_id, secret_type, utf8_offset, utf8_length) — the problem is that detector_kind, detector_version, and detector_metadata sit beside them as a second, unbounded vocabulary the schema cannot tell apart from the first.
Not fixed here because the detector runs in memory before the first SQL bind (R11), so the schema has no way to know whether a given blob came from a secret detection. The task covers the two real options — drop detector_metadata for secret detections, or have redaction enforcement refuse a non-empty blob on any row with secret_type set — and extends the planted-secret byte scan to cover the column explicitly rather than relying on it being unused.
Release builds could skip schema-name validation, and SQLite could report a non-WAL journal mode without failing, so the bootstrap could claim a profile it never installed. Const evaluation now proves COMPONENTS/KERNEL_SCHEMA_COMPONENT_NAMES agreement, the profile rejects connections that cannot enter WAL, and the schema digest is pinned to a checked-in golden so DDL drift fails a test. The synchronous [2,3] and trusted_schema rules move into a kernel-scoped verify_kernel_connection_contract; the shared verify_sqlite_connection_contract keeps the [1,2,3] vocabulary the TypeScript host's verifier mirrors. The consumer-checkpoint test covers two consumers advancing independently plus the MIN retention floor.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72afb135b6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ), | ||
| ( | ||
| "mc_kernel_format_marker", | ||
| r#"CREATE TABLE mc_kernel_format_marker(singleton INTEGER PRIMARY KEY CHECK(singleton=1),format_epoch INTEGER NOT NULL,database_incarnation_id TEXT NOT NULL,schema_digest TEXT NOT NULL,created_at INTEGER NOT NULL) STRICT;"#, |
There was a problem hiding this comment.
Even after correcting the application ID, a freshly bootstrapped kernel database still creates mc_kernel_format_marker with a kernel-specific column set, while the existing pristine-or-exact identity path looks for mc_format_marker and validates its component-manifest and marker digests. Any opener using that established vocabulary will therefore classify this database as malformed and quarantine it; create the shared marker shape and digest instead of defining a second identity protocol.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining this one — the kernel-specific marker table is the decided design, not an accidental second protocol. KTD8:
Identity manifest strengthened to normalized schema-SQL digest plus exact non-internal object inventory, in new kernel-specific fixture with kernel-specific marker table (
mc_kernel_format_marker) so kernel identity decidable against legacy files (KTD3). [...] Existingdirect-format-vocabulary-v1.jsontarget retired store.db shape, not reused.
The premise that an opener would quarantine this database is inverted. R2 and KTD3 make quarantine conditional on the kernel's own marker being present:
Quarantine fire only when kernel's own marker table prove file is kernel database at wrong shape. Non-empty family without marker (mis-targeted store.db, foreign SQLite file, path-config error) is
foreign: typed unavailable, nothing moved.
So a distinct marker name is exactly what stops a direct-format or legacy file from being destroyed by the kernel opener, and vice versa. A shared mc_format_marker name would make the two families indistinguishable and put both under each other's quarantine path.
The column sets differ for the same reason: the direct format's component_manifest_digest is computed over a schema composer's dependency graph (compute_schema_manifest_digest), which the kernel does not have — its components are a static ordered list and its digest is a normalized sqlite_schema digest per KTD8. Adopting the TS shape would mean inventing a composer to fill a column.
Your sibling point about the application_id was right, though, and is fixed in fb248d7 — KTD8 does keep MCTX. It is only the marker shape that is deliberately kernel-specific.
| ), | ||
| ( | ||
| "relation_registry", | ||
| r#"CREATE TABLE relation_registry(relation_id TEXT PRIMARY KEY,object_id TEXT NOT NULL UNIQUE REFERENCES object_registry(object_id),relation_name TEXT NOT NULL UNIQUE,source_kind TEXT NOT NULL,target_kind TEXT NOT NULL,symmetry TEXT NOT NULL,cardinality TEXT NOT NULL,created_commit_seq INTEGER NOT NULL REFERENCES commit_log(commit_seq),invalidated_commit_seq INTEGER REFERENCES commit_log(commit_seq),superseded_by TEXT REFERENCES object_registry(object_id),sensitivity_class TEXT NOT NULL) STRICT; CREATE INDEX idx_rel_known_as_of ON relation_registry(created_commit_seq,invalidated_commit_seq,relation_id);"#, |
There was a problem hiding this comment.
When a registry entry declares a one-to-one or otherwise bounded cardinality, this schema merely stores the declaration: asserted_edges has no uniqueness or trigger tying the permitted source/target multiplicity to relation_registry.cardinality. Multiple canonical edges that violate the registered relation are therefore accepted, so consumers cannot rely on the registry's advertised cardinality; add enforceable constraints or database guards for the supported cardinality modes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. Folded into the same tracking task as the endpoint-kind and registry-agreement gaps, for the same structural reason.
Declared cardinality is the hardest of the three to express declaratively: enforcing one_to_one would need a unique index on asserted_edges(relation_id, source_object_id) that applies only when a row in another table holds a particular cardinality value, and a partial index predicate cannot reference another table. So it is a trigger or the write path, and U4's version-bound helpers already own the equivalent checks.
Worth noting the enforcement will also have to be interval-aware: uniqueness has to hold over active rows only, or an R8 correction that appends a successor edge would collide with the predecessor it supersedes. That is the same trap the name constraints fell into on this PR, so the task records it explicitly.
| ), | ||
| ( | ||
| "outbox_consumers", | ||
| r#"CREATE TABLE outbox_consumers(consumer_id TEXT PRIMARY KEY,checkpoint_outbox_position INTEGER NOT NULL DEFAULT 0,updated_at INTEGER NOT NULL) STRICT; CREATE INDEX idx_consumers_checkpoint ON outbox_consumers(checkpoint_outbox_position,consumer_id);"#, |
There was a problem hiding this comment.
When a consumer accidentally acknowledges a future outbox position—or moves its checkpoint backward—this unrestricted integer update succeeds. In the future-position case, a single required consumer makes the minimum acknowledged position exceed rows it never consumed, allowing pruning to discard undelivered events; backward movement can instead pin retention indefinitely. Enforce monotonic advancement and reject positions beyond the current outbox high-water mark.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Half confirmed and fixed; the other half would introduce a bug.
Backward movement — fixed in fb248d7. You're right that this pins retention indefinitely, since R9 derives the prune boundary from the minimum required-consumer checkpoint. Added:
CREATE TRIGGER outbox_consumers_checkpoint_monotonic
BEFORE UPDATE OF checkpoint_outbox_position ON outbox_consumers
WHEN NEW.checkpoint_outbox_position<OLD.checkpoint_outbox_position
BEGIN SELECT RAISE(ABORT,'checkpoint_outbox_position must not move backward'); END;Equal-position re-acknowledgement stays legal so an idempotent redelivery is not an error. consumer_checkpoints_advance_but_never_retreat covers advance, retreat, and replay.
Bound to the outbox high-water mark — declining. "Reject positions beyond the current outbox high-water mark" would reject legitimate advancement, for two independent reasons. KTD10:
Consumer checkpoints =
commit_seqwatermarks acknowledged at whole-commit boundaries: consumer that processed every outbox row may advance past outbox-empty commits to commit-log tip, so event lag per policy R12d (maxcommit_seq− min watermark) read zero when caught up.
So a caught-up consumer is expected to move past any existing outbox position — otherwise event lag never reads zero. Separately, pruning deletes acknowledged rows, so MAX(outbox_position) shrinks over time and a bound derived from it would start refusing checkpoints it previously accepted.
The undelivered-events risk you identify is real, but the safe bound is the true high-water mark known to the writer, not one recoverable from the surviving rows. That belongs in the consumer-checkpoint write path in U5, alongside the registration rule that seeds a checkpoint immediately before the oldest retained position.
| const COMPONENTS: &[(&str, &str)] = &[ | ||
| ( | ||
| "commit_log", | ||
| r#"CREATE TABLE commit_log(commit_seq INTEGER PRIMARY KEY AUTOINCREMENT,transaction_id TEXT NOT NULL UNIQUE,writer_epoch INTEGER NOT NULL,recorded_at INTEGER NOT NULL,actor TEXT NOT NULL,cause TEXT NOT NULL) STRICT;"#, |
There was a problem hiding this comment.
When any maintenance path or later writer bug updates a committed row, fields such as writer_epoch, recorded_at, actor, and cause can be rewritten in place because this table has no update guard. That silently changes the authoritative transaction history used for audit and full-log replay without allocating a new commit_seq; make committed log rows immutable at the database boundary.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in fb248d7. This was the best catch in the batch.
commit_log is the authoritative history for audit and full-log replay, retained indefinitely under R8, and nothing stopped an in-place rewrite of writer_epoch, recorded_at, actor, or cause — nor deletion of an unreferenced row, since ON DELETE RESTRICT on the children only protects rows that happen to be referenced.
CREATE TRIGGER commit_log_no_update BEFORE UPDATE ON commit_log
BEGIN SELECT RAISE(ABORT,'commit_log is append-only'); END;
CREATE TRIGGER commit_log_no_delete BEFORE DELETE ON commit_log
BEGIN SELECT RAISE(ABORT,'commit_log is append-only'); END;Same guard shape as the format marker. commit_log_rejects_update_and_delete asserts field rewrites and deletion both abort and the original row survives.
| let tx = conn.transaction()?; | ||
| tx.pragma_update(None, "application_id", KERNEL_APPLICATION_ID)?; |
There was a problem hiding this comment.
When apply_kernel_schema is called on a database containing unrelated objects whose names do not collide with these components, every CREATE succeeds and the digest is computed over both the foreign objects and the kernel schema, so the transaction stamps that mixed database as a completed kernel format. The resulting digest differs from the frozen shape and a subsequent exact opener must quarantine it, potentially taking the pre-existing data with it; verify that the schema is pristine before setting identity or creating components.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in fb248d7. apply_schema now refuses before touching identity or creating anything:
let existing_objects: i64 = tx.query_row(
"SELECT COUNT(*) FROM sqlite_schema WHERE name NOT LIKE 'sqlite\\_%' ESCAPE '\\'",
[], |row| row.get(0))?;
if existing_objects != 0 {
return Err(rusqlite::Error::InvalidQuery);
}Scoping note on what this does and does not claim: R2's four-outcome classification (pristine / exact / kernel-mismatch / foreign) belongs to the opener in U3, and this is not an attempt to pre-empt it. It only stops the bootstrap primitive from being the thing that stamps a mixed database — which, as you note, is what would later hand a foreign file to quarantine and take the pre-existing data with it.
bootstrap_refuses_a_database_holding_foreign_objects asserts the refusal, that application_id is left at 0, and that the foreign table and its rows survive untouched.
| ), | ||
| ( | ||
| "extraction_runs", | ||
| r#"CREATE TABLE extraction_runs(extraction_run_id TEXT PRIMARY KEY,extractor TEXT NOT NULL,source_kind TEXT,source_id TEXT,source_revision INTEGER,sensitivity_class TEXT NOT NULL,provenance_witness BLOB NOT NULL,redaction_metadata BLOB NOT NULL,detector_id TEXT,secret_type TEXT,utf8_offset INTEGER,utf8_length INTEGER,started_at INTEGER NOT NULL,heartbeat_at INTEGER NOT NULL,lease_expires_at INTEGER NOT NULL,terminal_state TEXT,terminal_at INTEGER) STRICT; CREATE INDEX idx_runs_ttl ON extraction_runs(terminal_at,lease_expires_at,extraction_run_id); CREATE INDEX idx_runs_heartbeat ON extraction_runs(terminal_at,heartbeat_at,extraction_run_id);"#, |
There was a problem hiding this comment.
When a malformed or replayed extraction run writes a lease_expires_at arbitrarily far beyond heartbeat_at, this row remains active indefinitely because the schema imposes no relationship between the two timestamps. That defeats the one-hour renewable-lease boundary and prevents the staging reaper from abandoning a stuck run, retaining its candidates and potentially sensitive provenance without limit; reject inverted leases and expirations beyond the configured maximum lease interval.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Split verdict: the inverted-lease half is fixed, the maximum-interval half stays out of the schema.
Fixed in fb248d7 — CHECK(lease_expires_at>heartbeat_at) on both extraction_runs and candidates. That rejects a row written with a lease already dead at write time, including the zero-length case. staging_leases_must_outlive_their_heartbeat covers both tables.
Not fixed: the maximum-interval bound. A CHECK would have to compare against a literal, which bakes a policy constant into a frozen schema — changing the lease interval would then need a format-epoch bump. The interval belongs with the reaper, which is where the TTL sweep already lives (KTD10: "only sweep = staging TTL at open plus explicit maintenance entry point").
One correction to the premise: the one-hour renewable-lease boundary is not in this plan. R10 specifies a 30-day staging TTL with the clock starting at run completion or explicit abandonment, and explicitly says incomplete runs do not expire. So a stuck run is bounded by the reaper's lease check, not by TTL, which makes your underlying point about the reaper needing a defensible bound correct even though the specific interval was not the one stated.
The frozen kernel schema violated its own append-only, replay, and identity requirements in seven places. Supersession (R8, U4). A correction invalidates the predecessor row and appends a successor; the predecessor is retained and never deleted. The unconditional name constraints rejected that successor, so a predicate schema, domain, or relation could never be corrected. Replace them with partial unique indexes over active rows only, and restore idx_predicate_domain_fk, which the previous unconditional UNIQUE had covered and a partial index cannot. Canonical deletion (R8). Six references to canonical parents used ON DELETE SET NULL, which permits a delete the model forbids and rewrites history in place: a known_as_of read before the delete could no longer recover which evidence supported an edge, decision, or observation. Tighten them to RESTRICT. admission_decisions.candidate_id keeps SET NULL because staging rows do expire under R10 while the decision survives. Validity intervals (KTD6). Nothing rejected an invalidation at or before creation, so a malformed correction was absent from every created <= N < invalidated snapshot. Add the interval check to each canonical table. Commit ordering (R4, R9). consumer_abandonments and admission_decisions recorded only wall-clock time, so replay could not place them relative to canonical commits, and R9 requires the abandonment audit in the commit log. Add a commit_log reference to each: required for abandonment, which is a canonical control fact, and nullable for admission decisions, since a rejection produces no canonical commit. Object identity. UNIQUE(source_kind, source_id, source_revision, object_kind) allowed at most one object of a kind per source revision, which made ordinary multi-fact admission impossible even though candidates permits many results per run. Idempotency belongs to operation_receipts and change_event.idempotency_key, so demote it to a plain lookup index. Marker immutability. The migration contract calls the marker row immutable, and the TypeScript direct-format marker enforces that with no-update and no-delete triggers. The kernel marker had no guard, so a maintenance write could restamp the incarnation, epoch, or digest. Mirror the triggers. Bootstrap locking (U4). apply_schema opened a DEFERRED transaction, so a concurrent opener took a shared read lock and failed SQLITE_BUSY when the first DDL statement upgraded it rather than waiting out busy_timeout. Use BEGIN IMMEDIATE, which both U4 and the migration doc already specify. asserted_edges, decisions, and observations also lacked the scope, anchor, proposition, and superseded_by indexes their sibling tables declare. The pinned schema digest moves with the shape.
KTD8 keeps the MCTX application_id for the kernel and makes mc_kernel_format_marker the discriminator, so a non-pristine family without that table classifies as foreign and is refused rather than quarantined (R2, KTD3). The kernel stamped its own MCKR value instead, which broke the identity vocabulary restructure R6 pins. Alias the shared constant so one definition remains. commit_log is the authoritative history for audit and full-log replay and is retained indefinitely under R8, but nothing stopped a maintenance path from rewriting writer_epoch, recorded_at, actor, or cause in place, or from deleting an unreferenced row. Add no-update and no-delete triggers, matching the marker guard. R9 derives the prune boundary from the minimum required-consumer checkpoint, so a checkpoint that moves backward pins retention indefinitely. Add a trigger refusing retreat while leaving equal-position re-acknowledgement legal. The companion bound, rejecting positions past the outbox high-water mark, is not expressible here: KTD10 lets a caught-up consumer advance past outbox-empty commits to the commit-log tip, and pruning removes rows, so MAX(outbox_position) would reject valid advancement. extraction_runs and candidates imposed no relationship between heartbeat_at and lease_expires_at, so a row could be written with a lease that was already dead. Add the interval check. Bounding the lease to a maximum renewable interval stays with the reaper, since the interval is policy rather than schema. apply_kernel_schema created its components over whatever already existed, folding foreign objects into the digest and stamping the mixed database as a complete kernel format. Refuse a non-pristine schema before setting identity. R2's four-outcome classification still belongs to the opener; this only stops the bootstrap primitive from being the one that destroys pre-existing data.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb248d73f5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ), | ||
| ( | ||
| "propositions", | ||
| r#"CREATE TABLE propositions(proposition_id TEXT PRIMARY KEY,object_id TEXT NOT NULL UNIQUE REFERENCES object_registry(object_id),subject_id TEXT NOT NULL REFERENCES object_registry(object_id),predicate_schema_id TEXT NOT NULL REFERENCES predicate_schemas(predicate_schema_id),value BLOB NOT NULL,value_schema_id TEXT NOT NULL,normalized_hash TEXT NOT NULL,scope_id TEXT REFERENCES scopes(scope_id),anchor_id TEXT REFERENCES anchors(anchor_id),created_commit_seq INTEGER NOT NULL REFERENCES commit_log(commit_seq),invalidated_commit_seq INTEGER REFERENCES commit_log(commit_seq),superseded_by TEXT REFERENCES object_registry(object_id),sensitivity_class TEXT NOT NULL,CHECK(invalidated_commit_seq IS NULL OR invalidated_commit_seq>created_commit_seq)) STRICT; CREATE INDEX idx_prop_known_as_of ON propositions(created_commit_seq,invalidated_commit_seq,proposition_id); CREATE INDEX idx_prop_subject_fk ON propositions(subject_id,predicate_schema_id); CREATE INDEX idx_prop_predicate_fk ON propositions(predicate_schema_id); CREATE INDEX idx_prop_scope_fk ON propositions(scope_id); CREATE INDEX idx_prop_anchor_fk ON propositions(anchor_id);"#, |
There was a problem hiding this comment.
When a correction executes UPDATE propositions SET value = ..., the schema accepts it without allocating a new commit_seq; every known_as_of snapshot then sees the replacement bytes, including snapshots predating the correction. Since the table already exposes invalidation and supersession fields for append-only correction, guard immutable proposition fields against updates while permitting only the lifecycle transition.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed as a real gap, but declining to fix it in the schema — a blanket trigger here would block a write R8 explicitly permits.
R8 grants exactly two mutations of an existing canonical row, and the second is the problem:
sole permitted mutations of existing canonical row = set
invalidated_commit_seqfrom NULL exactly once, monotonic, inside envelope, plus operator-authorized secret-remediation overwrite below [...] operator-authorized remediation may overwrite single named field on located row with redaction placeholder to remove detector-missed secret
So UPDATE propositions SET value = ... is legal for exactly one caller: an authorized remediation removing a secret the detector missed. A BEFORE UPDATE OF value trigger cannot tell that caller from a buggy one, since both arrive as the same statement on the same row.
This is why the plan assigns it to U5 rather than U2 — "Append-only guards enforce R8 content-append-only rule on canonical tables (invalidation-bound writes only, through envelope), one audited exception" — and the guard has to be authority-aware, which means it needs the envelope's context.
Worth noting where I did take your approach: commit_log got exactly the blanket no-update trigger you describe here, in fb248d7. The difference is that commit_log has no permitted update at all — no invalidation lifecycle, no remediation exception — so a blanket guard is complete there and incomplete here. Same for the format marker.
| ), | ||
| ( | ||
| "decision_events", | ||
| r#"CREATE TABLE decision_events(decision_id TEXT NOT NULL REFERENCES decisions(decision_id) ON DELETE CASCADE,event_ordinal INTEGER NOT NULL,commit_seq INTEGER NOT NULL REFERENCES commit_log(commit_seq),event_kind TEXT NOT NULL,event_payload BLOB NOT NULL,evidence_id TEXT REFERENCES evidence_meta(evidence_id) ON DELETE RESTRICT,recorded_at INTEGER NOT NULL,PRIMARY KEY(decision_id,event_ordinal)) STRICT; CREATE INDEX idx_decision_events_commit ON decision_events(commit_seq,decision_id,event_ordinal); CREATE INDEX idx_decision_events_evidence_fk ON decision_events(evidence_id);"#, |
There was a problem hiding this comment.
When a decision row is deleted, this cascade also deletes every decision_events row, while the corresponding object_registry row remains and still claims that the decision exists. That irreversibly removes canonical event history and leaves snapshots internally inconsistent; canonical decisions should be invalidated and superseded, so reject their deletion rather than cascading it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 5ec00e5, and I extended it past decisions because the same reasoning covers five more edges.
Your argument generalizes: R8 says canonical rows are content-append-only with "DELETE never", so any ON DELETE CASCADE from a canonical parent encodes a deletion path the model forbids, and the internally-inconsistent-snapshot outcome you describe is the same in each case. Now RESTRICT on decision_events.decision_id, alignment_projection.decision_id, alignment_projection.observation_id, observation_dependencies.observation_id, scope_term.scope_id, and entity_aliases.entity_id.
Three cascades deliberately survive, because their parents genuinely are deletable: candidate_scores.candidate_id and candidates.extraction_run_id (staging, expires under R10's TTL) and capture_pin_refs.capture_pin_id (operational pins, not canonical).
canonical_parents_refuse_deletion_instead_of_cascading asserts the decision delete is refused and its event row survives. This pairs with the SET NULL to RESTRICT change in 902aa20 — same principle, other direction.
| ), | ||
| ( | ||
| "entity_aliases", | ||
| r#"CREATE TABLE entity_aliases(entity_id TEXT NOT NULL REFERENCES entities(entity_id) ON DELETE CASCADE,alias TEXT NOT NULL,alias_kind TEXT NOT NULL,created_commit_seq INTEGER NOT NULL REFERENCES commit_log(commit_seq),invalidated_commit_seq INTEGER REFERENCES commit_log(commit_seq),superseded_by TEXT REFERENCES object_registry(object_id),sensitivity_class TEXT NOT NULL,PRIMARY KEY(entity_id,alias,alias_kind),CHECK(invalidated_commit_seq IS NULL OR invalidated_commit_seq>created_commit_seq)) STRICT; CREATE INDEX idx_alias_lookup ON entity_aliases(alias,alias_kind,entity_id); CREATE INDEX idx_alias_known_as_of ON entity_aliases(created_commit_seq,invalidated_commit_seq,entity_id);"#, |
There was a problem hiding this comment.
When an alias is invalidated and later re-established for the same entity and alias kind, the retained historical row still occupies the (entity_id, alias, alias_kind) primary key, so inserting the new validity interval fails. This defeats the append-only lifecycle represented by created_commit_seq and invalidated_commit_seq; use a distinct row identity or include the creation commit in the key, with active-row uniqueness if needed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 5ec00e5. Good catch — this is the same defect as the name constraints elsewhere in this review, and I missed it because it was hidden in a PRIMARY KEY rather than a UNIQUE.
Took your first suggestion, adding the creation commit to the key, plus active-row uniqueness:
PRIMARY KEY(entity_id,alias,alias_kind,created_commit_seq)
CREATE UNIQUE INDEX idx_alias_active
ON entity_aliases(entity_id,alias,alias_kind) WHERE invalidated_commit_seq IS NULL;The partial index is load-bearing rather than optional: without it, widening the key would let two active rows hold the same alias, which is a different bug. invalidated_alias_can_be_reintroduced_for_the_same_entity covers the retire-and-revive path, asserts a second active row is still refused, and asserts both intervals survive so the history is not overwritten.
| ), | ||
| ( | ||
| "outbox_consumers", | ||
| r#"CREATE TABLE outbox_consumers(consumer_id TEXT PRIMARY KEY,checkpoint_outbox_position INTEGER NOT NULL DEFAULT 0,updated_at INTEGER NOT NULL) STRICT; CREATE INDEX idx_consumers_checkpoint ON outbox_consumers(checkpoint_outbox_position,consumer_id); CREATE TRIGGER outbox_consumers_checkpoint_monotonic BEFORE UPDATE OF checkpoint_outbox_position ON outbox_consumers WHEN NEW.checkpoint_outbox_position<OLD.checkpoint_outbox_position BEGIN SELECT RAISE(ABORT,'checkpoint_outbox_position must not move backward'); END;"#, |
There was a problem hiding this comment.
When a lagging required consumer is deleted directly, it immediately disappears from the minimum-checkpoint calculation even if it has neither caught up nor received an authorized abandonment. For example, deleting the only consumer at position 0 while another is at 100 permits pruning through 100 and loses events the removed consumer never processed; guard deletion so it requires a matching, commit-ordered abandonment recording the consumer's current checkpoint.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed as a real hole, deferring to U5 rather than fixing it in the schema, because the guard you describe would refuse a deregistration R9 permits.
Your worked example is right: deleting the consumer at position 0 while another sits at 100 moves the minimum-checkpoint prune boundary to 100 and loses everything the removed consumer never processed. R9 agrees that deletion must be gated:
caught-up deregistration succeed; pending deregistration refuse unless operator-authorized abandonment record audit fields in commit log
But that is two paths, not one. A trigger requiring a matching abandonment row would also block the caught-up case, which is the normal, unaudited way to deregister. Distinguishing them needs "is this consumer caught up", and that is not computable from the schema for the same reason the outbox high-water bound is not: KTD10 lets a caught-up consumer advance past outbox-empty commits to the commit-log tip, and pruning deletes acknowledged rows, so no surviving-row expression identifies the tip.
So the enforcement point is U5's deregistration path, which knows the tip and can route to the caught-up or abandonment branch. What did land in the schema is the half that is unconditional: consumer_abandonments.commit_seq is now NOT NULL (902aa20), so an abandonment cannot be recorded without its authorizing commit, and the checkpoint monotonicity trigger (fb248d7) stops the other direction of the same retention bug.
| ), | ||
| ( | ||
| "writer_fence", | ||
| r#"CREATE TABLE writer_fence(id INTEGER PRIMARY KEY CHECK(id=0),writer_epoch INTEGER) STRICT;"#, |
There was a problem hiding this comment.
When the active writer has advanced this row to epoch 8, an update back to epoch 7 or to NULL succeeds. A stale epoch-7 writer can then satisfy a fence comparison that should permanently reject it, defeating lease handover and allowing canonical commits from an obsolete writer; require non-null monotonic advancement after initialization and prevent deletion of the singleton fence row.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Split verdict. The delete half is fixed; strict monotonicity would break the documented restore path.
Fixed in 5ec00e5 — writer_fence_no_delete. KTD11 declares the fence row in the bootstrap DDL and inside the manifest digest, and nothing legitimately removes it, so refusing deletion is unambiguous.
Not enforcing monotonic advancement. KTD11 requires the fence to be writable downward:
restored database whose column max exceed fresh lease root epoch would fence new writer out permanently; restore therefore reclaim fence row with destination lease epoch
That reclaim is the documented remedy for a restored database, and the destination lease epoch can be lower than the epoch the restored file carries. A trigger enforcing NEW >= OLD would refuse it and leave the restored database permanently unopenable — the exact failure KTD11 is designing around.
Your underlying concern is right, though, and it is not really about monotonicity: it is about authority. An authorized restore may lower the fence; a stale epoch-7 writer may not. A schema trigger cannot distinguish them, because both arrive as an UPDATE on the same row. That check belongs in the open path, which knows whether it is holding a fresh lease or performing a restore, and it is the same shape as the append-only-guard problem on canonical tables: the guard has to permit one privileged writer and refuse everyone else.
Tracked with the lifecycle work rather than left implicit. The nullability question you raised earlier is a separate matter and is answered on that thread — NULL is the "declared, not yet stamped" state KTD11 needs to stay distinguishable.
| // `PRAGMA journal_mode` returns the resulting mode; reject a mode other than WAL. | ||
| let journal_mode: String = | ||
| conn.pragma_update_and_check(None, "journal_mode", "WAL", |row| row.get(0))?; |
There was a problem hiding this comment.
When this setup runs with an SQLite engine below MIN_SUPPORTED_SQLITE_VERSION or with an unrecognized source identity, it switches the real database into WAL and returns success without ever invoking the existing off-path runtime gate. A repo-wide search finds no kernel opener or non-test caller that performs probe_sqlite_engine_identity_off_path plus evaluate_sqlite_runtime_gate, so the kernel can run on an engine excluded for the WAL-reset corruption race; perform that gate before opening or profiling the real file.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and this is the most serious finding in the batch. Fixed in 5ec00e5.
Your repo-wide search matches mine: nothing called probe_sqlite_engine_identity_off_path plus evaluate_sqlite_runtime_gate outside tests, so the gate existed and was never on the path. Two contract lines make this a defect rather than a deferral to the opener:
- R3: "engine gate ≥ 3.47.1 (WAL-reset fix), WAL,
synchronous=FULL, ... applied per connection and verified, not assumed" - KTD2, listing the reasons the cortexkit-store opener could not be reused: "
open_sqliteenable WAL before any identity classification"
So enabling WAL before checking anything is the specific flaw that forced writing a kernel-owned open path, and apply_kernel_connection_profile had reproduced it — pragma_update_and_check(journal_mode, WAL) was its first statement against the real file.
apply_kernel_connection_profile now gates first:
let identity = crate::sqlite_runtime::probe_sqlite_engine_identity_off_path()?;
if !crate::sqlite_runtime::evaluate_sqlite_runtime_gate(&identity).is_empty() {
return Err(rusqlite::Error::InvalidQuery);
}Probing off-path on an in-memory connection, so the excluded engine never touches the real file. Note the floor is [3, 51, 3], not the 3.47.1 in R3's prose — MIN_SUPPORTED_SQLITE_VERSION is the authority and is already higher.
| ), | ||
| ( | ||
| "extraction_runs", | ||
| r#"CREATE TABLE extraction_runs(extraction_run_id TEXT PRIMARY KEY,extractor TEXT NOT NULL,source_kind TEXT,source_id TEXT,source_revision INTEGER,sensitivity_class TEXT NOT NULL,provenance_witness BLOB NOT NULL,redaction_metadata BLOB NOT NULL,detector_id TEXT,secret_type TEXT,utf8_offset INTEGER,utf8_length INTEGER,started_at INTEGER NOT NULL,heartbeat_at INTEGER NOT NULL,lease_expires_at INTEGER NOT NULL,terminal_state TEXT,terminal_at INTEGER,CHECK(lease_expires_at>heartbeat_at)) STRICT; CREATE INDEX idx_runs_ttl ON extraction_runs(terminal_at,lease_expires_at,extraction_run_id); CREATE INDEX idx_runs_heartbeat ON extraction_runs(terminal_at,heartbeat_at,extraction_run_id);"#, |
There was a problem hiding this comment.
When a run is marked terminal_state = 'completed' with terminal_at left null, the row passes every constraint but has no timestamp from which the 30-day staging retention period can be calculated; it can remain indefinitely or be mistaken for live expired work by the lease index. The inverse malformed state is also accepted, and candidates repeats the same shape, so require terminal state and timestamp to be null or non-null together.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 5ec00e5. Added to both tables:
CHECK((terminal_state IS NULL)=(terminal_at IS NULL))R10 starts the 30-day clock "at run completion or explicit abandonment" and explicitly says incomplete runs do not expire, so a terminal row without a timestamp is exactly the unreclaimable state you describe: too terminal to be reaped as live work, with no clock to expire from.
staging_terminal_state_and_timestamp_are_set_together covers both malformed directions and both legal ones, on extraction_runs and candidates.
Gate each connection before enabling WAL so excluded SQLite engines cannot enter the WAL-reset corruption race. apply_kernel_connection_profile switched the real file to WAL as its first act, and no caller ran probe_sqlite_engine_identity_off_path or evaluate_sqlite_runtime_gate, so R3's per-connection verification never happened. entity_aliases keyed on (entity_id, alias, alias_kind) while carrying created and invalidated commit bounds, so an invalidated alias could never be reintroduced: the retained historical row still occupied the key. Add created_commit_seq to the key and enforce single occupancy through a partial unique index over active rows. Six ON DELETE CASCADE edges pointed at canonical parents, which R8 never deletes. Deleting a decision destroyed its decision_events and alignment_projection rows while the object_registry row still claimed the decision existed. Tighten decisions, observations, scopes, and entities to RESTRICT. candidates, extraction_runs, and capture_pins keep CASCADE: staging rows expire under R10 and pins are operational. Add a no-delete trigger for the writer_fence singleton, which KTD11 declares in the bootstrap DDL and inside the manifest digest. Epoch monotonicity stays unenforced: KTD11 has restore reclaim the fence row with the destination lease epoch, which may be lower than the stored one, so a monotonic trigger would break restore. Telling an authorized restore from a stale writer needs the open path, not a trigger. extraction_runs and candidates accepted a terminal_state without a terminal_at, leaving a completed row with no clock for R10's 30-day retention, and the inverse. Require the pair to be null or non-null together.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ec00e5f0f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let existing_objects: i64 = tx.query_row( | ||
| "SELECT COUNT(*) FROM sqlite_schema WHERE name NOT LIKE 'sqlite\\_%' ESCAPE '\\'", | ||
| [], | ||
| |row| row.get(0), | ||
| )?; |
There was a problem hiding this comment.
When a database has no schema objects but already carries a nonzero application_id or user_version—for example, a foreign or interrupted header-only file—this query returns zero and bootstrap subsequently overwrites its identity. The earlier non-pristine fix only inspects sqlite_schema; this is fresh evidence that header-only non-pristine files still pass. The pristine-or-exact contract requires such a file to remain unchanged, so check the relevant header pragmas before creating or stamping anything.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 9c25fd8. You're right that the earlier fix only inspected sqlite_schema, so a header-only file slipped through:
let stamped_application_id: i64 = tx.query_row("PRAGMA application_id", [], |row| row.get(0))?;
let stamped_user_version: i64 = tx.query_row("PRAGMA user_version", [], |row| row.get(0))?;
if stamped_application_id != 0 || stamped_user_version != 0 {
return Err(rusqlite::Error::InvalidQuery);
}bootstrap_refuses_a_header_only_database covers both pragmas and asserts the foreign header survives untouched.
Scope note: R2 defines pristine at the file level — "absent or zero-byte main file, no WAL/SHM/journal/reset-marker sidecars" — which the opener owns in U3, since a Connection cannot see whether sidecars existed before it attached. This check is the in-database subset, so it narrows the primitive's blast radius rather than implementing R2.
| ), | ||
| ( | ||
| "change_event", | ||
| r#"CREATE TABLE change_event(commit_seq INTEGER NOT NULL REFERENCES commit_log(commit_seq) ON DELETE RESTRICT,ordinal INTEGER NOT NULL,object_id TEXT NOT NULL,change_kind TEXT NOT NULL,source_span_id TEXT,idempotency_key TEXT NOT NULL,payload BLOB,PRIMARY KEY(commit_seq,ordinal)) STRICT; CREATE INDEX idx_change_event_object_known_as_of ON change_event(object_id,commit_seq,ordinal); CREATE INDEX idx_change_event_operation ON change_event(idempotency_key,commit_seq,ordinal);"#, |
There was a problem hiding this comment.
When a committed change_event is updated or deleted, the corresponding immutable commit_log row and possibly its outbox entry remain, but full-log replay now observes altered or missing canonical changes. Since Phase-0 retains this authoritative event history indefinitely, add update/delete guards like those on commit_log rather than allowing committed event payloads and identities to be rewritten.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 9c25fd8, with one deliberate narrowing.
Deletion is guarded outright, and so is identity — but the guard is BEFORE UPDATE OF commit_seq, ordinal, object_id, change_kind, idempotency_key rather than a blanket BEFORE UPDATE, because payload has to stay writable:
R11 and KTD9 both list change_event among the durable text fields the detector scans ("canonical rows, change_event, outbox payloads, receipts"), and R8 grants an operator-authorized remediation overwrite of a single named field to remove a detector-missed secret. A blanket update guard would make that unreachable and leave a leaked secret uncorrectable short of discarding the format epoch.
So: identity columns and deletion are refused, payload and source_span_id stay open for remediation and are covered by U5's authority-aware append-only guards. commit_history_identity_is_immutable_and_undeletable asserts both halves, including that a payload rewrite still succeeds.
commit_log got the blanket guard you describe in fb248d7, because none of its columns carry scanned content.
| ), | ||
| ( | ||
| "operation_receipts", | ||
| r#"CREATE TABLE operation_receipts(receipt_id TEXT PRIMARY KEY,producer TEXT NOT NULL,operation_key TEXT NOT NULL,request_digest TEXT NOT NULL,commit_seq INTEGER REFERENCES commit_log(commit_seq) ON DELETE RESTRICT,result_payload BLOB NOT NULL,created_at INTEGER NOT NULL,UNIQUE(producer,operation_key)) STRICT; CREATE INDEX idx_receipts_commit_fk ON operation_receipts(commit_seq);"#, |
There was a problem hiding this comment.
When an operation_receipts row is deleted or its operation key, digest, or result is updated, a retry can be treated as unseen and execute the same mutation again, defeating the duplicate-processing idempotency contract. Receipts are required to survive for the database incarnation, so prevent deletion and make their identity and recorded result immutable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 9c25fd8, split the same way as change_event.
Deletion is refused outright — U4 makes receipts canonical control facts that survive pruning, and your re-execution scenario is exactly why. Identity is guarded via BEFORE UPDATE OF receipt_id, producer, operation_key, request_digest, commit_seq, so the dedup key and the request it answers cannot drift.
One narrowing from your "and recorded result" ask: result_payload stays writable. R11 and KTD9 include receipts in the detector's durable-text-field set, so R8's remediation overwrite has to be able to reach a secret that landed in a stored result. Guarding it would trade a real leak-remediation path for a defense against a bug the identity guard already blocks — a retry matches on (producer, operation_key) plus request_digest, all now immutable, so a tampered result cannot make a retry look unseen.
commit_history_identity_is_immutable_and_undeletable covers it.
| ), | ||
| ( | ||
| "scope_term", | ||
| r#"CREATE TABLE scope_term(scope_id TEXT NOT NULL REFERENCES scopes(scope_id) ON DELETE RESTRICT,ordinal INTEGER NOT NULL,dimension TEXT NOT NULL,operator TEXT NOT NULL,exact_value TEXT,set_values BLOB,range_start TEXT,range_end TEXT,version_range TEXT,git_oid TEXT,git_start_oid TEXT,git_end_oid TEXT,payload BLOB,PRIMARY KEY(scope_id,ordinal)) STRICT;"#, |
There was a problem hiding this comment.
When a scope_term is updated or deleted after its parent scope has been committed, every known_as_of snapshot—including snapshots before that edit—uses the changed term because the child has no validity interval or commit ordering of its own. Treat terms as immutable content of a scope and express corrections by invalidating the parent and appending a successor scope.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed as a real gap; deferring to U5's append-only guards rather than adding a trigger here.
Your diagnosis is right and the mechanism is worth stating precisely: scope_term has no validity interval of its own, so it inherits the parent scope's, and an in-place edit therefore applies to snapshots that predate it. The correction shape you describe — invalidate the parent scope and append a successor with new terms — is what KTD6 already implies.
Not fixed in the schema because the same remediation constraint applies as on propositions: exact_value, range_start, range_end, and payload are durable text fields, so R11's detector covers them and R8's operator-authorized overwrite has to be able to reach them. A blanket trigger would block that, and a column-scoped one would guard only the structural columns while leaving the value columns — the ones that actually carry the applicability semantics you are protecting — open. That is not a meaningful guard, so it belongs with the authority-aware enforcement in U5 instead of as a partial trigger U5 would have to rework.
Deletion is already narrowed: scope_term.scope_id moved from ON DELETE CASCADE to RESTRICT in 5ec00e5, so the parent scope can no longer be deleted out from under its terms.
| ), | ||
| ( | ||
| "admission_decisions", | ||
| r#"CREATE TABLE admission_decisions(admission_decision_id TEXT PRIMARY KEY,candidate_id TEXT REFERENCES candidates(candidate_id) ON DELETE SET NULL,subject_object_id TEXT REFERENCES object_registry(object_id),source_kind TEXT NOT NULL,source_id TEXT NOT NULL,source_revision INTEGER NOT NULL,source_class TEXT NOT NULL,taint_class TEXT NOT NULL,maturity TEXT NOT NULL,disposition TEXT NOT NULL,visibility TEXT NOT NULL,policy_revision INTEGER NOT NULL,reason TEXT NOT NULL,evidence_id TEXT REFERENCES evidence_meta(evidence_id) ON DELETE RESTRICT,approval_object_id TEXT REFERENCES object_registry(object_id) ON DELETE RESTRICT,commit_seq INTEGER REFERENCES commit_log(commit_seq) ON DELETE RESTRICT,decided_at INTEGER NOT NULL) STRICT; CREATE INDEX idx_admission_candidate_fk ON admission_decisions(candidate_id); CREATE INDEX idx_admission_subject_fk ON admission_decisions(subject_object_id); CREATE INDEX idx_admission_evidence_fk ON admission_decisions(evidence_id); CREATE INDEX idx_admission_approval_fk ON admission_decisions(approval_object_id); CREATE INDEX idx_admission_commit_fk ON admission_decisions(commit_seq,admission_decision_id); CREATE INDEX idx_admission_source ON admission_decisions(source_kind,source_id,source_revision,decided_at);"#, |
There was a problem hiding this comment.
When an admission decision that produced canonical state is updated or deleted, the canonical object and commit remain while the policy revision, disposition, visibility, evidence, and reason that authorized it can disappear or change in place. Because such decisions are the retained staging audit after candidates expire, reject deletion and make their recorded decision fields immutable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed as a real gap; deferring to U5 with the other append-only guards.
The retained-audit argument is the strongest part of this: R10 has the admission decision outlive the candidate it judged, so it is the only surviving record of why an object was admitted, and policy_revision, disposition, visibility, and reason are exactly the fields an auditor needs.
Same reason it is not a trigger here as on propositions and scope_term: reason is free text on a row whose provenance came from staging, so it falls under R11's detector coverage and R8's remediation exception. Guarding only the non-text columns would leave reason — one of the fields you specifically name — mutable, which is not the guarantee you are asking for.
What did land: admission_decisions.commit_seq in 902aa20, so a decision that produced canonical state is now ordered against the commit that produced it rather than only against wall-clock decided_at, plus evidence_id and approval_object_id moving from ON DELETE SET NULL to RESTRICT so the authorizing evidence and approval cannot be unlinked.
| ), | ||
| ( | ||
| "capture_pins", | ||
| r#"CREATE TABLE capture_pins(capture_pin_id TEXT PRIMARY KEY,pin_kind TEXT NOT NULL,owner_id TEXT NOT NULL,commit_seq INTEGER NOT NULL REFERENCES commit_log(commit_seq) ON DELETE RESTRICT,lease_epoch INTEGER NOT NULL,writer_epoch INTEGER NOT NULL,created_at INTEGER NOT NULL,expires_at INTEGER,released_at INTEGER) STRICT; CREATE INDEX idx_capture_pins_commit_fk ON capture_pins(commit_seq); CREATE INDEX idx_capture_pins_ttl ON capture_pins(released_at,expires_at,capture_pin_id);"#, |
There was a problem hiding this comment.
When an active capture_pins row is deleted, ON DELETE CASCADE silently removes all of its evidence references, after which concurrent deletion or GC can reclaim an artifact that an in-progress backup still needs. Require the pin to be released before deletion, or otherwise guard active pins, so capture cannot produce a partial backup after successfully pinning its objects.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 9c25fd8. Took the "require the pin to be released before deletion" option, since released_at already exists and makes the condition local:
CREATE TRIGGER capture_pins_release_before_delete BEFORE DELETE ON capture_pins
WHEN OLD.released_at IS NULL
BEGIN SELECT RAISE(ABORT,'an active capture pin must be released before deletion'); END;The ON DELETE CASCADE to capture_pin_refs is deliberately kept: pins are operational rather than canonical, so once released they should be reclaimable along with their references. The trigger is what makes the cascade safe — it can now only fire on a pin whose holder has finished with it.
active_capture_pin_must_be_released_before_deletion asserts the active delete is refused and the released one succeeds.
| ), | ||
| ( | ||
| "anchors", | ||
| r#"CREATE TABLE anchors(anchor_id TEXT PRIMARY KEY,object_id TEXT NOT NULL UNIQUE REFERENCES object_registry(object_id),anchor_kind TEXT NOT NULL,exact_value TEXT,reachable_from_oid TEXT,reachable_between_start_oid TEXT,reachable_between_end_oid TEXT,deployment_revision TEXT,config_revision TEXT,platform_version_range TEXT,wall_clock_start INTEGER,wall_clock_end INTEGER,payload BLOB,created_commit_seq INTEGER NOT NULL REFERENCES commit_log(commit_seq),invalidated_commit_seq INTEGER REFERENCES commit_log(commit_seq),superseded_by TEXT REFERENCES object_registry(object_id),sensitivity_class TEXT NOT NULL,CHECK(invalidated_commit_seq IS NULL OR invalidated_commit_seq>created_commit_seq)) STRICT; CREATE INDEX idx_anchors_known_as_of ON anchors(created_commit_seq,invalidated_commit_seq,anchor_id);"#, |
There was a problem hiding this comment.
When an anchor is inserted with anchor_kind = 'Exact' but no exact_value, or with conflicting payload columns from several variants, this table accepts it because the discriminator has no corresponding shape constraint. The applicability engine then cannot interpret the canonical anchor as one of the declared variants; enforce the required and forbidden columns for each supported anchor kind, including ordered wall-clock bounds.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed as a gap, declining to fix it in the schema — the variant matrix is not settled anywhere yet, and pinning a guess into a frozen schema is worse than leaving it open.
anchor_kind is free-form TEXT and no document in the tree enumerates the anchor variants or their required and forbidden columns. The plan assigns applicability to the downstream engine (kh8.x), not to this unit, so a CHECK here would encode my inference of the variant set. If that inference is wrong — a missing variant, or one whose column set later grows — correcting it needs a format-epoch bump, which is a materially worse outcome than the interpretation failure you describe.
The ordered wall-clock bounds are the one piece that is variant-independent, and I have left them out only to keep the anchor constraint a single coherent change once the vocabulary exists. Worth noting for whoever picks that up: the check must be wall_clock_start/wall_clock_end null-tolerant, since an anchor with one open bound is meaningful.
This belongs with the anchor vocabulary work rather than the schema freeze.
| ), | ||
| ( | ||
| "alignment_projection", | ||
| r#"CREATE TABLE alignment_projection(decision_id TEXT NOT NULL REFERENCES decisions(decision_id) ON DELETE RESTRICT,observation_id TEXT NOT NULL REFERENCES observations(observation_id) ON DELETE RESTRICT,alignment_kind TEXT NOT NULL,alignment_payload BLOB,built_through_commit_seq INTEGER NOT NULL REFERENCES commit_log(commit_seq),PRIMARY KEY(decision_id,observation_id)) STRICT; CREATE INDEX idx_alignment_observation_fk ON alignment_projection(observation_id,decision_id); CREATE INDEX idx_alignment_built ON alignment_projection(built_through_commit_seq,decision_id);"#, |
There was a problem hiding this comment.
When a projection built through commit N finishes after one already stored through N+1, this primary-keyed row can be updated back to N and replace newer alignment state with stale results. Because built_through_commit_seq is the freshness boundary consumers can inspect, reject updates that decrease it so delayed rebuild work cannot roll the live projection backward.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed as a real risk; declining the trigger because it would not fire on the write path that causes it.
U4 step 4 specifies the projection is written by full replacement, not update:
Derived projection replace-primitive: atomic full replace of
alignment_projectionstamped withbuilt_through_commit_seq; consume nocommit_seq, emit no events
So a delayed rebuild does not UPDATE built_through_commit_seq downward — it deletes the projection and reinserts it with the stale value. A BEFORE UPDATE ... WHEN NEW < OLD guard never sees that, and its presence would be actively misleading: it would look like the regression was handled.
Catching it requires comparing the incoming build watermark against the stored one before the replace, which only the replace primitive can do, and it also needs the check to be per-replace rather than per-row — a whole-projection rebuild through N must be refused as a unit when N is behind, not row by row.
Tracked with the projection work. The observation that built_through_commit_seq is the freshness boundary consumers inspect is the right framing for that check.
Extend the pristine check to the file header. A header-only file carries a stamped application_id or user_version with no schema objects, so the inventory check alone let bootstrap overwrite a foreign or interrupted file's identity. change_event and operation_receipts had no write guards. A rewritten event identity leaves the immutable commit_log row intact while full-log replay observes different canonical changes, and a deleted or rewritten receipt lets a retry re-execute a mutation the idempotency contract says was already applied. Both guards are scoped with UPDATE OF rather than blanket BEFORE UPDATE. R11 and KTD9 list change_event payloads and receipt results among the durable text fields the detector scans, so R8's operator-authorized remediation overwrite has to stay reachable for exactly those columns. The identity columns carry no scanned content and are guarded outright, as is deletion. An active capture pin was deletable, cascading its evidence references away so GC could reclaim an artifact an in-progress backup had already pinned. Require release first.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c25fd88fc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ), | ||
| ( | ||
| "capture_pin_refs", | ||
| r#"CREATE TABLE capture_pin_refs(capture_pin_id TEXT NOT NULL REFERENCES capture_pins(capture_pin_id) ON DELETE CASCADE,evidence_id TEXT NOT NULL REFERENCES evidence_meta(evidence_id) ON DELETE RESTRICT,expires_at INTEGER,released_at INTEGER,PRIMARY KEY(capture_pin_id,evidence_id)) STRICT; CREATE INDEX idx_capture_pin_refs_evidence_fk ON capture_pin_refs(evidence_id,capture_pin_id);"#, |
There was a problem hiding this comment.
When an active backup has inserted an evidence reference, any connection can delete that capture_pin_refs row directly without touching its parent, allowing GC to reclaim the artifact while capture is still in progress. Fresh evidence after the prior fix is that capture_pins_release_before_delete guards only deletion of capture_pins; require unreleased reference rows to be undeletable and prevent re-parenting them until release.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in cabd3e4. You're right that the previous fix only covered the parent.
Both halves you asked for are in, keyed on the reference's own released_at:
CREATE TRIGGER capture_pin_refs_release_before_delete BEFORE DELETE ON capture_pin_refs
WHEN OLD.released_at IS NULL BEGIN SELECT RAISE(ABORT,'...'); END;
CREATE TRIGGER capture_pin_refs_no_reparent BEFORE UPDATE OF capture_pin_id,evidence_id
ON capture_pin_refs WHEN OLD.released_at IS NULL BEGIN SELECT RAISE(ABORT,'...'); END;Worth recording a mechanic I had to check empirically before choosing that shape, because it is the opposite of what I expected: a BEFORE DELETE trigger fires for rows removed by an FK cascade action, even with recursive_triggers off. I probed it against rusqlite rather than trusting my reading of the docs.
That means the guard cannot distinguish a direct delete from a cascade, so the design has to make reference release part of pin teardown: an unreleased reference now blocks the cascade, and the lifecycle is release each reference, release the pin, delete the pin. capture_references_survive_until_released pins that ordering explicitly, including the intermediate state where the pin is released but a reference is not.
The ON DELETE CASCADE stays, since it is now only reachable for references their holder has finished with.
| ), | ||
| ( | ||
| "consumer_abandonments", | ||
| r#"CREATE TABLE consumer_abandonments(abandonment_id TEXT PRIMARY KEY,consumer_id TEXT NOT NULL,operator_id TEXT NOT NULL,last_checkpoint_outbox_position INTEGER NOT NULL,reason TEXT NOT NULL,commit_seq INTEGER NOT NULL REFERENCES commit_log(commit_seq) ON DELETE RESTRICT,abandoned_at INTEGER NOT NULL) STRICT; CREATE INDEX idx_abandonments_consumer ON consumer_abandonments(consumer_id,abandoned_at); CREATE INDEX idx_abandonments_commit_fk ON consumer_abandonments(commit_seq);"#, |
There was a problem hiding this comment.
After a required consumer is deregistered through an authorized abandonment, this row can still be deleted or have its consumer, checkpoint, operator, or commit association rewritten in place. The immutable referenced commit_log row does not retain those structured audit fields, so later pruning audits and replay can no longer establish why the missing consumer was legitimately excluded; make the abandonment identity immutable and reject deletion.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in cabd3e4. Your point about the commit_log row is the load-bearing one: it records that a commit happened with an actor and cause, but not the consumer, checkpoint, or operator, so deleting the abandonment row destroys the only structured evidence that a missing consumer was legitimately excluded from the prune boundary.
BEFORE UPDATE OF abandonment_id, consumer_id, operator_id, last_checkpoint_outbox_position, commit_seq, abandoned_at plus a no-delete guard. reason stays writable — it is operator-supplied free text, so R11 scans it and R8's remediation overwrite has to reach it. Every field you named as audit-critical is in the guarded set.
audit_and_event_identity_are_immutable_and_undeletable covers it alongside the decision_events guard from your sibling comment.
| if stamped_application_id != 0 || stamped_user_version != 0 { | ||
| return Err(rusqlite::Error::InvalidQuery); | ||
| } | ||
| tx.pragma_update(None, "application_id", KERNEL_APPLICATION_ID)?; |
There was a problem hiding this comment.
A fresh kernel database is stamped with the shared MCTX application ID but leaves PRAGMA user_version at zero, even though the shared direct-format vocabulary defines DIRECT_FORMAT_EPOCH = 1 and its exact identity requires that header value. An opener implementing the promised pristine-or-exact check will therefore reject or quarantine every database produced here; set user_version to the direct-format epoch in the bootstrap transaction while retaining the kernel-specific marker table.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in cabd3e4. DIRECT_FORMAT_EPOCH exists on both sides (sqlite_runtime.rs:15 and storage-format-epoch.ts:43), the classifier compares inspection.userVersion !== expected.formatEpoch, and bootstrap was leaving the header at zero.
tx.pragma_update(None, "user_version", DIRECT_FORMAT_EPOCH)?;Stamped from the shared constant rather than a literal, and bootstrap_stamps_the_direct_format_epoch asserts the header epoch and the marker's format_epoch agree — they were two independently-written values with no test tying them together.
This also closes a loop with the pristine check from 9c25fd8: that check refuses a nonzero user_version, so now a second bootstrap over an existing kernel database is refused on the header alone, before the inventory check.
| ), | ||
| ( | ||
| "decision_events", | ||
| r#"CREATE TABLE decision_events(decision_id TEXT NOT NULL REFERENCES decisions(decision_id) ON DELETE RESTRICT,event_ordinal INTEGER NOT NULL,commit_seq INTEGER NOT NULL REFERENCES commit_log(commit_seq),event_kind TEXT NOT NULL,event_payload BLOB NOT NULL,evidence_id TEXT REFERENCES evidence_meta(evidence_id) ON DELETE RESTRICT,recorded_at INTEGER NOT NULL,PRIMARY KEY(decision_id,event_ordinal)) STRICT; CREATE INDEX idx_decision_events_commit ON decision_events(commit_seq,decision_id,event_ordinal); CREATE INDEX idx_decision_events_evidence_fk ON decision_events(evidence_id);"#, |
There was a problem hiding this comment.
After a decision event is committed, a direct UPDATE or DELETE on decision_events can change its commit ordering, kind, evidence association, or recorded identity—or remove it entirely—without allocating another commit_seq. Fresh evidence after the parent-cascade fix is that ON DELETE RESTRICT now protects events only when deleting their parent; direct child mutation still rewrites the canonical event stream used for known_as_of reads and alignment rebuilds, so guard event identity and deletion while preserving any explicitly authorized payload-remediation path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in cabd3e4, and your framing of what the earlier fix did and did not cover is exactly right: ON DELETE RESTRICT protects events from a parent delete, not from direct child mutation.
BEFORE UPDATE OF decision_id, event_ordinal, commit_seq, event_kind, evidence_id plus a no-delete guard. That covers all four things you listed — commit ordering, kind, evidence association, and recorded identity.
You explicitly asked to preserve the authorized payload-remediation path, which is what event_payload being left out of the guard does: R11 scans it, so R8's single-field overwrite stays reachable. audit_and_event_identity_are_immutable_and_undeletable asserts both directions, including that an event_payload rewrite still succeeds.
Bootstrap stamped the shared MCTX application id but left user_version at zero, while the shared vocabulary defines DIRECT_FORMAT_EPOCH and the pristine-or-exact check compares the header epoch. Stamp it from the same constant the marker records, so the header and the marker agree. Guarding capture_pins alone left its references exposed: any connection could delete an active capture_pin_refs row directly, or re-parent it, and GC could then reclaim an artifact an in-progress backup had pinned. Guard both, keyed on the reference's own released_at. FK cascades run the delete guard, so unreleased references must be released before pin teardown. Test that ordering. consumer_abandonments and decision_events had no write guards. The abandonment row is the only structured record of why a required consumer was legitimately excluded from the prune boundary, and the referenced commit_log row does not carry those fields. A rewritten decision event changes the canonical event stream that known_as_of reads and alignment rebuilds consume. Keep reason and event_payload writable for remediation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cabd3e41b7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ), | ||
| ( | ||
| "mc_kernel_format_marker", | ||
| r#"CREATE TABLE mc_kernel_format_marker(singleton INTEGER PRIMARY KEY CHECK(singleton=1),format_epoch INTEGER NOT NULL,database_incarnation_id TEXT NOT NULL,schema_digest TEXT NOT NULL,created_at INTEGER NOT NULL) STRICT; CREATE TRIGGER mc_kernel_format_marker_no_update BEFORE UPDATE ON mc_kernel_format_marker BEGIN SELECT RAISE(ABORT,'mc_kernel_format_marker is immutable'); END; CREATE TRIGGER mc_kernel_format_marker_no_delete BEFORE DELETE ON mc_kernel_format_marker BEGIN SELECT RAISE(ABORT,'mc_kernel_format_marker is immutable'); END;"#, |
There was a problem hiding this comment.
Even with the kernel-specific marker shape, database_incarnation_id still uses the shared direct-format identity vocabulary, which requires exactly 32 lowercase hexadecimal characters (storage-format-epoch.ts:58,67-78 and lib.rs:3834-3837). apply_kernel_schema currently accepts values such as the tests' "incarnation-1" and permanently stores them behind immutable marker triggers, so an exact opener validating that vocabulary must classify the freshly bootstrapped database as malformed; validate the argument before bootstrap and enforce the shape in the table.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 36413e4. The combination you point at is the real problem: a malformed value is stored and then sealed behind the immutable marker triggers, so it is unrecoverable short of discarding the format epoch.
Validated at both layers, since they catch different things:
fn is_well_formed_incarnation_id(incarnation: &str) -> bool {
incarnation.len() == 32
&& incarnation.chars().all(|c| c.is_ascii_hexdigit() && !c.is_ascii_uppercase())
}plus CHECK(length(database_incarnation_id)=32) on the column. The Rust check runs before the transaction opens and enforces the full lowercase-hex shape; the column check is the durable floor for any writer that bypasses this function. A CHECK cannot express "32 hex characters" usefully on its own, which is why the shape check lives in Rust.
While there, schema_digest got CHECK(length(schema_digest)=64) — the TS marker constrains its digest lengths and the kernel marker constrained neither.
Tests now bootstrap with a real 32-hex incarnation, and malformed_incarnation_ids_are_refused covers the wrong length, uppercase, non-hex, and empty cases, asserting each leaves no schema behind.
| ), | ||
| ( | ||
| "extraction_runs", | ||
| r#"CREATE TABLE extraction_runs(extraction_run_id TEXT PRIMARY KEY,extractor TEXT NOT NULL,source_kind TEXT,source_id TEXT,source_revision INTEGER,sensitivity_class TEXT NOT NULL,provenance_witness BLOB NOT NULL,redaction_metadata BLOB NOT NULL,detector_id TEXT,secret_type TEXT,utf8_offset INTEGER,utf8_length INTEGER,started_at INTEGER NOT NULL,heartbeat_at INTEGER NOT NULL,lease_expires_at INTEGER NOT NULL,terminal_state TEXT,terminal_at INTEGER,CHECK(lease_expires_at>heartbeat_at),CHECK((terminal_state IS NULL)=(terminal_at IS NULL))) STRICT; CREATE INDEX idx_runs_ttl ON extraction_runs(terminal_at,lease_expires_at,extraction_run_id); CREATE INDEX idx_runs_heartbeat ON extraction_runs(terminal_at,heartbeat_at,extraction_run_id);"#, |
There was a problem hiding this comment.
When a malformed write or replay supplies started_at = 100, heartbeat_at = 50, or a non-null terminal_at earlier than the heartbeat, this row satisfies the current lease and null-pair checks. The retention reaper can then treat newly terminal work as already past its 30-day deadline, while live-run logic observes a heartbeat predating creation; require started_at <= heartbeat_at and terminal timestamps at or after the last heartbeat, with the equivalent created_at ordering on candidates.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 36413e4. Added both orderings you asked for:
CHECK(heartbeat_at>=started_at) -- created_at on candidates
CHECK(terminal_at IS NULL OR terminal_at>=heartbeat_at)The retention consequence is the one that makes this worth a constraint rather than a convention: R10 starts the 30-day clock at completion, so a terminal_at predating the last heartbeat hands the reaper a deadline that has already passed and the row is eligible for deletion the moment it is written.
terminal_at = heartbeat_at is allowed — a run terminating on the same tick as its final heartbeat is ordinary, so the bound is inclusive. staging_timestamps_must_be_chronological covers both violations, the inclusive boundary, and the candidates equivalent.
| let journal_mode: String = | ||
| conn.pragma_update_and_check(None, "journal_mode", "WAL", |row| row.get(0))?; |
There was a problem hiding this comment.
When another process briefly holds the database lock during a cold open, PRAGMA journal_mode=WAL can require that lock, but this connection still has SQLite's default zero-length busy handler because busy_timeout_ms is not installed until line 215. The profile therefore returns SQLITE_BUSY immediately instead of honoring the requested timeout; the established ordering in compaction-marker.ts:177-181 explicitly sets busy_timeout first for this case, so move that pragma ahead of the journal-mode transition.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 36413e4. busy_timeout now precedes the journal-mode transition, so the ordering is: engine gate, busy timeout, WAL, then the remaining pragmas.
You're right about the mechanism — the WAL transition can need the write lock, and until busy_timeout is installed the connection carries SQLite's default zero-length busy handler, so a cold open racing another process failed immediately instead of honoring the caller's timeout. That made the busy_timeout_ms argument silently inapplicable to the one pragma most likely to contend.
This also sequences correctly against the engine gate added in 5ec00e5: gate first (no lock needed, in-memory probe), then the busy handler, then the first statement that writes the real file.
| ), | ||
| ( | ||
| "mc_kernel_format_marker", | ||
| r#"CREATE TABLE mc_kernel_format_marker(singleton INTEGER PRIMARY KEY CHECK(singleton=1),format_epoch INTEGER NOT NULL,database_incarnation_id TEXT NOT NULL,schema_digest TEXT NOT NULL,created_at INTEGER NOT NULL) STRICT; CREATE TRIGGER mc_kernel_format_marker_no_update BEFORE UPDATE ON mc_kernel_format_marker BEGIN SELECT RAISE(ABORT,'mc_kernel_format_marker is immutable'); END; CREATE TRIGGER mc_kernel_format_marker_no_delete BEFORE DELETE ON mc_kernel_format_marker BEGIN SELECT RAISE(ABORT,'mc_kernel_format_marker is immutable'); END;"#, |
There was a problem hiding this comment.
Fresh evidence after the update/delete guards were added is that kernel connections leave PRAGMA recursive_triggers at SQLite's default off, so INSERT OR REPLACE INTO mc_kernel_format_marker ... implicitly deletes the singleton without firing this delete trigger and inserts replacement identity data; the same bypass can rewrite unreferenced commit, receipt, and audit rows protected only by delete triggers. The repository already documents this exact SQLite behavior in storage-claims-schema.ts:12-16,119-123; enable and verify recursive triggers on every kernel connection or add BEFORE INSERT collision guards so replacement cannot bypass append-only history.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 36413e4. Best finding in the review — the guards I added were bypassable in one statement.
I verified both directions against rusqlite rather than trusting my reading, because my last docs assumption about trigger firing on this codebase turned out to be backwards:
recursive_triggers=false REPLACE=ALLOWED value=hijacked
recursive_triggers=true REPLACE=REFUSED value=original
So INSERT OR REPLACE INTO mc_kernel_format_marker really did silently replace the singleton, and the same held for any row whose only protection was a delete trigger.
Took the pragma option over BEFORE INSERT collision guards: it is one line, it covers every guarded table at once (marker, commit_log, change_event, operation_receipts, decision_events, consumer_abandonments, writer_fence, capture pins and refs) rather than needing a per-table guard, and it cannot be forgotten when a table is added later. recursive_triggers is also now verified in verify_kernel_connection_contract, so a connection that skipped the profile fails the contract rather than silently losing the guards.
No recursion risk from turning it on: every kernel trigger body is a bare RAISE(ABORT) and mutates nothing.
replace_cannot_bypass_the_append_only_guards covers the marker and commit_log paths and asserts the original values survive.
…amps Enable and verify recursive_triggers on every kernel connection. With it off, REPLACE conflict resolution deletes the conflicting row without running BEFORE DELETE, so INSERT OR REPLACE rewrote the immutable format marker and any commit, receipt, or audit row whose only guard was a delete trigger. Verified both directions against rusqlite before choosing the fix. Install busy_timeout before the journal_mode transition. Switching to WAL can require the write lock, and the connection still carried SQLite's default zero-length busy handler, so a cold open racing another process returned SQLITE_BUSY instead of waiting. apply_kernel_schema accepted any string as database_incarnation_id and stored it behind the immutable marker triggers. The shared vocabulary requires 32 lowercase hex characters, so an exact opener would classify every database bootstrapped here as malformed. Validate the argument and carry a length check on the column, alongside the digest length the marker was also missing. extraction_runs and candidates accepted a heartbeat preceding creation and a terminal timestamp preceding the last heartbeat, which lets the reaper treat newly terminal work as long past its retention deadline. Require both orderings.
1 participant
Summary
Review focus
Schema shape and invariants only. Opening, mutation APIs, and lifecycle behavior land later in the stack.
Stack
Part 2 of 8. Depends on #111; followed by #113.