feat: arm GitHub-native auto-merge for labeled trusted-author PRs

[bborbe]

What

Adds the per-PR auto-merge opt-in for the end-to-end agent PR flow: when a trusted author's PR carries the auto-merge label, the watcher arms GitHub-native auto-merge via EnableAutoMerge (PUT /repos/{owner}/{repo}/pulls/{n}/auto-merge). GitHub then merges once required checks + required reviews are green.

Why

Task [[Add Per-PR autoMergeOnGreen for End-to-End Agent PR Flow]]: agent PRs currently stall on a human merge click. This is the watcher half of the label+arms mechanism (agent half: github-update-go-agent applies the label at PR creation).

Changes

• pkg/githubclient.go: add EnableAutoMerge to the GitHubClient interface + impl (raw REST, merge_method=merge); regenerate mock
• pkg/watcher.go: tryAutoMerge gate (label present + trusted author), called as a side effect in processPR — review path unchanged; new AUTO_MERGE_LABEL config threaded via factory
• main.go: AUTO_MERGE_LABEL env (default auto-merge)
• Tests: 5 new specs (arms for trusted + labeled, skips untrusted, skips unlabeled, disabled when empty, review still emitted)

Never-merge boundary

Held: the watcher arms auto-merge; the agent never calls gh pr merge. This runs outside the agent's tool scope (see github-update-go-agent docs/design.md § 7.0 decision 2026-08-18).

Precondition (deploy-time, not code)

The watcher GitHub App (id 3798945) is currently Contents: Read only (spec 037). Arming auto-merge requires Pull requests: Write — the App permission must be bumped before this is deployed.

[ben-s-pull-request-reviewer]

Now I have all I need. Let me compile the full review. The mechanical funnel findings are pre-confirmed MUST-tier from the ast-grep runner. I'll now produce the consolidated report.

Selector Mode — Step 4c-sel / 4d-sel: The diff is a Go project (.go files changed, no .py, .js, etc.). Changed files: pkg/watcher.go, pkg/githubclient.go, pkg/metrics.go, pkg/factory/factory.go, main.go, pkg/watcher_test.go, mocks/github_client.go, CHANGELOG.md, README.md. Judgment rules: None of the changed files match any judgment-rule trigger (no ADR/agent/command/docs patterns).

Mechanical findings (176 total from ast-grep-runner, pre-confirmed) map to the changed files. Now organizing by severity:

---

PR Review: feat: arm GitHub-native auto-merge for labeled trusted-author PRs

Must Fix (Critical)

• pkg/watcher_test.go — Ginkgo var _ = Describe(...) blocks exist but no *_suite_test.go with TestSuite + RunSpecs lives in pkg/. make test exits 0 with zero specs discovered. (rule: go-testing/suite-test-file-required)

• pkg/watcher_test.go — 50+ bare error-returning method calls inside BeforeEach/It/JustBeforeEach discard return values (e.g. ghClient.SearchPRsReturns(...), createSender.SendCommandReturns(...), ghClient.EnableAutoMergeReturns(nil), ghClient.GetPRDetailsReturns(...)). These are Counterfeiter-generated mock setter methods — all return error. Wrapping them in Expect(...).To(Succeed()) is required by the errcheck enforcement. (rule: go-testing/no-bare-error-call)

• pkg/watcher.go:386 — w.tryAutoMerge(ctx, pr) is called as a side-effect fire-and-forget. The returned bool (armed/unarmed) is discarded. While the design intent is correct (arm independently of review), this means the arming failure path leaves IncPRPublished("error") unverified — the caller cannot distinguish "armed" from "arm-failed-but-continued". This is acceptable only if the side-effect-only intent is documented in the godoc of processPR; currently tryAutoMerge's godoc states it returns false when arming fails, but the caller ignores it. Consider adding a comment in processPR explicitly acknowledging the discard, or store the result for the metrics call count assertion in tests. No action strictly required — the behavior is intentional — but the godoc at processPR should state the discard explicitly.

• pkg/githubclient.go:219 — EnableAutoMerge calls c.client.Do(ctx, req, nil) with no log statement summarizing the call outcome (method, path, status, latency). Boundary calls require an audit log line. (rule: go-logging/external-call-logs-response)

• pkg/factory/factory.go:30 — CreateGitHubAppClient is a Create*-prefixed factory returning ( *http.Client, error). Factories must not return error — composition belongs in main.go or behind a Provider interface. (rule: go-factory/no-error-return)

• pkg/githubclient.go (and others) — All 4 interfaces (Watcher, TaskPublisher, Metrics, GitHubClient) lack //counterfeiter:generate directives. (rule: go-architecture/counterfeiter-directive-on-interface)

