[MINOR] The package comment says 'Package review provides doc-map parsing and doc injection...' but the review package already exists and has a broader purpose. This package-level comment in docmap.go will either be ignored (not the first file) or will conflict with the package doc in other files. The file-level comment should just be a regular comment, not a package doc comment, or should be omitted since the package is already documented elsewhere.
[NIT] The fakeDocFetcher struct fields have inconsistent comment styles (inline comments on the struct fields are fine), but the dirs field type map[string]map[string]string could benefit from a brief comment explaining the structure, similar to the files field comment. Minor readability nit.
[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.
[NIT] Committing a PLAN-137.md file to the repository is unusual — planning/design documents are typically kept in issue trackers or wikis. This couples the implementation repository to an issue-tracking artifact. Consider removing it before merge or moving it to a docs/ or .design/ directory if architecture decision records are desired.
[MINOR] The truncateUTF8 function is duplicated — an identical implementation already exists in budget/budget.go. Per the DRY principle and the project's own patterns, this should be extracted to a shared utility. Since review and budget are separate packages, the options are: (1) move it to a shared internal package, or (2) accept the duplication with a comment. As-is, future changes to the truncation logic must be made in two places.
[NIT] The test TestLoadMatchingDocs_ContextSizeGuard uses MaxBytes: 350 with content of 200 bytes each and checks len(content) > 600. The content after metadata (headings, newlines) will be well above 350 bytes due to formatting overhead. The test is validating the right behavior (truncation occurs), but the boundary check (> 600) is loose. Not a bug, just slightly confusing as the actual formatted content with headings for doc a alone is ~220+ bytes.
[NIT] The comment says 'It verifies that the GitHub adapter is selected via VCS_TYPE=github' but the test never sets VCS_TYPE or exercises routing through the main entrypoint — it directly instantiates github.NewClient(). This makes the comment slightly misleading; it tests the GitHub adapter in isolation, not the VCS routing path. Consider updating the comment to accurately reflect what is being tested.
[NIT] The GitHub API URL is hardcoded as "https://api.github.com" rather than using the githubAPIURL() helper or an environment variable. This means the test cannot target a GHES instance via INTEGRATION_GITHUB_TOKEN. Minor inconsistency with the stated goal of testing the full GitHub adapter path, but acceptable for the current scope.
[NIT] The ListReviews loop breaks early on the first matching review but the found variable is set correctly — this is fine. Minor observation: the Gitea integration test (TestIntegration_PostAndCleanup) uses a similar sentinel pattern; the two tests are consistent, which is good.
[NIT] The comment says 'Verify the GitHub adapter is selected via VCS routing' and sets VCS_TYPE, but the test then constructs the GitHub client directly via github.NewClient(...) rather than going through the VCS routing dispatch. The t.Setenv call has no observable effect on the test — the routing isn't actually exercised here. The comment overstates what's being tested. Either the comment should be updated to reflect that this is a direct GitHub adapter test, or the test should be restructured to go through the main VCS dispatch.
[MINOR] In GetAllFilesInPath, when ListContents returns a 404, the code falls back to GetFileContent. But ListContents on a file path returns the file as a single-object response (normalized to a []ContentEntry slice) — it does NOT return 404 for files. The 404 fallback path is dead code for GitHub and misleads readers. The comment // 404 means path may be a file — try fetching directly is accurate for Gitea but not for this GitHub client where ListContents already handles the single-file case. This won't cause a bug, but it's confusing and the fallback is untested.
[NIT] In TestGetFileContent_Base64WithNewlines, the encoded variable is declared and assigned but only used via _ = encoded (a suppress-unused comment). The actual test hardcodes the string in the body literal. This is confusing — either use the variable to build the response body, or remove the variable entirely.
[NIT] vcsPullRequest uses an anonymous struct for Head rather than a named type. This is minor but means the struct literal syntax r.Head.Sha = ... across both adapters is slightly awkward. A named struct would be marginally cleaner, though the current approach matches what the gitea/github packages themselves do.
[MINOR] The URL heuristic strings.Contains(*vcsURL, "github.concur.com") is a hardcoded company-specific domain baked into an open-source tool. This is fragile — any GitHub Enterprise host other than github.concur.com won't be detected. The more robust approach would be to rely on the explicit VCS_TYPE env var for all GHES instances, or detect GHES by checking for /api/v3 in the URL, or check the GitHub API endpoint itself. The comment acknowledges the composite action sets VCS_TYPE explicitly, but this hardcoded fallback will silently mis-route for other GHES deployments.
[MINOR] ErrNotSupported is declared but never used — the comment on giteaExtClient says "GitHub implementations return ErrNotSupported for those", but githubVCSAdapter never returns it (those methods simply don't exist on the adapter). Either remove the variable or use it if the intent is to provide stub implementations. As-is, a linter would flag this as an unused export (though go vet won't).
[NIT] The translateGitHubReviewState comment mentions COMMENTED as a state that translates to COMMENT, but the GitHub API actually uses COMMENTED as the state on the stored review object when the event was COMMENT. The translation is correct, but the doc comment on PostReview says 'event strings (APPROVE, REQUEST_CHANGES, COMMENT)' — COMMENT here is the event while COMMENTED is the state, which are different fields. The code handles this correctly but the terminology in the comments could be clearer.