From ce9a5677c4f6cae10234eacfe20013e0122837e1 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 01:11:53 +0000 Subject: [PATCH 1/3] feat(service-settings): report-only sys_secret orphan classifier (#8103) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lands the non-destructive half of #8103: an operator-facing answer to "which sys_secret rows are orphaned, and why", with no deletion and no migration. The sweep's vehicle stays an open maintainer decision. Re-measuring #8063's three reachability facts falsified the middle one: sys_setting.value_enc is NOT the only column holding a sys_secret handle. The store has three producers (SettingsService, the engine's secret-field channel storing `secret:` on arbitrary business rows, and the datasource credential binder storing `sys_secret:` at external.credentialsRef), and the engine's holders are not statically enumerable. So the classifier reports a row it cannot attribute as `unattributable` rather than `orphaned` — the guard that stands between a future sweep and a live datasource credential. Also pins the two directional guards the card names: a rotateKey() re-wrap keeps the handle stable and reads as in-force (rotation metadata never decides a verdict), and a legacy inline value_enc contributes no handle while flagging its (namespace, key) siblings. Tests drive a real ObjectQL over the real SysSetting/SysSecret schemas: the orphan-generating mechanism is reproduced rather than cited, and the fixture carries all four classes so the pins are not vacuous. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ARidKDYSCD56LaygrvDPnk --- .changeset/olive-pots-tease.md | 31 + .../services/service-settings/src/index.ts | 19 + .../src/sys-secret-orphan-report.test.ts | 634 ++++++++++++++++++ .../src/sys-secret-orphan-report.ts | 364 ++++++++++ 4 files changed, 1048 insertions(+) create mode 100644 .changeset/olive-pots-tease.md create mode 100644 packages/services/service-settings/src/sys-secret-orphan-report.test.ts create mode 100644 packages/services/service-settings/src/sys-secret-orphan-report.ts diff --git a/.changeset/olive-pots-tease.md b/.changeset/olive-pots-tease.md new file mode 100644 index 0000000000..3d95de3a9a --- /dev/null +++ b/.changeset/olive-pots-tease.md @@ -0,0 +1,31 @@ +--- +'@objectstack/service-settings': minor +--- + +Add a **report-only** classifier for `sys_secret` orphans (#8103), plus the reachability +measurements a sweep would depend on. + +`classifySysSecretRows()` is a pure, read-only function over caller-supplied snapshots: it +never writes, never deletes and never decrypts, and its `SecretRowSnapshot` type +deliberately has no `ciphertext` member. It reports which `sys_secret` rows the settings +subsystem still references (`in_force`), which are unreferenced and attributable to a +declared encrypted specifier (`orphaned`), and which it cannot attribute at all +(`unattributable`). + +That third verdict exists because re-measuring #8063's reachability argument **falsified** +one of its three facts: `sys_setting.value_enc` is *not* the only column that holds a +`sys_secret` handle. The store has three producers — `SettingsService`, the engine's +`secret`-field channel (which stores `secret:` on any business row, including +tenant-authored objects), and the datasource credential binder (`sys_secret:` at +`external.credentialsRef`). Two are invisible from this package and the engine's set of +holders is not statically enumerable, so "unreferenced by `sys_setting`" is not +"unreferenced". Rows that cannot be attributed are reported, never classified as orphans, +and the report carries explicit caveats naming its own blind spots. + +The classifier also pins the two directional guards the card names: a row re-wrapped in +place by `rotateKey()` keeps its handle and is reported `in_force` (rotation metadata never +decides a verdict), and a legacy inline `value_enc` contributes no handle to the referenced +set while flagging any `sys_secret` row sharing its `(namespace, key)`. + +No deletion ships with this change — the vehicle for removing orphans remains an open +maintainer decision. diff --git a/packages/services/service-settings/src/index.ts b/packages/services/service-settings/src/index.ts index 09038292a8..8bc72d7c54 100644 --- a/packages/services/service-settings/src/index.ts +++ b/packages/services/service-settings/src/index.ts @@ -66,6 +66,25 @@ export { redactSecretValues, dropEchoedSecretMasks, } from './settings-secret-redaction.js'; +// #8103 — REPORT-ONLY classification of `sys_secret` rows against the settings +// subsystem's references. Published because the operator-facing vehicle for it +// (admin command / opt-in script) is still an open maintainer decision and will +// live outside this package; the classifier is the part that is safe to settle +// now. ⛔ Contains no deletion and must not grow one — and note the verdict +// vocabulary's third value: `sys_secret` has three producers, so "unreferenced +// by `sys_setting`" is NOT "unreferenced". See the module header. +export { + classifySysSecretRows, + collectEncryptedSpecifierRefs, + isSecretHandle, + SECRET_HANDLE_PREFIX, + type ClassifiedSecretRow, + type EncryptedSpecifierRef, + type SecretRowSnapshot, + type SecretRowVerdict, + type SettingRowSnapshot, + type SysSecretOrphanReport, +} from './sys-secret-orphan-report.js'; export { settingsObjects, settingsPluginManifestHeader, diff --git a/packages/services/service-settings/src/sys-secret-orphan-report.test.ts b/packages/services/service-settings/src/sys-secret-orphan-report.test.ts new file mode 100644 index 0000000000..b97866c0a0 --- /dev/null +++ b/packages/services/service-settings/src/sys-secret-orphan-report.test.ts @@ -0,0 +1,634 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #8103 — the **report-only** half: which `sys_secret` rows are orphaned, and + * why the answer is narrower than the card assumed. + * + * ## What this file measures, and why it runs against the real engine + * + * The card asks for #8063's three reachability facts to be **re-measured + * rather than cited**, because a sweep over existing rows has a far wider + * blast radius than the single point-delete those facts were argued for. A + * hand-written fake mirroring the shapes we expect would only prove the fake + * matches the fix, so the parts that can be driven end-to-end are driven + * through a real `ObjectQL` over the real `SysSetting` / `SysSecret` schemas — + * the same pieces the running server bolts together, with + * `@objectstack/objectql` aliased to SOURCE by `vitest.config.ts`. + * + * The three facts, as measured here: + * + * 1. **Handle ids are minted per `encrypt()` call** — GREEN (case 1). + * 2. **`sys_setting.value_enc` is the only column holding a handle** — ❌ + * **RED / FALSIFIED** (cases 2–3). The engine's own `secret`-field channel + * writes `sys_secret` rows and stores `secret:` on an arbitrary + * business row; the datasource binder does the same at + * `external.credentialsRef`. Case 3 is the safety pin that follows: a row + * this package cannot attribute is `unattributable`, never `orphaned`. + * 3. **Audit records digests, not handles** — GREEN (case 4). + * + * Case 5 reproduces the **orphan-generating mechanism itself** (the filer's + * 7 → 8 → 9 anecdote) rather than taking the count on trust, then classifies + * the genuine table state it produced. Cases 6–8 pin the two directional + * guards the card names, over a fixture carrying **all four classes** + * (orphaned, in-force, legacy-inline, re-wrapped) — a fixture missing any of + * them would make the pins vacuous. + */ + +import { describe, expect, it } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SysSecret, SysSetting } from '@objectstack/platform-objects/system'; +import type { SettingsManifest } from '@objectstack/spec/system'; +import type { CryptoHandle, ICryptoProvider } from '@objectstack/spec/contracts'; +import { SettingsService } from './settings-service.js'; +import { wrapEngineAsSettingsEngine } from './settings-service-plugin.js'; +import { LocalCryptoProvider } from './local-crypto-provider.js'; +import type { SettingsEngine, SettingsSecretStore } from './settings-service.types.js'; +import { + classifySysSecretRows, + collectEncryptedSpecifierRefs, + isSecretHandle, + SECRET_HANDLE_PREFIX, + type SecretRowSnapshot, + type SettingRowSnapshot, +} from './sys-secret-orphan-report.js'; + +// --------------------------------------------------------------------------- +// Fixtures +// --------------------------------------------------------------------------- + +const OWNER_PACKAGE = 'com.objectstack.test.sys-secret-orphan-report'; + +const smsManifest: SettingsManifest = { + namespace: 'sms', + version: 1, + label: 'SMS', + scope: 'tenant', + readPermission: 'setup.access', + writePermission: 'setup.write', + specifiers: [ + { type: 'text', key: 'twilio_account_sid', label: 'Account SID', required: false }, + { + type: 'password', + key: 'twilio_auth_token', + label: 'Auth token', + required: false, + encrypted: true, + }, + ], +}; + +type Store = Map>>; + +/** A driver over plain Maps — enough of `IDataDriver` for these paths. */ +function makeMemoryDriver() { + const store: Store = new Map(); + let nextId = 0; + const copy = (r: Record) => ({ ...r }); + const rowsOf = (object: string) => { + let s = store.get(object); + if (!s) { + s = new Map(); + store.set(object, s); + } + return s; + }; + const matches = (row: Record, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + return Object.entries(where).every(([k, v]) => { + if (k.startsWith('$')) return true; + const expected = v && typeof v === 'object' && '$eq' in (v as any) ? (v as any).$eq : v; + return (row[k] ?? null) === (expected ?? null); + }); + }; + const driver: any = { + name: 'memory', + version: '0.0.0', + supports: {} as any, + async connect() {}, + async disconnect() {}, + async checkHealth() { + return true; + }, + async execute() { + return null; + }, + async find(object: string, ast: any) { + return [...rowsOf(object).values()].filter((r) => matches(r, ast?.where)).map(copy); + }, + async findOne(object: string, ast: any) { + for (const r of rowsOf(object).values()) if (matches(r, ast?.where)) return copy(r); + return null; + }, + async create(object: string, data: Record) { + nextId += 1; + const id = (data.id as string) ?? `row_${nextId}`; + const row = { ...data, id }; + rowsOf(object).set(id, row); + return copy(row); + }, + async update(object: string, id: string, data: Record) { + const s = rowsOf(object); + const cur = s.get(id); + if (!cur) return null; + const next = { ...cur, ...data, id }; + s.set(id, next); + return copy(next); + }, + async upsert(object: string, data: Record) { + const id = data.id as string | undefined; + return id && rowsOf(object).has(id) ? this.update(object, id, data) : this.create(object, data); + }, + async delete(object: string, id: string) { + return rowsOf(object).delete(id); + }, + async count(object: string, ast: any) { + return (await this.find(object, ast)).length; + }, + async bulkCreate(object: string, rows: Record[]) { + return Promise.all(rows.map((r) => this.create(object, r))); + }, + async bulkUpdate() { + return []; + }, + async bulkDelete() {}, + async updateMany(object: string, ast: any, data: Record) { + const rows = await this.find(object, ast); + const s = rowsOf(object); + for (const r of rows) s.set(r.id as string, { ...s.get(r.id as string), ...data, id: r.id }); + return rows.length; + }, + async deleteMany(object: string, ast: any) { + const rows = await this.find(object, ast); + for (const r of rows) rowsOf(object).delete(r.id as string); + return rows.length; + }, + async syncSchema() {}, + async dropTable() {}, + async beginTransaction() { + return { commit: async () => {}, rollback: async () => {} }; + }, + async commit() {}, + async rollback() {}, + }; + return { driver, rowsOf }; +} + +/** + * Reproduces the PRE-#8030 adapter: identical to `wrapEngineAsSettingsEngine` + * except that it DROPS `context` on the way to the engine. + * + * That single omission is the whole defect — `sys_setting.value_enc` is + * declared `readonly: true` and the engine strips author-declared read-only + * columns from a NON-system caller's UPDATE (`stripReadonlyFields`, gated on + * `context.isSystem`), while the INSERT path is exempt (#3413). It is also + * exactly the breakage `SettingsEngine`'s doc comment warns an adapter author + * about ("⛔ An adapter over `IDataEngine` MUST forward this"), so reproducing + * the orphan mechanism and pinning that warning are the same measurement. + */ +function wrapEngineDroppingContext(engine: any): SettingsEngine { + const real = wrapEngineAsSettingsEngine(engine); + return { + find: real.find.bind(real), + insert: real.insert.bind(real), + async update(objectName, opts) { + const { context: _dropped, ...withoutContext } = opts as any; + return real.update(objectName, withoutContext); + }, + }; +} + +/** + * The four pieces the running server bolts together. + * + * `withDelete: false` drops the store's `delete`, which makes `reapRotatedSecret` + * no-op (`SettingsSecretStore.delete` is OPTIONAL precisely so stores that + * predate #8063 keep working). Faithfully reproducing the pre-#8030 era needs + * BOTH that and `forwardContext: false`, because the context fix and the reaper + * landed together in PR #8063 — an instance that dropped `context` while a + * reaper was live never existed in the field. + */ +async function boot(opts: { forwardContext?: boolean; withDelete?: boolean } = {}) { + const engine = new ObjectQL(); + const { driver, rowsOf } = makeMemoryDriver(); + engine.registerDriver(driver, true); + await engine.init(); + for (const o of [SysSetting, SysSecret]) { + engine.registry.registerObject(o as any, OWNER_PACKAGE); + } + + const eng: any = engine; + const secretStore: SettingsSecretStore = { + async insert(row) { + await eng.insert('sys_secret', row, { bypassTenantAudit: true }); + return { id: row.id }; + }, + async get(id) { + const rows = await eng.find('sys_secret', { where: { id }, limit: 1, bypassTenantAudit: true }); + return (Array.isArray(rows) ? rows[0] : null) ?? null; + }, + async update(id, patch) { + await eng.update('sys_secret', { id, ...patch }, { bypassTenantAudit: true }); + }, + ...(opts.withDelete === false + ? {} + : { + async delete(id: string) { + await eng.delete('sys_secret', { + where: { id }, + bypassTenantAudit: true, + context: { isSystem: true }, + }); + }, + }), + }; + + const auditEntries: Array> = []; + const svc = new SettingsService({ + env: {}, + engine: + opts.forwardContext === false + ? wrapEngineDroppingContext(engine) + : wrapEngineAsSettingsEngine(engine as any), + cryptoProvider: new LocalCryptoProvider(), + secretStore, + auditWriter: { + write(entry) { + auditEntries.push({ ...entry } as Record); + }, + }, + }); + svc.registerManifest(smsManifest); + + /** Snapshot the real tables in the shape the report consumes. */ + const snapshot = () => { + const secrets: SecretRowSnapshot[] = [...rowsOf('sys_secret').values()].map((r: any) => ({ + id: r.id, + namespace: r.namespace, + key: r.key, + version: r.version ?? null, + kms_key_id: r.kms_key_id ?? null, + created_at: r.created_at ?? null, + rotated_at: r.rotated_at ?? null, + })); + const settingRows: SettingRowSnapshot[] = [...rowsOf('sys_setting').values()].map((r: any) => ({ + namespace: r.namespace, + key: r.key, + scope: r.scope ?? null, + user_id: r.user_id ?? null, + value_enc: r.value_enc ?? null, + encrypted: r.encrypted ?? null, + })); + return { secrets, settingRows }; + }; + + return { engine, svc, rowsOf, snapshot, auditEntries }; +} + +const attributableTo = collectEncryptedSpecifierRefs([smsManifest]); + +// --------------------------------------------------------------------------- +// Reachability fact 1 — handle ids are minted per encrypt() call +// --------------------------------------------------------------------------- + +describe('#8103 reachability fact 1 — handle ids are minted per encrypt() call', () => { + it('mints a fresh sec_ id per encrypt, and rotateKey keeps the id STABLE', async () => { + const provider = new LocalCryptoProvider(); + const ctx = { namespace: 'sms', key: 'twilio_auth_token' }; + + const a = await provider.encrypt('token-a', ctx); + const b = await provider.encrypt('token-a', ctx); // same plaintext, same ctx + + // Per-CALL, not per-(namespace,key) and not per-plaintext: encrypting the + // identical value twice yields two distinct handles naming two ciphertexts. + expect(a.id).not.toBe(b.id); + expect(isSecretHandle(a.id)).toBe(true); + expect(isSecretHandle(b.id)).toBe(true); + + // The prefix constant is pinned against the REAL minter rather than against + // a copy of the literal in `SettingsService`. + expect(a.id.startsWith(SECRET_HANDLE_PREFIX)).toBe(true); + + // rotateKey re-wraps IN PLACE — same id, bumped version. This is why a + // re-wrapped row is still the referenced one and must never read as retired. + const rewrapped = await provider.rotateKey(a, ctx); + expect(rewrapped.id).toBe(a.id); + expect(rewrapped.version).toBe(a.version + 1); + expect(await provider.decrypt(rewrapped, ctx)).toBe('token-a'); + }); +}); + +// --------------------------------------------------------------------------- +// Reachability fact 2 — FALSIFIED +// --------------------------------------------------------------------------- + +describe('#8103 reachability fact 2 — `sys_setting.value_enc` is NOT the only holder', () => { + /** + * The engine's own `secret`-field channel is a second producer of `sys_secret` + * rows, and it stores its handle as `secret:` on an arbitrary business + * row. Nothing about it is visible from `sys_setting`. + * + * (The third producer, the datasource credential binder, stores + * `sys_secret:` at `external.credentialsRef` — same conclusion, and this + * package does not depend on `service-datasource`, so it is measured by + * source rather than driven here. `sys_secret`'s own schema names all three.) + */ + it('the engine secret-field channel writes sys_secret rows no sys_setting row references', async () => { + const engine = new ObjectQL(); + const { driver, rowsOf } = makeMemoryDriver(); + engine.registerDriver(driver, true); + await engine.init(); + for (const o of [SysSetting, SysSecret]) { + engine.registry.registerObject(o as any, OWNER_PACKAGE); + } + // A perfectly ordinary business object carrying a `secret` field — the kind + // a tenant can author. This is the set of holders that is not statically + // enumerable. + engine.registry.registerObject( + { + name: 'ext_vendor', + label: 'Vendor', + fields: { + id: { name: 'id', label: 'ID', type: 'text' }, + name: { name: 'name', label: 'Name', type: 'text' }, + api_key: { name: 'api_key', label: 'API key', type: 'secret' }, + }, + } as any, + OWNER_PACKAGE, + ); + + const provider = new LocalCryptoProvider(); + (engine as any).setCryptoProvider(provider as ICryptoProvider); + + const eng: any = engine; + await eng.insert('ext_vendor', { name: 'acme', api_key: 'vendor-live-key' }); + + const secretRows = [...rowsOf('sys_secret').values()]; + const settingRows = [...rowsOf('sys_setting').values()]; + + // A real ciphertext row exists... + expect(secretRows).toHaveLength(1); + // ...produced under (object name, field name), not (settings ns, specifier). + expect(secretRows[0]!.namespace).toBe('ext_vendor'); + expect(secretRows[0]!.key).toBe('api_key'); + // ...and NOTHING in sys_setting references it. Fact 2, as stated, is false. + expect(settingRows).toHaveLength(0); + + // The handle lives on the business row, behind a different ref spelling. + const vendor = (await eng.find('ext_vendor', {}))[0] as any; + const storedRef = [...rowsOf('ext_vendor').values()][0]!.api_key as string; + expect(storedRef.startsWith('secret:')).toBe(true); + expect(storedRef.endsWith(secretRows[0]!.id as string)).toBe(true); + expect(vendor).toBeDefined(); + }); + + it('SAFETY PIN: a row the settings report cannot attribute is `unattributable`, never `orphaned`', () => { + // Exactly the state the previous case produced: a live foreign-producer row, + // referenced by nothing in `sys_setting`. + const report = classifySysSecretRows({ + secrets: [{ id: 'sec_vendorlive', namespace: 'ext_vendor', key: 'api_key', version: 1 }], + settingRows: [], + attributableTo, + }); + + expect(report.counts.orphaned).toBe(0); + expect(report.counts.unattributable).toBe(1); + expect(report.rows[0]!.verdict).toBe('unattributable'); + // ⛔ This is the assertion that stands between a sweep and a live datasource + // credential / business-row secret. Do not relax it. + expect(report.rows[0]!.verdict).not.toBe('orphaned'); + expect(report.caveats.join(' ')).toContain('three producers'); + }); +}); + +// --------------------------------------------------------------------------- +// Reachability fact 3 — audit records digests, not handles +// --------------------------------------------------------------------------- + +describe('#8103 reachability fact 3 — the audit trail records digests, not handles', () => { + it('no audit entry carries a sec_ handle, so audit survives a deletion (and cannot reconstruct one)', async () => { + const { svc, auditEntries, snapshot } = await boot(); + await svc.set('sms', 'twilio_auth_token', 'tok-1'); + await svc.set('sms', 'twilio_auth_token', 'tok-2'); + + expect(auditEntries.length).toBeGreaterThan(0); + const serialised = JSON.stringify(auditEntries); + expect(serialised).not.toContain(SECRET_HANDLE_PREFIX); + expect(serialised).not.toContain('tok-1'); + expect(serialised).not.toContain('tok-2'); + + // What IS recorded is a content digest. + const encryptedEntry = auditEntries.find((e) => e.encrypted === true)!; + expect(String(encryptedEntry.newHash)).toMatch(/^sha256:[0-9a-f]{64}$/); + + // The other edge of the same fact: because no handle is ever recorded, the + // audit trail cannot tell an operator which handles once existed — a row + // deleted in error is untraceable from here. + const { secrets } = snapshot(); + for (const s of secrets) expect(serialised).not.toContain(s.id); + }); +}); + +// --------------------------------------------------------------------------- +// The orphan-generating mechanism, reproduced rather than cited +// --------------------------------------------------------------------------- + +describe('#8103 the pre-#8030 orphan mechanism, reproduced on this checkout', () => { + it('three writes on a faithful pre-fix instance grow sys_secret 1→2→3 with value_enc pinned', async () => { + const { svc, rowsOf } = await boot({ forwardContext: false, withDelete: false }); + + await svc.set('sms', 'twilio_auth_token', 'tok-1'); + const afterFirst = [...rowsOf('sys_secret').values()].length; + const pinnedHandle = [...rowsOf('sys_setting').values()][0]!.value_enc as string; + + await svc.set('sms', 'twilio_auth_token', 'tok-2'); + const afterSecond = [...rowsOf('sys_secret').values()].length; + + await svc.set('sms', 'twilio_auth_token', 'tok-3'); + const afterThird = [...rowsOf('sys_secret').values()].length; + + // The filer's 7 → 8 → 9, reproduced from a clean table: one new ciphertext + // row per write, none of them ever referenced. + expect([afterFirst, afterSecond, afterThird]).toEqual([1, 2, 3]); + + // And the handle never moved — the FIRST value is the one still in force. + const settingRows = [...rowsOf('sys_setting').values()]; + expect(settingRows).toHaveLength(1); + expect(settingRows[0]!.value_enc).toBe(pinnedHandle); + }); + + it('classifies the state that mechanism produced: 1 in-force + 2 orphaned', async () => { + const { svc, snapshot } = await boot({ forwardContext: false, withDelete: false }); + await svc.set('sms', 'twilio_auth_token', 'tok-1'); + await svc.set('sms', 'twilio_auth_token', 'tok-2'); + await svc.set('sms', 'twilio_auth_token', 'tok-3'); + + const { secrets, settingRows } = snapshot(); + const report = classifySysSecretRows({ secrets, settingRows, attributableTo }); + + expect(report.counts.total).toBe(3); + expect(report.counts.inForce).toBe(1); + expect(report.counts.orphaned).toBe(2); + expect(report.counts.unattributable).toBe(0); + + // The in-force row is the one the live `value_enc` names. + const inForce = report.rows.find((r) => r.verdict === 'in_force')!; + expect(settingRows[0]!.value_enc).toBe(inForce.id); + + // ⚠️ The card's security framing is INVERTED for this population, and the + // direction matters to whoever picks the sweep's vehicle. The card reads + // the orphans as "a decryptable copy of a credential an administrator + // believed they had retired". Measured, they are the opposite: the value + // still IN FORCE is the oldest one (`tok-1`) — the credential the admin + // believed replaced — while the ORPHANS hold `tok-2`/`tok-3`, the values + // the admin intended to set and which never took effect. + // + // So deleting the orphans does NOT retire the exposed credential; the + // exposed one is referenced and a sweep will not touch it. It is also why + // the orphans are not merely stale: if the admin rotated the credential at + // the provider too, `tok-3` may be CURRENTLY VALID there. + expect((await svc.get('sms', 'twilio_auth_token')).value).toBe('tok-1'); + + // Report mode names ids, never cipher material or plaintext. + const serialised = JSON.stringify(report); + expect(serialised).not.toContain('tok-1'); + expect(serialised).not.toContain('ciphertext'); + }); + + it('with the context forwarded (post-#8030), the same three writes leave no orphan at all', async () => { + const { svc, snapshot } = await boot(); + await svc.set('sms', 'twilio_auth_token', 'tok-1'); + await svc.set('sms', 'twilio_auth_token', 'tok-2'); + await svc.set('sms', 'twilio_auth_token', 'tok-3'); + + const { secrets, settingRows } = snapshot(); + const report = classifySysSecretRows({ secrets, settingRows, attributableTo }); + + // The forward-only reaper holds the population flat: this card is + // remediation of a FROZEN set, not containment of a growing one. + expect(report.counts.total).toBe(1); + expect(report.counts.inForce).toBe(1); + expect(report.counts.orphaned).toBe(0); + }); +}); + +// --------------------------------------------------------------------------- +// All four classes in one fixture + the directional guards +// --------------------------------------------------------------------------- + +describe('#8103 classification over a fixture carrying ALL FOUR classes', () => { + /** + * Deliberately one fixture, not four: the card's vacuity trap is a classifier + * test that is trivially green because the fixture contains no legacy-inline + * and no re-wrapped row. All four are present here. + */ + const secrets: SecretRowSnapshot[] = [ + // (a) in-force, plain + { id: 'sec_inforce', namespace: 'sms', key: 'twilio_auth_token', version: 1 }, + // (b) orphaned — attributable, referenced by nothing + { id: 'sec_orphan', namespace: 'sms', key: 'twilio_auth_token', version: 1 }, + // (c) re-wrapped IN PLACE and still referenced — version bumped, id stable + { + id: 'sec_rewrapped', + namespace: 'mail', + key: 'smtp_password', + version: 3, + kms_key_id: 'local:v3', + rotated_at: '2026-07-01T00:00:00.000Z', + }, + // (d) foreign producer — the engine secret-field channel + { id: 'sec_vendor', namespace: 'ext_vendor', key: 'api_key', version: 1 }, + ]; + + const settingRows: SettingRowSnapshot[] = [ + { namespace: 'sms', key: 'twilio_auth_token', value_enc: 'sec_inforce', encrypted: true }, + { namespace: 'mail', key: 'smtp_password', value_enc: 'sec_rewrapped', encrypted: true }, + // (e) LEGACY INLINE — the ciphertext itself sits in `value_enc`, no handle. + { namespace: 'mail', key: 'api_key', value_enc: 'b64:c2VjcmV0LXZhbHVl', encrypted: true }, + ]; + + const refs = [ + ...attributableTo, + { namespace: 'mail', key: 'smtp_password' }, + { namespace: 'mail', key: 'api_key' }, + ]; + + const report = classifySysSecretRows({ secrets, settingRows, attributableTo: refs }); + const byId = new Map(report.rows.map((r) => [r.id, r])); + + it('separates in-force / orphaned / unattributable', () => { + expect(report.counts).toEqual({ total: 4, inForce: 2, orphaned: 1, unattributable: 1 }); + expect(byId.get('sec_inforce')!.verdict).toBe('in_force'); + expect(byId.get('sec_orphan')!.verdict).toBe('orphaned'); + expect(byId.get('sec_vendor')!.verdict).toBe('unattributable'); + }); + + it('GUARD: a re-wrapped row is IN FORCE, not retired — version/rotated_at never decide a verdict', () => { + const rewrapped = byId.get('sec_rewrapped')!; + // The trap this pins: `version > 1` / `rotated_at != null` reads like "this + // was rotated, so it is the old one". `rotateKey` keeps the handle stable, + // so that row is the value IN FORCE — collecting it deletes a live secret. + expect(rewrapped.verdict).toBe('in_force'); + expect(rewrapped.rewrapped).toBe(true); + expect(rewrapped.reason).toContain('not a retirement'); + + // And the evidence is inert: stripping it changes no verdict. + const withoutEvidence = classifySysSecretRows({ + secrets: secrets.map((s) => ({ ...s, version: 1, rotated_at: null, kms_key_id: null })), + settingRows, + attributableTo: refs, + }); + expect(withoutEvidence.counts).toEqual(report.counts); + }); + + it('GUARD: a legacy inline value contributes no handle, and flags its (namespace,key) siblings', () => { + // #8063 guards its DELETE with a `sec_` prefix check; the guard needed here + // is the other direction — an inline ciphertext must not enter the + // referenced set, and must not make a sibling row look quietly collectable. + expect(report.legacyInlineRows).toEqual([ + { namespace: 'mail', key: 'api_key', scope: null, user_id: null }, + ]); + expect(isSecretHandle('b64:c2VjcmV0LXZhbHVl')).toBe(false); + + const withSibling = classifySysSecretRows({ + secrets: [...secrets, { id: 'sec_mailapikey', namespace: 'mail', key: 'api_key', version: 1 }], + settingRows, + attributableTo: refs, + }); + const sibling = withSibling.rows.find((r) => r.id === 'sec_mailapikey')!; + expect(sibling.verdict).toBe('orphaned'); + expect(sibling.legacyInlineSibling).toBe(true); + expect(sibling.reason).toContain('LEGACY INLINE'); + expect(withSibling.caveats.join(' ')).toContain('inline ciphertext'); + }); + + it('reports its own blind spots rather than a bare count', () => { + expect(report.caveats.length).toBeGreaterThanOrEqual(3); + expect(report.caveats.join(' ')).toContain('not statically enumerable'); + expect(report.caveats.join(' ')).toContain('HEURISTIC'); + }); + + it('an empty attribution set yields zero orphans — the safe direction', () => { + const none = classifySysSecretRows({ secrets, settingRows, attributableTo: [] }); + expect(none.counts.orphaned).toBe(0); + expect(none.counts.unattributable).toBe(2); // the two unreferenced rows + expect(none.caveats.join(' ')).toContain('No encrypted specifiers were supplied'); + }); +}); + +describe('#8103 collectEncryptedSpecifierRefs', () => { + it('collects only `encrypted: true` specifiers', () => { + expect(collectEncryptedSpecifierRefs([smsManifest])).toEqual([ + { namespace: 'sms', key: 'twilio_auth_token' }, + ]); + }); + + it('tolerates an empty / specifier-less manifest set', () => { + expect(collectEncryptedSpecifierRefs([])).toEqual([]); + expect( + collectEncryptedSpecifierRefs([{ namespace: 'x', version: 1, label: 'X' } as SettingsManifest]), + ).toEqual([]); + }); +}); + +// Keep the unused-handle type import honest for `typecheck`. +export type _HandleShape = CryptoHandle; diff --git a/packages/services/service-settings/src/sys-secret-orphan-report.ts b/packages/services/service-settings/src/sys-secret-orphan-report.ts new file mode 100644 index 0000000000..d966943d9e --- /dev/null +++ b/packages/services/service-settings/src/sys-secret-orphan-report.ts @@ -0,0 +1,364 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #8103 — **report-only** classification of `sys_secret` rows against the + * settings subsystem's references. Read-only by construction: nothing here + * writes, deletes, or decrypts. + * + * ## Why a report and not a sweep + * + * #8030 / PR #8063 made a settings rotation reap the ciphertext it retired, + * but the reaping is **forward-only** — it fires on the write that repoints a + * handle. Rows orphaned by rotations that already happened on a deployed + * instance are untouched by it. Removing those is a destructive, irreversible + * delete-many over stored credentials; the vehicle for it (migration / admin + * command / opt-in script) is a maintainer decision and is escalated + * separately. ⛔ This module deliberately contains no deletion, and must not + * grow one. + * + * ## The measurement that shapes this module's contract + * + * #8063's point-delete argues reachability from three facts. Re-measured on + * this checkout (see `sys-secret-orphan-report.test.ts`), **one of them is + * false**, and it is the one a sweep depends on most: + * + * - ✅ Handle ids are minted per `encrypt()` call — `LocalCryptoProvider` + * mints `sec_` + 16 random bytes inside `encrypt`, and `rotateKey` keeps + * `id` stable, so a handle names exactly one ciphertext lineage. + * - ❌ **`sys_setting.value_enc` is NOT the only column that holds a handle.** + * `sys_secret` has *three* privileged producers, as its own schema says + * (`platform-objects/src/system/sys-secret.object.ts`): `SettingsService`, + * the engine's `secret`-field encryption (`encryptSecretFields`, which + * stores `secret:` on an arbitrary business row), and the datasource + * credential binder (which stores `sys_secret:` at + * `external.credentialsRef`). Two of the three are invisible from this + * package, and the engine's set of holders is not even statically + * enumerable — it is every `secret`-typed field on every registered object, + * including tenant-authored ones. + * - ✅ The audit trail records digests, not handles (`old_hash` / `new_hash` + * are content digests; `SettingsService` passes the provider's `digest()`), + * so audit stays readable after a ciphertext is destroyed — and, the other + * way round, audit can never be used to reconstruct which handles existed. + * + * That falsified fact is why this module's verdict vocabulary has a third + * value. A row this package cannot ATTRIBUTE to the settings producer is + * reported {@link SecretRowVerdict `'unattributable'`}, never `'orphaned'` — + * "unreferenced by `sys_setting`" and "unreferenced" are different claims, and + * conflating them is what would delete a live datasource credential or a live + * business-row secret. The attribution guard is the safety property; it is + * pinned by test and ⛔ must not be relaxed into "everything unreferenced is an + * orphan". + * + * ## The two directional guards the card names + * + * - **Legacy inline crypto, in the other direction from #8063's reaper.** The + * pre-Phase-3 path stores the ciphertext ITSELF in `value_enc` rather than a + * `sec_` handle. #8063 guards its delete with a `sec_` prefix check; the + * guard needed *here* is that such a row contributes no handle to the + * referenced set and must never make a `sys_secret` row look collectable by + * implication. Rows whose `(namespace, key)` currently resolves through a + * legacy inline value are flagged {@link ClassifiedSecretRow.legacyInlineSibling} + * so an operator sees that the pair has been written through both paths. + * - **`rotateKey()` is not a retirement.** It re-wraps in place + * (`secretStore.update`) and keeps `id` stable, so a re-wrapped row is still + * the referenced one. Classification is therefore by REFERENCE only — + * `version` / `rotated_at` never contribute to a verdict, they are reported + * as {@link ClassifiedSecretRow.rewrapped} for the operator's eyes. A + * classifier that read "version > 1" as "was rotated, therefore retired" + * would delete the value in force; that inversion is pinned by test. + * + * ## Shape + * + * A pure function over snapshots the caller has already read. It takes no + * store, no engine and no provider, so it cannot write and cannot decrypt, and + * it adds nothing to any persistence port — the collection surface (how an + * operator obtains these snapshots, and through which command) is part of the + * escalated vehicle decision and is deliberately not settled here. + * + * ⛔ Snapshots must not carry `ciphertext`. The maintainer ruling of + * 2026-08-12 on this card is that report mode "names counts and row ids + * without decrypting", and {@link SecretRowSnapshot} is typed to make carrying + * the cipher material a type error rather than a review question. + */ + +import type { SettingsManifest } from '@objectstack/spec/system'; + +/** + * Prefix of a `sys_secret` handle id as minted by `ICryptoProvider.encrypt`. + * + * Must agree with the two literals `SettingsService` uses (the `sec_` guard in + * `reapRotatedSecret` and the dereference branch in `materialiseRow`). Pinned + * against the real minter rather than against those literals: the test mints a + * handle through `LocalCryptoProvider` and asserts {@link isSecretHandle} + * accepts it, so the prefix is checked against the code that actually produces + * ids, not against a copy of the same string. + */ +export const SECRET_HANDLE_PREFIX = 'sec_'; + +/** True when `value` is a `sys_secret` handle id rather than inline ciphertext. */ +export function isSecretHandle(value: unknown): value is string { + return typeof value === 'string' && value.startsWith(SECRET_HANDLE_PREFIX); +} + +/** + * A `sys_secret` row as the report sees it. + * + * ⛔ Deliberately has no `ciphertext` member. The report never decrypts and + * never needs the cipher material; leaving it off the type means a caller + * cannot accidentally pipe cleartext-recoverable bytes into a reporting + * surface that is expected to be safe to print. + */ +export interface SecretRowSnapshot { + /** `sys_secret.id` — the handle. */ + id: string; + /** `sys_secret.namespace`. Overloaded across producers; see `attributable`. */ + namespace: string; + /** `sys_secret.key`. */ + key: string; + /** Wrapping version. Reported, never a verdict input. */ + version?: number | null; + /** KMS key id. Reported, never a verdict input. */ + kms_key_id?: string | null; + created_at?: string | null; + /** Set when the row was re-wrapped in place. Never a verdict input. */ + rotated_at?: string | null; +} + +/** A `sys_setting` row as the report sees it. */ +export interface SettingRowSnapshot { + namespace: string; + key: string; + scope?: string | null; + user_id?: string | null; + /** Handle id (`sec_…`) or, on the legacy inline path, the ciphertext itself. */ + value_enc?: string | null; + encrypted?: boolean | null; +} + +/** A `(namespace, key)` pair the settings manifests declare as encrypted. */ +export interface EncryptedSpecifierRef { + namespace: string; + key: string; +} + +/** + * Verdict for one `sys_secret` row. + * + * - `in_force` — a live `sys_setting` row's `value_enc` names this handle. + * - `orphaned` — nothing in `sys_setting` names it AND it is attributable to + * the settings producer. The only class a settings-scoped sweep could ever + * consider, and even then only after the vehicle decision. + * - `unattributable` — nothing in `sys_setting` names it and this package + * cannot prove settings produced it. Belongs to (or may belong to) the + * engine's `secret`-field channel or the datasource binder. ⛔ NOT an + * orphan; a sweep must leave it alone. + */ +export type SecretRowVerdict = 'in_force' | 'orphaned' | 'unattributable'; + +/** One classified `sys_secret` row. */ +export interface ClassifiedSecretRow { + id: string; + namespace: string; + key: string; + verdict: SecretRowVerdict; + /** Human-readable justification, safe to print (no cipher material). */ + reason: string; + /** + * True when the row's `(namespace, key)` currently resolves through a LEGACY + * INLINE `sys_setting` value rather than a handle. The pair has been written + * through both storage paths; an operator should look before acting. + */ + legacyInlineSibling?: boolean; + /** + * True when the row carries re-wrap evidence (`version > 1` or `rotated_at`). + * Reported only — a re-wrap keeps the handle stable and is NOT a retirement. + */ + rewrapped?: boolean; +} + +/** The report. Counts and row ids only — never plaintext, never ciphertext. */ +export interface SysSecretOrphanReport { + counts: { + total: number; + inForce: number; + orphaned: number; + unattributable: number; + }; + rows: ClassifiedSecretRow[]; + /** + * `sys_setting` rows still on the legacy inline path (encrypted, but + * `value_enc` is not a handle). They reference no `sys_secret` row at all. + */ + legacyInlineRows: Array<{ + namespace: string; + key: string; + scope?: string | null; + user_id?: string | null; + }>; + /** + * What this report could NOT establish. Non-empty by design: an operator + * reading counts without these would over-trust the `orphaned` number. + */ + caveats: string[]; +} + +/** + * Collect the `(namespace, key)` pairs the given manifests declare + * `encrypted: true` — the settings producer's attribution set. + * + * Only these pairs can ever have been written to `sys_secret` BY + * `SettingsService`, because the service's encrypted branch is gated on the + * manifest's `encryptedKeys`. A `sys_secret` row outside this set was produced + * by someone else (or by a manifest this instance no longer registers — see the + * caveat the report emits). + */ +export function collectEncryptedSpecifierRefs( + manifests: readonly SettingsManifest[], +): EncryptedSpecifierRef[] { + const out: EncryptedSpecifierRef[] = []; + for (const manifest of manifests ?? []) { + const namespace = manifest?.namespace; + if (!namespace) continue; + for (const specifier of manifest.specifiers ?? []) { + if (specifier && (specifier as { encrypted?: unknown }).encrypted === true) { + out.push({ namespace, key: specifier.key }); + } + } + } + return out; +} + +const refKey = (namespace: string, key: string) => `${namespace}${key}`; + +/** + * Classify every `sys_secret` row against the settings subsystem's references. + * + * Pure and read-only. `attributableTo` is the set of `(namespace, key)` pairs + * the caller's REGISTERED manifests declare encrypted — pass + * {@link collectEncryptedSpecifierRefs} over the manifests the instance boots + * with. Passing an empty set is legal and yields zero orphans and an explicit + * caveat: with nothing to attribute against, no row can be proven to be + * settings' to collect, which is the safe direction. + */ +export function classifySysSecretRows(input: { + secrets: readonly SecretRowSnapshot[]; + settingRows: readonly SettingRowSnapshot[]; + attributableTo: readonly EncryptedSpecifierRef[]; +}): SysSecretOrphanReport { + const secrets = input.secrets ?? []; + const settingRows = input.settingRows ?? []; + const attributable = new Set( + (input.attributableTo ?? []).map((r) => refKey(r.namespace, r.key)), + ); + + // The referenced set: handles named by a live `sys_setting` row. A legacy + // inline value is NOT a handle and contributes nothing here — that is the + // `sec_` guard in the other direction from #8063's reaper. + const referenced = new Set(); + const legacyInlineRows: SysSecretOrphanReport['legacyInlineRows'] = []; + const legacyInlinePairs = new Set(); + + for (const row of settingRows) { + const enc = row?.value_enc; + if (typeof enc !== 'string' || enc === '') continue; + if (isSecretHandle(enc)) { + referenced.add(enc); + continue; + } + // Inline ciphertext (or a non-handle value on an encrypted row). + legacyInlineRows.push({ + namespace: row.namespace, + key: row.key, + scope: row.scope ?? null, + user_id: row.user_id ?? null, + }); + legacyInlinePairs.add(refKey(row.namespace, row.key)); + } + + const rows: ClassifiedSecretRow[] = []; + let inForce = 0; + let orphaned = 0; + let unattributable = 0; + + for (const secret of secrets) { + // Re-wrap evidence is REPORTED, never a verdict input: `rotateKey` keeps + // the handle stable, so a re-wrapped row is the row still in force. + const rewrapped = + (typeof secret.version === 'number' && secret.version > 1) || + (typeof secret.rotated_at === 'string' && secret.rotated_at !== ''); + const legacyInlineSibling = legacyInlinePairs.has(refKey(secret.namespace, secret.key)); + + let verdict: SecretRowVerdict; + let reason: string; + + if (referenced.has(secret.id)) { + verdict = 'in_force'; + reason = rewrapped + ? 'referenced by a live sys_setting.value_enc; re-wrapped in place (handle stable) — not a retirement' + : 'referenced by a live sys_setting.value_enc'; + inForce += 1; + } else if (attributable.has(refKey(secret.namespace, secret.key))) { + verdict = 'orphaned'; + reason = legacyInlineSibling + ? 'no sys_setting row names this handle; attributable to a declared encrypted specifier, ' + + 'but that (namespace, key) currently resolves through a LEGACY INLINE value — inspect before acting' + : 'no sys_setting row names this handle, and (namespace, key) matches a declared encrypted specifier'; + orphaned += 1; + } else { + verdict = 'unattributable'; + reason = + 'no sys_setting row names this handle AND (namespace, key) matches no declared encrypted ' + + 'specifier — may belong to the engine secret-field channel or the datasource credential ' + + 'binder, which this report cannot see. NOT an orphan.'; + unattributable += 1; + } + + rows.push({ + id: secret.id, + namespace: secret.namespace, + key: secret.key, + verdict, + reason, + ...(legacyInlineSibling ? { legacyInlineSibling: true } : {}), + ...(rewrapped ? { rewrapped: true } : {}), + }); + } + + const caveats: string[] = [ + 'Settings-scoped only. `sys_secret` has three producers (SettingsService, the engine ' + + 'secret-field channel storing `secret:` on arbitrary business rows, and the datasource ' + + 'credential binder storing `sys_secret:` at `external.credentialsRef`). This report reads ' + + 'only `sys_setting`, so a row it calls `unattributable` may be live elsewhere.', + 'The engine secret-field holders are not statically enumerable — they are every `secret`-typed ' + + 'field on every registered object, including tenant-authored ones. A complete reference set ' + + 'cannot be built from this package.', + '`sys_secret` carries no producer column, and `namespace`/`key` mean different things per ' + + 'producer (settings namespace/specifier key; object name/field name; `datasource`/datasource ' + + 'name). Attribution by (namespace, key) is therefore a HEURISTIC: an object named like a ' + + 'settings namespace with a field named like a specifier key would be attributed to settings.', + ]; + if (attributable.size === 0) { + caveats.push( + 'No encrypted specifiers were supplied, so nothing could be attributed to the settings ' + + 'producer and no row is reported as orphaned. Pass the registered manifests.', + ); + } + if (legacyInlineRows.length > 0) { + caveats.push( + `${legacyInlineRows.length} sys_setting row(s) still hold inline ciphertext rather than a ` + + 'handle. They reference no sys_secret row; rows sharing their (namespace, key) are flagged ' + + '`legacyInlineSibling`.', + ); + } + caveats.push( + 'A manifest that is no longer registered makes its previously-encrypted keys unattributable, ' + + 'so genuine settings orphans can be under-reported. Under-reporting is the safe direction.', + ); + + return { + counts: { total: rows.length, inForce, orphaned, unattributable }, + rows, + legacyInlineRows, + caveats, + }; +} From 36b5256ce49f9d8e14ac7081b3d4a26a0d5ea781 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 01:18:09 +0000 Subject: [PATCH 2/3] fix(service-settings): write the refKey NUL separator as an escape, not a raw byte check:nul-bytes caught a raw 0x00 in the composite-key separator -- the exact accident the gate documents (an editing tool materialising an escape while the author writes about it). The backslash-u-0000 spelling is byte-identical at run time. Also documents why NUL rather than a printable separator: no namespace or specifier key can contain one, so no two distinct pairs alias into one composite -- and an aliased pair here would silently change a row's verdict. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ARidKDYSCD56LaygrvDPnk --- .../src/sys-secret-orphan-report.ts | 24 +++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/packages/services/service-settings/src/sys-secret-orphan-report.ts b/packages/services/service-settings/src/sys-secret-orphan-report.ts index d966943d9e..1418ec9ecf 100644 --- a/packages/services/service-settings/src/sys-secret-orphan-report.ts +++ b/packages/services/service-settings/src/sys-secret-orphan-report.ts @@ -220,15 +220,31 @@ export function collectEncryptedSpecifierRefs( const namespace = manifest?.namespace; if (!namespace) continue; for (const specifier of manifest.specifiers ?? []) { - if (specifier && (specifier as { encrypted?: unknown }).encrypted === true) { - out.push({ namespace, key: specifier.key }); - } + if (!specifier || (specifier as { encrypted?: unknown }).encrypted !== true) continue; + // `key` is optional on the spec type (layout-only specifiers such as + // section headers carry none). A keyless specifier addresses no stored + // value, so it can never have produced a `sys_secret` row — skipping it + // keeps the attribution set to pairs that could really exist. + const key = specifier.key; + if (typeof key !== 'string' || key === '') continue; + out.push({ namespace, key }); } } return out; } -const refKey = (namespace: string, key: string) => `${namespace}${key}`; +/** + * Composite map key for a `(namespace, key)` pair. + * + * The separator is a NUL, written as the ESCAPE `\u0000` and never as the raw + * byte (`scripts/check-nul-bytes.mjs`; the same convention as `rest-server.ts`) + * -- byte-identical at run time. NUL rather than a printable separator because + * neither a settings namespace nor a specifier key can contain one, so no two + * distinct pairs can collide into one composite. A `.` or `/` separator would + * let ('a.b','c') and ('a','b.c') alias, and an aliased pair here silently + * changes a row's VERDICT. + */ +const refKey = (namespace: string, key: string) => `${namespace}\u0000${key}`; /** * Classify every `sys_secret` row against the settings subsystem's references. From 7cabe75260f46a13d97ab340457ff94638095516 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 01:50:45 +0000 Subject: [PATCH 3/3] test(service-settings): route the context-dropping engine double through assertEngineUpdateDispatch check:engine-double-contract (convention-scoped, so dispatch-gates.mjs cannot derive it) flagged the #8103 harness's SettingsEngine double as declaring an update() that does not route through the shared dispatch predicate. Took the pin rather than a baseline entry: the predicate reads only where / multi / data.id and never looks at context, so it is orthogonal to the defect this double reproduces (dropping context so the engine strips the read-only value_enc). The double stays loose in exactly one dimension and is now conformant in the one that drifts silently -- by-id vs multi. multi:true is passed unconditionally instead of re-deciding the branch, because that IS the settings adapter's contract: a scalar where.id outranks multi in the shared predicate, so one call reproduces both of wrapEngineAsSettingsEngine's branches without mirroring the guard. Imported from @objectstack/objectql (already a devDependency, already aliased to source by vitest.config.ts) rather than @objectstack/metadata-core: the cycle rule prefers metadata-core only when objectql depends on the package being pinned, and objectql does not depend on service-settings. No verdict logic changed; settings-service.ts and crypto-adapter.ts untouched. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ARidKDYSCD56LaygrvDPnk --- .../src/sys-secret-orphan-report.test.ts | 26 ++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/packages/services/service-settings/src/sys-secret-orphan-report.test.ts b/packages/services/service-settings/src/sys-secret-orphan-report.test.ts index b97866c0a0..4635c7334c 100644 --- a/packages/services/service-settings/src/sys-secret-orphan-report.test.ts +++ b/packages/services/service-settings/src/sys-secret-orphan-report.test.ts @@ -35,7 +35,14 @@ */ import { describe, expect, it } from 'vitest'; -import { ObjectQL } from '@objectstack/objectql'; +// `assertEngineUpdateDispatch` comes from `@objectstack/objectql` (which +// re-exports the predicate that lives in `@objectstack/metadata-core` since +// #5619) rather than from metadata-core directly: the cycle rule prefers +// metadata-core only when objectql DEPENDS ON the package being pinned, and +// objectql does not depend on `service-settings`. objectql is already a +// devDependency here and `vitest.config.ts` already aliases it to SOURCE, so +// this adds no dependency and no new alias. +import { assertEngineUpdateDispatch, ObjectQL } from '@objectstack/objectql'; import { SysSecret, SysSetting } from '@objectstack/platform-objects/system'; import type { SettingsManifest } from '@objectstack/spec/system'; import type { CryptoHandle, ICryptoProvider } from '@objectstack/spec/contracts'; @@ -191,6 +198,23 @@ function wrapEngineDroppingContext(engine: any): SettingsEngine { find: real.find.bind(real), insert: real.insert.bind(real), async update(objectName, opts) { + // This double is loose in exactly ONE dimension — `context` — and must + // stay conformant in every other, or the orphan counts it produces stop + // being evidence about the real engine. `assertEngineUpdateDispatch` + // pins the dimension that is easiest to drift silently: by-id vs multi. + // + // `multi: true` is passed unconditionally rather than re-deciding the + // branch here, because that IS the settings adapter's contract — a + // scalar `where.id` outranks `multi` in the shared predicate, so this one + // call reproduces both of `wrapEngineAsSettingsEngine`'s branches + // (by-id when `where.id` is scalar, multi otherwise) without mirroring + // the guard. The predicate reads only `where` / `multi` / `data.id` and + // never looks at `context`, so it is orthogonal to the defect being + // reproduced. + assertEngineUpdateDispatch((opts as any)?.data ?? {}, { + where: (opts as any)?.where, + multi: true, + }); const { context: _dropped, ...withoutContext } = opts as any; return real.update(objectName, withoutContext); },