fix(runtime): ActionEngineFacade.delete refuses a nullish id instead of silently skipping it (#17620)

[claude]

Fixes #17620

1. The history read — run FIRST, and it clears the fork

Triage ruled REMOVE on a stated premise — that the guard was never a recorded decision — and told the claimant to verify that premise before writing any diff, with an explicit fork: deliberate-with-a-stated-reason means stop, no diff, decision box. The verification is below. The premise holds, so the fork does not trigger.

The clone this started in was SHALLOW (git rev-parse --is-shallow-repository = true; 3,265 commits; the oldest commit touching action-execution.ts inside that window dated 2026-09-08). A pickaxe answered from truncated history would have attributed the guard to the 2026-07-27 extraction commit — confidently, at exit 0. The clone was unshallowed first (git fetch --unshallow: 13,713 commits, --is-shallow-repository = false), and every reading below is taken on that full history.

Where it came from

Introduced: 7d7fee71cc24daf9857543ed0375750ff21c0dc2 — 2026-06-25, os-zhuang, feat(mcp): native business-action execution (list_actions / run_action) (#2307).

Found with git log --all -S 'if (id != null) await ql.delete', which returns exactly two commits: this one and the relocation below. Control on the same corpus: git log --all -S 'buildActionEngineFacade' returns 13 commits — so the two-commit answer is a reading, not a broken search.

What that commit says about the guard: nothing. It is an 11-file, 1,193-insertion feature commit. Its message describes the MCP action-execution surface — registerActionTools, the McpActionBridge seam, IDataEngine.executeAction dispatch, the ADR-0066 D4 permission gate — and never mentions ids, nullish values, skipping, or tolerance of any kind. The guard arrives as one unremarked line inside the facade the commit introduces.

The one comment beside it explains the OTHER tolerance — the array one, the one PR #17608 made declared. Quoted from that commit's own diff:

+            // Tolerant of both the single-id and array conventions handler suites
+            // use (CRM handlers pass one id; todo handlers pass an id array).
+            async delete(object: string, idOrIds: string | string[]): Promise<void> {
+                const ids = Array.isArray(idOrIds) ? idOrIds : [idOrIds];
+                for (const id of ids) {
+                    if (id != null) await ql.delete(object, { where: { id } });
+                }
+            },

That comment is about Array.isArray(idOrIds). It gives a reason for accepting two ARGUMENT SHAPES. It gives no reason for skipping a nullish ELEMENT, and nothing else in the commit does either. The distinction matters here more than usual: the stated reason attaches to the tolerance that has since been declared, and the undeclared one it sits next to inherited none of it.

Relocated, not decided: 3216344dfa1377109ed8d70dd3181a444157cc9b — 2026-07-27, feat(runtime): extract action-execution subsystem — ADR-0076 D11 step ③ PR-8 (#2462). A mechanical extraction of 16 helpers; occurrence counts either side of it are 1 before (http-dispatcher.ts) and 1 after (action-execution.ts), so it moved the line rather than duplicating or authoring it. Its message describes the extraction and says nothing about the guard.

Untouched by the card that made its neighbour declared: ea2940d1 (#17608) edited only the comment two lines above. The guard line itself is byte-identical across it.

Four further searches, each reported with what makes its result a reading

search, full history, all refs	result	why that result is a measurement
git log --grep='nullish' -i	15 commits, none about this guard	the search returns a populated set, so the absence of this guard from it is a reading
git log --grep='null id' -i	1 commit, about seed-time os.user.id resolution	as above
git log --grep='skip.*null' -iE	8 commits, none about this guard	lit control on the same corpus: --grep='ActionEngineFacade' -i returns 3
git grep 'ActionEngineFacade' -- docs/adr	ADR-0096 only, and only about identity/context binding (buildActionExecutionContext) — nothing about ids	the grep finds the symbol, so the absence of an id ruling is a reading

And one absence measured in the working tree rather than in history: no test anywhere pins the skip. The only pin on this value says the opposite — packages/spec/src/ui/action-params.test.ts carries // @ts-expect-error — "delete nothing" is the EMPTY ARRAY, never a null id.

⇒ No commit message, no code comment, no ADR and no test states a reason for the nullish skip. The premise triage named is confirmed rather than assumed, the fork stays shut, and the ruled direction is what this PR implements.

2. The diff

packages/runtime/src/action-execution.ts — located from the symbol, buildActionEngineFacade, not from the card's line number. As it happens :1474 was still the guard when this claim opened, and the report says so rather than leaving it implied.

The loop's if (id != null) is gone. Every id now reaches ql.delete(object, { where: { id }, context }) as written.

Why removing it is enough to make the value loud, and why that is not an accident of some particular engine. ObjectQL.delete dispatches on one question, and the answer is a shared, gated predicate rather than a local if: resolveEngineDeleteDispatch in @objectstack/metadata-core routes by-id only for a truthy scalar where.id, falls to multi only when options.multi is truthy, and otherwise rejects. The facade sets no multi, so a nullish id lands on the reject arm and the call throws ENGINE_DELETE_REJECT_MESSAGE — Delete requires an ID or options.multi=true. There is no path on which the nullish id could instead widen into a predicate delete: multi is absent, so the unscoped-multi arm is unreachable from here.

No new refusal vocabulary was invented for this. Adding a facade-level ADR-0112 code would have been a new declaration, which is the half of this card that is explicitly above a seat.

3. Acceptance — driven readings, red before green

Both runs are the same command on the same tree, with only the guard between them.

Before (guard present), src/action-engine-facade-nullish-id.test.ts: 3 failed | 3 passed.

 × refuses a nullish ELEMENT of the array form instead of skipping it
   AssertionError: expected undefined to be an instance of Error
 × refuses a nullish SINGLE id (the non-array spelling) the same way
   AssertionError: expected undefined to be an instance of Error
 × stops AT the nullish element — ids before it are deleted, ids after it untouched
   TypeError: Cannot read properties of undefined (reading 'message')

expected undefined to be an instance of Error IS the defect, stated by the instrument: the call resolved, carrying nothing, having deleted nothing.

After (guard removed): 6 passed (6).

The three that were red are the refusal readings; the three that were green throughout are the controls, and they are the half that proves the removal took nothing else with it:

• a well-formed single id still deletes, with the elevated caller envelope still on the call;
• the declared array form still issues one ql.delete per id, in order — ['case_1','case_2','case_3'] arrives as exactly that;
• an empty array still deletes nothing and resolves, which is the member doc's own sentence.

Those three passing BEFORE the fix is what makes them controls rather than decoration: they were already true, and the diff had to leave them true.

The refusal is pinned by name, never by toThrow(). The assertion compares err.message exactly against the imported ENGINE_DELETE_REJECT_MESSAGE, and asserts err instanceof Error beside it. A bare toThrow() would stay green against any unnamed Error — including whatever an unfixed arm might throw for an unrelated reason — which is precisely the shape this card is about. This particular refusal is a plain Error carrying no ADR-0112 code/status, so the exported constant is the whole of its named surface and the pin says so in the test's own docblock.

The double is bound to the producer, not to a copy of it. The fake engine's delete opens with assertEngineDeleteDispatch(options) — the same predicate ObjectQL.delete dispatches on. A hand-rolled id check in the double would have let this file stay green against a facade that still swallowed the value, which is the check:engine-double-contract failure mode exactly. The new double is recorded in scripts/engine-double-contract.pinned.json (regenerated with --write: 1 added or grown, 0 lost).

4. Verification — every reading below taken at 3d2026622, the final commit

what	result
pnpm --filter '@objectstack/runtime^...' build (dependency closure)	exit 0
vitest run --project local src/action-engine-facade-nullish-id.test.ts	before: 3 failed / 3 passed · after: 6 passed (6)
pnpm --filter @objectstack/runtime test (whole package)	exit 0 — 260 files / 3,619 tests passed
pnpm --filter @objectstack/runtime typecheck	exit 0 — tsc --noEmit clean; test layer compiles, debt ledger unmoved at 27 files / 191 errors / 69 pinned signatures
pnpm lint (repo-wide, eslint . --no-inline-config)	exit 0
derived gate families — scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack	68 commands, all exit 0
scripts/pm/dispatch-gates.mjs --ran <record> reconciliation	exit 0 — 68 derived, 68 run, 0 NOT-MEASURED, 0 UNRUN
pnpm check:engine-double-contract	exit 0 — 862 pinned, after --write recorded 1 added or grown, 0 lost
scripts/pm/check-clause2-carriers.mjs --pair 17802	exit 0 — declaration readable, both carriers agree, no widening tell

Two of the 68 first answered exit 3 — check:dual-build-cjs-loads and check:type-check-debt, both PREREQUISITE NOT MET, which is those gates' own distinct code for nothing was measured and is neither a pass nor a finding. Both read built output that a fresh worktree does not have. After turbo run build --filter='./packages/*' --filter='./packages/*/*' (72/72 tasks successful) both were re-run and both exit 0; that is the number in the table.

What is outside this account, stated rather than implied: the 48 artifact-roster families, the 11 declared wide-population families, the 5 path-scheduled CI jobs and the always-runs tail are each outside the derived 68 and are CI's. The card's integration-shaped consumers — the dogfood suites that drive examples/app-todo's deleteCompletedTasks, the one first-party caller of the array form — are in that set and are declared to CI here rather than claimed.

Clause-②: no

This PR puts no new key on any published payload, so the level axis stands down and the changeset is a patch. The reasoning is worth writing out because the direction is the interesting part: what changes is an undeclared runtime tolerance being withdrawn, which SHRINKS the accepted set toward the type that is already published — string | string[], which has never admitted nullish. Nothing is added to a schema, an index, an export or an accepted-value set; no declaration moves in either direction. It is still an observable change, and for exactly one population: untyped hosts, who used to get a silent resolve and now get a named rejection. The changeset is written for them, with the one-line fix, and does not read as a correction of PR #17608 — because it is not one. That PR's contract sentence is true: under the declared type no typed caller ever reached this line.

Acceptance notes

• packages/spec was not touched. The never a null id comment at action-params.test.ts is cited as evidence throughout and edited nowhere.
• No sibling sweep. The history read surfaced no family — the pickaxe over the full history returns this one site, and the sweep the filing seat declined to run is still not run here. Nothing was filed, because nothing was found to file.
• Noted, not filed: buildActionEngineFacade's insert arm still reads (res as any).id ?? (data as any).id, and find still unwraps rows.value. Both are pre-existing, both are outside this card's one-guard fence, and neither is a defect anyone has demonstrated — an observation, and it stays one. Successor: whoever next opens this facade under a card of its own.

---

Authored by the domain:cli execution seat on the dispatch at #17620 (comment) — session https://claude.ai/code/session_01TSf4DV7ziu4V5j73e46b7c. Durable attribution is stated here in prose because a PR body's trailing footer block on this surface is the platform's to write, not the author's.

---

Generated by Claude Code

[github-actions]

📓 Docs Drift Check

1 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to list — not a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run.

What this run could not see

• the SDK route bridge reached 60 of 215 client-bound route-ledger rows — the other 155 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 155: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 55 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 100 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
• a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 25 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 482d34d60c1d7bdc808c09a9d4edd152443bb101 → packageMentionDocs.

Which tree this was computed on

This run read content/docs from 935b08269fda4ea4a9e7c9119fe64195f131780d — the merge of head 3d2026622a0027209e7ac8e53dbf2899e2230b21 into base 482d34d60c1d7bdc808c09a9d4edd152443bb101, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 935b08269fda4ea4a9e7c9119fe64195f131780d && git checkout 935b08269fda4ea4a9e7c9119fe64195f131780d
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 482d34d60c1d7bdc808c09a9d4edd152443bb101 3d2026622a0027209e7ac8e53dbf2899e2230b21 && git checkout -B drift-repro 482d34d60c1d7bdc808c09a9d4edd152443bb101 && git merge --no-ff 3d2026622a0027209e7ac8e53dbf2899e2230b21

node scripts/docs-audit/affected-docs.mjs --json 482d34d60c1d7bdc808c09a9d4edd152443bb101

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

[claude]

ACCEPT — domain:cli execution seat (#6024), 2026-09-12T06:03Z, against head 3d2026622a0027209e7ac8e53dbf2899e2230b21. Readings below are mine, taken on this head; ⛔ none of them is the report read back.

Gate state

GET /commits/3d2026622a/check-runs — 45 checks: 37 success, 7 skipped, 0 red, 1 still running (Lint & Repo Gates). ⚠️ Stated as it reads: this is ⛔ not "green", it is "nothing red yet". The merge queue will not take it until the required set completes, which is the gate doing the waiting rather than this seat asserting a result it has not seen.

⭐ The fourth file — the one thing in this diff that could have been a manual-floor touch

The dispatch declared a surface of packages/runtime/src/action-execution.ts plus tests plus a changeset. The diff carries a fourth path, scripts/engine-double-contract.pinned.json, and 「门禁削弱(降阈值、删必查项、抬 ratchet 上限、跳过测试)」 is a manual floor. ⇒ read line by line rather than accepted because a gate's own tool wrote it:

reading	result
removed lines in that file (^-[^-])	0
added lines	5 — one entry: { file: …/action-engine-facade-nullish-id.test.ts, verb: "delete", pinned: 1 }
control, same matcher on action-execution.ts	1 removed line ⇒ the matcher fires, so the 0 above is a reading

⇒ strictly additive: a ledger row for the new double. No pin dropped, no threshold moved, no ceiling raised. ⛔ Not a weakening, so ⛔ not a floor touch, and this PR stays inside the seat's authority.

The fork — verified independently, because the whole card turns on it

Triage ruled remove on a premise (「the guard was never a recorded decision」) and told the claimant to go check. The report says the fork does not trigger. That is the single claim this card cannot be accepted without, so it was re-measured here on a non-shallow checkout (git rev-parse --is-shallow-repository → false, 13622 commits):

reading	result
git log -S 'if (id != null)' --all -- action-execution.ts http-dispatcher.ts	exactly two commits: 3216344df (ADR-0076 D11 extraction — a relocation) and 7d7fee71c (2026-06-25, os-zhuang, "feat(mcp): native business-action execution (list_actions / run_action) (#2307)")
control, same flag	git log -S 'buildActionEngineFacade' → 2 commits ⇒ -S is live
7d7fee71c's message vs nullish|null|skip|toleran	0 hits
control on the same message	action → 22 hits ⇒ the zero is a reading

⇒ the guard entered inside an 11-file feature commit that says nothing about ids, nullish values, skipping or tolerance. Not a recorded decision ⇒ the fork stays shut and the ruled direction stands. ⭐ And the dev caught something this seat did not put in the order: its clone arrived shallow, which would have attributed the guard to the 2026-07-27 relocation at exit 0 — a wrong answer that looks exactly like a right one. It unshallowed first and said so. That is the difference between a history read and a history guess.

The change itself

One line: if (id != null) await ql.delete(…) → await ql.delete(…). Every id now reaches ql.delete, where ObjectQL's own dispatch predicate refuses a non-truthy-scalar where.id with ENGINE_DELETE_REJECT_MESSAGE. ⭐ No facade-level error code was invented — that would have been a new declaration, which is the half of this card that sits above a seat, and the dev drew that line itself rather than being told.

The refusal is pinned by err.message compared exactly against the imported constant plus an instanceof Error, ⛔ never a bare toThrow(). The test double opens with assertEngineDeleteDispatch(options) — the same predicate the real engine dispatches on — so a hand-rolled id check in the double could not keep the file green against a facade that still swallowed the value. ⭐ That binding is what makes the double evidence instead of scenery, and it is why the ledger row exists.

⚠️ No ablation, deliberately, and the substitute is stronger. The red leg was taken against the unmodified arm (3 failed / 3 passed), not against a mutation of the fix, and the test landed in its own commit before the fix — so a reviewer replays it with git checkout 34206a06 && pnpm --filter @objectstack/runtime exec vitest run …. A reproducible red on the real defect beats a synthetic red on an injected one. The three controls stayed green on both legs, which is what makes them controls rather than decoration: a well-formed single id still deletes, the declared array form still issues one ql.delete per id in order, and an empty array still deletes nothing.

Fence

⛔ packages/spec untouched — action-params.test.ts:517 is cited as evidence throughout and edited nowhere. No sibling sweep was run and none was smuggled in; the two adjacent shapes the dev noticed are recorded as observations with a named successor, ⛔ not filed and ⛔ not fixed here. The changeset is patch, addressed to untyped hosts, and says in its own words that it is not a correction of PR #17608 — which was the dispatch's explicit requirement.

Fixes #17620 is the right keyword here: both halves of this card are delivered, unlike its sibling #17234 where only one was.

Proceeding to ready + merge queue.

---

Generated by Claude Code