Skip to content

fix(v2): close the correctness gaps an end-to-end audit found - #6655

Open
waleedlatif1 wants to merge 55 commits into
stagingfrom
integrate/v2-w4
Open

fix(v2): close the correctness gaps an end-to-end audit found#6655
waleedlatif1 wants to merge 55 commits into
stagingfrom
integrate/v2-w4

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

Hardens the v2 surface after an end-to-end audit and roughly 2,000 live checks against a running server. Four waves of fixes, each verified at runtime rather than by unit test alone.

Caller-reachable 500s, now correct statuses

  • A file name of 126–255 characters made its own upload impossible. POST /files/uploads returned 201 and issued a transfer URL whose PUT then 500'd. Local staging artifacts were name-derived and stacked ~130 bytes of suffixes onto a key already budgeted to 255, crossing NAME_MAX. Staging artifacts are no longer derived from the caller's name at all, so no future suffix can reintroduce the arithmetic; the durable sidecar keeps a central reservation. Five more key builders had the same shape — including an inbound email attachment name that was neither sanitized nor bounded.
  • NUL bytes reached the driver from JSON bodies, query params, canonical folder paths (after percent-decoding), and multipart fields — including the multipart filename and field name, which no JSON boundary scan can see.
  • minDurationMs=1.5 and any value outside int4 reached Postgres unguarded; the contract published number against an integer column.
  • Year 0000 timestamps satisfied the published \d{4} pattern and 500'd in both logs and billing.
  • Unparseable date predicates, unknown afterRowId, zero-byte uploads, and MCP inputSchema.description all 500'd.

Silent wrong answers

  • Application-written timestamps were local time labeled Z. All 283 timestamp(...) columns are without time zone, so three writers and two readers disagreed about what instant a stored wall clock meant. A saved view's createdAt was seven hours off while passing schema validation, corrupting any sort or predicate over those fields. The session is now pinned to UTC with a UTC-anchored parser for oid 1114.
  • Cursors were bound to the sort but not the filters on 12 of 18 paged lists, and a keyset cursor was unbound on both encode and decode — replaying one under a wider filter silently omitted every earlier match. A cursor whose inner token was "" slipped the envelope check and restarted at page 1 with a nextCursor, the exact failure the billing ledger documents as unacceptable.
  • Uncoercible cell writes stored null and returned 200. Now rejected, with 'null' retained at three machine-write sites so one bad row in a CSV import cannot reject the other 99,999.
  • A saved view's config.sort was always discarded — the pruner compared column ids against a contract that specifies names.
  • A concurrent same-name create returned 500 for the losers of the race; the pre-check was a SELECT with no unique-violation handler.
  • HEAD skipped authorization entirely, answering 200 for a denied principal, a nonexistent resource, and an unauthorized workspace — and fabricated audit rows for exports and downloads that never happened.

Contract and documentation

69 contracts declared no query schema at all and silently accepted any parameter; 8 more were non-strict. Blank query values are now rejected surface-wide rather than coercing to 0. The spec now documents what the service does: HEAD semantics on the three head-unsafe routes, the predicate grammar with its * wildcard, the canonical folder-path format, the recursive vocabulary, retention windows, and a built-in skill's real id form. Descriptions lost their extraneous half — p99 down from 733 to 370 characters, the >400 band down 63% — with load-bearing rules relocated to one shared place rather than deleted.

Verification

Roughly 2,000 live checks across all 135 operations and seven resource families: every filter asserted to narrow results rather than merely return 200, ~40 pagination walks to exhaustion with zero duplicates or skips, every 2xx body validated against its published schema with additionalProperties enforced, and no secret material in any credential response across 52 filter permutations.

Full suite: 24,351 tests. type-check, biome, and all 25 audits green with DATABASE_URL unset.

For review

The UTC timestamp fix touches packages/db and every pool including the realtime client. It rests on production already running UTC on both primary and replica — a no-op there, and it makes every other environment match. That premise is worth confirming before merge.

Two defects on the live v1 surface were found and deliberately not fixed, since v1 is stable and unflagged: POST /api/v1/workflows/{id}/rollback silently strips a typo'd version and rolls back to the wrong version, and GET /api/v1/logs?startDate=abc returns a 500.

`v2McpToolInputSchema` declared `description: z.string().optional()` inside a
`.catchall(z.unknown())` object, and a declared key beats the catchall. The MCP
SDK's own `ToolSchema.inputSchema` does not declare `description` at all, so any
value — including the JSON `null` a Python server emits for an absent one —
passes its validation and reaches Sim unchecked. The builder's outbound `.parse()`
then threw, and the discovery error policy correctly declines to classify a
Sim-side schema defect, so the endpoint that completes MCP onboarding answered a
bare 500. The key is dropped and left to the catchall; `type`, `properties`, and
`required` stay pinned because the SDK enforces those at least as tightly.

Also in the v2 resources family:

- The single-resource query schemas for MCP servers, skills, custom tools, and
  secrets are now `.strict()`, matching every list in the same family. A mistyped
  flag was silently ignored behind a 200.
- `openapi/resources.ts` re-derived `RESOURCE_ERRORS` and
  `RESOURCE_CONFLICT_ERRORS` inline in 21 of 22 operations. They now import the
  shared constants; the generated spec is unchanged, which is the point.
- The internal MCP refresh route stamped `updatedAt` alongside `lastToolsRefresh`.
  `updatedAt` means "configuration last changed" and is a public keyset sort, so
  a refresh moved rows out from under an in-flight page. `updateServerStatus`
  already held that invariant; the route now matches it.
- The discovery cooldown is a typed `McpServerCooldownError` rather than a
  substring search for `cooldown`. `McpConnectionError` interpolates the server's
  display name into its message, so a server named after the word was reported as
  a transient cooldown when its connection had genuinely failed.
Deploy and rollback bodies were plain objects, so a misspelled key was
stripped rather than rejected. On rollback that is silent misbehavior:
an omitted `version` legitimately means "reactivate the preceding
version", so `{"versoin": 5}` rolled back somewhere else and answered
200. Both v2 bodies, the run-read query, and the versions cursor are now
strict.

Deployment versions are an `integer` column, but the path param, the
versions cursor, and the v1 body each bounded it differently or not at
all — an out-of-range value overflowed the comparison into an
unclassifiable 500. One exported bound now covers all three.

Resume admission raised bare `Error`s for a stale contextId or an
already-resumed run, which the resume surfaces could not classify and
reported as 500. They now use the sibling `ResumeAdmissionError` already
in that file, carrying 404/409/400 and whether an automatic retry can
clear the refusal.

Docs corrections: rollback publishes the 409 its webhook-path conflict
already produces; deploy/undeploy/rollback reject a workspace key with
403, not the concealed 404 they documented; the workflows OpenAPI module
imports the shared error sets instead of re-deriving them; import and
the folder ops explain their folder-tree 413. The export route is marked
`headSafe: false` so a HEAD probe stops filing a WORKFLOW_EXPORTED audit
event for an export that never happened. `runId` is one bounded schema
across the run and log resources.
…bounds

Security: the four knowledge document-upload routes rendered a bare upload
error policy with no resource concealment, while every sibling knowledge route
uses one. Because the use case resolves the knowledge-base context before
workspace authorization, the unconcealed 403 told any valid API-key holder that
a knowledge base exists in a workspace it cannot reach — the exact signal
GET /api/v2/knowledge/{id} withholds by answering 404 either way. All four now
use the composed concealing policy, which also renders the 415/402/413 the
route-local renderer already handled; that duplicate renderer is deleted.

Contracts:
- POST /knowledge/search is strict. It was the only non-strict v2 request body,
  so a mis-cased rerankerEnabled or topK returned 200 with the key stripped,
  changing what the caller was billed and silently disabling reranking.
- The document list takes limit, cursor, and search from the shared v2 schemas.
  search was an unbounded, empty-accepting v1 string, so ?search= answered 200
  with a full page here and 400 on GET /knowledge, and the term reached an
  unindexed filename LIKE scan with no ceiling.
- The 16 non-strict single-field workspace query slices across both families are
  strict, matching GET /knowledge/{id}/tags.
- GET /audit-logs takes workspaceIdSchema instead of a bare string (?workspaceId=
  was forwarded as a filter and returned zero rows) and the shared run-window
  bounds for startDate/endDate.

Documentation:
- listAuditLogs drops the 404 it has no code path to emit.
- upsertFileShare describes its workspace-key refusal as the 403 it renders;
  the operation denies the key by principal kind, which the concealment policy
  does not rewrite.
- The 12 body-reading knowledge and files operations publish the 413 their
  pre-validation body read raises, and the file list publishes the folder-tree
  413 its now-capped path index raises.

Correctness: queryWorkspaceFilePage loads its folder path index under
MAX_FOLDERS_PER_WORKSPACE like the workflow, table, and knowledge lists. An
uncapped index does not fail on truncation, so a real folder outside the read
rows resolved to undefined and answered "Folder not found".
`parseRequest` buffers a JSON body through `parseJsonBody` under
`DEFAULT_MAX_JSON_BODY_BYTES` before any schema runs, and the v2 builders supply
`V2_PARSE_DEFAULTS.payloadTooLargeResponse`, so every operation whose contract
declares a body already answers 413 above the cap. The resources family
published it on none of them. A status a caller cannot see in the spec is a
status they will not handle.

Adds `RESOURCE_BODY_ERRORS` and `RESOURCE_CONFLICT_BODY_ERRORS` to the shared
sets and applies them to the seven affected operations: createMcpServer,
updateMcpServer, createSkill, updateSkill, createCustomTool, updateCustomTool,
and setSecret. All seven are `defineV2JsonRoute` handlers on non-GET methods
with no `parseOptions` override, so the 413 is genuinely reachable on each. The
new sets are opt-in rather than folded into the base sets precisely because
reachability is not automatic — an operation with no body, or one whose payload
reaches it through an uncapped path, would be publishing a response that can
never arrive.

A sweep test pins the invariant across the resources, billing, and logs
documents. It is one-directional by construction: several bodyless operations
publish 413 for their own folder-tree and render ceilings, so the converse would
flag correct documentation.

Also completes the shared-constant consolidation started in cd3efef:
`openapi/billing.ts` and `openapi/logs.ts` each re-derived `RESOURCE_ERRORS`
inline in two operations. Both now import it, and both regenerate byte-identical.
… docs

Adds `headSafe` to `defineV2BinaryRoute`, mirroring the JSON builder: a HEAD
on a route that declares itself unsafe is authenticated and rate-limited, then
answered bodiless before parsing or executing. `GET /api/v2/files/{fileId}` is
the one binary v2 route and it records a `FILE_DOWNLOADED` audit event, so a
HEAD probe used to fabricate a download that never happened.

Names the cause of five refusals that reached the wire as codeless 403s
(billing principal-kind, personal-keys-disabled and role, secret admin and
write, the workspace table quota, and public sharing), adding three members to
the closed `FORBIDDEN_DETAIL_CODES` set. The billing cross-tenant refusal is
concealed as a 404 instead of coded, and the credential-list and knowledge
file-ownership refusals stay codeless deliberately, documented at the site.

Makes `WORKSPACE_KEY_OPERATION_NOT_PERMITTED` reachable: an operation that
denies workspace keys also omits them from `principalKinds`, so the kind guard
always fired first and callers got `PRINCIPAL_KIND_NOT_PERMITTED` instead of
the published code.

Drops the unused 410 response, shares one `order` schema between the two run
reads so both specs spell the enum the same way, and corrects the false
statements about 403 codes, 413 causes, cursor schemes, and full-set lists in
the conventions skill and the contract TSDoc.
- updateColumnOptions was the only column mutator with no lock assert: an
  options-only PATCH applied on a schema-locked table, and an option REMOVAL
  cleared cells on a delete-locked one. Assert schema always, escalate to the
  destructive gate only when options are dropped.
