Merge remote-tracking branch 'origin/staging' into fix/agiloft-connector

Author: mzxchandra

.agents/skills/v2-api-conventions/SKILL.md

@@ -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.
 
@@ -51,7 +52,7 @@ A route built with `defineV2JsonRoute` gets this for free: its `present` returns
 | 200 / 201 | — | Success. 201 only for a created resource. |
 | 400 | `BAD_REQUEST` | Contract validation. Carries field-level `details` from `serializeZodIssues`. |
 | 401 | `UNAUTHORIZED` | No/!valid API key. **Runs before the rollout gate.** |
-| 403 | `FORBIDDEN` | Authenticated, same tenant, insufficient rights. Carry a machine-readable `details.code` (e.g. `WORKFLOW_NOT_DEPLOYED`). |
+| 403 | `FORBIDDEN` | Authenticated, same tenant, insufficient rights. Carries a machine-readable `details.code` from the closed set in `lib/core/application/forbidden.ts` (e.g. `INSUFFICIENT_WORKSPACE_ROLE`, `PERSONAL_API_KEYS_DISABLED`). |
 | 404 | `NOT_FOUND` | Not found, **and** cross-tenant concealment, **and** the rollout gate, **and** an unknown path. |
 | 409 | `CONFLICT` | Uniqueness/state conflict, human-readable message. |
 | 413 | `PAYLOAD_TOO_LARGE` | Body over the route's `maxBodyBytes`. |
@@ -62,13 +63,23 @@ 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:
 
 - An operation whose `minimumRole` is `write` or `admin` can always 403 — a member with a lower role hits it. Document 403.
 - An operation whose `minimumRole` is `read` cannot 403 *that* way, because `read` is the floor of the `read < write < admin` ordering and anyone without access is concealed as 404 instead. It can still 403 through `PersonalApiKeysDisabledError` (a personal API key against a workspace whose organization disabled them) or `WorkspaceApiKeyAuthorizationError` (`workspaceApiKey: 'deny'`), and every v2 operation is reachable by a personal API key. **So in practice every workspace-scoped v2 operation documents 403**, and the reads that omitted it were wrong, not principled.
 
+**Every 403 names its cause in `error.details.code`.** One status covers four different remedies — raise a member's role, issue a personal key instead of a workspace-scoped one, re-point a workspace key, buy an enterprise plan — and prose is not branchable, so a client that must tell them apart was string-matching messages, which turns every reword into a silent break.
+
+The vocabulary is a closed set, `FORBIDDEN_DETAIL_CODES` in `lib/core/application/forbidden.ts`, with a `Record` of descriptions beside it that the generated OpenAPI 403 description is built from. Adding a member fails to compile until it is documented, so a code cannot reach the wire unpublished. Do not invent a code at a route: throw `ForbiddenOperationError(code, message)` from the domain and let `v2CaughtOrchestrationError` — the function every v2 error policy falls through to — attach it. `InsufficientWorkspacePermissionsError`, `PersonalApiKeysDisabledError`, `WorkspaceApiKeyAuthorizationError`, and `PrincipalKindAuthorizationError` already carry theirs.
+
+The cross-tenant refusals (`NoWorkspaceAccessError`, `WorkspaceApiKeyScopeAuthorizationError`, `DelegatedWorkspaceAuthorizationError`) deliberately carry **no** code. They are concealed as 404, and naming their cause would hand back the resource-existence signal the concealment exists to withhold.
+
 Use the shared sets in `contracts/v2/openapi/shared.ts` — `RESOURCE_ERRORS`, `RESOURCE_CONFLICT_ERRORS`, `RESOURCE_MUTATION_ERRORS` — rather than assembling a per-operation list; all three already include `Forbidden`, and hand-assembled lists are how three knowledge reads and three upload operations quietly lost it.
 
 **HEAD is answered by the `GET` handler, not rejected.** Next aliases a missing `HEAD` export onto `GET` and drops the body when sending, so a route's `GET` legitimately runs with `request.method === 'HEAD'`. The builders' method guard accepts that pairing via `methodMatchesContract`; any other mismatch stays a hard error. Never hand-write a `HEAD` export to "fix" this.
@@ -94,9 +105,15 @@ Two cursor schemes exist, both opaque base64-JSON from `response.ts`. Which one
 
 Return `nextCursor: null` on the last page and only then. Never construct a cursor client-side.
 
