feat(server): /query structured ai_context + operator-controlled raw-SQL logging

[abbccdda]

Context

POST /query (#158) takes final SQL with literal values inlined and logged the full statement at INFO (sql = %request.sql). Since that's a tracing field, it was also a values log that fans out to any OTLP collector / log sink — exposing any secret/PII passed as a literal.

What this PR does

Adds the design specs and their implementation. Changes to the single POST /query endpoint:

1. Structured ai_context object (optional). Application/console queries omit it. When present it must be a JSON object carrying two required non-empty strings — purpose (≤ 2000 chars, why the query runs) and session_id (≤ 200 chars, groups queries from one agent session) — plus any free-form keys the caller wants. The whole object must serialize to ≤ 4096 bytes. Recorded for observability, never executed; any violation → parameter_validation_error. This replaces the flat purpose string explored earlier on the branch, and leaves room for callers to design their own query context over time.
2. Raw SQL is no longer emitted into the general log/trace/OTLP stream. The INFO audit marker is now value-free — it carries only max_rows, kind, and ai_context, never the SQL text. Rejection/execution error paths log the reason but not the statement.
3. Opt-in operator audit sink. An operator who wants a raw-SQL audit trail passes --query-log <path>; the server appends one JSON line per executed statement (timestamp, sql, ai_context, max_rows) to that local file. Off by default. Because the file holds raw SQL (possibly secrets/PII), securing/rotating/retaining it is the operator's responsibility. It is the only sink that writes query text, so enabling it never leaks SQL to external collectors by accident.

Implementation

• crates/server/src/query_log.rs — append-only QueryLog sink (JSON Lines, mutex-guarded File); record takes the ai_context object. Write failures are logged and swallowed so a broken audit file never fails the query.
• crates/server/src/query_handlers.rs — ai_context field with validate_ai_context/validate_context_string (object shape, required purpose/session_id, size caps), value-free audit marker, records to the query log before execution.
• crates/server/src/config.rs — --query-log CLI arg.
• crates/server/src/server.rs — opens the sink at startup and threads it into AppState.
• Design docs: docs/superpowers/specs/2026-07-24-query-purpose-and-raw-log-design.md (raw-SQL logging + original flat purpose) and docs/superpowers/specs/2026-07-28-query-ai-context-design.md (evolves purpose into the structured ai_context). docs/server.md updated. The docs also record the rejected alternatives explored earlier on this branch (SQL parameterization; a separate /parameterized_query endpoint; a required ai_context).

Tests

crates/server/tests/query_http.rs covers ai_context accepted (with free-form keys) / omitted, and every validation path (not an object, missing purpose, missing session_id, empty/over-long purpose, over-long session_id, over-size object), plus the operator query-log file (raw SQL + ai_context recorded when configured; nothing written when not). query_log.rs has unit tests for append/one-line-per-record.

[codecov]

Codecov Report

❌ Patch coverage is 88.46676% with 85 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
crates/server/src/server.rs	50.76%	32 Missing ⚠️
crates/server/src/query_audit.rs	93.26%	27 Missing ⚠️
crates/server/src/query_handlers.rs	86.60%	15 Missing ⚠️
crates/server/src/logging.rs	94.11%	8 Missing ⚠️
crates/server/src/main.rs	0.00%	3 Missing ⚠️

📢 Thoughts on this report? Let us know!

[BtXin]

Requesting changes because the current implementation does not yet provide the confidentiality or durability guarantees described by this PR.

1. Sensitive SQL literals still reach the general tracing/OTLP stream at DEBUG. Removing the handler's explicit sql fields is not sufficient: the server enables DataFusion analyzer/optimizer/physical-plan tracing, and those plans contain literal values. On this commit, running SELECT 'TOP_SECRET_PR173' AS secret with DEBUG tracing emitted Projection: Utf8("TOP_SECRET_PR173") and ProjectionExec: expr=[TOP_SECRET_PR173 ...] repeatedly. The complete statement may be absent, but the secret/PII values motivating this change are still exported. Please redact literals or enforce a query execution/logging configuration that cannot emit value-bearing plans, and add a regression test that captures DEBUG/OTLP output and asserts a sentinel literal is absent.

2. The query-log write blocks Tokio request threads. QueryLog::record holds a std::sync::Mutex<File> and performs synchronous filesystem I/O directly from the async handler. A slow or network-mounted path can stall runtime workers, while concurrent requests block behind the same mutex. The persistence path needs to be asynchronous or isolated behind a dedicated writer.

3. ai_context: null bypasses the documented validation. Because the field is Option<serde_json::Value>, both a missing field and explicit JSON null deserialize to None. I added null to the exact-head HTTP test and it returned 200 instead of the documented 400 parameter_validation_error. Please preserve field presence during deserialization and add this case to the test suite.

4. A newly created raw-query log is not private by default. OpenOptions::create(true) uses normal process permissions; with the common 022 umask the file is typically 0644, readable by other local users. Any storage containing raw SQL, secrets, or PII needs secure creation/access permissions by default.

5. Use proper durable storage for the query audit log instead of an append-only JSONL file. Please back the audit log with an embedded persistent store such as SQLite, RocksDB, or LMDB. SQLite is likely the lowest-complexity option because this repository already uses it. The implementation should provide:

   • durable, transactional insertion before execution;
   • a stable record ID and timestamps;
   • SQL, ai_context, session_id, max_rows, and statement kind;
   • an execution outcome/status updated after completion or failure;
   • indexes for session/time lookup;
   • explicit retention/rotation behavior;
   • secure file/database permissions;
   • defined failure semantics.

When auditing is enabled, startup or migration failures should not silently disable it, and a failed pre-execution audit write should not allow an unrecorded query to run.

The dedicated audit store can remain opt-in, but once enabled it needs to be trustworthy, durable, and queryable rather than best-effort.

[abbccdda]

Thanks — all five points were real. Pushed f701c9c. Point-by-point:

1. Sensitive literals at DEBUG — fixed at the filter, not per-site

You're right that dropping the handler's sql field was cosmetic. The literals come back through two independent paths: DataFusion's log_plan helper (Projection: Utf8("…") at DEBUG from datafusion_optimizer::utils) and the datafusion-tracing span fields (datafusion.node = the node display).

I chose enforcement over redaction. Redacting means patching or wrapping upstream plan Display impls — an emitter list that goes stale the next time DataFusion adds a debug log. Instead, logging::build_env_filter (new crates/server/src/logging.rs, moved out of main.rs) applies a floor to the whole subscriber registry, which is the single point both the fmt layer and the OTLP layer sit behind:

• PLAN_VALUE_TARGET_PREFIXES = ["datafusion", "skardi_query_plan", "sqlparser"], prefix-matched the way EnvFilter matches, so one entry covers datafusion_optimizer, datafusion_sql, datafusion_federation, datafusion_tracing, …
• Any RUST_LOG directive that would put one of those at DEBUG/TRACE is dropped before parsing, so a targeted datafusion_optimizer=debug can't out-specify the floor by being longer.
• Each prefix then gets =info — unless the operator already set that exact target at or above the floor, so an explicit datafusion=off stays off (the floor raises a minimum, it doesn't clamp).

The datafusion-tracing instrumentations needed a change to be coverable at all: both macros default target: to module_path!() of the call site, which buried the spans under skardi_server::server. They're now pinned to skardi_query_plan, and setup_app_state's instrumentation block is extracted into server::instrument_session_state so tests exercise the real thing.

Escape hatch: SKARDI_ALLOW_PLAN_VALUE_LOGGING=1, a separate explicit opt-in since it re-enables value export.

Regression test — crates/server/tests/query_plan_logging.rs runs a real query through that same instrumented SessionState under a capturing subscriber and asserts TOP_SECRET_PR173 is absent, for RUST_LOG=debug, RUST_LOG=trace, and info,datafusion=trace,datafusion_optimizer=debug,skardi_query_plan=trace. A fourth test is a positive control: with the opt-in on, it asserts the sentinel is present — otherwise the other three would pass just as well if the harness captured nothing.

Also re-ran your live repro end to end. RUST_LOG=debug ./skardi-server --port 18099 --query-audit-db …, then SELECT 'TOP_SECRET_PR173' AS secret: grep -c TOP_SECRET_PR173 server.log → 0. The INFO marker is intact and value-free:

INFO skardi_server::query_handlers: Executing ad-hoc query max_rows=1000 kind=Query ai_context="{\"purpose\":\"repro finding 1\",\"session_id\":\"sess-repro\"}"

2 + 4 + 5. JSONL sink replaced by a durable SQLite ledger

Agreed the file was the wrong primitive — fixing the blocking write and the mode bits would still have left no outcome, no id, no index, no transaction, no retention. query_log.rs is deleted; crates/server/src/query_audit.rs backs the trail with SQLite over tokio-rusqlite, the same crate and pattern as the jobs ledger.

CLI: --query-log <path> → --query-audit-db <path>, plus --query-audit-retention-days <n>.

Against your checklist:

• Async (Fix Repo address #2) — tokio_rusqlite owns a dedicated blocking thread; no filesystem I/O on a Tokio worker, no std::sync::Mutex held across a write.
• Durable, transactional insertion before execution — journal_mode=WAL, synchronous=FULL; record_started commits and returns the id before the engine is called.
• Stable id + timestamps — id PK, created_at / finished_at RFC 3339.
• Fields — sql, ai_context (verbatim), session_id (denormalised out of the context so the index doesn't need JSON parsing), max_rows, statement_kind.
• Outcome updated after completion/failure — started → succeeded / failed, with row_count and error.
• Indexes — (session_id, created_at DESC), created_at DESC, status.
• Retention/rotation — --query-audit-retention-days prunes at startup (awaited, so a bad setup surfaces immediately) and hourly after. Unset = keep forever, documented as the operator's call.
• Permissions ([Feature Request] Support write for csv #4) — the file is created 0600 before SQLite touches it, so the umask never applies; the WAL sidecars inherit it. Verified live: -rw------- audit.db, audit.db-wal, audit.db-shm.
• Failure semantics — startup open/migrate failure is fatal (verified: exits 1 with Failed to open --query-audit-db at …, no silent disable). A failed pre-execution write returns 503 query_audit_error and the statement does not execute. A failed post-execution update is logged only — the query already ran, so the row stays started and startup reconciles it to unknown, same as rows killed mid-flight.

Live check of a recorded row:

{"id":"18c77aa13df20516-0","created_at":"2026-07-31T20:43:30.223+00:00","finished_at":"2026-07-31T20:43:30.232+00:00",
 "sql":"SELECT 'TOP_SECRET_PR173' AS secret","ai_context":"{\"purpose\":\"repro finding 1\",\"session_id\":\"sess-repro\"}",
 "session_id":"sess-repro","max_rows":1000,"statement_kind":"Query","status":"succeeded","row_count":1,"error":null}

3. ai_context: null

#[serde(default, deserialize_with = "deserialize_present")] maps a present field — including null — to Some, so only an omitted field yields None. The null then fails the existing "must be a JSON object" check. Verified live: {"sql":"SELECT 1","ai_context":null} → 400 ai_context must be a JSON object, and covered by explicit_null_ai_context_rejected.

Tests and docs

223 tests pass (cargo test -p skardi-server), no new clippy warnings. New coverage beyond the plan-logging suite: audit round trip, failure detail, session-ordered lookup, orphan reconciliation, retention pruning, 0600 on disk, durability across reopen; and at the HTTP layer — success and engine-failure records, rejected statements leaving no record, the fail-closed 503, and the null rejection.

Docs: docs/server.md gains "Query confidentiality" and "Query audit ledger" sections (schema table, failure semantics, retention). New spec docs/superpowers/specs/2026-07-31-query-audit-store-design.md records the design and the rejected alternatives; the two earlier specs are marked superseded where they now contradict it.

One judgement call worth flagging: I dropped --query-log outright rather than keeping it as a deprecated alias, since it has never shipped outside this branch.