• pkg/metrics.go:20 — pollCyclesTotal and prPublishedTotal are package-level var globals initialized via prometheus.NewCounterVec(...). Service dependencies must be injected via constructor. (rule: go-architecture/no-globals-or-singletons)

• pkg/watcher.go — Throughout the file, business logic calls glog.*, errors.Wrapf, slices.Contains, ctx.Done(), fmt.Sprintf directly as package-level functions. These hide dependencies. The entire watcher struct would benefit from injecting a Logger interface and a Formatter interface to make the logic testable without glog. (rule: go-composition/no-package-function-calls-in-business-logic) — applies to 50+ call sites.

• pkg/githubclient.go — Same pattern: fmt.Sprintf, errors.Wrapf, libtime.DateTime, issue.Get*() direct calls. (rule: go-composition/no-package-function-calls-in-business-logic)

• main.go:378-381 — Admin router registers only 4 of the 5 canonical endpoints: /healthz, /readiness, /metrics, /setloglevel/{level} — missing /gc. (rule: go-http-service/canonical-admin-endpoints)

• pkg/metrics.go:34,37 — for-loops pre-initialize metric labels without a ctx.Done() check. (rule: go-context/cancel-check-in-loop)

• pkg/githubclient.go:144,231 — Same missing ctx.Done() check in loops. (rule: go-context/cancel-check-in-loop)

• pkg/githubclient.go:120 — SearchPRs is a XxxList method (pagination) that iterates without checking ctx.Done() between pages. (rule: go-functional-composition/list-checks-ctx-done)

Should Fix (Important)

• pkg/watcher.go:17, main.go:30 — Both files import github.com/golang/glog. New Go projects should use log/slog. The project is mid-migration (glog is already present), so this is exempt from MUST — but it should be tracked for migration. (rule: go-cli/slog-not-glog-in-new-projects)

• main.go:183 — glog.Infof("watcher auth mode=github-app ...") is a bare V0/Info-level log at startup. If this is operator-facing startup info it belongs at V0, but the rule flags it for review — per project convention this should either be confirmed as V0-worthy or bumped to V(1). (rule: go-glog/use-v-for-debug-not-info)

• pkg/watcher.go:267,319 — glog.V(2).Infof unconditional heartbeat inside for loops at the production heartbeat level. When nothing changed on a poll cycle these produce log volume proportional to iteration frequency. Should be guarded with a "something happened" check or use a sampler. (rule: go-logging/skip-empty-v2-heartbeats)

• pkg/watcher.go:471-497 — tryAutoMerge trust errors call IncPRPublished("error") — this conflates arming failures with generic errors. The existing metric label set in metrics.go:35 pre-initializes "auto_merge" and "auto_merge_skipped" but NOT "error" for the auto-merge path. When EnableAutoMerge fails, IncPRPublished("error") is called — Prometheus will accept an unknown label but the metric label set won't pre-initialize it, making it invisible in dashboards until labeled. The auto_merge_skipped label covers the untrusted-author case but there's no dedicated label for arming failures. Consider using auto_merge_skipped for all failure modes (confusingly named) or adding auto_merge_error.

Nice to Have (Optional)

• pkg/watcher_test.go — Test helper newTestWatcher passes "" for autoMergeLabel and trust.NewAuthorAllowlist(nil) for trust — the old 2-param call site should be cleaned up to the new 4-param signature with a comment explaining why trustDecision can be nil for these tests.

• pkg/githubclient.go:200-205 — The comment on EnableAutoMerge says "merge_method is pinned to 'merge' to match the repo convention (bborbe repos are merge-commit-only)". This convention assumption should ideally be validated at startup or made configurable, but hardcoding is acceptable for a first implementation.

---

Traceability Report (Selector Mode)

• Candidates: 0 judgment rules matched the diff's trigger patterns
• Applicable: 0 rules
• Skipped: All judgment rules skipped — diff contains no agents/, commands/, skills/, docs/adr/, k8s/ files or architectural patterns triggering semantic rules
• Mechanical funnel: 176 findings across 5 rule-owners; all pre-confirmed MUST-tier findings included above

---

Concerns from Plan — Resolution Status

