From d3583fce68f90685ceff7b7d35a994334eb2ebd8 Mon Sep 17 00:00:00 2001 From: David Abram Date: Sun, 30 Aug 2026 15:30:10 +0200 Subject: [PATCH 1/8] context: Add plan for external mutation cursor taint Define the crash-safety boundary for mutation-cursor coordination before runtime implementation. The plan covers a worktree-scoped write-ahead marker, inherited-taint recovery, failure handling, and restart/integration validation while preserving existing DB protocol semantics. Plan: mutation-cursor-external-taint; Tasks: T01-T04 Co-authored-by: SCE --- .../plans/mutation-cursor-external-taint.md | 402 ++++++++++++++++++ 1 file changed, 402 insertions(+) create mode 100644 context/plans/mutation-cursor-external-taint.md diff --git a/context/plans/mutation-cursor-external-taint.md b/context/plans/mutation-cursor-external-taint.md new file mode 100644 index 00000000..9f06422b --- /dev/null +++ b/context/plans/mutation-cursor-external-taint.md @@ -0,0 +1,402 @@ +# Plan: mutation-cursor-external-taint + +## Change summary + +Add the external durability boundary for the mutation cursor: a worktree-local +filesystem marker at `/sce/mutation-cursor-tainted`, armed write-ahead +at the start of mutation-cursor boundary processing, that a later invocation +reads as the external signal that the previous invocation never proved a +trustworthy durable completion. This is the concrete runtime refinement of the +already-verified abstract `ProtocolState.external_taint` / `databaseFailure` / +`recover` semantics — it changes no protocol semantics, adds no database state, +and requires no migration. + +The fence must begin at the mutation-cursor runtime boundary, **before** +repository Agent Trace DB acquisition — not after an already-open +`RepositoryAgentTraceDb` has been handed in. The current +`coordinate(repository_root, &db, boundary)` signature arms the fence too late: +if the hook runtime's DB open fails, `coordinate()` is never entered, no marker +is armed, and a later invocation that opens the DB successfully sees no +inherited marker and can treat a lost `A → C` interval as trustworthy evidence. +This plan reshapes the outer `coordinate()` entrypoint to own the whole +protected operation: it resolves `git_dir`, acquires the `WorktreeLock`, arms +the marker, and only then acquires the DB (through a caller-supplied provider), +runs the snapshot / recovery / protocol / CAS pipeline, and clears the marker +only on complete success. The lower-level `coordinate_boundary` / +`coordinate_with_db` pipeline (the #244 snapshot/protocol/store logic) still +receives an already-open `&RepositoryAgentTraceDb` internally; +`MutationTraceStore` and `protocol.rs` never open or resolve a DB. + +Target ordering the plan establishes: + +```text +RuntimeBoundary + → resolve git_dir + → acquire WorktreeLock + → inspect inherited marker + → persist marker + → get/create checkout ID (WorktreeId) + → open RepositoryAgentTraceDb (caller-supplied provider) + → capture + pin one snapshot + → if inherited taint: overlay database_failure, recover against that snapshot + → prepare / commit the triggering boundary + → DB CAS + → complete success + → clear marker +``` + +Safety invariant: **no failure after a mutation-cursor boundary enters its +protected runtime section — including failure to open the Agent Trace DB +itself, or to resolve checkout identity — can disappear without leaving a +worktree-local external-taint signal for the next invocation.** + +All work is confined to `cli/src/services/mutation_trace/runtime/` (a new +`external_taint.rs` primitive, entrypoint reshaping plus arming and +inherited-taint recovery wiring in `coordinator.rs`, and updated integration +tests in `runtime/tests.rs`) and the durable context/spec docs that describe +the coordinator. It extends the existing `mutation-cursor-runtime-coordinator` +work and preserves that coordinator's current lock, snapshot, DB-backed +`SnapshotFailure`, and CAS-retry behavior. No harness or command wiring is part +of this plan, and `coordinate()` stays reachable only from within `runtime`. + +## Acceptance criteria + +How this plan is proven complete. Each criterion is observable and names the +check that proves it. `/validate` runs these checks; no task in the stack +performs final validation. + +- [ ] AC1: A marker is stored beneath the worktree-specific Git directory + (`/sce/mutation-cursor-tainted`), survives reconstruction of the + runtime service, and two linked worktrees resolve to two different marker + paths. + - Validate: `runtime::external_taint::tests::marker_is_worktree_scoped` and + `runtime::external_taint::tests::marker_persists_until_explicitly_cleared` +- [ ] AC2: `persist` then `persist` then `clear` then `clear` all succeed; + `clear` on an absent marker is success. + - Validate: `runtime::external_taint::tests::persist_and_clear_are_idempotent` +- [ ] AC3: A public `coordinate()` call arms its marker internally and clears it + only after a successful `CoordinateOutcome`. + - Validate: `runtime::tests::successful_coordinate_clears_external_taint_marker` +- [ ] AC4: After marker arming, any coordinator error leaves the marker present — + proven for the snapshot-failure path and at least one deterministic + non-snapshot failure path. + - Validate: `runtime::tests::snapshot_failure_leaves_external_taint_marker` + plus one deterministic non-snapshot failure test +- [ ] AC5: Given durable cursor A, an active scope S, an inherited marker, and a + current worktree C, the next invocation rebaselines to C, emits no A→C + mutation evidence, abandons S, then processes its triggering boundary. + - Validate: `runtime::tests::inherited_external_taint_recovers_before_boundary` +- [ ] AC6: External-taint recovery and triggering-boundary processing use the + same `observed_tree`; no second Git snapshot occurs. + - Validate: coordinator unit test with a call-counting `SnapshotCapture` + asserting exactly one `capture` +- [ ] AC7: A losing recovery CAS re-injects external taint on reload and + recomputes recovery until `Applied` or retry exhaustion; the marker remains + present throughout the retry loop. + - Validate: coordinator unit test racing a recovery CAS conflict and asserting + re-injection +- [ ] AC8: A failure injected after recovery has committed but before the + triggering boundary completes leaves recovery durable, the boundary + incomplete, and the marker still present; a later invocation recovers + conservatively again. + - Validate: `runtime::tests` marker-clear/boundary-incomplete scenario +- [ ] AC9: A marker that survives an invocation which never materialized a + worktree row causes the next successful invocation to establish a baseline + with no evidence for the unknown interval. + - Validate: coordinator unit test plus `runtime::tests` first-ever-failure + scenario +- [ ] AC10: A worktree with live scopes and inherited external taint persists + those scopes as `Abandoned` during recovery and never treats them as eligible + exclusive attribution after the unknown interval. + - Validate: coordinator unit test asserting `Abandoned` scope status after + inherited-taint recovery +- [ ] AC11: End to end, no `MutationEvent` treats an interval spanning an + incomplete/failed invocation (baseline A, scope start, edit→B, failed + invocation, edit→C, next successful invocation) as one trustworthy + AI-attributable A/B→C interval. + - Validate: `runtime::tests` end-to-end no-evidence-across-gap scenario +- [ ] AC12: An external-taint marker in linked worktree A does not trigger + recovery in linked worktree B, even with a shared repository Agent Trace DB. + - Validate: `runtime::tests` linked-worktree independence scenario +- [ ] AC13: When marker inspection or marker persistence cannot be established, + `coordinate()` returns a distinct external-taint marker error before any + checkout-identity, DB, snapshot, or protocol processing. + - Validate: coordinator unit test injecting marker inspect/persist I/O failure +- [ ] AC14: DB acquisition is inside the external-taint fence. Once + mutation-cursor boundary processing has acquired the `WorktreeLock` and armed + the marker, a failure to resolve or open the repository Agent Trace DB (the + caller-supplied provider returns `Err`) makes `coordinate()` return an error + with the marker still present, even though no `RepositoryAgentTraceDb` was + ever available and the lower-level coordinator pipeline was never entered. The + follow-up invocation, given a working DB provider, snapshots the current tree, + runs external-taint recovery, and produces no evidence across the lost + interval. + - Validate: `runtime::tests` DB-provider-returns-`Err` scenario (marker armed + → provider `Err` → error returned → marker still present) plus its + follow-up-invocation recovery assertion; and a coordinator unit test with a + fake failing DB provider + +### Full validation + +Repository-wide checks `/validate` runs after the last task, regardless of +which criterion they map to. + +- `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml services::mutation_trace::runtime::` +- `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml` +- `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml --all-targets -- -D warnings` +- `./scripts/run-cli-cargo.sh fmt --manifest-path cli/Cargo.toml -- --check` +- `nix flake check` +- `nix run .#pkl-check-generated` +- Confirm the existing Quint Connect model-based-testing harness + (`checks.cli-tests` / `checks.mutation-trace-quint-connect`) stays green; + no `spec/mutation_cursor.qnt` behavior change is expected. + +### Context sync + +- `context/cli/mutation-trace-runtime-coordinator.md` — document the + `ExternalTaintMarker` primitive and its worktree-scoped path; the reshaped + `coordinate()` entrypoint that owns `WorktreeLock`, marker arming, the + caller-supplied DB provider, the snapshot/pipeline, and marker clear; the + arm-before-DB-acquisition ordering and the safety invariant; the + inherited-vs-armed distinction and the invocation-local `external_taint_pending` + overlay onto `database_failure`; the new marker-I/O and + DB-provider-failure `CoordinateError` variants and their fail-closed + semantics; and the added `/sce/` on-disk-layout entry. +- `context/cli/mutation-trace-protocol.md` — note that the concrete runtime + refinement of `ProtocolState.external_taint` is the stale worktree-local + marker, armed write-ahead before DB acquisition and promoted to protocol + external taint only when inherited by a later invocation. +- `context/context-map.md` — refresh the coordinator domain-file annotation. +- `spec/mutation_cursor.md` — record that the abstract `externalTaint` marker's + concrete refinement is `/sce/mutation-cursor-tainted`, armed + write-ahead at the start of the protected runtime section (before Agent Trace + DB acquisition) and becoming protocol external taint only when inherited. +- Verify-only pass over `context/overview.md`, `context/architecture.md`, + `context/glossary.md`, `context/patterns.md`. + +## Task context synchronization lifecycle + +Persist this field in every plan; this is durable plan state, not chat state: + +- **Task context synchronization:** every task carries `pending | synced | blocked`. + A completed task must be `synced` before another task can start or the plan can + finish. +- For `blocked`, record **Blocker**, **Required action**, and **Retry condition** + beside the status. Never infer `synced` from conversation history; write every + lifecycle transition to the plan file. + +## Constraints and non-goals + +- **In scope:** `cli/src/services/mutation_trace/runtime/external_taint.rs` + (new), `cli/src/services/mutation_trace/runtime/mod.rs`, + `cli/src/services/mutation_trace/runtime/coordinator.rs` (including reshaping + the public `coordinate()` signature and updating its existing call sites in + `runtime/tests.rs`), `cli/src/services/mutation_trace/runtime/tests.rs`; + docs-only edits to `context/cli/mutation-trace-runtime-coordinator.md`, + `context/cli/mutation-trace-protocol.md`, `context/context-map.md`, and + `spec/mutation_cursor.md`. +- **Out of scope:** ref reconciliation; harness adapters and any + hook/command/`diff_traces` wiring; a `pub(crate)` re-export of `coordinate()`; + redesigning Agent Trace storage resolution beyond, at most, a narrow split + that lets the marker be armed before the existing DB-open primitive runs; + changes to `protocol.rs`, `spec/mutation_cursor.qnt`, or the Quint refinement + matrix; any `store.rs` / SQL / migration change; a daemon; cross-machine + locking; snapshot-ref reclamation; new Agent Trace evidence formats. +- **Constraints:** + - No new Cargo dependencies unless an unavoidable platform issue is discovered. + - The DB CAS stays the protocol linearization point. + - The outer `coordinate()` entrypoint owns `WorktreeLock`, + `ExternalTaintMarker`, the DB-provider call, the snapshot service, the + coordinator pipeline, and the marker clear. DB acquisition is supplied to + `coordinate()` as a caller-provided `FnOnce` provider (or equivalent seam), + not resolved by `coordinate()` itself — repository identity and remote come + from config the coordinator does not read. + - `MutationTraceStore` and `protocol.rs` never open or resolve a DB; the + lower-level `coordinate_boundary` / `coordinate_with_db` pipeline still + receives an already-open `&RepositoryAgentTraceDb`. + - The marker is never armed before the `WorktreeLock` is held, and + same-worktree marker state is inspected/persisted/cleared only while that + lock is held. + - The filesystem marker is never authoritative for normal cursor state. + - No RAII/`Drop`-based marker deletion; only a successful `CoordinateOutcome` + clears the marker. + - `WorktreeProjection::into_protocol_state()` still returns an empty + `external_taint`; the filesystem overlay is applied by runtime code only. + - Marker durability follows the existing + `checkout::persist_checkout_id_inner` style (`fsync` the marker file, + best-effort `#[cfg(unix)]` parent-directory `sync_all`). +- **Non-goal:** introducing a new protocol `FailureKind`; making + `WorktreeProjection::into_protocol_state()` read filesystem state; opening the + DB before arming the marker (that would leave DB-open failure uncovered, which + is the specific failure this plan exists to close); making the protocol or + store layer responsible for opening or resolving the DB; broadening the + durability claim to host power loss / filesystem crash. + +## Assumptions + +The user's change request states "exact names may change", "the exact type is +flexible", and "this exact API is NOT mandatory"; the following are recorded +local choices, not new requirements. + +- `coordinate()` is reshaped to + `coordinate(repository_root: &Path, boundary: &RuntimeBoundary, open_db: impl FnOnce() -> anyhow::Result) -> Result`. + The private `coordinate_inner(.., on_lock_contention, open_db)` test seam is + kept. The lower-level pipeline function is renamed/kept as + `coordinate_boundary` (or `coordinate_with_db`) taking `&RepositoryAgentTraceDb` + plus `inherited_external_taint`. A future harness adapter builds the provider + closure around `agent_trace_storage::resolve_agent_trace_storage_for_hook_runtime` + and hands `coordinate()` the resulting `.db`. +- Marker I/O failures surface as a dedicated `CoordinateError` variant, e.g. + `ExternalTaintMarker { operation: ExternalTaintOperation, source: anyhow::Error }` + with `ExternalTaintOperation { Inspect, Persist, Clear }`. A DB-provider + failure after the marker is armed surfaces as a separate variant, e.g. + `CoordinateError::AgentTraceDbUnavailable(anyhow::Error)`, and intentionally + leaves the marker in place. +- The new primitive lives at `runtime/external_taint.rs` as + `ExternalTaintMarker`, backed by an empty file at + `/sce/mutation-cursor-tainted`; its existence is the entire state. +- Marker durability protects against process error, non-graceful process exit, + `SIGKILL`, and normal runtime restart: `persist()` creates and `fsync`s the + marker file, `clear()` removes it, and both do a best-effort `#[cfg(unix)]` + parent-directory `sync_all` whose error is not propagated (mirroring + `checkout::persist_checkout_id_inner`). The plan does not claim durability + across host power loss or a filesystem-level crash, because that + parent-directory sync is best-effort. +- Test module and function names follow the paths named in the acceptance + criteria (`runtime::external_taint::tests::*`, `runtime::tests::*`). + +## Task stack + +- [ ] T01: `Add the external-taint marker primitive` (status:todo) + - Task ID: T01 + - Scope: In — new `cli/src/services/mutation_trace/runtime/external_taint.rs` + defining `ExternalTaintMarker` with `new(git_dir)`, `exists()`, `persist()`, + `clear()`; empty marker file at `/sce/mutation-cursor-tainted`; + checkout-identity-style durability (`fsync` the file on create, best-effort + `#[cfg(unix)]` parent-dir `sync_all` on create and remove); `NotFound` on + clear treated as success; no `Drop` deletion; registration in + `runtime/mod.rs`. Out — any `coordinator.rs` change, any protocol/store/ + error-type change, harness wiring. + - Dependencies: none + - Done when: the type compiles and is registered under the module's existing + `#[allow(dead_code)]` precedent; `persist`/`clear` are idempotent; the path + is derived from the caller-supplied worktree `git_dir`; inline + `#[cfg(test)] mod tests` (unique `std::env::temp_dir()` paths, per + `context/patterns.md`) covers persistence across marker-value reconstruction, + idempotent persist, idempotent clear, and two `git_dir`s resolving to + independent marker paths. + - Verify: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml services::mutation_trace::runtime::external_taint`; + `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml --all-targets -- -D warnings`; + `./scripts/run-cli-cargo.sh fmt --manifest-path cli/Cargo.toml -- --check`. + - Context synchronization: pending + +- [ ] T02: `Arm the write-ahead fence around the full runtime boundary, DB acquisition included` (status:todo) + - Task ID: T02 + - Scope: In — `runtime/coordinator.rs`: reshape the public `coordinate()` so + it no longer receives an already-open `&RepositoryAgentTraceDb` but a + caller-supplied DB provider (`impl FnOnce() -> anyhow::Result` + or equivalent seam); reorder the outer path to resolve `git_dir` → acquire + `WorktreeLock` → construct `ExternalTaintMarker` → read + `inherited_external_taint = marker.exists()?` → `marker.persist()?` → + `get_or_create_checkout_id` (`WorktreeId`) → invoke the DB provider → + construct `GitSnapshotService` → run the lower-level pipeline + (`coordinate_boundary` / `coordinate_with_db`, taking `&RepositoryAgentTraceDb` + plus `inherited_external_taint`, unused until T03) → `marker.clear()?` only + on `Ok`, leaving the marker on every `Err`; add a distinct marker-I/O + `CoordinateError` variant (inspect/persist failure returns before checkout, + DB, snapshot, or protocol work; clear failure after a successful boundary + returns `Err` with the marker left in place) and a distinct + DB-provider-failure variant that also leaves the marker; update the existing + `coordinate()` call sites in `runtime/tests.rs` to pass a provider closure. + Out — mapping inherited taint into recovery (T03); new integration scenarios + (T04). + - Dependencies: T01 + - Done when: every `coordinate()` call arms the marker after `WorktreeLock` + acquisition and before checkout-identity, DB-provider, snapshot, and + protocol work; a successful `CoordinateOutcome` clears it; every path after + arming leaves it present — snapshot failure, DB provider returning `Err`, + checkout-identity failure, DB read/write failure, CAS exhaustion, + scope-identity conflict, unexpected error; marker inspect/persist failure + returns the new marker error before any of that work; the private + `coordinate_inner` test seam and `on_lock_contention` closure are preserved; + `coordinator.rs` inline tests prove success→cleared, the snapshot-failure + path and one deterministic non-snapshot failure path both retaining the + marker, the fail-closed inspect/persist error path, and a fake DB provider + returning `Err` after arming leaving the marker present. + - Verify: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml services::mutation_trace::runtime::coordinator`; + `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml services::mutation_trace::runtime::`; + `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml --all-targets -- -D warnings`; + `./scripts/run-cli-cargo.sh fmt --manifest-path cli/Cargo.toml -- --check`. + - Context synchronization: pending + +- [ ] T03: `Map inherited marker into protocol recovery` (status:todo) + - Task ID: T03 + - Scope: In — `runtime/coordinator.rs`: invocation-local + `external_taint_pending`, seeded from `inherited_external_taint` and threaded + into the lower-level pipeline; when set, overlay `protocol::database_failure` + onto each freshly loaded projection before the existing `needs_recovery` + check, so `protocol::recover` runs against the single already-captured + `observed_tree`; keep `external_taint_pending` set across a recovery-CAS + `Conflict` and clear the in-memory flag only on recovery-CAS `Applied`; + never touch the filesystem marker here (T02's success path owns clearing); + first-ever inherited marker with no durable worktree row initializes the + worktree against the observed tree, then recovers. Out — filesystem marker + writes; new integration scenarios (T04). + - Dependencies: T02 + - Done when: an inherited marker forces exactly one recovery transition + (cursor := observed tree, revision += 1, `tainted`/external taint cleared, + active scopes → `Abandoned`, no `MutationEvent` for the skipped interval) + before the triggering boundary is processed against the recovered state; + recovery and boundary share one snapshot (call-counting `SnapshotCapture` + proves a single `capture`); a recovery-CAS conflict re-injects + `database_failure` on reload and recomputes; after recovery-CAS `Applied` + the flag is clear so a later boundary-CAS retry in the same invocation does + not re-trigger recovery; a first-ever inherited marker with no worktree row + produces no evidence for the unknown interval; `coordinator.rs` inline tests + cover each case. + - Verify: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml services::mutation_trace::runtime::coordinator`; + `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml --all-targets -- -D warnings`; + `./scripts/run-cli-cargo.sh fmt --manifest-path cli/Cargo.toml -- --check`. + - Context synchronization: pending + +- [ ] T04: `Add restart/failure integration tests through coordinate()` (status:todo) + - Task ID: T04 + - Scope: In — `runtime/tests.rs` cross-module tests driving only the public + `coordinate()` API against real `git init` / `git worktree add` repositories + and real temp-file `RepositoryAgentTraceDb`s (unique-temp-path precedent), + with the DB passed through the provider closure: successful invocation + leaves no marker; a failed invocation leaves the marker; a **DB provider + returning `Err` after the marker is armed** leaves the marker present and + the follow-up invocation (working provider) rebaselines to the current tree + with no evidence across the lost interval; a stale marker on the next + invocation rebaselines to the current tree, emits no evidence across the + gap, abandons prior live scopes, then processes its boundary; a first-ever + failed invocation that never materialized a worktree row cannot create + evidence; two linked worktrees keep independent markers over one shared DB + (a marker in A does not recover B); snapshot-failure interaction leaves both + a durable `SnapshotFailure` and the marker, and the next invocation performs + a single conservative recovery; a marker-clear failure after a durable + boundary keeps the marker for a later conservative re-recovery. Out — any + production-code change. + - Dependencies: T03 + - Done when: the listed scenarios pass through the public entrypoint only, + including the end-to-end "trusted A → failed/incomplete invocation (DB-open + failure included) → filesystem changes → next successful invocation + rebaselines to C with no A/B→C evidence" story; `runtime::tests` and the + full CLI test suite pass. + - Verify: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml services::mutation_trace::runtime::tests`; + `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml`. + - Context synchronization: pending + +## Open questions + +None. The entry-contract question raised in the prior draft is resolved by this +plan: PR #245 establishes an outer mutation-cursor runtime boundary that arms +external taint before Agent Trace DB acquisition, and future harness adapters +must call that protected boundary (`harness adapter → coordinate() → marker → +DB → snapshot / protocol / CAS`) rather than opening the DB themselves and +calling a lower-level coordinator. Whether a later change also splits +`agent_trace_storage` resolution into "resolve identity/path" and "open DB" +halves is left to implementation — the plan only requires the narrowest seam +that puts DB acquisition inside the fence, and the caller-supplied provider +closure achieves that without touching `agent_trace_storage` at all. From f8936d4b6f40d95d057cb1b4408e0264859e6462 Mon Sep 17 00:00:00 2001 From: David Abram Date: Sun, 30 Aug 2026 16:46:01 +0200 Subject: [PATCH 2/8] runtime: Add durable external-taint marker primitive Establish the worktree-local filesystem boundary needed to detect interrupted mutation-cursor runtime sections before the coordinator wires it into the full boundary. `ExternalTaintMarker` uses an empty `/sce/mutation-cursor-tainted` file whose existence is the state, with synced creation, idempotent clearing, and worktree-scoped paths. Add the runtime context and record plan `mutation-cursor-external-taint` task T01 as complete; coordinator integration remains follow-up work. Co-authored-by: SCE --- .../mutation_trace/runtime/external_taint.rs | 259 ++++++++++++++++++ .../services/mutation_trace/runtime/mod.rs | 1 + context/cli/mutation-trace-external-taint.md | 63 +++++ .../cli/mutation-trace-runtime-coordinator.md | 4 +- context/context-map.md | 1 + .../plans/mutation-cursor-external-taint.md | 44 ++- 6 files changed, 365 insertions(+), 7 deletions(-) create mode 100644 cli/src/services/mutation_trace/runtime/external_taint.rs create mode 100644 context/cli/mutation-trace-external-taint.md diff --git a/cli/src/services/mutation_trace/runtime/external_taint.rs b/cli/src/services/mutation_trace/runtime/external_taint.rs new file mode 100644 index 00000000..9e9ce508 --- /dev/null +++ b/cli/src/services/mutation_trace/runtime/external_taint.rs @@ -0,0 +1,259 @@ +//! External-taint marker primitive for the mutation-cursor runtime boundary. +//! +//! A worktree-local filesystem marker at `/sce/mutation-cursor-tainted`. +//! Its existence is its entire state: armed write-ahead at the start of the +//! protected runtime section, cleared only after a proven durable completion, and +//! read by a later invocation as the external signal that the previous +//! invocation never proved a trustworthy durable completion. +//! +//! Durability mirrors [`crate::services::checkout`]'s `persist_checkout_id_inner`: +//! [`ExternalTaintMarker::persist`] creates and `fsync`s the marker file, and +//! both `persist` and [`ExternalTaintMarker::clear`] do a best-effort +//! `#[cfg(unix)]` parent-directory `sync_all` whose error is not propagated. This +//! protects against process error, non-graceful process exit, `SIGKILL`, and +//! normal runtime restart — not host power loss or a filesystem-level crash. +//! +//! The filesystem marker is never authoritative for normal cursor state, and it +//! is never deleted via `Drop`: only an explicit `clear()` after a successful +//! `CoordinateOutcome` removes it. + +#![allow(dead_code)] + +use std::fs::OpenOptions; +use std::path::{Path, PathBuf}; + +use anyhow::{Context, Result}; + +/// Subdirectory inside `/` where SCE runtime metadata lives. +const SCE_RUNTIME_DIR: &str = "sce"; + +/// File name for the external-taint marker inside `/sce/`. +const MARKER_FILE: &str = "mutation-cursor-tainted"; + +/// Worktree-local external-taint marker. +/// +/// Construct with [`ExternalTaintMarker::new`] from the worktree-specific Git +/// directory (as resolved by [`crate::services::checkout::resolve_git_dir`]). +/// Two linked worktrees resolve to two different Git directories and therefore +/// two independent markers. +#[derive(Debug, Clone)] +pub struct ExternalTaintMarker { + marker_dir: PathBuf, + marker_path: PathBuf, +} + +impl ExternalTaintMarker { + /// Builds the marker handle rooted at `/sce/mutation-cursor-tainted`. + #[must_use] + pub fn new(git_dir: &Path) -> Self { + let marker_dir = git_dir.join(SCE_RUNTIME_DIR); + let marker_path = marker_dir.join(MARKER_FILE); + Self { + marker_dir, + marker_path, + } + } + + /// Returns `true` when the marker file is present. + /// + /// # Errors + /// + /// Returns an error when the marker path cannot be inspected for a reason + /// other than absence. + pub fn exists(&self) -> Result { + match std::fs::symlink_metadata(&self.marker_path) { + Ok(_) => Ok(true), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(false), + Err(err) => Err(err).with_context(|| { + format!( + "Failed to inspect external-taint marker '{}'", + self.marker_path.display() + ) + }), + } + } + + /// Creates and `fsync`s the marker file. + /// + /// Idempotent: a marker that already exists is re-synced and left in place. + /// + /// # Errors + /// + /// Returns an error when the marker directory or file cannot be created or + /// synced. + pub fn persist(&self) -> Result<()> { + std::fs::create_dir_all(&self.marker_dir).with_context(|| { + format!( + "Failed to create external-taint marker directory '{}'", + self.marker_dir.display() + ) + })?; + + let file = OpenOptions::new() + .write(true) + .create(true) + .truncate(false) + .open(&self.marker_path) + .with_context(|| { + format!( + "Failed to create external-taint marker '{}'", + self.marker_path.display() + ) + })?; + file.sync_data().with_context(|| { + format!( + "Failed to sync external-taint marker '{}'", + self.marker_path.display() + ) + })?; + drop(file); + + self.best_effort_sync_marker_dir(); + + Ok(()) + } + + /// Removes the marker file. + /// + /// Idempotent: clearing an absent marker is success. + /// + /// # Errors + /// + /// Returns an error when the marker file exists but cannot be removed. + pub fn clear(&self) -> Result<()> { + match std::fs::remove_file(&self.marker_path) { + Ok(()) => {} + Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(()), + Err(err) => { + return Err(err).with_context(|| { + format!( + "Failed to clear external-taint marker '{}'", + self.marker_path.display() + ) + }); + } + } + + self.best_effort_sync_marker_dir(); + + Ok(()) + } + + #[cfg(unix)] + fn best_effort_sync_marker_dir(&self) { + if let Ok(dir) = std::fs::File::open(&self.marker_dir) { + let _ = dir.sync_all(); + } + } + + #[cfg(not(unix))] + fn best_effort_sync_marker_dir(&self) {} +} + +#[cfg(test)] +mod tests { + use std::sync::atomic::{AtomicU64, Ordering}; + + use super::*; + + static NEXT_TEST_GIT_DIR_ID: AtomicU64 = AtomicU64::new(0); + + fn unique_test_git_dir(label: &str) -> PathBuf { + let id = NEXT_TEST_GIT_DIR_ID.fetch_add(1, Ordering::Relaxed); + std::env::temp_dir().join(format!( + "sce-external-taint-{label}-{}-{id}", + std::process::id() + )) + } + + fn remove_test_git_dir(git_dir: &Path) { + let _ = std::fs::remove_dir_all(git_dir); + } + + #[test] + fn marker_is_worktree_scoped() { + let git_dir_a = unique_test_git_dir("scoped-a"); + let git_dir_b = unique_test_git_dir("scoped-b"); + std::fs::create_dir_all(&git_dir_a).expect("git dir a should be created"); + std::fs::create_dir_all(&git_dir_b).expect("git dir b should be created"); + + let marker_a = ExternalTaintMarker::new(&git_dir_a); + let marker_b = ExternalTaintMarker::new(&git_dir_b); + + assert_ne!( + marker_a.marker_path, marker_b.marker_path, + "each git dir must derive an independent marker path" + ); + + marker_a.persist().expect("marker a should persist"); + + assert!( + marker_a + .exists() + .expect("marker a existence should resolve"), + "the armed marker must be visible in its own worktree" + ); + assert!( + !marker_b + .exists() + .expect("marker b existence should resolve"), + "arming worktree A must not arm worktree B" + ); + + remove_test_git_dir(&git_dir_a); + remove_test_git_dir(&git_dir_b); + } + + #[test] + fn marker_persists_until_explicitly_cleared() { + let git_dir = unique_test_git_dir("persists"); + std::fs::create_dir_all(&git_dir).expect("git dir should be created"); + + ExternalTaintMarker::new(&git_dir) + .persist() + .expect("marker should persist"); + + let reloaded = ExternalTaintMarker::new(&git_dir); + assert!( + reloaded.exists().expect("marker existence should resolve"), + "the marker must survive reconstruction of the handle" + ); + + reloaded.clear().expect("marker should clear"); + assert!( + !ExternalTaintMarker::new(&git_dir) + .exists() + .expect("marker existence should resolve"), + "an explicitly cleared marker must be gone" + ); + + remove_test_git_dir(&git_dir); + } + + #[test] + fn persist_and_clear_are_idempotent() { + let git_dir = unique_test_git_dir("idempotent"); + std::fs::create_dir_all(&git_dir).expect("git dir should be created"); + let marker = ExternalTaintMarker::new(&git_dir); + + marker + .clear() + .expect("clear on absent marker should succeed"); + + marker.persist().expect("first persist should succeed"); + marker.persist().expect("second persist should succeed"); + assert!( + marker.exists().expect("marker existence should resolve"), + "the marker must remain armed after repeated persist" + ); + + marker.clear().expect("first clear should succeed"); + marker.clear().expect("second clear should succeed"); + assert!( + !marker.exists().expect("marker existence should resolve"), + "the marker must remain cleared after repeated clear" + ); + + remove_test_git_dir(&git_dir); + } +} diff --git a/cli/src/services/mutation_trace/runtime/mod.rs b/cli/src/services/mutation_trace/runtime/mod.rs index 794a6e12..703058a6 100644 --- a/cli/src/services/mutation_trace/runtime/mod.rs +++ b/cli/src/services/mutation_trace/runtime/mod.rs @@ -1,4 +1,5 @@ mod coordinator; +mod external_taint; mod git_snapshot; mod worktree_lock; diff --git a/context/cli/mutation-trace-external-taint.md b/context/cli/mutation-trace-external-taint.md new file mode 100644 index 00000000..e1d10f55 --- /dev/null +++ b/context/cli/mutation-trace-external-taint.md @@ -0,0 +1,63 @@ +# Mutation-cursor external-taint boundary (`runtime::external_taint`) + +The worktree-local durability boundary for the mutation cursor: a filesystem +marker that a later hook invocation reads as the external signal that the +previous invocation never proved a trustworthy durable completion. This is the +concrete runtime refinement of the abstract `ProtocolState.external_taint` / +`databaseFailure` / `recover` semantics in +[`mutation-trace-protocol.md`](mutation-trace-protocol.md) — it changes no +protocol semantics, adds no database state, and needs no migration. + +Built by the `mutation-cursor-external-taint` plan +(`context/plans/mutation-cursor-external-taint.md`), which extends the +[`mutation-trace-runtime-coordinator.md`](mutation-trace-runtime-coordinator.md) +work. + +## `ExternalTaintMarker` primitive + +`cli/src/services/mutation_trace/runtime/external_taint.rs` — +`ExternalTaintMarker::new(git_dir: &Path)` builds a handle rooted at +`/sce/mutation-cursor-tainted`, the worktree-specific Git directory as +resolved by `checkout::resolve_git_dir`. Two linked worktrees resolve to two +different Git directories and therefore two independent markers. + +The marker file is empty; **its existence is its entire state**: + +- `exists() -> Result` — `symlink_metadata`; a `NotFound` is `Ok(false)`, + any other inspection failure is `Err`. +- `persist() -> Result<()>` — `create_dir_all` the `sce/` directory, + `create`-open the marker without truncation (`write(true).create(true).truncate(false)`, + since the contents carry no meaning), `sync_data()` it, then a best-effort + `#[cfg(unix)]` parent-directory `sync_all` whose error is not propagated. + Idempotent: an existing marker is re-opened and re-synced, never rewritten. +- `clear() -> Result<()>` — `remove_file`, treating `NotFound` as success, then + the same best-effort parent-directory sync. Idempotent. + +Durability mirrors `checkout::persist_checkout_id_inner`: it protects against +process error, non-graceful process exit, `SIGKILL`, and normal runtime +restart — not host power loss or a filesystem-level crash (the parent-directory +sync is best-effort). The marker is never removed via `Drop`; only an explicit +`clear()` removes it. It is never authoritative for normal cursor state. + +The module carries `#![allow(dead_code)]` (the `services/capabilities.rs` +precedent): nothing arms or reads the marker yet. Wiring it into a reshaped +`coordinate()` entrypoint — armed write-ahead after the `WorktreeLock` is held +and before Agent Trace DB acquisition, cleared only on a successful +`CoordinateOutcome`, and promoted to protocol external taint only when inherited +by a later invocation — is later work in the same plan. + +Inline `#[cfg(test)] mod tests` follows the unique-`std::env::temp_dir()`-path +precedent (see [`../patterns.md`](../patterns.md)): marker path is worktree +scoped, the marker survives reconstruction of the handle until an explicit +`clear()`, and `persist`/`clear` are idempotent. + +## On-disk layout addition + +```text +/sce/ +└── mutation-cursor-tainted (runtime::external_taint, empty; existence = armed) +``` + +See also: [`mutation-trace-runtime-coordinator.md`](mutation-trace-runtime-coordinator.md), +[`mutation-trace-protocol.md`](mutation-trace-protocol.md), +[`checkout-identity.md`](checkout-identity.md). diff --git a/context/cli/mutation-trace-runtime-coordinator.md b/context/cli/mutation-trace-runtime-coordinator.md index 5228e996..d475817b 100644 --- a/context/cli/mutation-trace-runtime-coordinator.md +++ b/context/cli/mutation-trace-runtime-coordinator.md @@ -244,4 +244,6 @@ work tracked by the `mutation-cursor-runtime-coordinator` plan's follow-ups. See also: [`mutation-trace-protocol.md`](mutation-trace-protocol.md), [`mutation-trace-store.md`](mutation-trace-store.md), -[`checkout-identity.md`](checkout-identity.md). +[`mutation-trace-external-taint.md`](mutation-trace-external-taint.md) +(the `/sce/mutation-cursor-tainted` marker primitive, to be wired into +`coordinate()`), [`checkout-identity.md`](checkout-identity.md). diff --git a/context/context-map.md b/context/context-map.md index 71858fd0..73f80d30 100644 --- a/context/context-map.md +++ b/context/context-map.md @@ -28,6 +28,7 @@ Feature/domain context: - `context/cli/mutation-trace-quint-connect.md` (`#[cfg(test)]`-only Quint Connect model-based-testing harness in `cli/src/services/mutation_trace/mbt/` continuously checking `protocol.rs` against `spec/mutation_cursor.qnt`: the verification-only `mbtAction`/`MbtAction` record-payload transport excluded from comparison, the operation-identity-vs-`MbtStutter` distinction on guarded/no-op branches with its two deterministic regressions, finite ID mapping, the AC5 comparable-state field list, `randomPrepare` staying a single `step` branch, deterministic/generated (500×30, seed-reproducible) test coverage, and the two Nix checks — generic `checks.cli-tests` and dedicated `checks.mutation-trace-quint-connect` — that both require the pinned Quint binary plus the top-level `spec/` directory in `workspaceSrc`'s Nix fileset) - `context/cli/mutation-trace-store.md` (durable persistence for the mutation-cursor protocol in `cli/src/services/mutation_trace/store.rs`, built by the `mutation-cursor-store-persistence` plan: the one-directional `protocol.rs` -> `DurableTransition::between` (pure structural diff) -> `store.rs` (SQL translation) -> `RepositoryAgentTraceDb` boundary; migration `003_mutation_trace_protocol.sql`'s five tables; `AttemptState`/`external_taint` non-persistence; the 8-byte big-endian `BLOB` revision encoding plus explicit non-`Debug` enum codecs; the bounded hot-path `load_worktree` read vs. the cold-path `load_mutation_event`; `commit`'s single-`BEGIN IMMEDIATE` CAS batch via `TursoDb::execute_transactional_cas_batch`, distinguishing `Conflict`/retryable-transient/deterministic-`Err` outcomes; and the store's non-goals — no Git/filesystem I/O, no attribution/boundary-kind decisions, no retry-after-`Conflict` loop, no terminal-scope garbage collection) - `context/cli/mutation-trace-runtime-coordinator.md` (imperative-shell runtime layer in `cli/src/services/mutation_trace/runtime/`, built by the `mutation-cursor-runtime-coordinator` plan: the per-worktree OS advisory lock, `runtime::worktree_lock::WorktreeLock::acquire(git_dir, timeout)` at `/sce/mutation-cursor.lock`, bounded `try_lock()` polling with a distinct matchable timeout error and RAII release, and its distinction from the separate checkout-identity-creation lock; and the isolated Git snapshot service, `runtime::git_snapshot::GitSnapshotService` (`capture_tree`/`pin_tree`/`diff_trees`), writing tree/blob objects into the repository's normal object database and pinning durable ones via a create-only, idempotent `refs/sce/mutation-cursor//` ref rather than a private object store; and `runtime::coordinator`'s protocol-integration pipeline (`RuntimeBoundary`/`CoordinateOutcome`/`CoordinateError`, the load → recover-if-needed → prepare/commit CAS-retry loop, and its own bounded snapshot-failure taint-retry loop) plus the public `coordinate(repository_root, db, boundary)` entrypoint that acquires the `WorktreeLock`, derives `WorktreeId` from `checkout::get_or_create_checkout_id`, and drives that pipeline under one held lock; `runtime/tests.rs` covers the public `coordinate()` API end to end against real linked worktrees and a real Agent Trace DB; a `pub(crate)` re-export of `coordinate()` beyond `runtime` and harness/command wiring remain future work) +- `context/cli/mutation-trace-external-taint.md` (the worktree-local mutation-cursor durability boundary in `cli/src/services/mutation_trace/runtime/external_taint.rs`, built by the `mutation-cursor-external-taint` plan: the `ExternalTaintMarker` primitive — `new(git_dir)`/`exists()`/`persist()`/`clear()` over an empty file at `/sce/mutation-cursor-tainted` whose existence is its entire state, `checkout::persist_checkout_id_inner`-style durability (`sync_data` plus best-effort `#[cfg(unix)]` parent-dir `sync_all`), idempotent persist/clear, `NotFound`-on-clear as success, no `Drop` deletion — as the concrete runtime refinement of the abstract `ProtocolState.external_taint`; currently `#![allow(dead_code)]` and not yet armed by `coordinate()`, which the same plan wires in write-ahead before Agent Trace DB acquisition) - `context/sce/cli-exit-code-contract.md` (stable class-based `sce` process exit-code contract in `cli/src/app.rs`, so automation can branch on failure category without parsing error text) - `context/sce/cli-error-code-taxonomy.md` (stable user-facing `SCE-ERR-*` diagnostic code classes rendered by `cli/src/app.rs`, complementing the numeric exit-code classes) - `context/sce/cli-stdout-stderr-contract.md` (implemented stream contract in `cli/src/app.rs`: command payloads on stdout only, redacted diagnostics on stderr) diff --git a/context/plans/mutation-cursor-external-taint.md b/context/plans/mutation-cursor-external-taint.md index 9f06422b..ddd93f49 100644 --- a/context/plans/mutation-cursor-external-taint.md +++ b/context/plans/mutation-cursor-external-taint.md @@ -65,10 +65,11 @@ How this plan is proven complete. Each criterion is observable and names the check that proves it. `/validate` runs these checks; no task in the stack performs final validation. -- [ ] AC1: A marker is stored beneath the worktree-specific Git directory - (`/sce/mutation-cursor-tainted`), survives reconstruction of the - runtime service, and two linked worktrees resolve to two different marker - paths. +- [ ] AC1: A marker is stored beneath the caller-supplied worktree-specific Git + directory (`/sce/mutation-cursor-tainted`), survives reconstruction + of the marker handle, and two distinct `git_dir` inputs derive independent + marker paths and marker state. (Actual linked-worktree independence over a + real `git worktree add` pair is AC12/T04.) - Validate: `runtime::external_taint::tests::marker_is_worktree_scoped` and `runtime::external_taint::tests::marker_persists_until_explicitly_cleared` - [ ] AC2: `persist` then `persist` then `clear` then `clear` all succeed; @@ -267,8 +268,39 @@ local choices, not new requirements. ## Task stack -- [ ] T01: `Add the external-taint marker primitive` (status:todo) +- [x] T01: `Add the external-taint marker primitive` (status:done) - Task ID: T01 + - Completed: 2026-08-30 + - Files changed: + - `cli/src/services/mutation_trace/runtime/external_taint.rs` (new) — + `ExternalTaintMarker` with `new`/`exists`/`persist`/`clear`, local + `SCE_RUNTIME_DIR` / `MARKER_FILE` consts, checkout-identity-style + durability, cfg-gated best-effort parent-dir sync, inline `#[cfg(test)] + mod tests`. + - `cli/src/services/mutation_trace/runtime/mod.rs` — `mod external_taint;`. + - Result: New primitive backed by an empty file at + `/sce/mutation-cursor-tainted`; existence is the entire state. + `persist()` does `create_dir_all` + non-truncating `create` open + (`write(true).create(true).truncate(false)` — marker contents carry no + meaning) + `sync_data()` + best-effort `#[cfg(unix)]` parent-dir `sync_all` + (error swallowed); `clear()` does `remove_file` (`NotFound` → `Ok`) + the + same best-effort dir sync; `exists()` via `symlink_metadata`. No `Drop` + deletion. Module carries `#![allow(dead_code)]` (per `services/capabilities.rs` + precedent) since nothing wires it in until T02. No `coordinator.rs`, + protocol, store, or error-type change. + - Verify (re-run after the PR #245 review fixes — `truncate` removal, AC1 wording): + - `test ...runtime::external_taint` — PASS (3 passed: `marker_is_worktree_scoped`, + `marker_persists_until_explicitly_cleared`, `persist_and_clear_are_idempotent`). + - `test ...runtime::` — PASS (38 passed, 0 failed). + - `clippy --all-targets -- -D warnings` — PASS (clean). + - `fmt -- --check` — PASS (clean). + - Context impact: docs-update-needed. Introduces a new runtime module + (`ExternalTaintMarker`) and a new durable on-disk artifact + (`/sce/mutation-cursor-tainted`). Affected durable context: + `context/cli/mutation-trace-runtime-coordinator.md` (primitive + on-disk + layout entry), `spec/mutation_cursor.md` (concrete refinement of the + abstract `externalTaint` marker). Matches the plan's Context sync section; + no behavior reaches the coordinator or protocol yet. - Scope: In — new `cli/src/services/mutation_trace/runtime/external_taint.rs` defining `ExternalTaintMarker` with `new(git_dir)`, `exists()`, `persist()`, `clear()`; empty marker file at `/sce/mutation-cursor-tainted`; @@ -288,7 +320,7 @@ local choices, not new requirements. - Verify: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml services::mutation_trace::runtime::external_taint`; `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml --all-targets -- -D warnings`; `./scripts/run-cli-cargo.sh fmt --manifest-path cli/Cargo.toml -- --check`. - - Context synchronization: pending + - Context synchronization: synced - [ ] T02: `Arm the write-ahead fence around the full runtime boundary, DB acquisition included` (status:todo) - Task ID: T02 From 9dc029cc9babbb04c0c11c89b4a09f9ff510aaf4 Mon Sep 17 00:00:00 2001 From: David Abram Date: Sun, 30 Aug 2026 17:26:08 +0200 Subject: [PATCH 3/8] runtime+context: Arm the external-taint fence around coordination Prevent failures during Agent Trace DB acquisition and the protected mutation-trace boundary from losing the worktree-local durability signal. Reshape `coordinate()` to accept a DB provider, arm the marker after acquiring the worktree lock and before checkout identity or DB access, clear it only after success, and report marker and DB-provider failures explicitly. Add end-to-end coverage for success, failure, and fail-closed marker arming, and document the fence ordering and concrete marker refinement. Plan: mutation-cursor-external-taint (T02) Co-authored-by: SCE --- .../mutation_trace/runtime/coordinator.rs | 405 ++++++++++++++++-- .../services/mutation_trace/runtime/tests.rs | 40 +- context/cli/mutation-trace-external-taint.md | 75 +++- context/cli/mutation-trace-protocol.md | 35 +- .../cli/mutation-trace-runtime-coordinator.md | 113 ++--- context/context-map.md | 4 +- .../plans/mutation-cursor-external-taint.md | 66 ++- spec/mutation_cursor.md | 2 +- 8 files changed, 615 insertions(+), 125 deletions(-) diff --git a/cli/src/services/mutation_trace/runtime/coordinator.rs b/cli/src/services/mutation_trace/runtime/coordinator.rs index a4d0bb89..29621336 100644 --- a/cli/src/services/mutation_trace/runtime/coordinator.rs +++ b/cli/src/services/mutation_trace/runtime/coordinator.rs @@ -12,6 +12,7 @@ use crate::services::mutation_trace::types::{ self, ActorKind, AttemptId, Boundary, EventId, MutationEvent, ScopeId, TreeId, WorktreeId, }; +use super::external_taint::ExternalTaintMarker; use super::git_snapshot::GitSnapshotService; use super::worktree_lock::{acquire_inner, WorktreeLockError}; @@ -48,6 +49,14 @@ pub struct CoordinateOutcome { pub mutation_event: Option, } +/// Which [`ExternalTaintMarker`] operation failed while coordinating a boundary. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ExternalTaintOperation { + Inspect, + Persist, + Clear, +} + #[derive(Debug)] pub enum CoordinateError { SnapshotFailure { @@ -63,6 +72,19 @@ pub enum CoordinateError { revision: u64, }, LockAcquisition(anyhow::Error), + /// Inspecting, persisting, or clearing the worktree-local external-taint + /// marker failed. An inspect/persist failure is returned before any + /// checkout-identity, DB, snapshot, or protocol work; a clear failure is + /// returned after a successful boundary with the marker left in place. + ExternalTaintMarker { + operation: ExternalTaintOperation, + source: anyhow::Error, + }, + /// The caller-supplied Agent Trace DB provider returned `Err` after the + /// external-taint marker was already armed. The marker is intentionally + /// left in place so a later invocation treats the lost interval + /// conservatively. + AgentTraceDbUnavailable(anyhow::Error), Other(anyhow::Error), } @@ -87,6 +109,13 @@ impl std::fmt::Display for CoordinateError { "Worktree {worktree_id:?} requires recovery but its revision \ ({revision}) cannot be advanced" ), + CoordinateError::ExternalTaintMarker { operation, source } => write!( + f, + "External-taint marker {operation:?} operation failed: {source}" + ), + CoordinateError::AgentTraceDbUnavailable(source) => { + write!(f, "Repository Agent Trace DB is unavailable: {source}") + } CoordinateError::ScopeIdentityConflict(source) | CoordinateError::LockAcquisition(source) | CoordinateError::Other(source) => write!(f, "{source}"), @@ -111,21 +140,35 @@ impl SnapshotCapture for GitSnapshotService { } } -pub fn coordinate( +/// Coordinates one mutation-cursor runtime boundary end to end. +/// +/// The entrypoint owns the whole protected operation: it resolves `git_dir`, +/// acquires the [`WorktreeLock`](super::worktree_lock::WorktreeLock), arms the +/// worktree-local [`ExternalTaintMarker`] write-ahead — **before** acquiring the +/// Agent Trace DB — and only then invokes the caller-supplied `open_db` +/// provider, captures a snapshot, runs the snapshot / recovery / protocol / CAS +/// pipeline, and clears the marker on complete success. Any failure after the +/// marker is armed — including `open_db` returning `Err` — leaves the marker in +/// place for the next invocation. +pub fn coordinate

