Repository navigation
Conversation
OpenWeather no longer sells One Call 3.0, so a new API key 401s on the hardcoded data/3.0 endpoints. Call 4.0 instead and map its split payloads back onto the existing current/hourly/daily, timestamp, daily aggregation, and overview contract. Signed-off-by: GokayAI <60583610+gokay-ai@users.noreply.github.com>
Static checks failed on ESLint (prettier/prettier) and Prettier --check for six formatting violations across the weather service, its specs, and the api-side tool spec. Applied eslint --fix; no logic changes.
![lia-by-librechat[bot]](https://avatars.githubusercontent.com/in/4931833?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pushed 58a552566 to this branch to fix the red Static checks lane: ESLint (prettier/prettier) and Prettier --check were failing on six formatting violations in service.spec.ts, normalize.ts, normalize.spec.ts, and api/.../specs/openweather.test.js. eslint --fix only, no logic touched. Everything else on the previous head was already green.
Review below.
What checks out
I verified the endpoint surface against OpenWeather's own 4.0 docs rather than trusting the diff: /data/4.0/onecall/current, /data/4.0/onecall/timeline/1min, /timeline/1h, /timeline/1day are the real paths, start and cnt are real parameters, geo/1.0/direct is unchanged, and data.alerts really does carry alert IDs with a separate /data/4.0/onecall/alert/{alertId} endpoint for detail. The mapping in normalize.ts matches the documented 3.0 → 4.0 field moves.
Two things I want to call out as good: stripping next/prev before serialising (those URLs embed appid, so returning them would hand the API key to the model) and constraining followed pagination to https://api.openweathermap.org/data/4.0/onecall/, both with tests. And moving the logic into packages/api/src/tools/weather behind a thin LangChain wrapper, with 49 unit tests on fixtures, is the right shape for this file.
Blocking-ish
1. Legacy 3.0 keys now break, and the PR assumes they cannot exist. The body says "no 3.0 fallback; that product is gone", but OpenWeather's launch note is explicit that existing One Call 3.0 subscriptions keep working and that 4.0 must be subscribed to separately. #16064 is about new keys not being able to buy 3.0. So this turns a currently-working deployment (3.0 subscription, no 4.0) into 401s on every call: the same bug, mirrored. An env escape hatch (OPENWEATHER_ONECALL_VERSION, default 4.0) or a 401-triggered retry against 3.0 would make the fix non-breaking in both directions. This is my main ask.
2. current_forecast went from 1 billed API call to 4–7. current + 1min + 1h (up to 3 pages) + 1day, plus the geocode. One Call by Call bills per call and the free daily allowance counts calls, so identical user traffic now costs roughly 5x and exhausts the free tier ~5x faster. Nothing in the tool or the docs tells the operator that. Two cheap mitigations: do not fetch 1min unless it was asked for (60 minutes of precipitation is rarely what an agent needs, and it is a whole extra billed call on every weather question), and stop at 24 hours by default so hourly is one page instead of three. At minimum this belongs in the PR body and the OpenWeather docs page.
3. Promise.all makes any one endpoint failure fail the whole action. 4.0 is a six-endpoint family; if the key's plan does not cover 1min, or that single endpoint 429s, current_forecast returns a bare error string and throws away the current conditions it successfully fetched. Promise.allSettled, keeping what succeeded and noting what did not, is both more robust and directly in the spirit of the 401 bug this PR fixes.
Correctness
4. Alerts silently disappear if IDs are not strings. asStringArray filters to typeof item === 'string'. The docs say "array of weather alert IDs" without pinning the JSON type; if OpenWeather emits numbers, every alert is dropped with no error and no log. Accept Array<string | number> and stringify. Separately, an agent asked "any weather warnings?" now gets opaque IDs where 3.0 gave it full government alert objects — resolving them through /onecall/alert/{alertId} (only when alerts is non-empty, so no extra cost in the common case) would close the real regression.
5. daily_aggregation fabricates per-period precision. humidity.morning/afternoon/evening/night all receive the same single daily humidity; cloud_cover.afternoon, pressure.afternoon and wind.max likewise present a daily value under a name that claims something narrower. The shape survives, but the model cannot tell which numbers are measurements and which are the same number repeated four times. I would leave the fields 4.0 cannot fill as undefined (they drop out of the JSON) rather than fill them with a relabelled daily mean. The temp.morn/day/eve/night → morning/afternoon/evening/night mapping is legitimate and should stay.
6. Day boundaries are UTC now, silently. convertDateToUnix produces UTC midnight and selectDailyRecord matches on utcDateString(record.dt), falling back to records[0]. 3.0's day_summary took tz and aggregated the local day. For a non-UTC location the fallback can return a different day's record, still stamped with the requested date in the output. Also tz is still advertised in openWeatherSchema but is now ignored entirely, so the model will keep sending a parameter that does nothing: either honour it or drop it from the schema.
7. The synthesized overview bypasses rounding. synthesizeOverview builds the sentence from raw record.temp before stringifyResult runs roundTemperatures over the object, so the text carries decimals. normalize.spec.ts encodes this: 'Temperatures range from 7.4°F to 21.9°F.'. Every other action rounds, and the help payload still promises "All temperatures are rounded to the nearest degree".
Minor
EXCLUDE_PARTSacceptsalerts, butexclude=alertshas no effect now that alerts ride inside records.minutelyis fetched with nocntand no pagination, so 3.0's 60 minutes probably becomes one server-default page. Pincntexplicitly, or drop the endpoint per point 2.stripPaginationintimestampForecastis a no-op:fetchTimelinealready returns a fresh object with nonext/prev.OPEN_WEATHER_TOOL_DESCRIPTIONlives inservice.tswhileopenWeatherSchemastays inregistry/definitions.ts, which now imports from~/tools/weatherwhiletools/index.tsre-exports both. The circular-dependency lane is green, but keeping the description next to the schema would avoid the barrel edge entirely.api/app/clients/tools/structured/specs/openWeather.integration.test.jswas not updated. It happens to still hold (it no-ops without a key, and the fields it asserts survive), but itsoverviewcase now asserts a synthesized sentence rather than OpenWeather's generated summary, which is worth a comment so the next reader is not misled.roundTemperaturesmutates its argument and returns it; harmless on a freshly parsed object, but the signature reads pure. Pre-existing.
OpenWeather still supports existing 3.0 subscriptions; 4.0 is a separate product. Default remains 4.0 for new keys, with OPENWEATHER_ONECALL_VERSION=3.0 as an explicit escape hatch. On 4.0, current_forecast no longer fetches 1min unless asked, stops hourly pagination at 24h (one page), keeps successful parts via Promise.allSettled, resolves alert IDs, honours tz for local day boundaries, and leaves 4.0-missing daily fields undefined instead of fabricating them. Signed-off-by: GokayAI <60583610+gokay-ai@users.noreply.github.com>
58a5525 to
8a76281
Compare
When every current_forecast endpoint fails, return the original OpenWeather status error instead of prefixing it with the first part name. Signed-off-by: GokayAI <60583610+gokay-ai@users.noreply.github.com>
There was a problem hiding this comment.
Re-reviewed exact head aabf77660565cae5896ea6f6b73513e36a92e79c. Thank you for the follow-up. The explicit 3.0 switch, partial-result handling, omitted fabricated daily fields, overview rounding, and exclude=alerts handling address the earlier concerns.
Four remaining findings are inline. The hourly-page recommendation in my earlier review was incorrect; I own that correction. The numeric-ID concern from that review was hypothetical, not an observed provider failure. This round uses the official payload examples and focused reproductions.
Verification: weather + telemetry tests: 63 passed. Four additional temporary regression reproductions fail on the four cases below. Changed-file ESLint and Prettier pass. Local packages/api tsc --noEmit fails on shared dependency/type mismatches and missing zod-openapi outside the weather changes; the exact-head CI TypeScript check passes. Static checks and all test shards are green; production runtime smoke is still pending at posting. No live-key API calls, local JS wrapper test, or full local static-checks runner in this review. No production code changed or pushed.
Graph scope: fork head is not indexed; dependency lookup used dev at fe79265. Findings were read and reproduced from the checked-out PR head, not inferred from that graph.
Normalize 4.0 multilingual alert descriptions from the official payload, allow opaque alert IDs that contain colons while still rejecting path traversal, request at most 20 hourly records per billed page, and return no-data when a requested daily date is missing instead of records[0]. Signed-off-by: GokayAI <60583610+gokay-ai@users.noreply.github.com>
The day-summary fixture used UTC midnight, which is the previous local day in America/New_York. With no records[0] fallback, the record must carry the requested local date. Signed-off-by: GokayAI <60583610+gokay-ai@users.noreply.github.com>
|
Addressed the four review notes in 81fcd32 — multilingual alert descriptions, opaque alert IDs with encodeURIComponent, 20-record hourly page, and no daily date fallback to records[0]. Ready for another look when you have a moment. |
There was a problem hiding this comment.
Reviewed exact head 81fcd32d46717f7b1c25a03bcaab927ff34b53a6. The four previous findings are resolved. One remaining data-integrity issue prevents my approval.
I re-ran the previous reproductions: multilingual alert text survives, the opaque colon-containing ID resolves, the documented one-page contract is now 20 records, and missing dates no longer substitute another day. All four pass. The author fixed what was requested; the precipitation mapping below is something I missed in the earlier rounds, not an incomplete response to those threads.
The broader source review covered the JS wrapper and registry/barrel exports, injected per-call credentials, both product-version paths, concurrent forecast requests and partial/all failures, alert deduplication/resolution/exclusion, pagination bounds, date selection, normalization, and serialized model output. There are no new database/persistence writers to migrate. Request cancellation/timeout/retry behavior remains limited; no live network behavior was validated. Graph dependency evidence is from indexed dev fe03f124f0a83372832a72692136ce19d9b1baa6, not this unindexed fork head. Findings and tests use the actual checked-out head.
Verification
| Check | Result |
|---|---|
| Weather + telemetry suites | 69 passed |
| JS OpenWeather wrapper against this head's locally built API (explicit Jest package mapping) | 13 passed |
| Previous diagnostic reproductions, adjusted to the accepted 20-record contract | 4 passed |
| New daily precipitation contract reproduction | 1 failed, confirming the inline finding |
| Local packages/api tsdown build | Passed |
| Local static-checks against PR merge base | ESLint, Prettier, import sorting, package.json validation, circular dependencies passed |
| CI at this head | 23 passed, 5 skipped, none pending/failing |
| Local packages/api tsc --noEmit | Failed on shared dependency/type mismatches outside the weather changes; CI TypeScript passed |
Not run locally: live-key OpenWeather integration, full repository suites, static-checks --full (config migration, unused i18n/dependency gates). Local diagnostic files only; no production edits, commits, pushes, or merge.
One Call 4.0 1-day rain.1h / snow.1h are mm/h rates, not a daily accumulation. daily_aggregation no longer copies those rates into precipitation.total; the field is omitted unless a documented daily total exists. Signed-off-by: GokayAI <60583610+gokay-ai@users.noreply.github.com>
Signed-off-by: GokayAI <60583610+gokay-ai@users.noreply.github.com>
|
Follow-up to your 2026-09-22 review: the remaining precipitation finding is addressed in Ready for another look when you have a moment. |
Keep both the weather and compare tool exports. Signed-off-by: GokayAI <60583610+gokay-ai@users.noreply.github.com>
Summary
A new OpenWeather key 401s on every tool call: the tool still hits
https://api.openweathermap.org/data/3.0, and OpenWeather no longer sells One Call 3.0. After this change the tool uses One Call API 4.0 by default and maps the split 4.0 payloads back onto the existing current/hourly/daily, timestampdata[], daily-aggregation temperature, and overviewweather_overviewcontract so agents keep the same fields.Existing One Call 3.0 subscriptions are still valid — 4.0 is a separate product. Operators with a 3.0-only key can set
OPENWEATHER_ONECALL_VERSION=3.0(documented in.env.example). Default stays4.0for new keys.Fixes #16064
How it works
next/prevpagination URLs are dropped so the API key never reaches the model. Hourly pages are followed only onhttps://api.openweathermap.org/data/4.0/onecall/, and by default we stop after one page (at most 20 hourly records, the documented 1h response cap). Alert detail IDs keep the fixed origin/path and areencodeURIComponent'd; colons and other opaque identifier characters are allowed, while/,.., and encoded path separators are rejected. 4.0 alertdescriptionarrays ({ language, description }) are flattened so the model still receives the hazard text. A requested daily date with no matching record returns no-data instead ofrecords[0].One Call 4.0 bills per HTTP call.
current_forecastwent from 1 billed call (3.0) to several; the default path is now current + hourly + daily (about 3 calls plus geocode), not 4–7. Minute precipitation is skipped unlessexcludeincludes+minutely.Type of change
As the code stands:
OPENWEATHER_ONECALL_VERSION=3.0keeps a working 3.0 subscription on/data/3.0.current_forecastkeeps successful 4.0 parts when another endpoint 401s/429s.exclude=alertsskips that).description(string fixtures still work).daily_aggregationmapstemp.morn/day/eve/nightand leaves 4.0-missing period humidity/cloud/pressure/wind undefined. A missing requested date is no-data, not another day's record.tz(IANA name or ±HH:MM) as the local day boundary.In-repo OpenWeather docs live in
.env.exampleand the toolhelppayload (the public docs site is a separate repo).Testing
cd packages/api && npx jest src/tools/weather --coverage=false— 43 passedcd packages/api && npx jest src/telemetry/sdk.spec.ts --coverage=false— 26 passedcd api && npx jest app/clients/tools/structured/specs/openweather.test.js --coverage=false— 13 passedpackages/apinpx tsc --noEmitafter building data-provider and data-schemas — cleannpm run static-checks -- --against origin/dev— ESLint, Prettier, import sorting passedTest Configuration:
Node v24.16.0. Fixtures/mocks only; no live OpenWeather key.
Checklist