Skip to content

server: add saved-query CRUD at /queries#96

Closed
joanreyero wants to merge 2 commits into
ModernRelay:mainfrom
joanreyero:feat/saved-queries-api
Closed

server: add saved-query CRUD at /queries#96
joanreyero wants to merge 2 commits into
ModernRelay:mainfrom
joanreyero:feat/saved-queries-api

Conversation

@joanreyero

@joanreyero joanreyero commented May 15, 2026

Copy link
Copy Markdown

Summary

  • Adds GET /queries, GET/PUT/DELETE /queries/{name} so clients can persist named .gq queries by name instead of passing the full source on every call. One JSON file per saved query under <root>/queries/<name>.json via the existing StorageAdapter — same surface schema source already uses, no Lance/manifest involvement.
  • Server parses the source at save time via omnigraph_compiler::query::parser::parse_query and persists the declared param signature alongside it, so MCP / UI clients can render typed inputs without re-parsing.
  • 1:1 invariant enforced: source must declare exactly one query <name>(...) block whose name matches the URL key. Names validated ^[a-z][a-z0-9_]{0,63}$; a small reserved list blocks shadowing built-in MCP tool names (read, change, …).
  • New Cedar actions query_read / query_write mirror the read / schema_apply wiring.

Motivating use case

The Omnigraph TypeScript MCP server currently exposes only generic read / change tools, so every saved query an agent or human authors lives in client memory. With this CRUD an MCP can list saved queries at startup and register one typed MCP tool per saved query (q_<name> prefix), making them discoverable in the agent's tool catalogue. The companion TS-side change lives in ModernRelay/omnigraph-ts (linked below).

Persisted shape

{
  "format_version": 1,
  "name": "find_person",
  "description": "by name",
  "source": "query find_person($name: String) { ... }",
  "params": [{ "name": "name", "type_name": "String", "nullable": false }],
  "updated_at_us": "1747315200000000"
}

Invariants checklist

  • Substrate respect: text files via StorageAdapter::write_text, no Lance datasets, no manifest entries, no new transaction manager.
  • Manifest atomicity: untouched — saved queries are out-of-band metadata, never published as part of a graph commit.
  • Snapshot semantics: untouched — saved queries don't participate in query reads.
  • Auth/policy boundary: new HTTP routes call authorize_request(...) with new QueryRead / QueryWrite PolicyAction variants; engine API is transport-agnostic.
  • Loud failures: bad name → BadRequest; missing → NotFound; mismatched source name → BadRequest with the specific reason; multi-query source → BadRequest; idempotent DELETE returns {deleted: false} rather than silent success.
  • Tests at the right boundary: 6 unit tests in crates/omnigraph/src/db/saved_queries.rs cover storage layer; 3 HTTP integration tests in crates/omnigraph-server/tests/server.rs cover the route layer.

Out of scope (deliberate)

  • Per-branch saved queries — global only, matching schema scope.
  • Read vs change distinction in metadata — all saved queries are read-class for v1. A kind: "change" flag can come later if needed.
  • Schema-coupled validation — source only needs to parse at save time. Schema-level typecheck happens at read time as today. Avoids the "schema_apply invalidates saved queries" tar pit.

Test plan

  • cargo test -p omnigraph-engine --lib saved_queries — 6 unit tests pass.
  • cargo test -p omnigraph-server — 47 server + 60 openapi tests pass, including 3 new HTTP integration tests.
  • openapi.json regenerated via OMNIGRAPH_UPDATE_OPENAPI=1 cargo test -p omnigraph-server --test openapi openapi_spec_is_up_to_date; path/security whitelist tests updated.
  • Local end-to-end smoke test: built server, ran against a temp repo, exercised all four endpoints + the validation negative paths via curl, then drove it from the companion TS MCP and confirmed q_<name> tools appear and dispatch correctly.

🤖 Generated with Claude Code


Open in Devin Review

Persist named .gq queries by name so clients (MCP, future UIs) can list
and invoke them as first-class endpoints rather than treating every
query as ad-hoc source. Storage follows the schema-source pattern — one
JSON file per saved query under `<root>/queries/<name>.json` via
`StorageAdapter::write_text`, with no Lance/manifest involvement.

