[Content Understanding] Fix --schema-dir natural version sort (v10 > v9)#47957
[Content Understanding] Fix --schema-dir natural version sort (v10 > v9)#47957chienyuanchang wants to merge 1 commit into
Conversation
Addresses the Copilot review comment from the sibling .NET PR (azure-sdk-for-net#60394, second-round review). The bug came from Python and was faithfully ported to Java (#49672) / JS (#39137) / .NET (#60394) — this PR fixes the source-of-truth so the four languages stay in lockstep. .NET is already fixed (commit 61bda1abd16), JS is queued (commit d2a463cde18 on the open PR), Java is queued (commit 7b07a7564d3 on the open PR). Before: _discover_inner_from_dir did 'sorted(schema_dir.glob("*.json"))' and took the last element as the 'newest version'. That breaks the moment any alias reaches double-digit versions: invoice_v9.json vs invoice_v10.json → sorted lexicographically, '1' < '9' at position 9, so invoice_v10 sorts BEFORE invoice_v9, 'alphabetical last' picks invoice_v9, and the tool silently loads the wrong (older) schema. After: sort by a 'version key' tuple, highest wins: - group 0 → bare '<alias>.json' (no suffix; oldest) - group 1 → '<alias>_v<N>.json' or (numeric suffix; '<alias>_<N>.json' sorted by N) - group 2 → any other '<alias>_<suffix>.json' (lex tiebreaker) So invoice_v10 beats invoice_v9 beats invoice, and 'invoice_draft' still beats bare 'invoice.json' but loses to any versioned file. New _version_sort_key helper is module-level (leading-underscore is just 'implementation detail', not test-invisible) so tests can pin the key extractor directly. Test coverage (+11 pytest cases in test_skills_classify_route_router.py): - test_version_sort_key_bare_alias_returns_group_zero - test_version_sort_key_v_prefixed_numeric_returns_group_one (v10 > v9) - test_version_sort_key_bare_numeric_returns_group_one (_42) - test_version_sort_key_non_numeric_suffix_returns_group_two - test_discover_inner_from_dir_resolves_exact_match_stem - test_discover_inner_from_dir_picks_natural_version_max_not_alphabetical (the exact regression the .NET Copilot review flagged) - test_discover_inner_from_dir_prefers_versioned_over_bare_alias - test_discover_inner_from_dir_skips_prebuilt_aliases - test_discover_inner_from_dir_skips_categories_without_analyzer_id - test_discover_inner_from_dir_missing_aliases_raises - test_discover_inner_from_dir_unrelated_json_files_ignored - test_discover_inner_from_dir_non_existent_dir_raises CHANGELOG.md: added a 'Bugs Fixed' entry to the existing 1.2.0b3 (Unreleased) section (no version bump needed — b3 has not shipped yet). Verified: pytest tests/test_skills_classify_route_router.py → 19/19 passing (was 8; +11 new cases).
There was a problem hiding this comment.
Pull request overview
This PR fixes a natural-version-sort bug in the --schema-dir shortcut of the Content Understanding "classify-and-route" skill helper script (create_and_test_router.py). Previously, _discover_inner_from_dir relied on lexicographic sorting (sorted(...)[-1]), which incorrectly ordered invoice_v10.json before invoice_v9.json (because '1' < '9' character-by-character), silently selecting the older schema once versions reached double digits. The fix introduces a module-level _version_sort_key helper that sorts by a (group, numeric_version, suffix) tuple so the highest numeric version wins. It is the source-of-truth counterpart to sibling fixes already opened for .NET, JS, and Java.
Changes:
- Added
_version_sort_key(with a_VERSION_SUFFIXregex) and switched_discover_inner_from_dirto sort matches by that key instead of alphabetical order. - Added 11 pytest cases covering the key extractor and directory-resolution behavior.
- Added a "Bugs Fixed" CHANGELOG entry under the unreleased
1.2.0b3section.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
.github/skills/cu-sdk-author-analyzer-classify-route/scripts/create_and_test_router.py |
Adds _version_sort_key/_VERSION_SUFFIX and replaces alphabetical sort with numeric-version-aware sorting in _discover_inner_from_dir. |
tests/test_skills_classify_route_router.py |
Adds 11 tests for the key extractor and directory resolution, including the v10 > v9 regression. |
CHANGELOG.md |
Documents the --schema-dir version-sort bug fix under 1.2.0b3 (Unreleased). |
| m = _VERSION_SUFFIX.fullmatch(suffix) | ||
| if m is not None: | ||
| try: | ||
| return (1, int(m.group(1)), "") | ||
| except ValueError: | ||
| pass # fall through to group 2 | ||
| return (2, 0, suffix) |
[Content Understanding] Fix
--schema-dirnatural version sort (source-of-truth for JS/Java/.NET ports)Summary
Fixes the version-sort bug in
_discover_inner_from_dir(the classify-and-route--schema-dirshortcut) that Copilot flagged during review of the sibling .NET PR (Azure/azure-sdk-for-net#60394).Bug (still on
main): when--schema-dirfinds multiple JSON files matching a category alias, the previous code didsorted(schema_dir.glob("*.json"))and tookmatches[-1]as the "newest version". That breaks as soon as any alias reaches double-digit versions:Fix
Sort by a "version key" tuple, highest wins:
<alias>.json(no suffix; oldest baseline)(0, 0, "")<alias>_v<N>.jsonor<alias>_<N>.json(1, N, "")<alias>_<suffix>.json(2, 0, suffix)So
invoice_v10beatsinvoice_v9beatsinvoice, andinvoice_draftstill beats bareinvoice.jsonbut loses to any versioned file.New
_version_sort_keyhelper is module-level (single-underscore = "implementation detail", not test-invisible) so tests can pin the key extractor directly. Same shape as the .NET / JS / Java ports for cross-language parity.Cross-language coordination
This bug came from Python and was faithfully ported to Java, JS, and .NET. To keep the four in lockstep:
61bda1abd16d2a463cde187b07a7564d3main, so it needs its own PRTest coverage (+11 pytest cases)
test_version_sort_key_bare_alias_returns_group_zero(0, 0, "")for bare<alias>test_version_sort_key_v_prefixed_numeric_returns_group_onev10 > v9— pins the fixtest_version_sort_key_bare_numeric_returns_group_one<alias>_<N>(novprefix) also recognisedtest_version_sort_key_non_numeric_suffix_returns_group_twotest_discover_inner_from_dir_resolves_exact_match_stemtest_discover_inner_from_dir_picks_natural_version_max_not_alphabeticaltest_discover_inner_from_dir_prefers_versioned_over_bare_aliastest_discover_inner_from_dir_skips_prebuilt_aliasesprebuilt-*needs no local filetest_discover_inner_from_dir_skips_categories_without_analyzer_idtest_discover_inner_from_dir_missing_aliases_raisestest_discover_inner_from_dir_unrelated_json_files_ignoredtest_discover_inner_from_dir_non_existent_dir_raisesTest results
Was 8 before this PR (7 pre-existing + 1 pre-existing versionSort ancestor); +11 new = 19 total.
Backward compatibility
--schema-dirwho have multiple matching files per alias with_v<N>suffixes AND at least one two-digit N. Before, they silently got the wrong file; now they get the right one.re).CHANGELOG
Added a "Bugs Fixed" entry under the existing
1.2.0b3 (Unreleased)section — no version bump needed since b3 has not shipped yet.Checklist