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

feat(#143): fetch doc-map config from trusted VCS ref #153

Merged
rodin merged 2 commits from issue-143 into main 2026-05-15 12:09:20 +00:00
Conversation 19 Commits 2 Files Changed 7
+50 -3
3 changed files with 50 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 02dfc12141 - Show all commits
cmd/review-bot/main.go
+5 -2
View File
2
 @@ -174,9 +174,12 @@ func main() {
os.Exit(1)
sonnet-review-bot commented 2026-05-15 11:29:53 +00:00
Review
Copy Link
Copy Source

[NIT] The resolvedDocMapFile variable is declared at a wide scope and left as the zero value (empty string) when *docMapTrustedRef != "". It is then used later in the else branch of the trusted-ref check. This is correct — the empty string is only used in the else branch — but it is slightly fragile. A future refactor could accidentally use resolvedDocMapFile in the trusted-ref path. A comment noting that resolvedDocMapFile is intentionally empty when docMapTrustedRef is set would improve clarity, or the variable could be moved into the else block. No bug here.

**[NIT]** The `resolvedDocMapFile` variable is declared at a wide scope and left as the zero value (empty string) when `*docMapTrustedRef != ""`. It is then used later in the `else` branch of the trusted-ref check. This is correct — the empty string is only used in the else branch — but it is slightly fragile. A future refactor could accidentally use `resolvedDocMapFile` in the trusted-ref path. A comment noting that `resolvedDocMapFile` is intentionally empty when `docMapTrustedRef` is set would improve clarity, or the variable could be moved into the `else` block. No bug here.
}
// Early validation of filesystem-path flags (fail fast before network I/O)
// Early validation of filesystem-path flags (fail fast before network I/O).
sonnet-review-bot commented 2026-05-15 08:40:13 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The early validation of --doc-map path now happens before client initialization (good for fail-fast), but when --doc-map-trusted-ref is set, the resolvedDocMapFile variable is never used (the trusted-ref path fetches from VCS, ignoring the local file). This is correct behavior but the variable name resolvedDocMapFile implies a local file will be used. A comment clarifying that this is only used in the local-workspace fallback path would improve readability.

**[NIT]** The early validation of `--doc-map` path now happens before client initialization (good for fail-fast), but when `--doc-map-trusted-ref` is set, the `resolvedDocMapFile` variable is never used (the trusted-ref path fetches from VCS, ignoring the local file). This is correct behavior but the variable name `resolvedDocMapFile` implies a local file will be used. A comment clarifying that this is only used in the local-workspace fallback path would improve readability.
// Skip local-path validation when --doc-map-trusted-ref is set: the flag
// value is used as a VCS API path, not a local filesystem path, and the
// file may not exist in the local checkout (sparse, PR-deleted, etc.).
var resolvedDocMapFile string
if *docMapFile != "" {
if *docMapFile != "" && *docMapTrustedRef == "" {
resolved, err := validateWorkspacePath(*docMapFile, "doc-map")
if err != nil {
slog.Error("invalid doc-map path", "error", err)
cmd/review-bot/main_test.go
+44
View File
@@ -1578,3 +1578,47 @@ func TestMainSubprocess_InvalidDocMapFile(t *testing.T) {
t.Errorf("expected error about failed resolution, got: %s", output)
}
}
// TestMainSubprocess_DocMapTrustedRefSkipsLocalValidation confirms that
// --doc-map-trusted-ref bypasses local filesystem validation for --doc-map.
// When the trusted-ref flag is set, the doc-map value is used as a VCS API
// path; a nonexistent local file must not cause an early exit before network I/O.
func TestMainSubprocess_DocMapTrustedRefSkipsLocalValidation(t *testing.T) {
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
os.Args = []string{"review-bot",
"--vcs-url", "https://gitea.example.com",
"--repo", "owner/repo",
"--pr", "1",
"--reviewer-token", "tok",
"--llm-base-url", "https://api.example.com",
"--llm-api-key", "key",
"--llm-model", "gpt-4",
"--doc-map", "nonexistent-local.yml",
"--doc-map-trusted-ref", "main",
}
main()
return
}
cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_DocMapTrustedRefSkipsLocalValidation")
cmd.Env = append(cleanEnv(),
"TEST_SUBPROCESS_MAIN=1",
"GITHUB_WORKSPACE="+t.TempDir(),
)
out, err := cmd.CombinedOutput()
output := string(out)
// The test must fail (network I/O or VCS API failure) but must NOT
// fail with the local filesystem validation error.
// "failed to resolve" would indicate the early validateWorkspacePath ran —
// that would be the bug this test is catching.
if strings.Contains(output, "failed to resolve") {
t.Errorf("--doc-map-trusted-ref should skip local path validation, but got filesystem error: %s", output)
}
// It must still exit non-zero (real VCS call to example.com will fail).
if err == nil {
t.Fatal("expected non-zero exit when VCS API is unreachable, got success")
}
}
review/docmap.go
+1 -1
View File
@@ -57,7 +57,7 @@ func ParseDocMapConfig(localPath string) (*DocMapConfig, error) {
// ParseDocMapConfigContent parses a doc-map YAML config from an in-memory
// string. The source parameter is used only for error messages and log entries
// (e.g. "main:main@<ref>").
// (e.g. "owner/repo@main:.review-bot/doc-map.yml").
gpt-review-bot commented 2026-05-15 08:36:10 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The comment example for the source parameter mentions a format like "main:main@", but the code constructs owner/repo@ref:path. Consider updating the example to match the actual format to avoid confusion.

**[NIT]** The comment example for the `source` parameter mentions a format like "main:main@<ref>", but the code constructs `owner/repo@ref:path`. Consider updating the example to match the actual format to avoid confusion.
//
// Use this when the config content has been fetched from a trusted VCS ref
// rather than read from the local workspace.
sonnet-review-bot commented 2026-05-15 08:35:33 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The doc comment for ParseDocMapConfigContent says 'e.g. "main:main@"' but the actual format used in main.go is 'owner/repo@ref:path'. Minor doc inconsistency, no functional impact.

**[NIT]** The doc comment for ParseDocMapConfigContent says 'e.g. "main:main@<ref>"' but the actual format used in main.go is 'owner/repo@ref:path'. Minor doc inconsistency, no functional impact.
4