|
| 1 | +# Unworked Review Issues |
| 2 | + |
| 3 | +**Run:** 2026-05-11 21:48:24 |
| 4 | +**Task:** TASK-030 |
| 5 | +**Total:** 23 (0 critical, 0 major, 23 minor) |
| 6 | + |
| 7 | +## Minor |
| 8 | + |
| 9 | +1. [ ] **architecture-alignment-checker** | `src/httpserver/create_webserver.hpp:49` | pattern-violation |
| 10 | + The public `error_handler` typedef (in create_webserver.hpp) and the internal `detail::lambda_handler` alias (in detail/route_entry.hpp) represent the identical type — std::function<http_response(const http_request&)> — but are defined independently in two translation-unit-visible headers. There is no architectural rule requiring them to be unified, and the separation is currently benign (one is public API, one is internal detail), but it creates a subtle risk: if one evolves (e.g., noexcept annotation, allocator param) the other would silently diverge. |
| 11 | + *Recommendation:* Consider having `error_handler` in create_webserver.hpp import or alias `detail::lambda_handler` once the internal header stabilises, or add a static_assert in webserver.hpp verifying the two types are the same. This is not urgent for v2.0 but worth tracking as a TASK-030 follow-up note. |
| 12 | + |
| 13 | +2. [ ] **code-quality-reviewer** | `examples/minimal_ip_ban.cpp:null` | clean-code |
| 14 | + Example file was updated to use the new _handler names but retains a variable named 'res' for the resource object, which now conflicts slightly with the _handler naming convention introduced by this task. |
| 15 | + *Recommendation:* Rename 'res' to 'handler' or a more descriptive name to align with the new naming conventions. |
| 16 | + |
| 17 | +3. [ ] **code-quality-reviewer** | `src/httpserver/create_webserver.hpp:46` | code-readability |
| 18 | + The block comment on lines 46-48 references the task ID (TASK-030) and a PRD requirement ID (PRD-NAM-REQ-003) inline with production code. Task references are useful during development but become outdated maintenance noise once the task is closed. |
| 19 | + *Recommendation:* Trim the comment to a description of the type alias purpose, e.g. '// Handler type for the three error-page setters; returns http_response by value.' Remove task/PRD cross-references from production headers. |
| 20 | + |
| 21 | +4. [ ] **code-quality-reviewer** | `src/httpserver/create_webserver.hpp:null` | code-readability |
| 22 | + The three new _handler setter methods are logically grouped but lack a brief doc comment distinguishing them from the object-resource setters, which could confuse future readers of the header. |
| 23 | + *Recommendation:* Add a one-line comment above each setter (e.g. '// Takes a function-shaped handler, not a resource object') to make the distinction self-documenting. |
| 24 | + |
| 25 | +5. [ ] **code-quality-reviewer** | `src/httpserver/webserver.hpp:82` | code-readability |
| 26 | + The comment on line 82 references the task ID (PRD-NAM-REQ-004) and instructs callers to use webserver ws{cw}. These cross-references are fine during development but production headers should document the intent of the qualifier, not refer to internal task IDs. |
| 27 | + *Recommendation:* Replace with a short descriptive comment, e.g. '// Explicit: prevents accidental implicit conversion from create_webserver.' Remove the PRD-NAM-REQ-004 reference. |
| 28 | + |
| 29 | +6. [ ] **code-quality-reviewer** | `src/webserver.cpp:1594` | code-elegance |
| 30 | + The three error-handler call sites each wrap the by-value return in std::make_shared<http_response>(...). This is correct and consistent, but worth noting as a potential future maintenance point: if the internal_error_page / not_found_page / method_not_allowed_page signatures ever change to return by value, these wrapping sites would need to be updated again. |
| 31 | + *Recommendation:* No immediate action required; the current pattern is idiomatic. Consider an inline helper lambda or a private utility function `wrap_handler_result` if this pattern proliferates to more than three call sites. |
| 32 | + |
| 33 | +7. [ ] **code-quality-reviewer** | `src/webserver.cpp:null` | code-elegance |
| 34 | + The three handler fields (not_found_handler_, method_not_allowed_handler_, internal_error_handler_) follow the same invocation pattern; a small private helper template or lambda could reduce the triplication. |
| 35 | + *Recommendation:* Consider extracting a private dispatch helper to avoid repeating the same null-check + invoke pattern for each of the three handlers. |
| 36 | + |
| 37 | +8. [ ] **code-quality-reviewer** | `test/unit/create_webserver_explicit_test.cpp:1` | test-coverage |
| 38 | + Test file exercises the explicit-constructor guard at compile time via a deleted-implicit-conversion pattern, but does not include a runtime smoke test confirming the constructed webserver is usable after explicit construction. |
| 39 | + *Recommendation:* Add a minimal runtime test that constructs a webserver via explicit create_webserver conversion and verifies it starts successfully, complementing the compile-time check. |
| 40 | + |
| 41 | +9. [ ] **code-quality-reviewer** | `test/unit/create_webserver_explicit_test.cpp:114` | test-coverage |
| 42 | + The runtime test suite (create_webserver_explicit_suite / smoke) contains only LT_CHECK_EQ(true, true), which contributes no actual behavioural assertion. All real guarantees are expressed as static_asserts — the LittleTest harness wrapper adds boilerplate noise without coverage value. |
| 43 | + *Recommendation:* Either remove the runtime test entirely (leave only the TU that compile-checks the static_asserts) or replace the smoke test with at least one runtime assertion, e.g. constructing a webserver from a create_webserver and verifying is_running() returns false. |
| 44 | + |
| 45 | +10. [ ] **code-quality-reviewer** | `test/unit/create_webserver_test.cpp:211` | test-coverage |
| 46 | + The three new builder_not_found_handler, builder_method_not_allowed_handler, and builder_internal_error_handler tests only verify that the chain compiles; they do not assert that the stored handler is actually invoked (i.e. that the value flows through to a webserver and produces the expected response). The same pattern applied to all 55 tests here is an existing issue, but it is freshest and most significant for the three renamed setters that are the core of this task. |
| 47 | + *Recommendation:* Consider adding at least one runtime round-trip test that starts a webserver with a custom not_found_handler, issues a request to an unregistered URL, and asserts the custom 404 response body is returned. The integ/basic.cpp tests do exercise this path (custom_not_found_handler, custom_method_not_allowed_handler), but having a focused unit-level or integ test that covers all three handlers in a single test would be cleaner. |
| 48 | + |
| 49 | +11. [ ] **code-quality-reviewer** | `test/unit/webserver_on_methods_test.cpp:null` | test-coverage |
| 50 | + Tests confirm the renamed methods are callable but do not verify that the old _resource names are absent (i.e., do not compile), leaving a gap where a regression could silently re-introduce the removed names. |
| 51 | + *Recommendation:* Add a compile-time negative test (similar to the explicit-constructor test) or a static_assert to document that the old _resource setter names do not exist on create_webserver. |
| 52 | + |
| 53 | +12. [ ] **code-simplifier** | `src/httpserver/webserver.hpp:0` | naming |
| 54 | + Same minor findings from iteration 1 remain in the source code but are unchanged since that review. |
| 55 | + *Recommendation:* No new action required; findings carried from iter1 are unchanged and non-blocking. |
| 56 | + |
| 57 | +13. [ ] **code-simplifier** | `test/unit/create_webserver_explicit_test.cpp:50` | code-structure |
| 58 | + The six SFINAE detector templates (has_not_found_handler, has_method_not_allowed_handler, has_internal_error_handler, has_not_found_resource, has_method_not_allowed_resource, has_internal_error_resource) are structurally identical 4-line boilerplate blocks. They could be collapsed into a single parametric detector, reducing the test body from ~50 lines of trait code to ~10. |
| 59 | + *Recommendation:* Replace all six trait templates with one generic `has_setter` detector parameterised on the method name via a pointer-to-member or a lambda probe, for example: `template <typename CW, typename Fn, typename = void> struct has_setter : std::false_type {}; template <typename CW, typename Fn> struct has_setter<CW, Fn, std::void_t<decltype(std::declval<Fn>()(std::declval<CW&>()))>> : std::true_type {};` and pass lambdas that invoke each setter. This is optional polish for a test-only file; the current verbose form is also acceptable and readable. |
| 60 | + |
| 61 | +14. [ ] **code-simplifier** | `test/unit/create_webserver_explicit_test.cpp:79` | code-structure |
| 62 | + The `always_void` template struct is declared but never referenced anywhere in the file. It is dead code left over from an earlier draft of the SFINAE detectors. |
| 63 | + *Recommendation:* Remove the three-line `template <typename T> struct always_void { using type = void; };` declaration entirely. All six SFINAE detectors already use `std::void_t` directly and have no dependency on `always_void`. |
| 64 | + |
| 65 | +15. [ ] **housekeeper** | `/Users/etr/progs/libhttpserver/.worktrees/TASK-030/specs/product_specs.md:128` | documentation-stale |
| 66 | + The product spec problem-description prose at line 128 mentions 'render_ptr' as a historical example of the parallel function-handler convention. This is in a 'Problem / outcome' section describing the pre-v2.0 state, so it is intentionally historical, not a stale current-API reference. |
| 67 | + *Recommendation:* No action required — the mention is in a historical problem description, not a current API specification. The EARS requirements at PRD-NAM-REQ-003 correctly document the new _handler suffix. |
| 68 | + |
| 69 | +16. [ ] **performance-reviewer** | `src/webserver.cpp:1595` | memory-allocation |
| 70 | + The three dispatch adapters (not_found_page, method_not_allowed_page, internal_error_page) now perform one extra heap allocation per invocation: make_shared allocates the shared control block + http_response together, then move-constructs the http_response from the handler's by-value return. Previously the handler returned shared_ptr<http_response> directly, so no extra allocation was needed. The move construction is correct (http_response's copy constructor is deleted and its move constructor is noexcept), so no copy occurs. The overhead is one additional heap allocation (the make_shared call) per error response. |
| 71 | + *Recommendation:* These paths are cold (error responses only), so the extra allocation is not a practical concern. If a future micro-optimisation is desired, the adapter could be changed to use allocate_shared with a pool allocator, or the internal representation could switch to returning http_response by value all the way up to the MHD layer. No action needed now. |
| 72 | + |
| 73 | +17. [ ] **performance-reviewer** | `src/webserver.cpp:1595` | memory-allocation |
| 74 | + The handler is called as `parent->not_found_handler(*mr->dhr)` — the result is a temporary (prvalue) http_response. It is passed directly into make_shared<http_response>(...), which move-constructs in-place inside the allocation. This is correct and optimal: no explicit std::move() is needed or beneficial here because the argument is already a prvalue and NRVO/move-elision applies. However, readers unfamiliar with value-category rules may add a redundant std::move() or, worse, a copy. A brief comment would prevent future regressions. |
| 75 | + *Recommendation:* Add a short comment such as `// handler returns by value; make_shared move-constructs — no copy` above each of the three adapter sites (lines 1595, 1605, 1615) to prevent unintentional copies being introduced during future refactors. |
| 76 | + |
| 77 | +18. [ ] **security-reviewer** | `src/httpserver/create_webserver.hpp:49` | insecure-design |
| 78 | + The new error_handler typedef (std::function<http_response(const http_request&)>) diverges in return type from auth_handler_ptr (std::function<std::shared_ptr<http_response>(const http_request&)>, line 66). This asymmetry means a caller who accidentally passes a shared_ptr-returning lambda to not_found_handler/method_not_allowed_handler/internal_error_handler will get a compile error rather than silent behaviour, which is safe. The inconsistency is a minor API hygiene issue rather than a security defect, but it warrants a note so future contributors do not silently unify them in the wrong direction. |
| 79 | + *Recommendation:* Document the intentional signature difference in the typedef comment, or track alignment of auth_handler_ptr with the by-value pattern for a future task. |
| 80 | + |
| 81 | +19. [ ] **security-reviewer** | `src/webserver.cpp:2099` | insecure-design |
| 82 | + The catch-all handler for user-supplied internal_error_handler exceptions calls internal_error_page(mr) (with force_our=false) before falling back to internal_error_page(mr, true). If the user handler throws AND the first fallback also throws (e.g., OOM), the second catch catches and calls force_our=true — this is the correct two-level fallback. However, there is no explicit log or audit trail when the user-supplied handler throws, making it impossible to distinguish a misbehaving error handler from a normal error path in production. |
| 83 | + *Recommendation:* Add a log_error call inside the outer catch block (before the inner try) recording that the user-supplied internal_error_handler threw, so operators can detect misbehaving handlers. CWE-390 (Detection of Error Condition Without Action). |
| 84 | + |
| 85 | +20. [ ] **test-quality-reviewer** | `test/unit/create_webserver_explicit_test.cpp:114` | unnecessary-test |
| 86 | + The 'smoke' test asserts `LT_CHECK_EQ(true, true)` — it can never fail regardless of implementation. The file's real value comes entirely from the static_asserts at file scope; the runtime test adds zero regression protection and only exists to satisfy the littletest harness requiring at least one LT_BEGIN_AUTO_TEST entry. |
| 87 | + *Recommendation:* Either document explicitly why a placeholder test is required by the harness (a one-line comment `// harness requires at least one runtime test; all guards are static_asserts above` already present as implied context), or promote one of the is_constructible checks to a runtime assertion: `webserver ws{create_webserver(8080)}; LT_CHECK_EQ(ws.port(), 8080);` — this would make the smoke test a real regression guard at zero cost. |
| 88 | + |
| 89 | +21. [ ] **test-quality-reviewer** | `test/unit/create_webserver_explicit_test.cpp:114` | naming-convention |
| 90 | + The test is named 'smoke' but the file already has a descriptive comment block explaining its purpose. The name 'smoke' is consistent with codebase conventions but within this particular file it is slightly misleading because there is nothing being smoked — the file is purely a compile-time sentinel. A name like `harness_entry_point` or `compile_time_sentinels_pass` would better communicate intent. |
| 91 | + *Recommendation:* Low priority. Consider renaming to `compile_time_sentinels_pass` for clarity, or keep 'smoke' if the project convention uses that name for minimal harness stubs. |
| 92 | + |
| 93 | +22. [ ] **test-quality-reviewer** | `test/unit/create_webserver_explicit_test.cpp:79` | unnecessary-test |
| 94 | + `template <typename T> struct always_void { using type = void; };` is declared but never referenced. The negative SFINAE detectors on lines 82-100 all use `std::void_t` directly, not `always_void`. This dead template is a red herring for future readers. |
| 95 | + *Recommendation:* Remove the unused `always_void` template entirely. All three negative detectors already rely on the standard `std::void_t` correctly. |
| 96 | + |
| 97 | +23. [ ] **test-quality-reviewer** | `test/unit/create_webserver_test.cpp:42` | unnecessary-test |
| 98 | + All 55 tests in this file use `LT_CHECK_EQ(true, true)` as their sole assertion — they verify only that the builder call compiles and does not throw, but assert nothing about the resulting state. This is pre-existing (not introduced by TASK-030) but the task added three new handler tests (builder_not_found_handler, builder_method_not_allowed_handler, builder_internal_error_handler at lines 211-235) using the same trivial pattern, missing an opportunity to assert the handler is callable or that the webserver stores it. |
| 99 | + *Recommendation:* For the three new handler tests specifically, verify observable behavior: e.g. construct a webserver from the create_webserver, then call the stored handler and assert the returned status code matches expectations. This directly pins the PRD-NAM-REQ-003 rename at the runtime level, complementing the compile-time SFINAE guards in the explicit test. |
0 commit comments