Skip to content

Commit d82b192

Browse files
heskewclaude
andcommitted
test(models): unit tests + simplify registry + writer mock seam
Part of #628 (Phase 1 step 7). 47 new unit tests + 1 Scope.test addition. backendRegistry.test.js (8 tests): direct-mapping API, default logicalName, multiple names, kind isolation, not-found error, re-mapping replaces, clearRegistry empties, error message does not enumerate registrations. TestBackend.test.js (11 tests): capabilities + name, embed shape + determinism + token math, generate string + Message[] + {messages} inputs + usage, generateStream chunks + finishReason. Models.test.js (17 tests): unwraps to Float32Array[], analytics record on success/failure, no-ALS fallback (tenant/app undefined), ALS-bound accounting, extractTenantId user.tenant→user.tenantId fallback, AbortSignal precedence (opts > ctx), ctx.signal forwarded when opts.signal absent, ModelCapabilityError, ModelPendingNotSupportedError, sanitized error_code (backend_error/aborted), no leak of raw upstream message into the row, generate adapter+conversation_id, generateStream success record at stream end, generateStream mid-stream throw → success=false. analyticsTable.test.js (11 tests): pure-buffer behavior via a mock table. write buffers without writing, multiple writes accumulate, write after stop is a no-op, flush writes batch + clears buffer, flush no-op on empty buffer, flush works after stop (drain), records carry the expected schema fields, put-throw doesn't drop remaining records, maxBufferSize triggers out-of-cadence flush, cleanup removes old rows, cleanup is a no-op when nothing is old. Real-LMDB analytics coverage is intentionally not unit-tested — mirrors `analytics/write.ts`'s posture (unit tests only the pure helpers; LMDB writes are end-to-end-only). Scope.test.js: extends the existing 'should create a default entry handler' test to assert `scope.models` is a `Models` instance and exposes the three callable methods. The wiring point itself, in the right place. Registry refactor (resources/models/backendRegistry.ts): the prior commit's `envGet('models.<kind>.<logicalName>.backend')` doesn't work — Harper's `getConfigValue` only reads keys in `CONFIG_PARAM_MAP`, so dynamic config paths return undefined. Adding entries per logical name is impossible (logical names are user-defined). Refactored to direct `setEmbedding(name, backend)` / `setGenerative(name, backend)`. Phase 2 will add a YAML→registry bootstrapper alongside the first real backend, calling these setters at boot. Error class collapsed to one `ModelBackendNotFoundError`; message identifies kind + logical name without enumerating other registrations. Models.ts is unaffected — it only uses `resolveEmbedding` / `resolveGenerative`, whose signatures didn't change. Writer testability (resources/models/analyticsTable.ts): added `getTable` to `ModelCallAnalyticsWriterOpts` so tests inject a mock table. Default still resolves to the real `getModelCallsTable()`. Also: `flush()` now awaits each `primaryStore.put()` return value (when promise-shaped) before resolving, so callers can read back inserted rows deterministically. Cycle workaround in Models.test.js: requires `#src/resources/databases` before `#src/resources/transaction` to mirror the load order in other unit tests; mocha's ESM-first loader otherwise hits a transaction.ts ↔ DatabaseTransaction/blob cycle when transaction is the first edge into the graph. Coverage on `resources/models/`: 94% statements, 92.6% branches. Models.ts and TestBackend.ts at 100%. analyticsTable.ts at 82% with the lazy-getter for the real LMDB table the deliberate gap. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 7fe0f23 commit d82b192

7 files changed

Lines changed: 716 additions & 48 deletions

File tree

resources/models/analyticsTable.ts

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,8 @@ export interface ModelCallAnalyticsWriterOpts {
8181
cleanupIntervalMs?: number;
8282
/** Default 90d. Rows older than this are removed by `cleanup()`. */
8383
retentionMs?: number;
84+
/** Override the table accessor. Tests inject a mock to avoid touching real LMDB. */
85+
getTable?: () => any;
8486
}
8587

