refactor(table): move the internal row routes onto the application boundary - #6809
Conversation
The hottest table write path authorized in its own handler and queried the
database from the adapter — the two things a surface adapter must never do. It
now declares itself with defineInternalJsonRoute against the readRow, updateRow
and deleteRow use cases: 127 lines instead of 274, with no db import, no
drizzle import and no checkAccess.
Doing that surfaced why the violation existed. Write-provenance resolution
needs the canonical schema to map a caller's column key to the storage column
it certifies, and the adapter could only do that because it was already loading
the table illegally. The envelope is now split along the real seam: the adapter
reads the header and payload field, which is transport, and the use case
resolves the selections against the canonical table, which is domain.
That split has to preserve a distinction the defaulting logic would erase. An
internal caller that sends no envelope stays deliberately untracked; defaulting
it to an exact-empty stamp would certify "this write introduced no secrets" on
a runtime write that may well have introduced some. Only an interactive caller
certifies exact-empty, over the storage columns its write actually persists.
Two further changes fell out of it:
present() now receives the same { principal, input } pair its sibling hooks
responseHeaders and finalizeResponse already got. This route serves a session
and a workflow execution on one path and owes them different column keyings, so
rendering per caller kind is presentation rather than domain. That was a gap in
the builder, not a special case for this route.
tableRowWireSchema describes what the single-row routes actually return. The
contract claimed a full TableRow, carrying the executions sidecar and Date
objects — true of the list and query routes, and never true here. The hand-
rolled handler was never checked against its own contract, so the drift was
invisible until the builder started validating it.
Wire changes, both deliberate and both narrower than before: a cross-tenant
table now conceals as 404 where the old blanket handler answered 403, while an
in-workspace role denial still answers 403. Nothing in hooks/queries/tables.ts
branches on either.
Verified to fail: forcing one keying, dropping the actor, pre-resolving the
envelope, certifying an untracked internal write, and skipping the bundle
completeness check each turn the covering tests red.
Same shape as the single-row route: declares itself against upsertTableRow, hands the provenance envelope over unresolved, and derives its column keying from the principal rather than assuming one. The keying and presentation helpers the two routes shared are now in row-wire beside the translators they wrap, so a third route does not restate them. Two response details changed on purpose. The row now carries `position`, which the use case always had and this route alone omitted — every other single-row response already returned it, and the contract now describes one shape instead of three. The upsert result also carries read-back provenance, which the route previously assembled for itself. The surface had no route-level tests; it has six now, covering both caller keyings, both operations, and the envelope handover.
…n boundary The last internal adapter that queried the database for itself. It now runs through readTableRowEnrichmentDetail, a new use case that shares tableOperations.readRow — reading a cell's cascade breakdown is a projection of the same row under the same role, not a second semantic operation. Its tests move to the same seam and gain one the old suite could not express: a cross-tenant table now conceals rather than confirming it exists.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Routes now only map input, present per caller kind, and finalize provenance on the response. Secret provenance is split at the real seam: adapters read the wire envelope; Wire / policy (intentional): cross-tenant and mismatched-workspace denials become 404 instead of 403/400; in-workspace role denials stay 403. Unclassified failures use the shared 500 envelope. Upsert responses now include Executor / Table tools: row read/update/delete/upsert operations admit Reviewed by Cursor Bugbot for commit 7f9a1de. Configure here. |
Greptile SummaryThe PR moves internal single-row table routes onto the shared application boundary while preserving caller-specific row keying and secret-provenance handling.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/app/api/table/[tableId]/rows/[rowId]/route.ts | Moves single-row reads, updates, and deletes to shared authenticated application use cases with principal-aware presentation. |
| apps/sim/app/api/table/[tableId]/rows/upsert/route.ts | Migrates row upsert to the shared route builder while forwarding workspace, keying, provenance, and client-attribution inputs. |
| apps/sim/lib/table/application/operations.ts | Assigns the four row-tool operations to policies that admit delegated executor principals. |
| apps/sim/lib/table/application/rows.ts | Implements the application-boundary row operations and centralizes authorization, canonical schema handling, and mutation behavior. |
| apps/sim/app/api/table/table-tool-auth.test.ts | Verifies that every row tool requests executor delegation and that its corresponding operation admits executor principals. |
| apps/sim/tools/table/get_row.ts | Requests executor delegation for workflow-driven row reads. |
| apps/sim/tools/table/update_row.ts | Requests executor delegation for workflow-driven row updates. |
| apps/sim/tools/table/delete_row.ts | Requests executor delegation for workflow-driven row deletion. |
| apps/sim/tools/table/upsert_row.ts | Requests executor delegation for workflow-driven row upserts. |
Sequence Diagram
sequenceDiagram
participant Executor as Workflow Executor
participant Tool as Table Row Tool
participant Route as Internal Row Route
participant Auth as Delegation Auth
participant UseCase as Authorized Row Use Case
participant Table as Table Operations
Executor->>Tool: Execute get/update/delete/upsert
Tool->>Route: Request with executor delegation token
Route->>Auth: Verify token and bind table scope
Auth-->>Route: Delegated executor principal
Route->>UseCase: Principal and mapped row input
UseCase->>Table: Read or mutate authorized row
Table-->>UseCase: Canonical table and row
UseCase-->>Route: Application result
Route-->>Executor: Caller-keyed wire response
Reviews (3): Last reviewed commit: "refactor(table): apply the quality pass" | Re-trigger Greptile
storageKeyByWireKey mapped an unrecognised id-keyed key to null, but rowDataToStorage persists that key when the caller is not writing strictly. A cell would have been written with no provenance recorded, under a stamp still marked complete — the same failure the bundle completeness check exists to prevent, arriving through the keying map instead of the selection set. The two wires genuinely differ and the code now says so: the name path drops an unrecognised key, so it maps to null; the id path stores what it is given, so every key it sends is a storage key. Unreachable today, since no delegated surface uses id keying and a session bundle is refused earlier. Fixed because the function's stated invariant — that it mirrors how the row data itself is normalized — was not true. Also corrects the delegated principal fixture in these tests, which used a kind that is not in the Principal union, so the subject-id branch was never actually exercised. It is now, and the scope check is asserted to receive the acting principal's own subject id. Verified to fail: restoring the schema-based lookup turns the covering test red.
The migration swapped checkSessionOrInternalAuth for the delegation policy, and that broke every Table block call to these endpoints in two ways at once. The old policy accepted a legacy internal token. The new one requires a delegation token, which the executor only mints when the tool asks for it — and none of the four table row tools did, so get/update/delete/upsert row would each have failed with a 401. The knowledge tools already declare it, because their routes migrated first. Even with a valid token the operations denied the caller: readRow, updateRow, deleteRow and upsertRow ran under a policy whose delegatedServices is ['copilot'], so the executor got a 403. They now use the tool-facing policy that already existed for the group operations. Neither was visible to the route tests, which mock the auth policy wholesale — so the gap is closed at the layer that actually decides: one test pinning that each tool requests delegation and that its operation admits the executor, mutation-verified against both failure modes. Also fixes findings from the review pass: the read surfaces no longer load an executions sidecar none of them put on the wire (two readers rather than a flag, so a caller cannot silently read an empty one); the provenance name index is built once per batch instead of once per row; the uniqueness comment now names the concurrent-insert race as well as the retro-added constraint; and the presenter context gets NoInfer plus a note that the v2 builder passes something different.
The rows error policy was built on the concealment base rather than the lock-aware one, so a TableLockedError fell through to the generic handler and the response lost its `lock` field — the only thing that tells a client which lock to clear. A row write is exactly as lockable as a group mutation, so it now shares that base. Also pins the two wire changes the review found undocumented: a mismatched workspace assertion answers 404 rather than 400, which is a superset of the cross-tenant concealment already intended, and an unclassified failure answers the builder's shared "Internal server error" rather than the old per-route text. Both are consistent with the ~80 routes already on this builder; they are asserted so they read as decisions rather than drift. Verified to fail: reverting the policy base turns the lock test red.
Four parallel reviews (reuse, simplification, efficiency, altitude). The highest-value finding cut against the branch's own purpose: the rows error policy sat in the barrel-exported route-policies module, so its import of the 1,200-line row use-case graph was paid by every one of the ~28 table routes that can never throw a row error — the barrel's import cost went from ~1.1s to ~1.7s. row-route-policies.ts already existed for exactly this and is deliberately not re-exported; the policy now lives there with its v2 sibling. The upsert path still loaded an executions sidecar no surface puts on the wire, and did it inside the write transaction, holding it open for a discarded result. The read path got that fix earlier; the write path next to it did not. rowKeyingForPrincipal fell through to name keying for anything that was not a session. The operation policy admits API-key principals, so the first one to reach these routes would have had every id-keyed cell dropped and the write reported as successful. It is now an exhaustive switch over the two kinds the auth policy yields — which immediately failed three tests using a principal kind that exists nowhere in the repo, so those fixtures are real now too. Also: reuses toWireTimestamp and createUnknownTableRowSecretProvenance instead of re-inlining them; shares one helper for the provenance choice the update and upsert use cases both make; keeps one canonical actorClientId doc with two cross-references; merges two maps keyed by the same string into one; stops round-tripping the principal through the legacy AuthType enum; hoists the presenter function type out of both conditional branches; drops a subsumed test; and freezes the shared locks fixture so a mutating test cannot poison its siblings.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 7f9a1de. Configure here.
5e0a2ce
into
refactor/table-rows-application-boundary
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 7f9a1de. Configure here.
…ite paths (#6808) * test(table): characterize the single-row route before migrating it The single-row surface (GET/PATCH/DELETE) had no route-level tests despite being the hottest table write path. Pin the behavior it emits today so the migration onto the shared internal route builder is verifiable rather than hopeful. Covers status codes, body shapes, ISO-8601 timestamp serialization, the access level each method demands, collaborator invocation, and the dual-caller wire keying — session callers pass column ids through untouched while internal-JWT callers translate names to ids in both directions. Verified to fail: mutating the wire translator, the deleted count, and the workspace-ownership guard each turn the corresponding tests red. * feat(table): model wire keying and actor attribution on row write use cases The row write use cases assumed every caller speaks column names. That holds for /api/v2, /api/v1 and the Copilot tools, but not for the first-party grid or the internal /api/table routes, which address cells by stable storage id. Feeding id-keyed data through the name remap drops every key it does not recognise — a storage id names no column name — so the write would store nothing and still report success. Make the wire an explicit, required property of the input rather than an assumption. `dataKeying: 'names' | 'ids'` sits alongside `strictWrite` and is required for the same reason: a new write surface must state which contract it publishes. Strictness now means the same thing on either wire — an unknown column id is refused exactly as an unknown column name already was. Single-row writes also gain optional actor attribution, so the acting tab can skip refetching its own write. It is optional and absent by default, so every existing caller keeps broadcasting to all subscribers as before. Only the single-row create, update and delete paths accept it; a batch write is not reconciled locally by the actor and must still refetch. The attribution pin moves with the behaviour: what selects the actor-scoped signal is no longer which file calls it but which surface supplies an actor, so that is now what the test pins. Verified to fail: ignoring the keying discriminator, and dropping actor attribution, each turn the corresponding tests red. * perf(table): remove two round trips from every single-row update A single-cell PATCH spends far more time in sequential round trips to a remote Postgres than in the UPDATE it issues. Prepared statements are disabled for PgBouncer transaction mode, so every await is a full Parse/Bind/Execute. Two of them were avoidable. getRowById issued the row lookup and its executions sidecar in series, but the sidecar is keyed on the row id the caller already supplied, so it never depended on the lookup. Issuing both together makes it one round trip. A miss now pays one redundant sidecar read, which is the rare path and costs no extra wall time. The uniqueness probe ran whenever the table had any unique column, passing the fully merged row, so editing an unrelated cell re-probed every unique column — its own transaction plus one query per column. It is now scoped to the columns the patch actually writes. A merge cannot newly violate uniqueness on a column it leaves alone: that value is the one already stored, and it satisfied the constraint when it was written. Sized before changing: few tables declare a unique column, but write traffic concentrates in the ones that do, so this is the larger of the two savings in practice. Verified to fail: reverting the probe scoping turns the covering test red. * perf(table): start the workspace load with the table load when a workspace is asserted resolveActiveTableContext ran two sequential round trips: load the table, then load the workspace it turned out to live in. When the caller asserts a workspace the second input is already in hand, so both can start together. What makes that safe is unchanged: requireTable still compares the table's canonical workspaceId against the assertion and reports a mismatch as not_found. The table outcome is inspected first and unconditionally, so any path that returns has proven the assertion equal to the canonical id, and a failing workspace load can never replace the concealing not_found. A final identity check on the loaded context restates the invariant at the point of return, so even with the first check removed the function cannot hand back a foreign workspace. Promise.allSettled keeps the discarded branch from surfacing as an unhandled rejection. With no asserted workspace the path stays sequential — the table load is what reveals which workspace to load, so there is nothing to start early. One existing assertion changed: it required the workspace load not to have been issued yet on a mismatched assertion, which is internal sequencing rather than caller-observable behaviour and is definitionally untrue once the loads overlap. The observable half is kept, and two timing tests now cover the sequencing directly. Verified to fail: removing either mismatch check, reading the workspace outcome first, and swapping allSettled for bare awaits each turn the corresponding tests red. * perf(table): read a table and its latest job in one round trip getTableById issued the table SELECT and then awaited latestJobForTable, so every table request paid two sequential round trips. With prepared statements disabled for PgBouncer transaction mode every await is a full round trip, and this loader is on essentially every table route. The job read cannot be skipped: a table's reported rowCount is the stored count minus the job's pendingDeleteRemaining, so dropping it would overstate the count during a pending delete and could wrongly reject inserts as over capacity. An opt-out flag would have made that a caller's trap. Instead the job is read in the same statement, as a correlated jsonb subquery in the select list — the select-list form of a LEFT JOIN LATERAL, which is what drizzle can type here. Output is unchanged for every input. latestJobForTable is deleted rather than left dangling: getTableById was its only caller, and keeping it would have carried a third copy of the exports-excluded / newest-started_at / limit-one rule. mapJobRow is now exported so the batch path and the lateral share one implementation of the doomedCount and pendingDeleteRemaining logic. The batch DISTINCT ON path used by the list endpoint is untouched. Verified to fail: dropping the export filter, reversing or re-keying the sort, dropping the limit, dropping the correlation, loosening either doomedCount condition, and removing the rowCount subtraction each turn tests red. Dropping the lateral from the projection initially survived, because the shared db mock returns queued rows regardless of predicate; a projection assertion now covers it. * chore(table): drop the unused single-row GET contract Authored as groundwork for migrating the internal row routes onto the shared builder, which is not in this change. An exported contract nothing consumes is dead code, so it lands with the migration that needs it instead. * fix(table): resolve strict id-keyed columns through getColumnId assertKnownColumnIds read column.id directly, but a column id is optional — pre-backfill columns have none and are stored under their name, which is why getColumnId exists and is what every other consumer of the schema uses. Such columns still exist, so a strict id-keyed write naming one would have been refused as unknown. Latent today: no surface yet combines dataKeying 'ids' with strictWrite. Fixed before one does. Verified to fail: reading column.id turns the covering test red. * refactor(table): apply review findings from the quality pass Four parallel reviews (reuse, simplification, efficiency, altitude) converged on the same set. Applied: Removed actorClientId entirely. It had no supplier anywhere in the repo, so every call reached signalTableRowsChangedByActor(id, undefined), which is byte-identical to the broadcast it replaced — three optional fields, three verbatim doc blocks and a pin test asserting the empty set, all inert. It belongs with the route migration that supplies an actor. The attribution pin is restored to its original form. The uniqueness probe was only half-narrowed: the patched column list was computed and then discarded, and the probe re-derived every unique column from the full schema. It now receives only the columns the patch touched, so a table with several unique columns runs one query instead of all of them. assertKnownColumnIds hand-rolled an id index and duplicated the sibling assert's message verbatim. It now reuses buildColumnNameById — which already keys by getColumnId, so the legacy pre-backfill column case is handled by the shared helper rather than by a special case here — and both asserts share one message builder. The job field list had become two copies, one drizzle-checked and one an unchecked sql<T> cast that could silently return undefined for a renamed field. The lateral now derives its jsonb pairs from JOB_PROJECTION, which satisfies Record<keyof LatestJobRow, Column>. That compile-time guarantee replaces the runtime drift test it makes redundant. Also: hoisted the id index out of the batch loop to match the names path, dropped a never-supplied parameter, narrowed an over-broad parameter type, removed a dead timer cleanup and its now-unused import, replaced dynamic re-imports with the static one already present, hoisted a repeated stub, and documented why filters need no keying counterpart and how laxness differs between the two wires. Verified to fail: removing a field from JOB_PROJECTION breaks the build in two places. * test(table): share one table-definition fixture factory buildTable was copy-pasted into 13 route test files under app/api/table, each a near-identical TableDefinition literal. A required field added to that type would have failed 13 files individually. packages/testing already owned createTableColumn and createTableRow but no definition factory, and — as it turns out — did not export any of them from the barrel, so they were unreachable from @sim/testing. Adds createTableDefinition beside them and exports all three. The fixture type is a structural stand-in rather than TableDefinition itself, because packages/* must not import from apps/* — the same approach the existing factories in that file already take. No assertion changed. Call sites that varied a field pass it as an override; the four files with several call sites hoist a shared options const and spread it so each call still gets a fresh object. * perf(table): project only the job field the row count needs Both job reads selected the whole payload jsonb, but mapJobRow reads exactly one number out of it, and only for a running delete. The payload also carries the delete job's filter and an unbounded excludeRowIds array, and the latest non-export job is read on essentially every table request — a table that once ran a large delete would ship that id list on every read, forever. LatestJobRow.payload becomes doomedCount, extracted in SQL. Both readers share JOB_PROJECTION so one edit reaches the batch DISTINCT ON and the correlated subquery alike; the compile-time constraint widens to Column | SQL rather than being dropped. Behaviour is identical. `->` keeps the value jsonb, which postgres-js decodes through its built-in JSON.parse handler, so it arrives as a number with no boundary coercion. A null payload, a payload without the key, a non-object payload and an explicit JSON null all collapse to the same `?? 0` the previous optional chain produced. Sized honestly before claiming a win: payloads are small in practice today, so this is defensive rather than impactful — it removes an unbounded growth path, not a measured cost. Verified against a real Postgres, not just the mocked driver: the generated correlated subquery returns doomedCount 12 for a delete job, null for an import job, and a null row for a table with no job. Also from the review pass: re-homes the strictWrite explanation onto rowWriteOptions, where six {@link} references now point; records why replaceProjectedWireRows carries no keying discriminator; notes the one case the uniqueness-narrowing invariant does not cover; pins the lax id-wire passthrough with a test; and renames a parameter that misled once only its keys were read. * chore(table): drop two comments the code already says One restated the identifier below it (checkUniqueConstraintsDb), and one was a section-divider banner in the factories barrel, which the repo's comment convention rules out. Everything else that survives is a why the code cannot express: round-trip rationale, sql.raw input safety, the narrowing invariant and the one case it does not cover. * refactor(table): move the internal row routes onto the application boundary (#6809) * refactor(table): move the single-row route onto the application boundary The hottest table write path authorized in its own handler and queried the database from the adapter — the two things a surface adapter must never do. It now declares itself with defineInternalJsonRoute against the readRow, updateRow and deleteRow use cases: 127 lines instead of 274, with no db import, no drizzle import and no checkAccess. Doing that surfaced why the violation existed. Write-provenance resolution needs the canonical schema to map a caller's column key to the storage column it certifies, and the adapter could only do that because it was already loading the table illegally. The envelope is now split along the real seam: the adapter reads the header and payload field, which is transport, and the use case resolves the selections against the canonical table, which is domain. That split has to preserve a distinction the defaulting logic would erase. An internal caller that sends no envelope stays deliberately untracked; defaulting it to an exact-empty stamp would certify "this write introduced no secrets" on a runtime write that may well have introduced some. Only an interactive caller certifies exact-empty, over the storage columns its write actually persists. Two further changes fell out of it: present() now receives the same { principal, input } pair its sibling hooks responseHeaders and finalizeResponse already got. This route serves a session and a workflow execution on one path and owes them different column keyings, so rendering per caller kind is presentation rather than domain. That was a gap in the builder, not a special case for this route. tableRowWireSchema describes what the single-row routes actually return. The contract claimed a full TableRow, carrying the executions sidecar and Date objects — true of the list and query routes, and never true here. The hand- rolled handler was never checked against its own contract, so the drift was invisible until the builder started validating it. Wire changes, both deliberate and both narrower than before: a cross-tenant table now conceals as 404 where the old blanket handler answered 403, while an in-workspace role denial still answers 403. Nothing in hooks/queries/tables.ts branches on either. Verified to fail: forcing one keying, dropping the actor, pre-resolving the envelope, certifying an untracked internal write, and skipping the bundle completeness check each turn the covering tests red. * refactor(table): move the upsert route onto the application boundary Same shape as the single-row route: declares itself against upsertTableRow, hands the provenance envelope over unresolved, and derives its column keying from the principal rather than assuming one. The keying and presentation helpers the two routes shared are now in row-wire beside the translators they wrap, so a third route does not restate them. Two response details changed on purpose. The row now carries `position`, which the use case always had and this route alone omitted — every other single-row response already returned it, and the contract now describes one shape instead of three. The upsert result also carries read-back provenance, which the route previously assembled for itself. The surface had no route-level tests; it has six now, covering both caller keyings, both operations, and the envelope handover. * refactor(table): move the enrichment-detail route onto the application boundary The last internal adapter that queried the database for itself. It now runs through readTableRowEnrichmentDetail, a new use case that shares tableOperations.readRow — reading a cell's cascade breakdown is a projection of the same row under the same role, not a second semantic operation. Its tests move to the same seam and gain one the old suite could not express: a cross-tenant table now conceals rather than confirming it exists. * fix(table): mirror the storage rule when keying write provenance storageKeyByWireKey mapped an unrecognised id-keyed key to null, but rowDataToStorage persists that key when the caller is not writing strictly. A cell would have been written with no provenance recorded, under a stamp still marked complete — the same failure the bundle completeness check exists to prevent, arriving through the keying map instead of the selection set. The two wires genuinely differ and the code now says so: the name path drops an unrecognised key, so it maps to null; the id path stores what it is given, so every key it sends is a storage key. Unreachable today, since no delegated surface uses id keying and a session bundle is refused earlier. Fixed because the function's stated invariant — that it mirrors how the row data itself is normalized — was not true. Also corrects the delegated principal fixture in these tests, which used a kind that is not in the Principal union, so the subject-id branch was never actually exercised. It is now, and the scope check is asserted to receive the acting principal's own subject id. Verified to fail: restoring the schema-based lookup turns the covering test red. * fix(table): restore executor access to the migrated row routes The migration swapped checkSessionOrInternalAuth for the delegation policy, and that broke every Table block call to these endpoints in two ways at once. The old policy accepted a legacy internal token. The new one requires a delegation token, which the executor only mints when the tool asks for it — and none of the four table row tools did, so get/update/delete/upsert row would each have failed with a 401. The knowledge tools already declare it, because their routes migrated first. Even with a valid token the operations denied the caller: readRow, updateRow, deleteRow and upsertRow ran under a policy whose delegatedServices is ['copilot'], so the executor got a 403. They now use the tool-facing policy that already existed for the group operations. Neither was visible to the route tests, which mock the auth policy wholesale — so the gap is closed at the layer that actually decides: one test pinning that each tool requests delegation and that its operation admits the executor, mutation-verified against both failure modes. Also fixes findings from the review pass: the read surfaces no longer load an executions sidecar none of them put on the wire (two readers rather than a flag, so a caller cannot silently read an empty one); the provenance name index is built once per batch instead of once per row; the uniqueness comment now names the concurrent-insert race as well as the retro-added constraint; and the presenter context gets NoInfer plus a note that the v2 builder passes something different. * fix(table): keep the lock on a 423 and pin the remaining wire changes The rows error policy was built on the concealment base rather than the lock-aware one, so a TableLockedError fell through to the generic handler and the response lost its `lock` field — the only thing that tells a client which lock to clear. A row write is exactly as lockable as a group mutation, so it now shares that base. Also pins the two wire changes the review found undocumented: a mismatched workspace assertion answers 404 rather than 400, which is a superset of the cross-tenant concealment already intended, and an unclassified failure answers the builder's shared "Internal server error" rather than the old per-route text. Both are consistent with the ~80 routes already on this builder; they are asserted so they read as decisions rather than drift. Verified to fail: reverting the policy base turns the lock test red. * refactor(table): apply the quality pass Four parallel reviews (reuse, simplification, efficiency, altitude). The highest-value finding cut against the branch's own purpose: the rows error policy sat in the barrel-exported route-policies module, so its import of the 1,200-line row use-case graph was paid by every one of the ~28 table routes that can never throw a row error — the barrel's import cost went from ~1.1s to ~1.7s. row-route-policies.ts already existed for exactly this and is deliberately not re-exported; the policy now lives there with its v2 sibling. The upsert path still loaded an executions sidecar no surface puts on the wire, and did it inside the write transaction, holding it open for a discarded result. The read path got that fix earlier; the write path next to it did not. rowKeyingForPrincipal fell through to name keying for anything that was not a session. The operation policy admits API-key principals, so the first one to reach these routes would have had every id-keyed cell dropped and the write reported as successful. It is now an exhaustive switch over the two kinds the auth policy yields — which immediately failed three tests using a principal kind that exists nowhere in the repo, so those fixtures are real now too. Also: reuses toWireTimestamp and createUnknownTableRowSecretProvenance instead of re-inlining them; shares one helper for the provenance choice the update and upsert use cases both make; keeps one canonical actorClientId doc with two cross-references; merges two maps keyed by the same string into one; stops round-tripping the principal through the legacy AuthType enum; hoists the presenter function type out of both conditional branches; drops a subsumed test; and freezes the shared locks fixture so a mutating test cannot poison its siblings.
Summary
Stacked on #6808. Moves the internal table row routes onto the application boundary — the routes that authorized in their own handlers and queried the database from the adapter.
rows/[rowId](GET/PATCH/DELETE) is 127 lines instead of 274, with no@sim/db, no drizzle and nocheckAccess.rows/upsertand the enrichment-detail route follow the same shape.present()now receives the same{ principal, input }its sibling hooksresponseHeadersandfinalizeResponsealready got. These routes serve a session and a workflow execution on one path and owe them different column keyings, so rendering per caller kind is presentation, not domain — a gap in the builder rather than a special case for one route.tableRowWireSchemadescribes what the single-row routes actually return. The contract claimed a fullTableRowwith the executions sidecar andDateobjects — true of the list route, never true here. The hand-rolled handlers were never validated against their own contracts, so the drift was invisible.readTableRowEnrichmentDetail, sharingtableOperations.readRowsince it is a projection of the same row under the same role.Deliberate wire changes
Both follow from adopting the shared concealment policy, and both are narrower than what they replace: a cross-tenant table conceals as 404 where the old blanket handler answered 403, while an in-workspace role denial still answers 403. The upsert row now carries
position, which every other single-row response already returned. Nothing inhooks/queries/tables.tsbranches on either status.Not in this PR
v1/tables/[tableId]/rows/[rowId]is the last direct-DB adapter. It resolves its actor throughresolveWorkspaceRequestActorrather than the canonicalresolvePrincipalAttributionthe use cases use, so migrating it changes billing attribution for workspace API keys. That deserves its own analysis rather than being folded in here.rows,queryandrows/findare also still on the legacy pattern.Wire changes
Beyond the refactor, four intended differences on the three migrated routes:
404 Table not foundinstead of403. In-workspace role denials still answer403. This matches what v2 and the ~80 other routes on the shared builder already do, and it stops the internal surface confirming a table's existence to a non-member.404rather than400.500 Internal server errorrather than leaking the message.position, which the siblingGET/PATCHalready emitted.No client branches on any of the changed statuses or strings —
hooks/queries/tables.tsinspects only423for the lock.Type of Change
Testing
Tested manually. 2,785 tests pass,
type-checkclean, all 29 audits pass. Every behavioural change is mutation-tested — forcing one keying, dropping the actor, pre-resolving the envelope, certifying an untracked internal write, and skipping the bundle completeness check each turn the covering tests red. The upsert and enrichment surfaces had no route-level tests before; they do now.Checklist