Skip to content

fix(rest): serve a sandboxed hook refusal's declared code at every status, not only where a bespoke arm caught it first - #11590

Merged
os-zhuang merged 2 commits into
mainfrom
claude/issue-10345-write-path-error-code
Aug 24, 2026
Merged

fix(rest): serve a sandboxed hook refusal's declared code at every status, not only where a bespoke arm caught it first#11590
os-zhuang merged 2 commits into
mainfrom
claude/issue-10345-write-path-error-code

Conversation

@claude

@claudeclaudeBot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Fixes#10345

The branch, named

The card deliberately made no claim about which function was responsible, and listed toRowApiError, a generic sendThrownError, and "a branch above them" as candidates. The answer is none of those:

packages/rest/src/error-response.tsclassifyDataError's sandbox unwrap door (if (typeof error?.innerMessage === 'string' && error.innerMessage), line ~619 on bcfe215d).

rest-server.ts owns none of it — it re-exports mapDataError and calls it from the route handlers; toRowApiError does not exist in this package. The door rendered from the raw error (the unwrapped innerMessage) and emitted no code at all, while every arm around it renders from the resolved envelope. That is the whole of the card's innerMessage correlation: the two always moved together because they are one branch, not because one causes the other.

It was never a policy about status, and never about conflicts

Measured in-process against the real route handlers on bcfe215d, before the fix:

thrown by the hookrouteHTTPcode?prose
VALIDATION_FAILED / 400POST /data/:obj400yes + fieldswrapper
DELETE_RESTRICTED / 409DELETE /data/:obj/:id409yeswrapper
RECORD_LOCKED / 409PATCH /data/:obj/:id409nounwrapped
DUPLICATE_VALUE / 409POST /data/:obj409nounwrapped
FORBIDDEN / 403POST /data/:obj403nounwrapped
VALUE_OUT_OF_RANGE / 400POST /data/:obj400nounwrapped
RECORD_LOCKED / 503PATCH /data/:obj/:id503yeswithheld
RECORD_LOCKED / 409, no innerMessagePATCH /data/:obj/:id409yesverbatim

The card's six rows reproduce exactly. The last three rows are the ones it could not have taken from outside, and they settle the question:

  • A declared 400 through the same door also loses its code. "Renders code only for 400" is an artifact of which codes have a bespoke arm above the doorDELETE_RESTRICTED and VALIDATION_FAILED do and never reach it, RECORD_LOCKED / DUPLICATE_VALUE / FORBIDDEN do not and did.
  • The same producer at 5xx kept its code, by falling through to the passthrough below. One sandboxed hook, its code surviving 503 and lost at 409, in one function.
  • The non-sandboxed twin — same code, same status, no innerMessage — always kept it. One property is the entire difference.

So the fix is at the branch, not per-status: the door now asks thrownCodeFields(error, status), the one definition the three sibling arms already use.

The fork the card named: is the demotion intentional?

The door carried a documented, deliberate omission — "Deliberately NO code field: older @objectstack/client builds (still bundled in deployed consoles) prepend any code to the human-readable message" — so this needed answering rather than assuming. It resolves against "deliberate narrowing", on measurement:

  1. The shipping client does the opposite by explicit rule.packages/client/src/index.ts's failure path: ".message is what UIs (e.g. the console's error toast) show to end users verbatim, so keep it to the server's human-readable message — no [ObjectStack] branding and no CODE: prefix. The code stays available programmatically via error.code."
  2. "Older bundled client, current server" is not a supported pairing.Decide when the client's three-location code probe retires — ADR-0112 D5 says "after batch 3", the code now says "legacy-server fallback, NOT debt" #4007 retired the compat read for exactly that combination: SDK and server ship on one release train (a changesets fixed group), and ADR-0112 renamed the code values anyway, so a code an old console could mis-render is one it could no longer match either.
  3. The rationale was already violated by its own neighbours.DELETE_RESTRICTED, VALIDATION_FAILED, PERMISSION_DENIED and the 4xx passthrough all ship a code beside a human-readable message, to the same consoles, on the same routes.
  4. The platform's sibling unwrap already does both.runtime/src/domains/actions.ts performs the same unwrap on the sameSandboxError and then exits through errorFromThrown, so the custom-action route has always answered with the business message and the code. Unwrapping and carrying the code were never alternatives.