8688
/**
@@ -100,13 +102,15 @@ export class ModelCallAnalyticsWriter {
100102
#cleanupTimer?: NodeJS.Timeout;
101103
#maxBufferSize: number;
102104
#retentionMs: number;
105+
#getTable: () => any;
103106
#stopped = false;
104107

105108
constructor(opts: ModelCallAnalyticsWriterOpts = {}) {
106109
const flushIntervalMs = opts.flushIntervalMs ?? DEFAULT_FLUSH_INTERVAL_MS;
107110
const cleanupIntervalMs = opts.cleanupIntervalMs ?? DEFAULT_CLEANUP_INTERVAL_MS;
108111
this.#maxBufferSize = opts.maxBufferSize ?? DEFAULT_MAX_BUFFER_SIZE;
109112
this.#retentionMs = opts.retentionMs ?? resolveRetentionMs();
113+
this.#getTable = opts.getTable ?? getModelCallsTable;
110114
this.#flushTimer = setInterval(() => {
111115
this.flush().catch((err) => log.warn?.(`Model-call analytics flush failed: ${err?.message ?? err}`));
112116
}, flushIntervalMs);
@@ -130,19 +134,26 @@ export class ModelCallAnalyticsWriter {
130134
if (this.#buffer.length === 0) return;
131135
const batch = this.#buffer;
132136
this.#buffer = [];
133-
const tbl = getModelCallsTable();
137+
const tbl = this.#getTable();
138+
const puts: Promise<unknown>[] = [];
134139
for (const record of batch) {
135140
try {
136-
tbl.primaryStore.put(record.id, record);
141+
const result = tbl.primaryStore.put(record.id, record);
142+
if (result && typeof (result as { then?: unknown }).then === 'function') {
143+
puts.push(result as Promise<unknown>);
144+
}
137145
} catch (err) {
138146
log.warn?.(`Model-call analytics put failed for id=${record.id}: ${(err as Error)?.message ?? err}`);
139147
}
140148
}
149+
if (puts.length > 0) {
150+
await Promise.allSettled(puts);
151+
}
141152
}
142153

143154
async cleanup(): Promise<void> {
144155
const end = Date.now() - this.#retentionMs;
145-
const tbl = getModelCallsTable();
156+
const tbl = this.#getTable();
146157
for (const key of tbl.primaryStore.getKeys({ start: false, end })) {
147158
tbl.primaryStore.remove(key);
148159
}
Lines changed: 35 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -1,76 +1,66 @@
1-
import { get as envGet } from '../../utility/environment/environmentManager.ts';
21
import { ServerError } from '../../utility/errors/hdbError.ts';
32
import type { ModelBackend } from './types.ts';
43

54
/**
65
* Process-wide model backend registry.
76
*
8-
* Backends register themselves by `backend.name` (e.g. `'test'`, `'ollama'`,
9-
* `'ollama:fast'`). Per-kind logical-name resolution is config-driven:
10-
* `resolveEmbedding('default')` reads `models.embedding.default.backend` from
11-
* config and returns the registered backend with that name.
7+
* Stores logical-name → backend-instance mappings for embedding and
8+
* generative kinds. App code or boot wiring populates the registry via
9+
* `setEmbedding(...)` / `setGenerative(...)`; the `Models` facade reads it
10+
* via `resolveEmbedding(...)` / `resolveGenerative(...)`.
1211
*
1312
* Module-scope state is intentional — one registry per Harper process,
14-
* mirroring `contextStorage` at `resources/transaction.ts:6`.
13+
* mirroring `contextStorage` at `resources/transaction.ts:6`. Translating
14+
* a YAML `models:` config block into registry entries (the bootstrapper
15+
* step) lands in Phase 2 alongside the first real backend.
1516
*/
1617

1718
type ModelKind = 'embedding' | 'generative';
1819

19-
const byName: Map<string, ModelBackend> = new Map();
20+
const embedding: Map<string, ModelBackend> = new Map();
21+
const generative: Map<string, ModelBackend> = new Map();
2022

