TASK-069: persist 12 unworked review findings and mark Done

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

Author: etr

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

@@ -8,9 +8,9 @@
 The two-arg `http_request_impl` constructor at `http_request_impl.hpp:95-99` is preserved "for source compatibility" with no removal date. Remove it now that the v2 cutover is complete, so the only constructor is the canonical one. Internal headers do not need a deprecation cycle.
 
 **Action Items:**
-- [ ] Grep `src/` and `test/` for callers of the two-arg form. Migrate each to the canonical form.
-- [ ] Delete the two-arg overload and its comment block.
-- [ ] Verify `make check` is green across all build-flag matrix lanes (HAVE_BAUTH, HAVE_DAUTH, HAVE_GNUTLS, HAVE_WEBSOCKET).
+- [x] Grep `src/` and `test/` for callers of the two-arg form. Migrate each to the canonical form.
+- [x] Delete the two-arg overload and its comment block.
+- [x] Verify `make check` is green across all build-flag matrix lanes (HAVE_BAUTH, HAVE_DAUTH, HAVE_GNUTLS, HAVE_WEBSOCKET).
 
 **Dependencies:**
 - Blocked by: TASK-015 (request impl PIMPL split, Done)
@@ -25,4 +25,4 @@ The two-arg `http_request_impl` constructor at `http_request_impl.hpp:95-99` is
 **Related Requirements:** PRD §2 API minimalism
 **Related Decisions:** None new
 
-**Status:** Backlog
+**Status:** Done

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

@@ -33,7 +33,7 @@ TASK-093).
 | TASK-066 | Runtime setter for hook alias slots | MED | M | Backlog |
 | TASK-067 | Remove v1 `registered_resources*` maps and `namespace compat` shim | MED | L | Done |
 | TASK-068 | `connection_state` hardening — CWE-226 / CWE-14 | MED | S | Done |
-| TASK-069 | Remove transitional two-arg `http_request_impl` constructor | MED | S | Backlog |
+| TASK-069 | Remove transitional two-arg `http_request_impl` constructor | MED | S | Done |
 | TASK-070 | Migrate `hook_table_` to `std::atomic<std::shared_ptr<T>>` | MED | M | Backlog |
 | TASK-071 | Wire `install_not_found_alias_` stub and remove dead `lambda_handler` arm | MED | S | Backlog |
 | TASK-072 | Arena-allocated unescape on the warm path | MED | M | Backlog |

specs/unworked_review_issues/2026-06-07_131618_task-069.md

