Skip to content

store: add with_audited_effect audit-before-effect combinator (#215) - #239

Merged
George-RD merged 1 commit into
mainfrom
George-RD/dev-215
Aug 21, 2026
Merged

store: add with_audited_effect audit-before-effect combinator (#215)#239
George-RD merged 1 commit into
mainfrom
George-RD/dev-215

Conversation

@George-RD

@George-RD George-RD commented Aug 21, 2026

Copy link
Copy Markdown
Owner

What

Adds the audit-before-effect combinator so a wrong ordering stops compiling on the effect path (spec #208 D-002, expand step; blocked-by #213 which is merged).

  • AuditDescriptor (store/audited_effect.rs) — owned value type carrying the audit-row inputs (kind, action, decision, reason, task_grant_id, target_refs, payload_refs) as data, decoupled from Store/rusqlite. new(kind) + with_reason(...) cover the common case; other fields are public and default to None/empty.
  • Store::with_audited_effect(descriptor, effect) — the ONLY effect-path entry that can write an effect row. It runs the effect closure, then append_audit_conn, inside one Immediate transaction and commits atomically. Routes through the existing with_immediate_tx combinator (Store: add with_immediate_tx / with_deferred_read transaction combinators #213) rather than restating the D-050 write-serialization discipline. Audit-only and internal-maintenance writes keep using their separately named entries.
  • Pilot migrationowner_assert_identity_binding moves off its hand-paired transaction onto the combinator.
  • Rollback regression test — a forced identity.bound audit failure (via the existing install_audit_append_failure_for_kind trigger harness) leaves no orphan effect row (AD-105 ledger-before-consume).

Why

The 83 hand-paired append_audit_conn sites enforce audit-before-effect by convention; a single missed pairing is invisible until an audit query comes up short. This encapsulates the discipline so an effect-only write cannot compile on the effect path. Mass migration of the remaining sites is #218 (not this ticket).

Notes

  • The pilot previously ran a Deferred transaction (conn.transaction()); routing it through with_audited_effectwith_immediate_tx upgrades it to Immediate, aligning with spec Spec: deepened Store interface #208 D-001 / D-050. Behavior-preserving on the happy path, strictly safer under write contention.
  • Diff scoped to with_audited_effect only (concurrency note: dev-214/dev-217 touch other store files in parallel).

Verification

./scripts/check.sh green — 45/45 spec items, full kernel suite including the shell-binary E2E; 7/7 store::identity tests pass (incl. the new rollback test).

Implementation-notes five-line summary

  1. Added AuditDescriptor value type + Store::with_audited_effect combinator in store/audited_effect.rs — the only effect-path entry that can write an effect row.
  2. It folds the effect closure and append_audit_conn into one Immediate transaction (via with_immediate_tx) and commits atomically; effect-only writes cannot be expressed on this path (AD-105 ledger-before-consume).
  3. Migrated the pilot owner_assert_identity_binding off its hand-paired Deferred transaction onto the combinator, upgrading it to Immediate.
  4. Added a rollback regression test: a forced identity.bound audit failure leaves no orphan effect row.
  5. ./scripts/check.sh green; diff scoped to with_audited_effect only.

Closes #215


Summary by cubic

Adds Store::with_audited_effect to enforce audit-before-effect on the effect path. Previously callers hand-paired effect writes with append_audit_conn; now the combinator runs the effect then appends audit in one Immediate transaction, so a write with no prior audit cannot be expressed on this path and rollbacks leave no orphan effect rows.

  • Introduces AuditDescriptor (owned inputs for audit metadata) and the with_audited_effect(descriptor, effect) API in store/audited_effect.rs, implemented via with_immediate_tx.
  • Migrates the pilot owner_assert_identity_binding from a Deferred transaction to with_audited_effect (Immediate); behavior is preserved, safety under write contention improves.
  • Adds a regression test that forces identity.bound audit failure and verifies no orphan identity or identifier rows remain.
  • Audit-only and internal-maintenance writes are unchanged and continue to use their dedicated entries.
  • Future work: migrate remaining effect-path sites to the combinator (tracked separately).

Written for commit 96661b7. Summary will update on new commits.

Review in cubic

Add AuditDescriptor + Store::with_audited_effect: the single effect-path
entry that can write an effect row, folding the effect closure and its
audit append into one Immediate transaction (via with_immediate_tx) and
committing atomically. Effect-only writes cannot be expressed on this
path (AD-105 ledger-before-consume, spec #208 D-002).

Migrate the pilot owner_assert_identity_binding off its hand-paired
Deferred transaction onto the combinator; add a rollback regression test
proving a forced audit failure leaves no orphan effect row.
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@coderabbitai

coderabbitai Bot commented Aug 21, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@George-RD, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 9 minutes

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: b3f317fb-97a8-4ed9-9ea5-8691f4bafbb6

📥 Commits

Reviewing files that changed from the base of the PR and between eb84d7a and 96661b7.

📒 Files selected for processing (4)
  • crates/openspine-kernel/src/store/audited_effect.rs
  • crates/openspine-kernel/src/store/identity.rs
  • crates/openspine-kernel/src/store/identity_tests.rs
  • crates/openspine-kernel/src/store/mod.rs

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@George-RD
George-RD merged commit ceb6d7a into main Aug 21, 2026
3 checks passed
@George-RD
George-RD deleted the George-RD/dev-215 branch August 21, 2026 03:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

1 participant