V3: minor findings from the third rewrite code review

[Loufe]

Parent: #33. Minor findings from the third full review of rewrite (at 75c79c3), batched in the pattern of #48 and #74. None are release-blocking. The high/medium items from the same review are #81, #82, #83, #84, #85, plus comments on #72 and #79.

crfty-core

• Applied::reject does not clear deltas: reject() (reducer.rs:318-333) sets the reply and pushes CommandRejected but leaves durable, config, and effects intact; apply folds applied.durable unconditionally (reducer.rs:384-389) and the driver journals it without consulting the reply (driver.rs:470). Every current site rejects before pushing deltas, so the invariant "rejected implies no state change" holds by call-site discipline across a dozen sites. Have reject drain the vectors, or split accepted/rejected into types where a rejected result cannot carry deltas.
• Two validators for output-destination concepts, different rules: Settings::validate (settings.rs:79-95) accepts a default suffix containing separators, reserved characters, or a trailing dot, all of which OutputTarget::validate (job.rs:71-101) rejects; folder emptiness checks also differ. Result: settings that persist fine but make every derived queue add fail per-item. Same concept should have one rule.
• Encoding-time summation implemented three times: SessionAggregates::absorb (state.rs:171-175), encoding_duration (state.rs:678-688), phase_totals (projection.rs:220-237). One helper over PhaseSpans.
• Reuse predicate stated twice: select_analysis (policy.rs:75-97) re-encodes both halves of what AnalysisResult::validate_reusable_for (job.rs:314-336) already checks, then calls it anyway. Filter through validate_reusable_for alone.
• Dead speculative fallback: analyzed_row's .or_else scanning record.analyses for a result with no surviving run (projection.rs:844-851) is unreachable (runs have no removal delta); the comment says "(future legacy adoption)". Delete until the feature exists.
• Three structurally identical active queue-item variants: Reserved/Claimed/Running each carry { claim_id, run_id } (state.rs:52-58), forcing triple-pattern matches at reducer.rs:1680-1695, :1705-1717. A nested Active { claim_id, run_id, phase } makes "same claim, any active phase" one pattern. Minor; current shape is defensible.

crfty-engine

• Windows sharing-violation retry exists only for the journal: journal.rs:318-331 documents and implements persist_with_retry for AV/indexer holds; config.rs:124-126 and vendor install.rs:141-143 do bare persists over the same kind of freshly watched small file. Either the hazard is real (retry belongs in filesystem.rs for all three) or it is not (journal carries dead complexity).
• Two effect-delivery mechanisms, one exists for tests: effects go to the effect_sink (production) or onto the event stream as DriverEvent::Effect (driver.rs:698-715); the only non-test caller of the sink-less path is inside a test module, and the shell ignores the variant (bridge.rs:694). A test-side sink channel deletes the dual path.
• observation_stability misused as equality: media.rs:518 passes before_identity twice, so the three-way classifier can only return Stable or ChangedDuringSampling; it is a dressed-up inequality that launders any change into "during sampling".
• Container decided by extension, not probe data: media.rs:323-330 sets MediaContainer::Matroska iff the path ends in .mkv, ignoring ffprobe's format_name already in hand. The weaker source wins, against the crate rule about human-oriented output vs contracts.
• append_batch returns records production discards: journal.rs:149-187 allocates Vec<JournalEnvelope> per append; the sole production caller maps it away (driver.rs:644). Only the durability tests read it.
• terminate_and_wait is a pure alias of terminate_group_and_wait (process.rs:40-42); two names for one operation.
• ConfigSink::write hides logging reconfiguration: the production impl calls logging::reconfigure inside the trait the driver tests replace (driver.rs:558-566), so tests can never exercise it and persist_settings gives no hint. Move the call visibly next to the write.
• scrub_log_files holds the sink mutex across the whole scrub (logging/mod.rs:360-406): every tracing::* caller blocks in write_line while up to 10 launches x 6 files are rewritten. Only the active file needs the lock.
• Compaction failure drops the original error: journal.rs:225-237 reports only the reopen failure; the persist error that explains the compaction failure is discarded.
• Grouped slop: three hand-rolled hex encoders (media.rs:580-592, logging/privacy.rs:102-106, vendor/download.rs:134-140); shadow-rebind at driver.rs:598-602; the driver calling coordinator::now_millis() (driver.rs:397,651) is a layering inversion worth fixing during the V3: split coordinator.rs and analysis/mod.rs along cohesion seams #73 split; emit_durable accepts and ignores its DurabilityToken (driver.rs:719-727).

