Skip to content

Commit f550901

Browse files
committed
Merge remote-tracking branch 'origin/main' into fix/pr62-16-resolve-dataset-token-match
# Conflicts: # docs/tool-api.md # src/tool_executor.rs
2 parents 6f67243 + 37f0997 commit f550901

2 files changed

Lines changed: 238 additions & 47 deletions

File tree

‎docs/tool-api.md‎

Lines changed: 28 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,8 @@ top-level set operation such as `UNION` and to a parenthesized outer query.
148148
Projections inside CTE definitions, derived tables and subqueries are not
149149
checked. Casts to `INT64`, `UINT64` or `FLOAT64` anywhere in the statement are
150150
rejected with a hint to use `BIGINT` or `DOUBLE`. The statement is wrapped with
151-
an outer `LIMIT 500`. `sample_events`
151+
an outer `LIMIT 500`, except in `get_trace_activity`, whose generated SQL carries
152+
its own limit (see below). `sample_events`
152153
returns at most 10 columns, choosing common observability fields such as
153154
`p_timestamp`, `service.name`, `severity_text`, `body` and `trace_id` before
154155
other fields.
@@ -160,23 +161,26 @@ datasets.
160161

161162
`resolve_dataset` accepts an optional `telemetryType` (`logs`, `metrics` or
162163
`traces`) and `limit` (1–20, default 5). Its `status` is `resolved`,
163-
`ambiguous`, `not_found` or `error`. Dataset names are matched
164-
case-insensitively.
164+
`ambiguous`, `not_found` or `error`.
165165

166166
- A `selectedDataset` or `contextDataset` resolves to the visible dataset with
167-
that name. If no visible dataset has that name, the result has
168-
`status: "error"`. If its telemetry type does not match `telemetryType`, the
169-
result is `not_found`.
167+
exactly that (case-sensitive) name. If none has that exact name, it resolves
168+
to the one visible dataset whose name matches case-insensitively; if several
169+
do, the result is `ambiguous` with those datasets as `candidates`. If no
170+
visible dataset matches, the result has `status: "error"`. If the selected
171+
dataset's telemetry type does not match `telemetryType`, the result is
172+
`not_found`.
170173
- Otherwise, if exactly one dataset is visible, it resolves to that dataset, or
171174
returns `not_found` if its telemetry type does not match.
172175
- Otherwise it scores the visible datasets that match `telemetryType` by name
173-
against `query`: 1000 if the name equals the query, 750 if the query contains
174-
the name as a whole word (`-` and `_` count as part of a name, so `api` is not
175-
in `rapid` or `api-gateway`), or 500 if the name contains the query, plus 25
176-
for each query term longer than two characters that the name contains, plus
177-
15 when `telemetryType` is given. It resolves only a unique top score of at least
178-
500. In every other case it returns `ambiguous` with up to `limit` scored
179-
`candidates`, or `not_found` if no visible dataset matches `telemetryType`.
176+
against `query`, ignoring case: 1000 if the name equals the query, 750 if the
177+
query contains the name as a whole word (`-` and `_` count as part of a name,
178+
so `api` is not in `rapid` or `api-gateway`), or 500 if the name contains the
179+
query, plus 25 for each query term longer than two characters that the name
180+
contains, plus 15 when `telemetryType` is given. It resolves only a unique top
181+
score of at least 500. In every other case it returns `ambiguous` with up to
182+
`limit` scored `candidates`, or `not_found` if no visible dataset matches
183+
`telemetryType`.
180184

181185
It never falls back to an arbitrary dataset.
182186

