Make the reviewer configuration a value, not a field

internal/crq/codex_replay_test.go

@@ -382,7 +382,7 @@ func TestObserveScopesShellFilterToCodeRabbit(t *testing.T) {
 	f.gh.reviews[key] = []ghapi.Review{crShell, codexReview}
 	f.gh.mu.Unlock()
 
-	obs, err := f.svc.observe(f.ctx, repo, pr, nil, f.clk.now())
+	obs, err := f.svc.observe(f.ctx, f.svc.cfg, repo, pr, nil, f.clk.now())
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -495,7 +495,7 @@ func TestSelfHealCodexClaimPreventsDoublePost(t *testing.T) {
 	}
 	st, _, _ := f.store.Load(f.ctx)
 	round := st.Round(repo, pr)
-	obs, err := f.svc.observe(f.ctx, repo, pr, round, f.clk.now())
+	obs, err := f.svc.observe(f.ctx, f.svc.cfg, repo, pr, round, f.clk.now())
 	if err != nil {
 		t.Fatal(err)
 	}

internal/crq/feedback.go

@@ -88,7 +88,7 @@ func (s *Service) Feedback(ctx context.Context, repo string, pr int) (FeedbackRe
 	// findings from the raw reviews/comments and derives convergence from
 	// engine.Completion over the same snapshot — no second fetch path, and the
 	// "is head reviewed?" rules live only in the engine.
-	obs, err := s.observe(ctx, repo, pr, round, now)
+	obs, err := s.observe(ctx, s.cfg, repo, pr, round, now)
 	if err != nil {
 		return FeedbackReport{}, err
 	}
@@ -123,13 +123,13 @@ func (s *Service) Feedback(ctx context.Context, repo string, pr int) (FeedbackRe
 	if anchorOK {
 		anchorCutoff = completionRound.FiredAt.UTC()
 	}
-	completion := engine.Completion(completionRound, obs.eng, s.policy())
+	completion := engine.Completion(completionRound, obs.eng, s.cfg.policy())
 	report.ReviewedBy = completion.ReviewedBy
 	// Completion does not always arrive as a review: a clean-summary comment, a
 	// paired completion reply and a co-reviewer check run all satisfy it. Anchor
 	// the settle window on the newest of ANY of them, or a round completed by a
 	// comment would settle against a stale timestamp and converge instantly.
-	evidenceBots := dialect.BotSet(unionBots(s.cfg.FeedbackBots, s.cfg.RequiredBots))
+	evidenceBots := s.cfg.evidenceBots()
 	noteEvidence := func(at time.Time) {
 		if at.After(report.LastEvidenceAt) {
 			report.LastEvidenceAt = at
@@ -156,7 +156,7 @@ func (s *Service) Feedback(ctx context.Context, repo string, pr int) (FeedbackRe
 		verdictCutoff = obs.eng.HeadAt
 	}
 	report.CoReviewers = coReviewerStatuses(s.cfg, obs.eng, verdictCutoff)
-	if why := engine.PrimaryUnavailableReason(obs.eng, s.policy(), head); why != "" {
+	if why := engine.PrimaryUnavailableReason(obs.eng, s.cfg.policy(), head); why != "" {
 		report.PrimaryUnavailable = true
 		report.PrimaryUnavailableReason = s.cfg.Bot + " " + why
 	}
@@ -166,7 +166,7 @@ func (s *Service) Feedback(ctx context.Context, repo string, pr int) (FeedbackRe
 	// hang convergence if it were) still has its findings reported instead of
 	// dropped. It always includes the required bots: a bot crq waits for whose
 	// findings it didn't surface would hang the loop forever.
-	extractBots := dialect.BotSet(unionBots(s.cfg.FeedbackBots, s.cfg.RequiredBots))
+	extractBots := s.cfg.evidenceBots()
 
 	// Review-body findings — CodeRabbit's detailed and "Prompt for AI agents"
 	// blocks — carry no per-finding resolution state, only the review's commit.
@@ -188,7 +188,7 @@ func (s *Service) Feedback(ctx context.Context, repo string, pr int) (FeedbackRe
 		// before that round belongs to the previous head. Unresolved threads are
 		// still surfaced below across commits, while thread-less body findings
 		// must be re-reported by the current round instead of trapping the loop.
-		if anchorOK && s.isConfiguredBot(login) &&
+		if anchorOK && s.cfg.isConfiguredBot(login) &&
 			(head == "" || !strings.HasPrefix(review.CommitID, head)) &&
 			!notBefore(review.SubmittedAt, anchorCutoff) {
 			continue
@@ -362,7 +362,7 @@ func (s *Service) Feedback(ctx context.Context, repo string, pr int) (FeedbackRe
 		if !dialect.InBots(extractBots, comment.User.Login) {
 			continue
 		}
-		if s.cr.IsReviewSkipped(comment.Body) && s.isConfiguredBot(comment.User.Login) &&
+		if s.cr.IsReviewSkipped(comment.Body) && s.cfg.isConfiguredBot(comment.User.Login) &&
 			skipAppliesToHead(comment.Body, head) &&
 			!skipPredatesHead(comment, headCutoffOf) {
 			// Checked BEFORE the rate-limit guard below: the skip notice embeds
@@ -406,7 +406,7 @@ func (s *Service) Feedback(ctx context.Context, repo string, pr int) (FeedbackRe
 		if _, ok := coEventKinds[comment.ID]; ok {
 			continue
 		}
-		if s.isConfiguredBot(comment.User.Login) {
+		if s.cfg.isConfiguredBot(comment.User.Login) {
 			continue
 		}
 		if dialect.IsNonActionableText(comment.Body) {

internal/crq/observe.go

@@ -31,7 +31,7 @@ type observation struct {
 // round anchors the round-relative facts: reactions target its fired command,
 // the adoption cutoff is its LastAttemptAt, and reactions/thumbs-up are fetched
 // only for a round that has fired.
-func (s *Service) observe(ctx context.Context, repo string, pr int, round *Round, now time.Time) (observation, error) {
+func (s *Service) observe(ctx context.Context, cfg Config, repo string, pr int, round *Round, now time.Time) (observation, error) {
 	pull, err := s.gh.GetPull(ctx, repo, pr)
 	if err != nil {
 		return observation{}, err
@@ -68,7 +68,7 @@ func (s *Service) observe(ctx context.Context, repo string, pr int, round *Round
 		// filter to the configured reviewer — only CodeRabbit posts these
 		// carriers, and dropping another bot's empty review could discard real
 		// evidence a Codex-gated round waits on.
-		if s.isConfiguredBot(review.User.Login) && strings.TrimSpace(review.Body) == "" && strings.EqualFold(review.State, "COMMENTED") {
+		if cfg.isConfiguredBot(review.User.Login) && strings.TrimSpace(review.Body) == "" && strings.EqualFold(review.State, "COMMENTED") {
 			continue
 		}
 		o.eng.Reviews = append(o.eng.Reviews, engine.ReviewSeen{
@@ -86,9 +86,9 @@ func (s *Service) observe(ctx context.Context, repo string, pr int, round *Round
 	o.comments = comments
 	classifier := dialect.Classifier{
 		CodeRabbit:    s.cr,
-		Bot:           s.cfg.Bot,
-		ReviewCommand: s.cfg.ReviewCommand,
-		CoReviewers:   s.classifierCoReviewers(),
+		Bot:           cfg.Bot,
+		ReviewCommand: cfg.ReviewCommand,
+		CoReviewers:   cfg.classifierCoReviewers(),
 	}
 	for _, c := range comments {
 		o.eng.Events = append(o.eng.Events, classifier.Classify(c.User.Login, c.Body, c.ID, c.CreatedAt, c.UpdatedAt))
@@ -101,7 +101,7 @@ func (s *Service) observe(ctx context.Context, repo string, pr int, round *Round
 	// wait rather than failing the observation; the log line is the operator's
 	// signal that a required check-bearing bot may time out spuriously.
 	checksUnknown := false
-	if o.eng.Open && pull.Head.SHA != "" && s.coChecksRelevant() {
+	if o.eng.Open && pull.Head.SHA != "" && cfg.coChecksRelevant() {
 		runs, cerr := s.gh.ListCheckRuns(ctx, repo, pull.Head.SHA)
 		if cerr != nil {
 			// Record the uncertainty rather than letting "no checks returned"
@@ -114,7 +114,7 @@ func (s *Service) observe(ctx context.Context, repo string, pr int, round *Round
 		} else {
 			for _, run := range runs {
 				login, verdict := dialect.ClassifyCheckRun(run.App.Slug, run.Name, run.Output.Title, run.Output.Summary, run.Status, run.Conclusion)
-				if verdict == dialect.CheckUnrelated || !s.coBotEnabled(login) {
+				if verdict == dialect.CheckUnrelated || !cfg.coBotEnabled(login) {
 					continue
 				}
 				o.eng.Checks = append(o.eng.Checks, engine.CheckSeen{Bot: login, Name: run.Name, Verdict: verdict, CompletedAt: run.CompletedAt})
@@ -131,15 +131,15 @@ func (s *Service) observe(ctx context.Context, repo string, pr int, round *Round
 				return observation{}, err
 			}
 			for _, reaction := range reactions {
-				if s.isConfiguredBot(reaction.User.Login) {
+				if cfg.isConfiguredBot(reaction.User.Login) {
 					o.eng.Reacted = true
 				}
 				if isCurrentCodexThumbsUp(reaction, cutoff) {
 					o.eng.CodexThumbsUp = true
 				}
 			}
 		}
-		if !o.eng.CodexThumbsUp && s.codexRelevant(o.eng) {
+		if !o.eng.CodexThumbsUp && cfg.codexRelevant(o.eng) {
 			reactions, err := s.gh.ListIssueReactions(ctx, repo, pr)
 			if err != nil {
 				return observation{}, err
@@ -156,9 +156,9 @@ func (s *Service) observe(ctx context.Context, repo string, pr int, round *Round
 	// Co-reviewer activity is derived from the same snapshot: whether each bot
 	// reviews the PR unprompted (drives the fire decision) and whether it
 	// participates in the current round (drives the dynamic completion gate).
-	if len(s.cfg.CoBots) > 0 {
+	if len(cfg.CoBots) > 0 {
 		o.eng.Co = map[string]engine.CoSeen{}
-		for _, cb := range s.cfg.CoBots {
+		for _, cb := range cfg.CoBots {
 			seen := engine.CoSeen{AutoActive: engine.CoAutoActive(o.eng, cb.Login)}
 			if co, ok := dialect.CoReviewerByName(cb.Name); ok && co.AppSlug != "" {
 				seen.ChecksUnknown = checksUnknown
@@ -172,7 +172,7 @@ func (s *Service) observe(ctx context.Context, repo string, pr int, round *Round
 
 	// Adoptable commands are only consulted for a fire-eligible round.
 	if round != nil && round.FireEligible(now) {
-		cr, co, err := s.reviewCommands(ctx, repo, pr, o.eng, adoptCutoff(*round), pull, comments, reviews)
+		cr, co, err := s.reviewCommands(ctx, cfg, repo, pr, o.eng, adoptCutoff(*round), pull, comments, reviews)
 		if err != nil {
 			return observation{}, err
 		}
@@ -189,9 +189,9 @@ func (s *Service) observe(ctx context.Context, repo string, pr int, round *Round
 
 // classifierCoReviewers resolves the enabled registry entries with their
 // config-resolved trigger commands.
-func (s *Service) classifierCoReviewers() []dialect.CoReviewer {
-	out := make([]dialect.CoReviewer, 0, len(s.cfg.CoBots))
-	for _, cb := range s.cfg.CoBots {
+func (c Config) classifierCoReviewers() []dialect.CoReviewer {
+	out := make([]dialect.CoReviewer, 0, len(c.CoBots))
+	for _, cb := range c.CoBots {
 		co, ok := dialect.CoReviewerByName(cb.Name)
 		if !ok {
 			continue
@@ -205,8 +205,8 @@ func (s *Service) classifierCoReviewers() []dialect.CoReviewer {
 // coChecksRelevant reports whether any enabled co-reviewer owns check runs,
 // so the extra REST fetch (ETag'd — repeat polls are 304s) is only spent when
 // a bot's evidence can live there.
-func (s *Service) coChecksRelevant() bool {
-	for _, cb := range s.cfg.CoBots {
+func (c Config) coChecksRelevant() bool {
+	for _, cb := range c.CoBots {
 		if co, ok := dialect.CoReviewerByName(cb.Name); ok && co.AppSlug != "" {
 			return true
 		}
@@ -215,8 +215,8 @@ func (s *Service) coChecksRelevant() bool {
 }
 
 // coBotEnabled reports whether login is one of the enabled co-reviewers.
-func (s *Service) coBotEnabled(login string) bool {
-	for _, cb := range s.cfg.CoBots {
+func (c Config) coBotEnabled(login string) bool {
+	for _, cb := range c.CoBots {
 		if dialect.NormalizeBotName(cb.Login) == dialect.NormalizeBotName(login) {
 			return true
 		}
@@ -236,8 +236,8 @@ func adoptCutoff(r Round) time.Time {
 
 // codexRelevant reports whether Codex participates in this round, so the extra
 // issue-reactions fetch for a Codex thumbs-up is only spent when it can matter.
-func (s *Service) codexRelevant(obs engine.Observation) bool {
-	if dialect.HasCodexBot(s.cfg.RequiredBots) {
+func (c Config) codexRelevant(obs engine.Observation) bool {
+	if dialect.HasCodexBot(c.RequiredBots) {
 		return true
 	}
 	for _, review := range obs.Reviews {
@@ -260,10 +260,10 @@ func (s *Service) codexRelevant(obs engine.Observation) bool {
 // computation (LastAttemptAt floor, head-commit date, force-push) so a stale
 // command from a previous head is excluded everywhere, and the head-guard/
 // cutoff lookups are skipped entirely when no command is on the PR.
-func (s *Service) reviewCommands(ctx context.Context, repo string, pr int, obs engine.Observation, notBeforeCutoff time.Time, pull ghapi.Pull, comments []ghapi.IssueComment, reviews []ghapi.Review) (cr []engine.CommandSeen, co map[string][]engine.CommandSeen, err error) {
-	command := strings.TrimSpace(s.cfg.ReviewCommand)
+func (s *Service) reviewCommands(ctx context.Context, cfg Config, repo string, pr int, obs engine.Observation, notBeforeCutoff time.Time, pull ghapi.Pull, comments []ghapi.IssueComment, reviews []ghapi.Review) (cr []engine.CommandSeen, co map[string][]engine.CommandSeen, err error) {
+	command := strings.TrimSpace(cfg.ReviewCommand)
 	hasCR := command != "" && hasCommentBody(comments, command)
-	coBodies := s.coCommandBodies()
+	coBodies := cfg.coCommandBodies()
 	present := map[string][]string{}
 	for key, bodies := range coBodies {
 		for _, body := range bodies {
@@ -310,7 +310,7 @@ func (s *Service) reviewCommands(ctx context.Context, repo string, pr int, obs e
 		cutoff = fp
 	}
 	if hasCR {
-		cr = s.adoptableCR(obs, cutoff, command, comments, reviews)
+		cr = s.adoptableCR(cfg, obs, cutoff, command, comments, reviews)
 	}
 	for key, bodies := range present {
 		if cmds := adoptableCo(obs, key, cutoff, bodies, comments, reviews); len(cmds) > 0 {
@@ -326,9 +326,9 @@ func (s *Service) reviewCommands(ctx context.Context, repo string, pr int, obs e
 // coCommandBodies maps each triggerable co-reviewer (normalized login) to the
 // comment bodies that count as its trigger: the config-resolved command plus
 // the registry's alternate spellings (`bugbot run` / `cursor review`).
-func (s *Service) coCommandBodies() map[string][]string {
+func (c Config) coCommandBodies() map[string][]string {
 	out := map[string][]string{}
-	for _, cb := range s.cfg.CoBots {
+	for _, cb := range c.CoBots {
 		var bodies []string
 		add := func(body string) {
 			body = strings.TrimSpace(body)
@@ -399,7 +399,7 @@ func adoptableCo(obs engine.Observation, loginKey string, cutoff time.Time, bodi
 // already-posted fire, or none. A command the bot already answered with a review
 // or a completion reply belongs to a finished round for an earlier head and is
 // never adopted (adopting it would mark the new head fired without reviewing it).
-func (s *Service) adoptableCR(obs engine.Observation, cutoff time.Time, command string, comments []ghapi.IssueComment, reviews []ghapi.Review) []engine.CommandSeen {
+func (s *Service) adoptableCR(cfg Config, obs engine.Observation, cutoff time.Time, command string, comments []ghapi.IssueComment, reviews []ghapi.Review) []engine.CommandSeen {
 	best := newestCommandSince(command, cutoff, comments)
 	if len(best) == 0 {
 		return nil
@@ -409,11 +409,11 @@ func (s *Service) adoptableCR(obs engine.Observation, cutoff time.Time, command
 		bestAt = best[0].UpdatedAt
 	}
 	for _, review := range reviews {
-		if s.isConfiguredBot(review.User.Login) && !review.SubmittedAt.Before(bestAt) {
+		if cfg.isConfiguredBot(review.User.Login) && !review.SubmittedAt.Before(bestAt) {
 			return nil
 		}
 	}
-	if engine.CommandHasCompletionReply(obs, s.policy(), best[0].ID) {
+	if engine.CommandHasCompletionReply(obs, cfg.policy(), best[0].ID) {
 		return nil
 	}
 	return best

internal/crq/observe_config_test.go

@@ -0,0 +1,42 @@
+package crq
+
+import (
+	"strings"
+	"testing"
+)
+
+// observe must consult the configuration it was HANDED, not the Service's own —
+// otherwise a per-repository setting is read at the top and quietly ignored
+// underneath: check runs for a bot this repo enabled would be fetched and then
+// discarded, and a repo-specific review command would be missed and reposted.
+//
+// A grep is the honest test here. The bug is a whole class of site, not one
+// behaviour, and every one of them compiles and passes today.
+func TestObserveUsesOnlyTheConfigItWasGiven(t *testing.T) {
+	// Scoped to observe itself. Checking the whole file would fail on an
+	// unrelated method, or on the words appearing in a comment.
+	body, ok := funcBody(readSource(t, "internal/crq/observe.go"), "func (s *Service) observe(")
+	if !ok {
+		t.Fatal("observe not found; this test is checking the wrong thing")
+	}
+	if !strings.Contains(body, "cfg Config") {
+		t.Fatal("observe no longer takes a Config; this test is checking the wrong thing")
+	}
+	if n := strings.Count(body, "s.cfg."); n != 0 {
+		t.Errorf("observe reaches for the Service configuration %d times; it must use the cfg parameter", n)
+	}
+}
+
+// funcBody returns the source of the function whose signature starts with
+// prefix, from the signature to the closing brace at column 0.
+func funcBody(source, prefix string) (string, bool) {
+	start := strings.Index(source, prefix)
+	if start < 0 {
+		return "", false
+	}
+	rest := source[start:]
+	if end := strings.Index(rest, "\n}\n"); end >= 0 {
+		return rest[:end], true
+	}
+	return rest, true
+}

internal/crq/reviewers.go

@@ -168,3 +168,10 @@ func silenceTrigger(coBots []CoBotConfig, login string) []CoBotConfig {
 	}
 	return out
 }
+
+// evidenceBots is the set whose output crq reads: everyone whose findings are
+// surfaced, plus everyone it waits for. The two must never diverge — a bot crq
+// gates on whose findings it did not surface would hang the round forever.
+func (c Config) evidenceBots() map[string]struct{} {
+	return dialect.BotSet(unionBots(c.FeedbackBots, c.RequiredBots))
+}