Merge TASK-078: resolve commented-out CONNECT bodies and dead validator integ test

Deletes two `/* ... */`-commented CONNECT-method test bodies in
`test/integ/basic.cpp` (`basic_suite::complete` and `basic_suite::only_render`,
dating to bd39cb4, Nov 2017) — the hang was a libcurl client-side artifact, not
a server bug. Server-side CONNECT dispatch remains pinned by
`test/unit/http_resource_test.cpp::render_connect_returns_by_value` and recorded
as REGRESSION.md divergence #6.

Also deletes `basic_suite::validator_builder`, which only asserted "server boots
with a validator callback set" — the validator hook has been unwired from the
dispatch path since 9163a4f (Jan 2013). Equivalent compile-time coverage already
lives in `test/unit/create_webserver_test.cpp::builder_validator`. RELEASE_NOTES
records that v2's `request_received` / `accept_decision` hooks are the
documented replacement for the URL-veto use case.

No public surface change. Basic integ test count drops by 1 (97 -> 96); full
`make check` 103/103 PASS. Includes the status flip to Done and 9 persisted
minor review findings (0 critical, 0 major) for future cleanup sweeps.

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

Author: etr

RELEASE_NOTES.md

@@ -80,6 +80,22 @@ v1.x is end-of-life on the day v2.0 ships.
   the legacy callable is no longer implicitly convertible to
   `auth_handler_ptr`. There is no runtime ABI surface to break — the
   overload was a header-only inline forwarder.
+- **`create_webserver::validator(...)` dispatch semantics — confirmed
+  never wired (TASK-078).** The `validator` builder method and the
+  `validator_ptr` callback typedef on `create_webserver` are retained
+  as inert v1 surface — the callback has not been invoked from the
+  dispatch path since commit `9163a4f` (Jan 2013, "Eliminated unescaper
+  and validator delegates"). v2 ships the surface unchanged for source
+  compatibility, but a configured validator callback is dead code. The
+  v2 replacement is `webserver::add_hook(hook_phase::request_received,
+  ...)` returning `hook_action::respond_with(http_response)` to reject
+  a request, or `hook_phase::accept_decision` for a connection-scoped
+  veto (see `specs/architecture/04-components/hooks.md`). The legacy
+  `validator_builder` integ test in `test/integ/basic.cpp` exercised
+  no validation behaviour (it only asserted the server booted with a
+  validator set) and has been removed; the equivalent compile-time
+  builder pin survives at
+  `test/unit/create_webserver_test.cpp::builder_validator`.
 - **v1 `registered_resources*` registration maps (internal).** No
   public-API change. Dispatch already routed through the v2 3-tier
   route table (`lookup_v2()`) after TASK-053; the residual

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

@@ -8,9 +8,9 @@
 `test/integ/basic.cpp:821-830, 885-896` carry `/* … */`-commented-out CONNECT-method test bodies with no tracking issue. `basic.cpp:2895` (`validator_builder` test) only asserts the server boots; the validator hook is "stored but not currently called in webserver". Either restore the bodies or remove them with a tracking link.
 
 **Action Items:**
-- [ ] Investigate why the CONNECT-method blocks at `basic.cpp:821-830` and `885-896` were commented out (git log of the lines). If the commented behaviour is no longer achievable under v2 dispatch, delete the blocks and add a one-line note to `test/REGRESSION.md` recording the v1-only feature.
-- [ ] If the blocks should still pass under v2, uncomment them and fix any breakage.
-- [ ] For the `validator_builder` test at `basic.cpp:2895`: either wire the validator hook into the dispatch path so the test exercises real behaviour, or delete the test and note in `RELEASE_NOTES.md` that `validator_builder` is a v1 surface with no v2 semantics.
+- [x] Investigate why the CONNECT-method blocks at `basic.cpp:821-830` and `885-896` were commented out (git log of the lines). If the commented behaviour is no longer achievable under v2 dispatch, delete the blocks and add a one-line note to `test/REGRESSION.md` recording the v1-only feature.
+- [x] If the blocks should still pass under v2, uncomment them and fix any breakage.
+- [x] For the `validator_builder` test at `basic.cpp:2895`: either wire the validator hook into the dispatch path so the test exercises real behaviour, or delete the test and note in `RELEASE_NOTES.md` that `validator_builder` is a v1 surface with no v2 semantics.
 
 **Dependencies:**
 - Blocked by: None
@@ -25,4 +25,4 @@
 **Related Requirements:** PRD §2 test reliability NFR
 **Related Decisions:** None new
 
-**Status:** Backlog
+**Status:** Done

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

@@ -42,7 +42,7 @@ TASK-093).
 | TASK-075 | Propagate `wait_for_server_ready` to sibling hooks integ tests | HIGH | M | Done |
 | TASK-076 | Replace tautological-pass blocks in TLS test lanes | HIGH | M | Done |
 | TASK-077 | Restore Windows / Darwin coverage in skipped test suites | HIGH | L | Done |
-| TASK-078 | Resolve commented-out CONNECT-method test bodies | HIGH | S | Backlog |
+| TASK-078 | Resolve commented-out CONNECT-method test bodies | HIGH | S | Done |
 | TASK-079 | Drive nonce/opaque state machine in v2 digest-auth integ tests | MED | M | Backlog |
 | TASK-080 | Tighten threadsafety_stress latency gate back from 100× to 10× | MED | M | Backlog |
 | TASK-081 | Fill empty-on-correct-build unit suites and re-enable pthread leak detector | MED | M | Backlog |

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.

