fix(conformance): address code-review findings — JSON robustness, navigate-grammar coverage, minors

dmealing · claude · dmealing · commit 591dd51e7091 · 2026-05-19T06:18:14.000-04:00
- Fix 1: add parseExpectedErrors() validated parser in operation-script.ts; use in runner.ts + fixture-lint.ts (no blind cast)
- Fix 2: wrap all fixture-file read+parse in runFixture with try/catch so malformed files produce a failed CheckResult instead of throwing
- Fix 3: recognize bracket segments (type[subType]) in fixture-lint segment validation; add navigator.test.ts covering bracket-path navigation to nameless nodes
- Fix 4: update namesIn doc comment to state it is an over-approximation collecting all name values
- Fix 5: add comment on error equality in resultsEqual stating code-only contract
- Fix 6: hoist const tree after outcome.tree !== undefined guards; eliminate outcome.tree! non-null assertions
- Fix 7: add comment on local FIELD_SUBTYPES in generator.ts noting it is an intentional subset of the metamodel's full list
- Fix 8: replace parseInt with Number()+Number.isInteger() in conformance CLI to reject "5abc"-style inputs
- Fix 9: guard ERROR-CODES.json shape in lint subcommand before Object.keys()
- Fix 10: add aggregator test asserting fixed-but-listed fixtures count in the Failing column

