Make a daemon actor the webhook route mutation authority - #2011
Conversation
| var api = CreateDaemonApi(request => Record(calls, request, r => r.Method == HttpMethod.Delete | ||
| ? new HttpResponseMessage(HttpStatusCode.NoContent) | ||
| : RouteListResponse())); | ||
| var stdout = new StringWriter(); |
67c75fa to
5ff8c3d
Compare
| public async Task Set_with_a_reachable_daemon_sends_the_patch_and_writes_no_file() | ||
| { | ||
| var daemon = FakeWebhookDaemon.Healthy(_paths); | ||
| var stdout = new StringWriter(); |
| public async Task Set_with_an_unreachable_daemon_fails_and_writes_no_file() | ||
| { | ||
| var daemon = FakeWebhookDaemon.Unreachable(_paths); | ||
| var stdout = new StringWriter(); |
| { | ||
| var daemon = FakeWebhookDaemon.Unreachable(_paths); | ||
| var stdout = new StringWriter(); | ||
| var stderr = new StringWriter(); |
| { | ||
| WriteRouteFile(); | ||
| var daemon = FakeWebhookDaemon.Unreachable(_paths); | ||
| var stdout = new StringWriter(); |
| WriteRouteFile(); | ||
| var daemon = FakeWebhookDaemon.Unreachable(_paths); | ||
| var stdout = new StringWriter(); | ||
| var stderr = new StringWriter(); |
748269f to
38ef62e
Compare
38ef62e to
a6d194a
Compare
a6d194a to
a5b3d07
Compare
a5b3d07 to
e7fc32c
Compare
| // 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(); |
| // remedy differs, so the message does too. | ||
| var daemon = FakeWebhookDaemon.WithoutRouteResource(_paths); | ||
| var stdout = new StringWriter(); | ||
| var stderr = new StringWriter(); |
| 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(); |
| 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(); |
e7fc32c to
b543995
Compare
b543995 to
92ca291
Compare
Aaronontheweb
left a comment
There was a problem hiding this comment.
Self-review: rationale and security trade-offs annotated inline at each decision site. Summary of the security envelope: every /api/webhooks verb sits behind the same RequireAuthorization() group policy as the other /api surfaces — loopback clients and paired remote devices only, via the device-pairing auth scheme — and route WRITES additionally require the Operator principal. No route can be created or changed by an unauthenticated caller, a non-paired device, or a non-Operator principal. The delivery endpoint (POST /api/webhooks/{route}) stays anonymous by design and is untouched.
| { | ||
| var routes = app.MapGroup("/api/webhooks") | ||
| .WithTags("Webhooks") | ||
| .RequireAuthorization(); |
There was a problem hiding this comment.
Security envelope. MapGroup("/api/webhooks").RequireAuthorization() is the identical pattern the reminders resource uses: the daemon's auth policy admits loopback clients and paired remote devices only. So the answer to "can this only be set over a paired device?" is yes — pairing (or loopback) is the floor for every verb here, and writes add an Operator gate on top (see the PUT handler). Exposure-mode rules apply unchanged because this rides the same middleware pipeline as every other /api surface.
| // 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) |
There was a problem hiding this comment.
Why writes refuse to default an authority. A webhook route is inbound attack surface: its audience decides what the fired session may do. Deriving a CreatorAudience for a non-Operator principal would mint authority silently, so anything that is not Operator gets a 403 here — fail closed. Trade-off accepted: a bare Operator PUT with no audience in the body mints the route at Personal (the Operator's own ceiling), while the CLI sends an explicit public for new routes and the agent tool inherits the session's audience. Three surfaces, three defaults — none exceeds the caller's own authority, so there is no escalation path, but the asymmetry is deliberate and worth a maintainer's conscious ack.
| .WithName("UpsertWebhookRoute") | ||
| .WithSummary("Create or update a webhook route. Omitted fields keep their stored values."); | ||
|
|
||
| routes.MapDelete("/{name}", async ValueTask<Results<NoContent, NotFound<WebhookRouteErrorResponse>>> ( |
There was a problem hiding this comment.
DELETE has no Operator gate — inherited decision. Any authenticated (paired/loopback) principal can remove a route, mirroring the reminders DELETE precedent and the old delete_webhook tool, which never had an audience guard. Rationale: deletion shrinks attack surface rather than minting authority. If you want symmetry with PUT instead, this is the one-line place to add it.
| UpsertRoute command, | ||
| WebhookRouteConfig? existing) | ||
| { | ||
| if (existing is not null && existing.Audience > command.CreatorAudience) |
There was a problem hiding this comment.
Downgrade-only authority, checked both ways. Two independent rejections: an existing route whose audience already exceeds the creator's authority cannot be touched at all (prevents a lower-authority caller laundering changes through a higher-authority route), and a requested audience above the creator's authority is refused (prevents minting). Order matters: authority errors take precedence over validation errors so a probing caller learns nothing about a route's contents from error ordering. This moved verbatim from SetWebhookTool so the guarantee now covers every front (tool, HTTP, CLI) in one place — the Cross-Boundary Contract Rule applied.
| || command.SignatureField is not null | ||
| || command.SignedPayloadSeparator is not null | ||
| || command.ToleranceSeconds is not null; | ||
| if (mergedKind != WebhookVerifierKind.HmacTimestamped && patchHasTimestampSettings) |
There was a problem hiding this comment.
Patch-scope guard, deliberately not merged-state scope. This rejects a patch that carries timestamp settings while the merged kind is not hmac-timestamped — closing the raw-HTTP hole where inert fields could persist and silently activate on a later kind flip. It intentionally does NOT strip stored timestamp fields when a patch downgrades the kind: those were configured legitimately while the kind was timestamped, and clearing them would break the uniform null-means-retain patch contract (the same way SignaturePrefix survives an Hmac→HeaderSecret→Hmac round trip). Policing the patch's own fields is the boundary; the validator continues to ignore timestamp fields for non-timestamped kinds at delivery time.
| /// <see cref="WebhookRouteStore"/>, so concurrent read-modify-write requests | ||
| /// serialize by mailbox order rather than by lock contention. | ||
| /// <para> | ||
| /// The actor is a plain <see cref="ReceiveActor"/> with no journal and no |
There was a problem hiding this comment.
Why cacheless, and why the mutex survives one release. No journal: Akka.Persistence would create a second copy of secret-bearing config plus wire-compat surface. No cache: disk stays the single source of truth, so a route file written behind the actor (an old CLI binary during version skew) is visible to the very next operation with zero reconciliation machinery. The store's named mutex stays exactly one deprecation release for that skew window — #2012 removes it once old CLI binaries age out. Known worst case during skew: an old CLI holding the mutex can stall this mailbox up to 30s while Asks time out at 10s; acceptable at route-mutation volumes and gone with #2012.
There was a problem hiding this comment.
This seems like over-engineering to me - just remove it now. People aren't adding webhooks nearly that often.
There was a problem hiding this comment.
Might as well just remove the mutex
There was a problem hiding this comment.
Done in 8299503. The mutex, its lock helpers, and the interim cross-process guard test are gone. The atomic temp-file write and the replacing move stay, so no reader sees a partial file. The accepted worst case is one lost update when an old CLI patches the same route at the same moment. Both spec copies now state the lock-free skew rule. #2012 can close after merge — its scope landed here.
| /// Reports whether the daemon can serve a route mutation. The probe runs once | ||
| /// per client instance, so one CLI invocation asks one time. | ||
| /// </summary> | ||
| public async Task<WebhookRouteApiResult> EnsureAvailableAsync(CancellationToken ct) |
There was a problem hiding this comment.
No fallback, by decision. The probe has exactly three outcomes and none of them writes a file: reachable resource → daemon path; unreachable or 404 (old daemon) → the command FAILS with the remedy; any refusal (400/401/403/5xx) → the command fails with the daemon's own message, because the daemon is the enforcement point and a fallback on refusal would bypass it. Maintainer decision, verbatim: "Don't have it." One store, one writer — a dual mode would preserve the second writer and with it the concurrency class the actor exists to remove.
| /// its request timeout on a linked token, so a timeout arrives as a | ||
| /// cancellation that the caller's own token did not request. | ||
| /// </summary> | ||
| private static bool IsDaemonUnreachable(Exception ex, CancellationToken ct) |
There was a problem hiding this comment.
Mid-flight failure is not retried and not fallen back. If the daemon dies between probe and write, the daemon may or may not have applied the change — a local file write here could apply it twice, and a silent retry could too. So the command fails closed and tells the operator to verify with show and retry. The !ct.IsCancellationRequested guard keeps caller-initiated cancellation propagating as cancellation instead of being converted into this failure result.
There was a problem hiding this comment.
This is correct - force the caller to retry. It's idempotent either way on the daemon side.
| if (merged is null) | ||
| return result; | ||
|
|
||
| var available = await daemon.EnsureAvailableAsync(CancellationToken.None); |
There was a problem hiding this comment.
Probe placement: last, not first. Argument grammar, the merge preview, the local validator, and --dry-run all answer before this line, so operator mistakes get their specific messages without a daemon round-trip, --dry-run needs no daemon at all, and the daemon is consulted exactly once per invocation, immediately before the one write it authorizes. The one deliberate breaking change in this PR lives here: with no reachable daemon this now fails (exit 1, remedy in the message) instead of writing the route file.
92ca291 to
6c89e27
Compare
Aaronontheweb
left a comment
There was a problem hiding this comment.
Some human-authored thoughts this time around
| /// <see cref="WebhookRouteStore"/>, so concurrent read-modify-write requests | ||
| /// serialize by mailbox order rather than by lock contention. | ||
| /// <para> | ||
| /// The actor is a plain <see cref="ReceiveActor"/> with no journal and no |
There was a problem hiding this comment.
This seems like over-engineering to me - just remove it now. People aren't adding webhooks nearly that often.
| } | ||
| catch (Exception ex) | ||
| { | ||
| _log.Warning(ex, "Webhook route '{0}' could not be read.", routeName); |
There was a problem hiding this comment.
Akka.NET's ILoggingAdapter supports semantic logging - take advantage of it. Do that generally throughout the PR.
There was a problem hiding this comment.
Done in 2c3763e. Every actor log call now uses named placeholders. I audited the rest of the PR: the endpoint file has no logger at all, and the CLI writes operator output, not log entries, so the actor was the only surface to convert.
| /// <see cref="WebhookRouteStore"/>, so concurrent read-modify-write requests | ||
| /// serialize by mailbox order rather than by lock contention. | ||
| /// <para> | ||
| /// The actor is a plain <see cref="ReceiveActor"/> with no journal and no |
There was a problem hiding this comment.
Might as well just remove the mutex
| Created: false, | ||
| Route: null, | ||
| WebhookRouteError.Authority, | ||
| $"Existing route audience '{existing.Audience.ToWireValue()}' exceeds creator authority ({command.CreatorAudience.ToWireValue()}).")); |
There was a problem hiding this comment.
LGTM - does this get logged somewhere too?
There was a problem hiding this comment.
It did not get logged before — good catch. Every rejection (authority and validation) now records a Warning with the route name, the rejection kind, the creator/requested/stored audiences, and the reason. It never records the secret. An EventFilter test proves the log entry exists (2c3763e).
|
|
||
| var definition = new WebhookRouteConfig | ||
| { | ||
| Enabled = command.Enabled ?? existing?.Enabled ?? true, |
There was a problem hiding this comment.
I'm a bit concerned about the nullability spam on here. Shouldn't we just make a lot more of these fields required? Are there backwards compatibility reasons for not doing that?
There was a problem hiding this comment.
The nullability here is the patch contract, not back-compat: null means "leave this field unchanged", which is what lets one UpsertRoute message serve create and partial update through the same RMW path. Required-ness is enforced on the merged result instead: WebhookRouteValidator already rejects an empty Prompt and a missing secret (for every kind, stricter than HMAC-only). b93484d adds the missing proof that a patch cannot blank the prompt, and the record now carries a remark that names the validator as the enforcement point.
| /// </summary> | ||
| public sealed record RouteSaved( | ||
| string RouteName, | ||
| bool Success, |
There was a problem hiding this comment.
Would an enum be better for this?
There was a problem hiding this comment.
Yes — done in e0ce25c. The two bools (Success, Created) and the separate error enum could disagree with each other. One RouteSaveOutcome enum (Created / Updated / ValidationRejected / AuthorityRejected) replaced all three; Success is now computed from it, and the HTTP handler maps it in one exhaustive switch.
| public sealed record RouteSaved( | ||
| string RouteName, | ||
| bool Success, | ||
| bool Created, |
There was a problem hiding this comment.
Ditto here. Would an enum be better?
There was a problem hiding this comment.
Folded into the same RouteSaveOutcome enum as the field above (e0ce25c) — one value now states what happened, so the reply cannot self-contradict.
| /// its request timeout on a linked token, so a timeout arrives as a | ||
| /// cancellation that the caller's own token did not request. | ||
| /// </summary> | ||
| private static bool IsDaemonUnreachable(Exception ex, CancellationToken ct) |
There was a problem hiding this comment.
This is correct - force the caller to retry. It's idempotent either way on the daemon side.
| UpsertRoute command, | ||
| WebhookRouteConfig? existing) | ||
| { | ||
| if (existing is not null && existing.Audience > command.CreatorAudience) |
| { | ||
| var routes = app.MapGroup("/api/webhooks") | ||
| .WithTags("Webhooks") | ||
| .RequireAuthorization(); |
8fbcd9b to
f587b07
Compare
Planning artifacts for the webhook route actor: proposal, design, webhook-route-authority spec delta, and tasks. They belong with the implementation PR, not with the test-handshake conversion.
Plan the single-writer WebhookRouteActor, the /api/webhooks resource, the CLI dual-mode write path, and the version-skew tolerance rules. The design records the decisions: plain actor over the existing store with disk canonical, reconciliation through the existing hot-reload signal, probe-based CLI mode selection where only unreachable or 404 selects the file path, and deterministic test replacement for the Windows-flaky mutex choreography.
WebhookRouteActor is a plain ReceiveActor with no journal and no cache. Every message reads the route file through the existing store, merges the message's field-level patch, validates with WebhookRouteValidator, and writes back. Concurrent read-modify-write requests serialize by mailbox order. Disk stays canonical, so an external file write is visible to the next actor operation with no reconciliation step. The store keeps its cross-process mutex for the version-skew window. set_webhook and delete_webhook now ask the actor; their schemas and result text are unchanged. New additive /api/webhooks resource (list, get, upsert, delete) fronts the actor with the reminders endpoint idiom, the same auth middleware, and an explicit Operator authority requirement for writes. Responses never carry verification secrets. The design's original signal-based reconciliation was rewritten: no route change signal exists in the codebase, and the inbound-webhooks spec permits mtime-gated pull. The cacheless actor supersedes it.
WebhookRouteWriteGateway is the single write-path seam for the CLI. It probes GET /api/webhooks once per invocation: reachable resource selects the daemon path; an unreachable daemon or a 404 from an old daemon selects the direct-file path with one stderr notice. Any other API status fails the command with the daemon's message and never falls back, so the daemon's enforcement point cannot be bypassed. stdout and exit codes are identical in both modes. A new CLI route sends an explicit public audience in the patch. A null audience would let the actor mint the route at the caller's authority, which would differ between modes. The TUI webhooks page has no route save (it edits only the Enabled and timeout settings and delegates authoring to the command), so it needs no mode seam; the design records this. Replace the two Windows-flaky mutex choreography tests with one outcome-only cross-process guard: concurrent updates through two store instances and a path alias, awaited unbounded under the test token, asserting no lost field. The guard is removed with the mutex follow-up.
Update the netclaw-operations skill (2.61.0) with the /api/webhooks management resource and the CLI direct-file mode notice. Sync the webhook-route-authority spec to the main specs.
PR #2007 removed the turn-complete lane outright, so task 6.6's enqueue gating has nothing to gate and task 5.3 keeps only the MemoryClass.Trace resolver-branch decision.
- A transport failure between the CLI probe and the write now fails the command with a readable message instead of an unhandled exception. It never falls back to a direct file write: the daemon may have applied the change before the connection broke. - The actor rejects timestamp verification settings when the merged verification kind is not hmac-timestamped. The tool and the CLI already reject this at their own fronts; the raw HTTP patch surface had no guard, so an inert field could persist and silently activate when the kind was later flipped. - Correct the stale task 1.4 wording left from the D2 amendment.
The eval daemon runs inside the container on UTC. The harness computed the log path with the host date, so a CDT-evening run that crossed UTC midnight pointed every daemon_log_contains at a file the daemon never wrote, and each log-based assert failed silently for the whole run. Resolve the newest daemon-*.log at each per-case baseline instead.
The eval container uses host networking with a fixed default port. Two concurrent runs collide: the loser's daemon crash-loops on address-in-use while the host-side readiness poll is answered by the winner's daemon, so the loser's whole run interrogates a stranger and every daemon-log assert reads its own dead container's empty log. Readiness now also proves the container's own daemon process is alive and aborts loudly with a NETCLAW_EVAL_PORT hint when it is not.
When the prompt timeout kills the CLI, block-buffered stdout dies with the process and a server-side-correct attempt scores as a hard fail with an empty capture file. stdbuf line-buffering flushes each line as it streams, so a killed attempt still leaves its partial transcript for the asserts and for triage.
The assert used gawk's three-argument match(). On a host whose awk is mawk, the script dies on a syntax error and the assert fails unconditionally, so the case could never pass regardless of daemon behavior — and the daemon behavior was verified correct on every attempt. Extract the counts with sub() instead of capture groups.
Maintainer decision on the disclosed dual mode: "Don't have it." The point of actor ownership is one writer; a CLI file path preserves the second writer and with it the concurrency class the actor exists to remove. set and delete now send every mutation to the daemon API and never write a route file. An unreachable daemon fails the command and tells the operator to start it. An old daemon without the resource fails and asks for an upgrade. A daemon refusal fails with the daemon's message. Reads stay on canonical disk: list, show, and validate are unchanged, and show needs the secret the API never returns. The daemon-absent path is a route file authored on disk, which the daemon loads at startup. WebhookRouteWriteGateway becomes WebhookRouteDaemonClient with no mode concept. Tests convert file-write assertions to recorded API payload assertions plus no-file checks; the specs, design, proposal, and the netclaw-operations skill record the decision.
The commit history records how the design changed. The artifacts now state the final design in one line per decision instead of narrating drafts.
Add the webhook-routes scenario to the light lane. It drives the real CLI against the real daemon: - webhooks set writes the route file through the daemon - a correctly signed POST answers 202 with no daemon restart - a wrongly signed POST answers 401 - webhooks delete makes a later POST answer 404, again with no restart - webhooks set with the daemon down exits 1 and writes no file
The skill said the feature defaults to enabled. WebhooksConfig.Enabled is an unset bool, so the real default is disabled — which matches the default-deny posture. The end-to-end smoke scenario surfaced the mismatch when it had to enable the feature explicitly.
The daemon actor is the single writer for webhook route files. A named OS mutex adds a stall risk during version skew and guards a race that the mailbox already removes. The store keeps its atomic write: it writes a temporary file and then replaces the route file in one move. No reader sees a partial file. Version-skew tolerance now rests on two properties: the actor holds no cache, and each write is atomic. The accepted worst case is one lost update when an old CLI patches the same route at the same moment. - delete the mutex, the lock wait, the lock path canonicalization, and the abandoned temp-file sweep - drop the now unused CancellationToken parameters on Update and Delete - delete the interim cross-process guard test - update the actor doc comment and both parity spec copies
Akka.NET's ILoggingAdapter supports named placeholders. The actor used positional placeholders, so the log backend saw one opaque string instead of named values. A refused route mutation was not recorded at all. A rejection is the one signal that a caller tried to take over a route above its own authority, or to mint one, so the actor now records every rejection at warning level. The record names the route, the rejection kind, the creator audience, the requested audience, the stored audience, and the reason. It never names the route secret. - convert the four operational warnings to named placeholders - add the Reject helper that both records and builds the reply - add an EventFilter test for the authority rejection record
…me enum A route name is the URL path segment and the file name of a route, so it must be safe for both. It travelled as a plain string, and each front normalized it again before it reached a file. WebhookRouteName is now the one place that decides what a route name may be. A value exists only through TryCreate or Create, so a value that exists is always trimmed, lowercase, and kebab-case. There is no implicit conversion to string. Every front parses the wire string once at its own boundary, so the actor and the store never see an unvalidated name. RouteSaved carried a success flag, a created flag, and an error code that could disagree with each other. One RouteSaveOutcome enum replaces all three: Created, Updated, ValidationRejected, or AuthorityRejected. The HTTP handler maps the enum to a status code with one exhaustive switch. Behavior does not change: an invalid name still returns 400 on PUT, 404 on GET and DELETE, and the same message from each agent tool.
The upsert message is a patch, so its fields are nullable by design. A null field means "keep the stored value". Required-ness therefore belongs to the merged definition, and WebhookRouteValidator already enforces it: it rejects a merged route without a prompt and one without a verification secret. The secret rule had a test on the merged result. The prompt rule did not, so a patch that blanks the prompt now has one. - add the actor test for a patch that blanks the prompt - name the validator as the required-ness enforcement point - record the patch contract in both parity spec copies
a1fd767 to
b2179ea
Compare
What
Implements OpenSpec change
webhook-route-actor-ownership(all artifacts in the PR): webhook route mutations serialize through one daemon-sideWebhookRouteActor, and the CLI's route mutations are daemon-only — there is no fallback (maintainer decision: one store, one writer; a dual mode would preserve the second writer and the concurrency class the actor exists to remove).WebhookRouteActor: plainReceiveActor, no journal, no cache — every message reads the route file through the existing store, merges a field-level patch (null = leave unchanged), validates, writes back. Concurrent RMW serializes by mailbox order. Disk stays canonical, so an external file write is visible to the next operation.set_webhook/delete_webhookask the actor; schemas and result text unchanged. The actor also rejects timestamp verification settings when the merged kind is nothmac-timestamped(the raw HTTP surface had no front-side guard)./api/webhooks(additive): GET list / GET-PUT-DELETE{name}, reminders-endpoint idiom, same auth middleware; PUT requires Operator authority (fail-closed, no defaulted authority); responses never carry verification secrets (asserted against raw JSON).WebhookRouteDaemonClient):set/deletesend every mutation to the daemon API and never write a route file. Daemon unreachable → exit 1, "The daemon is not reachable. Start the daemon to manage webhook routes." Old daemon without the resource → exit 1, "This daemon does not serve the webhook route API. Upgrade the daemon." Any daemon refusal → exit 1 with the daemon's own message. A transport failure mid-write fails closed and reports the uncertainty.list/show/validatekeep reading canonical disk (showneeds the secret the API never returns); argument grammar and--dry-runrun before any daemon call. The daemon-absent path is a route file authored on disk, loaded at daemon startup.tests/smoke/scenarios/webhook-routes.sh(in the PR-gating light lane) drives the real CLI against a real daemon — create via CLI → signed delivery202→ tampered signature401→ delete →404— and proves no daemon restart is needed for a new route (mtime-gated catalog reload). Also proves the daemon-down exit-1 contract.memory-core-redesigntasks superseded by the stack.Backward compatibility
Route file format unchanged and canonical; HTTP additive; tool schemas unchanged. The store's named mutex is removed now (maintainer review decision, supersedes #2012's deferred plan): skew tolerance rests on the cacheless actor plus atomic per-file writes; the accepted worst case is one lost update when an old CLI patches the same route at the same moment. Breaking by decision: a
netclaw webhooks set/deletewith no reachable daemon now fails instead of writing the file.Review incorporation (maintainer comments)
8299503e):WebhookRouteStoreloses the named mutex, its lock helpers, and the interim cross-process guard test; atomic temp-file + replacing move stays. Both spec copies rewrite the skew requirement. Close Remove the WebhookRouteStore cross-process mutex after the skew window #2012 after merge — its scope landed here.2c3763e7): all actor log calls use named placeholders. Authority and validation rejections were not logged before — every rejection now records a Warning with route name, rejection kind, creator/requested/stored audiences, and reason (never the secret), proven by anEventFiltertest.e0ce25cf): newWebhookRouteNamevalue object (kebab-case rule has one home; no implicit conversions) carried on every protocol message;RouteSaved's two bools + separate error enum collapse into oneRouteSaveOutcome(Created/Updated/ValidationRejected/AuthorityRejected) that cannot self-contradict, mapped exhaustively to HTTP statuses.b93484d6):UpsertRoutefields stay nullable — null means "leave unchanged" (the patch contract). Required-ness is enforced on the merged result byWebhookRouteValidator, which already requiredPromptand a secret for every kind; added the missing merged-result proof that a patch cannot blank the prompt.Review
Two adversarial passes: the original (drift/authority/secret-hygiene — findings fixed in-PR) and a post-rework delta review of the final form — SAFE TO MERGE: no file write reachable from any CLI path or ordering, failure-boundary ordering matches the spec's SHALLs, the actor guard aligns with both fronts, mid-flight cancellation semantics correct, converted tests prove their original claims.
Verification
Actors 3,457 / Daemon 1,036 / Cli 1,389 / Configuration 602 — 0 failures; webhook smoke scenario 11/11 (re-run after the review-incorporation commits); slopwatch 0; headers clean; zero build warnings. Eval gates: Skill 21/21, Memory 4/4 (after harness fixes;
memory_recall_filterswas failing on a gawk-only assert while the daemon was provably correct).netclaw-operationsskill → 2.61.0.