From fc7eff3f6d0a4f1db502c9bbadd070db03d9daea Mon Sep 17 00:00:00 2001 From: "claude[bot]" Date: Mon, 10 Aug 2026 07:40:45 +0000 Subject: [PATCH 1/2] fix(core): fail the QA `contains` assertion on a non-evaluable actual instead of silently passing (#7256) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `TestRunner.assert`'s `case 'contains':` handled an array (membership) and a string (substring) and had no `else`, so `undefined` — a typo'd `field` path or a response shape that moved — plus `null`, a number, a boolean or an object fell out of the switch throwing nothing, and the assertion reported PASSED. A suite asserting `contains` against a field the result does not carry was asserting nothing, and CI believed the green. `contains` was the only path in this engine that could decide "no comparison applies here" and report success; every other unhandled shape already throws. An assertion the engine cannot evaluate is now a FAILED assertion, with a message that names the field, the operator and the runtime type the path resolved to, and says which of the fixture or the assertion is the suspect: `undefined`/`null` mean the path did not resolve, anything else means the path resolved and the operator does not apply. The two evaluable shapes are unchanged in both directions. Pinned in a new `runner.test.ts`, including the sibling operators, which do not carry this defect — `not_contains`/`gt`/`gte`/`lt`/`lte`/`error` are declared in `TestAssertionTypeSchema` with no branch in the runner, and were already refused loudly at `default:`. Refs #7256, #6247 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01EdBxv9i73dqkDD2QcnvNzd --- .../qa-contains-non-evaluable-fails-loud.md | 60 +++++ packages/core/src/qa/runner.test.ts | 221 ++++++++++++++++++ packages/core/src/qa/runner.ts | 48 ++++ 3 files changed, 329 insertions(+) create mode 100644 .changeset/qa-contains-non-evaluable-fails-loud.md create mode 100644 packages/core/src/qa/runner.test.ts diff --git a/.changeset/qa-contains-non-evaluable-fails-loud.md b/.changeset/qa-contains-non-evaluable-fails-loud.md new file mode 100644 index 0000000000..cb2201bd69 --- /dev/null +++ b/.changeset/qa-contains-non-evaluable-fails-loud.md @@ -0,0 +1,60 @@ +--- +"@objectstack/core": minor +--- + +fix(core): the QA `contains` assertion fails loudly instead of silently passing on a non-array/non-string actual (#7256) + +`TestRunner.assert`'s `case 'contains':` handled the two shapes it can evaluate — +an array (membership) and a string (substring) — and had **no `else`**. Every +other shape fell straight out of the switch throwing nothing, so the assertion +reported **PASSED**. A scenario asserting +`{ field: "body.data.items", operator: "contains", expectedValue: "acme" }` +against a response that has no `body.data.items` at all reported ✅. The +overwhelmingly common way to reach that branch is the one that matters most: a +typo'd `field` path, or a response shape that moved under a suite nobody +re-read. The assertion that was supposed to *be* the test is the thing that +silently disappears, and CI believes the green. + +`contains` was the only path in this engine that could decide "no comparison +applies here" and report success. Every other unhandled shape already fails +loud — an operator with no branch throws `Unknown assertion operator`, an action +type with no adapter branch throws `Unsupported action type in HttpAdapter`, +and `equals`/`not_equals`/`is_null`/`not_null` all compare unconditionally. This +closes the asymmetry rather than adding a new posture: an assertion the engine +**cannot evaluate** is a **failed** assertion. + +The message is written for the author who has to act on it, so it names the +field, the operator and the runtime type of what the path actually resolved to +(`null` and arrays get their own names, not `typeof`'s `object`), and then says +which of the two things is wrong: + +``` +Assertion failed: body.data.items cannot be evaluated by 'contains' — expected an +array or a string at that path, got undefined. The path resolved to nothing — the +field is absent from the result, or the path is misspelled. Use 'is_null' if +asserting absence is what you meant. +``` + +`undefined`/`null` point at the **fixture** (the path did not resolve, so the +field path or the response shape it was written against is the suspect); +a number, boolean or object points at the **assertion** (the path resolved +fine and `contains` is the wrong operator for what it found). + +**Behaviour change, and its measured blast radius.** Suites that today pass a +`contains` against a non-array/non-string will start failing — which is the +point; each such assertion was asserting nothing. The in-tree radius was +measured on the loud build and is **zero**: `os test` is the runner's only +consumer, and the repository contains no Quality Protocol suite documents at +all (no `qa/*.test.json` anywhere; the three example apps run `vitest`, and +`packages/qa/*` are vitest suites that never touch `TestRunner`). No CI workflow +invokes `os test`. So no in-repo case was passing vacuously and none needed +repair. Downstream suites are the ones that will see red, and every case they +see is a test that was never running. + +The two evaluable shapes are untouched in both directions: a matching array or +string still passes, a non-matching one still fails with its existing message. +`not_contains`, `gt`, `gte`, `lt`, `lte` and `error` are declared in +`TestAssertionTypeSchema` and still have no branch in the runner — they were +already refused loudly at `default:` rather than silently passed, so they do not +carry this defect; that gap is recorded separately and is pinned here so a later +implementation is a deliberate change rather than an accident. diff --git a/packages/core/src/qa/runner.test.ts b/packages/core/src/qa/runner.test.ts new file mode 100644 index 0000000000..c9704149a4 --- /dev/null +++ b/packages/core/src/qa/runner.test.ts @@ -0,0 +1,221 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #7256 — the `contains` assertion used to SILENTLY PASS when the actual value was +// neither an array nor a string. The `case 'contains':` block handled the two +// evaluable shapes and had no `else`, so `undefined` (a typo'd `field` path, or a +// response shape that moved), `null`, a number or an object fell out of the switch +// throwing nothing, and the scenario reported ✅. A suite asserting `contains` +// against a field the result does not have was asserting NOTHING, and CI believed it. +// +// These pin both halves of the fix: the three non-evaluable shapes now fail LOUD with +// a message that names the runtime type and says which of the fixture or the assertion +// is the suspect, and the two evaluable shapes keep their pre-existing behaviour in +// BOTH directions (a match still passes, a miss still fails). + +import { describe, it, expect } from 'vitest'; +import * as QA from '@objectstack/spec/qa'; +import { TestRunner } from './runner.js'; +import type { TestExecutionAdapter } from './adapter.js'; + +/** An adapter that hands the runner one fixed result — the assertion is the unit under test. */ +class StubAdapter implements TestExecutionAdapter { + constructor(private result: unknown) {} + async execute(): Promise { + return this.result; + } +} + +/** Run one assertion against one canned adapter result, through the public runner surface. */ +async function runAssertion( + result: unknown, + assertion: QA.TestAssertion, +): Promise<{ passed: boolean; error: string }> { + const runner = new TestRunner(new StubAdapter(result)); + const [outcome] = await runner.runSuite({ + name: 'contains-pins', + scenarios: [ + { + id: 'scenario-1', + name: 'single assertion', + steps: [ + { + name: 'step-1', + action: { type: 'api_call', target: '/api/v1/accounts' }, + assertions: [assertion], + }, + ], + }, + ], + }); + const error = outcome.error; + return { + passed: outcome.passed, + error: error instanceof Error ? error.message : String(error ?? ''), + }; +} + +const containsAcme: QA.TestAssertion = { + field: 'body.data.items', + operator: 'contains', + expectedValue: 'acme', +}; + +describe("TestRunner — `contains` against a non-array/non-string actual fails loud (#7256)", () => { + it('fails when the field path resolves to nothing (the filed case: a missing path)', async () => { + const { passed, error } = await runAssertion({ body: { data: {} } }, containsAcme); + + expect(passed).toBe(false); + // Names the field, the operator and the runtime type... + expect(error).toContain('body.data.items'); + expect(error).toContain("'contains'"); + expect(error).toContain('got undefined'); + // ...and points at the FIXTURE, because the path is what did not resolve. + expect(error).toContain('absent from the result'); + }); + + it('fails when the whole response shape is missing, not just the leaf', async () => { + const { passed, error } = await runAssertion(undefined, containsAcme); + + expect(passed).toBe(false); + expect(error).toContain('got undefined'); + }); + + it('fails when the field path resolves to null', async () => { + const { passed, error } = await runAssertion({ body: { data: { items: null } } }, containsAcme); + + expect(passed).toBe(false); + expect(error).toContain('got null'); + // `null` is not reported as `object` — the author needs to see which one it is. + expect(error).not.toContain('got object'); + expect(error).toContain('resolved to null'); + }); + + it('fails when the field path resolves to a number', async () => { + const { passed, error } = await runAssertion({ body: { data: { items: 42 } } }, containsAcme); + + expect(passed).toBe(false); + expect(error).toContain('got number'); + // The path resolved fine here, so the ASSERTION is the suspect, not the fixture. + expect(error).toContain('array membership and string substrings only'); + }); + + it('fails when the field path resolves to an object', async () => { + const { passed, error } = await runAssertion( + { body: { data: { items: { acme: true } } } }, + containsAcme, + ); + + expect(passed).toBe(false); + expect(error).toContain('got object'); + expect(error).toContain('array membership and string substrings only'); + }); + + it('fails when the field path resolves to a boolean', async () => { + const { passed, error } = await runAssertion({ body: { data: { items: false } } }, containsAcme); + + expect(passed).toBe(false); + expect(error).toContain('got boolean'); + }); + + it('every non-evaluable shape reports the same failure, not a pass', async () => { + const nonEvaluable: unknown[] = [undefined, null, 0, 42, false, true, { acme: true }]; + + for (const value of nonEvaluable) { + const { passed, error } = await runAssertion({ body: { data: { items: value } } }, containsAcme); + expect(passed, `contains against ${String(value)} must not pass`).toBe(false); + expect(error).toContain("cannot be evaluated by 'contains'"); + } + }); +}); + +describe('TestRunner — `contains` keeps its behaviour on the two evaluable shapes (#7256)', () => { + it('passes when the array contains the expected member', async () => { + const { passed } = await runAssertion({ body: { data: { items: ['acme', 'globex'] } } }, containsAcme); + + expect(passed).toBe(true); + }); + + it('fails when the array does not contain the expected member', async () => { + const { passed, error } = await runAssertion({ body: { data: { items: ['globex'] } } }, containsAcme); + + expect(passed).toBe(false); + expect(error).toContain('array does not contain acme'); + // Still the membership failure, NOT the new inapplicable-shape failure. + expect(error).not.toContain("cannot be evaluated by 'contains'"); + }); + + it('passes when the string contains the expected substring', async () => { + const { passed } = await runAssertion({ body: { data: { items: 'acme corp' } } }, containsAcme); + + expect(passed).toBe(true); + }); + + it('fails when the string does not contain the expected substring', async () => { + const { passed, error } = await runAssertion({ body: { data: { items: 'globex corp' } } }, containsAcme); + + expect(passed).toBe(false); + expect(error).toContain('string does not contain acme'); + expect(error).not.toContain("cannot be evaluated by 'contains'"); + }); + + it('an empty array is evaluable — it simply does not contain the member', async () => { + const { passed, error } = await runAssertion({ body: { data: { items: [] } } }, containsAcme); + + expect(passed).toBe(false); + expect(error).toContain('array does not contain acme'); + }); + + it('an empty string is evaluable — every string contains the empty substring', async () => { + const { passed } = await runAssertion( + { body: { data: { items: '' } } }, + { field: 'body.data.items', operator: 'contains', expectedValue: '' }, + ); + + expect(passed).toBe(true); + }); +}); + +describe('TestRunner — the sibling operators are unchanged by #7256', () => { + it('`equals` still compares unconditionally, including against a missing path', async () => { + const { passed, error } = await runAssertion( + { body: {} }, + { field: 'body.status', operator: 'equals', expectedValue: 'active' }, + ); + + expect(passed).toBe(false); + expect(error).toContain('expected active'); + }); + + it('`is_null` still passes on a missing path — absence is what it asserts', async () => { + const { passed } = await runAssertion( + { body: {} }, + { field: 'body.status', operator: 'is_null', expectedValue: null }, + ); + + expect(passed).toBe(true); + }); + + it('`not_null` still fails on a missing path', async () => { + const { passed } = await runAssertion( + { body: {} }, + { field: 'body.status', operator: 'not_null', expectedValue: null }, + ); + + expect(passed).toBe(false); + }); + + // `not_contains` / `gt` / `gte` / `lt` / `lte` / `error` are declared in + // `TestAssertionTypeSchema` and have no branch in the runner. That is a DIFFERENT + // defect from #7256 (a declared operator the engine refuses is annoying but honest, + // where a silent pass is a lie), and it is pinned here so a later implementation is + // a deliberate change rather than an accident. + it('a declared-but-unimplemented operator is refused, not silently passed', async () => { + const { passed, error } = await runAssertion( + { body: { data: { items: ['globex'] } } }, + { field: 'body.data.items', operator: 'not_contains', expectedValue: 'acme' }, + ); + + expect(passed).toBe(false); + expect(error).toContain('Unknown assertion operator: not_contains'); + }); +}); diff --git a/packages/core/src/qa/runner.ts b/packages/core/src/qa/runner.ts index 9a54166d2b..2f6795f0b6 100644 --- a/packages/core/src/qa/runner.ts +++ b/packages/core/src/qa/runner.ts @@ -19,6 +19,40 @@ export interface StepResult { duration: number; } +/** + * Name the runtime shape of a value the way a suite author sees it in their fixture. + * `typeof` answers `object` for both `null` and an array, which are the two shapes a + * `contains` author most needs told apart from a plain record. + */ +function describeActualType(value: unknown): string { + if (value === null) return 'null'; + if (Array.isArray(value)) return 'array'; + return typeof value; +} + +/** + * Say WHICH of the two things is wrong, because the message is the only thing the + * author has: `undefined`/`null` mean the path did not resolve to a value, so the + * FIXTURE (the field path, or the response shape it was written against) is the + * suspect; anything else means the path resolved fine and the ASSERTION picked an + * operator that does not apply to what it found. + */ +function containsInapplicableHint(actual: unknown): string { + if (actual === undefined) { + return ( + 'The path resolved to nothing — the field is absent from the result, or the path is misspelled. ' + + "Use 'is_null' if asserting absence is what you meant." + ); + } + if (actual === null) { + return "The path resolved to null. Use 'is_null' if asserting absence is what you meant."; + } + return ( + "'contains' tests array membership and string substrings only. " + + "Use 'equals' to compare a scalar, or point the field at the array or string you meant to look inside." + ); +} + export class TestRunner { constructor(private adapter: TestExecutionAdapter) {} @@ -173,6 +207,20 @@ export class TestRunner { if (!actual.includes(expected)) throw new Error(`Assertion failed: ${assertion.field} array does not contain ${expected}`); } else if (typeof actual === 'string') { if (!actual.includes(String(expected))) throw new Error(`Assertion failed: ${assertion.field} string does not contain ${expected}`); + } else { + // `contains` is defined over arrays (membership) and strings (substring), and + // over nothing else. This branch used to be absent, so every other shape fell + // out of the switch and the assertion reported PASSED (#7256) — a `contains` + // written against a path the result does not carry was the test silently + // deleting itself, and CI believed the green. An assertion the engine cannot + // evaluate is a FAILED assertion, which is the posture every other unhandled + // shape in this engine already takes (`default:` below; the HTTP adapter's + // unknown action type). + throw new Error( + `Assertion failed: ${assertion.field} cannot be evaluated by 'contains' — ` + + `expected an array or a string at that path, got ${describeActualType(actual)}. ` + + containsInapplicableHint(actual) + ); } break; case 'not_null': From 8467c47ac9e8c1c893365d7da22a9e34b9d89366 Mon Sep 17 00:00:00 2001 From: "claude[bot]" Date: Mon, 10 Aug 2026 08:01:28 +0000 Subject: [PATCH 2/2] docs(cli): say that an assertion `os test` cannot evaluate fails rather than passes (#7256) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `os test` section already explains what stops a MALFORMED SUITE from reporting success (the load-site `TestSuiteSchema` parse). It said nothing about the assertion engine, where the same class of false green lived: a `contains` pointed at a path the response does not carry used to fall out of the switch and report ✅. Records the rule and the two operators an author reaches for instead — `is_null` for absence, `equals` for a scalar — so the new error message lands on a page that agrees with it. Refs #7256 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01EdBxv9i73dqkDD2QcnvNzd --- content/docs/deployment/cli.mdx | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/content/docs/deployment/cli.mdx b/content/docs/deployment/cli.mdx index d74ed0e53e..27f9e88e18 100644 --- a/content/docs/deployment/cli.mdx +++ b/content/docs/deployment/cli.mdx @@ -1026,6 +1026,15 @@ and counts as one failed suite — the rest of the glob still runs. This is what stops a malformed suite from reporting success: a misspelled `steps` key used to produce a scenario that passed having executed nothing. +An assertion the runner **cannot evaluate** fails, it does not pass. `contains` +is defined over an array (membership) and a string (substring); point it at +anything else — most often a `field` path the response does not carry, because it +was misspelled or the shape moved — and it fails, naming the field, the operator +and the runtime type it actually found. Until #7256 that case fell out of the +switch and reported ✅, so a `contains` against a missing path was a test that +silently deleted itself. Assert absence with `is_null`; compare a scalar with +`equals`. + #### `os doctor` Checks your development environment and reports issues: