Skip to content

Commit f9e16d8

Browse files
huangyiireneclaude
andauthored
fix(security,metadata-protocol): a packaged permission set's DELETE stops reporting a deletion it did not perform (#19411)
Fixes #19306 Clause-②: no ## What was wrong `DELETE /api/v1/data/sys_permission_set/{id}` on a **package-declared** set answered `200 {"object":…,"id":…,"success":true}` and changed nothing. The identical call on an environment-authored set really deletes. Both shipped a **byte-identical envelope**, so no caller could act differently on the two cases: a UI fired a success toast and showed the row again on refresh, on a *security-configuration* write. The mechanical source, in `packages/plugins/plugin-security/src/permission-set-projection.ts`: the write-through's delete leg opened with `let lastOutcome: any = true` and the loop body could only ever assign `true` again, so no path could report anything else. The difference was already named one line up — `logger.info('[security] permission set reset to its declared baseline …')` — but it went to the log, never to the caller. ## What this does NOT change The record surviving is the **design**, not the defect: an artifact-backed definition tombstones its overlay (an ADR-0005 RESET) and the record re-projects to the declared body instead of vanishing. The restore leg's own argument depends on that guarantee. This PR makes the **answer** truthful; it does not make a packaged set deletable. ## The fix 1. `permission-set-projection.ts` — the delete leg reports how many of the addressed **records** actually went, read off the record itself rather than off the re-projection's counter. (That counter is not the fact: on a kernel wired to the ADR-0094 *awaited* projector — the production shape — `deleteMetaItem` has already retired the record, so the idempotent re-projection legitimately reports `deleted: 0` for a delete that really happened. Reading it would have answered `success: false` for **every** delete. The first draft did exactly that and the new pin's control leg caught it.) 2. `metadata-protocol/src/protocol.ts` — `deleteData` maps that onto the already declared `success` key instead of hard-coding `true`. ⛔ **No spec change.** `DeleteDataResponseSchema` keeps its three keys; `success` is declared "Whether deletion succeeded" and a record that is still there was not deleted. The `/meta` door's `reset` discriminator was deliberately **not** moved onto this door — that is a `packages/spec` widening and belongs to the spec lane, not to this PR. ⛔ **Not `false` from the middleware either.** The boolean arm's `false` is the driver contract's "no row matched", which `deleteData` turns into `404 RECORD_NOT_FOUND` — about a record the same caller can still GET. Zero rows removed is the engine delete result's other declared arm and the only value that positively says "it is still there". ## On the wire | case | before | after | |:---|:---|:---| | packaged set | `200 {"success":true}`, row unchanged | `200 {"success":false}`, row unchanged | | environment-authored set | `200 {"success":true}`, row gone | `200 {"success":true}`, row gone | | unknown id | `404 RECORD_NOT_FOUND` | `404 RECORD_NOT_FOUND` | | ordinary object | `200 {"success":true}`, row gone | `200 {"success":true}`, row gone | ## Evidence **Live REST, `examples/app-showcase` booted `--fresh`, seeded platform admin over a cookie session — an ordinary non-system caller.** The card's reading was taken against `f93beea0a6`; this branch's base is `fade3da23`, so both legs were re-taken on today's tree. BEFORE (base `fade3da23`, port 38913) — the defect reproduced: ``` DELETE /api/v1/data/sys_permission_set/ps_mua1tdbwu14gimg2 (showcase_auditor, managed_by=package) -> 200 {"object":"sys_permission_set","id":"ps_mua1tdbwu14gimg2","success":true} GET same id -> 200, same id, updated_at 2026-09-20T16:46:31.868Z (unchanged), row count 17 -> 17 ``` BEFORE, negative control — an environment-authored set really deletes: ``` POST /api/v1/data/sys_permission_set {"name":"probe_env_set_19306"} -> 201, rows 17 -> 18 DELETE /api/v1/data/sys_permission_set/ps_mua1uc5xypituh2j -> 200 {"success":true} rows 18 -> 17 ; GET the id -> 404 RECORD_NOT_FOUND ``` AFTER (this branch, port 38914) — same two legs: ``` DELETE /api/v1/data/sys_permission_set/ps_mua26j2717zrru57 (showcase_member_default, managed_by=package) -> 200 {"object":"sys_permission_set","id":"ps_mua26j2717zrru57","success":false} GET same id -> 200, same id, updated_at 2026-09-20T16:56:45.823Z (unchanged), row count 17 -> 17 ``` ``` POST /api/v1/data/sys_permission_set {"name":"probe_env_set_19306_after"} -> 201, rows 17 -> 18 DELETE /api/v1/data/sys_permission_set/ps_mua276okvsajceez -> 200 {"success":true} rows 18 -> 17 ; GET the id -> 404 RECORD_NOT_FOUND DELETE /api/v1/data/sys_permission_set/ps_definitely_not_a_row -> 404 RECORD_NOT_FOUND DELETE /api/v1/data/showcase_task/dckUaD8NSPulx3wv -> 200 {"success":true}, GET -> 404 ``` ⇒ the packaged leg is the only answer that moved; record survival, ids, `updated_at` and row counts are identical before and after, and no delete became a no-op. **Unit.** `pnpm --filter @objectstack/plugin-security --filter @objectstack/metadata-protocol test` — plugin-security 115 files / 2234 tests passed, metadata-protocol 182 files (3 skipped) / 2601 tests passed (19 skipped). `typecheck` on both: exit 0. **Reverse verification** (both on committed state, mutation proven on disk by `scripts/ablation-replace.mjs`'s anchor and blob counts, restored byte-identical — the restored blob equals the HEAD blob in both cases): | mutation | pin | result | |:---|:---|:---| | `success: deleted !== 0` back to `success: true` (anchor x1 to x0, blob `01c0b69e23ad` to `2387a4ca325b`) | `protocol.record-not-found.test.ts` | 1 failed / 13 passed | | the delete-leg outcome back to `opCtx.result = true` (anchor x1 to x0, blob `d55d579b1a48` to `51005a746830`) | `permission-set-projection.test.ts` | 1 failed / 66 passed | Both restored: `git hash-object` equals `git rev-parse HEAD:path`, `git diff HEAD` empty. **Gates.** `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` derived 65 families from this diff; all 65 ran, all exit 0, reconciled with `--ran` (65 derived, 65 run, 0 NOT-MEASURED, 0 UNRUN). Repo-wide `pnpm lint` ran unnarrowed: exit 0. All readings at `9f1c8b100`. ## Acceptance notes Noticed while measuring, deliberately **not** fixed here: - `os data delete` in its default (non-JSON) format prints `Record deleted: {id}` unconditionally, so the human-readable CLI surface still reports a deletion for a packaged permission set even though its own `--format json` arm now forwards `deleted: false` correctly. Same defect family, different door — reported for filing, not carried here. - `deleteManyData`'s per-row `success` was not audited on this card; the batch door is out of this card's scope. - #19307 (the duplicate-name 409 in this same file carrying no `code`) is not addressed here and remains open — it is hard-serialised after this card and none of its lines were touched. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- _Generated by [Claude Code](https://claude.ai/code/session_01AhQASwqJr2Z7XfGWUdvnbF)_ --- ## 第二轮 —— 隔离契约复核打回一条阻塞项,已修(本节由认领席 `domain:services` 补写) ⚠️ 正文首轮由 dev 一次写成(dev ⛔ 不 PATCH 正文),因此以上描述的是**第一轮**的探针 —— 而那个探针正是复核判为阻塞的东西。本节是第二轮的记录,以 head `9de7b7b97` 为准。 ### 被打回的缺陷:存活探针 fail-open 第一轮的探针走 `tryFind`,其尾为 `catch { return []; }` ⇒ 读**抛异常**时返回空数组,与「行不在」无法区分 ⇒ 该行被计为已删除 ⇒ 门口答 `200 {"success":true}`,**而记录还在原地**。 即:本 PR 要消灭的那个谎被原样保留,只是触发条件从「打包声明集」换成了「这一次读失败」。 ### 修法 新增模块内 `probeRecordSurvival`(`permission-set-projection.ts:144`),返回三态 `'gone' | 'present' | 'unknown'`,`catch { return 'unknown'; }`。调用点 `:1385`: ```ts const survival = await probeRecordSurvival(ql, 'sys_permission_set', row.id); if (survival === 'unknown') { logger?.warn?.( /* 耐久通道:说明调用方被告知了什么、怎么落定 */ ); } if (survival === 'gone') removed += 1; ``` ⇒ **只有真正答复了的读才能产生 `gone`**;`unknown` 计为幸存(fail-closed),并在耐久通道报告一次。方向与先例写进了代码:同插件 `per-organization-catalog.ts` 的 `listSeedOrganizationIds` 早已写明同一条规则(零个组织是「没什么要种」,读不出来是「我们不知道」)。 探针的 context 与读法与 `tryFind` 逐字节同形(`seedCtx()`,不带 `transaction` 键),故复核已核准的事务语义不变 —— ambient 事务句柄照常承载。 ### 本轮新增的钉子 - **单元 pin**:`a read-back that cannot ANSWER reports the record as NOT deleted (fail-closed)`。它只让**读回那一次**失败(目标解析与探针都按 id 读,靠顺序区分),并**先断言 `byIdReads >= 2`** —— 注入若没打中,测试变**红**而不是空绿。 - **端到端 pin ×2**:`showcase-permission-projection.dogfood.test.ts` 与 `two-doors-permission.dogfood.test.ts` 此前只断言 `status < 300`,信封回归它们照样全绿;现各加一行 `expect((await res.json())?.success, 'a reset does not report a deletion').toBe(false)`。 ### 门禁面的变化 派生族集由 **65 → 69**(`packages/qa/dogfood` 进入改动集,新增 spec 的 `check:empty-state`、`check:liveness`、`check:strictness-ledger`、`check:variant-docs`),69 族全跑、全 exit 0;repo-wide `pnpm lint` 未收窄,exit 0。 ### ⛔ 未改动、经复核核准后逐条现验 - `packages/spec/**`:**零改动**(`git diff --name-only origin/main...HEAD -- packages/spec` 为空)。`Clause-②: no` 维持不变 —— 依 #16349 的方向裁决,「拉回已声明契约」不触条款②。 - `protocol.ts` 的另外两张未修面(`origin/main` 的 `:12518` batch 臂、`:13070` `deleteManyData`)**一字未动**,已另立 **#19412**(`domain:engine` 面)。 - `success: deleted !== 0` 与其上的 `deleted === false` → 404 分支与第一轮核准版本逐字节相同。 第一轮的实时 REST 前后读数仍然成立且未重取:本轮只改**读失败**这条路径,线上两条腿的行为未变,该路径由单元 pin 覆盖。 <sub>本节由 `domain:services` 席(`session_01AhQASwqJr2Z7XfGWUdvnbF`)补写,依 `.claude/agents/os-dev.md`「PR 正文 dev 只写一次…事后要改的报告点名改法,席位代写」。复核记录见卡上 `5751509468`。</sub> --- _Generated by [Claude Code](https://claude.ai/code)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 7ffddfa commit f9e16d8

