You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
feat(sources): Open Connector UDTFs, security policy, and observability (#165)
* feat(sources): Open Connector UDTFs, security policy, and observability
Milestone 4 of the Open Connector integration: the interactive SQL
surface, per docs/superpowers/specs/2026-07-11-open-connector-integration-tasks.md.
- open_connector_query(gateway, 'pack.table', resource_json[, alias]):
runs a built-in source-pack table without a persistent YAML binding,
compiling into the exact scan a bound table uses — same stable schema,
filter allowlist, fingerprint gate, safety bounds, and the same
per-gateway scan cache. Plans against registration-time discovery, so
an undiscovered action is a targeted planning error (planning never
performs network I/O).
- open_connector_scan(gateway, action_id, input_json, row_path[, alias]):
executes an explicitly allowlisted raw read action once (new
PaginationStrategy::SinglePage; always live, no filter pushdown), with
a deterministic row type derived at planning time from the discovered
output schema (raw_schema.rs) — primitives typed, ["T","null"] unions
nullable, everything else opaque JSON; indeterminate shapes fail with
an error recommending a source pack.
- Security, default-deny and pre-HTTP: discovery gains a read_only flag;
raw actions require allowlist membership AND an explicit read-only
classification (mutating vs unclassified rejected with distinct
errors before any request); tests pin that YAML bindings cannot
override pack action/row_path/pagination/columns.
- Observability: scan completion/failure tracing events with gateway,
binding, table, action, cache hit, pages, rows, and duration — never
tokens, inputs, or bodies.
- Engine refactor to enable dynamic schemas: OpenConnectorExec takes an
owned ScanTarget instead of &'static SourcePackTable; RowConverter
accepts owned ColumnSpecs.
- Wiring: register_open_connector_tables publishes a GatewayHandle into
a shared OpenConnectorGateways map (server OptimizerRegistry / CLI),
mirroring the DatasetRegistry pattern; UDTFs registered on both
front-ends.
- Docs: docs/open-connector.md guide, README supported-sources entry,
milestone checklist updated.
Verification: 147 open_connector tests (UDTF/YAML parity, shared-cache
replay with zero new requests, single-POST raw scan, pre-HTTP security
rejections asserted via recorded gateway traffic, federated CSV join);
full skardi/server/CLI suites green.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(sources): emit Open Connector scan-completion event with the final batch
A satisfied downstream LIMIT drops the scan stream without another poll
(DataFusion's LimitStream clears its input on the batch that fills the
fetch), so the completion event — previously emitted only from the
early-return branch of the NEXT next_page call — never fired for
LIMIT-satisfied scans. The docs' every-scan / exactly-once claims did
not hold for the most common query shape.
Log eagerly instead, with the final batch, wherever the scan is known
complete (guarded by completion_logged):
- live path: after the row counter includes the final batch, whenever
`done` is set (covers LIMIT-satisfied AND short-final-page exhaustion,
which had the same latent dependence on one more poll);
- cache-replay path: when the replay queue drains (cached LIMIT queries
replay under the same never-polled-again consumer).
Logging after the row-count update keeps the event's `rows` field
accurate — logging inside the LIMIT branch itself would under-count by
the final batch.
Regression tests drive ScanState directly against the mock gateway and
assert completion_logged flips exactly on the final batch for all three
shapes (LIMIT-satisfied, non-empty terminal page, cache replay), plus
accurate row totals.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* docs(sources): state the Open Connector failure-event contents accurately
The scan-failure event's comment, docs/open-connector.md, and the
milestone checklist claimed errors "carry JSON kinds only" / "response
bodies are never logged". That is true for conversion and row-path
failures, but ActionExecutionFailed deliberately quotes a bounded
(<=512-char) snippet of the gateway's *error* response (the milestone-2
diagnostic for terminal provider failures, which can echo request
identifiers such as owner/repo), and PaginationLoop carries the
offending cursor.
Align the three claims with reality instead of filtering the event: the
same error display also reaches the SQL client and the front-ends'
registration logs, so redacting one log site would not make the promise
true — it would only strip the diagnostic where operators look first.
The design spec's actual commitment (no tokens, credentials,
authorization headers, or full sensitive inputs) was never violated and
is now what the comment, guide, and checklist say.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* test(sources): pin Open Connector scan events with a tracing capture
The completion/failure-event invariants were asserted only through
ScanState's completion_logged flag — nothing verified that the events
are actually emitted, exactly once, with the documented fields.
Add a test-only tracing capture to testutil (a minimal Subscriber
recording level/message/fields into a shared vec, installed as a
thread-local default — #[tokio::test] bodies run single-threaded, so
parallel tests stay isolated) and four event-level tests that consume
the real execute() stream:
- LIMIT-satisfied scan: one batch polled, stream dropped without a
further poll (the LimitStream shape) → exactly one INFO completion
event with gateway/binding/table/action, cache_hit=false, pages=1,
rows=1, duration_ms;
- empty scan: polled to None (and once past it) → exactly one
completion with rows=0;
- cache replay: two full scans → exactly two completions, the second
cache_hit=true / pages=0 / rows=3 (the done-branch re-log is
guarded, so draining the queue and polling to None stays at one);
- terminal 5xx on execute → exactly one WARN failure event with the
scan identity, pages_fetched=0, and the HTTP status in the error —
and no completion event.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(sources): convert JSON null to SQL NULL in opaque Json columns
The FieldType::Json arm stringified cells directly, so a present JSON
null (Some(&Value::Null) — RowPath::extract returns the value when the
key exists) became the 4-char string "null" instead of Arrow null,
ignoring the column's nullability. Every other arm already routes
present nulls through collect_cells (nullable -> Arrow null,
non-nullable -> targeted per-column failure).
This got hot with open_connector_scan: raw schemas type every
object/array/wide-union field as nullable Json, and provider nulls
(assignee: null, user: null) are ubiquitous — WHERE x IS NULL matched
nothing while x = 'null' matched.
Route the Json arm through collect_cells like the rest: JSON null is
SQL NULL for nullable columns and a targeted ConversionFailed
(found: "null") for non-nullable ones, instead of surfacing later as a
batch-level nullability error.
Tests: unit coverage for present-null vs absent-key (both Arrow null)
and the non-nullable failure; an end-to-end raw-scan test pinning the
SQL semantics (IS NULL matches the provider null, = 'null' does not).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* docs(sources): pin the fetch-time semantics of pages_fetched
A review suggested moving the increment after the post-fetch deadline
check so a page that lands right at the deadline is not counted in the
failure event. Declined: pages_fetched measures gateway traffic
(requests actually made, rate-limit budget actually spent), not pages
emitted — a failed scan emits nothing at all, so an emission reading of
the field is meaningless there, and moving the increment would
under-report real gateway load in timeout diagnostics while staying
inconsistent with the extraction/conversion failure paths right below.
Document the intent at the increment so the placement reads as a
decision, not an accident.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* docs(sources): document the registration-snapshot staleness window
The raw-scan security gates (allowlist + read-only classification) read
metadata discovered at registration; planning never re-contacts the
gateway by design. An upstream action that turns mutating after
registration therefore keeps passing the Skardi-side gate until the
next restart or configuration reload. Spell that window out in the
UDTF module docs and the security-model section of the guide — the same
snapshot covers executability and contract fingerprints — and name
Open Connector's own action policies as the live, independent boundary
during it.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(cli): thread UDTF gateways through the ClickHouse test call sites
Merging main brought in the ClickHouse provider (#157), whose two
register_source tests still destructured new_session_context() as a
2-tuple; this branch extended it to also return the Open Connector
gateway map. Update the new call sites to the 3-tuple and pass the map.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
0 commit comments