Concern	Status
security: EnableAutoMerge raw REST, no explicit Content-Type	Addressed — go-github's NewRequest sets Content-Type to application/json by default; the library handles it correctly
security: watcher App needs Pull requests: Write permission	Not fixed in code — README.md documents it; deployer must bump the GitHub App permission before deployment. Pre-condition documented, not a code defect.
correctness: EnableAutoMerge has no retry logic	By design — arming is idempotent at API level; re-arming on next poll is a success no-op
correctness: tryAutoMerge errors swallowed — IncPRPublished("error") called but Poll() returns no error	Acknowledged — this is intentional per design (side-effect only); consider documenting this in processPR godoc
correctness: autoMergeLabel checked before trust — no validation that label name is specific	Low risk — label name defaults to auto-merge which is already specific; a generic name would be a deployer misconfiguration
correctness: trustDecision passed to NewWatcher but only used in tryAutoMerge	Intentional — the trust decision gates the auto-merge arming; other trust checks in the codebase use the same trust.Trust interface. Could be more explicit in godoc.
tests: 5 new specs coverage looks solid	Confirmed — 5 specs covering trusted/untrusted/disabled/no-label/review-still-emitted

---

Next Steps

1. Must Fix — Create pkg/watcher_suite_test.go with Ginkgo TestSuite entry point
2. Must Fix — Wrap all mock setter calls in pkg/watcher_test.go with Expect(...).To(Succeed())
3. Must Fix — Add //counterfeiter:generate above every interface declaration
4. Must Fix — Add ctx.Done() checks in SearchPRs pagination loop and the metrics init loops
5. Must Fix — Add log line to EnableAutoMerge boundary call
6. Must Fix — Fix CreateGitHubAppClient factory error return pattern
7. Must Fix — Register /gc admin endpoint
8. Must Fix — Inject logger interface to eliminate glog.* package-level calls in business logic (50+ sites)
9. Should Fix — Migrate from glog to slog (can be follow-up)
10. Should Fix — Address V2 heartbeat logging in poll loops
11. Should Fix — Add auto_merge_error metric label or rename auto_merge_skipped to cover all failure modes

