TASK-066: strike forward-debt comments; pin alias slots as construction-time-only

Option (b) of the task spec: rather than add a runtime alias setter (no
demand signal, two-mechanisms-behind-one-façade asymmetry, DR-012 already
frames aliases as construction-time sugar), remove the speculative
"if a future task adds a runtime setter..." comments and pin the
immutable-after-construction contract with tests and documentation.

- src/hook_handle.cpp: trim three forward-debt comment blocks (in
  erase_slot_for_handler_phase_, fire_handler_exception tail,
  fire_response_sent tail) to one-line "immutable after start()
  (DR-012 §4.10)" rationale.
- src/detail/webserver_aliases.cpp: trim the install_internal_error_alias
  call-site comment to the immutable-after-construction one-liner.
- src/httpserver/detail/webserver_impl.hpp: trim the lifetime-contract
  comments on handler_exception_alias_ and log_access_alias_ to drop the
  speculative "future runtime setter" paragraphs; keep the
  single-writer-at-construction rationale.
- src/httpserver/hook_handle.hpp: add an "Alias slots are
  construction-time-only" docstring paragraph naming the five v1 aliases
  and pointing users to add_hook() for runtime extension.
- specs/architecture/04-components/hooks.md: add an "Alias mutability"
  paragraph to §4.10 explicitly stating that v1 aliases are
  construction-time-only and that the hook bus is the runtime
  extension surface (per DR-012).
- test/unit/hooks_log_access_alias_slot_test.cpp: replace the
  misleading log_access_second_registration_replaces_first test (the
  smell TASK-085 finding 3 flagged) with two real contract pins:
  log_access_alias_is_immutable_after_construction and
  handler_exception_alias_is_immutable_after_construction. Both verify
  that user add_hook() at the relevant phase grows the user vector but
  never displaces the alias slot, and that hook_handle::remove()
  leaves the alias slot intact.

Acceptance:
- grep -nE 'if a future task adds a runtime setter for the alias slots' src/
  returns no matches.
- Broader audit grep
  'future task adds a runtime|future runtime setter|runtime re-registration path'
  src/ also returns no matches.
- hooks_log_access_alias_slot now: 7 tests, 29 checks, 0 failures.
- All hook-related tests pass; cpplint clean on changed files.
- specs/tasks/M7-v2-cleanup/TASK-066.md updated to Done with resolution
  notes and TASK-085 handoff.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Author: etr

specs/architecture/04-components/hooks.md

@@ -97,6 +97,8 @@ hook_handle http_resource::add_hook(hook_phase, std::function<...>);
 | `log_error(fn)` | *not* a hook alias — it is an MHD-level callback for backend errors, distinct from the request lifecycle |
 | `file_cleanup_callback(fn)` | *not* a hook alias — file-upload cleanup is a separate post-upload concern, not a lifecycle phase |
 
+**Alias mutability.** All v1 alias setters are **construction-time-only**. Their backing storage is wired during `create_webserver` → `webserver` construction and not mutated afterward. Two aliases (`log_access`, `internal_error_handler`) occupy dedicated single-slot members on `webserver_impl` (`log_access_alias_`, `handler_exception_alias_`); the other three (`auth_handler`, `method_not_allowed_handler`, `not_found_handler`) are seated into the regular per-phase hook vectors via `add_hook(...).detach()` at construction. In neither case is the slot mutable after `webserver::start()`. Users who need runtime registration or replacement of an extension point should use the hook bus directly: `webserver::add_hook(phase, callable)` returns a `hook_handle` that supports `remove()`. This is a deliberate v2.0 design choice (DR-012 / TASK-066): the hook bus IS the runtime extension surface; aliases are documented construction-time sugar. The semantics are pinned by `log_access_alias_is_immutable_after_construction` and `handler_exception_alias_is_immutable_after_construction` in `test/unit/hooks_log_access_alias_slot_test.cpp`.
+
 **Related requirements:** PRD-HOOK-REQ-001..009.
 **Related decisions:** DR-012, §5.6.
 

specs/tasks/M7-v2-cleanup/TASK-066.md

