TASK-078: persist 9 unworked review findings

All 9 are minor (0 critical, 0 major) — captured for future cleanup
sweeps, none block merge.

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

Author: etr

specs/unworked_review_issues/2026-06-11_004134_task-078.md

@@ -0,0 +1,43 @@
+# Unworked Review Issues
+
+**Run:** 2026-06-11 00:41:34
+**Task:** TASK-078
+**Total:** 9 (0 critical, 0 major, 9 minor)
+
+## Minor
+
+1. [ ] **architecture-alignment-checker** | `RELEASE_NOTES.md:83` | pattern-violation
+   The RELEASE_NOTES entry for validator dispatch says the v2 replacement is 'hook_phase::accept_decision for a connection-scoped veto'. Per specs/architecture/04-components/hooks.md, accept_decision fires at policy_callback (MHD_CONNECTION_NOTIFY_STARTED) and is observation-only with no short-circuit capability — it cannot veto a request. The correct connection-scoped veto is through MHD's IP-block API (block_ip/unblock_ip). The appropriate hook for per-request rejection is hook_phase::request_received or hook_phase::before_handler, not accept_decision.
+   *Recommendation:* Revise the RELEASE_NOTES.md validator note to remove 'hook_phase::accept_decision for a connection-scoped veto' or clarify that accept_decision is observation-only. The connection-scoped veto surface is webserver::block_ip; the request-level veto is hook_phase::request_received or before_handler with hook_action::respond_with(...).
+
+2. [ ] **code-quality-reviewer** | `test/integ/basic.cpp:2958` | code-readability
+   A stale explanatory comment ('Note: CONNECT method is a special HTTP method for tunneling...') was left in place at line 2958 after the commented-out CONNECT blocks were deleted. The note was written to contextualise those deleted blocks and now floats without an anchor, adding noise with no referent.
+   *Recommendation:* Remove the three-line comment block at lines 2958-2960 since the test blocks it was explaining are gone. Per the clean-code Comments Rules, comments that no longer explain live code are noise and should be deleted.
+
+3. [ ] **code-quality-reviewer** | `test/unit/http_resource_test.cpp:112` | test-coverage
+   The REGRESSION.md entry cites 'render_connect_returns_by_value' as the surviving CONNECT coverage pin, but the actual assertion in that test is a static_assert on the return type (decltype check), not a runtime behaviour assertion. This is accurate but the documentation's phrasing ('Server-side CONNECT dispatch IS exercised') slightly overstates what the static assertion covers — it checks return type, not dispatch routing.
+   *Recommendation:* Optionally soften the REGRESSION.md wording at line 183 from 'Server-side CONNECT dispatch IS exercised' to 'The return-type contract for render_connect is pinned by a static_assert in ...' to keep the documentation precise. This is optional and does not block approval.
+
+4. [ ] **code-simplifier** | `RELEASE_NOTES.md:83` | naming
+   The new RELEASE_NOTES entry uses two different phrasings — 'inert v1 surface' and 'dead code' — to describe the same thing (the validator callback is never invoked). Using both in adjacent sentences introduces a small inconsistency and mild redundancy.
+   *Recommendation:* Pick one term and use it consistently. 'Dead code' is the more precise term for 'stored but never called'; 'inert v1 surface' can be dropped or replaced with 'retained for source compatibility'. For example: 'The validator builder method and the validator_ptr callback typedef are retained for source compatibility but the callback is never invoked from the dispatch path (dead code since commit 9163a4f).' This removes the need for two different framings of the same fact.
+
+5. [ ] **code-simplifier** | `test/integ/basic.cpp:2958` | code-structure
+   Orphan comment referencing the now-deleted CONNECT test behaviour. After the `validator_builder` test block was removed, a standalone comment at line 2958 — '// Note: CONNECT method is a special HTTP method for tunneling that / behaves differently than standard HTTP methods, so we don't test / it the same way as other methods.' — lost its referent. The deleted CONNECT blocks were the only tests this comment was explaining; nothing in the immediately following `all_methods_fallthrough_to_render` test exercises CONNECT, so the comment now describes absent code and adds noise.
+   *Recommendation:* Delete the three-line comment block at lines 2958-2960. The rationale for not testing CONNECT via curl_easy_perform is already captured in test/REGRESSION.md §6, which is the appropriate place for such explanations.
+
+6. [ ] **housekeeper** | `specs/architecture/04-components/webserver.md:17` | architecture-not-updated
+   The 'Consumes' line still lists `validator` alongside `log_access`, `log_error`, `unescaper`, and `auth_handler` with no caveat. TASK-078's RELEASE_NOTES.md entry (under 'What's gone') now explicitly documents that the `validator` callback has never been wired into the dispatch path since 2013 and is inert dead code. The architecture component description is therefore misleading: it implies `validator` is an active consumed callback on the same footing as `auth_handler`, when it is actually a retained but non-functional v1 surface.
+   *Recommendation:* Add a parenthetical to the `validator` entry on that line — e.g. `validator` (retained for source compatibility; callback is not invoked in dispatch — see RELEASE_NOTES.md §What's gone) — or add a brief bullet under 'Key design notes' pointing at the RELEASE_NOTES entry. No full re-architecture is needed. Run /groundwork:source-architecture-from-code if a full resync is preferred.
+
+7. [ ] **security-reviewer** | `src/httpserver/webserver.hpp:248` | insecure-design
+   The public `get_request_validator()` accessor (line 248-251) and the `validator_ptr validator` field (line 410) remain in the shipped header with no deprecation attribute or doxygen `@deprecated` tag. Downstream users who read the accessor signature in isolation — without consulting RELEASE_NOTES.md — may still believe the validator is wired into the dispatch path and deploy a security control that is silently inert (CWE-636: Not Failing Securely).
+   *Recommendation:* Add `[[deprecated("validator callback is not invoked by v2 dispatch; use webserver::add_hook(hook_phase::request_received, ...) instead")]]` to the `validator()` builder method in create_webserver.hpp and the `get_request_validator()` accessor in webserver.hpp, so any consumer that compiles against v2 headers receives a compile-time warning rather than a silent security no-op.
+
+8. [ ] **spec-alignment-checker** | `test/integ/basic.cpp:2958` | specification-gap
+   A standalone comment '// Note: CONNECT method is a special HTTP method for tunneling...' remains in basic.cpp after the deletion of the CONNECT blocks. This comment was not part of the original commented-out block bodies and provides no tracking link; it is harmless but slightly redundant given that REGRESSION.md §6 now carries the full explanation.
+   *Recommendation:* Consider removing the standalone CONNECT note comment at line 2958 from basic.cpp since the authoritative explanation now lives in test/REGRESSION.md §6, keeping basic.cpp clean.
+
+9. [ ] **test-quality-reviewer** | `test/integ/basic.cpp:3064` | missing-test
+   PORT+72 is now unused (validator_builder removed) but there is no comment at the vacated slot noting it is free, unlike the explicit note left at PORT+76 (line 3064-3068). A future author adding a test might accidentally collide with PORT+72 if they only scan for the first gap.
+   *Recommendation:* Add a one-line comment near the end of the PORT+70/+71/+73 block (after single_resource_mode) similar to the PORT+76 note: '// Port 72 is free for future tests (validator_builder removed by TASK-078).' This is cosmetic — the missing comment poses no correctness risk.