TASK-084: persist 18 unworked review findings

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Rkuh4aSmrD8m2f2vYqakb6

Author: etr

specs/unworked_review_issues/2026-06-22_213828_task-084.md

@@ -0,0 +1,79 @@
+# Unworked Review Issues
+
+**Run:** 2026-06-22 21:38:28
+**Task:** TASK-084
+**Total:** 18 (0 critical, 0 major, 18 minor)
+
+## Minor
+
+1. [ ] **architecture-alignment-checker** | `test/bench_get_headers.cpp:56` | pattern-violation
+   The libstdc++ static_assert band check uses a magic lower bound of 500.0 with no documentation for its derivation. Architecture docs (PERFORMANCE.md) document 640 ns as the committed value and ~667 ns as the measured value; the 500 ns floor is unexplained and could silently accept a substantially under-measured constant in a future re-measurement without triggering the guard.
+   *Recommendation:* Add a brief inline comment explaining how 500.0 was chosen (e.g. '~25% below the 640 ns committed value, giving re-measurement latitude while still catching an accidental reversion to a very low placeholder'), or tighten the lower bound to a value derived from the documented range (e.g. 600.0).
+
+2. [ ] **code-quality-reviewer** | `test/bench_get_headers.cpp:55` | test-coverage
+   The compile-time static_asserts in bench_get_headers.cpp guard the libstdc++ and libc++ values, but there is no corresponding static_assert for the sizeof constants (V1_HTTP_RESOURCE_SIZEOF, V1_STD_MAP_STRING_BOOL_SIZEOF) even though those constants also carry per-stdlib values added earlier. This is mildly inconsistent: a future edit that accidentally swaps the sizeof constants between stdlibs would not be caught by any compile-time guard in the bench files.
+   *Recommendation:* If the sizeof bench (bench_sizeof_http_resource.cpp) already carries guards for these constants, document that here with a comment. If not, consider adding analogous static_asserts for the per-stdlib sizeof constants in that bench file (out of scope for TASK-084, but worth a TODO comment).
+
+3. [ ] **code-quality-reviewer** | `test/bench_get_headers.cpp:60` | test-coverage
+   The libstdc++ static_assert upper bound (V1_GET_HEADERS_NS_PER_CALL <= 760.0) is implicitly ensured by the != 760.0 guard plus the range check, but the range [500.0, 760.0] is quite wide. If a maintainer commits a wildly mis-measured value like 750.0 (nearly equal to the libc++ constant), neither assert would fire. The range guard is useful but the width warrants a brief comment explaining why 500 was chosen as the lower bound.
+   *Recommendation:* Add a one-line comment in bench_get_headers.cpp explaining the 500.0 floor (e.g., 'floor chosen as ~75% of the native aarch64 measurement to catch obviously mis-measured values while allowing for architecture variation'). This is documentation-only; no functional change needed.
+
+4. [ ] **code-quality-reviewer** | `test/bench_get_headers.cpp:61` | code-elegance
+   The libstdc++ range guard `V1_GET_HEADERS_NS_PER_CALL >= 500.0 && V1_GET_HEADERS_NS_PER_CALL <= 760.0` uses 760.0 as the upper bound, which is exactly the libc++ value the first static_assert was designed to reject. This means a future maintainer who accidentally sets the libstdc++ constant to 760.0 would pass the first assert (`!= 760.0` fails at 760.0, correctly) but the boundary check upper bound happens to be consistent with intentional commits anywhere up to 760.0 — the range is slightly wider than necessary given the known observed maximum (~742 ns). This is fine in practice, but tightening to, say, 750.0 would make the intent ('must be strictly below the libc++ value') self-documenting.
+   *Recommendation:* Consider lowering the upper bound from 760.0 to 750.0 (or the observed maximum 742 ns rounded up conservatively) with a comment explaining it must stay below the libc++ value to encode the measurement distinction clearly.
+
+5. [ ] **code-quality-reviewer** | `test/v1_baseline/v1_constants.hpp:121` | readability
+   The libstdc++ ns/call provenance comment discloses that the constant was measured on a native aarch64 Linux container (not a native x86-64 CI runner, the platform that will actually exercise the branch in verify-build.yml). This is documented, but the relationship between the measurement platform and the CI execution platform could be clearer. A reader who only scans the header (not PERFORMANCE.md or README.md) may not realise the 640.0 value was not taken on the same x86-64 hardware that runs the CI gate.
+   *Recommendation:* Add a one-line note directly adjacent to the literal — e.g. '// Note: CI runs on x86-64; this value was measured on native aarch64 libstdc++. The emulated x86-64 run confirmed libstdc++ is comparable-or-slower, validating the gate direction.' — so the header is self-explanatory without requiring the reader to cross-reference README.md.
+
+6. [ ] **code-quality-reviewer** | `test/v1_baseline/v1_constants.hpp:129` | documentation
+   The libstdc++ provenance comment mentions 'native aarch64 libstdc++ (Apple-silicon Linux container)' as the measurement environment, which is not the same architecture as the verify-build.yml x86-64 Linux runner that will actually use this constant in CI. The comment notes this limitation but the discrepancy between aarch64-native measurement and x86-64 CI target remains a subtle gap: aarch64 and x86-64 std::map traversal costs can differ due to cache-line sizes and branch-predictor characteristics.
+   *Recommendation:* The existing comment already documents this limitation clearly and the rounded-down conservative choice mitigates it. No code change required; optionally add a one-liner noting that x86-64 native measurement is the preferred future update path when a native runner is accessible.
+
+7. [ ] **code-simplifier** | `test/bench_get_headers.cpp:137` | naming
+   The local variable `v1_ns` is introduced solely to shadow `V1_GET_HEADERS_NS_PER_CALL` in the printf call. The name adds one level of indirection without adding clarity: `v1_ns` is less descriptive than the constant it aliases, and readers must mentally trace back to understand what value it holds.
+   *Recommendation:* Use `V1_GET_HEADERS_NS_PER_CALL` directly in the printf and ratio computation rather than introducing a short-lived alias. This removes the indirection without changing behavior: `const double ratio = V1_GET_HEADERS_NS_PER_CALL / v2_median_ns;` and replace `v1_ns` with `V1_GET_HEADERS_NS_PER_CALL` in the printf.
+
+8. [ ] **code-simplifier** | `test/v1_baseline/v1_constants.hpp:110` | code-structure
+   The `#if defined(__GLIBCXX__)` / `#elif defined(_LIBCPP_VERSION)` block for `V1_GET_HEADERS_NS_PER_CALL` mirrors the identical pattern already used for the sizeof constants (lines 70-90). The structure is consistent, which is good, but the comment block preceding the ns/call constant is substantially longer (~35 lines) than the one preceding the sizeof constants (~15 lines). Most of the extra lines in the ns/call comment describe measurement provenance details (Docker emulation, two vantage points, rounding rationale) that are already fully documented in `test/v1_baseline/README.md` and `test/PERFORMANCE.md`. The duplication between header comment and the README/PERFORMANCE.md creates a maintenance hazard: if measurement details are updated in one place, the other three fall out of sync.
+   *Recommendation:* Trim the ns/call comment in `v1_constants.hpp` to a brief provenance summary (platform, compiler, observed range, committed value) and a pointer to `test/v1_baseline/README.md` for the full measurement narrative. The sizeof constants' comment block is a good length template. This reduces the synchronization surface without losing the important conservative-rounding rationale, which can be kept as a single sentence.
+
+9. [ ] **code-simplifier** | `test/v1_baseline/v1_constants.hpp:131` | naming
+   The libstdc++ constant comment says '~667 ns rounded down to 640 ns' but 640 is a 4% rounding from 667, not a simple floor-to-nearest-ten. The rounding magnitude is fine, but the inline phrase 'rounded lower end of the un-emulated native range (~667 ns rounded down to 640 ns)' slightly overstates precision: 640 is 4% below the measured 667, which is a larger conservative margin than the libc++ case (756→760 is only 0.5% below). A brief note that the larger margin is intentional (wider observed spread) would pre-empt a future reader questioning the discrepancy.
+   *Recommendation:* Optionally add a parenthetical like '(wider observed spread; extra margin is intentional)' after the '640 ns' value in the libstdc++ branch comment to document the deliberate conservatism. This is cosmetic—behaviour is unaffected.
+
+10. [ ] **performance-reviewer** | `test/bench_get_headers.cpp:113` | missing-batching
+   Header keys and values are constructed via `std::snprintf` into stack buffers inside a loop when building the 16-header fixture. This is in cold-path setup code (not the measured path), so it has no impact on the benchmark numbers themselves. No performance concern for the benchmark output.
+   *Recommendation:* No action required — this is fixture setup code outside the timed window. Noted only for completeness.
+
+11. [ ] **performance-reviewer** | `test/bench_get_headers.cpp:123` | missing-caching
+   Warmup loop uses 10,000 iterations, but INNER is 1,000,000. The warmup count is generous for a memoised warm-path benchmark (one call populates the cache; subsequent calls are O(1) reference returns). This is acceptable but slightly over-specified — 100-1,000 iterations would equally saturate the icache and branch predictor for this code path.
+   *Recommendation:* No change required for correctness. If compile time or CI wall time is ever a concern, reducing warmup to 1,000 would still achieve full cache saturation. Document the chosen count as 'generous to cover branch predictor depth' in a comment.
+
+12. [ ] **performance-reviewer** | `test/bench_get_headers.cpp:61` | algorithmic-complexity
+   The static_assert range check `V1_GET_HEADERS_NS_PER_CALL >= 500.0 && V1_GET_HEADERS_NS_PER_CALL <= 760.0` uses 760.0 as the upper bound for the libstdc++ value. This upper bound equals the libc++ constant exactly, which could cause a false pass if someone accidentally copies the libc++ value (760.0) into the libstdc++ branch — the `!= 760.0` guard on line 57 prevents this, so the two together are sufficient. However the range band [500, 760] is wide enough to admit values that may not correspond to any real libstdc++ measurement.
+   *Recommendation:* Consider tightening the upper bound to something closer to the actual observed range top (742 ns rounded up), e.g. `<= 750.0`, to make the range check a tighter sentinel. The current 760.0 upper bound is still correct, but a narrower band is a more reliable guard against an un-re-measured constant slipping through.
+
+13. [ ] **performance-reviewer** | `test/v1_baseline/v1_constants.hpp:131` | algorithmic-complexity
+   The libstdc++ V1_GET_HEADERS_NS_PER_CALL value (640.0) was derived from a native aarch64 libstdc++ measurement (~667 ns rounded down to 640 ns), not from a native x86-64 measurement. The comment acknowledges the x86-64 emulated value (~4477-5029 ns) was NOT used, but the native measurement was itself on aarch64 with libstdc++ — a different micro-architecture than the x86-64 verify-build.yml Linux CI runner. This means the libstdc++ gate is anchored to aarch64 performance characteristics while the CI runs x86-64, creating a mild apples-to-oranges comparison for the libstdc++ lane.
+   *Recommendation:* Add an explicit comment clarifying that the 640 ns value is the aarch64-libstdc++ measurement used as a conservative proxy for x86-64-libstdc++ (since x86-64 native was not reachable). If x86-64 native measurement becomes available via the CI runner, update the constant. The current value is directionally correct (both platforms show libstdc++ map is comparable-or-slower) but the provenance gap should be documented clearly to prevent future confusion.
+
+14. [ ] **security-reviewer** | `test/v1_baseline/README.md:38` | sensitive-data
+   The README re-measurement procedure instructs writing binaries to /tmp (e.g., -o /tmp/measure_v1_sizes, -o /tmp/measure_v1_get_headers). These are one-off local measurement binaries that are never committed, but /tmp is world-writable on Linux; a local attacker sharing the same host could replace the binary before execution and influence the measured baseline constant that gets committed. The risk is negligible in practice (the baseline is a trusted developer workstation action), but the practice normalizes unsafe temp paths.
+   *Recommendation:* Use a subdirectory under $HOME or the project build directory instead of /tmp for measurement outputs, e.g., -o $(mktemp -d)/measure_v1_sizes. This eliminates any TOCTOU risk on multi-user hosts.
+
+15. [ ] **spec-alignment-checker** | `test/bench_get_headers.cpp:61` | specification-gap
+   The libstdc++ sanity-check static_assert band is [500.0, 760.0], where the upper bound (760.0) is the libc++ value. Since the libstdc++ constant is currently 640.0, the upper bound check passes. However, the range is somewhat ad hoc — 500.0 as a lower bound is not documented anywhere in the task spec or PERFORMANCE.md. If a future re-measurement yields a value below 500 ns (e.g., on a faster host), the assert would incorrectly reject it.
+   *Recommendation:* Add a comment next to the 500.0 lower-bound explaining its rationale (e.g., 'below 500 ns would be atypically fast for a 16-header std::map copy scenario, indicating a potential measurement error'). This is a documentation gap, not a behavioral issue.
+
+16. [ ] **spec-alignment-checker** | `test/v1_baseline/v1_constants.hpp:131` | acceptance-criteria
+   The libstdc++ V1_GET_HEADERS_NS_PER_CALL constant (640.0 ns) is derived from a native aarch64 libstdc++ container measurement rather than from a native x86-64 Linux host. The task's action item explicitly calls for measurement on 'a representative Linux x86-64 host with libstdc++ (Ubuntu 22.04, the verify-build.yml default)'. The comment in v1_constants.hpp acknowledges this, noting the x86-64 emulated measurement (4477-5029 ns/call) was not used because it was inflated by binary translation. The committed conservative 640 ns value is taken from the native aarch64 libstdc++ measurement instead.
+   *Recommendation:* This deviation is documented in the header with justification. The acceptance criteria do not mandate a specific measurement host; they only require 'separate ns/call values for libc++ and libstdc++'. The actual gate (>=10x speedup) passes on both architectures, so the practical impact is minimal. Consider noting in PERFORMANCE.md or the task that the x86-64 native value remains unmeasured and could be captured if a bare-metal Linux x86-64 host becomes available.
+
+17. [ ] **test-quality-reviewer** | `test/bench_get_headers.cpp:57` | implementation-coupling
+   The libstdc++ static_assert `V1_GET_HEADERS_NS_PER_CALL != 760.0` guards against accidental reuse of the libc++ literal, but the lower-bound check `>= 500.0` is an arbitrary floor with no documented basis. If the actual libstdc++ measurement on a future platform falls below 500 ns (e.g., a significantly faster std::map implementation), the assert would fire spuriously, blocking a legitimate re-measurement update. Conversely if it rises above 760.0 (e.g., a slower host), the upper-bound check `<= 760.0` also fires — this may be intentional to force re-measurement, but the intent is not stated.
+   *Recommendation:* Add a one-line comment at the static_assert explaining the rationale for the 500.0 lower bound and 760.0 upper bound (e.g., 'band derived from TASK-084 native aarch64 measurement; update if re-measured on a significantly different host'). This makes the intent clear and helps a future maintainer distinguish a 'stale constant' failure from a 'platform genuinely out of range' failure.
+
+18. [ ] **test-quality-reviewer** | `test/v1_baseline/v1_constants.hpp:131` | non-deterministic
+   The libstdc++ V1_GET_HEADERS_NS_PER_CALL value of 640.0 ns is derived from a native aarch64 Linux container measurement (on Apple Silicon hardware), not from the actual x86-64 verify-build.yml CI lane it guards. The emulated x86-64 measurement (4477-5029 ns) was explicitly discarded. This means the libstdc++ arm of the >=10x gate is enforced on x86-64 CI using an aarch64 baseline — the architectures differ in map node memory layout, cache line widths, and allocator behaviour, so the 640 ns number may not faithfully represent the v1 cost on the target platform (x86-64 Linux). While the ~230x headroom makes a false FAIL very unlikely, the mismatch between baseline architecture and CI target architecture is a documentation-level risk: a future maintainer may tighten the gate or change the CI matrix without realising the baseline is cross-architecture.
+   *Recommendation:* Document in the v1_constants.hpp comment and in PERFORMANCE.md that the libstdc++ 640.0 ns baseline was measured on native aarch64 Linux, NOT native x86-64. Add a note that if the CI matrix adds a native x86-64 libstdc++ lane, the constant should be re-measured on that architecture before tightening the gate. This is a documentation fix only; the current gate threshold is safe given the headroom.