Skip to content

Commit 5123af2

Browse files
abbccddaclaude
andauthored
feat(server): /query structured ai_context + operator-controlled raw-SQL logging (#173)
* docs: design for parameterized /query + caller purpose The POST /query endpoint takes final SQL with literal values inlined and logs the full statement at INFO as an audit trail. Because parameter values live inside the SQL string, that audit line also logs the values, which then propagate to any OTLP collector / log sink. This spec proposes accepting a {name}-templated SQL plus a separate params object (reusing the pipeline handler's injection-safe substitution) so the endpoint logs the template without values, plus an optional caller-supplied `purpose` field logged as structured context. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs: justify parameterization (injection/PII/types) + template-only storage Add a "Why parameterize" section stressing SQL injection, PII/secret protection, type accuracy, and eliminating ad-hoc escaping/serialization as the justification for a separate params channel over merely redacting the log. Make explicit that this version stores only the query template; param values never reach any log, trace, metric, or store. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs: split into raw /query (admin, never logged) + /parameterized_query Rather than overload one endpoint, keep /query for raw final SQL as an admin/console-only surface whose SQL is never logged, and add a new /parameterized_query for agents that takes a {name} template + params + purpose. Only the template and purpose are logged; param values never are. purpose exists only on the new endpoint. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> docs: narrow scope to purpose field + operator raw-SQL log config Drop parameterization and the /parameterized_query endpoint. The endpoint keeps taking raw final SQL; add an optional purpose field, and gate raw-SQL logging behind an opt-in operator config that writes to a local file (the operator's responsibility to secure in this OSS build). Default: raw SQL is not emitted into the general log/trace stream. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> feat(server): add /query purpose field + operator-controlled raw-SQL log Implements docs/superpowers/specs/2026-07-24-query-purpose-and-raw-log-design.md. - /query no longer logs raw SQL: the INFO audit line drops `sql` and keeps a value-free marker (purpose, max_rows, kind, timing); the DEBUG SQL lines are removed. Raw SQL (which may inline secrets/PII) is no longer emitted to logs/traces or OTLP by default. - Add optional `purpose` field to QueryRequest (<= 2000 chars, else 400 parameter_validation_error) so callers/agents document intent. - Add `--query-log <path>` operator flag: a new query_log::QueryLog appends each executed statement (raw sql, purpose, max_rows, timestamp) as one JSON line to a local file the operator secures. Off by default. Tests: query_log unit tests (append behavior) and query_http integration tests (purpose accepted / over-cap rejected / file records raw SQL + purpose). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> cleanup * docs: replace /query purpose with structured ai_context (purpose+session_id) Evolve the single /query endpoint so caller intent is carried in an optional, extensible ai_context JSON object instead of a flat purpose string. When present it must be an object with required non-empty purpose and session_id (groups queries per agent session); other keys are free-form under an overall size cap. Application/console queries omit it. Supersedes the flat purpose field from the 2026-07-24 spec. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(server): replace /query purpose with structured ai_context Implements docs/superpowers/specs/2026-07-28-query-ai-context-design.md. Replace the flat optional `purpose` string on POST /query with an optional `ai_context` JSON object. When present it must be an object carrying two required non-empty strings: `purpose` (<=2000 chars) and `session_id` (<=200 chars, groups queries from one agent session); other keys are free-form and the whole object must serialize to <=4096 bytes. Any violation -> 400 parameter_validation_error. Omitting ai_context is valid (application/console queries), so the change is backward compatible. The value-free INFO audit marker and the opt-in --query-log file now record the full ai_context object in place of purpose; raw SQL is still never logged in the general stream. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(server): enforce plan-value log suppression + durable /query audit store Addresses review on #173: neither the confidentiality nor the durability guarantee the PR described actually held. Confidentiality — removing the handler's `sql` field left literals reaching the trace/OTLP stream, because DataFusion reprints them inside plans logged at DEBUG. `logging::build_env_filter` now pins every plan-printing target (`datafusion*`, `sqlparser`, and the server's datafusion-tracing spans, newly given the explicit `skardi_query_plan` target) to INFO, dropping any RUST_LOG directive that would lower them. `SKARDI_ALLOW_PLAN_VALUE_LOGGING=1` lifts it. Enforcing at the filter covers emitters added by future DataFusion versions, which per-site redaction would not. Durability — the JSONL sink is replaced by a SQLite ledger (`--query-audit-db`, `--query-audit-retention-days`) on tokio-rusqlite, the same backend as the jobs ledger: - async, so no filesystem I/O on Tokio workers and no mutex across a write; - WAL + synchronous=FULL, committed before execution; - created 0600 (before SQLite touches it, so the umask never applies); - record id, timestamps, sql, ai_context, session_id, max_rows, kind, status, row_count, error; indexed on (session_id, created_at), created_at, status; - fail-closed: open/migrate errors abort startup, a failed pre-execution write returns 503 and the statement does not run, and rows left `started` by a crash reconcile to `unknown`. `ai_context: null` now deserializes as present-but-malformed instead of collapsing to absent, so it returns 400 as documented. Tests: tests/query_plan_logging.rs captures subscriber output for a real query and asserts a sentinel literal is absent under RUST_LOG=debug, trace, and targeted datafusion directives, with a positive control proving the harness sees plan output. Plus audit-store unit tests (round trip, orphan reconcile, retention, 0600, reopen) and HTTP tests for success/failure records, the fail-closed 503, and the null rejection. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent eb67a2d commit 5123af2

20 files changed

Lines changed: 2194 additions & 85 deletions

Cargo.lock

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

crates/server/Cargo.toml

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,8 @@ skardi = { path = "../skardi" }
5050
anyhow = "1.0"
5151
base64 = "0.22"
5252
thiserror = "1.0"
53-
tokio = { workspace = true, features = ["macros", "rt-multi-thread", "net", "sync"] }
53+
tokio = { workspace = true, features = ["macros", "rt-multi-thread", "net", "sync", "time"] }
54+
tokio-rusqlite = { workspace = true }
5455
tower = { workspace = true }
5556
tower-http = { workspace = true, features = ["fs", "trace", "cors"] }
5657
tracing = { workspace = true }

crates/server/src/auth/routes.rs

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -382,11 +382,13 @@ mod tests {
382382
ctx_file: None,
383383
semantics_path: None,
384384
port: 8080,
385+
query_audit_db: None,
386+
query_audit_retention_days: None,
385387
},
386388
};
387389
let session_ctx = Arc::new(SessionContext::new());
388390
let engine = Arc::new(DataFusionEngine::new_with_arc(session_ctx.clone()));
389-
AppState::new(config, engine, session_ctx, AuthLayer::None, None)
391+
AppState::new(config, engine, session_ctx, AuthLayer::None, None, None)
390392
}
391393

