feat: add file reference validator (MSSCI-14579)#8
feat: add file reference validator (MSSCI-14579)#8bmadcode merged 5 commits intobmad-code-org:mainfrom
Conversation
Tests cover all 10 acceptance criteria for MSSCI-14579: - _testing exports (path mapping, ref extraction, skip logic) - Module auto-detect from module.yaml - External ref detection and skip - Template placeholder detection - Absolute path leak detection - CLI exit codes (default vs --strict) - npm scripts (validate:refs, test:refs) - CI integration (quality.yaml step) - Live source tree baseline ratchet (30 known broken refs) 15 tests failing, 1 accidentally passing — RED state confirmed.
Implements validate-file-refs.mjs that catches broken cross-file references, absolute path leaks, and auto-detects module code from module.yaml. Skips external module refs, install-generated files, template placeholders, and runtime variables. - Add validate:refs and test:refs npm scripts - Wire test:refs into npm test chain - Add validate:refs step to CI quality workflow - All 40 tests pass (29 broken refs against 30 baseline)
WalkthroughAdds a new file-reference validator tool, tests, fixtures, npm scripts, and CI workflow step to scan YAML/Markdown/CSV under Changes
Sequence DiagramsequenceDiagram
participant CLI as User/CI
participant Validator as Validator Tool
participant FS as File System
participant Parser as Reference Parser
participant Resolver as Path Resolver
participant Reporter as Report Generator
CLI->>Validator: npm run validate:refs [--flags]
Validator->>FS: Discover src/ files
FS-->>Validator: YAML, Markdown, CSV files
Validator->>FS: Load module.yaml for external detection
FS-->>Validator: Module mappings
Validator->>Parser: Extract refs from files
Parser->>Parser: Parse frontmatter, body, CSV columns
Parser-->>Validator: Extracted references
Validator->>Resolver: Resolve refs (relative, {project-root})
Resolver->>Resolver: Map {project-root}/_bmad → src/
Resolver->>Resolver: Detect absolute path leaks & placeholders
Resolver->>FS: Verify existence
FS-->>Resolver: Exists / Missing
Resolver-->>Validator: Resolution results
Validator->>Reporter: Aggregate and format issues
Reporter-->>CLI: Print summary and set exit code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Poem
🚥 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)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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: 5
🤖 Fix all issues with AI agents
In `@test/test-validate-file-refs.cjs`:
- Around line 129-133: The JSDoc AC number in the test comment is inconsistent
with the test section and other tests: update the doc comment above function
testPathMapping so the AC number matches the section title and related tests
(change "AC3" to "AC2"); locate the comment immediately above the function
testPathMapping and adjust the identifier in the JSDoc to "AC2" so the comment,
section('AC2: Path mapping') and testModuleAutoDetect references are consistent.
- Line 18: Update the file usage comment string to reference the actual
filename: replace "node test/test-validate-file-refs.js" with "node
test/test-validate-file-refs.cjs" in the top-of-file usage comment so the
documented invocation matches the real file name.
- Line 425: The code uses logical OR to compute brokenCount which treats 0 as
falsy; update the expression in test/test-validate-file-refs.cjs that assigns
brokenCount (currently using data.brokenCount || data.broken_refs || 0) to use
nullish coalescing so it only falls back when values are null/undefined (i.e.,
change to use ?? between data.brokenCount, data.broken_refs, and the final 0) to
preserve a legitimate 0 result.
In `@tools/validate-file-refs.mjs`:
- Around line 511-516: The inner fs.existsSync(resolved) check is unreachable
because it's inside if (!fs.existsSync(resolved)); move the directory-exists
check so it happens before or outside the negated guard: first compute hasExt =
path.extname(resolved) !== '' and if (!hasExt && fs.existsSync(resolved))
continue to skip directories, then only afterwards run if
(!fs.existsSync(resolved)) { broken.push({ ref, resolved:
path.relative(PROJECT_ROOT, resolved) }); brokenRefs++; } — adjust the logic
around resolved, hasExt, fs.existsSync, broken.push and brokenRefs accordingly.
- Line 114: The ABS_PATH_LEAK regex currently uses [A-Z]:\\\\ which matches two
literal backslashes (e.g. "C:\\\\") and therefore misses typical Windows paths
with a single backslash; update the pattern to match a single backslash by
replacing [A-Z]:\\\\ with [A-Z]:\\ so ABS_PATH_LEAK =
/(?:\/Users\/|\/home\/|[A-Z]:\\)/ (or alternatively use a variant that allows
one or two backslashes if you want to accept both forms).
🧹 Nitpick comments (7)
tools/validate-file-refs.mjs (4)
406-408: UseString#slice()instead ofString#substring().Per ESLint
unicorn/prefer-string-slicerule flagged by static analysis.Proposed fix
- leaks.push({ file: filePath, line: i + 1, content: line.trim().substring(0, 100) }); + leaks.push({ file: filePath, line: i + 1, content: line.trim().slice(0, 100) });
219-221:issue()factory function appears unused.This function is defined but never called — the main loop constructs issue objects inline at lines 534 and 545. Consider removing it or using it consistently.
59-83: HardcodedUNRESOLVABLE_VARSallowlist requires code changes for each new runtime variable.This is acceptable for a project-specific tool, but worth noting that any new runtime variable patterns in source files will need a corresponding entry here to avoid false-positive broken-ref reports. A comment noting this maintenance requirement (or a broader pattern-based approach like matching any
{snake_case_var}) could reduce future churn.
416-434: Path containment check usesstartsWith(SRC_DIR)without a trailing separator.
resolved.startsWith(SRC_DIR)could match a sibling directory likesrc-backup/. In practice this is unlikely given the project structure, but a more robust check would be:if (!resolved.startsWith(SRC_DIR + path.sep) && resolved !== SRC_DIR) return null;test/test-validate-file-refs.cjs (3)
28-28:FIXTURE_DIRis declared but never used.ESLint flags this (
no-unused-vars). Either remove it or use it in fixture-based tests that currently hardcode paths or inline content.Proposed fix (remove)
-const FIXTURE_DIR = path.join(__dirname, 'fixtures', 'file-refs');
88-104: Consider extracting repeated_testingimport into a shared helper.Every test function independently imports the module and extracts
_testingwith identical boilerplate (~15 lines each). A sharedloadTesting()helper would eliminate this duplication and make it easier to maintain.Example helper
let _testingCache; async function loadTesting() { if (_testingCache) return _testingCache; const mod = await import(TOOL_PATH); const t = mod._testing || (mod.default && mod.default._testing); if (!t) throw new Error('_testing namespace not exported'); _testingCache = t; return t; }
95-95: Address remaining ESLint violations.A few linter flags worth cleaning up:
- Line 95: Rename catch parameter
err→error(unicorn/catch-error-name).- Line 415: Use
Number.parseInt(…)instead of bareparseInt(…)(unicorn/prefer-number-properties).The
unicorn/no-negated-conditionwarnings (Lines 234, 294, 483) andno-process-exitwarnings (Lines 554, 558, 563) are acceptable as-is — the negated conditions read naturally in assertion context, andprocess.exit()is appropriate for a CLI test runner.Also applies to: 415-415
- Fix eslint unicorn/prefer-string-slice (substring → slice) - Fix prettier formatting on both validator and test files - Remove unused issue() function (dead code) - Remove unreachable hasExt/existsSync branch
Fix 13 CJS-specific eslint errors (catch-error-name, no-negated-condition, prefer-number-properties, no-process-exit) and 4 CodeRabbit suggestions (comment filename, AC number, nullish coalescing, Windows path regex).
Module validate workflow and steps referenced step-01-validate.md which doesn't exist. Corrected to step-01-load-target.md (module) and v-01-load-review.md (agent) which are the actual entry points for their validation flows. Fixes bmad-code-org#15, bmad-code-org#16 Found by file reference validator (PR bmad-code-org#8)
All module workflow steps referenced ../../data/ and ../../templates/ but the actual files are at ../data/ and ../templates/ (one level closer). Also corrects agent-spec-template.md references from templates/ to data/ where it actually lives. Fixes bmad-code-org#9, bmad-code-org#10, bmad-code-org#11, bmad-code-org#12, bmad-code-org#13, bmad-code-org#14 Found by file reference validator (PR bmad-code-org#8)
Agent edit step referenced ../workflow.md (doesn't exist) instead of ../workflow-edit-agent.md. Module create continue step referenced ../workflow.md instead of ../workflow-create-module.md. Module edit workflow referenced step-01-assess.md instead of step-01-load-target.md. Fixes bmad-code-org#17, bmad-code-org#18, bmad-code-org#19 Found by file reference validator (PR bmad-code-org#8)
…trics Inline suppression: <!-- validate-file-refs:ignore --> on the same line or <!-- validate-file-refs:ignore-next-line --> on the previous line suppresses both broken-ref and abs-path-leak findings. Applied to 6 known false positives (doc examples, forbidden-pattern tables, checklist text) across 3 data/ files — drops findings from 32 to 26. Data-dir trap door: refs in data/ directories whose filename starts with EXAMPLE- or invalid- are auto-skipped. Convention for documentation authors writing illustrative examples without triggering validation. Legacy/archived content moved to data/ may need additional handling as that pattern emerges. Ref metrics: summary now reports reference counts by type (project-root vs relative), suppressed count, and issue rate (issues / total refs %). 50 tests (up from 40), baseline ratcheted from 30 to 26.
2 participants
Summary
tools/validate-file-refs.mjs— ESM validator that scans BMB source files for broken cross-file references, absolute path leaks, and stale pathsbmb) fromsrc/module.yamlto distinguish internal vs external refscore/,bmm/), install-generated files (config.yaml,docs/KBs), template placeholders ([N],[name]), and runtime variables ({output_folder}, etc.)<!-- validate-file-refs:ignore -->) for documentation examples that contain intentionally broken paths*/data/files withEXAMPLE-orinvalid-filename prefixes are auto-skippedtest/test-validate-file-refs.cjswith 50 tests covering all 10 acceptance criteria plus suppression and trap door featuresvalidate:refsandtest:refsnpm scripts into the test chain and CI workflowFixes #7
Prior art
This is the bmad-builder port of the Layer 1 file reference validator originally built for BMAD-METHOD:
Validation pyramid — bmad-builder
Implementation status across repos
sequenceDiagram participant BMB as bmad-builder participant BM as BMAD-METHOD Note over BMB: ✅ Layer 0 — Formatting, Linting & Docs<br/>Prettier, ESLint, markdownlint,<br/>doc link validation, docs build rect rgba(0, 128, 255, 0.1) BMB->>BMB: Layer 1 — File Reference Validator<br/>PR #8 (this PR) end Note over BMB: ✅ Layer 2 — Schema & Compilation<br/>Agent schema (Zod), agent compilation,<br/>install component tests Note over BMB: ⬚ Layer 3 — Graph Validation (planned) Note over BMB,BM: ── cross-repo status ── Note over BM: ✅ Layer 0 — Prettier, ESLint, markdownlint Note over BM: ✅ Layer 1 — File Reference Validator<br/>PR #1494 merged + CSV extension #1573 merged Note over BM: ✅ Layer 2 — Agent Schema Validator<br/>PR #774 merged Note over BM: ⬚ Layer 2b — Workflow Schema (replanning for workflow*.md) Note over BM: ⬚ Layer 3 — Graph Validation (planned)How it works
flowchart TD A[Discover source files in src/<br/>YAML, Markdown, CSV] --> B[Read file content] B --> C{File type?} C -->|YAML| D[Parse YAML tree<br/>walk maps, sequences, scalars] C -->|Markdown| E[Parse frontmatter<br/>+ strip code blocks<br/>+ regex body scan] C -->|CSV| F[Parse CSV<br/>extract workflow-file column] D --> G[Collect raw references] E --> G F --> G G --> H{Resolvable?} H -->|Template placeholder<br/>Runtime variable<br/>Mustache tag| I[Skip] H -->|Yes| J{External module?} J -->|core/, bmm/, etc.| K["Log [INFO] if --include-external"] J -->|Internal bmb/| L{Reference type?} L -->|project-root / _bmad| M["mapInstalledToSource()<br/>_bmad/bmb/ → src/"] L -->|Relative path| N["path.resolve()<br/>from containing file dir"] M --> O{fs.existsSync?} N --> O O -->|Missing| P["Report [BROKEN]<br/>with file:line"] O -->|Found| Q["Report [OK]<br/>in verbose mode"] B --> R{Absolute path leak?} R -->|"/Users/ /home/ C:\"| S["Report [ABS-PATH]<br/>with line number"] P --> T[Summary & exit code] Q --> T S --> T T --> U{--strict flag?} U -->|Yes + issues| V[Exit 1] U -->|No or clean| W[Exit 0]Reference types validated
{project-root}/_bmad/paths{project-root}/_bmad/bmb/workflows/workflow-create-workflow.md./step-02-analysis.md,../data/template.mdnextStepFile,stepTemplate,checklist_bmad/bmb/workflows/agent/workflow-create-agent.md/Users/dev/project/...,C:\Users\...What gets skipped
_bmad/core/,_bmad/bmm/config.yaml,docs/*.mdKBs[N],[name],[template]{output_folder},{bmb_creations_output_folder}{{variable_name}}../../../../core/(resolves outsidesrc/)<!-- validate-file-refs:ignore -->on same line<!-- validate-file-refs:ignore-next-line -->on prior line*/data/withEXAMPLE-orinvalid-prefixCurrent state
26 broken references found against a baseline ratchet of 26. These are pre-existing issues in the source tree — the validator surfaces them without blocking CI (warning-only mode by default,
--strictfor exit 1).Reference metrics
CLI flags
--strict--verbose--include-externalSuppressing false positives
For documentation files that contain intentionally broken paths (examples, FORBIDDEN patterns tables, etc.), add an inline HTML comment:
For data directory files that serve as examples or illustrations, prefix filenames with
EXAMPLE-orinvalid-and they'll be auto-skipped. As archived, legacy, or no-longer-valid content moves intodata/directories, additional validator handling may emerge as that pattern develops.Why is this safe to adopt
Non-blocking by design.
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:
When ready to enforce, one change in
package.json:The validator only reads files. It makes no changes to disk.
First-run findings
On first run against
main, the validator found 32 raw findings (29 broken references + 3 absolute path leaks) across 162 source files (373 references checked, 67 external refs correctly skipped). After analysis, 26 are confirmed real bugs and 6 are false positives (documentation examples). Fix PRs have been filed for 24 of the 26. The 6 false positives are now suppressed with inline ignore comments in this PR.All findings
module/steps-c/step-01-load-brief.md../../templates/agent-spec-template.md— wrong depth + wrong dirmodule/steps-c/step-01-load-brief.md../../templates/workflow-spec-template.md— wrong depthmodule/steps-c/step-01-load-brief.md../../data/module-standards.md— wrong depthmodule/steps-c/step-01-load-brief.md../../data/module-yaml-conventions.md— wrong depthmodule/steps-c/step-02-structure.md../../data/module-standards.md— wrong depthmodule/steps-c/step-03-config.md../../data/module-yaml-conventions.md— wrong depthmodule/steps-c/step-04-agents.md../../templates/agent-spec-template.md— wrong depth + wrong dirmodule/steps-c/step-04-agents.md../../data/agent-architecture.md— wrong depthmodule/steps-c/step-05-workflows.md../../templates/workflow-spec-template.md— wrong depthmodule/steps-c/step-07-complete.md../steps-v/step-01-validate.md— wrong filenamemodule/steps-c/step-01b-continue.md../workflow.md— wrong filenamemodule/steps-b/step-13-review.md../../templates/brief-template.md— wrong depthmodule/steps-b/step-14-finalize.md../../templates/brief-template.md— wrong depthmodule/steps-e/step-01-load-target.md../../data/module-standards.md— wrong depthmodule/steps-v/step-02-file-structure.md../../data/module-standards.md— wrong depthmodule/steps-v/step-03-module-yaml.md../../data/module-yaml-conventions.md— wrong depthmodule/steps-v/step-04-agent-specs.md../../templates/agent-spec-template.md— wrong depth + wrong dirmodule/steps-v/step-04-agent-specs.md../../data/agent-architecture.md— wrong depthmodule/steps-v/step-04-agent-specs.md{project-root}/…/agent/steps-v/step-01-validate.md— wrong filenamemodule/steps-v/step-05-workflow-specs.md../../templates/workflow-spec-template.md— wrong depthmodule/steps-v/step-08-report.md{project-root}/…/agent/steps-v/step-01-validate.md— wrong filenamemodule/workflow-validate-module.md./steps-v/step-01-validate.md— wrong filenamemodule/workflow-edit-module.md./steps-e/step-01-assess.md— wrong filenameagent/steps-e/e-01-load-existing.md../workflow.md— wrong filenameworkflow/workflow-rework-workflow.md./steps-r/step-01-assess-rework.md— missing dir (stub)workflow/templates/step-1b-template.md./step-01b-continue.md— template self-refworkflow/data/frontmatter-standards.md./step-XX.md— example in FORBIDDEN patterns tableworkflow/data/frontmatter-standards.md./workflow.md— example in FORBIDDEN patterns tableworkflow/data/step-type-patterns.md./step-01b-continue.md— instructional exampleagent/data/agent-validation.md/Users/— checklist text about what to avoidworkflow/data/frontmatter-standards.md/Users/user/dev/BMAD-METHOD— variable example tableworkflow/data/frontmatter-standards.md/Users/user/dev/BMAD-METHOD/output— variable example tableFindings by root cause
../../should be../)Reconciliation
Test plan
npm run validate:refsruns successfullynpm run test:refspassesAd hoc: wrong-depth findings verified against simulated install
The 17 wrong-depth findings (
../../→../) are the largest finding category. To confirm these aren't valid at install time (e.g., if the installer adds intermediate directories that would make../../correct), we tested paths against both the source tree and a simulated installed output.Method: Ran
bmad-method install(local dev CLI, non-interactive mode per docs) to a temp directory, then resolved paths from the installed output.Installed structure —
steps-*,data/, andtemplates/remain siblings undermodule/:Path resolution from the installed
steps-c/:All 17 tested against both source tree and installed output:
templates/agent-spec-template.md— file lives atdata/agent-spec-template.mdAll 17 are confirmed bugs. The installer preserves relative directory structure with no path rewriting (
manager.js:762).Summary by CodeRabbit
Release Notes
New Features
Tests
Chores
validate:refsandtest:refs.