feat: workflow schema validator (Zod)#1529
feat: workflow schema validator (Zod)#1529arcaven wants to merge 9 commits intobmad-code-org:mainfrom
Conversation
…dator 27 test fixtures (11 valid + 16 invalid) and a test runner that exercises the forthcoming Zod schema for workflow.yaml files. Includes a stub schema that fails all validation, confirming 26 failing / 1 passing (yaml-parse-error) — TDD red state. Follows the validate-agent-schema pattern (tools/schema/agent.js, test/test-agent-schema.js). Ref: MSSCI-12749
Implement the Zod schema for workflow.yaml files and the CLI validator tool, completing the GREEN phase of TDD. - tools/schema/workflow.js: Zod schema with 5 required fields, template polymorphism (string|false), input_file_patterns with load_strategy enum, execution_hints, and instructions extension validation. Passthrough for varying optional string fields. - tools/validate-workflow-schema.js: CLI tool mirroring validate-agent-schema.js with --strict flag and GitHub Actions ::warning annotations. - package.json: Wire test:schemas and validate:schemas to include workflow schema validation. 27/27 tests passing. 12/13 real workflow files pass validation (qa/automate missing standalone — pre-existing upstream defect). Ref: MSSCI-12749
📝 WalkthroughWalkthroughA new workflow YAML schema validation system is introduced, including Zod-based schema definitions, a test fixture framework, a CLI validation tool, and comprehensive test coverage for both valid and invalid workflow configurations across 40+ fixture files. Changes
Sequence DiagramsequenceDiagram
participant FileSystem as File<br/>System
participant TestRunner as Test<br/>Runner
participant YAMLParser as YAML<br/>Parser
participant Validator as Schema<br/>Validator
participant Output as Test<br/>Output
loop For each workflow fixture
FileSystem->>TestRunner: Discover fixture file
TestRunner->>FileSystem: Read metadata (comments)
TestRunner->>FileSystem: Read YAML content
alt YAML Parses Successfully
FileSystem->>YAMLParser: Raw YAML content
YAMLParser->>YAMLParser: Parse YAML
YAMLParser->>Validator: Parsed workflow object
Validator->>Validator: Validate against schema
alt Validation Success
Validator-->>TestRunner: Valid result
TestRunner->>TestRunner: Compare to expected (PASS)
else Validation Fails
Validator-->>TestRunner: Error details
TestRunner->>TestRunner: Compare to expected<br/>(error code, path, type)
end
else YAML Parse Fails
YAMLParser-->>TestRunner: Parse error
TestRunner->>TestRunner: Compare to expected<br/>(expect FAIL flag)
end
TestRunner->>Output: Record test result<br/>(pass/fail)
end
Output->>Output: Categorize and<br/>aggregate results
Output-->>FileSystem: Print summary<br/>(counts, failures)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@test/test-workflow-schema.js`:
- Around line 33-35: parseTestMetadata currently calls fs.readFileSync without
handling IO errors, which can bubble up before runTest's try/catch and abort the
whole suite; update either parseTestMetadata to catch and return a safe default
(or throw a controlled error) or move the metadata read so it happens inside
runTest's existing try/catch. Specifically, wrap the fs.readFileSync and content
parsing in a try/catch inside parseTestMetadata (or call parseTestMetadata from
within runTest's try block) and ensure on error you return null/empty metadata
or rethrow a test-scoped error so only the individual test fails instead of the
entire suite; reference parseTestMetadata and runTest to locate where to apply
the change.
In `@tools/schema/workflow.js`:
- Around line 1-2: The project requires the Zod runtime (see the require('zod')
import and use of z in tools/schema/workflow.js), but zod is not declared in
package.json; add "zod" to the "dependencies" section of package.json (pick a
compatible semver, e.g., latest stable) and run npm install / yarn install so CI
and clean installs include the module.
In `@tools/validate-workflow-schema.js`:
- Around line 50-52: The annotation paths are computed using
path.relative(process.cwd(), filePath) which misaligns when a custom project
root is used; update the computation in the workflowFiles loop to base
relativePath on the configured root (e.g., path.relative(root, filePath) or
projectRoot) so GitHub Actions annotations reference the repository root
correctly — adjust any variable names (relativePath, root/projectRoot,
workflowFiles) accordingly and ensure downstream uses of relativePath continue
to work.
🧹 Nitpick comments (1)
test/fixtures/workflow-schema/invalid/missing-load-strategy.workflow.yaml (1)
1-14: Minor metadata inconsistency compared to other fixtures.This fixture tests a missing required field (
load_strategy), which Zod reports asinvalid_typewithreceived: undefined. For consistency with otherinvalid_typefixtures (likewrong-type-name.workflow.yaml), consider adding theError expectedandError receivedmetadata lines:# Expected: FAIL # Error code: invalid_type # Error path: input_file_patterns.epics.load_strategy # Error expected: 'FULL_LOAD' | 'SELECTIVE_LOAD' | 'INDEX_GUIDED' # Error received: undefined # Tests: AC18 — input_file_patterns entry missing load_strategy produces errorThis is a minor documentation consistency improvement and does not affect test correctness.
| // Zod schema definition for workflow.yaml files | ||
| const { z } = require('zod'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate package.json files
find . -name "package.json" -type f | head -20
# Search for zod in package.json
if [ -f "package.json" ]; then
echo "=== package.json content (relevant sections) ==="
cat package.json | grep -A 5 -B 5 '"zod"' || echo "zod not found in main package.json"
fiRepository: bmad-code-org/BMAD-METHOD
Length of output: 167
🏁 Script executed:
#!/bin/bash
# Check if zod is used in the workflow.js file
cat tools/schema/workflow.js | head -20
rg -n "z\." tools/schema/workflow.js | head -5Repository: bmad-code-org/BMAD-METHOD
Length of output: 991
Add zod to package.json dependencies
The module requires zod at runtime (line 2 and throughout) but it's not declared in package.json. This will cause failures in clean installs and CI environments. Add zod to the dependencies section in package.json.
🤖 Prompt for AI Agents
In `@tools/schema/workflow.js` around lines 1 - 2, The project requires the Zod
runtime (see the require('zod') import and use of z in
tools/schema/workflow.js), but zod is not declared in package.json; add "zod" to
the "dependencies" section of package.json (pick a compatible semver, e.g.,
latest stable) and run npm install / yarn install so CI and clean installs
include the module.
There was a problem hiding this comment.
I believe this may be a false positive — zod is already available as a transitive dependency through the existing packages, and the agent schema validator (tools/schema/agent.js) uses the same require('zod') pattern without declaring it directly in package.json. This PR intentionally mirrors that established pattern.
Happy to add an explicit dependency if that's preferred, but wanted to flag the existing precedent first. @alexeyv — would you like zod declared directly, or is the current transitive approach acceptable?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| for (const filePath of workflowFiles.sort()) { | ||
| const relativePath = path.relative(process.cwd(), filePath); | ||
|
|
There was a problem hiding this comment.
Align annotation paths with the scanned root
relativePath is derived from process.cwd(), so running with a non-default projectRoot (or from a different working directory) can emit paths that don’t map to the repo root in GitHub Actions annotations. Consider basing the relative path on root to keep warnings accurate.
🤖 Prompt for AI Agents
In `@tools/validate-workflow-schema.js` around lines 50 - 52, The annotation paths
are computed using path.relative(process.cwd(), filePath) which misaligns when a
custom project root is used; update the computation in the workflowFiles loop to
base relativePath on the configured root (e.g., path.relative(root, filePath) or
projectRoot) so GitHub Actions annotations reference the repository root
correctly — adjust any variable names (relativePath, root/projectRoot,
workflowFiles) accordingly and ensure downstream uses of relativePath continue
to work.
There was a problem hiding this comment.
This matches the existing agent schema validator, which uses the same path.relative(process.cwd(), filePath) pattern at tools/validate-agent-schema.js line 48.
In practice, the tool is always invoked via npm run validate:schemas (which sets cwd to the package root) or through GitHub Actions (which runs from the repo checkout root). The CLI doesn't accept a --root flag, so there's no supported invocation path where process.cwd() would differ from the project root.
If the project wants to align both validators to use an explicit root, happy to contribute that as a follow-up across both tools.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the validate-agent-schema.js file around line 48 to verify the user's claim
cat tools/validate-agent-schema.js | head -60 | tail -20
Length of output: 688
@arcaven You're absolutely right—I've confirmed that validate-agent-schema.js uses the same path.relative(process.cwd(), filePath) pattern on line 48. Given that both validators follow the same convention and the practical usage through npm run validate:schemas and CI always runs from the repo root, the current approach is consistent and works correctly.
Thanks for the context and for offering to contribute a follow-up if the project decides to align both validators to use an explicit root in the future. That sounds like a reasonable enhancement if it's ever needed, but the current implementation is fine.
|
Re: the nitpick on Not a consistency issue — it's a deliberate choice. The fixture tests what it needs to test (AC18). Adding more metadata would increase maintenance surface for zero value. The test runner treats |
Adds test coverage for the case where a YAML number value appears in the variables record (e.g., `count: 5`). YAML parses unquoted numbers as integers, which the schema correctly rejects as invalid_type (expected string, received number). This is a likely contributor mistake worth covering. 28/28 fixtures now passing.
The maintainer (Brian Madison) removed these fields from all workflow files in two commits: - f699a36: removed `standalone: true` from all workflow.yaml files - 731bee2: removed `web_bundle: false` from all workflow.yaml files These fields are no longer part of the workflow.yaml format. Update the schema to match upstream and remove related test fixtures. 24/24 tests passing. 9/9 workflow files valid.
|
Updated schema to match upstream changes. The
Schema and test fixtures updated accordingly. All 9 workflow files now pass validation. 24/24 workflow schema tests passing. |
|
I just realize this was all for checking workflow.yaml files - these are deprecated, no more are being added, and all that exist are being replaced. There is no need to add this as they will be gone within the next week - and many of the properties are not used anymore. |
|
Understood — closing this PR given the We'll adapt the validator approach for the new Thanks for the heads-up on the direction. |
…erences (#1573) * feat: extend validate-file-refs.js to scan CSV workflow-file references Add CSV file reference extraction to the Layer 1 validation pipeline, preventing broken _bmad/ workflow-file paths in module-help.csv files. Closes the gap identified after PR #1529 where CSV references were unvalidated despite being a source of repeat community issues. Refs: #1519 * fix: include test:refs in aggregate test script Add CSV file-ref extraction tests to the aggregate `npm test` pipeline, matching the existing pattern for test:schemas and test:install. Thanks to CodeRabbit for catching the omission. * fix: address review feedback on CSV validator extension - Surface CSV parse errors visibly instead of silently swallowing (no Layer 2c schema validator exists yet to catch these) - Add explanatory comments for the !VERBOSE logging pattern (non-verbose prints file headers only when issues found) - Add verbose-mode diagnostics for extensionless path handling ([SKIP] when nothing exists, [OK-DIR] for valid directories) * refactor: collect-then-print to eliminate confusing !VERBOSE pattern Replace the split header-printing logic (print early in verbose mode, print late in non-verbose mode with a !VERBOSE guard) with a simpler collect-then-print approach. Refs are now classified into ok[] and broken[] arrays first, then printed in a single location with one straightforward if/else if decision. Addresses alexeyv's review feedback about the counterintuitive "if not verbose, log" pattern. * feat: promote extensionless unresolved paths from silent skip to [UNRESOLVED] Paths without file extensions that don't exist as files or directories are now flagged as [UNRESOLVED] — a distinct tag from [BROKEN] (which means a file with a known extension wasn't found). Both count toward the broken reference total and appear in CI annotations. This catches real bugs like wrong directory names in installed_path metadata and dead invoke-workflow references to removed workflows. Extensionless paths that DO exist as directories are still [OK-DIR]. --------- Co-authored-by: Alex Verkhovsky <alexey.verkhovsky@gmail.com> Co-authored-by: Brian <bmadcode@gmail.com>
2 participants
Overview
This PR adds Layer 2 (Schema Validation) to the BMAD-METHOD structural validation pipeline — a layered CI system where each layer catches structural failures that lower layers cannot.
The layers are ordered to provide the greatest value to the project in sequence. So Layer 0 prevents more bugs, the layer 1 I simulated as accounting for 25% of all issues reported, each layer accounting for fewer %, but valuable in narrowing ways.
All layers are deterministic with zero false positives. Each runs as an independent CI job with its own
--strictflag and npm script. The pyramid is a conceptual model for coverage, not an execution dependency chain.Summary
Adds a Zod-based schema validator for
workflow.yamlfiles, following the exact pattern of the existingvalidate-agent-schema.js(PR #1494).tools/schema/workflow.js): Validates 5 required fields (name,description,author,standalone,web_bundle), template polymorphism (string | false),input_file_patternswithload_strategyenum (FULL_LOAD | SELECTIVE_LOAD | INDEX_GUIDED),execution_hints,tags,required_tools,variables, andinstructionsfile extension (.mdor.xml). Uses.passthrough()for the many varying optional string config fields.tools/validate-workflow-schema.js): Scanssrc/{core,bmm}/workflows/**/workflow.yaml, reports errors with::warningannotations in GitHub Actions, supports--strictflag for CI enforcement.npm run test:schemasandnpm run validate:schemasnow include workflow schema validation.Forensic validation
12/13 existing
workflow.yamlfiles pass. The one failure isqa/automate/workflow.yamlwhich is missing thestandalonefield — a pre-existing data issue, not a false positive. Please see Question: Is missing core/tasks/validate-workflow.xml returning? — related to #1324, #1351, #1455 #1530 for a discussion/question about this unrelated file ref validation issue.Additional failures detected during development related to standalone and web_bundle I deemed requiring removal from the schema based on their intentional removal in in commits f699a36 and 731bee2. The schema change and test fixtures have been updated to reflect these are no longer valid. Also fixes [BUG] Edited workflows requires schema update: standalone/web_bundle fields #1541
Design decisions
.strict()would break when new fields are added..passthrough()validates structural fields while allowing any additional string config.workflow.yamlfiles (13 files). Markdown workflow frontmatter (12 files with only 3 trivial fields) is deferred.--strictflag required for hard enforcement, matching the contributor-friendly approach.Why here and What's next
BMAD-METHOD is:
Errors introduced here bypass any protections bmad-builder might provide for the subset of the community who are using it to generate content. CI validators in this repo catch those errors at the PR stage, before they reach users. That's why the validation pipeline starts here.
Layer 3 (workflow graph validation) — step transitions, reachability, dangling reference detection — will follow as a separate PR.
I'm planning to offer these validators in bmad-builder for both CI protection of contributions and validation of generated output (see bmad-builder#7). I could shift to adding Layers 1–2 in bmad-builder before Layer 3 here, or complete the sequence, as you prefer.
sequenceDiagram participant BM as BMAD-METHOD participant BMB as bmad-builder Note over BM: ✅ Layer 0 — Formatting & Linting<br/>Existing Note over BM: ✅ Layer 1 — File Reference<br/>Validator PR 1494 merged rect rgba(0, 128, 255, 0.1) BM->>BM: Layer 2 — Schema Validator<br/>PR 1529 this PR end BM->>BM: Layer 3 — Graph Validator (planned, separate PR) Note over BM,BMB: ── community checkpoint ── BM-->>BMB: Extend validators with --installed mode<br/>CI validation of BMB source files BMB->>BMB: Post-generation validation<br/>Run Layers 1-3 on builder outputThank you for allowing me to offer this, glad to change scope, direction, design or sequencing as you prefer.
Test plan
npm run validate:schemas