- GET/DELETE /tables/imports/{id} 500'd on a first-party import job (null
  payload) or an unrepresentable status. Both now read as absent, so the answer
  is the 404 it always was.
- Offset cursors stamped the sort but not the filters, so a page-2 cursor
  replayed under a different predicate paged an unrelated sequence silently.
  Offsets now carry a filter fingerprint and refuse a mismatch.
- Publish 413 on every tables operation that accepts a request body: the v2
  JSON builder reads the body under a byte ceiling before validation, so the
  status is reachable on all of them. Derived at document assembly so a new
  route cannot regress it.
- Enforce MAX_VIEWS_PER_TABLE on view create, making the list contract's
  "small bounded set" claim true.
- Accept the upload control token on the import read, so an upload-backed
  import is readable during the phase its own 201 reported; drop the `queued`
  status the reads can never return.
- Declare the Find search-term cap, the Find match cap, and the run row-id
  ceiling the domain already enforces.
- Uniform 201 on the row and column creates.
…-hardening

# Conflicts:
#	apps/sim/lib/api/contracts/v2/shared.ts
An enumeration of side-effecting v2 GETs flagged these two for issuing a
workflow_blocks update. The write is convergent and would be issued by the
next ordinary read, and headSafe: false answers 200 unconditionally, so
declaring it would cost HEAD its existence check to prevent nothing.
Four families of caller-reachable 500s share one shape: a value the
contract admits, the application forwards, and the database rejects.
An unclassified driver throw renders as INTERNAL_ERROR, so a bad
request came back as a server fault — on pure reads as well as writes.

NUL bytes are rejected at the contract boundary, in parseRequest, not
per field. A shared string primitive only protects the fields somebody
remembers to build on it, and it cannot protect the values that have no
string schema at all: a table cell and a predicate value are z.unknown()
because their type belongs to the column, not the wire, and those are
exactly the values found reaching the driver. One scan over the already
validated params/query/body covers every field including the ones nobody
has enumerated. Only U+0000 is rejected; every other control character
is ordinary content that Postgres stores verbatim.

Date bounds on a filter are now parsed, not merely type-checked, with
the same normalizer the date column type uses to store cells — so the
filter grammar and the storage grammar agree, and gt/gte/lt/lte on both
JSONB date columns and the createdAt/updatedAt system columns answer an
unparseable bound with 400 instead of an invalid-input-syntax 500.

An afterRowId/beforeRowId anchor that does not exist is a classified
not-found rather than a bare Error, and a zero-byte knowledge document
is refused at admission: every parser rejects an empty buffer outright,
so the upload could only ever consume storage and quota on its way to
processingStatus failed.
Six defects that share a shape: a 200 that misrepresents what happened,
which is the one class a caller cannot detect from the response.

Knowledge search silently degraded. Reranking is implemented and does
run, but a deployment with no Cohere credential, a provider error, or a
timeout was swallowed into a warning log and answered 200 with plain
vector ordering and no `rerankerScore` anywhere — indistinguishable from
a reranker that ran and agreed with the vector order. The fallback stays
(an outage should not take search down) and is now reported:
`rerankerStatus` is required on every search response. v2 also omitted
the `rerankerModel` default the internal contract supplies, so
`rerankerEnabled: true` alone failed the use case's model guard and
returned unreranked results after paying for the widened candidate
retrieval; it now defaults like its sibling.

`GET /billing/logs` accepted `startDate`/`endDate` with any relative
period and dropped them, answering over the default 30-day window — a
caller reconciling charges got real rows that were not the rows it asked
for. Both bounds are now rejected outside `period=custom`, take the same
strict UTC form as `GET /logs` via the shared `v2RunWindowBoundSchema`,
and reject an inverted window instead of returning an empty page.

MCP registration stamped `connectionStatus: 'connected'` and
`lastConnected: now` at insert without contacting the endpoint, and did
the same on any non-OAuth re-registration while leaving `lastError`
stale. `tool-validation` gates tool availability on that column, so an
unreachable server read as healthy. Both paths now leave the columns at
their honest defaults for `mcpService.updateServerStatus` to move after
a real discovery; the client-side optimistic copy matches.

`skills.create` allowed a workspace API key while every other skill
write denies one, so a key could only ever accumulate skills it could
never remove — and the row it left was attributed to the workspace's
billing owner, minting an editor grant for a human who did not act.
Creation now denies a workspace key, making the lifecycle symmetric on
the per-skill editor model that authorizes the rest of it.

`runCount` counts successful non-paused runs and is never decremented by
retention, so it disagrees with the runs list in both directions; the
description now says so rather than claiming "total recorded runs". Run
retention itself was undocumented — free-plan runs are hard-deleted after
30 days, which is why a workflow reports runs beside an empty list — and
is now stated on both reads over the execution-log table.
- Uncoercible cell values were stored as null under a 200 on any optional
  column: "abc"/true/[1] into number, "yes"/1/{} into boolean, "not-a-date"
  into date, an undeclared option into select, an object into string. The
  read side already 400s on the same mismatch in a predicate, so the two
  halves of the API disagreed about the same value. `coerceRowValues` /
  `coerceRowToSchema` now take an explicit policy and default to `reject`;
  `null` is passed only where a machine produced the value for a cell no
  caller typed — a computed (workflow/enrichment) write and a CSV import,
  neither of which has anyone to answer with a 400.
- A multi-select coerced `["green"]` to `[]` — the drop was inside the
  registry, so no policy above it could see it. It now refuses any part that
  matches no option, which is what the single branch and the bulk retype gate
  already did.
- A bare number in a date cell was read as epoch milliseconds, so the far more
  common Unix-seconds shape stored a timestamp 50 years early. The unit is not
  recoverable from the value and both readings are in range, so a bare number
  is refused in both directions and the retype gate no longer needs an
  override to be stricter than the write path.
- Unknown column names were dropped by the name→id remap: an insert of
  {"nosuchcol":"x"} created an empty row under a 201, and a patch of
  {"zzz":"x"} answered updatedCount:0, indistinguishable from an empty match.
  The v2 row boundary now names them and refuses.
