Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces catalog ID validation in specification/scripts/validate.py to ensure that catalog IDs match their schema $id and that all referenced catalog URLs are declared. It also updates catalog IDs in the a2ui_protocol.md documentation examples. Feedback on the validation script includes specifying encoding="utf-8" when opening files for cross-platform compatibility, verifying that the parsed JSON is a dictionary, and avoiding defensive .get() calls for strict invariants to allow failing fast.
| for catalog_path in catalog_paths: | ||
| rel = os.path.relpath(catalog_path, repo_root) | ||
| try: | ||
| with open(catalog_path, "r") as f: |
There was a problem hiding this comment.
When opening files for reading or writing text, always specify encoding="utf-8" to ensure cross-platform compatibility, especially on Windows environments where the default system encoding might not be UTF-8.
| with open(catalog_path, "r") as f: | |
| with open(catalog_path, "r", encoding="utf-8") as f: |
There was a problem hiding this comment.
Done in 5aa2437. Agreed — the spec tree contains non-ASCII content and the default encoding is locale-dependent, so this was a real portability bug rather than a style nit.
Scoped to the two open() calls this PR adds; the pre-existing ones elsewhere in the file are left alone to keep the diff reviewable.
| if f"{os.sep}node_modules{os.sep}" in path: | ||
| continue | ||
| try: | ||
| with open(path, "r", errors="ignore") as f: |
There was a problem hiding this comment.
Specify encoding="utf-8" when opening files to ensure consistent decoding across different platforms.
| with open(path, "r", errors="ignore") as f: | |
| with open(path, "r", encoding="utf-8", errors="ignore") as f: |
There was a problem hiding this comment.
Done in 5aa2437.
Worth noting errors="ignore" alone would not have saved this one: without an explicit encoding, a non-UTF-8 locale could decode bytes into different but still valid characters, which could silently hide a catalog URL from the regex rather than raise. Explicit encoding="utf-8" is what actually makes the scan deterministic.
| schema_id = catalog.get("$id") | ||
| catalog_id = catalog.get("catalogId") |
There was a problem hiding this comment.
Verify that the parsed JSON is a dictionary before accessing its keys to prevent potential crashes. Additionally, avoid using defensive .get() for keys that are strict invariants (such as $id and catalogId); direct key access is preferred to fail fast and signal invariant violations.
| schema_id = catalog.get("$id") | |
| catalog_id = catalog.get("catalogId") | |
| if not isinstance(catalog, dict): | |
| print(f" [FAIL] {rel} is not a JSON object") | |
| success = False | |
| continue | |
| schema_id = catalog["$id"] | |
| catalog_id = catalog["catalogId"] |
References
- Do not use defensive '.get()' or fallback values when accessing dictionary or metadata keys if the presence of the key is a strict invariant. Raising a 'KeyError' is preferred to fail fast and signal invariant violations.
There was a problem hiding this comment.
Split decision — took the first half, declining the second.
Adopted: the isinstance(catalog, dict) guard, in 5aa2437. A catalog.json containing a JSON array would previously have died with AttributeError on .get(). Now it reports [FAIL] <path> is not a JSON object. Verified against a synthetic non-object catalog.
Declining the catalog["$id"] / catalog["catalogId"] change, because the premise does not hold here: these keys are not invariants this function may assume, they are the invariants it exists to check.
Concretely, the very next lines are:
if not catalog_id:
print(f" [FAIL] {rel} declares no 'catalogId'")
success = False
continueDirect indexing would convert that reported failure into an uncaught KeyError, which is strictly worse for a validation script:
- It aborts the run, so you fix one file, re-run, discover the next — instead of getting every violation in one pass. The rest of
validate.pyis built around accumulating intosuccessand continuing, and this function follows that. - The traceback loses the
relpath context that the[FAIL]line carries. - A missing
$idis legitimately representable, and is already surfaced by the$id/catalogIddisagreement branch, which prints both values. Crashing would report less.
There is also precedent in this file — the existing ajv aliasing block guards with if "$id" in catalog: rather than indexing.
Fail-fast is the right default for production code that has already established its invariants. A linter whose job is to find malformed input is the standard exception.
…log schema The v0.9.1 protocol doc tells agents to send https://a2ui.org/specification/v0_9_1/catalogs/basic/catalog.json but no catalog schema in the repository declares that ID. The v0_9_1 basic catalog is byte-identical to the v0_9 one and declares the v0_9 ID, as do all 37 v0_9_1 examples and rules.txt. The doc is the only outlier, at two lines. Renderers that do not recognise a catalogId commonly fall back to registering an empty placeholder catalog, so the surface reports as created and then renders nothing, with no error anywhere. Agents written against the doc hit exactly that. Add check_catalog_ids() to the spec validation script to prevent recurrence. It enforces that every catalog schema's $id matches its catalogId, and that every catalog URL referenced under specification/ is an ID some schema actually declares. Widen the validate_specifications workflow trigger paths to include catalogs/ and docs/. The new check reads those files, so without this the guard would not run on the very class of change that introduced the bug.
7db640a to
8b6e9b2
Compare
…JSON Addresses review feedback on check_catalog_ids(): - Decode files as utf-8 explicitly rather than relying on the platform locale. - Report a clear failure when a catalog.json does not parse to a JSON object, instead of raising AttributeError on the subsequent key access.
This branch has not been deployed
1 participant
Summary
The v0.9.1 protocol documentation tells agents to emit a basic-catalog ID that no catalog schema in this repository declares. Clients that do not happen to tolerate the unknown ID render an empty surface with no error.
This PR corrects the two offending references and adds a check to
specification/scripts/validate.pyso the class of bug cannot return.The bug
specification/v0_9_1/docs/a2ui_protocol.md(lines 195 and 288) instructs agents to send:But
specification/v0_9_1/catalogs/basic/catalog.jsondeclares:A tally of every
https://a2ui.org/specification/*/catalogs/*/catalog.jsonreference under each version directory shows how isolated the discrepancy is:v0_9IDv0_9_1IDv1_0IDv0_9/v0_9_1/v1_0/v0_9/andv1_0/are internally consistent.v0_9_1/is not: the schema, all 37 examples, andrules.txtuse thev0_9ID, while the protocol doc uses av0_9_1ID in exactly 2 places.The
v0_9_1basic catalog is byte-identical to thev0_9one:so reusing the v0.9 catalog identity in v0.9.1 is deliberate and correct — the catalog genuinely did not change between the two releases. The doc is the outlier.
Why this matters in practice
Renderers differ in how they treat an unrecognised
catalogId.swift/core/Sources/BasicCatalog/BasicCatalog.swifthappens to accept both, viav091CatalogURI; others register only the declared ID.A renderer that does not recognise the ID generally cannot distinguish it from a typo, so the common behaviour is to register an empty placeholder catalog: the surface reports as created, then renders nothing, and no error is raised at any layer. That is expensive to debug from the agent side, because every layer reports success.
We hit precisely this failure mode downstream when an implementation's hard-coded basic-catalog constant disagreed with the spec URL. It took a full integration trace to find.
Changes
1.
specification/v0_9_1/docs/a2ui_protocol.mdTwo occurrences of the undeclared
v0_9_1catalog ID replaced with the declaredv0_9one.Note the
"version"field staysv0.9.1. Protocol version and catalog identity are independent; a v0.9.1 message legitimately references the v0.9 basic catalog.2.
specification/scripts/validate.py— newcheck_catalog_ids()Enforces two invariants:
$id==catalogId.specification/is an ID that some catalog schema actually declares.Invariant 2 is the one that generalises: it stops any doc, example, or test fixture from inventing a catalog ID that no catalog answers to.
On an invariant-1 failure the
catalogIdis still registered as declared, so one bad$iddoes not cascade into a bogus "undeclared ID" error for every file that references that catalog.3.
.github/workflows/validate_specifications.yml— trigger pathsAdded
specification/**/catalogs/**andspecification/**/docs/**.The workflow previously ran only on
specification/**/json/**and the script itself. The new check reads catalog and doc files, so without this the guard would not run on the very class of change that introduced this bug. This does mean documentation-only PRs now run the validation suite.Verification
Run against this branch and against
main.On
main(before the doc fix) — flags exactly one ID, from exactly one file, with no false positives anywhere else underspecification/:On this branch (after the doc fix):
Invariant 1 sensitivity — temporarily corrupting
$idinv1_0/catalogs/basic/catalog.jsonyields a single targeted error and no cascade:Deliberately out of scope
v0_9_1is absent from theconfigsdict invalidate.py, so that version's examples receive no schema validation today. Worth fixing, but it is a separate change with its own fallout to triage.BasicCatalog.v091CatalogURIin the Swift SDK now provably matches no declared catalog. Harmless as tolerance; removing it is an SDK decision.Open question for maintainers
This PR assumes the schema is authoritative and the doc is wrong, because 54 of 56 references within
v0_9_1/, plus the shipped SDKs and renderers, use thev0_9catalog ID, and the two catalogs are byte-identical.The alternative reading — that v0.9.1 was meant to mint its own catalog identity — would require changing the schema, all 37 examples,
rules.txt, and every SDK constant, and would break already-deployed agents. Please confirm the direction before merge if that was in fact the intent.