feat: load personas from target repo .review-bot/personas/ #61

Merged
aweiker merged 4 commits from issue-60 into main 2026-05-11 02:54:46 +00:00
Owner

Summary

Implements #60: Load personas from target repo .review-bot/personas/.

Changes

review/repo_persona.go (new file)

  • GiteaClient interface for fetching repo files
  • ContentEntry type mirroring gitea.ContentEntry
  • LoadRepoPersonas() to fetch and parse personas from repo
  • MergePersonas() to combine built-in and repo personas (repo takes precedence)
  • GetBuiltinPersonasMap() helper for loading all built-ins as map

review/persona.go

  • Add ParsePersonaBytes() for parsing personas from byte data (for external sources)

cmd/review-bot/main.go

  • Move persona loading after Gitea client initialization
  • When --persona is specified, check repo personas first, fall back to built-in
  • Add giteaClientAdapter to bridge gitea.Client to review.GiteaClient interface
  • Log whether persona was loaded from repo or built-in

Usage

Repos can now add domain-specific reviewers:

.review-bot/
  personas/
    trading.yaml      # Trading expert for gargoyle
    crypto.yaml       # Crypto expert for kms-lite
# trading.yaml
name: trading
display_name: Trading Expert
identity: |
  You are a trading domain expert...
focus:
  - order handling
  - position management
severity:
  major: Critical trading logic errors
  minor: Suboptimal patterns
  nit: Style suggestions

Testing

  • All existing tests pass
  • New tests for ParsePersonaBytes
  • New tests for LoadRepoPersonas (directory not found, empty, valid personas, invalid files skipped, auth errors propagated)
  • New tests for MergePersonas (override behavior, original maps not modified)
  • New tests for GetBuiltinPersonasMap
  • New tests for isYAMLFile and isNotFoundError
  • go vet ./... passes
  • scripts/check-deps.sh passes

Closes #60

