fix: sweep the 2026-09 review findings

.github/workflows/ci.yml

@@ -6,18 +6,28 @@ on:
   pull_request:
   workflow_dispatch:
 
+# Least privilege by default. The workflow runs on pull_request, and
+# consumer-smoke plus the FetchContent builds clone and execute third-party
+# code, so no job should inherit a writable token. A job that needs more grants
+# itself a narrower block.
+permissions:
+  contents: read
+
 jobs:
   build-linux:
     runs-on: ubuntu-24.04
     steps:
-      - uses: actions/checkout@v7.0.1
+      - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
 
       - name: Install dependencies
         run: |
           sudo apt-get update
           sudo apt-get install -y --no-install-recommends \
             build-essential cmake pkg-config clang-format \
-            libcurl4-openssl-dev
+            libcurl4-openssl-dev locales
+          # The LocaleIndependence tests need a comma-decimal locale; without
+          # one they GTEST_SKIP and the regression they guard goes unchecked.
+          sudo locale-gen de_DE.UTF-8
 
       - name: Build
         run: make build CMAKE_ARGS=-DSPC_WARNINGS_AS_ERRORS=ON
@@ -34,7 +44,7 @@ jobs:
   build-macos:
     runs-on: macos-latest
     steps:
-      - uses: actions/checkout@v7.0.1
+      - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
 
       - name: Install dependencies
         run: |
@@ -53,18 +63,19 @@ jobs:
   markdown-lint:
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v7.0.1
-      - uses: DavidAnson/markdownlint-cli2-action@v23
+      - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
+      - uses: DavidAnson/markdownlint-cli2-action@ded1f9488f68a970bc66ea5619e13e9b52e601cd # v23
 
   sanitizer:
     runs-on: ubuntu-24.04
     steps:
-      - uses: actions/checkout@v7.0.1
+      - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
       - name: Install dependencies
         run: |
           sudo apt-get update
           sudo apt-get install -y --no-install-recommends \
-            build-essential cmake libcurl4-openssl-dev
+            build-essential cmake libcurl4-openssl-dev locales
+          sudo locale-gen de_DE.UTF-8
       - name: Build with address and undefined behavior sanitizers
         run: >-
           cmake -S . -B build-sanitized
@@ -78,12 +89,13 @@ jobs:
   thread-sanitizer:
     runs-on: ubuntu-24.04
     steps:
-      - uses: actions/checkout@v7.0.1
+      - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
       - name: Install dependencies
         run: |
           sudo apt-get update
           sudo apt-get install -y --no-install-recommends \
-            build-essential cmake libcurl4-openssl-dev
+            build-essential cmake libcurl4-openssl-dev locales
+          sudo locale-gen de_DE.UTF-8
       - name: Build with ThreadSanitizer
         run: >-
           cmake -S . -B build-thread-sanitized
@@ -97,7 +109,7 @@ jobs:
   clang-tidy:
     runs-on: ubuntu-24.04
     steps:
-      - uses: actions/checkout@v7.0.1
+      - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
       - name: Install dependencies
         run: |
           sudo apt-get update
@@ -120,10 +132,11 @@ jobs:
   consumer-smoke:
     runs-on: ubuntu-24.04
     steps:
-      - uses: actions/checkout@v7.0.1
+      - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
       - name: Install dependencies
         run: |
           sudo apt-get update
           sudo apt-get install -y --no-install-recommends \
-            build-essential cmake libcurl4-openssl-dev
+            build-essential cmake libcurl4-openssl-dev locales
+          sudo locale-gen de_DE.UTF-8
       - run: ./tools/test_consumers.sh

CHANGELOG.md

