Record scoped-admission effect failures once, not twice

[George-RD]

What to build

On the scope-matched standing-rule path, a failed gmail.create_draft is recorded twice: the shared executor appends draft.creation_failed and calls batch_failure (pipeline/approval_draft.rs:322-336), and the mediation error handler then re-appends action.dispatch_failed and calls batch_failure again for the DispatchError::Connector that dispatch_scoped_effect synthesised from the executor's ConfirmedFailure/NotAttempted disposition (api/scoped_admission.rs:542-552, api/actions.rs:1286-1331). The interactive approval path records it once because it discards the executor outcome (pipeline/post_approval.rs:80). Result: duplicate owner-digest rows and path-dependent audit vocabulary for one failure; a pre-effect NotAttempted refusal (recipient changed) is also mis-filed as a Connector-class failure.

Make the record-once decision key off the typed disposition, the same way settlement already does: the executor self-audits and self-batches for its own dispositions, so the mediation error handler must not re-audit/re-batch a scoped effect whose executor owns its record.

Acceptance criteria

• One failed scoped draft produces exactly one owner-digest row and one audit row; parity test: scoped ConfirmedFailure and interactive-approval ConfirmedFailure produce the same digest/audit count.
• Digest-row and audit-row count assertions added to scoped_admission_outcome_tests.rs::failure_after_attempt_cancels_reservation plus a NotAttempted (target-mutated) case pinning the record shape.
• Pre-effect NotAttempted refusals are not batched as Connector-class failures.

Blocked by

