Repository navigation
Keep encoded failure attributes in StrandsPlugin - #155
Conversation
Breaking change (shipped in temporalio-strands-agents 0.1.0): StrandsPlugin now keeps the Client's payload codec and external storage. temporalio.contrib.strands in temporalio 1.34.0 and earlier dropped them, so its payloads are plain JSON. A codec that rejects payloads it did not encode now fails to read results of runs finished before the upgrade, and fails the Workflow Tasks of runs still open. Callers with such a codec must let it pass unencoded payloads through before upgrading. Rolling Workers back to temporalio.contrib.strands needs a plugin after StrandsPlugin that sets the codec again. Add an upgrade section to the README that says this.
StrandsPlugin replaced the failure converter of the Client's DataConverter with StrandsFailureConverter. A caller that set DefaultFailureConverterWithEncodedAttributes, so that failure messages and stack traces are encoded with the payload codec, had them stored as written. Keep the caller's failure converter class and add the Strands exception handling to it. The SDK default still becomes StrandsFailureConverter. Decoding is unchanged: every DefaultFailureConverter reads encoded and plain failure attributes, so old histories replay and older Workers read the new failures.
Replace the previous follow-up, which kept any caller failure converter class and subclassed it. That subclass inherited the caller's from_failure, which StrandsFailureConverter had replaced, so a caller converter that rebuilds its own exceptions changed the errors Workflow code reads and broke replay across the change. Map DefaultFailureConverterWithEncodedAttributes to a private StrandsFailureConverterWithEncodedAttributes, a StrandsFailureConverter that encodes common attributes. Every other class still becomes StrandsFailureConverter, as before. Decoding is unchanged: both classes use DefaultFailureConverter.from_failure, which reads encoded and plain failure attributes, so old histories replay and older Workers read the new failures. The README rollback example now restores the external storage as well as the codec: temporalio.contrib.strands drops both, and a Worker without the storage cannot read offloaded payloads.
| uv add temporalio-strands-agents | ||
| ``` | ||
|
|
||
| ### Upgrading from `temporalio.contrib.strands` |
There was a problem hiding this comment.
Since we're in public preview, I don't feel the need to document every change this verbosely. This is also a bug fix: Before, the payload codec was dropped, now it isn't, so I think the release notes are the best place to document this if it isn't included there already. Also, this seems largely unrelated to the rest of the content of this PR (failure converters).
There was a problem hiding this comment.
Agreed. I removed the section in 8ac59bc; the PR keeps only the one sentence on encoded failures in the interrupts section.
A short version for the next release notes, with DABH's corrections folded in:
Since 0.1.0,
StrandsPluginkeeps the payload codec and external storage of the Client'sDataConverter.temporalio.contrib.strandsintemporalio1.34.0 and earlier dropped them when the payload converter was the SDK default, so the payloads it wrote are plain JSON. If your codec rejects payloads it did not encode, change itsdecodeto pass through payloads without its own encoding before you upgrade, and don't run Workers with the old and the new plugin on the same Task Queue while those runs are open.With
DefaultFailureConverterWithEncodedAttributes(or a subclass), the plugin now keeps the encoding, so failure messages in History readEncoded failure(#155).
DABH
left a comment
DABH
left a comment
There was a problem hiding this comment.
Review of the encoded-failure-attributes change. Each code finding was reproduced on this branch with temporalio 1.34.0 (and the SDK's bundled temporalio.contrib.strands for the old-plugin comparisons); the rolling-upgrade finding was reproduced end to end on a dev server.
Severity: P1 = wrong result or stuck run in realistic use; P2 = correctness or robustness in an edge case; P3 = docs, tests, cleanup.
The two items that matter most: the exact-class identity check silently writes plaintext failure messages and stack traces for any caller subclass that enables encoding (the very outcome #153 reports), and a pydantic_data_converter-based caller converter, which the README names as the plugin's default, bypasses the Strands failure converter entirely so interrupts never work on that path.
Findings on lines outside the diff
python/strands_agents/src/temporalio/strands_agents/_plugin.py:157: P1: a non-default payload converter gets no Strands failure handling at all, and the README now implies otherwise.elif converter.payload_converter_class is not DefaultPayloadConverter: return converterreturns the caller's converter untouched, so withClient.connect(data_converter=replace(pydantic_data_converter, failure_converter_class=DefaultFailureConverterWithEncodedAttributes, payload_codec=codec), plugins=[StrandsPlugin()])(pydantic is the converter the README calls the plugin's default) neither the Strands converter nor the new encoded variant is installed. Reproduced through the realencode_failure/decode_failureround trip andTemporalActivityTool.stream(): an activity raisingInterruptExceptionis stored astype='InterruptException', non_retryable=False, details=0, the interrupt branch at_temporal_activity_tool.py:75is never taken, the model is fed the interrupt as a tool error, and the activity is retried under the default policy. Pre-existing pass-through (the SDK's 1.34.0 plugin does the same), andtest_custom_payload_converter_is_preservedpins it, but the PR body defers it and the README's new sentence at line 546 scopes only the encoding to 'the default payload converter' without saying the opposite case loses interrupts. The openai_agents plugin raises aValueErrorfor the equivalent mismatch; either do the same, applystrands_failure_converter_classon this path too, or state the consequence in the README.
Pre-existing items on lines outside the diff, for completeness: terminal Strands exceptions are re-created as fresh ApplicationErrors in _failure_converter.py (around line 65), dropping __traceback__ and __cause__, so the Failure proto has no stack trace and no cause chain (and the encoded variant encodes an empty stack trace), unlike DefaultFailureConverter.to_failure; the comment above _TERMINAL_EXCEPTIONS (around line 32) says TemporalAgent.invoke_async rewraps them as StrandsWorkflowError, a class that does not exist anywhere in the package; and under codec misconfiguration _temporal_activity_tool.py:95 feeds the literal 'Encoded failure' to the model as the tool error, with no fallback to cause.type.
| with ``StrandsFailureConverter``, so the failures that Workflow code reads stay | ||
| the same. | ||
| """ | ||
| if base in ( |
There was a problem hiding this comment.
P1: the identity check silently downgrades encoding subclasses to plaintext. base in (DefaultFailureConverterWithEncodedAttributes, StrandsFailureConverterWithEncodedAttributes) is an exact-class test, so class MyEnc(DefaultFailureConverterWithEncodedAttributes): pass, a DefaultFailureConverter subclass whose __init__ passes encode_common_attributes=True, and functools.partial(DefaultFailureConverter, encode_common_attributes=True) all map to plain StrandsFailureConverter. Verified on the branch: to_failure(ApplicationError('card 4111-1111')) then stores message='card 4111-1111' and a 192-character plaintext stack trace in history, with zero warnings or log lines, versus 'Encoded failure' for the exact SDK class. The PR's rationale ('a class with its own from_failure changes the errors Workflow code reads') does not apply to these: their from_failure is DefaultFailureConverter.from_failure, and the SDK decoder reads encoded_attributes unconditionally, so mapping them to the encoded Strands class changes only the write side. Use issubclass, or check base.from_failure is DefaultFailureConverter.from_failure plus the encode flag (the SDK already instantiates the class in DataConverter.__post_init__), and at minimum warn when a caller's class is replaced. The docstring at line 94 should also say the match is by exact identity.
There was a problem hiding this comment.
Fixed in dc2f54c: any subclass of DefaultFailureConverterWithEncodedAttributes now gets StrandsFailureConverterWithEncodedAttributes (issubclass, guarded so a functools.partial doesn't raise). As you say, the plugin never keeps the caller's from_failure, so this changes only the write side. test_caller_encoded_failure_converter_is_kept now also runs with a subclass; those cases fail on 891685d with assert 'secret' == 'Encoded failure'.
A plain DefaultFailureConverter subclass that sets the flag in its own __init__, or a partial, is still replaced by StrandsFailureConverter; the docstring says so. I didn't add a warning: those forms are rare, and a warning would also fire for every custom class that doesn't encode.
|
|
||
| `StrandsPlugin` keeps the payload codec and external storage of the `DataConverter` you pass to the Client. The `temporalio.contrib.strands` plugin in `temporalio` 1.34.0 and earlier dropped them, so the payloads it wrote are stored as plain JSON. After the upgrade, your codec also decodes those payloads: results of runs that finished before the upgrade, and the History of runs that are still open. If your codec rejects payloads it did not encode, `WorkflowHandle.result()` raises its error for those runs and their Workflow Tasks fail. Before you upgrade, change the codec's `decode` to return payloads without its encoding unchanged. | ||
|
|
||
| Do not roll Workers back to `temporalio.contrib.strands` while runs that this plugin wrote to are open: it drops your codec and external storage, so it cannot read their payloads. If you must roll back, add a plugin after `StrandsPlugin` that sets both again from the `DataConverter` you passed to the Client, for example `SimplePlugin("restore-converter", data_converter=lambda dc: dataclasses.replace(dc, payload_codec=converter.payload_codec, external_storage=converter.external_storage))`. |
There was a problem hiding this comment.
P1: the rollback warning applies equally to the rolling-upgrade window, and the README gives no rollout guidance. The stated condition ('old-plugin Workers running while runs this plugin wrote to are open') is exactly the state of a rolling upgrade on a shared task queue, yet it is attributed only to rolling back. Reproduced end to end on a dev server with a lenient decode exactly as line 15 prescribes: [A] a run started by the upgraded Client could not be progressed by an old-plugin Worker (WorkflowTaskFailed: Failed decoding arguments: 'Unknown payload encoding binary/b64') until that Worker was shut down; [B] with the old Worker polling activities, the activity input encoded by the upgraded Worker failed with a retryable ApplicationError: Failed decoding arguments, and under the bounded RetryPolicy(maximum_attempts=2) the README's own Retries section recommends, the workflow itself failed. A Client still on temporalio.contrib.strands cannot read results or failures written by upgraded Workers either. State that the warning applies while old Workers are still running, and advise draining, a new task queue, Worker Versioning, or a two-phase rollout that first deploys the restore-converter plugin on the old Workers.
The snippet itself also has three problems: it restores the codec and external storage but not failure-attribute encoding (the old plugin hard-codes plain StrandsFailureConverter, so every failure written after a rollback goes to history in plaintext, and restoring failure_converter_class instead loses the old plugin's interrupt translation; a subclass of the old converter with encode_common_attributes=True keeps both, verified); converter is a free variable the prose never defines; and dc is DataConverter | None, so dataclasses.replace(dc, ...) fails type checking and raises TypeError if the restore plugin is ordered first.
There was a problem hiding this comment.
The Upgrading section is gone in 8ac59bc, per Brian's comment on line 13: this belongs in the release notes. The rollback snippet went with it. I put your rolling-upgrade point (old and new Workers on one Task Queue can't read each other's payloads) into the release-note draft in Brian's thread.
|
|
||
| ### Upgrading from `temporalio.contrib.strands` | ||
|
|
||
| `StrandsPlugin` keeps the payload codec and external storage of the `DataConverter` you pass to the Client. The `temporalio.contrib.strands` plugin in `temporalio` 1.34.0 and earlier dropped them, so the payloads it wrote are stored as plain JSON. After the upgrade, your codec also decodes those payloads: results of runs that finished before the upgrade, and the History of runs that are still open. If your codec rejects payloads it did not encode, `WorkflowHandle.result()` raises its error for those runs and their Workflow Tasks fail. Before you upgrade, change the codec's `decode` to return payloads without its encoding unchanged. |
There was a problem hiding this comment.
P2: 'dropped them' is over-broad, and the decode advice is ambiguous. The 1.28.0 through 1.34.0 temporalio.contrib.strands plugin only replaced the converter when payload_converter_class was the SDK default (or none); a Pydantic-based converter kept its codec and external storage (reproduced: pydantic_data_converter + codec returns the same object with the codec intact; DataConverter.default + codec loses it). A user who passed DataConverter(payload_converter_class=PydanticPayloadConverter, payload_codec=EncryptionCodec()) always had encrypted history and would needlessly weaken decode by following this paragraph. For those users the upgrade-visible change is that interrupts start working, not payload encoding. Also, 'change the codec's decode to return payloads without its encoding unchanged' parses as 'strip the encoding'; the intended contract is to append input payloads whose metadata["encoding"] is not the codec's own marker to the result as-is (the SDK encryption sample pattern).
There was a problem hiding this comment.
Removed with the section in 8ac59bc. The release-note draft in Brian's thread limits the break to the SDK default payload converter and says decode should pass through payloads without its own encoding.
| uv add temporalio-strands-agents | ||
| ``` | ||
|
|
||
| ### Upgrading from `temporalio.contrib.strands` |
There was a problem hiding this comment.
P3: the Upgrading section covers only the move from temporalio.contrib.strands, not the change for users of the previous release of this package. A DefaultFailureConverterWithEncodedAttributes that was previously replaced is now honored, so raw history failure messages flip from plaintext to 'Encoded failure': temporal workflow show, the UI without a codec server, alerting on ActivityTaskFailed.failure.message, or tests asserting on raw history change behavior. Intended, but release notes come from commit subjects, so the README is the only place to flag it.
There was a problem hiding this comment.
The section is gone (8ac59bc, per Brian), so this went in two other places: the release-note draft in Brian's thread, and the PR description, which the squash commit carries. Both say that with DefaultFailureConverterWithEncodedAttributes, failure messages in History now read Encoded failure.
| """ | ||
| if base in ( | ||
| DefaultFailureConverterWithEncodedAttributes, | ||
| StrandsFailureConverterWithEncodedAttributes, |
There was a problem hiding this comment.
P3: this tuple entry is unreachable through the plugin, and the test that appears to cover it passes for another reason. _data_converter returns early at _plugin.py:157 for any converter whose payload class is not DefaultPayloadConverter, and the only branch that calls this function simultaneously sets payload_converter_class to pydantic's, so every converter the plugin configured short-circuits on a later pass; the function is never invoked with its own output class (verified by instrumenting both passes: the second call's input list is empty, and removing this entry keeps the assertion at tests/test_plugin.py:192-194 true for both targets). The comment there ('A second plugin pass, as a Worker built from the Client makes') is also inaccurate: SimplePlugin.configure_worker never resolves data_converter and the Worker reads client_config["data_converter"]; a second pass happens only via a Replayer or Client(**client.config()). To cover the entry, pass DataConverter(failure_converter_class=StrandsFailureConverterWithEncodedAttributes) through _configure.
There was a problem hiding this comment.
Right, the plugin's own output always returns early on the payload converter check. The tuple is gone in dc2f54c: the check is issubclass, and StrandsFailureConverterWithEncodedAttributes now subclasses DefaultFailureConverterWithEncodedAttributes, so it matches without a special case. I also fixed the test comment; the second _configure call now only claims that running the plugin on its own output keeps the class.
| super().to_failure(exception, payload_converter, failure) | ||
|
|
||
|
|
||
| class StrandsFailureConverterWithEncodedAttributes(StrandsFailureConverter): |
There was a problem hiding this comment.
P3: not a subclass of DefaultFailureConverterWithEncodedAttributes, so isinstance/issubclass guards keyed on the SDK class stop matching once the plugin has run (verified isinstance(StrandsFailureConverterWithEncodedAttributes(), DefaultFailureConverterWithEncodedAttributes) is False). A later plugin or startup guard that follows the same keep-or-replace pattern would treat the converter as non-encoding or replace it, dropping the Strands interrupt mapping. class StrandsFailureConverterWithEncodedAttributes(StrandsFailureConverter, DefaultFailureConverterWithEncodedAttributes) resolves cleanly and would also let strands_failure_converter_class use issubclass. The super().__init__(encode_common_attributes=True) at line 86 additionally relies on StrandsFailureConverter never defining __init__; a one-line forwarding __init__ on the base would make that explicit.
There was a problem hiding this comment.
Done in dc2f54c: it now subclasses both, so the __init__ override is gone and isinstance checks against the SDK class match. basedpyright flags the two bases (reportUnsafeMultipleInheritance), so the class has a targeted ignore and a comment saying why it's safe: StrandsFailureConverter has no __init__, so the SDK class's __init__ runs. The test's Encoded failure assertion fails if that ever changes.
| class StrandsFailureConverterWithEncodedAttributes(StrandsFailureConverter): | ||
| """``StrandsFailureConverter`` that moves message and stack trace to encoded attributes. | ||
|
|
||
| It encodes like ``DefaultFailureConverterWithEncodedAttributes`` and decodes like |
There was a problem hiding this comment.
P3 doc: 'decodes like StrandsFailureConverter' is vacuous; neither class overrides from_failure. The relevant fact for a reader checking Replayer symmetry is that DefaultFailureConverter.from_failure restores message and stack trace from encoded_attributes for every subclass, which is why a Replayer mapped to plain StrandsFailureConverter still decodes histories written by the encoded variant.
There was a problem hiding this comment.
Reworded in dc2f54c: the docstring now says both Strands classes decode with DefaultFailureConverter.from_failure, which reads encoded and plain attributes, so a Replayer with StrandsFailureConverter still reads what this class writes.
|
|
||
|
|
||
| @pytest.mark.parametrize("target", ["client", "replayer"]) | ||
| def test_caller_encoded_failure_converter_is_kept(target: str) -> None: |
There was a problem hiding this comment.
P3: the new tests never run the codec path they exist for. They call to_failure/from_failure directly with configured.payload_converter and no codec, bypassing DataConverter.encode_failure / decode_failure (which apply the codec to encoded_attributes and details) and the workflow-side rebuild of Interrupt from cause.details[0]. A regression where _apply_to_failure_payloads skipped encoded_attributes or details, or where from_failure's bare except: pass left message == 'Encoded failure' for _temporal_activity_tool.py:95, would pass CI. Extending test_activity_tool_interrupt with a codec plus DefaultFailureConverterWithEncodedAttributes on the Client, and replaying with the same converter, would cover it. Also, _configure routes every non-"client" string to the replayer branch, so a misspelled parametrize id silently drops client coverage; a Literal["client", "replayer"] or an else: raise AssertionError(target) makes that visible.
There was a problem hiding this comment.
Done in dc2f54c. test_activity_tool_interrupt now also runs with a codec and DefaultFailureConverterWithEncodedAttributes on the Client. It checks that the Activity failure in History reads Encoded failure and replays with the same converter. The Workflow rebuilds the interrupt from the codec-decoded details, so a codec that skipped details or encoded attributes would fail it. I moved CompressionCodec to tests/common.py for that, since importing tests.test_plugin from a Workflow module pulls botocore into the sandbox. _configure now raises on an unknown target.
The section documented a change that shipped in 0.1.0, not this fix, and belongs in the release notes.
…codedAttributes StrandsPlugin matched only the exact SDK class, so a caller subclass got plain StrandsFailureConverter and its failure messages were stored as written. Any subclass now gets StrandsFailureConverterWithEncodedAttributes, which also subclasses DefaultFailureConverterWithEncodedAttributes. The plugin still never keeps a caller's from_failure. The activity-tool interrupt test also runs with a codec and encoded failure attributes, and replays with the same converter.
campoy
changed the title
|
Thanks for the review. Replies are in each thread; the code changes are in dc2f54c and the README removal in 8ac59bc. On the items outside the diff:
|
DABH
left a comment
DABH
left a comment
There was a problem hiding this comment.
Second round, at head dc2f54c (two commits over 891685d: 8ac59bc removes the README section, dc2f54c changes the code and tests). On a worktree at the head: make lint clean and make test 59 of 59 on Python 3.14; the CI matrix is green, the branch merges cleanly with current main, and no strands_agents commit landed on main after that CI run. Probes against temporalio 1.34.0 at the head: converter selection for the SDK class, a subclass that overrides to_failure, and a functools.partial (the first two get StrandsFailureConverterWithEncodedAttributes, the partial gets plain StrandsFailureConverter as the docstring says); isinstance(StrandsFailureConverterWithEncodedAttributes(), DefaultFailureConverterWithEncodedAttributes) is True and the MRO runs the SDK class's __init__; and mixed-version decoding through DataConverter.encode_failure/decode_failure with the test codec in both directions: an InterruptException written by the encoded variant is stored as Encoded failure and read by plain StrandsFailureConverter as interrupt:approval, type StrandsInterrupt, non-retryable, details intact, and the reverse also round-trips, so a rolling upgrade from 0.1.0 is safe on the failure path. Refuted: a History-only change reaching commands. Every Workflow-side reader (_temporal_activity_tool.py, _temporal_sandbox.py, and main's _temporal_model.py overflow rebuild) reads the decoded ApplicationError, never the raw Failure.message. Not repeated: the dev-server cross-version replay in the PR body; the converter probe covers the same decode path.
Severity: P1 = wrong result or stuck run in realistic use; P2 = correctness or robustness in an edge case; P3 = docs, tests, cleanup.
Previous rounds. Addressed: the exact-class check is now issubclass, and test_caller_encoded_failure_converter_is_kept covers a subclass; the unreachable tuple entry is gone and the second-pass test comment now claims only what it checks; the class subclasses both converters, so isinstance guards keyed on the SDK class match; the docstring now gives the from_failure reason for Replayer symmetry; test_activity_tool_interrupt runs with a codec and DefaultFailureConverterWithEncodedAttributes, asserts Encoded failure in History and replays with that converter, and _configure raises on an unknown target. Accepted declines: no warning when a caller's class is replaced, and the own-__init__ and partial forms staying plaintext (rare, documented in the docstring). Superseded: the three README items (rolling upgrade and rollback, 'dropped them', the Upgrading section's scope) went with the section at the maintainer's request and are folded into the release-note draft in that thread; see the PR-body item below for how that draft reaches users. The non-default payload converter gap is pre-existing, unchanged here and filed as #299. The dropped traceback and cause, and Encoded failure reaching the model under a misconfigured codec, remain pre-existing follow-ups; the StrandsWorkflowError comment is fixed on main, and the merge keeps main's text.
Approving: the earlier subclass-encoding item is fixed, the other earlier P1 is pre-existing and tracked in #299, CI is green, and the two remaining items are non-blocking P3s.
Outside the diff
- P3 (PR body, non-blocking):
Closes #153will close an issue whose second half, the codec note for users moving offtemporalio.contrib.strands, now exists only as a draft in a review thread, and the generated release notes will not carry it.release_tool.py release-notesreads commit subjects only (git log --format=%h%x1f%s,scripts/release/release_tool.py:456), so the squash commit's body (this PR description) does not reach the notes either; the nextpython/strands_agentsrelease will list only this PR's title. The History change forDefaultFailureConverterWithEncodedAttributesusers (Encoded failurein place of the message) depends on the same channel. Either writePart of #153and keep the issue open until a release draft includes the note, or open a small tracking issue for the next strands_agents release notes and link it here. - Related PRs: #184 conflicts textually with this branch in
tests/common.py(both add imports and a helper at the end of the file; trivial to resolve); #184'sMCPToolNameConflictErrorin_TERMINAL_EXCEPTIONSis inherited by the encoded variant with no change. #190 and #192 merge cleanly with this branch.
| ``` | ||
|
|
||
| This relies on the plugin's failure converter, which is installed via the client's data converter. **Attach `StrandsPlugin` to the client** (not just the worker) for activity-tool interrupts to work — workers built from that client pick up the plugin automatically. | ||
| This relies on the plugin's failure converter, which is installed via the client's data converter. **Attach `StrandsPlugin` to the client** (not just the worker) for activity-tool interrupts to work — workers built from that client pick up the plugin automatically. If your data converter uses the default payload converter with `DefaultFailureConverterWithEncodedAttributes` or a subclass of it, the plugin's failure converter also encodes failure messages and stack traces with your codec. |
There was a problem hiding this comment.
P3 (non-blocking): the new sentence names subclasses, but the plugin keeps only the encoding, not the subclass itself. strands_failure_converter_class swaps any DefaultFailureConverterWithEncodedAttributes subclass for StrandsFailureConverterWithEncodedAttributes, so the subclass's own to_failure and from_failure never run. Probe at the head: class Sub(DefaultFailureConverterWithEncodedAttributes) whose to_failure calls super() and then sets failure.source = 'redacted-by-caller'; after StrandsPlugin(models={}).configure_client(...) the active converter is StrandsFailureConverterWithEncodedAttributes, the message is Encoded failure, and source is ''. The replacement is deliberate (dropping from_failure protects replay) and already applied to every other class, but nothing user-facing says so, and someone who subclassed to add behaviour will read this sentence as their class being kept. Suggested wording: "If your data converter uses the default payload converter with DefaultFailureConverterWithEncodedAttributes or a subclass of it, the plugin replaces it with its own failure converter but keeps the encoding, so failure messages and stack traces still go through your codec. Methods your class overrides are not used." The docstring at _failure_converter.py:99 could say the same for to_failure; today it mentions only from_failure.
There was a problem hiding this comment.
Applied your wording in 4e73b0a. The docstring now also says the caller's own to_failure and from_failure never run.
The README and the docstring now say that only the encoding is kept: a subclass's own to_failure and from_failure never run.
|
Thanks. On the PR-body item: the description now says |
What was changed
StrandsPluginkeeps the encoding of a caller'sDefaultFailureConverterWithEncodedAttributes, or of a subclass of it. It installs a privateStrandsFailureConverterWithEncodedAttributes, which subclasses bothStrandsFailureConverterandDefaultFailureConverterWithEncodedAttributes. Every other failure converter class still becomesStrandsFailureConverter, as before.Why?
Since 0.1.0 the plugin keeps the caller's codec and external storage, but it still replaces the failure converter class. With
DefaultFailureConverterWithEncodedAttributes, failure messages and stack traces were stored as written in History, outside the codec. They can hold tool arguments, model output and provider errors.The plugin still never keeps a caller's own
from_failure: a class with its ownfrom_failurechanges the errors Workflow code reads, so a Workflow that branches on the error type can take another branch and fail replay with[TMPRL1100]. Both classes the plugin can install decode withDefaultFailureConverter.from_failure, which reads encoded and plain attributes, so Workflow code reads the same errors and no command changes.For callers with
DefaultFailureConverterWithEncodedAttributes(or a subclass), raw History changes: failure messages now readEncoded failureand stack traces move into the encoded attributes, as with that converter and no plugin. Tools that read History without the codec, such astemporal workflow show, the UI without a codec server, or alerts onfailure.message, see that text.Checklist
Part of [Bug] strands_agents: StrandsPlugin replaces the caller's failure converter, and the codec upgrade needs a note #153 (it stays open for the codec upgrade note in the next release notes)
How was this tested:
tests/test_plugin.py, each for the Client and the Replayer.test_caller_encoded_failure_converter_is_keptruns withDefaultFailureConverterWithEncodedAttributesand with a subclass of it. It fails onmain(assert 'secret' == 'Encoded failure') and passes with this change; it also checks that anInterruptExceptionstill becomes a non-retryableStrandsInterruptfailure.test_other_failure_converter_is_replacedchecks that a class with its ownfrom_failureis still replaced; it passes on both.test_activity_tool_interruptalso runs with a codec andDefaultFailureConverterWithEncodedAttributes: the interrupt still reaches the Workflow, the Activity failure in History readsEncoded failure, and the History replays with the same converter.stored message: 'Encoded failure'and exits 0 with this change.DefaultFailureConverterWithEncodedAttributes: histories frommainand from this change (finished, waiting for an Activity-tool interrupt approval, and failed) replay on both. A run left waiting onmainand approved on this change completes, and so does one moved the other way; the Workers had no Workflow cache. A renamed tool Activity fails replay with[TMPRL1100](control). With a caller converter that has its ownfrom_failure, histories frommainand this change replay on each other.make test(59 tests) andmake lintpass locally on Python 3.13. CI runs the full matrix.Any docs updates needed?
One README sentence. The codec change that shipped in 0.1.0 needs a release note; a draft is in the README review thread.
Not in this PR: a caller payload converter that is not the SDK default is still returned unchanged, so it gets no Strands failure handling (#299).