Skip to content

Commit 650933e

Browse files
fix(runtime)!: the dispatcher's PATCH /data/:object/:id honours a body expectedVersion and If-Match, as RestServer's twin does (#23490)
Fixes #23462 Clause-②: no (narrowing) The runtime dispatcher's `PATCH /data/:object/:id` now reads the optimistic-concurrency token the way `RestServer`'s twin reads it, strips it from the patch, and forwards it to `updateData`. A stale update through the dispatcher is refused instead of applied, and a body `expectedVersion` is no longer handed to the engine as a field. This is the `PATCH` position of the family whose `DELETE` position landed as PR #23460 (`3d231ba787`), and it mirrors that landing's shapes. Dispatched by the `domain:cli` seat 2 PM (claim `6167715147`, branch `claude/issue-23462-dispatcher-patch-version`), session `session_01Q9PZ5Pzqcus4woVvTZwWNU`. ## What was wrong `RestServer` (`packages/rest/src/rest-server.ts:9014-9063`, read only) does three things: - it reads the body's `expectedVersion`, else `If-Match`; - it strips `expectedVersion` out of the patch, whatever its value; - it hands `String(token)` to `updateData` when the token is truthy. The protocol then refuses an update whose record has moved (`409 CONCURRENT_UPDATE`). The dispatcher dropped the token twice: 1. `domains/data.ts`, the `PATCH` branch, read neither and passed the whole body as `data`. 2. `action-execution.ts`, `callData`'s update arm, forwarded only `{ object, id, data }` to `updateData`. Measured before the fix (the pin file on unfixed source, `94f0cf085e`), through `dispatch()`: - A stale `If-Match` answered `200` and wrote the row. - A body `expectedVersion` was refused `400 INVALID_FIELD` ("Unknown field 'expectedVersion' on object 'task'"). The `createHonoApp` catch-all renders that as the `500` the card measured. ## What changes - **`domains/data.ts`, the `PATCH` branch.** - Reads the body's `expectedVersion` (when the body is an object), else `If-Match`, through the private `requestIfMatch` reader PR #23460 added. There is no second spelling of it. - Strips the key from the patch when present, whatever its value. - Forwards `String(token)` only when the token is truthy. - These are REST's own statements, so both doors send `updateData` the same request. - **`action-execution.ts`, `callData`'s update arm.** Forwards `params.expectedVersion` to `updateData` when present. Every other caller (`mcp.ts`, the declarative update action, the endpoint executor) sends `{ object, id, data }` and reaches `updateData` as before. - **The ObjectQL fallback.** A token sent to a host whose protocol has no `updateData` is refused `501 NOT_IMPLEMENTED` before anything is read or written, in the delete arm's shape. An update with no token reaches the fallback as before. - **The changeset**, `.changeset/23462-dispatcher-patch-expected-version.md`, follows PR #23460's shape: - `@objectstack/runtime` `minor`, `fix(runtime)!:`, FROM → TO; - one ADR-0087 marker, `not-required (no-migration-prescription)`; - the launch-window BREAKING line. It says a `createHonoApp` host still renders the refusal as `500` with the record unchanged. It promises no wire `409` there. ## Pins The new file `packages/runtime/src/domains/data-update-expected-version.test.ts` is the `PATCH` sibling of `data-delete-expected-version.test.ts`, on the same rig: - a real better-sqlite3 store, the real `ObjectQL` engine and a real `ObjectStackProtocolImplementation`; - `RestServer`'s registered handler and `HttpDispatcher.dispatch()` side by side. The dispatch side repeats the catch-all's statements over a real Fetch `Request`, with the body parsed from a clone of its text. Every row reads the store after the request. - **Per door, 5 rows each:** - no token updates, and `updateData` receives no token; - the current body token updates, and the patch handed over carries no `expectedVersion` key; - a stale body token is refused `409 CONCURRENT_UPDATE`, and the row is unchanged; - `If-Match` behaves like the body token, both current and stale; - the body token is judged before `If-Match`. - **Side by side, 6 token shapes plus the Node header record.** The shapes are: a stale token; a number token; `null` beside a stale `If-Match`; `null` beside the current one; `''` beside a stale `If-Match`; a body that is only the current token. For each, the two doors must agree on status, code and the row, and the key never reaches the engine as a field. - **The fallback, 3 rows.** A body token and `If-Match` each get `501 NOT_IMPLEMENTED`, with neither `find` nor `update` called. An unguarded update still reaches the fallback. How a refusal is read: as on `DELETE`, `/data` has no local catch. So the conflict is the protocol's `ConcurrentUpdateError`, thrown out of `dispatch()`. It is read through `resolveThrownHttpError` (`@objectstack/types`) and set beside what `RestServer` answered through `mapDataError`. **Before and after.** - **Before** (unfixed source, `94f0cf085e`): `Tests 13 failed | 7 passed (20)`. Every `RestServer` row and both controls were green. Every catch-all token row and both `501` rows were red. - **After** (`19d1b8dfdd`), run with the landed `DELETE` file: `Test Files 2 passed (2)`, `Tests 40 passed (40)`. **Ablations.** Each ran through `node scripts/ablation-replace.mjs` in WRAP mode, with a restore trap in the driver. - Each anchor hit once, and `mutation landed` was printed. - Each restore was proven by the blob hash equalling HEAD's and an empty `git diff HEAD`. - The expected direction was red each time, and each went red. - The pins import `../http-dispatcher.js` relatively, so the mutated source is what ran, and no `dist` was involved. The four readings: - **A**, the `data.ts` forward dropped: `10 failed | 10 passed`. - Red: the catch-all token rows; the side-by-side stale, number and null-beside-stale rows; the header-record row; both `501` rows, because no token reaches `callData`. - Green: every `RestServer` row, both controls, and the three side-by-side rows that do not depend on the forward. - A first attempt was refused by the tool itself before anything ran: the replacement `data,` was a prefix of the anchor, so its count could not rise. It measured nothing, and it was redone with a marker comment. - **B**, the `callData` update-arm forward dropped: `8 failed | 12 passed`. The same token rows went red. Both `501` rows stayed green, because the refusal is its own statement. - **C**, the fallback refusal disabled (`if (false)`): `2 failed | 18 passed`, the two `501` rows. - **D**, the strip dropped: `9 failed | 11 passed`. This one goes beyond the dispatch's three, for the card's "a body token is never written as a field". Every body-token row went red, and the `If-Match`-only rows stayed green. ## Mechanism readings (the dispatch's hypotheses) - **(a) Confirmed.** REST's twin is at `rest-server.ts:9014-9063`, as described. It is mirrored, and no `@objectstack/rest` export is added. - **(b) Confirmed.** `requestIfMatch` is reused, and so are the delete arm's forward and its `501` shape. There is no second spelling of the reader. - **(c) Measured on both doors**, in a scratch rig that is not committed. - On every token shape, the doors now agree: - a number or object token is stringified and judged (`409` when stale); - `null` falls through to `If-Match`; - `false` or `''` is stripped and forwarded as no token, so `If-Match` is not read (REST's `??`, then its truthy test); - the quoted-empty `""` is refused `400 VALIDATION_FAILED` by the protocol, on both doors. - On a body that is NOT a record, the doors still differ, and not about the token: - `RestServer`'s `UpdateDataRequestSchema` gate answers `400 VALIDATION_FAILED` for an array, string or number body, and `200` for `null` (patched as `{}`); - the dispatcher answers `500` (the engine's own error), with nothing written. - That divergence predates this change: the dispatcher never had REST's body-schema gate. It is a different defect class from this card, so it is reported as a finding, not fixed here. - **(d) Confirmed.** - Through `dispatch()`, a stale update throws `ConcurrentUpdateError`, read as `409 / CONCURRENT_UPDATE`. - Through a real `createHonoApp`, a stale body token and a stale `If-Match` each answer `500 INTERNAL_ERROR` with the conflict sentence, and the row is unchanged. The current token and no token answer `200` and update. This was a scratch run, not committed: the adapter source over the runtime `dist` built at `19d1b8dfdd`. - The `500` comes from the catch-all rendering from `statusCode` alone. #23365 remains open for it, and nothing there is edited here. - **(e) No `hono-*` pin added, so no surface supplement is needed.** - The two hand-offs a `PATCH` token rides are already pinned at the adapter: - `hono.test.ts` ("PATCH /api/data/account parses JSON body") pins that the parsed body reaches `dispatch()`; - `hono-catch-all-raw-request.test.ts` pins the raw `Request`, where `If-Match` rides, for `PUT`/`PATCH`. - The domain's reading of both is pinned here, through `dispatch()`. - The one fact a composed-host pin would add is the wire status, which today is #23365's `500`. Pinning it would pin that defect. - The end-to-end reading above was taken instead, as PR #23460 did. ## Gates (final head `75d46f1034`) This head merges `origin/main` at `1baf976cf9` (#23458, #23466, #23464). The incoming commits touched nothing on this path. They touched: - `metadata-protocol`'s `dropStorage` delete; - the Hono adapter's repeated-query handling; - `packages/runtime/src/domains/packages.ts` and `sandbox/*`; - `packages/spec` (the hook schema). After the merge: `pnpm install --frozen-lockfile`, `rm -rf packages/runtime/.objectstack`, and the runtime closure rebuilt (`pnpm --filter '@objectstack/runtime...' build`, VERDICT exit 0). - **Typecheck.** `pnpm --filter @objectstack/runtime run typecheck` exits 0. That covers `tsc --noEmit` and `check:test-typecheck`: 27 files, 190 errors, 68 pinned signatures held. The new test is compiled by `tsconfig.test.json` (`--listFilesOnly`: 1 hit) and excluded from `tsconfig.json` (0 hits). - **Tests.** `vitest run --project local --maxWorkers=2` on `@objectstack/runtime`: `Test Files 377 passed (377)`, `Tests 5507 passed | 19 skipped (5526)`. - **Derived gates.** `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` gives 65 commands. All ran at this head, each exit code captured before any pipe: - 64 exit 0; - `pnpm check:dual-build-cjs-loads` exits 3, `PREREQUISITE NOT MET`, because it reads a whole-repo `pnpm build`. It is NOT MEASURED locally, and CI runs it. The `--ran` reconciliation reads `65 derived, 64 run, 1 NOT-MEASURED (derived from a recorded exit 3), 0 UNRUN`. - **Roster gates.** The five whose roster sits under a touched directory also ran, each exit 0: `check-changeset-fixed`, `check:authz-resolver`, `check:error-code-casing`, `check:filter-alias-parity`, `check:route-ledger-census`. - **`pnpm lint`, as a declared narrowing.** The evidence: 1. The diff touches no eslint config or ignore file. 2. `pnpm exec eslint --no-inline-config`, with the repo config, over the three changed source files exits 0 with empty output. 3. The config is untyped and per-file. `eslint.config.mjs` says it "never enables type-aware linting", and `data.ts` has 6 active rules, none cross-file. So no unchanged file's verdict can move with this diff. CI runs the full `eslint .`. ## Acceptance notes Observations, not filed: - A `?expectedVersion` query on a `PATCH` is read by neither door: both apply a stale update sent that way. The spec declares the `PATCH` token as the body field or `If-Match`, so this is an undeclared spelling. - The declared-endpoint door (`endpoint-executor.ts`, `object_operation: update`) also hands `ctx.body` over as `data` and reads no token. This is a read-only inference, not measured. Two findings are handed to the seat for filing, in the `os-dev-report` comment on #23462: - the non-record body divergence described under (c); - an empty or `false` body token that silences a stale `If-Match`, on both doors. --- _Generated by [Claude Code](https://claude.ai/code/session_01Q9PZ5Pzqcus4woVvTZwWNU)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 8693110 commit 650933e

4 files changed

Lines changed: 445 additions & 2 deletions

File tree

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
---
2+
'@objectstack/runtime': minor
3+
---
4+
5+
fix(runtime)!: the runtime dispatcher's `PATCH /data/:object/:id` honours a body `expectedVersion` and `If-Match` as its `RestServer` twin does, so an update carrying a stale version is refused instead of applied
6+
7+
`RestServer`'s `PATCH /data/:object/:id` reads an optimistic-concurrency token from the body's `expectedVersion`, else from the `If-Match` header. It strips `expectedVersion` out of the patch and hands the token to the protocol's `updateData`, which refuses an update whose record has moved past it. The runtime dispatcher's `/data` domain read neither and handed the whole body over as the patch, and its shared `callData` update arm forwarded no token. So on a host whose `/data` routes the dispatcher serves (a `createHonoApp` host), an update carrying a stale `If-Match` wrote the record and answered `200`, the same answer an update carrying no token gets. A body `expectedVersion` reached the engine as a field and was refused as an unknown one. The domain now reads the token the way `RestServer` does, and `callData` forwards it to `updateData`.
8+
9+
Clause-②: no (narrowing)
10+
11+
<!-- adr-0087: not-required (no-migration-prescription) No metadata moves: no spec key, authorable spelling, object definition or stored shape is added, removed, renamed or re-shaped, and no exported declaration is added, removed or narrowed, so there is nothing for `objectstack migrate meta` to rewrite. What narrows is the accept set of one runtime dispatcher route: an update whose version token is stale, or sent to a host that cannot judge it, is now refused where it was applied, and the remedy is in the request a caller sends, never a rewrite of a package source or a stored row. The other categories are closed on facts: the package publishes (not unpublished); no ADR-0087 id is named or touched (not registered or already-registered); and nothing exported is removed or narrowed (not runtime-interface-only, type-surface-only or runtime-export-retired). -->
12+
13+
**BREAKING** (an accept-set narrowing), shipped as `minor` under the launch-window convention for breaking changes.
14+
15+
What changes for you, wherever the runtime dispatcher serves `PATCH /data/:object/:id`. Where `@objectstack/rest` is mounted it serves this route itself and already answered each of these, so the standalone server's answers do not move.
16+
17+
- FROM an update with a stale `If-Match` that wrote the record and answered `200`, TO a refusal with the record unchanged: the protocol's `409 CONCURRENT_UPDATE`, thrown out of `dispatch()` for the host's exit to render, as every other refusal `updateData` throws is. A `@objectstack/hono` `createHonoApp` host still renders that refusal as `500` (the record is unchanged): its catch-all reads only an error's `statusCode`, and rendering the declared `409` is a separate fix.
18+
- FROM a body `expectedVersion` that reached the engine as a field and was refused as unknown (`Unknown field 'expectedVersion'`, nothing written), TO the update's version token. It is stripped from the patch and never written as a field. The update is applied when the token names the current version and refused as above when it is stale. A body `expectedVersion` is read before `If-Match`, in `RestServer`'s order.
19+
- FROM an update carrying a token on a host whose protocol has no `updateData` (the ObjectQL fallback), which updated unguarded, TO `501 NOT_IMPLEMENTED`, before anything is read or written. An update with no token reaches the fallback as before.
20+
21+
To update over a version you read, send that version (the record's `updated_at`) as the body's `expectedVersion` or as `If-Match`. An update with no token is unchanged. The `@objectstack/client` SDK's `data.update(object, id, data)` sends `data` as the body, so an SDK caller whose `data` carries `expectedVersion` now gets, through the dispatcher, the guard it asked for.

‎packages/runtime/src/action-execution.ts‎

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -364,8 +364,34 @@ export async function callData(deps: ActionExecutionDeps,
364364
}
365365

366366
if (action === 'update') {
367+
// [#23462] `expectedVersion` rides through to `updateData`, whose
368+
// version check refuses a stale update. This arm forwarded only
369+
// `{ object, id, data }`, so a token the `/data` domain read was
370+
// dropped here a second time. Present only when a caller sent one:
371+
// every other caller hands `{ object, id, data }` and reaches
372+
// `updateData` as before. The `delete` arm below does the same.
373+
const expectedVersion = params?.expectedVersion;
367374
if (protocol && typeof protocol.updateData === 'function') {
368-
return await protocol.updateData({ object: params.object, id: params.id, data: params.data, ...(scopeId ? { environmentId: scopeId } : {}), context: executionContext });
375+
return await protocol.updateData({
376+
object: params.object,
377+
id: params.id,
378+
data: params.data,
379+
...(expectedVersion !== undefined ? { expectedVersion } : {}),
380+
...(scopeId ? { environmentId: scopeId } : {}),
381+
context: executionContext,
382+
});
383+
}
384+
// [#23462] The fallback below updates by id and nothing else, so it
385+
// cannot judge a version. A caller who sent one is refused `501`
386+
// before anything is read, never updated unguarded: that is the very
387+
// drop the arm above just closed. The `delete` arm's fallback refuses
388+
// it the same way.
389+
if (expectedVersion !== undefined) {
390+
throw {
391+
statusCode: 501,
392+
message: 'This host has no protocol updateData, so it cannot honour expectedVersion or If-Match on an '
393+
+ 'update. Send the update without them, or update through a host whose protocol implements updateData.',
394+
};
369395
}
370396
if (ql && params.id && typeof ql.update === 'function') {
371397
let all = await ql.find(params.object, findOpts({ where: { id: params.id }, limit: 1 }));

0 commit comments

Comments
 (0)