feat(tables): folder tools for the Table block, and folders as picker scope - #7496
feat(tables): folder tools for the Table block, and folders as picker scope#7496mzxchandra wants to merge 6 commits into
Conversation
… scope Tables already had the folder machinery - the five folder operations, their application use cases, and a public v2 route - but no agent-callable tools, so a run could query a table's rows and could not file the table anywhere. Six tools, mirroring the File block's set: `table_list_folders`, `table_create_folder`, `table_update_folder`, `table_delete_folder`, `table_restore_folder`, and `table_move`. Restore is addressed by the path the folder held when it was deleted rather than by id, because that is what `restoreTableFolderUseCase` takes; the response reports where the folder actually landed, which differs when its old parent is still archived or a live sibling has taken its name. Executor admission. The folder operations delegated to Copilot only, so every one of these tools would have 403'd. Widened to `toolReadOperation` / `toolWriteOperation(id, 'tables.use')`: tables.folders.list <- table_list_folders tables.folders.create <- table_create_folder tables.folders.update <- table_update_folder tables.folders.delete <- table_delete_folder tables.folders.restore <- table_restore_folder tables.update <- table_move `tables.use` rather than `tables.create` throughout: a folder organizes, it does not add a table, so a workspace with table creation withheld can still file what it has. `table_move` reuses `updateTableUseCase` with only `folderPath` set rather than growing a second way to move a table. Both `check:actorless-executor-operations` and `check:capability-subject` pass with the widening - neither the folder use cases nor `updateTableUseCase` requires a human subject, so an actorless run (schedule, webhook, deployed API) can reach them without a 500. Dispatch is in-process, as new cases in `executeTableTool`. The dispatcher keyed every case to a routed contract, which these have no business inventing - there is no HTTP route for them - so it now carries a response schema and the folder cases pass standalone schemas through `parseInternalOperationInput`, the path the Knowledge handler already takes. Reusing the public v2 folder contracts was the alternative and was rejected: `recursive` there is a `z.stringbool()` shaped by URL query encoding, and this flag is the guard between deleting one empty folder and deleting a subtree. None of the tools sends a `workspaceId`. The executor mints a delegated principal bound to the run's workspace and the operations read it from there, so accepting one would declare a field the server ignores. `table_move` is still scoped to its table, but through its own schema: `getTableContract`, which the older scoped tools use, requires the `workspaceId` these deliberately omit, and reading it that way rejected every move as malformed. On `table_v2`, each folder path is a tree selector paired with a manual text entry so a `<reference>` can be typed on a text surface. The one exception is the Folder that narrows the table picker: it never travels, because a folder cannot stand in for a table the way it stands in for a file, so it is basic-mode only rather than a pair whose advanced half would resolve a reference and have it discarded. It renders above the picker it narrows - below reads as a second choice rather than a filter. The scope comparison is by decoded segment, never by string prefix: `/a/bc` starts with `/a/b` and is not inside it, and a folder genuinely named `Q3/Q4` is one level. `isFileInFolderScope` now delegates to the same predicate instead of keeping a second copy.
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
…ble selection Two review findings, both on the block surface. A block used as an Agent tool is handed the TOOL's schema, not this block's subblock ids - `createLLMToolSchema(toolConfig, ...)` in `providers/utils.ts` - so a model answers with `path` / `folderPath` while the canvas stores `folderRef` / `moveTargetRef`. Every folder transformer read only the canvas id and dropped the model's answer. Five of them then failed schema validation, which is loud and recoverable. `move` did not: the tool substitutes the workspace root for an absent destination, so "move into /Reports" became "move to the root" with a success result. The canvas value still wins where the author set one; the model's answer is the fallback, matching what `columns` already does in this file. `update_folder` branches on the canvas source instead, because its destination is composed rather than picked and an unset parent is a real destination (the root), not a missing one. The delete cascade deliberately gets no model fallback - it is `user-only` on both the tool param and the subblock. Narrowing the folder scope filtered the selected table out of the picker without touching the stored value, so the combobox showed its placeholder while the block kept executing against the hidden table. The selection is now cleared when it falls out of scope - but only for a table that is actually loaded and actually out of scope, since a table missing from the list is still-loading or deleted, and clearing there would destroy a valid config over a transient cache state.
…eal delete guard Rebasing onto staging after the File folder work landed (#7393). Two adjustments to what that merge changed, and one correction. `readFolderPath` moved from under the sub-block component tree to `lib/folders/selection.ts`, and the `sim-folder-tree-selector` subblock type was consolidated into the pre-existing `folder-selector`. Both are better homes; this follows them. The delete-cascade assertion named the wrong layer. It claimed "the block's own schema is what an Agent tool surface is built from", which contradicts what this same change established for the move destination: `createLLMToolSchema` reads `toolConfig.params`, not the block's subblocks. So the guard that actually keeps a model out of the recursive delete is the TOOL param's `visibility: 'user-only'` — it drives both the omission from the model's schema and `modelBlockedParams`, which `stripModelBlockedParams` then removes from the model's arguments before merge. The subblock's `paramVisibility` governs the tool-input row UI via `isToolParamUserRequired`, which is a real thing but not a guard. Both are still set; the test now asserts the tool param first and says which one enforces.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
`workspace-folder-selector` stores a string when multiSelect is falsy and an array when it is true. TableSelector's narrowing reads a string, so flipping this field to multi-select would make the scope read as absent and the picker would silently stop filtering. The coupling was real but unpinned; the File block's equivalent is deliberately multi-select.
There was a problem hiding this comment.
1 issue found and verified against the latest diff
Confidence score: 3/5
- In
apps/sim/lib/internal/table/operations.ts,moveTableToFoldercan move the table successfully but reporttable_moveas failed when the subsequent authoritative read errors, leaving callers with an incorrect outcome and possible retry ambiguity—handle the partial result like the v2 PATCH route or return an explicit partial-success response.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/sim/lib/internal/table/operations.ts">
<violation number="1" location="apps/sim/lib/internal/table/operations.ts:659">
P2: When `moveTableToFolder` succeeds but the following authoritative read fails, this line makes `table_move` report an error after moving the table. Handle partial results like the v2 PATCH route, or return an explicit applied-fields result for this tool.</violation>
</file>
Heads up: you’re close to your flex budget. Increase your flex budget so reviews don’t pause.
Fix all with cubic | Re-trigger cubic
| * so a failure always means nothing was applied and the honest answer is to | ||
| * rethrow it rather than present a move that did not happen. | ||
| */ | ||
| if (result.failure) throw result.failure |
There was a problem hiding this comment.
P2: When moveTableToFolder succeeds but the following authoritative read fails, this line makes table_move report an error after moving the table. Handle partial results like the v2 PATCH route, or return an explicit applied-fields result for this tool.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/sim/lib/internal/table/operations.ts, line 659:
<comment>When `moveTableToFolder` succeeds but the following authoritative read fails, this line makes `table_move` report an error after moving the table. Handle partial results like the v2 PATCH route, or return an explicit applied-fields result for this tool.</comment>
<file context>
@@ -475,3 +499,175 @@ export async function executeTableUpsertRow(
+ * so a failure always means nothing was applied and the honest answer is to
+ * rethrow it rather than present a move that did not happen.
+ */
+ if (result.failure) throw result.failure
+ if (!result.table || result.folderPath === null) {
+ throw new Error('Moved table is missing from the authoritative result')
</file context>
Reading the scope by field id is correct only while it has no advanced twin. With both halves of a canonical pair filled the serializer resolves the basic member, so a hand-rolled read can disagree with what the run uses; the file picker goes through useActiveCanonicalSubBlockValue for exactly that reason. Names the hook to reach for if a twin is ever added.
Greptile SummaryThis PR adds six agent-callable table-folder operations, widens the corresponding application-operation admission for executor principals, and introduces folder-scoped table selection in the workflow editor.
Confidence Score: 3/5The PR is not yet safe to merge because ordinary table-placement changes can erase a persisted workflow selection, and the explicit canvas-sentence requirement must also be satisfied. The new picker effect persistently clears a valid table ID whenever resource data places that table outside the configured design-time scope, even without the workflow author changing the scope; the List Folders card also violates the repository's empty-target sentence rule. Files Needing Attention: apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/table-selector/table-selector.tsx, apps/sim/blocks/blocks/table_v2.ts
|
| Filename | Overview |
|---|---|
| apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/table-selector/table-selector.tsx | Adds folder-scoped picker filtering, but resource-list changes can now persistently erase a valid selected table. |
| apps/sim/blocks/blocks/table_v2.ts | Adds folder operations and parameter mapping; the List Folders canvas sentence misrepresents its empty-root behavior. |
| apps/sim/lib/api/contracts/tools/table.ts | Defines bounded standalone request and response schemas for the six in-process tools. |
| apps/sim/lib/internal/table/execute-tool.ts | Registers validated dispatch and scoped executor-principal handling for the new operations. |
| apps/sim/lib/internal/table/operations.ts | Maps tool inputs to authorized folder/table use cases and projects validated outputs. |
| apps/sim/lib/table/application/folders.ts | Extends authorized folder listing with recursive traversal, depth bounds, and canonical result metadata. |
| apps/sim/lib/table/application/operations.ts | Admits executor principals to the necessary table and folder semantic operations without exposing additional current executor call paths. |
| apps/sim/tools/table/folders.ts | Defines and documents the six tools, including a correctly enforced user-only recursive-delete parameter. |
Sequence Diagram
sequenceDiagram
participant User as User or Agent
participant Block as Table Block
participant Tool as Table Folder Tool
participant Dispatch as Internal Table Dispatcher
participant UseCase as Authorized Table Use Case
participant Store as Folder/Table Storage
User->>Block: Configure or invoke folder operation
Block->>Tool: Map canonical tool parameters
Tool->>Dispatch: Execute registered tool ID
Dispatch->>Dispatch: Validate input and mint scoped principal
Dispatch->>UseCase: Run semantic table operation
UseCase->>Store: Read or mutate folders/tables
Store-->>UseCase: Authoritative result
UseCase-->>Dispatch: Audited domain result
Dispatch-->>Tool: Validated response
Tool-->>User: Structured tool output
Reviews (1): Last reviewed commit: "fix(tables): follow the folder selector ..." | Re-trigger Greptile
| useEffect(() => { | ||
| if (isPreview || disabled || isLoading || !tableId) return | ||
| if (!tables.some((table) => table.id === tableId)) return | ||
| if (scoped.some((table) => table.id === tableId)) return | ||
| setStoreValue('') | ||
| }, [isPreview, disabled, isLoading, tableId, tables, scoped, setStoreValue]) |
There was a problem hiding this comment.
Scope changes erase selections
This effect runs for every change to tables or scoped, not only when the user changes the folder scope. If the selected table is moved outside the scope by another workspace surface or collaborator, the table remains valid, but this code calls setStoreValue('') and immediately persists the empty value. An organizational table move can therefore erase the workflow's table configuration even though the folder scope is described as a design-time filter that does not affect execution.
Knowledge Base Used:
| { text: 'from', field: TABLE_FIELD, core: true }, | ||
| ], | ||
| get_schema: [{ text: 'Read the schema of', field: TABLE_FIELD, core: true }], | ||
| list_folders: [{ text: 'List table folders in', field: FOLDER_PATH_FIELD, core: true }], |
There was a problem hiding this comment.
Empty scope misstates behavior
The optional folder path is marked core, so an untouched List Folders card appears to target one folder even though omitting the path lists from the entire workspace root. This violates the repository directive that operations whose empty target means “act on all” must use literal copy that remains accurate in both empty and configured states. This requirement must be satisfied before merging.
| list_folders: [{ text: 'List table folders in', field: FOLDER_PATH_FIELD, core: true }], | |
| list_folders: ['List table folders', { text: 'in', field: FOLDER_PATH_FIELD }], |
Context Used: Custom context (source)
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!
Summary
Adds the six folder tools to the Table block, so an agent can organize tables and not just read them. Rebased onto staging now that the File folder work has landed (#7393).
Tables already had the folder machinery — the five folder operations, their application use cases, and a public v2 route — but no agent-callable tools, so a run could query a table's rows and could not file the table anywhere.
Six tools:
table_list_folders,table_create_folder,table_update_folder,table_delete_folder,table_restore_folder,table_move.Restore is addressed by the path the folder held when it was deleted rather than by id, because that is what
restoreTableFolderUseCasetakes. The response reports where the folder actually landed, which differs when its old parent is still archived or a live sibling has taken its name.Executor admission
The folder operations delegated to Copilot only, so every one of these tools would have 403'd. Widened to
toolReadOperation/toolWriteOperation(id, 'tables.use'):tables.folders.listtable_list_folderstables.folders.createtable_create_foldertables.folders.updatetable_update_foldertables.folders.deletetable_delete_foldertables.folders.restoretable_restore_foldertables.updatetable_movetable_movereusesupdateTableUseCasewith onlyfolderPathset rather than growing a second way to move a table. It never setsnameordescription, so no executor path gains rename powers.check:actorless-executor-operationsandcheck:capability-subjectboth pass: neither the folder use cases norupdateTableUseCaserequires a human subject, so an actorless run (schedule, webhook, deployed API) reaches them without an opaque 500.Dispatch
In-process, as new cases in
executeTableTool. The dispatcher keyed every case to a routed contract, which these have no business inventing — there is no HTTP route for them — so it now carries a response schema, and the folder cases pass standalone schemas throughparseInternalOperationInput, the path the Knowledge handler already takes.Reusing the public v2 folder contracts was the alternative and was rejected:
recursivethere is az.stringbool()shaped by URL query encoding, and this flag is the guard between deleting one empty folder and deleting a subtree.None of the tools sends a
workspaceId. The executor mints a delegated principal bound to the run's workspace and the operations read it from there, so accepting one would declare a field the server ignores.table_moveis still scoped to its table, but through its own schema:getTableContract, which the older scoped tools use, requires theworkspaceIdthese deliberately omit.Agent-tool param mapping
A block used as an Agent tool is handed the tool's schema, not its own subblock ids (
createLLMToolSchema(toolConfig, ...)), so a model answers withpath/folderPathwhile the canvas storesfolderRef/moveTargetRef. Reading only the canvas id dropped the model's answer — and becauseproviders/utils.ts:664merges with a spread ({ ...result, ...transformed }), returningundefinedoverwrites the model's value rather than leaving it alone. Onmovethe tool then substituted the workspace root, turning "move into /Reports" into "move to the root" with a success result.Canvas value wins where the author set one; the model's answer is the fallback, matching what
columnsalready does in this block.update_folderbranches on the canvas source instead, because its destination is composed rather than picked and an unset parent is a real destination (the root), not a missing one.Block surface
Each folder path is a tree selector paired with a manual text entry so a
<reference>can be typed on a text surface. The exception is the Folder that narrows the table picker: it never travels, because a folder cannot stand in for a table the way it stands in for a file, so it is basic-mode only. It renders above the picker it narrows — below reads as a second choice rather than a filter. Selecting a folder that excludes the current table clears the selection, so the picker never shows a placeholder while the block still runs against a hidden table.Scope comparison is by decoded segment, never by string prefix:
/a/bcstarts with/a/band is not inside it, and a folder genuinely namedQ3/Q4is one level.Type of Change
Testing
567 test files / 10037 tests pass across the touched areas. 8 new test files. All 45 audits pass, plus
check:api-validation,check:client-boundary, biome.Tests deliberately cover the seam between "the schema accepted the field" and "the use case received it" — a tool option that parses and is then dropped looks identical to one that works from both ends.
End-to-end, driven through the real UI as real workflow runs and verified in Postgres:
create folderCreated table folder "/Q3%20Results"— the canonical percent-encoded path composed from the typed namemove tableQ3_Pipelinemoved fromReportsintoQ3 Resultsdelete folder, cascade off, non-emptyFolder is not empty; both folders intactdelete folder, cascade on["leads","q3_pipeline"]; Folder =Reports→["q3_pipeline"]Reviewers should focus on: the
tables.updatewidening fortable_move, and the delete cascade guard. The guard that keeps a model out of the cascade is the tool param'svisibility: 'user-only'— it drives both the omission from the model's schema andmodelBlockedParams, whichstripModelBlockedParamsremoves from the model's arguments before merge. The subblock'sparamVisibilityis also set, but that governs the tool-input row UI, not the model schema.Known issue, pre-existing and not from this PR
table_restore_folderthrows in local dev only.restoreFolder(lib/folders/orchestration.ts) wraps its body inwithFolderTreeLockbut drops thetx, so the body queries the globaldbpool inside the transaction and tripspackages/db/tx-tripwire.ts. That tripwire isthrowwhenNODE_ENV !== 'production'andwarnin production — re-running the same restore withDB_TX_TRIPWIRE=warnreturns{"success":true,"restoredItems":{"folders":1,"tables":1}}.Still present on staging as of this PR, and not introduced here: the Recently-deleted restore path in
lib/resources/orchestration/restore-resource.tshits it the same way for workflow and knowledge folders. The sibling transitions (create, relocate) both threadtx; restore is the odd one out. Fixing it means threading a tx throughrestoreFolderWithoutTreeLockand its per-resourcerestoreChildrenhooks across four resource types, so it belongs in its own PR.Checklist