21-
/**
22-
* Register a backend instance. Idempotent on `backend.name` — re-registering
23-
* with the same name replaces the prior instance (convenient for tests).
24-
*/
25-
export function registerBackend(backend: ModelBackend): void {
26-
byName.set(backend.name, backend);
23+
/** Map `logicalName` to a backend for embedding calls. Re-set replaces. */
24+
export function setEmbedding(logicalName: string, backend: ModelBackend): void {
25+
embedding.set(logicalName, backend);
26+
}
27+
28+
/** Map `logicalName` to a backend for generative calls. Re-set replaces. */
29+
export function setGenerative(logicalName: string, backend: ModelBackend): void {
30+
generative.set(logicalName, backend);
2731
}
2832

2933
/**
30-
* Resolve the backend configured as the embedding provider for `logicalName`
31-
* (default: `'default'`). Throws if no config entry exists or the named
32-
* backend is not registered.
34+
* Resolve the embedding backend mapped to `logicalName` (default: `'default'`).
35+
* Throws `ModelBackendNotFoundError` if no backend is mapped.
3336
*/
3437
export function resolveEmbedding(logicalName: string = 'default'): ModelBackend {
35-
return resolve('embedding', logicalName);
38+
const backend = embedding.get(logicalName);
39+
if (!backend) throw new ModelBackendNotFoundError('embedding', logicalName);
40+
return backend;
3641
}
3742

3843
/**
39-
* Resolve the backend configured as the generative provider for `logicalName`
40-
* (default: `'default'`). Throws if no config entry exists or the named
41-
* backend is not registered.
44+
* Resolve the generative backend mapped to `logicalName` (default: `'default'`).
45+
* Throws `ModelBackendNotFoundError` if no backend is mapped.
4246
*/
4347
export function resolveGenerative(logicalName: string = 'default'): ModelBackend {
44-
return resolve('generative', logicalName);
45-
}
46-
47-
/** Remove all registered backends. Test-only hygiene. */
48-
export function clearRegistry(): void {
49-
byName.clear();
50-
}
51-
52-
function resolve(kind: ModelKind, logicalName: string): ModelBackend {
53-
const backendName = envGet(`models.${kind}.${logicalName}.backend`);
54-
if (typeof backendName !== 'string' || !backendName) {
55-
throw new ModelBackendNotConfiguredError(kind, logicalName);
56-
}
57-
const backend = byName.get(backendName);
58-
if (!backend) throw new ModelBackendNotRegisteredError(kind, logicalName);
48+
const backend = generative.get(logicalName);
49+
if (!backend) throw new ModelBackendNotFoundError('generative', logicalName);
5950
return backend;
6051
}
6152

62-
export class ModelBackendNotConfiguredError extends ServerError {
63-
constructor(kind: ModelKind, logicalName: string) {
64-
super(`No '${kind}.${logicalName}.backend' configured`);
65-
this.name = 'ModelBackendNotConfiguredError';
66-
}
53+
/** Remove all registrations. Test-only hygiene. */
54+
export function clearRegistry(): void {
55+
embedding.clear();
56+
generative.clear();
6757
}
6858

69-
export class ModelBackendNotRegisteredError extends ServerError {
70-
// Deliberately does not name which backend was configured — avoids leaking
71-
// the set of registered backend identifiers in error responses.
59+
export class ModelBackendNotFoundError extends ServerError {
60+
// Message identifies the kind + logical name only; never enumerates other
61+
// registered names to avoid leaking the registry shape in error responses.
7262
constructor(kind: ModelKind, logicalName: string) {
73-
super(`Backend configured for '${kind}.${logicalName}' is not registered`);
74-
this.name = 'ModelBackendNotRegisteredError';
63+
super(`No backend registered for '${kind}.${logicalName}'`);
64+
this.name = 'ModelBackendNotFoundError';
7565
}
7666
}

unitTests/components/Scope.test.js

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
const { Scope, MissingDefaultFilesOptionError } = require('#src/components/Scope');
2+
const { Models } = require('#src/resources/models/Models');
23
const { EventEmitter } = require('node:events');
34
const assert = require('node:assert/strict');
45
const { join, basename } = require('node:path');
@@ -59,6 +60,14 @@ describe('Scope', () => {
5960
assert.ok(scope.options instanceof OptionsWatcher, 'Scope should have an OptionsWatcher instance');
6061
assert.ok(scope.resources instanceof Resources, 'Scope should have a resources property of type Map');
6162
assert.ok(scope.server !== undefined, 'Scope should have a server property');
63+
assert.ok(scope.models instanceof Models, 'Scope should expose a Models facade as scope.models');
64+
assert.strictEqual(typeof scope.models.embed, 'function', 'scope.models.embed should be callable');
65+
assert.strictEqual(typeof scope.models.generate, 'function', 'scope.models.generate should be callable');
66+
assert.strictEqual(
67+
typeof scope.models.generateStream,
68+
'function',
69+
'scope.models.generateStream should be callable'
70+
);
6271

6372
// Even though scope is ready, we haven't provided an entry handler yet so modifying a file matched by files option should not request a restart
6473
await writeFile(this.testFilePath, '"bar";');

0 commit comments

Comments
 (0)