# v1.30.1 Audit Findings

Generated: 2026-04-28T18:28:34Z
## Code Quality

#### dropped_this_cycle reset before snapshot publish hides drop signal from `--require-fresh`
- **Difficulty:** medium
- **Location:** `src/cli/watch/events.rs:139-146` (reset) + `src/cli/watch/mod.rs:1303` (publish ordering)
- **Description:** `process_file_changes` clears `state.dropped_this_cycle = 0` at the start (line 145) before reindexing the queued files. The snapshot is then published *later in the same outer iteration* (`publish_watch_snapshot` at `mod.rs:1303`). By the time the snapshot is built, the dropped counter is always 0 — even though events for those dropped files are gone forever (they were never queued; they hit the `pending_files.len() < max_pending_files()` cap and were silently discarded at `events.rs:115`). `WatchSnapshot::compute` uses `dropped_this_cycle > 0` as a Stale signal, but it never observes a non-zero value at publish time. Practical impact: `cqs eval --require-fresh` and `cqs status --watch-fresh --wait` can return `Fresh` immediately after a bulk burst that dropped files at the cap. The dropped files won't be reindexed until the next reconciliation pass (up to 30s later). The whole point of Layer 3 (#1182) is to gate eval on freshness; this defeats it for the high-pressure case the cap exists to handle.
- **Suggested fix:** Either (a) accumulate `dropped_total` cumulatively across cycles and only clear it after a successful reconcile pass that requeues all files, or (b) move the reset to *after* `publish_watch_snapshot` in the outer loop so at least the cycle in which the drop happened reports `Stale`. Option (a) is correct; option (b) is a one-line stop-gap.

