AI-3699: RFC for full-table export tool - #664
Conversation
|
@claude review |
There was a problem hiding this comment.
Pull request overview
Adds an RFC describing a proposed export_table MCP tool for full-table exports via Storage API’s async export endpoint, plus a patch version bump to keep the repository’s “every PR bumps version” rule satisfied.
Changes:
- Added
feature_spec/table_export/RFC.mddetailing the problem, required behavior, open questions, and an implementation strategy for full-table exports. - Bumped project version from
1.74.7to1.74.8inpyproject.tomlanduv.lock.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
feature_spec/table_export/RFC.md |
New RFC proposing the full-table export tool and outlining key design decisions to resolve before implementation. |
pyproject.toml |
Patch version bump (1.74.7 → 1.74.8) for this RFC-only change. |
uv.lock |
Lockfile updated to reflect the new project version (1.74.8). |
Suppressed comments (2)
feature_spec/table_export/RFC.md:88
- This section references
_Workspace._wait_for_new_workspaceand uses a shortened path forjob_detail. In this repo, the job polling happens inWorkspaceManager._create_ws, and earlier sections usesrc/...paths; aligning both references avoids confusion.
- Reuse existing `job_detail(job_id)` (`clients/storage.py:848`) to poll the resulting
job — it already hits the correct `jobs/{id}` Storage-API endpoint (the same one
`_Workspace._wait_for_new_workspace` polls), no new client method needed for that
part.
feature_spec/table_export/RFC.md:98
- This bullet again references
_Workspace._wait_for_new_workspace, but the analogous polling loop in this codebase is inWorkspaceManager._create_ws. Updating the reference keeps the Resolution Strategy actionable.
- Polling/timeout logic modeled directly on
`_Workspace._wait_for_new_workspace` (`workspace.py:767-794`): loop on `job_detail`,
check `status == 'success'`, bounded by a timeout, `asyncio.sleep` between polls.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
9e37752 to
c4f97f5
Compare
c4f97f5 to
62b65e8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (4)
feature_spec/table_export/RFC.md:89
- The referenced polling helper
_Workspace._wait_for_new_workspacedoes not exist inworkspace.py; the polling loop is implemented inWorkspaceManager._create_ws(seesrc/keboola_mcp_server/workspace.py:702+, with the loop at ~767-794). Updating the reference will keep the RFC accurate and easier to follow.
timeout — exactly the pattern `_Workspace._wait_for_new_workspace`
(`workspace.py:767-794`) already uses for the same job system. No fire-and-poll split
is needed or currently supported.
feature_spec/table_export/RFC.md:107
- This bullet references
_Workspace._wait_for_new_workspace, but the existing code that polls Storage API jobs lives inWorkspaceManager._create_ws. Using the correct symbol/path will avoid pointing implementers at a non-existent helper.
- Reuse existing `job_detail(job_id)` (`clients/storage.py:848`) to poll the resulting
job — it already hits the correct `jobs/{id}` Storage-API endpoint (the same one
`_Workspace._wait_for_new_workspace` polls), no new client method needed for that
part.
feature_spec/table_export/RFC.md:120
- The polling/timeout logic reference points to
_Workspace._wait_for_new_workspace, but the actual example inworkspace.py:767-794is insideWorkspaceManager._create_ws. Referencing the right method makes it clear where to copy the pattern from.
- Polling/timeout logic modeled directly on
`_Workspace._wait_for_new_workspace` (`workspace.py:767-794`): loop on `job_detail`,
check `status == 'success'`, bounded by a timeout, `asyncio.sleep` between polls.
feature_spec/table_export/RFC.md:134
- In this RFC-only PR the version bump is patch (as noted in the PR description), while this scope bullet reads like a requirement for this change set. Consider clarifying that the implementation PR would require a minor bump +
uv lock.
- Version bump (minor — new tool) and `uv lock`.
keboola-pr-reviewer-bot
left a comment
There was a problem hiding this comment.
Verdict: auto_approve (risk 1/5) · profile keboola-mcp-server
RFC-only change with a patch version bump and synced lock — auto-approve.
There was a problem hiding this comment.
Does RFC require version change?
There was a problem hiding this comment.
Maybe not but it's in general guidance in claude.md, should I get rid of it?
There was a problem hiding this comment.
🤷 Probably leave it if it's in the guidance.
There was a problem hiding this comment.
Already addressed ✅
Leaving it in per the earlier thread — it's part of the general CLAUDE.md guidance, not an accidental action item for this RFC-only PR.
| internally submits the export and polls `job_detail` to completion, bounded by a | ||
| timeout — exactly the pattern `_Workspace._wait_for_new_workspace` | ||
| (`workspace.py:767-794`) already uses for the same job system. No fire-and-poll split | ||
| is needed or currently supported. |
There was a problem hiding this comment.
What happens when it times out before the file is ready? How does the agent continue to poll that job/file?
There was a problem hiding this comment.
Fixed ✅ in commit 54ebac7
Added explicit timeout/recovery behavior to open question #2 and Required Behavior #3: on timeout the tool returns a typed error containing the Storage job ID, and the documented recovery is calling export_table again (the export job keeps running server-side; resubmitting is cheap) rather than polling a separate endpoint. Also added a verification step for this. If repeated resubmission on very slow exports turns out to be costly in practice, a resume-by-job-ID path can be added later.
| These materially affect feasibility/shape and should be resolved in review, not | ||
| silently decided during implementation: | ||
|
|
||
| 1. **How is the exported data delivered back to the agent?** Two options: |
There was a problem hiding this comment.
I don't like option 1 as it automatically pushes whole table into LLM context. With option 2, agent can fetch whole file but only read parts of it as needed.
Also I don't think 2 step process is a problem here. IMO it is valid approach to provide file URL instead. Anyway I think it is sufficient for MVP and if it becomes an issue we can change it pretty easily, no?
There was a problem hiding this comment.
I would ask others for opinions on this as well.
There was a problem hiding this comment.
Fixed ✅ in commit 54ebac7
Agreed — switched the recommendation in open question #1 to the URL-based delivery path (default), for exactly the reason you raised: dumping the whole table into LLM context is rarely what's needed, and the agent can read only the parts of the file it wants. Inline content is now the fallback that could be added later behind fileType/gzip if a client that can't fetch URLs needs it. Updated Required Behavior #2, Resolution Strategy, and Verification to match.
There was a problem hiding this comment.
Does this also have Kai-side counterpart?
There was a problem hiding this comment.
Question ❓
No Kai-side counterpart planned as part of this RFC — I'll integrate it on the Kai side myself once this RFC is settled. Noted explicitly in the Scope section as tracked separately.
- Open question #1: switch the recommended delivery mode from inline content to a Storage file download URL, so a full table isn't forced into LLM context when the agent only needs part of it. - Open question #2: spell out timeout behavior — the tool returns a typed error with the Storage job ID, and recovery is re-calling export_table rather than polling a separate endpoint. - Scope: clarify this RFC PR only adds the document; the listed scope (tests, TOOLS.md regen, version bump) belongs to the implementation PR that follows it. Also note Kai-side integration is tracked separately, to be picked up once this RFC is settled.
Dismissing prior approval — a new commit was pushed and this review was for an earlier SHA. Run @keboola-pr-reviewer-bot review to get a fresh verdict.
|
New commit on |
Description
Linear: AI-3699
Change Type
Summary
RFC only — no implementation in this PR.
query_datais the only tool that returns table rows today, and it hard-caps results atMAX_ROWS = 1_000(src/keboola_mcp_server/tools/sql.py:27). This blocks any automationthat needs to pull a full Storage table through the MCP server, and simply raising
MAX_ROWSdoesn't fix it —query_dataruns synchronously and returns rows inline,which doesn't scale to full-table exports. Storage API already supports this via an
async export job (
POST /v2/storage/branch/{branchId}/tables/{tableId}/export-async)that produces a file in File Storage — the MCP server has no client method or tool
using it yet.
Adds
feature_spec/table_export/RFC.mdproposing a newexport_tabletool built onthat endpoint. It maps 1:1 to the real
ExportTableRequest2request body (fileType,columns, format, gzip, includeInternalTimestamp, limit, orderBy, whereFilters,
timezone, sourceBranchId), and correctly models the export as a Storage-API job
(
GET /v2/storage/jobs/{id}via the existingAsyncStorageClient.job_detail) — adifferent system from the Job Queue that
run_job/get_jobspoll — so the toolsubmits, polls, and returns in a single call, mirroring the existing
_Workspace._wait_for_new_workspacepattern.The tool returns a Storage file download URL rather than inline content by default —
a full table dumped into LLM context is rarely what's needed, and this lets the agent
read only the parts of the file it actually wants. Timeout behavior is also spelled out:
on timeout the tool returns a typed error with the Storage job ID, and recovery is
re-calling
export_tablerather than polling a separate endpoint. A remaining openquestion covers whether unrestricted full-table export needs a size/row guard.
Explicitly out of scope for the eventual implementation: changes to
query_data/MAX_ROWS(tracked separately in AI-2772), pushing the exported data anywhere on thecaller's behalf, the general "too many MCP tools" tool-management question raised
separately (candidate for its own RFC/spike), and Kai-side integration (tracked
separately, to be picked up once this RFC is settled).
Version bump is patch-only (docs/RFC change per CLAUDE.md versioning rules); no code
changes in this PR.
Testing
Streamable-HTTPtransports)Optional testing
canary-orionMCP (Streamable-HTTP)canary-orioncanary-orionN/A — RFC-only change, no runtime behavior to test.
Checklist