|
| 1 | +# Unworked Review Issues |
| 2 | + |
| 3 | +**Run:** 2026-05-09 19:21:03 |
| 4 | +**Task:** TASK-022 |
| 5 | +**Total:** 13 (0 critical, 0 major, 13 minor) |
| 6 | + |
| 7 | +## Minor |
| 8 | + |
| 9 | +1. [ ] **architecture-alignment-checker** | `src/httpserver/http_resource.hpp:56` | interface-contract |
| 10 | + The return type of all render_* methods (and the render fallback) remains std::shared_ptr<http_response> rather than http_response by value. TASK-022 action item states 'Update return type to http_response by value' but also notes this is 'coupled with TASK-036's full handler-return refactor'. Leaving shared_ptr for now is the explicitly documented intent, so no violation — but the action item checkbox is not ticked. |
| 11 | + *Recommendation:* No action required in this task; confirm TASK-036 is tracked to complete the return-type migration. The existing task dependency (Blocks: TASK-036) is correct. |
| 12 | + |
| 13 | +2. [ ] **code-quality-reviewer** | `README.md:563` | documentation |
| 14 | + render_patch is absent from the README method list (lines 564-572 enumerate 8 of the 9 render_* methods; render_patch is not listed). This gap pre-existed the v2.0 branch but the task touched this section of the README and is an opportunity to fix it. |
| 15 | + *Recommendation:* Add a bullet for render_patch alongside the other eight render_* methods in the 'The Resource Object' section. |
| 16 | + |
| 17 | +3. [ ] **code-quality-reviewer** | `src/httpserver/http_resource.hpp:62` | readability |
| 18 | + Doxygen comment block still uses inconsistent indentation (mix of 5-space and 6-space leading asterisks across method docs), a pre-existing style issue carried into the renamed methods. |
| 19 | + *Recommendation:* Align comment block indentation to a single consistent style (e.g. always one space after the opening /**). |
| 20 | + |
| 21 | +4. [ ] **code-quality-reviewer** | `test/integ/basic.cpp:69` | code-readability |
| 22 | + Virtual render method overrides in integration test resource classes are missing the 'override' specifier (e.g., render_get, render_post in simple_resource, large_post_resource_last_value, etc.). This was also missing on the old camelCase names before this task, making it a pre-existing issue, but the rename is an opportunity to add override for correctness and readability. |
| 23 | + *Recommendation:* Add 'override' to all render_get/render_post/etc. declarations in test resource classes. The unit test http_resource_test.cpp already sets the correct example with 'override' on line 38. |
| 24 | + |
| 25 | +5. [ ] **code-quality-reviewer** | `test/unit/http_resource_test.cpp:35` | test-coverage |
| 26 | + Only render_get is tested via simple_resource; other renamed virtual methods (render_post, render_put, render_delete, etc.) have no direct unit test coverage confirming they delegate to render(). |
| 27 | + *Recommendation:* Consider adding a parameterised or per-method test asserting each render_<verb> falls back to render() so regressions are caught if a future override breaks the delegation chain. |
| 28 | + |
| 29 | +6. [ ] **code-simplifier** | `README.md:563` | naming |
| 30 | + render_patch is absent from the README's bullet-list of extensible methods (lines 564-572), even though render_patch exists in the header and the webserver dispatch table. The list now documents eight methods; the API exposes nine. This pre-existed in the base branch (render_PATCH was also absent there), but the rename task was an opportunity to fix it. |
| 31 | + *Recommendation:* Add `* _**std::shared_ptr<http_response>** http_resource::render_patch(**const http_request&** req):_ Invoked on an HTTP PATCH request.` after the render_put bullet so the documented API matches the implementation. |
| 32 | + |
| 33 | +7. [ ] **code-simplifier** | `test/integ/basic.cpp:69` | code-structure |
| 34 | + The rename was applied throughout test/integ/basic.cpp but the `override` keyword was not added. The only file in this task's change set that gained `override` was test/unit/http_resource_test.cpp (line 35). All ~95 render_* definitions in test/integ/basic.cpp, test/integ/authentication.cpp, and most other test/integ files lack `override`, making it inconsistent. The missing keyword silently allows typos to create new virtual functions rather than overriding the base. |
| 35 | + *Recommendation:* Add the `override` specifier to all render_get / render_post / etc. definitions in test files, matching the pattern already established in http_resource_test.cpp. This is purely a consistency and safety improvement — no behavior change. |
| 36 | + |
| 37 | +8. [ ] **housekeeper** | `specs/tasks/M4-handlers/TASK-022.md:37` | documentation-stale |
| 38 | + Related Decisions references '§3.7, §4.4' using section-number notation. §4.4 is now a split-directory file at specs/architecture/04-components/http-resource.md. The reference is still navigable but uses an older section-number convention rather than a file path link, consistent with other tasks in the same milestone. |
| 39 | + *Recommendation:* No immediate action required; section-number references are used consistently across M4 task files and remain resolvable. Can be normalized to file-path links in a future documentation sweep. |
| 40 | + |
| 41 | +9. [ ] **performance-reviewer** | `src/webserver.cpp:1594` | algorithmic-complexity |
| 42 | + The elif chain that resolves HTTP method strings to function-pointer+enum is O(n) over nine branches on every request. This predates the task and is unchanged, but the rename is a natural moment to note it. GET is checked first (most common), which is the only ordering optimization present. |
| 43 | + *Recommendation:* Not a regression introduced by this task. A future optimisation could use a flat lookup table keyed on the first character or a minimal perfect hash, but the chain is short (9 entries) and each strcmp is on a handful of bytes, so the practical cost per request is negligible. Leave as-is unless profiling shows this path as a hot spot. |
| 44 | + |
| 45 | +10. [ ] **spec-alignment-checker** | `/Users/etr/progs/libhttpserver/.worktrees/TASK-022/src/httpserver/http_resource.hpp:39` | specification-gap |
| 46 | + The detail helper `detail::empty_render(const http_request&)` returns `std::shared_ptr<http_response>` (pointer-based). This is the existing return type for all nine `render_*` virtuals and aligns with the deferred TASK-036 scope per the clarified spec. No action needed for TASK-022, but the pointer return is noted as a known carry-forward. |
| 47 | + *Recommendation:* No change needed in TASK-022 scope. TASK-036 will address the return-type change to http_response by value. |
| 48 | + |
| 49 | +11. [ ] **spec-alignment-checker** | `specs/tasks/M4-handlers/TASK-022.md:39` | specification-gap |
| 50 | + Task status field in TASK-022.md was not updated to 'Done' after implementation. The _index.md also still shows 'Not Started' for TASK-022 (line 107 of specs/tasks/_index.md). This is a bookkeeping omission — the status reflects the state before the branch was worked. |
| 51 | + *Recommendation:* Update 'Status: Not Started' to 'Status: Done' in both specs/tasks/M4-handlers/TASK-022.md and specs/tasks/_index.md. |
| 52 | + |
| 53 | +12. [ ] **test-quality-reviewer** | `test/integ/basic.cpp:3041` | naming-convention |
| 54 | + Comment inside `all_methods_fallthrough_to_render` reads "not render_get/POST/etc." — the POST part is still uppercase, inconsistent with the renamed snake_case convention applied everywhere else in this change. |
| 55 | + *Recommendation:* Change the comment to "not render_get/render_post/etc." to be consistent with the new naming convention. |
| 56 | + |
| 57 | +13. [ ] **test-quality-reviewer** | `test/integ/basic.cpp:778` | missing-test |
| 58 | + The CONNECT method block in both `complete` (line 774-783) and `only_render` (line 838-849) tests is commented out with no substitute. `complete_test_resource` defines `render_connect` but no integration test ever exercises the `render_connect` dispatch path end-to-end through the HTTP stack. The renamed virtual `render_connect` in the header and its dispatch wiring in `webserver.cpp` are therefore untested at the integration level. |
| 59 | + *Recommendation:* Either add a CONNECT dispatch integration test (even a minimal one using `CURLOPT_CUSTOMREQUEST, "CONNECT"` pointed at a local resource) or add a comment explicitly recording that CONNECT is intentionally excluded and why (e.g. libcurl tunneling semantics make it impractical). This is a pre-existing gap that the rename did not fix. |
0 commit comments