feat(route): extract route_call send-commit seam for mutation routing (NRN-228)#109
Merged
Conversation
Split ServiceClient::call_tool_structured into a phase-tagged core (call_tool_structured_phased) that reports whether a routed failure struck before or after the tools/call frame crossed to the daemon. Reads discard the phase and fall back to Direct on any failure, exactly as before. Extract the read-routing skeleton into a shared route_tool_call / execute_routed_call core parameterised by a FallbackAfterSend policy, and add route_call, the mutation sibling of route_read: - dry-run (writes nothing) -> Fallback: any failure falls back to Direct. - apply -> Commit: a pre-send failure falls back like a read, but a post-send failure surfaces an explicit uncertainty error (exit 1, the "may be partially mutated" code) instead of risking a double-apply on a Direct re-run. No production command routes through route_call yet (NRN-229+ wires the mutation commands); the seam is pub so tests and the future callers can reach it. A CAS/NRN-151 safe-retry policy slots in as another FallbackAfterSend arm without reshaping the seam.
Drive execute_routed_call against a stub UDS daemon on a temp socket (no process-global env touched) to observe the send-commit policy at the seam: - pre-send failure in Commit mode -> Direct fallback (None) - post-send failure in Commit mode -> Some(Err(..)) whose message names the inspect / --dry-run remedy, mapped to exit 1 by the top level; Some (not None) is the no-fallback proof - the SAME post-send failure in Fallback (dry-run) mode -> silent Direct fallback (None) Plus after_send_for's dry-run/apply mapping and a phase-tag guard so the fallback assertions can't pass on a mis-tagged phase.
…spatch Two review findings on the NRN-228 send-commit seam: - A reconstruct failure after a SUCCESSFUL tool call unconditionally fell back to Direct. The daemon had executed the tool, so under Commit that fallback is a double-apply — the exact hazard the seam exists to prevent. Commit now surfaces the post-send uncertainty error; Fallback (dry-run) keeps the silent Direct fallback with the verbose note. - Both policy decision points now dispatch via an exhaustive match instead of `==`, so a future FallbackAfterSend variant (the NRN-151/CAS retry seam) fails to compile at each decision point rather than silently falling through to an unguarded Direct re-run. Tests: a RespondOk stub mode (valid success envelope) plus a failing reconstruct closure pin both sides — Commit -> Some(Err) naming the inspect/--dry-run remedy; Fallback -> None.
Replace the bare anyhow prose from post_send_uncertainty_error with a typed
service::PostSendUncertainError carrying the machine-branchable kebab code
post-send-uncertain (the .code() convention of standards::apply::ApplyError /
EditError, NRN-220), and add it to ApplyError::from_anyhow's downcast chain so
the structured {code, message} failure envelope recovers the code instead of
laundering it to internal-error when NRN-229 wires the mutation commands.
Prose remedy text unchanged. The type is cross-platform (the downcast chain
references it on every target); only the unix routing seam constructs it.
Review hardening on the NRN-228 seam: - debug_assert in execute_routed_call against the FallBackDirect+Commit combination: under FallBackDirect a clean daemon-side refusal (isError: true) is a post-send Err, which Commit would misreport as a false "may have applied" alarm instead of rendering the refusal envelope. Mutation callers use AcceptWithPayload (NRN-220 refusal envelopes ride structuredContent through reconstruct); route_call's docs now say so. - Comment-level invariants: why the tools/call WRITE itself is tagged post-send (mutation safety must not couple to the daemon's framing invariant); why the dynamic-field-refusal branch may ignore after_send (the gate is pre-execution validation — a refusal proves no mutation); route_call's cfg(unix)-only shape and the required non-unix stub for NRN-229; and why the composed probe+policy path is deliberately untested (the process-global XDG_CACHE_HOME race the in-crate tests avoid by injecting a stub ServiceClient).
…ules The crate-root routing-seam tests duplicated the service test module's bind_trusted stub-socket helper. Hoist one pub(crate) cfg(test) copy out of the inner tests module in service/mod.rs and use it from both. spawn_stub stays in the crate-root tests: the service tests' per-test inline wire choreography varies frame-by-frame (error preambles, stale versions, isError flags), so a shared driver would fight those variations.
dbtlr
added a commit
that referenced
this pull request
Jul 10, 2026
…te-identically (NRN-229 PR A) (#113) ## Summary PR A of NRN-229 (Phase 1.5 write-routing under NRN-223): the first two routed *mutations*. `norn set` and `norn edit` now execute on the warm `norn serve` daemon when one is live — byte-identical output (stdout, stderr, exit code) and identical on-disk vault bytes — falling back to direct execution otherwise. Includes the deferred params-struct refactor of the routing seam and the mutation-routing test harness the remaining cascade commands (PR B: move/delete/rewrite-wikilink) will reuse. ## Design - **`CallSpec` params-struct** (deferred from #109 for its real callers): the seam layers now take `(context, spec, reconstruct, emit)`; the private `RoutedCall` wrapper keeps send-commit policy selection out of caller hands. All `too_many_arguments` allows removed; reads delegate through the same skeleton unchanged. - **Route-before-lock:** mutation arms attempt routing *before* the CLI-side mutation lock — the daemon acquires the same per-vault lock file in-process, so a held client lock would deadlock the daemon's acquire into timeout. The Direct fallback runs today's sweep+lock+execute sequence unchanged. - **Mode mapping:** `--dry-run` / `--format json` without `--yes` / non-TTY implicit preview route as `confirm:false` (Fallback policy); `--yes` routes as `confirm:true` (Commit policy: pre-send failures fall back, post-send failures surface `post-send-uncertain` exit 1 — never a double-apply). The interactive TTY preview→prompt→apply flow deliberately stays Direct (lock continuity across the prompt has no routed equivalent, and no CAS gate exists by design — NRN-224). - **Force-Direct guards:** `--config` / `--no-cache-refresh` force Direct exactly like routed reads (the wire speaks canonical vault roots only). set additionally gates `--field-json`/`--push`/`--pop` (BTreeMap wire params can't preserve argv order — tracked) and `--body-from-stdin`; edit gates nothing — its ops resolve locally and ship as an order-preserved array. - **Zero new rendering or MCP surface:** reconstruct deserializes the wire report into the native `SetReport`/`EditReport` (`Deserialize` added; serialization untouched) and emits through the same renderers Direct uses. ## Test harness `serve_util` gains mutation-grade helpers: fresh seeded vault per case, whole-vault recursive byte-snapshot comparison (routed vault vs direct vault — refusal shapes included, so a leaked partial write fails the suite), env-threaded daemon spawn. Suites `serve_set_routing` (4 tests) and `serve_edit_routing` (5 tests) cover records/json × apply/dry-run/preview/coded-refusal/unknown-doc (+ multi-op batch for edit), assert routing actually happened per shape (`served` counts), pin the no-daemon fallback, pin routed *safety* under lock contention (the exit 1-vs-2 divergence there is real and documented, not asserted away), and pin the force-Direct flags with semantically load-bearing alt configs. ## Adversarial review Independent empirical review (probe authority, live daemons) across eight surfaces: double-apply/policy pairing, route-before-lock ordering, decision-ladder precedence (`--dry-run --yes` combos probed routed vs direct), reconstruct roundtrip fidelity (caught that `warnings` needed `serde(default)` — without it a clean apply would misreport `post-send-uncertain`), gating completeness, seam-refactor drift, suite strength, test honesty. **One confirmed blocker found and fixed:** routed mutations initially bypassed the `routing_forced_direct` guard, letting a live daemon apply different bytes than an explicit `--config` dictated; fixed in `99077d3` with red-proofed regression shapes in both suites, re-probed by the reviewer and CLEARED (final verdict: approve). Known divergences deliberately not papered over: lock-contention exit code (routed 1 vs direct 2 — candidate for PR B's refusal-envelope parity), edit's format-blind preflight refusals (pre-existing, tracked). ## Verification - `cargo check --workspace --locked` / `cargo clippy --workspace --all-targets -- -D warnings` / `cargo fmt --check`: clean; `Cargo.lock` unmodified - `cargo test --workspace`: 2381 passed, 0 failed (69 binaries) — independently re-run and summed; read byte-identity suites pass unmodified
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Extracts
route_call— the mutation sibling ofroute_read— with a send-commit fallback policy (NRN-228, first task of Phase 1.5 write-routing under NRN-223). No production command routes through it yet (NRN-229+ wires the mutation commands); reads are byte-identical.Design
ServiceClient::call_tool_structured→call_tool_structured_phased, returningResult<Value, CallToolError>where every failure is taggedPreSendorPostSendaround a single explicit send boundary: the write of thetools/callframe. Socket trust, connect, hello/ready, the version gate, and MCP initialize are pre-send; the frame write and everything after are post-send. The write itself is deliberately post-send even though today's newline framing means a failed write provably never ran the tool — mutation safety must not couple to the daemon's framing invariant.FallbackAfterSendpolicy, decided at send time.Fallback(reads and--dry-run): any failure falls back to Direct silently, as today.Commit(apply mode): pre-send failures fall back Direct; post-send failures never fall back — they surface a typedPostSendUncertainError(codepost-send-uncertain, wired intoApplyError::from_anyhow) telling the operator the daemon may have applied the change, exit 1, distinct from clean pre-flight refusals (exit 2). Both policy decision points dispatch via exhaustivematch, so the future CAS retry variant (NRN-151) must be handled explicitly at each seam point.route_readnow delegates through the sameroute_tool_call/execute_routed_callskeleton (probe, canonical root, version gate, operator-note draining, reconstruct→emit split) withFallbackpolicy — behavior unchanged.Adversarial review
Eight-angle review + independent verification ran pre-PR. Confirmed and fixed: a reconstruct-failure-after-successful-call branch that fell back to Direct under
Commit(a double-apply for a routed mutation — now surfaces the uncertainty error, pinned by two tests); non-exhaustive policy dispatch; uncoded uncertainty error; adebug_assert+ docs guarding theFallBackDirect+Commitmis-wiring. Recorded as comments: why the frame write is post-send, why the dynamic-field-refusal branch may ignore the policy (gate refusals are pre-execution), and the cfg(unix)-only shape NRN-229 callers must stub. Deferred with reasons: params-struct refactor (NRN-229's callers dictate the shape), PreSend reclassification of write failures (refuted — conservative tag is correct).Verification
cargo check --workspace --locked/cargo clippy --workspace --all-targets -- -D warnings/cargo fmt --check: clean;Cargo.lockunmodifiedcargo test --workspace: 2339 passed, 0 failed (67 binaries) — read byte-identity suites (serve_count_routing,serve_find_get_routing) pass unmodified