Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69da1abdf9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| serialization_context_continue_as_new.Feature, | ||
| serialization_context_external_signal.Feature, | ||
| serialization_context_failure.Feature, | ||
| serialization_context_local_activity_payloads.Feature, |
There was a problem hiding this comment.
When the full Go feature suite runs, this registration executes local_activity_payloads, but its README explicitly documents that the current Go SDK encodes the local-activity result without context and then decodes it with workflow context. The signing codec therefore returns a context-mismatch error before CheckResult; because the config has no skip or expected-failure mechanism, every supported current Go run reports a failure. Skip this implementation until the SDK gap is fixed.
Useful? React with 👍 / 👎.
| serialization_context.child_workflow_payloads.feature.Impl.class, | ||
| serialization_context.continue_as_new.feature.Impl.class, |
There was a problem hiding this comment.
When the Java runner is given the full feature list discovered by GlobFeatures, it includes serialization_context/child_workflow_payloads_default_id because that directory now contains feature.java. Main resolves every requested directory through PreparedFeature.ALL, but this list jumps directly from the explicit-ID child feature to continue_as_new, so it throws NoSuchElementException and aborts the Java harness before running the remaining features.
Useful? React with 👍 / 👎.
68fc88c to
989fab2
Compare
|
PHP part is in separate PR: xepozz#1 |
|
Generally looks good! Other then the codex comments and failing CI jobs |
Register child_workflow_payloads_default_id in PreparedFeature.ALL. Drop the go minVersion gate: features.go imports every feature, so it never guarded compilation. Drop the Go local_activity_payloads until sdk-go#2562 is released.
The marshaller drops repeated fields coming from the protobuf C extension, which the runtime image loads, so schedule describe fails on the released SDK. Pin the branch that carries the fix and stop passing an explicit PHP version, which would override the pin.
The branch carrying the marshaller and serialization context fixes is serialization-context2, not feature/serialization-context2.
php-ver was built as 'v' + the version, so the empty version the pin needs produced "v" and build-image rejected it as invalid semver. The job takes no repo ref, so there is no release to build an image from until the fix ships.
|
@Quinn-With-Two-Ns all green. I think I'll release a new minor/patch after serialization context merged and revert composer/ci changes |
2 participants
What was changed
Why?
Implementing Serialization Context for PHP, need parity tests
Checklist
Closes
How was this tested: