refactor(cli): drop the runner's dead ephemeral store #427

2026-08-15T01:34:05Z

KofTwentyTwo commented

2026-08-15 01:34:05 +00:00

(Migrated from github.com)

Partial delivery. Both issues were verified item-by-item against current dev
first; this PR lands the one item that was both still reproducing and fully
finishable. Everything else is reported below with a verdict so the remaining
work is a known quantity rather than a re-audit.

#259 — CLI polish

1. Dead ephemeral DB in HeadlessRunner — FIXED.
runner.rs still built a DatabaseEngine, an EventBus, and a TempDir that
no command read; the only things touching them were two #[allow(dead_code)]
accessors. Removed the fields, the accessors, and RunnerError, which became
unreachable once construction could no longer fail.

Two consequences worth calling out for review:

With no store to open, construction is neither fallible nor async. The
constructors lose Result + async, and ephemeral* no longer described
them, so they are now connect / connect_with / connect_with_token_override.
All 39 call sites updated.
Doc comments across runner.rs contrasted the daemon's persistent store
against "this runner's own ephemeral local db" — a field that no longer
exists. Rewritten to say the CLI keeps no store at all.
The unit test runner_error_display was deleted, not weakened: its entire
subject (RunnerError) is gone. No other assertion was touched.

2. cal show / contact show / system watch — NOT DONE (still valid).
Confirmed still missing, and confirmed these are exactly the three RPCs with no
CLI caller: Calendar/GetEvent, Contacts/GetContact, System/Subscribe
(the subscribe/get_event/get_contact hits in runner.rs are test-stub
server impls, not client calls). Every other RPC in every service has a caller.
Left for a follow-up rather than half-landed.

3. update check / update apply non-gRPC exception — PARTIALLY ADDRESSED.
Confirmed still client-local: runner.rs calls nuncio_core::UpdateEngine
straight against api.github.com, with no daemon RPC in the contract and no
comment anywhere explaining why. The HeadlessRunner doc comment added here now
names it as the single sanctioned exception and says why (the daemon it would
have to ask may be the very binary being replaced). A dedicated rationale
comment on UpdateEngine itself is still worth adding.

Also surfaced while verifying: nunciod duplicates the same client-side GitHub
logic for its opt-in auto-update path, so "move it behind an RPC" is a larger
change than the issue implies — worth a decision before anyone attempts it.

#260 — Audit minor cleanups

None of these are addressed in this PR. Verified verdicts:

Dead nuncio_core::model::Contact — STILL VALID. Zero consumers; only
reference is its own re-export in lib.rs. Everything real uses
nuncio_contacts::Contact. Safe delete.
EventBus::process_command ReloadFilters no-op — STALE, do not do.
There is no ReloadFilters variant anywhere in the workspace. CoreCommand
has five variants, every arm does real work, and the match has no catch-all.
Filter reload is now a gRPC concern (FiltersGrpcService::reload_engine_from_store).
The issue's lib.rs:266 no longer points at anything.
compute_log_hash empty string on HMAC init failure — STILL VALID.
db.rs returns String::new() on the error path. Low severity (HMAC accepts
any key length, so it is unreachable today) but the shape is wrong: the same
degraded "" feeds both the chain write and the chain verify, so a
hypothetical failure would make the audit chain verify as intact.
uuid_simple / uuid_or_simple_hash — HALF ALREADY-FIXED.
parser.rs::uuid_or_simple_hash is gone, replaced by generate_rule_id using
OsRng — already fixed on dev. outbox.rs::uuid_simple still reproduces:
it is a 64-bit sha256 of a nanosecond timestamp used as the outbox row id.
The fix should mirror generate_rule_id rather than add a uuid dependency
(uuid is not currently a workspace dep; aes-gcm's OsRng already is).
vault.rs::get_or_create_key_bytes race — STILL VALID. Plain
check-then-set with no lock. Backs the storage, WORM, and ledger keys plus the
gRPC bearer token, so a lost race means unreadable ciphertext or an
unverifiable audit chain. Worth noting there is no CAS primitive in the OS
keyring, so a fix can narrow the window (in-process serialisation +
read-back-after-write) but cannot fully close it cross-process; that limit
should be documented honestly rather than papered over.
Stale JSON-RPC IPC comments — STILL VALID. grpc.rs and the Filters
service in nuncio.proto both still claim the hand-rolled filter.* JSON-RPC
IPC methods and main.rs's CustomRpcHandler "remain in place (both
transports coexist)". CustomRpcHandler does not exist; main.rs itself says
gRPC is "the daemon's ONLY client-facing transport". Also
orchestrator.rs ("Stage 5 IPC event broadcast") and the
DaemonTelemetry::active_ipc_clients field. The JMAP hits for "JSON-RPC" are
legitimate and must be left alone.
sync.rs docs understate incremental checkpointing — STILL VALID. The
module doc describes only full fetch-and-persist, but fetch_and_persist does
real per-folder checkpointing via get/save_folder_sync_state plus absence
reconciliation, proven by an existing test. Two test comments actively
contradict the code ("Sync is not yet incremental").

Gate

cargo fmt --all -- --check, cargo check-all, and cargo test-all all exit 0.
Then RUST_TEST_THREADS=2 cargo test-all three times: 0 / 0 / 0 FAILED lines.

Closes

Nothing. #259 is partially done and #260 is untouched, so both stay open.