crfty-shell

• Clean shutdown takes the "fatal stream sever" path: the forwarder tail (bridge.rs:626-641) claims the engine severs the stream only on overflow, but every normal exit also disconnects it, logging error! and emitting EngineFatal on the happy path. Separately, the engine's overflow contract says recovery is a reconnect from a fresh snapshot (coordinator.rs:300-301), but the shell has no reconnect; the two comments contradict each other. Decide which contract is real.
• Dead match arms from Health doubling as a start-error type: try_start can only fail Unavailable or SecondInstance, yet Bridge::start and commands() (bridge.rs:291-299, :591-607) carry arms for Degraded/Fatal/Ok that cannot execute and mislead about where command gating happens. A narrow start-error enum makes the impossible states unrepresentable.
• Stringly-typed error codes cross IPC: CommandError { code: String } (bridge.rs:107-124) assembles a closed set of nine literals by hand in a codebase that generates every other cross-boundary type via specta. A specta enum exports the set into bindings.
• Mirror summary structs duplicated field-for-field: ImportSummary/ScrubSummary (bridge.rs:84-97) twin engine types only to add Serialize/specta derives; deriving on the engine types (specta is not Tauri) deletes both mirrors. (ReleaseSummary legitimately differs: it strips html_url.)
• Small ceremony: three identical poison-recovery lock helpers (bridge.rs:725-744) replaceable by one generic; infallible subscribe returning Result; Bridge::check_for_update and release_url_slot are free functions wearing method clothing; pick_paths writes the same map-collect five times (commands.rs:52-86).
• map_reply catch-all duplicates the engine's identical defensive arm (bridge.rs:753-762 vs coordinator.rs:503-507): both must hand-write arms for Reply variants impossible on their channel because core's Reply is one wide enum for user and worker surfaces. Root cause in core; noted here because the second copy is the tell.

[Loufe]

Two cross-references after re-reading the open tracker:

• The shell clean-shutdown item has two halves with different owners. The error-level log plus EngineFatal emit on every normal exit is a today-bug, fixable now (distinguish deliberate shutdown from a sever before logging). The "unimplemented reconnect contract" half is resolved by design in V3 post-release: replace durable deltas and the TS fold with snapshot push #78: snapshot push makes every emit a full resync, so the sever-and-reconnect protocol disappears entirely. Fix the log noise now; leave the contract question to V3 post-release: replace durable deltas and the TS fold with snapshot push #78.
• The stringly CommandError.code item should land as part of V3: UI-accessible error/notice surface (rejected commands, import skips, background errors) #49's notice-surface design rather than standalone: V3: UI-accessible error/notice surface (rejected commands, import skips, background errors) #49 introduces the structured notice taxonomy (source, severity, message), and the command-error discriminant enum is the same design decision made once.

[Loufe]

The now_millis layering inversion under Grouped slop is done: 83ec083 moved it to a crate-level clock module as part of the #73 split, so driver.rs no longer calls into coordinator. Everything else in this issue is still open.

[Loufe]

#79 absorbs two findings here:

• Delete the dead “future legacy adoption” analysis fallback; no legacy adoption feature will exist.
• ImportSummary changes to native History inserted/duplicate/conflict/rejected counts. The shell mirror-struct observation remains valid, but derive/export the new engine summary directly rather than preserving the old adopted/dropped shape.

Also, nullable carried verdict summaries and their projection fallback chains are deleted under #79, not refactored. The remaining review items are unaffected.