@@ -0,0 +1,55 @@
+# Unworked Review Issues
+
+**Run:** 2026-06-07 13:16:18
+**Task:** TASK-069
+**Total:** 12 (0 critical, 0 major, 12 minor)
+
+## Minor
+
+1. [ ] **architecture-alignment-checker** | `test/unit/http_request_pimpl_test.cpp:136` | interface-contract
+   The test file now includes 'httpserver/detail/http_request_impl.hpp' directly using HTTPSERVER_COMPILATION guard semantics, which is correct per DR-002. However, the include appears in a test TU (not a library TU), so the build system must supply HTTPSERVER_COMPILATION in the test AM_CPPFLAGS. The comment at line 135 ('HTTPSERVER_COMPILATION is supplied by test/Makefile.am AM_CPPFLAGS') confirms this is already the case for this test file, so there is no violation — just a note that any future test file added for impl-level static_asserts must replicate that flag.
+   *Recommendation:* No action required. The existing pattern (HTTPSERVER_COMPILATION in test/Makefile.am AM_CPPFLAGS for this test binary) already covers the requirement. Document in the test-file header comment that the flag is needed for the new include, which line 31's existing comment already does.
+
+2. [ ] **code-quality-reviewer** | `src/http_request.cpp:140` | code-readability
+   The heap-fallback branch now constructs a polymorphic_allocator inline at the call site (`std::pmr::polymorphic_allocator<>(std::pmr::get_default_resource())`), while the arena branch reuses the already-constructed `alloc` variable. The asymmetry is minor but worth noting for future readers who might wonder if the two branches differ in resource ownership.
+   *Recommendation:* An explanatory inline comment on the heap branch — e.g. `// explicit allocator mirrors arena branch signature` — would make the symmetry intention clear. Not blocking.
+
+3. [ ] **code-quality-reviewer** | `test/unit/http_request_pimpl_test.cpp:137` | test-coverage
+   The TASK-069 block comment mixes two concerns: it explains the current constructor surface and doubles as a guard rationale. While informative, the comment slightly violates the 'explain intent, not mechanics' principle — the static_assert messages already carry the constraint prose.
+   *Recommendation:* Consider condensing the block comment to a single line referencing the task, letting the static_assert strings carry the detail. This keeps the comment to the 'why' rather than re-stating what the assertions do.
+
+4. [ ] **code-quality-reviewer** | `test/unit/http_request_pimpl_test.cpp:137` | readability
+   The TASK-069 comment block in the test file says 'After v2 cutover' without giving any reader a way to correlate this with a date or version tag. Since this is a static_assert regression guard meant to prevent future accidental re-introduction, the comment would be more durable if it noted what architectural state it is pinning (e.g. 'All callers now use the three-arg form; this assert is a permanent regression guard').
+   *Recommendation:* Expand the comment from a transitional rationale to a permanent guard rationale, e.g.: 'Permanent regression guard: all callers use the three-arg (MHD_Connection*, unescaper_ptr, alloc) form. The two-arg overload is gone; if it is ever re-added this assertion will break the build.'
+
+5. [ ] **code-simplifier** | `src/http_request.cpp:140` | code-structure
+   The explicit `std::pmr::polymorphic_allocator<>(std::pmr::get_default_resource())` construction in the heap-fallback branch is slightly verbose. Since the three-arg constructor already names `alloc` in the arena branch (line 149), a named local variable here would unify the two call sites and make it easier to grep the allocator handoff.
+   *Recommendation:* Introduce a named local before the if/else: `std::pmr::polymorphic_allocator<> alloc(res);` and use it in both branches. This removes the repeated `std::pmr::get_default_resource()` literal and makes the two construction sites structurally identical.
+
+6. [ ] **code-simplifier** | `src/http_request.cpp:140` | code-structure
+   The heap-fallback branch explicitly constructs `std::pmr::polymorphic_allocator<>(std::pmr::get_default_resource())` inline, while the arena branch (line 149) extracts the allocator into a named local variable `alloc`. The two branches are subtly inconsistent in style.
+   *Recommendation:* For symmetry with the arena branch, extract the default-resource allocator to a named local in the heap branch: `std::pmr::polymorphic_allocator<> default_alloc(std::pmr::get_default_resource()); impl_.reset(new detail::http_request_impl(underlying_connection, unescaper, default_alloc));`. This is a pure style point — behavior is identical either way.
+
+7. [ ] **code-simplifier** | `test/unit/http_request_pimpl_test.cpp:137` | naming
+   The block comment above the static_asserts mentions 'the no-arg default (test-request path, delegates to the three-arg form)' but the actual constructor delegates to the three-arg form with `nullptr, nullptr` — the comment does not mention those sentinel values, which could cause confusion if someone later reads why the default exists.
+   *Recommendation:* Extend the comment to say: 'the zero-arg default (test-request path, delegates to the three-arg form with nullptr connection and unescaper)'. This is a documentation-only fix; no code changes needed.
+
+8. [ ] **housekeeper** | `specs/tasks/M7-v2-cleanup/_index.md:71` | documentation-stale
+   The 'recommended sequencing' note in _index.md still lists TASK-069 among the mechanical Ss to knock out in week one, which is now moot since it is Done. This is a cosmetic staleness, not a tracking gap.
+   *Recommendation:* Optionally update the sequencing note to remove completed tasks for clarity, but this is not required.
+
+9. [ ] **security-reviewer** | `src/httpserver/detail/http_request_impl.hpp:33` | insecure-design
+   The internal header guard `#if !defined(HTTPSERVER_COMPILATION)` prevents inclusion outside the library translation units, but test/unit/http_request_pimpl_test.cpp now includes this header directly (line 34 of the test file). This only works because the test is compiled with HTTPSERVER_COMPILATION defined. There is no build-system enforcement visible in the diff ensuring that the test target always carries that flag; if a future consumer adds the include without the flag they get a hard #error, which is safe, but the compile-time gate is not verified by any CI check visible in this diff.
+   *Recommendation:* No action required from a security standpoint — the #error guard is the right mechanism. Optionally add a build-system comment or a note in the test file that HTTPSERVER_COMPILATION must remain in AM_CPPFLAGS for this test target.
+
+10. [ ] **spec-alignment-checker** | `specs/tasks/M7-v2-cleanup/TASK-069.md:13` | action-item
+   Action item 3 calls for verifying 'make check is green across all build-flag matrix lanes (HAVE_BAUTH, HAVE_DAUTH, HAVE_GNUTLS, HAVE_WEBSOCKET)'. The static_assert test in http_request_pimpl_test.cpp is compile-time only and covers the structural invariant well, but there is no evidence in the diff of an explicit multi-flag build matrix run or CI gate verifying this. The implementation is correct and the static_assert is sufficient to prevent regression, but the action item asks for matrix verification specifically.
+   *Recommendation:* Document in the task or CI configuration that the multi-flag matrix build was executed (or that the static_assert in http_request_pimpl_test.cpp is the verification artefact). This is minor because the change is a deletion-only in an internal header; conditional compilation in the removed constructor body was not conditional on any HAVE_* flag.
+
+11. [ ] **test-quality-reviewer** | `test/unit/http_request_pimpl_test.cpp:137` | naming-convention
+   The new assertions are placed under a block comment but are not grouped in a named section or scoped with any label that ties them together as a TASK-069 suite. The existing test file mixes TASK-015, TASK-017, and TASK-069 assertions in one flat list; a brief section separator comment (already present but only for the last block) is the only convention used — consistent, but the TASK-069 block would benefit from a matching closing separator comment for visual symmetry with the style used elsewhere.
+   *Recommendation:* Add a closing blank-line or a short trailing comment after line 153 to mirror the style used before the main() sentinel, keeping the file consistent when future tasks append more assertions.
+
+12. [ ] **test-quality-reviewer** | `test/unit/http_request_pimpl_test.cpp:143` | naming-convention
+   The three new static_assert blocks lack enclosing namespace or comment grouping that separates them from the TASK-017 assertions above. The per-task comment header at line 137 is present but there is no logical grouping (e.g. a blank line before the comment and after the last assertion) to visually delimit the new block from the existing ones at a glance.
+   *Recommendation:* Add one extra blank line before the TASK-069 comment block (line 137) so the section boundary is immediately visible when the file grows further. This is cosmetic only and does not affect correctness.