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

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

specs/unworked_review_issues/2026-06-05_173900_task-066.md

@@ -0,0 +1,65 @@
+# Unworked Review Issues
+
+**Run:** 2026-06-05 17:39:00
+**Task:** TASK-066
+**Total:** 14 (0 critical, 1 major, 13 minor)
+
+## Major
+
+1. [ ] **code-simplifier** | `test/unit/hooks_log_access_alias_slot_test.cpp:178` | code-structure
+   log_access_alias_is_immutable_after_construction duplicates roughly half of the pre-existing user_add_hook_grows_vector_alias_slot_untouched test (lines 91-110). Steps (a) and (b) of the new test—checking that construction wires the slot, that the vector starts at size 0, and that add_hook() grows the vector while leaving the slot set—are already pinned by the older test. The overlap means both tests must be updated if the invariant changes, and a reader cannot tell which test is authoritative for that assertion.
+   *Recommendation:* Remove steps (a) and (b) from log_access_alias_is_immutable_after_construction and retitle it to reflect its unique focus: that remove() leaves the alias slot intact and that direct invocation still reaches the construction-time callable. Alternatively, delete user_add_hook_grows_vector_alias_slot_untouched and let the new test be the single owner of the full add/remove/invoke contract.
+
+## Minor
+
+2. [ ] **architecture-alignment-checker** | `src/hook_handle.cpp:146` | pattern-violation
+   The comment on `erase_slot_for_handler_phase_` for `hook_phase::handler_exception` says 'any_hooks_ remains true while the alias is wired, which is correct -- the alias is the "still has a hook" signal.' This is correct behaviour but the comment conflates the user-vector gate with the alias-slot gate: the any_hooks_[handler_exception] flag is set to true by `install_internal_error_alias` when the alias slot is wired (see webserver_aliases.cpp:118). However, `erase_and_reset` only clears the flag when `hooks_handler_exception_` (the user vector) becomes empty -- it does NOT check whether `handler_exception_alias_` is also empty. If the alias is wired and the last user hook is removed, the flag stays true because the alias holds a live registration. The comment acknowledges this but does not cross-reference `install_internal_error_alias` line 118 where the gate is set for the alias. This is a documentation gap, not a logic error.
+   *Recommendation:* Extend the comment to explicitly reference `install_internal_error_alias` (webserver_aliases.cpp:118) as the place that sets `any_hooks_[handler_exception]` for the alias slot, and clarify that `erase_and_reset` correctly avoids clearing the flag when the alias is still wired.
+
+3. [ ] **code-quality-reviewer** | `src/hook_handle.cpp:480` | code-readability
+   fire_handler_exception (lines 454-519) contains inline error-logging calls that duplicate the log_hook_threw / log_hook_threw_unknown free-function pattern used by the template variants (lines 288-307). The two catch arms at lines 480-488 use .append() instead of the shared helpers.
+   *Recommendation:* This pre-existing inconsistency is not introduced by TASK-066, but the task touched these lines. A follow-up to replace the inline log_dispatch_error strings in fire_handler_exception with calls to log_hook_threw / log_hook_threw_unknown would make the error-logging pattern uniform.
+
+4. [ ] **code-quality-reviewer** | `test/unit/hooks_log_access_alias_slot_test.cpp:178` | code-readability
+   The block comment spanning lines 168-177 correctly explains the rationale for replacing the old test but is longer than the test body it introduces. Per clean-code comments rules, the code (test name + inline step comments) is already self-documenting, and the contextual history (what the old test did and why it was wrong) is better placed in the task/PR description than inline.
+   *Recommendation:* Trim the block comment to one or two sentences that state the contract being tested; move the historical rationale to the commit message or TASK-066.md where it already lives.
+
+5. [ ] **code-quality-reviewer** | `test/unit/hooks_log_access_alias_slot_test.cpp:221` | test-coverage
+   handler_exception_alias_is_immutable_after_construction does not include a direct invocation step (step (d)) comparable to the log_access test at line 211-218. The log_access test verifies the construction-time callable is still reachable after add/remove; the handler_exception mirror stops at vector-size assertions only.
+   *Recommendation:* Add an invocation check analogous to lines 211-218 in the log_access test: build a handler_exception_ctx, call impl->handler_exception_alias_(ctx), and assert the return value is the expected response to confirm the construction-time callable remains wired after user-hook removal.
+
+6. [ ] **code-simplifier** | `test/unit/hooks_log_access_alias_slot_test.cpp:168` | comments
+   The 10-line block comment before the two new tests (lines 168-177) reads as a commit-message changelog explaining why the old test was deleted and what the new tests pin. This is noise in the source file; the test names and inline step comments are already self-documenting. The comment also re-states the DR-012 / §4.10 contract that the @note in hook_handle.hpp now carries.
+   *Recommendation:* Replace the block with a one-line comment that states the invariant being pinned, e.g. '// TASK-066: alias slots are construction-time-only; add_hook() and remove() must not reseat them.'
+
+7. [ ] **code-simplifier** | `test/unit/hooks_log_access_alias_slot_test.cpp:221` | code-structure
+   handler_exception_alias_is_immutable_after_construction is asymmetric with its stated mirror (log_access_alias_is_immutable_after_construction): it omits the direct-invocation verification present in step (d) of the log_access variant. The comment on line 222 says 'Mirror of the log_access pin above', but the pin is incomplete — if the construction-time callable were silently cleared, this test would not catch it.
+   *Recommendation:* Add a step (d) that invokes impl->handler_exception_alias_ directly with a synthetic handler_exception_ctx and asserts that the construction-time lambda was called, matching the pattern in the log_access test.
+
+8. [ ] **housekeeper** | `specs/tasks/M7-v2-cleanup/TASK-085.md:13` | action-item-not-marked-complete
+   TASK-085 action item 3 ('Once TASK-066 ships the runtime setter (or pins the immutable-after-start contract), update this test to exercise the real path') is still unchecked (`[ ]`). TASK-066 both pinned the immutable-after-start contract and replaced the misleading test with `log_access_alias_is_immutable_after_construction` and `handler_exception_alias_is_immutable_after_construction`. TASK-066.md notes this ('TASK-085's action item 3... is satisfied here'), but TASK-085 itself has not been updated to reflect that AI-3 was completed by this task.
+   *Recommendation:* Mark TASK-085 action item 3 as complete (`[x]`) and add a brief note that it was delivered by TASK-066 rather than waiting for TASK-085 to be worked.
+
+9. [ ] **performance-reviewer** | `test/unit/hooks_log_access_alias_slot_test.cpp:242` | memory-allocation
+   In handler_exception_alias_is_immutable_after_construction, the lambda passed to add_hook wraps a std::function with an explicit construction: std::function<httpserver::hook_action(const httpserver::handler_exception_ctx&)>([...]). This causes a heap allocation for the std::function type-erased callable. In a test this is harmless, but the pattern is slightly more verbose and allocates where a direct lambda capture would also allocate — no real impact, just noting the pattern for consistency with the log_access test above it which passes a std::function<void(...)> the same way.
+   *Recommendation:* No action required for a test. In production hot-path code, prefer passing callables through template parameters or using small-buffer-optimised wrappers if std::function allocation is a concern; this test path is not a hot path.
+
+10. [ ] **security-reviewer** | `src/hook_handle.cpp:218` | insecure-design
+   The erase_and_reset lambda in hook_handle::remove() stores false into any_hooks_[handler_exception] when the user-vector becomes empty, without checking whether handler_exception_alias_ is still set. If a user registers a hook via add_hook(handler_exception, ...) and then removes it, any_hooks_[handler_exception] is cleared to false even though the alias slot is still live. This was pre-existing before TASK-066, but the task explicitly adopts the comment at line 142-146 that says 'any_hooks_ remains true while the alias is wired, which is correct' — yet the remove() path can falsify that invariant. In practice the dispatch path in webserver_dispatch.cpp at the server_chain check (line 372-373) directly ORs impl->handler_exception_alias_ into the gate, so the alias is never silently skipped even when any_hooks_ is wrong; the security impact is therefore nil in the current code. However, the invariant mismatch is a latent logic defect that could become a real bypass if a future caller relies solely on has_hooks_for(handler_exception) rather than the direct alias check.
+   *Recommendation:* In the erase_and_reset lambda (hook_handle.cpp line 218), guard the any_hooks_ store with a check on whether the alias slot is still populated: replace 'if (vec.empty())' with 'if (vec.empty() && !impl->handler_exception_alias_)' for the handler_exception phase. The same pattern should apply for log_access_alias_ at the response_sent phase. Alternatively, add a helper any_hooks_for_phase_() that centralises the alias-aware check and is called by both remove() and has_hooks_for().
+
+11. [ ] **spec-alignment-checker** | `specs/architecture/04-components/hooks.md:93` | specification-gap
+   The §4.10 'v1 aliases' table describes not_found_handler as aliasing 'route_resolved (when route is nullopt; runs after any user route_resolved hooks that did not short-circuit)', but the implementation in webserver_aliases.cpp installs only an empty observation stub for route_resolved and the actual 404 synthesis still lives in webserver_impl::not_found_page. The architecture doc's parenthetical 'when route is nullopt' implies functional behaviour at the hook site, which does not match the stub. This is a documentation ambiguity rather than a code defect — TASK-066's own task document explicitly notes the structural deferral — but the arch doc could confuse readers.
+   *Recommendation:* Clarify in the §4.10 not_found_handler row that the hook is an observation stub only; the 404 synthesis at this phase is deferred to a future task (per TASK-066 action item note). A parenthetical such as '(observation-only stub in v2.0; 404 synthesis deferred — see TASK-048)' avoids reader confusion.
+
+12. [ ] **test-quality-reviewer** | `test/unit/hooks_log_access_alias_slot_test.cpp:178` | missing-test
+   The immutability tests cover only two of the five alias slots (log_access -> response_sent and internal_error_handler -> handler_exception). The three before_handler aliases (auth_handler, method_not_allowed_handler, not_found_handler/route_resolved) are installed via add_hook().detach() into the user vector rather than a dedicated slot, but TASK-066 explicitly documents all five as immutable-after-construction. A reader of this test file has no pinned contract for the other three. The omission is not critical because those three aliases write into the user vector (not a separate slot) so the immutability property is structurally different, but a comment or separate minimal test noting 'these three use vector entries, not slots' would complete the TASK-066 contract narrative.
+   *Recommendation:* Add a brief comment (or a single baseline test) noting that auth_handler, method_not_allowed_handler, and not_found_handler aliases install into the user vector via detach() and are therefore not re-settable via any public API either. This avoids future confusion about why only two slots are pinned here.
+
+13. [ ] **test-quality-reviewer** | `test/unit/hooks_log_access_alias_slot_test.cpp:178` | multiple-concerns
+   log_access_alias_is_immutable_after_construction combines four distinct assertions in one test body: (a) slot set at construction, (b) add_hook grows user vector and slot remains, (c) remove() shrinks vector and slot remains, (d) direct invocation reaches original callable. The test name names only the top-level invariant; sub-assertions (b), (c), (d) are individually distinct behaviors. The comments labelled (a)-(d) partially mitigate this, but a test failure in sub-assertion (c) or (d) requires reading the body to find the failing step.
+   *Recommendation:* Consider splitting into two or three focused tests: one for the construction-time state, one for add_hook/remove not affecting the alias slot, and one for the callable remaining the original after add_hook/remove. The current structure is not wrong, but splitting would make failures self-diagnosing.
+
+14. [ ] **test-quality-reviewer** | `test/unit/hooks_log_access_alias_slot_test.cpp:221` | missing-test
+   handler_exception_alias_is_immutable_after_construction does not include a direct-invocation sub-check (equivalent to step (d) in the log_access test). The log_access test verifies that calling impl->log_access_alias_(ctx) after add_hook/remove still reaches the original callable. The handler_exception test omits this step, leaving a gap: it proves the slot stays set (bool check) but does not prove the callable identity is preserved.
+   *Recommendation:* Add a step (d) analogue to handler_exception_alias_is_immutable_after_construction: capture a counter in the internal_error_handler lambda, invoke impl->handler_exception_alias_(ctx), and assert the counter incremented.

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);
 
     // ----------------------------------------------------------------