Repository navigation
Port upstream tool catalog update (#1831): get_trace_activity, resolve_dataset/log context schema changes - #62
Merged
Merged
Conversation
Port of upstream parseablehq/parseable 32979cb. add get_trace_activity to tool catalog update executor to have tool implementations and reuse in enterprise Fork conflict resolutions: - tool_catalog: use upstream's validate_one_of helper (same exactly-one semantics and message as the fork's inline check) and keep the fork's `const` validation; tool counts become 54 total / 41 read. - tool_executor: adopt upstream's parser-based validate_tool_sql and bounded_tool_sql; keep fork time-expression imports, the Result handling for list_dashboards_internal, and the fork's call_tool_registry (dashboard tool routing, suspension and Query gates); keep calling execute_oss_tool_inner there because the spec is already validated. - tests: merge both suites; update the fork bounded-SQL test to avoid a wildcard CTE that upstream's validator now rejects. Also add tests for get_trace_activity (executed against a DataFusion fixture), its argument validation, resolve_dataset/get_log_context schema changes, timezone day boundaries and sample_events range pairing, and document the new and changed tools in docs/tool-api.md. (cherry picked from commit 32979cb) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`validate_type` ignored array-valued `type` (for example `["string", "null"]`), so the dashboard operation schemas' nullable and union fields were never type-checked. Accept a value when it matches any listed type, and name every allowed type in the error. Verified the dashboard agent E2E script's create, update, rename, placement, invalid-layout and protected-field payloads still pass schema validation, so service-side 422/400 responses are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 24, 2026
…flow, tests - list_tool_registry omits root-level oneOf/anyOf/allOf from each listed inputSchema, since LLM tool APIs reject them; the catalog schema keeps them and call_tool_registry still validates against it. - log_context_request requires exactly one of log, body or message. - resolve_time_range and the log-context window use checked chrono arithmetic, so huge durations return errors instead of panicking. - Extract trace_activity_time_field and summarize_dataset_activity so the trace preconditions and discover_datasets ranking can be unit tested. - Tests: no listed root combinators; exactly-one anchor; oneOf error text; log-context projection with more than 10 fields; log_context_request defaults; sample_events 10-column cap; trace activity ordering with millisecond timestamps and Float64 status; SQL validator query forms; resolve_dataset ties, case-insensitive and invisible selections; duration overflow. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Listed inputSchemas omit root combinators; the server still enforces them. - SQL limits apply to the outermost projection and top-level set-operation branches, not CTEs or subqueries; a semicolon followed by a comment fails. - resolve_dataset: case-insensitive names, the `error` status, and scoring. - get_log_context: bound pairing and exclusivity, minute rounding of windows, and the anchor range check. - get_trace_activity: `query` is the inner SQL without a time predicate; rerun it through the query API with the same startTime and endTime. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Author
|
Correction to the port commit message (2f8718d). It says the fork's bounded-SQL test was changed "to avoid a wildcard CTE that upstream's validator now rejects". That's inaccurate. The original input was We're not rewriting history to fix the message, since that would need a force-push. This comment records the correction instead, and the PR description has been updated to match. |
This was referenced Oct 24, 2026
…-1831 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 24, 2026
VibeSWE
added a commit
that referenced
this pull request
Oct 24, 2026
…ity-truncation PR #62 follow-up 14: signal get_trace_activity truncation at 500 rows
VibeSWE
added a commit
that referenced
this pull request
Oct 24, 2026
…-tool-gates PR #62 follow-up 20: document caller gates for execute_oss_tool helpers
VibeSWE
added a commit
that referenced
this pull request
Oct 24, 2026
…aset-exact-case PR #62 follow-up 17: resolve_dataset prefers exact-case selection; correct not_found message
VibeSWE
added a commit
that referenced
this pull request
Oct 24, 2026
…ity-exact-range PR #62 follow-up 15: apply exact time bounds in get_trace_activity
VibeSWE
added a commit
that referenced
this pull request
Oct 24, 2026
…aset-token-match PR #62 follow-up 16: match resolve_dataset names on token boundaries
VibeSWE
added a commit
that referenced
this pull request
Oct 24, 2026
…by-collisions PR #62 follow-up 21: reject repeated and output-colliding trace groupBy fields
VibeSWE
added a commit
that referenced
this pull request
Oct 25, 2026
…-projection PR #62 follow-up 19: run tool SQL ending in '; -- comment'
VibeSWE
added a commit
that referenced
this pull request
Oct 25, 2026
…e-range-parsing PR #62 follow-up 18: fix resolve_time_range parsing gaps
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Upstream origin
Ports parseablehq/parseable@32979cb4, "update: tool catalog (parseablehq#1831)", via
git cherry-pick -x. Upstream authorship is kept.Upstream changes:
get_trace_activitytool. It returns a bucketed (DATE_BIN) series ofCOUNT(DISTINCT span_span_id)/COUNT(DISTINCT span_trace_id), with optionalgroupBy(max 5) and scalarfilters. It also returns the exact SQL it ran.resolve_dataset: newtelemetryType/limitarguments, plus scored candidate ranking. It never falls back to an arbitrary dataset.get_log_context: the catalog schema now requires exactly one oflog/body/messagevia a rootoneOf. Clients never see thatoneOf: the fork strips root combinators from listed schemas (see "Review fixes"). The tool now delegates to the same code asPOST /api/v1/query/context, through the newquery_context_for_authorized_datasetinquery_context.rs.sample_events: new pairedstartTime/endTimearguments.oneOf, andanyOfno longer short-circuits sibling keywords. Forkmainalready had both, so this isn't a behaviour change here.execute_oss_toolandexecute_oss_tool_with_query_headers, for Enterprise. It also gains parser-based SQL validation, IANA-timezoneresolve_time_range, activity-countdiscover_datasets, and exclusion of internal datasets.Conflict resolutions
src/tool_catalog.rsoneOf: the fork already had an inline check, and it is semantically identical to upstream's: exactly one variant must match, with the same error message. I used upstream'svalidate_one_ofhelper to reduce future upstream conflicts. The fork'sconstcheck is kept immediately after it.get_trace_activity.formatandschema_combinators_*tests and upstream'sresolve_datasetandget_log_contextassertions.src/tool_executor.rsvalidate_tool_sql/bounded_tool_sql, replacing the fork's first-word check. It covers the fork's goals (multiline queries, trailing comments). The fork test's inputWITH x AS (SELECT 1) SELECT * FROM xis rejected because of its outerSELECT *(a wildcard inside a CTE would be accepted), so I changed it to select an explicit column. The port commit message wrongly calls this "a wildcard CTE"; see the correction comment on this PR.parse_start_time_expression, etc., used bylist_log_errors) and addedoss_tool_specs.list_dashboardsin the executor: took upstream's title/tagqueryfilter and kept the fork'sResulthandling oflist_dashboards_internal.call_tool_registry: kept the fork's version as-is. Dashboard tools route todashboard_tools::execute, and the tenant-suspension andQuerygates still apply to everything else. It still callsexecute_oss_tool_inner, not upstream'sexecute_oss_tool. The spec is already validated againstexternal_tool_specs, soexecute_oss_toolwould validate the arguments a second time, andoss_tool_specs()allocates and clones the catalog on every call. (The dashboard operation schemas themselves are cached in aLazyLock.)query_context.rs,Cargo.toml,Cargo.lockapplied cleanly. The fork had no divergence inquery_context.rs.Branch contents
2f8718da0097de1eci: migrate all workflows off Blacksmith. Not part of the port: it is identical tomain's #66 (de6c4a97), andgit diff origin/main origin/upstream/tool-catalog-1831 -- .github README.mdis empty. It was added so CI could run on this branch. It's left in place deliberately (rewriting would need a force-push) and becomes a no-op when merged.cc46ee5ftypeunions in argument validation (review nit 24).validate_typeaccepts atypearray, such as["string","null"], when any listed type matches; previously an array-valuedtypewas ignored.docs/tool-api.mddocuments this, andschema_type_unions_accept_any_listed_typetests it.6218538277723f97docs/tool-api.mdclaims match the code.83a43099origin/main(brings in #59 ingestion throughput and #60 API-key privilege filter). No conflicts;maindid not touch the tool files.Review fixes
oneOf/anyOf/allOf. Upstream'sget_log_contextoneOfwould have broken every tool for those clients, not just this one.list_tool_registrynow emitslisted_input_schema(...), which removes those root keys. The catalog keeps theoneOf,call_tool_registrystill validates against it (400, "arguments must match exactly one allowed schema"), and thelog/body/messagedescriptions state the rule. A test checks every listed tool. The catalog itself still matches upstream.log_context_requestrequires exactly one anchor. It used to accept two or more and say "at least one".resolve_time_range(last 1000000 years) and the log-contextcontextWindow(300000y) overflowed chrono and panicked. Both now usechecked_sub_signed/checked_add_signedand return errors.trace_activity_time_fieldandsummarize_dataset_activity. Both are moved verbatim from the async functions, which now call them.Dependency:
chrono-tz = "0.10"resolve_time_range(today/yesterdaylocal-day boundaries, DST-aware). Before this, the tool rejected any timezone other than UTC.chrono-tz0.10.4 was already in the build througharrow-arrayanddatafusion-functions.Cargo.lockonly gains a direct-edge line, so there are no new crates and no extra compile cost. The license is MIT OR Apache-2.0, whichabout.tomlallows.Overlap with pending tool-catalog work
The uncommitted
src/tool_catalog.rs,src/tool_executor.rsanddocs/tool-api.mdchanges in thedeploy-maincheckout (run_dashboard_queries,aftercursors,oneOf/const/maxLength) are byte-identical to the currentorigin/mainversions. They already landed via 54e8de3 / #51; that checkout sits on an oldermain(9747e2b). So this PR is resolved directly against that work:oneOfsemantics are unchanged, andconst/maxLengthare preserved.run_dashboard_queries,aftercursors and the dashboard tool routing are untouched.schema_combinators_enforce_branches_and_sibling_constraintstest still passes.Once that checkout moves to current
main, its diff should be empty. The changes indocs/tool-api.mdfrom this PR are appended sections only.Ramifications found
get_trace_activityfits this fork's trace storage.span_span_id/span_trace_idare the fork's flattened OTel trace columns (src/handlers/http/traces.rs). The tool requires traces telemetry and both columns, and uses the dataset's time partition, orp_timestampif none is set. A new test runs the generated SQL, wrapped bybounded_tool_sql, against a DataFusion fixture. This confirms thatDATE_BINwith a string interval, positionalGROUP BYand distinct counts work, including a duplicate span row.query_sql/explain_querynow rejectSELECT *and projections wider than 10 columns. The fork'slist_log_errorsSQL (5 columns) still validates.dashboard_runtimehas its ownbounded_sqland is unaffected.get_log_contextnow requires exactly one anchor value, and its response is now thequery/contextshape:records,anchorIndex,scope,queries, etc.get_log_contextbounds now followquery/context. You give eithercontextWindowor bothcontextStartTimeandcontextEndTime. A single explicit bound is now an error; the old tool filled in the other side with ±5 minutes. Combining a window with an explicit bound is also an error. The default iscontextWindow: "5 minutes"on each side of the anchor, with both ends rounded down to the minute. Explicit bounds are no longer rounded to the minute, and the anchor must fall inside them.resolve_time_rangenow parses IANA zone names case-sensitively, so lowercase"utc"is rejected. The fork used to accept any casing ofUTC, and only UTC.inputSchemanever carries root-leveloneOf/anyOf/allOf(see "Review fixes").discover_datasetsresults are now{name, eventsInRange}, plusunavailableDatasets, in place of{name, info}anderrors.list_datasets/resolve_dataset/discover_datasetshide internal datasets.scripts/test-dashboard-agent.py, uses only dashboard tools and is unaffected. The frontend only mentions the endpoints in a comment, and there are no tool snapshots.docs/tool-api.mdnow documents the SQL limits, dataset and time resolution, log context,get_trace_activityand the validator keywords.execute_oss_tooldispatches only to the OSS executor. Dashboard-service tools other thanlist_dashboards/get_dashboard/list_dashboard_tags, such asrun_dashboard_queries, return "no OSS internal executor" there.list_dashboardsreturns a bare array there, while the external path returns{dashboards, nextCursor}. That divergence already existed in the fork; I left it as-is.Tests
New tests:
get_trace_activity:groupBy/filtersfield validation and identifier/literal quoting.groupBymax 5,bucketCount1–100, required fields, no extra properties.readOnlyHint.resolve_dataset:telemetryTypeenum andlimitbounds.get_log_context: each anchor alone is accepted; all three together are rejected.62185382):oneOf.oneOferror text for zero and two matches.log_context_request: exactly one anchor; the defaultcontextWindow; explicit bounds suppress it;pageSizeclamped to 500; the output deserializes asLogContextRequest.{records}shapes. An identity projection now fails.sample_events10-column cap.COUNT(*),UNIONwith*, parenthesized*,INT64/UINT64/FLOAT64casts,VALUES,EXPLAIN, multiple statements.resolve_dataset: ties, case-insensitiveselectedDataset, invisibleselectedDataset.discover_datasetssummary: ordering and tie-break,truncated,status: "error", and the 20-itemunavailableDatasetscap.resolve_time_rangeandcontextWindow.resolve_time_range: localtoday/yesterdayboundaries inAsia/Kolkata, and rejection of an unknown zone.sample_events:startTime/endTimemust be paired.Results:
cargo fmt --check: clean.cargo clippy --all-targets: exit 0. Its warnings are pre-existing and none are in files this PR touches.cargo clippy -- -D warnings: clean.cargo test(at77723f97): 870 lib tests passed and 2 were ignored; all integration suites passed; 0 failures. The earlier "860" figure counted only the tests that passed at the port commit, out of 862.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need.