#317 feat(storage): WebhookAuditSink — first built-in concrete audit sink (F8.5)

[Anonymous]

Originally created by: Maqbool61

Slice

• Feature / Slice: F8.5 — Generic webhook AuditSink (first concrete sink)
• Closes [#233]

What this PR does

Adds WebhookAuditSink to storage/sinks.py — the first built-in concrete
AuditSink. Activated by .doberman/audit_webhook.yaml; inert with no file.
emit() enqueues to a bounded queue and returns immediately; a daemon worker
thread does the actual HTTPS POST using stdlib urllib.request. Queue overflow
drops oldest and counts. emit_to_sinks() is updated to consult plugin sinks
first, then the built-in sink.

Tests added (run in CI)

• tests/unit/test_webhook_audit_sink.py — 40 tests

Covers: synthetic secret never in POST body, emit() non-blocking under wedged
endpoint, drop-oldest counting, inert without config, HTTPS enforcement for
non-loopback URLs, auth token absent from all logs, plugin sinks run before
built-in, HTTP/URL/Timeout error paths swallowed.

Smoke test (real local HTTP server)

Ran end-to-end against a live http.server on loopback before opening this PR:

#	Scenario	Result
1	Inert sink (no config) — no crash, no thread spawned	✅
2	Real HTTP server on loopback receives POST	✅
3	YAML config parses url, auth_env, timeout_s correctly	✅
4	emit() returned in 0.01 ms against a 3 s timeout — truly non-blocking	✅
5	Auth token in Authorization header, absent from JSON body	✅
6	Synthetic secret never in POST body; HMAC fingerprint present	✅
7	Drop-oldest: drops=1 after overflow on a 1-slot queue	✅
8	http://example.com rejected, https://example.com accepted	✅
9	emit_to_sinks() wires and delivers via built-in sink	✅

Security checklist

• [x] Fails closed on error / uncertainty
• [x] No secret, full file, or unredacted prompt logged or committed
• [x] Any guardrail/learning change is raise-only (no silent loosening)
• [x] Every BLOCK/AUTH carries reason codes + a human explanation

Edge cases covered

• HTTP allowed for loopback targets (127.x, ::1, localhost) — dev/test use
• Bad timeout_s falls back to default (5 s) rather than failing
• Missing auth env var → POST continues without Authorization header
• Token value not stored on the object; read from env at POST time only
• Malformed YAML, non-mapping YAML, empty url → all produce inert sink

[Anonymous]

Originally posted by: fu351

This is strong work — 820 lines of real feature, and the hard parts are right. The body you POST is exactly the record build_record already redacted (path classes, verdicts, reason codes, HMAC fingerprints, never raw target/args/prompt); the auth token is read at call time into the header only, never cached and never in the body; emit() does a non-blocking put and swallows everything, so a webhook failure can't touch the decision path; the queue is bounded with drop-oldest; and plugin-sinks-then-builtin ordering is clean. That's the shape a concrete AuditSink should have.

One blocker before merge, and it's a single wire, not a redesign:

The webhook config resolves against the wrong directory. emit_to_sinks grew a repo_root parameter (default "."), but the only real caller — record_decision in storage/log.py:216 — still calls emit_to_sinks(record) bare, even though it already has the real repo_root in hand (it threads it into open_db(repo_root) two lines up). So the built-in sink resolves .doberman/audit_webhook.yaml against the process's cwd, not the repo Doberman is protecting. Since doberman serve --path <repo> sets the repo root to an operator path that usually isn't the cwd, the webhook goes silently inert for essentially every real deployment. Worse case: if the process cwd happens to be a different Doberman repo with its own audit_webhook.yaml, the protected repo's decision records get forwarded to the wrong repo's webhook, with no error. I confirmed the call site and the default against the code.

Fix: log.py:216 → emit_to_sinks(record, repo_root=repo_root), plus an integration test that drives record_decision(..., repo_root=<tmp>) end to end and asserts the sink resolves against that path, not cwd. (The singleton cache keyed on the first repo_root is fine once the real one is threaded, but worth a glance.)

Two test-integrity notes worth fixing in the same pass, since they guard the exact promise this PR makes:

1. The redaction/token tests replace _post wholesale with a test reimplementation, so the real body-and-header builder is never exercised. I dropped a logger.warning("token=%s", token) straight into the real _post and the suite stayed green — a real leak would ship undetected. Stub urllib.request.urlopen instead and call the real _post, asserting on the captured Request.data / .headers.
2. test_drop_oldest_when_queue_full calls the real, unmocked _post, so it fires a live HTTPS POST to example.com — it passes today only because this box has egress. Use the counting fake that the sibling test_drop_counter_increments_on_each_overflow already uses; the repo's tests need to be network-free and deterministic.

Small one: no close()/stop(), so each direct construction leaks a daemon poll thread — fine for now, worth a graceful shutdown later.

None of this is a knock on the design. The redaction and fail-closed discipline are exactly right, which is the hard half. Thread the repo_root, make those two tests exercise the real path, and it's ready. Happy to look again the moment you push.

[Anonymous]

Originally posted by: Maqbool61

Thanks for the thorough review — all three blockers addressed:

1. repo_root threaded through (log.py:216)
emit_to_sinks(record) → emit_to_sinks(record, repo_root=repo_root). The real caller already has repo_root in hand (it passes it into open_db two lines up) — was just missing the forward. Without this, the built-in sink resolved .doberman/audit_webhook.yaml against the process cwd, silently going inert in every real deployment where cwd ≠ repo root. Worst-case cross-repo forwarding is also closed.

2. Tests now stub urllib.request.urlopen, not _post
All body/header/secret/auth tests were rewritten to monkeypatch urlopen and assert on the real Request.data and Request.headers. The real _post serialisation and header-construction path is fully exercised now — a logger.warning("token=%s", token) leak would be caught.

3. test_drop_oldest_when_queue_full is network-free
Removed the orig_post fallthrough that was hitting the live urlopen. Worker is now frozen with a lambda (same pattern as the sibling overflow test), making it deterministic regardless of egress.

Full unit suite run locally across all 148 files: 2205 passed, 4 skipped, 0 failures. The 4 skips are pre-existing and unrelated.

[Anonymous]

Ticket changed by: fu351

• status: open --> closed

[Anonymous]

Originally posted by: fu351

Really happy with this one. The two things a sink like this has to get right, you got right: emit() never touches the network on the hot path, and the drop-oldest counting stays correct even when two threads overflow the queue at once — without a big lock around the whole thing, which most people get wrong. My favorite test in the file is the one that walks the sink's __dict__ to prove the auth token is never stored anywhere. That's testing the actual promise, not just the docstring. The loopback check dodging the 127.0.0.1.evil.com trick was a nice catch too.

Heads up on three small things I fixed on your branch before merging (5009f6d), so you know what changed:

• The sink cache was a single global slot, so a second repo root in the same process would silently reuse the first repo's config. It's a dict keyed by the resolved root path now — your from_repo design made it a five-line fix.
• Loopback URLs skipped the scheme check entirely, so ftp://127.0.0.1 was accepted and then every POST just failed quietly. http/https is now required everywhere.
• Added a short README section for audit_webhook.yaml — house rule is that any new user-facing config gets documented in the same slice.

One thing came out of review that I'd rather hand to you than patch myself: the sink has no close(), so a worker thread can't be stopped or reconfigured without killing the process. That's [#337] — a threading.Event in the worker loop plus a test fixture, and you know this code better than anyone right now. After that, [#245] (OpenTelemetry AuditSink, level-4) is sitting there waiting: the second sink through the seam you just built.