@@ -6,6 +6,141 @@ uses [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ## [Unreleased]
 
+### Changed
+
+- **HTTP 404 now says which kind of 404 it was.** `ErrorCode::FeedUnavailable`
+  ("no active outlook — clear the rows") is produced only by
+  `StaticFeedClient`, the one feed where SPC uses 404 that way. A 404 from the
+  ArcGIS MapServer or from IEM — a retired product, a renamed service path, a
+  wrong base URL — is now `ErrorCode::NotFound`, which was previously
+  unreachable. A logical ArcGIS `{"error":{"code":404}}` envelope, which is how
+  a renamed MapServer path is actually reported (over HTTP 200), maps to
+  `NotFound` as well. Consumers that branch on `is_feed_unavailable()` for
+  `ArcGISClient` or `ArchiveClient` results should treat `NotFound` as a fault
+  and alert on it instead of clearing rows.
+- `Error::from_response` takes a trailing `Feed404` argument stating what a 404
+  means for the feed that answered. It defaults to `Feed404::NotFound`, so
+  existing calls keep compiling and get the safe reading.
+
+### Fixed
+
+- **ArcGIS paging advanced `resultOffset` by the requested page size, not by
+  the records the server returned.** ArcGIS clamps `resultRecordCount` to the
+  layer's own `maxRecordCount`, so a truncated page can be shorter than the
+  2000 requested; the offset then skipped the gap and the caller got a
+  successful result with a silent hole. `ArcGISPager::advance()` now takes the
+  returned record count.
+- **`HttpClient` reached the local filesystem.** `is_absolute_url()` only
+  recognises `http://` and `https://`, so `file:///etc/hosts` was classified as
+  relative, appended to the empty default `base_url`, and handed to libcurl,
+  which read the file and returned it as the response body. The transport now
+  restricts libcurl to `http` and `https` on the request and on redirects, and
+  caps redirects at 10.
+- `ClientConfig::max_response_bytes` (64 MB default) bounds a single response
+  body. An IEM archive window is caller-chosen and unbounded, and the body was
+  buffered whole, then parsed into a full JSON AST, then into the payload.
+- Retry honours a `Retry-After` header on 429/503 (delta-seconds form, clamped
+  to `max_delay`) instead of retrying a server that asked for 60 s after
+  200 ms. The header was already captured and then discarded.
+- Retry jitter is applied before the `max_delay` clamp, not after, so a delay
+  can no longer exceed the documented ceiling by `jitter_factor`. A
+  `max_attempts` of 0 now performs the request once instead of returning a
+  fabricated "Max retry attempts exceeded" for a request never made.
+- The default `User-Agent` is generated from `PROJECT_VERSION` instead of a
+  hard-coded literal, and a test fails if the two ever disagree.
+- **Numeric-as-string probabilities were locale-dependent.** `std::stod`
+  delegates to `strtod`, which honours the process `LC_NUMERIC`. On a
+  comma-decimal host — any application that calls `setlocale(LC_ALL, "")` on a
+  de_DE / fr_FR / pt_BR desktop, as Qt and GTK apps do — `strtod("0.15")`
+  consumed only `"0"` and returned 0 without throwing. The live Day 4-8 static
+  feed carries its probability only as the string `"LABEL": "0.15"`, so the
+  feature was dropped by the `probability > 0.0` gate and
+  `StaticFeedClient::day4_8()` returned a successful, silently empty payload.
+  The same parse gated fire weather's no-risk sentinel filter, so a
+  `"Probability Too Low"` polygon shipped as a real band. Both now go through
+  a locale-independent `spc::detail::parse_double`, which uses
+  `std::from_chars` where the standard library provides it for `double` and a
+  classic-locale stream elsewhere (libc++ only implements floating-point
+  `from_chars` from version 20; the project's clang-tidy job builds against
+  libc++ 18). Output is unchanged in the C locale for every value in the
+  fixture corpus; the parse is narrower than `std::stod` only in rejecting
+  leading whitespace and a leading `+`, neither of which any SPC payload uses.
+- **`RateLimiter` crashed on a zero `refill_interval` (SIGFPE).**
+  `RateLimiter::Config` is a public aggregate with no validation, so
+  `RateLimiter{{.refill_interval = 0ms}}` reached an integer division by zero
+  in `refill()` on the first `try_acquire()`. The constructor now clamps a
+  non-positive interval to the 1000 ms default, and clamps `initial_tokens` to
+  `max_tokens`.
+- **`RateLimiter::acquire()` hung forever once a `daily_limit` was spent.**
+  With no `Config::max_wait` it polls `try_acquire()` every 10 ms, and
+  `try_acquire()` returns false permanently until the next UTC-midnight reset,
+  so the caller's thread spun until then. It now returns false immediately
+  when the daily quota is exhausted, since waiting cannot help.
+- `ArchiveClient` interpolated `ts`, `sts`, `ets` and `wfo` into IEM query URLs
+  with no percent-encoding, while the ArcGIS path in the same file encoded
+  every value. `api.hpp` documents the timestamps as ISO 8601, which permits a
+  `+HH:MM` offset, and a raw `+` decodes server-side as a space — so an
+  offset-bearing timestamp silently queried a different window. An `&` in any
+  of the four injected extra query parameters. All four are now encoded.
+- `ArchiveClient` now bounds its rate-limit wait (5 s) instead of blocking the
+  caller's thread indefinitely, which also makes the documented
+  `ErrorCode::RateLimited` result reachable, and acquires a token per retry
+  attempt rather than per call — `with_retry` re-issues up to 4 requests, and
+  retries precisely on 429/503, so one token was buying up to four requests
+  exactly when IEM was asking for less traffic.
+- ArcGIS paging is bounded. A page that reports truncation while carrying no
+  records, and a server that never stops reporting truncation, now fail with
+  `ErrorCode::ServerError` after at most `ArcGISPager::max_pages()` (100)
+  requests instead of looping forever and growing memory without bound.
+  `ArcGISPager::offset()` is a `std::int64_t`, so the arithmetic cannot
+  overflow.
+
+### Removed
+
+- The two-argument `parse_fire_weather(body, day)` overload. It silently
+  assumed `FireWeatherLayer::Outlook`, so a dry-thunderstorm body decoded
+  `dn=5` as `"ELEV"` (severity 1) instead of `"IDRT"` (severity 0) — the label
+  confusion 0.2.0 fixed, still reachable through the public API. The captured
+  day-1 and day-2 payloads carry no LABEL at all, only the shared numeric
+  `dn`, so nothing in a body says which layer produced it. Pass the layer
+  explicitly: `parse_fire_weather(body, day, FireWeatherLayer::Outlook)`
+  restores the old behaviour where that was in fact the right layer.
+
+- Release builds no longer default to `-march=x86-64-v3`. The probe only
+  proved the *compiler* accepted the flag, never that the run host has
+  AVX2/BMI2/FMA — and this is an installable SDK, so the build host and the run
+  host are routinely different. `-mtune=generic` is the default; set
+  `SPC_TUNE_X86_64_V3=ON` to opt in to the non-portable artifact.
+- The Esri-vs-GeoJSON parity gate now reads every captured fixture pair (three
+  categorical, four probabilistic). The test named for probabilistic parity
+  only ever opened the GeoJSON side, so `parse_esri_rings` was pinned by one
+  categorical layer.
+- The Linux CI jobs generate `de_DE.UTF-8`, so the locale regression tests run
+  there instead of skipping.
+- `ci.yml` declares `permissions: contents: read` at the top level — it runs on
+  `pull_request` and executes third-party build scripts — and pins both actions
+  to full commit SHAs instead of mutable tags.
+- `CLAUDE.md` and `CONTRIBUTING.md` list the `fixtures-check` and `lint-md`
+  gates that CI enforces, `CONTRIBUTING.md` names all seven CI jobs, `make help`
+  lists every target, and the README documents `src/core/`, `query_layer`, and
+  the `JSON library: Glaze (divergence note)` heading the CHANGELOG points at.
+
+### Deprecated
+
+- `ArcGISClient::query_storm_reports()`. The SPC MapServer has no Local Storm
+  Report layer, so the method always failed without touching the network — it
+  now carries the attribute and doc comment its sibling
+  `query_active_watches()` already had. Use `ArchiveClient::storm_reports()`.
+
+### Added
+
+- `ArcGISClient::query_fire_weather()` documents its all-or-nothing contract:
+  it merges two layers, and a failure on either discards both.
+- `Error::from_arcgis`, for ArcGIS logical failure envelopes. It keeps the
+  ArcGIS code in `Error::http_status` only while that code is HTTP-shaped
+  (100..599) and records it in `Error::detail`, so a code such as 1000 can no
+  longer masquerade as an HTTP status.
+
 ## [0.2.0] - 2026-09-03
 
 ### Added

CLAUDE.md

@@ -6,6 +6,8 @@
 make build
 make test
 make lint
+make fixtures-check   # fixture-provenance gate; CI fails on a stale SHA256SUMS
+make lint-md          # markdown-lint gate over **/*.md
 make test-consumers
 python3 tools/verify_arcgis_metadata.py  # live, opt-in
 ```
@@ -47,3 +49,11 @@ Glaze 8.3 parses loose SPC JSON. GoogleTest 1.18 runs the unit suite.
 
 The tag must match `project(spc-cpp VERSION ...)`. A `vX.Y.Z` tag triggers the
 release workflow, which reads the matching `CHANGELOG.md` section.
+
+The default `ClientConfig::user_agent` is generated from `PROJECT_VERSION` via
+`SPC_VERSION_STRING`, so a bump carries automatically for anything linking the
+targets, and `HttpClientLifecycle.DefaultUserAgentCarriesTheProjectVersion`
+fails if that stops being true. Also bump the `#ifndef SPC_VERSION_STRING`
+fallback literal in `include/spc/http_client.hpp`: it only applies to a
+consumer that includes the header without linking the target, so no test
+covers it.

CMakeLists.txt

@@ -14,6 +14,8 @@ option(SPC_BUILD_TESTS "Build tests" ON)
 option(SPC_BUILD_EXAMPLES "Build examples" ON)
 option(SPC_ENABLE_LTO "Enable Link Time Optimization" ON)
 option(SPC_NATIVE_ARCH "Use -march=native for CPU-specific tuning" OFF)
+option(SPC_TUNE_X86_64_V3
+    "Build Release with -march=x86-64-v3 (AVX2/BMI2/FMA). Makes the artifact non-portable." OFF)
 option(SPC_ENABLE_CLANG_TIDY "Run clang-tidy while compiling project targets" OFF)
 option(SPC_WARNINGS_AS_ERRORS "Treat project warnings as errors" OFF)
 
@@ -62,14 +64,24 @@ if(NOT MSVC)
     if(SPC_NATIVE_ARCH)
         set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} -march=native")
         message(STATUS "Using -march=native for CPU-specific optimizations")
