From 7adb296523a1df0cd83dfc913021a782472c394e Mon Sep 17 00:00:00 2001 From: Rodin Date: Fri, 15 May 2026 08:15:14 -0700 Subject: [PATCH 1/3] fix(#141): reject non-regular files in validateDocmapPath Add IsRegular() check after Lstat so directories, FIFOs, and device nodes produce a clear error ("docmap must be a regular file") instead of a confusing downstream parse error. Test: TestValidateDocmapPath_NonRegularFile Addresses MINOR finding in review #4175. --- cmd/review-bot/validatedocmap.go | 7 +++++++ cmd/review-bot/validatedocmap_test.go | 25 +++++++++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/cmd/review-bot/validatedocmap.go b/cmd/review-bot/validatedocmap.go index 3db4c76..988d0ea 100644 --- a/cmd/review-bot/validatedocmap.go +++ b/cmd/review-bot/validatedocmap.go @@ -61,6 +61,13 @@ func validateDocmapPath(localPath, resolvedRoot string) error { return fmt.Errorf("symlinks are not allowed") } + // Reject anything that is not a regular file (directories, FIFOs, device + // nodes, etc.) — ParseDocMapConfig expects a plain YAML file and would + // produce a confusing error on non-regular entries. + if !fi.Mode().IsRegular() { + return fmt.Errorf("docmap must be a regular file") + } + // 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) diff --git a/cmd/review-bot/validatedocmap_test.go b/cmd/review-bot/validatedocmap_test.go index e572ecc..2a8d241 100644 --- a/cmd/review-bot/validatedocmap_test.go +++ b/cmd/review-bot/validatedocmap_test.go @@ -599,3 +599,28 @@ func TestValidateDocmapPath_DirSymlinkBypass(t *testing.T) { t.Error("expected rejection of dir-symlink bypass, got nil error") } } + +// TestValidateDocmapPath_NonRegularFile verifies that --docmap pointing at a +// non-regular file (e.g. a directory) is rejected with a clear error before +// ParseDocMapConfig is called. +func TestValidateDocmapPath_NonRegularFile(t *testing.T) { + dir := t.TempDir() + + // Use the directory itself as the docmap path — directories pass Lstat but + // are not regular files. + reviewBotDir := filepath.Join(dir, ".review-bot") + if err := os.MkdirAll(reviewBotDir, 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + + code, _, stderr := stdinValidateDocmap(t, + "", + []string{"--docmap", reviewBotDir, "--repo-root", dir}, + ) + if code != 2 { + t.Errorf("expected exit 2 for directory docmap, got %d; stderr: %q", code, stderr) + } + if !strings.Contains(stderr, "regular file") && !strings.Contains(stderr, "invalid") { + t.Errorf("expected regular-file rejection in stderr, got %q", stderr) + } +} From 92efd1af2b4f2049e915c8116b0160cb19303570 Mon Sep 17 00:00:00 2001 From: Rodin Date: Fri, 15 May 2026 08:15:33 -0700 Subject: [PATCH 2/3] fix(#141): strip leading './' from coverage-check paths Non-git tools (e.g. `find`, `ls`) can emit paths with a "./" prefix. Without stripping this, "./cmd/foo.go" would not match the glob "cmd/**", producing a false-positive uncovered-file failure. Fix: add strings.TrimPrefix(f, "./") after backslash normalization. Test: TestRunValidateDocmap_DotSlashPrefix Addresses MINOR finding in review #4175. --- cmd/review-bot/validatedocmap.go | 3 +++ cmd/review-bot/validatedocmap_test.go | 25 +++++++++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/cmd/review-bot/validatedocmap.go b/cmd/review-bot/validatedocmap.go index 988d0ea..cffa822 100644 --- a/cmd/review-bot/validatedocmap.go +++ b/cmd/review-bot/validatedocmap.go @@ -178,6 +178,9 @@ func runValidateDocmap(args []string) int { // Normalize Windows-style backslashes to forward slashes so that // changed-file paths from git on Windows match doc-map globs. f = strings.ReplaceAll(f, "\\", "/") + // Strip a leading "./" emitted by non-git tools (e.g. `find`) so that + // paths like "./cmd/foo.go" match doc-map globs written as "cmd/**". + f = strings.TrimPrefix(f, "./") if !review.FileCoveredByDocMap(cfg, f) { uncovered = append(uncovered, f) } diff --git a/cmd/review-bot/validatedocmap_test.go b/cmd/review-bot/validatedocmap_test.go index 2a8d241..f30a08b 100644 --- a/cmd/review-bot/validatedocmap_test.go +++ b/cmd/review-bot/validatedocmap_test.go @@ -624,3 +624,28 @@ func TestValidateDocmapPath_NonRegularFile(t *testing.T) { t.Errorf("expected regular-file rejection in stderr, got %q", stderr) } } + +// TestRunValidateDocmap_DotSlashPrefix verifies that paths emitted with a +// leading "./" (e.g. from `find` or `ls`) match doc-map globs correctly. +// Without TrimPrefix, "./cmd/foo.go" would not match the pattern "cmd/**". +func TestRunValidateDocmap_DotSlashPrefix(t *testing.T) { + dir := t.TempDir() + makeDocFile(t, dir, "docs/foo.md") + + docmap := makeDocmapInDir(t, dir, ` +mappings: + - paths: + - "cmd/**" + docs: + - docs/foo.md +`) + + // File with a leading "./" should be treated as covered. + code, _, stderr := stdinValidateDocmap(t, + "./cmd/foo.go\n", + []string{"--docmap", docmap, "--repo-root", dir}, + ) + if code != 0 { + t.Errorf("expected exit 0 for './' prefixed covered file, got %d; stderr: %q", code, stderr) + } +} From bacb25e029cd8e3601e21fe108ed868d29fef5f6 Mon Sep 17 00:00:00 2001 From: Rodin Date: Fri, 15 May 2026 08:15:45 -0700 Subject: [PATCH 3/3] nit(#141): fix stale-docs error message phrasing "ERROR: stale docmap docs: entries" had a vestigial "docs:" fragment that reads awkwardly (looks like a YAML reference). Change to: "ERROR: stale docmap entries (paths do not exist):" Addresses NIT finding in review #4175. --- cmd/review-bot/validatedocmap.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/review-bot/validatedocmap.go b/cmd/review-bot/validatedocmap.go index cffa822..e29fa25 100644 --- a/cmd/review-bot/validatedocmap.go +++ b/cmd/review-bot/validatedocmap.go @@ -199,7 +199,7 @@ func runValidateDocmap(args []string) int { staleDocs := checkStaleDocs(cfg, resolvedRoot) if len(staleDocs) > 0 { failed = true - fmt.Fprintln(errWriter, "ERROR: stale docmap docs: entries (paths do not exist):") + fmt.Fprintln(errWriter, "ERROR: stale docmap entries (paths do not exist):") for _, d := range staleDocs { fmt.Fprintf(errWriter, " %s\n", d) }