diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index 461fbdb..22a2728 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -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)") - flag.Parse() flag.Parse() if *versionFlag { @@ -119,7 +118,6 @@ func main() { // Persona loading is deferred until after giteaClient is initialized, // so we can try loading from the target repo first. var persona *review.Persona - var personaErr error // Validate reviewer-name: only safe characters allowed in sentinel if err := validateReviewerName(*reviewerName); err != nil { @@ -184,6 +182,8 @@ func main() { remotePersonas, err := review.LoadRemotePersonas(ctx, fetcher, owner, repoName) if err != nil { slog.Warn("could not load remote personas", "repo", fmt.Sprintf("%s/%s", owner, repoName), "error", err) + // Assign empty map so the lookup below doesn't panic + remotePersonas = map[string]*review.Persona{} } if p, ok := remotePersonas[*personaName]; ok { @@ -191,9 +191,10 @@ func main() { slog.Info("loaded persona from target repo", "persona", persona.Name, "display", persona.DisplayName) } else { // Fall back to built-in persona - persona, personaErr = review.LoadBuiltinPersona(*personaName) - if personaErr != nil { - slog.Error("failed to load persona", "persona", *personaName, "error", personaErr) + var err error + persona, err = review.LoadBuiltinPersona(*personaName) + if err != nil { + slog.Error("failed to load persona", "persona", *personaName, "error", err) os.Exit(1) } slog.Info("loaded built-in persona", "persona", persona.Name, "display", persona.DisplayName) @@ -204,9 +205,10 @@ func main() { slog.Error("invalid persona-file path", "error", err) os.Exit(1) } - persona, personaErr = review.LoadPersona(resolvedPath) - if personaErr != nil { - slog.Error("failed to load persona file", "file", *personaFile, "error", personaErr) + var err2 error + persona, err2 = review.LoadPersona(resolvedPath) + if err2 != nil { + slog.Error("failed to load persona file", "file", *personaFile, "error", err2) os.Exit(1) } slog.Info("loaded persona from file", "file", *personaFile, "persona", persona.Name) diff --git a/review/remote_persona.go b/review/remote_persona.go index f24eccb..1b9c200 100644 --- a/review/remote_persona.go +++ b/review/remote_persona.go @@ -40,7 +40,9 @@ func LoadRemotePersonas(ctx context.Context, fetcher PersonaFetcher, owner, repo return LoadRemotePersonasFromPath(ctx, fetcher, owner, repo, DefaultPersonasPath) } -// LoadRemotePersonasFromPath is like LoadRemotePersonas but allows specifying a custom path. +// LoadRemotePersonasFromPath loads personas from a custom path in a remote repository. +// It behaves the same as LoadRemotePersonas but allows specifying a path other than +// the default .review-bot/personas directory. func LoadRemotePersonasFromPath(ctx context.Context, fetcher PersonaFetcher, owner, repo, path string) (map[string]*Persona, error) { entries, err := fetcher.ListContents(ctx, owner, repo, path) if err != nil { @@ -52,8 +54,17 @@ func LoadRemotePersonasFromPath(ctx context.Context, fetcher PersonaFetcher, own return nil, fmt.Errorf("list remote personas: %w", err) } + // Cap the number of files to process to prevent resource exhaustion + // from repos with thousands of small files. + const maxPersonaFiles = 50 + result := make(map[string]*Persona) + processed := 0 for _, entry := range entries { + if processed >= maxPersonaFiles { + slog.Warn("persona file limit reached", "limit", maxPersonaFiles, "repo", fmt.Sprintf("%s/%s", owner, repo)) + break + } if ctx.Err() != nil { return nil, ctx.Err() } @@ -85,6 +96,7 @@ func LoadRemotePersonasFromPath(ctx context.Context, fetcher PersonaFetcher, own } result[persona.Name] = persona + processed++ slog.Debug("loaded remote persona", "name", persona.Name, "file", entry.Path) } @@ -148,5 +160,5 @@ func isNotFoundError(err error) bool { return false } errStr := err.Error() - return strings.Contains(errStr, "HTTP 404") || strings.Contains(errStr, "not found") + return strings.Contains(errStr, "HTTP 404") } diff --git a/review/remote_persona_test.go b/review/remote_persona_test.go index 910f539..a7292be 100644 --- a/review/remote_persona_test.go +++ b/review/remote_persona_test.go @@ -379,7 +379,7 @@ func TestIsNotFoundError(t *testing.T) { }{ {"nil error", nil, false}, {"HTTP 404", errors.New("HTTP 404: not found"), true}, - {"not found text", errors.New("path not found"), true}, + {"not found text", errors.New("path not found"), false}, {"server error", errors.New("server error"), false}, {"HTTP 500", errors.New("HTTP 500: internal error"), false}, }