fix: model MalformedResponseError

Author: nborges-aws

src/core/datasetDiff.test.ts

@@ -1,11 +1,10 @@
 import { describe, expect, test } from "bun:test";
-import { AgentCoreCLIError, InputValidationError } from "../errors";
+import { InputValidationError, MalformedServiceResponseError } from "../errors";
 import {
   applyExampleIds,
   diffExamples,
   indexRemoteById,
   parseJsonl,
-  stableStringify,
   stripExampleId,
 } from "./datasetDiff";
 
@@ -72,7 +71,7 @@ describe("indexRemoteById", () => {
   test("rejects a remote example without an id", () => {
     const remote = parseJsonl(jsonl({ scenario_id: "missing-id" }), "remote");
 
-    expect(() => indexRemoteById(remote)).toThrow(AgentCoreCLIError);
+    expect(() => indexRemoteById(remote)).toThrow(MalformedServiceResponseError);
     expect(() => indexRemoteById(remote)).toThrow(/missing a valid exampleId/);
     try {
       indexRemoteById(remote);
@@ -84,7 +83,7 @@ describe("indexRemoteById", () => {
   test("rejects duplicate remote ids instead of overwriting an example", () => {
     const remote = parseJsonl(jsonl(withId("duplicate"), withId("duplicate")), "remote");
 
-    expect(() => indexRemoteById(remote)).toThrow(AgentCoreCLIError);
+    expect(() => indexRemoteById(remote)).toThrow(MalformedServiceResponseError);
     expect(() => indexRemoteById(remote)).toThrow(/duplicate exampleId "duplicate"/);
   });
 });
@@ -111,10 +110,16 @@ describe("diffExamples", () => {
   // must not read as modified.
   test("treats key reordering as unchanged", () => {
     const remote = indexRemoteById([
-      { exampleId: "a", content: { exampleId: "a", alpha: 1, beta: 2 } },
+      {
+        exampleId: "a",
+        content: { exampleId: "a", alpha: 1, nested: { first: 1, second: 2 } },
+      },
     ]);
     const local: Parameters<typeof diffExamples>[0] = [
-      { exampleId: "a", content: { beta: 2, exampleId: "a", alpha: 1 } },
+      {
+        exampleId: "a",
+        content: { nested: { second: 2, first: 1 }, exampleId: "a", alpha: 1 },
+      },
     ];
 
     const diff = diffExamples(local, remote);
@@ -123,6 +128,20 @@ describe("diffExamples", () => {
     expect(diff.updates).toEqual([]);
   });
 
+  test("treats array reordering as an update", () => {
+    const remote = indexRemoteById([
+      { exampleId: "a", content: { exampleId: "a", values: [1, 2] } },
+    ]);
+    const local: Parameters<typeof diffExamples>[0] = [
+      { exampleId: "a", content: { exampleId: "a", values: [2, 1] } },
+    ];
+
+    const diff = diffExamples(local, remote);
+
+    expect(diff.unchanged).toBe(0);
+    expect(diff.updates).toEqual([{ exampleId: "a", values: [2, 1] }]);
+  });
+
   // An id absent remotely is stale (e.g. the dataset was recreated); re-adding is
   // recoverable, whereas failing would leave the file unusable.
   test("treats a stale exampleId as an addition and strips the id", () => {
@@ -227,7 +246,9 @@ describe("applyExampleIds", () => {
     const local = parseJsonl(jsonl({ scenario_id: "one" }, { scenario_id: "two" }), "file-path");
     const additions = diffExamples(local, new Map()).additions;
 
-    expect(() => applyExampleIds(local, additions, assignedIds)).toThrow(AgentCoreCLIError);
+    expect(() => applyExampleIds(local, additions, assignedIds)).toThrow(
+      MalformedServiceResponseError,
+    );
     expect(() => applyExampleIds(local, additions, assignedIds)).toThrow(
       /returned \d+ exampleIds for 2 additions/,
     );
@@ -237,7 +258,7 @@ describe("applyExampleIds", () => {
     const local = parseJsonl(jsonl({ scenario_id: "one" }), "file-path");
     const additions = diffExamples(local, new Map()).additions;
 
-    expect(() => applyExampleIds(local, additions, [""])).toThrow(AgentCoreCLIError);
+    expect(() => applyExampleIds(local, additions, [""])).toThrow(MalformedServiceResponseError);
     expect(() => applyExampleIds(local, additions, [""])).toThrow(/invalid exampleId/);
   });
 
@@ -246,32 +267,14 @@ describe("applyExampleIds", () => {
     const additions = diffExamples(local, new Map()).additions;
 
     expect(() => applyExampleIds(local, additions, ["same-id", "same-id"])).toThrow(
-      AgentCoreCLIError,
+      MalformedServiceResponseError,
     );
     expect(() => applyExampleIds(local, additions, ["same-id", "same-id"])).toThrow(
       /duplicate exampleId "same-id"/,
     );
   });
 });
 
-describe("stableStringify", () => {
-  test("sorts object keys at every depth", () => {
-    expect(stableStringify({ b: 1, a: { d: 2, c: 3 } })).toBe(
-      stableStringify({ a: { c: 3, d: 2 }, b: 1 }),
-    );
-  });
-
-  // Arrays are ordered data — reordering them is a real change, unlike key order.
-  test("preserves array order", () => {
-    expect(stableStringify([1, 2])).not.toBe(stableStringify([2, 1]));
-  });
-
-  test("handles null and primitives", () => {
-    expect(stableStringify(null)).toBe("null");
-    expect(stableStringify({ a: null })).toBe('{"a":null}');
-  });
-});
-
 describe("stripExampleId", () => {
   test("removes only exampleId", () => {
     expect(stripExampleId({ exampleId: "a", scenario_id: "x" })).toEqual({ scenario_id: "x" });

src/core/datasetDiff.ts

@@ -1,4 +1,5 @@
-import { AgentCoreCLIError, ERROR_SOURCE, InputValidationError } from "../errors";
+import { isDeepStrictEqual } from "node:util";
+import { InputValidationError, MalformedServiceResponseError } from "../errors";
 import { parseJsonObjectLines, type JsonObject } from "../io";
 
 // One dataset example as it appears in a JSONL file: an opaque JSON document,
@@ -49,28 +50,22 @@ export function indexRemoteById(examples: ParsedExample[]): Map<string, DatasetE
   examples.forEach((example, index) => {
     const id = example.exampleId;
     if (!id) {
-      throw cliServiceError(`Remote dataset example ${index + 1} is missing a valid exampleId`, {
-        index,
-      });
+      throw new MalformedServiceResponseError(
+        `Remote dataset example ${index + 1} is missing a valid exampleId`,
+        { meta: { index } },
+      );
     }
     if (byId.has(id)) {
-      throw cliServiceError(`Remote dataset contains duplicate exampleId "${id}"`, {
-        exampleId: id,
-        index,
-      });
+      throw new MalformedServiceResponseError(
+        `Remote dataset contains duplicate exampleId "${id}"`,
+        { meta: { exampleId: id, index } },
+      );
     }
     byId.set(id, example.content);
   });
   return byId;
 }
 
-function cliServiceError(message: string, meta: Record<string, unknown>): AgentCoreCLIError {
-  return new AgentCoreCLIError(message, {
-    source: ERROR_SOURCE.SERVICE,
-    meta,
-  });
-}
-
 function validateUniqueLocalIds(local: ParsedExample[]): void {
   const ids = new Set<string>();
   for (const example of local) {
@@ -106,8 +101,9 @@ export function diffExamples(
       return;
     }
     matched.add(example.exampleId!);
-    if (contentEquals(example.content, remoteContent)) unchanged++;
-    else updates.push(example.content);
+    if (isDeepStrictEqual(stripExampleId(example.content), stripExampleId(remoteContent))) {
+      unchanged++;
+    } else updates.push(example.content);
   });
 
   const deleteIds = [...remote.keys()].filter((id) => !matched.has(id));
@@ -121,26 +117,6 @@ export function stripExampleId(content: DatasetExample): DatasetExample {
   return rest;
 }
 
-// contentEquals compares two examples ignoring exampleId and key order. The
-// service does not preserve the key order examples were submitted in, so a plain
-// JSON.stringify comparison would report all round-trip examples as changed.
-function contentEquals(a: DatasetExample, b: DatasetExample): boolean {
-  return stableStringify(stripExampleId(a)) === stableStringify(stripExampleId(b));
-}
-
-// stableStringify serializes a JSON document with object keys sorted, so two
-// documents differing only in key order produce the same string.
-export function stableStringify(value: unknown): string {
-  if (value === null || typeof value !== "object") return JSON.stringify(value) ?? "null";
-  if (Array.isArray(value)) return `[${value.map(stableStringify).join(",")}]`;
-
-  const entries = Object.entries(value as Record<string, unknown>)
-    .filter(([, v]) => v !== undefined)
-    .sort(([a], [b]) => (a < b ? -1 : a > b ? 1 : 0))
-    .map(([k, v]) => `${JSON.stringify(k)}:${stableStringify(v)}`);
-  return `{${entries.join(",")}}`;
-}
-
 // applyExampleIds renders the local file with service-assigned ids attached to the
 // rows that were added, leaving every other row untouched. `assignedIds` is
 // positional: the nth id belongs to the nth addition. Without these ids the next
@@ -151,26 +127,26 @@ export function applyExampleIds(
   assignedIds: string[],
 ): string {
   if (assignedIds.length !== additions.length) {
-    throw cliServiceError(
+    throw new MalformedServiceResponseError(
       `Dataset service returned ${assignedIds.length} exampleIds for ` +
         `${additions.length} additions`,
-      { expected: additions.length, received: assignedIds.length },
+      { meta: { expected: additions.length, received: assignedIds.length } },
     );
   }
 
   const uniqueIds = new Set<string>();
   assignedIds.forEach((id, index) => {
     if (typeof id !== "string" || id.trim() === "") {
-      throw cliServiceError(
+      throw new MalformedServiceResponseError(
         `Dataset service returned an invalid exampleId for addition ${index + 1}`,
-        { index },
+        { meta: { index } },
       );
     }
     if (uniqueIds.has(id)) {
-      throw cliServiceError(`Dataset service returned duplicate exampleId "${id}"`, {
-        exampleId: id,
-        index,
-      });
+      throw new MalformedServiceResponseError(
+        `Dataset service returned duplicate exampleId "${id}"`,
+        { meta: { exampleId: id, index } },
+      );
     }
     uniqueIds.add(id);
   });

src/errors/errors.tsx

@@ -150,6 +150,13 @@ export class NetworkingError extends AgentCoreCLIError {
   }
 }
 
+/** Service data was returned successfully, but did not match the expected contract. */
+export class MalformedServiceResponseError extends AgentCoreCLIError {
+  constructor(message: string, options?: Omit<AgentCoreCLIErrorOptions, "source">) {
+    super(message, { ...options, source: ERROR_SOURCE.SERVICE });
+  }
+}
+
 /** A file could not be written locally: missing directory, permission denial, etc. */
 export class FileWriteError extends AgentCoreCLIError {
   constructor(message: string, options?: AgentCoreCLIErrorOptions) {

src/errors/index.tsx

@@ -5,6 +5,7 @@ export {
   FileWriteError,
   InputValidationError,
   InvalidEnvironmentError,
+  MalformedServiceResponseError,
   NestedProjectError,
   NetworkingError,
   NotImplementedError,