fix(schemas): preserve nested annotations and carry forward element kinds - #3503
Conversation
…d element kinds Problem: automated review on PR #3502 (merged a7a5765) found two real regressions in the monotonic-merge fix for replace_provider_packages: P1 (annotation loss): merge_observed_structure_schemas only merges structural keywords (type/properties/items/additionalProperties) -- its docstring says "without retaining property history". The prior _merge_element_schema_with_existing restored x-polylogue-* annotations only at the document root after merging, so every regeneration stripped freshly-computed nested annotations (semantic role, format, frequency, observed distribution) from property-level nodes. P2 (element-kind loss): when a thinner regeneration observes zero samples for a previously-committed element kind, that kind is absent from element_schemas.items() in the fresh pass, so the merge loop never runs for it and the destructive versions/-tree delete-and-rewrite drops it entirely -- get_element_schema(..., element_kind=<old kind>) silently returns None afterward. Solution: - Add SchemaRegistry._annotate_merged_schema_node, a recursive walk that reattaches x-polylogue-* annotations at every node of a structurally merged schema tree (properties/items/additionalProperties), preferring the candidate's fresh annotations with the existing package's as fallback. _merge_element_schema_with_existing now calls it instead of only restoring annotations at the document root. - replace_provider_packages now computes the union of each version's existing-catalog element kinds and its freshly-observed kinds: a kind absent from the fresh pass is carried forward unmerged (its prior manifest + schema, pass-through) rather than dropped. Fixing this surfaced a second bug in the same method -- it persisted the caller's original `catalog.packages` via save_package_catalog(catalog) instead of the carry-forward-augmented packages, so a carried element's schema file was written to disk but never listed in the saved manifest. Both are fixed together via dataclasses.replace(catalog, packages=...). - Also folds in CodeRabbit's low-priority perf suggestion: _existing_provider_element_schemas now reads each element schema file directly (_read_local_element_schema_file) instead of re-loading and re-parsing the whole catalog JSON per element/version via _load_local_element_schema. - Adds TestMergeElementSchemaAnnotationPreservation and TestReplaceProviderPackagesCarriesForwardUnobservedElementKinds to tests/unit/schemas/test_promotion_monotonicity.py, following the existing TestReplaceProviderPackagesMonotonicity pattern. Verification: `python -m devtools test tests/unit/schemas/` (66 passed); `python -m devtools verify --quick` (exit_code 0); manually reverted runtime_registry.py and confirmed exactly the 3 new tests fail (the rest of the suite stays green), then restored the fix. Ref polylogue-46kg Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 4ea21f0a52
ℹ️ 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".
| # regeneration dropped entirely is a separate, out-of-scope loss | ||
| # class (a whole retired version, not a kind within a version). | ||
| carried_elements = list(package.elements) | ||
| for element_kind, prior_manifest in existing_elements_by_version.get(package.version, {}).items(): |
There was a problem hiding this comment.
When a thinner regeneration omits an earlier package family, schemas/generation/packages.py::_build_package_candidates re-sorts the remaining families and build_provider_catalog_artifacts renumbers them from v1, so an old v2 family can become the new v1. Looking up prior elements solely by package.version then attaches old v1 element kinds and schemas to this unrelated family, incorrectly claiming support and potentially misrouting schema resolution or synthetic generation. Match the prior package using a stable identity such as anchor_profile_family_id, not the ordinal version.
Useful? React with 👍 / 👎.
Co-Authored-By: Claude <noreply@anthropic.com>
1 participant
Summary
Fast-follow fix for two real regressions found by automated review on PR #3502 (merged a7a5765) minutes after it landed, both confirmed by reading the merged code (not speculative).
Problem
P1 (annotation loss).
merge_observed_structure_schemas(polylogue/schemas/generation/dynamic_keys.py:140) merges only structural keywords (type/properties/items/additionalProperties) — its own docstring says "without retaining property history". The_merge_element_schema_with_existingadded by #3502 restoredx-polylogue-*annotations only at the document ROOT after merging, so every regeneration stripped freshly-computed nested annotations (x-polylogue-semantic-role,x-polylogue-format,x-polylogue-frequency,x-polylogue-observed-distribution) from property-level nodes.P2 (element-kind loss). When a thinner regeneration observes zero samples for a previously-committed element kind, that kind is absent from
element_schemas.items()in the fresh pass, so the merge never runs for it, and the destructive versions-tree delete-and-rewrite inreplace_provider_packagesdrops it.get_element_schema(..., element_kind=<old kind>)silently returnsNoneafterward — the same destructive-loss bug class polylogue-ov5r (#3502) fixed, just narrower: whole missing element kinds rather than narrowed types within an observed kind.Solution
SchemaRegistry._annotate_merged_schema_node(polylogue/schemas/runtime_registry.py): a new recursive walk that reattachesx-polylogue-*annotations at every node of the structurally merged tree (properties/items/additionalProperties), matching existing/candidate/merged by path, preferring the candidate's fresh annotations with the existing package's as fallback._merge_element_schema_with_existingcalls it instead of a root-only restore.replace_provider_packagesnow computes, per version, the union of existing-catalog element kinds and freshly-observed element kinds. A kind absent from the fresh pass is carried forward unmerged (its prior manifest + schema, pass-through), scoped to kinds the same version previously carried (matchingget_element_schema/package.element()'s per-version lookup). A whole retired version disappearing entirely is a separate, out-of-scope loss class not addressed here.catalog.packagesviasave_package_catalog(catalog)instead of the carry-forward-augmented packages, so a carried element's schema file was written to disk but never listed in the saved manifest (get_element_schemawould still returnNone). Fixed together viadataclasses.replace(catalog, packages=...)._load_local_element_schemare-reading/re-parsing the same catalog JSON per element/version inside_existing_provider_element_schemas's loop whencatalogis already in scope there. Split out_read_local_element_schema_fileso that loop reads the schema file directly instead of reloading the catalog each time.Tests
Added to
tests/unit/schemas/test_promotion_monotonicity.py, following theTestReplaceProviderPackagesMonotonicitypattern from #3502:TestMergeElementSchemaAnnotationPreservation:test_nested_annotation_survives_when_fresh_pass_recomputes_no_annotation— existing schema has a nested property annotatedx-polylogue-semantic-role; a subsequent regen re-observes the same property's type with no annotation; asserts the annotation survives.test_nested_annotation_fresh_value_wins_over_stale_existing— the fresh regen carries a different annotation value for the same nested property; asserts the fresh value wins.TestReplaceProviderPackagesCarriesForwardUnobservedElementKinds:test_kind_absent_from_fresh_observation_window_is_carried_forward— a regen with a fresh sample set covering only element kindmessage, where the existing catalog has kindsmessageandtool_result; assertstool_result's schema still resolves viaget_element_schemaafterward, unmerged with fresh data.Anti-vacuity: I manually reverted
polylogue/schemas/runtime_registry.py(keeping the new tests) and rerandevtools test tests/unit/schemas/test_promotion_monotonicity.py: exactly the 3 new tests failed (2 annotation tests assertingNone == 'identifier'/'timestamp', 1 element-kind test assertingNone is not None), while the 8 pre-existing tests stayed green. Then restored the fix and reconfirmed all 11 pass.Verification
python -m devtools test tests/unit/schemas/→66 passed in 6.83spython -m devtools verify --quick→exit_code: 0(ruff format/check, mypy, render all, topology, layering, closure-matrix, schema roundtrip/versioning/promotion-audit, manifests, ci-workflows, doc-commands, docs-coverage, test-infra-currency, pytest-timeout-overrides, degrade-loudly, hash-boundary-census — allok)AC matrix
_annotate_merged_schema_noderecursive reattach, tested_load_local_element_schemare-parse)_read_local_element_schema_filesplit-outRef polylogue-46kg