Skip to content

Commit 7ad3d4e

Browse files
os-trumpclaude
andauthored
refactor(cli): drop the dead { records } normalizer from os secret orphans (#15093)
`os secret orphans` wrapped both of its driver reads in a local `rowsOf()` that unwrapped `{ data: [...] }`, lifted a bare row into `[row]` and filtered out non-object entries. Every limb was unreachable. The card that asked for this refused to let unreachability be inferred from `IDataDriver.find`'s declaration, because the counter-case is real: the console's `ObjectStackAdapter.find()` resolves to a normalized `QueryResult` envelope and never to an array. So the concrete driver was read instead. `secretDriver` resolves through `ObjectQL.getDriverForObject('sys_secret')`, which hands back a registered driver instance unwrapped; the five `IDataDriver` implementations in this tree — `SqlDriver` (and `SqliteWasmDriver`, which extends it without overriding `find`), `TursoDriver` local and remote, `MongoDBDriver`, `InMemoryDriver` — return an array on every path they can return on, `[]` included. Driven for real, the CLI's own boot resolves `com.objectstack.driver.sql` for both objects and both reads answer a bare array holding the seeded rows. `SecretReferenceDriverLike.find` is narrowed from `Promise<unknown>` to `Promise<Record<string, unknown>[]>` in the same change. That port is where the normalizer came from: its own doc comment already said it matched `IDataDriver.find`, and the return type said otherwise, so every caller that could not see an array in the type wrote its own answer for shapes no producer emits. The port now states the contract it claimed to state. The union's three reads keep their explicit `let result: unknown` locals and are untouched. Behaviour is unchanged for any driver that keeps the contract. One that does not now fails loudly rather than having a row silently dropped — and a `sys_secret` row dropped from this read is a row dropped from the report, which this command exists to prevent. Claude-Session: https://claude.ai/code/session_016yfqQh2dBgPAymYd7xipza Co-authored-by: Claude <noreply@anthropic.com>
1 parent 0f94cc7 commit 7ad3d4e

4 files changed

Lines changed: 358 additions & 15 deletions

File tree

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
---
2+
"@objectstack/cli": patch
3+
---
4+
5+
refactor(cli): `os secret orphans` reads its drivers as the arrays they return, and the union's driver port says so
6+
7+
`os secret orphans` wrapped both of its driver reads in a local `rowsOf()` that
8+
unwrapped `{ data: [...] }`, lifted a bare row into `[row]`, and filtered out
9+
non-object entries. None of those limbs was reachable. Every concrete driver
10+
that can sit behind `ObjectQL.getDriverForObject()` resolves `find` to an array
11+
on every path it can return on, `[]` included — `SqlDriver` (and
12+
`SqliteWasmDriver`, which extends it without overriding `find`), `TursoDriver`
13+
in both its local and remote faces, `MongoDBDriver` and `InMemoryDriver`, which
14+
are every `IDataDriver` implementation in this tree — and `registerDriver` /
15+
`getDriver` hand the registered instance back unwrapped, so nothing interposes
16+
another shape.
17+
18+
The reason the limb existed is the second half of this change.
19+
`SecretReferenceDriverLike`, the read-only driver port the command borrows from
20+
`secret-reference-union.ts`, declared `find` as `Promise<unknown>` while the
21+
sentence directly above it said it matched `IDataDriver.find` — which declares
22+
`Promise<Record<string, unknown>[]>`. The declaration and its own comment
23+
disagreed, and a caller that cannot see an array in the type writes a
24+
normalizer for envelope shapes no producer emits. The port now states the
25+
contract it always claimed to state, so the two reads are typed as the arrays
26+
they are and need nothing in front of them.
27+
28+
No behaviour changes for any driver that keeps the contract. What changes is
29+
what happens if one ever does not: the command now fails loudly instead of
30+
silently dropping the row, and dropping a `sys_secret` row from this read means
31+
dropping it from the report — which is the one thing this command's safety
32+
property forbids. A driver that answers something other than an array is a
33+
contract violation to fix at that driver, not to absorb here.
34+
35+
The shape is no longer assumed. `orphans.driver-contract.test.ts` boots the
36+
stack this command boots, names the concrete driver it resolves for
37+
`sys_secret` and `sys_setting`, and asserts that a seeded row comes back as a
38+
direct element of a bare array — then runs the command end to end against that
39+
same database and checks that a value from each of the two former call sites
40+
reaches the `--json` report.
Lines changed: 272 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,272 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* #14843 — what the driver behind `os secret orphans` ACTUALLY returns.
5+
*
6+
* The command used to wrap both of its reads in a local `rowsOf()` that
7+
* unwrapped `{ data: [...] }` and lifted a bare row into `[row]`. Removing it
8+
* rests on one fact, and the card that asked for the removal refused to let
9+
* that fact be inferred: `IDataDriver.find` is DECLARED to resolve to
10+
* `Record<string, unknown>[]`, but a declaration is not a reading, and the
11+
* counter-case is real — the console's `ObjectStackAdapter.find()` resolves to
12+
* a normalized `QueryResult` envelope and never to an array. Two methods
13+
* spelled `find`, opposite answers. So this file reads the CONCRETE driver,
14+
* through the boot this command performs, and asserts the shape.
15+
*
16+
* ## What is driven, and why it is the real thing
17+
*
18+
* `bootSchemaStack` with the command's own `extraPlugins` (platform objects +
19+
* the settings service), against a real sqlite file. That is the same call the
20+
* command makes, so `kernel.getService('objectql').getDriverForObject(…)`
21+
* resolves the same way it does at run time — `ObjectQL.getDriver()` hands back
22+
* a registered driver instance unwrapped, so whatever this test names is
23+
* exactly what the command holds.
24+
*
25+
* ⛔ `expect(Array.isArray(rows)).toBe(true)` on its own would be satisfied by a
26+
* driver that answers `[]` to everything, which is the reading that would make
27+
* the removal look safe while the command silently reported nothing. So every
28+
* shape assertion here is paired with a SEEDED row that has to come back
29+
* inside that array, and the two are asserted together.
30+
*
31+
* ## The second half: both call sites, end to end
32+
*
33+
* The shape is read at the seam; the command is then run for real against the
34+
* same database, and the `--json` report is checked for a value that could only
35+
* have travelled through EACH of the two former `rowsOf` call sites:
36+
*
37+
* - `counts.total` counts the `sys_secret` rows from the first read;
38+
* - `legacyInlineRows` can only be populated from `sys_setting` rows read by
39+
* the second, since a legacy inline `value_enc` exists nowhere else.
40+
*
41+
* A run that reported `total: 0` with an empty `legacyInlineRows` would be
42+
* indistinguishable from a broken read, which is why both are asserted with
43+
* seeded values rather than merely for absence of an error.
44+
*/
45+
46+
import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest';
47+
import { mkdtempSync, rmSync } from 'node:fs';
48+
import { tmpdir } from 'node:os';
49+
import { dirname, join, resolve } from 'node:path';
50+
import { fileURLToPath } from 'node:url';
51+
import { PlatformObjectsPlugin } from '@objectstack/platform-objects/plugin';
52+
import { SettingsServicePlugin } from '@objectstack/service-settings';
53+
import { bootSchemaStack, type SchemaStack } from '../../utils/schema-migrate.js';
54+
import type { SecretReferenceEngineLike } from '../../utils/secret-reference-union.js';
55+
import SecretOrphans from './orphans.js';
56+
57+
const HERE = dirname(fileURLToPath(import.meta.url));
58+
const CLI_ROOT = resolve(HERE, '..', '..', '..');
59+
60+
/**
61+
* The env vars that outrank an explicit `databaseUrl`, or move where the boot
62+
* keeps its state. Every one must be absent or this file measures some other
63+
* database and says nothing about anything (the `unmanaged-tables.integration`
64+
* discipline, same list and same reason).
65+
*/
66+
const OVERRIDING_ENV = [
67+
'OS_DATABASE_URL',
68+
'DATABASE_URL',
69+
'TURSO_DATABASE_URL',
70+
'OS_DATABASE_DRIVER',
71+
'OS_HOME',
72+
] as const;
73+
74+
/** A `sys_secret` row seeded straight through the driver, before any read. */
75+
const SEEDED_SECRET = {
76+
id: 'sec_14843_probe',
77+
namespace: 'smtp',
78+
key: 'probe_token',
79+
alg: 'aes-256-gcm',
80+
version: 1,
81+
kms_key_id: 'kms_local',
82+
ciphertext: 'ENC(v1:probe-cipher-material)',
83+
};
84+
85+
/**
86+
* A `sys_setting` row on the LEGACY INLINE path: `value_enc` holds ciphertext,
87+
* not a `sec_…` handle. Chosen because it is the one input whose effect on the
88+
* report (`legacyInlineRows`) can have come from nowhere but the second read.
89+
*/
90+
const SEEDED_SETTING = {
91+
namespace: 'smtp',
92+
key: 'inline_password',
93+
value_enc: 'ENC(v1:inline-legacy-ciphertext)',
94+
};
95+
96+
interface DriverProbe {
97+
find(object: string, query: Record<string, unknown>): Promise<unknown>;
98+
create(object: string, data: Record<string, unknown>): Promise<unknown>;
99+
}
100+
101+
describe('os secret orphans — the concrete driver behind both reads (#14843)', () => {
102+
let dir: string;
103+
let dbFile: string;
104+
let stack: SchemaStack | null = null;
105+
let secretDriver: DriverProbe;
106+
let settingDriver: DriverProbe;
107+
const savedEnv: Record<string, string | undefined> = {};
108+
const savedCwd = process.cwd();
109+
110+
beforeAll(async () => {
111+
dir = mkdtempSync(join(tmpdir(), 'os-14843-'));
112+
dbFile = join(dir, 'orphans.db');
113+
114+
for (const key of OVERRIDING_ENV) {
115+
savedEnv[key] = process.env[key];
116+
delete process.env[key];
117+
}
118+
savedEnv.OS_ARTIFACT_PATH = process.env.OS_ARTIFACT_PATH;
119+
savedEnv.NODE_ENV = process.env.NODE_ENV;
120+
// Deliberately absent: no compiled artifact, so the boot is the bare data
121+
// stack plus the two plugins the command passes.
122+
process.env.OS_ARTIFACT_PATH = join(dir, 'dist', 'objectstack.json');
123+
process.env.NODE_ENV = 'production';
124+
// The command does not pass `projectRoot`, so its boot takes `process.cwd()`
125+
// for its state directory. Stand in the tempdir so the run under test keeps
126+
// its state there instead of in whatever directory vitest started in.
127+
process.chdir(dir);
128+
129+
stack = await bootSchemaStack({
130+
jsonOutput: false,
131+
databaseUrl: `file:${dbFile}`,
132+
// Byte-identical to `orphans.ts`'s own list — the boot has to be the
133+
// command's, or the driver this file names is not the one it holds.
134+
extraPlugins: [new PlatformObjectsPlugin(), new SettingsServicePlugin({ registerRoutes: false })],
135+
});
136+
137+
const engine = stack.kernel.getService('objectql') as SecretReferenceEngineLike | undefined;
138+
if (!engine) throw new Error('no objectql engine on the booted stack — nothing to measure');
139+
secretDriver = engine.getDriverForObject('sys_secret') as unknown as DriverProbe;
140+
settingDriver = engine.getDriverForObject('sys_setting') as unknown as DriverProbe;
141+
if (!secretDriver || !settingDriver) {
142+
throw new Error('sys_secret / sys_setting resolved no driver — nothing to measure');
143+
}
144+
145+
await secretDriver.create('sys_secret', { ...SEEDED_SECRET });
146+
await settingDriver.create('sys_setting', { ...SEEDED_SETTING });
147+
}, 180_000);
148+
149+
afterAll(async () => {
150+
try { await stack?.shutdown(); } catch { /* torn down either way */ }
151+
stack = null;
152+
process.chdir(savedCwd);
153+
for (const key of OVERRIDING_ENV) {
154+
if (savedEnv[key] === undefined) delete process.env[key];
155+
else process.env[key] = savedEnv[key];
156+
}
157+
for (const key of ['OS_ARTIFACT_PATH', 'NODE_ENV'] as const) {
158+
if (savedEnv[key] === undefined) delete process.env[key];
159+
else process.env[key] = savedEnv[key];
160+
}
161+
try { rmSync(dir, { recursive: true, force: true }); } catch { /* ignore */ }
162+
});
163+
164+
it('names the concrete driver this command actually holds', () => {
165+
// Not a preference for a particular driver — it is the premise every
166+
// assertion below rests on, named so a failure says WHICH driver moved.
167+
//
168+
// `IDataDriver.name` is the identity to assert; the CLASS name is checked
169+
// with a suffix match because this package resolves the driver through
170+
// `@objectstack/driver-sql`'s BUILT entry, where the bundler renames the
171+
// class `_SqlDriver`. An `=== 'SqlDriver'` assertion here is a statement
172+
// about the bundler, not about the driver.
173+
expect((secretDriver as unknown as { name?: unknown }).name).toBe('com.objectstack.driver.sql');
174+
expect(secretDriver.constructor.name).toMatch(/SqlDriver$/);
175+
// The two reads must not silently resolve to different drivers: the command
176+
// treats both results the same way.
177+
expect((settingDriver as unknown as { name?: unknown }).name).toBe('com.objectstack.driver.sql');
178+
expect(settingDriver.constructor.name).toMatch(/SqlDriver$/);
179+
});
180+
181+
it('`sys_secret` find() resolves to a bare ARRAY holding the row — no envelope', async () => {
182+
const rows = await secretDriver.find('sys_secret', {});
183+
184+
expect(Array.isArray(rows)).toBe(true);
185+
// The envelope shapes the removed normalizer existed to unwrap. Asserted
186+
// as absent on the value itself, so this fails loudly if a driver ever
187+
// starts answering `{ data: [...] }` or `{ records: [...] }`.
188+
expect(Object.prototype.hasOwnProperty.call(rows, 'data')).toBe(false);
189+
expect(Object.prototype.hasOwnProperty.call(rows, 'records')).toBe(false);
190+
191+
// …and the array is the row list itself, not a one-element wrapper around
192+
// one: the seeded row is directly an element. This is what makes the
193+
// `Array.isArray` above mean something.
194+
const list = rows as Array<Record<string, unknown>>;
195+
const seeded = list.find((r) => r.id === SEEDED_SECRET.id);
196+
expect(seeded).toBeDefined();
197+
expect(seeded!.namespace).toBe(SEEDED_SECRET.namespace);
198+
expect(seeded!.key).toBe(SEEDED_SECRET.key);
199+
}, 60_000);
200+
201+
it('`sys_setting` find() answers the same way — the second read is not a different contract', async () => {
202+
const rows = await settingDriver.find('sys_setting', {});
203+
204+
expect(Array.isArray(rows)).toBe(true);
205+
const list = rows as Array<Record<string, unknown>>;
206+
const seeded = list.find((r) => r.key === SEEDED_SETTING.key);
207+
expect(seeded).toBeDefined();
208+
expect(seeded!.value_enc).toBe(SEEDED_SETTING.value_enc);
209+
}, 60_000);
210+
211+
it('an EMPTY result is `[]`, never null/undefined — the `!result` limb was dead too', async () => {
212+
// `rowsOf` opened with `if (!result) return []`. Read an object that exists
213+
// and holds nothing rather than one that does not exist: a throwing read
214+
// would prove nothing about what a successful empty read returns.
215+
const rows = await secretDriver.find('sys_secret', { where: { id: 'sec_no_such_row_14843' } });
216+
expect(Array.isArray(rows)).toBe(true);
217+
expect(rows as unknown[]).toEqual([]);
218+
// Positive control with the same call and the same flags: the unfiltered
219+
// read is non-empty, so the `[]` above is the filter and not a read that
220+
// cannot see anything.
221+
expect((await secretDriver.find('sys_secret', {})) as unknown[]).not.toEqual([]);
222+
}, 60_000);
223+
224+
it('the command runs against this database and BOTH former call sites carry their rows', async () => {
225+
const chunks: string[] = [];
226+
const stdout = vi.spyOn(process.stdout, 'write').mockImplementation(
227+
((chunk: unknown, ...rest: unknown[]) => {
228+
chunks.push(String(chunk));
229+
const done = rest.find((a) => typeof a === 'function') as ((e?: Error | null) => void) | undefined;
230+
done?.(null);
231+
return true;
232+
}) as never,
233+
);
234+
const savedExitCode = process.exitCode;
235+
try {
236+
await SecretOrphans.run(
237+
['--json', '--no-declared-datasources', '--database-url', `file:${dbFile}`],
238+
{ root: CLI_ROOT },
239+
);
240+
} finally {
241+
stdout.mockRestore();
242+
process.exitCode = savedExitCode;
243+
}
244+
245+
const lines = chunks.join('').split('\n').filter((l) => l.trim() !== '');
246+
const payload = JSON.parse(lines[lines.length - 1]) as {
247+
mode?: string;
248+
error?: string;
249+
plan?: {
250+
counts: { total: number };
251+
rows: Array<{ id: string }>;
252+
legacyInlineRows: Array<{ namespace: string; key: string }>;
253+
};
254+
};
255+
256+
// A boot or read failure lands as an `error` envelope; naming it here beats
257+
// a downstream `undefined` that reads like a shape change.
258+
expect(payload.error).toBeUndefined();
259+
expect(payload.mode).toBe('report');
260+
261+
// First former call site (`sys_secret`): the seeded row reached the plan.
262+
expect(payload.plan!.counts.total).toBeGreaterThanOrEqual(1);
263+
expect(payload.plan!.rows.map((r) => r.id)).toContain(SEEDED_SECRET.id);
264+
265+
// Second former call site (`sys_setting`): `legacyInlineRows` is derived
266+
// from `settingRows` and from nothing else, so this value can only have
267+
// travelled through the second read.
268+
expect(payload.plan!.legacyInlineRows).toContainEqual(
269+
expect.objectContaining({ namespace: SEEDED_SETTING.namespace, key: SEEDED_SETTING.key }),
270+
);
271+
}, 180_000);
272+
});