What was deliberate was the omission's reason, and the reason expired. What was accidental is the status-shaped appearance, which nothing ever decided. ⚠️needs:contract-review rides on this PR and on the card, and this section is the part for that seat to rule on — it is not cleared here.

The routes the card never exercised — measured, not assumed

route familyexitshares the branch?
POST / PATCH / DELETE/data/:obj[/:id], POST /data/:obj/querymapDataError directlyyes — fixed here
batch, createMany, updateMany, deleteMany, :id/clonehandleRouteErrorresolveErrorResponsepartly — its passthrough is a status-ONLY read sitting above mapDataError, so these lost the code only through the statusCode spelling. That half is fixed here and pinned in §5; the status half already carried it and is pinned as a control.
POST /analytics/dataset/queryits own inline { code, message } envelopeno — separate branch, already renders code
custom actionsruntime's domains/actions.tserrorFromThrownno — separate branch, already renders code, and already unwraps

Filed separately rather than fixed here: #11588 — every route in the second and third rows ships the sandbox debug wrapper hook '<name>' threw: Error: … as the client-facing message, which is the exact string the unwrap door exists to suppress. Same envelope, different half, and the repair is an ordering question the #5437/#5582 passthrough docblock argued deliberately. Also filed: #11589 (a dogfood pin still states the retired client-compat rationale as live; its assertion is correct and green, only the comment expired).

Pins

New: packages/rest/src/rest-hook-refusal-code-parity.test.ts — 27 cases. Both directions and both status classes, asserting the body (code + status pairs per ADR-0112, never toThrow), with DELETE's working 409 as a live control in the same file, plus the non-sandboxed twin, the 5xx sibling, and the "invents nothing" / crash-is-still-500 / prose-rules controls.

Extended: rest-thrown-code-vocabulary.test.ts's ARMS table gains a fifth arm. The unwrap door emitted no code at all, so #9232 found nothing there to narrow and left it out — and an exit that never speaks the vocabulary is exactly the one a vocabulary sweep cannot see. Extending the shared table rather than pinning only in the new file is what closes the class.

Anti-vacuity

Baseline leg: only packages/rest/src/error-response.ts reverted to origin/main, the fix committed first, revert via git checkout origin/main -- <path>, restore via git checkout HEAD -- <path>, both under trap … EXIT INT TERM. Mutation proven on disk by grepping both texts, not by an editor exit code: injected thrownCodeFields(error, status)0 occurrences under mutation, removed Deliberately NO code field: older1; after restore, 1 and 0. No rebuild between legs, and none was needed — every symbol under test is reached by a relative import inside this package, which vitest transforms from source; the only exports-resolved workspace deps in the file (@objectstack/spec/api, @objectstack/types) are untouched by the mutation.

Measured: 18 red of 55. The prediction said 16, and the two misses are recorded in the test header rather than fitted to the result — one §3 case asserts the code as well as the status and therefore belongs with the defect pins (predicted as a control, it is not one), and §1 of the vocabulary suite contributes two assertions per arm rather than one.

Verification

All at e551f5b1 (the final commit), re-run after it.

  • pnpm --filter @objectstack/rest typecheck — clean; pnpm --filter @objectstack/rest testTest Files 141 passed (141) / Tests 2259 passed (2259).
  • pnpm lint (eslint . --no-inline-config, whole repo) — exit 0, no output. Not a narrowing.
  • Gate set derived with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack, not a hand-built list. All 17 matched families plus the 5 convention-triggered ones and check:nul-bytes, each quoted from its own verdict line:
    • check-dispatcher-error-vocabulary: OK — 21 unregistered code-stamping site(s), all classified; 1 awaiting a ledger entry (#8846).
    • ✓ Express-style response modules — 4 module(s) discovered and audited …: 2 conformant, 2 ratcheted, 0 exempt, 0 vendor-wire (check:route-envelope; the two ⚠ ratchet lines are the pre-existing query-allowlist.ts / query-multiplicity.ts entries, untouched here)
    • OK: 14 package(s) read outside themselves, all declared, and turbo.json hashes every declared glob. (check:cross-package-test-inputs, both spellings)
    • check-test-source-alias OK — 72 packages with tests scanned; 61 registered as still resolving a workspace dep through dist/
    • check-engine-double-contract: OK — 397 pinned, 133 in the DEBT ledger, 2 exempt.
    • ✓ where-matcher conformance holds: 292 matcher(s) discovered, 292 answer the combinator battery correctly or refuse it loudly
    • ✓ query-options-erasure ratchet holds: 67 unswept non-test site(s) in 17 file(s), none new
    • ✓ slot-lookup ratchet holds: 107 unswept site(s) in 25 file(s), none new
    • check-nul-bytes: OK (scanned 6483 text file(s) … no raw ASCII control bytes).
    • check-type-check-coverage: OK — 65/78 workspace packages type-checked … and ✓ No empty-frontmatter changeset introduced by this diff (1 declaring changeset(s) added). / ✓ This diff introduces no major bump.
  • pnpm check:type-check-debtrefused on first run"--re-measure cannot run: 32 workspace dependenc(ies) … have no built type entry point on disk" — which is NOT MEASURED, not a pass. Built what it needs (turbo run build --filter='./packages/*' --filter='./packages/*/*', 70/70 successful) and re-ran to a real verdict: check-type-check-coverage --re-measure: OK — 32 ledger entr(ies) re-measured in 222.5s, 1897 raw tsc error(s) total, none above its recorded number.
  • Downstream consumers (prefix direction — --filter '...@objectstack/rest' enumerates 16 dependants; three of them run the flat data envelope end to end): @objectstack/http-conformance 86/86, @objectstack/client 316/316, @objectstack/cli 1868/1868.

Clause ②

Clause-②: yes — this enriches a public response surface. Kept draft, needs:contract-review applied to the PR and still on the card, ⛔ not cleared by this seat, no ready-flip, no auto-merge, no enqueue.


Generated by Claude Code

…status, not only where a bespoke arm caught it first
`classifyDataError`'s sandbox unwrap door (`typeof error?.innerMessage ===
'string'`) rendered from the raw error and emitted no `code` at all, while
every arm around it renders from the resolved envelope. A hook throwing
`{ code: 'RECORD_LOCKED', status: 409 }` reached the client with the status and
no machine-readable code; `DELETE_RESTRICTED` at the same 409 kept its code
only because it has a bespoke arm above the door.
Never a status policy: the door dropped `code` on a declared 400 exactly as on
409, and kept it on 5xx by falling through to the passthrough. The code now
rides via `thrownCodeFields` — the one definition the three sibling arms use —
so the door joins the closed ADR-0112 vocabulary and invents nothing for a
producer that declared none.
Fixes#10345
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019siH5jDmk5hrayvfyojUqR
… it corrects
The prediction said 13 red in this file and 2 in the vocabulary suite; the
measurement was 15 and 3. Both misses are recorded rather than fitted: one §3
case asserts the code as well as the status and belongs with the defect pins,
and §1 of the vocabulary suite contributes two assertions per arm, not one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019siH5jDmk5hrayvfyojUqR
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

2 anchor(s) derived from 1 changed package(s); no hand-written page names any of them. ✅

What this run could not see
  • the SDK route bridge reached 45 of 222 client-bound route-ledger rows — the other 177 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run: node scripts/docs-audit/affected-docs.mjs --bridge-coverage

Coarse fallback — 13 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 67ceb9aeff873b135e7b87954dd14cdcaf853116packageMentionDocs.

Which tree this was computed on

This run read content/docs from 5426bb867f8e572ca30fcfc3366657aed2b5c89a — the merge of head e551f5b1661fd6f4c59437b23858c3385abc7ef4 into base 67ceb9aeff873b135e7b87954dd14cdcaf853116, 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 5426bb867f8e572ca30fcfc3366657aed2b5c89a && git checkout 5426bb867f8e572ca30fcfc3366657aed2b5c89a
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 67ceb9aeff873b135e7b87954dd14cdcaf853116 e551f5b1661fd6f4c59437b23858c3385abc7ef4 && git checkout -B drift-repro 67ceb9aeff873b135e7b87954dd14cdcaf853116 && git merge --no-ff e551f5b1661fd6f4c59437b23858c3385abc7ef4
node scripts/docs-audit/affected-docs.mjs --json 67ceb9aeff873b135e7b87954dd14cdcaf853116

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

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 24, 2026
@os-zhuang
os-zhuang marked this pull request as ready for review August 24, 2026 10:42
@os-zhuang
os-zhuang added this pull request to the merge queueAug 24, 2026
Merged via the queue into main with commit cad8b42Aug 24, 2026
38 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-10345-write-path-error-code branch August 24, 2026 10:52
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rest: the /api/v1/data write path renders code only for 400 — a 409 refusal reaches the client with the status but no machine-readable code

2 participants

@os-zhuang@claude