fix(observability): right-size webhook log severities for oncall triage

[aparajon]

Why this matters

A severity-scanned log stream is the first tool an oncall operator reaches for, and today three webhook log sites bury it. A shared GitHub App installed org-wide forwards webhook deliveries for every repository it is installed on, so a deployment managing a handful of repos rejects a constant stream of deliveries for repos it does not manage — each one logged at warn as if it were config drift. The reconciler's report-only mode re-warns the same missing head on every pass. And the two paths that post a passing aggregate check log it under different messages, so a log search for one silently misses the other.

The rejection metric had the same conflation: routine unmanaged-repo traffic and genuine App-ownership drift counted under one status, so the drift signal could never be alerted on. The unmanaged case also recorded arbitrary repository names as a metric attribute — unbounded cardinality on a counter whose repository label is meant to be bounded by config.

What it does

webhook ownership rejection
  ├─ repo has no config entry (unmanaged traffic)
  │    └─ debug log + status "repo_not_configured", no repository attribute
  └─ declared repo, broken App mapping (drift / hostile install)
       └─ warn log + status "app_repo_mismatch", repository attribute kept

• ResolveGitHubAppForRepo wraps the new api.ErrRepoNotConfigured sentinel for undeclared repos so the webhook handler can separate the two causes.
• The reconciler's report-only missing-delivery line drops to info: it re-reports the same head on every pass, and the per-repo metric plus the per-pass summary already carry the operator signal.
• The merge-group path now logs "posted passing aggregate", matching the pull-request path, so one search covers both.

Two safety properties worth calling out:

• Rejection behavior is unchanged. Both ownership-rejection causes still fail closed with the same 401 and count on the events counter — only severity and status split.
• A nonzero app_repo_mismatch is now always actionable. With unmanaged traffic on its own status, the drift status can be alerted on directly, and the repository attribute stays bounded by configured repos.

How it moves us toward the northstar

Unattended fleet-scale GitOps needs an error/warn stream where every line is actionable and every safety-relevant metric can carry an alert. This clears the highest-volume warn sources and turns App-ownership drift — a trust-boundary signal — into something operators can alert on directly.

Opened by Claude (Fable 5).

[Copilot]

Pull request overview

This PR improves oncall triage signal quality by reducing non-actionable warn-level webhook logs, aligning passing-aggregate log messages across webhook paths, and splitting webhook rejection metrics so unmanaged-repo traffic can’t drown out (or inflate) actionable drift signals.

Changes:

• Split webhook ownership rejections into two causes (unmanaged repo vs configured repo with App mismatch), mapping them to different log levels and metric statuses while keeping fail-closed behavior unchanged.
• Downgrade reconciler report-only “missing delivery” per-head logging from warn to info to avoid repeated warn spam.
• Standardize the “posted passing aggregate” log message across pull-request and merge-group posting paths, and add targeted tests around the new ownership-rejection severity behavior.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
pkg/webhook/merge_group.go	Align passing-aggregate log message with the pull-request path for consistent log searching.
pkg/webhook/handler.go	Differentiate unmanaged-repo vs App-mismatch ownership rejections; adjust log severity and metrics labeling to preserve actionable signals and bounded cardinality.
pkg/webhook/durable_reconcile.go	Reduce report-only reconciler missing-delivery per-head log severity from warn to info.
pkg/webhook/dispatch_test.go	Add logger-injectable handler helper and a new test asserting the two ownership-rejection severities.
pkg/serve/serve.go	Update comments to reflect the “missing-delivery log” wording (severity change context).
pkg/metrics/metrics.go	Clarify (in comments) the contract for passing repo as empty when recording statuses for unmanaged repos.
pkg/api/config.go	Introduce ErrRepoNotConfigured sentinel and wrap it from ResolveGitHubAppForRepo for reliable cause detection via errors.Is.
pkg/api/config_test.go	Update resolver tests to assert ErrRepoNotConfigured classification and ensure declared-but-broken mappings don’t get misclassified as unmanaged.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for pull/1037, 0067898.

Verdict: 7 findings — 1 blocking (README metric contract), 2 non-blocking (operator doc, metric test coverage), 4 suggestions. CI 33/33 green at head 0067898 (state OPEN); GitHub still reports mergeable UNKNOWN, but merge-tree vs origin/main is clean — the 2 commits main is ahead touch only pkg/metrics/README.md and unrelated dispatch-supersede code.

Blocking

1. New metric statuses are missing from the documented schemabot.webhook.events_total contract. pkg/metrics/README.md:109 enumerates 13 statuses and includes neither repo_not_configured nor the now-alertable app_repo_mismatch, and :28 still lists repository as an unconditional attribute even though the PR deliberately omits it for repo_not_configured. An operator building the exact alert the PR body advertises consults this table and alerts on the wrong status, or assumes repo_not_configured series can be faceted by repository. Add both statuses and note the conditional attribute — this is the repo's recurring label-contract weak spot (fix(github): dead-letter webhook deliveries that can never succeed #1023, feat(observability): distinguish transport failures and 404s in GitHub request metrics #1024, feat(github): mirror remote control-request rejections back to the accepting plane #967).

Non-blocking

2. docs/configuration.md still describes a single ownership-rejection outcome. docs/configuration.md:1186 says a mismatch is "rejected with 401 and a app_repo_mismatch log line", but undeclared repos now reject as repo_not_configured at debug — invisible at the default LOG_LEVEL=info. An operator debugging constant 401s for an unmanaged repo searches the warn stream, finds nothing, and concludes the rejection comes from elsewhere; document both outcomes.

