Repository navigation
Commit 4ff18d3
fix(metadata-protocol): a batch row answers a sandboxed hook's refusal in the hook's words (#24631)
Fixes #24561
Clause-②: no
## What was wrong
A sandboxed `beforeInsert` hook body can refuse a row with `throw new
Error('QA-B: locked codes cannot be created.')`. On `POST
/api/v1/data/:object`, `/createMany` and `/import` the caller gets that
sentence. On `POST /api/v1/data/:object/batch` the row said `{ code:
'INTERNAL_ERROR', message: 'The create of this record failed. The reason
is in the server log.' }`, and the server logged the withhold.
The batch row sink (`toRowApiError` and `clientFacingRowFailureText` in
`packages/metadata-protocol/src/protocol.ts`) disclosed a row's text
only when the thrown error resolved to a 4xx. A sandboxed body's bare
`throw` declares no status, so the row withheld it as a fault. The
QuickJS runner puts the body's sentence on `SandboxError.innerMessage`
for the caller and keeps the `hook 'NAME' threw: ...` debug wrapper on
`.message` for the log. Every sibling door reads the sentence through
`sandboxBusinessMessage` (`@objectstack/types`).
## What changed
The change is in one place: the batch row sink in `protocol.ts`. The
per-row catches of `batchData`, `updateManyData` and `deleteManyData`
all go through it.
- New private `sandboxRefusalSentence(err)`. It is
`sandboxBusinessMessage(err)`, the read the import runner makes, and it
declines a refusal that declared a 5xx. That band test is the doors'
own: `declaredHttpStatus`, with the declared status below 500, as in
`classifyDataError`.
- `clientFacingRowFailureText` asks it first. After that, a sandbox
origin's `.message` is never quoted.
- `toRowApiError`'s code limb uses the `400` the doors serve when the
refusal declared no status. The code is `VALIDATION_ERROR`, the code
`/createMany` answers. A declared code or status still wins.
`httpStatus` is unchanged: it carries only a status the producer
declared, so none is stamped here.
The row for the card's request
(`{"operation":"create","records":[{"data":{"code":"LOCK-9"}}]}`,
refused by the hook):
| | code | message | httpStatus |
|---|---|---|---|
| before | INTERNAL_ERROR | The create of this record failed. The reason
is in the server log. | absent |
| after | VALIDATION_ERROR | QA-B: locked codes cannot be created. |
absent |
These stay withheld, with the `console.warn`, exactly as the doors
withhold them:
- a body that crashed;
- the runner's own fault (a budget, a denied capability);
- a refusal that declared a 5xx;
- any non-sandbox error without a declared 4xx.
An in-process hook's bare `Error` stays withheld too, even with the same
sentence. The read keys on the producer, not on the text.
### Bounded in-place fix, same function
Two more rows were measured red before the fix, at the same per-row
catch:
- **A sandboxed refusal that declared a 4xx** (`{ code: 'RECORD_LOCKED',
status: 409 }`). Its row quoted `.message`, which is the wrapper: `hook
'qa_lock_codes' threw: Error: QA-B: ...`. It now quotes the sentence,
with the declared code and status.
- **A crashed body that declared a 4xx** (`innerMessage: 'TypeError:
boom'`, `status: 400`). Its row quoted `hook 'qa_lock_codes' threw:
TypeError: boom`, where every data door answers `500` and withholds the
text. The text is now withheld. Its `code` and `httpStatus` are
unchanged; see Acceptance notes.
Both are the same defect class: the row sink's text disagrees with the
doors on a sandboxed error. Both sit in the claimed function, are pinned
in the same new file, and add no gate.
## Tests
The new
`packages/metadata-protocol/src/protocol.batch-row-sandbox-refusal.test.ts`
drives the real per-row catches with the runner's `SandboxError` shape:
- **§1, the card's create row:** `{ code: 'VALIDATION_ERROR', message:
SENTENCE }` and no `httpStatus`. The row parses against
`BatchOperationResultSchema`, no wrapper appears anywhere, and no
withhold warning is written. It equals `mapDataError`'s sentence, with
the code the door's status names.
- **§2, every loop:** batch create, update, upsert and delete,
`updateManyData` and `deleteManyData` give the same answer.
- **§3, declared code and status ride with the sentence:** 409
`RECORD_LOCKED`, `VALIDATION_FAILED` with fields, and an unregistered
code, which takes the 400 floor.
- **§4, the negative side:** each of these reads the fallback and warns
once with the original error: a crash, a declared 5xx, the runner's own
fault, a driver fault, and a crash with a declared 4xx. The control: the
same sentence thrown in-process stays withheld.
- **§5, NOT_ATTEMPTED and ROLLED_BACK siblings** carry the sentence,
never the wrapper.
- **§6, anti-vacuity:** the production recognisers and `mapDataError`
classify each double as the producer it stands for.
**Red before the fix.** The test was committed first (41c657b) and run
against the unfixed `protocol.ts`: `Tests 13 failed | 7 passed (20)`.
The §1 row was exactly the card's `{ code: 'INTERNAL_ERROR', message:
'The create of this record failed. ...' }`, and the log printed the
withhold line.
**Ablation** (one-shot, through `scripts/ablation-replace.mjs`, with the
restore proven). Removing the 5xx band from `sandboxRefusalSentence`
turned exactly §4's declared-5xx row red: `Tests 1 failed | 19 passed
(20)`, with `"message": "The ledger service is down."` where the
fallback was expected. After the restore, the blob equals HEAD and `git
diff HEAD` is empty. This was the one pinned behaviour the red-before
run could not prove, because that row was withheld before the fix too.
**At the final head 0c8dab6**, after merging `origin/main` 65a8eeb:
- `pnpm --filter @objectstack/metadata-protocol exec vitest run`: Test
Files 249 passed, 3 skipped (252); Tests 20751 passed, 19 skipped.
- `pnpm --filter @objectstack/metadata-protocol typecheck`: exit 0.
`--listFiles` shows the new file in the tsc program.
- `pnpm --filter @objectstack/spec check:generated`: all 14 artifacts
are current.
## Gates
Derived with `node scripts/pm/dispatch-gates.mjs --commands --repo
objectstack-ai/objectstack` at 0c8dab6: 4 paths against merge base
65a8eeb, 76 commands. All 76 ran at that head and exited 0. The
`--ran` reconciliation reads `76 derived, 76 run, 0 NOT-MEASURED, 0
UNRUN`.
- **`check:engine-double-contract`** was red once before this head. The
new pin's three fake-engine doubles (`delete`, `findOne`, `update`) were
missing from `scripts/engine-double-contract.pinned.json`. `--write`
added exactly those three rows and lost none, and the gate is green at
this head.
- **`check:dual-build-cjs-loads` and `check:lean-entry-closure`** first
answered PREREQUISITE NOT MET (exit 3) in a partly built tree. Both
measured green at this head: 111 require entry points across 69 packages
load, and the objectql lean entry closure holds at 15 packages. For
`dual-build-cjs-loads`, this package's `dist/` was rebuilt after the
merge, but the packages main's merge moved were read from a pre-merge
build. CI's fresh build is the reading of record for those.
- **CI-owned families** are read from this PR's check-runs: the rest of
`Lint & Repo Gates`, `Test Core`, `TypeScript Type Check` and `Build
Core`.
## Acceptance notes
- **The REST ledger pin cannot see this path.**
`packages/rest/src/rest-write-route-hook-refusal-sentence.ledger.test.ts`
lists `POST /api/v1/data/:object/batch` under ANSWERS_THE_SENTENCE. Its
double makes `protocol.batchData` reject as a whole, so the per-row
catch never runs, and the row passes there on the whole-request door's
answer. A real-path pin at that layer would need the real
`ObjectStackProtocolImplementation` (already a `@objectstack/rest`
dependency) over an engine double whose `insert` throws the sandbox
refusal. It would then assert `results[0].errors[0]` on the 200 body,
not the response status. That file belongs to another lane and is not
edited here. The pin in this PR drives the per-row catch directly, one
layer closer to the code.
- **`httpStatus` is not stamped for an undeclared sandboxed refusal.**
The row carries `code: 'VALIDATION_ERROR'` with no `httpStatus`, while
the doors answer HTTP 400. The limb carries only a status the producer
declared, and the dispatch scoped this card to the sentence and the
code. Whether a sandbox origin counts as a declared 400 for that limb,
the way the validation shape does, is a separate decision.
- **A crashed body that declared a 4xx keeps its row `code` and
`httpStatus`.** Its text is now withheld, but the row still says
`VALIDATION_ERROR` / `httpStatus: 400`, taken from the declared status,
while every door answers `500 INTERNAL_ERROR`. Withdrawing a declared
status from the row is the different decision that `toRowApiError`'s doc
names. The only producer is a body that sets `.status` on a native error
before throwing it.
- **The three loops share the sink**, so rows from `updateMany` and
`deleteMany` change the same way as `/batch`.
---
_Generated by [Claude
Code](https://claude.ai/code/session_01AHeUBbe6S3JRawcpbLejXD)_
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent b12e16a commit 4ff18d3
4 files changed
Lines changed: 472 additions & 4 deletions
File tree
- .changeset
- packages/metadata-protocol/src
- scripts
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
0 commit comments