7 files changed

Lines changed: 315 additions & 4 deletions

File tree

‎.changeset/hungry-pugs-shave.md‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
---
2+
'@objectstack/plugin-security': patch
3+
'@objectstack/metadata-protocol': patch
4+
---
5+
6+
`DELETE /api/v1/data/sys_permission_set/{id}` stops reporting a deletion it did not perform.
7+
8+
A package-declared permission set cannot be deleted from an environment: its delete is an
9+
ADR-0005 RESET — the overlay tombstones and the record re-projects to the declared body.
10+
That behaviour is unchanged and deliberate. What was wrong is the answer: the door replied
11+
`200 {"object":…,"id":…,"success":true}`, byte-identical to a real deletion, so a caller
12+
that meant to revoke a permission set was told it was gone while it was still enforced, and
13+
a UI fired a success toast and showed the row again on refresh.
14+
15+
The write-through's delete leg now reports how many of the addressed records actually went,
16+
and `deleteData` maps that onto the already declared `success` key instead of hard-coding
17+
`true`. No key is added to `DeleteDataResponseSchema`.
18+
19+
On the wire:
20+
21+
- packaged set — `200 {"success":false}`, the record still present with the same id (was
22+
`success: true`);
23+
- environment-authored set — `200 {"success":true}`, the record really gone (unchanged);
24+
- unknown id — `404 RECORD_NOT_FOUND` (unchanged: zero-removed is deliberately not read as
25+
not-found, because the record is still there to GET).
26+
27+
The read-back that decides this is fail-closed: a read that cannot answer reports the record
28+
as NOT deleted and warns on the durability channel, because "the read failed" and "the row is
29+
gone" are opposite facts and only the second may claim a deletion.
30+
31+
Clause-②: no

