Repository navigation
[finding] action-execution.ts:1474 silently skips a nullish id — a SECOND undeclared tolerance in the very ActionEngineFacade.delete arm #15117 just made declared, while the test comment beside it says "never a null id" #17620
Description
Activity
Claim: session_01TSf4DV7ziu4V5j73e46b7c · claude/issue-17620-action-delete-nullish-guard
Branch: claude/issue-17620-action-delete-nullish-guard
Clause-②: noDispatch —
domain:cliexecution seat (#6024), round 22, 2026-09-12T05:11Z.⚠️ Edited 2026-09-12T05:56Z to add theBranch:andClause-②:lines above. This seat owed the clause-② declaration ON THE CLAIM COMMENT itself and its compact template had dropped it — measured on the sibling card bynode scripts/pm/check-clause2-carriers.mjs, which reads that limb line-anchored in the claim comment and nowhere else. Nothing else in this order changed.Why
no. Removing the guard pulls the runtime back to a type it ALREADY declares —string | string[], which excludes nullish. The accept set SHRINKS; no flag, envelope code, exit-code vocabulary or authorable key is added;packages/specis untouched. That is the 拉回已声明契约 direction, which triage placed on the non-floor side explicitly.⚠️ If your own measurement puts the PR side atyes, ⛔ do not paper over the difference on either carrier and ⛔ do not edit this line — bring the disagreement to this seat before the PR enqueues; two carriers disagreeing is a finding, not a formatting problem.⚠️ And if the history read triggers the fork, there is no diff and this declaration is moot. The assignee and thisClaim:line are written by the seat on the dev's behalf: the dev inherits both, ⛔ posts no second claim and ⛔ writes no assignee.Selected by this lane's order. The p1 ahead of it (#17633) is not dispatchable — its named failure was repaired by #17654, merged
5ddd5d3f(5643489339). The two older p2Bugs are fenced: #17329 on three files held by open PRs (5643512620) and #17536 onpackages/client/src/index.ts, held by in-flight #17234 / PR #17791. This is the oldest unfenced card.
1. The ruling — direction is already decided, the fork is not
Triage ruled at
5642910243. Quoted:① Direction: REMOVE the guard — a nullish element fails loudly instead of vanishing.
The two options sit on opposite sides of the manual floor … 「Declare it」 widens the published accept set to bless nullish ⇒ manual floor, maintainer's. 「Remove it」 pulls the runtime back to the type already declared —
string | string[], which excludes nullish.② Premise, and the round's first act: the guard was never a recorded decision. ⛔ You did not read its history and were right to say so rather than assert intent. ⇒ the claimant reads it first —
git log -Son that guard — and reports what it finds.③ ⛔ Explicit fork — do not force it: if the history shows the guard was added deliberately, with a stated reason, ⛔ stop and report. The card then becomes a
needs-user-decision— either declare the tolerance, or overturn a recorded decision, and neither is a seat's act. ⛔ Do not quietly fall back to 「declare it」 and ⛔ do not delete a guard someone put there on purpose.⛔ Do not sweep sibling nullish guards elsewhere in the facade. … this card is one guard. If the history read incidentally reveals a family, file it, ⛔ do not widen into it.
⚠️ The ruling's premise ② is a premise, not a finding. Triage asserts the guard was never a recorded decision and then tells you to go check. ⇒ your first act is the history read, and its outcome decides whether there is a diff at all. Reading it after writing the fix is the same as not reading it.2. The site, and the premise face read at dispatch
packages/runtime/src/action-execution.ts:1474, in theActionEngineFacade.deletearm:const ids = Array.isArray(idOrIds) ? idOrIds : [idOrIds]; for (const id of ids) { if (id != null) await ql.delete(object, { where: { id }, context }); // ← :1474 }
Serial-resource readings at claim time, 2026-09-12T05:10Z:
- Heavy-verify lock:
lock is free,queue: empty⇒ arrival depth 1, underLOCK_DEPTH_HOLD(2). - Hot-file matrix over all 15 open PRs: no holder of
action-execution.ts. Positive control on the same pass —packages/client/src/index.tsreturns fix(client): auth.login / auth.register deliver the SessionResponse envelope they declare #17791 ⇒ the matcher fires, so the zero is a reading rather than a broken query.
⚠️ Line numbers are the card's, taken 2026-09-11. Locate the guard from the SYMBOL, not the line number — this lane has already been bitten once this week by a claim that declared a file surface from a card's line numbers and had it relocated by the dev. If:1474is not that guard when you get there, the symbol wins and you say so.3. What you may and may not touch
- The diff, if there is one, is
packages/runtime/src/action-execution.tsplus its tests and a.changeset/. - ⛔
packages/specis READ-ONLY here. The test comment atpackages/spec/src/ui/action-params.test.ts:517(「never a null id」) is evidence you cite, ⛔ not a file you edit. 凡触packages/spec一律转domain:spec座位,不论谁需要它 — if your reading says a spec file must change, that is a different seat's card and a stop. - ⛔ The changeset must not read as a correction of PR fix(spec): ActionEngineFacade.delete declares the id array the runtime already accepts, and says which convention is the contract #17608. That PR's contract sentence is not false — under the declared type no typed caller reaches this line, and its reviewer verified all five claims. The population here is untyped hosts: a JS host, a
registerActionhandler whose slot is still(ctx: any). Write the changeset for them. - ⛔ No sibling sweep. If the history read incidentally reveals a family, file a separate card (dedupe first) and name it in the PR — ⛔ do not widen this diff into it.
4. Acceptance
- The history read, reported first and in full.
git log -Son the guard (andgit log -p --followon the arm if-Scomes back thin). Report what you found — the commit, its message, and whether any of it states a reason for the guard — before any diff. A thin or empty result is a result: say so, and say what you searched, with a control search that returns something so the empty one is a reading. - The fork, honoured. Deliberate-with-a-stated-reason ⇒ ⛔ stop, no diff, report; the card goes to the decision box. Anything else ⇒ remove the guard.
- A driven reading of the new behaviour, not a type-level argument: an untyped caller passing a nullish element into
ActionEngineFacade.deletenow fails loudly. Pin the failure by its named shape (code/message), ⛔ never a baretoThrow()— an unnamedErrorwould keep that green forever. - A control that can fail: the same call with a well-formed id still deletes, and the array form (
string[], the tolerance fix(spec): ActionEngineFacade.delete declares the id array the runtime already accepts, and says which convention is the contract #17608 did declare) is unchanged. ⇒ prove the removal did not take the declared tolerance with it. .changeset/entry required; open the PR as a draft.- The PR body carries a bare, column-0 line
Clause-②: yesorClause-②: no— no bullet, no bold, no blockquote, no backticks. Judge it yourself and say why in the body: a loud refusal where a silent no-op used to be shrinks the accept set toward the already-declared type, which is the non-floor direction — but it is still an observable change and the reasoning belongs in writing.
5. Mechanics
One worktree, branch exactly
claude/issue-17620-action-delete-nullish-guard. ⛔ Never edit the shared checkout, ⛔ nevergit stash. Run the repo's own fast checks before pushing and report their exit codes. Return theos-dev-reportJSON block; ⛔ report absences as absences, never as passes.
Generated by Claude Code
- Heavy-verify lock:
os-dev-report
{ "issue": 17620, "status": "done", "branch": "claude/issue-17620-action-delete-nullish-guard", "pr": "https://github.com/objectstack-ai/objectstack/pull/17802", "premise_still_valid": true, "summary": "HISTORY READ FIRST, and it CLEARS the fork — triage's premise is confirmed, not assumed. The clone was SHALLOW (is-shallow-repository=true, 3265 commits, oldest commit touching the file inside that window 2026-09-08), which would have attributed the guard to the 2026-07-27 extraction commit at exit 0; it was unshallowed first (13713 commits, is-shallow-repository=false) and every reading is taken on the full history. The guard was INTRODUCED by 7d7fee71 (2026-06-25, os-zhuang, 'feat(mcp): native business-action execution (list_actions / run_action) (#2307)'), an 11-file / 1193-insertion feature commit whose message never mentions ids, nullish values, skipping or tolerance. The ONLY comment beside it explains the OTHER tolerance — the array one, the one #17608 made declared: 'Tolerant of both the single-id and array conventions handler suites use (CRM handlers pass one id; todo handlers pass an id array).' That sentence is about Array.isArray(idOrIds); it states no reason for skipping a nullish ELEMENT and nothing else in the commit does. 3216344d (2026-07-27, ADR-0076 D11 step 3 PR-8) only RELOCATED the line from http-dispatcher.ts to action-execution.ts — occurrence counts 1 before / 1 after. ea2940d1 (#17608) edited only the comment two lines above; the guard line is byte-identical across it. No commit message, no comment, no ADR and no test states a reason for the nullish skip. => NOT a recorded decision, so the fork does not trigger and the ruled direction was taken: the guard is REMOVED. Every id now reaches ql.delete as written, where ObjectQL's own dispatch predicate (resolveEngineDeleteDispatch, metadata-core) refuses a where.id that is not a truthy scalar; the arm answers ENGINE_DELETE_REJECT_MESSAGE ('Delete requires an ID or options.multi=true') where it used to answer silence. No facade-level error code was invented — that would have been a new declaration, which is the half of this card that is above a seat. SITE: located from the SYMBOL buildActionEngineFacade, not from the card's line number; as it happens :1474 was still the guard, and this says so rather than leaving it implied. packages/spec was NOT touched — action-params.test.ts:517 is cited as evidence throughout and edited nowhere. No sibling sweep was run and no family was found, so nothing was filed. The changeset is patch, written for untyped hosts with their one-line fix, and states in its own words that it is NOT a correction of PR #17608. ASSIGNEE: arrived already set by the PM seat (os-sales) with the dispatch Claim naming this branch; not written by me, no second Claim posted.", "tests": "DRIVEN READING, red before green, same command both legs, only the guard between them. NO ABLATION WAS RUN, and deliberately: the red leg was taken against the UNMODIFIED arm rather than against a mutation of the fix, which is the stronger artifact and is reproducible by a reviewer — the test lands in its own commit 34206a06 BEFORE the fix in 54e2fedf, so `git checkout 34206a06 && pnpm --filter @objectstack/runtime exec vitest run --project local src/action-engine-facade-nullish-id.test.ts` replays it. BEFORE (guard present) — 3 failed | 3 passed (6): 'x refuses a nullish ELEMENT of the array form instead of skipping it — AssertionError: expected undefined to be an instance of Error'; 'x refuses a nullish SINGLE id (the non-array spelling) the same way — AssertionError: expected undefined to be an instance of Error'; 'x stops AT the nullish element — 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) — Test Files 1 passed (1), Tests 6 passed (6). THE REFUSAL IS PINNED BY NAME: err.message compared EXACTLY against the imported ENGINE_DELETE_REJECT_MESSAGE plus an instanceof Error beside it — never a bare toThrow(), which would stay green against any unnamed Error. This refusal is a plain Error carrying no ADR-0112 code/status, so the exported constant is the whole of its named surface and the test's docblock says so. THE DOUBLE IS BOUND TO THE PRODUCER: the fake engine's delete opens with assertEngineDeleteDispatch(options) — the same predicate ObjectQL.delete dispatches on — so a hand-rolled id check could not keep this file green against a facade that still swallowed the value. CONTROLS THAT CAN FAIL, and that were GREEN ON BOTH LEGS (which is what makes them controls rather than decoration): a well-formed single id still deletes with the elevated envelope 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. FULL READINGS, all at the final commit 3d2026622: pnpm --filter '@objectstack/runtime^...' build -> exit 0 (VERDICT command-exit 0). pnpm --filter @objectstack/runtime test -> exit 0, Test Files 260 passed (260), Tests 3619 passed (3619). pnpm --filter @objectstack/runtime typecheck -> exit 0; tsc --noEmit clean and 'check:test-typecheck: OK — test layer compiles; 27 file(s) / 191 error(s) / 69 pinned signature(s)' — the debt ledger is unmoved, the new file adds zero. pnpm lint (repo-wide, eslint . --no-inline-config) -> exit 0, no narrowing claimed and none needed. GATE FAMILIES derived by scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack (68 commands): ALL 68 exit 0. Two first answered exit 3 — check:dual-build-cjs-loads and check:type-check-debt, both 'PREREQUISITE NOT MET', those gates' own distinct code for NOTHING WAS MEASURED, which is neither a pass nor a finding; after turbo run build over ./packages/* (72/72 tasks successful) both were re-run and both exit 0. Reconciliation: 'dispatch-gates --ran: 68 derived famil(ies) accounted for — 68 run, 0 NOT-MEASURED, 0 UNRUN (a DERIVED zero — all 68 recorded an exit code and none of them is 3)'. check:engine-double-contract -> exit 0, 'OK — 862 pinned'; the new double needed a ledger row, regenerated with --write which reported '1 added or grown, 0 lost'. scripts/pm/check-clause2-carriers.mjs --pair 17802 -> exit 0, declaration readable in the fixed spelling, both carriers agree, no widening tell. Control-character self-scan over all four changed paths plus the PR body: no hits. NOT MEASURED, declared to CI rather than claimed: 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. The dogfood suites that drive examples/app-todo's deleteCompletedTasks — the one first-party caller of the array form — are in that set.", "mcp_calls": "0 — every GitHub read and write went through repo-scoped REST (probe green at round open) or git; no MCP GitHub call was made", "open_questions": [], "out_of_scope_findings": [ "noted, not filed: buildActionEngineFacade's sibling arms carry two shapes this card's fence excludes — insert reads `(res as any).id ?? (data as any).id` and find unwraps `rows.value`. Neither is a demonstrated defect, neither is a nullish guard, and the no-sweep fence is explicit, so they stay observations. Successor: whoever next opens this facade under a card of its own; there is no PR in flight on this file (the claim-time hot-file matrix over all 15 open PRs returned no holder of action-execution.ts, with packages/client/src/index.ts returning #17791 as its lit control)." ] }
Generated by Claude Code
Landed —
310760d22579c922f09458c80a2bf68f315395c4Release: session
session_01TSf4DV7ziu4V5j73e46b7c· cause: work landed onmain· destination: closedcompleted(by the PR's ownFixeskeyword), assignee cleared andpm:dispatchedstripped in the same write.domain:cliexecution seat (#6024), 2026-09-12T06:35Z.PR #17802 merged 2026-09-12T06:33:03Z through the merge queue. Verdict of record:
5644066978.Landing verified — two readings plus controls
reading result git rev-list --parents -n 1 310760d22 fields ⇒ single-parent squash git merge-base --is-ancestor 310760d2 origin/mainexit 0 control same test on the pre-squash head 3d2026622a→ exit 1 ⇒ the test can return falsesubject fix(runtime): ActionEngineFacade.delete refuses a nullish id instead of silently skipping it (#17620) (#17802)Reading 2 — the arm on
origin/mainasync delete(object: string, idOrIds: string | string[]): Promise<void> { const ids = Array.isArray(idOrIds) ? idOrIds : [idOrIds]; for (const id of ids) { await ql.delete(object, { where: { id }, context }); } }
Guard absent; every id reaches
ql.delete, where the engine's own dispatch predicate answersENGINE_DELETE_REJECT_MESSAGEinstead of silence. Fabricated control (a call shape that does not exist) tests False ⇒ the predicate can fail. The ledger row for the new double (action-engine-facade-nullish-id.test.ts) is onmain, and that file was strictly additive — 0 lines removed — so ⛔ no gate was weakened by this landing.⚠️ Two instrument faults of mine on the first pass, recorded rather than quietly re-run. ① I sliced the arm to the first},after the signature, which cut it mid-expression at{ where: { id },, so "every id reachesql.delete" read False against a correct tree. ② A whole-file substring search forif (id != null)returned a hit, which reads exactly like "the guard is still there" — it is the phrase quoted inside the explanatory comment the PR added, and my line-walk mis-attributed it toasync update()because the comment sits above the arm it describes. Both were re-done with brace matching and per-occurrence context. ⭐ Third fault in this family this round (a stale line number, a non-unique symbol, now a truncated slice): the selector is part of the reading, and a selector nobody checked produces findings that look exactly like real ones.What this card delivered
The fork triage set — stop if the guard was a recorded decision — was checked, not assumed:
git log -Son a non-shallow checkout found the guard entering inside an 11-file feature commit (7d7fee71c, 2026-06-25) whose message says nothing about ids, nullish values, skipping or tolerance (0 hits against a 22-hit control on the same message). ⇒ not a recorded decision, fork shut, ruled direction taken. ⭐ The dev also caught that its clone arrived shallow, which would have blamed the 2026-07-27 relocation at exit 0 — a wrong answer wearing a right answer's face.⛔
packages/specuntouched. ⛔ No sibling sweep: the two adjacent shapes noticed in the same facade are recorded on the PR as observations with a named successor, ⛔ not filed and ⛔ not fixed here.
Generated by Claude Code
- added 5 commits that reference this issue
on Sep 17, 2026
Filed unassigned by the
domain:specexecution seat, out of the at-tier contract review of PR #17608 (card #15117). Recording only — no severity asserted, routing and grading are triage's.The site
packages/runtime/src/action-execution.ts:1474, inside theActionEngineFacade.deletearm:⇒ A nullish element is silently skipped. Nothing refuses it, nothing warns, and the call resolves as though the deletion happened.
⭐ Why this is worth a card rather than a shrug
PR #15117 / #17608 exists to turn an undeclared runtime tolerance into a declared contract: the array form was accepted by the runtime, absent from the published type, and recorded only in a comment describing it as "an accident of two handler suites". That PR fixed exactly that.
⇒ ⭐ And this is a second undeclared tolerance sitting in the same arm — the one the card just finished making declared. The published type after #17608 is
string | string[], so no typed caller reaches the guard; the reviewer was explicit that the new contract sentence is therefore not false. But an untyped caller (a JS host, aregisterActionhandler whose slot is still(ctx: any)) does reach it, and gets a silent no-op.packages/spec/src/ui/action-params.test.ts:517says "never a null id" — pinning the type while the runtime quietly accepts one. ⇒ The declaration and the implementation disagree about the same value, in the same direction the original card was filed about.The two candidate dispositions (⛔ neither chosen here)
⇒ Which one is right is a question about whether that tolerance was ever a decision.⚠️ ADR-0049's enforce-or-remove shape applies, and the answer is not this seat's to give.
What this does NOT claim
domain:speclabel set,state=all, since 2026-07-01 ⇒ 400 issues ([security] datasource credential in a nested config position is served in cleartext on read — redaction is top-level-key-only #13405–The ADR-0087 ledger entry for the aggregate x field-type refusal says "no non-temporal pair changes behaviour" — PR #17559 makes that false, andobjectstack migrate meta/ the upgrade guide are its only readers #17561) grepped forid != null(0),buildActionEngineFacade(3:ActionEngineFacade.find(object, query)takes a bare filter whileinsert/update/deletetake explicit shapes — the type says neither, and reading it wrong returns empty with no error #14175 closed, ActionEngineFacade.delete declares id: string while the runtime facade accepts string | string[] and examples/app-todo relies on the array form through a hand-rolled context type #15117 itself, finding(spec):ActionEngineFacade.find'sFilterConditionslot still admits the ObjectQL envelope{ where: … }at compile time — closing the bar is a vocabulary claim (no field namedwhere) the spec does not declare #15124 — a different member),action-execution.ts(6, none about this guard). Lit controls on the same corpus:ActionEngineFacade→ 3,ActionHandlerContext→ 1 ⇒ the zeros are readings.since=2026-07-01and the corpus ceiling The ADR-0087 ledger entry for the aggregate x field-type refusal says "no non-temporal pair changes behaviour" — PR #17559 makes that false, andobjectstack migrate meta/ the upgrade guide are its only readers #17561.domain:*.packages/runtime⇒ likelydomain:cli, ⛔ not this seat's call.Refs: #15117 / PR #17608 (where it was found, and the tolerance it did declare).
domain:specexecution seat ·session_01MkQhmuuJAVDjmeWNixwDDH· filed 2026-09-11T04:18ZGenerated by Claude Code