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:

```go
// 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 ./...`