Merge TASK-085: residual test-smell sweep

Fail-closed auth_handler guard (CWE-703 fix) + parallel-runner-safe
smartptr test refactor. Items 1 & 3 satisfied by TASK-061/TASK-066.
Includes 18 persisted unworked review findings. Marks TASK-085 Done.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Rkuh4aSmrD8m2f2vYqakb6

Author: etr

specs/architecture/04-components/hooks.md

@@ -99,6 +99,8 @@ hook_handle http_resource::add_hook(hook_phase, std::function<...>);
 
 **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`.
 
+**`auth_handler` is fail-closed on exception.** The generic short-circuit firing path (`fire_short_circuit_hooks_for_phase`) catches an exception thrown by a `before_handler`/`request_received`/`body_chunk` hook, logs it, and treats it as `pass()` — i.e. the chain continues. The `auth_handler` alias is the one deliberate exception to that policy: because the auth seat is a security boundary, letting a throwing auth callable fall through to the resource would be a fail-open on authentication (CWE-703). The alias therefore wraps its own callable invocation in a local fail-closed guard: an exception is caught, logged via `log_dispatch_error`, and short-circuits the request with a **500** instead of `pass()`. This matches the §5.2 / DR-009 contract that "an exception thrown by a hook is routed through the same path as a throwing resource handler" (which terminates in a 500). Pinned by `throwing_auth_handler_fails_closed_with_500` in `test/unit/auth_handler_optional_signature_test.cpp` (TASK-085). A user `auth_handler` that wants a custom non-500 rejection on its own error path must catch internally and return an engaged `std::optional<http_response>`.
+
 **Related requirements:** PRD-HOOK-REQ-001..009.
 **Related decisions:** DR-012, §5.6.
 

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

@@ -8,10 +8,10 @@
 A cluster of small, independent test smells that the audit groups under "Other test smells". Each is too small to be its own task but together they erode confidence in the test contract.
 
 **Action Items:**
-- [ ] `test/Makefile.am:67-74`: orphaned comment claiming `header_hygiene` is in `XFAIL_TESTS`; lines 532-535 admit it was removed when TASK-020 landed. Update the comment. (Mechanical — overlaps with TASK-061; do whichever lands first and drop from the other.)
-- [ ] `test/unit/auth_handler_optional_signature_test.cpp:176-192`: `throwing_handler_is_swallowed_and_request_passes` ratifies a questionable swallow-and-pass behaviour as the pin. Decide: is this the intended contract? If yes, update the test name to make the contract explicit (e.g., `auth_handler_exception_is_logged_and_request_proceeds_without_auth`) and document in `specs/architecture/` why. If no, change the behaviour to fail-closed (reject the request with 500 or equivalent) and update the test accordingly.
-- [ ] `test/unit/hooks_log_access_alias_slot_test.cpp:166-205`: "second registration replaces first" only simulates re-registration via two webservers. Once TASK-066 ships the runtime setter (or pins the immutable-after-start contract), update this test to exercise the real path.
-- [ ] `test/unit/webserver_register_smartptr_test.cpp:60-64, 148-156`: documented parallel-runner fragility. Diagnose root cause (most likely a shared static or filesystem path collision) and fix, or convert the affected sub-tests to serial-only with an explicit littletest annotation.
+- [x] `test/Makefile.am:67-74`: orphaned comment claiming `header_hygiene` is in `XFAIL_TESTS`; lines 532-535 admit it was removed when TASK-020 landed. Update the comment. (Mechanical — overlaps with TASK-061; do whichever lands first and drop from the other.) — **Satisfied by TASK-061** (status Done; lines 67-74 are now the cookie-test block, the accurate historical XFAIL note lives at the `header_hygiene_SOURCES` block ~103-115). No orphaned comment remains; dropped per the overlap clause.
+- [x] `test/unit/auth_handler_optional_signature_test.cpp:176-192`: `throwing_handler_is_swallowed_and_request_passes` ratifies a questionable swallow-and-pass behaviour as the pin. Decide: is this the intended contract? If yes, update the test name to make the contract explicit (e.g., `auth_handler_exception_is_logged_and_request_proceeds_without_auth`) and document in `specs/architecture/` why. If no, change the behaviour to fail-closed (reject the request with 500 or equivalent) and update the test accordingly. — **DECIDED: fail-closed.** A throwing auth handler now returns 500 (auth-local guard in `src/detail/webserver_aliases.cpp`); test renamed to `throwing_auth_handler_fails_closed_with_500`; contract documented in `specs/architecture/04-components/hooks.md` ("`auth_handler` is fail-closed on exception"), removing the spec/impl contradiction with §5.2/DR-009.
+- [x] `test/unit/hooks_log_access_alias_slot_test.cpp:166-205`: "second registration replaces first" only simulates re-registration via two webservers. Once TASK-066 ships the runtime setter (or pins the immutable-after-start contract), update this test to exercise the real path. — **Satisfied by TASK-066** (status Done). TASK-066 chose option (b): aliases are immutable-after-construction, NO runtime setter. The misleading test was replaced by `log_access_alias_is_immutable_after_construction` + `handler_exception_alias_is_immutable_after_construction`, which exercise the real (immutability) contract.
+- [x] `test/unit/webserver_register_smartptr_test.cpp:60-64, 148-156`: documented parallel-runner fragility. Diagnose root cause (most likely a shared static or filesystem path collision) and fix, or convert the affected sub-tests to serial-only with an explicit littletest annotation. — **Fixed (root cause).** Replaced fixed ports with `create_webserver(0)` + `ws.get_bound_port()` (no cross-test port collision) and replaced the shared static `counted_resource::dtor_count` with a per-test local `std::atomic<int>` passed by pointer (no cross-test contamination). Fragility comments and the `set_up()` reset removed.
 
 **Dependencies:**
 - Blocked by: TASK-066 (for the alias-slot test work)
@@ -26,4 +26,4 @@ A cluster of small, independent test smells that the audit groups under "Other t
 **Related Requirements:** PRD §2 test reliability NFR
 **Related Decisions:** None new
 
-**Status:** Backlog
+**Status:** Done

specs/tasks/M7-v2-cleanup/_index.md

@@ -49,7 +49,7 @@ TASK-093).
 | TASK-082 | Tighten static-size bounds in `http_resource_test` and `webserver_pimpl_test` | MED | S | Done |
 | TASK-083 | Wire real CI gates into benchmarks | MED | M | Done |
 | TASK-084 | Re-measure libstdc++/Linux v1 baseline for `get_headers` ns/call | MED | S | Done |
-| TASK-085 | Residual test-smell sweep | MED | S | Backlog |
+| TASK-085 | Residual test-smell sweep | MED | S | Done |
 | TASK-086 | Execute file-size split roadmap (FILE_LOC_MAX 750 → 500) | HIGH | L | Backlog |
 | TASK-087 | Restore msan CI lane | HIGH | M | Backlog |
 | TASK-088 | Re-enable helgrind / drd / sgcheck in valgrind CI lane | HIGH | M | Backlog |