-    else()
+    elseif(SPC_TUNE_X86_64_V3)
+        # Opt-in only. check_cxx_compiler_flag proves the *compiler* accepts
+        # the flag, never that the *run* host has AVX2/BMI2/FMA -- and this is
+        # an installable SDK (install(EXPORT) + find_package(spc)), so the
+        # build host and the run host are routinely different. Defaulting this
+        # on made every stock Release build SIGILL on pre-Haswell / pre-Zen
+        # hardware and inside VMs that do not expose AVX2.
         include(CheckCXXCompilerFlag)
         check_cxx_compiler_flag("-march=x86-64-v3" COMPILER_SUPPORTS_X86_64_V3)
         if(COMPILER_SUPPORTS_X86_64_V3)
             set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} -march=x86-64-v3")
+            message(STATUS "Release builds target x86-64-v3; the artifact is not portable")
         else()
+            message(WARNING "SPC_TUNE_X86_64_V3 requested but the compiler rejects the flag")
             set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} -mtune=generic")
         endif()
+    else()
+        set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} -mtune=generic")
     endif()
 endif()
 

CONTRIBUTING.md

@@ -15,11 +15,15 @@ cd spc-cpp
 sudo apt install -y build-essential cmake clang-format \
     libcurl4-openssl-dev
 
