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

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

Merged
aweiker merged 17 commits from review-bot-issue-82 into feature/github-support 2026-05-13 17:16:28 +00:00
Conversation 98 Commits 17 Files Changed 14
+9 -9
3 changed files with 9 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 5252143a33 - Show all commits
cmd/review-bot/main.go
+4 -6
View File
8
 @@ -85,8 +85,8 @@ func main() {
aicoreResourceGroup := flag.String("aicore-resource-group", envOrDefault("AICORE_RESOURCE_GROUP", "default"), "SAP AI Core resource group (for provider=aicore)")
rodin marked this conversation as resolved
sonnet-review-bot commented 2026-05-13 11:34:32 +00:00
Review
Copy Link
Copy Source

[NIT] The --gitea-url alias comment block is long (12 lines) and explains a subtle flag.StringVar trick. Consider extracting the alias registration to a small helper or at minimum shortening the comment to the key invariant: flag.StringVar shares the pointer, so whichever flag is set last wins. The current comment is accurate but its length makes it easy to miss the ORDERING constraint.

**[NIT]** The `--gitea-url` alias comment block is long (12 lines) and explains a subtle flag.StringVar trick. Consider extracting the alias registration to a small helper or at minimum shortening the comment to the key invariant: `flag.StringVar shares the pointer, so whichever flag is set last wins.` The current comment is accurate but its length makes it easy to miss the ORDERING constraint.
// Backward-compatible alias: --gitea-url shares vcsURL's pointer (last flag wins).
sonnet-review-bot commented 2026-05-13 09:24:34 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The --gitea-url alias registration reads *vcsURL at registration time (before flag.Parse()). At that point *vcsURL holds the default value from envOrDefault(...), so the alias default is correct. However, if a user passes --vcs-url foo on the command line, the alias's default is stale — only the variable is shared, not a live default. This is acceptable because the flag package updates the pointed-to variable via flag.StringVar, so any value written by either --vcs-url or --gitea-url goes to the same *string. But the help text for --gitea-url will show the env-var default rather than reflecting --vcs-url's current value, which is mildly confusing. Consider a brief comment explaining the aliasing semantics.

**[MINOR]** The `--gitea-url` alias registration reads `*vcsURL` at registration time (before `flag.Parse()`). At that point `*vcsURL` holds the default value from `envOrDefault(...)`, so the alias default is correct. However, if a user passes `--vcs-url foo` on the command line, the alias's default is stale — only the variable is shared, not a live default. This is acceptable because the flag package updates the pointed-to variable via `flag.StringVar`, so any value written by either `--vcs-url` or `--gitea-url` goes to the same `*string`. But the help text for `--gitea-url` will show the env-var default rather than reflecting `--vcs-url`'s current value, which is mildly confusing. Consider a brief comment explaining the aliasing semantics.
sonnet-review-bot commented 2026-05-13 15:02:31 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The comment on the --gitea-url alias registration says "The *vcsURL dereference captures the env-var-resolved default at registration time". This is accurate but the behaviour when both --vcs-url and --gitea-url are passed (last one wins) is subtle enough that a brief integration test would provide more confidence than a comment alone.

**[NIT]** The comment on the `--gitea-url` alias registration says "The *vcsURL dereference captures the env-var-resolved default at registration time". This is accurate but the behaviour when both `--vcs-url` and `--gitea-url` are passed (last one wins) is subtle enough that a brief integration test would provide more confidence than a comment alone.
sonnet-review-bot commented 2026-05-13 16:04:55 +00:00
Review
Copy Link
Copy Source

[MINOR] The --gitea-url backward-compatible alias is registered with flag.StringVar(vcsURL, "gitea-url", *vcsURL, ...) where *vcsURL is the default value captured at the moment of the call — before flag.Parse(). This is correct as written because *vcsURL is still the default string at that point, but it creates a subtle ordering dependency: if vcsURL ever acquired a non-default value between its declaration and this flag.StringVar call (e.g., if any code ran between them), the alias would get a stale default. The comment acknowledges this (Must stay after vcsURL declaration and before flag.Parse()), but the fragility is worth noting. A cleaner pattern would be flag.StringVar(vcsURL, "gitea-url", "", "...") or extracting the default value to a named constant.

**[MINOR]** The `--gitea-url` backward-compatible alias is registered with `flag.StringVar(vcsURL, "gitea-url", *vcsURL, ...)` where `*vcsURL` is the default value captured at the moment of the call — before `flag.Parse()`. This is correct as written because `*vcsURL` is still the default string at that point, but it creates a subtle ordering dependency: if `vcsURL` ever acquired a non-default value between its declaration and this `flag.StringVar` call (e.g., if any code ran between them), the alias would get a stale default. The comment acknowledges this (`Must stay after vcsURL declaration and before flag.Parse()`), but the fragility is worth noting. A cleaner pattern would be `flag.StringVar(vcsURL, "gitea-url", "", "...")` or extracting the default value to a named constant.
// Must stay after vcsURL declaration and before flag.Parse().
flag.StringVar(vcsURL, "gitea-url", *vcsURL, "Deprecated: use --vcs-url instead")
// Shares vcsURL pointer; empty default avoids ordering dependency with vcsURL declaration.
sonnet-review-bot commented 2026-05-13 10:30:42 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The --gitea-url alias registration uses *vcsURL as the default value at registration time. This works correctly because flag parsing hasn't happened yet, so *vcsURL holds the env-var-resolved default. However this is subtle — if the evaluation order ever changed (e.g., someone moves flag.Parse() above this line), the alias would silently use an empty string default instead. A brief comment noting this dependency on registration-before-parse order would help future readers.

**[NIT]** The `--gitea-url` alias registration uses `*vcsURL` as the default value at registration time. This works correctly because flag parsing hasn't happened yet, so `*vcsURL` holds the env-var-resolved default. However this is subtle — if the evaluation order ever changed (e.g., someone moves `flag.Parse()` above this line), the alias would silently use an empty string default instead. A brief comment noting this dependency on registration-before-parse order would help future readers.
flag.StringVar(vcsURL, "gitea-url", "", "Deprecated: use --vcs-url instead")
flag.Parse()
6
 @@ -162,7 +162,7 @@ func main() {
case vcs.ProviderGitea:
giteaClient := gitea.NewClient(*vcsURL, *reviewerToken)
client = gitea.NewAdapter(giteaClient)
case vcs.ProviderGithub:
case vcs.ProviderGitHub:
client = github.NewClient(*reviewerToken, *baseURL)
default:
rodin marked this conversation as resolved Outdated
sonnet-review-bot commented 2026-05-13 11:34:32 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The default case in the VCS client initialization switch (default: fmt.Fprintf(os.Stderr, ...) ; os.Exit(1)) is unreachable dead code: the provider is already validated by the earlier switch at line 109-114 which also exits on invalid values. This is harmless but adds noise.

**[MINOR]** The default case in the VCS client initialization switch (`default: fmt.Fprintf(os.Stderr, ...) ; os.Exit(1)`) is unreachable dead code: the provider is already validated by the earlier switch at line 109-114 which also exits on invalid values. This is harmless but adds noise.
panic("unreachable: provider validation should have caught " + vcsProvider.String())
37
 @@ -501,7 +501,7 @@ func main() {
os.Exit(1)
}
sonnet-review-bot commented 2026-05-13 13:20:49 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The supersedeOldReviews function uses a type assertion client.(*gitea.Adapter) in what is supposed to be a provider-dispatch function. This leaks the concrete Gitea type into logic that should be abstracted by the vcs.Client interface. The comment acknowledges this is 'guaranteed to succeed', but it's a coupling smell: cmd/main.go now imports and depends on gitea.Adapter internals for a Gitea-specific code path. This is acceptable for now given the Gitea-specific EditComment/ResolveComment/GetTimelineReviewCommentIDForReview calls have no VCS-agnostic equivalent, but it should be noted as technical debt if additional providers are added.

**[MINOR]** The `supersedeOldReviews` function uses a type assertion `client.(*gitea.Adapter)` in what is supposed to be a provider-dispatch function. This leaks the concrete Gitea type into logic that should be abstracted by the `vcs.Client` interface. The comment acknowledges this is 'guaranteed to succeed', but it's a coupling smell: `cmd/main.go` now imports and depends on `gitea.Adapter` internals for a Gitea-specific code path. This is acceptable for now given the Gitea-specific `EditComment`/`ResolveComment`/`GetTimelineReviewCommentIDForReview` calls have no VCS-agnostic equivalent, but it should be noted as technical debt if additional providers are added.
} else {
slog.Warn("provider does not support review superseding", "provider", vcsProvider)
slog.Error("provider does not support review superseding", "provider", vcsProvider)
}
sonnet-review-bot commented 2026-05-13 10:08:59 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] In supersedeOldReviews, the GitHub case iterates over oldReviews and logs a warning on DismissReview failure but continues without returning an error. The function still returns nil after the loop even if all dismissals failed. This is intentional (best-effort dismissal) but the caller in main() calls os.Exit(1) on any error returned — so failures here are silently swallowed. Consider either returning a joined error for all failures or at minimum documenting the best-effort semantics in the function doc comment.