The source must declare exactly one `query <name>(...)` block whose name
matches the URL key; the server parses it at save time via
`omnigraph_compiler::query::parser::parse_query` and persists the
extracted parameter signature alongside, so callers can build typed
inputs without re-parsing. Name validation is `^[a-z][a-z0-9_]{0,63}$`
with a small reserved list to prevent shadowing built-in MCP tool names.

Endpoints (all behind bearer auth + Cedar `query_read` / `query_write`):

  GET    /queries           list all
  GET    /queries/{name}    get one (404 if absent)
  PUT    /queries/{name}    insert-or-overwrite
  DELETE /queries/{name}    idempotent delete

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Signed-off-by: Joan Reyero <joan@reyero.io>
cubic-dev-ai[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

Review feedback from ModernRelay#96:

- list()/get() now check format_version, validate the parsed name, and
  verify it matches the filename it came from. list() skips invalid
  records with a tracing::warn so one tampered file doesn't poison the
  whole listing; get() rejects loudly since the caller already knows
  which name they wanted. Covers the case where an operator hand-edits
  the on-disk JSON (rename, format-version bump, etc.). 2 new unit
  tests pin both paths.
- docs/user/server.md: four /queries rows added to the endpoint
  inventory; new "Saved queries" subsection covers the persistence
  model and naming rules; admission-control paragraph updated to be
  explicit that /queries writes are intentionally not gated.
- docs/user/policy.md: query_read / query_write added to the actions
  list with a note that they are not branch-scoped.
- openapi.rs: EXPECTED_SCHEMAS whitelist updated with the five new
  component schemas.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Signed-off-by: Joan Reyero <joan@reyero.io>
@joanreyero joanreyero requested a review from ragnorc as a code owner May 15, 2026 10:03
@aaltshuler

Copy link
Copy Markdown
Collaborator

Thanks @joanreyero — this is clean, well-tested work: good error taxonomy, tests at the right boundaries, OpenAPI regenerated, and the control/data separation is exemplary (pure out-of-band metadata, no Lance/manifest entanglement, snapshot semantics untouched). I'm closing it for a sequencing/architecture reason, not a quality one — it surfaces a bigger question we need to settle before committing to a write surface for this class of resource.

The big question we need to address first: control plane + state

Saved queries are a control-plane resource — the same class as schema, policies, and (soon) UI. We're actively designing how the cluster's control plane and its state model should work: a declarative cluster spec, plan/apply/reconcile, and applied-state-as-projection (not the running system as truth). This PR is the first concrete CRUD for a control-plane resource, so whatever model it establishes sets precedent for all of them. We should decide the control-plane/state model first, then implement the resources to fit it — not the reverse.

The conflict: imperative vs declarative

This PR implements the resource imperativelyPUT/DELETE mutate live server state directly, making the running system the source of truth. The direction we're moving (the cluster-config spec work + RFC-002 §8 "capability-as-code") is declarative — the resource is declared as code in version control, the spec is the source of truth, and apply/an agent reconciles the running system to it. These models conflict directly on ownership of <root>/queries/: once a declarative apply also manages that directory, an imperative PUT and a declarative apply will fight over the same files — exactly the drift the declarative model exists to eliminate. And every client that learns to PUT /queries (e.g. the TS MCP) becomes a dependency on the imperative surface we'd then have to subordinate.

Concretely it's the "how does Bob know what Sarah did?" test: if one operator PUTs a query, another has no single spec, no git diff, and no attributed change record to learn from. The declarative model answers that by construction; the imperative one re-creates the fragmentation.

This isn't a rejection of the feature

It's almost certainly coming back. The storage shape here — one JSON file per query, parsed param signature persisted alongside source — is a fine substrate that a declarative reconciler can target. The likely end state: this CRUD becomes the low-level mechanism apply drives, not a parallel write path operators use directly. The path forward:

  1. Settle the control-plane + state model (declarative cluster spec, ownership of resource directories, plan/apply).
  2. Reframe this CRUD as the mechanism behind apply, not a standalone authoring surface.
  3. Rebase onto /graphs/{id}/… multi-graph routing ((feat): multi-graph server mode #119 already obsoleted the flat /queries route on main).

TL;DR — closing to avoid minting an imperative write surface for a control-plane resource ahead of the control-plane/state design. Let's settle that first; this returns as the mechanism behind it.

@aaltshuler aaltshuler closed this Jun 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants