ROCMAI-83: extract webhook.rs from apps/rocmd/src/lib.rs

[jussielo-amd]

Summary

Third PR of Phase 5 (ROCMAI-83, part of the modularization epic ROCMAI-27). Pulls the local webhook source — axum routes, request validation, watcher-kind allow-list — into its own webhook.rs module. Fully self-contained except one call into payload_string, which stays at the crate root until watchers.rs is extracted in a later PR (that PR will need to re-point this one call site once it moves).

• Pure code motion, no behavior change.
• Call sites repointed to webhook::....
• The 17 tests that exercise this module's own logic directly moved into webhook.rs's own #[cfg(test)] mod in this same PR. Three other tests that happen to share the local_webhook_* name prefix but actually exercise Cli parsing / run_daemon were left in place — they'll move with cli.rs/service.rs respectively.
• docs/architecture.md updated in this PR.

Independent of #477 (persistence.rs) and #479 (common.rs) — branched separately from main, not stacked, per the one-off PR approach for this phase.

Test plan

• cargo build (rocmd, and rocm+rocmd together to confirm the external API surface is untouched)
• cargo clippy --workspace --all-targets -- -D warnings
• cargo test -p rocmd — 130 tests, same count as before this change (129 passed + 1 pre-existing ignored)
• cargo xtask manifest --check
• cargo fmt / prek hooks clean
• tests/e2e-cucumber (CI)

[Copilot]

Copilot review overview

🟡 Changes recommended

The architecture documentation inaccurately claims independent sibling modules are already present.

Review effort: Balanced
Findings: 1

Open (1)

• Architecture document references modules absent from this checkout · New

What changed in this PR

Extracts the rocmd local webhook implementation into a dedicated module without intended behavior changes.

Changes:

• Moves webhook routing, validation, and tests into webhook.rs.
• Repoints daemon and test call sites.
• Updates architecture documentation, though it prematurely lists sibling extractions as complete.

File	Description
apps/​rocmd/​src/​webhook.rs	Contains extracted webhook logic and tests.
apps/​rocmd/​src/​lib.rs	Registers and calls the webhook module.
docs/​architecture.md	Documents modularization progress.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[jussielo-amd]

Resolved by restacking rather than rewording: PR #480 is now based on rocmai-83-common (which carries #477's persistence.rs and #479's common.rs), per this repo's AGENTS.md §11 stacked-PR convention — merge order 477 → 479 → 480. The architecture.md hunk is now a genuinely incremental diff on top of the already-landed predecessor wording (adds webhook.rs, drops it from "still pending"), not a claim that can go stale out of order, since #480's base branch doesn't exist upstream until #479 merges.

[r0x0r]

Review: code is clean; holding approval on one doc line

The extraction verified as pure code motion, including the security-relevant parts, which I checked closely because this module is a network-facing endpoint that turns JSON into automation triggers. One issue in docs/architecture.md is the only thing between this and an approval.

The doc hunk describes a state this branch is not in

The new text claims persistence.rs and common.rs are extracted alongside webhook.rs, but neither file exists in this branch — only lib.rs, main.rs and webhook.rs are present.

This is worth more than a doc nit because of how the five PRs in this phase interact, which is invisible from inside any one of them:

• All five rewrite the same single line of docs/architecture.md, and all five branch independently from main. Four are therefore guaranteed to conflict there.
• Each one's text is a cumulative prefix assuming the earlier sequence has merged, so it is only true if they land in exactly the order 477 → 479 → 480 → 481 → 483.
• The failure mode is quiet: if this merges before #477 and #479, there is nothing to conflict with, so it applies cleanly and main asserts two files that do not exist.

docs/architecture.md says of itself that it is updated in the same PR as the code it documents and that "a stale-but-plausible-looking note is worse than an explicit prompt to check". Either fix works:

1. Scope the hunk to webhook.rs only, letting each sibling append its own module as it lands — self-consistent at any merge order, and turns the conflicts into trivial appends.
2. Keep the cumulative text and gate merge order via Depends on #477, #479 in the body.

(1) is more robust; (2) depends on whoever merges remembering the order.

Verified clean

• Validation logic is identical — this was my main concern and I went through it guard by guard against main: the watcher_hint empty/unknown check, the kind empty check, every arm of the local_webhook_kind_allowed match (all six watcher IDs and their allowed kinds), the service_id path-separator rejection via rocm_core::ServiceId::new, server-recover requiring service_id, cache-warm requiring payload.artifact_ref, and driver-upgrade requiring payload.component == "driver". Order of checks preserved. The only textual change in that region is the driver-upgrade guard being rewrapped across two lines, which is formatting. A reordered or dropped guard here would be a security regression that no amount of test-count arithmetic would catch, so it is worth stating explicitly that there isn't one.
• The three expected deltas and nothing else — fn → pub(crate) fn on exported items, payload_string(...) → crate::payload_string(...) in two guards, and that one rewrap.
• payload_string back-reference is sound: it is pub(crate) in lib.rs:4516, and the moved tests reach it only indirectly through local_webhook_event_from_request, so there is no test-scope visibility problem. Worth flagging for whoever writes watchers.rs: that PR has to repoint this call site when payload_string moves.
• Call sites — all four repointed (start_local_webhook_source, receive_local_webhook_event, local_webhook_event_from_request + LocalWebhookEventRequest).
• Imports — builtin_watcher and Deserialize correctly dropped from lib.rs; Serialize correctly kept, since print_json<T: Serialize> and call_engine still need it — that is the easy mistake in this diff and it was not made. TcpListener, mpsc and get looked like survivors on a mechanical pass, but all remaining uses are fully-qualified (std::net::TcpListener, std::sync::mpsc), so nothing fails under -D warnings. I confirmed these by hand.
• Field visibility — LocalWebhookSource's endpoint, receiver and task are pub(crate) and run_daemon reads all three by name.
• Tests — 130 attributes conserved (113 + 17), and the claimed 17 is exactly right: 15 #[test] plus 2 #[tokio::test(flavor = "multi_thread")]. The three left behind do genuinely exercise other things — local_webhook_help_mentions_loopback_only_binding and local_webhook_port_rejects_out_of_range_values hit Cli, local_webhook_requires_enabled_automation_loop hits run_daemon — so the shared name prefix was correctly treated as a red herring rather than a sorting key.
• Blast radius — no reference to any moved symbol outside apps/rocmd/src/, and no e2e scenario exercises the webhook endpoint.
• Leak scan (AGENTS.md §2) — clean.

---

🤖 by agent-hub on AMD AgentHub

[jussielo-amd]

Re the review withholding approval on the docs/architecture.md doc-ordering concern (persistence.rs/common.rs not existing in this branch's independent checkout):

Resolved by restacking (your option framed closest to "(2)", but structural rather than a body note): #480 is now rebased onto rocmai-83-common (which itself sits on #477's rocmai-83-persistence), so the merge order 477 → 479 → 480 is enforced by the branch graph itself, not just documented. The architecture.md hunk is now a true incremental diff on top of the already-landed predecessor wording — it can't apply before #477/#479 land because its base branch doesn't exist upstream until they do. Converted back to draft to reflect the dependency; will mark ready once #479 merges and this is rebased onto main.

Everything else in the review (validation logic, import deltas, payload_string back-reference, test count, blast radius, leak scan) is unchanged by the rebase — no code in webhook.rs or the call sites moved.