Skip to content

fix: tolerate string subtype metadata - #774

Open
zhongkechen wants to merge 5 commits into
mainfrom
maintenance/python-subtype-consumer-prep-20261014
Open

zhongkechen wants to merge 5 commits into
mainfrom
maintenance/python-subtype-consumer-prep-20261014

Conversation

@zhongkechen

@zhongkechen zhongkechen commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

OTel and testing history projection assume every subtype has a .value attribute, so plain-string metadata raises AttributeError. 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 .value caller. 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

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 14, 2026 00:19 — with GitHub Actions Active
@zhongkechen zhongkechen mentioned this pull request Oct 14, 2026
1 of 7 tasks
@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 14, 2026 00:29 — with GitHub Actions Active
@github-actions

This comment has been minimized.

@zhongkechen zhongkechen changed the title fix: accept string subtypes in companion readers fix: tolerate string subtype metadata Oct 14, 2026
@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 14, 2026 01:41 — with GitHub Actions Active
@github-actions

This comment has been minimized.

@yaythomas

yaythomas commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

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 str in this release, each package only ever reads a str. Then no package needs code that handles both an enum member and a string, and nothing has to be removed later?

  1. One PR implements the plan in #749. Every subtype field in core and testing becomes str | None.
  2. Testing keeps each subtype as the string it receives, like the service does. OTel and insight read the subtype as a str.
  3. The same PR changes testing's and insight's core requirement to aws-durable-execution-sdk-python>=2.1.0. OTel already requires that.
  4. chore: prepare Python minor releases #764 waits for that PR, as it already waits for fix(otel): isolate invocations and replay telemetry #758. Then one release ships SDK 2.1.0, OTel 1.1.0, and testing 1.3.0. Insight isn't published, so it doesn't need a release.

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.

@zhongkechen

Copy link
Copy Markdown
Contributor Author

@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 OperationSubType | None would become str | None, changing the contract for typed callers that use enum accessors such as .value. Built-in StrEnum members can retain those accessors at runtime, but that does not preserve the declared public type or the existing string/equality semantics. The proposed #749 plan also changes validation for existing ChildConfig.sub_type values.

#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.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 14, 2026 17:27 — with GitHub Actions Active
@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 14, 2026 19:23 — with GitHub Actions Active
@github-actions

Copy link
Copy Markdown
Contributor

Codex AI review

No 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 bbceb88042fe2e2dc3a6d1608d700c97676b7236. Workflow run

This branch was successfully deployed

1 active deployment
ai-pr-review-runtime — bbceb880 Deployed Oct 14, 2026 by zhongkechen via ai-pr-review / Codex review / Generate Codex review #1685
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants