Skip to content

Commit c5ff96d

Browse files
authored
fix(approvals): a schedule-triggered run can write its own locked record (#3712) (#3749)
Closes #3712. Residual of #3456, left open by #3703. A run with no principal now passes provenance alone: `resolveRunDataContext` returns `{ flowRunId }` — no userId, positions, permissions, or `isSystem`. Every principal gate keys on one of those fields, so the context authorizes identically to no context at all; the run keeps its documented #1888 unscoped posture and only becomes attributable. Provenance moved out of the hook session into `ctx.provenance`, so an identity-less writer is never forced to present an empty session — which would have turned "no caller" into "an anonymous caller" and narrowed the #1888 fail-open for attachments alone. Also relaxes `BaseEngineOptionsSchema.context` to a partial envelope (`ExecutionContextInput`): parse-time defaults made positions/permissions/ isSystem required on a caller-supplied option, asserting that every data-engine context carries a principal.
1 parent 030125b commit c5ff96d

22 files changed

Lines changed: 611 additions & 89 deletions

.changeset/approval-dead-run-record-lock.md

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,8 @@ Both halves are now closed.
2020

2121
**Prevention — the owning run may write its own record.** The automation engine
2222
stamps `flowRunId` onto the run context at setup, alongside `runAs`, and it
23-
travels with every data node's ObjectQL context into `ctx.session`. The lock hook
24-
exempts a write whose `flowRunId` matches the pending request's `flow_run_id`.
23+
travels with every data node's ObjectQL context into `ctx.provenance`. The lock
24+
hook exempts a write whose `flowRunId` matches the pending request's `flow_run_id`.
2525
It is keyed on run identity rather than elevation on purpose: a `runAs:'user'`
2626
run stays fully RLS-scoped while it writes. `flowRunId` is pure provenance —
2727
server-constructed like `isSystem`, never client-supplied, evaluated by no
@@ -49,9 +49,9 @@ entries under one id, so every suspend-then-finish run — every approval, scree
4949
and wait flow — reported itself as `paused` forever, both on the Runs
5050
observability surface and to this sweep.
5151

52-
Residual, deliberately not changed here: a `runAs:'user'` run with no trigger
53-
user (a schedule) passes no ObjectQL context at all, so it carries no
54-
`flowRunId` and is still subject to the lock. Manufacturing a context just to
55-
carry the run id would flip that run from its documented unscoped fail-open
56-
(#1888) to baseline-member RLS — a separate, larger change. The sweep is what
57-
recovers that shape.
52+
One shape was left out here and closed separately in #3712: a `runAs:'user'` run
53+
with no trigger user (a schedule) resolved no ObjectQL context at all, so it
54+
carried no `flowRunId` and stayed subject to the lock. It now passes a
55+
provenance-only context — the run id and nothing the security middleware keys on
56+
— so it is attributable without acquiring a principal, and its documented
57+
unscoped posture (#1888) is unchanged.
Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
---
2+
"@objectstack/service-automation": patch
3+
"@objectstack/plugin-approvals": patch
4+
"@objectstack/objectql": patch
5+
"@objectstack/spec": patch
6+
---
7+
8+
fix(approvals): a schedule-triggered run can write its own locked record (#3712)
9+
10+
#3456 let the run that opened a pending approval write its own target record,
11+
keyed on `flowRunId`. It worked for every run that resolves an identity and
12+
missed the one that doesn't: an effective `runAs:'user'` run with **no trigger
13+
user** — a schedule being the canonical case — passed no ObjectQL context at
14+
all, so nothing carried the run id and the run still died on its own
15+
`RECORD_LOCKED`.
16+
17+
The blocker was never the lock. It was that "no identity" and "no context" were
18+
the same thing on the wire, so a run could not say *who it was* without also
19+
claiming *what it was allowed to do*.
20+
21+
**A run with no principal now passes provenance alone.**
22+
`resolveRunDataContext` returns `{ flowRunId }` — no `userId`, no `positions`,
23+
no `permissions`, not even `isSystem: false`. Every principal gate keys on one
24+
of those fields (the elevation short-circuit on `isSystem`, the ADR-0103
25+
engine-owned write guard and the ADR-0090 D12 delegated-admin gate on `userId`,
26+
the empty-principal fall-open on all three), so this context authorizes
27+
**identically to no context at all**. The run keeps the documented #1888
28+
unscoped posture, its loud `[runAs]` warning, and the
29+
`flow-schedule-runas-unscoped` build-time lint. Nothing about what it may touch
30+
changed — only that it can now be attributed.
31+
32+
**Provenance moved out of the hook session, into `ctx.provenance`.** `session`
33+
answers *who is calling* and is absent when no identity envelope was supplied —
34+
a distinction real gates depend on (the attachment access gate skips bare-kernel
35+
writes on exactly that test). Folding a run id into `session` would have forced
36+
an identity-less run to present an empty session, silently turning "no caller"
37+
into "an anonymous caller" and narrowing the #1888 fail-open for attachments
38+
alone. `HookContext.provenance.flowRunId` says what produced the write; the
39+
approvals lock reads it there.
40+
41+
Also relaxes `BaseEngineOptionsSchema.context` to a partial envelope
42+
(`ExecutionContextInput`). `positions`/`permissions`/`isSystem` carry parse-time
43+
defaults, which made them *required* on a caller-supplied option and asserted
44+
something untrue — that every data-engine context carries a principal. Callers
45+
have always passed slices (`{ isSystem: true }` for a system read); the type now
46+
says so.
47+
48+
Migration: nothing to change unless you read the run id inside a hook. If you
49+
wrote `ctx.session.flowRunId`, read `ctx.provenance.flowRunId` instead — the
50+
field never shipped under the old name.

content/docs/kernel/contracts/data-engine.mdx

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -94,10 +94,15 @@ interface EngineQueryOptions {
9494
search?: FullTextSearch; // Full-text search
9595
expand?: Record<string, QueryAST>; // Recursive relation loading
9696
distinct?: boolean; // SELECT DISTINCT
97-
context?: ExecutionContext; // Identity, tenant, transaction
97+
context?: ExecutionContextInput; // Identity, tenant, transaction — any subset
9898
}
9999
```
100100

101+
`ExecutionContextInput` is `ExecutionContext` with every field optional — supply
102+
what you have, the engine reads what it needs. A system read passes
103+
`{ isSystem: true }`; an automation run with no resolvable identity passes only
104+
its run id (`{ flowRunId }`), a context that deliberately carries no principal.
105+
101106
### FilterCondition (where)
102107

103108
Filters use the canonical **`where` + MongoDB-style `$op` object syntax** from `FilterConditionSchema`:
@@ -191,7 +196,7 @@ const tasks = await engine.insert('task', [
191196
```typescript
192197
interface DataEngineInsertOptions {
193198
returning?: boolean; // Return inserted record(s)? Default: true
194-
context?: ExecutionContext; // Identity, tenant, transaction
199+
context?: ExecutionContextInput; // Identity, tenant, transaction — any subset
195200
}
196201
```
197202

@@ -218,7 +223,7 @@ interface EngineUpdateOptions {
218223
upsert?: boolean; // Insert if not found? Default: false
219224
multi?: boolean; // Update multiple records? Default: false
220225
returning?: boolean; // Return updated record(s)? Default: false
221-
context?: ExecutionContext;
226+
context?: ExecutionContextInput;
222227
}
223228
```
224229

@@ -268,7 +273,7 @@ await engine.delete('task', {
268273
interface EngineDeleteOptions {
269274
where?: FilterCondition; // Filter to identify records (WHERE)
270275
multi?: boolean; // Delete multiple records? Default: false
271-
context?: ExecutionContext;
276+
context?: ExecutionContextInput;
272277
}
273278
```
274279

@@ -306,7 +311,7 @@ interface EngineAggregateOptions {
306311
where?: FilterCondition; // Pre-aggregation filter (WHERE)
307312
groupBy?: string[]; // GROUP BY fields
308313
aggregations?: AggregationNode[]; // Aggregation definitions
309-
context?: ExecutionContext;
314+
context?: ExecutionContextInput;
310315
}
311316

312317
interface AggregationNode {

content/docs/references/data/hook.mdx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ const result = HookContext.parse(data);
3838
| **result** | `any` | optional | Operation result (After hooks only) |
3939
| **previous** | `Record<string, any>` | optional | Record state before operation |
4040
| **session** | `{ userId?: string; organizationId?: string; roles?: string[]; accessToken?: string; … }` | optional | Current session context |
41+
| **provenance** | `{ flowRunId?: string }` | optional | Server-stamped write provenance (never client-supplied, never an authorization input) |
4142
| **transaction** | `any` | optional | Database transaction handle |
4243
| **ql** | `any` || ObjectQL Engine Reference |
4344
| **api** | `any` | optional | Cross-object data access (ScopedContext) |

packages/objectql/src/engine.test.ts

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -527,6 +527,77 @@ describe('ObjectQL Engine', () => {
527527
});
528528
});
529529

530+
/**
531+
* #3712 — write PROVENANCE (what produced the write) is surfaced separately
532+
* from the SESSION (who is calling), because a writer can have the first and
533+
* none of the second: a schedule-triggered flow run resolves no principal but
534+
* still owns its writes. Folding the two together would have forced such a run
535+
* to present an empty session, silently converting "no caller" into "an
536+
* anonymous caller" for every hook that skips when `ctx.session` is absent.
537+
*/
538+
describe('hook provenance is separate from the hook session (#3712)', () => {
539+
beforeEach(async () => {
540+
engine.registerDriver(mockDriver, true);
541+
await engine.init();
542+
vi.mocked(SchemaRegistry.getObject).mockReturnValue({ name: 'task', fields: {} } as any);
543+
});
544+
545+
const capture = () => {
546+
const seen: { session?: any; provenance?: any } = {};
547+
engine.registerHook('beforeInsert', async (ctx: any) => {
548+
seen.session = ctx.session;
549+
seen.provenance = ctx.provenance;
550+
}, { object: 'task' });
551+
return seen;
552+
};
553+
554+
it('a provenance-only context yields provenance and NO session', async () => {
555+
const seen = capture();
556+
557+
await engine.insert('task', { title: 'x' }, { context: { flowRunId: 'run_1' } as any });
558+
559+
expect(seen.provenance).toEqual({ flowRunId: 'run_1' });
560+
// The whole point: `!ctx.session` still means "no identity envelope
561+
// was supplied", so caller-gating hooks keep skipping for this write.
562+
expect(seen.session).toBeUndefined();
563+
});
564+
565+
it('an identity-bearing run carries both', async () => {
566+
const seen = capture();
567+
568+
await engine.insert('task', { title: 'y' }, { context: { userId: 'u1', flowRunId: 'run_2' } as any });
569+
570+
expect(seen.provenance).toEqual({ flowRunId: 'run_2' });
571+
expect(seen.session).toMatchObject({ userId: 'u1' });
572+
// Provenance is NOT identity — it must not reappear inside the session.
573+
expect(seen.session).not.toHaveProperty('flowRunId');
574+
});
575+
576+
it('no context at all yields neither', async () => {
577+
const seen = capture();
578+
579+
await engine.insert('task', { title: 'z' });
580+
581+
expect(seen.provenance).toBeUndefined();
582+
expect(seen.session).toBeUndefined();
583+
});
584+
585+
it('an anonymous transport request still yields a session (it has a caller)', async () => {
586+
// The shape resolveExecutionContext builds for a signed-out HTTP
587+
// request: no userId, but a resolved (empty) principal. That is an
588+
// anonymous CALLER, not the absence of one — hooks must keep gating it.
589+
const seen = capture();
590+
591+
await engine.insert('task', { title: 'w' }, {
592+
context: { isSystem: false, positions: [], permissions: [] } as any,
593+
});
594+
595+
expect(seen.session).toBeDefined();
596+
expect(seen.session.userId).toBeUndefined();
597+
expect(seen.provenance).toBeUndefined();
598+
});
599+
});
600+
530601
describe('execution context via the trailing options arg (read methods)', () => {
531602
// Regression: reads took context inside the query while writes took it in
532603
// a trailing options arg — so `find(obj, q, { context })` silently dropped

0 commit comments

Comments
 (0)