TASK-042: record unworked validation findings (2 major, 23 minor)

Captures the multi-agent review output that wasn't addressed in the
validation-fix commit. The two "Major" items map to the changes that
were applied (A5 allow_ip word-boundary anchor; v2-token duplication
between check-readme.sh and check-release-notes.sh — kept duplicated
on purpose, see the comment added above REQUIRED_V2_TOKENS). The
23 "Minor" items are style/polish or low-priority hardening notes
left for a future pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: etr

specs/unworked_review_issues/2026-05-20_102016_task-042.md

@@ -0,0 +1,109 @@
+# Unworked Review Issues
+
+**Run:** 2026-05-20 10:20:16
+**Task:** TASK-042
+**Total:** 25 (0 critical, 2 major, 23 minor)
+
+## Major
+
+1. [ ] **code-quality-reviewer** | `scripts/check-release-notes.sh:217` | test-coverage
+   A5 pair matching for 'allow_ip' has no word-boundary anchor, so the 'disallow_ip' prose line (line 38 of RELEASE_NOTES.md: '`disallow_ip` are removed. Use `block_ip` / `unblock_ip`') already contains 'allow_ip' as a substring and also contains 'block_ip'/'unblock_ip', making it satisfy the allow_ip pair check even if the dedicated allow_ip table row (RELEASE_NOTES.md line 115) were deleted. The grep pattern `(allow_ip.*(block_ip|unblock_ip)|...)` matches any line containing 'disallow_ip' plus a replacement, so the allow_ip pair check is not actually pinned to the allow_ip entry.
+   *Recommendation:* Add a word-boundary anchor to the A5 pair v1 regex: change the pair entry from 'allow_ip;(block_ip|unblock_ip)' to '\ballow_ip\b;(block_ip|unblock_ip)' and update the grep pattern to `grep -qE "(\b${v1}\b.*${v2}|${v2}.*\b${v1}\b)"` so that 'disallow_ip' does not satisfy the 'allow_ip' check.
+
+2. [ ] **code-simplifier** | `scripts/check-release-notes.sh:137` | code-structure
+   REQUIRED_V2_TOKENS is defined identically in both check-readme.sh (lines 91-118) and check-release-notes.sh (lines 108-135). The header comment on line 106 acknowledges this: 'Kept in lock-step with check-readme.sh A3.' Keeping two copies in sync manually is a maintenance hazard: if a new v2 surface is added and one script is updated but not the other, the checks silently diverge.
+   *Recommendation:* Source the shared token list from one canonical location. One approach: define the arrays in a scripts/check-shared-tokens.sh and source it with '. "$(dirname "$0")/check-shared-tokens.sh"' in both check scripts. Another approach (no new file) is to have one script call the other as a library: but sourcing a shared fragment is the standard shell pattern for this.
+
+## Minor
+
+3. [ ] **architecture-alignment-checker** | `Makefile.am:286` | pattern-violation
+   check-release-notes is added to check-local but the new rule does not use the shared staged-install guard pattern used by check-install-layout and check-hygiene. This is intentional and correct for a pure static check (like check-examples and check-readme), but the comment block above check-local (lines ~282-285) says it 'runs check-install-layout and check-hygiene against a single shared staged install' without updating the summary to mention check-release-notes. Minor documentation drift in the Makefile comment.
+   *Recommendation:* Update the comment block above check-local in Makefile.am to list check-release-notes alongside check-headers, check-examples, and check-readme as one of the pre-shared-install static checks, so the comment accurately describes what check-local does.
+
+4. [ ] **architecture-alignment-checker** | `RELEASE_NOTES.md:230` | interface-contract
+   The 'See also' section cross-links to specs/product_specs.md §3.1–§3.7 and ChangeLog, but not to specs/architecture/13-documentation.md which is the architecture deliverable definition for this file. Architecture §13 names RELEASE_NOTES.md as an explicit deliverable; a back-link would help readers trace the requirement lineage. This is not a functional issue.
+   *Recommendation:* Add a cross-link to specs/architecture/13-documentation.md in the 'See also' section of RELEASE_NOTES.md, consistent with the pattern already used for threading (§5.1) and error propagation (§5.2) sections.
+
+5. [ ] **architecture-alignment-checker** | `scripts/check-release-notes.sh:75` | interface-contract
+   A2 checks that HAVE_GNUTLS appears in RELEASE_NOTES.md (it does, in the 'What's gone' section about #ifdef guards). However, HAVE_BAUTH and HAVE_DAUTH are mentioned explicitly in the RELEASE_NOTES.md prose but are absent from REQUIRED_V1_TOKENS in check-release-notes.sh. Architecture §7 (feature-availability) and the PRD document these as first-class feature flags. This is an under-specification in the script's token list, not a bug in the document itself.
+   *Recommendation:* Add '\bHAVE_BAUTH\b' and '\bHAVE_DAUTH\b' to REQUIRED_V1_TOKENS in check-release-notes.sh to match the tokens that RELEASE_NOTES.md already mentions and that architecture §7 identifies as first-class feature flags.
+
+6. [ ] **code-quality-reviewer** | `Makefile.am:39` | code-elegance
+   The EXTRA_DIST line was split mid-entry to add check-release-notes.sh, producing a clean two-line continuation. However, the comment in the Makefile.am target at line 315-320 duplicates the description already given in the script header (A1-A7 checks, 'required v1/v2 tokens, sections, same-line rename pairs, arch citations, disclaimer'). The Makefile comment is slightly less precise than the script header.
+   *Recommendation:* Trim the Makefile comment to a single-line cross-reference like '# TASK-042: static checks on RELEASE_NOTES.md — see scripts/check-release-notes.sh' to avoid the description drifting out of sync with the script.
+
+7. [ ] **code-quality-reviewer** | `scripts/check-release-notes.sh:163` | code-readability
+   The section-heading grep pattern `^##[ \t]+.*${sec}` uses `[ \t]` inside a `-E` (ERE) context. POSIX ERE does not define `\t` as a character class shorthand; `\t` inside `[...]` is implementation-defined. On GNU grep and BSD grep it happens to work (both interpret `\t` as a tab in character classes), but it is non-portable. The same pattern appears in the awk-based `extract_section_body` function (line 234) where `\t` inside a character class is equally non-portable in POSIX awk.
+   *Recommendation:* Use `[[:space:]]` instead of `[ \t]` for POSIX portability, e.g. `^##[[:space:]]+.*${sec}`. Alternatively, use a literal tab via `$'\t'` if a tab is specifically intended and a space should not match.
+
+8. [ ] **code-quality-reviewer** | `scripts/check-release-notes.sh:213` | code-elegance
+   A5 RENAME_PAIRS entry 'disallow_ip;(block_ip|unblock_ip)' has no word-boundary anchors on the v1 side, while the symmetrical allow_ip entry (line 212) uses \ballow_ip\b. The asymmetry is intentional (disallow_ip has no ambiguous prefix/suffix in this file) but is worth a comment explaining why. Currently a reader must reason through it to understand the difference is deliberate rather than an oversight.
+   *Recommendation:* Add a brief inline comment on the disallow_ip pair: '# no \b needed — no shorter token contains disallow_ip as a substring' (analogous to the existing scope-note comment above the block).
+
+9. [ ] **code-quality-reviewer** | `scripts/check-release-notes.sh:288` | code-readability
+   The markdownlint invocation `if ! markdownlint -q "$NOTES" 2>&1; then` has a redundant `2>&1` redirect. The `-q` flag already suppresses markdownlint's normal output on success; on failure its error lines go to stdout (the default channel for markdownlint). The `2>&1` has no observable effect here and reads as if it were meant to capture output for display, which it does not — the combined stream is consumed by the shell's `if` condition check and then discarded.
+   *Recommendation:* Remove the `2>&1` to make the intent clear: `if ! markdownlint -q "$NOTES"; then`. If the goal was to suppress markdownlint's stderr entirely, use `2>/dev/null` explicitly.
+
+10. [ ] **code-quality-reviewer** | `scripts/check-release-notes.sh:51` | code-readability
+   check_tokens_present calls 'exit 1' rather than 'return 1'. This is correct because the script uses set -e and an early-exit-on-first-failure design, but it means the helper cannot be reused in a context that wants to collect all failures before exiting. The design matches the surrounding code's pattern (all other error paths also call exit 1 or fail()), so this is internally consistent. The minor concern is that the function comment says nothing about the exit-vs-return distinction, which could surprise a future maintainer who tries to use the helper in a tolerate-and-accumulate loop.
+   *Recommendation:* Add a one-line note to the check_tokens_present comment: '# Calls exit 1 directly (not return) — intended for fail-fast use only.'
+
+11. [ ] **code-quality-reviewer** | `scripts/check-release-notes.sh:58` | test-coverage
+   A2 claims 'the no_* setter family is enforced by A2 (presence)' (comment at line 180-182), but REQUIRED_V1_TOKENS only checks three of the twelve no_* setters removed in v2: no_ssl, no_basic_auth, no_digest_auth. The remaining nine (no_debug, no_pedantic, no_deferred, no_regex_checking, no_ban_system, no_post_process, no_single_resource, no_ipv6, no_dual_stack) are documented in RELEASE_NOTES.md but not in the token list, so future edits could drop them without the check catching it.
+   *Recommendation:* Either add the remaining no_* tokens to REQUIRED_V1_TOKENS so the comment matches reality, or update the comment to acknowledge the intentional partial coverage (e.g. 'A2 spot-checks three representative no_* names; the full list is covered by prose only').
+
+12. [ ] **code-quality-reviewer** | `scripts/check-release-notes.sh:67` | code-readability
+   In extract_section_body, the awk program applies tolower() to both $0 and the re variable (line 71: 'if (tolower($0) ~ tolower(re))'). This is correct but subtly redundant for the current callers: the heading_re arguments passed by check_section_cites already use all-lowercase patterns ('^##[ \t]+.*threading', '^##[ \t]+.*error[ \t-]+propag'). The tolower(re) wrapping provides no benefit for these callers but silently extends the helper's contract. The inconsistency between 're already lowercase' at the call site and 'tolower(re)' inside is a minor readability friction.
+   *Recommendation:* Either document in the function comment that heading_re is matched case-insensitively (so callers know they need not lowercase their pattern), or remove the tolower(re) call and lowercase the heading_re arguments explicitly at each call site for clarity. The former is lower churn.
+
+13. [ ] **code-simplifier** | `scripts/check-release-notes.sh:57` | code-structure
+   check_tokens_present() calls 'exit 1' directly instead of delegating to fail(). Every other error path in the script goes through fail(), which prefixes 'check-release-notes: FAIL:' and exits. The helper duplicates that prefix inline (line 58) and then calls exit 1 on line 61. This is a minor inconsistency: if the fail() message prefix is ever changed, this path won't update with it.
+   *Recommendation:* Replace the inline echo+exit block with a single fail() call. E.g.: fail "${label}: missing tokens:\n$(printf '  %s\n' "${missing[@]}")" — or accumulate the list and pass it to fail(). This removes the duplicate prefix string and keeps all exits through one funnel.
+
+14. [ ] **code-simplifier** | `scripts/check-release-notes.sh:79` | naming
+   check_section_cites() takes 5 positional parameters, with $5 being section_name used only in human-readable error messages. The parameter name 'section_name' is clear, but the order (label, file, heading_re, citation_re, section_name) differs from the usage at lines 254-262 where the section_name reads most naturally at position 3 (after label and file), before the two regexes. The current order requires callers to remember that the human-readable name comes last, after the two opaque regexes.
+   *Recommendation:* This is a low-priority polish note — the current order is workable and there are only two call sites. If the function is ever touched again, consider reordering to (label, file, section_name, heading_re, citation_re) so the readable name stays adjacent to label, and the two regexes are grouped at the end.
+
+15. [ ] **performance-reviewer** | `/Users/etr/progs/libhttpserver/.worktrees/TASK-042/scripts/check-release-notes.sh:77` | missing-batching
+   A2 and A3 token checks loop with one grep per token (32 + 26 = 58 grep invocations) on the same file. Each spawns a child process and opens the file. On a cold filesystem cache this is the dominant cost of the script.
+   *Recommendation:* Collapse both token loops into a single grep -oE run that emits matched tokens, then diff the result against the required list. Alternatively, read the file once into a variable and grep against that string. Given the file is ~500 lines and the script runs once per CI invocation, this is a cosmetic improvement with no practical impact; keep it as-is unless script startup time becomes a concern.
+
+16. [ ] **security-reviewer** | `scripts/check-release-notes.sh:163` | injection
+   The loop variable `$sec` from REQUIRED_SECTIONS is interpolated directly into the ERE pattern passed to `grep -qiE` (line 163: `"^##[ \t]+.*${sec}"`). The section strings come from a hardcoded array, so no untrusted data is involved today, but if the array were ever populated from an external source (environment variable, config file) a value containing ERE metacharacters such as `(`, `.`, or `*` would silently change the match semantics rather than failing loudly. CWE-88 (Argument Injection).
+   *Recommendation:* Either document explicitly that REQUIRED_SECTIONS must never be populated from external input, or pass the section strings through a regex-escaping step (e.g. `printf '%s' "$sec" | sed 's/[.[*+?^${}()|\\]/\\&/g'`) before interpolation. Since the array is fully static this is low priority.
+
+17. [ ] **security-reviewer** | `scripts/check-release-notes.sh:217` | injection
+   In the A5 rename-pair loop, `v1` and `v2` are derived from splitting `$pair` on `;` and are then interpolated directly into a `grep -qE` ERE pattern (line 217: `"(${v1}.*${v2}|${v2}.*${v1})"`). The pairs array contains regex metacharacters intentionally (e.g. `(block_ip|unblock_ip)`) which is by design. However, if a future entry in RENAME_PAIRS inadvertently includes unbalanced parentheses or other metacharacters it would cause a silent grep error. `grep -qE` exits non-zero on a bad pattern, and with `set -euo pipefail` this would abort the script with a confusing error. CWE-88.
+   *Recommendation:* This is acceptable given the fully-static array. Add a comment in the RENAME_PAIRS block warning that v2 values may use ERE alternation groups deliberately and that any new pair must be valid ERE.
+
+18. [ ] **security-reviewer** | `scripts/check-release-notes.sh:232` | injection
+   The `extract_section_body` function passes `$2` (a regex string) to `awk` via `-v re="$2"` and then applies it with `tolower($0) ~ tolower(re)` inside the awk program (line 236). The two call sites on lines 242–243 pass hardcoded string literals, so the actual risk is negligible. However, if a future caller passes a string containing a backslash or double-quote the awk `-v` assignment will misparse the value (awk's `-v` processes escape sequences in the value string). This is a latent robustness bug rather than an exploitable vulnerability. CWE-88.
+   *Recommendation:* Use `ENVIRON` to pass the regex instead of `-v`: export a temporary env var and read it inside the awk script with `ENVIRON["RE"]`, or switch the match to a `BEGINFILE`/`BEGIN` pattern that avoids regex injection. Since callers are internal and static, a comment documenting the restriction is an acceptable minimum.
+
+19. [ ] **spec-alignment-checker** | `/Users/etr/progs/libhttpserver/.worktrees/TASK-042/RELEASE_NOTES.md:152` | acceptance-criteria
+   PRD §3.6 (API-REQ, PRD-REQ-REQ-001) explicitly calls out get_path_pieces and get_files as changed to return const& — but the 'What changed semantically' section only mentions 'get_headers(), get_args(), …' using an ellipsis. A v1 user grepping for 'get_path_pieces' or 'get_files' will find nothing in the document. These methods are not renamed or removed so they fall outside the strict AC-1 wording ('renamed/removed/added'), but the ellipsis leaves the picture incomplete for porters auditing individual method call sites.
+   *Recommendation:* Expand the container-getter sentence to list all four named getters explicitly: 'Container getters (get_headers(), get_args(), get_path_pieces(), get_files()) return const& to the underlying map.' This makes the document greppable for those names and removes the ambiguity introduced by the ellipsis.
+
+20. [ ] **spec-alignment-checker** | `/Users/etr/progs/libhttpserver/.worktrees/TASK-042/RELEASE_NOTES.md:null` | specification-gap
+   PRD §3.7 explicitly grandfathers shoutCAST() as the sole camelCase survivor in the v2 public API. RELEASE_NOTES.md does not mention shoutCAST at all. A porter who notices shoutCAST in v1 code and consults the release notes will find no entry confirming it is intentionally preserved. The acceptance criteria do not require it (it is not renamed/removed), but a note under 'What's renamed' or a brief remark in the naming section would prevent confusion.
+   *Recommendation:* Add a one-line footnote to the 'What's renamed' table or to the 'What changed semantically' section, e.g.: 'shoutCAST() is the sole camelCase survivor — the name maps to the SHOUTcast streaming protocol and is intentionally preserved unchanged (PRD §3.7).'
+
+21. [ ] **test-quality-reviewer** | `/Users/etr/progs/libhttpserver/.worktrees/TASK-042/scripts/check-release-notes.sh:88` | missing-test
+   register_resource is documented in RELEASE_NOTES.md (lines 30, 80, 130) and its replacement (register_path / register_prefix) satisfies the acceptance criterion today, but register_resource is not in REQUIRED_V1_TOKENS (A2). If that content were accidentally removed from the document, A2 would not catch it — only a human reader would notice the gap. The A5 rename pairs also have no entry for register_resource, so neither A2 nor A5 independently guards its presence.
+   *Recommendation:* Add '\bregister_resource\b' to REQUIRED_V1_TOKENS in A2. Optionally add a corresponding A5 pair 'register_resource;(register_path|register_prefix)' to enforce that the replacement appears on the same line as the v1 name.
+
+22. [ ] **test-quality-reviewer** | `scripts/check-release-notes.sh:108` | missing-test
+   A3 (required v2-era tokens) does not include 'httpserver::constants' even though the namespace is the documented replacement for all removed #define constants (RELEASE_NOTES.md line 102). If the namespace name is misspelled or the section dropped, no check catches it.
+   *Recommendation:* Add '\bhttpserver::constants\b' to REQUIRED_V2_TOKENS.
+
+23. [ ] **test-quality-reviewer** | `scripts/check-release-notes.sh:151` | missing-test
+   A4 does not require a 'Threading' or 'Error propagation' section by name — their presence is enforced only transitively by A6 (which checks the body, but emits a slightly different error message). If A4 is the section-list-of-record, the two sections should be listed there so the checklist is self-documenting and the error message is consistent.
+   *Recommendation:* Add 'threading' and 'error propagation' to REQUIRED_SECTIONS in A4, or add a comment explaining why they are intentionally omitted from A4 and covered only by A6.
+
+24. [ ] **test-quality-reviewer** | `scripts/check-release-notes.sh:33` | non-deterministic
+   The script comments acknowledge CRLF line endings will cause silent A5 pair failures rather than a clear 'CRLF detected' error. There is no defensive check (e.g., grep -cP '\r' "$NOTES") to catch this explicitly.
+   *Recommendation:* Add an explicit CRLF guard near the top of the script: if grep -qP '\r' "$NOTES"; then fail 'CRLF line endings detected in RELEASE_NOTES.md; convert to LF'; fi
+
+25. [ ] **test-quality-reviewer** | `scripts/check-release-notes.sh:58` | missing-test
+   The A2 token list does not check DEFAULT_WS_TIMEOUT (separate from DEFAULT_WS_PORT which is checked). Both constants are called out together on RELEASE_NOTES.md line 45, so a partial deletion would leave DEFAULT_WS_PORT but silently drop DEFAULT_WS_TIMEOUT.
+   *Recommendation:* Add '\bDEFAULT_WS_TIMEOUT\b' to REQUIRED_V1_TOKENS (subsumes part of finding 3 — included here as a standalone minor for tracking clarity).