Compare commits
1 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
 claw
|
d75e737f07 |
@@ -2,14 +2,6 @@
|
||||
|
||||
## Unreleased
|
||||
|
||||
### Security
|
||||
|
||||
- **`validateDocmapPath`: add `EvalSymlinks` to close directory-symlink bypass** ([#150](https://gitea.weiker.me/rodin/review-bot/issues/150)): The previous implementation used `os.Lstat` which only avoids following the *final* path component. An intermediate directory symlink (e.g. `.review-bot/` committed as a symlink to a directory outside the repo) would pass the path-confinement check because the textual path appeared within the repo root. `filepath.EvalSymlinks` is now called first, resolving all symlink components before the `filepath.Rel` confinement check. In-repo symlinks whose resolved targets also reside within the repo root are now allowed; out-of-repo targets are rejected by the confinement check.
|
||||
|
||||
### Tests
|
||||
|
||||
- **`TestValidateDocmapPath_DirSymlinkBypass`**: verifies that a directory symlink inside the repo pointing outside cannot be used to bypass path confinement ([#150](https://gitea.weiker.me/rodin/review-bot/issues/150)).
|
||||
|
||||
### Added
|
||||
|
||||
- **`doc-map` input** (`--doc-map` flag / `DOC_MAP_FILE` env var): Path to a YAML file mapping source path globs to governing design docs. review-bot intersects the map with changed PR paths and injects matching docs into the system prompt under a `## Design Documents` heading. ([#137](https://gitea.weiker.me/rodin/review-bot/issues/137))
|
||||
|
||||
+25
-18
@@ -1,36 +1,43 @@
|
||||
=============================================================================
|
||||
REVIEW-BOT DEV LOOP STATUS — 2026-05-15 04:08 UTC
|
||||
REVIEW-BOT DEV LOOP STATUS — 2026-05-15 01:48 UTC (post-sync)
|
||||
=============================================================================
|
||||
|
||||
OVERALL STATUS: ✅ PR OPEN
|
||||
OVERALL STATUS: ✅ OPTIMAL
|
||||
|
||||
Active Work:
|
||||
- PR #140: test(#139): improve cmd/review-bot coverage 44.6% → 49.3%
|
||||
State: open, labeled: ready, self-reviewed
|
||||
Branch: issue-139
|
||||
|
||||
Test Results (last full run, worktree):
|
||||
Test Results (fresh run post-sync):
|
||||
- All 6 packages: PASS ✅
|
||||
- Build: ✅ clean
|
||||
- Vet: ✅ clean
|
||||
- Fresh run: -count=1 verified
|
||||
|
||||
Coverage (post-change):
|
||||
- cmd/review-bot: 49.3% (was 44.6%)
|
||||
- review: 91.9%
|
||||
- budget: 92.0%
|
||||
Recent Major Changes (synced from origin/main):
|
||||
- Significant new GitHub client methods (~360 lines added)
|
||||
- New validateurl package for URL validation
|
||||
- New vcs adapter layer for VCS abstraction
|
||||
- New gitea/ipcheck package for IP validation
|
||||
- Expanded integration tests in cmd/review-bot
|
||||
- All changes verified passing tests
|
||||
|
||||
Coverage (current post-sync):
|
||||
- review: 92.0%
|
||||
- budget: 91.8%
|
||||
- github: 86.3%
|
||||
- gitea: 85.2%
|
||||
- llm: 81.3%
|
||||
- cmd/review-bot: 46.1%
|
||||
|
||||
Repository (main):
|
||||
- Branch: main (up to date with origin — 1e3d86b)
|
||||
Repository:
|
||||
- Branch: main (synced with origin — 4ffa6b6)
|
||||
- Working tree: clean
|
||||
- Open issues: 1 (#139, addressed by PR #140)
|
||||
- Open PRs: 1 (#140, ready for review)
|
||||
- Open issues: 0
|
||||
- Open PRs: 0
|
||||
|
||||
System Health: ✅ GREEN
|
||||
✓ All tests passing
|
||||
✓ All tests passing (33 commits synced)
|
||||
✓ No warnings
|
||||
✓ PR ready for merge
|
||||
✓ Code clean
|
||||
✓ Ready for feature work
|
||||
|
||||
Next Cycle: Ready to pick up feature work
|
||||
|
||||
=============================================================================
|
||||
|
||||
@@ -296,40 +296,6 @@ review-bot \
|
||||
--conventions-file CONVENTIONS.md
|
||||
```
|
||||
|
||||
## Subcommands
|
||||
|
||||
### `validate-docmap`
|
||||
|
||||
Verifies that a `doc-map.yml` is consistent before running a review. Two checks:
|
||||
|
||||
1. **Coverage**: every changed file is matched by at least one `paths:` glob.
|
||||
2. **Stale docs**: every `docs:` entry exists on disk under `--repo-root`.
|
||||
|
||||
```bash
|
||||
# Typical CI usage — pipe git diff into the command
|
||||
git diff --name-only origin/main HEAD | \
|
||||
review-bot validate-docmap \
|
||||
--docmap .review-bot/doc-map.yml \
|
||||
--repo-root .
|
||||
```
|
||||
|
||||
| Flag | Required | Default | Description |
|
||||
|------|----------|---------|-------------|
|
||||
| `--docmap` | Yes | — | Path to doc-map YAML file |
|
||||
| `--repo-root` | No | `.` (cwd) | Root for resolving `docs:` paths |
|
||||
|
||||
Exit codes: `0`=clean, `1`=failures found, `2`=usage/parse error.
|
||||
|
||||
### `validate-url`
|
||||
|
||||
Resolves a URL and verifies all IPs are publicly routable (used in CI to prevent SSRF).
|
||||
|
||||
```bash
|
||||
review-bot validate-url https://gitea.example.com
|
||||
```
|
||||
|
||||
Exit codes: `0`=safe, `1`=blocked/private IP, `2`=error.
|
||||
|
||||
## Environment Variables
|
||||
|
||||
All flags have environment variable equivalents:
|
||||
|
||||
@@ -1,129 +0,0 @@
|
||||
# Dev-Loop Skill: review-bot
|
||||
|
||||
This file documents the dev-loop architecture for the `review-bot` project.
|
||||
It lives in the repo so changes are version-controlled alongside the code.
|
||||
|
||||
## Architecture
|
||||
|
||||
Dispatch is a **pure shell script** — no model reasoning.
|
||||
|
||||
```
|
||||
Cron (agentTurn, toolsAllow: [exec, sessions_spawn, read])
|
||||
→ runs dispatch script
|
||||
→ reads output for SPAWN or HANDOFF lines
|
||||
→ spawns worker if instructed
|
||||
|
||||
Dispatch script (~/.openclaw/workspace/scripts/dev-loop-dispatch.sh)
|
||||
→ pure bash, all decisions are curl API calls + branches
|
||||
→ exits after emitting one SPAWN line (at most one worker per run)
|
||||
→ emits HANDOFF for each qualifying PR (does not exit after HANDOFF)
|
||||
|
||||
Workers (Opus, spawned by cron model)
|
||||
→ receive precise task description
|
||||
→ do one job: self-review, fix CI, address feedback, or implement
|
||||
→ remove wip label when done, reply NO_REPLY
|
||||
```
|
||||
|
||||
The cron model's **only** job: run script, read output, spawn worker if told to.
|
||||
The model **never** assesses project state or makes dispatch decisions.
|
||||
|
||||
## Safety Invariants
|
||||
|
||||
1. **NEVER MERGE** — no merge API call exists anywhere in the script or worker templates
|
||||
2. **REQUEST_CHANGES always blocks** — checked first, before CI, before self-review, before handoff
|
||||
3. **WIP mutex** — one active worker per repo; WIP label gates new issue pickup
|
||||
4. **One SPAWN per run** — script emits at most one SPAWN line per execution
|
||||
5. **set -euo pipefail** — any curl failure aborts immediately, no partial actions
|
||||
6. **Workers reply NO_REPLY** — no dispatch-level side effects (workers may push changes and manage labels as part of their task)
|
||||
|
||||
## Dispatch Rules (in order)
|
||||
|
||||
| Rule | Condition | Action |
|
||||
|------|-----------|--------|
|
||||
| 0 | WIP label > 1hr old | Remove stale WIP, continue |
|
||||
| 0b | WIP label ≤ 1hr old | Mark ACTIVE_WIP=1, continue (only gates Rule 10) |
|
||||
| _(1)_ | _(reserved — intentionally unused)_ | — |
|
||||
| 2 | Any reviewer has REQUEST_CHANGES | SPAWN:findings |
|
||||
| 3 | PR not mergeable | SPAWN:rebase |
|
||||
| 4 | CI failure, no fix plan | SPAWN:ci-fix |
|
||||
| 4b | CI failure, fix plan exists | Skip (worker in progress) |
|
||||
| 5 | Bot review missing | Wait |
|
||||
| 6 | CI pending/unknown | Wait |
|
||||
| 7 | No clean self-review, no fix plan | SPAWN:self-review |
|
||||
| 7b | Self-review needs attention, no fix plan | SPAWN:sr-fix |
|
||||
| 8 | Unacknowledged bot review findings | SPAWN:address-feedback |
|
||||
| 9 | Unresolved inline diff comments | SPAWN:address-feedback |
|
||||
| 10 | All checks pass | HANDOFF |
|
||||
| 11 | No open PRs + no ACTIVE_WIP | SPAWN:impl (next issue) |
|
||||
|
||||
## Files
|
||||
|
||||
| File | Description |
|
||||
|------|-------------|
|
||||
| `~/.openclaw/workspace/scripts/dev-loop-dispatch.sh` | Dispatch script — pure bash |
|
||||
| `~/.openclaw/workspace/scripts/worker-tasks/self-review.md` | Self-review worker template |
|
||||
| `~/.openclaw/workspace/scripts/worker-tasks/sr-fix.md` | Fix findings from self-review |
|
||||
| `~/.openclaw/workspace/scripts/worker-tasks/ci-fix.md` | CI fix worker template |
|
||||
| `~/.openclaw/workspace/scripts/worker-tasks/address-feedback.md` | Address feedback worker template |
|
||||
| `~/.openclaw/workspace/scripts/worker-tasks/findings.md` | Address REQUEST_CHANGES findings |
|
||||
| `~/.openclaw/workspace/scripts/worker-tasks/rebase.md` | Rebase worker template |
|
||||
| `~/.openclaw/workspace/scripts/worker-tasks/impl.md` | Issue implementation worker template |
|
||||
| `~/.openclaw/workspace/scripts/test/dispatch.bats` | Unit tests (bats) |
|
||||
| `~/.openclaw/workspace/scripts/test/check-invariants.sh` | Static invariant checks |
|
||||
| `~/.openclaw/workspace/memory/projects/review-bot.yaml` | Project config |
|
||||
|
||||
## Project Config
|
||||
|
||||
Config is at `~/.openclaw/workspace/memory/projects/review-bot.yaml`.
|
||||
|
||||
Key fields:
|
||||
- `repo`: `rodin/review-bot`
|
||||
- `api_base`: `https://gitea.weiker.me/api/v1`
|
||||
- `user`: `rodin` (bot Gitea username)
|
||||
- `labels.wip`: WIP label ID
|
||||
- `labels.ready`: ready label ID
|
||||
- `review_bots`: list of bot sentinel names
|
||||
|
||||
## Cron Config
|
||||
|
||||
```yaml
|
||||
- label: review-bot-dev-loop
|
||||
schedule: "*/15 * * * *"
|
||||
prompt: |
|
||||
Run: bash ~/.openclaw/workspace/scripts/dev-loop-dispatch.sh review-bot
|
||||
|
||||
Read the output. If it contains a SPAWN line, load the matching template from
|
||||
~/.openclaw/workspace/scripts/worker-tasks/<type>.md, substitute {{PROJECT}},
|
||||
{{PR_NUM}}, and {{HEAD_SHA}}, then spawn with sessions_spawn(mode: "run",
|
||||
model: "hai-anthropic/anthropic--claude-4.6-opus", thinking: "high").
|
||||
|
||||
If no SPAWN line in output, reply NO_REPLY.
|
||||
|
||||
See ~/.openclaw/workspace/skills/dev-loop/SKILL.md for full instructions.
|
||||
(This repo's SKILL.md is deployed to that workspace path.)
|
||||
model: hai-anthropic/anthropic--claude-4.5-haiku
|
||||
toolsAllow: [exec, sessions_spawn, read]
|
||||
```
|
||||
|
||||
## Tests
|
||||
|
||||
```bash
|
||||
# Unit tests (no real API calls):
|
||||
bats ~/.openclaw/workspace/scripts/test/dispatch.bats
|
||||
|
||||
# Invariant checks (static analysis):
|
||||
bash ~/.openclaw/workspace/scripts/test/check-invariants.sh
|
||||
|
||||
# Dry-run against real API:
|
||||
DRY_RUN=1 bash ~/.openclaw/workspace/scripts/dev-loop-dispatch.sh review-bot
|
||||
```
|
||||
|
||||
## Related Issues
|
||||
|
||||
- **#144** — autonomous merge: eliminated by removing all merge API calls from dispatch
|
||||
- **#145** — merged despite REQUEST_CHANGES: eliminated by checking REQUEST_CHANGES first, unconditionally
|
||||
- **#148** — this redesign
|
||||
|
||||
## Spec
|
||||
|
||||
Full design spec: `docs/dev-loop-spec.md`
|
||||
@@ -0,0 +1,37 @@
|
||||
## Dev Loop Status: 2026-05-15 02:28 UTC
|
||||
|
||||
**Repository:** review-bot (rodin/review-bot on Gitea)
|
||||
**Status:** ✅ OPTIMAL
|
||||
|
||||
### Health Check
|
||||
|
||||
- **Working tree:** clean
|
||||
- **Branch:** main (up to date with origin)
|
||||
- **Build:** ✅ passes (`go build ./cmd/review-bot`)
|
||||
- **Tests:** ✅ ALL PASS (6/6 packages)
|
||||
- **Vet:** ✅ clean
|
||||
- **Open issues:** 0
|
||||
- **Open PRs:** 0
|
||||
|
||||
### Recent Changes
|
||||
|
||||
Last commit: `dcfd360` (2026-05-15 01:48) — health check post-sync
|
||||
|
||||
### Coverage
|
||||
|
||||
| Package | Coverage |
|
||||
|---------|----------|
|
||||
| cmd/review-bot | 46.1% |
|
||||
| gitea | 85.2% |
|
||||
| github | 86.3% |
|
||||
| review | 92.0% |
|
||||
|
||||
### Next Priority
|
||||
|
||||
- Increase cmd/review-bot coverage (lowest at 46.1%)
|
||||
- Monitor prod logs for edge cases
|
||||
- VCS integration stable; GitHub + Gitea paths clear
|
||||
|
||||
---
|
||||
|
||||
_Dev-loop cycle complete at 02:28 UTC._
|
||||
@@ -64,8 +64,6 @@ func main() {
|
||||
switch os.Args[1] {
|
||||
case "validate-url":
|
||||
os.Exit(runValidateURL(os.Args[2:]))
|
||||
case "validate-docmap":
|
||||
os.Exit(runValidateDocmap(os.Args[2:]))
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -1506,4 +1506,3 @@ func TestMainSubprocess_DeprecatedGiteaURLEnv(t *testing.T) {
|
||||
t.Errorf("expected deprecation warning for GITEA_URL, got: %s", out)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -1,274 +0,0 @@
|
||||
package main
|
||||
|
||||
import (
|
||||
"bufio"
|
||||
"flag"
|
||||
"fmt"
|
||||
"io"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
|
||||
"gitea.weiker.me/rodin/review-bot/review"
|
||||
)
|
||||
|
||||
// maxDocmapBytes is the maximum size of the doc-map YAML file that will be
|
||||
// read. Files larger than this are rejected before reading to prevent memory
|
||||
// exhaustion from an oversized PR-controlled file.
|
||||
const maxDocmapBytes int64 = 10 * 1024 * 1024 // 10 MB
|
||||
|
||||
// validateDocmapPath checks that localPath is safe to read as the doc-map
|
||||
// file. It enforces three invariants before the file is opened:
|
||||
//
|
||||
// 1. The path resolves to a regular file within resolvedRoot (path
|
||||
// confinement): prevents a PR-controlled --docmap from reading arbitrary
|
||||
// host files via absolute paths or ".." traversal.
|
||||
// 2. The path is not a symlink: prevents denial-of-service via /dev/zero or
|
||||
// information disclosure via symlinks that point outside the workspace.
|
||||
// 3. The file does not exceed maxDocmapBytes: prevents memory exhaustion
|
||||
// from an oversized but legitimately committed doc-map file.
|
||||
//
|
||||
// resolvedRoot must already be an absolute, symlink-free path (obtained from
|
||||
// filepath.Abs + filepath.EvalSymlinks).
|
||||
func validateDocmapPath(localPath, resolvedRoot string) error {
|
||||
// Resolve the docmap path to an absolute path.
|
||||
absPath, err := filepath.Abs(localPath)
|
||||
if err != nil {
|
||||
return fmt.Errorf("cannot resolve path: %w", err)
|
||||
}
|
||||
|
||||
// Resolve ALL symlink components, not just the final one.
|
||||
// os.Lstat only avoids following the *final* path component; intermediate
|
||||
// directory symlinks are still followed. EvalSymlinks resolves every
|
||||
// component, closing the directory-symlink bypass: a PR that commits
|
||||
// .review-bot/ as a directory symlink pointing outside the repo would
|
||||
// otherwise pass the filepath.Rel confinement check because the textual
|
||||
// path is inside the root while the actual destination is not.
|
||||
resolvedPath, err := filepath.EvalSymlinks(absPath)
|
||||
if err != nil {
|
||||
return fmt.Errorf("cannot resolve path (symlink): %w", err)
|
||||
}
|
||||
|
||||
// Lstat the resolved path — at this point resolvedPath is symlink-free, so
|
||||
// ModeSymlink will never be set. We keep the check as defense-in-depth.
|
||||
fi, err := os.Lstat(resolvedPath)
|
||||
if err != nil {
|
||||
return fmt.Errorf("cannot stat file: %w", err)
|
||||
}
|
||||
|
||||
// Defense-in-depth: reject any remaining symlink indicator.
|
||||
if fi.Mode()&os.ModeSymlink != 0 {
|
||||
return fmt.Errorf("symlinks are not allowed")
|
||||
}
|
||||
|
||||
// Confine to resolvedRoot: use the fully-resolved path so that a directory
|
||||
// symlink inside the repo cannot carry the path outside the root.
|
||||
rel, err := filepath.Rel(resolvedRoot, resolvedPath)
|
||||
if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) {
|
||||
return fmt.Errorf("path must be within --repo-root")
|
||||
}
|
||||
|
||||
// Enforce size cap before reading to prevent memory exhaustion.
|
||||
if fi.Size() > maxDocmapBytes {
|
||||
return fmt.Errorf("file size %d bytes exceeds %d-byte limit", fi.Size(), maxDocmapBytes)
|
||||
}
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
// runValidateDocmap implements the `review-bot validate-docmap` subcommand.
|
||||
//
|
||||
// It reads changed file paths from stdin (one per line, as produced by
|
||||
// `git diff --name-only`), parses a doc-map YAML file, and performs two checks:
|
||||
//
|
||||
// 1. Coverage check: every changed file must be matched by at least one
|
||||
// paths: glob in the docmap. Fails if any file is uncovered.
|
||||
//
|
||||
// 2. Stale-docs check: every docs: entry in the docmap must exist on disk
|
||||
// (relative to --repo-root). Fails if any path is missing.
|
||||
//
|
||||
// Both checks always run — all failures are reported before exiting.
|
||||
//
|
||||
// Exit codes:
|
||||
//
|
||||
// 0 — clean (all files covered, all docs exist)
|
||||
// 1 — one or more coverage or stale-doc failures
|
||||
// 2 — usage error, missing flag, or YAML parse error
|
||||
func runValidateDocmap(args []string) int {
|
||||
fs := flag.NewFlagSet("validate-docmap", flag.ContinueOnError)
|
||||
fs.SetOutput(errWriter)
|
||||
|
||||
docmapFlag := fs.String("docmap", "", "Path to doc-map YAML file (required)")
|
||||
repoRootFlag := fs.String("repo-root", ".", "Repo root for resolving docs: paths (default: cwd)")
|
||||
|
||||
if err := fs.Parse(args); err != nil {
|
||||
// flag.ContinueOnError already wrote the error to errWriter.
|
||||
return 2
|
||||
}
|
||||
|
||||
if *docmapFlag == "" {
|
||||
fmt.Fprintln(errWriter, "Error: --docmap is required")
|
||||
fmt.Fprintln(errWriter, "")
|
||||
fmt.Fprintln(errWriter, "usage: review-bot validate-docmap --docmap <path> [--repo-root <dir>]")
|
||||
fmt.Fprintln(errWriter, " Changed files are read from stdin, one per line.")
|
||||
fmt.Fprintln(errWriter, " Example: git diff --name-only origin/main HEAD | review-bot validate-docmap --docmap .review-bot/doc-map.yml")
|
||||
return 2
|
||||
}
|
||||
|
||||
// Resolve repoRoot first — the docmap path is validated against it below.
|
||||
// Use an absolute, symlink-free path so a symlinked --repo-root cannot
|
||||
// bypass the escape guard in validateDocmapPath or checkStaleDocs.
|
||||
absRoot, err := filepath.Abs(*repoRootFlag)
|
||||
if err != nil {
|
||||
fmt.Fprintf(errWriter, "Error: failed to resolve --repo-root %q: %v\n", *repoRootFlag, err)
|
||||
return 2
|
||||
}
|
||||
resolvedRoot, err := filepath.EvalSymlinks(absRoot)
|
||||
if err != nil {
|
||||
if os.IsNotExist(err) {
|
||||
fmt.Fprintf(errWriter, "Error: --repo-root %q does not exist\n", *repoRootFlag)
|
||||
} else {
|
||||
fmt.Fprintf(errWriter, "Error: failed to resolve --repo-root %q: %v\n", *repoRootFlag, err)
|
||||
}
|
||||
return 2
|
||||
}
|
||||
|
||||
// Harden the docmap file path before reading it. The --docmap flag value
|
||||
// may reference a PR-controlled file (e.g. .review-bot/doc-map.yml).
|
||||
// Validate that it:
|
||||
// 1. Resolves within resolvedRoot (prevent reading arbitrary host files).
|
||||
// 2. Is not a symlink (prevent /dev/zero or symlink-based host probing).
|
||||
// 3. Does not exceed maxDocmapBytes (prevent memory exhaustion from an
|
||||
// oversized committed file).
|
||||
if err := validateDocmapPath(*docmapFlag, resolvedRoot); err != nil {
|
||||
fmt.Fprintf(errWriter, "Error: --docmap %q is invalid: %v\n", *docmapFlag, err)
|
||||
return 2
|
||||
}
|
||||
|
||||
// Parse docmap YAML.
|
||||
cfg, err := review.ParseDocMapConfig(*docmapFlag)
|
||||
if err != nil {
|
||||
fmt.Fprintf(errWriter, "Error: failed to parse docmap %q: %v\n", *docmapFlag, err)
|
||||
return 2
|
||||
}
|
||||
|
||||
// Read changed files from stdin.
|
||||
changedFiles, err := readLines(os.Stdin)
|
||||
if err != nil {
|
||||
fmt.Fprintf(errWriter, "Error: failed to read stdin: %v\n", err)
|
||||
return 2
|
||||
}
|
||||
|
||||
failed := false
|
||||
|
||||
// --- Check 1: Coverage ---
|
||||
// Note: an empty docmap (no mappings) means every changed file is
|
||||
// uncovered — there are no patterns to match against. This is intentional:
|
||||
// if you declare a doc-map, every changed file must be accounted for.
|
||||
// On empty stdin the check is vacuously true (no files to cover).
|
||||
var uncovered []string
|
||||
for _, f := range changedFiles {
|
||||
// Normalize Windows-style backslashes to forward slashes so that
|
||||
// changed-file paths from git on Windows match doc-map globs.
|
||||
f = strings.ReplaceAll(f, "\\", "/")
|
||||
if !review.FileCoveredByDocMap(cfg, f) {
|
||||
uncovered = append(uncovered, f)
|
||||
}
|
||||
}
|
||||
if len(uncovered) > 0 {
|
||||
failed = true
|
||||
fmt.Fprintln(errWriter, "ERROR: changed files with no docmap coverage:")
|
||||
for _, f := range uncovered {
|
||||
fmt.Fprintf(errWriter, " %s\n", f)
|
||||
}
|
||||
}
|
||||
|
||||
// --- Check 2: Stale docs ---
|
||||
// checkStaleDocs validates each path before touching the filesystem; see
|
||||
// its documentation for the path-traversal hardening applied.
|
||||
staleDocs := checkStaleDocs(cfg, resolvedRoot)
|
||||
if len(staleDocs) > 0 {
|
||||
failed = true
|
||||
fmt.Fprintln(errWriter, "ERROR: stale docmap docs: entries (paths do not exist):")
|
||||
for _, d := range staleDocs {
|
||||
fmt.Fprintf(errWriter, " %s\n", d)
|
||||
}
|
||||
}
|
||||
|
||||
if failed {
|
||||
return 1
|
||||
}
|
||||
|
||||
fmt.Fprintln(outWriter, "OK: docmap is valid")
|
||||
return 0
|
||||
}
|
||||
|
||||
// checkStaleDocs returns deduplicated docs: entries that do not exist under
|
||||
// repoRoot.
|
||||
//
|
||||
// Path-traversal hardening: each docPath is validated with
|
||||
// review.ValidateDocPath (rejects absolute paths and ".." segments) and then
|
||||
// confined to repoRoot via filepath.Clean + filepath.Rel before os.Lstat is
|
||||
// called. Symlinks are treated as stale — a CI tool running against
|
||||
// PR-controlled content must not follow symlinks that could probe arbitrary
|
||||
// host paths. Paths that fail any check are treated as invalid (reported as
|
||||
// stale) without following any symlinks.
|
||||
func checkStaleDocs(cfg *review.DocMapConfig, repoRoot string) []string {
|
||||
seen := make(map[string]struct{})
|
||||
var stale []string
|
||||
|
||||
for _, mapping := range cfg.Mappings {
|
||||
for _, docPath := range mapping.Docs {
|
||||
if docPath == "" {
|
||||
continue
|
||||
}
|
||||
if _, ok := seen[docPath]; ok {
|
||||
continue
|
||||
}
|
||||
seen[docPath] = struct{}{}
|
||||
|
||||
// Guard 1: reject absolute paths and ".." segments sourced from
|
||||
// PR-controlled YAML before joining with repoRoot.
|
||||
if err := review.ValidateDocPath(docPath); err != nil {
|
||||
stale = append(stale, docPath)
|
||||
continue
|
||||
}
|
||||
|
||||
// Guard 2: verify the cleaned joined path does not escape repoRoot.
|
||||
// filepath.Clean resolves any remaining ".." after the join; the
|
||||
// filepath.Rel check confirms the path is still under repoRoot.
|
||||
fullPath := filepath.Clean(filepath.Join(repoRoot, filepath.FromSlash(docPath)))
|
||||
rel, err := filepath.Rel(repoRoot, fullPath)
|
||||
if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) {
|
||||
stale = append(stale, docPath)
|
||||
continue
|
||||
}
|
||||
|
||||
// Use Lstat (not Stat) so symlinks are never followed. A symlink
|
||||
// under repoRoot could point anywhere on the host, allowing a
|
||||
// malicious PR to probe file existence. Treat symlinks as stale.
|
||||
fi, err := os.Lstat(fullPath)
|
||||
if err != nil {
|
||||
stale = append(stale, docPath)
|
||||
continue
|
||||
}
|
||||
if fi.Mode()&os.ModeSymlink != 0 {
|
||||
stale = append(stale, docPath)
|
||||
}
|
||||
}
|
||||
}
|
||||
return stale
|
||||
}
|
||||
|
||||
// readLines reads all non-empty trimmed lines from r.
|
||||
func readLines(r io.Reader) ([]string, error) {
|
||||
scanner := bufio.NewScanner(r)
|
||||
var lines []string
|
||||
for scanner.Scan() {
|
||||
line := strings.TrimSpace(scanner.Text())
|
||||
if line != "" {
|
||||
lines = append(lines, line)
|
||||
}
|
||||
}
|
||||
return lines, scanner.Err()
|
||||
}
|
||||
@@ -1,601 +0,0 @@
|
||||
package main
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"testing"
|
||||
)
|
||||
|
||||
// makeDocmapYAML writes a YAML string to a temp file and returns its path.
|
||||
// The file is created in t.TempDir() — use makeDocmapInDir when the docmap
|
||||
// must be located inside a specific repo-root directory.
|
||||
func makeDocmapYAML(t *testing.T, content string) string {
|
||||
t.Helper()
|
||||
f, err := os.CreateTemp(t.TempDir(), "doc-map-*.yml")
|
||||
if err != nil {
|
||||
t.Fatalf("CreateTemp: %v", err)
|
||||
}
|
||||
defer f.Close()
|
||||
if _, err := f.WriteString(content); err != nil {
|
||||
t.Fatalf("WriteString: %v", err)
|
||||
}
|
||||
return f.Name()
|
||||
}
|
||||
|
||||
// makeDocmapInDir writes a YAML string to a file inside dir and returns the
|
||||
// file path. Use this instead of makeDocmapYAML when also passing --repo-root,
|
||||
// because validateDocmapPath requires the docmap to be within the repo root.
|
||||
func makeDocmapInDir(t *testing.T, dir, content string) string {
|
||||
t.Helper()
|
||||
if err := os.MkdirAll(filepath.Join(dir, ".review-bot"), 0o755); err != nil {
|
||||
t.Fatalf("MkdirAll: %v", err)
|
||||
}
|
||||
path := filepath.Join(dir, ".review-bot", "doc-map.yml")
|
||||
if err := os.WriteFile(path, []byte(content), 0o644); err != nil {
|
||||
t.Fatalf("WriteFile: %v", err)
|
||||
}
|
||||
return path
|
||||
}
|
||||
|
||||
// makeDocFile creates a file (and any parent dirs) at the given path relative to dir.
|
||||
func makeDocFile(t *testing.T, dir, rel string) {
|
||||
t.Helper()
|
||||
full := filepath.Join(dir, rel)
|
||||
if err := os.MkdirAll(filepath.Dir(full), 0o755); err != nil {
|
||||
t.Fatalf("MkdirAll: %v", err)
|
||||
}
|
||||
if err := os.WriteFile(full, []byte("# doc\n"), 0o644); err != nil {
|
||||
t.Fatalf("WriteFile: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
// captureOutput redirects outWriter/errWriter to buffers for the duration of f.
|
||||
func captureOutput(f func()) (stdout, stderr string) {
|
||||
var outBuf, errBuf bytes.Buffer
|
||||
origOut, origErr := outWriter, errWriter
|
||||
outWriter = &outBuf
|
||||
errWriter = &errBuf
|
||||
defer func() {
|
||||
outWriter = origOut
|
||||
errWriter = origErr
|
||||
}()
|
||||
f()
|
||||
return outBuf.String(), errBuf.String()
|
||||
}
|
||||
|
||||
func TestRunValidateDocmap_Clean(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
makeDocFile(t, dir, "docs/foo.md")
|
||||
|
||||
docmap := makeDocmapInDir(t, dir, `
|
||||
mappings:
|
||||
- paths:
|
||||
- "lib/foo/**"
|
||||
docs:
|
||||
- docs/foo.md
|
||||
`)
|
||||
|
||||
// A covered file with all docs existing → clean.
|
||||
code, stdout, _ := stdinValidateDocmap(t,
|
||||
"lib/foo/bar.ex\n",
|
||||
[]string{"--docmap", docmap, "--repo-root", dir},
|
||||
)
|
||||
if code != 0 {
|
||||
t.Errorf("expected exit 0 for clean, got %d", code)
|
||||
}
|
||||
if !strings.Contains(stdout, "OK") {
|
||||
t.Errorf("expected 'OK' in stdout, got %q", stdout)
|
||||
}
|
||||
}
|
||||
|
||||
func TestRunValidateDocmap_MissingDocmapFlag(t *testing.T) {
|
||||
var code int
|
||||
_, stderr := captureOutput(func() {
|
||||
code = runValidateDocmap([]string{})
|
||||
})
|
||||
if code != 2 {
|
||||
t.Errorf("expected exit 2 for missing --docmap, got %d", code)
|
||||
}
|
||||
if !strings.Contains(stderr, "--docmap") {
|
||||
t.Errorf("expected --docmap in stderr, got %q", stderr)
|
||||
}
|
||||
}
|
||||
|
||||
func TestRunValidateDocmap_BadYAML(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
docmap := makeDocmapInDir(t, dir, "mappings: [{{invalid")
|
||||
var code int
|
||||
_, stderr := captureOutput(func() {
|
||||
code = runValidateDocmap([]string{"--docmap", docmap, "--repo-root", dir})
|
||||
})
|
||||
if code != 2 {
|
||||
t.Errorf("expected exit 2 for bad YAML, got %d", code)
|
||||
}
|
||||
if !strings.Contains(stderr, "failed to parse") {
|
||||
t.Errorf("expected parse error in stderr, got %q", stderr)
|
||||
}
|
||||
}
|
||||
|
||||
func TestRunValidateDocmap_StaleDocs(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
// docs/foo.md does NOT exist on disk.
|
||||
|
||||
docmap := makeDocmapInDir(t, dir, `
|
||||
mappings:
|
||||
- paths:
|
||||
- "lib/foo/**"
|
||||
docs:
|
||||
- docs/foo.md
|
||||
`)
|
||||
|
||||
var code int
|
||||
_, stderr := captureOutput(func() {
|
||||
code = runValidateDocmap([]string{
|
||||
"--docmap", docmap,
|
||||
"--repo-root", dir,
|
||||
})
|
||||
})
|
||||
if code != 1 {
|
||||
t.Errorf("expected exit 1 for stale docs, got %d", code)
|
||||
}
|
||||
if !strings.Contains(stderr, "docs/foo.md") {
|
||||
t.Errorf("expected stale path in stderr, got %q", stderr)
|
||||
}
|
||||
if !strings.Contains(stderr, "stale docmap") {
|
||||
t.Errorf("expected 'stale docmap' in stderr, got %q", stderr)
|
||||
}
|
||||
}
|
||||
|
||||
// stdinValidateDocmap runs runValidateDocmap with a synthetic stdin.
|
||||
//
|
||||
// Implementation note: we write stdinContent to a temp file and point
|
||||
// os.Stdin at it. The defer f.Close() fires after stdinValidateDocmap
|
||||
// returns, which is after runValidateDocmap has finished reading stdin
|
||||
// synchronously — so the file is not closed while still in use.
|
||||
// Tests must not call t.Parallel() while sharing the global os.Stdin.
|
||||
func stdinValidateDocmap(t *testing.T, stdinContent string, args []string) (code int, stdout, stderr string) {
|
||||
t.Helper()
|
||||
// Write stdin content to a temp file and redirect os.Stdin.
|
||||
f, err := os.CreateTemp(t.TempDir(), "stdin-*")
|
||||
if err != nil {
|
||||
t.Fatalf("CreateTemp for stdin: %v", err)
|
||||
}
|
||||
defer f.Close()
|
||||
if _, err := f.WriteString(stdinContent); err != nil {
|
||||
t.Fatalf("WriteString for stdin: %v", err)
|
||||
}
|
||||
if _, err := f.Seek(0, 0); err != nil {
|
||||
t.Fatalf("Seek for stdin: %v", err)
|
||||
}
|
||||
|
||||
origStdin := os.Stdin
|
||||
os.Stdin = f
|
||||
defer func() { os.Stdin = origStdin }()
|
||||
|
||||
stdout, stderr = captureOutput(func() {
|
||||
code = runValidateDocmap(args)
|
||||
})
|
||||
return
|
||||
}
|
||||
|
||||
func TestRunValidateDocmap_UncoveredFile(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
makeDocFile(t, dir, "docs/foo.md")
|
||||
|
||||
docmap := makeDocmapInDir(t, dir, `
|
||||
mappings:
|
||||
- paths:
|
||||
- "lib/foo/**"
|
||||
docs:
|
||||
- docs/foo.md
|
||||
`)
|
||||
|
||||
code, _, stderr := stdinValidateDocmap(t,
|
||||
"lib/bar/uncovered.ex\n",
|
||||
[]string{"--docmap", docmap, "--repo-root", dir},
|
||||
)
|
||||
if code != 1 {
|
||||
t.Errorf("expected exit 1 for uncovered file, got %d", code)
|
||||
}
|
||||
if !strings.Contains(stderr, "lib/bar/uncovered.ex") {
|
||||
t.Errorf("expected uncovered file in stderr, got %q", stderr)
|
||||
}
|
||||
if !strings.Contains(stderr, "no docmap coverage") {
|
||||
t.Errorf("expected 'no docmap coverage' in stderr, got %q", stderr)
|
||||
}
|
||||
}
|
||||
|
||||
func TestRunValidateDocmap_BothFailures(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
// docs/foo.md intentionally missing
|
||||
|
||||
docmap := makeDocmapInDir(t, dir, `
|
||||
mappings:
|
||||
- paths:
|
||||
- "lib/foo/**"
|
||||
docs:
|
||||
- docs/foo.md
|
||||
`)
|
||||
|
||||
code, _, stderr := stdinValidateDocmap(t,
|
||||
"lib/bar/uncovered.ex\n",
|
||||
[]string{"--docmap", docmap, "--repo-root", dir},
|
||||
)
|
||||
if code != 1 {
|
||||
t.Errorf("expected exit 1 for both failures, got %d", code)
|
||||
}
|
||||
if !strings.Contains(stderr, "no docmap coverage") {
|
||||
t.Errorf("expected coverage error in stderr, got %q", stderr)
|
||||
}
|
||||
if !strings.Contains(stderr, "stale docmap") {
|
||||
t.Errorf("expected stale-docs error in stderr, got %q", stderr)
|
||||
}
|
||||
}
|
||||
|
||||
func TestRunValidateDocmap_EmptyStdin(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
makeDocFile(t, dir, "docs/foo.md")
|
||||
|
||||
docmap := makeDocmapInDir(t, dir, `
|
||||
mappings:
|
||||
- paths:
|
||||
- "lib/foo/**"
|
||||
docs:
|
||||
- docs/foo.md
|
||||
`)
|
||||
|
||||
code, stdout, _ := stdinValidateDocmap(t,
|
||||
"",
|
||||
[]string{"--docmap", docmap, "--repo-root", dir},
|
||||
)
|
||||
if code != 0 {
|
||||
t.Errorf("expected exit 0 for empty stdin, got %d", code)
|
||||
}
|
||||
if !strings.Contains(stdout, "OK") {
|
||||
t.Errorf("expected 'OK' in stdout, got %q", stdout)
|
||||
}
|
||||
}
|
||||
|
||||
func TestRunValidateDocmap_BlankLinesSkipped(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
makeDocFile(t, dir, "docs/foo.md")
|
||||
|
||||
docmap := makeDocmapInDir(t, dir, `
|
||||
mappings:
|
||||
- paths:
|
||||
- "lib/foo/**"
|
||||
docs:
|
||||
- docs/foo.md
|
||||
`)
|
||||
|
||||
// stdin with only blank lines → effectively empty, should be clean
|
||||
code, stdout, _ := stdinValidateDocmap(t,
|
||||
"\n \n\n",
|
||||
[]string{"--docmap", docmap, "--repo-root", dir},
|
||||
)
|
||||
if code != 0 {
|
||||
t.Errorf("expected exit 0 for blank-only stdin, got %d", code)
|
||||
}
|
||||
if !strings.Contains(stdout, "OK") {
|
||||
t.Errorf("expected 'OK' in stdout for blank-only stdin, got %q", stdout)
|
||||
}
|
||||
}
|
||||
|
||||
func TestRunValidateDocmap_DuplicateDocsDeduped(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
// docs/shared.md intentionally missing — but it appears in TWO mappings.
|
||||
// Should appear only once in stale list.
|
||||
|
||||
docmap := makeDocmapInDir(t, dir, `
|
||||
mappings:
|
||||
- paths:
|
||||
- "lib/foo/**"
|
||||
docs:
|
||||
- docs/shared.md
|
||||
- paths:
|
||||
- "lib/bar/**"
|
||||
docs:
|
||||
- docs/shared.md
|
||||
`)
|
||||
|
||||
code, _, stderr := stdinValidateDocmap(t,
|
||||
"",
|
||||
[]string{"--docmap", docmap, "--repo-root", dir},
|
||||
)
|
||||
if code != 1 {
|
||||
t.Errorf("expected exit 1 for stale doc, got %d", code)
|
||||
}
|
||||
count := strings.Count(stderr, "docs/shared.md")
|
||||
if count != 1 {
|
||||
t.Errorf("expected docs/shared.md to appear exactly once in stderr (deduplicated), got %d occurrences: %q", count, stderr)
|
||||
}
|
||||
}
|
||||
|
||||
// TestCheckStaleDocs_PathTraversal verifies that checkStaleDocs rejects
|
||||
// traversal and absolute paths without touching the host filesystem.
|
||||
func TestCheckStaleDocs_PathTraversal(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
|
||||
// Baseline: a valid doc that exists.
|
||||
makeDocFile(t, dir, "docs/valid.md")
|
||||
|
||||
tests := []struct {
|
||||
name string
|
||||
docPath string
|
||||
wantStale bool
|
||||
}{
|
||||
{"dot-dot traversal", "../../etc/passwd", true},
|
||||
{"dot-dot single", "../outside", true},
|
||||
{"absolute path", "/etc/passwd", true},
|
||||
{"valid present path", "docs/valid.md", false},
|
||||
{"valid missing path", "docs/missing.md", true},
|
||||
}
|
||||
|
||||
for _, tc := range tests {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
docmap := makeDocmapInDir(t, dir, `
|
||||
mappings:
|
||||
- paths:
|
||||
- "lib/**"
|
||||
docs:
|
||||
- `+tc.docPath+`
|
||||
`)
|
||||
code, _, stderr := stdinValidateDocmap(t,
|
||||
"",
|
||||
[]string{"--docmap", docmap, "--repo-root", dir},
|
||||
)
|
||||
|
||||
if tc.wantStale {
|
||||
if code != 1 {
|
||||
t.Errorf("path %q: expected exit 1 (stale/invalid), got %d; stderr: %q", tc.docPath, code, stderr)
|
||||
}
|
||||
} else {
|
||||
if code != 0 {
|
||||
t.Errorf("path %q: expected exit 0 (valid), got %d; stderr: %q", tc.docPath, code, stderr)
|
||||
}
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestCheckStaleDocs_SymlinkOutside verifies that a symlink under repoRoot
|
||||
// pointing outside the repo is treated as stale (not followed).
|
||||
func TestCheckStaleDocs_SymlinkOutside(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
|
||||
// Create a symlink inside repoRoot pointing to a file outside the repo.
|
||||
// We point at /etc/hostname (exists on Linux CI) but the test does not
|
||||
// depend on that file existing — Lstat must reject the symlink itself.
|
||||
linkPath := filepath.Join(dir, "docs", "secret.md")
|
||||
if err := os.MkdirAll(filepath.Dir(linkPath), 0o755); err != nil {
|
||||
t.Fatalf("MkdirAll: %v", err)
|
||||
}
|
||||
if err := os.Symlink("/etc/hostname", linkPath); err != nil {
|
||||
t.Fatalf("Symlink: %v", err)
|
||||
}
|
||||
|
||||
docmap := makeDocmapInDir(t, dir, `
|
||||
mappings:
|
||||
- paths:
|
||||
- "lib/**"
|
||||
docs:
|
||||
- docs/secret.md
|
||||
`)
|
||||
|
||||
code, _, stderr := stdinValidateDocmap(t,
|
||||
"",
|
||||
[]string{"--docmap", docmap, "--repo-root", dir},
|
||||
)
|
||||
if code != 1 {
|
||||
t.Errorf("expected exit 1 for symlink doc, got %d; stderr: %q", code, stderr)
|
||||
}
|
||||
if !strings.Contains(stderr, "docs/secret.md") {
|
||||
t.Errorf("expected stale path in stderr, got %q", stderr)
|
||||
}
|
||||
}
|
||||
|
||||
// TestCheckStaleDocs_SymlinkInsideRepo verifies that a symlink pointing to
|
||||
// another file *within* the repo is also treated as stale. We refuse all
|
||||
// symlinks regardless of target to keep the check simple and safe.
|
||||
func TestCheckStaleDocs_SymlinkInsideRepo(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
|
||||
// Real doc file.
|
||||
makeDocFile(t, dir, "docs/real.md")
|
||||
|
||||
// Symlink inside repo pointing at the real file.
|
||||
linkPath := filepath.Join(dir, "docs", "link.md")
|
||||
if err := os.Symlink(filepath.Join(dir, "docs", "real.md"), linkPath); err != nil {
|
||||
t.Fatalf("Symlink: %v", err)
|
||||
}
|
||||
|
||||
docmap := makeDocmapInDir(t, dir, `
|
||||
mappings:
|
||||
- paths:
|
||||
- "lib/**"
|
||||
docs:
|
||||
- docs/link.md
|
||||
`)
|
||||
|
||||
code, _, stderr := stdinValidateDocmap(t,
|
||||
"",
|
||||
[]string{"--docmap", docmap, "--repo-root", dir},
|
||||
)
|
||||
if code != 1 {
|
||||
t.Errorf("expected exit 1 for symlink doc (even intra-repo), got %d; stderr: %q", code, stderr)
|
||||
}
|
||||
}
|
||||
|
||||
// TestRunValidateDocmap_SymlinkRepoRoot verifies that a --repo-root that is
|
||||
// itself a symlink to a valid directory resolves correctly.
|
||||
func TestRunValidateDocmap_SymlinkRepoRoot(t *testing.T) {
|
||||
realDir := t.TempDir()
|
||||
makeDocFile(t, realDir, "docs/foo.md")
|
||||
|
||||
// Create a symlink pointing at realDir.
|
||||
symlinkDir := filepath.Join(t.TempDir(), "link-root")
|
||||
if err := os.Symlink(realDir, symlinkDir); err != nil {
|
||||
t.Fatalf("Symlink: %v", err)
|
||||
}
|
||||
|
||||
// Place the docmap inside realDir so it passes the confinement check.
|
||||
// (symlinkDir resolves to realDir, so files inside realDir are also inside
|
||||
// the resolved repo-root.)
|
||||
docmap := makeDocmapInDir(t, realDir, `
|
||||
mappings:
|
||||
- paths:
|
||||
- "lib/**"
|
||||
docs:
|
||||
- docs/foo.md
|
||||
`)
|
||||
|
||||
// Using the symlinked repo-root: the real doc exists → should be clean.
|
||||
code, stdout, stderr := stdinValidateDocmap(t,
|
||||
"lib/foo.go\n",
|
||||
[]string{"--docmap", docmap, "--repo-root", symlinkDir},
|
||||
)
|
||||
if code != 0 {
|
||||
t.Errorf("expected exit 0 for symlinked repo-root with existing doc, got %d; stderr: %q", code, stderr)
|
||||
}
|
||||
if !strings.Contains(stdout, "OK") {
|
||||
t.Errorf("expected 'OK' in stdout, got %q", stdout)
|
||||
}
|
||||
}
|
||||
|
||||
// TestValidateDocmapPath_Symlink verifies that --docmap pointing at a symlink
|
||||
// whose resolved target is outside --repo-root is rejected (prevents reading
|
||||
// arbitrary host files via PR-controlled symlinks).
|
||||
//
|
||||
// Note: after the EvalSymlinks fix (issue #150), in-repo symlinks whose
|
||||
// targets also reside within the repo root are now allowed — the confinement
|
||||
// check is applied to the resolved path, not the symlink entry itself. The
|
||||
// security invariant is: the resolved destination must be within the root.
|
||||
func TestValidateDocmapPath_Symlink(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
outside := t.TempDir()
|
||||
|
||||
// Create a docmap file OUTSIDE the repo root to serve as the symlink
|
||||
// target. EvalSymlinks will resolve to this path, which the Rel check
|
||||
// must then reject.
|
||||
if err := os.MkdirAll(filepath.Join(outside, ".review-bot"), 0o755); err != nil {
|
||||
t.Fatalf("MkdirAll: %v", err)
|
||||
}
|
||||
outsideDocmap := filepath.Join(outside, ".review-bot", "doc-map.yml")
|
||||
if err := os.WriteFile(outsideDocmap, []byte("mappings: []\n"), 0o644); err != nil {
|
||||
t.Fatalf("WriteFile: %v", err)
|
||||
}
|
||||
|
||||
// Create a symlink inside dir pointing to the file outside the repo.
|
||||
if err := os.MkdirAll(filepath.Join(dir, ".review-bot"), 0o755); err != nil {
|
||||
t.Fatalf("MkdirAll: %v", err)
|
||||
}
|
||||
symlinkPath := filepath.Join(dir, ".review-bot", "doc-map-link.yml")
|
||||
if err := os.Symlink(outsideDocmap, symlinkPath); err != nil {
|
||||
t.Fatalf("Symlink: %v", err)
|
||||
}
|
||||
|
||||
code, _, stderr := stdinValidateDocmap(t,
|
||||
"",
|
||||
[]string{"--docmap", symlinkPath, "--repo-root", dir},
|
||||
)
|
||||
if code != 2 {
|
||||
t.Errorf("expected exit 2 for out-of-repo symlink docmap, got %d; stderr: %q", code, stderr)
|
||||
}
|
||||
if !strings.Contains(stderr, "invalid") && !strings.Contains(stderr, "repo-root") {
|
||||
t.Errorf("expected confinement rejection in stderr, got %q", stderr)
|
||||
}
|
||||
}
|
||||
|
||||
// TestValidateDocmapPath_OutsideRepoRoot verifies that --docmap pointing
|
||||
// outside --repo-root is rejected (prevents reading arbitrary host files).
|
||||
func TestValidateDocmapPath_OutsideRepoRoot(t *testing.T) {
|
||||
repoDir := t.TempDir()
|
||||
|
||||
// Create a docmap in a separate temp dir (outside the repo root).
|
||||
outside := makeDocmapYAML(t, `
|
||||
mappings:
|
||||
- paths:
|
||||
- "lib/**"
|
||||
docs:
|
||||
- docs/foo.md
|
||||
`)
|
||||
|
||||
code, _, stderr := stdinValidateDocmap(t,
|
||||
"",
|
||||
[]string{"--docmap", outside, "--repo-root", repoDir},
|
||||
)
|
||||
if code != 2 {
|
||||
t.Errorf("expected exit 2 for docmap outside repo-root, got %d; stderr: %q", code, stderr)
|
||||
}
|
||||
if !strings.Contains(stderr, "invalid") && !strings.Contains(stderr, "repo-root") {
|
||||
t.Errorf("expected confinement rejection in stderr, got %q", stderr)
|
||||
}
|
||||
}
|
||||
|
||||
// TestValidateDocmapPath_SizeLimit verifies that --docmap files exceeding
|
||||
// maxDocmapBytes are rejected before reading (prevents memory exhaustion).
|
||||
func TestValidateDocmapPath_SizeLimit(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
|
||||
// Write a file larger than maxDocmapBytes.
|
||||
bigPath := filepath.Join(dir, ".review-bot", "big-doc-map.yml")
|
||||
if err := os.MkdirAll(filepath.Dir(bigPath), 0o755); err != nil {
|
||||
t.Fatalf("MkdirAll: %v", err)
|
||||
}
|
||||
// Exceed the limit by one byte.
|
||||
bigContent := make([]byte, maxDocmapBytes+1)
|
||||
if err := os.WriteFile(bigPath, bigContent, 0o644); err != nil {
|
||||
t.Fatalf("WriteFile: %v", err)
|
||||
}
|
||||
|
||||
code, _, stderr := stdinValidateDocmap(t,
|
||||
"",
|
||||
[]string{"--docmap", bigPath, "--repo-root", dir},
|
||||
)
|
||||
if code != 2 {
|
||||
t.Errorf("expected exit 2 for oversized docmap, got %d; stderr: %q", code, stderr)
|
||||
}
|
||||
if !strings.Contains(stderr, "limit") && !strings.Contains(stderr, "size") && !strings.Contains(stderr, "invalid") {
|
||||
t.Errorf("expected size limit error in stderr, got %q", stderr)
|
||||
}
|
||||
}
|
||||
|
||||
// TestValidateDocmapPath_DirSymlinkBypass verifies that a directory-symlink
|
||||
// inside the repo pointing outside cannot be used to read arbitrary host files.
|
||||
//
|
||||
// Attack vector: a PR commits .review-bot/ as a directory symlink targeting a
|
||||
// directory outside the repo. The textual path of the docmap file is inside
|
||||
// the repo root, so the old Rel-only check passed — but the actual file is
|
||||
// outside. This is closed by calling EvalSymlinks on the full path before the
|
||||
// confinement check.
|
||||
func TestValidateDocmapPath_DirSymlinkBypass(t *testing.T) {
|
||||
repoDir := t.TempDir()
|
||||
outsideDir := t.TempDir()
|
||||
|
||||
// Secret file outside the repo.
|
||||
secretPath := filepath.Join(outsideDir, "secret.yml")
|
||||
if err := os.WriteFile(secretPath, []byte("mappings: []\n"), 0o644); err != nil {
|
||||
t.Fatalf("WriteFile: %v", err)
|
||||
}
|
||||
|
||||
// Create .review-bot/ as a directory symlink pointing outside the repo.
|
||||
reviewBotDir := filepath.Join(repoDir, ".review-bot")
|
||||
if err := os.Symlink(outsideDir, reviewBotDir); err != nil {
|
||||
t.Skipf("cannot create dir symlink (platform may not support it): %v", err)
|
||||
}
|
||||
|
||||
// Textually inside repo — .review-bot/secret.yml — but resolves outside.
|
||||
attackPath := filepath.Join(repoDir, ".review-bot", "secret.yml")
|
||||
|
||||
// Resolve repoDir to a symlink-free path, as runValidateDocmap does.
|
||||
resolvedRoot, err := filepath.EvalSymlinks(repoDir)
|
||||
if err != nil {
|
||||
t.Fatalf("EvalSymlinks(repoDir): %v", err)
|
||||
}
|
||||
|
||||
if err := validateDocmapPath(attackPath, resolvedRoot); err == nil {
|
||||
t.Error("expected rejection of dir-symlink bypass, got nil error")
|
||||
}
|
||||
}
|
||||
@@ -1,278 +0,0 @@
|
||||
# Dev-Loop Dispatch Spec
|
||||
|
||||
**Version:** 1.0
|
||||
**Status:** Implemented
|
||||
**Implements:** Issue #148
|
||||
|
||||
This document is the authoritative spec for the review-bot dev-loop dispatch architecture.
|
||||
The dispatch script (`~/.openclaw/workspace/scripts/dev-loop-dispatch.sh`) and its tests
|
||||
are validated against the rules and invariants in this document.
|
||||
|
||||
---
|
||||
|
||||
## 1. Overview
|
||||
|
||||
The dev-loop is a 15-minute cron that advances the state of open pull requests and picks up
|
||||
new issues when there is nothing in review. It is designed for **zero human intervention**
|
||||
in the normal flow and **hard stops at key safety boundaries**.
|
||||
|
||||
### Architecture
|
||||
|
||||
```
|
||||
Cron (15-min cadence)
|
||||
→ exec: bash dev-loop-dispatch.sh <project>
|
||||
→ read stdout for SPAWN/HANDOFF lines
|
||||
→ if SPAWN: load worker template, spawn subagent
|
||||
→ if HANDOFF: log, do nothing else
|
||||
→ if neither: NO_REPLY
|
||||
```
|
||||
|
||||
The cron model has **no ambient knowledge** of the project state. All state is derived
|
||||
from the dispatch script's output, which in turn comes from live API calls.
|
||||
|
||||
---
|
||||
|
||||
## 2. Inputs
|
||||
|
||||
### Project Config
|
||||
|
||||
```yaml
|
||||
# memory/projects/<project>.yaml
|
||||
repo: rodin/review-bot # <owner>/<repo>
|
||||
api_base: https://gitea.../v1 # API base URL
|
||||
token_path: ~/.openclaw/... # path to bearer token
|
||||
user: rodin # bot Gitea username
|
||||
labels:
|
||||
wip: <id>
|
||||
ready: <id>
|
||||
review_bots: # sentinel names in review bodies
|
||||
- sonnet
|
||||
- gpt
|
||||
- security
|
||||
```
|
||||
|
||||
### Script Arguments
|
||||
|
||||
```bash
|
||||
bash dev-loop-dispatch.sh <project> # normal run
|
||||
DRY_RUN=1 bash dev-loop-dispatch.sh <project> # dry-run (no mutations)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 3. State
|
||||
|
||||
The dispatch script is **stateless per run**. All state lives in the Gitea API:
|
||||
|
||||
| State | API location |
|
||||
|-------|-------------|
|
||||
| Open PRs | `GET /repos/:repo/pulls?state=open` |
|
||||
| PR labels | `GET /repos/:repo/issues/:n/labels` |
|
||||
| PR reviews | `GET /repos/:repo/pulls/:n/reviews` |
|
||||
| CI status | `GET /repos/:repo/commits/:sha/status` |
|
||||
| Issue comments | `GET /repos/:repo/issues/:n/comments` |
|
||||
| Inline diff comments | `GET /repos/:repo/pulls/:n/comments` |
|
||||
| Issue timeline | `GET /repos/:repo/issues/:n/timeline` |
|
||||
|
||||
No file-based state. No cron-to-cron carry-over.
|
||||
|
||||
---
|
||||
|
||||
## 4. Output Protocol
|
||||
|
||||
The script emits structured lines to stdout. Stderr is diagnostic logging.
|
||||
|
||||
### `SPAWN:<type>:<number>:<sha>`
|
||||
|
||||
A worker is needed. The cron model reads this and spawns a subagent using the
|
||||
template at `worker-tasks/<type>.md`.
|
||||
|
||||
| Field | Description |
|
||||
|-------|-------------|
|
||||
| `type` | Worker type: `self-review`, `ci-fix`, `address-feedback`, `findings`, `rebase`, `impl` |
|
||||
| `number` | PR number (or issue number for `impl`) |
|
||||
| `sha` | HEAD SHA of the PR (empty for `impl`) |
|
||||
|
||||
At most **one SPAWN** is emitted per script run.
|
||||
|
||||
### `HANDOFF:<pr_num>`
|
||||
|
||||
All checks passed for `pr_num`. The script applied the `ready` label and assigned
|
||||
to the human reviewer. The cron model logs this and takes no further action.
|
||||
|
||||
Multiple HANDOFFs may be emitted in one run (one per qualifying PR).
|
||||
|
||||
---
|
||||
|
||||
## 5. Dispatch Rules
|
||||
|
||||
Rules are evaluated **in order** for each open PR. The first matching condition wins.
|
||||
Only one SPAWN is emitted per full pass.
|
||||
|
||||
### Rule 0: WIP Cleanup
|
||||
|
||||
For each open PR with a `wip` label:
|
||||
|
||||
1. Find the timestamp when the label was most recently applied (via timeline events)
|
||||
2. If age > 1hr: **remove the label** (stale lock — worker likely crashed)
|
||||
3. If age ≤ 1hr: **set ACTIVE_WIP=1** (do not exit, only gates Rule 10)
|
||||
|
||||
### Rule 2: REQUEST_CHANGES Blocks
|
||||
|
||||
**ALWAYS evaluated before any other per-PR rule.**
|
||||
|
||||
For each reviewer, take their **latest** review state. If any reviewer's latest
|
||||
state is `REQUEST_CHANGES`:
|
||||
|
||||
→ Acquire WIP label on this PR
|
||||
→ Emit `SPAWN:findings:<pr_num>:<head_sha>`
|
||||
→ Continue to next PR (but only one SPAWN total)
|
||||
|
||||
This rule cannot be bypassed by any condition. There is no waiver mechanism.
|
||||
|
||||
### Rule 3: Merge Conflicts
|
||||
|
||||
If `mergeable == false`:
|
||||
|
||||
→ Acquire WIP
|
||||
→ Emit `SPAWN:rebase:<pr_num>:<head_sha>`
|
||||
|
||||
### Rule 4: CI Failure
|
||||
|
||||
If CI state is `failure` or `error`:
|
||||
|
||||
- If a fix plan comment exists for this HEAD SHA: **skip** (worker in progress)
|
||||
- Otherwise:
|
||||
|
||||
→ Acquire WIP
|
||||
→ Emit `SPAWN:ci-fix:<pr_num>:<head_sha>`
|
||||
|
||||
### Rule 5: Bot Reviews Missing
|
||||
|
||||
For each configured `review_bot`, check whether a review body contains the
|
||||
sentinel `<!-- review-bot:<name> -->`.
|
||||
|
||||
If any sentinel is missing: **wait** (continue to next PR, no SPAWN).
|
||||
|
||||
### Rule 6: CI Pending/Unknown
|
||||
|
||||
If CI state is `pending` or `unknown`: **wait**.
|
||||
|
||||
### Rule 7: Self-Review
|
||||
|
||||
Check for a self-review comment from the bot user against the current HEAD SHA:
|
||||
- Comment contains `Self-review against <head_sha>`
|
||||
|
||||
Sub-cases:
|
||||
- **Missing**: No self-review comment →
|
||||
→ Acquire WIP, emit `SPAWN:self-review:<pr_num>:<head_sha>`
|
||||
- **Needs attention** (`Assessment: ⚠️`): Found, but has findings:
|
||||
- Fix plan exists for HEAD SHA: skip
|
||||
- No fix plan: → Acquire WIP, emit `SPAWN:sr-fix:<pr_num>:<head_sha>`
|
||||
- **Clean** (`Assessment: ✅ Clean`): Continue to Rule 8
|
||||
|
||||
### Rule 8: Unacknowledged Bot Review Findings
|
||||
|
||||
For each **current** (contains `Evaluated against <head_short>`) APPROVED bot review
|
||||
that has a findings table:
|
||||
|
||||
A finding is **unacknowledged** if it does not appear as `Finding #N` in a fix plan
|
||||
comment from the bot user for this HEAD SHA.
|
||||
|
||||
If any unacknowledged findings exist:
|
||||
- Fix plan exists: skip
|
||||
- No fix plan: → Acquire WIP, emit `SPAWN:address-feedback:<pr_num>:<head_sha>`
|
||||
|
||||
### Rule 9: Unresolved Inline Diff Comments
|
||||
|
||||
An inline diff comment is **unresolved** if:
|
||||
1. `in_reply_to_id` is null (top-level comment)
|
||||
2. `resolver` is null (not formally resolved)
|
||||
3. No other comment has `in_reply_to_id` pointing to this comment (no reply)
|
||||
|
||||
If unresolved comments exist:
|
||||
- Fix plan exists: skip
|
||||
- No fix plan: → Acquire WIP, emit `SPAWN:address-feedback:<pr_num>:<head_sha>`
|
||||
|
||||
### Rule 10: Handoff
|
||||
|
||||
All rules above passed. Verify all bot reviews are current (contain `Evaluated against <head_short>`).
|
||||
|
||||
If all current:
|
||||
- Apply `ready` label
|
||||
- Assign to `aweiker`
|
||||
- Emit `HANDOFF:<pr_num>`
|
||||
- Continue evaluating remaining PRs (do NOT exit)
|
||||
|
||||
If already assigned to `aweiker`: skip (assume handoff was already performed; continue to next PR without emitting another HANDOFF).
|
||||
|
||||
### Rule 11: New Issue Pickup
|
||||
|
||||
Only runs if: no open PRs exist AND `ACTIVE_WIP == 0`.
|
||||
|
||||
Fetch open, unassigned issues. Priority: bugs first, then by number ascending.
|
||||
|
||||
Claim the issue (assign to bot user to prevent double-pick), then:
|
||||
→ Emit `SPAWN:impl:<issue_num>:`
|
||||
|
||||
---
|
||||
|
||||
## 6. Safety Invariants
|
||||
|
||||
These are statically checked by `~/.openclaw/workspace/scripts/test/check-invariants.sh` and enforced in all changes:
|
||||
|
||||
| ID | Invariant |
|
||||
|----|-----------|
|
||||
| S1 | Zero merge API calls in dispatch script (`/merge` does not appear) |
|
||||
| S2 | REQUEST_CHANGES check (Rule 2) appears before CI check (Rule 4) |
|
||||
| S3 | REQUEST_CHANGES check (Rule 2) appears before ready label application (Rule 10) |
|
||||
| S4 | No model/AI API references in dispatch script |
|
||||
| S5 | `set -euo pipefail` present |
|
||||
| S6 | Active WIP does not cause early exit (only sets ACTIVE_WIP flag) |
|
||||
| S7 | SPAWN:impl guarded by `ACTIVE_WIP == 0` check |
|
||||
| S8 | No merge calls in any worker template |
|
||||
|
||||
---
|
||||
|
||||
## 7. Error Handling
|
||||
|
||||
| Error | Behavior |
|
||||
|-------|----------|
|
||||
| `curl` returns error | `set -euo pipefail` aborts script — no partial actions |
|
||||
| `jq` parse error | Script aborts |
|
||||
| Worker crashes | WIP label left on PR; stale WIP cleanup (Rule 0) removes it after 1hr |
|
||||
| Race: two crons fire | WIP mutex prevents double-dispatch for same PR |
|
||||
| `sessions_spawn` fails | Worker not spawned; WIP label orphaned → cleaned in 1hr |
|
||||
| Config file missing | Exit code 2 with error message |
|
||||
|
||||
---
|
||||
|
||||
## 8. Worker Templates
|
||||
|
||||
Each worker receives a precise task description with substituted values:
|
||||
|
||||
| Template | Trigger | Key job |
|
||||
|----------|---------|---------|
|
||||
| `self-review.md` | No clean self-review | Post self-review comment, remove WIP |
|
||||
| `sr-fix.md` | Self-review needs attention | Address self-review findings, push, remove WIP |
|
||||
| `ci-fix.md` | CI failing | Diagnose, fix, push, remove WIP |
|
||||
| `address-feedback.md` | Unacknowledged findings or inline comments | Address feedback, push, remove WIP |
|
||||
| `findings.md` | REQUEST_CHANGES present | Address REQUEST_CHANGES, push, remove WIP |
|
||||
| `rebase.md` | Merge conflicts | Rebase on main, push, remove WIP |
|
||||
| `impl.md` | New issue | Implement feature/fix, open PR |
|
||||
|
||||
Workers **always** remove the WIP label on completion and reply `NO_REPLY`.
|
||||
|
||||
---
|
||||
|
||||
## 9. Fixes for Issues #144 and #145
|
||||
|
||||
**Issue #144** (autonomous merge):
|
||||
The dispatch script contains no merge API calls anywhere. The `~/.openclaw/workspace/scripts/test/check-invariants.sh`
|
||||
invariant `S1` verifies this. Workers do not receive merge instructions.
|
||||
|
||||
**Issue #145** (merged despite REQUEST_CHANGES):
|
||||
Rule 2 is the **first** rule evaluated per PR. It cannot be skipped, reasoned past,
|
||||
or bypassed. It is checked before CI, before self-review, before handoff. The check
|
||||
uses latest-per-reviewer state, so a reviewer who re-approved after REQUEST_CHANGES
|
||||
is correctly handled.
|
||||
+6
-22
@@ -66,18 +66,6 @@ func ParseDocMapConfig(localPath string) (*DocMapConfig, error) {
|
||||
return &cfg, nil
|
||||
}
|
||||
|
||||
// FileCoveredByDocMap reports whether at least one paths: glob in any mapping
|
||||
// of cfg matches the given file path. It is used by static validation tooling
|
||||
// (e.g. the validate-docmap subcommand) to check per-file docmap coverage.
|
||||
func FileCoveredByDocMap(cfg *DocMapConfig, file string) bool {
|
||||
for _, mapping := range cfg.Mappings {
|
||||
if mappingMatches(mapping.Paths, []string{file}) {
|
||||
return true
|
||||
}
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
// MatchDocs returns deduplicated doc paths for the given changed file paths.
|
||||
// A mapping matches if any of its path globs matches any of the changed files.
|
||||
func MatchDocs(cfg *DocMapConfig, changedFiles []string) []string {
|
||||
@@ -257,7 +245,7 @@ type docEntry struct {
|
||||
// If the path is a directory, all .md files under it are returned.
|
||||
// If it's a file, a single entry is returned.
|
||||
func loadDocEntries(ctx context.Context, fetcher DocFetcher, owner, repo, docPath string) ([]docEntry, error) {
|
||||
if err := ValidateDocPath(docPath); err != nil {
|
||||
if err := validateDocPath(docPath); err != nil {
|
||||
return nil, fmt.Errorf("doc path %q rejected: %w", docPath, err)
|
||||
}
|
||||
|
||||
@@ -310,15 +298,11 @@ func readFileBytes(path string) ([]byte, error) {
|
||||
return os.ReadFile(path)
|
||||
}
|
||||
|
||||
// ValidateDocPath rejects doc paths that could cause path traversal
|
||||
// (absolute paths, any ".." segment, backslashes). Defense-in-depth: callers
|
||||
// must also confine the joined path to the repo root via filepath.Rel before
|
||||
// any filesystem access. Backslashes are rejected explicitly to prevent
|
||||
// Windows platform edge cases.
|
||||
func ValidateDocPath(p string) error {
|
||||
if strings.Contains(p, "\\") {
|
||||
return fmt.Errorf("backslashes not allowed in doc paths")
|
||||
}
|
||||
// validateDocPath rejects doc paths that could cause path traversal via the
|
||||
// VCS API (absolute paths, any ".." segment). Defense-in-depth: the VCS API
|
||||
// should already scope paths to the repo, but we validate locally to avoid
|
||||
// any quirk in backend path handling.
|
||||
func validateDocPath(p string) error {
|
||||
if filepath.IsAbs(p) {
|
||||
return fmt.Errorf("absolute paths not allowed")
|
||||
}
|
||||
|
||||
+2
-76
@@ -384,7 +384,7 @@ func TestValidateDocPath(t *testing.T) {
|
||||
"a/b/c",
|
||||
}
|
||||
for _, p := range valid {
|
||||
if err := ValidateDocPath(p); err != nil {
|
||||
if err := validateDocPath(p); err != nil {
|
||||
t.Errorf("expected valid path %q to pass, got error: %v", p, err)
|
||||
}
|
||||
}
|
||||
@@ -395,13 +395,9 @@ func TestValidateDocPath(t *testing.T) {
|
||||
"docs/../../../etc/passwd",
|
||||
"../sibling-repo/file.md",
|
||||
"a/b/../c",
|
||||
// Backslashes must be rejected (Finding #3 — Windows platform edge cases).
|
||||
`docs\foo.md`,
|
||||
`docs\..\secret`,
|
||||
`\absolute`,
|
||||
}
|
||||
for _, p := range invalid {
|
||||
if err := ValidateDocPath(p); err == nil {
|
||||
if err := validateDocPath(p); err == nil {
|
||||
t.Errorf("expected path %q to be rejected, but it was accepted", p)
|
||||
}
|
||||
}
|
||||
@@ -424,27 +420,6 @@ func TestLoadMatchingDocs_PathTraversalRejected(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestValidateDocPath_Backslash verifies that backslash-bearing paths are
|
||||
// rejected to prevent Windows platform edge cases where a path separator
|
||||
// could be normalised differently by the host OS or VCS backend.
|
||||
func TestValidateDocPath_Backslash(t *testing.T) {
|
||||
backslashPaths := []string{
|
||||
`docs\foo.md`,
|
||||
`docs\subdir\file.md`,
|
||||
`\absolute`,
|
||||
}
|
||||
for _, p := range backslashPaths {
|
||||
if err := ValidateDocPath(p); err == nil {
|
||||
t.Errorf("expected backslash path %q to be rejected, but it was accepted", p)
|
||||
}
|
||||
}
|
||||
|
||||
// Sanity: forward-slash path must still be accepted.
|
||||
if err := ValidateDocPath("docs/foo.md"); err != nil {
|
||||
t.Errorf("expected forward-slash path to be accepted, got: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
// ============================================================
|
||||
// Helpers
|
||||
// ============================================================
|
||||
@@ -461,52 +436,3 @@ func writeTempYAML(t *testing.T, content string) string {
|
||||
}
|
||||
return filepath.Clean(f.Name())
|
||||
}
|
||||
|
||||
// ============================================================
|
||||
// FileCoveredByDocMap
|
||||
// ============================================================
|
||||
|
||||
func TestFileCoveredByDocMap(t *testing.T) {
|
||||
cfg := &DocMapConfig{
|
||||
Mappings: []DocMapping{
|
||||
{
|
||||
Paths: []string{"lib/foo/**", "lib/bar/*.go"},
|
||||
Docs: []string{"docs/foo.md"},
|
||||
},
|
||||
{
|
||||
Paths: []string{"cmd/**"},
|
||||
Docs: []string{"docs/cmd.md"},
|
||||
},
|
||||
},
|
||||
}
|
||||
|
||||
cases := []struct {
|
||||
file string
|
||||
covered bool
|
||||
}{
|
||||
{"lib/foo/baz.ex", true},
|
||||
{"lib/foo/sub/deep.ex", true},
|
||||
{"lib/bar/util.go", true},
|
||||
{"lib/bar/sub/util.go", false}, // *.go only matches one level
|
||||
{"cmd/main.go", true},
|
||||
{"cmd/sub/main.go", true},
|
||||
{"internal/secret.go", false},
|
||||
{"", false},
|
||||
}
|
||||
|
||||
for _, tc := range cases {
|
||||
t.Run(tc.file, func(t *testing.T) {
|
||||
got := FileCoveredByDocMap(cfg, tc.file)
|
||||
if got != tc.covered {
|
||||
t.Errorf("FileCoveredByDocMap(%q) = %v, want %v", tc.file, got, tc.covered)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestFileCoveredByDocMap_EmptyConfig(t *testing.T) {
|
||||
cfg := &DocMapConfig{}
|
||||
if FileCoveredByDocMap(cfg, "lib/foo/bar.go") {
|
||||
t.Error("expected false for empty config, got true")
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user