From 0b42e58afaf036b8aabc0e818ba4293660ba3ee0 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Fri, 24 Apr 2026 10:29:40 -0400 Subject: [PATCH 1/3] test(drift): warn on redundant field_value_or_absent against required fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #876 review. When a storyboard asserts `field_value_or_absent` on a path the response schema already marks required, the tolerance is dead code — the spec rules out the absent branch. The new `isPathRequired` schema walker + describe block catches this at the same gate as the existing reachability lint, so authors get redirected to plain `field_value` when tolerance is meaningless. Conservative on unions (required only if every branch requires it), aggressive on intersections (required if either side requires it) — matches the symmetry of `isPathReachable`. Covers 10 unit-test cases on the helper + a storyboard-wide gate that's empty today (no storyboards use the matcher yet) but engages as adoption grows. Pure test-file change, no library code touched — no changeset. Co-Authored-By: Claude Opus 4.7 (1M context) --- test/lib/storyboard-drift.test.js | 151 +++++++++++++++++++++++++++++- 1 file changed, 149 insertions(+), 2 deletions(-) diff --git a/test/lib/storyboard-drift.test.js b/test/lib/storyboard-drift.test.js index aab3cde80..d01ab5c51 100644 --- a/test/lib/storyboard-drift.test.js +++ b/test/lib/storyboard-drift.test.js @@ -3,12 +3,15 @@ * * Catches when field_present / field_value / field_value_or_absent paths in * storyboard YAML reference fields that don't exist in the corresponding - * Zod response schemas, and when context extractors reference tasks without - * schemas. + * Zod response schemas, when context extractors reference tasks without + * schemas, and when `field_value_or_absent` is asserted on a path the + * response schema already marks required (the tolerance is meaningless — + * the storyboard author should have used `field_value`). */ const { describe, it } = require('node:test'); const assert = require('node:assert/strict'); +const { z } = require('zod'); const { listAllComplianceStoryboards } = require('../../dist/lib/testing/storyboard/index.js'); const { parsePath } = require('../../dist/lib/testing/storyboard/path.js'); @@ -98,6 +101,69 @@ function isPathReachable(schema, segments) { return false; } +/** + * Walk a Zod v4 schema along a parsed path and report whether the path is + * *required* — i.e. the spec guarantees the field is present. A path that + * traverses ANY `optional` / `nullable` / `default` wrapper is not required; + * neither is a path through a `record` (any key) or a `union` branch that + * omits the field. + * + * Conservative on unions: required only if ALL branches require it. Aggressive + * on intersections: required if EITHER side requires it (matches + * `isPathReachable` symmetry). + * + * Used to lint `field_value_or_absent` assertions: if the path the tolerant + * matcher targets is already required by the schema, the tolerance is dead + * code — the author should have used `field_value`. + */ +function isPathRequired(schema, segments) { + const type = schema?._zod?.def?.type; + if (!type) return false; + + // Any tolerance wrapper means "not required at this level." + if (type === 'optional' || type === 'nullable' || type === 'default' || type === 'catch') { + return false; + } + + if (type === 'pipe') { + return isPathRequired(schema._zod.def.in, segments); + } + + if (type === 'union') { + const options = schema.options || []; + return options.length > 0 && options.every(opt => isPathRequired(opt, segments)); + } + + if (type === 'intersection') { + const { left, right } = schema._zod.def; + return isPathRequired(left, segments) || isPathRequired(right, segments); + } + + if (segments.length === 0) { + // Reached the end of the path on a non-tolerance schema — required. + return true; + } + + const [head, ...rest] = segments; + + if (typeof head === 'number') { + if (type === 'array' && schema.element) return isPathRequired(schema.element, rest); + return false; + } + + if (type === 'object' && schema.shape) { + const field = schema.shape[head]; + if (!field) return false; + return isPathRequired(field, rest); + } + + // Records don't guarantee any specific key. + if (type === 'record') return false; + + // Leaf with segments remaining — path doesn't exist. + return false; +} + // ──────────────────────────────────────────────────────────── // Collect validation paths from all storyboards // ──────────────────────────────────────────────────────────── @@ -275,6 +341,87 @@ describe('storyboard schema drift', () => { } }); + // Lint: `field_value_or_absent` is meaningful only when the schema does NOT + // already guarantee the field is present. If a storyboard uses the tolerant + // matcher on a required field, the tolerance is dead code — the spec already + // rules out the "absent" branch. Redirect authors to `field_value` there. + // Envelope-tolerant paths (declared in `ENVELOPE_PATHS` above) are skipped + // because they target protocol-level fields not modeled on individual tool + // response schemas. + describe('field_value_or_absent is not redundantly applied to schema-required fields', () => { + const tolerantValidations = fieldValidations.filter(v => v.check === 'field_value_or_absent'); + + for (const entry of tolerantValidations) { + const schema = TOOL_RESPONSE_SCHEMAS[entry.task]; + if (!schema) continue; + + it(`${entry.storyboard}/${entry.step}: ${entry.path} is not schema-required (use field_value if it is)`, () => { + const segments = parsePath(entry.path); + const required = isPathRequired(schema, segments); + assert.ok( + !required, + `Path "${entry.path}" is required in ${entry.task} response schema — ` + + `the tolerance in \`field_value_or_absent\` is meaningless. Use \`field_value\` instead.` + ); + }); + } + }); + + describe('isPathRequired helper', () => { + it('returns true for a top-level required string field', () => { + const schema = z.object({ status: z.string() }); + assert.equal(isPathRequired(schema, ['status']), true); + }); + + it('returns false for a top-level optional field', () => { + const schema = z.object({ replayed: z.boolean().optional() }); + assert.equal(isPathRequired(schema, ['replayed']), false); + }); + + it('returns false for a nullable field (null is a legal value, not presence)', () => { + const schema = z.object({ note: z.string().nullable() }); + assert.equal(isPathRequired(schema, ['note']), false); + }); + + it('returns false for a defaulted field (default implies absence is tolerated)', () => { + const schema = z.object({ currency: z.string().default('USD') }); + assert.equal(isPathRequired(schema, ['currency']), false); + }); + + it('returns true through a nested required object', () => { + const schema = z.object({ envelope: z.object({ status: z.string() }) }); + assert.equal(isPathRequired(schema, ['envelope', 'status']), true); + }); + + it('returns false when the intermediate wrapper is optional', () => { + const schema = z.object({ envelope: z.object({ status: z.string() }).optional() }); + assert.equal(isPathRequired(schema, ['envelope', 'status']), false); + }); + + it('returns false for a missing field', () => { + const schema = z.object({ status: z.string() }); + assert.equal(isPathRequired(schema, ['nope']), false); + }); + + it('returns true through an array element when the element schema requires the key', () => { + const schema = z.object({ accounts: z.array(z.object({ id: z.string() })) }); + assert.equal(isPathRequired(schema, ['accounts', 0, 'id']), true); + }); + + it('returns false for record key access (no specific key is guaranteed)', () => { + const schema = z.object({ props: z.record(z.string(), z.string()) }); + assert.equal(isPathRequired(schema, ['props', 'whatever']), false); + }); + + it('union: required only if EVERY branch requires it', () => { + const both = z.union([z.object({ id: z.string() }), z.object({ id: z.string(), extra: z.number() })]); + assert.equal(isPathRequired(both, ['id']), true); + + const onlyOne = z.union([z.object({ id: z.string() }), z.object({ other: z.string() })]); + assert.equal(isPathRequired(onlyOne, ['id']), false); + }); + }); + describe('context extractor tasks have registered response schemas', () => { const extractorTasks = Object.keys(CONTEXT_EXTRACTORS); From 46f1180d89ad230934a2213b5e74ccc09fc73c65 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Fri, 24 Apr 2026 10:34:17 -0400 Subject: [PATCH 2/3] chore: clarify intersection semantics comment, grep-friendlier lint message Review nits from code-reviewer pass on PR #889: - Intersection handling is correct semantics (a value must satisfy both sides), not a 'symmetry' hack. Rewrite the doc comment. - Quote `field_value` in the lint failure title so CI log grep finds both sides of the matcher pair. Co-Authored-By: Claude Opus 4.7 (1M context) --- test/lib/storyboard-drift.test.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/test/lib/storyboard-drift.test.js b/test/lib/storyboard-drift.test.js index d01ab5c51..17d39d2cd 100644 --- a/test/lib/storyboard-drift.test.js +++ b/test/lib/storyboard-drift.test.js @@ -108,9 +108,10 @@ function isPathReachable(schema, segments) { * neither is a path through a `record` (any key) or a `union` branch that * omits the field. * - * Conservative on unions: required only if ALL branches require it. Aggressive - * on intersections: required if EITHER side requires it (matches - * `isPathReachable` symmetry). + * Conservative on unions: required only if ALL branches require it. On + * intersections: required if EITHER side requires it — a value in + * `z.intersection(L, R)` must satisfy both sides, so the union of their + * requirements applies. * * Used to lint `field_value_or_absent` assertions: if the path the tolerant * matcher targets is already required by the schema, the tolerance is dead @@ -355,7 +356,7 @@ describe('storyboard schema drift', () => { const schema = TOOL_RESPONSE_SCHEMAS[entry.task]; if (!schema) continue; - it(`${entry.storyboard}/${entry.step}: ${entry.path} is not schema-required (use field_value if it is)`, () => { + it(`${entry.storyboard}/${entry.step}: ${entry.path} is not schema-required (use \`field_value\` if it is)`, () => { const segments = parsePath(entry.path); const required = isPathRequired(schema, segments); assert.ok( From ccf3de6fc6f499e6bc701047ceb97e58b0c5df63 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Fri, 24 Apr 2026 10:36:07 -0400 Subject: [PATCH 3/3] chore: empty changeset to satisfy CI gate on test-only diff Co-Authored-By: Claude Opus 4.7 (1M context) --- .changeset/redundant-tolerance-lint.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changeset/redundant-tolerance-lint.md diff --git a/.changeset/redundant-tolerance-lint.md b/.changeset/redundant-tolerance-lint.md new file mode 100644 index 000000000..4c6a62f51 --- /dev/null +++ b/.changeset/redundant-tolerance-lint.md @@ -0,0 +1,6 @@ +--- +--- + +Test-only — adds a drift-test lint that warns when `field_value_or_absent` is +asserted on a schema-required path. No library code touched; this empty +changeset is present only to satisfy the `changeset status` CI gate.