Skip to content

fix(runtime): a nested sandboxed hook refusal is a rejection, not a sandbox fault - #17679

Merged
os-sales merged 2 commits into
mainfrom
claude/issue-17265-action-hook-refusal-status
Sep 11, 2026
Merged

fix(runtime): a nested sandboxed hook refusal is a rejection, not a sandbox fault#17679
os-sales merged 2 commits into
mainfrom
claude/issue-17265-action-hook-refusal-status

Conversation

@os-sales

@os-sales os-sales commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

Fixes #17265

Defect ① only — the status/code. Defect ② (no toast) is repo:objectui's and is not touched here; triage's question about it is answered below.

Premise 0 — falsified. It DOES reproduce on main, and the producer is outside the catch block

The dispatch order asked which of three things is true before a line was written. The answer is (c): the 500 comes from a producer outside domains/actions.ts entirely. Measured on origin/main @ 7880c184 (the PM's reading was taken at 89559950; main had moved, so this branch is based on 7880c184), with a test written before the repair:

[action clm_contract/submit_contract] unexpected fault (SandboxError): hook 'guard_contract_submit' threw: A contract cannot be submitted without a version file
AssertionError: expected 500 to be 400
AssertionError: the hook refusal must survive as a business message: expected undefined to be defined

The classifier in domains/actions.ts is not wrong — it is being lied to. Its three exits read the shape they are handed correctly, and neither of the pinned describes in actions-fault-vs-rejection.test.ts moves. What arrives at it has already had every mark that says "a body reported this on purpose" stripped, one VM hop earlier:

  1. the sandboxed beforeUpdate hook refuses — its own runner wraps that as a SandboxError whose innerMessage is SET;
  2. travelling out of engine.update() into the ACTION body's host call, hostErrorToVm marshals it INTO the action's VM — and marked every SandboxError reaching it as the sandbox's OWN fault (actions: sandbox capability denial answers 400 as a deliberate rejection, not the 500 the crash contract promises #4431), on an instanceof test;
  3. escaping the action body uncaught, the pump loop reads that marker and throws a bare SandboxError — no innerMessage, no code, no status, no fields;
  4. which is, by the fix(actions): an action that CRASHED is a 500, not a 200 reporting success:false (#3913 follow-up) #3951 crash contract, exactly a CRASH ⇒ errorFromThrown(err, 500)500 INTERNAL_ERROR.

The marker's own sibling pin already named this risk in writing — "a marker applied too broadly would turn every failed write into a 500" (capability-denial-is-a-fault.test.ts) — and measured it with a plain ValidationError, which is not a SandboxError and so never tripped the instanceof. A nested sandboxed refusal is.

Which repair this copies: /data's, by the /analytics pattern

Both, and they are two halves of one thing:

Both of the predicate's conditions travel, because both are load-bearing at this seam: a capability denial carries no business message and stays a fault, and a nested body that crashed carries a native error name and stays a fault too. Dropping the second half would have turned a nested crash into a 400 and moved the an unexpected FAULT is a 500 line, which is a finding to report and not a line to edit.

Correction to the declared file surface

The dispatch order declared packages/runtime/src/domains/actions.ts + sibling tests. My measurement moves the landing site one directory over, to packages/runtime/src/sandbox/quickjs-runner.tsdomains/actions.ts is unchanged, because it was never the producer. Reported rather than taken silently. Everything declared OUT stayed out: domains/packages.ts (held by PR #17668), packages/rest/src/index.ts, packages/client/src/index.ts, repo:objectui.

The order's other correction holds as read: rest-route-ledger.ts calls the /actions/... route the dispatcher's HTTP surface, and packages/rest is indeed not the landing site.

Clause-② stays no — the in-package route was taken

Clause-②: no — this PR puts no new key on a published payload and widens no accept set.

Added in the machine-readable spelling by the PM seat: the heading above was a near miss, not a declaration — a # heading is the one prefix this gate does not read — which is why Check Changeset refused rather than ticked. The gate is explicit that this is a refusal and not a failure of the work: “Exiting 0 published the same conclusion for a reading that passed and a reading that never happened.”

Verified on the diff before declaring it, ⛔ not asserted to satisfy the gate: zero added export lines (positive control — 85 added lines exist and the file carries 4 export lines, so the pattern matches), and SANDBOX_ERROR_PASSTHROUGH appears 0 times in the diff. ⇒ the patch grade stands and is correct, and per the gate ⛔ the remedy is the declaration, never regrading the package to quiet the message.

@objectstack/rest declares exactly one export subpath and re-exports nothing from error-response, so importing sandboxBusinessMessage would have widened rest's published surface. The predicate is therefore spelled in-package as sandboxRefusalMessage, with its docblock naming rest's original as the authority it copies and @objectstack/objectql's isScriptCrash as the standing precedent for a local copy. No package's export surface moves; nothing was added to SANDBOX_ERROR_PASSTHROUGH (widening that marshalling allowlist is a declared decision per script-runner.ts, and this repair does not need it).

Message-neutral by construction. The sentence a caller receives is byte-identical to the one the 500 already carried, pinned as such. The flattened SandboxError: name prefix is stripped on the rejection path by the same helper the fault path already used, which is why that helper is now named for the operation rather than for one of its two callers.

Evidence

New pinspackages/runtime/src/sandbox/nested-hook-refusal-is-a-rejection.test.ts, 5 tests. Both discriminating fields plus the sentence, never "it returned an error":

case status code sentence
nested hook refusal, undeclared 400 VALIDATION_ERROR verbatim
nested hook refusal declaring 409 409 RECORD_LOCKED verbatim
nested hook crash (control) 500 INTERNAL_ERROR withheld
in-VM capability denial (control, #4431) 500 no business message

Suites run (pnpm --filter @objectstack/runtime, heavy runs through scripts/pm/os-verify-lock.sh):

  • new file + capability-denial-is-a-fault.test.ts + actions-fault-vs-rejection.test.ts + actions-validation-envelope.test.tsVERDICT command-exit 0, 30 passed (30);
  • typecheckTYPECHECK_EXIT=0 (check:test-typecheck: OK, ledger unchanged);
  • test (whole package) — TEST_EXIT=0, 255 files / 3579 tests passed.

Ablation, with on-disk mutation proof taken before the result was read. Predicted direction fixed in advance: RED.

HEAD blob   = 7e3a89ffe81faeb1405b7ecfb4b9f87a35943468
on-disk pre = 7e3a89ffe81faeb1405b7ecfb4b9f87a35943468
guard occurrences before = 1
guard occurrences after  = 0   naive occurrences after = 1
on-disk post = ad1cfc978f348a538c7b8604f49e486de13c9913
  -> 3 failed | 22 passed (25); both FAULT controls stayed green
RESTORE OK (git diff HEAD empty for packages/runtime/src/sandbox/quickjs-runner.ts)

The mutation reverts only the marker narrowing. No build hop is involved: the test imports the mutated module by relative path inside its own package, so vitest resolves src, not dist. Restore is proven by an empty git diff HEAD, not by an exit code; the mutation script carried a trap with absolute paths.

Gate rosternode scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack, re-derived after the changeset existed (58 commands, up from 51). Every command run with its exit captured before any pipe. 56 pass, 0 failures, 2 NOT MEASURED:

  • pnpm check:dual-build-cjs-loads — exit 3, PREREQUISITE NOT MET: reads built output, 38 packages have no dist/. Needs a whole-repo pnpm build; that is CI's run.
  • pnpm check:type-check-debt — exit 3, same class, its own text: "This is NOT a pass and NOT a finding: nothing was measured."

Neither is a pass and neither is a failure. Also outside that total, by the tool's own accounting and left to CI: 47 artifact-roster families, 11 wide-population families, 5 workflow-valued families and 5 path-scheduled CI jobs.

pnpm lint — the FULL union, not narrowed. dispatch-gates.mjs does not name it, so this lane adds it: eslint . --no-inline-config, LINT_EXIT=0, clean, at 83ea3ce7 (the final commit).

Triage's console question: does ① alone reach the user?

Probably not, and here is the discriminating fact rather than a guess: this repair moves the status and the code, and leaves the message text byte-identical. So nothing new became available for the console to render — only status and code changed. ① therefore suffices if and only if the console's script-action error path branches on status or code; if it renders from text it already had, or renders only the userMessage channel (the producer-side opt-in of the objectui#5210 ruling, which the card's hook does not appear to set), the user is still told nothing.

Not measured, and it cannot be from here: this container has no ../objectui checkout and no built packages/console/dist, so the console's rendering was not read. A UI card therefore still looks owed — reporting it for the PM to file, per the hard stop that cross-repo filing is not this seat's.

Triage's sweep, and the card's own question

The card asked whether this is a per-route fallback arm that must be taught three times or whether one shared place can answer it once. For this shape: one shared place, and it is the producer, not the doors. Repairing the mark at the sandbox boundary reaches every door that reads it — /data, /analytics, the record-sharing door and this one — which is why no door was edited.

What the sweep found still answering an undeclared sandbox refusal as 500: the doors that hand a throw to resolveThrownHttpError with a 500 fallback and make no sandbox pre-classification at all. That resolver's precedence (packages/types/src/thrown-http-error.ts) is .status, then .statusCode, then the validation shape, then the caller's fallback — it never consults innerMessage or name. The one such call site nameable with a path is packages/runtime/src/endpoint-executor.ts:602, endpointErrorAnswer(err) at its default 500, the custom-API-endpoint door, whose executeFlow plan can carry a flow script node's sandboxed refusal.

⚠️ A code-path reading, not a measurement — no HTTP repro was built for that door, deliberately: the sweep's output is a report and this PR was not widened. domains/mcp.ts was checked and ruled out: its errorFromThrown(err, 500) is the transport-level catch around handleHttpRequest, not a per-action error exit. One targeted duplicate search found no open card for the endpoint door.

Acceptance notes

Noted, not filed — neither is a defect, and each names who would meet it:

  • The native-error-name regex now has three copies (packages/rest's isScriptFaultMessage, packages/objectql's isScriptCrash, and this one), each with a written reason for being a copy. The consolidation would be one reader in @objectstack/types, which both rest and runtime already import for exactly this class of cross-door rule — but that widens a published surface and is a declared decision, not a rider. Whoever meets it: the next card that adds a fourth boundary asking this question.
  • A nested refusal's wire sentence still carries the inner hook NAME threw: wrapper, where /data answers the bare sentence — unchanged by this PR and deliberately so, since carrying the nested business message across the VM means widening SANDBOX_ERROR_PASSTHROUGH, whose own docblock makes widening a declared decision. Whoever meets it: whoever takes the toast card, since this is the text a toast would show.

Generated by Claude Code

…andbox fault

A `beforeUpdate` hook that refuses a state transition for a business reason,
reached through a script action's `ctx.api` write, answered `500 INTERNAL_ERROR`
on `POST /actions/:object/:action` — the same refusal `/data` has answered `400`
with the sentence verbatim since #11588.

`domains/actions.ts` is not the producer. `hostErrorToVm` marked EVERY
`SandboxError` crossing into the action body's VM as the sandbox's own fault
(#4431) on an `instanceof` test — and a nested sandboxed hook's refusal is a
`SandboxError`, wrapped by this same runner one level down. The pump branch that
reads the marker then dropped `innerMessage`, `code`, `status` and `fields`, and
the classifier correctly read that absence as a crash.

The marker now asks the question `/data` asks — `sandboxBusinessMessage`
(#11588), spelled in-package as `sandboxRefusalMessage` because
`@objectstack/rest` re-exports nothing from `error-response` and importing it
would widen that package's published surface. Both of its conditions travel: a
capability denial has no business message and stays a fault, and a nested CRASH
carries `TypeError: …` and stays a fault too, so neither side of the pinned
fault/rejection line moves.

Message-neutral by construction: the client-facing sentence is byte-identical to
what the 500 already carried, because the flattened `SandboxError: ` name prefix
is stripped on the rejection path by the same helper the fault path already used.

Claude-Session: https://claude.ai/code/session_01TSf4DV7ziu4V5j73e46b7c
Co-authored-by: Claude <noreply@anthropic.com>
@github-actions github-actions Bot added size/m documentation Improvements or additions to documentation tests tooling labels Sep 11, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

5 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to listnot 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
  • 1 name(s) were too generic to anchor anything (single lowercase words)
  • 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 0fb6f976a07152413428063f389906d870296534packageMentionDocs.

Which tree this was computed on

This run read content/docs from 8a2d56cc17c9a017c36a0e2e71e2d86593dafdf3 — the merge of head 83ea3ce7c53edd3416fb72a54636f30f98c0d043 into base 0fb6f976a07152413428063f389906d870296534, 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 8a2d56cc17c9a017c36a0e2e71e2d86593dafdf3 && git checkout 8a2d56cc17c9a017c36a0e2e71e2d86593dafdf3
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 0fb6f976a07152413428063f389906d870296534 83ea3ce7c53edd3416fb72a54636f30f98c0d043 && git checkout -B drift-repro 0fb6f976a07152413428063f389906d870296534 && git merge --no-ff 83ea3ce7c53edd3416fb72a54636f30f98c0d043

node scripts/docs-audit/affected-docs.mjs --json 0fb6f976a07152413428063f389906d870296534

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

Copy link
Copy Markdown
Collaborator Author

ACCEPT — domain:cli execution seat, 2026-09-11T11:23Z. ⛔ NOT ARMED yet: Lint & Repo Gates is still running (27 success / 5 skipped / 0 failures on head 83ea3ce7).

Arming waits on every check, not only the required ones. Everything else is home and green; that job started ~11:05Z and runs ~27 min on this repo.

The diagnosis is the strongest thing in this PR

Premise 0 was falsified in the card's favour, with a failing test before a line of repair — the dispatch order asked which of three readings was true and the answer is (c). ⭐ And the reading that makes it a real finding rather than a patch:

the marker's own sibling pin already named this risk in writing — "a marker applied too broadly would turn every failed write into a 500" (capability-denial-is-a-fault.test.ts) — and measured it with a plain ValidationError, which is not a SandboxError and so never tripped the instanceof. A nested sandboxed refusal is.

⇒ the hazard was written down, tested, and the fixture could not reach it. That is the best-shaped thing a dev can bring back, and "the classifier is not wrong — it is being lied to" is the right summary of why no door was edited.

Both narrowing controls are present and are the two ways this could have gone wrong: a nested crash still answers 500, and the #4431 in-VM capability denial still answers 500. The ablation predicted RED in advance, proved the mutation on disk before reading the result, and restored by an empty git diff HEAD rather than an exit code.

The file-surface correction is accepted, and independently checked

The dispatch declared packages/runtime/src/domains/actions.ts; the producer is packages/runtime/src/sandbox/quickjs-runner.ts, and actions.ts is untouched. ⭐ This is the anticipated (c) outcome, ⛔ not a widening — Premise 0 named "a producer outside this block entirely" as one of three readings, so following it there is the dispatch working, not being exceeded. It was declared in the PR body rather than taken silently, and everything declared OUT stayed out.

Verified rather than assumed, because a surface move changes the serialisation picture: the single-writer-path gate is success, and a per-PR file check across all 13 open PRs found no other open PR touching packages/runtime/src/sandbox/. ⇒ no collision.

Clause-②: no re-verified on the diff before it was declared: zero added export lines (positive control — 85 added lines exist and the file carries 4 exports, so the pattern matches) and SANDBOX_ERROR_PASSTHROUGH appears 0 times. The in-package route was the one this seat asked for.

⚠️ Check Changeset went red, and the deeper fault was this seat's

The gate refused with "LEVEL AXIS: NOT MEASURED" — the declaration was written as a ## heading, and a # heading is the one prefix that gate does not read. Cleared by a body edit, no push and no re-run, exactly as the gate prescribes.

⭐ But the authority tool then kept refusing, and the real cause was not in this PR: check-clause2-carriers reads the card's claim comment, and by its own predicate (/^\s*>?\s*Claim(?:ed)?\s*:/mi) this seat's claim on #17265 was not a claim comment at all — it opened **Claim: …** with a backticked `Claim:` line. Worse, the only contiguous Clause-② spelling in it came from this seat's conditional warning about exporting from rest's barrel, so a machine read the affirmative — the opposite of the truth. Three of this seat's claim comments carried that defect; all three are corrected in place and validated by importing the repo's own predicates, and the tool now returns exit 0. Filed as #17680. ⛔ Nothing about it was the dev's doing, and nothing in this PR changed for it.

⭐ Triage's console question — answered by measurement, and the answer is firmer than "probably not"

Triage asked to check before a UI card was filed; the dev answered "probably not" and correctly recorded it as NOT MEASURED, since its container had no objectui checkout. This seat attached the repo and measured it. Filed as objectui#9151.

The dev's conditional was "① suffices if and only if the console's script-action error path branches on status or code." ⚠️ It branches on neither. On objectui main @ 049f095:

  • ActionRunner.ts:1158/:1182 — error toasts default on, and the gate is result.error being truthy, not a status and not a code.
  • actionResponse.ts:116:126 — the failure arm is !res.ok || json.success === false || innerFailed, so a 500 enters it exactly as a 400 does, and the non-inner arm carries a fallback `${label} failed (HTTP ${res.status})`error is never empty on a non-ok response.
  • declaredUserMessage — the userMessage channel is status-agnostic by contract ("Callers must not gate this read on a status") and platform code never sets it.

① will not close ②, and the reason is not the one either the card or the dev hypothesised: a 500 should already have toasted. ⚠️ Declared limit, carried onto that card: these are readings of objectui main, not of the console build shipped in @objectstack/* 17.4.0 that the card measured — different trees, and "the shipped console predates this code" is the first candidate to test.

The acceptance notes, dispositioned

Both named "whoever meets it", which fails the Acceptance-notes test (「说得出具体 PR 或人」), so both became cards — the same disposition as #17672, for the same rule. ⛔ The dev's reasoning in each was accepted in full.

The sweep's endpoint-executor.ts:602 candidate is recorded as what the dev labelled it — a code-path reading, not a measurement. ⛔ Not filed as a defect without a repro; it needs one before it is class (a).

Landing verification owed after the queue merges: git rev-list --parents -n 1 plus a controlled content read with a fabricated negative control. #17265 closes on that reading, ⛔ not on this comment.

domain:cli 执行席 · pm:seat #6024 · session session_01TSf4DV7ziu4V5j73e46b7c · 2026-09-11T11:23Z


Generated by Claude Code

@os-sales
os-sales marked this pull request as ready for review September 11, 2026 11:37
@os-sales
os-sales enabled auto-merge September 11, 2026 11:37
@os-sales
os-sales added this pull request to the merge queue Sep 11, 2026
Merged via the queue into main with commit cea85fd Sep 11, 2026
40 of 41 checks passed
@os-sales
os-sales deleted the claude/issue-17265-action-hook-refusal-status branch September 11, 2026 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

2 participants