‎packages/metadata-protocol/src/protocol.record-not-found.test.ts‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -161,6 +161,29 @@ describe('[#4435] deleteData reports what actually happened', () => {
161161
await expect(p.deleteData({ object: 'task', id: 'whatever' } as any))
162162
.resolves.toMatchObject({ success: true });
163163
});
164+
165+
it('a row that MATCHED and was NOT removed answers `success: false`, not a deletion', async () => {
166+
// The same measurement one outcome over. `success` stayed a LITERAL for
167+
// every answer that was not the contract's `false`, so a write that found
168+
// its row and deliberately did not remove it still reported a deletion.
169+
//
170+
// `sys_permission_set` is the shipped case: deleting a package-declared
171+
// set is an ADR-0005 RESET — the record re-projects to the declared body
172+
// instead of vanishing — and its envelope was byte-identical to a real
173+
// delete's, so a UI fired a success toast and showed the row again on
174+
// refresh. The engine's delete result declares a COUNT arm beside the
175+
// driver boolean, and zero rows removed is the one value that positively
176+
// means "the row is still there".
177+
const engine = {
178+
registry: { getObject: () => SCHEMA },
179+
delete: vi.fn(async () => 0),
180+
};
181+
const p = new ObjectStackProtocolImplementation(engine as any);
182+
const res = await p.deleteData({ object: 'task', id: 'still_there' } as any);
183+
// Not a 404 either: the caller can still GET this record, so reading zero
184+
// as not-found would trade one wrong answer for a louder one.
185+
expect(res).toEqual({ object: 'task', id: 'still_there', success: false });
186+
});
164187
});
165188