**[MINOR]** In `supersedeOldReviews`, the GitHub case iterates over `oldReviews` and logs a warning on `DismissReview` failure but continues without returning an error. The function still returns `nil` after the loop even if all dismissals failed. This is intentional (best-effort dismissal) but the caller in `main()` calls `os.Exit(1)` on any error returned — so failures here are silently swallowed. Consider either returning a joined error for all failures or at minimum documenting the best-effort semantics in the function doc comment.
}
}
20
 @@ -721,8 +721,6 @@ func validateWorkspacePath(path, pathName string) (string, error) {
return resolvedPath, nil
sonnet-review-bot commented 2026-05-13 16:04:55 +00:00
Review
Copy Link
Copy Source

[NIT] There is a dangling comment fragment: // with collapsed original content and the commit it was evaluated against. — this is the second half of the buildSupersededBody doc comment that was removed when the function moved to gitea/adapter.go. The first line of the doc comment (// buildSupersededBody creates the body for a superseded review: struck-through banner) was deleted but this trailing line was left orphaned. It should be removed.

**[NIT]** There is a dangling comment fragment: `// with collapsed original content and the commit it was evaluated against.` — this is the second half of the `buildSupersededBody` doc comment that was removed when the function moved to `gitea/adapter.go`. The first line of the doc comment (`// buildSupersededBody creates the body for a superseded review: struck-through banner`) was deleted but this trailing line was left orphaned. It should be removed.
}
// with collapsed original content and the commit it was evaluated against.
// hasSharedToken detects if another review-bot role posted under the same
// VCS user. This indicates misconfiguration where two roles share a token
// instead of having separate accounts. Returns true if shared token
gitea/adapter.go
+3 -1
View File
3
 @@ -282,7 +282,9 @@ func (a *Adapter) SupersedeReviews(ctx context.Context, owner, repo string, prNu
if c.ID == 0 {
continue
}
_ = underlying.ResolveComment(ctx, owner, repo, c.ID)
if err := underlying.ResolveComment(ctx, owner, repo, c.ID); err != nil {
slog.Debug("could not resolve inline comment", "comment_id", c.ID, "error", err)
}
}
}
return nil
vcs/provider.go
+2 -2
View File
@@ -7,13 +7,13 @@ type VCSProvider string
const (
ProviderGitea VCSProvider = "gitea"
ProviderGithub VCSProvider = "github"
ProviderGitHub VCSProvider = "github"
sonnet-review-bot commented 2026-05-13 16:04:55 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] ProviderGithub VCSProvider = "github" — the constant name uses Github (lowercase 'h') while the package import path and the flag help text use github. Per the Go acronym convention documented in patterns/style.md (Acronyms Are All-Caps), this should be ProviderGitHub. This is a new exported symbol so fixing it now is low-cost; changing it later would break callers.

**[NIT]** `ProviderGithub VCSProvider = "github"` — the constant name uses `Github` (lowercase 'h') while the package import path and the flag help text use `github`. Per the Go acronym convention documented in `patterns/style.md` (Acronyms Are All-Caps), this should be `ProviderGitHub`. This is a new exported symbol so fixing it now is low-cost; changing it later would break callers.
)
// Valid reports whether p is a known VCS provider.
func (p VCSProvider) Valid() bool {
switch p {
case ProviderGitea, ProviderGithub:
case ProviderGitea, ProviderGitHub:
return true
default:
return false