- The table ceiling was enforced only inside createTable, which for an
  upload-backed import does not run until the CSV has crossed the wire: a full
  workspace got a 201 and a presigned PUT for up to 5 GiB, then a 403 at
  complete with an orphaned object left behind. The advisory check now runs
  when the session is created; the authoritative one stays in the transaction
  because the quota can move mid-upload.
- Cap workflow groups per table. GET /tables/{id}/groups is published as a
  full-set list, and the group count had no bound of its own — the indirect
  one does not survive an update path that adds no columns.
- Present a group's outputs/dependencies/inputMappings by column NAME. They
  are created by name, stored by id, and were read back as ids on a surface
  that is otherwise name-keyed, so a group could not be round-tripped.
- Publish the predicate grammar: the operator set, the per-type restrictions,
  and that `*` — not `%` — is the wildcard. It was true only in the SQL
  builder's own comments, so the natural guess matched zero rows under a 200.
- Stop advertising a `workflowId` default of "" on group create; a manual
  group that omits it has always been refused.
…sort

A v2 cursor names a position in one sequence, and a list decides that
sequence from its sort AND its filters. Only the sort was stamped on the
shared keyset codec, so a cursor from an unfiltered walk was accepted
under a changed `search`, `scope`, `deployedOnly`, or folder and answered
from a sequence the caller never asked for. The two offset lists already
stamped both; nothing else did.

The failure differs by scheme but is silent in both. An offset lands at
an unrelated ordinal. A keyset stays internally coherent — correctly
ordered, duplicate-free — and drops every match sorting before its
position, which a caller holding an opaque token reads as "almost
nothing matched".

One mechanism, shared with the table-row codec: canonical JSON plus a
SHA-256 fingerprint (`lib/api/cursor-binding.ts`), stamped by
`cursorFilterScope` alongside `cursorSortKey`. The two stamps stay
separate so the 400 names which half changed. `limit` is never bound —
it selects how much of the sequence to return, not what it is.

The three lists whose token is minted by a domain codec (`/logs`,
`/audit-logs`, `/billing/logs`) get the same binding by wrapping that
token in a query-stamped envelope; the domain cursor is untouched.

`present` now also receives the parsed request, so a presenter reads the
filters it stamps straight from the query instead of the use case
carrying an HTTP cursor concern back out — the `cursorSort`/`cursorScope`
round-trips through three application services are removed.

`list-pagination.test.ts` now declares each paged list's binding and
checks it against the contract in both directions, so a new list, or a
new filter on an existing one, fails until its binding is decided.
Two ways the v2 surface answered a request it had not checked.

`headSafe: false` exists so a HEAD cannot fire the side effect its GET
performs — an outbound MCP discovery, a FILE_DOWNLOADED audit event, a
WORKFLOW_EXPORTED audit event. The short-circuit sat between admission
and parsing, so it returned a bodiless 200 before resource authorization
ran at all: authorization lives inside the use case, and the use case was
exactly what the short-circuit skipped. Any valid API key drew 200 for a
denied principal kind, a nonexistent id, another tenant's workspace, and
a request missing a required param, while the GET beside it answered 403
or 404. That is an existence oracle over MCP server ids, file ids, and
workflow ids.

`OperationUseCase` gains an optional `authorize()` that runs the phase
before the business transaction — allowed-principal check, canonical
load, asserted-scope comparison, current access check — and stops.
`defineAuthorizedWorkspaceUseCase` shares one implementation between it
and `execute`, so the two cannot answer differently. A HEAD on a
not-head-safe route is now admitted, parsed, and authorized like the GET,
rendering refusals through the route's own error policy, then answered
bodiless. The builders refuse at definition time to pair
`headSafe: false` with a use case that has no `authorize`, so the next
such route is a boot failure rather than a silent 200.

Separately, `parseRequest` validates the query slice only when the
contract declares one, so an omitted `query` means "never look at the
query string" rather than "takes no query params". 69 v2 contracts
omitted it and accepted anything: `?bogus=1` was a 200 on
`GET /workflows/{id}` and a 400 on every list. They now declare
`noInputSchema`, and 8 more contracts that declared a query without
`.strict()` are tightened. A sweep over the contracts tree is the
enforcement — a compile-time gate on `defineRouteContract` was tried and
reverted because the required intersection collapses inference of the
sibling generics.

Four route tests appended `?workspaceId=` to a PATCH/PUT that reads it
from the body; that copy was being silently dropped and is now a 400.

The generated specs are byte-identical: the OpenAPI generator learns that
a slice declaring no keys publishes no parameters.
…t empty

cleanCellValue runs the same registry coercion the server does, so tightening
multiselect on the server changed this helper too. The case asserting an empty
array was pinning the silent-drop the tightening removed.
The description was already published on every spec but did not appear in the
rendered Authorization block. It carried a raw > and backticks, which the
markdown pass in the docs renderer does not survive; the operation description
on the same page renders fine. Reworded to plain prose with the same substance.
Two agents each fixed half of this: the shared list codecs gained filter
binding, and the table codec gained a fingerprint, but the pure-keyset shape
stamped it on neither encode nor decode. A keyset position is absolute in
(order_key, id), which is why it was left unbound — but absolute ordering is
not completeness. Replaying the cursor under a wider filter silently omits
every match sorting before it, so paging predicate A then B returned rows 7,9
where the full B sequence is 1,3,5,7,9.

Also answers a lost create race with the conflict it already documents, and
shortens three descriptions that dwarfed their siblings — the forbidden-code
catalogue now lives on the error envelope's details field, published once per
document instead of on all 135 operations.
A view config stores every column reference as a stable column id, but two
things wrote it in different vocabularies and nothing translated between them.

