Repository navigation
Commit 0fb55fc
Fixes #23529
Clause-②: no
`run_action` and `resume_run` now withhold a `PERMISSION_DENIED`
refusal's `details`, as the data tools, the record resource, the runtime
dispatcher and the REST actions door already do (the ruling on #7450).
Every other coded refusal on the two tools keeps its `details`, so
#15942's confirmation contract is unchanged.
Dispatched by the PM loop, `domain:spec` seat 4, building round after
triage's regrade (comment 6177046363), claim 6177192097, session
`session_0121ooBYRmS8ziFXm9ai5YHm`.
## What changed
- `packages/mcp/src/mcp-http-tools.ts`, `errorResultFromThrown` only
(plus the one import it reads). A thrown refusal whose declared code is
the standard catalogue's `PERMISSION_DENIED` now renders through
`errorResultWithCode`, the function the data tools already render
through, so its `details` are withheld. The code is spelled from
`StandardErrorCode` (`@objectstack/spec/api`, already a dependency), not
by hand. Every other coded refusal keeps its `details`. The `run_action`
and `resume_run` catches are unchanged: both already call
`errorResultFromThrown`.
- `packages/mcp/src/mcp-action-refusal-details.test.ts`: the pin, on the
MCP HTTP face over the real Streamable HTTP transport, the way
`mcp-coded-refusal.test.ts` drives it.
- `.changeset/23529-mcp-action-tools-withhold-permission-details.md`:
`@objectstack/mcp` patch.
## Why
The security gate's `PERMISSION_DENIED` carries the caller's own
positions and permission-set names in `details`. The ruling on #7450
keeps those off the wire. The dispatcher drops them, the REST actions
door answers through `resolveThrownHttpError`'s closed `details` list,
and since #23509 the data tools render through `errorResultWithCode`,
which never reads `details`. The two action tools rendered through
`errorResultFromThrown`, which spread `details` verbatim into a tool
result. That tool result rides a JSON-RPC 200, so the dispatcher's
withholding never saw it.
Reach, read from source (reading 6176605181, re-read on this tree):
`executeDeclarativeUpdateAction` writes through `callData('update', …,
ec)` as the caller, and nothing between the security middleware and
`errorResultFromThrown` catches the gate's write-side refusal. The
subject load does not carry it: `loadActionSubjectRecord`'s bare catch
drops the throw and `refuseDeniedSubjectLoad` answers `RECORD_NOT_FOUND`
with no `details`.
## Route: the first choice measured, then the second
- **First choice, measured and not taken.** `resolveThrownHttpError`
(`@objectstack/types`) builds `details` from a closed list (non-string
`code`, `issues`, validation `fields`) and never reads the thrown
`details`. A probe on its built output:
```text
ACTION_CONFIRMATION_REQUIRED -> code ACTION_CONFIRMATION_REQUIRED status
428 details undefined
PERMISSION_DENIED -> code PERMISSION_DENIED status 403 details undefined
```
It withholds the denial, but it also drops the confirmation refusal's
retry `details`, which `mcp-action-confirmation-member.test.ts` and
`mcp-resume-run-tool.test.ts` pin. So it would break #15942.
- **Taken: the withholding keyed on the declared code, through an
existing export.** No key-name list. No second filter of `details`: the
class's answer is `errorResultWithCode`'s, reused. Every
`PermissionDeniedError` declares this code, because it is a class
constant. Every throw in `plugin-security` that puts positions or
permission sets in `details` is that class. A throw with no code never
reaches the `details` read.
- No other package is edited: `packages/runtime`, `packages/types` and
`plugin-security` are untouched, and so is `packages/mcp/src/plugin.ts`
(#23367 has it in flight).
## The pin: red on `main`, green after
The bridge is a double, because `@objectstack/mcp` does not depend on
`@objectstack/runtime`. It lets the caller read the subject row, and it
refuses both action tools with the gate's `PermissionDeniedError` shape.
The assertions: the tool error carries `code` `PERMISSION_DENIED` and
status 403. Then each position and permission-set name is checked
against the whole HTTP response body, not one key. A control row on each
tool keeps `ACTION_CONFIRMATION_REQUIRED`'s `details` served.
Run against `main`'s renderer (base `e1cefa58e3`, the pin added, no
fix):
```text
❯ src/mcp-action-refusal-details.test.ts (5 tests | 2 failed)
× run_action: PERMISSION_DENIED answers code, message and status, and the caller's grants appear nowhere
× resume_run: PERMISSION_DENIED answers code, message and status, and the caller's grants appear nowhere
+ "details": {
+ "object": "crm_case",
+ "operation": "update",
+ "permissionSets": [ "pin23529_set_case_read_only", "pin23529_set_member_default" ],
+ "positions": [ "pin23529_position_case_reader", "pin23529_position_everyone" ],
Tests 2 failed | 3 passed (5)
```
After the fix, the pin and the three suites that must not move,
unchanged:
```text
src/mcp-action-refusal-details.test.ts src/mcp-action-confirmation-member.test.ts src/mcp-coded-refusal.test.ts src/mcp-resume-run-tool.test.ts
Test Files 4 passed (4)
Tests 62 passed (62)
```
`mcp-action-confirmation-member.test.ts` and `mcp-coded-refusal.test.ts`
(its `RECORD_NOT_FOUND` rows included) are byte-unchanged by this PR.
**`resume_run` is covered by a pin, at the renderer.** It calls the same
`errorResultFromThrown`, and its row fails and passes with
`run_action`'s. By reading, nothing reaches it today: it makes no
caller-scope write of its own, and its subject re-read ends at
`RECORD_NOT_FOUND`. So its row guards the shared renderer, not a
measured producer.
## Ablation
`node scripts/ablation-replace.mjs`, wrap mode, under the verify lock.
The mutation deletes the guard line and the pin runs on the mutated
file. The subject resolves through the package's own `src/`, so no build
or `dist/` leg applies.
```text
ablation-replace: anchor x1 -> x0
ablation-replace: blob 232f1b5 -> c6feca3cd8d737389cf84d21191c717c7c5e149a
× run_action: PERMISSION_DENIED answers code, message and status, and the caller's grants appear nowhere
× resume_run: PERMISSION_DENIED answers code, message and status, and the caller's grants appear nowhere
Tests 2 failed | 3 passed (5)
ablation-replace: ok restored: blob == HEAD (232f1b5) and `git diff HEAD` is empty
```
The direction is the ordinary one: the two denial rows turn red and the
two confirmation controls stay green. That shows the guard is keyed on
the code and is not a blanket withhold. `git status` was clean after the
restore. `232f1b56c0` is still this file's blob at the head.
## Tests and gates, at head `03c6a098a3`
- `pnpm --filter @objectstack/mcp test`: `Test Files 39 passed (39)`,
`Tests 467 passed (467)`.
- `pnpm --filter @objectstack/mcp typecheck`: exit 0, and
`check:test-typecheck: OK` with no new debt. The new test file compiles
under `tsconfig.test.json`.
- `pnpm --filter @objectstack/mcp build`: exit 0. The CJS and ESM builds
both load. The CJS bundle requires `@objectstack/spec/api`, and it
resolves.
- `node scripts/pm/dispatch-gates.mjs --commands --repo
objectstack-ai/objectstack` derived 63 commands. All 63 were run, with
each exit code captured before any pipe. Reconciliation: `✓
dispatch-gates --ran: 63 derived famil(ies) accounted for — 63 run, 0
NOT-MEASURED (a DERIVED zero — all 63 recorded an exit code and none of
them is 3)`.
- Two gates first answered `PREREQUISITE NOT MET` (exit 3):
`check:lean-entry-closure` and `check:dual-build-cjs-loads`. I built
their prerequisites (`turbo run build`, all but one task a cache hit)
and re-ran them. Both exit 0, and those are the readings recorded.
- `main` was merged once before this PR opened. The one incoming commit
touched `packages/lint` only, and `@objectstack/lint` was rebuilt.
- CI owns the repo-wide scans and the families that run over a whole
root. Their check-runs on this PR are the evidence for them.
## Acceptance notes
- **Residual, noted and not filed (no carrier).** The withholding is
keyed on one code. Any other coded refusal's `details` still render
verbatim on these two tools. REST passes a thrown refusal's `details`
only through the closed list. No other producer that reaches these tools
puts caller authorization data in `details` today. A fail-closed
alternative is to serve only the confirmation refusal's `details`. It
was not taken because it would also drop `resume_run`'s REST-built run
artefacts on a failed resume, which narrows more than this card asks.
- **Script-handler rethrow (the reading's second note).** By reading,
`executeRegisteredAction` rethrows a handler's own error raw. A rethrown
`PermissionDeniedError` keeps its code, so this renderer-level fix
covers it with no producer change. No in-tree producer was found.
- **The comment above `errorResultWithCode` is untouched.** It sits
outside the claimed surface, and it still holds for that function.
`errorResultFromThrown`'s own doc names the one code-keyed exception and
why it is not a second copy of the policy.
- **What this pin does not measure.** It does not cover the runtime
layers between the gate and the bridge. Those rest on the reading and on
#23509's measured fixture, where the gate's `details` reach the MCP HTTP
face intact for a data verb.
- **The stdio face** serves no action tools today (the reading's R3).
The fix sits in the transport-agnostic renderer, so a host that bridged
an action seam onto stdio would get it too.
---
_Generated by [Claude
Code](https://claude.ai/code/session_0121ooBYRmS8ziFXm9ai5YHm)_
---------
Co-authored-by: Claude <noreply@anthropic.com>
1 parent b9b6189 commit 0fb55fc
3 files changed
Lines changed: 221 additions & 2 deletions
File tree
- .changeset
- packages/mcp/src
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 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 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
35 | 35 | | |
36 | 36 | | |
37 | 37 | | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
38 | 41 | | |
39 | 42 | | |
40 | 43 | | |
| |||
302 | 305 | | |
303 | 306 | | |
304 | 307 | | |
305 | | - | |
306 | | - | |
| 308 | + | |
307 | 309 | | |
308 | 310 | | |
309 | 311 | | |
310 | 312 | | |
| 313 | + | |
| 314 | + | |
| 315 | + | |
| 316 | + | |
| 317 | + | |
| 318 | + | |
| 319 | + | |
| 320 | + | |
| 321 | + | |
| 322 | + | |
| 323 | + | |
| 324 | + | |
| 325 | + | |
| 326 | + | |
| 327 | + | |
| 328 | + | |
| 329 | + | |
| 330 | + | |
| 331 | + | |
311 | 332 | | |
312 | 333 | | |
313 | 334 | | |
314 | 335 | | |
| 336 | + | |
315 | 337 | | |
316 | 338 | | |
317 | 339 | | |
| |||
0 commit comments