Repository navigation
fix: tolerate string subtype metadata - #774
zhongkechen wants to merge 5 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Instead of preparing OTel and testing ahead of core, is it possible to do the whole change in one PR with a minimum required dependency for the consuming repos? then we don't need to tip-toe around "maybe it's a str, maybe it's an enum", we just fix it in one go now that we're doing a coordinated release of everything anyway? This coming release already ships core, OTel, and testing together. OTel and testing will both require core 2.1.0. So if all three switch the subtype to
The 2.1.0 release notes should then say that custom subtypes need OTel 1.1.0 and testing 1.3.0. With a custom subtype, OTel 1.0.0 creates no span for the operation, and testing 1.2.1 raises an error. |
|
@yaythomas, coordinating the full migration and its consumer dependency floors is a reasonable approach for the eventual subtype feature. The release we are preparing here is a minor release, so we are keeping the current public subtype contract and reserving the #749 type migration for a separately reviewed major release. Dependency floors align package versions; they do not migrate application or third-party plugin code. Public fields currently typed as #774 therefore remains the three metadata-read compatibility changes, with the enum, public annotations, parsers, configuration validation, serializers, replay behavior and dependency floors preserved. #764 remains the SDK 2.1.0 / OTel 1.1.0 / testing 1.3.0 minor-release preparation. Full custom-subtype support and the coordinated type migration belong to the major-release work. Both PR descriptions now state this boundary explicitly. |
This comment has been minimized.
This comment has been minimized.
Codex AI reviewNo actionable findings in the PR changes. Tests were inspected but not executed under the read-only review constraints, so runtime and integration results remain unverified. Reviewed commit |
OTel and testing history projection assume every subtype has a
.valueattribute, so plain-string metadata raisesAttributeError. Read enum values or plain strings in both OTel views and the testing history-event projection, preserving the recorded label and explicit operation type.This preparation keeps checkpoint parsing and typed history reconstruction enum-only. Unknown checkpoint subtypes are rejected before state/token mutation, and typed operation results retain the existing enum contract. Core
OperationSubType, public type annotations, configuration, serializers, replay identity, versions and dependency floors are unchanged. This PR is compatibility preparation for the coordinated minor release. The public subtype type migration and broader custom-subtype parsing/persistence/API changes proposed in issue #749 are reserved for a separately reviewed major release. Coordinating package releases or raising consumer dependency floors does not remove the impact of a public type change on application and third-party plugin code.Review exposed an unsafe interaction in the initial unknown-string parser fallback: an in-memory checkpoint could consume its token before response serialization failed. The final scope restores both existing parsers instead of broadening core serialization or public result types. Regression coverage exercises the actual checkpoint endpoint with real invocations and memory, filesystem and SQLite stores; known labels remain accepted, unknown labels leave state/token intact, and the old token remains usable after rejection.
Validation: the affected testing suite passes 1,615 tests; the 210-test focused OTel/history/endpoint set and six-file type check pass, including an existing typed
.valuecaller. Both OTel implementation files are unchanged from the previously validated 676-test suite. Formatting and diff checks pass. No end-to-end current-core custom-subtype or future-core compatibility claim is made.Related review: #764 (comment)
Related future work: #749