History

Address review: classifier coverage, write bookkeeping, equality bound

Rhys Sullivan committed 2 months ago

Commit a30e4331 parent 27bd4196 files · +98 −10Browse files

Unified · Split

6 files
  1. packages/core/sdk/src/shape-inference.test.ts +10 −0
  2. packages/core/sdk/src/shape-inference.ts +17 −1
  3. packages/core/sdk/src/shape-memory.test.ts +33 −3
  4. packages/core/sdk/src/shape-memory.ts +9 −2
  5. packages/core/sdk/src/tool-result-normalization.test.ts +16 −0
  6. packages/core/sdk/src/tool-result-normalization.ts +13 −4
packages/core/sdk/src/shape-inference.test.ts+10−0
@@ -57,6 +57,10 @@ describe("inferShape", () => { · 56 unmodified lines
5757 { "12345": { qty: 2 }, "67890": { qty: 1 } },
5858 { "https://example.com/page": 3 },
5959 { deadbeefdeadbeef00: true },
60+ { U012ABCDEF: { presence: "active" } },
61+ { cus_9s6XKzkNRiz8i3: { plan: "pro" } },
62+ { "10.0.0.7": "reachable" },
63+ { sk4bcD3fGh1jKlMnOpQr: true },
6064 ];
6165 for (const value of cases) {
6266 const shape = inferShape(value);
@@ -71,13 +75,19 @@ describe("inferShape", () => { · 8 unmodified lines
7175 created_at: "2026-01-01",
7276 pageUrl: "https://x",
7377 email2fa: true,
78+ "@odata.context": "ctx",
79+ organizationMembershipSettings: {},
80+ sha256Fingerprint: "…",
7481 });
7582 expect(shape.properties).toBeDefined();
7683 expect(Object.keys(shape.properties ?? {}).sort()).toEqual([
84+ "@odata.context",
7785 "created_at",
7886 "email2fa",
7987 "id",
88+ "organizationMembershipSettings",
8089 "pageUrl",
90+ "sha256Fingerprint",
8191 ]);
8292 });
8393
packages/core/sdk/src/shape-inference.ts+17−1
@@ -55,12 +55,28 @@ const isUnknown = (shape: InferredShape): boolean => · 54 unmodified lines
5555 * collapsing.
5656 */
5757 const DATA_KEY_PATTERNS: readonly RegExp[] = [
58− /@/,
58+ // Email addresses (full-string — `@odata.context`-style annotation keys are
59+ // legitimate API surface and must NOT collapse).
60+ /^[^\s@]+@[^\s@]+\.[^\s@]+$/,
61+ // UUIDs.
5962 /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i,
63+ // Timestamps / dates.
6064 /^\d{4}-\d{2}-\d{2}/,
65+ // Bare numbers and IPv4 addresses.
6166 /^\d+$/,
67+ /^\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}$/,
68+ // URLs.
6269 /^https?:\/\//,
70+ // Long hex tokens (hashes, ids).
6371 /^[0-9a-f]{16,}$/i,
72+ // Platform opaque ids: Slack-style ALL-CAPS ids (U012ABCDEF), and
73+ // prefix_body ids (cus_..., price_..., asst_...). Real field names in
74+ // snake_case are lowercase words, not lowercase prefix + mixed-case body.
75+ /^[A-Z][A-Z0-9]{8,}$/,
76+ /^[a-z]{1,6}_(?=.*[A-Z0-9])[A-Za-z0-9]{10,}$/,
77+ // Generic digit-bearing opaque tokens (API keys, base62/base64 ids). Long
78+ // camelCase field names rarely contain digits at this length.
79+ /^(?=.*\d)[A-Za-z0-9+/=_-]{20,}$/,
6480 ];
6581
6682 const looksLikeDataKey = (key: string): boolean =>
packages/core/sdk/src/shape-memory.test.ts+33−3
@@ -27,6 +27,7 @@ const makeStubStorage = () => { · 26 unmodified lines
2727 updatedAt: new Date(0),
2828 };
2929 };
30+ let failNextWrites = 0;
3031 const unsupported = (member: string) => () =>
3132 Effect.die(`stub storage does not implement ${member}`);
3233 const storage: PluginStorageFacade = {
@@ -43,16 +44,27 @@ const makeStubStorage = () => { · 10 unmodified lines
4344 getForOwner: (input) => Effect.sync(() => entryFor(input.key)),
4445 list: unsupported("list"),
4546 put: (input) =>
46− Effect.sync(() => {
47+ Effect.suspend(() => {
48+ if (failNextWrites > 0) {
49+ failNextWrites -= 1;
50+ return Effect.fail({ _tag: "StorageError" as const }) as never;
51+ }
4752 writes += 1;
4853 rows.set(input.key, input.data);
49− return entryFor(input.key) as never;
54+ return Effect.sync(() => entryFor(input.key) as never);
5055 }),
5156 putMany: unsupported("putMany"),
5257 remove: unsupported("remove"),
5358 removeMany: unsupported("removeMany"),
5459 };
55− return { storage, rows, writeCount: () => writes };
60+ return {
61+ storage,
62+ rows,
63+ writeCount: () => writes,
64+ failWrites: (count: number) => {
65+ failNextWrites = count;
66+ },
67+ };
5668 };
5769
5870 const HOUR = 60 * 60 * 1000;
@@ -120,6 +132,24 @@ describe("makeShapeMemory", () => { · 61 unmodified lines
120132 }),
121133 );
122134
135+ it.effect("retries after a failed write instead of pretending it persisted", () =>
136+ Effect.gen(function* () {
137+ const stub = makeStubStorage();
138+ const memory = makeShapeMemory(stub.storage);
139+
140+ stub.failWrites(1);
141+ yield* memory.observe(ADDRESS, OWNER, "direct", { id: 1 });
142+ expect(stub.rows.has(ADDRESS), "the failed write stored nothing").toBe(false);
143+
144+ // The very next observation retries — no waiting out the freshness
145+ // interval on bookkeeping that lied about persisting.
146+ yield* memory.observe(ADDRESS, OWNER, "direct", { id: 2 });
147+ expect(stub.rows.has(ADDRESS), "the retry persisted").toBe(true);
148+ const stored = stub.rows.get(ADDRESS) as { observations: number };
149+ expect(stored.observations).toBe(2);
150+ }),
151+ );
152+
123153 it.effect("treats legacy records without a contract field as direct", () =>
124154 Effect.gen(function* () {
125155 const stub = makeStubStorage();
packages/core/sdk/src/shape-memory.ts+9−2
@@ -125,9 +125,16 @@ export const makeShapeMemory = (storage: PluginStorageFacade): ShapeMemory => { · 124 unmodified lines
125125 next.observations % OBSERVATION_WRITE_MILESTONE === 0 ||
126126 now - lastWrite >= FRESHNESS_WRITE_INTERVAL_MS;
127127 if (!shouldWrite) return;
128− yield* storage
128+ // Advance the persisted-state bookkeeping only on a successful write:
129+ // otherwise a transient storage failure would silence retries for the
130+ // whole freshness interval while nothing is actually stored.
131+ const wrote = yield* storage
129132 .put({ owner, collection: COLLECTION, key: address, data: next })
130− .pipe(Effect.catch(() => Effect.succeed(null)));
133+ .pipe(
134+ Effect.map(() => true),
135+ Effect.catch(() => Effect.succeed(false)),
136+ );
137+ if (!wrote) return;
131138 persistedSchema.set(key, schemaJson);
132139 persistedAt.set(key, now);
133140 }).pipe(Effect.catchCause(() => Effect.void));
packages/core/sdk/src/tool-result-normalization.test.ts+16−0
@@ -86,6 +86,22 @@ describe("normalizeMcpCallToolResult", () => { · 85 unmodified lines
8686 expect(normalized.meta).toEqual({ "io.modelcontextprotocol/serverInfo": { name: "s" } });
8787 });
8888
89+ it("survives adversarially deep valid JSON without blowing the stack", () => {
90+ const deep = "[".repeat(50_000) + "]".repeat(50_000);
91+ // Deep duplicate-detection degrades to "not a duplicate" (block kept);
92+ // it must never throw from inside the invocation path.
93+ const normalized = normalizeMcpCallToolResult({
94+ content: [text(deep)],
95+ structuredContent: { safe: true },
96+ });
97+ expect(normalized.data).toEqual({ safe: true });
98+ expect(normalized.content).toEqual([text(deep)]);
99+ // The lone-text parse path is equally safe: a stack-exceeding parse is
100+ // simply "not JSON" and the text stays a string.
101+ const lone = normalizeMcpCallToolResult({ content: [text(deep)] });
102+ expect(typeof lone.data === "string" || Array.isArray(lone.data)).toBe(true);
103+ });
104+
89105 it("passes non-envelope values through untouched", () => {
90106 expect(normalizeMcpCallToolResult({ rows: [1] }).data).toEqual({ rows: [1] });
91107 expect(normalizeMcpCallToolResult("plain").data).toBe("plain");
packages/core/sdk/src/tool-result-normalization.ts+13−4
@@ -77,21 +77,30 @@ const parseJsonPayload = (text: string): unknown | undefined => { · 76 unmodified lines
7777 }
7878 };
7979
80+/** Depth bound for duplicate detection. Beyond it two values are treated as
81+ * DIFFERENT — the safe direction: the block is kept rather than dropped —
82+ * and, critically, an adversarially deep (but valid) upstream JSON cannot
83+ * blow the stack inside the invocation path. */
84+const MAX_EQUALITY_DEPTH = 64;
85+
8086 /** Structural equality against the parsed form of a text block, used to
8187 * suppress the spec-mandated serialized duplicate of `structuredContent`.
82− * Key-order insensitive; bounded by the same size guard as payload parsing. */
83−const structurallyEquals = (left: unknown, right: unknown): boolean => {
88+ * Key-order insensitive; depth-bounded. */
89+const structurallyEquals = (left: unknown, right: unknown, depth = 0): boolean => {
8490 if (Object.is(left, right)) return true;
91+ if (depth >= MAX_EQUALITY_DEPTH) return false;
8592 if (Array.isArray(left) && Array.isArray(right)) {
8693 return (
8794 left.length === right.length &&
88− left.every((item, index) => structurallyEquals(item, right[index]))
95+ left.every((item, index) => structurallyEquals(item, right[index], depth + 1))
8996 );
9097 }
9198 if (isRecord(left) && isRecord(right)) {
9299 const leftKeys = Object.keys(left);
93100 if (leftKeys.length !== Object.keys(right).length) return false;
94− return leftKeys.every((key) => key in right && structurallyEquals(left[key], right[key]));
101+ return leftKeys.every(
102+ (key) => key in right && structurallyEquals(left[key], right[key], depth + 1),
103+ );
95104 }
96105 return false;
97106 };