`config.sort` was pruned on read against the live column ID set while the
contract defines `sort[].field` as a column NAME, so every name-keyed sort —
the only kind the v2 surface can express — pruned to nothing and the view came
back with `sort: null`, on both create and PATCH, with no warning. The same
prune dropped a sort on `createdAt`/`updatedAt`/`id`, which are sortable row
columns that simply are not in `schema.columns`. `config.filter` had the
opposite failure: it was stored verbatim, so a predicate naming a column that
does not exist saved happily and then 400'd on every `/query`, `/query/count`,
and `/rows/find` that tried to use it.

The write path now canonicalizes a config before storing it: every column
reference (layout keys, `sort[].field`, each `filter` leaf `field`) is resolved
to the column's stable id, and `filter`/`sort` are validated against the live
schema so a reference that can never resolve is refused instead of saved. The
v2 read presents the config back keyed by column name, matching
`presentV2WorkflowGroup` and every other v2 row/data surface — a caller never
sees a `col_…` id, and what it wrote is what it reads. Resolution is a lookup
with pass-through, so the id-keyed first-party UI is unaffected.

Column LAYOUT stays unvalidated on write and pruned on read: it auto-saves as
the user drags, so racing a column delete must self-heal, not fail the drag.
The read path still never prunes a predicate, for the reason already documented
there — a pruned condition silently widens the view's row set.
…derived keys

Four caller-reachable 500s shared one shape: input passed boundary
validation, then failed in the storage/key layer. Each is fixed at the
boundary that owns the transformation, not at the call sites.

Percent-encoded NUL in a canonical folder path. `parseRequest`'s NUL scan
sees `%00` as three ordinary characters; the NUL only exists after
`parseFolderPath` decodes it. Reads survived as 404s, writers carried the
decoded name into an INSERT and the driver threw. The rejection now lives
in `encodeFolderPathSegment`, the single chokepoint both building and
parsing funnel through, so it covers every escape a caller can spell.

NUL in a multipart field. A multipart route declares no body contract, so
its fields never reach contract validation at all — the knowledge-document
key was sanitized while `original_name` was not, and the object landed in
storage before the insert threw. `readFormDataWithLimit` is the shared
multipart reader every such route already funnels through, so the scan
goes there and runs before a caller holds a File to upload, which removes
the orphan rather than cleaning it up.

Storage-key overflow at 225 characters. Every generator embedded the file
name in a path component it also prefixed with a timestamp and a
uniquifier, so the effective limit was 255 minus that prefix while the
contract advertised 255 — a 225-character name produced a 256-byte
component and ENAMETOOLONG from local storage, and the upload session
handed out a transfer URL that could never succeed.
`buildStorageKeySegment` reserves the prefix out of the component's budget,
making the key independent of name length and the declared limit honest.

The NUL predicate is now shared from `@sim/utils/string` by all three
boundaries instead of being restated at each.
Three descriptions asserted behavior the code no longer has, and three rules
the code enforces were published as unconstrained strings.

`downloadFile` and `listMcpServerTools` still told callers a `HEAD` on a
not-head-safe route "is answered with an empty 200 ... reports only that the
endpoint exists and the caller is authorized". That was true of the old
short-circuit, which sat between admission and parsing and therefore returned
200 for an id the same caller's `GET` refused. The builders now authorize a
HEAD exactly as the GET, so the spec said the opposite of a security fix. One
`HEAD_MIRRORS_GET` constant replaces both sentences and is added to
`exportWorkflow`, whose `headSafe: false` was never documented at all. A test
walks the `app/api/v2` tree for the declaration and fails on any operation that
carries it without the sentence, or that resurrects the old claim.

`createMcpServer` promised that re-registering an existing URL "rewrites the
configuration and returns the server to the same unverified state"; it is a
409 pointing at PATCH. `authType` claimed Sim "detects it from the server when
omitted" — registration deliberately never contacts the server, and the column
defaults to `headers`. The default stays: `headers` and `none` are
behaviourally identical (only `oauth` branches), so changing it is a migration
with no caller-visible payoff, while the sentence was simply false.

`predicate` was the API's most consequential gap: a `pipe` over `z.unknown()`
documents from its input, so the leaf keys `field`/`op`/`value` appeared
nowhere in the contract and `{column, operator, value}` was a 400 a caller
could not correct against. Both predicate schemas now publish a real recursive
JSON Schema through `.meta()`, self-referencing so the recursion resolves from
one `$defs` entry, with every bound read from the constant that enforces it.

Also published: the canonical folder-path rule and its 4096-byte cap on the
four path components (the `superRefine` contributed nothing to JSON Schema);
the closed 12-value `recursive` vocabulary on a destructive delete; and the
null-matching behaviour of the negating operators. The clamping `limit` branch
drops `minimum`/`maximum`, which in JSON Schema mean "rejected outside" and
made SDKs refuse locally what the server clamps.

`deleteFile` stops publishing a 409 nothing in its path can raise. `restoreFile`
and `abortFileUpload` keep theirs — the report called them unemittable, but
restore raises `FileConflictError` after exhausting its rename retries and
abort refuses a completed session.

Description tail, across the seven specs: p99 733 to 465, max operation 1643 to
1114, over 700 chars 31 to 13, over 400 70 to 61. Constraints moved from
operation prose onto the fields they constrain rather than being deleted.
…er filters answer correctly

Four defects on the v2 surface, each reproduced before it was fixed.

Upload completion dispatched document indexing from inside the completion
transaction, so a queue or processing failure returned 500 after the object was
stored, the document row was created, and the session was marked completed —
and the only recovery, replaying the request, answered 200. The dispatch is now
a follow-on step that runs after the session is durably completed and is logged
rather than raised. Its outcome stays visible on the document itself (`failed`
with an error, or `pending` when it was never picked up), and the recovery path
re-queues a `pending` registration instead of keying off a message left on the
session.

A query parameter sent with no value was read as `0`, `false`, or the parameter
default: `?limit=` became `LIMIT 1` on the three lists that clamp, and
`?minCost=` on `/logs` became a live `cost >= 0` filter. `search` and `cursor`
already rejected a blank and documented "omit the parameter instead"; that rule
now applies to every v2 parameter, enforced on the raw query before coercion so
a parameter added later inherits it.

