From acc3de8da4faae92f5afaf543743514b7e4e606b Mon Sep 17 00:00:00 2001 From: David Abram Date: Fri, 4 Sep 2026 01:54:42 +0200 Subject: [PATCH 1/5] planning: Add mutation-scope hook ingress plan Define a harness-neutral CLI ingress for normalized mutation-scope lifecycle events, including strict parsing, runtime wiring, failure semantics, and lazy database access. Track parser and routing work, runtime integration, real Git/DB regressions, and durable context synchronization. Plan: mutation-scope-hook-ingress (T01-T05) Co-authored-by: SCE --- context/plans/mutation-scope-hook-ingress.md | 413 +++++++++++++++++++ 1 file changed, 413 insertions(+) create mode 100644 context/plans/mutation-scope-hook-ingress.md diff --git a/context/plans/mutation-scope-hook-ingress.md b/context/plans/mutation-scope-hook-ingress.md new file mode 100644 index 00000000..1debb009 --- /dev/null +++ b/context/plans/mutation-scope-hook-ingress.md @@ -0,0 +1,413 @@ +# Plan: mutation-scope-hook-ingress + +## Change summary + +The mutation-scope runtime (`cli/src/services/mutation_trace/runtime/`) is fully +built: `coordinate()` drives observed `Start`/`Advance`/`Close`/`Flush` +boundaries against a real worktree behind the shared `ProtectedWorktree` safety +prefix, and `abandon_scope()` retires a scope whose final boundary was never +observed. Both are `pub(crate)` re-exported from `runtime/mod.rs`, and the +harness-adapter contract is recorded in `context/cli/mutation-scope-runtime.md`. +Nothing calls either entrypoint — no hook, plugin, extension, or command. + +This plan adds one harness-neutral CLI ingress, `sce hooks mutation-scope`, that +reads a single normalized JSON object from STDIN, strictly parses and validates +it, translates it directly into a `RuntimeBoundary` (for `start`/`advance`/ +`close`/`flush`) or an `abandon_scope()` call (for `abandon`), and invokes the +existing runtime with a **lazy** DB provider so DB acquisition stays inside the +runtime's protected-worktree ordering. It is the generic transport/normalization +seam every future Claude Code, Codex, OpenCode, and Pi adapter will target; it +contains no concrete harness mapping and no lifecycle-event translation. + +The ingress owns nothing durable. The external adapter owns `scope_id`, +`event_id`, and `actor_kind`; SCE owns `worktree_id`, Git tree identities, +mutation revisions, and attempt IDs. The payload never accepts `worktree_id` — +worktree identity is derived by the runtime from the invoking checkout. +`scope_id` and `event_id` are translated verbatim into `ScopeId(..)` / +`EventId(..)` with no prefixing, hashing, normalization, UUID generation, or +timestamping, because `EventId` equality is the existing replay/idempotency key. + +Unlike `diff-trace` / `conversation-trace`, a lost mutation-scope lifecycle +boundary can change which scope stays live and therefore alter attribution, so a +valid boundary must never be silently discarded. Two failure classes are +distinguished, matching what the runtime already models: + +- An ordinary runtime error *before* durable completion + (`CoordinateError` / `AbandonScopeError` variants other than the two below) is + a command failure with non-zero exit. +- `CoordinateError::MarkerClearAfterCommit { committed, source }` and + `AbandonScopeError::MarkerClearAfterCompletion { completed, source }` mean the + durable mutation operation **already succeeded** and only the trailing + external-taint marker cleanup failed. The ingress treats these as durable + success: it reports the cleanup failure diagnostically, emits empty stdout, and + exits zero. It does **not** re-run or retry the mutation transition. The marker + stays armed, so the next runtime invocation recovers conservatively per + existing runtime semantics. + +Successful operations produce empty stdout; the ingress serializes no outcome, +revision, worktree ID, or scope state. + +This extends existing behavior and disturbs none of it: no change to +`spec/mutation_cursor.qnt`, `protocol.rs`, the mutation-trace SQL schema, +migrations, `diff_traces`, `post_commit_patch_intersections`, `agent_traces`, or +#259 attribution behavior. The pure mutation-domain types gain no serde derives +for this command. + +## 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: `sce hooks mutation-scope` exists and routes through the normal + CLI/hook command stack (`cli_schema::HooksSubcommand::MutationScope` → + `convert_hooks_subcommand_request` → `services::hooks::HookSubcommand::MutationScope` + → `run_hooks_subcommand_in_repo`), hidden with the rest of the `hooks` surface. + - Validate: `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml services::parse::command_runtime` plus `services::hooks::` — a parser test asserts `sce hooks mutation-scope` converts to `HookSubcommand::MutationScope`; `sce hooks --help` does not appear in top-level `sce --help`. +- [ ] AC2: The normalized JSON contract strictly supports exactly `start`, + `advance`, `close`, `flush`, `abandon` with exact field validation, rejecting + unknown operation, unknown `actor_kind`, missing/empty/blank `scope_id` or + `event_id`, unexpected fields, any `worktree_id` key, wrong JSON type, and + malformed JSON. `flush` accepts no scope/event/actor fields; `abandon` accepts + only `scope_id`. + - Validate: `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml services::hooks::mutation_scope` — focused parser tests cover every operation and every listed rejection case. +- [ ] AC3: `start` / `advance` / `close` map exactly to `RuntimeBoundary::Start` + / `Advance` / `Close` and forward `ScopeId`, `EventId`, and `ActorKind` + unchanged (`claude_code → ClaudeCode`, `codex → Codex`, `opencode → OpenCode`, + `pi → Pi`). + - Validate: integration test T03-Test1 asserts durable processed-event keys `(A,e1)`,`(A,e2)`,`(A,e3)` exactly as supplied; T03-Test3 asserts a mismatched `actor_kind` reaches `ScopeIdentityConflict`. +- [ ] AC4: `flush` maps to `RuntimeBoundary::Flush` with no scope/event/actor + identity, and drives the runtime's real observed-flush behavior. Against a + baseline tree followed by an unscoped filesystem edit, a `{"operation":"flush"}` + advances `cursor_tree` to the edited Git tree, advances `revision` by one, + writes exactly one `mutation_trace_events` row for the tree transition with + `attribution = IneligibleUnscoped`, invents no `mutation_trace_scopes` row, and + invents no `mutation_trace_processed_events` row. + - Validate: `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml services::hooks::mutation_scope` — integration test T03-Test5 asserts each durable value against real Git trees; a `Flush => Ok("")` stub fails it. +- [ ] AC5: `abandon` calls `abandon_scope()` directly and never acquires + `Close`/`Flush` snapshot semantics. In `start(A,e1) → record cursor_tree → + unobserved filesystem edit → abandon(A)`, `abandon_scope()` captures no Git + snapshot and no mutation boundary, and `cursor_tree` stays the pre-edit tree — + it does **not** become the edited Git tree — with no `mutation_trace_events` + row for the edit. + - Validate: `rg -n 'RuntimeBoundary|GitSnapshotService|capture_tree|pin_tree|diff_trees|coordinate\(' cli/src/services/hooks/mutation_scope.rs` shows the abandon arm calls only `abandon_scope`; integration test T03-Test4 asserts the `cursor_tree` invariance and absent rows. +- [ ] AC6: The ingress never accepts or constructs a `WorktreeId`; the payload + type has no worktree field and the module never names `WorktreeId`. + - Validate: `rg -n 'WorktreeId|worktree_id' cli/src/services/hooks/mutation_scope.rs` returns no constructor/field use; parser test rejects a payload containing `"worktree_id"`. +- [ ] AC7: DB acquisition stays lazy inside the runtime's protected-worktree + sequence — the ingress passes a `FnOnce` provider closure to `coordinate()` and + `abandon_scope()`, never an already-open handle, reusing + `open_agent_trace_db_for_hook_runtime`. + - Validate: `rg -n 'open_agent_trace_db_for_hook_runtime|open_db|coordinate\(|abandon_scope\(' cli/src/services/hooks/mutation_scope.rs` shows the DB resolver is invoked only inside the provider closure passed to the runtime entrypoint. +- [ ] AC8: Runtime results are classified by durable completion, not fail-open: + - a pre-completion `CoordinateError` / `AbandonScopeError` (any variant other + than the two carried-outcome variants) → `CliError` / non-zero exit; + - `CoordinateError::MarkerClearAfterCommit { committed, .. }` and + `AbandonScopeError::MarkerClearAfterCompletion { completed, .. }` → durable + success: the carried outcome is treated as the result, the marker-cleanup + failure is logged diagnostically, stdout is empty, exit is zero, and the + runtime transition is **not** executed or retried again; + - no `"failed open" / exit 0` branch exists for a dropped or malformed boundary. + - Validate: `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml services::hooks::mutation_scope` — tests assert (a) malformed JSON and an injected ordinary runtime error return `Err`; (b) an injected `MarkerClearAfterCommit` and an injected `MarkerClearAfterCompletion` each return `Ok` with empty stdout and the runtime entrypoint is invoked exactly once (no second transition); `rg -n 'fail.open|failed open|exit 0' cli/src/services/hooks/mutation_scope.rs` returns nothing. +- [ ] AC9: A successful mutation-scope hook execution produces empty stdout, with + no serialized `CoordinateOutcome`, `AbandonScopeOutcome`, `MutationEvent`, + revision, worktree ID, or scope state. + - Validate: integration tests assert the returned success string is empty (zero stdout bytes). +- [ ] AC10: A real Git/DB `Start → edit → Advance → Close` flow through the + ingress creates scope status `Closed`, processed events `(A,e1)`,`(A,e2)`,`(A,e3)`, + exactly one mutation event over the edit interval with `AiExclusive(A)`, and a + cursor tree equal to the final observed Git tree. + - Validate: `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml services::hooks::mutation_scope` — integration test T03-Test1 asserts each durable value against real Git trees, with no manually inserted mutation event. +- [ ] AC11: A replayed `(ScopeId, EventId)` boundary through the ingress is + fully idempotent. In `start(A,e1) → edit → advance(A,e2)`, with `revision`, + `mutation_trace_events` count, and `mutation_trace_processed_events` count + snapshotted immediately after the first `advance(A,e2)`, a second + `advance(A,e2)` leaves all three counts unchanged and `mutation_trace_processed_events` + holds exactly one key `(A,e2)`. The test fails if the ingress regenerates, + prefixes, hashes, or otherwise transforms `EventId`. + - Validate: integration test T03-Test2 snapshots the three values before replay and asserts equality plus the single `(A,e2)` processed key via direct DB reads. +- [ ] AC12: A conflicting `ActorKind` for an existing scope commits no second + boundary. In `start(A,e1,claude_code)` (recording `revision`, processed-event + rows, scope actor/status) → `advance(A,e2,codex)`, the runtime returns + `ScopeIdentityConflict` and afterwards: `revision` is unchanged, `(A,e2)` is + absent from `mutation_trace_processed_events`, scope `A` still has + `actor_kind = claude_code` and an unchanged status, and no new + `mutation_trace_events` row exists (the assertion holds even when the rejected + boundary observed no tree change). + - Validate: integration test T03-Test3 asserts each value. +- [ ] AC13: Abandonment through the ingress retains no-snapshot semantics and + the correct durable transition. In `start(A,e1) → record cursor_tree → + unobserved edit → abandon(A)`: scope `A` is `Abandoned`, `revision` advances + exactly once for the abandonment, `needs_rebaseline` is `true`, `cursor_tree` + remains the pre-edit tree (never the edited Git tree), and no + `mutation_trace_events` row for the edit and no `mutation_trace_processed_events` + row from abandon exist. + - Validate: integration test T03-Test4 asserts each durable value after the sequence, exercising the real `abandon_scope()` path. +- [ ] AC14: The ingress reaches durable storage only through the existing + mutation runtime, writing `mutation_trace_*` rows only. No production ingress + path writes `diff_traces`, `post_commit_patch_intersections`, or `agent_traces`. + The T03 integration tests are expected to read/assert those three table names in + order to prove they stay empty; the ban is on the production write path, not on + table names appearing in the source file. + - Validate: the T03 regressions assert `diff_traces`, `post_commit_patch_intersections`, and `agent_traces` each hold zero rows after every ingress flow; and the production implementation (excluding the `#[cfg(test)]` section) calls none of `insert_diff_trace`, `DiffTraceInsert`, `insert_post_commit_patch_intersection`, `PostCommitPatchIntersectionInsert`, `insert_agent_trace`, or `AgentTraceInsert` — confirm by inspecting the non-test module body, or `rg -n 'insert_diff_trace|DiffTraceInsert|insert_post_commit_patch_intersection|PostCommitPatchIntersectionInsert|insert_agent_trace|AgentTraceInsert' cli/src/services/hooks/mutation_scope.rs` shows hits only within the `#[cfg(test)]` test module, if any. +- [ ] AC15: No change attributable to this plan exists in + `spec/mutation_cursor.qnt`, `cli/src/services/mutation_trace/protocol.rs`, + `cli/migrations/agent-trace-repository/`, or the Agent Trace schema. + - Validate: `git diff origin/mutation-trace-agent-attribution -- spec/mutation_cursor.qnt cli/src/services/mutation_trace/protocol.rs cli/migrations/agent-trace-repository/ config/schema/agent-trace.schema.json` is empty. +- [ ] AC16: Durable context establishes this as the generic adapter seam and + states that concrete harness lifecycle integration remains future work. + - Validate: `context/cli/mutation-scope-hook-ingress.md` exists and documents the JSON contract, operation mapping, identity ownership, the no-`worktree_id` rule, the no-`ScopeId`/`EventId`-generation rule, the non-fail-open error semantics including the marker-clear-after-durable-completion classification, the lazy DB-provider requirement, the empty-stdout contract, abandonment ownership, and the generic-ingress vs harness-adapter boundary; `context/cli/mutation-scope-runtime.md` Status says a generic ingress now drives the runtime with no concrete harness adapter wired. + +### Full validation + +- `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml services::hooks::` +- `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml services::mutation_trace::` +- `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml` +- `nix develop -c ./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml --all-targets -- -D warnings` +- `nix develop -c ./scripts/run-cli-cargo.sh fmt --manifest-path cli/Cargo.toml -- --check` +- `nix run .#pkl-check-generated` +- `nix flake check` +- Confirm the branch diff is against `origin/mutation-trace-agent-attribution`, not `main`. + +### Context sync + +- New: `context/cli/mutation-scope-hook-ingress.md` — the normalized JSON + contract, operation mapping, identity ownership, why `worktree_id` is refused, + why `ScopeId`/`EventId` are not generated, why errors are not fail-open, the + marker-clear-after-durable-completion classification, the lazy DB-provider + requirement, the silent-success stdout contract, abandonment ownership, and the + generic-ingress vs harness-adapter boundary. +- `context/cli/mutation-scope-runtime.md` — Status section: a generic + mutation-scope hook ingress now drives the runtime; no concrete Claude Code, + Codex, OpenCode, or Pi lifecycle adapter is wired yet. +- `context/sce/agent-trace-hooks-command-routing.md` — add `sce hooks + mutation-scope` to the implemented command surface and describe its + non-fail-open intake contract (including the two carried-outcome success + variants) distinct from `diff-trace`/`conversation-trace`. +- `context/context-map.md` — register the new domain file and update the + hooks-routing / mutation-scope-runtime annotations. +- `context/overview.md` — update the `mutation_trace` paragraph clause that says + the runtime entrypoints are "still not wired into any hook or command". + +## 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/hooks/mutation_scope.rs` (new), + `cli/src/services/hooks/mod.rs` (module registration + dispatch + + `hook_runtime_invocation_name`), `cli/src/cli_schema.rs` + (`HooksSubcommand::MutationScope`), `cli/src/services/parse/command_runtime.rs` + (`convert_hooks_subcommand_request` + tests), the five durable-context files in + **Context sync**. +- **Out of scope:** any concrete harness mapping (Claude Code hooks, Codex hook + mapping, OpenCode plugin, Pi extension); `SubagentStart`/`SubagentStop`/ + `PostToolUse`/tool-call translation; `session → ScopeId` or `tool-call → + EventId` derivation; PID tracking, process supervisors, staleness detection, + automatic scope abandonment; harness settings generation or `sce setup` + integration for the new hook; new mutation protocol semantics, Quint actions, + DB tables, migrations, retention, or GC; any Agent Trace attribution, + post-commit, diff-trace, or conversation-trace behavior change; #259 + attribution behavior. Changing the underlying runtime error types. +- **Constraints:** stacked on `mutation-trace-agent-attribution`; confirm all + diffs against `origin/mutation-trace-agent-attribution`. Reuse + `open_agent_trace_db_for_hook_runtime` as the sole DB resolver — no second + DB-opening implementation. Keep hook transport types local to + `mutation_scope.rs`. Do not add serde derives to `mutation_trace` domain types. + Preserve the runtime safety-prefix ordering: the DB provider closure must be + the value passed to `coordinate()` / `abandon_scope()`, never invoked before + them. Follow the repo's inline `#[cfg(test)] mod tests` + RAII + `tempfile::TempDir` + real `git init` / `RepositoryAgentTraceDb` pattern + (`context/patterns.md`). +- **Non-goal:** turning `abandon` into a `RuntimeBoundary` variant, or capturing + a Git snapshot on behalf of abandonment. The ingress does not interpret valid + runtime outcomes (`accepted = false`, `observes = false`, duplicate processed + event, no tree change, `Abandoned` / `AlreadyTerminal` / `RecoveryRequired`) — + those stay existing runtime semantics — beyond the marker-clear-after-durable- + completion classification required by AC8. There is never a completed task + state in which `sce hooks mutation-scope` accepts a valid lifecycle boundary + but does not drive the runtime. + +## Assumptions + +- The user's cover note allows ordinary local shape choices ("the exact Rust + shape may differ if repository conventions suggest something cleaner"). +- serde's `deny_unknown_fields` is not honored on the variant structs of an + internally tagged (`tag = "operation"`) enum. The parser therefore validates + "unexpected fields" / `worktree_id` rejection explicitly — e.g. deserialize the + tag first, then deserialize the remainder into a per-operation struct that does + carry `#[serde(deny_unknown_fields)]`, or match on a `serde_json::Map` and + reject unknown keys. This is a local implementation choice recorded here rather + than asked, per the note above. +- The command stays hidden because `HOOKS_SHOW_IN_TOP_LEVEL_HELP` is already + `false`; no new visibility flag is needed. +- The ingress reads the invoking checkout via the same `repository_root` + (`std::env::current_dir()`) that `run_hooks_subcommand` already resolves for + every hook; the runtime derives `git_dir` and `WorktreeId` from it. +- `CoordinateError::MarkerClearAfterCommit { source, committed: Box }` + and `AbandonScopeError::MarkerClearAfterCompletion { source, completed: Box }` + are the exact carried-outcome variants on the current base + (`origin/mutation-trace-agent-attribution`), confirmed by inspection of + `runtime/coordinator.rs` and `runtime/scope_runtime.rs`. +- Flush against an unscoped edit on a healthy, non-rebaseline worktree yields a + single `mutation_trace_events` row with `attribution = IneligibleUnscoped` and + no processed-event row (`is_hook(Flush) == false`), confirmed by inspection of + `protocol.rs` (`evaluate`/`apply`) and `types.rs` (`is_hook`, + `boundary_event_key`). + +## Task stack + +- [ ] T01: `Add strict normalized mutation-scope payload parser` (status:todo) + - Task ID: T01 + - Scope: In — new `cli/src/services/hooks/mutation_scope.rs` with the local + `MutationScopePayload` transport enum (`Start`/`Advance`/`Close`/`Flush`/ + `Abandon`), the ingress-local actor-kind parser mapping the four wire strings + (`claude_code`/`codex`/`opencode`/`pi`) to `ActorKind`, a + `parse_mutation_scope_payload(&str) -> Result` + function with strict wire-format validation (empty/blank string rejection, + unknown operation, unknown actor kind, missing fields, empty/blank + `scope_id`/`event_id`, unexpected fields, any `worktree_id` key, wrong JSON + type, malformed JSON, `flush` rejecting any scope/event/actor field, + `abandon` accepting only `scope_id`), the `pub mod mutation_scope;` + registration in `hooks/mod.rs` (under the existing `#[allow(dead_code)]` + module policy if needed), and focused `#[cfg(test)] mod tests` covering every + operation and every rejection case. Out — CLI routing, runtime invocation, + STDIN reading, context. + - Dependencies: none + - Done when: the module compiles, `parse_mutation_scope_payload` accepts each + valid operation shape and rejects each listed invalid case, and the parser + tests pass; `cargo clippy --all-targets -- -D warnings` and `fmt --check` are + clean for the new file. + - Verify: `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml services::hooks::mutation_scope`; `nix develop -c ./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml --all-targets -- -D warnings`. + - Context synchronization: pending + +- [ ] T02: `Add CLI routing and wire the existing runtime` (status:todo) + - Task ID: T02 + - Scope: In — `cli_schema::HooksSubcommand::MutationScope` (hidden, kebab + `mutation-scope`, "reads JSON payload from STDIN" about text); + `convert_hooks_subcommand_request` arm → + `services::hooks::HookSubcommand::MutationScope`; the `HookSubcommand` + variant; the `run_hooks_subcommand_in_repo` dispatch arm calling a new + `mutation_scope::run_mutation_scope_subcommand(repository_root, logger)`; + `hook_runtime_invocation_name` arm ("mutation-scope runtime invocation"); + updated parser/help tests in `command_runtime.rs` per repo convention. The + new function reads STDIN, calls `parse_mutation_scope_payload`, and drives the + runtime: `Start`/`Advance`/`Close` → `RuntimeBoundary::Start`/`Advance`/`Close` + with `ScopeId(scope_id)`, `EventId(event_id)`, mapped `ActorKind`, passed to + `mutation_trace::runtime::coordinate(repository_root, &boundary, || open_agent_trace_db_for_hook_runtime(repository_root, "Failed to open Agent Trace DB for mutation-scope runtime."))`; + `Flush` → `RuntimeBoundary::Flush` through the same call; `Abandon` → + `mutation_trace::runtime::abandon_scope(repository_root, &ScopeId(scope_id), || open_agent_trace_db_for_hook_runtime(..))`. + Result classification: a pre-completion `CoordinateError` / `AbandonScopeError` + → `CliError` (non-zero); `CoordinateError::MarkerClearAfterCommit { .. }` and + `AbandonScopeError::MarkerClearAfterCompletion { .. }` → durable success + (carried outcome kept, cleanup failure logged, empty stdout, exit 0, no + re-execution of the transition); any valid `CoordinateOutcome` / + `AbandonScopeOutcome` (including no-op/rejected/replayed) → empty stdout; no + outcome/revision/worktree/scope serialization to stdout. Malformed payload → + `CliError`. Focused `#[cfg(test)] mod tests` for the two carried-outcome + classifications (proving the runtime entrypoint runs once) and the malformed + → `Err` path, using a seam that lets the test inject each runtime result. + Out — real Git/DB regressions (T03), context (T04). Do not disturb + `pre-commit`/`commit-msg`/`post-commit`/`post-rewrite`/`diff-trace`/ + `conversation-trace`/`codex`/`claude-model-state`. Do not move DB acquisition + above the runtime's protected prefix. There must be no completed state in + which a valid payload returns success without invoking the runtime. + - Dependencies: T01 + - Done when: `sce hooks mutation-scope` parses through clap to + `HookSubcommand::MutationScope`; every valid operation reaches its mapped + runtime entrypoint with identities forwarded verbatim; the DB resolver is + only ever called inside the provider closure; pre-completion runtime failures + propagate as `CliError`; the two carried-outcome variants resolve to + empty-stdout success without re-running the transition; malformed payloads + return `CliError`; `sce --help` still hides `hooks`; existing hook + parser/help tests plus the new conversion and classification tests pass; + `clippy --all-targets -- -D warnings` and `fmt --check` are clean. + - Verify: `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml services::parse::command_runtime`; `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml services::hooks::`; `rg -n 'open_db|coordinate\(|abandon_scope\(|MarkerClearAfterCommit|MarkerClearAfterCompletion|open_agent_trace_db_for_hook_runtime' cli/src/services/hooks/mutation_scope.rs`; `nix develop -c ./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml --all-targets -- -D warnings`. + - Context synchronization: pending + +- [ ] T03: `Add real Git/DB mutation-scope ingress regressions` (status:todo) + - Task ID: T03 + - Scope: In — `#[cfg(test)] mod tests` coverage (in `mutation_scope.rs` or a + sibling test module following `hooks/mod.rs`'s `mutation_attribution_e2e` + precedent) that drives the real ingress entrypoint against an RAII + `tempfile::TempDir` real `git init` repository and a real temp-file + `RepositoryAgentTraceDb`, with direct durable-row assertions and real Git + trees: + - **Test1 — observed lifecycle:** `start(A,e1,claude_code) → edit → + advance(A,e2,claude_code) → close(A,e3,claude_code)`; assert scope `Closed`, + processed events `(A,e1)/(A,e2)/(A,e3)`, exactly one mutation event over + the edit interval, `AiExclusive(A)`, cursor tree = final observed Git tree, + no manually inserted event. + - **Test2 — replay:** `start(A,e1) → edit → advance(A,e2)`, snapshot + `revision` + `mutation_trace_events` count + `mutation_trace_processed_events` + count, then `advance(A,e2)` again; assert all three counts unchanged and + exactly one processed key `(A,e2)`. Fails if `EventId` is transformed. + - **Test3 — actor mismatch:** `start(A,e1,claude_code)` recording `revision` + + processed rows + scope actor/status, then `advance(A,e2,codex)`; assert + `ScopeIdentityConflict`, `revision` unchanged, `(A,e2)` absent from + `mutation_trace_processed_events`, scope `A` still `actor_kind = claude_code` + with unchanged status, no new `mutation_trace_events` row. + - **Test4 — abandonment with unobserved edit:** `start(A,e1) → record + cursor_tree → unobserved filesystem edit → abandon(A)`; assert scope + `Abandoned`, `revision` advanced exactly once, `needs_rebaseline = true`, + `cursor_tree` still the pre-edit tree (not the edited Git tree), no + `mutation_trace_events` row for the edit, no processed-event row from + abandon. + - **Test5 — adversarial flush:** baseline tree → unscoped filesystem edit → + `{"operation":"flush"}`; assert `cursor_tree` moved to the edited Git tree, + `revision` advanced, exactly one `mutation_trace_events` row for the + transition with `attribution = IneligibleUnscoped`, no `mutation_trace_scopes` + row invented, no `mutation_trace_processed_events` row invented. A + `Flush => Ok("")` stub must fail this test. + - **Test6 — marker-clear-after-commit carried outcome:** drive an + attributable `advance` whose trailing `marker.clear()` fails (via the + runtime's existing test seam), asserting the ingress returns empty-stdout + success, the committed `CoordinateOutcome` is the durable state, and the + transition is not executed a second time. + - **Test7 — marker-clear-after-abandon carried outcome:** the equivalent for + `AbandonScopeError::MarkerClearAfterCompletion`. + - Every test also asserts `diff_traces` / `post_commit_patch_intersections` / + `agent_traces` are untouched. + Out — context updates (T04). + - Dependencies: T02 + - Done when: all seven regressions pass, exercising the real `coordinate()` / + `abandon_scope()` paths through the ingress. + - Verify: `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml services::hooks::mutation_scope`; `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml services::mutation_trace::`. + - Context synchronization: pending + +- [ ] T04: `Document the mutation-scope hook ingress` (status:todo) + - Task ID: T04 + - Scope: In — create `context/cli/mutation-scope-hook-ingress.md` (JSON + contract, operation mapping, identity ownership, no-`worktree_id` rationale, + no-`ScopeId`/`EventId`-generation rationale, non-fail-open error semantics + including the two carried-outcome success variants and why they must not be + retried, lazy DB-provider requirement, empty-stdout contract, abandonment + ownership, generic-ingress vs harness-adapter boundary); update + `context/cli/mutation-scope-runtime.md` Status; add the command to + `context/sce/agent-trace-hooks-command-routing.md` with its non-fail-open + intake contract; register the new file and adjust annotations in + `context/context-map.md`; update the "not wired into any hook or command" + clause in `context/overview.md`. Out — code changes. + - Dependencies: T03 + - Done when: the new context file exists and covers every listed point, the + four updated files reflect the shipped ingress while preserving the + generic-ingress vs harness-adapter distinction, and `nix run .#pkl-check-generated` + plus `nix flake check` pass. + - Verify: `nix run .#pkl-check-generated`; `nix flake check`; `rg -n 'mutation-scope-hook-ingress' context/context-map.md context/cli/mutation-scope-runtime.md`. + - Context synchronization: pending + +## Open questions + +None. The runtime seam, the identity-ownership rules, and the two-class error +contract are all fixed by `context/cli/mutation-scope-runtime.md`, the runtime +source on the current base, and the request; the one implementation wrinkle +(serde `deny_unknown_fields` on an internally tagged enum) is a local parser +choice recorded under Assumptions, not a scope question. From d95836ab93efe51332fe4df53b7ce0d7860329df Mon Sep 17 00:00:00 2001 From: David Abram Date: Fri, 4 Sep 2026 02:24:38 +0200 Subject: [PATCH 2/5] hooks: Add strict mutation-scope payload parser Validate JSON STDIN payloads for start, advance, close, flush, and abandon operations while preserving boundary identifiers and mapping actor kinds to the runtime enum. Reject unknown fields, worktree identity, invalid types, and blank identifiers before routing is added. Plan: mutation-scope-hook-ingress.md (T01) Co-authored-by: SCE --- cli/src/services/hooks/mod.rs | 2 + cli/src/services/hooks/mutation_scope.rs | 402 +++++++++++++++++++ context/plans/mutation-scope-hook-ingress.md | 41 +- 3 files changed, 443 insertions(+), 2 deletions(-) create mode 100644 cli/src/services/hooks/mutation_scope.rs diff --git a/cli/src/services/hooks/mod.rs b/cli/src/services/hooks/mod.rs index abff0c34..7b49e3d2 100644 --- a/cli/src/services/hooks/mod.rs +++ b/cli/src/services/hooks/mod.rs @@ -44,6 +44,8 @@ pub mod claude_transcript; pub mod codex; pub mod command; pub mod lifecycle; +#[allow(dead_code)] +pub mod mutation_scope; pub const NAME: &str = "hooks"; pub const CANONICAL_SCE_COAUTHOR_TRAILER: &str = "Co-authored-by: SCE "; diff --git a/cli/src/services/hooks/mutation_scope.rs b/cli/src/services/hooks/mutation_scope.rs new file mode 100644 index 00000000..1ca9335a --- /dev/null +++ b/cli/src/services/hooks/mutation_scope.rs @@ -0,0 +1,402 @@ +use anyhow::{anyhow, bail, Context, Result}; +use serde_json::{Map, Value}; + +use crate::services::mutation_trace::types::ActorKind; + +const OPERATION_FIELD: &str = "operation"; +const SCOPE_ID_FIELD: &str = "scope_id"; +const EVENT_ID_FIELD: &str = "event_id"; +const ACTOR_KIND_FIELD: &str = "actor_kind"; +const WORKTREE_ID_FIELD: &str = "worktree_id"; + +const ACTOR_KIND_CLAUDE_CODE: &str = "claude_code"; +const ACTOR_KIND_CODEX: &str = "codex"; +const ACTOR_KIND_OPENCODE: &str = "opencode"; +const ACTOR_KIND_PI: &str = "pi"; + +#[derive(Clone, Debug, Eq, PartialEq)] +pub(crate) enum MutationScopePayload { + Start { + scope_id: String, + event_id: String, + actor_kind: ActorKind, + }, + Advance { + scope_id: String, + event_id: String, + actor_kind: ActorKind, + }, + Close { + scope_id: String, + event_id: String, + actor_kind: ActorKind, + }, + Flush, + Abandon { + scope_id: String, + }, +} + +pub(crate) fn parse_mutation_scope_payload(stdin_payload: &str) -> Result { + if stdin_payload.trim().is_empty() { + bail!(validation_error( + "expected a JSON object, got an empty payload" + )); + } + + let parsed: Value = serde_json::from_str(stdin_payload) + .with_context(|| validation_error("expected valid JSON"))?; + let object = parsed + .as_object() + .ok_or_else(|| anyhow!(validation_error("expected a JSON object")))?; + + let operation = required_str(object, OPERATION_FIELD)?; + + match operation.as_str() { + "start" => parse_scope_boundary(object, |scope_id, event_id, actor_kind| { + MutationScopePayload::Start { + scope_id, + event_id, + actor_kind, + } + }), + "advance" => parse_scope_boundary(object, |scope_id, event_id, actor_kind| { + MutationScopePayload::Advance { + scope_id, + event_id, + actor_kind, + } + }), + "close" => parse_scope_boundary(object, |scope_id, event_id, actor_kind| { + MutationScopePayload::Close { + scope_id, + event_id, + actor_kind, + } + }), + "flush" => parse_flush(object), + "abandon" => parse_abandon(object), + other => bail!(validation_error(&format!( + "field 'operation' must be one of 'start', 'advance', 'close', 'flush' or 'abandon', got '{other}'" + ))), + } +} + +fn parse_scope_boundary( + object: &Map, + build: impl FnOnce(String, String, ActorKind) -> MutationScopePayload, +) -> Result { + reject_unexpected_keys( + object, + &[ + OPERATION_FIELD, + SCOPE_ID_FIELD, + EVENT_ID_FIELD, + ACTOR_KIND_FIELD, + ], + )?; + + let scope_id = required_non_blank_str(object, SCOPE_ID_FIELD)?; + let event_id = required_non_blank_str(object, EVENT_ID_FIELD)?; + let actor_kind = parse_actor_kind(&required_str(object, ACTOR_KIND_FIELD)?)?; + + Ok(build(scope_id, event_id, actor_kind)) +} + +fn parse_flush(object: &Map) -> Result { + reject_unexpected_keys(object, &[OPERATION_FIELD])?; + Ok(MutationScopePayload::Flush) +} + +fn parse_abandon(object: &Map) -> Result { + reject_unexpected_keys(object, &[OPERATION_FIELD, SCOPE_ID_FIELD])?; + let scope_id = required_non_blank_str(object, SCOPE_ID_FIELD)?; + Ok(MutationScopePayload::Abandon { scope_id }) +} + +fn parse_actor_kind(wire: &str) -> Result { + match wire { + ACTOR_KIND_CLAUDE_CODE => Ok(ActorKind::ClaudeCode), + ACTOR_KIND_CODEX => Ok(ActorKind::Codex), + ACTOR_KIND_OPENCODE => Ok(ActorKind::OpenCode), + ACTOR_KIND_PI => Ok(ActorKind::Pi), + other => bail!(validation_error(&format!( + "field 'actor_kind' must be one of 'claude_code', 'codex', 'opencode' or 'pi', got '{other}'" + ))), + } +} + +fn reject_unexpected_keys(object: &Map, allowed: &[&str]) -> Result<()> { + for key in object.keys() { + if key == WORKTREE_ID_FIELD { + bail!(validation_error( + "field 'worktree_id' is not accepted; worktree identity is derived from the invoking checkout" + )); + } + if !allowed.contains(&key.as_str()) { + bail!(validation_error(&format!("unexpected field '{key}'"))); + } + } + Ok(()) +} + +fn required_field<'a>(object: &'a Map, field: &str) -> Result<&'a Value> { + object.get(field).ok_or_else(|| { + anyhow!(validation_error(&format!( + "missing required field '{field}'" + ))) + }) +} + +fn required_str(object: &Map, field: &str) -> Result { + required_field(object, field)? + .as_str() + .map(str::to_owned) + .ok_or_else(|| { + anyhow!(validation_error(&format!( + "field '{field}' must be a string" + ))) + }) +} + +fn required_non_blank_str(object: &Map, field: &str) -> Result { + let value = required_str(object, field)?; + if value.trim().is_empty() { + bail!(validation_error(&format!( + "field '{field}' must be a non-blank string" + ))); + } + Ok(value) +} + +fn validation_error(detail: &str) -> String { + format!("Invalid mutation-scope payload from STDIN: {detail}.") +} + +#[cfg(test)] +mod tests { + use super::*; + + fn parse(payload: &str) -> Result { + parse_mutation_scope_payload(payload) + } + + fn error_of(payload: &str) -> String { + parse(payload) + .expect_err("expected the payload to be rejected") + .to_string() + } + + #[test] + fn start_maps_all_fields_verbatim() { + let payload = parse( + r#"{"operation":"start","scope_id":" scope-A ","event_id":"e1","actor_kind":"claude_code"}"#, + ) + .expect("valid start payload"); + + assert_eq!( + payload, + MutationScopePayload::Start { + scope_id: " scope-A ".to_string(), + event_id: "e1".to_string(), + actor_kind: ActorKind::ClaudeCode, + } + ); + } + + #[test] + fn advance_and_close_parse_to_their_variants() { + assert_eq!( + parse(r#"{"operation":"advance","scope_id":"A","event_id":"e2","actor_kind":"codex"}"#) + .expect("valid advance payload"), + MutationScopePayload::Advance { + scope_id: "A".to_string(), + event_id: "e2".to_string(), + actor_kind: ActorKind::Codex, + } + ); + + assert_eq!( + parse( + r#"{"operation":"close","scope_id":"A","event_id":"e3","actor_kind":"opencode"}"# + ) + .expect("valid close payload"), + MutationScopePayload::Close { + scope_id: "A".to_string(), + event_id: "e3".to_string(), + actor_kind: ActorKind::OpenCode, + } + ); + } + + #[test] + fn every_actor_kind_wire_string_maps() { + for (wire, expected) in [ + ("claude_code", ActorKind::ClaudeCode), + ("codex", ActorKind::Codex), + ("opencode", ActorKind::OpenCode), + ("pi", ActorKind::Pi), + ] { + let payload = parse(&format!( + r#"{{"operation":"start","scope_id":"A","event_id":"e1","actor_kind":"{wire}"}}"# + )) + .expect("valid start payload"); + match payload { + MutationScopePayload::Start { actor_kind, .. } => assert_eq!(actor_kind, expected), + other => panic!("expected Start, got {other:?}"), + } + } + } + + #[test] + fn flush_takes_no_identity_fields() { + assert_eq!( + parse(r#"{"operation":"flush"}"#).expect("valid flush payload"), + MutationScopePayload::Flush + ); + } + + #[test] + fn abandon_takes_only_scope_id() { + assert_eq!( + parse(r#"{"operation":"abandon","scope_id":"A"}"#).expect("valid abandon payload"), + MutationScopePayload::Abandon { + scope_id: "A".to_string(), + } + ); + } + + #[test] + fn empty_or_blank_payload_is_rejected() { + assert!(parse("").is_err()); + assert!(parse(" \n\t ").is_err()); + } + + #[test] + fn malformed_json_is_rejected() { + assert!(parse("{").is_err()); + assert!(parse(r#"{"operation":"start""#).is_err()); + assert!(parse("not json at all").is_err()); + } + + #[test] + fn non_object_json_is_rejected() { + assert!(parse("123").is_err()); + assert!(parse(r#""start""#).is_err()); + assert!(parse(r#"["start"]"#).is_err()); + assert!(parse("null").is_err()); + } + + #[test] + fn missing_operation_is_rejected() { + assert!(parse(r#"{"scope_id":"A","event_id":"e1","actor_kind":"pi"}"#).is_err()); + } + + #[test] + fn unknown_operation_is_rejected() { + let error = + error_of(r#"{"operation":"reopen","scope_id":"A","event_id":"e1","actor_kind":"pi"}"#); + assert!(error.contains("'operation'"), "unexpected error: {error}"); + } + + #[test] + fn operation_wrong_type_is_rejected() { + assert!(parse(r#"{"operation":5}"#).is_err()); + } + + #[test] + fn unknown_actor_kind_is_rejected() { + let error = error_of( + r#"{"operation":"start","scope_id":"A","event_id":"e1","actor_kind":"cursor"}"#, + ); + assert!(error.contains("'actor_kind'"), "unexpected error: {error}"); + } + + #[test] + fn missing_scope_or_event_or_actor_is_rejected() { + assert!(parse(r#"{"operation":"start","event_id":"e1","actor_kind":"pi"}"#).is_err()); + assert!(parse(r#"{"operation":"start","scope_id":"A","actor_kind":"pi"}"#).is_err()); + assert!(parse(r#"{"operation":"start","scope_id":"A","event_id":"e1"}"#).is_err()); + } + + #[test] + fn empty_or_blank_scope_id_or_event_id_is_rejected() { + assert!( + parse(r#"{"operation":"start","scope_id":"","event_id":"e1","actor_kind":"pi"}"#) + .is_err() + ); + assert!(parse( + r#"{"operation":"start","scope_id":" ","event_id":"e1","actor_kind":"pi"}"# + ) + .is_err()); + assert!( + parse(r#"{"operation":"start","scope_id":"A","event_id":"","actor_kind":"pi"}"#) + .is_err() + ); + assert!( + parse(r#"{"operation":"start","scope_id":"A","event_id":"\t","actor_kind":"pi"}"#) + .is_err() + ); + assert!(parse(r#"{"operation":"abandon","scope_id":" "}"#).is_err()); + } + + #[test] + fn wrong_field_json_type_is_rejected() { + assert!( + parse(r#"{"operation":"start","scope_id":123,"event_id":"e1","actor_kind":"pi"}"#) + .is_err() + ); + assert!( + parse(r#"{"operation":"start","scope_id":"A","event_id":true,"actor_kind":"pi"}"#) + .is_err() + ); + assert!(parse( + r#"{"operation":"start","scope_id":"A","event_id":"e1","actor_kind":["pi"]}"# + ) + .is_err()); + } + + #[test] + fn unexpected_field_is_rejected() { + let error = error_of( + r#"{"operation":"start","scope_id":"A","event_id":"e1","actor_kind":"pi","attempt_id":"x"}"#, + ); + assert!( + error.contains("unexpected field 'attempt_id'"), + "unexpected error: {error}" + ); + } + + #[test] + fn any_worktree_id_key_is_rejected_with_a_dedicated_diagnostic() { + for payload in [ + r#"{"operation":"start","scope_id":"A","event_id":"e1","actor_kind":"pi","worktree_id":"wt"}"#, + r#"{"operation":"flush","worktree_id":"wt"}"#, + r#"{"operation":"abandon","scope_id":"A","worktree_id":"wt"}"#, + ] { + let error = error_of(payload); + assert!( + error.contains("'worktree_id'"), + "unexpected error for {payload}: {error}" + ); + } + } + + #[test] + fn flush_rejects_any_scope_event_or_actor_field() { + assert!(parse(r#"{"operation":"flush","scope_id":"A"}"#).is_err()); + assert!(parse(r#"{"operation":"flush","event_id":"e1"}"#).is_err()); + assert!(parse(r#"{"operation":"flush","actor_kind":"pi"}"#).is_err()); + } + + #[test] + fn abandon_rejects_event_and_actor_fields() { + assert!(parse(r#"{"operation":"abandon","scope_id":"A","event_id":"e1"}"#).is_err()); + assert!(parse(r#"{"operation":"abandon","scope_id":"A","actor_kind":"pi"}"#).is_err()); + } + + #[test] + fn abandon_missing_scope_id_is_rejected() { + assert!(parse(r#"{"operation":"abandon"}"#).is_err()); + } +} diff --git a/context/plans/mutation-scope-hook-ingress.md b/context/plans/mutation-scope-hook-ingress.md index 1debb009..ebbc4a59 100644 --- a/context/plans/mutation-scope-hook-ingress.md +++ b/context/plans/mutation-scope-hook-ingress.md @@ -265,7 +265,7 @@ Persist this field in every plan; this is durable plan state, not chat state: ## Task stack -- [ ] T01: `Add strict normalized mutation-scope payload parser` (status:todo) +- [x] T01: `Add strict normalized mutation-scope payload parser` (status:done) - Task ID: T01 - Scope: In — new `cli/src/services/hooks/mutation_scope.rs` with the local `MutationScopePayload` transport enum (`Start`/`Advance`/`Close`/`Flush`/ @@ -287,7 +287,44 @@ Persist this field in every plan; this is durable plan state, not chat state: tests pass; `cargo clippy --all-targets -- -D warnings` and `fmt --check` are clean for the new file. - Verify: `nix develop -c ./scripts/run-cli-cargo.sh test --manifest-path cli/Cargo.toml services::hooks::mutation_scope`; `nix develop -c ./scripts/run-cli-cargo.sh clippy --manifest-path cli/Cargo.toml --all-targets -- -D warnings`. - - Context synchronization: pending + - Completed: 2026-09-04 + - Files changed: + - `cli/src/services/hooks/mutation_scope.rs` (new) — `MutationScopePayload` + transport enum (`Start`/`Advance`/`Close`/`Flush`/`Abandon`), private + `parse_actor_kind` wire-string mapper, `parse_mutation_scope_payload` + strict parser, and `#[cfg(test)] mod tests` (20 tests). + - `cli/src/services/hooks/mod.rs` — `#[allow(dead_code)] pub mod mutation_scope;` + registration only. + - Result: Added the ingress-local wire-format parser. `MutationScopePayload` + carries `scope_id`/`event_id` as raw owned strings (no trim/prefix/hash) and + `actor_kind` as the real `ActorKind`. `parse_mutation_scope_payload` guards + empty/blank input, requires a JSON object with a required `operation` string, + and dispatches to a shared scope-boundary parser (`start`/`advance`/`close`), + a flush parser, or an abandon parser. Each enforces its exact allowed key set + via `reject_unexpected_keys`, which emits a dedicated diagnostic for any + `worktree_id` key. Rejected: malformed JSON, non-object JSON, wrong field + types, unknown operation, unknown `actor_kind` (`claude_code`/`codex`/ + `opencode`/`pi` only), missing/blank `scope_id`/`event_id`, unexpected fields, + `flush` with any scope/event/actor field, `abandon` with anything but + `scope_id`. Error type is `anyhow::Result` with the existing + `Invalid