Store: add with_audited_effect (type-enforced audit-before-effect pairing)

[George-RD]

Parent

#208 (Spec: deepened Store interface)

Blocked by

#213 (Add Store transaction combinators)

Deliverables

Add the audit-before-effect combinator so a wrong ordering stops compiling on the effect path (expand step).

• AuditDescriptor value type carrying the audit-row inputs (kind, action, decision, reason, task_grant_id, refs) as data.
• Store::with_audited_effect(&self, descriptor: AuditDescriptor, effect: impl FnOnce(&rusqlite::Transaction) -> Result<T, StoreError>) -> Result<T, StoreError>: runs effect and append_audit_conn inside one Immediate transaction, commits both atomically. It is the ONLY effect-path entry that can write an effect row; audit-only and non-effect internal-maintenance writes use separately named entries.
• Pilot the rollbackable shape by migrating store/identity.rs::owner_assert_identity_binding (currently hand-pairs effect + append_audit_conn).
• Test: a forced append_audit failure rolls back the effect row (no orphan effect — AD-105 ledger-before-consume).

Rationale in spec #208 (decision D-002). This encapsulates the 83 hand-paired append_audit_conn sites; the mass migration is T6.

Verification

./scripts/check.sh green. An effect-only write on the effect path must not compile (exercised by the migrated pilot).

🤖 Generated from spec #208 (wayfinder #189, map #182).

[coderabbitai]

Coding Plan

Summary

Add an owned AuditDescriptor value type and a Store::with_audited_effect combinator that runs the effect closure and append_audit_conn in one Immediate transaction and commits atomically.

Make the combinator the single effect-path entry that can write an effect row. Its signature forces audit metadata, so an effect-only write cannot compile on this path.

Migrate owner_assert_identity_binding as the pilot to validate the rollbackable shape. Defer the mass migration of the remaining approximately 83 sites to T6.

Add a rollback regression test using the existing install_audit_append_failure_for_kind trigger harness to prove no orphan effect row remains, per AD-105 ledger-before-consume.

Design Choices

Design Choice 1: Location of `AuditDescriptor`

Options Considered:

1. Place AuditDescriptor in openspine-schemas next to AuditEvent/AuditKind.
2. Place AuditDescriptor in the kernel store module next to append_audit_conn and with_audited_effect.

Chosen Option: 2

Rationale: The combinator is a Store method and the descriptor is consumed only by the store audit path. Keep both together to avoid a cross-crate coupling that no other consumer needs.

Design Choice 2: Call order of the effect closure and the audit append inside the transaction

Options Considered:

1. Run the effect closure first, then append_audit_conn, then commit.
2. Run append_audit_conn first, then the effect closure, then commit.

Chosen Option: 1

Rationale: The atomic single-transaction commit makes both writes durable together. The "audit-before-effect pairing" guarantee is enforced by the shared type, because the combinator forces an audit for every effect, not by intra-transaction call order. Option 1 also matches the existing owner_assert_identity_binding structure and keeps the migration change small.

💡 User Tips

Regenerate the plan with different choices with @coderabbitai <feedback>.

Implementation Steps

Phase 1: Introduce the audited-effect combinator

This phase adds the value type and the combinator. The combinator becomes the single effect-path entry that can write an effect row. Its signature forces every caller to supply audit metadata, so an effect-only write cannot be expressed on this path.

Task 1: Define the `AuditDescriptor` value type

Create a value type that carries all audit-row inputs as owned data.

• Add a new module crates/openspine-kernel/src/store/audited_effect.rs and declare it in crates/openspine-kernel/src/store/mod.rs.
• Define pub struct AuditDescriptor with owned fields that mirror the append_audit_conn inputs: kind: String, action: Option<ActionId>, decision: Option<GateDecision>, reason: Option<String>, task_grant_id: Option<Ulid>, target_refs: Vec<ArtifactRef>, payload_refs: Vec<ArtifactRef>.
• Provide a small constructor or builder for the common case (kind plus reason), so pilot and future call sites stay concise. Keep optional fields defaulting to None/empty.
• Derive Debug and Clone. Do not couple the type to Store or rusqlite.

Task 2: Implement `Store::with_audited_effect`

Add the combinator that pairs an effect closure with its audit append in one transaction.

