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', ); }