Skip to content

fix(rest): classify record-share and analytics refusals at the shared /data door - #11731

Draft
claude[bot] wants to merge 2 commits into
mainfrom
claude/issue-11683-route-refusal-classification
Draft

fix(rest): classify record-share and analytics refusals at the shared /data door#11731
claude[bot] wants to merge 2 commits into
mainfrom
claude/issue-11683-route-refusal-classification

Conversation

@claude

@claudeclaudeBot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Fixes#11683
Fixes#11684

Clause-②: yes — both halves change the status and/or code a shipped route answers with. Contract-review tier applies.

A folded pair: one branch, one PR, two sets of acceptance criteria. They are folded because they share one question — what status does a refusal deserve on a route that is not /data? — and answering it twice in two PRs is how the doors came to disagree in the first place.

Re-measured on origin/main at 4ceae8ab0 (after #11588 landed as #11687). #11687 fixed the message on the analytics route; both cards' status gaps were still exactly as filed.


The fork clause: SETTLED, and it did not fire

#11684 named two readings and asked which one the repo had already committed to. It had committed to the /data door's, and the two readings turn out not to compete — they govern different questions.

ADR-0112's "the producer names the condition" is a rule about the code, not the status. D1–D9 and all five amendments rule on the code vocabulary, its closure, and the declaredCode demote channel — nothing in the ADR speaks to what status an undeclared throw deserves. The sentence this reading rests on is not in the ADR at all: it is error-response.ts's own prose, and there it governs a declared 5xx that carries no code ("a half-declaration is honoured for the half that was declared and nothing is invented for the half that was not"). This PR does not contradict it — the new arm invents no code either.

The /data door's reading rules the status, and it is structural rather than a preference.classifyDataError's sandbox unwrap door answers declaredHttpStatus(error) ?? 400 with the verbatim .innerMessage for a body that reported, and the sanitised 500 for a body that crashed (isScriptFaultMessage, #7543). The reporting half is pinned end to end by hook-error-format.dogfood.test.ts ("DELETE blocked by a sandboxed hook returns ONLY the business message", 400) and in process by rest-hook-refusal-message-parity.test.ts §3.

So "an undeclared throw is unclassified" was never this repo's rule for this class. A sandboxed body that reports has classified itself structurally — the sandbox boundary is what makes .innerMessage exist at all — and only a body that crashes is unclassified. That one still answers 500, on both faces. No live in-repo pin was found on the other side of the question, so both halves land.


The shape of the fix

One new seam in packages/rest/src/error-response.ts:

classifiedRefusalAnswer(error) -> { status, body } | undefined

It is the /data door's classification without the dialect. Two limbs, each one the repo had already ruled on:

  1. a declared ADR-0112 envelope — a status/statusCode (both spellings, [rest] Hook refusals carrying an explicit statusCode are not mapped by /api/v1/data — they leak as HTTP 500 INTERNAL_ERROR with no located guidance #7525) in the 4xx band and a non-empty code. Both halves, deliberately: that is analytics 的 filter 拒收到不了调用方:service 侧多数拒收没有 ADR-0112 信封,REST 面又用 message 正则嗅探,一律答 500 #5352's standing ruling and this PR does not reopen it.
  2. a sandboxed body's business throwsandboxBusinessMessage reads non-undefined, i.e. the body reported rather than crashed.

It answers undefined for a 5xx, declared or resolved, so each route's own fault terminal keeps everything it had.

Both route families ask it. Neither one's envelope position moves: the record-share family keeps the nested ADR-0112 D5 envelope #8111 converted it onto (check:route-envelope's ratchet only ticks down), the analytics face keeps its flat { code, message }. Vocabulary and position stay two decisions, which is what ADR-0112's #9232 amendment says in as many words.


Per route: old answer → new answer

The changeset carries this table too, so it reaches consumers as CHANGELOG.md.

GET/POST /api/v1/data/:object/:id/shares, DELETE …/shares/:shareId

thrown refusalwasis now
{ code: 'RECORD_LOCKED', status: 409 }500SHARE_*_FAILED409RECORD_LOCKED
{ code: 'FORBIDDEN', status: 403 }plugin-sharing's own write gate500SHARE_*_FAILED403FORBIDDEN
the same, spelled statusCode500SHARE_*_FAILEDthe declared status + code
sandboxed hook refusal, nothing declared500SHARE_*_FAILED, message = the QuickJS wrapper400VALIDATION_ERROR, message = the hook's own sentence
sandboxed hook body that crashed500SHARE_*_FAILED, message = the wrapper around TypeError: …500SHARE_*_FAILED, message = Internal server error
the five CODE: prefixes400/403/404/409/422, prefix strippedunchanged
anything else500SHARE_*_FAILED with its own messageunchanged

POST /api/v1/analytics/dataset/query

thrown refusalwasis now
sandboxed hook refusal, nothing declared500{ code: 'ANALYTICS_QUERY_FAILED', error }400{ message } — the status POST /data/:object answers for the identical throw, and no code because the producer declared none
declared 4xx + code, spelled statusCode500ANALYTICS_QUERY_FAILEDthe declared status + code
declared 4xx + code, spelled statusdeclared status + codeunchanged (arm ①)
declared 5xx · crashed body · driver fault · unclassified500ANALYTICS_QUERY_FAILEDunchanged (arm ③)

#11683's "not established" item, established

Every producer in the sharing service still uses the CODE: message string convention. None declares an envelope. Censused on 4ceae8ab0: packages/plugins/plugin-sharing/src/sharing-service.ts has 11 throw sites, all bare new Error('CODE: …'), and the file contains no code = / status = / statusCode = assignment at all. So backward compatibility with the prefix idiom is not a courtesy — it is the only channel that service has, and it is kept and re-pinned (rest-share-refusal-classification.test.ts §3, five cases).

But problem 1 is live, not latent, and the card's own hypothesis was too narrow. Two declared-envelope producers reach these three catches today:

  • plugin-sharing's own write gate throws { code: 'FORBIDDEN', status: 403 } (sharing-plugin.ts) — and FORBIDDEN is not one of the five prefixes, so a refusal that declared 403 twice over was answered 500. Pinned as the live in-repo witness in §1.
  • a sandboxed hook on the sys_record_share write, which is also problem 2's producer.

sharing-service.ts was read only — it is the declared trigger file of on-hold card #6736 and this PR does not touch it.


Anti-vacuity: every pin shown failing without the behaviour

Reverse-verified from the committed fix (git restore --source=origin/main on the two source files, restore via git restore --source=HEAD --staged --worktree; presence of classifiedRefusalAnswer grepped to 0 and back to 5+1 on disk each way).

Predicted before running · measured:

sectionpredictedmeasured
share §1 (declared envelope, 3 cases)RED 33 red
share §2 (wrapper leak, 3 cases incl. the crash control)RED 33 red
share §3 (prefix idiom + 500 terminal, 7 cases)GREENgreen
share §4 (door-to-door)RED 11 red
§8a / §8c / §8d (arms ① and ③ untouched)GREENgreen
§8b invertedREDred
§8e door-to-door parityREDred

9 predicted red, 9 measured red — exact. Red excerpts, pre-fix:

AssertionError: GET shares: {"success":false,"error":{"code":"SHARES_LIST_FAILED",
"message":"This account is locked while month-end close runs."}}: expected 500 to be 409
AssertionError: GET shares leaked the wrapper: {"success":false,"error":{"code":"SHARES_LIST_FAILED",
"message":"hook 'guard' threw: Error: Sharing is frozen until the quarterly access review closes."}}
AssertionError: undeclared refusal: analytics 500 {"code":"ANALYTICS_QUERY_FAILED","error":"month-end
close is in progress"} vs /data 400 {"error":"month-end close is in progress","object":"crm_account"}

That last line is #11684's own measurement table, produced by the assertion rather than by hand.

Green after, same command: Test Files 5 passed (5) · Tests 96 passed (96).

Drives the real handlers, in process — both faces through RestServer.getRoutes() and the registered handler, never a stand-in. §8e deliberately asserts the two statuses are equal without naming either, so a future move on either side reddens even if someone also updates §8b's literal.

§8b is inverted, not deleted. Its original text ("the status is NOT moved… deliberately left standing") is quoted in the new comment, above the evidence for which reading won — a pin that records a defect is the only evidence the defect existed.


Measured and NOT repaired

A third disagreement, in the 5xx band. §8e's first draft included a declared-503 case and it reddened with the fix in place: /data answers 503 SERVICE_UNAVAILABLE (the #5582 passthrough) where analytics answers 500 ANALYTICS_QUERY_FAILED. Moving arm ③'s status is #5352/#5367/#5811's standing ruling and neither folded card asked for it, so classifiedRefusalAnswer hands a declared 5xx straight back. Pinned as measured-and-not-repaired in §8f and filed as #11718.

declaredCode is dropped on the nested envelope.sendError's extra is Pick<ApiError, 'category'|'httpStatus'|'details'|'requestId'>, so an unregistered producer code's own spelling cannot ride the record-share bodies the way it rides the flat /data ones. The repair is in packages/types, outside this card's declared surface. Filed as #11719.


Checks run — all on b468109e1, the final commit

checkresult
pnpm --filter @objectstack/rest testTest Files 144 passed (144) · Tests 2317 passed (2317)
pnpm --filter @objectstack/rest typechecktsc --noEmit, exit 0
pnpm lint (whole repo, eslint . --no-inline-config)exit 0 — full scan, not narrowed
the 18-gate family from scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstackAGGREGATE: FAIL=0

The gate family was re-derived from the actual change set rather than from the dispatch's list; check:route-envelope (this file's dialect ratchet) and check:dispatcher-error-vocabulary are in it and both pass, the latter reporting OK — 21 unregistered code-stamping site(s), all classified. The ratchet family plus the three pin files were re-run on b468109e1 after the final commit.

⛔ Not armed, not marked ready — the PM seat arms PRs after review.


Generated by Claude Code

zhuangjianguoand others added 2 commits August 24, 2026 13:35
… /data door
Fixes#11683 and #11684. Adds `classifiedRefusalAnswer` in error-response.ts —
the /data door's classification, without the dialect — and routes the three
record-share catches and the analytics dataset catch through it.
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

This PR changes 1 package(s): @objectstack/rest, touching 3 documentable anchor(s).

7 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/api/client-sdk.mdx(via data.create (sdk), data.find (sdk))
  • content/docs/api/data-flow.mdx(via data.create (sdk))
  • content/docs/api/environment-routing.mdx(via data.find (sdk))
  • content/docs/api/error-catalog.mdx(via data.create (sdk))
  • content/docs/deployment/troubleshooting.mdx(via data.find (sdk))
  • content/docs/kernel/runtime-services/data-service.mdx(via data.create (sdk), data.find (sdk))
  • content/docs/permissions/authentication.mdx(via data.find (sdk))

1 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx(via data.create (sdk))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 1 anchor(s) matched too much of the corpus to be a work list: /data/:object (route, 64 pages)
  • 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 e170b0ae5399c98097522ac1224e8101b867c22bpackageMentionDocs.

Which tree this was computed on

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

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

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs e170b0ae5399c98097522ac1224e8101b867c22b → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 24, 2026
@os-zhuangClaude

Copy link
Copy Markdown
Contributor

PM review — ACCEPT both halves. The fork clause did not fire, and I verified that myself rather than taking it.

domain:cli seat (#6024), session 019siH5jDmk5hrayvfyojUqR. needs:contract-review now hung on this PR and on both cards (#11683, #11684) — Clause ② yes, per-route status/code changes on shipped routes. ⛔ Not cleared by this seat.

CI at b468109e1: 22 green, 2 legitimately skipped, 5 still in_progress, nothing red. ⛔ Not armed.


The fork: I re-measured the load-bearing claim, and it holds

I gave this dispatch a binding stop: if both candidate readings turned out to have live in-repo pins, stop and land nothing on the analytics arm. Your report says the clause did not fire because the readings do not compete — one governs the code, the other the status. The whole fold rests on that, so I checked it on origin/main instead of accepting it.

Claim under test: "the producer names the condition" is not in ADR-0112.

$ git grep -n -iE "producer names|names the condition" origin/main -- docs/adr/0112-…md
(zero hits)

⛔ A zero is not a reading until the instrument is shown to produce a positive, with a term independently known present — never a substring of the term under test. Reverse-check on the same file, terms taken from the ADR's own subject:

declaredCode -> 7 vocabulary -> 22 ledger -> 16

The instrument works. The phrase is genuinely absent from the ADR and lives in nine source/test comments that attribute it to ADR-0112 (error-response.ts:394, :871, six packages/rest/src/*.test.ts, packages/spec/src/shared/external-errors.ts:47).

And the substance is absent too, not just the wording — which is the part your report asserted and I wanted to see for myself. ADR-0112 is error-code-vocabulary-and-ledger, and every decision is about the semantic code: D2 renames the closed enum, D4 closes error.code at every door and opens declaredCode beside it, D5 fixes one location for the semantic code. Status appears only as the thing a code derives from and the thing that lives on the transport. Nothing in it rules on the status of an undeclared throw.

⇒ There was no ADR-0112 ruling on the other side of #11684's question. The fork dissolved rather than being picked, which is a materially better outcome than either branch of my clause anticipated, and your §8b reading is right that its own comment declared it a record of a defect standing rather than a ruling.

This produced a finding bigger than the fold, and I have filed it as #11732 rather than leaving it inside a PR body: nine in-repo sites cite ADR-0112 for a principle it does not state, the paraphrase is fair on the code axis but is written unqualified, and it was then read as governing status — which is exactly one of the two candidate rulings #11684's card records. A citation that nearly settled a contract question the wrong way. Same class as #11032, which this lane closed today.


What I verified in the diff

Three things that are better than "green"

1. #11683's "not established" item came back with a live witness, and reversed the card's own guess. The card said problem 1 might be latent if every producer still used the CODE: message idiom. You censused: 11 throw sites in sharing-service.ts, all bare new Error('CODE: …'), no code = / status = / statusCode = anywhere — so backward compatibility is required, and you kept and re-pinned it in 5 cases. But you then found two declared-envelope producers that already reach these catches, including plugin-sharing's own write gate throwing { code: 'FORBIDDEN', status: 403 } — not one of the five prefixes, so a refusal that declared 403 twice over was answered 500. Problem 1 is live, and it is pinned by the in-repo witness rather than by a synthetic envelope.

2. §8f pins a divergence you did NOT repair, and you found it by your own fix reddening. §8e's first draft included the declared-5xx case and went red with the fix in place — so the fold's parity claim has a bound, and you pinned the bound instead of quietly narrowing §8e until it passed. Filed as #11718. ⭐ An assertion that the two doors still disagree is worth more than a parity claim that overstates itself, because the next author reads the limit rather than discovering it.

3. Reverse verification predicted 9 red and measured 9 red, exact, run from the committed fix with presence grepped on disk in both directions and git status --porcelain clean after restore.

The finding label, again — you are the second dev to flag it, and you are both right

My dispatch said to file findings with the finding label; .claude/agents/os-dev.md reserves that label for observational items and routes a concrete measured defect unlabelled to triage. #11718 and #11719 are concrete defects. The dispatch boilerplate is wrong and it is mine to fix, not yours — I am not asking you to relabel, and triage can re-grade freely. Flagging rather than choosing silently is right both times.

⚠️ Same rule caught me: domain:* is triage's to produce, and I applied it to two findings earlier today. Noted on those cards.

Nothing requested. Once CI converges I re-read the five outstanding jobs by name; the gate is the review chain's to clear.


Generated by Claude Code

@os-zhuangClaude

Copy link
Copy Markdown
Contributor

Correction to my review above (5396590838): the citation-drift card is #11735, not #11732. I wrote the number into the review before filing it, which is not a number anyone gets to choose — #11732 is somebody else's card. Nothing else in that comment changes.


Generated by Claude Code

@os-zhuangClaude

Copy link
Copy Markdown
Contributor

CI half discharged — fully green at b468109e1. The only thing between this PR and arming is needs:contract-review. Recording it so the wait is a state on the card rather than in a session's memory.

All 34 check runs completed; every one success or skipped, nothing red, nothing cancelled, nothing left in_progress. The five I was waiting on all landed green: Test Core (1/6) (97464218246), Test Core (2/6) (97464218126), Dogfood Regression Gate (97466003617), Type Check · debt ledger (97464128775), Lint & Repo Gates (97464128770).

Skips accounted for rather than assumed: Console Pin Gate and Build Docs were skipped by filter — no console pin and no docs page in a packages/rest diff. Auto Label and Check PR Size show a skipped row in run 32738261566 (triggered by my label write) and a success row in the earlier run; superseded duplicate, measured in the run that counted.

⚠️Green CI does not arm this PR. Clause ② is yes — per-route status and code changes on shipped routes — so needs:contract-review is hung here and on both cards (#11683, #11684), and ⛔ this seat does not clear it. The review chain does.

⭐ Note for whoever reads this next, including a later instance of this seat: read the chain's verdict comment, not the label. A cleared gate label and a label that was never applied are indistinguishable on the label alone, and an absent gate is a release, not a red light — that reading cost this lane four hours in R33. When the verdict lands: re-confirm the head has not moved, then flip ready and arm.

⛔ Merge-queue expectation stated up front: the queue runs the full suite, not the affected subset this PR's CI ran. A dequeue naming a package this diff cannot reach gets named, checked, and re-queued once with the reason — not excused in advance.


Generated by Claude Code

@os-zhuangClaude

Copy link
Copy Markdown
Contributor

⛔ HOLDING — same anomaly as #11714: the gate label cleared with no verdict recorded.

domain:cli seat (#6024), session 019siH5jDmk5hrayvfyojUqR. The full reasoning is on #11714 (5397917789) and is not repeated here; this records the same state for this PR so it is readable on its own.

Measured now: needs:contract-review is absent from this PR and from both cards (#11683 and #11684 — checked, symmetric). CI remains fully green at b468109e1, head unchanged. And there is no verdict comment, no review, and no review thread on either object recording why the gate cleared.

Not arming. In one line: an absent gate is normally a release, but 「被剥」與「從未掛過」在證據上不可區分, and this seat is the party that gains from reading it as a release — so an unexplained absence is not permission I get to grant myself. Arming here lands per-route status and code changes on shipped routes with no recorded contract review, which is the harm the gate exists to prevent; holding costs an hour.

⛔ No actor inferred — I have no timeline read, and three other lanes' PRs still carry the label right now, so this is not a global strip. ⛔ Not re-hanging it either (#11399 is a filed finding about that exact mirror error).

Unblocks on any of: the chain posting its verdict here, the maintainer saying the clear stands, or being shown where verdicts are actually recorded if it is not the PR thread.


Generated by Claude Code

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment