Skip to content

refactor(table): move the internal row routes onto the application boundary - #6809

Merged
waleedlatif1 merged 7 commits into
refactor/table-rows-application-boundaryfrom
refactor/table-internal-row-route-boundary
Aug 18, 2026
Merged

refactor(table): move the internal row routes onto the application boundary#6809
waleedlatif1 merged 7 commits into
refactor/table-rows-application-boundaryfrom
refactor/table-internal-row-route-boundary

Conversation

@waleedlatif1

@waleedlatif1 waleedlatif1 commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

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 no checkAccess. rows/upsert and the enrichment-detail route follow the same shape.
  • Both internal direct-DB adapters are now closed. Only the v1 one remains (see below).
  • Migrating 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 now splits on the real seam — the adapter reads the header and payload field (transport), the use case resolves the selections against the canonical table (domain).
  • That split preserves a distinction the defaulting logic would erase: an internal caller that sends no envelope stays deliberately untracked. Defaulting it to exact-empty would certify "this write introduced no secrets" on a runtime write that may well have introduced some.
  • present() now receives the same { principal, input } its sibling hooks responseHeaders and finalizeResponse already 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.
  • tableRowWireSchema describes what the single-row routes actually return. The contract claimed a full TableRow with the executions sidecar and Date objects — true of the list route, never true here. The hand-rolled handlers were never validated against their own contracts, so the drift was invisible.
  • New use case readTableRowEnrichmentDetail, sharing tableOperations.readRow since 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 in hooks/queries/tables.ts branches on either status.

Not in this PR

v1/tables/[tableId]/rows/[rowId] is the last direct-DB adapter. It resolves its actor through resolveWorkspaceRequestActor rather than the canonical resolvePrincipalAttribution the 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, query and rows/find are also still on the legacy pattern.

Wire changes

Beyond the refactor, four intended differences on the three migrated routes:

  • Cross-tenant denial is now concealed as 404 Table not found instead of 403. In-workspace role denials still answer 403. 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.
  • Workspace mismatch answers 404 rather than 400.
  • An unclassified failure answers 500 Internal server error rather than leaking the message.
  • The upsert response now carries position, which the sibling GET/PATCH already emitted.

No client branches on any of the changed statuses or strings — hooks/queries/tables.ts inspects only 423 for the lock.

Type of Change

  • Refactor
  • Bug fix (contract drift, provenance certification)

Testing

Tested manually. 2,785 tests pass, type-check clean, 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

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

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.
@vercel

vercel Bot commented Aug 18, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Aug 18, 2026 5:38am

Request Review

@cursor

cursor Bot commented Aug 18, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Touches authorization concealment, secret provenance certification, and workflow executor auth on core row APIs; wire status/message changes are deliberate but affect all callers of these endpoints.

Overview
Moves the internal single-row, upsert, and enrichment-detail table routes off hand-rolled handlers (direct DB, checkAccess, inline provenance) onto defineInternalJsonRoute and the existing row use cases.

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; resolveRowWriteProvenance maps selections against the canonical schema inside the use case, preserving untacked internal writes when no envelope is sent (vs falsely certifying exact-empty).

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 position. Contracts align with tableRowWireSchema (narrow row projection, ISO timestamps, no executions sidecar).

Executor / Table tools: row read/update/delete/upsert operations admit executor delegation; tools request executor_delegation tokens. present() on the internal JSON builder receives { principal, input } for session (column ids) vs delegated (column names) rendering. Reads/upserts use getRowSummaryById where the executions sidecar is not returned.

Reviewed by Cursor Bugbot for commit 7f9a1de. Configure here.

@greptile-apps

greptile-apps Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR moves internal single-row table routes onto the shared application boundary while preserving caller-specific row keying and secret-provenance handling.

  • Replaces direct database access in the GET, PATCH, DELETE, upsert, and enrichment-detail adapters with authorized table use cases.
  • Adds executor delegation metadata to all four row tools and admits executor principals through the corresponding operation policies.
  • Aligns single-row wire contracts, error projection, attribution, and route-level tests with the shared route builder.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

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
Loading

Reviews (3): Last reviewed commit: "refactor(table): apply the quality pass" | Re-trigger Greptile

Comment thread apps/sim/app/api/table/[tableId]/rows/upsert/route.ts
Comment thread apps/sim/lib/table/api/route-policies.ts Outdated
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.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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.

@waleedlatif1
waleedlatif1 merged commit 5e0a2ce into refactor/table-rows-application-boundary Aug 18, 2026
5 checks passed
@waleedlatif1
waleedlatif1 deleted the refactor/table-internal-row-route-boundary branch August 18, 2026 05:51
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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.

waleedlatif1 added a commit that referenced this pull request Aug 18, 2026
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant