fix(v2): hold create to the rules update enforces, and bind the last two cursors - #6684
Merged
Merged
Conversation
…two cursors Three defects, one shape: a rule applied to one path and not its sibling. Two were found by probing the live surface after the previous fixes deployed, and the third by reading for the pattern. Creating a workflow group through the public surface validated almost nothing the update path validates. An enrichment group could name an enrichment the registry does not define, or an output the enrichment does not have, or carry no output id at all — each a 201 storing a column no run can ever write, discovered only when the caller later tried to edit the group and got the 400 create should have given. The workflow half was the same: a fabricated block-and-path coordinate was stored on create and refused on update. Create now runs the same two registry helpers and the same workflow-output check the update path uses. The discriminator there is the backing workflow id, not the declared type. The workflow sidebar creates enrichment-template groups labelled `enrichment` while backed by a real workflow and carrying no enrichment id, so keying on the label would have refused the first-party create path outright. A group's producer type could also be relabelled after the fact into a state creation refuses. Nothing rejected it and nothing could repair it, since the update body carries no enrichment id to supply. Relabelling an enrichment group as workflow-backed is the harmful direction: it keeps the enrichment id while moving the group onto the workflow branch with an empty workflow id, so every cell run fails. An update may now only restate the type the group already has. The workflow-version and workspace-member lists were the last two paged reads minting cursors with no route identity, so a token from one parent resumed another at a position that silently skips rows — the defect the previous change closed everywhere else. Both now wrap their domain token with the same scope binding, and the pagination guardrail gained a declaration of every nested list's parent path param, because the old one recorded only query filters and so could not tell an unfiltered list from a forgotten parent.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryCursor Bugbot is generating a summary for commit 717b4bf. Configure here. |
Contributor
Greptile SummaryThe PR aligns table-group creation validation with update validation, prevents producer-type relabeling, and scopes the remaining nested-list cursors to their parent resources.
Confidence Score: 5/5The PR appears safe to merge, with no concrete changed-code-triggered failures identified. The new validation and cursor-binding paths match existing contracts and sibling implementations, while the type guard preserves same-type updates and downstream missing-group handling.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/table/application/groups.ts | Adds create-time output validation and prevents update-time producer-type changes without exposing a concrete regression. |
| apps/sim/app/api/v2/workflows/[id]/versions/route.ts | Consistently binds version cursors to the workflow path parameter when reading and minting tokens. |
| apps/sim/app/api/v2/workspaces/[workspaceId]/members/route.ts | Consistently binds member cursors to the workspace path parameter when reading and minting tokens. |
| apps/sim/lib/api/contracts/tables.ts | Updates the producer-type contract description to reflect the newly enforced immutability. |
| apps/sim/lib/api/contracts/v2/tests/list-pagination.test.ts | Adds an audit guard requiring nested paginated routes to declare all parent path bindings. |
| apps/sim/lib/table/application/groups.test.ts | Covers invalid create coordinates, valid workflow-backed templates, forbidden relabeling, and no-op type echoes. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Request[Paginated list request] --> Scope[Derive scope from route and parent ID]
Scope --> Cursor{Cursor supplied?}
Cursor -->|No| FirstPage[Load first page]
Cursor -->|Yes| Verify[Read scoped cursor]
Verify -->|Scope mismatch or malformed| Reject[Return validation error]
Verify -->|Valid| Decode[Decode and validate inner keyset]
Decode --> Load[Load page from keyset]
FirstPage --> Present[Present results]
Load --> Present
Present --> More{More results?}
More -->|Yes| Mint[Mint cursor with same parent scope]
More -->|No| Null[Return null nextCursor]
Reviews (1): Last reviewed commit: "fix(v2): hold create to the rules update..." | Re-trigger Greptile
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three defects, one shape: a rule applied to one path and not its sibling. Two were found by probing the live surface after #6681 deployed; the third by reading for the pattern. Each was reproduced before any code changed, and each fix is verified red-then-green.
Create validated almost nothing update validates
createTableGroupUseCase— whatPOST /api/v2/tables/{tableId}/groupscalls — never received the registry checks #6681 added toupdateTableGroupUseCase. Confirmed live on a self-created table:enrichmentId: "company-info", outputoutputId: "zz5_nosuch"Enrichment "Company Info" has no output "zz5_nosuch"enrichmentId: "zz5_no_such_enrichment", output with nooutputIdblockId/pathEach 201 stores a column no run can ever write — the runner fills a cell from
result[out.outputId]and skips an output with none. The caller discovers it only when a later edit returns the 400 create should have given.Create now runs the same
requireEnrichment/requireKnownEnrichmentOutputIdshelpers and the samevalidateRequestedOutputscheck as update. No third validator.The discriminator is
workflowId, nottype— load-bearing.workflow-sidebar.tsxcreates enrichment-template groups labelledtype: 'enrichment'while backed by a realworkflowIdand carrying noenrichmentId. Keying on the label would have sent that straight intorequireEnrichment(undefined)and 400'd the first-party create path. Pinned by a regression test.A group's producer type could be relabelled into a state create forbids
refineGroupSourceenforces "enrichmentId required when type is enrichment" on create; the v2 update body has no such refine, declarestypeas a "replacement producer type", and declares noenrichmentId— soPATCH {groupId, type:"enrichment"}on a manual group reached the writer, which applies it unconditionally. Unrepairable: the strict body has no field that would make it valid.The harmful direction is the reverse flip. Relabelling an enrichment group as manual keeps
enrichmentIdwhile moving it onto the workflow branch withworkflowId === '', so every cell run fails. (The forward flip is survivable —workflowIdpersists and the runner falls through.) An update may now only restate the type the group already has; re-sending the stored value stays a no-op, so a caller echoing the whole group back is unaffected.Option (b) — accepting
enrichmentIdon update and validating the merged state — was rejected: it needs the writer to persistenrichmentIdand clearworkflowId, i.e. a producer-swap feature nobody asked for.The last two unbound cursors
GET /workflows/{id}/versionsandGET /workspaces/{workspaceId}/memberswere the only paged reads still minting bare tokens — literally{"version":2}and{"email":"..."}base64'd. Confirmed live: a versions cursor from workflow A is accepted by workflow B, and a hand-crafted{"version":99}is accepted verbatim. The keyset still applies within the authorized parent, so it is a silently-wrong start position, not a leak.Both now wrap their domain token with the same
{scope,inner}binding used everywhere else. No new mechanism.Guardrail closed too:
CURSOR_BINDINGSrecorded only query/body params, so[]could not distinguish "no filters" from "parent forgotten" — which is exactly what hid these two.CURSOR_BOUND_PATH_PARAMSnow declares the parent path param for all six nested paged lists, with tests that fail actionably if an entry is dropped. The cursor tests were also reworked to mint through the route, so a route that stops binding cannot pass.In-flight tokens for these two lists are invalidated. They are single-walk and unpersisted, and the refusal message is accurate.
Not fixed, flagged for follow-up
workflowId:PATCH {workflowId}on a registry-enrichment group makes it workflow-backed whileenrichmentIdstays set, so the runner still takes the enrichment branch and ignores the workflow. No first-party caller does this.type:'enrichment'+workflowId+ noenrichmentId), so a public caller cannot create a group the UI can.Verification
check:audits26/26,check:api-validation:strict,check:openapi— all cleantypefield