TASK-075: persist 19 unworked review findings

1 major (test-quality-reviewer flags scripts/test_check_server_ready_helper.sh
as unreachable from `make check` because it is neither in EXTRA_DIST nor
wired into a check-local target) and 18 minor findings deferred for
future cleanup. See specs/unworked_review_issues/2026-06-09_103302_task-075.md.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Author: etr

specs/unworked_review_issues/2026-06-09_103302_task-075.md

@@ -0,0 +1,85 @@
+# Unworked Review Issues
+
+**Run:** 2026-06-09 10:33:02
+**Task:** TASK-075
+**Total:** 19 (0 critical, 1 major, 18 minor)
+
+## Major
+
+1. [ ] **test-quality-reviewer** | `scripts/test_check_server_ready_helper.sh:1` | missing-test
+   The bash self-test for check-server-ready-helper.sh is not wired into `make check`, not listed in EXTRA_DIST, and not referenced anywhere in Makefile.am. The lint guard (check-server-ready-helper.sh) IS properly wired via check-local, but the test that validates the guard itself (test_check_server_ready_helper.sh) is unreachable from any automated run. It will not be executed in CI or by any developer running `make check`, making its five cases dead as regression protection.
+   *Recommendation:* Add `scripts/test_check_server_ready_helper.sh` to EXTRA_DIST and add a `lint-server-ready-helper-selftest` target (calling `bash $(top_srcdir)/scripts/test_check_server_ready_helper.sh`) wired into check-local alongside lint-server-ready-helper. This mirrors the pattern presumably used for other self-tested lint scripts in the project.
+
+## Minor
+
+2. [ ] **architecture-alignment-checker** | `specs/architecture/09-testing.md:null` | pattern-violation
+   The architecture testing doc (section 9) enumerates the test surfaces requiring first-class coverage but does not mention the shared test-helper convention established by test/integ/test_utils.hpp, curl_helpers.hpp, and now server_ready.hpp, nor the lint-gate pattern for enforcing helper adoption (lint-server-ready-helper, check-warning-suppressions, etc.). The grep regression guard introduced by TASK-075 is operationally significant enough that future contributors adding hooks_*.cpp files benefit from knowing this contract exists.
+   *Recommendation:* Add a brief note to section 9 describing the shared-helper pattern in test/integ/ (server_ready.hpp, curl_helpers.hpp, test_utils.hpp) and the associated lint gates that enforce their use, so new contributors know where helpers live and that bare sleep_for calls in hooks tests will break CI.
+
+3. [ ] **code-quality-reviewer** | `scripts/check-server-ready-helper.sh:57` | code-readability
+   The awk script inside the while-loop re-reads the entire file from the beginning for every matching candidate line (NR==target with the full lines[] array built each time). For test files with many candidate lines this is an O(n*k) pass where n=file-lines and k=matching-lines. In practice hooks_*.cpp files are small and this is not a real bottleneck, but the pattern is surprising for a reader expecting a single-pass approach.
+   *Recommendation:* Document with a brief comment that the O(n*k) cost is acceptable given that test files are short, or restructure awk to do a single pass over the file collecting all candidates and their preceding non-blank lines.
+
+4. [ ] **code-quality-reviewer** | `scripts/test_check_server_ready_helper.sh:66` | test-coverage
+   The test suite covers the allow-marker on the previous non-blank line (Test 4) but does not test the case where the marker is on the same line as the sleep_for call (the 'NON-READINESS-SLEEP' marker inline on the same line, which is the first branch in the awk NR==target block). Both branches in the detection logic exist and are tested by the marker-on-previous-line case, but the same-line marker path is not independently exercised.
+   *Recommendation:* Add a Test 4b fixture where the allow-marker appears on the same line as the sleep_for to independently pin both allow-marker positions.
+
+5. [ ] **code-quality-reviewer** | `test/integ/server_ready.hpp:9` | code-readability
+   The block comment in server_ready.hpp references two plan documents ('specs/tasks/M7-v2-cleanup/TASK-075.md' and '.groundwork-plans/TASK-075-plan.md'). The second path (.groundwork-plans/TASK-075-plan.md) is a planning artefact that may not survive long-term in the repository, making the reference potentially stale for future maintainers.
+   *Recommendation:* Drop or shorten the reference to the groundwork plan file; the task-spec reference alone is sufficient context.
+
+6. [ ] **code-simplifier** | `scripts/check-server-ready-helper.sh:53` | patterns
+   The `${WATCHED_FILES[@]+"${WATCHED_FILES[@]}"}` idiom for safe empty-array expansion is correct but unusual enough that the long inline comment is necessary to explain it. An alternative is to guard with `[ ${#WATCHED_FILES[@]} -eq 0 ] && exit 0` before the loop, which is self-explanatory and eliminates the need for the comment entirely.
+   *Recommendation:* Replace the `${arr[@]+"${arr[@]}"}` idiom with an explicit early-exit guard: `if [ ${#WATCHED_FILES[@]} -eq 0 ]; then echo '...'; exit 0; fi` — this removes the need for the explanatory comment and uses a pattern any bash author will recognise immediately.
+
+7. [ ] **code-simplifier** | `scripts/check-server-ready-helper.sh:57` | naming
+   The `while IFS=: read -r lineno _` loop uses `_` as a throwaway variable for the rest of the grep output, which is idiomatic in Python but less obvious in bash. The variable name `rest` or simply discarding into a named variable would be marginally clearer, though `_` is widely used in bash too. More notably, the outer loop variable is named `file` while the inner extraction variable is `lineno`, which is fine, but the `allowed` variable computed by `awk` is re-declared on every iteration of the inner loop. Extracting the awk script to a named function would reduce cognitive overhead when reading the script.
+   *Recommendation:* Minor: consider renaming the awk subshell result variable from `allowed` to `is_allowed` for clarity, or leave as-is — this is a style note only.
+
+8. [ ] **code-simplifier** | `scripts/test_check_server_ready_helper.sh:46` | code-structure
+   Tests 3, 4, and 5 each do `rm -f "$FAKE_REPO/test/integ"/*.cpp` before writing a new fixture file. This repeated teardown-then-setup pattern is error-prone if a glob matches no files (benign here due to `|| true` semantics of `-f`, but it could silently leave stale fixtures if a test name ever changes). A helper function like `reset_integ_dir` encapsulating the rm + mkdir -p would make each test case's setup intent explicit and eliminate the repetition.
+   *Recommendation:* Extract `rm -f "$FAKE_REPO/test/integ"/*.cpp` into a `reset_fixtures` helper called at the top of each test block.
+
+9. [ ] **code-simplifier** | `test/integ/server_ready.hpp:69` | code-structure
+   The loop rebuilds `url` as a local `const std::string` inside the while-condition scope but `url` is actually declared outside the loop (line 67-68). The loop body allocates and destroys a CURL handle on every iteration, which is intentional, but constructing `url` could be moved before the while-loop to make the split between one-time setup and per-iteration work visually clearer. Currently `url` is already outside the loop (correct), but the variable declaration sits between `end` and the loop, making it slightly harder to spot the per-iteration vs. one-time split at a glance. A blank line separating one-time setup from the loop body would aid readability.
+   *Recommendation:* Add a blank line between the one-time setup block (`end`, `url`) and the `while` loop to separate the two conceptual phases.
+
+10. [ ] **housekeeper** | `specs/architecture/09-testing.md:null` | documentation-stale
+   The new shared helper test/integ/server_ready.hpp and the lint gate lint-server-ready-helper (wired into check-local) are not mentioned in the testing architecture doc. The task itself says 'Related Decisions: None new', and the existing lint scripts (check-warning-suppressions, check-complexity, etc.) are similarly absent from the architecture docs, so this follows the established project convention. No other lint gate appears in the architecture docs.
+   *Recommendation:* Optionally add a bullet to specs/architecture/09-testing.md noting the server_ready.hpp shared helper pattern and the lint-server-ready-helper gate, for parity with the existing check-doxygen.sh mention in specs/architecture/13-documentation.md. Not blocking.
+
+11. [ ] **performance-reviewer** | `test/integ/server_ready.hpp:70` | memory-allocation
+   curl_easy_init() / curl_easy_cleanup() are called inside the polling loop on every iteration. In the happy path (server up on first try) this is a single allocation, which is fine. In the worst case on a loaded CI runner (~28 iterations before the 3000 ms deadline) the loop performs 28 back-to-back handle alloc/free cycles. Additionally, none of the hooks_*.cpp callers invoke curl_global_init before calling wait_for_server_ready, so libcurl falls back to its implicit auto-init path, which itself is not thread-safe if another curl handle happens to be in use concurrently in the same test binary.
+   *Recommendation:* Initialize the CURL handle once before the loop and reuse it across iterations, cleaning up after the loop exits (success or timeout). This also sidesteps the implicit-init concern: if callers already call curl_global_init in their test harness (as the non-hooks tests do), hoisting the handle out of the loop is sufficient. Example: CURL* curl = curl_easy_init(); if (!curl) return; curl_easy_setopt(curl, CURLOPT_URL, url.c_str()); curl_easy_setopt(curl, CURLOPT_CONNECT_ONLY, 1L); curl_easy_setopt(curl, CURLOPT_CONNECTTIMEOUT_MS, 100L); while (clock::now() < end) { CURLcode rc = curl_easy_perform(curl); if (rc != CURLE_COULDNT_CONNECT) { curl_easy_cleanup(curl); return; } std::this_thread::sleep_for(5ms); } curl_easy_cleanup(curl);
+
+12. [ ] **security-reviewer** | `scripts/check-server-ready-helper.sh:57` | input-validation
+   The inner while-loop reads lineno from grep -n output using IFS=: read -r lineno _. If a matched file path itself contains a colon (e.g. a path like test/integ/hooks_foo:bar.cpp), IFS splitting would mis-assign the line-number field and cause the awk invocation to silently pass any violation on that line. find output is also not NUL-delimited. In practice, the glob hooks_*.cpp makes a colon-in-filename nearly impossible, but the pattern is worth noting.
+   *Recommendation:* Use grep --line-number with a delimiter that cannot appear in a line number, or use grep -n and split only on the first colon with read -r lineno rest <<< "${line#*:}". For robustness against unusual CI filesystems, consider using find -print0 | mapfile -d '' to populate WATCHED_FILES.
+
+13. [ ] **security-reviewer** | `scripts/test_check_server_ready_helper.sh:52` | insecure-design
+   Glob expansion rm -f "$FAKE_REPO/test/integ"/*.cpp: if the glob matches no files (already-empty directory), bash under set -euo pipefail with nullglob unset will pass the literal string /*.cpp to rm. With the -f flag this is harmless (rm -f silently ignores non-existent files), but the pattern can surprise if -f is ever removed. The TMPDIR_BASE is created with mktemp -d and cleaned up via EXIT trap, which is correct; no TOCTOU risk.
+   *Recommendation:* Prefer explicit removal of known fixture files (rm -f "$FAKE_REPO/test/integ/hooks_bare.cpp" etc.) rather than a glob, or set the nullglob option before each glob-based rm.
+
+14. [ ] **security-reviewer** | `test/integ/server_ready.hpp:71` | error-handling
+   Silent bail-out on curl_easy_init() failure: if libcurl cannot allocate a handle the function returns immediately without waiting, silently treating an allocation failure as 'server ready'. This could cause tests to run before the server is actually listening, producing false-positive test passes rather than a detectable error.
+   *Recommendation:* Either loop and retry on nullptr (up to the deadline), or abort with a fatal test assertion: if (curl == nullptr) { /* LT_FAIL or throw */ return; }. Returning silently conflates a resource exhaustion condition with successful server startup.
+
+15. [ ] **spec-alignment-checker** | `/Users/etr/progs/libhttpserver/.worktrees/TASK-075/scripts/test_check_server_ready_helper.sh:null` | specification-gap
+   The gate self-test script (scripts/test_check_server_ready_helper.sh) is tracked in git and executable, but is not referenced in EXTRA_DIST and is not wired into any make target or CI step. The peer self-test scripts/test_check_warning_suppressions.sh has the same gap, so this is a pre-existing pattern rather than a regression introduced by TASK-075. The task action items and acceptance criteria do not require the self-test to be wired in, but leaving it unwired means it can silently drift out of alignment with the gate it covers.
+   *Recommendation:* Wire scripts/test_check_server_ready_helper.sh into lint-server-ready-helper (or a separate lint-server-ready-helper-self-test target) and add both test_check_*.sh scripts to EXTRA_DIST for completeness. This is consistent housekeeping rather than a task requirement.
+
+16. [ ] **spec-alignment-checker** | `/Users/etr/progs/libhttpserver/.worktrees/TASK-075/test/integ/server_ready.hpp:71` | specification-gap
+   The shared helper silently returns (without waiting at all) when curl_easy_init() returns nullptr. This was a known issue in the original hooks_not_found_alias_test.cpp implementation (documented in unworked review issue specs/unworked_review_issues/2026-06-07_232548_task-071.md), and the same silent-return behavior was carried into the new shared header (line 71: `if (curl == nullptr) return;`). On a resource-exhausted host the helper exits immediately and the calling test proceeds as if the server is ready, silently removing the reliability guarantee the helper was introduced to provide.
+   *Recommendation:* Add a brief fallback sleep (e.g., std::this_thread::sleep_for(std::chrono::milliseconds(200)); return;) when curl_easy_init() returns nullptr so that even on allocation failure the caller gets some delay. Alternatively, log or assert to make the failure diagnosable.
+
+17. [ ] **test-quality-reviewer** | `scripts/check-server-ready-helper.sh:85` | missing-test
+   The grep pattern `^[[:space:]]*std::this_thread::sleep_for\(([^)]*ms\)|std::chrono::milliseconds)` only catches milliseconds-unit sleeps (literal `50ms` UDL and `std::chrono::milliseconds(...)`). A developer could regress to `std::this_thread::sleep_for(std::chrono::microseconds(50000))` or `std::this_thread::sleep_for(std::chrono::seconds(1))` as a server-ready wait, and the guard would silently pass. The bash self-test (cases 3 and 4) only exercises the milliseconds form, so the gap is also untested.
+   *Recommendation:* Broaden the grep pattern to catch any `std::this_thread::sleep_for(` call regardless of duration unit, e.g. `^[[:space:]]*std::this_thread::sleep_for\(`. The NON-READINESS-SLEEP marker escape hatch handles legitimate non-readiness uses, so a blanket catch is safe. Add a test-5b case to the bash self-test covering a `std::chrono::seconds(1)` form.
+
+18. [ ] **test-quality-reviewer** | `test/integ/server_ready.hpp:61` | implementation-coupling
+   The function signature takes a raw `int port` rather than a `const webserver&` (the original task description shows `wait_for_server_ready(ws)` in the goal). Several call sites pass the hard-coded `PORT` macro, meaning a test that uses a different port (or changes its port) must manually keep the macro and the call site in sync. The design decision is recorded in the header comments as intentional, but callers passing `PORT` defeat the refactoring's stated goal of obtaining the port from the webserver object itself.
+   *Recommendation:* This is a minor coupling smell; the task description's `wait_for_server_ready(ws)` API would be more robust. If the webserver object exposes a `port()` accessor (or similar), prefer an overload that takes a `const webserver&` to avoid the macro-sync hazard. Not blocking.
+
+19. [ ] **test-quality-reviewer** | `test/integ/server_ready.hpp:78` | non-deterministic
+   When `curl_easy_init()` returns `nullptr` (OOM), the helper returns immediately without waiting, even if the server is not yet listening. This silently downgrades the helper to the old racy behavior — the test proceeds with no readiness guarantee. The header comment documents `if (curl == nullptr) return` but does not explain why an uninitialised curl library is treated as "ready". On a loaded CI runner where libcurl is under memory pressure this will reintroduce intermittent connection failures.
+   *Recommendation:* On `curl_easy_init()` returning `nullptr`, either sleep `5ms` and retry on the next loop iteration (preserving the timeout bound), or fall back to a single fixed sleep of the full deadline. Both are preferable to silently treating OOM as "server is ready".