166189
describe('[#4435] deleteManyData reports per id, not per request', () => {

‎packages/metadata-protocol/src/protocol.ts‎

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11436,10 +11436,33 @@ export class ObjectStackProtocolImplementation implements
1143611436
// POSITIVE not-found signal, and inventing a 404 out of it would break
1143711437
// deletes against third-party drivers rather than report honestly.
1143811438
if (deleted === false) throw recordNotFoundError(request.object, request.id);
11439+
// The same measurement, one outcome over: `success` was still a
11440+
// LITERAL for every answer that was not the contract's `false`, so a
11441+
// row that MATCHED and was deliberately NOT removed reported a
11442+
// deletion. `sys_permission_set` is the shipped case — a
11443+
// package-declared set's delete is an ADR-0005 RESET, the record
11444+
// re-projects to the declared body instead of vanishing — and the
11445+
// envelope was byte-identical to a real delete, so a UI fired a
11446+
// success toast and showed the row again on refresh.
11447+
//
11448+
// `success` is declared "Whether deletion succeeded", and this is the
11449+
// only key on `DeleteDataResponseSchema` that can carry the
11450+
// difference; zero rows removed is a deletion that did not succeed.
11451+
// The engine's delete result declares two arms — the driver's boolean
11452+
// for a by-id write, a COUNT of rows removed otherwise — so a numeric
11453+
// zero is the one answer that positively means "the row is still
11454+
// there", and it is what the middleware that performs a reset now
11455+
// returns. It cannot say so with `false`: that value is spoken for by
11456+
// the not-found 404 above, which would be a second lie about a record
11457+
// this caller can still GET.
11458+
//
11459+
// Everything else keeps its #4435 reading, including an off-contract
11460+
// `undefined` from a third-party driver: only a POSITIVE zero is read
11461+
// as "not removed".
1143911462
return {
1144011463
object: request.object,
1144111464
id: request.id,
11442-
success: true
11465+
success: deleted !== 0
1144311466
};
1144411467
}
1144511468

‎packages/plugins/plugin-security/src/permission-set-projection.test.ts‎

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -887,6 +887,133 @@ describe('createPermissionSetWriteThrough (data door → metadata store)', () =>
887887
expect(ql.permRows[0].managed_by).toBe('package');
888888
});
889889

