From b0493e6429994dda376749b1c29c043f82a2a5c0 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 18:22:43 +0200 Subject: [PATCH 1/3] Make the reviewer configuration a value, not a field MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Who reviews a PR was read straight off the Service: s.cfg.CoBots, s.cfg.RequiredBots, s.cfg.FeedbackBots, and a policy() built from them. That is fine while every repository has the same answer, and it is the one thing that has to stop being true — "which bots you want for which project" cannot be a property of the process. So the reads become functions of a Config value: policy, the classifier's co-reviewer list, the check-run and Codex relevance probes, the trigger command lookups, and the evidence set. observe takes the configuration it should use as a parameter rather than reaching for the Service's own. No behaviour change — every caller passes s.cfg, which is what it read before. What changes is that passing something else is now possible, and that the union of feedback and required bots has one definition instead of two copies that had to stay equal. --- internal/crq/codex_replay_test.go | 4 +-- internal/crq/feedback.go | 10 +++---- internal/crq/observe.go | 44 +++++++++++++++---------------- internal/crq/reviewers.go | 7 +++++ internal/crq/service.go | 32 +++++++++++----------- internal/crq/state.go | 16 +++++------ 6 files changed, 60 insertions(+), 53 deletions(-) diff --git a/internal/crq/codex_replay_test.go b/internal/crq/codex_replay_test.go index fc84fe7..5373d08 100644 --- a/internal/crq/codex_replay_test.go +++ b/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) } diff --git a/internal/crq/feedback.go b/internal/crq/feedback.go index 85d81b1..12d007a 100644 --- a/internal/crq/feedback.go +++ b/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. diff --git a/internal/crq/observe.go b/internal/crq/observe.go index e571585..7d1e046 100644 --- a/internal/crq/observe.go +++ b/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 @@ -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 || !s.cfg.coBotEnabled(login) { continue } o.eng.Checks = append(o.eng.Checks, engine.CheckSeen{Bot: login, Name: run.Name, Verdict: verdict, CompletedAt: run.CompletedAt}) @@ -139,7 +139,7 @@ func (s *Service) observe(ctx context.Context, repo string, pr int, round *Round } } } - 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 @@ -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 { @@ -263,7 +263,7 @@ func (s *Service) codexRelevant(obs engine.Observation) bool { 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) hasCR := command != "" && hasCommentBody(comments, command) - coBodies := s.coCommandBodies() + coBodies := s.cfg.coCommandBodies() present := map[string][]string{} for key, bodies := range coBodies { for _, body := range bodies { @@ -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) @@ -413,7 +413,7 @@ func (s *Service) adoptableCR(obs engine.Observation, cutoff time.Time, command return nil } } - if engine.CommandHasCompletionReply(obs, s.policy(), best[0].ID) { + if engine.CommandHasCompletionReply(obs, s.cfg.policy(), best[0].ID) { return nil } return best diff --git a/internal/crq/reviewers.go b/internal/crq/reviewers.go index 6ee95f3..bcfe7df 100644 --- a/internal/crq/reviewers.go +++ b/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)) +} diff --git a/internal/crq/service.go b/internal/crq/service.go index fb33c75..98886e2 100644 --- a/internal/crq/service.go +++ b/internal/crq/service.go @@ -298,12 +298,12 @@ func (s *Service) Pump(ctx context.Context) (PumpResult, error) { if next == nil { return PumpResult{Action: "idle"}, nil } - obs, err := s.observe(ctx, next.Repo, next.PR, next, now) + obs, err := s.observe(ctx, s.cfg, next.Repo, next.PR, next, now) if err != nil { return PumpResult{}, err } global := s.global(st, now) - decision := engine.DecideFire(global, *next, obs.eng, now, s.policy()) + decision := engine.DecideFire(global, *next, obs.eng, now, s.cfg.policy()) result, err := s.applyFire(ctx, *next, obs.eng, decision, now) if err != nil { return result, err @@ -344,7 +344,7 @@ func (s *Service) sweepQuotaFree(ctx context.Context, st State, now time.Time, s if len(queued) == 0 { return PumpResult{}, false, nil } - policy := s.policy() + policy := s.cfg.policy() global := s.global(st, now) scanned := 0 defer func() { s.scanOffset = (s.scanOffset + scanned + 1) % len(queued) }() @@ -363,7 +363,7 @@ func (s *Service) sweepQuotaFree(ctx context.Context, st State, now time.Time, s // left them behind the account block for hours. The budget above bounds // the cost instead. scanned++ - obs, err := s.observe(ctx, round.Repo, round.PR, &round, now) + obs, err := s.observe(ctx, s.cfg, round.Repo, round.PR, &round, now) if err != nil { continue } @@ -399,11 +399,11 @@ func (s *Service) advanceQuotaFree(ctx context.Context, repo string, pr int) (Pu if round == nil || !round.FireEligible(now) { return PumpResult{}, false, nil } - obs, err := s.observe(ctx, repo, pr, round, now) + obs, err := s.observe(ctx, s.cfg, repo, pr, round, now) if err != nil { return PumpResult{}, false, err } - d := engine.DecideFire(s.global(st, now), *round, obs.eng, now, s.policy()) + d := engine.DecideFire(s.global(st, now), *round, obs.eng, now, s.cfg.policy()) if !quotaFreeVerdict(d.Verdict) { return PumpResult{}, false, nil } @@ -487,12 +487,12 @@ func (s *Service) progressSlotRound(ctx context.Context, slot Round) (PumpResult if err != nil { return PumpResult{}, err } - obs, err := s.observe(ctx, slot.Repo, slot.PR, &slot, now) + obs, err := s.observe(ctx, s.cfg, slot.Repo, slot.PR, &slot, now) if err != nil { return PumpResult{}, err } s.selfHealCoReviewers(ctx, slot, obs.eng, now) - tr := engine.Progress(slot, st.Account, obs.eng, now, s.policy()) + tr := engine.Progress(slot, st.Account, obs.eng, now, s.cfg.policy()) if tr.Outcome == engine.KeepWaiting { return PumpResult{Action: "waiting", Repo: slot.Repo, PR: slot.PR, Reason: tr.Reason}, nil } @@ -626,7 +626,7 @@ func (s *Service) sweepReviewing(ctx context.Context, st State, now time.Time) ( if target == nil { return st, nil } - obs, err := s.observe(ctx, target.Repo, target.PR, target, now) + obs, err := s.observe(ctx, s.cfg, target.Repo, target.PR, target, now) if err != nil { if s.log != nil { s.log.Printf("warning: reviewing-round sweep for %s#%d failed: %v", target.Repo, target.PR, err) @@ -634,7 +634,7 @@ func (s *Service) sweepReviewing(ctx context.Context, st State, now time.Time) ( return st, nil } s.selfHealCoReviewers(ctx, *target, obs.eng, now) - tr := engine.Progress(*target, st.Account, obs.eng, now, s.policy()) + tr := engine.Progress(*target, st.Account, obs.eng, now, s.cfg.policy()) if tr.Outcome == engine.KeepWaiting { return st, nil } @@ -834,7 +834,7 @@ func (s *Service) fireRound(ctx context.Context, round Round, obs engine.Observa // posting a duplicate; recording it here keeps the round "asked". Its // timestamp anchors that bot's cutoff too, or a SHA-less answer that // landed before this adopted fire would never bind to the round. - for _, cp := range s.policy().CoReviewerPolicies() { + for _, cp := range s.cfg.policy().CoReviewerPolicies() { if hasLogin(postCo, cp.Login) || r.Co(cp.Login).CommandID != 0 { continue } @@ -954,8 +954,8 @@ func hasLogin(logins []string, login string) bool { } // coCommandFor resolves the trigger comment body crq posts for login. -func (s *Service) coCommandFor(login string) string { - for _, cb := range s.cfg.CoBots { +func (c Config) coCommandFor(login string) string { + for _, cb := range c.CoBots { if dialect.NormalizeBotName(cb.Login) == dialect.NormalizeBotName(login) { return cb.Command } @@ -1151,7 +1151,7 @@ func (s *Service) fireCoReviewWait(ctx context.Context, round Round, obs engine. at time.Time } var adopts []adoptCmd - for _, cp := range s.policy().CoReviewerPolicies() { + for _, cp := range s.cfg.policy().CoReviewerPolicies() { cmds := obs.CoSeenFor(cp.Login).Commands id := newestCommandID(cmds) if id == 0 { @@ -1255,7 +1255,7 @@ func (s *Service) recordFire(ctx context.Context, round Round, token string, com // command unset so a later pump's self-heal retries. The fresh-fire path // folds the returned id into recordFire's write. func (s *Service) postCoTrigger(ctx context.Context, round Round, login string) (int64, time.Time) { - command := strings.TrimSpace(s.coCommandFor(login)) + command := strings.TrimSpace(s.cfg.coCommandFor(login)) if command == "" { return 0, time.Time{} } @@ -1423,7 +1423,7 @@ func (s *Service) selfHealCoReviewers(ctx context.Context, round Round, obs engi return } firedAt := round.FiredAt.UTC() - for _, cp := range s.policy().CoReviewerPolicies() { + for _, cp := range s.cfg.policy().CoReviewerPolicies() { if round.Co(cp.Login).CommandID != 0 || (dialect.IsCodexBot(cp.Login) && round.CodexCommandID != 0) { continue } diff --git a/internal/crq/state.go b/internal/crq/state.go index d6cbfc3..a9da1d2 100644 --- a/internal/crq/state.go +++ b/internal/crq/state.go @@ -111,16 +111,16 @@ func issueBody(st State, cfg Config) (string, error) { } // policy assembles the engine Policy from config. -func (s *Service) policy() engine.Policy { +func (c Config) policy() engine.Policy { p := engine.Policy{ - Bot: s.cfg.Bot, - RequiredBots: s.cfg.RequiredBots, - MinInterval: s.cfg.MinInterval, - InflightTimeout: s.cfg.InflightTimeout, - RateLimitFallback: s.cfg.RateLimitFallback, - RateLimitCoDegrade: s.cfg.RateLimitCoDegrade, + Bot: c.Bot, + RequiredBots: c.RequiredBots, + MinInterval: c.MinInterval, + InflightTimeout: c.InflightTimeout, + RateLimitFallback: c.RateLimitFallback, + RateLimitCoDegrade: c.RateLimitCoDegrade, } - for _, cb := range s.cfg.CoBots { + for _, cb := range c.CoBots { p.CoReviewers = append(p.CoReviewers, engine.CoReviewerPolicy{ Login: cb.Login, Command: cb.Command, From 1ed0ed95f987f6b102569e493baef750012dd746 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 19:02:34 +0200 Subject: [PATCH 2/3] Use the config observe was handed, all the way down MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The parameter reached the top of observe and stopped there. Underneath, the review-shell and reaction filters still asked s.isConfiguredBot, check runs were filtered with s.cfg.coBotEnabled, and reviewCommands searched and adopted commands using s.cfg. That is worse than not taking the parameter at all. A repository that enables Bugbot while the fleet does not would have its check runs fetched and then discarded — so crq triggers a bot already running, or times out waiting for evidence it threw away — and a repo-specific review command would be missed and posted again. isConfiguredBot becomes a Config method, and reviewCommands/adoptableCR take the configuration too. A grep-shaped test keeps observe.go at zero reads of the Service configuration: the bug is a class of site, not one behaviour, and every instance of it compiled and passed. --- internal/crq/feedback.go | 6 +++--- internal/crq/observe.go | 22 ++++++++++----------- internal/crq/observe_config_test.go | 23 ++++++++++++++++++++++ internal/crq/service.go | 8 ++++---- internal/crq/service_test.go | 2 +- internal/crq/source_test.go | 30 +++++++++++++++++++++++++++++ 6 files changed, 72 insertions(+), 19 deletions(-) create mode 100644 internal/crq/observe_config_test.go create mode 100644 internal/crq/source_test.go diff --git a/internal/crq/feedback.go b/internal/crq/feedback.go index 12d007a..c2722bf 100644 --- a/internal/crq/feedback.go +++ b/internal/crq/feedback.go @@ -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) { diff --git a/internal/crq/observe.go b/internal/crq/observe.go index 7d1e046..409cda3 100644 --- a/internal/crq/observe.go +++ b/internal/crq/observe.go @@ -68,7 +68,7 @@ func (s *Service) observe(ctx context.Context, cfg Config, repo string, pr int, // 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{ @@ -114,7 +114,7 @@ func (s *Service) observe(ctx context.Context, cfg Config, repo string, pr int, } 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.cfg.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,7 +131,7 @@ func (s *Service) observe(ctx context.Context, cfg Config, repo string, pr int, 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) { @@ -172,7 +172,7 @@ func (s *Service) observe(ctx context.Context, cfg Config, repo string, pr int, // 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 } @@ -260,10 +260,10 @@ func (c Config) 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.cfg.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 { @@ -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.cfg.policy(), best[0].ID) { + if engine.CommandHasCompletionReply(obs, cfg.policy(), best[0].ID) { return nil } return best diff --git a/internal/crq/observe_config_test.go b/internal/crq/observe_config_test.go new file mode 100644 index 0000000..72b68bd --- /dev/null +++ b/internal/crq/observe_config_test.go @@ -0,0 +1,23 @@ +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) { + source := readSource(t, "internal/crq/observe.go") + if n := strings.Count(source, "s.cfg."); n != 0 { + t.Errorf("observe.go reaches for the Service configuration %d times; it must use the cfg parameter", n) + } + if !strings.Contains(source, "func (s *Service) observe(ctx context.Context, cfg Config,") { + t.Error("observe no longer takes a Config; this test is checking the wrong thing") + } +} diff --git a/internal/crq/service.go b/internal/crq/service.go index 98886e2..4a6feac 100644 --- a/internal/crq/service.go +++ b/internal/crq/service.go @@ -1692,7 +1692,7 @@ func (s *Service) isCalibrationNoise(c ghapi.IssueComment) bool { if strings.TrimSpace(c.Body) == strings.TrimSpace(s.cfg.RateLimitCommand) { return true } - return s.isConfiguredBot(c.User.Login) && strings.Contains(c.Body, s.cfg.CalibrationMarker) + return s.cfg.isConfiguredBot(c.User.Login) && strings.Contains(c.Body, s.cfg.CalibrationMarker) } func (s *Service) latestCalibrationReply(ctx context.Context, issue int, after time.Time) (ghapi.IssueComment, bool, error) { @@ -1703,7 +1703,7 @@ func (s *Service) latestCalibrationReply(ctx context.Context, issue int, after t var best ghapi.IssueComment ok := false for _, comment := range comments { - if !s.isConfiguredBot(comment.User.Login) || !comment.UpdatedAt.After(after) { + if !s.cfg.isConfiguredBot(comment.User.Login) || !comment.UpdatedAt.After(after) { continue } if !strings.Contains(comment.Body, s.cfg.CalibrationMarker) { @@ -1733,8 +1733,8 @@ func reviewedByConfiguredBot(reviewedBy map[string]bool, bot string) bool { return false } -func (s *Service) isConfiguredBot(login string) bool { - return dialect.NormalizeBotName(login) == dialect.NormalizeBotName(s.cfg.Bot) +func (c Config) isConfiguredBot(login string) bool { + return dialect.NormalizeBotName(login) == dialect.NormalizeBotName(c.Bot) } // notBefore reports whether t is at or after baseline. GitHub timestamps are diff --git a/internal/crq/service_test.go b/internal/crq/service_test.go index 59aba22..715abb3 100644 --- a/internal/crq/service_test.go +++ b/internal/crq/service_test.go @@ -842,7 +842,7 @@ func TestAdoptableCommandsRequiresExpectedHead(t *testing.T) { comments, _ := gh.ListIssueComments(ctx, "owner/repo", 12) reviews, _ := gh.ListReviews(ctx, "owner/repo", 12) - cmds, _, err := service.reviewCommands(ctx, "owner/repo", 12, engine.Observation{Head: "abcdef123", Open: true}, time.Time{}, pull, comments, reviews) + cmds, _, err := service.reviewCommands(ctx, service.cfg, "owner/repo", 12, engine.Observation{Head: "abcdef123", Open: true}, time.Time{}, pull, comments, reviews) if err != nil || len(cmds) != 0 { t.Fatalf("must not adopt a review command after the PR head changed, cmds=%v err=%v", cmds, err) } diff --git a/internal/crq/source_test.go b/internal/crq/source_test.go new file mode 100644 index 0000000..7590534 --- /dev/null +++ b/internal/crq/source_test.go @@ -0,0 +1,30 @@ +package crq + +import ( + "os" + "path/filepath" + "testing" +) + +// readSource returns a repository file's contents, for the few tests whose +// subject is the shape of the code rather than its behaviour. +func readSource(t *testing.T, rel string) string { + t.Helper() + dir, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + for i := 0; i < 5; i++ { + path := filepath.Join(dir, rel) + if data, err := os.ReadFile(path); err == nil { + return string(data) + } + parent := filepath.Dir(dir) + if parent == dir { + break + } + dir = parent + } + t.Fatalf("could not find %s from %s", rel, dir) + return "" +} From 9853acf120b2782aa6bb9fe5a9ed9f0a948be75d Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 21:27:35 +0200 Subject: [PATCH 3/3] Scope the config-threading guard to observe itself Counting s.cfg. across the whole file would fail on an unrelated method, or on the words appearing in a comment, so the guard would break for reasons that have nothing to do with what it protects. It reads the function's own body now. --- internal/crq/observe_config_test.go | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/internal/crq/observe_config_test.go b/internal/crq/observe_config_test.go index 72b68bd..ed882f5 100644 --- a/internal/crq/observe_config_test.go +++ b/internal/crq/observe_config_test.go @@ -13,11 +13,30 @@ import ( // 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) { - source := readSource(t, "internal/crq/observe.go") - if n := strings.Count(source, "s.cfg."); n != 0 { - t.Errorf("observe.go reaches for the Service configuration %d times; it must use the cfg parameter", n) + // 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(source, "func (s *Service) observe(ctx context.Context, cfg Config,") { - t.Error("observe no longer takes a Config; 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 }