fix(v2): close the defects live probing found - #6681
Conversation
Staging finally deployed the merged release, so the surface could be exercised for real. Every fix already shipped held up. These are the defects only live traffic surfaced, plus the ones a static sweep had found and left. A cursor named a position in a sequence without naming the sequence. `cursorScopeKey` hashed only the caller's filters, so any two lists filtering on nothing but `workspaceId` produced one fingerprint and accepted each other's tokens: a tables cursor replayed against the knowledge list answered 200 and silently skipped a row. Table rows never reached that check at all, so a cursor from one table paged another. Identity now comes from the route's own contract — method plus resolved path — because a hand-written name is the step an author forgets, and forgetting it is invisible. An unresolved path placeholder throws rather than fingerprinting the template, so a misconfigured route fails on every request instead of an unlucky one. Every token minted before this is refused with an accurate message; they are single-walk and unpersisted. Knowledge search and the document list answered different questions. Search grouped same-tag filters by slot and joined them with OR while the list conjoined every filter, so `gte 9` and `lte 2` on one tag returned nothing from the list and a full billed page from search. Search now conjoins. The OR grouping replaced an explicit `|OR|` mechanism that was deleted outright, was never documented in any contract, and cost the ability to express a range on a single tag; the union it gave is still reachable as separate searches. The search body also accepted an unbounded query that was billed and then silently truncated to the embedding model's window, and ignored the tag-filter cap the list enforces. A body over ten mebibytes was reported as malformed JSON. Next's proxy truncates there, well under this app's fifty-megabyte ceiling, so the parse failed on a body the caller sent whole and the size branch was unreachable. The ceiling is now clamped to what the proxy will pass. Also: a group's output columns accepted a `workflowGroupId` and discarded it; an enrichment group could never gain an output, because a new output coordinate demanded workflow metadata a group with no workflow cannot have; `newOutputColumns` alone reported success and created nothing; a saved view stored layout references to columns that do not exist while refusing the same name in a filter; an MCP server stored `retries: 0` as three and overrode an explicit auth type; a disabled server answered tool discovery with an unclassified fault; rotating a header server's headers left it reading connected; and a run whose workflow was deleted reported the root folder path while also reporting the workflow deleted. Where the honest fix was out of reach, the contract was corrected instead of half-fixing the code: the polled run resource rebuilds `error.code` by matching the persisted message, so it can never report the two codes that need block attribution, and now says so. `OUTPUT_TOO_LARGE` is removed — no path ever emitted it. `triggers` was left alone deliberately. It reads as a closed enum but production holds 43 distinct values, because a webhook run stores its provider id; pinning the enum would refuse legitimate history a log search exists to find. The description now says the vocabulary is open.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Knowledge search conjoins every tag filter (including duplicate tags on the same slot), matching document list behavior; same-tag OR grouping is removed. The search body enforces query max length (embedding ceiling) and at most 10 tag filters. JSON bodies are clamped to the 10 MiB proxy ceiling so oversize requests return 413 instead of malformed JSON from truncated streams. Tables / views: workflow group output column schemas are strict (reject client MCP: Workflows / logs: OpenAPI and contract descriptions are updated across knowledge, logs, resources, tables, and workflows. Reviewed by Cursor Bugbot for commit 6dafb52. Configure here. |
Greptile SummaryThe PR addresses defects found while exercising the v2 API against staging.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/api/server/validation.ts | Clamps default and route-specific JSON body limits to the proxy ceiling so declared oversized requests return an accurate 413 response. |
| apps/sim/lib/api/server/validation.test.ts | Adds boundary coverage for default, explicit, below-ceiling, optional-body, and malformed-JSON parsing behavior. |
| apps/sim/lib/api/cursor-binding.ts | Strengthens cursor scope identity to include each route contract and resolved path. |
| apps/sim/lib/knowledge/search/queries.ts | Makes repeated knowledge tag filters conjunctive to match document-list semantics. |
| apps/sim/lib/mcp/application/use-cases.ts | Corrects MCP registration and update defaults, authentication selection, retries, and credential lifecycle behavior. |
| apps/sim/lib/table/application/groups.ts | Corrects output-column validation and enrichment-group output update behavior. |
| apps/sim/lib/table/views/service.ts | Validates saved-view layout references against existing table columns. |
Reviews (2): Last reviewed commit: "fix(v2): close the two holes the first r..." | Re-trigger Greptile
The previous commit clamped the default JSON body cap but left explicit per-route overrides alone, so a route declaring a larger `maxBodyBytes` still fell into the truncation it was meant to report: the four inline workspace-file routes at 70 MB and the deployed-chat route at 220 MB. Next attaches `proxyClientMaxBodySize` to every request and clones the body unconditionally for any non-GET method on a matched path, pushing EOF at ten mebibytes with only a warning, so the handler reads a truncated prefix. Those routes therefore already fail above that size — as a malformed-JSON 400. Clamping the effective limit inside the two body readers makes the same request fail as payload-too-large, quoting the limit actually in force. One existing test asserted the unreachable case, allowing a sixty-mebibyte base64 body; it now asserts what the proxy will forward intact. The inline-file path still advertises fifty mebibytes and cannot exceed the proxy ceiling until that ceiling is raised, which changes buffering for every route and belongs in its own change.
Both are places where a fix in this branch shut one door and left a smaller one open in the same wall. Letting an enrichment group gain an output meant skipping workflow resolution — but that resolution was the only thing validating a new output, so a PATCH began storing coordinates the runner can never fill. It fills a cell from `result[out.outputId]` and skips an output with no `outputId` at all, while the writer diffs on that same id and the sidebar reads and writes by it; the contract leaves it optional. The regression test added with that fix was itself asserting such a dead coordinate. Create's registry checks are now two shared helpers both paths call, and on update an output is exempt only when an identical binding already existed, so renaming a group whose enrichment has since changed still works while anything added or repointed must name a real output. `mappingUpdates` on an enrichment group now says it is inexpressible rather than resolving an empty workflow id into a missing workflow. The layout-reference check was handed the tolerant column set, so a placeholder minted to keep a dangling filter writable also whitelisted a brand-new layout reference — storing an entry the next read discards, which is the inconsistency the check was added to remove. Layout now resolves against the live columns, which is exactly what pruning keeps, while filters and sorts keep the exemption they need.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 6dafb52. Configure here.
Staging finally deployed the merged release, so the v2 surface could be exercised for real. Every fix already shipped held up — cursor scope ordering, NUL rejection, CORS
exposeHeaders, the enrichment-group P1, predicate cursor ordering, cost/duration refines,workflowGroupIdrejection, and tag-filter value coercion all verified live. These are the defects only live traffic surfaced, plus the ones a static sweep had found and left.Every finding below was reproduced against staging before any code changed, and each fix was verified red-then-green.
P1 — a cursor named a position without naming the sequence
cursorScopeKeyhashed only the caller's filters, so any two lists filtering on nothing butworkspaceIdproduced one fingerprint and accepted each other's tokens. Seven of seven same-sort pairs returned 200, and atablescursor replayed against/v2/knowledgesilently skipped a row. Table rows never reached that check at all, so a cursor from one table paged another — andPOST /queryshared the defect whenever no predicate was present.Identity now comes from the route's own contract — method plus resolved path — rather than a per-route literal, because a hand-written name is the step an author forgets and forgetting it is invisible. An unresolved path placeholder throws rather than fingerprinting the template, so a misconfigured route fails on every request instead of an unlucky one. A source sweep now enforces that every route calling
cursorScopeKeypasses its contract — closing the gap that let this hide, sincelist-pagination.test.tsreads contracts and never the route's actual call.Every token minted before this is refused with an accurate message. They are single-walk and unpersisted.
P0 — the two knowledge surfaces answered different questions
Search grouped same-tag filters by slot and joined them with
OR; the list conjoined every filter. Sogte 9ANDlte 2on one tag returned 0 documents from the list and a full billed page from search.Search now conjoins. The OR grouping replaced an explicit
|OR|mechanism that was deleted outright, was never documented in any contract, and cost the ability to express a range on a single tag — the union it provided is still reachable as separate searches. The search body also accepted an unboundedquerythat was billed and then silently truncated to the embedding window, and ignored the tag-filter cap the list enforces.A body over 10 MiB was reported as malformed JSON
Proven by sending an 11 MB body whose JSON terminated at 9 MB with trailing padding — it parsed, so the body is silently truncated, not stream-errored. The cause is Next's
proxyClientMaxBodySizedefault of 10 MiB, well under this app's 50 MB ceiling, leaving the size branch unreachable. The ceiling is clamped to what the proxy passes. (Math.minis load-bearing: the env default is hard-coded to 50 MB, so editing the constant alone would have been inert.)Also fixed
outputColumnsaccepted aworkflowGroupIdand discarded it — the sibling of the create-body fix, via anomit()-derived nested shape the top-level.strict()doesn't bindnewOutputColumnsalone reported success and created nothingretries: 0stored as3; an explicitauthTypeoverridden byoauthClientId; a disabled server answering tool discovery with an unclassified 500; a header rotation leaving the server readingconnected; and theauthTypePATCH default that revokes a stored OAuth grant when an SDK materializes itfolderPath: "/"while also reportingdeleted: true(9,642 production rows haveworkflow_id IS NULL)Where the honest fix was out of reach, the contract was corrected instead
The polled run resource rebuilds
error.codeby matching the persisted message, so it can never report the two codes needing block attribution — it now says so, and the PR records the exact three-file delta for the real fix (no migration needed; the trace spans already carry the block).OUTPUT_TOO_LARGEis removed — no path ever emitted it.One finding deliberately not fixed
triggersreads as a closed enum, but production holds 43 distinct values — a webhook run stores its provider id, including bothmicrosoft-teamsandmicrosoftteams. Pinning the enum would refuse legitimate history that a diagnostic log search exists to find. The description now states the vocabulary is open and matching is case-sensitive.Verification
check:audits26/26,check:api-validation— all clean