Nothing (coordinate with #216's settlement-seam landing — same surface).

Original report

Effect Truth architecture-review checkpoint (run_5eb4c103537d), finding F1/F2; serves the Auditor + owner digest accuracy per the DIRECTION.md decision test.

[coderabbitai]

Coding Plan

Summary

- Carry one additive boolean marker on DispatchedEffect that states the executor already recorded the effect, instead of adding a DispatchError variant.

• Set the marker true for every disposition the scoped executor returned, and false for the generic path, the registry-miss case, and the executor-error case.
• Make the mediation error handler return early when the marker is set, so it does not re-append action.dispatch_failed or re-call batch_failure. Settlement stays keyed off disposition alone.
• Add digest-row and audit-row count assertions, a NotAttempted target-mutated case, and a scoped-versus-interactive parity test to pin the record-once shape.

Design Choices

Design Choice 1: Mechanism to mark an executor-owned disposition so the mediation handler skips re-recording

Options Considered:

1. Add a new DispatchError variant (for example ScopedConnectorHandled) with an early-return carve-out, mirroring ConnectorUnavailable. DispatchError is matched exhaustively in several places (generic_effect_disposition, two digest-classification matches with unreachable!() arms, and multiple test files), so a new variant forces edits across all of them and risks changing existing Connector/NoExecutor behavior.
2. Add a boolean field to DispatchedEffect (for example executor_recorded: bool). Both producer functions already build DispatchedEffect field-by-field, so the change is additive. Only the two producers and one new conditional in the single consumer change. The DispatchError variant space and its exhaustive matches stay unchanged.

Chosen Option: 2

Rationale: A boolean field on DispatchedEffect is additive, local, and preserves the existing disposition/result separation and all DispatchError matches.

Design Choice 2: Scope of the skip for scoped-path dispositions other than `ConfirmedFailure`/`NotAttempted`

Options Considered:

1. Skip re-recording only for ConfirmedFailure/NotAttempted.
2. Skip re-recording for every disposition the scoped executor returned itself (all Ok(...) arms), and keep re-recording only for the executor-error arm (Err(err) mapped to Resource) and the registry-miss (NoExecutor) arm, where no executor recorded anything.

Chosen Option: 2

Rationale: The executor owns its record for every disposition it returns, so the mediation handler must not re-record any of them. The mediation handler must still record the executor-error and registry-miss cases, where no executor produced a record.

💡 User Tips

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

Implementation Steps

Phase 1: Route the record-once decision through the typed disposition

This phase carries a single durable signal from the dispatch boundary to the mediation error handler. The signal states whether the executor already produced the audit and digest record for this effect. The mediation handler then skips its generic action.dispatch_failed audit and batch_failure call when the executor owns the record. This removes the duplicate digest row and the path-dependent audit vocabulary. It stops mis-filing a pre-effect NotAttempted refusal as a Connector-class failure.

Task 1: Add an executor-owned-record marker to `DispatchedEffect`

Add a third signal to the shared dispatch outcome struct.

• Update crates/openspine-kernel/src/api/actions.rs to add a pub(crate) boolean field to DispatchedEffect (for example executor_recorded).
• Extend the struct doc-comment to state that the field means the executor already appended its own audit and, if applicable, self-batched the failure, so callers must not re-record it.
• Do not add any DispatchError variant. Keep the disposition/result fields unchanged.

Task 2: Set the marker at the scoped dispatch boundary

Mark every disposition the scoped executor returned as executor-owned.

• Update dispatch_scoped_effect in crates/openspine-kernel/src/api/scoped_admission.rs.
• Set the marker to true for all Ok(EffectDisposition::...) arms (ConfirmedSuccess, DeliveryUnknown, NotAttempted, ConfirmedFailure), because the shared executor self-audited each of these.
• Set the marker to false for the registry-miss arm (NoExecutor) and for the executor-error arm (Err(err) mapped to DispatchError::Resource), because no executor produced a record in these cases.
• Keep the existing disposition and result mapping unchanged; settlement behavior must not change.

Task 3: Set the marker false on the generic dispatch path

Preserve existing audit and batch behavior for non-scoped dispatch.

• Update dispatch_allowed_action in crates/openspine-kernel/src/api/connector_breaker.rs.
• Set the marker to false in the returned DispatchedEffect, so generic connector, NoExecutor, BadRequest, and Resource errors keep their current action.dispatch_failed audit and batch_failure behavior.

Task 4: Skip re-recording in the mediation error handler

Make the mediation handler honor the executor-owned-record marker.

• Update the error-handling block in mediate_and_dispatch_action_with_attribution_and_token in crates/openspine-kernel/src/api/actions.rs.
• Add a conditional next to the existing DispatchError::ConnectorUnavailable early-return: if the marker is set, return Err(err) without appending action.dispatch_failed and without calling batch_failure.
• Keep settle_reservations running before this block, unchanged, because settlement already keys off disposition alone.
• Keep the digest_class/digest_summary matches on DispatchError unchanged, because the skipped path returns before reaching them.

🤖 Prompt for AI agents

Implement the record-once decision by carrying a typed signal from the dispatch
boundary to the mediation error handler in
`crates/openspine-kernel/src/api/actions.rs`.

- In `actions.rs`, add a `pub(crate)` boolean field (for example
`executor_recorded`) to the `DispatchedEffect` struct. Update the struct
doc-comment to state that this field means the executor already appended its own
audit and, if applicable, self-batched the failure, so callers must not
re-record it. Do not add any new `DispatchError` variant. Keep `disposition` and
`result` fields unchanged.

- In `dispatch_scoped_effect` in
`crates/openspine-kernel/src/api/scoped_admission.rs`, set the new marker to
`true` for all `Ok(EffectDisposition::...)` arms (`ConfirmedSuccess`,
`DeliveryUnknown`, `NotAttempted`, `ConfirmedFailure`), because the shared
executor self-audited each of these. Set the marker to `false` for the
`NoExecutor` arm and for the `Err(err)` arm mapped to `DispatchError::Resource`,
because no executor produced a record in these cases. Do not change the existing
`disposition` and `result` mapping.

- In `dispatch_allowed_action` in
`crates/openspine-kernel/src/api/connector_breaker.rs`, set the marker to
`false` in the returned `DispatchedEffect` so generic connector, `NoExecutor`,
`BadRequest`, and `Resource` errors keep their current `action.dispatch_failed`
audit and `batch_failure` behavior.

- In the error-handling block of
`mediate_and_dispatch_action_with_attribution_and_token` in `actions.rs`, add a
conditional next to the existing `DispatchError::ConnectorUnavailable`
early-return: if the marker is set, return `Err(err)` without appending
`action.dispatch_failed` and without calling `batch_failure`. Keep
`settle_reservations` running before this block unchanged. Keep the
`digest_class`/`digest_summary` matches on `DispatchError` unchanged, since the
skipped path returns before reaching them.

Phase 2: Pin the record shape with tests

This phase adds count assertions that pin the record-once behavior and the correct audit vocabulary. It also adds a parity assertion between the scoped path and the interactive-approval path for a ConfirmedFailure.

Task 1: Add a digest-count helper for tests

Provide a count helper consistent with existing helpers.

• Add a small digest_count(state) helper to crates/openspine-kernel/src/api/scoped_admission_support.rs, styled like the existing audit_count, that returns state.store.owner_digest_items().unwrap().len().
• Use this helper in the tests below. Reuse the existing audit_count helper for audit-row assertions.

Task 2: Extend `failure_after_attempt_cancels_reservation` with record-count assertions

Pin the single-record shape for a scoped ConfirmedFailure.

• Update failure_after_attempt_cancels_reservation in crates/openspine-kernel/src/api/scoped_admission_outcome_tests.rs.
• Keep the existing reservation and pending-write-fence assertions.
• Add: exactly one draft.creation_failed audit row (audit_count(&env.state, "draft.creation_failed") == 1).
• Add: zero action.dispatch_failed audit rows (audit_count(&env.state, "action.dispatch_failed") == 0).
• Add: exactly one owner-digest row (digest_count(&env.state) == 1).

Task 3: Add a NotAttempted target-mutated record-shape test

Pin that a pre-effect refusal is audited with the correct vocabulary and is not batched as Connector-class.

• Extend the existing pre_effect_refusal_cancels_reservation_without_writing test in crates/openspine-kernel/src/api/scoped_admission_outcome_tests.rs, or add a sibling test that reuses its two-stage thread-fetch mock fixture (up_to_n_times(2) returning alice@example.com, then an unconditional mock returning bob@example.com).
• Keep the existing drafts_written == 0, reservation, and fence assertions.
• Add: exactly one draft.target_mutated_since_approval audit row.
• Add: zero action.dispatch_failed audit rows.
• Add: zero owner-digest rows (digest_count(&env.state) == 0), because a target-mutation refusal is not surfaced as a Connector-class digest failure.

Task 4: Add a parity test between scoped and interactive-approval ConfirmedFailure

Prove both paths produce the same record shape for one failed draft.

• Add a parity test that drives a scoped ConfirmedFailure and an interactive-approval ConfirmedFailure against the same HTTP 400 draft response.
• Assert both paths produce the same digest-row count (one) and the same audit vocabulary (one draft.creation_failed, zero action.dispatch_failed).
• Place the interactive-approval side using the create_approved_draft executor path via pipeline/post_approval.rs, or drive the executor directly with the shared fixtures if a full interactive path is not reachable from this test module; keep the comparison focused on digest-row and audit-row counts.

🤖 Prompt for AI agents

Add tests that pin the record-once behavior and audit vocabulary from Phase 1,
using `crates/openspine-kernel/src/api/scoped_admission_outcome_tests.rs` and
`crates/openspine-kernel/src/api/scoped_admission_support.rs`.

- In `scoped_admission_support.rs`, add a `digest_count(state)` helper styled
like the existing `audit_count` helper, returning
`state.store.owner_digest_items().unwrap().len()`. Reuse the existing
`audit_count` helper for audit-row assertions in the tests below.

- Update `failure_after_attempt_cancels_reservation` in
`scoped_admission_outcome_tests.rs`: keep the existing reservation and
pending-write-fence assertions, and add assertions for exactly one
`draft.creation_failed` audit row (`audit_count(&env.state,
"draft.creation_failed") == 1`), zero `action.dispatch_failed` audit rows
(`audit_count(&env.state, "action.dispatch_failed") == 0`), and exactly one
owner-digest row (`digest_count(&env.state) == 1`).

- Add a NotAttempted target-mutated test: extend
`pre_effect_refusal_cancels_reservation_without_writing`, or add a sibling test
reusing its two-stage thread-fetch mock fixture (`up_to_n_times(2)` returning
`alice@example.com`, then an unconditional mock returning `bob@example.com`).
Keep the existing `drafts_written == 0`, reservation, and fence assertions. Add
assertions for exactly one `draft.target_mutated_since_approval` audit row, zero
`action.dispatch_failed` audit rows, and zero owner-digest rows
(`digest_count(&env.state) == 0`).

- Add a parity test that drives a scoped `ConfirmedFailure` and an
interactive-approval `ConfirmedFailure` against the same HTTP 400 draft
response. Assert both paths produce the same digest-row count (one) and the same
audit vocabulary (one `draft.creation_failed`, zero `action.dispatch_failed`).
Implement the interactive-approval side using the `create_approved_draft`
executor path via `pipeline/post_approval.rs`, or drive the executor directly
with the shared fixtures if a full interactive path is not reachable from this
test module. Keep the comparison focused on digest-row and audit-row counts.

Research

The kernel dispatches effects through one shared boundary. mediate_and_dispatch_action* in crates/openspine-kernel/src/api/actions.rs forks between dispatch_scoped_effect (scope-matched standing rule) in api/scoped_admission.rs and dispatch_allowed_action (generic path) in api/connector_breaker.rs. Both return one shared DispatchedEffect { disposition, result } struct defined in actions.rs.

Settlement (settle_reservations) keys off disposition alone. After settlement, a generic error block in actions.rs appends action.dispatch_failed and calls batch_failure for any Err(result), with one existing carve-out: DispatchError::ConnectorUnavailable returns early because the breaker helper already recorded it.

The shared gmail.create_draft executor create_approved_draft in pipeline/approval_draft.rs self-audits every disposition and self-batches connector/resource failures. The interactive approval path in pipeline/post_approval.rs discards the executor outcome, so it records the failure once.

Tests live in api/scoped_admission_outcome_tests.rs with helpers in api/scoped_admission_support.rs; digest rows read back via state.store.owner_digest_items().

---

🚀 Next Steps

🤖 All AI agent prompts combined

Task: 1

Implement the record-once decision by carrying a typed signal from the dispatch
boundary to the mediation error handler in
`crates/openspine-kernel/src/api/actions.rs`.

- In `actions.rs`, add a `pub(crate)` boolean field (for example
`executor_recorded`) to the `DispatchedEffect` struct. Update the struct
doc-comment to state that this field means the executor already appended its own
audit and, if applicable, self-batched the failure, so callers must not
re-record it. Do not add any new `DispatchError` variant. Keep `disposition` and
`result` fields unchanged.

- In `dispatch_scoped_effect` in
`crates/openspine-kernel/src/api/scoped_admission.rs`, set the new marker to
`true` for all `Ok(EffectDisposition::...)` arms (`ConfirmedSuccess`,
`DeliveryUnknown`, `NotAttempted`, `ConfirmedFailure`), because the shared
executor self-audited each of these. Set the marker to `false` for the
`NoExecutor` arm and for the `Err(err)` arm mapped to `DispatchError::Resource`,
because no executor produced a record in these cases. Do not change the existing
`disposition` and `result` mapping.

- In `dispatch_allowed_action` in
`crates/openspine-kernel/src/api/connector_breaker.rs`, set the marker to
`false` in the returned `DispatchedEffect` so generic connector, `NoExecutor`,
`BadRequest`, and `Resource` errors keep their current `action.dispatch_failed`
audit and `batch_failure` behavior.

- In the error-handling block of
`mediate_and_dispatch_action_with_attribution_and_token` in `actions.rs`, add a
conditional next to the existing `DispatchError::ConnectorUnavailable`
early-return: if the marker is set, return `Err(err)` without appending
`action.dispatch_failed` and without calling `batch_failure`. Keep
`settle_reservations` running before this block unchanged. Keep the
`digest_class`/`digest_summary` matches on `DispatchError` unchanged, since the
skipped path returns before reaching them.
===============================================================================

Task: 2

Add tests that pin the record-once behavior and audit vocabulary from Phase 1,
using `crates/openspine-kernel/src/api/scoped_admission_outcome_tests.rs` and
`crates/openspine-kernel/src/api/scoped_admission_support.rs`.

- In `scoped_admission_support.rs`, add a `digest_count(state)` helper styled
like the existing `audit_count` helper, returning
`state.store.owner_digest_items().unwrap().len()`. Reuse the existing
`audit_count` helper for audit-row assertions in the tests below.

- Update `failure_after_attempt_cancels_reservation` in
`scoped_admission_outcome_tests.rs`: keep the existing reservation and
pending-write-fence assertions, and add assertions for exactly one
`draft.creation_failed` audit row (`audit_count(&env.state,
"draft.creation_failed") == 1`), zero `action.dispatch_failed` audit rows
(`audit_count(&env.state, "action.dispatch_failed") == 0`), and exactly one
owner-digest row (`digest_count(&env.state) == 1`).

- Add a NotAttempted target-mutated test: extend
`pre_effect_refusal_cancels_reservation_without_writing`, or add a sibling test
reusing its two-stage thread-fetch mock fixture (`up_to_n_times(2)` returning
`alice@example.com`, then an unconditional mock returning `bob@example.com`).
Keep the existing `drafts_written == 0`, reservation, and fence assertions. Add
assertions for exactly one `draft.target_mutated_since_approval` audit row, zero
`action.dispatch_failed` audit rows, and zero owner-digest rows
(`digest_count(&env.state) == 0`).

- Add a parity test that drives a scoped `ConfirmedFailure` and an
interactive-approval `ConfirmedFailure` against the same HTTP 400 draft
response. Assert both paths produce the same digest-row count (one) and the same
audit vocabulary (one `draft.creation_failed`, zero `action.dispatch_failed`).
Implement the interactive-approval side using the `create_approved_draft`
executor path via `pipeline/post_approval.rs`, or drive the executor directly
with the shared fixtures if a full interactive path is not reachable from this
test module. Keep the comparison focused on digest-row and audit-row counts.

💡 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!