Replace the four skills with one owid skill backed by reference files - #25
Open
pabloarosado wants to merge 33 commits into
Open
pabloarosado wants to merge 33 commits into
pabloarosado wants to merge 33 commits into
Conversation
The repo shipped search-charts, fetch-chart-data, joining-data and
owid-catalog. They covered adjacent ground and competed for the same prompts,
and two of them pulled in dependencies (duckdb, a Python library) most users
do not have. This folds them into a single `owid` skill: a ~130-line SKILL.md
with the workflow, the hard rules and the traps, plus four references the
agent reads on demand:
references/search-api.md every /api/search parameter, both response
shapes, the ?tab= mapping, page search
references/chart-data-api.md .csv/.metadata.json/.readme.md/.zip/.png/.svg,
the filter parameters, the metadata schema
references/data-format.md entity/code/year conventions, regions,
joining with other data (from joining-data)
references/embedding.md iframe snippet, PNG fallback, attribution
Everything documented was checked against the live API and the grapher
source, which turned up corrections to the planning doc: `resultType` is not
a search parameter (it is silently ignored; `type=charts|pages` is), page
search defaults to `pageTypes=article,about-page`, `countries`, `topics` and
`requireAllCountries` exist, `hitsPerPage` caps at 100, on map-default
charts `country=` needs `tab=chart`, and `ignoreProjections` has no effect
on the CSV. Every request now carries a fixed User-Agent so usage of the
skill is visible in OWID's logs.
Evals: one contract.sh with 141 checks replaces the four, and can now pin
claims to the reference files (doc_contains / doc_table_covers) as well as
SKILL.md; new http_status and content_type helpers cover 400s and images.
triggers.json has 6 positives across the use cases and 4 near-misses.
`make validate` also checks that relative links inside skills/ resolve and
that references/ never mention eval files. Prerequisites shrink to curl and
jq; CI no longer installs duckdb.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…inst the API
chart-data-api.md and data-format.md described the same thing from two sides:
one the endpoints, the other what comes back from them. They are now a single
references/data-api.md, rewritten and checked claim by claim against the live
endpoints. What that turned up:
- The Code column has three families, not one. Besides OWID_ codes there are
UN_ and WB_ codes for regions those bodies define, and about a quarter of
the entities in OWID's largest charts have no code at all: Scotland,
Northern Ireland, every (FAO) region, every urban/rural split. Joining on
Code drops them silently, so the file says so and the joining section is
about that rather than about recipes.
- The CSV strips accents. It says "Cote d'Ivoire" and "Curacao", and holds no
non-ASCII bytes at all. SKILL.md claimed the opposite, using those two names
as its examples. The metadata and readme do carry non-ASCII (en dashes and
curly quotes in the citations), which is what must be passed through.
- conversionFactor and tolerance are in none of ten charts' metadata, so the
SKILL.md rule about not re-applying conversionFactor described a field that
is not there. Removed, along with the tolerance guidance.
- Multi-dimensional charts: a wrong dimension value returns 500, a partial set
returns 500, but a wrong parameter NAME returns 200 and the default view, so
you get the wrong indicator with no error. They also have no .config.json.
On explorers even a wrong value is silent.
- tab=table undoes country filtering.
Citation guidance moves into the metadata section and gets stronger: say where
the numbers came from at least once per session even when nobody asked, using
citationShort because it reads as prose and names the producer before OWID.
Both citation forms are shown, with the CC BY terms and why the producer must
be credited.
Per-capita and GDP-merging are out of scope for this skill and are gone from the
repo: the recipe and reference series, the capability clause in both
descriptions, one trigger query, one eval case and its fixture, the four
reference-chart contract checks, and the counts in evals/README.md.
Contract tests follow the file: 15 new checks for the behaviours above, the
code-family and encoding claims pinned to real charts, and the four
reference-series checks dropped. One check is deliberately left failing --
"the embedding reference documents the image parameters" -- because the image
parameter table was in the merged file and embedding.md has not been written
yet. Those parameters also need correcting: imType=og is 1200x627, imType=
thumbnail is 1200x640 and is not small, imDetails=1 has no effect, and an
unknown imType is ignored.
SKILL.md's description changed, so `make triggers` should be re-run before this
merges. The trigger set is now 9 queries with 5 positives.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The headings had grown by accretion. Two started with "Always", one nested inside the other; "What you can ask for" and "What comes back" read as a pair but one was about building a request and the other about the CSV, while the metadata's shape sat under "What is in it"; and "Traps" did not say why anyone should read it. Renamed throughout: The URL -> How a request is built Always get the metadata -> Start with the metadata What is in it -> The metadata fields Always cite the original producer -> Citing the source What you can ask for -> Suffixes and parameters All the data, or part of it -> How much data to ask for What comes back -> Reading the CSV Codes -> The Code column Time -> Years and days Text encoding -> Text and accents Traps -> Silent failures Nothing links to these anchors, so the rename is self-contained. Two things fell out of it. "Silent failures" now opens by saying what the section means -- each case returns a 200 and a plausible file, with nothing to say it is wrong -- and the one bullet that was not a failure, multi-dimensional charts having no title, is dropped: it is not a failure and the chart.title row already says it. columns.*.type was documented as Numeric, Integer, Categorical, Ordinal. Categorical is never emitted: variableTypeToColumnType in the grapher source maps the five database types onto String, Numeric, Integer, NumberOrString and Ordinal, with Continent as a special override. A sample of 163 charts (407 columns) returned Numeric, Integer, Continent, SeriesAnnotation, NumberOrString, String and Ordinal, and no type field at all on six columns. The row now says that, and a contract check pins the list so a new value shows up as a failure rather than as stale prose. Also dropped, on review: the activeFilters paragraph, the descriptionProcessing bullet, and two sentences that carried no information. Filters passed to .metadata.json change only activeFilters and chart.originalChartUrl -- nothing about the data, not even timespan -- so the examples here and in SKILL.md no longer filter the metadata request. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The skill told people to use curl and jq. That assumes a shell, and it assumes
jq is installed, which it is not on Linux or Windows by default and only became
true on macOS with version 15. Plenty of people will reach this API from Python,
R, a notebook or a browser, and nothing about the API needs a particular client.
Every example is now the URL and what it returns. The User-Agent is stated as a
header rather than as a curl flag, because that part is a real requirement and
the rest never was.
- allowed-tools is dropped from SKILL.md. It is deprecated, and it listed
Bash(curl), Bash(jq), Bash(cat), which assumed a Bash-capable agent.
- "Needs only curl and jq" is gone from both descriptions.
- The quick reference is a list of URLs instead of a shell script.
- Advice that assumed a filesystem ("save the response to a file and process
it with jq or awk") now says to keep the response out of context and
process it however the project already does.
- install-prerequisites-macos.sh existed only to install jq on macOS via
Homebrew, so it is deleted, and the Prerequisites sections in README and FAQ
now say there are none.
- AGENTS.md's rule changes from "skills must rely on curl and jq" to
documenting endpoints as URLs and naming no tool. The repository's own tests
may use whatever they like; the shipped skill may not.
owid-catalog is dropped as a recommendation everywhere. The name survives only
where README and FAQ list the four skills this one replaced, which is history
rather than a pointer.
embedding.md carried the User-Agent only inside the shell variable that was
deleted, so it now states the header in prose. Without that, a reader of that
file alone would never have known to send it.
search-api.md still has six curl-and-jq examples and is deliberately untouched
here; it is being edited in parallel and gets the same treatment in its own
commit.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Same change as the previous commit, kept separate because this file is being edited in parallel and this commit can be dropped or rebased past on its own. The six recipes were curl piped into jq. They are now the URL plus a sentence about what to do with the response, which says the intent rather than one implementation of it: "sort the results by date yourself for newest first" instead of a jq sort_by, and "keep the results whose authors array contains it" instead of a jq select. The User-Agent line stops being a curl -A flag and becomes the header it actually is. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
marketplace.json carries two: one for the marketplace, one for the plugin. SKILL.md carries a third. They had drifted into three different claim sets -- the marketplace one left out explorers, embedding and the no-API-key line, and said "cite it correctly" where the others said "cite the original sources". All three now share one capability clause, differing only in how they open. "Cite the original sources" is the wording everywhere, since naming the original producer rather than OWID is the point the skill keeps making. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… single-skill branch Resolves the four files both sides edited (AGENTS.md, Makefile, README.md, evals/README.md), keeping this branch's single-skill text and adding main's plugin manifests, `make install` and the behaviour-eval layer. Moves the behaviour case to evals/skills/owid/ and points its skill-fired grader at `owid`; rewrites the install and eval sections that still described four skills; updates both plugin manifests' descriptions. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Brings in the three commits that stop prescribing curl and jq, document the search recipes as URLs and unify the descriptions. Both plugin manifests now carry the same description as the marketplace, and the README and evals README no longer assume a shell. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Ported from the output-eval runs into the claude plugin eval format: data insights search (the baseline never finds the page search), a topic OWID does not cover (the baseline guesses slugs that 404), a year the data does not reach (the skill says to give the latest year and name UN IGME), and recent articles by one author. Route graders are marked arm: with-only so they show as indicators without inflating the score. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Several claims in the data reference were generalised from too few charts. Each of these was checked across a wider sample and the wording changed to match. Sub-annual data. The file knew only Year and Day; there is also a Month column, holding YYYY-MM, on charts like global-co2-concentration, sea-surface-temperature and monthly-temperature-anomalies. There is no Week column: weekly figures are dated by day. One chart can mix granularities, as global-co2-concentration does with a monthly and an annual average on the same Month axis. timespan is an empty string, not a missing field, on every sub-annual chart checked, so the advice to read it before promising a year does not hold there; the coverage has to come from the first and last rows instead. time= needs full dates on Month and Day charts. time=2021-01 is ignored and returns everything. A bare number is read as an offset in days from an internal origin, so time=2020 on daily-cases-covid-region returns a single day in 2025. display.zeroDay exists in the full indicator metadata and varies by indicator (2020-01-21 on the COVID chart, 1850-01-15 on the CO2 one), but it is absent from .metadata.json and is not what the download endpoint interprets time= against, so the file tells agents not to work the date out at all. The map trap was described as a property of charts that open as a map. It is a property of the map tab: csvType=filtered copies whatever tab is active, and a map shows every country at one point in time. life-expectancy with tab=map returns 201 entities for 2023. It also collapses the time range, which the old wording did not mention. Entity codes. UN_ and WB_ were presented as the whole set beyond OWID_. A sample of 163 charts also turned up UNSDG_, WHO_ and PEW_, and the region-definitions page names more schemes than that, so the list is now explicitly open. Empty codes were described as a narrow category of about a quarter of entities; in the wider sample it is 630 of 964 and includes entities that are not places at all, such as individual wars and projects, so the share is no longer quoted. The regions bullet now links Definitions of world regions and picks up the rule that page states and the file did not: no brackets means OWID's own definition. The readme row says what only that file carries inline -- each source's producer, dates, URL and licence -- and points at fullMetadata for the same thing as structured data. tab= is marked filtered-only, like country= and time=. Contract tests follow each claim: the Month column, the empty timespan, date filtering, the bare-number behaviour, tab= being inert with csvType=full, and a code-shape check that allows any PREFIX_ family rather than a fixed three. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The repo described the skill in six different ways: two fields in marketplace.json, one in each of the two plugin.json manifests, an OpenAI shortDescription and longDescription with their own wording, SKILL.md's frontmatter, and README's tagline. They made overlapping but unequal claims -- some listed explorers, some did not; some mentioned embedding, some did not; some said "cite it correctly" where others said "cite the original sources". They are now one string, used verbatim wherever a description is served: Agent skills for working with Our World in Data: search published charts, explorers and articles, fetch the data and metadata behind them, embed charts, and cite the original sources. No API key. SKILL.md opens with the same clause and then continues into the trigger text it needs. README's tagline uses the same clause. longDescription is deleted rather than kept in sync, so description is the only source for every surface. No length limit is documented for the OpenAI shortDescription field, so it carries the full sentence. Also corrected: the author name was "Our World In Data" in two manifests and "Our World in Data" in the third. It is the lowercase form throughout now, which is how OWID writes it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`make install` resolved `owid/skills` with no ref unless BRANCH was passed, so running it from a feature branch silently installed main - and printed nothing about which ref it used. BRANCH now defaults to the checked-out branch, the ref is always explicit, and a `git ls-remote` gate turns an unpushed branch or a typo into an error instead of an install of something else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The plugin declared no icon, so the listing fell back to a generic book emoji. The ChatGPT interface block takes an icon, a logo and a brand colour; these are the assets ourworldindata.org serves for itself - the square "Data" mark it uses as its own app icon, and the square wordmark - plus the OWID navy. Claude Code's marketplace manifest has no icon field, so this is ChatGPT and Codex only. `make validate` now checks the asset paths resolve, because a bad path degrades back to the generic icon rather than failing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
One case per example prompt from the planning doc, in the agentskills.io evals.json format: the prompt, the automatic `checks` an eval runner can grade from a transcript, and the `assertions` a person grades from the answer. Five are "trap" prompts built on behaviours the skill documents (map-default country filter, filtered default selection, multi-dimensional URL parameters, data-insight search, no real match). Two small fixtures serve the fact-check and citation prompts. This is the source list for the behaviour cases under evals/skills/owid/; four of them are ported already. The runner that first executed these cases is not included: claude plugin eval does that job now. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The embedding reference now shows imType=og inside a URL rather than in backticks, so the drift check looked for punctuation that is no longer there. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Record the 25 example prompts as output-eval cases
curl treats 5xx as retryable, and on the CI runner's curl version an exhausted retry exits non-zero, so the two checks that expect a 500 from a bad multi-dimensional request saw HTTP 000 there while passing locally. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Generated from evals.json: each case keeps its prompt, turns the route checks into with-only tool_used / tool_order graders (indicators, not score), turns any answer check into a regex grader, and gives the judge the human checklist as its rubric. The three prompts that need a file get it under resources/ via case.yaml add_dirs. The User-Agent check is not ported: it cannot be met through WebFetch. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
SKILL.md had grown into a summary of both references: a five-step workflow, a rules list, a quick reference and five traps, all restating parameters the references document properly. It had already drifted -- three of its five traps were wrong by the time they were deleted. It said the search never returns zero hits, that map-default charts ignore country=, and that about a quarter of entities have no code. None of those survived checking. It is now 21 lines: what OWID publishes, a table saying which reference covers which task, and the one line that is true of the whole skill (everything is public; a login means the wrong URL). The description is untouched, since that is what decides triggering. search-api.md: the two TypeScript blocks become field tables. The intro points at owid-grapher/docs/search-api.openapi.yaml, the spec the endpoint is built against, rather than the docs site. topics takes one value only, and an unknown one returns a 400 listing every valid topic, which is the quickest way to get that list. originalAvailableEntities is dropped: it is set when the Algolia records are built but appeared on none of ~150 hits. Vocabulary and relevance advice moves into the opening, and the failure modes get their own section, including the corrected behaviour: a search CAN return zero hits, and a query where only some words match returns loosely related hits with closestMatches set, so a non-empty response is not a match. embedding.md: documents sizing only -- imWidth, imHeight, imSquareSize -- rather than every image parameter OWID's API accepts. The numbers are measured: 850x600 by default, one dimension implies the other, both give that exact box including portrait, and there is no clamping at either extreme. The claim that most chat surfaces block third-party iframes is softened, since it was never verified and asserts something about other people's products. Attribution keeps only what is true of an embedded chart and points at data-api.md for how to cite. The re-plotting section goes, and its row in the form table with it. Contract tests: the four assertions pinned to SKILL.md move to the references that now own those claims, replaced by one checking SKILL.md still links all three. Image sizing is pinned. 177 checks, none failing, none skipped -- the embedding image-parameter check that had been left deliberately red is closed by documenting sizing rather than by loosening the assertion. make triggers on claude-opus-5, one run: recall 100%, accuracy 89%, level with the previous measurement. Single run on one model, so it is evidence the cut did no harm rather than a figure to quote. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e behaviour The three cases that handed the model a file never reached it: the run's sandbox did not expose the resources directory, and the model spent its turns searching for the file. The article and draft are now part of the prompt text, which is also how a user would paste them. extract-claims no longer requires the skill to fire (its checklist says the task needs no data), and its rubric is written as concrete pass/fail conditions. The default small judge still failed a correct answer three votes to none; a sonnet judge passes it, so `make behaviour JUDGE=sonnet` exists for that, and MODEL= for the model under test. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Installing `owid` in Claude Code from any marketplace - this repo on GitHub included - failed with "conflicting manifests: both plugin.json and marketplace entry specify components". `strict: false` declares that the marketplace entry is the plugin's whole definition, which contradicts the plugin.json shipped beside it; the field is optional and its default, true, is what a repo that owns both files wants. Codex was unaffected, and `claude plugin validate` passes either way, so `make validate` now checks for it: the error only appears at install time, which CI never does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`make install` only ever registered the plugin with the Codex CLI, so trying a branch in Claude Code meant knowing to run `claude --plugin-dir .` yourself. The two halves are not symmetric, because the CLIs differ: `claude plugin marketplace add` takes no ref, so Claude Code gets a directory marketplace pointing at the worktree - which it reads live, uncommitted edits included - while Codex keeps resolving from GitHub at BRANCH, and still needs it pushed. Either CLI being absent now skips its half instead of failing the target. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six independent reviews checked these four files against the live API: three open-ended, three testing every claim in data-api.md against 30 published charts each. Nine findings came back from more than one reviewer. Everything below was measured, not argued. The claims that were wrong in ways that produce a wrong answer: columns is keyed by the indicator's title, not the CSV header. The chart renames its columns for display, so the match rate is about one in three. An agent following the old text to look up a unit before quoting a number failed on our own example chart. useColumnShortNames=true plus columns.*.shortName is the only reliable join, allowing for server-added suffixes. csvType=filtered is not a filter. It copies whatever view the chart opens in, and any view that is not a line or bar-over-time discards country= and collapses to one time point: maps, scatter, Marimekko, discrete bar. Ten to sixteen charts in every twenty-five. tab=chart rescues some and does nothing for the rest. The file now leads with csvType=full and filtering the rows yourself. On a filtered map view the Year column is the year you asked for, not the year of the number. The map carries values forward and puts the real year in an undocumented (Original Year) column. One chart had 102 of 104 rows mis-dated. chart.selection does not predict what filtered returns; it is often empty. earliest and latest resolve against the entities you selected, not the chart: the same chart gives 2023, 2022 or 2021 depending on country=, and entities without data that year vanish. A time= range outside the data returns the nearest year rather than nothing. The estimate/projection split is per country, not per chart: one chart's World projection starts in 2025 and another entity's in 2002. timespan is the indicator's range, an upper bound on what a chart returns; one column claims -10000-2023 while its CSV starts in 1950. A negative start also breaks a split on the dash. descriptionKey is absent on about half of all charts and is one string of markdown bullets, not a list. The file had sold the metadata largely on it. The global-co2-concentration second column is a rolling twelve-month average filled every month, not an annual figure filled once a year. Empty Code is a few per cent per chart, not "a large share" -- small enough that a bad join looks like it worked. The Code column can also be absent entirely on charts whose entities are not places. A misspelled dimension name on a multi-dimensional chart is a 500, because it leaves the set partial. Only a non-dimension parameter is ignored. Explorers fail silently in every case. The slug used to illustrate "never invent a slug" was itself invented. Newly documented: charts whose producer forbids redistribution return 403 on .csv and .zip while everything else returns 200 -- between one chart in fifteen and one in five, concentrated in health. It has its own section rather than a caveat on every other paragraph. search-api.md: countries= filters rather than prefers and returns zero hits for an ISO code or a misspelling; subtitle is optional and absent on the top hit of our own first recipe; variantName is an empty string, never missing; slug on an explorer hit is not a grapher slug; content is 100-170 words, not "a few hundred"; the author recipe pages and names its page types instead of silently returning articles only; closestMatches is a boolean whose nbHits is not a total; page search has a stop condition; and the claim that view parameters work on the data endpoint, which contradicted data-api.md, is gone. embedding.md: image sizes clamp silently at 4250x3000 and imSquareSize above about 4000 returns a 500. SKILL.md: "everything is free to reuse under CC BY" was wrong -- OWID's own work is CC BY, the data keeps its producer's licence. Also dropped a contract assertion that duplicated another, and the duplication the review turned up inside data-api.md itself. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three more reviews read the four files for over-detail, nitpicks and repetition inside a file. All three independently flagged the same thing first, and they were right: the prose carried the evidence for the corrections rather than the corrections. "Three independent samples put the match rate at roughly one column in three", "between one chart in fifteen and one in five", "ten to sixteen charts in every twenty-five", "102 of 104 rows", a walk-through of one chart returning 2023, 2022 and 2021. None of it changes what an agent does. All of it is gone; every rule it supported stays. Also cut, where a file said one thing twice: search-api.md had bullets for resultType, pageTypes and countries that restated their own parameter rows a screen further up. The rows keep the warnings, and the one tip that was only in a bullet -- check a country name against availableEntities -- moved into the countries row. The topic-list recipe went the same way: the topics row already says an unknown value returns a 400 that lists every topic. The eleven availableTabs values were written out three times. The mapping table keeps them and lost its third column, which explained what a scatter plot is. data-api.md named charts with no Code column twice in thirty lines with the same list, stated timespan's emptiness in the table and again below it, argued for useColumnShortNames from scratch a third time, and repeated "country= and time= need csvType=filtered" a fourth time under Silent failures after three bold table rows. embedding.md said an embedded chart is self-attributing in two places; SKILL.md closed by restating its own opening. Four calls went the other way from the reviewers, on Ed's decisions: The User-Agent block is now stated once in SKILL.md, which is always loaded, and removed from all three references. This is a genuine trade: a reference read on its own no longer mentions the header. Three contract checks asserting each reference carried it become one asserting SKILL.md does. The link to search-api.openapi.yaml is dropped -- the reference is meant to be sufficient, and an agent will not fetch a YAML spec mid-task. The ASCII diagram under the example URL is dropped; the bullets under it already name the slug, the suffix and the view. SKILL.md's licensing sentence stays. It is the one substantive thing that file says, and without it an agent that reads only SKILL.md believes everything is CC BY. One fix fell out of the review rather than the trimming: the "recommended base" line sat directly under a bullet telling the agent to use csvType=full, which read as a contradiction. Reworded. 708 lines to 647. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review — please focus on the four skill documents:
These are documentation an agent reads in order to use our public HTTP API, so what matters is: claims that are factually wrong or true only for the one example given, advice an agent would misapply, contradictions between the four files, and anything stated twice that could drift apart. The eval harness and the plugin manifests are not the target this time. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f009e0edcb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| Three things at the top level: `chart`, describing the chart as a whole, | ||
| `columns`, one entry per indicator, and `dateDownloaded`. Multi-dimensional | ||
| charts add `activeFilters`, echoing the dimensions you asked for. |
There was a problem hiding this comment.
Ordinary Grapher metadata also includes activeFilters when view parameters are supplied; for example, the contract check requests life-expectancy.metadata.json with country and time and requires both to be echoed. Restricting this field to multi-dimensional charts misstates the response shape and discourages agents from validating filters on normal charts; describe it as conditional on supplied filters rather than chart type.
AGENTS.md reference: AGENTS.md:L52-L56
Useful? React with 👍 / 👎.
Comment on lines
+305
to
+307
| - **`earliest` and `latest` are one year for the entities you selected**, not one | ||
| per country and not fixed for the chart. Ask for two countries whose data ends in | ||
| different years and you get the earlier one, with the other silently dropped. For |
There was a problem hiding this comment.
latest to choose the later endpoint
When selected entities end in different years, time=latest selects the later maximum year and drops entities without a value then; it does not choose the earlier shared year. For example, when one series ends in 2022 and another in 2021, the response contains 2022 and omits the latter entity. This wording makes an agent expect a valid same-year comparison and can lead it to overlook the missing entity.
AGENTS.md reference: AGENTS.md:L52-L56
Useful? React with 👍 / 👎.
Comment on lines
+352
to
+354
| - **Rows with an empty `Code` drop out silently.** Count your rows before and | ||
| after, and decide what to do with the ones you lost rather than not noticing | ||
| them. |
There was a problem hiding this comment.
When a CSV reader preserves blank codes as empty strings, or a merge library matches null keys, rows with the same year and no Code match one another instead of disappearing, potentially producing a many-to-many row explosion across unrelated entities. Counting only what was lost will not detect this corruption; agents should exclude blank keys or handle them explicitly using Entity, and check both row losses and gains.
AGENTS.md reference: AGENTS.md:L43-L45
Useful? React with 👍 / 👎.
Comment on lines
+392
to
+393
| - **A large `.csv` can fail with 503** and a Cloudflare body rather than a CSV. | ||
| Retry once, then fall back to `csvType=filtered`. |
There was a problem hiding this comment.
When a full CSV fails with 503, csvType=filtered is not an equivalent fallback: as this reference explains above, it copies the configured view and may collapse the time range or ignore requested countries. A caller that needs the full dataset can therefore receive a plausible but incomplete file. Recommend retrying later or partitioning verified requests rather than silently changing to filtered output.
AGENTS.md reference: AGENTS.md:L52-L56
Useful? React with 👍 / 👎.
| | Parameter | Values | Default | What it does | | ||
| |---|---|---|---| | ||
| | `csvType` | `full`, `filtered` | `full` | `full` gives every entity and every year in the chart. `filtered` gives what the chart itself shows. A misspelt value is ignored, so `csvType=fitlered` quietly gives you everything. | | ||
| | `country` | codes joined by `~`, e.g. `USA~GBR~OWID_WRL` | the chart's own selection | Which entities to include. **Only works with `csvType=filtered`**, and many charts ignore it even then — see below. Names work too, but codes are safer. Case matters: `usa` returns nothing. | |
There was a problem hiding this comment.
The claimed default contradicts the metadata section above: on map, scatter, Marimekko, and bar views, omitting country from a filtered request returns every entity drawn by the view, regardless of chart.selection. An agent relying on this parameter table can mistake a full cross-section for the chart's small highlighted selection, so the default should be documented as view-dependent.
AGENTS.md reference: AGENTS.md:L43-L45
Useful? React with 👍 / 👎.
| | `imHeight` | Height in pixels, likewise. | | ||
| | `imSquareSize` | Side length for a square image, with `imType=square`. | | ||
|
|
||
| Sizes are capped: ask for more than 4250×3000 and you silently get 4250×3000 back. |
There was a problem hiding this comment.
The blanket clamping advice does not apply to square images: with imType=square, an imSquareSize above roughly 4000 returns a server error rather than silently clamping to the stated maximum. Agents requesting high-resolution square artwork will therefore treat a real failure as unexpected; document this exception beside the square parameter.
AGENTS.md reference: AGENTS.md:L52-L56
Useful? React with 👍 / 👎.
Asked Codex to review, scoped by a free-text focus line naming the four skill documents. All six of its findings landed inside those files, so the steer works even though there is no path filter. Five are acted on here. latest was documented backwards. It resolves to the most recent year any selected entity has, so asking for two countries whose data ends in different years returns the later one and drops the entity that stopped earlier. The file said you get the earlier one. That is the worse way round to be wrong: an agent would have assumed the missing country had no data at all. activeFilters is not a multi-dimensional thing. It comes back on any chart when you pass filters, which the contract test already proves on life-expectancy, so restricting it to multi-dim both misstated the response and discouraged the one cheap check that confirms you got the view you asked for. The country row gave "the chart's own selection" as its default while the silent-failures section says a map, scatter, Marimekko or bar view returns every entity it draws. Now "depends on the view". The 503 advice told the agent to fall back to csvType=filtered -- the exact thing this file spends a section warning about, since filtered copies the view and can drop countries and collapse the time range. It still offers the fallback, but says it is not the same file and to say so to the user. The joining advice assumed the row count can only go down: "rows with an empty Code drop out silently... decide what to do with the ones you lost". Blank keys can also match each other and multiply rows across unrelated entities. Reworded so it does not point at one direction, without adding a new instruction. Not taken: documenting that imSquareSize above about 4000 returns a 500 rather than clamping. Two of three earlier reviewers called it a nitpick and it was cut deliberately; a 500 is at least visible, unlike the silent clamp that is documented. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The 21 ported cases had 20 turns and a 10-minute limit; with only WebFetch granted, the ones that build an HTML page, slides, a notebook or pipeline code cannot finish and burn the whole timeout. They now have 10 turns and 5 minutes, and the five artifact-building prompts are tagged needs-write so a WebFetch-only run can leave them out (--tag, or exclude by name). The states-the-data-year rubric assumed OWID's data would not reach 2024. It does (UN IGME 2025), so a correct 2024 answer was being failed. The rubric now accepts the 2024 figure when the data has it and only demands "latest year, stated" when it does not. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…g cases Every case listed allowed_tools: [WebFetch, Skill]. Read was not in it, so the model could load SKILL.md but was denied the reference files it points to, in every behaviour run so far. Read, Glob and Grep are read-only and are now listed in all 26 cases. Bash cannot be granted in this sandbox on a Mac with Docker Desktop, so a WebFetch-only run cannot finish tasks that need to process a full CSV. The three that do (data-from-url, whats-interesting, trap-filtered-default-selection) are tagged needs-bash, next to the five tagged needs-write, so they can be excluded until a machine can grant Bash. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…20 turns `claude plugin eval` can include tags but not exclude them, so the 18 cases that finish under a WebFetch-only grant carry `webfetch-ok`, and `make behaviour TAG=webfetch-ok` runs exactly those. The 10-turn cap set earlier cut off correct runs before they answered (a two-topic fact-check needs two searches, two metadata fetches and two data fetches); the ported cases are back to 20 turns, with the 5-minute timeout as the cost guard. Pilot, three cases, both arms, one run each, sonnet judge: $2.06 and 218 s at two in parallel. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Author
Behaviour eval run finished ( Report: https://claude.ai/artifact/2G9qRTosJM2R91PqrtyQkq (published private to the account that ran it; local copy at
How to read it: the skill wins clearly where it teaches something the model does not know (which producers sit behind a topic, the data-insight page search, the readme for provenance, the map-default trap). The five cases at 1.00/1.00 are outcomes a frontier model gets right unaided; the route differences show in the report as unscored indicators. The seven cases scoring low in both arms are not yet evidence about the skill: their Excluded from this run: 5 cases tagged 🤖 Generated with Claude Code |
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
3 participants
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.
Implements a new structure: instead of four skills that compete for the same prompts, one
owidskill with a shortSKILL.mdand reference files the agent reads on demand.What changes
Removed:
search-charts,fetch-chart-data,joining-data,owid-catalog(and their evals). The first two are folded into the references below; the joining knowledge lives on indata-format.md;owid-catalogis out (Python + a library few users have) and is mentioned in one line as an option for Python users.Added:
skills/owid/SKILL.mdreferences/search-api.md/api/searchparameter, chart and page response shapes,?tab=mapping, recipes (articles by author, topic pages, data insights)references/chart-data-api.md.csv.metadata.json.readme.md.zip.png.svg.config.json, all filter and image parameters, the metadata schemareferences/data-format.mdOWID_codes, regions, joining with other data, the reference population and GDP seriesreferences/embedding.mdPrerequisites shrink to
curlandjq. Every request carriesUser-Agent: owid-skills/1.0 (+https://github.com/owid/skills), which is the only usage signal we have (it answers the "can we measure skill usage" question from the doc, at least for the HTTP side).Corrections to the planning doc, found while verifying against the live API and the grapher source
resultType=writingis not a parameter of/api/search; it is silently ignored and returns charts. The parameter istype=charts|pages(anything else is a 400).pageTypes=article,about-page, so data insights and topic pages need an explicitpageTypes=. Valid values are taken from the API's own 400 error and checked in the contract test.countries,requireAllCountriesandtopics, and capshitsPerPageat 100. Relaxed no-match responses are flaggedclosestMatches: true.csvType=filtered&country=...is ignored unlesstab=chartis added. This bit the multi-dimreligious-compositionchart during testing.time=acceptsearliestandlatest; filters only apply withcsvType=filtered.ignoreProjectionsexists in the grapher source but has no effect on the CSV endpoint in production, so it is not documented.Evals
contract.shwith 141 checks (make testpasses). New helpersdoc_contains/doc_table_coverspin claims to the reference files, not justSKILL.md;http_statusandcontent_typecover 400s and image endpoints.claude-opus-5(make triggers RUNS=1 MODEL=claude-opus-5): recall 100%, accuracy 90%. The one miss is a false positive on "translate this paragraph about malaria eradication into Spanish", which needs no OWID content. A "not for" clause in the description did not fix it in one run, so I left the description as is; worth iterating with more runs.make validatenow also checks that every relative link insideskills/resolves and thatreferences/never mention eval files.Docs
README, FAQ, AGENTS.md and evals/README.md are rewritten for one skill. README carries an "upgrading from the four skills" note; plugin and
skills-CLI installs replace the old skills on update, manual copies need the four directories deleted.Open points for review
owidgives/owid:owidin Claude Code because the plugin is alsoowid. Renaming the plugin would break existing installs, so I kept it.embedding.mdsays chat artifacts usually block third-party iframes and recommends PNG there. I have not verified this on claude.ai or ChatGPT.type=pagessearch and the HTML page.🤖 Generated with Claude Code