890+
it('the delete OUTCOME tells a real deletion apart from a packaged RESET', async () => {
891+
// Both endings above are correct — a runtime-only set goes, a packaged one
892+
// re-projects to its declaration. What the CALLER could not do was tell
893+
// them apart: the outcome this middleware wrote was `true` on every path
894+
// (it started there and the loop could only ever assign `true` again), so
895+
// the data door answered `200 {"success":true}` for both. A UI fired a
896+
// success toast on a permission set it had not deleted and showed the row
897+
// again on refresh — the two legs' own assertions above stayed green
898+
// throughout, because they read the STORE and never the answer.
899+
//
900+
// ⛔ The reset's outcome must not be `false` either: that value is the
901+
// driver contract's "no row matched", which `deleteData` turns into a
902+
// `404 RECORD_NOT_FOUND` — about a record this same caller can still GET.
903+
// Zero rows removed is the other declared arm and the only one that says
904+
// "it is still there"; `metadata-protocol`'s own
905+
// `protocol.record-not-found.test.ts` pins the mapping onto `success`.
906+
907+
// Leg 1 — runtime-only definition: the record really goes.
908+
const envQl = makeQl();
909+
const envProtocol = makeProtocol(envQl);
910+
registerPermissionSetProjection(envProtocol, { ql: envQl });
911+
await envProtocol.saveMetaItem({ type: 'permission', name: 'organization_admin', item: envBody() });
912+
const envCtx: any = {
913+
object: 'sys_permission_set',
914+
operation: 'delete',
915+
options: { where: { id: envQl.permRows[0].id } },
916+
context: userCtx,
917+
};
918+
await run(makeMiddleware(envQl, envProtocol), envCtx);
919+
expect(envQl.permRows.length, 'control: the env-authored record really went').toBe(0);
920+
expect(envCtx.result, 'a completed delete keeps answering the boolean arm').toBe(true);
921+
922+
// Leg 2 — artifact-backed definition: the record survives the RESET.
923+
const pkgQl = makeQl();
924+
const declaredBody = envBody({ name: 'crm_rep', systemPermissions: ['pkg.baseline'] });
925+
(pkgQl as any).registry = { listItems: (t: string) => (t === 'permission' ? [declaredBody] : []) };
926+
const pkgProtocol = makeProtocol(pkgQl, { crm_rep: declaredBody });
927+
registerPermissionSetProjection(pkgProtocol, { ql: pkgQl });
928+
pkgQl.permRows.push({
929+
id: 'ps_pkg', name: 'crm_rep', managed_by: 'package',
930+
package_id: 'com.example.crm', system_permissions: '["pkg.baseline"]',
931+
});
932+
seedLegacyOverlay(pkgQl, 'crm_rep', envBody({ name: 'crm_rep', systemPermissions: ['customized'] }));
933+
await projectPermissionMutation(pkgProtocol, { ql: pkgQl }, { type: 'permission', name: 'crm_rep', state: 'active', organizationId: null });
934+
const pkgCtx: any = {
935+
object: 'sys_permission_set',
936+
operation: 'delete',
937+
options: { where: { id: 'ps_pkg' } },
938+
context: userCtx,
939+
};
940+
await run(makeMiddleware(pkgQl, pkgProtocol), pkgCtx);
941+
expect(pkgQl.permRows.length, 'the record survives — the designed ADR-0094 reset').toBe(1);
942+
expect(
943+
typeof pkgCtx.result,
944+
'the COUNT arm — `false` would be read as not-found and 404 a record that is still there',
945+
).toBe('number');
946+
expect(pkgCtx.result, 'zero records removed').toBe(0);
947+
948+
// The card's own claim, stated as an assertion: the two endings no longer
949+
// hand the caller the same answer.
950+
expect(pkgCtx.result).not.toEqual(envCtx.result);
951+
});
952+
953+
it('a read-back that cannot ANSWER reports the record as NOT deleted (fail-closed)', async () => {
954+
// The outcome above is read off the record, which puts the whole answer at
955+
// the mercy of ONE read — and the obvious helper for it, `tryFind`, ends in
956+
// `catch { return []; }`. Probing through that helper makes a FAILED read
957+
// byte-identical to "the row is gone", so the door answers
958+
// `200 {"success":true}` with the record sitting exactly where it was:
959+
// this card's own lie, re-triggered by an outage instead of by a packaged
960+
// set. Nothing else in this file would have said a word about it.
961+
//
962+
// `unknown` is therefore counted as a survivor. A caller told "not
963+
// deleted" re-reads the record and learns the truth; a caller told
964+
// "deleted" has no reason to look again.
965+
const ql = makeQl();
966+
const declaredBody = envBody({ name: 'crm_rep', systemPermissions: ['pkg.baseline'] });
967+
(ql as any).registry = { listItems: (t: string) => (t === 'permission' ? [declaredBody] : []) };
968+
const protocol = makeProtocol(ql, { crm_rep: declaredBody });
969+
registerPermissionSetProjection(protocol, { ql });
970+
ql.permRows.push({
971+
id: 'ps_pkg', name: 'crm_rep', managed_by: 'package',
972+
package_id: 'com.example.crm', system_permissions: '["pkg.baseline"]',
973+
});
974+
975+
// Fail ONLY the read-back. Target resolution and the probe both read
976+
// `sys_permission_set` by id, so they are told apart by ORDER: the first
977+
// is the middleware resolving what the caller addressed, the second is the
978+
// read-back under test. `byIdReads` is asserted below so that a change in
979+
// that shape turns this test RED rather than leaving it vacuously green
980+
// with an injection that never fired.
981+
const realFind = ql.find.bind(ql);
982+
let byIdReads = 0;
983+
(ql as any).find = async (object: string, q: any) => {
984+
if (object === 'sys_permission_set' && q?.where?.id !== undefined) {
985+
byIdReads += 1;
986+
if (byIdReads >= 2) throw new Error('db unavailable');
987+
}
988+
return realFind(object, q);
989+
};
990+
991+
const warnings: { m: string; meta?: any }[] = [];
992+
const mw = createPermissionSetWriteThrough({
993+
ql, getProtocol: () => protocol,
994+
logger: { info: () => {}, warn: (m: string, meta?: any) => warnings.push({ m, meta }) },
995+
});
996+
const opCtx: any = {
997+
object: 'sys_permission_set',
998+
operation: 'delete',
999+
options: { where: { id: 'ps_pkg' } },
1000+
context: userCtx,
1001+
};
1002+
await run(mw, opCtx);
1003+
1004+
expect(byIdReads, 'the read-back really ran — otherwise this test proves nothing').toBeGreaterThanOrEqual(2);
1005+
expect(ql.permRows.length, 'the record is still right there').toBe(1);
1006+
expect(
1007+
opCtx.result,
1008+
'⛔ never `true`: that is the deletion claim this card exists to remove, and a failed read must not manufacture it',
1009+
).not.toBe(true);
1010+
expect(opCtx.result, 'counted as a survivor').toBe(0);
1011+
expect(
1012+
warnings.some((w) => String(w.m).includes('could not read the sys_permission_set record back')),
1013+
'the degradation is reported, not swallowed',
1014+
).toBe(true);
1015+
});
1016+
8901017
it('SINGLE-STORE kernel (no protocol): package rows keep the legacy two-doors refusal', async () => {
8911018
const ql = makeQl();
8921019
ql.permRows.push({ id: 'ps_pkg', name: 'crm_rep', managed_by: 'package', package_id: 'com.example.crm' });

‎packages/plugins/plugin-security/src/permission-set-projection.ts‎

Lines changed: 98 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,33 @@ export async function tryFind(ql: any, object: string, where: any, limit = 100,
124124
return Array.isArray(rows) ? rows : [];
125125
} catch { return []; }
126126
}
127+
128+
/**
129+
* Is this record still on the table? THREE outcomes, not two.
130+
*
131+
* {@link tryFind} deliberately collapses a failed read into `[]` — right for
132+
* every caller that wants "the rows I could see", wrong for anyone who has to
133+
* decide whether a row is GONE. `[]` from that helper means "no row matched OR
134+
* the read blew up", and a caller reading it as "gone" reports a deletion that
135+
* an outage invented. `listSeedOrganizationIds` in `per-organization-catalog.ts`
136+
* states the same rule for the same shape: zero organizations is "nothing to
137+
* seed", an unreadable table is "we do not know".
138+
*
139+
* So `unknown` is its own answer and the caller must decide what it means —
140+
* ⛔ never folded into `gone` here. The context and the read are otherwise
141+
* IDENTICAL to `tryFind`'s (no `transaction` key, so an ambient handle still
142+
* carries it), because the difference under test is the catch and nothing else.
143+
*/
144+
async function probeRecordSurvival(
145+
ql: any,
146+
object: string,
147+
id: unknown,
148+
): Promise<'gone' | 'present' | 'unknown'> {
149+
try {
150+
const rows = await ql.find(object, { where: { id }, limit: 1 }, { context: seedCtx() });
151+
return (Array.isArray(rows) ? rows : [])[0] ? 'present' : 'gone';
152+
} catch { return 'unknown'; }
153+
}
127154
/**
128155
* ⛔ The `catch` RECORDS the refusal before it answers, when the caller passed
129156
* a log to record into.
@@ -1292,7 +1319,47 @@ export function createPermissionSetWriteThrough(
12921319
// an ADR-0005 RESET — and the record re-projects to the declared body
12931320
// instead of vanishing (a packaged definition cannot be deleted from the
12941321
// environment).
1295-
let lastOutcome: any = true;
1322+
//
1323+
// The RESET is the designed outcome and stays exactly as it is. What this
1324+
// loop owes the CALLER is the ability to tell the two endings apart: the
1325+
// outcome below used to start at `true` and could only ever be assigned
1326+
// `true` again, so a reset answered the data door with the same
1327+
// `{"success":true}` a real deletion does — the difference was named only
1328+
// in the `logger.info` line above, which no caller can read. A machine
1329+
// surface that reports a refusal as a deletion is the one thing
1330+
// "machine-readable surfaces must not lie" forbids, and on a
1331+
// security-configuration write it tells an operator a permission set is
1332+
// gone while it is still being enforced.
1333+
//
1334+
// The outcome is therefore the COUNT of target RECORDS that actually went
1335+
// — the number arm of this engine method's declared result ("how many rows
1336+
// the delete removed"), which the data door turns into an honest
1337+
// `success: false` on a 200. Every target removed keeps answering `true`,
1338+
// the boolean arm's value for a completed by-id delete, so a real deletion
1339+
// is unchanged on the wire.
1340+
//
1341+
// ⛔ NOT `false` for the reset: the boolean arm's `false` is the driver
1342+
// contract's "no row matched", which the data door turns into a
1343+
// `404 RECORD_NOT_FOUND` — and the record is still right there, so that
1344+
// would trade this lie for a louder one and hand a UI a reason to drop the
1345+
// row it must keep showing.
1346+
//
1347+
// Zero also covers every other way a record survives this loop: a metadata
1348+
// delete that lands on a package-owned RECORD (left to the package door),
1349+
// a retire whose `ql.delete` failed, and a read-back that could not answer
1350+
// at all. All three leave the row in place — or leave us unable to say it
1351+
// does not — and none is a deletion the caller should be told happened.
1352+
//
1353+
// ⛔ The outcome is read off the RECORD, never off the re-projection's own
1354+
// `deleted` counter. On a kernel wired to the ADR-0094 AWAITED projector —
1355+
// the production shape — `deleteMetaItem` has already retired the record
1356+
// by the time control returns here, so the idempotent re-projection below
1357+
// finds no row and legitimately reports `deleted: 0` for a delete that
1358+
// really happened. Reading that counter would have answered
1359+
// `success: false` for EVERY delete, which is the same defect with its
1360+
// sign flipped: honest about the packaged set and a fresh lie about the
1361+
// ordinary one. `permission-set-projection.test.ts` holds both legs.
1362+
let removed = 0;
12961363
for (const row of targets) {
12971364
await protocol.deleteMetaItem({ type: 'permission', name: row.name, ...actorArg });
12981365
const res = await projectPermissionMutation(protocol, deps, {
@@ -1301,9 +1368,37 @@ export function createPermissionSetWriteThrough(
13011368
if (res && (res.seeded + res.updated) > 0) {
13021369
logger?.info?.('[security] permission set reset to its declared baseline (artifact-backed; ADR-0094)', { name: row.name });
13031370
}
1304-
lastOutcome = res?.deleted ? true : lastOutcome;
1371+
// The caller addressed a RECORD by id, so that is the question this
1372+
// answers: is it still there? A row re-projected under the same id is
1373+
// the reset; a row that is gone is the deletion.
1374+
//
1375+
// ⛔ The probe has THREE outcomes and this loop counts only ONE of them.
1376+
// Probing through `tryFind` — whose catch returns `[]` — would have made
1377+
// a FAILED read indistinguishable from "the row is gone", and answered
1378+
// `success: true` with the record sitting exactly where it was: this
1379+
// card's own lie with a different trigger (an outage instead of a
1380+
// packaged set). `unknown` is therefore counted as a SURVIVOR, the
1381+
// fail-closed direction: a caller told "not deleted" re-reads the record
1382+
// and finds out, a caller told "deleted" has no reason to look again.
1383+
// It is also the only direction that cannot be produced by a broken
1384+
// read — `gone` now requires a read that actually ANSWERED.
1385+
const survival = await probeRecordSurvival(ql, 'sys_permission_set', row.id);
1386+
if (survival === 'unknown') {
1387+
// Durability channel: the write half of this delete already ran, so
1388+
// the row's fate is decided and only our READING of it failed. Say so
1389+
// once, naming what the caller was told and how to settle it.
1390+
logger?.warn?.(
1391+
'[security] could not read the sys_permission_set record back after its metadata delete, so the ' +
1392+
'data-door answer reports it as NOT deleted (fail-closed). The delete itself already ran; re-read the ' +
1393+
'record to see whether it was removed or reset to its declared baseline.',
1394+
{ name: row.name, id: row.id },
1395+
);
1396+
}
1397+
if (survival === 'gone') removed += 1;
13051398
}
1306-
opCtx.result = lastOutcome;
1399+
// `targets` is non-empty here — the early return above sends an empty
1400+
// target set to the driver — so this is never the vacuous `true`.
1401+
opCtx.result = removed === targets.length ? true : removed;
13071402
return;
13081403
};
13091404
}

0 commit comments

Comments
 (0)