|
| 1 | +# Unworked Review Issues |
| 2 | + |
| 3 | +**Run:** 2026-06-08 13:36:58 |
| 4 | +**Task:** TASK-073 |
| 5 | +**Total:** 12 (0 critical, 0 major, 12 minor) |
| 6 | + |
| 7 | +## Minor |
| 8 | + |
| 9 | +1. [ ] **architecture-alignment-checker** | `src/detail/webserver_callbacks.cpp:346` | pattern-violation |
| 10 | + The comment states 'URL path: base_unescaper() in webserver_request.cpp' but base_unescaper is called on the full raw URL string from MHD (the local variable t_url at line 657 of webserver_request.cpp), not solely the path component. The URL includes the query string portion at that point; the path is extracted only after standardize_url. The claim is directionally correct but the term 'URL path' is slightly imprecise — 'URL' or 'request URL' would be more accurate. |
| 11 | + *Recommendation:* Change '- URL path: base_unescaper() in webserver_request.cpp' to '- request URL: base_unescaper() in webserver_request.cpp (answer_to_connection, line ~657)' to match the actual scope of the call site. |
| 12 | + |
| 13 | +2. [ ] **architecture-alignment-checker** | `src/detail/webserver_request.cpp:115` | pattern-violation |
| 14 | + Pre-existing (out-of-diff) comments in webserver_request.cpp at lines 114-116 and 153-164 describe 'MHD's unescaper_func' as if it performs decoding ('must receive an already-unescaped path', 'request path produced by MHD's unescaper'). After TASK-073, the registered unescaper_func is explicitly a no-op — MHD performs no decoding; libhttpserver performs all decoding itself via base_unescaper. These pre-existing comments are now misleading and contradict the architectural story documented by the TASK-073 comment. This finding is out-of-diff but was surfaced by the TASK-073 changes that make the no-op contract explicit. |
| 15 | + *Recommendation:* In a follow-up, update the normalize_auth_skip_paths comment block (webserver_request.cpp lines 114-116 and 153-164) to say 'libhttpserver's base_unescaper() runs' rather than 'MHD's unescaper_func runs', to be consistent with the architectural decision now documented in webserver_callbacks.cpp. |
| 16 | + |
| 17 | +3. [ ] **code-quality-reviewer** | `specs/tasks/M7-v2-cleanup/TASK-073.md:13` | task-values |
| 18 | + The task spec's third action item offered two branches: (a) remove the workaround and add a version guard, or (b) leave it and document. The implemented path chose (b) — document — but also retained the no-op code without a '#if MHD_VERSION < 0x00096400' guard. The task accepts 'add a static guard only if we want belt-and-braces', so omitting it is within scope; however the task definition is now marked Complete without explicitly recording which sub-option was chosen. |
| 19 | + *Recommendation:* The decision is implicit in the comment prose ('the no-op stays for the architectural reasons above') which is sufficient for a documentation task. No code change needed, but future readers would benefit from a one-liner in the task spec closing the loop on why the guard was not added. |
| 20 | + |
| 21 | +4. [ ] **code-quality-reviewer** | `specs/tasks/M7-v2-cleanup/TASK-073.md:30` | task-values |
| 22 | + The task status field says `Complete` rather than `Done`, which is the convention used in the `_index.md` task table and other completed task files in this milestone. |
| 23 | + *Recommendation:* Change `Status: Complete` to `Status: Done` for consistency with the index and peer task files. |
| 24 | + |
| 25 | +5. [ ] **code-quality-reviewer** | `src/detail/webserver_callbacks.cpp:295` | code-readability |
| 26 | + There are two separate `namespace detail {` blocks reopened at lines 295 and 373 within the same translation unit (the first closes at line 293, reopens at 295). This fragmentation is unusual and makes the file harder to scan. |
| 27 | + *Recommendation:* Merge the three `namespace detail` blocks into one contiguous block, or add a brief comment explaining that the splits are intentional (e.g., due to ordering requirements with the `using` declarations). |
| 28 | + |
| 29 | +6. [ ] **code-quality-reviewer** | `src/detail/webserver_callbacks.cpp:338` | code-readability |
| 30 | + The `unescaper_func` comment block (lines 344-368) is thorough and accurate but quite long for an inline function comment — it mixes the current architectural rationale with a historical note that could be in a separate section header. |
| 31 | + *Recommendation:* Consider splitting the historical note into a distinct `// Historical note:` paragraph already present, or pruning to a one-liner cross-reference to the TASK-073 commit message, keeping the inline comment focused on the invariant that callers must understand. |
| 32 | + |
| 33 | +7. [ ] **code-quality-reviewer** | `src/detail/webserver_callbacks.cpp:362` | code-readability |
| 34 | + The comment cites 'libmicrohttpd 0.9.64' as where the NUL-embedding bug was resolved, but does not provide a direct upstream URL (issue tracker, commit hash, or ChangeLog file path) that a future reader could follow. The task spec action item says 'Link the upstream commit/release in the commit message', which addresses this partially, but the comment itself would benefit from a pointer for long-term auditability. |
| 35 | + *Recommendation:* Add a parenthetical URL such as '(https://git.gnunet.org/libmicrohttpd.git/log/)' or the specific ChangeLog URL next to the 0.9.64 citation so future maintainers can verify the claim without consulting the commit message history. |
| 36 | + |
| 37 | +8. [ ] **code-quality-reviewer** | `src/detail/webserver_callbacks.cpp:365` | code-readability |
| 38 | + The release date for libmicrohttpd 1.0.0 is written as '2024-02' (year-month only). Other date references in the same block use full ISO-8601 format ('2019-06-09', '2019-03-20', etc.). The inconsistency is minor but slightly breaks the pattern established within the same comment. |
| 39 | + *Recommendation:* Expand '2024-02' to a full date such as '2024-02-14' (the actual 1.0.0 release) or qualify it explicitly as 'released 2024-02 (approx.)' to be consistent with the level of precision used for the 0.9.64 dates above. |
| 40 | + |
| 41 | +9. [ ] **code-simplifier** | `src/detail/webserver_callbacks.cpp:338` | code-structure |
| 42 | + unescaper_func comment block (lines 343-368) is detailed and accurate but substantially longer than the function body itself. The historical note (lines 355-366) could be condensed — the key fact (bug fixed in MHD 0.9.64, minimum required is >= 1.0.0) is already stated twice. |
| 43 | + *Recommendation:* Consider collapsing the historical note into a single sentence: 'Historical note: this no-op also defended against a v0.99-era MHD embedded-NUL bug, fixed in 0.9.64 (2019); our configure.ac floor of >= 1.0.0 makes that unreachable.' |
| 44 | + |
| 45 | +10. [ ] **code-simplifier** | `src/detail/webserver_callbacks.cpp:363` | code-structure |
| 46 | + The historical note closes with 'the no-op stays for the architectural reasons above', which restates what the opening paragraph already establishes ('Decoding is performed by libhttpserver itself … This is required so we can honour …'). The closing sentence adds no new information. |
| 47 | + *Recommendation:* Drop the trailing clause. End the historical note at 'so the original v0.99 bug is no longer reachable.' The architectural rationale lives in the opening paragraph and does not need a back-reference. |
| 48 | + |
| 49 | +11. [ ] **spec-alignment-checker** | `src/detail/webserver_callbacks.cpp:354` | acceptance-criteria |
| 50 | + The task's action item for 'if bug is fixed in min-supported version: remove the workaround and add a static #if MHD_VERSION < guard (or equivalent)' was fulfilled by keeping the registration with a rewritten comment rather than removing or adding a version guard. The task explicitly allowed this path ('if the bug is still present in the min-supported version: leave the workaround but add a comment'), and the justification given is architectural (TASK-072 arena routing + user-registered unescaper hook), not that the bug persists. The comment clearly states the bug is fixed in 0.9.64 while configure.ac requires >= 1.0.0, making the original bug-fix path unnecessary. The architectural reason for keeping the no-op is independent and valid, and the comment correctly distinguishes between the two motivations. This is a minor interpretation choice: the task framed the decision as binary (remove vs leave-because-still-needed), but the actual outcome is a third valid path (leave-for-architectural-reasons-not-bug-workaround), which the task's wording supports because 'update the inline comment with verification outcome' is an explicit action item that was completed. |
| 51 | + *Recommendation:* No action required. The comment at lines 344-368 fully documents the decision, names the version cutoff (0.9.64), cites configure.ac's >= 1.0.0 requirement, and explains the independent architectural reason for retention. This satisfies both the acceptance criterion ('decision documented in comment with MHD version reference') and the dependency hygiene NFR (no stale workaround masquerading as a bug fix). |
| 52 | + |
| 53 | +12. [ ] **test-quality-reviewer** | `test/unit/http_request_unescape_arena_test.cpp:279` | missing-test |
| 54 | + The new comment explicitly names the double-decoding prevention invariant ('%2F' must not be decoded twice when a custom MHD unescape callback is registered). The unescape arena unit tests cover the libhttpserver-side decode correctly, but no test asserts that `unescaper_func` itself returns the raw input length without mutating `s` — the specific contract that prevents MHD from pre-decoding behind our back. This is a structural no-op contract that is currently only verified by integration test `custom_unescaper` (line 2179), which does not exercise the `%2F` double-decode case specifically. |
| 55 | + *Recommendation:* Add a small unit test for `webserver_impl::unescaper_func` that (a) passes a percent-encoded string and verifies the return value equals `strlen(s)` and `s` is unmodified, and (b) passes `nullptr` and verifies return 0. This would directly pin the no-op contract the TASK-073 comment describes as the architectural reason the callback exists. |
0 commit comments