Skip to content

feat(plugin): stabilize plugin interface and otel package - #625

Merged
zhongkechen merged 2 commits into
mainfrom
codex/stabilize-plugin-interface
Aug 11, 2026
Merged

feat(plugin): stabilize plugin interface and otel package#625
zhongkechen merged 2 commits into
mainfrom
codex/stabilize-plugin-interface

Conversation

@zhongkechen

@zhongkechen zhongkechen commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Summary

  • remove the experimental documentation and runtime FutureWarning from the plugins handler parameter
  • keep payload-bearing plugin API fields experimental with source documentation and dataclass metadata
  • require every plugin info field containing input, output, result, or error to carry the experimental marker
  • mark the OTel distribution as Production/Stable
  • add regression coverage for warning behavior, payload-field markers, and package metadata

Testing

  • focused payload-field marker test passed
  • Python compilation passed for all changed Python files
  • OTel package metadata assertions passed
  • git diff --check passed

The local shell only provides Python 3.10, while the SDK requires Python 3.11+, so the focused test used compatibility shims for typing.Self, datetime.UTC, and enum.StrEnum. CI will run the full suite with the supported toolchain.

@zhongkechen
zhongkechen temporarily deployed to ai-pr-review-runtime August 11, 2026 04:43 — with GitHub Actions Inactive
@zhongkechen
zhongkechen temporarily deployed to ai-pr-review-runtime August 11, 2026 04:43 — with GitHub Actions Inactive
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen temporarily deployed to ai-pr-review-runtime August 11, 2026 04:48 — with GitHub Actions Inactive
@zhongkechen
zhongkechen temporarily deployed to ai-pr-review-runtime August 11, 2026 04:48 — with GitHub Actions Inactive
@github-actions

Copy link
Copy Markdown
Contributor

Codex AI review

No actionable findings. Residual risk is limited to tests not being executed under the review constraints.

Reviewed commit 7ac7acc6a7dae231f2abbb8e37f9780cc9b89af0. Workflow run

@github-actions

Copy link
Copy Markdown
Contributor

Claude AI review

No actionable findings.

This is a clean, well-scoped stabilization PR:

  • execution.py — removes the FutureWarning on the plugins parameter and the now-unused warnings import; docstring updated consistently. No remaining references to the removed warning.
  • plugin.py — adds metadata={"experimental": True} plus attribute docstrings to OperationInfo.result, OperationInfo.error, and InvocationEndInfo.error. InvocationEndInfo.error retains default=None and stays non-kw_only, so dataclass field ordering is unaffected. Inherited fields keep their metadata, so the new marker test correctly covers subclasses.
  • otel/pyproject.toml — classifier bumped to Development Status :: 5 - Production/Stable, matched by the new test_package_metadata.py.
  • Teststest_durable_execution_loads_plugins_when_handler_is_initialized now escalates FutureWarning to an error under warnings.catch_warnings(), correctly asserting the new no-warning behavior; test_payload_fields_are_marked_experimental enforces the experimental marker across all nine plugin info dataclasses.

The attribute docstrings are no-op statements and are not flagged by the configured Ruff rule set (E4/E7/E9/F/TID252; no B018).

Residual test risk (not a finding):

  • test_payload_fields_are_marked_experimental iterates a hardcoded tuple of info types; a future info dataclass with an input/output/result/error field would need to be added to that tuple to stay covered.
  • Per the PR description, local runs used Python 3.10 compatibility shims (typing.Self, datetime.UTC, enum.StrEnum) since the SDK targets 3.11+; the full suite is expected to run on CI with the supported toolchain.
  • Marking the OTel distribution Production/Stable while its payload fields (result/error) remain flagged EXPERIMENTAL is an intentional product/classifier decision, not a code defect.

Reviewed commit 7ac7acc6a7dae231f2abbb8e37f9780cc9b89af0. Workflow run

@zhongkechen
zhongkechen merged commit c93073f into main Aug 11, 2026
35 of 38 checks passed
@zhongkechen
zhongkechen deleted the codex/stabilize-plugin-interface branch August 11, 2026 18:03
wangyb-A pushed a commit that referenced this pull request Aug 11, 2026
Mark execution_input and execution_result with
metadata={"experimental": True} and a leading EXPERIMENTAL docstring,
matching the pattern #625 established for OperationInfo.result /
OperationInfo.error / InvocationEndInfo.error.

Unlike a docstring alone the metadata is introspectable at runtime, so
the generic test #625 added -- which requires the marker on every
plugin info field named for input, output, result or error -- now
covers these two fields. That test fails on this branch without them.

This matters more since #625 removed the plugins= FutureWarning: the
per-field markers are now the only stability signal on these surfaces.

Refs #616
wangyb-A pushed a commit that referenced this pull request Aug 11, 2026
Mark execution_input and execution_result with
metadata={"experimental": True} and a leading EXPERIMENTAL docstring,
matching the pattern #625 established for OperationInfo.result /
OperationInfo.error / InvocationEndInfo.error.

Unlike a docstring alone the metadata is introspectable at runtime, so
the generic test #625 added -- which requires the marker on every
plugin info field named for input, output, result or error -- now
covers these two fields. That test fails on this branch without them.

This matters more since #625 removed the plugins= FutureWarning: the
per-field markers are now the only stability signal on these surfaces.

Refs #616
wangyb-A pushed a commit that referenced this pull request Aug 11, 2026
Addresses the medium-severity Codex review comment on #616.

The payload fields joined the generated __eq__ and __hash__, so the
widening was not additive. execution_input holds arbitrary deserialized
JSON, and a dict or list value made a previously hashable
InvocationStartInfo raise TypeError on hash(); both fields also made
infos built from the earlier field set compare unequal to infos
carrying a payload. Both effects were reproduced first.

Set compare=False, hash=False on execution_input and execution_result.
Identity fields still drive equality, so payload-only differences now
compare equal -- payloads are incidental data on what is otherwise an
event record.

Adds tests for hashability across dict, list, nested and scalar
payloads, for equality against the prior field set, and for the field
declarations themselves.

Note OperationInfo.result / OperationInfo.error from #625 remain in
compare. They are hashable types so they do not break hash(), but the
equality asymmetry with these fields is worth a maintainer decision.

Refs #616
wangyb-A pushed a commit that referenced this pull request Aug 11, 2026
Addresses the medium-severity Codex review comment on #616.

The payload fields joined the generated __eq__ and __hash__, so the
widening was not additive. execution_input holds arbitrary deserialized
JSON, and a dict or list value made a previously hashable
InvocationStartInfo raise TypeError on hash(); both fields also made
infos built from the earlier field set compare unequal to infos
carrying a payload. Both effects were reproduced first.

Set compare=False, hash=False on execution_input and execution_result.
Identity fields still drive equality, so payload-only differences now
compare equal -- payloads are incidental data on what is otherwise an
event record.

Adds tests for hashability across dict, list, nested and scalar
payloads, for equality against the prior field set, and for the field
declarations themselves.

Note OperationInfo.result / OperationInfo.error from #625 remain in
compare. They are hashable types so they do not break hash(), but the
equality asymmetry with these fields is worth a maintainer decision.

Refs #616
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