3. The metric half of the headline claim is untested. No test asserts repo_not_configured vs app_repo_mismatch status, the dropped repository attribute, or that both causes still increment events_total — pkg/webhook/dispatch_test.go:210 pins only log level/fields and the 401. A later refactor of the handler.go:676-692 branch could silently break the alerting contract or reintroduce unbounded repo names while CI stays green; extend the test using the manual-reader assertion infrastructure already in pkg/metrics tests.

General suggestions

4. The two now byte-identical "posted passing aggregate" log sites use divergent attribute keys. pkg/webhook/merge_group.go:323 logs head_sha/environment/operation while pkg/webhook/check_publisher.go:718 logs pr/env/action, so msg:"posted passing aggregate" env:production silently misses every merge-group posting. Unifying the key names would finish in the attribute dimension what this PR fixed in the message dimension.

5. The reconciler downgrade and message unification have no level/message tests. pkg/webhook/durable_reconcile.go:238 could revert to Warn (or the enforcing-mode line at :242 be downgraded by mistake) with no test failing, while the analogous dispatch split got a dedicated test. A small level/message assertion would pin the de-noising property.

6. Pre-existing adjacent gap: RecordUnregisteredRepositoryWebhook still records unbounded repo names. pkg/metrics/metrics.go:1566 unconditionally sets a repository attribute from raw payload names, reachable in legacy single-App mode (where the new ownership gate is bypassed, handler.go:856) with 10+ callers. An org-wide single-App install mints a new attribute value per unmanaged repo — the same cardinality blow-up this PR fixes, one layer down; worth a follow-up.

7. Commit type is arguably feat(observability), not fix. The PR adds a new metric status value, and AGENTS.md:68 says to use feat(observability) for a new metric label/dimension — precedent 7ff109a (feat(observability): distinguish transport failures and 404s in GitHub request metrics #1024) typed an analogous status split as feat. The same line also sanctions fix(observability) for severity changes, so both readings are defensible; flagging for changelog consistency.

The one thing that could have broken, verified

The riskiest mechanism is the errors.Is-based three-way split (log level, metric status, repository attribute) at pkg/webhook/handler.go:676-692: misclassification would either demote genuine App-ownership drift to debug (defeating the new trust-boundary alert) or keep unmanaged traffic in the warn stream. Traced the sentinel end to end: exactly one %w wrap at pkg/api/config.go:2243 and a bare return at handler.go:861, so errors.Is always sees the sentinel and the repository attribute can never carry an unmanaged repo name. The negative case is pinned: a declared repo with a broken github_app mapping returns a non-sentinel error, asserted with NotErrorIs at pkg/api/config_test.go:3322, so drift can never take the debug branch. Both outcomes still hit the identical fail-closed tail (RecordWebhookEvent plus 401 at handler.go:692-693). TestDispatch_OwnershipRejectionSeverity, which asserts the exact slog level, repo, app_name, and 401 for each cause, was independently re-run in the worktree and passes.

Verified correct

• Sentinel chain sound: single %w wrap at pkg/api/config.go:2243, bare return at handler.go:861.
• Rejection fails closed on both branches — same 401 + counter at handler.go:692-693; severity test passes.
• Drift cannot be misclassified as unmanaged: NotErrorIs pinned at pkg/api/config_test.go:3322.
• metricRepo="" genuinely omits the attribute — appended only when non-empty at pkg/metrics/metrics.go:1514; no other unbounded attribute introduced.
• Ping's empty action stays allowlisted on the new path: knownWebhookActions[""] at metrics.go:1192.
• Reconciler downgrade correctly scoped: only durable_reconcile.go:238 became Info; the enforcing line at :242 stays Warn and the per-head metric still fires at :232.
• Message unification byte-identical at merge_group.go:323 and check_publisher.go:717, old message fully gone; no file or symbol overlap with sibling PR feat(github): surface engine throttle pauses and checkpoint resumes #1039, so merge order is safe.
• CI 33/33 checks pass at head 0067898; go build/vet and -race unit suites for pkg/api, pkg/metrics, pkg/serve, pkg/webhook clean in the worktree; gofmt clean on all 8 changed files.

This review was generated by Claude Code (claude-fable-5).

[aparajon]

🤖 All 7 addressed in c0f29a1 (fixes in 0df9cc6).

1–5 implemented as described: both statuses plus the conditional repository attribute are now in the README with a Webhook Ownership Rejections section, docs/configuration.md documents both rejection outcomes and their log levels, TestDispatch_OwnershipRejectionSeverity asserts the metric status and the dropped repository attribute per cause, the two "posted passing aggregate" sites share attribute keys, and TestWebhookReconcilerMissingHeadSeverityByMode pins the report-only-vs-synthesizing Info/Warn split.

6: correctly identified, but we're accepting the repository attribute on RecordUnregisteredRepositoryWebhook as-is for now — the cardinality is tolerable at current install shape and the attribute is useful. Not filing a follow-up. 7 noted; leaving the type as fix(observability) since the severity right-sizing is the headline and the status value is a consequence of it.

Note on the merge: main moved ahead and #1032 had landed an identical collectCounterPoints in the integration-tagged auto_plan_integration_test.go. The untagged copy is visible in both builds, so I kept that one (with #1032's exact body) and dropped the tagged duplicate — no test or behavior removed.

Addressed by Claude Code (claude-opus-5).