diff --git a/.changeset/storyboard-expected-arm-annotation.md b/.changeset/storyboard-expected-arm-annotation.md new file mode 100644 index 0000000000..c677ee699f --- /dev/null +++ b/.changeset/storyboard-expected-arm-annotation.md @@ -0,0 +1,32 @@ +--- +--- + +ci(storyboards): `expected_arm` annotation for oneOf disambiguation in path lints + +Phase 2d follow-up from #3918's expert review. Closes the last item from the original meta-issue's expert-review follow-ups. + +Both path lints (context-output-paths and validations-paths) currently accept any `oneOf` arm when validating a step's paths — the right default for "no prior info." But when a storyboard step semantically expects a specific arm (e.g., `acquire_rights` should return `AcquireRightsAcquired` on the success path), today's lints accept paths that resolve through a *different* arm. A storyboard could capture `terms` (Acquired-arm-only) on a step that at runtime returns `AcquireRightsRejected` and the lint says nothing. + +### What this PR adds + +1. **Storyboard step field `expected_arm: `** (documented in `static/compliance/source/universal/storyboard-schema.yaml`). When present, both lints restrict path resolution to the matching `oneOf`/`anyOf` branch only. Match rule: any property in the branch declares `const: ""` (covers `status` for rights/media-buy responses, `type` / `state` for others). +2. **Inferred Error-arm restriction when `expect_error: true` is set without an explicit `expected_arm`.** Most response schemas have an Error arm whose `required` list includes `errors` and which has no const-style discriminator (e.g., `AcquireRightsError`). The lint walks `oneOf` for that shape and restricts to it. Saves storyboard authors from threading `expected_arm: "error"` on every error step. +3. **`unknown_expected_arm` violation rule** when an author names an arm value that doesn't match any branch — surfaces typos at storyboard-author time. + +### Behavior summary + +| Step state | Arm restriction | +|---|---| +| `expected_arm: ""` set | Restricted to matching branch (`unknown_expected_arm` if none) | +| `expect_error: true` only | Restricted to Error arm (if findable) | +| Neither | Any `oneOf` arm (current behavior) | + +### Code organization + +The arm-resolution helpers (`findArmByDiscriminator`, `findErrorArm`, `resolveExpectedArmSchema`) are duplicated across the two lint scripts rather than extracted to a shared module. Same trade-off as PR #3942 — the two lints have semantically different scopes (capture vs. assert) and forcing a shared module is premature. Future refactor when a third lint surfaces. + +### Test plan + +- [x] `npm run test:storyboard-validations-paths` (20 tests pass — 7 new for arm filtering, error-arm inference, `unknown_expected_arm` violations, and full-schema fallback) +- [x] `npm run test:storyboard-context-output-paths` (14 tests pass — 3 new for the same behaviors on the capture-side lint) +- [x] Both lints clean across all 82 storyboard files — adding the rule didn't introduce regressions, meaning no existing storyboard relied on `expect_error: true` looseness to capture from non-error arms. diff --git a/scripts/lint-storyboard-context-output-paths.cjs b/scripts/lint-storyboard-context-output-paths.cjs index 1d6668d030..a09f59a627 100644 --- a/scripts/lint-storyboard-context-output-paths.cjs +++ b/scripts/lint-storyboard-context-output-paths.cjs @@ -185,6 +185,8 @@ function* findContextOutputSteps(node, trail) { responseRef: node.response_schema_ref, contextOutputs: node.context_outputs, stepId: typeof node.id === 'string' ? node.id : null, + expectedArm: typeof node.expected_arm === 'string' ? node.expected_arm : null, + expectError: node.expect_error === true, trail: [...trail], }; } @@ -194,12 +196,81 @@ function* findContextOutputSteps(node, trail) { } } +/** + * Mirrors `findArmByDiscriminator` in lint-storyboard-validations-paths.cjs. + * See that file for the full design rationale of `expected_arm` annotations. + */ +function findArmByDiscriminator(node, expectedArm, seen = new Set()) { + if (!node || typeof node !== 'object') return null; + if (node.$ref) { + if (seen.has(node.$ref)) return null; + const next = new Set(seen); + next.add(node.$ref); + return findArmByDiscriminator(loadSchema(node.$ref), expectedArm, next); + } + const variants = node.oneOf || node.anyOf; + if (!Array.isArray(variants)) return null; + for (const variant of variants) { + if (!variant || typeof variant !== 'object') continue; + const props = variant.properties || {}; + for (const propName of Object.keys(props)) { + const propSchema = props[propName]; + if (propSchema && propSchema.const === expectedArm) { + return variant; + } + } + } + return null; +} + +/** Mirrors `findErrorArm` in lint-storyboard-validations-paths.cjs. */ +function findErrorArm(node, seen = new Set()) { + if (!node || typeof node !== 'object') return null; + if (node.$ref) { + if (seen.has(node.$ref)) return null; + const next = new Set(seen); + next.add(node.$ref); + return findErrorArm(loadSchema(node.$ref), next); + } + const variants = node.oneOf || node.anyOf; + if (!Array.isArray(variants)) return null; + for (const variant of variants) { + if (!variant || typeof variant !== 'object') continue; + const required = Array.isArray(variant.required) ? variant.required : []; + if (!required.includes('errors')) continue; + const props = variant.properties || {}; + let hasConstDiscriminator = false; + for (const propName of Object.keys(props)) { + if (props[propName] && props[propName].const !== undefined) { + hasConstDiscriminator = true; + break; + } + } + if (!hasConstDiscriminator) return variant; + } + return null; +} + +/** Mirrors `resolveExpectedArmSchema` in lint-storyboard-validations-paths.cjs. */ +function resolveExpectedArmSchema(schema, expectedArm, expectError) { + if (typeof expectedArm === 'string' && expectedArm.length > 0) { + const arm = findArmByDiscriminator(schema, expectedArm); + if (!arm) return { error: 'unknown_expected_arm' }; + return { schema: arm }; + } + if (expectError) { + const arm = findErrorArm(schema); + if (arm) return { schema: arm }; + } + return { schema }; +} + function lintDoc(doc, filePath, allowlist = []) { const violations = []; if (!doc) return violations; for (const step of findContextOutputSteps(doc, [])) { - const schema = loadSchema(step.responseRef); - if (!schema) { + const fullSchema = loadSchema(step.responseRef); + if (!fullSchema) { violations.push({ rule: 'response_schema_not_found', filePath, @@ -208,6 +279,18 @@ function lintDoc(doc, filePath, allowlist = []) { }); continue; } + const armResult = resolveExpectedArmSchema(fullSchema, step.expectedArm, step.expectError); + if (armResult.error === 'unknown_expected_arm') { + violations.push({ + rule: 'unknown_expected_arm', + filePath, + stepId: step.stepId, + responseRef: step.responseRef, + expectedArm: step.expectedArm, + }); + continue; + } + const schema = armResult.schema; for (let i = 0; i < step.contextOutputs.length; i++) { const out = step.contextOutputs[i]; const rawPath = out?.path; @@ -264,6 +347,10 @@ const RULE_MESSAGES = { }, response_schema_not_found: ({ responseRef }) => `response_schema_ref \`${responseRef}\` could not be loaded — fix the ref or the schema path.`, + unknown_expected_arm: ({ expectedArm, responseRef }) => + `expected_arm \`${expectedArm}\` does not match any oneOf/anyOf branch in \`${responseRef}\`. ` + + 'Match rule: a branch must declare some property with `const: ""`. ' + + 'Verify the discriminator value against the response schema.', }; function formatMessage(violation) { @@ -296,6 +383,9 @@ module.exports = { loadSchema, loadAllowlist, pathResolves, + findArmByDiscriminator, + findErrorArm, + resolveExpectedArmSchema, parsePath, formatMessage, }; diff --git a/scripts/lint-storyboard-validations-paths.cjs b/scripts/lint-storyboard-validations-paths.cjs index 295302ffc4..e15e5453f7 100644 --- a/scripts/lint-storyboard-validations-paths.cjs +++ b/scripts/lint-storyboard-validations-paths.cjs @@ -196,6 +196,8 @@ function* findStepsWithValidations(node, trail) { responseRef: node.response_schema_ref, validations: node.validations, stepId: typeof node.id === 'string' ? node.id : null, + expectedArm: typeof node.expected_arm === 'string' ? node.expected_arm : null, + expectError: node.expect_error === true, trail: [...trail], }; } @@ -205,6 +207,88 @@ function* findStepsWithValidations(node, trail) { } } +/** + * Walk a node's `oneOf` / `anyOf` (after $ref / allOf flattening) looking + * for the variant whose discriminator matches `expectedArm`. The match rule: + * any property in that variant declares `const: ""`. Returns + * the matching variant, or null when no variant matches (an authoring bug + * the lint surfaces as `unknown_expected_arm`). + */ +function findArmByDiscriminator(node, expectedArm, seen = new Set()) { + if (!node || typeof node !== 'object') return null; + if (node.$ref) { + if (seen.has(node.$ref)) return null; + const next = new Set(seen); + next.add(node.$ref); + return findArmByDiscriminator(loadSchema(node.$ref), expectedArm, next); + } + const variants = node.oneOf || node.anyOf; + if (!Array.isArray(variants)) return null; + for (const variant of variants) { + if (!variant || typeof variant !== 'object') continue; + const props = variant.properties || {}; + for (const propName of Object.keys(props)) { + const propSchema = props[propName]; + if (propSchema && propSchema.const === expectedArm) { + return variant; + } + } + } + return null; +} + +/** + * Find the response schema's "Error arm" — the oneOf branch whose `required` + * list includes `errors` and which has no const discriminator (so it isn't + * already const-tagged with a status value). Used as a fallback when a step + * has `expect_error: true` but no explicit `expected_arm`. + */ +function findErrorArm(node, seen = new Set()) { + if (!node || typeof node !== 'object') return null; + if (node.$ref) { + if (seen.has(node.$ref)) return null; + const next = new Set(seen); + next.add(node.$ref); + return findErrorArm(loadSchema(node.$ref), next); + } + const variants = node.oneOf || node.anyOf; + if (!Array.isArray(variants)) return null; + for (const variant of variants) { + if (!variant || typeof variant !== 'object') continue; + const required = Array.isArray(variant.required) ? variant.required : []; + if (!required.includes('errors')) continue; + const props = variant.properties || {}; + let hasConstDiscriminator = false; + for (const propName of Object.keys(props)) { + if (props[propName] && props[propName].const !== undefined) { + hasConstDiscriminator = true; + break; + } + } + if (!hasConstDiscriminator) return variant; + } + return null; +} + +/** + * Resolve which schema node the lint should validate paths against, given + * the step's `expected_arm` and `expect_error` annotations. Returns either: + * { schema: } — proceed normally with this schema (full or arm-restricted) + * { error: 'unknown_expected_arm' } — author named an arm that doesn't exist + */ +function resolveExpectedArmSchema(schema, expectedArm, expectError) { + if (typeof expectedArm === 'string' && expectedArm.length > 0) { + const arm = findArmByDiscriminator(schema, expectedArm); + if (!arm) return { error: 'unknown_expected_arm' }; + return { schema: arm }; + } + if (expectError) { + const arm = findErrorArm(schema); + if (arm) return { schema: arm }; + } + return { schema }; +} + function pathResolvesAgainstResponseOrEnvelope(schema, segments) { if (pathResolves(schema, segments)) return true; // Fall back to the protocol envelope. Storyboards address envelope-level @@ -223,8 +307,8 @@ function lintDoc(doc, filePath, allowlist = []) { const violations = []; if (!doc) return violations; for (const step of findStepsWithValidations(doc, [])) { - const schema = loadSchema(step.responseRef); - if (!schema) { + const fullSchema = loadSchema(step.responseRef); + if (!fullSchema) { violations.push({ rule: 'response_schema_not_found', filePath, @@ -233,6 +317,18 @@ function lintDoc(doc, filePath, allowlist = []) { }); continue; } + const armResult = resolveExpectedArmSchema(fullSchema, step.expectedArm, step.expectError); + if (armResult.error === 'unknown_expected_arm') { + violations.push({ + rule: 'unknown_expected_arm', + filePath, + stepId: step.stepId, + responseRef: step.responseRef, + expectedArm: step.expectedArm, + }); + continue; + } + const schema = armResult.schema; for (let i = 0; i < step.validations.length; i++) { const v = step.validations[i]; if (!v || typeof v !== 'object') continue; @@ -288,6 +384,10 @@ const RULE_MESSAGES = { '`scripts/storyboard-validations-paths-allowlist.json` with a `reason` string.', response_schema_not_found: ({ responseRef }) => `response_schema_ref \`${responseRef}\` could not be loaded — fix the ref or the schema path.`, + unknown_expected_arm: ({ expectedArm, responseRef }) => + `expected_arm \`${expectedArm}\` does not match any oneOf/anyOf branch in \`${responseRef}\`. ` + + 'Match rule: a branch must declare some property with `const: ""`. ' + + 'Verify the discriminator value against the response schema.', }; function formatMessage(violation) { @@ -324,6 +424,9 @@ module.exports = { isEnvelopeProperty, pathResolves, pathResolvesAgainstResponseOrEnvelope, + findArmByDiscriminator, + findErrorArm, + resolveExpectedArmSchema, parsePath, formatMessage, }; diff --git a/static/compliance/source/universal/storyboard-schema.yaml b/static/compliance/source/universal/storyboard-schema.yaml index 9880936f6a..45001434fb 100644 --- a/static/compliance/source/universal/storyboard-schema.yaml +++ b/static/compliance/source/universal/storyboard-schema.yaml @@ -488,6 +488,34 @@ # sparingly; prefer `negative_path: schema_invalid` for intentionally malformed # negative-path fixtures.) # +# expected_arm: string (optional — names the discriminator value of the `oneOf` +# arm this step expects the agent to return. When present, both the +# context-output-paths and validations-paths lints restrict path resolution +# to the matching arm only. Default behavior (no annotation) accepts a path +# that resolves through ANY oneOf arm — the right call for "no prior info," +# but it lets a step capture or assert on a path that's defined on, say, the +# Acquired arm even when the storyboard semantically expects the Rejected +# arm. The annotation tightens that. +# +# Match shape: the lint walks each `oneOf` / `anyOf` branch and looks for +# ANY property with `const: ""`. The first matching branch +# is the expected arm; subsequent path resolution restricts to it. +# +# Examples: +# expected_arm: "acquired" # matches AcquireRightsAcquired (status.const = "acquired") +# expected_arm: "rejected" # matches AcquireRightsRejected +# expected_arm: "pending_approval" # matches AcquireRightsPendingApproval +# +# When `expect_error: true` is set without `expected_arm`, the lint also +# tries to find an Error arm (one whose `required` list includes `errors` +# and which has no `status` const) and restricts to it. This covers +# `acquire_rights` / `creative_approval` / similar response shapes whose +# Error arm isn't const-discriminated. +# +# When the named arm doesn't match any branch, the lint emits an +# `unknown_expected_arm` violation — an authoring bug to fix at storyboard +# author time, not at runtime. +# # --- Phase (optional fields for conditional execution) --- # # optional: boolean (default false — see semantics below) diff --git a/tests/lint-storyboard-context-output-paths.test.cjs b/tests/lint-storyboard-context-output-paths.test.cjs index 8714dfa7a0..e53c08525a 100644 --- a/tests/lint-storyboard-context-output-paths.test.cjs +++ b/tests/lint-storyboard-context-output-paths.test.cjs @@ -27,6 +27,8 @@ const { parsePath, loadSchema, loadAllowlist, + findArmByDiscriminator, + findErrorArm, } = require('../scripts/lint-storyboard-context-output-paths.cjs'); test('source tree passes the context-output path lint', () => { @@ -183,6 +185,80 @@ test('allowlist suppresses violations for documented exceptions', () => { assert.deepEqual(violations, []); }); +test('expected_arm: "acquired" restricts capture path resolution', () => { + // get_rights stores rights[0].rights_id; acquire_rights captures rights_id + // from the SAME response. With expected_arm: "acquired", capturing `terms` + // resolves (Acquired arm) but capturing `reason` does not (only Rejected). + const docOk = { + phases: [ + { + id: 'p', + steps: [ + { + id: 's', + task: 'acquire_rights', + response_schema_ref: 'brand/acquire-rights-response.json', + expected_arm: 'acquired', + context_outputs: [{ name: 'rights_id', path: 'rights_id' }], + }, + ], + }, + ], + }; + assert.deepEqual(lintDoc(docOk, '/synth/test.yaml'), []); + + const docWrongArm = { + phases: [ + { + id: 'p', + steps: [ + { + id: 's', + task: 'acquire_rights', + response_schema_ref: 'brand/acquire-rights-response.json', + expected_arm: 'acquired', + // `reason` is on Rejected arm only + context_outputs: [{ name: 'r', path: 'reason' }], + }, + ], + }, + ], + }; + const violations = lintDoc(docWrongArm, '/synth/test.yaml'); + assert.equal(violations.length, 1); + assert.equal(violations[0].rule, 'path_not_in_schema'); +}); + +test('unknown_expected_arm fires for context_outputs steps too', () => { + const doc = { + phases: [ + { + id: 'p', + steps: [ + { + id: 's', + task: 'acquire_rights', + response_schema_ref: 'brand/acquire-rights-response.json', + expected_arm: 'no_such_arm', + context_outputs: [{ name: 'x', path: 'rights_id' }], + }, + ], + }, + ], + }; + const violations = lintDoc(doc, '/synth/test.yaml'); + assert.equal(violations.length, 1); + assert.equal(violations[0].rule, 'unknown_expected_arm'); +}); + +test('findArmByDiscriminator and findErrorArm are exported and consistent across lints', () => { + const schema = loadSchema('brand/acquire-rights-response.json'); + assert.ok(findArmByDiscriminator(schema, 'acquired')); + assert.ok(findArmByDiscriminator(schema, 'rejected')); + assert.equal(findArmByDiscriminator(schema, 'no_match'), null); + assert.ok(findErrorArm(schema)); +}); + test('loadAllowlist returns an array', () => { const allowlist = loadAllowlist(); assert.ok(Array.isArray(allowlist), 'allowlist is an array'); diff --git a/tests/lint-storyboard-validations-paths.test.cjs b/tests/lint-storyboard-validations-paths.test.cjs index cb33da911f..ca099cd0c6 100644 --- a/tests/lint-storyboard-validations-paths.test.cjs +++ b/tests/lint-storyboard-validations-paths.test.cjs @@ -29,6 +29,9 @@ const { loadSchema, loadAllowlist, isEnvelopeProperty, + findArmByDiscriminator, + findErrorArm, + resolveExpectedArmSchema, PATH_BEARING_CHECKS, } = require('../scripts/lint-storyboard-validations-paths.cjs'); @@ -261,6 +264,149 @@ test('allowlist suppresses violations for documented exceptions', () => { assert.deepEqual(violations, []); }); +test('expected_arm restricts path resolution to the matching oneOf arm', () => { + const schema = loadSchema('brand/acquire-rights-response.json'); + const acquired = findArmByDiscriminator(schema, 'acquired'); + assert.ok(acquired, 'acquired arm found'); + // `terms` is on the Acquired arm only + assert.equal(pathResolves(acquired, parsePath('terms')), true); + // `reason` is only on the Rejected arm — NOT on Acquired, so should fail + assert.equal(pathResolves(acquired, parsePath('reason')), false); + + const rejected = findArmByDiscriminator(schema, 'rejected'); + assert.ok(rejected, 'rejected arm found'); + assert.equal(pathResolves(rejected, parsePath('reason')), true); + assert.equal(pathResolves(rejected, parsePath('terms')), false); +}); + +test('findErrorArm finds the errors[]-required branch with no const discriminator', () => { + const schema = loadSchema('brand/acquire-rights-response.json'); + const errorArm = findErrorArm(schema); + assert.ok(errorArm, 'error arm found'); + // Error arm has `errors` required and no const status + assert.equal(pathResolves(errorArm, parsePath('errors[0].code')), true); + // Error arm has no rights_id + assert.equal(pathResolves(errorArm, parsePath('rights_id')), false); +}); + +test('expect_error: true without expected_arm restricts to the Error arm', () => { + const doc = { + phases: [ + { + id: 'p', + steps: [ + { + id: 's', + task: 'acquire_rights', + response_schema_ref: 'brand/acquire-rights-response.json', + expect_error: true, + // rights_id is on Acquired/PendingApproval/Rejected but NOT on Error. + // With expect_error: true, the lint should restrict to Error and flag this. + validations: [{ check: 'field_present', path: 'rights_id' }], + }, + ], + }, + ], + }; + + const violations = lintDoc(doc, '/synth/test.yaml'); + assert.equal(violations.length, 1); + assert.equal(violations[0].rule, 'path_not_in_schema'); +}); + +test('expect_error: true paths to errors[].code resolve correctly', () => { + const doc = { + phases: [ + { + id: 'p', + steps: [ + { + id: 's', + task: 'acquire_rights', + response_schema_ref: 'brand/acquire-rights-response.json', + expect_error: true, + validations: [{ check: 'field_present', path: 'errors[0].code' }], + }, + ], + }, + ], + }; + + const violations = lintDoc(doc, '/synth/test.yaml'); + assert.deepEqual(violations, []); +}); + +test('expected_arm: "acquired" restricts path resolution', () => { + const docOk = { + phases: [ + { + id: 'p', + steps: [ + { + id: 's', + task: 'acquire_rights', + response_schema_ref: 'brand/acquire-rights-response.json', + expected_arm: 'acquired', + validations: [{ check: 'field_present', path: 'terms' }], + }, + ], + }, + ], + }; + assert.deepEqual(lintDoc(docOk, '/synth/test.yaml'), []); + + const docWrongArm = { + phases: [ + { + id: 'p', + steps: [ + { + id: 's', + task: 'acquire_rights', + response_schema_ref: 'brand/acquire-rights-response.json', + expected_arm: 'acquired', + // `reason` is on the Rejected arm, NOT on Acquired + validations: [{ check: 'field_present', path: 'reason' }], + }, + ], + }, + ], + }; + const violations = lintDoc(docWrongArm, '/synth/test.yaml'); + assert.equal(violations.length, 1); + assert.equal(violations[0].rule, 'path_not_in_schema'); +}); + +test('unknown_expected_arm fires when no oneOf branch matches', () => { + const doc = { + phases: [ + { + id: 'p', + steps: [ + { + id: 's', + task: 'acquire_rights', + response_schema_ref: 'brand/acquire-rights-response.json', + expected_arm: 'no_such_arm_value', + validations: [{ check: 'field_present', path: 'terms' }], + }, + ], + }, + ], + }; + const violations = lintDoc(doc, '/synth/test.yaml'); + assert.equal(violations.length, 1); + assert.equal(violations[0].rule, 'unknown_expected_arm'); + assert.equal(violations[0].expectedArm, 'no_such_arm_value'); +}); + +test('resolveExpectedArmSchema returns full schema when no annotation present', () => { + const schema = loadSchema('brand/acquire-rights-response.json'); + const result = resolveExpectedArmSchema(schema, null, false); + assert.equal(result.schema, schema); + assert.equal(result.error, undefined); +}); + test('loadAllowlist enforces a reason field', () => { const allowlist = loadAllowlist(); assert.ok(Array.isArray(allowlist), 'allowlist is an array');