Add support for XTCE 1.3 alongside XTCE 1.2

[medley56]

Closes #184.

OMG published XTCE 1.3 in July 2025, in a new namespace (http://www.omg.org/spec/XTCE/20250214). This adds 1.3 support in addition to 1.2 — nothing about 1.2 is dropped, and 1.2 parse output is byte-identical to main (verified at the head commit: every decoded and raw value for all four bundled mission definitions over their real packet files, plus the full type/parameter/container inventories — 67,596,155 bytes, identical).

The issue also asked to "re-evaluate the way we handle 'standard' XTCE namespacing versus parsing it out of the document," which turned out to be where most of the real bugs were.

Targets 6.3.0. No breaking changes — see Compatibility.

What supporting 1.3 actually required

The two OMG schemas were diffed element by element to find what genuinely affects this library. Most of the 1.2→1.3 delta is on the command/argument side, which we don't parse. Three changes reach the telemetry half:

1. The namespace URI, which is the only thing in a document that identifies the XTCE version. (Header/@version is not the standard version — per the XSD it's a free-form descriptor for the document itself. That misconception was baked into the code.)
2. VariableStringType. 1.2 requires a DynamicValue/DiscreteLookupList to declare the raw buffer length and treats LeadingSize/TerminationChar as optional. 1.3 inverts this: the declared length is optional and exactly one delimiter is required — so a Pascal string or C string can be described without inventing a separate length parameter. A schema-valid 1.3 document of that shape previously raised ValueError on parse.
3. LinearAdjustment/@slope gained a default of 1. We defaulted a missing slope to 0, collapsing the adjustment to a constant.

Two smaller ones also land: SpaceSystem/@systemType and @assetType are new in 1.3, and TimeUnitsType respells picoSeconds as picoseconds while adding several values.

Changes

Version awareness

• A registry in space_packet_parser.xtce: SUPPORTED_XTCE_VERSIONS, LATEST_XTCE_VERSION, DEFAULT_XTCE_VERSION, per-version namespace/schema-URL/time-unit mappings, and xtce_version_from_uri / xtce_uri_for_version / xtce_nsmap.
• The OMG 1.3 XSD is bundled, so 1.3 documents validate offline exactly like 1.2 ones. Both bundled schemas are byte-identical to the OMG downloads, with SHA-256s recorded in the schemas README. Whitespace-normalizing pre-commit hooks are excluded from that directory, which had previously altered the 1.2 copy.
• XtcePacketDefinition.xtce_standard_version reports the version implied by the namespace URI; assigning to it retargets a definition. ValidationResult.xtce_version and spp validate report it too.
• Namespace detection in from_xtce reads the root element's actual namespace and prefix instead of scanning the nsmap for a URI containing "xtce".
• Only versions whose schema is bundled may be selected as a write target, so a definition can't be pointed at an XSD that isn't shipped. Detection stays permissive — a 1.1 document is still reported as 1.1.

XTCE 1.3 constructs

• Variable-length strings in the 1.3 delimiter-only form parse, with the buffer length derived from a LeadingSize (the size tag plus the content length it reports, bounded by maxSizeInBits) or a TerminationChar (up to and including the terminator).
• systemType and assetType round-trip, as the new space_system_type and asset_type fields.

Where a construct is version-specific

Serializing warns rather than silently producing an invalid document, naming the parameter types involved. This covers variable-length strings in either direction, time units the target version doesn't define, and the 1.3-only SpaceSystem attributes when writing 1.2. Content is never rewritten — a unit is what the author wrote.

Bugs this exposed

All pre-existing, all in the changelog:

• _validate_xtce_structure hardcoded the 1.2 namespace in its XPath, so reference-integrity checks matched nothing — and reported no errors — for any other namespace: 1.3, a non-standard mission URI, or none at all. It silently "passed" everything.
• XTCE_1_2_XMLNS used https where the real targetNamespace is http, and XTCE_1_1_XMLNS named a URI no XTCE schema declares. A definition built from scratch serialized to a document no schema recognized.
• SequenceContainer serialization emitted BaseContainer before EntryList; the XSD (both versions) orders it after.
• maxSizeInBits was never written, so every variable-length string we serialized was schema-invalid in both versions.
• A multi-byte termination character was searched byte by byte, so in UTF-16BE the terminator b"\x00\x00" matched inside b"\x41\x00\x00\x42" — two ordinary characters — truncating the string. The search is character-aligned now, and still a single C-level bytes.find call for the single-byte and UTF-8 encodings that cover nearly every real definition.
• An empty <TerminationChar/> was read as an absent delimiter rather than the null terminator both schemas declare as its default, rejecting a valid C string.
• Header/@version and @validationStatus were dropped on round trip.

Serialized documents now declare xsi:schemaLocation for their own version, so a definition written by this library is schema-valid as-is.

Compatibility

Nothing here is breaking, so this is a minor release.

The XTCE_1_2_XMLNS / XTCE_1_1_XMLNS correction changes two constant values, but the old values never produced a schema-validatable document — they matched no schema's targetNamespace. Documents written by space_packet_parser 6.2 and earlier carry the old https URI; they are still recognized as XTCE 1.2 (LEGACY_XTCE_XMLNS_ALIASES), validation names the cause instead of giving a generic namespace error, and re-serializing repairs the URI. Code that string-compares namespace URIs against these constants will see the new values.

Two smaller behavior changes are listed under ### Changed: xtce_ns_prefix=None now binds a default namespace rather than producing a document with no namespace, and serializing rewrites xsi:schemaLocation to the canonical OMG URL.

Testing

589 tests (108 new), passing on ubuntu/macos/windows across Python 3.10–3.14.

• Parity. Every bundled mission definition (CTIM, JPSS, SUDA, IDEX) is rewritten into the 1.3 namespace in memory and must produce identical parameter types, parameters and containers, validate against the bundled 1.3 schema offline, decode its real packet file to identical values, and round-trip to a valid 1.3 document. No duplicated multi-hundred-KB fixtures.
• A genuinely-1.3 fixture. test_xtce_1_3_only_features.xml uses three constructs that exist only in 1.3. Rewritten into the 1.2 namespace it must still fail against the 1.2 schema, and fail on its content rather than on a namespace mismatch — so the test can't pass without the 1.3 schema actually being consulted.
• A drift guard pins test_xtce_1_3.xml to being a pure version-swap of test_xtce.xml, which several tests assume.
• Suite organization. Version-specific tests no longer sit interleaved with the general suite: test_xtce_1_3_features.py (1.3-only constructs), test_version_conversion.py (detection, retargeting, version-dependent serialization), test_version_validation.py (version-dependent validation), test_versions.py (the registry).
• A string-parsing benchmark was added; the suite had none, which is why a mid-PR performance regression on the terminator search was invisible to it and had to be caught in review.

Review

Reviewed adversarially over three rounds before opening, verifying claims against the OMG schemas and the CCSDS green book rather than against commit messages. That found seven substantive defects — including the VariableStringType gap and the LinearAdjustment slope default, both of which had originally been dismissed as validation-only.

The PR review then found nine more, all addressed here. The one ⚠️ was a real performance regression introduced mid-PR: making the terminator search character-aligned replaced bytes.index with a Python loop that sat on the existing 1.2 path. Fixed and measured — end-to-end parse_value is within 1–5% of main across a 256× range of string lengths, with a flat ratio.

Follow-ups

• Segregate XTCE 1.2 and 1.3 test expectations with a version-aware suite scheme #294 — the rest of the version-aware test scheme (parametrized fixture, xtce12/xtce13 markers, versioned test-data directories). Split out to keep both diffs reviewable.
• Bare UTF-16 / UTF-32 string encodings ignore the XTCE byteOrder attribute #295 — bare UTF-16/UTF-32 codecs ignore XTCE's byteOrder without a BOM. Pre-existing on main; fixing it changes decoded values for existing definitions.

Also deliberately not changed: XtcePacketDefinition.xtce_version still means Header/@version (the document's version), which reads confusingly next to the new xtce_standard_version. Renaming it is a breaking change worth doing on its own.

🤖 Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.36%. Comparing base (fa65e8c) to head (0bd3c37).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #291      +/-   ##
==========================================
+ Coverage   94.62%   95.36%   +0.74%     
==========================================
  Files          49       54       +5     
  Lines        4203     4812     +609     
==========================================
+ Hits         3977     4589     +612     
+ Misses        226      223       -3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

🟡 Changes recommended

Unresolved moderate issues remain in version conversion, metadata round-tripping, string decoding, bounds handling, and defaults.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds XTCE 1.3 support alongside XTCE 1.2, including version-aware namespaces, bundled schemas, validation, serialization, and variable-string parsing.

Changes:

• Adds XTCE version detection, schema resolution, and CLI reporting.
• Updates parsing and serialization for XTCE 1.3 features.
• Adds extensive unit, integration, fixture, and parity coverage.

File summaries

File	Review summary
tests/unit/test_xtce/test_versions.py	Version registry tests.
tests/unit/test_xtce/test_validation.py	Versioned schema and structural validation tests.
tests/unit/test_xtce/test_encodings.py	Moderate (1 vote): bare UTF-16/UTF-32 cases require endian-specific codec handling.
tests/unit/test_xtce/test_definitions.py	Definition conversion and round-trip tests.
tests/test_data/test_xtce_1_3.xml	XTCE 1.3 fixture.
tests/test_data/test_xtce_1_3_only_features.xml	XTCE 1.3-only feature fixture.
tests/integration/test_xtce_based_parsing/test_xtce_version_parity.py	Nit (1 vote): real-packet parity coverage omits SUDA and IDEX.
tests/integration/test_cli.py	XTCE 1.3 CLI coverage.
tests/conftest.py	Bundled schema fixture.
space_packet_parser/xtce/validation.py	Version-aware offline schema and structural validation.
space_packet_parser/xtce/schemas/SpaceSystem-20180204.xsd	Bundled XTCE 1.2 schema.
space_packet_parser/xtce/schemas/README.md	Schema provenance and checksums.
space_packet_parser/xtce/encodings.py	Moderate (1 vote): enforce maxSizeInBits; moderate (1 vote): normalize bare UTF codecs; moderate (1 vote): apply the empty TerminationChar default; nit (1 vote): clarify supported length/delimiter combinations.
space_packet_parser/xtce/definitions.py	Moderate (1 vote): cover version-specific units and references; moderate (2 votes): handle 1.3 time-unit serialization; moderate (1 vote): preserve systemType/assetType; nit (2 votes): correct constructor documentation.
space_packet_parser/xtce/containers.py	Schema-correct container serialization order.
space_packet_parser/xtce/__init__.py	XTCE version and namespace registry.
space_packet_parser/cli.py	XTCE and schema version reporting.
space_packet_parser/__init__.py	Public XTCE version exports.
README.md	XTCE support overview.
docs/source/user_guide/xtce_validation.md	Nit (1 vote): correct the invalid claim that spaced parameter names are valid in XTCE 1.2.
CHANGELOG.md	Documents XTCE 1.3 support and compatibility changes.
.pre-commit-config.yaml	Preserves bundled schema bytes.

Review details

Suppressed comments (9)

docs/source/user_guide/xtce_validation.md:96

• This says that a parameter name containing spaces is valid in XTCE 1.2, but the 1.2 schema's NameType already excludes spaces (SpaceSystem-20180204.xsd, lines 4989-4995). The version difference is in some reference syntaxes, not in allowing spaced parameter names; please correct this example so users do not construct an invalid 1.2 document based on the guide.

- **Name references.** XTCE 1.3 tightened the characters allowed in a parameter or container
  reference, excluding space and tab (and adding array subscript syntax). A definition whose
  parameter names contain spaces is valid XTCE 1.2 but not valid XTCE 1.3.

space_packet_parser/xtce/definitions.py:255

• This compatibility check only inspects variable-length string encodings, but the conversion docstring explicitly identifies time-unit enums and name references as version-specific. A 1.2 definition containing units="picoSeconds" or a reference with a space can be retargeted to 1.3, after which serialization writes the unchanged value and produces schema-invalid XML without any warning. Please either translate/reject these values or include them in the incompatibility warning so the documented conversion contract is enforced.

    def _warn_on_version_incompatible_encodings(self) -> None:
        """Warn about content that is valid in one XTCE version but not in the version being written.

        Retargeting a definition at another XTCE version rewrites namespaces, not element content, so
        a definition can hold a construct the target version's schema rejects. Variable-length strings
        are the case that actually bites: XTCE 1.2 requires a declared buffer length (`DynamicValue` or
        `DiscreteLookupList`) and treats `LeadingSize`/`TerminationChar` as optional, while XTCE 1.3
        makes the buffer length optional and requires exactly one of `LeadingSize`/`TerminationChar`.
        Either direction can therefore produce a document that fails schema validation.
        """
        version = self.xtce_standard_version
        if version not in ("1.2", "1.3"):
            return

        offenders = []
        for parameter_type_name, parameter_type in self.parameter_types.items():
            encoding = getattr(parameter_type, "encoding", None)
            if not isinstance(encoding, encodings.StringDataEncoding) or encoding.fixed_length:
                continue
            has_declared_length = bool(encoding.dynamic_length_reference or encoding.discrete_lookup_length)
            has_delimiter = bool(encoding.leading_length_size or encoding.termination_character)
            if version == "1.3" and not has_delimiter:
                offenders.append(parameter_type_name)
            elif version == "1.2" and not has_declared_length:

space_packet_parser/xtce/definitions.py:459

• from_xtce() captures only the root name and header fields here. XTCE 1.3's systemType (and assetType) are meaningful root attributes, but XtcePacketDefinition has no fields for them and to_xml_tree() never emits them; round-tripping the supplied 1.3 fixture silently changes systemType="asset" to the schema default unknown. Preserve these attributes or explicitly document the metadata loss.

            space_system_name=space_system.attrib.get("name", None),

space_packet_parser/xtce/encodings.py:345

• The new LeadingSize-only path uses the reported length directly and never checks max_size_in_bits, even though the XTCE schema defines that attribute as the upper bound for the variable string buffer. A packet whose size tag exceeds the declared maximum will therefore consume bytes beyond the field (potentially from the next field) instead of rejecting the over-sized value; enforce the bound before reading, ideally through a common check for every sizing branch.

        elif self.leading_length_size:
            # XTCE 1.3 permits a Variable string with no declared buffer length, in which case the
            # leading size tag delimits the buffer: the tag itself, plus the content length it reports.
            buflen_bits = self.leading_length_size + self._peek_leading_size(packet)
        elif self.termination_character is not None:
            # As above, but delimited by a terminator: the buffer runs up to and including it.
            buflen_bits = self._scan_to_termination_character(packet)
        else:
            raise ValueError("No raw length specifier found when decoding a string.")
        return int(buflen_bits)

space_packet_parser/xtce/encodings.py:186

• The constructor docstring says only one of termination_character, fixed_length, or leading_length_size may be set, but this class deliberately supports a declared raw length together with a delimiter (the existing tests construct dynamic_length_reference plus termination_character, and XTCE permits that combination). Please distinguish the raw-buffer length selectors from the optional content delimiter so callers are not told that a supported encoding is nonsensical.

        f"""Constructor
        Only one of termination_character, fixed_length, or leading_length_size should be set. Setting more than one
        is nonsensical.

space_packet_parser/xtce/encodings.py:166

• The newly supported bare UTF-16/UTF-32 spellings are still passed directly to Python's decode(self.encoding) (including during termination-character validation). Those codecs require a BOM; with XTCE's explicit byteOrder and the new BOM-less buffers, construction raises UnicodeDecodeError before _find_termination_character runs, and parsing would fail similarly. Normalize to the endian-specific codec selected by byte_order and preserve that setting.

        "UTF-16": 2,
        "UTF-16LE": 2,
        "UTF-16BE": 2,
        "UTF-32": 4,
        "UTF-32LE": 4,

space_packet_parser/xtce/encodings.py:565

• This newly added 1.3 path should also honor the XSD default on TerminationChar: both bundled schemas define it as default="00". A schema-valid empty <TerminationChar/> has text is None, so the assignment below treats the delimiter as absent and the constructor rejects the valid C-string. Apply the default when the element is empty.

            # A Variable element with neither is legal in XTCE 1.3, where the buffer length is derived
            # from the LeadingSize or TerminationChar read below. The constructor rejects the case
            # where neither of those is present either.

tests/integration/test_xtce_based_parsing/test_xtce_version_parity.py:85

• This parity test claims real-packet decoding coverage for the bundled mission definitions, but the parameter list contains only CTIM and JPSS while the definition parity tests also include SUDA and IDEX. The corresponding SUDA and IDEX packet fixtures are present under tests/test_data, so two of the four mission definitions in the PR's stated parity claim remain untested on actual packet bytes.

@pytest.mark.parametrize(
    ("definition_name", "packet_file_name", "root_container_name"),
    [
        ("ctim/ctim_xtce_v1.xml", "ctim/ccsds_2021_155_14_39_51", "CCSDSTelemetryPacket"),
        ("jpss/jpss1_geolocation_xtce_v1.xml", "jpss/J01_G011_LZ_2021-04-09T00-00-00Z_V01.DAT1", "CCSDSPacket"),
    ],

tests/unit/test_xtce/test_encodings.py:685

• These cases construct a bare UTF-16/UTF-32 encoding with an explicit byte_order, but StringDataEncoding.__init__ validates the termination bytes with bytes.decode(encoding). Python's BOM-dependent codecs reject these sequences without a BOM, so every bare case raises before _find_termination_character runs; parse_value would also decode with the bare codec and ignore the requested endianness. Resolve an explicit BE/LE codec from byte_order and use it consistently before adding these cases.

    encoding = encodings.StringDataEncoding(
        encoding=encoding_name,
        byte_order="mostSignificantByteFirst",
        termination_character="00" * char_width,

• Files reviewed: 21/23 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[medley56]

Fixed in 71d6b8b, as a diagnostic rather than a conversion.

_warn_on_version_incompatible_encodings now dispatches to two checks, and the new _warn_on_incompatible_time_units scans TimeParameterType.unit against TIME_UNITS_BY_VERSION, a new per-version table taken from each schema's TimeUnitsType enumeration. The warning names the parameter types, their units, and what the target version permits.

Not translating on the way out is deliberate: a unit is document content, and the PR's stated contract is that element content is passed through unchanged. Silently rewriting picoSeconds to picoseconds would make the output valid while changing what the author wrote. The gap was that the contract was documented but not enforced, which is what this closes.

Compound units (a tuple, from multiple <Unit> elements) are skipped — they are not expressible in the enumeration either way.

🤖 AI-assisted comment, reviewed and approved by @medley56 before posting.

[medley56]

Fixed in 71d6b8b. The docstring now states the mutual exclusion — passing both raises ValueError, and when ns is given the version is inferred from the URI it binds to xtce_ns_prefix — instead of saying the argument is ignored.

🤖 AI-assisted comment, reviewed and approved by @medley56 before posting.

[medley56]

I have no doubt that this works, but I have real concerns about the maintainability of our test suite after these changes. We now have test modules that mix expectations for XTCE 1.3 with expectations for XTCE 1.2 and future developers will have a very rough time parsing through those.

We need a test suite organizational system that handles segregating XTCE 1.3 tests from XTCE 1.2 tests in a way that is obvious to future human developers and AI agents. In other words, we need to keep our test modules human-readable even as we test across multiple XTCE versions.

[medley56]

The PR does what it says. I verified namespace-based version detection including the legacy https alias, the 1.3 VariableStringType shape, the LinearAdjustment slope default, EntryList/BaseContainer ordering, maxSizeInBits emission, and the bundled-schema checksums against both OMG XSDs and the checked-out head. Nothing blocks merging. The one warning is a performance regression on the existing 1.2 terminated-string path, where a C-level bytes.index became a Python loop. The rest is API tightening (version 1.1 is accepted as a serialization target), a more helpful error for legacy-namespace documents, test hygiene and organization, and CHANGELOG framing. The branch conflicts with main in CHANGELOG.md only, where #290 added a ### Fixed entry under [Unreleased]; a rebase resolves it. Copilot's existing comments on time units, the constructor docstring, systemType round-trip, and the unchecked max_size_in_bits on the LeadingSize path were independently confirmed and are not repeated here.

Severity tally: 0 critical, 1 warning, 7 suggestions, 0 nitpicks posted (2 dropped).

---

💡 SUGGESTION — 1.2 and 1.3 expectations are interleaved within test modules

Current state: version-awareness tests are a block appended at test_definitions.py:656-944 and test_validation.py:506-645, and the delimiter-derived string tests sit among general encoding tests at test_encodings.py:560-693. Only test_versions.py and test_xtce_version_parity.py are cleanly scoped. A reader cannot tell from a test's location or ID which version it targets, and there is no rule for where the next version-specific test goes.

A scheme that fits the existing layout:

1. Version-agnostic behavior (must hold identically in 1.2 and 1.3) stays in the existing modules, parametrized over a shared xtce_version fixture in tests/conftest.py with params=SUPPORTED_XTCE_VERSIONS and ids xtce12/xtce13, yielding the version and a matching definition path.
2. Version-specific behavior (delimiter-only strings, systemType, 1.2-only warnings) moves to test_xtce_1_3_features.py and test_xtce_1_2_features.py with a module-level pytestmark.
3. Cross-version behavior (retargeting, compatibility warnings, legacy alias repair, schemaLocation rewriting) moves to test_version_conversion.py; test_versions.py keeps the registry tests.
4. Register xtce12 and xtce13 markers in pyproject.toml so pytest -m xtce13 runs exactly the 1.3 surface and -m "not xtce13" proves nothing else depends on it.
5. Version-specific fixtures live under tests/test_data/xtce_1_3/; test_xtce.xml stays where it is to avoid churning every existing test.
6. test_encodings.py stays version-neutral: StringDataEncoding does not know about XTCE versions, so the delimiter-derived tests belong there with the "XTCE 1.3 form" phrasing kept to docstrings.

This can be a follow-up PR; nothing else in this review depends on it.

🤖 AI-assisted comment, reviewed and approved by @medley56 before posting.

[medley56]

⚠️ WARNING — terminator search is now a Python loop on the existing 1.2 path

On main, parse_value found the terminator with raw_string_buffer.index(...), a single C call. _find_termination_character replaces that with a Python loop that slices at every step, and parse_value at encodings.py:463 now routes every terminated string through it, including the pre-existing declared-length path, not just the new 1.3 derived-length scan. For UTF-8 and the single-byte encodings, which cover nearly every real definition, the cost is proportional to string length on every packet.

bytes.find with an alignment check keeps the character-boundary guarantee and is one C call in the width-1 case:

Suggested change

	char_width = self._FIXED_CHARACTER_WIDTH_BYTES.get(self.encoding, 1)
	for index in range(0, len(buffer) - len(self.termination_character) + 1, char_width):
	if buffer[index : index + len(self.termination_character)] == self.termination_character:
	return index
	return -1
	char_width = self._FIXED_CHARACTER_WIDTH_BYTES.get(self.encoding, 1)
	start = 0
	while (index := buffer.find(self.termination_character, start)) != -1:
	if index % char_width == 0:
	return index
	start = index + 1
	return -1

🤖 AI-assisted comment, reviewed and approved by @medley56 before posting.

[medley56]

Fixed in 900b581, taking the suggested bytes.find + alignment check.

You were right that this hit the 1.2 path, not just the new one. Measured before and after, UTF-8 with a one-byte terminator:

buffer	before	after
16 B	0.697 µs	0.072 µs	9.7×
64 B	2.388 µs	0.075 µs	31.6×
256 B	9.063 µs	0.080 µs	112.9×
1024 B	44.827 µs	0.096 µs	469.1×

Both edge cases still hold: the UTF-16BE straddle (b"\x41\x00\x00\x42" → no terminator, b"\x41\x00\x00\x42\x00\x00" → index 4) and the UTF-8 multi-byte terminator that a fixed stride would step over.

Also added test_benchmark__terminated_string_parsing to tests/benchmark/test_fast_benchmarks.py. The repo had no string-parsing benchmark, which is why this was invisible to the suite rather than caught by it.

🤖 AI-assisted comment, reviewed and approved by @medley56 before posting.

[medley56]

💡 SUGGESTION — xtce_standard_version="1.1" is accepted although 1.1 is not supported

The docstring at definitions.py:88-89 restricts this argument to SUPPORTED_XTCE_VERSIONS, but xtce_uri_for_version resolves any key of XTCE_XMLNS_BY_VERSION, which includes "1.1". Confirmed on the head: XtcePacketDefinition(xtce_standard_version="1.1") succeeds, the setter at definitions.py:217 does too, and to_xml_tree (definitions.py:326) then writes an xsi:schemaLocation pointing at the 1.1 XSD, which is not bundled. Validating that output triggers a network download instead of the offline validation the CHANGELOG promises for serialized documents. Rejecting versions outside SUPPORTED_XTCE_VERSIONS in the constructor and setter, while leaving xtce_uri_for_version permissive for detection, would close this. A test for the rejection is missing either way.

🤖 AI-assisted comment, reviewed and approved by @medley56 before posting.

[medley56]

Fixed in 71d6b8b.

Added _check_writable_xtce_version, called from the constructor and the setter, which rejects anything outside SUPPORTED_XTCE_VERSIONS. xtce_uri_for_version and XTCE_XMLNS_BY_VERSION stay permissive, so a document in the 1.1 namespace is still detected as 1.1 — which is what the module docstring says that entry is for. Only selecting 1.1 as a write target is refused, and the error says why (no bundled schema, so the output could not be validated offline).

test_cannot_select_a_version_with_no_bundled_schema covers both entry points over 1.0, 1.1 and 9.9, and test_xtce_1_1_namespace_is_still_detected pins the asymmetry.

🤖 AI-assisted comment, reviewed and approved by @medley56 before posting.

[medley56]

💡 SUGGESTION — legacy https namespace gets the generic namespace error

_validate_xtce_schema already recognizes the legacy https://www.omg.org/spec/XTCE/20180204 URI and sets xtce_version = "1.2" at validation.py:630-631, but the INVALID_XTCE_NAMESPACE message built here is the generic "does your xmlns match the targetNamespace" text. Running such a document through validate_xtce(level="schema") on the head gives valid=False, xtce_version="1.2", and nothing in the errors says this is the URI written by space_packet_parser <= 6.2 or that loading and re-writing the file repairs it. spp validate is where users will hit this first. A check for document_xtce_uri in LEGACY_XTCE_XMLNS_ALIASES with a one-sentence hint would make the error actionable.

🤖 AI-assisted comment, reviewed and approved by @medley56 before posting.

[medley56]

Fixed in 71d6b8b.

The INVALID_XTCE_NAMESPACE branch now checks document_xtce_uri in LEGACY_XTCE_XMLNS_ALIASES and, when it matches, replaces the generic text with one that names the URI as written by space_packet_parser 6.2 and earlier, gives the canonical URI, and says that reading the document with from_xtce and writing it back out repairs it. Every other URI keeps the generic message, and the error_code is unchanged so anything filtering on it is unaffected.

test_version_validation.py covers both sides: test_legacy_namespace_gets_a_specific_hint and test_non_legacy_wrong_namespace_gets_the_generic_hint, so an arbitrary wrong namespace does not get blamed on an old release.

🤖 AI-assisted comment, reviewed and approved by @medley56 before posting.

[medley56]

💡 SUGGESTION — two new error branches have no test

This non-byte-aligned raise is one of the two uncovered lines behind the 99.59% patch figure. It does fire as written (confirmed by setting _parsing_pos = 4 by hand). The other is the xtce_ns_uri is None raise in the xtce_standard_version setter at definitions.py:218-222; the neighbouring "not bound by its namespace mapping" branch is tested at test_definitions.py:939 but this one is not. Each is a one-test addition.

🤖 AI-assisted comment, reviewed and approved by @medley56 before posting.

[medley56]

Fixed in ddfe4f0. Both branches now have a test:

• test_termination_delimited_string_must_start_on_a_byte_boundary in test_encodings.py, which advances _parsing_pos by 3 bits before parsing.
• test_convert_definition_with_no_namespace_raises in the new test_version_conversion.py, next to the "not bound by its namespace mapping" test you mentioned.

🤖 AI-assisted comment, reviewed and approved by @medley56 before posting.

[medley56]

💡 SUGGESTION — test_xtce_1_3.xml is an unguarded hand copy of test_xtce.xml

The two fixtures differ only in the namespace/schema URLs and systemType="asset" on the root (diffed after normalizing the header date). This test, test_validation.py:514-531, and the packet-parity test below all assume they are twins, but nothing enforces it, so the next edit to test_xtce.xml silently makes the 1.3 tests exercise a different definition. test_xtce_version_parity.py:20-30 already derives a 1.3 document in memory with _as_xtce_1_3; a fixture doing the same here would remove the 209-line duplicate. If a checked-in 1.3 file is preferred, a test asserting the two differ only on the expected lines would serve as the drift guard.

🤖 AI-assisted comment, reviewed and approved by @medley56 before posting.

[medley56]

Fixed in ddfe4f0, with the drift guard rather than the in-memory fixture.

test_xtce_1_3_fixture_tracks_the_1_2_fixture normalizes the 1.3 file — namespace URI, schema URL, and systemType="asset" — and asserts the result equals test_xtce.xml exactly. Deriving it in memory would have meant moving test_validation.py and the CLI integration tests onto a fixture, and those want a real path on disk; and the helper would have had to inject systemType anyway to keep the 1.3-only attribute covered.

The guard moves with the fixture if tests/test_data/xtce_1_3/ happens, which is noted in #294.

🤖 AI-assisted comment, reviewed and approved by @medley56 before posting.

[medley56]

💡 SUGGESTION — unit test reads real JPSS packet data

This opens J01_G011_LZ_2021-04-09T00-00-00Z_V01.DAT1 and parses five real packets. The project instructions (.github/instructions/space_packet_parser.instructions.md:51) reserve real mission data for integration tests, and the same assertion already runs on that file in test_xtce_version_parity.py:80-102. Dropping this copy, or feeding it a synthetic packet from create_ccsds_packet as test_definitions.py:836 does, keeps the unit suite self-contained.

🤖 AI-assisted comment, reviewed and approved by @medley56 before posting.

[medley56]

Fixed in ddfe4f0. The test now builds its packet with create_ccsds_packet(data=bytes(range(65)), apid=11) — 65 bytes being the JPSS_ATT_EPHEM payload this definition describes — and the jpss_test_data_dir fixture is gone. Varying bytes rather than zeros, so a field read at the wrong offset changes the result.

It also moved to test_version_conversion.py as part of the split, and its docstring points at the integration parity test for the real-bytes version.

While there: that parity test only covered CTIM and JPSS for packet decoding, though it covered all four missions for definition parity. It now covers SUDA (with skip_header_bytes=4) and IDEX too. Still ~5 s.

🤖 AI-assisted comment, reviewed and approved by @medley56 before posting.

[medley56]

💡 SUGGESTION — "Breaking" label commits the next release to 7.0.0

Under the semver policy this file declares, this label means the next release is 7.0.0. Two other behavior changes in the same PR carry no label: XtcePacketDefinition(xtce_ns_prefix=None) now binds a default namespace (CHANGELOG.md:57-58), and from_xtce no longer raises for documents binding multiple XTCE-looking URIs (CHANGELOG.md:90-93, listed under Fixed). Either the old constant values were a bug (they never produced a validatable document, and LEGACY_XTCE_XMLNS_ALIASES keeps reading them working), in which case this belongs under Fixed with the compatibility paragraph kept, or this is 7.0 and the other two entries need the same label.

🤖 AI-assisted comment, reviewed and approved by @medley56 before posting.

[medley56]

Fixed in 3065af0, taking the first of your two options: next release is 6.3.0.

The constant correction moved to ### Fixed and lost the Breaking label, on the grounds you gave — the old values never produced a schema-validatable document, and LEGACY_XTCE_XMLNS_ALIASES keeps reading documents that use them working. The compatibility paragraph stays with it, and the sentence about imported constants now points at it rather than standing alone.

### Changed keeps the two behavior changes that really are changes: xtce_ns_prefix=None binding a default namespace, and xsi:schemaLocation being rewritten to the canonical OMG URL on serialize. Neither is breaking under semver.

🤖 AI-assisted comment, reviewed and approved by @medley56 before posting.

[medley56]

Review feedback addressed. Branch rebased onto main (the CHANGELOG.md conflict with #290 is resolved keeping both sides) and force-pushed; head is now 3065af0.

Each inline thread has a reply. This covers the review-level comment and the nine items Copilot folded into its review body, which have no threads to reply on.

Test suite organization

Done partially here, rest in #294, since you noted it could be a follow-up and nothing else depended on it. Version-specific tests are no longer interleaved with the general suite — they moved out of test_definitions.py and test_validation.py into modules whose remit is stated in the docstring:

module	remit
test_versions.py	the version registry
test_xtce_1_3_features.py	constructs that exist only in 1.3
test_version_conversion.py	detection, retargeting, version-dependent serialization
test_version_validation.py	version-dependent validation

The parametrized fixture, the xtce12/xtce13 markers, and tests/test_data/xtce_1_3/ are in #294. That last one interacts with the fixture drift guard added here, which is noted in the issue.

Copilot review-body items (no threads)

Fixed:

• maxSizeInBits not enforced on the LeadingSize path (71d6b8b) — correct, and it mattered: the size tag is packet data, and on the derived path it alone determines how much of the packet the string consumes, so an oversized tag consumed later fields. Bounded, with a test. Scoped to the 1.3 derived path deliberately — extending it to dynamic_length_reference/discrete_lookup_length as the comment suggested in passing would start rejecting 1.2 documents that parse today.
• Empty <TerminationChar/> (900b581) — correct, default="00" in both schemas, and nothing here applies XSD defaults, so a schema-valid C string was rejected.
• Stale "only one of…" in StringDataEncoding.__init__ (900b581) — correct, it contradicted the paragraph directly below it.
• systemType/assetType dropped on round trip (71d6b8b) — now preserved as space_system_type and asset_type; serializing as 1.2, where they do not exist, drops them with a warning.
• Real-packet parity missing SUDA and IDEX (ddfe4f0) — added.
• The "spaces in parameter names are valid 1.2" claim (3065af0) — this one was my error, in both the user guide and a docstring. 1.2's NameType is [^./:\[\] ]+, which already excludes space. The 1.2→1.3 tightening is in the reference patterns: 1.2's NameReferenceType permitted space and tab inside a path, and 1.3's four reference types do not, while adding [n] subscripts. Corrected to describe that.

Declined:

• Bare UTF-16/UTF-32 raising UnicodeDecodeError on construction. This does not reproduce. b"\x00\x00".decode("UTF-16") is "\x00" — one character — so it passes the constructor's single-character terminator check, and the tests in question pass as written. There is a real residual issue underneath: without a BOM those codecs default to little-endian and ignore XTCE's byteOrder. That predates this PR (both spellings are in _supported_encodings on main unchanged), and fixing it changes decoded values for existing definitions, so it is Bare UTF-16 / UTF-32 string encodings ignore the XTCE byteOrder attribute #295 rather than more scope here.

Verification

589 tests pass (579 plus benchmarks), ruff check clean, all pre-commit hooks pass. The terminator fix was measured rather than assumed — numbers are in the reply on that thread.

🤖 AI-assisted comment, reviewed and approved by @medley56 before posting.

[greglucas]

Reading this from a high-level there is a lot of extra handling for 1.2 vs 1.3 even though there isn't much that changed. I'm wondering if we really need to support writing 1.2 documents versus just being able to read them still which I think is compatible regardless? If someone gives us a 1.2 document we can still parse it equivalently, but if someone gives us a 1.3 document we issue warnings if we try to serialize to a 1.2 version. That seems like an unnecessary guard for all this extra code where we could just add the new features/defaults and explain what changed in the version.

[greglucas]

I don't think you need explanations for 1.3 vs 1.2 differences here. You should just document this as a changed default in this version of space packet parser.