fix: sweep the 2026-09 review findings - #6
Merged
Merged
Conversation
… feeds
Error::from_response mapped every HTTP 404 to FeedUnavailable, so
ErrorCode::NotFound was unreachable and a retired ArcGIS layer, a renamed
MapServer path or a dead IEM endpoint reached consumers as SPC's normal
"no active outlook" state. The documented consumer pattern then clears
rows for what is actually a broken integration.
from_response now takes a Feed404 trust-boundary argument. Only
StaticFeedClient passes NoActiveOutlook; ArcGISClient and ArchiveClient
take the NotFound default.
inspect_arcgis_envelope no longer reuses the HTTP status mapper for
ArcGIS logical errors, which arrive over HTTP 200: Error::from_arcgis
maps code 404 to NotFound (verified live -- a renamed MapServer path
answers HTTP 200 with {"error":{"code":404}}) and keeps a non-HTTP code
such as 1000 out of Error::http_status.
…und the loop ArcGISPager::advance() added the requested resultRecordCount to the offset. ArcGIS clamps that request to the layer's own maxRecordCount, so a short page still flagged exceededTransferLimit made the next resultOffset skip every record in the gap -- a successful result with a silent hole. inspect_arcgis_envelope already had the parsed root, so it now reports the features-array length too and advance() consumes it. The loop also had no ceiling: a server or caching proxy that keeps reporting truncation looped forever, accumulating one raw page body per iteration, and the int32 offset would eventually overflow. Paging now fails with ServerError when a truncated page carries no records, and after ArcGISPager::max_pages() (100) requests while the server is still truncating. offset() is int64. Verified live against SPC_wx_outlks layer 1 with resultRecordCount=1: f=json reports exceededTransferLimit at the root, f=geojson at the root and under properties. Both placements are now read.
The two-argument parse_fire_weather(body, day) defaulted to FireWeatherLayer::Outlook, so a dry-thunderstorm body decoded dn=5 as "ELEV" (severity 1) rather than "IDRT" (severity 0) -- the dn label confusion 0.2.0 fixed, still reachable through the public API. The captured day-1 and day-2 fire-weather payloads carry no LABEL key at all, only the numeric dn band index that both layers use with different meanings, so nothing in a body identifies its layer and there is no safe default. The overload is removed; the layer is now a required argument. A regression test asserts the ambiguity directly: both captured payloads contain no LABEL, and the same body yields ELEV/CRIT/EXTM through the outlook layer and IDRT/SDRT through the dry-thunderstorm layer. It also gives arcgis_day2_fire_weather.esri.json its first reader.
…r hanging Three defects on the ArchiveClient path. Percent-encoding: ts, sts, ets and wfo went into IEM query URLs raw 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 value injected extra parameters. All four now go through the existing percent_encode. SIGFPE: RateLimiter::Config is a public aggregate with no validation, so a refill_interval of zero reached 'elapsed / refill_interval' in refill() and crashed on the first try_acquire(). The constructor clamps it, and clamps initial_tokens to max_tokens. Hang: acquire() with no max_wait polls try_acquire() every 10 ms, and try_acquire() returns false permanently once a configured daily_limit is spent -- only a UTC-midnight reset clears it -- so the caller's thread spun until midnight. Waiting cannot help, so acquire() and acquire_for() now fail immediately in that state. ArchiveClient additionally gives its limiter a 5 s bound, which makes the documented RateLimited result reachable, and acquires a token inside the retry lambda so every attempt is paid for.
…dependency
std::stod delegates to strtod, which honours the process LC_NUMERIC. On
a comma-decimal host -- any app calling setlocale(LC_ALL, "") on a
de_DE/fr_FR/pt_BR desktop -- strtod("0.15") consumes only "0" and
returns 0 without throwing. Verified locally under LC_NUMERIC=de_DE.UTF-8.
The live Day 4-8 static feed carries its probability only as the string
LABEL "0.15", so normalized_probability returned 0, parse_day4_8's
probability > 0.0 gate dropped the feature, and day4_8() returned a
successful but silently empty payload. has_zero_dn had the mirror image:
a string dn of "0.0" failed its full-consume check, so the fire-weather
no-risk sentinel shipped as a real band. Both are pinned by new tests
that skip when de_DE.UTF-8 is unavailable.
spc::detail::parse_double is locale-independent: std::from_chars where
the standard library implements it for double, and a classic-locale
stream otherwise. libc++ only ships floating-point from_chars from
version 20 and this project's clang-tidy job builds against libc++ 18,
so the fallback is not hypothetical; both branches were compiled and the
full suite run against each.
Output is byte-identical to a C-locale stod for every value in the
fixture corpus, which is what the spc-data byte-identity gate covers.
The parse is narrower than stod only in rejecting leading whitespace and
a leading '+' -- neither appears in any SPC payload.
…etry-After
HttpClient::get("file:///etc/hosts") returned the file's contents:
is_absolute_url() recognises only http:// and https://, so a file:// URL
was treated as relative, concatenated onto the empty default base_url,
and passed to libcurl unchanged. CURLOPT_PROTOCOLS_STR and
CURLOPT_REDIR_PROTOCOLS_STR now pin the transport to http and https,
with MAXREDIRS at 10.
ClientConfig gains max_response_bytes (64 MB) so an unbounded IEM window
cannot buffer an arbitrary body, a full JSON AST of it and the payload
at once. Note: this ceiling has no automated test, since exercising it
needs a real server and unit tests must not touch the network.
with_retry now reads the Retry-After it was already capturing (the
delta-seconds form, clamped to max_delay) instead of retrying a server
that asked for 60 s after 200 ms. The jitter multiply moved before the
max_delay clamp so a delay cannot exceed the documented ceiling, and
max_attempts == 0 performs the request once rather than reporting a
network error for a request never made.
The review also claimed max_attempts == 255 loops forever. It does not:
the 'attempt < max_attempts' guard returns at 255 before the counter can
wrap. A test pins that.
The default User-Agent is generated from PROJECT_VERSION, with a test
that fails when the two drift.
Default Release builds passed -march=x86-64-v3 whenever the compiler accepted it. check_cxx_compiler_flag says nothing about the CPU the built library will run on, and this SDK installs an exported package, so the build host and the run host are routinely different -- 'make build' produced an archive that SIGILLs on pre-Haswell/pre-Zen hardware. It is now behind SPC_TUNE_X86_64_V3, defaulting off, with -mtune=generic. ci.yml gains a top-level 'permissions: contents: read' (it runs on pull_request and executes third-party build scripts) and pins both actions to full commit SHAs. ArcGISClient::query_storm_reports() is a permanently failing stub; it now carries the deprecation attribute, doc comment, README line and CHANGELOG entry its sibling query_active_watches() already had. query_fire_weather's all-or-nothing contract across its two merged layers is documented and pinned by a test. The Esri/GeoJSON parity gate now reads all seven captured fixture pairs instead of one; the test named for probabilistic parity never opened the Esri side at all. CLAUDE.md and CONTRIBUTING.md list the fixtures-check and lint-md gates CI enforces, CONTRIBUTING names all seven jobs, make help lists every target, and the README gains src/core/, query_layer, and the heading the CHANGELOG cross-reference pointed at.
bugprone-* is enabled with WarningsAsErrors: '*', and the two int32 constructor parameters are adjacent and same-typed. Matches the suppression the repo already uses on ArchiveClient::storm_reports. Also names the SPC_VERSION_STRING fallback literal in the CLAUDE.md release checklist: the generated definition covers anyone linking the targets, but the header's #ifndef fallback is not test-covered.
clang-tidy's bugprone-easily-swappable-parameters flagged the adjacent 'double& value, std::size_t& consumed' out-parameters as convertible and swappable. They are: transposing them at a call site compiles. Returning a ParsedNumber removes the hazard instead of suppressing the warning, and reads better at both call sites. Both parse paths (from_chars and the classic-locale stream fallback) rebuilt and the full suite run against each.
The LocaleIndependence tests GTEST_SKIP when a comma-decimal locale is unavailable, which on the Ubuntu runners is always -- so the locale defect they guard was only ever checked on a developer's macOS box.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes findings from an exhaustive review of the SDK: all 7 high findings and
most of the mediums. Every change is test-first — the failing test is in the
same commit as the fix.
Breaking changes
parse_fire_weather(body, day)removed. It silently assumedFireWeatherLayer::Outlook, so a dry-thunderstorm body decodeddn=5as"ELEV"(severity 1) instead of"IDRT"(severity 0). The captured day-1and day-2 payloads carry no
LABELat all, only the numericdnboth layersshare, so nothing in a body identifies its layer. Pass the layer explicitly.
ArcGISPager::advance(bool)→advance(std::int32_t records_returned, bool).ArcGISPager::offset()returnsstd::int64_t(wasstd::int32_t).ErrorCode::NotFound, notFeedUnavailable. Consumers that branch onis_feed_unavailable()forArcGISClientorArchiveClientshould treatNotFoundas a fault and alertrather than clearing rows.
StaticFeedClientis unchanged.A crash and a hang
Both are in
RateLimiter, which is public and installed. Neither is reachablethrough
ArchiveClient, whose config avoids both inputs.RateLimiter::Configis a public aggregate withno validation, so
RateLimiter{{.refill_interval = 0ms}}reachedelapsed.count() / config_.refill_interval.count()inrefill()on the firsttry_acquire(). UBSan against the pre-fix code reportsrate_limit.cpp:34:51: runtime error: division by zero. The consequence isarchitecture-dependent: x86-64 traps (SIGFPE), which is where CI runs and what
the removed
-march=x86-64-v3baseline targeted, while AArch64 silentlyyields 0. Undefined behaviour either way. Fixed with a constructor clamp;
initial_tokensis clamped tomax_tokensin the same place. The repo's ownASan/UBSan job would have caught this, but no test ever built such a limiter.
daily_limit > 0and nomax_wait,acquire()polledtry_acquire()every 10 ms, andtry_acquire()returns false permanentlyuntil the next UTC-midnight reset — so the calling thread spun until midnight.
Waiting cannot help in that state, so
acquire()/acquire_for()now failimmediately.
ArchiveClientadditionally bounds its wait at 5 s, which makesthe documented
ErrorCode::RateLimitedresult reachable. This is distinctfrom an empty bucket, which was and remains a bounded wait.
Both reproduced first as failing tests: the hang as a ctest timeout, the
division by zero via UBSan on the pre-fix source.
The rest
Error::from_responsemapped every 404 toFeedUnavailable, soErrorCode::NotFoundwas unreachable and a retiredendpoint looked like a quiet weather day. It now takes a
Feed404argument;only
StaticFeedClientpassesNoActiveOutlook. ArcGIS logical errors, whicharrive over HTTP 200, go through a new
Error::from_arcgisinstead of theHTTP mapper — verified live: a renamed MapServer path answers HTTP 200 with
{"error":{"code":404}}, and a bogus layer id answers with code 400.page size, and is bounded: a truncated page with no records, or a server that
never stops truncating, fails after at most 100 requests.
std::stodhonoursLC_NUMERIC, so on a comma-decimal host theDay 4-8 static feed parsed to a successful but empty payload and the fire
weather no-risk sentinel shipped as a real band. Both now use a
locale-independent
detail::parse_double.ts,sts,ets,wfo) are percent-encoded.HttpClientreached the local filesystem:file:///etc/hostsreturnedthe file's contents. libcurl is now restricted to
http/https.Retry-After; jitter can no longer exceedmax_delay;max_attempts = 0performs the request once.-march=x86-64-v3.ci.ymlgainspermissions: contents: readand SHA-pinned actions.Gates
Release 65/65, Debug + ASan/UBSan 65/65, clang-format clean,
cpp_auto_auditclean, markdownlint 0 errors,
fixtures-check28 verified, consumer smoke(installed package + FetchContent) exit 0.
Not covered by a test
ClientConfig::max_response_bytes: needs a real server, and unit tests mustnot touch the network.
LocaleIndependencetestsGTEST_SKIPwherede_DE.UTF-8is absent,which is likely every Ubuntu job.