Skip to content

Commit 5a84d41

Browse files
os-zhuangclaude
andauthored
fix(automation,approvals): 屏幕流 resume 服务端校验 + 审批越权可审计 (#4477, #4466, #4469, #4470) (#4502)
* wip: rescue in-progress work before container restart loss (flows) * fix(automation): validate screen resume input against the node's declared fields (#4477) Changeset for the screen-resume field contract landed in the preceding commit (which the container restart forced out as a WIP rescue): `resume` now refuses a bag that violates the suspended screen's declared `fields` — a missing `required` value the caller WAS asked for, or a key the screen never declared — with the new `INVALID_SCREEN_INPUT` code, mapped to 400 by the automation domain route. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD * fix(approvals): record an admin override of a staffed approver slate as an override (#4466) `sys_approval_action` had no override column, so an admin overriding a properly staffed approver slate wrote a row byte-for-byte identical to the designated approver approving normally. A reader of the timeline saw `approve` by the admin and could not tell which had happened; the bypassed approver's `409 INVALID_STATE` was the only trace, and only if they happened to try. The platform knows at decision time — it took the `isOverrideActor` branch to admit the call — so this was dropped information, not unavailable information. Adds `sys_approval_action.via_override` (optional boolean), set on exactly the actions admitted by that branch: `decideNode`'s approve/reject and `reassign`'s admin rescue. Surfaced on `ApprovalActionRow`, returned by `listActions`, and added to `highlightFields` + two grid views. `true` = admitted only by the override branch. `false` = checked, not an override (an admin who IS a slot holder approves normally). Absent = a row predating the column — "not recorded" is not the claim "not an override", so `rowFromAction` maps `null` to `undefined`. Additive and nullable: no migration needed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD * fix(automation): type screen-input issue codes as FieldErrorCode, exempt the file from ADR-0112 D1 `check:error-code-casing` flagged `ScreenInputIssue.code`'s `'required' | 'unknown_field'` literal union as a lowercase `error.code`. It is not one: these are FIELD-ADDRESSED validator codes (ADR-0114 D2, ADR-0112 D6) carried inside the refusal's message, while the refusal's own machine code is the SCREAMING `INVALID_SCREEN_INPUT` the engine returns. Typed as the shared `FieldErrorCode` for the reason `ActionParamIssue` gives — a screen field, an action param and a record column must not drift into three vocabularies for the same two conditions — and registered in the checker's EXEMPT_FILES with its reason, beside the `action-params.zod.ts` precedent it mirrors exactly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD * fix(approvals,verify): find stranded terminal requests; stop the harness pinning memory (#4469, #4470) #4469 — `releaseDeadRunRequests` scans `status: 'pending'`, and the very step that zombifies a request is the one that takes it OUT of `pending`: breaking it removed it from the only sweeper's field of view. Its oracle could not have answered anyway — `getRun` reads the execution LOG, which returns null for a perfectly alive suspended run after a restart. Adds `ApprovalService.inspectStrandedRequests()`, a READ-ONLY inspection over `approved`/`rejected`/`returned` rows that uses BOTH oracles and reports only rows failing both: `hasSuspendedRun(runId) === false` (no live pause) AND `getRun(runId) == null` (no terminal history row either). A store that THROWS is skipped and counted as `undetermined`, never condemned — an outage means unknown. It never rewrites a decision that really happened; it reports which requests are stuck at which step and what the mirrored status field still reads, and rides the existing sweep clock so the finding surfaces without an operator going looking. #4470 — `packages/verify/src/harness.ts` pinned `suspendedRunStore: 'memory'`, making the DB-backed path structurally unreachable from every dogfood/e2e fixture. Engine-side persistence was unit-tested against a fake table and the approval chain e2e-tested wholly in memory, while the assembly between them was covered by nothing — the seam #4420 grew in. The harness now boots the plugin's own `'auto'` default (a fixture opts out explicitly), and gains `databaseFile` so two sequential boots over one file are a genuine cold start. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD * wip: rescue in-progress work before container restart loss * test(dogfood): assert what the durable suspended-run row actually contains (#4470) CI showed the harness store change (`'memory'` → the plugin's own `'auto'`) regressed nothing: 38 dogfood files and 239 tests passed, and the single failure was this file's own cold-boot assertion. Investigating that failure found a real blocker, so the proof is rewritten around what can be established rather than around what would be convenient. A second `bootStack` over the same `databaseFile` reads a database whose tables exist but whose ROWS are gone — ordinary records do not survive it either, which is what identifies it as a harness/driver persistence gap rather than a defect in the suspended-run store. #4470's third bullet is therefore documented as a KNOWN GAP in the file header, not faked with a weakened assertion. What is asserted instead is the substance of the issue's first two bullets, and all of it is a fact rather than the absence of an error: - `sys_automation_run` exists and is queryable (the table #4420 was missing); - the pause lands as a real `paused` row, read back by id, carrying every field a rehydration needs — `node_id`, the `node_type` the resume gate keys on, a parseable `variables_json` holding the trigger input, `steps_json`, and the `screen_json` a resumed pause validates against; - resuming CONSUMES that row and leaves the `run_`-prefixed terminal history row in its place — the zombie shape #4420 produced; - the screen contract (#4477) is enforced against the persisted `screen_json`, and a refusal leaves the durable row intact. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 2fa286c commit 5a84d41

22 files changed

Lines changed: 1698 additions & 10 deletions
Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
---
2+
"@objectstack/spec": patch
3+
"@objectstack/plugin-approvals": patch
4+
---
5+
6+
fix(approvals): record an admin override of a staffed approver slate AS an override (#4466)
7+
8+
An admin who is not in a request's `pending_approvers` may still act on it — the
9+
`#3424` privileged-override path exists so a request routed to an unstaffed
10+
position, or to approvers who have all left, is not undecidable forever. The
11+
override is defensible; what was not is what the audit trail recorded.
12+
13+
`sys_approval_action` had no override column at all. So an admin overriding a
14+
properly-staffed slate wrote a row **byte-for-byte identical** to the designated
15+
approver approving normally: a reader of the timeline saw `approve` by the admin
16+
and could not tell whether the admin *was* an approver or *overrode* the ones who
17+
were, and the bypassed approver's later `409 INVALID_STATE` was the only trace —
18+
existing only if they happened to try. The platform knows at decision time (it
19+
took the `isOverrideActor` branch to admit the call at all), so this was dropped
20+
information, not unavailable information. The whole point of an approval record
21+
is to answer "who authorized this, and were they entitled to?".
22+
23+
`sys_approval_action` now carries **`via_override`** (boolean, optional), set on
24+
exactly the actions admitted by that branch — `decideNode`'s approve/reject and
25+
`reassign`'s admin rescue. It is surfaced on `ApprovalActionRow.via_override`
26+
(`@objectstack/spec/contracts`), returned by `listActions`, and added to the
27+
object's `highlightFields` and two grid list views so a timeline can say
28+
"overrode the approver slate" instead of rendering it as an ordinary approval.
29+
30+
Three distinctions the column keeps apart deliberately:
31+
32+
- **`true`** — the actor held no slot in the slate and was admitted only by the
33+
override branch.
34+
- **`false`** — checked, and it was not an override. An admin who *is* a
35+
designated approver is approving normally and records `false`: the marker is
36+
about which branch admitted the call, not about whether the actor holds admin
37+
rights.
38+
- **absent** — a row written before this column existed. "Not recorded" is not
39+
the same claim as "not an override", so `rowFromAction` maps `null` to
40+
`undefined` rather than to `false`.
41+
42+
Additive and nullable, so this needs no data migration: existing rows keep
43+
working and simply read as unrecorded. Levelled `patch` rather than `minor`
44+
because nothing an author writes changes — but note it *is* an observable
45+
behaviour change on a read surface: `listActions` responses and the
46+
`sys_approval_action` grid views now carry a field consumers did not see before,
47+
and `sys_approval_action` gains a column on next schema sync.
Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
---
2+
"@objectstack/spec": patch
3+
"@objectstack/service-automation": patch
4+
"@objectstack/runtime": patch
5+
---
6+
7+
fix(automation): `resume` enforces the suspended screen's declared field contract (#4477)
8+
9+
A `screen` node's `config.fields` is a complete input contract — the author
10+
declares the keys, their `required`-ness, and (via `visibleWhen`) when a field
11+
is even asked for. The RENDER half honoured all of it: the paused result and
12+
`GET …/runs/:runId/screen` carry `required` and `visibleWhen` intact. There was
13+
no VALIDATION half — `POST …/runs/:runId/resume` folded whatever bag it was
14+
handed straight into the flow variables, so a caller that skipped the dialog and
15+
posted here directly was unconstrained by every `required` the author wrote.
16+
Missing required fields, and keys the screen never declared, all completed the
17+
run with `success: true`.
18+
19+
Screen flows are the one place where the declared field contract is the ONLY
20+
contract — no object schema sits behind a screen node to catch a bad bag
21+
downstream. The platform already enforces the analogous contract everywhere else
22+
this seam appears: action params (ADR-0104 D2), record writes (ADR-0113),
23+
approval `decisionOutputs` (#3447). This is that rule for screen resume, built in
24+
the same shape.
25+
26+
`resume` now refuses a non-conforming submission with the new
27+
`AutomationResult.code` `'INVALID_SCREEN_INPUT'` (a transport maps it to **400**,
28+
as the automation domain route now does) and an `Invalid screen input: …` message
29+
that names each violation and lists the declared field names. The refusal happens
30+
BEFORE the suspension is consumed, so the pause stays live and the legitimate
31+
submission still lands.
32+
33+
`visibleWhen` is evaluated against the SUBMITTED values first (layered over the
34+
run's variable snapshot), so a hidden field's `required` never fires — enforcing
35+
it would dead-end the run at a field the user was never shown, which is #3528
36+
reproduced server-side. A predicate that cannot be evaluated is logged and
37+
treated as hidden rather than visible: the client decides what the user saw, and
38+
a broken predicate is not evidence a field was on screen.
39+
40+
Scope, deliberately narrow — three shapes keep the historical pass-through:
41+
42+
- an **object-form** screen (`kind: 'object-form'`), whose `fields` is empty by
43+
construction because the client renders the object's own form and the write
44+
path enforces that object's `required` fields itself;
45+
- a **message-only** screen (`waitForInput: true`, no fields), which declares no
46+
keys and so constrains none — the same pass-through `enforceActionParams`
47+
gives a param-less action;
48+
- `signal.output`, the node-OUTPUT namespace, which belongs to the approval-style
49+
resume envelope rather than to the screen's collected-values channel.
Lines changed: 210 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,210 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* An admin override of a STAFFED approver slate is recorded as an override (#4466).
5+
*
6+
* The reproduction is the issue's: `showcase_dynamic_approval` stage 2 resolved
7+
* its `expression` approvers correctly to exactly one designated user, and the
8+
* admin — who was not in that slate — approved it anyway through the #3424
9+
* privileged path. The designated approver then got `409 INVALID_STATE`.
10+
*
11+
* The override itself is defensible; what was not is the audit trail. Before
12+
* this, `sys_approval_action` had no override column at all, so an admin
13+
* overriding a properly-staffed slate wrote a row byte-for-byte identical to the
14+
* designated approver approving normally. A reader of the timeline saw `approve`
15+
* by the admin and could not tell which had happened, and the bypassed
16+
* approver's 409 was the only trace — existing only if they happened to try.
17+
*
18+
* The platform KNOWS at decision time: it took the `isOverrideActor` branch to
19+
* admit the call. This was dropped information, not unavailable information.
20+
*/
21+
22+
import { describe, it, expect, beforeEach } from 'vitest';
23+
import { ApprovalService } from './approval-service.js';
24+
25+
interface FakeRow { [k: string]: any }
26+
27+
/** The same minimal engine shape `approval-service.test.ts` uses. */
28+
function makeFakeEngine() {
29+
const tables: Record<string, FakeRow[]> = {};
30+
const ensure = (n: string) => (tables[n] ??= []);
31+
function matches(row: FakeRow, filter: any): boolean {
32+
if (!filter || typeof filter !== 'object') return true;
33+
for (const [k, v] of Object.entries(filter)) {
34+
if (k === '$or') {
35+
if (!(v as any[]).some(sub => matches(row, sub))) return false;
36+
continue;
37+
}
38+
const rv = row[k];
39+
if (v != null && typeof v === 'object' && '$in' in (v as any)) {
40+
if (!(v as any).$in.includes(rv)) return false;
41+
continue;
42+
}
43+
if (rv !== v) return false;
44+
}
45+
return true;
46+
}
47+
return {
48+
_tables: tables,
49+
async find(object: string, options?: any) {
50+
const rows = ensure(object).filter(r => matches(r, options?.filter ?? options?.where));
51+
if (options?.orderBy?.[0]) {
52+
const { field, order } = options.orderBy[0];
53+
rows.sort((a, b) => {
54+
const av = a[field]; const bv = b[field];
55+
if (av === bv) return 0;
56+
const cmp = av > bv ? 1 : -1;
57+
return order === 'desc' ? -cmp : cmp;
58+
});
59+
}
60+
const start = options?.offset ?? 0;
61+
return rows.slice(start, start + (options?.limit ?? 1000));
62+
},
63+
async insert(object: string, data: any) { ensure(object).push({ ...data }); return { ...data }; },
64+
async update(object: string, idOrData: any, _opts?: any) {
65+
const data = typeof idOrData === 'object' ? idOrData : _opts;
66+
const id = typeof idOrData === 'object' ? idOrData.id : idOrData;
67+
const table = ensure(object);
68+
const i = table.findIndex(r => r.id === id);
69+
if (i >= 0) table[i] = { ...table[i], ...data };
70+
return table[i];
71+
},
72+
async delete(object: string, options?: any) {
73+
const table = ensure(object);
74+
const i = table.findIndex(r => r.id === (options?.where?.id ?? options?.id));
75+
if (i >= 0) table.splice(i, 1);
76+
return {};
77+
},
78+
registerHook() {}, unregisterHooksByPackage() { return 0; }, async fire() {},
79+
};
80+
}
81+
82+
const SYS = { isSystem: true, positions: [], permissions: [] } as any;
83+
const SUBMITTER = { userId: 'u1', tenantId: 't1', positions: [], permissions: [] } as any;
84+
/** The designated approver the slate actually names. */
85+
const DESIGNATED = { userId: 'u9', tenantId: 't1', positions: [], permissions: [] } as any;
86+
/** An admin who is NOT in the slate — the issue's actor. */
87+
const ADMIN = { userId: 'root', tenantId: 't1', positions: [], permissions: ['admin_full_access'] } as any;
88+
/** An admin who IS a designated approver — approving normally, not overriding. */
89+
const ADMIN_ON_SLATE = { userId: 'u9', tenantId: 't1', positions: [], permissions: ['admin_full_access'] } as any;
90+
91+
describe('approval override audit marker (#4466)', () => {
92+
let engine: ReturnType<typeof makeFakeEngine>;
93+
let svc: ApprovalService;
94+
let n = 0;
95+
const baseTime = new Date('2026-01-15T10:00:00Z').getTime();
96+
97+
beforeEach(() => {
98+
engine = makeFakeEngine();
99+
n = 0;
100+
svc = new ApprovalService({
101+
engine: engine as any,
102+
clock: { now: () => new Date(baseTime + (n++) * 1000) },
103+
});
104+
});
105+
106+
/** A request whose slate is properly STAFFED — one real, designated user. */
107+
const staffedInput = (extra: Record<string, any> = {}) => ({
108+
object: 'opportunity', recordId: 'opp1', runId: 'run_1', nodeId: 'co_sign',
109+
flowName: 'showcase_dynamic_approval',
110+
config: {
111+
approvers: [{ type: 'user' as const, value: 'u9' }],
112+
behavior: 'first_response' as const, lockRecord: true,
113+
},
114+
record: { id: 'opp1', amount: 100 },
115+
...extra,
116+
});
117+
118+
it('the repro: the slate names exactly one designated user, and the admin is not on it', async () => {
119+
const req = await svc.openNodeRequest(staffedInput(), SUBMITTER);
120+
expect(req.pending_approvers).toEqual(['u9']);
121+
expect(req.pending_approvers).not.toContain('root');
122+
});
123+
124+
it('marks an admin override of a STAFFED slate as `via_override`', async () => {
125+
const req = await svc.openNodeRequest(staffedInput(), SUBMITTER);
126+
await svc.decideNode(req.id, { decision: 'approve', actorId: 'root', comment: 'not mine' }, ADMIN);
127+
128+
const acts = await svc.listActions(req.id, SYS);
129+
const decision = acts.at(-1)!;
130+
expect(decision).toMatchObject({ action: 'approve', actor_id: 'root', comment: 'not mine' });
131+
// The bit the audit trail used to drop entirely.
132+
expect(decision.via_override).toBe(true);
133+
});
134+
135+
it('does NOT mark the designated approver’s own approval', async () => {
136+
const req = await svc.openNodeRequest(staffedInput(), SUBMITTER);
137+
await svc.decideNode(req.id, { decision: 'approve', actorId: 'u9' }, DESIGNATED);
138+
139+
const decision = (await svc.listActions(req.id, SYS)).at(-1)!;
140+
expect(decision).toMatchObject({ action: 'approve', actor_id: 'u9' });
141+
// An explicit `false`, not absent: "checked, and it was not an override" is
142+
// a different claim from a legacy row's "not recorded".
143+
expect(decision.via_override).toBe(false);
144+
});
145+
146+
it('does NOT mark an admin who is ALSO a designated approver — that is an ordinary approval', async () => {
147+
// The marker is about which BRANCH admitted the call, not about whether the
148+
// actor happens to hold admin rights. Collapsing the two would make every
149+
// admin's ordinary decision read as a slate bypass.
150+
const req = await svc.openNodeRequest(staffedInput(), SUBMITTER);
151+
await svc.decideNode(req.id, { decision: 'approve', actorId: 'u9' }, ADMIN_ON_SLATE);
152+
153+
expect((await svc.listActions(req.id, SYS)).at(-1)!.via_override).toBe(false);
154+
});
155+
156+
it('marks an override REJECTION too — the direction of the decision is irrelevant', async () => {
157+
const req = await svc.openNodeRequest(staffedInput(), SUBMITTER);
158+
await svc.decideNode(req.id, { decision: 'reject', actorId: 'root' }, ADMIN);
159+
160+
const decision = (await svc.listActions(req.id, SYS)).at(-1)!;
161+
expect(decision).toMatchObject({ action: 'reject', via_override: true });
162+
});
163+
164+
it('the override and the ordinary approval are no longer identical rows', async () => {
165+
// The issue's core claim, asserted directly: before the column, these two
166+
// decisions differed only in `actor_id` — and an `actor_id` alone cannot
167+
// answer "were they entitled to?".
168+
const a = await svc.openNodeRequest(staffedInput(), SUBMITTER);
169+
await svc.decideNode(a.id, { decision: 'approve', actorId: 'root' }, ADMIN);
170+
const overrideRow = (await svc.listActions(a.id, SYS)).at(-1)!;
171+
172+
const b = await svc.openNodeRequest(
173+
staffedInput({ recordId: 'opp2', runId: 'run_2', record: { id: 'opp2', amount: 100 } }),
174+
SUBMITTER,
175+
);
176+
await svc.decideNode(b.id, { decision: 'approve', actorId: 'u9' }, DESIGNATED);
177+
const normalRow = (await svc.listActions(b.id, SYS)).at(-1)!;
178+
179+
expect(overrideRow.action).toBe(normalRow.action);
180+
expect(overrideRow.via_override).not.toBe(normalRow.via_override);
181+
});
182+
183+
it('marks an admin RESCUE reassign of a slate they hold no slot in', async () => {
184+
// #3424's other privileged action: handing a stuck (or staffed) request to
185+
// a real approver. Same fact, same column.
186+
const req = await svc.openNodeRequest(staffedInput(), SUBMITTER);
187+
await svc.reassign(req.id, { actorId: 'root', to: 'u7' }, ADMIN);
188+
189+
const row = (await svc.listActions(req.id, SYS)).at(-1)!;
190+
expect(row).toMatchObject({ action: 'reassign', reassign_to: 'u7', via_override: true });
191+
});
192+
193+
it('does NOT mark a slot holder handing over their own slot', async () => {
194+
const req = await svc.openNodeRequest(staffedInput(), SUBMITTER);
195+
await svc.reassign(req.id, { actorId: 'u9', to: 'u7' }, DESIGNATED);
196+
197+
expect((await svc.listActions(req.id, SYS)).at(-1)!.via_override).toBe(false);
198+
});
199+
200+
it('a legacy row (written before the column existed) reads as UNRECORDED, not as "not an override"', async () => {
201+
const req = await svc.openNodeRequest(staffedInput(), SUBMITTER);
202+
// Simulate a row persisted by an older build: the column is simply absent.
203+
const rows = engine._tables['sys_approval_action'];
204+
delete rows[rows.length - 1].via_override;
205+
206+
const row = (await svc.listActions(req.id, SYS)).at(-1)!;
207+
expect(row.via_override).toBeUndefined();
208+
expect(row.via_override).not.toBe(false);
209+
});
210+
});

0 commit comments

Comments
 (0)