Skip to content

Commit 3d3f60e

Browse files
os-warrenclaude
andauthored
fix(approvals,rest,types): a stranded decision publishes finalized / decision / runId / repairable beside its 500 (#15436)
* fix(approvals,rest,types): a stranded decision publishes finalized/decision/runId/repairable beside its 500 Maintainer ruling 2026-09-04, decision batch #37, option B. One `POST /api/v1/approvals/requests/{id}/reject` produced three coexisting outcomes: the caller read HTTP 500, the request row WAS terminal and had left the pending inbox, and the run was stranded. 500 alone reads as "the rejection did not happen", so callers retried against a durable decision. The status code does not move — the effect landing while the run strands is still a failure — and the door does not become atomic: the #13937 shape-4 ruling binds this door's own writes too, so no decision is ever rolled back. What changed is that the door stops discarding what the engine said. - `serviceResume` carries `AutomationResult.status` through. It read only success/code/error, and the stranded exit reports a status and NO code, so the repairability signal died one line before the envelope was built — a member with a producer and, until now, zero consumers. - `resumeRecordedOutcome` throws a carrier with `finalized` (the decision stands), `decision`, `runId`, and `repairable` derived from the engine's `'stranded'` discriminator. Absence of that stamp is `false`, never a default: a repair verb that would refuse is worse than no promise. - The REST approvals door forwards those four fields on `RESUME_FAILED` only, presence-gated — an error with no carrier answers exactly the body it always did. - `@objectstack/types` hosts the constructor and its recogniser in one module (the producer is a plugin; rest cannot import one), the same Home rule as the validation-failure pair beside it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y * test(approvals): route the new pin's engine double through ObjectQL's dispatch predicates `check:engine-double-contract` and `check:objectql-double-limit` both caught the new fake in `decision-strand-envelope.test.ts`: its `update()`/`delete()` hand-rolled the dispatch and its `find()` read the caller's bound by truthiness, so `limit: 0` would have returned the whole table. Both verbs now open with `assertEngineUpdateDispatch` / `assertEngineDeleteDispatch` and the bound is honoured by presence; the two new rows are recorded in the pinned ledger (`--write`, 2 added, 0 lost). `check:system-context-census` anchors are re-anchored by its own `--fix`: pure line rot from this branch's edits, uniform +5 on `rest-server.ts` (the import block) and +52 on `approval-service.ts` (the docblocks), which is what makes it rot rather than a finding. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y * chore(docs): regenerate the system-context census page from the merged tree The os-regen merge driver hands this page back rather than text-merging it — it is generated. Regenerated with `pnpm gen:system-context-census` after merging origin/main; the three re-anchors are all line rot in `packages/runtime/src/domains/actions.ts` (+10 from upstream commits), none of them this branch's own files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 19fa30c commit 3d3f60e

10 files changed

Lines changed: 719 additions & 11 deletions

File tree

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
---
2+
"@objectstack/types": minor
3+
"@objectstack/plugin-approvals": minor
4+
"@objectstack/rest": minor
5+
---
6+
7+
An approval decision that lands while its flow run strands now says so in fields, not only in prose.
8+
9+
`POST /api/v1/approvals/requests/{id}/reject` — and its sibling decision doors — could produce three coexisting outcomes from one call: the caller read HTTP 500, the request row **was** in its terminal status and had left the pending inbox, and the workflow run was stranded. A caller reading 500 has one honest inference available — "the rejection did not happen" — and it was the wrong one, so scripts and operators retried or escalated against a decision that was already durable. The only carrier of the truth was English prose in `error`, so finding the affected run meant regexing a run id out of a sentence, and nothing said whether that run could be repaired at all.
10+
11+
The 500 stays. A recorded decision whose flow never advances is still a failure and is still reported as one; the door does not become atomic and no decision is ever rolled back. What changed is that it stops discarding what the engine already said:
12+
13+
- **The `RESUME_FAILED` body gains four fields**, additively — `finalized` (always `true`: the decision stands), `decision`, `runId`, and `repairable`. Existing consumers see the same `code`, the same `error` and the same status.
14+
- **`repairable` carries the engine's own discriminator**`AutomationResult.status === 'stranded'`, the state stamped on exactly the exit that journals a repair snapshot. `false` is the answer for every other failure, including a lost run: absence of the signal is not repairability, and a repair verb that would refuse is worse than no promise.
15+
- **`serviceResume` carries `status`** through to the door. It previously read only `success` / `code` / `error`, and the stranded exit reports a `status` and no `code` at all — so the platform's own repairability signal died one line before the envelope was built.
16+
17+
`@objectstack/types` gains `strandedDecisionFailure` / `strandedDecisionDetails` and the `StrandedDecisionDetails` type — the constructor and its recogniser in one module, so the producing service and the REST door cannot drift. A `RESUME_FAILED` raised without that carrier answers exactly the body it always did; the door never synthesises the envelope.

content/docs/permissions/system-context.mdx

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ not on any flag.
6464
## How the flag is set
6565

6666
`isSystem` is **server-constructed and never client-supplied**. Inbound HTTP
67-
cannot set it (`packages/rest/src/rest-server.ts:1548`, `:1577`), and neither
67+
cannot set it (`packages/rest/src/rest-server.ts:1553`, `:1582`), and neither
6868
can an action body (`packages/runtime/src/domains/actions.ts:414`). It is
6969
written by internal callers only, as an option on the engine call:
7070

@@ -103,7 +103,7 @@ that silently does not happen.
103103
| 14 | MCP stdio bridge skips the object API-exposure gate | mcp | Get: the bridge reaches objects whose `apiEnabled` / `apiMethods` would refuse an external caller | `stdio-data-bridge.ts:246` |
104104
| 15 | **Read-audit rows are not written** | plugin-audit | Lose: the "a person opened this record" trail. `sudo()` keeps the caller's `userId`, so this flag is the only thing separating a human read from a platform one | `read-audit.ts:556` |
105105
| 16 | Approval snapshot payload redaction skipped | plugin-approvals | Get: the whole snapshot on `find` / `findOne` — the audit/replay channel. Lose: field-visibility redaction over approval payloads | `payload-redaction-middleware.ts:115` |
106-
| 17 | REST anonymous-deny seam satisfied | rest | Get: `enforceAuth` passes with no `userId`. Not reachable from the wire — `isSystem` is never set on an inbound request | `rest-server.ts:1580` |
106+
| 17 | REST anonymous-deny seam satisfied | rest | Get: `enforceAuth` passes with no `userId`. Not reachable from the wire — `isSystem` is never set on an inbound request | `rest-server.ts:1585` |
107107

108108
### 2. Write pipeline and data integrity
109109

@@ -145,7 +145,7 @@ The largest single consumer — **17 of the 106 sites**.
145145
|:--|:---|:---|:---|:---|
146146
| 40 | **Approval record lock released** — a locked record is writable | plugin-approvals | Get: engine self-writes (the status mirror) pass. Lose: the lock that stops edits while an approval is live. Note there is deliberately **no admin exemption** here — only `isSystem` | `lifecycle-hooks.ts:347` |
147147
| 41 | Delegation write guard bypassed | plugin-approvals | Get: service / seed / import may write delegation rows naming another delegator | `lifecycle-hooks.ts:570` |
148-
| 42 | Approval actor / submitter / pending-approver checks bypassed (8 sites) | plugin-approvals | Get: approve, reject, recall, reassign without being a pending approver or the submitter | `plugin-approvals/src/approval-service.ts:963`, `:1072`, `:3196`, `:3342`, `:3509`, `:3580`, `:3769`, `:3809` |
148+
| 42 | Approval actor / submitter / pending-approver checks bypassed (8 sites) | plugin-approvals | Get: approve, reject, recall, reassign without being a pending approver or the submitter | `plugin-approvals/src/approval-service.ts:963`, `:1072`, `:3248`, `:3396`, `:3564`, `:3635`, `:3824`, `:3864` |
149149
| 43 | Saved-report ownership is **assignable**, and an update may reassign it | plugin-reports | Get: `ownerId` from input is honoured. A non-system caller always owns what it creates and can never reassign | `plugin-reports/src/report-service.ts:404`, `:425` |
150150
| 44 | Saved-report access / export / mutation gates bypassed | plugin-reports | Get: read, bulk-export and overwrite any report | `plugin-reports/src/report-service.ts:343`, `:372`, `:447`, `:684` |
151151
| 45 | Attachment access hooks return early (insert + update + delete, and the read AST) | service-storage | Lose: attachment visibility scoping | `attachment-access-hooks.ts:300`, `:349`, `:448`, `:524` |
@@ -158,7 +158,7 @@ The largest single consumer — **17 of the 106 sites**.
158158
|:--|:---|:---|:---|:---|
159159
| 48 | Object API-exposure gate bypassed (`apiEnabled` / `apiMethods`) | runtime | Get: internal self-writes ignore exposure declarations — these govern **external** exposure, not engine self-writes | `action-execution.ts:138` |
160160
| 49 | Action `requiredPermissions` bypassed | runtime | Get: engine self-invocation runs any action | `action-execution.ts:401` |
161-
| 50 | `manage_metadata` bypassed on metadata writes | runtime, rest | Get: schema writes without the capability | `domains/meta.ts:471`, `:874`, `rest-server.ts:4967`, `:6381`, `:6629`, `:7060`, `:7253` |
161+
| 50 | `manage_metadata` bypassed on metadata writes | runtime, rest | Get: schema writes without the capability | `domains/meta.ts:471`, `:874`, `rest-server.ts:4972`, `:6386`, `:6634`, `:7065`, `:7258` |
162162
| 51 | The shared metadata-write verdict itself returns `allowed` | metadata-core | Get: the one function all of row 50's doors consult answers yes before any capability is examined | `meta-write-capability.ts:134` |
163163
| 52 | Anonymous-deny seam satisfied on the domain dispatchers and the package/federation routes | runtime, rest | Get: passes with no `userId` | `domains/actions.ts:421`, `domains/ai.ts:60`, `domains/automation.ts:989`, `domains/meta.ts:232`, `domains/security.ts:78`, `domains/packages.ts:422`, `external-datasource-routes.ts:302`, `package-routes.ts:97` |
164164
| 53 | MCP principal check satisfied | runtime | Get: MCP surface reachable with no user | `domains/mcp.ts:61` |
@@ -199,7 +199,7 @@ assuming `isSystem` covers it is a documented source of bugs.
199199
| "It preserves a supplied `updated_at` / `updated_by`" | **No.** That is `preserveAudit`, a separate opt-in — and an UPDATE-path exemption only | `field.zod.ts:1580` (#3493 / #6640) |
200200
| "It stamps `created_by`" | **No.** Audit stamping reads `userId` from the context. A user-less system write stamps nothing — that is today's behaviour, not an error | `runtime-identity.ts:280``281` |
201201
| "It bypasses every guard" | **No.** The last-admin guard applies to **every** context, `isSystem` included — the deprovision path that actually locks an org out is the system one | `last-admin-guard.ts:299` |
202-
| "A client can request it" | **No.** Never settable from inbound HTTP or from an action body | `rest-server.ts:1548`, `:1577`; `domains/actions.ts:414` |
202+
| "A client can request it" | **No.** Never settable from inbound HTTP or from an action body | `rest-server.ts:1553`, `:1582`; `domains/actions.ts:414` |
203203

204204
---
205205

packages/plugins/plugin-approvals/src/approval-service.ts

Lines changed: 60 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ import { ExpressionEngine, collectCelRootIdentifiers } from '@objectstack/formul
1818
// writer-local re-derivation here was rejected by name (Option B): it would be
1919
// a third answer to a question the codebase already answered two ways.
2020
import { createRecordOrganizationResolver, type RecordOrganizationResolver } from '@objectstack/metadata-core';
21-
import { keysetWalk } from '@objectstack/types';
21+
import { keysetWalk, strandedDecisionFailure } from '@objectstack/types';
2222
import {
2323
ADMIN_FULL_ACCESS,
2424
ORGANIZATION_ADMIN_GRANTS,
@@ -2665,15 +2665,17 @@ export class ApprovalService implements IApprovalService {
26652665
signal: { output?: Record<string, unknown>; branchLabel?: string },
26662666
): Promise<void> {
26672667
const result = await this.automation!.resume!(runId, { ...signal, [RESUME_AUTHORITY_SERVICE]: true });
2668-
const reported = result as { success?: boolean; code?: string; error?: string } | undefined;
2668+
const reported = result as
2669+
{ success?: boolean; code?: string; error?: string; status?: string } | undefined;
26692670
// Only an explicit `success: false` is a failure. An engine (or a test
26702671
// double) that returns nothing is reporting nothing, and has always meant
26712672
// "it ran".
26722673
if (reported && typeof reported === 'object' && reported.success === false) {
26732674
const err = new Error(
26742675
`resume of run '${runId}' failed${reported.code ? ` [${reported.code}]` : ''}: ${reported.error ?? 'unknown error'}`,
2675-
) as Error & { resumeCode?: string };
2676+
) as Error & { resumeCode?: string; resumeStatus?: string };
26762677
err.resumeCode = reported.code;
2678+
err.resumeStatus = reported.status;
26772679
throw err;
26782680
}
26792681
}
@@ -2683,6 +2685,23 @@ export class ApprovalService implements IApprovalService {
26832685
return (err as { resumeCode?: string } | undefined)?.resumeCode;
26842686
}
26852687

2688+
/**
2689+
* The engine's own run-state discriminator behind a {@link serviceResume}
2690+
* rejection — `AutomationResult.status` — if the engine reported one
2691+
* (#13807).
2692+
*
2693+
* Read as a SIBLING of {@link resumeCodeOf}, never as a substitute: the two
2694+
* answer different questions and the stranded exit proves they are not
2695+
* interchangeable. It reports `status: 'stranded'` and **no `code` at all**
2696+
* (`service-automation` `engine.ts`, the resume catch arm), so a door that
2697+
* reads only the code sees an unnamed failure and cannot tell a repairable
2698+
* strand from a dead run — which is how the platform's own repairability
2699+
* signal had a producer and zero consumers until this call site.
2700+
*/
2701+
private static resumeStatusOf(err: unknown): string | undefined {
2702+
return (err as { resumeStatus?: string } | undefined)?.resumeStatus;
2703+
}
2704+
26862705
/**
26872706
* Refuse an operation whose whole point is to advance a flow run when that
26882707
* run no longer exists — BEFORE anything is written down (#4420).
@@ -2788,14 +2807,38 @@ export class ApprovalService implements IApprovalService {
27882807
* which cannot throw without breaking every standalone deployment — it
27892808
* reports through `resumeError` instead.
27902809
*
2810+
* ## The throw is truthful, not merely loud (#13807)
2811+
*
2812+
* Maintainer ruling 2026-09-04 (decision batch #37, option B): this door
2813+
* KEEPS its status code — the effect landing while the run strands is still
2814+
* a failure and must still be reported as one — and stops discarding what
2815+
* the engine said. ⛔ Not "return 200", which the card forbids; ⛔ not
2816+
* atomic, because rolling a real human decision back is excluded by the
2817+
* #13937 shape-4 ruling, which binds this door's own writes too (a machine
2818+
* that re-armed strandings by itself would re-run the node that threw,
2819+
* forever, with nobody deciding it should).
2820+
*
2821+
* So the error carries {@link StrandedDecisionDetails} beside its prose:
2822+
* `finalized` (the decision stands), `decision`, `runId`, and `repairable`
2823+
* derived from the engine's `'stranded'` discriminator. Before this a caller
2824+
* had a 500 and a sentence — and 500 alone reads as "the rejection did not
2825+
* happen", which is the misreading that makes a caller retry or escalate
2826+
* against a decision that IS durable.
2827+
*
27912828
* @param what - how the recorded outcome reads in the error, e.g.
27922829
* `"the approve decision"`.
2830+
* @param decision - the outcome label for the machine-readable envelope
2831+
* (`'approve'` / `'reject'` / `'revise'` / `'resubmit'`). Passed
2832+
* explicitly rather than parsed back out of `what` or the signal: the
2833+
* prose is for humans and `output` is the flow's, and neither is a place
2834+
* to keep a wire value.
27932835
*/
27942836
private async resumeRecordedOutcome(
27952837
runId: string,
27962838
requestId: string,
27972839
what: string,
27982840
signal: { output?: Record<string, unknown>; branchLabel?: string },
2841+
decision: string,
27992842
): Promise<{ resumed: boolean; resumeError?: string }> {
28002843
const missing = this.missingRunCapability(runId, requestId, what, 'resume');
28012844
if (missing) return { resumed: false, resumeError: missing };
@@ -2810,12 +2853,20 @@ export class ApprovalService implements IApprovalService {
28102853
});
28112854
return { resumed: false, resumeError: reason };
28122855
}
2856+
// #13807: the engine's own discriminator decides `repairable`, never
2857+
// this door and never the message text. `'stranded'` is the ONE exit
2858+
// that journalled a repair snapshot, so it is the one exit an operator
2859+
// can act on; every other failure (a lost run, an engine too old to
2860+
// report a status) is honestly `false`.
2861+
const status = ApprovalService.resumeStatusOf(err);
2862+
const repairable = status === 'stranded';
28132863
this.logger?.error?.('[approvals] resume failed — the run is stranded', {
2814-
request: requestId, run: runId, outcome: what, error: reason,
2864+
request: requestId, run: runId, outcome: what, error: reason, status, repairable,
28152865
});
2816-
throw new Error(
2866+
throw strandedDecisionFailure(
28172867
`RESUME_FAILED: ${what} was recorded on request ${requestId}, but its flow run '${runId}' ` +
28182868
`could not be resumed and is now stranded: ${reason}`,
2869+
{ finalized: true, decision, runId, repairable },
28192870
);
28202871
}
28212872
}
@@ -2862,6 +2913,7 @@ export class ApprovalService implements IApprovalService {
28622913
// whitelist already rejects them; this is defense in depth).
28632914
output: { ...(result.outputs ?? {}), decision: result.decision, requestId },
28642915
},
2916+
result.decision,
28652917
);
28662918
resumed = outcome.resumed;
28672919
resumeError = outcome.resumeError;
@@ -3254,6 +3306,7 @@ export class ApprovalService implements IApprovalService {
32543306
branchLabel: APPROVAL_BRANCH_LABELS.reject,
32553307
output: { decision: 'reject', autoRejected: true, requestId },
32563308
},
3309+
'reject',
32573310
);
32583311
resumed = outcome.resumed;
32593312
resumeError = outcome.resumeError;
@@ -3295,6 +3348,7 @@ export class ApprovalService implements IApprovalService {
32953348
branchLabel: APPROVAL_BRANCH_LABELS.revise,
32963349
output: { decision: 'revise', requestId },
32973350
},
3351+
'revise',
32983352
);
32993353
resumed = outcome.resumed;
33003354
resumeError = outcome.resumeError;
@@ -3384,6 +3438,7 @@ export class ApprovalService implements IApprovalService {
33843438
branchLabel: APPROVAL_BRANCH_LABELS.resubmit,
33853439
output: { resubmitted: true, requestId },
33863440
},
3441+
'resubmit',
33873442
);
33883443
resumed = outcome.resumed;
33893444
resumeError = outcome.resumeError;

0 commit comments

Comments
 (0)