Repository navigation
Commit b1cfa3d
fix(mcp): a coded refusal reaches the client with its code on the data tools, list_actions and the record resource (#23526)
Fixes #23509
Clause-②: yes
## What changes
The MCP data tools rendered a thrown refusal with
`errorResult(messageOf(err))`: `list_objects`, `describe_object`,
`validate_expression`, `query_records`, `aggregate_records`,
`get_record`, `create_record`, `update_record` and `delete_record`. So
did `list_actions`. The record resource
`objectstack://objects/{objectName}/records/{recordId}` wrote `{ error:
message }`.
So when a producer threw the ADR-0112 envelope, an MCP client received
the message and nothing else. The code is the machine-readable half the
client acts on, and it was lost for every coded refusal on these
surfaces:
- a held owner's key, `MFA_REQUIRED`. Ruling C′ on #23287, item 1: "a
held user's API key is refused at every seam with the same code";
- `RECORD_NOT_FOUND`;
- `PERMISSION_DENIED`;
- `INVALID_FIELD`;
- the exposure refusals.
**`packages/mcp/src/mcp-http-tools.ts`**
- The 10 catches now return `errorResultWithCode(err)`. It renders `{
error: { code, message, status } }`, the shape `run_action` already
answers with (chosen for the action tools in #15942), but **without
`details`**. The section below explains why.
- `run_action` and `resume_run` keep `errorResultFromThrown`. Their text
is byte-identical: same members, same key order.
- Both renderers read the throw through one function,
`thrownRefusalEnvelope(err)`. It is a named export of the module and is
not re-exported from the package index.
**`packages/mcp/src/mcp-server-runtime.ts`**
- The record resource's catch adds `code`, and `status` when the refusal
carries one, beside the unchanged `error` sentence.
**Unchanged**
- A throw with no non-empty string `code` renders exactly as today on
both surfaces.
- No code is minted. No refusal's message changes.
### FROM → TO: what an MCP client reads
| surface | refusal | FROM | TO |
|:--|:--|:--|:--|
| the 9 data tools and `list_actions` | coded | `isError: true`; the
text is the message | `isError: true`; the text is the JSON `{ "error":
{ "code", "message", "status" } }`. `status` appears only when the
refusal carries one. |
| record resource (stdio) | coded | `{ "error": MESSAGE }` | `{ "error":
MESSAGE, "code", "status" }`. `status` appears only when the refusal
carries one. `error` is the same string as before. |
| either | no code | the message | unchanged, byte-identical |
### Why `details` is withheld
I measured it at the real `/mcp` door, on the showcase stack with
security enabled. A `PERMISSION_DENIED` thrown by the security gate
(`plugin-security` `security-plugin.ts`, the denial with `positions` /
`permissionSets`) reaches the tool catch carrying the caller's
`positions` (`org_member`, `everyone`) and `permissionSets`
(`showcase_member_default`, `member_default`) in `details`.
The runtime dispatcher drops those `details` from the wire. Its catch in
`http-dispatcher.ts` reads "The denial's `details` does NOT reach the
wire" and records the ruling on #7450.
That policy lives in `@objectstack/runtime` and
`@objectstack/plugin-security`, and neither is a dependency of this
package. So these surfaces serve the members every boundary serves:
`code`, `message` and `status`. The card's done line asks for exactly
that: "with its `code` (and `status` where the envelope carries one)".
The first revision of this branch rendered `details`. The real-door
probe caught the disclosure, and the head commit removes it. Ten pins
plus an ablation now hold the withholding (below).
### Why the record resource keeps a flat shape
The tools take `errorResultFromThrown`'s nesting. The resource takes the
same members but not the nesting: it answers `{ error: MESSAGE, code,
status }`, not `{ error: { code, message, status } }`. Three reasons:
- The two sibling resources already answer in this flat vocabulary.
`objectstack://objects/{objectName}` answers `{ error, code, status }`
on a miss or an outage (#6055), and `objectstack://objects` answers the
same pair on a degraded listing (#6504).
- Two existing readers in this package tell a refusal from a row by
`typeof body.error === 'string'`. One is the normalizer in
`plugin.record-resource-exposure.test.ts`; the other is the member case
in `stdio-data-bridge.stored-metadata-body.test.ts`. A nested `error`
object would turn their refusal branch into "this is a row". That would
be a narrowing on a surface this PR declares as a widening.
- The dispatch scope for the resource ("carries the `code`, and the
`status` where the envelope has one") is met either way.
## Faces
- **stdio** serves the tools and the record resource. `MFA_REQUIRED` is
pinned on both, over a real `StdioServerTransport`.
- **HTTP `/mcp`** serves the tools only. `handleHttpRequest` registers
no resources: the record resource exists only on the long-lived server
(`plugin.ts:738`).
- A held key is refused at the dispatcher before the MCP server sees it.
That is the dispatch's mechanism note, and I did not re-measure it. So
the HTTP pins use refusals that do reach the tool catch.
- Measured at the real door (`POST /api/v1/mcp`, showcase stack, a
member caller), on the final head:
- `get_record` and `update_record` on another owner's row:
`RECORD_NOT_FOUND` / 404;
- `delete_record`: `PERMISSION_DENIED` / 403, with no `details`;
- `create_record` with an unknown field and `query_records` filtering on
one: `INVALID_FIELD` / 400.
- Each answered `isError: true` with `{ error: { code, message, status }
}`.
- The probe was a temporary file, deleted after the run and never
committed.
## Tests re-judged (none loosened, none deleted)
| test | refusal it pins | verdict |
|:--|:--|:--|
| `stdio-api-key-account-posture.pin.test.ts` (#23508's real-transport
pin) | `MFA_REQUIRED` / 403, coded | **moved to the coded rendering.**
`tools/call query_records` now asserts `{ error: { code: 'MFA_REQUIRED',
message, status: 403 } }`; it asserted the text equal to the message.
Extended with `resources/read` on the record over the same wire: `{
error: message, code: 'MFA_REQUIRED', status: 403 }`. |
| `plugin.record-resource-exposure.test.ts`, the `apiEnabled: false` and
the whitelist-excludes-`get` cases | `OBJECT_API_DISABLED` / 404,
`OBJECT_API_METHOD_NOT_ALLOWED` / 405, coded | **moved.** The normalizer
reads the code and status from both wires. The cases still assert the
same sentence on both paths, and now also the code and status. |
| `stdio-data-bridge.relation-condition-target-exposure.test.ts`, the
`query_records` tool case | `OBJECT_API_DISABLED` / 404, coded |
**moved.** It stayed green as a substring inside the JSON, so it was
re-judged rather than left as is. It now parses the envelope and asserts
the code, the status and both message fragments. |
| `mcp-server-runtime.http.test.ts` (`RLS: not permitted`, `Field read
denied`) | plain `Error`, no code | **code-less**, unchanged |
| `mcp-action-tools.test.ts` (`list_actions`: `metadata unavailable`) |
plain `Error`, no code | **code-less**, unchanged |
| `mcp-action-confirmation-member.test.ts` (`handler exploded`) | plain
`Error`, no code | **code-less**, unchanged |
| `mcp-validate-expression.test.ts` (`not found`),
`mcp-resume-run-tool.test.ts` (`runId is required`), every `system
object` case | an `errorResult` literal, never a throw | not a thrown
refusal, unchanged |
| `stdio-data-bridge.stored-metadata-body.test.ts`, the member case |
reads `typeof body.error === 'string'` | holds under the flat shape,
unchanged |
## Pins added: `packages/mcp/src/mcp-coded-refusal.test.ts` (42 cases)
- **HTTP face, every registered tool.** The pins run through the real
Streamable HTTP transport, one row per tool, each driven by its own
bridge member. The table is held equal to `tools/list`, so a tool added
later needs a row.
- The real `recordNotFoundError` renders as `{ error: { code:
'RECORD_NOT_FOUND', message, status: 404 } }` with `isError`.
- A code-less `Error` renders as its bare message, byte-identical.
- On the 10 rows other than `run_action` and `resume_run`, the security
gate's `PERMISSION_DENIED` renders as code, message and status, and the
`details` are withheld.
- **Boundaries.**
- The HTTP bridge's exposure gate throws `{ statusCode, message }` with
no `code`; it renders as the bare message, so nothing is invented.
- An empty or numeric `code`, or a thrown string, is not a code.
- A non-numeric `status` is omitted.
- **Record resource.**
- `MFA_REQUIRED` renders as `{ error, code, status }`.
- `PERMISSION_DENIED` renders as code and status, with `details`
withheld.
- A refusal with no `status` gets no `status`.
- A code-less throw answers `{ error }` byte-identically.
## Ablation (one-time; nothing committed)
All three legs ran on the final code. Each went through
`scripts/ablation-replace.mjs`: the anchor hit once, the blob changed,
and the restore was proven by blob == HEAD and an empty `git diff HEAD`.
The subject resolves through relative `src/` imports, not a dependency's
`dist/`, so no rebuild leg applies. Every leg turned red, the expected
direction.
1. **`query_records`' catch reverted to `errorResult(messageOf(err))`.**
Blob `b7a491d6641b → 6c8a41093f41`. **4 red:**
- the `query_records` `RECORD_NOT_FOUND` row;
- the `query_records` `PERMISSION_DENIED` row;
- the non-numeric-status row;
- the stdio `MFA_REQUIRED` pin.
The code-less row stayed green, as it should. Restored to
`b7a491d6641b`.
2. **`errorResultWithCode` made to carry `details`.** Blob `b7a491d6641b
→ f80698dd3e44`. **10 red:** every `WITHHOLDS its details` row. Nothing
else went red. Restored.
3. **The record resource's envelope read disabled.** Blob `3ef6ce641eb8
→ 65c81c69b058`. **6 red:**
- the stdio `MFA_REQUIRED` resource read;
- the three coded resource rows;
- both #8266 parity cases.
The code-less resource row stayed green. Restored.
`mcp-http-tools.ts` is byte-identical between the head of legs 1 and 2
and the final head (`b7a491d6641b`). The merge of `main` touched no
`packages/mcp` file.
## Verification
All at `6c70ec74ab`.
- `pnpm --filter @objectstack/mcp test`: **38 files, 462 tests passed**,
`VERDICT command-exit 0`. That is 420 before this PR plus 42 new; the
three coded pins that asserted the message alone were moved, not
deleted.
- `pnpm --filter @objectstack/mcp typecheck`: `tsc --noEmit` passes, and
`check:test-typecheck: OK`. The 53 errors it reports are the ones held
in the shrink-only debt ledger, and none is in a touched file. All four
touched test files are in the `tsconfig.test.json` program, counted with
`--listFiles`.
- Dependency closure: `pnpm --filter '@objectstack/mcp^...' build`
exited 0. After the merge of `main`, `pnpm turbo run build
--filter='@objectstack/dogfood^...'` ran 65/65 tasks.
- Consumer tests that read MCP tool output, run on the final head:
`packages/qa/dogfood` `showcase-mcp-http-identity`,
`flow-door-elevated-start`, `showcase-mcp-self-connection` and
`showcase-declarative-mcp`, plus the temporary real-door probe. **5
files, 39 tests passed.**
- `packages/cli/test/package-install-local-handlers.integration.test.ts`
reads only `run_action` and a successful `list_actions`, paths this PR
does not change. It is integration tier and declared to CI.
- Gates: `node scripts/pm/dispatch-gates.mjs --commands --repo
objectstack-ai/objectstack` derived 63 families from the change set. I
ran all 63 locally on this head and each exited 0. The `--ran`
reconciliation, fed `command :: exit code` lines, reads: "63 derived, 63
run, 0 NOT-MEASURED, 0 UNRUN", a zero derived from the recorded codes.
- Verdict lines include `check-adr-0087-registration: this PR adds no
declared-breaking changeset (1 non-breaking changeset(s) seen)`, `No
empty-frontmatter changeset introduced by this diff`, `check-nul-bytes:
OK`, `doc authoring guard: … clean`, `check-test-source-alias OK`,
`check:dual-build-cjs-loads — 110 published require entry point(s)
across 68 package(s) load` and `check-lean-entry-closure: 2 published
condition(s) measured`.
- In an earlier round on a superseded head, the last two answered exit
3, PREREQUISITE NOT MET, because a concurrent build was rewriting
`dist/`. They were re-run and are exit 0 on this head.
- The repo-wide scans, with `pnpm lint` first, and the rest of the farm
run in CI.
## Acceptance notes
Two findings go to the seat in this PR's report and are not filed from
here:
1. **On `/mcp`, the bridge's exposure refusal carries no code.**
- `callData`'s gate on the addressed object
(`packages/runtime/src/action-execution.ts:304`) throws `{ statusCode,
message }` with no `code`, and `buildMcpBridge` calls `callData` for
every `/mcp` data verb.
- So after this PR, `get_record` / `query_records` on an object
declaring `apiEnabled: false` still renders the message alone on `/mcp`.
That case is pinned as code-less here: nothing is invented.
- The stdio face (`enforceApiExposure`) and REST (`rest-server.ts:510`)
answer `OBJECT_API_DISABLED`, and so does the same `callData`'s
relation-target refusal.
- The fix belongs at that producer, in `packages/runtime`, not in this
consumer.
2. **`run_action` / `resume_run` render `details` verbatim** through
`errorResultFromThrown` (#15942's contract, unchanged here).
- `resume_run`'s errors are built through the REST error builder.
- `run_action` loads its subject record through `callData` as the
caller. If a security-gate `PERMISSION_DENIED` escapes it, its
`positions` / `permissionSets` would reach the MCP client, while the
dispatcher withholds them.
- I have not measured this path.
Observations, not filed:
- The AI tool-registry bridge's catch (`registerToolFromDefinition` in
`mcp-server-runtime.ts`) also renders a throw as its message alone. No
coded producer was measured on that path.
- Members beside `code` / `message` / `status` are not rendered on these
surfaces: the stdio 405's `allowedOperations`, and the runtime refusal's
`allowed`, `object` and `param`. REST's 405 carries `allowed`.
- A refusal's `details` could help an AI self-correct, for example a
validation error's field list. Serving them would need the per-class
disclosure policy this package does not hold. Nothing pulls on it today.
---
_Generated by [Claude
Code](https://claude.ai/code/session_0121ooBYRmS8ziFXm9ai5YHm)_
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent 56147a8 commit b1cfa3d
7 files changed
Lines changed: 482 additions & 50 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
| 265 | + | |
| 266 | + | |
| 267 | + | |
| 268 | + | |
| 269 | + | |
| 270 | + | |
| 271 | + | |
| 272 | + | |
| 273 | + | |
| 274 | + | |
| 275 | + | |
| 276 | + | |
| 277 | + | |
| 278 | + | |
| 279 | + | |
| 280 | + | |
| 281 | + | |
| 282 | + | |
| 283 | + | |
| 284 | + | |
0 commit comments