feat(cmd): wire --provider and --base-url flags into CLI (Phase 5) #106
@@ -13,6 +13,7 @@ import (
|
|||||||
|
|
||||||
"gitea.weiker.me/rodin/review-bot/budget"
|
"gitea.weiker.me/rodin/review-bot/budget"
|
||||||
"gitea.weiker.me/rodin/review-bot/gitea"
|
"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/llm"
|
||||||
"gitea.weiker.me/rodin/review-bot/review"
|
"gitea.weiker.me/rodin/review-bot/review"
|
||||||
"gitea.weiker.me/rodin/review-bot/vcs"
|
"gitea.weiker.me/rodin/review-bot/vcs"
|
||||||
@@ -54,12 +55,15 @@ func main() {
|
|||||||
// Logging flags
|
// Logging flags
|
||||||
logFormat := flag.String("log-format", envOrDefault("LOG_FORMAT", "text"), "Log output format: text or json")
|
logFormat := flag.String("log-format", envOrDefault("LOG_FORMAT", "text"), "Log output format: text or json")
|
||||||
|
|
|||||||
verbosity := flag.String("verbosity", envOrDefault("LOG_VERBOSITY", "info"), "Log verbosity: debug, info, warn, error")
|
verbosity := flag.String("verbosity", envOrDefault("LOG_VERBOSITY", "info"), "Log verbosity: debug, info, warn, error")
|
||||||
// CLI flags
|
// VCS flags
|
||||||
giteaURL := flag.String("gitea-url", envOrDefault("GITEA_URL", envOrDefault("GITHUB_SERVER_URL", "")), "Gitea instance URL")
|
provider := flag.String("provider", envOrDefault("VCS_PROVIDER", "gitea"), "VCS provider: gitea or github")
|
||||||
repo := flag.String("repo", envOrDefault("GITEA_REPO", envOrDefault("GITHUB_REPOSITORY", "")), "Repository (owner/name)")
|
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 backward-compatible 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")
|
prNum := flag.String("pr", envOrDefault("PR_NUMBER", ""), "Pull request number")
|
||||||
reviewerName := flag.String("reviewer-name", envOrDefault("REVIEWER_NAME", ""), "Reviewer display name")
|
reviewerName := flag.String("reviewer-name", envOrDefault("REVIEWER_NAME", ""), "Reviewer display name")
|
||||||
|
sonnet-review-bot
commented
[NIT] The **[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")
|
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")
|
llmAPIKey := flag.String("llm-api-key", envOrDefault("LLM_API_KEY", ""), "LLM API key")
|
||||||
llmModel := flag.String("llm-model", envOrDefault("LLM_MODEL", ""), "LLM model name")
|
llmModel := flag.String("llm-model", envOrDefault("LLM_MODEL", ""), "LLM model name")
|
||||||
@@ -80,6 +84,11 @@ func main() {
|
|||||||
aicoreAPIURL := flag.String("aicore-api-url", envOrDefault("AICORE_API_URL", ""), "SAP AI Core API URL (for provider=aicore)")
|
aicoreAPIURL := flag.String("aicore-api-url", envOrDefault("AICORE_API_URL", ""), "SAP AI Core API URL (for provider=aicore)")
|
||||||
|
sonnet-review-bot
commented
[MINOR] The --gitea-url alias registration uses **[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
[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
[MINOR] The **[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
Added a comment explaining the shared 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
Updated the inline comment from "hidden alias" to "backward-compatible alias" in 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
[NIT] The comment **[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)")
|
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
[NIT] The **[NIT]** The `--gitea-url` alias comment block is long (12 lines) and explains a subtle flag.StringVar trick. Consider extracting the alias registration to a small helper or at minimum shortening the comment to the key invariant: `flag.StringVar shares the pointer, so whichever flag is set last wins.` The current comment is accurate but its length makes it easy to miss the ORDERING constraint.
|
|||||||
|
|
||||||
|
// Backward-compatible alias: --gitea-url shares vcsURL's pointer (last flag wins).
|
||||||
|
sonnet-review-bot
commented
[MINOR] The **[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
[NIT] The comment on the **[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
[MINOR] The **[MINOR]** The `--gitea-url` backward-compatible alias is registered with `flag.StringVar(vcsURL, "gitea-url", *vcsURL, ...)` where `*vcsURL` is the default value captured at the moment of the call — before `flag.Parse()`. This is correct as written because `*vcsURL` is still the default string at that point, but it creates a subtle ordering dependency: if `vcsURL` ever acquired a non-default value between its declaration and this `flag.StringVar` call (e.g., if any code ran between them), the alias would get a stale default. The comment acknowledges this (`Must stay after vcsURL declaration and before flag.Parse()`), but the fragility is worth noting. A cleaner pattern would be `flag.StringVar(vcsURL, "gitea-url", "", "...")` or extracting the default value to a named constant.
|
|||||||
|
// Must use *vcsURL as default: StringVar sets *p=value at registration, so empty
|
||||||
|
sonnet-review-bot
commented
[NIT] The **[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.
|
|||||||
|
// string would overwrite the env-resolved value from the --vcs-url declaration.
|
||||||
|
flag.StringVar(vcsURL, "gitea-url", *vcsURL, "Deprecated: use --vcs-url instead")
|
||||||
|
|
||||||
flag.Parse()
|
flag.Parse()
|
||||||
|
|
||||||
if *versionFlag {
|
if *versionFlag {
|
||||||
@@ -92,12 +101,23 @@ func main() {
|
|||||||
|
|
||||||
|
gpt-review-bot
commented
[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
[NIT] The **[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.
|
|||||||
slog.Info("review-bot starting", "version", version)
|
slog.Info("review-bot starting", "version", version)
|
||||||
|
|
||||||
|
// Validate VCS provider
|
||||||
|
vcsProvider := vcs.VCSProvider(*provider)
|
||||||
|
if !vcsProvider.Valid() {
|
||||||
|
fmt.Fprintf(os.Stderr, "Error: invalid --provider %q (valid: gitea, github)\n", *provider)
|
||||||
|
os.Exit(1)
|
||||||
|
}
|
||||||
|
|
||||||
// Validate required fields
|
// Validate required fields
|
||||||
// For aicore provider, llm-base-url and llm-api-key are not required
|
|
||||||
isAICore := llm.Provider(*llmProvider) == llm.ProviderAICore
|
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, "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 vcsProvider == vcs.ProviderGitea && *vcsURL == "" {
|
||||||
|
fmt.Fprintf(os.Stderr, "Error: --vcs-url (or --gitea-url) is required for provider=gitea\n")
|
||||||
os.Exit(1)
|
os.Exit(1)
|
||||||
}
|
}
|
||||||
if !isAICore && (*llmBaseURL == "" || *llmAPIKey == "") {
|
if !isAICore && (*llmBaseURL == "" || *llmAPIKey == "") {
|
||||||
@@ -116,8 +136,6 @@ func main() {
|
|||||||
os.Exit(1)
|
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
|
// Validate reviewer-name: only safe characters allowed in sentinel
|
||||||
if err := validateReviewerName(*reviewerName); err != nil {
|
if err := validateReviewerName(*reviewerName); err != nil {
|
||||||
slog.Error("invalid reviewer name", "error", err)
|
slog.Error("invalid reviewer name", "error", err)
|
||||||
@@ -139,8 +157,20 @@ func main() {
|
|||||||
os.Exit(1)
|
os.Exit(1)
|
||||||
}
|
}
|
||||||
|
|
||||||
// Initialize clients
|
// Initialize VCS client
|
||||||
giteaClient := gitea.NewClient(*giteaURL, *reviewerToken)
|
var client vcs.Client
|
||||||
|
switch vcsProvider {
|
||||||
|
case vcs.ProviderGitea:
|
||||||
|
giteaClient := gitea.NewClient(*vcsURL, *reviewerToken)
|
||||||
|
client = gitea.NewAdapter(giteaClient)
|
||||||
|
case vcs.ProviderGitHub:
|
||||||
|
client = github.NewClient(*reviewerToken, *baseURL)
|
||||||
|
rodin marked this conversation as resolved
Outdated
sonnet-review-bot
commented
[MINOR] The default case in the VCS client initialization switch ( **[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.
|
|||||||
|
default:
|
||||||
|
panic("unreachable: provider validation should have caught " + vcsProvider.String())
|
||||||
|
}
|
||||||
|
sonnet-review-bot
commented
[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
[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
[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.
|
|||||||
|
slog.Info("VCS client initialized", "provider", vcsProvider)
|
||||||
|
sonnet-review-bot
commented
[MINOR] The switch for initializing **[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] 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.
|
|||||||
|
|
||||||
|
// Initialize LLM client
|
||||||
|
sonnet-review-bot
commented
[NIT] The **[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.
|
|||||||
llmClient := llm.NewClient(*llmBaseURL, *llmAPIKey, *llmModel)
|
llmClient := llm.NewClient(*llmBaseURL, *llmAPIKey, *llmModel)
|
||||||
if *llmTemp < 0 || *llmTemp > 2 {
|
if *llmTemp < 0 || *llmTemp > 2 {
|
||||||
|
[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
[MINOR] The **[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.
|
|||||||
slog.Error("invalid LLM temperature", "temperature", *llmTemp, "range", "0-2")
|
slog.Error("invalid LLM temperature", "temperature", *llmTemp, "range", "0-2")
|
||||||
|
[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.
|
|||||||
@@ -174,16 +204,13 @@ func main() {
|
|||||||
ctx, cancel := context.WithTimeout(context.Background(), overallTimeout)
|
ctx, cancel := context.WithTimeout(context.Background(), overallTimeout)
|
||||||
defer cancel()
|
defer cancel()
|
||||||
|
|
||||||
// Load persona if specified (after Gitea client init to support repo personas)
|
// Load persona if specified
|
||||||
var persona *review.Persona
|
var persona *review.Persona
|
||||||
if *personaName != "" {
|
if *personaName != "" {
|
||||||
// Try loading from repo first, then fall back to built-in
|
// 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 {
|
if err != nil {
|
||||||
slog.Warn("could not load repo personas", "repo", owner+"/"+repoName, "error", err)
|
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 {
|
if p, ok := repoPersonas[*personaName]; ok {
|
||||||
persona = p
|
persona = p
|
||||||
@@ -214,7 +241,7 @@ func main() {
|
|||||||
slog.Info("reviewing pull request", "pr", prNumber, "repo", fmt.Sprintf("%s/%s", owner, repoName))
|
slog.Info("reviewing pull request", "pr", prNumber, "repo", fmt.Sprintf("%s/%s", owner, repoName))
|
||||||
|
|
||||||
// Step 1: Fetch PR metadata
|
// Step 1: Fetch PR metadata
|
||||||
pr, err := giteaClient.GetPullRequest(ctx, owner, repoName, prNumber)
|
pr, err := client.GetPullRequest(ctx, owner, repoName, prNumber)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
slog.Error("failed to fetch PR", "pr", prNumber, "error", err)
|
slog.Error("failed to fetch PR", "pr", prNumber, "error", err)
|
||||||
os.Exit(1)
|
os.Exit(1)
|
||||||
@@ -222,7 +249,7 @@ func main() {
|
|||||||
slog.Info("fetched PR metadata", "pr", prNumber, "title", pr.Title)
|
slog.Info("fetched PR metadata", "pr", prNumber, "title", pr.Title)
|
||||||
|
|
||||||
// Step 2: Fetch diff
|
// Step 2: Fetch diff
|
||||||
diff, err := giteaClient.GetPullRequestDiff(ctx, owner, repoName, prNumber)
|
diff, err := client.GetPullRequestDiff(ctx, owner, repoName, prNumber)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
slog.Error("failed to fetch diff", "pr", prNumber, "error", err)
|
slog.Error("failed to fetch diff", "pr", prNumber, "error", err)
|
||||||
os.Exit(1)
|
os.Exit(1)
|
||||||
@@ -231,21 +258,21 @@ func main() {
|
|||||||
|
|
||||||
|
sonnet-review-bot
commented
[MINOR] The **[MINOR]** The `vcsURL` flag captures the env-var-resolved default via a pointer dereference (`*vcsURL`) when registering the `--gitea-url` alias. The comment explains this, but the approach is subtle and fragile: if someone ever moves the `flag.StringVar` call before `flag.String` completes or wraps it in a helper, it silently uses the zero-value default. A simpler alternative would be to compute the default value once into a local variable (`giteaURLDefault := envOrDefault(...)`) and use it for both registrations, eliminating the ordering dependency entirely.
|
|||||||
// Step 3: Fetch full file content for modified files
|
// Step 3: Fetch full file content for modified files
|
||||||
fileContext := ""
|
fileContext := ""
|
||||||
files, err := giteaClient.GetPullRequestFiles(ctx, owner, repoName, prNumber)
|
files, err := client.GetPullRequestFiles(ctx, owner, repoName, prNumber)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
slog.Warn("could not fetch PR files list", "pr", prNumber, "error", err)
|
slog.Warn("could not fetch PR files list", "pr", prNumber, "error", err)
|
||||||
} else {
|
} 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))
|
slog.Debug("fetched file context", "files", len(files))
|
||||||
}
|
}
|
||||||
|
|
||||||
// Step 4: Check CI status
|
// Step 4: Check CI status
|
||||||
ciPassed := true
|
ciPassed := true
|
||||||
ciDetails := ""
|
ciDetails := ""
|
||||||
if pr.Head.Sha != "" {
|
if pr.Head.SHA != "" {
|
||||||
statuses, err := giteaClient.GetCommitStatuses(ctx, owner, repoName, pr.Head.Sha)
|
statuses, err := client.GetCommitStatuses(ctx, owner, repoName, pr.Head.SHA)
|
||||||
if err != nil {
|
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 {
|
} else {
|
||||||
ciPassed, ciDetails = evaluateCIStatus(statuses)
|
ciPassed, ciDetails = evaluateCIStatus(statuses)
|
||||||
slog.Info("CI status checked", "passed", ciPassed)
|
slog.Info("CI status checked", "passed", ciPassed)
|
||||||
@@ -255,7 +282,7 @@ func main() {
|
|||||||
// Step 5: Load conventions file if specified
|
// Step 5: Load conventions file if specified
|
||||||
conventions := ""
|
conventions := ""
|
||||||
if *conventionsFile != "" {
|
if *conventionsFile != "" {
|
||||||
content, err := giteaClient.GetFileContent(ctx, owner, repoName, *conventionsFile)
|
content, err := client.GetFileContent(ctx, owner, repoName, *conventionsFile, "")
|
||||||
if err != nil {
|
if err != nil {
|
||||||
slog.Warn("could not load conventions file", "file", *conventionsFile, "error", err)
|
slog.Warn("could not load conventions file", "file", *conventionsFile, "error", err)
|
||||||
} else {
|
} else {
|
||||||
@@ -267,7 +294,7 @@ func main() {
|
|||||||
// Step 6: Load patterns from external repo if specified
|
// Step 6: Load patterns from external repo if specified
|
||||||
patterns := ""
|
patterns := ""
|
||||||
if *patternsRepo != "" {
|
if *patternsRepo != "" {
|
||||||
patterns = fetchPatterns(ctx, giteaClient, *patternsRepo, *patternsFiles)
|
patterns = fetchPatterns(ctx, client, *patternsRepo, *patternsFiles)
|
||||||
slog.Debug("loaded patterns", "repo", *patternsRepo, "bytes", len(patterns))
|
slog.Debug("loaded patterns", "repo", *patternsRepo, "bytes", len(patterns))
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -360,15 +387,16 @@ func main() {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// Add commit footer so readers know which commit was evaluated
|
// Add commit footer so readers know which commit was evaluated
|
||||||
if pr.Head.Sha != "" {
|
if pr.Head.SHA != "" {
|
||||||
shortSHA := pr.Head.Sha
|
shortSHA := pr.Head.SHA
|
||||||
if len(shortSHA) > 8 {
|
if len(shortSHA) > 8 {
|
||||||
shortSHA = shortSHA[:8]
|
shortSHA = shortSHA[:8]
|
||||||
}
|
}
|
||||||
reviewBody += fmt.Sprintf("\n\n---\n*Evaluated against %s*", shortSHA)
|
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)
|
||||||
|
|
||||||
if *dryRun {
|
if *dryRun {
|
||||||
fmt.Println("--- DRY RUN ---")
|
fmt.Println("--- DRY RUN ---")
|
||||||
|
gpt-review-bot
commented
[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.
|
|||||||
@@ -380,34 +408,40 @@ func main() {
|
|||||||
sentinel := fmt.Sprintf("<!-- review-bot:%s -->", *reviewerName)
|
sentinel := fmt.Sprintf("<!-- review-bot:%s -->", *reviewerName)
|
||||||
|
|
||||||
|
gpt-review-bot
commented
[MINOR] In dry-run output, fmt.Printf("Event: %s\n\n", event) assumes vcs.ReviewEvent formats as a string. If ReviewEvent’s underlying type isn’t string or lacks String(), prefer %v or explicitly convert to string to avoid formatting issues. **[MINOR]** In dry-run output, fmt.Printf("Event: %s\n\n", event) assumes vcs.ReviewEvent formats as a string. If ReviewEvent’s underlying type isn’t string or lacks String(), prefer %v or explicitly convert to string to avoid formatting issues.
|
|||||||
// Stale check: verify HEAD hasn't moved since we started
|
// Stale check: verify HEAD hasn't moved since we started
|
||||||
evaluatedSHA := pr.Head.Sha
|
evaluatedSHA := pr.Head.SHA
|
||||||
var currentSHA string
|
var currentSHA string
|
||||||
currentPR, err := giteaClient.GetPullRequest(ctx, owner, repoName, prNumber)
|
currentPR, err := client.GetPullRequest(ctx, owner, repoName, prNumber)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
slog.Warn("could not re-fetch PR for stale check", "pr", prNumber, "error", err)
|
slog.Warn("could not re-fetch PR for stale check", "pr", prNumber, "error", err)
|
||||||
// currentSHA stays empty — shouldSkipStaleReview will return false
|
|
||||||
} else {
|
} else {
|
||||||
currentSHA = currentPR.Head.Sha
|
currentSHA = currentPR.Head.SHA
|
||||||
}
|
}
|
||||||
if shouldSkipStaleReview(evaluatedSHA, currentSHA) {
|
if shouldSkipStaleReview(evaluatedSHA, currentSHA) {
|
||||||
slog.Warn("HEAD moved during review — skipping stale review",
|
slog.Warn("HEAD moved during review -- skipping stale review",
|
||||||
"evaluated", evaluatedSHA,
|
"evaluated", evaluatedSHA,
|
||||||
"current", currentSHA,
|
"current", currentSHA,
|
||||||
"pr", prNumber)
|
"pr", prNumber)
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
// Map findings to inline comments for lines present in the diff
|
// Build line→position map for inline comments
|
||||||
diffRanges := gitea.ParseDiffNewLines(diff)
|
lineToPosition := vcs.BuildLineToPositionMap(diff)
|
||||||
var inlineComments []gitea.ReviewComment
|
var inlineComments []vcs.ReviewComment
|
||||||
for _, f := range result.Findings {
|
for _, f := range result.Findings {
|
||||||
if f.File != "" && f.Line > 0 && diffRanges.Contains(f.File, f.Line) {
|
if f.File == "" || f.Line <= 0 {
|
||||||
inlineComments = append(inlineComments, gitea.ReviewComment{
|
continue
|
||||||
Path: f.File,
|
|
||||||
NewPosition: int64(f.Line),
|
|
||||||
Body: fmt.Sprintf("**[%s]** %s", f.Severity, f.Finding),
|
|
||||||
})
|
|
||||||
}
|
}
|
||||||
|
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 {
|
if len(inlineComments) > 0 {
|
||||||
slog.Debug("attaching inline comments", "count", len(inlineComments))
|
slog.Debug("attaching inline comments", "count", len(inlineComments))
|
||||||
@@ -416,10 +450,9 @@ func main() {
|
|||||||
// --- Review update strategy ---
|
// --- Review update strategy ---
|
||||||
// 1. POST new review first (gets non-stale approval badge on HEAD)
|
// 1. POST new review first (gets non-stale approval badge on HEAD)
|
||||||
// 2. Then supersede old review with link to the new one
|
// 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 []vcs.Review
|
||||||
var oldReviews []gitea.Review
|
|
||||||
if *reviewerName != "" {
|
if *reviewerName != "" {
|
||||||
existingReviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber)
|
existingReviews, err := client.ListReviews(ctx, owner, repoName, prNumber)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
slog.Warn("could not list existing reviews", "pr", prNumber, "error", err)
|
slog.Warn("could not list existing reviews", "pr", prNumber, "error", err)
|
||||||
} else {
|
} else {
|
||||||
@@ -431,74 +464,63 @@ func main() {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
gpt-review-bot
commented
[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.
|
|||||||
// Self-request as reviewer (ensures we appear in required-reviewer checks)
|
// Self-request as reviewer (Gitea-specific; ensures we appear in required-reviewer checks)
|
||||||
|
sonnet-review-bot
commented
[NIT] **[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.
|
|||||||
authUser, err := giteaClient.GetAuthenticatedUser(ctx)
|
if selfReq, ok := client.(vcs.ReviewerSelfRequester); ok {
|
||||||
if err != nil {
|
authUser, err := client.GetAuthenticatedUser(ctx)
|
||||||
|
sonnet-review-bot
commented
[MINOR] The **[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.
|
|||||||
slog.Warn("could not determine authenticated user for reviewer self-request", "error", err)
|
if err != nil {
|
||||||
} else if authUser != "" {
|
slog.Warn("could not determine authenticated user for reviewer self-request", "error", err)
|
||||||
if err := giteaClient.RequestReviewer(ctx, owner, repoName, prNumber, authUser); err != nil {
|
} else if authUser != "" {
|
||||||
slog.Warn("could not self-request as reviewer", "user", authUser, "error", err)
|
if err := selfReq.RequestReviewerSelf(ctx, owner, repoName, prNumber, authUser); err != nil {
|
||||||
} else {
|
slog.Warn("could not self-request as reviewer", "user", authUser, "error", err)
|
||||||
slog.Debug("self-requested as reviewer", "user", authUser, "pr", prNumber)
|
} else {
|
||||||
|
slog.Debug("self-requested as reviewer", "user", authUser, "pr", prNumber)
|
||||||
|
sonnet-review-bot
commented
[NIT] The type assertion **[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 {
|
||||||
|
sonnet-review-bot
commented
[MINOR] **[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
[MINOR] The type assertion **[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.
|
|||||||
|
slog.Debug("RequestReviewer not supported for provider, skipping")
|
||||||
|
sonnet-review-bot
commented
[MINOR] The Gitea-specific **[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
[MINOR] When **[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.
|
|||||||
}
|
}
|
||||||
|
|
||||||
// POST new review
|
// POST new review
|
||||||
|
sonnet-review-bot
commented
[MINOR] In **[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').
|
|||||||
slog.Info("posting review", "event", event, "pr", prNumber)
|
slog.Info("posting review", "event", event, "pr", prNumber)
|
||||||
|
sonnet-review-bot
commented
[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.
|
|||||||
posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, inlineComments)
|
reviewReq := vcs.ReviewRequest{
|
||||||
|
Body: reviewBody,
|
||||||
|
sonnet-review-bot
commented
[MINOR] The **[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
[MINOR] The type assertion **[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.
|
|||||||
|
Event: event,
|
||||||
|
Comments: inlineComments,
|
||||||
|
sonnet-review-bot
commented
[MINOR] The **[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
[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 **[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.
|
|||||||
|
}
|
||||||
|
sonnet-review-bot
commented
[MINOR] The **[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.
|
|||||||
|
posted, err := client.PostReview(ctx, owner, repoName, prNumber, reviewReq)
|
||||||
|
sonnet-review-bot
commented
[MINOR] The self-request reviewer logic uses 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.
|
|||||||
if err != nil {
|
if err != nil {
|
||||||
slog.Error("failed to post review", "pr", prNumber, "event", event, "error", err)
|
slog.Error("failed to post review", "pr", prNumber, "event", event, "error", err)
|
||||||
os.Exit(1)
|
os.Exit(1)
|
||||||
}
|
}
|
||||||
|
sonnet-review-bot
commented
[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.
|
|||||||
slog.Info("review posted", "review_id", posted.ID, "user", posted.User.Login, "pr", prNumber)
|
slog.Info("review posted", "review_id", posted.ID, "user", posted.User.Login, "pr", prNumber)
|
||||||
|
sonnet-review-bot
commented
[NIT] The type assertion **[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.
|
|||||||
|
|
||||||
// Supersede all old reviews with link to the new one
|
// Supersede all old reviews via optional interface
|
||||||
if len(oldReviews) > 0 {
|
if len(oldReviews) > 0 {
|
||||||
newReviewURL := fmt.Sprintf("%s/%s/%s/pulls/%d#pullrequestreview-%d", strings.TrimRight(*giteaURL, "/"), owner, repoName, prNumber, posted.ID)
|
if superseder, ok := client.(vcs.ReviewSuperseder); ok {
|
||||||
|
sonnet-review-bot
commented
[MINOR] When **[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.
|
|||||||
for _, oldReview := range oldReviews {
|
if err := superseder.SupersedeReviews(ctx, owner, repoName, prNumber, oldReviews, posted.ID, *vcsURL, sentinel); err != nil {
|
||||||
cid, err := giteaClient.GetTimelineReviewCommentIDForReview(ctx, owner, repoName, prNumber, oldReview.ID)
|
slog.Error("failed to supersede old reviews", "error", err)
|
||||||
if err != nil {
|
os.Exit(1)
|
||||||
|
sonnet-review-bot
commented
[MINOR] The **[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.
|
|||||||
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)
|
|
||||||
|
|
||||||
// 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)
|
|
||||||
}
|
}
|
||||||
|
} else {
|
||||||
|
slog.Error("provider does not support review superseding", "provider", vcsProvider)
|
||||||
|
sonnet-review-bot
commented
[MINOR] In **[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.
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// verdictToEvent maps a verdict string from the LLM response to a canonical vcs.ReviewEvent.
|
||||||
|
func verdictToEvent(verdict string) vcs.ReviewEvent {
|
||||||
|
switch verdict {
|
||||||
|
case "APPROVE":
|
||||||
|
return vcs.ReviewEventApprove
|
||||||
|
case "REQUEST_CHANGES":
|
||||||
|
rodin marked this conversation as resolved
Outdated
[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
[MAJOR] In **[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.
|
|||||||
|
return vcs.ReviewEventRequestChanges
|
||||||
|
default:
|
||||||
|
return vcs.ReviewEventComment
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
gpt-review-bot
commented
[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.
|
|||||||
|
|
||||||
// fetchFileContext fetches the full content of modified files from the PR branch.
|
// 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
|
var sb strings.Builder
|
||||||
for _, f := range files {
|
for _, f := range files {
|
||||||
if ctx.Err() != nil {
|
if ctx.Err() != nil {
|
||||||
@@ -507,7 +529,7 @@ func fetchFileContext(ctx context.Context, client *gitea.Client, owner, repo, re
|
|||||||
if f.Status == "removed" {
|
if f.Status == "removed" {
|
||||||
continue // Skip deleted files
|
continue // Skip deleted files
|
||||||
|
sonnet-review-bot
commented
[MINOR] supersedeOldReviews uses a switch with a **[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
Good catch — reworded from "Fall through to Gitea-specific logic below the switch" to "Continue to Gitea-specific logic below the switch" in 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.
|
|||||||
}
|
}
|
||||||
content, err := client.GetFileContentRef(ctx, owner, repo, f.Filename, ref)
|
content, err := client.GetFileContentAtRef(ctx, owner, repo, f.Filename, ref)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
slog.Warn("could not fetch file content", "file", f.Filename, "error", err)
|
slog.Warn("could not fetch file content", "file", f.Filename, "error", err)
|
||||||
|
sonnet-review-bot
commented
[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.
|
|||||||
continue
|
continue
|
||||||
@@ -524,7 +546,8 @@ func fetchFileContext(ctx context.Context, client *gitea.Client, owner, repo, re
|
|||||||
// patternsRepo is comma-separated list of owner/name repos.
|
// patternsRepo is comma-separated list of owner/name repos.
|
||||||
// patternsFiles is comma-separated list of file paths or directories.
|
// 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.
|
// If a path ends with / or is a directory, all files within it are fetched recursively.
|
||||||
|
gpt-review-bot
commented
[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.
|
|||||||
func fetchPatterns(ctx context.Context, client *gitea.Client, patternsRepo, patternsFiles string) string {
|
// Empty entries in patternsFiles are skipped (no implicit repo-root fetch).
|
||||||
|
func fetchPatterns(ctx context.Context, client vcs.FileReader, patternsRepo, patternsFiles string) string {
|
||||||
var sb strings.Builder
|
var sb strings.Builder
|
||||||
|
|
||||||
repos := strings.Split(patternsRepo, ",")
|
repos := strings.Split(patternsRepo, ",")
|
||||||
@@ -554,7 +577,7 @@ func fetchPatterns(ctx context.Context, client *gitea.Client, patternsRepo, patt
|
|||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
|
|
||||||
files, err := client.GetAllFilesInPath(ctx, owner, repo, path)
|
files, err := vcs.GetAllFilesInPath(ctx, client, owner, repo, path)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
slog.Warn("could not fetch patterns", "path", path, "repo", repoRef, "error", err)
|
slog.Warn("could not fetch patterns", "path", path, "repo", repoRef, "error", err)
|
||||||
continue
|
continue
|
||||||
@@ -593,18 +616,20 @@ func isPatternFile(path string) bool {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// evaluateCIStatus checks if all CI statuses indicate success.
|
// evaluateCIStatus checks if all CI statuses indicate success.
|
||||||
func evaluateCIStatus(statuses []gitea.CommitStatus) (passed bool, details string) {
|
// Returns passed=true if no checks have failed (pending checks are not treated as failures).
|
||||||
|
func evaluateCIStatus(statuses []vcs.CommitStatus) (passed bool, details string) {
|
||||||
if len(statuses) == 0 {
|
if len(statuses) == 0 {
|
||||||
return true, "no CI statuses found"
|
return true, "no CI statuses found"
|
||||||
}
|
}
|
||||||
|
|
||||||
var failed []string
|
var failed []string
|
||||||
|
var pending int
|
||||||
for _, s := range statuses {
|
for _, s := range statuses {
|
||||||
switch s.Status {
|
switch s.Status {
|
||||||
case "success":
|
case "success":
|
||||||
// good
|
// good
|
||||||
case "pending":
|
case "pending":
|
||||||
// treat pending as not-failed
|
pending++
|
||||||
case "failure", "error":
|
case "failure", "error":
|
||||||
failed = append(failed, fmt.Sprintf("%s: %s", s.Context, s.Description))
|
failed = append(failed, fmt.Sprintf("%s: %s", s.Context, s.Description))
|
||||||
}
|
}
|
||||||
@@ -613,6 +638,9 @@ func evaluateCIStatus(statuses []gitea.CommitStatus) (passed bool, details strin
|
|||||||
if len(failed) > 0 {
|
if len(failed) > 0 {
|
||||||
return false, strings.Join(failed, "; ")
|
return false, strings.Join(failed, "; ")
|
||||||
}
|
}
|
||||||
|
if pending > 0 {
|
||||||
|
return true, fmt.Sprintf("no failures (%d pending)", pending)
|
||||||
|
}
|
||||||
return true, "all checks passed"
|
return true, "all checks passed"
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -643,14 +671,6 @@ func envOrDefaultInt(key string, defaultVal int) int {
|
|||||||
return defaultVal
|
return defaultVal
|
||||||
}
|
}
|
||||||
|
|
||||||
func envOrDefaultBool(key string, defaultVal bool) bool {
|
|
||||||
v := strings.TrimSpace(strings.ToLower(os.Getenv(key)))
|
|
||||||
if v == "" {
|
|
||||||
return defaultVal
|
|
||||||
}
|
|
||||||
return v == "true" || v == "1" || v == "yes"
|
|
||||||
}
|
|
||||||
|
|
||||||
// validateReviewerName checks that the name contains only safe characters
|
// validateReviewerName checks that the name contains only safe characters
|
||||||
// for embedding in an HTML comment sentinel ([a-zA-Z0-9_-]).
|
// for embedding in an HTML comment sentinel ([a-zA-Z0-9_-]).
|
||||||
func validateReviewerName(name string) error {
|
func validateReviewerName(name string) error {
|
||||||
@@ -702,36 +722,11 @@ func validateWorkspacePath(path, pathName string) (string, error) {
|
|||||||
return resolvedPath, nil
|
return resolvedPath, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
// buildSupersededBody creates the body for a superseded review: struck-through banner
|
|
||||||
// with collapsed original content and the commit it was evaluated against.
|
|
||||||
func buildSupersededBody(originalBody, commitSHA, newReviewURL, sentinel string) string {
|
|
||||||
shortSHA := commitSHA
|
|
||||||
if len(shortSHA) > 8 {
|
|
||||||
shortSHA = shortSHA[:8]
|
|
||||||
}
|
|
||||||
var sb strings.Builder
|
|
||||||
sb.WriteString("~~Original review~~\n\n")
|
|
||||||
sb.WriteString("**Superseded** \u2014 [see current review](")
|
|
||||||
sb.WriteString(newReviewURL)
|
|
||||||
sb.WriteString(") for up-to-date findings.\n\n")
|
|
||||||
if shortSHA != "" {
|
|
||||||
sb.WriteString("<details><summary>Previous findings (commit ")
|
|
||||||
sb.WriteString(shortSHA)
|
|
||||||
sb.WriteString(")</summary>\n\n")
|
|
||||||
} else {
|
|
||||||
sb.WriteString("<details><summary>Previous findings</summary>\n\n")
|
|
||||||
}
|
|
||||||
sb.WriteString(originalBody)
|
|
||||||
sb.WriteString("\n\n</details>\n\n")
|
|
||||||
sb.WriteString(sentinel)
|
|
||||||
return sb.String()
|
|
||||||
}
|
|
||||||
|
|
||||||
// hasSharedToken detects if another review-bot role posted under the same
|
// hasSharedToken detects if another review-bot role posted under the same
|
||||||
// Gitea user. This indicates misconfiguration where two roles share a token
|
// VCS user. This indicates misconfiguration where two roles share a token
|
||||||
// instead of having separate Gitea accounts. Returns true if shared token
|
// instead of having separate accounts. Returns true if shared token
|
||||||
// detected (caller should skip update-in-place logic to avoid clobbering).
|
// 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 := ""
|
ownLogin := ""
|
||||||
for _, r := range reviews {
|
for _, r := range reviews {
|
||||||
if strings.Contains(r.Body, ownSentinel) {
|
if strings.Contains(r.Body, ownSentinel) {
|
||||||
@@ -744,7 +739,7 @@ func hasSharedToken(reviews []gitea.Review, ownSentinel string) bool {
|
|||||||
}
|
}
|
||||||
for _, r := range reviews {
|
for _, r := range reviews {
|
||||||
if r.User.Login == ownLogin && strings.Contains(r.Body, "<!-- review-bot:") && !strings.Contains(r.Body, ownSentinel) {
|
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)
|
"sibling_role", extractSentinelName(r.Body), "user", ownLogin)
|
||||||
return true
|
return true
|
||||||
}
|
}
|
||||||
@@ -765,29 +760,26 @@ func extractSentinelName(body string) string {
|
|||||||
if end < 0 {
|
if end < 0 {
|
||||||
return "unknown"
|
return "unknown"
|
||||||
}
|
}
|
||||||
return rest[:end]
|
name := rest[:end]
|
||||||
}
|
// Sanitize: strip control characters to prevent log injection.
|
||||||
|
name = strings.Map(func(r rune) rune {
|
||||||
// findOwnReview locates the most recent non-superseded review matching the sentinel.
|
if r < 0x20 || r == 0x7f {
|
||||||
func findOwnReview(reviews []gitea.Review, sentinel string) *gitea.Review {
|
return -1
|
||||||
var best *gitea.Review
|
|
||||||
for i := range reviews {
|
|
||||||
if !strings.Contains(reviews[i].Body, sentinel) {
|
|
||||||
continue
|
|
||||||
}
|
|
||||||
if strings.Contains(reviews[i].Body, "~~Original review~~") {
|
|
||||||
continue
|
|
||||||
}
|
|
||||||
if best == nil || reviews[i].ID > best.ID {
|
|
||||||
best = &reviews[i]
|
|
||||||
}
|
}
|
||||||
|
return r
|
||||||
|
}, name)
|
||||||
|
if len(name) > 64 {
|
||||||
|
name = name[:64]
|
||||||
}
|
}
|
||||||
return best
|
if name == "" {
|
||||||
|
return "unknown"
|
||||||
|
}
|
||||||
|
return name
|
||||||
}
|
}
|
||||||
|
|
||||||
// findAllOwnReviews returns all non-superseded reviews matching the sentinel.
|
// findAllOwnReviews returns all non-superseded reviews matching the sentinel.
|
||||||
func findAllOwnReviews(reviews []gitea.Review, sentinel string) []gitea.Review {
|
func findAllOwnReviews(reviews []vcs.Review, sentinel string) []vcs.Review {
|
||||||
var result []gitea.Review
|
var result []vcs.Review
|
||||||
for i := range reviews {
|
for i := range reviews {
|
||||||
if !strings.Contains(reviews[i].Body, sentinel) {
|
if !strings.Contains(reviews[i].Body, sentinel) {
|
||||||
continue
|
continue
|
||||||
@@ -812,35 +804,3 @@ func shouldSkipStaleReview(evaluatedSHA, currentSHA string) bool {
|
|||||||
}
|
}
|
||||||
return evaluatedSHA != currentSHA
|
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)
|
|
||||||
}
|
|
||||||
|
|||||||
@@ -10,7 +10,7 @@ import (
|
|||||||
"strings"
|
"strings"
|
||||||
"testing"
|
"testing"
|
||||||
|
|
||||||
"gitea.weiker.me/rodin/review-bot/gitea"
|
"gitea.weiker.me/rodin/review-bot/vcs"
|
||||||
)
|
)
|
||||||
|
|
||||||
func TestValidateReviewerName(t *testing.T) {
|
func TestValidateReviewerName(t *testing.T) {
|
||||||
@@ -107,9 +107,7 @@ func TestValidateWorkspacePath(t *testing.T) {
|
|||||||
workspace: tmpDir,
|
workspace: tmpDir,
|
||||||
path: "/etc/passwd",
|
path: "/etc/passwd",
|
||||||
wantErr: true,
|
wantErr: true,
|
||||||
// Go 1.21+ filepath.Join normalizes absolute paths: Join("/tmp/x", "/etc/passwd")
|
errMatch: "failed to resolve",
|
||||||
// becomes "/tmp/x/etc/passwd", which is within workspace but doesn't exist.
|
|
||||||
errMatch: "failed to resolve",
|
|
||||||
},
|
},
|
||||||
{
|
{
|
||||||
name: "nonexistent file",
|
name: "nonexistent file",
|
||||||
@@ -154,155 +152,20 @@ func TestValidateWorkspacePath(t *testing.T) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
func makeReview(id int64, login, state string, stale bool, body string) gitea.Review {
|
func makeReview(id int64, login, state string, stale bool, body string) vcs.Review {
|
||||||
r := gitea.Review{
|
return vcs.Review{
|
||||||
ID: id,
|
ID: id,
|
||||||
Body: body,
|
Body: body,
|
||||||
|
User: vcs.UserInfo{Login: login},
|
||||||
State: state,
|
State: state,
|
||||||
Stale: stale,
|
Stale: stale,
|
||||||
}
|
}
|
||||||
r.User.Login = login
|
|
||||||
return r
|
|
||||||
}
|
|
||||||
|
|
||||||
func TestBuildSupersededBody(t *testing.T) {
|
|
||||||
original := "# Review\n\nLooks good.\n\n<!-- review-bot:sonnet -->"
|
|
||||||
sentinel := "<!-- review-bot:sonnet -->"
|
|
||||||
newURL := "https://gitea.example.com/owner/repo/pulls/1#pullrequestreview-99"
|
|
||||||
|
|
||||||
result := buildSupersededBody(original, "abcdef1234567890", newURL, sentinel)
|
|
||||||
|
|
||||||
// Should contain the struck-through banner
|
|
||||||
if !strings.Contains(result, "~~Original review~~") {
|
|
||||||
t.Error("missing struck-through banner")
|
|
||||||
}
|
|
||||||
// Should contain superseded notice with link
|
|
||||||
if !strings.Contains(result, "**Superseded**") {
|
|
||||||
t.Error("missing superseded notice")
|
|
||||||
}
|
|
||||||
if !strings.Contains(result, "[see current review]("+newURL+")") {
|
|
||||||
t.Error("missing link to new review")
|
|
||||||
}
|
|
||||||
// Should contain collapsed original
|
|
||||||
if !strings.Contains(result, "<details>") {
|
|
||||||
t.Error("missing details/collapse")
|
|
||||||
}
|
|
||||||
// Should contain short commit SHA
|
|
||||||
if !strings.Contains(result, "abcdef12") {
|
|
||||||
t.Error("missing short SHA")
|
|
||||||
}
|
|
||||||
// Should NOT contain full SHA
|
|
||||||
if strings.Contains(result, "abcdef1234567890") {
|
|
||||||
t.Error("should truncate SHA to 8 chars")
|
|
||||||
}
|
|
||||||
// Should contain the original body inside details
|
|
||||||
if !strings.Contains(result, original) {
|
|
||||||
t.Error("original body not preserved in collapsed section")
|
|
||||||
}
|
|
||||||
// Should end with sentinel
|
|
||||||
if !strings.Contains(result, sentinel) {
|
|
||||||
t.Error("missing sentinel")
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
func TestBuildSupersededBodyShortSHA(t *testing.T) {
|
|
||||||
// Short SHA should pass through without panic
|
|
||||||
result := buildSupersededBody("body", "abc", "https://example.com/review", "<!-- review-bot:x -->")
|
|
||||||
if !strings.Contains(result, "abc") {
|
|
||||||
t.Error("short SHA not preserved")
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
func TestFindOwnReview(t *testing.T) {
|
|
||||||
tests := []struct {
|
|
||||||
name string
|
|
||||||
reviews []gitea.Review
|
|
||||||
sentinel string
|
|
||||||
wantID int64
|
|
||||||
wantNil bool
|
|
||||||
}{
|
|
||||||
{
|
|
||||||
name: "no reviews",
|
|
||||||
reviews: nil,
|
|
||||||
sentinel: "<!-- review-bot:sonnet -->",
|
|
||||||
wantNil: true,
|
|
||||||
},
|
|
||||||
{
|
|
||||||
name: "found by sentinel",
|
|
||||||
reviews: []gitea.Review{
|
|
||||||
makeReview(42, "bot", "APPROVED", false, "review body\n<!-- review-bot:sonnet -->"),
|
|
||||||
},
|
|
||||||
sentinel: "<!-- review-bot:sonnet -->",
|
|
||||||
wantID: 42,
|
|
||||||
},
|
|
||||||
{
|
|
||||||
name: "wrong sentinel",
|
|
||||||
reviews: []gitea.Review{
|
|
||||||
makeReview(42, "bot", "APPROVED", false, "body\n<!-- review-bot:gpt -->"),
|
|
||||||
},
|
|
||||||
sentinel: "<!-- review-bot:sonnet -->",
|
|
||||||
wantNil: true,
|
|
||||||
},
|
|
||||||
{
|
|
||||||
name: "multiple reviews, returns first match",
|
|
||||||
reviews: []gitea.Review{
|
|
||||||
makeReview(10, "bot", "APPROVED", false, "old\n<!-- review-bot:gpt -->"),
|
|
||||||
makeReview(20, "bot", "APPROVED", false, "new\n<!-- review-bot:sonnet -->"),
|
|
||||||
},
|
|
||||||
sentinel: "<!-- review-bot:sonnet -->",
|
|
||||||
wantID: 20,
|
|
||||||
},
|
|
||||||
{
|
|
||||||
name: "skips superseded review",
|
|
||||||
reviews: []gitea.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 -->"),
|
|
||||||
},
|
|
||||||
sentinel: "<!-- review-bot:sonnet -->",
|
|
||||||
wantID: 20,
|
|
||||||
},
|
|
||||||
{
|
|
||||||
name: "only superseded reviews exist",
|
|
||||||
reviews: []gitea.Review{
|
|
||||||
makeReview(10, "bot", "APPROVED", false, "~~Original review~~\n\n<!-- review-bot:sonnet -->"),
|
|
||||||
},
|
|
||||||
sentinel: "<!-- review-bot:sonnet -->",
|
|
||||||
wantNil: true,
|
|
||||||
},
|
|
||||||
{
|
|
||||||
name: "picks highest ID among matches",
|
|
||||||
reviews: []gitea.Review{
|
|
||||||
makeReview(50, "bot", "APPROVED", false, "v1\n<!-- review-bot:sonnet -->"),
|
|
||||||
makeReview(30, "bot", "APPROVED", false, "v0\n<!-- review-bot:sonnet -->"),
|
|
||||||
},
|
|
||||||
sentinel: "<!-- review-bot:sonnet -->",
|
|
||||||
wantID: 50,
|
|
||||||
},
|
|
||||||
}
|
|
||||||
|
|
||||||
for _, tc := range tests {
|
|
||||||
t.Run(tc.name, func(t *testing.T) {
|
|
||||||
got := findOwnReview(tc.reviews, tc.sentinel)
|
|
||||||
if tc.wantNil {
|
|
||||||
if got != nil {
|
|
||||||
t.Errorf("findOwnReview() = %v, want nil", got)
|
|
||||||
}
|
|
||||||
} else {
|
|
||||||
if got == nil {
|
|
||||||
t.Fatal("findOwnReview() = nil, want non-nil")
|
|
||||||
}
|
|
||||||
if got.ID != tc.wantID {
|
|
||||||
t.Errorf("findOwnReview().ID = %d, want %d", got.ID, tc.wantID)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
})
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
||||||
func TestHasSharedToken(t *testing.T) {
|
func TestHasSharedToken(t *testing.T) {
|
||||||
tests := []struct {
|
tests := []struct {
|
||||||
name string
|
name string
|
||||||
reviews []gitea.Review
|
reviews []vcs.Review
|
||||||
sentinel string
|
sentinel string
|
||||||
want bool
|
want bool
|
||||||
}{
|
}{
|
||||||
@@ -314,36 +177,36 @@ func TestHasSharedToken(t *testing.T) {
|
|||||||
},
|
},
|
||||||
{
|
{
|
||||||
name: "no own review yet - cannot detect",
|
name: "no own review yet - cannot detect",
|
||||||
reviews: []gitea.Review{
|
reviews: []vcs.Review{
|
||||||
{ID: 1, User: struct{ Login string `json:"login"` }{Login: "other"}, Body: "<!-- review-bot:gpt --> body"},
|
makeReview(1, "other", "APPROVED", false, "<!-- review-bot:gpt --> body"),
|
||||||
},
|
},
|
||||||
sentinel: "<!-- review-bot:sonnet -->",
|
sentinel: "<!-- review-bot:sonnet -->",
|
||||||
want: false,
|
want: false,
|
||||||
},
|
},
|
||||||
{
|
{
|
||||||
name: "separate users - no shared token",
|
name: "separate users - no shared token",
|
||||||
reviews: []gitea.Review{
|
reviews: []vcs.Review{
|
||||||
{ID: 1, User: struct{ Login string `json:"login"` }{Login: "sonnet-review-bot"}, Body: "<!-- review-bot:sonnet --> body"},
|
makeReview(1, "sonnet-review-bot", "APPROVED", false, "<!-- review-bot:sonnet --> body"),
|
||||||
{ID: 2, User: struct{ Login string `json:"login"` }{Login: "security-review-bot"}, Body: "<!-- review-bot:security --> body"},
|
makeReview(2, "security-review-bot", "APPROVED", false, "<!-- review-bot:security --> body"),
|
||||||
},
|
},
|
||||||
sentinel: "<!-- review-bot:sonnet -->",
|
sentinel: "<!-- review-bot:sonnet -->",
|
||||||
want: false,
|
want: false,
|
||||||
},
|
},
|
||||||
{
|
{
|
||||||
name: "shared token detected - same user different sentinels",
|
name: "shared token detected - same user different sentinels",
|
||||||
reviews: []gitea.Review{
|
reviews: []vcs.Review{
|
||||||
{ID: 1, User: struct{ Login string `json:"login"` }{Login: "sonnet-review-bot"}, Body: "<!-- review-bot:sonnet --> body"},
|
makeReview(1, "sonnet-review-bot", "APPROVED", false, "<!-- review-bot:sonnet --> body"),
|
||||||
{ID: 2, User: struct{ Login string `json:"login"` }{Login: "sonnet-review-bot"}, Body: "<!-- review-bot:security --> body"},
|
makeReview(2, "sonnet-review-bot", "APPROVED", false, "<!-- review-bot:security --> body"),
|
||||||
},
|
},
|
||||||
sentinel: "<!-- review-bot:sonnet -->",
|
sentinel: "<!-- review-bot:sonnet -->",
|
||||||
want: true,
|
want: true,
|
||||||
},
|
},
|
||||||
{
|
{
|
||||||
name: "three roles same user",
|
name: "three roles same user",
|
||||||
reviews: []gitea.Review{
|
reviews: []vcs.Review{
|
||||||
{ID: 1, User: struct{ Login string `json:"login"` }{Login: "bot"}, Body: "<!-- review-bot:sonnet --> body"},
|
makeReview(1, "bot", "APPROVED", false, "<!-- review-bot:sonnet --> body"),
|
||||||
{ID: 2, User: struct{ Login string `json:"login"` }{Login: "bot"}, Body: "<!-- review-bot:security --> body"},
|
makeReview(2, "bot", "APPROVED", false, "<!-- review-bot:security --> body"),
|
||||||
{ID: 3, User: struct{ Login string `json:"login"` }{Login: "bot"}, Body: "<!-- review-bot:gpt --> body"},
|
makeReview(3, "bot", "APPROVED", false, "<!-- review-bot:gpt --> body"),
|
||||||
},
|
},
|
||||||
sentinel: "<!-- review-bot:sonnet -->",
|
sentinel: "<!-- review-bot:sonnet -->",
|
||||||
want: true,
|
want: true,
|
||||||
@@ -507,7 +370,7 @@ func TestIsPatternFile(t *testing.T) {
|
|||||||
func TestEvaluateCIStatus(t *testing.T) {
|
func TestEvaluateCIStatus(t *testing.T) {
|
||||||
tests := []struct {
|
tests := []struct {
|
||||||
name string
|
name string
|
||||||
statuses []gitea.CommitStatus
|
statuses []vcs.CommitStatus
|
||||||
wantPassed bool
|
wantPassed bool
|
||||||
wantSubstr string
|
wantSubstr string
|
||||||
}{
|
}{
|
||||||
@@ -519,7 +382,7 @@ func TestEvaluateCIStatus(t *testing.T) {
|
|||||||
},
|
},
|
||||||
{
|
{
|
||||||
name: "all success",
|
name: "all success",
|
||||||
statuses: []gitea.CommitStatus{
|
statuses: []vcs.CommitStatus{
|
||||||
{Status: "success", Context: "ci/build", Description: "Build passed"},
|
{Status: "success", Context: "ci/build", Description: "Build passed"},
|
||||||
{Status: "success", Context: "ci/test", Description: "Tests passed"},
|
{Status: "success", Context: "ci/test", Description: "Tests passed"},
|
||||||
},
|
},
|
||||||
@@ -528,7 +391,7 @@ func TestEvaluateCIStatus(t *testing.T) {
|
|||||||
},
|
},
|
||||||
{
|
{
|
||||||
name: "one failure",
|
name: "one failure",
|
||||||
statuses: []gitea.CommitStatus{
|
statuses: []vcs.CommitStatus{
|
||||||
{Status: "success", Context: "ci/build", Description: "Build passed"},
|
{Status: "success", Context: "ci/build", Description: "Build passed"},
|
||||||
{Status: "failure", Context: "ci/test", Description: "Tests failed"},
|
{Status: "failure", Context: "ci/test", Description: "Tests failed"},
|
||||||
},
|
},
|
||||||
@@ -537,7 +400,7 @@ func TestEvaluateCIStatus(t *testing.T) {
|
|||||||
},
|
},
|
||||||
{
|
{
|
||||||
name: "error status",
|
name: "error status",
|
||||||
statuses: []gitea.CommitStatus{
|
statuses: []vcs.CommitStatus{
|
||||||
{Status: "error", Context: "ci/lint", Description: "Lint error"},
|
{Status: "error", Context: "ci/lint", Description: "Lint error"},
|
||||||
},
|
},
|
||||||
wantPassed: false,
|
wantPassed: false,
|
||||||
@@ -545,16 +408,16 @@ func TestEvaluateCIStatus(t *testing.T) {
|
|||||||
},
|
},
|
||||||
{
|
{
|
||||||
name: "pending treated as not-failed",
|
name: "pending treated as not-failed",
|
||||||
statuses: []gitea.CommitStatus{
|
statuses: []vcs.CommitStatus{
|
||||||
{Status: "pending", Context: "ci/build", Description: "In progress"},
|
{Status: "pending", Context: "ci/build", Description: "In progress"},
|
||||||
{Status: "success", Context: "ci/test", Description: "Tests passed"},
|
{Status: "success", Context: "ci/test", Description: "Tests passed"},
|
||||||
},
|
},
|
||||||
wantPassed: true,
|
wantPassed: true,
|
||||||
|
sonnet-review-bot
commented
[NIT] **[NIT]** `TestBuildPatternPaths` duplicates the path-building logic from `fetchPatterns` inline rather than exercising the real function. This tests a copy of the logic, not the actual implementation. If `fetchPatterns` is ever refactored, this test could pass while the real code breaks. Consider extracting the path-building logic into a helper function in main.go and testing that directly, or restructuring `fetchPatterns` to be testable.
sonnet-review-bot
commented
[NIT] **[NIT]** `TestBuildPatternPaths` duplicates the path-building logic from `fetchPatterns` in a local closure rather than testing `fetchPatterns` directly or extracting the logic into a named helper. This means the test can diverge from the production code without failing. Consider extracting the path-building into a small private function (`buildPatternPaths`) and testing that directly via `export_test.go`, or wiring the test through `fetchPatterns` with a mock `vcs.FileReader`.
|
|||||||
wantSubstr: "all checks passed",
|
wantSubstr: "no failures",
|
||||||
},
|
},
|
||||||
{
|
{
|
||||||
name: "multiple failures",
|
name: "multiple failures",
|
||||||
statuses: []gitea.CommitStatus{
|
statuses: []vcs.CommitStatus{
|
||||||
{Status: "failure", Context: "ci/build", Description: "Build failed"},
|
{Status: "failure", Context: "ci/build", Description: "Build failed"},
|
||||||
{Status: "failure", Context: "ci/test", Description: "Tests failed"},
|
{Status: "failure", Context: "ci/test", Description: "Tests failed"},
|
||||||
},
|
},
|
||||||
@@ -563,7 +426,7 @@ func TestEvaluateCIStatus(t *testing.T) {
|
|||||||
},
|
},
|
||||||
{
|
{
|
||||||
name: "mixed with pending and failure",
|
name: "mixed with pending and failure",
|
||||||
statuses: []gitea.CommitStatus{
|
statuses: []vcs.CommitStatus{
|
||||||
{Status: "success", Context: "ci/build", Description: "Build passed"},
|
{Status: "success", Context: "ci/build", Description: "Build passed"},
|
||||||
{Status: "pending", Context: "ci/deploy", Description: "Deploying"},
|
{Status: "pending", Context: "ci/deploy", Description: "Deploying"},
|
||||||
{Status: "failure", Context: "ci/test", Description: "Tests failed"},
|
{Status: "failure", Context: "ci/test", Description: "Tests failed"},
|
||||||
@@ -685,47 +548,6 @@ func TestEnvOrDefaultInt(t *testing.T) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
func TestEnvOrDefaultBool(t *testing.T) {
|
|
||||||
tests := []struct {
|
|
||||||
name string
|
|
||||||
envVal string
|
|
||||||
setEnv bool
|
|
||||||
defaultVal bool
|
|
||||||
want bool
|
|
||||||
}{
|
|
||||||
{"unset returns default true", "", false, true, true},
|
|
||||||
{"unset returns default false", "", false, false, false},
|
|
||||||
{"true", "true", true, false, true},
|
|
||||||
{"TRUE", "TRUE", true, false, true},
|
|
||||||
{"True", "True", true, false, true},
|
|
||||||
{"1", "1", true, false, true},
|
|
||||||
{"yes", "yes", true, false, true},
|
|
||||||
{"YES", "YES", true, false, true},
|
|
||||||
{"false", "false", true, true, false},
|
|
||||||
{"0", "0", true, true, false},
|
|
||||||
{"no", "no", true, true, false},
|
|
||||||
{"random string", "random", true, true, false},
|
|
||||||
{"empty string returns default", "", true, true, true},
|
|
||||||
{"whitespace true", " true ", true, false, true},
|
|
||||||
}
|
|
||||||
|
|
||||||
for _, tc := range tests {
|
|
||||||
t.Run(tc.name, func(t *testing.T) {
|
|
||||||
envKey := "TEST_ENV_BOOL_" + strings.ReplaceAll(tc.name, " ", "_")
|
|
||||||
if tc.setEnv {
|
|
||||||
os.Setenv(envKey, tc.envVal)
|
|
||||||
defer os.Unsetenv(envKey)
|
|
||||||
} else {
|
|
||||||
os.Unsetenv(envKey)
|
|
||||||
}
|
|
||||||
got := envOrDefaultBool(envKey, tc.defaultVal)
|
|
||||||
if got != tc.want {
|
|
||||||
t.Errorf("envOrDefaultBool(%q, %v) = %v, want %v", tc.envVal, tc.defaultVal, got, tc.want)
|
|
||||||
}
|
|
||||||
})
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
func TestExtractSentinelName_EdgeCases(t *testing.T) {
|
func TestExtractSentinelName_EdgeCases(t *testing.T) {
|
||||||
tests := []struct {
|
tests := []struct {
|
||||||
body string
|
body string
|
||||||
@@ -734,8 +556,8 @@ func TestExtractSentinelName_EdgeCases(t *testing.T) {
|
|||||||
{"<!-- review-bot:sonnet --> rest", "sonnet"},
|
{"<!-- review-bot:sonnet --> rest", "sonnet"},
|
||||||
{"<!-- review-bot:gpt-review --> rest", "gpt-review"},
|
{"<!-- review-bot:gpt-review --> rest", "gpt-review"},
|
||||||
{"no sentinel here", "unknown"},
|
{"no sentinel here", "unknown"},
|
||||||
{"<!-- review-bot:", "unknown"}, // prefix but no suffix
|
{"<!-- review-bot:", "unknown"}, // prefix but no suffix
|
||||||
{"prefix <!-- review-bot:abc --> end", "abc"}, // embedded in text
|
{"prefix <!-- review-bot:abc --> end", "abc"}, // embedded in text
|
||||||
}
|
}
|
||||||
|
|
||||||
for _, tc := range tests {
|
for _, tc := range tests {
|
||||||
@@ -792,7 +614,7 @@ func TestMainSubprocess_InvalidReviewerName(t *testing.T) {
|
|||||||
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
|
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
|
||||||
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
|
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
|
||||||
os.Args = []string{"review-bot",
|
os.Args = []string{"review-bot",
|
||||||
"--gitea-url", "http://localhost",
|
"--vcs-url", "http://localhost",
|
||||||
"--repo", "owner/repo",
|
"--repo", "owner/repo",
|
||||||
"--pr", "1",
|
"--pr", "1",
|
||||||
"--reviewer-name", "invalid name",
|
"--reviewer-name", "invalid name",
|
||||||
@@ -820,7 +642,7 @@ func TestMainSubprocess_InvalidRepo(t *testing.T) {
|
|||||||
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
|
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
|
||||||
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
|
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
|
||||||
os.Args = []string{"review-bot",
|
os.Args = []string{"review-bot",
|
||||||
"--gitea-url", "http://localhost",
|
"--vcs-url", "http://localhost",
|
||||||
"--repo", "invalidrepo",
|
"--repo", "invalidrepo",
|
||||||
"--pr", "1",
|
"--pr", "1",
|
||||||
"--reviewer-token", "tok",
|
"--reviewer-token", "tok",
|
||||||
@@ -847,7 +669,7 @@ func TestMainSubprocess_InvalidPRNumber(t *testing.T) {
|
|||||||
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
|
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
|
||||||
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
|
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
|
||||||
os.Args = []string{"review-bot",
|
os.Args = []string{"review-bot",
|
||||||
"--gitea-url", "http://localhost",
|
"--vcs-url", "http://localhost",
|
||||||
"--repo", "owner/repo",
|
"--repo", "owner/repo",
|
||||||
"--pr", "notanumber",
|
"--pr", "notanumber",
|
||||||
"--reviewer-token", "tok",
|
"--reviewer-token", "tok",
|
||||||
@@ -874,7 +696,7 @@ func TestMainSubprocess_InvalidTemperature(t *testing.T) {
|
|||||||
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
|
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
|
||||||
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
|
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
|
||||||
os.Args = []string{"review-bot",
|
os.Args = []string{"review-bot",
|
||||||
"--gitea-url", "http://localhost",
|
"--vcs-url", "http://localhost",
|
||||||
"--repo", "owner/repo",
|
"--repo", "owner/repo",
|
||||||
"--pr", "1",
|
"--pr", "1",
|
||||||
"--reviewer-token", "tok",
|
"--reviewer-token", "tok",
|
||||||
@@ -902,7 +724,7 @@ func TestMainSubprocess_InvalidProvider(t *testing.T) {
|
|||||||
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
|
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
|
||||||
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
|
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
|
||||||
os.Args = []string{"review-bot",
|
os.Args = []string{"review-bot",
|
||||||
"--gitea-url", "http://localhost",
|
"--vcs-url", "http://localhost",
|
||||||
"--repo", "owner/repo",
|
"--repo", "owner/repo",
|
||||||
"--pr", "1",
|
"--pr", "1",
|
||||||
"--reviewer-token", "tok",
|
"--reviewer-token", "tok",
|
||||||
@@ -926,7 +748,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.
|
// interfere with testing missing-flag scenarios.
|
||||||
func cleanEnv() []string {
|
func cleanEnv() []string {
|
||||||
var env []string
|
var env []string
|
||||||
@@ -934,6 +784,7 @@ func cleanEnv() []string {
|
|||||||
key := strings.SplitN(e, "=", 2)[0]
|
key := strings.SplitN(e, "=", 2)[0]
|
||||||
switch {
|
switch {
|
||||||
case strings.HasPrefix(key, "GITEA_"),
|
case strings.HasPrefix(key, "GITEA_"),
|
||||||
|
strings.HasPrefix(key, "VCS_"),
|
||||||
strings.HasPrefix(key, "LLM_"),
|
strings.HasPrefix(key, "LLM_"),
|
||||||
strings.HasPrefix(key, "REVIEWER_"),
|
strings.HasPrefix(key, "REVIEWER_"),
|
||||||
strings.HasPrefix(key, "PR_"),
|
strings.HasPrefix(key, "PR_"),
|
||||||
@@ -951,12 +802,12 @@ func cleanEnv() []string {
|
|||||||
}
|
}
|
||||||
|
|
||||||
func TestFindAllOwnReviews(t *testing.T) {
|
func TestFindAllOwnReviews(t *testing.T) {
|
||||||
reviews := []gitea.Review{
|
reviews := []vcs.Review{
|
||||||
{ID: 1, Body: "<!-- review-bot:sonnet -->\nfirst review"},
|
makeReview(1, "bot", "APPROVED", false, "<!-- review-bot:sonnet -->\nfirst review"),
|
||||||
{ID: 2, Body: "<!-- review-bot:gpt -->\nother bot"},
|
makeReview(2, "bot", "APPROVED", false, "<!-- review-bot:gpt -->\nother bot"),
|
||||||
{ID: 3, Body: "<!-- review-bot:sonnet -->\nsecond review"},
|
makeReview(3, "bot", "APPROVED", false, "<!-- review-bot:sonnet -->\nsecond review"),
|
||||||
{ID: 4, Body: "~~Original review~~\n<!-- review-bot:sonnet -->\nsuperseded"},
|
makeReview(4, "bot", "APPROVED", false, "~~Original review~~\n<!-- review-bot:sonnet -->\nsuperseded"),
|
||||||
{ID: 5, Body: "<!-- review-bot:sonnet -->\nthird review"},
|
makeReview(5, "bot", "APPROVED", false, "<!-- review-bot:sonnet -->\nthird review"),
|
||||||
}
|
}
|
||||||
|
|
||||||
got := findAllOwnReviews(reviews, "<!-- review-bot:sonnet -->")
|
got := findAllOwnReviews(reviews, "<!-- review-bot:sonnet -->")
|
||||||
@@ -1020,3 +871,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)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
@@ -3,6 +3,8 @@ package gitea
|
|||||||
import (
|
import (
|
||||||
"context"
|
"context"
|
||||||
"fmt"
|
"fmt"
|
||||||
|
"log/slog"
|
||||||
|
"strings"
|
||||||
|
|
||||||
"gitea.weiker.me/rodin/review-bot/vcs"
|
"gitea.weiker.me/rodin/review-bot/vcs"
|
||||||
)
|
)
|
||||||
@@ -16,6 +18,7 @@ type Adapter struct {
|
|||||||
|
|
||||||
// Compile-time interface conformance assertion.
|
// Compile-time interface conformance assertion.
|
||||||
var _ vcs.Client = (*Adapter)(nil)
|
var _ vcs.Client = (*Adapter)(nil)
|
||||||
|
var _ vcs.ReviewerSelfRequester = (*Adapter)(nil)
|
||||||
|
|
||||||
// NewAdapter creates a new Adapter wrapping the given gitea Client.
|
// NewAdapter creates a new Adapter wrapping the given gitea Client.
|
||||||
func NewAdapter(client *Client) *Adapter {
|
func NewAdapter(client *Client) *Adapter {
|
||||||
@@ -230,3 +233,84 @@ func (a *Adapter) DismissReview(ctx context.Context, owner, repo string, number
|
|||||||
func (a *Adapter) GetAuthenticatedUser(ctx context.Context) (string, error) {
|
func (a *Adapter) GetAuthenticatedUser(ctx context.Context) (string, error) {
|
||||||
|
sonnet-review-bot
commented
[NIT] The comment block and both **[NIT]** The comment block and both `var _ vcs.ReviewerSelfRequester = (*Adapter)(nil)` and `var _ vcs.ReviewSuperseder = (*Adapter)(nil)` are declared in the same file but separated by the `SupersedeReviews` implementation. The compile-time assertion for `ReviewerSelfRequester` is near line 19, while the one for `ReviewSuperseder` is mid-file (line 233). Convention (per patterns/style.md and stdlib) is to place all `var _ Interface = ...` assertions together near the top of the file for discoverability.
|
|||||||
return a.client.GetAuthenticatedUser(ctx)
|
return a.client.GetAuthenticatedUser(ctx)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// RequestReviewerSelf adds the given user as a requested reviewer on a pull request.
|
||||||
|
// This implements vcs.ReviewerSelfRequester for the Gitea adapter.
|
||||||
|
func (a *Adapter) RequestReviewerSelf(ctx context.Context, owner, repo string, number int, user string) error {
|
||||||
|
return a.client.RequestReviewer(ctx, owner, repo, number, user)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Compile-time interface conformance assertion for ReviewSuperseder.
|
||||||
|
var _ vcs.ReviewSuperseder = (*Adapter)(nil)
|
||||||
|
|
||||||
|
// SupersedeReviews marks prior reviews as superseded by editing their body with a
|
||||||
|
// link to the new review and resolving their inline comments. This is Gitea-specific
|
||||||
|
// behavior that has no GitHub equivalent (GitHub uses DismissReview instead).
|
||||||
|
//
|
||||||
|
// baseURL is the Gitea instance URL used to construct review permalink URLs.
|
||||||
|
// sentinel is the HTML comment sentinel that identifies reviews belonging to this reviewer.
|
||||||
|
func (a *Adapter) SupersedeReviews(ctx context.Context, owner, repo string, prNumber int, oldReviews []vcs.Review, newReviewID int64, baseURL, sentinel string) error {
|
||||||
|
// Validate baseURL scheme before embedding in Markdown link (defense-in-depth).
|
||||||
|
if !strings.HasPrefix(baseURL, "http://") && !strings.HasPrefix(baseURL, "https://") {
|
||||||
|
return fmt.Errorf("SupersedeReviews: baseURL must have http or https scheme, got %q", baseURL)
|
||||||
|
}
|
||||||
|
|
||||||
|
sonnet-review-bot
commented
[NIT] **[NIT]** `SupersedeReviews` silently ignores resolution errors for individual inline comments via `_ = underlying.ResolveComment(...)`. The old code in `main.go` tracked `resolved` and `failed` counts and logged them. The new adapter drops that observability entirely. Consider at minimum logging at debug level on resolution failure, matching the pattern used for `EditComment` failures a few lines above.
|
|||||||
|
underlying := a.client
|
||||||
|
|
||||||
|
newReviewURL := fmt.Sprintf("%s/%s/%s/pulls/%d#pullrequestreview-%d",
|
||||||
|
strings.TrimRight(baseURL, "/"), owner, repo, prNumber, newReviewID)
|
||||||
|
|
||||||
|
for _, oldReview := range oldReviews {
|
||||||
|
cid, err := underlying.GetTimelineReviewCommentIDForReview(ctx, owner, repo, 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 := underlying.EditComment(ctx, owner, repo, cid, supersededBody); err != nil {
|
||||||
|
slog.Warn("could not mark old review as superseded", "review_id", oldReview.ID, "error", err)
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
|
||||||
|
// Resolve old review's inline comments
|
||||||
|
oldComments, err := underlying.ListReviewComments(ctx, owner, repo, prNumber, oldReview.ID)
|
||||||
|
if err != nil {
|
||||||
|
slog.Warn("could not list old review comments for resolution", "review_id", oldReview.ID, "error", err)
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
for _, c := range oldComments {
|
||||||
|
if c.ID == 0 {
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
if err := underlying.ResolveComment(ctx, owner, repo, c.ID); err != nil {
|
||||||
|
slog.Debug("could not resolve inline comment", "comment_id", c.ID, "error", err)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return nil
|
||||||
|
}
|
||||||
|
|
||||||
|
// buildSupersededBody creates the body for a superseded review: struck-through banner
|
||||||
|
// with collapsed original content and the commit it was evaluated against.
|
||||||
|
func buildSupersededBody(originalBody, commitSHA, newReviewURL, sentinel string) string {
|
||||||
|
shortSHA := commitSHA
|
||||||
|
if len(shortSHA) > 8 {
|
||||||
|
shortSHA = shortSHA[:8]
|
||||||
|
}
|
||||||
|
var sb strings.Builder
|
||||||
|
sb.WriteString("~~Original review~~\n\n")
|
||||||
|
sb.WriteString("**Superseded** \u2014 [see current review](")
|
||||||
|
sb.WriteString(newReviewURL)
|
||||||
|
sb.WriteString(") for up-to-date findings.\n\n")
|
||||||
|
if shortSHA != "" {
|
||||||
|
sb.WriteString("<details><summary>Previous findings (commit ")
|
||||||
|
sb.WriteString(shortSHA)
|
||||||
|
sb.WriteString(")</summary>\n\n")
|
||||||
|
} else {
|
||||||
|
sb.WriteString("<details><summary>Previous findings</summary>\n\n")
|
||||||
|
}
|
||||||
|
sb.WriteString(originalBody)
|
||||||
|
sb.WriteString("\n\n</details>\n\n")
|
||||||
|
sb.WriteString(sentinel)
|
||||||
|
return sb.String()
|
||||||
|
}
|
||||||
|
|||||||
@@ -386,3 +386,25 @@ func TestAdapter_GetFileContent_RefRouting(t *testing.T) {
|
|||||||
t.Errorf("GetFileContent(ref=\"abc123\") = %q, want %q", got, "content-at-ref")
|
t.Errorf("GetFileContent(ref=\"abc123\") = %q, want %q", got, "content-at-ref")
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func TestAdapter_RequestReviewerSelf(t *testing.T) {
|
||||||
|
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
|
if r.Method != http.MethodPost {
|
||||||
|
t.Errorf("expected POST, got %s", r.Method)
|
||||||
|
}
|
||||||
|
expected := "/api/v1/repos/owner/repo/pulls/5/requested_reviewers"
|
||||||
|
if r.URL.Path != expected {
|
||||||
|
t.Errorf("path = %q, want %q", r.URL.Path, expected)
|
||||||
|
}
|
||||||
|
w.WriteHeader(http.StatusCreated)
|
||||||
|
}))
|
||||||
|
defer server.Close()
|
||||||
|
|
||||||
|
client := gitea.NewClient(server.URL, "token")
|
||||||
|
adapter := gitea.NewAdapter(client)
|
||||||
|
|
||||||
|
err := adapter.RequestReviewerSelf(context.Background(), "owner", "repo", 5, "bot-user")
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("RequestReviewerSelf() error = %v", err)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
@@ -0,0 +1,54 @@
|
|||||||
|
package gitea
|
||||||
|
|
||||||
|
import (
|
||||||
|
"strings"
|
||||||
|
"testing"
|
||||||
|
)
|
||||||
|
|
||||||
|
func TestBuildSupersededBody(t *testing.T) {
|
||||||
|
original := "# Review\n\nLooks good.\n\n<!-- review-bot:sonnet -->"
|
||||||
|
sentinel := "<!-- review-bot:sonnet -->"
|
||||||
|
newURL := "https://gitea.example.com/owner/repo/pulls/1#pullrequestreview-99"
|
||||||
|
|
||||||
|
result := buildSupersededBody(original, "abcdef1234567890", newURL, sentinel)
|
||||||
|
|
||||||
|
// Should contain the struck-through banner
|
||||||
|
if !strings.Contains(result, "~~Original review~~") {
|
||||||
|
t.Error("missing struck-through banner")
|
||||||
|
}
|
||||||
|
// Should contain superseded notice with link
|
||||||
|
if !strings.Contains(result, "**Superseded**") {
|
||||||
|
t.Error("missing superseded notice")
|
||||||
|
}
|
||||||
|
if !strings.Contains(result, "[see current review]("+newURL+")") {
|
||||||
|
t.Error("missing link to new review")
|
||||||
|
}
|
||||||
|
// Should contain collapsed original
|
||||||
|
if !strings.Contains(result, "<details>") {
|
||||||
|
t.Error("missing details/collapse")
|
||||||
|
}
|
||||||
|
// Should contain short commit SHA
|
||||||
|
if !strings.Contains(result, "abcdef12") {
|
||||||
|
t.Error("missing short SHA")
|
||||||
|
}
|
||||||
|
// Should NOT contain full SHA in summary (it's truncated to 8)
|
||||||
|
if strings.Contains(result, "abcdef1234567890") {
|
||||||
|
t.Error("should truncate SHA to 8 chars")
|
||||||
|
}
|
||||||
|
// Should contain the original body inside details
|
||||||
|
if !strings.Contains(result, original) {
|
||||||
|
t.Error("original body not preserved in collapsed section")
|
||||||
|
}
|
||||||
|
// Should end with sentinel
|
||||||
|
if !strings.Contains(result, sentinel) {
|
||||||
|
t.Error("missing sentinel")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestBuildSupersededBodyShortSHA(t *testing.T) {
|
||||||
|
// Short SHA should pass through without panic
|
||||||
|
result := buildSupersededBody("body", "abc", "https://example.com/review", "<!-- review-bot:x -->")
|
||||||
|
if !strings.Contains(result, "abc") {
|
||||||
|
t.Error("short SHA not preserved")
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -6,6 +6,7 @@ package github
|
|||||||
import (
|
import (
|
||||||
"bytes"
|
"bytes"
|
||||||
"context"
|
"context"
|
||||||
|
"encoding/json"
|
||||||
"errors"
|
"errors"
|
||||||
"fmt"
|
"fmt"
|
||||||
"io"
|
"io"
|
||||||
@@ -214,6 +215,11 @@ type requestOptions struct {
|
|||||||
func (c *Client) doRequestCore(ctx context.Context, method, reqURL string, opts requestOptions) ([]byte, error) {
|
func (c *Client) doRequestCore(ctx context.Context, method, reqURL string, opts requestOptions) ([]byte, error) {
|
||||||
const maxRetryAfter = 120 * time.Second
|
const maxRetryAfter = 120 * time.Second
|
||||||
|
|
||||||
|
// maxErrorBodyBytes limits how much of an error response body is stored.
|
||||||
|
// Kept small (4 KiB) to reduce the risk of sensitive data leakage if callers
|
||||||
|
// log APIError.Body directly. Error() further truncates to 200 bytes.
|
||||||
|
const maxErrorBodyBytes = 4 * 1024
|
||||||
|
|
||||||
// backoff holds per-attempt delays: backoff[i] is the delay before attempt i+1.
|
// backoff holds per-attempt delays: backoff[i] is the delay before attempt i+1.
|
||||||
// Length must be maxRetryAttempts-1 (one entry per retry gap).
|
// Length must be maxRetryAttempts-1 (one entry per retry gap).
|
||||||
// SetRetryBackoff validates at configuration time; the default is always valid.
|
// SetRetryBackoff validates at configuration time; the default is always valid.
|
||||||
@@ -227,11 +233,6 @@ func (c *Client) doRequestCore(ctx context.Context, method, reqURL string, opts
|
|||||||
copy(backoff, defaultBackoff)
|
copy(backoff, defaultBackoff)
|
||||||
}
|
}
|
||||||
|
|
||||||
// maxErrorBodyBytes limits how much of an error response body is stored.
|
|
||||||
// Kept small (4 KiB) to reduce the risk of sensitive data leakage if callers
|
|
||||||
// log APIError.Body directly. Error() further truncates to 200 bytes.
|
|
||||||
const maxErrorBodyBytes = 4 * 1024
|
|
||||||
|
|
||||||
// Reject non-HTTPS URLs early since the URL is immutable across retries.
|
// Reject non-HTTPS URLs early since the URL is immutable across retries.
|
||||||
if c.token != "" && !c.allowInsecureHTTP {
|
if c.token != "" && !c.allowInsecureHTTP {
|
||||||
parsed, err := url.Parse(reqURL)
|
parsed, err := url.Parse(reqURL)
|
||||||
@@ -384,3 +385,15 @@ func (c *Client) doRequestWithBody(ctx context.Context, method, reqURL string, r
|
|||||||
}
|
}
|
||||||
return c.doRequestCore(ctx, method, reqURL, opts)
|
return c.doRequestCore(ctx, method, reqURL, opts)
|
||||||
}
|
}
|
||||||
|
gpt-review-bot
commented
[NIT] Minor formatting: there is an extra blank line between the closing brace of doRequestWithBody and the start of doJSONRequest that could be removed for consistency. **[NIT]** Minor formatting: there is an extra blank line between the closing brace of doRequestWithBody and the start of doJSONRequest that could be removed for consistency.
sonnet-review-bot
commented
[NIT] There is a spurious blank line before the closing brace of **[NIT]** There is a spurious blank line before the closing brace of `doRequestWithBody` (after `return c.doRequestCore(...)`). Minor formatting issue that gofmt would not flag since it's inside a function body, but it's visually inconsistent.
|
|||||||
|
|
||||||
|
sonnet-review-bot
commented
[NIT] doJSONRequest is added but not yet called by any public methods in the diff (the github package has no PostReview/DismissReview/etc. visible in the diff). This is fine for an incremental PR, but it means the new helper is untested at the integration level. The unit tests for doJSONRequest itself are present and cover the retry path. **[NIT]** doJSONRequest is added but not yet called by any public methods in the diff (the github package has no PostReview/DismissReview/etc. visible in the diff). This is fine for an incremental PR, but it means the new helper is untested at the integration level. The unit tests for doJSONRequest itself are present and cover the retry path.
|
|||||||
|
// doJSONRequest performs an HTTP request with a JSON body and returns the response body.
|
||||||
|
// It delegates retry/backoff/429 handling to doRequestWithBody.
|
||||||
|
// This is a general-purpose helper used by any method that needs to send JSON payloads
|
||||||
|
sonnet-review-bot
commented
[MINOR] **[MINOR]** `doJSONRequest` does not retry on HTTP 429 like `doRequest` does. Since `PostReview` and `DismissReview` are write operations that can be rate-limited by GitHub, missing retry logic here could cause failures in high-traffic repos. Consider either adding retry logic or documenting that write operations are not retried.
|
|||||||
|
// (e.g. PostReview, DismissReview).
|
||||||
|
func (c *Client) doJSONRequest(ctx context.Context, method, reqURL string, payload any) ([]byte, error) {
|
||||||
|
sonnet-review-bot
commented
[MAJOR] doJSONRequest duplicates ~100 lines of retry logic that already exists in doRequest. Both functions implement identical 429-retry-with-backoff loops, HTTPS validation, and Retry-After header parsing. This violates DRY and doubles the maintenance burden — a future change to retry semantics must be applied in two places. The preferred approach would be to make doRequest accept an optional body (io.Reader) or to extract the shared retry scaffolding into a single private helper that both GET and write operations call. **[MAJOR]** doJSONRequest duplicates ~100 lines of retry logic that already exists in doRequest. Both functions implement identical 429-retry-with-backoff loops, HTTPS validation, and Retry-After header parsing. This violates DRY and doubles the maintenance burden — a future change to retry semantics must be applied in two places. The preferred approach would be to make doRequest accept an optional body (io.Reader) or to extract the shared retry scaffolding into a single private helper that both GET and write operations call.
|
|||||||
|
jsonBody, err := json.Marshal(payload)
|
||||||
|
if err != nil {
|
||||||
|
sonnet-review-bot
commented
[MINOR] **[MINOR]** `doJSONRequest` is added to `client.go` but is only used internally by `review.go` methods. Placing transport-layer helpers (`doRequestCore`, `doRequestWithBody`) together with a higher-level JSON serialisation helper makes the file slightly inconsistent. This is a minor organisation concern — a comment grouping the helpers or moving `doJSONRequest` to `review.go` would improve cohesion, but the current placement is not wrong.
|
|||||||
|
return nil, fmt.Errorf("marshal request body: %w", err)
|
||||||
|
}
|
||||||
|
return c.doRequestWithBody(ctx, method, reqURL, jsonBody)
|
||||||
|
}
|
||||||
|
|||||||
@@ -2,6 +2,7 @@ package github
|
|||||||
|
|
||||||
import (
|
import (
|
||||||
"context"
|
"context"
|
||||||
|
"errors"
|
||||||
"net/http"
|
"net/http"
|
||||||
"net/http/httptest"
|
"net/http/httptest"
|
||||||
"net/url"
|
"net/url"
|
||||||
@@ -567,7 +568,6 @@ func TestSetHTTPClient_NilRestoresDefault(t *testing.T) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
func TestSetRetryBackoff_RejectsInvalidLength(t *testing.T) {
|
func TestSetRetryBackoff_RejectsInvalidLength(t *testing.T) {
|
||||||
c := NewClient("token", "https://api.github.com")
|
c := NewClient("token", "https://api.github.com")
|
||||||
|
|
||||||
@@ -592,3 +592,59 @@ func TestSetRetryBackoff_RejectsInvalidLength(t *testing.T) {
|
|||||||
t.Fatalf("unexpected error for valid backoff: %v", err)
|
t.Fatalf("unexpected error for valid backoff: %v", err)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func TestDoJSONRequest_429Retry(t *testing.T) {
|
||||||
|
attempts := 0
|
||||||
|
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
|
attempts++
|
||||||
|
if attempts < 3 {
|
||||||
|
w.WriteHeader(429)
|
||||||
|
w.Write([]byte(`{"message":"rate limit exceeded"}`))
|
||||||
|
return
|
||||||
|
}
|
||||||
|
w.WriteHeader(200)
|
||||||
|
w.Write([]byte(`{"id":1}`))
|
||||||
|
}))
|
||||||
|
defer ts.Close()
|
||||||
|
|
||||||
|
c := NewClient("token", ts.URL, AllowInsecureHTTP())
|
||||||
|
if err := c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}); err != nil {
|
||||||
|
t.Fatalf("SetRetryBackoff: %v", err)
|
||||||
|
}
|
||||||
|
sonnet-review-bot
commented
[MINOR] **[MINOR]** `c.SetRetryBackoff(...)` return value is not checked in `TestDoJSONRequest_429Retry`. The call on line 613 ignores the error. All other tests in the file use `if err := c.SetRetryBackoff(...); err != nil { t.Fatalf(...) }`. Should be consistent.
|
|||||||
|
|
||||||
|
body, err := c.doJSONRequest(context.Background(), http.MethodPost, ts.URL+"/test", map[string]string{"key": "val"})
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("unexpected error: %v", err)
|
||||||
|
}
|
||||||
|
if attempts != 3 {
|
||||||
|
t.Errorf("expected 3 attempts, got %d", attempts)
|
||||||
|
}
|
||||||
|
if string(body) != `{"id":1}` {
|
||||||
|
t.Errorf("unexpected body: %s", body)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestDoJSONRequest_429ExhaustsRetries(t *testing.T) {
|
||||||
|
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
|
w.WriteHeader(429)
|
||||||
|
w.Write([]byte(`{"message":"rate limit"}`))
|
||||||
|
}))
|
||||||
|
defer ts.Close()
|
||||||
|
|
||||||
|
c := NewClient("token", ts.URL, AllowInsecureHTTP())
|
||||||
|
if err := c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}); err != nil {
|
||||||
|
t.Fatalf("SetRetryBackoff: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
_, err := c.doJSONRequest(context.Background(), http.MethodPost, ts.URL+"/test", map[string]string{"key": "val"})
|
||||||
|
if err == nil {
|
||||||
|
t.Fatal("expected error after exhausting retries")
|
||||||
|
}
|
||||||
|
var apiErr *APIError
|
||||||
|
if !errors.As(err, &apiErr) {
|
||||||
|
t.Fatalf("expected APIError, got %T: %v", err, err)
|
||||||
|
}
|
||||||
|
if apiErr.StatusCode != 429 {
|
||||||
|
t.Errorf("expected 429, got %d", apiErr.StatusCode)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
@@ -9,3 +9,6 @@ import (
|
|||||||
// This verifies github.Client satisfies the full vcs.Client interface
|
// This verifies github.Client satisfies the full vcs.Client interface
|
||||||
// (PRReader, FileReader, Reviewer, Identity).
|
// (PRReader, FileReader, Reviewer, Identity).
|
||||||
var _ vcs.Client = (*github.Client)(nil)
|
var _ vcs.Client = (*github.Client)(nil)
|
||||||
|
|
||||||
|
// Verify github.Client implements ReviewSuperseder.
|
||||||
|
var _ vcs.ReviewSuperseder = (*github.Client)(nil)
|
||||||
|
|||||||
@@ -210,3 +210,16 @@ func (c *Client) DismissReview(ctx context.Context, owner, repo string, number i
|
|||||||
}
|
}
|
||||||
return nil
|
return nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// SupersedeReviews marks prior reviews as superseded by dismissing them.
|
||||||
|
// This implements vcs.ReviewSuperseder for the GitHub adapter.
|
||||||
|
// The baseURL and sentinel parameters are unused for GitHub (dismissal is the mechanism).
|
||||||
|
func (c *Client) SupersedeReviews(ctx context.Context, owner, repo string, prNumber int, oldReviews []vcs.Review, newReviewID int64, _, _ string) error {
|
||||||
|
var errs []error
|
||||||
|
for _, old := range oldReviews {
|
||||||
|
if err := c.DismissReview(ctx, owner, repo, prNumber, old.ID, "Superseded by new review"); err != nil {
|
||||||
|
errs = append(errs, fmt.Errorf("dismiss review %d: %w", old.ID, err))
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return errors.Join(errs...)
|
||||||
|
}
|
||||||
|
|||||||
@@ -10,18 +10,6 @@ func FormatMarkdown(result *ReviewResult, reviewerName string) string {
|
|||||||
return FormatMarkdownWithDisplay(result, reviewerName, reviewerName)
|
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.
|
// FormatMarkdownWithDisplay formats a ReviewResult with separate display name and sentinel name.
|
||||||
// Note: displayName is not HTML-escaped as Gitea sanitizes rendered Markdown.
|
// Note: displayName is not HTML-escaped as Gitea sanitizes rendered Markdown.
|
||||||
// Persona display names are controlled by repo owners (trusted input).
|
// Persona display names are controlled by repo owners (trusted input).
|
||||||
|
|||||||
@@ -98,25 +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)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
func TestFormatMarkdown_Sentinel(t *testing.T) {
|
func TestFormatMarkdown_Sentinel(t *testing.T) {
|
||||||
|
sonnet-review-bot
commented
[NIT] Similar to the formatter.go nit — a blank line artifact remains after removing **[NIT]** Similar to the formatter.go nit — a blank line artifact remains after removing `TestGiteaEvent`. Minor formatting issue.
|
|||||||
result := &ReviewResult{
|
result := &ReviewResult{
|
||||||
Verdict: "APPROVE",
|
Verdict: "APPROVE",
|
||||||
|
|||||||
@@ -355,7 +355,7 @@ func TestCapitalizeFirst(t *testing.T) {
|
|||||||
{"HELLO", "HELLO"},
|
{"HELLO", "HELLO"},
|
||||||
{"a", "A"},
|
{"a", "A"},
|
||||||
{"", ""},
|
{"", ""},
|
||||||
{"日本語", "日本語"}, // Non-ASCII: Japanese doesn't have case
|
{"日本語", "日本語"}, // Non-ASCII: Japanese doesn't have case
|
||||||
{"über", "Über"}, // German umlaut
|
{"über", "Über"}, // German umlaut
|
||||||
{"élève", "Élève"}, // French accent
|
{"élève", "Élève"}, // French accent
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -41,3 +41,20 @@ type Client interface {
|
|||||||
Reviewer
|
Reviewer
|
||||||
Identity
|
Identity
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// ReviewerSelfRequester is an optional interface implemented by adapters that support
|
||||||
|
// requesting the authenticated user as a reviewer on a pull request. This is used for
|
||||||
|
// Gitea-specific behavior (ensuring the bot appears in required-reviewer checks).
|
||||||
|
// Consumers should use interface assertion: if sr, ok := client.(ReviewerSelfRequester); ok { ... }
|
||||||
|
type ReviewerSelfRequester interface {
|
||||||
|
RequestReviewerSelf(ctx context.Context, owner, repo string, number int, user string) error
|
||||||
|
}
|
||||||
|
|
||||||
|
// ReviewSuperseder is an optional interface implemented by adapters that support
|
||||||
|
// marking old reviews as superseded. For Gitea this means editing the review body
|
||||||
|
// with a link to the new review and resolving inline comments. For GitHub this
|
||||||
|
// means dismissing old reviews.
|
||||||
|
// Consumers should use interface assertion: if rs, ok := client.(ReviewSuperseder); ok { ... }
|
||||||
|
type ReviewSuperseder interface {
|
||||||
|
SupersedeReviews(ctx context.Context, owner, repo string, prNumber int, oldReviews []Review, newReviewID int64, baseURL, sentinel string) error
|
||||||
|
}
|
||||||
|
|||||||
@@ -0,0 +1,26 @@
|
|||||||
|
package vcs
|
||||||
|
|
||||||
|
// VCSProvider identifies a VCS platform. Using a typed string instead of bare
|
||||||
|
// strings makes provider values compiler-checkable and prevents typos from
|
||||||
|
// silently passing validation.
|
||||||
|
type VCSProvider string
|
||||||
|
|
||||||
|
const (
|
||||||
|
ProviderGitea VCSProvider = "gitea"
|
||||||
|
ProviderGitHub VCSProvider = "github"
|
||||||
|
sonnet-review-bot
commented
[NIT] **[NIT]** `ProviderGithub VCSProvider = "github"` — the constant name uses `Github` (lowercase 'h') while the package import path and the flag help text use `github`. Per the Go acronym convention documented in `patterns/style.md` (Acronyms Are All-Caps), this should be `ProviderGitHub`. This is a new exported symbol so fixing it now is low-cost; changing it later would break callers.
|
|||||||
|
)
|
||||||
|
|
||||||
|
// Valid reports whether p is a known VCS provider.
|
||||||
|
func (p VCSProvider) Valid() bool {
|
||||||
|
switch p {
|
||||||
|
case ProviderGitea, ProviderGitHub:
|
||||||
|
return true
|
||||||
|
default:
|
||||||
|
return false
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// String returns the string representation of the provider.
|
||||||
|
func (p VCSProvider) String() string {
|
||||||
|
return string(p)
|
||||||
|
}
|
||||||
[NIT] The
envOrDefaultBoolfunction is declared and used in the file but does not appear to be called frommain()in this diff — it may be leftover from a previous version. This is not a new issue introduced by this PR, just an observation.