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

test(#139): improve cmd/review-bot coverage from 44.6% to 49.3% #140

Merged
rodin merged 1 commits from issue-139 into main 2026-05-15 04:35:54 +00:00
Conversation 4 Commits 1 Files Changed 2
+180
rodin commented 2026-05-15 04:15:31 +00:00
Owner
Copy Link
Copy Source

Summary

Closes #139. Adds 7 tests covering previously uncovered paths in cmd/review-bot.

Problem

cmd/review-bot had 44.6% coverage. Three specific files had untested branches:

  • validateurl.go: nil input to isValidateError, empty-host URL, success path
  • main.go: deprecated GITEA_URL env fallback, missing --llm-base-url for openai, missing aicore creds, conflicting persona flags

Changes

validateurl_test.go (3 new tests)

  • TestIsValidateError_Nil — confirms isValidateError(nil, &ve) returns false (nil branch)
  • TestValidateURL_EmptyHost — https:// has no hostname; confirms code-2 error with "no host" message
  • TestRunValidateURL_Success — exercises the success path: valid public URL → "OK: ... is safe" on stdout, exit 0. Requires DNS; skips if unavailable.

main_test.go (4 new subprocess tests)

  • TestMainSubprocess_MissingLLMBaseURL — --llm-base-url required for provider=openai
  • TestMainSubprocess_MissingAICoreCredentials — aicore credentials required for provider=aicore
  • TestMainSubprocess_ConflictingPersonaFlags — --persona and --persona-file are mutually exclusive
  • TestMainSubprocess_DeprecatedGiteaURLEnv — GITEA_URL env var emits deprecation warning (backward compat)

Result

Package Before After
cmd/review-bot 44.6% 49.3%

Checklist

  • Tests only, no production code changes
  • All packages: go test ./... -count=1 passes
  • go vet ./... clean
  • scripts/check-deps.sh clean (no new deps)
  • Subprocess tests follow existing pattern
## Summary Closes #139. Adds 7 tests covering previously uncovered paths in `cmd/review-bot`. ## Problem `cmd/review-bot` had 44.6% coverage. Three specific files had untested branches: - `validateurl.go`: nil input to `isValidateError`, empty-host URL, success path - `main.go`: deprecated `GITEA_URL` env fallback, missing `--llm-base-url` for openai, missing aicore creds, conflicting persona flags ## Changes ### `validateurl_test.go` (3 new tests) - `TestIsValidateError_Nil` — confirms `isValidateError(nil, &ve)` returns `false` (nil branch) - `TestValidateURL_EmptyHost` — `https://` has no hostname; confirms code-2 error with "no host" message - `TestRunValidateURL_Success` — exercises the success path: valid public URL → `"OK: ... is safe"` on stdout, exit 0. Requires DNS; skips if unavailable. ### `main_test.go` (4 new subprocess tests) - `TestMainSubprocess_MissingLLMBaseURL` — `--llm-base-url` required for `provider=openai` - `TestMainSubprocess_MissingAICoreCredentials` — aicore credentials required for `provider=aicore` - `TestMainSubprocess_ConflictingPersonaFlags` — `--persona` and `--persona-file` are mutually exclusive - `TestMainSubprocess_DeprecatedGiteaURLEnv` — `GITEA_URL` env var emits deprecation warning (backward compat) ## Result | Package | Before | After | |---------|--------|-------| | `cmd/review-bot` | 44.6% | 49.3% | ## Checklist - [x] Tests only, no production code changes - [x] All packages: `go test ./... -count=1` passes - [x] `go vet ./...` clean - [x] `scripts/check-deps.sh` clean (no new deps) - [x] Subprocess tests follow existing pattern
rodin added 1 commit 2026-05-15 04:15:31 +00:00
test(#139): improve cmd/review-bot coverage from 44.6% to 49.3%
CI / test (pull_request) Successful in 17s
Details
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 27s
Details
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 34s
Details
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 50s
Details
d75e737f07 
Add tests for previously uncovered paths:

- TestIsValidateError_Nil: isValidateError(nil, ...) returns false
- TestValidateURL_EmptyHost: URL with no hostname (https://) → code-2 error
- TestRunValidateURL_Success: success path (OK output + exit 0) via example.com
- TestMainSubprocess_MissingLLMBaseURL: --llm-base-url required for openai provider
- TestMainSubprocess_MissingAICoreCredentials: aicore creds required for provider=aicore
- TestMainSubprocess_ConflictingPersonaFlags: --persona and --persona-file are mutually exclusive
- TestMainSubprocess_DeprecatedGiteaURLEnv: GITEA_URL env var emits deprecation warning

All tests pass; no production code changes; dep check clean.
rodin commented 2026-05-15 04:16:01 +00:00
Author
Owner
Copy Link
Copy Source

Self-review against d75e737f07

Assessment: ✅ Clean

Self-Review: issue-139 — 2026-05-15

Verdict: PASS

No issues found — ready for human review.

Coherence: Single purpose (improve coverage). Each test targets a specific previously-uncovered path. No scope creep — tests only, no production changes.

Fit: All tests follow existing patterns exactly (subprocess TEST_SUBPROCESS_MAIN pattern, buffer capture for writers, t.Skipf for network-dependent tests). Naming consistent with existing TestMainSubprocess_* and TestValidate* conventions.

Solution: Pre-validation in TestRunValidateURL_Success correctly mirrors TestValidateURL_BlockedPrivateIP's skip-on-DNS-fail approach. TestMainSubprocess_DeprecatedGiteaURLEnv ignores exit code intentionally — subprocess fails (no real server) but the deprecation warning must still appear in output. cleanEnv() strips GITEA_URL (HasPrefix match), so the injected value is clean.

Completeness: 7 tests covering 7 uncovered paths. Coverage: 44.6% → 49.3%. go test ./..., go vet ./..., and scripts/check-deps.sh all clean.

Self-review against d75e737f07baf82ba66aef78760ced91b4d90a8e Assessment: ✅ Clean ## Self-Review: issue-139 — 2026-05-15 ### Verdict: PASS No issues found — ready for human review. **Coherence:** Single purpose (improve coverage). Each test targets a specific previously-uncovered path. No scope creep — tests only, no production changes. **Fit:** All tests follow existing patterns exactly (subprocess `TEST_SUBPROCESS_MAIN` pattern, buffer capture for writers, `t.Skipf` for network-dependent tests). Naming consistent with existing `TestMainSubprocess_*` and `TestValidate*` conventions. **Solution:** Pre-validation in `TestRunValidateURL_Success` correctly mirrors `TestValidateURL_BlockedPrivateIP`'s skip-on-DNS-fail approach. `TestMainSubprocess_DeprecatedGiteaURLEnv` ignores exit code intentionally — subprocess fails (no real server) but the deprecation warning must still appear in output. `cleanEnv()` strips `GITEA_URL` (HasPrefix match), so the injected value is clean. **Completeness:** 7 tests covering 7 uncovered paths. Coverage: 44.6% → 49.3%. `go test ./...`, `go vet ./...`, and `scripts/check-deps.sh` all clean.
rodin added the readyself-reviewed labels 2026-05-15 04:16:04 +00:00
security-review-bot requested review from security-review-bot 2026-05-15 04:16:09 +00:00
security-review-bot approved these changes 2026-05-15 04:16:09 +00:00
security-review-bot left a comment
Collaborator
Copy Link
Copy Source

Security Review

Summary

This PR adds only test code to improve coverage, with no production changes. The tests follow safe practices (e.g., environment sanitization) and do not introduce security risks.

Recommendation

APPROVE — CI has passed and the changes are limited to tests exercising error paths and configuration validation. There are no security-relevant issues introduced by these tests. Approve as submitted.

Review by security

Evaluated against d75e737f

# Security Review ## Summary This PR adds only test code to improve coverage, with no production changes. The tests follow safe practices (e.g., environment sanitization) and do not introduce security risks. ## Recommendation **APPROVE** — CI has passed and the changes are limited to tests exercising error paths and configuration validation. There are no security-relevant issues introduced by these tests. Approve as submitted. --- *Review by security* <!-- review-bot:security --> --- *Evaluated against d75e737f*
sonnet-review-bot approved these changes 2026-05-15 04:16:15 +00:00
sonnet-review-bot left a comment
First-time contributor
Copy Link
Copy Source

Sonnet Review

Summary

This PR adds 7 tests to improve coverage of cmd/review-bot from 44.6% to 49.3%. The tests follow existing patterns in the codebase well, CI is passing, and the code is idiomatic Go. No production code is changed.

Findings

# Severity File Line Finding
1 [MINOR] cmd/review-bot/validateurl_test.go 156 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.
2 [MINOR] cmd/review-bot/main_test.go 1487 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.

Recommendation

APPROVE — Approve. The tests are well-structured, follow the existing subprocess test pattern established in the file, use t.Skip correctly for network-dependent tests, and the cleanEnv() helper is used consistently. CI passes. The two minor findings are non-blocking style observations — discarding the error in the deprecated env test is slightly inconsistent but causes no correctness issues since the test only cares about the output content, not the exit code.

Review by sonnet

Evaluated against d75e737f

# Sonnet Review ## Summary This PR adds 7 tests to improve coverage of `cmd/review-bot` from 44.6% to 49.3%. The tests follow existing patterns in the codebase well, CI is passing, and the code is idiomatic Go. No production code is changed. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `cmd/review-bot/validateurl_test.go` | 156 | 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. | | 2 | [MINOR] | `cmd/review-bot/main_test.go` | 1487 | 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. | ## Recommendation **APPROVE** — Approve. The tests are well-structured, follow the existing subprocess test pattern established in the file, use t.Skip correctly for network-dependent tests, and the `cleanEnv()` helper is used consistently. CI passes. The two minor findings are non-blocking style observations — discarding the error in the deprecated env test is slightly inconsistent but causes no correctness issues since the test only cares about the output content, not the exit code. --- *Review by sonnet* <!-- review-bot:sonnet --> --- *Evaluated against d75e737f*
cmd/review-bot/main_test.go
@@ -1386,0 +1484,4 @@
// No --vcs-url: should fall back to GITEA_URL env var
"--repo", "owner/repo",
"--pr", "1",
"--reviewer-token", "tok",
sonnet-review-bot commented 2026-05-15 04:16:15 +00:00
First-time contributor
Copy Link
Copy Source

[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.

**[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.
cmd/review-bot/validateurl_test.go
@@ -128,0 +153,4 @@
}
}
// TestRunValidateURL_Success confirms that a resolvable public URL prints "OK" and returns 0.
sonnet-review-bot commented 2026-05-15 04:16:15 +00:00
First-time contributor
Copy Link
Copy Source

[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]** 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.
gpt-review-bot approved these changes 2026-05-15 04:16:39 +00:00
gpt-review-bot left a comment
First-time contributor
Copy Link
Copy Source

Gpt Review

Summary

Well-structured test additions that increase coverage without touching production code. Subprocess tests follow the existing pattern, environment isolation is handled, and validateurl tests guard against network variability with skips.

Recommendation

APPROVE — The added tests are idiomatic, focused, and align with repository conventions (no new deps, errors checked, subprocess isolation). The validateurl success-path test prudently skips in no-network environments. Overall, this improves coverage on important edge cases (flag validation, deprecated env fallback, URL validation). No changes requested.

Review by gpt

Evaluated against d75e737f

# Gpt Review ## Summary Well-structured test additions that increase coverage without touching production code. Subprocess tests follow the existing pattern, environment isolation is handled, and validateurl tests guard against network variability with skips. ## Recommendation **APPROVE** — The added tests are idiomatic, focused, and align with repository conventions (no new deps, errors checked, subprocess isolation). The validateurl success-path test prudently skips in no-network environments. Overall, this improves coverage on important edge cases (flag validation, deprecated env fallback, URL validation). No changes requested. --- *Review by gpt* <!-- review-bot:gpt --> --- *Evaluated against d75e737f*
rodin merged commit 1a4bab8ddc into main 2026-05-15 04:35:54 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
security-review-bot
sonnet-review-bot
gpt-review-bot
Labels
Clear labels
ai-review
bug
needs-review
ready
Ready for human review
 self-reviewed
Self-review passed since last push
 wip
Work in progress - do not start new work on this PR
No Label ready self-reviewed
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
security-review-bot rodin (Rodin)
No Assignees
4 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: rodin/review-bot#140
Delete Branch "issue-139"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?