[NIT] readLines uses bufio.Scanner with the default 64KB token buffer. Very long file paths (unlikely but possible with generated paths) would silently be truncated by scanner.Scan() returning false without error when a line exceeds the buffer. For a CI tool reading git diff output this is practically fine, but a comment noting the 64KB line limit or using scanner.Buffer to increase it would be defensive.
[NIT] TestRunValidateDocmap_Clean has a long comment block explaining why it relies on process-level stdin instead of the stdinValidateDocmap helper, which is defined later in the same file. The test would be cleaner and more consistent if it simply used stdinValidateDocmap with an empty string — which is exactly what TestRunValidateDocmap_EmptyStdin does. These two tests appear to be testing the same case (empty stdin → clean). Consider removing TestRunValidateDocmap_Clean or consolidating it with TestRunValidateDocmap_EmptyStdin.
[MINOR] TestRunValidateURL_Success calls validateURL directly as a pre-check, which means the actual DNS resolution happens twice (once in the pre-check, once in runValidateURL). This is not a bug, but it's slightly inconsistent with the pattern in TestValidateURL_BlockedPrivateIP which calls validateURL once and inspects the result. More critically, if validateURL passes the pre-check but the network state changes between the two calls (unlikely in practice), the test could produce a confusing failure. Using t.Skip based on a simple net.LookupHost check might be slightly cleaner, but the current approach is not wrong.
[MINOR] TestMainSubprocess_DeprecatedGiteaURLEnv ignores the error return from cmd.CombinedOutput() with out, _ := cmd.CombinedOutput(). All other subprocess tests in this file capture out, err := cmd.CombinedOutput() and check err (or intentionally assert on it). The deprecation test explicitly doesn't need a non-zero exit assertion, but discarding the error silently is inconsistent with the rest of the file. Consider out, _ := cmd.CombinedOutput() with a comment, or just accept and ignore the error explicitly: //nolint or a comment explaining why it's intentionally ignored.
[NIT] The fakeDocFetcher struct has inconsistent alignment on the field comments — dirs has a trailing space before the comment to align with files. This would normally be fixed by gofmt automatically, but gofmt aligns struct field comments differently from map literal comments. Not a real issue, just a minor style note.
[MINOR] The LoadMatchingDocs function has a subtle double-write bug for the truncation notice. When available <= 0 is detected at the start of the inner loop, it writes a truncation notice and breaks. But when len(content) > available causes a mid-content truncation, it writes a different truncation notice inline. Then, on the next outer-loop iteration, limitReached is true and we hit the if limitReached { break } before writing another notice. This works correctly, but there are two different truncation messages (one at the outer level `
[NIT] The package doc comment is at the file level but not in the // Package review ... form. Per the documentation pattern, the package-level comment (if this is the first file users will encounter for new functionality) should be // Package review .... However, since this is a new file in an existing package and the primary package doc presumably exists elsewhere, a file-level comment is acceptable. The comment could be more explicit: // Package review provides ... or simply be a file comment. Minor style nit.
[MINOR] The loadDocEntries function silently falls through from a successful-but-empty GetAllFilesInPath call to a GetFileContent call. If a user specifies a directory path like docs/domain/ (with trailing slash) and the directory exists but has no files, the function will then try to fetch docs/domain/ as a single file, which will likely fail with an obscure error. The debug log message only fires when dirErr != nil, not when the directory exists but is empty. This means the caller's debug log "doc-map: no .md files found under path" in LoadMatchingDocs will never fire for this case — instead it falls to the file-fetch error path. Consider: if GetAllFilesInPath returns nil error with an empty map, and the path ends with /, skip the single-file fallback and return empty entries directly.
[MINOR] The ParseDocMapConfig strict-mode fallback parses twice on any strict-mode failure, not just unknown-key failures. If the YAML is structurally valid but has a type mismatch (e.g., paths: "not-a-list"), the strict parse fails, the relaxed parse also fails (returning an error), and the function returns the strict-mode error wrapping localPath. But for unknown keys, it succeeds on the relaxed parse and logs a warning. The comment says "Re-parse without strict mode to log which keys are unknown" — but the code doesn't actually log which keys are unknown; it just logs the original strict error. The behavior is correct (lenient for unknown keys, strict for structural issues) but the comment is misleading about what gets logged.
[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] Committing a PLAN file to the repository is unusual and will persist in the repo history as a planning artifact. Consider whether this file should be in the PR description or a wiki instead. That said, some projects intentionally keep ADRs/plans in the repo, so this is a process note rather than a code issue.
[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).
[MINOR] LoadMatchingDocs writes a truncation notice ("⚠️ Design document context truncated — size limit reached.") to the string builder when limitReached is set inside the outer loop over entries, but this notice can be written multiple times if the outer loop continues processing additional docPaths after limitReached is already set. The outer loop does break on limitReached, so the notice in the inner loop (line 229) is the concern. Looking more carefully: the inner loop only writes the notice once then breaks, but if the same doc triggers truncation, the outer notice at line 229 fires then the outer loop also has a break due to limitReached. The flow is actually correct — the inner notice fires at most once (truncated flag set, limitReached set, inner break), and the outer loop breaks at the top. No double-write occurs. However, this is subtle enough to warrant a clarifying comment.
[MINOR] ParseDocMapConfig uses yaml.Strict() first, then re-parses with yaml.Unmarshal on failure to get the 'unknown keys' warning. This double-parse is slightly wasteful but more importantly, it means the error message logged is the strict-mode error (which describes the unknown key) rather than a custom message. The current approach works correctly but could be simplified: just always use non-strict mode and rely on the YAML library's warning mechanisms if available, or accept that users won't get per-key detail. The current approach is acceptable given the go-yaml library's API.
[NIT] The fakeDocFetcher struct fields lack alignment/formatting: dirs map[string]map[string]string // dir path -> (file path -> content) — the comment is useful but gofmt may reformat the spacing. Not a correctness issue.
[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.