test/REGRESSION.md

@@ -164,6 +164,28 @@ it at the head of `lookup_v2`, including cache keying. This brings v2
 in line with v1 semantics; the test
 `exact_path_normalization_aliases` pins it.
 
+### 6. CONNECT-method client-roundtrip integ test (TASK-078)
+
+`test/integ/basic.cpp` previously contained two `/* ... */`-commented
+CONNECT bodies inside `basic_suite::complete` and `basic_suite::only_render`
+that issued `curl_easy_setopt(curl, CURLOPT_CUSTOMREQUEST, "CONNECT")`
+through the high-level libcurl easy API. They were commented out in
+2017 (commit bd39cb4) because libcurl treats CONNECT as a tunnel-establishing
+verb and waits for the socket to be upgraded; `curl_easy_perform` then
+hangs against a plain HTTP server. The bodies have been deleted; there is
+no way to drive a CONNECT round-trip through `curl_easy_perform` and the
+high-level easy API has no other shape that produces a useful integ
+assertion against an HTTP server that is not also a proxy.
+
+Server-side CONNECT dispatch IS exercised: the method-to-render-fn table
+in `src/detail/webserver_request.cpp` (the `methods[]` array around line
+608) maps the `CONNECT` wire token to `http_resource::render_connect`,
+and `test/unit/http_resource_test.cpp::render_connect_returns_by_value`
+pins the public signature. The deleted integ blocks added no coverage
+beyond that.
+
+No port required; not a v1-only feature, but a v1-era libcurl misuse.
+
 ## How to extend
 
 When adding a new routing pattern to the public API:

test/integ/basic.cpp

@@ -833,16 +833,6 @@ LT_BEGIN_AUTO_TEST(basic_suite, complete)
     LT_ASSERT_EQ(res, 0);
     curl_easy_cleanup(curl);
     }
-/*
-    {
-    CURL* curl = curl_easy_init();
-    curl_easy_setopt(curl, CURLOPT_URL, "localhost:" PORT_STRING "/base");
-    curl_easy_setopt(curl, CURLOPT_CUSTOMREQUEST, "CONNECT");
-    CURLcode res = curl_easy_perform(curl);
-    LT_ASSERT_EQ(res, 0);
-    curl_easy_cleanup(curl);
-    }
-*/
 
     {
     CURL* curl = curl_easy_init();
@@ -897,19 +887,6 @@ LT_BEGIN_AUTO_TEST(basic_suite, only_render)
     LT_CHECK_EQ(s, "OK");
     curl_easy_cleanup(curl);
 
-/*
-    s = "";
-    curl = curl_easy_init();
-    curl_easy_setopt(curl, CURLOPT_URL, "localhost:" PORT_STRING "/base");
-    curl_easy_setopt(curl, CURLOPT_CUSTOMREQUEST, "CONNECT");
-    curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, writefunc);
-    curl_easy_setopt(curl, CURLOPT_WRITEDATA, &s);
-    res = curl_easy_perform(curl);
-    LT_ASSERT_EQ(res, 0);
-    LT_CHECK_EQ(s, "OK");
-    curl_easy_cleanup(curl);
-*/
-
     s = "";
     curl = curl_easy_init();
     curl_easy_setopt(curl, CURLOPT_URL, "localhost:" PORT_STRING "/base");
@@ -2907,39 +2884,6 @@ LT_BEGIN_AUTO_TEST(basic_suite, single_resource_mode)
     ws2.stop();
 LT_END_AUTO_TEST(single_resource_mode)
 
-// Test validator builder method (validator is stored but not currently called in webserver)
-bool test_validator_func(const std::string& url) {
-    return url.find("valid") != std::string::npos;
-}
-
-LT_BEGIN_AUTO_TEST(basic_suite, validator_builder)
-    // Test that the validator builder method works (for coverage of create_webserver.hpp)
-    webserver ws2{create_webserver(PORT + 72)
-        .validator(test_validator_func)};
-    auto resource = std::make_shared<ok_resource>();
-    ws2.register_path("test", resource);
-    ws2.start(false);
-
-    // Just verify the server works with a validator set
-    {
-        curl_global_init(CURL_GLOBAL_ALL);
-        string s;
-        CURL *curl = curl_easy_init();
-        CURLcode res;
-        std::string url = "http://localhost:" + std::to_string(PORT + 72) + "/test";
-        curl_easy_setopt(curl, CURLOPT_URL, url.c_str());
-        curl_easy_setopt(curl, CURLOPT_HTTPGET, 1L);
-        curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, writefunc);
-        curl_easy_setopt(curl, CURLOPT_WRITEDATA, &s);
-        res = curl_easy_perform(curl);
-        LT_ASSERT_EQ(res, 0);
-        LT_CHECK_EQ(s, "OK");
-        curl_easy_cleanup(curl);
-    }
-
-    ws2.stop();
-LT_END_AUTO_TEST(validator_builder)
-
 // Test resource with no render methods overridden (exercises empty_render path)
 // Note: empty_render returns string_response with code -1, which triggers internal error
 class empty_render_resource : public http_resource {