TASK-085: persist 18 unworked review findings

18 minor findings (0 critical, 0 major) from the post-implementation
review pass that were triaged but not actioned in this task. Persisted
for backlog visibility, mirroring the TASK-084 precedent.

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

Author: etr

specs/unworked_review_issues/2026-06-23_100730_task-085.md

@@ -0,0 +1,79 @@
+# Unworked Review Issues
+
+**Run:** 2026-06-23 10:07:30
+**Task:** TASK-085
+**Total:** 18 (0 critical, 0 major, 18 minor)
+
+## Minor
+
+1. [ ] **architecture-alignment-checker** | `specs/architecture/04-components/hooks.md:102` | pattern-violation
+   The new auth_handler fail-closed paragraph states the exception is caught and short-circuits with a 500 'matching the §5.2 / DR-009 contract that an exception thrown by a hook is routed through the same path as a throwing resource handler.' However §5.2 and DR-009 define the propagation path for resource handler exceptions (caught → log_error → internal_error_handler → 500). The auth_handler guard bypasses the internal_error_handler alias entirely and produces a bare 500 directly via hook_action::respond_with. The outcome (500) is the same but the propagation path is subtly different: internal_error_handler is NOT invoked, log_dispatch_error is used rather than log_error. The architectural documentation does not call out this distinction, leaving a minor documentation gap for operators who configure internal_error_handler expecting it to intercept all 500s.
+   *Recommendation:* Clarify in hooks.md that the auth fail-closed 500 bypasses internal_error_handler (it short-circuits at the before_handler phase before dispatch, so the handler_exception chain never fires). A one-sentence note like 'Because the short-circuit occurs before resource dispatch, the handler_exception chain and the internal_error_handler alias are not invoked on this path' would prevent operator confusion. No code change required.
+
+2. [ ] **code-quality-reviewer** | `src/detail/webserver_aliases.cpp:246` | readability
+   The two catch blocks (std::exception and catch-all) share almost identical structure and produce the same outcome (log + return 500). The small code duplication is acceptable but the message strings use slightly different phrasing ('auth_handler threw: <what>; failing closed with 500' vs 'auth_handler threw unknown exception; failing closed with 500').
+   *Recommendation:* Minor nit only; the duplication is intentional to avoid an extra helper function. No change required, but a brief inline comment noting why the blocks aren't merged (cannot call e.what() in the catch-all) would remove any reader confusion.
+
+3. [ ] **code-quality-reviewer** | `test/unit/auth_handler_optional_signature_test.cpp:195` | test-coverage
+   The negative assertion `LT_CHECK(fr.body != std::string("SUCCESS"))` is weaker than necessary. It checks only that the body is not the success string but does not verify the body is actually empty (or some specific error body), so a broken implementation that returns 500 with body "SUCCESS" would still pass.
+   *Recommendation:* Consider replacing with `LT_CHECK_EQ(fr.body, std::string(""))` or verifying the body is empty/a specific error string, to make the contract fully precise.
+
+4. [ ] **code-quality-reviewer** | `test/unit/webserver_register_smartptr_test.cpp:156` | readability
+   The empty `set_up()` body is kept explicitly. Leaving an empty `set_up()` in the suite is not harmful but adds a small noise line; the tear_down() companion is similarly empty.
+   *Recommendation:* If the test framework allows omitting set_up/tear_down when they have no body, removing both would be a minor cleanliness win. If the framework requires them, leave as-is — this is purely cosmetic.
+
+5. [ ] **code-quality-reviewer** | `test/unit/webserver_register_smartptr_test.cpp:58` | readability
+   The block comment at lines 58-67 accurately explains the parallel-runner safety guarantees but is longer than typical file-level comments in this codebase. The content is correct and useful, so this is purely a length preference.
+   *Recommendation:* No action required. The comment is well-intentioned and its length matches the detail needed to explain why no fixed port or shared static remains.
+
+6. [ ] **code-simplifier** | `src/detail/webserver_aliases.cpp:244` | code-structure
+   The two catch clauses (std::exception and catch-all) share identical structure: log a message then return the same 500 response. The only difference is the message string, so the return statement is duplicated. This is a small violation of the DRY principle inside a single try/catch block.
+   *Recommendation:* Extract the 500 return to after the try/catch, using a flag or a local lambda to collapse the two branches: bool auth_threw = false; try { rejection = ...; } catch (const std::exception& e) { impl_ptr->log_dispatch_error(...); auth_threw = true; } catch (...) { impl_ptr->log_dispatch_error(...); auth_threw = true; } if (auth_threw) return hook_action::respond_with(http_response::empty().with_status(500)); — eliminates the duplicated return statement. Alternatively, wrap both catches in a shared helper that takes the message string.
+
+7. [ ] **code-simplifier** | `test/unit/auth_handler_optional_signature_test.cpp:195` | naming
+   LT_CHECK(fr.body != std::string("SUCCESS")) is a weaker assertion than necessary and reads ambiguously. Any non-SUCCESS body — including an empty body, a 500 error page, or any other string — satisfies it. The test intent is to verify that the resource was NOT reached; checking that the body is empty (or that it equals whatever the server's 500 body is) would be more precise and self-documenting.
+   *Recommendation:* If the server returns an empty body on 500, use LT_CHECK_EQ(fr.body, std::string("")) or LT_CHECK(fr.body.empty()); if it returns a known error string, assert that exactly. This pins the contract more tightly and makes the intent clearer than a negation check.
+
+8. [ ] **code-simplifier** | `test/unit/webserver_register_smartptr_test.cpp:153` | code-structure
+   The suite now has an empty set_up() body (just a comment explaining why it is empty). An empty set_up() that exists only as boilerplate — with no logic and no likelihood of future use — adds noise without value. The comment explaining the absence of shared state is useful, but it could live as a block comment above the suite or inside the relevant tests.
+   *Recommendation:* If the test framework requires set_up() / tear_down() to be defined even when empty, keep them but remove the comment from set_up() and place the explanation above the LT_BEGIN_SUITE block where it is more visible. If the framework allows omitting empty hooks, remove set_up() entirely and let tear_down() stand alone. This is YAGNI: the empty body declares an intention to do something but does nothing.
+
+9. [ ] **housekeeper** | `specs/tasks/M7-v2-cleanup/_index.md:33` | documentation-stale
+   TASK-066 is listed as 'Backlog' in the milestone index but its own task file (specs/tasks/M7-v2-cleanup/TASK-066.md) shows 'Status: Done'. The TASK-085 action item 3 resolution note correctly cites TASK-066 as Done, which is accurate per TASK-066.md — but the milestone index was not updated when TASK-066 landed. This inconsistency predates TASK-085 and was not introduced by this commit; however, since TASK-085 explicitly references TASK-066's Done status, the gap is visible in this review.
+   *Recommendation:* Update the TASK-066 row in specs/tasks/M7-v2-cleanup/_index.md from 'Backlog' to 'Done' to match the status in TASK-066.md.
+
+10. [ ] **performance-reviewer** | `src/detail/webserver_aliases.cpp:245` | missing-caching
+   In the exception catch branch, a std::string is heap-allocated to concatenate the log message ('auth_handler threw: ' + e.what() + '; failing closed with 500'). This only runs on the exception path, which is not a hot path by design (exceptions should be rare). There is no concern for the normal request flow.
+   *Recommendation:* No change needed. The string allocation is on the exceptional (error) path only. If log volume from misbehaving auth callables ever becomes a concern, a fixed-size buffer or fmt::format_to could be used, but this is premature optimization for an error path.
+
+11. [ ] **security-reviewer** | `src/detail/webserver_aliases.cpp:209` | A01: Broken Access Control / CWE-288 Authentication Bypass
+   The auth_handler alias fires only for route-hit requests (found==true path in finalize_answer). Requests to unregistered paths receive 404 without invoking the auth callable. This is documented in the install_auth_alias_ comment (lines 182-195) and is the correct semantic for resource-level auth, but it creates an auth oracle: a client can enumerate registered routes by observing 401 (auth blocked) vs 404 (no route). This is a pre-existing design decision (noted as 'Design note (security-reviewer-iter1-7)' in the comment, suggesting a prior review cycle already flagged it), not introduced by this commit.
+   *Recommendation:* No code change required (the design note in the comment already documents this and the mitigation strategy — add a catch-all route or auth in a not_found_handler). The comment coverage is adequate.
+
+12. [ ] **security-reviewer** | `src/detail/webserver_aliases.cpp:245` | A09: Logging Failures / CWE-532 Sensitive Information in Log
+   The std::exception catch arm logs e.what() verbatim: 'auth_handler threw: <e.what()>; failing closed with 500'. The existing code comment in webserver_error_pages.cpp:115-117 acknowledges that e.what() may contain sensitive data (DB connection strings, file paths, user-supplied input that triggered the exception). This is a pre-existing documented risk in the logging subsystem (log_dispatch_error always forwards verbatim, DR-009 Revision 1), not a regression introduced by this change, but it is worth surfacing for the auth path specifically since auth callables are likely to handle credential material.
+   *Recommendation:* Consider adding a note in the auth_handler alias comment (mirroring the existing CWE-532 note in log_dispatch_error) warning auth callable authors to sanitize or wrap exceptions that might embed credential material before re-throwing. No code change required unless a stricter policy is desired for the auth path specifically.
+
+13. [ ] **security-reviewer** | `src/detail/webserver_aliases.cpp:249` | A05: Security Misconfiguration / CWE-209 Information Exposure
+   The auth fail-closed 500 is built with http_response::empty().with_status(500), which produces an empty body regardless of the create_webserver::expose_exception_messages flag. This is the safe default, but it is inconsistent with internal_error_page (webserver_error_pages.cpp:96-97), which does honour expose_exception_messages to emit the exception text in the body for development use. A developer who sets expose_exception_messages(true) expecting all 500 responses to carry diagnostic text will get no body from the auth path, making the inconsistency a silent footgun rather than a security risk.
+   *Recommendation:* Either document in the auth_handler alias comment that the auth 500 deliberately ignores expose_exception_messages (preferred — keeps the auth path fail-safe by default), or route through internal_error_page with a synthetic modded_request if parity with the normal error path is desired. The current behaviour is the safer choice; the documentation gap is the actual deficiency to close.
+
+14. [ ] **spec-alignment-checker** | `specs/tasks/M7-v2-cleanup/_index.md:33` | specification-gap
+   TASK-066 is still listed as 'Backlog' in the milestone index, but TASK-085's own task file (action item 3) claims TASK-066 is 'Done' and TASK-066.md itself carries Status: Done. The dependency note on line 66 of _index.md also still says TASK-066 'blocks' TASK-085, even though both tasks are now marked Done. TASK-085 did not update TASK-066's status in the index when it claimed satisfaction of the TASK-066 dependency.
+   *Recommendation:* Update _index.md line 33 to reflect TASK-066 Status: Done (consistent with TASK-066.md) and optionally update the dependency note on line 66 to past tense, since both tasks have shipped.
+
+15. [ ] **spec-alignment-checker** | `src/detail/webserver_aliases.cpp:231` | ears-requirement
+   PRD-HOOK-REQ-005 states 'When a hook throws then the system shall route the exception through the DR-009 §5.2 dispatch error path — log via log_error, invoke the configured handler_exception hook chain'. The fail-closed guard added by TASK-085 logs via log_dispatch_error and short-circuits with a 500 response directly, bypassing the handler_exception hook chain. This is a deliberate and documented security exception (auth seat is a security boundary), but it technically diverges from the literal PRD-HOOK-REQ-005 text. The hooks.md section added in this commit documents the divergence, but the PRD itself is not updated to reflect the auth_handler carve-out.
+   *Recommendation:* Consider adding a note in product_specs.md §3.8 EARS Requirements (PRD-HOOK-REQ-005) that auth_handler is a named exception to the generic exception-routing policy due to the security fail-closed requirement. Alternatively, accept this as intentional and documented in the architecture spec — the finding is informational.
+
+16. [ ] **test-quality-reviewer** | `test/unit/auth_handler_optional_signature_test.cpp:181` | missing-test
+   The production code in webserver_aliases.cpp has two catch clauses: `catch (const std::exception& e)` and `catch (...)`. Only the std::exception branch is exercised by the test (which throws std::runtime_error). The catch-all `catch (...)` branch for non-std::exception throws (e.g., throwing an int) is unverified. Given that auth is a security boundary, confirming the catch-all path also returns 500 and not an unhandled crash would strengthen the pin.
+   *Recommendation:* Add a second test case `throwing_auth_handler_non_std_exception_fails_closed_with_500` that installs an auth_handler throwing a non-std::exception value (e.g., `throw 42;`) and asserts the response code is 500. This ensures the `catch (...)` branch is pinned to the same fail-closed contract.
+
+17. [ ] **test-quality-reviewer** | `test/unit/webserver_register_smartptr_test.cpp:153` | naming-convention
+   `set_up()` is left as an empty stub with only a comment. The littletest framework likely requires `set_up` and `tear_down` to be defined; the empty body is fine, but keeping a visually prominent empty function with a multi-line comment attached to it adds more ceremony than the empty body deserves.
+   *Recommendation:* Either remove the body content and collapse to `void set_up() {}` on one line (already done for the declaration itself on line 156, so this is already acceptable), or simply confirm it matches the project's convention. This is cosmetic only.
+
+18. [ ] **test-quality-reviewer** | `test/unit/webserver_register_smartptr_test.cpp:199` | aaa-violation
+   In `unique_ptr_dtor_runs_on_webserver_destruction` and `shared_ptr_caller_keeps_resource_alive`, the first assertion `LT_CHECK_EQ(dtor_count.load(), 0)` appears as the first statement of the test body before any Act phase. This is a pre-condition check, not an assertion on behaviour. Since `dtor_count` is a freshly constructed local variable with value 0, the check is tautological and adds noise without protecting against a real regression.
+   *Recommendation:* Remove the `LT_CHECK_EQ(dtor_count.load(), 0)` opening assertion. The local variable is trivially 0 at construction; if that ever changes it would be a language bug, not a test regression. Removing the line shortens the Arrange phase and makes the meaningful assertions (after the Act block) stand out more clearly.