@@ -8,10 +8,10 @@
 Five separate "if a future task adds a runtime setter for the alias slots, this MUST take the lock shared" comments cluster across `src/hook_handle.cpp:142, 449, 497` and `src/httpserver/detail/webserver_impl.hpp:336, 360`. All point at the same missing capability: runtime re-registration of internal alias slots (`log_access`, `not_found_handler`, `method_not_allowed_handler`, `internal_error_handler`, `auth_handler`). Land that capability or strike the speculation.
 
 **Action Items:**
-- [ ] Decide: (a) add a `webserver::set_alias(hook_phase, alias_id, hook_action_fn)` runtime setter that re-points the relevant alias slot under the existing `shared_mutex`, with replacement semantics matching the construction-time alias (last-position, single-slot), or (b) remove the forward-debt comments and pin "aliases are immutable after `webserver::start()`" in the docs.
-- [ ] If (a): implement the setter, document it in the architecture doc (§4.10), add a hook-bus integration test covering concurrent alias replacement under load, and remove all five forward-debt comments.
-- [ ] If (b): remove the five comments and add a documentation note (in `hook_handle.hpp` and the §4.10 architecture page) that alias slots are construction-time-only.
-- [ ] Update `test/unit/hooks_log_access_alias_slot_test.cpp` to reflect the chosen semantics — coordinated with TASK-085.
+- [x] Decide: (a) add a `webserver::set_alias(hook_phase, alias_id, hook_action_fn)` runtime setter that re-points the relevant alias slot under the existing `shared_mutex`, with replacement semantics matching the construction-time alias (last-position, single-slot), or (b) remove the forward-debt comments and pin "aliases are immutable after `webserver::start()`" in the docs. **→ Option (b) selected. Rationale: zero demand signal (no examples, no PRD, no test reaches for runtime replacement); option (a) would hide two different mechanisms behind one façade (`log_access`/`internal_error_handler` are single-slot members; `auth_handler`/`not_found_handler`/`method_not_allowed_handler` are detached vector entries); DR-012 already framed aliases as "documented sugar that internally registers a hook" with no "open follow-ups" entry for runtime replacement; the public runtime extension surface IS `add_hook()` + `hook_handle`. See `.groundwork-plans/TASK-066-plan.md` for the full plan.**
+- [x] If (a): ~~implement the setter, document it in the architecture doc (§4.10), add a hook-bus integration test covering concurrent alias replacement under load, and remove all five forward-debt comments.~~ Not selected.
+- [x] If (b): remove the five comments and add a documentation note (in `hook_handle.hpp` and the §4.10 architecture page) that alias slots are construction-time-only. **Done.** Removed seven comment blocks (the spec's five plus two more found in `webserver_aliases.cpp` and the duplicated `fire_handler_exception` tail). Added the docstring paragraph to `hook_handle.hpp` and the "Alias mutability" paragraph to §4.10.
+- [x] Update `test/unit/hooks_log_access_alias_slot_test.cpp` to reflect the chosen semantics — coordinated with TASK-085. **Done.** Replaced the misleading `log_access_second_registration_replaces_first` test with two real contract pins: `log_access_alias_is_immutable_after_construction` and `handler_exception_alias_is_immutable_after_construction`. TASK-085's action item 3 ("update this test once TASK-066 ships") is satisfied here.
 
 **Dependencies:**
 - Blocked by: TASK-045 (hook bus skeleton, Done)
@@ -27,4 +27,4 @@ Five separate "if a future task adds a runtime setter for the alias slots, this
 **Related Requirements:** PRD-HOOK-REQ-009 (v1 setters documented as aliases)
 **Related Decisions:** DR-012
 
-**Status:** Backlog
+**Status:** Done

src/detail/webserver_aliases.cpp

@@ -342,14 +342,10 @@ void webserver::install_default_alias_hooks_() {
     // calling it a second time would observably invoke the user code
     // twice for one logical exception). See webserver_dispatch.cpp.
     //
-    // Re-registration semantics (finding #35 / spec-alignment): the
-    // task spec says "re-registration replaces the existing alias hook".
-    // At v2.0 there is no runtime re-registration path -- the slot is
-    // written exactly once at construction (write-once-at-construction
-    // contract). If a future task adds a runtime setter, it must
-    // overwrite handler_exception_alias_ under hook_table_mutex_
-    // exclusive and update any_hooks_ accordingly; "replace" semantics
-    // are then enforced by the plain std::function overwrite.
+    // The alias slot is written exactly once here and is immutable
+    // thereafter (DR-012 / §4.10). Runtime extension of the
+    // handler_exception phase is via add_hook(); the alias slot is not
+    // user-mutable post-construction.
     install_internal_error_alias(impl_.get(), internal_error_handler);
 
     // ----------------------------------------------------------------

src/hook_handle.cpp

@@ -139,13 +139,11 @@ void erase_slot_for_handler_phase_(detail::webserver_impl* impl,
     case hook_phase::before_handler:
         erase_and_reset(impl->hooks_before_handler_); break;
     case hook_phase::handler_exception:
-        // Finding #6 (forward-looking): if a future task adds a runtime
-        // re-registration path for handler_exception_alias_, remove()
-        // will also need a path to clear that slot and re-evaluate
-        // any_hooks_. Currently any_hooks_ remains true while the alias
-        // is wired; removing only a user-vector entry does not clear it
-        // if the alias is still set. Add that handling alongside the
-        // runtime setter.
+        // The alias slot (handler_exception_alias_) is immutable after
+        // construction (DR-012 / §4.10), so remove() only touches the
+        // user vector here; any_hooks_ remains true while the alias is
+        // wired, which is correct -- the alias is the "still has a
+        // hook" signal.
         erase_and_reset(impl->hooks_handler_exception_); break;
     case hook_phase::after_handler:
         erase_and_reset(impl->hooks_after_handler_); break;
@@ -493,15 +491,9 @@ detail::webserver_impl::fire_handler_exception(
         log_dispatch_error(
             "fire_handler_exception: snapshot copy failed");
     }
-    // Tail: invoke the alias slot, if any. Read without synchronisation;
-    // the slot is single-writer-at-construction (see webserver_impl.hpp).
-    //
-    // Finding #33 (security-reviewer): if a future task adds a runtime
-    // setter for handler_exception_alias_, this read MUST be upgraded to
-    // take hook_table_mutex_ shared and the writer MUST take it exclusive.
-    // The synchronisation contract lives in webserver_impl.hpp at the
-    // handler_exception_alias_ field declaration -- keep both sites in
-    // sync when adding any runtime re-registration path.
+    // Tail: invoke the alias slot, if any. Read without synchronisation:
+    // the slot is single-writer-at-construction, immutable after
+    // webserver::start() (DR-012 / §4.10).
     //
     // Throw containment: a throwing alias is logged with the legacy
     // "internal_error_handler threw" prefix so the DR-009 §5.2 point 4
@@ -545,8 +537,9 @@ detail::webserver_impl::fire_after_handler(
 void detail::webserver_impl::fire_response_sent(
     const ::httpserver::response_sent_ctx& ctx) noexcept {
     fire_hooks_for_phase(this, hooks_response_sent_, ctx, "response_sent");
-    // Tail: invoke the alias slot, if any. Read without synchronisation;
-    // the slot is single-writer-at-construction (see webserver_impl.hpp).
+    // Tail: invoke the alias slot, if any. Read without synchronisation:
+    // the slot is single-writer-at-construction, immutable after
+    // webserver::start() (DR-012 / §4.10).
     if (log_access_alias_) {
         try {
             log_access_alias_(ctx);

src/httpserver/detail/webserver_impl.hpp

@@ -347,12 +347,8 @@ class webserver_impl {
     // at webserver construction, before start() is called -- the daemon is
     // not yet running, so no synchronisation is required for the write.
     // Read on the dispatch hot path from fire_handler_exception with no
-    // lock (single-writer-before-readers contract, same as
-    // parent->internal_error_handler). If a future task adds a runtime
-    // setter the writer MUST take hook_table_mutex_ exclusively and the
-    // reader MUST take it shared -- same pattern as the per-phase vectors.
-    // (Finding #14: full rationale lives here; fire_handler_exception in
-    // hook_handle.cpp has a cross-reference comment per finding #33.)
+    // lock. Slot is immutable after construction (DR-012 / §4.10); the
+    // reader path requires no synchronisation.
     std::function<::httpserver::hook_action(
             const ::httpserver::handler_exception_ctx&)>
         handler_exception_alias_;
@@ -370,11 +366,8 @@ class webserver_impl {
     // The slot fires AFTER user-added response_sent hooks so user hooks
     // observe the response before the legacy access logger formats it.
     //
-    // A future runtime setter that allows re-registration MUST take
-    // hook_table_mutex_ exclusively for the write, and the reader in
-    // fire_response_sent MUST take it shared. At v2.0 there is no runtime
-    // setter -- the slot is written exactly once at webserver construction
-    // before start() is called.
+    // Slot is immutable after construction (DR-012 / §4.10); the reader
+    // path requires no synchronisation.
     std::function<void(const ::httpserver::response_sent_ctx&)>
         log_access_alias_;
     std::vector<phase_entry<void(const ::httpserver::request_completed_ctx&)>>

src/httpserver/hook_handle.hpp

@@ -82,6 +82,15 @@ class http_resource;
  * `weak_ptr<resource_hook_table>` instead of a raw pointer; their
  * `remove()` is safe even after the resource is destroyed (the
  * weak_ptr expires and the call becomes a no-op).
+ *
+ * @note **Alias slots are construction-time-only.** The v1-derived
+ * setters on `create_webserver` (`log_access`,
+ * `internal_error_handler`, `not_found_handler`,
+ * `method_not_allowed_handler`, `auth_handler`) wire their slot
+ * exactly once during webserver construction and are immutable
+ * thereafter. To add or remove an observation/intervention point at
+ * runtime, use `webserver::add_hook(phase, ...)` and the returned
+ * `hook_handle`. See DR-012 / §4.10.
  */
 class hook_handle {
  public:

test/unit/hooks_log_access_alias_slot_test.cpp

@@ -39,6 +39,7 @@
 #include <algorithm>
 #include <functional>
 #include <string>
+#include <string_view>
 
 #include "./httpserver.hpp"
 #include "httpserver/create_test_request.hpp"
@@ -164,45 +165,97 @@ LT_BEGIN_AUTO_TEST(hooks_log_access_alias_slot_suite,
     LT_CHECK(captured.find("GET") != std::string::npos);
 LT_END_AUTO_TEST(alias_sanitizes_control_chars_in_method)
 
-// Re-registration: calling log_access a second time on the builder
-// replaces the previous callable. At v2.0 the only write point is
-// webserver construction (write-once-at-construction contract); runtime
-// re-registration via a setter is deferred to a future task.
-// This test pins the replace semantics at construction time.
+// TASK-066: alias slots are construction-time-only. The previous test on
+// this site (`log_access_second_registration_replaces_first`) simulated
+// re-registration by constructing two webservers and asserting they did
+// not pollute each other -- but that was not a runtime "replace"
+// semantics pin, because no runtime replacement path exists or is
+// planned (DR-012, §4.10). The two tests below replace that simulated
+// scenario with the real contract: alias slots are written exactly once
+// at webserver construction and are immutable thereafter; the public
+// runtime extension surface for both phases is add_hook(), which lives
+// in the per-phase user vector and never touches the alias slot.
 LT_BEGIN_AUTO_TEST(hooks_log_access_alias_slot_suite,
-                   log_access_second_registration_replaces_first)
-    int first_calls = 0;
-    int second_calls = 0;
-    // Simulate re-registration by creating two builders and checking that
-    // only the callable set on the webserver used for construction is stored.
-    webserver ws1{create_webserver(8244)
-        .log_access([&first_calls](const std::string&) { ++first_calls; })};
-    webserver ws2{create_webserver(8245)
-        .log_access([&second_calls](const std::string&) { ++second_calls; })};
-
-    // Each webserver holds its own callable; neither pollutes the other.
-    auto* impl1 = impl_of(ws1);
-    auto* impl2 = impl_of(ws2);
-    LT_CHECK(static_cast<bool>(impl1->log_access_alias_));
-    LT_CHECK(static_cast<bool>(impl2->log_access_alias_));
-
-    // Invoke each alias independently.
-    http_request req1 =
+                   log_access_alias_is_immutable_after_construction)
+    int direct_calls = 0;
+    int user_hook_calls = 0;
+    webserver ws{create_webserver(8244)
+        .log_access([&direct_calls](const std::string&) { ++direct_calls; })};
+    auto* impl = impl_of(ws);
+
+    // (a) Construction wires the slot exactly once.
+    LT_CHECK(static_cast<bool>(impl->log_access_alias_));
+    LT_CHECK_EQ(impl->hooks_response_sent_.size(),
+                static_cast<std::size_t>(0));
+
+    // (b) Adding a user response_sent hook grows the vector but leaves
+    // the alias slot untouched -- the public way to add observation at
+    // response_sent post-construction is add_hook(), which routes into
+    // the user vector and never reseats the alias slot.
+    auto h = ws.add_hook(hook_phase::response_sent,
+        std::function<void(const response_sent_ctx&)>(
+            [&user_hook_calls](const response_sent_ctx&) {
+                ++user_hook_calls;
+            }));
+    LT_CHECK(static_cast<bool>(impl->log_access_alias_));
+    LT_CHECK_EQ(impl->hooks_response_sent_.size(),
+                static_cast<std::size_t>(1));
+
+    // (c) Removing the user hook leaves the alias slot untouched -- the
+    // slot is not under user control via the hook bus.
+    h.remove();
+    LT_CHECK(static_cast<bool>(impl->log_access_alias_));
+    LT_CHECK_EQ(impl->hooks_response_sent_.size(),
+                static_cast<std::size_t>(0));
+
+    // (d) Direct invocation still reaches the construction-time callable.
+    http_request req =
         create_test_request().path("/a").method("GET").build();
-    http_request req2 =
-        create_test_request().path("/b").method("GET").build();
-    response_sent_ctx ctx1{};
-    ctx1.request = &req1;
-    response_sent_ctx ctx2{};
-    ctx2.request = &req2;
-
-    impl1->log_access_alias_(ctx1);
-    impl2->log_access_alias_(ctx2);
-
-    // Only the callable stored on each respective webserver was invoked.
-    LT_CHECK_EQ(first_calls, 1);
-    LT_CHECK_EQ(second_calls, 1);
-LT_END_AUTO_TEST(log_access_second_registration_replaces_first)
+    response_sent_ctx ctx{};
+    ctx.request = &req;
+    impl->log_access_alias_(ctx);
+    LT_CHECK_EQ(direct_calls, 1);
+    LT_CHECK_EQ(user_hook_calls, 0);
+LT_END_AUTO_TEST(log_access_alias_is_immutable_after_construction)
+
+LT_BEGIN_AUTO_TEST(hooks_log_access_alias_slot_suite,
+                   handler_exception_alias_is_immutable_after_construction)
+    // Mirror of the log_access pin above for handler_exception_alias_.
+    // Construction wires the slot; user add_hook() at handler_exception
+    // grows hooks_handler_exception_ but does not displace or clear the
+    // alias slot. Removing the user hook leaves the alias slot intact.
+    webserver ws{create_webserver(8244)
+        .internal_error_handler(
+            [](const httpserver::http_request&, std::string_view)
+                -> httpserver::http_response {
+                return httpserver::http_response::string("oops");
+            })};
+    auto* impl = impl_of(ws);
+
+    // (a) Construction wires the alias slot exactly once.
+    LT_CHECK(static_cast<bool>(impl->handler_exception_alias_));
+    LT_CHECK_EQ(impl->hooks_handler_exception_.size(),
+                static_cast<std::size_t>(0));
+
+    // (b) User add_hook(handler_exception, ...) grows the vector; alias
+    // slot remains set independently.
+    auto h = ws.add_hook(hook_phase::handler_exception,
+        std::function<httpserver::hook_action(
+                const httpserver::handler_exception_ctx&)>(
+            [](const httpserver::handler_exception_ctx&) {
+                return httpserver::hook_action::pass();
+            }));
+    LT_CHECK(static_cast<bool>(impl->handler_exception_alias_));
+    LT_CHECK_EQ(impl->hooks_handler_exception_.size(),
+                static_cast<std::size_t>(1));
+
+    // (c) Removing the user hook does not clear the alias slot -- the
+    // slot is not under user control via the hook bus.
+    h.remove();
+    LT_CHECK(static_cast<bool>(impl->handler_exception_alias_));
+    LT_CHECK_EQ(impl->hooks_handler_exception_.size(),
+                static_cast<std::size_t>(0));
+LT_END_AUTO_TEST(handler_exception_alias_is_immutable_after_construction)
 
 LT_BEGIN_AUTO_TEST_ENV()
     AUTORUN_TESTS()