The document list matched `_` and `%` in `search` as live LIKE wildcards while
every sibling list escaped them through `searchFilter`, so the documented
substring match returned everything for `a_itest`. It now uses the same helper.

A `folderPath`/`folderPaths` naming no folder answered 404 on `/logs`, `/files`,
`/workflows`, `/tables`, and `/knowledge`, while every other filter answers an
empty page and the sibling folder lists already do. All five now return an empty
page. Mutations keep their 404.
…tions

The v2 spec's description median was already healthy at 42 characters; the
tail was not. 174 descriptions ran past 200 characters and 13 past 700,
almost all of it rationale, cross-references, and constraints restated on
the wrong object.

Trim the shared error, folder-path, retention, pagination, and workspace-key
constants first, since each is published on between two and twenty-seven
operations. `FOLDER_TREE_TOO_LARGE` dropped the clause explaining why the
tree has to load, `FULL_SET_LIST` dropped a second sentence restating its
first, `RUN_RETENTION` dropped the `runCount` caveat that already lives on
`runCount`, and the 503 and 499 descriptions dropped the paragraphs
narrating why they are documented at all. That reasoning belongs in the
TSDoc beside each constant, which is where it now is.

Then the operations. Execute Workflow and List Runs each restated a rule
their own parameters already carry — the `X-Run-Id` uniqueness claim and the
`order` sort deviation — so both moved to the parameter that owns them. The
run-status enum sent a caller to `paused.automaticResumeWaitingReason` and
then explained that field in place of describing it; the explanation moved
onto the field, which previously said only that it was "the reason automatic
resume is waiting".

Align the parameter vocabulary a caller meets in every family. One `cursor`
description had forked on the table row query, one `sortBy` on knowledge
documents, and the table row `limit` published neither its bounds nor its
default. `nameSortCollation` is now a function of the column it names, so
the knowledge document list can state the caveat about `filename` without
claiming a `name` field it does not have. `scripts/openapi/documents.test.ts`
pins `cursor` and `sortOrder` to one string each, and the retention window to
both reads that publish it.

Distribution over the seven documents: mean 71 to 67, p95 223 to 199, p99 453
to 370. Over 200 characters 174 to 147, over 300 94 to 59, over 400 54 to 20,
over 700 13 to 9. The median is unchanged at 42.
# Conflicts:
#	apps/docs/openapi-v2-logs.json
#	apps/sim/lib/api/contracts/v2/logs.ts
…te/v2-w4

# Conflicts:
#	apps/docs/openapi-v2-resources.json
#	apps/docs/openapi-v2-workflows.json
#	apps/sim/app/api/v2/lib/response.ts
Two branches each added the constant, in cursor-binding and list-query. It
belongs beside its sibling REFILTERED_CURSOR_MESSAGE, so the list-query copy
and its importers move there.
@waleedlatif1
waleedlatif1 requested a review from a team as a code owner August 13, 2026 03:17
@vercel

vercel Bot commented Aug 13, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
docs Ready Ready Preview Aug 13, 2026 4:57am

Request Review

@cursor

cursor Bot commented Aug 13, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Changes are documentation and generated OpenAPI only; they do not alter runtime handlers. Risk is limited to spec/doc drift if codegen was not run from the same commit as the implementation.

Overview
Aligns agent-facing v2 convention docs (skill + mirrored Claude/Cursor commands) with behavior hardened in the audit: 403 details.code is documented as optional on the wire until the forbidden-code sweep finishes; 413 covers fixed ceilings on request bodies and on collections that must be fully materialized (folder trees, rendered artifacts); side-effecting GETs must set headSafe: false so HEAD probes do not fabricate audits or outbound calls.

Pagination rules are expanded to three cursor schemes (keyset, offset, per-domain with scoped wrapping), cursorFilterScope / cursorSortKey binding (filters hashed via cursor-binding.ts), explicit limit is never stamped, and list-pagination.test.ts as the declaration gate. Rule 5 now says present reads sortBy / filters from the parsed request, not from the use-case return value.

Regenerates openapi-v2-billing.json and openapi-v2-files-audit.json so the published spec matches those rules: billing log period / startDate / endDate as strict UTC date-time with clearer cursor replay text; files/audit lists use shared FolderPathInput schemas, document 413 on folder-tree caps, scope=archived + folderPath as empty pages, download HEAD (auth/errors like GET, no audit or Content-Length), and tighter operation descriptions. Shared components consolidate 403 detail codes into V2Error.details, shorten cross-operation boilerplate, drop Gone, and document blank query values as 400.

Reviewed by Cursor Bugbot for commit 165db83. Configure here.

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile-apps

@greptile-apps

greptile-apps Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR broadly hardens v2 validation, pagination, authorization-safe HEAD handling, uploads, table writes, and database timestamp behavior. The follow-up cursor fix correctly normalizes member order but remains incomplete for duplicate members.

  • Adds shared cursor sort/filter bindings across paged v2 lists.
  • Strengthens request validation and error projection for malformed boundary inputs.
  • Standardizes UTC handling across shared database pools and timestamp parsing.
  • Hardens upload naming, multipart validation, and route authorization behavior.

Confidence Score: 4/5

The PR is not yet safe to merge because equivalent duplicate-bearing log filters can still invalidate a valid pagination cursor.

The author’s reply says the raw-filter cursor overbinding was fixed, but the new canonicalizer only sorts members; because it preserves duplicates while the SQL membership predicates do not, A,B and A,A,B still select the same rows yet produce different cursor fingerprints.

Files Needing Attention: apps/sim/lib/api/cursor-binding.ts, apps/sim/app/api/v2/logs/route.ts

Important Files Changed

