Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions .agents/skills/v2-api-conventions/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,13 @@ failure (always) { "error": { "code": "...", "message": "...", "details"?:

Nothing else at the top level. No `success: true`, no bare `{ "error": "string" }`, no HTML.

That promise is worth stating as a rule because it has been broken four separate ways, each time by a route or a builder taking a shortcut that looked local:
That promise is worth stating as a rule because it has been broken five separate ways, each time by a route or a builder taking a shortcut that looked local:

- `GET /workflows?limit=1.5` returned **500**. The contract was copied from a sibling and lost its `.int()`, so a fractional limit passed validation and reached Postgres as `LIMIT 2.5`.
- A malformed JSON body returned **`{"error":"Request body must be valid JSON"}`** — a bare string. The envelope was a per-route opt-in that only 8 of 77 routes remembered.
- `GET /api/v2/nonexistent` returned a **full HTML 404 document**, because no route file matched and the request fell through to the app's global not-found page.
- Four collections returned `nextCursor` while **silently discarding** any `limit` the caller sent, because Zod strips unknown keys unless the schema is `.strict()`.
- Handing back a `nextCursor` from any timestamp-sorted list and passing it straight in returned **500**. The value was validated and bound — but bound with no SQL type, into `date_trunc`, which is overloaded, so Postgres could resolve no overload. Validation was never the missing half; the type was.

Each was one line. The rules below are the generalisations.

Expand Down Expand Up @@ -62,7 +63,11 @@ Two of these carry real design weight:

**404 is deliberately overloaded.** A workspace the caller cannot reach answers `404 "Workspace not found"`, never 403 — a 403 would confirm the resource exists. `createV2ResourceConcealmentPolicy` does this by mapping a cross-tenant authorization failure to `v2Error('NOT_FOUND', ...)`. The rollout gate answers the same way for the same reason (`gate.ts`: "an ungated caller cannot distinguish 'not in the rollout cohort' from 'no such endpoint'"), and so does the unknown-path catch-all at `app/api/v2/[[...segments]]/route.ts` — its body is byte-identical to the gate's on purpose.

**500 is never caller-reachable.** Any input a caller can send must be rejected at the contract boundary with a 400. If you can construct a query string or body that produces a 500, that is a bug in the contract, not something to wrap in a `try`/`catch`. `v2ErrorForOrchestration` also replaces the message on an unclassified failure with a generic one, so internal detail never leaks. A caller-reachable 500 has shipped twice — a fractional `limit` reaching `LIMIT 2.5`, and a plain `HEAD` tripping the builder's method guard — so treat "a well-formed request produced a 500" as the highest-severity class of defect on this surface.
**500 is never caller-reachable.** Any input a caller can send must be rejected at the contract boundary with a 400. If you can construct a query string or body that produces a 500, that is a bug in the contract, not something to wrap in a `try`/`catch`. `v2ErrorForOrchestration` also replaces the message on an unclassified failure with a generic one, so internal detail never leaks. A caller-reachable 500 has shipped three times — a fractional `limit` reaching `LIMIT 2.5`, a plain `HEAD` tripping the builder's method guard, and a keyset cursor's timestamp reaching `date_trunc` as an untyped placeholder — so treat "a well-formed request produced a 500" as the highest-severity class of defect on this surface.

**Validating a value is only half of it; the value also has to reach SQL with a type.** A bound parameter arrives as `unknown` and takes its type from context. Against a typed column (`sort_order > $1`) that inference always succeeds, which is why the gap stays invisible almost everywhere — but as an argument to an overloaded function it can resolve to nothing at all. So: **if a bound value is an argument to a SQL function rather than one side of a comparison, write its type down** (`lib/api/list-query.ts`, `timestampKey`, casts from the column).

And this class survives a green test suite — `keysetAfter` returned well-formed SQL and every assertion passed; only Postgres's parser rejected it. When a change alters the *shape* of generated SQL rather than its values, execute it somewhere before believing the suite.

**Which of 403 and 404 an operation documents follows from its authorization, not from whether it is a read.** `requirePermission` throws two different failures: no workspace access at all is `NoWorkspaceAccessError`, which `createV2ResourceConcealmentPolicy` conceals as 404; access below the operation's `minimumRole` is `InsufficientWorkspacePermissionsError`, which stays a 403. So:

Expand Down Expand Up @@ -182,7 +187,7 @@ Run this against any new or changed v2 endpoint.
- [ ] Success body is exactly `{data}` or `{data, nextCursor}`; failures are exactly `{error:{code,message,details?}}`.
- [ ] Route uses a shared builder; no hand-built `NextResponse.json`.
- [ ] Query and body schemas are `.strict()`.
- [ ] No caller-supplied value can produce a 500 — check every numeric param reaches SQL as a validated integer.
- [ ] No caller-supplied value can produce a 500 — check every numeric param reaches SQL as a validated integer, and that any bound value passed as an argument to a SQL function carries an explicit type.
- [ ] `limit` comes from `v2PaginationFields`, not a hand-written `z.coerce.number()`.
- [ ] If the response carries `nextCursor`, the query accepts `limit` + `cursor` and the query actually applies them.
- [ ] Keyset sorts end in a unique `id` key.
Expand Down
11 changes: 8 additions & 3 deletions .claude/commands/v2-api-conventions.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,13 @@ failure (always) { "error": { "code": "...", "message": "...", "details"?:

Nothing else at the top level. No `success: true`, no bare `{ "error": "string" }`, no HTML.

That promise is worth stating as a rule because it has been broken four separate ways, each time by a route or a builder taking a shortcut that looked local:
That promise is worth stating as a rule because it has been broken five separate ways, each time by a route or a builder taking a shortcut that looked local:

- `GET /workflows?limit=1.5` returned **500**. The contract was copied from a sibling and lost its `.int()`, so a fractional limit passed validation and reached Postgres as `LIMIT 2.5`.
- A malformed JSON body returned **`{"error":"Request body must be valid JSON"}`** — a bare string. The envelope was a per-route opt-in that only 8 of 77 routes remembered.
- `GET /api/v2/nonexistent` returned a **full HTML 404 document**, because no route file matched and the request fell through to the app's global not-found page.
- Four collections returned `nextCursor` while **silently discarding** any `limit` the caller sent, because Zod strips unknown keys unless the schema is `.strict()`.
- Handing back a `nextCursor` from any timestamp-sorted list and passing it straight in returned **500**. The value was validated and bound — but bound with no SQL type, into `date_trunc`, which is overloaded, so Postgres could resolve no overload. Validation was never the missing half; the type was.

Each was one line. The rules below are the generalisations.

Expand Down Expand Up @@ -61,7 +62,11 @@ Two of these carry real design weight:

**404 is deliberately overloaded.** A workspace the caller cannot reach answers `404 "Workspace not found"`, never 403 — a 403 would confirm the resource exists. `createV2ResourceConcealmentPolicy` does this by mapping a cross-tenant authorization failure to `v2Error('NOT_FOUND', ...)`. The rollout gate answers the same way for the same reason (`gate.ts`: "an ungated caller cannot distinguish 'not in the rollout cohort' from 'no such endpoint'"), and so does the unknown-path catch-all at `app/api/v2/[[...segments]]/route.ts` — its body is byte-identical to the gate's on purpose.

**500 is never caller-reachable.** Any input a caller can send must be rejected at the contract boundary with a 400. If you can construct a query string or body that produces a 500, that is a bug in the contract, not something to wrap in a `try`/`catch`. `v2ErrorForOrchestration` also replaces the message on an unclassified failure with a generic one, so internal detail never leaks. A caller-reachable 500 has shipped twice — a fractional `limit` reaching `LIMIT 2.5`, and a plain `HEAD` tripping the builder's method guard — so treat "a well-formed request produced a 500" as the highest-severity class of defect on this surface.
**500 is never caller-reachable.** Any input a caller can send must be rejected at the contract boundary with a 400. If you can construct a query string or body that produces a 500, that is a bug in the contract, not something to wrap in a `try`/`catch`. `v2ErrorForOrchestration` also replaces the message on an unclassified failure with a generic one, so internal detail never leaks. A caller-reachable 500 has shipped three times — a fractional `limit` reaching `LIMIT 2.5`, a plain `HEAD` tripping the builder's method guard, and a keyset cursor's timestamp reaching `date_trunc` as an untyped placeholder — so treat "a well-formed request produced a 500" as the highest-severity class of defect on this surface.

**Validating a value is only half of it; the value also has to reach SQL with a type.** A bound parameter arrives as `unknown` and takes its type from context. Against a typed column (`sort_order > $1`) that inference always succeeds, which is why the gap stays invisible almost everywhere — but as an argument to an overloaded function it can resolve to nothing at all. So: **if a bound value is an argument to a SQL function rather than one side of a comparison, write its type down** (`lib/api/list-query.ts`, `timestampKey`, casts from the column).

And this class survives a green test suite — `keysetAfter` returned well-formed SQL and every assertion passed; only Postgres's parser rejected it. When a change alters the *shape* of generated SQL rather than its values, execute it somewhere before believing the suite.

**Which of 403 and 404 an operation documents follows from its authorization, not from whether it is a read.** `requirePermission` throws two different failures: no workspace access at all is `NoWorkspaceAccessError`, which `createV2ResourceConcealmentPolicy` conceals as 404; access below the operation's `minimumRole` is `InsufficientWorkspacePermissionsError`, which stays a 403. So:

Expand Down Expand Up @@ -181,7 +186,7 @@ Run this against any new or changed v2 endpoint.
- [ ] Success body is exactly `{data}` or `{data, nextCursor}`; failures are exactly `{error:{code,message,details?}}`.
- [ ] Route uses a shared builder; no hand-built `NextResponse.json`.
- [ ] Query and body schemas are `.strict()`.
- [ ] No caller-supplied value can produce a 500 — check every numeric param reaches SQL as a validated integer.
- [ ] No caller-supplied value can produce a 500 — check every numeric param reaches SQL as a validated integer, and that any bound value passed as an argument to a SQL function carries an explicit type.
- [ ] `limit` comes from `v2PaginationFields`, not a hand-written `z.coerce.number()`.
- [ ] If the response carries `nextCursor`, the query accepts `limit` + `cursor` and the query actually applies them.
- [ ] Keyset sorts end in a unique `id` key.
Expand Down
11 changes: 8 additions & 3 deletions .cursor/commands/v2-api-conventions.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,13 @@ failure (always) { "error": { "code": "...", "message": "...", "details"?:

Nothing else at the top level. No `success: true`, no bare `{ "error": "string" }`, no HTML.

That promise is worth stating as a rule because it has been broken four separate ways, each time by a route or a builder taking a shortcut that looked local:
That promise is worth stating as a rule because it has been broken five separate ways, each time by a route or a builder taking a shortcut that looked local:

- `GET /workflows?limit=1.5` returned **500**. The contract was copied from a sibling and lost its `.int()`, so a fractional limit passed validation and reached Postgres as `LIMIT 2.5`.
- A malformed JSON body returned **`{"error":"Request body must be valid JSON"}`** — a bare string. The envelope was a per-route opt-in that only 8 of 77 routes remembered.
- `GET /api/v2/nonexistent` returned a **full HTML 404 document**, because no route file matched and the request fell through to the app's global not-found page.
- Four collections returned `nextCursor` while **silently discarding** any `limit` the caller sent, because Zod strips unknown keys unless the schema is `.strict()`.
- Handing back a `nextCursor` from any timestamp-sorted list and passing it straight in returned **500**. The value was validated and bound — but bound with no SQL type, into `date_trunc`, which is overloaded, so Postgres could resolve no overload. Validation was never the missing half; the type was.

Each was one line. The rules below are the generalisations.

Expand Down Expand Up @@ -56,7 +57,11 @@ Two of these carry real design weight:

**404 is deliberately overloaded.** A workspace the caller cannot reach answers `404 "Workspace not found"`, never 403 — a 403 would confirm the resource exists. `createV2ResourceConcealmentPolicy` does this by mapping a cross-tenant authorization failure to `v2Error('NOT_FOUND', ...)`. The rollout gate answers the same way for the same reason (`gate.ts`: "an ungated caller cannot distinguish 'not in the rollout cohort' from 'no such endpoint'"), and so does the unknown-path catch-all at `app/api/v2/[[...segments]]/route.ts` — its body is byte-identical to the gate's on purpose.

**500 is never caller-reachable.** Any input a caller can send must be rejected at the contract boundary with a 400. If you can construct a query string or body that produces a 500, that is a bug in the contract, not something to wrap in a `try`/`catch`. `v2ErrorForOrchestration` also replaces the message on an unclassified failure with a generic one, so internal detail never leaks. A caller-reachable 500 has shipped twice — a fractional `limit` reaching `LIMIT 2.5`, and a plain `HEAD` tripping the builder's method guard — so treat "a well-formed request produced a 500" as the highest-severity class of defect on this surface.
**500 is never caller-reachable.** Any input a caller can send must be rejected at the contract boundary with a 400. If you can construct a query string or body that produces a 500, that is a bug in the contract, not something to wrap in a `try`/`catch`. `v2ErrorForOrchestration` also replaces the message on an unclassified failure with a generic one, so internal detail never leaks. A caller-reachable 500 has shipped three times — a fractional `limit` reaching `LIMIT 2.5`, a plain `HEAD` tripping the builder's method guard, and a keyset cursor's timestamp reaching `date_trunc` as an untyped placeholder — so treat "a well-formed request produced a 500" as the highest-severity class of defect on this surface.

**Validating a value is only half of it; the value also has to reach SQL with a type.** A bound parameter arrives as `unknown` and takes its type from context. Against a typed column (`sort_order > $1`) that inference always succeeds, which is why the gap stays invisible almost everywhere — but as an argument to an overloaded function it can resolve to nothing at all. So: **if a bound value is an argument to a SQL function rather than one side of a comparison, write its type down** (`lib/api/list-query.ts`, `timestampKey`, casts from the column).

And this class survives a green test suite — `keysetAfter` returned well-formed SQL and every assertion passed; only Postgres's parser rejected it. When a change alters the *shape* of generated SQL rather than its values, execute it somewhere before believing the suite.

**Which of 403 and 404 an operation documents follows from its authorization, not from whether it is a read.** `requirePermission` throws two different failures: no workspace access at all is `NoWorkspaceAccessError`, which `createV2ResourceConcealmentPolicy` conceals as 404; access below the operation's `minimumRole` is `InsufficientWorkspacePermissionsError`, which stays a 403. So:

Expand Down Expand Up @@ -176,7 +181,7 @@ Run this against any new or changed v2 endpoint.
- [ ] Success body is exactly `{data}` or `{data, nextCursor}`; failures are exactly `{error:{code,message,details?}}`.
- [ ] Route uses a shared builder; no hand-built `NextResponse.json`.
- [ ] Query and body schemas are `.strict()`.
- [ ] No caller-supplied value can produce a 500 — check every numeric param reaches SQL as a validated integer.
- [ ] No caller-supplied value can produce a 500 — check every numeric param reaches SQL as a validated integer, and that any bound value passed as an argument to a SQL function carries an explicit type.
- [ ] `limit` comes from `v2PaginationFields`, not a hand-written `z.coerce.number()`.
- [ ] If the response carries `nextCursor`, the query accepts `limit` + `cursor` and the query actually applies them.
- [ ] Keyset sorts end in a unique `id` key.
Expand Down
35 changes: 31 additions & 4 deletions apps/sim/lib/api/list-query.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,13 +105,24 @@ describe('timestampKey', () => {
)
})

it('truncates the bound cursor value to match, binding it through the column encoder', () => {
it('casts the bound cursor value so date_trunc has a resolvable overload', () => {
const { sql: text, params } = render(createdKey.bind('2024-01-01T00:00:00.123Z')!)

expect(text).toBe(`date_trunc('milliseconds', $1)`)
expect(text).toBe(`date_trunc('milliseconds', cast($1 as timestamp))`)
expect(params).toEqual(['2024-01-01T00:00:00.123Z'])
Comment thread
waleedlatif1 marked this conversation as resolved.
})

it('takes the cast from the column, so a timestamptz column keeps its offset', () => {
const zoned = pgTable('zoned', {
at: timestamp('at', { withTimezone: true }).notNull(),
})
const zonedKey = timestampKey<{ at: Date }>(zoned.at, (r) => r.at)

expect(render(zonedKey.bind('2024-01-01T00:00:00.123Z')!).sql).toBe(
`date_trunc('milliseconds', cast($1 as timestamp with time zone))`
)
})

it('rejects a cursor value that is not a parseable timestamp', () => {
expect(createdKey.bind('not-a-date')).toBeNull()
expect(createdKey.bind(1700000000000)).toBeNull()
Expand Down Expand Up @@ -173,13 +184,29 @@ describe('keysetAfter', () => {
)

expect(text).toBe(
`(date_trunc('milliseconds', "thing"."created_at") > date_trunc('milliseconds', $1) or ` +
`(date_trunc('milliseconds', "thing"."created_at") = date_trunc('milliseconds', $2) and ` +
`(date_trunc('milliseconds', "thing"."created_at") > date_trunc('milliseconds', cast($1 as timestamp)) or ` +
`(date_trunc('milliseconds', "thing"."created_at") = date_trunc('milliseconds', cast($2 as timestamp)) and ` +
`"thing"."id" > $3))`
)
expect(params).toEqual(['2024-01-01T00:00:00.123Z', '2024-01-01T00:00:00.123Z', 'file-7'])
})

/**
* Pins the class, not the instance: `date_trunc` is today's only wrapping, so
* what this catches is a future key that wraps its bound value untyped.
*/
it('leaves no bound value bare inside a function call', () => {
const { sql: text } = render(
keysetAfter(
[numberKey<Row>(thing.size, () => 0), createdKey, idKey],
[7, '2024-01-01T00:00:00.123Z', 'file-7'],
'asc'
)!
)

expect(text).not.toMatch(/[a-z_]+\((?:[^()]*,)?\s*\$\d+\s*\)/i)
})

/** A caller controls the cursor's contents, so a bad value is a 400, not a 500 from SQL. */
it('refuses a cursor carrying a value its key cannot hold', () => {
expect(keysetAfter([createdKey, idKey], ['not-a-date', 'file-7'], 'asc')).toBeNull()
Expand Down
Loading
Loading