feat: first-class clients (clients table + projects.client_id) - #402
Conversation
Reversible migration 040: clients (id, name UNIQUE non-empty, library_id SET NULL, timestamps) + projects.client_id RESTRICT + index. Commits the approved design doc and implementation plan for #391. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
New src/db/queries/clients.ts (createClient/listClients/getClient + assertClientExists, ClientNotFoundError/ClientLibraryNotFoundError). getClient joins its active projects as full ProjectSummary rows (LATERAL json_agg for sources). ProjectSummary gains clientId/clientName; updateProject gains a validated clientId set/clear path (unknown client → ClientNotFoundError → 422). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
POST/GET /clients + GET /clients/{id} (src/api/clients.ts, wired in router).
PATCH /projects/{id} accepts clientId (uuid | null): associate/disassociate,
echoes clientId, unknown client -> 422. openapi.yaml documents the endpoints,
ClientSummary/ClientDetail schemas, ClientId param, and ProjectSummary +
PATCH-response clientId/clientName; both directions of the contract gate pass.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
list_clients/get_client (read) + create_client (write) mirror the REST surface; registered in tools.ts with OP_TO_TOOL + TOOL_TIERS entries (contract gate green). update_project tool gains clientId (uuid | null): associate/disassociate, echoes clientId, unknown client -> isError. Tools never throw. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Records the four locked decisions (link-not-merge, RESTRICT/SET-NULL delete semantics, firm forward-compat as prose, clients-are-organizations-not-actors) and supersedes the ADR-025 firm/client-tier deferral. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 15 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughAdds a first-class ChangesFirst-class clients feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant ClientsHandler
participant ClientsQuery
participant Postgres
Client->>Router: POST /clients {name, libraryId}
Router->>ClientsHandler: createClientHandler
ClientsHandler->>ClientsQuery: createClient(input)
ClientsQuery->>Postgres: INSERT INTO clients
Postgres-->>ClientsQuery: client row
ClientsQuery-->>ClientsHandler: ClientSummary
ClientsHandler-->>Client: 201 ClientSummary
Client->>Router: PATCH /projects/{id} {clientId}
Router->>ClientsQuery: assertClientExists(clientId)
ClientsQuery->>Postgres: SELECT client by id
Postgres-->>ClientsQuery: row or none
ClientsQuery-->>Router: ok or ClientNotFoundError
Router->>Postgres: UPDATE projects SET client_id
Router-->>Client: 200 ProjectSummary with clientId
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Heads-up on the Test check: the unit file |
Fold in the lint fixes that were left uncommitted: prettier reformat of the clients API integration test, and the DB duplicate-name test asserting via the typed getPgCode helper (avoids an expect.objectContaining any). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
The patch handler returns 422 via ClientNotFoundError when a syntactically
valid but unknown clientId is supplied (src/api/projects.ts), and the clientId
description already promises it ('an unknown client is rejected 422'), but the
responses list omitted the 422 entry. Add it, referencing the shared
UnprocessableEntity response — consistent with POST /clients. Keeps the
authoritative contract in sync with the emitted status (the contract gate
validates documented responses but cannot enumerate an undocumented one).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codex (GPT-5.5, xhigh) adversarial review — backup gateCodeRabbit is rate-limited on this PR (bare "Review finished" ack, no walkthrough), so Codex ran as the review gate. Two P2 findings, both evaluated against ADR-054 and the code: [P2] Document the new 422 patch response on [P2] Reject non-client-tier library links in |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/db/queries/projects.ts (1)
395-416: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSame TOCTOU pattern as
createClient:assertClientExiststhenUPDATEisn't race-safe.If the client is deleted between the
assertClientExistscheck (line 403) and theUPDATE(line 404), theUPDATEfails with a raw FK violation (23503) rather than a cleanClientNotFoundError(→ 422), surfacing an ambiguous 500 instead. Same fix as suggested forcreateClientinclients.ts: catch the FK-violation code and map it toClientNotFoundError.🔒️ Proposed fix
+import { ClientNotFoundError, assertClientExists } from './clients.js'; +import { getPgCode } from '../../lib/pg-errors.js'; -import { assertClientExists } from './clients.js'; ... } catch (err) { if (err instanceof DatabaseError) throw err; + if (getPgCode(err) === '23503' && typeof input.clientId === 'string') { + throw new ClientNotFoundError(`client ${input.clientId} not found`, { cause: err }); + } throw new DatabaseError(`updateProject: update failed for ${id}`, { cause: err }); }Based on learnings, "treat the FK-violation error from the DELETE as the authoritative signal" for check-then-act races — the same principle applies to this check-then-update path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/queries/projects.ts` around lines 395 - 416, The updateProject flow still has a check-then-act race: `assertClientExists` can pass and the later `pool.query` UPDATE in `updateProject` can still fail with a foreign-key violation if the client was deleted in between. Update `updateProject` to treat the FK-violation from the UPDATE as the authoritative signal, similar to `createClient` in `clients.ts`: catch the database error code for missing client and rethrow `ClientNotFoundError` instead of letting it fall through as a generic `DatabaseError`.Source: Learnings
src/db/queries/clients.ts (1)
97-116: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTOCTOU:
assertLibraryExistscheck then INSERT is not race-safe.If the referenced library is deleted between the existence check (line 102) and the
INSERT(lines 103-106), the insert fails with a raw FK violation (23503), which falls through to the genericDatabaseErrorwrap (line 114) instead of the intendedClientLibraryNotFoundError(→ 422). Per an established pattern in this codebase, the FK-violation error from the write itself should be treated as the authoritative signal, not just the pre-check.
getPgCode(already used inclients.integration.test.tsfrom../../lib/pg-errors.js) can classify this without an unsafe cast.🔒️ Proposed fix
+import { getPgCode } from '../../lib/pg-errors.js'; ... } catch (err) { // ClientLibraryNotFoundError / DatabaseError re-throw unwrapped; a raw pg error // (e.g. 23505 unique name) is wrapped with its cause so getPgCode → 409 at the handler. if (err instanceof DatabaseError) throw err; + if (getPgCode(err) === '23503') { + throw new ClientLibraryNotFoundError(`library ${input.libraryId} not found`, { cause: err }); + } throw new DatabaseError(`createClient: insert failed for "${input.name}"`, { cause: err }); }Based on learnings, "treat the FK-violation error from the DELETE as the authoritative signal" for check-then-act races applies analogously here to check-then-insert.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/queries/clients.ts` around lines 97 - 116, The createClient flow in clients.ts is race-prone because it relies on assertLibraryExists before the INSERT, so a deleted library can turn into a generic DatabaseError instead of ClientLibraryNotFoundError. Update createClient to treat the FK violation from the db.query INSERT as the authoritative signal: inspect the caught pg error with getPgCode, and when it is the library foreign-key code map it to ClientLibraryNotFoundError; keep other errors wrapped as they are now. Preserve the existing DatabaseError handling for non-FK failures and locate the change in createClient / assertLibraryExists.Source: Learnings
src/db/migrations/040_create_clients.ts (1)
11-17: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider indexing
clients.library_id.
library_idis a FK withON DELETE SET NULL; without an index, deleting a library forces a full scan ofclientsto null out references, and any future lookup bylibrary_id(e.g., checking if a library is already linked to a client) would also scan. Worth adding an index alongside the FK.♻️ Proposed addition
pgm.addConstraint('clients', 'clients_name_unique', 'UNIQUE (name)'); pgm.addConstraint('clients', 'clients_name_nonempty', 'CHECK (length(trim(name)) > 0)'); + pgm.createIndex('clients', 'library_id', { name: 'clients_library_id_idx' });And in
down, drop it before dropping the table (table drop will remove it anyway via cascade, but for symmetry):export const down = (pgm: MigrationBuilder): void => { pgm.dropIndex('projects', 'client_id', { name: 'projects_client_id_idx' }); pgm.dropColumns('projects', ['client_id']); pgm.dropTable('clients', { cascade: true }); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/migrations/040_create_clients.ts` around lines 11 - 17, Add an index for the clients.library_id foreign key in the 040_create_clients migration so deletes and lookups don’t scan the whole clients table. Update the migration that calls pgm.createTable for clients to also create an index on library_id, and make the corresponding down path remove that index before the table drop for symmetry.openapi.yaml (1)
1461-1468: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the PATCH
/projects/{id}description to mentionclientId.The
anyOfwas extended to includeclientId(Line 1481) and the description now covers a third alternative, but the prose still only says "At least one ofnameorsectionNumberFormatmust be provided." Update it to includeclientIdfor consistency with the actual contract.📝 Proposed fix
description: > - Updates a project's mutable settings. At least one of `name` or - `sectionNumberFormat` must be provided. The name must be non-empty. - There is no uniqueness constraint on project names, so there is no 409 - path. + Updates a project's mutable settings. At least one of `name`, + `sectionNumberFormat`, or `clientId` must be provided. The name must + be non-empty. There is no uniqueness constraint on project names, so + there is no 409 path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openapi.yaml` around lines 1461 - 1468, The PATCH /projects/{id} description is out of sync with the request contract because it still says only name or sectionNumberFormat are allowed, while the schema in the patchProject operation also accepts clientId. Update the description text in the patchProject section of openapi.yaml so it explicitly says at least one of name, sectionNumberFormat, or clientId must be provided, keeping the prose consistent with the anyOf definition.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@openapi.yaml`:
- Around line 1461-1468: The PATCH /projects/{id} description is out of sync
with the request contract because it still says only name or sectionNumberFormat
are allowed, while the schema in the patchProject operation also accepts
clientId. Update the description text in the patchProject section of
openapi.yaml so it explicitly says at least one of name, sectionNumberFormat, or
clientId must be provided, keeping the prose consistent with the anyOf
definition.
In `@src/db/migrations/040_create_clients.ts`:
- Around line 11-17: Add an index for the clients.library_id foreign key in the
040_create_clients migration so deletes and lookups don’t scan the whole clients
table. Update the migration that calls pgm.createTable for clients to also
create an index on library_id, and make the corresponding down path remove that
index before the table drop for symmetry.
In `@src/db/queries/clients.ts`:
- Around line 97-116: The createClient flow in clients.ts is race-prone because
it relies on assertLibraryExists before the INSERT, so a deleted library can
turn into a generic DatabaseError instead of ClientLibraryNotFoundError. Update
createClient to treat the FK violation from the db.query INSERT as the
authoritative signal: inspect the caught pg error with getPgCode, and when it is
the library foreign-key code map it to ClientLibraryNotFoundError; keep other
errors wrapped as they are now. Preserve the existing DatabaseError handling for
non-FK failures and locate the change in createClient / assertLibraryExists.
In `@src/db/queries/projects.ts`:
- Around line 395-416: The updateProject flow still has a check-then-act race:
`assertClientExists` can pass and the later `pool.query` UPDATE in
`updateProject` can still fail with a foreign-key violation if the client was
deleted in between. Update `updateProject` to treat the FK-violation from the
UPDATE as the authoritative signal, similar to `createClient` in `clients.ts`:
catch the database error code for missing client and rethrow
`ClientNotFoundError` instead of letting it fall through as a generic
`DatabaseError`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d45e0d5f-62bc-4b80-bb28-ea55730fae13
📒 Files selected for processing (24)
docs/adr/054-first-class-clients.mddocs/superpowers/plans/2026-07-07-issue-391-clients.mddocs/superpowers/specs/2026-07-07-issue-391-clients-design.mdopenapi.yamlsrc/api/clients.integration.test.tssrc/api/clients.tssrc/api/contract.integration.test.tssrc/api/projects.test.tssrc/api/projects.tssrc/api/router.tssrc/db/index.tssrc/db/migrations/040_create_clients.tssrc/db/queries/clients.integration.test.tssrc/db/queries/clients.test.tssrc/db/queries/clients.tssrc/db/queries/projects.test.tssrc/db/queries/projects.tssrc/mcp/capabilities.tssrc/mcp/clients-handlers.tssrc/mcp/clients-tools.tssrc/mcp/clients.integration.test.tssrc/mcp/contract-map.tssrc/mcp/project-handlers.tssrc/mcp/tools.ts
assertClientExists / assertLibraryExists are fast-path existence checks, but the referenced row can be deleted between the check and the write. The FK then raises pg 23503 on the write, which fell through to a generic DatabaseError → ambiguous 500 at the handler. Catch 23503 (via getPgCode) and re-throw the same typed error as the fast path — ClientNotFoundError (updateProject) / ClientLibraryNotFoundError (createClient) — so the race surfaces as the intended clean 422 with its pg cause chained. A 23505 (duplicate client name) still keeps the generic wrap → 409. Pinned with unit regressions faking a 23503 pg error on the write for both paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
clients.library_id is ON DELETE SET NULL, so deleting a library scans clients for referencing rows; without an index that is a sequential scan. Add clients_library_id_idx in up and drop it in down before the table drop for symmetry. Verified up/down/up round-trips cleanly on a fresh DB. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The prose still read 'at least one of name or sectionNumberFormat', omitting the clientId field the operation now accepts — the handler's own validation message already lists all three. Align the description with the request schema and handler. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CodeRabbit body nitpicks (17:56Z review) — all 4 fixedAll four triaged VALID and fixed. Branch updated from main first (PR #399 merged, so the pre-existing
Verified locally: |
1 participant
Why
The operating hierarchy is Client → Project → Package → Issuance (→ Revision), but SpecR had no
Cliententity — "client" existed only as a library tier (libraries.tier='client') and could only be approximated by inspecting a project's client-tier source libraries. #209/ADR-025 deferred firm/client tables as YAGNI ("firm/client as future links in the resolution chain"). This PR is that future link, now needed: a client (e.g. a hyperscale-datacenter owner) runs many campuses/projects, and grouping, defaults, and custody all want a real edge from a project to the organization that owns it.What
A first-class
clientsentity, exposed over REST and MCP, plus project↔client association. Additive and back-compat.clients(id,nameUNIQUE non-empty,library_id→ libraries ON DELETE SET NULL, timestamps) +projects.client_id→ clients ON DELETE RESTRICT + index.src/db/queries/clients.ts(createClient/listClients/getClient— the latter joins its active projects as fullProjectSummaryrows via a singleLATERAL json_agg).ProjectSummarygainsclientId/clientName;updateProjectgains a validatedclientIdset/clear path (unknown client →ClientNotFoundError→ 422).POST/GET /clients,GET /clients/{id}(client + its projects);PATCH /projects/{id}acceptsclientId(uuid | null — associate/disassociate, echoes it back); dup name → 409, unknown client → 422, unknownlibraryId→ 422.openapi.yamlupdated in lockstep (both contract-gate directions pass).list_clients/get_client(read),create_client(write);update_projecttool gainsclientId. Tools never throw.Delete semantics are governed entirely by the FKs — there is deliberately no
DELETE /clientsendpoint (a client with projects can't be hard-deleted; disassociate first). Non-goals (follow-ups): afirmstier; auth/tenancy (#43); migrating existing scoped profiles onto the client scope.Testing
updateProjectclient path; full unit suite: 1428 pass, only the pre-existingstart-specr-shellfailure inherited from base commit 19343eb)pnpm lint: eslint + tsc + prettier)🤖 Co-authored by Claude Fable 5. Closes #391.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation