From 85c0e735dc0e12fcd3bc6b06e7fc51c36d2024d4 Mon Sep 17 00:00:00 2001 From: Space-Banane Date: Sun, 12 Jul 2026 22:20:48 +0200 Subject: [PATCH] fix. harden Go review flow Fix runner bootstrap and auth handling, preserve queued SHAs, make event/job acceptance atomic, fence stale runs, correct retries and prompts, add fake end-to-end coverage, and fix deployment defaults. Co-Authored-By: Claude --- .dockerignore | 9 ++ .env.example | 5 +- README.md | 4 +- cmd/gitea-codex/main.go | 34 ++++-- docker-compose.dev.yml | 15 +-- docker-compose.yml | 15 +-- docs/go-rebuild.md | 2 +- internal/gitea/client.go | 9 +- internal/httpapi/httpapi.go | 78 ++++++------ internal/review/review.go | 89 ++++++++++---- internal/review/review_test.go | 18 +++ internal/runner/runner.go | 35 ++++-- internal/runner/runner_test.go | 6 +- internal/store/sqlstore/sqlstore.go | 99 ++++++++++++++-- internal/store/store.go | 1 + internal/worker/e2e_test.go | 144 +++++++++++++++++++++++ internal/worker/worker.go | 55 ++++++--- migrations/0001_initial.sql | 60 +++++++++- migrations/0002_trigger_comment_body.sql | 3 +- 19 files changed, 539 insertions(+), 142 deletions(-) create mode 100644 .dockerignore create mode 100644 internal/worker/e2e_test.go diff --git a/.dockerignore b/.dockerignore new file mode 100644 index 0000000..deb5c26 --- /dev/null +++ b/.dockerignore @@ -0,0 +1,9 @@ +.git +.tmp +.db +worktrees +db +*.db +.env +__pycache__ +.pytest_cache diff --git a/.env.example b/.env.example index 55986c5..1820728 100644 --- a/.env.example +++ b/.env.example @@ -41,6 +41,7 @@ DB_PORT=3306 DB_NAME=gitea_codex DB_USER=gitea_codex DB_PASSWORD=replace +MARIADB_ROOT_PASSWORD=replace-with-a-different-root-password WORKDIR=/var/lib/gitea-codex/worktrees MAX_DIFF_BYTES=200000 @@ -53,7 +54,7 @@ REVIEW_RUNNER_IMAGE=node:22-bookworm-slim # Security: fork PRs are skipped unless explicitly enabled. ALLOW_UNTRUSTED_FORKS=false -# Optional SQLite/MariaDB override. When unset, DB_* values compose a MariaDB DSN. -DATABASE_URL=sqlite://./gitea-codex.db +# Optional database override. Leave blank for the MariaDB DSN composed from DB_*. +DATABASE_URL= WEBHOOK_MAX_BYTES=2097152 PORT=8000 diff --git a/README.md b/README.md index ced6101..2c72630 100644 --- a/README.md +++ b/README.md @@ -88,7 +88,7 @@ cp .env.example .env docker compose -f docker-compose.dev.yml up --build ``` -The bot container needs access to the host Docker API to launch isolated review containers. Mounting `/var/run/docker.sock` is a privileged deployment decision; use a dedicated runner service or hardened Docker host where possible. +The bot container needs access to the host Docker API to launch isolated review containers. Mounting `/var/run/docker.sock` is a privileged deployment decision; use a dedicated runner service or hardened Docker host where possible. `docker-compose.yml` consumes the published image, while `docker-compose.dev.yml` builds locally. ChatGPT auth mode should use a private Compose override to mount `auth.json`; the checked-in Compose files do not mount it in API-key mode. ## Repository configuration @@ -108,7 +108,7 @@ ignore: - generated/ ``` -The file is read from the PR head and is treated as untrusted data. It cannot choose commands, credentials, images, host paths, container privileges, or network policy. Tests are disabled by default; `tests` mode or `include_tests: true` explicitly permits the runner to execute project tests. +The file is read from the PR head and is treated as untrusted data. It cannot choose commands, credentials, images, host paths, container privileges, or network policy. Tests are disabled by default; use `tests` mode only when the deployment explicitly accepts execution of repository code in the runner. ## Webhooks and deployment diff --git a/cmd/gitea-codex/main.go b/cmd/gitea-codex/main.go index d10ab34..bb8eaac 100644 --- a/cmd/gitea-codex/main.go +++ b/cmd/gitea-codex/main.go @@ -3,6 +3,7 @@ package main import ( "context" "errors" + "fmt" "log/slog" "net/http" "os" @@ -21,34 +22,41 @@ import ( func main() { logger := slog.New(slog.NewJSONHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelInfo})) + if err := run(logger); err != nil { + logger.Error("service stopped with error", "error", err) + os.Exit(1) + } +} + +func run(logger *slog.Logger) error { settings, err := config.Load() if err != nil { - logger.Error("invalid configuration", "error", err) - os.Exit(1) + return fmt.Errorf("invalid configuration: %w", err) } st, err := sqlstore.Open(settings) if err != nil { - logger.Error("open database", "error", err) - os.Exit(1) + return fmt.Errorf("open database: %w", err) } defer st.Close() ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) err = st.Migrate(ctx) cancel() if err != nil { - logger.Error("migrate database", "error", err) - os.Exit(1) + return fmt.Errorf("migrate database: %w", err) } client := gitea.NewClient(settings) reviewRunner := runner.NewDockerRunner(settings) w := worker.New(settings, st, client, reviewRunner, logger) runCtx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) defer stop() + workerDone := make(chan struct{}) go func() { + defer close(workerDone) if err := w.Run(runCtx); err != nil && !errors.Is(err, context.Canceled) { logger.Error("worker stopped", "error", err) } }() + server := &http.Server{Addr: listenAddress(envString("PORT", "8000")), Handler: httpapi.New(settings, st, client, logger), ReadHeaderTimeout: 10 * time.Second, ReadTimeout: 30 * time.Second, WriteTimeout: 30 * time.Second, IdleTimeout: 60 * time.Second} logger.Info("server starting", "addr", server.Addr, "gitea_base_url", settings.GiteaBaseURL, "auth_mode", settings.CodexAuthMode, "concurrency", settings.Concurrency) go func() { @@ -57,11 +65,19 @@ func main() { defer cancel() _ = server.Shutdown(shutdown) }() - if err := server.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) { - logger.Error("server stopped", "error", err) - os.Exit(1) + serverErr := server.ListenAndServe() + stop() + select { + case <-workerDone: + case <-time.After(15 * time.Second): + return fmt.Errorf("worker shutdown timed out") } + if serverErr != nil && !errors.Is(serverErr, http.ErrServerClosed) { + return fmt.Errorf("server stopped: %w", serverErr) + } + return nil } + func envString(name, fallback string) string { if v := os.Getenv(name); v != "" { return v diff --git a/docker-compose.dev.yml b/docker-compose.dev.yml index 36528a9..3b15247 100644 --- a/docker-compose.dev.yml +++ b/docker-compose.dev.yml @@ -2,16 +2,16 @@ services: mariadb: image: mariadb:11 environment: - MARIADB_DATABASE: gitea_codex - MARIADB_USER: gitea_codex - MARIADB_PASSWORD: gitea_codex - MARIADB_ROOT_PASSWORD: rootpass + MARIADB_DATABASE: ${DB_NAME:-gitea_codex} + MARIADB_USER: ${DB_USER:-gitea_codex} + MARIADB_PASSWORD: ${DB_PASSWORD:?DB_PASSWORD must be set} + MARIADB_ROOT_PASSWORD: ${MARIADB_ROOT_PASSWORD:?MARIADB_ROOT_PASSWORD must be set} ports: - "3306:3306" volumes: - ./db:/var/lib/mysql healthcheck: - test: ["CMD", "mariadb-admin", "ping", "-h", "localhost", "-uroot", "-prootpass"] + test: ["CMD", "mariadb-admin", "ping", "-h", "localhost", "-uroot", "-p${MARIADB_ROOT_PASSWORD}"] interval: 5s timeout: 3s retries: 20 @@ -25,10 +25,11 @@ services: - .env environment: DATABASE_URL: gitea_codex:gitea_codex@tcp(mariadb:3306)/gitea_codex?parseTime=true - CODEX_AUTH_JSON_PATH: /root/.codex/auth.json volumes: - ./worktrees:/var/lib/gitea-codex/worktrees - - ~/.codex/auth.json:/root/.codex/auth.json:ro - /var/run/docker.sock:/var/run/docker.sock ports: - "8000:8000" + +# ChatGPT auth mode should use a separate local override that mounts the auth +# file only when CODEX_AUTH_MODE=chatgpt. API-key mode needs no auth.json mount. diff --git a/docker-compose.yml b/docker-compose.yml index fb9d380..09f2bd2 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -3,22 +3,20 @@ services: image: mariadb:11 restart: unless-stopped environment: - MARIADB_DATABASE: gitea_codex - MARIADB_USER: gitea_codex - MARIADB_PASSWORD: gitea_codex - MARIADB_ROOT_PASSWORD: rootpass - ports: - - "3306:3306" + MARIADB_DATABASE: ${DB_NAME:-gitea_codex} + MARIADB_USER: ${DB_USER:-gitea_codex} + MARIADB_PASSWORD: ${DB_PASSWORD:?DB_PASSWORD must be set} + MARIADB_ROOT_PASSWORD: ${MARIADB_ROOT_PASSWORD:?MARIADB_ROOT_PASSWORD must be set} volumes: - ./db:/var/lib/mysql healthcheck: - test: ["CMD", "mariadb-admin", "ping", "-h", "localhost", "-uroot", "-prootpass"] + test: ["CMD", "mariadb-admin", "ping", "-h", "localhost", "-uroot", "-p${MARIADB_ROOT_PASSWORD}"] interval: 5s timeout: 3s retries: 20 bot: - build: . + image: gitea.reversed.dev/space/gitea-codex:latest depends_on: mariadb: condition: service_healthy @@ -26,7 +24,6 @@ services: - .env volumes: - ./worktrees:/var/lib/gitea-codex/worktrees - - ~/.codex/auth.json:/root/.codex/auth.json:ro # The bot needs the host Docker API to launch isolated review containers. - /var/run/docker.sock:/var/run/docker.sock ports: diff --git a/docs/go-rebuild.md b/docs/go-rebuild.md index 4fceb34..46abd6b 100644 --- a/docs/go-rebuild.md +++ b/docs/go-rebuild.md @@ -8,7 +8,7 @@ The service was rebuilt from the product behavior rather than ported module-for- - Review result output is validated strictly and bounded before persistence or posting. - The landing and 404 pages are embedded and do not load Tailwind from a third-party CDN. - The current tested append-comment behavior is retained: each completed review posts a new comment and updates the latest `bot_comments` mapping. -- Docker execution is treated as a privileged deployment boundary. The bundled image runs the bot as root because direct Docker-socket access otherwise fails; production should replace this with a socket proxy or separate runner service, pinned images, resource limits, and least-privilege credentials. +- Docker execution is treated as a privileged deployment boundary. The bundled image runs the bot as root because direct Docker-socket access otherwise fails; production should replace this with a socket proxy or separate runner service, pin the runner image and Codex CLI instead of using the development bootstrap defaults, apply resource limits, and use least-privilege credentials. ## Migration compatibility diff --git a/internal/gitea/client.go b/internal/gitea/client.go index 6dc727c..ca8379f 100644 --- a/internal/gitea/client.go +++ b/internal/gitea/client.go @@ -8,7 +8,6 @@ import ( "io" "net/http" "net/url" - "strconv" "strings" "time" @@ -143,6 +142,9 @@ func (c *Client) PostIssueComment(ctx context.Context, repo string, number int, if err := json.Unmarshal(data, &p); err != nil { return 0, err } + if p.ID <= 0 { + return 0, fmt.Errorf("gitea comment response missing a positive id") + } return p.ID, nil } func (c *Client) EditIssueComment(ctx context.Context, repo string, commentID int64, body string) (int64, error) { @@ -160,6 +162,9 @@ func (c *Client) EditIssueComment(ctx context.Context, repo string, commentID in if err := json.Unmarshal(data, &p); err != nil { return 0, err } + if p.ID <= 0 { + return 0, fmt.Errorf("gitea comment response missing a positive id") + } return p.ID, nil } func (c *Client) GetIssueComments(ctx context.Context, repo string, number int) ([]map[string]any, error) { @@ -192,5 +197,3 @@ func (c *Client) GetIssueComment(ctx context.Context, repo string, commentID int } return p, nil } - -var _ = strconv.Itoa diff --git a/internal/httpapi/httpapi.go b/internal/httpapi/httpapi.go index b5a32ed..a3bf011 100644 --- a/internal/httpapi/httpapi.go +++ b/internal/httpapi/httpapi.go @@ -136,7 +136,44 @@ func (s *Server) webhook(w http.ResponseWriter, r *http.Request) { writeJSON(w, 200, map[string]any{"accepted": false, "reason": "no codex command"}) return } - inserted, err := s.store.InsertWebhookEvent(r.Context(), event) + + if cmd.IsReview() { + pr, prErr := s.gitea.GetPullRequest(r.Context(), event.Repo, event.PRNumber) + if prErr == nil { + event.HeadSHA = pr.HeadSHA + if text, configured, cfgErr := s.gitea.GetFileContent(r.Context(), event.Repo, ".codex-review.yml", event.HeadSHA); cfgErr != nil { + writeError(w, cfgErr) + return + } else if configured { + cfg, parseErr := review.ParseRepoConfig(text) + if parseErr != nil { + writeError(w, parseErr) + return + } + review.ApplyServerMaxDiff(&cfg, s.settings.MaxDiffBytes) + if !cfg.Enabled { + inserted, eventErr := s.store.InsertWebhookEvent(r.Context(), event) + if eventErr != nil { + writeError(w, eventErr) + return + } + if !inserted { + writeJSON(w, 200, map[string]any{"accepted": true, "reason": "duplicate event"}) + return + } + _, _ = s.gitea.PostIssueComment(r.Context(), event.Repo, event.PRNumber, review.FormatDisabledAck()) + writeJSON(w, 200, map[string]any{"accepted": true, "reason": "review disabled by repo config"}) + return + } + } + } + } + + cooldown := time.Duration(0) + if cmd.Name == "review" { + cooldown = time.Duration(s.settings.CooldownSeconds) * time.Second + } + job, inserted, remaining, err := s.store.EnqueueAcceptedJob(r.Context(), event, cmd, cooldown) if err != nil { writeError(w, err) return @@ -145,42 +182,9 @@ func (s *Server) webhook(w http.ResponseWriter, r *http.Request) { writeJSON(w, 200, map[string]any{"accepted": true, "reason": "duplicate event"}) return } - if cmd.IsReview() { - pr, prErr := s.gitea.GetPullRequest(r.Context(), event.Repo, event.PRNumber) - if prErr == nil { - event.HeadSHA = pr.HeadSHA - } - cfg := review.MissingRepoConfig() - if prErr == nil { - if text, configured, cfgErr := s.gitea.GetFileContent(r.Context(), event.Repo, ".codex-review.yml", event.HeadSHA); cfgErr == nil && configured { - cfg, err = review.ParseRepoConfig(text) - if err != nil { - writeError(w, err) - return - } - } - } - if !cfg.Enabled { - _, _ = s.gitea.PostIssueComment(r.Context(), event.Repo, event.PRNumber, review.FormatDisabledAck()) - writeJSON(w, 200, map[string]any{"accepted": true, "reason": "review disabled by repo config"}) - return - } - if cmd.Name != "rerun" { - remaining, err := s.store.CooldownRemaining(r.Context(), event.Repo, event.PRNumber, time.Duration(s.settings.CooldownSeconds)*time.Second) - if err != nil { - writeError(w, err) - return - } - if remaining > 0 { - _, _ = s.gitea.PostIssueComment(r.Context(), event.Repo, event.PRNumber, review.FormatCooldownAck(remaining)) - writeJSON(w, 200, map[string]any{"accepted": true, "reason": "cooldown active", "cooldown_seconds_remaining": remaining}) - return - } - } - } - job, err := s.store.EnqueueJob(r.Context(), event, cmd) - if err != nil { - writeError(w, err) + if remaining > 0 { + _, _ = s.gitea.PostIssueComment(r.Context(), event.Repo, event.PRNumber, review.FormatCooldownAck(remaining)) + writeJSON(w, 200, map[string]any{"accepted": true, "reason": "cooldown active", "cooldown_seconds_remaining": remaining}) return } if cmd.IsReview() { diff --git a/internal/review/review.go b/internal/review/review.go index 25d4cb3..1790833 100644 --- a/internal/review/review.go +++ b/internal/review/review.go @@ -44,11 +44,22 @@ func ParseRepoConfig(text string) (domain.RepoReviewConfig, error) { cfg.Ignore = boundedStrings(raw.Ignore, 128, 500) return cfg, nil } + func MissingRepoConfig() domain.RepoReviewConfig { cfg := domain.DefaultRepoReviewConfig() cfg.Configured = false return cfg } + +func ApplyServerMaxDiff(cfg *domain.RepoReviewConfig, serverMax int) { + if serverMax <= 0 { + return + } + if cfg.MaxDiffBytes <= 0 || cfg.MaxDiffBytes > serverMax { + cfg.MaxDiffBytes = serverMax + } +} + func boundedStrings(input []string, maxItems, maxLen int) []string { out := make([]string, 0, min(len(input), maxItems)) for _, item := range input { @@ -62,6 +73,7 @@ func boundedStrings(input []string, maxItems, maxLen int) []string { } return out } + func ResolveMode(cmd *domain.ParsedCommand, cfg domain.RepoReviewConfig) { if cmd.Name == "review" && !cmd.ModeExplicit { if cfg.DefaultMode == "full" || cfg.DefaultMode == "summary" || cfg.DefaultMode == "security" || cfg.DefaultMode == "performance" || cfg.DefaultMode == "tests" { @@ -71,15 +83,9 @@ func ResolveMode(cmd *domain.ParsedCommand, cfg domain.RepoReviewConfig) { } } } + func BuildPrompt(cmd domain.ParsedCommand, cfg domain.RepoReviewConfig, pr domain.PullRequestContext) string { - raw := strings.TrimSpace(cmd.Raw) - intent := raw - if at := strings.IndexAny(raw, " \t\r\n"); at >= 0 { - intent = strings.TrimSpace(raw[at:]) - } - if at := strings.IndexAny(intent, " \t\r\n"); at >= 0 { - intent = strings.TrimSpace(intent[at:]) - } + intent := commandIntent(cmd.Raw) if intent == "" { intent = "review this pull request and report introduced issues." } @@ -95,10 +101,28 @@ func BuildPrompt(cmd domain.ParsedCommand, cfg domain.RepoReviewConfig, pr domai if cmd.Mode == "tests" || cfg.IncludeTests { tests = "Tests may be executed for this run because tests mode/include_tests is explicitly enabled." } - return fmt.Sprintf("review: %s\nReview only issues introduced by this PR.\nCompare exactly these commits: base `%s` ... head `%s`.\nUse local git data from this checkout; do not review unrelated history.\nRequested mode: %s.\nFocus areas: %s.\nIgnore patterns: %s.\nInclude tests setting: %t.\n%s\nFull review requested: %t.\nReturn strict JSON matching the provided output schema.", intent, pr.BaseSHA, pr.HeadSHA, cmd.Mode, focus, ignore, cfg.IncludeTests, tests, cmd.Full) + return fmt.Sprintf("review: %s\nReview only issues introduced by this PR.\nCompare exactly these commits: base `%s` ... head `%s`.\nUse local git data from this checkout; do not review unrelated history.\nRequested mode: %s.\nFocus areas: %s.\nIgnore patterns: %s.\nMaximum diff bytes: %d.\nInclude tests setting: %t.\n%s\nFull review requested: %t.\nReturn strict JSON matching the provided output schema.", intent, pr.BaseSHA, pr.HeadSHA, cmd.Mode, focus, ignore, cfg.MaxDiffBytes, cfg.IncludeTests, tests, cmd.Full) +} + +func commandIntent(raw string) string { + raw = strings.TrimSpace(raw) + if raw == "" { + return "" + } + aliasEnd := strings.IndexAny(raw, " \t\r\n") + if aliasEnd < 0 { + return "" + } + remainder := strings.TrimSpace(raw[aliasEnd:]) + commandEnd := strings.IndexAny(remainder, " \t\r\n") + if commandEnd < 0 { + return "" + } + return strings.TrimSpace(remainder[commandEnd:]) } func ValidateResult(result domain.ReviewResult) error { return result.Validate() } + func DecodeResult(data []byte) (domain.ReviewResult, error) { var result domain.ReviewResult dec := json.NewDecoder(strings.NewReader(string(data))) @@ -111,6 +135,7 @@ func DecodeResult(data []byte) (domain.ReviewResult, error) { } return result, nil } + func FormatQueueAck(sha string) string { return fmt.Sprintf("👀 Codex review queued for commit `%s`.", first(sha, 7)) } @@ -123,28 +148,14 @@ func FormatDisabledAck() string { func FormatUnsupportedAck(name string) string { return fmt.Sprintf("⚠️ Command `@codex %s` is not enabled on this repository.", name) } + func FormatResultComment(sha string, result domain.ReviewResult, configured bool) string { marker := fmt.Sprintf("", sha) body := strings.TrimSpace(result.MarkdownComment) if body == "" { - body = fmt.Sprintf("## Codex Review\n\nVerdict: `%s`\nConfidence: `%.2f`\n\n%s", result.Verdict, result.Confidence, result.Summary) - if len(result.Findings) == 0 { - body += "\n\nNo blocking issues found." - } else { - body += "\n\nFindings:" - for i, f := range result.Findings { - suggestion := "n/a" - if f.Suggestion != nil && *f.Suggestion != "" { - suggestion = *f.Suggestion - } - body += fmt.Sprintf("\n\n%d. `%s:%d-%d` (%s)\n %s\n %s\n Suggestion: %s", i+1, f.File, f.LineStart, f.LineEnd, f.Severity, f.Title, f.Body, suggestion) - } - } + body = fallbackDetails(result) } else if len(result.Findings) > 0 { - body += "\n\n---\n\n### Structured Findings\n\n" - for i, f := range result.Findings { - body += fmt.Sprintf("%d. `%s:%d-%d` (%s)\n %s\n %s\n\n", i+1, f.File, f.LineStart, f.LineEnd, f.Severity, f.Title, f.Body) - } + body += "\n\n---\n\n" + structuredDetails(result) } if result.Meta != nil && result.Meta.Model != "" { body += fmt.Sprintf("\n_Note: model `%s`, input `%d`, output `%d`, total `%d` tokens used._", result.Meta.Model, result.Meta.Usage.InputTokens, result.Meta.Usage.OutputTokens, result.Meta.Usage.TotalTokens) @@ -162,6 +173,32 @@ func FormatResultComment(sha string, result domain.ReviewResult, configured bool } return body } + +func fallbackDetails(result domain.ReviewResult) string { + body := fmt.Sprintf("## Codex Review\n\nVerdict: `%s`\nConfidence: `%.2f`\n\n%s", result.Verdict, result.Confidence, result.Summary) + if len(result.Findings) == 0 { + return body + "\n\nNo blocking issues found." + } + return body + "\n\n" + structuredFindings(result.Findings) +} + +func structuredDetails(result domain.ReviewResult) string { + return fmt.Sprintf("### Structured Findings\n\nVerdict: `%s`\nConfidence: `%.2f`\n\n%s\n\n%s", result.Verdict, result.Confidence, result.Summary, structuredFindings(result.Findings)) +} + +func structuredFindings(findings []domain.Finding) string { + var body strings.Builder + body.WriteString("Findings:") + for i, f := range findings { + suggestion := "n/a" + if f.Suggestion != nil && *f.Suggestion != "" { + suggestion = *f.Suggestion + } + fmt.Fprintf(&body, "\n\n%d. `%s:%d-%d` (%s)\n %s\n %s\n Suggestion: %s", i+1, f.File, f.LineStart, f.LineEnd, f.Severity, f.Title, f.Body, suggestion) + } + return body.String() +} + func FailureComment(sha, errText string) string { return fmt.Sprintf("⚠️ Codex review run failed after queueing.\n\n- Commit: `%s`\n- Error: `%s`\n\nPlease rerun `@codex rerun` after checking worker logs.", first(sha, 7), first(strings.Join(strings.Fields(errText), " "), 500)) } diff --git a/internal/review/review_test.go b/internal/review/review_test.go index 34ad5a2..60388f5 100644 --- a/internal/review/review_test.go +++ b/internal/review/review_test.go @@ -35,6 +35,24 @@ func TestResultValidationAndFormatting(t *testing.T) { t.Fatalf("missing formatted details: %s", body) } } +func TestBuildPromptUsesGenericIntentForBareReview(t *testing.T) { + prompt := BuildPrompt(domain.ParsedCommand{Name: "review", Raw: "@codex review", Mode: "summary"}, domain.DefaultRepoReviewConfig(), domain.PullRequestContext{BaseSHA: "base", HeadSHA: "head"}) + if contains(prompt, "review: review\n") || !contains(prompt, "review: review this pull request and report introduced issues.") { + t.Fatalf("unexpected bare-review prompt: %s", prompt) + } +} + +func TestMarkdownCommentIncludesStructuredSummaryAndSuggestion(t *testing.T) { + suggestion := "Use a checked conversion." + result := domain.ReviewResult{Verdict: "has_issues", Confidence: .8, Summary: "Summary", MarkdownComment: "Primary markdown", Findings: []domain.Finding{{Severity: "medium", File: "x.go", LineStart: 2, LineEnd: 2, Title: "Issue", Body: "Details", Suggestion: &suggestion}}} + body := FormatResultComment("head", result, true) + for _, part := range []string{"Structured Findings", "Verdict: `has_issues`", "Summary", "Use a checked conversion."} { + if !contains(body, part) { + t.Fatalf("formatted comment missing %q: %s", part, body) + } + } +} + func contains(s, needle string) bool { for i := 0; i+len(needle) <= len(s); i++ { if s[i:i+len(needle)] == needle { diff --git a/internal/runner/runner.go b/internal/runner/runner.go index 95b0b2a..043763d 100644 --- a/internal/runner/runner.go +++ b/internal/runner/runner.go @@ -10,6 +10,7 @@ import ( "os/exec" "path/filepath" "strings" + "sync" "time" "gitea-codex-bot/internal/config" @@ -33,17 +34,17 @@ func (r *DockerRunner) Run(parent context.Context, pr domain.PullRequestContext, nonce := fmt.Sprintf("%d", time.Now().UnixNano()) begin, end := startMarker+"_"+nonce, endMarker+"_"+nonce prompt := review.BuildPrompt(cmd, cfg, pr) - script := r.script(pr, prompt, begin, end) + script := r.script(pr, prompt, begin, end, cfg.MaxDiffBytes) name := "codex-review-" + nonce - args := []string{"run", "--rm", "-i", "--name", name, "--cap-drop=ALL", "--security-opt", "no-new-privileges", "--read-only", "--tmpfs", "/tmp:rw,noexec,nosuid,size=512m", "--tmpfs", "/work:rw,nosuid,size=1g", "-e", "CODEX_DISABLE_TELEMETRY=1"} + args := []string{"run", "--rm", "-i", "--name", name, "--cap-drop=ALL", "--security-opt", "no-new-privileges", "--tmpfs", "/tmp:rw,noexec,nosuid,size=512m", "--tmpfs", "/work:rw,nosuid,size=1g", "-e", "CODEX_DISABLE_TELEMETRY=1"} if r.settings.CodexAuthMode == "chatgpt" { - args = append(args, "-e", "CODEX_AUTH_JSON_B64") + args = append(args, "--tmpfs", "/root/.codex:rw,nosuid,size=16m", "-e", "CODEX_AUTH_JSON_B64") } else { args = append(args, "-e", "OPENAI_API_KEY") } - args = append(args, "-e", "GITEA_TOKEN", "-e", "GITEA_GIT_USERNAME", r.settings.RunnerImage, "bash", "-lc", script) + args = append(args, "-e", "OPENAI_ORG_ID", "-e", "OPENAI_PROJECT_ID", "-e", "GITEA_TOKEN", "-e", "GITEA_GIT_USERNAME", r.settings.RunnerImage, "bash", "-lc", script) cmdExec := exec.CommandContext(ctx, "docker", args...) - cmdExec.Env = append(os.Environ(), "OPENAI_API_KEY="+r.settings.OpenAIAPIKey, "GITEA_TOKEN="+r.settings.GiteaToken, "GITEA_GIT_USERNAME="+r.settings.GiteaBotUsername) + cmdExec.Env = append(os.Environ(), "OPENAI_API_KEY="+r.settings.OpenAIAPIKey, "OPENAI_ORG_ID="+r.settings.OpenAIOrgID, "OPENAI_PROJECT_ID="+r.settings.OpenAIProjectID, "GITEA_TOKEN="+r.settings.GiteaToken, "GITEA_GIT_USERNAME="+r.settings.GiteaBotUsername) if r.settings.CodexAuthMode == "chatgpt" { data, err := readAuthJSON(r.settings.CodexAuthJSONPath) if err != nil { @@ -62,6 +63,9 @@ func (r *DockerRunner) Run(parent context.Context, pr domain.PullRequestContext, } return domain.ReviewResult{}, fmt.Errorf("review runner failed: %w", err) } + if output.Truncated() { + return domain.ReviewResult{}, fmt.Errorf("review runner output exceeded %d bytes", maxRunnerOutput) + } text := output.String() start := strings.Index(text, begin) endPos := strings.LastIndex(text, end) @@ -81,12 +85,12 @@ func (r *DockerRunner) Run(parent context.Context, pr domain.PullRequestContext, func readAuthJSON(rawPath string) ([]byte, error) { path := os.ExpandEnv(rawPath) - if strings.HasPrefix(path, "~/") { + if strings.HasPrefix(path, "~") && len(path) > 1 && (path[1] == '/' || path[1] == '\\') { home, err := os.UserHomeDir() if err != nil { return nil, err } - path = filepath.Join(home, strings.TrimPrefix(path, "~/")) + path = filepath.Join(home, strings.TrimLeft(path[1:], "/\\")) } data, err := os.ReadFile(filepath.Clean(path)) if err != nil { @@ -98,7 +102,7 @@ func readAuthJSON(rawPath string) ([]byte, error) { return data, nil } -func (r *DockerRunner) script(pr domain.PullRequestContext, prompt, begin, end string) string { +func (r *DockerRunner) script(pr domain.PullRequestContext, prompt, begin, end string, maxDiffBytes int) string { auth := base64.StdEncoding.EncodeToString([]byte(r.settings.GiteaBotUsername + ":" + r.settings.GiteaToken)) schema := `{"type":"object","additionalProperties":false,"required":["verdict","confidence","summary","findings","markdown_comment"],"properties":{"verdict":{"type":"string","enum":["correct","has_issues"]},"confidence":{"type":"number"},"summary":{"type":"string"},"markdown_comment":{"type":"string"},"findings":{"type":"array"}}}` quote := func(value string) string { return "'" + strings.ReplaceAll(value, "'", "'\"'\"'") + "'" } @@ -112,9 +116,10 @@ func (r *DockerRunner) script(pr domain.PullRequestContext, prompt, begin, end s if r.settings.CodexAuthMode == "chatgpt" { authSetup = "mkdir -p /root/.codex; printf '%s' \"$CODEX_AUTH_JSON_B64\" | base64 -d > /root/.codex/auth.json; chmod 600 /root/.codex/auth.json" } + bootstrap := "if ! command -v git >/dev/null 2>&1; then apt-get update >/tmp/apt-update.log 2>&1 && apt-get install -y --no-install-recommends ca-certificates git >/tmp/apt-install.log 2>&1; fi; if ! command -v codex >/dev/null 2>&1; then npm install -g @openai/codex@latest >/tmp/codex-install.log 2>&1; fi; command -v git >/dev/null 2>&1; command -v codex >/dev/null 2>&1" fetchHead := "git -c http.extraHeader=" + quote("Authorization: Basic "+auth) + " fetch --no-tags origin " + quote(pr.HeadRef) + " || git -c http.extraHeader=" + quote("Authorization: Basic "+auth) + " fetch --no-tags origin " + quote(pr.HeadSHA) fetchBase := "git -c http.extraHeader=" + quote("Authorization: Basic "+auth) + " fetch --no-tags " + baseRemote + " " + quote(pr.BaseRef) + " || git -c http.extraHeader=" + quote("Authorization: Basic "+auth) + " fetch --no-tags " + baseRemote + " " + quote(pr.BaseSHA) - steps := []string{"set -eu", "printf '%s' " + quote(schema) + " > /tmp/schema.json"} + steps := []string{"set -eu", "printf '%s' " + quote(schema) + " > /tmp/schema.json", bootstrap} if authSetup != "" { steps = append(steps, authSetup) } @@ -122,17 +127,24 @@ func (r *DockerRunner) script(pr domain.PullRequestContext, prompt, begin, end s if remoteSetup != "" { steps = append(steps, remoteSetup) } - steps = append(steps, fetchHead, fetchBase, "git checkout --detach "+quote(pr.HeadSHA), "test \"$(git rev-parse HEAD)\" = "+quote(pr.HeadSHA), "unset GITEA_TOKEN", "codex exec --sandbox danger-full-access --json --output-schema /tmp/schema.json -o /tmp/result.json -m "+quote(r.settings.OpenAIReviewModel)+" "+quote(prompt), "test -s /tmp/result.json", "printf '%s\\n' "+quote(begin), "cat /tmp/result.json", "printf '%s\\n' "+quote(end)) + steps = append(steps, fetchHead, fetchBase, "git checkout --detach "+quote(pr.HeadSHA), "test \"$(git rev-parse HEAD)\" = "+quote(pr.HeadSHA)) + if maxDiffBytes > 0 { + steps = append(steps, "diff_bytes=$(git diff --binary "+quote(pr.BaseSHA)+" "+quote(pr.HeadSHA)+" | wc -c); test \"$diff_bytes\" -le "+fmt.Sprintf("%d", maxDiffBytes)) + } + steps = append(steps, "unset GITEA_TOKEN", "codex exec --sandbox danger-full-access --json --output-schema /tmp/schema.json -o /tmp/result.json -m "+quote(r.settings.OpenAIReviewModel)+" "+quote(prompt), "test -s /tmp/result.json", "printf '%s\\n' "+quote(begin), "cat /tmp/result.json", "printf '%s\\n' "+quote(end)) return strings.Join(steps, "; ") } type limitedBuffer struct { + mu sync.Mutex buffer bytes.Buffer limit int truncated bool } func (b *limitedBuffer) Write(p []byte) (int, error) { + b.mu.Lock() + defer b.mu.Unlock() remaining := b.limit - b.buffer.Len() if remaining <= 0 { b.truncated = true @@ -145,6 +157,7 @@ func (b *limitedBuffer) Write(p []byte) (int, error) { } return b.buffer.Write(p) } -func (b *limitedBuffer) String() string { return b.buffer.String() } +func (b *limitedBuffer) String() string { b.mu.Lock(); defer b.mu.Unlock(); return b.buffer.String() } +func (b *limitedBuffer) Truncated() bool { b.mu.Lock(); defer b.mu.Unlock(); return b.truncated } var _ domain.ReviewRunner = (*DockerRunner)(nil) diff --git a/internal/runner/runner_test.go b/internal/runner/runner_test.go index 34a254c..ee48e4d 100644 --- a/internal/runner/runner_test.go +++ b/internal/runner/runner_test.go @@ -14,7 +14,7 @@ func samplePR() domain.PullRequestContext { func TestScriptChecksExactHeadAndBase(t *testing.T) { r := NewDockerRunner(config.Settings{GiteaBotUsername: "bot", GiteaToken: "token", OpenAIReviewModel: "model", CodexAuthMode: "api_key"}) - script := r.script(samplePR(), "review prompt", "BEGIN_nonce", "END_nonce") + script := r.script(samplePR(), "review prompt", "BEGIN_nonce", "END_nonce", 200000) for _, fragment := range []string{"git checkout --detach", "git rev-parse HEAD", "fetch --no-tags origin 'feature'", "fetch --no-tags origin '" + strings.Repeat("b", 40) + "'", "BEGIN_nonce", "END_nonce", "--output-schema", "-o /tmp/result.json"} { if !strings.Contains(script, fragment) { t.Fatalf("script missing %q: %s", fragment, script) @@ -32,7 +32,7 @@ func TestForkScriptUsesUpstreamBaseRemote(t *testing.T) { pr := samplePR() pr.BaseCloneURL = "https://gitea.test/base/repo.git" r := NewDockerRunner(config.Settings{GiteaBotUsername: "bot", GiteaToken: "token", OpenAIReviewModel: "model", CodexAuthMode: "api_key"}) - script := r.script(pr, "prompt", "BEGIN", "END") + script := r.script(pr, "prompt", "BEGIN", "END", 200000) if !strings.Contains(script, "git remote add upstream") || !strings.Contains(script, "fetch --no-tags upstream") { t.Fatalf("fork base remote was not configured: %s", script) } @@ -40,7 +40,7 @@ func TestForkScriptUsesUpstreamBaseRemote(t *testing.T) { func TestChatGPTScriptWritesAuthFile(t *testing.T) { r := NewDockerRunner(config.Settings{GiteaBotUsername: "bot", GiteaToken: "token", OpenAIReviewModel: "model", CodexAuthMode: "chatgpt"}) - script := r.script(samplePR(), "prompt", "BEGIN", "END") + script := r.script(samplePR(), "prompt", "BEGIN", "END", 200000) if !strings.Contains(script, "CODEX_AUTH_JSON_B64") || !strings.Contains(script, "chmod 600 /root/.codex/auth.json") { t.Fatalf("chatgpt auth setup missing: %s", script) } diff --git a/internal/store/sqlstore/sqlstore.go b/internal/store/sqlstore/sqlstore.go index 7936e93..c6567fd 100644 --- a/internal/store/sqlstore/sqlstore.go +++ b/internal/store/sqlstore/sqlstore.go @@ -8,6 +8,7 @@ import ( "fmt" "path/filepath" "strings" + "sync" "time" "gitea-codex-bot/internal/config" @@ -18,9 +19,14 @@ import ( _ "modernc.org/sqlite" ) +const defaultMaxRetries = 2 + +var ErrStaleRun = errors.New("review run is no longer current") + type Store struct { - db *sql.DB - dialect string + db *sql.DB + dialect string + enqueueMu sync.Mutex } func Open(settings config.Settings) (*Store, error) { @@ -41,11 +47,24 @@ func Open(settings config.Settings) (*Store, error) { } else if strings.HasPrefix(dsn, "file:") || dsn == ":memory:" || filepath.Ext(dsn) == ".db" { dialect = "sqlite" } + if dialect == "mysql" && !strings.Contains(dsn, "parseTime=") { + if strings.Contains(dsn, "?") { + dsn += "&parseTime=true" + } else { + dsn += "?parseTime=true" + } + } db, err := sql.Open(map[string]string{"sqlite": "sqlite", "mysql": "mysql"}[dialect], dsn) if err != nil { return nil, err } - db.SetMaxOpenConns(8) + if dialect == "sqlite" { + // SQLite has one writer and in-memory databases are connection-local. + db.SetMaxOpenConns(1) + db.SetMaxIdleConns(1) + } else { + db.SetMaxOpenConns(8) + } if err := db.Ping(); err != nil { _ = db.Close() return nil, err @@ -70,7 +89,7 @@ func (s *Store) schema() []string { if s.dialect == "sqlite" { return []string{ `CREATE TABLE IF NOT EXISTS webhook_events (id INTEGER PRIMARY KEY AUTOINCREMENT, delivery_id TEXT NULL UNIQUE, event_name TEXT NOT NULL, repo TEXT NOT NULL, comment_id INTEGER NULL, payload_sha256 TEXT NOT NULL, created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, UNIQUE(repo, comment_id))`, - `CREATE TABLE IF NOT EXISTS review_jobs (id INTEGER PRIMARY KEY AUTOINCREMENT, repo TEXT NOT NULL, pr_number INTEGER NOT NULL, head_sha TEXT NOT NULL, trigger_comment_id INTEGER NOT NULL, command TEXT NOT NULL, command_args TEXT NULL, trigger_comment_body TEXT NULL, requested_by TEXT NOT NULL, status TEXT NOT NULL, last_error TEXT NULL, result_json TEXT NULL, created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, updated_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, started_at TIMESTAMP NULL, finished_at TIMESTAMP NULL, UNIQUE(repo, trigger_comment_id))`, + `CREATE TABLE IF NOT EXISTS review_jobs (id INTEGER PRIMARY KEY AUTOINCREMENT, repo TEXT NOT NULL, pr_number INTEGER NOT NULL, head_sha TEXT NOT NULL, trigger_comment_id INTEGER NOT NULL, command TEXT NOT NULL, command_args TEXT NULL, requested_by TEXT NOT NULL, status TEXT NOT NULL, last_error TEXT NULL, result_json TEXT NULL, created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, updated_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, started_at TIMESTAMP NULL, finished_at TIMESTAMP NULL, UNIQUE(repo, trigger_comment_id))`, `CREATE INDEX IF NOT EXISTS ix_review_jobs_lookup ON review_jobs(repo, pr_number, head_sha, status, created_at)`, `CREATE TABLE IF NOT EXISTS review_runs (id INTEGER PRIMARY KEY AUTOINCREMENT, job_id INTEGER NOT NULL, status TEXT NOT NULL, runner_container_id TEXT NULL, result_json TEXT NULL, error_message TEXT NULL, started_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, finished_at TIMESTAMP NULL, FOREIGN KEY(job_id) REFERENCES review_jobs(id) ON DELETE CASCADE)`, `CREATE INDEX IF NOT EXISTS ix_review_runs_job_status ON review_runs(job_id, status)`, @@ -80,7 +99,7 @@ func (s *Store) schema() []string { } return []string{ `CREATE TABLE IF NOT EXISTS webhook_events (id BIGINT AUTO_INCREMENT PRIMARY KEY, delivery_id VARCHAR(255) NULL UNIQUE, event_name VARCHAR(128) NOT NULL, repo VARCHAR(255) NOT NULL, comment_id BIGINT NULL, payload_sha256 VARCHAR(64) NOT NULL, created_at DATETIME(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6), UNIQUE KEY uq_webhook_events_repo_comment (repo, comment_id)) ENGINE=InnoDB`, - `CREATE TABLE IF NOT EXISTS review_jobs (id BIGINT AUTO_INCREMENT PRIMARY KEY, repo VARCHAR(255) NOT NULL, pr_number INT NOT NULL, head_sha VARCHAR(64) NOT NULL, trigger_comment_id BIGINT NOT NULL, command VARCHAR(64) NOT NULL, command_args TEXT NULL, trigger_comment_body TEXT NULL, requested_by VARCHAR(255) NOT NULL, status VARCHAR(32) NOT NULL, last_error TEXT NULL, result_json JSON NULL, created_at DATETIME(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6), updated_at DATETIME(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6), started_at DATETIME(6) NULL, finished_at DATETIME(6) NULL, UNIQUE KEY uq_review_jobs_repo_trigger_comment (repo, trigger_comment_id), KEY ix_review_jobs_lookup (repo, pr_number, head_sha, status, created_at)) ENGINE=InnoDB`, + `CREATE TABLE IF NOT EXISTS review_jobs (id BIGINT AUTO_INCREMENT PRIMARY KEY, repo VARCHAR(255) NOT NULL, pr_number INT NOT NULL, head_sha VARCHAR(64) NOT NULL, trigger_comment_id BIGINT NOT NULL, command VARCHAR(64) NOT NULL, command_args TEXT NULL, requested_by VARCHAR(255) NOT NULL, status VARCHAR(32) NOT NULL, last_error TEXT NULL, result_json JSON NULL, created_at DATETIME(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6), updated_at DATETIME(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6), started_at DATETIME(6) NULL, finished_at DATETIME(6) NULL, UNIQUE KEY uq_review_jobs_repo_trigger_comment (repo, trigger_comment_id), KEY ix_review_jobs_lookup (repo, pr_number, head_sha, status, created_at)) ENGINE=InnoDB`, `CREATE TABLE IF NOT EXISTS review_runs (id BIGINT AUTO_INCREMENT PRIMARY KEY, job_id BIGINT NOT NULL, status VARCHAR(32) NOT NULL, runner_container_id VARCHAR(128) NULL, result_json JSON NULL, error_message TEXT NULL, started_at DATETIME(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6), finished_at DATETIME(6) NULL, CONSTRAINT fk_review_runs_job FOREIGN KEY(job_id) REFERENCES review_jobs(id) ON DELETE CASCADE, KEY ix_review_runs_job_status (job_id, status)) ENGINE=InnoDB`, `CREATE TABLE IF NOT EXISTS bot_comments (id BIGINT AUTO_INCREMENT PRIMARY KEY, repo VARCHAR(255) NOT NULL, pr_number INT NOT NULL, head_sha VARCHAR(64) NOT NULL, gitea_comment_id BIGINT NOT NULL, marker VARCHAR(255) NOT NULL, created_at DATETIME(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6), updated_at DATETIME(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6), UNIQUE KEY uq_bot_comments_marker (repo, pr_number, marker), KEY ix_bot_comments_repo_pr (repo, pr_number)) ENGINE=InnoDB`, } @@ -131,6 +150,57 @@ func (s *Store) EnqueueJob(ctx context.Context, event domain.WebhookEvent, comma return s.getJob(ctx, id) } +func (s *Store) EnqueueAcceptedJob(ctx context.Context, event domain.WebhookEvent, command domain.ParsedCommand, cooldown time.Duration) (domain.Job, bool, int, error) { + s.enqueueMu.Lock() + defer s.enqueueMu.Unlock() + tx, err := s.db.BeginTx(ctx, nil) + if err != nil { + return domain.Job{}, false, 0, err + } + defer tx.Rollback() + _, err = tx.ExecContext(ctx, `INSERT INTO webhook_events(delivery_id,event_name,repo,comment_id,payload_sha256) VALUES(?,?,?,?,?)`, nullable(event.DeliveryID), event.EventName, event.Repo, event.CommentID, event.PayloadSHA256) + if err != nil { + if isConstraint(err) { + return domain.Job{}, false, 0, nil + } + return domain.Job{}, false, 0, err + } + if command.Name == "review" && cooldown > 0 { + cutoff := time.Now().UTC().Add(-cooldown) + query := `SELECT created_at FROM review_jobs WHERE repo=? AND pr_number=? AND created_at>=? ORDER BY created_at DESC LIMIT 1` + if s.dialect == "mysql" { + query += ` FOR UPDATE` + } + var created time.Time + if err := tx.QueryRowContext(ctx, query, event.Repo, event.PRNumber, cutoff).Scan(&created); err == nil { + remaining := int((cooldown - time.Since(created)).Seconds()) + if remaining < 0 { + remaining = 0 + } + if err := tx.Commit(); err != nil { + return domain.Job{}, false, 0, err + } + return domain.Job{}, true, remaining, nil + } else if !errors.Is(err, sql.ErrNoRows) { + return domain.Job{}, false, 0, err + } + } + args, _ := json.Marshal(command.Arguments) + result, err := tx.ExecContext(ctx, `INSERT INTO review_jobs(repo,pr_number,head_sha,trigger_comment_id,command,command_args,trigger_comment_body,requested_by,status) VALUES(?,?,?,?,?,?,?,?,?)`, event.Repo, event.PRNumber, event.HeadSHA, event.CommentID, command.Name, string(args), event.CommentBody, event.Sender, domain.JobQueued) + if err != nil { + return domain.Job{}, false, 0, err + } + id, err := result.LastInsertId() + if err != nil { + return domain.Job{}, false, 0, err + } + if err := tx.Commit(); err != nil { + return domain.Job{}, false, 0, err + } + job, err := s.getJob(ctx, id) + return job, true, 0, err +} + func (s *Store) ClaimNextJob(ctx context.Context, now time.Time, lease time.Duration, maxRetries int) (*domain.Job, *domain.ReviewRun, error) { if err := s.recoverStale(ctx, now, lease, maxRetries); err != nil { return nil, nil, err @@ -215,7 +285,7 @@ func (s *Store) FinishJob(ctx context.Context, jobID, runID int64, success, skip if err := s.db.QueryRowContext(ctx, `SELECT COUNT(*) FROM review_runs WHERE job_id=?`, jobID).Scan(&attempts); err != nil { return err } - if attempts <= 3 { + if attempts-1 < defaultMaxRetries { status = domain.JobQueued } } @@ -232,17 +302,24 @@ func (s *Store) FinishJob(ctx context.Context, jobID, runID int64, success, skip return err } defer tx.Rollback() + runUpdate, err := tx.ExecContext(ctx, `UPDATE review_runs SET status=?,finished_at=?,error_message=?,result_json=? WHERE id=? AND status=?`, runStatus, now, nullable(errText), nullableBytes(resultJSON), runID, domain.RunRunning) + if err != nil { + return err + } + if affected, affectedErr := runUpdate.RowsAffected(); affectedErr != nil || affected == 0 { + return ErrStaleRun + } + var jobUpdate sql.Result if status == domain.JobQueued { - _, err = tx.ExecContext(ctx, `UPDATE review_jobs SET status=?,started_at=NULL,finished_at=NULL,last_error=?,result_json=?,updated_at=? WHERE id=?`, status, nullable(errText), nullableBytes(resultJSON), now, jobID) + jobUpdate, err = tx.ExecContext(ctx, `UPDATE review_jobs SET status=?,started_at=NULL,finished_at=NULL,last_error=?,result_json=?,updated_at=? WHERE id=? AND status=?`, status, nullable(errText), nullableBytes(resultJSON), now, jobID, domain.JobRunning) } else { - _, err = tx.ExecContext(ctx, `UPDATE review_jobs SET status=?,finished_at=?,last_error=?,result_json=?,updated_at=? WHERE id=?`, status, now, nullable(errText), nullableBytes(resultJSON), now, jobID) + jobUpdate, err = tx.ExecContext(ctx, `UPDATE review_jobs SET status=?,finished_at=?,last_error=?,result_json=?,updated_at=? WHERE id=? AND status=?`, status, now, nullable(errText), nullableBytes(resultJSON), now, jobID, domain.JobRunning) } if err != nil { return err } - _, err = tx.ExecContext(ctx, `UPDATE review_runs SET status=?,finished_at=?,error_message=?,result_json=? WHERE id=?`, runStatus, now, nullable(errText), nullableBytes(resultJSON), runID) - if err != nil { - return err + if affected, affectedErr := jobUpdate.RowsAffected(); affectedErr != nil || affected == 0 { + return ErrStaleRun } return tx.Commit() } diff --git a/internal/store/store.go b/internal/store/store.go index 1e1fe53..ff37043 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -12,6 +12,7 @@ type Store interface { InsertWebhookEvent(context.Context, domain.WebhookEvent) (bool, error) CooldownRemaining(context.Context, string, int, time.Duration) (int, error) EnqueueJob(context.Context, domain.WebhookEvent, domain.ParsedCommand) (domain.Job, error) + EnqueueAcceptedJob(context.Context, domain.WebhookEvent, domain.ParsedCommand, time.Duration) (domain.Job, bool, int, error) ClaimNextJob(context.Context, time.Time, time.Duration, int) (*domain.Job, *domain.ReviewRun, error) FinishJob(context.Context, int64, int64, bool, bool, *domain.ReviewResult, error) error LatestFailedJob(context.Context) (*domain.Job, error) diff --git a/internal/worker/e2e_test.go b/internal/worker/e2e_test.go new file mode 100644 index 0000000..311c1e2 --- /dev/null +++ b/internal/worker/e2e_test.go @@ -0,0 +1,144 @@ +package worker + +import ( + "context" + "crypto/hmac" + "crypto/sha256" + "encoding/hex" + "encoding/json" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "strconv" + "strings" + "sync" + "testing" + "time" + + "gitea-codex-bot/internal/config" + "gitea-codex-bot/internal/domain" + "gitea-codex-bot/internal/gitea" + "gitea-codex-bot/internal/httpapi" + "gitea-codex-bot/internal/store/sqlstore" +) + +type e2eRunner struct { + mu sync.Mutex + headSHA string +} + +func (r *e2eRunner) Run(_ context.Context, pr domain.PullRequestContext, _ domain.ParsedCommand, _ domain.RepoReviewConfig) (domain.ReviewResult, error) { + r.mu.Lock() + r.headSHA = pr.HeadSHA + r.mu.Unlock() + return domain.ReviewResult{Verdict: "correct", Confidence: 0.99, Summary: "fake review completed", MarkdownComment: "Fake review completed.", Findings: []domain.Finding{}}, nil +} + +func TestWebhookToWorkerToGiteaWithFakeRunner(t *testing.T) { + var commentsMu sync.Mutex + var comments []string + giteaServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch { + case strings.Contains(r.URL.Path, "/pulls/"): + _, _ = io.WriteString(w, `{"base":{"ref":"main","sha":"base-sha","repo":{"clone_url":"https://gitea.test/acme/repo.git","full_name":"acme/repo"}},"head":{"ref":"feature","sha":"head-sha","repo":{"clone_url":"https://gitea.test/acme/repo.git","full_name":"acme/repo"}},"html_url":"https://gitea.test/acme/repo/pulls/9"}`) + case strings.Contains(r.URL.Path, "/contents/"): + http.NotFound(w, r) + case r.Method == http.MethodPost && strings.Contains(r.URL.Path, "/comments"): + body, _ := io.ReadAll(r.Body) + var payload map[string]string + _ = json.Unmarshal(body, &payload) + commentsMu.Lock() + comments = append(comments, payload["body"]) + commentID := 100 + len(comments) + commentsMu.Unlock() + _, _ = io.WriteString(w, `{"id":`+jsonNumber(commentID)+`}`) + default: + http.NotFound(w, r) + } + })) + defer giteaServer.Close() + + settings := config.Settings{GiteaBaseURL: giteaServer.URL, GiteaToken: "test-token", GiteaBotUsername: "codex-bot", GiteaWebhookSecret: "secret", AllowedRepos: []string{"acme/repo"}, DatabaseURL: "sqlite://" + t.TempDir() + "/e2e.db", CooldownSeconds: 60, MaxDiffBytes: 200000, MaxReviewMinutes: 1, Concurrency: 1, WebhookMaxBytes: 1 << 20} + st, err := sqlstore.Open(settings) + if err != nil { + t.Fatal(err) + } + defer st.Close() + if err := st.Migrate(context.Background()); err != nil { + t.Fatal(err) + } + + runner := &e2eRunner{} + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + client := gitea.NewClient(settings) + worker := New(settings, st, client, runner, logger) + workerCtx, cancelWorker := context.WithCancel(context.Background()) + workerDone := make(chan struct{}) + go func() { defer close(workerDone); _ = worker.Run(workerCtx) }() + defer func() { cancelWorker(); <-workerDone }() + apiServer := httptest.NewServer(httpapi.New(settings, st, client, logger)) + defer apiServer.Close() + + payload := []byte(`{"repository":{"full_name":"acme/repo"},"sender":{"username":"alice"},"comment":{"id":901,"body":"@codex review security"},"issue":{"number":9,"pull_request":{"url":"fake"}},"pull_request":{"head":{"sha":"head-sha"}}}`) + mac := hmac.New(sha256.New, []byte(settings.GiteaWebhookSecret)) + _, _ = mac.Write(payload) + req, err := http.NewRequest(http.MethodPost, apiServer.URL+"/webhook/gitea", strings.NewReader(string(payload))) + if err != nil { + t.Fatal(err) + } + req.Header.Set("Content-Type", "application/json") + req.Header.Set("X-Gitea-Event", "issue_comment") + req.Header.Set("X-Gitea-Delivery", "e2e-901") + req.Header.Set("X-Gitea-Signature", hex.EncodeToString(mac.Sum(nil))) + resp, err := http.DefaultClient.Do(req) + if err != nil { + t.Fatal(err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Fatalf("webhook status: %d", resp.StatusCode) + } + + deadline := time.Now().Add(5 * time.Second) + var job domain.Job + for time.Now().Before(deadline) { + candidate, queryErr := st.LatestJob(context.Background()) + if queryErr != nil { + t.Fatal(queryErr) + } + if candidate != nil { + job = *candidate + if job.Status == domain.JobSucceeded { + break + } + } + time.Sleep(50 * time.Millisecond) + } + if job.Status != domain.JobSucceeded { + t.Fatalf("job did not succeed: %#v", job) + } + runner.mu.Lock() + observedHead := runner.headSHA + runner.mu.Unlock() + if observedHead != "head-sha" { + t.Fatalf("runner reviewed %q instead of queued head", observedHead) + } + commentID, err := st.BotCommentID(context.Background(), "acme/repo", 9, "codex-review") + if err != nil { + t.Fatal(err) + } + if commentID <= 0 { + t.Fatalf("missing bot comment mapping: %d", commentID) + } + commentsMu.Lock() + defer commentsMu.Unlock() + if len(comments) < 2 || !strings.Contains(comments[len(comments)-1], "codex-review:head_sha=head-sha") { + t.Fatalf("unexpected comments: %#v", comments) + } +} + +func jsonNumber(value int) string { return strconv.Itoa(value) } + +var _ domain.ReviewRunner = (*e2eRunner)(nil) diff --git a/internal/worker/worker.go b/internal/worker/worker.go index 7bde56b..f85d4c1 100644 --- a/internal/worker/worker.go +++ b/internal/worker/worker.go @@ -66,13 +66,17 @@ func (w *Worker) process(ctx context.Context, job domain.Job, run domain.ReviewR if err != nil { return w.fail(ctx, job, run, err) } + if job.HeadSHA != "" && job.HeadSHA != "unknown" { + // The queued SHA is immutable job input. The PR may have advanced while queued. + pr.HeadSHA = job.HeadSHA + } if pr.IsFork && !w.settings.AllowUntrustedForks { message := "Skipped review for fork PR because `ALLOW_UNTRUSTED_FORKS=false`." _, postErr := w.gitea.PostIssueComment(ctx, job.Repo, job.PRNumber, message) if postErr != nil { return w.fail(ctx, job, run, postErr) } - return w.store.FinishJob(ctx, job.ID, run.ID, true, true, &domain.ReviewResult{Verdict: "correct", Confidence: 1, Summary: message, Findings: []domain.Finding{}}, nil) + return w.finish(ctx, job.ID, run.ID, true, true, &domain.ReviewResult{Verdict: "correct", Confidence: 1, Summary: message, Findings: []domain.Finding{}}, nil) } cfg := review.MissingRepoConfig() text, configured, cfgErr := w.gitea.GetFileContent(ctx, job.Repo, ".codex-review.yml", pr.HeadSHA) @@ -85,12 +89,13 @@ func (w *Worker) process(ctx context.Context, job domain.Job, run domain.ReviewR return w.fail(ctx, job, run, err) } } + review.ApplyServerMaxDiff(&cfg, w.settings.MaxDiffBytes) if !cfg.Enabled { _, postErr := w.gitea.PostIssueComment(ctx, job.Repo, job.PRNumber, review.FormatDisabledAck()) if postErr != nil { return w.fail(ctx, job, run, postErr) } - return w.store.FinishJob(ctx, job.ID, run.ID, true, true, &domain.ReviewResult{Verdict: "correct", Confidence: 1, Summary: review.FormatDisabledAck(), Findings: []domain.Finding{}}, nil) + return w.finish(ctx, job.ID, run.ID, true, true, &domain.ReviewResult{Verdict: "correct", Confidence: 1, Summary: review.FormatDisabledAck(), Findings: []domain.Finding{}}, nil) } review.ResolveMode(&cmd, cfg) result, err := w.runner.Run(ctx, pr, cmd, cfg) @@ -105,13 +110,13 @@ func (w *Worker) process(ctx context.Context, job domain.Job, run domain.ReviewR if err := w.store.UpsertBotComment(ctx, job.Repo, job.PRNumber, "codex-review", commentID, pr.HeadSHA); err != nil { return w.fail(ctx, job, run, err) } - return w.store.FinishJob(ctx, job.ID, run.ID, true, false, &result, nil) + return w.finish(ctx, job.ID, run.ID, true, false, &result, nil) } func (w *Worker) processNonReview(ctx context.Context, job domain.Job, run domain.ReviewRun, cmd domain.ParsedCommand) error { switch cmd.Name { case "ignore": result := domain.ReviewResult{Verdict: "correct", Confidence: 1, Summary: "Ignore command acknowledged. No review run executed.", Findings: []domain.Finding{}} - return w.store.FinishJob(ctx, job.ID, run.ID, true, true, &result, nil) + return w.finish(ctx, job.ID, run.ID, true, true, &result, nil) case "explain": latest, err := w.store.LatestSuccessfulReview(ctx, job.Repo, job.PRNumber) if err != nil { @@ -127,7 +132,7 @@ func (w *Worker) processNonReview(ctx context.Context, job domain.Job, run domai if _, err := w.gitea.PostIssueComment(ctx, job.Repo, job.PRNumber, message); err != nil { return w.fail(ctx, job, run, err) } - return w.store.FinishJob(ctx, job.ID, run.ID, true, true, &domain.ReviewResult{Verdict: "correct", Confidence: 1, Summary: message, Findings: []domain.Finding{}}, nil) + return w.finish(ctx, job.ID, run.ID, true, true, &domain.ReviewResult{Verdict: "correct", Confidence: 1, Summary: message, Findings: []domain.Finding{}}, nil) case "help": comments, err := w.gitea.GetIssueComments(ctx, job.Repo, job.PRNumber) if err != nil { @@ -141,7 +146,7 @@ func (w *Worker) processNonReview(ctx context.Context, job domain.Job, run domai if _, err := w.gitea.PostIssueComment(ctx, job.Repo, job.PRNumber, message); err != nil { return w.fail(ctx, job, run, err) } - return w.store.FinishJob(ctx, job.ID, run.ID, true, true, &domain.ReviewResult{Verdict: "correct", Confidence: 1, Summary: "Help/status summary posted.", Findings: []domain.Finding{}}, nil) + return w.finish(ctx, job.ID, run.ID, true, true, &domain.ReviewResult{Verdict: "correct", Confidence: 1, Summary: "Help/status summary posted.", Findings: []domain.Finding{}}, nil) } return w.fail(ctx, job, run, fmt.Errorf("unsupported worker command %q", cmd.Name)) } @@ -150,25 +155,41 @@ func (w *Worker) fail(ctx context.Context, job domain.Job, run domain.ReviewRun, if errorText == "" { errorText = "review failed" } - if _, postErr := w.gitea.PostIssueComment(ctx, job.Repo, job.PRNumber, review.FailureComment(job.HeadSHA, errorText)); postErr != nil { + finalCtx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + if _, postErr := w.gitea.PostIssueComment(finalCtx, job.Repo, job.PRNumber, review.FailureComment(job.HeadSHA, errorText)); postErr != nil { w.logger.Error("post failure comment", "job_id", job.ID, "error", postErr) } - return w.store.FinishJob(ctx, job.ID, run.ID, false, false, nil, fmt.Errorf("%s", errorText)) + return w.store.FinishJob(finalCtx, job.ID, run.ID, false, false, nil, fmt.Errorf("%s", errorText)) +} +func (w *Worker) finish(ctx context.Context, jobID, runID int64, success, skipped bool, result *domain.ReviewResult, err error) error { + if ctx.Err() != nil { + finalCtx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + return w.store.FinishJob(finalCtx, jobID, runID, success, skipped, result, err) + } + return w.store.FinishJob(ctx, jobID, runID, success, skipped, result, err) } func commandFromJob(job domain.Job, aliases map[string]bool) domain.ParsedCommand { if parsed, ok := commands.Parse(job.TriggerCommentBody, aliases); ok { return parsed } - args := strings.Fields(job.CommandArgs) - return domain.ParsedCommand{Name: job.Command, Raw: job.TriggerCommentBody, Arguments: args, Mode: "summary", Full: contains(args, "--full")} -} -func contains(items []string, needle string) bool { - for _, item := range items { - if item == needle { - return true + var args []string + if json.Unmarshal([]byte(job.CommandArgs), &args) != nil { + args = strings.Fields(job.CommandArgs) + } + cmd := domain.ParsedCommand{Name: job.Command, Raw: job.TriggerCommentBody, Arguments: args, Mode: "summary"} + if job.Command == "review" { + for _, arg := range args { + switch arg { + case "security", "performance", "tests": + cmd.Mode, cmd.ModeExplicit = arg, true + case "--full": + cmd.Mode, cmd.ModeExplicit, cmd.Full = "full", true, true + } } } - return false + return cmd } func helpComment(comments []map[string]any, bot string, pending int) string { bot = strings.ToLower(strings.TrimSpace(bot)) @@ -197,7 +218,7 @@ func helpComment(comments []map[string]any, bot string, pending int) string { lines = append(lines, fmt.Sprintf("- @%s: %s", user, body)) } } - lines[11] = fmt.Sprintf("Discussion summary (%d comments, human `%d`, bot `%d`):", len(comments), human, bots) + lines[12] = fmt.Sprintf("Discussion summary (%d comments, human `%d`, bot `%d`):", len(comments), human, bots) return strings.Join(lines, "\n") } func sleep(ctx context.Context, duration time.Duration) { diff --git a/migrations/0001_initial.sql b/migrations/0001_initial.sql index 113e338..e9fc1ab 100644 --- a/migrations/0001_initial.sql +++ b/migrations/0001_initial.sql @@ -1,3 +1,57 @@ --- The Go migrator creates this logical schema with dialect-specific SQL. --- This file documents the compatibility baseline: webhook_events, review_jobs, --- review_runs, and bot_comments with the constraints described in README.md. +-- MariaDB baseline schema. The Go startup migrator emits equivalent SQLite SQL. +CREATE TABLE IF NOT EXISTS webhook_events ( + id BIGINT AUTO_INCREMENT PRIMARY KEY, + delivery_id VARCHAR(255) NULL UNIQUE, + event_name VARCHAR(128) NOT NULL, + repo VARCHAR(255) NOT NULL, + comment_id BIGINT NULL, + payload_sha256 VARCHAR(64) NOT NULL, + created_at DATETIME(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6), + UNIQUE KEY uq_webhook_events_repo_comment (repo, comment_id) +) ENGINE=InnoDB; + +CREATE TABLE IF NOT EXISTS review_jobs ( + id BIGINT AUTO_INCREMENT PRIMARY KEY, + repo VARCHAR(255) NOT NULL, + pr_number INT NOT NULL, + head_sha VARCHAR(64) NOT NULL, + trigger_comment_id BIGINT NOT NULL, + command VARCHAR(64) NOT NULL, + command_args TEXT NULL, + requested_by VARCHAR(255) NOT NULL, + status VARCHAR(32) NOT NULL, + last_error TEXT NULL, + result_json JSON NULL, + created_at DATETIME(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6), + updated_at DATETIME(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6), + started_at DATETIME(6) NULL, + finished_at DATETIME(6) NULL, + UNIQUE KEY uq_review_jobs_repo_trigger_comment (repo, trigger_comment_id), + KEY ix_review_jobs_lookup (repo, pr_number, head_sha, status, created_at) +) ENGINE=InnoDB; + +CREATE TABLE IF NOT EXISTS review_runs ( + id BIGINT AUTO_INCREMENT PRIMARY KEY, + job_id BIGINT NOT NULL, + status VARCHAR(32) NOT NULL, + runner_container_id VARCHAR(128) NULL, + result_json JSON NULL, + error_message TEXT NULL, + started_at DATETIME(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6), + finished_at DATETIME(6) NULL, + CONSTRAINT fk_review_runs_job FOREIGN KEY (job_id) REFERENCES review_jobs(id) ON DELETE CASCADE, + KEY ix_review_runs_job_status (job_id, status) +) ENGINE=InnoDB; + +CREATE TABLE IF NOT EXISTS bot_comments ( + id BIGINT AUTO_INCREMENT PRIMARY KEY, + repo VARCHAR(255) NOT NULL, + pr_number INT NOT NULL, + head_sha VARCHAR(64) NOT NULL, + gitea_comment_id BIGINT NOT NULL, + marker VARCHAR(255) NOT NULL, + created_at DATETIME(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6), + updated_at DATETIME(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6), + UNIQUE KEY uq_bot_comments_marker (repo, pr_number, marker), + KEY ix_bot_comments_repo_pr (repo, pr_number) +) ENGINE=InnoDB; diff --git a/migrations/0002_trigger_comment_body.sql b/migrations/0002_trigger_comment_body.sql index dad4611..bb37ebe 100644 --- a/migrations/0002_trigger_comment_body.sql +++ b/migrations/0002_trigger_comment_body.sql @@ -1 +1,2 @@ --- Compatibility migration: add review_jobs.trigger_comment_body when absent. +-- MariaDB compatibility migration. +ALTER TABLE review_jobs ADD COLUMN trigger_comment_body TEXT NULL;