feat(store): add atomic kernel commits - #115
Conversation
|
Warning Review limit reachedNext included review available in 6 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 107 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 selected for processing (9)
Comment |
Acquire the writer lease before database access, classify foreign files from the header, and quarantine only conclusive kernel mismatches. Exact opens verify the frozen schema before enabling WAL and stamping the writer fence.
Use one checked-in fixture to keep Rust detection metadata and TypeScript redacted output aligned. Compile the detector once and preserve scalar diagnostics that are not credentials.
Fence each mutation inside BEGIN IMMEDIATE, replay idempotent receipts without effects, and publish canonical changes with one commit sequence. Historical reads mask future corrections, while shared redaction runs before durable binds.
926eb21 to
68e045d
Compare
|
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: 68e045dbb9
ℹ️ 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".
| &replaced, | ||
| Some(self.commit_seq), | ||
| )?; | ||
| insert_domain(self.tx, self.commit_seq, &replacement)?; |
There was a problem hiding this comment.
When the replacement insert fails after the old object has already been invalidated—for example, because the replacement violates the current-name uniqueness constraint—the callback can catch this Err and still return Ok. The transaction then commits the invalidation and receipt without adding a change event or outbox row, breaking the atomic-envelope guarantee. Use a savepoint for each compound mutation or poison the envelope so commit cannot succeed after any mutation error.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in b2827b7.
Reproduced exactly as described: correct_domain invalidates the predecessor, then the replacement insert trips idx_domains_current_name, and a closure that discards the Err still returns Ok — so the transaction committed the invalidation plus a receipt with no change event or outbox row.
Took the poison option rather than per-mutation savepoints, since the failure is already atomic at the transaction level; what was missing was refusing to commit a change set that no longer described its writes. Envelope now records the first mutation error, every mutating method short-circuits once poisoned, and commit_inner returns that error after the closure instead of proceeding.
Pinned by a_swallowed_mutation_error_cannot_commit_a_partial_correction, which builds the name collision you describe, swallows the Err, and asserts both objects are still live and no third commit exists. I verified it fails if the poison check is removed.
| record( | ||
| self.tx, | ||
| "change_event", | ||
| &replaced.text, | ||
| "replaced_object_id", | ||
| &replaced, | ||
| Some(self.commit_seq), | ||
| )?; |
There was a problem hiding this comment.
When a corrected object's ID contains a detected secret, this records its metadata under the old object ID, while every other change_event redaction uses the computed commit_seq:ordinal event identity. Because replaced_object_id is not included in the later change.redactions, consumers looking up metadata for the emitted correction event cannot find this detection; defer the record until the event ID is available.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and resolved in b2827b7 (landed earlier in 4e714ea), though by removing the row rather than deferring it.
You were right that there were two incompatible owner_id namespaces under owner_kind='change_event': {commit_seq}:{ordinal} from the commit loop, and the raw object id from correct_domain/retire_domain. Rows in the second namespace could not be joined back to change_event(commit_seq,ordinal).
Deferring until the event id exists turned out to be unnecessary once replaced_object_id became a validated identity: it is rejected if the detector fires, so it can never carry a detection and there is no metadata to record. Both record() calls are gone and retire_domain pushes an empty redactions list. grep now finds exactly one change_event owner form in the file, the {commit_seq}:{ordinal} one.
If identity fields ever become redactable again, your deferral is the right shape and the constraint should come back with it.
| tx.execute("DELETE FROM alignment_projection", []) | ||
| .map_err(|_| KernelError::Io)?; |
There was a problem hiding this comment.
When both the old and replacement projection contain a detected secret in the same field, deleting only alignment_projection leaves the old durable_text_redactions row behind. row.record() then inserts the same (owner_kind, owner_id, field_name, detection_ordinal) key, producing KernelError::Io and rolling back every subsequent refresh for that decision/observation pair. Delete the projection-owned metadata in the same transaction before inserting the replacement rows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in b2827b7 (landed earlier in 4e714ea), exactly as suggested.
replace_alignment_projection now calls clear_owner_kind(&tx, "alignment_projection") in the same transaction as the DELETE FROM alignment_projection, before reinserting.
Worth noting the existing test could not have caught this: it replaced twice with secret-free payloads ("first"/"second"), so record() inserted nothing and the PK was never exercised. projection_replace_repeats_when_a_field_carries_a_detected_secret now puts a detected secret in the payload on both passes; verified it fails when the cleanup call is removed.
| producer: redact(&intent.producer), | ||
| operation_key: redact(&intent.operation_key), | ||
| request_digest: intent.request_digest, |
There was a problem hiding this comment.
When two distinct operation keys contain different secrets recognized by the redactor, both values can collapse to the same placeholder text. Because receipt lookup and operation_identity use this redacted value, the second request can incorrectly replay the first request when their digests match, or report a conflict when they differ. Keep a non-reversible digest of the original key for identity comparisons while storing only the redacted display value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, fixed in b2827b7 (landed earlier in 4e714ea) — but with a different remedy than you proposed, so flagging the deviation.
The diagnosis is right and it was the most serious problem in the PR. redact_secret_text maps every match of a rule to one constant replacement, so two distinct operation_keys containing different secrets collapsed; with matching digests the second request replayed the first's commit_seq and result while its own operation never ran.
You suggested keeping a non-reversible digest of the original key for identity while storing the redacted display value. I rejected the key instead: identity() returns the text unchanged when the detector finds nothing and InvalidInput when it fires. Reasoning — a digest keeps the identity injective but leaves a secret-derived value in the dedup path and forces every consumer to carry two representations of one key. Rejecting keeps the stored key both injective and secret-free, and a key that contains an API token is a caller bug worth surfacing rather than silently accommodating.
Applied to producer, operation_key, object_id, domain_id, candidate_id, extraction_run_id, decision_id, observation_id, and both lookup arguments. Free text stays redacted.
One deliberate asymmetry: DomainSpec.source_kind/source_id stay redacted even though they sit in object_registry's UNIQUE constraint. Equality used to accept (run reuse, receipt replay) must be injective or it matches wrongly; equality used to reject (a uniqueness constraint) over-rejects under collapse, which is fail-closed. Your later comment on the run-reuse tuple is the accept case, and I fixed that one by rejecting — see the reply there.
Pinned by a_lookup_key_carrying_a_detected_secret_is_rejected_not_redacted; verified it fails when identity() is reverted to plain redaction.
| "UPDATE object_registry SET invalidated_commit_seq=?1 | ||
| WHERE object_id=?2 AND invalidated_commit_seq IS NULL", | ||
| params![self.commit_seq, replaced.text], | ||
| ) |
There was a problem hiding this comment.
When the supplied ID belongs to an active non-domain object, this update still invalidates its object_registry row because it does not check object_kind; the following domains update affects zero rows but is not checked, and the transaction can then commit a domain replacement that supersedes the unrelated object. Require a matching active domain row—or at least object_kind='domain' plus a checked domain update—before mutating the registry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in b2827b7 (landed earlier in 4e714ea).
Verified with a probe: retire_domain("decision-object") returned Ok, invalidated the registry row, emitted a retire event with object_kind=decision, and left the decisions row live.
correct_domain and retire_domain now share one invalidate_domain helper that loads the row, rejects object_kind != 'domain' with InvalidInput, and requires exactly one affected row from both the object_registry and domains updates. set_domain_successor does the same for the superseded_by pair.
Pinned by retire_and_correct_refuse_a_non_domain_object; verified it fails when the kind guard is removed.
| let sensitivity = if spec.provenance.is_some() { | ||
| Sensitivity::Normal | ||
| } else { | ||
| Sensitivity::Sensitive | ||
| }; |
There was a problem hiding this comment.
When a candidate has repository provenance but its payload contains a vocabulary-detected secret, this branch assigns Normal solely because provenance is present, ignoring spec.payload.detections. The stored row therefore loses the required secret classification and downstream egress gates that rely on the sensitivity column can treat secret-derived material as remotely eligible. Add a Secret class and make any detected secret override provenance-based normal classification.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Partially fixed in b2827b7 (escalation landed in 4e714ea); declining the third class.
The mechanism you identified is real and is fixed: classification no longer keys off provenance.is_some() alone, so a detected secret in a candidate's payload now escalates it regardless of provenance.
I did not add a distinct Secret class. Sensitivity is a two-value vocabulary persisted in sensitivity_class across roughly fifteen tables and republished on outbox, and from_stored already folds legacy internal/secret spellings into Sensitive. Introducing a third value changes the stored vocabulary and the outbox contract for every consumer, which is a schema and protocol decision rather than an envelope one. Escalating to the existing terminal class closes the egress hole you describe; splitting Sensitive into two levels can follow with the consumer side designed alongside it.
Note your follow-up comment caught a regression in my first attempt at this — see the reply on the run-vs-candidate sensitivity thread.
| if spec.source_revision < 0 | ||
| || spec.recorded_at < 0 | ||
| || spec.lease_expires_at < spec.recorded_at | ||
| { |
There was a problem hiding this comment.
A caller can set lease_expires_at arbitrarily far beyond recorded_at because validation only checks their ordering. Such a run remains active beyond the one-hour staging lease bound and will not be abandoned by a time-based reaper when required, allowing stale candidates to remain live indefinitely. Reject durations over one hour using checked arithmetic before persisting the run.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining — the premise does not hold in this repository, so please push back if I have missed something.
The claim rests on "the one-hour staging lease bound". I searched for such a bound and there is no one-hour staging-lease constant, config value, or documented invariant anywhere in crates/ or docs/. The only 3600-ish constants are unrelated (MAX_RETRY_AFTER_SECS and MAX_DEADLINE_MS in mc-host). So there is no reaper contract here for an unbounded lease to violate, and picking one hour would be inventing a policy at the wrong layer — the store cannot know the caller's extraction duration.
The narrower version of your point is real and is fixed in b2827b7 for a different reason: the reuse path now rejects a run whose lease_expires_at has already passed recorded_at, so an expired lease is not silently extended. That closes the resurrection path without asserting a duration cap.
If a staging lease bound is meant to exist, it should land as a named constant with the reaper that enforces it, and I would rather do that in the change that introduces the reaper.
| let mut statement = tx | ||
| .prepare( | ||
| "SELECT object_id,object_kind,domain_id,source_kind,source_id,source_revision, | ||
| created_commit_seq,sensitivity_class | ||
| FROM object_registry | ||
| WHERE created_commit_seq<=?1 | ||
| AND (invalidated_commit_seq IS NULL OR ?1<invalidated_commit_seq) | ||
| ORDER BY object_id", |
There was a problem hiding this comment.
For any snapshot containing a domain, this query returns only its object_registry fields and omits canonical domain data such as domains.name. Consequently, commits that differ in domain content can produce indistinguishable known_as_of results, and callers cannot atomically reconstruct the canonical state at the requested sequence through any exposed transaction. Include the typed canonical rows in the same read transaction rather than exposing only registry metadata.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real gap, deliberately not fixed here. Tracking it as follow-up rather than folding it into this PR.
The observation is correct: known_as_of projects object_registry only, so two commits differing solely in domains.name yield indistinguishable snapshots, and there is no exposed transaction that reconstructs canonical state at a sequence.
Not doing it now because it is an API change rather than a bug fix. KnownAsOf.objects is Vec<ObjectRow>, a registry-shaped type; returning typed canonical rows means either a per-kind sum type or a per-kind accessor, and the answer depends on the kinds still to land in this stack (entities, propositions, decisions, observations all have canonical tables already in the schema but no envelope methods yet). Designing that surface around domain alone would very likely be the wrong shape, and it would also rewrite the reference-model test that currently gives this PR its history oracle.
What this PR does guarantee is that the registry projection is internally consistent: object_history_as_of masks invalidation metadata past the requested sequence, and the paired-update fix means the registry can no longer disagree with domains about liveness. The canonical-content read is the missing piece and wants its own change once more object kinds exist.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (9 files)
Previous Review Summaries (4 snapshots, latest commit e219f31)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit e219f31)Status: No Issues Found | Recommendation: Merge Files Reviewed (9 files)
Previous review (commit b2827b7)Status: No Issues Found | Recommendation: Merge Files Reviewed (10 files)
Previous review (commit 029ff79)Status: No Issues Found | Recommendation: Merge Files Reviewed (10 files)
Previous review (commit 68e045d)Status: No Issues Found | Recommendation: Merge Files Reviewed (8 files)
Reviewed by gemini-3.7-flash · Input: 239.6K · Output: 26K · Cached: 1.6M |
Redaction collapses every match of a rule to one constant replacement, so a redacted identity could alias another caller's key. Identity and lookup fields now reject a detected secret; free text stays redacted. Domain invalidation now guards on object_kind and requires one affected row in both object_registry and domains. Projection rebuild clears its durable_text_redactions rows in the same transaction and refuses an empty batch. Constraint violations map to Conflict and lock waits to a retryable Busy. The format epoch covers the new schema shape.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e714ea378
ℹ️ 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".
| Some(spec.source_kind.text.clone()), | ||
| Some(spec.source_id.text.clone()), | ||
| Some(spec.source_revision), | ||
| sensitivity.as_str().to_string(), |
There was a problem hiding this comment.
When an existing extraction run was created by a normal candidate and a later candidate in that run contains a detected secret (or vice versa), RedactedCandidate::sensitivity() changes because it scans the current candidate's payload fields, and this tuple treats that per-candidate value as immutable run metadata. stage_candidate therefore returns Conflict before inserting the later candidate, even though one run can naturally produce candidates with different classifications and the schema stores sensitivity on each candidate. Derive the compared run sensitivity from run fields, then classify each candidate from its own fields plus any inherited run restriction.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in b2827b7. Good catch — this was a regression I introduced in 4e714ea while addressing your P1 classification comment, not a pre-existing issue.
Reproduced: a clean candidate stored the run as normal, then a secret-bearing candidate in the same run recomputed sensitivity() as Sensitive, failed the identity tuple, and returned Conflict before inserting.
Fixed the way you describe. run_sensitivity() derives from run-level facts only (provenance presence) and is what the reuse tuple compares and what extraction_runs stores. candidate_sensitivity() starts from the run class and escalates on detections in that candidate's own fields, and is what the candidates row and StagingCandidateRow carry. The schema already had separate sensitivity_class columns on both tables, so this needed no migration.
Pinned by one_run_accepts_candidates_with_different_classifications, which stages a clean then a secret candidate in one run and asserts the run stays normal while the candidates are normal and sensitive.
| tx.execute( | ||
| "UPDATE extraction_runs | ||
| SET heartbeat_at=MAX(heartbeat_at,?1),lease_expires_at=MAX(lease_expires_at,?2) | ||
| WHERE extraction_run_id=?3", |
There was a problem hiding this comment.
When an extraction run has already been marked completed, failed, canceled, or abandoned, this reuse path does not load or check terminal_state; it renews the run's heartbeat and lease and then inserts another candidate under the still-terminal run. That candidate can subsequently be cascade-deleted according to the run's older terminal_at, or processed despite belonging to a closed extraction. Include the terminal state in the lookup and reject attempts to append to terminal runs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in b2827b7.
The reuse path selected neither terminal_state nor lease_expires_at, so it renewed the heartbeat and appended a candidate to a closed run. The lookup now reads both and returns Conflict when terminal_state IS NOT NULL.
Pinned by a_terminal_or_expired_run_refuses_further_candidates, which marks the run completed out of band and asserts the next stage_candidate is refused.
|
Ran four independent review passes over this diff (over-engineering, SQLite/storage, Rust correctness, and a design/safety/invariant/test-adequacy pass) and addressed the findings in 4e714ea. FixedOperation identity was derived from lossy redaction. Domain invalidation could leave the registry and the typed table disagreeing. Projection rebuild wedged itself permanently. Every rusqlite error collapsed to
Pragmas were unverifiable. Two test oracles could not fail. Also: format epoch bumped for the new schema shape (no migration path exists — an earlier epoch is quarantined and rebuilt, which is now stated at the constant); redaction span columns renamed to Deletions the reviewers converged on: VerificationSix new regression tests, each confirmed to fail against the pre-fix code by reverting the corresponding fix:
Plus
Not fixed here, deliberately
Two pre-existing CI failures, both from ancestors of this branchNeither is caused by this PR, and I have not touched either:
GitHub has not dispatched a CI run for 4e714ea yet (other branches are firing, so it looks like a dispatch hiccup rather than a config issue). Worth a re-run once it picks up. |
Reword the kernel doc comments so each passes the comment gate on its own, and drop the three that no wording satisfied. The contracts those three carried are pinned by tests instead: the epoch quarantine by every_conclusive_kernel_mismatch_is_quarantined_and_rebuilt, and the paired domain updates by retire_and_correct_refuse_a_non_domain_object.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 029ff790d1
ℹ️ 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".
| tx.execute( | ||
| "UPDATE extraction_runs | ||
| SET heartbeat_at=MAX(heartbeat_at,?1),lease_expires_at=MAX(lease_expires_at,?2) | ||
| WHERE extraction_run_id=?3", |
There was a problem hiding this comment.
When a candidate arrives after the existing run's lease_expires_at but before the reaper marks it abandoned, this update accepts the request and extends the lease with MAX, effectively resurrecting an expired run. Load the existing lease and reject or abandon the run when lease_expires_at < spec.recorded_at before inserting another candidate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in b2827b7, in the same lookup as the terminal-state check.
The reuse path now rejects with Conflict when lease_expires_at < spec.recorded_at, so an expired run is not resurrected by the MAX renewal.
I return Conflict rather than abandoning the run in place: marking it abandoned is a reaper decision with its own terminal-state bookkeeping, and doing it opportunistically inside a staging call would let one caller's timing silently close another's run. The same test covers this — the late candidate arrives with recorded_at past the stored lease and is refused.
| let expected = ( | ||
| spec.extractor.text.clone(), | ||
| Some(spec.source_kind.text.clone()), | ||
| Some(spec.source_id.text.clone()), | ||
| Some(spec.source_revision), | ||
| sensitivity.as_str().to_string(), | ||
| provenance.clone(), |
There was a problem hiding this comment.
When a reused run supplies a different secret-bearing source_id, extractor, or provenance value that matches the same detector, both inputs collapse to the same placeholder before this tuple comparison. The equality then passes and associates the new candidate with the wrong immutable source instead of returning Conflict; reject secrets in these identity fields or compare a non-reversible digest of their original values.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in b2827b7.
Verified with a probe before changing anything: two candidates whose source_id differed only inside a detected secret both landed in the same run, the second silently associated with the wrong immutable source.
This is the accept-side case of the identity problem, so it gets the same treatment: extractor, source_kind, source_id, and both provenance fields are now identities and reject a detected secret, which makes the reuse tuple injective. Rejecting rather than digesting for the reasons in my reply on the operation-key comment.
Pinned by run_identity_fields_reject_a_detected_secret.
| pub fn is_retryable(self) -> bool { | ||
| matches!(self, Self::Busy) | ||
| } |
There was a problem hiding this comment.
When KernelStore::open returns KernelError::Held because another writer currently owns the lease, retrying the unchanged open request after that lease is released is valid, yet this method reports it as permanent. Callers that use is_retryable() for startup retry policy will therefore fail immediately during transient writer overlap; include Held alongside Busy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in b2827b7. is_retryable() now matches Busy | Held.
You are right that the distinction I drew was wrong: a lease held by another writer is released on drop, so retrying an unchanged open is valid in exactly the sense Busy is.
Comparing a per-candidate sensitivity against stored run metadata made a run reject its own later candidates: a clean candidate stored `normal`, and a secret-bearing candidate in the same run then failed the identity tuple. Run classification now derives from run-level facts and each candidate carries its own class. The reuse tuple also compared redacted `extractor`, `source_kind`, `source_id`, and provenance, so two different secret-bearing sources collapsed to one value and equality passed. Those fields are identities and reject a detected secret. A reused run now also rejects a terminal state or an expired lease instead of renewing it. An `Envelope` mutation can fail after writing some rows, and a closure may discard that error. The envelope records the failure and `commit` refuses, so a partial correction cannot commit its invalidation without the matching change event. Lease contention is retryable, so `is_retryable` covers `Held`.
|
All 13 inline review comments now have replies. Second round of fixes in b2827b7. Fixed this roundA regression I introduced. The run/candidate sensitivity comment caught a real break in my own 4e714ea: making any detection escalate the class meant The accept-side identity case. The run-reuse tuple still compared redacted This settles an asymmetry worth stating: equality used to accept (run reuse, receipt replay) must be injective or it matches the wrong thing; equality used to reject (a uniqueness constraint) over-rejects under collapse, which is fail-closed. That is why the run-reuse fields reject secrets while Partial commits could survive a swallowed error. Terminal and expired run reuse, both now
Declined, with reasoning
Also worth noting on method: the digest-based remedy suggested for operation identity would keep the key injective but leaves a secret-derived value in the dedup path and forces two representations of one key. I rejected secret-bearing keys instead. Verification165 No One cautionKilo reported "No Issues Found | Recommendation: Merge" on 68e045d, the commit where Codex independently found 13 issues including two P1s and the identity collapse that let one caller supersede another caller's object. Worth not treating that signal as coverage. Still open: GitHub has not dispatched a CI run for this branch across three pushes, and the two pre-existing ancestor failures (the Biome import-order error from c9aa10a in the parent PR, and the macOS-only |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2827b7af9
ℹ️ 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".
| source_kind: redact(&spec.source_kind), | ||
| source_id: redact(&spec.source_id), |
There was a problem hiding this comment.
When two domains use distinct secret-bearing source IDs that match the same detector—for example, two credentialed URLs with the same source kind and revision—both IDs collapse to the same placeholder here. Because object_registry enforces UNIQUE(source_kind,source_id,source_revision,object_kind), the second distinct source then fails with Conflict. Treat these fields as identities by rejecting detected secrets or storing a non-reversible digest for uniqueness.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and already fixed in e219f31 before I saw this thread — a later comment on domains.name raised the same class of problem and I applied the fix to the whole DomainSpec.
source_kind and source_id are now identities and reject a detected secret, exactly as you propose in your first option, so object_registry's UNIQUE(source_kind,source_id,source_revision,object_kind) can no longer refuse a legitimately distinct source. Your credentialed-URL example is the case that convinced me the over-rejection I had defended earlier was not harmless.
Apologies for the delay replying: my comment poller seeded this batch as already-seen when I started it, so these six never reached my triage queue. That was my setup error, not a judgement about the findings.
| ), | ||
| ( | ||
| "durable_text_redactions", | ||
| r#"CREATE TABLE durable_text_redactions(owner_kind TEXT NOT NULL,owner_id TEXT NOT NULL,field_name TEXT NOT NULL,detection_ordinal INTEGER NOT NULL,detector_id TEXT NOT NULL,secret_type TEXT NOT NULL,source_utf8_offset INTEGER NOT NULL,source_utf8_length INTEGER NOT NULL,commit_seq INTEGER REFERENCES commit_log(commit_seq) ON DELETE RESTRICT,PRIMARY KEY(owner_kind,owner_id,field_name,detection_ordinal)) STRICT; CREATE INDEX idx_text_redactions_commit_fk ON durable_text_redactions(commit_seq);"#, |
There was a problem hiding this comment.
When a secret-bearing candidate is deleted directly or through its extraction run's cascade, its staging_candidate rows in this polymorphic table have no foreign key or cleanup path and remain orphaned. A later candidate reusing that now-available ID then fails in RedactedCandidate::record on this primary key, while an equivalent candidate without detections can reuse the ID successfully. Delete these owner rows in the candidate cleanup transaction or give them lifecycle linkage.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and partly fixed in 77b816f; the rest is genuinely outside this PR and I want to be clear about the split.
Fixed here: RedactedCandidate::record now clears any rows under ('staging_candidate', candidate_id) before inserting. That closes the asymmetry you identified, where a reused id succeeded for a detection-free candidate but collided for a secret-bearing one.
Not fixed here: the orphaned rows themselves. durable_text_redactions.commit_seq is nullable and staging rows pass None, so there is no lifecycle linkage to a candidate or run, and nothing in this PR deletes candidates — the reaper that performs the cascade does not exist yet. Adding a foreign key to candidates would also be wrong for a polymorphic owner table that already serves commit_log, outbox, and change_event owners.
So the right fix is for the cleanup transaction to delete its own redaction rows, and that belongs with the reaper. clear_owner and clear_owner_kind are in place for it to call.
Pinned by a_reused_candidate_id_does_not_collide_with_deleted_redaction_rows, which deletes a candidate out of band to simulate the cascade and then re-stages the same id.
| object, | ||
| kind: "retire", | ||
| replaced_object_id: None, | ||
| redactions: Vec::new(), |
There was a problem hiding this comment.
When the retired domain originally had detections in source_kind or source_id, its ObjectRow republishes the stored placeholders in the retirement payload and outbox columns, but this explicit empty list creates no metadata under the new change-event or outbox owner IDs. Consumers therefore cannot audit those redacted occurrences. Fresh evidence beyond the corrected replaced-ID case is that this retirement branch republishes source fields while unconditionally discarding their redaction metadata; load and carry forward the original object's detections.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved in e219f31, though by removing the metadata rather than carrying it.
Your diagnosis was right for the code as it stood. It is now moot for a different reason: source_kind and source_id became identities that reject a detected secret, so a retired domain has no source detections to carry forward. DomainSpec has no free-text field left at all, which is why the empty redactions list you flagged is now correct rather than lossy.
If a future object kind does republish redacted free text in a retirement payload, your point stands and the metadata will need loading from the original object. Worth keeping in mind when the next kind lands.
| .transaction_with_behavior(TransactionBehavior::Immediate) | ||
| .map_err(map_sqlite)?; | ||
| check_fence(&tx, self.lease_epoch())?; | ||
| tx.execute("DELETE FROM alignment_projection", []) |
There was a problem hiding this comment.
When two projection builds finish out of order, an older build can enter this transaction after a newer one and unconditionally delete the newer projection before inserting rows with a lower built_through_commit_seq. The writer mutex serializes the replacements but does not preserve the snapshots' generation order, so a slow stale build can regress the derived state. Compare the incoming generation with the stored projection generation, require one consistent generation per batch, and reject stale replacements before deleting rows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 77b816f. This was the sharpest of the six — the mutex genuinely serializes without ordering, which is easy to mistake for safety.
Implemented both parts you asked for. A rebuild now requires one built_through_commit_seq across the batch, and rejects a generation older than MAX(built_through_commit_seq) already stored, before the delete. Re-publishing the same generation stays allowed, since a retry of the current build is not a regression.
Pinned by a_stale_projection_rebuild_cannot_regress_a_newer_one, which publishes generation 2, refuses generation 1 with Conflict, asserts the generation-2 rows survived, allows a same-generation republish, and refuses a mixed-generation batch with InvalidInput.
| } | ||
| let candidate_metadata = spec.candidate_detection_json()?; | ||
| tx.execute( | ||
| "INSERT INTO candidates( |
There was a problem hiding this comment.
When a successful staging response is lost and the producer retries the identical StagingCandidateSpec, the existing run passes the identity checks but this unconditional insert hits the candidate primary key and returns Conflict. The unchanged request therefore cannot be retried safely and may be reported as failed even though its candidate is already durable. Load and compare an existing candidate by ID, returning the stored row for an exact match and reserving Conflict for mismatched content.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 77b816f. This is the narrow, well-specified version of the idempotency gap I had deferred as too large, and framed this way it was small.
stage_candidate now loads any existing candidate by id and compares extraction_run_id, candidate_kind, payload, sensitivity, and provenance. An exact match commits and returns the stored row, so a producer that lost its response can retry unchanged. Conflict is reserved for changed content, as you suggested.
Pinned by an_identical_restage_replays_instead_of_conflicting, which asserts the retry returns the same row with no second candidate, and that changed content still conflicts.
| ordinal, | ||
| change.object.object_id, | ||
| change.kind, | ||
| intent.operation_key, |
There was a problem hiding this comment.
When two producers use the same operation key, both commits are valid because receipt identity is (producer, operation_key), but their change events store the same value here. Consumers or diagnostics using the indexed idempotency_key cannot distinguish the operations and may deduplicate one producer's event as a replay of the other. Store the composite operation identity or the already computed transaction_id instead of the bare operation key.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 77b816f.
change_event.idempotency_key held the bare operation_key while receipt identity is (producer, operation_key), so two producers legitimately sharing a key wrote indistinguishable events under an indexed column, and a consumer could dedupe one producer's event as the other's replay. The column now stores transaction_id, which already hashes producer, operation key, and digest with length prefixes — your second suggestion, and it needed no new computation.
Pinned by change_event_identity_distinguishes_two_producers_sharing_an_operation_key.
The parent branch was rebased onto main and its redaction commit rewritten, so this branch carried a stale copy and conflicted. The parent owns the shared schema and connection profile, so schema.rs, open.rs, and both test files take its versions and re-apply only the envelope's own additions: the commit_log operation-identity columns with their unique index, the durable_text_redactions relation, the typed writer_fence sentinel, the envelope-layer KernelError variants, and the reader query_only read-back. The parent independently added the journal_mode read-back and a stricter connection contract, so the equivalents added here are dropped in favour of its versions. Its BLOB payload columns are also restored: the envelope now binds result_payload, candidates.payload, and alignment_payload as bytes rather than redeclaring those columns TEXT. Adding columns changes the schema digest, so PINNED_SCHEMA_DIGEST moves to the merged shape and existing databases are quarantined and rebuilt.
Auto-merge duplicated the import and the whole describe block in the plugin redaction test, which broke the plugin lint gate. That file has no changes from this branch, so it returns to the base version. Taking the parent's kernel_open.rs and kernel_schema.rs also dropped the tests added here. The two schema tests return because they pin this branch's own changes: the commit_log identity columns reject an omitted producer and a repeated operation key, and bootstrap seeds a typed writer_fence sentinel. The two open tests stay dropped because the parent already covers journal_mode and the connection contract, and it tests reader query_only as a unit test.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19a5e23605
ℹ️ 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".
| Ok(Self { | ||
| domain_id: identity(&spec.domain_id)?, | ||
| object_id: identity(&spec.object_id)?, | ||
| name: redact(&spec.name), |
There was a problem hiding this comment.
When two live domain names differ only in a detected secret, both names collapse to the same replacement token here, so idx_domains_active_name rejects the second distinct domain as a conflict. Unlike the previously flagged source tuple, this name index is actually unique; reject detected secrets in this identity-bearing field or retain a non-reversible digest for uniqueness.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in e219f31. You are right and this corrects a position I argued explicitly in an earlier reply.
I had drawn an asymmetry: equality used to accept must not collapse distinct values, while equality used to reject over-rejects and is therefore fail-closed and safe. That reasoning is wrong for a uniqueness constraint. Over-rejection is not harmless here, because a uniqueness constraint decides identity just as a lookup does: the second caller cannot create a legitimately distinct domain at all, and the row that did land carries a name with the secret stripped, so it no longer identifies what the caller named.
DomainSpec.name now rejects a detected secret. I applied the same reasoning to source_kind and source_id, which sit in object_registry's UNIQUE(source_kind,source_id,source_revision,object_kind) and have exactly the same shape — leaving those redacted while fixing name would have made the rule incoherent.
The consequence is that a DomainSpec now has no free-text field at all: every field is a key or part of a uniqueness constraint. Redaction still applies where there is genuine free text — actor, cause, candidate payloads, and alignment_payload.
Pinned by two_distinct_secret_bearing_names_are_rejected_not_collapsed, which also asserts a rejected name does not consume the live-name index.
| ); | ||
| if stored_identity != expected | ||
| || terminal_state.is_some() | ||
| || lease_expires_at < spec.recorded_at |
There was a problem hiding this comment.
When spec.recorded_at equals the stored lease_expires_at, this condition passes and the following MAX update extends the run, resurrecting it at the instant its lease is no longer live. Fresh evidence beyond the earlier expired-run fix is that this boundary still uses <, while the schema requires lease_expires_at > heartbeat_at; use <= so exact-expiry arrivals are also rejected.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in e219f31: the comparison is now <=.
Your reasoning holds on its own — a lease expiring at T is not live at T — and the schema agrees: CHECK(lease_expires_at>heartbeat_at) is strict, so the boundary arrival I was accepting would have set heartbeat_at equal to lease_expires_at and tripped that CHECK anyway, surfacing as an opaque constraint error instead of the typed Conflict the caller should get.
Pinned by a_run_whose_lease_expires_exactly_now_is_not_resurrected, which refuses recorded_at == 11 against a stored lease of 11 and still accepts 10. Verified it fails with <.
| pub(super) fn lock_writer(&self) -> Result<std::sync::MutexGuard<'_, Connection>, KernelError> { | ||
| self.writer.lock().map_err(|_| KernelError::Io) |
There was a problem hiding this comment.
When the user-supplied commit callback panics and its caller catches the unwind, the writer guard is dropped during unwinding and this mutex becomes poisoned; every subsequent commit, staging write, or projection rebuild then returns KernelError::Io even though SQLite rolled back the interrupted transaction. Recover the guard with PoisonError::into_inner, as the existing with_writer path already does, so one callback panic does not permanently disable the store instance.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in e219f31. Good catch, and the inconsistency was mine.
with_writer already recovered with unwrap_or_else(PoisonError::into_inner); the lock_writer/lock_reader accessors I added for the envelope used map_err(|_| KernelError::Io) instead. So one caught callback panic poisoned the mutex and every later commit, staging write, and projection rebuild failed permanently — on a store whose transaction SQLite had already rolled back when the guard dropped. Both accessors now recover, matching with_writer.
Recovering is sound here because dropping a rusqlite Transaction rolls it back, so the recovered connection has no in-flight statement.
Pinned by a_caught_callback_panic_leaves_the_store_usable, which panics inside the closure, catches the unwind, and then commits successfully. Verified it fails when the accessor maps poisoning to Io.
Conflicts resolved — now
|
A value compared for equality to decide identity must not collapse distinct values into one, and a uniqueness constraint decides identity just as a lookup does: when two distinct names collapse to one replacement, idx_domains_active_name rejects a legitimately distinct domain and the surviving row carries a name that no longer identifies it. DomainSpec name, source_kind, and source_id join the keys in rejecting a detected secret, superseding the earlier reasoning that over-rejection was harmless here. A domain now holds no free text, so redaction applies to commit metadata and candidate payloads. A lease expiring at T is not live at T, so run reuse rejects an arrival whose recorded_at equals the stored lease_expires_at. lock_writer and lock_reader recover a poisoned guard. A panic in a caller's closure drops the guard mid-unwind, and mapping that to Io left every later write failing on a store whose transaction SQLite had already rolled back. with_writer already recovered; these now match it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e219f316f2
ℹ️ 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".
| fn new(spec: StagingCandidateSpec) -> Result<Self, KernelError> { | ||
| if spec.source_revision < 0 | ||
| || spec.recorded_at < 0 | ||
| || spec.lease_expires_at < spec.recorded_at |
There was a problem hiding this comment.
When a fresh candidate has lease_expires_at == recorded_at, this validation accepts it, but both extraction_runs and candidates enforce lease_expires_at > heartbeat_at; because each inserted heartbeat equals recorded_at, SQLite rejects the request and map_sqlite misreports the malformed input as KernelError::Conflict. Fresh evidence beyond the previously fixed existing-run expiry boundary is that the new-run path still uses <; use <= here so this is rejected consistently as InvalidInput before any transaction work.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in c71f0a0. You are right that I fixed only the reuse boundary and left the fresh-run path at <.
Verified the mechanism: both extraction_runs and candidates carry CHECK(lease_expires_at>heartbeat_at), and the insert sets heartbeat_at from recorded_at, so a zero-duration lease reached SQLite and map_sqlite reported malformed input as Conflict. Validation now rejects it as InvalidInput before the transaction opens.
Pinned by a_zero_duration_lease_is_invalid_input_not_a_conflict.
| if rows.is_empty() { | ||
| return Err(KernelError::InvalidInput); |
There was a problem hiding this comment.
When a full rebuild legitimately produces no decision/observation alignments, this early return prevents the transaction from deleting rows left by the previous rebuild. A repo-wide search finds no separate clear operation, so the caller cannot publish the empty projection and stale alignment rows remain queryable indefinitely; either accept an empty full replacement or expose an explicit transactional clear path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in c71f0a0. This was an over-correction on my part and your reading of the consequence is right.
I had refused an empty slice to stop an accidental empty vector from silently truncating the table, which was a real finding. But that made the legitimate case unreachable: a rebuild that genuinely produces no alignments had no way to retire the previous rows, and your repo-wide search is correct that no other clear path existed, so stale rows stayed queryable forever.
Took the second option you offered. clear_alignment_projection publishes an empty rebuild inside the same fence-checked transaction, returns the row count it retired, and clears the projection's durable_text_redactions rows along with it. Both paths now share one truncate_alignment_projection helper so they cannot drift.
replace_alignment_projection(&[]) still returns InvalidInput, which keeps the original protection: an accidental empty vector is refused, while an intentional empty result has a named entry point the caller has to choose deliberately.
Pinned by an_empty_rebuild_is_publishable_through_the_clear_path, which asserts the refusal still holds, the clear retires both the projection rows and their redaction rows, and a second clear reports zero.
Both extraction_runs and candidates require lease_expires_at greater than heartbeat_at, and each inserted heartbeat equals recorded_at, so a zero-duration lease reached SQLite and surfaced as Conflict. Validation now rejects it as InvalidInput before any transaction work. Refusing an empty replace_alignment_projection left a rebuild that legitimately produced no alignments with no way to retire the previous rows, since no other clear path existed. clear_alignment_projection publishes that result and returns the row count. An empty slice passed to replace stays refused, so an accidental empty vector still cannot erase the projection.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c71f0a0c1b
ℹ️ 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".
| if detected { | ||
| Sensitivity::Sensitive | ||
| } else { |
There was a problem hiding this comment.
When candidate_kind or payload contains a detected secret, this collapses the candidate to Sensitive, even though sensitive records may use local inference while secret records must never be injected or egressed. Fresh evidence beyond the earlier declined thread is the normative magic-context-kh8.1 R10 policy in .beads/issues.jsonl, which explicitly requires every staging candidate to store one of normal, sensitive, or secret and assigns that enforcement to magic-context-kh8.2; add a distinct Secret variant and return it for detections so downstream gates can enforce the stricter rule.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You are right and I was wrong to decline this. Fixed in 5478ebb.
I verified your citation. magic-context-kh8.1 R10 reads: "Every artifact and staging candidate stores normal, sensitive, or secret at ingestion. Unknown content defaults to sensitive. Normal requires affirmative repository provenance and may use configured remote providers. Sensitive content may use local inference only. Vocabulary-covered secrets are redacted ... never inj[ected]". And magic-context-kh8.2, this PR's issue, states Authority: magic-context-kh8 design record — U1; R1–R6, R10. So the third class is normative and enforcing it is in scope here.
My earlier decline argued that a third value was a vocabulary change beyond this PR. That was based on an incomplete search: I looked in crates/ and docs/ and never searched .beads/, which is where this repo keeps its normative design records. My mistake, and it mattered — collapsing a detection into Sensitive is exactly the loss you describe, because sensitive permits local inference while secret must never be injected or egressed.
Sensitivity now has Secret, and a vocabulary detection in any candidate field yields it. Two things I decided beyond the literal ask, both stated so you can push back:
from_storedmaps an unrecognized token toSecretrather thanSensitive. R10's "unknown content defaults to sensitive" governs ingestion-time classification of content; an unreadable stored token is an integrity failure, so it fails to the strictest class instead.- Unproven provenance without any detection still yields
Sensitive, matching R10's stated default.
Pinned by staging_requires_affirmative_repository_provenance_for_normal (now covering all three classes) and one_run_accepts_candidates_with_different_classifications.
| || spec.recorded_at < 0 | ||
| || spec.lease_expires_at <= spec.recorded_at |
There was a problem hiding this comment.
When a caller supplies lease_expires_at more than one hour after recorded_at, this validation accepts it, allowing an active run to evade the required reaper boundary for an arbitrarily long time. Fresh evidence beyond the earlier rebuttal is the normative magic-context-kh8.1 R4 policy in .beads/issues.jsonl, which says an active staging lease expires no later than one hour after its last heartbeat and explicitly assigns this enforcement to magic-context-kh8.2; reject overlong durations using checked arithmetic.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You are right and my earlier rebuttal was wrong. Fixed in 5478ebb.
I claimed no one-hour bound existed in this repository. It does: magic-context-kh8.1 R4 reads "Every active staging run holds a renewable lease that expires no later than one hour after its last heartbeat. The daemon reaper marks an expired lease abandoned." And kh8.2 lists R1–R6 in its authority line, so enforcement belongs here.
The error was in my search, not my reasoning: I grepped crates/ and docs/ and concluded the constant was invented, without searching .beads/. That is where the normative records live, so my rebuttal was confidently wrong on a checkable fact. Thank you for pinning it with the citation rather than restating the claim.
MAX_STAGING_LEASE_MS is now enforced with checked_add, as you asked, so a saturating recorded_at cannot bypass the ceiling.
Pinned by a_lease_beyond_the_one_hour_ceiling_is_rejected, which covers one millisecond over, the i64::MAX overflow path, and exactly-at-ceiling as still valid.
| producer: identity(&intent.producer)?, | ||
| operation_key: identity(&intent.operation_key)?, |
There was a problem hiding this comment.
When a caller leaves producer or operation_key empty or whitespace-only, identity accepts it and the request consumes that blank deduplication key. A later otherwise valid request using the same omitted/default identity is then incorrectly replayed when its digest matches or rejected as Conflict when it differs, rather than being treated as a separate operation; validate both components as nonblank before inserting the commit or receipt.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 5478ebb.
identity() only rejects a detected secret, so a blank or whitespace-only producer/operation_key passed through and claimed ('','') as a dedup key. The next caller that also omitted the field then either replayed that receipt on a digest match or got Conflict on a mismatch, instead of being treated as its own operation. RedactedIntent::new now requires both to be non-blank, alongside the existing digest check, so the request is refused before any commit_log or receipt row is written.
Pinned by a_blank_commit_identity_component_is_rejected, which covers empty and whitespace-only in both positions and asserts commit_log stays empty.
The kh8.1 design record is normative and kh8.2 claims authority over R1-R6 and R10. R10 requires every staging candidate to store normal, sensitive, or secret, and the classes differ materially: sensitive content may use local inference while secret content is never injected or egressed. Collapsing a vocabulary detection into Sensitive lost that distinction, so Sensitivity gains Secret and a detection yields it. An unrecognized stored class now reads back as Secret, since an unreadable token is an integrity failure rather than the ingestion-time default. R4 caps an active staging lease at one hour past its last heartbeat. Validation now rejects a longer duration through checked arithmetic, so a caller cannot hold a run past the reaper boundary. A blank producer or operation_key also consumed the dedup key for every other caller that omitted it, so both must be non-blank.
The writer mutex serializes projection rebuilds but does not order them, so an older build could delete a newer projection and insert rows with a lower generation. A rebuild now requires one generation per batch and refuses a generation older than the stored one. A lost staging response left the producer unable to retry: the identical request reached the candidate primary key and returned Conflict even though the row was already durable. An exact match now returns the stored row and Conflict is reserved for changed content. change_event stored the bare operation_key, but receipt identity is (producer, operation_key), so two producers sharing a key produced indistinguishable events. The column now carries transaction_id, which already hashes both. A candidate id can be reused once its row is deleted, so recording its redactions first clears any rows left under that owner.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77b816fd96
ℹ️ 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".
| .transaction_with_behavior(TransactionBehavior::Immediate) | ||
| .map_err(map_sqlite)?; | ||
| check_fence(&tx, self.lease_epoch())?; | ||
| let removed = truncate_alignment_projection(&tx)?; |
There was a problem hiding this comment.
When a newer rebuild produces no alignments, this path deletes every row without recording the rebuild's built_through_commit_seq; a slower non-empty rebuild from an older snapshot then observes MAX(...) = NULL and is allowed to repopulate stale rows. Fresh evidence beyond the earlier stale-rebuild comment is that only non-empty replacements retain a comparable generation. Accept the empty rebuild's generation and persist a projection watermark independently of its rows before permitting later replacements.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 22e7a4f. You are right that my generation guard only worked for non-empty rebuilds.
Took the approach you describe: the watermark is now persisted independently of the rows, in a new alignment_projection_state singleton relation, so deleting every row no longer erases the ordering information. Both rebuild paths guard against it and then advance it, and clear_alignment_projection now takes the generation it publishes rather than clearing anonymously.
I added a new relation rather than a column on alignment_projection because a row-less projection has nowhere to put a generation, and rather than a column on one of the parent branch's tables because a new component is additive while modifying its tables is not.
Pinned by an_empty_rebuild_still_orders_later_replacements, which clears at generation 2 and then refuses a generation-1 rebuild with Conflict, asserting no stale rows return.
| "SELECT extraction_run_id,candidate_kind,payload,sensitivity_class, | ||
| provenance_witness | ||
| FROM candidates WHERE candidate_id=?1", |
There was a problem hiding this comment.
When the same candidate is restaged with later recorded_at and lease_expires_at values, this lookup omits those timestamps, so the request is accepted and the parent run is renewed while the candidate retains its original heartbeat and lease. The candidate can therefore reach its independently indexed expiry while its run is still live. Either require timestamps to match for an idempotent replay or renew the candidate's lease in the same transaction.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 22e7a4f.
Chose your second option: the replay renews the candidate's heartbeat_at and lease_expires_at on the same MAX terms already applied to its run, in the same transaction. Requiring the timestamps to match would have made a legitimate later retry fail, which defeats the point of the retry path.
Pinned by an_identical_restage_renews_the_candidate_lease_with_its_run, which restages at a later timestamp and asserts the candidate's heartbeat and lease equal the run's rather than lagging.
| let expected = ( | ||
| spec.extraction_run_id.clone(), | ||
| spec.candidate_kind.text.clone(), | ||
| spec.payload.text.as_bytes().to_vec(), | ||
| candidate_sensitivity.as_str().to_string(), | ||
| provenance.clone(), |
There was a problem hiding this comment.
When two requests reuse a candidate ID and their payloads differ only in secret bytes recognized as the same secret type, redaction collapses both payloads to the same replacement before this comparison, so the second request is treated as an identical replay rather than a conflict. Fresh evidence beyond the prior retry comment is that the new equality check compares only redacted bytes and stores no pre-redaction fingerprint. Compare a non-reversible digest of the original candidate request so distinct secret-bearing payloads cannot alias.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 22e7a4f. This is the third place the lossy-comparison problem surfaced, and it was in code I added one round earlier — the replay check compared post-redaction bytes, which is exactly the aliasing I had been fixing elsewhere.
Implemented your suggestion. request_digest hashes the request before redaction, with each component length-prefixed so no two field splits share a preimage, and the replay compares that instead of the stored payload.
It needed no schema change: the digest travels inside the redaction_metadata BLOB this module already writes, so the parent branch's candidates columns are untouched.
Pinned by two_payloads_differing_only_in_secret_bytes_are_not_the_same_replay, which stages two payloads that redact identically under the same secret type and asserts the second conflicts.
…ests An empty rebuild deleted every row, so the generation vanished with them and a slower non-empty rebuild from an older snapshot saw no watermark and repopulated stale rows. The generation now lives in alignment_projection_state, independent of the rows, and both rebuild paths guard against and then advance it. clear_alignment_projection takes the generation it publishes. The staging replay check compared redacted payloads, so two requests whose payloads differed only inside a secret of the same type aliased and the second was accepted as an identical replay. A digest of the request before redaction now travels in the metadata this module already writes, and the replay compares that instead. A replayed candidate also kept its original heartbeat and lease while its run was renewed, so it could expire under a live run. The replay now renews the candidate on the same terms as its run.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22e7a4ff91
ℹ️ 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".
| if spec.source_revision < 0 | ||
| || spec.recorded_at < 0 | ||
| || spec.lease_expires_at <= spec.recorded_at | ||
| || spec.lease_expires_at > lease_ceiling |
There was a problem hiding this comment.
When a fresh request supplies a far-future recorded_at, this validation accepts it as long as lease_expires_at is within one hour of that caller-controlled timestamp. The inserted run then has a future heartbeat and will remain active until that future lease expires, so a timestamp such as i64::MAX - MAX_STAGING_LEASE_MS effectively bypasses the staging-expiry policy and prevents the reaper from abandoning the run. Derive the heartbeat from the store clock or reject timestamps beyond an explicitly bounded clock skew.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 3255750. Good catch — the R4 ceiling I added last round was measured entirely against a caller-controlled value, so it enforced nothing.
Took the bounded-skew option rather than deriving the heartbeat from the store clock, because several callers and tests legitimately supply their own timestamps and asserting on them is useful. MAX_STAGING_CLOCK_SKEW_MS is one minute, deliberately small against the one-hour lease cap so a future heartbeat cannot materially move the reaper boundary, and the addition is checked so a saturating value cannot wrap past the bound.
Pinned by a_far_future_heartbeat_cannot_outrun_the_reaper, which rejects your i64::MAX - MAX_STAGING_LEASE_MS case, rejects an hour of clock lead, and accepts a present-time heartbeat.
| || spec.extraction_run_id.trim().is_empty() | ||
| || spec.candidate_id.trim().is_empty() |
There was a problem hiding this comment.
When extractor, source_kind, or source_id is empty or whitespace-only, this validation still accepts the request because it checks only the run and candidate IDs, and identity() does not reject blanks. The run is then durably created with unusable immutable source metadata; retrying the same run ID with the actual values returns Conflict, and a candidate with repository provenance can even be classified normal despite the missing source classification. Validate these required run identity fields as nonblank before insertion.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 3255750. Same gap as the commit identity fields: identity() rejects a detected secret but says nothing about blanks.
extractor, source_kind, and source_id must now be non-blank, and I added candidate_kind for the same reason since it is equally required and equally unvalidated. Your point about classification is the sharper half: a blank source_kind with repository provenance would have been stored normal with no source classification behind it.
Pinned by blank_run_identity_fields_are_rejected, which covers empty and whitespace-only across all four fields and asserts no extraction_runs row is created.
The lease ceiling was measured from a caller-supplied heartbeat, so a recorded_at far in the future produced a run that stayed active for years within its own one-hour window. A heartbeat may now lead the store clock only by a bounded skew, which is small against the lease cap. identity() rejects a detected secret but not a blank string, so a blank extractor, source_kind, source_id, or candidate_kind reached the run row as unusable immutable metadata. Retrying that run id with real values then returned Conflict, and a candidate with provenance could be classified normal without any source classification. All four must be non-blank.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32557507a7
ℹ️ 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".
| ); | ||
| if stored_identity != expected | ||
| || terminal_state.is_some() | ||
| || lease_expires_at <= spec.recorded_at |
There was a problem hiding this comment.
When a run expired recently, a caller can set recorded_at just before the old expiry and lease_expires_at up to an hour after that timestamp; this comparison passes and the following MAX update renews the expired run into the future. Fresh evidence beyond the earlier expired-run fix is that validation imposes no lower bound on caller-supplied heartbeats, so comparing the stored expiry only with spec.recorded_at does not establish that the run is still live; compare against the store clock inside the transaction before renewing it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in b98f289. You are right that comparing the stored expiry against spec.recorded_at proves nothing, since the caller supplies both sides.
Reuse now additionally requires the stored lease_expires_at to be ahead of current_time_ms(), evaluated inside the transaction. The skew bound I added last round only constrains how far a heartbeat may lead the clock, so a caller could still place one in the past exactly as you describe.
This changed the test contract: the staging fixtures used synthetic timestamps like 1 and 5, which no longer describe a live run once liveness is judged against the real clock. They are now anchored to the store clock, and the two tests that asserted absolute renewal values assert the relationship instead. Expiry is exercised by moving the stored lease into the past out of band, which is closer to the stalled-worker case anyway.
Pinned by the rewritten a_terminal_or_expired_run_refuses_further_candidates.
| Ok(Self { | ||
| decision_id: identity(&spec.decision_id)?, | ||
| observation_id: identity(&spec.observation_id)?, | ||
| alignment_kind: redact(&spec.alignment_kind), |
There was a problem hiding this comment.
When a rebuild supplies an empty or whitespace-only alignment_kind, this constructor accepts it and the full replacement commits successfully because the schema enforces only NOT NULL. The transaction consequently deletes the previous valid projection and replaces it with a row whose relationship kind consumers cannot interpret; validate this required discriminator as nonblank before publishing the rebuild.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in b98f289.
alignment_kind is the discriminator consumers read and the schema only enforces NOT NULL, so a blank value would have truncated a valid projection and replaced it with an uninterpretable row. It must now be non-blank, and I added the same check to decision_id and observation_id, which were equally unvalidated — identity() rejects a detected secret but says nothing about blanks.
Pinned by a_blank_alignment_kind_cannot_replace_a_valid_projection, which covers empty, spaces, and a tab, and asserts the previous projection survives each rejection.
| "SELECT extraction_run_id,sensitivity_class,redaction_metadata | ||
| FROM candidates WHERE candidate_id=?1", |
There was a problem hiding this comment.
When a candidate has independently reached completed, failed, canceled, or abandoned while its extraction run remains active, an identical restage never observes that state: this lookup omits terminal_state, and the replay path renews the candidate or returns success while it remains terminal. Fresh evidence beyond the earlier terminal-run fix is that only the parent run lookup loads its terminal state; load and reject the candidate's own terminal state before treating it as an idempotent replay.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in b98f289. The parallel to the run-terminal fix is exact, and I had only applied it to the parent.
The candidate lookup now loads terminal_state and refuses a replay when it is set, so a finished candidate is neither renewed nor reported as a successful replay while its run is still live.
Pinned by a_terminal_candidate_is_not_replayed_under_a_live_run, which marks the candidate completed out of band, leaves the run live, and asserts the restage conflicts.
The expiry check compared the stored lease against a caller-supplied recorded_at, which the caller can place just before an old expiry to renew a dead run. Reuse now also requires the stored lease to be ahead of the store clock. The staging tests move to timestamps near real time, since synthetic values no longer describe a live run. A candidate can reach a terminal state while its run stays active, so the replay lookup loads terminal_state and refuses rather than renewing. alignment_kind is the discriminator consumers read, so a blank value would publish an uninterpretable rebuild over a valid projection. It and both projection identifiers must be non-blank.
Status: all review comments answered, conflicts resolved, merge blocked externally35 inline comments, 35 replies, 0 unanswered. Six review rounds handled since the original four-lens pass; each round of fixes surfaced the next layer, which is worth reading as a good sign about the reviewer rather than a bad one about the code. The through-lineNearly every finding was one defect wearing different clothes: a value compared for equality to decide identity must not collapse distinct inputs, and The corollary I got wrong twice and have corrected: I argued that over-rejection under a uniqueness constraint was fail-closed and therefore harmless. It is not — the second caller cannot create a distinct row at all, and the row that landed carries a name that no longer identifies it. Two findings I declined and should not haveI rejected the one-hour staging lease bound as an invented constant, and a third ConflictsThe base was rebased and its redaction commit rewritten, so this branch carried a stale duplicate. Resolved by taking the parent's version of every shared file and re-applying only what the envelope adds. Three of my earlier fixes were redundant against work the parent had done independently and were dropped. Git also auto-merged one plugin test file by duplicating an import and an entire Verification212 What blocks mergeNothing on this branch. The remaining CI failures are inherited: base
So: this PR is |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b98f28958a
ℹ️ 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".
| struct Envelope<'a> { | ||
| request_digest: &'a str, | ||
| detections: Vec<Metadata<'a>>, |
There was a problem hiding this comment.
When a candidate contains a low-entropy detected secret, persisting request_digest here creates an offline verifier for the original unredacted request: the other digest components are stored in the same database, so an attacker can enumerate candidate secret values and compare SHA-256 outputs. This also violates the normative R10 requirement in .beads/issues.jsonl that staging detection metadata contain only detector ID, secret type, offset, and length. Preserve retry distinction with a keyed verifier whose key is not stored in core.sqlite, or avoid retaining a digest derived from secret-bearing input.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 68a556c. This is the most serious finding on the PR and it was self-inflicted: the digest I added two rounds ago to stop replay aliasing reintroduced secret exposure.
Both halves of your claim check out. Every other digest component — run id, candidate id, extractor, source kind, source id, candidate kind, provenance, revision — is stored in plaintext in the same database, so the digest reduces to an unsalted hash of the payload with all other inputs known. For a low-entropy secret that is a practical offline recovery. And R10 is explicit: "Detection metadata stores only detector ID, secret type, offset, and length." So it violated the spec directly, not just in spirit.
Took your second option and removed the digest rather than introducing a keyed verifier. A key that must not live in core.sqlite brings placement, rotation, and backup questions that belong in their own change, and R12a's backup requirement would need an answer for it too.
That means accepting a real limitation, stated plainly: replay now compares only R10-permitted data plus the stored payload, and a detection on either side makes an unchanged retry unprovable, so it returns Conflict. A producer staging a secret-bearing candidate cannot silently retry. That is the honest consequence of redacting the payload — the information needed to prove the retry is exactly what we deliberately destroyed. Detection-free candidates stay replayable because their stored payload is exact.
Pinned by two tests: staging_metadata_retains_no_verifier_for_a_redacted_secret asserts the stored keys are exactly the R10 set and that no digest, hash, or fingerprint field survives; a_secret_bearing_candidate_is_not_replayed_from_a_lossy_payload asserts the secret case conflicts while the clean case still replays.
|
The commit this PR was still missing is now up as #133, against For the record on why a new PR rather than reopening this one: GitHub refuses to reopen a merged PR, and this one is marked merged against I did not revert |
1 participant
Summary
Review focus
Atomic mutation envelope and historical-read semantics. Retention policy lands in #116.
Stack
Part 5 of 8. Depends on #114; followed by #116.