## Summary Implements #60: Load personas from target repo `.review-bot/personas/`. ## Changes ### review/repo_persona.go (new file) - `GiteaClient` interface for fetching repo files - `ContentEntry` type mirroring gitea.ContentEntry - `LoadRepoPersonas()` to fetch and parse personas from repo - `MergePersonas()` to combine built-in and repo personas (repo takes precedence) - `GetBuiltinPersonasMap()` helper for loading all built-ins as map ### review/persona.go - Add `ParsePersonaBytes()` for parsing personas from byte data (for external sources) ### cmd/review-bot/main.go - Move persona loading after Gitea client initialization - When `--persona` is specified, check repo personas first, fall back to built-in - Add `giteaClientAdapter` to bridge gitea.Client to review.GiteaClient interface - Log whether persona was loaded from repo or built-in ## Usage Repos can now add domain-specific reviewers: ``` .review-bot/ personas/ trading.yaml # Trading expert for gargoyle crypto.yaml # Crypto expert for kms-lite ``` ```yaml # trading.yaml name: trading display_name: Trading Expert identity: | You are a trading domain expert... focus: - order handling - position management severity: major: Critical trading logic errors minor: Suboptimal patterns nit: Style suggestions ``` ## Testing - [x] All existing tests pass - [x] New tests for `ParsePersonaBytes` - [x] New tests for `LoadRepoPersonas` (directory not found, empty, valid personas, invalid files skipped, auth errors propagated) - [x] New tests for `MergePersonas` (override behavior, original maps not modified) - [x] New tests for `GetBuiltinPersonasMap` - [x] New tests for `isYAMLFile` and `isNotFoundError` - [x] `go vet ./...` passes - [x] `scripts/check-deps.sh` passes Closes #60
rodin added 1 commit 2026-05-11 02:06:24 +00:00
feat: load personas from target repo .review-bot/personas/
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 9m32s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 10m10s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 10m51s
CI / review (gpt-5, security, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 10m33s
3f06ba2ea6
Implements #60.

- Add ParsePersonaBytes() for parsing personas from byte data
- Add LoadRepoPersonas() to fetch personas from repo via Gitea API
- Add MergePersonas() to combine built-in and repo personas
- Add GetBuiltinPersonasMap() helper
- Update main.go to load repo personas first, fall back to built-in
- Add giteaClientAdapter to bridge gitea.Client to review.GiteaClient

When --persona is specified, the bot now:
1. Attempts to fetch personas from .review-bot/personas/*.yaml
2. If the named persona exists in the repo, uses it
3. Otherwise falls back to built-in personas

This allows repos to define domain-specific personas (e.g., trading
experts for gargoyle, crypto experts for kms-lite) without modifying
the review-bot codebase.
rodin force-pushed issue-60 from c1c8e61e9d to 3f06ba2ea6 2026-05-11 02:06:24 +00:00 Compare
Author
Owner

Self-review: PASS

No issues found — ready for human review.

Review notes:

Coherence: Single clear purpose (load personas from target repo). Scope is right — adds types, loading logic, integration, and tests. No unrelated changes.

Fit:

  • Interface pattern is new to this codebase but necessary to avoid import cycles (review → gitea would create cycle)
  • Logging uses existing slog.Warn/slog.Debug/slog.Info patterns
  • Error handling follows existing conventions (fmt.Errorf with %w where appropriate, log-and-skip for non-fatal errors)
  • Test structure matches existing patterns (table-driven, subtests)

Solution:

  • Simple approach: fetch directory, parse files, return map
  • Handles all edge cases: 404 (expected), auth errors (propagate), parse failures (skip), empty dir (return empty)
  • Interface adapter pattern bridges gitea.Client to review.GiteaClient without import cycles
  • String-based 404 detection is necessary given the package boundary; matches the format APIError.Error() produces

Completeness:

  • All acceptance criteria met:
    • Fetch .review-bot/personas/*.yaml from target repo via Gitea API
    • Parse YAML personas with same schema as built-in
    • Fall back to built-in if directory missing or empty
    • Tests for persona loading from remote repo
  • All exported functions have tests
  • go test ./... passes
  • go vet ./... passes
  • scripts/check-deps.sh passes
## Self-review: PASS No issues found — ready for human review. **Review notes:** **Coherence:** ✅ Single clear purpose (load personas from target repo). Scope is right — adds types, loading logic, integration, and tests. No unrelated changes. **Fit:** ✅ - Interface pattern is new to this codebase but necessary to avoid import cycles (review → gitea would create cycle) - Logging uses existing `slog.Warn`/`slog.Debug`/`slog.Info` patterns - Error handling follows existing conventions (`fmt.Errorf` with `%w` where appropriate, log-and-skip for non-fatal errors) - Test structure matches existing patterns (table-driven, subtests) **Solution:** ✅ - Simple approach: fetch directory, parse files, return map - Handles all edge cases: 404 (expected), auth errors (propagate), parse failures (skip), empty dir (return empty) - Interface adapter pattern bridges gitea.Client to review.GiteaClient without import cycles - String-based 404 detection is necessary given the package boundary; matches the format `APIError.Error()` produces **Completeness:** ✅ - All acceptance criteria met: - [x] Fetch `.review-bot/personas/*.yaml` from target repo via Gitea API - [x] Parse YAML personas with same schema as built-in - [x] Fall back to built-in if directory missing or empty - [x] Tests for persona loading from remote repo - All exported functions have tests - `go test ./...` passes - `go vet ./...` passes - `scripts/check-deps.sh` passes
rodin added the readyself-reviewed labels 2026-05-11 02:08:26 +00:00
aweiker was assigned by rodin 2026-05-11 02:08:26 +00:00
sonnet-review-bot approved these changes 2026-05-11 02:16:46 +00:00
sonnet-review-bot left a comment
First-time contributor

Original review

Supersededsee current review for up-to-date findings.

Previous findings (commit 3f06ba2e)

Sonnet Review

Summary

Clean, well-structured implementation of repo persona loading. The code follows established patterns correctly, has thorough test coverage, and CI passes. Two minor issues worth noting: a duplicate flag.Parse() call and a subtle nil-map access risk when LoadRepoPersonas returns an error.

Findings

# Severity File Line Finding
1 [MINOR] cmd/review-bot/main.go 103 Duplicate flag.Parse() call. The file calls flag.Parse() twice (lines ~100 and ~103 in the full file). The second call is a no-op but is dead code that should be removed.
2 [MINOR] cmd/review-bot/main.go 185 When LoadRepoPersonas returns an error, repoPersonas will be nil (not an empty map), but the subsequent repoPersonas[*personaName] map index on line ~187 is safe in Go (indexing a nil map returns the zero value, false). However, the err variable from LoadRepoPersonas is shadowed by the later persona, err = review.LoadBuiltinPersona(...) assignment at line ~193, which is correct since the outer err is already declared. The logic is sound but slightly subtle — consider using a blank identifier or a named variable repoErr to make the intent clearer.
3 [NIT] review/repo_persona.go 133 isNotFoundError uses a string-based heuristic (strings.Contains(errStr, "not found")) that could produce false positives for errors like "user not found" from application logic. The HTTP 404 check is precise; the bare not found check is looser. This is acknowledged in the comment but worth considering whether to tighten to only the HTTP 404 pattern.
4 [NIT] review/repo_persona_test.go 18 Minor formatting inconsistency: the TestParsePersonaBytes table struct has inconsistent field alignment (extra blank line between source and wantName fields in some entries). This is cosmetic and gofmt should handle it, but it's visible in the raw source.

Recommendation

APPROVE — Approve with the recommendation to fix the duplicate flag.Parse() call before or shortly after merge — it's dead code that could confuse future readers. The nil-map access on an errored repoPersonas return is safe in Go but the error shadowing pattern is slightly subtle. The isNotFoundError heuristic is documented and acceptable given the import-cycle constraint preventing use of a typed error. Overall, this is a well-designed feature: proper interface abstraction to avoid import cycles, thorough test coverage following table-driven patterns, correct fallback behavior, and appropriate error propagation semantics.


Review by sonnet


Evaluated against 3f06ba2e

~~Original review~~ **Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/61#pullrequestreview-2226) for up-to-date findings. <details><summary>Previous findings (commit 3f06ba2e)</summary> # Sonnet Review ## Summary Clean, well-structured implementation of repo persona loading. The code follows established patterns correctly, has thorough test coverage, and CI passes. Two minor issues worth noting: a duplicate `flag.Parse()` call and a subtle nil-map access risk when `LoadRepoPersonas` returns an error. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `cmd/review-bot/main.go` | 103 | Duplicate `flag.Parse()` call. The file calls `flag.Parse()` twice (lines ~100 and ~103 in the full file). The second call is a no-op but is dead code that should be removed. | | 2 | [MINOR] | `cmd/review-bot/main.go` | 185 | When `LoadRepoPersonas` returns an error, `repoPersonas` will be nil (not an empty map), but the subsequent `repoPersonas[*personaName]` map index on line ~187 is safe in Go (indexing a nil map returns the zero value, false). However, the `err` variable from `LoadRepoPersonas` is shadowed by the later `persona, err = review.LoadBuiltinPersona(...)` assignment at line ~193, which is correct since the outer `err` is already declared. The logic is sound but slightly subtle — consider using a blank identifier or a named variable `repoErr` to make the intent clearer. | | 3 | [NIT] | `review/repo_persona.go` | 133 | `isNotFoundError` uses a string-based heuristic (`strings.Contains(errStr, "not found")`) that could produce false positives for errors like "user not found" from application logic. The `HTTP 404` check is precise; the bare `not found` check is looser. This is acknowledged in the comment but worth considering whether to tighten to only the `HTTP 404` pattern. | | 4 | [NIT] | `review/repo_persona_test.go` | 18 | Minor formatting inconsistency: the `TestParsePersonaBytes` table struct has inconsistent field alignment (extra blank line between `source` and `wantName` fields in some entries). This is cosmetic and `gofmt` should handle it, but it's visible in the raw source. | ## Recommendation **APPROVE** — Approve with the recommendation to fix the duplicate `flag.Parse()` call before or shortly after merge — it's dead code that could confuse future readers. The nil-map access on an errored `repoPersonas` return is safe in Go but the error shadowing pattern is slightly subtle. The `isNotFoundError` heuristic is documented and acceptable given the import-cycle constraint preventing use of a typed error. Overall, this is a well-designed feature: proper interface abstraction to avoid import cycles, thorough test coverage following table-driven patterns, correct fallback behavior, and appropriate error propagation semantics. --- *Review by sonnet* <!-- review-bot:sonnet --> --- *Evaluated against 3f06ba2e* </details> <!-- review-bot:sonnet -->
@@ -199,0 +182,4 @@
if err != nil {
slog.Warn("could not load repo personas", "repo", owner+"/"+repoName, "error", err)
// Continue with built-in personas only
}
First-time contributor

[MINOR] When LoadRepoPersonas returns an error, repoPersonas will be nil (not an empty map), but the subsequent repoPersonas[*personaName] map index on line ~187 is safe in Go (indexing a nil map returns the zero value, false). However, the err variable from LoadRepoPersonas is shadowed by the later persona, err = review.LoadBuiltinPersona(...) assignment at line ~193, which is correct since the outer err is already declared. The logic is sound but slightly subtle — consider using a blank identifier or a named variable repoErr to make the intent clearer.

**[MINOR]** When `LoadRepoPersonas` returns an error, `repoPersonas` will be nil (not an empty map), but the subsequent `repoPersonas[*personaName]` map index on line ~187 is safe in Go (indexing a nil map returns the zero value, false). However, the `err` variable from `LoadRepoPersonas` is shadowed by the later `persona, err = review.LoadBuiltinPersona(...)` assignment at line ~193, which is correct since the outer `err` is already declared. The logic is sound but slightly subtle — consider using a blank identifier or a named variable `repoErr` to make the intent clearer.
@@ -0,0 +130,4 @@
// isNotFoundError checks if an error represents a 404 response.
// This is a string-based heuristic since we don't have access to gitea.APIError here.
func isNotFoundError(err error) bool {
if err == nil {
First-time contributor

[NIT] isNotFoundError uses a string-based heuristic (strings.Contains(errStr, "not found")) that could produce false positives for errors like "user not found" from application logic. The HTTP 404 check is precise; the bare not found check is looser. This is acknowledged in the comment but worth considering whether to tighten to only the HTTP 404 pattern.

**[NIT]** `isNotFoundError` uses a string-based heuristic (`strings.Contains(errStr, "not found")`) that could produce false positives for errors like "user not found" from application logic. The `HTTP 404` check is precise; the bare `not found` check is looser. This is acknowledged in the comment but worth considering whether to tighten to only the `HTTP 404` pattern.
@@ -0,0 +15,4 @@
wantName string
wantErr string
}{
{
First-time contributor

[NIT] Minor formatting inconsistency: the TestParsePersonaBytes table struct has inconsistent field alignment (extra blank line between source and wantName fields in some entries). This is cosmetic and gofmt should handle it, but it's visible in the raw source.

**[NIT]** Minor formatting inconsistency: the `TestParsePersonaBytes` table struct has inconsistent field alignment (extra blank line between `source` and `wantName` fields in some entries). This is cosmetic and `gofmt` should handle it, but it's visible in the raw source.
gpt-review-bot approved these changes 2026-05-11 02:17:15 +00:00
gpt-review-bot left a comment
First-time contributor

Original review

Supersededsee current review for up-to-date findings.

Previous findings (commit 3f06ba2e)

Gpt Review

Summary

The changes cleanly add support for loading repo-specific personas and correctly defer persona selection until after Gitea initialization. Implementation is idiomatic, well-tested, and CI passes.

Findings

# Severity File Line Finding
1 [MINOR] review/repo_persona.go 64 LoadRepoPersonas parses repo personas via ParsePersonaBytes without enforcing MaxPersonaFileSize. A very large YAML file with few nodes (e.g., a huge identity string) could still be fetched and parsed, potentially increasing memory usage. Consider checking len(content) against MaxPersonaFileSize before parsing.
2 [NIT] review/repo_persona.go 121 isNotFoundError uses broad string matching ("not found") which may incorrectly classify unrelated errors as 404s. If possible, narrow the check (e.g., look for an API error type or more specific message prefix) to reduce false positives.
3 [NIT] review/persona.go 231 ParsePersonaBytes delegates to parsePersona, which is fine, but does not apply any size checks like LoadPersona. Consider documenting that size limits are only enforced for filesystem sources, or add a size check for external sources if appropriate.

Recommendation

APPROVE — Overall the PR is solid: it defers persona loading to enable repo-based personas, adds a clean adapter for the Gitea client, and includes comprehensive tests. CI has passed. Consider adding a content size check for repo persona files in LoadRepoPersonas (similar to MaxPersonaFileSize for local files) and slightly tightening the isNotFoundError heuristic. These are minor improvements and do not block approval.


Review by gpt


Evaluated against 3f06ba2e

~~Original review~~ **Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/61#pullrequestreview-2227) for up-to-date findings. <details><summary>Previous findings (commit 3f06ba2e)</summary> # Gpt Review ## Summary The changes cleanly add support for loading repo-specific personas and correctly defer persona selection until after Gitea initialization. Implementation is idiomatic, well-tested, and CI passes. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `review/repo_persona.go` | 64 | LoadRepoPersonas parses repo personas via ParsePersonaBytes without enforcing MaxPersonaFileSize. A very large YAML file with few nodes (e.g., a huge identity string) could still be fetched and parsed, potentially increasing memory usage. Consider checking len(content) against MaxPersonaFileSize before parsing. | | 2 | [NIT] | `review/repo_persona.go` | 121 | isNotFoundError uses broad string matching ("not found") which may incorrectly classify unrelated errors as 404s. If possible, narrow the check (e.g., look for an API error type or more specific message prefix) to reduce false positives. | | 3 | [NIT] | `review/persona.go` | 231 | ParsePersonaBytes delegates to parsePersona, which is fine, but does not apply any size checks like LoadPersona. Consider documenting that size limits are only enforced for filesystem sources, or add a size check for external sources if appropriate. | ## Recommendation **APPROVE** — Overall the PR is solid: it defers persona loading to enable repo-based personas, adds a clean adapter for the Gitea client, and includes comprehensive tests. CI has passed. Consider adding a content size check for repo persona files in LoadRepoPersonas (similar to MaxPersonaFileSize for local files) and slightly tightening the isNotFoundError heuristic. These are minor improvements and do not block approval. --- *Review by gpt* <!-- review-bot:gpt --> --- *Evaluated against 3f06ba2e* </details> <!-- review-bot:gpt -->
@@ -227,0 +228,4 @@
// This is useful for parsing personas fetched from external sources (e.g., Gitea API)
// without requiring filesystem access. Format is detected by source extension.
func ParsePersonaBytes(data []byte, source string) (*Persona, error) {
return parsePersona(data, source)
First-time contributor

[NIT] ParsePersonaBytes delegates to parsePersona, which is fine, but does not apply any size checks like LoadPersona. Consider documenting that size limits are only enforced for filesystem sources, or add a size check for external sources if appropriate.

**[NIT]** ParsePersonaBytes delegates to parsePersona, which is fine, but does not apply any size checks like LoadPersona. Consider documenting that size limits are only enforced for filesystem sources, or add a size check for external sources if appropriate.
@@ -0,0 +61,4 @@
slog.Warn("could not fetch repo persona file",
"file", entry.Path,
"repo", owner+"/"+repo,
"error", err)
First-time contributor

[MINOR] LoadRepoPersonas parses repo personas via ParsePersonaBytes without enforcing MaxPersonaFileSize. A very large YAML file with few nodes (e.g., a huge identity string) could still be fetched and parsed, potentially increasing memory usage. Consider checking len(content) against MaxPersonaFileSize before parsing.

**[MINOR]** LoadRepoPersonas parses repo personas via ParsePersonaBytes without enforcing MaxPersonaFileSize. A very large YAML file with few nodes (e.g., a huge identity string) could still be fetched and parsed, potentially increasing memory usage. Consider checking len(content) against MaxPersonaFileSize before parsing.
@@ -0,0 +118,4 @@
}
result[name] = p
}
return result
First-time contributor

[NIT] isNotFoundError uses broad string matching ("not found") which may incorrectly classify unrelated errors as 404s. If possible, narrow the check (e.g., look for an API error type or more specific message prefix) to reduce false positives.

**[NIT]** isNotFoundError uses broad string matching ("not found") which may incorrectly classify unrelated errors as 404s. If possible, narrow the check (e.g., look for an API error type or more specific message prefix) to reduce false positives.
security-review-bot requested review from security-review-bot 2026-05-11 02:17:34 +00:00
security-review-bot requested changes 2026-05-11 02:17:34 +00:00
security-review-bot left a comment
Collaborator

Security Review

Summary

Good feature addition with strict YAML parsing and tests; however, repo-loaded personas are parsed without enforcing the existing MaxPersonaFileSize limit, creating a potential resource exhaustion vector. CI is green, but this security issue must be addressed.

Findings

# Severity File Line Finding
1 [MAJOR] review/repo_persona.go 63 Repo persona content is parsed without enforcing a maximum size. An attacker can commit an excessively large YAML persona (e.g., single huge scalar) to the repo, causing high memory/CPU usage when the bot fetches and parses it. Unlike LoadPersona() (filesystem), LoadRepoPersonas()/ParsePersonaBytes() do not check MaxPersonaFileSize, enabling a DoS vector.
2 [MINOR] review/repo_persona.go 104 isNotFoundError uses a broad substring match ("HTTP 404" or "not found"). This can misclassify non-404 errors that contain "not found" (e.g., upstream message wording) and silently skip persona loading. While not directly exploitable, it can mask auth or transport errors.

Recommendation

REQUEST_CHANGES — Add a strict size check for repo persona content before parsing to prevent resource exhaustion. Recommended options:

  • Enforce MaxPersonaFileSize in LoadRepoPersonas() after fetching content (e.g., if len(content) > MaxPersonaFileSize { log/skip }).
  • Preferably, enforce the limit in ParsePersonaBytes() itself so all external parse paths (including future uses) uniformly apply size constraints, mirroring LoadPersona().
    Also consider tightening isNotFoundError by checking a typed error/status code from the adapter rather than substring matching. You could extend the GiteaClient interface or the adapter to surface an explicit 404 indicator so non-404 errors are propagated correctly. Once the size limit is enforced, the PR can be approved.

Review by security


Evaluated against 3f06ba2e

# Security Review ## Summary Good feature addition with strict YAML parsing and tests; however, repo-loaded personas are parsed without enforcing the existing MaxPersonaFileSize limit, creating a potential resource exhaustion vector. CI is green, but this security issue must be addressed. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MAJOR] | `review/repo_persona.go` | 63 | Repo persona content is parsed without enforcing a maximum size. An attacker can commit an excessively large YAML persona (e.g., single huge scalar) to the repo, causing high memory/CPU usage when the bot fetches and parses it. Unlike LoadPersona() (filesystem), LoadRepoPersonas()/ParsePersonaBytes() do not check MaxPersonaFileSize, enabling a DoS vector. | | 2 | [MINOR] | `review/repo_persona.go` | 104 | isNotFoundError uses a broad substring match ("HTTP 404" or "not found"). This can misclassify non-404 errors that contain "not found" (e.g., upstream message wording) and silently skip persona loading. While not directly exploitable, it can mask auth or transport errors. | ## Recommendation **REQUEST_CHANGES** — Add a strict size check for repo persona content before parsing to prevent resource exhaustion. Recommended options: - Enforce MaxPersonaFileSize in LoadRepoPersonas() after fetching content (e.g., if len(content) > MaxPersonaFileSize { log/skip }). - Preferably, enforce the limit in ParsePersonaBytes() itself so all external parse paths (including future uses) uniformly apply size constraints, mirroring LoadPersona(). Also consider tightening isNotFoundError by checking a typed error/status code from the adapter rather than substring matching. You could extend the GiteaClient interface or the adapter to surface an explicit 404 indicator so non-404 errors are propagated correctly. Once the size limit is enforced, the PR can be approved. --- *Review by security* <!-- review-bot:security --> --- *Evaluated against 3f06ba2e*
@@ -0,0 +60,4 @@
if err != nil {
slog.Warn("could not fetch repo persona file",
"file", entry.Path,
"repo", owner+"/"+repo,
Collaborator

[MAJOR] Repo persona content is parsed without enforcing a maximum size. An attacker can commit an excessively large YAML persona (e.g., single huge scalar) to the repo, causing high memory/CPU usage when the bot fetches and parses it. Unlike LoadPersona() (filesystem), LoadRepoPersonas()/ParsePersonaBytes() do not check MaxPersonaFileSize, enabling a DoS vector.

**[MAJOR]** Repo persona content is parsed without enforcing a maximum size. An attacker can commit an excessively large YAML persona (e.g., single huge scalar) to the repo, causing high memory/CPU usage when the bot fetches and parses it. Unlike LoadPersona() (filesystem), LoadRepoPersonas()/ParsePersonaBytes() do not check MaxPersonaFileSize, enabling a DoS vector.
@@ -0,0 +101,4 @@
slog.Debug("repo persona overrides built-in", "name", name)
}
result[name] = p
}
Collaborator

[MINOR] isNotFoundError uses a broad substring match ("HTTP 404" or "not found"). This can misclassify non-404 errors that contain "not found" (e.g., upstream message wording) and silently skip persona loading. While not directly exploitable, it can mask auth or transport errors.

**[MINOR]** isNotFoundError uses a broad substring match ("HTTP 404" or "not found"). This can misclassify non-404 errors that contain "not found" (e.g., upstream message wording) and silently skip persona loading. While not directly exploitable, it can mask auth or transport errors.
sonnet-review-bot approved these changes 2026-05-11 02:22:58 +00:00
sonnet-review-bot left a comment
First-time contributor

Original review

Supersededsee current review for up-to-date findings.

Previous findings (commit 3f06ba2e)

Sonnet Review

Summary

Clean, well-structured implementation of repo persona loading. The code follows established patterns (small interface, adapter pattern, graceful degradation), is well-tested, and CI passes. There are a few minor issues worth noting but none are blockers.

Findings

# Severity File Line Finding
1 [MINOR] cmd/review-bot/main.go 183 When LoadRepoPersonas returns an error (non-404), the code logs a warning and continues with repoPersonas being nil (its zero value). The subsequent if p, ok := repoPersonas[*personaName] safely handles nil maps (map indexing on nil returns zero value), so this works correctly, but it's subtle. A reader might wonder if repoPersonas could cause a panic. Consider initializing repoPersonas to an empty map before the error check, or adding a brief comment that nil map indexing is safe in Go.
2 [MINOR] cmd/review-bot/main.go 78 flag.Parse() is called twice (lines 78 and 79 in the full file). This is a pre-existing bug not introduced by this PR, but it's worth noting since the diff shows the surrounding context.
3 [MINOR] review/repo_persona.go 128 isNotFoundError uses string-based heuristics ("not found" case-insensitive substring match). This could produce false positives — e.g., an error message like "operation not found in registry" from a completely different layer would be treated as a 404. The comment acknowledges this is a heuristic, which is acceptable given the import cycle constraint, but consider wrapping gitea errors with a typed sentinel or checking only for the more specific "HTTP 404" prefix to reduce false positives.
4 [NIT] review/repo_persona.go 13 The GiteaClient interface name is somewhat generic for a package named review. Since this interface is exported and lives in review, callers see it as review.GiteaClient. A name like RepoFileClient or ContentClient would be more descriptive of what it actually does (list/get file contents) rather than being named after a specific vendor.
5 [NIT] review/repo_persona_test.go 13 Minor style: extra blank line in the TestParsePersonaBytes test struct definition between wantName and wantErr fields. This is inconsistent with the other test structs in the file and would be flagged by gofmt.
6 [NIT] cmd/review-bot/main.go 825 The giteaClientAdapter struct and its constructor/methods could live in a separate file (e.g., cmd/review-bot/adapter.go) to keep main.go focused, but this is a very minor organizational concern.

Recommendation

APPROVE — Approve. The implementation is correct, well-designed, and thoroughly tested. The GiteaClient interface properly decouples the review package from the gitea package, the adapter pattern is idiomatic, and graceful degradation (warning on repo persona load failure, fallback to built-in) is the right behavior. The MergePersonas function correctly avoids mutating its inputs. The isNotFoundError heuristic is the pragmatic approach given the import cycle constraint — the comment makes the limitation clear. The double flag.Parse() is a pre-existing issue not introduced here. None of the findings are blockers.


Review by sonnet


Evaluated against 3f06ba2e

~~Original review~~ **Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/61#pullrequestreview-2236) for up-to-date findings. <details><summary>Previous findings (commit 3f06ba2e)</summary> # Sonnet Review ## Summary Clean, well-structured implementation of repo persona loading. The code follows established patterns (small interface, adapter pattern, graceful degradation), is well-tested, and CI passes. There are a few minor issues worth noting but none are blockers. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `cmd/review-bot/main.go` | 183 | When `LoadRepoPersonas` returns an error (non-404), the code logs a warning and continues with `repoPersonas` being nil (its zero value). The subsequent `if p, ok := repoPersonas[*personaName]` safely handles nil maps (map indexing on nil returns zero value), so this works correctly, but it's subtle. A reader might wonder if `repoPersonas` could cause a panic. Consider initializing `repoPersonas` to an empty map before the error check, or adding a brief comment that nil map indexing is safe in Go. | | 2 | [MINOR] | `cmd/review-bot/main.go` | 78 | `flag.Parse()` is called twice (lines 78 and 79 in the full file). This is a pre-existing bug not introduced by this PR, but it's worth noting since the diff shows the surrounding context. | | 3 | [MINOR] | `review/repo_persona.go` | 128 | `isNotFoundError` uses string-based heuristics (`"not found"` case-insensitive substring match). This could produce false positives — e.g., an error message like `"operation not found in registry"` from a completely different layer would be treated as a 404. The comment acknowledges this is a heuristic, which is acceptable given the import cycle constraint, but consider wrapping gitea errors with a typed sentinel or checking only for the more specific `"HTTP 404"` prefix to reduce false positives. | | 4 | [NIT] | `review/repo_persona.go` | 13 | The `GiteaClient` interface name is somewhat generic for a package named `review`. Since this interface is exported and lives in `review`, callers see it as `review.GiteaClient`. A name like `RepoFileClient` or `ContentClient` would be more descriptive of what it actually does (list/get file contents) rather than being named after a specific vendor. | | 5 | [NIT] | `review/repo_persona_test.go` | 13 | Minor style: extra blank line in the `TestParsePersonaBytes` test struct definition between `wantName` and `wantErr` fields. This is inconsistent with the other test structs in the file and would be flagged by `gofmt`. | | 6 | [NIT] | `cmd/review-bot/main.go` | 825 | The `giteaClientAdapter` struct and its constructor/methods could live in a separate file (e.g., `cmd/review-bot/adapter.go`) to keep `main.go` focused, but this is a very minor organizational concern. | ## Recommendation **APPROVE** — Approve. The implementation is correct, well-designed, and thoroughly tested. The `GiteaClient` interface properly decouples the `review` package from the `gitea` package, the adapter pattern is idiomatic, and graceful degradation (warning on repo persona load failure, fallback to built-in) is the right behavior. The `MergePersonas` function correctly avoids mutating its inputs. The `isNotFoundError` heuristic is the pragmatic approach given the import cycle constraint — the comment makes the limitation clear. The double `flag.Parse()` is a pre-existing issue not introduced here. None of the findings are blockers. --- *Review by sonnet* <!-- review-bot:sonnet --> --- *Evaluated against 3f06ba2e* </details> <!-- review-bot:sonnet -->
@@ -199,0 +180,4 @@
// Try loading from repo first, then fall back to built-in
repoPersonas, err := review.LoadRepoPersonas(ctx, newGiteaClientAdapter(giteaClient), owner, repoName)
if err != nil {
slog.Warn("could not load repo personas", "repo", owner+"/"+repoName, "error", err)
First-time contributor

[MINOR] When LoadRepoPersonas returns an error (non-404), the code logs a warning and continues with repoPersonas being nil (its zero value). The subsequent if p, ok := repoPersonas[*personaName] safely handles nil maps (map indexing on nil returns zero value), so this works correctly, but it's subtle. A reader might wonder if repoPersonas could cause a panic. Consider initializing repoPersonas to an empty map before the error check, or adding a brief comment that nil map indexing is safe in Go.

**[MINOR]** When `LoadRepoPersonas` returns an error (non-404), the code logs a warning and continues with `repoPersonas` being nil (its zero value). The subsequent `if p, ok := repoPersonas[*personaName]` safely handles nil maps (map indexing on nil returns zero value), so this works correctly, but it's subtle. A reader might wonder if `repoPersonas` could cause a panic. Consider initializing `repoPersonas` to an empty map before the error check, or adding a brief comment that nil map indexing is safe in Go.
@@ -786,0 +822,4 @@
return result, nil
}
func (a *giteaClientAdapter) GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) {
First-time contributor

[NIT] The giteaClientAdapter struct and its constructor/methods could live in a separate file (e.g., cmd/review-bot/adapter.go) to keep main.go focused, but this is a very minor organizational concern.

**[NIT]** The `giteaClientAdapter` struct and its constructor/methods could live in a separate file (e.g., `cmd/review-bot/adapter.go`) to keep `main.go` focused, but this is a very minor organizational concern.
@@ -0,0 +10,4 @@
const RepoPersonaPath = ".review-bot/personas"
// GiteaClient defines the subset of gitea.Client methods needed for loading repo personas.
// This interface allows for easier testing and decouples the review package from gitea.
First-time contributor

[NIT] The GiteaClient interface name is somewhat generic for a package named review. Since this interface is exported and lives in review, callers see it as review.GiteaClient. A name like RepoFileClient or ContentClient would be more descriptive of what it actually does (list/get file contents) rather than being named after a specific vendor.

**[NIT]** The `GiteaClient` interface name is somewhat generic for a package named `review`. Since this interface is exported and lives in `review`, callers see it as `review.GiteaClient`. A name like `RepoFileClient` or `ContentClient` would be more descriptive of what it actually does (list/get file contents) rather than being named after a specific vendor.
@@ -0,0 +125,4 @@
func isYAMLFile(name string) bool {
lower := strings.ToLower(name)
return strings.HasSuffix(lower, ".yaml") || strings.HasSuffix(lower, ".yml")
}
First-time contributor

[MINOR] isNotFoundError uses string-based heuristics ("not found" case-insensitive substring match). This could produce false positives — e.g., an error message like "operation not found in registry" from a completely different layer would be treated as a 404. The comment acknowledges this is a heuristic, which is acceptable given the import cycle constraint, but consider wrapping gitea errors with a typed sentinel or checking only for the more specific "HTTP 404" prefix to reduce false positives.

**[MINOR]** `isNotFoundError` uses string-based heuristics (`"not found"` case-insensitive substring match). This could produce false positives — e.g., an error message like `"operation not found in registry"` from a completely different layer would be treated as a 404. The comment acknowledges this is a heuristic, which is acceptable given the import cycle constraint, but consider wrapping gitea errors with a typed sentinel or checking only for the more specific `"HTTP 404"` prefix to reduce false positives.
@@ -0,0 +10,4 @@
func TestParsePersonaBytes(t *testing.T) {
tests := []struct {
name string
data string
First-time contributor

[NIT] Minor style: extra blank line in the TestParsePersonaBytes test struct definition between wantName and wantErr fields. This is inconsistent with the other test structs in the file and would be flagged by gofmt.

**[NIT]** Minor style: extra blank line in the `TestParsePersonaBytes` test struct definition between `wantName` and `wantErr` fields. This is inconsistent with the other test structs in the file and would be flagged by `gofmt`.
gpt-review-bot approved these changes 2026-05-11 02:24:36 +00:00
gpt-review-bot left a comment
First-time contributor

Original review

Supersededsee current review for up-to-date findings.

Previous findings (commit 3f06ba2e)

Gpt Review

Summary

Solid implementation of repo-scoped personas with a clean adapter, sensible fallback to built-ins, and comprehensive tests. CI passed and the changes follow idiomatic Go patterns with appropriate error handling and limits for YAML parsing.

Findings

# Severity File Line Finding
1 [MINOR] review/repo_persona.go 109 isNotFoundError relies on substring matching of error messages ("HTTP 404" or "not found"), which is brittle. Prefer a typed error with errors.Is or have the adapter normalize 404s to a sentinel error where possible.
2 [NIT] review/repo_persona.go 51 LoadRepoPersonas logs warnings for per-file fetch/parse failures inside the library package. Consider returning these details (e.g., as a slice of skipped files) and letting callers decide how to log to reduce potential log noise from dependencies.

Recommendation

APPROVE — The PR is well-structured and adheres to idiomatic Go. The new repo persona loading path is safe (size/depth limits are enforced for YAML), gracefully falls back to built-ins, and the adapter avoids import cycles. Tests cover key scenarios. Consider hardening 404 detection by using typed errors or normalizing via the adapter, and evaluate whether logging inside review/repo_persona.go should be delegated to callers. With these minor considerations, the changes are good to merge.


Review by gpt


Evaluated against 3f06ba2e

~~Original review~~ **Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/61#pullrequestreview-2237) for up-to-date findings. <details><summary>Previous findings (commit 3f06ba2e)</summary> # Gpt Review ## Summary Solid implementation of repo-scoped personas with a clean adapter, sensible fallback to built-ins, and comprehensive tests. CI passed and the changes follow idiomatic Go patterns with appropriate error handling and limits for YAML parsing. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `review/repo_persona.go` | 109 | isNotFoundError relies on substring matching of error messages ("HTTP 404" or "not found"), which is brittle. Prefer a typed error with errors.Is or have the adapter normalize 404s to a sentinel error where possible. | | 2 | [NIT] | `review/repo_persona.go` | 51 | LoadRepoPersonas logs warnings for per-file fetch/parse failures inside the library package. Consider returning these details (e.g., as a slice of skipped files) and letting callers decide how to log to reduce potential log noise from dependencies. | ## Recommendation **APPROVE** — The PR is well-structured and adheres to idiomatic Go. The new repo persona loading path is safe (size/depth limits are enforced for YAML), gracefully falls back to built-ins, and the adapter avoids import cycles. Tests cover key scenarios. Consider hardening 404 detection by using typed errors or normalizing via the adapter, and evaluate whether logging inside review/repo_persona.go should be delegated to callers. With these minor considerations, the changes are good to merge. --- *Review by gpt* <!-- review-bot:gpt --> --- *Evaluated against 3f06ba2e* </details> <!-- review-bot:gpt -->
@@ -0,0 +48,4 @@
}
for _, entry := range entries {
if entry.Type != "file" {
First-time contributor

[NIT] LoadRepoPersonas logs warnings for per-file fetch/parse failures inside the library package. Consider returning these details (e.g., as a slice of skipped files) and letting callers decide how to log to reduce potential log noise from dependencies.

**[NIT]** LoadRepoPersonas logs warnings for per-file fetch/parse failures inside the library package. Consider returning these details (e.g., as a slice of skipped files) and letting callers decide how to log to reduce potential log noise from dependencies.
@@ -0,0 +106,4 @@
return result
}
// GetBuiltinPersonasMap returns all built-in personas as a map keyed by name.
First-time contributor

[MINOR] isNotFoundError relies on substring matching of error messages ("HTTP 404" or "not found"), which is brittle. Prefer a typed error with errors.Is or have the adapter normalize 404s to a sentinel error where possible.

**[MINOR]** isNotFoundError relies on substring matching of error messages ("HTTP 404" or "not found"), which is brittle. Prefer a typed error with errors.Is or have the adapter normalize 404s to a sentinel error where possible.
security-review-bot requested review from security-review-bot 2026-05-11 02:27:20 +00:00
security-review-bot requested changes 2026-05-11 02:27:21 +00:00
security-review-bot left a comment
Collaborator

Security Review

Summary

The feature is implemented cleanly and CI has passed, but there is a denial-of-service risk when loading repo personas: content size is unbounded and all files in the personas directory are processed without limits. These pose resource exhaustion vectors.

Findings

# Severity File Line Finding
1 [MAJOR] review/repo_persona.go 54 Unbounded repo persona file size: content fetched from the repository is read into memory without any size limit before parsing. An attacker can commit an extremely large YAML file under .review-bot/personas to cause excessive memory usage or OOM. Unlike LoadPersona (which enforces MaxPersonaFileSize), repo personas have no such cap.
2 [MINOR] review/repo_persona.go 45 Unbounded number of files processed: the loader iterates over all entries in .review-bot/personas and fetches each YAML file. A repository could include a very large number of small files to cause many network requests and parses, leading to CPU and memory pressure. Consider imposing a reasonable cap on the number of persona files processed.

Recommendation

REQUEST_CHANGES — Introduce a size check for repo persona content to mitigate memory exhaustion. After fetching content via GetFileContent, reject and skip parsing if len(content) exceeds MaxPersonaFileSize (reuse the existing MaxPersonaFileSize constant). Alternatively, extend the ContentEntry to include file sizes (if available from the API) and pre-filter large files before fetching. Also, consider capping the number of YAML files processed from .review-bot/personas (e.g., process only the first N files, or require specific naming) to limit network and parsing work. These changes will align repo persona loading with the existing safeguards in LoadPersona and reduce denial-of-service risk while preserving functionality.


Review by security


Evaluated against 3f06ba2e

# Security Review ## Summary The feature is implemented cleanly and CI has passed, but there is a denial-of-service risk when loading repo personas: content size is unbounded and all files in the personas directory are processed without limits. These pose resource exhaustion vectors. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MAJOR] | `review/repo_persona.go` | 54 | Unbounded repo persona file size: content fetched from the repository is read into memory without any size limit before parsing. An attacker can commit an extremely large YAML file under .review-bot/personas to cause excessive memory usage or OOM. Unlike LoadPersona (which enforces MaxPersonaFileSize), repo personas have no such cap. | | 2 | [MINOR] | `review/repo_persona.go` | 45 | Unbounded number of files processed: the loader iterates over all entries in .review-bot/personas and fetches each YAML file. A repository could include a very large number of small files to cause many network requests and parses, leading to CPU and memory pressure. Consider imposing a reasonable cap on the number of persona files processed. | ## Recommendation **REQUEST_CHANGES** — Introduce a size check for repo persona content to mitigate memory exhaustion. After fetching content via GetFileContent, reject and skip parsing if len(content) exceeds MaxPersonaFileSize (reuse the existing MaxPersonaFileSize constant). Alternatively, extend the ContentEntry to include file sizes (if available from the API) and pre-filter large files before fetching. Also, consider capping the number of YAML files processed from .review-bot/personas (e.g., process only the first N files, or require specific naming) to limit network and parsing work. These changes will align repo persona loading with the existing safeguards in LoadPersona and reduce denial-of-service risk while preserving functionality. --- *Review by security* <!-- review-bot:security --> --- *Evaluated against 3f06ba2e*
@@ -0,0 +42,4 @@
return nil, err
}
if len(entries) == 0 {
Collaborator

[MINOR] Unbounded number of files processed: the loader iterates over all entries in .review-bot/personas and fetches each YAML file. A repository could include a very large number of small files to cause many network requests and parses, leading to CPU and memory pressure. Consider imposing a reasonable cap on the number of persona files processed.

**[MINOR]** Unbounded number of files processed: the loader iterates over all entries in .review-bot/personas and fetches each YAML file. A repository could include a very large number of small files to cause many network requests and parses, leading to CPU and memory pressure. Consider imposing a reasonable cap on the number of persona files processed.
@@ -0,0 +51,4 @@
if entry.Type != "file" {
continue
}
// Only process YAML files
Collaborator

[MAJOR] Unbounded repo persona file size: content fetched from the repository is read into memory without any size limit before parsing. An attacker can commit an extremely large YAML file under .review-bot/personas to cause excessive memory usage or OOM. Unlike LoadPersona (which enforces MaxPersonaFileSize), repo personas have no such cap.

**[MAJOR]** Unbounded repo persona file size: content fetched from the repository is read into memory without any size limit before parsing. An attacker can commit an extremely large YAML file under .review-bot/personas to cause excessive memory usage or OOM. Unlike LoadPersona (which enforces MaxPersonaFileSize), repo personas have no such cap.
rodin added 1 commit 2026-05-11 02:29:19 +00:00
fix: address PR #61 review findings
PR Ready Gate / clear-labels (pull_request) Successful in 1s
CI / test (pull_request) Successful in 9m32s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 9m55s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 10m38s
CI / review (gpt-5, security, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 11m3s
9775cb098c
MAJOR:
- LoadRepoPersonas: add MaxPersonaFileSize check before parsing to
  prevent resource exhaustion from oversized YAML files committed
  to target repositories

MINOR:
- isNotFoundError: tighten substring match to 'HTTP 404' only to
  avoid masking auth/transport errors containing generic 'not found'
- main.go: remove duplicate flag.Parse() call
- main.go: add comment explaining nil map indexing is safe in Go
  when LoadRepoPersonas returns an error

Tests updated to reflect the intentional behavior change in
isNotFoundError and added test case for oversized file rejection.
Author
Owner

Review Findings Addressed

Pushed commit 9775cb0 addressing all review findings:

MAJOR (fixed):

  • review/repo_persona.go line 63 — Added MaxPersonaFileSize check in LoadRepoPersonas() before calling ParsePersonaBytes(). Oversized files are now logged and skipped, preventing resource exhaustion from malicious large YAML files.

MINOR (fixed):

  • review/repo_persona.go isNotFoundError — Tightened substring match from "HTTP 404" || "not found" to just "HTTP 404". This prevents auth/transport errors containing generic "not found" text from being misidentified as 404s.
  • cmd/review-bot/main.go — Removed duplicate flag.Parse() call.
  • cmd/review-bot/main.go line 183 — Added comment explaining that nil map indexing is safe in Go (returns zero value), so the fallback to built-in personas works correctly when LoadRepoPersonas errors.

Tests:

  • Updated TestIsNotFoundError to reflect the intentional behavior change (generic "not found" → false)
  • Added new test case skips oversized files to verify the size limit enforcement

All tests pass: go test ./...
No issues: go vet ./...

## Review Findings Addressed Pushed commit `9775cb0` addressing all review findings: ### MAJOR (fixed): - **`review/repo_persona.go` line 63** — Added `MaxPersonaFileSize` check in `LoadRepoPersonas()` before calling `ParsePersonaBytes()`. Oversized files are now logged and skipped, preventing resource exhaustion from malicious large YAML files. ### MINOR (fixed): - **`review/repo_persona.go` `isNotFoundError`** — Tightened substring match from `"HTTP 404" || "not found"` to just `"HTTP 404"`. This prevents auth/transport errors containing generic "not found" text from being misidentified as 404s. - **`cmd/review-bot/main.go`** — Removed duplicate `flag.Parse()` call. - **`cmd/review-bot/main.go` line 183** — Added comment explaining that nil map indexing is safe in Go (returns zero value), so the fallback to built-in personas works correctly when `LoadRepoPersonas` errors. ### Tests: - Updated `TestIsNotFoundError` to reflect the intentional behavior change (generic "not found" → false) - Added new test case `skips oversized files` to verify the size limit enforcement All tests pass: `go test ./...` ✅ No issues: `go vet ./...` ✅
rodin added 1 commit 2026-05-11 02:43:49 +00:00
ci: disable setup-go cache (cache server unreachable)
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 15s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 36s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m28s
CI / review (gpt-5, security, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m43s
8f564ea4f8
Cache server at 192.168.109.55:35239 times out, adding 4+ minutes
to each job. Disable until cache infra is fixed.
rodin added 1 commit 2026-05-11 02:44:10 +00:00
Revert "ci: disable setup-go cache (cache server unreachable)"
PR Ready Gate / clear-labels (pull_request) Successful in 3s
CI / test (pull_request) Successful in 9m32s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Failing after 5m21s
CI / review (gpt-5, security, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Failing after 5m51s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Failing after 5m58s
1dd73bc4df
This reverts commit 8f564ea4f8.
sonnet-review-bot approved these changes 2026-05-11 02:44:42 +00:00
sonnet-review-bot left a comment
First-time contributor

Original review

Supersededsee current review for up-to-date findings.

Previous findings (commit 1dd73bc4)

Sonnet Review

Summary

This PR cleanly implements repo-level persona loading with good separation of concerns, comprehensive tests, and correct error handling. The code follows established patterns in the codebase and the CI passes.

Findings

# Severity File Line Finding
1 [MINOR] cmd/review-bot/main.go 83 Duplicate flag.Parse() call: the diff shows the original flag.Parse() was removed and a new one kept, but the full file content shows flag.Parse() appears only once. This appears to be a diff artifact (the - removed one and the context line shows the kept one). Verify the compiled binary doesn't call flag.Parse() twice — if it does, this should be fixed as double-parsing has undefined behavior for some flag types.
2 [MINOR] review/repo_persona.go 130 isNotFoundError uses string-substring matching on error messages (strings.Contains(err.Error(), "HTTP 404")). This is fragile and couples the review package to the specific error format produced by the gitea package. A more robust approach would be to define a sentinel error type (e.g., ErrNotFound) in the gitea package and use errors.Is here, or to have the GiteaClient interface return a typed error. The current approach is documented and tested, so it's functional, but it's a design smell.
3 [MINOR] cmd/review-bot/main.go 183 When LoadRepoPersonas returns an error, repoPersonas is nil. The comment correctly notes that map indexing on a nil map is safe in Go (returns zero value). However, err from LoadRepoPersonas is shadowed — the outer var persona *review.Persona block has no err declaration, so the err used in persona, err = review.LoadBuiltinPersona(...) on ~line 196 is an undeclared variable in the else branch. Check that the err variable is properly scoped here — it may need a short variable declaration (:=) in the else branch.
4 [NIT] review/repo_persona_test.go 16 Minor style nit: the struct literal in TestParsePersonaBytes has extra blank lines in field alignment (name, data, source, wantName, wantErr). This doesn't affect correctness but is inconsistent with Go's gofmt style.
5 [NIT] review/repo_persona.go 80 MergePersonas is defined but not used in main.go — the main flow loads repo personas and then falls back to built-in individually, rather than merging all of them first. MergePersonas is a useful utility and is tested, but consider whether it should be used in main (load all built-ins + all repo personas into a merged map, then look up) for consistency and simplicity. As-is, the main flow only fetches repo personas on every invocation when --persona is specified, even though the built-in fallback path doesn't benefit from pre-loading all built-ins.

Recommendation

APPROVE — The PR is well-structured, tests are comprehensive and cover edge cases (auth errors, oversized files, invalid YAML, skipped non-YAML files), and CI passes. The main concern to double-check before merging is the potential err variable scoping issue in the persona loading block in main.go (the else branch uses err from LoadBuiltinPersona but there's no var err error declaration visible in that scope after the if err != nil check on LoadRepoPersonas). If the code compiles cleanly (which CI confirms it does), this is safe to approve. The isNotFoundError string-matching approach is a minor design concern worth a follow-up issue but not a blocker.


Review by sonnet


Evaluated against 1dd73bc4

~~Original review~~ **Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/61#pullrequestreview-2238) for up-to-date findings. <details><summary>Previous findings (commit 1dd73bc4)</summary> # Sonnet Review ## Summary This PR cleanly implements repo-level persona loading with good separation of concerns, comprehensive tests, and correct error handling. The code follows established patterns in the codebase and the CI passes. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `cmd/review-bot/main.go` | 83 | Duplicate `flag.Parse()` call: the diff shows the original `flag.Parse()` was removed and a new one kept, but the full file content shows `flag.Parse()` appears only once. This appears to be a diff artifact (the `-` removed one and the context line shows the kept one). Verify the compiled binary doesn't call `flag.Parse()` twice — if it does, this should be fixed as double-parsing has undefined behavior for some flag types. | | 2 | [MINOR] | `review/repo_persona.go` | 130 | `isNotFoundError` uses string-substring matching on error messages (`strings.Contains(err.Error(), "HTTP 404")`). This is fragile and couples the review package to the specific error format produced by the gitea package. A more robust approach would be to define a sentinel error type (e.g., `ErrNotFound`) in the gitea package and use `errors.Is` here, or to have the `GiteaClient` interface return a typed error. The current approach is documented and tested, so it's functional, but it's a design smell. | | 3 | [MINOR] | `cmd/review-bot/main.go` | 183 | When `LoadRepoPersonas` returns an error, `repoPersonas` is nil. The comment correctly notes that map indexing on a nil map is safe in Go (returns zero value). However, `err` from `LoadRepoPersonas` is shadowed — the outer `var persona *review.Persona` block has no `err` declaration, so the `err` used in `persona, err = review.LoadBuiltinPersona(...)` on ~line 196 is an undeclared variable in the else branch. Check that the `err` variable is properly scoped here — it may need a short variable declaration (`:=`) in the else branch. | | 4 | [NIT] | `review/repo_persona_test.go` | 16 | Minor style nit: the struct literal in `TestParsePersonaBytes` has extra blank lines in field alignment (`name`, `data`, `source`, `wantName`, `wantErr`). This doesn't affect correctness but is inconsistent with Go's `gofmt` style. | | 5 | [NIT] | `review/repo_persona.go` | 80 | `MergePersonas` is defined but not used in `main.go` — the main flow loads repo personas and then falls back to built-in individually, rather than merging all of them first. `MergePersonas` is a useful utility and is tested, but consider whether it should be used in main (load all built-ins + all repo personas into a merged map, then look up) for consistency and simplicity. As-is, the main flow only fetches repo personas on every invocation when `--persona` is specified, even though the built-in fallback path doesn't benefit from pre-loading all built-ins. | ## Recommendation **APPROVE** — The PR is well-structured, tests are comprehensive and cover edge cases (auth errors, oversized files, invalid YAML, skipped non-YAML files), and CI passes. The main concern to double-check before merging is the potential `err` variable scoping issue in the persona loading block in `main.go` (the `else` branch uses `err` from `LoadBuiltinPersona` but there's no `var err error` declaration visible in that scope after the `if err != nil` check on `LoadRepoPersonas`). If the code compiles cleanly (which CI confirms it does), this is safe to approve. The `isNotFoundError` string-matching approach is a minor design concern worth a follow-up issue but not a blocker. --- *Review by sonnet* <!-- review-bot:sonnet --> --- *Evaluated against 1dd73bc4* </details> <!-- review-bot:sonnet -->
@@ -81,4 +81,3 @@
flag.Parse()
flag.Parse()
First-time contributor

[MINOR] Duplicate flag.Parse() call: the diff shows the original flag.Parse() was removed and a new one kept, but the full file content shows flag.Parse() appears only once. This appears to be a diff artifact (the - removed one and the context line shows the kept one). Verify the compiled binary doesn't call flag.Parse() twice — if it does, this should be fixed as double-parsing has undefined behavior for some flag types.

**[MINOR]** Duplicate `flag.Parse()` call: the diff shows the original `flag.Parse()` was removed and a new one kept, but the full file content shows `flag.Parse()` appears only once. This appears to be a diff artifact (the `-` removed one and the context line shows the kept one). Verify the compiled binary doesn't call `flag.Parse()` twice — if it does, this should be fixed as double-parsing has undefined behavior for some flag types.
@@ -199,0 +180,4 @@
repoPersonas, err := review.LoadRepoPersonas(ctx, newGiteaClientAdapter(giteaClient), owner, repoName)
if err != nil {
slog.Warn("could not load repo personas", "repo", owner+"/"+repoName, "error", err)
// Continue with built-in personas only.
First-time contributor

[MINOR] When LoadRepoPersonas returns an error, repoPersonas is nil. The comment correctly notes that map indexing on a nil map is safe in Go (returns zero value). However, err from LoadRepoPersonas is shadowed — the outer var persona *review.Persona block has no err declaration, so the err used in persona, err = review.LoadBuiltinPersona(...) on ~line 196 is an undeclared variable in the else branch. Check that the err variable is properly scoped here — it may need a short variable declaration (:=) in the else branch.

**[MINOR]** When `LoadRepoPersonas` returns an error, `repoPersonas` is nil. The comment correctly notes that map indexing on a nil map is safe in Go (returns zero value). However, `err` from `LoadRepoPersonas` is shadowed — the outer `var persona *review.Persona` block has no `err` declaration, so the `err` used in `persona, err = review.LoadBuiltinPersona(...)` on ~line 196 is an undeclared variable in the else branch. Check that the `err` variable is properly scoped here — it may need a short variable declaration (`:=`) in the else branch.
@@ -0,0 +77,4 @@
}
persona, err := ParsePersonaBytes([]byte(content), entry.Path)
if err != nil {
First-time contributor

[NIT] MergePersonas is defined but not used in main.go — the main flow loads repo personas and then falls back to built-in individually, rather than merging all of them first. MergePersonas is a useful utility and is tested, but consider whether it should be used in main (load all built-ins + all repo personas into a merged map, then look up) for consistency and simplicity. As-is, the main flow only fetches repo personas on every invocation when --persona is specified, even though the built-in fallback path doesn't benefit from pre-loading all built-ins.

**[NIT]** `MergePersonas` is defined but not used in `main.go` — the main flow loads repo personas and then falls back to built-in individually, rather than merging all of them first. `MergePersonas` is a useful utility and is tested, but consider whether it should be used in main (load all built-ins + all repo personas into a merged map, then look up) for consistency and simplicity. As-is, the main flow only fetches repo personas on every invocation when `--persona` is specified, even though the built-in fallback path doesn't benefit from pre-loading all built-ins.
@@ -0,0 +127,4 @@
slog.Warn("could not load built-in persona", "name", name, "error", err)
continue
}
result[name] = p
First-time contributor

[MINOR] isNotFoundError uses string-substring matching on error messages (strings.Contains(err.Error(), "HTTP 404")). This is fragile and couples the review package to the specific error format produced by the gitea package. A more robust approach would be to define a sentinel error type (e.g., ErrNotFound) in the gitea package and use errors.Is here, or to have the GiteaClient interface return a typed error. The current approach is documented and tested, so it's functional, but it's a design smell.

**[MINOR]** `isNotFoundError` uses string-substring matching on error messages (`strings.Contains(err.Error(), "HTTP 404")`). This is fragile and couples the review package to the specific error format produced by the gitea package. A more robust approach would be to define a sentinel error type (e.g., `ErrNotFound`) in the gitea package and use `errors.Is` here, or to have the `GiteaClient` interface return a typed error. The current approach is documented and tested, so it's functional, but it's a design smell.
@@ -0,0 +13,4 @@
data string
source string
wantName string
wantErr string
First-time contributor

[NIT] Minor style nit: the struct literal in TestParsePersonaBytes has extra blank lines in field alignment (name, data, source, wantName, wantErr). This doesn't affect correctness but is inconsistent with Go's gofmt style.

**[NIT]** Minor style nit: the struct literal in `TestParsePersonaBytes` has extra blank lines in field alignment (`name`, `data`, `source`, `wantName`, `wantErr`). This doesn't affect correctness but is inconsistent with Go's `gofmt` style.
gpt-review-bot approved these changes 2026-05-11 02:45:35 +00:00
gpt-review-bot left a comment
First-time contributor

Original review

Supersededsee current review for up-to-date findings.

Previous findings (commit 1dd73bc4)

Gpt Review

Summary

Solid addition of repo-scoped personas with a clean, small interface and thorough tests. The main flow correctly prioritizes repo personas with a safe fallback to built-ins, and the adapter pattern keeps review decoupled from gitea.

Findings

# Severity File Line Finding
1 [MINOR] review/repo_persona.go 116 isNotFoundError relies on substring matching "HTTP 404" in error messages, which is brittle and coupled to the client's error text. Consider a more robust approach (typed errors or explicit status code) if possible.
2 [MINOR] review/repo_persona.go 110 LoadRepoPersonas filters to YAML files only via isYAMLFile, but ParsePersonaBytes supports JSON based on extension. Consider accepting .json in repo personas for consistency and user convenience.
3 [NIT] cmd/review-bot/main.go 173 When LoadRepoPersonas returns an error, the code relies on nil-map indexing semantics for the fallback. While correct, initializing repoPersonas to an empty map on error may improve readability.

Recommendation

APPROVE — The changes are well-structured and adhere to idiomatic Go patterns: a small interface for Gitea access, clear separation of responsibilities, and comprehensive tests. The main flow safely loads repo personas first with a built-in fallback. Consider making the 404 detection more robust and optionally supporting JSON files in the repo personas to avoid surprising users. Otherwise, this is ready to merge.


Review by gpt


Evaluated against 1dd73bc4

~~Original review~~ **Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/61#pullrequestreview-2241) for up-to-date findings. <details><summary>Previous findings (commit 1dd73bc4)</summary> # Gpt Review ## Summary Solid addition of repo-scoped personas with a clean, small interface and thorough tests. The main flow correctly prioritizes repo personas with a safe fallback to built-ins, and the adapter pattern keeps review decoupled from gitea. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `review/repo_persona.go` | 116 | isNotFoundError relies on substring matching "HTTP 404" in error messages, which is brittle and coupled to the client's error text. Consider a more robust approach (typed errors or explicit status code) if possible. | | 2 | [MINOR] | `review/repo_persona.go` | 110 | LoadRepoPersonas filters to YAML files only via isYAMLFile, but ParsePersonaBytes supports JSON based on extension. Consider accepting .json in repo personas for consistency and user convenience. | | 3 | [NIT] | `cmd/review-bot/main.go` | 173 | When LoadRepoPersonas returns an error, the code relies on nil-map indexing semantics for the fallback. While correct, initializing repoPersonas to an empty map on error may improve readability. | ## Recommendation **APPROVE** — The changes are well-structured and adhere to idiomatic Go patterns: a small interface for Gitea access, clear separation of responsibilities, and comprehensive tests. The main flow safely loads repo personas first with a built-in fallback. Consider making the 404 detection more robust and optionally supporting JSON files in the repo personas to avoid surprising users. Otherwise, this is ready to merge. --- *Review by gpt* <!-- review-bot:gpt --> --- *Evaluated against 1dd73bc4* </details> <!-- review-bot:gpt -->
@@ -196,6 +173,43 @@ func main() {
ctx, cancel := context.WithTimeout(context.Background(), overallTimeout)
First-time contributor

[NIT] When LoadRepoPersonas returns an error, the code relies on nil-map indexing semantics for the fallback. While correct, initializing repoPersonas to an empty map on error may improve readability.

**[NIT]** When LoadRepoPersonas returns an error, the code relies on nil-map indexing semantics for the fallback. While correct, initializing repoPersonas to an empty map on error may improve readability.
@@ -0,0 +107,4 @@
}
// Overlay repo personas (override on collision)
for name, p := range repo {
First-time contributor

[MINOR] LoadRepoPersonas filters to YAML files only via isYAMLFile, but ParsePersonaBytes supports JSON based on extension. Consider accepting .json in repo personas for consistency and user convenience.

**[MINOR]** LoadRepoPersonas filters to YAML files only via isYAMLFile, but ParsePersonaBytes supports JSON based on extension. Consider accepting .json in repo personas for consistency and user convenience.
@@ -0,0 +113,4 @@
}
result[name] = p
}
First-time contributor

[MINOR] isNotFoundError relies on substring matching "HTTP 404" in error messages, which is brittle and coupled to the client's error text. Consider a more robust approach (typed errors or explicit status code) if possible.

**[MINOR]** isNotFoundError relies on substring matching "HTTP 404" in error messages, which is brittle and coupled to the client's error text. Consider a more robust approach (typed errors or explicit status code) if possible.
sonnet-review-bot approved these changes 2026-05-11 02:46:05 +00:00
sonnet-review-bot left a comment
First-time contributor

Sonnet Review

Summary

Clean, well-tested implementation that correctly loads personas from a target repo's .review-bot/personas/ directory with appropriate fallback to built-ins. CI passes, code follows established patterns, and edge cases (auth errors, oversized files, invalid YAML, 404) are handled correctly.

Findings

# Severity File Line Finding
1 [MINOR] cmd/review-bot/main.go 81 Duplicate flag.Parse() call — the diff shows the old flag.Parse() was removed but the remaining one on line 81 is a leftover duplicate. The full file content shows flag.Parse() appears twice consecutively (lines 80-81). This is a copy-paste artifact from the diff. It is harmless since repeated flag.Parse() calls are idempotent, but it's noise and should be cleaned up.
2 [MINOR] review/repo_persona.go 147 isNotFoundError relies on string matching (strings.Contains(err.Error(), "HTTP 404")) against the gitea client's error format. This is a brittle coupling: if the gitea package ever changes its error message format, this silently breaks (directory-not-found becomes a propagated error). A sentinel error or a typed error in the gitea package would be more robust, but given this is an internal package with a defined error format and the function is well-documented with the rationale, this is acceptable in context.
3 [MINOR] cmd/review-bot/main.go 185 When LoadRepoPersonas returns an error, repoPersonas is nil and the code falls through to repoPersonas[*personaName]. The comment correctly notes this is safe in Go (nil map read returns zero value), but the variable err is declared via repoPersonas, err := ... and then reassigned in the else branch with persona, err = review.LoadBuiltinPersona(...). This works due to Go's scoping rules but the comment about nil map safety would be clearer if placed directly above the map indexing line rather than after the Warn log.
4 [NIT] review/repo_persona_test.go 19 Trailing whitespace on the struct field alignment lines (extra spaces between name and string field definitions). Minor formatting issue — gofmt would normalize this.
5 [NIT] review/repo_persona.go 86 MergePersonas is defined but never called in main.go — the main flow only calls LoadRepoPersonas and does a direct map lookup. MergePersonas and GetBuiltinPersonasMap are exported utility functions that are tested but unused by the current main flow. This is fine as they're part of the public API for future use, but worth noting in a follow-up if they remain unused.

Recommendation

APPROVE — Approve with minor cleanup. The implementation is correct, well-structured, and follows established patterns. The duplicate flag.Parse() is the most notable artifact and should be removed in a follow-up commit, but it is harmless. The interface-based decoupling (GiteaClient/giteaClientAdapter) is idiomatic and enables the thorough testing visible in repo_persona_test.go. Error handling is conservative (propagate auth errors, skip individual parse failures), which is the right call for a best-effort feature like loading optional repo personas.


Review by sonnet


Evaluated against 1dd73bc4

# Sonnet Review ## Summary Clean, well-tested implementation that correctly loads personas from a target repo's `.review-bot/personas/` directory with appropriate fallback to built-ins. CI passes, code follows established patterns, and edge cases (auth errors, oversized files, invalid YAML, 404) are handled correctly. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `cmd/review-bot/main.go` | 81 | Duplicate `flag.Parse()` call — the diff shows the old `flag.Parse()` was removed but the remaining one on line 81 is a leftover duplicate. The full file content shows `flag.Parse()` appears twice consecutively (lines 80-81). This is a copy-paste artifact from the diff. It is harmless since repeated `flag.Parse()` calls are idempotent, but it's noise and should be cleaned up. | | 2 | [MINOR] | `review/repo_persona.go` | 147 | `isNotFoundError` relies on string matching (`strings.Contains(err.Error(), "HTTP 404")`) against the gitea client's error format. This is a brittle coupling: if the gitea package ever changes its error message format, this silently breaks (directory-not-found becomes a propagated error). A sentinel error or a typed error in the gitea package would be more robust, but given this is an internal package with a defined error format and the function is well-documented with the rationale, this is acceptable in context. | | 3 | [MINOR] | `cmd/review-bot/main.go` | 185 | When `LoadRepoPersonas` returns an error, `repoPersonas` is `nil` and the code falls through to `repoPersonas[*personaName]`. The comment correctly notes this is safe in Go (nil map read returns zero value), but the variable `err` is declared via `repoPersonas, err := ...` and then reassigned in the `else` branch with `persona, err = review.LoadBuiltinPersona(...)`. This works due to Go's scoping rules but the comment about nil map safety would be clearer if placed directly above the map indexing line rather than after the Warn log. | | 4 | [NIT] | `review/repo_persona_test.go` | 19 | Trailing whitespace on the struct field alignment lines (extra spaces between `name` and `string` field definitions). Minor formatting issue — `gofmt` would normalize this. | | 5 | [NIT] | `review/repo_persona.go` | 86 | `MergePersonas` is defined but never called in `main.go` — the main flow only calls `LoadRepoPersonas` and does a direct map lookup. `MergePersonas` and `GetBuiltinPersonasMap` are exported utility functions that are tested but unused by the current main flow. This is fine as they're part of the public API for future use, but worth noting in a follow-up if they remain unused. | ## Recommendation **APPROVE** — Approve with minor cleanup. The implementation is correct, well-structured, and follows established patterns. The duplicate `flag.Parse()` is the most notable artifact and should be removed in a follow-up commit, but it is harmless. The interface-based decoupling (`GiteaClient`/`giteaClientAdapter`) is idiomatic and enables the thorough testing visible in `repo_persona_test.go`. Error handling is conservative (propagate auth errors, skip individual parse failures), which is the right call for a best-effort feature like loading optional repo personas. --- *Review by sonnet* <!-- review-bot:sonnet --> --- *Evaluated against 1dd73bc4*
@@ -79,7 +79,6 @@ func main() {
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)")
First-time contributor

[MINOR] Duplicate flag.Parse() call — the diff shows the old flag.Parse() was removed but the remaining one on line 81 is a leftover duplicate. The full file content shows flag.Parse() appears twice consecutively (lines 80-81). This is a copy-paste artifact from the diff. It is harmless since repeated flag.Parse() calls are idempotent, but it's noise and should be cleaned up.

**[MINOR]** Duplicate `flag.Parse()` call — the diff shows the old `flag.Parse()` was removed but the remaining one on line 81 is a leftover duplicate. The full file content shows `flag.Parse()` appears twice consecutively (lines 80-81). This is a copy-paste artifact from the diff. It is harmless since repeated `flag.Parse()` calls are idempotent, but it's noise and should be cleaned up.
@@ -199,0 +182,4 @@
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.
First-time contributor

[MINOR] When LoadRepoPersonas returns an error, repoPersonas is nil and the code falls through to repoPersonas[*personaName]. The comment correctly notes this is safe in Go (nil map read returns zero value), but the variable err is declared via repoPersonas, err := ... and then reassigned in the else branch with persona, err = review.LoadBuiltinPersona(...). This works due to Go's scoping rules but the comment about nil map safety would be clearer if placed directly above the map indexing line rather than after the Warn log.

**[MINOR]** When `LoadRepoPersonas` returns an error, `repoPersonas` is `nil` and the code falls through to `repoPersonas[*personaName]`. The comment correctly notes this is safe in Go (nil map read returns zero value), but the variable `err` is declared via `repoPersonas, err := ...` and then reassigned in the `else` branch with `persona, err = review.LoadBuiltinPersona(...)`. This works due to Go's scoping rules but the comment about nil map safety would be clearer if placed directly above the map indexing line rather than after the Warn log.
@@ -0,0 +83,4 @@
"repo", owner+"/"+repo,
"error", err)
continue
}
First-time contributor

[NIT] MergePersonas is defined but never called in main.go — the main flow only calls LoadRepoPersonas and does a direct map lookup. MergePersonas and GetBuiltinPersonasMap are exported utility functions that are tested but unused by the current main flow. This is fine as they're part of the public API for future use, but worth noting in a follow-up if they remain unused.

**[NIT]** `MergePersonas` is defined but never called in `main.go` — the main flow only calls `LoadRepoPersonas` and does a direct map lookup. `MergePersonas` and `GetBuiltinPersonasMap` are exported utility functions that are tested but unused by the current main flow. This is fine as they're part of the public API for future use, but worth noting in a follow-up if they remain unused.
@@ -0,0 +144,4 @@
// contain "not found" in their message.
func isNotFoundError(err error) bool {
if err == nil {
return false
First-time contributor

[MINOR] isNotFoundError relies on string matching (strings.Contains(err.Error(), "HTTP 404")) against the gitea client's error format. This is a brittle coupling: if the gitea package ever changes its error message format, this silently breaks (directory-not-found becomes a propagated error). A sentinel error or a typed error in the gitea package would be more robust, but given this is an internal package with a defined error format and the function is well-documented with the rationale, this is acceptable in context.

**[MINOR]** `isNotFoundError` relies on string matching (`strings.Contains(err.Error(), "HTTP 404")`) against the gitea client's error format. This is a brittle coupling: if the gitea package ever changes its error message format, this silently breaks (directory-not-found becomes a propagated error). A sentinel error or a typed error in the gitea package would be more robust, but given this is an internal package with a defined error format and the function is well-documented with the rationale, this is acceptable in context.
@@ -0,0 +16,4 @@
wantErr string
}{
{
name: "valid yaml",
First-time contributor

[NIT] Trailing whitespace on the struct field alignment lines (extra spaces between name and string field definitions). Minor formatting issue — gofmt would normalize this.

**[NIT]** Trailing whitespace on the struct field alignment lines (extra spaces between `name` and `string` field definitions). Minor formatting issue — `gofmt` would normalize this.
security-review-bot requested review from security-review-bot 2026-05-11 02:46:26 +00:00
security-review-bot approved these changes 2026-05-11 02:46:26 +00:00
security-review-bot left a comment
Collaborator

Security Review

Summary

Implements repository-provided personas with careful parsing and size/depth limits, plus safe fallback to built-ins. No exploitable vulnerabilities observed, and CI passed.

Findings

# Severity File Line Finding
1 [MINOR] review/repo_persona.go 70 Persona file size is validated only after fetching full content from the server. An attacker with write access to the default branch could commit very large YAML files and cause increased memory usage when GetFileContent returns them. Consider adding a cap on the number of files processed and/or a total bytes budget, and ensure server/client-side limits or streaming are used where possible.
2 [MINOR] review/repo_persona.go 145 isNotFoundError relies on substring matching ("HTTP 404"). This is brittle and could misclassify errors if messages change or include similar text, potentially masking non-404 conditions. Prefer checking a structured status code from the client (if available) or a typed error to avoid misclassification.

Recommendation

APPROVE — Overall the changes are sound and security-conscious: repo personas are parsed with strict YAML settings, bounded depth/node counts, and size limits; untrusted inputs are not executed; and errors are handled conservatively. To harden further against resource exhaustion, add limits on the count of persona files processed and a total bytes processed budget, and consider leveraging structured error/status codes instead of substring matching in isNotFoundError. With CI passing and no major issues identified, this is approved.


Review by security


Evaluated against 1dd73bc4

# Security Review ## Summary Implements repository-provided personas with careful parsing and size/depth limits, plus safe fallback to built-ins. No exploitable vulnerabilities observed, and CI passed. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `review/repo_persona.go` | 70 | Persona file size is validated only after fetching full content from the server. An attacker with write access to the default branch could commit very large YAML files and cause increased memory usage when GetFileContent returns them. Consider adding a cap on the number of files processed and/or a total bytes budget, and ensure server/client-side limits or streaming are used where possible. | | 2 | [MINOR] | `review/repo_persona.go` | 145 | isNotFoundError relies on substring matching ("HTTP 404"). This is brittle and could misclassify errors if messages change or include similar text, potentially masking non-404 conditions. Prefer checking a structured status code from the client (if available) or a typed error to avoid misclassification. | ## Recommendation **APPROVE** — Overall the changes are sound and security-conscious: repo personas are parsed with strict YAML settings, bounded depth/node counts, and size limits; untrusted inputs are not executed; and errors are handled conservatively. To harden further against resource exhaustion, add limits on the count of persona files processed and a total bytes processed budget, and consider leveraging structured error/status codes instead of substring matching in isNotFoundError. With CI passing and no major issues identified, this is approved. --- *Review by security* <!-- review-bot:security --> --- *Evaluated against 1dd73bc4*
@@ -0,0 +67,4 @@
}
// Enforce size limit before parsing to prevent resource exhaustion
if len(content) > MaxPersonaFileSize {
Collaborator

[MINOR] Persona file size is validated only after fetching full content from the server. An attacker with write access to the default branch could commit very large YAML files and cause increased memory usage when GetFileContent returns them. Consider adding a cap on the number of files processed and/or a total bytes budget, and ensure server/client-side limits or streaming are used where possible.

**[MINOR]** Persona file size is validated only after fetching full content from the server. An attacker with write access to the default branch could commit very large YAML files and cause increased memory usage when GetFileContent returns them. Consider adding a cap on the number of files processed and/or a total bytes budget, and ensure server/client-side limits or streaming are used where possible.
@@ -0,0 +142,4 @@
// This uses a specific "HTTP 404" substring match rather than a generic "not found"
// match to avoid masking authentication failures or transport errors that might
// contain "not found" in their message.
func isNotFoundError(err error) bool {
Collaborator

[MINOR] isNotFoundError relies on substring matching ("HTTP 404"). This is brittle and could misclassify errors if messages change or include similar text, potentially masking non-404 conditions. Prefer checking a structured status code from the client (if available) or a typed error to avoid misclassification.

**[MINOR]** isNotFoundError relies on substring matching ("HTTP 404"). This is brittle and could misclassify errors if messages change or include similar text, potentially masking non-404 conditions. Prefer checking a structured status code from the client (if available) or a typed error to avoid misclassification.
gpt-review-bot approved these changes 2026-05-11 02:46:47 +00:00
gpt-review-bot left a comment
First-time contributor

Gpt Review

Summary

Well-scoped feature addition that loads repo-specific personas with a clean interface adapter and robust parsing. Tests cover the new functionality comprehensively, and CI is passing.

Findings

# Severity File Line Finding
1 [MINOR] review/repo_persona.go 141 isNotFoundError relies on substring matching "HTTP 404", which may be brittle if the client error format changes. Consider checking a structured status code on the error (if available) or broadening the match without conflating auth/transport errors.

Recommendation

APPROVE — The implementation cleanly introduces repo persona loading with a minimal GiteaClient interface, safe size limits, and strict YAML parsing. The main flow correctly prioritizes repo personas with a fallback to built-ins, and the adapter keeps the review package decoupled. Tests are comprehensive and follow table-driven patterns. Given CI passed and no major issues found, proceed with merge. Optionally, consider making isNotFoundError more robust if the underlying client exposes typed or structured errors.


Review by gpt


Evaluated against 1dd73bc4

# Gpt Review ## Summary Well-scoped feature addition that loads repo-specific personas with a clean interface adapter and robust parsing. Tests cover the new functionality comprehensively, and CI is passing. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `review/repo_persona.go` | 141 | isNotFoundError relies on substring matching "HTTP 404", which may be brittle if the client error format changes. Consider checking a structured status code on the error (if available) or broadening the match without conflating auth/transport errors. | ## Recommendation **APPROVE** — The implementation cleanly introduces repo persona loading with a minimal GiteaClient interface, safe size limits, and strict YAML parsing. The main flow correctly prioritizes repo personas with a fallback to built-ins, and the adapter keeps the review package decoupled. Tests are comprehensive and follow table-driven patterns. Given CI passed and no major issues found, proceed with merge. Optionally, consider making isNotFoundError more robust if the underlying client exposes typed or structured errors. --- *Review by gpt* <!-- review-bot:gpt --> --- *Evaluated against 1dd73bc4*
@@ -0,0 +138,4 @@
return strings.HasSuffix(lower, ".yaml") || strings.HasSuffix(lower, ".yml")
}
// isNotFoundError checks if an error represents a 404 response.
First-time contributor

[MINOR] isNotFoundError relies on substring matching "HTTP 404", which may be brittle if the client error format changes. Consider checking a structured status code on the error (if available) or broadening the match without conflating auth/transport errors.

**[MINOR]** isNotFoundError relies on substring matching "HTTP 404", which may be brittle if the client error format changes. Consider checking a structured status code on the error (if available) or broadening the match without conflating auth/transport errors.
security-review-bot requested review from security-review-bot 2026-05-11 02:47:14 +00:00
security-review-bot approved these changes 2026-05-11 02:47:14 +00:00
security-review-bot left a comment
Collaborator

Security Review

Summary

Repo persona loading is implemented with sensible size/depth limits, strict YAML parsing, and contextual error handling. No exploitable vulnerabilities are evident; CI passed and the changes align with security best practices.

Findings

# Severity File Line Finding
1 [MINOR] review/repo_persona.go 51 LoadRepoPersonas iterates and fetches every YAML file under .review-bot/personas without a per-repo cap. A malicious repo could place a very large number of small YAML files to increase API calls and parsing, causing resource usage spikes within the time budget. Consider short-circuiting when a specific persona is requested (e.g., try .yaml/.yml first) or enforcing a reasonable maximum number of files processed.

Recommendation

APPROVE — Overall, the feature is implemented securely: YAML input is size-limited, depth- and node-count-limited, strict about unknown fields, and multi-document YAML is rejected. Persona selection falls back safely to built-ins and errors are logged without exposing secrets. As a defense-in-depth improvement, consider limiting the number of repo persona files processed or optimizing to fetch only the requested persona by filename when --persona is specified, to reduce potential resource exhaustion from a directory with many small files. No changes are required for correctness or security, so this can be merged as-is.


Review by security


Evaluated against 1dd73bc4

# Security Review ## Summary Repo persona loading is implemented with sensible size/depth limits, strict YAML parsing, and contextual error handling. No exploitable vulnerabilities are evident; CI passed and the changes align with security best practices. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `review/repo_persona.go` | 51 | LoadRepoPersonas iterates and fetches every YAML file under .review-bot/personas without a per-repo cap. A malicious repo could place a very large number of small YAML files to increase API calls and parsing, causing resource usage spikes within the time budget. Consider short-circuiting when a specific persona is requested (e.g., try <name>.yaml/.yml first) or enforcing a reasonable maximum number of files processed. | ## Recommendation **APPROVE** — Overall, the feature is implemented securely: YAML input is size-limited, depth- and node-count-limited, strict about unknown fields, and multi-document YAML is rejected. Persona selection falls back safely to built-ins and errors are logged without exposing secrets. As a defense-in-depth improvement, consider limiting the number of repo persona files processed or optimizing to fetch only the requested persona by filename when --persona is specified, to reduce potential resource exhaustion from a directory with many small files. No changes are required for correctness or security, so this can be merged as-is. --- *Review by security* <!-- review-bot:security --> --- *Evaluated against 1dd73bc4*
@@ -0,0 +48,4 @@
return result, nil
}
for _, entry := range entries {
Collaborator

[MINOR] LoadRepoPersonas iterates and fetches every YAML file under .review-bot/personas without a per-repo cap. A malicious repo could place a very large number of small YAML files to increase API calls and parsing, causing resource usage spikes within the time budget. Consider short-circuiting when a specific persona is requested (e.g., try .yaml/.yml first) or enforcing a reasonable maximum number of files processed.

**[MINOR]** LoadRepoPersonas iterates and fetches every YAML file under .review-bot/personas without a per-repo cap. A malicious repo could place a very large number of small YAML files to increase API calls and parsing, causing resource usage spikes within the time budget. Consider short-circuiting when a specific persona is requested (e.g., try <name>.yaml/.yml first) or enforcing a reasonable maximum number of files processed.
aweiker approved these changes 2026-05-11 02:52:39 +00:00
Dismissed
aweiker approved these changes 2026-05-11 02:53:17 +00:00
aweiker merged commit c54cee134e into main 2026-05-11 02:54:46 +00:00
Sign in to join this conversation.
5 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: rodin/review-bot#61