packages/cli/src/commands/secret/orphans.ts

Lines changed: 17 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -242,7 +242,22 @@ export default class SecretOrphans extends Command {
242242
return;
243243
}
244244

245-
const rawSecrets = rowsOf(await secretDriver.find('sys_secret', {}));
245+
// Read as an ARRAY, with no normalizer in front of it. `IDataDriver.find`
246+
// resolves to `Record<string, unknown>[]`, and the union's driver port
247+
// now says so too, so there is no `{ records }` / `{ data }` / bare-row
248+
// envelope for a limb to unwrap: every concrete driver behind
249+
// `getDriverForObject()` — SqlDriver (and SqliteWasmDriver, which extends
250+
// it), TursoDriver local and remote, MongoDBDriver, InMemoryDriver —
251+
// returns an array on every path it can return on, `[]` included.
252+
//
253+
// ⛔ Do not reintroduce one "just in case". A normalizer here is not a
254+
// safety net: its limbs are unreachable, so nothing exercises them, and
255+
// its filter would silently DROP a row the drivers cannot produce anyway
256+
// — dropping a `sys_secret` row from this read is dropping it from the
257+
// report, and this command's whole safety property is that no row goes
258+
// unmentioned. A driver that ever answered something else is a contract
259+
// violation to fix at that driver, not to absorb here.
260+
const rawSecrets = await secretDriver.find('sys_secret', {});
246261
const rawById = new Map(rawSecrets.map((r) => [String(r.id), r]));
247262
// ⛔ `ciphertext` is dropped here and not carried into the plan: the plan
248263
// is printed and serialised, and cipher material must not be reachable
@@ -259,7 +274,7 @@ export default class SecretOrphans extends Command {
259274

260275
const settingDriver = engine.getDriverForObject('sys_setting');
261276
const settingRows: SettingRowSnapshot[] = settingDriver
262-
? rowsOf(await settingDriver.find('sys_setting', {})).map((r) => ({
277+
? (await settingDriver.find('sys_setting', {})).map((r) => ({
263278
namespace: String(r.namespace ?? ''),
264279
key: String(r.key ?? ''),
265280
scope: (r.scope as string | null | undefined) ?? null,
@@ -434,17 +449,6 @@ export function asDeletingDriver(driver: unknown): SecretDeleteDriverLike | null
434449
: null;
435450
}
436451

437-
/** Normalise a driver result (`T[]` or `{ data: T[] }` or a single row). */
438-
function rowsOf(result: unknown): Array<Record<string, unknown>> {
439-
if (!result) return [];
440-
const list = Array.isArray(result)
441-
? result
442-
: Array.isArray((result as { data?: unknown }).data)
443-
? (result as { data: unknown[] }).data
444-
: [result];
445-
return list.filter((r): r is Record<string, unknown> => !!r && typeof r === 'object');
446-
}
447-
448452
/**
449453
* Read the host's declared datasource artefacts.
450454
*

0 commit comments

Comments
 (0)