Skip to content

Commit 5c8f5af

Browse files
claude[bot]claude
andauthored
fix(engine): ObjectRepository declares the findOne / update shapes it already published (#17255)
* fix(engine): ObjectRepository declares the findOne/update shapes it already publishes `IScopedObjectRepository.findOne` / `.update` declare `Record<string, any> | null` and `Record<string, any> | number | null`, and `IDataEngine` — the call each of these forwards to — declares the same. `ObjectRepository` sat between two narrow declarations and re-widened the value back to `Promise<any>` on the way out, which `implements IScopedObjectRepository` accepts (a wider return always satisfies a narrower one) while every call site reaching a repository through the CLASS kept reading `any`, `ObjectQL.createContext(…).object(n).findOne(…)` included. Census: one consumer, `engine-filter-alias.test.ts`, which read `.status` off a value that can be null. Repaired with the file's own `not.toBeNull()` / `!` idiom. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XTBcV7zZHmokdyQgXjbyEU * test(engine): pin that a class-typed `object(name)` hands back a declared repository Compiler-driven probes (`ts.createProgram`, the idiom `packages/spec/src/contracts/scoped-context.test.ts` uses) over the exported class doors — `ScopedContext`, `ObjectQL.createContext`, `sudo()` — asserting the diagnostic NAMES the declared shape, so neither a bare "it errored" nor an `any` that erased the type can satisfy it. Anti-vacuity: the legal spelling must compile clean and no probe may report TS2307. Probes go through the CLASS, not `HookContext`: `HookContext.api` was narrowed to `IScopedContext` by #5945, so a `(ctx: HookContext)` probe is green on both sides of this fix and pins nothing. Measured, and recorded in the file header. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XTBcV7zZHmokdyQgXjbyEU * chore(engine): changeset for the declared repository return shapes Graded `patch`: nothing is widened and no symbol is added. The contract already published these shapes; the implementation is coming back to a declaration it had already published. Checked against the recorded WHICH LEVEL ruling of 2026-09-04 (decision batch #35, on #15294), whose `minor` trigger is additive widening. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XTBcV7zZHmokdyQgXjbyEU * chore(engine): re-grade the repository return declarations to minor + BREAKING Landed precedent PR #15280 measured: `SqlDriver.update()` and the `TursoDriver.update()` override moved off an explicit `Promise<any>` onto the shape `IDataDriver` already declared -- no new exported symbol, `packages/spec` untouched -- and both changesets shipped `minor` with a **BREAKING** banner. That is this change's shape exactly, so the earlier `patch` reasoning ("the contract already published it, so nothing moved") is the very fact pattern that precedent grades `minor`: the emitted `.d.ts` read `any`, so no caller holding the class was ever asked to narrow. The ADR-0087 disposition is `no-migration-prescription`, as sibling PR #16783 used for the same family. `type-surface-only` is semantically the right category but its predicate 4 cannot address either narrowed symbol; the marker records that measurement. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XTBcV7zZHmokdyQgXjbyEU --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent d64bcb6 commit 5c8f5af

4 files changed

Lines changed: 251 additions & 4 deletions

File tree

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
---
2+
"@objectstack/objectql": minor
3+
---
4+
5+
feat(engine): `ObjectRepository.findOne` / `.update` publish their honest types — the contract's shapes, not `any` (#16786)
6+
7+
**BREAKING** for TypeScript consumers — a published TYPE-surface narrowing, shipped as `minor` under the launch-window convention (the one PR #15280 used for `SqlDriver.update()` and the `TursoDriver.update()` override, and PR #14434 before it on `@objectstack/driver-memory`).
8+
9+
`ObjectRepository.findOne()` and `.update()` were written out with an explicit `Promise<any>` while they have always answered what the contract declares — each one forwards, one line down, to an `IDataEngine` door that already declares the shape:
10+
11+
- `findOne``Promise<Record<string, any> | null>`
12+
- `update``Promise<Record<string, any> | number | null>`
13+
14+
`IScopedObjectRepository` — the contract this class carries an `implements` clause for — declares both, and has since ruling A on #16231 landed (PR #16783). An explicit `any` satisfies that structurally, because a **wider** declared return always satisfies a narrower one: `class ObjectRepository implements IScopedObjectRepository` compiled green the whole time while the emitted `.d.ts` read `Promise<any>`, so no caller holding an `ObjectRepository` — or reaching one through `ScopedContext` or `ObjectQL.createContext()`, both exported from this package's index — was ever asked to narrow. They are now declared as the contract declares them. No runtime behaviour changes.
15+
16+
A caller that read fields off `findOne()`'s result through the `any` now narrows the `null` arm first; a caller that read `update()`'s result now separates the by-id record from the predicate-form count. The in-repo census for this change was one file, repaired alongside.
17+
18+
`updateById` is deliberately untouched: `IScopedObjectRepository.updateById` itself declares `Promise<any>`, so the class already matches its contract and there is no drift to repair on this side. That half stays open on #16786.
19+
20+
<!-- adr-0087: not-required (no-migration-prescription) Nothing authorable moves. No metadata key, no spec key, no authored property, no config field, no accepted request shape and no stored artifact changes spelling or shape; `packages/spec` is untouched, so `objectstack migrate meta` has nothing to rewrite, `spec-changes.json` has nothing to project and the upgrade guide has no row to gain. What moves is the declared RETURN TYPE of two TypeScript methods, and the rewrite this ships -- narrow the `null` arm -- is addressed to a TYPESCRIPT CONSUMER and delivered by the compiler at their own call site, which is the audience the ADR-0087 ledger explicitly does not serve.
21+
`type-surface-only` is the category built for exactly this class and it is NOT claimed here, because its predicate 4 cannot be made to name this change's symbols -- measured, not assumed. The bare form `engine.ts#findOne` resolves to the FIRST same-named member in the file, `ObjectQL.findOne` (line 9761), which #16783 already narrowed, so predicate 4 reads `narrowed-from-erased is FALSE: at the merge base ... was already CONCRETE` -- a true statement about a member this diff never touched. The documented fallback, a dotted member path, is walked only through OBJECT-LITERAL nesting and refuses a class member: `ObjectRepository.findOne does not resolve: no ObjectRepository object literal is declared`. Both narrowed members are class members whose names repeat in the file, so neither spelling can address them. The gap is reported on the card rather than worked around, and the **BREAKING** banner above is carried rather than dropped -- which is the erosion #13080 was filed about. -->

packages/objectql/src/engine-filter-alias.test.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -217,8 +217,15 @@ describe('filter → where folds on every engine method (#4346)', () => {
217217
const repo = ctx.object('task');
218218
const viaWhere = await repo.findOne({ where: { status: 'done' } });
219219
const viaFilter = await repo.findOne({ filter: { status: 'done' } });
220-
expect(viaFilter.status).toBe('done');
221-
expect(viaWhere.status).toBe('done');
220+
// [#16786] `repo.findOne` declares `Record<string, any> | null`, so the
221+
// null both spellings could return is asserted away rather than read
222+
// through — the same `expect(row).not.toBeNull()` / `row!` idiom this
223+
// file already uses above. Under the old `Promise<any>` this pair
224+
// agreed vacuously if BOTH lookups came back null.
225+
expect(viaFilter).not.toBeNull();
226+
expect(viaWhere).not.toBeNull();
227+
expect(viaFilter!.status).toBe('done');
228+
expect(viaWhere!.status).toBe('done');
222229
});
223230

224231
it('a cleanup hook calling repo.delete({filter, multi}) no longer empties the object', async () => {

packages/objectql/src/engine.ts

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14791,7 +14791,23 @@ export class ObjectRepository implements IScopedObjectRepository {
1479114791
});
1479214792
}
1479314793

14794-
async findOne(query: any = {}): Promise<any> {
14794+
/**
14795+
* [#16786] Declared `Promise<Record<string, any> | null>`, not `Promise<any>`.
14796+
*
14797+
* `IScopedObjectRepository.findOne` has declared that shape since #16231's
14798+
* ruling A landed (PR #16783), and `IDataEngine.findOne` — the call this
14799+
* method forwards to, one line down — declares it too. This method sat
14800+
* between two narrow declarations and re-widened the value back to `any` on
14801+
* the way out, so `implements IScopedObjectRepository` stayed satisfied (a
14802+
* wider return always satisfies a narrower one) while every call site that
14803+
* reaches a repository through the CLASS rather than the interface kept
14804+
* reading `any` — `ObjectQL.createContext(…).object(n).findOne(…)` among
14805+
* them, which is exported.
14806+
*
14807+
* ⛔ Not a narrowing of the contract: the contract already said this. This
14808+
* is the implementation coming back to the declaration it published.
14809+
*/
14810+
async findOne(query: any = {}): Promise<Record<string, any> | null> {
1479514811
return this.engine.findOne(this.objectName, {
1479614812
...query,
1479714813
context: this.context,
@@ -14809,7 +14825,18 @@ export class ObjectRepository implements IScopedObjectRepository {
1480914825
return this.insert(data);
1481014826
}
1481114827

14812-
async update(data: any, options: any = {}): Promise<any> {
14828+
/**
14829+
* [#16786] Declared `Promise<Record<string, any> | number | null>`, the same
14830+
* re-widening as {@link findOne} and repaired the same way: the record for
14831+
* the single-record form, the affected-row count for the predicate form
14832+
* (`{ where, multi: true }`), `null` when the write matched nothing.
14833+
*
14834+
* ⛔ `updateById` is deliberately NOT touched here. Its `Promise<any>` is
14835+
* what `IScopedObjectRepository.updateById` itself declares, so the class
14836+
* matches its contract and there is no drift to repair on this side; that
14837+
* member is `packages/spec`'s to narrow and stays open on #16786.
14838+
*/
14839+
async update(data: any, options: any = {}): Promise<Record<string, any> | number | null> {
1481314840
return this.engine.update(this.objectName, data, {
1481414841
...options,
1481514842
context: this.context,
Lines changed: 192 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,192 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
import { describe, it, expect } from 'vitest';
4+
import ts from 'typescript';
5+
import { dirname, resolve } from 'node:path';
6+
import { fileURLToPath } from 'node:url';
7+
8+
// ─── [#16786] the repository a CLASS-typed call site reaches is declared ────
9+
//
10+
// `IScopedObjectRepository` (`packages/spec/src/contracts/scoped-context.ts`)
11+
// declares `findOne` as `Promise<Record<string, any> | null>` and `update` as
12+
// `Promise<Record<string, any> | number | null>` — ruling A on #16231, landed
13+
// as PR #16783. `IDataEngine`, the call each `ObjectRepository` member forwards
14+
// to, declares the same shapes. `ObjectRepository` sat between those two narrow
15+
// declarations and re-widened the result back to `Promise<any>`.
16+
//
17+
// `implements` does not catch that: a WIDER declared return always satisfies a
18+
// narrower one, so `class ObjectRepository implements IScopedObjectRepository`
19+
// compiled green while the members it published were `any`. The interface's
20+
// narrowing therefore reached only the call sites whose STATIC type is the
21+
// interface — and the doors this package exports are typed as the CLASS:
22+
//
23+
// ObjectQL.createContext(ctx).object(n) -> ScopedContext -> ObjectRepository
24+
// ScopedContext.sudo().object(n) -> ObjectRepository
25+
// engine.transaction((trxCtx) => …) -> ScopedContext -> ObjectRepository
26+
//
27+
// ## What this file measures, and what it deliberately does not
28+
//
29+
// Measured on `origin/main` ae19f5edb7 before the fix, with these probes:
30+
//
31+
// ctx: HookContext ; ctx.api!.object(n).findOne(…) -> ALREADY NARROW
32+
// api: ScopedContext ; api.object(n).findOne(…) -> `any`
33+
// ql.createContext({}).object(n).findOne(…) -> `any`
34+
//
35+
// ⚠️ The first line is why the probes below are written through the CLASS and
36+
// the exported engine door rather than through `HookContext`. `HookContext.api`
37+
// was narrowed to `IScopedContext` by #5945/#6311, so a handler typed
38+
// `(ctx: HookContext) => …` reads the narrow type today and read it before this
39+
// fix too — a probe written that way is GREEN on both sides and pins nothing.
40+
// The `any` lives on the class-typed doors, so that is where the probes go.
41+
//
42+
// ## Why the compiler API rather than `@ts-expect-error`
43+
//
44+
// The same reason `packages/spec/src/contracts/scoped-context.test.ts` gives:
45+
// `@ts-expect-error` is satisfied by ANY error on the next line, and this
46+
// file's whole subject is WHICH type a call resolves to. Every negative probe
47+
// below asserts that the diagnostic NAMES the declared shape, so a bare "it
48+
// errored" — or an `any` that erased the type entirely — cannot satisfy it.
49+
//
50+
// Anti-vacuity: a harness that resolves nothing reports zero diagnostics and
51+
// looks exactly like success, so `control-legal` must compile CLEAN, and no
52+
// probe may report TS2307 (unresolved module).
53+
54+
const HERE = dirname(fileURLToPath(import.meta.url));
55+
const PKG = resolve(HERE, '..');
56+
57+
/**
58+
* Compile probe files against this package's real `src/engine.ts` and return
59+
* each one's diagnostics. The probes live (virtually) beside the source, so
60+
* `../engine` resolves the way any sibling module would and `@objectstack/spec`
61+
* resolves the way a real consumer's does — through the installed package.
62+
*/
63+
function compileProbes(probes: Readonly<Record<string, string>>): Map<string, ts.Diagnostic[]> {
64+
const dir = resolve(PKG, 'src/__scoped_repo_probes__');
65+
const paths = new Map<string, string>();
66+
for (const [name, text] of Object.entries(probes)) paths.set(resolve(dir, `${name}.ts`), text);
67+
68+
const options: ts.CompilerOptions = {
69+
target: ts.ScriptTarget.ES2020,
70+
module: ts.ModuleKind.ESNext,
71+
moduleResolution: ts.ModuleResolutionKind.Bundler,
72+
strict: true,
73+
skipLibCheck: true,
74+
noEmit: true,
75+
// A probe declares a const and stops; TS6133 is an opinion about the
76+
// probe's framing, not about whether the call is well-typed.
77+
noUnusedLocals: false,
78+
noUnusedParameters: false,
79+
types: ['node'],
80+
baseUrl: PKG,
81+
};
82+
83+
const host = ts.createCompilerHost(options, true);
84+
const realGetSourceFile = host.getSourceFile.bind(host);
85+
const realFileExists = host.fileExists.bind(host);
86+
const realReadFile = host.readFile.bind(host);
87+
host.getSourceFile = (fileName, languageVersion, onError, shouldCreate) => {
88+
const overlay = paths.get(resolve(fileName));
89+
return overlay === undefined
90+
? realGetSourceFile(fileName, languageVersion, onError, shouldCreate)
91+
: ts.createSourceFile(fileName, overlay, languageVersion, true);
92+
};
93+
host.fileExists = (fileName) => paths.has(resolve(fileName)) || realFileExists(fileName);
94+
host.readFile = (fileName) => paths.get(resolve(fileName)) ?? realReadFile(fileName);
95+
96+
const program = ts.createProgram([...paths.keys()], options, host);
97+
const out = new Map<string, ts.Diagnostic[]>();
98+
for (const name of Object.keys(probes)) out.set(name, []);
99+
for (const d of ts.getPreEmitDiagnostics(program)) {
100+
const file = d.file?.fileName ? resolve(d.file.fileName) : undefined;
101+
for (const name of Object.keys(probes)) {
102+
if (file === resolve(dir, `${name}.ts`)) out.get(name)!.push(d);
103+
}
104+
}
105+
return out;
106+
}
107+
108+
/** One diagnostic per line, `TS<code>: <message>`, for readable assertions. */
109+
function render(diagnostics: readonly ts.Diagnostic[]): string {
110+
return diagnostics
111+
.map((d) => `TS${d.code}: ${ts.flattenDiagnosticMessageText(d.messageText, ' ')}`)
112+
.join('\n');
113+
}
114+
115+
const PROBES = {
116+
// ── anti-vacuity: the harness really resolves and really compiles ────────
117+
'control-legal': `
118+
import type { ObjectQL } from '../engine';
119+
export const probe = async (ql: ObjectQL) => {
120+
const row = await ql.createContext({}).object('task').findOne({ where: { id: 't1' } });
121+
return row === null ? 'missing' : String(row.status);
122+
};`,
123+
// ── the class-typed door: what a hook reaches at RUNTIME ─────────────────
124+
'class-door-findOne': `
125+
import type { ScopedContext } from '../engine';
126+
export const probe = async (api: ScopedContext) => {
127+
const bad: number = await api.object('task').findOne({ where: { id: 't1' } });
128+
};`,
129+
// ── the exported public door ─────────────────────────────────────────────
130+
'public-door-findOne': `
131+
import type { ObjectQL } from '../engine';
132+
export const probe = async (ql: ObjectQL) => {
133+
const bad: number = await ql.createContext({}).object('task').findOne({ where: { id: 't1' } });
134+
};`,
135+
// ── the elevated door ────────────────────────────────────────────────────
136+
'sudo-door-findOne': `
137+
import type { ScopedContext } from '../engine';
138+
export const probe = async (api: ScopedContext) => {
139+
const bad: number = await api.sudo().object('task').findOne({ where: { id: 't1' } });
140+
};`,
141+
// ── update carries the same repair ───────────────────────────────────────
142+
'class-door-update': `
143+
import type { ScopedContext } from '../engine';
144+
export const probe = async (api: ScopedContext) => {
145+
const bad: boolean = await api.object('task').update({ id: 't1', status: 'done' });
146+
};`,
147+
// ── the direct any-detector, in case a future edit reaches `any` by ──────
148+
// ── some route the assignment probes above do not cover ─────────────────
149+
'not-any-findOne': `
150+
import type { ScopedContext } from '../engine';
151+
type IsAny<T> = 0 extends (1 & T) ? true : false;
152+
type Row = Awaited<ReturnType<ReturnType<ScopedContext['object']>['findOne']>>;
153+
export const isAny: IsAny<Row> = true;`,
154+
} as const;
155+
156+
describe('[#16786] `object(name)` hands back a DECLARED repository, not `any`', () => {
157+
const diagnostics = compileProbes(PROBES);
158+
159+
it('resolves every probe against real source (anti-vacuity)', () => {
160+
for (const [name, ds] of diagnostics) {
161+
expect(render(ds), `${name} failed to resolve its imports`).not.toContain('TS2307');
162+
}
163+
// The legal spelling — null handled — must compile with nothing to say.
164+
// Without this, a harness that compiled nothing would satisfy every
165+
// negative probe below by reporting no diagnostics at all.
166+
expect(render(diagnostics.get('control-legal')!)).toBe('');
167+
});
168+
169+
it.each([
170+
['class-door-findOne', 'Record<string, any> | null'],
171+
['public-door-findOne', 'Record<string, any> | null'],
172+
['sudo-door-findOne', 'Record<string, any> | null'],
173+
// TypeScript normalises this union's order; the string is the compiler's
174+
// own rendering, not the source order in `IScopedObjectRepository`.
175+
['class-door-update', 'number | Record<string, any> | null'],
176+
])('%s: the diagnostic NAMES the declared shape', (probe, declared) => {
177+
const text = render(diagnostics.get(probe)!);
178+
// Not merely "some error": `any` produces NO error here, and an erased or
179+
// widened declaration produces one that does not name this shape.
180+
expect(text).toContain('TS2322');
181+
expect(text).toContain(declared);
182+
});
183+
184+
it('findOne is not `any` — measured by the compiler, not by reading the source', () => {
185+
// `IsAny<Row>` is `false` once the declaration is honest, so assigning
186+
// `true` to it is an error. When `Row` is `any`, `IsAny<Row>` is `true`
187+
// and this probe compiles clean — which is the pre-fix reading.
188+
const text = render(diagnostics.get('not-any-findOne')!);
189+
expect(text).toContain('TS2322');
190+
expect(text).toContain("Type 'true' is not assignable to type 'false'");
191+
});
192+
});

0 commit comments

Comments
 (0)