392394
#[tokio::test]
@@ -428,11 +430,13 @@ mod tests {
428430
ctx_file: None,
429431
semantics_path: None,
430432
port: 8080,
433+
query_audit_db: None,
434+
query_audit_retention_days: None,
431435
},
432436
};
433437
let session_ctx = Arc::new(SessionContext::new());
434438
let engine = Arc::new(DataFusionEngine::new_with_arc(session_ctx.clone()));
435-
AppState::new(config, engine, session_ctx, layer, None)
439+
AppState::new(config, engine, session_ctx, layer, None, None)
436440
}
437441

438442
#[tokio::test]

crates/server/src/config.rs

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,26 @@ pub struct CliArgs {
8383
/// Server port number
8484
#[arg(long, default_value = "8080", help = "Server port number")]
8585
pub port: u16,
86+
87+
/// Path to the SQLite audit ledger for ad-hoc `/query` statements. When
88+
/// unset (the default), raw SQL is never persisted and never written to
89+
/// logs or traces. The ledger records raw SQL (which may embed
90+
/// secrets/PII); it is created owner-only, and a failure to open it is
91+
/// fatal rather than a silent downgrade. See [`crate::query_audit`].
92+
#[arg(
93+
long = "query-audit-db",
94+
help = "Record /query statements in this SQLite audit ledger (off by default; created 0600)"
95+
)]
96+
pub query_audit_db: Option<PathBuf>,
97+
98+
/// Delete audit records older than this many days, at startup and hourly
99+
/// thereafter. Unset means keep everything. Ignored without
100+
/// `--query-audit-db`.
101+
#[arg(
102+
long = "query-audit-retention-days",
103+
help = "Prune /query audit records older than N days (default: keep forever)"
104+
)]
105+
pub query_audit_retention_days: Option<u32>,
86106
}
87107

