Skip to content

Commit 7bf217e

Browse files
committed
chore(storage): pre-merge polish from final review (easy minors)
Six low-risk follow-ups from the final pre-merge review. Nothing here changes behaviour beyond error wording and JSDoc; the public API surface tightens slightly. #4 Hide cursor format internals from the public API. `common.ts` switches the Cursor module from `export *` to a selective re-export of `PageCursor`, `encodeCursor`, `decodeCursor`, `assertCursorMatches`, and `MAX_CURSOR_LENGTH`. `CursorPayload` (the wire shape) and `CURSOR_VERSION` (the format integer) are now internal-only — callers can't depend on the encoding format, which keeps it free to evolve. The unit test inlines the version constant. #6 ScopedTabularStorage emits `query` from `getPage`/`queryPage`. Cache invalidators that listen for tabular reads were missing page reads. Switching a caller from `query` to `getPage` would silently invalidate their invalidation logic. The emitted criteria is the user-facing criteria (without our injected `kb_id`). #7 Document `pages()` defensive `.slice()`. The async generator yields a fresh copy of `Page.items` per page so callers can't poison subsequent iterations by mutating the yielded array. JSDoc now says so. #8 Cursor JSON-roundtrip test. Pins the contract that cursors are safe to persist and re-load across process boundaries (URLs, durable queues, etc). The codec is pure so this should always work, but a one-line test locks it in. #10 Hoist the `jsToSqlValue` coupling note. `ITabularStorage.query` JSDoc now spells out the third-party-backend implementation contract: bind SearchCondition values through the same conversion path as row values going *into* the store, so the cursor pagination machinery's round-trip comparison works. #11 More actionable `StorageInvalidLimitError` message. Says "must be a positive integer, got X" instead of "must be greater than 0, got X" so a user staring at `limit: 1.5` knows fractional values are the problem. 914 vitest storage / 934 bun-native / 22 scoped storage / 322 queue tests pass. Pre-existing build issues on `chrome-ai` (from main's `bootstrapWorkglow` refactor) and the `KnowledgeBaseRegistry` `registerInputResolver` arity mismatch are unaffected and unchanged.
1 parent 8cc66a2 commit 7bf217e

6 files changed

Lines changed: 52 additions & 4 deletions

File tree

packages/knowledge-base/src/knowledge-base/ScopedTabularStorage.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,12 @@ export class ScopedTabularStorage<
183183
const scopedCriteria = { ...((criteria ?? {}) as any), kb_id: this.kbId };
184184
const innerPage = await this.inner.queryPage(scopedCriteria, request as any);
185185
const items = innerPage.items.map((row: any) => this.strip(row)) as Entity[];
186+
// Emit `query` for parity with `ScopedTabularStorage.query`. Cache
187+
// invalidators that listen for tabular reads need to see page reads
188+
// too; otherwise switching a caller from `query` to `getPage` would
189+
// silently invalidate their invalidation logic. The emitted criteria
190+
// is the *user-facing* criteria (without our injected `kb_id`).
191+
this.events.emit("query", (criteria ?? {}) as Partial<Entity>, items);
186192
return { items, nextCursor: innerPage.nextCursor };
187193
}
188194

packages/storage/src/common.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,13 @@ export type {
1414
PickCoveringIndexInput,
1515
PickedIndex,
1616
} from "./tabular/coveringIndexPicker";
17-
export * from "./tabular/Cursor";
17+
// Cursor codec: the public surface is the opaque type, the codec
18+
// functions, the length cap, and the cross-arity assertion. The
19+
// payload shape and version constant are implementation details —
20+
// keep them out of `@workglow/storage`'s public API so callers can't
21+
// depend on the encoding format and we stay free to evolve it.
22+
export { encodeCursor, decodeCursor, assertCursorMatches, MAX_CURSOR_LENGTH } from "./tabular/Cursor";
23+
export type { PageCursor } from "./tabular/Cursor";
1824
export * from "./tabular/HuggingFaceTabularStorage";
1925
export * from "./tabular/InMemoryTabularStorage";
2026
export * from "./tabular/ITabularStorage";

