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 <noreply@anthropic.com>
2026-07-12 22:20:48 +02:00
parent f19b271642
commit 85c0e735dc
19 changed files with 539 additions and 142 deletions
@@ -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)
@@ -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) {