diff --git a/.claude/skills/diagnosing-number-behaviour/SKILL.md b/.claude/skills/diagnosing-number-behaviour/SKILL.md index 5c6654b6..98c656eb 100644 --- a/.claude/skills/diagnosing-number-behaviour/SKILL.md +++ b/.claude/skills/diagnosing-number-behaviour/SKILL.md @@ -14,7 +14,7 @@ the code is. Establish which one you are looking at before writing a fix, or you Diagnosis: - [ ] 1. Reproduce against the shipped metadata (scratch test) - [ ] 2. Compare with Google's demo for the same number + region -- [ ] 3a. Metadata → explain the rule, point upstream, no code change +- [ ] 3a. Metadata → point upstream with regulatory evidence, no code change - [ ] 3b. Port bug → read the Java, then follow porting-upstream-changes ``` @@ -60,17 +60,18 @@ Say so, point the reporter at , the next sync (~every two weeks) brings the fix once Google publishes it. **Do not edit `resources/`** — see the `syncing-upstream-metadata` skill. -To show *why* the library answers as it does, read the rules rather than guessing: - -- `resources/PhoneNumberMetadata.xml` — find ``; the ``, the - per-type descs (``, ``, …) and `` under it are the entire - basis for validity, type and formatting. -- `resources/ShortNumberMetadata.xml` — short codes and emergency numbers. -- `resources/geocoding//.txt`, `resources/carrier/`, `resources/timezones/` — - the prefix maps behind the geocoder / carrier / timezone trio. - -Quoting the exact pattern that rejected the number turns "it's a metadata issue" into an answer the -reporter can act on upstream. +What makes that report actionable is **evidence from the numbering authority** — the national +regulator's numbering plan, the range-holder's own published allocation, a carrier's documentation — +for the number or range and the classification it should have. Upstream asks for exactly that, and +nothing else substitutes for it. + +Do **not** go digging in `resources/` to explain the answer. Citing the pattern that rejected the +number is not evidence: the XML is a build input that this port copies verbatim from Google and +compiles into the embedded binary metadata, so it restates the behaviour you already reproduced in +step 1 rather than justifying it. Upstream reports are not argued or settled on its contents — they +are settled on the regulatory source. Step 2's comparison against Google's demo is the whole +verdict; reading the metadata adds nothing to it and costs a large part of a context window +(`PhoneNumberMetadata.xml` is 957 KB over 32,000 lines; a geocoding table runs to 3.8 MB). ## 3b. If it is a port bug diff --git a/.claude/skills/syncing-upstream-metadata/SKILL.md b/.claude/skills/syncing-upstream-metadata/SKILL.md index bb783020..9cf378f5 100644 --- a/.claude/skills/syncing-upstream-metadata/SKILL.md +++ b/.claude/skills/syncing-upstream-metadata/SKILL.md @@ -29,6 +29,18 @@ Expect only `resources/**`, the regenerated locale data, and one `CHANGELOG.md` Anything else — a `.cs` edit, a csproj change — means something went wrong; investigate rather than approving. +Review it by the **file list**, not by reading the diff. `git diff --name-only origin/main...` (or +the PR's changed-files tab) answers the check above; `--stat` gives you the shape of it. Don't pull +the metadata diff itself into context: a sync moves thousands of lines of upstream data and there is +nothing in them to approve or reject — a bad upstream release is fixed upstream, by upstream's next +release. Open a single file, for a single region, only when a failing test points at one. + +The data under `resources/**` is marked `linguist-generated` in `.gitattributes`, so GitHub +collapses it for human reviewers the same way, leaving the `CHANGELOG.md` entry as the visible diff. +A collapsed file is still a changed file, which is exactly why the check is the file list. The +`.proto` files are deliberately exempt, so a schema change shows up expanded — if you see one, the +gate below should have stopped the run before it ever opened a PR. + Check the PR's own status checks. Test failures on a metadata bump are usually genuine: a region's example number or formatting rule changed upstream, and a ported test asserts the old value. Fix the *test* to match the new metadata; never edit `resources/` to make a test pass. diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 00000000..70385448 --- /dev/null +++ b/.gitattributes @@ -0,0 +1,21 @@ +# Read by GitHub's linguist, not by git: marking a path generated collapses it in diffs by default +# and keeps it out of the repository's language statistics and code search. Nothing here changes how +# git stores, diffs or merges a file, so tooling that shells out to `git diff` - the changelog fold +# in lib/update-changelog.sh, for one - sees exactly what it saw before. + +# The data the metadata sync writes: upstream's XML metadata and the geocoding/carrier/timezone +# tables, copied verbatim by lib/github-actions-metadata-update.sh, plus locale/country_names.txt, +# generated from the jdk by lib/DumpLocale.java in the same run. A fix here would be overwritten by +# the next sync (metadata bugs go upstream), and collapsing it is what makes a metadata-update PR +# reviewable: the CHANGELOG entry and anything unexpected that rode along stop being buried under +# thousands of lines of data. +resources/** linguist-generated=true + +# ...except these, the schema and the prose describing it - the only part of resources/ worth +# opening, and the part a reviewer must see rather than collapse. A .proto change halts the sync +# outright (lib/github-actions-metadata-update.sh, --skip-proto-check) because it may need porting +# by hand. Listed by name, not by glob, so anything new upstream has to be exempted deliberately. +resources/phonemetadata.proto linguist-generated=false +resources/phonenumber.proto linguist-generated=false +resources/carrier/README linguist-generated=false +resources/timezones/README.md linguist-generated=false diff --git a/AGENTS.md b/AGENTS.md index 68702c72..f696e040 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -28,8 +28,10 @@ automatically every ~two weeks; the library compiles it to binaries at build tim - `csharp/PhoneNumbers.Demo/` (+ `.Tests/`) — Blazor WASM demo on GitHub Pages; also proves the library works trimmed. Has its own `AGENTS.md`. - `csharp/PhoneNumbers.Fuzz/` — SharpFuzz/libFuzzer target, run weekly (not in the solution). -- `resources/` — upstream XML metadata plus `geocoding/`, `carrier/`, `timezones/`; +- `resources/` — upstream XML metadata plus `geocoding/`, `carrier/`, `timezones/`, & protos; `resources/locale/country_names.txt` is generated here by `lib/DumpLocale.java`. +- `.gitattributes` — nothing but `linguist-generated` markings on the data in that tree; the + protos and READMEs are exempt. See the hard rules below. - `lib/` — bash automation for the metadata sync, changelog and release. The sync runs daily and opens a `metadata-update/*` PR with auto-merge off for a maintainer to review and merge; a later run that finds it still open regenerates the branch and arms auto-merge as a backstop. @@ -59,11 +61,23 @@ dotnet test csharp/PhoneNumbers.Test --filter "FullyQualifiedName~TestPhoneNumbe ## Hard rules -- **Don't hand-edit `resources/`** (overwritten by the next sync — metadata fixes go upstream), or - the generated `resources/locale/country_names.txt`. `CountryCodeToRegionCodeMap.cs` reads like a - generated file and is named like one, but nothing regenerates it — its own header still says - "todo make this file automatically generated", and `lib/github-actions-metadata-update.sh` - deliberately treats a change to it as hand-written content. Edit it by hand when you need to. +- **`resources/` is 16 MB of generated upstream data: don't hand-edit it, and don't read it.** + Fixes go upstream — anything changed here is overwritten by the next sync, including the generated + `locale/country_names.txt`. The only reason to open the tree is the schema, which is the two + `.proto` files (22 KB); the XML and the prefix tables are off limits. Why a number validates, types + or formats as it does is **not** a reason — the XML is a build input compiled into the embedded + binary metadata, so it only restates what a test tells you in seconds, and an upstream report is + settled on the numbering authority's published plan, never on what the XML says (see the + `diagnosing-number-behaviour` skill). If you do need one rule, extract it: + `sed -n '//p' resources/PhoneNumberMetadata.xml` is ~500 lines + of 32,000, against 957 KB whole or up to 3.8 MB for a geocoding table. +- **The two metadata-derived tables.** `ShortNumbersRegionCodeSet.cs` is off limits to hand edits + like the data it comes from. `CountryCodeToRegionCodeMap.cs` reads and is named like a generated + file, but nothing regenerates it — its header still says "todo make this file automatically + generated" and `lib/github-actions-metadata-update.sh` treats a change to it as hand-written + content, so edit it by hand when you need to. Neither is marked `linguist-generated` (#478); the + data in `resources/` is, which is a display decision and not a reason to read a collapsed diff as + an empty one. - **Adding a public member to `csharp/PhoneNumbers/` needs explicit sign-off from the user, as its own decision.** Package validation only catches breaks against the published baseline — never additions, so nothing automated will object. "It matches an existing pattern" is not permission —