@@ -244,14 +248,17 @@ dataset's time partition, or `p_timestamp` if none is configured. The dataset
244248
must have traces telemetry and contain `span_span_id` and `span_trace_id`.
245249
Unknown fields are rejected. `span_count` is `COUNT(DISTINCT span_span_id)` and
246250
`trace_count` is `COUNT(DISTINCT span_trace_id)`, so duplicate span rows are
247-
not double counted. Rows are ordered by bucket. The result contains `query`,
248-
`bucketInterval`, `semantics` and up to 500 `rows`.
249-
250-
`query` is the aggregation SQL before the tool wraps it with `LIMIT 500`. It
251-
has no time predicate: the time range comes from the request's `startTime` and
252-
`endTime`, which the query API rounds down to the minute. To reproduce the
253-
result, for example in a dashboard tile, run `query` through the query API with
254-
the same `startTime` and `endTime`.
251+
not double counted. Rows are ordered by bucket, oldest first. The result
252+
contains `query`, `bucketInterval`, `semantics`, up to 500 `rows` and
253+
`truncated`.
254+
255+
`query` is the exact SQL the tool runs; it ends in `LIMIT 501` so the tool can
256+
detect overflow. When more than 500 rows match, the newest buckets are dropped
257+
and `truncated` is `true`; reduce `bucketCount`, `groupBy` or the time range to
258+
get the full series. `query` has no time predicate: the time range comes from
259+
the request's `startTime` and `endTime`, which the query API rounds down to the
260+
minute. To reproduce the result, for example in a dashboard tile, run `query`
261+
through the query API with the same `startTime` and `endTime`.
255262

256263
## Argument schema validation
257264

‎src/tool_executor.rs‎

