Repository navigation
fix(core): apply Surface allowedParents rule only to root and check all parents - #3360
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the composition constraint validation logic across Python, TypeScript, Dart, and Swift implementations. Specifically, it ensures that the 'Surface' allowedParents rule is only applied to the surface's root component (even if referenced by other components), and that all parent edges are checked when a component has multiple parents. Additionally, unreferenced non-root components are no longer checked against 'Surface'. The changes include comprehensive conformance and unit tests, as well as updates to the changelogs. I have no feedback to provide as the implementation is thorough and well-tested.
|
Rebased onto Python (
TypeScript (
Tests that encoded the old rule, now updated
Verified: Python core/agent/ADK/A2A/conformance tests (3303 passed), web_core tests (1062) and conformance (566/566), TS agent (1108), Dart core (1857) and |
9b481d2 to
02c71ed
Compare
gspencergoog
left a comment
There was a problem hiding this comment.
The core validation logic in Python and TypeScript is sound and matches the v1.0 specification and sibling SDKs. A few minor revisions are needed in the Swift conformance runner (ValidatorConformanceTests.swift skips expect.surfaces and expectError.message checks on composition_constraints.yaml), the Python and TypeScript CHANGELOG.md entries, and a stale docstring in ComponentGraphBound.
🤖 Agent-generated
| @classmethod | ||
| def of(cls, components: Mapping[str, ComponentModel]) -> ComponentGraphBound: | ||
| """Returns the bound of a graph that passed the checks, in surface order. | ||
| """Returns the bound of a graph that passed the checks. |
There was a problem hiding this comment.
Removing self._order and self._next_order updated of() at line 128 and composition_holds() at line 193, but left the class docstring at lines 100-108 stale. Lines 100-101 still mention "the position of each component on the surface", and line 108 says "its parent" instead of "its parents". Future maintainers reading ComponentGraphBound may assume the bound still tracks component positions and checks only a single parent.
🤖 Agent-generated
There was a problem hiding this comment.
Fixed in efebacb. The class docstring no longer mentions component positions and says a component's composition depends on its parents. I also rewrapped the composition_holds docstring line that ran past the line length.
83eef93 to
efebacb
Compare
gspencergoog
left a comment
There was a problem hiding this comment.
One compilation error remains in the Swift conformance test file: ValidatorConformanceTests.swift declares assertComponents(_ expectedSurfaces: OrderedDictionary<String, JSONValue>, ...) at line 324 without importing OrderedCollections, causing the GitHub Actions Swift test job (40788908308) to fail to compile.
🤖 Agent-generated
|
|
||
| /// Checks each expected component's type and properties on the processor's surfaces. | ||
| private func assertComponents( | ||
| _ expectedSurfaces: OrderedDictionary<String, JSONValue>, |
There was a problem hiding this comment.
assertComponents references OrderedDictionary<String, JSONValue> in its parameter list, but ValidatorConformanceTests.swift does not import OrderedCollections. Add import OrderedCollections to the file imports at line 20 (matching ConformanceTestHelper.swift and MessageProcessorConformanceTests.swift), or pass step.expect (JSONValue?) into assertComponents and unwrap expected?["surfaces"]?.objectValue inside the helper. The A2UIConformanceTests target fails to compile in the Swift CI test job with cannot find type 'OrderedDictionary' in scope.
🤖 Agent-generated
There was a problem hiding this comment.
Fixed in 68dd32e (import OrderedCollections); the Swift test job passes now.
…ll parents Signed-off-by: Nan Yu <nanyu@google.com>
… types in Swift harness - Python: validate_components_update with no config checked composition constraints over the whole surface for every batch, which made processing n single-component messages quadratic. The model now tracks the catalogs its components come from and skips the check when none of them, nor the batch's, declares allowedParents or allowedChildren, as the TypeScript processor does. - Swift conformance harness: an inline catalog with no common types schema now loads the published one for its protocol version, so the new composition cases' `#/$defs/ChildList` references resolve. - Drop the insert_component docstring note about allowedParents reading component order, which no longer applies. Signed-off-by: Nan Yu <nanyu@google.com>
The composition cases used the action to pick the checks: a validate case ran strictly, and a process_messages case ran with no config in Python and TypeScript but with the relaxed config in Dart and Swift. Two cases with the same payloads differed only in their action, which hid what differs, and the case for a container that drops a child passed on Python's main because nothing was checked. Add a case-level validationConfig (relaxed or strict) for process_messages cases, read by the Python, TypeScript, Dart and Swift harnesses, and make every composition case a process_messages case that names its config. The dropped-child pair shares its catalog and payloads through YAML anchors and differs only in validationConfig and the second step's expectation. The relaxed case now fails on main in Python, as the strict and root-reference cases do in Python and TypeScript. The composition suite no longer has validate cases, so the Dart validator harness stops registering it. Signed-off-by: Nan Yu <nanyu@google.com>
A case's validationConfig is now an object of the four validation flags every SDK's ValidationConfig has (allowOrphanComponents, allowDanglingReferences, allowMissingRoot, allowUnknownElements), instead of the relaxed and strict preset names. A flag the case leaves out is false, so each case states exactly the checks it turns off, and the schema and harnesses reject an unknown flag. The dropped-child cases now differ in one flag. A third case also allows dangling references, which lets Python and TypeScript check only what an update changes, so both the full and the incremental check are covered. Against main's code, five cases fail in Python and two in TypeScript. Signed-off-by: Nan Yu <nanyu@google.com>
…arness Signed-off-by: Nan Yu <nanyu@google.com>
…ace root id - python/a2ui_core CHANGELOG: mark the composition change as breaking, add an entry for the check a MessageProcessor without a validation config now makes, and link the issue and PR. - web_core: move validateParentConstraints' doc comment back above it. - web_core: test that composition reads the surface's own root id, as a v0.8 beginRendering root sets it, and no longer treats a component named 'root' as the root. Signed-off-by: Nan Yu <nanyu@google.com>
… the Swift composition runner - CHANGELOGs: put the breaking composition entries at the top of Unreleased, mark the TypeScript entry as breaking with the issue and PR links, and stop the Python insert_component entry from saying that allowedParents reads component order. - ComponentGraphBound: drop the component positions from the class docstring and say a component's composition depends on its parents. - Swift composition runner: check the error message as well as its code, and check the expected components after the last step. Signed-off-by: Nan Yu <nanyu@google.com>
…nce tests Signed-off-by: Nan Yu <nanyu@google.com>
main now writes the v1.0 protocol version as '1.0' (#3726). Update the version strings this PR adds in the composition conformance cases and in the Python, TypeScript and Swift tests. Signed-off-by: Nan Yu <nanyu@google.com>
68dd32e to
475b0d3
Compare
Summary
Resolves #3354 by aligning Python (
python/a2ui_core) and TypeScript (typescript/core/web_core) composition constraint validation with the v1.0 specification (specification/v1_0/json/catalog_definition.jsonandspecification/v1_0/docs/a2ui_protocol.md) and with the Dart and Swift implementations, and adding shared conformance test vectors toconformance/core/composition_constraints.yaml.Fixes #3354
Changes
python/a2ui_core/src/a2ui/core/state/validation_helpers.py&python/a2ui_core/src/a2ui/core/state/surface_components_model.py:root_id: str = ROOT_IDtovalidate_composition_constraintsand wiredeffective_root_idfromSurfaceComponentsModel.validate_components_update."Surface"inallowedParentsif and only ifcomp_id == root_id, even when another component also references the root."Surface"(leaving unreachable components to topology checks).parent_mapfrom a single overwritten entry per child to a list of(parent_id, parent_type)pairs so every parent edge of a multi-parent component is checked againstallowedParents.validate_composition_constraintsalso runs whenconfig is Noneinvalidate_components_update, matching TypeScript, Dart, and Swift.typescript/core/web_core/src/processing/message-processor.ts:rootId(this.validationConfig?.rootId ?? surface.rootId ?? 'root') invalidateCompositionConstraintsand passed it tovalidateParentConstraints.id === rootIdagainst'Surface'inallowedParentsunconditionally (even whenparentsis non-empty), and removed theallowOrphanComponents === falsebranch that treated non-root unreferenced components as children of'Surface'.conformance/core/composition_constraints.yaml& Conformance Harnesses:validationConfigforprocess_messagescases toconformance/conformance_schema.json: an object of the four validation flags every SDK'sValidationConfighas (allowOrphanComponents,allowDanglingReferences,allowMissingRoot,allowUnknownElements). A flag the case leaves out is false, so{}turns every check on, and an unknown flag fails the case. The Python, TypeScript, Dart and Swift harnesses read it, so a case's checks no longer depend on the harness. Before, the action picked them:validateran strictly, andprocess_messagesran with no config in Python and TypeScript but with the relaxed config in Dart and Swift.process_messagescase that sets its flags. Cases share catalogs, and the dropped-child cases share their payloads, through YAML anchors, so those cases differ only in their flags and in what the second step expects.test_composition_dropped_child_is_not_checked_against_surface(allowOrphanComponents: true)test_composition_dropped_child_is_not_checked_against_surface_when_references_may_dangle(allowOrphanComponents: true,allowDanglingReferences: true), which lets Python and TypeScript check only what each update changestest_composition_dropped_child_is_reported_as_orphan(allowOrphanComponents: false)test_composition_root_referenced_by_another_component_checked_against_surface(allowOrphanComponents: true)test_composition_multiple_parents_disallowed_first({})test_composition_multiple_parents_disallowed_second({})main's code, cases 1–5 fail in Python and cases 3–4 fail in TypeScript; case 6 already passed in both. Dart already followed the spec.codefields. The Dart validator harness no longer registers the composition suite, which has novalidatecases left; the message-processor harness runs it.python/a2ui_core/tests/test_state.pyandtypescript/core/web_core/src/processing/message-processor.test.ts.python/a2ui_core/CHANGELOG.mdandtypescript/core/web_core/CHANGELOG.md.Impact & Risks
root_idinvalidate_composition_constraintsdefaults toROOT_ID).Testing
uv run pytest python/a2ui_core conformance/tests(1629 passed, 15 skipped)node typescript/core/web_core/tests/conformance/conformance_test.mjs(573/573 Passed) andnode --test "dist/**/*.test.js"intypescript/core/web_core(1050 passed)dart testindart/a2ui_core(1867 passed)dart format --language-version=3.5and the license check.Pre-launch Checklist
One time:
For this PR: