fix(otel): keep gatekeeper's own OTel diagnostics out of the log export bridge

[andybons]

Fixes #48.

Problem

configureLogging fans every slog record to otelslog.NewHandler("gatekeeper") unconditionally (the bridge's Enabled() defers to the OTel logger, not the console level). The logOTelError handler added in #47 emits a DEBUG diagnostic on every failed OTel export — which is then enqueued into the same failing log-export pipeline: failed export → DEBUG record → re-enqueued → next export fails carrying it → another record, indefinitely while the collector is unreachable. Bounded (one record per attempt) but a real feedback loop.

Fix

An otelDiagnosticFilter wraps only the otelslog bridge handler and drops any record carrying a new exported marker, gatekeeper.OTelDiagnosticKey ("otel_diagnostic"); logOTelError tags its Debug call with that key. The console/file handler still receives every record, so diagnostics remain visible locally at DEBUG — they just never re-enter the export pipeline. WithAttrs/WithGroup are overridden so the filter survives slog's handler chaining.

Chose the filter-wrapper over a dedicated non-bridged logger because logOTelError is registered globally via otel.SetErrorHandler and calls slog.Default(); the wrapper localizes bridge-specific behavior where the bridge is built without fighting slog's single-default-logger convention.

Tests

TestOTelDiagnosticFilter_KeepsBridgeOutOfLoop (marked record reaches console, not bridge; unmarked reaches both) and TestLogOTelError_MarksAsOTelDiagnostic, both mutation-checked (neutering each flips its test red). go build ./... && go vet ./... && gofmt -l . && go test -race ./... all clean.

CHANGELOG v0.17.2 Fixed entry.

[claude]

📦 Previous Review (superseded by newer review)

Claude finished @andybons's task in 1m 56s —— View job

---

Claude's Review

• Gather context (PR body, changed files)
• Read changed files in detail
• Review for security issues, races, bugs, error handling, edge cases
• Post final summary

Note: I was unable to run go build/go vet/go test -race locally in this environment (tool permissions), so this is a static read of the diff. The PR body's stated go build ./... && go vet ./... && gofmt -l . && go test -race ./... results couldn't be independently re-verified here — worth confirming CI is green.

Summary

This is a small, well-targeted fix. I traced the actual mechanics end-to-end and didn't find bugs:

• otelDiagnosticFilter embeds slog.Handler and only overrides Handle/WithAttrs/WithGroup, leaving Enabled deferring to the wrapped otelslog bridge — which correctly preserves the existing (if awkward) behavior described in the PR ("the bridge's Enabled() defers to the OTel logger, not the console level"). The filter operates purely inside Handle, so it doesn't change what multiHandler.Enabled/multiHandler.Handle decide to call — it just no-ops the forwarding call when the marker is present.
• record.Attrs() iteration + early return false on match is correct per slog.Record's callback semantics, and since logOTelError passes the marker as a direct key/value arg to Debug(...) (not via .With(...)), it lands in the Record itself where Attrs() can see it — confirmed this is the only call site tagging OTelDiagnosticKey (grep across the repo).
• multiHandler fans out to the console/file handler independently of the filter, so diagnostics remain visible locally — matches the stated design.
• No new goroutines, shared mutable state, or locking introduced — nothing here changes the proxy's credential/TLS/host-matching paths at all (the diff is confined to logging plumbing), so the higher-risk areas this repo's review priorities call out (credential injection, TLS, TOCTOU, proxy forwarding) are untouched by this PR.
• Test coverage matches the two behaviors claimed: TestOTelDiagnosticFilter_KeepsBridgeOutOfLoop (marked record reaches console only) and TestLogOTelError_MarksAsOTelDiagnostic (tags the record), and the pre-existing TestLogOTelError_DemotesToDebug still holds since the new attribute is just appended to the args list.

Minor observations (non-blocking)

• otelDiagnosticFilter.Handle treats any record carrying the OTelDiagnosticKey attribute as a filter match, regardless of its value (a.Key == OTelDiagnosticKey — the value isn't checked). Today the only call site always sets it to true, so this is harmless, but a future caller setting OTelDiagnosticKey, false would still be filtered out of the bridge, which reads as a slight mismatch between the name ("is this a diagnostic") and the semantics ("does this key exist"). Not worth blocking on — just flagging in case a future record wants to carry the key without opting out of export.
• recordingHandler (cmd/gatekeeper/main_test.go) and fakeSlogHandler (gatekeeper_test.go) are near-identical test doubles, but they live in different packages (main vs gatekeeper), so there's no reasonable way to share them — not a real issue.

No security, race, or correctness issues found. The scope is appropriately narrow for the bug it fixes.

[andybons]

Addressed the value-vs-presence observation in 68ff74d: otelDiagnosticFilter.Handle now only filters when OTelDiagnosticKey is present and its boolean value is true, so a future record carrying the key set to false stays in the export bridge — matching the name's intent. Added a case to TestOTelDiagnosticFilter_KeepsBridgeOutOfLoop (key=false must pass through to the bridge); confirmed it fails against the old presence-only check before the fix. Full go test -race ./... green. The test-double duplication is inherent to the package split, as you noted — left as-is.

[claude]

Claude finished @andybons's task in 2m 0s —— View job

---

Reviewing PR #51 (updated)

• Gather context (diff vs origin/main, prior review, new commit 68ff74d)
• Review the current diff (static read — see note on tooling below)
• Post final review

Claude's Review

Re-reviewed after commit 68ff74d, which addresses the one non-blocking observation from my prior review (value-vs-presence filtering).

Note: As with the prior review, I was unable to run go build/go vet/go test -race/gofmt in this environment (Bash tool calls require interactive approval that isn't available here). This is a static read of the diff. The PR author's stated go test -race ./... green result couldn't be independently re-verified — worth confirming CI is green.

Verified: the value-vs-presence fix is correct

otelDiagnosticFilter.Handle (gatekeeper.go:111-124) now reads:

record.Attrs(func(a slog.Attr) bool {
	if a.Key == OTelDiagnosticKey && a.Value.Kind() == slog.KindBool && a.Value.Bool() {
		marked = true
		return false
	}
	return true
})

This correctly filters only when the key is present and its value is slog.KindBool and true — matching the doc comment's intent ("is this a diagnostic," not "does this key exist"). A record carrying OTelDiagnosticKey=false now passes through to the bridge, closing the gap I flagged previously.

logOTelError (cmd/gatekeeper/main.go:60) still tags its own record with gatekeeper.OTelDiagnosticKey, true — the only call site — so the existing feedback-loop fix is unaffected by this refinement.

Test coverage matches the new behavior

TestOTelDiagnosticFilter_KeepsBridgeOutOfLoop (gatekeeper_test.go) now exercises three cases: a true-marked diagnostic (filtered from bridge, still reaches console), a normal record (reaches both), and a new OTelDiagnosticKey=false record (asserted to reach the bridge as bridge.records[1]). This directly encodes the fixed behavior and would fail against the old presence-only check, per the author's mutation-check claim.

I also confirmed by reading the surrounding code that:

• gatekeeper.go's context import was already present (no new import needed for the Handle change).
• cmd/gatekeeper/main_test.go's new context and github.com/majorcontext/gatekeeper imports are both used (by recordingHandler.Enabled and gatekeeper.OTelDiagnosticKey respectively).
• No other files were touched beyond what's described — the diff is confined to logging plumbing (gatekeeper.go, cmd/gatekeeper/main.go), their tests, and the CHANGELOG entry. The proxy's credential injection, TLS, and host-matching paths (this repo's higher-risk areas per AGENTS.md) are untouched.

No new issues found. This is a clean, well-targeted refinement of an already-correct fix — no inline comments needed.