Co-Authored-By: Claude Sonnet 4.6 &lt;noreply@anthropic.com&gt;
diff --git a/typescript/packages/conformance/bin/conformance.ts b/typescript/packages/conformance/bin/conformance.ts
@@ -20,6 +20,17 @@ switch (subcommand) {
     const fixtures = await discoverFixtures(corpusRoot);
     const errorCodesRaw: unknown = JSON.parse(
       readFileSync(join(corpusRoot, "ERROR-CODES.json"), "utf8"));
+    // Fix 9: guard the shape — a missing or non-object `codes` key would cause
+    // Object.keys(undefined) to throw an opaque TypeError at runtime.
+    if (
+      typeof errorCodesRaw !== "object" ||
+      errorCodesRaw === null ||
+      typeof (errorCodesRaw as Record<string, unknown>).codes !== "object" ||
+      (errorCodesRaw as Record<string, unknown>).codes === null
+    ) {
+      console.error("lint: ERROR-CODES.json must be an object with a 'codes' object property");
+      process.exit(1);
+    }
     const errorCodes = Object.keys(
       (errorCodesRaw as { codes: Record<string, string> }).codes);
 
@@ -74,13 +85,15 @@ switch (subcommand) {
       console.error("Usage: conformance generate <corpusRoot> <count> [startSeed]");
       process.exit(1);
     }
-    const count = parseInt(countStr, 10);
-    if (isNaN(count) || count < 1) {
+    // Fix 8: use Number() + Number.isInteger() so "5abc" is rejected (parseInt
+    // would silently accept it as 5).
+    const count = Number(countStr);
+    if (!Number.isInteger(count) || count < 1) {
       console.error("generate: <count> must be a positive integer");
       process.exit(1);
     }
-    const startSeed = startSeedStr !== undefined ? parseInt(startSeedStr, 10) : 1;
-    if (isNaN(startSeed)) {
+    const startSeed = startSeedStr !== undefined ? Number(startSeedStr) : 1;
+    if (!Number.isInteger(startSeed)) {
       console.error("generate: [startSeed] must be an integer");
       process.exit(1);
     }
diff --git a/typescript/packages/conformance/src/fixture-lint.ts b/typescript/packages/conformance/src/fixture-lint.ts
@@ -3,12 +3,16 @@
 import { readFileSync } from "node:fs";
 import { join } from "node:path";
 import type { Fixture } from "./fixture.js";
-import { parseOperationScript } from "./operation-script.js";
+import { parseOperationScript, parseExpectedErrors } from "./operation-script.js";
 
 /**
- * Collect every node name that appears in a canonical expected.json — used to
- * cheaply check that a navigate path's terminal node exists in the resolved
- * tree. (A wrapper key is `type.subType`; the body's `name` is the node name.)
+ * Collect every `"name"` value that appears anywhere in a canonical
+ * expected.json — used as a cheap over-approximation when checking that a
+ * navigate path's colon-segment terminal exists.  Because this recurses into
+ * attr/relationship bodies as well as node bodies, it can produce false-
+ * negatives (a path may lint as clean even though the named node is absent),
+ * but it never produces false-positives (a truly present node name is always
+ * collected here).
  */
 function namesIn(value: unknown, acc: Set<string>): void {
   if (Array.isArray(value)) { for (const v of value) namesIn(v, acc); return; }
@@ -24,8 +28,15 @@ export function lintFixture(fix: Fixture, errorCodes: readonly string[]): string
   const problems: string[] = [];
 
   if (fix.hasExpectedErrors) {
-    const codes = JSON.parse(
-      readFileSync(join(fix.dir, "expected-errors.json"), "utf8")) as { code: string }[];
+    // Fix 1: use parseExpectedErrors for validated parsing instead of a blind cast.
+    let codes: { code: string }[];
+    try {
+      codes = parseExpectedErrors(
+        JSON.parse(readFileSync(join(fix.dir, "expected-errors.json"), "utf8")));
+    } catch (err) {
+      problems.push(`${fix.name}: malformed expected-errors.json — ${(err as Error).message}`);
+      return problems;
+    }
     for (const { code } of codes) {
       if (!errorCodes.includes(code)) {
         problems.push(`${fix.name}: unregistered error code '${code}'`);
@@ -45,16 +56,31 @@ export function lintFixture(fix: Fixture, errorCodes: readonly string[]): string
     if (fix.hasExpected) {
       const known = new Set<string>();
       namesIn(JSON.parse(readFileSync(join(fix.dir, "expected.json"), "utf8")), known);
+      // Fix 3a: recognize both segment grammars — `type:name` and `type[subType]`.
+      const COLON_SEG = /^[a-z][a-z0-9-]*:[^[]+$/;
+      const BRACKET_SEG = /^[a-z][a-z0-9-]*\[[a-zA-Z][a-zA-Z0-9-]*\]$/;
       for (const op of script.operations) {
         for (const segment of op.navigate) {
-          const colon = segment.indexOf(":");
-          if (colon !== -1) {
-            const nodeName = segment.slice(colon + 1);
-            if (!known.has(nodeName)) {
-              problems.push(
-                `${fix.name}: navigate segment '${segment}' names a node `
-                  + `absent from expected.json`);
-            }
+          if (BRACKET_SEG.test(segment)) {
+            // Bracket segments (`type[subType]`) address nameless nodes — we
+            // cannot cheaply verify existence against expected.json, so we
+            // accept any syntactically valid bracket segment without emitting a
+            // false "absent node" problem.
+            continue;
+          }
+          if (!COLON_SEG.test(segment)) {
+            // Neither colon nor bracket — malformed segment grammar.
+            problems.push(
+              `${fix.name}: navigate segment '${segment}' has malformed syntax `
+                + `(expected 'type:name' or 'type[subType]')`);
+            continue;
+          }
+          // Colon segment: check the named node exists in expected.json.
+          const nodeName = segment.slice(segment.indexOf(":") + 1);
+          if (!known.has(nodeName)) {
+            problems.push(
+              `${fix.name}: navigate segment '${segment}' names a node `
+                + `absent from expected.json`);
           }
         }
       }
diff --git a/typescript/packages/conformance/src/generator.ts b/typescript/packages/conformance/src/generator.ts
@@ -10,6 +10,11 @@ function mulberry32(seed: number): () => number {
   };
 }
 
+// Intentionally a small subset for fixture generation — @metaobjects/metadata
+// exports the full FIELD_SUBTYPES array (including currency, date, object, etc.)
+// which is too broad for deterministic round-trip fixtures.  Keep this list in
+// sync with the generator's intent: only "safe" primitive types that the TS
+// oracle handles without extra provider registration.
 const FIELD_SUBTYPES = ["string", "int", "long", "boolean"] as const;
 
 /** Generate a canonical-shaped metadata document deterministically from a seed. */
diff --git a/typescript/packages/conformance/src/operation-script.ts b/typescript/packages/conformance/src/operation-script.ts
@@ -2,6 +2,27 @@
 
 import type { NormalizedResult } from "./result.js";
 
+/**
+ * Parse + validate a parsed expected-errors.json value.
+ * Throws a clear Error if `raw` is not an array of objects each with a string
+ * `code` — mirrors the validation style of `parseOperationScript`.
+ */
+export function parseExpectedErrors(raw: unknown): { code: string }[] {
+  if (!Array.isArray(raw)) {
+    throw new Error("expected-errors.json must be a JSON array");
+  }
+  return raw.map((item, i) => {
+    if (typeof item !== "object" || item === null) {
+      throw new Error(`expected-errors.json entry ${i} is not an object`);
+    }
+    const { code } = item as Record<string, unknown>;
+    if (typeof code !== "string") {
+      throw new Error(`expected-errors.json entry ${i} missing string 'code' field`);
+    }
+    return { code };
+  });
+}
+
 /** capability-id grammar: `<type>.<capability>`, both kebab-case. */
 const CAPABILITY_ID = /^[a-z][a-z0-9-]*\.[a-z][a-z0-9-]*$/;
 
diff --git a/typescript/packages/conformance/src/result.ts b/typescript/packages/conformance/src/result.ts
@@ -23,6 +23,8 @@ export function resultsEqual(a: NormalizedResult, b: NormalizedResult): boolean
   if ("effective-tree" in a && "effective-tree" in b) {
     return a["effective-tree"] === b["effective-tree"];
   }
+  // Error equality is by `code` ONLY — message and path are intentionally not
+  // part of the cross-language contract, so implementations may vary freely.
   if ("error" in a && "error" in b) return a.error.code === b.error.code;
   return false;
 }
diff --git a/typescript/packages/conformance/src/runner.ts b/typescript/packages/conformance/src/runner.ts
@@ -5,7 +5,7 @@ import { join } from "node:path";
 import type { Fixture } from "./fixture.js";
 import type { ConformanceAdapter } from "./adapter.js";
 import { UnknownCapabilityError } from "./adapter.js";
-import { parseOperationScript } from "./operation-script.js";
+import { parseOperationScript, parseExpectedErrors } from "./operation-script.js";
 import { resultsEqual, type NormalizedResult } from "./result.js";
 import type { CheckResult, FixtureReport } from "./report.js";
 
@@ -23,16 +23,29 @@ export async function runFixture(
   const outcome = await adapter.loadFixture(fix.inputDir, fix.providers);
 
   if (fix.hasExpectedErrors) {
-    const expected = (await readJson(join(fix.dir, "expected-errors.json"))) as
-      { code: string }[];
-    const want = expected.map((e) => e.code).sort();
-    const got = [...outcome.errorCodes].sort();
-    const passed = want.length === got.length && want.every((c, i) => c === got[i]);
-    checks.push({
-      kind: "expected-errors",
-      passed,
-      ...(passed ? {} : { detail: `expected [${want}], got [${got}]` }),
-    });
+    // Fix 1: use parseExpectedErrors — throws a clear Error on malformed input.
+    // Fix 2: wrap the read+parse so a bad file produces a failed check, not a throw.
+    let want: string[] | undefined;
+    try {
+      want = parseExpectedErrors(await readJson(join(fix.dir, "expected-errors.json")))
+        .map((e) => e.code)
+        .sort();
+    } catch (err) {
+      checks.push({
+        kind: "expected-errors",
+        passed: false,
+        detail: `expected-errors.json parse error: ${(err as Error).message}`,
+      });
+    }
+    if (want !== undefined) {
+      const got = [...outcome.errorCodes].sort();
+      const passed = want.length === got.length && want.every((c, i) => c === got[i]);
+      checks.push({
+        kind: "expected-errors",
+        passed,
+        ...(passed ? {} : { detail: `expected [${want}], got [${got}]` }),
+      });
+    }
   }
 
   // If the load produced no tree but tree-dependent checks are expected, push a
@@ -48,60 +61,102 @@ export async function runFixture(
     });
   }
 
+  // Fix 6: hoist tree after the undefined guard so tree-dependent blocks never
+  // need outcome.tree! non-null assertions.
   if (fix.hasExpected && outcome.tree !== undefined) {
-    const want = (await readFile(join(fix.dir, "expected.json"), "utf8")).trim();
-    const got = adapter.canonicalSerialize(outcome.tree).trim();
-    const passed = want === got;
-    checks.push({
-      kind: "expected",
-      passed,
-      ...(passed ? {} : { detail: "canonical serialization mismatch" }),
-    });
+    const tree = outcome.tree;
+    let want: string | undefined;
+    try {
+      // Fix 2: wrap file read so parse errors become failed checks, not throws.
+      want = (await readFile(join(fix.dir, "expected.json"), "utf8")).trim();
+    } catch (err) {
+      checks.push({
+        kind: "expected",
+        passed: false,
+        detail: `expected.json read error: ${(err as Error).message}`,
+      });
+    }
+    if (want !== undefined) {
+      const got = adapter.canonicalSerialize(tree).trim();
+      const passed = want === got;
+      checks.push({
+        kind: "expected",
+        passed,
+        ...(passed ? {} : { detail: "canonical serialization mismatch" }),
+      });
+    }
   }
 
   if (fix.hasExpectedEffective && outcome.tree !== undefined) {
-    const want = (await readFile(join(fix.dir, "expected-effective.json"), "utf8")).trim();
-    const got = adapter.canonicalSerializeEffective(outcome.tree).trim();
-    const passed = want === got;
-    checks.push({
-      kind: "expected-effective",
-      passed,
-      ...(passed ? {} : { detail: "effective serialization mismatch" }),
-    });
+    const tree = outcome.tree;
+    let want: string | undefined;
+    try {
+      // Fix 2: wrap file read so parse errors become failed checks, not throws.
+      want = (await readFile(join(fix.dir, "expected-effective.json"), "utf8")).trim();
+    } catch (err) {
+      checks.push({
+        kind: "expected-effective",
+        passed: false,
+        detail: `expected-effective.json read error: ${(err as Error).message}`,
+      });
+    }
+    if (want !== undefined) {
+      const got = adapter.canonicalSerializeEffective(tree).trim();
+      const passed = want === got;
+      checks.push({
+        kind: "expected-effective",
+        passed,
+        ...(passed ? {} : { detail: "effective serialization mismatch" }),
+      });
+    }
   }
 
   if (fix.hasScript && outcome.tree !== undefined) {
-    const script = parseOperationScript(await readJson(join(fix.dir, "script.json")));
-    script.operations.forEach((op, i) => {
-      if (!capabilities.includes(op.invoke)) capabilities.push(op.invoke);
-      const node = adapter.navigate(outcome.tree!, op.navigate);
-      if (node === undefined) {
+    const tree = outcome.tree;
+    let script: ReturnType<typeof parseOperationScript> | undefined;
+    try {
+      // Fix 2: wrap script parse so malformed script.json produces a failed check.
+      script = parseOperationScript(await readJson(join(fix.dir, "script.json")));
+    } catch (err) {
+      checks.push({
+        kind: "operation",
+        operationIndex: 0,
+        passed: false,
+        detail: `script.json parse error: ${(err as Error).message}`,
+      });
+    }
+    if (script !== undefined) {
+      script.operations.forEach((op, i) => {
+        if (!capabilities.includes(op.invoke)) capabilities.push(op.invoke);
+        const node = adapter.navigate(tree, op.navigate);
+        if (node === undefined) {
+          checks.push({
+            kind: "operation",
+            operationIndex: i,
+            passed: false,
+            detail: `navigate [${op.navigate}] did not resolve`,
+          });
+          return;
+        }
+        let actual: NormalizedResult;
+        try {
+          actual = adapter.invoke(node, op.invoke, op.args ?? {});
+        } catch (err) {
+          const detail = err instanceof UnknownCapabilityError
+            ? `unbound capability '${op.invoke}'`
+            : `invoke threw: ${(err as Error).message}`;
+          checks.push({ kind: "operation", operationIndex: i, passed: false, detail });
+          return;
+        }
+        const passed = resultsEqual(actual, op.expect);
         checks.push({
           kind: "operation",
           operationIndex: i,
-          passed: false,
-          detail: `navigate [${op.navigate}] did not resolve`,
+          passed,
+          ...(passed ? {} : { detail: `expected ${JSON.stringify(op.expect)}, got ${JSON.stringify(actual)}` }),
         });
-        return;
-      }
-      let actual: NormalizedResult;
-      try {
-        actual = adapter.invoke(node, op.invoke, op.args ?? {});
-      } catch (err) {
-        const detail = err instanceof UnknownCapabilityError
-          ? `unbound capability '${op.invoke}'`
-          : `invoke threw: ${(err as Error).message}`;
-        checks.push({ kind: "operation", operationIndex: i, passed: false, detail });
-        return;
-      }
-      const passed = resultsEqual(actual, op.expect);
-      checks.push({
-        kind: "operation",
-        operationIndex: i,
-        passed,
-        ...(passed ? {} : { detail: `expected ${JSON.stringify(op.expect)}, got ${JSON.stringify(actual)}` }),
       });
-    });
+    }
   }
 
   // A fixture with no expectation files at all is a configuration error — push an
diff --git a/typescript/packages/conformance/test/aggregator.test.ts b/typescript/packages/conformance/test/aggregator.test.ts
@@ -13,3 +13,21 @@ test("dashboard reports per-language pass and known-gap counts", () => {
   expect(md).toMatch(/java/);
   expect(md).toMatch(/known-gap|known gap/i);
 });
+
+// Fix 10: renderDashboard buckets `fixed-but-listed` into the Failing column
+// alongside `fail`; verify this disjunct is actually exercised.
+test("dashboard counts fixed-but-listed fixtures in the Failing column", () => {
+  const report = emptyReport("rust");
+  report.fixtures.push({ name: "x", checks: [], status: "pass" });
+  report.fixtures.push({ name: "y", checks: [], status: "fixed-but-listed" });
+  report.fixtures.push({ name: "z", checks: [], status: "fixed-but-listed" });
+  const md = renderDashboard([report]);
+  // 1 pass out of 3 total → "1/3 (33%)" in the Passing column
+  expect(md).toContain("1/3 (33%)");
+  // 2 fixed-but-listed → Failing column shows 2
+  const row = md.split("\n").find((line) => line.includes("rust"))!;
+  expect(row).toBeDefined();
+  // The failing count is the last pipe-delimited cell in the row
+  const cells = row.split("|").map((s) => s.trim());
+  expect(cells.at(-2)).toBe("2");
+});
diff --git a/typescript/packages/conformance/test/fixture-lint.test.ts b/typescript/packages/conformance/test/fixture-lint.test.ts
diff --git a/typescript/packages/conformance/test/runner.test.ts b/typescript/packages/conformance/test/runner.test.ts
diff --git a/typescript/packages/metadata/test/conformance/navigator.test.ts b/typescript/packages/metadata/test/conformance/navigator.test.ts

Original file line number	Diff line number	Diff line change
`@@ -23,6 +23,8 @@ export function resultsEqual(a: NormalizedResult, b: NormalizedResult): boolean`
`23`	`23`	`if ("effective-tree" in a && "effective-tree" in b) {`
`24`	`24`	`return a["effective-tree"] === b["effective-tree"];`
`25`	`25`	`}`
	`26`	+ // Error equality is by `code` ONLY — message and path are intentionally not
	`27`	`+ // part of the cross-language contract, so implementations may vary freely.`
`26`	`28`	`if ("error" in a && "error" in b) return a.error.code === b.error.code;`
`27`	`29`	`return false;`
`28`	`30`	`}`