Lines changed: 210 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -384,12 +384,43 @@ pub fn resolve_dataset_candidates(payload: Value, arguments: &Value) -> Value {
384384
.and_then(Value::as_str)
385385
.filter(|name| !name.is_empty());
386386
if let Some(requested_dataset) = requested_dataset {
387-
let selected = datasets.iter().find(|dataset| {
388-
dataset
389-
.get("name")
390-
.and_then(Value::as_str)
391-
.is_some_and(|name| name.eq_ignore_ascii_case(requested_dataset))
392-
});
387+
// Dataset names are case-sensitive: prefer an exact match and fall
388+
// back to a case-insensitive match only when it is unique.
389+
fn dataset_name(dataset: &Value) -> Option<&str> {
390+
dataset.get("name").and_then(Value::as_str)
391+
}
392+
let exact = datasets
393+
.iter()
394+
.find(|dataset| dataset_name(dataset) == Some(requested_dataset));
395+
let case_insensitive = datasets
396+
.iter()
397+
.filter(|dataset| {
398+
dataset_name(dataset)
399+
.is_some_and(|name| name.eq_ignore_ascii_case(requested_dataset))
400+
})
401+
.collect::<Vec<_>>();
402+
let selected = match (exact, case_insensitive.as_slice()) {
403+
(Some(dataset), _) => Some(dataset),
404+
(None, [dataset]) => Some(*dataset),
405+
(None, []) => None,
406+
(None, matches) => {
407+
return json!({
408+
"status": "ambiguous",
409+
"datasets": [],
410+
"candidates": matches
411+
.iter()
412+
.map(|dataset| json!({
413+
"name": dataset_name(dataset),
414+
"reasons": ["case-insensitive dataset name match"],
415+
"dataset": dataset,
416+
}))
417+
.collect::<Vec<_>>(),
418+
"message": format!(
419+
"Dataset `{requested_dataset}` matches several caller-visible datasets that differ only in case. Call resolve_dataset again with the exact dataset name."
420+
),
421+
});
422+
}
423+
};
393424
return match selected {
394425
Some(dataset) if dataset_matches_telemetry_type(dataset, telemetry_type.as_deref()) => {
395426
json!({
@@ -509,9 +540,16 @@ pub fn resolve_dataset_candidates(payload: Value, arguments: &Value) -> Value {
509540
"candidates": candidates,
510541
"query": query,
511542
"message": if datasets.is_empty() {
512-
"No caller-visible datasets were found."
543+
"No caller-visible datasets were found.".to_owned()
544+
} else if candidates.is_empty() {
545+
match telemetry_type.as_deref() {
546+
Some(telemetry_type) => format!(
547+
"No caller-visible datasets match requested telemetry type `{telemetry_type}`."
548+
),
549+
None => "No caller-visible datasets match the request.".to_owned(),
550+
}
513551
} else {
514-
"Dataset resolution is ambiguous. Inspect candidate schemas, then call resolve_dataset again with selectedDataset."
552+
"Dataset resolution is ambiguous. Inspect candidate schemas, then call resolve_dataset again with selectedDataset.".to_owned()
515553
}
516554
})
517555
}
@@ -1418,15 +1456,32 @@ fn trace_activity_query(
14181456
.first()
14191457
.map(|_| format!(" WHERE {}", clauses.join(" AND ")))
14201458
.unwrap_or_default();
1459+
// One row past the tool limit lets the caller detect truncation.
14211460
format!(
1422-
"SELECT {} FROM {}{} GROUP BY {} ORDER BY 1 LIMIT 500",
1461+
"SELECT {} FROM {}{} GROUP BY {} ORDER BY 1 LIMIT {}",
14231462
select.join(", "),
14241463
quote_identifier(dataset),
14251464
where_clause,
14261465
groups,
1466+
MAX_TOOL_ROWS + 1,
14271467
)
14281468
}
14291469

1470+
/// Caps trace activity rows at the tool limit and reports whether any were dropped.
1471+
fn truncate_trace_activity_rows(rows: &mut Value) -> bool {
1472+
let rows = match rows {
1473+
Value::Array(rows) => rows,
1474+
Value::Object(object) => match object.get_mut("records").and_then(Value::as_array_mut) {
1475+
Some(rows) => rows,
1476+
None => return false,
1477+
},
1478+
_ => return false,
1479+
};
1480+
let truncated = rows.len() > MAX_TOOL_ROWS as usize;
1481+
rows.truncate(MAX_TOOL_ROWS as usize);
1482+
truncated
1483+
}
1484+
14301485
async fn trace_activity_fields(
14311486
dataset: &str,
14321487
context: OssToolContext<'_>,
@@ -1550,9 +1605,12 @@ async fn execute_trace_activity_tool(
15501605
let bucket_count = bounded_integer(arguments, "bucketCount", 60, 1, 100);
15511606
let interval = time_bucket_interval(start_time, end_time, bucket_count);
15521607
let query = trace_activity_query(dataset, &time_field, &interval, &group_by, &clauses);
1608+
// The generated query carries its own LIMIT; wrapping it in the generic
1609+
// 500-row bound would hide the extra row used to detect truncation.
1610+
validate_tool_sql(&query)?;
15531611
let response = query::query_internal(
15541612
query::Query {
1555-
query: bounded_select(&query)?,
1613+
query: query.clone(),
15561614
start_time: start_time.to_owned(),
15571615
end_time: end_time.to_owned(),
15581616
send_null: true,
@@ -1565,7 +1623,8 @@ async fn execute_trace_activity_tool(
15651623
)
15661624
.await
15671625
.map_err(|error| error.to_string())?;
1568-
let rows = response_value(response).await?;
1626+
let mut rows = response_value(response).await?;
1627+
let truncated = truncate_trace_activity_rows(&mut rows);
15691628
Ok(json!({
15701629
"dataset": dataset,
15711630
"queryType": "sql",
@@ -1576,6 +1635,7 @@ async fn execute_trace_activity_tool(
15761635
"trace_count": "COUNT(DISTINCT span_trace_id)"
15771636
},
15781637
"rows": rows,
1638+
"truncated": truncated,
15791639
}))
15801640
}
15811641

@@ -1836,6 +1896,26 @@ async fn execute_oss_tool_inner(
18361896
/// Execute a registered OSS tool in-process. Enterprise uses this only after
18371897
/// applying its own agent policy and action-confirmation checks, so catalog
18381898
/// validation and the underlying platform implementation remain canonical.
1899+
///
1900+
/// # Caller obligations
1901+
///
1902+
/// Unlike [`call_tool_registry`], this function applies only catalog argument
1903+
/// validation and each tool's own per-resource `authorize()` check. Before
1904+
/// calling it, the caller must:
1905+
///
1906+
/// - confirm the session is still valid (`Users.session_exists`);
1907+
/// - reject suspended tenants with
1908+
/// `middleware::check_suspension_for_tenant(tenant_id, Action::Query)`;
1909+
/// - authorize the global `Action::Query` gate for the session;
1910+
/// - restrict which tools may run. Lookup uses the full [`oss_tool_specs`],
1911+
/// including action tools such as `create_alert`, `enable_alert`,
1912+
/// `disable_alert`, `evaluate_alert` and `create_alert_target`, whereas the
1913+
/// HTTP route exposes only [`oss_read_tool_specs`] (plus dashboard writes
1914+
/// when enabled). Action tools need the caller's own confirmation policy.
1915+
///
1916+
/// The same obligations apply to [`execute_oss_tool_with_query_headers`].
1917+
///
1918+
/// [`oss_read_tool_specs`]: crate::tool_catalog::oss_read_tool_specs
18391919
pub async fn execute_oss_tool(
18401920
name: &str,
18411921
arguments: &Value,
@@ -1850,6 +1930,10 @@ pub async fn execute_oss_tool(
18501930
/// Execute a registered OSS tool with separate headers for internal query-node
18511931
/// requests. Prism callers use this to preserve their server-authenticated
18521932
/// routing headers without changing the external MCP execution path.
1933+
///
1934+
/// Callers must apply the session, tenant-suspension, global `Query` and
1935+
/// tool-restriction gates listed on [`execute_oss_tool`]; none are enforced
1936+
/// here.
18531937
pub async fn execute_oss_tool_with_query_headers(
18541938
name: &str,
18551939
arguments: &Value,
@@ -2166,6 +2250,56 @@ mod tests {
21662250
assert_ne!(underscored["status"], "resolved");
21672251
}
21682252

2253+
#[actix_web::test]
2254+
async fn shared_dataset_resolution_prefers_exact_case_selection() {
2255+
let datasets = json!([{ "name": "Logs" }, { "name": "logs" }, { "name": "Traces" }]);
2256+
for (requested, expected) in [("logs", "logs"), ("Logs", "Logs")] {
2257+
let resolved = resolve_dataset_candidates(
2258+
datasets.clone(),
2259+
&json!({ "selectedDataset": requested }),
2260+
);
2261+
assert_eq!(resolved["status"], "resolved");
2262+
assert_eq!(resolved["selectedDatasets"], json!([expected]));
2263+
assert_eq!(resolved["datasets"][0]["name"], expected);
2264+
}
2265+
2266+
let unique_fallback =
2267+
resolve_dataset_candidates(datasets.clone(), &json!({ "contextDataset": "traces" }));
2268+
assert_eq!(unique_fallback["status"], "resolved");
2269+
assert_eq!(unique_fallback["selectedDatasets"], json!(["Traces"]));
2270+
2271+
let ambiguous_case =
2272+
resolve_dataset_candidates(datasets, &json!({ "selectedDataset": "LOGS" }));
2273+
assert_eq!(ambiguous_case["status"], "ambiguous");
2274+
assert_eq!(ambiguous_case["datasets"], json!([]));
2275+
let names = ambiguous_case["candidates"]
2276+
.as_array()
2277+
.unwrap()
2278+
.iter()
2279+
.map(|candidate| candidate["name"].clone())
2280+
.collect::<Vec<_>>();
2281+
assert_eq!(names, vec![json!("Logs"), json!("logs")]);
2282+
}
2283+
2284+
#[actix_web::test]
2285+
async fn shared_dataset_resolution_reports_no_matching_candidates() {
2286+
let none_match = resolve_dataset_candidates(
2287+
json!([
2288+
{ "name": "app-logs", "telemetryType": "logs" },
2289+
{ "name": "otel-metrics", "telemetryType": "metrics" }
2290+
]),
2291+
&json!({ "query": "checkout", "telemetryType": "traces" }),
2292+
);
2293+
assert_eq!(none_match["status"], "not_found");
2294+
assert_eq!(none_match["candidates"], json!([]));
2295+
let message = none_match["message"].as_str().unwrap();
2296+
assert!(!message.contains("ambiguous"), "{message}");
2297+
assert_eq!(
2298+
message,
2299+
"No caller-visible datasets match requested telemetry type `traces`."
2300+
);
2301+
}
2302+
21692303
#[actix_web::test]
21702304
async fn shared_time_resolution_honors_timezone_and_reference() {
21712305
let reference = DateTime::parse_from_rfc3339("2026-10-01T12:00:00Z")
@@ -2352,21 +2486,8 @@ mod tests {
23522486
let interval =
23532487
super::time_bucket_interval("2026-10-22T00:00:00Z", "2026-10-22T00:02:00Z", 2);
23542488
assert_eq!(interval, "60 seconds");
2355-
let query = bounded_tool_sql(&trace_activity_query(
2356-
"traces",
2357-
"p_timestamp",
2358-
&interval,
2359-
group_by,
2360-
clauses,
2361-
))
2362-
.unwrap();
2363-
let batches = context.sql(&query).await.unwrap().collect().await.unwrap();
2364-
let mut writer = arrow_json::ArrayWriter::new(Vec::new());
2365-
writer
2366-
.write_batches(&batches.iter().collect::<Vec<_>>())
2367-
.unwrap();
2368-
writer.finish().unwrap();
2369-
let rows: Vec<Value> = serde_json::from_slice(&writer.into_inner()).unwrap();
2489+
let query = trace_activity_query("traces", "p_timestamp", &interval, group_by, clauses);
2490+
let rows = execute_trace_activity_sql(&context, &query).await;
23702491
let times = rows
23712492
.iter()
23722493
.map(|row| row["time"].as_str().unwrap())
@@ -2378,6 +2499,69 @@ mod tests {
23782499
rows
23792500
}
23802501

2502+
/// Runs trace activity SQL the way the tool does: validated, without the
2503+
/// generic 500-row wrapper.
2504+
async fn execute_trace_activity_sql(
2505+
context: &datafusion::prelude::SessionContext,
2506+
query: &str,
2507+
) -> Vec<Value> {
2508+
super::validate_tool_sql(query).unwrap();
2509+
let batches = context.sql(query).await.unwrap().collect().await.unwrap();
2510+
let mut writer = arrow_json::ArrayWriter::new(Vec::new());
2511+
writer
2512+
.write_batches(&batches.iter().collect::<Vec<_>>())
2513+
.unwrap();
2514+
writer.finish().unwrap();
2515+
serde_json::from_slice(&writer.into_inner()).unwrap()
2516+
}
2517+
2518+
#[actix_web::test]
2519+
async fn trace_activity_signals_truncation_past_the_row_limit() {
2520+
// 20 services x 60 one-minute buckets = 1200 groups x buckets.
2521+
let context = datafusion::prelude::SessionContext::new();
2522+
let fixture = context
2523+
.sql(
2524+
r#"SELECT CAST(TO_TIMESTAMP_SECONDS(1792627200 + (value / 20) * 60) AS TIMESTAMP)
2525+
AS p_timestamp,
2526+
CONCAT('t', value) AS span_trace_id, CONCAT('s', value) AS span_span_id,
2527+
CONCAT('svc-', value % 20) AS "service.name"
2528+
FROM range(1200)"#,
2529+
)
2530+
.await
2531+
.unwrap();
2532+
context
2533+
.register_table("traces", fixture.into_view())
2534+
.unwrap();
2535+
let interval =
2536+
super::time_bucket_interval("2026-10-22T00:00:00Z", "2026-10-22T01:00:00Z", 60);
2537+
assert_eq!(interval, "60 seconds");
2538+
let query = trace_activity_query(
2539+
"traces",
2540+
"p_timestamp",
2541+
&interval,
2542+
&["service.name".to_owned()],
2543+
&[],
2544+
);
2545+
let mut rows = Value::Array(execute_trace_activity_sql(&context, &query).await);
2546+
assert!(super::truncate_trace_activity_rows(&mut rows));
2547+
let rows = rows.as_array().unwrap();
2548+
assert_eq!(rows.len(), 500);
2549+
// Ascending order keeps the oldest 25 buckets; the newest are the ones dropped.
2550+
assert_eq!(rows[499]["time"], "2026-10-22T00:24:00");
2551+
2552+
// A result that fits is returned whole and not flagged.
2553+
let query = trace_activity_query("traces", "p_timestamp", &interval, &[], &[]);
2554+
let mut rows = Value::Array(execute_trace_activity_sql(&context, &query).await);
2555+
assert!(!super::truncate_trace_activity_rows(&mut rows));
2556+
assert_eq!(rows.as_array().unwrap().len(), 60);
2557+
2558+
let mut exact = Value::Array(vec![json!({}); 500]);
2559+
assert!(!super::truncate_trace_activity_rows(&mut exact));
2560+
let mut records = json!({ "records": vec![json!({}); 501] });
2561+
assert!(super::truncate_trace_activity_rows(&mut records));
2562+
assert_eq!(records["records"].as_array().unwrap().len(), 500);
2563+
}
2564+
23812565
#[actix_web::test]
23822566
async fn trace_activity_query_executes_with_distinct_bucketed_counts() {
23832567
let rows = run_trace_activity_fixture(&["service.name".to_owned()], &[], "BIGINT").await;

0 commit comments

Comments
 (0)