Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
9f5eabb
fix(v2): close four validation holes in the logs and billing surfaces
waleedlatif1 Aug 12, 2026
45e4431
feat(v2): expose the MCP tool plane and page the MCP server list
waleedlatif1 Aug 12, 2026
f9462b6
feat(v2/tables): strict row bodies, a filtered row count, and round-t…
waleedlatif1 Aug 12, 2026
39090ce
test(skills): pin the workspace-API-key split as structural, not acci…
waleedlatif1 Aug 12, 2026
491f9d5
feat(v2): read deployment state, and undo a file delete
waleedlatif1 Aug 12, 2026
66b4865
fix(v2): keep the unresolvable-cursor rejection a 400 on every surface
waleedlatif1 Aug 12, 2026
c9b16eb
fix(v2/tables): sweep union bodies per member and name the shapes on …
waleedlatif1 Aug 12, 2026
891fb2d
fix(v2): classify MCP discovery failures by type, not by substring
waleedlatif1 Aug 12, 2026
c12f71f
Merge branch 'wip/v2-logs-billing' into HEAD
waleedlatif1 Aug 12, 2026
ee70a87
merge: reconcile shared ratchets across the four v2 workstreams
waleedlatif1 Aug 12, 2026
8437e64
merge: bring in the MCP tool plane workstream
waleedlatif1 Aug 12, 2026
06492de
merge: bring in the MCP tool plane workstream and reconcile ratchets
waleedlatif1 Aug 12, 2026
1e68826
feat(v2): make knowledge tags usable and let documents be updated
waleedlatif1 Aug 12, 2026
9ce4403
merge: bring in the knowledge workstream and reconcile ratchets
waleedlatif1 Aug 12, 2026
03382c7
fix(v2): name every 403 cause, unfork boolean params, close nested st…
waleedlatif1 Aug 12, 2026
d150f90
style: format the files the workspace-scoped lint gate does not reach
waleedlatif1 Aug 12, 2026
db5f7ef
fix(v2): collapse the four knowledge document projections onto one nu…
waleedlatif1 Aug 12, 2026
860c4c8
fix(v2): stop HEAD driving MCP discovery, and unbreak the updatedAt k…
waleedlatif1 Aug 12, 2026
8ba9755
fix(v2): bound the bulk update echo, close the search leak, and make …
waleedlatif1 Aug 12, 2026
ae57d2f
fix(mcp): bound the connect handshake, and stop the 403 description o…
waleedlatif1 Aug 12, 2026
756310e
Merge remote-tracking branch 'origin/staging' into HEAD
waleedlatif1 Aug 12, 2026
d673626
chore: reconcile the route ratchet with staging
waleedlatif1 Aug 12, 2026
ffb8306
style: sort imports and format the three files biome flagged
waleedlatif1 Aug 12, 2026
59232f8
fix(openapi): import the forbidden-code constants from their module, …
waleedlatif1 Aug 12, 2026
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
22 changes: 18 additions & 4 deletions .agents/skills/v2-api-conventions/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,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`. |
Expand All @@ -74,6 +74,12 @@ And this class survives a green test suite — `keysetAfter` returned well-forme
- 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.
Expand All @@ -99,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:

Expand Down Expand Up @@ -194,12 +206,14 @@ Run this against any new or changed v2 endpoint.
- [ ] 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.
22 changes: 18 additions & 4 deletions .claude/commands/v2-api-conventions.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,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`. |
Expand All @@ -73,6 +73,12 @@ And this class survives a green test suite — `keysetAfter` returned well-forme
- 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.
Expand All @@ -98,9 +104,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:

Expand Down Expand Up @@ -193,12 +205,14 @@ Run this against any new or changed v2 endpoint.
- [ ] 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.
Loading
Loading