Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 13 additions & 12 deletions .claude/skills/diagnosing-number-behaviour/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
```

Expand Down Expand Up @@ -60,17 +60,18 @@ Say so, point the reporter at <https://github.com/google/libphonenumber/issues>,
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 `<territory id="GB" …>`; the `<generalDesc>`, the
per-type descs (`<mobile>`, `<fixedLine>`, …) and `<availableFormats>` under it are the entire
basis for validity, type and formatting.
- `resources/ShortNumberMetadata.xml` — short codes and emergency numbers.
- `resources/geocoding/<lang>/<country-code>.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

Expand Down
12 changes: 12 additions & 0 deletions .claude/skills/syncing-upstream-metadata/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
21 changes: 21 additions & 0 deletions .gitattributes
Original file line number Diff line number Diff line change
@@ -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
26 changes: 20 additions & 6 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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 '/<territory id="GB"/,/<\/territory>/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 —
Expand Down
Loading