Feat/webhooks

[jjanella]

Description

Webhooks

[the-company-company]

🔴 2 high · 🟠 3 medium · 🔵 1 low

_{Reviewed commit 95891de.}

[the-company-company]

🔴 High · Webhook delete always fails for existing webhooks

For an existing webhook, current is Some(serialized_config), so this proposal carries expect_one_of: vec![Some(...)]. epoxy v2 rejects that shape outright: SetProposal::from_proposal (engine/packages/epoxy/src/ops/propose.rs:122) bails with "epoxy v2 does not support multiple expect_one_of values for CheckAndSet or expect_one_of values that are not None" unless the value is exactly vec![None], and epoxy_propose propagates that as an operation error. DELETE /webhooks/{webhook_name} therefore returns an internal error for every existing webhook, and the delete_removes_webhook test in this PR fails as the inline TODO notes.

The mirror-image case is also wrong: when the webhook does not exist, current is None, the validation passes, and the delete silently commits a no-op (200) instead of returning webhook.not_found.

Until epoxy v2 supports real CAS expectations, make the delete a plain set-to-None proposal (the same degradation upsert's expect_one_of: vec![None] already relies on) and turn the current.is_none() read into a NotFound error.

[the-company-company]

🔴 High · No engine code ever triggers webhooks

The whole delivery path is driven by Trigger { event } signals carrying runner_pool.error / runner_pool.healthy, but the only sender of webhook::workflows::webhook::Trigger in the tree is the test helper in engine/packages/engine/tests/api_webhooks.rs. Runner pool failures are reported to runner_pool_error_tracker via ReportError, and nothing fans those (or recovery) out to subscribed webhook workflows; pegboard even gains a webhook dependency in this PR that no pegboard code references.

As shipped, users can create webhooks and subscribe to both event types, but no delivery is ever recorded or sent outside tests. Wire the runner pool error/healthy reporting path (or an equivalent emitter) to signal each subscribed webhook workflow, or hold the API surface until the emitter exists.

[the-company-company]

🟠 Medium · Delivery records grow unbounded and every events read scans them all

record_delivery writes a DeliveryKey record for every delivery round, and nothing ever removes those records: the terminal Succeeded/Failed writes keep them, the delete op only clears the config key, and there is no retention or TTL. Meanwhile this op reads the webhook's entire delivery range (StreamingMode::WantAll) into memory, and the events endpoint sorts the full set just to return one page. A webhook that receives steady events makes every events request an unbounded read, and deleting the webhook leaves the orphaned records behind (they also resurface if the same name is re-created).

Cap retained deliveries per webhook (write-time pruning or a capped recent index), clear the delivery subspace in the delete op, and page the range read instead of collecting everything.

[the-company-company]

🟠 Medium · SSRF guard allows loopback destinations by default

The engine dials user-configured URLs from inside its own network namespace, and the guard's stated rationale is deny-by-default for anything not publicly routable. Loopback resolves to the engine's own pod or host, though, and it is allowed unless an operator explicitly sets outbound.allow_loopback = false. A runner or webhook URL like http://127.0.0.1:<port>/... therefore reaches anything listening in the engine's own namespace (internal peer/admin endpoints, metrics, pprof), which is exactly the class of target this guard exists to protect, and the new troubleshooting doc tells users non-publicly-routable destinations are refused by default without mentioning the loopback exception.

Prefer flipping the default to deny and letting local development opt in explicitly, or at least set allow_loopback = false in the managed/self-host production configs and document the exception in troubleshooting.mdx.

[the-company-company]

🟠 Medium · Local nix development environment files are committed

This symlink (and .direnv/flake-profile-2-link, which points at a machine-specific /nix/store/... path) is a local direnv artifact that is broken for anyone else. The same PR adds .envrc and flake-profile* to .gitignore precisely to keep these files out, yet they are committed anyway. flake.nix, flake.lock, and rust-toolchain.toml come from the "chore: setup nix development env" commits and are likewise local environment setup; rust-toolchain.toml additionally forces channel = "stable" on every rustup-based contributor's build.

Drop these files from the PR and let the new .gitignore entries keep them out.

[the-company-company]

🔵 Low · Compose regeneration drops RIVET_OTEL_SAMPLER_RATIO=1

Regenerating this file from the template drops RIVET_OTEL_SAMPLER_RATIO=1 from all nine engine services because the template never emitted it. The engine's default when the variable is unset is a 0.001 sampling ratio (engine/packages/metrics-server/src/providers.rs:94), so tracing in this dev environment silently goes from 100% to 0.1%. If full sampling is still wanted here, add the variable to the template service definition so regeneration preserves it.