|
| 1 | +# Unworked Review Issues |
| 2 | + |
| 3 | +**Run:** 2026-06-07 13:16:18 |
| 4 | +**Task:** TASK-069 |
| 5 | +**Total:** 12 (0 critical, 0 major, 12 minor) |
| 6 | + |
| 7 | +## Minor |
| 8 | + |
| 9 | +1. [ ] **architecture-alignment-checker** | `test/unit/http_request_pimpl_test.cpp:136` | interface-contract |
| 10 | + 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. |
| 11 | + *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. |
| 12 | + |
| 13 | +2. [ ] **code-quality-reviewer** | `src/http_request.cpp:140` | code-readability |
| 14 | + 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. |
| 15 | + *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. |
| 16 | + |
| 17 | +3. [ ] **code-quality-reviewer** | `test/unit/http_request_pimpl_test.cpp:137` | test-coverage |
| 18 | + 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. |
| 19 | + *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. |
| 20 | + |
| 21 | +4. [ ] **code-quality-reviewer** | `test/unit/http_request_pimpl_test.cpp:137` | readability |
| 22 | + 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'). |
| 23 | + *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.' |
| 24 | + |
| 25 | +5. [ ] **code-simplifier** | `src/http_request.cpp:140` | code-structure |
| 26 | + 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. |
| 27 | + *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. |
| 28 | + |
| 29 | +6. [ ] **code-simplifier** | `src/http_request.cpp:140` | code-structure |
| 30 | + 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. |
| 31 | + *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. |
| 32 | + |
| 33 | +7. [ ] **code-simplifier** | `test/unit/http_request_pimpl_test.cpp:137` | naming |
| 34 | + 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. |
| 35 | + *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. |
| 36 | + |
| 37 | +8. [ ] **housekeeper** | `specs/tasks/M7-v2-cleanup/_index.md:71` | documentation-stale |
| 38 | + 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. |
| 39 | + *Recommendation:* Optionally update the sequencing note to remove completed tasks for clarity, but this is not required. |
| 40 | + |
| 41 | +9. [ ] **security-reviewer** | `src/httpserver/detail/http_request_impl.hpp:33` | insecure-design |
| 42 | + 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. |
| 43 | + *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. |
| 44 | + |
| 45 | +10. [ ] **spec-alignment-checker** | `specs/tasks/M7-v2-cleanup/TASK-069.md:13` | action-item |
| 46 | + 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. |
| 47 | + *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. |
| 48 | + |
| 49 | +11. [ ] **test-quality-reviewer** | `test/unit/http_request_pimpl_test.cpp:137` | naming-convention |
| 50 | + 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. |
| 51 | + *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. |
| 52 | + |
| 53 | +12. [ ] **test-quality-reviewer** | `test/unit/http_request_pimpl_test.cpp:143` | naming-convention |
| 54 | + 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. |
| 55 | + *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. |
0 commit comments