Skip to content

Commit 6a2866a

Browse files
feat(plugin-approvals,rest,client): a record-anchored approval lock read, { locked } for every caller who can read the record (#26361)
<!-- claude-projects-attribution: {"github_login":"os-project-manager"} --> _Requested by **Leehom** · [project thread](https://claude.ai/code/project/chan_01TenzsRopZj4jMbvjX8uz1X?thread=cmsg_01TenzsRopZj4jMbvjX8uz1X38EQhJnKnxazCiq8xQpv9c)_ Fixes #26314 Clause-②: yes (widening) Metadata-protocol: none ## What changes #25934 ruling A ("readers"): the record lock gets a carrier of its own, so a member who can read a record but is not on its approval is no longer offered an edit the server then refuses with `409 RECORD_LOCKED`. - **`@objectstack/spec`**: one optional member on `IApprovalService`, `getRecordLock?(object, recordId, context): Promise<{ locked: boolean } | null>`, with its doc block. No schema, no exported type. - **`@objectstack/plugin-approvals`**: `ApprovalService.getRecordLock` reads the record **as the caller** (`engine.find` with the caller's context, `fields: ['id']`, `limit: 1`). An unreadable record answers `null`, the same as a missing id; a caller refused the object's read is refused for every id. `locked` comes from the pending-request lookup the hook already uses. - **One lock judgement**: `nodeConfigLocksRecord(config)` (`lockRecord !== false`) in `lifecycle-hooks.ts` is now the only spelling. The `beforeUpdate` record-lock hook, the request row's `lock_record` mirror and the new read all call it; the read goes through `recordHeldByLockingApproval`, which shares the hook's lookup and its bookkeeping-object exemption. - **`@objectstack/rest`**: `GET /api/v1/approvals/records/:object/:recordId/lock` answers `{ success: true, data: { locked } }` and nothing else. Every `null` answers one `404 RECORD_NOT_FOUND` body, so an unreadable record and a missing one cannot be told apart. The closed query-parameter set is empty (route rule 5). A service without the member answers `501`. Ledgered as `sdk`. - **`@objectstack/client`**: `client.approvals.getRecordLock(object, recordId)`. - **Docs**: `content/docs/automation/approvals.mdx` gains a "Reading the lock" paragraph with a curl example. #8652's ruling is untouched: request content stays behind `approvalsVisibleToReaders`, and the tests assert the non-participant still gets no request row. ## Spec change: needs the spec seat's sign-off The only `packages/spec/**` change is `packages/spec/src/contracts/approval-service.ts`, new lines 935–972: the doc block plus the optional member `getRecordLock?` inserted right after `getRequest(...)` in `IApprovalService`. Nothing else under `packages/spec/` changed. It is optional so existing implementations and the spec's own `minimalService` literal still satisfy the contract. ## Tests - `packages/qa/dogfood/test/approval-record-lock-read.dogfood.test.ts` (new): the card's four cases through the REST door on a real boot with the real `SecurityPlugin`, flows and approvals plugin, on two `private` objects: 1. a non-participant who can read and edit the held record gets exactly `{ success: true, data: { locked: true } }`, still gets `[]` from the request list, and their `PATCH` is still `409 RECORD_LOCKED`; 2. a read-only reader gets `{ locked: true }`; 3. a caller who cannot read the record (their `GET /data` is 404) gets the same status and body as for a non-existent id (`404 RECORD_NOT_FOUND`); 4. a pending node with `lockRecord: false`, and the held record after approval, both read `{ locked: false }` and the edit lands. - `packages/plugins/plugin-approvals/src/record-lock-read.integration.test.ts` (new, 4 tests): real ObjectQL over SQLite with the real plugin start; also pins `lock_record` and the read agreeing on every request. - `packages/rest/src/rest-approvals-record-lock-route.test.ts` (new, 5 tests): preservation (extra keys from a service are dropped), one 404 body for every `null`, the empty closed query set, `501`, and a `403` relay. - `packages/client/src/client.test.ts`: URL encoding and envelope unwrapping. - `packages/rest/src/execctx-consumer-census.test.ts`: counts move by one for the new route. Dogfood does not run on PR CI, so it was run locally under the verify lock. At head `6ba5f8cc`: plugin-approvals 99 files / 1280 tests pass, client 52 / 672 pass, the new dogfood file 1 / 1 pass. Rest (324 files / 6483 pass, 326 skipped) and spec (725 files / 21974 pass) ran at `99f67d74`; the later merges of `main` changed no file in either package. ### Gates - `dispatch-gates.mjs --commands` derived 114 commands; all 114 ran at `6ba5f8cc` with exit 0, reconciled with `--ran` (114 run, 0 not measured, 0 unrun). `main` moved 13 commits during the run; none touches a file in this diff. - `typecheck` on spec, plugin-approvals, rest and client: exit 0. Spec `check:generated`: exit 0. - `test:repo` on core, objectql, rest, runtime, spec and types: exit 0. - Scoped `eslint` over the touched files and `check-nul-bytes`: exit 0. ### Ablations (through the REST door, dogfood) 1. Lock judgement mutated to always lock: red at `approval-record-lock-read.dogfood.test.ts:217`, expected `locked: false` for the `lockRecord: false` record, received `true`. Restored, blob equals HEAD. 2. As-caller read mutated to a system context: red at `:209`, expected `404` for the caller who cannot read the record, received `200`. Restored, blob equals HEAD. Each mutation was rebuilt into `dist` and confirmed on disk by `scripts/ablation-dist-preflight.mjs` before the run, and `--absent` after the restore. Ablation 2's declaration build failed (TS6133, `context` unused once the mutation dropped its use); the JS build still carried the mutation, which the preflight found in both `index.js` and `index.mjs`, so the run is valid. ## Deviations from the claim - The claim named plugin-approvals; the route, ledger row and client method live in `packages/rest` (`approvals-routes.ts`, `rest-route-ledger.ts`) and `packages/client/src/index.ts`. Agreed with the dispatching seat before the change so the ledger row ships as `sdk`, not as a gap. - Skill teaching is not in this PR: `skills/**` is a governed surface and would make the whole PR need maintainer approval. A follow-up card is named in the report. ## Acceptance notes - `pendingRequestFor` reads with `limit: 1`, so with two pending requests on one record the hook judges only the first one it gets. The new read reuses the same lookup, so the two still agree; whether two pending requests on one record can happen is not verified here. - Overlap: open #26337 also edits `approval-service.ts` (different regions) and `content/docs/automation/approvals.mdx` (a nearby section); whichever lands second may need a trivial rebase. --- _Generated by [Claude Code](https://claude.ai/code/session_01QAsn9ToxaDxbnRkP6HxERw)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent e843b5b commit 6a2866a

13 files changed

Lines changed: 884 additions & 23 deletions
Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
---
2+
'@objectstack/spec': patch
3+
'@objectstack/plugin-approvals': patch
4+
'@objectstack/rest': patch
5+
'@objectstack/client': patch
6+
---
7+
8+
feat(plugin-approvals,rest,client): a record-anchored approval lock read, `{ locked }` for every caller who can read the record
9+
10+
Clause-②: yes (widening)
11+
Metadata-protocol: none
12+
13+
A member who could read and edit a record but was not on its approval was offered an edit the server then refused with `409 RECORD_LOCKED`. The lock's only carrier was the request row's `lock_record`, and that row is rightly not served to a non-participant unless the object turns on `approvalsVisibleToReaders`. The lock now has a carrier of its own.
14+
15+
- **`@objectstack/spec`.** `IApprovalService` gains the optional member `getRecordLock(object, recordId, context)`, which answers `{ locked: boolean }`, or `null` when the caller cannot read the record. No exported schema or type is added.
16+
- **`@objectstack/plugin-approvals`.** `ApprovalService.getRecordLock` reads the record as the caller. A record the caller cannot read answers `null`, the same answer as an id that does not exist. A caller with no read on the object is refused for every id, whether it exists or not. `locked` is `true` while a pending request's node has `lockRecord` other than `false`. The lock judgement is now one function, `nodeConfigLocksRecord` (not exported from the package), and the record-lock hook, the request row's `lock_record` and the new read all call it. Request content stays behind `approvalsVisibleToReaders`: the answer carries no request id, node, approvers or `pending` flag.
17+
- **`@objectstack/rest`.** New route `GET /api/v1/approvals/records/:object/:recordId/lock`. It answers `{ success: true, data: { locked } }`. A `null` answers `404 RECORD_NOT_FOUND` with one body, whether the record is unreadable or missing. A service without the member answers `501`. The route's closed query-parameter set is empty, so any query parameter is refused with a `400`. The route is ledgered as `sdk`.
18+
- **`@objectstack/client`.** `client.approvals.getRecordLock(object, recordId)` calls that route.

‎content/docs/automation/approvals.mdx‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -376,6 +376,27 @@ never deadlock against itself. The exemption is keyed on run identity rather tha
376376
elevation, so a `runAs:'user'` run stays row-level-security scoped while it
377377
writes — it does not become a system write.
378378

379+
**Reading the lock.** Anyone who can read the record can ask whether it is
380+
locked, including a member who is not on the approval and so is served no
381+
request row:
382+
383+
```bash
384+
curl -b cookies.txt \
385+
"https://your-app.example.com/api/v1/approvals/records/expense/rec_123/lock"
386+
# → { "success": true, "data": { "locked": true } }
387+
```
388+
389+
The answer is `locked` and nothing else: no request, approvers or history,
390+
which stay behind the object's `approvalsVisibleToReaders` switch. `locked` is
391+
`true` while a pending request's node has `lockRecord` other than `false`. That
392+
is the same check the server applies when it refuses an edit with `409
393+
RECORD_LOCKED`, so a page that reads it never offers an edit the server will
394+
refuse. The record is read as the caller: a record they cannot read answers
395+
`404 RECORD_NOT_FOUND`, the same body a record id that does not exist gets. The
396+
SDK call is `client.approvals.getRecordLock(object, recordId)`, and the service
397+
member is `IApprovalService.getRecordLock`. An approvals service without that
398+
member answers `501`.
399+
379400
Only `approvers` is required on the node; everything else has a default
380401
(`behavior: 'first_response'`, `lockRecord: true`, `maxRevisions: 3`).
381402

‎packages/client/src/client.test.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -704,6 +704,15 @@ describe('Approvals namespace (ADR-0019)', () => {
704704
expect(url).toContain('/api/v1/approvals/requests/req-1');
705705
});
706706

707+
it('[#26314] reads a record\'s approval lock off the envelope, path segments encoded', async () => {
708+
const { client, fetchMock } = createMockClient({ success: true, data: { locked: true } });
709+
const result = await client.approvals.getRecordLock('order', 'rec/1');
710+
expect(result).toEqual({ locked: true });
711+
const [url, opts] = fetchMock.mock.calls[0];
712+
expect(String(url)).toBe('http://localhost:3000/api/v1/approvals/records/order/rec%2F1/lock');
713+
expect(opts?.method ?? 'GET').toBe('GET');
714+
});
715+
707716
it('should record an approve decision', async () => {
708717
const { client, fetchMock } = createMockClient({
709718
request: { id: 'req-1', status: 'approved' }, finalized: true, decision: 'approve', resumed: true

‎packages/client/src/index.ts‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6260,6 +6260,21 @@ export class ObjectStackClient {
62606260
return this.unwrapResponse<ApprovalRequestRow>(res);
62616261
},
62626262

6263+
/**
6264+
* Whether a record is held by a locking approval — `{ locked }` and
6265+
* nothing else, answered to every caller who can read the record, whether
6266+
* or not they take part in the approval (`IApprovalService.getRecordLock`).
6267+
* A record the caller cannot read answers 404, exactly as a record id that
6268+
* does not exist; a deployment without the read answers 501.
6269+
*/
6270+
getRecordLock: async (object: string, recordId: string): Promise<{ locked: boolean }> => {
6271+
const route = this.getRoute('approvals');
6272+
const res = await this.fetch(
6273+
`${this.baseUrl}${route}/records/${encodeURIComponent(object)}/${encodeURIComponent(recordId)}/lock`,
6274+
);
6275+
return this.unwrapResponse<{ locked: boolean }>(res);
6276+
},
6277+
62636278
/**
62646279
* Record an approve decision on a request. Finalises the request when the
62656280
* node's behaviour is satisfied and resumes the owning flow run.

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

Lines changed: 48 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,7 @@ import {
120120
type FieldVisibilitySource,
121121
} from './payload-redaction.js';
122122
import { APPROVAL_REQUEST_CHILD_OBJECTS, type RequestVisibilitySource } from './request-read-gate.js';
123+
import { nodeConfigLocksRecord, recordHeldByLockingApproval } from './lifecycle-hooks.js';
123124
// The ADR-0043 pages — the ONE copy of their text, rendered by the plugin's
124125
// self-hosted mount and by `handleActionPage` alike.
125126
import { renderConfirmPage, renderResultPage } from './action-link-pages.js';
@@ -1280,13 +1281,13 @@ function rowFromRequest(row: any): ApprovalRequestRow {
12801281
// ADR-0044 revision round (rides the config snapshot; absent ⇒ round 1).
12811282
round: typeof cfg?.__round === 'number' ? cfg.__round : undefined,
12821283
// objectui#2902: the node's record-lock policy. The lock is enforced
1283-
// server-side in `lifecycle-hooks.ts` off THIS SAME snapshot with the
1284-
// same `!== false` default, so the flag a client renders and the rule the
1285-
// server applies can never drift. Without it a console can only see
1286-
// "a pending request exists" and has to assume the record is locked —
1287-
// which mislabels every `lockRecord: false` node as locked and hides an
1288-
// edit the server would have accepted.
1289-
lock_record: cfg?.lockRecord !== false,
1284+
// server-side in `lifecycle-hooks.ts` off THIS SAME snapshot by the SAME
1285+
// judgement (`nodeConfigLocksRecord`, #26314), so the flag a client renders
1286+
// and the rule the server applies can never drift. Without it a console can
1287+
// only see "a pending request exists" and has to assume the record is
1288+
// locked — which mislabels every `lockRecord: false` node as locked and
1289+
// hides an edit the server would have accepted.
1290+
lock_record: nodeConfigLocksRecord(cfg),
12901291
// #3447 P2: the node's author-declared decision outputs, surfaced so a
12911292
// decision UI can render input fields for them and POST `outputs` on
12921293
// approve/reject. Per-request (each node declares its own), which is why
@@ -8641,6 +8642,46 @@ export class ApprovalService implements IApprovalService {
86418642
return this.loadRequest(requestId, context, true);
86428643
}
86438644

8645+
/**
8646+
* [#26314] `IApprovalService.getRecordLock` — is this record held by a
8647+
* locking approval, for EVERY caller who can read the record (#25934 ruling
8648+
* A). A non-participant is told the record is locked before they edit it,
8649+
* instead of being offered an edit the lock hook then refuses.
8650+
*
8651+
* ## Who is answered — the #8652 anchor, read as the caller
8652+
*
8653+
* `engine.find(object, { where: { id }, context })` with the CALLER's
8654+
* context, exactly as {@link addRecordReaderVisibleIds} reads it: object CRUD
8655+
* read, then RLS. A row the caller may not see comes back empty and answers
8656+
* `null` — the very answer a record id that does not exist gets, because it
8657+
* is the same branch. ⛔ No path tells the two apart. A refusal of the read
8658+
* itself (no read on the object) is thrown, and is the same refusal for any
8659+
* id, existing or not. ⚠️ Probing with {@link SYSTEM_CTX} here would admit
8660+
* every authenticated user in the tenant.
8661+
*
8662+
* ## What is answered — the lock, and nothing about the request
8663+
*
8664+
* `{ locked }` only: no request id, node, participants or `pending`. Request
8665+
* content stays behind `approvalsVisibleToReaders` (#8652 ruling 5299823744,
8666+
* untouched). The judgement is the record-lock hook's own
8667+
* ({@link recordHeldByLockingApproval}), never a second copy of it.
8668+
*/
8669+
async getRecordLock(
8670+
object: string,
8671+
recordId: string,
8672+
context: ExecutionContext,
8673+
): Promise<{ locked: boolean } | null> {
8674+
const objectName = String(object ?? '').trim();
8675+
const id = String(recordId ?? '').trim();
8676+
if (!objectName || !id) return null;
8677+
// The anchor. As the CALLER — see the doc block.
8678+
const readable = await this.engine.find(objectName, {
8679+
where: { id }, fields: ['id'], limit: 1, context,
8680+
});
8681+
if (!Array.isArray(readable) || readable.length === 0) return null;
8682+
return { locked: await recordHeldByLockingApproval(this.engine, objectName, id) };
8683+
}
8684+
86448685
private async loadRequest(
86458686
requestId: string,
86468687
context: ExecutionContext,

‎packages/plugins/plugin-approvals/src/lifecycle-hooks.ts‎

Lines changed: 55 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -315,7 +315,7 @@ function asIdList(id: unknown): Array<string | number> | null {
315315

316316
/** The pending request gating a record, plus its snapshotted node config. */
317317
async function pendingRequestFor(
318-
engine: MinimalEngine,
318+
engine: Pick<MinimalEngine, 'find'>,
319319
objectName: string,
320320
recordId: string,
321321
): Promise<any | null> {
@@ -333,7 +333,7 @@ async function pendingRequestFor(
333333

334334
/** Pending requests gating any of the records a write names by id. */
335335
async function pendingRequestsForRecords(
336-
engine: MinimalEngine,
336+
engine: Pick<MinimalEngine, 'find'>,
337337
objectName: string,
338338
recordIds: ReadonlyArray<string | number>,
339339
): Promise<any[]> {
@@ -388,6 +388,57 @@ async function pendingRequestsForObject(
388388
}
389389
}
390390

391+
/**
392+
* THE lock judgement: does a pending request whose node snapshotted `config`
393+
* lock its target record against edits?
394+
*
395+
* `lockRecord` defaults to `true` (`ApprovalNodeConfigSchema`), so a node locks
396+
* unless it explicitly opted out. This is the one place that reading lives
397+
* (#26314): the record-lock hook below enforces it, the request row mirrors it
398+
* as `lock_record` (`rowFromRequest` in `approval-service.ts`), and the
399+
* record-anchored read ({@link recordHeldByLockingApproval}) answers it — so
400+
* what a client is told and what the server refuses can never drift. ⛔ Never
401+
* re-spell `lockRecord !== false` at a call site.
402+
*/
403+
export function nodeConfigLocksRecord(config: unknown): boolean {
404+
return (config as { lockRecord?: unknown } | null | undefined)?.lockRecord !== false;
405+
}
406+
407+
/**
408+
* The approvals plugin's own bookkeeping objects are never locked: the hook
409+
* skips them with this cheap prefix test (it is a `beforeUpdate` guard, and
410+
* the delete linkage below excludes them at registration instead).
411+
*/
412+
function isApprovalBookkeepingObject(objectName: string): boolean {
413+
return String(objectName).startsWith('sys_approval');
414+
}
415+
416+
/**
417+
* [#26314] Is `recordId` of `objectName` held by a LOCKING pending approval —
418+
* would an ordinary edit of it be refused `RECORD_LOCKED`?
419+
*
420+
* Asked exactly as the record-lock hook asks it of a by-id write: the same
421+
* pending-request lookup ({@link pendingRequestsForRecords}, read as SYSTEM, so
422+
* the bookkeeping is never narrowed by a caller's visibility) and the same
423+
* judgement ({@link nodeConfigLocksRecord}). What it leaves out is what belongs
424+
* to a WRITE, not to the record: the opening run's own writes (#3456) and a
425+
* write touching only the status mirror are exempt there because of who writes
426+
* and what, and neither is a property of the record.
427+
*
428+
* ⚠️ It answers for the record, not for a caller, and reads nothing as one.
429+
* Whether the asker may learn the answer — may read the record at all — is the
430+
* caller's question to settle first (`ApprovalService.getRecordLock`).
431+
*/
432+
export async function recordHeldByLockingApproval(
433+
engine: Pick<MinimalEngine, 'find'>,
434+
objectName: string,
435+
recordId: string,
436+
): Promise<boolean> {
437+
if (isApprovalBookkeepingObject(objectName)) return false;
438+
const gating = await pendingRequestsForRecords(engine, objectName, [recordId]);
439+
return gating.some((pending) => nodeConfigLocksRecord(parseJson<unknown>(pending?.node_config_json, {})));
440+
}
441+
391442
/**
392443
* Narrow `candidates` (pending requests) to the ones whose record the caller's
393444
* predicate actually matches — asked of the engine as an intersection, so the
@@ -487,7 +538,7 @@ export function bindApprovalLockHook(
487538
engine.registerHook('beforeUpdate', async (ctx: any) => {
488539
const object = (ctx?.object ?? ctx?.objectName) as string | undefined;
489540
// No object name (shouldn't happen) or our own bookkeeping objects → skip.
490-
if (!object || String(object).startsWith('sys_approval')) return;
541+
if (!object || isApprovalBookkeepingObject(object)) return;
491542

492543
const data = (ctx?.input?.data ?? {}) as Record<string, unknown>;
493544
const changedFields = Object.keys(data).filter((k) => k !== 'id' && k !== 'updated_at');
@@ -538,7 +589,7 @@ export function bindApprovalLockHook(
538589
if (writerRun && pending?.flow_run_id && String(writerRun) === String(pending.flow_run_id)) continue;
539590

540591
const config = parseJson<any>(pending?.node_config_json, {});
541-
if (config?.lockRecord === false) continue;
592+
if (!nodeConfigLocksRecord(config)) continue;
542593

543594
// Allow when every changed field is the approval status mirror.
544595
const mirror = config?.approvalStatusField;

0 commit comments

Comments
 (0)