|
| 1 | +# Unworked Review Issues |
| 2 | + |
| 3 | +**Run:** 2026-06-22 14:08:15 |
| 4 | +**Task:** TASK-082 |
| 5 | +**Total:** 18 (0 critical, 0 major, 18 minor) |
| 6 | + |
| 7 | +## Minor |
| 8 | + |
| 9 | +1. [ ] **architecture-alignment-checker** | `test/unit/http_resource_test.cpp:58` | pattern-violation |
| 10 | + The per-lane size table uses '~104' (approximate) for libstdc++ lanes rather than exact observed values. The TASK-082 acceptance criteria require 'Both gates are static_asserts at observed + 16 (or tighter) on every CI lane' with a per-lane table. Using approximations weakens the documentation contract slightly — future maintainers cannot tell whether 104 was the exact measurement or a rounded estimate, which matters when deciding how much headroom remains before needing a table update. |
| 11 | + *Recommendation:* Replace '~104' with exact byte counts obtained from each CI lane's compile log (e.g., 104, 112, or whatever the probing static_assert reported). This matches the procedure described in the comment itself and satisfies the acceptance criterion that reads 'The gate's comment carries the per-lane size table.' |
| 12 | + |
| 13 | +2. [ ] **code-quality-reviewer** | `test/unit/http_resource_test.cpp:154` | code-readability |
| 14 | + The `set_up` and `tear_down` methods in `http_resource_suite` are empty stubs that add visual noise without serving any purpose. They are present in the file but do nothing. |
| 15 | + *Recommendation:* Remove the empty `set_up` and `tear_down` bodies (lines 154-158) unless the test framework requires their presence. If the framework requires them, add a brief comment explaining why (e.g., `// required by LT_BEGIN_SUITE macro`). |
| 16 | + |
| 17 | +3. [ ] **code-quality-reviewer** | `test/unit/http_resource_test.cpp:220` | test-coverage |
| 18 | + The test `default_render_returns_sentinel` checks `render()` and `render_get()` but does not check `render_post()`, `render_put()`, etc. The comment says 'render_get / render_post / etc. forward to render(), so they also return the -1 sentinel by default' but does not verify it. This assertion is made in a comment but not in executable test code. |
| 19 | + *Recommendation:* Add checks for at least one or two additional verb-specific render methods (e.g., `render_post`, `render_delete`) within the same test or in a companion test to confirm the forwarding chain is intact and not accidentally broken for a specific verb. |
| 20 | + |
| 21 | +4. [ ] **code-quality-reviewer** | `test/unit/http_resource_test.cpp:304` | test-coverage |
| 22 | + The test `set_allowing_multiple_times` contains six assertions in a single test case, violating the clean-code one-assert-per-test principle. While each assertion is logically related to toggling a flag, a failure in assertion 3 masks whether assertions 4-6 also fail, reducing diagnostic clarity. |
| 23 | + *Recommendation:* Split into two or three focused test cases: one for the toggle round-trip (false → true), one for idempotent false (double-false stays false). This mirrors the pattern used in `set_allowing_disable` which is already appropriately sized. |
| 24 | + |
| 25 | +5. [ ] **code-quality-reviewer** | `test/unit/http_resource_test.cpp:45` | code-readability |
| 26 | + The opening comment block for the sizeof gate (lines 45-86) is exceptionally long at 42 lines — longer than some of the test functions themselves. The re-measurement procedure (steps 1-3 on lines 76-80) duplicates the same procedure already described in the same comment for the other tripwire file. The comment quality is excellent, but the duplication of the procedure text across both files creates a maintenance risk: if the procedure changes, both files must be updated. |
| 27 | + *Recommendation:* Consider extracting the re-measurement procedure into a shared file (e.g., `test/unit/SIZE_GATE_PROCEDURE.txt` or a comment in a shared header) and referencing it from both gate comments with a one-liner like `// Re-measurement procedure: see SIZE_GATE_PROCEDURE.txt`. This reduces the comment block length and eliminates the duplicated maintenance surface. |
| 28 | + |
| 29 | +6. [ ] **code-quality-reviewer** | `test/unit/http_resource_test.cpp:59` | code-readability |
| 30 | + The ubuntu/clang and windows CI-lane rows in the observed-sizes table use '~104' (approximate). If these values were actually measured, recording the exact byte counts (e.g., 104 exactly) improves the table's precision and makes a future re-measurement trivially comparable. If they are genuinely approximate because the lanes were not measured exactly, a brief note ('not directly measured; inferred from libstdc++ ABI') would clarify the approximation's source. |
| 31 | + *Recommendation:* Either pin the exact observed value per lane, or add an inline note explaining why the tilde is present (e.g., 'inferred, not directly measured on this lane'). |
| 32 | + |
| 33 | +7. [ ] **code-quality-reviewer** | `test/unit/webserver_pimpl_test.cpp:62` | code-readability |
| 34 | + Same issue as finding #2: the ubuntu/clang and windows lanes in the webserver observed-sizes table use '~848' approximations without explaining whether these are measured or inferred from the libstdc++ ABI. |
| 35 | + *Recommendation:* Consistent with the http_resource_test comment, either record exact values or annotate with 'inferred from libstdc++ std::string=32 ABI' so a maintainer knows whether to re-measure all lanes or just the annotated ones. |
| 36 | + |
| 37 | +8. [ ] **code-quality-reviewer** | `test/unit/webserver_pimpl_test.cpp:86` | code-readability |
| 38 | + The lower-bound static_assert at line 86 (sizeof(webserver) >= sizeof(void*)) lost its original explanatory clause 'Also documents that the PIMPL split had a real structural effect (the post-split size is strictly smaller than the 1600-byte baseline).' The removed clause carried useful historical rationale that was separate from the surviving sentence. Its deletion is not wrong but does reduce context for a future reader who wants to know why a lower-bound guard exists at all. |
| 39 | + *Recommendation:* Consider appending a brief sentence like: 'The post-PIMPL-split layout should be substantially smaller than the pre-split ~1600-byte baseline.' This preserves the historical anchor without reinstating the deleted upper-bound assert. |
| 40 | + |
| 41 | +9. [ ] **code-quality-reviewer** | `test/unit/webserver_pimpl_test.cpp:86` | code-elegance |
| 42 | + The lower-bound assert on line 86 (`sizeof(httpserver::webserver) >= sizeof(void*)`) is a useful defensive check, but it cannot realistically fire given that the upper-bound assert already proves the size is at most 864 bytes. The comment describes the failure scenario but it is only possible if the upper bound is also relaxed. This is not harmful but is slightly superfluous. |
| 43 | + *Recommendation:* Keep the lower-bound assert as documentation of intent (it is cheap and self-documenting), but consider adding a note that it is a sanity guard complementary to the upper bound, not an independent trip condition. |
| 44 | + |
| 45 | +10. [ ] **code-simplifier** | `test/unit/http_resource_test.cpp:154` | naming |
| 46 | + The set_up() and tear_down() methods in the test suite are empty. While this may be required by the LittleTest framework, empty bodies with no comment explaining why they are kept add noise. |
| 47 | + *Recommendation:* If the framework requires these stubs, add a brief comment like '// required by LittleTest framework' or remove if the framework allows omission. |
| 48 | + |
| 49 | +11. [ ] **code-simplifier** | `test/unit/http_resource_test.cpp:45` | naming |
| 50 | + The block comment above the static_assert opens with 'sizeof(http_resource) tripwire (TASK-082)' and immediately explains it is a bystander gate, but then the final sentence of the opening paragraph repeats 'The authoritative v1-anchored static_assert lives in test/bench_sizeof_http_resource.cpp; this one is the day-to-day tripwire that runs as part of `make check`.' The phrase 'day-to-day tripwire' is introduced twice: once in the opening sentence and again at the end of that same paragraph, making the reader parse the same idea twice. |
| 51 | + *Recommendation:* Remove the trailing restatement at line 49-50. Keep only 'This is a bystander gate: any new field on http_resource breaks the build until the maintainer rolls the threshold and records the new size in the table below.' The reference to bench_sizeof_http_resource.cpp is the only unique information worth preserving, so the sentence can become: 'The authoritative v1-anchored gate lives in test/bench_sizeof_http_resource.cpp.' |
| 52 | + |
| 53 | +12. [ ] **code-simplifier** | `test/unit/http_resource_test.cpp:45` | code-structure |
| 54 | + The sizeof tripwire comment block (lines 45-86) is 41 lines for a single static_assert. The re-measurement procedure (steps 1-3) is repeated nearly verbatim in webserver_pimpl_test.cpp (lines 68-74), creating maintained duplication. If the procedure ever changes, both files need updating. |
| 55 | + *Recommendation:* Extract the re-measurement steps into a shared comment header file (e.g., test/unit/sizeof_gate_procedure.h) included by both files, or simply reference a single canonical location. Alternatively, the three-step procedure could live only in CONTRIBUTING or a CI runbook and the comment could just say 'see docs/sizeof-gate-procedure.md for the re-measurement procedure'. |
| 56 | + |
| 57 | +13. [ ] **code-simplifier** | `test/unit/webserver_pimpl_test.cpp:86` | code-structure |
| 58 | + The lower-bound static_assert at line 86 ('webserver is suspiciously small — impl_ pointer may be missing') uses an em dash inside the string literal, which is a multi-byte Unicode character. All other static_assert messages in both files use only ASCII. This inconsistency may cause surprises on toolchains with restricted character set settings. |
| 59 | + *Recommendation:* Replace the em dash with an ASCII hyphen-minus or a colon: 'webserver is suspiciously small: impl_ pointer may be missing' to stay consistent with the ASCII-only style used everywhere else in these files. |
| 60 | + |
| 61 | +14. [ ] **security-reviewer** | `test/unit/http_resource_test.cpp:83` | logging |
| 62 | + The sizeof gate comment instructs maintainers to use a 'probe static_assert(sizeof(...) == 0, ...)' technique to extract sizes from CI compile logs. This leaks internal struct layout details into CI build logs, which depending on CI log retention and access control policies could expose internal memory layout information to anyone with log access. This is low-risk in a FOSS project but worth noting for proprietary deployments. |
| 63 | + *Recommendation:* No immediate action required for an open-source library. If this pattern is ever adopted in a proprietary context, consider using a separate measurement binary that prints sizes at test runtime rather than embedding the probe in CI logs. |
| 64 | + |
| 65 | +15. [ ] **spec-alignment-checker** | `test/unit/http_resource_test.cpp:58` | acceptance-criteria |
| 66 | + The per-lane size table lists ubuntu/clang and Windows lanes as '~104' (with tilde indicating approximation) rather than exact observed values. The acceptance criterion requires the 'gates' comment to carry the per-lane size table; approximate values are arguably acceptable but technically do not constitute a fully-locked table as the task description implies ('locked at the value of max(observed)'). |
| 67 | + *Recommendation:* If exact values are available from CI runs, replace '~104' with the actual integer. If the lanes genuinely produce different values within that range, add a note explaining the variance. Otherwise this is low-risk as the gate threshold (248) still dominates. |
| 68 | + |
| 69 | +16. [ ] **spec-alignment-checker** | `test/unit/webserver_pimpl_test.cpp:61` | acceptance-criteria |
| 70 | + Similarly, the webserver per-lane table records ubuntu/clang and Windows lanes as '~848' (approximations). The task action item says to 'capture in a comment table' based on actual measurements. Using '~848' rather than exact values slightly weakens the table's usefulness as an audit record. |
| 71 | + *Recommendation:* Same as above — replace with exact figures if available, or add a brief note about why values vary within a few bytes across those lanes. |
| 72 | + |
| 73 | +17. [ ] **test-quality-reviewer** | `/Users/etr/progs/libhttpserver/.worktrees/TASK-082/test/unit/http_resource_test.cpp:220` | multiple-concerns |
| 74 | + The test `default_render_returns_sentinel` checks both `er.render(req)` and `er.render_get(req)` in a single test body. These are two independent behaviors; bundling them makes the test name imprecise and the failure message ambiguous. |
| 75 | + *Recommendation:* Extract `render_get` returns sentinel into its own `LT_BEGIN_AUTO_TEST` named `default_render_get_returns_sentinel` so each test is a single assertion unit. |
| 76 | + |
| 77 | +18. [ ] **test-quality-reviewer** | `/Users/etr/progs/libhttpserver/.worktrees/TASK-082/test/unit/http_resource_test.cpp:282` | redundant-test |
| 78 | + `is_allowed_known_methods` (line 282-293) verifies all nine methods are allowed on a freshly constructed `simple_resource`, which is the exact same assertion set as the second half of `allow_all_methods` (line 181-194) and overlaps substantially with `render_only_resource_methods_allowed` (line 245-257). These three tests exercise the same code path — the default method-set — without covering any distinct scenario. |
| 79 | + *Recommendation:* Keep `is_allowed_known_methods` as the canonical default-state test and delete or narrow `render_only_resource_methods_allowed` to cover only what is unique about `render_only_resource` (i.e., that `render()` dispatches correctly), not that all methods are allowed by default. |
0 commit comments