Uh oh!
There was an error while loading. Please reload this page.
fix(objectql): gate by-id update/delete on record existence — 404 RECORD_NOT_FOUND instead of a 400 from further down the pipeline (#7867) - #7989
Merged
Conversation
…ORD_NOT_FOUND instead of a 400 from further down the pipeline (#7867) Nothing on the action-body write path ever asked whether the target row existed. `ctx.api.object(name).update({ id, … })` reached `ObjectQL.update()`'s by-id branch through `buildSandboxApi` → `ObjectRepository`, and that branch had no existence gate at all: `engine.update()` on a ghost id was a silent no-op that resolved `null`, so the write ran on into validation, the driver and the hook chain and died on whichever complained first. Which one varied with the object's declarations — a `HookConditionError` 400 on a hooked object, a required-field `VALIDATION_FAILED` 400 on an unhooked one. The 400 class varied; the missing 404 was the constant. `delete()` had the same shape and was worse: it reported success for a row that was never there. The gate goes at the engine, in the by-id branches of `update()` and `delete()` — the one point all three action-body write faces funnel through (`ctx.api.object()`, its context-less repo-facade fallback, and `ctx.engine.update()`). Two sibling paths already gated correctly (`protocol.updateData`/`deleteData`, `callData`'s ObjectQL fallback); all three now throw the same `recordNotFoundError`, which moves to `@objectstack/core` so `engine.ts` can reach it without importing `@objectstack/metadata-protocol` (forbidden in the `/core` closure by ADR-0076 D2's boundary ratchet). `@objectstack/metadata-protocol` re-exports it unchanged. The `if (priorRecord) hookContext.previous = …` never-fabricate rule (ADR-0058 Addendum II / #4649) is untouched — it was behaving correctly on a path that should never have been entered, so the producer is removed rather than the message it produced specialized. Consequence worth knowing: the by-id prior-row read is now unconditional. The #5284 / #5929 narrowings asked "does anything CONSUME the prior row?" and skipped the read when nothing did; existence is a consumer that list never enumerated and the one consumer every by-id write has, and no cheaper question answers it. Their read-count pins are rewritten in place, each recording what changed and why. The dispatch half of both cards — the per-object question, `excludeObjects` subtraction, and the retired `sys_fetch_previous_*` builtins — is untouched and still pinned. `@objectstack/runtime`: the sandbox error passthrough also carries `status` now, so an error that names its own HTTP status keeps it across the QuickJS boundary. Without it the action surface served the right diagnosis at the wrong status (`{ code: 'RECORD_NOT_FOUND', httpStatus: 400 }`); `domains/actions.ts` already honoured `.status` first — the number never arrived. Scope: by-id only. A `multi: true` write matching zero rows still resolves "0 rows affected". Tests: a new `engine-write-not-found-gate.test.ts` covering hooked AND unhooked objects (the defect is not about hooks), the delete twin, the predicate-path scope line, and the shared-envelope check; `status` cases in the runtime's `error-passthrough.test.ts`; and an assertion on the dogfood fixture that has been passing 23/23 while logging this exact error through a required 3-shard gate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LRQ39odXrHFtxM777obo8t
… the skip (#7867) #5860's acceptance criterion — the per-object demand gate judges a SKIP_OBJECTS object unhooked — is unchanged and still asserted directly by the sibling cases. What changed is that the gate no longer decides whether the engine LOOKS at the row: #7867's not-found gate needs the by-id prior read unconditionally, so `sys_job_queue` pays one read like everything else. One, not two — an audit handler forcing its own read would still be caught, and the audit ledger is still empty for the skipped object. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LRQ39odXrHFtxM777obo8t
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📓 Docs Drift CheckThis PR changes 4 package(s): 37 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
⛔ 5 release-owned page(s) also reference the affected code. These are read-only:
|
This was referenced Aug 12, 2026
… its fixtures instead of casting (#7867) `engine-write-not-found-gate.test.ts` added one raw tsc error to `@objectstack/objectql`'s hidden test layer (TS2554 — `registerObject` takes a required `packageId`), pushing TEST_DEBT from 355 to 356 and failing `check:type-check-debt --re-measure`. That ledger is a shrink-only ratchet (#5278), so the error is fixed rather than the number raised. Fixed by typing the two fixtures as `ServiceObject` and passing the package id — not by widening the cast. Typing them also surfaced that `primaryKey` is not a declared field property; the registry provisions the primary key itself, so the key was a no-op the compiler could not see while the fixture stayed untyped. Removed, with the reasoning in place. objectql's TEST_DEBT re-measures at 355 (its recorded number) and this file now contributes zero errors. The ledger entry in scripts/check-type-check-coverage.mjs is untouched, and --lower was not run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LRQ39odXrHFtxM777obo8t
…st id (#7867) `last-admin-guard.test.ts`'s 'deleting an unrelated row on another object is not this guard's business' deleted the `sys_account` id 'nope' — never seeded — and asserted it RESOLVED. It passed for a reason unrelated to the guard: `ObjectQL.delete()` had no existence gate on its by-id path, so a delete naming no row was a silent no-op reporting success. The case was asserting the absence of a guard by way of that defect, so #7867's gate turned it red. Rewritten to delete a REAL `sys_account` row (the fixture already seeds one via `accountProvider`), which states the same thing more strongly: the guard does not merely fail to fire on a write that touched nothing, it lets a write that really removes a row on this object through. The ghost-id half is KEPT as its own assertion — refused by the ENGINE with RECORD_NOT_FOUND, and explicitly not by the last-admin guard — so the two questions the case conflated are now answered separately. Reverse-verified: with the delete gate reverted the new assertion goes red (`expected undefined to be 'RECORD_NOT_FOUND'`). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LRQ39odXrHFtxM777obo8t
This was referenced Aug 12, 2026
huangyiirene
marked this pull request as ready for review
August 12, 2026 13:42
This was referenced Aug 12, 2026
This was referenced Aug 12, 2026
Closed
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes#7867.
The defect
Nothing on the action-body write path ever asked whether the target row existed.
engine.update()on a ghost id was a silent no-op that resolvednull, so the write ran on into validation, the driver and the hook chain and died on whichever complained first:showcase_task)400HookConditionError— anafterUpdatecondition readingpreviouson a row nobody readshowcase_invoice)400VALIDATION_FAILED"Issued On is required" — with no prior row a PATCH is validated as a whole recordThe 400 class varied with the object's declarations; the missing 404 was the constant.
delete()had the same shape and was worse — it reported success for a row that was never there.Measured on one showcase stack, same id, same object, same second:
⛔ This is not a
previous-binding bugif (priorRecord) hookContext.previous = …is correct and untouched — ADR-0058 Addendum II / #4649 require that an absent row leavepreviousUNBOUND rather than fabricated. It was behaving correctly on a path that should never have been entered. The fix removes the producer, which is #5574's ruled remedy for this family, rather than specializing the message the symptom produced.Placement — settled by measurement, not preference
The gate goes at the engine, in the by-id branches of
update()anddelete(). The action body reaches the engine three ways and only one passes throughObjectRepository:ctx.api.object(n).update(…)→ScopedContext→ObjectRepositoryctx.api.object(n)with nocreateContext→buildEngineRepoFacade→ql.update(…)directlyctx.engine.update(o, id, data)→buildActionEngineFacade→ql.update(…)directly (used byexamples/app-todo)A repository-level gate closes (1), leaves (2) and (3) with the original defect, and makes
ql.update(o, {id})andctx.api.object(o).update({id})answer one ghost id two different ways — the second de-facto contract PD #12 exists to keep out.Two sibling paths already gated correctly (
protocol.updateData/deleteData#4435,callData's ObjectQL fallback #5138). All three now throw the samerecordNotFoundError, which moves to@objectstack/coresoengine.tscan reach it without importing@objectstack/metadata-protocol— forbidden in the/coreclosure by ADR-0076 D2's boundary ratchet.@objectstack/metadata-protocolre-exports it unchanged, so every existing importer is untouched. No new error code is minted;RECORD_NOT_FOUNDis already in the ADR-0112 ledger.Existence is asked with a pre-write read, never off the write's own result:
IDataDriver.updatedeclares no not-found signal, and the engine's post-write readback isnullfor a second reason (a write that moves the row out of the caller's row scope), so reading either would answer 404 to a write that landed.#5284 (update) and #5929 (delete) narrowed that read to "does anything CONSUME the prior row?". Existence is a consumer that demand list never enumerated and the one consumer every by-id write has, and no cheaper question answers it — the skip and the gate are mutually exclusive. Reverse verification proved it directly: restoring the narrowing with the gate in place makes existing rows 404 (
Record 1 not found in doc).Measured cost: #5929's own record enumerates the global hook registrants (plugin-sharing, service-storage, plugin-auth, plugin-audit — all registering with no
object), so on any kernel loading them the demand was already true for every object and the narrowing skipped nothing. The read is genuinely new only for a bare@objectstack/objectql/coreembedder with no hook, no prior-reading rule and no roll-up — which is buying a 404 it did not have.The dispatch halves of all three affected cards are untouched and still pinned: the per-object question, the
excludeObjectssubtraction, and the retiredsys_fetch_previous_*builtins. Their read-count pins are rewritten in place, each recording what changed and why rather than being deleted.Sandbox:
statusnow crosses the VM boundaryThe engine's 404 reached the wire as
{ code: 'RECORD_NOT_FOUND', httpStatus: 400 }— the right diagnosis at the wrong status, because the sandbox error passthrough allowlistedcodeandfieldsbut notstatus.domains/actions.tsalready honours.statusfirst; the number never arrived.statusjoins the allowlist (finite numbers only), and the security assertion widens with it — arecordNotFoundError's ownobjectproperty still does not cross.Side effect worth knowing: a permission refusal thrown inside a body now keeps its 403 instead of flattening to 400.
Scope
By-id only. A
multi: truewrite matching zero rows still resolves "0 rows affected" — the same line both sibling paths draw, pinned so the gate cannot creep onto the bulk path.Tests
packages/objectql/src/engine-write-not-found-gate.test.ts— hooked and unhooked objects (the defect is not about hooks), thedelete()twin, "no handler ran", the predicate-path scope line, and a shared-envelope check againstrecordNotFoundErroritself.packages/runtime/src/sandbox/error-passthrough.test.ts—statuscases, including the widened allowlist assertion and a non-finite-status guard.packages/qa/dogfood/test/showcase-anonymous-deny-surfaces.dogfood.test.ts— the assertion the card asked for. That fixture has been passing 23/23 while logging this exact error (hook-wrappers.ts→triggerHooks→engine.ts, plus[BodyRunner] sandboxed action threw) through a required 3-shard gate. Two new cases assert the member's answer is 404RECORD_NOT_FOUNDand that/actionsand/dataagree on it. The existing.not.toBe(401)anonymity case is deliberately left as it was.@objectstack/plugin-audit's plugin-audit 的 5 个 hook 全部无object注册 ⇒ 引擎「按对象」需求门(#5284 单 id / #5038 批量)在 audit 启用时恒真 #5860sys_job_queuecase, which a full-repo run found after the objectql ones. None deleted; each records what changed and why. Five engine doubles taught to answer the by-id read they previously returnednullfor.pnpm testrepo-wide is flaky on this build container under load and I did not see it go green — the runs, and the one real failure they surfaced, are itemised in the<!-- os-dev-report -->comment on #7867. Every package this diff can touch was re-run standalone and is green there. CI's verdict is the reviewer's to call.Every behavioural change was reverse-verified in isolation — predictions and actuals in that same comment.
Generated by Claude Code