88108
/// Main server configuration containing pipelines and data sources
@@ -1966,6 +1986,8 @@ spec:
19661986
ctx_file: Some(context_path),
19671987
semantics_path: None,
19681988
port: 8080,
1989+
query_audit_db: None,
1990+
query_audit_retention_days: None,
19691991
};
19701992

19711993
let config = load_server_config(args).await.unwrap();
@@ -1991,6 +2013,8 @@ spec:
19912013
ctx_file: None,
19922014
semantics_path: None,
19932015
port: 3000,
2016+
query_audit_db: None,
2017+
query_audit_retention_days: None,
19942018
};
19952019

19962020
let config = load_server_config(args).await.unwrap();
@@ -2045,6 +2069,8 @@ spec:
20452069
ctx_file: None,
20462070
semantics_path: None,
20472071
port: 3000,
2072+
query_audit_db: None,
2073+
query_audit_retention_days: None,
20482074
};
20492075

20502076
let config = load_server_config(args).await.unwrap();
@@ -2073,6 +2099,8 @@ spec:
20732099
ctx_file: None,
20742100
semantics_path: None,
20752101
port: 8080,
2102+
query_audit_db: None,
2103+
query_audit_retention_days: None,
20762104
};
20772105

20782106
let result = load_server_config(args).await;
@@ -2097,6 +2125,8 @@ spec:
20972125
ctx_file: None,
20982126
semantics_path: None,
20992127
port: 8080,
2128+
query_audit_db: None,
2129+
query_audit_retention_days: None,
21002130
};
21012131

21022132
let config = load_server_config(args).await.unwrap();

crates/server/src/lib.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,11 @@ pub mod config;
33
pub mod gui;
44
pub mod handlers;
55
pub mod jobs_handlers;
6+
pub mod logging;
67
pub mod metrics;
78
pub mod optimizer_registry;
89
pub mod pipeline_handlers;
10+
pub mod query_audit;
911
pub mod query_handlers;
1012
pub mod remote_storage;
1113
pub mod response;

crates/server/src/logging.rs

Lines changed: 267 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,267 @@
1+
//! Tracing filter construction, including the hard floor that keeps
2+
//! value-bearing query plans out of the log/OTLP stream.
3+
//!
4+
//! `POST /query` deliberately never logs the raw statement (see
5+
//! [`crate::query_handlers`]) because callers may inline literal secrets or
6+
//! PII. Suppressing the handler's own `sql` field is not enough on its own:
7+
//! DataFusion reconstructs the *same literals* inside the plans it prints at
8+
//! DEBUG, e.g.
9+
//!
10+
//! ```text
11+
//! Projection: Utf8("TOP_SECRET") <- datafusion_optimizer::utils::log_plan
12+
//! ProjectionExec: expr=[TOP_SECRET as ...] <- datafusion-tracing span field
13+
//! ```
14+
//!
15+
//! Those lines flow through the same subscriber — and therefore the same OTLP
16+
//! exporter — as everything else. So the confidentiality guarantee is enforced
17+
//! here, at the filter: every target known to print plans is pinned to INFO and
18+
//! cannot be lowered by `RUST_LOG`.
19+
//!
20+
//! An operator who is knowingly debugging a planning problem on non-sensitive
21+
//! data can lift the floor with `SKARDI_ALLOW_PLAN_VALUE_LOGGING=1`. It is an
22+
//! explicit, separate opt-in precisely because it re-enables value export.
23+
24+
use tracing_subscriber::EnvFilter;
25+
26+
/// Env var that lifts the plan-value logging floor. Any value other than
27+
/// `0`/`false`/`no`/empty enables plan logging.
28+
pub const ALLOW_PLAN_VALUE_LOGGING_ENV: &str = "SKARDI_ALLOW_PLAN_VALUE_LOGGING";
29+
30+
/// Tracing target used for the datafusion-tracing execution/rule spans this
31+
/// server installs (see [`crate::server::setup_app_state`]).
32+
///
33+
/// Those macros default their target to `module_path!()` of the *call site*,
34+
/// which would bury the spans under `skardi_server::server` where no filter
35+
/// could single them out. Naming the target explicitly is what makes the floor
36+
/// below able to cover them.
37+
pub const QUERY_PLAN_TARGET: &str = "skardi_query_plan";
38+
39+
/// Target prefixes whose DEBUG/TRACE records embed query plans, and with them
40+
/// the literal values from the statement.
41+
///
42+
/// Matching is by prefix, mirroring `EnvFilter`'s own target matching, so
43+
/// `datafusion` also covers `datafusion_optimizer`, `datafusion_sql`,
44+
/// `datafusion_physical_optimizer`, `datafusion_federation` and
45+
/// `datafusion_tracing`.
46+
const PLAN_VALUE_TARGET_PREFIXES: &[&str] = &["datafusion", QUERY_PLAN_TARGET, "sqlparser"];
47+
48+
/// Build the tracing filter from the given `RUST_LOG` value (`None` when the
49+
/// variable is unset or invalid unicode), defaulting to `info`.
50+
///
51+
/// Two hard caps are applied on top of whatever the operator asked for:
52+
///
53+
/// * aws_config logs the AWS access key id in plaintext at INFO when resolving
54+
/// credentials, so it is capped at WARN unless `RUST_LOG` explicitly opts in.
55+
/// * Every target in [`PLAN_VALUE_TARGET_PREFIXES`] is pinned to INFO unless
56+
/// `allow_plan_value_logging` is set. Directives in `RUST_LOG` that would
57+
/// lower one of those targets are dropped before parsing, so neither a global
58+
/// `RUST_LOG=debug` nor a targeted `RUST_LOG=datafusion_optimizer=debug` can
59+
/// put query literals on the wire.
60+
pub fn build_env_filter(rust_log: Option<&str>, allow_plan_value_logging: bool) -> EnvFilter {
61+
let retained: Vec<&str> = match rust_log {
62+
Some(v) if !allow_plan_value_logging => v
63+
.split(',')
64+
.filter(|d| !directive_leaks_plan_values(d))
65+
.collect(),
66+
Some(v) => v.split(',').collect(),
67+
None => Vec::new(),
68+
};
69+
70+
// Dropping every directive leaves an empty string, which `EnvFilter` reads
71+
// as "enable nothing" rather than "unset" — fall back to the default.
72+
let sanitized = retained.join(",");
73+
let mut env_filter = Some(sanitized.as_str())
74+
.filter(|s| !s.trim().is_empty())
75+
.and_then(|v| EnvFilter::try_new(v).ok())
76+
.unwrap_or_else(|| "info".into());
77+
78+
if !allow_plan_value_logging {
79+
for prefix in PLAN_VALUE_TARGET_PREFIXES {
80+
// Anything the operator set for this exact target survived the
81+
// filter above, so it is already at or above the floor — adding the
82+
// floor would *raise* it (an explicit `datafusion=off` would come
83+
// back as `info`, since `add_directive` replaces by target).
84+
if retained.iter().any(|d| directive_target(d) == Some(prefix)) {
85+
continue;
86+
}
87+
env_filter = env_filter.add_directive(
88+
format!("{prefix}=info")
89+
.parse()
90+
.expect("valid plan-value floor directive"),
91+
);
92+
}
93+
}
94+
95+
if !rust_log.is_some_and(|v| v.contains("aws_config")) {
96+
env_filter = env_filter.add_directive("aws_config=warn".parse().expect("valid directive"));
97+
}
98+
env_filter
99+
}
100+
101+
/// Read the plan-logging opt-out from the environment.
102+
pub fn allow_plan_value_logging_from_env() -> bool {
103+
std::env::var(ALLOW_PLAN_VALUE_LOGGING_ENV).is_ok_and(|v| {
104+
!matches!(
105+
v.trim().to_ascii_lowercase().as_str(),
106+
"" | "0" | "false" | "no" | "off"
107+
)
108+
})
109+
}
110+
111+
/// Whether one comma-separated `RUST_LOG` directive would enable DEBUG/TRACE
112+
/// output for a plan-printing target.
113+
///
114+
/// Bare level names (`debug`) set the *global default* and are left alone — the
115+
/// per-target floor directives out-specify them. A bare target (`datafusion`)
116+
/// means TRACE for that target in env_logger syntax, so it counts as a leak.
117+
fn directive_leaks_plan_values(directive: &str) -> bool {
118+
let Some(target) = directive_target(directive) else {
119+
return false;
120+
};
121+
if !PLAN_VALUE_TARGET_PREFIXES
122+
.iter()
123+
.any(|prefix| target.starts_with(prefix))
124+
{
125+
return false;
126+
}
127+
128+
let level = directive
129+
.trim()
130+
.rsplit_once('=')
131+
.map(|(_, level)| level.trim());
132+
match level {
133+
// `datafusion` / `datafusion=` both mean TRACE.
134+
None | Some("") => true,
135+
Some(level) => {
136+
let level = level.to_ascii_lowercase();
137+
level == "debug" || level == "trace" || level.parse::<u8>().is_ok_and(|n| n >= 4)
138+
}
139+
}
140+
}
141+
142+
/// The target a directive names, with any span filter stripped
143+
/// (`datafusion[span{k=v}]=debug` -> `datafusion`). `None` for a bare level
144+
/// name, which sets the global default rather than naming a target.
145+
fn directive_target(directive: &str) -> Option<&str> {
146+
let directive = directive.trim();
147+
if directive.is_empty() {
148+
return None;
149+
}
150+
let target = match directive.rsplit_once('=') {
151+
Some((target, _)) => target,
152+
None if is_level_name(directive) => return None,
153+
None => directive,
154+
};
155+
Some(target.split('[').next().unwrap_or(target).trim())
156+
}
157+
158+
/// Whether a bare `RUST_LOG` token is a level (global default) rather than a
159+
/// target name.
160+
fn is_level_name(token: &str) -> bool {
161+
matches!(
162+
token.to_ascii_lowercase().as_str(),
163+
"off" | "error" | "warn" | "info" | "debug" | "trace"
164+
) || token.parse::<u8>().is_ok()
165+
}
166+
167+
#[cfg(test)]
168+
mod tests {
169+
use super::*;
170+
171+
#[test]
172+
fn env_filter_defaults_to_info_and_caps_aws_config() {
173+
let filter = build_env_filter(None, false).to_string();
174+
assert!(filter.contains("info"), "got {filter}");
175+
assert!(filter.contains("aws_config=warn"), "got {filter}");
176+
}
177+
178+
#[test]
179+
fn env_filter_caps_aws_config_for_unrelated_rust_log() {
180+
let filter = build_env_filter(Some("debug"), false).to_string();
181+
assert!(filter.contains("debug"), "got {filter}");
182+
assert!(filter.contains("aws_config=warn"), "got {filter}");
183+
}
184+
185+
#[test]
186+
fn env_filter_honors_explicit_aws_config_opt_in() {
187+
let filter = build_env_filter(Some("info,aws_config=debug"), false).to_string();
188+
assert!(filter.contains("aws_config=debug"), "got {filter}");
189+
assert!(!filter.contains("aws_config=warn"), "got {filter}");
190+
}
191+
192+
#[test]
193+
fn env_filter_falls_back_to_info_on_invalid_rust_log() {
194+
let filter = build_env_filter(Some("not a [valid directive"), false).to_string();
195+
assert!(filter.contains("info"), "got {filter}");
196+
assert!(filter.contains("aws_config=warn"), "got {filter}");
197+
}
198+
199+
#[test]
200+
fn global_debug_does_not_lower_plan_targets() {
201+
let filter = build_env_filter(Some("debug"), false).to_string();
202+
for prefix in PLAN_VALUE_TARGET_PREFIXES {
203+
assert!(filter.contains(&format!("{prefix}=info")), "got {filter}");
204+
}
205+
}
206+
207+
#[test]
208+
fn targeted_plan_debug_directives_are_dropped() {
209+
let filter = build_env_filter(
210+
Some(
211+
"info,datafusion_optimizer=debug,datafusion_tracing=trace,skardi_query_plan=debug",
212+
),
213+
false,
214+
)
215+
.to_string();
216+
assert!(
217+
!filter.contains("datafusion_optimizer=debug"),
218+
"got {filter}"
219+
);
220+
assert!(!filter.contains("datafusion_tracing=trace"), "got {filter}");
221+
assert!(!filter.contains("skardi_query_plan=debug"), "got {filter}");
222+
assert!(filter.contains("datafusion=info"), "got {filter}");
223+
}
224+
225+
#[test]
226+
fn bare_plan_target_directive_is_dropped() {
227+
// `RUST_LOG=datafusion` means TRACE for that target.
228+
let filter = build_env_filter(Some("datafusion"), false).to_string();
229+
assert!(filter.contains("datafusion=info"), "got {filter}");
230+
assert!(filter.contains("info"), "got {filter}");
231+
}
232+
233+
#[test]
234+
fn plan_only_rust_log_falls_back_to_info() {
235+
let filter = build_env_filter(Some("datafusion=debug"), false).to_string();
236+
assert!(filter.contains("info"), "got {filter}");
237+
}
238+
239+
#[test]
240+
fn non_plan_targets_keep_their_debug_level() {
241+
let filter = build_env_filter(Some("warn,skardi_server=debug"), false).to_string();
242+
assert!(filter.contains("skardi_server=debug"), "got {filter}");
243+
}
244+
245+
#[test]
246+
fn raising_plan_targets_is_still_allowed() {
247+
// Only *lowering* is blocked; `off`/`warn` are honored as written.
248+
let filter = build_env_filter(Some("debug,datafusion=off"), false).to_string();
249+
assert!(filter.contains("datafusion=off"), "got {filter}");
250+
}
251+
252+
#[test]
253+
fn explicit_opt_in_lifts_the_floor() {
254+
let filter = build_env_filter(Some("datafusion_optimizer=debug"), true).to_string();
255+
assert!(
256+
filter.contains("datafusion_optimizer=debug"),
257+
"got {filter}"
258+
);
259+
assert!(!filter.contains("datafusion=info"), "got {filter}");
260+
}
261+
262+
#[test]
263+
fn span_filter_syntax_is_recognised() {
264+
assert!(directive_leaks_plan_values("datafusion[span{k=v}]=debug"));
265+
assert!(!directive_leaks_plan_values("datafusion[span{k=v}]=info"));
266+
}
267+
}

0 commit comments

Comments
 (0)