Repository navigation
fix: tolerate string subtype metadata - #774
zhongkechen wants to merge 10 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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Codex AI reviewNo actionable findings in the PR diff. Regression tests cover subtype projection, enum contracts, and checkpoint rejection across all three stores. Residual test risk: tests were inspected but not executed under the review restrictions. Reviewed commit |
|
On core 2.x the new code changes nothing, because every subtype is still an enum member. It would only matter if a user upgraded core to 3.0, used custom subtypes, and kept OTel 1.1.0 or testing 1.3.0. Those users have to upgrade OTel and testing anyway, because the testing checkpoint endpoint still rejects custom subtypes, and the 3.0 release notes have to tell OTel 1.0.0 users to upgrade regardless. Accordingly, after discussion with @zhongkechen, will close this PR unmerged and do this work for next major release. |
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.Base integration
321d597ebf9e3c42793598ccfbc23c839009fc13incorporates main04daf4081b804a4d871e0a7ae0d445af7c46541c: the MicroVM worker reports heartbeat-start failures and rejects oversized Content-Length values. Only the worker implementation and its tests change from715fedeb21635de1642633ee7f4cf96f9a98f0bc. The identical worker package tree passed 261 standalone tests and 13-file type/format checks once; this PR also passed 6 actual worker/caller integration tests with its own Core/testing/Extras sources. Core, OTel, testing, Extras, version/dependency declarations and workflow/test references are byte-unchanged by this integration. Their previous validation remains recorded at715fedeb: 1836 Core + 676 OTel tests; both typechecks and Core fmt passed; exact-source imports verified. Current-head CI runs separately.Related review: #764 (comment)
Related future work: #749