fix(route): forward daemon-side operator notes to the routed CLI (NRN-215)#105
Merged
Conversation
Routed reads (count/find/get) lost the daemon's lock-contention note: when the warm `norn serve` refresh times out on the write lock it proceeds against the current cache state and produces the note `vault: another cache operation is in progress; using current cache state` — but on the routed path that note was written to the daemon's own stderr and never reached the caller, so the operator lost the possibly-stale-read signal exactly when routing. Carry operator notes as a general envelope facility. A per-request note buffer on `VaultContext` is cleared at the `begin_request` seam and drained by the shared `run_wrapped` funnel — both under the process-wide `call_lock`, so a note is bound to the single request that produced it and never leaks between concurrent connections' serialized requests. `query_cache_warm` records the contention note there instead of printing it to the daemon's stderr. The new `Noted<R>` wrapper (produced once, in `run_wrapped`, over every tool's own result) injects the notes as an additive `operator_notes` string array sibling in `structuredContent`; the routed CLI reads that array in `route_read` and re-emits each note on its own stderr, byte-identical to the direct path (both reference one shared `LOCK_CONTENTION_NOTE` constant). The field is additive: a note-free run emits no key and keeps routed stdout/stderr byte-for-byte identical to direct; a missing key reconstructs as no-notes. The routed request connection is version-gated to the client's exact build, so "absent" always means "this run had no notes," never a dropped signal. `operator_notes` is a cross-cutting envelope sibling, not part of any tool's advertised `outputSchema`. Every read tool now publishes its `outputSchema` explicitly (the `Noted` wrapper defeats rmcp's `Json`-only auto-derive), reusing `output_schema_for`. Tests: a genuine-contention proof holds the vault's cache write lock while a warm read runs, so `index_incremental` times out through the real capture point and the note serializes as `operator_notes`; plus envelope inject/read-back unit tests and a count-route reconstruct-tolerance case.
Five review fixes on the note-forwarding envelope: - Drain notes on EVERY run_wrapped arm, not just success. The in-band failure shape (MutationResult isError, NRN-219/220 refusals, vault.get not-found) flows through the Ok arm and carries notes in its preserved structuredContent like a success (new injects_notes_into_is_error_envelope test). The bare-Err arm crosses as a JSON-RPC error with no structuredContent to ride, so it drains the buffer — the note is not lost (the capture point logs it to the daemon's stderr, and the routed client falls back to Direct, re-producing it canonically) and can never leak into the next request's envelope (regression-tested). - Both surfaces at the capture point: query_cache_warm keeps the eprintln to the daemon's own stderr (its operational log, alongside the served markers) AND buffers the note for the envelope. - Move the routed CLI's note re-emit past the reconstruct commit point, so a reconstruct failure falling back to Direct can never double-print the note. - Make the cache write-lock timeout injectable via the test-only NORN_CACHE_LOCK_TIMEOUT_MS env (mirroring the service handshake-timeout idiom); production paths keep the exact 5s default, pinned by a unit test. The genuine-contention unit test now runs at 150ms instead of stalling the suite 5s. - Add operator_notes-sibling tolerance tests for the find and get reconstructors, mirroring count's. New end-to-end proof (tests/serve_note_forwarding.rs): a routed count under GENUINE daemon-side flock contention produces the same (stdout, stderr, exit) triple as a direct contended run — forwarded note included, byte-identical — with the served-marker count proving the read was daemon-served and the daemon's stderr keeping the note as its log line.
- Gate the NORN_CACHE_LOCK_TIMEOUT_MS read behind #[cfg(debug_assertions)]: release builds compile the env read out entirely, so an inherited value (dev shell, baked plist env) can never shrink the production 5s write-lock budget. Debug builds — what `cargo test` builds and what its integration tests spawn via norn_bin() (the sibling target/<profile>/norn binary) — honor the override. The 5s default stays pinned by test; the e2e serve_note_forwarding test is cfg-gated to the debug profile so a release test run skips it instead of stalling past the routed budget. - Single cross-request isolation mechanism: remove the redundant Ok(Err)- and join-arm note drains from run_wrapped — begin_request's per-request clear (under call_lock) is the one invariant, and the capture point's daemon-stderr line already preserves the signal for envelope-less failures. The regression test is renamed to pin what it actually pins (begin_request_clears_notes_left_by_a_failed_request). - Drop the in-process env mutation (set_var races parallel tests reading the environment): the unit tests now exercise the funnel via the push_operator_note seam directly (run_wrapped_forwards_request_notes_in_ the_envelope), and the genuine flock-contention path — real daemon, real capture point, byte-identical CLI stderr — stays covered end-to-end by serve_note_forwarding, where each child process owns its environment.
Three review rounds (engine=code-review, agents pinned to Opus): 9 findings — 8 fixed (all-arms note handling collapsed to the single begin_request clear invariant, both-surface emission, post-commit re-emit, debug-gated timeout override, no env mutation in-process, reconstructor tolerance symmetry), 1 dismissed (release-profile test runs are not a verification gate; the debug gate is self-consistent with the debug-only override and CI runs debug). Adversarial-Review: run engine=code-review tier=high findings=9 fixed=8 dismissed=1 deferred=0
Merged
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
Routed reads no longer lose daemon-side operator notes. When the daemon's warm refresh hits lock contention, the "possibly-stale-read" warning previously landed only on the daemon's stderr — invisible to the CLI caller. It now travels the tool-response envelope and is re-emitted on the routed CLI's stderr, byte-identical to the direct path, while also remaining in the daemon's own log stream for operators tailing it.
Design
Noted<R>wrapper in the sharedrun_wrappedfunnel injects an additiveoperator_notessibling into any tool'sstructuredContent— absent when empty, so note-free envelopes are byte-for-byte unchanged. All 14 tools inherit it.query_cache_warm'sLockTimeoutarm), emitting to both surfaces via one sharedLOCK_CONTENTION_NOTEconstant, so daemon-log and CLI wording cannot drift.begin_requestclears the per-request buffer under the call lock — no scattered drains.NORN_CACHE_LOCK_TIMEOUT_MS) is compiled out of release builds (cfg(debug_assertions)) — production always uses the 5s budget, pinned by test.Verification
check --locked/ 2316 tests, 0 failed / clippy-D warnings/fmt;Cargo.lockuntouched.tests/serve_note_forwarding.rs): a routedcountwhile the daemon's cache write lock is held produces the same (stdout, stderr, exit) triple as a direct contended run — forwarded note included — with served markers proving routing and the daemon log retaining its own note line. Runs in ~1.2s via the debug-only timeout override.Adversarial review
Three rounds, converging 5 → 3 → 1: 9 findings — 8 fixed, 1 dismissed, 0 deferred. Highlights: round 1 caught notes being dropped on error arms and the daemon's own log losing the signal entirely; round 2 caught the timeout override leaking into production builds (now compiled out) and an env-mutation data race in tests; round 3's single PLAUSIBLE (no release-profile test coverage of the contention path) is dismissed on the record — the debug gate is self-consistent with the debug-only override, and this repo's verification gate is the debug workspace suite.
Adversarial-Review: run engine=code-review tier=high findings=9 fixed=8 dismissed=1 deferred=0