packages/storage/src/tabular/BaseTabularStorage.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -463,6 +463,9 @@ export abstract class BaseTabularStorage<
463463
* Async generator that yields pages of records.
464464
*
465465
* Backed by cursor-based pagination — stable under concurrent writes.
466+
* Each yielded array is a fresh copy of `Page.items`; mutating the
467+
* yielded array won't affect the underlying storage or subsequent
468+
* pages, at the cost of one allocation per page.
466469
*
467470
* @param pageSize - Number of records per page (default: 100)
468471
*/

packages/storage/src/tabular/ITabularStorage.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -360,6 +360,15 @@ export interface ITabularStorage<
360360
* Queries entries matching the specified search criteria with optional ordering, limit, and offset.
361361
* Uses optimized index paths when possible, falls back to full scan otherwise.
362362
*
363+
* Implementation contract for third-party backends: when binding a
364+
* `SearchCondition` value into the underlying datastore, run it
365+
* through the same conversion path as a row value going *into* the
366+
* store (e.g. `jsToSqlValue` for SQL backends — Date → ISO string,
367+
* etc.). The cursor pagination machinery in {@link getPage} relies
368+
* on this round-trip to compare a row's stored representation
369+
* against a cursor's decoded value; any backend that skips the
370+
* conversion would silently mis-page on Date or other rich types.
371+
*
363372
* @param criteria - Object with column names as keys and values or SearchConditions
364373
* @param options - Optional ordering, limit, and offset options
365374
* @returns Array of matching entities or undefined if no matches found

packages/storage/src/tabular/StorageError.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,12 @@ export class StorageEmptyCriteriaError extends StorageValidationError {
2424
export class StorageInvalidLimitError extends StorageValidationError {
2525
static override readonly type: string = "StorageInvalidLimitError";
2626
constructor(limit: number) {
27-
super(`Query limit must be greater than 0, got ${limit}`);
27+
// Message names both constraints (positive AND integer) so callers
28+
// hitting the error from `runPage` (which rejects non-integer limits)
29+
// see the same wording as offset paths — and a user staring at
30+
// `limit: 1.5` isn't left guessing that fractional values are the
31+
// problem.
32+
super(`Query limit must be a positive integer, got ${limit}`);
2833
}
2934
}
3035

packages/test/src/test/storage-tabular/Cursor.test.ts

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,17 @@
66

77
import { describe, expect, it } from "vitest";
88
import {
9-
CURSOR_VERSION,
109
MAX_CURSOR_LENGTH,
1110
decodeCursor,
1211
encodeCursor,
1312
StorageValidationError,
1413
} from "@workglow/storage";
1514

15+
// Pinned in the unit test rather than imported from `@workglow/storage`:
16+
// the cursor format version is an implementation detail of the codec
17+
// (kept out of the package's public API to leave room for evolution).
18+
const CURSOR_FORMAT_VERSION = 1;
19+
1620
describe("Cursor codec", () => {
1721
it("round-trips a payload through encode → decode", () => {
1822
const payload = {
@@ -104,7 +108,7 @@ describe("Cursor codec", () => {
104108
.replace(/=+$/, "");
105109
expect(() => decodeCursor(future)).toThrow(StorageValidationError);
106110
expect(() => decodeCursor(future)).toThrow(
107-
new RegExp(`expected v${CURSOR_VERSION}`)
111+
new RegExp(`expected v${CURSOR_FORMAT_VERSION}`)
108112
);
109113
});
110114

@@ -125,4 +129,19 @@ describe("Cursor codec", () => {
125129
const cursor = encodeCursor({ v: 1, n: ["x", "y"], c: [null, "z"] });
126130
expect(decodeCursor(cursor)).toEqual({ v: 1, n: ["x", "y"], c: [null, "z"] });
127131
});
132+
133+
it("survives a JSON.stringify → JSON.parse round-trip", () => {
134+
// Cursors are designed to be safe to persist and re-load across
135+
// process boundaries (URLs, durable queues, etc). The codec is pure
136+
// — no in-memory state — so transport via JSON should be lossless.
137+
// Pin that contract here.
138+
const original = encodeCursor({
139+
v: 1,
140+
n: ["createdAt", "id"],
141+
c: ["2026-01-02T03:04:05.000Z", "abc"],
142+
});
143+
const transported = JSON.parse(JSON.stringify({ cursor: original })).cursor;
144+
expect(transported).toBe(original);
145+
expect(decodeCursor(transported)).toEqual(decodeCursor(original));
146+
});
128147
});

0 commit comments

Comments
 (0)