• Implement pub fn with_audited_effect<T>(&self, descriptor: AuditDescriptor, effect: impl FnOnce(&rusqlite::Transaction) -> Result<T, StoreError>) -> Result<T, StoreError> in audited_effect.rs as a Store method.
• Lock the connection, open a transaction with TransactionBehavior::Immediate.
• Run effect(&tx)? to perform the effect row writes.
• Call Self::append_audit_conn(&tx, ...) with borrowed values derived from descriptor (descriptor.kind.as_str(), descriptor.action.as_ref(), descriptor.decision.as_ref(), descriptor.reason.as_deref(), descriptor.task_grant_id, &descriptor.target_refs, &descriptor.payload_refs).
• On the audit-append error, return early so the transaction drops and rolls back both the effect writes and the failed audit write. Commit only when both succeed. Return the effect result T.
• Document that this is the only effect-path entry allowed to write an effect row; audit-only and non-effect internal-maintenance writes must use separately named entries (append_audit, append_audit_conn, or dedicated methods). Reference AD-105 ledger-before-consume in the doc comment.

🤖 Prompt for AI agents

Implement the audited-effect combinator infrastructure in the kernel store
crate. Goal: introduce a value type that carries audit-row inputs, and a `Store`
method that pairs an effect closure with an audit append inside one atomic
transaction.

- Create `crates/openspine-kernel/src/store/audited_effect.rs` and declare it as
a module in `crates/openspine-kernel/src/store/mod.rs`.
- Define `pub struct AuditDescriptor` with owned fields: `kind: String`,
`action: Option<ActionId>`, `decision: Option<GateDecision>`, `reason:
Option<String>`, `task_grant_id: Option<Ulid>`, `target_refs: Vec<ArtifactRef>`,
`payload_refs: Vec<ArtifactRef>`. These fields mirror the inputs of
`append_audit_conn`.
- Add a constructor or builder for the common case of kind plus reason, keeping
optional fields defaulted to `None` or empty. Derive `Debug` and `Clone`. Do not
couple `AuditDescriptor` to `Store` or `rusqlite`.
- Implement `pub fn with_audited_effect<T>(&self, descriptor: AuditDescriptor,
effect: impl FnOnce(&rusqlite::Transaction) -> Result<T, StoreError>) ->
Result<T, StoreError>` as a method on `Store` in `audited_effect.rs`.
- Inside `with_audited_effect`: lock the connection, open a transaction with
`TransactionBehavior::Immediate`, run `effect(&tx)?` for the effect row writes,
then call `Self::append_audit_conn(&tx, ...)` with values derived from the
descriptor (`descriptor.kind.as_str()`, `descriptor.action.as_ref()`,
`descriptor.decision.as_ref()`, `descriptor.reason.as_deref()`,
`descriptor.task_grant_id`, `&descriptor.target_refs`,
`&descriptor.payload_refs`).
- On audit-append error, return early so the transaction drops and rolls back
both the effect writes and the failed audit write. Commit only when both
operations succeed. Return the effect result `T`.
- Add a doc comment stating that `with_audited_effect` is the only effect-path
entry allowed to write an effect row. State that audit-only and non-effect
internal-maintenance writes must use separately named entries (`append_audit`,
`append_audit_conn`, or dedicated methods). Reference AD-105
ledger-before-consume in the doc comment.

Phase 2: Pilot migration and rollback regression test

This phase proves the rollbackable shape by migrating one hand-paired site and adding a regression test that a forced audit failure leaves no orphan effect row.

Task 1: Migrate `owner_assert_identity_binding` to the combinator

Replace the hand-paired transaction in the pilot function with with_audited_effect.

• Update crates/openspine-kernel/src/store/identity.rs.
• Keep the pre-check self.owner_principal_by_id(owner_principal_id)? outside the combinator.
• Build an AuditDescriptor with kind = "identity.bound" and reason = Some(format!("owner={owner_principal_id} identity={}", identity.id)); leave action, decision, task_grant_id, and both ref vectors empty.
• Call self.with_audited_effect(descriptor, |tx| { ... }). Move the identities insert and the identity_identifiers loop inserts into the closure body. Return Ok(()) from the closure.
• Remove the manual conn.lock(), conn.transaction(), inline append_audit_conn, and tx.commit() from this function. Preserve the existing function signature and return type.

Task 2: Add the audit-failure rollback test

Add a regression test that proves atomic rollback of the effect rows when the audit append fails.

