diff --git a/evals/run-evals.sh b/evals/run-evals.sh index c8a0817b9..c4e8f38e4 100755 --- a/evals/run-evals.sh +++ b/evals/run-evals.sh @@ -564,6 +564,16 @@ start_eval_daemon() { local deadline=$((SECONDS + 60)) while (( SECONDS < deadline )); do if curl -fsS "http://127.0.0.1:$EVAL_PORT/api/health/ready" >/dev/null 2>&1; then + # The container runs with --network host, so any process on this + # port can answer the host-side readiness poll — including another + # eval run's daemon. Readiness must also prove THIS container's + # daemon is alive, or the whole run interrogates a stranger while + # every daemon-log assert reads its own dead container's empty log. + if ! docker exec "$EVAL_CONTAINER_NAME" pgrep -f netclawd >/dev/null 2>&1; then + echo "ERROR: port $EVAL_PORT answered but this container's daemon is not running — another daemon owns the port. Set NETCLAW_EVAL_PORT to a free port." >&2 + docker logs "$EVAL_CONTAINER_NAME" >&2 2>&1 || true + exit 2 + fi echo "Eval daemon ready at http://127.0.0.1:$EVAL_PORT" return 0 fi @@ -862,6 +872,7 @@ run_prompt() { # Record daemon log position before the prompt (the daemon writes to a # daily-rotating file at /root/.netclaw/logs/daemon-YYYY-MM-DD.log, and # the container bind-mounts that directory from $EVAL_HOME/logs). + resolve_daemon_log if [[ -f "$DAEMON_LOG" ]]; then DAEMON_LOG_LINES_BEFORE=$(wc -l < "$DAEMON_LOG") else @@ -882,7 +893,7 @@ run_prompt() { # stdout, producing a false eval failure rather than a real one. NETCLAW_DAEMON_ENDPOINT="http://127.0.0.1:$EVAL_PORT" \ NETCLAW_HOME="$EVAL_HOME" \ - timeout "$PROMPT_TIMEOUT" "$NETCLAW_BIN" chat -p "${output_args[@]}" "$prompt" \ + timeout "$PROMPT_TIMEOUT" stdbuf -oL -eL "$NETCLAW_BIN" chat -p "${output_args[@]}" "$prompt" \ > "$STDOUT_FILE" 2> "$STDERR_FILE" || true # Brief pause for daemon log flush @@ -922,6 +933,7 @@ run_prompt_resume() { fi STDERR_FILE="$MULTI_TURN_STDERR_FILE" + resolve_daemon_log if [[ -f "$DAEMON_LOG" ]]; then DAEMON_LOG_LINES_BEFORE=$(wc -l < "$DAEMON_LOG") else @@ -935,7 +947,7 @@ run_prompt_resume() { NETCLAW_DAEMON_ENDPOINT="http://127.0.0.1:$EVAL_PORT" \ NETCLAW_HOME="$EVAL_HOME" \ - timeout "$PROMPT_TIMEOUT" "$NETCLAW_BIN" chat -p --resume "$session_id" \ + timeout "$PROMPT_TIMEOUT" stdbuf -oL -eL "$NETCLAW_BIN" chat -p --resume "$session_id" \ "${output_args[@]}" "$prompt" \ > "$turn_file" 2> "$turn_stderr_file" || true @@ -1069,6 +1081,19 @@ stdout_response_not_contains() { return 0 } +## The daemon runs inside the container on its own clock (UTC), so a host +## date computation can name a log file the daemon never writes — every +## daemon_log_contains then fails silently for the whole run (observed when a +## CDT-evening run crossed UTC midnight). Resolve the newest real log file +## instead of trusting a computed date. Callers re-resolve before they take a +## per-case line baseline; a midnight rollover inside a single case remains +## unhandled and acceptable. +resolve_daemon_log() { + local newest + newest=$(ls -1t "$EVAL_HOME"/logs/daemon-*.log 2>/dev/null | head -n 1) + [[ -n "$newest" ]] && DAEMON_LOG="$newest" +} + daemon_log_tail() { if [[ -f "$DAEMON_LOG" ]]; then tail -n +"$((DAEMON_LOG_LINES_BEFORE + 1))" "$DAEMON_LOG" 2>/dev/null @@ -1365,9 +1390,14 @@ assert_memory_explicit_store() { assert_memory_recall_filters() { # After overfetch fix: at least one candidate selection should reduce the set. + # POSIX awk only: mawk lacks gawk's 3-argument match(), which made this + # assert die on a syntax error and fail unconditionally on hosts without + # gawk. Extract the two counts with sub() instead of capture groups. daemon_log_tail | awk ' - match($0, /rawCount=([0-9]+).*selectedCount=([0-9]+)/, m) { - if ((m[1] + 0) > (m[2] + 0)) { + /rawCount=[0-9]+.*selectedCount=[0-9]+/ { + raw = $0; sub(/.*rawCount=/, "", raw); sub(/[^0-9].*/, "", raw) + sel = $0; sub(/.*selectedCount=/, "", sel); sub(/[^0-9].*/, "", sel) + if ((raw + 0) > (sel + 0)) { found = 1 } } diff --git a/feeds/skills/.system/files/netclaw-operations/SKILL.md b/feeds/skills/.system/files/netclaw-operations/SKILL.md index a1f1c352d..aead910a8 100644 --- a/feeds/skills/.system/files/netclaw-operations/SKILL.md +++ b/feeds/skills/.system/files/netclaw-operations/SKILL.md @@ -3,7 +3,7 @@ name: netclaw-operations description: "REQUIRED when the user asks about scheduling, reminders, cron jobs, timers, background jobs, diagnostics, troubleshooting, MCP tools, daemon health, identity updates, or Netclaw capabilities and self-maintenance." metadata: author: netclaw - version: "2.60.0" + version: "2.61.0" --- # Netclaw Operations diff --git a/feeds/skills/.system/files/netclaw-operations/references/webhooks.md b/feeds/skills/.system/files/netclaw-operations/references/webhooks.md index eb98eb5dd..64f85eaf9 100644 --- a/feeds/skills/.system/files/netclaw-operations/references/webhooks.md +++ b/feeds/skills/.system/files/netclaw-operations/references/webhooks.md @@ -4,7 +4,7 @@ ## Webhook Management -Webhooks are gated on `Webhooks.Enabled` in `netclaw.json` (default `true`). +Webhooks are gated on `Webhooks.Enabled` in `netclaw.json` (default `false` — enable it explicitly before routes serve). When disabled, the webhook HTTP endpoint returns 404 for all routes and webhook tools are hidden from discovery. @@ -61,6 +61,23 @@ existing values; provide an argument only when changing that setting. Route files hot-reload without restarting the daemon. If a route file becomes invalid, Netclaw removes that route immediately and emits an operational alert. +Route mutations serialize through one daemon-side authority. The daemon also +exposes an authenticated management resource, separate from the anonymous +delivery endpoint: + +- `GET /api/webhooks` -> list routes (no secrets in responses) +- `GET /api/webhooks/{route}` -> route detail (no secrets) +- `PUT /api/webhooks/{route}` -> create or update; requires Operator authority +- `DELETE /api/webhooks/{route}` -> remove the route + +The `netclaw webhooks` CLI manages routes through this resource. `set` and +`delete` require a running daemon: when the daemon does not answer, when an older +daemon lacks the resource, or when the daemon rejects the call, the command fails +and changes no file. The CLI never writes a route file. `list`, `show`, and +`validate` read the route files on disk, which stay canonical. To author a route +without a daemon, write the route file to the webhooks directory; the daemon +loads it at startup. + **Approval gate:** Webhooks run without a human — they cannot prompt for approval. The same rules as reminders apply: shell commands must be pre-approved in `tool-approvals.json`, and path arguments are scoped by the route's audience diff --git a/openspec/changes/memory-core-redesign/tasks.md b/openspec/changes/memory-core-redesign/tasks.md index a5d6f7281..ac2a7abde 100644 --- a/openspec/changes/memory-core-redesign/tasks.md +++ b/openspec/changes/memory-core-redesign/tasks.md @@ -54,7 +54,7 @@ constitution gates (tests, evals where mapped, schema/skill sync, slopwatch). - [ ] 5.1 **BREAKING**: restrict automatic recall to `recall_mode='auto'` in `SearchByPlanAsync` (searchable leaves the auto pool); update `MemoryIndexContextLayer` guidance - [ ] 5.2 Formation: policy gate honors sidecar-proposed recall mode for durable facts, defaulting to `searchable`; observer distillation prompt rewritten for fewer, more comprehensive proposals with an explicit auto-mode whitelist (identity/preferences/environment) -- [ ] 5.3 Trace revival: reachable producer (sidecar may propose `trace` with 72 h TTL), fresh-trace auto-recall eligibility weighted below durable facts, removal of the unreachable turn-complete Trace dead code +- [ ] 5.3 [SUPERSEDED by PR #2007 — the turn-complete Trace lane was removed; only the unreachable MemoryClass.Trace resolver branches remain to adjudicate] Trace revival: reachable producer (sidecar may propose `trace` with 72 h TTL), fresh-trace auto-recall eligibility weighted below durable facts, removal of the unreachable turn-complete Trace dead code - [ ] 5.4 `MemoryClass.ToolLesson` (`tool_lesson`) → Document/MergeDocument/Searchable with per-tool anchors; `store_memory` accepts the class and sets the `VerifiedToolFinding` checkpoint flag - [ ] 5.5 Sidecar distillation prompt: correction-hunting instruction producing tool-lesson proposals - [ ] 5.6 Per-tool context injection in the tool-execution pipeline: `[tool-lessons:]` block on first use per session (bounded, once per tool, reset on compaction); remove the dead `verified-tool-finding` +25 recall bonus @@ -69,7 +69,7 @@ constitution gates (tests, evals where mapped, schema/skill sync, slopwatch). - [ ] 6.3 `netclaw memory consolidate --apply --plan `: live-daemon refusal (override flag), `VACUUM INTO` backup, batched apply, re-embed + FTS rebuild, ledger row - [ ] 6.4 Expiry sweep in the daemon maintenance loop (grace window, per-class deletion logging) - [ ] 6.5 `netclaw memory status` (composition, coverage, pending checkpoints, expired-awaiting-sweep, recent ledger) -- [ ] 6.6 Checkpoint enqueue gating: turn-complete lane gated by the extractor's precondition at enqueue time +- [ ] 6.6 [SUPERSEDED by PR #2007 — the turn-complete lane was removed, not gated; no enqueue gating is needed] Checkpoint enqueue gating: turn-complete lane gated by the extractor's precondition at enqueue time - [ ] 6.7 Subtraction: drop `memory_edges` DDL, remove facet/soft-scope inference from `DeterministicRetrievalPlanning` (keep stopword hygiene + lexical terms), delete dead Trace path remnants - [ ] 6.8 Integration tests on a seeded corpus: backfill→dry-run→edited-plan apply→status round-trip; sweep deletes only past-grace rows - [ ] 6.9 Runbook update (`docs/runbooks/memory-health-and-evals.md`): embedding, consolidation, sweep operations diff --git a/openspec/changes/webhook-route-actor-ownership/.openspec.yaml b/openspec/changes/webhook-route-actor-ownership/.openspec.yaml new file mode 100644 index 000000000..41c30bab8 --- /dev/null +++ b/openspec/changes/webhook-route-actor-ownership/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-08-19 diff --git a/openspec/changes/webhook-route-actor-ownership/design.md b/openspec/changes/webhook-route-actor-ownership/design.md new file mode 100644 index 000000000..786161f67 --- /dev/null +++ b/openspec/changes/webhook-route-actor-ownership/design.md @@ -0,0 +1,75 @@ +# Design: webhook-route-actor-ownership + +## Context + +`WebhookRouteStore` serialized read-modify-write over per-route JSON files with a named OS mutex, because two processes wrote: the daemon (agent tools) and the CLI (command + TUI). The daemon-internal race is real — two sessions can invoke `set_webhook` concurrently on tool-executor threads. The mutex-based test proved flaky on Windows CI: its failing assertion measured thread-pool dispatch latency, not store correctness (root cause confirmed by reproduction under forced pool limits). The maintainer direction: an actor should own this state; CLI configuration should route through the daemon's HTTP API. + +## Goals / Non-Goals + +**Goals:** + +- One mutation authority for webhook routes inside the daemon. +- CLI writes routes only through the daemon. +- Deterministic tests: message ordering, not thread choreography. +- An old CLI keeps working against a new daemon: its direct file write stays visible, because the actor holds no cache. +- A new CLI against an old daemon fails loudly and names the upgrade. Per D4 it does not fall back. + +**Non-Goals:** + +- No change to delivery, verification, hot-reload, or route file format. +- No generalization to other config surfaces yet. + +## Decisions + +### D1: Plain actor over the existing store; disk stays canonical + +`WebhookRouteActor` is an ordinary `ReceiveActor` (not persistent). It handles `UpsertRoute`/`DeleteRoute`/`GetRoute`/`ListRoutes` messages, validates via `WebhookRouteValidator`, and persists through the existing `WebhookRouteStore` (which keeps its atomic temp-file-and-move write). Disk is the canonical store; the actor is the serialization point, not a second source of truth. Rationale: Akka.Persistence would create journal types and a second copy of secret-bearing config — both forbidden by the back-compat and security constraints. Alternative considered: journaled actor with disk projection — rejected for exactly those reasons. + +### D2: The actor is cacheless — external file changes need no reconciliation + +Implementation finding (supersedes the original signal-based wording): no route change signal exists. The `inbound-webhooks` spec permits request-time mtime-gated reload, and `WebhookRouteCatalog` re-reads route files lazily; there is no watcher on the webhooks directory. Rather than build one, the actor holds no cache: every read and every read-modify-write goes through the store to disk. An external write (old CLI, operator edit) is therefore visible to the very next actor operation with no reconciliation step. This is the direct consequence of D1 — the actor is the serialization point, not a second source of truth. The store takes no cross-process lock: each write is atomic on its own, and the accepted worst case during version skew is one lost same-route update. No new watcher machinery. + +### D3: HTTP resource mirrors the reminders precedent + +`/api/webhooks` endpoints are thin minimal-API handlers that `Ask` the actor with the standard request timeout and map results to HTTP statuses (validation failure → 400 with the validator's message; unknown route → 404; success → 200/204). Same auth middleware and exposure-mode rules as every `/api/*` surface. Agent tools inside the daemon `Ask` the actor directly — no loopback HTTP. + +### D4: CLI route mutations are daemon-only + +Maintainer decision: no fallback of any kind — no dual mode, no `--offline` flag, no local write path. One store, one writer. + +`WebhooksCommand` probes availability once per invocation, immediately before the write. Three answers fail the command with exit code 1 and no file change: + +- The daemon does not answer (transport failure, timeout, or no daemon client at all) → "The daemon is not reachable. Start the daemon to manage webhook routes." +- The daemon answers 404 for the resource (a daemon that predates it) → "This daemon does not serve the webhook route API. Upgrade the daemon." +- The daemon answers any other failure status → the daemon's own message, because the daemon is the enforcement point. + +A transport failure between the probe and the write also fails the command. The daemon may have applied the change before the connection broke, so the CLI reports the uncertainty rather than repeating the write. + +Reads (`list`, `show`, `validate`) stay on canonical disk. That is the read path, not a fallback: disk is the route store, the actor holds no cache, and `show --show-secret` needs a secret the API never returns. Argument grammar, `--dry-run`, and the merge preview all run before the probe, so they keep their own messages and exit codes with no daemon present. + +The supported daemon-absent authoring path is a route file written on disk outside the CLI, which the daemon loads at startup. + +### D5: Ask timeout and failure semantics + +Tool and HTTP fronts use the daemon's standard ask timeout. A store I/O failure inside the actor faults the message with the error returned to the caller (tool result error / HTTP 500) — the actor does not swallow persistence failures. The actor itself restarts under default supervision on unexpected exceptions; its state is rebuilt from disk on restart, so a restart is always safe. + +### D6: Test replacement, not test repair + +The Windows-flaky choreography test is deleted and replaced by: (a) actor tests proving mailbox serialization of concurrent RMW (two `Ask`s, deterministic final state), (b) endpoint tests for `/api/webhooks` status mapping, (c) CLI command tests per daemon answer (daemon up → API call recorded and no file written; daemon down → exit 1 with the unreachable message and no file written; old daemon 404 → exit 1 with the upgrade message and no file written; 400/401/403 → exit 1 with the daemon's message and no file written), (d) no store-level cross-process-guard test, because the store holds no lock to guard. + +## Risks / Trade-offs + +- [Skew window: old CLI writes while the actor serves a request] → D2 keeps the actor cacheless; atomic writes keep every file complete; per-route files bound the blast radius to one lost same-route update. +- [CLI now depends on daemon availability for every route mutation] → accepted by maintainer decision (D4). A fallback would hide a misconfigured or stopped daemon and would let an operator bypass the enforcement point by inducing an error. An operator without a daemon authors the route file on disk and starts the daemon, which loads it. +- [Actor becomes a throughput bottleneck] → route mutations are rare, low-volume operator/agent actions; a single mailbox is far above the required throughput. +- [Two fronts drift (HTTP vs tools)] → both are thin `Ask` adapters over the same messages; validation lives only in the actor. +- [Skill/docs drift] → `netclaw-operations` skill row updated in the same PR per the skills sync rule. + +## Migration Plan + +Ship steps 1 and 2 together in one release. No data migration; no config change. Rollback is a PR revert; the disk format never changed. + +## Open Questions + +- RESOLVED: `InboundWebhooksConfigViewModel` needs no mode-selection seam — it has no route save. It writes only the `Webhooks.Enabled`/`ExecutionTimeoutSeconds` section of `netclaw.json` and delegates route authoring to the `netclaw webhooks` command. Its route read runs against canonical disk and is already correct under the cacheless design. +- RESOLVED BY D2 AMENDMENT: no change-signal plumbing exists or is needed — the actor is cacheless, so no watcher machinery was built. diff --git a/openspec/changes/webhook-route-actor-ownership/proposal.md b/openspec/changes/webhook-route-actor-ownership/proposal.md new file mode 100644 index 000000000..4cba67c25 --- /dev/null +++ b/openspec/changes/webhook-route-actor-ownership/proposal.md @@ -0,0 +1,37 @@ +# Proposal: webhook-route-actor-ownership + +## Why + +Webhook route files have two writer processes today: the daemon (agent tools `set_webhook`/`delete_webhook`) and the CLI (`netclaw webhooks`, config TUI). A named OS mutex in `WebhookRouteStore` guarded the overlap. Locks guard shared state instead of removing the sharing; the guard already produced a flaky Windows CI test whose two-blocked-threads choreography asserts on thread-pool scheduling, not on the store. An actor that owns the state removes the race by construction and gives validation and ACL one enforcement point. + +Source PRDs: PRD-002 (gateway security envelope), PRD-003 (operator UX and ops console), PRD-004 (CLI onboarding and config). Related capability: `inbound-webhooks` (delivery side, unchanged). + +## What Changes + +- **Step 1 — daemon-side ownership.** A new `WebhookRouteActor` becomes the single authority for webhook route mutations inside the daemon. `SetWebhookTool` and `DeleteWebhookTool` `Ask` the actor. The actor validates with `WebhookRouteValidator`, then persists through the existing store. Mailbox order replaces in-process mutex contention. The flaky mutex-choreography test is replaced by deterministic actor message-order tests. +- **Step 2 — CLI routes through the daemon.** New additive HTTP resource `/api/webhooks` (GET list, GET/PUT/DELETE by name) fronting the actor via `Ask`, in the same shape as the existing reminders HTTP-CRUD-over-actor endpoints. `WebhooksCommand` uses the authenticated `DaemonApi` client for every route mutation. Maintainer decision: no fallback — a mutation with no reachable daemon fails and names the remedy. Reads stay on canonical disk. An operator with no daemon authors the route file on disk, which the daemon loads at startup. +- **Backward compatibility is a hard requirement, not an aspiration:** + - The per-route JSON file format is unchanged and disk stays the canonical store. Actor state is a cache of disk. No Akka.Persistence, no journal types. + - CLI flags, exit codes, and stdout formats are unchanged for the read subcommands and for a successful mutation. A mutation with no reachable daemon now fails instead of writing a file (D4). + - The HTTP API change is additive only. Tool schemas for `set_webhook`/`delete_webhook` are unchanged. + - The named `Global\` mutex in `WebhookRouteStore` is REMOVED. The actor tolerates external file changes by reload, and each write stays atomic on its own. The accepted worst case during version skew is one lost same-route update. + +In scope: the actor, tool rewiring, `/api/webhooks`, CLI daemon-only write path, test replacement, `netclaw-operations` skill row for the new endpoints. +Out of scope: any change to webhook delivery, verification, or hot-reload semantics; routing other config surfaces (channels, providers) through the daemon. + +## Capabilities + +### New Capabilities + +- `webhook-route-authority`: single-writer ownership of webhook route mutations — the daemon actor as authority, the HTTP resource and agent tools as thin fronts, the CLI daemon-only write path, and the version-skew tolerance rules. + +### Modified Capabilities + + + +## Impact + +- Code: `src/Netclaw.Actors` (new actor; two tools rewired), `src/Netclaw.Daemon` (new endpoints), `src/Netclaw.Cli` (`WebhooksCommand`, `InboundWebhooksConfigViewModel`, `DaemonApi` client), `src/Netclaw.Configuration.Tests` (replace the choreography test), `feeds/skills/.system/files/netclaw-operations/SKILL.md`. +- Security impact: validation and ACL for route mutations consolidate in the actor (Cross-Boundary Contract Rule). `/api/webhooks` rides existing pairing auth and exposure-mode rules; route files remain secret-bearing config with the same on-disk posture. Default-deny is unchanged. +- Operational impact: `netclaw webhooks set` and `delete` require a running daemon and fail with a readable remedy without one; runbook and CLI help updated. No config migration, no schema change, no restart requirement. +- Rollout: step 1 and step 2 ship in one release. Revert is a plain PR revert; disk format never changes. diff --git a/openspec/changes/webhook-route-actor-ownership/specs/webhook-route-authority/spec.md b/openspec/changes/webhook-route-actor-ownership/specs/webhook-route-authority/spec.md new file mode 100644 index 000000000..23eb9531f --- /dev/null +++ b/openspec/changes/webhook-route-actor-ownership/specs/webhook-route-authority/spec.md @@ -0,0 +1,120 @@ +# webhook-route-authority Specification (delta) + +## ADDED Requirements + +### Requirement: Daemon actor is the single mutation authority + +The daemon SHALL route every webhook route mutation through one `WebhookRouteActor`. The agent tools `set_webhook` and `delete_webhook` SHALL send messages to the actor and SHALL NOT call the route store directly. The actor SHALL validate a mutation before persistence and SHALL return the validation error to the caller on failure. Disk SHALL remain the canonical store: the actor persists through the route store and holds no journaled state. + +#### Scenario: Concurrent agent mutations serialize by mailbox order + +- **GIVEN** two sessions that invoke `set_webhook` for the same route at the same time +- **WHEN** both requests reach the actor +- **THEN** the actor applies them one at a time in arrival order +- **AND** the final route file reflects both read-modify-write operations with no lost update + +#### Scenario: Validation failure does not persist + +- **GIVEN** a `set_webhook` request that fails `WebhookRouteValidator` +- **WHEN** the actor processes it +- **THEN** no file write occurs +- **AND** the caller receives the validator's error + +### Requirement: The mutation message is a patch; the merged definition carries the required fields + +An upsert message SHALL be a field-level patch. A null field SHALL mean "keep the stored value", so a caller SHALL NOT need to resend a value it does not change. Two patches of different fields on the same route SHALL therefore compose instead of overwrite. + +The message SHALL require only the two fields a patch can never inherit from a file: the route name and the authority of the caller. The route name SHALL travel as a validated value object, so no unvalidated name SHALL reach a file path. Every other required field SHALL be enforced on the merged definition by `WebhookRouteValidator`, which SHALL reject a merged route without a prompt and a merged route without a verification secret. + +#### Scenario: A patch that blanks a required field is rejected + +- **GIVEN** a stored route with a prompt +- **WHEN** an upsert patches the prompt to a blank value +- **THEN** the actor rejects the merged definition with the validator's message +- **AND** the stored route file is unchanged + +### Requirement: HTTP resource fronts the actor + +The daemon SHALL expose `/api/webhooks` (list), `/api/webhooks/{name}` (get, upsert, delete) as thin handlers that ask the actor. The resource SHALL use the same authentication and exposure-mode rules as the other `/api` surfaces. The change SHALL be additive: no existing endpoint changes. Handlers SHALL map actor results to HTTP statuses: validation failure to 400, unknown route to 404, success to 200 or 204. + +#### Scenario: Upsert through the API persists through the actor + +- **GIVEN** an authenticated `PUT /api/webhooks/{name}` with a valid route body +- **WHEN** the daemon handles it +- **THEN** the actor validates and persists the route +- **AND** the response is a success status with the stored route + +#### Scenario: Unauthenticated access is rejected + +- **GIVEN** a request to `/api/webhooks` that does not carry a paired-device credential +- **WHEN** the daemon evaluates it +- **THEN** the request is rejected by the same auth rules as the other `/api` surfaces + +### Requirement: CLI route mutations require the daemon + +The CLI SHALL send every webhook route mutation to the daemon API. The CLI SHALL NOT write a route file. When the daemon does not answer, or answers 404 for the resource, or refuses the call, the command SHALL fail and SHALL leave every route file unchanged. The failure message SHALL name the state and the remedy. The CLI read subcommands (`list`, `show`, `validate`) SHALL keep reading canonical disk, because disk is the route store and `show` reveals a secret that the API never returns. Argument grammar, `--dry-run`, and the merge preview SHALL run before the daemon call and SHALL keep their own messages and exit codes. The supported daemon-absent path is a route file authored on disk outside the CLI, which the daemon loads at startup. + +#### Scenario: Daemon reachable routes through the API + +- **GIVEN** a running paired daemon +- **WHEN** the operator runs `netclaw webhooks set` +- **THEN** the CLI sends the mutation to `/api/webhooks/{name}` +- **AND** writes no file itself + +#### Scenario: Daemon down fails the command + +- **GIVEN** no running daemon +- **WHEN** the operator runs `netclaw webhooks set` with valid arguments +- **THEN** the command fails with exit code 1 +- **AND** the error names the daemon as unreachable and tells the operator to start it +- **AND** no route file is created or changed + +#### Scenario: Old daemon without the resource fails the command + +- **GIVEN** a running daemon that predates the webhook route resource +- **WHEN** the operator runs `netclaw webhooks set` and the probe answers 404 +- **THEN** the command fails with exit code 1 +- **AND** the error tells the operator to upgrade the daemon +- **AND** no route file is created or changed + +#### Scenario: Validation rejection does not bypass the daemon + +- **GIVEN** a running daemon that rejects a mutation with a validation error +- **WHEN** the CLI receives the 400 response +- **THEN** the command fails with the validator's message +- **AND** no route file is created or changed + +#### Scenario: Dry run needs no daemon + +- **GIVEN** an operator who runs `netclaw webhooks set --dry-run` +- **WHEN** the CLI validates the merged route +- **THEN** it reports the result without a daemon call +- **AND** writes no file + +### Requirement: Version-skew tolerance without a cross-process lock + +The route store SHALL hold no cross-process lock. Version-skew tolerance SHALL rest on two properties instead. First, the actor SHALL hold no cache: every read and every read-modify-write SHALL go through the store to disk, so a direct file write by an old CLI is visible to the next actor operation without a reconciliation step. Second, the store SHALL write each route file atomically through a temporary file and one replacing move, so no reader SHALL see a partial file. The per-route JSON file format SHALL NOT change. + +Accepted edge case: if an old CLI patches the same route at the same moment as the daemon actor, one of the two updates is lost. Webhook route mutations are rare, so the project accepts this risk rather than a lock. + +#### Scenario: Old CLI writes a file behind the actor + +- **GIVEN** a running new daemon and an old CLI that writes a route file directly +- **WHEN** any subsequent read or update reaches the actor +- **THEN** the actor serves the file's current content from disk + +#### Scenario: A write never exposes a partial file + +- **GIVEN** a reader that opens a route file while the store writes it +- **WHEN** the store replaces the file +- **THEN** the reader sees either the complete old content or the complete new content + +### Requirement: Deterministic tests replace scheduling choreography + +The serialization guarantee SHALL be tested through actor message ordering and outcome assertions. No test of this capability SHALL assert on thread scheduling, bounded event waits, or elapsed time. + +#### Scenario: Serialization test is message-order based + +- **GIVEN** the actor test for concurrent mutations +- **WHEN** it runs on a starved thread pool +- **THEN** it still passes or fails only on the serialization outcome diff --git a/openspec/changes/webhook-route-actor-ownership/tasks.md b/openspec/changes/webhook-route-actor-ownership/tasks.md new file mode 100644 index 000000000..4e635c038 --- /dev/null +++ b/openspec/changes/webhook-route-actor-ownership/tasks.md @@ -0,0 +1,34 @@ +# Tasks: webhook-route-actor-ownership + +Implementation branch: decided at apply time (standalone off `dev`, or stacked on stack #2003 once it merges). Every group ends with a green build (zero warnings), the affected suites green, `dotnet slopwatch analyze` clean, and header verification clean, and is one commit. + +## 1. WebhookRouteActor (daemon-side authority) + +- [x] 1.1 Add `WebhookRouteActor` (plain `ReceiveActor`) with `UpsertRoute`/`DeleteRoute`/`GetRoute`/`ListRoutes` messages; validate via `WebhookRouteValidator` before persistence; persist through the existing `WebhookRouteStore`; store I/O failures return errors to the caller, never swallowed +- [x] 1.2 Register the actor in daemon wiring; rewire `SetWebhookTool` and `DeleteWebhookTool` to `Ask` the actor; tool schemas and result shapes unchanged +- [x] 1.3 RESOLVED BY DESIGN AMENDMENT (D2 rewritten): no route change signal exists in the codebase — the actor is cacheless instead, so every operation reads disk and external changes need no reconciliation; proven by `An_external_writer_change_is_visible_to_the_next_actor_read` +- [x] 1.4 Actor tests: two concurrent FIELD-LEVEL updates to the same route lose neither field (the real RMW lost-update proof — mutation messages carry data, the actor does read-modify-write per message), validation-rejection-does-not-persist, restart rebuilds from disk, external-writer visibility proven outcome-only (cacheless actor per amended D2) + +## 2. /api/webhooks resource + +- [x] 2.1 Add minimal-API endpoints (GET list, GET/PUT/DELETE by name) that `Ask` the actor; map validation failure → 400, unknown route → 404, success → 200/204; same auth middleware and exposure-mode rules as sibling `/api` surfaces +- [x] 2.2 Endpoint tests: status mapping per outcome, auth rejection parity with an existing `/api` surface, upsert-persists-through-actor round trip + +## 3. CLI daemon-only write path + +- [x] 3.1 Extend the `DaemonApi` client with the webhook resource calls +- [x] 3.2 `WebhooksCommand` route mutations are daemon-only (maintainer decision: no fallback). The probe runs once, immediately before the write. Unreachable → fail with "The daemon is not reachable. Start the daemon to manage webhook routes."; 404 on the resource → fail with "This daemon does not serve the webhook route API. Upgrade the daemon."; any other failure status → fail with the daemon's own message. No path writes a route file. Reads (`list`, `show`, `validate`), argument grammar, and `--dry-run` stay local and keep their messages and exit codes +- [x] 3.3 RESOLVED NOT APPLICABLE: `InboundWebhooksConfigViewModel` has no route save — it writes only `Webhooks.Enabled`/`ExecutionTimeoutSeconds` to `netclaw.json` and delegates route authoring to `netclaw webhooks` (its own UI says so); its only route access is a read against canonical disk, already correct +- [x] 3.4 ~~CLI tests: mode selection (API path recorded when daemon up; file written + notice when daemon DOWN; file written + notice on 404 from an OLD daemon — a distinct test from daemon-down; 400 and 401 fail the command with NO file write); existing `WebhooksCommandTests` stay green unchanged in file mode~~ REWORKED: CLI tests per daemon answer (API path recorded and NO file written when the daemon is up; daemon DOWN → exit 1, unreachable message, NO file written; 404 from an OLD daemon → exit 1, upgrade message, NO file written — a distinct test from daemon-down; 400, 401, and 403 fail with the daemon's message and NO file write). In `WebhooksCommandTests`, the tests that stop at argument grammar, at the merge preview, or at `--dry-run` stay green unchanged with no daemon; the eight tests that reached a file write now drive a `FakeWebhookDaemon` and assert the patch the CLI sent +- [x] 3.5 RESOLVED NOT APPLICABLE with 3.3: no view-model route save exists to fake-fail; the command-level 400/401 no-file-write tests cover the save-blocked-before-persistence guarantee for the surface that actually mutates routes + +## 4. Test replacement and skew guard + +- [x] 4.1 Delete `Update_serializes_read_modify_write_operations_across_store_instances_and_path_aliases` and the same choreography pattern in `Update_lock_wait_honors_cancellation`; remove the store's cross-process mutex so no store-level lock test remains +- [x] 4.2 Verify no remaining test in the repo asserts on thread-pool scheduling for this capability (grep for the choreography pattern) + +## 5. Finish + +- [x] 5.1 Update `feeds/skills/.system/files/netclaw-operations/SKILL.md` for the new endpoints and the daemon-only CLI write path; bump `metadata.version` +- [x] 5.2 Full solution build, full `Netclaw.Actors.Tests` + `Netclaw.Daemon.Tests` + `Netclaw.Cli.Tests` + `Netclaw.Configuration.Tests`, slopwatch, headers; native smoke tapes for the webhooks TUI surface if touched (Termina rule) +- [x] 5.3 `/opsx-sync` the `webhook-route-authority` spec; PR with the back-compat story in the body diff --git a/openspec/specs/webhook-route-authority/spec.md b/openspec/specs/webhook-route-authority/spec.md new file mode 100644 index 000000000..49cc7f93a --- /dev/null +++ b/openspec/specs/webhook-route-authority/spec.md @@ -0,0 +1,128 @@ +# webhook-route-authority Specification + +## Purpose + +Guarantee single-writer ownership of webhook route mutations: the daemon's +`WebhookRouteActor` is the one mutation authority, the `/api/webhooks` +resource and the webhook agent tools are thin fronts over it, and the CLI +daemon-only for route mutations. Disk stays the canonical store, and the +store takes no cross-process lock. + +## Requirements + +### Requirement: Daemon actor is the single mutation authority + +The daemon SHALL route every webhook route mutation through one `WebhookRouteActor`. The agent tools `set_webhook` and `delete_webhook` SHALL send messages to the actor and SHALL NOT call the route store directly. The actor SHALL validate a mutation before persistence and SHALL return the validation error to the caller on failure. Disk SHALL remain the canonical store: the actor persists through the route store and holds no journaled state. + +#### Scenario: Concurrent agent mutations serialize by mailbox order + +- **GIVEN** two sessions that invoke `set_webhook` for the same route at the same time +- **WHEN** both requests reach the actor +- **THEN** the actor applies them one at a time in arrival order +- **AND** the final route file reflects both read-modify-write operations with no lost update + +#### Scenario: Validation failure does not persist + +- **GIVEN** a `set_webhook` request that fails `WebhookRouteValidator` +- **WHEN** the actor processes it +- **THEN** no file write occurs +- **AND** the caller receives the validator's error + +### Requirement: The mutation message is a patch; the merged definition carries the required fields + +An upsert message SHALL be a field-level patch. A null field SHALL mean "keep the stored value", so a caller SHALL NOT need to resend a value it does not change. Two patches of different fields on the same route SHALL therefore compose instead of overwrite. + +The message SHALL require only the two fields a patch can never inherit from a file: the route name and the authority of the caller. The route name SHALL travel as a validated value object, so no unvalidated name SHALL reach a file path. Every other required field SHALL be enforced on the merged definition by `WebhookRouteValidator`, which SHALL reject a merged route without a prompt and a merged route without a verification secret. + +#### Scenario: A patch that blanks a required field is rejected + +- **GIVEN** a stored route with a prompt +- **WHEN** an upsert patches the prompt to a blank value +- **THEN** the actor rejects the merged definition with the validator's message +- **AND** the stored route file is unchanged + +### Requirement: HTTP resource fronts the actor + +The daemon SHALL expose `/api/webhooks` (list), `/api/webhooks/{name}` (get, upsert, delete) as thin handlers that ask the actor. The resource SHALL use the same authentication and exposure-mode rules as the other `/api` surfaces. The change SHALL be additive: no existing endpoint changes. Handlers SHALL map actor results to HTTP statuses: validation failure to 400, unknown route to 404, success to 200 or 204. + +#### Scenario: Upsert through the API persists through the actor + +- **GIVEN** an authenticated `PUT /api/webhooks/{name}` with a valid route body +- **WHEN** the daemon handles it +- **THEN** the actor validates and persists the route +- **AND** the response is a success status with the stored route + +#### Scenario: Unauthenticated access is rejected + +- **GIVEN** a request to `/api/webhooks` that does not carry a paired-device credential +- **WHEN** the daemon evaluates it +- **THEN** the request is rejected by the same auth rules as the other `/api` surfaces + +### Requirement: CLI route mutations require the daemon + +The CLI SHALL send every webhook route mutation to the daemon API. The CLI SHALL NOT write a route file. When the daemon does not answer, or answers 404 for the resource, or refuses the call, the command SHALL fail and SHALL leave every route file unchanged. The failure message SHALL name the state and the remedy. The CLI read subcommands (`list`, `show`, `validate`) SHALL keep reading canonical disk, because disk is the route store and `show` reveals a secret that the API never returns. Argument grammar, `--dry-run`, and the merge preview SHALL run before the daemon call and SHALL keep their own messages and exit codes. The supported daemon-absent path is a route file authored on disk outside the CLI, which the daemon loads at startup. + +#### Scenario: Daemon reachable routes through the API + +- **GIVEN** a running paired daemon +- **WHEN** the operator runs `netclaw webhooks set` +- **THEN** the CLI sends the mutation to `/api/webhooks/{name}` +- **AND** writes no file itself + +#### Scenario: Daemon down fails the command + +- **GIVEN** no running daemon +- **WHEN** the operator runs `netclaw webhooks set` with valid arguments +- **THEN** the command fails with exit code 1 +- **AND** the error names the daemon as unreachable and tells the operator to start it +- **AND** no route file is created or changed + +#### Scenario: Old daemon without the resource fails the command + +- **GIVEN** a running daemon that predates the webhook route resource +- **WHEN** the operator runs `netclaw webhooks set` and the probe answers 404 +- **THEN** the command fails with exit code 1 +- **AND** the error tells the operator to upgrade the daemon +- **AND** no route file is created or changed + +#### Scenario: Validation rejection does not bypass the daemon + +- **GIVEN** a running daemon that rejects a mutation with a validation error +- **WHEN** the CLI receives the 400 response +- **THEN** the command fails with the validator's message +- **AND** no route file is created or changed + +#### Scenario: Dry run needs no daemon + +- **GIVEN** an operator who runs `netclaw webhooks set --dry-run` +- **WHEN** the CLI validates the merged route +- **THEN** it reports the result without a daemon call +- **AND** writes no file + +### Requirement: Version-skew tolerance without a cross-process lock + +The route store SHALL hold no cross-process lock. Version-skew tolerance SHALL rest on two properties instead. First, the actor SHALL hold no cache: every read and every read-modify-write SHALL go through the store to disk, so a direct file write by an old CLI is visible to the next actor operation without a reconciliation step. Second, the store SHALL write each route file atomically through a temporary file and one replacing move, so no reader SHALL see a partial file. The per-route JSON file format SHALL NOT change. + +Accepted edge case: if an old CLI patches the same route at the same moment as the daemon actor, one of the two updates is lost. Webhook route mutations are rare, so the project accepts this risk rather than a lock. + +#### Scenario: Old CLI writes a file behind the actor + +- **GIVEN** a running new daemon and an old CLI that writes a route file directly +- **WHEN** any subsequent read or update reaches the actor +- **THEN** the actor serves the file's current content from disk + +#### Scenario: A write never exposes a partial file + +- **GIVEN** a reader that opens a route file while the store writes it +- **WHEN** the store replaces the file +- **THEN** the reader sees either the complete old content or the complete new content + +### Requirement: Deterministic tests replace scheduling choreography + +The serialization guarantee SHALL be tested through actor message ordering and outcome assertions. No test of this capability SHALL assert on thread scheduling, bounded event waits, or elapsed time. + +#### Scenario: Serialization test is message-order based + +- **GIVEN** the actor test for concurrent mutations +- **WHEN** it runs on a starved thread pool +- **THEN** it still passes or fails only on the serialization outcome diff --git a/scripts/smoke/run-smoke.sh b/scripts/smoke/run-smoke.sh index e05bd0987..0e7bb2176 100755 --- a/scripts/smoke/run-smoke.sh +++ b/scripts/smoke/run-smoke.sh @@ -67,6 +67,7 @@ LIGHT_SCENARIOS=( reminders pairing mcp-setup + webhook-routes ) FULL_SCENARIOS=("${LIGHT_SCENARIOS[@]}") diff --git a/src/Netclaw.Actors.Tests/Tools/DispatchingToolExecutorTests.cs b/src/Netclaw.Actors.Tests/Tools/DispatchingToolExecutorTests.cs index fba40b4a3..803a514e6 100644 --- a/src/Netclaw.Actors.Tests/Tools/DispatchingToolExecutorTests.cs +++ b/src/Netclaw.Actors.Tests/Tools/DispatchingToolExecutorTests.cs @@ -2589,6 +2589,10 @@ public void Profile_exposes_configured_tools_and_hides_the_rest( var paths = new NetclawPaths(Path.Combine(Path.GetTempPath(), $"netclaw-{audience}-tools-{Guid.NewGuid():N}")); paths.EnsureDirectoriesExist(); registry.WithFirstPartyTools(config, paths: paths, pathPolicy: new ToolPathPolicy([]), shellCommandPolicy: new ShellCommandPolicy(), toolAccessPolicy: policy, webhookRouteStore: new WebhookRouteStore(paths)); + // set_webhook and delete_webhook ask WebhookRouteActor. This test reads + // exposure metadata only and never executes them, so an unresolvable + // actor reference is enough to put them in the registry. + registry.WithWebhookRouteTools(ActorRefs.Nobody); foreach (var toolName in exposedToolNames) { diff --git a/src/Netclaw.Actors.Tests/Tools/SetWebhookToolProvenanceTests.cs b/src/Netclaw.Actors.Tests/Tools/SetWebhookToolProvenanceTests.cs index dfd2faca0..93646dc06 100644 --- a/src/Netclaw.Actors.Tests/Tools/SetWebhookToolProvenanceTests.cs +++ b/src/Netclaw.Actors.Tests/Tools/SetWebhookToolProvenanceTests.cs @@ -3,7 +3,11 @@ // Copyright (C) 2026 - 2026 Petabridge, LLC // // ----------------------------------------------------------------------- +using Akka.Actor; +using Akka.Hosting; +using Akka.Hosting.TestKit; using Netclaw.Actors.Tools; +using Netclaw.Actors.Webhooks; using Netclaw.Configuration; using Netclaw.Tests.Utilities; using Netclaw.Tools; @@ -16,26 +20,33 @@ namespace Netclaw.Actors.Tests.Tools; /// audience (transitive provenance, matching set_reminder) and cannot be /// minted above the creator's authority (downgrade-only escalation guard). /// -public sealed class SetWebhookToolProvenanceTests : IDisposable +public class SetWebhookToolProvenanceTests : TestKit, IDisposable { private readonly DisposableTempDir _dir = new(); - private readonly WebhookRouteStore _store; + private WebhookRouteStore _store = null!; + private IActorRef _routeActor = null!; - public SetWebhookToolProvenanceTests() + public SetWebhookToolProvenanceTests(ITestOutputHelper output) : base(output: output) { } + + protected override void ConfigureAkka(AkkaConfigurationBuilder builder, IServiceProvider provider) { var paths = new NetclawPaths(_dir.Path); paths.EnsureDirectoriesExist(); _store = new WebhookRouteStore(paths); + builder.StartActors((system, _, _) => + { + _routeActor = system.ActorOf(WebhookRouteActor.CreateProps(_store), "webhook-routes"); + }); } - public void Dispose() => _dir.Dispose(); + void IDisposable.Dispose() => _dir.Dispose(); private static ToolExecutionContext Context(TrustAudience audience) => TestToolExecutionContext.CreateUnbound(new TestToolExecutionContextOptions { Audience = audience }); private async Task CreateRouteAsync(string routeName, TrustAudience creator, string? requestedAudience) { - var tool = new SetWebhookTool(_store); + var tool = new SetWebhookTool(_routeActor); var args = new Dictionary { ["RouteName"] = routeName, @@ -84,7 +95,7 @@ public async Task Requested_audience_above_creator_is_rejected_and_not_persisted [Fact] public async Task Notify_instructions_require_notification_target() { - var tool = new SetWebhookTool(_store); + var tool = new SetWebhookTool(_routeActor); var result = await tool.ExecuteAsync(new Dictionary { @@ -103,7 +114,7 @@ public async Task Notify_instructions_require_notification_target() [Fact] public async Task Timestamped_hmac_settings_are_persisted() { - var tool = new SetWebhookTool(_store); + var tool = new SetWebhookTool(_routeActor); var result = await tool.ExecuteAsync(new Dictionary { @@ -131,7 +142,7 @@ public async Task Timestamped_hmac_settings_are_persisted() [Fact] public async Task Timestamp_settings_are_rejected_for_body_hmac() { - var tool = new SetWebhookTool(_store); + var tool = new SetWebhookTool(_routeActor); var result = await tool.ExecuteAsync(new Dictionary { @@ -149,7 +160,7 @@ public async Task Timestamp_settings_are_rejected_for_body_hmac() [Fact] public async Task Update_preserves_omitted_route_and_verification_settings() { - var tool = new SetWebhookTool(_store); + var tool = new SetWebhookTool(_routeActor); var createResult = await tool.ExecuteAsync(new Dictionary { ["RouteName"] = "stripe-events", @@ -210,7 +221,7 @@ public async Task Lower_audience_cannot_update_higher_audience_route() { var createResult = await CreateRouteAsync("team-route", TrustAudience.Team, requestedAudience: null); Assert.DoesNotContain("Error", createResult); - var tool = new SetWebhookTool(_store); + var tool = new SetWebhookTool(_routeActor); var updateResult = await tool.ExecuteAsync(new Dictionary { @@ -234,7 +245,7 @@ public async Task Lower_audience_cannot_update_higher_audience_route() [InlineData("téstamp")] public async Task Unusable_timestamp_field_names_are_rejected_before_save(string timestampField) { - var tool = new SetWebhookTool(_store); + var tool = new SetWebhookTool(_routeActor); var result = await tool.ExecuteAsync(new Dictionary { diff --git a/src/Netclaw.Actors.Tests/Webhooks/WebhookRouteActorTests.cs b/src/Netclaw.Actors.Tests/Webhooks/WebhookRouteActorTests.cs new file mode 100644 index 000000000..2708ee37e --- /dev/null +++ b/src/Netclaw.Actors.Tests/Webhooks/WebhookRouteActorTests.cs @@ -0,0 +1,327 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- +using Akka.Actor; +using Akka.Hosting; +using Akka.Hosting.TestKit; +using Netclaw.Actors.Hosting; +using Netclaw.Actors.Webhooks; +using Netclaw.Configuration; +using Netclaw.Tests.Utilities; +using Xunit; +using static Netclaw.Actors.Webhooks.WebhookRouteProtocol; + +namespace Netclaw.Actors.Tests.Webhooks; + +/// +/// The actor is the single mutation authority for webhook route files. These +/// tests assert on message outcomes and on the resulting file, never on thread +/// scheduling or elapsed time. +/// +public class WebhookRouteActorTests : TestKit +{ + private readonly DisposableTempDir _dir = new(); + private NetclawPaths _paths = null!; + private WebhookRouteStore _store = null!; + + public WebhookRouteActorTests(ITestOutputHelper output) : base(output: output) { } + + protected override void ConfigureAkka(AkkaConfigurationBuilder builder, IServiceProvider provider) + { + _paths = new NetclawPaths(_dir.Path); + _paths.EnsureDirectoriesExist(); + _store = new WebhookRouteStore(_paths); + + builder.StartActors((system, registry, _) => + { + var actor = system.ActorOf(WebhookRouteActor.CreateProps(_store), "webhook-routes"); + registry.Register(actor); + }); + } + + protected override async Task AfterAllAsync() + { + _dir.Dispose(); + await base.AfterAllAsync(); + } + + private IActorRef RouteActor => ActorRegistry.For(Sys).Get(); + + private static UpsertRoute NewRoute(string routeName) => new() + { + RouteName = WebhookRouteName.Create(routeName), + CreatorAudience = TrustAudience.Personal, + Prompt = "Handle inbound delivery.", + Secret = "original-secret", + VerificationKind = WebhookVerifierKind.Hmac + }; + + private string RouteFilePath(string routeName) + => Path.Combine(_paths.WebhooksDirectory, $"{routeName}.json"); + + private async Task CreateRouteAsync(string routeName) + { + var created = await RouteActor.Ask( + NewRoute(routeName), TestContext.Current.CancellationToken); + Assert.Equal(RouteSaveOutcome.Created, created.Outcome); + } + + /// + /// The lost-update proof. Two field-level patches of the same route arrive + /// back to back. Each is a read-modify-write inside one message turn, so the + /// second patch reads the first patch's result and neither field is lost. + /// The mailbox does the serializing — no thread choreography is involved. + /// + [Fact] + public async Task Concurrent_field_level_updates_lose_neither_field() + { + await CreateRouteAsync("concurrent-route"); + + // Both patches are in the mailbox before either reply is read. + RouteActor.Tell( + new UpsertRoute + { + RouteName = WebhookRouteName.Create("concurrent-route"), + CreatorAudience = TrustAudience.Personal, + Prompt = "Patched by the first writer." + }, + TestActor); + RouteActor.Tell( + new UpsertRoute + { + RouteName = WebhookRouteName.Create("concurrent-route"), + CreatorAudience = TrustAudience.Personal, + RateLimitPerMinute = 99 + }, + TestActor); + + var first = await ExpectMsgAsync(cancellationToken: TestContext.Current.CancellationToken); + var second = await ExpectMsgAsync(cancellationToken: TestContext.Current.CancellationToken); + // Both patches found the route on disk, so both report Updated. + Assert.Equal(RouteSaveOutcome.Updated, first.Outcome); + Assert.Equal(RouteSaveOutcome.Updated, second.Outcome); + + var response = await RouteActor.Ask( + new GetRoute(WebhookRouteName.Create("concurrent-route")), TestContext.Current.CancellationToken); + + Assert.True(response.Found); + var route = Assert.IsType(response.Route); + Assert.Equal("Patched by the first writer.", route.Prompt); + Assert.Equal(99, route.RateLimitPerMinute); + // Neither patch carried a secret, so both preserved the stored one. + Assert.Equal(new SensitiveString("original-secret"), route.Verification.Secret); + } + + [Fact] + public async Task Validation_rejection_writes_no_file_for_a_new_route() + { + var response = await RouteActor.Ask( + new UpsertRoute + { + RouteName = WebhookRouteName.Create("invalid-new-route"), + CreatorAudience = TrustAudience.Personal, + Prompt = "Handle inbound delivery." + // No secret: WebhookRouteValidator rejects the merged definition. + }, + TestContext.Current.CancellationToken); + + Assert.Equal(RouteSaveOutcome.ValidationRejected, response.Outcome); + Assert.Equal("Verification secret is required.", response.ErrorMessage); + Assert.False(File.Exists(RouteFilePath("invalid-new-route"))); + } + + [Fact] + public async Task Validation_rejection_leaves_an_existing_route_file_unchanged() + { + await CreateRouteAsync("guarded-route"); + var before = await File.ReadAllTextAsync( + RouteFilePath("guarded-route"), TestContext.Current.CancellationToken); + + var response = await RouteActor.Ask( + new UpsertRoute + { + RouteName = WebhookRouteName.Create("guarded-route"), + CreatorAudience = TrustAudience.Personal, + MaxBodyBytes = 0 + }, + TestContext.Current.CancellationToken); + + Assert.Equal(RouteSaveOutcome.ValidationRejected, response.Outcome); + Assert.Equal("MaxBodyBytes must be >= 1.", response.ErrorMessage); + + var after = await File.ReadAllTextAsync( + RouteFilePath("guarded-route"), TestContext.Current.CancellationToken); + Assert.Equal(before, after); + } + + /// + /// Required-ness lives on the merged definition, not on the patch. The + /// patch may leave the prompt out, but the merged route may not: a webhook + /// without a prompt has nothing to run. + /// + [Theory] + [InlineData(" ")] + [InlineData("")] + public async Task A_patch_that_blanks_the_prompt_is_rejected(string blankPrompt) + { + await CreateRouteAsync("prompted-route"); + + var response = await RouteActor.Ask( + new UpsertRoute + { + RouteName = WebhookRouteName.Create("prompted-route"), + CreatorAudience = TrustAudience.Personal, + Prompt = blankPrompt + }, + TestContext.Current.CancellationToken); + + Assert.Equal(RouteSaveOutcome.ValidationRejected, response.Outcome); + Assert.Equal("Prompt is required.", response.ErrorMessage); + Assert.True(_store.TryGet("prompted-route", out var stored)); + Assert.Equal("Handle inbound delivery.", stored.Definition!.Prompt); + } + + [Fact] + public async Task A_route_above_the_creator_authority_is_not_overwritten() + { + await CreateRouteAsync("personal-route"); + + var response = await RouteActor.Ask( + new UpsertRoute + { + RouteName = WebhookRouteName.Create("personal-route"), + CreatorAudience = TrustAudience.Public, + Prompt = "Take over the route." + }, + TestContext.Current.CancellationToken); + + Assert.Equal(RouteSaveOutcome.AuthorityRejected, response.Outcome); + Assert.True(_store.TryGet("personal-route", out var stored)); + Assert.Equal("Handle inbound delivery.", stored.Definition!.Prompt); + } + + /// + /// The security audit trail. A refused mutation is the one signal that a + /// caller tried to take over authority above its own, so the actor records + /// it at warning level with both audiences. + /// + [Fact] + public async Task An_authority_rejection_is_recorded_in_the_log() + { + await CreateRouteAsync("audited-route"); + + await EventFilter + .Warning(contains: "audited-route") + .ExpectOneAsync(async () => + { + var response = await RouteActor.Ask( + new UpsertRoute + { + RouteName = WebhookRouteName.Create("audited-route"), + CreatorAudience = TrustAudience.Public, + Prompt = "Take over the route." + }, + TestContext.Current.CancellationToken); + Assert.Equal(RouteSaveOutcome.AuthorityRejected, response.Outcome); + }, + TestContext.Current.CancellationToken); + } + + /// + /// A new incarnation of the actor serves the route the previous incarnation + /// wrote. The actor keeps no cache, so a restart has nothing to lose and + /// rebuilds its whole answer from the route files. + /// + [Fact] + public async Task A_new_incarnation_rebuilds_its_answers_from_disk() + { + await CreateRouteAsync("survivor-route"); + + await WatchAsync(RouteActor); + RouteActor.Tell(PoisonPill.Instance); + await ExpectTerminatedAsync(RouteActor, cancellationToken: TestContext.Current.CancellationToken); + + var replacement = Sys.ActorOf(WebhookRouteActor.CreateProps(_store)); + var response = await replacement.Ask( + new GetRoute(WebhookRouteName.Create("survivor-route")), TestContext.Current.CancellationToken); + + Assert.True(response.Found); + Assert.Equal("Handle inbound delivery.", response.Route!.Prompt); + } + + /// + /// Version-skew tolerance. An old CLI writes a route file directly, behind + /// the actor. The next read through the actor returns the file's content + /// because every read goes to disk. + /// + [Fact] + public async Task An_external_writer_change_is_visible_to_the_next_actor_read() + { + await CreateRouteAsync("skew-route"); + + // A second store instance stands in for the old CLI process. + var externalWriter = new WebhookRouteStore(_paths); + externalWriter.Save("skew-route", new WebhookRouteConfig + { + Prompt = "Written by an old CLI.", + RateLimitPerMinute = 7, + Verification = new WebhookVerificationConfig + { + Kind = WebhookVerifierKind.Hmac, + Secret = new SensitiveString("external-secret") + } + }); + + var response = await RouteActor.Ask( + new GetRoute(WebhookRouteName.Create("skew-route")), TestContext.Current.CancellationToken); + + Assert.True(response.Found); + Assert.Equal("Written by an old CLI.", response.Route!.Prompt); + Assert.Equal(7, response.Route.RateLimitPerMinute); + + // A later patch merges onto the external content instead of the actor's + // pre-skew view of the route. + var patched = await RouteActor.Ask( + new UpsertRoute + { + RouteName = WebhookRouteName.Create("skew-route"), + CreatorAudience = TrustAudience.Personal, + MaxBodyBytes = 2048 + }, + TestContext.Current.CancellationToken); + + Assert.Equal(RouteSaveOutcome.Updated, patched.Outcome); + Assert.Equal("Written by an old CLI.", patched.Route!.Prompt); + Assert.Equal(2048, patched.Route.MaxBodyBytes); + } + + [Fact] + public async Task Delete_reports_whether_the_route_existed() + { + await CreateRouteAsync("doomed-route"); + + var deleted = await RouteActor.Ask( + new DeleteRoute(WebhookRouteName.Create("doomed-route")), TestContext.Current.CancellationToken); + Assert.True(deleted.Found); + Assert.False(File.Exists(RouteFilePath("doomed-route"))); + + var again = await RouteActor.Ask( + new DeleteRoute(WebhookRouteName.Create("doomed-route")), TestContext.Current.CancellationToken); + Assert.False(again.Found); + } + + [Fact] + public async Task List_returns_every_route_file() + { + await CreateRouteAsync("alpha-route"); + await CreateRouteAsync("beta-route"); + + var response = await RouteActor.Ask( + ListRoutes.Instance, TestContext.Current.CancellationToken); + + Assert.Equal(["alpha-route", "beta-route"], response.Routes.Select(x => x.RouteName)); + Assert.All(response.Routes, entry => Assert.NotNull(entry.Definition)); + } +} diff --git a/src/Netclaw.Actors/Hosting/ActorRegistryKeys.cs b/src/Netclaw.Actors/Hosting/ActorRegistryKeys.cs index 6316b89d4..64f08e2b2 100644 --- a/src/Netclaw.Actors/Hosting/ActorRegistryKeys.cs +++ b/src/Netclaw.Actors/Hosting/ActorRegistryKeys.cs @@ -64,6 +64,14 @@ public sealed class BackgroundJobManagerActorKey; /// public sealed class SessionLogDispatcherActorKey; +/// +/// Marker type for lookup of the +/// webhook route actor. The actor is the single mutation authority for +/// webhook route files; the set_webhook and delete_webhook tools +/// and the /api/webhooks resource resolve it to ask for a mutation. +/// +public sealed class WebhookRouteActorKey; + /// /// Marker type for lookup of the /// Discord gateway parent actor (DiscordGatewayActor -> DiscordSessionBindingActor). diff --git a/src/Netclaw.Actors/Hosting/NetclawAkkaHostingExtensions.cs b/src/Netclaw.Actors/Hosting/NetclawAkkaHostingExtensions.cs index b56882213..4a48c8744 100644 --- a/src/Netclaw.Actors/Hosting/NetclawAkkaHostingExtensions.cs +++ b/src/Netclaw.Actors/Hosting/NetclawAkkaHostingExtensions.cs @@ -17,6 +17,7 @@ using Netclaw.Actors.Serialization; using Netclaw.Actors.Sessions; using Netclaw.Actors.Tools; +using Netclaw.Actors.Webhooks; using Netclaw.Security; namespace Netclaw.Actors.Hosting; @@ -133,6 +134,28 @@ public static AkkaConfigurationBuilder WithToolApprovalActor( }); } + /// + /// Registers the webhook route actor as a singleton actor. The actor is the + /// single mutation authority for webhook route files. Requires + /// in DI. + /// + /// Registration does not depend on Webhooks.Enabled: an operator + /// configures routes before enabling delivery, and the + /// /api/webhooks management resource resolves the actor either way. + /// + /// + public static AkkaConfigurationBuilder WithWebhookRouteActor( + this AkkaConfigurationBuilder builder) + { + return builder.StartActors((system, registry, resolver) => + { + var actor = system.ActorOf( + resolver.Props(), + "webhook-routes"); + registry.Register(actor); + }); + } + public static AkkaConfigurationBuilder WithBackgroundJobManager( this AkkaConfigurationBuilder builder) => builder.WithBackgroundJobManager( diff --git a/src/Netclaw.Actors/Tools/DeleteWebhookTool.cs b/src/Netclaw.Actors/Tools/DeleteWebhookTool.cs index db7d96a49..c419d8655 100644 --- a/src/Netclaw.Actors/Tools/DeleteWebhookTool.cs +++ b/src/Netclaw.Actors/Tools/DeleteWebhookTool.cs @@ -4,8 +4,10 @@ // // ----------------------------------------------------------------------- using System.ComponentModel; +using Akka.Actor; using Netclaw.Configuration; using Netclaw.Tools; +using static Netclaw.Actors.Webhooks.WebhookRouteProtocol; namespace Netclaw.Actors.Tools; @@ -14,31 +16,34 @@ namespace Netclaw.Actors.Tools; Grant = "webhook_admin")] public sealed partial class DeleteWebhookTool : NetclawTool { - private readonly WebhookRouteStore _store; + private static readonly TimeSpan AskTimeout = TimeSpan.FromSeconds(10); + + private readonly IActorRef _routeActor; public record Params( [property: Description("Webhook route name to delete (for example 'github-issues').")] string RouteName); - public DeleteWebhookTool(WebhookRouteStore store) + public DeleteWebhookTool(IActorRef routeActor) { - _store = store; + _routeActor = routeActor; } - protected override Task ExecuteAsync(Params args, ToolInvocationContext context, CancellationToken ct) + protected override async Task ExecuteAsync(Params args, ToolInvocationContext context, CancellationToken ct) { - if (!WebhookRouteStore.TryNormalizeRouteName(args.RouteName, out var routeName, out var routeError)) - return Task.FromResult($"Error: {routeError}"); + if (!WebhookRouteName.TryCreate(args.RouteName, out var routeName, out var routeError)) + return $"Error: {routeError}"; try { - return Task.FromResult(_store.Delete(routeName, ct) - ? $"Webhook route '{routeName}' deleted." - : $"Webhook route '{routeName}' not found."); + var response = await _routeActor.Ask(new DeleteRoute(routeName), AskTimeout, ct); + return response.Found + ? $"Webhook route '{routeName.Value}' deleted." + : $"Webhook route '{routeName.Value}' not found."; } catch (TimeoutException ex) { - return Task.FromResult($"Error: {ex.Message}"); + return $"Error: {ex.Message}"; } } } diff --git a/src/Netclaw.Actors/Tools/SetWebhookTool.cs b/src/Netclaw.Actors/Tools/SetWebhookTool.cs index fadf30e5e..6a97bc559 100644 --- a/src/Netclaw.Actors/Tools/SetWebhookTool.cs +++ b/src/Netclaw.Actors/Tools/SetWebhookTool.cs @@ -4,8 +4,10 @@ // // ----------------------------------------------------------------------- using System.ComponentModel; +using Akka.Actor; using Netclaw.Configuration; using Netclaw.Tools; +using static Netclaw.Actors.Webhooks.WebhookRouteProtocol; namespace Netclaw.Actors.Tools; @@ -14,7 +16,9 @@ namespace Netclaw.Actors.Tools; Grant = "webhook_admin")] public sealed partial class SetWebhookTool : NetclawTool { - private readonly WebhookRouteStore _store; + private static readonly TimeSpan AskTimeout = TimeSpan.FromSeconds(10); + + private readonly IActorRef _routeActor; public record Params( [property: Description("Stable route name used in the webhook URL path (kebab-case, for example 'github-issues').")] @@ -60,23 +64,26 @@ public record Params( [property: Description("Accepted timestamp tolerance in seconds for HmacTimestamped routes, from 1 to 3600. Defaults to 300.")] int? ToleranceSeconds = null); - public SetWebhookTool(WebhookRouteStore store) + public SetWebhookTool(IActorRef routeActor) { - _store = store; + _routeActor = routeActor; } - protected override Task ExecuteAsync(Params args, ToolInvocationContext context, CancellationToken ct) + protected override async Task ExecuteAsync(Params args, ToolInvocationContext context, CancellationToken ct) { - if (!WebhookRouteStore.TryNormalizeRouteName(args.RouteName, out var routeName, out var routeError)) - return Task.FromResult($"Error: {routeError}"); + // The tool front parses the wire string once. Past this line the route + // name is a WebhookRouteName, so no later step can reach a file with an + // unvalidated name. + if (!WebhookRouteName.TryCreate(args.RouteName, out var routeName, out var routeError)) + return $"Error: {routeError}"; if (string.IsNullOrWhiteSpace(args.Prompt)) - return Task.FromResult("Error: 'prompt' is required."); + return "Error: 'prompt' is required."; if (string.IsNullOrWhiteSpace(args.Secret)) - return Task.FromResult("Error: 'secret' is required."); + return "Error: 'secret' is required."; if (!WebhookRouteValidator.TryParseVerifierKind(args.VerificationKind, out var verificationKind)) - return Task.FromResult("Error: 'verificationKind' must be 'Hmac', 'HmacTimestamped', or 'HeaderSecret'."); + return "Error: 'verificationKind' must be 'Hmac', 'HmacTimestamped', or 'HeaderSecret'."; if ((args.TimestampField is not null || args.SignatureField is not null @@ -84,152 +91,97 @@ protected override Task ExecuteAsync(Params args, ToolInvocationContext || args.ToleranceSeconds is not null) && verificationKind != WebhookVerifierKind.HmacTimestamped) { - return Task.FromResult("Error: Timestamp signature settings require 'verificationKind' to be 'HmacTimestamped'."); + return "Error: Timestamp signature settings require 'verificationKind' to be 'HmacTimestamped'."; } + if (!TryResolveRequestedAudience(args.Audience, out var requestedAudience, out var audienceError)) + return audienceError!; + try { - var result = _store.Update( - routeName, - ct, - existing => BuildUpdate(routeName, args, context.Audience, verificationKind, existing)); - return Task.FromResult(result); + var response = await _routeActor.Ask( + BuildCommand(routeName, args, context.Audience, verificationKind, requestedAudience), + AskTimeout, + ct); + + return response.Success + ? $"Webhook route '{routeName.Value}' saved at /api/webhooks/{routeName.Value}. Secret stored in the route file; keep it aligned with the sender configuration." + : $"Error: {response.ErrorMessage}"; } catch (InvalidDataException ex) { - return Task.FromResult($"Error: {ex.Message}"); + return $"Error: {ex.Message}"; } catch (TimeoutException ex) { - return Task.FromResult($"Error: {ex.Message}"); + return $"Error: {ex.Message}"; } } - private static (WebhookRouteConfig? Definition, string Result) BuildUpdate( - string routeName, + /// + /// Projects the tool arguments into the actor's field-level patch. The tool + /// owns the wire grammar — comma-separated events, the audience and + /// verification-kind spellings — and the actor owns the merge, the audience + /// authority check, and validation. + /// + private static UpsertRoute BuildCommand( + WebhookRouteName routeName, Params args, TrustAudience creatorAudience, WebhookVerifierKind verificationKind, - WebhookRouteConfig? existing) - { - if (existing is not null && existing.Audience > creatorAudience) - { - return (null, - $"Error: Existing route audience '{existing.Audience.ToWireValue()}' exceeds creator authority ({creatorAudience.ToWireValue()})."); - } - - TrustAudience audience; - if (string.IsNullOrWhiteSpace(args.Audience) && existing is not null) + TrustAudience? requestedAudience) => new() { - audience = existing.Audience; - } - else if (!TryResolveAudience(args.Audience, creatorAudience, out audience, out var audienceError)) - { - return (null, audienceError!); - } - - var existingVerification = existing?.Verification; - - var definition = new WebhookRouteConfig - { - Enabled = args.Enabled ?? existing?.Enabled ?? true, - Prompt = args.Prompt.Trim(), - Events = args.Events is null ? [.. existing?.Events ?? []] : ParseEvents(args.Events), - Audience = audience, - NotifyInstructions = args.NotifyInstructions?.Trim() ?? existing?.NotifyInstructions ?? string.Empty, - DeliveryRequired = args.DeliveryRequired ?? existing?.DeliveryRequired ?? true, - MaxBodyBytes = args.MaxBodyBytes ?? existing?.MaxBodyBytes ?? 1024 * 1024, - RateLimitPerMinute = args.RateLimitPerMinute ?? existing?.RateLimitPerMinute ?? 30, - Verification = new WebhookVerificationConfig - { - Kind = verificationKind, - HmacAlgorithm = existingVerification?.HmacAlgorithm ?? WebhookHmacAlgorithm.Sha256, - Secret = new SensitiveString(args.Secret), - SignatureHeaderName = args.SignatureHeaderName is null - ? existingVerification?.SignatureHeaderName - : NormalizeOptional(args.SignatureHeaderName), - SignaturePrefix = args.SignaturePrefix is null - ? existingVerification?.SignaturePrefix - : NormalizeOptional(args.SignaturePrefix, trim: false), - SecretHeaderName = args.SecretHeaderName is null - ? existingVerification?.SecretHeaderName - : NormalizeOptional(args.SecretHeaderName), - EventHeaderName = args.EventHeaderName is null - ? existingVerification?.EventHeaderName - : NormalizeOptional(args.EventHeaderName), - DeliveryIdHeaderName = args.DeliveryIdHeaderName is null - ? existingVerification?.DeliveryIdHeaderName - : NormalizeOptional(args.DeliveryIdHeaderName), - TimestampField = args.TimestampField is null - ? existingVerification?.TimestampField - : args.TimestampField, - SignatureField = args.SignatureField is null - ? existingVerification?.SignatureField - : args.SignatureField, - SignedPayloadSeparator = args.SignedPayloadSeparator ?? existingVerification?.SignedPayloadSeparator, - ToleranceSeconds = args.ToleranceSeconds ?? existingVerification?.ToleranceSeconds - } + RouteName = routeName, + CreatorAudience = creatorAudience, + RequestedAudience = requestedAudience, + Prompt = args.Prompt, + Secret = args.Secret, + VerificationKind = verificationKind, + Events = args.Events is null ? null : ParseEvents(args.Events), + NotifyInstructions = args.NotifyInstructions, + DeliveryRequired = args.DeliveryRequired, + NotificationChannelId = args.NotificationChannelId, + MaxBodyBytes = args.MaxBodyBytes, + RateLimitPerMinute = args.RateLimitPerMinute, + Enabled = args.Enabled, + SignatureHeaderName = args.SignatureHeaderName, + SignaturePrefix = args.SignaturePrefix, + SecretHeaderName = args.SecretHeaderName, + EventHeaderName = args.EventHeaderName, + DeliveryIdHeaderName = args.DeliveryIdHeaderName, + TimestampField = args.TimestampField, + SignatureField = args.SignatureField, + SignedPayloadSeparator = args.SignedPayloadSeparator, + ToleranceSeconds = args.ToleranceSeconds }; - if (args.NotificationChannelId is null && existing?.NotificationTarget is { } existingTarget) - { - definition.NotificationTarget = new NotificationTargetConfig - { - Kind = existingTarget.Kind, - ChannelId = existingTarget.ChannelId - }; - } - else if (!string.IsNullOrWhiteSpace(args.NotificationChannelId)) - { - definition.NotificationTarget = new NotificationTargetConfig - { - Kind = NotificationTargetKind.Slack, - ChannelId = args.NotificationChannelId.Trim() - }; - } - - var validationErrors = WebhookRouteValidator.Validate(routeName, definition); - if (validationErrors.Count > 0) - return (null, $"Error: {validationErrors[0]}"); - - return (definition, - $"Webhook route '{routeName}' saved at /api/webhooks/{routeName}. Secret stored in the route file; keep it aligned with the sender configuration."); - } - /// - /// Resolves the route's audience from the optional explicit argument, falling - /// back to the creating context's audience (transitive provenance, mirroring - /// set_reminder). A route may not be minted above the creator's - /// authority — downgrade-only, mirroring - /// ReminderManagerActor.ValidateRequestedAudience. A context-less - /// invocation carries the unbound tool scope's + /// Parses the optional explicit audience argument. A blank argument leaves + /// the audience unrequested, so the actor inherits the stored route's + /// audience or the creating context's audience (transitive provenance, + /// mirroring set_reminder). The actor enforces the downgrade-only + /// rule: a route may not be minted above the creator's authority. A + /// context-less invocation carries the unbound tool scope's /// , so it cannot escalate. (Routes defined /// directly in config never reach this tool; they keep /// WebhooksConfig.Audience's default.) /// - private static bool TryResolveAudience(string? requested, TrustAudience creatorAudience, out TrustAudience audience, out string? error) + private static bool TryResolveRequestedAudience(string? requested, out TrustAudience? audience, out string? error) { if (string.IsNullOrWhiteSpace(requested)) { - audience = creatorAudience; + audience = null; error = null; return true; } if (!SecurityPolicyDefaults.TryParseAudience(requested, out var parsed)) { - audience = creatorAudience; + audience = null; error = "Error: 'audience' must be Public, Team, or Personal."; return false; } - if (parsed > creatorAudience) - { - audience = creatorAudience; - error = $"Error: Requested audience '{parsed.ToWireValue()}' exceeds creator authority ({creatorAudience.ToWireValue()})."; - return false; - } - audience = parsed; error = null; return true; @@ -242,12 +194,4 @@ private static List ParseEvents(string? value) return [.. value.Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries).Where(x => !string.IsNullOrWhiteSpace(x))]; } - - private static string? NormalizeOptional(string value, bool trim = true) - { - if (string.IsNullOrWhiteSpace(value)) - return null; - - return trim ? value.Trim() : value; - } } diff --git a/src/Netclaw.Actors/Tools/ToolRegistrationExtensions.cs b/src/Netclaw.Actors/Tools/ToolRegistrationExtensions.cs index 2a6a3d6ba..1e28f09d5 100644 --- a/src/Netclaw.Actors/Tools/ToolRegistrationExtensions.cs +++ b/src/Netclaw.Actors/Tools/ToolRegistrationExtensions.cs @@ -42,11 +42,7 @@ public static ToolRegistry WithFirstPartyTools( registry.Register(new FileEditTool(config, paths, pathPolicy)); registry.Register(new AttachFileTool(config, paths, pathPolicy)); if (webhookRouteStore is not null) - { - registry.Register(new SetWebhookTool(webhookRouteStore)); registry.Register(new ListWebhooksTool(webhookRouteStore)); - registry.Register(new DeleteWebhookTool(webhookRouteStore)); - } if (searchBackend is not null) registry.Register(new WebSearchTool(searchBackend)); registry.Register(new WebFetchTool(config)); @@ -111,6 +107,21 @@ public static ToolRegistry WithReminderTools( return registry; } + /// + /// Registers the webhook route mutation tools (set, delete). Both ask the + /// , the single + /// mutation authority for route files. list_webhooks is a read and + /// stays on the store — see . + /// + public static ToolRegistry WithWebhookRouteTools( + this ToolRegistry registry, + IActorRef routeActor) + { + registry.Register(new SetWebhookTool(routeActor)); + registry.Register(new DeleteWebhookTool(routeActor)); + return registry; + } + /// /// Registers background job tools that communicate with the /// via Ask. diff --git a/src/Netclaw.Actors/Webhooks/WebhookRouteActor.cs b/src/Netclaw.Actors/Webhooks/WebhookRouteActor.cs new file mode 100644 index 000000000..b110653bc --- /dev/null +++ b/src/Netclaw.Actors/Webhooks/WebhookRouteActor.cs @@ -0,0 +1,288 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- +using Akka.Actor; +using Akka.Event; +using Netclaw.Configuration; +using static Netclaw.Actors.Webhooks.WebhookRouteProtocol; + +namespace Netclaw.Actors.Webhooks; + +/// +/// The single webhook route mutation authority inside the daemon. The agent +/// tools set_webhook and delete_webhook and the +/// /api/webhooks resource ask this actor instead of touching +/// , so concurrent read-modify-write requests +/// serialize by mailbox order rather than by lock contention. +/// +/// The actor is a plain with no journal and no +/// cache. Disk stays the canonical store: every message reads the route file +/// through the store, merges the message's fields, and writes the result back. +/// A route file that an external writer changed is therefore visible to the +/// next read with no reconciliation step, and a restart rebuilds nothing +/// because there is nothing to rebuild. +/// +/// +/// This actor is the only writer. An old CLI binary that writes a route file +/// directly during a version skew risks one lost update, and only when it +/// patches the same route at the same moment. Each write stays atomic on its +/// own, so no reader sees a partial file. At webhook mutation rates that risk +/// is accepted; it does not justify a cross-process lock. +/// +/// +public sealed class WebhookRouteActor : ReceiveActor +{ + private readonly WebhookRouteStore _store; + private readonly ILoggingAdapter _log = Context.GetLogger(); + + public WebhookRouteActor(WebhookRouteStore store) + { + _store = store; + + Receive(HandleUpsert); + Receive(HandleDelete); + Receive(HandleGet); + Receive(_ => HandleList()); + } + + private void HandleUpsert(UpsertRoute command) + { + var routeName = command.RouteName; + try + { + var outcome = _store.Update( + routeName.Value, + existing => Merge(routeName, command, existing)); + Sender.Tell(outcome); + } + catch (Exception ex) + { + // A persistence failure is never swallowed: the ask faults with the + // original exception so the tool reports it and the HTTP handler + // returns a server error. The actor keeps serving — its state lives + // on disk, so one failed write leaves nothing to repair in memory. + _log.Warning(ex, "Webhook route {RouteName} could not be saved.", routeName.Value); + Sender.Tell(new Status.Failure(ex)); + } + } + + private void HandleDelete(DeleteRoute command) + { + var routeName = command.RouteName; + try + { + Sender.Tell(new RouteDeleted(routeName, _store.Delete(routeName.Value))); + } + catch (Exception ex) + { + _log.Warning(ex, "Webhook route {RouteName} could not be deleted.", routeName.Value); + Sender.Tell(new Status.Failure(ex)); + } + } + + private void HandleGet(GetRoute query) + { + var routeName = query.RouteName; + try + { + var found = _store.TryGet(routeName.Value, out var result); + Sender.Tell(new RouteResponse(routeName, found, found ? result.Definition : null)); + } + catch (Exception ex) + { + _log.Warning(ex, "Webhook route {RouteName} could not be read.", routeName.Value); + Sender.Tell(new Status.Failure(ex)); + } + } + + private void HandleList() + { + try + { + var entries = _store.ListRouteFiles() + .Select(x => new RouteEntry(x.RouteName, x.Definition)) + .ToList(); + Sender.Tell(new RouteListResponse(entries)); + } + catch (Exception ex) + { + _log.Warning(ex, "Webhook routes could not be listed."); + Sender.Tell(new Status.Failure(ex)); + } + } + + /// + /// Applies one field-level patch to the stored route and validates the + /// result. Returns a null definition for every rejection, which tells + /// to leave the file untouched. + /// + private (WebhookRouteConfig? Definition, RouteSaved Result) Merge( + WebhookRouteName routeName, + UpsertRoute command, + WebhookRouteConfig? existing) + { + if (existing is not null && existing.Audience > command.CreatorAudience) + { + return Reject( + routeName, + command, + existing, + RouteSaveOutcome.AuthorityRejected, + $"Existing route audience '{existing.Audience.ToWireValue()}' exceeds creator authority ({command.CreatorAudience.ToWireValue()})."); + } + + TrustAudience audience; + if (command.RequestedAudience is not { } requested) + { + audience = existing?.Audience ?? command.CreatorAudience; + } + else if (requested > command.CreatorAudience) + { + return Reject( + routeName, + command, + existing, + RouteSaveOutcome.AuthorityRejected, + $"Requested audience '{requested.ToWireValue()}' exceeds creator authority ({command.CreatorAudience.ToWireValue()})."); + } + else + { + audience = requested; + } + + var existingVerification = existing?.Verification; + + // The tool and the CLI reject timestamp settings on a non-timestamped + // kind before they reach this actor, each with its own parameter + // wording. The HTTP patch surface has no such front, so the authority + // enforces the same rule here: a patch that carries timestamp settings + // while the merged kind is not HmacTimestamped would persist inert + // fields that silently activate when the kind is later flipped. + var mergedKind = command.VerificationKind ?? existingVerification?.Kind ?? WebhookVerifierKind.Hmac; + var patchHasTimestampSettings = + command.TimestampField is not null + || command.SignatureField is not null + || command.SignedPayloadSeparator is not null + || command.ToleranceSeconds is not null; + if (mergedKind != WebhookVerifierKind.HmacTimestamped && patchHasTimestampSettings) + { + return Reject( + routeName, + command, + existing, + RouteSaveOutcome.ValidationRejected, + "Timestamp verification settings require verification kind 'hmac-timestamped'."); + } + + var definition = new WebhookRouteConfig + { + Enabled = command.Enabled ?? existing?.Enabled ?? true, + Prompt = command.Prompt?.Trim() ?? existing?.Prompt ?? string.Empty, + Events = command.Events is null ? [.. existing?.Events ?? []] : [.. command.Events], + Audience = audience, + NotifyInstructions = command.NotifyInstructions?.Trim() ?? existing?.NotifyInstructions ?? string.Empty, + DeliveryRequired = command.DeliveryRequired ?? existing?.DeliveryRequired ?? true, + MaxBodyBytes = command.MaxBodyBytes ?? existing?.MaxBodyBytes ?? 1024 * 1024, + RateLimitPerMinute = command.RateLimitPerMinute ?? existing?.RateLimitPerMinute ?? 30, + Verification = new WebhookVerificationConfig + { + Kind = command.VerificationKind ?? existingVerification?.Kind ?? WebhookVerifierKind.Hmac, + HmacAlgorithm = existingVerification?.HmacAlgorithm ?? WebhookHmacAlgorithm.Sha256, + Secret = command.Secret is null + ? existingVerification?.Secret + : new SensitiveString(command.Secret), + SignatureHeaderName = command.SignatureHeaderName is null + ? existingVerification?.SignatureHeaderName + : NormalizeOptional(command.SignatureHeaderName), + SignaturePrefix = command.SignaturePrefix is null + ? existingVerification?.SignaturePrefix + : NormalizeOptional(command.SignaturePrefix, trim: false), + SecretHeaderName = command.SecretHeaderName is null + ? existingVerification?.SecretHeaderName + : NormalizeOptional(command.SecretHeaderName), + EventHeaderName = command.EventHeaderName is null + ? existingVerification?.EventHeaderName + : NormalizeOptional(command.EventHeaderName), + DeliveryIdHeaderName = command.DeliveryIdHeaderName is null + ? existingVerification?.DeliveryIdHeaderName + : NormalizeOptional(command.DeliveryIdHeaderName), + TimestampField = command.TimestampField ?? existingVerification?.TimestampField, + SignatureField = command.SignatureField ?? existingVerification?.SignatureField, + SignedPayloadSeparator = command.SignedPayloadSeparator ?? existingVerification?.SignedPayloadSeparator, + ToleranceSeconds = command.ToleranceSeconds ?? existingVerification?.ToleranceSeconds + } + }; + + if (command.NotificationChannelId is null && existing?.NotificationTarget is { } existingTarget) + { + definition.NotificationTarget = new NotificationTargetConfig + { + Kind = existingTarget.Kind, + ChannelId = existingTarget.ChannelId + }; + } + else if (!string.IsNullOrWhiteSpace(command.NotificationChannelId)) + { + definition.NotificationTarget = new NotificationTargetConfig + { + Kind = NotificationTargetKind.Slack, + ChannelId = command.NotificationChannelId.Trim() + }; + } + + var validationErrors = WebhookRouteValidator.Validate(routeName.Value, definition); + if (validationErrors.Count > 0) + { + return Reject( + routeName, + command, + existing, + RouteSaveOutcome.ValidationRejected, + validationErrors[0]); + } + + return (definition, new RouteSaved( + routeName, + existing is null ? RouteSaveOutcome.Created : RouteSaveOutcome.Updated, + definition)); + } + + /// + /// Builds a rejection reply and records it. A refused route mutation is a + /// security event: it is the one signal that a caller tried to take over or + /// to mint authority above its own. The record names the route, both + /// audiences, and the reason. It never names the route secret. + /// + private (WebhookRouteConfig? Definition, RouteSaved Result) Reject( + WebhookRouteName routeName, + UpsertRoute command, + WebhookRouteConfig? existing, + RouteSaveOutcome outcome, + string reason) + { + _log.Warning( + "Webhook route {RouteName} rejected ({RejectionKind}). Creator audience {CreatorAudience}, " + + "requested audience {RequestedAudience}, stored audience {StoredAudience}. Reason: {Reason}", + routeName.Value, + outcome, + command.CreatorAudience.ToWireValue(), + command.RequestedAudience?.ToWireValue() ?? "(inherited)", + existing?.Audience.ToWireValue() ?? "(new route)", + reason); + + return (null, new RouteSaved(routeName, outcome, Route: null, reason)); + } + + private static string? NormalizeOptional(string value, bool trim = true) + { + if (string.IsNullOrWhiteSpace(value)) + return null; + + return trim ? value.Trim() : value; + } + + public static Props CreateProps(WebhookRouteStore store) + => Props.Create(() => new WebhookRouteActor(store)); +} diff --git a/src/Netclaw.Actors/Webhooks/WebhookRouteProtocol.cs b/src/Netclaw.Actors/Webhooks/WebhookRouteProtocol.cs new file mode 100644 index 000000000..09ad63dd4 --- /dev/null +++ b/src/Netclaw.Actors/Webhooks/WebhookRouteProtocol.cs @@ -0,0 +1,203 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- +using Akka.Actor; +using Netclaw.Configuration; + +namespace Netclaw.Actors.Webhooks; + +/// +/// Message contract for , the single webhook +/// route mutation authority inside the daemon. +/// +/// Every message is local-only. The actor holds no cluster identity and the +/// payloads carry mutable instances, so each +/// message opts out of serialization verification. +/// +/// +public static class WebhookRouteProtocol +{ + /// Marker for webhook route mutations. + public interface IWebhookRouteCommand; + + /// Marker for webhook route reads. + public interface IWebhookRouteQuery; + + /// Marker for webhook route replies. + public interface IWebhookRouteResponse; + + // ===== Commands ===== + + /// + /// Field-level route patch. The actor reads the stored route, applies the + /// fields this message carries, validates the merged definition, and writes + /// it back — all inside one message turn. + /// + /// The mutation travels as DATA, never as a delegate. A null property means + /// "leave the stored value unchanged", which is what both set_webhook + /// and netclaw webhooks set already mean by an omitted argument. Two + /// concurrent patches of different fields therefore compose instead of + /// overwriting each other. + /// + /// + /// Almost every field is nullable because this is a patch, not a full + /// definition. Null does not mean "absent"; it means "keep what the file + /// holds". A required field here would force every caller to resend values + /// it does not want to change, and the resend would overwrite a concurrent + /// patch of that same field. + /// + /// Required-ness belongs to the merged definition instead, and + /// is the one place that enforces it. + /// A route needs a prompt and a verification secret, so the validator + /// rejects a merged definition without them, whatever the patch omitted. + /// The two fields that this message does require are the ones a patch can + /// never inherit from a file: the route to patch, and the authority of the + /// caller. + /// + /// + /// + public sealed record UpsertRoute : IWebhookRouteCommand, INoSerializationVerificationNeeded + { + /// The route to create or to patch. + public required WebhookRouteName RouteName { get; init; } + + /// + /// Authority of the caller that requested the mutation. A route may not + /// be minted or updated above this audience — the downgrade-only guard + /// that keeps a low-authority session from taking over a high-authority + /// route. + /// + public required TrustAudience CreatorAudience { get; init; } + + /// + /// Audience explicitly requested for the route. Null inherits the stored + /// audience, or for a new route. + /// + public TrustAudience? RequestedAudience { get; init; } + + public string? Prompt { get; init; } + + public string? Secret { get; init; } + + public WebhookVerifierKind? VerificationKind { get; init; } + + public IReadOnlyList? Events { get; init; } + + public string? NotifyInstructions { get; init; } + + public bool? DeliveryRequired { get; init; } + + /// + /// Slack channel for human-facing notifications. A blank (but non-null) + /// value clears the stored notification target. + /// + public string? NotificationChannelId { get; init; } + + public int? MaxBodyBytes { get; init; } + + public int? RateLimitPerMinute { get; init; } + + public bool? Enabled { get; init; } + + public string? SignatureHeaderName { get; init; } + + public string? SignaturePrefix { get; init; } + + public string? SecretHeaderName { get; init; } + + public string? EventHeaderName { get; init; } + + public string? DeliveryIdHeaderName { get; init; } + + public string? TimestampField { get; init; } + + public string? SignatureField { get; init; } + + public string? SignedPayloadSeparator { get; init; } + + public int? ToleranceSeconds { get; init; } + } + + /// Removes one route file. + public sealed record DeleteRoute(WebhookRouteName RouteName) + : IWebhookRouteCommand, INoSerializationVerificationNeeded; + + // ===== Queries ===== + + /// Reads one route from disk. + public sealed record GetRoute(WebhookRouteName RouteName) + : IWebhookRouteQuery, INoSerializationVerificationNeeded; + + /// Reads every route file from disk. + public sealed record ListRoutes : IWebhookRouteQuery, INoSerializationVerificationNeeded + { + public static readonly ListRoutes Instance = new(); + } + + // ===== Responses ===== + + /// + /// What the actor did with an . The four states are + /// exclusive, so one enum replaces the success flag, the created flag, and + /// the separate error code that could disagree with each other. + /// + public enum RouteSaveOutcome + { + /// The actor wrote a route file that did not exist before. + Created = 0, + + /// The actor merged the patch into an existing route file. + Updated = 1, + + /// The merged definition failed validation. No file changed. + ValidationRejected = 2, + + /// The caller lacks the authority for the route. No file changed. + AuthorityRejected = 3 + } + + /// + /// Outcome of an . carries + /// the stored definition on success, including the secret, so callers that + /// project it to an external surface must strip the secret first. A + /// rejection carries a null route and the operator-facing reason. + /// + public sealed record RouteSaved( + WebhookRouteName RouteName, + RouteSaveOutcome Outcome, + WebhookRouteConfig? Route, + string? ErrorMessage = null) : IWebhookRouteResponse, INoSerializationVerificationNeeded + { + /// True when the actor wrote the route file. + public bool Success => Outcome is RouteSaveOutcome.Created or RouteSaveOutcome.Updated; + } + + /// Outcome of a . + public sealed record RouteDeleted(WebhookRouteName RouteName, bool Found) + : IWebhookRouteResponse, INoSerializationVerificationNeeded; + + /// + /// Outcome of a . reports + /// whether the file exists; a found route with a null + /// is a file that exists but does not parse. + /// + public sealed record RouteResponse(WebhookRouteName RouteName, bool Found, WebhookRouteConfig? Route) + : IWebhookRouteResponse, INoSerializationVerificationNeeded; + + /// + /// One entry of a . A null + /// is a route file that does not parse. + /// + /// The name stays a string here. This entry reports what the webhooks + /// directory holds, and an operator can drop a file there whose name is not + /// a valid route name. The list must show that file, not hide it. + /// + /// + public sealed record RouteEntry(string RouteName, WebhookRouteConfig? Definition); + + /// Outcome of a . + public sealed record RouteListResponse(IReadOnlyList Routes) + : IWebhookRouteResponse, INoSerializationVerificationNeeded; +} diff --git a/src/Netclaw.Cli.Tests/Cli/UpdateCommandTests.cs b/src/Netclaw.Cli.Tests/Cli/UpdateCommandTests.cs index 14395edf9..ce7e96df1 100644 --- a/src/Netclaw.Cli.Tests/Cli/UpdateCommandTests.cs +++ b/src/Netclaw.Cli.Tests/Cli/UpdateCommandTests.cs @@ -17,7 +17,7 @@ namespace Netclaw.Cli.Tests.Cli; -[Collection("Update verification")] +[Collection(ConsoleRedirectionCollection.Name)] public sealed class UpdateCommandTests : IDisposable { /// diff --git a/src/Netclaw.Cli.Tests/ConsoleRedirectionCollection.cs b/src/Netclaw.Cli.Tests/ConsoleRedirectionCollection.cs new file mode 100644 index 000000000..7581e3986 --- /dev/null +++ b/src/Netclaw.Cli.Tests/ConsoleRedirectionCollection.cs @@ -0,0 +1,20 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- +using Xunit; + +namespace Netclaw.Cli.Tests; + +/// +/// Groups every test class that redirects . The CLI +/// writes failures to Console.Error, which is process-wide state, so two +/// classes that swap it at the same time can restore each other's writer. This +/// collection runs alone, so the swap is always safe. +/// +[CollectionDefinition(Name, DisableParallelization = true)] +public sealed class ConsoleRedirectionCollection +{ + public const string Name = "Console redirection"; +} diff --git a/src/Netclaw.Cli.Tests/Webhooks/FakeWebhookDaemon.cs b/src/Netclaw.Cli.Tests/Webhooks/FakeWebhookDaemon.cs new file mode 100644 index 000000000..8966eff12 --- /dev/null +++ b/src/Netclaw.Cli.Tests/Webhooks/FakeWebhookDaemon.cs @@ -0,0 +1,93 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- +using System.Net; +using System.Text; +using System.Text.Json; +using Microsoft.Extensions.Configuration; +using Netclaw.Cli.Config; +using Netclaw.Cli.Daemon; +using Netclaw.Configuration; +using Netclaw.Tests.Utilities; + +namespace Netclaw.Cli.Tests.Webhooks; + +/// +/// A daemon that answers the webhook route resource, and the record of what the +/// CLI asked it. Route mutations are daemon-only, so every write test needs one. +/// +internal sealed class FakeWebhookDaemon +{ + private readonly List _calls = []; + + /// + /// Creates the fake. answers each request; the + /// fake records the method, path, and body before it runs. + /// + public FakeWebhookDaemon(NetclawPaths paths, Func respond) + { + ClientConfigFile.WriteEndpoint(paths, "http://127.0.0.1:5199"); + Api = new DaemonApi(new FakeHttpClientFactory(request => Record(request, respond)), new ConfigurationBuilder().Build(), paths); + } + + /// The client the CLI calls. + public DaemonApi Api { get; } + + /// Every request the CLI made, in order. + public IReadOnlyList Calls => _calls; + + /// A daemon that accepts the probe, every upsert, and every delete. + public static FakeWebhookDaemon Healthy(NetclawPaths paths) + => new(paths, request => request.Method == HttpMethod.Delete + ? new HttpResponseMessage(HttpStatusCode.NoContent) + : RouteList()); + + /// A daemon that is not running: the transport never connects. + public static FakeWebhookDaemon Unreachable(NetclawPaths paths) + => new(paths, _ => throw new HttpRequestException("connection refused")); + + /// An older daemon: it answers, but has no webhook route resource. + public static FakeWebhookDaemon WithoutRouteResource(NetclawPaths paths) + => new(paths, _ => new HttpResponseMessage(HttpStatusCode.NotFound)); + + /// The empty route list that the availability probe reads. + public static HttpResponseMessage RouteList() + => Json(HttpStatusCode.OK, Array.Empty()); + + public static HttpResponseMessage Json(HttpStatusCode status, T body) + => new(status) + { + Content = new StringContent(JsonSerializer.Serialize(body), Encoding.UTF8, "application/json") + }; + + /// Reads the recorded body of the single upsert the CLI sent. + public JsonDocument SingleUpsertBody(string routeName) + { + var upsert = _calls.Single(call => call.Method == "PUT"); + if (upsert.Path != $"/api/webhooks/{routeName}") + throw new InvalidOperationException($"Expected an upsert of '{routeName}', got '{upsert.Path}'."); + + return JsonDocument.Parse(upsert.Body); + } + + private HttpResponseMessage Record( + HttpRequestMessage request, + Func respond) + { + // ReadAsStream is the synchronous content reader, so the fake handler + // records the body without a blocking wait on a task. + var body = string.Empty; + if (request.Content is { } content) + { + using var reader = new StreamReader(content.ReadAsStream(), Encoding.UTF8); + body = reader.ReadToEnd(); + } + + _calls.Add(new RecordedCall(request.Method.Method, request.RequestUri!.AbsolutePath, body)); + return respond(request); + } + + internal sealed record RecordedCall(string Method, string Path, string Body); +} diff --git a/src/Netclaw.Cli.Tests/Webhooks/WebhookRouteDaemonClientTests.cs b/src/Netclaw.Cli.Tests/Webhooks/WebhookRouteDaemonClientTests.cs new file mode 100644 index 000000000..0086cb57e --- /dev/null +++ b/src/Netclaw.Cli.Tests/Webhooks/WebhookRouteDaemonClientTests.cs @@ -0,0 +1,174 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- +using System.Net; +using Netclaw.Cli.Webhooks; +using Netclaw.Configuration; +using Netclaw.Tests.Utilities; +using Xunit; + +namespace Netclaw.Cli.Tests.Webhooks; + +/// +/// The write rule itself (design D4). Every CLI surface that mutates a webhook +/// route calls the daemon through this client, so these tests own the rule: the +/// daemon writes, or the command fails. No answer selects a local write. +/// +public sealed class WebhookRouteDaemonClientTests : IDisposable +{ + private readonly DisposableTempDir _dir = new(); + private readonly NetclawPaths _paths; + + public WebhookRouteDaemonClientTests() + { + _paths = new NetclawPaths(_dir.Path); + _paths.EnsureDirectoriesExist(); + } + + public void Dispose() => _dir.Dispose(); + + [Fact] + public async Task A_reachable_daemon_is_available() + { + var client = CreateClient(_ => FakeWebhookDaemon.RouteList()); + + var available = await client.EnsureAvailableAsync(TestContext.Current.CancellationToken); + + Assert.True(available.Success); + Assert.Null(available.Error); + } + + [Fact] + public async Task An_unreachable_daemon_fails_and_names_the_remedy() + { + var client = CreateClient(_ => throw new HttpRequestException("connection refused")); + + var available = await client.EnsureAvailableAsync(TestContext.Current.CancellationToken); + + Assert.False(available.Success); + Assert.Equal( + "The daemon is not reachable. Start the daemon to manage webhook routes.", + available.Error); + } + + [Fact] + public async Task An_old_daemon_without_the_resource_fails_and_asks_for_an_upgrade() + { + // A 404 is a different probe answer from an unreachable daemon: the + // process runs, the resource does not exist yet. The operator needs a + // different remedy, so the message differs. + var client = CreateClient(_ => new HttpResponseMessage(HttpStatusCode.NotFound)); + + var available = await client.EnsureAvailableAsync(TestContext.Current.CancellationToken); + + Assert.False(available.Success); + Assert.Equal( + "This daemon does not serve the webhook route API. Upgrade the daemon.", + available.Error); + } + + [Fact] + public async Task A_missing_daemon_client_fails_like_an_unreachable_daemon() + { + var client = new WebhookRouteDaemonClient(daemonApi: null); + + var available = await client.EnsureAvailableAsync(TestContext.Current.CancellationToken); + + Assert.False(available.Success); + Assert.Equal( + "The daemon is not reachable. Start the daemon to manage webhook routes.", + available.Error); + } + + [Fact] + public async Task Availability_resolves_once_so_one_invocation_probes_one_time() + { + var probes = 0; + var client = CreateClient(_ => + { + probes++; + return FakeWebhookDaemon.RouteList(); + }); + var ct = TestContext.Current.CancellationToken; + + await client.EnsureAvailableAsync(ct); + await client.EnsureAvailableAsync(ct); + await client.EnsureAvailableAsync(ct); + + Assert.Equal(1, probes); + } + + [Theory] + [InlineData(HttpStatusCode.Unauthorized)] + [InlineData(HttpStatusCode.Forbidden)] + [InlineData(HttpStatusCode.InternalServerError)] + public async Task A_daemon_that_refuses_the_probe_fails_with_its_own_answer(HttpStatusCode status) + { + var client = CreateClient(_ => new HttpResponseMessage(status)); + + var available = await client.EnsureAvailableAsync(TestContext.Current.CancellationToken); + + Assert.False(available.Success); + Assert.Contains(((int)status).ToString(), available.Error!, StringComparison.Ordinal); + } + + [Fact] + public async Task A_rejected_upsert_reports_the_daemon_message_and_never_succeeds() + { + var client = CreateClient(request => request.Method == HttpMethod.Put + ? FakeWebhookDaemon.Json(HttpStatusCode.BadRequest, new { error = "Prompt is required." }) + : FakeWebhookDaemon.RouteList()); + var ct = TestContext.Current.CancellationToken; + + Assert.True((await client.EnsureAvailableAsync(ct)).Success); + var saved = await client.UpsertAsync("guarded-route", new WebhookRoutePatch { Prompt = "x" }, ct); + + Assert.False(saved.Success); + Assert.Equal("Prompt is required.", saved.Error); + } + + [Fact] + public async Task A_daemon_that_dies_mid_write_fails_closed_and_names_the_uncertainty() + { + // The probe succeeded, then the transport broke. The daemon may have + // applied the change, so the command must not retry silently or write a + // file of its own. + var probed = false; + var client = CreateClient(_ => + { + if (probed) + throw new HttpRequestException("connection reset"); + + probed = true; + return FakeWebhookDaemon.RouteList(); + }); + var ct = TestContext.Current.CancellationToken; + + Assert.True((await client.EnsureAvailableAsync(ct)).Success); + var saved = await client.UpsertAsync("guarded-route", new WebhookRoutePatch { Prompt = "x" }, ct); + + Assert.False(saved.Success); + Assert.Contains("may or may not have applied", saved.Error!, StringComparison.Ordinal); + } + + [Fact] + public async Task A_delete_of_a_missing_route_reports_not_found_rather_than_an_error() + { + var client = CreateClient(request => request.Method == HttpMethod.Delete + ? new HttpResponseMessage(HttpStatusCode.NotFound) + : FakeWebhookDaemon.RouteList()); + var ct = TestContext.Current.CancellationToken; + + await client.EnsureAvailableAsync(ct); + var removed = await client.DeleteAsync("missing-route", ct); + + Assert.False(removed.Success); + Assert.True(removed.NotFound); + Assert.Null(removed.Error); + } + + private WebhookRouteDaemonClient CreateClient(Func respond) + => new(new FakeWebhookDaemon(_paths, respond).Api); +} diff --git a/src/Netclaw.Cli.Tests/Webhooks/WebhooksCommandModeSelectionTests.cs b/src/Netclaw.Cli.Tests/Webhooks/WebhooksCommandModeSelectionTests.cs new file mode 100644 index 000000000..ada57509f --- /dev/null +++ b/src/Netclaw.Cli.Tests/Webhooks/WebhooksCommandModeSelectionTests.cs @@ -0,0 +1,240 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- +using System.Net; +using System.Text.Json; +using Netclaw.Cli.Webhooks; +using Netclaw.Configuration; +using Netclaw.Tests.Utilities; +using Xunit; + +namespace Netclaw.Cli.Tests.Webhooks; + +/// +/// What netclaw webhooks does with each daemon answer. Route mutations are +/// daemon-only: each test names the answer and asserts the observable effect — +/// which HTTP call the command made, the exit code, and that no route file +/// changed on any path. +/// +/// The failure tests read Console.Error, so the class joins the console +/// redirection collection. +/// +/// +[Collection(ConsoleRedirectionCollection.Name)] +public sealed class WebhooksCommandModeSelectionTests : IDisposable +{ + private const string RouteName = "mode-route"; + + private readonly DisposableTempDir _dir = new(); + private readonly NetclawPaths _paths; + + public WebhooksCommandModeSelectionTests() + { + _paths = new NetclawPaths(_dir.Path); + _paths.EnsureDirectoriesExist(); + } + + public void Dispose() => _dir.Dispose(); + + private string RouteFilePath => Path.Combine(_paths.WebhooksDirectory, $"{RouteName}.json"); + + private static string[] SetArguments() => + [ + "webhooks", "set", RouteName, + "--prompt", "Triage the delivery", + "--secret-env", "NETCLAW_TEST_WEBHOOK_SECRET" + ]; + + [Fact] + public async Task Set_with_a_reachable_daemon_sends_the_patch_and_writes_no_file() + { + var daemon = FakeWebhookDaemon.Healthy(_paths); + var stdout = new StringWriter(); + + var result = await RunSetAsync(stdout, daemon); + + Assert.Equal(0, result); + Assert.False(File.Exists(RouteFilePath)); + Assert.Contains($"[OK] Created webhook route '{RouteName}'.", stdout.ToString(), StringComparison.Ordinal); + + // The patch is the cross-boundary contract with the daemon's request body: + // camel-case names, the CLI's documented 'public' default for a new route, + // and a null for every flag the operator did not pass. + using var body = daemon.SingleUpsertBody(RouteName); + Assert.Equal("Triage the delivery", body.RootElement.GetProperty("prompt").GetString()); + Assert.Equal("test-secret-value", body.RootElement.GetProperty("secret").GetString()); + Assert.Equal("public", body.RootElement.GetProperty("audience").GetString()); + Assert.Equal(JsonValueKind.Null, body.RootElement.GetProperty("enabled").ValueKind); + } + + [Fact] + public async Task Delete_with_a_reachable_daemon_calls_the_resource_instead_of_the_file() + { + WriteRouteFile(); + var daemon = FakeWebhookDaemon.Healthy(_paths); + var stdout = new StringWriter(); + + var result = await WebhooksCommand.RunAsync( + ["webhooks", "delete", RouteName, "--force"], _paths, stdout, daemon.Api); + + Assert.Equal(0, result); + Assert.Contains(daemon.Calls, call => call.Method == "DELETE" && call.Path == $"/api/webhooks/{RouteName}"); + Assert.Equal($"[OK] Deleted webhook route '{RouteName}'.", stdout.ToString().TrimEnd()); + + // The daemon owns the deletion, so the command must not remove the file itself. + Assert.True(File.Exists(RouteFilePath)); + } + + [Fact] + public async Task Set_with_an_unreachable_daemon_fails_and_writes_no_file() + { + var daemon = FakeWebhookDaemon.Unreachable(_paths); + var stdout = new StringWriter(); + var stderr = new StringWriter(); + + var result = await RunSetAsync(stdout, daemon, stderr); + + Assert.Equal(1, result); + Assert.False(File.Exists(RouteFilePath)); + Assert.Equal(string.Empty, stdout.ToString()); + Assert.Contains( + "[FAIL] The daemon is not reachable. Start the daemon to manage webhook routes.", + stderr.ToString(), + StringComparison.Ordinal); + } + + [Fact] + public async Task Delete_with_an_unreachable_daemon_fails_and_leaves_the_file() + { + WriteRouteFile(); + var daemon = FakeWebhookDaemon.Unreachable(_paths); + var stdout = new StringWriter(); + var stderr = new StringWriter(); + + var result = await RunWithStderrAsync( + stderr, + () => WebhooksCommand.RunAsync( + ["webhooks", "delete", RouteName, "--force"], _paths, stdout, daemon.Api)); + + Assert.Equal(1, result); + Assert.True(File.Exists(RouteFilePath)); + Assert.Equal(string.Empty, stdout.ToString()); + Assert.Contains( + "[FAIL] The daemon is not reachable. Start the daemon to manage webhook routes.", + stderr.ToString(), + StringComparison.Ordinal); + } + + [Fact] + public async Task Set_against_an_old_daemon_without_the_resource_fails_and_asks_for_an_upgrade() + { + // An old daemon answers, so this is a different outcome from an + // unreachable daemon: the resource is absent, not the process. The + // remedy differs, so the message does too. + var daemon = FakeWebhookDaemon.WithoutRouteResource(_paths); + var stdout = new StringWriter(); + var stderr = new StringWriter(); + + var result = await RunSetAsync(stdout, daemon, stderr); + + Assert.Equal(1, result); + Assert.False(File.Exists(RouteFilePath)); + Assert.DoesNotContain(daemon.Calls, call => call.Method == "PUT"); + Assert.Contains( + "[FAIL] This daemon does not serve the webhook route API. Upgrade the daemon.", + stderr.ToString(), + StringComparison.Ordinal); + } + + [Fact] + public async Task Set_rejected_with_a_validation_error_fails_without_writing_a_file() + { + var daemon = new FakeWebhookDaemon(_paths, request => request.Method == HttpMethod.Put + ? FakeWebhookDaemon.Json(HttpStatusCode.BadRequest, new { error = "Route audience exceeds creator authority." }) + : FakeWebhookDaemon.RouteList()); + var stdout = new StringWriter(); + + var result = await RunSetAsync(stdout, daemon); + + Assert.Equal(1, result); + Assert.False(File.Exists(RouteFilePath)); + Assert.Equal(string.Empty, stdout.ToString()); + } + + [Fact] + public async Task Set_rejected_by_authentication_fails_without_writing_a_file() + { + var daemon = new FakeWebhookDaemon(_paths, _ => new HttpResponseMessage(HttpStatusCode.Unauthorized)); + var stdout = new StringWriter(); + + var result = await RunSetAsync(stdout, daemon); + + Assert.Equal(1, result); + Assert.False(File.Exists(RouteFilePath)); + Assert.DoesNotContain(daemon.Calls, call => call.Method == "PUT"); + } + + [Fact] + public async Task Delete_rejected_by_authorization_fails_without_removing_the_file() + { + WriteRouteFile(); + var daemon = new FakeWebhookDaemon(_paths, request => request.Method == HttpMethod.Delete + ? new HttpResponseMessage(HttpStatusCode.Forbidden) + : FakeWebhookDaemon.RouteList()); + var stdout = new StringWriter(); + + var result = await WebhooksCommand.RunAsync( + ["webhooks", "delete", RouteName, "--force"], _paths, stdout, daemon.Api); + + Assert.Equal(1, result); + Assert.True(File.Exists(RouteFilePath)); + } + + private Task RunSetAsync(TextWriter stdout, FakeWebhookDaemon daemon, TextWriter stderr) + => RunWithStderrAsync(stderr, () => RunSetAsync(stdout, daemon)); + + private async Task RunSetAsync(TextWriter stdout, FakeWebhookDaemon daemon) + { + // --secret-env keeps the shell-history warning out of the command output. + Environment.SetEnvironmentVariable("NETCLAW_TEST_WEBHOOK_SECRET", "test-secret-value"); + try + { + return await WebhooksCommand.RunAsync(SetArguments(), _paths, stdout, daemon.Api); + } + finally + { + Environment.SetEnvironmentVariable("NETCLAW_TEST_WEBHOOK_SECRET", null); + } + } + + /// + /// Captures the command's stderr. The command writes failures to + /// , which is process-wide, so only the tests that + /// assert on a message pay the cost of redirecting it. + /// + private static async Task RunWithStderrAsync(TextWriter stderr, Func> run) + { + var original = Console.Error; + Console.SetError(stderr); + try + { + return await run(); + } + finally + { + Console.SetError(original); + } + } + + private void WriteRouteFile() + { + var store = new WebhookRouteStore(_paths); + store.Save(RouteName, new WebhookRouteConfig + { + Prompt = "Existing prompt", + Verification = new WebhookVerificationConfig { Secret = new SensitiveString("existing-secret") } + }); + } +} diff --git a/src/Netclaw.Cli.Tests/Webhooks/WebhooksCommandTests.cs b/src/Netclaw.Cli.Tests/Webhooks/WebhooksCommandTests.cs index 7449f5515..17300d58b 100644 --- a/src/Netclaw.Cli.Tests/Webhooks/WebhooksCommandTests.cs +++ b/src/Netclaw.Cli.Tests/Webhooks/WebhooksCommandTests.cs @@ -3,6 +3,7 @@ // Copyright (C) 2026 - 2026 Petabridge, LLC // // ----------------------------------------------------------------------- +using System.Net; using System.Text.Json; using System.Text.Json.Nodes; using Json.Schema; @@ -14,6 +15,13 @@ namespace Netclaw.Cli.Tests.Webhooks; +/// +/// The netclaw webhooks surface. Reads run against canonical disk with no +/// daemon. Writes are daemon-only, so every set or delete test that +/// reaches the write step supplies a and asserts +/// the patch the CLI sent. A test that stops at argument grammar, at the merge +/// preview, or at --dry-run never contacts the daemon and passes none. +/// public sealed class WebhooksCommandTests : IDisposable { private readonly DisposableTempDir _dir = new(); @@ -245,46 +253,56 @@ public async Task Show_InvalidRouteWithNullVerification_ReturnsOne() } [Fact] - public async Task Set_NewRoute_CreatesFile() + public async Task Set_NewRoute_SendsTheRouteToTheDaemon() { + var daemon = FakeWebhookDaemon.Healthy(_paths); + var result = await WebhooksCommand.RunAsync([ "webhooks", "set", "new-route", "--prompt", "Test prompt", "--secret", "test-secret" - ], _paths); + ], _paths, output: null, daemon.Api); Assert.Equal(0, result); - Assert.True(File.Exists(Path.Combine(_paths.WebhooksDirectory, "new-route.json"))); + using var body = daemon.SingleUpsertBody("new-route"); + Assert.Equal("Test prompt", body.RootElement.GetProperty("prompt").GetString()); + Assert.Equal("test-secret", body.RootElement.GetProperty("secret").GetString()); + + // The daemon writes the file, so the CLI must leave the directory alone. + Assert.False(File.Exists(Path.Combine(_paths.WebhooksDirectory, "new-route.json"))); } [Fact] public async Task Set_WriteFailure_DoesNotReportSuccess() { - Directory.CreateDirectory(Path.Combine(_paths.WebhooksDirectory, "blocked-route.json")); + var daemon = new FakeWebhookDaemon(_paths, request => request.Method == HttpMethod.Put + ? new HttpResponseMessage(HttpStatusCode.InternalServerError) + : FakeWebhookDaemon.RouteList()); using var output = new StringWriter(); - var exception = await Assert.ThrowsAnyAsync(() => WebhooksCommand.RunAsync([ + var result = await WebhooksCommand.RunAsync([ "webhooks", "set", "blocked-route", "--prompt", "Test prompt", "--secret", "test-secret" - ], _paths, output)); + ], _paths, output, daemon.Api); - Assert.True(exception is IOException or UnauthorizedAccessException, - $"Expected a persistence IO exception, got {exception.GetType().Name}: {exception.Message}"); + Assert.Equal(1, result); Assert.DoesNotContain("[OK]", output.ToString(), StringComparison.Ordinal); } [Fact] public async Task Set_WithUppercaseRoute_NormalizesToLowercase() { + var daemon = FakeWebhookDaemon.Healthy(_paths); + var result = await WebhooksCommand.RunAsync([ "webhooks", "set", "GitHub-Issues", "--prompt", "Test prompt", "--secret", "test-secret" - ], _paths); + ], _paths, output: null, daemon.Api); Assert.Equal(0, result); - Assert.True(File.Exists(Path.Combine(_paths.WebhooksDirectory, "github-issues.json"))); + Assert.Contains(daemon.Calls, call => call.Method == "PUT" && call.Path == "/api/webhooks/github-issues"); } [Theory] @@ -426,8 +444,10 @@ public async Task Set_MissingSecretEnvVariable_ReturnsOne() } [Fact] - public async Task Set_TimestampedHmac_Persists_advanced_settings() + public async Task Set_TimestampedHmac_Sends_advanced_settings() { + var daemon = FakeWebhookDaemon.Healthy(_paths); + var result = await WebhooksCommand.RunAsync([ "webhooks", "set", "stripe-events", "--prompt", "Process Stripe event", @@ -438,31 +458,35 @@ public async Task Set_TimestampedHmac_Persists_advanced_settings() "--signature-field", "signature", "--signed-payload-separator", "::", "--signature-tolerance-seconds", "120" - ], _paths); + ], _paths, output: null, daemon.Api); Assert.Equal(0, result); - var route = ReadRoute("stripe-events"); - Assert.Equal(WebhookVerifierKind.HmacTimestamped, route.Verification.Kind); - Assert.Equal("Stripe-Signature", route.Verification.SignatureHeaderName); - Assert.Equal("timestamp", route.Verification.TimestampField); - Assert.Equal("signature", route.Verification.SignatureField); - Assert.Equal("::", route.Verification.SignedPayloadSeparator); - Assert.Equal(120, route.Verification.ToleranceSeconds); + using var body = daemon.SingleUpsertBody("stripe-events"); + var patch = body.RootElement; + Assert.Equal("HmacTimestamped", patch.GetProperty("verificationKind").GetString()); + Assert.Equal("Stripe-Signature", patch.GetProperty("signatureHeaderName").GetString()); + Assert.Equal("timestamp", patch.GetProperty("timestampField").GetString()); + Assert.Equal("signature", patch.GetProperty("signatureField").GetString()); + Assert.Equal("::", patch.GetProperty("signedPayloadSeparator").GetString()); + Assert.Equal(120, patch.GetProperty("toleranceSeconds").GetInt32()); } [Fact] public async Task Set_HeaderSecret_Accepts_documented_hyphenated_spelling() { + var daemon = FakeWebhookDaemon.Healthy(_paths); + var result = await WebhooksCommand.RunAsync([ "webhooks", "set", "internal-events", "--prompt", "Process internal event", "--secret", "shared-secret", "--verification-kind", "header-secret", "--secret-header", "X-Internal-Secret" - ], _paths); + ], _paths, output: null, daemon.Api); Assert.Equal(0, result); - Assert.Equal(WebhookVerifierKind.HeaderSecret, ReadRoute("internal-events").Verification.Kind); + using var body = daemon.SingleUpsertBody("internal-events"); + Assert.Equal("HeaderSecret", body.RootElement.GetProperty("verificationKind").GetString()); } [Fact] @@ -503,22 +527,28 @@ public async Task Set_Unusable_timestamp_fields_fail_without_persisting( } [Fact] - public async Task Set_Unrelated_update_preserves_legacy_verifier_without_timestamp_fields() + public async Task Set_Unrelated_update_leaves_the_legacy_verifier_untouched() { CreateValidRoute("legacy-route"); + var daemon = FakeWebhookDaemon.Healthy(_paths); var result = await WebhooksCommand.RunAsync([ "webhooks", "set", "legacy-route", "--rate-limit", "12" - ], _paths); + ], _paths, output: null, daemon.Api); Assert.Equal(0, result); - var route = ReadRoute("legacy-route"); - Assert.Equal(WebhookVerifierKind.Hmac, route.Verification.Kind); - Assert.Equal(12, route.RateLimitPerMinute); - var json = File.ReadAllText(Path.Combine(_paths.WebhooksDirectory, "legacy-route.json")); - Assert.DoesNotContain("ToleranceSeconds", json, StringComparison.Ordinal); - Assert.DoesNotContain("TimestampField", json, StringComparison.Ordinal); + + // The patch carries only the flag the operator passed. Every verifier + // field stays null, so the daemon keeps the stored HMAC settings and adds + // no timestamp fields to a route that has none. + using var body = daemon.SingleUpsertBody("legacy-route"); + var patch = body.RootElement; + Assert.Equal(12, patch.GetProperty("rateLimitPerMinute").GetInt32()); + Assert.Equal(JsonValueKind.Null, patch.GetProperty("verificationKind").ValueKind); + Assert.Equal(JsonValueKind.Null, patch.GetProperty("toleranceSeconds").ValueKind); + Assert.Equal(JsonValueKind.Null, patch.GetProperty("timestampField").ValueKind); + Assert.Equal(JsonValueKind.Null, patch.GetProperty("signatureField").ValueKind); } [Fact] @@ -574,17 +604,27 @@ public async Task Show_Json_adds_timestamp_fields_only_for_timestamped_kind() public async Task Delete_ExistingRoute_ReturnsZero() { CreateValidRoute("delete-me"); + var daemon = FakeWebhookDaemon.Healthy(_paths); - var result = await WebhooksCommand.RunAsync(["webhooks", "delete", "delete-me", "--force"], _paths); + var result = await WebhooksCommand.RunAsync( + ["webhooks", "delete", "delete-me", "--force"], _paths, output: null, daemon.Api); Assert.Equal(0, result); - Assert.False(File.Exists(Path.Combine(_paths.WebhooksDirectory, "delete-me.json"))); + Assert.Contains(daemon.Calls, call => call.Method == "DELETE" && call.Path == "/api/webhooks/delete-me"); } [Fact] public async Task Delete_NonexistentRoute_ReturnsOne() { - var result = await WebhooksCommand.RunAsync(["webhooks", "delete", "nonexistent", "--force"], _paths); + // The daemon owns the route set, so a missing route is its 404, not a + // missing file on the CLI's disk. + var daemon = new FakeWebhookDaemon(_paths, request => request.Method == HttpMethod.Delete + ? new HttpResponseMessage(HttpStatusCode.NotFound) + : FakeWebhookDaemon.RouteList()); + + var result = await WebhooksCommand.RunAsync( + ["webhooks", "delete", "nonexistent", "--force"], _paths, output: null, daemon.Api); + Assert.Equal(1, result); } diff --git a/src/Netclaw.Cli/Daemon/DaemonApi.cs b/src/Netclaw.Cli/Daemon/DaemonApi.cs index 6a1eada61..7060995c0 100644 --- a/src/Netclaw.Cli/Daemon/DaemonApi.cs +++ b/src/Netclaw.Cli/Daemon/DaemonApi.cs @@ -234,6 +234,43 @@ public async Task ImportReminderAsync(object request, JsonS : await client.PostAsJsonAsync($"{_endpoint}/api/reminders/import", request, cts.Token); } + // ── Webhook routes ──────────────────────────────────────────────── + + /// + /// Lists the daemon's webhook routes. The CLI also uses this call as its + /// write availability probe: a transport failure means the daemon is down, + /// and a 404 means the daemon predates the resource. Every answer other than + /// success fails the mutation; there is no local write path. + /// + public async Task ListWebhookRoutesAsync(CancellationToken ct = default) + { + using var cts = CreateTimeoutCts(DefaultTimeout, ct); + var client = CreateHttpClient(); + return await client.GetAsync($"{_endpoint}/api/webhooks", cts.Token); + } + + /// + /// Creates or updates one webhook route. is a + /// field-level patch: an omitted (null) property leaves the stored value + /// unchanged, so two patches of different fields compose in the daemon. + /// + public async Task UpsertWebhookRouteAsync( + string name, + object request, + CancellationToken ct = default) + { + using var cts = CreateTimeoutCts(DefaultTimeout, ct); + var client = CreateHttpClient(); + return await client.PutAsJsonAsync($"{_endpoint}/api/webhooks/{Uri.EscapeDataString(name)}", request, cts.Token); + } + + public async Task DeleteWebhookRouteAsync(string name, CancellationToken ct = default) + { + using var cts = CreateTimeoutCts(DefaultTimeout, ct); + var client = CreateHttpClient(); + return await client.DeleteAsync($"{_endpoint}/api/webhooks/{Uri.EscapeDataString(name)}", cts.Token); + } + // ── MCP OAuth ───────────────────────────────────────────────────── public async Task StartMcpOAuthAsync(string name, CancellationToken ct = default) diff --git a/src/Netclaw.Cli/Program.cs b/src/Netclaw.Cli/Program.cs index a4cce36f0..7d595a316 100644 --- a/src/Netclaw.Cli/Program.cs +++ b/src/Netclaw.Cli/Program.cs @@ -860,6 +860,37 @@ static async Task RunAsync(string[] args) // ── Webhook management ── if (mode is "webhooks") { + var webhooksSubcommand = args.Length > 1 ? args[1] : "list"; + + // `set` and `delete` mutate routes, which only the daemon does — they need + // the DaemonApi client. Building the DI host parses local config + // (netclaw.json, secrets.json); a corrupt file must produce a readable + // error, not a stack trace. Every other subcommand reads route files, + // which stay canonical, so it needs no daemon. + if (webhooksSubcommand is "set" or "delete") + { + try + { + var builder = CreateQuietHostBuilder(args); + using var webhooksHost = builder.Build(); + var webhooksPaths = webhooksHost.Services.GetRequiredService(); + webhooksPaths.EnsureDirectoriesExist(); + Environment.ExitCode = await WebhooksCommand.RunAsync( + args, + webhooksPaths, + output: null, + webhooksHost.Services.GetRequiredService()); + } + catch (Exception ex) when (ex is InvalidDataException or InvalidOperationException or FormatException) + { + Console.Error.WriteLine($"webhooks {webhooksSubcommand}: could not load local configuration: {ex.Message}"); + Console.Error.WriteLine("Fix the file it names (under ~/.netclaw/config) and retry."); + Environment.ExitCode = 1; + } + + return; + } + var paths = new NetclawPaths(); paths.EnsureDirectoriesExist(); Environment.ExitCode = await WebhooksCommand.RunAsync(args, paths); diff --git a/src/Netclaw.Cli/Webhooks/WebhookRouteDaemonClient.cs b/src/Netclaw.Cli/Webhooks/WebhookRouteDaemonClient.cs new file mode 100644 index 000000000..c097c1b0f --- /dev/null +++ b/src/Netclaw.Cli/Webhooks/WebhookRouteDaemonClient.cs @@ -0,0 +1,262 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- +using System.Net; +using System.Text.Json; +using Netclaw.Cli.Daemon; + +namespace Netclaw.Cli.Webhooks; + +/// Outcome of one webhook route call against the daemon. +/// True when the daemon accepted the call. +/// +/// True only for a delete of a route the daemon does not hold. The availability +/// probe never sets it: a 404 there means the daemon predates the resource, which +/// is a hard failure with its own message. +/// +/// The message to report. It is null when the call succeeded. +internal readonly record struct WebhookRouteApiResult(bool Success, bool NotFound, string? Error); + +/// +/// The one write path for webhook routes. Every CLI surface that mutates a route +/// calls the daemon through this client, so the command and the config TUI cannot +/// drift into two different rules. +/// +/// The rule (design D4): the daemon owns route mutations. There is no local write +/// path and no fallback. A daemon that does not answer, a daemon that does not +/// serve the resource, and a daemon that refuses the call all fail the command. +/// The supported daemon-absent path is a route file authored on disk and loaded +/// at daemon startup, not a CLI write. +/// +/// +internal sealed class WebhookRouteDaemonClient +{ + /// The daemon did not answer, so no route mutation can happen. + internal const string DaemonUnreachableMessage = + "The daemon is not reachable. Start the daemon to manage webhook routes."; + + /// The daemon answered but predates the webhook route resource. + internal const string DaemonMissingResourceMessage = + "This daemon does not serve the webhook route API. Upgrade the daemon."; + + private readonly DaemonApi? _daemonApi; + private WebhookRouteApiResult? _availability; + + /// + /// Creates the client. is null when the caller + /// holds no daemon client at all. That is the same operator-visible state as + /// an unreachable daemon — this process cannot reach one — so the probe fails + /// with the same message. It never selects a local write. + /// + public WebhookRouteDaemonClient(DaemonApi? daemonApi) + { + _daemonApi = daemonApi; + } + + /// + /// Reports whether the daemon can serve a route mutation. The probe runs once + /// per client instance, so one CLI invocation asks one time. + /// + public async Task EnsureAvailableAsync(CancellationToken ct) + { + if (_availability is { } cached) + return cached; + + var availability = await ProbeAsync(ct); + _availability = availability; + return availability; + } + + /// + /// Sends one field-level route patch to the daemon. Call it only after + /// reported success. + /// + public async Task UpsertAsync( + string routeName, + WebhookRoutePatch patch, + CancellationToken ct) + { + var api = RequireDaemonApi(); + try + { + using var response = await api.UpsertWebhookRouteAsync(routeName, patch, ct); + if (response.IsSuccessStatusCode) + return new WebhookRouteApiResult(Success: true, NotFound: false, Error: null); + + return new WebhookRouteApiResult( + Success: false, + NotFound: false, + Error: await DescribeFailureAsync(response, ct)); + } + catch (Exception ex) when (IsDaemonUnreachable(ex, ct)) + { + // The daemon died between the probe and this write. Fail with a + // readable error that names the uncertainty: the daemon may have + // applied the change before the connection broke. + return new WebhookRouteApiResult( + Success: false, + NotFound: false, + Error: MidFlightFailureMessage(ex)); + } + } + + /// + /// Deletes one route through the daemon. The probe already ran when this + /// runs, so a 404 here means the route is missing, never an old daemon. + /// + public async Task DeleteAsync(string routeName, CancellationToken ct) + { + var api = RequireDaemonApi(); + try + { + using var response = await api.DeleteWebhookRouteAsync(routeName, ct); + if (response.IsSuccessStatusCode) + return new WebhookRouteApiResult(Success: true, NotFound: false, Error: null); + + if (response.StatusCode is HttpStatusCode.NotFound) + return new WebhookRouteApiResult(Success: false, NotFound: true, Error: null); + + return new WebhookRouteApiResult( + Success: false, + NotFound: false, + Error: await DescribeFailureAsync(response, ct)); + } + catch (Exception ex) when (IsDaemonUnreachable(ex, ct)) + { + // Same rule as UpsertAsync: a mid-flight transport failure fails the + // command with a readable error. + return new WebhookRouteApiResult( + Success: false, + NotFound: false, + Error: MidFlightFailureMessage(ex)); + } + } + + private static string MidFlightFailureMessage(Exception ex) + => "the daemon became unreachable while the write was in flight" + + $" ({ex.Message}). The daemon may or may not have applied the" + + " change. Verify with 'netclaw webhooks show' and retry."; + + private DaemonApi RequireDaemonApi() + => _daemonApi ?? throw new InvalidOperationException( + "The webhook route client has no daemon client. Probe availability before you call the daemon."); + + private async Task ProbeAsync(CancellationToken ct) + { + if (_daemonApi is null) + return Unavailable(DaemonUnreachableMessage); + + try + { + using var response = await _daemonApi.ListWebhookRoutesAsync(ct); + + // An old daemon has no route resource, so the path resolves to nothing. + if (response.StatusCode is HttpStatusCode.NotFound) + return Unavailable(DaemonMissingResourceMessage); + + if (response.IsSuccessStatusCode) + return new WebhookRouteApiResult(Success: true, NotFound: false, Error: null); + + // The daemon answered and refused. It is the enforcement point, so its + // refusal stops the command and keeps its own message. + return Unavailable(await DescribeFailureAsync(response, ct)); + } + catch (Exception ex) when (IsDaemonUnreachable(ex, ct)) + { + return Unavailable(DaemonUnreachableMessage); + } + } + + private static WebhookRouteApiResult Unavailable(string error) + => new(Success: false, NotFound: false, Error: error); + + /// + /// Reports whether the failure means "no daemon answered". The client puts + /// its request timeout on a linked token, so a timeout arrives as a + /// cancellation that the caller's own token did not request. + /// + private static bool IsDaemonUnreachable(Exception ex, CancellationToken ct) + => ex is HttpRequestException or OperationCanceledException && !ct.IsCancellationRequested; + + /// + /// Reads the daemon's own message out of a failure response. The route + /// handlers answer either {"error": ...} or a problem document with + /// detail; an unreadable body degrades to the status code, which is + /// still the daemon's answer. + /// + private static async Task DescribeFailureAsync(HttpResponseMessage response, CancellationToken ct) + { + var fallback = $"daemon returned {(int)response.StatusCode} {response.ReasonPhrase}".TrimEnd(); + + string body; + try + { + body = await response.Content.ReadAsStringAsync(ct); + } + catch (Exception ex) when (ex is HttpRequestException or IOException) + { + return fallback; + } + + if (string.IsNullOrWhiteSpace(body)) + return fallback; + + try + { + using var document = JsonDocument.Parse(body); + if (document.RootElement.ValueKind is JsonValueKind.Object) + { + foreach (var property in new[] { "error", "detail", "title" }) + { + if (document.RootElement.TryGetProperty(property, out var value) + && value.ValueKind is JsonValueKind.String + && !string.IsNullOrWhiteSpace(value.GetString())) + { + return value.GetString()!; + } + } + } + } + catch (JsonException) + { + // Not a JSON body — an HTML error page from a proxy, for example. + // Report the status line rather than the page text. + return fallback; + } + + return fallback; + } +} + +/// +/// Field-level patch for PUT /api/webhooks/{name}. It mirrors the +/// daemon's request body: every property is optional and a null property leaves +/// the stored value unchanged, which is what an omitted CLI flag already means. +/// The property names are the wire contract — rename one only with the daemon's +/// UpsertWebhookRouteRequest. +/// +internal sealed record WebhookRoutePatch +{ + public string? Prompt { get; init; } + public string? Secret { get; init; } + public string? VerificationKind { get; init; } + public string? Audience { get; init; } + public IReadOnlyList? Events { get; init; } + public string? NotifyInstructions { get; init; } + public bool? DeliveryRequired { get; init; } + public string? NotificationChannelId { get; init; } + public int? MaxBodyBytes { get; init; } + public int? RateLimitPerMinute { get; init; } + public bool? Enabled { get; init; } + public string? SignatureHeaderName { get; init; } + public string? SignaturePrefix { get; init; } + public string? SecretHeaderName { get; init; } + public string? EventHeaderName { get; init; } + public string? DeliveryIdHeaderName { get; init; } + public string? TimestampField { get; init; } + public string? SignatureField { get; init; } + public string? SignedPayloadSeparator { get; init; } + public int? ToleranceSeconds { get; init; } +} diff --git a/src/Netclaw.Cli/Webhooks/WebhooksCommand.cs b/src/Netclaw.Cli/Webhooks/WebhooksCommand.cs index a56d27b84..eda736ac2 100644 --- a/src/Netclaw.Cli/Webhooks/WebhooksCommand.cs +++ b/src/Netclaw.Cli/Webhooks/WebhooksCommand.cs @@ -4,6 +4,7 @@ // // ----------------------------------------------------------------------- using System.Text.Json; +using Netclaw.Cli.Daemon; using Netclaw.Cli.Json; using Netclaw.Configuration; @@ -11,29 +12,48 @@ namespace Netclaw.Cli.Webhooks; /// /// Handles netclaw webhooks <subcommand> CLI subcommands. -/// All commands are offline — no daemon required. +/// +/// Reads (list, show, validate) are always local: disk is +/// the canonical route store and the daemon actor holds no cache, so a file read +/// is always current, and show needs the secret that the API never +/// returns. Writes (set, delete) require the daemon — see +/// . There is no local write path. +/// /// internal static class WebhooksCommand { - public static Task RunAsync(string[] args, NetclawPaths paths, TextWriter? output = null) + /// + /// Runs one netclaw webhooks invocation. + /// + /// + /// The daemon client for route mutations. The read subcommands never use it, + /// so they accept null. A null on set or delete fails the + /// command exactly as an unreachable daemon does. + /// + public static async Task RunAsync( + string[] args, + NetclawPaths paths, + TextWriter? output = null, + DaemonApi? daemonApi = null) { output ??= Console.Out; var subcommand = args.Length > 1 ? args[1] : "list"; if (subcommand is "help" or "-h" or "--help") - return Task.FromResult(WriteHelp(output)); + return WriteHelp(output); var store = new WebhookRouteStore(paths); + var daemon = new WebhookRouteDaemonClient(daemonApi); - return Task.FromResult(subcommand switch + return subcommand switch { "list" => RunList(args, store, paths, output), "show" => RunShow(args, store, paths, output), - "set" => RunSet(args, store, paths, output), - "delete" => RunDelete(args, store, output), + "set" => await RunSetAsync(args, store, paths, output, daemon), + "delete" => await RunDeleteAsync(args, output, daemon), "validate" => RunValidate(args, paths, output), _ => WriteHelp(output) - }); + }; } // ── list ── @@ -265,7 +285,12 @@ private static int RunShow(string[] args, WebhookRouteStore store, NetclawPaths // ── set ── - private static int RunSet(string[] args, WebhookRouteStore store, NetclawPaths paths, TextWriter output) + private static async Task RunSetAsync( + string[] args, + WebhookRouteStore store, + NetclawPaths paths, + TextWriter output, + WebhookRouteDaemonClient daemon) { if (args.Length < 3 || HasFlag(args, "--help") || HasFlag(args, "-h")) { @@ -295,255 +320,329 @@ private static int RunSet(string[] args, WebhookRouteStore store, NetclawPaths p if (!TryResolveTextInput(args, "--notify-instructions", "--notify-instructions-file", out var notifyInstructions, out var hasNotifyInstructions)) return 1; - var routeSaved = false; - var updatedExistingRoute = false; - int result; - try + // Argument grammar stays local: these checks read only the command line, + // so they answer the same way with or without a daemon. + if (!TryGetFlagValue(args, "--verification-kind", out var verificationKindText, out var hasVerificationKind)) + return 1; + + var verificationKind = WebhookVerifierKind.Hmac; + if (hasVerificationKind && !WebhookRouteValidator.TryParseVerifierKind(verificationKindText, out verificationKind)) { - result = store.Update(routeName, CancellationToken.None, existing => - { - var exists = existing is not null; + Console.Error.WriteLine($"[FAIL] Invalid verification kind: '{verificationKindText}'. Use 'hmac', 'hmac-timestamped', or 'header-secret'."); + return 1; + } - if (createOnly && exists) - { - Console.Error.WriteLine($"[FAIL] Webhook route '{routeName}' already exists (--create-only specified)."); - return (null, 1); - } + if (!TryGetFlagValue(args, "--signature-header", out var signatureHeader, out var hasSignatureHeader)) + return 1; - if (updateOnly && !exists) - { - Console.Error.WriteLine($"[FAIL] Webhook route '{routeName}' does not exist (--update-only specified)."); - return (null, 1); - } + if (!TryGetFlagValue(args, "--signature-prefix", out var signaturePrefix, out var hasSignaturePrefix)) + return 1; - // Start with existing config or defaults - var route = existing ?? new WebhookRouteConfig(); - route.Verification ??= new WebhookVerificationConfig(); - route.Events ??= []; + if (!TryGetFlagValue(args, "--secret-header", out var secretHeader, out var hasSecretHeader)) + return 1; - if (hasPrompt) - route.Prompt = prompt; + if (!TryGetFlagValue(args, "--event-header", out var eventHeader, out var hasEventHeader)) + return 1; - if (hasSecret) - route.Verification.Secret = new SensitiveString(secret); + if (!TryGetFlagValue(args, "--delivery-header", out var deliveryHeader, out var hasDeliveryHeader)) + return 1; - // Parse verification kind - if (!TryGetFlagValue(args, "--verification-kind", out var verificationKind, out var hasVerificationKind)) - return (null, 1); + if (!TryGetFlagValue(args, "--timestamp-field", out var timestampField, out var hasTimestampField)) + return 1; - if (hasVerificationKind) - { - if (!WebhookRouteValidator.TryParseVerifierKind(verificationKind, out var kind)) - { - Console.Error.WriteLine($"[FAIL] Invalid verification kind: '{verificationKind}'. Use 'hmac', 'hmac-timestamped', or 'header-secret'."); - return (null, 1); - } - route.Verification.Kind = kind; - } + if (!TryGetFlagValue(args, "--signature-field", out var signatureField, out var hasSignatureField)) + return 1; - // Parse verification headers - if (!TryGetFlagValue(args, "--signature-header", out var signatureHeader, out var hasSignatureHeader)) - return (null, 1); + if (!TryGetFlagValue(args, "--signed-payload-separator", out var payloadSeparator, out var hasPayloadSeparator)) + return 1; - if (hasSignatureHeader) - route.Verification.SignatureHeaderName = signatureHeader; + if (!TryGetFlagValue(args, "--signature-tolerance-seconds", out var toleranceText, out var hasTolerance)) + return 1; - if (!TryGetFlagValue(args, "--signature-prefix", out var signaturePrefix, out var hasSignaturePrefix)) - return (null, 1); + var toleranceSeconds = 0; + if (hasTolerance && !int.TryParse(toleranceText, out toleranceSeconds)) + { + Console.Error.WriteLine($"[FAIL] Invalid signature tolerance: '{toleranceText}'. Must be a whole number from 1 to 3600."); + return 1; + } - if (hasSignaturePrefix) - route.Verification.SignaturePrefix = signaturePrefix; + if (!TryGetFlagValue(args, "--events", out var eventsText, out var hasEvents)) + return 1; - if (!TryGetFlagValue(args, "--secret-header", out var secretHeader, out var hasSecretHeader)) - return (null, 1); + string[] events = hasEvents + ? [.. eventsText.Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries)] + : []; - if (hasSecretHeader) - route.Verification.SecretHeaderName = secretHeader; + if (!TryGetFlagValue(args, "--audience", out var audienceText, out var hasAudience)) + return 1; - if (!TryGetFlagValue(args, "--event-header", out var eventHeader, out var hasEventHeader)) - return (null, 1); + var audience = TrustAudience.Public; + if (hasAudience && !Enum.TryParse(audienceText, ignoreCase: true, out audience)) + { + Console.Error.WriteLine($"[FAIL] Invalid audience: '{audienceText}'. Use 'public', 'team', or 'personal'."); + return 1; + } - if (hasEventHeader) - route.Verification.EventHeaderName = eventHeader; + var deliveryRequired = HasFlag(args, "--delivery-required"); + var noDeliveryRequired = HasFlag(args, "--no-delivery-required"); + if (deliveryRequired && noDeliveryRequired) + { + Console.Error.WriteLine("[FAIL] --delivery-required and --no-delivery-required cannot be used together."); + return 1; + } - if (!TryGetFlagValue(args, "--delivery-header", out var deliveryHeader, out var hasDeliveryHeader)) - return (null, 1); + if (!TryGetFlagValue(args, "--notification-channel", out var notificationChannel, out var hasNotificationChannel)) + return 1; - if (hasDeliveryHeader) - route.Verification.DeliveryIdHeaderName = deliveryHeader; + if (!TryGetFlagValue(args, "--max-body", out var maxBodyText, out var hasMaxBody)) + return 1; - if (!TryGetFlagValue(args, "--timestamp-field", out var timestampField, out var hasTimestampField)) - return (null, 1); + var maxBodyBytes = 0; + if (hasMaxBody && (!int.TryParse(maxBodyText, out maxBodyBytes) || maxBodyBytes < 1)) + { + Console.Error.WriteLine($"[FAIL] Invalid max body size: '{maxBodyText}'. Must be a positive integer."); + return 1; + } - if (hasTimestampField) - route.Verification.TimestampField = timestampField; + if (!TryGetFlagValue(args, "--rate-limit", out var rateLimitText, out var hasRateLimit)) + return 1; - if (!TryGetFlagValue(args, "--signature-field", out var signatureField, out var hasSignatureField)) - return (null, 1); + var rateLimit = 0; + if (hasRateLimit && (!int.TryParse(rateLimitText, out rateLimit) || rateLimit < 1)) + { + Console.Error.WriteLine($"[FAIL] Invalid rate limit: '{rateLimitText}'. Must be a positive integer."); + return 1; + } - if (hasSignatureField) - route.Verification.SignatureField = signatureField; + var enabled = HasFlag(args, "--enabled"); + var disabled = HasFlag(args, "--disabled"); + if (enabled && disabled) + { + Console.Error.WriteLine("[FAIL] --enabled and --disabled cannot be used together."); + return 1; + } - if (!TryGetFlagValue(args, "--signed-payload-separator", out var payloadSeparator, out var hasPayloadSeparator)) - return (null, 1); + var updatedExistingRoute = false; - if (hasPayloadSeparator) - route.Verification.SignedPayloadSeparator = payloadSeparator; + // Merges the parsed flags onto the stored route and validates the result. + // It is a local preview: it answers --create-only / --update-only, the + // Created-or-Updated wording, and --dry-run before the command contacts + // the daemon. A null definition means the command sends nothing. The + // daemon re-reads and re-validates the patch, so it stays the one + // enforcement point. + (WebhookRouteConfig? Definition, int Result) Merge(WebhookRouteConfig? existing) + { + var exists = existing is not null; - if (!TryGetFlagValue(args, "--signature-tolerance-seconds", out var tolerance, out var hasTolerance)) - return (null, 1); + if (createOnly && exists) + { + Console.Error.WriteLine($"[FAIL] Webhook route '{routeName}' already exists (--create-only specified)."); + return (null, 1); + } - if (hasTolerance) - { - if (!int.TryParse(tolerance, out var toleranceSeconds)) - { - Console.Error.WriteLine($"[FAIL] Invalid signature tolerance: '{tolerance}'. Must be a whole number from 1 to 3600."); - return (null, 1); - } + if (updateOnly && !exists) + { + Console.Error.WriteLine($"[FAIL] Webhook route '{routeName}' does not exist (--update-only specified)."); + return (null, 1); + } - route.Verification.ToleranceSeconds = toleranceSeconds; - } + var route = existing ?? new WebhookRouteConfig(); + route.Verification ??= new WebhookVerificationConfig(); + route.Events ??= []; - if ((hasTimestampField || hasSignatureField || hasPayloadSeparator || hasTolerance) - && route.Verification.Kind != WebhookVerifierKind.HmacTimestamped) - { - Console.Error.WriteLine("[FAIL] Timestamp signature options require '--verification-kind hmac-timestamped'."); - return (null, 1); - } + if (hasPrompt) + route.Prompt = prompt; - // Parse events - if (!TryGetFlagValue(args, "--events", out var events, out var hasEvents)) - return (null, 1); + if (hasSecret) + route.Verification.Secret = new SensitiveString(secret); - if (hasEvents) - { - route.Events = [.. events.Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries)]; - } + if (hasVerificationKind) + route.Verification.Kind = verificationKind; - // Parse audience - if (!TryGetFlagValue(args, "--audience", out var audience, out var hasAudience)) - return (null, 1); + if (hasSignatureHeader) + route.Verification.SignatureHeaderName = signatureHeader; - if (hasAudience) - { - if (!Enum.TryParse(audience, ignoreCase: true, out var aud)) - { - Console.Error.WriteLine($"[FAIL] Invalid audience: '{audience}'. Use 'public', 'team', or 'personal'."); - return (null, 1); - } - route.Audience = aud; - } + if (hasSignaturePrefix) + route.Verification.SignaturePrefix = signaturePrefix; - if (hasNotifyInstructions) - route.NotifyInstructions = notifyInstructions; + if (hasSecretHeader) + route.Verification.SecretHeaderName = secretHeader; - var deliveryRequired = HasFlag(args, "--delivery-required"); - var noDeliveryRequired = HasFlag(args, "--no-delivery-required"); - if (deliveryRequired && noDeliveryRequired) - { - Console.Error.WriteLine("[FAIL] --delivery-required and --no-delivery-required cannot be used together."); - return (null, 1); - } + if (hasEventHeader) + route.Verification.EventHeaderName = eventHeader; - if (deliveryRequired) - route.DeliveryRequired = true; - if (noDeliveryRequired) - route.DeliveryRequired = false; + if (hasDeliveryHeader) + route.Verification.DeliveryIdHeaderName = deliveryHeader; - if (!TryGetFlagValue(args, "--notification-channel", out var notificationChannel, out var hasNotificationChannel)) - return (null, 1); + if (hasTimestampField) + route.Verification.TimestampField = timestampField; - if (hasNotificationChannel) - { - route.NotificationTarget ??= new NotificationTargetConfig(); - route.NotificationTarget.ChannelId = notificationChannel; - } + if (hasSignatureField) + route.Verification.SignatureField = signatureField; - // Parse limits - if (!TryGetFlagValue(args, "--max-body", out var maxBody, out var hasMaxBody)) - return (null, 1); + if (hasPayloadSeparator) + route.Verification.SignedPayloadSeparator = payloadSeparator; - if (hasMaxBody) - { - if (!int.TryParse(maxBody, out var bytes) || bytes < 1) - { - Console.Error.WriteLine($"[FAIL] Invalid max body size: '{maxBody}'. Must be a positive integer."); - return (null, 1); - } - route.MaxBodyBytes = bytes; - } + if (hasTolerance) + route.Verification.ToleranceSeconds = toleranceSeconds; - if (!TryGetFlagValue(args, "--rate-limit", out var rateLimit, out var hasRateLimit)) - return (null, 1); + // The merged kind decides this, because an omitted --verification-kind + // keeps the stored kind. + if ((hasTimestampField || hasSignatureField || hasPayloadSeparator || hasTolerance) + && route.Verification.Kind != WebhookVerifierKind.HmacTimestamped) + { + Console.Error.WriteLine("[FAIL] Timestamp signature options require '--verification-kind hmac-timestamped'."); + return (null, 1); + } - if (hasRateLimit) - { - if (!int.TryParse(rateLimit, out var limit) || limit < 1) - { - Console.Error.WriteLine($"[FAIL] Invalid rate limit: '{rateLimit}'. Must be a positive integer."); - return (null, 1); - } - route.RateLimitPerMinute = limit; - } + if (hasEvents) + route.Events = [.. events]; - // Parse enabled/disabled - var enabled = HasFlag(args, "--enabled"); - var disabled = HasFlag(args, "--disabled"); - if (enabled && disabled) - { - Console.Error.WriteLine("[FAIL] --enabled and --disabled cannot be used together."); - return (null, 1); - } + if (hasAudience) + route.Audience = audience; - if (enabled) - route.Enabled = true; - if (disabled) - route.Enabled = false; + if (hasNotifyInstructions) + route.NotifyInstructions = notifyInstructions; - // Validate - var errors = WebhookRouteValidator.Validate(routeName, route); - if (errors.Count > 0) - { - Console.Error.WriteLine($"[FAIL] Webhook route '{routeName}' has validation errors:"); - foreach (var error in errors) - { - Console.Error.WriteLine($" - {error}"); - } - return (null, 1); - } + if (deliveryRequired) + route.DeliveryRequired = true; + if (noDeliveryRequired) + route.DeliveryRequired = false; + + if (hasNotificationChannel) + { + route.NotificationTarget ??= new NotificationTargetConfig(); + route.NotificationTarget.ChannelId = notificationChannel; + } - if (dryRun) + if (hasMaxBody) + route.MaxBodyBytes = maxBodyBytes; + + if (hasRateLimit) + route.RateLimitPerMinute = rateLimit; + + if (enabled) + route.Enabled = true; + if (disabled) + route.Enabled = false; + + var errors = WebhookRouteValidator.Validate(routeName, route); + if (errors.Count > 0) + { + Console.Error.WriteLine($"[FAIL] Webhook route '{routeName}' has validation errors:"); + foreach (var error in errors) { - output.WriteLine($"[OK] Webhook route '{routeName}' is valid (dry run, not saved)."); - output.WriteLine($" Endpoint: /api/webhooks/{routeName}"); - return (null, 0); + Console.Error.WriteLine($" - {error}"); } + return (null, 1); + } + + if (dryRun) + { + output.WriteLine($"[OK] Webhook route '{routeName}' is valid (dry run, not saved)."); + output.WriteLine($" Endpoint: /api/webhooks/{routeName}"); + return (null, 0); + } + + updatedExistingRoute = exists; + return (route, 0); + } - routeSaved = true; - updatedExistingRoute = exists; - return (route, 0); - }); + WebhookRouteConfig? existing; + WebhookRouteConfig? merged; + int result; + try + { + existing = ReadExistingRoute(store, routeName); + (merged, result) = Merge(existing); } - catch (Exception ex) when (ex is InvalidDataException or TimeoutException) + catch (InvalidDataException ex) { Console.Error.WriteLine($"[FAIL] {ex.Message}"); return 1; } - if (routeSaved) + // A dry run and a rejected merge both send nothing, so neither needs the + // daemon. Merge already reported the reason. + if (merged is null) + return result; + + var available = await daemon.EnsureAvailableAsync(CancellationToken.None); + if (!available.Success) { - var action = updatedExistingRoute ? "Updated" : "Created"; - output.WriteLine($"[OK] {action} webhook route '{routeName}'."); - output.WriteLine($" File: {Path.Combine(paths.WebhooksDirectory, $"{routeName}.json")}"); - output.WriteLine($" Endpoint: /api/webhooks/{routeName}"); + Console.Error.WriteLine($"[FAIL] {available.Error}"); + return 1; } + var saved = await daemon.UpsertAsync(routeName, BuildPatch(existing is null), CancellationToken.None); + if (!saved.Success) + { + Console.Error.WriteLine($"[FAIL] {saved.Error}"); + return 1; + } + + var action = updatedExistingRoute ? "Updated" : "Created"; + output.WriteLine($"[OK] {action} webhook route '{routeName}'."); + output.WriteLine($" File: {Path.Combine(paths.WebhooksDirectory, $"{routeName}.json")}"); + output.WriteLine($" Endpoint: /api/webhooks/{routeName}"); + return result; + + // Projects the parsed flags into the daemon's field-level patch. An + // unspecified flag stays null so the daemon keeps the stored value. + WebhookRoutePatch BuildPatch(bool isNewRoute) => new() + { + Prompt = hasPrompt ? prompt : null, + Secret = hasSecret ? secret : null, + VerificationKind = hasVerificationKind ? verificationKind.ToString() : null, + // A new route keeps the CLI's documented 'public' default. Left null, + // the daemon would mint the route at the caller's own authority, which + // is higher than the flag default and would raise the route's audience. + Audience = hasAudience + ? audience.ToWireValue() + : isNewRoute ? TrustAudience.Public.ToWireValue() : null, + Events = hasEvents ? events : null, + NotifyInstructions = hasNotifyInstructions ? notifyInstructions : null, + DeliveryRequired = ResolveToggle(deliveryRequired, noDeliveryRequired), + NotificationChannelId = hasNotificationChannel ? notificationChannel : null, + MaxBodyBytes = hasMaxBody ? maxBodyBytes : null, + RateLimitPerMinute = hasRateLimit ? rateLimit : null, + Enabled = ResolveToggle(enabled, disabled), + SignatureHeaderName = hasSignatureHeader ? signatureHeader : null, + SignaturePrefix = hasSignaturePrefix ? signaturePrefix : null, + SecretHeaderName = hasSecretHeader ? secretHeader : null, + EventHeaderName = hasEventHeader ? eventHeader : null, + DeliveryIdHeaderName = hasDeliveryHeader ? deliveryHeader : null, + TimestampField = hasTimestampField ? timestampField : null, + SignatureField = hasSignatureField ? signatureField : null, + SignedPayloadSeparator = hasPayloadSeparator ? payloadSeparator : null, + ToleranceSeconds = hasTolerance ? toleranceSeconds : null + }; + } + + private static bool? ResolveToggle(bool onFlag, bool offFlag) + => onFlag ? true : offFlag ? false : null; + + /// + /// Reads the stored route for the merge preview. An unparseable file stops + /// the command: the CLI must not send a patch built on a route it could not + /// read. + /// + private static WebhookRouteConfig? ReadExistingRoute(WebhookRouteStore store, string routeName) + { + if (!store.TryGet(routeName, out var match)) + return null; + + return match.Definition + ?? throw new InvalidDataException($"Existing webhook route '{routeName}' could not be parsed."); } // ── delete ── - private static int RunDelete(string[] args, WebhookRouteStore store, TextWriter output) + private static async Task RunDeleteAsync( + string[] args, + TextWriter output, + WebhookRouteDaemonClient daemon) { if (args.Length < 3) { @@ -567,18 +666,23 @@ private static int RunDelete(string[] args, WebhookRouteStore store, TextWriter } } - bool deleted; - try + var available = await daemon.EnsureAvailableAsync(CancellationToken.None); + if (!available.Success) { - deleted = store.Delete(routeName, CancellationToken.None); + Console.Error.WriteLine($"[FAIL] {available.Error}"); + return 1; } - catch (TimeoutException ex) + + // The probe already ran, so a 404 here is a missing route, not an old + // daemon without the resource. + var removed = await daemon.DeleteAsync(routeName, CancellationToken.None); + if (!removed.Success && !removed.NotFound) { - Console.Error.WriteLine($"[FAIL] {ex.Message}"); + Console.Error.WriteLine($"[FAIL] {removed.Error}"); return 1; } - if (!deleted) + if (!removed.Success) { Console.Error.WriteLine($"[FAIL] Webhook route '{routeName}' not found."); return 1; @@ -821,12 +925,16 @@ private static bool TryParseRouteName(string rawRouteName, out string routeName) { routeName = string.Empty; - if (!WebhookRouteStore.TryNormalizeRouteName(rawRouteName, out routeName, out var error)) + // The CLI keeps the name as a string: it travels over HTTP as a path + // segment. The parse still runs here so an operator typo gets its + // message before any daemon call. + if (!WebhookRouteName.TryCreate(rawRouteName, out var parsed, out var error)) { Console.Error.WriteLine($"[FAIL] {error}"); return false; } + routeName = parsed.Value; return true; } @@ -862,6 +970,9 @@ private static int WriteHelp(TextWriter output) output.WriteLine("Routes are stored in ~/.netclaw/config/webhooks/.json"); output.WriteLine("and served at /api/webhooks/ by the daemon."); output.WriteLine(); + output.WriteLine("'set' and 'delete' need a running daemon: the daemon owns route"); + output.WriteLine("changes. 'list', 'show', and 'validate' read the files directly."); + output.WriteLine(); output.WriteLine("Note: This command manages INBOUND webhook routes (external services"); output.WriteLine("calling Netclaw). For OUTBOUND notifications (Netclaw posting to Slack),"); output.WriteLine("see `netclaw secrets set Slack.BotToken` and notification target config."); @@ -872,7 +983,8 @@ private static void WriteSetHelp(TextWriter output) { output.WriteLine("Usage: netclaw webhooks set [options]"); output.WriteLine(); - output.WriteLine("Create or update an inbound webhook route."); + output.WriteLine("Create or update an inbound webhook route. The daemon must be"); + output.WriteLine("running: it owns every route change. --dry-run needs no daemon."); output.WriteLine(); output.WriteLine("Required (for new routes):"); output.WriteLine(" --prompt Prompt instructions for the agent"); diff --git a/src/Netclaw.Configuration.Tests/WebhookRouteStoreTests.cs b/src/Netclaw.Configuration.Tests/WebhookRouteStoreTests.cs index 078364759..337d67e3a 100644 --- a/src/Netclaw.Configuration.Tests/WebhookRouteStoreTests.cs +++ b/src/Netclaw.Configuration.Tests/WebhookRouteStoreTests.cs @@ -30,12 +30,12 @@ public void Dispose() [InlineData("github-issues")] [InlineData("x")] [InlineData("route-2")] - public void TryNormalizeRouteName_AcceptsValidKebabCase(string value) + public void TryCreate_AcceptsValidKebabCase(string value) { - var ok = WebhookRouteStore.TryNormalizeRouteName(value, out var normalized, out var error); + var ok = WebhookRouteName.TryCreate(value, out var routeName, out var error); Assert.True(ok); - Assert.Equal(value, normalized); + Assert.Equal(value, routeName.Value); Assert.Null(error); } @@ -52,9 +52,9 @@ public void TryNormalizeRouteName_AcceptsValidKebabCase(string value) [InlineData("-foo")] [InlineData("foo-")] [InlineData("foo--bar")] - public void TryNormalizeRouteName_RejectsInvalidNames(string value) + public void TryCreate_RejectsInvalidNames(string value) { - var ok = WebhookRouteStore.TryNormalizeRouteName(value, out _, out var error); + var ok = WebhookRouteName.TryCreate(value, out _, out var error); Assert.False(ok); Assert.False(string.IsNullOrWhiteSpace(error)); @@ -74,7 +74,7 @@ public void Delete_RejectsTraversalRouteName() { var store = new WebhookRouteStore(_paths); - Assert.Throws(() => store.Delete("../secrets", CancellationToken.None)); + Assert.Throws(() => store.Delete("../secrets")); } [Fact] @@ -224,111 +224,6 @@ public void Route_rejects_undefined_numeric_verification_enums(bool invalidKind) Assert.Contains(errors, error => error.Contains("not supported", StringComparison.Ordinal)); } - [Fact] - public async Task Update_serializes_read_modify_write_operations_across_store_instances_and_path_aliases() - { - var firstStore = new WebhookRouteStore(_paths); - string? aliasPath = null; - string? parentAliasPath = null; - NetclawPaths secondPaths = _paths; - if (!OperatingSystem.IsWindows()) - { - var parentPath = Path.GetDirectoryName(_dir.Path) - ?? throw new InvalidOperationException("Test directory has no parent directory."); - parentAliasPath = $"{_dir.Path}-parent-alias"; - Directory.CreateSymbolicLink(parentAliasPath, parentPath); - - var targetThroughParentAlias = Path.Combine(parentAliasPath, Path.GetFileName(_dir.Path)); - aliasPath = $"{_dir.Path}-alias"; - Directory.CreateSymbolicLink(aliasPath, targetThroughParentAlias); - secondPaths = new NetclawPaths(aliasPath); - } - - var secondStore = new WebhookRouteStore(secondPaths); - firstStore.Save("concurrent-route", CreateValidRoute()); - using var firstEntered = new ManualResetEventSlim(); - using var secondStarted = new ManualResetEventSlim(); - using var releaseFirst = new ManualResetEventSlim(); - var cancellationToken = TestContext.Current.CancellationToken; - - try - { - var first = Task.Run(() => firstStore.Update("concurrent-route", cancellationToken, existing => - { - firstEntered.Set(); - Assert.True(secondStarted.Wait(TimeSpan.FromSeconds(10), cancellationToken)); - Assert.True(releaseFirst.Wait(TimeSpan.FromSeconds(10), cancellationToken)); - existing!.RateLimitPerMinute = 12; - return (existing, true); - }), cancellationToken); - Assert.True(firstEntered.Wait(TimeSpan.FromSeconds(10), cancellationToken)); - - var second = Task.Run(() => - { - secondStarted.Set(); - return secondStore.Update("concurrent-route", cancellationToken, existing => - { - existing!.MaxBodyBytes = 2048; - return (existing, true); - }); - }, cancellationToken); - - Assert.True(secondStarted.Wait(TimeSpan.FromSeconds(10), cancellationToken)); - releaseFirst.Set(); - await Task.WhenAll(first, second); - - Assert.True(firstStore.TryGet("concurrent-route", out var saved)); - Assert.Equal(12, saved.Definition!.RateLimitPerMinute); - Assert.Equal(2048, saved.Definition.MaxBodyBytes); - } - finally - { - releaseFirst.Set(); - if (aliasPath is not null) - Directory.Delete(aliasPath); - if (parentAliasPath is not null) - Directory.Delete(parentAliasPath); - } - } - - [Fact] - public async Task Update_lock_wait_honors_cancellation() - { - var firstStore = new WebhookRouteStore(_paths); - firstStore.Save("cancelled-update", CreateValidRoute()); - using var firstEntered = new ManualResetEventSlim(); - using var releaseFirst = new ManualResetEventSlim(); - using var cancellation = new CancellationTokenSource(); - var testCancellation = TestContext.Current.CancellationToken; - - var first = Task.Run(() => firstStore.Update( - "cancelled-update", - testCancellation, - existing => - { - firstEntered.Set(); - Assert.True(releaseFirst.Wait(TimeSpan.FromSeconds(10), testCancellation)); - return (existing, true); - }), testCancellation); - Assert.True(firstEntered.Wait(TimeSpan.FromSeconds(10), testCancellation)); - - var second = Task.Run(() => firstStore.Update( - "cancelled-update", - cancellation.Token, - existing => (existing, true)), testCancellation); - cancellation.Cancel(); - - try - { - await Assert.ThrowsAnyAsync(() => second); - } - finally - { - releaseFirst.Set(); - await first; - } - } - [Fact] public void Embedded_config_and_route_schemas_share_timestamped_verification_contract() { diff --git a/src/Netclaw.Configuration/WebhookRouteName.cs b/src/Netclaw.Configuration/WebhookRouteName.cs new file mode 100644 index 000000000..c9ae994b9 --- /dev/null +++ b/src/Netclaw.Configuration/WebhookRouteName.cs @@ -0,0 +1,79 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- +using System.Text.RegularExpressions; + +namespace Netclaw.Configuration; + +/// +/// A validated webhook route name. The name is the URL path segment of an +/// inbound webhook and the file name of its definition, so it must be safe for +/// both. This type is the one place that decides what a route name may be. +/// +/// A value exists only through or , +/// so a value that exists is always trimmed, lowercase, and kebab-case. Read +/// the name through . There is no implicit conversion: a +/// route name must never become a plain string by accident. +/// +/// +/// A default value carries no name. throws for it +/// rather than return a substitute. +/// +/// +public readonly record struct WebhookRouteName +{ + private static readonly Regex RouteNamePattern = new( + "^[a-z0-9]+(?:-[a-z0-9]+)*$", + RegexOptions.Compiled | RegexOptions.CultureInvariant); + + private readonly string? _value; + + private WebhookRouteName(string value) => _value = value; + + /// The validated, normalized route name. + public string Value => _value ?? throw new InvalidOperationException( + "Webhook route name is not initialized. Build one with TryCreate or Create."); + + /// + /// Normalizes and validates a candidate route name. Returns false and an + /// operator-facing message when the candidate is not a route name. + /// + public static bool TryCreate(string? candidate, out WebhookRouteName routeName, out string? error) + { + routeName = default; + + var normalized = candidate?.Trim().ToLowerInvariant(); + if (string.IsNullOrWhiteSpace(normalized)) + { + error = "Webhook route name is required."; + return false; + } + + if (!RouteNamePattern.IsMatch(normalized)) + { + error = + "Webhook route name must be lowercase kebab-case (letters, numbers, single dashes)."; + return false; + } + + routeName = new WebhookRouteName(normalized); + error = null; + return true; + } + + /// + /// Normalizes and validates a candidate route name, or throws. Use this + /// where an invalid name is a programming error, not operator input. + /// + public static WebhookRouteName Create(string candidate) + { + if (!TryCreate(candidate, out var routeName, out var error)) + throw new ArgumentException(error, nameof(candidate)); + + return routeName; + } + + public override string ToString() => _value ?? "(uninitialized)"; +} diff --git a/src/Netclaw.Configuration/WebhookRouteStore.cs b/src/Netclaw.Configuration/WebhookRouteStore.cs index 46478aa0a..58fcab24e 100644 --- a/src/Netclaw.Configuration/WebhookRouteStore.cs +++ b/src/Netclaw.Configuration/WebhookRouteStore.cs @@ -3,24 +3,22 @@ // Copyright (C) 2026 - 2026 Petabridge, LLC // // ----------------------------------------------------------------------- -using System.Diagnostics; -using System.Security.Cryptography; -using System.Text; using System.Text.Json; using System.Text.Json.Serialization; -using System.Text.RegularExpressions; namespace Netclaw.Configuration; +/// +/// Reads and writes the per-route webhook JSON files. One route is one file. +/// +/// The store takes no lock. Inside the daemon, WebhookRouteActor is the +/// only writer, and its mailbox serializes every read-modify-write. Each write +/// is still atomic on its own: the store writes a temporary file and then +/// replaces the route file in one move, so no reader ever sees a partial file. +/// +/// public sealed class WebhookRouteStore { - private static readonly TimeSpan RouteLockTimeout = TimeSpan.FromSeconds(30); - private static readonly TimeSpan RouteLockPollInterval = TimeSpan.FromMilliseconds(50); - - private static readonly Regex RouteNamePattern = new( - "^[a-z0-9]+(?:-[a-z0-9]+)*$", - RegexOptions.Compiled | RegexOptions.CultureInvariant); - private static readonly JsonSerializerOptions JsonOptions = new(JsonSerializerDefaults.Web) { PropertyNameCaseInsensitive = true, @@ -36,40 +34,6 @@ public WebhookRouteStore(NetclawPaths paths) Directory.CreateDirectory(_paths.WebhooksDirectory); } - /// - /// Normalizes a route name to lowercase kebab-case format. - /// - public static string NormalizeRouteName(string value) - { - if (!TryNormalizeRouteName(value, out var normalized, out var error)) - throw new ArgumentException(error, nameof(value)); - - return normalized; - } - - /// - /// Attempts to normalize and validate a route name. - /// - public static bool TryNormalizeRouteName(string value, out string normalized, out string? error) - { - normalized = value.Trim().ToLowerInvariant(); - if (string.IsNullOrWhiteSpace(normalized)) - { - error = "Webhook route name is required."; - return false; - } - - if (!RouteNamePattern.IsMatch(normalized)) - { - error = - "Webhook route name must be lowercase kebab-case (letters, numbers, single dashes)."; - return false; - } - - error = null; - return true; - } - /// /// Attempts to read a single route by name. More efficient than /// when you only need one route. @@ -93,25 +57,20 @@ public bool TryGet(string routeName, out (string FilePath, WebhookRouteConfig? D .ToList(); public void Save(string routeName, WebhookRouteConfig definition) - { - var filePath = GetPath(routeName); - using var routeLock = AcquireRouteLock(filePath, CancellationToken.None); - Write(filePath, definition); - } + => Write(GetPath(routeName), definition); /// - /// Reads and conditionally replaces one route while holding a route-scoped interprocess lock. - /// Returning a null definition leaves the file unchanged. + /// Reads one route, gives it to , and writes the + /// result back. Returning a null definition leaves the file unchanged. + /// The caller owns the serialization of concurrent updates. /// public TResult Update( string routeName, - CancellationToken cancellationToken, Func update) { ArgumentNullException.ThrowIfNull(update); var filePath = GetPath(routeName); - using var routeLock = AcquireRouteLock(filePath, cancellationToken); WebhookRouteConfig? existing = null; if (File.Exists(filePath)) { @@ -127,10 +86,9 @@ public TResult Update( return outcome.Result; } - public bool Delete(string routeName, CancellationToken cancellationToken) + public bool Delete(string routeName) { var filePath = GetPath(routeName); - using var routeLock = AcquireRouteLock(filePath, cancellationToken); if (!File.Exists(filePath)) return false; @@ -166,117 +124,9 @@ private void Write(string filePath, WebhookRouteConfig definition) } } - private static IDisposable AcquireRouteLock(string filePath, CancellationToken cancellationToken) - { - var canonicalPath = GetCanonicalLockPath(filePath); - var lockIdentity = OperatingSystem.IsWindows() - ? canonicalPath.ToUpperInvariant() - : canonicalPath; - var lockId = Convert.ToHexString(SHA256.HashData(Encoding.UTF8.GetBytes(lockIdentity))); - var lockScope = OperatingSystem.IsWindows() ? @"Global\" : string.Empty; - var mutex = new Mutex(initiallyOwned: false, $"{lockScope}netclaw-webhook-route-{lockId}"); - var ownsMutex = false; - - try - { - try - { - ownsMutex = WaitForMutex(mutex, cancellationToken); - } - catch (AbandonedMutexException) - { - ownsMutex = true; - // The abandoning process exited while holding the route lock. This process now owns it, - // and the route's atomic file replacement guarantees the existing file is still complete. - DeleteAbandonedTempFiles(canonicalPath); - } - - if (!ownsMutex) - throw new TimeoutException($"Timed out waiting to update webhook route '{Path.GetFileNameWithoutExtension(filePath)}'."); - - return new RouteLock(mutex); - } - catch - { - if (ownsMutex) - mutex.ReleaseMutex(); - mutex.Dispose(); - throw; - } - } - - private static bool WaitForMutex(Mutex mutex, CancellationToken cancellationToken) - { - if (!cancellationToken.CanBeCanceled) - return mutex.WaitOne(RouteLockTimeout); - - var startedAt = Stopwatch.GetTimestamp(); - while (true) - { - cancellationToken.ThrowIfCancellationRequested(); - if (mutex.WaitOne(TimeSpan.Zero)) - return true; - - var remaining = RouteLockTimeout - Stopwatch.GetElapsedTime(startedAt); - if (remaining <= TimeSpan.Zero) - return false; - - var wait = remaining < RouteLockPollInterval ? remaining : RouteLockPollInterval; - if (cancellationToken.WaitHandle.WaitOne(wait)) - cancellationToken.ThrowIfCancellationRequested(); - } - } - - private static string GetCanonicalLockPath(string filePath) - { - var fullPath = Path.GetFullPath(filePath); - var directoryPath = Path.GetDirectoryName(fullPath) - ?? throw new InvalidOperationException("Webhook route path has no parent directory."); - return Path.Combine(GetCanonicalDirectoryPath(directoryPath), Path.GetFileName(fullPath)); - } - - private static string GetCanonicalDirectoryPath(string directoryPath) - { - var fullPath = Path.GetFullPath(directoryPath); - var root = Path.GetPathRoot(fullPath) - ?? throw new InvalidOperationException("Webhook route path has no root directory."); - var current = root; - var relativeDirectory = Path.GetRelativePath(root, fullPath); - foreach (var segment in relativeDirectory.Split( - [Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar], - StringSplitOptions.RemoveEmptyEntries)) - { - var directory = new DirectoryInfo(Path.Combine(current, segment)); - var target = directory.ResolveLinkTarget(returnFinalTarget: true); - current = target is null - ? directory.FullName - : GetCanonicalDirectoryPath(target.FullName); - } - - return current; - } - - private static void DeleteAbandonedTempFiles(string filePath) - { - var directory = Path.GetDirectoryName(filePath) - ?? throw new InvalidOperationException("Webhook route path has no parent directory."); - var fileName = Path.GetFileName(filePath); - foreach (var tempPath in Directory.EnumerateFiles(directory, $"{fileName}.*.tmp")) - File.Delete(tempPath); - } - - private sealed class RouteLock(Mutex mutex) : IDisposable - { - public void Dispose() - { - mutex.ReleaseMutex(); - mutex.Dispose(); - } - } - private string GetPath(string routeName) { - var normalizedRouteName = NormalizeRouteName(routeName); + var normalizedRouteName = WebhookRouteName.Create(routeName).Value; var webhooksRootPath = Path.GetFullPath(_paths.WebhooksDirectory); var path = Path.GetFullPath(Path.Combine(webhooksRootPath, $"{normalizedRouteName}.json")); diff --git a/src/Netclaw.Configuration/WebhookRouteValidator.cs b/src/Netclaw.Configuration/WebhookRouteValidator.cs index ac49cf12c..ca10d16b4 100644 --- a/src/Netclaw.Configuration/WebhookRouteValidator.cs +++ b/src/Netclaw.Configuration/WebhookRouteValidator.cs @@ -8,6 +8,12 @@ namespace Netclaw.Configuration; /// /// Shared validation logic for webhook route configurations. /// Used by both CLI commands and doctor checks. +/// +/// This is also the one place that enforces required-ness for a route. The +/// mutation message is a patch whose fields are nullable by design, so a route +/// gets its required fields checked here, on the merged definition, after the +/// patch is applied. +/// /// public static class WebhookRouteValidator { @@ -25,7 +31,7 @@ public static IReadOnlyList Validate(string routeName, WebhookRouteConfi return errors; } - if (!WebhookRouteStore.TryNormalizeRouteName(routeName, out _, out var routeNameError)) + if (!WebhookRouteName.TryCreate(routeName, out _, out var routeNameError)) errors.Add(routeNameError!); if (route.Verification is null) @@ -99,7 +105,7 @@ public static void ValidateOrThrow(string routeName, WebhookRouteConfig route) /// Validates a route name and returns a user-facing error if invalid. /// public static string? ValidateRouteName(string routeName) - => WebhookRouteStore.TryNormalizeRouteName(routeName, out _, out var error) + => WebhookRouteName.TryCreate(routeName, out _, out var error) ? null : error; diff --git a/src/Netclaw.Daemon.Tests/Webhooks/WebhookRouteEndpointTests.cs b/src/Netclaw.Daemon.Tests/Webhooks/WebhookRouteEndpointTests.cs new file mode 100644 index 000000000..2a816f336 --- /dev/null +++ b/src/Netclaw.Daemon.Tests/Webhooks/WebhookRouteEndpointTests.cs @@ -0,0 +1,389 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- +using System.Net; +using System.Net.Http.Json; +using System.Security.Claims; +using System.Text.Encodings.Web; +using System.Text.Json; +using Akka.Actor; +using Akka.Hosting; +using Microsoft.AspNetCore.Authentication; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.TestHost; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; +using Netclaw.Actors.Channels; +using Netclaw.Actors.Hosting; +using Netclaw.Actors.Reminders; +using Netclaw.Actors.Webhooks; +using Netclaw.Configuration; +using Netclaw.Daemon.Reminders; +using Netclaw.Daemon.Security; +using Netclaw.Daemon.Webhooks; +using Netclaw.Tests.Utilities; +using Xunit; + +namespace Netclaw.Daemon.Tests.Webhooks; + +/// +/// The /api/webhooks management resource. The tests run against the real +/// over a temporary route store, so the status +/// mapping and the persistence round trip are both exercised end to end. +/// +public sealed class WebhookRouteEndpointTests : IAsyncDisposable +{ + private readonly DisposableTempDir _dir = new(); + private readonly NetclawPaths _paths; + private readonly WebhookRouteStore _store; + private readonly ActorSystem _actorSystem; + private readonly IActorRef _routeActor; + + public WebhookRouteEndpointTests() + { + _paths = new NetclawPaths(_dir.Path); + _paths.EnsureDirectoriesExist(); + _store = new WebhookRouteStore(_paths); + + _actorSystem = ActorSystem.Create($"webhook-route-endpoint-tests-{Guid.NewGuid():N}"); + _routeActor = _actorSystem.ActorOf(WebhookRouteActor.CreateProps(_store)); + } + + public async ValueTask DisposeAsync() + { + await _actorSystem.Terminate(); + _dir.Dispose(); + } + + private static object ValidRouteBody(string secret = "endpoint-secret") => new + { + prompt = "Handle inbound delivery.", + secret, + verificationKind = "Hmac" + }; + + // ── Auth ── + + /// + /// Auth parity: the new resource is rejected by exactly the rules that + /// already reject the sibling /api/reminders resource. + /// + [Fact] + public async Task Unauthenticated_requests_are_rejected_like_the_sibling_api_surface() + { + await using var app = await CreateAppAsync(spoofLoopback: false); + var client = app.GetTestClient(); + + var webhooks = await client.GetAsync("/api/webhooks", TestContext.Current.CancellationToken); + var reminders = await client.GetAsync("/api/reminders", TestContext.Current.CancellationToken); + + Assert.Equal(HttpStatusCode.Unauthorized, reminders.StatusCode); + Assert.Equal(reminders.StatusCode, webhooks.StatusCode); + } + + [Fact] + public async Task Unauthenticated_PUT_writes_no_route_file() + { + await using var app = await CreateAppAsync(spoofLoopback: false); + + var response = await app.GetTestClient().PutAsJsonAsync( + "/api/webhooks/unauthenticated-route", ValidRouteBody(), TestContext.Current.CancellationToken); + + Assert.Equal(HttpStatusCode.Unauthorized, response.StatusCode); + Assert.False(_store.TryGet("unauthenticated-route", out _)); + } + + [Fact] + public async Task NonOperator_PUT_is_forbidden_and_writes_no_route_file() + { + await using var app = await CreateAppAsync(spoofLoopback: false, addNonOperatorScheme: true); + var client = app.GetTestClient(); + client.DefaultRequestHeaders.Add(NonOperatorAuthHandler.HeaderName, NonOperatorAuthHandler.HeaderValue); + + var response = await client.PutAsJsonAsync( + "/api/webhooks/non-operator-route", ValidRouteBody(), TestContext.Current.CancellationToken); + + Assert.Equal(HttpStatusCode.Forbidden, response.StatusCode); + Assert.False(_store.TryGet("non-operator-route", out _)); + } + + // ── Status mapping ── + + [Fact] + public async Task Upsert_persists_through_the_actor_and_returns_the_stored_route() + { + await using var app = await CreateAppAsync(spoofLoopback: true); + + var response = await app.GetTestClient().PutAsJsonAsync( + "/api/webhooks/round-trip-route", ValidRouteBody(), TestContext.Current.CancellationToken); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + + var body = await response.Content.ReadAsStringAsync(TestContext.Current.CancellationToken); + var json = JsonSerializer.Deserialize(body); + Assert.Equal("round-trip-route", json.GetProperty("name").GetString()); + Assert.Equal("Handle inbound delivery.", json.GetProperty("prompt").GetString()); + Assert.Equal("Hmac", json.GetProperty("verification").GetProperty("kind").GetString()); + // The management surface never echoes the route secret. + Assert.DoesNotContain("endpoint-secret", body, StringComparison.Ordinal); + + // The actor wrote the route file, secret included. + Assert.True(_store.TryGet("round-trip-route", out var stored)); + Assert.Equal(new SensitiveString("endpoint-secret"), stored.Definition!.Verification.Secret); + Assert.Equal(TrustAudience.Personal, stored.Definition.Audience); + } + + [Fact] + public async Task Upsert_applies_a_field_level_patch_to_an_existing_route() + { + await using var app = await CreateAppAsync(spoofLoopback: true); + var client = app.GetTestClient(); + + var created = await client.PutAsJsonAsync( + "/api/webhooks/patched-route", ValidRouteBody(), TestContext.Current.CancellationToken); + Assert.Equal(HttpStatusCode.OK, created.StatusCode); + + var patched = await client.PutAsJsonAsync( + "/api/webhooks/patched-route", + new { rateLimitPerMinute = 5 }, + TestContext.Current.CancellationToken); + + Assert.Equal(HttpStatusCode.OK, patched.StatusCode); + Assert.True(_store.TryGet("patched-route", out var stored)); + Assert.Equal(5, stored.Definition!.RateLimitPerMinute); + Assert.Equal("Handle inbound delivery.", stored.Definition.Prompt); + Assert.Equal(new SensitiveString("endpoint-secret"), stored.Definition.Verification.Secret); + } + + [Fact] + public async Task Validation_failure_returns_400_with_the_validator_message_and_writes_no_file() + { + await using var app = await CreateAppAsync(spoofLoopback: true); + + var response = await app.GetTestClient().PutAsJsonAsync( + "/api/webhooks/no-secret-route", + new { prompt = "Handle inbound delivery.", verificationKind = "Hmac" }, + TestContext.Current.CancellationToken); + + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + var json = await response.Content.ReadFromJsonAsync(TestContext.Current.CancellationToken); + Assert.Equal("Verification secret is required.", json.GetProperty("error").GetString()); + Assert.False(_store.TryGet("no-secret-route", out _)); + } + + [Fact] + public async Task An_unsupported_verification_kind_returns_400_before_the_actor_is_asked() + { + await using var app = await CreateAppAsync(spoofLoopback: true); + + var response = await app.GetTestClient().PutAsJsonAsync( + "/api/webhooks/bad-kind-route", + new { prompt = "Handle inbound delivery.", secret = "s", verificationKind = "quantum" }, + TestContext.Current.CancellationToken); + + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + var json = await response.Content.ReadFromJsonAsync(TestContext.Current.CancellationToken); + Assert.Contains("verificationKind", json.GetProperty("error").GetString()!, StringComparison.Ordinal); + Assert.False(_store.TryGet("bad-kind-route", out _)); + } + + [Fact] + public async Task An_invalid_route_name_returns_400() + { + await using var app = await CreateAppAsync(spoofLoopback: true); + + var response = await app.GetTestClient().PutAsJsonAsync( + "/api/webhooks/Not_Kebab", ValidRouteBody(), TestContext.Current.CancellationToken); + + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + var json = await response.Content.ReadFromJsonAsync(TestContext.Current.CancellationToken); + Assert.Contains("kebab-case", json.GetProperty("error").GetString()!, StringComparison.Ordinal); + } + + [Fact] + public async Task Get_returns_the_route_without_its_secret() + { + await using var app = await CreateAppAsync(spoofLoopback: true); + var client = app.GetTestClient(); + await client.PutAsJsonAsync("/api/webhooks/readable-route", ValidRouteBody(), TestContext.Current.CancellationToken); + + var response = await client.GetAsync("/api/webhooks/readable-route", TestContext.Current.CancellationToken); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + var body = await response.Content.ReadAsStringAsync(TestContext.Current.CancellationToken); + Assert.DoesNotContain("endpoint-secret", body, StringComparison.Ordinal); + var json = JsonSerializer.Deserialize(body); + Assert.Equal("readable-route", json.GetProperty("name").GetString()); + } + + [Fact] + public async Task Get_of_an_unknown_route_returns_404() + { + await using var app = await CreateAppAsync(spoofLoopback: true); + + var response = await app.GetTestClient().GetAsync( + "/api/webhooks/missing-route", TestContext.Current.CancellationToken); + + Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); + } + + [Fact] + public async Task List_returns_a_summary_for_every_route() + { + await using var app = await CreateAppAsync(spoofLoopback: true); + var client = app.GetTestClient(); + await client.PutAsJsonAsync("/api/webhooks/alpha-route", ValidRouteBody(), TestContext.Current.CancellationToken); + await client.PutAsJsonAsync("/api/webhooks/beta-route", ValidRouteBody(), TestContext.Current.CancellationToken); + + var response = await client.GetAsync("/api/webhooks", TestContext.Current.CancellationToken); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + var body = await response.Content.ReadAsStringAsync(TestContext.Current.CancellationToken); + Assert.DoesNotContain("endpoint-secret", body, StringComparison.Ordinal); + var json = JsonSerializer.Deserialize(body); + Assert.Equal( + ["alpha-route", "beta-route"], + json.EnumerateArray().Select(x => x.GetProperty("name").GetString())); + } + + [Fact] + public async Task Delete_returns_204_and_removes_the_route_file() + { + await using var app = await CreateAppAsync(spoofLoopback: true); + var client = app.GetTestClient(); + await client.PutAsJsonAsync("/api/webhooks/doomed-route", ValidRouteBody(), TestContext.Current.CancellationToken); + + var response = await client.DeleteAsync("/api/webhooks/doomed-route", TestContext.Current.CancellationToken); + + Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); + Assert.False(_store.TryGet("doomed-route", out _)); + } + + [Fact] + public async Task Delete_of_an_unknown_route_returns_404() + { + await using var app = await CreateAppAsync(spoofLoopback: true); + + var response = await app.GetTestClient().DeleteAsync( + "/api/webhooks/missing-route", TestContext.Current.CancellationToken); + + Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); + } + + // ── App factory ── + + private async Task CreateAppAsync( + bool spoofLoopback, + bool addNonOperatorScheme = false) + { + var builder = WebApplication.CreateBuilder(); + builder.WebHost.UseTestServer(); + + builder.Services.AddSingleton(_paths); + builder.Services.AddSingleton(_store); + builder.Services.AddSingleton(); + builder.Services.AddSingleton>( + new FakeRequiredActor(_routeActor)); + + // The reminders resource is mapped only as the auth-parity reference. + // Its dependencies must resolve so minimal APIs bind them as services + // rather than inferring them as request bodies. + builder.Services.AddSingleton(TimeProvider.System); + builder.Services.AddSingleton(new SchedulingConfig { Enabled = true }); + builder.Services.AddSingleton(new ReminderDefinitionStore(_paths)); + builder.Services.AddSingleton(new ReminderHistoryStore(_paths)); + builder.Services.AddSingleton>( + new FakeRequiredActor(_actorSystem.DeadLetters)); + + if (addNonOperatorScheme) + { + builder.Services + .AddAuthentication("TestAuthSelector") + .AddPolicyScheme("TestAuthSelector", "non-operator or loopback", options => + { + options.ForwardDefaultSelector = ctx => + ctx.Request.Headers.ContainsKey(NonOperatorAuthHandler.HeaderName) + ? NonOperatorAuthHandler.SchemeName + : LoopbackAuthenticationHandler.SchemeName; + }) + .AddScheme( + NonOperatorAuthHandler.SchemeName, _ => { }) + .AddScheme( + LoopbackAuthenticationHandler.SchemeName, _ => { }); + builder.Services.AddSingleton(new DaemonConfig()); + } + else + { + builder.Services.AddNetclawAuthSchemes(new DaemonConfig()); + } + + builder.Services.AddAuthorization(); + builder.Services.AddLogging(); + + var app = builder.Build(); + + if (spoofLoopback) + { + app.Use(async (ctx, next) => + { + ctx.Connection.RemoteIpAddress = IPAddress.Loopback; + await next(ctx); + }); + } + + app.UseAuthentication(); + app.UseAuthorization(); + app.MapWebhookRouteEndpoints(); + // Mapped only as the auth-parity reference surface; its handlers are + // never reached because the parity assertion is unauthenticated. + app.MapReminderEndpoints(); + + await app.StartAsync(TestContext.Current.CancellationToken); + return app; + } + + // ── Fakes and helpers ── + + private sealed class FakeRequiredActor(IActorRef actorRef) : IRequiredActor + { + public IActorRef ActorRef => actorRef; + + public Task GetAsync(CancellationToken cancellationToken = default) + => Task.FromResult(actorRef); + } + + /// + /// Authenticates a request carrying X-Test-NonOperator as an + /// authenticated principal WITHOUT the Operator claim, so + /// classifies it as + /// . + /// + private sealed class NonOperatorAuthHandler : AuthenticationHandler + { + public const string SchemeName = "NonOperatorTest"; + public const string HeaderName = "X-Test-NonOperator"; + public const string HeaderValue = "ok"; + + public NonOperatorAuthHandler( + IOptionsMonitor options, + ILoggerFactory logger, + UrlEncoder encoder) + : base(options, logger, encoder) + { + } + + protected override Task HandleAuthenticateAsync() + { + if (!Request.Headers.TryGetValue(HeaderName, out var value) || value != HeaderValue) + return Task.FromResult(AuthenticateResult.NoResult()); + + var identity = new ClaimsIdentity([new Claim(ClaimTypes.Name, "device-user")], SchemeName); + return Task.FromResult(AuthenticateResult.Success( + new AuthenticationTicket(new ClaimsPrincipal(identity), SchemeName))); + } + } +} diff --git a/src/Netclaw.Daemon/Program.cs b/src/Netclaw.Daemon/Program.cs index 3d5f4411b..d42818a7d 100644 --- a/src/Netclaw.Daemon/Program.cs +++ b/src/Netclaw.Daemon/Program.cs @@ -316,6 +316,7 @@ static async Task RunDaemonAsync( .WithTags("Stats") .RequireAuthorization(); app.MapWebhookEndpoints(); + app.MapWebhookRouteEndpoints(); app.MapMattermostActionEndpoint(); app.MapPairingEndpoints(); @@ -1110,6 +1111,7 @@ static void ConfigureDaemonServices( akkaBuilder.WithNetclawSerialization(); akkaBuilder.WithNetclawActors(shellEnvironment, reminderStorage); + akkaBuilder.WithWebhookRouteActor(); akkaBuilder.WithSessionLogDispatcher(paths.SessionLogsDirectory, sp.GetRequiredService()); akkaBuilder.WithSignalRGateway(); akkaBuilder.WithDailyStatsActor(); @@ -1127,6 +1129,12 @@ static void ConfigureDaemonServices( var bgJobManager = registry.Get(); toolRegistry.WithBackgroundJobTools(bgJobManager); + // Route mutation tools ask the webhook route actor, so they register + // here rather than with the other first-party tools. The webhooks + // config gate matches the one on `list_webhooks` above. + if (webhooksConfig.Enabled) + toolRegistry.WithWebhookRouteTools(registry.Get()); + // Drain all active LLM sessions during any actor system termination (SIGTERM, daemon stop). // Runs in an early CoordinatedShutdown phase while actors are still alive. // If DaemonRestartCoordinator already drained sessions (config reload), the ingress diff --git a/src/Netclaw.Daemon/Webhooks/WebhookRouteEndpointRouteBuilderExtensions.cs b/src/Netclaw.Daemon/Webhooks/WebhookRouteEndpointRouteBuilderExtensions.cs new file mode 100644 index 000000000..96bd6a468 --- /dev/null +++ b/src/Netclaw.Daemon/Webhooks/WebhookRouteEndpointRouteBuilderExtensions.cs @@ -0,0 +1,322 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- +using Akka.Actor; +using Akka.Hosting; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Http.HttpResults; +using Microsoft.AspNetCore.Routing; +using Netclaw.Actors.Channels; +using Netclaw.Actors.Hosting; +using Netclaw.Configuration; +using static Netclaw.Actors.Webhooks.WebhookRouteProtocol; + +namespace Netclaw.Daemon.Webhooks; + +/// +/// Webhook route management resource. Every handler is a thin front over +/// WebhookRouteActor, the single mutation authority — validation and the +/// audience authority check live in the actor, never here. +/// +/// The resource is additive. It shares the /api/webhooks prefix with the +/// anonymous delivery endpoint (POST /api/webhooks/{route}) but claims +/// only the GET, PUT, and DELETE methods, so delivery is untouched. +/// +/// +public static class WebhookRouteEndpointRouteBuilderExtensions +{ + private static readonly TimeSpan AskTimeout = TimeSpan.FromSeconds(10); + + public static IEndpointRouteBuilder MapWebhookRouteEndpoints(this IEndpointRouteBuilder app) + { + var routes = app.MapGroup("/api/webhooks") + .WithTags("Webhooks") + .RequireAuthorization(); + + routes.MapGet("", async ValueTask>> ( + IRequiredActor actor, + CancellationToken ct) => + { + var routeActor = await actor.GetAsync(ct); + var response = await routeActor.Ask(ListRoutes.Instance, AskTimeout, ct); + return TypedResults.Ok(response.Routes.Select(ToSummary)); + }) + .WithName("ListWebhookRoutes") + .WithSummary("List configured inbound webhook routes."); + + routes.MapGet("/{name}", async ValueTask, NotFound, ProblemHttpResult>> ( + string name, + IRequiredActor actor, + CancellationToken ct) => + { + // A name the value object refuses can never name a stored file, so the + // caller gets the same answer as a missing route. + if (!WebhookRouteName.TryCreate(name, out var routeName, out _)) + return TypedResults.NotFound(new WebhookRouteErrorResponse($"Webhook route '{name}' not found.")); + + var routeActor = await actor.GetAsync(ct); + var response = await routeActor.Ask(new GetRoute(routeName), AskTimeout, ct); + + if (!response.Found) + return TypedResults.NotFound(new WebhookRouteErrorResponse($"Webhook route '{name}' not found.")); + + // The file exists but does not parse. Report it loudly instead of + // pretending the route is absent — a corrupt route file fails + // delivery closed and the operator needs to see that. + if (response.Route is null) + return TypedResults.Problem( + detail: $"Webhook route '{response.RouteName.Value}' exists but could not be parsed.", + statusCode: StatusCodes.Status500InternalServerError); + + return TypedResults.Ok(ToDto(response.RouteName.Value, response.Route)); + }) + .WithName("GetWebhookRoute") + .WithSummary("Get one webhook route. The response never carries the route secret."); + + routes.MapPut("/{name}", async ValueTask, BadRequest, ProblemHttpResult>> ( + string name, + UpsertWebhookRouteRequest request, + IRequiredActor actor, + ClaimsPrincipalMapper mapper, + HttpContext httpContext, + CancellationToken ct) => + { + // Creating or updating a route requires Operator authority, mirroring + // POST /api/reminders. Without it there is no audience to attribute + // the route to, and defaulting one would mint authority silently. + if (ResolveCreatorAudience(mapper, httpContext) is not { } creatorAudience) + return TypedResults.Problem( + detail: "Writing a webhook route requires Operator authority.", + statusCode: StatusCodes.Status403Forbidden); + + if (!WebhookRouteName.TryCreate(name, out var routeName, out var nameError)) + return TypedResults.BadRequest(new WebhookRouteErrorResponse(nameError!)); + + if (!TryBuildUpsert(routeName, request, creatorAudience, out var command, out var requestError)) + return TypedResults.BadRequest(new WebhookRouteErrorResponse(requestError!)); + + var routeActor = await actor.GetAsync(ct); + var response = await routeActor.Ask(command!, AskTimeout, ct); + + return response.Outcome switch + { + RouteSaveOutcome.Created or RouteSaveOutcome.Updated => + TypedResults.Ok(ToDto(response.RouteName.Value, response.Route!)), + RouteSaveOutcome.AuthorityRejected => TypedResults.Problem( + detail: response.ErrorMessage, + statusCode: StatusCodes.Status403Forbidden), + _ => TypedResults.BadRequest( + new WebhookRouteErrorResponse(response.ErrorMessage ?? "Webhook route rejected.")) + }; + }) + .WithName("UpsertWebhookRoute") + .WithSummary("Create or update a webhook route. Omitted fields keep their stored values."); + + routes.MapDelete("/{name}", async ValueTask>> ( + string name, + IRequiredActor actor, + CancellationToken ct) => + { + if (!WebhookRouteName.TryCreate(name, out var routeName, out _)) + return TypedResults.NotFound(new WebhookRouteErrorResponse($"Webhook route '{name}' not found.")); + + var routeActor = await actor.GetAsync(ct); + var response = await routeActor.Ask(new DeleteRoute(routeName), AskTimeout, ct); + + return response.Found + ? TypedResults.NoContent() + : TypedResults.NotFound(new WebhookRouteErrorResponse($"Webhook route '{name}' not found.")); + }) + .WithName("DeleteWebhookRoute") + .WithSummary("Delete a webhook route."); + + return app; + } + + /// + /// Maps the caller to the authority the route is created under. Only an + /// Operator carries one; every other principal gets null and is refused. + /// + private static TrustAudience? ResolveCreatorAudience(ClaimsPrincipalMapper mapper, HttpContext httpContext) + { + var identity = mapper.Map(httpContext.User); + return identity.Principal is PrincipalClassification.Operator + ? TrustAudience.Personal + : null; + } + + /// + /// Converts the request body's wire spellings into the actor's field-level + /// patch. A null property in the body means "leave the stored value + /// unchanged", the same rule the agent tool and the CLI already use. + /// + private static bool TryBuildUpsert( + WebhookRouteName routeName, + UpsertWebhookRouteRequest request, + TrustAudience creatorAudience, + out UpsertRoute? command, + out string? error) + { + command = null; + error = null; + + WebhookVerifierKind? verificationKind = null; + if (request.VerificationKind is not null) + { + if (!WebhookRouteValidator.TryParseVerifierKind(request.VerificationKind, out var parsedKind)) + { + error = "'verificationKind' must be 'Hmac', 'HmacTimestamped', or 'HeaderSecret'."; + return false; + } + + verificationKind = parsedKind; + } + + TrustAudience? requestedAudience = null; + if (request.Audience is not null) + { + if (!SecurityPolicyDefaults.TryParseAudience(request.Audience, out var parsedAudience)) + { + error = "'audience' must be Public, Team, or Personal."; + return false; + } + + requestedAudience = parsedAudience; + } + + command = new UpsertRoute + { + RouteName = routeName, + CreatorAudience = creatorAudience, + RequestedAudience = requestedAudience, + VerificationKind = verificationKind, + Prompt = request.Prompt, + Secret = request.Secret, + Events = request.Events, + NotifyInstructions = request.NotifyInstructions, + DeliveryRequired = request.DeliveryRequired, + NotificationChannelId = request.NotificationChannelId, + MaxBodyBytes = request.MaxBodyBytes, + RateLimitPerMinute = request.RateLimitPerMinute, + Enabled = request.Enabled, + SignatureHeaderName = request.SignatureHeaderName, + SignaturePrefix = request.SignaturePrefix, + SecretHeaderName = request.SecretHeaderName, + EventHeaderName = request.EventHeaderName, + DeliveryIdHeaderName = request.DeliveryIdHeaderName, + TimestampField = request.TimestampField, + SignatureField = request.SignatureField, + SignedPayloadSeparator = request.SignedPayloadSeparator, + ToleranceSeconds = request.ToleranceSeconds + }; + return true; + } + + private static WebhookRouteSummaryDto ToSummary(RouteEntry entry) => new( + Name: entry.RouteName, + Valid: entry.Definition is not null, + Enabled: entry.Definition?.Enabled, + Audience: entry.Definition?.Audience.ToWireValue(), + VerificationKind: entry.Definition?.Verification.Kind.ToString(), + DeliveryRequired: entry.Definition?.DeliveryRequired); + + /// + /// Projects a stored route for an HTTP response. The verification secret is + /// never projected: route files are secret-bearing config and the resource + /// is a management surface, not a secret-read surface. + /// + private static WebhookRouteDto ToDto(string routeName, WebhookRouteConfig route) => new( + Name: routeName, + Enabled: route.Enabled, + Audience: route.Audience.ToWireValue(), + Prompt: route.Prompt, + Events: route.Events, + NotifyInstructions: route.NotifyInstructions, + DeliveryRequired: route.DeliveryRequired, + NotificationChannelId: route.NotificationTarget?.ChannelId, + MaxBodyBytes: route.MaxBodyBytes, + RateLimitPerMinute: route.RateLimitPerMinute, + Verification: new WebhookVerificationDto( + Kind: route.Verification.Kind.ToString(), + HmacAlgorithm: route.Verification.HmacAlgorithm.ToString(), + SignatureHeaderName: route.Verification.SignatureHeaderName, + SignaturePrefix: route.Verification.SignaturePrefix, + SecretHeaderName: route.Verification.SecretHeaderName, + EventHeaderName: route.Verification.EventHeaderName, + DeliveryIdHeaderName: route.Verification.DeliveryIdHeaderName, + TimestampField: route.Verification.TimestampField, + SignatureField: route.Verification.SignatureField, + SignedPayloadSeparator: route.Verification.SignedPayloadSeparator, + ToleranceSeconds: route.Verification.ToleranceSeconds)); +} + +/// +/// Request body for PUT /api/webhooks/{name}. Every property is optional: +/// an omitted property leaves the stored value unchanged. +/// +internal sealed record UpsertWebhookRouteRequest +{ + public string? Prompt { get; init; } + public string? Secret { get; init; } + public string? VerificationKind { get; init; } + public string? Audience { get; init; } + public IReadOnlyList? Events { get; init; } + public string? NotifyInstructions { get; init; } + public bool? DeliveryRequired { get; init; } + public string? NotificationChannelId { get; init; } + public int? MaxBodyBytes { get; init; } + public int? RateLimitPerMinute { get; init; } + public bool? Enabled { get; init; } + public string? SignatureHeaderName { get; init; } + public string? SignaturePrefix { get; init; } + public string? SecretHeaderName { get; init; } + public string? EventHeaderName { get; init; } + public string? DeliveryIdHeaderName { get; init; } + public string? TimestampField { get; init; } + public string? SignatureField { get; init; } + public string? SignedPayloadSeparator { get; init; } + public int? ToleranceSeconds { get; init; } +} + +/// Summary projection returned by GET /api/webhooks. +internal sealed record WebhookRouteSummaryDto( + string Name, + bool Valid, + bool? Enabled, + string? Audience, + string? VerificationKind, + bool? DeliveryRequired); + +/// Full route projection, without the verification secret. +internal sealed record WebhookRouteDto( + string Name, + bool Enabled, + string Audience, + string Prompt, + IReadOnlyList Events, + string NotifyInstructions, + bool DeliveryRequired, + string? NotificationChannelId, + int MaxBodyBytes, + int RateLimitPerMinute, + WebhookVerificationDto Verification); + +/// Verification settings projection, without the secret. +internal sealed record WebhookVerificationDto( + string Kind, + string HmacAlgorithm, + string? SignatureHeaderName, + string? SignaturePrefix, + string? SecretHeaderName, + string? EventHeaderName, + string? DeliveryIdHeaderName, + string? TimestampField, + string? SignatureField, + string? SignedPayloadSeparator, + int? ToleranceSeconds); + +/// Error payload returned when a webhook route request fails. +internal sealed record WebhookRouteErrorResponse(string Error); diff --git a/tests/smoke/scenarios/webhook-routes.sh b/tests/smoke/scenarios/webhook-routes.sh new file mode 100755 index 000000000..7ab828724 --- /dev/null +++ b/tests/smoke/scenarios/webhook-routes.sh @@ -0,0 +1,180 @@ +#!/usr/bin/env bash +# webhook-routes.sh — goal: prove one webhook route lifecycle end to end +# through the real CLI, the real daemon, and the anonymous delivery endpoint. +# +# The CLI has no local route write path: `webhooks set` and `webhooks delete` +# go to the daemon, which owns route mutations. This scenario checks the whole +# loop against a running daemon: +# set -> exit 0 and the daemon writes the route file +# signed POST -> 202 accepted, with no daemon restart +# bad-sig POST -> 401, so verification fails closed +# delete -> exit 0, and a later POST answers 404, again with no restart +# +# The two delivery results after a mutation are the hot-reload proof: the +# catalog re-reads the route directory on each delivery, so a route the actor +# wrote (or removed) serves immediately. + +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=../../../scripts/smoke/lib/common.sh +. "${SCRIPT_DIR}/../../../scripts/smoke/lib/common.sh" + +command -v jq >/dev/null 2>&1 || die "jq is required for webhook-routes.sh" +command -v openssl >/dev/null 2>&1 || die "openssl is required for webhook-routes.sh" + +ROUTE_NAME="smoke-e2e-route" +ROUTE_SECRET="smoke-e2e-secret" +ROUTE_BODY='{"smoke":"e2e"}' +ROUTE_PROMPT="Report the smoke webhook payload in one word." +NETCLAW_JSON="${NETCLAW_HOME}/config/netclaw.json" +ROUTE_FILE="${NETCLAW_HOME}/config/webhooks/${ROUTE_NAME}.json" + +trap stop_daemon EXIT + +log "Seeding provider + model ($SMOKE_MODEL)..." +seed_provider_model + +# Inbound webhooks are off by default, and the delivery endpoint answers 404 +# for every route while the feature is off. Turn it on before the daemon reads +# its configuration. +log "Enabling inbound webhooks in netclaw.json..." +[[ -f "$NETCLAW_JSON" ]] || die "expected config file at $NETCLAW_JSON" +jq '.Webhooks = { "Enabled": true }' "$NETCLAW_JSON" >"${NETCLAW_JSON}.tmp" \ + || die "jq could not enable Webhooks in $NETCLAW_JSON" +mv "${NETCLAW_JSON}.tmp" "$NETCLAW_JSON" + +log "Starting daemon..." +start_daemon || die "daemon did not start" +wait_for_health || die "daemon health endpoint not ready" + +# post_delivery — POST one JSON delivery to the anonymous endpoint +# and print the HTTP status code. +post_delivery() { + local signature="$1" + run_timed "$STEP_TIMEOUT_SECONDS" \ + curl -sS -o /dev/null -w '%{http_code}' \ + -X POST "${DAEMON_BASE_URL}/api/webhooks/${ROUTE_NAME}" \ + -H 'Content-Type: application/json' \ + -H "X-Webhook-Signature: ${signature}" \ + --data-binary "$ROUTE_BODY" 2>/dev/null || true +} + +# The Hmac verifier expects HMAC-SHA256 over the raw body, hex-encoded in lower +# case, in the default X-Webhook-Signature header, with no prefix. +valid_signature="$(printf '%s' "$ROUTE_BODY" \ + | openssl dgst -sha256 -hmac "$ROUTE_SECRET" -r | awk '{print $1}')" +[[ -n "$valid_signature" ]] || die "openssl produced no HMAC signature" + +# ── set: the CLI writes through the daemon ── + +log "Creating route '$ROUTE_NAME' through the CLI..." +set_status=0 +set_output="$(nc webhooks set "$ROUTE_NAME" \ + --prompt "$ROUTE_PROMPT" \ + --secret "$ROUTE_SECRET" \ + --verification-kind hmac 2>&1)" || set_status=$? +echo "$set_output" +if [[ "$set_status" -eq 0 && "$set_output" == *"[OK] Created webhook route '${ROUTE_NAME}'."* ]]; then + pass "webhooks set: CLI reported the route as created" +else + die "webhooks set: expected exit 0 and a created line, got exit $set_status" +fi + +# The CLI never writes a route file, so the file on disk is the daemon's work. +if [[ -f "$ROUTE_FILE" ]]; then + pass "webhooks set: the daemon wrote $ROUTE_FILE" +else + die "webhooks set: expected the daemon to write $ROUTE_FILE" +fi + +log "Verifying 'webhooks list' shows the route..." +list_output="$(nc webhooks list 2>/dev/null || true)" +echo "$list_output" +if [[ "$list_output" == *"$ROUTE_NAME"* ]]; then + pass "webhooks list: includes $ROUTE_NAME" +else + fail "webhooks list: expected $ROUTE_NAME" +fi + +log "Verifying 'webhooks show' reports the route endpoint..." +show_output="$(nc webhooks show "$ROUTE_NAME" 2>/dev/null || true)" +echo "$show_output" +if [[ "$show_output" == *"/api/webhooks/${ROUTE_NAME}"* ]]; then + pass "webhooks show: reports endpoint /api/webhooks/${ROUTE_NAME}" +else + fail "webhooks show: expected endpoint /api/webhooks/${ROUTE_NAME}" +fi + +# ── delivery: the new route serves without a daemon restart ── + +log "Posting a correctly signed delivery (expect HTTP 202)..." +accepted_status="$(post_delivery "$valid_signature")" +log "HTTP status for the signed delivery: $accepted_status" +if [[ "$accepted_status" == "202" ]]; then + pass "delivery: signed POST accepted with 202 and no daemon restart" +else + fail "delivery: expected 202 for the signed POST, got $accepted_status" +fi + +log "Posting a wrongly signed delivery (expect HTTP 401)..." +rejected_status="$(post_delivery "0000000000000000000000000000000000000000000000000000000000000000")" +log "HTTP status for the wrongly signed delivery: $rejected_status" +if [[ "$rejected_status" == "401" ]]; then + pass "delivery: wrong signature rejected with 401" +else + fail "delivery: expected 401 for the wrong signature, got $rejected_status" +fi + +# ── delete: the route stops serving without a daemon restart ── + +log "Deleting route '$ROUTE_NAME' through the CLI..." +delete_status=0 +delete_output="$(nc webhooks delete "$ROUTE_NAME" --force 2>&1)" || delete_status=$? +echo "$delete_output" +if [[ "$delete_status" -eq 0 && "$delete_output" == *"[OK] Deleted webhook route '${ROUTE_NAME}'."* ]]; then + pass "webhooks delete: CLI reported the route as deleted" +else + die "webhooks delete: expected exit 0 and a deleted line, got exit $delete_status" +fi + +if [[ -f "$ROUTE_FILE" ]]; then + fail "webhooks delete: route file still present at $ROUTE_FILE" +else + pass "webhooks delete: the daemon removed $ROUTE_FILE" +fi + +log "Posting a signed delivery to the deleted route (expect HTTP 404)..." +deleted_status="$(post_delivery "$valid_signature")" +log "HTTP status after delete: $deleted_status" +if [[ "$deleted_status" == "404" ]]; then + pass "delivery: deleted route answers 404 with no daemon restart" +else + fail "delivery: expected 404 after delete, got $deleted_status" +fi + +# ── daemon-down contract: a route write requires the daemon ── + +log "Stopping the daemon to check the CLI write contract..." +stop_daemon + +down_status=0 +down_output="$(nc webhooks set "$ROUTE_NAME" \ + --prompt "$ROUTE_PROMPT" \ + --secret "$ROUTE_SECRET" \ + --verification-kind hmac 2>&1)" || down_status=$? +echo "$down_output" +if [[ "$down_status" -eq 1 && "$down_output" == *"daemon is not reachable"* ]]; then + pass "webhooks set: fails with exit 1 and a not-reachable message when the daemon is down" +else + fail "webhooks set: expected exit 1 and a not-reachable message, got exit $down_status" +fi + +if [[ -f "$ROUTE_FILE" ]]; then + fail "webhooks set: wrote $ROUTE_FILE without the daemon" +else + pass "webhooks set: wrote no route file without the daemon" +fi + +summarize +exit $?