From 4ddd509ae0c937c28ce023285dcf78752aecf7b8 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 28 Aug 2026 05:50:43 +0000 Subject: [PATCH] fix(cli): ask the engine for family 3 of the sys_secret reference union (#12804) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cross-producer reference union's family 3 was pure over the artefacts its caller supplied. #12758 landed the producer half — registerDatasourceDef retains external.credentialsRef and ObjectQL.listDatasourceDefs() reads it back — so the engine can answer; the union never asked. Measured before the change: a code-registered datasource with a bound handle plus declaredDatasources: [] produced a union reporting complete: true while omitting that live handle. Family 3 now assembles from three sources — persisted sys_metadata rows, the definitions the engine holds, and the host's declared list — as a union, not a replacement, because neither code-side source dominates the other. The declared gap is re-scoped, not removed: declaredDatasources: undefined still refuses the whole union (a datasource declared in code and never registered reaches neither source), and an engine slice that cannot list its definitions now gaps the family rather than contributing an empty answer. listDatasourceDefs is an OPTIONAL port member, so every slice that satisfied SecretReferenceEngineLike before still does. The three prose sites #12758 falsified are rewritten, not trimmed: the retired mechanism was "the engine drops credentialsRef"; the live one is "the engine's index covers only what was registered, so the residue is invisible until the host is asked". A test pins that the operator-facing gap message carries the new mechanism and not the old. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UjujZN219uFzBhSYfMykCd --- .../secret-reference-union-asks-the-engine.md | 43 ++++ .../src/utils/secret-reference-union.test.ts | 197 ++++++++++++++++++ .../cli/src/utils/secret-reference-union.ts | 127 +++++++++-- 3 files changed, 347 insertions(+), 20 deletions(-) create mode 100644 .changeset/secret-reference-union-asks-the-engine.md diff --git a/.changeset/secret-reference-union-asks-the-engine.md b/.changeset/secret-reference-union-asks-the-engine.md new file mode 100644 index 0000000000..3db0c06e4a --- /dev/null +++ b/.changeset/secret-reference-union-asks-the-engine.md @@ -0,0 +1,43 @@ +--- +"@objectstack/cli": patch +--- + +fix(cli): the `sys_secret` reference union asks the engine for family 3 instead of trusting every host to remember (#12804) + +Family 3 of the cross-producer reference union — handles held at a datasource +artefact's `external.credentialsRef` — was pure over the artefacts its caller +supplied. `#12758` landed the producer half (`registerDatasourceDef` retains +`external.credentialsRef`, `ObjectQL.listDatasourceDefs()` reads it back), so +the engine could answer the question; the union never asked it. Measured on the +pre-change tree: a datasource registered in code with a bound credentials +handle, with `declaredDatasources: []`, produced a union reporting +`complete: true` while omitting that live handle. A complete-looking union that +is short one live credential is the precondition failure `#8103`'s deletion +predicate rests on. + +The union now assembles family 3 from **three** sources — persisted +`sys_metadata` rows, the definitions the engine holds, and the host's declared +list — as a union, not a replacement. Neither code-side source dominates: the +engine indexes only what was REGISTERED on the runtime, so a config file +nothing ever installed is invisible to it, while a host's list can omit a +datasource a package manifest installed behind its back. + +The declared gap is **re-scoped, not removed**. `declaredDatasources: +undefined` still refuses the whole union, because the residue it covers is +still unreachable: a datasource declared in code that nothing ever registered +reaches neither `sys_metadata` nor `listDatasourceDefs()`. A second refusing +shape joins it — an engine slice that cannot list its definitions gaps the +family rather than contributing an empty answer, symmetric with the host's +`undefined`. In both cases `[]` remains the way to state "there are none". + +`SecretReferenceEngineLike` gains `listDatasourceDefs?()` as an **optional** +member, so every slice that satisfied the port before still satisfies it. The +three prose sites that `#12758` falsified are rewritten rather than trimmed: +the retired mechanism was "the engine drops `credentialsRef`", and the live one +is "the engine's index covers only what was registered, so the residue is +invisible until the host is asked". The operator-facing gap message carries the +new mechanism, and a test pins that it does not carry the old one. + +Bump kept at `patch`, matching `#12663` which created the module: nothing here +reaches the package's entry barrel — `packages/cli/src/index.ts` names no +symbol of this module, and no consumer outside `@objectstack/cli` imports it. diff --git a/packages/cli/src/utils/secret-reference-union.test.ts b/packages/cli/src/utils/secret-reference-union.test.ts index e628d95a42..a368f803c1 100644 --- a/packages/cli/src/utils/secret-reference-union.test.ts +++ b/packages/cli/src/utils/secret-reference-union.test.ts @@ -20,6 +20,16 @@ * asserts a handle that ONLY that family holds. A family whose removal left * everything green would be a family these tests do not cover. * + * **#12804 — family 3 now has TWO sources, so it needs TWO ablations.** The + * union asks the engine (`listDatasourceDefs()`) as well as the host, and + * neither source dominates: the engine indexes only what was REGISTERED on the + * runtime, while the host's list is the only channel for a datasource declared + * in code that nothing ever installed. Each half therefore carries a named pin + * asserting a handle that ONLY that half can reach — + * `family 3 (engine half)` and `family 3 (host half)`. Ablating one half must + * red its own pin ALONE; if ablating one leaves everything green, the other is + * covering for it and the union's two branches were never reached. + * * Family 2 carries an extra pin, because it is the one family that cannot be * precomputed: its holders are every `secret`-typed field on every REGISTERED * object, tenant-authored ones included. `registers a new secret field at @@ -47,6 +57,7 @@ import { collectSecretReferenceUnion, collectSettingsSecretReferences, IncompleteSecretReferenceUnionError, + readEngineDatasourceDefs, type SecretReferenceEngineLike, } from './secret-reference-union.js'; @@ -476,6 +487,10 @@ describe('family 3 — datasource artefacts (`external.credentialsRef`)', () => expect(undeclared.gaps[0].reason).toContain('declaredDatasources'); // The partial references survive — they are real, they just cannot complete. expect(undeclared.handleIds.has(rt.datasourceHandleId)).toBe(true); + // #12804: the engine half answered, so ONLY the host half is missing — one + // gap reason, not two. (Its wording is pinned separately, in the host-half + // suite below.) + expect(undeclared.gaps).toHaveLength(1); const declaredEmpty = await collect(rt, []); expect(declaredEmpty.complete).toBe(true); @@ -491,6 +506,188 @@ describe('family 3 — datasource artefacts (`external.credentialsRef`)', () => }); }); +// --------------------------------------------------------------------------- +// #12804 — family 3's SECOND source. Two halves, two ablations. +// --------------------------------------------------------------------------- + +/** + * Mint a credentials handle through the REAL binder, so the ref spelling under + * test comes from the producer rather than from this file. + */ +async function bindCredential(rt: Runtime, name: string) { + const binder = createDatasourceSecretBinder({ + engine: rt.realEngine as never, + cryptoProvider: rt.crypto as never, + }); + const ref = await binder.bind({ value: `${name}-password` }, { name }); + return { ref, handleId: ref.slice('sys_secret:'.length) }; +} + +/** An engine slice built by hand, so a port member can be removed or broken. */ +const sliceOf = ( + rt: Runtime, + listDatasourceDefs?: SecretReferenceEngineLike['listDatasourceDefs'], +): SecretReferenceEngineLike => ({ + getConfigs: () => rt.engine.getConfigs(), + getDriverForObject: (o) => rt.engine.getDriverForObject(o), + ...(listDatasourceDefs ? { listDatasourceDefs } : {}), +}); + +describe('family 3 (engine half) — definitions the engine holds', () => { + let rt: Runtime; + beforeEach(async () => { rt = await buildRuntime(); }); + + it('names a handle held ONLY by an engine-registered datasource definition', async () => { + const { ref, handleId } = await bindCredential(rt, 'analytics'); + // Registered IN CODE only: never written to sys_metadata, never declared by + // the host. Before #12804 this handle was invisible to the union. + rt.realEngine.registerDatasourceDef({ + name: 'analytics', schemaMode: 'external', external: { allowWrites: false, credentialsRef: ref }, + }); + + // Anti-vacuity: neither other source can reach it. + expect(rt.store.rowsOf('sys_metadata').some((r) => String(r.metadata).includes(handleId))).toBe(false); + + const union = await collect(rt, []); // host says it has NONE + assertSecretReferenceUnionComplete(union); + expect(union.handleIds.has(handleId)).toBe(true); + const ref3 = union.references.find((r) => r.handleId === handleId); + expect(ref3?.family).toBe('datasource'); + expect(ref3?.holder).toBe('datasource(analytics).external.credentialsRef'); + expect(union.references.filter((r) => r.handleId === handleId)).toHaveLength(1); + }); + + it('an engine that cannot list its definitions is a GAP, never an empty answer', async () => { + const noAccessor = sliceOf(rt); + const read = readEngineDatasourceDefs(noAccessor); + expect(read.artefacts).toEqual([]); + expect(read.gap).toContain('listDatasourceDefs'); + + const union = await collectSecretReferenceUnion({ engine: noAccessor, declaredDatasources: [] }); + expect(union.complete).toBe(false); + expect(union.gaps.map((g) => g.family)).toEqual(['datasource']); + // The persisted half still enumerated, so its handle survives as a partial. + expect(union.handleIds.has(rt.datasourceHandleId)).toBe(true); + }); + + it('a throwing listDatasourceDefs is a GAP naming the cause', async () => { + const throwing = sliceOf(rt, () => { throw new Error('definition index unavailable'); }); + const read = readEngineDatasourceDefs(throwing); + expect(read.gap).toContain('definition index unavailable'); + + const union = await collectSecretReferenceUnion({ engine: throwing, declaredDatasources: [] }); + expect(union.complete).toBe(false); + expect(union.gaps[0].reason).toContain('definition index unavailable'); + }); + + it('an engine answering [] is an ANSWER, exactly as the host\'s [] is', async () => { + const empty = sliceOf(rt, () => []); + const union = await collectSecretReferenceUnion({ engine: empty, declaredDatasources: [] }); + expect(union.complete).toBe(true); + }); +}); + +describe('family 3 (host half) — artefacts the engine never saw', () => { + let rt: Runtime; + beforeEach(async () => { rt = await buildRuntime(); }); + + it('names a handle held ONLY by the host-declared list', async () => { + const { ref, handleId } = await bindCredential(rt, 'never_installed'); + // Declared in a config file nothing ever registered: the engine's index + // cannot see it, and it never reached sys_metadata either. + expect(rt.realEngine.listDatasourceDefs().some((d) => d.name === 'never_installed')).toBe(false); + expect(rt.store.rowsOf('sys_metadata').some((r) => String(r.metadata).includes(handleId))).toBe(false); + + const union = await collect(rt, [{ name: 'never_installed', external: { credentialsRef: ref } }]); + assertSecretReferenceUnionComplete(union); + expect(union.handleIds.has(handleId)).toBe(true); + const ref3 = union.references.find((r) => r.handleId === handleId); + expect(ref3?.family).toBe('datasource'); + expect(ref3?.holder).toBe('datasource(never_installed).external.credentialsRef'); + expect(union.references.filter((r) => r.handleId === handleId)).toHaveLength(1); + }); + + it('the host half still REFUSES when nobody answered — the guarantee #12804 must not remove', async () => { + // The falsification criterion: an input shape that makes the union refuse + // rather than return a silent empty answer must still exist after wiring + // the engine in. `declaredDatasources: undefined` is that shape. + const undeclared = await collectSecretReferenceUnion({ + engine: rt.engine, + declaredDatasources: undefined, + }); + expect(undeclared.complete).toBe(false); + expect(undeclared.gaps.map((g) => g.family)).toEqual(['datasource']); + expect(() => assertSecretReferenceUnionComplete(undeclared)) + .toThrow(IncompleteSecretReferenceUnionError); + }); + + it('the gap message states the LIVE mechanism, not the retired one', async () => { + const undeclared = await collectSecretReferenceUnion({ + engine: rt.engine, + declaredDatasources: undefined, + }); + const reason = undeclared.gaps[0].reason; + // The reason an operator reads mid-incident. The engine DID answer; what is + // still unreachable is a datasource declared in code and never registered. + expect(reason).toContain('declaredDatasources'); + expect(reason).toContain('REGISTERED on this runtime'); + expect(reason).toContain('until the host is asked'); + // …and it must not carry the mechanism #12758 retired. A true sentence + // resting on a dead mechanism is the defect class this pin exists for. + expect(reason).not.toContain('engine drops'); + expect(reason).not.toContain('could not be seen at all'); + }); +}); + +describe('family 3 — the two sources are a UNION, not a replacement', () => { + let rt: Runtime; + beforeEach(async () => { rt = await buildRuntime(); }); + + it('one datasource named by BOTH sources contributes ONE reference, not two', async () => { + const { ref, handleId } = await bindCredential(rt, 'shared'); + rt.realEngine.registerDatasourceDef({ name: 'shared', external: { credentialsRef: ref } }); + + const union = await collect(rt, [{ name: 'shared', external: { credentialsRef: ref } }]); + assertSecretReferenceUnionComplete(union); + expect(union.references.filter((r) => r.handleId === handleId)).toHaveLength(1); + }); + + it('two sources DISAGREEING keeps both handles — dropping either would under-report', async () => { + const fromEngine = await bindCredential(rt, 'drifted'); + const fromHost = await bindCredential(rt, 'drifted'); + expect(fromEngine.handleId).not.toBe(fromHost.handleId); + rt.realEngine.registerDatasourceDef({ name: 'drifted', external: { credentialsRef: fromEngine.ref } }); + + const union = await collect(rt, [{ name: 'drifted', external: { credentialsRef: fromHost.ref } }]); + assertSecretReferenceUnionComplete(union); + expect(union.handleIds.has(fromEngine.handleId)).toBe(true); + expect(union.handleIds.has(fromHost.handleId)).toBe(true); + }); + + it('a handle held ONLY in sys_metadata still arrives — the persisted source is untouched', async () => { + const union = await collect(rt, []); + assertSecretReferenceUnionComplete(union); + expect(union.handleIds.has(rt.datasourceHandleId)).toBe(true); + }); +}); + +/** + * Type-level pin, evaluated by `tsc --noEmit`: `packages/cli/tsconfig.json` + * includes `src` with NO test exclusion (unlike `tsconfig.build.json`), so a + * type assertion written here IS in the typecheck program — verified with + * `tsc --listFiles`. + * + * What it pins: the REAL engine's answer fits the port's declared return type. + * Taken off the METHOD so re-narrowing `ObjectQL.listDatasourceDefs` moves the + * pin even if the named types survive. + */ +type EngineDefsPort = NonNullable; +export function __pinRealEngineSatisfiesTheDatasourcePort( + engine: ObjectQL, +): ReturnType { + return engine.listDatasourceDefs(); +} + describe('completeness is the contract', () => { let rt: Runtime; beforeEach(async () => { rt = await buildRuntime(); }); diff --git a/packages/cli/src/utils/secret-reference-union.ts b/packages/cli/src/utils/secret-reference-union.ts index f18697ad52..150c41fb66 100644 --- a/packages/cli/src/utils/secret-reference-union.ts +++ b/packages/cli/src/utils/secret-reference-union.ts @@ -48,8 +48,9 @@ * rather than a union that is silently one producer short. * - **A read that did not happen is a GAP, never an empty answer.** Every * collector returns {@link FamilyGap} when it could not enumerate — a - * missing driver, an unregistered holder object, a throwing read, a host - * that did not declare its code-defined datasources. A gap makes the union + * missing driver, an unregistered holder object, a throwing read, an engine + * that cannot list the datasource definitions it holds, a host that did not + * declare its code-defined datasources. A gap makes the union * `complete: false`, and {@link assertSecretReferenceUnionComplete} refuses * it. Returning `[]` there would be the read-invention defect AGENTS.md * names, with a credential delete on the other end of it. @@ -266,6 +267,22 @@ export interface SecretReferenceEngineLike { getConfigs(): Record; /** The driver serving an object, or `undefined` when none resolves. */ getDriverForObject(objectName: string): SecretReferenceDriverLike | undefined; + /** + * Every datasource DEFINITION the engine holds — family 3's SECOND source, + * covering the artefacts that never reach `sys_metadata`. + * + * OPTIONAL, and that is a WIDENING of this port rather than a narrowing: + * every slice that satisfied this interface before still satisfies it, and + * the real `ObjectQL` satisfies the new member structurally (pinned in this + * module's test file). + * + * ⛔ Absence is NOT "the engine holds none". A slice without the accessor + * gaps the family — see {@link readEngineDatasourceDefs} — for the same + * reason `declaredDatasources: undefined` does: a read that did not happen + * cannot be reported as an empty answer. A slice that genuinely holds none + * says so exactly the way the host does, by answering `[]`. + */ + listDatasourceDefs?(): readonly DatasourceArtefactLike[]; } /** A datasource artefact, as far as this module reads it. */ @@ -423,28 +440,50 @@ export async function collectObjectFieldSecretReferences( /** * Family 3 — handles held at a datasource artefact's `external.credentialsRef`. * - * Pure over the artefacts the caller supplies, because the engine cannot answer - * this one: `registerDatasourceDef` keeps only `schemaMode` and - * `external.allowWrites`, dropping `credentialsRef` on the way in. Artefacts - * come from the metadata channel instead — {@link readStoredDatasourceArtefacts} - * for the persisted ones, plus whatever the host declared in code. + * Pure over the artefacts its caller supplies — but the caller now assembles + * those from THREE sources, and none of the three dominates the others: + * + * - {@link readStoredDatasourceArtefacts} — the persisted `sys_metadata` rows; + * - {@link readEngineDatasourceDefs} — the definitions this engine holds. + * `registerDatasourceDef` retains `external.credentialsRef` and + * `listDatasourceDefs()` reads it back, so the engine can now answer for + * every datasource REGISTERED on this runtime, by either entry route (the + * direct call and the package-manifest install path); + * - `SecretReferenceUnionInput.declaredDatasources` — the host's own list. + * + * A union, ⛔ not a replacement, because the two code-side sources have + * different blind spots: the engine indexes only what was registered on it, so + * a config file nothing ever installed is invisible to it, while a host's list + * can omit a datasource a package manifest installed behind its back. Letting + * either source stand for the other would under-report, and under-reporting is + * the direction that deletes live credentials. + * + * References are de-duplicated on the EXACT `(handleId, holder)` pair, which + * is information-preserving: the same handle at the same holder coordinate, + * seen twice because two sources both name it, is one reference. Two sources + * disagreeing — one holder, two different handles — keeps BOTH, because both + * are real and dropping either is the under-report this module refuses. */ export function collectDatasourceSecretReferences( artefacts: readonly DatasourceArtefactLike[], ): FamilyResult { const family: SecretReferenceFamily = 'datasource'; const references: SecretReference[] = []; + // Keyed on the JSON of the exact pair rather than a joined string: a + // separator character would have to be one no holder coordinate can contain, + // and that is a property this module cannot enforce over datasource names. + const seen = new Set(); for (const artefact of artefacts) { const ref = artefact?.external?.credentialsRef; if (typeof ref !== 'string' || ref === '') continue; const handleId = parseCredentialsRef(ref); if (handleId === undefined) continue; // a ref shape this producer did not mint - references.push({ - handleId, - family, - holder: `datasource(${String(artefact.name)}).external.credentialsRef`, - }); + const holder = `datasource(${String(artefact.name)}).external.credentialsRef`; + const key = JSON.stringify([handleId, holder]); + if (seen.has(key)) continue; // the same reference, reached through two sources + seen.add(key); + references.push({ handleId, family, holder }); } return { family, status: 'enumerated', references }; @@ -499,6 +538,45 @@ export async function readStoredDatasourceArtefacts( return { artefacts }; } +/** + * Read the datasource definitions THIS ENGINE HOLDS — family 3's code-side + * source, and the half of it the union used to be unable to reach. + * + * `ObjectQL.registerDatasourceDef` retains `external.credentialsRef`, and + * `listDatasourceDefs()` answers every definition the engine indexed, from both + * entry routes and UNFILTERED by `schemaMode` (a managed datasource may carry a + * `credentialsRef` too, so filtering here would hide live handles). + * + * ⛔ The accessor's ABSENCE is a gap, never an empty answer. An engine slice + * that cannot list its definitions has not answered the question, and a + * code-declared datasource never reaches `sys_metadata` — so nothing else in + * this module would see the handle it holds. A slice that holds none states + * that by answering `[]`, exactly as the host does with `declaredDatasources`. + */ +export function readEngineDatasourceDefs( + engine: SecretReferenceEngineLike, +): { artefacts: DatasourceArtefactLike[]; gap?: string } { + if (typeof engine.listDatasourceDefs !== 'function') { + return { + artefacts: [], + gap: + 'this runtime\'s engine exposes no `listDatasourceDefs()`, so the datasource definitions ' + + 'held in code could not be read; a code-declared datasource never reaches `sys_metadata`, ' + + 'so its `external.credentialsRef` is invisible to the persisted read — implement the ' + + 'accessor, or have it answer `[]` to state the engine holds none', + }; + } + + try { + return { artefacts: [...engine.listDatasourceDefs()] }; + } catch (err) { + return { + artefacts: [], + gap: `listing the engine's datasource definitions threw — ${describeCause(err)}`, + }; + } +} + /** Input to {@link collectSecretReferenceUnion}. */ export interface SecretReferenceUnionInput { /** A booted runtime's ObjectQL engine. */ @@ -507,12 +585,16 @@ export interface SecretReferenceUnionInput { * The datasource artefacts the HOST declared in code (`defineStack`, an app * manifest, a config file) — the ones that never reach `sys_metadata`. * - * **Required, and `undefined` is not the same as `[]`.** The engine drops - * `credentialsRef` from the definitions it keeps, so this module cannot - * discover code-defined artefacts and will not pretend to: `undefined` says - * "nobody answered", which opens a declared gap, while `[]` is the host - * stating it has none. Collapsing the two would be exactly the silent - * incompleteness this union exists to refuse. + * **Required, and `undefined` is not the same as `[]`.** The union now ASKS + * the engine as well ({@link readEngineDatasourceDefs}), so the code-side + * blind spot is narrower than it was — but it has not closed. The engine + * indexes only what was REGISTERED on this runtime, so a datasource declared + * in code that nothing ever installed reaches neither `sys_metadata` nor + * `listDatasourceDefs()`. This module cannot discover THAT residue until it + * asks the host, and it will not pretend to: `undefined` still says "nobody + * answered", which opens a declared gap, while `[]` is the host stating it + * has none. Collapsing the two would be exactly the silent incompleteness + * this union exists to refuse. */ declaredDatasources: readonly DatasourceArtefactLike[] | undefined; } @@ -533,18 +615,23 @@ export async function collectSecretReferenceUnion( const objectField = await collectObjectFieldSecretReferences(engine); const stored = await readStoredDatasourceArtefacts(engine); + const held = readEngineDatasourceDefs(engine); const datasource = collectDatasourceSecretReferences([ ...stored.artefacts, + ...held.artefacts, ...(declaredDatasources ?? []), ]); const datasourceGaps: string[] = []; if (stored.gap) datasourceGaps.push(stored.gap); + if (held.gap) datasourceGaps.push(held.gap); if (declaredDatasources === undefined) { datasourceGaps.push( 'the host did not declare its code-defined datasource artefacts (`declaredDatasources` was ' - + 'undefined), and the engine drops `external.credentialsRef` from the definitions it keeps, ' - + 'so a code-defined credential could not be seen at all — pass `[]` to state there are none', + + 'undefined). The engine\'s own definitions WERE read, but an engine indexes only the ' + + 'datasources REGISTERED on this runtime — a datasource declared in code that nothing ever ' + + 'installed reaches neither `sys_metadata` nor `listDatasourceDefs()`, so this union cannot ' + + 'discover it until the host is asked — pass `[]` to state there are none', ); }