Filename Overview
apps/sim/lib/api/cursor-binding.ts Introduces shared cursor-scope canonicalization, but unordered list normalization preserves duplicate members and therefore still fingerprints some semantically equivalent filters differently.
apps/sim/app/api/v2/logs/route.ts Binds domain cursors to normalized log filters, while inheriting the duplicate-member mismatch from unorderedScopePart.
apps/sim/lib/api/cursor-binding.test.ts Thoroughly covers cursor binding, ordering, malformed tokens, and empty members, but omits equivalent sets containing duplicates.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[First logs request<br/>workflowIds=A,B] --> B[unorderedScopePart<br/>A,B]
  B --> C[Cursor stores scope fingerprint]
  D[Resume request<br/>workflowIds=A,A,B] --> E[unorderedScopePart<br/>A,A,B]
  E --> F{Fingerprint matches?}
  F -->|No| G[400 filter mismatch]
  D --> H[SQL IN A,A,B]
  A --> I[SQL IN A,B]
  H --> J[Same matching rows]
  I --> J
Loading

Reviews (2): Last reviewed commit: "fix(v2): bind a cursor to what a set fil..." | Re-trigger Greptile

Comment thread apps/sim/app/api/v2/logs/route.ts
…lled

workflowIds, triggers and folderPaths are comma lists the query treats as
unordered sets, and tagFilters is an object whose key order carries no meaning.
Fingerprinting the raw spelling bound the cursor to the spelling, so a caller
who reordered an equivalent filter mid-walk got a 400 for a page that was
genuinely the next one.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile-apps

Comment thread apps/sim/lib/api/cursor-binding.ts Outdated
Comment on lines +76 to +80
const members = raw
.split(',')
.map((member) => member.trim())
.filter((member) => member.length > 0)
.sort()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Duplicate members still overbind cursors

When a client resumes pagination with an equivalent filter such as workflowIds=A,A,B instead of workflowIds=A,B, unorderedScopePart preserves the duplicate and produces a different fingerprint, causing a 400 even though the SQL membership predicate selects the same rows.

Suggested change
const members = raw
.split(',')
.map((member) => member.trim())
.filter((member) => member.length > 0)
.sort()
const members = [...new Set(
raw
.split(',')
.map((member) => member.trim())
.filter((member) => member.length > 0)
)].sort()

…ict query

Three follow-ups on the w5 policy work: one decision recorded, two tests that
could not fail.

The `query: noInputSchema` sweep is kept. It is a real tightening — 69 v2
operations that ignored an unknown query param now answer 400 — so it was
weighed rather than assumed. The v2 body slice on those same endpoints was
already `.strict()`, and every v2 list already rejected `?bogus=1`, so the
split was arbitrary rather than a promise: the same typo was a 400 on
`GET /workflows` and a silent 200 on `GET /workflows/{id}`. A parameter the
server drops without saying so is the bug class the lists' rule already exists
to prevent. No first-party caller is affected — the two SDKs send only
`includeOutput`/`selectedOutputs`, both declared; the UI and the desktop app
make no v2 calls at all; `requestJson` appends nothing implicitly and no v2
cache buster exists; every docs example uses a declared param. A third-party
caller appending a tracking tag does break, which is why the behavior is now
documented in the API reference with the exact 400 body rather than left to be
discovered, and why the reasoning sits in the v2 conventions skill next to the
rule instead of only in a commit message.

`packages/db/timestamps.test.ts` asserted that `withUtcTimestamps` registers a
UTC parser on oid 1114 by reading it off a bare postgres.js client. Every real
client is then handed to `drizzle()`, which overwrites that entry with a
transparent parser, so the assertion held whether or not the parser had any
effect. The mechanism is fine and stays: drizzle's own `PgTimestamp` mapper
appends `+0000`, so the read is UTC-correct either way and the session
`TimeZone` pin — the write-side fix — is untouched by `drizzle()`. The test now
resolves the parser both before and after `drizzle()`, pins the clobbering it
depends on, and asserts the instant recovered through the full composition, so
a regression in either layer is red. `timestamps.ts` records why the inert
entry is kept.

`nul-byte-boundary.test.ts` embedded a raw U+0000, so git classified it binary
and rendered it as `Bin 0 -> 4102 bytes` — the test proving the NUL hardening
works was the one file a reviewer could not read. The escape is byte-for-byte
equivalent at runtime. Two older files had the same defect and are fixed the
same way. `check:source-text` now fails the build on a raw NUL in any tracked
source file, and `.gitattributes` forces source files to diff as text so the
next one is visible in review rather than hidden by it.
The workflow-create `23505` handler answered for the whole transaction, which
also runs `saveWorkflowToNormalizedTables`. `workflow_blocks.id` is a global
primary key, so a block-id collision — an integrity fault already seen in
production — surfaced as `A workflow named "X" already exists in this folder`.
Match on the constraint name; any other unique violation propagates unchanged.

Moving the knowledge dispatch out of the completion transaction was right, but a
dispatch failure then committed the session as `completed` and left the document
at `pending`, which nothing sweeps and `retryProcessing` refuses. Record the
failure on the document instead, so it lands on the existing failed-document
path, and describe what the code does rather than a recovery branch that cannot
fire for this state.

The MCP re-registration reset stopped a registration claiming a connection it
never made, but reset for any re-registration. `isServerEligibleForDiscovery`
skips an OAuth row that is not `connected`, so a rename removed every tool the
server published with no path back. Scope the reset to url, transport, headers,
auth type, OAuth credentials, and revival.
The null-policy work made `reject` the default for caller-supplied writes,
which is right, but it landed on the wrong values.

- A partial update coerces the MERGED row, so an untouched legacy cell failed
  an unrelated column's update — and failed a paged bulk job after its earlier
  pages had committed. The merged-row callers now name the patch's keys; every
  other key follows the `null` policy, in the in-memory copy only (the write
  sends the patched keys alone).
- A multiselect whose members do not all resolve returned `{ok:false}`, which
  on the machine paths that pass `'null'` — CSV import, computed writes, the
  cell-write snapshot — erased the whole cell. Those paths now consult a new
  `salvage` hook and keep the members that do resolve; a caller-supplied write
  still 400s on an unknown option.
