TASK-066: persist 14 unworked review findings from final review cycle

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

Author: etr

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.