test: fix cleanEnv VCS_ leak, add githubAPIURL tests, add GitHub integration test #136
Closed
rodin
wants to merge 0 commits from
review-bot-issue-133 into main
pull from: review-bot-issue-133
merge into: rodin:main
rodin:main
rodin:ci/cleanup
rodin:ci-selfreview-gate
rodin:issue-150
rodin:issue-157
rodin:issue-141
rodin:issue-154
rodin:review-bot-dev-loop
rodin:issue-143
rodin:issue-146
rodin:pr-153
rodin:review-bot-issue-130-work
rodin:issue-148
rodin:issue-139
rodin:issue-137
rodin:review-bot-fixes
rodin:review-bot-issue-130
rodin:issue-130
rodin:github-support
rodin:issue-123-work
rodin:issue-123
rodin:review-bot-issue-120
rodin:fix/125-readme-cli-example
rodin:issue-125
rodin:issue-124
rodin:issue-120
rodin:feature/github-support
rodin:review-bot-issue-116
rodin:review-bot-issue-115
rodin:review-bot-issue-114
rodin:review-bot-issue-96
rodin:review-bot-issue-107
rodin:review-bot-issue-82
rodin:review-bot-issue-95
rodin:review-bot-issue-92
rodin:review-bot-issue-94
rodin:review-bot-issue-81
rodin:review-bot-issue-91
rodin:review-bot-issue-97
rodin:issue-80-c-file-reader
rodin:issue-80-b-pr-reader
rodin:issue-80-a-client
rodin:review-bot-issue-80
rodin:review-bot-issue-87
rodin:review-bot-issue-79
rodin:review-bot-issue-84
rodin:review-bot-issue-78
rodin:issue-73
rodin:issue-70
rodin:issue-68
rodin:issue-66
rodin:issue-64
rodin:issue-60-remote-personas
rodin:issue-60
rodin:issue-57
rodin:allow-deps
rodin:feat/aicore-provider-v2
rodin:issue-51
rodin:ci/pr-ready-gate
rodin:fix/stale-commit-check
rodin:feat/aicore-provider
rodin:fix/response-body-truncation
rodin:fix/json-repair
rodin:fix/sonnet-reviewer
rodin:fix/consistent-path-escape
rodin:feat/inline-review-comments
rodin:feat/6-update-existing-review
rodin:fix/19-context-overflow
rodin:feat/18-anthropic-api
rodin:fix/url-escaping-and-shadow
rodin:fix/quick-wins
rodin:fix/context-and-encapsulation
rodin:docs/code-review-report
rodin:ci/release-workflow
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Delete Branch "review-bot-issue-133"
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?
Summary
Fixes three post-merge bugs from PR #131 (issue #130).
Changes
Fix #135 — cleanEnv() leaks VCS_TYPE and VCS_URL
cleanEnv()inmain_test.gobuilds a clean subprocess environment for flag-validation tests. It strippedGITEA_*,LLM_*, etc. but did not stripVCS_TYPEorVCS_URL. Now addsVCS_prefix to the exclusion list, preventing environment contamination in all current and future subprocess tests.Fix #134 — githubAPIURL() has no unit tests
Adds
TestGithubAPIURLwith table-driven cases covering all documented behaviors:https://api.github.comhttps://github.com→https://api.github.comhttps://github.com/(trailing slash) →https://api.github.comhttps://ghe.example.com(GHES) →https://ghe.example.com/api/v3https://github.concur.com(GHES, concur domain) →https://github.concur.com/api/v3Fix #133 — integration test not extended for GitHub
Adds
TestIntegration_GitHub_PostAndVerifyReviewthat exercises the GitHub adapter end-to-end:INTEGRATION_GITHUB_TOKEN,INTEGRATION_GITHUB_REPO, orINTEGRATION_GITHUB_PRare absentVCS_TYPE=githubroutes to the GitHub adapterGetAuthenticatedUser,GetPullRequest,PostReview, andListReviewssucceedDeleteReviewon submitted GitHub reviews is expected to return 422 (informational log only)Testing
go test ./...andgo vet ./...pass.Closes #133
Closes #134
Closes #135
Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
bd516cd0)Sonnet Review
Summary
This PR makes three focused, correct fixes: adds VCS_ to the cleanEnv exclusion list, adds unit tests for githubAPIURL, and adds a GitHub integration test. All changes are idiomatic, well-structured, and CI passes.
Findings
cmd/review-bot/integration_test.gogithub.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.cmd/review-bot/integration_test.gofoundvariable 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.Recommendation
APPROVE — Approve. The changes are correct and solve the stated issues. The only meaningful finding is a NIT: the
t.Setenv("VCS_TYPE", "github")call in TestIntegration_GitHub_PostAndVerifyReview has no effect since the test bypasses VCS routing entirely and constructs the GitHub client directly. The comment claiming this 'verifies adapter selection via VCS routing' is misleading and should be corrected to say the test exercises the GitHub adapter directly. This is documentation-only and doesn't affect test correctness or coverage.Review by sonnet
Evaluated against
bd516cd0@@ -162,0 +195,4 @@ctx := context.Background()// Verify the GitHub adapter is selected via VCS routing.[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.@@ -162,0 +234,4 @@found = truebreak}}[NIT] The ListReviews loop breaks early on the first matching review but the
foundvariable 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.Self-review against
4d48917e36Assessment: ✅ Clean
No issues found — ready for human review.
Review notes:
Sonnet Review
Summary
Clean, well-scoped bugfix PR with three isolated changes: a VCS_ env leak fix in cleanEnv(), unit tests for githubAPIURL(), and a GitHub integration test. All changes follow established patterns and CI passes.
Findings
cmd/review-bot/integration_test.gocmd/review-bot/integration_test.goRecommendation
APPROVE — Approve. The three changes are correct and well-targeted: (1) the VCS_ prefix addition to cleanEnv() is a genuine bug fix that prevents environment contamination in subprocess tests; (2) TestGithubAPIURL follows the table-driven pattern correctly with t.Run subtests and proper error messages; (3) the integration test follows the established pattern from TestIntegration_PostAndCleanup, skips gracefully when credentials are absent, and honestly documents the known GitHub limitation around deleting submitted reviews. The two nits are documentation accuracy issues that don't affect correctness. CI passes.
Review by sonnet
Evaluated against
4d48917e@@ -162,0 +194,4 @@owner, repoName := parts[0], parts[1]ctx := context.Background()ghClient := github.NewClient(githubToken, "https://api.github.com")[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.
@@ -162,0 +227,4 @@found := falsefor _, r := range reviews {if r.ID == posted.ID && strings.Contains(r.Body, sentinel) {found = true[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.
Gpt Review
Summary
Solid set of test additions and a small but important fix to cleanEnv. CI passed and the changes are correct and idiomatic. One minor comment/doc mismatch in the new GitHub integration test.
Findings
cmd/review-bot/integration_test.goRecommendation
APPROVE — The changes are well-scoped and improve test coverage and reliability. The cleanEnv addition to filter VCS_ variables prevents test contamination, and the new TestGithubAPIURL covers key cases. The new GitHub integration test is guarded by the integration build tag and skips cleanly when env vars are missing. Please consider updating the comment in TestIntegration_GitHub_PostAndVerifyReview to reflect that the test directly uses the GitHub client (or set VCS_TYPE if you intend to exercise routing), but this is non-blocking. Approve as-is.
Review by gpt
Evaluated against
4d48917e@@ -159,3 +160,85 @@ func TestIntegration_PostAndCleanup(t *testing.T) {t.Logf("Warning: could not delete test review %d: %v", posted.ID, err)}}[NIT] The comment says the test verifies the GitHub adapter is selected via VCS_TYPE=github, but the test constructs the GitHub client directly and does not set or rely on VCS_TYPE. Consider updating the comment or setting VCS_TYPE explicitly for consistency.
Security Review
Summary
The changes add GitHub integration and unit tests and harden test subprocess environments by stripping VCS_* variables. No exploitable vulnerabilities are introduced; CI passed.
Findings
cmd/review-bot/integration_test.gocmd/review-bot/integration_test.goRecommendation
APPROVE — Approve the PR. The changes are confined to tests and improve robustness by cleaning VCS_* env vars and adding coverage for githubAPIURL and GitHub adapter integration. There are no new security vulnerabilities. As a hardening suggestion, consider avoiding logging potentially sensitive repository metadata (usernames, PR titles, SHAs) in integration tests or gating them behind a verbose flag, especially if CI logs may be accessible outside the organization. Optionally add request timeouts in integration tests to prevent long hangs.
Review by security
Evaluated against
4d48917e@@ -162,0 +197,4 @@ghClient := github.NewClient(githubToken, "https://api.github.com")// Verify adapter selection: GetAuthenticatedUser must succeed.user, err := ghClient.GetAuthenticatedUser(ctx)[MINOR] Integration test logs the authenticated GitHub username. While not a secret, in some setups this could expose account identifiers in CI logs for private environments.
@@ -162,0 +204,4 @@t.Logf("Authenticated as: %s", user)// Verify PR is accessible via GitHub adapter.pr, err := ghClient.GetPullRequest(ctx, owner, repoName, prNumber)[MINOR] Integration test logs PR title and commit SHA, which could leak private repository metadata to CI logs if those logs are accessible. Consider reducing or gating such logs.
Pull request closed