( repository_root: &Path, - db: &RepositoryAgentTraceDb, boundary: &RuntimeBoundary, -) -> Result { - coordinate_inner(repository_root, db, boundary, || {}) + open_db: P, +) -> Result +where + P: FnOnce() -> anyhow::Result, +{ + coordinate_inner(repository_root, boundary, open_db, || {}) } -fn coordinate_inner( +fn coordinate_inner( repository_root: &Path, - db: &RepositoryAgentTraceDb, boundary: &RuntimeBoundary, + open_db: P, on_lock_contention: F, ) -> Result where + P: FnOnce() -> anyhow::Result, F: FnOnce(), { let git_dir = resolve_git_dir(repository_root).map_err(CoordinateError::Other)?; @@ -133,12 +176,63 @@ where let _lock = acquire_inner(&git_dir, WORKTREE_LOCK_TIMEOUT, on_lock_contention) .map_err(lock_acquisition)?; - let checkout_id = get_or_create_checkout_id(&git_dir).map_err(CoordinateError::Other)?; + let marker = ExternalTaintMarker::new(&git_dir); + let inherited_external_taint = + marker + .exists() + .map_err(|source| CoordinateError::ExternalTaintMarker { + operation: ExternalTaintOperation::Inspect, + source, + })?; + marker + .persist() + .map_err(|source| CoordinateError::ExternalTaintMarker { + operation: ExternalTaintOperation::Persist, + source, + })?; + + let outcome = coordinate_protected( + repository_root, + &git_dir, + boundary, + open_db, + inherited_external_taint, + )?; + + marker + .clear() + .map_err(|source| CoordinateError::ExternalTaintMarker { + operation: ExternalTaintOperation::Clear, + source, + })?; + + Ok(outcome) +} + +fn coordinate_protected

( + repository_root: &Path, + git_dir: &Path, + boundary: &RuntimeBoundary, + open_db: P, + inherited_external_taint: bool, +) -> Result +where + P: FnOnce() -> anyhow::Result, +{ + let checkout_id = get_or_create_checkout_id(git_dir).map_err(CoordinateError::Other)?; let worktree_id = WorktreeId(checkout_id); + let db = open_db().map_err(CoordinateError::AgentTraceDbUnavailable)?; + let snapshot = GitSnapshotService::new(repository_root).map_err(CoordinateError::Other)?; - coordinate_boundary(db, &snapshot, &worktree_id, boundary) + coordinate_boundary( + &db, + &snapshot, + &worktree_id, + boundary, + inherited_external_taint, + ) } fn lock_acquisition(error: WorktreeLockError) -> CoordinateError { @@ -150,6 +244,7 @@ fn coordinate_boundary( capture: &C, worktree_id: &WorktreeId, boundary: &RuntimeBoundary, + _inherited_external_taint: bool, ) -> Result { let store = MutationTraceStore::new(db); @@ -583,7 +678,7 @@ mod tests { let worktree = WorktreeId("wt-1".to_string()); let capture = FakeSnapshotCapture::new(TreeId("tree-a".to_string())); - let outcome = coordinate_boundary(&db, &capture, &worktree, &RuntimeBoundary::Flush) + let outcome = coordinate_boundary(&db, &capture, &worktree, &RuntimeBoundary::Flush, false) .expect("first observation should succeed"); assert_eq!(outcome.observed_tree, TreeId("tree-a".to_string())); @@ -617,6 +712,7 @@ mod tests { event: EventId("evt-start".to_string()), actor_kind: ActorKind::ClaudeCode, }, + false, ) .expect("start should succeed"); @@ -630,6 +726,7 @@ mod tests { event: EventId("evt-advance".to_string()), actor_kind: ActorKind::ClaudeCode, }, + false, ) .expect("advance should succeed"); @@ -659,6 +756,7 @@ mod tests { event: EventId("evt-start".to_string()), actor_kind: ActorKind::ClaudeCode, }, + false, ) .expect("start should succeed"); @@ -669,12 +767,12 @@ mod tests { }; capture.push_success(TreeId("tree-b".to_string())); - let first = coordinate_boundary(&db, &capture, &worktree, &advance) + let first = coordinate_boundary(&db, &capture, &worktree, &advance, false) .expect("first advance should commit"); assert!(first.mutation_event.is_some()); capture.push_success(TreeId("tree-b".to_string())); - let replay = coordinate_boundary(&db, &capture, &worktree, &advance).expect( + let replay = coordinate_boundary(&db, &capture, &worktree, &advance, false).expect( "replaying the identical (scope, event) boundary must be a no-op, not an error", ); assert!( @@ -705,6 +803,7 @@ mod tests { event: EventId("evt-start".to_string()), actor_kind: ActorKind::ClaudeCode, }, + false, ) .expect("start should succeed"); @@ -718,6 +817,7 @@ mod tests { event: EventId("evt-close".to_string()), actor_kind: ActorKind::ClaudeCode, }, + false, ) .expect("close should succeed"); @@ -757,6 +857,7 @@ mod tests { event: EventId("evt-start-a".to_string()), actor_kind: actor_a, }, + false, ) .expect("starting scope a should succeed"); coordinate_boundary( @@ -768,6 +869,7 @@ mod tests { event: EventId("evt-start-b".to_string()), actor_kind: actor_b, }, + false, ) .expect("starting scope b should succeed"); @@ -781,6 +883,7 @@ mod tests { event: EventId("evt-advance-a".to_string()), actor_kind: actor_a, }, + false, ) .expect("advance should succeed"); @@ -816,8 +919,14 @@ mod tests { { let db = RepositoryAgentTraceDb::new_at(&db_path).expect("test db should open"); let bootstrap_capture = FakeSnapshotCapture::new(TreeId("tree-a".to_string())); - coordinate_boundary(&db, &bootstrap_capture, &worktree, &RuntimeBoundary::Flush) - .expect("baseline flush should succeed"); + coordinate_boundary( + &db, + &bootstrap_capture, + &worktree, + &RuntimeBoundary::Flush, + false, + ) + .expect("baseline flush should succeed"); } let barrier = std::sync::Arc::new(std::sync::Barrier::new(WRITERS)); @@ -832,9 +941,10 @@ mod tests { let capture = FakeSnapshotCapture::new(TreeId(format!("tree-writer-{i}"))); barrier.wait(); let outcome = - coordinate_boundary(&db, &capture, &worktree, &RuntimeBoundary::Flush).expect( - "each racing writer should eventually succeed after reload+recompute", - ); + coordinate_boundary(&db, &capture, &worktree, &RuntimeBoundary::Flush, false) + .expect( + "each racing writer should eventually succeed after reload+recompute", + ); ( outcome.revision, capture.capture_call_count(), @@ -885,6 +995,7 @@ mod tests { event: EventId("evt-start-live".to_string()), actor_kind: ActorKind::ClaudeCode, }, + false, ) .expect("starting the live scope should succeed"); coordinate_boundary( @@ -896,6 +1007,7 @@ mod tests { event: EventId("evt-start-abandoned".to_string()), actor_kind: ActorKind::ClaudeCode, }, + false, ) .expect("starting the to-be-abandoned scope should succeed"); @@ -932,6 +1044,7 @@ mod tests { event: EventId("evt-advance-live".to_string()), actor_kind: ActorKind::ClaudeCode, }, + false, ) .expect("advance should trigger needs_rebaseline recovery first, then succeed"); assert!( @@ -977,6 +1090,7 @@ mod tests { event: EventId("evt-start".to_string()), actor_kind: ActorKind::ClaudeCode, }, + false, ) .expect("start should succeed"); @@ -1013,6 +1127,7 @@ mod tests { event: EventId("evt-advance".to_string()), actor_kind: ActorKind::ClaudeCode, }, + false, ) .expect("advance should trigger taint recovery first, then succeed"); @@ -1064,6 +1179,7 @@ mod tests { event: event.clone(), actor_kind: ActorKind::ClaudeCode, }, + false, ) .expect_err("recovery that cannot advance revision must reject the triggering boundary"); @@ -1125,13 +1241,13 @@ mod tests { let (db, db_path) = test_db("ac11-taints-existing"); let worktree = WorktreeId("wt-1".to_string()); let bootstrap = FakeSnapshotCapture::new(TreeId("tree-a".to_string())); - coordinate_boundary(&db, &bootstrap, &worktree, &RuntimeBoundary::Flush) + coordinate_boundary(&db, &bootstrap, &worktree, &RuntimeBoundary::Flush, false) .expect("baseline flush should materialize the worktree"); let failing = FakeSnapshotCapture::new(TreeId("tree-b".to_string())); failing.push_failure("simulated git snapshot failure"); - let error = coordinate_boundary(&db, &failing, &worktree, &RuntimeBoundary::Flush) + let error = coordinate_boundary(&db, &failing, &worktree, &RuntimeBoundary::Flush, false) .expect_err("a capture failure against an existing worktree should be reported"); match error { CoordinateError::SnapshotFailure { @@ -1159,7 +1275,7 @@ mod tests { let (db, db_path) = test_db("ac11-taint-retry-succeeds"); let worktree = WorktreeId("wt-1".to_string()); let bootstrap = FakeSnapshotCapture::new(TreeId("tree-a".to_string())); - coordinate_boundary(&db, &bootstrap, &worktree, &RuntimeBoundary::Flush) + coordinate_boundary(&db, &bootstrap, &worktree, &RuntimeBoundary::Flush, false) .expect("baseline flush should materialize the worktree"); let store = MutationTraceStore::new(&db); @@ -1202,7 +1318,7 @@ mod tests { let (db, db_path) = test_db("ac11-taint-exhaustion"); let worktree = WorktreeId("wt-1".to_string()); let bootstrap = FakeSnapshotCapture::new(TreeId("tree-a".to_string())); - coordinate_boundary(&db, &bootstrap, &worktree, &RuntimeBoundary::Flush) + coordinate_boundary(&db, &bootstrap, &worktree, &RuntimeBoundary::Flush, false) .expect("baseline flush should materialize the worktree"); let store = MutationTraceStore::new(&db); @@ -1244,7 +1360,7 @@ mod tests { let failing = FakeSnapshotCapture::new(TreeId("unused".to_string())); failing.push_failure("simulated git snapshot failure before any baseline exists"); - let error = coordinate_boundary(&db, &failing, &worktree, &RuntimeBoundary::Flush) + let error = coordinate_boundary(&db, &failing, &worktree, &RuntimeBoundary::Flush, false) .expect_err("a capture failure with no prior worktree row should still be reported"); match error { CoordinateError::SnapshotFailure { @@ -1283,7 +1399,7 @@ mod tests { }, ); - let error = coordinate_boundary(&db, &capture, &worktree, &RuntimeBoundary::Flush) + let error = coordinate_boundary(&db, &capture, &worktree, &RuntimeBoundary::Flush, false) .expect_err( "a capture failure racing a concurrent materialization should still be reported", ); @@ -1320,13 +1436,16 @@ mod tests { let repo_root_clone = repo_root.clone(); let db_path_clone = db_path.clone(); let worker = thread::spawn(move || { - let db = RepositoryAgentTraceDb::new_at(&db_path_clone).expect("worker db should open"); - let outcome = - coordinate_inner(&repo_root_clone, &db, &RuntimeBoundary::Flush, move || { + let outcome = coordinate_inner( + &repo_root_clone, + &RuntimeBoundary::Flush, + || RepositoryAgentTraceDb::new_at(&db_path_clone), + move || { contention_tx .send(()) .expect("contention signal channel should still be open"); - }); + }, + ); result_tx .send(()) .expect("result signal channel should still be open"); @@ -1362,4 +1481,238 @@ mod tests { remove_test_repo(&repo_root); } + + #[test] + fn public_coordinate_clears_marker_on_success() { + let repo_root = unique_test_repo("t02-success-clears-marker"); + init_repo(&repo_root); + let db_path = repo_root.join("agent-trace.db"); + RepositoryAgentTraceDb::new_at(&db_path).expect("seed db should open with schema"); + let git_dir = resolve_git_dir(&repo_root).expect("git dir should resolve"); + let marker = ExternalTaintMarker::new(&git_dir); + + let outcome = coordinate(&repo_root, &RuntimeBoundary::Flush, || { + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + }) + .expect("a first observation should succeed"); + assert_eq!( + outcome.revision, 0, + "a first-observation flush should not advance the revision" + ); + assert!( + !marker.exists().expect("marker existence should resolve"), + "a successful coordinate() must clear the marker it armed" + ); + + remove_test_repo(&repo_root); + } + + #[test] + fn public_coordinate_leaves_marker_after_a_snapshot_failure() { + let repo_root = unique_test_repo("t02-snapshot-failure-marker"); + init_repo(&repo_root); + let db_path = repo_root.join("agent-trace.db"); + RepositoryAgentTraceDb::new_at(&db_path).expect("seed db should open with schema"); + let git_dir = resolve_git_dir(&repo_root).expect("git dir should resolve"); + let marker = ExternalTaintMarker::new(&git_dir); + + coordinate(&repo_root, &RuntimeBoundary::Flush, || { + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + }) + .expect("the baseline observation should succeed"); + assert!( + !marker.exists().expect("marker existence should resolve"), + "the successful baseline must have cleared its marker" + ); + + let tmp_index_dir = git_dir.join("sce").join("tmp"); + let _ = std::fs::remove_dir_all(&tmp_index_dir); + std::fs::write(&tmp_index_dir, b"not a directory").expect( + "planting a file where the snapshot service expects its temp-index directory should succeed", + ); + + let error = coordinate(&repo_root, &RuntimeBoundary::Flush, || { + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + }) + .expect_err("a Git snapshot failure after marker arming should be reported"); + assert!( + matches!(error, CoordinateError::SnapshotFailure { .. }), + "expected SnapshotFailure, got {error:?}" + ); + assert!( + marker.exists().expect("marker existence should resolve"), + "a snapshot failure after arming must leave the external-taint marker in place" + ); + + remove_test_repo(&repo_root); + } + + #[test] + fn public_coordinate_leaves_marker_after_a_non_snapshot_failure() { + let repo_root = unique_test_repo("t02-non-snapshot-failure-marker"); + init_repo(&repo_root); + let db_path = repo_root.join("agent-trace.db"); + let seed_db = RepositoryAgentTraceDb::new_at(&db_path).expect("seed db should open"); + let git_dir = resolve_git_dir(&repo_root).expect("git dir should resolve"); + let checkout_id = get_or_create_checkout_id(&git_dir).expect("checkout id should resolve"); + insert_worktree_at_revision( + &seed_db, + &checkout_id, + "tree-a", + u64::MAX, + true, + "snapshot_failure", + false, + ); + drop(seed_db); + + let marker = ExternalTaintMarker::new(&git_dir); + let error = coordinate(&repo_root, &RuntimeBoundary::Flush, || { + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + }) + .expect_err("mandatory recovery that cannot advance the revision must reject the boundary"); + assert!( + matches!(error, CoordinateError::RevisionExhausted { .. }), + "expected RevisionExhausted, got {error:?}" + ); + assert!( + marker.exists().expect("marker existence should resolve"), + "a non-snapshot failure after arming must leave the external-taint marker in place" + ); + + remove_test_repo(&repo_root); + } + + #[test] + fn public_coordinate_fails_closed_when_the_marker_cannot_be_armed() { + let repo_root = unique_test_repo("t02-marker-arm-failure"); + init_repo(&repo_root); + let db_path = repo_root.join("agent-trace.db"); + RepositoryAgentTraceDb::new_at(&db_path).expect("seed db should open with schema"); + let git_dir = resolve_git_dir(&repo_root).expect("git dir should resolve"); + + // Plant a directory exactly where the marker file must be created, so + // `ExternalTaintMarker::persist` fails deterministically regardless of uid. + std::fs::create_dir_all(git_dir.join("sce").join("mutation-cursor-tainted")) + .expect("planting a directory at the marker path should succeed"); + + let provider_called = std::sync::atomic::AtomicBool::new(false); + let error = coordinate(&repo_root, &RuntimeBoundary::Flush, || { + provider_called.store(true, Ordering::SeqCst); + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + }) + .expect_err("an unarmed marker must fail coordinate() closed"); + assert!( + matches!( + error, + CoordinateError::ExternalTaintMarker { + operation: ExternalTaintOperation::Persist, + .. + } + ), + "expected an ExternalTaintMarker persist failure, got {error:?}" + ); + assert!( + !provider_called.load(Ordering::SeqCst), + "a marker-arming failure must return before DB-provider, snapshot, or protocol work" + ); + + remove_test_repo(&repo_root); + } + + #[test] + fn public_coordinate_fails_closed_when_marker_inspection_fails() { + let repo_root = unique_test_repo("t02-marker-inspect-failure"); + init_repo(&repo_root); + let db_path = repo_root.join("agent-trace.db"); + RepositoryAgentTraceDb::new_at(&db_path).expect("seed db should open with schema"); + let git_dir = resolve_git_dir(&repo_root).expect("git dir should resolve"); + let sce_dir = git_dir.join("sce"); + std::fs::create_dir_all(&sce_dir).expect("the sce directory should be creatable"); + + let held = acquire_inner(&git_dir, Duration::from_secs(5), || {}) + .expect("the test should hold the runtime lock before the worker runs"); + + let provider_called = std::sync::Arc::new(std::sync::atomic::AtomicBool::new(false)); + let (contention_tx, contention_rx) = mpsc::channel(); + + let worker = { + let repo_root = repo_root.clone(); + let db_path = db_path.clone(); + let provider_called = std::sync::Arc::clone(&provider_called); + thread::spawn(move || { + coordinate_inner( + &repo_root, + &RuntimeBoundary::Flush, + || { + provider_called.store(true, Ordering::SeqCst); + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + }, + move || { + contention_tx + .send(()) + .expect("contention signal channel should still be open"); + }, + ) + }) + }; + + contention_rx.recv_timeout(Duration::from_secs(5)).expect( + "the worker's coordinate_inner should reach the WorktreeLock contention branch", + ); + + std::fs::remove_dir_all(&sce_dir).expect("the sce directory should be removable"); + std::fs::write(&sce_dir, b"not a directory") + .expect("planting a file where the sce directory was should succeed"); + + drop(held); + + let error = worker + .join() + .expect("worker thread should not panic") + .expect_err("marker inspection failure must fail coordinate() closed"); + assert!( + matches!( + error, + CoordinateError::ExternalTaintMarker { + operation: ExternalTaintOperation::Inspect, + .. + } + ), + "expected an ExternalTaintMarker inspect failure, got {error:?}" + ); + assert!( + !provider_called.load(Ordering::SeqCst), + "marker inspection failing closed must precede DB-provider, checkout, snapshot, and protocol work" + ); + + remove_test_repo(&repo_root); + } + + #[test] + fn public_coordinate_leaves_marker_when_the_db_provider_fails() { + let repo_root = unique_test_repo("t02-db-provider-failure-marker"); + init_repo(&repo_root); + let git_dir = resolve_git_dir(&repo_root).expect("git dir should resolve"); + let marker = ExternalTaintMarker::new(&git_dir); + + let error = coordinate( + &repo_root, + &RuntimeBoundary::Flush, + || -> anyhow::Result { + Err(anyhow::anyhow!("simulated Agent Trace DB open failure")) + }, + ) + .expect_err("a DB provider that returns Err must fail coordinate()"); + assert!( + matches!(error, CoordinateError::AgentTraceDbUnavailable(_)), + "expected AgentTraceDbUnavailable, got {error:?}" + ); + assert!( + marker.exists().expect("marker existence should resolve"), + "a DB-provider failure after arming must leave the external-taint marker present" + ); + + remove_test_repo(&repo_root); + } } diff --git a/cli/src/services/mutation_trace/runtime/tests.rs b/cli/src/services/mutation_trace/runtime/tests.rs index aea7491d..02d37d45 100644 --- a/cli/src/services/mutation_trace/runtime/tests.rs +++ b/cli/src/services/mutation_trace/runtime/tests.rs @@ -81,16 +81,20 @@ fn linked_worktrees_have_independent_locks_and_worktree_ids() { "a linked worktree must resolve its own worktree-specific git dir, giving it a distinct lock and identity path" ); - let main_outcome = coordinate(&main_root, &db_main, &RuntimeBoundary::Flush) - .expect("first observation on the main worktree should succeed"); + let main_outcome = coordinate(&main_root, &RuntimeBoundary::Flush, || { + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + }) + .expect("first observation on the main worktree should succeed"); let held = WorktreeLock::acquire(&main_git_dir, Duration::from_secs(5)) .expect("the main worktree's runtime lock should be acquirable"); - let db_linked = RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path).expect( - "a second handle to the same caller-supplied repository-scoped DB path should open", - ); - let linked_outcome = coordinate(&linked_root, &db_linked, &RuntimeBoundary::Flush).expect( + let linked_outcome = coordinate(&linked_root, &RuntimeBoundary::Flush, || { + // A second handle to the same caller-supplied repository-scoped DB path, + // opened through the provider while the main worktree's lock is held. + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + }) + .expect( "coordinate() on the linked worktree must acquire its own distinct runtime lock while the main worktree's lock is still held", ); @@ -139,8 +143,8 @@ fn agent_trace_storage_and_coordinator_observe_the_same_checkout_id() { std::fs::create_dir_all(&state_root).expect("state root should be created"); let coordinator_db_path = repo_root.join("coordinator.db"); - let db = RepositoryAgentTraceDb::new_at(&coordinator_db_path) - .expect("the coordinator's repository DB should open"); + RepositoryAgentTraceDb::new_at(&coordinator_db_path) + .expect("the coordinator's repository DB should open with schema"); let barrier = Arc::new(Barrier::new(2)); @@ -162,8 +166,10 @@ fn agent_trace_storage_and_coordinator_observe_the_same_checkout_id() { }; barrier.wait(); - let outcome = coordinate(&repo_root, &db, &RuntimeBoundary::Flush) - .expect("the coordinator's first observation should succeed"); + let outcome = coordinate(&repo_root, &RuntimeBoundary::Flush, || { + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&coordinator_db_path) + }) + .expect("the coordinator's first observation should succeed"); let storage_checkout_id = storage_thread .join() @@ -193,8 +199,10 @@ fn a_snapshot_failure_then_recovery_cycle_runs_through_the_public_api() { let db_path = repo_root.join("agent-trace.db"); let db = RepositoryAgentTraceDb::new_at(&db_path).expect("repository DB should open"); - let baseline = coordinate(&repo_root, &db, &RuntimeBoundary::Flush) - .expect("the baseline observation should materialize the worktree"); + let baseline = coordinate(&repo_root, &RuntimeBoundary::Flush, || { + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + }) + .expect("the baseline observation should materialize the worktree"); let worktree_id = baseline.worktree_id.clone(); let git_dir = resolve_git_dir(&repo_root).expect("git dir should resolve"); @@ -207,12 +215,12 @@ fn a_snapshot_failure_then_recovery_cycle_runs_through_the_public_api() { let scope = ScopeId("scope-recovery".to_string()); let failure = coordinate( &repo_root, - &db, &RuntimeBoundary::Start { scope: scope.clone(), event: EventId("evt-during-failure".to_string()), actor_kind: ActorKind::ClaudeCode, }, + || RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path), ) .expect_err("a Git snapshot failure against a materialized worktree should be reported"); match failure { @@ -240,8 +248,10 @@ fn a_snapshot_failure_then_recovery_cycle_runs_through_the_public_api() { std::fs::remove_file(&tmp_index_dir) .expect("removing the planted file should let the snapshot service recreate its temp dir"); - let recovered = coordinate(&repo_root, &db, &RuntimeBoundary::Flush) - .expect("the coordinator should recover from the taint and process the boundary"); + let recovered = coordinate(&repo_root, &RuntimeBoundary::Flush, || { + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + }) + .expect("the coordinator should recover from the taint and process the boundary"); assert_eq!( recovered.worktree_id, worktree_id, "recovery must operate on the same worktree identity" diff --git a/context/cli/mutation-trace-external-taint.md b/context/cli/mutation-trace-external-taint.md index e1d10f55..912ba208 100644 --- a/context/cli/mutation-trace-external-taint.md +++ b/context/cli/mutation-trace-external-taint.md @@ -40,17 +40,82 @@ sync is best-effort). The marker is never removed via `Drop`; only an explicit `clear()` removes it. It is never authoritative for normal cursor state. The module carries `#![allow(dead_code)]` (the `services/capabilities.rs` -precedent): nothing arms or reads the marker yet. Wiring it into a reshaped -`coordinate()` entrypoint — armed write-ahead after the `WorktreeLock` is held -and before Agent Trace DB acquisition, cleared only on a successful -`CoordinateOutcome`, and promoted to protocol external taint only when inherited -by a later invocation — is later work in the same plan. +precedent) for the parts not yet reached; `new`/`exists`/`persist`/`clear` are +armed by `coordinate()` (below). Inline `#[cfg(test)] mod tests` follows the unique-`std::env::temp_dir()`-path precedent (see [`../patterns.md`](../patterns.md)): marker path is worktree scoped, the marker survives reconstruction of the handle until an explicit `clear()`, and `persist`/`clear` are idempotent. +## The write-ahead fence around `coordinate()` + +The public entrypoint in +[`mutation-trace-runtime-coordinator.md`](mutation-trace-runtime-coordinator.md) +owns the whole protected operation and no longer receives an already-open DB +handle: + +```text +coordinate(repository_root, boundary, open_db) + open_db: impl FnOnce() -> anyhow::Result +``` + +Order inside the held `WorktreeLock`: + +```text +resolve git_dir → acquire WorktreeLock + → ExternalTaintMarker::new(git_dir) + → inherited_external_taint = marker.exists()? + → marker.persist()? ← fence armed here, write-ahead + → get_or_create_checkout_id → WorktreeId + → open_db() ← DB acquired INSIDE the fence + → GitSnapshotService::new + → coordinate_boundary(&db, .., inherited_external_taint) + → marker.clear()? ← only on a successful outcome +``` + +**Safety invariant:** no failure after the marker is armed — including a +failure to open the Agent Trace DB, or to resolve checkout identity — can +disappear without leaving the worktree-local signal for the next invocation. +Arming *before* `open_db()` is the whole point: if the DB open fails, the marker +is already on disk, so a later invocation that opens the DB successfully still +sees the inherited signal instead of trusting a lost interval. + +The marker is cleared only by `coordinate()`'s success path (`marker.clear()` +after an `Ok` outcome). Every error path — snapshot failure, DB provider `Err`, +checkout-identity failure, DB read/write failure, CAS exhaustion, scope-identity +conflict, unexpected error — returns with the marker present. No `Drop` clears +it. + +### `CoordinateError` variants + +- `ExternalTaintMarker { operation: ExternalTaintOperation, source }` where + `ExternalTaintOperation` is `Inspect | Persist | Clear`. An `Inspect` or + `Persist` failure is returned **before** any checkout-identity, DB, snapshot, + or protocol work (fail closed — an unarmed fence must abort the boundary). A + `Clear` failure is returned **after** a durable boundary, with the marker + deliberately left in place for a later conservative re-recovery. +- `AgentTraceDbUnavailable(source)` — the caller-supplied `open_db` provider + returned `Err` after the marker was armed. The marker is intentionally left in + place; the lower-level `coordinate_boundary` pipeline is never entered. + +### Inherited taint, not yet consumed + +`coordinate()` computes `inherited_external_taint` from `marker.exists()` and +threads it to `coordinate_boundary` as `_inherited_external_taint`, unused so +far. Mapping an inherited marker into `protocol::database_failure` recovery +against the single captured snapshot — and re-injecting it across a losing +recovery CAS — is the next task in the same plan. `WorktreeProjection:: +into_protocol_state()` still returns an empty `external_taint`; the filesystem +overlay is applied only by runtime code. + +Inline `coordinator.rs` tests drive the public `coordinate()` against real +repositories: a successful call clears the marker; a snapshot failure, a +non-snapshot failure (revision-exhausted recovery), and a DB provider returning +`Err` each leave the marker present; and both marker-I/O failure operations — +`Inspect` (a deterministic `ENOTDIR`, no permission changes) and `Persist` — +fail the call closed before the DB provider is ever invoked. + ## On-disk layout addition ```text diff --git a/context/cli/mutation-trace-protocol.md b/context/cli/mutation-trace-protocol.md index 14459949..5d100d64 100644 --- a/context/cli/mutation-trace-protocol.md +++ b/context/cli/mutation-trace-protocol.md @@ -203,7 +203,13 @@ guard — `abandon` resolves it through the referenced scope's own materialized `worktree_id` rather than taking a `WorktreeId` directly — which keeps `external_taint ⊆ ProtocolState.worktrees` an invariant of every state this module can produce, since `database_failure` is the sole path that inserts -into `external_taint`. +into `external_taint`. The concrete runtime refinement of `external_taint` is +the worktree-local `/sce/mutation-cursor-tainted` marker (see +[`mutation-trace-external-taint.md`](mutation-trace-external-taint.md)), armed +write-ahead before Agent Trace DB acquisition and overlaid onto +`database_failure` recovery only when a later invocation inherits it; +`WorktreeProjection::into_protocol_state()` itself always returns an empty +`external_taint`. Future responsibility split (mirrors "Runtime scope materialization" above): the coordinator/store layer resolves/materializes worktree identity/state and @@ -213,14 +219,12 @@ loads a `ProtocolState`; `protocol.rs` only transitions already-known ones. The plan's file split anticipated three seams beyond `protocol.rs`. `store.rs`, `runtime/git_snapshot.rs`, and `coordinator.rs` (with its public `coordinate()` -entrypoint) now all exist as real call sites (see -[`mutation-trace-runtime-coordinator.md`](mutation-trace-runtime-coordinator.md)), -with the runtime layer covered by cross-module integration tests; only -harness/command wiring remains: +entrypoint) now all exist as real call sites, covered by cross-module +integration tests; only harness/command wiring remains: ```mermaid flowchart LR - coordinator["coordinator.rs (implemented)\n(imperative shell:\nlock, DB load, Git snapshot,\nCAS/retry, persist)"] + coordinator["coordinator.rs (implemented)\n(imperative shell: lock,\nexternal-taint fence, DB provider,\nGit snapshot, CAS/retry, persist)"] protocol["protocol.rs\n(pure transitions —\nprepare/commit/attribution/\ntaint/abandon/recover\nall implemented)"] git_snapshot["runtime/git_snapshot.rs (implemented)\n(isolated Git snapshot,\ntemporary index, tree capture/diff,\nSCE-owned ref pinning)"] store["store.rs\n(cursor/revision, scopes,\nprocessed events, mutation\nevidence, CAS transaction)"] @@ -230,18 +234,13 @@ flowchart LR coordinator --> store ``` -- **`coordinator.rs`** (implemented) — its public `coordinate()` acquires the - per-worktree lock, derives `WorktreeId` from checkout identity, drives one - Git snapshot, and calls the pure protocol under a bounded CAS-retry loop. -- **`runtime/git_snapshot.rs`** (implemented) — captures an isolated worktree - snapshot and pins it durably; called by `coordinator.rs`. -- **`store.rs`** (implemented) — loads and persists worktree/scope/event state - via a CAS-guarded commit; never remaps an existing `ScopeId`'s - `actor_kind`/`worktree_id` (see "Runtime scope materialization" above). -- **`protocol.rs`** — assumes referenced scopes already exist in - `ProtocolState.scopes`; validates and transitions lifecycle state only. - -`protocol.rs` stays free of any Git object, DB row, or CAS transaction concept. +`coordinator.rs` (see +[`mutation-trace-runtime-coordinator.md`](mutation-trace-runtime-coordinator.md)) +owns the lock, the external-taint fence, the caller-supplied DB provider, one +Git snapshot, and a bounded CAS-retry loop; `store.rs` never remaps an existing +`ScopeId`'s `actor_kind`/`worktree_id`; `protocol.rs` assumes referenced scopes +already exist and stays free of any Git object, DB row, or CAS transaction +concept. ## Authoritative source diff --git a/context/cli/mutation-trace-runtime-coordinator.md b/context/cli/mutation-trace-runtime-coordinator.md index d475817b..298f7adb 100644 --- a/context/cli/mutation-trace-runtime-coordinator.md +++ b/context/cli/mutation-trace-runtime-coordinator.md @@ -22,11 +22,10 @@ documented convention. ## Current code surface The per-worktree runtime lock, the isolated Git snapshot service, the -coordinator's internal protocol-integration pipeline, and the public, -lock-wrapped `coordinate()` entrypoint that drives the lock and checkout -identity around that pipeline all exist, with cross-module integration tests -in `runtime/tests.rs` exercising the public API end to end. Only -harness/command wiring remains. +coordinator's protocol-integration pipeline, and the public `coordinate()` +entrypoint (lock, external-taint fence, checkout identity, and DB provider +around that pipeline) all exist, with `runtime/tests.rs` exercising the public +API end to end. Only harness/command wiring remains. - `cli/src/services/mutation_trace/runtime/worktree_lock.rs` — `WorktreeLock::acquire(git_dir: &Path, timeout: Duration) -> @@ -88,23 +87,28 @@ harness/command wiring remains. `Flush` carries nothing — its worktree is always the invocation's own already-resolved one, never caller-supplied) and documents the `(ScopeId, EventId)` replay-identity contract a future harness adapter must - uphold. The public `coordinate(repository_root, db, boundary) -> - Result` entrypoint takes an - already-resolved `RepositoryAgentTraceDb` from its caller — it never - resolves or opens the repository-scoped Agent Trace DB itself — and owns - the critical section: resolve `git_dir` via `checkout::resolve_git_dir`, - acquire the `WorktreeLock` (bounded 10s, held for the whole call), resolve + uphold. The public `coordinate(repository_root, boundary, open_db) -> + Result` entrypoint owns the whole + protected operation. It does **not** receive an already-open DB handle: + `open_db: impl FnOnce() -> anyhow::Result` is a + caller-supplied provider it invokes itself, so DB acquisition falls inside + the external-taint fence. The critical section: resolve `git_dir` via + `checkout::resolve_git_dir`, acquire the `WorktreeLock` (bounded 10s, held + for the whole call), arm the `ExternalTaintMarker` write-ahead, resolve checkout identity via `checkout::get_or_create_checkout_id` and wrap it as `WorktreeId` — no caller-supplied `WorktreeId` or `Boundary` is ever - accepted — then construct `GitSnapshotService` and delegate to the internal, - generic-over-`SnapshotCapture` pipeline. Identity flows - `repository_root → git_dir → WorktreeLock → checkout ID → WorktreeId`; - the `RepositoryAgentTraceDb` is not on that chain. (`coordinate()` is a one-line - delegation to a private `coordinate_inner(.., on_lock_contention: - impl FnOnce())` test seam; production passes a no-op closure.) A - `WorktreeLock` acquisition failure (timeout or I/O) surfaces as - `CoordinateError::LockAcquisition`. The - pipeline does, per invocation: capture and pin + accepted — invoke `open_db()`, construct `GitSnapshotService`, delegate to + the internal generic-over-`SnapshotCapture` pipeline, and clear the marker + only on a successful outcome. Identity flows + `repository_root → git_dir → WorktreeLock → checkout ID → WorktreeId`; the + DB is not on that chain. (`coordinate()` is a one-line delegation to a + private `coordinate_inner(.., open_db, on_lock_contention: impl FnOnce())` + test seam; production passes a no-op contention closure.) A `WorktreeLock` + acquisition failure surfaces as `CoordinateError::LockAcquisition`; marker-I/O + and DB-provider failures have their own fail-closed variants — see + [`mutation-trace-external-taint.md`](mutation-trace-external-taint.md) for the + fence ordering, the safety invariant, and the `CoordinateError` variants it + adds. The pipeline does, per invocation: capture and pin exactly one Git snapshot; on failure, run a bounded taint-retry loop instead (below) and return without touching the rest of the pipeline; on success, idempotently materialize the worktree row and, for hook boundaries, the @@ -128,11 +132,12 @@ harness/command wiring remains. `Conflict`, reporting `persisted_taint: false` only once every bounded attempt has been exhausted. -The runtime lock guards the coordinator's own critical section (snapshot -capture, worktree/scope materialization, recovery, and the CAS retry loop): -`coordinate()` acquires it before resolving checkout identity and holds it -until the call returns. It is held on every `coordinate()` call, unlike the -checkout-identity-creation lock. +The runtime lock guards the coordinator's own critical section (external-taint +marker arming/clearing, snapshot capture, worktree/scope materialization, +recovery, and the CAS retry loop): `coordinate()` acquires it before arming the +marker and resolving checkout identity, and holds it until the call returns. It +is held on every `coordinate()` call, unlike the checkout-identity-creation +lock. ## Two distinct locks, two distinct invariants @@ -153,6 +158,7 @@ On-disk layout so far: ├── checkout-id (services::checkout) ├── checkout-id.lock (services::checkout) ├── mutation-cursor.lock (runtime::worktree_lock) +├── mutation-cursor-tainted (runtime::external_taint, empty; existence = fence armed) └── tmp/ └── index- (runtime::git_snapshot, ephemeral per capture) @@ -199,16 +205,14 @@ taint-retry loop taints an existing worktree, survives a losing CAS before committing on retry, reports `persisted_taint: false` once exhausted, makes no write when no worktree row exists yet, and still finds and taints a worktree another caller materializes concurrently during this invocation's -own failing capture. One further test proves the critical-section -serialization with a real happens-before ordering: `coordinate()` delegates -to a private `coordinate_inner(.., on_lock_contention: impl FnOnce())` that -takes the lock via `worktree_lock::acquire_inner` (T02's seam, `pub(super)`), -and with a first `WorktreeLock` held, a worker's `coordinate_inner` call -observes the real `TryLockError::WouldBlock` branch (signalling a channel -from `on_lock_contention`) while that first guard is still alive, then — once -the guard is dropped — the same invocation acquires the lock and returns -`Ok`. Production `coordinate()` passes a no-op closure, so its code path, -signature, and lock semantics are unchanged. +own failing capture. Further tests drive the public `coordinate()` against +real repositories: the critical-section serialization (a worker's +`coordinate_inner(.., open_db, on_lock_contention)` observes the real +`TryLockError::WouldBlock` branch while a first `WorktreeLock` is held, then +acquires and returns `Ok` once it drops); and the external-taint fence — a +successful call clears the marker, while a snapshot failure, a non-snapshot +failure, a DB-provider `Err`, and an un-armable marker each leave it present +(the last failing closed before the DB provider runs). `runtime/tests.rs` is `runtime`'s own `#[cfg(test)] mod tests`, holding cross-module integration tests that drive only the public `coordinate()` API @@ -218,32 +222,29 @@ precedent: two linked worktrees of one repository (different `git_dir` → different lock paths → different `WorktreeId`s) are proven independently locked by holding one worktree's `WorktreeLock` across a synchronous `coordinate()` call for the other and observing that call return `Ok` before -the held guard is dropped — a shared lock could not be acquired while the -guard is alive, and no wall-clock timing is used. The test opens one -repository-scoped DB path itself and hands a separate handle to each -`coordinate()` call (`coordinate()` does not resolve the DB), then asserts -both distinct worktree rows coexist in that one supplied DB, and that a tree -pinned by one worktree's coordinator resolves through the other's `GIT_DIR`. -A first-ever `agent_trace_storage` resolution and a `coordinate()` call on -the same checkout converge on one checkout identity (matching the on-disk -`checkout-id` file); and a full failure/recovery cycle — baseline call, a -snapshot-failing call that durably taints the worktree, then a recovery call -that clears the taint before processing its boundary — runs entirely through -the public entrypoint. +the held guard is dropped. Each call is handed a provider closure that opens +the one shared repository-scoped DB path (`coordinate()` never resolves the +DB), and both distinct worktree rows then coexist in it. A first-ever +`agent_trace_storage` resolution and a `coordinate()` call on the same +checkout converge on one checkout identity; and a full failure/recovery +cycle — baseline call, a snapshot-failing call that durably taints the +worktree, then a recovery call that clears the taint before processing its +boundary — runs entirely through the public entrypoint. ## Status -The per-worktree runtime lock, the isolated Git snapshot service, the -coordinator's internal protocol-integration pipeline (above), and the public, -lock-wrapped `coordinate()` entrypoint (resolving `git_dir`, acquiring -`WorktreeLock`, resolving checkout identity, deriving `WorktreeId`, delegating -to the pipeline) are all implemented, and `runtime/tests.rs` covers the -public `coordinate()` API end to end (above). A `pub(crate)` re-export of -`coordinate()` beyond `runtime` and any harness/command wiring remain future -work tracked by the `mutation-cursor-runtime-coordinator` plan's follow-ups. +The lock, snapshot service, protocol-integration pipeline, and the public +`coordinate()` entrypoint (resolve `git_dir` → `WorktreeLock` → arm the +external-taint marker → checkout identity → caller-supplied DB provider → +pipeline → clear the marker on success) are all implemented, with +`runtime/tests.rs` covering the public API end to end. Mapping an inherited +marker into protocol recovery, a `pub(crate)` re-export of `coordinate()` +beyond `runtime`, and harness/command wiring remain future work tracked by the +`mutation-cursor-external-taint` and `mutation-cursor-runtime-coordinator` +plans. See also: [`mutation-trace-protocol.md`](mutation-trace-protocol.md), [`mutation-trace-store.md`](mutation-trace-store.md), [`mutation-trace-external-taint.md`](mutation-trace-external-taint.md) -(the `/sce/mutation-cursor-tainted` marker primitive, to be wired into +(the `/sce/mutation-cursor-tainted` write-ahead fence armed by `coordinate()`), [`checkout-identity.md`](checkout-identity.md). diff --git a/context/context-map.md b/context/context-map.md index 73f80d30..8054695a 100644 --- a/context/context-map.md +++ b/context/context-map.md @@ -27,8 +27,8 @@ Feature/domain context: - `context/cli/mutation-trace-revision-refinement.md` (the Quint `revision: int` → Rust `WorktreeState::revision: u64` bounded-integer refinement: the private `next_revision` checked-arithmetic helper `commit`/`taint`/`abandon`/`recover` all route through instead of a raw `+ 1`, so a worktree at `revision: u64::MAX` is a guarded no-op/rejection rather than a silent wrap to `0`) - `context/cli/mutation-trace-quint-connect.md` (`#[cfg(test)]`-only Quint Connect model-based-testing harness in `cli/src/services/mutation_trace/mbt/` continuously checking `protocol.rs` against `spec/mutation_cursor.qnt`: the verification-only `mbtAction`/`MbtAction` record-payload transport excluded from comparison, the operation-identity-vs-`MbtStutter` distinction on guarded/no-op branches with its two deterministic regressions, finite ID mapping, the AC5 comparable-state field list, `randomPrepare` staying a single `step` branch, deterministic/generated (500×30, seed-reproducible) test coverage, and the two Nix checks — generic `checks.cli-tests` and dedicated `checks.mutation-trace-quint-connect` — that both require the pinned Quint binary plus the top-level `spec/` directory in `workspaceSrc`'s Nix fileset) - `context/cli/mutation-trace-store.md` (durable persistence for the mutation-cursor protocol in `cli/src/services/mutation_trace/store.rs`, built by the `mutation-cursor-store-persistence` plan: the one-directional `protocol.rs` -> `DurableTransition::between` (pure structural diff) -> `store.rs` (SQL translation) -> `RepositoryAgentTraceDb` boundary; migration `003_mutation_trace_protocol.sql`'s five tables; `AttemptState`/`external_taint` non-persistence; the 8-byte big-endian `BLOB` revision encoding plus explicit non-`Debug` enum codecs; the bounded hot-path `load_worktree` read vs. the cold-path `load_mutation_event`; `commit`'s single-`BEGIN IMMEDIATE` CAS batch via `TursoDb::execute_transactional_cas_batch`, distinguishing `Conflict`/retryable-transient/deterministic-`Err` outcomes; and the store's non-goals — no Git/filesystem I/O, no attribution/boundary-kind decisions, no retry-after-`Conflict` loop, no terminal-scope garbage collection) -- `context/cli/mutation-trace-runtime-coordinator.md` (imperative-shell runtime layer in `cli/src/services/mutation_trace/runtime/`, built by the `mutation-cursor-runtime-coordinator` plan: the per-worktree OS advisory lock, `runtime::worktree_lock::WorktreeLock::acquire(git_dir, timeout)` at `/sce/mutation-cursor.lock`, bounded `try_lock()` polling with a distinct matchable timeout error and RAII release, and its distinction from the separate checkout-identity-creation lock; and the isolated Git snapshot service, `runtime::git_snapshot::GitSnapshotService` (`capture_tree`/`pin_tree`/`diff_trees`), writing tree/blob objects into the repository's normal object database and pinning durable ones via a create-only, idempotent `refs/sce/mutation-cursor//` ref rather than a private object store; and `runtime::coordinator`'s protocol-integration pipeline (`RuntimeBoundary`/`CoordinateOutcome`/`CoordinateError`, the load → recover-if-needed → prepare/commit CAS-retry loop, and its own bounded snapshot-failure taint-retry loop) plus the public `coordinate(repository_root, db, boundary)` entrypoint that acquires the `WorktreeLock`, derives `WorktreeId` from `checkout::get_or_create_checkout_id`, and drives that pipeline under one held lock; `runtime/tests.rs` covers the public `coordinate()` API end to end against real linked worktrees and a real Agent Trace DB; a `pub(crate)` re-export of `coordinate()` beyond `runtime` and harness/command wiring remain future work) -- `context/cli/mutation-trace-external-taint.md` (the worktree-local mutation-cursor durability boundary in `cli/src/services/mutation_trace/runtime/external_taint.rs`, built by the `mutation-cursor-external-taint` plan: the `ExternalTaintMarker` primitive — `new(git_dir)`/`exists()`/`persist()`/`clear()` over an empty file at `/sce/mutation-cursor-tainted` whose existence is its entire state, `checkout::persist_checkout_id_inner`-style durability (`sync_data` plus best-effort `#[cfg(unix)]` parent-dir `sync_all`), idempotent persist/clear, `NotFound`-on-clear as success, no `Drop` deletion — as the concrete runtime refinement of the abstract `ProtocolState.external_taint`; currently `#![allow(dead_code)]` and not yet armed by `coordinate()`, which the same plan wires in write-ahead before Agent Trace DB acquisition) +- `context/cli/mutation-trace-runtime-coordinator.md` (imperative-shell runtime layer in `cli/src/services/mutation_trace/runtime/`, built by the `mutation-cursor-runtime-coordinator` plan: the per-worktree OS advisory lock, `runtime::worktree_lock::WorktreeLock::acquire(git_dir, timeout)` at `/sce/mutation-cursor.lock`, bounded `try_lock()` polling with a distinct matchable timeout error and RAII release, and its distinction from the separate checkout-identity-creation lock; and the isolated Git snapshot service, `runtime::git_snapshot::GitSnapshotService` (`capture_tree`/`pin_tree`/`diff_trees`), writing tree/blob objects into the repository's normal object database and pinning durable ones via a create-only, idempotent `refs/sce/mutation-cursor//` ref rather than a private object store; and `runtime::coordinator`'s protocol-integration pipeline (`RuntimeBoundary`/`CoordinateOutcome`/`CoordinateError`, the load → recover-if-needed → prepare/commit CAS-retry loop, and its own bounded snapshot-failure taint-retry loop) plus the public `coordinate(repository_root, boundary, open_db)` entrypoint that acquires the `WorktreeLock`, arms the external-taint write-ahead fence, derives `WorktreeId` from `checkout::get_or_create_checkout_id`, invokes the caller-supplied `open_db` provider so DB acquisition falls inside that fence, drives the pipeline under one held lock, and clears the marker only on success; `runtime/tests.rs` covers the public `coordinate()` API end to end against real linked worktrees and a real Agent Trace DB; mapping an inherited marker into protocol recovery, a `pub(crate)` re-export of `coordinate()` beyond `runtime`, and harness/command wiring remain future work) +- `context/cli/mutation-trace-external-taint.md` (the worktree-local mutation-cursor durability boundary in `cli/src/services/mutation_trace/runtime/external_taint.rs`, built by the `mutation-cursor-external-taint` plan: the `ExternalTaintMarker` primitive — `new(git_dir)`/`exists()`/`persist()`/`clear()` over an empty file at `/sce/mutation-cursor-tainted` whose existence is its entire state, `checkout::persist_checkout_id_inner`-style durability (`sync_data` plus best-effort `#[cfg(unix)]` parent-dir `sync_all`), idempotent persist/clear, `NotFound`-on-clear as success, no `Drop` deletion — as the concrete runtime refinement of the abstract `ProtocolState.external_taint`; armed by the reshaped `coordinate()` entrypoint write-ahead after the `WorktreeLock` and before Agent Trace DB acquisition (a caller-supplied DB provider closure), cleared only on a successful `CoordinateOutcome`, with dedicated fail-closed `CoordinateError::ExternalTaintMarker`/`AgentTraceDbUnavailable` variants; mapping an inherited marker into `database_failure` recovery is the next task in the plan) - `context/sce/cli-exit-code-contract.md` (stable class-based `sce` process exit-code contract in `cli/src/app.rs`, so automation can branch on failure category without parsing error text) - `context/sce/cli-error-code-taxonomy.md` (stable user-facing `SCE-ERR-*` diagnostic code classes rendered by `cli/src/app.rs`, complementing the numeric exit-code classes) - `context/sce/cli-stdout-stderr-contract.md` (implemented stream contract in `cli/src/app.rs`: command payloads on stdout only, redacted diagnostics on stderr) diff --git a/context/plans/mutation-cursor-external-taint.md b/context/plans/mutation-cursor-external-taint.md index ddd93f49..ee39105c 100644 --- a/context/plans/mutation-cursor-external-taint.md +++ b/context/plans/mutation-cursor-external-taint.md @@ -322,8 +322,70 @@ local choices, not new requirements. `./scripts/run-cli-cargo.sh fmt --manifest-path cli/Cargo.toml -- --check`. - Context synchronization: synced -- [ ] T02: `Arm the write-ahead fence around the full runtime boundary, DB acquisition included` (status:todo) +- [x] T02: `Arm the write-ahead fence around the full runtime boundary, DB acquisition included` (status:done) - Task ID: T02 + - Completed: 2026-08-30 + - Files changed: + - `cli/src/services/mutation_trace/runtime/coordinator.rs` — reshaped the + public `coordinate()` to take a caller-supplied + `open_db: impl FnOnce() -> anyhow::Result` provider + instead of `&RepositoryAgentTraceDb`; `coordinate_inner` now resolves + `git_dir` → acquires `WorktreeLock` → constructs `ExternalTaintMarker` → + reads `inherited_external_taint = marker.exists()?` → `marker.persist()?` → + runs the new `coordinate_protected` (checkout identity → `open_db()` → + `GitSnapshotService` → `coordinate_boundary`) → `marker.clear()?` only on + `Ok`; added `ExternalTaintOperation { Inspect, Persist, Clear }` and the + `CoordinateError::ExternalTaintMarker { operation, source }` and + `CoordinateError::AgentTraceDbUnavailable(_)` variants plus their `Display` + arms; `coordinate_boundary` gained an unused `_inherited_external_taint: + bool` param (consumed in T03); updated the `two_threads_…_serialize` + `coordinate_inner` call site and added six inline tests + (`public_coordinate_clears_marker_on_success`, + `public_coordinate_leaves_marker_after_a_snapshot_failure`, + `public_coordinate_leaves_marker_after_a_non_snapshot_failure`, + `public_coordinate_fails_closed_when_the_marker_cannot_be_armed` + (`ExternalTaintOperation::Persist` path), + `public_coordinate_fails_closed_when_marker_inspection_fails` + (`ExternalTaintOperation::Inspect` path — holds the runtime lock, swaps + `/sce` from a directory to a regular file at the worker's + lock-contention point so `marker.exists()` hits a deterministic `ENOTDIR` + with no permission changes, asserts the DB provider is never called), + `public_coordinate_leaves_marker_when_the_db_provider_fails`). + - `cli/src/services/mutation_trace/runtime/tests.rs` — updated all five + `coordinate()` call sites to pass a provider closure + (`|| RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&path)`). + - Result: The external-taint fence now spans the whole protected runtime + section. `coordinate()` arms the worktree-local marker after `WorktreeLock` + acquisition and before checkout-identity, DB-provider, snapshot, and + protocol work; a successful `CoordinateOutcome` clears it; every failure + after arming (snapshot failure, DB provider `Err`, revision exhaustion, CAS + exhaustion, scope conflict, DB read/write, unexpected) returns with the + marker present; both the `ExternalTaintOperation::Inspect` and + `ExternalTaintOperation::Persist` marker-I/O failure paths fail closed with + the `CoordinateError::ExternalTaintMarker` error before any checkout, + DB-provider, snapshot, or protocol work — each proven by a dedicated + deterministic regression test (AC13). `MutationTraceStore` and + `protocol.rs` are untouched; `coordinate_boundary` still receives an + already-open `&RepositoryAgentTraceDb`. Inherited-taint recovery mapping is + still T03 (`_inherited_external_taint` is threaded but unused). + - Verify: + - `test ...services::mutation_trace::runtime::coordinator` — PASS (21 passed, + 0 failed; `public_coordinate_fails_closed_when_marker_inspection_fails` + also re-run 5× for determinism). + - `test ...services::mutation_trace::runtime::` — PASS (44 passed, 0 failed). + - `clippy --all-targets -- -D warnings` — PASS (clean). + - `fmt -- --check` — PASS (clean). + - Context impact: docs-update-needed. Reshapes the public `coordinate()` + entrypoint (caller-supplied DB provider, `WorktreeLock`/marker ownership, + arm-before-DB-acquisition ordering, marker clear on success only) and adds + two `CoordinateError` variants with fail-closed semantics. Affected durable + context: `context/cli/mutation-trace-runtime-coordinator.md` (reshaped + entrypoint, marker arming, DB provider, new error variants, safety + invariant), `context/cli/mutation-trace-protocol.md` (write-ahead marker as + concrete refinement armed before DB acquisition), `context/context-map.md` + (coordinator annotation refresh), `spec/mutation_cursor.md` (write-ahead + timing of the concrete `externalTaint` refinement). Matches the plan's + Context sync section. No protocol semantics, DB state, or migration changed. - Scope: In — `runtime/coordinator.rs`: reshape the public `coordinate()` so it no longer receives an already-open `&RepositoryAgentTraceDb` but a caller-supplied DB provider (`impl FnOnce() -> anyhow::Result` @@ -359,7 +421,7 @@ local choices, not new requirements. `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml services::mutation_trace::runtime::`; `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml --all-targets -- -D warnings`; `./scripts/run-cli-cargo.sh fmt --manifest-path cli/Cargo.toml -- --check`. - - Context synchronization: pending + - Context synchronization: synced - [ ] T03: `Map inherited marker into protocol recovery` (status:todo) - Task ID: T03 diff --git a/spec/mutation_cursor.md b/spec/mutation_cursor.md index 7bb5e0d7..957c938c 100644 --- a/spec/mutation_cursor.md +++ b/spec/mutation_cursor.md @@ -45,7 +45,7 @@ Database unavailability is different. `databaseFailure(worktree)` changes only: externalTaint: Set[WorktreeId] ``` -`externalTaint` is the abstract external durability boundary: conceptually, the filesystem `TAINTED` marker that can survive an unavailable database. It is not a database row and does not model marker paths or filesystem syscalls. +`externalTaint` is the abstract external durability boundary: conceptually, the filesystem `TAINTED` marker that can survive an unavailable database. It is not a database row and does not model marker paths or filesystem syscalls. Its concrete refinement in the CLI is the worktree-local marker file `/sce/mutation-cursor-tainted`, armed write-ahead at the start of the protected runtime section — before Agent Trace DB acquisition — and cleared only after a proven durable completion; it becomes protocol `externalTaint` (overlaid onto `databaseFailure` recovery) only when a later invocation inherits it. See `context/cli/mutation-trace-external-taint.md`. Thus the model does **not** perform this contradictory transition: From 63e825518e4456afe62644f3c6509e095a205f99 Mon Sep 17 00:00:00 2001 From: David Abram Date: Sun, 30 Aug 2026 18:15:06 +0200 Subject: [PATCH 4/8] tests: Fix sync terminal status request expectation Update the sync test fixture to return terminal stream counts and assert that only messages trigger one ingestion batch request. This verifies terminal statuses do not resend batches. Co-authored-by: SCE --- cli/src/services/sync/sync.rs | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/cli/src/services/sync/sync.rs b/cli/src/services/sync/sync.rs index 32f1b35d..25a2b0ca 100644 --- a/cli/src/services/sync/sync.rs +++ b/cli/src/services/sync/sync.rs @@ -1139,13 +1139,11 @@ mod tests { seed_one_row_per_stream(&db); let server = TestHttpServer::start(); - server.queue_response(CannedResponse::json(200, &state_response(0, 0, 0, 0))); - for _ in 0..4 { - server.queue_response(CannedResponse::json( - 404, - &json!({"message": "unknown ingestion route"}), - )); - } + server.queue_response(CannedResponse::json(200, &state_response(0, 1, 1, 1))); + server.queue_response(CannedResponse::json( + 404, + &json!({"message": "unknown ingestion route"}), + )); let client = test_client(&server); let error = run_sync_against( @@ -1179,9 +1177,9 @@ mod tests { .iter() .filter(|request| request.path == "/agent-trace/ingestion/batch") .count(); - assert!( - (1..=4).contains(&batch_count), - "terminal /batch statuses must not resend batches; observed {batch_count}" + assert_eq!( + batch_count, 1, + "only `messages` should send a /batch request, and a terminal status must not resend it; observed {batch_count}" ); remove_test_db(&db_path); From 724462c99a23545d8b24308a07304ff954497659 Mon Sep 17 00:00:00 2001 From: David Abram Date: Sun, 30 Aug 2026 18:32:59 +0200 Subject: [PATCH 5/8] runtime+context: Recover inherited external taint Map an inherited external-taint marker onto `database_failure` for each freshly loaded projection so recovery rebaselines against the already captured tree, retries losing recovery CAS operations without recapturing, and clears the pending overlay once recovery lands. Add focused pipeline coverage for first-use recovery and boundary retries, and document the recovery lifecycle and completed task. Plan: mutation-cursor-external-taint (T03) Co-authored-by: SCE --- .../mutation_trace/runtime/coordinator.rs | 295 +++++++++++++++++- context/cli/mutation-trace-external-taint.md | 37 ++- .../cli/mutation-trace-runtime-coordinator.md | 24 +- context/context-map.md | 4 +- .../plans/mutation-cursor-external-taint.md | 66 +++- 5 files changed, 397 insertions(+), 29 deletions(-) diff --git a/cli/src/services/mutation_trace/runtime/coordinator.rs b/cli/src/services/mutation_trace/runtime/coordinator.rs index 29621336..ff48f3f8 100644 --- a/cli/src/services/mutation_trace/runtime/coordinator.rs +++ b/cli/src/services/mutation_trace/runtime/coordinator.rs @@ -244,8 +244,33 @@ fn coordinate_boundary( capture: &C, worktree_id: &WorktreeId, boundary: &RuntimeBoundary, - _inherited_external_taint: bool, + inherited_external_taint: bool, ) -> Result { + coordinate_boundary_inner( + db, + capture, + worktree_id, + boundary, + inherited_external_taint, + |_attempt| {}, + |_attempt| {}, + ) +} + +fn coordinate_boundary_inner( + db: &RepositoryAgentTraceDb, + capture: &C, + worktree_id: &WorktreeId, + boundary: &RuntimeBoundary, + inherited_external_taint: bool, + mut after_load: AfterLoad, + mut after_recovery: AfterRecovery, +) -> Result +where + C: SnapshotCapture, + AfterLoad: FnMut(u32), + AfterRecovery: FnMut(u32), +{ let store = MutationTraceStore::new(db); let observed_tree = match capture.capture().and_then(|tree| { @@ -270,7 +295,9 @@ fn coordinate_boundary( let scope_ref = types::boundary_scope(&type_boundary); let event_key_ref = types::boundary_event_key(&type_boundary); - for _ in 0..MAX_CAS_RETRY_ATTEMPTS { + let mut external_taint_pending = inherited_external_taint; + + for attempt_index in 0..MAX_CAS_RETRY_ATTEMPTS { let Some(projection) = store .load_worktree(worktree_id, scope_ref.as_ref(), event_key_ref.as_ref()) .map_err(CoordinateError::Other)? @@ -280,8 +307,14 @@ fn coordinate_boundary( ))); }; + after_load(attempt_index); + let mut state = projection.into_protocol_state(); + if external_taint_pending { + state = protocol::database_failure(&state, worktree_id); + } + if needs_recovery(&state, worktree_id) { let recovered = protocol::recover(&state, worktree_id, observed_tree.clone()); let Some(transition) = DurableTransition::between(&state, &recovered, worktree_id) @@ -299,7 +332,11 @@ fn coordinate_boundary( }; match store.commit(&transition).map_err(CoordinateError::Other)? { - CasResult::Applied => state = recovered, + CasResult::Applied => { + state = recovered; + external_taint_pending = false; + after_recovery(attempt_index); + } CasResult::Conflict => continue, } } @@ -1715,4 +1752,256 @@ mod tests { remove_test_repo(&repo_root); } + + #[test] + fn inherited_external_taint_recovers_once_before_the_boundary() { + let (db, db_path) = test_db("t03-inherited-recovers-once"); + let worktree = WorktreeId("wt-1".to_string()); + let scope = ScopeId("scope-live".to_string()); + let capture = FakeSnapshotCapture::new(TreeId("tree-a".to_string())); + + coordinate_boundary( + &db, + &capture, + &worktree, + &RuntimeBoundary::Start { + scope: scope.clone(), + event: EventId("evt-start".to_string()), + actor_kind: ActorKind::ClaudeCode, + }, + false, + ) + .expect("start should establish the baseline and the live scope"); + + let advance_capture = FakeSnapshotCapture::new(TreeId("tree-b".to_string())); + let outcome = coordinate_boundary( + &db, + &advance_capture, + &worktree, + &RuntimeBoundary::Advance { + scope: scope.clone(), + event: EventId("evt-advance".to_string()), + actor_kind: ActorKind::ClaudeCode, + }, + true, + ) + .expect("an inherited external-taint marker must recover, then process the boundary"); + + assert!( + outcome.mutation_event.is_none(), + "no mutation evidence may span the interval the inherited marker fenced off" + ); + assert_eq!( + outcome.revision, 3, + "one recovery transition (rev 1 -> 2), then the triggering boundary (rev 2 -> 3)" + ); + assert_eq!( + advance_capture.capture_call_count(), + 1, + "recovery and the triggering boundary must share the single already-captured snapshot" + ); + assert_eq!(advance_capture.pin_call_count(), 1); + + let store = MutationTraceStore::new(&db); + let projection = store + .load_worktree(&worktree, Some(&scope), None) + .expect("load should succeed") + .expect("worktree should exist"); + assert!(!projection.worktree_state.tainted); + assert!(!projection.worktree_state.needs_rebaseline); + assert_eq!(projection.worktree_state.failure_kind, FailureKind::Healthy); + assert_eq!( + projection.worktree_state.cursor_tree, + TreeId("tree-b".to_string()), + "recovery rebaselines the cursor to this invocation's observed tree" + ); + assert_eq!( + projection.scopes.get(&scope).map(|s| s.status), + Some(ScopeStatus::Abandoned), + "inherited-taint recovery abandons the live scopes the fenced interval made untrustworthy" + ); + + remove_test_db(&db_path); + } + + #[test] + fn inherited_external_taint_with_no_worktree_row_baselines_without_evidence() { + let (db, db_path) = test_db("t03-inherited-no-row"); + let worktree = WorktreeId("wt-1".to_string()); + let capture = FakeSnapshotCapture::new(TreeId("tree-a".to_string())); + + let outcome = coordinate_boundary(&db, &capture, &worktree, &RuntimeBoundary::Flush, true) + .expect("a first-ever invocation carrying an inherited marker must still succeed"); + + assert!( + outcome.mutation_event.is_none(), + "a worktree with no prior durable row cannot produce evidence for the unknown interval" + ); + assert_eq!( + outcome.revision, 1, + "the freshly initialized worktree is baselined against the observed tree, then conservatively recovered once" + ); + + let store = MutationTraceStore::new(&db); + let projection = store + .load_worktree(&worktree, None, None) + .expect("load should succeed") + .expect("the worktree row should now exist"); + assert!(!projection.worktree_state.tainted); + assert_eq!(projection.worktree_state.failure_kind, FailureKind::Healthy); + assert_eq!( + projection.worktree_state.cursor_tree, + TreeId("tree-a".to_string()) + ); + + remove_test_db(&db_path); + } + + #[test] + fn a_losing_recovery_cas_reinjects_external_taint_until_it_applies() { + let (db, db_path) = test_db("t03-recovery-cas-reinjection"); + let worktree = WorktreeId("wt-1".to_string()); + let scope = ScopeId("scope-live".to_string()); + let bootstrap = FakeSnapshotCapture::new(TreeId("tree-a".to_string())); + + coordinate_boundary( + &db, + &bootstrap, + &worktree, + &RuntimeBoundary::Start { + scope: scope.clone(), + event: EventId("evt-start".to_string()), + actor_kind: ActorKind::ClaudeCode, + }, + false, + ) + .expect("bootstrap start should establish the baseline and live scope"); + + let store = MutationTraceStore::new(&db); + let competing_scope = ScopeId("competing-scope".to_string()); + let interfered = Cell::new(false); + let capture = FakeSnapshotCapture::new(TreeId("tree-a".to_string())); + + let outcome = coordinate_boundary_inner( + &db, + &capture, + &worktree, + &RuntimeBoundary::Flush, + true, + |attempt| { + if attempt == 0 && !interfered.get() { + interfered.set(true); + commit_competing_advance( + &store, + &worktree, + &competing_scope, + "competing-event-1", + true, + ); + } + }, + |_attempt| {}, + ) + .expect("recovery must recompute past the losing CAS and still succeed"); + + assert_eq!( + capture.capture_call_count(), + 1, + "a recovery CAS conflict must not trigger a second Git snapshot" + ); + assert_eq!( + outcome.revision, 3, + "bootstrap start (1) + competing advance (2) + exactly one landed recovery (3)" + ); + + let projection = store + .load_worktree(&worktree, Some(&scope), None) + .expect("load should succeed") + .expect("worktree should exist"); + assert!(!projection.worktree_state.tainted); + assert_eq!( + projection.scopes.get(&scope).map(|s| s.status), + Some(ScopeStatus::Abandoned), + "the re-injected recovery still abandons the fenced-off live scope" + ); + + let competing_projection = store + .load_worktree(&worktree, Some(&competing_scope), None) + .expect("load should succeed") + .expect("worktree should exist"); + assert_eq!( + competing_projection + .scopes + .get(&competing_scope) + .map(|s| s.status), + Some(ScopeStatus::Abandoned) + ); + + remove_test_db(&db_path); + } + + #[test] + fn a_landed_recovery_clears_the_flag_so_a_boundary_cas_retry_does_not_re_recover() { + let (db, db_path) = test_db("t03-flag-clears-after-recovery"); + let worktree = WorktreeId("wt-1".to_string()); + let scope = ScopeId("scope-live".to_string()); + let bootstrap = FakeSnapshotCapture::new(TreeId("tree-a".to_string())); + + coordinate_boundary( + &db, + &bootstrap, + &worktree, + &RuntimeBoundary::Start { + scope: scope.clone(), + event: EventId("evt-start".to_string()), + actor_kind: ActorKind::ClaudeCode, + }, + false, + ) + .expect("bootstrap start should establish the baseline and live scope"); + + let store = MutationTraceStore::new(&db); + let competing_scope = ScopeId("competing-scope".to_string()); + let interfered = Cell::new(false); + let capture = FakeSnapshotCapture::new(TreeId("tree-a".to_string())); + capture.push_success(TreeId("tree-b".to_string())); + + let outcome = coordinate_boundary_inner( + &db, + &capture, + &worktree, + &RuntimeBoundary::Advance { + scope: scope.clone(), + event: EventId("evt-advance".to_string()), + actor_kind: ActorKind::ClaudeCode, + }, + true, + |_attempt| {}, + |attempt| { + if attempt == 0 && !interfered.get() { + interfered.set(true); + commit_competing_advance( + &store, + &worktree, + &competing_scope, + "competing-event-1", + true, + ); + } + }, + ) + .expect("the boundary CAS retry after a landed recovery must still succeed"); + + assert_eq!( + capture.capture_call_count(), + 1, + "neither the recovery nor the boundary retry may take a second snapshot" + ); + assert_eq!( + outcome.revision, 4, + "start (1) + one landed recovery (2) + competing advance (3) + the retried boundary (4); a re-triggered recovery on the retry would land at 5" + ); + + remove_test_db(&db_path); + } } diff --git a/context/cli/mutation-trace-external-taint.md b/context/cli/mutation-trace-external-taint.md index 912ba208..455625c9 100644 --- a/context/cli/mutation-trace-external-taint.md +++ b/context/cli/mutation-trace-external-taint.md @@ -99,22 +99,39 @@ it. returned `Err` after the marker was armed. The marker is intentionally left in place; the lower-level `coordinate_boundary` pipeline is never entered. -### Inherited taint, not yet consumed - -`coordinate()` computes `inherited_external_taint` from `marker.exists()` and -threads it to `coordinate_boundary` as `_inherited_external_taint`, unused so -far. Mapping an inherited marker into `protocol::database_failure` recovery -against the single captured snapshot — and re-injecting it across a losing -recovery CAS — is the next task in the same plan. `WorktreeProjection:: -into_protocol_state()` still returns an empty `external_taint`; the filesystem -overlay is applied only by runtime code. +### Inherited taint recovery + +`coordinate()` reads `inherited_external_taint` from `marker.exists()` before +arming the marker and threads it into `coordinate_boundary`, which seeds an +invocation-local `external_taint_pending` flag. While that flag is set, each +freshly loaded projection is overlaid with `protocol::database_failure` before +the recovery check, so `protocol::recover` performs exactly one conservative +recovery against the single already-captured snapshot — cursor rebaselined to +the observed tree, revision advanced once, live scopes abandoned, no +`MutationEvent` for the fenced interval — and the triggering boundary is then +processed against the recovered state. A worktree with no durable row yet is +baselined against the observed tree first, then recovered the same way. + +The overlay is never persisted: `DurableTransition` ignores `external_taint` and +`WorktreeProjection::into_protocol_state()` always returns it empty, so passing +the overlaid state as the CAS baseline is safe. A losing recovery CAS keeps +`external_taint_pending` set, so the next reload re-injects the overlay and +recomputes until recovery lands or the retry budget is spent; once it lands the +flag clears, so a later boundary-CAS retry in the same invocation does not +re-trigger recovery. The filesystem marker is never touched here — +`coordinate()`'s success path still owns clearing it. Inline `coordinator.rs` tests drive the public `coordinate()` against real repositories: a successful call clears the marker; a snapshot failure, a non-snapshot failure (revision-exhausted recovery), and a DB provider returning `Err` each leave the marker present; and both marker-I/O failure operations — `Inspect` (a deterministic `ENOTDIR`, no permission changes) and `Persist` — -fail the call closed before the DB provider is ever invoked. +fail the call closed before the DB provider is ever invoked. Pipeline tests +cover the inherited-taint overlay directly: one conservative recovery sharing +the single snapshot before the boundary, a first-ever inherited marker with no +worktree row baselining without evidence, a losing recovery CAS re-injecting the +overlay until it applies, and the flag clearing so a post-recovery boundary-CAS +retry does not recover again. ## On-disk layout addition diff --git a/context/cli/mutation-trace-runtime-coordinator.md b/context/cli/mutation-trace-runtime-coordinator.md index 298f7adb..66af11f0 100644 --- a/context/cli/mutation-trace-runtime-coordinator.md +++ b/context/cli/mutation-trace-runtime-coordinator.md @@ -113,13 +113,13 @@ API end to end. Only harness/command wiring remains. (below) and return without touching the rest of the pipeline; on success, idempotently materialize the worktree row and, for hook boundaries, the scope row; then loop (bounded, `MAX_CAS_RETRY_ATTEMPTS = 5`, no backoff): - load durable state fresh, recover first if the worktree is tainted or needs - rebaseline (its own CAS commit, reusing the one captured tree as the - rebaseline target), then `prepare`/`commit` the triggering boundary against - that state (a second CAS commit) — reloading and recomputing from scratch - on `Conflict`, without ever re-capturing or re-pinning. A settled no-op - result (a stale, rejected, or replayed attempt) is a successful return, not - an error. + load durable state fresh, recover first if the worktree is tainted, needs + rebaseline, or inherited an external-taint marker (overlaid as + `database_failure`; its CAS commit reuses the one captured tree), then + `prepare`/`commit` the triggering boundary against that state (a second CAS + commit) — reloading and recomputing from scratch on `Conflict`, without ever + re-capturing or re-pinning. A settled no-op result (a stale, rejected, or + replayed attempt) is a successful return, not an error. A capture or pin failure is handled by its own bounded taint-retry loop: a fresh `load_worktree` on every iteration, always evaluated after the @@ -237,11 +237,11 @@ The lock, snapshot service, protocol-integration pipeline, and the public `coordinate()` entrypoint (resolve `git_dir` → `WorktreeLock` → arm the external-taint marker → checkout identity → caller-supplied DB provider → pipeline → clear the marker on success) are all implemented, with -`runtime/tests.rs` covering the public API end to end. Mapping an inherited -marker into protocol recovery, a `pub(crate)` re-export of `coordinate()` -beyond `runtime`, and harness/command wiring remain future work tracked by the -`mutation-cursor-external-taint` and `mutation-cursor-runtime-coordinator` -plans. +`runtime/tests.rs` covering the public API end to end; an inherited external-taint +marker is now overlaid onto `database_failure` recovery on the next invocation. A +`pub(crate)` re-export of `coordinate()` beyond `runtime` and harness/command +wiring remain future work tracked by the `mutation-cursor-external-taint` and +`mutation-cursor-runtime-coordinator` plans. See also: [`mutation-trace-protocol.md`](mutation-trace-protocol.md), [`mutation-trace-store.md`](mutation-trace-store.md), diff --git a/context/context-map.md b/context/context-map.md index 8054695a..3dee6821 100644 --- a/context/context-map.md +++ b/context/context-map.md @@ -27,8 +27,8 @@ Feature/domain context: - `context/cli/mutation-trace-revision-refinement.md` (the Quint `revision: int` → Rust `WorktreeState::revision: u64` bounded-integer refinement: the private `next_revision` checked-arithmetic helper `commit`/`taint`/`abandon`/`recover` all route through instead of a raw `+ 1`, so a worktree at `revision: u64::MAX` is a guarded no-op/rejection rather than a silent wrap to `0`) - `context/cli/mutation-trace-quint-connect.md` (`#[cfg(test)]`-only Quint Connect model-based-testing harness in `cli/src/services/mutation_trace/mbt/` continuously checking `protocol.rs` against `spec/mutation_cursor.qnt`: the verification-only `mbtAction`/`MbtAction` record-payload transport excluded from comparison, the operation-identity-vs-`MbtStutter` distinction on guarded/no-op branches with its two deterministic regressions, finite ID mapping, the AC5 comparable-state field list, `randomPrepare` staying a single `step` branch, deterministic/generated (500×30, seed-reproducible) test coverage, and the two Nix checks — generic `checks.cli-tests` and dedicated `checks.mutation-trace-quint-connect` — that both require the pinned Quint binary plus the top-level `spec/` directory in `workspaceSrc`'s Nix fileset) - `context/cli/mutation-trace-store.md` (durable persistence for the mutation-cursor protocol in `cli/src/services/mutation_trace/store.rs`, built by the `mutation-cursor-store-persistence` plan: the one-directional `protocol.rs` -> `DurableTransition::between` (pure structural diff) -> `store.rs` (SQL translation) -> `RepositoryAgentTraceDb` boundary; migration `003_mutation_trace_protocol.sql`'s five tables; `AttemptState`/`external_taint` non-persistence; the 8-byte big-endian `BLOB` revision encoding plus explicit non-`Debug` enum codecs; the bounded hot-path `load_worktree` read vs. the cold-path `load_mutation_event`; `commit`'s single-`BEGIN IMMEDIATE` CAS batch via `TursoDb::execute_transactional_cas_batch`, distinguishing `Conflict`/retryable-transient/deterministic-`Err` outcomes; and the store's non-goals — no Git/filesystem I/O, no attribution/boundary-kind decisions, no retry-after-`Conflict` loop, no terminal-scope garbage collection) -- `context/cli/mutation-trace-runtime-coordinator.md` (imperative-shell runtime layer in `cli/src/services/mutation_trace/runtime/`, built by the `mutation-cursor-runtime-coordinator` plan: the per-worktree OS advisory lock, `runtime::worktree_lock::WorktreeLock::acquire(git_dir, timeout)` at `/sce/mutation-cursor.lock`, bounded `try_lock()` polling with a distinct matchable timeout error and RAII release, and its distinction from the separate checkout-identity-creation lock; and the isolated Git snapshot service, `runtime::git_snapshot::GitSnapshotService` (`capture_tree`/`pin_tree`/`diff_trees`), writing tree/blob objects into the repository's normal object database and pinning durable ones via a create-only, idempotent `refs/sce/mutation-cursor//` ref rather than a private object store; and `runtime::coordinator`'s protocol-integration pipeline (`RuntimeBoundary`/`CoordinateOutcome`/`CoordinateError`, the load → recover-if-needed → prepare/commit CAS-retry loop, and its own bounded snapshot-failure taint-retry loop) plus the public `coordinate(repository_root, boundary, open_db)` entrypoint that acquires the `WorktreeLock`, arms the external-taint write-ahead fence, derives `WorktreeId` from `checkout::get_or_create_checkout_id`, invokes the caller-supplied `open_db` provider so DB acquisition falls inside that fence, drives the pipeline under one held lock, and clears the marker only on success; `runtime/tests.rs` covers the public `coordinate()` API end to end against real linked worktrees and a real Agent Trace DB; mapping an inherited marker into protocol recovery, a `pub(crate)` re-export of `coordinate()` beyond `runtime`, and harness/command wiring remain future work) -- `context/cli/mutation-trace-external-taint.md` (the worktree-local mutation-cursor durability boundary in `cli/src/services/mutation_trace/runtime/external_taint.rs`, built by the `mutation-cursor-external-taint` plan: the `ExternalTaintMarker` primitive — `new(git_dir)`/`exists()`/`persist()`/`clear()` over an empty file at `/sce/mutation-cursor-tainted` whose existence is its entire state, `checkout::persist_checkout_id_inner`-style durability (`sync_data` plus best-effort `#[cfg(unix)]` parent-dir `sync_all`), idempotent persist/clear, `NotFound`-on-clear as success, no `Drop` deletion — as the concrete runtime refinement of the abstract `ProtocolState.external_taint`; armed by the reshaped `coordinate()` entrypoint write-ahead after the `WorktreeLock` and before Agent Trace DB acquisition (a caller-supplied DB provider closure), cleared only on a successful `CoordinateOutcome`, with dedicated fail-closed `CoordinateError::ExternalTaintMarker`/`AgentTraceDbUnavailable` variants; mapping an inherited marker into `database_failure` recovery is the next task in the plan) +- `context/cli/mutation-trace-runtime-coordinator.md` (imperative-shell runtime layer in `cli/src/services/mutation_trace/runtime/`, built by the `mutation-cursor-runtime-coordinator` plan: the per-worktree OS advisory lock, `runtime::worktree_lock::WorktreeLock::acquire(git_dir, timeout)` at `/sce/mutation-cursor.lock`, bounded `try_lock()` polling with a distinct matchable timeout error and RAII release, and its distinction from the separate checkout-identity-creation lock; and the isolated Git snapshot service, `runtime::git_snapshot::GitSnapshotService` (`capture_tree`/`pin_tree`/`diff_trees`), writing tree/blob objects into the repository's normal object database and pinning durable ones via a create-only, idempotent `refs/sce/mutation-cursor//` ref rather than a private object store; and `runtime::coordinator`'s protocol-integration pipeline (`RuntimeBoundary`/`CoordinateOutcome`/`CoordinateError`, the load → recover-if-needed → prepare/commit CAS-retry loop, and its own bounded snapshot-failure taint-retry loop) plus the public `coordinate(repository_root, boundary, open_db)` entrypoint that acquires the `WorktreeLock`, arms the external-taint write-ahead fence, derives `WorktreeId` from `checkout::get_or_create_checkout_id`, invokes the caller-supplied `open_db` provider so DB acquisition falls inside that fence, drives the pipeline under one held lock, and clears the marker only on success; `runtime/tests.rs` covers the public `coordinate()` API end to end against real linked worktrees and a real Agent Trace DB; an inherited external-taint marker is overlaid onto `protocol::database_failure` recovery on the next invocation, against the single captured snapshot and re-injected across a losing recovery CAS; a `pub(crate)` re-export of `coordinate()` beyond `runtime` and harness/command wiring remain future work) +- `context/cli/mutation-trace-external-taint.md` (the worktree-local mutation-cursor durability boundary in `cli/src/services/mutation_trace/runtime/external_taint.rs`, built by the `mutation-cursor-external-taint` plan: the `ExternalTaintMarker` primitive — `new(git_dir)`/`exists()`/`persist()`/`clear()` over an empty file at `/sce/mutation-cursor-tainted` whose existence is its entire state, `checkout::persist_checkout_id_inner`-style durability (`sync_data` plus best-effort `#[cfg(unix)]` parent-dir `sync_all`), idempotent persist/clear, `NotFound`-on-clear as success, no `Drop` deletion — as the concrete runtime refinement of the abstract `ProtocolState.external_taint`; armed by the reshaped `coordinate()` entrypoint write-ahead after the `WorktreeLock` and before Agent Trace DB acquisition (a caller-supplied DB provider closure), cleared only on a successful `CoordinateOutcome`, with dedicated fail-closed `CoordinateError::ExternalTaintMarker`/`AgentTraceDbUnavailable` variants; an inherited marker seeds an invocation-local `external_taint_pending` flag that overlays `protocol::database_failure` onto each freshly loaded projection so `recover` runs once against the captured snapshot, held across a losing recovery CAS and cleared once it lands) - `context/sce/cli-exit-code-contract.md` (stable class-based `sce` process exit-code contract in `cli/src/app.rs`, so automation can branch on failure category without parsing error text) - `context/sce/cli-error-code-taxonomy.md` (stable user-facing `SCE-ERR-*` diagnostic code classes rendered by `cli/src/app.rs`, complementing the numeric exit-code classes) - `context/sce/cli-stdout-stderr-contract.md` (implemented stream contract in `cli/src/app.rs`: command payloads on stdout only, redacted diagnostics on stderr) diff --git a/context/plans/mutation-cursor-external-taint.md b/context/plans/mutation-cursor-external-taint.md index ee39105c..b2517fbd 100644 --- a/context/plans/mutation-cursor-external-taint.md +++ b/context/plans/mutation-cursor-external-taint.md @@ -423,8 +423,70 @@ local choices, not new requirements. `./scripts/run-cli-cargo.sh fmt --manifest-path cli/Cargo.toml -- --check`. - Context synchronization: synced -- [ ] T03: `Map inherited marker into protocol recovery` (status:todo) +- [x] T03: `Map inherited marker into protocol recovery` (status:done) - Task ID: T03 + - Completed: 2026-08-30 + - Files changed: + - `cli/src/services/mutation_trace/runtime/coordinator.rs` — renamed + `coordinate_boundary`'s `_inherited_external_taint` param to + `inherited_external_taint` and split the body into a 5-arg + `coordinate_boundary` wrapper plus a private + `coordinate_boundary_inner(.., after_load: FnMut(u32), after_recovery: FnMut(u32))` + test seam (mirroring the existing `coordinate_inner` / + `run_taint_retry_loop_inner` seam precedent; production passes no-ops). + Seeded `let mut external_taint_pending = inherited_external_taint;` before + the CAS loop; after each `load_worktree` the loop overlays + `state = protocol::database_failure(&state, worktree_id)` while + `external_taint_pending`, so the existing `needs_recovery` / + `protocol::recover` path runs against the one already-captured + `observed_tree`. On recovery-CAS `Applied` the flag is cleared (and + `after_recovery` fires); on `Conflict` the existing `continue` keeps it set + so the next reload re-injects the overlay. Added four inline tests + (`inherited_external_taint_recovers_once_before_the_boundary`, + `inherited_external_taint_with_no_worktree_row_baselines_without_evidence`, + `a_losing_recovery_cas_reinjects_external_taint_until_it_applies`, + `a_landed_recovery_clears_the_flag_so_a_boundary_cas_retry_does_not_re_recover`). + - Result: An inherited external-taint marker (T02's `inherited_external_taint`) + now drives protocol recovery. `external_taint_pending` is invocation-local + and seeded from the inherited flag; while set, `protocol::database_failure` + is overlaid onto every freshly loaded projection before the recovery check, + so `protocol::recover` performs exactly one conservative recovery (cursor := + observed tree, revision += 1, external taint cleared, live scopes → + `Abandoned`, no `MutationEvent` for the fenced interval) against the single + captured snapshot, then the triggering boundary is processed against the + recovered state. The overlay is never persisted (`DurableTransition` ignores + `external_taint`; `into_protocol_state()` always returns it empty). A losing + recovery CAS re-injects the overlay on the next reload and recomputes until + `Applied` or retry exhaustion; once recovery lands, the flag is clear so a + later boundary-CAS retry in the same invocation does not re-trigger recovery. + A first-ever inherited marker with no durable worktree row is baselined + against the observed tree by the existing `initialize_worktree`, then + conservatively recovered once. The filesystem marker is never touched here — + `coordinate_inner`'s success path still owns clearing it. No `protocol.rs`, + `store.rs`, SQL, or migration change. + - Verify: + - `test ...services::mutation_trace::runtime::coordinator` — PASS (25 passed, + 0 failed). + - `test ...services::mutation_trace::runtime::` — PASS (48 passed, 0 failed). + - `clippy --all-targets -- -D warnings` — PASS (clean). + - `fmt -- --check` — PASS (clean, after `cargo fmt`). + - Context impact: docs-update-needed. Adds the invocation-local + `external_taint_pending` overlay onto `database_failure` and its + inherited-vs-armed recovery semantics to the coordinator pipeline; no + protocol semantics, DB state, error variants, or public signatures changed + beyond T02's already-recorded reshape. Affected durable context: + `context/cli/mutation-trace-runtime-coordinator.md` (inherited-taint overlay + onto `database_failure`, one-recovery-per-inherited-marker, flag lifecycle + across recovery-CAS conflict/apply), `context/cli/mutation-trace-protocol.md` + (the stale marker becomes protocol external taint only when inherited by a + later invocation), `spec/mutation_cursor.md` (same inherited-only promotion). + Matches the plan's Context sync section. + - Deviations: added the private `coordinate_boundary_inner` `after_load` / + `after_recovery` test seams — consistent with the existing + `coordinate_inner(on_lock_contention)` and + `run_taint_retry_loop_inner(after_load)` precedent — because deterministically + forcing a recovery-CAS conflict, and a boundary-CAS conflict after a landed + recovery, is otherwise only reachable through non-deterministic thread races. - Scope: In — `runtime/coordinator.rs`: invocation-local `external_taint_pending`, seeded from `inherited_external_taint` and threaded into the lower-level pipeline; when set, overlay `protocol::database_failure` @@ -451,7 +513,7 @@ local choices, not new requirements. - Verify: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml services::mutation_trace::runtime::coordinator`; `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml --all-targets -- -D warnings`; `./scripts/run-cli-cargo.sh fmt --manifest-path cli/Cargo.toml -- --check`. - - Context synchronization: pending + - Context synchronization: synced - [ ] T04: `Add restart/failure integration tests through coordinate()` (status:todo) - Task ID: T04 From 039c3cbdb06010d18efc0118194acf98868e5974 Mon Sep 17 00:00:00 2001 From: David Abram Date: Sun, 30 Aug 2026 19:03:23 +0200 Subject: [PATCH 6/8] runtime: Add public API tests for external-taint recovery Exercise external-taint marker fencing and conservative recovery through coordinate() so DB-open, snapshot, stale-marker, linked-worktree, and clear-failure paths cannot emit untrusted mutation evidence. Record the completed integration coverage for T04. Plan: mutation-cursor-external-taint (T04) Co-authored-by: SCE --- .../services/mutation_trace/runtime/tests.rs | 683 +++++++++++++++++- .../plans/mutation-cursor-external-taint.md | 58 +- 2 files changed, 737 insertions(+), 4 deletions(-) diff --git a/cli/src/services/mutation_trace/runtime/tests.rs b/cli/src/services/mutation_trace/runtime/tests.rs index 02d37d45..9d0ec9b9 100644 --- a/cli/src/services/mutation_trace/runtime/tests.rs +++ b/cli/src/services/mutation_trace/runtime/tests.rs @@ -11,9 +11,12 @@ use crate::services::agent_trace_storage::{ }; use crate::services::checkout::{read_checkout_id, resolve_git_dir}; use crate::services::mutation_trace::store::MutationTraceStore; -use crate::services::mutation_trace::types::{ActorKind, EventId, ScopeId}; +use crate::services::mutation_trace::types::{ + ActorKind, EventId, FailureKind, ScopeId, ScopeStatus, +}; -use super::coordinator::{coordinate, CoordinateError, RuntimeBoundary}; +use super::coordinator::{coordinate, CoordinateError, ExternalTaintOperation, RuntimeBoundary}; +use super::external_taint::ExternalTaintMarker; use super::git_snapshot::GitSnapshotService; use super::worktree_lock::WorktreeLock; @@ -269,3 +272,679 @@ fn a_snapshot_failure_then_recovery_cycle_runs_through_the_public_api() { cleanup(&repo_root); } + +#[test] +fn a_successful_coordinate_through_the_public_api_leaves_no_external_taint_marker() { + let repo_root = unique_path("public-success-no-marker"); + init_repo(&repo_root); + // The DB lives outside the worktree so it never perturbs the observed tree. + let db_path = unique_path("public-success-no-marker-db").join("agent-trace.db"); + RepositoryAgentTraceDb::new_at(&db_path).expect("the repository DB should open with schema"); + let git_dir = resolve_git_dir(&repo_root).expect("git dir should resolve"); + let marker = ExternalTaintMarker::new(&git_dir); + + let outcome = coordinate(&repo_root, &RuntimeBoundary::Flush, || { + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + }) + .expect("a first observation through the public entrypoint should succeed"); + assert_eq!( + outcome.revision, 0, + "a first-observation flush should not advance the revision" + ); + assert!( + !marker + .exists() + .expect("marker existence should resolve after a successful coordinate()"), + "a successful coordinate() must clear the external-taint marker it armed" + ); + + cleanup( + db_path + .parent() + .expect("the DB path has a parent directory"), + ); + cleanup(&repo_root); +} + +#[test] +#[allow(clippy::too_many_lines)] +fn a_db_open_failure_after_arming_leaves_the_marker_and_the_next_invocation_rebaselines_without_evidence( +) { + let repo_root = unique_path("public-db-open-failure-gap"); + init_repo(&repo_root); + // The DB lives outside the worktree so it never perturbs the observed tree. + let db_path = unique_path("public-db-open-failure-gap-db").join("agent-trace.db"); + RepositoryAgentTraceDb::new_at(&db_path).expect("the repository DB should open with schema"); + let git_dir = resolve_git_dir(&repo_root).expect("git dir should resolve"); + let marker = ExternalTaintMarker::new(&git_dir); + let ok_db = || RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path); + + // A trusted baseline at cursor A, then one exclusive AI edit A -> B. + let baseline = coordinate(&repo_root, &RuntimeBoundary::Flush, ok_db) + .expect("the baseline observation should materialize the worktree"); + let worktree_id = baseline.worktree_id.clone(); + let scope = ScopeId("scope-across-the-gap".to_string()); + coordinate( + &repo_root, + &RuntimeBoundary::Start { + scope: scope.clone(), + event: EventId("evt-start".to_string()), + actor_kind: ActorKind::ClaudeCode, + }, + ok_db, + ) + .expect("starting the scope should succeed"); + std::fs::write(repo_root.join("work.txt"), b"v1").expect("the A -> B edit should write"); + let advanced = coordinate( + &repo_root, + &RuntimeBoundary::Advance { + scope: scope.clone(), + event: EventId("evt-advance".to_string()), + actor_kind: ActorKind::ClaudeCode, + }, + ok_db, + ) + .expect("the advance should commit exactly one event"); + let tree_b = advanced.observed_tree.clone(); + assert!( + advanced.mutation_event.is_some(), + "the exclusive A -> B edit must land one trustworthy event before the gap" + ); + + // A boundary invocation whose DB provider fails after the marker is armed, + // then the working tree keeps moving during the lost interval: B -> C. + let db_failure = coordinate( + &repo_root, + &RuntimeBoundary::Flush, + || -> anyhow::Result { + Err(anyhow::anyhow!("simulated Agent Trace DB open failure")) + }, + ) + .expect_err("a failing DB provider must fail coordinate()"); + assert!( + matches!(db_failure, CoordinateError::AgentTraceDbUnavailable(_)), + "expected AgentTraceDbUnavailable, got {db_failure:?}" + ); + assert!( + marker.exists().expect("marker existence should resolve"), + "the armed marker must survive an invocation whose DB provider returned Err" + ); + std::fs::write(repo_root.join("work.txt"), b"v2-during-the-gap").expect("the B -> C edit"); + + // The next successful invocation rebaselines to C with no evidence for the gap. + let recovered = coordinate(&repo_root, &RuntimeBoundary::Flush, ok_db) + .expect("the follow-up invocation with a working provider should recover, then process its boundary"); + let tree_c = recovered.observed_tree.clone(); + assert_ne!( + tree_c, tree_b, + "the gap edit must have moved the observed tree" + ); + assert!( + recovered.mutation_event.is_none(), + "no mutation evidence may span the interval the DB-open failure fenced off" + ); + assert!( + !marker.exists().expect("marker existence should resolve"), + "the recovering invocation must clear the marker on success" + ); + let db = RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + .expect("reopening the DB for assertions should succeed"); + let store = MutationTraceStore::new(&db); + let projection = store + .load_worktree(&worktree_id, Some(&scope), None) + .expect("loading the worktree row should succeed") + .expect("the worktree row should exist"); + assert_eq!( + projection.worktree_state.cursor_tree, tree_c, + "recovery must rebaseline the cursor to the follow-up invocation's own observed tree" + ); + assert!(!projection.worktree_state.tainted); + assert_eq!( + projection.scopes.get(&scope).map(|s| s.status), + Some(ScopeStatus::Abandoned), + "a scope live across the fenced interval must be abandoned afterward" + ); + for revision in 1..=recovered.revision { + if let Some(event) = store + .load_mutation_event(&worktree_id, revision) + .expect("loading a mutation event should succeed") + { + assert_ne!( + event.after_tree, tree_c, + "no MutationEvent may treat an interval ending at the post-gap tree as one trustworthy AI-attributable interval" + ); + } + } + + cleanup( + db_path + .parent() + .expect("the DB path has a parent directory"), + ); + cleanup(&repo_root); +} + +#[test] +fn a_stale_marker_rebaselines_to_the_current_tree_abandons_scopes_then_processes_the_boundary() { + let repo_root = unique_path("public-stale-marker-rebaseline"); + init_repo(&repo_root); + // The DB lives outside the worktree so it never perturbs the observed tree. + let db_path = unique_path("public-stale-marker-rebaseline-db").join("agent-trace.db"); + RepositoryAgentTraceDb::new_at(&db_path).expect("the repository DB should open with schema"); + let git_dir = resolve_git_dir(&repo_root).expect("git dir should resolve"); + let marker = ExternalTaintMarker::new(&git_dir); + let ok_db = || RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path); + + // Trusted cursor A plus an active scope S. + let baseline = coordinate(&repo_root, &RuntimeBoundary::Flush, ok_db) + .expect("the baseline observation should materialize the worktree"); + let worktree_id = baseline.worktree_id.clone(); + let tree_a = baseline.observed_tree.clone(); + let scope = ScopeId("scope-stranded".to_string()); + coordinate( + &repo_root, + &RuntimeBoundary::Start { + scope: scope.clone(), + event: EventId("evt-start".to_string()), + actor_kind: ActorKind::ClaudeCode, + }, + ok_db, + ) + .expect("starting scope S should succeed"); + + // A crashed invocation: the marker was armed and never cleared. + marker + .persist() + .expect("simulating a crashed invocation that armed but never cleared the marker"); + + // The working tree moves on to C while the process was gone. + std::fs::write( + repo_root.join("stranded.txt"), + b"edited-while-the-process-was-gone", + ) + .expect("the A -> C edit should write"); + + // The next invocation inherits the stale marker. + let recovered = coordinate( + &repo_root, + &RuntimeBoundary::Advance { + scope: scope.clone(), + event: EventId("evt-advance".to_string()), + actor_kind: ActorKind::ClaudeCode, + }, + ok_db, + ) + .expect("an inherited marker must recover, then process the triggering boundary"); + let tree_c = recovered.observed_tree.clone(); + assert_ne!( + tree_c, tree_a, + "the working tree must have moved during the gap" + ); + assert!( + recovered.mutation_event.is_none(), + "no A -> C evidence may be emitted across the fenced interval" + ); + assert!( + !marker.exists().expect("marker existence should resolve"), + "a successful recovery must clear the inherited marker" + ); + + let db = RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + .expect("reopening the DB for assertions should succeed"); + let store = MutationTraceStore::new(&db); + let projection = store + .load_worktree(&worktree_id, Some(&scope), None) + .expect("loading the worktree row should succeed") + .expect("the worktree row should exist"); + assert_eq!( + projection.worktree_state.cursor_tree, tree_c, + "recovery must rebaseline the cursor to the current tree C" + ); + assert!(!projection.worktree_state.tainted); + assert_eq!( + projection.scopes.get(&scope).map(|s| s.status), + Some(ScopeStatus::Abandoned), + "the scope that was live across the gap must be abandoned during recovery" + ); + + // The triggering boundary was processed inside the inheriting invocation: + // a plain follow-up flush finds nothing left to recover or advance. + let stable = coordinate(&repo_root, &RuntimeBoundary::Flush, ok_db) + .expect("a plain flush after recovery should succeed"); + assert_eq!( + stable.revision, recovered.revision, + "recovery and the triggering boundary already completed in the inheriting invocation" + ); + assert!(stable.mutation_event.is_none()); + + cleanup( + db_path + .parent() + .expect("the DB path has a parent directory"), + ); + cleanup(&repo_root); +} + +#[test] +fn a_first_ever_failed_invocation_that_never_materialized_a_worktree_row_creates_no_evidence() { + let repo_root = unique_path("public-first-ever-failure"); + init_repo(&repo_root); + // The DB lives outside the worktree so it never perturbs the observed tree. + let db_path = unique_path("public-first-ever-failure-db").join("agent-trace.db"); + RepositoryAgentTraceDb::new_at(&db_path).expect("the repository DB should open with schema"); + let git_dir = resolve_git_dir(&repo_root).expect("git dir should resolve"); + let marker = ExternalTaintMarker::new(&git_dir); + + std::fs::write( + repo_root.join("pre-existing.txt"), + b"content before any observation", + ) + .expect("a pre-existing edit should write"); + + // The very first invocation fails opening the DB, after arming the marker. + let first = coordinate( + &repo_root, + &RuntimeBoundary::Flush, + || -> anyhow::Result { + Err(anyhow::anyhow!( + "simulated Agent Trace DB open failure on the first-ever invocation" + )) + }, + ) + .expect_err("the first-ever invocation's DB provider fails"); + assert!( + matches!(first, CoordinateError::AgentTraceDbUnavailable(_)), + "expected AgentTraceDbUnavailable, got {first:?}" + ); + assert!( + marker.exists().expect("marker existence should resolve"), + "the first-ever failed invocation still leaves an armed marker" + ); + + // More edits during the still-unobserved interval. + std::fs::write( + repo_root.join("during-the-gap.txt"), + b"more unknown-interval content", + ) + .expect("another unobserved edit should write"); + + // The first *successful* invocation establishes a baseline with no evidence + // for the unknown interval. + let established = coordinate(&repo_root, &RuntimeBoundary::Flush, || { + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + }) + .expect("the first successful invocation establishes the baseline"); + assert!( + established.mutation_event.is_none(), + "a worktree with no prior durable row cannot produce evidence for the unknown interval" + ); + assert!( + !marker.exists().expect("marker existence should resolve"), + "the successful baseline must clear the inherited marker" + ); + + let db = RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + .expect("reopening the DB for assertions should succeed"); + let store = MutationTraceStore::new(&db); + let projection = store + .load_worktree(&established.worktree_id, None, None) + .expect("loading the worktree row should succeed") + .expect("the worktree row should now exist"); + assert_eq!( + projection.worktree_state.cursor_tree, established.observed_tree, + "the baseline is established against the first observed tree" + ); + assert!(!projection.worktree_state.tainted); + assert_eq!(projection.worktree_state.failure_kind, FailureKind::Healthy); + for revision in 0..=established.revision { + assert!( + store + .load_mutation_event(&established.worktree_id, revision) + .expect("loading a mutation event should succeed") + .is_none(), + "no MutationEvent may exist for a worktree whose history began with an unobserved interval" + ); + } + + cleanup( + db_path + .parent() + .expect("the DB path has a parent directory"), + ); + cleanup(&repo_root); +} + +#[test] +#[allow(clippy::too_many_lines)] +fn linked_worktrees_keep_independent_external_taint_markers_over_a_shared_db() { + let main_root = unique_path("public-taint-linked-main"); + init_repo(&main_root); + let linked_root = unique_path("public-taint-linked-secondary"); + run_git( + &main_root, + &[ + "worktree", + "add", + "--quiet", + linked_root.to_str().expect("worktree path should be UTF-8"), + ], + ); + + // One shared repository DB, outside either worktree so it never perturbs a + // captured tree. + let db_path = unique_path("public-taint-linked-db").join("agent-trace.db"); + RepositoryAgentTraceDb::new_at(&db_path).expect("the shared repository DB should open"); + + let main_git_dir = resolve_git_dir(&main_root).expect("main git dir should resolve"); + let linked_git_dir = resolve_git_dir(&linked_root).expect("linked git dir should resolve"); + let main_marker = ExternalTaintMarker::new(&main_git_dir); + let linked_marker = ExternalTaintMarker::new(&linked_git_dir); + + // Baseline both worktrees against the one shared DB. + let main_baseline = coordinate(&main_root, &RuntimeBoundary::Flush, || { + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + }) + .expect("the main worktree baseline should succeed"); + let linked_baseline = coordinate(&linked_root, &RuntimeBoundary::Flush, || { + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + }) + .expect("the linked worktree baseline should succeed"); + assert_ne!(main_baseline.worktree_id, linked_baseline.worktree_id); + + // Strand a live scope in the linked worktree behind an armed marker. + let linked_scope = ScopeId("scope-linked".to_string()); + coordinate( + &linked_root, + &RuntimeBoundary::Start { + scope: linked_scope.clone(), + event: EventId("evt-linked-start".to_string()), + actor_kind: ActorKind::ClaudeCode, + }, + || RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path), + ) + .expect("starting the linked worktree's scope should succeed"); + linked_marker + .persist() + .expect("arming the linked worktree's marker should succeed"); + + // A boundary in the MAIN worktree must not observe the linked worktree's marker. + std::fs::write(main_root.join("main-work.txt"), b"v1") + .expect("a main-worktree edit should write"); + coordinate(&main_root, &RuntimeBoundary::Flush, || { + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + }) + .expect("the main worktree flush must succeed without inheriting the linked worktree's marker"); + assert!( + !main_marker + .exists() + .expect("marker existence should resolve"), + "the main worktree clears its own marker on success" + ); + assert!( + linked_marker + .exists() + .expect("marker existence should resolve"), + "the main worktree's invocation must not touch the linked worktree's independent marker" + ); + + let db = RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + .expect("reopening the shared DB for assertions should succeed"); + let store = MutationTraceStore::new(&db); + let linked_mid = store + .load_worktree(&linked_baseline.worktree_id, Some(&linked_scope), None) + .expect("loading the linked worktree row should succeed") + .expect("the linked worktree row should exist"); + assert_eq!( + linked_mid.scopes.get(&linked_scope).map(|s| s.status), + Some(ScopeStatus::Active), + "a marker in the linked worktree must not trigger recovery of the linked worktree from the main worktree's invocation" + ); + + // The linked worktree's own next invocation inherits and recovers. + std::fs::write(linked_root.join("linked-work.txt"), b"v1") + .expect("a linked-worktree edit should write"); + let linked_recovered = coordinate(&linked_root, &RuntimeBoundary::Flush, || { + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + }) + .expect("the linked worktree's own invocation recovers from its inherited marker"); + assert!( + linked_recovered.mutation_event.is_none(), + "the linked worktree's conservative recovery emits no evidence for its fenced interval" + ); + assert!( + !linked_marker + .exists() + .expect("marker existence should resolve"), + "the linked worktree clears its marker after its own successful recovery" + ); + + let linked_after = store + .load_worktree(&linked_baseline.worktree_id, Some(&linked_scope), None) + .expect("loading the linked worktree row should succeed") + .expect("the linked worktree row should exist"); + assert_eq!( + linked_after.scopes.get(&linked_scope).map(|s| s.status), + Some(ScopeStatus::Abandoned), + "the linked worktree's inherited-taint recovery abandons its own live scope" + ); + + cleanup( + db_path + .parent() + .expect("the DB path has a parent directory"), + ); + cleanup(&linked_root); + cleanup(&main_root); +} + +#[test] +fn a_snapshot_failure_arms_the_marker_and_the_next_invocation_recovers_once() { + let repo_root = unique_path("public-snapshot-failure-marker-recovery"); + init_repo(&repo_root); + // The DB lives outside the worktree so it never perturbs the observed tree. + let db_path = unique_path("public-snapshot-failure-marker-recovery-db").join("agent-trace.db"); + RepositoryAgentTraceDb::new_at(&db_path).expect("the repository DB should open with schema"); + let git_dir = resolve_git_dir(&repo_root).expect("git dir should resolve"); + let marker = ExternalTaintMarker::new(&git_dir); + let ok_db = || RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path); + + let baseline = coordinate(&repo_root, &RuntimeBoundary::Flush, ok_db) + .expect("the baseline observation should materialize the worktree"); + let worktree_id = baseline.worktree_id.clone(); + assert!(!marker.exists().expect("marker existence should resolve")); + + // Plant a file where the snapshot service needs its temp-index directory. + let tmp_index_dir = git_dir.join("sce").join("tmp"); + let _ = std::fs::remove_dir_all(&tmp_index_dir); + std::fs::write(&tmp_index_dir, b"not a directory") + .expect("planting a file where the snapshot service expects its temp-index directory"); + + let scope = ScopeId("scope-during-the-failure".to_string()); + let failure = coordinate( + &repo_root, + &RuntimeBoundary::Start { + scope: scope.clone(), + event: EventId("evt-during-failure".to_string()), + actor_kind: ActorKind::ClaudeCode, + }, + ok_db, + ) + .expect_err("a Git snapshot failure after marker arming should be reported"); + assert!( + matches!( + failure, + CoordinateError::SnapshotFailure { + persisted_taint: true, + .. + } + ), + "expected a SnapshotFailure that durably tainted the worktree, got {failure:?}" + ); + assert!( + marker.exists().expect("marker existence should resolve"), + "a snapshot failure after arming must leave BOTH a durable taint and the external-taint marker" + ); + + { + let db = RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + .expect("reopening the DB for assertions should succeed"); + let store = MutationTraceStore::new(&db); + let projection = store + .load_worktree(&worktree_id, None, None) + .expect("loading the worktree row should succeed") + .expect("the worktree row should still exist"); + assert!(projection.worktree_state.tainted); + assert_eq!( + projection.worktree_state.failure_kind, + FailureKind::SnapshotFailure + ); + } + + std::fs::remove_file(&tmp_index_dir) + .expect("removing the planted file should let the snapshot service recreate its temp dir"); + std::fs::write(repo_root.join("work.txt"), b"v1") + .expect("an edit before recovery should write"); + + let recovered = coordinate(&repo_root, &RuntimeBoundary::Flush, ok_db) + .expect("the coordinator should recover from the taint and process the boundary"); + assert!( + recovered.mutation_event.is_none(), + "the conservative recovery emits no evidence for the fenced interval" + ); + assert!( + !marker.exists().expect("marker existence should resolve"), + "a successful recovery clears the marker" + ); + + let db = RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + .expect("reopening the DB for assertions should succeed"); + let store = MutationTraceStore::new(&db); + let projection = store + .load_worktree(&worktree_id, None, None) + .expect("loading the worktree row should succeed") + .expect("the worktree row should still exist"); + assert!( + !projection.worktree_state.tainted, + "recovery must clear the durable taint" + ); + assert_eq!(projection.worktree_state.failure_kind, FailureKind::Healthy); + assert_eq!( + projection.worktree_state.cursor_tree, recovered.observed_tree, + "recovery rebaselines the cursor to the recovering invocation's own observed tree" + ); + + // A plain follow-up flush proves the recovery already fully completed once: + // there is nothing left to recover. + let stable = coordinate(&repo_root, &RuntimeBoundary::Flush, ok_db) + .expect("a plain flush after recovery should succeed"); + assert_eq!( + stable.revision, recovered.revision, + "the single conservative recovery already completed; a follow-up flush is a no-op" + ); + assert!(stable.mutation_event.is_none()); + + cleanup( + db_path + .parent() + .expect("the DB path has a parent directory"), + ); + cleanup(&repo_root); +} + +#[test] +fn a_marker_clear_failure_after_a_durable_boundary_keeps_the_marker_for_a_later_recovery() { + let repo_root = unique_path("public-marker-clear-failure"); + init_repo(&repo_root); + // The DB lives outside the worktree so it never perturbs the observed tree. + let db_path = unique_path("public-marker-clear-failure-db").join("agent-trace.db"); + RepositoryAgentTraceDb::new_at(&db_path).expect("the repository DB should open with schema"); + let git_dir = resolve_git_dir(&repo_root).expect("git dir should resolve"); + let marker = ExternalTaintMarker::new(&git_dir); + let marker_path = git_dir.join("sce").join("mutation-cursor-tainted"); + + let baseline = coordinate(&repo_root, &RuntimeBoundary::Flush, || { + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + }) + .expect("the baseline observation should materialize the worktree"); + let worktree_id = baseline.worktree_id.clone(); + + // Ensure the next boundary is a real, durable transition. + std::fs::write(repo_root.join("work.txt"), b"v1") + .expect("an edit before the boundary should write"); + + // The caller-supplied DB provider closure runs after the marker is armed and + // before the trailing clear(): swap the marker file for a directory so the + // clear()'s remove_file fails, with the boundary already durably committed. + let clear_marker_path = marker_path.clone(); + let clear_db_path = db_path.clone(); + let error = coordinate(&repo_root, &RuntimeBoundary::Flush, move || { + std::fs::remove_file(&clear_marker_path) + .expect("the armed marker file should be present mid-invocation"); + std::fs::create_dir_all(clear_marker_path.join("nested")) + .expect("planting a non-empty directory at the marker path should succeed"); + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&clear_db_path) + }) + .expect_err("clearing a marker that is now a non-empty directory must fail"); + assert!( + matches!( + error, + CoordinateError::ExternalTaintMarker { + operation: ExternalTaintOperation::Clear, + .. + } + ), + "expected a Clear-operation marker failure, got {error:?}" + ); + assert!( + marker.exists().expect("marker existence should resolve"), + "the marker is left in place after a clear failure" + ); + + // The triggering boundary itself committed durably despite the clear failure. + let durable_cursor = { + let db = RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + .expect("reopening the DB for assertions should succeed"); + let store = MutationTraceStore::new(&db); + let projection = store + .load_worktree(&worktree_id, None, None) + .expect("loading the worktree row should succeed") + .expect("the worktree row should exist"); + assert!( + projection.worktree_state.revision >= 1, + "the triggering boundary must have committed durably before the clear failed" + ); + projection.worktree_state.cursor_tree.clone() + }; + + // Normalize the marker scaffolding back to a plain file (it stays logically + // armed) and confirm a later invocation still recovers conservatively. + std::fs::remove_dir_all(&marker_path).expect("removing the planted directory should succeed"); + marker + .persist() + .expect("re-arming a plain marker file should succeed"); + std::fs::write(repo_root.join("work.txt"), b"v2").expect("a later edit should write"); + + let recovered = coordinate(&repo_root, &RuntimeBoundary::Flush, || { + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) + }) + .expect("the later invocation recovers from the still-armed marker"); + assert!( + recovered.mutation_event.is_none(), + "the deferred conservative recovery emits no evidence for the interval it could not prove" + ); + assert!( + !marker.exists().expect("marker existence should resolve"), + "the successful later recovery finally clears the marker" + ); + assert_ne!( + recovered.observed_tree, durable_cursor, + "the later recovery rebaselines to the newer tree" + ); + + cleanup( + db_path + .parent() + .expect("the DB path has a parent directory"), + ); + cleanup(&repo_root); +} diff --git a/context/plans/mutation-cursor-external-taint.md b/context/plans/mutation-cursor-external-taint.md index b2517fbd..68165f75 100644 --- a/context/plans/mutation-cursor-external-taint.md +++ b/context/plans/mutation-cursor-external-taint.md @@ -515,8 +515,62 @@ local choices, not new requirements. `./scripts/run-cli-cargo.sh fmt --manifest-path cli/Cargo.toml -- --check`. - Context synchronization: synced -- [ ] T04: `Add restart/failure integration tests through coordinate()` (status:todo) +- [x] T04: `Add restart/failure integration tests through coordinate()` (status:done) - Task ID: T04 + - Completed: 2026-08-30 + - Files changed: + - `cli/src/services/mutation_trace/runtime/tests.rs` — three added imports + (`FailureKind`, `ScopeStatus`; `ExternalTaintOperation`; + `super::external_taint::ExternalTaintMarker`) and seven new cross-module + `#[test]`s driving only the public `coordinate()` API against real + `git init` / `git worktree add` repos and real temp-file + `RepositoryAgentTraceDb`s (DB placed in a sibling temp dir, outside the + worktree, so it never perturbs the captured tree): + `a_successful_coordinate_through_the_public_api_leaves_no_external_taint_marker`, + `a_db_open_failure_after_arming_leaves_the_marker_and_the_next_invocation_rebaselines_without_evidence` + (AC14 + the end-to-end A→B→gap→C no-trusted-evidence story, with a + `load_mutation_event` revision sweep asserting no event ends at the + post-gap tree), + `a_stale_marker_rebaselines_to_the_current_tree_abandons_scopes_then_processes_the_boundary` + (AC5), `a_first_ever_failed_invocation_that_never_materialized_a_worktree_row_creates_no_evidence` + (AC9), `linked_worktrees_keep_independent_external_taint_markers_over_a_shared_db` + (AC12), `a_snapshot_failure_arms_the_marker_and_the_next_invocation_recovers_once`, + `a_marker_clear_failure_after_a_durable_boundary_keeps_the_marker_for_a_later_recovery`. + Two of the seven carry `#[allow(clippy::too_many_lines)]` (end-to-end + multi-step scenarios; crate has precedent for the allow). + - Result: The external-taint fence is now proven end to end through the public + `coordinate()` entrypoint only, no production-code change. Success clears the + marker; a DB-provider `Err` after arming (AC14) and a Git snapshot failure + both leave the marker present (the snapshot path also leaves a durable + `SnapshotFailure` taint), and the next working invocation performs a single + conservative recovery, rebaselines the cursor to the current tree, abandons + scopes that were live across the fenced interval, emits no `MutationEvent` + across the gap, then processes its triggering boundary. A first-ever failed + invocation that never materialized a worktree row cannot create evidence for + the unknown interval. A stale marker in linked worktree A does not recover + linked worktree B over one shared DB; each worktree clears only its own + marker. A marker-clear failure after a durable boundary (injected by swapping + the marker file for a directory inside the caller-supplied `open_db` closure) + returns `CoordinateError::ExternalTaintMarker { operation: Clear }` with the + boundary already durable and the marker left for a later conservative + re-recovery. Tests use a sibling-dir DB so consecutive no-edit flushes are + genuinely stable. + - Verify: + - `test ...services::mutation_trace::runtime::tests` — PASS (10 passed, 0 failed). + - `test ...services::mutation_trace::runtime::` — PASS (55 passed, 0 failed). + - `test ...` (full CLI suite) — PASS (844 passed, 0 failed). + - `clippy --all-targets -- -D warnings` — PASS (clean). + - `fmt -- --check` — PASS (clean). + - Context impact: none. Test-only change; no production code, public + interface, protocol semantics, DB state, on-disk layout, or documented + behavior changed. The added tests only exercise the `coordinate()` / + `ExternalTaintMarker` behavior already documented by T01–T03's context + synchronization. The mandatory five-root-file verification pass still applies. + - Deviations: the DB is placed in a sibling temp directory rather than inside + the repo worktree (as some pre-existing tests in this file do) so the SQLite + file never appears in a captured tree and revision-stability / no-evidence + assertions hold; two long end-to-end tests carry + `#[allow(clippy::too_many_lines)]`. - Scope: In — `runtime/tests.rs` cross-module tests driving only the public `coordinate()` API against real `git init` / `git worktree add` repositories and real temp-file `RepositoryAgentTraceDb`s (unique-temp-path precedent), @@ -542,7 +596,7 @@ local choices, not new requirements. full CLI test suite pass. - Verify: `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml services::mutation_trace::runtime::tests`; `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml`. - - Context synchronization: pending + - Context synchronization: synced ## Open questions From a66c6418a6791222d972b7fb008b46df38a782d3 Mon Sep 17 00:00:00 2001 From: David Abram Date: Sun, 30 Aug 2026 19:56:04 +0200 Subject: [PATCH 7/8] runtime+sync: Preserve committed taint outcomes and stabilize batch tests Keep the external-taint marker armed after a durable boundary when cleanup fails, while returning the committed CoordinateOutcome so mutation events remain available. Add recovery-transition coverage and document the pre-commit and post-commit failure semantics. Make the malformed batch response test deterministic by seeding only the messages stream and asserting the exact reconciliation request order. Co-authored-by: SCE --- .../mutation_trace/runtime/coordinator.rs | 210 ++++++++++++++++-- .../mutation_trace/runtime/external_taint.rs | 2 - .../services/mutation_trace/runtime/tests.rs | 101 +++++---- cli/src/services/sync/sync.rs | 41 ++-- context/cli/mutation-trace-external-taint.md | 56 ++++- context/cli/mutation-trace-protocol.md | 7 +- .../cli/mutation-trace-runtime-coordinator.md | 23 +- context/context-map.md | 2 +- .../plans/mutation-cursor-external-taint.md | 131 +++++++---- spec/mutation_cursor.md | 2 +- 10 files changed, 431 insertions(+), 144 deletions(-) diff --git a/cli/src/services/mutation_trace/runtime/coordinator.rs b/cli/src/services/mutation_trace/runtime/coordinator.rs index ff48f3f8..5678df57 100644 --- a/cli/src/services/mutation_trace/runtime/coordinator.rs +++ b/cli/src/services/mutation_trace/runtime/coordinator.rs @@ -49,12 +49,16 @@ pub struct CoordinateOutcome { pub mutation_event: Option, } -/// Which [`ExternalTaintMarker`] operation failed while coordinating a boundary. +/// Which pre-commit [`ExternalTaintMarker`] operation failed while coordinating a +/// boundary. Both happen **before** any protected work, so no +/// [`CoordinateOutcome`] exists yet. A marker-clear failure happens *after* a +/// durable commit and is reported through +/// [`CoordinateError::MarkerClearAfterCommit`] instead, which carries the +/// committed outcome. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum ExternalTaintOperation { Inspect, Persist, - Clear, } #[derive(Debug)] @@ -72,14 +76,25 @@ pub enum CoordinateError { revision: u64, }, LockAcquisition(anyhow::Error), - /// Inspecting, persisting, or clearing the worktree-local external-taint - /// marker failed. An inspect/persist failure is returned before any - /// checkout-identity, DB, snapshot, or protocol work; a clear failure is - /// returned after a successful boundary with the marker left in place. + /// Inspecting or persisting the worktree-local external-taint marker failed. + /// Both operations run **before** any checkout-identity, DB, snapshot, or + /// protocol work, so no mutation boundary has committed and there is no + /// [`CoordinateOutcome`] to surface — the boundary is aborted fail-closed + /// with the fence left in whatever state it was in. ExternalTaintMarker { operation: ExternalTaintOperation, source: anyhow::Error, }, + /// The mutation boundary committed successfully to the Agent Trace DB and + /// produced a [`CoordinateOutcome`], but clearing the write-ahead + /// external-taint marker afterwards failed. The boundary did **not** fail: + /// `committed` carries the durable outcome (including any [`MutationEvent`]) + /// so the caller never loses it. The marker remains logically armed, so the + /// next invocation conservatively recovers. + MarkerClearAfterCommit { + source: anyhow::Error, + committed: Box, + }, /// The caller-supplied Agent Trace DB provider returned `Err` after the /// external-taint marker was already armed. The marker is intentionally /// left in place so a later invocation treats the lost interval @@ -111,7 +126,13 @@ impl std::fmt::Display for CoordinateError { ), CoordinateError::ExternalTaintMarker { operation, source } => write!( f, - "External-taint marker {operation:?} operation failed: {source}" + "External-taint marker {operation:?} operation failed before any \ + mutation boundary committed: {source}" + ), + CoordinateError::MarkerClearAfterCommit { source, .. } => write!( + f, + "Mutation boundary committed, but clearing the external-taint \ + marker failed: {source}" ), CoordinateError::AgentTraceDbUnavailable(source) => { write!(f, "Repository Agent Trace DB is unavailable: {source}") @@ -158,18 +179,20 @@ pub fn coordinate

( where P: FnOnce() -> anyhow::Result, { - coordinate_inner(repository_root, boundary, open_db, || {}) + coordinate_inner(repository_root, boundary, open_db, || {}, |_attempt| Ok(())) } -fn coordinate_inner( +fn coordinate_inner( repository_root: &Path, boundary: &RuntimeBoundary, open_db: P, on_lock_contention: F, + after_recovery: R, ) -> Result where P: FnOnce() -> anyhow::Result, F: FnOnce(), + R: FnMut(u32) -> Result<()>, { let git_dir = resolve_git_dir(repository_root).map_err(CoordinateError::Other)?; @@ -197,27 +220,29 @@ where boundary, open_db, inherited_external_taint, + after_recovery, )?; - marker - .clear() - .map_err(|source| CoordinateError::ExternalTaintMarker { - operation: ExternalTaintOperation::Clear, + match marker.clear() { + Ok(()) => Ok(outcome), + Err(source) => Err(CoordinateError::MarkerClearAfterCommit { source, - })?; - - Ok(outcome) + committed: Box::new(outcome), + }), + } } -fn coordinate_protected

( +fn coordinate_protected( repository_root: &Path, git_dir: &Path, boundary: &RuntimeBoundary, open_db: P, inherited_external_taint: bool, + after_recovery: R, ) -> Result where P: FnOnce() -> anyhow::Result, + R: FnMut(u32) -> Result<()>, { let checkout_id = get_or_create_checkout_id(git_dir).map_err(CoordinateError::Other)?; let worktree_id = WorktreeId(checkout_id); @@ -226,12 +251,14 @@ where let snapshot = GitSnapshotService::new(repository_root).map_err(CoordinateError::Other)?; - coordinate_boundary( + coordinate_boundary_inner( &db, &snapshot, &worktree_id, boundary, inherited_external_taint, + |_attempt| {}, + after_recovery, ) } @@ -239,6 +266,7 @@ fn lock_acquisition(error: WorktreeLockError) -> CoordinateError { CoordinateError::LockAcquisition(anyhow::Error::new(error)) } +#[cfg(test)] fn coordinate_boundary( db: &RepositoryAgentTraceDb, capture: &C, @@ -253,7 +281,7 @@ fn coordinate_boundary( boundary, inherited_external_taint, |_attempt| {}, - |_attempt| {}, + |_attempt| Ok(()), ) } @@ -269,7 +297,7 @@ fn coordinate_boundary_inner( where C: SnapshotCapture, AfterLoad: FnMut(u32), - AfterRecovery: FnMut(u32), + AfterRecovery: FnMut(u32) -> Result<()>, { let store = MutationTraceStore::new(db); @@ -335,7 +363,7 @@ where CasResult::Applied => { state = recovered; external_taint_pending = false; - after_recovery(attempt_index); + after_recovery(attempt_index).map_err(CoordinateError::Other)?; } CasResult::Conflict => continue, } @@ -1482,6 +1510,7 @@ mod tests { .send(()) .expect("contention signal channel should still be open"); }, + |_attempt| Ok(()), ); result_tx .send(()) @@ -1690,6 +1719,7 @@ mod tests { .send(()) .expect("contention signal channel should still be open"); }, + |_attempt| Ok(()), ) }) }; @@ -1900,7 +1930,7 @@ mod tests { ); } }, - |_attempt| {}, + |_attempt| Ok(()), ) .expect("recovery must recompute past the losing CAS and still succeed"); @@ -1988,6 +2018,7 @@ mod tests { true, ); } + Ok(()) }, ) .expect("the boundary CAS retry after a landed recovery must still succeed"); @@ -2004,4 +2035,139 @@ mod tests { remove_test_db(&db_path); } + + #[test] + #[allow(clippy::too_many_lines)] + fn a_failure_after_recovery_before_boundary_commit_leaves_marker_and_forces_later_recovery() { + let repo_root = unique_test_repo("t-ac8-recovery-then-fail"); + init_repo(&repo_root); + let db_path = unique_test_db_path("t-ac8-recovery-then-fail"); + RepositoryAgentTraceDb::new_at(&db_path).expect("seed db should open with schema"); + let git_dir = resolve_git_dir(&repo_root).expect("git dir should resolve"); + let marker = ExternalTaintMarker::new(&git_dir); + let ok_db = || RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path); + + std::fs::write(repo_root.join("work.txt"), b"a").expect("the baseline edit should write"); + let baseline = coordinate(&repo_root, &RuntimeBoundary::Flush, ok_db) + .expect("the baseline observation should establish cursor A"); + let worktree_id = baseline.worktree_id.clone(); + let tree_a = baseline.observed_tree.clone(); + + let scope = ScopeId("scope-live".to_string()); + coordinate( + &repo_root, + &RuntimeBoundary::Start { + scope: scope.clone(), + event: EventId("evt-start".to_string()), + actor_kind: ActorKind::ClaudeCode, + }, + ok_db, + ) + .expect("starting the live scope should succeed"); + + marker.persist().expect( + "simulating a prior crashed invocation that armed but never cleared the marker", + ); + std::fs::write(repo_root.join("work.txt"), b"b").expect("the A -> B edit should write"); + + let error = coordinate_inner( + &repo_root, + &RuntimeBoundary::Advance { + scope: scope.clone(), + event: EventId("evt-advance".to_string()), + actor_kind: ActorKind::ClaudeCode, + }, + ok_db, + || {}, + |_attempt| { + anyhow::bail!("injected failure after recovery, before the boundary commits") + }, + ) + .expect_err("the injected post-recovery failure must fail the invocation"); + assert!( + matches!(error, CoordinateError::Other(_)), + "expected CoordinateError::Other from the injected failure, got {error:?}" + ); + assert!( + marker.exists().expect("marker existence should resolve"), + "a failure after recovery but before the boundary commits must leave the marker armed" + ); + + let store_db = ok_db().expect("reopening the DB for assertions should succeed"); + let store = MutationTraceStore::new(&store_db); + let after_fail = store + .load_worktree(&worktree_id, Some(&scope), None) + .expect("loading the worktree row should succeed") + .expect("the worktree row should exist"); + assert!( + !after_fail.worktree_state.tainted, + "the recovery CAS committed durably before the injected failure" + ); + assert_eq!( + after_fail.worktree_state.failure_kind, + FailureKind::Healthy, + "recovery cleared the failure state before the injected failure" + ); + assert!( + after_fail.worktree_state.revision >= 2, + "the durable recovery advanced the revision past the start boundary" + ); + assert_ne!( + after_fail.worktree_state.cursor_tree, tree_a, + "recovery rebaselined the cursor away from A to the invocation's own observed tree" + ); + assert_eq!( + after_fail.scopes.get(&scope).map(|s| s.status), + Some(ScopeStatus::Abandoned), + "the live scope was abandoned by the durable recovery" + ); + assert!( + after_fail.processed_events.is_empty(), + "the triggering Advance must never have been processed" + ); + assert!( + store + .load_mutation_event(&worktree_id, after_fail.worktree_state.revision) + .expect("loading a mutation event should succeed") + .is_none(), + "no MutationEvent may be emitted for the boundary that never committed" + ); + drop(store_db); + + std::fs::write(repo_root.join("work.txt"), b"c").expect("the B -> C edit should write"); + let recovered = coordinate(&repo_root, &RuntimeBoundary::Flush, ok_db) + .expect("the later invocation inherits the still-armed marker and recovers again"); + assert!( + recovered.mutation_event.is_none(), + "no evidence may cross the interval the still-armed marker fenced off" + ); + assert_ne!( + recovered.observed_tree, tree_a, + "the later recovery rebaselines to the newer tree C" + ); + assert!( + !marker.exists().expect("marker existence should resolve"), + "the later successful recovery finally clears the marker" + ); + + let store_db = ok_db().expect("reopening the DB for assertions should succeed"); + let store = MutationTraceStore::new(&store_db); + let after_recover = store + .load_worktree(&worktree_id, Some(&scope), None) + .expect("loading the worktree row should succeed") + .expect("the worktree row should exist"); + assert_eq!( + after_recover.worktree_state.cursor_tree, recovered.observed_tree, + "the later recovery rebaselines the cursor to its own observed tree" + ); + assert_eq!( + after_recover.scopes.get(&scope).map(|s| s.status), + Some(ScopeStatus::Abandoned), + "the later invocation must not resurrect the abandoned scope" + ); + drop(store_db); + + remove_test_db(&db_path); + remove_test_repo(&repo_root); + } } diff --git a/cli/src/services/mutation_trace/runtime/external_taint.rs b/cli/src/services/mutation_trace/runtime/external_taint.rs index 9e9ce508..38bdc7da 100644 --- a/cli/src/services/mutation_trace/runtime/external_taint.rs +++ b/cli/src/services/mutation_trace/runtime/external_taint.rs @@ -17,8 +17,6 @@ //! is never deleted via `Drop`: only an explicit `clear()` after a successful //! `CoordinateOutcome` removes it. -#![allow(dead_code)] - use std::fs::OpenOptions; use std::path::{Path, PathBuf}; diff --git a/cli/src/services/mutation_trace/runtime/tests.rs b/cli/src/services/mutation_trace/runtime/tests.rs index 9d0ec9b9..a0a2f0d4 100644 --- a/cli/src/services/mutation_trace/runtime/tests.rs +++ b/cli/src/services/mutation_trace/runtime/tests.rs @@ -15,7 +15,7 @@ use crate::services::mutation_trace::types::{ ActorKind, EventId, FailureKind, ScopeId, ScopeStatus, }; -use super::coordinator::{coordinate, CoordinateError, ExternalTaintOperation, RuntimeBoundary}; +use super::coordinator::{coordinate, CoordinateError, RuntimeBoundary}; use super::external_taint::ExternalTaintMarker; use super::git_snapshot::GitSnapshotService; use super::worktree_lock::WorktreeLock; @@ -852,6 +852,7 @@ fn a_snapshot_failure_arms_the_marker_and_the_next_invocation_recovers_once() { } #[test] +#[allow(clippy::too_many_lines)] fn a_marker_clear_failure_after_a_durable_boundary_keeps_the_marker_for_a_later_recovery() { let repo_root = unique_path("public-marker-clear-failure"); init_repo(&repo_root); @@ -861,73 +862,89 @@ fn a_marker_clear_failure_after_a_durable_boundary_keeps_the_marker_for_a_later_ let git_dir = resolve_git_dir(&repo_root).expect("git dir should resolve"); let marker = ExternalTaintMarker::new(&git_dir); let marker_path = git_dir.join("sce").join("mutation-cursor-tainted"); + let ok_db = || RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path); - let baseline = coordinate(&repo_root, &RuntimeBoundary::Flush, || { - RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) - }) - .expect("the baseline observation should materialize the worktree"); + let baseline = coordinate(&repo_root, &RuntimeBoundary::Flush, ok_db) + .expect("the baseline observation should materialize the worktree"); let worktree_id = baseline.worktree_id.clone(); - // Ensure the next boundary is a real, durable transition. + let scope = ScopeId("scope-attributable".to_string()); + coordinate( + &repo_root, + &RuntimeBoundary::Start { + scope: scope.clone(), + event: EventId("evt-start".to_string()), + actor_kind: ActorKind::ClaudeCode, + }, + ok_db, + ) + .expect("starting the scope should succeed"); std::fs::write(repo_root.join("work.txt"), b"v1") - .expect("an edit before the boundary should write"); + .expect("an exclusive edit before the boundary should write"); - // The caller-supplied DB provider closure runs after the marker is armed and - // before the trailing clear(): swap the marker file for a directory so the - // clear()'s remove_file fails, with the boundary already durably committed. let clear_marker_path = marker_path.clone(); let clear_db_path = db_path.clone(); - let error = coordinate(&repo_root, &RuntimeBoundary::Flush, move || { - std::fs::remove_file(&clear_marker_path) - .expect("the armed marker file should be present mid-invocation"); - std::fs::create_dir_all(clear_marker_path.join("nested")) - .expect("planting a non-empty directory at the marker path should succeed"); - RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&clear_db_path) - }) + let error = coordinate( + &repo_root, + &RuntimeBoundary::Advance { + scope: scope.clone(), + event: EventId("evt-advance".to_string()), + actor_kind: ActorKind::ClaudeCode, + }, + move || { + std::fs::remove_file(&clear_marker_path) + .expect("the armed marker file should be present mid-invocation"); + std::fs::create_dir_all(clear_marker_path.join("nested")) + .expect("planting a non-empty directory at the marker path should succeed"); + RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&clear_db_path) + }, + ) .expect_err("clearing a marker that is now a non-empty directory must fail"); - assert!( - matches!( - error, - CoordinateError::ExternalTaintMarker { - operation: ExternalTaintOperation::Clear, - .. - } - ), - "expected a Clear-operation marker failure, got {error:?}" - ); + + let committed = match error { + CoordinateError::MarkerClearAfterCommit { committed, .. } => committed, + other => panic!("expected MarkerClearAfterCommit, got {other:?}"), + }; assert!( marker.exists().expect("marker existence should resolve"), - "the marker is left in place after a clear failure" + "the marker stays logically armed after a post-commit clear failure" ); - // The triggering boundary itself committed durably despite the clear failure. - let durable_cursor = { + let (durable_revision, durable_cursor) = { let db = RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) .expect("reopening the DB for assertions should succeed"); let store = MutationTraceStore::new(&db); let projection = store - .load_worktree(&worktree_id, None, None) + .load_worktree(&worktree_id, Some(&scope), None) .expect("loading the worktree row should succeed") .expect("the worktree row should exist"); - assert!( - projection.worktree_state.revision >= 1, - "the triggering boundary must have committed durably before the clear failed" - ); - projection.worktree_state.cursor_tree.clone() + let event = store + .load_mutation_event(&worktree_id, projection.worktree_state.revision) + .expect("loading the committed mutation event should succeed") + .expect("the attributable Advance must have committed one durable event"); + assert_eq!(event.after_tree, projection.worktree_state.cursor_tree); + ( + projection.worktree_state.revision, + projection.worktree_state.cursor_tree.clone(), + ) }; - // Normalize the marker scaffolding back to a plain file (it stays logically - // armed) and confirm a later invocation still recovers conservatively. + assert_eq!(committed.worktree_id, worktree_id); + assert_eq!(committed.revision, durable_revision); + assert_eq!(committed.observed_tree, durable_cursor); + assert!( + committed.mutation_event.is_some(), + "the committed outcome carried by the error must still expose the MutationEvent" + ); + std::fs::remove_dir_all(&marker_path).expect("removing the planted directory should succeed"); marker .persist() .expect("re-arming a plain marker file should succeed"); std::fs::write(repo_root.join("work.txt"), b"v2").expect("a later edit should write"); - let recovered = coordinate(&repo_root, &RuntimeBoundary::Flush, || { - RepositoryAgentTraceDb::open_for_hooks_without_migrations_at(&db_path) - }) - .expect("the later invocation recovers from the still-armed marker"); + let recovered = coordinate(&repo_root, &RuntimeBoundary::Flush, ok_db) + .expect("the later invocation recovers from the still-armed marker"); assert!( recovered.mutation_event.is_none(), "the deferred conservative recovery emits no evidence for the interval it could not prove" diff --git a/cli/src/services/sync/sync.rs b/cli/src/services/sync/sync.rs index 25a2b0ca..fadc6ee2 100644 --- a/cli/src/services/sync/sync.rs +++ b/cli/src/services/sync/sync.rs @@ -1238,18 +1238,15 @@ mod tests { let metadata = db .verify_or_initialize_repository_metadata("repo-malformed-batch") .expect("metadata should initialize"); - seed_one_row_per_stream(&db); + seed_messages(&db, 1); let server = TestHttpServer::start(); server.queue_response(CannedResponse::json(200, &state_response(0, 0, 0, 0))); // Syntactically successful but undecodable as `AgentTraceIngestionBatchResponse`. server.queue_response(CannedResponse::json(200, &json!({"unexpected": "shape"}))); - // The four initial stream requests overlap. Messages receives the - // malformed response, while the other streams receive their normal - // responses. Reconciliation then refetches state and resends messages. - server.queue_response(CannedResponse::json(200, &batch_response(1))); - server.queue_response(CannedResponse::json(200, &batch_response(1))); - server.queue_response(CannedResponse::json(200, &batch_response(1))); + // Only messages has pending data in this test. The malformed successful + // batch response must cause an explicit state reconciliation before the + // same batch is retried. server.queue_response(CannedResponse::json(200, &state_response(0, 0, 0, 0))); server.queue_response(CannedResponse::json(200, &batch_response(1))); let client = test_client(&server); @@ -1262,20 +1259,26 @@ mod tests { ) .expect("an undecodable 2xx /batch body should still reconcile via /state and succeed"); + assert_eq!(server.call_count(), 4); + let requests = server.captured_requests(); assert_eq!( - server.call_count(), - 7, - "an undecodable 2xx /batch body must reconcile via /state before resending, not fail immediately" + requests + .iter() + .map(|request| (request.method.as_str(), request.path.as_str())) + .collect::>(), + vec![ + ("POST", "/agent-trace/ingestion/state"), + ("POST", "/agent-trace/ingestion/batch"), + ("POST", "/agent-trace/ingestion/state"), + ("POST", "/agent-trace/ingestion/batch"), + ], + "an undecodable 2xx /batch body must reconcile via /state before retrying the same batch" ); - for stream in [ - report.streams.messages, - report.streams.parts, - report.streams.diff_traces, - report.streams.agent_traces, - ] { - assert_eq!(stream.uploaded, 1); - assert_eq!(stream.final_cursor, 1); - } + assert_eq!(report.streams.messages.uploaded, 1); + assert_eq!(report.streams.messages.final_cursor, 1); + assert_eq!(report.streams.parts.uploaded, 0); + assert_eq!(report.streams.diff_traces.uploaded, 0); + assert_eq!(report.streams.agent_traces.uploaded, 0); remove_test_db(&db_path); } diff --git a/context/cli/mutation-trace-external-taint.md b/context/cli/mutation-trace-external-taint.md index 455625c9..caef5b1e 100644 --- a/context/cli/mutation-trace-external-taint.md +++ b/context/cli/mutation-trace-external-taint.md @@ -39,9 +39,8 @@ restart — not host power loss or a filesystem-level crash (the parent-director sync is best-effort). The marker is never removed via `Drop`; only an explicit `clear()` removes it. It is never authoritative for normal cursor state. -The module carries `#![allow(dead_code)]` (the `services/capabilities.rs` -precedent) for the parts not yet reached; `new`/`exists`/`persist`/`clear` are -armed by `coordinate()` (below). +Every method — `new`/`exists`/`persist`/`clear` — is now reached by the +`coordinate()` fence (below), so the module carries no `allow(dead_code)`. Inline `#[cfg(test)] mod tests` follows the unique-`std::env::temp_dir()`-path precedent (see [`../patterns.md`](../patterns.md)): marker path is worktree @@ -90,15 +89,38 @@ it. ### `CoordinateError` variants - `ExternalTaintMarker { operation: ExternalTaintOperation, source }` where - `ExternalTaintOperation` is `Inspect | Persist | Clear`. An `Inspect` or - `Persist` failure is returned **before** any checkout-identity, DB, snapshot, - or protocol work (fail closed — an unarmed fence must abort the boundary). A - `Clear` failure is returned **after** a durable boundary, with the marker - deliberately left in place for a later conservative re-recovery. + `ExternalTaintOperation` is `Inspect | Persist`. Both operations run + **before** any checkout-identity, DB, snapshot, or protocol work, so **no + mutation boundary has committed** and there is no `CoordinateOutcome` to + surface. The boundary is aborted fail closed — an unarmed fence must not let + the boundary proceed. +- `MarkerClearAfterCommit { source, committed: Box }` — the + mutation boundary **committed successfully** to the Agent Trace DB (the + `CoordinateOutcome`, including any `MutationEvent`, is durable), but the + trailing `marker.clear()` failed. This is deliberately **not** the same shape + as an `Inspect`/`Persist` failure: the committed outcome is carried in + `committed` so the caller never loses access to it (a future harness must read + it out of the error and still route the `MutationEvent` onward), the `Display` + text says the boundary committed and only cleanup failed, and the marker stays + logically armed so the next invocation conservatively recovers. - `AgentTraceDbUnavailable(source)` — the caller-supplied `open_db` provider returned `Err` after the marker was armed. The marker is intentionally left in place; the lower-level `coordinate_boundary` pipeline is never entered. +Summary of the pre-commit vs. post-commit distinction: + +```text +Inspect / Persist failure → no mutation boundary committed; + no CoordinateOutcome exists; + boundary aborted fail closed. + +Clear failure → mutation boundary already committed; +(MarkerClearAfterCommit) CoordinateOutcome (with any MutationEvent) + remains available in `committed`; + marker stays armed; + next invocation conservatively recovers. +``` + ### Inherited taint recovery `coordinate()` reads `inherited_external_taint` from `marker.exists()` before @@ -121,6 +143,12 @@ flag clears, so a later boundary-CAS retry in the same invocation does not re-trigger recovery. The filesystem marker is never touched here — `coordinate()`'s success path still owns clearing it. +A private `coordinate_inner(.., after_recovery)` / `coordinate_boundary_inner(.., +after_load, after_recovery)` seam lets tests inject a failure at the exact +transition *after* the recovery CAS returns `Applied` and *before* the triggering +boundary is prepared (production passes `|_| Ok(())`). It is never exposed +publicly. + Inline `coordinator.rs` tests drive the public `coordinate()` against real repositories: a successful call clears the marker; a snapshot failure, a non-snapshot failure (revision-exhausted recovery), and a DB provider returning @@ -132,6 +160,18 @@ the single snapshot before the boundary, a first-ever inherited marker with no worktree row baselining without evidence, a losing recovery CAS re-injecting the overlay until it applies, and the flag clearing so a post-recovery boundary-CAS retry does not recover again. +`a_failure_after_recovery_before_boundary_commit_leaves_marker_and_forces_later_recovery` +uses the `after_recovery` seam to prove the recovery-committed / boundary-failed +window: the recovery is durable (cursor rebaselined, scope abandoned, revision +advanced), the triggering boundary is unprocessed with no `MutationEvent`, the +on-disk marker survives, and a later `coordinate()` inherits it and recovers +conservatively again without resurrecting the abandoned scope. +`runtime/tests.rs`'s +`a_marker_clear_failure_after_a_durable_boundary_keeps_the_marker_for_a_later_recovery` +proves an attributable `Advance` commits durably, the returned +`MarkerClearAfterCommit` carries the matching `committed` outcome +(`worktree_id` / `revision` / `observed_tree`, and `mutation_event.is_some()`), +and a later invocation still recovers off the still-armed marker. ## On-disk layout addition diff --git a/context/cli/mutation-trace-protocol.md b/context/cli/mutation-trace-protocol.md index 5d100d64..8bd83068 100644 --- a/context/cli/mutation-trace-protocol.md +++ b/context/cli/mutation-trace-protocol.md @@ -209,7 +209,12 @@ the worktree-local `/sce/mutation-cursor-tainted` marker (see write-ahead before Agent Trace DB acquisition and overlaid onto `database_failure` recovery only when a later invocation inherits it; `WorktreeProjection::into_protocol_state()` itself always returns an empty -`external_taint`. +`external_taint`. A pre-protected marker inspect/persist failure means no +mutation boundary committed; a marker-*clear* failure means the boundary already +committed durably — the coordinator surfaces that as +`CoordinateError::MarkerClearAfterCommit`, carrying the committed +`CoordinateOutcome` so no evidence is lost, and leaves the marker armed so the +next invocation still promotes it to protocol `external_taint`. Future responsibility split (mirrors "Runtime scope materialization" above): the coordinator/store layer resolves/materializes worktree identity/state and diff --git a/context/cli/mutation-trace-runtime-coordinator.md b/context/cli/mutation-trace-runtime-coordinator.md index 66af11f0..082ed7c7 100644 --- a/context/cli/mutation-trace-runtime-coordinator.md +++ b/context/cli/mutation-trace-runtime-coordinator.md @@ -102,10 +102,15 @@ API end to end. Only harness/command wiring remains. only on a successful outcome. Identity flows `repository_root → git_dir → WorktreeLock → checkout ID → WorktreeId`; the DB is not on that chain. (`coordinate()` is a one-line delegation to a - private `coordinate_inner(.., open_db, on_lock_contention: impl FnOnce())` - test seam; production passes a no-op contention closure.) A `WorktreeLock` - acquisition failure surfaces as `CoordinateError::LockAcquisition`; marker-I/O - and DB-provider failures have their own fail-closed variants — see + private `coordinate_inner(.., open_db, on_lock_contention: impl FnOnce(), + after_recovery: impl FnMut(u32) -> Result<()>)` test seam; production passes a + no-op contention closure and `|_| Ok(())`.) A `WorktreeLock` + acquisition failure surfaces as `CoordinateError::LockAcquisition`; pre-commit + marker-I/O and DB-provider failures have their own fail-closed variants, and a + post-commit `marker.clear()` failure surfaces as + `CoordinateError::MarkerClearAfterCommit { source, committed }` — the boundary + did commit, so the durable `CoordinateOutcome` (with any `MutationEvent`) rides + along in `committed` rather than being lost, and the marker stays armed. See [`mutation-trace-external-taint.md`](mutation-trace-external-taint.md) for the fence ordering, the safety invariant, and the `CoordinateError` variants it adds. The pipeline does, per invocation: capture and pin @@ -212,7 +217,15 @@ real repositories: the critical-section serialization (a worker's acquires and returns `Ok` once it drops); and the external-taint fence — a successful call clears the marker, while a snapshot failure, a non-snapshot failure, a DB-provider `Err`, and an un-armable marker each leave it present -(the last failing closed before the DB provider runs). +(the last failing closed before the DB provider runs). A further test drives the +private `after_recovery` seam to inject a failure at the exact +recovery-committed / boundary-not-yet-prepared transition and proves the +recovery is durable, the boundary unprocessed with no `MutationEvent`, the +on-disk marker still present, and a later `coordinate()` re-recovering +conservatively off it; `runtime/tests.rs` separately proves an attributable +`Advance` that commits durably then fails its trailing `marker.clear()` surfaces +`MarkerClearAfterCommit` carrying the matching committed outcome (including its +`MutationEvent`). `runtime/tests.rs` is `runtime`'s own `#[cfg(test)] mod tests`, holding cross-module integration tests that drive only the public `coordinate()` API diff --git a/context/context-map.md b/context/context-map.md index 3dee6821..f00bb36d 100644 --- a/context/context-map.md +++ b/context/context-map.md @@ -28,7 +28,7 @@ Feature/domain context: - `context/cli/mutation-trace-quint-connect.md` (`#[cfg(test)]`-only Quint Connect model-based-testing harness in `cli/src/services/mutation_trace/mbt/` continuously checking `protocol.rs` against `spec/mutation_cursor.qnt`: the verification-only `mbtAction`/`MbtAction` record-payload transport excluded from comparison, the operation-identity-vs-`MbtStutter` distinction on guarded/no-op branches with its two deterministic regressions, finite ID mapping, the AC5 comparable-state field list, `randomPrepare` staying a single `step` branch, deterministic/generated (500×30, seed-reproducible) test coverage, and the two Nix checks — generic `checks.cli-tests` and dedicated `checks.mutation-trace-quint-connect` — that both require the pinned Quint binary plus the top-level `spec/` directory in `workspaceSrc`'s Nix fileset) - `context/cli/mutation-trace-store.md` (durable persistence for the mutation-cursor protocol in `cli/src/services/mutation_trace/store.rs`, built by the `mutation-cursor-store-persistence` plan: the one-directional `protocol.rs` -> `DurableTransition::between` (pure structural diff) -> `store.rs` (SQL translation) -> `RepositoryAgentTraceDb` boundary; migration `003_mutation_trace_protocol.sql`'s five tables; `AttemptState`/`external_taint` non-persistence; the 8-byte big-endian `BLOB` revision encoding plus explicit non-`Debug` enum codecs; the bounded hot-path `load_worktree` read vs. the cold-path `load_mutation_event`; `commit`'s single-`BEGIN IMMEDIATE` CAS batch via `TursoDb::execute_transactional_cas_batch`, distinguishing `Conflict`/retryable-transient/deterministic-`Err` outcomes; and the store's non-goals — no Git/filesystem I/O, no attribution/boundary-kind decisions, no retry-after-`Conflict` loop, no terminal-scope garbage collection) - `context/cli/mutation-trace-runtime-coordinator.md` (imperative-shell runtime layer in `cli/src/services/mutation_trace/runtime/`, built by the `mutation-cursor-runtime-coordinator` plan: the per-worktree OS advisory lock, `runtime::worktree_lock::WorktreeLock::acquire(git_dir, timeout)` at `/sce/mutation-cursor.lock`, bounded `try_lock()` polling with a distinct matchable timeout error and RAII release, and its distinction from the separate checkout-identity-creation lock; and the isolated Git snapshot service, `runtime::git_snapshot::GitSnapshotService` (`capture_tree`/`pin_tree`/`diff_trees`), writing tree/blob objects into the repository's normal object database and pinning durable ones via a create-only, idempotent `refs/sce/mutation-cursor//` ref rather than a private object store; and `runtime::coordinator`'s protocol-integration pipeline (`RuntimeBoundary`/`CoordinateOutcome`/`CoordinateError`, the load → recover-if-needed → prepare/commit CAS-retry loop, and its own bounded snapshot-failure taint-retry loop) plus the public `coordinate(repository_root, boundary, open_db)` entrypoint that acquires the `WorktreeLock`, arms the external-taint write-ahead fence, derives `WorktreeId` from `checkout::get_or_create_checkout_id`, invokes the caller-supplied `open_db` provider so DB acquisition falls inside that fence, drives the pipeline under one held lock, and clears the marker only on success; `runtime/tests.rs` covers the public `coordinate()` API end to end against real linked worktrees and a real Agent Trace DB; an inherited external-taint marker is overlaid onto `protocol::database_failure` recovery on the next invocation, against the single captured snapshot and re-injected across a losing recovery CAS; a `pub(crate)` re-export of `coordinate()` beyond `runtime` and harness/command wiring remain future work) -- `context/cli/mutation-trace-external-taint.md` (the worktree-local mutation-cursor durability boundary in `cli/src/services/mutation_trace/runtime/external_taint.rs`, built by the `mutation-cursor-external-taint` plan: the `ExternalTaintMarker` primitive — `new(git_dir)`/`exists()`/`persist()`/`clear()` over an empty file at `/sce/mutation-cursor-tainted` whose existence is its entire state, `checkout::persist_checkout_id_inner`-style durability (`sync_data` plus best-effort `#[cfg(unix)]` parent-dir `sync_all`), idempotent persist/clear, `NotFound`-on-clear as success, no `Drop` deletion — as the concrete runtime refinement of the abstract `ProtocolState.external_taint`; armed by the reshaped `coordinate()` entrypoint write-ahead after the `WorktreeLock` and before Agent Trace DB acquisition (a caller-supplied DB provider closure), cleared only on a successful `CoordinateOutcome`, with dedicated fail-closed `CoordinateError::ExternalTaintMarker`/`AgentTraceDbUnavailable` variants; an inherited marker seeds an invocation-local `external_taint_pending` flag that overlays `protocol::database_failure` onto each freshly loaded projection so `recover` runs once against the captured snapshot, held across a losing recovery CAS and cleared once it lands) +- `context/cli/mutation-trace-external-taint.md` (the worktree-local mutation-cursor durability boundary in `cli/src/services/mutation_trace/runtime/external_taint.rs`, built by the `mutation-cursor-external-taint` plan: the `ExternalTaintMarker` primitive — `new(git_dir)`/`exists()`/`persist()`/`clear()` over an empty file at `/sce/mutation-cursor-tainted` whose existence is its entire state, `checkout::persist_checkout_id_inner`-style durability (`sync_data` plus best-effort `#[cfg(unix)]` parent-dir `sync_all`), idempotent persist/clear, `NotFound`-on-clear as success, no `Drop` deletion — as the concrete runtime refinement of the abstract `ProtocolState.external_taint`; armed by the reshaped `coordinate()` entrypoint write-ahead after the `WorktreeLock` and before Agent Trace DB acquisition (a caller-supplied DB provider closure), cleared only on a successful `CoordinateOutcome`, with dedicated fail-closed pre-commit `CoordinateError::ExternalTaintMarker` (`Inspect`/`Persist` only)/`AgentTraceDbUnavailable` variants plus a post-commit `MarkerClearAfterCommit { source, committed }` that carries the durable outcome so a failed trailing clear never hides a committed `MutationEvent`; an inherited marker seeds an invocation-local `external_taint_pending` flag that overlays `protocol::database_failure` onto each freshly loaded projection so `recover` runs once against the captured snapshot, held across a losing recovery CAS and cleared once it lands) - `context/sce/cli-exit-code-contract.md` (stable class-based `sce` process exit-code contract in `cli/src/app.rs`, so automation can branch on failure category without parsing error text) - `context/sce/cli-error-code-taxonomy.md` (stable user-facing `SCE-ERR-*` diagnostic code classes rendered by `cli/src/app.rs`, complementing the numeric exit-code classes) - `context/sce/cli-stdout-stderr-contract.md` (implemented stream contract in `cli/src/app.rs`: command payloads on stdout only, redacted diagnostics on stderr) diff --git a/context/plans/mutation-cursor-external-taint.md b/context/plans/mutation-cursor-external-taint.md index 68165f75..b834741b 100644 --- a/context/plans/mutation-cursor-external-taint.md +++ b/context/plans/mutation-cursor-external-taint.md @@ -65,65 +65,76 @@ How this plan is proven complete. Each criterion is observable and names the check that proves it. `/validate` runs these checks; no task in the stack performs final validation. -- [ ] AC1: A marker is stored beneath the caller-supplied worktree-specific Git +- [x] AC1: A marker is stored beneath the caller-supplied worktree-specific Git directory (`/sce/mutation-cursor-tainted`), survives reconstruction of the marker handle, and two distinct `git_dir` inputs derive independent marker paths and marker state. (Actual linked-worktree independence over a - real `git worktree add` pair is AC12/T04.) + real `git worktree add` pair is AC12.) - Validate: `runtime::external_taint::tests::marker_is_worktree_scoped` and `runtime::external_taint::tests::marker_persists_until_explicitly_cleared` -- [ ] AC2: `persist` then `persist` then `clear` then `clear` all succeed; +- [x] AC2: `persist` then `persist` then `clear` then `clear` all succeed; `clear` on an absent marker is success. - Validate: `runtime::external_taint::tests::persist_and_clear_are_idempotent` -- [ ] AC3: A public `coordinate()` call arms its marker internally and clears it +- [x] AC3: A public `coordinate()` call arms its marker internally and clears it only after a successful `CoordinateOutcome`. - - Validate: `runtime::tests::successful_coordinate_clears_external_taint_marker` -- [ ] AC4: After marker arming, any coordinator error leaves the marker present — - proven for the snapshot-failure path and at least one deterministic - non-snapshot failure path. - - Validate: `runtime::tests::snapshot_failure_leaves_external_taint_marker` - plus one deterministic non-snapshot failure test -- [ ] AC5: Given durable cursor A, an active scope S, an inherited marker, and a + - Validate: `runtime::coordinator::tests::public_coordinate_clears_marker_on_success` + and `runtime::tests::a_successful_coordinate_through_the_public_api_leaves_no_external_taint_marker` +- [x] AC4: After marker arming, any coordinator error leaves the marker present — + proven for the snapshot-failure path and deterministic non-snapshot failure + paths. + - Validate: `runtime::coordinator::tests::public_coordinate_leaves_marker_after_a_snapshot_failure`, + `runtime::coordinator::tests::public_coordinate_leaves_marker_after_a_non_snapshot_failure`, + and `runtime::coordinator::tests::public_coordinate_leaves_marker_when_the_db_provider_fails` +- [x] AC5: Given durable cursor A, an active scope S, an inherited marker, and a current worktree C, the next invocation rebaselines to C, emits no A→C mutation evidence, abandons S, then processes its triggering boundary. - - Validate: `runtime::tests::inherited_external_taint_recovers_before_boundary` -- [ ] AC6: External-taint recovery and triggering-boundary processing use the + - Validate: `runtime::tests::a_stale_marker_rebaselines_to_the_current_tree_abandons_scopes_then_processes_the_boundary` + and `runtime::coordinator::tests::inherited_external_taint_recovers_once_before_the_boundary` +- [x] AC6: External-taint recovery and triggering-boundary processing use the same `observed_tree`; no second Git snapshot occurs. - - Validate: coordinator unit test with a call-counting `SnapshotCapture` - asserting exactly one `capture` -- [ ] AC7: A losing recovery CAS re-injects external taint on reload and + - Validate: `runtime::coordinator::tests::inherited_external_taint_recovers_once_before_the_boundary` + (call-counting `FakeSnapshotCapture` asserting exactly one `capture` and one `pin`) +- [x] AC7: A losing recovery CAS re-injects external taint on reload and recomputes recovery until `Applied` or retry exhaustion; the marker remains present throughout the retry loop. - - Validate: coordinator unit test racing a recovery CAS conflict and asserting - re-injection -- [ ] AC8: A failure injected after recovery has committed but before the + - Validate: `runtime::coordinator::tests::a_losing_recovery_cas_reinjects_external_taint_until_it_applies` + and `runtime::coordinator::tests::a_landed_recovery_clears_the_flag_so_a_boundary_cas_retry_does_not_re_recover`; + on-disk-marker survival across the retry loop is + `runtime::tests::a_snapshot_failure_arms_the_marker_and_the_next_invocation_recovers_once` +- [x] AC8: A failure injected after recovery has committed but before the triggering boundary completes leaves recovery durable, the boundary incomplete, and the marker still present; a later invocation recovers - conservatively again. - - Validate: `runtime::tests` marker-clear/boundary-incomplete scenario -- [ ] AC9: A marker that survives an invocation which never materialized a + conservatively again. A post-commit `marker.clear()` failure likewise keeps + the marker armed and returns the committed `CoordinateOutcome` inside + `CoordinateError::MarkerClearAfterCommit` rather than a boundary failure. + - Validate: `runtime::coordinator::tests::a_failure_after_recovery_before_boundary_commit_leaves_marker_and_forces_later_recovery` + and `runtime::tests::a_marker_clear_failure_after_a_durable_boundary_keeps_the_marker_for_a_later_recovery` +- [x] AC9: A marker that survives an invocation which never materialized a worktree row causes the next successful invocation to establish a baseline with no evidence for the unknown interval. - - Validate: coordinator unit test plus `runtime::tests` first-ever-failure - scenario -- [ ] AC10: A worktree with live scopes and inherited external taint persists + - Validate: `runtime::coordinator::tests::inherited_external_taint_with_no_worktree_row_baselines_without_evidence` + and `runtime::tests::a_first_ever_failed_invocation_that_never_materialized_a_worktree_row_creates_no_evidence` +- [x] AC10: A worktree with live scopes and inherited external taint persists those scopes as `Abandoned` during recovery and never treats them as eligible exclusive attribution after the unknown interval. - - Validate: coordinator unit test asserting `Abandoned` scope status after - inherited-taint recovery -- [ ] AC11: End to end, no `MutationEvent` treats an interval spanning an + - Validate: `runtime::coordinator::tests::inherited_external_taint_recovers_once_before_the_boundary` + (asserts `ScopeStatus::Abandoned` after recovery) and + `runtime::coordinator::tests::recovers_from_snapshot_failure_taint_abandoning_live_scopes` +- [x] AC11: End to end, no `MutationEvent` treats an interval spanning an incomplete/failed invocation (baseline A, scope start, edit→B, failed invocation, edit→C, next successful invocation) as one trustworthy AI-attributable A/B→C interval. - - Validate: `runtime::tests` end-to-end no-evidence-across-gap scenario -- [ ] AC12: An external-taint marker in linked worktree A does not trigger + - Validate: `runtime::tests::a_db_open_failure_after_arming_leaves_the_marker_and_the_next_invocation_rebaselines_without_evidence` + (per-revision `load_mutation_event` sweep asserting no event ends at the post-gap tree) +- [x] AC12: An external-taint marker in linked worktree A does not trigger recovery in linked worktree B, even with a shared repository Agent Trace DB. - - Validate: `runtime::tests` linked-worktree independence scenario -- [ ] AC13: When marker inspection or marker persistence cannot be established, + - Validate: `runtime::tests::linked_worktrees_keep_independent_external_taint_markers_over_a_shared_db` +- [x] AC13: When marker inspection or marker persistence cannot be established, `coordinate()` returns a distinct external-taint marker error before any checkout-identity, DB, snapshot, or protocol processing. - - Validate: coordinator unit test injecting marker inspect/persist I/O failure -- [ ] AC14: DB acquisition is inside the external-taint fence. Once + - Validate: `runtime::coordinator::tests::public_coordinate_fails_closed_when_the_marker_cannot_be_armed` + and `runtime::coordinator::tests::public_coordinate_fails_closed_when_marker_inspection_fails` +- [x] AC14: DB acquisition is inside the external-taint fence. Once mutation-cursor boundary processing has acquired the `WorktreeLock` and armed the marker, a failure to resolve or open the repository Agent Trace DB (the caller-supplied provider returns `Err`) makes `coordinate()` return an error @@ -132,10 +143,9 @@ performs final validation. follow-up invocation, given a working DB provider, snapshots the current tree, runs external-taint recovery, and produces no evidence across the lost interval. - - Validate: `runtime::tests` DB-provider-returns-`Err` scenario (marker armed - → provider `Err` → error returned → marker still present) plus its - follow-up-invocation recovery assertion; and a coordinator unit test with a - fake failing DB provider + - Validate: `runtime::tests::a_db_open_failure_after_arming_leaves_the_marker_and_the_next_invocation_rebaselines_without_evidence` + (marker armed → provider `Err` → error returned → marker still present → follow-up recovery) + and `runtime::coordinator::tests::public_coordinate_leaves_marker_when_the_db_provider_fails` ### Full validation @@ -160,9 +170,11 @@ which criterion they map to. caller-supplied DB provider, the snapshot/pipeline, and marker clear; the arm-before-DB-acquisition ordering and the safety invariant; the inherited-vs-armed distinction and the invocation-local `external_taint_pending` - overlay onto `database_failure`; the new marker-I/O and + overlay onto `database_failure`; the new pre-commit marker-I/O and DB-provider-failure `CoordinateError` variants and their fail-closed - semantics; and the added `/sce/` on-disk-layout entry. + semantics, plus the post-commit `MarkerClearAfterCommit` variant that carries + the committed `CoordinateOutcome`; and the added `/sce/` + on-disk-layout entry. - `context/cli/mutation-trace-protocol.md` — note that the concrete runtime refinement of `ProtocolState.external_taint` is the stale worktree-local marker, armed write-ahead before DB acquisition and promoted to protocol @@ -175,6 +187,32 @@ which criterion they map to. - Verify-only pass over `context/overview.md`, `context/architecture.md`, `context/glossary.md`, `context/patterns.md`. +### Deviations from stated scope + +- **Unrelated sync test stabilization (test-only).** During full-suite + validation an unrelated pre-existing flaky sync test was exposed: + `services::sync::sync::tests::terminal_batch_status_fails_without_state_reconciliation`. + The test incorrectly started four concurrent unsynchronized streams while + asserting that `messages` must fail first. The fixture was made deterministic + by advancing the other three stream cursors (`state_response(0, 1, 1, 1)` so + only `messages` needs a `/batch`, and `assert_eq!(batch_count, 1)`). No sync + production behavior changed; this is not part of external-taint architecture + and the PR is not broadened into further sync cleanup. Recorded here rather + than silently violating the "mutation-cursor runtime + docs only" scope + statement. +- **PR-review follow-up (T05, post-stack).** After the T01–T04 stack, PR #245 + review added: a distinct `CoordinateError::MarkerClearAfterCommit { source, + committed: Box }` so a post-commit `marker.clear()` failure + no longer collapses into the same shape as a pre-commit inspect/persist + failure and can no longer hide a committed `MutationEvent` (`ExternalTaintOperation` + loses its `Clear` arm); a private `coordinate_inner(.., after_recovery)` / + `coordinate_boundary_inner(.., after_recovery: FnMut(u32) -> Result<()>)` seam + and the AC8 regression + `a_failure_after_recovery_before_boundary_commit_leaves_marker_and_forces_later_recovery`; + removal of the now-obsolete module-wide `#![allow(dead_code)]` in + `external_taint.rs`; and the acceptance-criteria / `Validate:` bookkeeping fix + above. + ## Task context synchronization lifecycle Persist this field in every plan; this is durable plan state, not chat state: @@ -247,10 +285,17 @@ local choices, not new requirements. plus `inherited_external_taint`. A future harness adapter builds the provider closure around `agent_trace_storage::resolve_agent_trace_storage_for_hook_runtime` and hands `coordinate()` the resulting `.db`. -- Marker I/O failures surface as a dedicated `CoordinateError` variant, e.g. +- Pre-commit marker I/O failures surface as a dedicated `CoordinateError` + variant, e.g. `ExternalTaintMarker { operation: ExternalTaintOperation, source: anyhow::Error }` - with `ExternalTaintOperation { Inspect, Persist, Clear }`. A DB-provider - failure after the marker is armed surfaces as a separate variant, e.g. + with `ExternalTaintOperation { Inspect, Persist }`. A post-commit + `marker.clear()` failure surfaces as a *distinct* variant that carries the + already-committed outcome, e.g. + `MarkerClearAfterCommit { source: anyhow::Error, committed: Box }` + (see the PR #245 review deviation below) — it is never collapsed into the + inspect/persist shape, because those happen before any protected work and a + clear failure happens after a durable commit. A DB-provider failure after the + marker is armed surfaces as a separate variant, e.g. `CoordinateError::AgentTraceDbUnavailable(anyhow::Error)`, and intentionally leaves the marker in place. - The new primitive lives at `runtime/external_taint.rs` as diff --git a/spec/mutation_cursor.md b/spec/mutation_cursor.md index 957c938c..67a1e6c4 100644 --- a/spec/mutation_cursor.md +++ b/spec/mutation_cursor.md @@ -45,7 +45,7 @@ Database unavailability is different. `databaseFailure(worktree)` changes only: externalTaint: Set[WorktreeId] ``` -`externalTaint` is the abstract external durability boundary: conceptually, the filesystem `TAINTED` marker that can survive an unavailable database. It is not a database row and does not model marker paths or filesystem syscalls. Its concrete refinement in the CLI is the worktree-local marker file `/sce/mutation-cursor-tainted`, armed write-ahead at the start of the protected runtime section — before Agent Trace DB acquisition — and cleared only after a proven durable completion; it becomes protocol `externalTaint` (overlaid onto `databaseFailure` recovery) only when a later invocation inherits it. See `context/cli/mutation-trace-external-taint.md`. +`externalTaint` is the abstract external durability boundary: conceptually, the filesystem `TAINTED` marker that can survive an unavailable database. It is not a database row and does not model marker paths or filesystem syscalls. Its concrete refinement in the CLI is the worktree-local marker file `/sce/mutation-cursor-tainted`, armed write-ahead at the start of the protected runtime section — before Agent Trace DB acquisition — and cleared only after a proven durable completion; it becomes protocol `externalTaint` (overlaid onto `databaseFailure` recovery) only when a later invocation inherits it. If that trailing clear fails *after* the boundary has committed durably, the CLI keeps the marker armed and returns the committed outcome inside a distinct error rather than reporting a boundary failure, so the next invocation still recovers conservatively. See `context/cli/mutation-trace-external-taint.md`. Thus the model does **not** perform this contradictory transition: From 2a0fa06786cd0d6b9ddb067e8b9d44b78cdc045a Mon Sep 17 00:00:00 2001 From: David Abram Date: Sun, 30 Aug 2026 20:22:28 +0200 Subject: [PATCH 8/8] runtime+context: Tighten recovery boundary event assertion Identify the triggering Advance by its scope and event key, then verify that recovery does not mark that boundary event as processed after a failed invocation. Record the tightened AC8 validation in the mutation-cursor-external-taint plan. Co-authored-by: SCE --- .../mutation_trace/runtime/coordinator.rs | 14 ++-- .../plans/mutation-cursor-external-taint.md | 72 ++++++++++++++++--- 2 files changed, 71 insertions(+), 15 deletions(-) diff --git a/cli/src/services/mutation_trace/runtime/coordinator.rs b/cli/src/services/mutation_trace/runtime/coordinator.rs index 5678df57..3cb218ff 100644 --- a/cli/src/services/mutation_trace/runtime/coordinator.rs +++ b/cli/src/services/mutation_trace/runtime/coordinator.rs @@ -523,7 +523,7 @@ mod tests { use super::*; use crate::services::mutation_trace::store::encode_revision; - use crate::services::mutation_trace::types::{Attribution, FailureKind, ScopeStatus}; + use crate::services::mutation_trace::types::{Attribution, EventKey, FailureKind, ScopeStatus}; static NEXT_TEST_DB_ID: AtomicU64 = AtomicU64::new(0); @@ -2070,11 +2070,17 @@ mod tests { ); std::fs::write(repo_root.join("work.txt"), b"b").expect("the A -> B edit should write"); + let advance_event = EventId("evt-advance".to_string()); + let event_key = EventKey { + scope_id: scope.clone(), + event_id: advance_event.clone(), + }; + let error = coordinate_inner( &repo_root, &RuntimeBoundary::Advance { scope: scope.clone(), - event: EventId("evt-advance".to_string()), + event: advance_event.clone(), actor_kind: ActorKind::ClaudeCode, }, ok_db, @@ -2096,7 +2102,7 @@ mod tests { let store_db = ok_db().expect("reopening the DB for assertions should succeed"); let store = MutationTraceStore::new(&store_db); let after_fail = store - .load_worktree(&worktree_id, Some(&scope), None) + .load_worktree(&worktree_id, Some(&scope), Some(&event_key)) .expect("loading the worktree row should succeed") .expect("the worktree row should exist"); assert!( @@ -2122,7 +2128,7 @@ mod tests { "the live scope was abandoned by the durable recovery" ); assert!( - after_fail.processed_events.is_empty(), + !after_fail.processed_events.contains(&event_key), "the triggering Advance must never have been processed" ); assert!( diff --git a/context/plans/mutation-cursor-external-taint.md b/context/plans/mutation-cursor-external-taint.md index b834741b..9345a142 100644 --- a/context/plans/mutation-cursor-external-taint.md +++ b/context/plans/mutation-cursor-external-taint.md @@ -189,17 +189,26 @@ which criterion they map to. ### Deviations from stated scope -- **Unrelated sync test stabilization (test-only).** During full-suite - validation an unrelated pre-existing flaky sync test was exposed: - `services::sync::sync::tests::terminal_batch_status_fails_without_state_reconciliation`. - The test incorrectly started four concurrent unsynchronized streams while - asserting that `messages` must fail first. The fixture was made deterministic - by advancing the other three stream cursors (`state_response(0, 1, 1, 1)` so - only `messages` needs a `/batch`, and `assert_eq!(batch_count, 1)`). No sync - production behavior changed; this is not part of external-taint architecture - and the PR is not broadened into further sync cleanup. Recorded here rather - than silently violating the "mutation-cursor runtime + docs only" scope - statement. +- **Unrelated sync-test stabilization (test-only).** During full-suite + validation, two pre-existing sync tests were found to depend on concurrent + stream request ordering: + + - `services::sync::sync::tests::terminal_batch_status_fails_without_state_reconciliation` + previously seeded all four streams while asserting that `messages` must be + the stream whose terminal error is observed first. The fixture now advances + the other three cursors (`state_response(0, 1, 1, 1)`), so only `messages` + sends one `/batch` request (`assert_eq!(batch_count, 1)`). + + - `services::sync::sync::tests::malformed_2xx_batch_response_still_reconciles_via_state` + previously seeded all four streams and depended on concurrent requests + consuming canned responses in a particular order. It now seeds only one + `messages` row and deterministically asserts the intended request sequence: + `/state → /batch → /state → /batch`. + + No sync production behavior changed. These are test-only determinism fixes + discovered during PR validation and are not part of the external-taint + architecture. Recorded here rather than silently violating the + "mutation-cursor runtime + docs only" scope statement. - **PR-review follow-up (T05, post-stack).** After the T01–T04 stack, PR #245 review added: a distinct `CoordinateError::MarkerClearAfterCommit { source, committed: Box }` so a post-commit `marker.clear()` failure @@ -655,3 +664,44 @@ calling a lower-level coordinator. Whether a later change also splits halves is left to implementation — the plan only requires the narrowest seam that puts DB acquisition inside the fence, and the caller-supplied provider closure achieves that without touching `agent_trace_storage` at all. + +## Validation Report + +**Status:** validated +**Date:** 2026-08-30 (revalidated after the AC8 `processed_events` assertion tightening) + +### Commands run + +- `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml services::mutation_trace::runtime::` -> exit 0 (56 passed, 0 failed; every AC-mapped `external_taint`, `coordinator`, and cross-module `runtime::tests` case passed) +- `./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml` -> exit 0 (845 passed, 0 failed; full CLI suite, including `mbt` Quint-refinement tests and the stabilized `sync` batch tests) +- `./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml --all-targets -- -D warnings` -> exit 0 (clean, no warnings) +- `./scripts/run-cli-cargo.sh fmt --manifest-path cli/Cargo.toml -- --check` -> exit 0 (clean) +- `nix run .#pkl-check-generated` -> exit 0 (ephemeral Pkl generation passed: 141 files, inventory sha256 bf5db9c962cc9ce2776b4fc218dcbd8787fa7567744a5e2faff1fc9f9212a003) +- `nix flake check` -> exit 0 (all 4 flake checks passed, including `checks.cli-tests` and `checks.mutation-trace-quint-connect`) + +### Success-criteria verification + +- [x] AC1: Marker is stored under the caller-supplied worktree git-dir, survives handle reconstruction, and two `git_dir` inputs derive independent paths/state -> `external_taint::tests::marker_is_worktree_scoped` + `marker_persists_until_explicitly_cleared` passed +- [x] AC2: `persist`/`persist`/`clear`/`clear` all succeed; `clear` on an absent marker is success -> `external_taint::tests::persist_and_clear_are_idempotent` passed +- [x] AC3: Public `coordinate()` arms its marker internally and clears it only after a successful `CoordinateOutcome` -> `coordinator::tests::public_coordinate_clears_marker_on_success` + `runtime::tests::a_successful_coordinate_through_the_public_api_leaves_no_external_taint_marker` passed +- [x] AC4: After arming, any coordinator error leaves the marker present (snapshot-failure and deterministic non-snapshot paths) -> `coordinator::tests::public_coordinate_leaves_marker_after_a_snapshot_failure`, `..._after_a_non_snapshot_failure`, `..._when_the_db_provider_fails` passed +- [x] AC5: Inherited marker rebaselines to current tree, emits no A→C evidence, abandons scopes, then processes the boundary -> `runtime::tests::a_stale_marker_rebaselines_to_the_current_tree_abandons_scopes_then_processes_the_boundary` + `coordinator::tests::inherited_external_taint_recovers_once_before_the_boundary` passed +- [x] AC6: Recovery and triggering-boundary processing share one `observed_tree`; no second Git snapshot -> `coordinator::tests::inherited_external_taint_recovers_once_before_the_boundary` (call-counting capture asserts exactly one `capture`/`pin`) passed +- [x] AC7: Losing recovery CAS re-injects external taint on reload and recomputes until `Applied`/exhaustion; marker persists through the loop -> `coordinator::tests::a_losing_recovery_cas_reinjects_external_taint_until_it_applies`, `a_landed_recovery_clears_the_flag_so_a_boundary_cas_retry_does_not_re_recover`, `runtime::tests::a_snapshot_failure_arms_the_marker_and_the_next_invocation_recovers_once` passed +- [x] AC8: Failure after recovery commit but before boundary completion leaves recovery durable, boundary incomplete, marker present; post-commit `marker.clear()` failure returns `MarkerClearAfterCommit` carrying the committed outcome -> `coordinator::tests::a_failure_after_recovery_before_boundary_commit_leaves_marker_and_forces_later_recovery` + `runtime::tests::a_marker_clear_failure_after_a_durable_boundary_keeps_the_marker_for_a_later_recovery` passed +- [x] AC9: Marker surviving an invocation that never materialized a worktree row → next successful invocation baselines with no evidence for the unknown interval -> `coordinator::tests::inherited_external_taint_with_no_worktree_row_baselines_without_evidence` + `runtime::tests::a_first_ever_failed_invocation_that_never_materialized_a_worktree_row_creates_no_evidence` passed +- [x] AC10: Worktree with live scopes and inherited taint persists them as `Abandoned` during recovery, never eligible for exclusive attribution afterward -> `coordinator::tests::inherited_external_taint_recovers_once_before_the_boundary` (asserts `ScopeStatus::Abandoned`) + `recovers_from_snapshot_failure_taint_abandoning_live_scopes` passed +- [x] AC11: End to end, no `MutationEvent` treats an interval spanning a failed invocation as one trustworthy AI-attributable interval -> `runtime::tests::a_db_open_failure_after_arming_leaves_the_marker_and_the_next_invocation_rebaselines_without_evidence` (per-revision `load_mutation_event` sweep, no event ends at the post-gap tree) passed +- [x] AC12: A marker in linked worktree A does not trigger recovery in linked worktree B over a shared repository Agent Trace DB -> `runtime::tests::linked_worktrees_keep_independent_external_taint_markers_over_a_shared_db` passed +- [x] AC13: When marker inspection or persistence cannot be established, `coordinate()` returns a distinct external-taint marker error before any checkout-identity, DB, snapshot, or protocol processing -> `coordinator::tests::public_coordinate_fails_closed_when_the_marker_cannot_be_armed` + `..._when_marker_inspection_fails` passed +- [x] AC14: DB acquisition is inside the fence — a DB-provider `Err` after lock+arming makes `coordinate()` return an error with the marker present though no `RepositoryAgentTraceDb` existed; the follow-up invocation with a working provider snapshots, recovers, and produces no evidence across the lost interval -> `runtime::tests::a_db_open_failure_after_arming_leaves_the_marker_and_the_next_invocation_rebaselines_without_evidence` + `coordinator::tests::public_coordinate_leaves_marker_when_the_db_provider_fails` passed + +### Failed checks and follow-ups + +- None. + +### Residual risks + +- Marker durability is best-effort at the parent-directory `sync_all` level (`#[cfg(unix)]`, error swallowed); the plan explicitly does not claim durability across host power loss or filesystem-level crash. +- The `spec/mutation_cursor.qnt` model and Quint refinement matrix are unchanged by this plan; the abstract↔concrete correspondence for the write-ahead marker is asserted by the Rust integration tests and prose refinement notes, not by a machine-checked refinement. +- The recorded sync-test stabilization (`cli/src/services/sync/sync.rs`, test-only) touched `terminal_batch_status_fails_without_state_reconciliation` and `malformed_2xx_batch_response_still_reconciles_via_state`; no sync production behavior changed and the full suite is green.