feat(workspace): zero-copy CLONE by default, large-COPY guard, fire-and-forget timeout fix - #692
Conversation
… fix (#687) workspace load now resolves a per-table load type instead of always COPYing: - Default (no --load-type) mirrors the server's LoadTypeDecider::canClone client-side and sends an explicit CLONE for every eligible table (same backend snowflake/bigquery, no external schema, unfiltered auto-sync alias, not a linked bucket on BigQuery), COPY for the rest -- each with a clone_ineligible_reason in the JSON result. - --load-type clone|copy|view forces the type for all tables; an ineligible explicit choice fails with the server's precise 400 instead of silently degrading. - COPY of a table larger than 1 GiB (WORKSPACE_LOAD_COPY_GUARD_BYTES) now asks for confirmation in human mode and refuses with the new WORKSPACE_LOAD_COPY_TOO_LARGE error (HTTP 400 over serve) unless --force is passed. - --timeout SECONDS (default 300 for workspace load) replaces the hard-coded 60 s wait; the shared storage-job timeout message now names the job id and warns the job continues running server-side, and STORAGE_JOB_TIMEOUT maps to exit 4 (retryable) like QUEUE_JOB_TIMEOUT. - Docs surfaces updated per convention #17 (vNEXT tags), including the read-only input-mapping alternative for analytics-only access.
padak
left a comment
There was a problem hiding this comment.
Review of #692 — workspace load: zero-copy CLONE by default, large-COPY guard, fire-and-forget timeout fix
Generated by
kbagent-pr-reviewersubagent. Verdict and findings below
are advisory; the human author retains every veto. CI-coverable issues
(lint, format, tests) are confirmed viamake check, not duplicated here.
Summary
This PR changes kbagent workspace load to auto-decide CLONE vs COPY per table (mirroring the server's LoadTypeDecider), adds an explicit --load-type clone|copy|view override, adds a >1 GiB COPY size guard requiring confirmation or --force, replaces the hard-coded 60s wait with a configurable --timeout (default 300s), and improves the shared storage-job timeout message to name the job id and warn it keeps running server-side. The change is implemented cleanly across all three layers (a new pure decision module _workspace_load_plan.py using a frozen dataclass, service orchestration with clear guard/plan separation, thin CLI/REST wiring), is exercised by 33 new tests across service/CLI/router/client layers, and every mandated documentation surface (CLAUDE.md, commands/context.py, keboola-expert.md, commands-reference.md, gotchas.md, workspace-workflow.md) was updated with the correct vNEXT placeholder. make check passes clean (6194 passed, 0 failed). Verdict: APPROVE — no blocking issues found; two minor non-blocking/nit items noted below.
Verdict
- Verdict: APPROVE
- Blocking findings: 0
- Non-blocking findings: 1
- Nits: 2
Blocking findings
(none)
Non-blocking findings
[NB-1] src/keboola_agent_cli/services/workspace_service.py:874 — timeout or WORKSPACE_LOAD_JOB_MAX_WAIT conflates 0.0 with None at the service layer
load_tables resolves the poll budget with max_wait=timeout or WORKSPACE_LOAD_JOB_MAX_WAIT. A timeout=0.0 passed directly to the service (bypassing both the CLI's if timeout <= 0 guard in commands/workspace.py and the router's Field(gt=0) on WorkspaceLoad.timeout) would silently fall back to the 300s default instead of raising, contradicting the CLI's own comment that "a zero budget cannot mean 'wait forever' either." Today this is unreachable through any shipped caller, but per CONTRIBUTING.md's "validate at boundaries" guidance the service is nominally the layer that should not trust upper-layer validation alone (e.g. a future SDK facade method calling load_tables directly would not get this check). Consider max_wait = timeout if timeout else WORKSPACE_LOAD_JOB_MAX_WAIT only if timeout is not None, i.e. explicitly branch on None rather than truthiness, or raise INVALID_ARGUMENT in the service itself for a non-positive value the way _normalize_load_type already does for the load type.
(none)
Nits
[NIT-1]src/keboola_agent_cli/commands/workspace.py:69—_print_load_result'ssize_note = f", {size_bytes / 1024**3:.2f} GB" if size_bytes else ""treats a legitimately empty table (data_size_bytes == 0) the same as "unknown" (None) and suppresses the size in human output. The JSON payload still carries the real0, so this is cosmetic only.[NIT-2]PR title"workspace load: zero-copy CLONE by default, large-COPY guard, fire-and-forget timeout fix"has no conventional-commit prefix, while the actual commit on the branch is correctlyfeat(workspace): .... If this repo squash-merges using the PR title as the commit subject, retitle the PR to keep the merged history's convention (feat:/feat(workspace):) that CONTRIBUTING.md requires.
Verification log
gh auth status→ authenticated aspadak,reposcope ✓gh pr view 692 --json title,body,files,additions,deletions,baseRefName,headRefName,labels,state→ OPEN, 22 files, +1510/-64, basemain✓git rev-parse --abbrev-ref HEAD→claude/issue-687-pr-70253ematches<branch>✓;git status -sbcleangit log --oneline main..HEAD→ single commitfeat(workspace): zero-copy CLONE load type, large-COPY guard, timeout fix (#687)— correct conventional prefix at the commit level ✓- Layer-violation greps (
typer/click/formatter/console.printinservices/;httpx/requestsincommands/;formatter/typerin clients) → all empty, no violations ✓ permissions.py—"workspace.load": "write"already present (pre-existing command, flags only; no new registry entry needed) ✓- Read
CONTRIBUTING.mdPlugin synchronization map + per-command checklist,CLAUDE.mdconvention #17,keboola-expert.md§1/§2/§3 — confirmed §2 row 126 ("Ad-hoc SQL / row-count / type audit") and §3 gotcha section updated for the new auto-CLONE default and size guard ✓ - Doc surfaces checked present in diff:
CLAUDE.md(## All CLI Commands),commands/context.py(AGENT_CONTEXT),keboola-expert.md(§2 + §4.4),commands-reference.md,gotchas.md(new dedicated section, tagged(since vNEXT, closes #687)),workspace-workflow.md(new "Load types" section) — all present and consistent ✓ wc -c plugins/kbagent/agents/keboola-expert.md→ 51325 B, well under the 70000 BPROMPT_BYTE_BUDGET✓- Magic-number / raw-error-code / bare-except /
print()/ token-leakage / new-bare-tuple-return greps on the diff → all clean;_workspace_load_plan.pyuses a frozen@dataclass(LoadTablePlan) for the multi-value per-table decision, matching the Code Quality Patterns convention ✓ - Test coverage count (new
def test_/+ def test_lines by file):test_workspace_service.py19,test_workspace_cli.py7,test_server_router_calls.py4,test_client.py2,test_helpers.py1 — service, CLI, REST router, and client layers all covered, including guard-declined/approved/json-refusal, explicit clone/copy/view skip-detail-fetch paths, invalid load-type, non-positive timeout ✓ tests/test_e2e.py::_test_workspace_loadextended with assertions onload_type_requestedand per-tableload_type(not juststatus == "ok") — matches the PR's claimed live E2E run structure in~/kbagent/e2e/workspace_load_e2e.py(read; matches PR description's 9-step recipe) ✓ — could not re-run live myself (noE2E_API_TOKEN/E2E_URLin this environment, and per project convention the CLI, not the reviewer, is the token custodian); relying on the author's reported live verification on both Snowflake (5946) and BigQuery (6100) projects.- Backward compatibility:
tables_loadedkey preserved verbatim; new keys (load_type_requested,tables[]) are additive;grep -rn 'Loaded.*table(s) into workspace' src/ tests/ plugins/ docs/found only the producing line and test fixtures (no downstream parser depends on the exactmessagestring) ✓; all other_wait_for_storage_jobcallers (branches.py,storage_tables.py,merge_requests.py) omitmax_waitand correctly keep defaulting toSTORAGE_JOB_TIMEOUTvia the newNone-means-default semantics ✓ STORAGE_JOB_TIMEOUTexit-code remap (1 → 4) cross-checked againstcommands/_helpers.py::map_error_to_exit_codeanddocs/error-codes.md;WORKSPACE_LOAD_COPY_TOO_LARGEadded to bothErrorCodeand_ERROR_CODE_TO_TYPE("validation") in the same PR, per CONTRIBUTING.md's "add it to_ERROR_CODE_TO_TYPEin the same PR" rule ✓server/app.py_CALLER_REFUSAL_CODES→WORKSPACE_LOAD_COPY_TOO_LARGEmaps to HTTP 400 instead of the default 502; confirmed bytest_workspace_load_copy_guard_answers_400intests/test_server_router_calls.py✓docs/web-server-endpoints.md— route path/summary forPOST /workspaces/{project}/{workspace_id}/loadunchanged (only the request-body schema gained fields), so no regen was needed;make check'sendpoints-checkstage passed, confirming no drift ✓make check(background run) → 6194 passed, 12 skipped, exit code 0 — lint, format, typecheck, skill freshness, version sync, command-sync, endpoints-check, changelog-check, error-code enum, sentinel-guards, and the full test suite all green ✓ (PR description reports 6191 passed/61 skipped from the author's own run; the skip-count delta is most likely local-environment E2E/skip-marker differences, not a regression — full suite passed either way)
Open questions for the author
(none)
- Map INVALID_ARGUMENT to HTTP 400 over serve (was the default 502): every reachable raise site is a kbagent-side pre-upstream refusal, so a client typo no longer reports as a gateway failure (Devin finding). - Validate timeout at the service boundary: a non-positive value raises INVALID_ARGUMENT instead of silently becoming the 300 s default when a future direct caller bypasses the CLI/router guards (reviewer NB-1). - Render a legitimately empty table as 0.00 GB in human output instead of hiding the size like an unknown one (reviewer NIT-1).
|
Review findings addressed in 53c05d1:
Both Devin findings are handled too: Full suite after the fixes: 6200 passed, 181 skipped. |
…ading (same CONTRIBUTING rule as 0af5071)
…ading (same CONTRIBUTING rule as 0af5071)
- SKILL.md triggers: zero-copy clone, workspace load type, table too big to load, load table into workspace cheaply - workspace-workflow: cheapest-first ordering (skip load > clone > view > copy), verify-via-tables[] note, and fix a misleading example -- the size guard only fires for resolved COPY, so --force on an explicit clone is a no-op; --json mode never prompts (--force up front) - typify-table / storage-types workflows: warn that a large typeless table COPY needs --force in non-interactive mode - keboola-expert: STORAGE_JOB_TIMEOUT is now exit 4 (retryable) and the job keeps running server-side
SKILL.md triggers for the new behavior (zero-copy clone, workspace load type; trimmed to the 1024-char Claude Desktop limit), workspace-workflow cheapest-first ordering + verify-via-tables[] note + corrected misleading example (--force is a no-op on an explicit clone; --json never prompts), size-guard warnings in typify-table and storage-types workflows, and the STORAGE_JOB_TIMEOUT exit-4 note in keboola-expert. Docs-only; plugin ships via git, no release needed.
Closes #687.
What changed
kbagent workspace loadno longer always COPYs. It now resolves a load type per table:LoadTypeDecider::canClonein keboola/connection: same backend (snowflake/bigquery), no external-schema bucket, alias only when column-auto-sync is ON and unfiltered, and on BigQuery not an Analytics-Hub-linked bucket. Ineligible tables fall back to COPY individually, each carrying aclone_ineligible_reasonin the JSON result. The mirror only picks the default — a stale rule costs a slower load, never a wrong one.--load-type clone|copy|viewforces the type for all tables and is sent as-is; an ineligible explicit choice fails with the server's precise HTTP 400 (the Storage API never silently degrades an explicitloadType).WORKSPACE_LOAD_COPY_GUARD_BYTES) asks for confirmation in human mode and refuses with the newWORKSPACE_LOAD_COPY_TOO_LARGEerror in--json/non-interactive mode unless--forceis passed. A 282 GB copy can no longer start silently (the incident that motivated the issue).--timeout SECONDS(default 300 for workspace load) replaces the hard-coded 60 s wait. The shared storage-job timeout message (all_wait_for_storage_jobcallers) now names the job id and warns that the job continues running server-side and keeps consuming backend resources, pointing atGET /v2/storage/jobs/{id}.STORAGE_JOB_TIMEOUTnow maps to exit 4 (network/retryable), consistent withQUEUE_JOB_TIMEOUT.load_type_requested(autowhen the flag was omitted) andtables: [{table_id, load_type, data_size_bytes, clone_ineligible_reason}]; existing keys are unchanged.POST /workspaces/{project}/{id}/loadgainsload_type/force/timeout; the guard refusal answers HTTP 400 (new_CALLER_REFUSAL_CODESmapping) instead of the misleading default 502.vNEXT): CLAUDE.md,commands/context.py, commands-reference, gotchas, workspace-workflow (incl. the read-only input-mappingKBC_<STACK>_<PROJECT>alternative for analytics-only access), keboola-expert (byte budget OK).Verification
services/_workspace_load_plan.py) parametrized over all COPY-fallback reasons; service planning/guard/timeout passthrough; CLI flag forwarding + confirm flow; router body + 400 mapping; timeout message + exit-code mapping.~/kbagent/e2e/workspace_load_e2e.py, run against both e2e projects on connection.us-east4.gcp.keboola.com):load_type=clone, data verified viaworkspace query(3 rows); explicitcopyandviewsucceed;--load-type bananaexits 2 withINVALID_ARGUMENT.Acceptance criteria from #687
--load-typeoverride works; ineligible combinations fail with a clear message (server-side 400, never a silent degrade)--force