{
"verdict": "request-changes",
"summary": "Feature implementation is sound — auto-merge arming correctly gates on trusted-author + label, is idempotent, and does not skip review. However, the PR has 11 Must-Fix findings: missing Ginkgo suite entry point for watcher tests, 50+ bare error-returning mock calls in tests, missing counterfeiter directives, missing ctx.Done() in loops, unlogged boundary call, factory error-return violation, missing /gc admin endpoint, and 50+ package-level function calls in business logic that hide dependencies and prevent unit testing.",
"comments": [
{
"file": "pkg/watcher_test.go",
"line": 1,
"severity": "critical",
"message": "pkg/watcher_test.go has Ginkgo Describe blocks but no *_suite_test.go with TestSuite+RunSpecs in pkg/. make test exits 0 with zero specs discovered. Create pkg/watcher_suite_test.go. *(rule: go-testing/suite-test-file-required)*"
},
{
"file": "pkg/watcher_test.go",
"line": 166,
"severity": "critical",
"message": "ghClient.SearchPRsReturns(...) discards its error return inside a Ginkgo BeforeEach block. All 50+ mock setter calls (SearchPRsReturns, GetPRDetailsReturns, SendCommandReturns, EnableAutoMergeReturns, etc.) return error and must be wrapped: Expect(xxxReturns(...)).To(Succeed()). *(rule: go-testing/no-bare-error-call)*"
},
{
"file": "pkg/watcher.go",
"line": 23,
"severity": "critical",
"message": "Watcher interface lacks //counterfeiter:generate directive — mocks will drift silently when the interface changes. *(rule: go-architecture/counterfeiter-directive-on-interface)*"
},
{
"file": "pkg/watcher.go",
"line": 44,
"severity": "critical",
"message": "TaskPublisher interface lacks //counterfeiter:generate directive. *(rule: go-architecture/counterfeiter-directive-on-interface)*"
},
{
"file": "pkg/metrics.go",
"line": 11,
"severity": "critical",
"message": "Metrics interface lacks //counterfeiter:generate directive. *(rule: go-architecture/counterfeiter-directive-on-interface)*"
},
{
"file": "pkg/githubclient.go",
"line": 81,
"severity": "critical",
"message": "GitHubClient interface lacks //counterfeiter:generate directive. *(rule: go-architecture/counterfeiter-directive-on-interface)*"
},
{
"file": "pkg/githubclient.go",
"line": 120,
"severity": "critical",
"message": "SearchPRs is a paginated list method that iterates without checking ctx.Done() between pages — cancellation may not stop mid-way. Add select { case <-ctx.Done(): return ctx.Err(); default: } at loop top. *(rule: go-functional-composition/list-checks-ctx-done)*"
},
{
"file": "pkg/metrics.go",
"line": 34,
"severity": "critical",
"message": "for-loop pre-initializing metrics labels has no ctx.Done() check. *(rule: go-context/cancel-check-in-loop)*"
},
{
"file": "pkg/metrics.go",
"line": 37,
"severity": "critical",
"message": "for-loop pre-initializing metrics labels has no ctx.Done() check. *(rule: go-context/cancel-check-in-loop)*"
},
{
"file": "pkg/githubclient.go",
"line": 144,
"severity": "critical",
"message": "for-range over issues has no ctx.Done() check between iterations. *(rule: go-context/cancel-check-in-loop)*"
},
{
"file": "pkg/githubclient.go",
"line": 231,
"severity": "critical",
"message": "for-range over labels has no ctx.Done() check. *(rule: go-context/cancel-check-in-loop)*"
},
{
"file": "pkg/githubclient.go",
"line": 219,
"severity": "critical",
"message": "c.client.Do(ctx, req, nil) is a boundary call with no audit log line (method + path + status + latency). *(rule: go-logging/external-call-logs-response)*"
},
{
"file": "pkg/factory/factory.go",
"line": 30,
"severity": "critical",
"message": "CreateGitHubAppClient is a Create*-prefixed factory that returns (http.Client, error). Factories must not return error — composition belongs in main or behind a Provider interface. *(rule: go-factory/no-error-return)*"
},
{
"file": "main.go",
"line": 378,
"severity": "critical",
"message": "Admin router registers 4 of 5 canonical endpoints — /gc is missing. *(rule: go-http-service/canonical-admin-endpoints)*"
},
{
"file": "pkg/metrics.go",
"line": 20,
"severity": "critical",
"message": "pollCyclesTotal and prPublishedTotal are package-level var globals initialized via prometheus.NewCounterVec. Service dependencies must be injected via constructor, not package-level vars. *(rule: go-architecture/no-globals-or-singletons)*"
},
{
"file": "pkg/watcher.go",
"line": 99,
"severity": "major",
"message": "glog.Errorf called directly in method body — 50+ sites across watcher.go and githubclient.go. Business logic must inject a Logger interface and call through it. *(rule: go-composition/no-package-function-calls-in-business-logic)*"
},
{
"file": "pkg/watcher.go",
"line": 386,
"severity": "major",
"message": "tryAutoMerge return value (bool) is discarded in processPR. While intentional (side-effect only), the godoc of processPR should explicitly acknowledge this discard to prevent future refactors from silently changing behavior."
},
{
"file": "main.go",
"line": 183,
"severity": "major",
"message": "glog.Infof at V0 (always-on) used for startup auth info — flag for review: should this be V(1) debug or confirmed V0 operator info? *(rule: go-glog/use-v-for-debug-not-info)*"
},
{
"file": "pkg/watcher.go",
"line": 267,
"severity": "major",
"message": "Unconditional glog.V(2).Infof heartbeat inside for-loop — fires every poll cycle even when nothing happened. Should be guarded or use a sampler. *(rule: go-logging/skip-empty-v2-heartbeats)*"
},
{
"file": "pkg/watcher.go",
"line": 319,
"severity": "major",
"message": "Unconditional glog.V(2).Infof heartbeat inside for-range — fires per PR on every poll. *(rule: go-logging/skip-empty-v2-heartbeats)*"
},
{
"file": "pkg/watcher.go",
"line": 17,
"severity": "minor",
"message": "github.com/golang/glog imported in a Go 1.21+ project. Migration to log/slog is recommended. *(rule: go-cli/slog-not-glog-in-new-projects — exempt mid-migration but tracked)*"
},
{
"file": "main.go",
"line": 30,
"severity": "minor",
"message": "glog imported in main.go — same migration note as above. *(rule: go-cli/slog-not-glog-in-new-projects)*"
},
{
"file": "pkg/watcher.go",
"line": 471,
"severity": "minor",
"message": "tryAutoMerge calls IncPRPublished(\"error\") on arming failure but no dedicated auto_merge_error metric label is pre-initialized — arming failures will appear as unknown label in Prometheus. Consider using auto_merge_skipped for all non-armed cases or adding auto_merge_error."
}
],
"concerns_addressed": [
"security: EnableAutoMerge Content-Type — addressed: go-github NewRequest sets application/json by default",
"security: GitHub App permission bump — raised: deploy-time precondition documented in README, not a code defect",
"correctness: EnableAutoMerge no retry — addressed: idempotent at API level, intentional",
"correctness: tryAutoMerge errors swallowed — acknowledged: side-effect-only design, consider documenting the discard in processPR godoc",
"correctness: autoMergeLabel no validation — addressed: label name is specific by convention, low risk",
"correctness: trustDecision stored but only used for auto-merge — acknowledged: intentional per design",
"tests: 5 new specs solid coverage — confirmed"
]
}

[ben-s-pull-request-reviewer]

Override APPROVE by ben-s-pull-request-reviewer[bot] — the override-review label was applied by a trusted author. Posting APPROVE so the earlier false-positive review no longer blocks merge. CI status checks still apply.