-make build      # CMake configure + Release build
-make test       # Run unit tests (ctest)
-make lint       # clang-format --dry-run + cpp_auto_audit
+make build           # CMake configure + Release build
+make test            # Run unit tests (ctest)
+make lint            # clang-format --dry-run + cpp_auto_audit
+make fixtures-check  # fixture-provenance gate (stale SHA256SUMS fails CI)
+make lint-md         # markdown-lint gate over **/*.md
 ```
 
+`make help` lists every target.
+
 ## Code style
 
 - `.clang-format` (LLVM base, **tabs**, width 4, 100-col). `make
@@ -45,8 +49,9 @@ their own product-specific severity mappers.
 
 ## PRs
 
-- Branch, push, open a PR against `main`. CI (linux + macos +
-  markdown-lint) must be green.
+- Branch, push, open a PR against `main`. Every CI job must be green:
+  `build-linux`, `build-macos`, `markdown-lint`, `sanitizer`,
+  `thread-sanitizer`, `clang-tidy` and `consumer-smoke`.
 - Conventional-commit subject lines.
 - Update `CHANGELOG.md` under `[Unreleased]`.
 - A maintainer merges with a **merge commit** (history reachability

Makefile

@@ -108,9 +108,15 @@ help:
 	@echo "  make build           - Configure and build the SDK (Release)"
 	@echo "  make debug           - Configure and build the SDK (Debug)"
 	@echo "  make test            - Run tests"
+	@echo "  make test-consumers  - Installed-package + FetchContent consumer checks"
 	@echo "  make lint            - Check formatting + cpp_auto_audit"
+	@echo "  make lint-md         - Markdown lint (CI gate)"
 	@echo "  make format          - Format code in place"
+	@echo "  make format-md       - Fix markdown lint findings in place"
+	@echo "  make fixtures-check  - Verify fixture provenance (CI gate)"
 	@echo "  make coverage        - Generate code coverage report (requires lcov)"
+	@echo "  make install-hooks   - Install the pre-commit hook"
+	@echo "  make pre-commit      - Format, then lint"
 	@echo "  make clean           - Remove build artifacts"
 	@echo "  make help            - Show this help"
 	@echo ""

README.md

@@ -57,6 +57,14 @@ publishes CAP and WFO polygons, but it omits the SPC parameters represented by
 `ArcGISClient::query_active_watches()` method is deprecated and returns
 `InvalidRequest` instead of fabricating incomplete watches.
 
+Storm reports are the same story: the SPC MapServer publishes no Local Storm
+Report layer, so `ArcGISClient::query_storm_reports()` is deprecated and always
+fails. Use `ArchiveClient::storm_reports()`.
+
+For anything the typed methods do not cover,
+`ArcGISClient::query_layer(service, layer_id, params)` runs a raw paged query
+against any of the three NOAA MapServers and returns the page bodies.
+
 ### Custom networking
 
 `HttpClient` is the default GET transport. Implement `HttpTransport` and pass
@@ -94,6 +102,7 @@ target_link_libraries(myapp PRIVATE spc::spc)
 | --- | --- |
 | `include/spc/` | Public clients, models, errors, and geometry helpers |
 | `src/api/` | Static feed, ArcGIS, and IEM routing |
+| `src/core/` | Errors, geometry, and the rate limiter |
 | `src/models/` | Glaze-backed GeoJSON and Esri parsers |
 | `src/http/` | libcurl transport |
 | `tests/` | Public client tests and captured parser fixtures |
@@ -117,6 +126,8 @@ The normal unit suite does not depend on NOAA availability. The metadata
 command compares the live ArcGIS layer IDs and names with the checked
 2026-09-03 contract, then queries all 39 feature layers.
 
+### JSON library: Glaze (divergence note)
+
 SPC payloads vary in key case, numeric representation, and geometry type. The
 parsers use Glaze 8.3's generic JSON tree to handle those shapes. The original
 convective parser stays aligned with the downstream `spc-data` service; new