diff --git a/.changeset/lint-storyboard-context-output-paths.md b/.changeset/lint-storyboard-context-output-paths.md new file mode 100644 index 0000000000..4e78fcde60 --- /dev/null +++ b/.changeset/lint-storyboard-context-output-paths.md @@ -0,0 +1,14 @@ +--- +--- + +ci(storyboards): lint context_outputs.path against the response schema + +Closes the CI-gate item on #3918. New `lint-storyboard-context-output-paths.cjs` walks every storyboard under `static/compliance/source/`, finds steps with both `response_schema_ref` and `context_outputs[]`, and verifies each `path` resolves to a defined field in the referenced response schema (descending through `$ref`, `oneOf` / `anyOf` / `allOf` variants, and `items` for numeric indices). + +Catches the class of bug where a storyboard captures from a path the spec doesn't define — a real agent's response will silently miss the capture, downstream `$context.` consumers see undefined values, and the storyboard nominally passes lint while failing in practice. + +The lint surfaced one real typo on land: `static/compliance/source/protocols/sponsored-intelligence/index.yaml`'s `si_get_offering` step was capturing `path: 'offering_id'` from `si-get-offering-response.json`, but the schema defines `offering.offering_id` (the id is nested inside the `offering` object). Fixed in this PR. + +Two captures that traverse spec-sanctioned extension points (`error.details` polymorphism keyed on `error.code`, and request-key echo through `additionalProperties: true`) are documented in `scripts/storyboard-context-output-paths-allowlist.json` with `reason` strings explaining what the lint cannot statically verify; reviewers should re-read these on spec changes that might give those polymorphisms first-class schema treatment. + +Wired into the `npm test` pipeline via `npm run test:storyboard-context-output-paths`. Companion to the existing `lint-error-codes.cjs` (which already covers the `expect_error.code` half of #3918's CI-gate ask). diff --git a/package.json b/package.json index e45fe4b379..73aacd11ac 100644 --- a/package.json +++ b/package.json @@ -42,6 +42,7 @@ "test:storyboard-test-kits": "node --test --test-force-exit --test-timeout=30000 tests/lint-storyboard-test-kits.test.cjs", "test:storyboard-sample-request-schema": "node --test --test-force-exit --test-timeout=30000 tests/lint-storyboard-sample-request-schema.test.cjs", "test:storyboard-response-schema": "node --test --test-force-exit --test-timeout=30000 tests/lint-storyboard-response-schema.test.cjs", + "test:storyboard-context-output-paths": "node --test --test-force-exit --test-timeout=30000 tests/lint-storyboard-context-output-paths.test.cjs", "test:storyboard-check-enum": "node --test --test-force-exit --test-timeout=30000 tests/lint-storyboard-check-enum.test.cjs", "test:storyboard-advisory-expiry": "node --test --test-force-exit --test-timeout=30000 tests/lint-storyboard-advisory-expiry.test.cjs", "test:storyboard-raw-mode-required": "node --test --test-force-exit --test-timeout=30000 tests/lint-storyboard-raw-mode-required.test.cjs", @@ -65,7 +66,7 @@ "test:platform-agnostic": "node tests/check-platform-agnostic.cjs", "test:oneof-discriminators": "node scripts/audit-oneof.mjs --check", "audit:oneof": "node scripts/audit-oneof.mjs", - "test": "npm run test:docs-nav && npm run test:schemas && npm run test:examples && npm run test:extensions && npm run test:extension-schemas && npm run test:error-handling && npm run test:json-schema && npm run test:composed && npm run test:migrations && npm run test:hmac-vectors && npm run test:hmac-signer-conformance && npm run test:transport-errors && npm run test:targeting-overlay-vectors && npm run test:storyboard-scoping && npm run test:storyboard-branch-sets && npm run test:storyboard-provides-state-for && npm run test:storyboard-contradictions && npm run test:storyboard-context-entity && npm run test:storyboard-auth-shape && npm run test:storyboard-test-kits && npm run test:storyboard-sample-request-schema && npm run test:storyboard-response-schema && npm run test:storyboard-check-enum && npm run test:storyboard-advisory-expiry && npm run test:storyboard-raw-mode-required && npm run test:storyboard-doc-parity && npm run test:pagination-invariant && npm run test:version-envelope && npm run test:test-dynamic-imports && npm run test:callapi-state-change && npm run test:build-schemas-hoist-enums && npm run test:error-codes && npm run test:substitution-vector-names && npm run test:platform-agnostic && npm run test:oneof-discriminators && npm run test:unit && npm run test:server-unit && npm run test:openapi && npm run typecheck", + "test": "npm run test:docs-nav && npm run test:schemas && npm run test:examples && npm run test:extensions && npm run test:extension-schemas && npm run test:error-handling && npm run test:json-schema && npm run test:composed && npm run test:migrations && npm run test:hmac-vectors && npm run test:hmac-signer-conformance && npm run test:transport-errors && npm run test:targeting-overlay-vectors && npm run test:storyboard-scoping && npm run test:storyboard-branch-sets && npm run test:storyboard-provides-state-for && npm run test:storyboard-contradictions && npm run test:storyboard-context-entity && npm run test:storyboard-auth-shape && npm run test:storyboard-test-kits && npm run test:storyboard-sample-request-schema && npm run test:storyboard-response-schema && npm run test:storyboard-context-output-paths && npm run test:storyboard-check-enum && npm run test:storyboard-advisory-expiry && npm run test:storyboard-raw-mode-required && npm run test:storyboard-doc-parity && npm run test:pagination-invariant && npm run test:version-envelope && npm run test:test-dynamic-imports && npm run test:callapi-state-change && npm run test:build-schemas-hoist-enums && npm run test:error-codes && npm run test:substitution-vector-names && npm run test:platform-agnostic && npm run test:oneof-discriminators && npm run test:unit && npm run test:server-unit && npm run test:openapi && npm run typecheck", "test:all": "npm run test:schemas && npm run test:examples && npm run test:extensions && npm run test:error-handling && npm run test:snippets && npm run typecheck", "precommit": "bash scripts/with-timeout.sh 60 npm run test:unit && npm run test:test-dynamic-imports && npm run test:callapi-state-change && npm run typecheck", "test:storyboards": "bash scripts/run-storyboards-matrix.sh", diff --git a/scripts/lint-storyboard-context-output-paths.cjs b/scripts/lint-storyboard-context-output-paths.cjs new file mode 100644 index 0000000000..70f83a5e0a --- /dev/null +++ b/scripts/lint-storyboard-context-output-paths.cjs @@ -0,0 +1,273 @@ +#!/usr/bin/env node +/** + * Validate every storyboard step's `context_outputs[].path` resolves to a + * field defined by its `response_schema_ref`. Catches the class of bug where + * a storyboard captures `rights[0].rights_id` from a response whose schema + * has no `rights` property — the path is wishful and a real agent's response + * silently misses the capture. + * + * Per #3918: storyboard-author drift between assertion and spec is a class + * we want to catch at build time, not at adopter-validation time. + * + * Coverage: every step under static/compliance/source/ that declares both + * `response_schema_ref` and `context_outputs[]`. Scope intentionally narrow — + * `validations[].path` is a separate generalisation tracked elsewhere. + * + * Rule: + * path_not_in_schema — `path` does not resolve to any defined property in + * the response schema (after $ref / oneOf / anyOf + * resolution and numeric-index descent into items). + */ + +'use strict'; + +const fs = require('node:fs'); +const path = require('node:path'); +const yaml = require('js-yaml'); + +const STORYBOARD_DIR = path.resolve(__dirname, '..', 'static', 'compliance', 'source'); +const SCHEMA_DIR = path.resolve(__dirname, '..', 'static', 'schemas', 'source'); +const ALLOWLIST_PATH = path.join(__dirname, 'storyboard-context-output-paths-allowlist.json'); + +/** + * The allowlist exists for legitimate captures the static lint can't verify + * against the spec schema alone — most commonly paths through error.details + * (polymorphic extension keyed on error.code, see error-handling.mdx) and + * runtime-convention echoes the spec leaves to additionalProperties: true. + * Each entry MUST carry a `reason` explaining why the lint can't verify the + * path; review during spec changes. + */ +function loadAllowlist() { + if (!fs.existsSync(ALLOWLIST_PATH)) return []; + const doc = JSON.parse(fs.readFileSync(ALLOWLIST_PATH, 'utf8')); + return Array.isArray(doc.allowlist) ? doc.allowlist : []; +} + +function isAllowlisted(allowlist, filePath, stepId, contextPath) { + const rel = path.relative(path.resolve(__dirname, '..'), filePath); + return allowlist.some( + (entry) => + entry.file === rel && + entry.step === stepId && + entry.path === contextPath, + ); +} + +function walkYaml(dir) { + const out = []; + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) out.push(...walkYaml(full)); + else if (entry.isFile() && (entry.name.endsWith('.yaml') || entry.name.endsWith('.yml'))) { + out.push(full); + } + } + return out; +} + +function parsePath(raw) { + if (!raw) return []; + return raw.replace(/\[(\d+)\]/g, '.$1').split('.').filter(Boolean); +} + +function schemaRefToPath(ref) { + if (!ref) return null; + const trimmed = ref.startsWith('/schemas/') ? ref.slice('/schemas/'.length) : ref; + return path.join(SCHEMA_DIR, trimmed); +} + +const schemaCache = new Map(); + +function loadSchema(ref) { + const full = schemaRefToPath(ref); + if (!full) return null; + if (schemaCache.has(full)) return schemaCache.get(full); + let doc = null; + try { + doc = JSON.parse(fs.readFileSync(full, 'utf8')); + } catch { + doc = null; + } + schemaCache.set(full, doc); + return doc; +} + +/** + * Walk a JSON Schema node to determine whether a dotted path resolves to + * any defined element. Follows `$ref`, descends through `properties.` + * for object steps and `items` for numeric-index steps, and accepts any + * variant of `oneOf` / `anyOf` / `allOf` that resolves. Returns true when + * EVERY segment was either resolved by a defined property/items or accepted + * by at least one composite variant. + * + * Empty path resolves trivially (the root itself exists). + */ +function pathResolves(node, segments, seen = new Set()) { + if (!node || typeof node !== 'object') return false; + if (segments.length === 0) return true; + + if (node.$ref) { + if (seen.has(node.$ref)) return false; + const next = new Set(seen); + next.add(node.$ref); + const resolved = loadSchema(node.$ref); + return pathResolves(resolved, segments, next); + } + + const [seg, ...rest] = segments; + + // Numeric — array index. Only valid when this node has `items`. + if (/^\d+$/.test(seg)) { + if (node.items && pathResolves(node.items, rest, seen)) return true; + } else if (node.properties && Object.prototype.hasOwnProperty.call(node.properties, seg)) { + if (pathResolves(node.properties[seg], rest, seen)) return true; + } + + // Composite variants — any variant that resolves the FULL remaining path + // (starting from the current segment, since composites apply at this node). + // Union-of-branches is correct for `oneOf`/`anyOf` (the resolved instance + // matches one variant, and is a path-existence question) AND for `allOf` + // (intersection of constraints means the resolved object carries the union + // of properties across branches — a property defined in any branch is + // present after merging). + const variants = node.oneOf || node.anyOf || node.allOf; + if (Array.isArray(variants)) { + for (const variant of variants) { + if (pathResolves(variant, segments, seen)) return true; + } + } + + return false; +} + +function* findContextOutputSteps(node, trail) { + if (Array.isArray(node)) { + for (let i = 0; i < node.length; i++) { + yield* findContextOutputSteps(node[i], [...trail, i]); + } + return; + } + if (node && typeof node === 'object') { + if ( + typeof node.response_schema_ref === 'string' && + Array.isArray(node.context_outputs) && + node.context_outputs.length > 0 + ) { + yield { + responseRef: node.response_schema_ref, + contextOutputs: node.context_outputs, + stepId: typeof node.id === 'string' ? node.id : null, + trail: [...trail], + }; + } + for (const key of Object.keys(node)) { + yield* findContextOutputSteps(node[key], [...trail, key]); + } + } +} + +function lintDoc(doc, filePath, allowlist = []) { + const violations = []; + if (!doc) return violations; + for (const step of findContextOutputSteps(doc, [])) { + const schema = loadSchema(step.responseRef); + if (!schema) { + violations.push({ + rule: 'response_schema_not_found', + filePath, + stepId: step.stepId, + responseRef: step.responseRef, + }); + continue; + } + for (let i = 0; i < step.contextOutputs.length; i++) { + const out = step.contextOutputs[i]; + const rawPath = out?.path; + if (typeof rawPath !== 'string' || rawPath.length === 0) continue; + const segments = parsePath(rawPath); + if (pathResolves(schema, segments)) continue; + if (isAllowlisted(allowlist, filePath, step.stepId, rawPath)) continue; + violations.push({ + rule: 'path_not_in_schema', + filePath, + stepId: step.stepId, + responseRef: step.responseRef, + contextPath: rawPath, + captureName: out?.key || out?.name || null, + index: i, + }); + } + } + return violations; +} + +function lint() { + const violations = []; + const files = walkYaml(STORYBOARD_DIR); + const allowlist = loadAllowlist(); + for (const file of files) { + let doc; + try { + doc = yaml.load(fs.readFileSync(file, 'utf8')); + } catch { + // YAML parse errors are surfaced by sibling lints + // (lint-storyboard-response-schema, etc.); skip rather than double-report. + continue; + } + violations.push(...lintDoc(doc, file, allowlist)); + } + return violations; +} + +const RULE_MESSAGES = { + path_not_in_schema: ({ contextPath, responseRef, captureName }) => { + const name = captureName ? ` (capture: \`${captureName}\`)` : ''; + return ( + `context_outputs path \`${contextPath}\`${name} does not resolve to any defined ` + + `field in \`${responseRef}\`. The storyboard captures from a path the spec ` + + 'schema does not define — a real agent response will silently miss this capture, ' + + 'and downstream `$context.` consumers will see undefined values.\n' + + ' Fix one of:\n' + + ' 1. Update the path to a field that exists in the response schema.\n' + + ' 2. Update the response_schema_ref to the schema that defines this path.\n' + + ' 3. If the path is conditional (e.g., only present on a discriminated arm), ' + + 'use a path that resolves through that arm\'s defined properties.' + ); + }, + response_schema_not_found: ({ responseRef }) => + `response_schema_ref \`${responseRef}\` could not be loaded — fix the ref or the schema path.`, +}; + +function formatMessage(violation) { + const builder = RULE_MESSAGES[violation.rule]; + return builder ? builder(violation) : `unknown rule ${violation.rule}`; +} + +function main() { + const violations = lint(); + if (violations.length === 0) { + const fileCount = walkYaml(STORYBOARD_DIR).length; + console.log(` storyboard context-output path lint: clean (${fileCount} storyboard files scanned)`); + return; + } + for (const v of violations) { + const rel = path.relative(path.resolve(__dirname, '..'), v.filePath); + const stepLabel = v.stepId ? `:${v.stepId}` : ''; + console.error(` error: ${rel}${stepLabel} — ${formatMessage(v)}`); + } + console.error(`\n ${violations.length} storyboard context_outputs path violation(s).`); + process.exit(1); +} + +if (require.main === module) main(); + +module.exports = { + RULE_MESSAGES, + lint, + lintDoc, + loadSchema, + loadAllowlist, + pathResolves, + parsePath, + formatMessage, +}; diff --git a/scripts/storyboard-context-output-paths-allowlist.json b/scripts/storyboard-context-output-paths-allowlist.json new file mode 100644 index 0000000000..3721d3142a --- /dev/null +++ b/scripts/storyboard-context-output-paths-allowlist.json @@ -0,0 +1,17 @@ +{ + "$comment": "Per-storyboard exceptions for lint-storyboard-context-output-paths.cjs. Each entry MUST carry a `reason` explaining why the lint can't statically verify the path against the response schema. Re-review during spec changes — if the polymorphism the lint can't follow gets first-class schema treatment, remove the entry.", + "allowlist": [ + { + "file": "static/compliance/source/universal/billing-gate-dispatch.yaml", + "step": "sync_accounts_passthrough_rejects_agent", + "path": "accounts[0].errors[0].details.suggested_billing", + "reason": "Captures from `error.details.suggested_billing`, which is defined in `error-details/billing-not-permitted-for-agent.json` (the per-error-code details schema selected by `error.code`). The lint can't follow error-details polymorphism keyed on the runtime error code — `core/error.json` declares `details` as `additionalProperties: true` because the structured shape lives in the error-details schemas. Capture is spec-conformant; lifting this requires extending the lint to switch on sibling `error.code` values." + }, + { + "file": "static/compliance/source/universal/idempotency.yaml", + "step": "create_media_buy_initial", + "path": "idempotency_key", + "reason": "Captures the seller's echo of the request `idempotency_key` for replay-comparison in the next step. Response schemas declare `additionalProperties: true` and don't list `idempotency_key` explicitly — the echo is a runtime convention the storyboard depends on, not a spec requirement. Idempotency-replay testing inherently asserts on the echo; removing the convention would require redesigning how the runner threads request-side values into downstream comparisons." + } + ] +} diff --git a/static/compliance/source/protocols/sponsored-intelligence/index.yaml b/static/compliance/source/protocols/sponsored-intelligence/index.yaml index 9846ad7bcc..1e0b14e19d 100644 --- a/static/compliance/source/protocols/sponsored-intelligence/index.yaml +++ b/static/compliance/source/protocols/sponsored-intelligence/index.yaml @@ -114,7 +114,7 @@ phases: context_outputs: - name: offering_id - path: 'offering_id' + path: 'offering.offering_id' validations: - check: response_schema description: "Response matches si-get-offering-response.json schema" diff --git a/tests/lint-storyboard-context-output-paths.test.cjs b/tests/lint-storyboard-context-output-paths.test.cjs new file mode 100644 index 0000000000..a05a30d6be --- /dev/null +++ b/tests/lint-storyboard-context-output-paths.test.cjs @@ -0,0 +1,169 @@ +#!/usr/bin/env node +/** + * Tests for the storyboard context-output path lint (issue #3918, CI gate + * item). Concerns: + * 1. Source-tree guard — every real storyboard under + * static/compliance/source passes the lint. Regression guard. + * 2. The `path_not_in_schema` rule fires for the canonical bug class — + * a context_outputs path that doesn't resolve to any defined field + * in the response schema (the offering_id / offering.offering_id typo + * that surfaced this lint). + * 3. The path resolver follows the bracket / dot equivalence and descends + * through oneOf / anyOf variants and items. + * 4. The allowlist mechanism suppresses entries for paths the lint can't + * statically verify (error.details polymorphism, additionalProperties + * runtime conventions). + */ + +'use strict'; + +const test = require('node:test'); +const assert = require('node:assert/strict'); + +const { + lint, + lintDoc, + pathResolves, + parsePath, + loadSchema, + loadAllowlist, +} = require('../scripts/lint-storyboard-context-output-paths.cjs'); + +test('source tree passes the context-output path lint', () => { + const violations = lint(); + assert.deepEqual( + violations, + [], + 'real storyboards have context_outputs path violations:\n' + + violations + .map((v) => ` ${v.filePath}:${v.stepId} — ${v.contextPath} (${v.rule})`) + .join('\n'), + ); +}); + +test('path_not_in_schema fires for the canonical offering_id typo', () => { + // Reproduce the bug this lint surfaced: si_get_offering captures + // `offering_id` from si-get-offering-response.json, but the schema defines + // `offering.offering_id` — top-level `offering_id` is undefined. + const doc = { + phases: [ + { + id: 'discovery', + steps: [ + { + id: 'si_get_offering', + task: 'si_get_offering', + schema_ref: 'sponsored-intelligence/si-get-offering-request.json', + response_schema_ref: 'sponsored-intelligence/si-get-offering-response.json', + context_outputs: [{ name: 'offering_id', path: 'offering_id' }], + }, + ], + }, + ], + }; + + const violations = lintDoc(doc, '/synth/test.yaml'); + assert.equal(violations.length, 1); + assert.equal(violations[0].rule, 'path_not_in_schema'); + assert.equal(violations[0].contextPath, 'offering_id'); + assert.equal(violations[0].captureName, 'offering_id'); +}); + +test('path_not_in_schema does not fire when the path resolves through nested objects', () => { + // Same storyboard with the corrected path — resolves through + // properties.offering.properties.offering_id. + const doc = { + phases: [ + { + id: 'discovery', + steps: [ + { + id: 'si_get_offering', + task: 'si_get_offering', + schema_ref: 'sponsored-intelligence/si-get-offering-request.json', + response_schema_ref: 'sponsored-intelligence/si-get-offering-response.json', + context_outputs: [{ name: 'offering_id', path: 'offering.offering_id' }], + }, + ], + }, + ], + }; + + const violations = lintDoc(doc, '/synth/test.yaml'); + assert.deepEqual(violations, []); +}); + +test('pathResolves descends oneOf variants — captures from a discriminated arm resolve', () => { + // acquire-rights-response.json is a 4-arm oneOf. `rights_id` is on three + // of the four arms (Acquired, PendingApproval, Rejected). The path + // resolver should accept it because at least one variant defines it. + const schema = loadSchema('brand/acquire-rights-response.json'); + assert.ok(schema, 'fixture schema loads'); + assert.equal(pathResolves(schema, parsePath('rights_id')), true); + assert.equal(pathResolves(schema, parsePath('status')), true); + assert.equal(pathResolves(schema, parsePath('reason')), true); +}); + +test('pathResolves rejects paths that no oneOf variant defines', () => { + const schema = loadSchema('brand/acquire-rights-response.json'); + assert.equal(pathResolves(schema, parsePath('does_not_exist')), false); + assert.equal(pathResolves(schema, parsePath('rights_id.subfield')), false); +}); + +test('pathResolves treats bracket and dotted notation as equivalent for array indexing', () => { + const schema = loadSchema('brand/acquire-rights-response.json'); + // `errors[0].code` and `errors.0.code` both descend through items into + // the error.json $ref's `code` property. + assert.equal(pathResolves(schema, parsePath('errors[0].code')), true); + assert.equal(pathResolves(schema, parsePath('errors.0.code')), true); +}); + +test('parsePath accepts both bracket and dotted forms', () => { + assert.deepEqual(parsePath('rights[0].rights_id'), ['rights', '0', 'rights_id']); + assert.deepEqual(parsePath('rights.0.rights_id'), ['rights', '0', 'rights_id']); + assert.deepEqual(parsePath(''), []); + assert.deepEqual(parsePath(null), []); +}); + +test('allowlist suppresses violations for documented exceptions', () => { + const doc = { + phases: [ + { + id: 'p', + steps: [ + { + id: 'allowed_step', + task: 'noop', + response_schema_ref: 'sponsored-intelligence/si-get-offering-response.json', + context_outputs: [{ name: 'x', path: 'definitely_not_real' }], + }, + ], + }, + ], + }; + + const allowlist = [ + { + file: 'tests/synth/allowed.yaml', + step: 'allowed_step', + path: 'definitely_not_real', + reason: 'synthesized for test', + }, + ]; + + const path = require('node:path'); + const ROOT = path.resolve(__dirname, '..'); + const filePath = path.join(ROOT, 'tests', 'synth', 'allowed.yaml'); + + const violations = lintDoc(doc, filePath, allowlist); + assert.deepEqual(violations, []); +}); + +test('loadAllowlist returns an array', () => { + const allowlist = loadAllowlist(); + assert.ok(Array.isArray(allowlist), 'allowlist is an array'); + for (const entry of allowlist) { + assert.ok(entry.reason, `every allowlist entry MUST carry a reason: ${JSON.stringify(entry)}`); + assert.ok(entry.file && entry.step && entry.path, `entry MUST identify file/step/path: ${JSON.stringify(entry)}`); + } +});