• Add the test to crates/openspine-kernel/src/store/identity_tests.rs, reusing existing fixtures (Store::open_in_memory, bootstrap_owner_principal, VerifiedOwnerContext::test_new, Identity/Identifier construction with a sha2::Sha256 value hash).
• Call store.install_audit_append_failure_for_kind("identity.bound") before the operation.
• Call owner_assert_identity_binding(...) and capture the error. Assert it is a StoreError::Sqlite(_) (the injected RAISE(FAIL, ...)).
• Assert the effect rows rolled back: get_identity(counterparty_id) returns None, resolve_identity_by_identifier_hash(...) returns None, count_identities() equals the pre-operation baseline (owner identity only), and count_audit_events_of_kind("identity.bound") equals 0.
• Keep the existing success and non-owner tests unchanged so they continue to guard the migrated function.

🤖 Prompt for AI agents

Migrate the pilot function to the new combinator and add a rollback regression
test. Goal: prove the audited-effect combinator produces atomic rollback
behavior on a real call site.

- In `crates/openspine-kernel/src/store/identity.rs`, update
`owner_assert_identity_binding`. Keep the pre-check
`self.owner_principal_by_id(owner_principal_id)?` outside the combinator call.
- Build an `AuditDescriptor` with `kind = "identity.bound"` and `reason =
Some(format!("owner={owner_principal_id} identity={}", identity.id))`. Leave
`action`, `decision`, `task_grant_id`, and both ref vectors (`target_refs`,
`payload_refs`) empty.
- Call `self.with_audited_effect(descriptor, |tx| { ... })`, moving the
`identities` insert and the `identity_identifiers` loop inserts into the closure
body. Return `Ok(())` from the closure.
- Remove the manual `conn.lock()`, `conn.transaction()`, inline
`append_audit_conn` call, and `tx.commit()` from this function. Preserve the
existing function signature and return type.
- In `crates/openspine-kernel/src/store/identity_tests.rs`, add a new test that
reuses existing fixtures: `Store::open_in_memory`, `bootstrap_owner_principal`,
`VerifiedOwnerContext::test_new`, and `Identity`/`Identifier` construction with
a `sha2::Sha256` value hash.
- In the test, call
`store.install_audit_append_failure_for_kind("identity.bound")` before invoking
the operation.
- Call `owner_assert_identity_binding(...)`, capture the resulting error, and
assert it is a `StoreError::Sqlite(_)` variant (matching the injected
`RAISE(FAIL, ...)`).
- Assert the effect rows rolled back: `get_identity(counterparty_id)` returns
`None`, `resolve_identity_by_identifier_hash(...)` returns `None`,
`count_identities()` equals the pre-operation baseline (owner identity only),
and `count_audit_events_of_kind("identity.bound")` equals 0.
- Do not modify the existing success and non-owner tests; keep them unchanged so
they continue to guard the migrated function.

Research

The kernel store lives in crates/openspine-kernel/src/store/. Store (in mod.rs) is a cloneable handle over a single rusqlite::Connection behind a parking_lot::Mutex. There is no pool. StoreError is the crate-wide error enum.

Audit rows are appended through audit_append.rs. append_audit/append_audit_with_payload_json self-lock and open their own Immediate transaction. append_audit_conn (pub(crate)) is connection-scoped and folds an audit write into an existing transaction.

Today, effect writes and their audit rows are hand-paired at approximately 83 call sites. Each site manually locks, opens a transaction, writes effect rows, calls append_audit_conn, and commits. No transaction combinator abstraction exists yet.

owner_assert_identity_binding in identity.rs is a representative hand-paired site. It pre-checks the owner principal, opens a transaction, inserts into identities and identity_identifiers, appends an "identity.bound" audit row, and commits.

The codebase already has a deterministic audit-failure test harness. install_audit_append_failure_for_kind (in audit_support_tests.rs) installs a SQLite BEFORE INSERT trigger that raises RAISE(FAIL, ...) for a target kind. personality_seed_tests.rs shows the rollback-assertion pattern.

---

🚀 Next Steps

🤖 All AI agent prompts combined

Task: 1

Implement the audited-effect combinator infrastructure in the kernel store
crate. Goal: introduce a value type that carries audit-row inputs, and a `Store`
method that pairs an effect closure with an audit append inside one atomic
transaction.