Partial delivery. Both issues were verified item-by-item against current `dev` first; this PR lands the one item that was both still reproducing and fully finishable. Everything else is reported below with a verdict so the remaining work is a known quantity rather than a re-audit. ## #259 — CLI polish **1. Dead ephemeral DB in `HeadlessRunner` — FIXED.** `runner.rs` still built a `DatabaseEngine`, an `EventBus`, and a `TempDir` that no command read; the only things touching them were two `#[allow(dead_code)]` accessors. Removed the fields, the accessors, and `RunnerError`, which became unreachable once construction could no longer fail. Two consequences worth calling out for review: - With no store to open, construction is neither fallible nor `async`. The constructors lose `Result` + `async`, and `ephemeral*` no longer described them, so they are now `connect` / `connect_with` / `connect_with_token_override`. All 39 call sites updated. - Doc comments across `runner.rs` contrasted the daemon's persistent store against "this runner's own ephemeral local `db`" — a field that no longer exists. Rewritten to say the CLI keeps no store at all. - The unit test `runner_error_display` was deleted, not weakened: its entire subject (`RunnerError`) is gone. No other assertion was touched. **2. `cal show` / `contact show` / `system watch` — NOT DONE (still valid).** Confirmed still missing, and confirmed these are exactly the three RPCs with no CLI caller: `Calendar/GetEvent`, `Contacts/GetContact`, `System/Subscribe` (the `subscribe`/`get_event`/`get_contact` hits in `runner.rs` are test-stub server impls, not client calls). Every other RPC in every service has a caller. Left for a follow-up rather than half-landed. Note for whoever picks this up: the issue predates the mutation verbs, but those are fine — `mail move|delete|flag|unflag|conflicts|resolve` all exist and all have callers, so nothing there is missing. **3. `update check` / `update apply` non-gRPC exception — PARTIALLY ADDRESSED.** Confirmed still client-local: `runner.rs` calls `nuncio_core::UpdateEngine` straight against `api.github.com`, with no daemon RPC in the contract and no comment anywhere explaining why. The `HeadlessRunner` doc comment added here now names it as the single sanctioned exception and says why (the daemon it would have to ask may be the very binary being replaced). A dedicated rationale comment on `UpdateEngine` itself is still worth adding. Also surfaced while verifying: `nunciod` duplicates the same client-side GitHub logic for its opt-in auto-update path, so "move it behind an RPC" is a larger change than the issue implies — worth a decision before anyone attempts it. ## #260 — Audit minor cleanups None of these are addressed in this PR. Verified verdicts: - **Dead `nuncio_core::model::Contact` — STILL VALID.** Zero consumers; only reference is its own re-export in `lib.rs`. Everything real uses `nuncio_contacts::Contact`. Safe delete. - **`EventBus::process_command` `ReloadFilters` no-op — STALE, do not do.** There is no `ReloadFilters` variant anywhere in the workspace. `CoreCommand` has five variants, every arm does real work, and the match has no catch-all. Filter reload is now a gRPC concern (`FiltersGrpcService::reload_engine_from_store`). The issue's `lib.rs:266` no longer points at anything. - **`compute_log_hash` empty string on HMAC init failure — STILL VALID.** `db.rs` returns `String::new()` on the error path. Low severity (HMAC accepts any key length, so it is unreachable today) but the shape is wrong: the same degraded `""` feeds both the chain write and the chain *verify*, so a hypothetical failure would make the audit chain verify as intact. - **`uuid_simple` / `uuid_or_simple_hash` — HALF ALREADY-FIXED.** `parser.rs::uuid_or_simple_hash` is gone, replaced by `generate_rule_id` using `OsRng` — already fixed on `dev`. `outbox.rs::uuid_simple` still reproduces: it is a 64-bit sha256 of a nanosecond timestamp used as the outbox row id. The fix should mirror `generate_rule_id` rather than add a `uuid` dependency (`uuid` is not currently a workspace dep; `aes-gcm`'s `OsRng` already is). - **`vault.rs::get_or_create_key_bytes` race — STILL VALID.** Plain check-then-set with no lock. Backs the storage, WORM, and ledger keys plus the gRPC bearer token, so a lost race means unreadable ciphertext or an unverifiable audit chain. Worth noting there is no CAS primitive in the OS keyring, so a fix can narrow the window (in-process serialisation + read-back-after-write) but cannot fully close it cross-process; that limit should be documented honestly rather than papered over. - **Stale JSON-RPC IPC comments — STILL VALID.** `grpc.rs` and the `Filters` service in `nuncio.proto` both still claim the hand-rolled `filter.*` JSON-RPC IPC methods and `main.rs`'s `CustomRpcHandler` "remain in place (both transports coexist)". `CustomRpcHandler` does not exist; `main.rs` itself says gRPC is "the daemon's ONLY client-facing transport". Also `orchestrator.rs` ("Stage 5 IPC event broadcast") and the `DaemonTelemetry::active_ipc_clients` field. The JMAP hits for "JSON-RPC" are legitimate and must be left alone. - **`sync.rs` docs understate incremental checkpointing — STILL VALID.** The module doc describes only full fetch-and-persist, but `fetch_and_persist` does real per-folder checkpointing via `get/save_folder_sync_state` plus absence reconciliation, proven by an existing test. Two test comments actively contradict the code ("Sync is not yet incremental"). ## Gate `cargo fmt --all -- --check`, `cargo check-all`, and `cargo test-all` all exit 0. Then `RUST_TEST_THREADS=2 cargo test-all` three times: 0 / 0 / 0 `FAILED` lines. ## Closes Nothing. #259 is partially done and #260 is untouched, so both stay open.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: KofTwentyTwo/nuncio#427