rodin/review-bot
2
1
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 6 Wiki Activity

feat(#137): add doc-map input for path-scoped doc injection #138

Merged
rodin merged 7 commits from issue-137 into main 2026-05-15 03:39:37 +00:00
Conversation 10 Commits 7 Files Changed 9
+34 -5
1 changed files with 34 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit a24edeee89 - Show all commits
review/docmap.go
+34 -5
View File
@@ -1,5 +1,4 @@
// Package review provides doc-map parsing and doc injection for path-scoped // doc-map parsing and doc injection for path-scoped design document context in AI code reviews.
sonnet-review-bot commented 2026-05-15 03:33:32 +00:00
Review
Copy Link
Copy Source

[NIT] The package doc comment is a single-line imperative description rather than the conventional // Package review ... format documented in the Go patterns (documentation.md pattern #3). The comment reads // doc-map parsing and doc injection for path-scoped design document context in AI code reviews. — but this is a file-level comment, not a package comment. The review package presumably has a package comment elsewhere; this file comment is fine as a file-level description, though it's atypical (most Go files don't have pre-package file comments).

**[NIT]** The package doc comment is a single-line imperative description rather than the conventional `// Package review ...` format documented in the Go patterns (documentation.md pattern #3). The comment reads `// doc-map parsing and doc injection for path-scoped design document context in AI code reviews.` — but this is a file-level comment, not a package comment. The review package presumably has a package comment elsewhere; this file comment is fine as a file-level description, though it's atypical (most Go files don't have pre-package file comments).
// design document context in AI code reviews.
package review package review
import ( import (
5
 @@ -106,7 +105,7 @@ func mappingMatches(patterns, files []string) bool {
// globMatch matches a path against a glob pattern that may contain **. // globMatch matches a path against a glob pattern that may contain **.
// It supports: // It supports:
// - Standard path.Match patterns (*, ?, [range]) // - filepath.Match patterns (*, ?, [range])
// - ** as a path segment that matches zero or more segments // - ** as a path segment that matches zero or more segments
// - Trailing /** to match a directory and all its contents // - Trailing /** to match a directory and all its contents
// //
9
 @@ -246,9 +245,13 @@ type docEntry struct {
// If the path is a directory, all .md files under it are returned. // If the path is a directory, all .md files under it are returned.
sonnet-review-bot commented 2026-05-15 03:27:16 +00:00
Review
Copy Link
Copy Source

[MINOR] In loadDocEntries, when GetAllFilesInPath returns an error (rather than empty results), the code silently falls through to try GetFileContent. The plan's decision explicitly states 'If GetAllFilesInPath returns an error, try GetFileContent', which this does — but the directory error is swallowed without logging. If both calls fail, only the file error is returned. Consider logging a debug message for the directory error before the fallback, which would help diagnose unexpected behavior.

**[MINOR]** In `loadDocEntries`, when `GetAllFilesInPath` returns an error (rather than empty results), the code silently falls through to try `GetFileContent`. The plan's decision explicitly states 'If GetAllFilesInPath returns an error, try GetFileContent', which this does — but the directory error is swallowed without logging. If both calls fail, only the file error is returned. Consider logging a debug message for the directory error before the fallback, which would help diagnose unexpected behavior.
// If it's a file, a single entry is returned. // If it's a file, a single entry is returned.
func loadDocEntries(ctx context.Context, fetcher DocFetcher, owner, repo, docPath string) ([]docEntry, error) { func loadDocEntries(ctx context.Context, fetcher DocFetcher, owner, repo, docPath string) ([]docEntry, error) {
if err := validateDocPath(docPath); err != nil {
return nil, fmt.Errorf("doc path %q rejected: %w", docPath, err)
}
// Try directory expansion first. // Try directory expansion first.
files, err := fetcher.GetAllFilesInPath(ctx, owner, repo, docPath) files, dirErr := fetcher.GetAllFilesInPath(ctx, owner, repo, docPath)
sonnet-review-bot commented 2026-05-15 03:38:27 +00:00
Review
Copy Link
Copy Source

[NIT] sortDocEntries uses an insertion sort described as "doc lists are small". This is fine, but Go's sort.Slice or slices.SortFunc (Go 1.21+) would be more idiomatic and readable with no meaningful performance difference at these sizes. The comment justifying insertion sort is unnecessary ceremony for a standard sort.

**[NIT]** `sortDocEntries` uses an insertion sort described as "doc lists are small". This is fine, but Go's `sort.Slice` or `slices.SortFunc` (Go 1.21+) would be more idiomatic and readable with no meaningful performance difference at these sizes. The comment justifying insertion sort is unnecessary ceremony for a standard sort.
if err == nil && len(files) > 0 { if dirErr == nil && len(files) > 0 {
// Filter for .md files only. // Filter for .md files only.
var entries []docEntry var entries []docEntry
for path, content := range files { for path, content := range files {
@@ -261,6 +264,11 @@ func loadDocEntries(ctx context.Context, fetcher DocFetcher, owner, repo, docPat
return entries, nil return entries, nil
} }
// Directory expansion returned nothing; log and fall through to single-file fetch.
if dirErr != nil {
slog.Debug("doc-map: directory expansion failed, trying as single file", "path", docPath, "error", dirErr)
}
// Try as a single file. // Try as a single file.
content, fileErr := fetcher.GetFileContent(ctx, owner, repo, docPath) content, fileErr := fetcher.GetFileContent(ctx, owner, repo, docPath)
if fileErr != nil { if fileErr != nil {
1
 @@ -290,8 +298,29 @@ func readFileBytes(path string) ([]byte, error) {
return os.ReadFile(path) return os.ReadFile(path)
} }
// validateDocPath rejects doc paths that could cause path traversal via the
// VCS API (absolute paths, any ".." segment). Defense-in-depth: the VCS API
sonnet-review-bot commented 2026-05-15 03:33:32 +00:00
Review
Copy Link
Copy Source

[MINOR] sortDocEntries uses an insertion sort implemented manually. The standard library's sort.Slice would be idiomatic and more readable: sort.Slice(entries, func(i, j int) bool { return entries[i].path < entries[j].path }). The comment says "doc lists are small" which justifies the O(n²) complexity, but the standard library sort is both clearer and handles all sizes correctly. This is a NIT-level style issue per project conventions.

**[MINOR]** sortDocEntries uses an insertion sort implemented manually. The standard library's sort.Slice would be idiomatic and more readable: `sort.Slice(entries, func(i, j int) bool { return entries[i].path < entries[j].path })`. The comment says "doc lists are small" which justifies the O(n²) complexity, but the standard library sort is both clearer and handles all sizes correctly. This is a NIT-level style issue per project conventions.
// 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")
}
for _, segment := range strings.Split(p, "/") {
if segment == ".." {
return fmt.Errorf("path traversal ('..' segment) not allowed")
}
}
return nil
}
// truncateUTF8 truncates s to at most maxBytes without splitting multi-byte // truncateUTF8 truncates s to at most maxBytes without splitting multi-byte
// UTF-8 characters. Returns a valid UTF-8 string of at most maxBytes bytes. // UTF-8 characters. Returns a valid UTF-8 string of at most maxBytes bytes.
//
// Note: an identical implementation exists in budget/budget.go. The two
// packages are intentionally separate (review does not import budget), so
// the duplication is accepted rather than introducing a shared internal
// package for a single small function.
func truncateUTF8(s string, maxBytes int) string { func truncateUTF8(s string, maxBytes int) string {
if len(s) <= maxBytes { if len(s) <= maxBytes {
return s return s