review-bot/PLAN-141.md

Plan: validate-docmap subcommand (Issue #141)

Problem

CI has no way to verify that doc-map.yml is kept up to date. When a developer adds a new module/directory, they may forget to add a paths: entry. When a design doc is deleted or moved, the docs: entry becomes stale. Both failures are silent — the AI reviewer just gets no docs injected, and nobody notices.

This is a pure static check: no AI, no VCS API. Just YAML parsing + glob matching + os.Stat.

Constraints

• No external API calls or AI involvement
• Must compose with git diff --name-only output via stdin (standard CI pattern)
• Reuse existing ParseDocMapConfig from review/docmap.go
• Glob matching logic must also reuse (or expose) existing globMatch/mappingMatches
• Follow the validate-url subcommand pattern exactly
• Both checks must always run — report all failures, not just the first
• outWriter/errWriter vars must be respected for testability

Proposed Approach

1. Export a glob-coverage helper from review/docmap.go

Add one new exported function:

// FileCoveredByDocMap returns true if any paths: glob in cfg matches the given file.
func FileCoveredByDocMap(cfg *DocMapConfig, file string) bool

This is a thin wrapper over the existing unexported mappingMatches. It lets the cmd/ layer call into the review package without duplicating glob logic.

Alternative considered: Duplicate the loop in cmd/. Rejected — duplication of non-trivial glob matching is a maintenance hazard. Exporting one function is cleaner.

2. New file: cmd/review-bot/validatedocmap.go

Implements runValidateDocmap(args []string) int following the validateurl.go pattern.

Flag parsing (use flag.NewFlagSet — NOT global flag, to avoid polluting main.go's flag state):
  --docmap     (required) path to YAML file
  --repo-root  (optional, default ".") base for resolving docs: paths

Step 1: Parse flags. Validate --docmap is set. Exit 2 on error.
Step 2: ParseDocMapConfig(docmapPath)  → exit 2 on parse error
Step 3: Read stdin lines → changedFiles []string
Step 4: Coverage check — for each file in changedFiles:
          if !FileCoveredByDocMap(cfg, file) → record as uncovered
Step 5: Stale-docs check — for each unique docs: entry across all mappings:
          if os.Stat(filepath.Join(repoRoot, docPath)) fails → record as stale
Step 6: If any uncovered or stale entries → print ERROR sections → return 1
        Else → print "OK" → return 0

Exit codes (parallel to validate-url):

• 0 — clean
• 1 — coverage or stale-doc failures
• 2 — usage error, missing flag, or YAML parse error

3. Wire into main.go

Add case "validate-docmap": to the existing os.Args[1] switch.

4. Tests: cmd/review-bot/validatedocmap_test.go

Test table covering:

Case	stdin	docmap	repo-root	want exit
clean	covered file	valid docmap	docs exist	0
uncovered file	uncovered file	valid docmap	docs exist	1
stale doc	covered file	stale docs:	missing path	1
both failures	uncovered + stale			1
empty stdin	(empty)	valid docmap	docs exist	0
missing --docmap flag				2
bad YAML		invalid YAML		2

Use os.MkdirTemp + os.WriteFile to create real temp directories for the stale-docs check.

5. README update

Add a subsection under the validate-url section showing the validate-docmap invocation.

State/Data Model

No persistent state. All inputs are flags + stdin + local filesystem.

Error Cases

Scenario	Behavior
--docmap flag missing	Print usage, exit 2
YAML parse fails	Print error message, exit 2
stdin read error	Print error, exit 2
--repo-root does not exist	Individual docs: entries will fail Stat; logged per-path, exit 1
changed file is empty string (blank line)	Skip (trim + ignore empty)

Edge Cases

• Blank lines in stdin input (from git diff with trailing newline) → trim and skip
• Duplicate docs: entries across multiple mappings → deduplicate before checking existence
• docs: entry that is a directory (ends with /) → os.Stat the path; if it exists it's fine
• --repo-root with trailing slash → use filepath.Join which normalizes it
• Changed files with ../ or absolute paths → check only (no traversal needed here since we're just calling FileCoveredByDocMap, which is pure string matching)

Testing Strategy

• Unit tests with real temp files for stale-doc check (no mocking needed for os.Stat)
• outWriter/errWriter capture pattern (same as validateurl_test.go)
• Table-driven tests

Open Questions

• stdin vs --files flag: Using stdin matches the standard CI pipe idiom and avoids shell quoting issues with many files. Confirmed by Aaron's clarification.
• Empty stdin coverage: Aaron said empty stdin = no coverage failures. This means "no changed files, no problem" — vacuously true. Makes sense for git diff on unchanged branches.
• Directory docs: entries: os.Stat is sufficient — if the directory exists, it's valid. We don't recursively verify it has .md files. Kept simple.
• --repo-root vs always cwd: Default to cwd but allow override. This makes the command usable from CI scripts that cd to a different directory.

Completion Checklist (generated for this task)

1. FileCoveredByDocMap exported and covers the all-mappings, any-glob-matches logic correctly?
2. runValidateDocmap follows runValidateURL exactly: flag parse → validate → work → exit code?
3. Both checks always run (no early exit after first failure section)?
4. Empty stdin treated as clean (exit 0, no coverage errors)?
5. All docs: entries deduplicated before stale check?
6. outWriter/errWriter used (not fmt.Println directly), so tests can capture output?
7. case "validate-docmap": added to main.go dispatch switch?
8. Tests cover all 7 cases in the table above?
9. README updated with usage example?
10. go test ./... passes with no new failures?

Implementation Phases

Phase 1: Export helper in review/docmap.go

• Add FileCoveredByDocMap(cfg *DocMapConfig, file string) bool
• Add test in review/docmap_test.go

Phase 2: cmd/review-bot/validatedocmap.go

• Full runValidateDocmap implementation

Phase 3: Wire into main.go + tests

• case "validate-docmap": dispatch
• validatedocmap_test.go with full table

Phase 4: README + final

• Update README
• go test ./...