+**Ordering is `sortBy` + `sortOrder`, except where there is nothing to sort by.** Fourteen lists take the pair. Two — `GET /logs` and `GET /workflows/{id}/runs` — have exactly one sortable column (start time), so there is no `sortBy` to pair with and the direction rides on a single `order` param; `sortBy`/`sortOrder` are not accepted there. That split is documented in both contracts and is the *only* sanctioned deviation. A new list picks the pair. Do not "fix" the two by accepting `sortOrder` as an alias: an alias is a second spelling of one thing with undefined precedence when both arrive, which is its own inconsistency, and renaming `order` would break every shipped caller.
+
+**A boolean query param is a real boolean**, declared with `booleanQueryFlagSchema` from `contracts/primitives.ts`. It coerces `'true'`/`'1'` and `'false'`/`'0'`/`''`, so it is a strict widening of a `z.enum(['true','false'])` — which is what two v2 params used to be, purely by inheritance from the internal shapes they reused. Reusing an internal `.shape.x` inherits the internal spelling; re-declare instead when the internal one is not the v2 convention.
+
 ## Rule 4 — reject what you do not implement
 
-Query and body schemas are **`.strict()`**. Zod strips unknown keys by default, so a non-strict schema answers `?limit=1` with 200 and the whole set — the caller believes it bounded the response and it did not. That is a contract lie, and on an uncapped list it is also an unbounded-response risk.
+Query and body schemas are **`.strict()`** — and `.strict()` binds the **top level only**. A strict body containing a non-strict nested object still drops unknown keys one level down, which is the headline `filter` bug at a smaller scale: `sort: [{ field, direction, nulls: 'last' }]` answered 200 and ordered by the default. Strictness belongs on the shared nested schema (`sortSpecSchema`'s element, `tableViewConfigSchema`), not restated per body.
+
+Before tightening a schema that is **also** a response or a stored blob, make the read canonical first. `table_views.config` is schemaless JSONB, so a legacy row carrying a retired key would fail a newly strict response parse and become a 500; `normalizeStoredViewConfig` projects the stored blob onto the declared keys so the tightening is safe in both directions. Zod strips unknown keys by default, so a non-strict schema answers `?limit=1` with 200 and the whole set — the caller believes it bounded the response and it did not. That is a contract lie, and on an uncapped list it is also an unbounded-response risk.
 
 Error messages name the field and, where there is one, the escape hatch:
 
@@ -182,19 +199,21 @@ 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.
 - [ ] The list is classified in `list-pagination.test.ts`.
 - [ ] Cross-tenant access answers 404, never 403 — and carries `Cache-Control: private, no-store`, because RFC 9110 §15.5.5 makes 404 heuristically cacheable and an authorization-dependent 404 must never be stored. `v2Error` sets this unconditionally; do not build a v2 response any other way.
 - [ ] A retryable failure says when: 429 and 503 carry `Retry-After`. No other status invents one.
-- [ ] 403s carry a machine-readable `details.code`.
+- [ ] 403s carry a machine-readable `details.code` from `FORBIDDEN_DETAIL_CODES`, thrown as `ForbiddenOperationError` in the domain rather than attached at the route.
+- [ ] Nested objects inside a `.strict()` body are strict too — `.strict()` does not recurse.
+- [ ] Ordering uses `sortBy` + `sortOrder`; boolean query params use `booleanQueryFlagSchema`.
 - [ ] Validation messages name the field and echo the valid set.
 - [ ] Response schema matches every field the route actually emits.
 - [ ] OpenAPI description regenerated and truthful about pagination.
 - [ ] `bun run type-check`, `bun run check:api-validation`, `bun run check:openapi` pass.
 
 ## Known gap
 
-A 405 on a path that *does* have a route file but does not export that verb is generated by Next.js before any Sim code runs: zero-byte body, no `content-type`, and no `Allow` header, which RFC 9110 §15.5.6 requires. Fixing it means either exporting explicit rejecting handlers from all 77 v2 route files or intercepting in `apps/sim/proxy.ts` with a static path→methods table. Neither is done. Unknown *paths* are handled — the catch-all covers those.
+A 405 on a path that *does* have a route file but does not export that verb is generated by Next.js before any Sim code runs: zero-byte body, no `content-type`, and no `Allow` header, which RFC 9110 §15.5.6 requires. Fixing it means either exporting explicit rejecting handlers from every v2 route file or intercepting in `apps/sim/proxy.ts` with a static path→methods table. Neither is done. Unknown *paths* are handled — the catch-all covers those.