- Refusing a bare number in `date.coerce` reached the executor, v1, copilot and
  the grid. The refusal stays where there is a caller to tell, and `salvage`
  restores the milliseconds reading where the only other answer is a blank cell.

Also: the cursor docblocks claimed pure-keyset cursors were left unbound while
the code and its tests bind them; a saved-view create took the table's SCHEMA
advisory lock, so it queued behind column rewrites whose statement timeouts run
past its 3s lock_timeout, and now takes a views-scoped lock instead; and a view
whose column was deleted could not be saved at all, because the Save chip always
resends the filter — references the stored config already carries are now exempt
while a newly introduced one is still refused.

The cursor version is deliberately not bumped: the stamp is additive, unfiltered
in-flight tokens keep working, and a filtered one fails with the accurate
"restart paging without the cursor" rather than a generic unreadable-cursor 400.
mapFromDriverValue is typed unknown, so the composition assertions did not
type-check outside the test's own runner.
…nd them

Five of the reported defects were real and four of them were documentation
that had stopped describing its own code.

`cleanCellValue` said only "coerce a raw input value"; it also answers `null`
for anything the column type refuses, and since the multiselect write path
started refusing partial matches that is the difference between a paste
storing one option and blanking the cell. It deliberately does not consult
`salvage`, which would read the same paste as the option that did resolve —
that reading is for writes with no caller to answer, and a typed cell has one.
The pairing is now asserted, so a future helper that "improves" the paste by
salvaging it fails.

`EXECUTE_OPTION_CONSTRAINTS` carried two stacked TSDoc blocks, the second
explaining that the enumeration had moved onto the fields; the body schema
still told a reader the six combinations were enumerated in the constant. The
deployment route's second block orphaned the endpoint documentation above it,
and `list-query.ts` kept the TSDoc for a cursor message that now lives, with
its own rewritten doc, in `cursor-binding.ts`. Two agents left near-identical
essays arguing the same 400-vs-403-vs-409 question about the table ceilings
and concluding that neither status changes; the decision is recorded once, in
`billing.ts`, and `service.ts` points at it.

The credentials use case echoed `sortBy`/`sortOrder` back with a TSDoc
explaining that the presenter needs them, which it no longer does — it reads
`query.*`. The local upload roots move from the data-plane provider to
`core/storage-key.ts`, beside the sidecar suffix, so the cleanup sweep can name
what it reclaims without importing the transport that writes it.

`documents.test.ts` justified sweeping only knowledge and files for the 413 by
saying the same sweep over the other five documents still reported gaps. It
does not: widened to all seven, every body-carrying operation publishes it.

Three reports did not survive checking, and the evidence is recorded where the
next reader will look. An empty rerank result is not the reranker matching
nothing — `rerank` asks for `top_n` over a non-empty document list, so an empty
array means the response carried nothing usable, which is what `unavailable`
already promises. The zero-byte knowledge document is refused on the
upload-session path too, by `validateFile`, under both boundary contracts;
that parity is now pinned, and it fails if the guard is removed. The MCP
re-registration reports exactly the connection fields its SET clause writes,
and the create mutation already drops both caches — what lags is the status
badge, not the tools, because discovery is gated on `connected` for OAuth rows
only.
The filters compile to inArray, which is set membership, so workflowIds=A,A,B
selects exactly what A,B does. Sorting alone still bound them to different
pages, so an equivalent filter with a repeated member 400d mid-walk.
…hat could not fail

Six risks an adversarial read of this week's diff raised, verified one at a
time. Two of the six were already correct and are reported as such rather than
changed.

`v2HeadAuthorizationResponse` optional-called the use case's authorization
phase, so a use case without one would have answered the bodiless 200 that
`headSafe: false` exists to prevent. The definition-time guard does cover both
builders that reach it — they are its only callers — but an optional call turns
a missing phase into that leak silently, so the responder now refuses instead
of skipping.

`packages/db/timestamps.test.ts` assigned `process.env.TZ` at module scope and
never restored it. `TZ` is process state: a worker running files back to back
carried Asia/Tokyo into every file that followed, and only when the ordering put
it after this one. The zone is now set and restored around the file, with both
properties the suite depends on intact.

Upload publication moved its staging area out of the destination's own
directory into a shared `.staging` root, which makes the publishing `link` a
cross-subtree one. A volume mounted under part of the uploads tree puts the two
on different devices and `link` answers `EXDEV`, which the same-directory link
could not. Publication now copies onto the destination's device and links from
there, keeping the create-or-fail step that stops a replay from overwriting a
stored object.

Five tests that passed regardless of the code:

- `resolveFolderPathFilter` was only ever exercised through hand-written
  reimplementations in the suites that mock it out, so widening a miss to
  unfiltered — every filtered list answering with the whole workspace — left
  them all green. The real helper is now tested where it lives.
- The only measurement of `generateWorkspaceFileKey` asserted the key's last
  component against `NAME_MAX` rather than the component plus the sidecar
  written beside it, so it passed with the sidecar reservation removed.
- `GET /logs` asserted only that a rejected cursor does NOT name `sortBy`,
  which almost any wording satisfies, including one saying nothing at all.
- The skills lifecycle test asserted that the four writes agree on a
  workspace-key policy, which a lifecycle uniformly allowing one also
  satisfies; it now pins the policy they agree on and the kinds they admit.
- The v2 skills create test lost `expect(capture).not.toHaveBeenCalled()` when
  the create path moved to a personal key. The behaviour it pinned is gone —
  the workspace-key create is refused now — so it is re-homed as the refusal
  reaching the caller as a 403 with no analytics behind it.

Two claims did not hold. `CURSOR_VERSION` is correctly left at 1: the filter
stamp is additive, a pre-stamp token still decodes, an unfiltered read still
resumes, and only a filtered replay fails — with a conflict that names the
filter, where a version bump would answer a generic unreadable-cursor 400 to
every in-flight token. Tests pin all three, plus the minted version itself. And
the upload-session key-budget cases do exercise the real shared budget through
the real segment builder; only the workspace-key prefix is the stub's, which is
now stated where the stub is declared.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant