feat(cmd): wire --provider and --base-url flags into CLI (Phase 5) #82

Closed
opened 2026-05-12 16:59:58 +00:00 by rodin · 2 comments
Owner

Architecture Reference

This issue is self-contained. For any design questions, the canonical reference is:
docs/DESIGN-vcs-abstraction.md
(Parts 3, 5, and 6 are most relevant to this issue)

Base branch: feature/github-support — open all PRs against this branch.


Goal

Wire the vcs.Client abstraction into cmd/review-bot/main.go with --provider and --base-url flags. This is the phase that makes github.concur.com work end-to-end.

Background

By this point:

  • vcs/interfaces.go, vcs/types.go, vcs/util.go exist (issue #78)
  • gitea.Adapter satisfies vcs.Client (issue #79)
  • github.Client satisfies vcs.Client (issues #80, #81)

The current main.go uses *gitea.Client directly throughout. This issue updates it to use vcs.Client and selects the right adapter at startup.

Auth token

The existing --reviewer-token flag (env: REVIEWER_TOKEN) is reused for both providers. No new token flag needed. The adapters handle the header format difference internally:

  • Gitea adapter: Authorization: token <token>
  • GitHub client: Authorization: Bearer <token>

New flags

--provider github|gitea     default: gitea (backward compatible)
--base-url <url>            VCS API base URL
                            default when --provider github: https://api.github.com
                            default when --provider gitea: use --vcs-url (--gitea-url alias) directly as the base URL; --base-url is unused for provider=gitea and has no effect
                            for GHE: https://github.concur.com/api/v3

Rename --gitea-url--vcs-url (keep --gitea-url and env var GITEA_URL as deprecated aliases). Add env var VCS_URL. Similarly GITEA_REPOVCS_REPO with GITEA_REPO as alias.

Client selection

var client vcs.Client
switch *provider {
case "gitea":
    giteaClient := gitea.NewClient(*vcsURL, *reviewerToken)
    client = gitea.NewAdapter(giteaClient)
case "github":
    baseURL := *baseURL
    if baseURL == "" {
        baseURL = "https://api.github.com"
    }
    client = github.NewClient(*reviewerToken, baseURL)
}

Complete caller audit

Every *gitea.Client call in main.go — what to do with each:

Call Action
giteaClient.GetPullRequest (step 1, fetch metadata) client.GetPullRequest — returns *vcs.PullRequest; all downstream field accesses (pr.Title, pr.Body, pr.Head.Sha, pr.Head.Ref) have identical names, no renames needed
giteaClient.GetPullRequest (step 7, stale check) client.GetPullRequest — same call, different variable; update this separately
giteaClient.GetPullRequestDiff client.GetPullRequestDiff
giteaClient.GetPullRequestFiles client.GetPullRequestFiles; returns []vcs.ChangedFile
giteaClient.GetFileContentRef client.GetFileContentAtRef
giteaClient.GetFileContent client.GetFileContent
giteaClient.GetCommitStatuses client.GetCommitStatuses
evaluateCIStatus(statuses []gitea.CommitStatus, ...) Update parameter type to []vcs.CommitStatus
giteaClient.ListReviews client.ListReviews; returns []vcs.Review; update findOwnReview, findAllOwnReviews, hasSharedToken signatures to take []vcs.Review
giteaClient.PostReview client.PostReview(ctx, owner, repo, prNumber, vcs.ReviewRequest{...}) — see inline comment construction section
giteaClient.GetAuthenticatedUser client.GetAuthenticatedUser
giteaClient.RequestReviewer Gitea-specific — use safe type assertion (see RequestReviewer section)
giteaClient.GetTimelineReviewCommentIDForReview Gitea-specific — see supersede section
giteaClient.EditComment Gitea-specific — see supersede section
giteaClient.ResolveComment Gitea-specific — see supersede section
giteaClient.ListReviewComments Gitea-specific — see supersede section
gitea.ParseDiffNewLines(diff) Gitea-specific — see inline comment construction section
giteaClient.GetAllFilesInPath vcs.GetAllFilesInPath(ctx, client, owner, repo, path) from issue #78; update fetchPatterns to take vcs.FileReader
giteaClient.ListContents client.ListContents
review.GiteaEvent(result.Verdict) Delete — replace with result.Verdict directly (LLM produces canonical values "APPROVE", "REQUEST_CHANGES", "COMMENT"). Delete review.GiteaEvent function from review/ package.

Inline comment construction (critical)

Currently: gitea.ReviewComment{Path: f.File, NewPosition: int64(f.Line), Body: ...} — the LLM produces f.Line as a new-file line number.

After migration: vcs.ReviewComment{Path: f.File, Position: <diff-position>, CommitID: pr.Head.Sha, Body: ...}.

The LLM produces line numbers, not diff-positions. Position in vcs.ReviewComment is a diff-position (GitHub convention). These are different values. The conversion is required:

// Build a line→position map from the diff for each file
lineToPosition := vcs.BuildLineToPositionMap(diff)  // new utility in vcs/util.go

for _, f := range result.Findings {
    pos, ok := lineToPosition[f.File][f.Line]
    if !ok {
        slog.Warn("line not in diff, skipping comment", "file", f.File, "line", f.Line)
        continue
    }
    comments = append(comments, vcs.ReviewComment{
        Path:     f.File,
        Position: pos,
        CommitID: pr.Head.Sha,
        Body:     f.Body,
    })
}

Add vcs.BuildLineToPositionMap(diff string) map[string]map[int]int to vcs/util.go in issue #78. This replaces gitea.ParseDiffNewLines entirely. The map is file → (new-file line → diff-position).

The existing diffRanges.Contains filter is replaced: skip any f.Line not present in the map.

This applies to both providers — the Gitea adapter's PostReview also receives diff-positions (that's the whole point of issue #79), so the line→position conversion must happen in main.go before calling PostReview on either adapter.

fetchFileContext signature

fetchFileContext calls GetFileContentAtRef, which is on vcs.PRReader, not vcs.FileReader. Use vcs.PRReader:

func fetchFileContext(ctx context.Context, client vcs.PRReader, owner, repo, ref string, files []vcs.ChangedFile) string

fetchPatterns only needs GetFileContent + ListContents:

func fetchPatterns(ctx context.Context, client vcs.FileReader, patternsRepo, patternsFiles string) string

giteaClientAdapter / review.GiteaClient removal

review.GiteaClient is defined in review/repo_persona.go. mockGiteaClient in review/repo_persona_test.go implements it.

Action:

  1. Change review.LoadRepoPersonas to accept vcs.FileReader
  2. Delete review.GiteaClient interface from review/repo_persona.go
  3. Delete review.ContentEntry type from review/repo_persona.go (replaced by vcs.ContentEntry)
  4. Update review/repo_persona_test.go: change mockGiteaClient to implement vcs.FileReader — update ListContents return type from []review.ContentEntry to []vcs.ContentEntry, update test data accordingly
  5. Delete giteaClientAdapter struct from main.go
  6. Pass client directly: review.LoadRepoPersonas(ctx, client, owner, repoName)
  7. Update cmd/review-bot/main_test.go: change makeReview return type from gitea.Review to vcs.Review; update all []gitea.Review test fixtures in findOwnReview/hasSharedToken tests to []vcs.Review

RequestReviewer (safe type assertion)

if giteaAdapter, ok := client.(*gitea.Adapter); ok {
    if err := giteaAdapter.Underlying().RequestReviewer(...); err != nil {
        slog.Warn("failed to request reviewer", "error", err)
    }
} else {
    slog.Debug("RequestReviewer not supported for provider, skipping")
}

Never use an unguarded type assertion (client.(*gitea.Adapter)) — it will panic when --provider github.

Supersede flow (provider-branched, safe assertions)

if *provider == "github" {
    for _, old := range ownReviews {
        if err := client.DismissReview(ctx, owner, repo, prNumber, old.ID, "Superseded by new review"); err != nil {
            slog.Warn("failed to dismiss review", "id", old.ID, "error", err)
        }
    }
} else {
    // Gitea: existing EditComment + ResolveComment flow
    giteaAdapter, ok := client.(*gitea.Adapter)
    if !ok {
        slog.Error("expected gitea.Adapter for gitea provider")
        os.Exit(1)
    }
    underlying := giteaAdapter.Underlying()
    // ... existing supersede logic using underlying ...
}

All type assertions to *gitea.Adapter must use the two-value form. Never assert without ok.

vcs.BuildLineToPositionMap utility (add to issue #78)

Note: the inline comment construction section above depends on a new utility vcs.BuildLineToPositionMap(diff string) map[string]map[int]int. This must be added to vcs/util.go in issue #78 (or as part of this issue if preferred). It inverts the position→line map from the Gitea adapter's diff parser: given a unified diff, returns file → (new-line → diff-position).

Exit criteria

  • ./review-bot --provider gitea ... works identically to before (zero regression)
  • ./review-bot --provider github --base-url https://github.concur.com/api/v3 ... posts a review on a real GHE PR
  • ./review-bot --provider github ... works against github.com
  • --gitea-url and GITEA_URL still accepted as deprecated aliases
  • Validation block updated: *vcsURL is only required when *provider == "gitea"; for provider == "github" it is optional (base URL defaults to https://api.github.com or *baseURL)
  • giteaClientAdapter, review.GiteaClient, review.ContentEntry, review.GiteaEvent all deleted
  • review/repo_persona_test.go updated: mockGiteaClient implements vcs.FileReader
  • No unguarded client.(*gitea.Adapter) assertions anywhere
  • All existing tests pass

Out of scope

  • GitHub Actions workflow (future issue)
  • Binary release pipeline (future issue)
## Architecture Reference > This issue is self-contained. For any design questions, the canonical reference is: > [`docs/DESIGN-vcs-abstraction.md`](https://gitea.weiker.me/rodin/review-bot/src/branch/feature/github-support/docs/DESIGN-vcs-abstraction.md) > (Parts 3, 5, and 6 are most relevant to this issue) **Base branch:** `feature/github-support` — open all PRs against this branch. --- ## Goal Wire the `vcs.Client` abstraction into `cmd/review-bot/main.go` with `--provider` and `--base-url` flags. This is the phase that makes `github.concur.com` work end-to-end. ## Background By this point: - `vcs/interfaces.go`, `vcs/types.go`, `vcs/util.go` exist (issue #78) - `gitea.Adapter` satisfies `vcs.Client` (issue #79) - `github.Client` satisfies `vcs.Client` (issues #80, #81) The current `main.go` uses `*gitea.Client` directly throughout. This issue updates it to use `vcs.Client` and selects the right adapter at startup. ## Auth token The existing `--reviewer-token` flag (env: `REVIEWER_TOKEN`) is reused for both providers. No new token flag needed. The adapters handle the header format difference internally: - Gitea adapter: `Authorization: token <token>` - GitHub client: `Authorization: Bearer <token>` ## New flags ``` --provider github|gitea default: gitea (backward compatible) --base-url <url> VCS API base URL default when --provider github: https://api.github.com default when --provider gitea: use --vcs-url (--gitea-url alias) directly as the base URL; --base-url is unused for provider=gitea and has no effect for GHE: https://github.concur.com/api/v3 ``` Rename `--gitea-url` → `--vcs-url` (keep `--gitea-url` and env var `GITEA_URL` as deprecated aliases). Add env var `VCS_URL`. Similarly `GITEA_REPO` → `VCS_REPO` with `GITEA_REPO` as alias. ## Client selection ```go var client vcs.Client switch *provider { case "gitea": giteaClient := gitea.NewClient(*vcsURL, *reviewerToken) client = gitea.NewAdapter(giteaClient) case "github": baseURL := *baseURL if baseURL == "" { baseURL = "https://api.github.com" } client = github.NewClient(*reviewerToken, baseURL) } ``` ## Complete caller audit Every `*gitea.Client` call in `main.go` — what to do with each: | Call | Action | |------|--------| | `giteaClient.GetPullRequest` (step 1, fetch metadata) | → `client.GetPullRequest` — returns `*vcs.PullRequest`; all downstream field accesses (`pr.Title`, `pr.Body`, `pr.Head.Sha`, `pr.Head.Ref`) have identical names, no renames needed | | `giteaClient.GetPullRequest` (step 7, stale check) | → `client.GetPullRequest` — same call, different variable; update this separately | | `giteaClient.GetPullRequestDiff` | → `client.GetPullRequestDiff` | | `giteaClient.GetPullRequestFiles` | → `client.GetPullRequestFiles`; returns `[]vcs.ChangedFile` | | `giteaClient.GetFileContentRef` | → `client.GetFileContentAtRef` | | `giteaClient.GetFileContent` | → `client.GetFileContent` | | `giteaClient.GetCommitStatuses` | → `client.GetCommitStatuses` | | `evaluateCIStatus(statuses []gitea.CommitStatus, ...)` | Update parameter type to `[]vcs.CommitStatus` | | `giteaClient.ListReviews` | → `client.ListReviews`; returns `[]vcs.Review`; update `findOwnReview`, `findAllOwnReviews`, `hasSharedToken` signatures to take `[]vcs.Review` | | `giteaClient.PostReview` | → `client.PostReview(ctx, owner, repo, prNumber, vcs.ReviewRequest{...})` — see inline comment construction section | | `giteaClient.GetAuthenticatedUser` | → `client.GetAuthenticatedUser` | | `giteaClient.RequestReviewer` | Gitea-specific — use safe type assertion (see RequestReviewer section) | | `giteaClient.GetTimelineReviewCommentIDForReview` | Gitea-specific — see supersede section | | `giteaClient.EditComment` | Gitea-specific — see supersede section | | `giteaClient.ResolveComment` | Gitea-specific — see supersede section | | `giteaClient.ListReviewComments` | Gitea-specific — see supersede section | | `gitea.ParseDiffNewLines(diff)` | Gitea-specific — see inline comment construction section | | `giteaClient.GetAllFilesInPath` | → `vcs.GetAllFilesInPath(ctx, client, owner, repo, path)` from issue #78; update `fetchPatterns` to take `vcs.FileReader` | | `giteaClient.ListContents` | → `client.ListContents` | | `review.GiteaEvent(result.Verdict)` | **Delete** — replace with `result.Verdict` directly (LLM produces canonical values `"APPROVE"`, `"REQUEST_CHANGES"`, `"COMMENT"`). Delete `review.GiteaEvent` function from `review/` package. | ## Inline comment construction (critical) Currently: `gitea.ReviewComment{Path: f.File, NewPosition: int64(f.Line), Body: ...}` — the LLM produces `f.Line` as a new-file line number. After migration: `vcs.ReviewComment{Path: f.File, Position: <diff-position>, CommitID: pr.Head.Sha, Body: ...}`. **The LLM produces line numbers, not diff-positions.** `Position` in `vcs.ReviewComment` is a diff-position (GitHub convention). These are different values. The conversion is required: ```go // Build a line→position map from the diff for each file lineToPosition := vcs.BuildLineToPositionMap(diff) // new utility in vcs/util.go for _, f := range result.Findings { pos, ok := lineToPosition[f.File][f.Line] if !ok { slog.Warn("line not in diff, skipping comment", "file", f.File, "line", f.Line) continue } comments = append(comments, vcs.ReviewComment{ Path: f.File, Position: pos, CommitID: pr.Head.Sha, Body: f.Body, }) } ``` Add `vcs.BuildLineToPositionMap(diff string) map[string]map[int]int` to `vcs/util.go` in issue #78. This replaces `gitea.ParseDiffNewLines` entirely. The map is `file → (new-file line → diff-position)`. The existing `diffRanges.Contains` filter is replaced: skip any `f.Line` not present in the map. This applies to **both providers** — the Gitea adapter's PostReview also receives diff-positions (that's the whole point of issue #79), so the line→position conversion must happen in main.go before calling PostReview on either adapter. ## `fetchFileContext` signature `fetchFileContext` calls `GetFileContentAtRef`, which is on `vcs.PRReader`, not `vcs.FileReader`. Use `vcs.PRReader`: ```go func fetchFileContext(ctx context.Context, client vcs.PRReader, owner, repo, ref string, files []vcs.ChangedFile) string ``` `fetchPatterns` only needs `GetFileContent` + `ListContents`: ```go func fetchPatterns(ctx context.Context, client vcs.FileReader, patternsRepo, patternsFiles string) string ``` ## `giteaClientAdapter` / `review.GiteaClient` removal `review.GiteaClient` is defined in `review/repo_persona.go`. `mockGiteaClient` in `review/repo_persona_test.go` implements it. **Action:** 1. Change `review.LoadRepoPersonas` to accept `vcs.FileReader` 2. Delete `review.GiteaClient` interface from `review/repo_persona.go` 3. Delete `review.ContentEntry` type from `review/repo_persona.go` (replaced by `vcs.ContentEntry`) 4. Update `review/repo_persona_test.go`: change `mockGiteaClient` to implement `vcs.FileReader` — update `ListContents` return type from `[]review.ContentEntry` to `[]vcs.ContentEntry`, update test data accordingly 5. Delete `giteaClientAdapter` struct from `main.go` 6. Pass `client` directly: `review.LoadRepoPersonas(ctx, client, owner, repoName)` 7. Update `cmd/review-bot/main_test.go`: change `makeReview` return type from `gitea.Review` to `vcs.Review`; update all `[]gitea.Review` test fixtures in `findOwnReview`/`hasSharedToken` tests to `[]vcs.Review` ## `RequestReviewer` (safe type assertion) ```go if giteaAdapter, ok := client.(*gitea.Adapter); ok { if err := giteaAdapter.Underlying().RequestReviewer(...); err != nil { slog.Warn("failed to request reviewer", "error", err) } } else { slog.Debug("RequestReviewer not supported for provider, skipping") } ``` Never use an unguarded type assertion (`client.(*gitea.Adapter)`) — it will panic when `--provider github`. ## Supersede flow (provider-branched, safe assertions) ```go if *provider == "github" { for _, old := range ownReviews { if err := client.DismissReview(ctx, owner, repo, prNumber, old.ID, "Superseded by new review"); err != nil { slog.Warn("failed to dismiss review", "id", old.ID, "error", err) } } } else { // Gitea: existing EditComment + ResolveComment flow giteaAdapter, ok := client.(*gitea.Adapter) if !ok { slog.Error("expected gitea.Adapter for gitea provider") os.Exit(1) } underlying := giteaAdapter.Underlying() // ... existing supersede logic using underlying ... } ``` **All type assertions to `*gitea.Adapter` must use the two-value form.** Never assert without `ok`. ## `vcs.BuildLineToPositionMap` utility (add to issue #78) Note: the inline comment construction section above depends on a new utility `vcs.BuildLineToPositionMap(diff string) map[string]map[int]int`. This must be added to `vcs/util.go` in issue #78 (or as part of this issue if preferred). It inverts the position→line map from the Gitea adapter's diff parser: given a unified diff, returns `file → (new-line → diff-position)`. ## Exit criteria - `./review-bot --provider gitea ...` works identically to before (zero regression) - `./review-bot --provider github --base-url https://github.concur.com/api/v3 ...` posts a review on a real GHE PR - `./review-bot --provider github ...` works against `github.com` - `--gitea-url` and `GITEA_URL` still accepted as deprecated aliases - Validation block updated: `*vcsURL` is only required when `*provider == "gitea"`; for `provider == "github"` it is optional (base URL defaults to `https://api.github.com` or `*baseURL`) - `giteaClientAdapter`, `review.GiteaClient`, `review.ContentEntry`, `review.GiteaEvent` all deleted - `review/repo_persona_test.go` updated: `mockGiteaClient` implements `vcs.FileReader` - No unguarded `client.(*gitea.Adapter)` assertions anywhere - All existing tests pass ## Out of scope - GitHub Actions workflow (future issue) - Binary release pipeline (future issue)
Author
Owner

Update: When auditing callers that use *gitea.Client directly, pay attention to any call to DeleteReview that is used to replace a submitted review before posting a new one. Those callers must be updated to use DismissReview instead — DeleteReview only works on PENDING reviews on GitHub, and calling it on a submitted review returns 422.

**Update:** When auditing callers that use `*gitea.Client` directly, pay attention to any call to `DeleteReview` that is used to *replace* a submitted review before posting a new one. Those callers must be updated to use `DismissReview` instead — `DeleteReview` only works on PENDING reviews on GitHub, and calling it on a submitted review returns 422.
Author
Owner

Update: fetchPatterns and fetchFileContext signatures — Both helpers currently take *gitea.Client directly.

  • Update fetchPatterns to take vcs.FileReader and use vcs.GetAllFilesInPath (utility added in #78) for recursive traversal
  • Update fetchFileContext to take vcs.FileReader (uses GetFileContentAtRef)

Neither function needs the full vcs.Clientvcs.FileReader is sufficient and keeps the signatures narrow.

**Update: `fetchPatterns` and `fetchFileContext` signatures** — Both helpers currently take `*gitea.Client` directly. - Update `fetchPatterns` to take `vcs.FileReader` and use `vcs.GetAllFilesInPath` (utility added in #78) for recursive traversal - Update `fetchFileContext` to take `vcs.FileReader` (uses `GetFileContentAtRef`) Neither function needs the full `vcs.Client` — `vcs.FileReader` is sufficient and keeps the signatures narrow.
rodin self-assigned this 2026-05-13 08:11:53 +00:00
Sign in to join this conversation.
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: rodin/review-bot#82