#### `delta_saturated` flag is published but ignored by `FreshnessState::compute`
- **Difficulty:** easy
- **Location:** `src/watch_status.rs:199-209`
- **Description:** `WatchSnapshotInput` carries `delta_saturated`, and the snapshot serializes it on the wire — but `compute()` never consults it when picking `FreshnessState`. When the in-flight rebuild's pending delta saturates, the rebuilt HNSW will be discarded on swap (`rebuild.rs:352-366`); operators get a `tracing::warn!` line, but `--require-fresh` callers don't see it because the snapshot still classifies as `Rebuilding` (or, after the doomed rebuild's discard, `Fresh`). The published flag is informational only — there's no path that ties it back to a state-machine consequence.
- **Suggested fix:** Either roll `delta_saturated` into `Stale` in `compute()` (rebuild is doomed, treat as stale until next threshold rebuild lands) or drop the field from the wire shape entirely. Half-wired flags rot.

#### `cqs eval --require-fresh` Timeout error message omits drop/saturation signals
- **Difficulty:** easy
- **Location:** `src/cli/commands/eval/mod.rs:248-255`
- **Description:** The Timeout `bail!` shows `modified_files`, `pending_notes`, `rebuild_in_flight` from the snapshot — but not `dropped_this_cycle` or `delta_saturated`. An operator who hits a timeout will see the queue isn't draining but won't be told *why* (cap-saturation vs. ordinary slow indexing vs. delta-saturated rebuild that's about to be thrown away). Diagnostic gap right where it matters most.
- **Suggested fix:** Include `dropped_this_cycle=` and `delta_saturated=` in the bail message. Adds two fields, costs nothing.

#### `strip_token_param` doesn't case-fold or percent-decode — token leaks into redirect URL
- **Difficulty:** medium
- **Location:** `src/serve/auth.rs:246-260` (and `check_request:305-318`)
- **Description:** Both `strip_token_param` and `check_request` match the literal byte prefix `"token="`. A request with `?Token=<token>` (capital T) or `?%74oken=<token>` (percent-encoded `t`) is *not* recognised by `check_request` (so it 401s — fine) but it's *also* not stripped on the post-auth redirect path. The current tests (`p2_30_strip_token_param_capital_T_token_NOT_stripped_today`, `p2_30_strip_token_param_percent_encoded_key_NOT_stripped_today`) explicitly *pin* this gap with comments calling out "SEC-7 leakage: token survives in redirect URL". The bug is acknowledged in the test corpus but not fixed. Anyone naive enough to type `?Token=…` into the URL bar will leave the token visible there after the auth flow runs (and cookies handle subsequent requests). For an audit-reviewable security surface this is the wrong shape — the gap is filed *as a comment in a test that asserts the wrong behaviour*.
- **Suggested fix:** Lowercase the parameter key once during the URI walk and percent-decode it before the comparison. Then invert the two pinned tests to assert the token is stripped. This is the fix the test comments already say is needed.

#### Three near-identical `ort_err` helpers across embedder / reranker / splade
- **Difficulty:** easy
- **Location:** `src/embedder/provider.rs:14`, `src/reranker.rs:100`, `src/splade/mod.rs:27`
- **Description:** Each module defines `fn ort_err(e: ort::Error) -> XxxError { XxxError::Inference(e.to_string()) }` (with minor signature differences — generic `<T>` vs concrete). The reranker's doc comment even cross-references `crate::embedder::ort_err` for rationale, signalling the duplication is known. P3.1 already deduped `panic_message` in three modules; this is the next instance of the same pattern. With `pub(crate)` visibility on the embedder helper and an `impl From<String>` shim per error enum (or a generic `ort_to_inference<E: From<String>>` helper), the three become one.
- **Suggested fix:** Promote `embedder::ort_err` to a crate-private free function (or move it to a `crate::ort_helpers` module) and have reranker/splade reuse it via a per-enum `From<String>` impl on the `Inference`-shaped variant. Saves ~9 LOC and removes a "see other module for rationale" cross-reference.

#### Auth `--no-auth` warning at boot is silent for localhost binds
- **Difficulty:** easy
- **Location:** `src/cli/commands/serve.rs:27-37`
- **Description:** `cmd_serve` only prints the `WARN: --bind ... with --no-auth` banner when bind is *not* localhost. The localhost+`--no-auth` case is the most common operator footgun (a personal box where another user account or a container peer can reach `127.0.0.1`), and the existing structured warning at `serve/mod.rs:162-165` only fires from inside `run_server` (after the bind happens, mixed with the listening-banner stream). Two near-identical warnings exist for the same scenario; one is silent on the most common case.
- **Suggested fix:** Drop the early CLI-side warning in favour of the unconditional `run_server` warning at `serve/mod.rs:162`, *or* widen the CLI predicate to fire on any `--no-auth` regardless of bind. Don't carry both.
# Audit Findings — Documentation (v1.30.1, 2026-04-28)

Audit mode: ON. Scope: README, CONTRIBUTING, SECURITY, PRIVACY, CHANGELOG, ROADMAP, doc comments. Triage state checked against `docs/audit-triage-v1.30.0.md` (no v1.30.1 triage file yet).

---

## Documentation

#### PRIVACY/SECURITY misstate the embedding-cache primary key (drops `purpose` column)
- **Difficulty:** easy
- **Location:** `PRIVACY.md:16`, plus the v1.30.0 CHANGELOG entry at `CHANGELOG.md:71` and ROADMAP at `ROADMAP.md:131`
- **Description:** PRIVACY.md line 16 says the project cache is `keyed by (content_hash, model_id)`. The actual schema in `src/cache.rs:277` is `PRIMARY KEY (content_hash, model_fingerprint, purpose)` — `purpose` was added in #1128 to discriminate embedding-vs-summary lookups. The doc-claimed two-column key would imply summary-pass embeddings could be served to non-summary callers, which is exactly the bug #1128 fixed. This is a privacy-relevant lie because PRIVACY.md is what users read to understand what gets stored — telling them about a 2-tuple when the system stores a 3-tuple obscures that summary-pass content fingerprints are persisted as a separate row class. SECURITY.md line 47 likewise says "summary is cached by `content_hash`" with no mention of `model_fingerprint` / `purpose`.
- **Suggested fix:** PRIVACY.md:16 → "keyed by `(content_hash, model_fingerprint, purpose)` (#1105, #1128). Skips re-embedding chunks that haven't changed across reindexes / model swaps; the `purpose` discriminator (`embedding` vs `summary`) prevents cross-pass collisions." Apply the same column update to SECURITY.md:47 and to the historical CHANGELOG / ROADMAP entries (or add a note that the schema gained `purpose` post-#1105).
- **Existing issue:** none

#### README canonical command list omits `cqs hook` and `cqs status` (#1182 commands)
- **Difficulty:** easy
- **Location:** `README.md:540-569`
- **Description:** The dashed list of every CLI command at lines 540-569 is the README's own command index — `cqs ping`, `cqs eval`, `cqs model`, `cqs serve`, `cqs refresh`, `cqs doctor`, `cqs completions` are all listed. The two new top-level commands shipped with #1182 — `cqs hook` (install/uninstall/status/fire) and `cqs status --watch-fresh` — are missing from this list, even though they are demonstrated in the Watch Mode section earlier (lines 207-220). Per the team memory rule, "new CLI commands need full ecosystem updates," and the canonical command list is the agent-discoverable index. An agent grepping the README for "what commands exist" misses the two new ones.
- **Suggested fix:** Insert two rows in the list around line 568:
  ```
  - `cqs hook install/uninstall/status/fire` - manage `.git/hooks/post-{checkout,merge,rewrite}` for watch-mode reconciliation. Idempotent; respects third-party hooks via marker check (#1182)
  - `cqs status --watch-fresh [--wait [--wait-secs N]]` - report watch-loop freshness; `--wait` blocks until `state == fresh` (default 30 s, capped at 600 s) (#1182)
  ```
- **Existing issue:** none

#### CONTRIBUTING Architecture Overview is stale — missing eval/, watch_status.rs, daemon_translate.rs, fs.rs, limits.rs, aux_model.rs, plus 6+ command files
- **Difficulty:** easy
- **Location:** `CONTRIBUTING.md:149-340`
- **Description:** The Architecture Overview tree lists every top-level src file but is missing several that exist on main:
  - `src/eval/` (mod.rs + schema.rs) — eval harness module, not listed
  - `src/watch_status.rs` — `WatchSnapshot`, `SharedWatchSnapshot`, `FreshnessState` (#1182 Layer 3)
  - `src/daemon_translate.rs` — `daemon_ping`, `daemon_status`, `daemon_reconcile`, `wait_for_fresh`, `FreshnessWait` (#1182)
  - `src/fs.rs` (atomic_replace helper, #953)
  - `src/limits.rs`
  - `src/aux_model.rs`
  - In `src/cli/commands/infra/`: `hook.rs`, `model.rs`, `ping.rs`, `slot.rs`, `status.rs` are missing from the list at line 166 (which stops at `audit_mode, cache_cmd, convert, doctor, init, project, reference, telemetry_cmd`)
  - In `src/cli/commands/index/`: `umap.rs` missing
  - `src/cli/commands/serve.rs` (top-level commands/, not listed)
  - In `src/cli/watch/`: `reconcile.rs` (#1182 Layer 2) missing from the list at line 187-197
  
  Per the rule at CONTRIBUTING.md top-of-section ("update it when adding/moving/renaming source files"), this section is the contract between the codebase shape and contributor onboarding. Drift here means new contributors and dispatched agents misroute their first edit.
- **Suggested fix:** Add the missing entries with one-line descriptions matching adjacent rows. Group `eval/`, `watch_status.rs`, `daemon_translate.rs`, `fs.rs`, `limits.rs`, `aux_model.rs` in the top-level src/ block. Append the missing command files to their category lines. Add `reconcile.rs - Layer 2 periodic full-tree reconciliation (#1182)` to the watch/ block.
- **Existing issue:** none

#### ROADMAP claims #1182 acceptance test still pending, but #1196 already merged
- **Difficulty:** easy
- **Location:** `ROADMAP.md:16` and `ROADMAP.md:142`
- **Description:** Both lines say:
  > "remaining acceptance item is the WSL-specific integration test"
  
  But `git log` shows `a240ad08 test(watch): bulk-delta reconcile pass for #1182 acceptance — 47-file scenario (#1196)` already merged before v1.30.1 cut. The 47-file bulk-delta reconcile test is the WSL acceptance scenario. The ROADMAP is now lying about open work — agents reading the roadmap will think there's still a test gap to fill.
- **Suggested fix:** Update line 16 and line 142 to:
  > "**Status (2026-04-28):** Layers 1-4 shipped (#1189 freshness API, #1191 periodic reconciliation, #1193 git hooks, #1194 eval `--require-fresh`); 47-file bulk-delta acceptance test landed in #1196. #1182 fully closed."
- **Existing issue:** none

#### PRIVACY/SECURITY claim platform-native cache dirs but document only the Linux path for query_log/query_cache
- **Difficulty:** easy
- **Location:** `PRIVACY.md:21-22`, `SECURITY.md:111-112,135-136`
- **Description:** PRIVACY.md and SECURITY.md document the recent query embedding cache and query log as `~/.cache/cqs/query_cache.db` / `~/.cache/cqs/query_log.jsonl`. The implementation in `src/cache.rs:180-188` and `src/cli/batch/commands.rs:457-461` uses `dirs::cache_dir()` first and only falls back to `~/.cache`. On macOS this resolves to `~/Library/Caches/cqs/...`; on Windows, `%LOCALAPPDATA%\cqs\...`. The deletion instruction in PRIVACY.md:59 (`rm -rf ~/.cache/cqs/`) does not actually delete the cache on macOS/Windows.
- **Suggested fix:** Match the cross-platform note already present in `src/cache.rs:171-174`. PRIVACY.md should list:
  > - Linux: `$XDG_CACHE_HOME/cqs/...` or `~/.cache/cqs/...`
  > - macOS: `~/Library/Caches/cqs/...`
  > - Windows: `%LOCALAPPDATA%\cqs\...`
  
  Update the `rm -rf` block in PRIVACY.md:59 to instruct macOS users to also remove `~/Library/Caches/cqs/` and Windows users `%LOCALAPPDATA%\cqs\`. Apply the same to SECURITY.md filesystem tables at lines 111-112 and 135-136.
- **Existing issue:** none

#### README "Watch Mode" section omits the default `--wait-secs` budget (30 s)
- **Difficulty:** easy
- **Location:** `README.md:219-220`
- **Description:** README:219 says `cqs status --watch-fresh --wait` "block until state == fresh (250 ms poll, capped at 600 s)" and the next line shows `--wait-secs 30  # custom budget`. The clap default at `src/cli/definitions.rs:792` is `default_value_t = 30`, i.e. 30 seconds is also the default — not a "custom budget" example. Without naming the default, an agent reading the README assumes the default is the 600 s cap (the only number stated) and writes scripts that wait up to 10 minutes silently. The CHANGELOG entry at `CHANGELOG.md:18` correctly says "30 s default budget, capped at 600 s."
- **Suggested fix:** Replace lines 219-220 with:
  ```
  cqs status --watch-fresh --wait                     # block until fresh (default 30 s budget, 250 ms poll)
  cqs status --watch-fresh --wait --wait-secs 600     # extend up to the 600 s cap
  ```
- **Existing issue:** none

#### SECURITY.md auth claim names neither `cqs_token_<port>` cookie nor the AuthMode enum from #1135 / #1136
- **Difficulty:** easy
- **Location:** `SECURITY.md:17`
- **Description:** SECURITY.md:17 describes the auth surface as "cookie handoff is `HttpOnly; SameSite=Strict`; compare is constant-time. `--no-auth` opts out". This is accurate as far as it goes, but it predates #1135 (cookie renamed to `cqs_token_<port>`) and #1136 (`AuthMode::Disabled(NoAuthAcknowledgement)`). The threat-model row should reflect both hardenings:
  - Cookie name now port-scoped per RFC 6265 — without this clarification, an operator running multiple `cqs serve` instances on the same host doesn't learn that cqs has already solved the cookie-collision problem (and conversely doesn't realize that pre-v1.30.2 instances could leak tokens cross-port).
  - `--no-auth` now requires constructing a `NoAuthAcknowledgement` proof token; an internal caller cannot accidentally ship a fully-open server. This is a load-bearing claim for anyone reviewing the security surface.
  
  Per the team's "Docs Lying Is P1" rule, SECURITY.md is the canonical surface for auth claims; omitting protections that were just landed makes the doc weaker than the code.
- **Suggested fix:** Replace SECURITY.md:17 with:
  > Per-launch 256-bit auth token gates every request (#1118 / SEC-7). Three credential surfaces: `Authorization: Bearer`, `cqs_token_<port>` cookie (port-scoped per RFC 6265, #1135), `?token=` query param. Cookie handoff is `HttpOnly; SameSite=Strict; Path=/`; compare is constant-time. Disabling auth requires `--no-auth` plus an internal `NoAuthAcknowledgement` proof token (#1136), so no internal caller can ship a fully-open server by accident; the disabled branch logs a structured `tracing::error!` regardless of `quiet`.
- **Existing issue:** none

#### CONTRIBUTING test count and file count out of date (uses pre-#1105 numbers)
- **Difficulty:** easy
- **Location:** `CONTRIBUTING.md` (architecture overview), and CLAUDE.md / MEMORY.md cross-check
- **Description:** Per `MEMORY.md` and ROADMAP, post-v1.30.1 tests are ~3001 pass + 51 ignored. CONTRIBUTING.md doesn't cite a current test count, but the architecture overview's parser/store/embedder claims still reference 54 languages + 29 chunk types — those line up with `src/language/mod.rs` (count of `define_chunk_types!` entries = 29 by manual count: Function, Method, Class, Struct, Enum, Trait, Interface, Constant, Section, Property, Delegate, Event, Module, Macro, Object, TypeAlias, Extension, Constructor, Impl, ConfigKey, Test, Variable, Endpoint, Service, StoredProc, Extern, Namespace, Middleware, Modifier). README:655 says "10 named + 19 other = 29" implicit, also fine. So this finding is narrower than initially scoped — the actual drift is that CONTRIBUTING.md:198 says `language/` covers "54 languages + L5X/L5K" but `src/language/queries/` directory contents and `language_names()` registry should be cross-checked. **Skip if not actionable; the bigger issue is the file-tree gaps captured separately.**
- **Suggested fix:** None separate — folded into the Architecture Overview drift finding above.
- **Existing issue:** n/a

#### v1.30.0 CHANGELOG entry's `cqs cache {stats,prune,compact}` list still missing `clear` (cosmetic, historical)
- **Difficulty:** easy
- **Location:** `CHANGELOG.md:71`, `ROADMAP.md:131`
- **Description:** Both the v1.30.0 CHANGELOG row and the ROADMAP "Done" row for #1105 list `cqs cache {stats,prune,compact}` — the same 3-of-4 list that triage P2.10 already flagged for the README and that has since been fixed at README:561 (which now correctly says `cqs cache stats/clear/prune/compact`). The historical CHANGELOG/ROADMAP entries weren't backfilled. Low-impact because they are historical, but they are the rows agents read when verifying "did `clear` exist at v1.30.0 release time?" The answer is yes — see `src/cli/commands/infra/cache_cmd.rs:37,102`.
- **Suggested fix:** In CHANGELOG.md:71 change `{stats,prune,compact}` to `{stats,clear,prune,compact}`; same edit at ROADMAP.md:131. Optionally add a brief one-liner in the v1.30.1 [Unreleased] CHANGELOG noting the doc backfill so the change is auditable.
- **Existing issue:** none (P2.10 was the README fix only)
# Audit Findings — API Design (v1.30.1)

Scope: lib.rs `pub` surface, CLI flag surface, JSON output schemas, and daemon socket protocol — with focus on the just-shipped #1182 (watch_status / freshness) and #1134/#1135/#1136 (serve auth) APIs.

## API Design

#### API-V1.30.1-1: `cqs status --wait` emits a *success* envelope but exits 1 on timeout
- **Difficulty:** easy
- **Location:** `src/cli/commands/infra/status.rs:85-90`
- **Description:** On `FreshnessWait::Timeout(snap)` the handler calls `emit_snapshot(&snap, json)?` and then `std::process::exit(1)`. `emit_snapshot` routes through `crate::cli::json_envelope::emit_json(&snap)` which produces a *success* envelope (`{"data": <snap>, "error": null, "version": 1, "_meta": {...}}`). Process exit code is 1, but the envelope shape signals success. An agent that branches on `error == null` will treat the response as success, while a wrapper script that checks `$?` will treat it as failure. The two signals contradict — same situation that the `_meta` envelope contract was designed to avoid. (`cqs ping` correctly emits an *error* envelope on its failure path via `emit_json_error(IO_ERROR, msg)`; status's timeout path doesn't follow the same convention.)
- **Suggested fix:** On `Timeout`, emit an envelope with `error: { code: "stale_index", message: "..." }` and the snapshot as a structured field on the error (or as a separate `details` field), or at minimum re-route through `emit_json_error` with the snapshot info embedded in `message`. Add a new `error_codes::STALE_INDEX` constant (currently the taxonomy only has `not_found`, `invalid_input`, `parse_error`, `io_error`, `internal`). Whichever route is taken, exit code and envelope error nullity must agree.

#### API-V1.30.1-2: `--watch-fresh --wait-secs 30` (status) vs `--require-fresh-secs 600` (eval) — same operation, 20× different default budget
- **Difficulty:** easy
- **Location:** `src/cli/definitions.rs:792-793` (`wait_secs: default 30`) vs `src/cli/commands/eval/mod.rs:85-86` (`require_fresh_secs: default 600`)
- **Description:** Both `cqs status --watch-fresh --wait` and `cqs eval` route through `cqs::daemon_translate::wait_for_fresh(&cqs_dir, budget)`. They are the same poll loop with the same daemon. Default budgets are 30s and 600s respectively — a 20× spread. Worse, the flag *names* differ for what is the same knob: `--wait-secs` vs `--require-fresh-secs`. An agent reading both man pages cannot reuse muscle memory; an operator who tunes one will not realize the other exists. Internally they call the same helper and resolve the same `min(600)` cap, so the wire-level semantics are identical.
- **Suggested fix:** Pick one canonical name (`--fresh-wait-secs` reads cleanly on both subcommands) and one default. 30s makes sense for status (`--wait` is a debugging poll); 600s makes sense for eval (a 5-minute build). Resolve by either (a) making both subcommands use the same flag name and same default, or (b) documenting the divergence explicitly in both `--help` strings — currently neither `--help` mentions the other surface.

#### API-V1.30.1-3: `cqs eval --no-require-fresh` is the only `--no-X` flag in the entire CLI; everywhere else uses positive flags
- **Difficulty:** easy
- **Location:** `src/cli/commands/eval/mod.rs:79-80`
- **Description:** `cqs eval --no-require-fresh` is a `clap::ArgAction::SetTrue` boolean. A grep across `src/cli/` for `long = "no-` shows this is the only `--no-X`-style flag in the public CLI surface. Every other "default-on" gate either uses (a) a positive flag with a default value (`--limit 20`), (b) an environment-variable override (`CQS_EMBEDDING_DIM`), or (c) a flag that flips off-by-default behavior on (`--rerank`, `--hyde-queries`, `--watch-fresh`). The convention in the rest of cqs is "positive flag turns thing on; absence is default" — not "negative flag turns off-by-default thing off". `cqs hook install --no-overwrite` is the only sibling, and it has a clearly opposite semantic (overwrite is the default *destructive* action that `--no-overwrite` makes safe). For `--no-require-fresh`, the gate could equally be expressed as `--require-fresh` (off-by-default) — which would match the `--watch-fresh` / `--rerank` / `--hyde-queries` pattern — at the cost of changing the default. The current shape papers over a **default-on safety gate**, which is a reasonable design choice, but it leaves the CLI surface inconsistent.
- **Suggested fix:** Document in the eval `--help` that `--no-require-fresh` is the off-switch for a default-on gate (rare in cqs); reference `CQS_EVAL_REQUIRE_FRESH=0` as the env-var equivalent. Alternative (more invasive): rename to `--require-fresh` with `default_value_t = true`, accepting `--require-fresh=false`. The latter is the clap-idiomatic shape but breaks scripts that already pass `--no-require-fresh`. Lowest-cost fix is to leave the flag and add a doc-comment cross-reference so future reviewers don't trip.

#### API-V1.30.1-4: `PingResponse.last_indexed_at` and `WatchSnapshot.last_synced_at` describe the same field with different names
- **Difficulty:** easy
- **Location:** `src/daemon_translate.rs:236` (`PingResponse.last_indexed_at`) vs `src/watch_status.rs:105,189` (`WatchSnapshot.last_synced_at`)
- **Description:** Both fields are `Option<i64>` Unix-seconds timestamps for the same underlying value: the mtime of `index.db` after the most recent write. Confirmed by the doc comments — `PingResponse.last_indexed_at`: "Unix timestamp (UTC seconds) of the last write to `index.db`"; `WatchSnapshot.last_synced_at`: "Unix timestamp (UTC seconds) of the last completed reindex — the mtime of `index.db` after the most recent write." Two names for the same wire concept means: (a) agents parsing both endpoints (`cqs ping --json` and `cqs status --watch-fresh --json`) must learn two field names; (b) a future "is this index stale" helper has to choose one and ignore the other or merge them. The schemas were shipped at different times (PingResponse in v1.27, WatchSnapshot in v1.30 #1182) — this is API drift, not a deliberate distinction.
- **Suggested fix:** Pick one name (suggest `last_indexed_at` since `cqs ping` shipped first and has more users) and rename the other. Renaming `WatchSnapshot.last_synced_at` is cheap because no external users exist yet (the field landed in v1.30 with #1182 and only the internal CLI reads it). If renaming is too disruptive, add a serde alias on one of them so both wire names deserialize.

#### API-V1.30.1-5: `daemon_ping` / `daemon_status` / `daemon_reconcile` return `Result<T, String>` — stringly-typed errors on a public API
- **Difficulty:** medium
- **Location:** `src/daemon_translate.rs:271,422,541` (three sibling functions all returning `Result<T, String>`)
- **Description:** All three daemon-client helpers in the public `cqs::daemon_translate` module return `Result<T, String>`. The error type is the *concatenated message string* assembled with `format!("connect to {} failed: {e}", ...)`. Callers cannot programmatically distinguish the failure modes that matter — "no socket / no daemon" vs "connect timed out" vs "non-ok status returned" vs "deserialize failed". The CLI surface compensates by string-matching ("no daemon running") or by lumping every failure into `IO_ERROR`. With a typed error enum, callers like `cqs eval --require-fresh` could surface a different message for "you forgot to start the daemon" vs "the daemon is responding but we can't parse its output" vs "the daemon returned an explicit error status" — today they all collapse to a single anyhow!() string. This is the same pattern called out in the CRITICAL LESSON in MEMORY.md ("error handling for new modules") — three new public helpers shipped with stringly-typed errors.
- **Suggested fix:** Define a `pub enum DaemonClientError { NoSocket, Connect(io::Error), Timeout, NonOkStatus { code: String, message: String }, MalformedEnvelope(String), Deserialize(serde_json::Error) }` with `#[derive(thiserror::Error)]` and have all three functions return `Result<T, DaemonClientError>`. Callers can then match on the variant they care about (the eval gate's "no daemon" message is the obvious case). Keep a `Display` impl so existing `format!("{e}")` paths still produce the same human-readable strings.

#### API-V1.30.1-6: `DaemonReconcileResponse.queued: bool` field is documented "Always `true`" — useless field on a public response
- **Difficulty:** easy
- **Location:** `src/daemon_translate.rs:517-519`
- **Description:** `pub struct DaemonReconcileResponse { pub queued: bool, ... }`. The field's only doc comment is `/// Always `true` — the daemon flipped the signal.` If a field is invariantly true on success, it carries zero information; the success itself is signaled by `Ok(...)`. If it could be false in some edge case (e.g. daemon present but reconcile globally disabled), the field is meaningful but the doc lies. Either way the contract is broken: agents writing JSON consumers have to decide whether `queued: false` is reachable, and the doc says "no" but the *type* says "yes".
- **Suggested fix:** Either (a) remove the `queued` field entirely — `Ok(DaemonReconcileResponse { ... })` already conveys "queued"; or (b) repurpose it to carry real info ("`true` if the request flipped the signal from `false`; `false` if it was already pending and got coalesced" — which is what `was_pending` already says inverted). Option (a) is simpler and matches the principle of "no debt in foundation layers" (MEMORY.md). Drop the field, bump the JSON_OUTPUT_VERSION constant.

#### API-V1.30.1-7: `WatchSnapshotInput::_marker: PhantomData<&'a ()>` is a leaked private invariant on a public struct
- **Difficulty:** easy
- **Location:** `src/watch_status.rs:181-193`
- **Description:** `WatchSnapshotInput<'a>` is `pub` with all `pub` fields including `pub _marker: std::marker::PhantomData<&'a ()>`. The `_marker` doc says "Phantom keeps the API future-proof if we add borrow-only fields (e.g. last-error string). No-op today." Because it's `pub`, every external (binary-crate) construction site has to write `_marker: std::marker::PhantomData,` — the binary already does this in `src/cli/watch/mod.rs:174`. Three concerns:
  1. The lifetime `'a` is unused (no `&'a` field). Adding one later would force every existing call site to pin a concrete lifetime, which is a breaking change anyway — the phantom lifetime defers no work.
  2. The `pub _marker` field is verbosity tax with zero information value. Making it private with a constructor (`WatchSnapshotInput::new(...)`) hides the marker.
  3. Today the only caller is `src/cli/watch/mod.rs` (the binary crate). The pub-ness exists because cqs is a binary and the binary needs to construct one — this means the struct should probably be `pub(crate)` or replaced with a builder.
- **Suggested fix:** Drop the lifetime parameter and the phantom marker entirely; restore them when an actual borrowed field is added. Move construction behind a `WatchSnapshotInput::new(pending_files, pending_notes, ...)` so the field set is encapsulated. Alternative: convert to `WatchSnapshot::compute()` taking explicit named arguments (the current input struct has no behavior — it's just an arg pack).

#### API-V1.30.1-8: `cqs status` with no flag exits 1 ("must pass --watch-fresh") — the only mode is gated on a flag the user can't avoid
- **Difficulty:** easy
- **Location:** `src/cli/commands/infra/status.rs:41-54`, definitions.rs:779-794
- **Description:** `cqs status` (no flag) prints `cqs status: pass --watch-fresh to query daemon freshness` and exits 1. The doc comment explains: "kept explicit (not the implied default) so future siblings (`--last-error`, `--clients`) can join cleanly." But today (a) `--watch-fresh` is the *only* mode, (b) future siblings don't exist yet, (c) the current behavior is non-obvious to anyone running `cqs status --help` and finding three flags but not knowing which is the default. The forward-compat design choice imposes friction on every user today for a hypothetical future. This is the design opposite of `cqs ping`, which has no required mode flag.
- **Suggested fix:** Either (a) make `--watch-fresh` the default action when no other mode flag is set — add it as `default_value_t = true` so explicit `--watch-fresh` still parses but absence works too; or (b) keep the gate and surface a stderr hint (`"hint: try --watch-fresh"`) on the no-flag path. Option (a) is friendlier for the today-state and is reversible when future modes land — at that point a multi-mode resolution can pick a default. The "future-proof" justification is precisely the kind of premature-design that causes friction.

#### API-V1.30.1-9: `FreshnessState` has `as_str()` but no `Display` — `format!("{state}")` doesn't compile
- **Difficulty:** easy
- **Location:** `src/watch_status.rs:51-60`
- **Description:** `FreshnessState` provides `pub fn as_str(&self) -> &'static str` but no `Display` impl. The CLI consumer in `src/cli/commands/infra/status.rs:136` writes `println!("state: {}", snap.state.as_str())` because `{}` won't compile against the bare enum. This is a small papercut — Rust convention is that public string-returning enums implement `Display`, and consumers pay a tax of `.as_str()` everywhere they format the value. The convention also means a `tracing::info!(state = %snap.state)` would not work today; you'd have to write `tracing::info!(state = %snap.state.as_str())` or `state = ?snap.state` (which gives `Fresh` not `"fresh"`).
- **Suggested fix:** Add `impl std::fmt::Display for FreshnessState` that delegates to `self.as_str()`. Two-line addition; pure ergonomics win.

#### API-V1.30.1-10: `WatchSnapshot.idle_secs: u64` is computed via `last_event.elapsed().as_secs()` — the snapshot is *not* a snapshot in the time sense
- **Difficulty:** medium
- **Location:** `src/watch_status.rs:101,219` (`idle_secs`); compare to `snapshot_at: i64` at line 109
- **Description:** `WatchSnapshot` has both `snapshot_at: i64` (Unix seconds when the snapshot was published) and `idle_secs: u64` (seconds since the last filesystem event, computed at `compute()` time). The snapshot is meant to be cheaply cloneable and re-readable — but `idle_secs` is *frozen* at computation time. A daemon that publishes a snapshot at T=100 and serves it to a client at T=200 will report `idle_secs=5` (the value at T=100) when the actual live idle is 105 seconds. Every other counter (`modified_files`, `dropped_this_cycle`) is also frozen — but those are *event counts*, where freezing is the correct semantic. `idle_secs` is the only field whose value drifts as wall-clock advances; freezing it makes the wire shape lie. This isn't a bug today because the daemon publishes once per 100ms tick, but the field name and semantics are inconsistent with the rest of the struct.
- **Suggested fix:** Document that `idle_secs` is "idle at snapshot time" not "idle now"; agents should compute live idle as `now - (snapshot_at - idle_secs)` using `last_event_at: i64` instead. Even better: replace `idle_secs` with `last_event_at: Option<i64>` (Unix seconds of the last event), so consumers can compute live idle themselves. Same shape as `last_synced_at`. Rename to make the freshness vector consistent: `last_event_at`, `last_synced_at`, `snapshot_at` — three timestamps, every consumer can compute deltas against `now()`.
# Audit Findings — Error Handling (v1.30.1)

Audit scope: result chains, error context, recovery paths, swallowed errors. Reviewed against `docs/audit-triage-v1.30.0.md` to skip already-fixed items.

## Error Handling

#### EH-V1.30.1-1: Parse failure leaves file with stale chunks AND no mtime update — reconciles forever
- **Difficulty:** medium
- **Location:** `src/cli/watch/reindex.rs:255-314` (`parse_file_all_with_chunk_calls` Err branch)
- **Description:** When `parser.parse_file_all_with_chunk_calls` returns `Err` (file has a syntax error after edit), the watch loop logs a warn and emits zero chunks (`vec![]`) for that file. Two follow-on bugs:
  1. The file's previous chunks are NOT pruned — `upsert_chunks_calls_and_prune` only runs once per file via the `by_file` HashMap, and a parse-failed file never lands in that map. Old chunks stay in the index as ghosts.
  2. The file's `source_mtime` in `chunks.source_mtime` is never updated, so reconcile.rs:124 (`disk > stored`) keeps classifying the file as MODIFIED on every `run_daemon_reconcile` tick. Result: one syntax error in one file produces an unbounded reconcile-requeue-fail-warn loop in the journal until the file parses again.
- **Suggested fix:** On parse failure, still call `store.touch_source_mtime(rel_path, disk_mtime)` (or equivalent) so reconcile.rs sees `disk == stored` and stops requeuing. Optionally also queue the file's chunks for deletion, matching the deleted-file branch at line 244-253. Add a parse_errors counter to `WatchSnapshot` so `cqs status --watch-fresh` can report stuck-bad-file files instead of hiding them.

#### EH-V1.30.1-2: `wait_for_fresh` collapses transport AND parse/dispatch errors into `NoDaemon`
- **Difficulty:** easy
- **Location:** `src/daemon_translate.rs:660-678`, eval consumer at `src/cli/commands/eval/mod.rs:256-263`
- **Description:** Any `Err(msg)` from `daemon_status` — including read timeout, malformed envelope, daemon-returned non-`ok` status, JSON parse failure — collapses into `FreshnessWait::NoDaemon(msg)`. The eval gate then bails with "watch daemon not reachable: ..." and instructs the user to start `cqs watch --serve`. But if the daemon IS running and just returned a malformed status payload, telling the user to start the daemon is wrong advice. The user starts/restarts, sees the same error, and is stuck.
- **Suggested fix:** Distinguish three cases in `wait_for_fresh`'s error path: socket-missing → `NoDaemon`; transport (connect/read/write timeout) → new `Transport(msg)` variant; envelope/parse/non-ok → new `BadResponse(msg)` variant. The eval gate's bail message should differ per variant. Alternatively, surface the verbatim daemon error string in the bail and append the "or start the daemon" advice as a hint instead of a categorical claim.

#### EH-V1.30.1-3: `dispatch.rs:207` swallows slot-name validation errors
- **Difficulty:** easy
- **Location:** `src/cli/dispatch.rs:207`
- **Description:** `let resolved_slot = cqs::slot::resolve_slot_name(cli.slot.as_deref(), project_cqs_dir).ok();` — `resolve_slot_name` only fails on `validate_slot_name` (the user's `--slot foo!` or `CQS_SLOT=bad/name`). Silently `.ok()`-ing the error means the persisted slot model is silently NOT loaded, but the rest of the program continues. The user gets no signal that their slot name was rejected here; if downstream slot resolution succeeds via a different path (e.g., directory existence check), the model intent is silently wrong. If it fails again later, the error message points at a different layer than the actual culprit.
- **Suggested fix:** Replace `.ok()` with `match` that emits `tracing::warn!(error = %e, slot = ?cli.slot, "slot resolution failed when looking up persisted model intent")` before falling through to `None`. Cost: 5 lines, makes operator debugging tractable.

#### EH-V1.30.1-4: `doctor` silently treats `list_slots` failure as empty — audit tool hides its own diagnostic
- **Difficulty:** easy
- **Location:** `src/cli/commands/infra/doctor.rs:923`
- **Description:** `let names = cqs::slot::list_slots(project_cqs_dir).unwrap_or_default();` — `cqs doctor` is the diagnostic command users run when something is wrong. If `list_slots` itself fails (permission denied on `.cqs/slots/`, IO error, etc.) the doctor reports "no slots" instead of "I couldn't read the slots directory" — the very signal the user came to find. Doctor's job is to surface failures, not paper over them.
- **Suggested fix:** Replace with explicit `match`: on Err, emit a warn AND include the error in the doctor JSON envelope under a new `slot_listing_error` field so `cqs doctor --json` consumers can detect the failure. Same pattern other doctor checks use (e.g., daemon connectivity).

#### EH-V1.30.1-5: `cqs index --json` envelope reports model="" on resolution failure, no signal
- **Difficulty:** easy
- **Location:** `src/cli/commands/index/build.rs:863-867`, also `chunk_count = store.chunk_count().unwrap_or(0)` line 867
- **Description:** Two `unwrap_or` calls in the JSON envelope construction silently substitute defaults:
  - `try_model_config().map(|c| c.name.clone()).unwrap_or_default()` → empty string
  - `store.chunk_count().unwrap_or(0)` → 0
  CI consumers parsing this JSON cannot distinguish "indexed 0 chunks because the corpus is empty" from "DB query failed and chunks are present but uncountable". Same for the model field. A failing post-index health check would be silently masked.
- **Suggested fix:** Both are `Result`s. Either propagate via `?` (the index command already returns `Result`), or include explicit `error` field in the envelope when either fails. JSON consumers that opted into `--json` deserve to know when fields are unavailable vs. legitimately zero/empty.

#### EH-V1.30.1-6: Reranker checksum-marker write silently dropped — re-verifies on every cold start
- **Difficulty:** easy
- **Location:** `src/reranker.rs:524`
- **Description:** `let _ = std::fs::write(&marker, &expected_marker);` — the marker that records "checksums verified, skip on next launch" is written with a discarded Result. If the write fails (read-only model dir, no space, perm), every cold start re-runs blake3 on `model.onnx` (~80MB) + `tokenizer.json`. Slow startup, no operator signal that the optimization is broken. The doc-comment above just says "Write marker after successful verification" — no indication that the write itself can fail silently.
- **Suggested fix:** `if let Err(e) = std::fs::write(&marker, &expected_marker) { tracing::warn!(error = %e, path = %marker.display(), "Failed to write reranker verification marker — next launch will re-verify checksums"); }`. Two lines, ends a silent perf loss.

#### EH-V1.30.1-7: Reconcile mtime-stat error swallowed, file may oscillate as "always stale"
- **Difficulty:** medium
- **Location:** `src/cli/watch/reconcile.rs:116-127`, plus same pattern at `src/cli/watch/reindex.rs:501-509`
- **Description:** `lookup_path.metadata().and_then(|m| m.modified())` on Err returns `None`, then `(Some(_), None) => false` → "leave to GC". Comment says this is intentional, but two issues:
  1. No tracing — operators can't tell stat failures from "file genuinely missing". Permission-denied during git checkout (briefly held by a Windows AV scanner under WSL `/mnt/c/`) silently leaves the file un-reconciled until the next walk picks it up.
  2. The pair case `(None, Some(_))` AND `(None, None)` both hit `(None, _) => true` (always requeue), but if the disk-mtime read also fails, the same file gets requeued every reconcile cycle (default 30s) without ever progressing — process_file_changes will see no event-driven trigger and reconcile alone will keep adding it.
- **Suggested fix:** On Err, emit `tracing::debug!(path = %lookup_path.display(), error = %e, "Reconcile: stat failed, leaving file to GC")` so operators have a journal trail. Same pattern in reindex.rs:501 — silently storing `mtime=None` should at minimum warn once.

#### EH-V1.30.1-8: `try_init_embedder` Err path returns from `process_file_changes` without clearing dirty flags
- **Difficulty:** medium
- **Location:** `src/cli/watch/events.rs:154-157` (early return) vs. the dirty flags set immediately after at lines 178-185
- **Description:** Look at the order of operations in `process_file_changes`:
  1. Line 154: `try_init_embedder` returns `None` on failure → bare `return`.
  2. Lines 178/182: `set_hnsw_dirty(...)` calls happen AFTER, so this case doesn't strand dirty flags.

  Reading more carefully: the flags are set BELOW the early return. The hazard is the inverse — if `set_hnsw_dirty` succeeds at line 178 but reindex_files fails between 186 and the matching `clear_hnsw_dirty_with_retry` later (e.g. SQLite busy, panic, OOM), the dirty flag stays `true` until the next successful reindex. That dirty flag drives the daemon to do a full HNSW rebuild on next startup, which is correct but expensive. There's no operator visibility into "we've been dirty for 3 cycles in a row" — each failed cycle just emits a warn and moves on.
- **Suggested fix:** Add a counter to `WatchState` for "consecutive cycles where set-dirty succeeded but reindex failed". When > N (say 3), emit a louder warn ("HNSW has been dirty for 3 cycles — search may serve stale results until reindex completes; check journalctl for prior errors"). Surface the count in `WatchSnapshot` so `cqs status --watch-fresh` shows it. Stops a single transient failure from masking a persistent problem.

# Observability Audit — v1.30.1

Baseline: 550 `info_span!`/`trace_span!`/`debug_span!` sites across `src/`. Default subscriber bug fixed in v1.30.0 (OB-V1.30-1) so info-level events are now visible — exposes hot-path spam below.

## Observability

#### OB-V1.30.1-1: `tracing::info!` "SPLADE routing" fires on every search call — journal spam at default level
- **Difficulty:** easy
- **Location:** `src/search/router.rs:469-474, 491-496, 549-554`
- **Description:** `resolve_splade_alpha()` is called on every search request (CLI, daemon dispatch, batch). Three call sites emit `tracing::info!(category, alpha, source, "SPLADE routing")` — one per call. Now that OB-V1.30-1 made the default subscriber show info-level events, each interactive `cqs search` and every batched query writes a journal line. With agents running thousands of searches per session this is high-volume noise that drowns useful events. The values rarely change after the first call (env vars resolve identically), so a once-per-process emission would carry the same diagnostic value.
- **Suggested fix:** Demote the three "SPLADE routing" sites to `tracing::debug!`, or gate the info emission behind a `OnceLock<()>` so it fires only on first resolution per process. The existing `_span = debug_span!("resolve_splade_alpha")` already gives traceability for those who enable debug logs.

#### OB-V1.30.1-2: `reclassify_with_centroid` emits `tracing::info!` for every Unknown-gap fill — per-search spam
- **Difficulty:** easy
- **Location:** `src/search/router.rs:1146-1150`
- **Description:** Same problem as above on a different surface: every search whose rule-based classifier returns `Unknown` and where the centroid classifier fills the gap (a common case — `Unknown` is the default classification for many natural-language queries) emits `tracing::info!(centroid_category, margin, "centroid filled Unknown gap")`. Combined with OB-V1.30.1-1 above, a single search produces 2-3 info-level lines unrelated to the user-issued operation. Hot loop, structured field shape doesn't help when frequency is the problem.
- **Suggested fix:** Demote to `tracing::debug!`. The function already has `info_span!("reclassify_with_centroid")` at entry which provides span-based traceability when the operator opts in.

#### OB-V1.30.1-3: `WatchSnapshot` state transitions are silent — no journal trail for Stale/Fresh/Rebuilding flips
- **Difficulty:** medium
- **Location:** `src/cli/watch/mod.rs:149-185` (`publish_watch_snapshot`); `src/watch_status.rs:195-224` (`WatchSnapshot::compute`)
- **Description:** The watch loop calls `publish_watch_snapshot` every tick (~100 ms). The new snapshot silently overwrites the previous via `*guard = snap`. There is no event when `state` flips Fresh→Stale (a file change actually arrived), Stale→Fresh (drain completed), or Rebuilding→Fresh (HNSW swap landed). Operators running `journalctl --user -u cqs-watch -f` to debug "why does the eval gate think the index is stale?" see nothing — the only signals are info-level events buried in `process_file_changes`/`spawn_hnsw_rebuild`, which fire on internal events but not on the *resolved* freshness verdict. The `idle_secs`/`modified_files` history is also unrecoverable after the fact because the snapshot is overwrite-only.
- **Suggested fix:** In `publish_watch_snapshot`, compare `prev_state` against `new.state` before the overwrite. On change, emit `tracing::info!(prev = prev.as_str(), next = next.as_str(), modified_files, rebuild_in_flight, dropped_this_cycle, "watch state transition")`. One event per actual change, not per tick. Cardinality is bounded (4×4 - 4 self-edges = 12 transitions max). The reconcile/rebuild call sites can keep their existing detail.

#### OB-V1.30.1-4: `wait_for_fresh` polling loop returns without a closing tracing event recording outcome + elapsed
- **Difficulty:** easy
- **Location:** `src/daemon_translate.rs:660-679`
- **Description:** `wait_for_fresh` opens an `info_span!("wait_for_fresh", wait_secs)` but every return path (Fresh/Timeout/NoDaemon) is silent. The eval gate's failure mode "watch index is still stale after Ns" surfaces an `anyhow::bail!` to the user, but no journal record exists of how long the wait actually was, what the final modified_files count was, or whether the loop exited early on Fresh vs hit deadline. For long waits (`--require-fresh-secs 600`), this is the difference between "I know my agent waited 8 minutes for a 47-file rebuild" and "I have no idea why my run was slow." The 250 ms poll itself is intentionally not logged (would be 2400 events on a 600s wait), but the resolution at exit should be.
- **Suggested fix:** Add three terminal `tracing::info!`s before each return: `info!(elapsed_ms = start.elapsed().as_millis(), modified_files = snap.modified_files, "wait_for_fresh: index reached Fresh")` for the success path, `info!(elapsed_ms, modified_files = snap.modified_files, pending_notes = snap.pending_notes, rebuild_in_flight = snap.rebuild_in_flight, "wait_for_fresh: timeout — index still stale")` for timeout, and `info!(error = %msg, "wait_for_fresh: daemon unreachable")` for NoDaemon.

#### OB-V1.30.1-5: `enforce_auth` 401 warn lacks rejection reason — three failure modes collapse to one log line
- **Difficulty:** easy
- **Location:** `src/serve/auth.rs:389-401`, `src/serve/auth.rs:269-321` (`check_request`)
- **Description:** The 401 warn (added by P1.21 in v1.30.0) logs `method` + `path` but not *why* the request was rejected. `check_request` has three independent paths that can fail (no `Authorization: Bearer …`, mismatched cookie value, missing/wrong `?token=` query param), and operators investigating "why are clients hitting 401?" can't tell from the journal whether tokens are being sent at all (auth missing) vs sent in the wrong format (auth misconfigured) vs being intercepted/expired (auth replaced). With the `cqs serve` cookie-and-header scheme this matters: a frontend that has the wrong cookie name and no header looks identical in the log to one that's missing auth entirely. `check_request`'s control flow already knows which fallthrough fired; just plumb that out.
- **Suggested fix:** Change `AuthOutcome::Unauthorized` to carry a low-cardinality reason enum (`MissingAll`, `BearerMismatch`, `CookieMismatch`, `QueryParamMismatch`) — built from which guards fired in `check_request`. Log that as a structured field: `tracing::warn!(method = %req.method(), path = %req.uri().path(), reason = %reason, "serve: rejected unauthenticated request")`. Reason is bounded so cardinality stays safe.

#### OB-V1.30.1-6: `require_fresh_gate` lacks entry span and final-decision info — eval gate decisions invisible in trace
- **Difficulty:** easy
- **Location:** `src/cli/commands/eval/mod.rs:219-275`
- **Description:** `require_fresh_gate` is the central control point for the new `--require-fresh` watch-mode gate (#1182). Two of its decision paths (`--no-require-fresh`, `CQS_EVAL_REQUIRE_FRESH=0`) emit `tracing::info!`, but the unix happy path that actually calls `wait_for_fresh` has no entry span and no completion event. The eval run summary in the journal — the thing an operator searches for to answer "did this run wait? did it skip? did it bail?" — is missing. Compare with `runner::run_eval` which has a full `info_span!("eval_run", query_file = ..., budget = ...)` at entry; the gate that decides whether `run_eval` even fires has no parallel.
- **Suggested fix:** Wrap the function body in `let _span = tracing::info_span!("require_fresh_gate", wait_secs).entered()`. After `wait_for_fresh` returns, emit a single `tracing::info!(outcome = "fresh"|"timeout"|"no_daemon", elapsed_ms, modified_files, "require_fresh_gate: resolved")` so the result is greppable. Removes the need for the operator to correlate `wait_for_fresh` span output with the bail message in the user's stderr.

#### OB-V1.30.1-7: `daemon_reconcile` walk has no elapsed timing field — reconcile pass duration unrecoverable
- **Difficulty:** easy
- **Location:** `src/cli/watch/reconcile.rs:63-148`
- **Description:** `run_daemon_reconcile` opens `info_span!("daemon_reconcile")` at entry and emits `tracing::info!(queued, added, modified, "Reconcile: queued divergent files for reindex")` at the end, but the duration of the disk walk + diff is never recorded. On large repos with bulk renames this can be multi-second; on WSL with `/mnt/c` it can be tens of seconds. Without `elapsed_ms`, operators can't tell whether reconcile is the cause of a "watch loop missed events" complaint or whether the walk is fast enough to ignore. Same story for `run_daemon_startup_gc` (`src/cli/watch/gc.rs:103-180`) and `run_daemon_periodic_gc` (`:195-243`) — the spans emit counts but not timings.
- **Suggested fix:** Capture `let start = std::time::Instant::now()` at entry; include `elapsed_ms = start.elapsed().as_millis()` in the terminal `tracing::info!` (or `tracing::debug!` for the no-divergence branch). One field per call, three call sites — small, mechanical. Pattern matches what the embedder/HNSW build sites already do.

#### OB-V1.30.1-8: `daemon_status` connect failure logs at warn during normal startup race — every poll iteration
- **Difficulty:** medium
- **Location:** `src/daemon_translate.rs:438-441` (in `daemon_status`), called from `wait_for_fresh` poll loop at `:660-679`
- **Description:** `daemon_status` calls `tracing::warn!(stage = "connect", error = %e, "daemon_status failed")` whenever `UnixStream::connect` fails. That's fine for one-shot use, but `wait_for_fresh` invokes it in a 250 ms polling loop. If the daemon restart-races a `cqs eval --require-fresh` invocation (e.g. systemctl restart), every 250 ms the loop emits a connect-warn for up to `wait_secs`. A 600s wait could write 2400 warn lines. The connect-failure path also already returns `FreshnessWait::NoDaemon` and exits — but the warn fires before the exit, and on transient mid-poll errors (less common but possible) the warns accumulate. Also affects `daemon_ping` similarly.
- **Suggested fix:** Move the `tracing::warn!` out of `daemon_status`/`daemon_ping`'s connect-failure closure and into the *caller* that decides whether the failure is fatal. `wait_for_fresh` should own the "this is the final answer" warn; the inner functions should `tracing::debug!` connect failures so polling doesn't paper over the journal. Alternatively, sample the warn (log first + every Nth) — but call-site relocation is the cleaner fix.

#### OB-V1.30.1-9: `process_file_changes` uses `println!` in non-quiet mode — bypasses tracing infrastructure entirely
- **Difficulty:** easy
- **Location:** `src/cli/watch/events.rs:147-152`
- **Description:** When `cfg.quiet` is false, the watch loop prints `\n{} file(s) changed, reindexing...` followed by per-file paths via `println!` — straight to stdout. The systemd unit captures stdout to journal, so this works *technically*, but: (1) it's not a structured event so `journalctl --output=json | jq` can't parse it, (2) it doesn't honour the trace subscriber's filter level, (3) the print is unconditional on `!quiet` even though there's already a separate `tracing::info!` at `:286` in `spawn_hnsw_rebuild` covering the same ground at the right level. A daemon process should never write user-facing UI to stdout — that's reserved for the foreground CLI.
- **Suggested fix:** Replace the `println!` block with `tracing::info!(file_count = files.len(), files = ?files, "watch: reindexing changed files")`. If the foreground (non-daemon) UX really needs the unstructured print, gate it on a separate `cfg.foreground` flag rather than `!quiet`, since the daemon has no terminal to print to anyway.

#### OB-V1.30.1-10: `serve::search` handler logs full query string at info — matches TraceLayer span and risks query-content leak in journal
- **Difficulty:** easy
- **Location:** `src/serve/handlers.rs:189-232`
- **Description:** `tracing::info!(query = %params.q, limit = params.limit, "serve::search")` at line 193 logs the user's full search query at info level. The TraceLayer span installed by `cqs serve` already records the request URI (with query string redacted per P1.11), so this duplicates that — but at a different layer that doesn't go through the redaction logic. A user search like `serve` getting `?q=AKIA...` (accidentally pasting a credential) writes the credential to the journal verbatim, where the redacted TraceLayer would have sanitized it. Same applies to the `tracing::info!(matches = matches.len(), "search returned")` at line 231 — that one's fine, but the entry log should follow the redaction policy.
- **Suggested fix:** Either (a) demote the entry log to `tracing::debug!` so it doesn't fire at default level, or (b) log only `q_len = params.q.len()` and `limit` — the matches-returned line at the end carries the per-call diagnostic value. The query content already lives in `query_log` (when enabled) which has its own opt-in surface; the live tracing journal shouldn't double-record it.
## Test Coverage — adversarial / sad-path

#### TC-ADV-1.30.1-1: `AuthToken::try_from_string` has no upper-bound length check, no length test
- **Difficulty:** easy
- **Location:** src/serve/auth.rs:123-129 (production); src/serve/auth.rs:432-473 (tests)
- **Description:** `try_from_string` only validates the alphabet — it accepts any-length input including multi-megabyte strings. Comment at line 143-147 explicitly says "We do not require the length be exactly 43 … future deterministic tokens may be shorter". There is no `MAX_TOKEN_LEN` cap (verified by `grep MAX_TOKEN_LEN` returning zero hits) and no test pins behavior at extreme lengths. A future env-var-stable-token feature that accidentally reads a binary blob (e.g. `CQS_SERVE_TOKEN=$(cat huge.bin | tr -dc 'A-Za-z0-9_-')`) succeeds, then becomes the value sent through `HeaderValue::from_str` in the Set-Cookie path on every redirect. axum / hyper enforce a header-line cap (~16 KiB by default), so an oversized token silently fails the redirect handoff at runtime instead of failing at construction. The test suite never observes this — boundary inputs (length = 0 verified, length = 1 verified, length = 1 MiB unverified) are not covered.
- **Suggested fix:** Add an explicit `MAX_TOKEN_LEN` (e.g., 256) constant, reject in `is_valid_token_alphabet`, and add `try_from_string_rejects_overlong_input` exercising 257/4096/1 MiB lengths. If the explicit cap is rejected as policy, at minimum add a test that pins "10 KiB alphabet-valid input is accepted today" so a future cap lands intentionally rather than as a surprise.

#### TC-ADV-1.30.1-2: `check_request` ambiguous-channel collision — cookie + query both present, query wrong, cookie right
- **Difficulty:** easy
- **Location:** src/serve/auth.rs:269-321
- **Description:** Three-channel auth is tested for each channel in isolation, but never for the combination. The function checks Bearer → cookie → query in order and returns `Ok` on the first hit. If a stale `?token=wrong` appears alongside a correct `cqs_token_<port>=right` cookie (e.g., a bookmarked URL whose token has been rotated, but the browser still has the new cookie), the cookie path matches first and the request authenticates as `Ok` — but the redirect handoff that strips the bad query never fires, so the operator's URL bar continues to display a stale leaked token. Tests in `auth_tests` (lines 1480-1505 multi-cookie; 1508+ query) cover each channel singly; none mix wrong-query + right-cookie or right-query + wrong-cookie. The latter is more interesting: with cookie wrong and query right, current code returns `OkViaQueryParam` and overwrites the cookie — correct behavior, but unpinned. A future refactor that swaps order or short-circuits on the first present channel could regress silently.
- **Suggested fix:** Add `auth_query_wrong_cookie_right_authenticates_via_cookie_no_redirect`, `auth_query_right_cookie_wrong_redirects_and_overwrites_cookie`, and `auth_two_cookies_with_same_name` (RFC 6265 allows duplicates; current code matches the first occurrence — pin that).

#### TC-ADV-1.30.1-3: `check_request` Bearer-prefix grammar gaps — `Bearer<token>`, double-space, lowercase `bearer`
- **Difficulty:** easy
- **Location:** src/serve/auth.rs:271-280
- **Description:** Bearer parsing uses `strip_prefix("Bearer ")` — a single literal space, case-sensitive. Per RFC 6750 §2.1 the scheme is case-insensitive (`Bearer` / `bearer` / `BEARER` all valid) and the BNF allows multiple SP between scheme and credential. There are no tests pinning current strict behavior. Today: `Authorization: bearer <token>` → 401 (treated as no Bearer). `Authorization: Bearer  <token>` (two spaces) → 401 (the leading space ends up in `bearer` value, fails `ct_eq`). `Authorization: Bearer<token>` (no space) → 401. All three are real client behaviors (some HTTP libraries normalize, some don't). A future refactor to use the `headers` crate's `Authorization::<Bearer>` parser would change all three to 200 — silently relaxing auth. No test catches the change in either direction.
- **Suggested fix:** Add three regression-pin tests asserting current 401 behavior for `bearer ` (lowercase scheme), `Bearer  ` (double-space), and `Bearer<token>` (no separator). Mirror the strip_token_param pattern from P2.30 — comment that the assertion inverts when the SEC fix lands.

#### TC-ADV-1.30.1-4: `wait_for_fresh` — daemon-dies-mid-poll path untested; partial-read / malformed-JSON path untested
- **Difficulty:** medium
- **Location:** src/daemon_translate.rs:660-679 (production); 1204-1376 (tests)
- **Description:** The three existing tests cover (a) fresh on first poll, (b) timeout when daemon stays stale, (c) NoDaemon when socket missing. None of them cover the realistic outage shape: the daemon was up at the first poll but exits between polls. After the listener closes, `daemon_status`'s `UnixStream::connect` returns `ECONNREFUSED` and `wait_for_fresh` collapses to `NoDaemon` — the eval gate then bails with the "watch daemon not reachable" message even though we *just* heard from it. Pin or re-design. Worse, there is no test for malformed-JSON / partial-line responses: if the daemon is mid-write and the connection is interrupted (system load, OOM kill), `read_line` returns a partial JSON line and `serde_json::from_str` fails. `daemon_status` returns the raw serde-json error string `Err("parse envelope failed: ...")` which `wait_for_fresh` then collapses into `NoDaemon`. A user sees "no daemon running" when the daemon is alive but stressed — a misleading diagnostic.
- **Suggested fix:** Add `wait_for_fresh_returns_no_daemon_when_daemon_exits_mid_poll` (mock listener closes after first response, second poll gets refused). Add `daemon_status_surfaces_distinct_error_for_malformed_response` (mock writes `{"status":"ok","output":` then drops, asserting the error string is distinguishable from "no daemon running"). Optionally introduce a `FreshnessWait::TransportError` variant so the eval gate can give a different hint than "start the daemon".

#### TC-ADV-1.30.1-5: `run_daemon_reconcile` — clock-skew (stored mtime in the future) keeps file out of queue forever
- **Difficulty:** medium
- **Location:** src/cli/watch/reconcile.rs:108-132
- **Description:** Predicate is `disk > stored` (strict). If a file's stored mtime is *in the future* (clock was wrong when index was built — common on WSL after host suspends; common on systems with a battery RTC drifting forward; common when copying an index from a machine in the future timezone via `rsync`), then every subsequent edit produces `disk_mtime < stored_mtime` and reconcile silently skips the file. The watch path through inotify still catches the change, but on `/mnt/c/` (where reconcile is the *only* reliable layer per the module docstring) the file silently stays stale. No test exists. The complementary case in `reconcile_skips_unchanged_files` (line 396) only pins `disk == stored`. The existing `reconcile_detects_bulk_modify_burst` pins the forward direction.
- **Suggested fix:** Add `reconcile_clock_skew_stored_mtime_in_future_still_drains_eventually` — seed a chunk with `stored_mtime = SystemTime::now() + 1h`, write the file at `now`, then run reconcile and assert one of: (a) the file is queued anyway (bug fix), or (b) the file is *not* queued today (regression-pin). Either way, also add a `tracing::warn!(stored, disk, "reconcile: stored mtime is in the future")` arm in production so journalctl surfaces the corruption.

#### TC-ADV-1.30.1-6: `run_daemon_reconcile` — `metadata()` Err silently maps to "leave to GC", masking permission-denied stale state
- **Difficulty:** medium
- **Location:** src/cli/watch/reconcile.rs:116-127
- **Description:** When `lookup_path.metadata()` fails (EACCES, EPERM, ELOOP from a symlink loop), `disk_mtime = None`. The match arm `(Some(_), None) => false` then says "can't read disk mtime → leave to GC" — but the GC pass (`prune_missing`) only runs `metadata()` to *remove* indexed-but-missing files; it never reindexes a file that exists-but-can't-be-stat'd. So a permission-denied file silently lingers in the index with stale content forever. There is no `tracing::warn!` on this arm — the failure is invisible. Per the v0.12.1 audit lesson, this is exactly the "bare unwrap_or_default swallowing real errors" pattern: not unwrap_or_default literally, but the equivalent — an Err arm that returns a defaulted decision with no log. No test seeds an unreadable file and observes the silent skip.
- **Suggested fix:** Add `tracing::warn!(error = %e, path = %lookup_path.display(), "reconcile: stat failed")` on the metadata-err arm. Add a Linux-only test that uses `chmod 0` on a parent dir and asserts the warn fires (capture via `tracing_subscriber::test`). Document that the file remains stale until permissions are fixed — or queue it anyway (the reindex path will produce its own clearer error).

#### TC-ADV-1.30.1-7: `env_disables_freshness_gate` test re-implements the function instead of calling it; garbage / whitespace inputs untested through real env
- **Difficulty:** easy
- **Location:** src/cli/commands/eval/mod.rs:282-290 (production); 405-432 (test)
- **Description:** The "test" at line 406 reads:
    ```rust
    for (input, expected) in cases {
        let lower = input.trim().to_ascii_lowercase();
        let observed = matches!(lower.as_str(), "0" | "false" | "no" | "off");
        assert_eq!(observed, *expected, ...);
    }
    ```
  This is testing the body of the function inlined, not the function itself. If a future refactor changes `env_disables_freshness_gate` to read a *different* env var, or to flip the falsy list, the test still passes — it never calls the function. Real behavior gaps that a real-call test would surface: (a) `CQS_EVAL_REQUIRE_FRESH=garbage` returns `false` (gate stays on) — fine, but unpinned. (b) `CQS_EVAL_REQUIRE_FRESH=" 0 "` (whitespace-padded) returns `true` (the `.trim()` handles it) — pinned indirectly only. (c) `CQS_EVAL_REQUIRE_FRESH=` (empty) returns `false` — handled by the empty-string match miss, but not pinned. The same comment-block (lines 410-411) admits "We can't safely set/unset a real env var here" — which is true per default, but `serial_test::serial` is already used for `CQS_DAEMON_TIMEOUT_MS`-touching tests in `daemon_translate.rs::tests`, so the pattern exists.
- **Suggested fix:** Rewrite the test to call `env_disables_freshness_gate()` under `#[serial_test::serial(cqs_eval_require_fresh_env)]`, set/unset the env var per case, save/restore in scope-guard. Cover at minimum: unset, "1", "0", "garbage", "  off  ", "" (empty string). Apply the same `unsafe` env-var save/restore pattern used by `reconcile_enabled_default_true`.

#### TC-ADV-1.30.1-8: `WatchSnapshot::compute` — `delta_saturated=true` with `pending_files=0`, `rebuild_in_flight=false` reports `Fresh` despite a real correctness flag
- **Difficulty:** easy
- **Location:** src/watch_status.rs:199-223
- **Description:** The state machine considers `pending_files_count`, `pending_notes`, `dropped_this_cycle`, and `rebuild_in_flight`. It does **not** consider `delta_saturated`. Per the field's own docstring (lines 84-87), `delta_saturated == true` means "the rebuilt index is discarded on swap and the next threshold rebuild reads SQLite fresh — operators should know if this is happening repeatedly". Today, after a saturated rebuild swap completes, `rebuild_in_flight` flips to false; if `pending_files_count` happens to be zero on the same tick, the snapshot reports `state == Fresh` while `delta_saturated == true` quietly rides along in JSON. An eval consumer gating on `state == fresh` proceeds; a CLI human reading text output sees "fresh" and never notices. There is no test for `delta_saturated=true` (only the field default of `false` is exercised across all 7 tests).
- **Suggested fix:** Decide whether `delta_saturated == true` *should* mark `Stale` (it represents recently-discarded incremental work — the index is correct, but the operator should know). Either way, add `compute_delta_saturated_does_not_affect_state_today` as a regression pin, or change the state machine to include it. If the latter, the change is: `else if input.pending_files_count > 0 || input.pending_notes || input.dropped_this_cycle > 0 || input.delta_saturated`.

#### TC-ADV-1.30.1-9: `daemon_status` — non-`ok` envelope with `null`/`number` `message` field returns "daemon error: daemon error" placeholder
- **Difficulty:** easy
- **Location:** src/daemon_translate.rs:487-499
- **Description:** When the daemon returns `{"status":"err","message": null}` or `{"status":"err","message": 42}` or `{"status":"err"}` (no message), the `.and_then(|v| v.as_str()).unwrap_or("daemon error")` fallback produces the literal string `"daemon error: daemon error"` — a debugging black hole. Tests cover happy path (status=ok) and `daemon_status_errors_without_socket` (no socket) but **not** the malformed/missing-message envelope. The `tracing::warn!` on the same arm logs `msg` (which is the placeholder) so journal also shows `msg="daemon error"` — no diagnostic value at all. Per the v0.12.1 audit lesson, this is the canonical "warn-then-default arm not exercised" anti-pattern.
- **Suggested fix:** Add `daemon_status_handles_err_envelope_with_no_message` and `daemon_status_handles_err_envelope_with_non_string_message` (both return error containing the *shape* of what the daemon sent — at least include the raw JSON, e.g. `format!("daemon error (no message): {envelope}")`).

#### TC-ADV-1.30.1-10: `unwrap_dispatch_payload` accepts a missing `data` field by passing through the wrapper as the payload — no test for "envelope without data"
- **Difficulty:** easy
- **Location:** src/daemon_translate.rs:387-404
- **Description:** Lines 400-402:
    ```rust
    match parsed.as_object().and_then(|m| m.get("data")) {
        Some(data) => Ok(data.clone()),
        None => Ok(parsed),
    }
    ```
  Comment says "Otherwise pass through (legacy bare-payload mock form)". This means: if the daemon sends `{"version":1,"error":null}` (envelope shape but no `data`), the function returns that wrapper as the WatchSnapshot/PingResponse to deserialize from — `serde_json::from_value::<WatchSnapshot>` fails with "missing field `state`". The user sees an opaque deserialize error. The fall-through-to-wrapper branch is exercised only by the "legacy bare-payload mock" pattern in `daemon_status_mock_round_trip` (which sends real bare data, not a missing-data envelope). A real daemon misbehaviour (envelope present, data missing — e.g., a future refactor that sets data=None on internal error) is conflated with the legacy form. Production WatchSnapshot consumers can't distinguish "daemon spoke wrong protocol" from "daemon's legacy mode".
- **Suggested fix:** Add `unwrap_dispatch_payload_distinguishes_envelope_no_data_from_bare_form` test: send `{"data": null, "error": "internal", "version": 1}` and assert the function surfaces `error` as the failure rather than passing the whole envelope through. If keeping the legacy fallthrough is intentional, at least gate it on "object that has neither `data` nor `error` nor `version`" so a partial envelope doesn't masquerade as a payload.
## Robustness

#### RB-1: `to_string_lossy()` on path keys silently mangles non-UTF-8 paths into permanent reindex storms
- **Difficulty:** medium
- **Location:** `src/cli/watch/reconcile.rs:99` (key constructor) + `src/store/chunks/staleness.rs:627-637` (DB-side keys came from a different lossy conversion at insert time)
- **Description:** `run_daemon_reconcile` builds the lookup key for every disk file via `rel.to_string_lossy().replace('\\', "/")` and indexes it against `store.indexed_file_origins()` (a `HashMap<String, _>`). On a path with non-UTF-8 bytes (Windows wide-char surrogate halves, latin-1 squatter byte sequences from foreign filesystems mounted under WSL `/mnt/c/`, or anything dropped on disk by a tool that didn't enforce UTF-8), `to_string_lossy` substitutes `U+FFFD` replacement characters. The DB-side origin string was stored at index time via a different code path (`Chunk::file: PathBuf` → `String` conversion that may have used `to_string_lossy()` *or* `display()` *or* the `path.to_str()` early-return). If the lossy encoding shape disagrees between the two sites — even by one byte — the lookup misses every cycle, the file is forever classified as ADDED, and `pending_files` re-queues it on every periodic reconcile pass (default 30 s). On WSL `/mnt/c/` (the documented worst case for missed events) a single non-UTF-8 file wedges the reconcile loop into a steady-state burn. No tracing, no warning — it looks like the reconcile loop is just doing its job.
- **Suggested fix:** Skip non-UTF-8 paths up front with a warn. Replace the body of the loop with `let Some(origin) = rel.to_str().map(|s| s.replace('\\', "/")) else { tracing::warn!(path = %rel.display(), "reconcile: skipping non-UTF-8 path"); continue; };` so the encoding shape is exact-match by construction. Same change should land at the indexer side so origins are never inserted with replacement characters, eliminating the disagreement class entirely.

#### RB-2: `wait_for_fresh` panics on `Instant + Duration::from_secs(u64::MAX)` if any non-CLI caller forgets the cap
- **Difficulty:** easy
- **Location:** `src/daemon_translate.rs:660-662`
- **Description:** `pub fn wait_for_fresh(cqs_dir, wait_secs: u64)` does `Instant::now() + Duration::from_secs(wait_secs)`. `Add<Duration> for Instant` panics on overflow per Rust docs. Today every CLI callsite caps at 600 (`eval/mod.rs:237`, `infra/status.rs:63`), but the function is `pub` — any future consumer (an external crate? a new in-tree command? a script forwarding `u64::MAX` as a "wait forever" sentinel?) who skips the cap immediately panics the worker. The cap is enforced by convention, not by the type system. The doc comment even claims "wait_secs (capped at 600 by the caller's clap default — caller is …)" which makes the function's contract caller-dependent.
- **Suggested fix:** Cap defensively inside the function: `let wait_secs = wait_secs.min(86_400);` (one day is more than any sane caller needs and well below `Instant + Duration` overflow). Or use `Instant::now().checked_add(Duration::from_secs(wait_secs)).unwrap_or_else(|| Instant::now() + Duration::from_secs(86_400))`. The function then satisfies its contract for every u64 input.

#### RB-3: `as_secs() as i64` SystemTime-to-i64 cast pattern landed in 5 new locations post-RB-V1.30-3
- **Difficulty:** easy
- **Location:** `src/watch_status.rs:229`, `src/cli/batch/mod.rs:779`, `src/cli/batch/mod.rs:1988`, `src/cli/commands/infra/ping.rs:122`, `src/cli/watch/mod.rs:159`
- **Description:** RB-V1.30-3 fixed the pattern in `src/cache.rs` only. The same `SystemTime::now().duration_since(UNIX_EPOCH).map(|d| d.as_secs() as i64)` cast (u64 → i64) sits in five more production paths added or untouched in v1.30, all of which silently wrap to negative after year 2554. `watch_status.rs:229` is on the `now_unix_secs()` helper that powers every `WatchSnapshot::compute` call; `cli/watch/mod.rs:159` is the `last_synced_at` value the watch loop publishes on every tick. Latent (the deadline is 528 years out), but the same pattern that motivated the original finding has spread, so the original fix's "Defense-in-depth" rationale applies identically.
- **Suggested fix:** Hoist a single helper `fn unix_secs_i64() -> Option<i64> { SystemTime::now().duration_since(UNIX_EPOCH).ok().and_then(|d| i64::try_from(d.as_secs()).ok()) }` in `crate::time` (or extend the existing `cache::now_unix_i64`), have all five callsites use it. Eliminates the cast-pattern re-emergence. Bonus: the helper makes "what does the time look like right now?" a single grep target for future audits.

#### RB-4: `as_millis() as i64` cast in reindex pipeline truncates u128 to i64 silently
- **Difficulty:** easy
- **Location:** `src/cli/watch/reindex.rs:507-508`
- **Description:** `t.duration_since(UNIX_EPOCH).ok().and_then(|d| ...).map(|d| d.as_millis() as i64)` casts a `u128` to `i64`. For typical mtimes this fits, but a corrupt or maliciously-crafted file with mtime far in the future (e.g. year-9999 `touch -d` from a tool, or a tar extracted with a wrong epoch) gives `as_millis()` a value beyond `i64::MAX` (= ~year 292,277,000 in milliseconds) — wait, well above any plausible year-9999 corruption. The actual risk: SystemTime can also represent times *before* 1970 if the file was pre-dated (`touch -d 1969-01-01`), in which case `duration_since(UNIX_EPOCH)` returns `Err` and the `.ok()` short-circuits to `None`. So pre-epoch is silently treated as "no mtime" — fine. The real concern is the *u128 → i64 truncation* path: it can produce arbitrary nonsense values (negative, zero, anything) on platforms that report wildly future mtimes through `metadata().modified()`. The reindex pipeline then writes that nonsense into `source_mtime`, and the next reconcile pass uses it for the `disk > stored` comparison.
- **Suggested fix:** Use `i64::try_from(d.as_millis()).ok()` to surface the overflow as `None` (handled the same way as missing mtime). Defense-in-depth — silently corrupting `source_mtime` for one pathological file class is worse than treating it as unknown.

#### RB-5: `migrate_legacy` sentinel read uses unbounded `fs::read_to_string`
- **Difficulty:** easy
- **Location:** `src/slot/mod.rs:656`
- **Description:** RB-V1.30-2 capped the slot pointer + `slot.toml` reads at `SLOT_POINTER_MAX_BYTES`, but the migration sentinel file (`MIGRATION_SENTINEL_FILE`, written by RB-V1.30-4's fix) is still read via `fs::read_to_string(&sentinel).unwrap_or_default()`. Same OOM class as RB-V1.30-2: a stray editor swap-file collision, corrupt write during the previous mid-flight crash, or hostile co-tenant in `.cqs/` can leave a multi-GB sentinel behind. Every cqs invocation in that project then attempts `read_to_string` at the top of `migrate_legacy_index_to_default_slot` and OOMs before any other recovery action runs — bricking the project on the *recovery path* the previous fix added.
- **Suggested fix:** Read with a hard cap, mirror the helper RB-V1.30-2's fix introduced. ~64 KiB is plenty for sentinel content (which is the inventory of partially-moved files plus a timestamp and a tracing-style error string). Reuse the same `read_capped` helper if it was extracted; otherwise inline `let mut buf = String::new(); fs::File::open(&sentinel)?.take(64 * 1024).read_to_string(&mut buf)?;` and gracefully degrade the user-facing message on cap-hit ("sentinel file is unreadable / too large; manually rm to retry").

#### RB-6: `enumerate_files` `path.strip_prefix(&root).unwrap_or(&path)` on canonicalize-mismatch silently leaks absolute paths into the relative-path workflow
- **Difficulty:** medium
- **Location:** `src/lib.rs:680-685`
- **Description:** After per-entry canonicalize, `if path.starts_with(&root)` is true → `path.strip_prefix(&root).unwrap_or(&path).to_path_buf()`. The `unwrap_or(&path)` fallback is a code-smell: `starts_with` says yes but `strip_prefix` says no. This shouldn't happen for canonicalized paths from the same canonicalized root, but on case-insensitive filesystems (NTFS/HFS+) `starts_with` does byte comparison — `C:\Projects\cqs` starts_with `C:\Projects\Cqs` is false even though they refer to the same directory. The reverse is also possible: `dunce::canonicalize` on Windows can return paths with different case than the canonicalized root if a junction/symlink is involved. When the fallback fires, an absolute path leaks into the file list, gets stored as the chunk origin, and subsequent reconcile/staleness lookups (which all assume relative paths) miss every match. Symptom: a subset of files perpetually re-indexed every reconcile pass.
- **Suggested fix:** Replace the `unwrap_or(&path)` with an explicit warn-and-skip: `match path.strip_prefix(&root) { Ok(rel) => Some(rel.to_path_buf()), Err(_) => { tracing::warn!(path = %path.display(), root = %root.display(), "starts_with said yes but strip_prefix failed — case-insensitive fs?"); None } }`. Surfaces the disagreement instead of silently corrupting the index.

#### RB-7: Atomic `as u64` / `as i64` casts in WatchSnapshot trust unbounded `usize` from caller
- **Difficulty:** easy
- **Location:** `src/watch_status.rs:213, 217, 218`
- **Description:** `WatchSnapshot::compute` casts `pending_files_count: usize` → `u64` etc. directly: `modified_files: input.pending_files_count as u64`. On 64-bit platforms (every release target) the cast is widening — fine. On 32-bit (technically still in MSRV scope) it's also widening. The actual issue: `dropped_this_cycle: input.dropped_this_cycle as u64` is a counter that's monotonically incremented in the watch loop. If a long-lived daemon overflows usize::MAX — implausible (~2e10/sec for 8 years on 64-bit) — the subsequent cast to u64 silently misses the overflow. More immediately: the snapshot exposes *all three* counters as `u64` over JSON to clients without any saturating semantics — a JSON consumer that does arithmetic on `modified_files - last_modified_files` to get a delta gets a wrap when the counters reset on daemon restart. Not a panic path; defense-in-depth.
- **Suggested fix:** Document the wrap semantics on the JSON schema, or use `saturating_cast` (e.g. `u64::try_from(input.pending_files_count).unwrap_or(u64::MAX)`) so over-large values clamp instead of wrapping. Low priority but lowers the surface for "snapshot.modified_files = 0 right after a burst" misdiagnosis.

#### RB-8: `print_text_report::pct` on empty eval set → division by zero / NaN bleed
- **Difficulty:** medium
- **Location:** `src/cli/commands/eval/mod.rs:296-309` + the `pct` helper it calls
- **Description:** `print_text_report` prints `R@1=, R@5=, R@20=` from `report.overall.r_at_5` (and friends), which is computed as a ratio (recall hits / N). If `N == 0` (empty fixture, all queries skipped because none have a `gold_chunk`) the ratio is `0.0 / 0.0 = NaN`. `pct` formats it; `NaN%` shows up in the human-readable output and `null`/`NaN` (depending on serializer) in `--json`. A downstream consumer like the eval-gate workflow sees NaN, branches "is this less than 0.5?", false, says "looks fine", and an empty fixture passes the gate silently. The skipped-count log line at `:308` is the only signal, easy to miss.
- **Suggested fix:** Short-circuit at the top: `if report.overall.n == 0 { eprintln!("[eval] no queries with gold_chunk; refusing to emit report (use --allow-empty to override)"); std::process::exit(2); }`. Also add an assertion or return Err when computing the ratio with N==0, so the failure happens at the report-build site and the error message carries the fixture name.

#### RB-9: `wait_for_fresh` infinite poll loop on slow daemon — no max-poll-iterations safety
- **Difficulty:** medium
- **Location:** `src/daemon_translate.rs:665-678`
- **Description:** The loop polls every 250ms until the deadline. If `daemon_status` returns `Ok` immediately on every call but `is_fresh()` keeps flipping false (e.g. a very busy reindex queue that drains and re-fills as fast as the watch loop ticks), the function spins for the full `wait_secs` budget burning CPU on `daemon_status` calls (each makes a Unix socket connect, write, read) and never makes progress. Worse: each poll call connects to the same daemon socket, so a 600-second poll = 2400 socket connects on a healthy daemon — eats `accept` budget on the daemon side and shows up as a thundering-herd in journal logs. There's no exponential backoff or circuit breaker.
- **Suggested fix:** Cap poll count at `wait_secs * 4` (4 polls/sec is the implied rate) AND introduce an exponential backoff between failed-but-Ok polls: 250ms → 500ms → 1s → 2s, capped at 5s. The 95th-percentile case (daemon already fresh) still returns in one poll; the unhealthy case stops hammering the socket.

#### RB-10: `now_unix_secs()` swallows clock-before-epoch errors as `0`, masking systemic bad-clock conditions
- **Difficulty:** easy
- **Location:** `src/watch_status.rs:226-231`
- **Description:** `SystemTime::now().duration_since(UNIX_EPOCH).map(...).unwrap_or(0)` silently returns `0` (= 1970-01-01) on a clock-before-epoch error. This is reachable on real systems: a Raspberry Pi or VM that boots before NTP sync can have a clock around 1970, on WSL a hypervisor pause-and-resume across host-clock changes can produce transient backwards jumps, on Windows a misconfigured RTC can present 1601-epoch values. When this happens, every WatchSnapshot publishes `snapshot_at: 0`, every JSON consumer sees the snapshot is "from 1970" and downstream freshness logic that does `now - snapshot_at > threshold` (e.g. "is the snapshot itself stale?") suddenly thinks every snapshot is 56 years stale and marks the daemon dead. Silent fallback to `0` collides with "missing/unknown" semantics.
- **Suggested fix:** On the `Err` branch, emit a `tracing::warn!` once (use `OnceLock`) so journal logs surface the bad-clock condition, and return `None`/`-1` instead of `0` so consumers can distinguish "clock is broken" from "snapshot is genuinely from 1970". Update `WatchSnapshot::snapshot_at` to `Option<i64>` to make the missing-clock case unrepresentable as a valid timestamp.
# v1.30.1 Audit Findings — Scaling & Hardcoded Limits

Generated: 2026-04-28T18:28:34Z

Audit mode: ON. Scope: constants that should scale with model config / corpus size / hardware; magic numbers without rationale; defaults sized for testing; dimensions hardcoded for the default model.

## Scaling & Hardcoded Limits

#### SHL-V1.30-1: `embed_batch_size_for` is dead code — production still uses unscaled `embed_batch_size()` of 64
- **Difficulty:** easy
- **Location:** `src/cli/pipeline/types.rs:178-207`, `src/cli/pipeline/parsing.rs:42`, `src/cli/enrichment.rs:74`
- **Description:** P2.41 in the v1.30.0 triage was marked "fixed (added `embed_batch_size_for(model)`; pipeline migration follow-on)" — but the helper is still `#[allow(dead_code)]` and zero production callers exist. `pipeline::parsing::run_pipeline` (line 42) and `cli::enrichment::run_enrich` (line 74) both call the legacy `embed_batch_size()` which returns 64 regardless of model. The helper's own docstring is explicit: "Nomic-coderank (768 dim, 2048 seq) at batch=64 OOMs the same GPU because the tensor blows up to ~390 MB." So when a user runs `cqs index --model nomic-coderank` on the default RTX 4060 8GB, the indexer ships the OOM-known config. The wiring-verification rule in CLAUDE.md (the configurable-models disaster) is the exact pattern: API exists, no production calls.
- **Suggested fix:** Migrate the two production sites to `embed_batch_size_for(model)`. Drop `#[allow(dead_code)]`. Mark the legacy `embed_batch_size()` `#[deprecated]` or rename to `embed_batch_size_legacy_test_only` so test callers stay valid but production grep clearly fails.

#### SHL-V1.30-2: `wait_for_fresh` poll interval hardcoded at 250 ms — no env knob
- **Difficulty:** easy
- **Location:** `src/daemon_translate.rs:663`
- **Description:** `wait_for_fresh` is the shared client-side gate for `cqs status --watch-fresh --wait` and `cqs eval --require-fresh`. It hardcodes `poll_interval = Duration::from_millis(250)`. On a 100k-chunk reindex after `git checkout`, the daemon takes seconds-to-minutes to converge — 250ms polling is fine but generates ~240 socket accepts/min on the daemon. Conversely, on tiny corpora a 250ms first-poll latency is noticeable in CI orchestration scripts that fire `cqs eval` back-to-back. No env override exists; the comment claims "callers don't pay 250 ms latency on an already-fresh tree" because the loop polls before sleeping (correct), but every subsequent cycle pays the full 250ms. Adjacent constants (`CQS_DAEMON_TIMEOUT_MS`, `CQS_WATCH_RECONCILE_SECS`, `CQS_DAEMON_PERIODIC_GC_INTERVAL_SECS`) all have env knobs — this one is an outlier.
- **Suggested fix:** Add `CQS_FRESHNESS_POLL_MS` to `crate::limits` with a sane default of 250, floor of ~25, ceiling of ~5000. Read once per `wait_for_fresh` call so tests can flip values without rebuild.

#### SHL-V1.30-3: `eval --require-fresh-secs` silently capped to 600 s inside the wait helper
- **Difficulty:** easy
- **Location:** `src/cli/commands/eval/mod.rs:237`, helper at line 246
- **Description:** The CLI's `--require-fresh-secs` clap default is 600. But the helper *also* applies `let budget_secs = wait_secs.min(600);` — so a user passing `--require-fresh-secs 3600` (deliberately for a huge `cqs ref` package or a 100k-chunk post-checkout reindex) gets silently clamped to 600 with no warning. The flag's docstring at line 83 says "Capped at 600 (10 min) inside the wait helper so a runaway agent can't pin the socket" — but a runaway agent can still call `cqs eval` in a loop with `--no-require-fresh` if it really wants to. The cap punishes legitimate slow-corpus users while not actually closing the runaway-agent door. The 600-second cap is a magic number lifted from "10 minutes feels right" with no scaling rationale (corpus-aware: a `cqs ref` package can take 30+ minutes to reindex from cold).
- **Suggested fix:** Either (a) drop the `min(600)` clamp and trust the user's flag value, or (b) raise the ceiling significantly (e.g. 7200 / 2h) and emit a `tracing::warn!` when the clamp engages so users see "your --require-fresh-secs=3600 was clamped to 600". Add an env knob `CQS_EVAL_REQUIRE_FRESH_MAX_SECS` for operator override.

#### SHL-V1.30-4: `task::run_task` hardcodes BFS knobs (depth=2, max_nodes=100, multiplier=3) with no env / CLI override
- **Difficulty:** easy
- **Location:** `src/task.rs:19-25,143-149`
- **Description:** `cqs task` is a public CLI command meant for "implementation brief: scout + gather + impact + placement". It hardcodes:
  - `TASK_GATHER_DEPTH = 2` — BFS expansion depth
  - `TASK_GATHER_MAX_NODES = 100` — node ceiling
  - `TASK_GATHER_LIMIT_MULTIPLIER = 3` — over-fetch factor
  
  The underlying `gather` subsystem honors `CQS_GATHER_MAX_NODES` via `DEFAULT_MAX_EXPANDED_NODES`, but `task` overrides that with its hardcoded 100 by calling `.with_max_expanded_nodes(TASK_GATHER_MAX_NODES)`. So the gather env knob is silently ignored when invoked through `task`. On a 50k+ chunk corpus where 100 nodes can't capture a real-feature implementation surface, the user has no recourse short of editing source.
- **Suggested fix:** Read `CQS_TASK_GATHER_DEPTH` / `CQS_TASK_GATHER_MAX_NODES` (or just inherit the same `CQS_GATHER_*` env vars `gather` already uses) instead of hardcoding. Defaults stay at 2/100/3. Add CLI flags `--task-depth N` / `--task-max-nodes N` if the typical workflow is "I need broader context for this one task".

#### SHL-V1.30-5: `onboard` callee/caller fetch caps hardcoded at 30/15 — no env or CLI override
- **Difficulty:** easy
- **Location:** `src/onboard.rs:30-33,174-175`
- **Description:** `cqs onboard` is the "guided codebase tour". `MAX_CALLEE_FETCH = 30` and `MAX_CALLER_FETCH = 15` cap the BFS-discovered nodes that get content fetched. There's a separate `DEFAULT_ONBOARD_DEPTH = 3` that *is* a public const, but the fetch caps are private and no env override exists. For an onboard query targeting a high-fanout entry point (e.g. `Store::open` has 50+ callers in this codebase alone), 15 callers is half the picture and the user gets a misleading "reading list" without knowing that ~70% of callers were silently dropped. The agent docs (`README.md`) sell `onboard` as a comprehensive tour — silent truncation undermines that contract.
- **Suggested fix:** Promote the constants to env-overridable resolvers (`CQS_ONBOARD_CALLEE_FETCH`, `CQS_ONBOARD_CALLER_FETCH`) and add a `--max-callees N` / `--max-callers N` pair to the `cqs onboard` clap definition. When the cap engages, surface the truncation in the `OnboardSummary` JSON so callers can react instead of silently misinterpreting completeness.

#### SHL-V1.30-6: `MAX_REFERENCES = 20` hardcoded in config validator — no env knob
- **Difficulty:** easy
- **Location:** `src/config.rs:390-405`
- **Description:** `Config::validate` truncates `references` to the first 20 entries with a stderr warning. The rationale comment cites "~50-100MB RAM per reference" → ~1-2GB at 20. But on the project's own A6000 workstation with 256+ GB RAM, this is conservative by ~100×. With v1.30 named slots, a project may legitimately want 25-30 reference packages (every major dependency family). The user has no way to opt up. Hard-cap with no escape hatch.
- **Suggested fix:** Replace the const with `crate::limits::max_references()` reading `CQS_MAX_REFERENCES` (default 20, no upper ceiling — let the user OOM their own machine if they ask for it). Memo the warning so it fires at *load* time not *every validate call*. Keep the existing soft warning; remove the silent truncation when env override is set.

#### SHL-V1.30-7: `MAX_NOTES_FILE_SIZE = 10 MB` hardcoded twice with no env knob
- **Difficulty:** easy
- **Location:** `src/note.rs:169` and `src/note.rs:245` (duplicate definitions)
- **Description:** Two separate `const MAX_NOTES_FILE_SIZE: u64 = 10 * 1024 * 1024;` declarations in different functions of the same file — DRY violation in addition to the scaling concern. 10 MB is plenty for hand-edited `notes.toml`, but on a long-running project with `cqs notes add` invocations from agents, the file can grow. There's also a separate `MAX_NOTES = 10_000` (line 20) that silently truncates parses (line 331) — comment at line 612 acknowledges "Files with > MAX_NOTES entries are truncated silently". Two magic numbers, both potential silent-failure surfaces.
- **Suggested fix:** Hoist a single `const MAX_NOTES_FILE_SIZE` to module scope. Wrap with an env-overridable resolver `crate::limits::max_notes_file_size()` reading `CQS_NOTES_MAX_FILE_SIZE` (default 10 MB). Same treatment for `MAX_NOTES` via `CQS_NOTES_MAX_ENTRIES`. Surface the silent truncation as a `tracing::warn!` instead of a silent `take(MAX_NOTES)`.

#### SHL-V1.30-8: `ENRICHMENT_PAGE_SIZE = 500` hardcoded — no env knob; affects 100k-chunk reindex memory
- **Difficulty:** easy
- **Location:** `src/cli/enrichment.rs:46,127`
- **Description:** Enrichment pages chunks at 500/page, batch-fetches callers/callees per page (line 142-143). Adjacent comments call out that `all_summaries` / `all_hyde` / `all_enrichment_hashes` are intentionally pre-loaded once (~20MB at 100k chunks) — so the design *does* care about memory at scale — but the page size itself is unmovable. On a small repo (this one, 17k chunks) 500 is 35 pages, fine. On a 200k+ chunk reference package the query batch for callers/callees per page becomes the dominant cost and a tunable would help. Other pipeline batch sizes (`embed_batch_size`, `file_batch_size`) all have env knobs; this one is the outlier.
- **Suggested fix:** Promote to `crate::limits::enrichment_page_size()` reading `CQS_ENRICHMENT_PAGE_SIZE` with default 500. Allows operators on huge corpora to drop it to 100 (less per-page memory, more SQLite round-trips) or push it to 2000 (fewer round-trips, more memory).

#### SHL-V1.30-9: `LAST_INDEXED_PRUNE_SIZE_THRESHOLD = 5_000` doc claims "intentionally not an env var" — but `cqs ref` packages routinely exceed
- **Difficulty:** easy
- **Location:** `src/cli/watch/gc.rs:36-42`
- **Description:** The watch loop's `last_indexed_mtime` dedup map prunes at >5,000 entries. Comment says "intentionally not an env var to avoid knob proliferation. Re-adding a file on its next watch event is a trivial insert". That reasoning holds for normal-sized repos. For an indexed `cqs ref linux-kernel` (~75k files), the prune fires every cycle, dedup loses signal across cycles, and the watch loop ends up reindexing files it just touched because the prune dropped their last-indexed timestamp. The CLAUDE.md memory entry "Knob Count Depends on Consumer — for agent-facing tools (cqs and Monitor), more knobs are cheap and bias toward configurability" applies — this is exactly an agent-facing knob. The "knob proliferation" concern was wrong here.
- **Suggested fix:** Add `CQS_WATCH_PRUNE_SIZE_THRESHOLD` env knob, default 5_000. For `cqs ref`-heavy setups raise to 50_000 to keep dedup signal across cycles. Keep `LAST_INDEXED_PRUNE_AGE_SECS = 86_400` in place; the recency rule is independent of cardinality.

#### SHL-V1.30-10: `daemon_periodic_gc_cap` env override is read-once via `OnceLock` — `systemctl set-environment` can't change it
- **Difficulty:** easy
- **Location:** `src/cli/watch/gc.rs:78-86`
- **Description:** Adjacent env knobs in the watch module pattern read-on-every-call (e.g. `reconcile_enabled` at `reconcile.rs:154` reads on every tick — explicit goal: "an operator can flip it via `systemctl --user set-environment` without daemon restart"). But `daemon_periodic_gc_cap` caches via `OnceLock` and the comment at line 67 claims "parsed at first read" as if that's a feature. It isn't — it's an inconsistency with the sibling `reconcile_enabled` semantic and breaks the live-tunable contract operators expect. Once the daemon starts, the cap is frozen until restart. CLAUDE.md memory: "for agent-facing tools, more knobs are cheap and bias toward configurability — only requirement is the knob works as specified." This knob doesn't work as specified — the docstring promises env-driven tuning but delivers boot-time-frozen tuning.
- **Suggested fix:** Drop the `OnceLock` cache. Read on every call (one `getenv` syscall per GC tick is microseconds; ticks are minutes apart). Match the `reconcile_enabled` / `daemon_periodic_gc_interval_secs` / `daemon_periodic_gc_idle_secs` semantic so all four knobs behave the same way operationally.
## Algorithm Correctness

#### AC-V1.30.1-1: `run_daemon_reconcile` strict `disk > stored` mtime predicate misses git-checkout-to-older-commit
- **Difficulty:** medium
- **Location:** src/cli/watch/reconcile.rs:124
- **Description:** The reconcile predicate is `(stored, disk) => disk > *stored`. Reconcile is the layer-2 safety net for bulk git operations the inotify path misses (the module's own docstring at line 7-9 calls out `git checkout` / `reset --hard` / `merge` as the motivating cases). But `git checkout` of a sibling branch restores file mtimes to their **commit time** — which can easily be *older* than the indexed `source_mtime`. Concrete repro: index file `foo.rs` at HEAD (mtime = now), then `git checkout HEAD~5 -- foo.rs` where the file's HEAD~5 commit is from last week. Disk content is now different, but `disk_mtime (last week) <= stored_mtime (now)`, so reconcile classifies the file as "fine" and skips it. The inotify event-driven path also uses `mtime <= last` (events.rs:100) so the file is silently stale until either `cqs index --force` or the file is touched again. The bulk-delta acceptance test at line 304 only exercises *forward*-mtime cases, missing this asymmetry.
- **Suggested fix:** Reconcile should compare on a content fingerprint, not just mtime, when mtime is non-monotonic. Cheapest path: reuse the existing `content_hash` already stored per chunk — when `disk_mtime != stored_mtime` (in either direction), open the file and hash it; only skip if the file's hash matches the indexed chunk's content hash. Alternative: store a `(mtime, size)` tuple and treat any difference (not just mtime forward) as "queue for reindex". Add a regression test seeding `stored_mtime = now`, then writing the file with `utime` set to `now - 1 day`, asserting reconcile queues it.

#### AC-V1.30.1-2: `is_structural_query` keyword match is case-sensitive — capitalised structural terms misroute to Conceptual
- **Difficulty:** easy
- **Location:** src/search/router.rs:813-816
- **Description:** `is_structural_query` splits on whitespace and compares words to `STRUCTURAL_KEYWORDS` (`struct`, `enum`, `trait`, `impl`, `interface`, `class`, `module`, `namespace`, `protocol`, `type`) with `w == kw` — exact bytewise equality. Queries like `"Trait Iterator"`, `"Class Foo"`, `"Find all Structs"`, or any sentence-cased query starting with a capitalised keyword fail the predicate. P2.44 (already triaged) fixed the *position*-sensitivity bug ("end-of-query keyword misses"), but the *case*-sensitivity sibling is still open. Effect: queries that are unambiguously structural in intent route to Conceptual (α=0.70) instead of Structural (α=0.90), measurably hurting R@5 on capitalised-noun queries. Agents that compose queries from a UI title bar ("Class Hierarchy") are particularly affected. The rest of the router (e.g. `is_behavioral_query`'s phrase probes) appears similarly case-sensitive.
- **Suggested fix:** Lowercase the query once at the top of `classify_query_inner` and pass a normalised &str through both the regex probes and keyword-word loop. The structural keywords are already all-lowercase constants, so equality holds after the normalisation. Add a regression-pin test alongside the existing `test_p2_44_*` that asserts `"Class Foo"`, `"Trait Iterator"`, `"FIND ALL STRUCTS"` all classify as Structural.

#### AC-V1.30.1-3: BFS `bfs_expand` cap check misses the per-neighbor inner break, leaving `expansion_capped` false on edge boundary
- **Difficulty:** medium
- **Location:** src/gather.rs:357-381
- **Description:** Outer-loop cap check (line 345) and inner per-neighbor cap check (line 357) both set `expansion_capped = true`, but the *inner* `break` only exits the `for neighbor in neighbors` loop. The post-loop `if expansion_capped { break; }` (line 379) catches it for the outer queue loop. So far so good. But the **inner** check at line 357 fires *before* the `visited.contains(&neighbor)` test — so when the cap is reached mid-neighbor-expansion, the rest of the neighbors of the current node are dropped without giving an "already-visited" neighbor a chance to bump its score (the `existing.0 = new_score` branch at line 373). Effect: when the cap fires partway through a high-fanout node's neighbors, downstream nodes that *would* have had their `(score, depth)` bumped to a higher score keep their stale lower score. The post-expansion sort then under-weights those nodes. Determinism is preserved (the seed-sort + cap is stable), but the cap silently degrades the *quality* of the expansion at the boundary, not just truncating it.
- **Suggested fix:** Move the cap check to *after* the score-bump branch so already-visited neighbors still get their best-score update even when the cap is reached. Concretely: inside the for-loop, do `if !visited.contains(...) { ...insert with cap check... } else { ...always do the score bump, no cap check... }`. The cap is on `name_scores.len()`, not on score updates, so the relaxation is sound. Add a test where two seeds expand into a shared third node and the cap fires between them — the shared node should end up with the higher of the two scores, not whichever the BFS reached first.

#### AC-V1.30.1-4: `process_file_changes` resets `dropped_this_cycle` BEFORE embedder availability check — drops on embedder failure are silently lost
- **Difficulty:** medium
- **Location:** src/cli/watch/events.rs:139-156
- **Description:** Line 145 sets `state.dropped_this_cycle = 0` unconditionally before the function attempts `try_init_embedder` at line 154. If the embedder can't be initialised (line 156 `return`), the function exits leaving `pending_files` drained at line 132 (already gone) AND `dropped_this_cycle` zeroed, having processed no chunks. The subsequent snapshot publish reports `dropped_this_cycle = 0` while events from this cycle's drops are permanently lost. The watch loop will retry next cycle (because `pending_files` re-fills), but the *signal* that drops happened is gone — `cqs status --watch-fresh` and the `delta_saturated`-style operator log line never fire for the lost drops. This is adjacent to the already-reported "dropped_this_cycle reset before snapshot publish" finding, but the failure mode there was timing; this one is total signal loss when embedder initialization fails.
- **Suggested fix:** Move the `dropped_this_cycle = 0` reset to *after* the embedder-availability check. If `try_init_embedder` returns `None`, leave the counter alone — the next cycle's snapshot still surfaces the drop count, and the operator sees the truthful "events were dropped AND embedder is offline" state. Better yet: bookkeep a `dropped_pending_publish` field separate from `dropped_this_cycle` so the snapshot publish always sees an accurate figure regardless of reindex success.

#### AC-V1.30.1-5: `check_request` ambiguous-channel preference: cookie wins over query, leaks stale `?token=` in URL bar
- **Difficulty:** easy
- **Location:** src/serve/auth.rs:269-321
- **Description:** When a request arrives with BOTH a valid `cqs_token_<port>` cookie AND a `?token=<value>` query parameter (regardless of whether the query value is correct), the cookie path matches first and returns `AuthOutcome::Ok` — the redirect handler at line 360 (which strips the query token) is never reached. Effect: a user who bookmarks a URL with `?token=…` and later loads it on a tab that already has the auth cookie keeps the token in the URL bar permanently. This is exactly the SEC-7 leakage path the redirect mechanism was designed to close. The intended invariant ("token in URL → set cookie, redirect to clean URL") fails when a stale URL is reloaded. Test surface gap: every existing `check_request` test either uses ONE channel or pairs Bearer with cookie — none exercise "valid cookie + redundant query" to assert the redirect fires.
- **Suggested fix:** Reorder to check for `?token=` *first*; if present, validate it, set the cookie, and redirect — regardless of whether a cookie is also present. The cookie is a server-set HttpOnly + SameSite=Strict marker; trust it but strip the query param so the URL bar stops carrying live token material. Alternatively, after a successful Ok via cookie/Bearer, sniff for a `?token=` param and issue a redirect to strip it (without re-validating). Add a test: build a request with a valid cookie AND `?token=anything` → expect 302 to the clean URL.

#### AC-V1.30.1-6: `wait_for_fresh` deadline math allows one over-budget poll on a slow daemon
- **Difficulty:** easy
- **Location:** src/daemon_translate.rs:660-679
- **Description:** The polling loop is `poll → if fresh return → if now >= deadline return Timeout → sleep 250ms`. If `daemon_status` itself takes longer than `wait_secs` (e.g. on a slow / contended socket — `resolve_daemon_timeout_ms` defaults to 30s), the **first** poll can complete *after* the deadline yet still go through the not-fresh branch, then the deadline check trips. So the function can overshoot the budget by up to one daemon-timeout's worth (default 30s) on a heavily-loaded daemon, EVEN with `wait_secs = 0`. Caller (eval gate) sets a 600s budget which becomes effectively 600+30s = 630s. Not a correctness bug, but the docstring at line 646-648 promises "deadline of `wait_secs`" — the actual upper bound is `wait_secs + resolve_daemon_timeout_ms`. Adversarial test gap: a mock daemon that intentionally `thread::sleep(31_000)` before responding would surface this.
- **Suggested fix:** Either (a) check `Instant::now() >= deadline` *before* calling `daemon_status`, returning `Timeout(WatchSnapshot::unknown())` when the deadline is already past; or (b) document the actual upper bound in the docstring and the eval gate's "Timeout after N s" message — `(budget_secs, daemon_timeout_secs)` so users know "30s slack on a 600s budget" is by design. Option (a) is the cleaner fix since it makes the contract match the docstring.

#### AC-V1.30.1-7: `BoundedScoreHeap::push` score-equality uses `==` not `total_cmp` — safe today only because `is_finite()` filter precedes
- **Difficulty:** easy
- **Location:** src/search/scoring/candidate.rs:231
- **Description:** Line 231 reads `score > *worst_score || (score == *worst_score && id < *worst_id)`. The `==` between two `f32`s is `PartialEq`, which returns false for `NaN == NaN`. The struct's own `OrderedFloat` wrapper uses `total_cmp` precisely to make NaN comparable, but the eviction-comparison bypasses the wrapper. Today this is safe because line 213 filters `score.is_finite()` upstream, so NaN can't reach this code. But the wrapper exists for a reason; the inconsistency is a footgun — a future refactor that bypasses the `is_finite` filter (e.g. someone adds a "trust the caller" fast path) will silently corrupt the eviction invariant. The neighboring `into_sorted_vec` at line 251 also uses `total_cmp` consistently. The line-231 mix is the lone outlier.
- **Suggested fix:** Replace `score == *worst_score` with `score.total_cmp(worst_score) == std::cmp::Ordering::Equal`. Pin the invariant with a regression-pin debug-assert that NaN is unreachable here (since the filter is one branch up). The performance delta is zero — `total_cmp` is one inline integer compare on x86.

#### AC-V1.30.1-8: `last_event.elapsed().as_secs()` truncates sub-second freshness — `idle_secs` is 0 for the first second of a stale period
- **Difficulty:** easy
- **Location:** src/watch_status.rs:219
- **Description:** `idle_secs: input.last_event.elapsed().as_secs()` truncates sub-second elapsed time to whole seconds. For a watch loop ticking every 100ms (per the comment at line 64), the first 9 ticks after a file event report `idle_secs == 0` even though `last_event.elapsed()` has actually measured 100ms, 200ms, …, 900ms. A consumer polling `cqs status --watch-fresh --json` to gate "wait for index quiescence" can't distinguish "just-now event" from "990ms-ago event"; both report `0`. The status response's primary purpose is to decide "is the daemon idle enough to safely reindex?" — a millisecond field would be more honest. Compounding: `now_unix_secs()` at line 226 also truncates, so `snapshot_at` and `idle_secs` are both whole-second resolutions, but published at sub-second cadence.
- **Suggested fix:** Add `idle_ms: u64` (or expand `idle_secs` to `idle_millis`) so consumers polling for quiescence can react sub-second. Keep `idle_secs` for back-compat if any test pins it. Same for `snapshot_at` — emit `snapshot_at_ms: i64` in addition, so a consumer can compute snapshot age sub-second.

#### AC-V1.30.1-9: `daemon_socket_path` uses `DefaultHasher` — collision risk across projects in the same `XDG_RUNTIME_DIR`
- **Difficulty:** medium
- **Location:** src/daemon_translate.rs:174-200 (around)
- **Description:** Socket name uses `DefaultHasher` (SipHash 1-3 with fixed but Rust-version-dependent keys) of the project path, then formats into a fixed-length socket name. The hash output is u64 (truncated to fit the socket-name length budget). For a multi-tenant box running 10+ projects, a u64 collision is mathematically unlikely (~5×10⁻¹⁹), but Rust's docs explicitly say "do not rely on `DefaultHasher` for stable output across compiler versions." A `cargo update` of the std lib could change the hash output, breaking systemd `cqs-watch` services that hardcoded the previous socket name. This is a stability concern more than a collision concern, but the comment at line 170-171 ("not a security property") understates the cross-version-stability gotcha. Also, `DefaultHasher::new()` produces deterministic hashes within ONE run, but the comment about "collision avoidance" implies determinism — which is only guaranteed within a Rust version.
- **Suggested fix:** Use a stable hash (BLAKE3, SHA256, or even FNV-1a) over the canonical project path bytes, truncated to the socket-name length budget. The codebase already depends on `blake3` (used in reconcile.rs:330 for content hashing). `blake3::hash(path.as_os_str().as_bytes())` is one line, deterministic across Rust versions, and has a 32-byte output of which 8 hex bytes is plenty. Document the hash choice in the docstring so a future maintainer knows the socket name is stable across builds.

#### AC-V1.30.1-10: `incremental_count = 0` reset on idle-clear loses delta context for next rebuild threshold check
- **Difficulty:** medium
- **Location:** src/cli/watch/mod.rs:1180
- **Description:** When the watch loop has been idle for ~5 minutes (3000 cycles × 100ms), it clears the embedder session AND resets `state.incremental_count = 0` AND drops `state.hnsw_index`. The reset of `incremental_count` is the algorithm bug: this counter governs `hnsw_rebuild_threshold` triggering. After 5 minutes idle, the *actual* number of incremental inserts that have accumulated since the last full rebuild is unchanged — the chunks are still in SQLite, the HNSW on disk still has the corresponding nodes (or not, depending on dirty state). Resetting the counter to 0 means the next file event starts the threshold timer from scratch, *understating* delta size. So the next rebuild fires later than it should, leaving the HNSW index further behind SQLite than the threshold contract promises. Snapshot also reports `incremental_count: 0` immediately after idle-clear, which is misleading for `cqs status` consumers. The hnsw_index drop at line 1179 is reasonable (free GPU memory); the counter reset is an over-correction.
- **Suggested fix:** Don't reset `incremental_count` on idle-clear — only when an actual full rebuild swap completes. The counter's contract is "incremental inserts since last full rebuild", not "since last embedder-session reset". A regression-pin test would seed `state.incremental_count = threshold - 1`, simulate idle-clear, then add ONE file event and assert a full rebuild fires (not an incremental insert).
# Audit Findings — Extensibility (v1.30.1, 2026-04-28)

Audit mode: ON. Scope: open/closed violations, hardcoded values that should be table-driven, trait-shaped opportunities, multi-file surgery to add a single feature.

Triage state checked against `docs/audit-triage-v1.30.0.md` (no v1.30.1 triage file). Skipped: P2.88 (third score signal), P2.89 (IndexBackend trait — issue #1131 landed), P2.90 (ScoringOverrides resolver), P2.91 (NoteEntry tag taxonomy), P3.26 (`is_pipeable` registry — already fixed via macro), P3.27 (LlmProvider registry), P3.28-29 (fixed), P3.30 (structural_matchers), P3.31 (embedder preset extras hook).

---

## Extensibility

#### EX-V1.30.1-1: `daemon_ping` / `daemon_status` / `daemon_reconcile` are three near-identical 80-LOC copies — adding a fourth daemon RPC means cloning the boilerplate again
- **Difficulty:** medium
- **Location:** `src/daemon_translate.rs:271-356, 422-510, 537-621`
- **Description:** Each function does the same socket connect → set_read_timeout → set_write_timeout → write request line → read response line → parse envelope → check `status == "ok"` → extract `output` → unwrap dispatch payload → deserialize sequence. The only differences across the three copies are: (a) the `command` string in the request, (b) the `tracing::info_span!` name, (c) the deserialized type, and (d) the error tag in `unwrap_dispatch_payload`. The 5-second timeout, 64 KiB read cap, and 6+ `tracing::warn!` `stage=` arms are duplicated verbatim. Adding the next typed daemon RPC (e.g. `daemon_gc`, `daemon_invalidate`, `daemon_flush_caches`, or any of the audit's already-filed P3.27/EX work) requires another ~80-LOC copy with three find-and-replace points. Three is the threshold where the duplication becomes its own bug surface — when the timeout default changes, or when `unwrap_dispatch_payload` grows a new error path, you have three places to sync. It is also the shape EX-V1.29-1 (Commands trait, #1097) was filed for; this is the *daemon-client* side of the same problem.
- **Suggested fix:** Extract `fn daemon_request<T: DeserializeOwned>(cqs_dir, command: &str, payload_label: &str) -> Result<T, String>` taking the command name and the envelope's expected payload tag. The three public entry points become 3-line shims: `daemon_request(cqs_dir, "ping", "PingResponse")`. Centralizes timeout, cap, span, and warn `stage=` so a future `daemon_gc` is a one-line addition. Composes with `daemon_request_with_args` for the `Reconcile { hook, args }` shape.

#### EX-V1.30.1-2: `BatchCmd` dispatch is a 130-line hand-routed match — the same anti-pattern `is_pipeable` already escaped via macro
- **Difficulty:** medium
- **Location:** `src/cli/batch/commands.rs:503-636` (dispatch) vs `:372-447` (the macro-driven `is_pipeable` table)
- **Description:** Variant pipeability is now table-driven (issue #1137 fix, lines 372-447): adding a `BatchCmd` variant *requires* adding a `(Variant, bool)` row to `for_each_batch_cmd_pipeability!` or the build fails. But the `dispatch()` function 60 lines further down is still a hand-maintained match: `BatchCmd::Search { args, .. } => handlers::dispatch_search(...)`, `BatchCmd::Callers { args, .. } => handlers::dispatch_callers(...)`, etc. — 33 arms, one per variant. Adding a new `BatchCmd` variant means: (a) add the variant, (b) add the pipeability row (compile-enforced), (c) add the dispatch arm (NOT compile-enforced — the match has no `_` wildcard *now* but a future refactor that adds one would silently route new variants to nowhere), (d) write the handler. The dispatch arms are the largest still-hand-coded surface in the daemon; the `Commands trait` work in #1097 addresses the CLI side (`cli/dispatch.rs`) but leaves this. `Refresh` is also special-cased *outside* `dispatch()` in `dispatch_via_view` (line 1481) which compounds the surgery cost — a new "side-effect" command (e.g. `Reindex`, `Quarantine`, `Migrate`) requires touching `dispatch_via_view` *and* `dispatch` *and* the pipeability table.
- **Suggested fix:** Extend the macro table to carry the handler function pointer per row: `(Search, false, dispatch_search, args)`. The macro emits both `is_pipeable` and a single `dispatch_handler` function. Side-effect commands like `Refresh` get a third column flag (`needs_outer_lock`) so `dispatch_via_view` consults the same table instead of an `if matches!()` special case. Reduces the cost of a new command from "edit 4 places" to "edit 1 row + write handler".

#### EX-V1.30.1-3: `log_query` is hand-sprinkled across 6 dispatch arms — not a property of the command
- **Difficulty:** easy
- **Location:** `src/cli/batch/commands.rs:508, 531, 567, 577, 581, 603` (call sites of `log_query`)
- **Description:** `log_query("search", &args.query)`, `log_query("gather", &args.query)`, `log_query("onboard", &args.query)`, `log_query("scout", &args.query)`, `log_query("where", &args.description)`, `log_query("task", &args.description)`. Six identical-shape calls inside `dispatch()` arms. Adding a new query-shaped command (e.g. a hypothetical `cqs ask`) requires remembering to also call `log_query("ask", ...)`. The set of commands that *should* log is a static property of the variant (commands that take a free-text query, used by eval workflow capture) and belongs in the same table as `is_pipeable` — not at every individual dispatch site. Equally, *which* arg field carries the query (`args.query` vs `args.description`) is itself an inconsistency: `Where` and `Task` use `description`, the other four use `query`. Adding a new query-shaped command means: (a) add the dispatch arm, (b) remember to call `log_query`, (c) decide what the arg-struct field should be called. (b) and (c) silently differ today.
- **Suggested fix:** Extend the same dispatch macro proposed in EX-V1.30.1-2 with a `log_as: Option<&str>` column. The macro emits the `log_query` call automatically when `log_as` is `Some`. While there, normalize the arg-struct field name to `query` for query-shaped commands (or add a `fn query(&self) -> &str` accessor on the arg structs) so the macro can emit `log_query` without the per-variant field-name divergence.

#### EX-V1.30.1-4: `write_slot_model` clobbers all non-`[embedding]` keys on every write — adding any new slot metadata field is a breaking schema change
- **Difficulty:** medium
- **Location:** `src/slot/mod.rs:307-351` (and the doc lie at `:301-302`)
- **Description:** The function's doc comment claims "Existing TOML keys outside `[embedding]` are not preserved — slot.toml is owned by cqs" — i.e. *acknowledges* the issue and waves it away with "users shouldn't hand-edit it". The body confirms: line 326 emits `format!("[embedding]\nmodel = {}\n", ...)` and overwrites the file with that single section. The deserialize side at `:283-286` only reads `[embedding].model`. Practical extensibility cost: adding *any* future per-slot field (issue #1107 already filed for `--model` not being persisted on `slot create`; obvious candidates are `[reranker]`, `[splade]`, `[index].backend`, `[chunk].max_seq_len`, per-slot `[ignore]` overrides, etc.) requires (a) adding the field to `SlotConfigFile`, (b) extending `write_slot_model` to accept and serialize it, (c) renaming `write_slot_model` to something like `write_slot_config` because it now writes more than the model, (d) auditing every existing slot.toml since old files don't carry the new section. The "owned by cqs" hand-wave is the same shape that bites every "don't edit this file" config — eventually a feature wants user input or composition, and the round-trip-clobber design makes it expensive. Compounds with #1107: that bug is filed against `slot create --model`; *fixing* it via this code path would require this function to first do a read-modify-write rather than the current write-only.
- **Suggested fix:** Replace the hand-written `format!()` with a `toml_edit::Document` round-trip (a few lines via the `toml_edit` crate already in workspace dependencies, or a hand-rolled section-preserving edit). Rename `write_slot_model` → `update_slot_field` taking `(section, key, value)`, or expose a `SlotConfig` builder so each future field is one accessor. Either approach makes the next slot field zero-cost rather than a migration event.

#### EX-V1.30.1-5: `check_request` is a hardcoded three-channel `if/else` ladder — adding a fourth auth channel (mTLS, API key, JWT) requires touching the function plus its strip-token sibling
- **Difficulty:** medium
- **Location:** `src/serve/auth.rs:269-321` (check_request) + `:246-260` (strip_token_param) + `:323-332` (AuthOutcome enum)
- **Description:** `check_request` walks three explicit code blocks, in order: (1) `Authorization: Bearer …` header (lines 270-280), (2) `cqs_token_<port>` cookie (lines 287-300), (3) `?token=…` query param (lines 305-318). Each block has its own `for/loop`, its own `ct_eq` call, and its own success-return. Adding a fourth channel — say, an mTLS client cert (the SECURITY threat model already references this), an API key for headless CI, a session JWT for audit-trail integration, or an SSO bearer — requires: (a) a new code block in `check_request`, (b) potentially a new `AuthOutcome` variant if the channel needs special-case post-auth behaviour (the way `OkViaQueryParam` triggers a redirect), (c) a sibling helper if the channel needs sanitization (the way `strip_token_param` strips `?token=`), (d) explicit ordering decision (does mTLS supersede a bearer header?). Today's three channels also have a known leakage gap (`strip_token_param` doesn't case-fold or percent-decode — already reported in code-quality batch 1) that *should* be a property of the channel module, not a free function bolted onto the URI walk. The `AuthOutcome` enum is the natural seam here but it currently encodes "what to do post-auth", not "which channel matched", so it can't be inverted into a channel registry without enum churn.
- **Suggested fix:** Define `trait AuthChannel { fn check(&self, req: &Request) -> Option<ChannelMatch>; fn sanitize_request(&self, req: &mut Request); }` and an `AuthChannelRegistry` holding `Vec<Box<dyn AuthChannel>>` evaluated in priority order. Each existing channel becomes one impl (~30 LOC). The query-param channel's `sanitize_request` does the strip-token work. Adding mTLS/API key/JWT becomes "implement the trait, add to the registry constructor". The `AuthOutcome` enum collapses into `Option<ChannelMatch { needs_redirect: bool }>` — single decision point.

#### EX-V1.30.1-6: Reconcile fingerprint is `(path, mtime)` only — adding content-hash or size detection requires schema migration plus rewriting the divergence logic
- **Difficulty:** hard
- **Location:** `src/cli/watch/reconcile.rs:84-134` (run_daemon_reconcile) and `Store::indexed_file_origins` (returns `HashMap<String, Option<i64>>` — origin → stored mtime)
- **Description:** `run_daemon_reconcile` decides "is this file divergent?" by `disk > stored` mtime comparison only (line 124). Two well-known reconciliation bugs slip through this fingerprint: (a) **mtime collisions on coarse-grained filesystems** — the existing comment on `events.rs:85-100` explicitly notes WSL DrvFS / NTFS coarse mtime resolution causes ties to be skipped, and the workaround was a per-FS `mtime < last` vs `<=` toggle. The same coarse-mtime issue applies on disk-walk reconcile but isn't compensated. (b) **content-identical-but-mtime-bumped** files (formatter passes, `touch`, branch checkouts that restore the same content) are re-embedded for nothing — full embedder cost on every `git checkout`. The fingerprint shape is hardcoded into both the SQLite column choice (`indexed_files.last_indexed_mtime` is a single i64) and the in-memory map type (`HashMap<String, Option<i64>>`). Adding a content hash column means: (a) schema v23 + migration, (b) widen `indexed_file_origins`'s return type from `Option<i64>` to a struct, (c) update *every caller* of `indexed_file_origins` (the daemon GC sweep, the reconcile pass, any future tools), (d) extend reconcile's tuple-match arm to handle hash + mtime + size, (e) decide policy when only one of the three diverges. Today the design is anchored to a single-field stored mtime — every consumer assumes that shape.
- **Suggested fix:** Introduce a `FileFingerprint { mtime: Option<i64>, size: Option<u64>, content_hash: Option<[u8; 32]> }` struct returned from `indexed_file_origins`. Schema column lands as a nullable BLOB; mtime stays the cheap path, size/hash become opt-in via `cqs index --strict`. `run_daemon_reconcile` calls `disk.matches(stored, FingerprintPolicy::MtimeOrHash)` so the divergence policy is one place, not 5 callsites. Pays for itself the first time someone runs an audit pass that bumps mtimes on 4000 files.

#### EX-V1.30.1-7: Env-var falsy parsing (`"0" | "false" | "no" | "off"`) is hand-rolled in `eval/mod.rs` and not reused for the 30+ other CQS_* env vars
- **Difficulty:** easy
- **Location:** `src/cli/commands/eval/mod.rs:282-289` (`env_disables_freshness_gate`) — the falsy match is at line 286
- **Description:** `env_disables_freshness_gate()` is the *only* CQS env-var consumer that respects `false` / `no` / `off` — every other env var in the codebase (30+ call sites grepped: `CQS_NO_DAEMON`, `CQS_DISABLE_BASE_INDEX`, `CQS_FORCE_BASE_INDEX`, `CQS_TRUST_DELIMITERS`, `CQS_LLM_PROVIDER`, `CQS_TELEMETRY`, `CQS_TELEMETRY_REDACT_QUERY`, `CQS_CHAT_HISTORY`, `CQS_WATCH_RECONCILE`, `CQS_DAEMON_PERIODIC_GC`, `CQS_CAGRA_PERSIST`, ...) does its own ad hoc match: some compare to `"0"` only, some to `"1"`, some `as_deref() != Ok("0")`, some `as_deref() == Ok("1")`. Net effect: `CQS_NO_DAEMON=false` is silently ignored (matches `Ok(_)` ≠ `Ok("1")`); `CQS_WATCH_RECONCILE=off` is also ignored (treated as enabled). Operators have to know which env var accepts which falsy spelling — and there is no shared spec. Adding a new boolean env knob means picking one of the 4-5 inconsistent shapes and re-typing it. The `cqs doctor` output (`collect_cqs_env_vars`) just dumps key/value verbatim; it can't tell you whether your `CQS_NO_DAEMON=false` is being honoured.
- **Suggested fix:** Add `crate::env::truthy(name) -> bool` and `crate::env::falsy(name) -> bool` with one canonical falsy set (`"0" | "false" | "no" | "off"` lowercase) and one truthy set (`"1" | "true" | "yes" | "on"`). Audit the 30+ call sites and route all CQS_* boolean env reads through the helpers. Optional: register each known knob in an `ENV_REGISTRY` table (name, default, kind: Bool/Int/Path) so `cqs doctor --env-help` can list every knob, default, and parse status — replacing today's "set the var and hope the right code path matches your spelling" UX.

#### EX-V1.30.1-8: `Reranker` is a concrete struct with hardcoded ONNX-cross-encoder assumptions — no trait, no second implementation possible
- **Difficulty:** hard
- **Location:** `src/reranker.rs:108-167` (struct + ctor) + `:172-211` (rerank) + `:212-...` (rerank_with_passages)
- **Description:** `Reranker` bakes "ONNX session + tokenizer + token_type_ids feature-detection" directly into the type. The `rerank` and `rerank_with_passages` entry points return `Result<(), RerankerError>` where `RerankerError` is itself ONNX-shaped (`Inference(String)` from `ort::Error`). Adding any non-ONNX scoring family — an LLM-judge reranker via the existing `BatchProvider` trait, a BM25-on-content baseline for evaluation parity with classic IR systems, a dot-product reranker over a different embedding model entirely (e.g. a tiny code-trained model), or a no-op pass-through for benchmarking — requires touching every callsite that holds a `&Reranker` (search, search_filtered, daemon batch path, eval harness, doctor) and either adding an enum or duplicating each callsite per-implementation. The codebase already extracted `BatchProvider` for LLMs (`src/llm/provider.rs:42`) — it's the right shape for rerankers too. The friction here also blocks one of the cheap eval gains the project has already prototyped (LLM-as-reranker for the v3.v2 fixture top-50 has been discussed in tears) because there's no plug point. The known BERT-vs-RoBERTa input-shape divergence (lines 121-124, comments 341-355) is *itself* a within-implementation polymorphism leak — `expects_token_type_ids: Mutex<Option<bool>>` is the kind of state a cleaner trait split would put in the impl, not the trait surface.
- **Suggested fix:** Extract `pub trait Reranker: Send + Sync { fn rerank(&self, query: &str, results: &mut Vec<SearchResult>, limit: usize) -> Result<(), RerankerError>; fn rerank_with_passages(...); }`. The current struct becomes `OnnxReranker: Reranker`. Hold rerankers as `Arc<dyn Reranker>` everywhere — the existing Mutex-around-session pattern means `dyn` overhead is below the noise floor. Add `NoopReranker` (immediate use: eval harness ablations) and an `LlmReranker` skeleton that delegates to `BatchProvider`. The token-type-id feature detection becomes private to `OnnxReranker`.

## Platform Behavior

#### PB-V1.30.1-1: `cmd_serve` `--no-auth` warning misses `0.0.0.0` and `::` wildcard binds — the most exposed configurations
- **Difficulty:** easy
- **Location:** `src/cli/commands/serve.rs:27`
- **Description:** The "non-loopback + no-auth" warning fires on `--bind 192.168.1.5 --no-auth` (correct), but is silent for the two binds that expose the server to *every* interface: `0.0.0.0` and `::`. The check is `bind != "127.0.0.1" && bind != "localhost" && bind != "::1"` — passes wildcard strings unchanged. Wildcard binds also trigger the empty-allowlist code path in `allowed_host_set` (mod.rs:285) which logs a warn but doesn't block; combined with `--no-auth`, the operator gets an un-authenticated server reachable from any interface and only sees the (unrelated) wildcard-allowlist warning. The intent was "warn on anything that isn't loopback" — wildcard binds are *more* exposed than a concrete LAN IP and should warn at minimum.
- **Suggested fix:** Parse `bind` as `IpAddr` once, then warn on `!ip.is_loopback()`. Falls through cleanly to `localhost` / hostname strings (treat parse-fail as non-loopback). This also subsumes `0.0.0.0` and `::` because `Ipv4Addr::UNSPECIFIED.is_loopback() == false`.

#### PB-V1.30.1-2: `--bind localhost` documented as valid but always fails `SocketAddr::parse`
- **Difficulty:** easy
- **Location:** `src/cli/commands/serve.rs:39-41`, `src/cli/definitions.rs:842-849`
- **Description:** The `cmd_serve` no-auth gate at line 27 explicitly compares `bind != "localhost"`, suggesting `--bind localhost` is a supported value. But the very next line (`format!("{bind}:{port}").parse::<SocketAddr>()`) only accepts IP literals — `"localhost:8080"` errors with `invalid IP address syntax`. The user gets `Failed to parse localhost:8080 as a SocketAddr` after seeing CLI help that doesn't disclaim hostname binds. This is also inconsistent with the `cookie_name_for_port` cookie-jar comment in `src/serve/auth.rs:49-55` which name-checks `localhost`.
- **Suggested fix:** Either (a) resolve `localhost` to `127.0.0.1` (`ToSocketAddrs` lookup, take first IPv4) before parse, with a debug log noting the resolution, or (b) reject hostname binds explicitly with a clear error pointing at the `--bind 127.0.0.1` flag. Option (a) matches what users expect from every other web server CLI.

#### PB-V1.30.1-3: `process_exists` (Windows) substring-matches the literal `"INFO:"` against localized `tasklist` output
- **Difficulty:** medium
- **Location:** `src/cli/files.rs:59-72`
- **Description:** `tasklist /FI "PID eq <pid>" /NH` emits the localized string `INFO: No tasks are running which match the specified criteria.` when no process matches. The Windows-side stale-PID detection at `acquire_index_lock` (line 218) checks `!output.contains("INFO:")` — this is `true` (process "exists") on every locale where the prefix isn't literally `"INFO:"`. German (`INFORMATION:`), French (`INFORMATIONS:`), Japanese (`情報:`), and most non-English Windows installs surface a different prefix. On those systems a stale lock is *never* detected because `process_exists` always returns `true`, so the stale-lock retry loop at `files.rs:213-231` never fires and the user gets `Another cqs process holds the index lock at .../index.lock (PID 1234 may be stale)` even when PID 1234 is long dead. The user has to manually delete the lock file every time.
- **Suggested fix:** Don't parse human-readable output. Use the CSV format and check exit code / row count instead: `tasklist /FI "PID eq <pid>" /FO CSV /NH` — when no match exists the stdout is empty (or just a header line); when a match exists exactly one row is emitted. Alternatively call the Win32 `OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, ...)` via `windows-sys` — already in scope as a transitive of `notify` — which returns immediately without spawning a subprocess.

#### PB-V1.30.1-4: `open_browser` on WSL launches a Linux browser via `xdg-open` instead of the Windows-side default browser
- **Difficulty:** medium
- **Location:** `src/cli/commands/serve.rs:99-132`
- **Description:** WSL satisfies `cfg(target_os = "linux")` so the Linux branch fires `xdg-open <url>`. On a fresh WSL install there is no Linux GUI / browser, so `xdg-open` either errors with `xdg-open: no method available for opening 'http://127.0.0.1:8080/?token=...'` or hangs trying to launch a non-existent browser. The user sees `WARN: --open requested but failed to launch browser` and has to copy-paste the URL into Windows. The intended behavior on WSL is `cmd.exe /c start <url>` (or `wslview <url>` if `wslu` is installed) which hands the URL to the Windows default browser. `is_wsl()` already exists in `src/config.rs:47` and is used elsewhere — `open_browser` ignores it.
- **Suggested fix:** Branch on `is_wsl()` first inside the linux arm: try `wslview` (preferred — handles auth-token URLs cleanly), fall back to `cmd.exe /c start "" "<url>"` (note: `cmd.exe` works from WSL Linux because the interop layer translates), and only fall through to `xdg-open` on non-WSL Linux. A successful spawn on any of the three is enough.

#### PB-V1.30.1-5: `events.rs` mtime-equality skip is wrong on macOS HFS+ (1-second resolution) — only `is_wsl_drvfs_path` triggers the strict `<` check
- **Difficulty:** medium
- **Location:** `src/cli/watch/events.rs:85-102`
- **Description:** Comment claims "On Linux/macOS we keep the original `<=` because sub-second mtimes there are reliable and equality genuinely means same content". This is true for ext4 / APFS / btrfs. It is **false** for HFS+ on macOS (1-second mtime resolution) and for SMB/NFS shares mounted on macOS / Linux. Two saves of the same file within the same second produce identical mtimes; the watch loop sees the second save's mtime equal to `last_indexed_mtime` and skips the reindex. The user's last edit silently doesn't make it into the index. HFS+ is rare on modern Macs but still common on external drives, Time Machine restores, and any user on macOS < 10.14 (which kept HFS+ as default for spinning disks). The `is_wsl_drvfs_path`-only gate at line 95 misses every non-WSL coarse-mtime filesystem.
- **Suggested fix:** Change the predicate from "is WSL drvfs" to "is the cached mtime within `coarse_fs_resolution()` of `mtime`". Treat any cached value that's `<= 1s` away from `mtime` as ambiguous and force-reindex. Cheap conservative default; the cost is at most one redundant reindex on rapid re-saves on fine-grained filesystems, vs. silent missed reindexes on coarse ones.

#### PB-V1.30.1-6: `atomic_replace` opens the parent directory and calls `sync_all` on every Windows write — opens always fail, debug-spam on every persisted file
- **Difficulty:** easy
- **Location:** `src/fs.rs:90-108`
- **Description:** Step 3 (parent-dir fsync) is documented as "no-op on Windows because NTFS journals the rename" (lines 39-40), but the code unconditionally tries `std::fs::File::open(parent)`. On Windows, opening a directory as a `File` requires `FILE_FLAG_BACKUP_SEMANTICS` which Rust's `std::fs::File::open` does not pass. The open fails with `Access is denied. (os error 5)` for every directory on Windows. The function then logs at `debug!` level: `could not open parent dir for fsync after atomic_replace (non-fatal)`. Every config save, slot save, embedding-cache write, and SQLite WAL checkpoint emits one of these in `RUST_LOG=debug`. Not a correctness bug (we *want* the no-op on Windows) but the implementation does it the wrong way around — it tries-and-fails rather than skipping.
- **Suggested fix:** Wrap the parent-fsync block in `#[cfg(unix)]` and elide it entirely on Windows. The doc comment already promises this behavior. As a bonus the code is faster on Windows because we skip a syscall.

#### PB-V1.30.1-7: `cqs hook fire` on Windows can never reach the daemon — hook scripts always degrade to `.cqs/.dirty`, but no Windows daemon exists to consume the marker
- **Difficulty:** medium
- **Location:** `src/cli/commands/infra/hook.rs:309-335`, daemon at `src/cli/watch/mod.rs:593-608`
- **Description:** `cqs hook fire` (line 309) is `#[cfg(unix)]`-gated for the daemon path; on Windows it falls through to `.cqs/.dirty`. The daemon-startup path that promotes `.cqs/.dirty` into a one-shot reconcile (`watch/mod.rs:593-608`) only runs from `cqs watch --serve`, which itself requires Unix sockets and does not start on Windows-native (the watch loop's daemon thread is `#[cfg(unix)]` at `mod.rs:621`). So a Windows-native user who runs `cqs hook install`, then does `git checkout`, gets a `.cqs/.dirty` file that nothing will ever read — every reconcile mechanism is unix-gated. The user must manually run `cqs index` after every git operation, defeating the entire #1182 layer. `cqs hook status` will report `daemon_up: false` (line 367-370 is also `#[cfg(unix)]`) but `cqs hook install` happily writes the hooks regardless.
- **Suggested fix:** Either (a) refuse to install hooks on Windows-native with a clear error pointing at the `cqs index` workaround, or (b) make `cqs index` (the foreground reindex command) check for `.cqs/.dirty` at startup and consume it. (b) is the smaller change and gives Windows users equivalent functionality on next manual reindex. The hook script body's `command -v cqs` short-circuit at `hook.rs:246` already prevents broken hooks on hosts without cqs installed; just make the consumer side platform-portable.

#### PB-V1.30.1-8: `dispatch_drift` / `cqs hook install` write platform-native paths into the install report — `git_dir` field is `\`-separated on Windows but every other JSON consumer expects `/`
- **Difficulty:** easy
- **Location:** `src/cli/commands/infra/hook.rs:99-105, 152, 354`
- **Description:** `InstallReport`, `UninstallReport`, `StatusReport`, and `FireReport` all carry `git_dir: PathBuf` and `dirty_marker: Option<PathBuf>` and serialize via `serde_json` (line 408) which renders Windows `PathBuf` as `"C:\\Projects\\repo\\.git\\hooks"` (escaped backslashes). Every other JSON field in cqs that names a path uses `normalize_path()` / `normalize_slashes()` to emit forward slashes (per the audit fix at `src/store/types.rs:220` for `dispatch_drift`). The hook reports were not migrated. A consumer parsing the structured output (`cqs hook install --json | jq '.git_dir'`) gets `"C:\\Projects\\repo\\.git\\hooks"` on Windows but `"/home/user/repo/.git/hooks"` on Linux — string-equality and substring-matching against this output break.
- **Suggested fix:** Either (a) change `git_dir: PathBuf` to `git_dir: String` and store `cqs::normalize_path(&path)` at construction, or (b) wrap a serialize-with helper that calls `normalize_path` for every `PathBuf`-typed field on serialization. Match the convention already established in the rest of the JSON surface.

#### PB-V1.30.1-9: `reconcile.rs:128` inserts non-normalized `PathBuf` into `pending_files`, but the canonical key in `last_indexed_mtime` and the watch debounce queue compare against OS-native PathBufs from `notify` events — Windows path-separator skew possible
- **Difficulty:** medium
- **Location:** `src/cli/watch/reconcile.rs:103, 128`; cross-ref `src/cli/watch/events.rs:84, 109`
- **Description:** `enumerate_files` (lib.rs:578-688) returns paths via `dunce::canonicalize` then `strip_prefix(&root)` — on Windows these have backslashes. `reconcile.rs:103` and `:128` insert these into `pending_files: HashSet<PathBuf>`. The `notify` event path at `events.rs:84` does `path.strip_prefix(cfg.root)` on the watcher's own path — `notify` on Windows uses `ReadDirectoryChangesW` which produces backslash paths; on Linux native paths come back forward-slash. Both inserters end up agreeing on the same OS-native shape so the `HashSet` deduplicates correctly *today*. But the comment at `reconcile.rs:96-99` ("normalize to forward slashes for cross-platform matching parity with the rest of the store layer") is misleading — only the *origin lookup string* gets normalized; the `pending_files` key does not. If a future refactor on the events.rs side adds normalization (or a code path uses raw event paths without strip_prefix), the two queues will silently fragment on Windows: same file enqueued twice, double-reindex, double-mtime-update. There is no test that pins this contract.
- **Suggested fix:** Document the invariant at the top of `pending_files` declaration ("PathBuf keys are OS-native; do not normalize in producers") and add a debug-assertion at insertion sites that the key does not contain a backslash on `cfg(unix)` (catches accidental Windows-format leakage from a future change).

#### PB-V1.30.1-10: `restart_daemon_if_needed` Linux branch hardcodes the systemd unit name — a user-installed cqs without the systemd unit gets a confusing `Unit cqs-watch.service not found` after `cqs model use`
- **Difficulty:** easy
- **Location:** `src/cli/commands/infra/model.rs:710-738`
- **Description:** The Linux restart path runs `systemctl --user start cqs-watch` unconditionally. If the user did not install the systemd unit (running `cqs watch --serve` from a tmux pane, from a containerized init, from `nohup`, or with `runit`/`s6`/`launchd`-style supervisors), `systemctl --user start cqs-watch` fails with `Failed to start cqs-watch.service: Unit cqs-watch.service not found.` and the user gets `warning: failed to restart cqs-watch daemon (systemctl exited Some(5)). Run systemctl --user start cqs-watch manually.` — telling them to run the same command that just failed. The macOS branch at line 745 spawns the binary directly, which is the platform-portable behavior the Linux branch should fall back to when systemctl reports `Unit not found`.
- **Suggested fix:** On Linux, when `systemctl --user is-enabled cqs-watch` (cheap probe) reports the unit isn't loaded, fall back to spawning `cqs watch --serve` directly — same path the macOS branch takes. The probe can be done once and cached per process. Alternatively check `was_running` provenance (was the daemon started by systemctl, or by direct spawn?) and route the restart accordingly.
## Security

#### SEC-V1.30.1-1: SECURITY.md falsely claims `cqs read --focus` and `cqs context` carry `trust_level` / `injection_flags`
- **Difficulty:** medium
- **Location:** `SECURITY.md:14` (mitigation list) vs `src/cli/commands/io/read.rs:312-323` (`FocusedReadJsonOutput`) and `src/cli/commands/io/context.rs:269-325` (`full_to_json`, `compact_to_json`, `summary_to_json`)
- **Description:** SECURITY.md "Indirect Prompt Injection" section asserts that *every* chunk-returning JSON output (`search`, `gather`, `task`, `scout`, `onboard`, **`read`**, **`read --focus`**, **`context`**, `similar`) carries `trust_level: "user-code" | "reference-code"` and a per-chunk `injection_flags: []` array. Verification:
  - `FocusedReadJsonOutput` (read.rs:312) has fields `{focus, content, hints, warnings}`. No `trust_level`, no `injection_flags`.
  - `full_to_json` / `compact_to_json` / `summary_to_json` build `FullChunkEntry` / similar from `data.chunks` and emit `{name, chunk_type, signature, line_start, line_end, doc, content}` — again no `trust_level`, no `injection_flags`.
  - `cqs read` (non-focus) prints text-only via `cmd_read`; the JSON branch (`emit_json(&result)` in read.rs:369) emits the same structure with no trust tagging.
  - The only sites that *do* tag are `display_unified_results_json` (search), `gather` (explicit ref-name threading), and the `tag_user_code_trust_level` walker used by `scout`/`onboard` (mod.rs:216) — and that walker only walks specific shapes (`entry_point`, `call_chain[]`, `callers[]`, `file_groups[].chunks[]`).
- **Impact:** Per the team's "Docs Lying Is P1" rule, this is a correctness bug. Agents that filter on `trust_level` or `injection_flags` (per the public mitigation contract) will silently process `read --focus` / `context` payloads as untagged blobs and either misclassify them or reject them outright. Worse: the SECURITY.md narrative leads operators and downstream agent designers to *trust the contract* — but `read --focus` content can include reference-store dependencies surfaced through `get_types_used_by` + `search_by_names_batch`, so the lack of tagging matters semantically too.
- **Suggested fix:** Either (a) extend `FocusedReadJsonOutput` and the context outputs to carry `trust_level` (always `"user-code"` for the project store path) plus an `injection_flags` array per emitted chunk-shaped sub-object, OR (b) remove `read`, `read --focus`, and `context` from the SECURITY.md mitigation list and explicitly document that those surfaces are exempt. Option (a) is the minimum-surprise fix and matches the rest of the contract.

#### SEC-V1.30.1-2: SECURITY.md "Symlink Behavior" section directly contradicts the actual indexer (and SECURITY.md's own "Mitigations" line)
- **Difficulty:** easy
- **Location:** `SECURITY.md:203-215` ("Symlinks are followed, then the resolved path is validated"; allows `project/link → project/src/file.rs`) vs `src/lib.rs:601` (`enumerate_files` uses `WalkBuilder::follow_links(false)`) and `SECURITY.md:162` ("Symlink filtering: Symlinks are skipped during directory walks and archive extraction").
- **Description:** The "Symlink Behavior" matrix promises that an in-tree symlink (`project/link → project/src/file.rs`) is allowed and an out-of-tree symlink (`project/link → /etc/passwd`) is blocked after canonicalization. Reality: `cqs::enumerate_files` — the only call path used by `cqs index`, `cqs ref add`, and the watch reconcile (`src/cli/watch/reconcile.rs:74`, `src/cli/watch/gc.rs:127,209`) — sets `follow_links(false)` and silently skips every symlink, regardless of target. Two lines above, SECURITY.md correctly states "Symlinks are skipped during directory walks." The two sections cannot both be true.
- **Impact:** Operators who read the matrix expect indexable in-tree symlinks (e.g. monorepo workspaces that symlink common code) and will silently get a partial index — surprise stale results, no error message. Worse, the doc tells a contributor reading the threat model to test the wrong invariant ("symlinks resolved → validated" instead of "symlinks dropped"), which can mask a real regression that *does* start following links.
- **Suggested fix:** Replace the "Symlink Behavior" matrix with the truth: directory-walk paths (`enumerate_files`, watch reconcile, `cqs convert`) drop symlinks entirely; only `cqs read <explicit-path>` and `cqs ref add` source-arg canonicalization follow symlinks (and reject targets outside the project root). The current matrix can be the example for the read/canonicalize path; the walk path needs its own row.

#### SEC-V1.30.1-3: `callgraph-3d.js` interpolates `e.message` into `innerHTML` without `escapeHtml` — SEC-2 fix gap
- **Difficulty:** easy
- **Location:** `src/serve/assets/views/callgraph-3d.js:55` (`container.innerHTML = ...3D bundle failed to load: ${e.message}...`) vs `src/serve/assets/views/cluster-3d.js:98` and `src/serve/assets/views/hierarchy-3d.js:75` (both use `${escapeHtml(e.message)}`).
- **Description:** The SEC-2 hardening pass landed `escapeHtml` mirrors inside the cluster-3d and hierarchy-3d IIFEs (both files even carry an explicit comment: "Local XSS guard: this IIFE can't reach app.js's escapeHtml, so mirror it here for any server-derived string interpolated into innerHTML"). `callgraph-3d.js` has no `escapeHtml` mirror at all, and its bundle-load-failure handler interpolates `e.message` raw. While `e.message` is normally a static-ish JS error string, the bundle loader (`window.cqsEnsureThreeBundle`) can be made to throw a custom `Error` whose message embeds attacker-controlled response text — e.g. a fetch error against a CDN that the server doesn't control, or a future code path that surfaces server response bodies in the message.
- **Impact:** Stored-XSS class regression on the server-side error path. Defence-in-depth value: SEC-2 already established the contract that every `${...}` going into `innerHTML` is escaped — this one site silently violates it, and a future cluster/hierarchy author copy-pasting from callgraph would propagate the gap.
- **Suggested fix:** Mirror the `escapeHtml` helper at the top of `callgraph-3d.js` (matching cluster-3d.js:21 / hierarchy-3d.js:19) and wrap `e.message` at line 55. The parallel error path at lines 62-66 is a static template — fine as-is.

#### SEC-V1.30.1-4: `tag_user_code_trust_level` is shape-coupled — silently no-ops on unknown JSON shapes, leaving chunks untagged
- **Difficulty:** medium
- **Location:** `src/cli/commands/mod.rs:216-257`
- **Description:** `tag_user_code_trust_level` is the implementation backing the SECURITY.md trust contract for scout/onboard/where/plan. It walks four hardcoded shapes:
  1. `entry_point` (object)
  2. `call_chain[]`
  3. `callers[]`
  4. `file_groups[].chunks[]`

  Any chunk-shaped object outside those four containers (e.g. a future `dependents[]`, `examples[]`, `nested_callees[]`, or a top-level `chunks[]`) is silently emitted with **no `trust_level` field**. There is no schema check, no test that asserts every emitted chunk has `trust_level`, and no fallback "tag any object that has both `name` and `file`" walker.
- **Impact:** The contract is "every chunk-returning JSON output carries `trust_level`," but the implementation is "every chunk in one of these four arrays carries `trust_level`." A contributor adding a new top-level field that holds chunks (very plausible — `onboard` is built specifically to grow new graph-shaped sections) will ship a fully-untagged chunk surface and the test suite won't catch it. Compounds with SEC-V1.30.1-1: scout/onboard's tagging coverage is more fragile than the docs claim.
- **Suggested fix:** Replace the hand-rolled walker with a recursive visitor: for every JSON object encountered, if it carries the chunk-shape signature (e.g. `{name, file, line_start, ...}` and the chunk is from a project-store path), tag it. Alternatively, push the tagging into the result-builder layer (the structs that `serde::Serialize` into chunk JSON), so the field is structurally inseparable from the data. Add a test that dumps the emitted JSON for each scout/onboard sub-shape and asserts every chunk-shaped sub-object has `trust_level`.

#### SEC-V1.30.1-5: Search results without `--include-refs` always emit `trust_level: "user-code"` — but the user's project store can itself include third-party content via `cqs notes mention` or vendored sources committed to the tree
- **Difficulty:** hard
- **Location:** `src/store/helpers/types.rs:172-196` (`to_json_with_origin`) + `src/cli/commands/search/query.rs:497-519`
- **Description:** When `--include-refs` is not set (the default), `query.rs` short-circuits to `display_unified_results_json` which calls `r.to_json()` (helpers/types.rs:161) → `to_json_with_origin(None)` → unconditional `"trust_level": "user-code"`. The "user-code" claim is structural: it tracks "did this come from a `cqs ref` reference index", not "is this code authored by the project owner." Vendored/copied third-party code committed into the project tree, or content surfaced by `cqs notes` from `docs/notes.toml` (which can ship in a cloned repo per SECURITY.md's own "shared notes" surface), all emit as `user-code` despite being exactly the indirect-prompt-injection surface the trust-level field exists to flag.
- **Impact:** Agents that adopt the documented posture ("treat `reference-code` as untrusted, `user-code` as more trusted") get a false sense of security on the most common attack surface — committed third-party content in the project tree. SECURITY.md explicitly calls out vendored upstream content as a payload vector ("committed by a contributor or embedded by an upstream dependency") but the trust-level signal cannot distinguish vendored from authored code; both are "user-code." The first-encounter shared-notes gate (#1168) partially mitigates the notes path but does not change the emitted `trust_level` after acceptance.
- **Suggested fix:** Two options: (a) Document the limit explicitly in SECURITY.md — `trust_level: "user-code"` means "from the user's project store," not "authored by the user" — and add a note that vendored/upstream content inside the project tree retains `user-code`. (b) Add a path-prefix denylist (e.g. `vendor/`, `third_party/`, `node_modules/`) at index time, persist a per-chunk `vendored: bool`, and have `to_json_with_origin` downgrade to `"third-party-code"` for vendored chunks. Option (a) is the cheap fix; option (b) is the proper one.

#### SEC-V1.30.1-6: `cqs ref add` accepts a local source path with no symlink-target audit — references can index outside their declared source root via in-tree symlinks
- **Difficulty:** medium
- **Location:** `src/cli/commands/infra/reference.rs:130-150`
- **Description:** `cmd_ref_add` canonicalizes the `--source` path (`dunce::canonicalize(source)`) but then hands the *canonicalized* root to `enumerate_files(&source, ...)`. The walk uses `follow_links(false)` so symlinks at the *root* are followed only to the extent that `dunce::canonicalize` resolved them — which is fine — but the indexed corpus is whatever lives at the resolved root. There is no audit comparing "the source path the user typed" against "the canonical root that gets indexed." A user who runs `cqs ref add foo /home/me/projects/foo` against a path that is itself a symlink to `/some/other/dir/that/contains/proprietary-code/` will silently get an index of the *other* dir. Nothing flags the redirection. The reference path is then persisted into `.cqs.toml` as `source = <canonical-resolved-path>`, so a follow-up `cqs ref list` shows the resolved path — but if the user's mental model is "I added /home/me/projects/foo," the reindex behavior is surprising.
- **Impact:** Low-severity but worth a note. The bigger issue is operators who symlink `vendored-monorepo-pull/` → `~/work/customer-A-private/` to "test cqs ref" and silently end up with a customer-content reference index. Also relevant if `cqs ref add` is used inside CI/automation that takes a `--source` from a config file controlled by a less-trusted contributor.
- **Suggested fix:** Compare `source` (user input, after `Path::new`) to `dunce::canonicalize(source)`; if they differ, log a `tracing::warn!` (and emit a JSON `warnings` field on `cmd_ref_add`'s return) telling the user the symlink was resolved before indexing. Alternative: refuse to index symlinked roots without a `--allow-symlink-source` opt-in.

#### SEC-V1.30.1-7: `LocalProvider` follows up to 2 redirects on `Authorization`-bearing requests — bearer token can leak across origins on misconfigured local LLM endpoints
- **Difficulty:** medium
- **Location:** `src/llm/local.rs:124-129` (`Policy::limited(2)`) + `src/llm/local.rs:435-437` (`Authorization: Bearer <key>` header)
- **Description:** P2.36 aligned the production redirect policy with the doctor probe (`Policy::limited(2)`). The change rationale comment claims "Same-origin HTTP→HTTPS redirects on bind-localhost are benign" — but `Policy::limited(2)` does *not* restrict to same-origin. A misconfigured `CQS_LLM_API_BASE` (e.g. a load-balancer that 302s from `http://internal-llm/` → `http://attacker-controlled-origin/v1/chat/completions`) will see the request follow the redirect, and `reqwest`'s default behavior of stripping `Authorization` *only on cross-origin redirects* assumes you're on the latest reqwest with the strip-on-redirect default. cqs's reqwest version pin and policy interaction should be verified. (cqs uses reqwest 0.12.x, which does strip `Authorization` cross-origin by default — but the strip is silent and operators may not notice the bearer was stripped, leading to an infinite-401 loop instead of a clean fail-fast.)
- **Impact:** If reqwest's strip-on-redirect remains default-on, this is observability-grade: silent 401 instead of a clear "redirect to other origin, bearer stripped" warning. If a future reqwest bump opts back into propagating the header, this becomes a credential-leak path. Either way, the assumption "same-origin only" baked into the comment is wrong — the policy doesn't enforce it.
- **Suggested fix:** Pin behavior with a custom `redirect::Policy::custom` closure that explicitly checks `previous.last().origin() == new.url().origin()` and otherwise rejects. Logs a `tracing::warn!` on cross-origin attempts so operators see misconfigured endpoints. The doctor probe should match.

#### SEC-V1.30.1-8: Daemon env snapshot logs every `CQS_*` variable at info — `CQS_LLM_API_KEY` lands in the journal
- **Difficulty:** easy
- **Location:** `src/cli/watch/mod.rs:529-532`
- **Description:** On daemon startup, the code iterates *every* env var starting with `CQS_` and logs them via `tracing::info!(cqs_vars = ?cqs_vars, "Daemon env snapshot")`. The list is not redacted. `CQS_LLM_API_KEY` (read in `src/llm/local.rs:110`) is one of the env knobs that flows through this snapshot if set. Other secret-bearing vars include any future `CQS_*_API_KEY`, `CQS_*_TOKEN`, `CQS_*_PASSWORD`. With OB-V1.30-1 fixing the default subscriber to surface info-level events to the journal, every daemon start now writes the API key into systemd-journald's 30-day retention.
- **Impact:** Same class as P1.13 (auth token printed to stdout, captured by journald): a secret meant for in-memory use becomes a 30-day journal artifact. This one is worse because it lands automatically every daemon restart, not just on first launch.
- **Suggested fix:** Maintain a `SECRET_PREFIXES` list (`API_KEY`, `TOKEN`, `PASSWORD`, `SECRET`) and substitute logged values with a `<redacted len=N>` placeholder. Better: switch the snapshot to log *names only* (the keyset is the operationally interesting part — values are visible via `systemctl show-environment`).

#### SEC-V1.30.1-9: `cqs ref add` storage dir created with `0o700` but the parent (`~/.local/share/cqs/refs/`) inherits umask — sibling references can be world-readable on permissive umask
- **Difficulty:** easy
- **Location:** `src/cli/commands/infra/reference.rs:137-145`
- **Description:** The code does `std::fs::create_dir_all(&ref_dir)` and then `set_permissions(&ref_dir, 0o700)`. `create_dir_all` creates *every* missing component, but only the final component gets chmod-ed to `0o700` afterward. The parent (`~/.local/share/cqs/refs/`) is created with default umask (`0o755` on most systems). On a multi-user box, a sibling user can `ls ~user1/.local/share/cqs/refs/` and enumerate every reference name, and — if the original creating umask was `0o022` and a future ref's directory is created without the explicit chmod (or before it lands) — read the index DB. The `0o700` on the final directory is correct; the gap is the parent.
- **Impact:** Reference-name leakage at minimum (an attacker on the same box knows you've indexed `customer-secret-codebase`). The rapidsai/private-repos issue from SECURITY.md becomes worse: a multi-user developer box leaks the *list* of customer code that's been referenced even if the indexes themselves stay private.
- **Suggested fix:** Walk every parent component the call may have created and chmod each to `0o700` (or, for `~/.local/share/cqs/`, `0o755` is fine — but `~/.local/share/cqs/refs/` should be `0o700`). Alternatively, set the process umask to `0o077` for the duration of the create_dir_all call, then restore (mirroring the SEC-D.6 socket pattern at watch/mod.rs:496).

#### SEC-V1.30.1-10: `cqs ref add` does not set `0o600` on the index DB after writing — falls back to umask-default permissions
- **Difficulty:** easy
- **Location:** `src/cli/commands/infra/reference.rs:165-178` (Store::open + run_index_pipeline)
- **Description:** After `cmd_ref_add` chmods the *containing directory* to `0o700`, it opens a SQLite store at `db_path` (`ref_dir.join(INDEX_DB_FILENAME)`) via `Store::open`. SQLite creates the file at default umask (`0o644` on most systems). The directory is `0o700` so a sibling user can't enter it — but if the user later changes the directory permissions (e.g. for a backup tool) or if the ref dir is on a network filesystem that ignores Unix permissions, the file itself is world-readable. There's no explicit `set_permissions(0o600)` on the DB after creation. Compare `model_fingerprint`/`model.toml` at SEC-19 (export-model) which *does* set `0o600`.
- **Impact:** Defence-in-depth gap. The directory chmod is the primary defence; this is the secondary layer that's missing. On NFS / SMB / overlay filesystems where directory perms are advisory and file perms are authoritative, the gap matters. On a normal local ext4 filesystem with `0o700` parent, exposure is minimal.
- **Suggested fix:** After `Store::open(...)` succeeds in `cmd_ref_add`, walk every file in `ref_dir` and chmod to `0o600`. Match the pattern used in `cqs export-model` for `model.toml`.
# Audit Findings — Data Safety (v1.30.1, 2026-04-28)

Audit mode: ON. Scope: data corruption, validation, schema migrations, races, deadlocks, thread safety, transaction integrity, atomic-write patterns, fsync placement, lock ordering. Triage state: cross-checked against `docs/audit-triage-v1.30.0.md` (DS-V1.30 series — migration backups, fsync placement, slot TOCTOU — all fixed or filed). v1.30.1 introduces `#1182` watch reconcile + `WatchSnapshot` surface; this audit focuses on those new code paths plus what was deferred.

---

## Data Safety

#### `cqs index --force` reopen leaves stale `pending_rebuild` against orphaned store handle
- **Difficulty:** medium
- **Location:** `src/cli/watch/mod.rs:1102-1122` (DS-W5 reopen)
- **Description:** When the watch loop detects `cqs index --force` rotated `index.db` (line 1106 `db_id` mismatch), it drops the old `Store`, opens a fresh one, and clears `state.hnsw_index = None; state.incremental_count = 0`. It does NOT reset `state.pending_rebuild`. If a background HNSW rebuild was in flight when the rotation happened, two failure modes follow:
  1. The rebuild thread opened its own readonly pool via `Store::open_readonly_pooled(&index_path)` (rebuild.rs:240) — it transparently sees the NEW DB content. Good.
  2. But `pending_rebuild.delta` was populated by the watch loop's `process_file_changes` calls *before* the rotation, with `(id, embedding, content_hash)` triples whose `id` strings reference the OLD DB's chunk IDs. After the rotation, the same id strings may collide with NEW DB chunks that have completely different content. When `drain_pending_rebuild` replays the delta on swap (rebuild.rs:325+), it applies stale embeddings against new IDs. The new HNSW carries the wrong vectors for those IDs until the next threshold rebuild reads SQLite fresh.
  3. Worse: `delta_saturated` may be inappropriately triggered, causing the rebuilt index to be *thrown away* (rebuild.rs:60-63) even though a clean rebuild against the new DB was perfectly valid. Operator sees `delta_saturated=true` warnings on every threshold rebuild post-rotation until the next `cqs index --force`.
- **Suggested fix:** In the DS-W5 reopen block, also `state.pending_rebuild.take()` (drop the channel + abandon the in-flight build's outcome). The rebuild thread's `tx.send(result)` becomes a no-op when the receiver is dropped (rebuild.rs:289 already comments "Receiver may have been dropped if the daemon shut down — that's fine"). Force a fresh rebuild on the next threshold tick against the new DB.

#### `run_daemon_reconcile` bypasses `max_pending_files()` cap — drowns the queue on bulk branch switch
- **Difficulty:** medium
- **Location:** `src/cli/watch/reconcile.rs:103, 128` (the two `pending_files.insert` sites)
- **Description:** The inotify ingest path at `events.rs:108` enforces `pending_files.len() < max_pending_files()` before inserting and increments `dropped_this_cycle` when the queue is full (line 115). This is the documented backpressure. `run_daemon_reconcile` blindly inserts every divergent file into `pending_files` with no cap check. On a `git checkout` of a sibling branch with 50k file changes (or a large monorepo's first-time index population), reconcile pushes the queue size to 50k. Two concrete consequences:
  1. The queue is now well past `max_pending_files()` (default 5000). The next inotify event hits `events.rs:108`, sees `len() >= max`, increments `dropped_this_cycle` — but the count of "events dropped" is now meaningless because the cap is being held above the line by reconcile, not by sustained event pressure. Operators see persistent `dropped` warnings that have nothing to do with what reconcile actually did.
  2. The next `process_file_changes` tries to drain 50k files in one batch — `pending_files.drain().collect()` allocates a single 50k-element `Vec<PathBuf>`, then `reindex_files` parses + embeds all 50k synchronously while holding the index lock. The whole point of `max_pending_files()` is to bound the per-cycle work; reconcile defeats it.
- **Suggested fix:** Pass `max_pending_files()` into `run_daemon_reconcile` and stop inserting once the queue hits the cap. Track skipped count locally and surface it as a `tracing::warn!` so the next reconcile pass picks them up. Alternatively, drain reconcile-queued files in chunks (`take(cap - existing)` per cycle) so the watcher's drain semantics stay intact.

#### Periodic reconcile reads through stale `store` handle on `cqs index --force` race
- **Difficulty:** medium
- **Location:** `src/cli/watch/mod.rs:1262-1283` (periodic reconcile call)
- **Description:** The periodic reconcile path skips the `db_file_identity` check that `should_process` does at line 1105. If `cqs index --force` rotated the DB while the watch loop was idle, periodic reconcile fires, calls `run_daemon_reconcile(&store, ...)` against the stale store handle (orphaned inode), and reads `indexed_file_origins()` + `source_mtime` from the OLD DB. Files that got newer mtimes in the NEW DB look "stale" against the old store; reconcile queues them as MODIFIED. The next `should_process` cycle reopens the store, drains those queued paths, and re-embeds files that were already current in the new DB. Worst case isn't corruption — it's silent re-work that defeats `--force`'s "I just rebuilt cleanly, you can stand down" semantics.
- **Suggested fix:** Either (a) add a `db_file_identity` check before each periodic reconcile call and reopen the store if it changed (matching the pattern at line 1105), or (b) acquire a *shared* (read) index lock during reconcile so it serializes against `cqs index --force`'s exclusive lock. Option (a) is the smaller change.

#### `slot remove` does not check whether a daemon is actively serving the slot it deletes
- **Difficulty:** medium
- **Location:** `src/cli/commands/infra/slot.rs:322-369`
- **Description:** `slot_remove` holds `acquire_slots_lock` across `read_active_slot → list_slots → remove_dir_all`, which prevents concurrent CLI invocations from racing each other. But the lock doesn't bind a *daemon* that's already serving from `slots/<name>/index.db`. Concrete failure: `cqs watch --serve --slot foo` is running (holds a long-lived `Store::open` against `slots/foo/index.db` + an HNSW Arc + ~500MB ONNX session). Operator runs `cqs slot remove foo --force`. The CLI acquires the slots lock (the daemon doesn't hold it — it isn't a slot-lifecycle operation), passes the existence check, calls `fs::remove_dir_all(&dir)`. On Linux the unlink succeeds because the daemon's open file descriptors keep the inodes alive — but:
  - The daemon's WAL checkpoint on next write hits an unlinked-but-open inode; checkpoints work but the rebuilt HNSW persists into the (now-detached) directory tree, which is reaped on daemon exit. Hours of incremental rebuild work vanish silently.
  - The daemon's BatchContext serves stale snapshots forever — no path notices the directory disappeared until restart.
  - `fs::remove_dir_all` on `index.db-wal` / `index.db-shm` while another process is mmap'ing them is undefined per SQLite docs (the project's MEMORY.md and the v1.30.0 P2.62 fix document the inverse case). If WSL or any non-overlay FS surfaces an EBUSY or partial removal, the user gets a half-deleted slot dir and no rollback.
- **Suggested fix:** Before `remove_dir_all`, check `daemon_status(project_cqs_dir)` (the same probe `cqs hook status` uses). If a daemon is up *and* its active slot equals `name`, refuse with a clear error: `bail!("daemon is currently serving slot '{name}'. Stop it first: systemctl --user stop cqs-watch")`. With `--force`, downgrade to a `tracing::warn!` and proceed (operator opt-in). Mirrors the existing `--force` semantics for "this is the active slot" at line 357.

#### `.cqs/.dirty` fallback marker write is not atomic — partial create + crash leaves daemon's existence check ambiguous
- **Difficulty:** easy
- **Location:** `src/cli/commands/infra/hook.rs:329-332`
- **Description:** The hook fallback writes the dirty marker with `std::fs::write(&dirty, b"")` — a non-atomic open(O_CREAT|O_WRONLY|O_TRUNC) + write + close. Three failure modes:
  1. If `cqs hook fire` is killed between `open()` returning and `write()` completing (extremely small window for a 0-byte write but non-zero on slow filesystems), the directory entry exists but with no inode commit. On daemon startup (`watch/mod.rs:594`), `dirty_marker_path.exists()` returns true, the daemon promotes a reconcile, then `remove_file` succeeds — fine. Low risk.
  2. The bigger gap: if the parent `.cqs/` does not exist, line 330 creates it. If a concurrent process is *also* running `cqs hook fire` and the dirty file already exists with content, the second writer's `O_TRUNC` empties it. No locking, no atomicity, but the daemon only checks existence so this is benign. Acceptable but worth a note.
  3. The actual concern: there's no fsync after the write (line 331). On a crash + power loss between the hook's `fs::write` and the next fsync of `.cqs/`, the file's directory entry can be lost from the parent directory's metadata — the daemon never sees the marker, the reconcile request is silently dropped. Since `cqs hook` exists *specifically* for the case where the daemon was offline (so the marker is the *only* signal), losing the marker means a `git checkout` post-reboot won't trigger the reconcile that would have caught the bulk diff.
- **Suggested fix:** Replace `std::fs::write(&dirty, b"")` with `crate::fs::atomic_replace` (the helper already used by `notes.toml`, `audit-mode.json`, and the slot writers post-#P3.39). It writes the temp, fsyncs the file, renames atomically, and fsyncs the parent dir. For an empty file the cost is negligible and the durability guarantee aligns with the marker's role as a crash-recovery breadcrumb.

#### `WatchSnapshot.delta_saturated` is published but `compute()` ignores it — `--require-fresh` accepts a doomed rebuild as `Rebuilding`
- **Difficulty:** easy
- **Location:** `src/watch_status.rs:199-209` (state-machine), `src/cli/watch/mod.rs:160-164` (publish)
- **Description:** `WatchSnapshotInput.delta_saturated` is plumbed through and serialized on the wire (`watch_status.rs:84-87`), but `compute()` never consults it when picking `FreshnessState`. When a rebuild's incremental delta saturates `MAX_PENDING_REBUILD_DELTA` (rebuild.rs:60-63), the rebuilt HNSW is *discarded* on swap — operators get a `tracing::warn!` line, but the snapshot still classifies as `Rebuilding`. Once the doomed rebuild's `drain_pending_rebuild` runs, `pending_rebuild` becomes `None`, snapshot flips to `Fresh` even though no actual swap happened — the on-disk HNSW is whatever was there before the rebuild started. `cqs eval --require-fresh` accepts this as fresh.
  This is a duplicate of a finding already filed under Code Quality (`audit-findings.md` "delta_saturated flag is published but ignored by FreshnessState::compute"). I'm pinning it here too because the data-safety angle is distinct: the saturation case is the one where the rebuild silently failed and the fresh-gate is the *exact* contract that's supposed to prevent eval against a stale index.
- **Suggested fix:** In `compute()`, treat `delta_saturated == true && rebuild_in_flight == false` as `Stale` (the rebuild already discarded, no in-flight work, but the next threshold rebuild hasn't fired yet — caller should wait). Drop the field from the wire shape if it's not going to drive state.

#### HNSW rollback path leaves `.bak` files orphaned when restore-rename fails
- **Difficulty:** medium
- **Location:** `src/hnsw/persist.rs:509-553` (rollback block)
- **Description:** The rollback path (after `atomic_replace` fails mid-rename) iterates `all_exts` and calls `std::fs::rename(&bak_path, &final_path)` for each. If even *one* of those renames fails (line 519 `tracing::error!` arm), the rollback continues to the next extension — but `bak_path` is now in an indeterminate state: some `.bak` files were successfully restored, some are still present alongside their (newly-removed) `final_path`. The next `save_owned` call (line 437) hits `final_path.exists() == false` for those extensions and skips the `rename(final_path, bak_path)` step (line 436 guards on `exists`). It then writes new `final_path`s, and subsequent rollback restores from the *stale* leftover `.bak` files from the prior failed save — silently overwriting a known-good index with a known-bad backup from the previous failure.
  The rollback fsync at 532-549 is only a `tracing::debug!` on failure, so an audit can't even tell this happened. The `tracing::error!` at line 521 is the only signal, and it doesn't bail — it logs and continues.
- **Suggested fix:** Track which `.bak` files were successfully renamed back. On any failure, leave the unrenamed `.bak` files in place AND emit a `tracing::error!` with a clear marker (e.g. "HNSW rollback incomplete; manual recovery required: restore .bak files matching {basename}.*.bak") so operators have a recovery breadcrumb. Better: at the start of `save_owned`, if any `.bak` files exist from a previous failed save, refuse the new save with a clear error pointing operators at the recovery step.

#### `dropped_this_cycle` reset before snapshot publish hides drop signal — distinct from the code-quality finding because the data-safety angle is "fresh state lies"
- **Difficulty:** easy
- **Location:** `src/cli/watch/events.rs:139-146` (reset) + `src/cli/watch/mod.rs:1303` (publish ordering)
- **Description:** Already filed under Code Quality (audit-findings.md), but worth re-flagging here for the data-safety implication. `process_file_changes` clears `state.dropped_this_cycle = 0` at line 145 before reindex completes; `publish_watch_snapshot` runs *after* the drain. The snapshot always sees `dropped_this_cycle == 0` even when the cycle that just ran started with a non-zero count. `WatchSnapshot::compute` uses `dropped_this_cycle > 0` as a Stale signal — but the read-after-reset means it never observes a non-zero value at publish time. The dropped files won't be reindexed until the next reconciliation pass (up to 30s later). For data-safety: `cqs eval --require-fresh` is *the* gate that's supposed to prevent eval against a silently-incomplete index. This collapses the gate.
- **Suggested fix:** See the original finding. Either accumulate `dropped_total` across cycles and only reset after a successful reconcile, or move the reset to *after* `publish_watch_snapshot` in the outer loop.

## Performance

#### PF-V1.30.1-1: Daemon publishes watch snapshot every 100ms with a `fs::metadata(index_path)` syscall on each tick
- **Difficulty:** easy
- **Location:** `src/cli/watch/mod.rs:149-185` (`publish_watch_snapshot`); called at `src/cli/watch/mod.rs:1303` in the main loop's 100ms tick
- **Description:** The watch loop calls `publish_watch_snapshot` at the bottom of every iteration of the outer `recv_timeout(100ms)` loop. Inside, line 155 unconditionally does `std::fs::metadata(index_path).ok().and_then(|m| m.modified().ok())` to populate `last_synced_at`. Even when the file hasn't changed, that's a `stat()` syscall every 100ms (~10/sec, ~36k/hour). On WSL `/mnt/c/` 9P this is amplified — a single `stat` on a Windows-side path is 1-50ms; the syscall stream contributes measurable background load on the daemon thread that's documented elsewhere in this codebase as a known WSL pain point. The `last_synced_at` value can only change when this same daemon writes the index, so the metadata read is redundant: the daemon already knows when it has written. (Compare `prune_last_indexed_mtime` in `src/cli/watch/gc.rs:52-62`, which was reworked specifically to avoid per-entry `stat()` calls under the same WSL constraint.)
- **Suggested fix:** Cache `last_synced_at` as `Arc<AtomicI64>` (or a field on `WatchState`) updated only when the daemon successfully commits a write batch. The publish path then reads the atomic with no syscall. Alternative: throttle the metadata call to once per N ticks (e.g. every 10s) since `last_synced_at` granularity is seconds anyway. The first option is strictly better — zero syscall and exact precision.

---

#### PF-V1.30.1-2: `wait_for_fresh` polls the daemon every 250ms with a fresh socket connect + JSON round-trip
- **Difficulty:** medium
- **Location:** `src/daemon_translate.rs:660-679` (`wait_for_fresh`); per-poll cost in `src/daemon_translate.rs:422-510` (`daemon_status`)
- **Description:** `wait_for_fresh` busy-polls at a fixed 250ms interval until either `state == fresh` or the deadline expires. With the default `--require-fresh-secs=600`, a stuck-stale tree triggers up to **2,400 socket connects + 2,400 JSON round trips** in a single eval gate. Each `daemon_status` call (a) opens a new `UnixStream` to the daemon socket, (b) serialises a JSON request, (c) writes/flushes, (d) reads a response line, (e) parses it through `serde_json` twice (envelope + payload), (f) closes the socket. The daemon's `dispatch_status` itself is an `RwLock::read().clone()` — microseconds — so virtually all wall time is connect + JSON parse overhead the client pays needlessly. The poll interval doesn't back off: a tree that's been stale for 60s gets the same 4 polls/sec as one that's been stale for 1s, even though the cost of waiting an extra second is identical at both points. `cqs eval --require-fresh` adds this round-trip cost at the head of every eval invocation; an A/B over a fixture that takes 30s to run pays the gate cost once, but a sweep over many slots pays it N times.
- **Suggested fix:** Two complementary fixes. (1) **Server-side wait**: add a `status --wait <secs>` daemon command that blocks server-side on a `tokio::sync::Notify` flipped by the watch loop when `state` transitions to `fresh`, then returns. One round-trip total instead of N. (2) If client-side polling is preferred for simplicity, **exponential backoff with cap**: start at 100ms, double up to 2s. A tree that's been stale for 30s polls every 2s (~15 polls in the next 30s) instead of every 250ms (~120 polls). Cuts connect/JSON cost ~8× for the long-stale case at zero correctness cost — `is_fresh()` is monotonic in practice, so checking less often only delays the success signal, never misses it.

---

#### PF-V1.30.1-3: Periodic GC and reconcile each call `enumerate_files` independently — back-to-back full tree walks within a single tick
- **Difficulty:** medium
- **Location:** `src/cli/watch/mod.rs:1198-1283` (idle-tick gating + dispatch); `src/cli/watch/gc.rs:209` and `src/cli/watch/reconcile.rs:74` (the two `enumerate_files` callsites); `src/lib.rs:574-689` (the walk itself, including per-entry `dunce::canonicalize`)
- **Description:** When the daemon idles ≥ `daemon_periodic_gc_idle_secs()` (default 60s), both periodic GC and periodic reconcile may fire in the same tick: GC's `Duration::from_secs(daemon_periodic_gc_interval_secs())` (default 1800s) gates Pass 1, reconcile's `daemon_reconcile_interval_secs()` (default 30s) gates the second walk. Their idle gates are identical, so on tick boundaries that satisfy both intervals, the daemon walks the whole working tree **twice in succession**, each walk doing per-file canonicalisation through `dunce::canonicalize` (line 659). On a 17k-chunk corpus with ~3-5k unique source files on WSL `/mnt/c/`, each walk is ~1s wall (per the docstring on `DAEMON_RECONCILE_INTERVAL_SECS_DEFAULT`); doing it twice back-to-back is a 2s contention window every 30 minutes. Worse: even when only one fires, both call paths build a `HashSet<PathBuf>` of disk files (gc.rs:211, reconcile.rs:84+95), so the same data is materialised twice in the same tick whenever both run.
- **Suggested fix:** Lift the walk out of both call sites: walk once at the top of the idle-tick block, share the resulting `HashSet<PathBuf>` between GC's `prune_missing` and reconcile's loop. Both already operate on the same set of disk files; the only daylight is reconcile checks each file's mtime and GC checks its existence, both derivable from one walk. Given `daemon_periodic_gc_idle_secs()` is the single shared idle gate, the walk-once refactor is purely organisational — no semantic change, no new env knob.

---

#### PF-V1.30.1-4: `run_daemon_reconcile` allocates a fresh `String` for every disk file via `to_string_lossy().replace('\\', "/")` even on Linux where no replacement happens
- **Difficulty:** easy
- **Location:** `src/cli/watch/reconcile.rs:99` inside the `for rel in disk_files` loop
- **Description:** For every file enumerated (3-5k on a typical 17k-chunk corpus), the loop runs `let origin = rel.to_string_lossy().replace('\\', "/")`. `String::replace` *always* allocates a new `String`, even when no occurrences of the pattern are found. On Linux there are zero backslashes in any path, so every iteration pays an unconditional ~50-100 byte allocation that produces an exact byte-equal copy of the input. With reconcile firing every 30s, that's ~3-5k unnecessary allocs per tick, ~360-600k per hour from this single line. The function also already calls `rel.clone()` separately at line 103 and 128 for the `pending_files.insert`, so the per-file allocation budget is doubled.
- **Suggested fix:** Skip the replacement when no backslash is present. `let origin = rel.to_string_lossy(); let origin: Cow<str> = if origin.contains('\\') { Cow::Owned(origin.replace('\\', "/")) } else { origin };` — Linux fast path stays as a `Cow::Borrowed`, only Windows pays the allocation. Then `indexed.get(origin.as_ref())` works against the `HashMap<String, _>` by `&str`. Cuts the hot-loop allocation count roughly in half on Linux and WSL.

---

#### PF-V1.30.1-5: `build_stats` issues 4 sequential `fetch_one` round-trips for what is one query
- **Difficulty:** easy
- **Location:** `src/serve/data.rs:1105-1128` (`build_stats`)
- **Description:** The function makes four serial round-trips through the connection pool: `COUNT(*) FROM chunks`, `COUNT(DISTINCT origin) FROM chunks`, `COUNT(*) FROM function_calls`, `COUNT(*) FROM type_edges`. Each `fetch_one` call against a sqlx pool (a) checks out a connection, (b) prepares/binds, (c) round-trips to SQLite, (d) returns the connection. SQLite is in-process so per-call latency is small (~tens of microseconds), but the call is on the hot HTTP path of the `cqs serve` UI and runs once per page render of the dashboard (and on every poll if the front-end refreshes). One query with subselects collapses 4 connection checkouts into 1 with no functional change.
- **Suggested fix:** ```sql
SELECT
  (SELECT COUNT(*) FROM chunks),
  (SELECT COUNT(DISTINCT origin) FROM chunks),
  (SELECT COUNT(*) FROM function_calls),
  (SELECT COUNT(*) FROM type_edges)
``` parsed into `(i64, i64, i64, i64)` via `query_as`. 1 connection checkout, 1 round-trip. Strictly faster, identical semantics.

---

#### PF-V1.30.1-6: `enforce_auth` allocates two strings per HTTP request for the cookie name and the `name=` lookup needle
- **Difficulty:** easy
- **Location:** `src/serve/auth.rs:357` (`cookie_name_for_port` call); `src/serve/auth.rs:292` (`format!("{cookie_name}=")` inside `check_request`)
- **Description:** Per HTTP request, `enforce_auth` runs `let cookie_name = cookie_name_for_port(state.cookie_port)` which `format!`s `"cqs_token_<port>"` into a fresh `String`. It then passes that to `check_request`, which immediately runs `let needle = format!("{cookie_name}=")` — a second `format!` allocating yet another `String` just to hold the lookup prefix. The cookie name for a given server instance is fixed at server startup (the port doesn't change), so both strings could be precomputed once into `AuthMiddlewareState` and borrowed per request. For a UI doing tens of requests per second (browsing the graph, polling `/health`), this is two allocations per request that buy nothing. Bounded, but unnecessary.
- **Suggested fix:** Add `cookie_name: Arc<str>` and `cookie_lookup_needle: Arc<str>` fields to `AuthMiddlewareState`, populated at construction time from `cookie_name_for_port(port)` and `format!("{cookie_name}=")`. Update `check_request` to take `&str` for the needle (or borrow from state directly). Both fields are `Arc<str>` so `Clone` of the state stays cheap. Net: zero allocations per request for the auth happy path.

---

#### PF-V1.30.1-7: Watch reindex clones each `content_hash` String into a `Vec<String>` for incremental HNSW even when the data is read-only thereafter
- **Difficulty:** easy
- **Location:** `src/cli/watch/reindex.rs:414-417`
- **Description:** ```rust
let content_hashes: Vec<String> = to_embed
    .iter()
    .map(|(_, c)| c.content_hash.clone())
    .collect();
``` Each `content_hash` is a 64-char hex string (~64 byte allocation). For a typical incremental-watch pass that re-embeds 50-200 chunks, this is 50-200 unnecessary allocations per cycle; bigger reindex bursts (e.g. branch switch with 1000 changed chunks) scale linearly. The chunks themselves outlive this `Vec` (they're owned earlier in the function), so the clone exists only because the downstream HNSW API takes `Vec<String>` rather than `&[&str]`. A consumer that takes borrowed strs would let this Vec be `Vec<&str>` with no allocation per element.
- **Suggested fix:** Two paths. (1) Change the downstream HNSW insert API to take `&[&str]` — works if no caller stores the slice past the call. (2) If the API contract requires owned strings (e.g. async insert that captures), at least preallocate `Vec::with_capacity(to_embed.len())` to avoid Vec resizes. Option 1 is the real fix; option 2 is the cheap one. Worth pairing with `Arc<str>` for `Chunk::content_hash` if it's referenced from multiple places (it is: `store_hits`, `global_hits`, the chunk itself).

---

#### PF-V1.30.1-8: `indexed_file_origins` returns `HashMap<String, Option<i64>>` populated from a `SELECT DISTINCT` that may overwrite entries silently when one origin has multiple stored mtimes
- **Difficulty:** medium
- **Location:** `src/store/chunks/staleness.rs:627-637`
- **Description:** The query `SELECT DISTINCT origin, source_mtime FROM chunks WHERE source_type='file'` returns one row per (origin, mtime) **pair**. If a file's chunks were written across two upserts at different mtimes (a known edge case during partial reindex failures, or transient writes during a watch tick), `rows.into_iter().collect::<HashMap<_,_>>()` arbitrarily picks the **last** one in iteration order — silently dropping the earlier mtimes. From the reconcile caller's perspective, the wrong stored mtime causes either a missed reindex (if the chosen mtime happens to be ≥ disk mtime) or a spurious one. More fundamentally: `DISTINCT` is doing extra work the index could skip. The caller wants one row per origin with a deterministic mtime — it should be the **max** stored mtime, since reindex semantics are "did anything indexed for this origin lag the disk." A `GROUP BY origin` with `MAX(source_mtime)` produces fewer rows, no nondeterminism, and the index already supports it. As-is, the DISTINCT scan returns up to 17k rows for a 3k-file corpus when chunks-per-file averages 5 and mtimes-per-origin diverges; that's wasted SQLite work plus wasted hashmap insertions.
- **Suggested fix:** Replace the query with `SELECT origin, MAX(source_mtime) FROM chunks WHERE source_type='file' GROUP BY origin`. Returns one row per origin with the most-recent stored mtime, which is the semantically correct value for reconcile's `disk > stored` predicate. Fixes the silent-drop bug and reduces row count to exactly the file count.
# Audit Findings — Resource Management (v1.30.1, 2026-04-28)

Audit mode: ON. Scope: memory leaks, fd leaks, idle CPU/memory cost,
startup time, OOM protection, RAII discipline, clone/copy of huge buffers,
hash-table growth without bound. Triage state checked against
`docs/audit-triage-v1.30.0.md` (RM-V1.30-* covered) and prior audit
findings in `docs/audit-findings.md`.

---

## Resource Management

#### RM-1: `daemon-client` thread_local `REQ_LINE` is a no-op — daemon spawns a fresh thread per accepted connection
- **Difficulty:** easy
- **Location:** `src/cli/watch/socket.rs:91-99` (the `thread_local!` declaration) + `src/cli/watch/daemon.rs:189-205` (the spawn site)
- **Description:** The comment at `socket.rs:92-96` justifies a `thread_local! REQ_LINE: RefCell<String>` with: *"`handle_socket_client` runs on a Tokio blocking-pool thread that services many connections in succession. Allocating a fresh `String` (and its grow-during-`read_line` churn) per accept is ~80% of the allocator pressure on this path under high-QPS agent workloads."* That premise is wrong. `daemon.rs::spawn_daemon_thread` spawns a brand-new `std::thread::Builder` thread *for every accepted connection* (line 189), not a Tokio blocking pool with thread reuse. The thread terminates on connection close, taking the thread_local with it. Net effect: every connection still pays one fresh `String::with_capacity(8192)` allocation, plus the additional thread_local-init overhead, and the comment's "80%" claim is unfounded. (The Tokio blocking pool only services axum's `cqs serve` — a different binary path.) The optimization (RM-V1.29-10 / #1116) was either misanalyzed or attached to the wrong dispatch path.
- **Suggested fix:** Either (a) drop the `thread_local!` and use a plain `let mut line = String::with_capacity(8192)`; the cost is identical, the code is simpler, and the comment stops lying; or (b) port the daemon to a real thread pool (`rayon`, a bounded `crossbeam` worker fan-out, or a Tokio current-thread + blocking-pool dispatch) so the thread_local actually amortizes. (a) is the one-line fix; (b) is an architectural change that would need its own design pass.

#### RM-2: `wait_for_fresh` opens a fresh Unix socket connect+disconnect every 250ms for up to 600s
- **Difficulty:** medium
- **Location:** `src/daemon_translate.rs:660-679` (poll loop) + `src/daemon_translate.rs:438` (per-call `UnixStream::connect`)
- **Description:** `wait_for_fresh` is the shared client-side polling primitive for `cqs status --watch-fresh --wait` and `cqs eval --require-fresh`. Each iteration calls `daemon_status`, which opens a fresh `UnixStream::connect`, sets read/write timeouts, writes a 30-byte JSON request, reads a 64 KiB-bounded response, and drops the stream. The poll cadence is 250 ms; the caller's deadline is `wait_secs` (clap default 30 s, max 600 s = 2400 round trips). On the daemon side, every connection: (1) wakes the accept loop's `WouldBlock`-poll thread, (2) spawns a *fresh OS thread* (see RM-1 — there is no thread pool), (3) takes the `BatchContext` mutex to dispatch `status`, (4) RwLock-reads the snapshot, (5) tears the thread down. Two concurrent `eval --require-fresh` runs (a real scenario when an agent batch fans out) easily generate 4-5k connect-spawn-teardown cycles in a 60s wait. None of this is *catastrophic* — Unix sockets are cheap — but for a primitive that's now on the hot path of #1182 (the freshness gate) the cost shape is wrong. The daemon's accept loop also pays this cost end-to-end on a polling tick + thread spawn.
- **Suggested fix:** Either (a) keep the `UnixStream` open across polls inside `wait_for_fresh` and reuse it for subsequent `status` requests (one connect + N writes + N read_lines until either fresh or timeout); or (b) switch the freshness signal to a daemon-pushed event (e.g., write an inotify-watchable `.cqs/freshness` marker file when the snapshot transitions to `Fresh`, have the client `inotify_wait` on it). (a) is a 30-line change confined to `daemon_translate.rs`; (b) eliminates polling entirely but is invasive. Either way, RM-1's "fresh-thread-per-accept" pain disappears for the freshness-wait case.

#### RM-3: `compute_context` reads the entire source file into memory just to extract N context lines per search result
- **Difficulty:** easy
- **Location:** `src/cli/display.rs:59-99` (and second instance at `src/cli/display.rs:489`)
- **Description:** `compute_context` is called once per search result to produce the few lines of code shown above and below the chunk's `line_start`/`line_end`. Implementation does `std::fs::read_to_string(file)` followed by `let lines: Vec<&str> = content.lines().collect()` — pulling the entire file into RAM and materializing a `Vec<&str>` covering every line, only to slice `[start..end]` near the bottom. With the default `cqs search --limit 50` and a 200-300 KB source file per result (large generated code, vendored JSON, schema files, etc.) this is up to ~15 MB of transient allocations per command, all to display ~10 lines per hit. There's a `max_display_size` cap (`CQS_MAX_DISPLAY_FILE_SIZE`, default 1 MiB) — but that just caps the worst case; the typical case still reads everything when it could read a small window. The 1 MiB cap also doesn't help an agent fanning out 20 searches against a typical Rust monorepo where 1MB files exist.
- **Suggested fix:** Use `std::io::BufReader::new(File::open(path))` plus `.lines().enumerate()` and break out of the iteration once `enumerate()` reaches `max(end_idx, end_idx + context) + 1`. This bounds the read to `O(line_end + context)` lines (typically <1 KB) instead of `O(file_size)`. Same number of syscalls, much less heap. Apply at both call sites (display.rs:59 and :489).

#### RM-4: `build_hnsw_index_owned` accumulates a full `HashMap<String, String>` of (id, content_hash) snapshots that's only used at swap time
- **Difficulty:** medium
- **Location:** `src/cli/commands/index/build.rs:1110-1126` (build) + `src/cli/watch/rebuild.rs:262-267` (carries it across thread boundary) + `src/cli/watch/rebuild.rs:368-380` (consumes at swap)
- **Description:** During a background HNSW rebuild, `build_hnsw_index_owned` builds a `snapshot_hashes: HashMap<String, String>` with one entry per chunk. Each entry is roughly `30 B (id) + 64 B (hex hash) + 80 B (HashMap overhead)` ≈ 174 B. For a 100k-chunk corpus that's ~17 MB. This map is held in the background-rebuild thread for the *entire rebuild duration* (commonly 10-30s; longer on first builds), then handed across the channel to the watch loop, where `drain_pending_rebuild` reads it once at swap time to dedup the captured `pending.delta` against id-collisions, then drops it. Meanwhile the watch loop is *also* keeping the previous in-memory HNSW resident (~`chunk_count * dim * 4` = 400 MB at BGE-large 1024-dim) so search keeps working during the rebuild, and the rebuild thread is building the new HNSW (another 400 MB). Net peak: 2× HNSW + 17 MB map + delta. The 17 MB is small relative to 800 MB of HNSW, but it's *unnecessary* — the only consumer (`drain_pending_rebuild`) reads it after the rebuild completes and could fetch the snapshot fresh from SQLite at that point with a single `SELECT id, content_hash FROM chunks WHERE id IN (?,?,…)` against just the delta's ids (typically <`MAX_PENDING_REBUILD_DELTA` = small N), not the full 100k.
- **Suggested fix:** Drop `snapshot_hashes` from the rebuild-thread return shape entirely. At swap time, `drain_pending_rebuild` already knows the delta's ids; do `store.get_content_hashes_by_ids(&delta_ids)` to fetch *just* the rows needed for collision detection. That's a bounded (delta-sized) query against fresh SQLite state, not a full-corpus snapshot held for the rebuild's lifetime. Saves ~17 MB during every rebuild on a 100k-chunk repo, and removes the cross-thread `HashMap<String, String>` serialization cost.

#### RM-5: Reconcile pass holds the entire repo's filename set + index origins in RAM simultaneously every 30s
- **Difficulty:** medium
- **Location:** `src/cli/watch/reconcile.rs:74-90` (`enumerate_files` + `indexed_file_origins`) + `src/lib.rs:618` (`enumerate_files` returns `Vec<PathBuf>` not iterator)
- **Description:** `run_daemon_reconcile` runs every 30s by default (`CQS_WATCH_RECONCILE_SECS`) when the daemon is idle. Each tick:
  1. `enumerate_files(root, &exts, no_ignore)` returns a `Vec<PathBuf>` materialized via `walker.collect::<Vec<_>>()` — the *entire* tree's matching files in one allocation (lib.rs:618). For a 100k-file monorepo at avg 80 B/PathBuf that's ~8 MB.
  2. `indexed_file_origins()` returns a `HashMap<String, Option<i64>>` with one entry per indexed source file — typically ~30k entries × ~120 B = ~3.5 MB.
  3. Both are held simultaneously through the loop body that does set-difference and metadata() probes.
  Every 30s, ~12 MB allocated, walked, and dropped. Not a leak — the allocations are scoped — but it's repetitive heap churn the watch daemon pays *forever in idle*, and it doesn't scale: a 1M-file Chromium-class repo would hit ~120 MB transient per tick. The walker already supports streaming (`Walk::iter()`) and the `indexed` map could be queried per-file with a prepared statement during the walk, but the current shape forces an all-at-once materialization. Reconcile is also single-threaded and CPU-bound on the walk under WSL `/mnt/c/`, holding the watch loop off rebuilding for the duration.
- **Suggested fix:** Stream the disk walk: change `enumerate_files` to expose an `enumerate_files_iter(root, exts, no_ignore) -> impl Iterator<Item = anyhow::Result<PathBuf>>` (the `Vec`-collecting version stays as a thin wrapper for callers that genuinely need the count). In reconcile, switch `for rel in disk_files` to consume the iterator directly, prepared-statement-querying `chunks.source_mtime WHERE origin = ?` per file (or chunked in 1k batches) instead of pre-loading the full `indexed` HashMap. Memory drops to O(batch_size) regardless of repo size, and the walk + DB lookups overlap in time. Important enough that #1182's whole-tree-walk-on-idle premise should not break on a 1M-file repo.

#### RM-6: `serve` auth path allocates a fresh `format!("{cookie_name}=")` String on every authenticated request
- **Difficulty:** easy
- **Location:** `src/serve/auth.rs:292`
- **Description:** Inside `check_request`, the cookie-channel branch does `let needle = format!("{cookie_name}=");` on every request. `cookie_name` is per-port-fixed (set at `cqs serve` startup, see #1135) — there is exactly one cookie name for the lifetime of the server. The format-and-allocate happens once per HTTP request. At the dashboard's typical load (an agent fanning out 50-100 graph/search/chunk requests per page) that's 50-100 short-lived `String` allocations per second, none of which the allocator can pool because each is dropped before the next is created. Comparable per-request work happens in `strip_token_param` (line 246-260 — allocates a `Vec<&str>` and joins) but only when the *post-auth redirect* fires, which is once per session. The cookie needle is in the *steady-state* request path.
- **Suggested fix:** Pre-build the needle once in `AuthMiddlewareState::new` (or store it on `AuthMiddlewareState` directly as `cookie_needle: String`), then the auth check borrows `&self.cookie_needle` instead of allocating per request. Saves an allocation per authenticated request at zero behavioural change. Same shape would also let the bearer-prefix `"Bearer "` be a `&'static str` constant if it isn't already.

#### RM-7: `check_request` cookie loop scans every `;`-separated pair even after a match
- **Difficulty:** easy
- **Location:** `src/serve/auth.rs:287-300`
- **Description:** The cookie-header branch in `check_request` does `for pair in cookie_header.split(';')`. When the request carries an unrelated leading cookie (e.g. `_ga=GA1.2.123; cqs_token_8080=abc`) that isn't `cqs_token_*`, the loop continues to subsequent pairs and `strip_prefix(&needle)` returns `None` until it hits ours. That's correct. However, the loop doesn't `break` on a successful `ct_eq` — it returns. So far fine. The actual issue: `pair.trim().strip_prefix(&needle)` allocates nothing, but `needle` is the heap-allocated `format!("{cookie_name}=")` from RM-6. Fix RM-6 and the per-pair work shrinks to a `&str` comparison. (Not a separate finding from RM-6 in impact, but the fix surface is the same line.)
- **Suggested fix:** Subsumed by RM-6's pre-built needle. No additional work.

## Test Coverage — happy-path gaps

#### TC-HAP-1.30.1-1: `cmd_install` upgrade-marker path never executed by any test
- **Difficulty:** easy
- **Location:** src/cli/commands/infra/hook.rs:149-200 (production); 414-487 (tests)
- **Description:** The four tests in the `tests` mod cover `render_hook_script` (template content), `locate_git_hooks_dir` (path fallback), and a hand-written `write_hook_script` loop that simulates a fresh-install. None of them invoke `cmd_install` itself. The crucial branch — `Some(content)` containing `HOOK_MARKER_PREFIX` → `report.upgraded.push(hook)` (lines 175-181) — is the entire idempotence claim of `cqs hook install`. It has zero test coverage. The `skipped_existing` branch (foreign hook detected, line 182-187) is "tested" only by `install_skips_foreign_hook_marker_check`, which **re-implements the marker-check inline** rather than calling production code (line 463-464 admits "Reproduce the marker check inline so the assertion is independent of the (file-system-bound) `cmd_install` function"). A future refactor that flips `contains(HOOK_MARKER_PREFIX)` to `starts_with` (a real possibility — the marker is line-2-not-line-1 by design) silently breaks idempotence: every `cqs hook install` re-run on an existing cqs-managed hook would land in the *foreign* arm and stop overwriting. No test catches it.
- **Suggested fix:** Add `cmd_install_upgrade_replaces_v0_marker_with_current` — write a fake hook with body `# cqs:hook v0\n...` to a tempdir, run the `cmd_install` *body* (factor out a `do_install(git_dir, no_overwrite)` so the test doesn't need `find_project_root`), assert the file's content now contains `HOOK_MARKER_CURRENT`. Add `cmd_install_idempotent_second_run_only_upgrades` — run twice, assert the second run reports `upgraded == 3` and `installed == 0`. Add `cmd_install_no_overwrite_skips_missing_hooks` — fresh tree with `no_overwrite=true`, assert `skipped_no_overwrite.len() == 3` and no files were written.

#### TC-HAP-1.30.1-2: `cmd_uninstall`, `cmd_fire`, `cmd_hook_status` — three entire CLI commands have zero tests
- **Difficulty:** medium
- **Location:** src/cli/commands/infra/hook.rs:262-373
- **Description:** Three of the four `HookCommand` variants — `Uninstall`, `Fire`, `Status` — ship with zero test coverage. `cmd_uninstall` has the foreign-hook-skip branch (line 283-285) that's the symmetric concern to `cmd_install`'s idempotence: a future refactor that drops the marker check would silently delete user-authored `post-checkout` hooks. `cmd_fire` is the runtime path — `daemon_reconcile` Ok → `sent_to_daemon = true` → return (line 312-316), and the fallback path (`.cqs/.dirty` touch, line 329-332). Both are unverified. `cmd_hook_status` has three text outputs (installed / foreign / missing) and a daemon-up probe, none pinned. The `daemon_reconcile` mock at `src/daemon_translate.rs:1104` exists — `cmd_fire` happy path is testable in-process today but has no test. Per the audit's "always do things properly" principle: a new command shipped without exercising at least one happy path per subcommand is not done.
- **Suggested fix:** Add `cmd_uninstall_removes_only_marked_hooks` — seed two cqs-marked + one foreign hook in a tempdir, run uninstall, assert removed=2, skipped_foreign=1. Add `cmd_fire_writes_dirty_marker_when_daemon_absent` — point at a tempdir with no socket, run `cmd_fire("post-checkout", vec![], false)`, assert `.cqs/.dirty` exists and `report.sent_to_daemon == false`. Add `cmd_hook_status_classifies_three_hook_states` — fresh tempdir, write one cqs-marked + one foreign + leave one missing, run status, assert installed/foreign/missing each have one entry.

#### TC-HAP-1.30.1-3: `cmd_status` — entire CLI command body is untested; behaviour matrix promises 6 outcomes, zero are pinned
- **Difficulty:** medium
- **Location:** src/cli/commands/infra/status.rs:38-103 (production); 147-174 (tests)
- **Description:** The three tests in the `tests` mod (`fresh_snapshot_is_fresh`, `stale_snapshot_is_not_fresh`, `unknown_snapshot_is_not_fresh`) exercise `WatchSnapshot::is_fresh()` — a one-line accessor on the *type*, not on `cmd_status`. The CLI body (lines 38-103) ships zero tests. The docstring (lines 28-35) explicitly promises a six-row "behaviour matrix": `(no flag)` → exit 1 with a specific message, `--watch-fresh` → exit 0 with one-line text, `--wait` without `--watch-fresh` → exit 1, `--watch-fresh --wait --json` with budget expired → exit 1, etc. None of those exit codes or output shapes are pinned. The `--wait`-without-`--watch-fresh` early-error (which would now exit 0 if the dispatch order in registry.rs ever changed, since the flag-check happens first) is invisible to the test suite. `print_text` (the multi-line text output a user actually sees) and `emit_no_daemon` (the friendly "is the daemon up?" stderr message) are both unreached. A regression that swaps `state:` and `modified_files=` lines (an agent's grep filter: `^state: fresh`) goes undetected.
- **Suggested fix:** Add a `tests/cli_status_test.rs` integration test (mirrors `tests/cli_ref_test.rs` shape — XDG-isolated, assert_cmd, `gated behind slow-tests`) covering: (a) `cqs status` (no flag) exits 1 with `"pass --watch-fresh"` on stderr, (b) `cqs status --wait` (no `--watch-fresh`) exits 1 with the same gate message, (c) `cqs status --watch-fresh` against a tempdir without a daemon exits 1 and prints `cqs:` + the no-daemon message on stderr. For the daemon-up paths use the same `UnixListener` mock pattern as `wait_for_fresh_returns_fresh_on_first_poll` (daemon_translate.rs:1210) — pin the one-line text format (`state: fresh`) and the JSON envelope shape.

#### TC-HAP-1.30.1-4: `require_fresh_gate` — the function itself is never called by any test; both env-disable and `--no-require-fresh` short-circuits unverified end-to-end
- **Difficulty:** easy
- **Location:** src/cli/commands/eval/mod.rs:219-275 (production); 405-432 (test reimplements env logic inline)
- **Description:** Sibling of TC-ADV-1.30.1-7 but on the *happy* axis. The two short-circuits at the top of `require_fresh_gate` — `if *no_require_fresh_flag { return Ok(()) }` (line 220-223) and `if env_disables_freshness_gate() { return Ok(()) }` (line 224-230) — are the entire bypass surface that makes `cqs eval` runnable in CI / dev shells without a daemon. Neither path is exercised by *calling* `require_fresh_gate`. The test at line 406 reimplements the env body inline (called out in TC-ADV-1.30.1-7); the `--no-require-fresh` path has no test at all. Every integration test in `tests/eval_subcommand_test.rs` and `tests/eval_baseline_test.rs` sets `CQS_EVAL_REQUIRE_FRESH=0` to bypass the gate (`tests/eval_subcommand_test.rs:91`) — so even the "eval works end-to-end" suite is silent on whether the gate is wired. A future change that swaps the order of the two short-circuits, or accidentally returns `Err` on `*no_require_fresh_flag = true` (a one-character typo `if !*no_require_fresh_flag`), passes every test.
- **Suggested fix:** Make `require_fresh_gate` `pub(crate)` (already is) and add `require_fresh_gate_no_require_fresh_flag_returns_ok_without_daemon` — call directly with `&true, 5`, assert `Ok(())` returns immediately even though no socket exists. Add `require_fresh_gate_env_disable_returns_ok_without_daemon` (gated `#[serial_test::serial]` to share the env namespace with the existing tests in daemon_translate.rs) — set `CQS_EVAL_REQUIRE_FRESH=0`, call with `&false, 5`, assert `Ok(())`. Add `require_fresh_gate_fresh_daemon_returns_ok` — spin up the same `UnixListener` mock used by `wait_for_fresh_returns_fresh_on_first_poll`, point `CQS_HOME`/`XDG_RUNTIME_DIR` at the tempdir, call with `&false, 5`, assert `Ok(())`.

#### TC-HAP-1.30.1-5: `wait_for_fresh` — Stale → Fresh transition (the realistic wait case) untested; only "fresh on first poll" pinned
- **Difficulty:** medium
- **Location:** src/daemon_translate.rs:660-679 (production); 1208-1376 (tests)
- **Description:** Of the three `FreshnessWait` outcomes, only the *trivial* Fresh path is pinned: `wait_for_fresh_returns_fresh_on_first_poll` (line 1210) accepts a single connection, returns Fresh immediately, the helper exits the loop on iteration zero. The *common* shape — daemon reports Stale on first poll, then Fresh on a subsequent poll within budget — is never tested. That is the literal use case the helper exists for: "wait up to N seconds for a reindex to drain". A regression that breaks the polling loop (e.g., a refactor that reads the snapshot but never re-checks `is_fresh()`, or sleeps inside the lock and starves the polling thread) returns Fresh on iteration zero (test pass) and never converges on iteration N (production hang). With a 250 ms hardcoded poll interval (SHL-V1.30-2) the test would still run in <1 s — `wait_secs=2`, accept three connections, return Stale-Stale-Fresh, assert the helper takes ≥250 ms and returns Fresh. No such test exists.
- **Suggested fix:** Add `wait_for_fresh_returns_fresh_after_two_stale_polls` — `UnixListener` accepts 3 connections in order, writes Stale envelope twice then Fresh, the helper returns `FreshnessWait::Fresh(_)` after roughly one poll interval. Pin the elapsed `>= POLL_INTERVAL` lower bound. Optionally also pin `incremental_count` from the *third* response so the test fails if the helper accidentally returns the first response.

#### TC-HAP-1.30.1-6: `process_file_changes` — the central watch-loop reindex function has zero direct tests
- **Difficulty:** hard
- **Location:** src/cli/watch/events.rs:131-300+ (production)
- **Description:** `process_file_changes` is the function the watch daemon calls every cycle to drain `pending_files` into the index — the entire post-event happy path. It has zero direct tests. `grep -rn process_file_changes src` returns three production call sites in `watch/mod.rs` (lines 1125, 1254, 1276) and zero test sites. The `reconcile.rs::tests::reconcile_detects_bulk_modify_burst` test exercises *queueing* into `pending_files`, then asserts the snapshot reads Stale — but never invokes `process_file_changes` to drain it. So the canonical "queue → drain → snapshot reads Fresh" round-trip is **not** an end-to-end test in the suite; both halves exist, only the seam is untested. The `try_init_embedder` Err arm (already flagged as EH-V1.30.1-8) silently returns without clearing dirty flags — invisible without a test that drives the full drain. The `dropped_this_cycle > 0` warn arm (line 139-145) is also unreached. The println! gating on `cfg.quiet` (line 147) is not pinned (related to OB-V1.30.1-9 which flagged the println! itself).
- **Suggested fix:** Add a `process_file_changes_zero_files_is_noop` test — empty `pending_files`, run, assert `state.pending_files.is_empty()` and no panics, no embedder init. Add `process_file_changes_single_file_drains_into_index` — seed one Rust file in a tempdir, push its rel path into `pending_files`, run with a stub embedder (existing `placeholder_embedding` helper from reconcile tests), assert (a) `state.pending_files` is empty after, (b) `store.search` returns the new chunk. Add `process_file_changes_reports_dropped_warn_once_then_resets` — set `state.dropped_this_cycle = 5`, run, assert `state.dropped_this_cycle == 0` after (this pins the reset-after-warn semantic that's currently invisible).

#### TC-HAP-1.30.1-7: Eval freshness gate — no integration test confirms `cqs eval` actually runs after a Fresh gate verdict
- **Difficulty:** medium
- **Location:** tests/eval_subcommand_test.rs:88-93 + tests/eval_baseline_test.rs:96
- **Description:** Every integration test in the eval suite sets `CQS_EVAL_REQUIRE_FRESH=0` to bypass the gate (`eval_subcommand_test.rs:91`, `eval_baseline_test.rs:96`). There is no integration test that exercises the gate in its primary mode: spin up a daemon mock that returns Fresh, run `cqs eval` *without* the env bypass, confirm the gate passes and the runner proceeds to compute R@K. This is the entire "PR 4 of #1182" landed feature surface — it ships untested end-to-end. Worse, the wiring between `require_fresh_gate(Ok)` and the rest of `cmd_eval` is unverified: a future refactor that changes `require_fresh_gate` to return `Result<bool>` (e.g., to surface "gate disabled by env" in JSON output) and has the caller forget to re-check the bool would silently skip eval entirely on every Fresh verdict — and every test would still pass, because every test sets the bypass env.
- **Suggested fix:** Add `tests/cli_eval_freshness_gate_test.rs::eval_runs_against_seeded_store_with_fresh_daemon` — same fixture shape as `test_eval_runs_against_seeded_store` (eval_subcommand_test.rs:100) but **without** the `CQS_EVAL_REQUIRE_FRESH=0` env, with an `XDG_RUNTIME_DIR` pointing at a tempdir hosting a `UnixListener` mock that responds to one `status` request with a Fresh envelope. Assert (a) the eval command exits 0 within 10 s, (b) the `[eval] checking watch-mode freshness` stderr line appears (heads-up message at line 242), (c) the report JSON contains a non-zero `recall_at_5`. Gate behind `slow-tests` like the other CLI tests.

#### TC-HAP-1.30.1-8: `WatchSnapshot::compute` — the `Rebuilding` state entry is untested even though the production state machine has it
- **Difficulty:** easy
- **Location:** src/watch_status.rs (production); src/cli/watch/reconcile.rs:303-388 (only Stale + Fresh tested via compute)
- **Description:** `reconcile_detects_bulk_modify_burst` (reconcile.rs:303) calls `WatchSnapshot::compute` and asserts `state == Stale`. `reconcile_skips_unchanged_files` (line 396) covers the implicit Fresh case but doesn't call `compute` — it asserts on the queue, not on the snapshot. There is **no test** that drives `WatchSnapshot::compute` with `rebuild_in_flight: true` and asserts the resulting state is `Rebuilding`. The four-state enum (`Fresh / Stale / Rebuilding / Unknown` per watch_status.rs:63 region) has only two states pinned through the `compute` entrypoint. A regression that maps `rebuild_in_flight: true` → `Stale` (e.g., a refactor merging the two "not fresh" states into one) silently breaks `cqs status --watch-fresh --wait`'s exit-code distinction between "actively rebuilding, will finish soon" and "stale, no rebuild scheduled". Also untested: `delta_saturated: true` with `pending_files: 0` (TC-ADV-1.30.1-8 already flagged this — but on the adversarial axis; the *happy* axis of "saturated → Rebuilding because we know we missed events" is also unpinned).
- **Suggested fix:** Add a unit test in `src/watch_status.rs` itself (no temp files needed): `compute_with_rebuild_in_flight_returns_rebuilding` — feed `WatchSnapshotInput { rebuild_in_flight: true, pending_files_count: 0, ... }`, assert `snap.state == FreshnessState::Rebuilding`. Add `compute_unknown_when_no_input_observed` if `unknown()` is reachable from `compute` (e.g., a `last_event` of `Instant::now() - 0` with no other signals). Pinning these three states in one place keeps the four-state enum's full transition table inside the test suite.

#### TC-HAP-1.30.1-9: Eval text-report rendering (`print_text_report`) — only the `pct` helper is tested in isolation; the full report's row-by-row format is unpinned
- **Difficulty:** easy
- **Location:** src/cli/commands/eval/mod.rs:296+ (`print_text_report`)
- **Description:** `print_text_report` is the canonical user-facing output of `cqs eval`. The `tests` mod tests `pct` formatting (line 349) and arg-parser flags (lines 360, 383, 405, 435) — none assert on the rendered report shape. The format docstring at line 294-295 explicitly says "Format mirrors the spec exactly so a user comparing old python output against `cqs eval` output can eyeball the same shape". That spec-compatibility guarantee is unenforced: any change to the header row (`=== eval results: <name> (N=<n>) ===`), to the metric ordering (R@K, MRR, etc.), or to the column count silently breaks downstream `grep | awk` consumers — and there is no test that fails. The eval baseline diff workflow (PR-N of #1182) ships an `eval --baseline` mode (line 406) that compares two report files; if `print_text_report` ever drops a column, every diff comparison silently degrades and no test catches it.
- **Suggested fix:** Add `print_text_report_renders_canonical_header_and_metrics` — build a fixture `EvalReport` with deterministic numbers (recall_at_1=0.5, recall_at_5=1.0, mrr=0.75, N=2), capture stdout via the `gag` crate or a test-side `Write` impl, assert the output contains the literal lines `=== eval results: test (N=2) ===`, `R@1: 0.5`, `R@5: 1.0`, `MRR: 0.750` in that order. Pin the output as a multi-line string literal so any column reordering / float-format change produces a single clear diff.

#### TC-HAP-1.30.1-10: `daemon_translate::daemon_reconcile` — happy-path round-trip pinned, but the `args` payload (hook arguments forwarded from git) is never observed end-to-end
- **Difficulty:** easy
- **Location:** src/daemon_translate.rs:537-… (production); 1102+ (tests); src/cli/commands/infra/hook.rs:296-322 (the production caller)
- **Description:** `daemon_reconcile_mock_round_trip` (line 1104) covers the happy `Ok` envelope, but the test passes empty `args` and an explicit `name`. The actual production caller in `cmd_fire` (hook.rs:311) forwards `args` verbatim from git — for `post-rewrite`, that's e.g. `["amend", "abc123"]`; for `post-checkout`, that's `[<old-sha>, <new-sha>, "1"]` (3-element vector with branch flag). No test pins the wire shape: does `daemon_reconcile` JSON-encode the `args` array correctly? Are non-ASCII bytes (Git can technically pass UTF-8 in commit messages used as ref names) preserved? A regression that drops or truncates `args` in the request payload is invisible — the daemon doesn't *use* `args` for the reconcile algorithm itself per hook.rs:80-82 ("Captured verbatim for tracing; not used for the reconcile algorithm itself"), so the daemon side never errors and the response envelope still says ok. The whole tracing audit trail (which hook fired, with which args) silently breaks.
- **Suggested fix:** Add `daemon_reconcile_forwards_hook_args_verbatim` — extend the mock to *capture* the request line (don't just consume it), assert the parsed JSON contains `args: ["abc123", "def456", "1"]` exactly when the caller passes that vec. Optionally also add `daemon_reconcile_forwards_unicode_args` with `["mañana", "🚀"]` to pin UTF-8 round-trip — guards against a future serde-json switch to a binary-string encoding.
