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
+515 -212
5 changed files with 515 additions and 212 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 4881a21ecb - Show all commits
cmd/review-bot/main.go
+186 -136
View File
@@ -13,6 +13,7 @@ import (
"gitea.weiker.me/rodin/review-bot/budget"
"gitea.weiker.me/rodin/review-bot/gitea"
"gitea.weiker.me/rodin/review-bot/github"
"gitea.weiker.me/rodin/review-bot/llm"
"gitea.weiker.me/rodin/review-bot/review"
"gitea.weiker.me/rodin/review-bot/vcs"
@@ -54,12 +55,15 @@ func main() {
// Logging flags
logFormat := flag.String("log-format", envOrDefault("LOG_FORMAT", "text"), "Log output format: text or json")
sonnet-review-bot commented 2026-05-13 14:15:32 +00:00
Review
Copy Link
Copy Source

[NIT] The envOrDefaultBool function is declared and used in the file but does not appear to be called from main() in this diff — it may be leftover from a previous version. This is not a new issue introduced by this PR, just an observation.

**[NIT]** The `envOrDefaultBool` function is declared and used in the file but does not appear to be called from `main()` in this diff — it may be leftover from a previous version. This is not a new issue introduced by this PR, just an observation.
verbosity := flag.String("verbosity", envOrDefault("LOG_VERBOSITY", "info"), "Log verbosity: debug, info, warn, error")
// CLI flags
giteaURL := flag.String("gitea-url", envOrDefault("GITEA_URL", envOrDefault("GITHUB_SERVER_URL", "")), "Gitea instance URL")
repo := flag.String("repo", envOrDefault("GITEA_REPO", envOrDefault("GITHUB_REPOSITORY", "")), "Repository (owner/name)")
// VCS flags
provider := flag.String("provider", envOrDefault("VCS_PROVIDER", "gitea"), "VCS provider: gitea or github")
baseURL := flag.String("base-url", envOrDefault("VCS_BASE_URL", ""), "VCS API base URL (for github provider; defaults to https://api.github.com)")
vcsURL := flag.String("vcs-url", envOrDefault("VCS_URL", envOrDefault("GITEA_URL", envOrDefault("GITHUB_SERVER_URL", ""))), "VCS instance URL (Gitea) [deprecated alias: --gitea-url]")
// Keep --gitea-url as hidden alias (flag package doesn't support aliases natively, handle below)
repo := flag.String("repo", envOrDefault("VCS_REPO", envOrDefault("GITEA_REPO", envOrDefault("GITHUB_REPOSITORY", ""))), "Repository (owner/name)")
prNum := flag.String("pr", envOrDefault("PR_NUMBER", ""), "Pull request number")
reviewerName := flag.String("reviewer-name", envOrDefault("REVIEWER_NAME", ""), "Reviewer display name")
sonnet-review-bot commented 2026-05-13 13:45:25 +00:00
Review
Copy Link
Copy Source

[NIT] The --gitea-url alias registration comment says 'If a user passes both --vcs-url and --gitea-url, the last one on the command line takes effect'. However, since both --vcs-url and --gitea-url share the same pointer via StringVar, only the last flag parsed wins. This is the documented behavior but may be surprising to users who set GITEA_URL env var and also pass --vcs-url on the command line — the flag will win. This is standard flag package behavior but worth a brief note in the help text.

**[NIT]** The `--gitea-url` alias registration comment says 'If a user passes both --vcs-url and --gitea-url, the last one on the command line takes effect'. However, since both `--vcs-url` and `--gitea-url` share the same pointer via `StringVar`, only the *last* flag parsed wins. This is the documented behavior but may be surprising to users who set `GITEA_URL` env var and also pass `--vcs-url` on the command line — the flag will win. This is standard flag package behavior but worth a brief note in the help text.
reviewerToken := flag.String("reviewer-token", envOrDefault("REVIEWER_TOKEN", ""), "Gitea token for posting review")
reviewerToken := flag.String("reviewer-token", envOrDefault("REVIEWER_TOKEN", ""), "VCS token for posting review")
llmBaseURL := flag.String("llm-base-url", envOrDefault("LLM_BASE_URL", ""), "LLM API base URL")
llmAPIKey := flag.String("llm-api-key", envOrDefault("LLM_API_KEY", ""), "LLM API key")
llmModel := flag.String("llm-model", envOrDefault("LLM_MODEL", ""), "LLM model name")
@@ -80,6 +84,9 @@ func main() {
aicoreAPIURL := flag.String("aicore-api-url", envOrDefault("AICORE_API_URL", ""), "SAP AI Core API URL (for provider=aicore)")
sonnet-review-bot commented 2026-05-13 09:37:04 +00:00
Review
Copy Link
Copy Source

[MINOR] The --gitea-url alias registration uses flag.StringVar(vcsURL, "gitea-url", *vcsURL, ...) after vcsURL has already been parsed from env vars. This means the default for the alias is whatever vcsURL resolved to at registration time, not a live binding. In practice this works fine because flag.Parse() hasn't run yet, but it's subtly fragile: if the env-var chain for vcsURL ever produces an empty string, the alias and the primary flag start with different defaults. A comment explaining this would help future maintainers.

**[MINOR]** The --gitea-url alias registration uses `flag.StringVar(vcsURL, "gitea-url", *vcsURL, ...)` after vcsURL has already been parsed from env vars. This means the default for the alias is whatever vcsURL resolved to at registration time, not a live binding. In practice this works fine because flag.Parse() hasn't run yet, but it's subtly fragile: if the env-var chain for vcsURL ever produces an empty string, the alias and the primary flag start with different defaults. A comment explaining this would help future maintainers.
gpt-review-bot commented 2026-05-13 09:37:54 +00:00
Review
Copy Link
Copy Source

[NIT] The comment says the --gitea-url flag is a hidden alias, but it is registered normally and will appear in usage. Either adjust wording to 'deprecated alias' or consider not exposing it in help if truly intended to be hidden.

**[NIT]** The comment says the --gitea-url flag is a hidden alias, but it is registered normally and will appear in usage. Either adjust wording to 'deprecated alias' or consider not exposing it in help if truly intended to be hidden.
sonnet-review-bot commented 2026-05-13 10:08:59 +00:00
Review
Copy Link
Copy Source

[MINOR] The --gitea-url backward-compatible alias is registered with flag.StringVar(vcsURL, "gitea-url", *vcsURL, ...). This dereferences *vcsURL at flag registration time to capture the current default, which is correct. However, the comment says 'whichever flag is set last by flag.Parse wins' — this is accurate but the behavior may surprise users if both --vcs-url and --gitea-url are passed simultaneously (last one wins rather than an error). This is a known limitation of the flag package alias approach and should ideally be documented in the flag usage or a warning emitted if both are set.

**[MINOR]** The `--gitea-url` backward-compatible alias is registered with `flag.StringVar(vcsURL, "gitea-url", *vcsURL, ...)`. This dereferences `*vcsURL` at flag registration time to capture the current default, which is correct. However, the comment says 'whichever flag is set last by flag.Parse wins' — this is accurate but the behavior may surprise users if both `--vcs-url` and `--gitea-url` are passed simultaneously (last one wins rather than an error). This is a known limitation of the `flag` package alias approach and should ideally be documented in the flag usage or a warning emitted if both are set.
rodin commented 2026-05-13 10:10:20 +00:00
Review
Copy Link
Copy Source

Added a comment explaining the shared *string pointer mechanism. flag.StringVar(vcsURL, ...) binds --gitea-url to the same underlying *string as --vcs-url, so there's no divergence risk — both flags always resolve to the same value after flag.Parse(). Comment added in 7d6fe27.

Added a comment explaining the shared `*string` pointer mechanism. `flag.StringVar(vcsURL, ...)` binds `--gitea-url` to the same underlying `*string` as `--vcs-url`, so there's no divergence risk — both flags always resolve to the same value after `flag.Parse()`. Comment added in 7d6fe27.
rodin commented 2026-05-13 10:10:20 +00:00
Review
Copy Link
Copy Source

Updated the inline comment from "hidden alias" to "backward-compatible alias" in 7d6fe27. The flag does appear in --help output (Go's flag package doesn't support hiding flags), so "hidden" was inaccurate. "Backward-compatible" better describes the intent.

Updated the inline comment from "hidden alias" to "backward-compatible alias" in 7d6fe27. The flag does appear in `--help` output (Go's `flag` package doesn't support hiding flags), so "hidden" was inaccurate. "Backward-compatible" better describes the intent.
sonnet-review-bot commented 2026-05-13 16:34:41 +00:00
Review
Copy Link
Copy Source

[NIT] The comment // Backward-compatible alias: --gitea-url shares vcsURL's pointer (last flag wins). is accurate but slightly misleading: with flag.StringVar(vcsURL, "gitea-url", *vcsURL, ...) the last parsed flag wins at parse time. If both --vcs-url and --gitea-url are supplied, the rightmost one on the command line takes effect. This is the correct and intended behaviour, but a small clarification in the comment ("last supplied on the command line wins") would help future readers.

**[NIT]** The comment `// Backward-compatible alias: --gitea-url shares vcsURL's pointer (last flag wins).` is accurate but slightly misleading: with `flag.StringVar(vcsURL, "gitea-url", *vcsURL, ...)` the last *parsed* flag wins at parse time. If both `--vcs-url` and `--gitea-url` are supplied, the rightmost one on the command line takes effect. This is the correct and intended behaviour, but a small clarification in the comment ("last supplied on the command line wins") would help future readers.
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.
// Register --gitea-url as a deprecated alias for --vcs-url
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.
flag.StringVar(vcsURL, "gitea-url", *vcsURL, "Deprecated: use --vcs-url instead")
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.Parse()
if *versionFlag {
4
 @@ -92,12 +99,25 @@ func main() {
slog.Info("review-bot starting", "version", version)
gpt-review-bot commented 2026-05-13 13:51:46 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] Flag description for --base-url could mention GitHub Enterprise explicitly (e.g., "GitHub API base URL for github.com or GitHub Enterprise"), though this is already implied by tests and usage.

**[NIT]** Flag description for --base-url could mention GitHub Enterprise explicitly (e.g., "GitHub API base URL for github.com or GitHub Enterprise"), though this is already implied by tests and usage.
sonnet-review-bot commented 2026-05-13 14:46:18 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The envOrDefaultBool function is defined and tested but never called in main.go (no usages in the diff or full file). It may be used elsewhere or kept for future use, but if unused it could be removed to keep the package clean.

**[NIT]** The `envOrDefaultBool` function is defined and tested but never called in main.go (no usages in the diff or full file). It may be used elsewhere or kept for future use, but if unused it could be removed to keep the package clean.
// Validate VCS provider
switch *provider {
case "gitea", "github":
// valid
default:
fmt.Fprintf(os.Stderr, "Error: invalid --provider %q (valid: gitea, github)\n", *provider)
os.Exit(1)
}
// Validate required fields
// For aicore provider, llm-base-url and llm-api-key are not required
isAICore := llm.Provider(*llmProvider) == llm.ProviderAICore
if *giteaURL == "" || *repo == "" || *prNum == "" || *reviewerToken == "" || *llmModel == "" {
if *repo == "" || *prNum == "" || *reviewerToken == "" || *llmModel == "" {
fmt.Fprintf(os.Stderr, "Error: missing required flags or environment variables\n\n")
fmt.Fprintf(os.Stderr, "Required: --gitea-url, --repo, --pr, --reviewer-token, --llm-model\n")
fmt.Fprintf(os.Stderr, "Required: --repo, --pr, --reviewer-token, --llm-model\n")
os.Exit(1)
}
// --vcs-url is required only for gitea provider
if *provider == "gitea" && *vcsURL == "" {
fmt.Fprintf(os.Stderr, "Error: --vcs-url (or --gitea-url) is required for provider=gitea\n")
os.Exit(1)
}
if !isAICore && (*llmBaseURL == "" || *llmAPIKey == "") {
@@ -116,8 +136,6 @@ func main() {
os.Exit(1)
}
// NOTE: Persona loading deferred until after Gitea client init to support repo personas
// Validate reviewer-name: only safe characters allowed in sentinel
if err := validateReviewerName(*reviewerName); err != nil {
slog.Error("invalid reviewer name", "error", err)
@@ -139,8 +157,22 @@ func main() {
os.Exit(1)
}
// Initialize clients
giteaClient := gitea.NewClient(*giteaURL, *reviewerToken)
// Initialize VCS client
var client vcs.Client
switch *provider {
case "gitea":
giteaClient := gitea.NewClient(*vcsURL, *reviewerToken)
client = gitea.NewAdapter(giteaClient)
case "github":
ghBaseURL := *baseURL
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.
if ghBaseURL == "" {
ghBaseURL = "https://api.github.com"
}
sonnet-review-bot commented 2026-05-13 08:52:25 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The panic on the default branch of the VCS provider switch (panic("unreachable: unhandled provider " + *provider)) is technically correct since the provider is validated above, but per the repository's convention (Return errors; never panic), this should arguably be an error return or os.Exit. In main() context an os.Exit with an error message would be more consistent with the rest of the validation error handling.

**[NIT]** The panic on the default branch of the VCS provider switch (panic("unreachable: unhandled provider " + *provider)) is technically correct since the provider is validated above, but per the repository's convention (Return errors; never panic), this should arguably be an error return or os.Exit. In main() context an os.Exit with an error message would be more consistent with the rest of the validation error handling.
gpt-review-bot commented 2026-05-13 08:53:50 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] Default branch of the provider switch panics on an unreachable path. Repository convention states 'Return errors; never panic.' Prefer logging an error and exiting with a non-zero status instead of panic, even for unreachable cases.

**[MINOR]** Default branch of the provider switch panics on an unreachable path. Repository convention states 'Return errors; never panic.' Prefer logging an error and exiting with a non-zero status instead of panic, even for unreachable cases.
gpt-review-bot commented 2026-05-13 16:36:00 +00:00
Review
Copy Link
Copy Source

[MINOR] Default case in the VCS provider switch uses panic("unreachable..."). Repository conventions state "never panic"; prefer logging and exiting with a non-zero status to enforce unreachable conditions.

**[MINOR]** Default case in the VCS provider switch uses panic("unreachable..."). Repository conventions state "never panic"; prefer logging and exiting with a non-zero status to enforce unreachable conditions.
client = github.NewClient(*reviewerToken, ghBaseURL)
sonnet-review-bot commented 2026-05-13 08:22:47 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The switch for initializing var client vcs.Client has no default case. Since the provider is validated earlier with an explicit switch+os.Exit(1), client will always be set before use. However, the Go compiler cannot prove this, and if the validation switch ever diverges from the initialization switch (e.g., a new provider is added to validation but not initialization), client would remain nil and panic at first use. A default: panic(...) or a default: os.Exit(1) would make the code more robust.

**[MINOR]** The switch for initializing `var client vcs.Client` has no `default` case. Since the provider is validated earlier with an explicit switch+os.Exit(1), `client` will always be set before use. However, the Go compiler cannot prove this, and if the validation switch ever diverges from the initialization switch (e.g., a new provider is added to validation but not initialization), `client` would remain nil and panic at first use. A `default: panic(...)` or a `default: os.Exit(1)` would make the code more robust.
security-review-bot commented 2026-05-13 11:16:22 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] When initializing the Gitea client, there is no enforcement or warning against using an insecure (HTTP) VCS URL with a token. Unlike the GitHub client which refuses to send credentials over HTTP by default, this path may allow plaintext token transmission depending on gitea.Client behavior. Recommend warning or rejecting non-HTTPS --vcs-url unless explicitly allowed for trusted/local use.

**[MINOR]** When initializing the Gitea client, there is no enforcement or warning against using an insecure (HTTP) VCS URL with a token. Unlike the GitHub client which refuses to send credentials over HTTP by default, this path may allow plaintext token transmission depending on gitea.Client behavior. Recommend warning or rejecting non-HTTPS --vcs-url unless explicitly allowed for trusted/local use.
}
slog.Info("VCS client initialized", "provider", *provider)
sonnet-review-bot commented 2026-05-13 10:55:52 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The default case in the VCS client factory switch (lines ~188-191) sets fmt.Fprintf(os.Stderr, ...) and os.Exit(1). This is unreachable dead code since the same provider values are validated in the earlier switch at line 109-115. It's defensive coding which is fine, but it's worth noting in a comment that this case cannot be reached given the prior validation.

**[NIT]** The `default` case in the VCS client factory switch (lines ~188-191) sets `fmt.Fprintf(os.Stderr, ...)` and `os.Exit(1)`. This is unreachable dead code since the same provider values are validated in the earlier switch at line 109-115. It's defensive coding which is fine, but it's worth noting in a comment that this case cannot be reached given the prior validation.
// Initialize LLM client
security-review-bot commented 2026-05-13 10:31:43 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The --base-url (VCS_BASE_URL) for the GitHub API is fully user-controlled and used with the reviewer token; while HTTPS is enforced and cross-host redirects strip Authorization, an attacker who can influence this configuration could direct requests (and thus tokens) to an arbitrary HTTPS endpoint. Consider allowlisting known GitHub Enterprise hosts or warning when the base URL differs from the default to reduce misconfiguration/exfiltration risk.

**[MINOR]** The --base-url (VCS_BASE_URL) for the GitHub API is fully user-controlled and used with the reviewer token; while HTTPS is enforced and cross-host redirects strip Authorization, an attacker who can influence this configuration could direct requests (and thus tokens) to an arbitrary HTTPS endpoint. Consider allowlisting known GitHub Enterprise hosts or warning when the base URL differs from the default to reduce misconfiguration/exfiltration risk.
sonnet-review-bot commented 2026-05-13 16:34:41 +00:00
Review
Copy Link
Copy Source

[MINOR] The panic("unreachable: ...") in the default case of the VCS client switch violates the project convention "Return errors; never panic" (from CONVENTIONS.md). Although the validation at line 104 does make this branch genuinely unreachable at runtime, the convention is unconditional. A cleaner alternative is slog.Error(...); os.Exit(1) — consistent with every other fatal error path in this file — or the validation could be made to work through a single code path. The panic is acceptable as a defensive measure (and correctly named 'unreachable'), but it deviates from stated conventions.

**[MINOR]** The `panic("unreachable: ...")` in the `default` case of the VCS client switch violates the project convention "Return errors; never panic" (from CONVENTIONS.md). Although the validation at line 104 does make this branch genuinely unreachable at runtime, the convention is unconditional. A cleaner alternative is `slog.Error(...); os.Exit(1)` — consistent with every other fatal error path in this file — or the validation could be made to work through a single code path. The `panic` is acceptable as a defensive measure (and correctly named 'unreachable'), but it deviates from stated conventions.
llmClient := llm.NewClient(*llmBaseURL, *llmAPIKey, *llmModel)
security-review-bot commented 2026-05-13 11:35:31 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The GitHub client uses a user-supplied base URL (--base-url/VCS_BASE_URL). While HTTPS is enforced and redirects strip Authorization, a misconfigured or attacker-controlled base URL could still receive the token over HTTPS. Consider validating/allowlisting expected hosts for production deployments, or requiring an explicit opt-in when using non-default hosts.

**[MINOR]** The GitHub client uses a user-supplied base URL (--base-url/VCS_BASE_URL). While HTTPS is enforced and redirects strip Authorization, a misconfigured or attacker-controlled base URL could still receive the token over HTTPS. Consider validating/allowlisting expected hosts for production deployments, or requiring an explicit opt-in when using non-default hosts.
if *llmTemp < 0 || *llmTemp > 2 {
slog.Error("invalid LLM temperature", "temperature", *llmTemp, "range", "0-2")
5
 @@ -174,16 +206,13 @@ func main() {
ctx, cancel := context.WithTimeout(context.Background(), overallTimeout)
defer cancel()
// Load persona if specified (after Gitea client init to support repo personas)
// Load persona if specified
var persona *review.Persona
if *personaName != "" {
// Try loading from repo first, then fall back to built-in
repoPersonas, err := review.LoadRepoPersonas(ctx, newGiteaClientAdapter(giteaClient), owner, repoName)
repoPersonas, err := review.LoadRepoPersonas(ctx, client, owner, repoName)
if err != nil {
slog.Warn("could not load repo personas", "repo", owner+"/"+repoName, "error", err)
// Continue with built-in personas only.
// NOTE: repoPersonas is nil here, but map indexing on a nil map is safe in Go
// (returns the zero value), so the fallback to built-in below works correctly.
}
if p, ok := repoPersonas[*personaName]; ok {
persona = p
@@ -214,7 +243,7 @@ func main() {
slog.Info("reviewing pull request", "pr", prNumber, "repo", fmt.Sprintf("%s/%s", owner, repoName))
// Step 1: Fetch PR metadata
pr, err := giteaClient.GetPullRequest(ctx, owner, repoName, prNumber)
pr, err := client.GetPullRequest(ctx, owner, repoName, prNumber)
if err != nil {
slog.Error("failed to fetch PR", "pr", prNumber, "error", err)
os.Exit(1)
@@ -222,7 +251,7 @@ func main() {
slog.Info("fetched PR metadata", "pr", prNumber, "title", pr.Title)
// Step 2: Fetch diff
diff, err := giteaClient.GetPullRequestDiff(ctx, owner, repoName, prNumber)
diff, err := client.GetPullRequestDiff(ctx, owner, repoName, prNumber)
if err != nil {
slog.Error("failed to fetch diff", "pr", prNumber, "error", err)
os.Exit(1)
1
 @@ -231,21 +260,21 @@ func main() {
// Step 3: Fetch full file content for modified files
fileContext := ""
files, err := giteaClient.GetPullRequestFiles(ctx, owner, repoName, prNumber)
files, err := client.GetPullRequestFiles(ctx, owner, repoName, prNumber)
if err != nil {
slog.Warn("could not fetch PR files list", "pr", prNumber, "error", err)
} else {
fileContext = fetchFileContext(ctx, giteaClient, owner, repoName, pr.Head.Ref, files)
fileContext = fetchFileContext(ctx, client, owner, repoName, pr.Head.Ref, files)
slog.Debug("fetched file context", "files", len(files))
}
// Step 4: Check CI status
ciPassed := true
ciDetails := ""
if pr.Head.Sha != "" {
statuses, err := giteaClient.GetCommitStatuses(ctx, owner, repoName, pr.Head.Sha)
if pr.Head.SHA != "" {
statuses, err := client.GetCommitStatuses(ctx, owner, repoName, pr.Head.SHA)
if err != nil {
slog.Warn("could not fetch CI status", "sha", pr.Head.Sha, "error", err)
slog.Warn("could not fetch CI status", "sha", pr.Head.SHA, "error", err)
} else {
ciPassed, ciDetails = evaluateCIStatus(statuses)
slog.Info("CI status checked", "passed", ciPassed)
@@ -255,7 +284,7 @@ func main() {
// Step 5: Load conventions file if specified
conventions := ""
if *conventionsFile != "" {
content, err := giteaClient.GetFileContent(ctx, owner, repoName, *conventionsFile)
content, err := client.GetFileContent(ctx, owner, repoName, *conventionsFile, "")
if err != nil {
slog.Warn("could not load conventions file", "file", *conventionsFile, "error", err)
} else {
@@ -267,7 +296,7 @@ func main() {
// Step 6: Load patterns from external repo if specified
patterns := ""
if *patternsRepo != "" {
patterns = fetchPatterns(ctx, giteaClient, *patternsRepo, *patternsFiles)
patterns = fetchPatterns(ctx, client, *patternsRepo, *patternsFiles)
slog.Debug("loaded patterns", "repo", *patternsRepo, "bytes", len(patterns))
}
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 supersedeOldReviews function uses a switch with a fall-through comment for gitea case (// Continue to Gitea-specific logic below the switch.) followed by a type assertion on the concrete *gitea.Adapter. This is idiomatic Go for provider dispatch, but the pattern leaks the concrete gitea type through the vcs.Client abstraction boundary in main.go. The existing comment acknowledges this, but a cleaner alternative would be to add a Supersede(ctx, ...) method to the vcs.Client interface or a separate GiteaSuperseder interface. Not a bug for now since the provider switch validates upfront, but a future decorator wrapping gitea.Adapter would silently break this.

**[MINOR]** The `supersedeOldReviews` function uses a `switch` with a fall-through comment for `gitea` case (`// Continue to Gitea-specific logic below the switch.`) followed by a type assertion on the concrete `*gitea.Adapter`. This is idiomatic Go for provider dispatch, but the pattern leaks the concrete gitea type through the vcs.Client abstraction boundary in main.go. The existing comment acknowledges this, but a cleaner alternative would be to add a `Supersede(ctx, ...)` method to the vcs.Client interface or a separate `GiteaSuperseder` interface. Not a bug for now since the provider switch validates upfront, but a future decorator wrapping gitea.Adapter would silently break this.
@@ -360,15 +389,16 @@ func main() {
}
// Add commit footer so readers know which commit was evaluated
if pr.Head.Sha != "" {
shortSHA := pr.Head.Sha
if pr.Head.SHA != "" {
shortSHA := pr.Head.SHA
if len(shortSHA) > 8 {
shortSHA = shortSHA[:8]
}
reviewBody += fmt.Sprintf("\n\n---\n*Evaluated against %s*", shortSHA)
}
event := review.GiteaEvent(result.Verdict)
// Map verdict to canonical review event
event := verdictToEvent(result.Verdict)
gpt-review-bot commented 2026-05-13 10:41:42 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] In dry-run mode, the event is printed with %s. Ensure vcs.ReviewEvent has an underlying string type or implements fmt.Stringer; otherwise use %v to avoid formatting issues.

**[NIT]** In dry-run mode, the event is printed with %s. Ensure vcs.ReviewEvent has an underlying string type or implements fmt.Stringer; otherwise use %v to avoid formatting issues.
if *dryRun {
fmt.Println("--- DRY RUN ---")
1
 @@ -380,14 +410,13 @@ func main() {
sentinel := fmt.Sprintf("<!-- review-bot:%s -->", *reviewerName)
// Stale check: verify HEAD hasn't moved since we started
evaluatedSHA := pr.Head.Sha
evaluatedSHA := pr.Head.SHA
var currentSHA string
currentPR, err := giteaClient.GetPullRequest(ctx, owner, repoName, prNumber)
currentPR, err := client.GetPullRequest(ctx, owner, repoName, prNumber)
if err != nil {
slog.Warn("could not re-fetch PR for stale check", "pr", prNumber, "error", err)
// currentSHA stays empty — shouldSkipStaleReview will return false
} else {
currentSHA = currentPR.Head.Sha
currentSHA = currentPR.Head.SHA
}
if shouldSkipStaleReview(evaluatedSHA, currentSHA) {
slog.Warn("HEAD moved during review — skipping stale review",
@@ -397,17 +426,24 @@ func main() {
return
}
// Map findings to inline comments for lines present in the diff
diffRanges := gitea.ParseDiffNewLines(diff)
var inlineComments []gitea.ReviewComment
// Build line→position map for inline comments
lineToPosition := vcs.BuildLineToPositionMap(diff)
var inlineComments []vcs.ReviewComment
for _, f := range result.Findings {
if f.File != "" && f.Line > 0 && diffRanges.Contains(f.File, f.Line) {
inlineComments = append(inlineComments, gitea.ReviewComment{
Path: f.File,
NewPosition: int64(f.Line),
Body: fmt.Sprintf("**[%s]** %s", f.Severity, f.Finding),
})
if f.File == "" || f.Line <= 0 {
continue
}
pos, ok := lineToPosition[f.File][f.Line]
if !ok {
slog.Warn("line not in diff, skipping comment", "file", f.File, "line", f.Line)
continue
}
inlineComments = append(inlineComments, vcs.ReviewComment{
Path: f.File,
Position: pos,
CommitID: pr.Head.SHA,
Body: fmt.Sprintf("**[%s]** %s", f.Severity, f.Finding),
})
}
if len(inlineComments) > 0 {
slog.Debug("attaching inline comments", "count", len(inlineComments))
@@ -416,10 +452,9 @@ func main() {
// --- Review update strategy ---
// 1. POST new review first (gets non-stale approval badge on HEAD)
// 2. Then supersede old review with link to the new one
// Order matters: post first so we have the new review's URL for the supersede message.
var oldReviews []gitea.Review
var oldReviews []vcs.Review
if *reviewerName != "" {
existingReviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber)
existingReviews, err := client.ListReviews(ctx, owner, repoName, prNumber)
if err != nil {
slog.Warn("could not list existing reviews", "pr", prNumber, "error", err)
} else {
@@ -431,74 +466,120 @@ func main() {
}
gpt-review-bot commented 2026-05-13 16:36:00 +00:00
Review
Copy Link
Copy Source

[NIT] When a provider does not support review superseding, the code logs an error (slog.Error) even though this is an optional capability. Consider using Warn or Info to reduce noise, since the absence of this optional feature is not fatal.

**[NIT]** When a provider does not support review superseding, the code logs an error (slog.Error) even though this is an optional capability. Consider using Warn or Info to reduce noise, since the absence of this optional feature is not fatal.
}
sonnet-review-bot commented 2026-05-13 10:40:59 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] findOwnReview is defined but no longer called anywhere in main.go after this refactor — findAllOwnReviews replaced it. Dead code should be removed or the function should be unexported with a //nolint if intentionally kept for future use.

**[NIT]** `findOwnReview` is defined but no longer called anywhere in `main.go` after this refactor — `findAllOwnReviews` replaced it. Dead code should be removed or the function should be unexported with a `//nolint` if intentionally kept for future use.
// Self-request as reviewer (ensures we appear in required-reviewer checks)
authUser, err := giteaClient.GetAuthenticatedUser(ctx)
if err != nil {
slog.Warn("could not determine authenticated user for reviewer self-request", "error", err)
} else if authUser != "" {
if err := giteaClient.RequestReviewer(ctx, owner, repoName, prNumber, authUser); err != nil {
slog.Warn("could not self-request as reviewer", "user", authUser, "error", err)
} else {
slog.Debug("self-requested as reviewer", "user", authUser, "pr", prNumber)
// Self-request as reviewer (Gitea-specific; ensures we appear in required-reviewer checks)
sonnet-review-bot commented 2026-05-13 09:24:34 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The supersedeOldReviews switch falls through from case "gitea": (empty body, no break/return) into the post-switch Gitea-specific code. This is correct Go behaviour (no implicit fallthrough), but the pattern is unusual: the switch validates the provider and returns for github, then execution unconditionally continues to the Gitea block below. Adding a brief comment like // fall through to Gitea-specific logic below or restructuring with an explicit default: return fmt.Errorf(...) before the switch would make the control flow clearer and prevent a future developer from accidentally adding code between the switch and the Gitea block.

**[MINOR]** The `supersedeOldReviews` switch falls through from `case "gitea":` (empty body, no break/return) into the post-switch Gitea-specific code. This is correct Go behaviour (no implicit fallthrough), but the pattern is unusual: the `switch` validates the provider and returns for `github`, then execution unconditionally continues to the Gitea block below. Adding a brief comment like `// fall through to Gitea-specific logic below` or restructuring with an explicit `default: return fmt.Errorf(...)` before the switch would make the control flow clearer and prevent a future developer from accidentally adding code between the switch and the Gitea block.
if giteaAdapter, ok := client.(*gitea.Adapter); ok {
authUser, err := client.GetAuthenticatedUser(ctx)
if err != nil {
slog.Warn("could not determine authenticated user for reviewer self-request", "error", err)
} else if authUser != "" {
if err := giteaAdapter.Underlying().RequestReviewer(ctx, owner, repoName, prNumber, authUser); err != nil {
slog.Warn("could not self-request as reviewer", "user", authUser, "error", err)
sonnet-review-bot commented 2026-05-13 09:24:34 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The type assertion client.(*gitea.Adapter) for the Gitea-specific self-request path is a runtime check that could silently no-op if the interface is ever satisfied by a different Gitea implementation. The else branch logs RequestReviewer not supported for provider, skipping which is acceptable, but it would be clearer to guard this block with *provider == "gitea" first (which is already known statically at this point) and then assert. This would also avoid the log message appearing for a hypothetical future second Gitea implementation.

**[NIT]** The type assertion `client.(*gitea.Adapter)` for the Gitea-specific self-request path is a runtime check that could silently no-op if the interface is ever satisfied by a different Gitea implementation. The `else` branch logs `RequestReviewer not supported for provider, skipping` which is acceptable, but it would be clearer to guard this block with `*provider == "gitea"` first (which is already known statically at this point) and then assert. This would also avoid the log message appearing for a hypothetical future second Gitea implementation.
} else {
slog.Debug("self-requested as reviewer", "user", authUser, "pr", prNumber)
}
sonnet-review-bot commented 2026-05-13 10:40:59 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] supersedeOldReviews takes provider and vcsURL as plain strings rather than deriving them from the client value. This means the provider switch inside the function can diverge from the actual client type at runtime — e.g., passing provider="gitea" with a *github.Client would fall through to the type-assertion that expects *gitea.Adapter and return a confusing error. A type switch on client directly (or a provider-specific supersede method on the interface) would be more robust and remove the stringly-typed dispatch.

**[MINOR]** `supersedeOldReviews` takes `provider` and `vcsURL` as plain strings rather than deriving them from the `client` value. This means the provider switch inside the function can diverge from the actual client type at runtime — e.g., passing `provider="gitea"` with a `*github.Client` would fall through to the type-assertion that expects `*gitea.Adapter` and return a confusing error. A type switch on `client` directly (or a provider-specific supersede method on the interface) would be more robust and remove the stringly-typed dispatch.
sonnet-review-bot commented 2026-05-13 10:55:52 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The type assertion client.(*gitea.Adapter) in supersedeOldReviews is fragile. If the gitea provider is ever initialized differently (e.g. a wrapper or decorator), this assertion silently falls through to the error. The function already receives provider string — this is a valid guard, but the assertion is load-bearing for the gitea path and there's no test covering the !ok failure branch. A doc comment noting that the assertion is expected to always succeed given the prior provider switch (enforced by the caller) would make this safer to maintain.

**[MINOR]** The type assertion `client.(*gitea.Adapter)` in `supersedeOldReviews` is fragile. If the gitea provider is ever initialized differently (e.g. a wrapper or decorator), this assertion silently falls through to the error. The function already receives `provider string` — this is a valid guard, but the assertion is load-bearing for the gitea path and there's no test covering the `!ok` failure branch. A doc comment noting that the assertion is expected to always succeed given the prior provider switch (enforced by the caller) would make this safer to maintain.
}
sonnet-review-bot commented 2026-05-13 14:30:36 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The Gitea-specific RequestReviewer self-request uses a type assertion client.(*gitea.Adapter) to detect provider rather than checking *provider == "gitea". Both approaches work, but using the string flag is more consistent with how the rest of the code (e.g., supersedeOldReviews) already branches on provider. The type-assertion approach is also slightly fragile — if gitea.Adapter is ever wrapped in a decorator (as the comment in supersedeOldReviews also notes), this silently stops working.

**[MINOR]** The Gitea-specific `RequestReviewer` self-request uses a type assertion `client.(*gitea.Adapter)` to detect provider rather than checking `*provider == "gitea"`. Both approaches work, but using the string flag is more consistent with how the rest of the code (e.g., `supersedeOldReviews`) already branches on provider. The type-assertion approach is also slightly fragile — if `gitea.Adapter` is ever wrapped in a decorator (as the comment in `supersedeOldReviews` also notes), this silently stops working.
sonnet-review-bot commented 2026-05-13 16:34:41 +00:00
Review
Copy Link
Copy Source

[MINOR] When len(oldReviews) > 0 and the client does not implement vcs.ReviewSuperseder, the code logs an error (slog.Error) but does NOT call os.Exit(1). This silently continues after reporting a failure, which is inconsistent with every other slog.Error call in main() that is followed by os.Exit(1). The new review has already been posted at this point, so the old reviews will remain un-superseded without any caller-visible error. Either add os.Exit(1) or downgrade to slog.Warn if non-fatal is intentional.

**[MINOR]** When `len(oldReviews) > 0` and the client does not implement `vcs.ReviewSuperseder`, the code logs an error (`slog.Error`) but does NOT call `os.Exit(1)`. This silently continues after reporting a failure, which is inconsistent with every other `slog.Error` call in `main()` that is followed by `os.Exit(1)`. The new review has already been posted at this point, so the old reviews will remain un-superseded without any caller-visible error. Either add `os.Exit(1)` or downgrade to `slog.Warn` if non-fatal is intentional.
} else {
slog.Debug("RequestReviewer not supported for provider, skipping")
}
sonnet-review-bot commented 2026-05-13 08:22:47 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] In supersedeOldReviews, for the Gitea path, os.Exit(1) is called inside a non-main function (supersedeOldReviews). This violates the convention of returning errors rather than panicking/exiting from library-style functions. The caller (main) should receive an error and call os.Exit. While this function is in main.go and is unexported, calling os.Exit in nested functions makes testing harder and is an anti-pattern per the repo conventions ('Return errors; never panic').

**[MINOR]** In `supersedeOldReviews`, for the Gitea path, `os.Exit(1)` is called inside a non-main function (`supersedeOldReviews`). This violates the convention of returning errors rather than panicking/exiting from library-style functions. The caller (main) should receive an error and call os.Exit. While this function is in main.go and is unexported, calling os.Exit in nested functions makes testing harder and is an anti-pattern per the repo conventions ('Return errors; never panic').
sonnet-review-bot commented 2026-05-13 08:52:25 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The Gitea-specific self-request reviewer block uses a type assertion (client.(*gitea.Adapter)) which leaks provider-specific behavior into main(). This is the established pattern here since Gitea's RequestReviewer isn't part of the vcs.Client interface, but worth documenting why it's intentional (it is partially documented with the comment). The pattern is acceptable but if more Gitea-specific operations accumulate, it may be worth a GiteaExtension optional interface.

**[MINOR]** The Gitea-specific self-request reviewer block uses a type assertion (client.(*gitea.Adapter)) which leaks provider-specific behavior into main(). This is the established pattern here since Gitea's RequestReviewer isn't part of the vcs.Client interface, but worth documenting why it's intentional (it is partially documented with the comment). The pattern is acceptable but if more Gitea-specific operations accumulate, it may be worth a GiteaExtension optional interface.
// POST new review
slog.Info("posting review", "event", event, "pr", prNumber)
sonnet-review-bot commented 2026-05-13 10:55:52 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The supersedeOldReviews function accepts vcsURL string but for the GitHub case it is never used. For the default case in the outer switch it returns an error, so the parameter is effectively only used in the gitea path. This is fine functionally, but could cause confusion — a comment explaining the parameter is gitea-only would help.

**[MINOR]** The `supersedeOldReviews` function accepts `vcsURL string` but for the GitHub case it is never used. For the `default` case in the outer switch it returns an error, so the parameter is effectively only used in the gitea path. This is fine functionally, but could cause confusion — a comment explaining the parameter is gitea-only would help.
sonnet-review-bot commented 2026-05-13 13:49:34 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The type assertion client.(*gitea.Adapter) in the self-request block (Gitea-specific behavior) imports a concrete provider type into main.go, creating a coupling that the vcs.Client abstraction was designed to avoid. The same pattern recurs in supersedeOldReviews. Consider adding an optional interface (e.g., type SelfRequester interface { SelfRequestReview(...) error }) checked at runtime — following the Optional Interfaces pattern used in net/http — so main.go doesn't need to import gitea for this path. That said, given the Gitea-only nature of the feature and the explicit // Gitea-specific comment, this is a pragmatic trade-off rather than a bug.

**[MINOR]** The type assertion `client.(*gitea.Adapter)` in the self-request block (Gitea-specific behavior) imports a concrete provider type into `main.go`, creating a coupling that the `vcs.Client` abstraction was designed to avoid. The same pattern recurs in `supersedeOldReviews`. Consider adding an optional interface (e.g., `type SelfRequester interface { SelfRequestReview(...) error }`) checked at runtime — following the Optional Interfaces pattern used in `net/http` — so `main.go` doesn't need to import `gitea` for this path. That said, given the Gitea-only nature of the feature and the explicit `// Gitea-specific` comment, this is a pragmatic trade-off rather than a bug.
posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, inlineComments)
reviewReq := vcs.ReviewRequest{
Body: reviewBody,
sonnet-review-bot commented 2026-05-13 10:30:42 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The supersedeOldReviews function accepts provider as a plain string and then type-asserts client.(*gitea.Adapter) for the gitea case. This couples the supersede logic to the concrete Gitea type rather than going through the interface. The type assertion failure path returns an error (good), but the design creates a hidden coupling: adding a new provider that also needs Gitea-style supersede would require modifying this function. Consider whether DismissReview-style behavior should be part of vcs.Client or whether the Gitea-specific path should be documented as intentionally exceptional.

**[MINOR]** The `supersedeOldReviews` function accepts `provider` as a plain string and then type-asserts `client.(*gitea.Adapter)` for the gitea case. This couples the supersede logic to the concrete Gitea type rather than going through the interface. The type assertion failure path returns an error (good), but the design creates a hidden coupling: adding a new provider that also needs Gitea-style supersede would require modifying this function. Consider whether `DismissReview`-style behavior should be part of `vcs.Client` or whether the Gitea-specific path should be documented as intentionally exceptional.
sonnet-review-bot commented 2026-05-13 14:15:32 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] supersedeOldReviews uses os.Exit(1) on error from the caller, but the function itself returns an error — this is correct and the caller handles it. However, the function signature accepts vcsURL unconditionally even for the GitHub path where it is unused. The comment explains this is intentional for API uniformity, which is acceptable, but it could be a source of confusion in future refactors. A more idiomatic approach might be to embed the URL into a Gitea-specific struct or pass it only to the Gitea path. This is a design nit, not a bug.

**[MINOR]** supersedeOldReviews uses os.Exit(1) on error from the caller, but the function itself returns an error — this is correct and the caller handles it. However, the function signature accepts `vcsURL` unconditionally even for the GitHub path where it is unused. The comment explains this is intentional for API uniformity, which is acceptable, but it could be a source of confusion in future refactors. A more idiomatic approach might be to embed the URL into a Gitea-specific struct or pass it only to the Gitea path. This is a design nit, not a bug.
Event: event,
sonnet-review-bot commented 2026-05-13 14:15:32 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The supersedeOldReviews function's Gitea path does a client.(*gitea.Adapter) type assertion after already validating provider == "gitea" via a switch. The comment explains the !ok guard is for future-proofing against decorator wrappers. This is reasonable, but it means the abstraction boundary (vcs.Client) is broken for this function — it requires concrete knowledge of the gitea.Adapter type. A cleaner approach would be to add a SupersedeReview method to vcs.Client or define a narrower provider-specific interface. Not a bug in this PR, but worth noting as technical debt.

**[MINOR]** The `supersedeOldReviews` function's Gitea path does a `client.(*gitea.Adapter)` type assertion after already validating `provider == "gitea"` via a switch. The comment explains the !ok guard is for future-proofing against decorator wrappers. This is reasonable, but it means the abstraction boundary (vcs.Client) is broken for this function — it requires concrete knowledge of the gitea.Adapter type. A cleaner approach would be to add a `SupersedeReview` method to vcs.Client or define a narrower provider-specific interface. Not a bug in this PR, but worth noting as technical debt.
Comments: inlineComments,
sonnet-review-bot commented 2026-05-13 13:45:25 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The self-request reviewer logic uses a type assertion client.(*gitea.Adapter) and then calls both client.GetAuthenticatedUser and giteaAdapter.Underlying().RequestReviewer. The GetAuthenticatedUser call is redundant — it could be called on giteaAdapter directly since Adapter implements Identity. More importantly, this is still provider-specific logic leaked into the main flow via type assertion rather than the provider switch. It would be cleaner to check *provider == "gitea" (which is already available) rather than relying on a type assertion.

**[MINOR]** The self-request reviewer logic uses a type assertion `client.(*gitea.Adapter)` and then calls both `client.GetAuthenticatedUser` and `giteaAdapter.Underlying().RequestReviewer`. The `GetAuthenticatedUser` call is redundant — it could be called on `giteaAdapter` directly since `Adapter` implements `Identity`. More importantly, this is still provider-specific logic leaked into the main flow via type assertion rather than the provider switch. It would be cleaner to check `*provider == "gitea"` (which is already available) rather than relying on a type assertion.
}
posted, err := client.PostReview(ctx, owner, repoName, prNumber, reviewReq)
if err != nil {
slog.Error("failed to post review", "pr", prNumber, "event", event, "error", err)
sonnet-review-bot commented 2026-05-13 15:51:02 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] supersedeOldReviews accepts provider as a plain string rather than a typed constant/enum. The valid provider values are scattered: validated at entry (switch *provider) and re-evaluated here. A typed VCSProvider string type would make the contract explicit and allow the compiler to help catch new provider additions that aren't handled in both switches.

**[MINOR]** supersedeOldReviews accepts provider as a plain string rather than a typed constant/enum. The valid provider values are scattered: validated at entry (switch *provider) and re-evaluated here. A typed VCSProvider string type would make the contract explicit and allow the compiler to help catch new provider additions that aren't handled in both switches.
os.Exit(1)
sonnet-review-bot commented 2026-05-13 14:46:18 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The type assertion client.(*gitea.Adapter) for the self-reviewer path is necessary but creates a direct dependency on a concrete type inside main. The comment explaining this is good. Consider whether RequestReviewer belongs on a provider-specific optional interface (similar to the http.Flusher optional interface pattern) for cleaner future extensibility — e.g., type SelfReviewer interface { RequestReviewerSelf(ctx, owner, repo string, pr int, user string) error }. Not blocking, just worth noting for the next phase.

**[NIT]** The type assertion `client.(*gitea.Adapter)` for the self-reviewer path is necessary but creates a direct dependency on a concrete type inside main. The comment explaining this is good. Consider whether `RequestReviewer` belongs on a provider-specific optional interface (similar to the `http.Flusher` optional interface pattern) for cleaner future extensibility — e.g., `type SelfReviewer interface { RequestReviewerSelf(ctx, owner, repo string, pr int, user string) error }`. Not blocking, just worth noting for the next phase.
}
slog.Info("review posted", "review_id", posted.ID, "user", posted.User.Login, "pr", prNumber)
// Supersede all old reviews with link to the new one
// Supersede all old reviews
sonnet-review-bot commented 2026-05-13 16:04:55 +00:00
Review
Copy Link
Copy Source

[MINOR] When client.(vcs.ReviewSuperseder) returns false and len(oldReviews) > 0, the code logs a warning and silently skips superseding. For the GitHub provider, github.Client implements ReviewSuperseder, so this path is unreachable. For Gitea, gitea.Adapter also implements it. The warning will therefore never fire for any configured provider, making it dead code in practice. It's harmless, but the slog.Warn may mislead operators if a future provider is wired without implementing the interface — they'd see a warning but no error. Consider whether this should be an error for unknown cases.

**[MINOR]** When `client.(vcs.ReviewSuperseder)` returns false and `len(oldReviews) > 0`, the code logs a warning and silently skips superseding. For the GitHub provider, `github.Client` implements `ReviewSuperseder`, so this path is unreachable. For Gitea, `gitea.Adapter` also implements it. The warning will therefore never fire for any configured provider, making it dead code in practice. It's harmless, but the `slog.Warn` may mislead operators if a future provider is wired without implementing the interface — they'd see a warning but no error. Consider whether this should be an error for unknown cases.
if len(oldReviews) > 0 {
newReviewURL := fmt.Sprintf("%s/%s/%s/pulls/%d#pullrequestreview-%d", strings.TrimRight(*giteaURL, "/"), owner, repoName, prNumber, posted.ID)
for _, oldReview := range oldReviews {
cid, err := giteaClient.GetTimelineReviewCommentIDForReview(ctx, owner, repoName, prNumber, oldReview.ID)
if err != nil {
slog.Warn("could not find comment ID for old review", "review_id", oldReview.ID, "error", err)
continue
}
supersededBody := buildSupersededBody(oldReview.Body, oldReview.CommitID, newReviewURL, sentinel)
if err := giteaClient.EditComment(ctx, owner, repoName, cid, supersededBody); err != nil {
slog.Warn("could not mark old review as superseded", "review_id", oldReview.ID, "comment_id", cid, "error", err)
continue
}
slog.Info("marked old review as superseded", "review_id", oldReview.ID, "new_review_id", posted.ID, "pr", prNumber)
supersedeOldReviews(ctx, client, *provider, *vcsURL, owner, repoName, prNumber, oldReviews, posted.ID, sentinel)
}
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.
}
// Resolve old review's inline comments
oldComments, err := giteaClient.ListReviewComments(ctx, owner, repoName, prNumber, oldReview.ID)
if err != nil {
slog.Warn("could not list old review comments for resolution", "review_id", oldReview.ID, "error", err)
continue
}
resolved, failed := 0, 0
for _, c := range oldComments {
if c.ID == 0 {
continue
}
if err := giteaClient.ResolveComment(ctx, owner, repoName, c.ID); err != nil {
slog.Debug("could not resolve inline comment", "comment_id", c.ID, "error", err)
failed++
} else {
resolved++
}
}
if resolved > 0 {
slog.Info("resolved old inline comments", "review_id", oldReview.ID, "count", resolved, "pr", prNumber)
}
if failed > 0 {
slog.Warn("some inline comments could not be resolved", "review_id", oldReview.ID, "failed", failed, "pr", prNumber)
// verdictToEvent maps a verdict string from the LLM response to a canonical vcs.ReviewEvent.
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.
func verdictToEvent(verdict string) vcs.ReviewEvent {
switch verdict {
case "APPROVE":
return vcs.ReviewEventApprove
case "REQUEST_CHANGES":
return vcs.ReviewEventRequestChanges
default:
return vcs.ReviewEventComment
}
}
rodin marked this conversation as resolved Outdated
security-review-bot commented 2026-05-13 11:16:22 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] In supersedeOldReviews (Gitea path), the supersedure body includes a permalink constructed from vcsURL without scheme validation. If misconfigured to a non-HTTPS URL, users may be encouraged to click insecure links. Consider validating or normalizing to HTTPS where applicable or logging a warning.

**[NIT]** In supersedeOldReviews (Gitea path), the supersedure body includes a permalink constructed from vcsURL without scheme validation. If misconfigured to a non-HTTPS URL, users may be encouraged to click insecure links. Consider validating or normalizing to HTTPS where applicable or logging a warning.
sonnet-review-bot commented 2026-05-13 13:45:25 +00:00
Outdated
Review
Copy Link
Copy Source

[MAJOR] In supersedeOldReviews for the GitHub path, errors.Join(errs...) returns a non-nil error even when errs is empty (it returns nil correctly), but the caller does os.Exit(1) on any non-nil error. If all DismissReview calls fail, this correctly exits. However, the bigger issue is that the function signature says it returns an error, but in the Gitea path it returns nil even when individual review supersede operations fail (they only log warnings and continue). This asymmetry means GitHub failures are fatal but Gitea failures are silently swallowed. Either both should be fatal or both should be best-effort — the inconsistency will surprise operators.

**[MAJOR]** In `supersedeOldReviews` for the GitHub path, `errors.Join(errs...)` returns a non-nil error even when `errs` is empty (it returns nil correctly), but the caller does `os.Exit(1)` on any non-nil error. If *all* DismissReview calls fail, this correctly exits. However, the bigger issue is that the function signature says it returns an error, but in the Gitea path it returns `nil` even when individual review supersede operations fail (they only log warnings and `continue`). This asymmetry means GitHub failures are fatal but Gitea failures are silently swallowed. Either both should be fatal or both should be best-effort — the inconsistency will surprise operators.
// supersedeOldReviews marks old reviews as superseded.
// For GitHub: dismisses old reviews.
// For Gitea: edits the review body and resolves inline comments.
func supersedeOldReviews(ctx context.Context, client vcs.Client, provider, vcsURL, owner, repoName string, prNumber int, oldReviews []vcs.Review, newReviewID int64, sentinel string) {
gpt-review-bot commented 2026-05-13 13:22:09 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] supersedeOldReviews requires a concrete type assertion to *gitea.Adapter for Gitea-specific operations. This is acceptable but couples main to a concrete type; consider extending the vcs.Client (or a sub-interface) with provider-agnostic capabilities for superseding/resolve flows to avoid type assertions.

**[MINOR]** supersedeOldReviews requires a concrete type assertion to *gitea.Adapter for Gitea-specific operations. This is acceptable but couples main to a concrete type; consider extending the vcs.Client (or a sub-interface) with provider-agnostic capabilities for superseding/resolve flows to avoid type assertions.
if provider == "github" {
for _, old := range oldReviews {
if err := client.DismissReview(ctx, owner, repoName, prNumber, old.ID, "Superseded by new review"); err != nil {
slog.Warn("failed to dismiss review", "id", old.ID, "error", err)
} else {
slog.Info("dismissed old review", "review_id", old.ID, "new_review_id", newReviewID, "pr", prNumber)
}
}
return
}
sonnet-review-bot commented 2026-05-13 09:37:04 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] supersedeOldReviews uses a switch with a case "gitea": fall-through comment but doesn't actually use the Go fall-through mechanism — it just breaks out of the switch and continues below. This is correct behavior but the comment 'Fall through to Gitea-specific logic below the switch' is slightly misleading since Go switch cases don't fall through by default and no fallthrough keyword is used. The intent is clear but the comment could be reworded to 'continue to Gitea-specific logic below' to avoid confusion.

**[MINOR]** supersedeOldReviews uses a switch with a `case "gitea":` fall-through comment but doesn't actually use the Go fall-through mechanism — it just breaks out of the switch and continues below. This is correct behavior but the comment 'Fall through to Gitea-specific logic below the switch' is slightly misleading since Go switch cases don't fall through by default and no `fallthrough` keyword is used. The intent is clear but the comment could be reworded to 'continue to Gitea-specific logic below' to avoid confusion.
rodin commented 2026-05-13 10:10:20 +00:00
Outdated
Review
Copy Link
Copy Source

Good catch — reworded from "Fall through to Gitea-specific logic below the switch" to "Continue to Gitea-specific logic below the switch" in 7d6fe27. The term "fall through" is misleading in Go context since it implies the fallthrough keyword.

Good catch — reworded from "Fall through to Gitea-specific logic below the switch" to "Continue to Gitea-specific logic below the switch" in 7d6fe27. The term "fall through" is misleading in Go context since it implies the `fallthrough` keyword.
// Gitea: existing EditComment + ResolveComment flow
giteaAdapter, ok := client.(*gitea.Adapter)
if !ok {
sonnet-review-bot commented 2026-05-13 15:51:02 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] supersedeOldReviews performs a type assertion (client.(*gitea.Adapter)) that creates a layering violation: the cmd package now depends on the concrete gitea.Adapter type for provider-specific logic. The comment acknowledges the brittleness ('guards against future refactors') but a cleaner approach would be to add a Gitea-specific optional interface (e.g. vcs.GiteaSuperseder) similar to how ReviewerSelfRequester is handled, avoiding the concrete type assertion entirely.

**[MINOR]** supersedeOldReviews performs a type assertion (client.(*gitea.Adapter)) that creates a layering violation: the cmd package now depends on the concrete gitea.Adapter type for provider-specific logic. The comment acknowledges the brittleness ('guards against future refactors') but a cleaner approach would be to add a Gitea-specific optional interface (e.g. vcs.GiteaSuperseder) similar to how ReviewerSelfRequester is handled, avoiding the concrete type assertion entirely.
slog.Error("expected gitea.Adapter for gitea provider")
os.Exit(1)
}
sonnet-review-bot commented 2026-05-13 08:52:25 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] supersedeOldReviews uses a string-based provider switch ('github' / else-Gitea) rather than a typed constant or enum. The provider value is passed as a plain string through multiple function calls. If a third provider is added later, it's easy to miss this function. Consider using the same switch structure as the client factory (case 'gitea': / case 'github': / default: return fmt.Errorf) to make exhaustiveness explicit.

**[MINOR]** supersedeOldReviews uses a string-based provider switch ('github' / else-Gitea) rather than a typed constant or enum. The provider value is passed as a plain string through multiple function calls. If a third provider is added later, it's easy to miss this function. Consider using the same switch structure as the client factory (case 'gitea': / case 'github': / default: return fmt.Errorf) to make exhaustiveness explicit.
underlying := giteaAdapter.Underlying()
rodin marked this conversation as resolved Outdated
sonnet-review-bot commented 2026-05-13 11:15:28 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The supersedeOldReviews function has a duplicate doc comment: the first line says 'supersedeOldReviews marks old reviews as superseded.' and the second line repeats 'supersedeOldReviews marks prior reviews as superseded so only the latest review is visible.' This is a copy-paste artifact that should be cleaned up.

**[MINOR]** The `supersedeOldReviews` function has a duplicate doc comment: the first line says 'supersedeOldReviews marks old reviews as superseded.' and the second line repeats 'supersedeOldReviews marks prior reviews as superseded so only the latest review is visible.' This is a copy-paste artifact that should be cleaned up.
newReviewURL := fmt.Sprintf("%s/%s/%s/pulls/%d#pullrequestreview-%d", strings.TrimRight(vcsURL, "/"), owner, repoName, prNumber, newReviewID)
for _, oldReview := range oldReviews {
cid, err := underlying.GetTimelineReviewCommentIDForReview(ctx, owner, repoName, prNumber, oldReview.ID)
if err != nil {
slog.Warn("could not find comment ID for old review", "review_id", oldReview.ID, "error", err)
gpt-review-bot commented 2026-05-13 13:51:45 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] Provider-specific behavior is implemented via a concrete type assertion to *gitea.Adapter in supersedeOldReviews. This is acceptable, but consider an optional capability interface (e.g., an optional interface on vcs.Client) to avoid tight coupling and make future wrappers/decorators simpler.

**[MINOR]** Provider-specific behavior is implemented via a concrete type assertion to *gitea.Adapter in supersedeOldReviews. This is acceptable, but consider an optional capability interface (e.g., an optional interface on vcs.Client) to avoid tight coupling and make future wrappers/decorators simpler.
continue
}
supersededBody := buildSupersededBody(oldReview.Body, oldReview.CommitID, newReviewURL, sentinel)
if err := underlying.EditComment(ctx, owner, repoName, cid, supersededBody); err != nil {
gpt-review-bot commented 2026-05-13 15:52:47 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] supersedeOldReviews asserts client.(*gitea.Adapter), coupling main to the gitea package. Consider an optional interface (e.g., a GiteaSuperseder) to avoid a concrete type assertion and keep main fully provider-agnostic.

**[MINOR]** supersedeOldReviews asserts client.(*gitea.Adapter), coupling main to the gitea package. Consider an optional interface (e.g., a GiteaSuperseder) to avoid a concrete type assertion and keep main fully provider-agnostic.
slog.Warn("could not mark old review as superseded", "review_id", oldReview.ID, "comment_id", cid, "error", err)
continue
}
slog.Info("marked old review as superseded", "review_id", oldReview.ID, "new_review_id", newReviewID, "pr", prNumber)
// Resolve old review's inline comments
rodin marked this conversation as resolved Outdated
sonnet-review-bot commented 2026-05-13 11:15:28 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] There is a double blank line between the closing brace of the github case and the Gitea-specific code (after case "gitea": // Continue to Gitea-specific logic below the switch.). Minor formatting issue that gofmt would catch — verify this was intentional.

**[MINOR]** There is a double blank line between the closing brace of the `github` case and the Gitea-specific code (after `case "gitea": // Continue to Gitea-specific logic below the switch.`). Minor formatting issue that `gofmt` would catch — verify this was intentional.
oldComments, err := underlying.ListReviewComments(ctx, owner, repoName, prNumber, oldReview.ID)
if err != nil {
rodin marked this conversation as resolved Outdated
sonnet-review-bot commented 2026-05-13 11:15:28 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The type assertion client.(*gitea.Adapter) inside supersedeOldReviews couples this package-level function to the gitea package's internal concrete type. The comment acknowledges this is intentional and guarded, but if a decorator/wrapper is ever added around the adapter, this silently breaks. A cleaner long-term design would be to add a Gitea() GiteaUnderlying interface method or pass underlying directly rather than asserting. This is acceptable for now given the comment explains the rationale, but worth tracking.

**[MINOR]** The type assertion `client.(*gitea.Adapter)` inside `supersedeOldReviews` couples this package-level function to the `gitea` package's internal concrete type. The comment acknowledges this is intentional and guarded, but if a decorator/wrapper is ever added around the adapter, this silently breaks. A cleaner long-term design would be to add a `Gitea() GiteaUnderlying` interface method or pass `underlying` directly rather than asserting. This is acceptable for now given the comment explains the rationale, but worth tracking.
slog.Warn("could not list old review comments for resolution", "review_id", oldReview.ID, "error", err)
sonnet-review-bot commented 2026-05-13 15:02:31 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] supersedeOldReviews uses a provider string parameter and a switch that ultimately does a type assertion (client.(*gitea.Adapter)) for the Gitea path. This leaks a concrete type from the gitea package into what is otherwise a provider-agnostic function, and creates a tight coupling that will silently break if the adapter is ever wrapped (as the comment acknowledges). A cleaner design would be an optional interface (e.g., vcs.OldReviewSuperseder) that the Gitea adapter implements, similar to how ReviewerSelfRequester was handled, eliminating both the string switch and the type assertion.

**[MINOR]** `supersedeOldReviews` uses a provider string parameter and a `switch` that ultimately does a type assertion (`client.(*gitea.Adapter)`) for the Gitea path. This leaks a concrete type from the `gitea` package into what is otherwise a provider-agnostic function, and creates a tight coupling that will silently break if the adapter is ever wrapped (as the comment acknowledges). A cleaner design would be an optional interface (e.g., `vcs.OldReviewSuperseder`) that the Gitea adapter implements, similar to how `ReviewerSelfRequester` was handled, eliminating both the string switch and the type assertion.
sonnet-review-bot commented 2026-05-13 15:02:31 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The vcsURL parameter is accepted by supersedeOldReviews even for the GitHub path (where it is unused) with the stated goal of keeping the signature uniform. However this means the GitHub path silently ignores a potentially user-visible parameter with no validation. If vcsURL is an empty string and the provider is github, the Gitea URL-scheme validation block is never reached, which is fine — but a caller could accidentally pass a bad vcsURL for GitHub and receive no diagnostic. A doc comment clarifying this intentional skip would help future readers.

**[MINOR]** The `vcsURL` parameter is accepted by `supersedeOldReviews` even for the GitHub path (where it is unused) with the stated goal of keeping the signature uniform. However this means the GitHub path silently ignores a potentially user-visible parameter with no validation. If `vcsURL` is an empty string and the provider is `github`, the Gitea URL-scheme validation block is never reached, which is fine — but a caller could accidentally pass a bad vcsURL for GitHub and receive no diagnostic. A doc comment clarifying this intentional skip would help future readers.
continue
}
resolved, failed := 0, 0
for _, c := range oldComments {
if c.ID == 0 {
continue
}
if err := underlying.ResolveComment(ctx, owner, repoName, c.ID); err != nil {
slog.Debug("could not resolve inline comment", "comment_id", c.ID, "error", err)
failed++
} else {
resolved++
}
}
if resolved > 0 {
slog.Info("resolved old inline comments", "review_id", oldReview.ID, "count", resolved, "pr", prNumber)
}
security-review-bot commented 2026-05-13 14:47:46 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] When constructing the supersede link for Gitea reviews, vcsURL is concatenated without scheme validation. Although this value is operator-provided and Gitea typically sanitizes Markdown links, validating that vcsURL starts with http/https before embedding would reduce the risk of an unsafe link (e.g., javascript:) if misconfigured.

**[MINOR]** When constructing the supersede link for Gitea reviews, vcsURL is concatenated without scheme validation. Although this value is operator-provided and Gitea typically sanitizes Markdown links, validating that vcsURL starts with http/https before embedding would reduce the risk of an unsafe link (e.g., javascript:) if misconfigured.
if failed > 0 {
slog.Warn("some inline comments could not be resolved", "review_id", oldReview.ID, "failed", failed, "pr", prNumber)
sonnet-review-bot commented 2026-05-13 14:46:18 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] supersedeOldReviews has a vcsURL parameter that is documented as 'accepted unconditionally to keep the function signature uniform across providers' — but it is only used in the Gitea branch. While the comment explains the reasoning, this creates a leaky abstraction: callers must always supply a Gitea-specific URL even for GitHub. A cleaner approach would be to pass the URL only when needed (e.g. via a config struct or only for the Gitea case), or accept that the function is not truly provider-agnostic and name/document it accordingly. This is a minor design issue that could cause confusion for future maintainers.

**[MINOR]** supersedeOldReviews has a `vcsURL` parameter that is documented as 'accepted unconditionally to keep the function signature uniform across providers' — but it is only used in the Gitea branch. While the comment explains the reasoning, this creates a leaky abstraction: callers must always supply a Gitea-specific URL even for GitHub. A cleaner approach would be to pass the URL only when needed (e.g. via a config struct or only for the Gitea case), or accept that the function is not truly provider-agnostic and name/document it accordingly. This is a minor design issue that could cause confusion for future maintainers.
}
}
}
// fetchFileContext fetches the full content of modified files from the PR branch.
func fetchFileContext(ctx context.Context, client *gitea.Client, owner, repo, ref string, files []gitea.ChangedFile) string {
func fetchFileContext(ctx context.Context, client vcs.PRReader, owner, repo, ref string, files []vcs.ChangedFile) string {
var sb strings.Builder
for _, f := range files {
if ctx.Err() != nil {
1
 @@ -507,7 +588,7 @@ func fetchFileContext(ctx context.Context, client *gitea.Client, owner, repo, re
if f.Status == "removed" {
continue // Skip deleted files
}
content, err := client.GetFileContentRef(ctx, owner, repo, f.Filename, ref)
content, err := client.GetFileContentAtRef(ctx, owner, repo, f.Filename, ref)
if err != nil {
slog.Warn("could not fetch file content", "file", f.Filename, "error", err)
continue
gpt-review-bot commented 2026-05-13 14:16:39 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The provider-specific gitea path relies on a concrete type assertion to *gitea.Adapter. While guarded, consider exposing a minimal capability interface (e.g., an underlying accessor) on vcs.Client to avoid concrete type checks and keep the abstraction boundary cleaner.

**[MINOR]** The provider-specific gitea path relies on a concrete type assertion to *gitea.Adapter. While guarded, consider exposing a minimal capability interface (e.g., an underlying accessor) on vcs.Client to avoid concrete type checks and keep the abstraction boundary cleaner.
@@ -524,7 +605,8 @@ func fetchFileContext(ctx context.Context, client *gitea.Client, owner, repo, re
// patternsRepo is comma-separated list of owner/name repos.
// patternsFiles is comma-separated list of file paths or directories.
// If a path ends with / or is a directory, all files within it are fetched recursively.
func fetchPatterns(ctx context.Context, client *gitea.Client, patternsRepo, patternsFiles string) string {
// If patternsFiles is empty, all files from the repo root are fetched.
func fetchPatterns(ctx context.Context, client vcs.FileReader, patternsRepo, patternsFiles string) string {
var sb strings.Builder
repos := strings.Split(patternsRepo, ",")
1
 @@ -554,7 +636,7 @@ func fetchPatterns(ctx context.Context, client *gitea.Client, patternsRepo, patt
continue
security-review-bot commented 2026-05-13 15:03:23 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] fetchPatterns now treats an empty --patterns-files as 'fetch all files from repo root' and recursively aggregates all markdown/text/yaml files without size or count limits. An attacker-controlled patterns repo (or misconfiguration) could cause excessive memory/CPU usage and inflate prompt size, enabling a denial-of-service condition.

**[MINOR]** fetchPatterns now treats an empty --patterns-files as 'fetch all files from repo root' and recursively aggregates all markdown/text/yaml files without size or count limits. An attacker-controlled patterns repo (or misconfiguration) could cause excessive memory/CPU usage and inflate prompt size, enabling a denial-of-service condition.
}
files, err := client.GetAllFilesInPath(ctx, owner, repo, path)
files, err := vcs.GetAllFilesInPath(ctx, client, owner, repo, path)
if err != nil {
slog.Warn("could not fetch patterns", "path", path, "repo", repoRef, "error", err)
continue
@@ -593,7 +675,7 @@ func isPatternFile(path string) bool {
}
// evaluateCIStatus checks if all CI statuses indicate success.
func evaluateCIStatus(statuses []gitea.CommitStatus) (passed bool, details string) {
func evaluateCIStatus(statuses []vcs.CommitStatus) (passed bool, details string) {
if len(statuses) == 0 {
return true, "no CI statuses found"
}
1
 @@ -728,10 +810,10 @@ func buildSupersededBody(originalBody, commitSHA, newReviewURL, sentinel string)
}
// hasSharedToken detects if another review-bot role posted under the same
// Gitea user. This indicates misconfiguration where two roles share a token
// instead of having separate Gitea accounts. Returns true if shared token
// VCS user. This indicates misconfiguration where two roles share a token
// instead of having separate accounts. Returns true if shared token
// detected (caller should skip update-in-place logic to avoid clobbering).
func hasSharedToken(reviews []gitea.Review, ownSentinel string) bool {
func hasSharedToken(reviews []vcs.Review, ownSentinel string) bool {
ownLogin := ""
for _, r := range reviews {
if strings.Contains(r.Body, ownSentinel) {
@@ -744,7 +826,7 @@ func hasSharedToken(reviews []gitea.Review, ownSentinel string) bool {
}
security-review-bot marked this conversation as resolved Outdated
security-review-bot commented 2026-05-13 08:23:53 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] Potential log injection: extractSentinelName() returns untrusted content from review bodies, which is logged as "sibling_role" without escaping. An attacker-controlled sentinel (e.g., with newlines) could inject extra log lines when using text logging. Consider sanitizing/escaping newlines and control characters before logging or enforcing a stricter pattern when extracting the sentinel.

**[MINOR]** Potential log injection: extractSentinelName() returns untrusted content from review bodies, which is logged as "sibling_role" without escaping. An attacker-controlled sentinel (e.g., with newlines) could inject extra log lines when using text logging. Consider sanitizing/escaping newlines and control characters before logging or enforcing a stricter pattern when extracting the sentinel.
for _, r := range reviews {
if r.User.Login == ownLogin && strings.Contains(r.Body, "<!-- review-bot:") && !strings.Contains(r.Body, ownSentinel) {
slog.Warn("shared token detected — another review-bot role is using the same Gitea user",
slog.Warn("shared token detected — another review-bot role is using the same VCS user",
"sibling_role", extractSentinelName(r.Body), "user", ownLogin)
return true
}
@@ -769,8 +851,8 @@ func extractSentinelName(body string) string {
}
// findOwnReview locates the most recent non-superseded review matching the sentinel.
func findOwnReview(reviews []gitea.Review, sentinel string) *gitea.Review {
var best *gitea.Review
func findOwnReview(reviews []vcs.Review, sentinel string) *vcs.Review {
var best *vcs.Review
for i := range reviews {
if !strings.Contains(reviews[i].Body, sentinel) {
continue
@@ -786,8 +868,8 @@ func findOwnReview(reviews []gitea.Review, sentinel string) *gitea.Review {
}
// findAllOwnReviews returns all non-superseded reviews matching the sentinel.
func findAllOwnReviews(reviews []gitea.Review, sentinel string) []gitea.Review {
var result []gitea.Review
func findAllOwnReviews(reviews []vcs.Review, sentinel string) []vcs.Review {
var result []vcs.Review
for i := range reviews {
if !strings.Contains(reviews[i].Body, sentinel) {
continue
1
 @@ -812,35 +894,3 @@ func shouldSkipStaleReview(evaluatedSHA, currentSHA string) bool {
}
return evaluatedSHA != currentSHA
}
// giteaClientAdapter adapts gitea.Client to vcs.FileReader interface.
type giteaClientAdapter struct {
client *gitea.Client
}
func newGiteaClientAdapter(c *gitea.Client) *giteaClientAdapter {
return &giteaClientAdapter{client: c}
}
func (a *giteaClientAdapter) ListContents(ctx context.Context, owner, repo, path string) ([]vcs.ContentEntry, error) {
entries, err := a.client.ListContents(ctx, owner, repo, path)
if err != nil {
return nil, err
}
result := make([]vcs.ContentEntry, len(entries))
for i, e := range entries {
result[i] = vcs.ContentEntry{
Name: e.Name,
Path: e.Path,
Type: e.Type,
}
}
return result, nil
}
func (a *giteaClientAdapter) GetFileContent(ctx context.Context, owner, repo, filePath, ref string) (string, error) {
if ref != "" {
return a.client.GetFileContentRef(ctx, owner, repo, filePath, ref)
}
return a.client.GetFileContent(ctx, owner, repo, filePath)
}
82
cmd/review-bot/main_test.go
+93 -47
View File
@@ -10,7 +10,7 @@ import (
"strings"
"testing"
"gitea.weiker.me/rodin/review-bot/gitea"
"gitea.weiker.me/rodin/review-bot/vcs"
)
func TestValidateReviewerName(t *testing.T) {
@@ -107,9 +107,7 @@ func TestValidateWorkspacePath(t *testing.T) {
workspace: tmpDir,
path: "/etc/passwd",
wantErr: true,
// Go 1.21+ filepath.Join normalizes absolute paths: Join("/tmp/x", "/etc/passwd")
// becomes "/tmp/x/etc/passwd", which is within workspace but doesn't exist.
errMatch: "failed to resolve",
errMatch: "failed to resolve",
},
{
name: "nonexistent file",
@@ -154,15 +152,14 @@ func TestValidateWorkspacePath(t *testing.T) {
}
}
func makeReview(id int64, login, state string, stale bool, body string) gitea.Review {
r := gitea.Review{
func makeReview(id int64, login, state string, stale bool, body string) vcs.Review {
return vcs.Review{
ID: id,
Body: body,
User: vcs.UserInfo{Login: login},
State: state,
Stale: stale,
}
r.User.Login = login
return r
}
func TestBuildSupersededBody(t *testing.T) {
@@ -216,7 +213,7 @@ func TestBuildSupersededBodyShortSHA(t *testing.T) {
func TestFindOwnReview(t *testing.T) {
tests := []struct {
name string
reviews []gitea.Review
reviews []vcs.Review
sentinel string
wantID int64
wantNil bool
@@ -229,7 +226,7 @@ func TestFindOwnReview(t *testing.T) {
},
{
name: "found by sentinel",
reviews: []gitea.Review{
reviews: []vcs.Review{
makeReview(42, "bot", "APPROVED", false, "review body\n<!-- review-bot:sonnet -->"),
},
sentinel: "<!-- review-bot:sonnet -->",
@@ -237,7 +234,7 @@ func TestFindOwnReview(t *testing.T) {
},
{
name: "wrong sentinel",
reviews: []gitea.Review{
reviews: []vcs.Review{
makeReview(42, "bot", "APPROVED", false, "body\n<!-- review-bot:gpt -->"),
},
sentinel: "<!-- review-bot:sonnet -->",
@@ -245,7 +242,7 @@ func TestFindOwnReview(t *testing.T) {
},
{
name: "multiple reviews, returns first match",
reviews: []gitea.Review{
reviews: []vcs.Review{
makeReview(10, "bot", "APPROVED", false, "old\n<!-- review-bot:gpt -->"),
makeReview(20, "bot", "APPROVED", false, "new\n<!-- review-bot:sonnet -->"),
},
@@ -254,7 +251,7 @@ func TestFindOwnReview(t *testing.T) {
},
{
name: "skips superseded review",
reviews: []gitea.Review{
reviews: []vcs.Review{
makeReview(10, "bot", "APPROVED", false, "~~Original review~~\n\n**Superseded**\n<!-- review-bot:sonnet -->"),
makeReview(20, "bot", "APPROVED", false, "fresh review\n<!-- review-bot:sonnet -->"),
},
@@ -263,7 +260,7 @@ func TestFindOwnReview(t *testing.T) {
},
{
name: "only superseded reviews exist",
reviews: []gitea.Review{
reviews: []vcs.Review{
makeReview(10, "bot", "APPROVED", false, "~~Original review~~\n\n<!-- review-bot:sonnet -->"),
},
sentinel: "<!-- review-bot:sonnet -->",
@@ -271,7 +268,7 @@ func TestFindOwnReview(t *testing.T) {
},
{
name: "picks highest ID among matches",
reviews: []gitea.Review{
reviews: []vcs.Review{
makeReview(50, "bot", "APPROVED", false, "v1\n<!-- review-bot:sonnet -->"),
makeReview(30, "bot", "APPROVED", false, "v0\n<!-- review-bot:sonnet -->"),
},
@@ -302,7 +299,7 @@ func TestFindOwnReview(t *testing.T) {
func TestHasSharedToken(t *testing.T) {
tests := []struct {
name string
reviews []gitea.Review
reviews []vcs.Review
sentinel string
want bool
}{
@@ -314,36 +311,36 @@ func TestHasSharedToken(t *testing.T) {
},
{
name: "no own review yet - cannot detect",
reviews: []gitea.Review{
{ID: 1, User: struct{ Login string `json:"login"` }{Login: "other"}, Body: "<!-- review-bot:gpt --> body"},
reviews: []vcs.Review{
makeReview(1, "other", "APPROVED", false, "<!-- review-bot:gpt --> body"),
},
sentinel: "<!-- review-bot:sonnet -->",
want: false,
},
{
name: "separate users - no shared token",
reviews: []gitea.Review{
{ID: 1, User: struct{ Login string `json:"login"` }{Login: "sonnet-review-bot"}, Body: "<!-- review-bot:sonnet --> body"},
{ID: 2, User: struct{ Login string `json:"login"` }{Login: "security-review-bot"}, Body: "<!-- review-bot:security --> body"},
reviews: []vcs.Review{
makeReview(1, "sonnet-review-bot", "APPROVED", false, "<!-- review-bot:sonnet --> body"),
makeReview(2, "security-review-bot", "APPROVED", false, "<!-- review-bot:security --> body"),
},
sentinel: "<!-- review-bot:sonnet -->",
want: false,
},
{
name: "shared token detected - same user different sentinels",
reviews: []gitea.Review{
{ID: 1, User: struct{ Login string `json:"login"` }{Login: "sonnet-review-bot"}, Body: "<!-- review-bot:sonnet --> body"},
{ID: 2, User: struct{ Login string `json:"login"` }{Login: "sonnet-review-bot"}, Body: "<!-- review-bot:security --> body"},
reviews: []vcs.Review{
makeReview(1, "sonnet-review-bot", "APPROVED", false, "<!-- review-bot:sonnet --> body"),
makeReview(2, "sonnet-review-bot", "APPROVED", false, "<!-- review-bot:security --> body"),
},
sentinel: "<!-- review-bot:sonnet -->",
want: true,
},
{
name: "three roles same user",
reviews: []gitea.Review{
{ID: 1, User: struct{ Login string `json:"login"` }{Login: "bot"}, Body: "<!-- review-bot:sonnet --> body"},
{ID: 2, User: struct{ Login string `json:"login"` }{Login: "bot"}, Body: "<!-- review-bot:security --> body"},
{ID: 3, User: struct{ Login string `json:"login"` }{Login: "bot"}, Body: "<!-- review-bot:gpt --> body"},
reviews: []vcs.Review{
makeReview(1, "bot", "APPROVED", false, "<!-- review-bot:sonnet --> body"),
makeReview(2, "bot", "APPROVED", false, "<!-- review-bot:security --> body"),
makeReview(3, "bot", "APPROVED", false, "<!-- review-bot:gpt --> body"),
},
sentinel: "<!-- review-bot:sonnet -->",
want: true,
2
 @@ -507,7 +504,7 @@ func TestIsPatternFile(t *testing.T) {
func TestEvaluateCIStatus(t *testing.T) {
tests := []struct {
name string
statuses []gitea.CommitStatus
statuses []vcs.CommitStatus
wantPassed bool
wantSubstr string
}{
@@ -519,7 +516,7 @@ func TestEvaluateCIStatus(t *testing.T) {
},
{
name: "all success",
statuses: []gitea.CommitStatus{
statuses: []vcs.CommitStatus{
{Status: "success", Context: "ci/build", Description: "Build passed"},
{Status: "success", Context: "ci/test", Description: "Tests passed"},
},
@@ -528,7 +525,7 @@ func TestEvaluateCIStatus(t *testing.T) {
},
{
name: "one failure",
statuses: []gitea.CommitStatus{
statuses: []vcs.CommitStatus{
{Status: "success", Context: "ci/build", Description: "Build passed"},
{Status: "failure", Context: "ci/test", Description: "Tests failed"},
},
@@ -537,7 +534,7 @@ func TestEvaluateCIStatus(t *testing.T) {
},
{
name: "error status",
statuses: []gitea.CommitStatus{
statuses: []vcs.CommitStatus{
{Status: "error", Context: "ci/lint", Description: "Lint error"},
},
wantPassed: false,
@@ -545,7 +542,7 @@ func TestEvaluateCIStatus(t *testing.T) {
},
{
name: "pending treated as not-failed",
statuses: []gitea.CommitStatus{
statuses: []vcs.CommitStatus{
{Status: "pending", Context: "ci/build", Description: "In progress"},
{Status: "success", Context: "ci/test", Description: "Tests passed"},
},
@@ -554,7 +551,7 @@ func TestEvaluateCIStatus(t *testing.T) {
},
{
name: "multiple failures",
statuses: []gitea.CommitStatus{
statuses: []vcs.CommitStatus{
{Status: "failure", Context: "ci/build", Description: "Build failed"},
{Status: "failure", Context: "ci/test", Description: "Tests failed"},
},
@@ -563,7 +560,7 @@ func TestEvaluateCIStatus(t *testing.T) {
},
{
name: "mixed with pending and failure",
statuses: []gitea.CommitStatus{
statuses: []vcs.CommitStatus{
{Status: "success", Context: "ci/build", Description: "Build passed"},
{Status: "pending", Context: "ci/deploy", Description: "Deploying"},
{Status: "failure", Context: "ci/test", Description: "Tests failed"},
@@ -792,7 +789,7 @@ func TestMainSubprocess_InvalidReviewerName(t *testing.T) {
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
os.Args = []string{"review-bot",
"--gitea-url", "http://localhost",
"--vcs-url", "http://localhost",
"--repo", "owner/repo",
"--pr", "1",
"--reviewer-name", "invalid name",
@@ -820,7 +817,7 @@ func TestMainSubprocess_InvalidRepo(t *testing.T) {
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
os.Args = []string{"review-bot",
"--gitea-url", "http://localhost",
"--vcs-url", "http://localhost",
"--repo", "invalidrepo",
"--pr", "1",
"--reviewer-token", "tok",
@@ -847,7 +844,7 @@ func TestMainSubprocess_InvalidPRNumber(t *testing.T) {
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
os.Args = []string{"review-bot",
"--gitea-url", "http://localhost",
"--vcs-url", "http://localhost",
"--repo", "owner/repo",
"--pr", "notanumber",
"--reviewer-token", "tok",
@@ -874,7 +871,7 @@ func TestMainSubprocess_InvalidTemperature(t *testing.T) {
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
os.Args = []string{"review-bot",
"--gitea-url", "http://localhost",
"--vcs-url", "http://localhost",
"--repo", "owner/repo",
"--pr", "1",
"--reviewer-token", "tok",
@@ -902,7 +899,7 @@ func TestMainSubprocess_InvalidProvider(t *testing.T) {
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
os.Args = []string{"review-bot",
"--gitea-url", "http://localhost",
"--vcs-url", "http://localhost",
"--repo", "owner/repo",
"--pr", "1",
"--reviewer-token", "tok",
@@ -926,7 +923,35 @@ func TestMainSubprocess_InvalidProvider(t *testing.T) {
}
}
// cleanEnv returns environ without any GITEA/LLM/REVIEWER env vars that would
func TestMainSubprocess_InvalidVCSProvider(t *testing.T) {
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
os.Args = []string{"review-bot",
"--provider", "invalid",
"--vcs-url", "http://localhost",
"--repo", "owner/repo",
"--pr", "1",
"--reviewer-token", "tok",
"--llm-base-url", "http://localhost",
"--llm-api-key", "key",
"--llm-model", "model",
}
main()
return
}
cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_InvalidVCSProvider")
cmd.Env = append(cleanEnv(), "TEST_SUBPROCESS_MAIN=1")
out, err := cmd.CombinedOutput()
if err == nil {
t.Fatal("expected non-zero exit with invalid VCS provider")
}
if !strings.Contains(string(out), "invalid --provider") {
t.Errorf("expected error about invalid --provider, got: %s", out)
}
}
// cleanEnv returns environ without any GITEA/LLM/REVIEWER/VCS env vars that would
// interfere with testing missing-flag scenarios.
func cleanEnv() []string {
var env []string
@@ -934,6 +959,7 @@ func cleanEnv() []string {
key := strings.SplitN(e, "=", 2)[0]
switch {
case strings.HasPrefix(key, "GITEA_"),
strings.HasPrefix(key, "VCS_"),
strings.HasPrefix(key, "LLM_"),
strings.HasPrefix(key, "REVIEWER_"),
strings.HasPrefix(key, "PR_"),
@@ -951,12 +977,12 @@ func cleanEnv() []string {
}
func TestFindAllOwnReviews(t *testing.T) {
reviews := []gitea.Review{
{ID: 1, Body: "<!-- review-bot:sonnet -->\nfirst review"},
{ID: 2, Body: "<!-- review-bot:gpt -->\nother bot"},
{ID: 3, Body: "<!-- review-bot:sonnet -->\nsecond review"},
{ID: 4, Body: "~~Original review~~\n<!-- review-bot:sonnet -->\nsuperseded"},
{ID: 5, Body: "<!-- review-bot:sonnet -->\nthird review"},
reviews := []vcs.Review{
makeReview(1, "bot", "APPROVED", false, "<!-- review-bot:sonnet -->\nfirst review"),
makeReview(2, "bot", "APPROVED", false, "<!-- review-bot:gpt -->\nother bot"),
makeReview(3, "bot", "APPROVED", false, "<!-- review-bot:sonnet -->\nsecond review"),
makeReview(4, "bot", "APPROVED", false, "~~Original review~~\n<!-- review-bot:sonnet -->\nsuperseded"),
makeReview(5, "bot", "APPROVED", false, "<!-- review-bot:sonnet -->\nthird review"),
}
got := findAllOwnReviews(reviews, "<!-- review-bot:sonnet -->")
@@ -1020,3 +1046,23 @@ func TestShouldSkipStaleReview(t *testing.T) {
})
}
}
func TestVerdictToEvent(t *testing.T) {
tests := []struct {
verdict string
want vcs.ReviewEvent
}{
{"APPROVE", vcs.ReviewEventApprove},
{"REQUEST_CHANGES", vcs.ReviewEventRequestChanges},
{"COMMENT", vcs.ReviewEventComment},
{"other", vcs.ReviewEventComment},
{"", vcs.ReviewEventComment},
}
for _, tc := range tests {
got := verdictToEvent(tc.verdict)
if got != tc.want {
t.Errorf("verdictToEvent(%q) = %q, want %q", tc.verdict, got, tc.want)
}
}
}
github/reviews.go
+236
View File
@@ -0,0 +1,236 @@
package github
sonnet-review-bot commented 2026-05-13 10:55:52 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The DeleteReview method is implemented but appears to have no callers (GitHub supersede uses DismissReview). This is fine as part of satisfying the vcs.Client interface if that interface requires it, but if it's an extra method not in the interface, it adds untested surface area. The conformance test confirms vcs.Client is satisfied, so if DeleteReview is in the interface this is fine; otherwise it's unused public API.

**[NIT]** The `DeleteReview` method is implemented but appears to have no callers (GitHub supersede uses `DismissReview`). This is fine as part of satisfying the `vcs.Client` interface if that interface requires it, but if it's an extra method not in the interface, it adds untested surface area. The conformance test confirms `vcs.Client` is satisfied, so if `DeleteReview` is in the interface this is fine; otherwise it's unused public API.
import (
"bytes"
"context"
"encoding/json"
"fmt"
"io"
"net/http"
"net/url"
"strings"
"gitea.weiker.me/rodin/review-bot/vcs"
)
// reviewResponse is the GitHub API response for a pull request review.
type reviewResponse struct {
ID int64 `json:"id"`
Body string `json:"body"`
User struct {
Login string `json:"login"`
} `json:"user"`
State string `json:"state"`
CommitID string `json:"commit_id"`
}
// reviewCreateRequest is the GitHub API request body for creating a pull request review.
type reviewCreateRequest struct {
Body string `json:"body"`
Event string `json:"event"`
Comments []reviewCommentCreate `json:"comments,omitempty"`
CommitID string `json:"commit_id,omitempty"`
}
// reviewCommentCreate is a single inline comment in a review creation request.
type reviewCommentCreate struct {
Path string `json:"path"`
Position int `json:"position"`
Body string `json:"body"`
}
// dismissReviewRequest is the GitHub API request body for dismissing a review.
type dismissReviewRequest struct {
Message string `json:"message"`
Event string `json:"event"`
}
// userResponse is the GitHub API response for the authenticated user.
sonnet-review-bot commented 2026-05-13 08:52:25 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The translateReviewEvent function has a default case that falls through to string(event), effectively passing unknown event types directly to the GitHub API as-is. This could cause confusing API errors. A safer default would be to return 'COMMENT' (matching the vcs.ReviewEventComment canonical value) rather than the raw string.

**[NIT]** The translateReviewEvent function has a default case that falls through to string(event), effectively passing unknown event types directly to the GitHub API as-is. This could cause confusing API errors. A safer default would be to return 'COMMENT' (matching the vcs.ReviewEventComment canonical value) rather than the raw string.
type userResponse struct {
Login string `json:"login"`
}
// translateReviewEvent converts a vcs.ReviewEvent to the GitHub API event string.
func translateReviewEvent(event vcs.ReviewEvent) string {
switch event {
case vcs.ReviewEventApprove:
return "APPROVE"
case vcs.ReviewEventRequestChanges:
sonnet-review-bot commented 2026-05-13 10:30:42 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The translateReviewEvent function has a default case that returns "COMMENT" and also an explicit case vcs.ReviewEventComment that returns "COMMENT". These are equivalent, so the explicit case is redundant. The default case alone is sufficient and is the idiomatic pattern for exhaustive switch fallbacks.

**[MINOR]** The `translateReviewEvent` function has a `default` case that returns `"COMMENT"` and also an explicit `case vcs.ReviewEventComment` that returns `"COMMENT"`. These are equivalent, so the explicit case is redundant. The `default` case alone is sufficient and is the idiomatic pattern for exhaustive switch fallbacks.
return "REQUEST_CHANGES"
case vcs.ReviewEventComment:
sonnet-review-bot commented 2026-05-13 10:08:59 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The translateReviewEvent function has a case vcs.ReviewEventComment: return "COMMENT" followed by default: return "COMMENT". The default case is unreachable given the current vcs.ReviewEvent type but is harmless. No change needed; just noting it's redundant.

**[NIT]** The `translateReviewEvent` function has a `case vcs.ReviewEventComment: return "COMMENT"` followed by `default: return "COMMENT"`. The default case is unreachable given the current `vcs.ReviewEvent` type but is harmless. No change needed; just noting it's redundant.
return "COMMENT"
default:
return string(event)
}
}
// PostReview creates a new review on a pull request.
func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, req vcs.ReviewRequest) (*vcs.Review, error) {
reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews",
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
payload := reviewCreateRequest{
Body: req.Body,
Event: translateReviewEvent(req.Event),
}
for _, comment := range req.Comments {
rc := reviewCommentCreate{
Path: comment.Path,
Position: comment.Position,
Body: comment.Body,
}
payload.Comments = append(payload.Comments, rc)
// Use CommitID from the first comment that has one
if payload.CommitID == "" && comment.CommitID != "" {
payload.CommitID = comment.CommitID
}
}
body, err := c.doJSONRequest(ctx, http.MethodPost, reqURL, payload)
if err != nil {
return nil, fmt.Errorf("post review: %w", err)
}
var resp reviewResponse
if err := json.Unmarshal(body, &resp); err != nil {
return nil, fmt.Errorf("parse review response: %w", err)
sonnet-review-bot commented 2026-05-13 13:45:25 +00:00
Outdated
Review
Copy Link
Copy Source

[MAJOR] In PostReview, the commit_id is set from the first comment that has one: if payload.CommitID == "" && comment.CommitID != "". The GitHub PR Reviews API requires that all comments in a single review reference the same commit_id (passed at the top-level request, not per-comment). If comments have different CommitIDs (which is possible since vcs.ReviewComment has a per-comment CommitID field), later comments' CommitIDs are silently ignored. This should either be validated (return an error if CommitIDs differ) or documented clearly.

**[MAJOR]** In `PostReview`, the commit_id is set from the first comment that has one: `if payload.CommitID == "" && comment.CommitID != ""`. The GitHub PR Reviews API requires that all comments in a single review reference the same commit_id (passed at the top-level request, not per-comment). If comments have different CommitIDs (which is possible since vcs.ReviewComment has a per-comment CommitID field), later comments' CommitIDs are silently ignored. This should either be validated (return an error if CommitIDs differ) or documented clearly.
}
return &vcs.Review{
sonnet-review-bot commented 2026-05-13 10:30:42 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The commit-ID selection logic ('Use CommitID from the first comment that has one') applies it to all inline comments in the review, but GitHub actually requires every inline comment to be on the same commit. This is correct behavior, but the comment implies it's a best-effort choice when it should be a guaranteed invariant. Worth adding a note that the caller (main.go) is expected to set CommitID to pr.Head.SHA on all comments.

**[NIT]** The commit-ID selection logic ('Use CommitID from the first comment that has one') applies it to all inline comments in the review, but GitHub actually requires every inline comment to be on the same commit. This is correct behavior, but the comment implies it's a best-effort choice when it should be a guaranteed invariant. Worth adding a note that the caller (`main.go`) is expected to set `CommitID` to `pr.Head.SHA` on all comments.
ID: resp.ID,
sonnet-review-bot commented 2026-05-13 09:24:34 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] In PostReview, CommitID is taken from the first inline comment that has one (payload.CommitID == "" && comment.CommitID != ""). GitHub's API requires commit_id to match the head commit for the review, and all inline comments must target the same commit. If comments have different CommitID values (e.g. mixed) the request may fail silently with incorrect positioning. A warning log when subsequent comments have a differing CommitID would help diagnose this in production.

**[NIT]** In `PostReview`, `CommitID` is taken from the first inline comment that has one (`payload.CommitID == "" && comment.CommitID != ""`). GitHub's API requires `commit_id` to match the head commit for the review, and all inline comments must target the same commit. If comments have different `CommitID` values (e.g. mixed) the request may fail silently with incorrect positioning. A warning log when subsequent comments have a differing CommitID would help diagnose this in production.
Body: resp.Body,
User: vcs.UserInfo{Login: resp.User.Login},
State: resp.State,
CommitID: resp.CommitID,
gpt-review-bot commented 2026-05-13 10:11:30 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] ListReviews paginates with a hard-coded 100-page limit. Consider extracting the per_page and max pages values into named constants for clarity and easier tuning.

**[NIT]** ListReviews paginates with a hard-coded 100-page limit. Consider extracting the per_page and max pages values into named constants for clarity and easier tuning.
}, nil
}
// ListReviews lists all reviews on a pull request.
func (c *Client) ListReviews(ctx context.Context, owner, repo string, number int) ([]vcs.Review, error) {
var allReviews []vcs.Review
for page := 1; page <= 100; page++ {
sonnet-review-bot commented 2026-05-13 09:37:04 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The CommitID on the review request is taken from the first comment that has one. If the reviews have multiple comments with different CommitIDs (which shouldn't happen in practice but is structurally possible), the behavior is implicit. A brief comment stating 'all comments are expected to reference the same commit' would make the intent explicit.

**[NIT]** The CommitID on the review request is taken from the first comment that has one. If the reviews have multiple comments with different CommitIDs (which shouldn't happen in practice but is structurally possible), the behavior is implicit. A brief comment stating 'all comments are expected to reference the same commit' would make the intent explicit.
rodin commented 2026-05-13 10:10:20 +00:00
Outdated
Review
Copy Link
Copy Source

Added: // All comments in a single review are expected to reference the same commit. — fixed in 7d6fe27.

Added: `// All comments in a single review are expected to reference the same commit.` — fixed in 7d6fe27.
reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews?per_page=100&page=%d",
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, page)
body, err := c.doGet(ctx, reqURL)
if err != nil {
sonnet-review-bot commented 2026-05-13 10:40:59 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] ListReviews paginates up to maxReviewPages=100 pages of 100 reviews each (10,000 reviews). For typical PRs this is fine, but the cap is silent — if a PR somehow exceeds 10,000 reviews the function returns a truncated list with no warning. Consider logging a warning when page == maxReviewPages && len(reviews) == reviewsPerPage to indicate potential truncation, consistent with how other bounded loops in the codebase behave.

**[MINOR]** `ListReviews` paginates up to `maxReviewPages=100` pages of 100 reviews each (10,000 reviews). For typical PRs this is fine, but the cap is silent — if a PR somehow exceeds 10,000 reviews the function returns a truncated list with no warning. Consider logging a warning when `page == maxReviewPages && len(reviews) == reviewsPerPage` to indicate potential truncation, consistent with how other bounded loops in the codebase behave.
return nil, fmt.Errorf("list reviews page %d: %w", page, err)
}
sonnet-review-bot commented 2026-05-13 13:20:49 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] In ListReviews, the page-limit warning if page == maxReviewPages fires at the end of the last iteration but after the len(reviews) < reviewsPerPage break check. The comment correctly explains this only fires when the final page is full, but the warning fires before returning — the loop will exit naturally after this iteration without an explicit break. This is correct but slightly subtle; a break after the warning would make control flow clearer.

**[NIT]** In `ListReviews`, the page-limit warning `if page == maxReviewPages` fires at the end of the last iteration but after the `len(reviews) < reviewsPerPage` break check. The comment correctly explains this only fires when the final page is full, but the warning fires before returning — the loop will exit naturally after this iteration without an explicit break. This is correct but slightly subtle; a `break` after the warning would make control flow clearer.
var reviews []reviewResponse
security-review-bot commented 2026-05-13 09:38:13 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] ListReviews and GetAuthenticatedUser use c.doGet (and DeleteReview uses c.doRequest). Ensure these helper methods enforce the same HTTPS-only constraint when a token is present as doJSONRequest does, to avoid accidental credential leakage if an http base URL is provided.

**[MINOR]** ListReviews and GetAuthenticatedUser use c.doGet (and DeleteReview uses c.doRequest). Ensure these helper methods enforce the same HTTPS-only constraint when a token is present as doJSONRequest does, to avoid accidental credential leakage if an http base URL is provided.
rodin commented 2026-05-13 10:10:20 +00:00
Outdated
Review
Copy Link
Copy Source

Confirmed: doGet delegates to doRequest, which enforces the same HTTPS-only check (lines 219-226 of client.go) when c.token != "" && !c.allowInsecureHTTP. DeleteReview also uses doRequest. All credential-bearing HTTP methods share the same HTTPS enforcement. No gap exists in this PR or in the existing code.

Confirmed: `doGet` delegates to `doRequest`, which enforces the same HTTPS-only check (lines 219-226 of `client.go`) when `c.token != "" && !c.allowInsecureHTTP`. `DeleteReview` also uses `doRequest`. All credential-bearing HTTP methods share the same HTTPS enforcement. No gap exists in this PR or in the existing code.
if err := json.Unmarshal(body, &reviews); err != nil {
sonnet-review-bot commented 2026-05-13 13:45:25 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The ListReviews page-limit warning fires inside the loop when page == maxReviewPages, but the loop condition is page <= maxReviewPages, so the final iteration runs after the warning. The log message 'results may be truncated' is correct, but it would be cleaner to check after the loop (if len(allReviews) == reviewsPerPage*maxReviewPages) rather than inside it.

**[NIT]** The `ListReviews` page-limit warning fires inside the loop when `page == maxReviewPages`, but the loop condition is `page <= maxReviewPages`, so the final iteration runs after the warning. The log message 'results may be truncated' is correct, but it would be cleaner to check after the loop (`if len(allReviews) == reviewsPerPage*maxReviewPages`) rather than inside it.
return nil, fmt.Errorf("parse reviews JSON: %w", err)
}
if len(reviews) == 0 {
break
}
for _, r := range reviews {
allReviews = append(allReviews, vcs.Review{
ID: r.ID,
Body: r.Body,
User: vcs.UserInfo{Login: r.User.Login},
State: r.State,
CommitID: r.CommitID,
})
}
if len(reviews) < 100 {
break
}
sonnet-review-bot commented 2026-05-13 10:55:52 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] In ListReviews, the warning log at page == maxReviewPages is placed after the inner loop that appended reviews from the final page, but the break after the len(reviews) < reviewsPerPage check means the warning only fires when the full page was received AND we hit the limit. This is intentional but the condition order (check truncation before the page-limit log) makes it slightly non-obvious. No functional bug, but the log fires on the iteration where we exit via page == maxReviewPages after the loop body completes — which is correct. A minor ordering note.

**[MINOR]** In `ListReviews`, the warning log at `page == maxReviewPages` is placed after the inner loop that appended reviews from the final page, but the `break` after the `len(reviews) < reviewsPerPage` check means the warning only fires when the full page was received AND we hit the limit. This is intentional but the condition order (check truncation before the page-limit log) makes it slightly non-obvious. No functional bug, but the log fires on the iteration where we exit via `page == maxReviewPages` after the loop body completes — which is correct. A minor ordering note.
sonnet-review-bot commented 2026-05-13 11:15:28 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The comment on the page-limit warning says 'The loop naturally exits after this iteration since page increments past maxReviewPages.' This is slightly misleading: the loop condition is page <= maxReviewPages, so when page == maxReviewPages and this warning fires, the loop body completes and then page++ makes it maxReviewPages+1, failing the condition. The behavior is correct, but the comment could be clearer: 'The loop exits on the next iteration check since page will be incremented to maxReviewPages+1.'

**[NIT]** The comment on the page-limit warning says 'The loop naturally exits after this iteration since page increments past maxReviewPages.' This is slightly misleading: the loop condition is `page <= maxReviewPages`, so when `page == maxReviewPages` and this warning fires, the loop body completes and then `page++` makes it `maxReviewPages+1`, failing the condition. The behavior is correct, but the comment could be clearer: 'The loop exits on the next iteration check since page will be incremented to maxReviewPages+1.'
}
return allReviews, nil
}
// DeleteReview deletes a review from a pull request.
func (c *Client) DeleteReview(ctx context.Context, owner, repo string, number int, reviewID int64) error {
reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews/%d",
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, reviewID)
_, err := c.doRequest(ctx, http.MethodDelete, reqURL, "")
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

[NIT] The ListReviews page-limit warning comment (// NOTE: This warning only fires when...) is placed after the if page == maxReviewPages block but before the loop's closing brace. The comment is correct but its placement after the action it describes is mildly confusing. Moving it above the if block would improve readability.

**[NIT]** The `ListReviews` page-limit warning comment (`// NOTE: This warning only fires when...`) is placed after the `if page == maxReviewPages` block but before the loop's closing brace. The comment is correct but its placement after the action it describes is mildly confusing. Moving it above the `if` block would improve readability.
if err != nil {
return fmt.Errorf("delete review: %w", err)
}
return nil
}
sonnet-review-bot commented 2026-05-13 08:22:47 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The DeleteReview method is implemented but appears to not be used anywhere in the codebase (GitHub uses DismissReview for superseding). If DeleteReview is required to satisfy the vcs.Client interface, that's fine, but it's worth confirming the interface definition requires it. If it's only for completeness, a brief doc comment noting when to use it versus DismissReview would help.

**[NIT]** The `DeleteReview` method is implemented but appears to not be used anywhere in the codebase (GitHub uses `DismissReview` for superseding). If `DeleteReview` is required to satisfy the `vcs.Client` interface, that's fine, but it's worth confirming the interface definition requires it. If it's only for completeness, a brief doc comment noting when to use it versus `DismissReview` would help.
gpt-review-bot commented 2026-05-13 08:53:50 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] DismissReview sends an 'event' field with value 'DISMISS' in the request payload. GitHub’s dismiss review endpoint typically requires a 'message' and may not require or accept 'event'; verify against the current GitHub REST API to avoid 4xx responses.

**[MINOR]** DismissReview sends an 'event' field with value 'DISMISS' in the request payload. GitHub’s dismiss review endpoint typically requires a 'message' and may not require or accept 'event'; verify against the current GitHub REST API to avoid 4xx responses.
// DismissReview dismisses a review on a pull request with a message.
func (c *Client) DismissReview(ctx context.Context, owner, repo string, number int, reviewID int64, message string) error {
reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews/%d/dismissals",
sonnet-review-bot commented 2026-05-13 08:52:25 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] doJSONRequest duplicates HTTP request logic that likely already exists in the Client (doGet/doRequest methods are referenced but not shown in the diff). The HTTPS enforcement check, header setting, and response reading are implemented inline here rather than being shared. This creates two code paths to maintain. Consider extracting the shared transport logic or delegating to a shared helper, especially since doGet and doRequest already exist on the same type.

**[MINOR]** doJSONRequest duplicates HTTP request logic that likely already exists in the Client (doGet/doRequest methods are referenced but not shown in the diff). The HTTPS enforcement check, header setting, and response reading are implemented inline here rather than being shared. This creates two code paths to maintain. Consider extracting the shared transport logic or delegating to a shared helper, especially since doGet and doRequest already exist on the same type.
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, reviewID)
payload := dismissReviewRequest{
Message: message,
Event: "DISMISS",
sonnet-review-bot commented 2026-05-13 09:37:04 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] doJSONRequest is defined on *Client in this file but conceptually duplicates HTTP infrastructure that likely already exists elsewhere in the github package (the file references c.doGet, c.doRequest, c.allowInsecureHTTP, c.httpClient, c.token, userAgent, maxResponseBytes — all of which must live in another file). This is fine architecturally, but the HTTPS-enforcement check inside doJSONRequest is inconsistent with doGet/doRequest which presumably handle their own security. If those methods don't enforce HTTPS, the security posture is uneven. If they do, the duplication is unnecessary. Worth ensuring the pattern is consistent across the package.

**[MINOR]** doJSONRequest is defined on *Client in this file but conceptually duplicates HTTP infrastructure that likely already exists elsewhere in the github package (the file references c.doGet, c.doRequest, c.allowInsecureHTTP, c.httpClient, c.token, userAgent, maxResponseBytes — all of which must live in another file). This is fine architecturally, but the HTTPS-enforcement check inside doJSONRequest is inconsistent with doGet/doRequest which presumably handle their own security. If those methods don't enforce HTTPS, the security posture is uneven. If they do, the duplication is unnecessary. Worth ensuring the pattern is consistent across the package.
rodin commented 2026-05-13 10:10:20 +00:00
Outdated
Review
Copy Link
Copy Source

Confirmed: doRequest (which doGet and DeleteReview use) already enforces the identical HTTPS-only check at lines 219-226 of client.go. doJSONRequest has its own copy because it handles the request lifecycle differently (JSON marshaling, bytes.NewReader, content headers) and doesn't route through doRequest. The security posture is consistent across all HTTP methods — no gap exists.

A future refactor could extract the URL-scheme check into a shared helper, but that's cosmetic. No changes needed here.

Confirmed: `doRequest` (which `doGet` and `DeleteReview` use) already enforces the identical HTTPS-only check at lines 219-226 of `client.go`. `doJSONRequest` has its own copy because it handles the request lifecycle differently (JSON marshaling, `bytes.NewReader`, content headers) and doesn't route through `doRequest`. The security posture is consistent across all HTTP methods — no gap exists. A future refactor could extract the URL-scheme check into a shared helper, but that's cosmetic. No changes needed here.
}
sonnet-review-bot commented 2026-05-13 09:24:34 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] doJSONRequest is defined in github/reviews.go but it is a general HTTP helper. If other files in the github package (e.g. the existing client file) also define HTTP helpers, there may be duplication or naming conflicts. This is not visible from the diff alone, but worth verifying that doRequest (used by DeleteReview) and doGet (used by ListReviews, GetAuthenticatedUser) are also defined in the github package and that doJSONRequest doesn't conflict with or duplicate them.

**[MINOR]** `doJSONRequest` is defined in `github/reviews.go` but it is a general HTTP helper. If other files in the `github` package (e.g. the existing client file) also define HTTP helpers, there may be duplication or naming conflicts. This is not visible from the diff alone, but worth verifying that `doRequest` (used by `DeleteReview`) and `doGet` (used by `ListReviews`, `GetAuthenticatedUser`) are also defined in the `github` package and that `doJSONRequest` doesn't conflict with or duplicate them.
sonnet-review-bot commented 2026-05-13 10:08:59 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] doJSONRequest is defined in github/reviews.go but is a general HTTP helper. If other files in the github package (e.g., the client file not shown in the diff) also define HTTP helpers, this may cause a naming conflict or duplication. Consider whether this belongs in a shared internal helper or the main client file. This is a structural concern rather than a correctness issue.

**[MINOR]** `doJSONRequest` is defined in `github/reviews.go` but is a general HTTP helper. If other files in the `github` package (e.g., the client file not shown in the diff) also define HTTP helpers, this may cause a naming conflict or duplication. Consider whether this belongs in a shared internal helper or the main client file. This is a structural concern rather than a correctness issue.
_, err := c.doJSONRequest(ctx, http.MethodPut, reqURL, payload)
if err != nil {
return fmt.Errorf("dismiss review: %w", err)
}
return nil
}
// GetAuthenticatedUser returns the login name of the authenticated user.
func (c *Client) GetAuthenticatedUser(ctx context.Context) (string, error) {
reqURL := fmt.Sprintf("%s/user", c.baseURL)
body, err := c.doGet(ctx, reqURL)
if err != nil {
return "", fmt.Errorf("get authenticated user: %w", err)
}
var resp userResponse
if err := json.Unmarshal(body, &resp); err != nil {
return "", fmt.Errorf("parse user response: %w", err)
}
return resp.Login, nil
}
sonnet-review-bot commented 2026-05-13 08:22:47 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] doJSONRequest uses interface{} as the payload parameter type instead of any. While both are equivalent, any is the idiomatic alias since Go 1.18 and is used throughout the rest of the codebase (e.g., the vcs package). This is a minor style inconsistency.

**[MINOR]** doJSONRequest uses `interface{}` as the payload parameter type instead of `any`. While both are equivalent, `any` is the idiomatic alias since Go 1.18 and is used throughout the rest of the codebase (e.g., the `vcs` package). This is a minor style inconsistency.
// doJSONRequest performs an HTTP request with a JSON body and returns the response body.
// It handles HTTPS validation, authentication, and response reading.
func (c *Client) doJSONRequest(ctx context.Context, method, reqURL string, payload interface{}) ([]byte, error) {
const maxErrorBodyBytes = 4 * 1024
jsonBody, err := json.Marshal(payload)
if err != nil {
return nil, fmt.Errorf("marshal request body: %w", err)
}
gpt-review-bot commented 2026-05-13 08:53:50 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] Authorization header uses 'Bearer' scheme. While modern GitHub tokens support 'Bearer', some environments still document 'token '. Consider supporting or documenting both for broader compatibility.

**[NIT]** Authorization header uses 'Bearer' scheme. While modern GitHub tokens support 'Bearer', some environments still document 'token <TOKEN>'. Consider supporting or documenting both for broader compatibility.
if c.token != "" && !c.allowInsecureHTTP {
parsed, err := url.Parse(reqURL)
if err != nil {
gpt-review-bot commented 2026-05-13 09:37:54 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] Authorization header uses "Bearer" for GitHub API; classic PATs typically use the "token" scheme. Consider supporting both schemes or documenting the expected token type to avoid auth issues.

**[MINOR]** Authorization header uses "Bearer" for GitHub API; classic PATs typically use the "token" scheme. Consider supporting both schemes or documenting the expected token type to avoid auth issues.
rodin commented 2026-05-13 10:10:20 +00:00
Outdated
Review
Copy Link
Copy Source

Pushing back on this one: Bearer is the correct and standard scheme per RFC 6750. GitHub's own docs recommend it for fine-grained personal access tokens and GitHub App tokens. The token scheme is a GitHub-specific legacy convention that predates their OAuth2 adoption. Modern GitHub tokens (fine-grained PATs, GitHub App installation tokens) all use Bearer. No change needed.

Pushing back on this one: `Bearer` is the correct and standard scheme per [RFC 6750](https://datatracker.ietf.org/doc/html/rfc6750). GitHub's own docs recommend it for fine-grained personal access tokens and GitHub App tokens. The `token` scheme is a GitHub-specific legacy convention that predates their OAuth2 adoption. Modern GitHub tokens (fine-grained PATs, GitHub App installation tokens) all use Bearer. No change needed.
return nil, fmt.Errorf("parse request URL: %w", err)
}
if !strings.EqualFold(parsed.Scheme, "https") {
return nil, fmt.Errorf("refusing to send credentials over non-HTTPS URL %q (use AllowInsecureHTTP option for trusted networks)", reqURL)
}
}
req, err := http.NewRequestWithContext(ctx, method, reqURL, bytes.NewReader(jsonBody))
if err != nil {
return nil, fmt.Errorf("create request: %w", err)
}
if c.token != "" {
req.Header.Set("Authorization", "Bearer "+c.token)
}
req.Header.Set("User-Agent", userAgent)
req.Header.Set("Accept", "application/vnd.github+json")
req.Header.Set("Content-Type", "application/json")
resp, err := c.httpClient.Do(req)
if err != nil {
return nil, fmt.Errorf("do request: %w", err)
}
defer resp.Body.Close()
if resp.StatusCode >= 200 && resp.StatusCode < 300 {
body, err := io.ReadAll(io.LimitReader(resp.Body, int64(maxResponseBytes)+1))
if err != nil {
return nil, fmt.Errorf("read response body: %w", err)
}
if len(body) > maxResponseBytes {
return nil, fmt.Errorf("response body exceeded %d bytes", maxResponseBytes)
}
return body, nil
}
errBody, _ := io.ReadAll(io.LimitReader(resp.Body, int64(maxErrorBodyBytes)))
return nil, &APIError{StatusCode: resp.StatusCode, Body: string(errBody)}
}
review/formatter.go
-11
View File
@@ -10,17 +10,6 @@ func FormatMarkdown(result *ReviewResult, reviewerName string) string {
return FormatMarkdownWithDisplay(result, reviewerName, reviewerName)
}
// GiteaEvent converts the verdict to the Gitea API event string.
func GiteaEvent(verdict string) string {
switch verdict {
case "APPROVE":
return "APPROVED"
case "REQUEST_CHANGES":
return "REQUEST_CHANGES"
default:
return "COMMENT"
}
}
// FormatMarkdownWithDisplay formats a ReviewResult with separate display name and sentinel name.
// Note: displayName is not HTML-escaped as Gitea sanitizes rendered Markdown.
review/formatter_test.go
-18
View File
@@ -98,24 +98,6 @@ func TestFormatMarkdown_SpecialChars(t *testing.T) {
}
}
func TestGiteaEvent(t *testing.T) {
tests := []struct {
verdict string
expected string
}{
{"APPROVE", "APPROVED"},
{"REQUEST_CHANGES", "REQUEST_CHANGES"},
{"UNKNOWN", "COMMENT"},
{"", "COMMENT"},
}
for _, tc := range tests {
got := GiteaEvent(tc.verdict)
if got != tc.expected {
t.Errorf("GiteaEvent(%q) = %q, want %q", tc.verdict, got, tc.expected)
}
}
}
sonnet-review-bot commented 2026-05-13 08:22:47 +00:00
Review
Copy Link
Copy Source

[NIT] Similar to the formatter.go nit — a blank line artifact remains after removing TestGiteaEvent. Minor formatting issue.

**[NIT]** Similar to the formatter.go nit — a blank line artifact remains after removing `TestGiteaEvent`. Minor formatting issue.
func TestFormatMarkdown_Sentinel(t *testing.T) {
result := &ReviewResult{