Compare commits

..

20 Commits

Author SHA1 Message Date
claw 25cb55449e fix(nit): align CommitID field in vcs/types.go and document no-op in github/review.go
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 19s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 29s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 48s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m38s
2026-05-13 13:49:41 -07:00
claw 7e3b6ec8f1 fix(vcs): thread CommitID through abstraction layer (#114)
CI / test (pull_request) Successful in 20s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 39s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 46s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m24s
Add CommitID field to vcs.ReviewRequest so the commit anchor propagates
through the vcs.Client interface to platform adapters.

Changes:
- vcs/types.go: Add CommitID string field to ReviewRequest
- gitea/client.go: Add commitID parameter to PostReview, include in API payload
- gitea/adapter.go: Pass req.CommitID to underlying client
- github/review.go: Use req.CommitID as primary, fall back to comment-level
- cmd/review-bot/main.go: Set CommitID on ReviewRequest from pr.Head.SHA

Fixes #114
2026-05-13 13:30:48 -07:00
aweiker a32a5b694b Merge pull request 'feat(cmd): wire --provider and --base-url flags into CLI (Phase 5)' (#106) from review-bot-issue-82 into feature/github-support
Reviewed-on: #106
Reviewed-by: security-review-bot <10+security-review-bot@noreply.gitea.weiker.me>
Reviewed-by: Aaron Weiker <aaron@weiker.org>
2026-05-13 17:16:28 +00:00
claw 91fba770d9 fix(ci): restore *vcsURL default in --gitea-url alias registration
PR Ready Gate / clear-labels (pull_request) Successful in 1s
CI / test (pull_request) Successful in 20s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 45s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m49s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 2m3s
flag.StringVar sets *p = value at registration time. Using "" as the
default overwrites the env-resolved value that --vcs-url already stored
in *vcsURL. Restore *vcsURL as the default to preserve the GITEA_URL /
VCS_URL / GITHUB_SERVER_URL resolution chain.

Fixes CI error: --vcs-url (or --gitea-url) is required for provider=gitea
2026-05-13 09:33:06 -07:00
claw 5252143a33 fix: address review feedback — alias default, acronym convention, observability
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 19s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Failing after 6s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Failing after 10s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Failing after 10s
- #19639: Use empty default for --gitea-url alias to remove ordering dependency
- #19640: Upgrade slog.Warn to slog.Error for missing ReviewSuperseder (signals bug)
- #19641: Remove orphaned comment fragment from buildSupersededBody relocation
- #19642: Rename ProviderGithub → ProviderGitHub per Go acronym convention
- #19643: Log resolution failures at debug level in SupersedeReviews
2026-05-13 09:20:33 -07:00
claw ac6d34f5bd fix: address review feedback - eliminate type assertion via ReviewSuperseder interface
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 19s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 56s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m51s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 2m21s
- Introduce vcs.VCSProvider typed constant (replaces plain string provider)
- Introduce vcs.ReviewSuperseder optional interface for supersede logic
- Implement SupersedeReviews on gitea.Adapter (edit + resolve) and
  github.Client (dismiss)
- Remove concrete type assertion client.(*gitea.Adapter) from main
- Remove redundant baseURL fallback for github (NewClient defaults it)
- Condense --gitea-url alias comment block
- Fix fetchPatterns comment (empty paths are skipped, not fetched)
- Add default panic to VCS client init switch

Addresses: #19607, #19608, #19609, #19610, #19621, #19622, #19623
2026-05-13 09:03:42 -07:00
claw 34f7393892 fix: address review feedback on PR #106
PR Ready Gate / clear-labels (pull_request) Successful in 1s
CI / test (pull_request) Successful in 26s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 50s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m44s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 2m21s
- Remove unused envOrDefaultBool function and its test (Sonnet #3266 NIT)
- Replace Unicode em dashes with ASCII in slog messages (GPT #3267 NIT)
- Add scheme validation for vcsURL before embedding in Markdown link
  (Security #3269 MINOR — defense-in-depth against unsafe schemes)
- Extract ReviewerSelfRequester interface to remove concrete gitea.Adapter
  dependency from main's self-reviewer path (Sonnet #3266 NIT)
- Add compile-time conformance assertion and test for Adapter.RequestReviewerSelf
2026-05-13 08:48:09 -07:00
claw bdc109901d fix(github): remove double blank line in client_test.go (gofmt) 2026-05-13 08:48:09 -07:00
claw 271ea7f5fe style: remove stray blank line in doRequestWithBody 2026-05-13 08:48:09 -07:00
claw e70b54f238 fix: address review feedback — gofmt NITs and remove unreachable default
- github/client.go: add missing blank line between doRequestWithBody and doJSONRequest
- cmd/review-bot/main.go: remove double blank line before findAllOwnReviews
- cmd/review-bot/main.go: remove unreachable default case in VCS client init switch
  (provider is already validated at startup)
- cmd/review-bot/main_test.go: remove double blank line before TestHasSharedToken
- cmd/review-bot/main_test.go: fix comment alignment (gofmt)
- review/persona_test.go: fix comment alignment in table literal (gofmt)
2026-05-13 08:48:09 -07:00
claw c5bc807d2c fix(cmd): remove duplicate doc comment and double blank line 2026-05-13 08:48:09 -07:00
claw e8664714c4 docs(cmd,github): clarify type assertion and parameter usage in review superseding
Address sonnet-review feedback on PR #106:

- Document that the type assertion in supersedeOldReviews is guaranteed to
  succeed given the caller's provider switch, with the !ok branch guarding
  against future refactors (comment 18889).
- Clarify that vcsURL is only used in the Gitea path for constructing
  review permalink URLs (comment 18890).
- Add note explaining why the page-limit warning in ListReviews only fires
  when the final page is full, confirming the logic is intentional
  (comment 18891).
2026-05-13 08:48:09 -07:00
claw d40902771e fix: address self-review findings
- Remove dead code: findOwnReview (replaced by findAllOwnReviews)
- Check SetRetryBackoff return value in doJSONRequest tests
- Extract doWithRetry shared helper to eliminate ~100 lines of
  duplicated 429-retry/backoff/Retry-After logic between doRequest
  and doJSONRequest
- Fix import order: context before encoding/json (goimports)
- Add slog.Warn when ListReviews hits maxReviewPages limit
2026-05-13 08:48:09 -07:00
claw a30ee7df6e fix: address review feedback on PR #106
- Add 429 rate-limit retry logic to doJSONRequest (matching doRequest
  behavior) so write operations (PostReview, DismissReview) properly
  retry when rate-limited by GitHub
- Remove redundant explicit case for ReviewEventComment in
  translateReviewEvent (default already handles it)
- Add ordering comment on --gitea-url alias registration explaining
  the dependency on registration-before-parse evaluation order
- Add tests for doJSONRequest retry/exhaust behavior
2026-05-13 08:48:09 -07:00
claw a89dce1c52 fix(review): address bot review feedback on PR #106
- Document --gitea-url/--vcs-url last-one-wins behavior when both flags
  are passed simultaneously (sonnet MINOR #1)
- Move doJSONRequest from github/reviews.go to github/client.go where
  other HTTP helpers live (sonnet MINOR #2)
- Return joined error from supersedeOldReviews GitHub case instead of
  silently swallowing DismissReview failures (sonnet MINOR #3)
- Fix evaluateCIStatus to distinguish 'all checks passed' from 'no
  failures (N pending)' to avoid misleading status (gpt MINOR #2)
- Extract reviewsPerPage and maxReviewPages named constants for
  ListReviews pagination (gpt NIT #3)
2026-05-13 08:48:09 -07:00
claw 4c189d18a2 fix(review): address inline review feedback on PR #106
- Reword misleading 'Fall through' comment to 'Continue to' in
  supersedeOldReviews (comment #18704)
- Add shared-pointer explanation comment for --gitea-url alias
  registration (comment #18703)
- Add comment clarifying CommitID same-commit expectation in
  PostReview (comment #18705)
- Rename 'hidden alias' to 'backward-compatible alias' in flag
  comment (comment #18708)
2026-05-13 08:48:09 -07:00
claw 28e63a2338 fix(cmd): clarify empty gitea case control flow in supersedeOldReviews
The empty case "gitea": body exits the switch and continues to the
Gitea-specific logic below. Replace the vague comment with an explicit
note about the fall-through intent, per self-review feedback.
2026-05-13 08:48:09 -07:00
claw c4af35cd78 fix(cmd,github): address review feedback on PR #106
- Replace panic() with fmt.Fprintf+os.Exit(1) in provider switch default
  (repo convention: never panic)
- Remove spurious 'event' field from DismissReview payload (GitHub dismiss
  endpoint only documents 'message')
- Change translateReviewEvent default to return 'COMMENT' as canonical
  fallback instead of passing unknown events through to GitHub API
- Refactor supersedeOldReviews to use explicit switch/case with default
  error for exhaustiveness
2026-05-13 08:48:09 -07:00
claw 02920b685b fix: address review feedback on PR #106
- Replace interface{} with any in github/reviews.go (Go 1.18+ idiom)
- Add default panic case to VCS client init switch
- Refactor supersedeOldReviews to return error instead of os.Exit(1)
- Remove spurious blank lines in formatter.go and formatter_test.go
- Add doc comment to DeleteReview explaining when to use vs DismissReview
- Sanitize extractSentinelName output to prevent log injection
2026-05-13 08:48:09 -07:00
claw 4881a21ecb feat(cmd): wire --provider and --base-url flags into CLI
- Add --provider flag (gitea|github) for VCS backend selection
- Add --base-url flag for GitHub API endpoint configuration
- Rename --gitea-url to --vcs-url with backward-compatible alias
- Replace direct gitea.Client usage with vcs.Client interface
- Create vcs.Client via factory switch based on --provider value
- Implement Reviewer + Identity interfaces on github.Client
- Add verdictToEvent() using canonical vcs.ReviewEvent types
- Remove review.GiteaEvent() (replaced by verdictToEvent)
- GitHub supersede uses DismissReview; Gitea keeps EditComment flow
- Add VCS_PROVIDER, VCS_BASE_URL, VCS_URL env var support

Closes #82
2026-05-13 08:48:09 -07:00
20 changed files with 582 additions and 646 deletions
+1 -1
View File
@@ -9,7 +9,7 @@
| Package | Use Case | Scope | | Package | Use Case | Scope |
|---------|----------|-------| |---------|----------|-------|
| `github.com/goccy/go-yaml` | YAML parsing and AST inspection (subpkgs: `ast`, `parser`) | production | | `gopkg.in/yaml.v3` | YAML parsing (persona files, config) | production |
| `github.com/google/go-cmp` | Test comparisons (`cmp.Diff`) | test only | | `github.com/google/go-cmp` | Test comparisons (`cmp.Diff`) | test only |
**Any import not in this table or the Go standard library is forbidden.** **Any import not in this table or the Go standard library is forbidden.**
+28 -150
View File
@@ -2,7 +2,6 @@ package main
import ( import (
"context" "context"
"errors"
"flag" "flag"
"fmt" "fmt"
"log/slog" "log/slog"
@@ -71,7 +70,7 @@ func main() {
conventionsFile := flag.String("conventions-file", envOrDefault("CONVENTIONS_FILE", ""), "Conventions file path in repo (e.g. CLAUDE.md)") conventionsFile := flag.String("conventions-file", envOrDefault("CONVENTIONS_FILE", ""), "Conventions file path in repo (e.g. CLAUDE.md)")
systemPromptFile := flag.String("system-prompt-file", envOrDefault("SYSTEM_PROMPT_FILE", ""), "Local file with additional system prompt instructions") systemPromptFile := flag.String("system-prompt-file", envOrDefault("SYSTEM_PROMPT_FILE", ""), "Local file with additional system prompt instructions")
patternsRepo := flag.String("patterns-repo", envOrDefault("PATTERNS_REPO", ""), "Repo with language patterns (e.g. rodin/elixir-patterns)") patternsRepo := flag.String("patterns-repo", envOrDefault("PATTERNS_REPO", ""), "Repo with language patterns (e.g. rodin/elixir-patterns)")
patternsFiles := flag.String("patterns-files", envOrDefault("PATTERNS_FILES", ""), "Comma-separated file paths to fetch from patterns repo (empty = all files)") patternsFiles := flag.String("patterns-files", envOrDefault("PATTERNS_FILES", "README.md"), "Comma-separated file paths to fetch from patterns repo")
dryRun := flag.Bool("dry-run", false, "Print review to stdout instead of posting") dryRun := flag.Bool("dry-run", false, "Print review to stdout instead of posting")
llmTemp := flag.Float64("llm-temperature", envOrDefaultFloat("LLM_TEMPERATURE", 0), "LLM temperature (0 = server default)") llmTemp := flag.Float64("llm-temperature", envOrDefaultFloat("LLM_TEMPERATURE", 0), "LLM temperature (0 = server default)")
llmTimeout := flag.Int("llm-timeout", envOrDefaultInt("LLM_TIMEOUT", 300), "LLM request timeout in seconds (default 300)") llmTimeout := flag.Int("llm-timeout", envOrDefaultInt("LLM_TIMEOUT", 300), "LLM request timeout in seconds (default 300)")
@@ -85,16 +84,9 @@ 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)")
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)")
// Register --gitea-url as a backward-compatible alias for --vcs-url. // Backward-compatible alias: --gitea-url shares vcsURL's pointer (last flag wins).
// StringVar shares the *string pointer with vcsURL, so whichever flag is // Must use *vcsURL as default: StringVar sets *p=value at registration, so empty
// set last by flag.Parse wins — both point to the same underlying value. // string would overwrite the env-resolved value from the --vcs-url declaration.
// NOTE: If a user passes both --vcs-url and --gitea-url, the last one on
// the command line takes effect (standard flag package behavior). This is
// acceptable since --gitea-url is deprecated and both serve the same purpose.
//
// ORDERING: This must remain AFTER vcsURL's flag.String declaration and BEFORE
// flag.Parse(). The *vcsURL dereference captures the env-var-resolved default
// at registration time; moving flag.Parse() above this line would break it.
flag.StringVar(vcsURL, "gitea-url", *vcsURL, "Deprecated: use --vcs-url instead") flag.StringVar(vcsURL, "gitea-url", *vcsURL, "Deprecated: use --vcs-url instead")
flag.Parse() flag.Parse()
@@ -110,10 +102,8 @@ func main() {
slog.Info("review-bot starting", "version", version) slog.Info("review-bot starting", "version", version)
// Validate VCS provider // Validate VCS provider
switch *provider { vcsProvider := vcs.VCSProvider(*provider)
case "gitea", "github": if !vcsProvider.Valid() {
// valid
default:
fmt.Fprintf(os.Stderr, "Error: invalid --provider %q (valid: gitea, github)\n", *provider) fmt.Fprintf(os.Stderr, "Error: invalid --provider %q (valid: gitea, github)\n", *provider)
os.Exit(1) os.Exit(1)
} }
@@ -126,7 +116,7 @@ func main() {
os.Exit(1) os.Exit(1)
} }
// --vcs-url is required only for gitea provider // --vcs-url is required only for gitea provider
if *provider == "gitea" && *vcsURL == "" { if vcsProvider == vcs.ProviderGitea && *vcsURL == "" {
fmt.Fprintf(os.Stderr, "Error: --vcs-url (or --gitea-url) is required for provider=gitea\n") fmt.Fprintf(os.Stderr, "Error: --vcs-url (or --gitea-url) is required for provider=gitea\n")
os.Exit(1) os.Exit(1)
} }
@@ -169,18 +159,16 @@ func main() {
// Initialize VCS client // Initialize VCS client
var client vcs.Client var client vcs.Client
switch *provider { switch vcsProvider {
case "gitea": case vcs.ProviderGitea:
giteaClient := gitea.NewClient(*vcsURL, *reviewerToken) giteaClient := gitea.NewClient(*vcsURL, *reviewerToken)
client = gitea.NewAdapter(giteaClient) client = gitea.NewAdapter(giteaClient)
case "github": case vcs.ProviderGitHub:
ghBaseURL := *baseURL client = github.NewClient(*reviewerToken, *baseURL)
if ghBaseURL == "" { default:
ghBaseURL = "https://api.github.com" panic("unreachable: provider validation should have caught " + vcsProvider.String())
} }
client = github.NewClient(*reviewerToken, ghBaseURL) slog.Info("VCS client initialized", "provider", vcsProvider)
}
slog.Info("VCS client initialized", "provider", *provider)
// Initialize LLM client // Initialize LLM client
llmClient := llm.NewClient(*llmBaseURL, *llmAPIKey, *llmModel) llmClient := llm.NewClient(*llmBaseURL, *llmAPIKey, *llmModel)
@@ -497,6 +485,7 @@ func main() {
reviewReq := vcs.ReviewRequest{ reviewReq := vcs.ReviewRequest{
Body: reviewBody, Body: reviewBody,
Event: event, Event: event,
CommitID: pr.Head.SHA,
Comments: inlineComments, Comments: inlineComments,
} }
posted, err := client.PostReview(ctx, owner, repoName, prNumber, reviewReq) posted, err := client.PostReview(ctx, owner, repoName, prNumber, reviewReq)
@@ -506,12 +495,16 @@ func main() {
} }
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)
// Supersede all old reviews // Supersede all old reviews via optional interface
if len(oldReviews) > 0 { if len(oldReviews) > 0 {
if err := supersedeOldReviews(ctx, client, *provider, *vcsURL, owner, repoName, prNumber, oldReviews, posted.ID, sentinel); err != nil { if superseder, ok := client.(vcs.ReviewSuperseder); ok {
if err := superseder.SupersedeReviews(ctx, owner, repoName, prNumber, oldReviews, posted.ID, *vcsURL, sentinel); err != nil {
slog.Error("failed to supersede old reviews", "error", err) slog.Error("failed to supersede old reviews", "error", err)
os.Exit(1) os.Exit(1)
} }
} else {
slog.Error("provider does not support review superseding", "provider", vcsProvider)
}
} }
} }
@@ -527,88 +520,6 @@ func verdictToEvent(verdict string) vcs.ReviewEvent {
} }
} }
// supersedeOldReviews marks prior reviews as superseded so only the latest review is visible.
// For GitHub: dismisses old reviews (vcsURL is unused in this path).
// For Gitea: edits the review body with a link to the new review and resolves inline comments.
//
// The vcsURL parameter is only used in the Gitea path to construct review permalink URLs;
// it is accepted unconditionally to keep the function signature uniform across providers.
func supersedeOldReviews(ctx context.Context, client vcs.Client, provider, vcsURL, owner, repoName string, prNumber int, oldReviews []vcs.Review, newReviewID int64, sentinel string) error {
switch provider {
case "github":
// Best-effort dismissal: attempt all reviews, join any errors.
var errs []error
for _, old := range oldReviews {
if err := client.DismissReview(ctx, owner, repoName, prNumber, old.ID, "Superseded by new review"); err != nil {
slog.Warn("failed to dismiss review", "id", old.ID, "error", err)
errs = append(errs, fmt.Errorf("dismiss review %d: %w", old.ID, err))
} else {
slog.Info("dismissed old review", "review_id", old.ID, "new_review_id", newReviewID, "pr", prNumber)
}
}
return errors.Join(errs...)
case "gitea":
// Continue to Gitea-specific logic below the switch.
default:
return fmt.Errorf("supersedeOldReviews: unsupported provider %q", provider)
}
// The type assertion below is guaranteed to succeed: the caller's provider switch
// ensures we only reach this point when provider == "gitea", and the gitea provider
// always constructs a *gitea.Adapter. The !ok branch guards against future refactors
// (e.g. wrapping the adapter in a decorator) that would silently break this path.
giteaAdapter, ok := client.(*gitea.Adapter)
if !ok {
return fmt.Errorf("expected gitea.Adapter for gitea provider, got %T", client)
}
underlying := giteaAdapter.Underlying()
// Validate vcsURL scheme before embedding in Markdown link (defense-in-depth).
if !strings.HasPrefix(vcsURL, "http://") && !strings.HasPrefix(vcsURL, "https://") {
return fmt.Errorf("supersedeOldReviews: vcsURL must have http or https scheme, got %q", vcsURL)
}
newReviewURL := fmt.Sprintf("%s/%s/%s/pulls/%d#pullrequestreview-%d", strings.TrimRight(vcsURL, "/"), owner, repoName, prNumber, newReviewID)
for _, oldReview := range oldReviews {
cid, err := underlying.GetTimelineReviewCommentIDForReview(ctx, owner, repoName, prNumber, oldReview.ID)
if err != nil {
slog.Warn("could not find comment ID for old review", "review_id", oldReview.ID, "error", err)
continue
}
supersededBody := buildSupersededBody(oldReview.Body, oldReview.CommitID, newReviewURL, sentinel)
if err := underlying.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", newReviewID, "pr", prNumber)
// Resolve old review's inline comments
oldComments, err := underlying.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 := underlying.ResolveComment(ctx, owner, repoName, c.ID); err != nil {
slog.Debug("could not resolve inline comment", "comment_id", c.ID, "error", err)
failed++
} else {
resolved++
}
}
if resolved > 0 {
slog.Info("resolved old inline comments", "review_id", oldReview.ID, "count", resolved, "pr", prNumber)
}
if failed > 0 {
slog.Warn("some inline comments could not be resolved", "review_id", oldReview.ID, "failed", failed, "pr", prNumber)
}
}
return nil
}
// 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 vcs.PRReader, owner, repo, ref string, files []vcs.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
@@ -636,25 +547,12 @@ func fetchFileContext(ctx context.Context, client vcs.PRReader, owner, repo, ref
// 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.
// If patternsFiles is empty, all files from the repo root are fetched. // Empty entries in patternsFiles are skipped (no implicit repo-root fetch).
func fetchPatterns(ctx context.Context, client vcs.FileReader, patternsRepo, patternsFiles string) string { 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, ",")
paths := strings.Split(patternsFiles, ",")
// Build the list of paths to fetch
var paths []string
if patternsFiles == "" {
// Empty patternsFiles means "fetch all files from repo root"
paths = []string{""}
} else {
for _, p := range strings.Split(patternsFiles, ",") {
p = strings.TrimSpace(p)
if p != "" {
paths = append(paths, p)
}
}
}
for _, repoRef := range repos { for _, repoRef := range repos {
if ctx.Err() != nil { if ctx.Err() != nil {
@@ -675,6 +573,11 @@ func fetchPatterns(ctx context.Context, client vcs.FileReader, patternsRepo, pat
var repoSkippedFiles []string var repoSkippedFiles []string
for _, path := range paths { for _, path := range paths {
path = strings.TrimSpace(path)
if path == "" {
continue
}
files, err := vcs.GetAllFilesInPath(ctx, client, 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)
@@ -820,31 +723,6 @@ 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
// VCS 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 accounts. Returns true if shared token // instead of having separate accounts. Returns true if shared token
-94
View File
@@ -162,54 +162,6 @@ func makeReview(id int64, login, state string, stale bool, body string) vcs.Revi
} }
} }
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 TestHasSharedToken(t *testing.T) { func TestHasSharedToken(t *testing.T) {
tests := []struct { tests := []struct {
name string name string
@@ -415,52 +367,6 @@ func TestIsPatternFile(t *testing.T) {
} }
} }
// TestBuildPatternPaths verifies the path-building logic for fetchPatterns.
// Empty patternsFiles means "fetch all from root" (represented as [""]).
func TestBuildPatternPaths(t *testing.T) {
buildPaths := func(patternsFiles string) []string {
if patternsFiles == "" {
return []string{""}
}
var paths []string
for _, p := range strings.Split(patternsFiles, ",") {
p = strings.TrimSpace(p)
if p != "" {
paths = append(paths, p)
}
}
return paths
}
tests := []struct {
name string
input string
want []string
}{
{"empty fetches root", "", []string{""}},
{"single file", "README.md", []string{"README.md"}},
{"multiple files", "README.md,PATTERNS.md", []string{"README.md", "PATTERNS.md"}},
{"trims whitespace", " foo.md , bar.md ", []string{"foo.md", "bar.md"}},
{"skips empty between commas", "foo.md,,bar.md", []string{"foo.md", "bar.md"}},
{"directory path", "patterns/", []string{"patterns/"}},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
got := buildPaths(tc.input)
if len(got) != len(tc.want) {
t.Errorf("buildPaths(%q) = %v, want %v", tc.input, got, tc.want)
return
}
for i := range got {
if got[i] != tc.want[i] {
t.Errorf("buildPaths(%q)[%d] = %q, want %q", tc.input, i, got[i], tc.want[i])
}
}
})
}
}
func TestEvaluateCIStatus(t *testing.T) { func TestEvaluateCIStatus(t *testing.T) {
tests := []struct { tests := []struct {
name string name string
+31 -10
View File
@@ -9,7 +9,7 @@ JSON is awkward for persona files that contain multi-line text (identity, severi
- Backwards compatibility: existing JSON personas must continue to work - Backwards compatibility: existing JSON personas must continue to work
- Security: protect against DoS via deeply nested YAML (AIKIDO-2024-10486) - Security: protect against DoS via deeply nested YAML (AIKIDO-2024-10486)
- Consistency: use `.yaml` extension (not `.yml`) - Consistency: use `.yaml` extension (not `.yml`)
- Library: use `github.com/goccy/go-yaml` v1.16.0+ (approved in CONVENTIONS.md); we implement custom AST-based depth/node-count checks for precise alias-aware validation - Library: use `gopkg.in/yaml.v3` (approved in CONVENTIONS.md) with explicit depth limiting
## Proposed Approach ## Proposed Approach
@@ -33,16 +33,37 @@ func parsePersona(data []byte, source string) (*Persona, error) {
### YAML Parsing with Depth Protection ### YAML Parsing with Depth Protection
We implement a custom AST-based depth/node-count walk (`checkYAMLDepth` in ```go
`review/persona.go`) rather than relying on library decoder options. Key design func unmarshalYAMLWithDepthLimit(data []byte, out any, maxDepth int) error {
decisions: var node yaml.Node
dec := yaml.NewDecoder(bytes.NewReader(data))
if err := dec.Decode(&node); err != nil {
return err
}
if err := checkYAMLDepth(&node, 0, maxDepth); err != nil {
return err
}
return node.Decode(out)
}
- **Library:** `github.com/goccy/go-yaml` with `ast.Node`-based traversal func checkYAMLDepth(node *yaml.Node, depth, maxDepth int) error {
- **Dual-map tracking:** `validated` (depth-aware short-circuit) + `visiting` (cycle detection) if depth > maxDepth {
- **Node-count limit:** Conservative overcounting bounds total validation work return fmt.Errorf("YAML nesting depth exceeds maximum (%d)", maxDepth)
- **Alias-aware depth:** Aliases increment depth and are re-checked when encountered at greater depths }
// Handle alias nodes by following the Alias pointer
if node.Kind == yaml.AliasNode && node.Alias != nil {
return checkYAMLDepth(node.Alias, depth, maxDepth)
}
for _, child := range node.Content {
if err := checkYAMLDepth(child, depth+1, maxDepth); err != nil {
return err
}
}
return nil
}
```
See `review/persona.go:checkYAMLDepth` for the authoritative implementation. The `gopkg.in/yaml.v3` library does not have built-in depth protection, so we implement explicit depth checking by first decoding into a `yaml.Node`, walking the tree to verify depth (including alias resolution), then decoding into the target struct.
## State/Data Model ## State/Data Model
@@ -53,7 +74,7 @@ No new state. Same `Persona` struct, just different parsing.
| Error | Handling | | Error | Handling |
|-------|----------| |-------|----------|
| Invalid YAML syntax | Return parse error with source file | | Invalid YAML syntax | Return parse error with source file |
| Deeply nested YAML | Custom AST walk (`checkYAMLDepth`) rejects before decode | | Deeply nested YAML | Library rejects (v1.16.0+ fix) |
| Unknown extension | Fall back to JSON parsing | | Unknown extension | Fall back to JSON parsing |
| Missing required fields | Validation rejects after parse | | Missing required fields | Validation rejects after parse |
+81 -4
View File
@@ -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"
) )
@@ -168,9 +170,9 @@ func (a *Adapter) PostReview(ctx context.Context, owner, repo string, number int
if err != nil { if err != nil {
return nil, fmt.Errorf("translate position %d in %s: %w", c.Position, c.Path, err) return nil, fmt.Errorf("translate position %d in %s: %w", c.Position, c.Path, err)
} }
// CommitID from vcs.ReviewComment is intentionally not forwarded: // Per-comment CommitID is not forwarded to Gitea inline comments:
// Gitea review comments are pinned to the PR head SHA automatically, // Gitea's CreatePullReview API has no per-comment commit_id field.
// and the CreatePullReview API has no per-comment commit_id field. // The review-level commit anchor is set via req.CommitID instead.
giteaComments = append(giteaComments, ReviewComment{ giteaComments = append(giteaComments, ReviewComment{
Path: c.Path, Path: c.Path,
NewPosition: int64(lineNum), NewPosition: int64(lineNum),
@@ -179,7 +181,7 @@ func (a *Adapter) PostReview(ctx context.Context, owner, repo string, number int
} }
} }
review, err := a.client.PostReview(ctx, owner, repo, number, event, req.Body, giteaComments) review, err := a.client.PostReview(ctx, owner, repo, number, event, req.Body, req.CommitID, giteaComments)
if err != nil { if err != nil {
return nil, fmt.Errorf("post review: %w", err) return nil, fmt.Errorf("post review: %w", err)
} }
@@ -237,3 +239,78 @@ func (a *Adapter) GetAuthenticatedUser(ctx context.Context) (string, error) {
func (a *Adapter) RequestReviewerSelf(ctx context.Context, owner, repo string, number int, user string) error { func (a *Adapter) RequestReviewerSelf(ctx context.Context, owner, repo string, number int, user string) error {
return a.client.RequestReviewer(ctx, owner, repo, number, user) 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)
}
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()
}
+70
View File
@@ -408,3 +408,73 @@ func TestAdapter_RequestReviewerSelf(t *testing.T) {
t.Fatalf("RequestReviewerSelf() error = %v", err) t.Fatalf("RequestReviewerSelf() error = %v", err)
} }
} }
func TestAdapter_PostReview_CommitID_Threading(t *testing.T) {
var gotPayload struct {
Body string `json:"body"`
Event string `json:"event"`
CommitID string `json:"commit_id"`
}
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/json")
json.NewDecoder(r.Body).Decode(&gotPayload)
json.NewEncoder(w).Encode(map[string]any{
"id": 1,
"body": "test",
"user": map[string]any{"login": "bot"},
"commit_id": "abc123def456",
})
}))
defer server.Close()
client := gitea.NewClient(server.URL, "token")
adapter := gitea.NewAdapter(client)
review, err := adapter.PostReview(context.Background(), "owner", "repo", 1, vcs.ReviewRequest{
Body: "LGTM",
Event: vcs.ReviewEventApprove,
CommitID: "abc123def456",
// No comments → no diff fetch needed
})
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if gotPayload.CommitID != "abc123def456" {
t.Errorf("commit_id = %q, want %q", gotPayload.CommitID, "abc123def456")
}
if review.CommitID != "abc123def456" {
t.Errorf("review.CommitID = %q, want %q", review.CommitID, "abc123def456")
}
}
func TestAdapter_PostReview_EmptyCommitID_Omitted(t *testing.T) {
var gotRawPayload map[string]any
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/json")
json.NewDecoder(r.Body).Decode(&gotRawPayload)
json.NewEncoder(w).Encode(map[string]any{
"id": 1,
"body": "test",
"user": map[string]any{"login": "bot"},
})
}))
defer server.Close()
client := gitea.NewClient(server.URL, "token")
adapter := gitea.NewAdapter(client)
_, err := adapter.PostReview(context.Background(), "owner", "repo", 1, vcs.ReviewRequest{
Body: "looks good",
Event: vcs.ReviewEventComment,
// CommitID intentionally empty
})
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
// With empty CommitID and omitempty tag, the field should not appear in JSON
if _, exists := gotRawPayload["commit_id"]; exists {
t.Errorf("commit_id should be omitted when empty, but was present: %v", gotRawPayload["commit_id"])
}
}
+6 -2
View File
@@ -186,18 +186,22 @@ func (c *Client) GetFileContentRef(ctx context.Context, owner, repo, filepath, r
} }
// PostReview submits a review to a PR and returns the created review. // PostReview submits a review to a PR and returns the created review.
// event should be "APPROVED" or "REQUEST_CHANGES". // event should be one of "APPROVED", "REQUEST_CHANGES", or "COMMENT".
// commitID anchors the review to a specific commit SHA. If empty, Gitea
// defaults to the current PR head.
// comments are optional inline comments attached to specific lines. // comments are optional inline comments attached to specific lines.
func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, event, body string, comments []ReviewComment) (*Review, error) { func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, event, body, commitID string, comments []ReviewComment) (*Review, error) {
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number) reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
payload := struct { payload := struct {
Body string `json:"body"` Body string `json:"body"`
Event string `json:"event"` Event string `json:"event"`
CommitID string `json:"commit_id,omitempty"`
Comments []ReviewComment `json:"comments,omitempty"` Comments []ReviewComment `json:"comments,omitempty"`
}{ }{
Body: body, Body: body,
Event: event, Event: event,
CommitID: commitID,
Comments: comments, Comments: comments,
} }
+61 -2
View File
@@ -135,7 +135,7 @@ func TestPostReview(t *testing.T) {
defer server.Close() defer server.Close()
client := NewClient(server.URL, "test-token") client := NewClient(server.URL, "test-token")
review, err := client.PostReview(context.Background(), "owner", "repo", 3, "APPROVED", "LGTM", nil) review, err := client.PostReview(context.Background(), "owner", "repo", 3, "APPROVED", "LGTM", "", nil)
if err != nil { if err != nil {
t.Fatalf("unexpected error: %v", err) t.Fatalf("unexpected error: %v", err)
} }
@@ -182,7 +182,7 @@ func TestPostReview_Non200(t *testing.T) {
defer server.Close() defer server.Close()
client := NewClient(server.URL, "test-token") client := NewClient(server.URL, "test-token")
_, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "test", nil) _, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "test", "", nil)
if err == nil { if err == nil {
t.Fatal("expected error for 403, got nil") t.Fatal("expected error for 403, got nil")
} }
@@ -1144,3 +1144,62 @@ func TestSanitizeErrorForLog(t *testing.T) {
}) })
} }
} }
func TestPostReview_CommitID_InPayload(t *testing.T) {
var gotPayload struct {
Body string `json:"body"`
Event string `json:"event"`
CommitID string `json:"commit_id"`
}
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
json.NewDecoder(r.Body).Decode(&gotPayload)
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(200)
json.NewEncoder(w).Encode(map[string]any{
"id": 200,
"body": "LGTM",
"user": map[string]any{"login": "bot"},
"state": "APPROVED",
"commit_id": "deadbeef1234",
})
}))
defer server.Close()
client := NewClient(server.URL, "test-token")
review, err := client.PostReview(context.Background(), "owner", "repo", 5, "APPROVED", "LGTM", "deadbeef1234", nil)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if gotPayload.CommitID != "deadbeef1234" {
t.Errorf("sent commit_id = %q, want %q", gotPayload.CommitID, "deadbeef1234")
}
if review.CommitID != "deadbeef1234" {
t.Errorf("response commit_id = %q, want %q", review.CommitID, "deadbeef1234")
}
}
func TestPostReview_EmptyCommitID_OmittedFromPayload(t *testing.T) {
var gotRaw map[string]any
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
json.NewDecoder(r.Body).Decode(&gotRaw)
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(200)
json.NewEncoder(w).Encode(map[string]any{
"id": 201,
"body": "ok",
"user": map[string]any{"login": "bot"},
})
}))
defer server.Close()
client := NewClient(server.URL, "test-token")
_, err := client.PostReview(context.Background(), "owner", "repo", 5, "COMMENT", "ok", "", nil)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if _, exists := gotRaw["commit_id"]; exists {
t.Errorf("commit_id should be omitted when empty, but was present: %v", gotRaw["commit_id"])
}
}
+2 -2
View File
@@ -37,7 +37,7 @@ func TestPostReview_WithComments(t *testing.T) {
{Path: "util.go", NewPosition: 10, Body: "[MINOR] Style issue"}, {Path: "util.go", NewPosition: 10, Body: "[MINOR] Style issue"},
} }
_, err := client.PostReview(context.Background(), "owner", "repo", 1, "REQUEST_CHANGES", "summary", comments) _, err := client.PostReview(context.Background(), "owner", "repo", 1, "REQUEST_CHANGES", "summary", "", comments)
if err != nil { if err != nil {
t.Fatalf("unexpected error: %v", err) t.Fatalf("unexpected error: %v", err)
} }
@@ -72,7 +72,7 @@ func TestPostReview_NilComments(t *testing.T) {
defer server.Close() defer server.Close()
client := NewClient(server.URL, "test-token") client := NewClient(server.URL, "test-token")
_, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "all good", nil) _, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "all good", "", nil)
if err != nil { if err != nil {
t.Fatalf("unexpected error: %v", err) t.Fatalf("unexpected error: %v", err)
} }
+54
View File
@@ -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")
}
}
+3
View File
@@ -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)
+23 -5
View File
@@ -82,9 +82,11 @@ func translateGitHubReviewState(state string) string {
// (via the omitempty tag on postReviewRequest.Comments). // (via the omitempty tag on postReviewRequest.Comments).
// //
// The GitHub API accepts a single commit_id per review submission. PostReview // The GitHub API accepts a single commit_id per review submission. PostReview
// extracts it from the first comment with a non-empty CommitID. If any subsequent // uses req.CommitID as the primary commit anchor. If req.CommitID is empty,
// comment specifies a different CommitID, PostReview returns ErrConflictingCommitIDs. // it falls back to extracting from the first comment with a non-empty CommitID.
// Comments with an empty CommitID are allowed and inherit the review-level value. // If any subsequent comment specifies a different CommitID, PostReview returns
// ErrConflictingCommitIDs. Comments with an empty CommitID are allowed and
// inherit the review-level value.
func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, req vcs.ReviewRequest) (*vcs.Review, error) { func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, req vcs.ReviewRequest) (*vcs.Review, error) {
reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews", reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews",
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number) c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
@@ -92,11 +94,13 @@ func (c *Client) PostReview(ctx context.Context, owner, repo string, number int,
payload := postReviewRequest{ payload := postReviewRequest{
Body: req.Body, Body: req.Body,
Event: string(req.Event), Event: string(req.Event),
CommitID: req.CommitID,
} }
// Build the payload in one pass. The GitHub API accepts a single commit_id // Build the payload in one pass. The GitHub API accepts a single commit_id
// per review; we extract it from the first comment that supplies one and // per review. req.CommitID is the primary source; if empty, we extract from
// reject the request if any other comment disagrees. // the first comment that supplies one. Reject if any comment disagrees with
// the resolved commit_id.
for _, comment := range req.Comments { for _, comment := range req.Comments {
if comment.CommitID != "" { if comment.CommitID != "" {
if payload.CommitID == "" { if payload.CommitID == "" {
@@ -104,6 +108,7 @@ func (c *Client) PostReview(ctx context.Context, owner, repo string, number int,
} else if payload.CommitID != comment.CommitID { } else if payload.CommitID != comment.CommitID {
return nil, ErrConflictingCommitIDs return nil, ErrConflictingCommitIDs
} }
// else: matching SHA is a no-op by design
} }
payload.Comments = append(payload.Comments, reviewCommentEntry{ payload.Comments = append(payload.Comments, reviewCommentEntry{
Path: comment.Path, Path: comment.Path,
@@ -210,3 +215,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...)
}
+93
View File
@@ -389,3 +389,96 @@ func TestPostReview_ConflictingCommitIDs(t *testing.T) {
t.Errorf("expected ErrConflictingCommitIDs, got: %v", err) t.Errorf("expected ErrConflictingCommitIDs, got: %v", err)
} }
} }
func TestPostReview_RequestCommitID_TakesPriority(t *testing.T) {
var gotPayload struct {
CommitID string `json:"commit_id"`
Body string `json:"body"`
}
c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
json.NewDecoder(r.Body).Decode(&gotPayload)
w.Header().Set("Content-Type", "application/json")
json.NewEncoder(w).Encode(map[string]any{
"id": 42,
"body": "LGTM",
"state": "APPROVED",
"commit_id": "req-level-sha",
"user": map[string]any{"login": "bot"},
})
})
review, err := c.PostReview(context.Background(), "owner", "repo", 1, vcs.ReviewRequest{
Body: "LGTM",
Event: vcs.ReviewEventApprove,
CommitID: "req-level-sha",
Comments: []vcs.ReviewComment{
{Path: "a.go", Position: 1, CommitID: "req-level-sha", Body: "looks good"},
},
})
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if gotPayload.CommitID != "req-level-sha" {
t.Errorf("sent commit_id = %q, want %q", gotPayload.CommitID, "req-level-sha")
}
if review.CommitID != "req-level-sha" {
t.Errorf("review.CommitID = %q, want %q", review.CommitID, "req-level-sha")
}
}
func TestPostReview_RequestCommitID_ConflictsWithComment(t *testing.T) {
c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
t.Fatal("request should not be sent when commit IDs conflict")
})
// req.CommitID is set, and a comment has a different CommitID → conflict
_, err := c.PostReview(context.Background(), "owner", "repo", 1, vcs.ReviewRequest{
Body: "Review",
Event: vcs.ReviewEventComment,
CommitID: "req-sha",
Comments: []vcs.ReviewComment{
{Path: "a.go", Position: 1, CommitID: "different-sha", Body: "nit"},
},
})
if err == nil {
t.Fatal("expected error for conflicting commit IDs")
}
if !errors.Is(err, ErrConflictingCommitIDs) {
t.Errorf("expected ErrConflictingCommitIDs, got: %v", err)
}
}
func TestPostReview_RequestCommitID_FallbackToComment(t *testing.T) {
var gotPayload struct {
CommitID string `json:"commit_id"`
}
c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
json.NewDecoder(r.Body).Decode(&gotPayload)
w.Header().Set("Content-Type", "application/json")
json.NewEncoder(w).Encode(map[string]any{
"id": 43,
"body": "ok",
"state": "COMMENTED",
"commit_id": "comment-sha",
"user": map[string]any{"login": "bot"},
})
})
// req.CommitID is empty, so it falls back to the comment's CommitID
_, err := c.PostReview(context.Background(), "owner", "repo", 1, vcs.ReviewRequest{
Body: "ok",
Event: vcs.ReviewEventComment,
// CommitID intentionally empty
Comments: []vcs.ReviewComment{
{Path: "a.go", Position: 1, CommitID: "comment-sha", Body: "note"},
},
})
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if gotPayload.CommitID != "comment-sha" {
t.Errorf("sent commit_id = %q, want %q (fallback from comment)", gotPayload.CommitID, "comment-sha")
}
}
+1 -1
View File
@@ -2,4 +2,4 @@ module gitea.weiker.me/rodin/review-bot
go 1.26.2 go 1.26.2
require github.com/goccy/go-yaml v1.19.2 require gopkg.in/yaml.v3 v3.0.1
+4 -2
View File
@@ -1,2 +1,4 @@
github.com/goccy/go-yaml v1.19.2 h1:PmFC1S6h8ljIz6gMRBopkjP1TVT7xuwrButHID66PoM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
github.com/goccy/go-yaml v1.19.2/go.mod h1:XBurs7gK8ATbW4ZPGKgcbrY1Br56PdM69F7LkFRi1kA= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=
gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
+37 -145
View File
@@ -5,15 +5,12 @@ import (
"embed" "embed"
"encoding/json" "encoding/json"
"fmt" "fmt"
"io"
"os" "os"
"sort" "sort"
"strings" "strings"
"unicode/utf8" "unicode/utf8"
"github.com/goccy/go-yaml" "gopkg.in/yaml.v3"
"github.com/goccy/go-yaml/ast"
"github.com/goccy/go-yaml/parser"
) )
//go:embed personas/*.yaml //go:embed personas/*.yaml
@@ -121,8 +118,10 @@ func ListBuiltinPersonas() []string {
default: default:
continue continue
} }
if !seen[personaName] {
seen[personaName] = true seen[personaName] = true
} }
}
names := make([]string, 0, len(seen)) names := make([]string, 0, len(seen))
for name := range seen { for name := range seen {
names = append(names, name) names = append(names, name)
@@ -143,19 +142,10 @@ func parsePersona(data []byte, source string) (*Persona, error) {
err = unmarshalYAMLWithDepthLimit(data, &p, MaxYAMLDepth) err = unmarshalYAMLWithDepthLimit(data, &p, MaxYAMLDepth)
} else { } else {
// Use json.Decoder with DisallowUnknownFields for consistency with // Use json.Decoder with DisallowUnknownFields for consistency with
// YAML's Strict() - both reject unknown fields to catch typos. // YAML's KnownFields(true) - both reject unknown fields to catch typos.
dec := json.NewDecoder(bytes.NewReader(data)) dec := json.NewDecoder(bytes.NewReader(data))
dec.DisallowUnknownFields() dec.DisallowUnknownFields()
err = dec.Decode(&p) err = dec.Decode(&p)
if err == nil {
// Reject trailing content after the first valid JSON object.
// Without this check, input like `{"name":"x"}garbage` would
// silently succeed because Decoder stops after one object.
var dummy json.RawMessage
if err2 := dec.Decode(&dummy); err2 != io.EOF {
err = fmt.Errorf("unexpected trailing content after JSON object")
}
}
} }
if err != nil { if err != nil {
return nil, fmt.Errorf("parse persona %s: %w", source, err) return nil, fmt.Errorf("parse persona %s: %w", source, err)
@@ -166,176 +156,78 @@ func parsePersona(data []byte, source string) (*Persona, error) {
return &p, nil return &p, nil
} }
// unmarshalYAMLWithDepthLimit unmarshals YAML data with three safety checks: // unmarshalYAMLWithDepthLimit unmarshals YAML data with explicit depth limiting
// - Depth limiting: rejects AST trees exceeding maxDepth to prevent stack exhaustion. // and strict field checking. This protects against stack exhaustion from deeply
// - Multi-document rejection: prevents silent data loss from ignored extra documents. // nested structures and catches typos in field names.
// - Strict field checking: rejects unknown YAML keys to catch typos early. // Multi-document YAML files are rejected to prevent silent data loss.
func unmarshalYAMLWithDepthLimit(data []byte, out any, maxDepth int) error { func unmarshalYAMLWithDepthLimit(data []byte, out any, maxDepth int) error {
// First pass: parse into AST to check depth limits, node counts, and // First pass: decode into a yaml.Node to check depth limits and node counts.
// multi-document rejection. This prevents stack exhaustion before we // This prevents stack exhaustion before we attempt to decode into structs.
// attempt to decode into structs. var node yaml.Node
file, err := parser.ParseBytes(data, 0) dec := yaml.NewDecoder(bytes.NewReader(data))
if err != nil { if err := dec.Decode(&node); err != nil {
return err return err
} }
// Reject empty YAML input (whitespace-only, comment-only, or truly empty files).
// The parser returns a single doc with nil body for these cases.
if len(file.Docs) == 0 || file.Docs[0].Body == nil {
return fmt.Errorf("empty YAML document")
}
// Reject multi-document YAML files - silently ignoring additional documents // Reject multi-document YAML files - silently ignoring additional documents
// could lead to confusing behavior where users think their changes take effect. // could lead to confusing behavior where users think their changes take effect.
if len(file.Docs) > 1 { var extra yaml.Node
if dec.Decode(&extra) == nil {
return fmt.Errorf("multi-document YAML is not supported; only single-document files are allowed") return fmt.Errorf("multi-document YAML is not supported; only single-document files are allowed")
} }
nodeCount := 0 nodeCount := 0
if err := checkYAMLDepth(file.Docs[0].Body, 0, maxDepth, MaxYAMLNodes, make(map[ast.Node]int), make(map[ast.Node]bool), &nodeCount); err != nil { if err := checkYAMLDepth(&node, 0, maxDepth, MaxYAMLNodes, make(map[*yaml.Node]struct{}), &nodeCount); err != nil {
return err return err
} }
// Second pass: decode with strict field checking enabled. // Second pass: decode with strict field checking enabled.
// Strict() rejects unknown keys, catching typos like "focuss" or "identiy". // KnownFields(true) rejects unknown keys, catching typos like "focuss" or "identiy".
// // We must re-decode from the original data because yaml.Node.Decode() doesn't
// Safety note: goccy/go-yaml's decoder does not expand YAML aliases // support the KnownFields option.
// recursively — it resolves them via the pre-built AST, which our first strictDec := yaml.NewDecoder(bytes.NewReader(data))
// pass already depth-checked. Alias chains that would exceed depth limits strictDec.KnownFields(true)
// are caught above; the decoder merely reads the resolved scalar values. return strictDec.Decode(out)
dec := yaml.NewDecoder(bytes.NewReader(data), yaml.Strict())
return dec.Decode(out)
}
// checkYAMLDepth recursively checks that YAML AST nodes don't exceed the depth
// limit or the total node count limit. It uses two tracking maps:
// - validated: maps each node to the maximum depth at which it was previously
// checked. If a node is revisited at a deeper depth (e.g., via an alias),
// we re-check it to ensure the combined effective depth doesn't exceed limits.
// - visiting: per-path recursion stack for true cycle detection. A node on the
// current path is a cycle (alias loop); we return nil to avoid infinite recursion.
//
// This design prevents the alias depth bypass where an anchored subtree validated
// at a shallow depth could be referenced via alias at a greater depth, effectively
// exceeding MaxYAMLDepth.
func checkYAMLDepth(node ast.Node, depth, maxDepth, maxNodes int, validated map[ast.Node]int, visiting map[ast.Node]bool, nodeCount *int) error {
if node == nil {
return nil
} }
// checkYAMLDepth recursively checks that YAML nodes don't exceed the depth limit
// or the total node count limit. It also detects alias cycles to prevent infinite
// recursion from crafted YAML with self-referential aliases.
func checkYAMLDepth(node *yaml.Node, depth, maxDepth, maxNodes int, seen map[*yaml.Node]struct{}, nodeCount *int) error {
if depth > maxDepth { if depth > maxDepth {
return fmt.Errorf("YAML nesting depth exceeds maximum (%d)", maxDepth) return fmt.Errorf("YAML nesting depth exceeds maximum (%d)", maxDepth)
} }
// Cycle detection: if we're currently visiting this node on the current
// recursion path, it's a cycle (e.g., alias pointing to an ancestor).
// Return nil to break the cycle without error — cycles are a structural
// property, not a depth violation.
if visiting[node] {
return nil
}
// Track total nodes visited as defense-in-depth against wide-but-shallow attacks. // Track total nodes visited as defense-in-depth against wide-but-shallow attacks.
// Placed after cycle detection but before the depth-aware short-circuit. This means
// nodes revisited at shallower depths (via aliases) are counted each time they are
// encountered — intentional conservative overcounting. This bounds the total work
// performed during validation rather than tracking unique nodes, which is the safer
// security posture for untrusted YAML input.
*nodeCount++ *nodeCount++
if *nodeCount > maxNodes { if *nodeCount > maxNodes {
return fmt.Errorf("YAML node count exceeds maximum (%d)", maxNodes) return fmt.Errorf("YAML node count exceeds maximum (%d)", maxNodes)
} }
// Depth-aware short-circuit: skip re-validation only when the current visit // Cycle detection: if we've seen this node before, we're in a cycle.
// depth is the same or shallower than the depth at which this node was if _, ok := seen[node]; ok {
// previously validated. A shallower (or equal) current depth means the return nil // Already validated this subtree, skip to avoid infinite recursion.
// prior, deeper validation already covered any subtree depth violations.
// If the current depth exceeds the previous validation depth (e.g., an alias
// references this node deeper in the tree), we must re-traverse to ensure
// the combined effective depth doesn't exceed maxDepth.
//
// Note: using ast.Node (interface) as map key relies on pointer identity,
// which is correct because all goccy/go-yaml AST node types are pointer
// receivers (*MappingNode, *SequenceNode, etc.), never value types.
if prevDepth, ok := validated[node]; ok && depth <= prevDepth {
return nil
} }
validated[node] = depth seen[node] = struct{}{}
// Mark as visiting (on the current recursion path) for cycle detection. // Handle alias nodes: follow the alias to its anchor target.
visiting[node] = true // Increment depth when following aliases since they expand the effective structure.
defer func() { visiting[node] = false }() if node.Kind == yaml.AliasNode && node.Alias != nil {
return checkYAMLDepth(node.Alias, depth+1, maxDepth, maxNodes, seen, nodeCount)
}
// Walk children based on node type. for _, child := range node.Content {
switch n := node.(type) { if err := checkYAMLDepth(child, depth+1, maxDepth, maxNodes, seen, nodeCount); err != nil {
case *ast.MappingNode:
for _, value := range n.Values {
if err := checkYAMLDepth(value, depth+1, maxDepth, maxNodes, validated, visiting, nodeCount); err != nil {
return err return err
} }
} }
case *ast.MappingValueNode:
// Both Key and Value are visited at depth+1 relative to this
// MappingValueNode. Since MappingNode visits its MappingValueNode
// children at depth+1 as well, keys and values end up at depth+2
// from the parent MappingNode. This is intentional: it mirrors the
// actual nesting structure (mapping → key-value pair → key/value).
if err := checkYAMLDepth(n.Key, depth+1, maxDepth, maxNodes, validated, visiting, nodeCount); err != nil {
return err
}
if err := checkYAMLDepth(n.Value, depth+1, maxDepth, maxNodes, validated, visiting, nodeCount); err != nil {
return err
}
case *ast.SequenceNode:
for _, value := range n.Values {
if err := checkYAMLDepth(value, depth+1, maxDepth, maxNodes, validated, visiting, nodeCount); err != nil {
return err
}
}
case *ast.AliasNode:
// Follow alias to its target, incrementing depth since aliases expand
// the effective structure.
if err := checkYAMLDepth(n.Value, depth+1, maxDepth, maxNodes, validated, visiting, nodeCount); err != nil {
return err
}
case *ast.AnchorNode:
// Increment depth for anchor values as a conservative measure: the
// anchor definition itself is structural, and treating it as a depth
// level ensures that deeply nested anchors are caught at definition
// time rather than only when referenced via alias. This +1 is
// asymmetric with alias (which also increments) — by design, the
// effective depth budget for anchored-then-aliased content is reduced
// because both the definition site and the reference site each consume
// a level, making deeply nested anchor/alias pairs hit the limit sooner.
if err := checkYAMLDepth(n.Value, depth+1, maxDepth, maxNodes, validated, visiting, nodeCount); err != nil {
return err
}
case *ast.TagNode:
if err := checkYAMLDepth(n.Value, depth+1, maxDepth, maxNodes, validated, visiting, nodeCount); err != nil {
return err
}
case *ast.MergeKeyNode:
// MergeKeyNode represents the literal "<<" merge key token. It has no
// child nodes — the value side of a merge (e.g., *alias) lives in the
// parent MappingValueNode.Value, which is already recursed into above.
// Explicitly listed here (rather than in the default case) to prevent
// future library changes from silently bypassing depth checks.
default:
// Scalar leaf nodes (StringNode, IntegerNode, FloatNode, BoolNode,
// NullNode, InfinityNode, NanNode, LiteralNode) have no children to
// recurse into.
}
return nil return nil
} }
// ParsePersonaBytes parses persona data from bytes with a source label for errors. // ParsePersonaBytes parses persona data from bytes with a source label for errors.
// This is useful for parsing personas fetched from external sources (e.g., Gitea API) // This is useful for parsing personas fetched from external sources (e.g., Gitea API)
// without requiring filesystem access. Format is detected by source extension. // without requiring filesystem access. Format is detected by source extension.
// Input is bounded by MaxPersonaFileSize to prevent resource exhaustion.
func ParsePersonaBytes(data []byte, source string) (*Persona, error) { func ParsePersonaBytes(data []byte, source string) (*Persona, error) {
if len(data) > MaxPersonaFileSize {
return nil, fmt.Errorf("persona data from %s exceeds maximum size (%d bytes, limit %d)", source, len(data), MaxPersonaFileSize)
}
return parsePersona(data, source) return parsePersona(data, source)
} }
+41 -222
View File
@@ -7,7 +7,7 @@ import (
"strings" "strings"
"testing" "testing"
"github.com/goccy/go-yaml/ast" "gopkg.in/yaml.v3"
) )
func TestLoadBuiltinPersona(t *testing.T) { func TestLoadBuiltinPersona(t *testing.T) {
@@ -459,14 +459,7 @@ func TestYAMLDeeplyNestedRejection(t *testing.T) {
path := filepath.Join(dir, "deeply-nested.yaml") path := filepath.Join(dir, "deeply-nested.yaml")
// Build a deeply nested YAML structure that exceeds MaxYAMLDepth (20). // Build a deeply nested YAML structure that exceeds MaxYAMLDepth (20).
// Depth accumulation trace for "nested: \n level0: \n level1: ...": // Each level adds 2 to the depth count (key + value mapping).
// - Document root parsed at depth 0
// - Root MappingNode children (MappingValueNodes) visited at depth 1
// - "nested" MappingValueNode: key at depth 2, value at depth 2
// - Each levelN adds depth via MappingValueNode traversal (key + value)
// - Exact depth per level depends on AST structure (MappingNode wrapping),
// but 25 levels reliably exceeds MaxYAMLDepth (20) with comfortable margin.
// The test uses 25 levels rather than exactly 21 to avoid brittleness.
var sb strings.Builder var sb strings.Builder
sb.WriteString("name: test\nidentity: test\nnested:\n") sb.WriteString("name: test\nidentity: test\nnested:\n")
indent := " " indent := " "
@@ -490,35 +483,6 @@ func TestYAMLDeeplyNestedRejection(t *testing.T) {
} }
} }
func TestYAMLEmptyFileRejection(t *testing.T) {
tests := []struct {
name string
content string
}{
{"completely_empty", ""},
{"whitespace_only", " \n\n "},
{"comment_only", "# just a comment\n"},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
dir := t.TempDir()
path := filepath.Join(dir, tc.name+".yaml")
if err := os.WriteFile(path, []byte(tc.content), 0644); err != nil {
t.Fatalf("failed to write test file: %v", err)
}
_, err := LoadPersona(path)
if err == nil {
t.Fatal("expected error for empty YAML input, got nil")
}
if !strings.Contains(err.Error(), "empty YAML document") {
t.Errorf("expected error containing %q, got: %v", "empty YAML document", err)
}
})
}
}
func TestYAMLFileSizeLimit(t *testing.T) { func TestYAMLFileSizeLimit(t *testing.T) {
dir := t.TempDir() dir := t.TempDir()
path := filepath.Join(dir, "huge.yaml") path := filepath.Join(dir, "huge.yaml")
@@ -540,41 +504,41 @@ func TestYAMLFileSizeLimit(t *testing.T) {
func TestYAMLAliasCycleDetection(t *testing.T) { func TestYAMLAliasCycleDetection(t *testing.T) {
// Test that our checkYAMLDepth function handles alias cycles gracefully // Test that our checkYAMLDepth function handles alias cycles gracefully
// by using the visiting map to prevent infinite recursion. // by using the seen map to prevent infinite recursion.
// We test this directly because go-yaml's parser handles most cycles
// at parse time, but we need to ensure our checker is robust.
// Create a node structure where an alias points to a parent node, // Create a node structure where an alias points to a parent node,
// simulating what could happen with crafted input. // simulating what could happen with malicious input that bypasses
parent := &ast.MappingNode{ // go-yaml's cycle detection.
Values: []*ast.MappingValueNode{ parent := &yaml.Node{
{ Kind: yaml.MappingNode,
Key: &ast.StringNode{Value: "name"}, Content: []*yaml.Node{
Value: &ast.StringNode{Value: "test"}, {Kind: yaml.ScalarNode, Value: "name"},
}, {Kind: yaml.ScalarNode, Value: "test"},
{Kind: yaml.ScalarNode, Value: "nested"},
}, },
} }
// Create a child that aliases back to the parent (artificial cycle) // Create a child that aliases back to the parent (artificial cycle)
aliasToParent := &ast.AliasNode{ aliasToParent := &yaml.Node{
Value: parent, Kind: yaml.AliasNode,
Alias: parent,
} }
parent.Values = append(parent.Values, &ast.MappingValueNode{ parent.Content = append(parent.Content, aliasToParent)
Key: &ast.StringNode{Value: "nested"},
Value: aliasToParent,
})
nodeCount := 0 nodeCount := 0
validated := make(map[ast.Node]int) seen := make(map[*yaml.Node]struct{})
visiting := make(map[ast.Node]bool)
// This should NOT hang or stack overflow - cycle detection prevents infinite recursion // This should NOT hang or stack overflow - the seen map prevents infinite recursion
err := checkYAMLDepth(parent, 0, MaxYAMLDepth, MaxYAMLNodes, validated, visiting, &nodeCount) err := checkYAMLDepth(parent, 0, MaxYAMLDepth, MaxYAMLNodes, seen, &nodeCount)
if err != nil { if err != nil {
t.Errorf("unexpected error traversing cyclic structure: %v", err) t.Errorf("unexpected error traversing cyclic structure: %v", err)
} }
// Verify we tracked the parent in the validated map // Verify we tracked the parent in the seen map
if _, ok := validated[parent]; !ok { if _, ok := seen[parent]; !ok {
t.Error("parent node not tracked in validated map") t.Error("parent node not tracked in seen map")
} }
} }
@@ -630,82 +594,36 @@ func TestYAMLNodeCountLimit(t *testing.T) {
func TestCheckYAMLDepthCycleDetectionDirect(t *testing.T) { func TestCheckYAMLDepthCycleDetectionDirect(t *testing.T) {
// Direct test of cycle detection in checkYAMLDepth by creating // Direct test of cycle detection in checkYAMLDepth by creating
// a node structure with an artificial cycle. // a node structure with an artificial cycle.
node := &ast.MappingNode{ // This tests the seen map logic independent of go-yaml's parsing.
Values: []*ast.MappingValueNode{ node := &yaml.Node{
{ Kind: yaml.MappingNode,
Key: &ast.StringNode{Value: "key"}, Content: []*yaml.Node{
Value: &ast.StringNode{Value: "value"}, {Kind: yaml.ScalarNode, Value: "key"},
}, {Kind: yaml.ScalarNode, Value: "value"},
}, },
} }
// Create a cycle by making a child reference the parent // Create a cycle by making a child reference the parent
cycleChild := &ast.AliasNode{ cycleChild := &yaml.Node{
Value: node, // Points back to the parent Kind: yaml.AliasNode,
Alias: node, // Points back to the parent
} }
node.Values = append(node.Values, &ast.MappingValueNode{ node.Content = append(node.Content,
Key: &ast.StringNode{Value: "cyclic"}, &yaml.Node{Kind: yaml.ScalarNode, Value: "cyclic"},
Value: cycleChild, cycleChild,
}) )
nodeCount := 0 nodeCount := 0
validated := make(map[ast.Node]int) seen := make(map[*yaml.Node]struct{})
visiting := make(map[ast.Node]bool) err := checkYAMLDepth(node, 0, MaxYAMLDepth, MaxYAMLNodes, seen, &nodeCount)
err := checkYAMLDepth(node, 0, MaxYAMLDepth, MaxYAMLNodes, validated, visiting, &nodeCount)
// Should complete without infinite recursion due to cycle detection // Should complete without infinite recursion due to cycle detection
if err != nil { if err != nil {
t.Errorf("unexpected error: %v", err) t.Errorf("unexpected error: %v", err)
} }
// The validated map should contain multiple entries // The seen map should contain multiple entries
if len(validated) < 2 { if len(seen) < 2 {
t.Errorf("validated map has %d entries, expected at least 2", len(validated)) t.Errorf("seen map has %d entries, expected at least 2", len(seen))
}
}
func TestYAMLAliasDepthBypass(t *testing.T) {
// Test that an anchored subtree first validated at a shallow depth is
// re-checked when referenced via alias at a deeper position. Without the
// depth-aware validated map, the alias reference would skip re-checking
// and allow the effective nesting to exceed MaxYAMLDepth.
dir := t.TempDir()
path := filepath.Join(dir, "alias-depth-bypass.yaml")
// Build YAML with an anchor at shallow depth containing a subtree near the limit,
// then reference it via alias deep enough that effective depth exceeds MaxYAMLDepth.
var sb strings.Builder
sb.WriteString("name: test\nidentity: test\n")
// Create the anchored subtree at depth 1 (key level) that nests 15 levels deep.
sb.WriteString("anchor_key: &deep_anchor\n")
for i := 0; i < 15; i++ {
sb.WriteString(strings.Repeat(" ", i+1))
sb.WriteString(fmt.Sprintf("level%d:\n", i))
}
sb.WriteString(strings.Repeat(" ", 16))
sb.WriteString("leaf: value\n")
// Create a wrapper that nests 6 levels deep, then references the anchor.
// Effective depth at alias target = 6 (wrapper nesting) + 1 (alias) + 15 (subtree) = 22 > 20
sb.WriteString("wrapper:\n")
for i := 0; i < 6; i++ {
sb.WriteString(strings.Repeat(" ", i+1))
sb.WriteString(fmt.Sprintf("n%d:\n", i))
}
sb.WriteString(strings.Repeat(" ", 7))
sb.WriteString("alias_ref: *deep_anchor\n")
if err := os.WriteFile(path, []byte(sb.String()), 0644); err != nil {
t.Fatalf("failed to write test file: %v", err)
}
_, err := LoadPersona(path)
if err == nil {
t.Fatal("expected error for alias depth bypass, got nil")
}
if !strings.Contains(err.Error(), "nesting depth exceeds") {
t.Errorf("error = %q, want containing 'nesting depth exceeds'", err.Error())
} }
} }
@@ -858,102 +776,3 @@ identity: test identity
t.Errorf("Name = %q, want %q", p.Name, "test") t.Errorf("Name = %q, want %q", p.Name, "test")
} }
} }
func TestJSONTrailingContentRejected(t *testing.T) {
tests := []struct {
name string
content string
}{
{
name: "trailing garbage after object",
content: `{"name":"test","identity":"test identity"}garbage`,
},
{
name: "two JSON objects",
content: `{"name":"test","identity":"test identity"}{"name":"other"}`,
},
{
name: "trailing array",
content: `{"name":"test","identity":"test identity"}[]`,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
dir := t.TempDir()
path := filepath.Join(dir, "test.json")
if err := os.WriteFile(path, []byte(tt.content), 0644); err != nil {
t.Fatalf("failed to write test file: %v", err)
}
_, err := LoadPersona(path)
if err == nil {
t.Fatal("expected error for trailing content, got nil")
}
if !strings.Contains(err.Error(), "trailing content") {
t.Errorf("error = %q, want to contain 'trailing content'", err.Error())
}
})
}
}
func TestParsePersonaBytesSizeLimit(t *testing.T) {
// ParsePersonaBytes should reject input exceeding MaxPersonaFileSize
oversized := make([]byte, MaxPersonaFileSize+1)
for i := range oversized {
oversized[i] = 'x'
}
_, err := ParsePersonaBytes(oversized, "oversized.yaml")
if err == nil {
t.Fatal("expected error for oversized input, got nil")
}
if !strings.Contains(err.Error(), "exceeds maximum size") {
t.Errorf("error = %q, want to contain 'exceeds maximum size'", err.Error())
}
// Just under the limit should not trigger size error (may fail parse, but not size)
underLimit := []byte("name: test\nidentity: test persona\n")
p, err := ParsePersonaBytes(underLimit, "valid.yaml")
if err != nil {
t.Fatalf("unexpected error for valid input: %v", err)
}
if p.Name != "test" {
t.Errorf("Name = %q, want %q", p.Name, "test")
}
}
func TestYAMLMergeKeyDepthCheck(t *testing.T) {
// Verify that YAML merge keys (<<: *alias) are properly handled by the
// depth checker. The merge key content is in the MappingValueNode.Value
// (an AliasNode), not in the MergeKeyNode itself.
p, err := ParsePersonaBytes([]byte("name: merge-test\nidentity: test\n"), "merge.yaml")
if err != nil {
t.Fatalf("basic parse failed: %v", err)
}
if p.Name != "merge-test" {
t.Errorf("Name = %q, want %q", p.Name, "merge-test")
}
// Test that deeply nested merge keys still hit depth limit.
// Build YAML with merge key content nested beyond MaxYAMLDepth.
var sb strings.Builder
sb.WriteString("name: deep-merge\nidentity: deep merge persona\n")
sb.WriteString("anchor: &deep\n")
indent := " "
for i := 0; i < MaxYAMLDepth+5; i++ {
sb.WriteString(indent)
sb.WriteString(fmt.Sprintf("level%d:\n", i))
indent += " "
}
sb.WriteString(indent + "leaf: value\n")
sb.WriteString("target:\n <<: *deep\n")
_, err = ParsePersonaBytes([]byte(sb.String()), "deep-merge.yaml")
if err == nil {
t.Fatal("expected error for deeply nested merge key content, got nil")
}
if !strings.Contains(err.Error(), "depth") {
t.Errorf("error = %q, want to contain 'depth'", err.Error())
}
}
+9
View File
@@ -49,3 +49,12 @@ type Client interface {
type ReviewerSelfRequester interface { type ReviewerSelfRequester interface {
RequestReviewerSelf(ctx context.Context, owner, repo string, number int, user string) error 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
}
+26
View File
@@ -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"
)
// 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)
}
+5
View File
@@ -94,5 +94,10 @@ type ReviewRequest struct {
Body string `json:"body"` Body string `json:"body"`
// Event is the review action (approve, request changes, or comment). // Event is the review action (approve, request changes, or comment).
Event ReviewEvent `json:"event"` Event ReviewEvent `json:"event"`
// CommitID anchors the review to a specific commit SHA.
// If empty, the platform defaults to the current PR head.
// Adapters use this as the primary commit anchor for the review submission.
CommitID string `json:"commit_id,omitempty"`
Comments []ReviewComment `json:"comments,omitempty"` Comments []ReviewComment `json:"comments,omitempty"`
} }