Skip to content

feat: add user-scoped SQL query tools to MCP - #436

Open
alosec wants to merge 15 commits into
mainfrom
feat/mcp-sql-query-tools
Open

alosec wants to merge 15 commits into
mainfrom
feat/mcp-sql-query-tools

Conversation

@alosec

@alosec alosec commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Add sql_query, describe_views, query_guide, and get_query_result to MCP, backed by a typed SDK assistant query manager and existing Rails endpoints.
  • Preserve delegated-agent / OAuth connected-client access, backend account/user ownership and feature checks; no API-key access widening.
  • Preserve preview/page/count/cap contracts, no automatic retries, safe actionable errors, and scoped in-flight SQL coalescing with independent result copies.
  • Add OpenAPI contracts and public MCP docs; regenerate SDK types/reference and Postman.

Rollout

Deploy and verify this MCP change before merging the companion t49 agent-removal PR https://github.com/Terminal49/t49/pull/4442. No Rails endpoint changes are required.

Review follow-up (1605731)

  • SQL discovery fails closed per user/account using the existing assistant views endpoint, without depending on the unmerged identity PR. API keys and missing account context skip the lookup. No cross-request permission cache or automatic retries; lookup timeout is five seconds. Backend execution authorization is unchanged.
  • sql_query is read-only with respect to account records; snapshots are temporary result caches. Client consent behavior remains client-specific.
  • Guide topics accept capitals, spaces, underscores and hyphens, limited to 64 characters. SDK path encoding is covered by regression tests. Alias resolution requires the updated Rails guide resolver to be deployed.
  • Connector manifests describe conditional availability rather than promising SQL access to every account.
  • To minimize duplicate local/MCP tools, merge and deploy t49 #4442 promptly after this MCP deployment is verified. Do not merge the removal first. No merge has been performed.
  • Earlier delegated-preview evidence below predates this follow-up; new capability gating is verified locally with protocol fixtures. Latest remote checks are pending.

Verification

  • Local npm run check, npm run build, and npm test pass: 441 MCP + 369 SDK + 31 CLI tests (one existing SDK skip).
  • All six baseline protocol smoke versions pass with 13 standard tools. Authorized transport tests verify the additional four SQL tools on modern and legacy protocols.
  • Existing Rails authorization, feature-gating, ownership and result request coverage: 52 examples, 0 failures.
  • Real delegated-agent preview verified: guide/schema discovery → SQL → local table display; follow-up pages the same stored result without rerunning SQL. Private evidence is on the companion t49 PR.
  • The live fixture eval still fails authentication during list_shipments discovery. No gate is bypassed; this pre-existing issue is explicitly deferred by the author. No credential workaround or CI gate bypass is included.
  • Ready for review at the author's request; not approved for merge. OAuth end-to-end login has not been retested on this feature preview.

AI review

  • Both Greptile findings addressed with regressions: generated Postman account scope and external-agent measurement/CSV guidance. Threads resolved.
  • Claude review integration fails authentication despite its green continue-on-error job; Codex connector setup is unavailable for this repository. Neither is represented as a completed review.

Related work

Ports the intent of t49 #4434 in-flight deduplication and #4374 server-failure guidance into the MCP execution path. The t49 companion retains local display error safeguards. These PRs need coordination to avoid reintroducing local wrappers.

RetriggerView in GreptileConfidence Score: 4/5

The PR appears safe to merge with non-blocking fixes to the Postman requests and agent-facing guidance.

Fix All in CodexFindings

  1. P2 Postman omits the account ▶
  2. P2 Agents miss query guidance ▶
Fix with agent prompt
### Issue 1
Terminal49-API.postman_collection.json:21186-21195
The new requests send a bearer token but omit `X-Account-ID`, which the new OpenAPI contract requires alongside it. Nothing else in the collection supplies this header. Users must repair each request before they can send the required account choice.

Add `X-Account-ID: {{accountId}}` to all six assistant requests and document the variable.

### Issue 2
packages/mcp/src/tools/sql-query.ts:129-130
The descriptions tell agents to read the guides, but omit advice to define the measure, avoid treating zero rows as proof that nothing exists, and share the CSV link for bulk data. Instead, they point toward paging or presentation tools that this MCP server does not provide. Add the missing guidance to the descriptions customers’ agents receive.

Greptile automatically discovered a related ticket stating that these instructions must be carried by the tool descriptions because external agents do not see the internal assistant instructions, which informed this comment.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

Adds four SQL tools to MCP through a new typed SDK manager.

  • MCP clients can query account data and page through saved results.

Tests were inspected, not executed. alosec explicitly identified the existing live-eval API-key authentication failure as a known, separate blocker; it is not reported here.

Diagram

sequenceDiagram
  participant Agent
  participant MCP
  participant SDK
  participant Rails
  Agent->>MCP: sql_query(sql)
  MCP->>MCP: Match credential, account, endpoint and SQL
  MCP->>SDK: Share matching in-flight query
  SDK->>Rails: POST assistant_queries with credential and account
  Rails-->>SDK: Preview and result_id, or error
  SDK-->>MCP: Return without retry
  MCP-->>Agent: Separate result copy or safe error
  Agent->>MCP: get_query_result(result_id)
  MCP->>SDK: Read requested page
  SDK->>Rails: "GET assistant_queries/{id}"
  Rails-->>Agent: Stored rows through SDK and MCP
Loading

Reviews (1) · Last reviewed commit: "test: require SQL tools in every protoco..." · Reviewed by Greptile

@mintlify

mintlify Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated
terminal49 🟢 Ready View Preview Oct 15, 2026, 7:17 PM

@vercel

vercel Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
api Ready Ready Preview Oct 15, 2026 7:18pm UTC

Request Review

@alosec

alosec commented Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

@codex review Please review the SDK/MCP SQL migration for authorization boundaries, snapshot ownership, retries, in-flight isolation, and response contracts. This remains a draft; no merge requested.

@chatgpt-codex-connector

Copy link
Copy Markdown

To use Codex here, create a Codex account and connect to github.

@alosec
alosec marked this pull request as ready for review October 15, 2026 18:09
Comment thread Terminal49-API.postman_collection.json
Comment thread packages/mcp/src/tools/sql-query.ts Outdated
@alosec
alosec had a problem deploying to mcp-preview-feat/mcp-sql-query-tools October 15, 2026 18:16 — with GitHub Actions Failure
@alosec
alosec deployed to mcp-preview-feat/mcp-sql-query-tools October 15, 2026 18:16 — with GitHub Actions Active
@linear-code

linear-code Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

DEV-13504

DEV-13506

This branch was successfully deployed

3 active (1 outdated) deployments
Preview — 1605731a Deployed Oct 15, 2026 by vercel[bot]
staging - docs — 1605731a Deployed Oct 15, 2026 by mintlify[bot]
mcp-preview-feat/mcp-sql-query-tools — 425c912d Deployed Oct 15, 2026 by alosec via MCP preview 2025-11-25 #921
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.

1 participant