fix(v2): close the correctness gaps the release audit found - #6671
Conversation
An end-to-end audit of the v0.8.1 release surfaced one regression the release itself introduced and a set of filter/cursor gaps that let a caller's spelling change what a query answered. An `enrichment` workflow group stored no `workflowId`. The public contract invites a caller to omit it, the write persisted caller input through an `as WorkflowGroup` cast that hid the omission from the type checker, and the response schema still required it — so the write committed and then the outbound parse threw. Because the list presenter maps every group through that schema, one such row made GET, PATCH and DELETE on that table's groups fail from then on, with no public way to remove it. The cast is gone rather than papered over, so the same class of omission cannot recur silently. Knowledge tag filters accepted any operator string and then ignored an unrecognized one in opposite directions: the document list dropped the predicate and answered with the whole knowledge base, while search fell through to equality and answered a different question. Both now reject at the boundary. `.strict()` is applied on the v2 chain only — v1 has always stripped unrecognized keys, and the `between`-requires-`valueTo` rule already closes the mis-cased `valueTo` trap on both versions. Cursor scopes bound set-valued filters to the caller's ordering: `all`/`any` clause order and `in`/`nin` operand order in the table predicate, and the raw `resourceType` text on audit logs, whose query splits it into an `inArray`. Audit logs is canonicalized on both sides, because canonicalizing the scope alone would have given two genuinely different result sets one fingerprint. Also: the exposed-header list reached only the fallback CORS policy, so all five matched rules — including the wildcard-origin execute route, the only one that emits `X-Run-Id` — could not hand a browser the run id or a 429's `Retry-After`; a bulk row update reported an uncoercible value only when its filter happened to match; the cost and duration windows accepted an inverted pair and answered it with an empty page; and the sortless runs list advised callers to fix a `sortBy` it rejects.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryMedium Risk Overview Regression: Knowledge tag filters: v1/v2 schemas now reject unknown operators and Cursor / set-valued filters: Audit Logs list: Other: CORS applies default exposed headers ( Reviewed by Cursor Bugbot for commit 7be8c7b. Configure here. |
Greptile SummaryThe PR closes v2 correctness gaps around table-group persistence, filter validation, cursor binding, bulk-update validation, CORS response-header visibility, and range-validation errors.
Confidence Score: 5/5The PR appears safe to merge; no concrete changed-code defect remains after reviewing the corrected contracts, persistence paths, cursor semantics, bulk-update validation, and CORS behavior. The new validation and normalization paths align with their downstream query, persistence, and presentation contracts, and the cursor and CORS changes preserve the relevant scope and credential boundaries.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/table/application/groups.ts | Replaces an unsafe cast with explicit normalization of enrichment-group workflow and output fields. |
| apps/sim/lib/api/contracts/v1/knowledge/index.ts | Constrains tag operators and requires an upper bound for between while retaining v1 unknown-key leniency. |
| apps/sim/lib/api/contracts/v2/knowledge.ts | Preserves base tag-filter refinements and makes the v2 filter element strict. |
| apps/sim/lib/knowledge/tags/filter-resolution.ts | Validates operators and upper-bound values against resolved tag field types. |
| apps/sim/lib/api/contracts/v2/logs.ts | Adds cost bounds and rejects inverted cost and duration windows. |
| apps/sim/lib/api/cursor-binding.ts | Centralizes deterministic canonicalization for unordered array scopes. |
| apps/sim/lib/table/rows/cursor.ts | Canonicalizes logically unordered predicate positions without flattening or changing order-sensitive operands. |
| apps/sim/lib/audit-logs/query.ts | Uses the same resource-type set parser as cursor binding to keep query and cursor semantics aligned. |
| apps/sim/lib/table/rows/service.ts | Prevalidates bulk-update patch values so invalid requests do not depend on whether rows match. |
| apps/sim/proxy.ts | Applies the shared exposed-header list to matched CORS policies without changing their origin or credential rules. |
Reviews (1): Last reviewed commit: "fix(v2): close the correctness gaps the ..." | Re-trigger Greptile
An end-to-end audit of the v0.8.1 release (#6646) surfaced one regression the release itself introduces, plus a set of filter and cursor gaps where a caller's spelling changed what a query answered. This closes them.
The regression
POST /api/v2/tables/{tableId}/groupswithtype:"enrichment"wrote a row that then broke every later read of that table's groups.The request contract invites a caller to omit
workflowId("omit it for anenrichmentgroup"). The write persisted caller input through anas WorkflowGroupcast, which hid the omission from the type checker. The response schema still required the field — and the v2 builder validates outbound bodies — so the write committed and then the response threw an opaque 500 that never returned thegroupId. Because the list presenter maps every group through that schema, one such row madeGET,PATCHandDELETEon that table's groups fail permanently, with no public way to remove it.The cast is removed rather than papered over, so the same class of omission cannot recur silently. Verified: no table on staging currently carries a poisoned group, so this is fix-before-exposure.
Filters that silently answered a different question
Knowledge tag filters accepted any operator string (
z.string().default('eq')) and then ignored an unrecognized one in opposite directions — the document list dropped the predicate and returned the whole knowledge base, while search fell through to equality. Proven live on staging: same tag and value,operator:"eq"→ 1 item,operator:"nosuchop"→ 100 items.Both now reject at the boundary via a per-field-type operator enum, plus a
between-requires-valueTorule..strict()is applied on the v2 chain only — v1 has always stripped unrecognized keys and is a shipped public API, and thebetweenrule already closes the dangerous mis-casedvalueTocase on both versions. A test pins the v1 leniency as deliberate.Cursor scopes bound set-valued filters to the caller's ordering:
all/anyclause order andin/ninoperand order in the table predicate, and the rawresourceTypetext on audit logs, whose query splits it into aninArray. Audit logs is canonicalized on both sides — canonicalizing the scope alone would have given two genuinely different result sets one fingerprint, which is worse than the bug.Also
X-Run-Id— could not hand a browser the run id or a 429'sRetry-After. Rules now inherit the default and opt out explicitly.sortByits.strict()schema rejects.v2ValidationErrorimports.Deliberately not changed
The release's intentional compatibility changes are left alone —
folderPath404→empty 200,.strict()query params, the skills/billing policy changes, andstrictWrite. Also unchanged: 405s lacking anAllowheader (Next App Router default, needs middleware across the whole surface), and the four collections that emitnextCursorbut rejectlimit(needs a direction chosen).Two audit findings were retracted on closer reading rather than fixed: the upload PUT's parse-before-auth ordering (the contract declares no body, so
parseRequestnever touches the stream, and the token comes from the parsed headers — the ordering is forced), and audit-logsactionbinding raw (it iseq(...), a genuine exact match).Verification
Every fix was confirmed red-then-green — the source change reverted, the test watched to fail, then restored. Two cursor tests pass in both states by design; they are the over-canonicalization guards, and a guard that only passes after the fix would not be guarding anything.
check:api-validation,check:audits26/26,check:openapi— all clean