From abaca0e037c70334e0a0726e1a5b3c073585e0b5 Mon Sep 17 00:00:00 2001 From: Archon Maintainer Bot Date: Wed, 5 Aug 2026 09:47:52 +0300 Subject: [PATCH 1/2] test(workflows): guard against workflow-level schema fields being silently dropped at parse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit parseWorkflow hand-assembles its result field by field, so a field added to workflowDefinitionSchema but not to that object literal is silently discarded: the YAML parses, the workflow loads, and the feature is inert. That already happened. `requires:` landed in workflowBaseSchema in ab81248d (2026-06-01) without touching the loader, and the assembly block only arrived in 2d7bf587 (2026-07-16) — six weeks in which the GitHub capability gate could never fire for a discovered workflow, fixed incidentally inside an unrelated PR. This is the third instance of one pattern: parallel enumerations that must agree with nothing enforcing agreement. The ref-surface enumerations carry a KEEP IN SYNC comment and were found broken anyway (#2450); the nested key sets are derived from each schema's .shape and cannot drift (#2455). This applies the derived form to the second case. The field list comes from workflowDefinitionSchema.shape, so a new schema field fails the test until it is given a fixture. Deliberately not solved by deriving the assembly itself — the hand assembly exists because of warn-and-drop, and schema.parse() would reject a bad field instead of logging and dropping it. The per-field assertion clears the mock logger first so it can tell the two failure causes apart: a warning means the fixture value is invalid (warn-and-drop working as designed), silence means a valid field was dropped (the actual bug). Verified by breaking it both ways: removing `requires` from the object literal reproduces the historical bug and fails with the right diagnosis, and adding a new schema key fails the ratchet until a fixture exists. --- packages/workflows/src/loader.test.ts | 144 ++++++++++++++++++++++++++ 1 file changed, 144 insertions(+) diff --git a/packages/workflows/src/loader.test.ts b/packages/workflows/src/loader.test.ts index f5173cdd73..f939e06419 100644 --- a/packages/workflows/src/loader.test.ts +++ b/packages/workflows/src/loader.test.ts @@ -35,6 +35,9 @@ registerBuiltinProviders(); import { discoverWorkflows, discoverWorkflowsWithConfig } from './workflow-discovery'; import { isBashNode, isCancelNode, isLoopNode } from './schemas'; +import { parseWorkflow } from './loader'; +import { workflowDefinitionSchema } from './schemas/workflow'; +import type { WorkflowDefinition } from './schemas/workflow'; import * as bundledDefaults from './defaults/bundled-defaults'; describe('Workflow Loader', () => { @@ -4074,3 +4077,144 @@ nodes: }); }); }); + +// --------------------------------------------------------------------------- +// Workflow-level field parity (#2457) +// --------------------------------------------------------------------------- + +/** + * `parseWorkflow` does not derive its result from `workflowDefinitionSchema` — it + * hand-assembles a WorkflowDefinition field by field into an object literal. A field + * added to the schema but not added to that literal is SILENTLY DISCARDED: the YAML + * parses, the workflow loads, and the feature is simply inert. + * + * That is not hypothetical. `requires:` was added to `workflowBaseSchema` in ab81248d + * (2026-06-01) without touching the loader, and the assembly block only landed in + * 2d7bf587 (2026-07-16) — six weeks in which the GitHub capability gate could never + * fire for any discovered workflow, fixed incidentally inside an unrelated PR. + * + * This is the guard. The field list is DERIVED from `workflowDefinitionSchema.shape`, + * so a new schema field fails the test until it is given a fixture here — the same + * "the derived check fails until the new thing is registered" ratchet used by + * `check:capability-matrix` and the schema-parity test in `sqlite.test.ts`. + * + * Deliberately NOT solved by deriving the assembly itself (`schema.parse(raw)`): the + * hand assembly exists BECAUSE of warn-and-drop — a present-but-invalid field is logged + * and dropped rather than aborting the whole discovery pass — and `.parse()` rejects + * instead. See #2457. + */ +describe('workflow-level field parity (#2457)', () => { + /** + * One fixture per workflow-level schema key: a YAML fragment setting the field, and a + * predicate proving it survived `parseWorkflow`. `present` is deliberately a survival + * check rather than deep equality — several fields are normalised on the way through + * (tags deduped, betas trimmed, thinking preprocessed), and this guard is about the + * field reaching the result at all, not about how it is parsed. + */ + const FIELD_FIXTURES: Record< + string, + { yaml: string; present: (w: WorkflowDefinition) => boolean } + > = { + name: { yaml: '', present: w => w.name === 'parity' }, + description: { yaml: '', present: w => w.description === 'parity fixture' }, + nodes: { yaml: '', present: w => w.nodes.length === 1 }, + provider: { yaml: 'provider: claude', present: w => w.provider === 'claude' }, + model: { yaml: 'model: sonnet', present: w => w.model === 'sonnet' }, + modelReasoningEffort: { + yaml: 'modelReasoningEffort: high', + present: w => w.modelReasoningEffort === 'high', + }, + webSearchMode: { yaml: 'webSearchMode: live', present: w => w.webSearchMode === 'live' }, + interactive: { yaml: 'interactive: true', present: w => w.interactive === true }, + effort: { yaml: 'effort: high', present: w => w.effort !== undefined }, + thinking: { yaml: 'thinking: adaptive', present: w => w.thinking !== undefined }, + fallbackModel: { + yaml: 'fallbackModel: haiku', + present: w => w.fallbackModel === 'haiku', + }, + betas: { yaml: 'betas:\n - some-beta', present: w => w.betas?.includes('some-beta') === true }, + sandbox: { yaml: 'sandbox:\n enabled: true', present: w => w.sandbox !== undefined }, + worktree: { yaml: 'worktree:\n enabled: false', present: w => w.worktree?.enabled === false }, + container: { + yaml: 'container:\n enabled: true', + present: w => w.container?.enabled === true, + }, + evidence_policy: { + yaml: 'evidence_policy:\n required: true', + present: w => w.evidence_policy?.required === true, + }, + mutates_checkout: { + yaml: 'mutates_checkout: false', + present: w => w.mutates_checkout === false, + }, + persist_sessions: { + yaml: 'persist_sessions: true', + present: w => w.persist_sessions === true, + }, + tags: { yaml: 'tags:\n - alpha', present: w => w.tags?.includes('alpha') === true }, + requires: { + yaml: 'requires:\n - github', + present: w => w.requires?.includes('github') === true, + }, + }; + + const schemaKeys = Object.keys(workflowDefinitionSchema.shape); + + it('has a fixture for every workflow-level schema key (the ratchet)', () => { + const missing = schemaKeys.filter(k => !(k in FIELD_FIXTURES)); + expect( + missing, + `Workflow-level schema keys with no parity fixture: ${missing.join(', ')}. ` + + 'Add a fixture in FIELD_FIXTURES AND make sure parseWorkflow actually carries the ' + + 'field into its returned object literal — a schema field missing from that literal ' + + 'is silently discarded at parse (see #2457).' + ).toEqual([]); + }); + + it('has no fixture for a key that is not in the schema', () => { + const stale = Object.keys(FIELD_FIXTURES).filter(k => !schemaKeys.includes(k)); + expect(stale, `Parity fixtures for keys no longer in the schema: ${stale.join(', ')}`).toEqual( + [] + ); + }); + + for (const key of Object.keys(FIELD_FIXTURES)) { + it(`round-trips '${key}' through parseWorkflow`, () => { + const fixture = FIELD_FIXTURES[key]; + const yaml = [ + 'name: parity', + 'description: parity fixture', + fixture.yaml, + 'nodes:', + ' - id: only', + ' prompt: hello', + ] + .filter(line => line !== '') + .join('\n'); + + // Warn-and-drop means an INVALID fixture value is dropped by design. Clearing the + // logger first lets the assertion below tell the two causes apart: a warn means the + // fixture is wrong, silence means the loader dropped a valid field (the #2457 bug). + mockLogger.warn.mockClear(); + + const result = parseWorkflow(yaml, `parity-${key}.yaml`); + expect( + result.error, + `parseWorkflow rejected the '${key}' fixture: ${result.error?.error}` + ).toBeNull(); + + const warned = mockLogger.warn.mock.calls.length > 0; + expect( + fixture.present(result.workflow as WorkflowDefinition), + warned + ? `Field '${key}' did not survive parseWorkflow, but a warning fired — the FIXTURE ` + + 'value above is almost certainly invalid for this field, which warn-and-drop ' + + 'discards by design. Fix the fixture, not the loader.' + : `Field '${key}' is declared on workflowDefinitionSchema, was accepted without a ` + + 'warning, and still did NOT survive parseWorkflow — so it is missing from the ' + + 'object literal parseWorkflow returns. That is the #2457 bug: add the field to ' + + 'that literal.' + ).toBe(true); + }); + } +}); From c078e604d1e19f20555d34cee728ec8e6f651b50 Mon Sep 17 00:00:00 2001 From: Archon Maintainer Bot Date: Wed, 5 Aug 2026 10:37:05 +0300 Subject: [PATCH 2/2] test(workflows): tighten the parity guard after review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses I1, I2, I3 and S1, S3, S4 from the review on #2459. No change to what the guard catches; all six make a precision tool more precise. I1 — the docblock claimed warn-and-drop universally. Re-verified the field audit against loader.ts rather than taking it on faith: 4 of the 20 hard-reject (name, description, nodes, evidence_policy at :619-629), 13 warn-and-drop, and 3 coerce silently with no log at all (provider :423, model :425, persist_sessions :473 — there is no invalid_provider/invalid_model/invalid_persist_sessions warn event anywhere in the file). Rewritten to say most rather than all, and to point at loader.ts as the authority instead of restating a per-field table that would rot the moment a field changes category. I3 — the two-branch failure message was backwards for exactly those 3 silent fields: a bad `provider: 123` fixture is discarded with no warning, so the message confidently blamed the loader and sent the reader into parseWorkflow when the fixture was at fault. That is the same failure the message exists to prevent, and the one I hit during development with a bad `thinking: true` fixture. Fixed by ranking rather than verdict: a warning is still strong evidence the fixture is wrong, but silence now names both causes and points at the fixture first. Chosen over listing the three exceptions in a comment, which would duplicate loader.ts and rot. This subsumes S2's unstated-invariant concern. I2 — effort, thinking and sandbox used presence checks where the other 17 fixtures check values, and their schemas transform deterministically, so exact checks are available. Verified by mutation: returning effort:'low' and thinking:{type:'disabled'} from the loader now fails both round-trips, where before it left them green. S1 — the hand-assembly literal predates 2d7bf587; only the requires entry landed there. Reworded so it cannot be skimmed as "the mechanism didn't exist until then". S3 — the two diagnostic strings moved out of the assertion into a named message. S4 — nodes?.length, so a dropped nodes yields a clean false instead of a TypeError. Verified: full validate green (132 batches, 0 fail); the I3 message re-checked by running a deliberately invalid provider fixture; I2 re-checked by mutation. --- packages/workflows/src/loader.test.ts | 51 +++++++++++++++------------ 1 file changed, 28 insertions(+), 23 deletions(-) diff --git a/packages/workflows/src/loader.test.ts b/packages/workflows/src/loader.test.ts index f939e06419..77c6d36d94 100644 --- a/packages/workflows/src/loader.test.ts +++ b/packages/workflows/src/loader.test.ts @@ -4089,7 +4089,7 @@ nodes: * parses, the workflow loads, and the feature is simply inert. * * That is not hypothetical. `requires:` was added to `workflowBaseSchema` in ab81248d - * (2026-06-01) without touching the loader, and the assembly block only landed in + * (2026-06-01) without touching the loader, and was not added to that literal until * 2d7bf587 (2026-07-16) — six weeks in which the GitHub capability gate could never * fire for any discovered workflow, fixed incidentally inside an unrelated PR. * @@ -4098,10 +4098,13 @@ nodes: * "the derived check fails until the new thing is registered" ratchet used by * `check:capability-matrix` and the schema-parity test in `sqlite.test.ts`. * - * Deliberately NOT solved by deriving the assembly itself (`schema.parse(raw)`): the - * hand assembly exists BECAUSE of warn-and-drop — a present-but-invalid field is logged - * and dropped rather than aborting the whole discovery pass — and `.parse()` rejects - * instead. See #2457. + * Deliberately NOT solved by deriving the assembly itself (`schema.parse(raw)`): most + * fields warn-and-drop, logging a present-but-invalid value and continuing rather than + * aborting the whole discovery pass, and `.parse()` would reject the workflow instead. + * That is not universal — a few fields deliberately hard-reject and a few coerce + * silently — but one warn-and-drop field is enough to make a blanket `.parse()` wrong. + * `loader.ts` is the authority on which field does what; do not restate it here. + * See #2457. */ describe('workflow-level field parity (#2457)', () => { /** @@ -4117,7 +4120,7 @@ describe('workflow-level field parity (#2457)', () => { > = { name: { yaml: '', present: w => w.name === 'parity' }, description: { yaml: '', present: w => w.description === 'parity fixture' }, - nodes: { yaml: '', present: w => w.nodes.length === 1 }, + nodes: { yaml: '', present: w => w.nodes?.length === 1 }, provider: { yaml: 'provider: claude', present: w => w.provider === 'claude' }, model: { yaml: 'model: sonnet', present: w => w.model === 'sonnet' }, modelReasoningEffort: { @@ -4126,14 +4129,14 @@ describe('workflow-level field parity (#2457)', () => { }, webSearchMode: { yaml: 'webSearchMode: live', present: w => w.webSearchMode === 'live' }, interactive: { yaml: 'interactive: true', present: w => w.interactive === true }, - effort: { yaml: 'effort: high', present: w => w.effort !== undefined }, - thinking: { yaml: 'thinking: adaptive', present: w => w.thinking !== undefined }, + effort: { yaml: 'effort: high', present: w => w.effort === 'high' }, + thinking: { yaml: 'thinking: adaptive', present: w => w.thinking?.type === 'adaptive' }, fallbackModel: { yaml: 'fallbackModel: haiku', present: w => w.fallbackModel === 'haiku', }, betas: { yaml: 'betas:\n - some-beta', present: w => w.betas?.includes('some-beta') === true }, - sandbox: { yaml: 'sandbox:\n enabled: true', present: w => w.sandbox !== undefined }, + sandbox: { yaml: 'sandbox:\n enabled: true', present: w => w.sandbox?.enabled === true }, worktree: { yaml: 'worktree:\n enabled: false', present: w => w.worktree?.enabled === false }, container: { yaml: 'container:\n enabled: true', @@ -4192,9 +4195,11 @@ describe('workflow-level field parity (#2457)', () => { .filter(line => line !== '') .join('\n'); - // Warn-and-drop means an INVALID fixture value is dropped by design. Clearing the - // logger first lets the assertion below tell the two causes apart: a warn means the - // fixture is wrong, silence means the loader dropped a valid field (the #2457 bug). + // An INVALID fixture value is dropped by design, which looks identical to the bug + // this test hunts. Clearing the logger first lets the failure message rank the two + // causes: a warning is strong evidence the fixture is at fault. Silence is NOT + // proof of the opposite — a few fields coerce an invalid value away with no log at + // all — so the silent branch names both causes rather than rendering a verdict. mockLogger.warn.mockClear(); const result = parseWorkflow(yaml, `parity-${key}.yaml`); @@ -4204,17 +4209,17 @@ describe('workflow-level field parity (#2457)', () => { ).toBeNull(); const warned = mockLogger.warn.mock.calls.length > 0; - expect( - fixture.present(result.workflow as WorkflowDefinition), - warned - ? `Field '${key}' did not survive parseWorkflow, but a warning fired — the FIXTURE ` + - 'value above is almost certainly invalid for this field, which warn-and-drop ' + - 'discards by design. Fix the fixture, not the loader.' - : `Field '${key}' is declared on workflowDefinitionSchema, was accepted without a ` + - 'warning, and still did NOT survive parseWorkflow — so it is missing from the ' + - 'object literal parseWorkflow returns. That is the #2457 bug: add the field to ' + - 'that literal.' - ).toBe(true); + const message = warned + ? `Field '${key}' did not survive parseWorkflow, and a warning fired — the FIXTURE ` + + 'value above is almost certainly invalid for this field, which warn-and-drop ' + + 'discards by design. Fix the fixture, not the loader.' + : `Field '${key}' is declared on workflowDefinitionSchema and did NOT survive ` + + 'parseWorkflow, with no warning logged. Two possible causes, likeliest first: ' + + "(1) the field is missing from the object literal parseWorkflow returns — that's " + + 'the #2457 bug, add it there; or (2) the fixture value is invalid for a field ' + + 'that coerces silently without logging, in which case fix the fixture. Check the ' + + 'fixture value against the schema first — it is the cheaper of the two to rule out.'; + expect(fixture.present(result.workflow as WorkflowDefinition), message).toBe(true); }); } });