- Create `crates/openspine-kernel/src/store/audited_effect.rs` and declare it as
a module in `crates/openspine-kernel/src/store/mod.rs`.
- Define `pub struct AuditDescriptor` with owned fields: `kind: String`,
`action: Option<ActionId>`, `decision: Option<GateDecision>`, `reason:
Option<String>`, `task_grant_id: Option<Ulid>`, `target_refs: Vec<ArtifactRef>`,
`payload_refs: Vec<ArtifactRef>`. These fields mirror the inputs of
`append_audit_conn`.
- Add a constructor or builder for the common case of kind plus reason, keeping
optional fields defaulted to `None` or empty. Derive `Debug` and `Clone`. Do not
couple `AuditDescriptor` to `Store` or `rusqlite`.
- Implement `pub fn with_audited_effect<T>(&self, descriptor: AuditDescriptor,
effect: impl FnOnce(&rusqlite::Transaction) -> Result<T, StoreError>) ->
Result<T, StoreError>` as a method on `Store` in `audited_effect.rs`.
- Inside `with_audited_effect`: lock the connection, open a transaction with
`TransactionBehavior::Immediate`, run `effect(&tx)?` for the effect row writes,
then call `Self::append_audit_conn(&tx, ...)` with values derived from the
descriptor (`descriptor.kind.as_str()`, `descriptor.action.as_ref()`,
`descriptor.decision.as_ref()`, `descriptor.reason.as_deref()`,
`descriptor.task_grant_id`, `&descriptor.target_refs`,
`&descriptor.payload_refs`).
- On audit-append error, return early so the transaction drops and rolls back
both the effect writes and the failed audit write. Commit only when both
operations succeed. Return the effect result `T`.
- Add a doc comment stating that `with_audited_effect` is the only effect-path
entry allowed to write an effect row. State that audit-only and non-effect
internal-maintenance writes must use separately named entries (`append_audit`,
`append_audit_conn`, or dedicated methods). Reference AD-105
ledger-before-consume in the doc comment.
===============================================================================

Task: 2

Migrate the pilot function to the new combinator and add a rollback regression
test. Goal: prove the audited-effect combinator produces atomic rollback
behavior on a real call site.

- In `crates/openspine-kernel/src/store/identity.rs`, update
`owner_assert_identity_binding`. Keep the pre-check
`self.owner_principal_by_id(owner_principal_id)?` outside the combinator call.
- Build an `AuditDescriptor` with `kind = "identity.bound"` and `reason =
Some(format!("owner={owner_principal_id} identity={}", identity.id))`. Leave
`action`, `decision`, `task_grant_id`, and both ref vectors (`target_refs`,
`payload_refs`) empty.
- Call `self.with_audited_effect(descriptor, |tx| { ... })`, moving the
`identities` insert and the `identity_identifiers` loop inserts into the closure
body. Return `Ok(())` from the closure.
- Remove the manual `conn.lock()`, `conn.transaction()`, inline
`append_audit_conn` call, and `tx.commit()` from this function. Preserve the
existing function signature and return type.
- In `crates/openspine-kernel/src/store/identity_tests.rs`, add a new test that
reuses existing fixtures: `Store::open_in_memory`, `bootstrap_owner_principal`,
`VerifiedOwnerContext::test_new`, and `Identity`/`Identifier` construction with
a `sha2::Sha256` value hash.
- In the test, call
`store.install_audit_append_failure_for_kind("identity.bound")` before invoking
the operation.
- Call `owner_assert_identity_binding(...)`, capture the resulting error, and
assert it is a `StoreError::Sqlite(_)` variant (matching the injected
`RAISE(FAIL, ...)`).
- Assert the effect rows rolled back: `get_identity(counterparty_id)` returns
`None`, `resolve_identity_by_identifier_hash(...)` returns `None`,
`count_identities()` equals the pre-operation baseline (owner identity only),
and `count_audit_events_of_kind("identity.bound")` equals 0.
- Do not modify the existing success and non-owner tests; keep them unchanged so
they continue to guard the migrated function.

💡 Iterate on the plan with: `@coderabbitai` <feedback>

Example Feedback
- `@coderabbitai` You can skip phase 3. Add a simple unit test case for phase 2.
- `@coderabbitai` For design choice 1 go ahead with option 3 and replan.

---

💬 Have feedback or questions? Drop into our discord!

[George-RD]

Coordination note (map #182 final pass): this seam is the extension point the Effect Truth epic #198 fills with disposition logic — see the mirror note on #198. Seam shape only here; no disposition classification.

[George-RD]

Correction: the earlier coordination note referring to the begin_effect/settle_effect seam belongs on #216, not here. #215 is the with_audited_effect pairing only.