From 3deb0ca060d5eb3944d143e3f1501ca2ac4c99a0 Mon Sep 17 00:00:00 2001 From: Lann Martin Date: Sat, 22 Aug 2026 23:20:42 -0400 Subject: [PATCH] harness: validate actual result shape against declared arity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit assert_return with an empty expected list never inspected the actual value: compareValues short-circuited to a vacuous pass, and invoke collapsed the raw return by declared arity, discarding it for arity 0 and casting unchecked for 2+. A spurious result from an arity-0 export passed silently. Extract the collapse into collapseResultsByArity (value-mapping.ts), which validates the observed shape against the declared arity per the runtime's convention (resultsToHost, runtime/src/exec/boundary.ts): arity 0 requires undefined, arity 1 requires non-undefined (a ComponentValue is never undefined; arrays stay legal — a single list result IS an array, the deliberate deviation from the issue text), and arity n>=2 requires an array of exactly n. compareValues now rejects a non-undefined actual when nothing is expected. Error messages use a bigint-safe describe helper (JSON.stringify throws on lifted i64s). Unit coverage in harness/tests/value_mapping_test.ts; the arity-0 spurious-result path is untestable end-to-end (it requires a broken runtime), which is why the collapse is a pure exported function. Fixes #188 --- harness/src/runtime-executor.ts | 7 ++- harness/src/value-mapping.ts | 75 ++++++++++++++++++++++- harness/tests/value_mapping_test.ts | 93 +++++++++++++++++++++++++++++ 3 files changed, 171 insertions(+), 4 deletions(-) create mode 100644 harness/tests/value_mapping_test.ts diff --git a/harness/src/runtime-executor.ts b/harness/src/runtime-executor.ts index 75e79c1..e6d3ec7 100644 --- a/harness/src/runtime-executor.ts +++ b/harness/src/runtime-executor.ts @@ -34,7 +34,7 @@ import { TrapError, } from "./executor.ts"; import type { Value } from "./schema.ts"; -import { toComponentValue } from "./value-mapping.ts"; +import { collapseResultsByArity, toComponentValue } from "./value-mapping.ts"; /** Substrings that indicate the command needs a not-yet-built runtime * capability (async/streams/etc, M2) rather than a genuine bug. Checked @@ -274,8 +274,9 @@ export class RuntimeExecutor implements CommandExecutor { } throw e; } - const arity = ref.arities.get(field) ?? (raw === undefined ? 0 : 1); - const values = arity === 0 ? [] : arity === 1 ? [raw] : (raw as unknown[]); + // Validates the observed shape against the declared arity and throws + // loudly on mismatch instead of discarding/coercing (issue #188). + const values = collapseResultsByArity(raw, ref.arities.get(field), field); return { kind: "returned", values }; } diff --git a/harness/src/value-mapping.ts b/harness/src/value-mapping.ts index 10e68a7..a15d6cc 100644 --- a/harness/src/value-mapping.ts +++ b/harness/src/value-mapping.ts @@ -368,6 +368,74 @@ export function compareValue( } } +/** Bigint-safe JSON.stringify substitute for error/diagnostic messages: + * plain JSON.stringify throws on bigint (lifted i64 results are bigints). + * Maps bigint -> "n" and falls back to String(v) if stringify still + * throws for some other reason (e.g. cyclic values). */ +export function describeValue(v: unknown): string { + try { + return JSON.stringify( + v, + (_k, val) => typeof val === "bigint" ? `${val}n` : val, + ) ?? String(v); + } catch { + return String(v); + } +} + +/** Collapses a raw invoke() return by the export's *declared* arity + * (from the plan's FuncType.results.length, see computeExportArities in + * runtime-executor.ts), validating the observed shape against it per the + * runtime's arity convention (`resultsToHost` in + * runtime/src/exec/boundary.ts:327-331): 0 results -> undefined, 1 -> + * bare value, 2+ -> array. Throws on a shape mismatch rather than + * silently discarding or coercing (issue #188). + * + * Deliberate deviation from issue #188's suggested fix: for arity 1 the + * issue suggests rejecting an array `raw`. That's wrong — a single + * `list`/tuple result IS legitimately a JS array, and disambiguating + * "one result that happens to be an array" from "N results" is exactly + * why computeExportArities (runtime-executor.ts:83-88) exists in the + * first place. So arity 1 only requires `raw !== undefined` (a + * ComponentValue is never undefined — runtime/src/cabi/types.ts:244-253), + * with no shape restriction. */ +export function collapseResultsByArity( + raw: unknown, + declaredArity: number | undefined, + field: string, +): unknown[] { + if (declaredArity === undefined) { + // Defensive fallback only: the export wasn't found in the plan's + // arity map. No declared shape to validate against. + return raw === undefined ? [] : [raw]; + } + if (declaredArity === 0) { + if (raw !== undefined) { + throw new Error( + `invoke '${field}': declared 0 results but got ${describeValue(raw)}`, + ); + } + return []; + } + if (declaredArity === 1) { + if (raw === undefined) { + throw new Error( + `invoke '${field}': declared 1 result but got undefined`, + ); + } + return [raw]; + } + if (!Array.isArray(raw) || raw.length !== declaredArity) { + const got = Array.isArray(raw) + ? `array of length ${raw.length}` + : describeValue(raw); + throw new Error( + `invoke '${field}': declared ${declaredArity} results but got ${got}`, + ); + } + return raw; +} + /** Compare an expected `Value[]` list against actual ComponentValue results * (per the runtime's arity convention: 0 results -> undefined, 1 -> bare * value, 2+ -> array — see `resultsToHost` in runtime/src/exec/boundary.ts). */ @@ -377,12 +445,17 @@ export function compareValues( ): string | undefined { let actualList: unknown[]; if (expected.length === 0) { + // Issue #188: don't vacuously pass — an unexpected actual value here + // (e.g. a spurious result from an arity-0 export) must be reported. + if (actual !== undefined) { + return `expected 0 results, got ${describeValue(actual)}`; + } actualList = []; } else if (expected.length === 1) { actualList = [actual]; } else { if (!Array.isArray(actual)) { - return `expected ${expected.length} results, got ${JSON.stringify(actual)}`; + return `expected ${expected.length} results, got ${describeValue(actual)}`; } actualList = actual; } diff --git a/harness/tests/value_mapping_test.ts b/harness/tests/value_mapping_test.ts new file mode 100644 index 0000000..a9276ee --- /dev/null +++ b/harness/tests/value_mapping_test.ts @@ -0,0 +1,93 @@ +// Unit tests for harness/src/value-mapping.ts's arity-collapse and +// empty-expected comparison paths. Regression coverage for issue #188: +// `assert_return` with zero expected results never checked the actual +// value returned by the export (two stacked discard sites). + +import { collapseResultsByArity, compareValues } from "../src/value-mapping.ts"; + +function assertEq(actual: unknown, expected: unknown, what: string) { + const a = JSON.stringify(actual, (_k, v) => typeof v === "bigint" ? `${v}n` : v); + const e = JSON.stringify(expected, (_k, v) => typeof v === "bigint" ? `${v}n` : v); + if (a !== e) throw new Error(`${what}: expected ${e}, got ${a}`); +} + +function assertThrows(fn: () => void, msgContains: string, what: string) { + try { + fn(); + } catch (e) { + const msg = e instanceof Error ? e.message : String(e); + if (!msg.includes(msgContains)) { + throw new Error( + `${what}: expected error containing "${msgContains}", got "${msg}"`, + ); + } + return; + } + throw new Error(`${what}: expected throw, but function returned normally`); +} + +// --- compareValues: empty-expected must not vacuously pass --- + +Deno.test("compareValues([], undefined) passes", () => { + assertEq(compareValues([], undefined), undefined, "match"); +}); + +Deno.test("compareValues([], 5) is a mismatch", () => { + const m = compareValues([], 5); + if (m === undefined) throw new Error("expected mismatch, got pass"); +}); + +Deno.test("compareValues([], 0n) is a mismatch and does not throw", () => { + // bigint must not blow up describeValue's JSON.stringify use. + const m = compareValues([], 0n); + if (m === undefined) throw new Error("expected mismatch, got pass"); +}); + +// --- collapseResultsByArity --- + +Deno.test("arity 0 + undefined -> []", () => { + assertEq(collapseResultsByArity(undefined, 0, "f"), [], "arity0-ok"); +}); + +Deno.test("arity 0 + spurious value throws naming the field", () => { + assertThrows(() => collapseResultsByArity(42, 0, "f"), "'f'", "arity0-bad"); +}); + +Deno.test("arity 0 + spurious bigint throws (bigint-safe message)", () => { + assertThrows(() => collapseResultsByArity(7n, 0, "f"), "'f'", "arity0-bigint"); +}); + +Deno.test("arity 1 + bare value -> [v]", () => { + assertEq(collapseResultsByArity(42, 1, "f"), [42], "arity1-ok"); +}); + +Deno.test("arity 1 + array value -> [arr] (deliberate deviation from issue text)", () => { + // A single list/tuple result IS a JS array; must NOT throw. + const arr = [1, 2, 3]; + assertEq(collapseResultsByArity(arr, 1, "f"), [arr], "arity1-array"); +}); + +Deno.test("arity 1 + undefined throws", () => { + assertThrows(() => collapseResultsByArity(undefined, 1, "f"), "'f'", "arity1-undef"); +}); + +Deno.test("arity 2 + matching-length array returned as-is", () => { + const arr = [1, 2]; + assertEq(collapseResultsByArity(arr, 2, "f"), arr, "arity2-ok"); +}); + +Deno.test("arity 2 + wrong-length array throws", () => { + assertThrows(() => collapseResultsByArity([1], 2, "f"), "'f'", "arity2-wronglen"); +}); + +Deno.test("arity 2 + non-array throws", () => { + assertThrows(() => collapseResultsByArity(42, 2, "f"), "'f'", "arity2-nonarray"); +}); + +Deno.test("undefined declared arity + undefined -> [] (fallback)", () => { + assertEq(collapseResultsByArity(undefined, undefined, "f"), [], "fallback-undef"); +}); + +Deno.test("undefined declared arity + value -> [value] (fallback)", () => { + assertEq(collapseResultsByArity(42, undefined, "f"), [42], "fallback-val"); +});