feat: extend Layer 1 file-ref validator to scan CSV workflow-file references #1573
Conversation
📝 WalkthroughWalkthroughThe PR extends the file-reference validator with CSV support, adding CSV parsing capabilities to extract references from CSV files with workflow columns, a corresponding test suite using fixtures to validate the CSV extraction logic, a new npm script to run the tests, and documentation updates describing the new validation capability. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
🤖 Fix all issues with AI agents
In `@package.json`:
- Around line 48-52: The aggregate "test" script in package.json omits the new
"test:refs" task so npm test won't run the CSV extraction tests; update the
"test" script to include "npm run test:refs" (e.g., add "&& npm run test:refs"
into the "test" script sequence alongside "test:schemas", "test:install",
"validate:schemas", "lint", "lint:md", and "format:check") so that running npm
test executes the "test:refs" job as well.
🧹 Nitpick comments (2)
tools/validate-file-refs.js (2)
296-327: CSV refs always typed asproject-root— consider future-proofing.All extracted CSV refs are hardcoded to
type: 'project-root'(Line 323). If aworkflow-filecell ever contains a relative path (e.g.,./local-workflow.md),resolveRefwould route it throughmapInstalledToSource, which would produce an incorrect resolved path. This is fine for v1 scope limited to_bmad/prefixed paths, but worth a comment or a guard.Optional: detect relative refs
if (!isResolvable(raw)) continue; // Line = header (1) + data row index (0-based) + 1 const line = i + 2; - refs.push({ file: filePath, raw, type: 'project-root', line }); + const type = raw.startsWith('./') || raw.startsWith('../') ? 'relative' : 'project-root'; + refs.push({ file: filePath, raw, type, line });
388-389: OnlyextractCsvRefsis exported — intentional?
extractYamlRefsandextractMarkdownRefsare not exported, which means they can't be unit-tested in isolation. If this is intentional for now that's fine, but consider exporting them too for consistency and testability.
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
🤖 Augment PR SummarySummary: This PR extends the Layer 1 file reference validator to detect broken workflow file references inside CSV catalogs. Changes:
Technical Notes: CSV scanning is currently focused on the 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
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 bmad-code-org#1529 where CSV references were unvalidated despite being a source of repeat community issues. Refs: bmad-code-org#1519
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.
arcaven
force-pushed
the
feat/csv-file-ref-validation
branch
from
4b19304 to
ac156fc
Compare
alexeyv
left a comment
alexeyv
left a comment
There was a problem hiding this comment.
Another deterministic quality gate. Love it!
Code looks fine, with a couple of questions.
- 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)
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.
…ESOLVED] 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].
alexeyv
left a comment
alexeyv
left a comment
There was a problem hiding this comment.
Looks like a merge to me
3 participants
What
Extends
tools/validate-file-refs.js(the Layer 1 file reference validator) to scan CSV files for broken_bmad/workflow-file references. Adds.csvto the scan pipeline with a dedicatedextractCsvRefs()function, 7 test fixtures, and a standalone test runner.Why
After the Layer 2 schema validation work in PR #1529 (closed — being re-planned for the
workflow.yaml→workflow*.mdmigration), analysis of community issue patterns revealed an additional class of repeat bugs: broken or incorrect paths in CSV manifest and help files. These CSV references were invisible to Layer 1 because it only scanned YAML, markdown, and XML. This PR closes that gap.Additional schema-aware CSV validation (column-level type checking, enum validation) is planned for Layer 2. This PR focuses on structural file-reference checking, which fits naturally in Layer 1.
NOTE: this shows one validator at each level, but I have plans for:
Layer 2: Schema Validity (per artifact type)
validate-agent-schema.js*.agent.yamlworkflow*.mdfrontmattermodule-help.csvcatalogLayer 3: Structural/Semantic Validation
Summary
Issues and bugs this class of validator addresses
bmad-help.csvhas incorrect command for Create Brief workflow_bmad/paths; command column validation planned for Layer 2task-manifest.csvreferences non-existentdaily-standup.xmlusing old.bmadprefixworkflow-manifest.csvwith column count mismatch (5 vs 4 cols)relax_column_counthandles gracefully; schema validation is Layer 2shard-doctask orphaned: manifest mismatch731bee26:module-help.csvreferencedworkflow-create-prd.mdbefore file existed; fixed same-day inbd620e38Current validation status
The 2 broken refs are
core/tasks/validate-workflow.xmlreferenced fromcreate-story/checklist.md— tracked in #1530, pre-existing.How
csv-parse/syncimport (already a dependency, v6.1.0) and.csvtoSCAN_EXTENSIONSextractCsvRefs(filePath, content)— parses CSV with{columns: true, skip_empty_lines: true, relax_column_count: true}, extractsworkflow-filecolumn values astype: 'project-root'refselse if (ext === '.csv')dispatch in the main scan loop (without this, CSV falls through toextractMarkdownRefs)require.main === moduleguard for testability, exportedextractCsvRefstest:refsnpm script and file-pattern-to-validator mapping table in CONTRIBUTING.mdDesign decisions
workflow-fileonly (v1). Other columns don't contain file refs.project-roottype — no new resolution logic needed.relax_column_count: truehandles the trailing commas insrc/bmm/module-help.csv.require.mainguard: Standard Node.js pattern. Allowsrequire()for unit testing without triggering the scan +process.exit().Why is this safe to adopt
Adapted from the original Layer 1 PR (#1494) — same design principles apply.
The validator runs in warning mode by default (exit 0). Broken references appear in the build log for visibility, but build results are unaffected. No existing CI checks, pre-commit hooks, or npm scripts are modified in behavior. The validator is purely additive.
Every existing CI check continues to enforce exactly as before:
The
test:refsunit tests validate the extraction function in isolation — they test code correctness, not file resolution. The full scan (npm run validate:refs) remains warning-only.When ready to enforce strict mode:
The validator only reads files. It makes no changes to disk.
What's next — and why BMAD-METHOD first
BMAD-METHOD is where the formats are defined, the primary install source, and where most community contributions land as direct PRs. Errors introduced here bypass any protections bmad-builder might provide for the subset of the community 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 2 schema-aware CSV validation (column types, command uniqueness, enum constraints) will follow when the
workflow.yaml→workflow*.mdmigration is complete. This PR handles only structural file-reference integrity, which is Layer 1's job.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 Validator<br/>PR #1494 merged rect rgba(0, 128, 255, 0.1) BM->>BM: Layer 1 + CSV extension<br/>PR #1573 (this PR) end BM->>BM: Layer 2 — Schema Validator<br/>(planned — replanning for workflow*.md) BM->>BM: Layer 3 — Graph Validator (planned) 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 outputTesting
node test/test-file-refs-csv.js)npm run validate:refsscans 212 files including 10 CSV files, 501 refs checkedRefs: #1519, #1136, #1070, #1097, #832, #1536, #1260