feat: Add analytics catalog and query tools#57
Conversation
- Introduced `run_analytics_query` tool for executing SQL analytics against contract data. - Added detailed documentation for the tool in AGENTS.md and analytics_query_docs.ts. - Implemented error handling for redirect responses in SpotDraftClient. - Updated README.md and AGENTS.md to include the new analytics tool.
- Added `get_analytics_catalog` tool for retrieving allowlisted analytics tables and column schemas. - Updated README.md and AGENTS.md to include the new analytics tool. - Enhanced analytics_query_docs.ts with guidance on using `get_analytics_catalog` for accurate schema discovery. - Revised AGENTS.md in the analytics directory to reflect the new tool and its usage.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e9c7aef22
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| body.user_scoped = request.user_scoped; | ||
| } | ||
|
|
||
| return spotdraftClient.post('/v2.1/public/analytics/query', body); |
There was a problem hiding this comment.
run_analytics_query posts to /v2.1/public/analytics/query (no trailing slash), but this tool’s own upstream contract documents the route as /v2.1/public/analytics/query/. In this same commit, SpotDraftClient.post() was changed to redirect: 'manual' and now throws on any 3xx via rejectIfRedirect, so environments that canonicalize the missing-slash URL with a 301/308 will now fail every analytics query with a 502 instead of executing it.
Useful? React with 👍 / 👎.
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| throw new APIError( | ||
| `SpotDraft API returned HTTP ${response.status} redirect (${location ?? 'missing Location header'}). Set baseUrl to the canonical API URL so requests do not redirect (scheme, host, path, trailing slashes).`, | ||
| 502, | ||
| { endpoint, method, redirectStatus: response.status, location, requestUrl: url }, |
There was a problem hiding this comment.
Since location/requestUrl are passed verbatim into APIError and MCPError.toJSON() serializes context unchanged, should we omit or redact them from client-facing error context?
Finding type: Basic Security Patterns | Severity: 🔴 High
Want Baz to fix this for you? Activate Fixer
Prompt for AI Agents:
Before applying, verify this suggestion against the current code. In
src/spotdraft_client.ts around lines 124-135 in the rejectIfRedirect method, stop
including redirect target and full request URL in the thrown APIError context. Instead
of passing location and requestUrl verbatim into the context, either omit those fields
entirely or sanitize them to only safe data (e.g., endpoint and API origin without query
string/internal path); ensure MCPError.toJSON (and any shared context serialization)
does not re-emit these sensitive values unchanged. Update the corresponding call sites
for rejectIfRedirect in get (around lines 137-168) and post (around lines 195-205) if
they rely on the removed context fields, and adjust tests/docs accordingly.
| const runAnalyticsQuery = async ( | ||
| request: RunAnalyticsQueryRequest, | ||
| spotdraftClient: SpotDraftClient, | ||
| ): Promise<unknown> => { | ||
| const query = request.query?.trim() ?? ''; | ||
| if (query === '') { | ||
| throw new ValidationError('query is required and cannot be empty.', { field: 'query' }); |
There was a problem hiding this comment.
request isn’t defaulted, so if MCP arguments is omitted the handler can pass undefined and request.query?.trim() throws before ValidationError('query is required...') can run—should we normalize to {} first?
Finding type: Logical Bugs | Severity: 🟠 Medium
Want Baz to fix this for you? Activate Fixer
Prompt for AI Agents:
Before applying, verify this suggestion against the current code. In
src/tools/analytics/run_analytics_query.ts around lines 34-40 in the runAnalyticsQuery
function, the code uses request.query?.trim() but assumes request is always defined; if
the dispatcher passes undefined, this can throw a TypeError before the existing
ValidationError logic runs. Refactor the function signature or first line to normalize
request to an empty object when it’s undefined (e.g., default request = {}). Then
compute and validate the trimmed query from that normalized object so the “query is
required and cannot be empty” ValidationError is consistently returned for omitted or
empty inputs.
| body.user_scoped = request.user_scoped; | ||
| } | ||
|
|
||
| return spotdraftClient.post('/v2.1/public/analytics/query', body); |
There was a problem hiding this comment.
run_analytics_query calls /v2.1/public/analytics/query but the contract expects POST /v2.1/public/analytics/query/, and with SpotDraftClient rejecting 3xx redirects this should be updated to the slash-suffixed path.
Finding type: Breaking Changes | Severity: 🔴 High
Want Baz to fix this for you? Activate Fixer
Prompt for AI Agents:
Before applying, verify this suggestion against the current code. In
`src/tools/analytics/run_analytics_query.ts` around lines 54-54 inside the
`runAnalyticsQuery` function, update the `spotdraftClient.post` path from
`'/v2.1/public/analytics/query'` to the canonical slash-suffixed endpoint
`'/v2.1/public/analytics/query/'` as specified by the analytics contract/docs. This
prevents a 3xx redirect that now gets rejected (causing a 502) due to `SpotDraftClient`
using `redirect: 'manual'`. After changing the path, quickly scan the repo for any other
calls to the no-slash analytics query endpoint and align them to the slash-suffixed
version.
| const getAnalyticsCatalog = async ( | ||
| request: GetAnalyticsCatalogRequest, | ||
| spotdraftClient: SpotDraftClient, | ||
| ): Promise<unknown> => { | ||
| const endpoint = '/v2.1/public/analytics/catalog/'; | ||
| const search = request.search?.trim(); | ||
| const queryParams = search !== undefined && search !== '' ? new URLSearchParams({ search }) : undefined; | ||
| return spotdraftClient.get(endpoint, queryParams); |
There was a problem hiding this comment.
handleCallToolRequest() reads request.search even though request.params.arguments can be omitted per MCP, so empty calls can throw before tools can return the no-search catalog—can we default request to {} (or in the dispatcher) before accessing search?
Finding type: Logical Bugs | Severity: 🔴 High
Want Baz to fix this for you? Activate Fixer
Prompt for AI Agents:
Before applying, verify this suggestion against the current code. In
src/tools/analytics/get_analytics_catalog.ts around lines 23-30, in the
`getAnalyticsCatalog` function, make the `request` parameter resilient to being omitted
by defaulting it to an empty object (e.g., treat `undefined` as `{}`) before reading
`request.search`. This prevents dereferencing/reading `search` when the MCP `tools/call`
omits `arguments` and ensures the tool returns the full catalog when no search is
provided. Apply the normalization at the entry point of `getAnalyticsCatalog` (or
equivalently in the dispatcher that calls it) and keep the existing trim/empty-string
behavior for `search`.
2 participants
User description
Overview
This PR introduces new Analytics tools to the SpotDraft MCP server, allowing users to discover schemas and execute read-only GoogleSQL queries against their workspace's analytics data. Additionally, it improves the core API client by enforcing strict handling of HTTP redirects to prevent unintended request alterations.
Key Changes
1. New Analytics Tools
get_analytics_catalog: Added a new tool to fetch allowlisted analytics tables and column schemas. It includes an optionalsearchparameter to filter results by table or column name/description.run_analytics_query: Added a new tool to execute read-only GoogleSQL against analytics data. Features include:user_scopedboolean parameter to toggle workspace-wide vs. user-restricted record visibility.analytics_query_docs.ts).2. HTTP Redirect Handling in
SpotDraftClientSpotDraftClientGETandPOSTmethods to useredirect: 'manual'.rejectIfRedirect, a helper method that catchesHTTP 3xxresponses and throws an explicitAPIError. This preventsfetchfrom silently following redirects (which is critical, as following a redirect can strip payloads and convertPOSTrequests intoGETrequests).3. Documentation & Tool Registration
README.md: Listed the newAnalyticssection and its available tools.AGENTS.md: Updated the central tool listing and added a dedicatedsrc/tools/analytics/AGENTS.mdfile to provide maintainers with upstream API context and internal component responsibilities.src/tools/index.ts: Registeredget_analytics_catalogToolandrun_analytics_queryToolin the global tool registry.Generated description
Below is a concise technical summary of the changes proposed in this PR:
Add new analytics catalog discovery and query tools so users can inspect allowlisted schemas and execute read-only GoogleSQL through
get_analytics_catalogandrun_analytics_query. Document their availability in the registry and reference materials to guide maintainers on the analytics surface.get_analytics_catalogandrun_analytics_queryusage.Modified files (7)
Latest Contributors(2)
SpotDraftClientGET/POST so the newrejectIfRedirecthelper surfaces HTTP 3xx responses instead of silently following redirects.Modified files (1)
Latest Contributors(2)