Skip to content

fix(strands): keep the caller's failure converter and turn Strands errors into Temporal failures (breaking) - #200

Merged
campoy merged 19 commits into
mainfrom
fix/strands-typescript-plugin-replaces-caller-failure-converter
Oct 30, 2026
Merged

campoy merged 19 commits into
mainfrom
fix/strands-typescript-plugin-replaces-caller-failure-converter

Conversation

@campoy

@campoy campoy commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

What was changed

  • StrandsPlugin no longer sets dataConverter.failureConverterPath. A Client or Worker keeps the failure converter it sets, or the SDK default.
  • An Activity inbound interceptor rethrows Strands interrupt and terminal errors (ContextWindowOverflowError, MaxTokensError, StructuredOutputError) as typed, non-retryable ApplicationFailures that keep the error's stack and cause. The Worker turns any other error an Activity throws into a retryable ApplicationFailure before a failure converter runs, so the plugin's converter never saw these errors from Activities. Interrupts are BENIGN, and so is a context overflow on the plugin's model Activities, so the Worker neither logs them at WARN nor counts them in its Activity failure metric. workerInterceptors is a function, because the object form drops the Worker's deprecated activityInbound interceptors.
  • An Activity interrupt is any error with a non-empty interrupts array. Each entry needs a toJSON() that returns an object with a unique string name, and the details must be JSON-encodable; a malformed interrupt fails without retries as type StrandsMalformedInterrupt (exported) and keeps the Activity's message. TemporalActivityTool raises every entry in order, so the agent pauses at each.
  • TemporalModel.stream rethrows a model Activity failure of type ContextWindowOverflowError as Strands' own error, with the Activity failure as its cause, so the conversation manager trims and calls the model again. It also reads the type from the failure's proto, so a converter that returns another TemporalFailure still works.
  • TemporalAgent.stream(), which invoke() uses, ends a turn with a Temporal failure where Strands would end it with an error that only fails the Workflow Task. A terminal error becomes a typed, non-retryable ApplicationFailure with the original error as its cause. A ModelError that wraps a Temporal failure, from a model Activity that failed, timed out or was cancelled, becomes that failure. So the run, or the Update that runs the turn, fails or is cancelled.
  • Removes StrandsFailureConverter, its failureConverter instance and the index export; failure-converter.ts becomes strands-errors.ts. STRANDS_INTERRUPT_TYPE is still exported.
  • README: Retries says where each terminal error comes from and what happens to it, what a failed or cancelled model call does, what changes for code that catches these errors and for interceptors, and for workflowFailureErrorTypes; Human-in-the-loop gives the interrupt shape, several entries, the malformed type, benign metrics, and that every Worker running an activityAsTool Activity needs the plugin; Streaming, Observability and "Pre-built workflow bundles" notes; one "Upgrading from 1.24 and earlier" section.
  • Tests, all in test-strands.ts, listed below.

Earlier revisions composed the two converters through a generated module (review showed the converter never saw Activity errors), and then used a Workflow interceptor that wrapped Signal and Update handlers (review showed it changed when handlers finish, which broke replay of some older runs). This revision catches the errors in TemporalAgent.stream() instead. The context overflow decode and the StrandsFailureConverter removal were reviewed as #279 and #281.

Why?

StrandsPlugin set failureConverterPath to its own module, which replaced the caller's converter (#199). A converter that encodes failure messages and stack traces (encodeCommonAttributes), as the Temporal docs recommend for sensitive error text, was dropped without a warning. Activity, Workflow and Workflow Task failures then went to History in plain text.

The plugin's converter also missed its main job: an Activity that threw an interrupt-shaped error was retried as an ordinary failure, so the agent never paused. The existing interrupt test passed through that retry. A context overflow thrown by the model Activity was retried as well, and then reached the agent as a plain ModelError, so the conversation manager never trimmed.

A model Activity that failed for good, timed out or was cancelled also reached Workflow code as a plain ModelError, which the SDK does not recognize, so the Workflow Task failed on every attempt: a failed model call left the run open, and a cancel was ignored.

Breaking change

  • Workflow and Client code decode failures with the caller's converter instead of the plugin's. When the caller's converter decodes a failure into another error than the SDK's default converter, a run started before this version also gets that error on replay. If that changes which commands the Workflow issues, the run fails with [TMPRL1100]. If it only changes a value the Workflow keeps or returns, the run replays without error and continues with the new value, so replay tests do not catch it. That covers failures the Workflow reads from Activities and child Workflows, and a retried run's workflowInfo().lastFailure. Clients and Workers without a failure converter of their own decode as before.
  • Strands interrupt and terminal errors thrown by an Activity are no longer retried. An interrupt-shaped error now pauses the agent, and a malformed one fails without retries.
  • A turn that ends in a Strands terminal error now makes the run, or the Update, fail with a non-retryable failure of its type; a turn whose model Activity failed, timed out or was cancelled makes the run fail or be cancelled. Before, both failed the Workflow Task on every attempt. Workflow code that catches errors from invoke() by class now sees an ApplicationFailure or an ActivityFailure; if it acts on that, a run started before this version can fail to replay. A Worker that lists one of these errors in workflowFailureErrorTypes no longer gets a retry run, since the failure is non-retryable.
  • A run whose Workflow Task kept failing that way fails or is cancelled once this version runs, and one stuck on a context overflow trims and continues, but only if it started on a build that already reads the model config. A run started on 1.24 and stuck this way fails to replay at the config read instead and must be reset or terminated.
  • A run whose Workflow code caught a context overflow before this version calls the model again on replay instead, and fails with [TMPRL1100].
  • Activity interceptors of plugins listed after StrandsPlugin see the original Strands error; your own and those listed before it see the ApplicationFailure.
  • StrandsFailureConverter is removed. Code that imports it, or sets failureConverterPath to @temporalio/strands-agents/lib/failure-converter, no longer builds or starts; remove the import or the path. It decoded like the SDK's default converter, so histories written with it replay.
  • The plugin no longer rewrites a Strands error that is the cause of another error, or an interrupt-shaped error that escapes Workflow code. The latter fails the Workflow Task, and that failure now records the error as the SDK's default converter does, without the StrandsInterrupt type. Workflow Tasks retry either way.

Upgrade: if you set a failure converter whose decoding differs from the SDK default, your Workflow code catches errors from invoke() by class, or it catches a context overflow from the agent, let runs that started on the earlier version finish before you upgrade, or keep them there with Worker Versioning. The same applies to rolling back. Rebuild pre-built Workflow bundles with this version: the plugin's Workflow code is in the bundle. The README's "Upgrading from 1.24 and earlier" section says all of this.

The PR title says it is breaking because release notes come from the squash commit subject.

Checklist

  1. Closes [Bug] strands-agents (TypeScript): StrandsPlugin replaces the caller's failure converter #199

  2. How was this tested:

    • activity-tool interrupt pauses the agent and resumes (existing test, extended): the first deleteThing Activity fails with a non-retryable StrandsInterrupt on its first attempt, the failure is BENIGN and keeps the thrown error's stack, and the resumed agent schedules a second Activity. On main the Activity is retried and History has no failed Activity.
    • an activity interrupt with two entries pauses the agent once for each: an Activity that always throws two entries; the agent pauses at the first, then at the second, and finishes on the third call with both responses. With 072b20e only the first entry was ever raised.
    • a malformed activity interrupt fails without retries: an entry without toJSON(), a plain { name, reason } entry, two entries with the same name, a BigInt reason, and arrays with holes each fail with type StrandsMalformedInterrupt, non-retryable, keeping the Activity's message. With 072b20e the plain entry is retried as Error; with c083568 the first is a retryable TypeError and the third stops the Worker. Without this commit's name check the repeated name is a StrandsInterrupt, and with map in place of Array.from, as on db052d6, the holes stop the Worker.
    • a worker's own failure converter still applies with the plugin: a Worker that bundles the Workflows itself, with DefaultFailureConverter({ encodeCommonAttributes: true }) and the plugin, runs a Workflow whose Activity throws an interrupt-shaped error and then a StructuredOutputError. Both Activity failures and the Workflow failure are Encoded failure in History, the Activity failures are non-retryable and typed, Workflow code decodes the interrupt as TemporalActivityTool expects, and the Worker's own activityInbound interceptor runs for both Activities. It fails with main's plugin code.
    • a context overflow in the model activity lets the agent trim the conversation (the model Activity's failure is BENIGN; it runs a second time with a converter that decodes application failures to another TemporalFailure), and a context overflow the agent cannot trim fails the run as non-retryable (the client's cause chain holds the model Activity's ActivityFailure).
    • a Strands terminal error thrown in workflow code fails the run as non-retryable (a model that stops at its token limit), and a Strands terminal error in an update handler fails the update (the Update fails with MaxTokensError and the run completes).
    • a cancel during a model activity cancels the run and a model activity that fails for good fails the run (with its ActivityFailure). With 072b20e both leave the Workflow Task failing on every attempt. A one-off check also cancels a run during an activityAsTool tool Activity: it ends with CancelledFailure.
    • a history whose code depends on when a handler finishes still replays: a handlerTiming history recorded with main's plugin at 63d7aec replays on this revision. With 072b20e's handler wrapper it failed with [TMPRL1100].
    • The agent histories from the overflow replay check (recorded on cb4cfa4 and on fix(strands): let the agent trim the conversation on a context overflow (breaking) #279's head) replay on this revision.
    • The reproduction from [Bug] strands-agents (TypeScript): StrandsPlugin replaces the caller's failure converter #199, on an earlier revision of this PR: the Activity, Workflow and Workflow Task failures are all Encoded failure. Its last check, that an interrupt-shaped error thrown in Workflow code keeps type StrandsInterrupt in the Workflow Task failure, now fails as described under Breaking change, so it exits 1 for that reason only.
    • Replay check against a local dev server, Workers with a Workflow cache of 0, and a caller converter that encodes failures and decodes type Custom into its own error. The Workflow schedules a different Activity depending on which converter decoded a failure. main here is 5e91c26, whose failure converter setup is the same as current main's.
      • main's finished, waiting and retried histories with a caller converter fail with [TMPRL1100] on this revision: Workflow code now decodes with the caller's converter, also the retry's lastFailure, and schedules the other Activity.
      • main's histories without a caller converter replay on this revision, and a waiting main run without one completes on it.
      • This revision's histories replay on it.
      • Rolling back to main: histories without a caller converter replay and a waiting run completes; histories with one fail with [TMPRL1100].
      • A control Workflow with one extra Activity fails with [TMPRL1100].
      • Overflow replay check with real agent Workflows on the six-message stub model, against cb4cfa4: runs without an overflow replay in both directions; a cb4cfa4 run that caught the overflow fails with [TMPRL1100] here; a cb4cfa4 run whose Workflow Task kept failing on it completes here with the trimmed conversation, and its history replays; rolling back, a trimmed history fails replay on cb4cfa4; the control fails with [TMPRL1100].
      • Rerun on cb4cfa4 with the same results, plus: the histories recorded on e942996 replay on cb4cfa4; a run whose Workflow Task kept failing on main on an error named StructuredOutputError fails on cb4cfa4 with a non-retryable StructuredOutputError, and its history replays there. On main that history fails replay, since main lets the error escape again.
    • Local, in a Linux container with Node 24 and pnpm 10.27.0, on cf3b928: make sync, make lint, make test (46 tests, parallel), make build, the npm package check, the smoke install and check_conventions.py pass, and Prettier with the SDK settings reports the files clean. Undoing any one of cf3b928's checks (the unique name, Array.from, the BENIGN category, the proto fallback) fails at least one test. The Node 22, macOS and Windows cells did not run locally.
    • Not run against a real model provider.
  3. Any docs updates needed?
    The plugin README is updated. Release notes must say that failures are encoded and decoded with the caller's converter, that Strands errors from Activities are no longer retried, that context overflows are trimmed, that failed or cancelled model calls now fail or cancel the run, that StrandsFailureConverter is removed, and which runs started before the upgrade do not replay on it. The Temporal docs page for the plugin and the TypeScript sample still name StrandsFailureConverter; they need a change once this is released.

Not covered

  • Other errors Strands raises in Workflow code, such as a ModelError for a model response it cannot parse, still fail the Workflow Task on every attempt.
  • The stream() override adds microtasks to each agent event. A review checked two turns that run at once in one Workflow and resume in the same Workflow Task: in one of about 800 combinations of event and await counts, they schedule their Activities in a different order than on main, so that history fails replay across the change, both ways. Runs started on 1.24 already fail replay at the model config read, so upgrades from 1.24 get no new break. The stream() docstring says its awaits are part of replay; the suite does not check it.
  • Inside a Strands Graph or Swarm node, Strands catches the turn's failure and marks the node FAILED, so a cancel during that node's model Activity does not cancel the run.
  • Streaming (invokeModelStreaming) goes through the same overflow catch. By source, not run.
  • A caller converter that does not decode StrandsInterrupt failures as the SDK default does loses activity-tool interrupts. The README states this; the plugin does not check it.
  • The Python plugin has the same converter problem ([Bug] strands_agents: StrandsPlugin replaces the caller's failure converter, and the codec upgrade needs a note #153), with its own fix in Keep encoded failure attributes in StrandsPlugin #155; it builds interrupts without the BENIGN category.

StrandsPlugin set `dataConverter.failureConverterPath` to its own module,
replacing a converter the client or worker already had. A converter that
encodes failure messages and stack traces (`encodeCommonAttributes`) was
dropped without a warning, so those failures went to history in plain
text.

The plugin now passes `dataConverter` as a function. A client or worker
with no failure converter of its own gets Strands' converter, as before.
With one, the plugin writes a small module to a private temporary
directory that puts Strands' rewrite of interrupt and terminal errors,
applied to the whole cause chain, in front of the caller's converter, and
points `failureConverterPath` at it, so the worker also bundles it into
workflow code. The module that writes it is kept out of the workflow
bundle.

Encoding only changes what history stores, so it always uses the caller's
converter, also in workflow code. Decoding in workflow code uses it only
in runs that record the `strands-agents.caller-failure-converter` patch,
which a workflow interceptor the plugin adds checks when a run starts.
Runs that started before keep Strands' converter for decoding, so a caller
converter that decodes failures into other errors does not change what
their workflow code sees on replay. To roll back, keep this version and
decline the patch with the worker's `patchActivationCallback`.

Breaking change: clients and activities decode with the caller's
converter as soon as this version runs, and new runs do in workflow code.
Code that matched the errors the SDK's default converter returns can get
the errors the caller's converter returns instead.
…onverter

The SDK decodes `continuedFailure` into `workflowInfo().lastFailure` while
it initializes a run, before the plugin's workflow interceptor decides
whether the run decodes with the caller's failure converter. A retried or
continued run that recorded the `strands-agents.caller-failure-converter`
patch therefore saw its last failure decoded by Strands' converter.

When the run records the patch, the interceptor now decodes that failure
again with the caller's converter, before workflow code reads it. Runs
without the patch keep the old decoding, so their histories replay.
… causes

Adding the plugin's workflow interceptor module made SimplePlugin rebuild
the worker's interceptors, and its rebuild drops the deprecated
activityInbound field, so a worker's activity guards stopped running.
The plugin now copies them back after configureWorker and
configureReplayWorker.

Only the first failure decoded before the gate is kept for the
last-failure re-decode: that is the SDK's decode of continuedFailure, and
a caller's workflow interceptor that awaits a failing activity before
next no longer replaces it.

The cause-chain rewrite defines the substitute error's own cause, so an
error with a read-only cause, such as a frozen one, no longer fails the
workflow task.
…rsion gate

Fixes add no version gates for now, so remove the
`strands-agents.caller-failure-converter` patch. Workflow code now
decodes failures with the caller's converter in every run, as Client and
Activity code already did. The SDK decodes a retried run's last failure
with it too, so the plugin's workflow interceptor module, its
re-decode of `workflowInfo().lastFailure`, and the `activityInbound`
copy that adding the module needed are gone, with their tests.

Breaking change: when a caller's converter decodes a failure into
another error than the SDK's default converter, and Workflow code acts
on the difference, a run started before this version fails to replay
on it with a nondeterminism error. Let those runs finish before you
upgrade, or use Worker Versioning. Clients and Workers without a
failure converter of their own are not affected.
@campoy
campoy force-pushed the fix/strands-typescript-plugin-replaces-caller-failure-converter branch from af0af4e to 722ac31 Compare October 29, 2026 00:40
@campoy
campoy marked this pull request as ready for review October 29, 2026 01:17
@campoy
campoy requested a review from a team as a code owner October 29, 2026 01:17

@DABH DABH left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of composing the caller's failure converter. Findings were reproduced against SDK 1.24.0 on a local dev server unless marked otherwise; the PR's own tests pass locally (39/39), and check_conventions.py is clean.

Altitude. The plugin does error transformation (Strands interrupt or terminal error to a typed non-retryable ApplicationFailure) through the single-occupancy slot failureConverterPath. That forces the generated temp module, the bundler rewrite, the cause-walk composition and the breaking decode change. The SDK's composable mechanism for this is interceptors: a workerInterceptors.activity interceptor that rethrows rewriteError(err) plus a workflowModules inbound interceptor, the pattern already used by google-adk's absorbed-failure.ts and the openai-agents and langsmith activity interceptors. An activity execute interceptor sees the raw error before ensureApplicationFailure, so it is the only mechanism I found that performs the advertised top-level rewrite (verified: the workflow sees type: 'StrandsInterrupt', nonRetryable: true), and the caller's converter is then used unchanged for encoding and decoding, with no temp dir, no __dirname guard, no Build ID drift and no TMPRL1100 rollout warning. What is lost is only the rewrite of Strands errors nested as cause under a user error, which no consumer reads: the server's retryability check and TemporalActivityTool read the top-level or direct failure only. See the comment on plugin.ts:111.

Not anchored to a diff line:

  • src/failure-converter.ts:62 (pre-existing). The doc comment's "marks deterministic Strands model errors as non-retryable" does not hold for errors raised inside the workflow sandbox: a non-TemporalFailure thrown from workflow code goes to recordWorkflowTaskError and the converter is never consulted. MaxTokensError (from Model.streamAggregated) and StructuredOutputError thrown in the sandbox produce an endless WorkflowTaskFailed loop instead of a failed run, and even with workflowFailureErrorTypes they fail with nonRetryable: false because ensureTemporalFailure wraps first. No test throws a bare Strands error from a workflow.
  • src/temporal-activity-tool.ts:88. With decoding now delegated entirely to the caller, the interrupt check (err instanceof ActivityFailure && err.cause instanceof ApplicationFailure && err.cause.type === STRANDS_INTERRUPT_TYPE) silently depends on the caller's failureToError. A caller converter that maps application failures to domain classes, relabels type, wraps every failure or unwraps ActivityFailure makes it false, the agent continues, and the human-in-the-loop interrupt is lost with no error. No test runs an interrupt with a caller converter. Document the precondition and add that test, or decode StrandsInterrupt chains with the SDK default.
  • README line 229 (pre-existing). "Attach StrandsPlugin to the client ... workers built from that client pick up the plugin" is wrong: plugins propagate from the NativeConnection, not a Client (the interrupt test passes with a plugin-less env client because interrupts are detected from the activity's encoded failure).

* workflow code like any other converter module.
*/
export function failureConverterPathFor(callerPath: string | undefined): string {
if (callerPath === undefined || callerPath === STRANDS_FAILURE_CONVERTER) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SDK's loadDataConverter ignores any falsy failureConverterPath, but only undefined is treated as "no converter" here, so an empty string falls through to require.resolve(''). dataConverter: { failureConverterPath: process.env.FAILURE_CONVERTER_PATH ?? '' } worked before (the plugin's static object overwrote it); now configureWorker throws MODULE_NOT_FOUND - Cannot find module '' and Worker.create/new Client reject. Use if (!callerPath || ...).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer applies: the plugin doesn't touch failureConverterPath anymore, so an empty value reaches the SDK as before.

}
// Resolve the caller's path as the SDK does when it loads it.
const resolved = require.resolve(callerPath, { paths: [path.dirname(require.resolve('@temporalio/common'))] });
let composedPath = composedPaths.get(resolved);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cached path is returned for the process lifetime without checking it still exists, and composedDir is never recreated. After a tmp cleaner (systemd-tmpfiles, macOS periodic purge) removes the directory, failureConverterPathFor(sameCaller) returns the deleted path and loadDataConverter throws Could not find a file at the specified failureConverterPath; a new caller path would writeFileSync into the missing dir (ENOENT). The mechanism is confirmed, the trigger needs a long-lived process that creates another Worker or Client days later. Fix: mkdirSync(composedDir, { recursive: true }) and re-write when !existsSync(composedPath).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gone with the generated module in e942996.

let composedPath = composedPaths.get(resolved);
if (composedPath === undefined) {
if (composedDir === undefined) {
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'temporal-strands-agents-'));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Random per-process directory changes the default Build ID on every worker start. The path lands in the workflow bundle (development-mode module ids and the inline source map), so the SDK's auto-derived Build ID @temporalio/worker@<ver>+sha256(bundle) changes on every start whenever a caller converter is set. Verified: bundling the same workflows with failureConverterPathFor(caller) in two processes embeds ...temporal-strands-agents-smBGwk/... vs ...-RVR40G/... and gives different hashes (identical hashes without a caller converter). Every restart or replica of identical code then reports a new default build id, and the README's own "use Worker Versioning" advice only works with an explicit buildId. A content-addressed location (for example os.tmpdir()/temporal-strands-agents/<hash>.cjs, written atomically) restores the invariant.

Also, a read-only or missing os.tmpdir() makes mkdtempSync throw ENOENT/EACCES from the plugin's configureClient/configureWorker hook, with no mention of why a temp file is needed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gone with the generated module in e942996: the bundle no longer contains a temporary path, and the plugin no longer needs a writable os.tmpdir().

if (composedPath === undefined) {
if (composedDir === undefined) {
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'temporal-strands-agents-'));
process.once('exit', () => fs.rmSync(dir, { recursive: true, force: true }));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

process.once('exit', () => fs.rmSync(...)) can throw inside the exit listener (force masks only ENOENT): an ENOTEMPTY/EBUSY at natural exit prints an uncaught exception and flips the exit code from 0 to 1 (ava runs each test file in a child process; a Windows-CI flake vector when antivirus holds the file). It also never runs on SIGTERM/SIGINT without a handler or on SIGKILL, leaking one 0700 dir per start. Wrap the cleanup in try/catch (with maxRetries).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gone with the generated module in e942996.

"'use strict';",
"// Generated by @temporalio/strands-agents: Strands' failure converter in front of the caller's.",
`const { composeFailureConverter } = require(${JSON.stringify(STRANDS_FAILURE_CONVERTER)});`,
`exports.failureConverter = composeFailureConverter(require(${JSON.stringify(resolved)}).failureConverter);`,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The caller's export is never validated. The SDK's requireConverter now validates only this always-well-formed wrapper, so a caller module that exports failureConverters (typo) is no longer rejected at startup (the SDK alone throws Module ... does not have a failureConverter named export) and instead fails at the first conversion with TypeError: Cannot read properties of undefined (reading 'errorToFailure'), as a workflow-task failure loop in the sandbox or on handle.result(). A MODULE_NOT_FOUND raised inside the caller module is reported against the random temp file (failure-converter-<hash>.js), and a bad caller path throws a raw Cannot find module from require.resolve at line 26 instead of the SDK's ValueError. Require and shape-check the caller module in failureConverterPathFor.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SDK loads the caller's module directly again (e942996), so its own check and error messages apply.

Comment thread typescript/strands-agents/src/plugin.ts Outdated
},
// Keep a failure converter the client or worker already has: Strands' rewrite runs first,
// then the caller's converter.
dataConverter: (existing) => ({

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the interceptor alternative in the review summary: using failureConverterPath for error transformation is what forces everything else in this PR (the temp module, the bundler rewrite below, the cause-walk, the breaking decode change). Activity and workflow interceptors compose per plugin, are carried into prebuilt bundles by configureBundler, and leave the caller's converter untouched.

Separately, the README says "replaces each cause ... that comes from a Strands error", but only the first Strands cause along a chain is replaced and everything beneath it is dropped (outer -> StructuredOutputError('a') -> StructuredOutputError('b') encodes as [outer, a]), and "writes a small module to a private temporary directory" omits that a writable os.tmpdir() is now required at Client/Worker construction and that the path changes the default Build ID.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I switched to this in e942996. The plugin no longer sets failureConverterPath, and an Activity inbound interceptor rethrows rewriteError(err), so the caller's converter is used as it is. A few notes:

  • I didn't add a Workflow interceptor. It would turn a Strands error thrown in Workflow code from a Workflow Task failure into a Workflow failure. That is the separate issue you raised on failure-converter.ts:62.
  • The upgrade note stays. Workflow code used to decode with the plugin's converter and now decodes with the caller's, so a caller converter that decodes some failures differently still breaks replay of older runs. The replay check in the PR body shows it.
  • SimplePlugin drops activityInbound when a plugin adds Activity interceptors, so configureWorker puts them back.

The README text you flagged is rewritten.

Comment thread typescript/strands-agents/src/plugin.ts Outdated
* transports into the client entrypoint, so their static EventSource
* imports must also be stubbed. Transport I/O only runs worker-side.
*
* - Replace the plugin's own `failure-converter-path` module, which writes files

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prebuilt bundles do not get the composed converter. configureBundler never composes failureConverterPath (SimplePlugin.configureBundler does not propagate dataConverter), the Worker ignores dataConverter.failureConverterPath when workflowBundle is set, and neither failureConverterPathFor nor composeFailureConverter is exported. With the same caller converter on client, worker and bundleWorkflowCode({ plugins: [new StrandsPlugin()], failureConverterPath }), the sandbox encodes with the caller's converter alone while activities and the client use the composed one: failTwice('workflow failed', ['direct']) records the cause as { message: 'Encoded failure' } with no applicationFailureInfo, versus StructuredOutputError/nonRetryable on the worker-bundled path, with no warning. Pre-existing gap, but the README (Client, activity and workflow code also decode with it) and the doc comment ("The worker bundles it into workflow code like any other converter module") now promise otherwise. Verified one-line fix: have configureBundler return failureConverterPath: failureConverterPathFor(base.failureConverterPath) (idempotent on the worker-bundled path).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer applies: the plugin leaves failureConverterPath to the caller, so a prebuilt bundle and the Worker use the same converter. The rewrite happens in the Activity interceptor, which doesn't depend on the bundle.

Comment thread typescript/strands-agents/src/plugin.ts Outdated
resource.request = empty;
}).apply(compiler);
new NormalModuleReplacementPlugin(/^\.\/failure-converter-path$/, (resource) => {
if (resource.context === __dirname) resource.request = empty;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The __dirname gate fails when workflow code resolves a different physical copy of this plugin. If the workflows package and the worker package resolve different installs of @temporalio/strands-agents (a monorepo, or --preserve-symlinks), resource.context === __dirname is false, the real failure-converter-path.js is bundled, and its import * as os from 'os' trips the SDK's disallowed-builtin check (os is not in ignoreModules, unlike fs/path/crypto): importing the following disallowed modules: - 'os', failing Worker.create (reproduced). Verified alternative: add 'os' to ignoreModules and drop this second NormalModuleReplacementPlugin (and the new context field); the module then bundles as an inert stub, and both the PR's integration test and the two-copies scenario pass. Also mark the dynamic require.resolve(callerPath, { paths }) with /* webpackIgnore: true */ (or an ignoreWarnings entry) to avoid a webpack "Critical dependency" warning on every bundle.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed with the generated module in e942996, so there's nothing to stub out anymore.

Comment thread typescript/strands-agents/README.md Outdated
const client = new Client({ /* ... */, plugins: [new StrandsPlugin({ models })] });
```

If the client or worker already sets `dataConverter.failureConverterPath`, for example to [encode failure messages](https://docs.temporal.io/failure-converter), the plugin keeps it: Strands' rewrite runs first, then your converter. Your converter is called as it is, and Strands then replaces each `cause` in the failure it returns that comes from a Strands error with your converter's encoding of the rewritten error. Strands only does this where the failure's `cause` is the same as your converter's own encoding of the error's `cause`, as with the SDK's default converter. Where your converter adds, drops or changes failure levels, Strands keeps that part of the failure as your converter returned it, and the nested causes there are not rewritten. To load the two together, the plugin writes a small module to a private temporary directory. Failures are encoded with your converter as soon as you deploy this version. Client, activity and workflow code also decode with it, so code that matched the errors the SDK's default converter returns may now get the errors your converter returns. If that changes what a workflow does, a run that started before you deploy this version fails to replay on it with a nondeterminism error: let those runs finish first, or use Worker Versioning.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the review summary and the inline comments for the accuracy problems in this paragraph: only the first Strands cause along a chain is replaced and everything beneath it is dropped, "Client, activity and workflow code also decode with it" is unqualified for prebuilt bundles, and the temp-module sentence hides that a writable os.tmpdir() is required and that the default Build ID now changes per start. Also, if a caller's failureToError does not preserve the StrandsInterrupt application failure, interrupts are lost silently (see the review summary), which belongs in this paragraph.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rewritten in e942996. It says the plugin keeps your converter and that your converter must decode StrandsInterrupt as the SDK default does for activity-tool interrupts to work, and it keeps the upgrade note. The temp module and nested-cause text are gone.

args: ['frozen failure'],
workflowExecutionTimeout: '30 seconds',
});
await t.throwsAsync(frozen.result());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

await t.throwsAsync(frozen.result()); accepts any error, including the 30 s workflowExecutionTimeout that the frozen-cause regression (commit 6ba6127) would produce: the run loops on workflow-task failures until the timeout, throwsAsync passes, and the failure is caught only 30 s later by the history assertion as [undefined, undefined, undefined]. Also, frozen and retried come from helpers(t)'s plugin-less, default-converter env.client, while a second hand-built Client with the caller converter exists only for one result() call, so any future assertion on their decoded errors would silently use the SDK default converter. Suggest configuring the env once with createLocal({ client: { dataConverter: { failureConverterPath }, plugins: [new StrandsPlugin()] } }) inside the startLocalWithRetry callback, asserting instanceOf: WorkflowFailedError, message: 'Workflow execution failed' (or no workflowTaskFailedEventAttributes in the history), and sharing one chain() helper and one frozen delegating wrapper (delegatingFrozen duplicates encoding-failure-converter.ts:34-39).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworked in e942996: the test env's Client now uses the caller's converter (createLocal({ client: { dataConverter } })), throwsAsync checks for WorkflowFailedError, and the frozen, retried and delegating cases are gone with the composed converter.

…king)

Review found that the plugin's failure converter never saw a Strands
error thrown by an activity: the worker turns it into a retryable
ApplicationFailure before any failure converter runs. An
interrupt-shaped error was retried instead of pausing the agent.
Composing the converter with the caller's also needed a generated module
in a temporary directory, which brought its own problems.

The plugin now rethrows Strands' interrupt and terminal errors from an
activity inbound interceptor as typed, non-retryable failures, and no
longer sets failureConverterPath. A client or worker keeps its own
failure converter, or the SDK default. SimplePlugin drops the worker's
activityInbound interceptors when it adds the plugin's, so
configureWorker puts them back. The generated module,
composeFailureConverter and their tests are removed.

Breaking change: workflow and client code decode failures with the
caller's converter instead of the plugin's. A run started before this
version fails to replay when its workflow acts on a failure that the
caller's converter decodes differently. Strands errors thrown by
activities are no longer retried, and Strands errors nested as the
cause of another error are no longer rewritten.
@campoy campoy changed the title fix(strands): keep the caller's failure converter (breaking) fix(strands): keep the caller's failure converter and stop retrying activity interrupts (breaking) Oct 29, 2026
@campoy

campoy commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor Author

@DABH thanks for the thorough review. I switched to the interceptor design in e942996, and replied on each thread. On the three points outside the diff:

  • failure-converter.ts:62: agreed, a Strands error thrown in Workflow code fails the Workflow Task, not the Workflow. The old converter only changed the type recorded on that Workflow Task failure, and with this change it no longer does that either (noted in the PR body). It predates this PR, and fixing it changes behavior, so I'd keep it for a separate PR. The StrandsFailureConverter doc comment now says only what the converter does, and that the plugin no longer installs it.
  • temporal-activity-tool.ts:88: the README now says that for activity-tool interrupts, your converter must decode StrandsInterrupt failures as the SDK default does. test-failure-converter.ts runs an interrupt-shaped Activity error through a caller's converter and checks how Workflow code decodes it.
  • README line 229: that paragraph and its code sample are gone. Interrupts now need the plugin on the Worker only, and the class doc no longer says to add it to the Client.

…re converter

A run started before the upgrade decodes failures with the caller's
converter on replay too. It fails replay only when that changes the
Workflow's commands. When it only changes a value the Workflow keeps or
returns, the run replays without error and continues with the new value,
so replay tests do not catch it.

@brianstrauch brianstrauch left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed ebcc4c1. make lint and make test pass locally on Node 24 (37 tests). One diagnostics regression below.

Comment thread typescript/strands-agents/src/plugin.ts Outdated
try {
return await next(input);
} catch (err) {
throw rewriteError(err);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Preserve the original Activity error stack and cause

Calling rewriteError here replaces every recognized Strands error with a freshly constructed ApplicationFailure, without copying the original stack or cause. Before this change, the SDK's ensureApplicationFailure preserved those fields for an Activity's thrown error. Now both Worker logs and History point only to ApplicationFailure.create / rewriteError / this interceptor, losing the provider or tool throw site and any underlying cause.

Reproduced with StructuredOutputError, ContextWindowOverflowError, and MaxTokensError: an original stack containing /app/provider.ts:123:7 survives the previous conversion path but disappears through this interceptor, with no cause linking back to it. Preserve the original stack and cause when constructing the replacement failure.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed in cb4cfa4. rewriteError now uses ApplicationFailure.fromError, which keeps the thrown error's stack and cause, for the terminal errors and the interrupt. The interrupt test now checks that the stored failure's stack comes from the thrown error; it fails with the previous head.

@DABH DABH left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second pass, on the interceptor-based head. Moving the rewrite into an activity interceptor and leaving the caller's converter untouched resolves the whole previous round (the temp module, the bundler gate, the Build ID drift, the prebuilt-bundle gap and the quadratic re-encoding are gone, and the interrupt test now proves the activity failed once without a retry). Lint, the 37 tests and the conventions check pass. Findings below were verified in-process against SDK 1.24.0 or by reading the SDK and Strands sources as cited.

Severity: P1 = wrong result or stuck run in realistic use; P2 = correctness or robustness in an edge case; P3 = docs, tests, cleanup.

Two altitude points. (1) Of the three terminal types the interceptor rewrites, only ContextWindowOverflowError can originate inside the plugin's activities: MaxTokensError is thrown by Model.streamAggregated in workflow code (the activity returns the maxTokens stop event) and StructuredOutputError by the agent loop, also workflow-side, where a plain error still fails the workflow task forever. The rewrite belongs where the errors are thrown: a workflowModules inbound interceptor applying the same rewriteError, or merging workflowFailureErrorTypes for those names as the google-adk plugin documents. (2) Nothing decodes the typed ContextWindowOverflowError failure back into a Strands error on the workflow side (temporal-model.ts:107 rethrows the ActivityFailure), so the non-retryable type the interceptor now emits is a dead contract: Strands' overflow hooks test event.error instanceof ContextWindowOverflowError, reduce() never runs, and a SlidingWindowConversationManager user whose context overflows gets a failed workflow instead of a trimmed conversation. Only STRANDS_INTERRUPT_TYPE is consumed anywhere in src/. The decode in TemporalModel.stream is the missing half of this change.

Comment thread typescript/strands-agents/src/plugin.ts Outdated
// `SimplePlugin` rebuilds the worker's `interceptors` when it adds the plugin's activity
// interceptor, which drops the caller's `activityInbound` interceptors. Put them back.
const configured = super.configureWorker(options);
const activityInbound = options.interceptors?.activityInbound;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: the override restores activityInbound only when StrandsPlugin is the last plugin that sets workerInterceptors, and it is unneeded. Worker.create applies plugins in order and every SimplePlugin whose workerInterceptors is an object re-runs resolveWorkerInterceptors, which returns a fresh {client, activity, nexus, workflowModules} with no activityInbound key. So with the README's own order, plugins: [otel, new StrandsPlugin({ models })], the OpenTelemetry plugin drops the caller's activityInbound first and this override sees undefined; with [new StrandsPlugin(), new OpenAIAgentsPlugin()] Strands restores it and the next plugin drops it again; [LangSmithPlugin, StrandsPlugin, OpenAIAgentsPlugin] loses LangSmith's activity tracing silently. configureReplayWorker performs the same merge and is not overridden. The SDK-sanctioned form needs no override: workerInterceptors: (existing) => ({ ...existing, activity: [...(existing?.activity ?? []), () => ({ inbound: strandsActivityInbound })] }) keeps every caller key (including the deprecated one) on configureWorker, configureReplayWorker and configureBundler alike (verified in-process: caller activityInbound preserved by identity, order [callerActivity, plugin], workflowInterceptorModules unchanged; safe here because the plugin registers no workflowModules). The OTel-first case cannot be fixed from this plugin, so the README should tell callers to use interceptors.activity; and either way there is currently no test that passes interceptors to a worker with the plugin, so the restore branch never executes in CI.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, switched to the function form in cb4cfa4 and removed the override, so it now applies on configureReplayWorker and configureBundler too. Since the plugin now adds a Workflow interceptor module, that module goes in through configureBundler, which the Worker also calls when it bundles Workflow code itself. That keeps the function's result free of workflowModules, so there's no "ignoring workflowModules" warning with a prebuilt bundle.

On the README: a plugin that passes workerInterceptors as an object drops activityInbound through SimplePlugin's merge, whatever this plugin does. That affects any Worker with such a plugin, so I'd rather not document it here. Happy to add a line if you think it belongs.

* Rewrite a Strands interrupt or terminal model error as a typed, non-retryable
* {@link ApplicationFailure}. Any other value is returned as it is.
*/
export function rewriteError(err: unknown): unknown {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: now that rewriteError runs before ensureApplicationFailure, the terminal-error branch (line 51) discards the original stack trace, cause and MaxTokensError.partialMessage. On the base the raw ContextWindowOverflowError reached Activity.run untouched and ensureApplicationFailure copied error.stack and error.cause into the recorded failure. Now ApplicationFailure.create({ message, type, nonRetryable }) is a fresh error whose stack starts at rewriteError/strandsActivityInbound.execute and whose cause is undefined (verified in-process), so ActivityTaskFailed.stackTrace and the worker's 'Activity failed' log point at the interceptor instead of the provider's throw site. ApplicationFailure.fromError(err, { type: name, nonRetryable: true }) is documented for exactly this and keeps message, stack and cause with the same type/nonRetryable result.

Three smaller points in the same function: the interrupt branch (line 41) builds an ApplicationFailure with no category, so every human-in-the-loop pause is logged by the worker as a WARN 'Activity failed' and counted as an activity failure; category: ApplicationFailureCategory.BENIGN routes it to debug with no change to retry semantics or to what TemporalActivityTool reads. The terminal types are matched with instanceof against the plugin's own @strands-agents/sdk copy while interrupts are matched structurally, so with two installed copies (--legacy-peer-deps, a nested duplicate) the error stays retryable; Strands sets this.name to the same strings, so a name-based fallback is copy-proof. And names serializes every interrupt a second time (details.map((d) => d.name) reads the same source).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in cb4cfa4:

  • Both branches use ApplicationFailure.fromError, so the failure keeps the stack and cause.
  • Interrupts get ApplicationFailureCategory.BENIGN.
  • Terminal errors also match on name, next to instanceof.
  • The interrupt names come from details.

MaxTokensError.partialMessage is still dropped: fromError doesn't carry custom fields.

Comment thread typescript/strands-agents/README.md Outdated
Strands supports interrupts via `toolContext.interrupt(...)` and `event.interrupt(...)`. Both work with the plugin: `agent.invoke()` returns `AgentResult` with `stopReason: 'interrupt'` and `interrupts: [...]`. Pair this with a signal handler that supplies responses, then resume by calling `agent.invoke(responses)`.

Interrupts also survive the activity boundary when raised from an `activityAsTool`-wrapped activity. The plugin's failure converter packages the interrupt as a typed `ApplicationFailure`, and `TemporalActivityTool` re-raises it through the agent's interrupt machinery on the workflow side, so `AgentResult.interrupts` is populated just like the in-workflow case.
Interrupts also survive the activity boundary when raised from an `activityAsTool`-wrapped activity. An activity interceptor the plugin adds to the worker packages the interrupt as a typed, non-retryable `ApplicationFailure`, and `TemporalActivityTool` re-raises it through the agent's interrupt machinery on the workflow side, so `AgentResult.interrupts` is populated just like the in-workflow case. The same interceptor makes Strands' `ContextWindowOverflowError`, `MaxTokensError` and `StructuredOutputError` non-retryable when an activity throws them.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: this sentence lists three errors the interceptor makes non-retryable 'when an activity throws them', but two of them are never thrown by the plugin's activities. MaxTokensError is raised by Model.streamAggregated, which the agent calls in workflow code on TemporalModel (ModelActivity.invokeModel calls the raw provider stream(), so the activity returns the maxTokens stop event); StructuredOutputError is raised by the agent loop, also workflow-side. Both escape agent.invoke() as plain error subclasses and, uncaught, fail the workflow task and retry, which is what a reader of this sentence would expect the plugin to prevent. Only ContextWindowOverflowError (provider stream() in bedrock/anthropic/google/openai) originates inside invokeModel. Name that one as the plugin-activity case and qualify the other two as 'if your own activity throws them', or add the workflow-side twin described in the summary.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the Workflow-side twin in cb4cfa4. A Workflow interceptor module rethrows the three terminal errors as non-retryable failures from execute and from Signal and Update handlers. A new test has the stub model stop at maxTokens, and the run now fails with a non-retryable MaxTokensError instead of failing the Workflow Task until it times out. The paragraph moved to Retries and says where each error is thrown.

* Failure converter that preserves Strands `InterruptError` payloads across the
* activity boundary as typed, non-retryable {@link ApplicationFailure}s and
* marks deterministic Strands model errors as non-retryable.
* Failure converter that encodes Strands `InterruptError`s as typed,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: StrandsFailureConverter and the failureConverter export are now dead public API with a docstring that overpromises. Nothing loads this module through failureConverterPath any more (the ts-prune-ignore-next export existed only for the deleted require.resolve('./failure-converter'), and ts-prune is not wired anywhere in the repo); index.ts still re-exports the class and no test references it. A user who reads 'encodes Strands InterruptErrors as typed, non-retryable ApplicationFailures' and installs it to get the old behavior gets no top-level rewrite anywhere, because every SDK encode site wraps with ensureApplicationFailure/ensureTemporalFailure first; the class only ever retypes a terminal error nested as a cause, which changes neither retries (the server reads only the top-level failure) nor interrupt routing (TemporalActivityTool reads one level). The PR body presents the loss of cause-level rewriting as a breaking change; the only observable change is the shape of nested causes. In a release already marked breaking: delete the class and the instance (or mark both @deprecated with this reason), move rewriteError and the interceptor to their own module, and keep STRANDS_INTERRUPT_TYPE in a constants module so workflow code stops bundling the dead class. The public docs page's 'Interrupt from an Activity tool' section names StrandsFailureConverter and carries the 'attach StrandsPlugin to the client' caution this PR retracts, so it needs rewriting regardless.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. #281, stacked on this PR, deletes the class, its instance and the index export, and renames the module to strands-errors, which keeps rewriteError and STRANDS_INTERRUPT_TYPE. The docs page and the TypeScript sample need a change once it ships.

Comment thread typescript/strands-agents/README.md Outdated
```ts
const client = new Client({ /* ... */, plugins: [new StrandsPlugin({ models })] });
```
The plugin keeps the failure converter your client and worker set with `dataConverter.failureConverterPath`, for example to [encode failure messages](https://docs.temporal.io/failure-converter). For activity-tool interrupts to work, your converter must decode a failure of type `StrandsInterrupt` to an `ApplicationFailure` with that type and its details, as the SDK's default converter does. Earlier versions replaced your converter with the plugin's, so workflow code now decodes the failures of runs that started before the upgrade with your converter. If your converter decodes them into other errors than the SDK's default converter and that changes which commands a workflow issues, those runs fail to replay with a nondeterminism error. If it only changes a value the workflow keeps or returns, the runs replay without error and continue with the new value, so replay tests do not catch them. Let those runs finish before you upgrade, or use Worker Versioning.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 (docs): this paragraph mixes three concerns (the plugin keeps your converter; what it must decode for interrupts; an upgrade/replay caveat with two conditional branches) and buries the upgrade warning mid-paragraph, while line 96 already carries the same 'let those runs finish first, or use Worker Versioning' instruction in different words. Keep the first two sentences here and split the caveat into a short bold-led note (or point both sections at one shared upgrade note) so the next caveat lands in one place.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split in cb4cfa4: the paragraph keeps the first two sentences, and the caveat is its own Upgrading from an earlier version note. I left the note at line 96 alone, since it's about the model-config read, a different change.

test.before(async (t) => {
t.context = {
env: await startLocalWithRetry(() =>
TestWorkflowEnvironment.createLocal({ client: { dataConverter: { failureConverterPath } } })

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 (tests): this file pays for its own ava child process, a second dev server and a custom-converter client that no assertion needs, and because ava runs files one at a time it lands fully on the wall clock: measured 2.76 s of an 11.89 s local make test (23%), roughly 4-8 s per CI cell. err.cause.details is decoded from the details payloads independent of encodeCommonAttributes, and both 'Encoded failure' assertions read raw history, so the shared env's default-converter client returns the same values. The memory justification at line 11 is unsupported (no heap limit is configured anywhere, and one extra 10.5 MB bundle plus one worker is ~5% on top of the 20 concurrent workers). Cheaper and equivalent: build a second bundle with failureConverterPath in test-strands.ts's test.before (sequential, before any worker exists), and run this test there with createWorker({ workflowBundle, dataConverter: { failureConverterPath }, plugins: [new StrandsPlugin()], activities }). Two smaller reuse points: the history projection at line 61 and the one in test-strands.ts:831-837 are two shapes of the same helper (google-adk has countScheduledActivities as the precedent), and the inline interrupt-shaped throw at line 41 duplicates the deleteThing fixture, so the structural contract isInterruptError relies on is hand-written in two files.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved in cb4cfa4. The test now runs in test-strands.ts on a second bundle that test.before builds with the caller's converter, using the shared env's Client, and test-failure-converter.ts is gone. activityFailures is one helper for both tests, and interruptError is a shared fixture that deleteThing uses too.

decoded.push(cause instanceof ApplicationFailure ? [cause.type, cause.nonRetryable, cause.details] : null);
});
}
throw ApplicationFailure.create({ message: 'workflow failed', nonRetryable: true, details: decoded });

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 nit: failActivities smuggles its assertion data out through a synthetic ApplicationFailure (Promise<never>, a .catch pushing null, details: decoded) and the test unpacks it with a boolean-or-array deepEqual whose failure mode is deepEqual(false, [...]). Returning decoded is the plain form; the only thing the throw enables is t.is(workflowFailure?.message, 'Encoded failure'), the one assertion proving the bundled workflow-side converter is the caller's. If that proof is kept, a one-line comment saying so explains the shape.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kept the throw, since the Encoded failure check on the Workflow failure is what shows the bundle uses the caller's converter. I added a comment saying so (cb4cfa4).

… their stacks (breaking)

MaxTokensError and StructuredOutputError are thrown by the agent in
workflow code, where the activity interceptor never sees them, so they
still failed the workflow task on every attempt. The plugin now adds a
workflow interceptor module, through configureBundler so worker-built
and prebuilt bundles both get it, that rethrows them as typed,
non-retryable failures from the main function and from signal and
update handlers.

The rewrite now uses ApplicationFailure.fromError, so the failure keeps
the error's stack and cause. Interrupts are BENIGN failures, so the
worker no longer logs each pause as a failed activity. Terminal errors
are also matched by name, for a second copy of @strands-agents/sdk.

workerInterceptors is now a function that keeps the worker's other
interceptors, including activityInbound, so the configureWorker
override is gone.

README: the terminal errors move to Retries, with where each one is
thrown, and the upgrade caveat becomes its own note. Tests: the
converter test moves into test-strands.ts with a second bundle, a new
test covers MaxTokensError thrown in workflow code, and the interrupt
test checks that the failure keeps the thrown error's stack.

Breaking change: a run whose workflow task keeps failing on one of
these errors fails with it once this version runs.
@campoy

campoy commented Oct 29, 2026

Copy link
Copy Markdown
Contributor Author

@DABH thanks for the second pass. Replies are on each thread; on the two points in the summary:

  1. Errors thrown in Workflow code: done in cb4cfa4. A Workflow interceptor module rewrites the three terminal errors where the agent throws them, so the run fails with a non-retryable failure instead of looping on Workflow Task failures. A new test covers MaxTokensError.
  2. Decoding the overflow back into Strands' error: agreed, it's needed. It changes Workflow behavior on its own (a run that caught the overflow calls the model again on replay), so it's in fix(strands): let the agent trim the conversation on a context overflow (breaking) #279, stacked on this PR, with its own replay check.

@brianstrauch brianstrauch left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One additional replay compatibility finding on cb4cfa4. Lint and all 38 existing tests pass; a targeted replay reproduction fails as described inline.

inbound: [
{
execute: (input, next) => rethrowTerminalError(() => next(input)),
handleSignal: (input, next) => rethrowTerminalError(() => next(input)),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Replay can break with the default failure converter too

The extra async wrapper delays when the SDK removes the Signal handler from its in-progress set. This changes allHandlersFinished() even when the handler succeeds and no custom failure converter is configured. For example, in a Workflow with finished and unfinished Activity proxies:

let signaled = false;
setHandler(defineSignal('finish'), () => { signaled = true; });
await condition(() => signaled);
await Promise.resolve();
return allHandlersFinished() ? activities.finished() : activities.unfinished();

I recorded this with the exact base revision's plugin (63d7aec4): it schedules finished, and its history replays successfully with that plugin. Replaying the same history with this head schedules unfinished and fails with [TMPRL1100] Activity type of scheduled event 'finished' does not match activity type of activity command 'unfinished'. There are no thrown errors or custom converters in this reproduction.

The current README upgrade warning only identifies changes in custom-converter decoding, so it misses this affected group. Please gate the new interceptor timing for existing histories, or broaden the upgrade/Worker Versioning guidance to cover this case, and add a replay regression test.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the reproduction. We're keeping the handler wrapper, since this release already breaks compatibility and the wrapper lets a Strands error in an Update handler reject the Update. caae774 documents the break instead:

  • The README (Retries) and the PR body say that a run whose code depends on when a Signal or Update handler finishes, such as checking allHandlersFinished() right after a handler runs, can fail to replay. They give the same advice: drain those runs or use Worker Versioning.
  • A replay test uses your case. It records handlerTiming with main's plugin at 63d7aec, which schedules finished, and checks that this version fails to replay it with [TMPRL1100] ... 'finished' ... 'unfinished'.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more case, now named in the README and PR body (3c36571): an Update handler that only releases the Workflow, which then schedules an Activity in the same Workflow Task. Both versions take the same branch, but the Update's completion is now recorded after the Activity, so an earlier history fails with [TMPRL1100] Activity machine does not handle this event.

…rceptor

The workflow interceptor wraps every signal and update handler, so the
SDK counts a handler as finished a little later than before. A run
started on an earlier version whose code depends on that moment, for
example by checking allHandlersFinished() right after a handler runs,
can take another path on replay, even with no failure converter and no
Strands error.

The README's Retries section now says so, with the same upgrade advice
as the other breaking changes. A test replays a handlerTiming history
recorded with main's plugin at 63d7aec and checks that it now fails
with a nondeterminism error.
An update handler that only releases the workflow, which then schedules
an activity in the same task, also fails to replay across this change
with no allHandlersFinished() read: the update's completion is now
recorded after the activity instead of before it. Say so next to the
allHandlersFinished() example.
@campoy
campoy requested review from DABH and brianstrauch October 29, 2026 22:57

@DABH DABH left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Third round, at head 3c36571. Same recipe: ten finder angles, 29 raw items deduplicated into 24 verifiable ones plus one from the gap sweep, each put through an adversarial verifier; 17 confirmed, 2 plausible, 5 refuted. Sixteen of the verdicts were settled by running scripts against a local dev server at the head; pnpm run build, eslint and the 39 tests pass there. Refuted and dropped: the claim that the execute wrapper also shifts the terminal command (sdk-core's terminal-command normalization places it last; 90 cross-replays over five variants were clean with a failing positive control), narrowing the terminal rewrite to the plugin's own Activities (documented contract, matches the Python twin), the loadHistoryFromDir nit, and the two suggestions to drop the type: name override and the literal strings (both are load-bearing under consumer minification and for #279's cause.type match). The stacked #279 and #281 were read for context.

Severity: P1 = wrong result or stuck run in realistic use; P2 = correctness or robustness in an edge case; P3 = docs, tests, cleanup.

Previous rounds. Round one is addressed or superseded throughout: the interceptor altitude replaced the converter path, the temp-module code and its seven items are gone, the Activity-side rewrite runs before ensureApplicationFailure, the README's client paragraph and the temp-dir text are gone, and the converter test moved into test-strands.ts with a typed assertion. Round two: the workflow-side twin for MaxTokensError and StructuredOutputError exists, the function-form workerInterceptors replaced the override, stack, cause, BENIGN and the name fallback are in, README line 113 names where each error is thrown, the upgrade note exists. Deferred by you and noted as such: decoding the typed overflow back in TemporalModel (#279) and the dead StrandsFailureConverter export (#281). Not addressed: a test that passes interceptors to a Worker with the plugin, and the remedy is now duplicated three times in the README instead of twice.

Where this round's problems cluster: the Activity-side rewrite is right, and the workflow-side half has three gaps the README does not admit: the overflow failure comes back wrapped in a plain ModelError that nothing recognises, prebuilt bundles from the previous version never get the interceptor module, and the SDK's configured failure types are silently overridden. The rest is the documentation stating outcomes the code delivers only once #279 lands.

Findings on lines outside the diff

  • typescript/strands-agents/src/failure-converter.ts:28: P3 (pre-existing, scope): two other ModelErrors Strands raises in workflow code over the recorded Activity output loop the Workflow Task exactly like the overflow case. TemporalModel overrides only stream(), so Model.streamAggregated runs in the Workflow over the events array the Activity returned; a provider that streams a toolUseInputDelta whose accumulated JSON does not parse (model.js:334) or ends without a messageStop event (model.js:315) completes the Activity successfully and throws the ModelError on every replay; the list here names only the three errors. Reproduced on the head with stub providers for both: ACTIVITY_TASK_COMPLETED, then WORKFLOW_TASK_FAILED twice, then timeout; same on main. A workflow-side-only ModelError match would cover these; it cannot go in the shared list because ModelThrottledError and ProviderTokenCountError extend ModelError and must stay retryable Activity-side. Line 113 reads as the general behaviour, so at least scope it.
  • typescript/strands-agents/README.md:332: P3: 'Register both on the client and the worker' is the last sentence still telling readers to put StrandsPlugin on a Client after this PR removed the 'Attach StrandsPlugin to the client' paragraph and plugin.ts:71-75 now says 'Register on the worker'; the example below registers only the OTel plugin on the client. Harmless at runtime (SimplePlugin.configureClient with no converter or client interceptors returns the Client's own unchanged), but the sentence contradicts the worker-only story and the upgrade note at 235 does not say client registration can be dropped. Suggested: 'Register OpenTelemetryPlugin on the client and the worker, and StrandsPlugin on the worker.'

Not verifiable here: the external docs page and sample that still mention StrandsFailureConverter (other repositories; #281's body lists them), a second @strands-agents/sdk copy (mechanism documented, not reproduced), and real provider behaviour. The macOS, Windows and Node 22 cells are green on the head.

Comment thread typescript/strands-agents/README.md Outdated

Passing `retryStrategy` to `new TemporalAgent(...)` throws; remove the argument and put the retry config on the activity options instead.

The plugin makes Strands' terminal errors (`ContextWindowOverflowError`, `MaxTokensError` and `StructuredOutputError`) non-retryable. Its activity interceptor does this for a `ContextWindowOverflowError` from the model activity, and for any of the three that your own activity throws. The agent throws `MaxTokensError` and `StructuredOutputError` in workflow code, where the plugin's workflow interceptor does it: the run fails, or the update is rejected, with a failure of that type, instead of the workflow task failing on every attempt.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: with this PR alone, a ContextWindowOverflowError from the model Activity no longer retries but still never ends the run; the Workflow Task fails on every attempt until the execution timeout, the opposite of what this paragraph and the breaking-change bullet say. The Activity interceptor records ActivityTaskFailed {type: 'ContextWindowOverflowError', nonRetryable: true}; TemporalModel.stream() (temporal-model.ts:107-111, no catch) lets the ActivityFailure into Strands' Model.streamAggregated (models/model.js:349-355), which rethrows new ModelError('Activity task failed', { cause }); the conversation manager's recovery hook tests instanceof ContextWindowOverflowError (false); rewriteTerminalError matches neither instanceof nor the name; handleWorkflowFailure records a task failure. Reproduced on the head with a stub model: ACTIVITY_TASK_FAILED then WORKFLOW_TASK_FAILED('Activity task failed') repeated until WORKFLOW_EXECUTION_TIMED_OUT, never WORKFLOW_EXECUTION_FAILED; without an execution timeout the run stays Running, and a __stack_trace query sent during the loop never settled. #279's own body confirms the loop. Either rewrite, workflow-side only, a ModelError whose cause is a TemporalFailure (ModelThrottledError and ProviderTokenCountError extend ModelError and must stay retryable in the Activity interceptor), or say in README and body that until #279 lands the overflow fails the Workflow Task on every attempt. P1 if #200 can ship without #279.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed by folding #279 into this PR (455d742): TemporalModel.stream turns the failure back into Strands' ContextWindowOverflowError, so the conversation manager trims and the turn answers. I also checked the case where nothing can be trimmed: the run now fails with a non-retryable ContextWindowOverflowError, where 3c36571 looped on the Workflow Task until the execution timeout. The Retries text now says this, and it is scoped to the three errors (072b20e).

* benign failure, so the worker does not log it as an activity failure. Any
* other value is returned as it is.
*/
export function rewriteError(err: unknown): unknown {
if (isInterruptError(err)) {
const details = err.interrupts.map((i) => i.toJSON());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: rewriteError trusts the hand-built interrupt shape beyond what isInterruptError checks, and the two failure modes are a lost pause and a dead Worker. Lines 20-26 validate only interrupts[0].toJSON; line 59 calls toJSON() on every element inside the Activity interceptor's catch, line 61 reads .name from each result, and line 64 forwards the results as failure details unchecked. (a) An activityAsTool Activity throwing Object.assign(new Error('interrupt'), { interrupts: [{ toJSON: () => ({...}) }, {}] }): rewriteError throws TypeError: i.toJSON is not a function, Activity.run encodes it as a retryable TypeError, the server retries, the interrupt branch in temporal-activity-tool.ts:88-91 is never reached and the agent never pauses (reproduced: attempt count rose 2 to 5 over 9 s, run stayed RUNNING). (b) Same shape, well formed, but toJSON() returns a circular or BigInt reason: the interceptor succeeds, then encodeErrorToFailure throws ValueError: Unable to convert [object Object] to payload at payload-converter.js:197, worker.js:813 has no catch, and worker.run() rejects with 'Worker failed' (reproduced). Every Activity-side interrupt is necessarily hand-built (Strands exports neither Interrupt nor InterruptError from its root, and toolContext.interrupt() exists only workflow-side), the README documents no shape and InterruptLike is not exported. Validate every element (function toJSON, object result with a string name), wrap the mapping, and fall back to a non-retryable ApplicationFailure describing the malformed interrupt.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 072b20e. rewriteError checks every entry: toJSON() must be a function and return an object with a string name, and the details must be JSON-encodable. A malformed interrupt fails without retries, with a message that says so. A new test covers both of your shapes (an entry without toJSON(), and a BigInt reason). With c083568 they give a retryable TypeError and "Worker failed"; now both are non-retryable failures and the Worker keeps running. The README now documents the shape an Activity throws.

Comment thread typescript/strands-agents/src/plugin.ts Outdated
return {
...base,
ignoreModules,
workflowInterceptorModules: [

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: the workflow interceptor reaches a bundle only through configureBundler, so a Worker given a prebuilt workflowBundle built by the previous plugin version has no Strands workflow interceptor and nothing warns. The function-form workerInterceptors at 122-125 returns no workflowModules, so the Worker's 'Ignoring interceptors.workflowModules' check (worker.js:439-445) never fires, getOrCreateBundle parses the bundle as-is, and the runtime calls only the bundle's own importInterceptors(), which the published 1.24.0 plugin leaves empty. Reproduced on the head with a bundle whose configureBundler strips the module: zero warnings at Worker.create, helloAgent with a maxTokens stop gives two WorkflowTaskFailed then WorkflowExecutionTimedOut; the control bundle fails the run with type MaxTokensError in 120 ms. bundleWorkflowCode without plugins is not a practical trigger (it fails on disallowed modules), so the trigger is a bundle not rebuilt after the upgrade, which is the SDK-recommended production deployment (worker-options.d.ts:111-130). The README has no prebuilt-bundle guidance at all; the openai-agents README has a 'Pre-built workflow bundles' section. Add that section (pass plugins: [new StrandsPlugin()] to bundleWorkflowCode, rebuild bundles when upgrading), scope line 113 and the PR-body sentence to bundles built with this version, and extend the line-235 note. A bundle-independent fallback is merging workflowFailureErrorTypes in configureWorker (design note below).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a "Pre-built workflow bundles" section (pass the plugin to bundleWorkflowCode, rebuild when you upgrade), and the upgrade section and PR body say that a bundle built with an earlier version has no Workflow interceptor and nothing warns (072b20e). I kept the interceptor rather than the workflowFailureErrorTypes fallback, for the Update reason in your design note.

for (const { ctor, name } of TERMINAL_ERROR_TYPES) {
// Match the name too, in case another copy of `@strands-agents/sdk` threw it.
if (err instanceof ctor || err.name === name) {
return ApplicationFailure.fromError(err, { type: name, nonRetryable: true });

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: a Worker that already handled a Strands terminal error with the SDK's workflowFailureErrorTypes (or failureExceptionTypes) plus a workflow retry policy got a retryable failure and a retry run on main; with this PR the plugin's innermost wrapper rewrites the error to nonRetryable: true first, so the retry policy is silently skipped. The module is appended last (plugin.ts:184-187), so the execute wrapper is innermost; handleWorkflowFailure (internals.js:1051-1073) takes the instanceof TemporalFailure branch before isConfiguredFailureException and pushes failWorkflowExecution with nonRetryable kept; on main the configured-type branch ran ensureTemporalFailure, nonRetryable: false. Reproduced on the head with workflowFailureErrorTypes: { '*': ['MaxTokensError'] } and retry: { maximumAttempts: 2 }: PR bundle RETRY_STATE_NON_RETRYABLE_FAILURE, no new run; control bundle RETRY_STATE_IN_PROGRESS with a second run. Same for signal handlers. Nothing in the plugin inspects the option, the body's 'Before, it failed the Workflow Task on every attempt' describes only the unconfigured case, and the google-adk README recommends exactly this option. Name the interaction under Retries and in the breaking-change list, or leave nonRetryable false when the Worker configures these types, or expose an option.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documented in 072b20e, under Retries and in the breaking-change list: a Worker that lists these errors in workflowFailureErrorTypes used to get a retryable failure and a retry run, and now the run is not retried. I didn't change the behavior: a retry would hit the same deterministic error again.

Comment thread typescript/strands-agents/README.md Outdated

Passing `retryStrategy` to `new TemporalAgent(...)` throws; remove the argument and put the retry config on the activity options instead.

The plugin makes Strands' terminal errors (`ContextWindowOverflowError`, `MaxTokensError` and `StructuredOutputError`) non-retryable. Its activity interceptor does this for a `ContextWindowOverflowError` from the model activity, and for any of the three that your own activity throws. The agent throws `MaxTokensError` and `StructuredOutputError` in workflow code, where the plugin's workflow interceptor does it: the run fails, or the update is rejected, with a failure of that type, instead of the workflow task failing on every attempt.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: interceptors registered through the plain SDK options always run outside the plugin's, so a user interceptor that recovered from a Strands error by class or name on main now sees an ApplicationFailure (name ApplicationFailure, type MaxTokensError) and silently stops matching. Verified by printing the lists: with interceptors.workflowModules: [user] or bundleWorkflowCode({ workflowInterceptorModules: [user], plugins: [strands] }) the bundler order is [user, lib/workflow-interceptors.js], and interceptors.activity factories precede the plugin's; index 0 is outermost. Reproduced: a workflow interceptor returning a fallback on instanceof MaxTokensError worked on the control bundle and failed on the PR bundle (ctor=ApplicationFailure type=MaxTokensError instanceofMaxTokens=false); same for an Activity interceptor recovering from ContextWindowOverflowError. The one placement that lands inside is a SimplePlugin({ workerInterceptors: ... }) listed after StrandsPlugin, which nothing documents. Say to match on ApplicationFailure.type, or name the plugin placement; note the original instance and MaxTokensError.partialMessage are not reachable from an outer interceptor.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added to Retries (072b20e): your own Activity and Workflow interceptors run outside the plugin's, so they see the ApplicationFailure and should match on its type; the original error and fields like MaxTokensError.partialMessage aren't reachable from them.

Comment thread typescript/strands-agents/src/plugin.ts Outdated
dataConverter: {
failureConverterPath: require.resolve('./failure-converter'),
},
// A function, so that the worker's other interceptors, such as `activityInbound`, are kept.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 (comment): 'so that the worker's other interceptors, such as activityInbound, are kept' overstates. The object form already keeps and concatenates client, activity, nexus and workflowModules (plugin.js:170-180); the one key it rebuilds without is the deprecated activityInbound, which compileWorkerInterceptors still honours (worker-options.js:91-100). The function form is the right choice; the comment should name exactly that key.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 072b20e: the comment now says the object form rebuilds the interceptors without the deprecated activityInbound key.

export function rewriteTerminalError(err: unknown): unknown {
if (err instanceof Error) {
for (const { ctor, name } of TERMINAL_ERROR_TYPES) {
// Match the name too, in case another copy of `@strands-agents/sdk` threw it.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 (comment, plausible): keep both predicates, but say what each buys. err.name === name rewrites any Error whose name is one of the three strings whatever threw it (this interceptor runs around every Activity on the Worker, so a user class that sets this.name = 'MaxTokensError' in an unrelated Activity is also made non-retryable, verified), while instanceof still catches a subclass that overrides name (class Bar extends ContextWindowOverflowError with this.name = 'Bar', verified). 'another copy of @strands-agents/sdk' undersells the first and omits the second; a second copy is realistic under pnpm peer-keyed resolution, and the SDK's own workflowFailureErrorTypes is name-based too. The type: name override and the literal strings are load-bearing (constructor names are mangled under consumer minification, and #279 matches err.cause.type === 'ContextWindowOverflowError'), so keep them.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 072b20e: the comment now says the name catches another copy of @strands-agents/sdk or any error with that name, and instanceof also catches a subclass with a name of its own.

import type { WorkflowInterceptorsFactory } from '@temporalio/workflow';
import { rewriteTerminalError } from './failure-converter';

async function rethrowTerminalError<T>(run: () => Promise<T>): Promise<T> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 (style): a generic two-argument async function rethrowTerminalError<I, R>(input: I, next: (input: I) => Promise<R>): Promise<R> assigned directly to execute, handleSignal and handleUpdate type-checks under strict TS, behaves identically through composeInterceptors (which calls interceptor[method](input, next) with exactly two arguments), and drops the per-call thunk. .catch chaining is not the simpler alternative: same extra microtask (ticks measured bare 1, async wrapper 2, catch chain 2), and only the async form turns a synchronous throw into a rejection. google-adk's thunk is load-bearing because it passes a frame; this one has no such argument.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 072b20e: rethrowTerminalError(input, next) is assigned directly to the three methods.

];
}

/** Each failed activity's message, type and non-retryable flag in a run's history. */

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 (readability): activityFailures returns positional unknown[] triples that the call sites read as ['interrupt:approval', 'StrandsInterrupt', true] (883, 918-921), relying on the doc comment for the order. Self-labelled { message, type, nonRetryable } objects make the expectations self-describing; the tuples at workflows/strands.ts:221 and 941-943 would follow. Optional.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 072b20e: activityFailures and failActivities return labeled objects.

const failure = history.events?.find((e) => e.activityTaskFailedEventAttributes)?.activityTaskFailedEventAttributes
?.failure;
t.regex(failure?.stackTrace ?? '', /interruptError/);
const scheduled = history.events?.filter(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 (tests): t.is(scheduled?.length, 2) is implied by deleteState.calls === 2 at 880, the single terminal StrandsInterrupt failure at 883 and the run finishing with 'done'. ActivityTaskFailed is recorded only for the terminal attempt of a scheduling, and 'done' is reached only through the resume path, which re-schedules deleteThing, so two calls force exactly two schedulings; on main the run records zero failed Activities, which 883 already catches. Keeping it as a restatement of the comment at 881-882 is defensible.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropped it in 072b20e, with the comment reworded to say what the other checks show.

…ow (breaking)

A ContextWindowOverflowError from the model activity reached the agent
as an ActivityFailure, which Strands wraps in a plain ModelError. The
conversation manager only trims on ContextWindowOverflowError, so it
never ran: the error left agent.invoke and, uncaught, failed the
workflow task on every attempt.

TemporalModel.stream now rethrows a failure of type
ContextWindowOverflowError, which the plugin's activity interceptor
produces, as Strands' own ContextWindowOverflowError. The conversation
manager then trims and calls the model again, as outside Temporal.

Breaking change: a run whose workflow code caught that overflow before
this version calls the model again on replay and fails with a
nondeterminism error. A run whose workflow task kept failing on it
continues and trims.
The plugin no longer installs StrandsFailureConverter: an activity
interceptor and a workflow interceptor rewrite Strands errors instead.
As a failure converter it could only retype a Strands error nested as
the cause of another error, since the SDK wraps every top-level error
before a converter sees it, so it changed neither retries nor interrupt
routing.

Remove the class, its failureConverter instance and the index export,
and rename the module to strands-errors, which keeps rewriteError,
rewriteTerminalError and STRANDS_INTERRUPT_TYPE.

Breaking change: code that imports StrandsFailureConverter, or sets
failureConverterPath to @temporalio/strands-agents/lib/failure-converter,
no longer builds or starts. Remove the import or the path.
@campoy campoy changed the title fix(strands): keep the caller's failure converter and stop retrying activity interrupts (breaking) fix(strands): rewrite Strands errors with interceptors and keep the caller's failure converter (breaking) Oct 30, 2026
…de notes

Activities build interrupts by hand. An entry without toJSON() made the
activity interceptor throw a retryable TypeError, so the agent never
paused, and details the payload converter could not encode stopped the
worker. rewriteError now checks every entry and its details, and fails
a malformed interrupt without retries, with a message that says so.

README:
- Retries names where each terminal error comes from, scopes the
  workflow-side rewrite to those three errors, says that the worker's
  own interceptors see the ApplicationFailure, and that a worker listing
  these errors in workflowFailureErrorTypes no longer gets a retry. The
  run, or the update, fails; nothing rejects it.
- The upgrade notes from Models, Retries and Human-in-the-loop become
  one "Upgrading from 1.24 and earlier" section, with the remedy and
  rollback once, the removed StrandsFailureConverter, and that the
  plugin is no longer needed on the client.
- A pre-built workflow bundles section, the interrupt shape an activity
  throws, and OpenTelemetryPlugin on the client with StrandsPlugin on
  the worker.

Tests: the converter test runs on a worker-built bundle, so its
workflow-failure check tells this version from main, and checks that a
worker's own activityInbound interceptor still runs; activityFailures
returns labelled objects; a new test covers malformed interrupts; the
interrupt test drops a count its other checks imply.

Comments: say why the workflow interceptor skips interrupts, what the
name and instanceof matches each catch, and that the function form of
workerInterceptors keeps activityInbound. rethrowTerminalError takes
the interceptor's input and next directly.
@campoy

campoy commented Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

@DABH thanks for the third pass. #279 and #281 are now folded into this PR (455d742, c083568), and 072b20e covers the rest. Replies are on each thread. On the points outside the diff:

  • Other ModelErrors from Workflow code (failure-converter.ts:28): agreed, they're pre-existing. Retries now says the rewrite covers only the three errors, and that other errors Strands raises in Workflow code, such as a ModelError for a response it can't parse, still fail the Workflow Task. I'd keep a broader ModelError rule for a separate change.
  • Observability (README.md:332): now "Register OpenTelemetryPlugin on the client and the worker, and StrandsPlugin on the worker", and the upgrade section says the plugin can be removed from the client.
  • The interceptors test from your earlier rounds: the converter test now passes an activityInbound interceptor and checks that it runs.
  • The repeated remedy: the README now has one upgrade section with the remedy and rollback once.

Lint, Prettier and the 41 tests pass locally, and CI is green on 072b20e. I think this covers everything from this round. Let me know if anything still blocks an approval.

@campoy
campoy requested a review from DABH October 30, 2026 04:51

@DABH DABH left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fourth round, at head 072b20e (17 commits since 63d7aec; the stacked #279 and #281 are closed and folded in as 455d742 and c083568, and 072b20e adds the malformed-interrupt validation, the pre-built-bundle and upgrading sections, and the test tidy-ups). Same recipe: ten finder angles, 44 raw items deduplicated to 19 candidates plus two from the gap sweep, one adversarial verifier per candidate, eleven of them settled by running Workers against a local dev server at the head. Verdicts: 13 confirmed, 4 plausible, 2 refuted and dropped (the replay fixture pinning SDK microtask order is the SDK's flag-gated determinism contract and SDK 1.25.0 reproduces the identical outcome matrix; a workflow.patched gate on the overflow decode would protect no run, since the ungated model-config read from #158 already fails replay of every 1.24.0 agent run on its first task, verified against the committed fixture). In a fresh worktree at the head: tsc, eslint and ava 41 of 41 clean. Not verifiable here: real provider mid-stream overflows, the external docs page and sample that still name StrandsFailureConverter (acknowledged in the body), the macOS, Windows and Node 22 cells, and whether any commonly used payload converter is stricter than JSON.

Severity: P1 = wrong result or stuck run in realistic use; P2 = correctness or robustness in an edge case; P3 = docs, tests, cleanup.

Round three. Addressed: the overflow decode in TemporalModel.stream with its trim test; prebuilt bundles documented; the workflowFailureErrorTypes override documented and kept by design; the interceptor-placement sentence; the update wording; the converter docstring superseded by the rename to strands-errors.ts; the interrupt rationale comment; the design note answered in the body; one Upgrading section with the rollback sentence (the '1.24 and earlier' anchor is right: npm's 1.24.0 lacks strandsGetModelConfig); the converter test now builds from workflowsPath and counts a Worker interceptor; the comment and predicate nits; the labeled helper; the redundant assertion. Partially: malformed interrupts are validated entry by entry, but the gate still probes only the first entry (P2 below). Older rounds: the interceptor test exists, the remedy triplication is gone, the stack and cause loss is fixed, and the handler-timing break is documented with a regression fixture; the external docs and sample stay open in other repositories.

Where this round's problems cluster: one material gap, pre-existing on main but squarely in this PR's subject: every model-Activity failure other than an overflow (cancel, timeout, exhausted retries, a non-retryable provider error) is wrapped by Strands in a plain ModelError that nothing recognises, so the run loops on the Workflow Task and cannot be cancelled. Then the first-entry gate on interrupts, and a set of docs and test gaps around the contract this PR publishes for Activity-side interrupts.

Findings on lines outside the diff

  • typescript/strands-agents/src/temporal-activity-tool.ts:98: P3: rewriteError validates every interrupts entry and encodes all of them (message interrupt:<a,b>, two details) and README 235 now says 'each entry needs a toJSON()', yet stream routes only details[0] through toolContext.interrupt(), so every further entry is never raised, never answered and never appears in AgentResult.interrupts. Reproduced in process with Strands' real InterruptState: an Activity throws { interrupts: [approval, confirm] }; rewriteError records interrupt:approval,confirm with two details; the first run raises only 'approval'; after the Workflow answers it and resumes, the Activity re-throws, interrupt({ name: 'approval' }) returns 'yes', the tool result is 'yes', and 'confirm' is never raised and nothing logs it. toolContext.interrupt() raises one interrupt per call, but sequential or accumulated raising is possible, so iterating is a design option, not a forced outcome. Either say at README 235 and in the docstring (39-44) that an Activity interrupt raises exactly one interrupt and make rewriteError treat interrupts.length > 1 as malformed (dropping the join), or iterate details here, calling toolContext.interrupt() for each in order and returning the collected responses. No test covers a well-formed multi-entry interrupt.

Threads: the README 235 and strands-errors.ts:59 comments describe the same split-Worker failure mode (docs fix now versus contract fix); the temporal-model.ts:122 comment bundles three items on the same lines.

) {
throw new ContextWindowOverflowError(err.cause.message);
}
throw err;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 (P2 if scoped out as pre-existing): every model-Activity failure other than a context overflow is rethrown here as an ActivityFailure, which Strands' Model.streamAggregated wraps in a plain ModelError (model.js:349-356) that neither rewriteTerminalError (three constructors and names only) nor the SDK's handleWorkflowFailure recognises (isCancellation(ModelError) is false; not a TemporalFailure), so the Workflow Task fails on every attempt instead of the run failing or cancelling, and a cancel request is silently ignored. Reproduced at the head on a dev server: a stub model throwing a non-retryable ApplicationFailure with maximumAttempts 2 leaves the run RUNNING after 9 s with one ActivityTaskFailed and two WorkflowTaskFailed ('Activity task failed'), never WorkflowExecutionFailed; same for a plain Error with retries exhausted and for a 2 s startToCloseTimeout. handle.cancel() 1.5 s into a hanging model call: RUNNING with WorkflowExecutionCancelRequested, two failed tasks ('Activity cancelled') and no ActivityTaskCancelRequested, because the failing task's commands are discarded, so the provider call keeps running; with a model that answers 6 s after the cancel, the run COMPLETED with 'ok'. Update handlers awaiting agent.invoke() take the same route. Identical on main, but this PR adds the one catch that sees the raw ActivityFailure, adds the workflow-side rewrite whose purpose is this failure class, and README 115 now presents the leftover loop as a parse-error edge case under a section that says retries are handled exclusively by Temporal (98). Cheap fix verified: an override of the public streamAggregated in TemporalModel that rethrows err.cause when err instanceof ModelError && err.cause instanceof TemporalFailure (or the same unwrap in rewriteTerminalError) makes cancel end CANCELLED in 26 ms with the client receiving CancelledFailure, non-retryable FAILED in 117 ms, timeout FAILED with TimeoutFailure, and the late-completing cancel CANCELLED. The user-side workflowFailureErrorTypes { '*': ['ModelError'] } only turns both into FAILED with a retryable ModelError (cancel ends FAILED, not CANCELLED), so no configuration restores cancellation. Add two tests (cancel during the model Activity ends CANCELLED; model Activity fails after its retries ends FAILED with an ActivityFailure), and until then state the behaviour under Retries.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in db052d6, as part of the same failure class. TemporalAgent.stream(), which invoke() uses, now rethrows the Temporal failure that Strands wraps in a ModelError. So a cancel during a model call ends the run CANCELLED, and a model Activity that fails for good or times out fails the run with its ActivityFailure. Two new tests cover the cancel and the non-retryable failure. A one-off check also shows a cancel during an activityAsTool tool Activity now ends the run CANCELLED. Retries in the README says this.

const candidate = err as { interrupts?: unknown };
if (!Array.isArray(candidate.interrupts) || candidate.interrupts.length === 0) return false;
const first = candidate.interrupts[0] as { toJSON?: unknown } | undefined;
return typeof first?.toJSON === 'function';

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: isInterruptError gates on interrupts[0].toJSON only, so an interrupt whose first entry is malformed, including the natural mistake of plain { name, reason } entries with no toJSON, is never routed to the new 'malformed interrupt' branch and is retried as a plain Error, contradicting README 235 ('The plugin fails a malformed interrupt without retries') and the round-three reply ('rewriteError checks every entry'). Reproduced on a dev server: an activityAsTool Activity throws Object.assign(new Error('interrupt'), { interrupts: [{ name: 'approval', reason: 'delete foo?' }] }); rewriteError returns it unchanged (interruptDetails at 84-96 never runs), Activity.run encodes it as { type: 'Error', nonRetryable: false }, the tool proxy has no retry policy (temporal-activity-tool.ts:77-83), and the server retries with backoff (attempts at 1, 2, 4 and 8 s), the 'approve' signal ignored. With the committed fixture's succeed-on-second-call behaviour the run COMPLETED 'done' after two calls with no approval ever sent: the gated action ran without the pause and History records no failure. In process the same holds for [{}, { toJSON }], ['approval'], [null, { toJSON }] and []; only [{ toJSON }, {}] (the committed test shape) and a well-formed array take the new branches. Treat any Error with a non-empty interrupts array as an interrupt attempt and have interruptDetails check typeof entry?.toJSON === 'function' for every entry before calling it, so all these shapes take the existing non-retryable branch; add [{ name: 'approval', reason: '...' }] to the malformed test and assert nonRetryable and the attempt count in the Activity-tool interrupt test. Trade-off: a foreign error carrying an interrupts array (LangGraph's GraphInterrupt) escaping an Activity on a StrandsPlugin Worker would then fail as malformed instead of retrying.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in db052d6: any value with a non-empty interrupts array is now taken as an interrupt, and every entry is checked, so [{ name, reason }], [{}, { toJSON }], [null, ...] and ['approval'] all fail as malformed without retries. An empty array is still an ordinary error. The malformed test now includes the plain { name, reason } entry, and the interrupt test checks that the failure came from the Activity's first attempt. We accept the trade-off you name for a foreign error with an interrupts array.

Comment thread typescript/strands-agents/README.md Outdated
Interrupts also survive the activity boundary when raised from an `activityAsTool`-wrapped activity. The plugin's failure converter packages the interrupt as a typed `ApplicationFailure`, and `TemporalActivityTool` re-raises it through the agent's interrupt machinery on the workflow side, so `AgentResult.interrupts` is populated just like the in-workflow case.

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.
Interrupts also survive the activity boundary when raised from an `activityAsTool`-wrapped activity. An activity interceptor the plugin adds to the worker packages the interrupt as a typed, non-retryable `ApplicationFailure`, and `TemporalActivityTool` re-raises it through the agent's interrupt machinery on the workflow side, so `AgentResult.interrupts` is populated just like the in-workflow case. From an activity, throw an error with an `interrupts` array, the shape of Strands' `InterruptError`: each entry needs a `toJSON()` that returns an object with a string `name` and, optionally, a `reason`, all JSON-encodable. The plugin fails a malformed interrupt without retries.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: this paragraph never says that every Worker which executes an activityAsTool Activity must register StrandsPlugin; the old 'Attach StrandsPlugin to the client (not just the worker)' precondition was deleted without a replacement, and an Activity-only Worker on another task queue without the plugin turns the interrupt into a retryable Error, so the agent never pauses. Reproduced with two Workers: the Workflow Worker with the plugin on queue A, an Activity-only Worker without it on queue B hosting the PR's own deleteThing fixture, activityAsTool(..., { activityOptions: { taskQueue: B } }). Always-interrupting Activity: attempt 5 at 9 s, still RUNNING after the approve signal. Fixture that interrupts on call one and succeeds on call two, no signal: COMPLETED 'done', two calls, no failures (the deletion ran without approval). With the plugin on the tools Worker: paused, resumed on approve, one StrandsInterrupt failure (and that Worker then also registers the model Activities with the default Bedrock factory, plugin.ts:81-96). Plugins propagate only through Worker.create's list and the shared NativeConnection; every README mention is the singular 'the worker' while line 92 advertises per-agent task queues. One sentence here: the interceptor runs only on Workers that register StrandsPlugin, so register it on every Worker that executes an activityAsTool Activity, including Activity-only Workers on other task queues; optionally export the Activity interceptor so such Workers can add it via interceptors.activity without the model Activities (the contract alternative is in the strands-errors.ts:59 comment).

P3 (237): the caller-converter precondition is stated for interrupts only, but the new overflow recovery has the identical dependence (temporal-model.ts:117-121 requires err.cause instanceof ApplicationFailure && err.cause.type === 'ContextWindowOverflowError'), so line 113's unconditional 'It reaches the agent as Strands' own error, so its conversation manager trims' carries the same precondition, new in this PR because the base forced the plugin's own converter subclass. Reproduced with a DefaultFailureConverter subclass whose failureToErrorInner returns a DomainFailure extends TemporalFailure (in contract: the interface requires only a TemporalFailure) on the PR's own turnsAgent overflow scenario: control completes ['four', 6] with zero failed tasks; the domain converter gives the non-retryable ContextWindowOverflowError Activity failure and then 'Activity task failed' on every task until the execution timed out, the round-three loop re-opened for such converters. Name both decoded types at 237, and consider reading err.cause.failure?.applicationFailureInfo?.type (set by DefaultFailureConverter.failureToError), which removes the dependence for subclassing converters.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in db052d6: the interceptor runs only on Workers that register StrandsPlugin, so register it on every Worker that runs an activityAsTool Activity, including Activity-only Workers on other task queues. On 237: overflow recovery now reads the type through a helper that falls back to failure.failure.applicationFailureInfo.type, so a converter subclass that returns another TemporalFailure still recovers. The README names both conditions. I didn't export the Activity interceptor.

err.cause instanceof workflow.ApplicationFailure &&
err.cause.type === 'ContextWindowOverflowError'
) {
throw new ContextWindowOverflowError(err.cause.message);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: throw new ContextWindowOverflowError(err.cause.message) rebuilds the overflow with the message only (Strands' constructor takes no options), so when the conversation manager cannot trim, ApplicationFailure.fromError in the workflow interceptor finds no cause, and the WorkflowExecutionFailed failure and the client's WorkflowFailedError.cause.cause have no link back to the ActivityFailure or the provider's throw site; the Python twin keeps the chain (raise ContextWindowOverflowException(cause.message) from error, _temporal_model.py:246). Reproduced end to end with an always-overflowing model and the default sliding window: the client cause is ApplicationFailure{type: ContextWindowOverflowError, nonRetryable: true} with cause.cause undefined, the failure's first frame is this line, and the ActivityTaskFailed event (invokeModel, activityId 2, provider frame) is unlinked. With Object.assign(new ContextWindowOverflowError(msg), { cause: err }) in a scratch copy: cause.cause is the ActivityFailure with retryState NON_RETRYABLE_FAILURE, cause.cause.cause the ApplicationFailure with the provider stack, History chains the three infos, replay is OK both ways, 586 bytes more in the default config, and instanceof stays true for both classes. A no-trim test (pairs with the test gap at test-strands.ts:992) would assert err.cause.cause instanceof ActivityFailure.

P3 (plausible, same lines): the overflow type literal is written on the encode side (strands-errors.ts:29, stamped at 45) and again here at 120, while the interrupt half of the same contract is shared through the exported STRANDS_INTERRUPT_TYPE; export CONTEXT_WINDOW_OVERFLOW_TYPE from strands-errors.ts and use it at both sites (a shared string is safe under minification, unlike ctor.name), and export InterruptDetails for the inline cast at temporal-activity-tool.ts:93-97.

P3 (plausible): with streamingTopic, a model call that fails with an overflow after publishing events leaves them on the topic and the post-trim call publishes a complete message after them. Reproduced with a stub that yields three events then overflows: offsets 15-17 (messageStart, blockStart, delta 'STALE-PARTIAL') followed by 18-22 (the full message). Each call starts with its own modelMessageStartEvent, so subscribers can segment, but there is no terminal marker for the abandoned message and nothing documents it; the same shape already comes from an ordinary Activity retry on any mid-stream failure, so this is a README sentence under Retries or Streaming, not a code change.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in db052d6: the overflow keeps the ActivityFailure as its cause, and the new untrimmable-overflow test checks it's in the client's cause chain. CONTEXT_WINDOW_OVERFLOW_TYPE is now shared by both sides, and InterruptDetails replaces the inline cast. The Streaming section says that events from a failed call stay on the topic and the next call publishes a whole message after them.

t.true(failures.every((f) => f.message.startsWith('malformed interrupt')));
});

test('a context overflow in the model activity lets the agent trim the conversation', async (t) => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 (tests): README 113-115 and the body promise that an overflow the conversation manager cannot trim fails the run with a non-retryable ContextWindowOverflowError, and that a terminal error escaping a signal or update handler fails the run or the update, but the committed suite exercises neither path. The only overflow test is the trim path, the MaxTokensError test throws from the main function only, no test workflow defines an update handler (no defineUpdate in src/__tests__) or a throwing signal handler, and the body says both were checked with separate scripts. Verified end to end that both behaviours hold at the head (an always-overflowing model fails the run with the typed non-retryable failure and zero failed tasks; an update handler throwing MaxTokensError fails the update and the run completes; a signal handler fails the run). So no committed test would fail if TemporalModel.stream stopped decoding the overflow (the pre-#279 loop), if handleUpdate were dropped from the interceptor, or if the module were dropped from configureBundler; the update outcome is the sole stated reason for the handler-wrapping design and its documented replay break. Add 'a context overflow the agent cannot trim fails the run as non-retryable' beside this test (SmallContextModel(0, []) plus helloAgent, asserting ['ContextWindowOverflowError', true] like 1017-1022), and a workflow with defineUpdate whose handler throws MaxTokensError plus a done signal, asserting executeUpdate rejects with WorkflowUpdateFailedError while the run completes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added both in db052d6: a context overflow the agent cannot trim fails the run as non-retryable, and a Strands terminal error in an update handler fails the update, which runs an agent turn in an Update handler and checks for WorkflowUpdateFailedError while the run completes.

if (details === undefined) {
// Activities build interrupts by hand. Fail a malformed one without retries, rather than
// retrying it with no pause or stopping the worker when its details cannot be encoded.
return ApplicationFailure.fromError(err, {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: the malformed-interrupt branch is the only rewrite in this file that sets no plugin type (it inherits the thrown error's constructor name, Error or a user class) and it replaces the Activity's own message, so under encodeCommonAttributes the History entry is type Error, message 'Encoded failure', indistinguishable from any plain Activity error, although the PR's own converter test (951-954) proves the other rewrites stay identifiable by type under the same encoding; the comment at 53-54 promises a 'typed, non-retryable ApplicationFailure' and test-strands.ts:985-986 pins type Error. Verified against the built module: a nameless entry, a BigInt reason or a second entry without toJSON all give { type: 'Error', message: 'malformed interrupt: ...', nonRetryable: true, details: undefined }, the original message surviving only in the first stack line. Give the branch its own exported type constant (for example STRANDS_MALFORMED_INTERRUPT_TYPE), keep the original message (malformed interrupt (${err.message}): each entry needs ...), and assert that type in the test.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in db052d6: the malformed branch has its own exported type, STRANDS_MALFORMED_INTERRUPT_TYPE (StrandsMalformedInterrupt), keeps the Activity's message (malformed interrupt (<message>): ...), and the test asserts the whole record. On the nuance in your addendum: the README now says a malformed interrupt reaches the model as an error tool result.

Comment thread typescript/strands-agents/README.md Outdated
- An activity of yours that throws one of the three fails without retries.
- The agent throws `MaxTokensError` and `StructuredOutputError` in workflow code. When one of the three escapes your workflow function or a signal or update handler, the plugin's workflow interceptor makes the run, or the update, fail with a failure of that type, instead of the workflow task failing on every attempt. Other errors Strands raises in workflow code, such as a `ModelError` for a model response it cannot parse, still fail the workflow task on every attempt.

Your own activity and workflow interceptors run outside the plugin's, so they see the `ApplicationFailure`, not the Strands error: match on its `type`. The original error, and fields such as `MaxTokensError.partialMessage`, are not reachable from them.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: 'Your own activity and workflow interceptors run outside the plugin's, so they see the ApplicationFailure' is unconditional, but the Activity interceptor of any plugin listed after StrandsPlugin runs inside it and sees the raw Strands error or interrupt-shaped Error, so matching on type as advised never fires there. Activity factories compile to [WorkerOptions.activity, before-plugin, strands, after-plugin] (index 0 outermost): on a thrown ContextWindowOverflowError the after-plugin sees the constructor while the before-plugin and WorkerOptions see the ApplicationFailure. With OpenTelemetryPlugin 1.24.0: in the README's order [otel, strands] OTel sees the BENIGN StrandsInterrupt failure and leaves the span UNSET; in [strands, otel] it sees the raw Error and marks every Activity span that ends in an interrupt as ERROR. Workflow side the Strands module is always innermost on the workflowsPath path; an after-listed plugin's workflow interceptor lands inside only in a bundleWorkflowCode({ plugins: [strands, after] }) bundle. Qualify the sentence, and say under Observability that OpenTelemetryPlugin should precede StrandsPlugin so interrupt spans keep the benign status.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Qualified in db052d6: your own Activity interceptors, and those of plugins listed before StrandsPlugin, see the ApplicationFailure; those of plugins listed after it see the original error. Observability now says to list OpenTelemetryPlugin first. With the stream() override there's no Workflow interceptor left, so the Workflow side doesn't depend on order.

* malformed one becomes a non-retryable failure that says so. Any other value is
* returned as it is.
*/
export function rewriteError(err: unknown): unknown {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 (design): the Activity-side interrupt contract is a hand-built error shape (an Error with interrupts: [{ toJSON() }]) that exists only because Strands exports neither Interrupt nor InterruptError and no Strands code path ever hands user code a real InterruptError; a typed ApplicationFailure (type StrandsInterrupt, details [{ name, reason }], non-retryable, BENIGN) already crosses the Worker boundary intact with or without the plugin and is exactly what TemporalActivityTool decodes. Verified: on a Worker without the plugin a typed failure reaches the Workflow as NON_RETRYABLE_FAILURE with type, details and category intact in one attempt, while the README 235 shape becomes a retryable Error with empty details; with the plugin, the committed deleteThing fixture throwing the typed failure yields 'done', two calls and exactly the ActivityTaskFailed the existing test asserts; in the split-Worker topology of the 235 comment the typed failure pauses the agent and the hand-built shape proceeds as if approved. README 235 at this head is the first publication of the shape, in a breaking PR. An exported interrupt(name, reason?) helper returning that failure (with a JSON.stringify(reason) guard, optionally accepting a { name, reason } object so a nested agent's result.interrupts[0] can be forwarded) would replace isInterruptError, interruptDetails, the JSON oracle, the malformed branch and its test, and the shape prose, and remove the split-Worker failure mode. P2 if Activity-only Workers count as a supported deployment.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We kept the error shape and fixed it in db052d6: every entry is checked, a malformed one gets its own type, and every entry is raised in order. The README now says to register the plugin on every Worker that runs an activityAsTool Activity. A helper alone would make the shape that the plugin's own test used retry again, which is the defect this PR fixes.

* task, which a code fix can recover from, rather than fail the run.
*/
// ts-prune-ignore-next (loaded by path from workflowInterceptorModules)
export const interceptors: WorkflowInterceptorsFactory = () => ({

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 (design, plausible): a try/catch override of TemporalAgent.stream() (which invoke() consumes) that throws the typed failure delivers everything the handler-wrapping interceptor was chosen for, including failing just the update, with no workflow interceptor at all. doUpdate already fails the update for any TemporalFailure thrown by the handler body (internals.js:795-801) and the main function and signal handlers already fail the run non-retryably for one (1061-1070). Verified end to end on a bundle whose interceptor list was emptied (the control run on the same bundle looped the task twice then timed out, proving no interceptor was active): with an agent overriding only stream(), the main function fails the run with type MaxTokensError non-retryable and no new run under maximumAttempts 2; an update handler yields WorkflowUpdateFailedError with that cause and the run then completes; a signal handler fails the run; StructuredOutputError and an untrimmable overflow both surface through stream(); the committed handler-timing fixture replays OK on the interceptor-less bundle. That removes the handler-timing replay break (README 383, the fixture and its test), the silent prebuilt-bundle degradation (375), the bundler registration, and the bundle-wide err.name match on every workflow and handler. Build the failure with ApplicationFailure.create({ message, type, nonRetryable: true, cause: err }) so workflow code can still reach MaxTokensError.partialMessage on cause (fromError copies error.cause, not error). Trade-offs: workflow code catching Strands errors by instanceof around invoke() (README 114) would see the ApplicationFailure, and a plain Strands Agent given a TemporalModel would not be covered; if either is a hard requirement, keep the interceptor or a hybrid (override plus a handleUpdate-only wrapper). Precedent: openai-agents/src/workflow/runner.ts:306-339.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched to this in db052d6. TemporalAgent.stream() catches the three errors and throws ApplicationFailure.create({ message, type, nonRetryable: true, cause: err }), and the Workflow interceptor module and its bundler registration are gone. The update test above checks that the Update fails and the run completes. The handler-timing fixture recorded on 63d7aec now replays, and its test asserts that. The README states the trade-off: code that catches these errors by class around invoke() sees the ApplicationFailure, with the original error as its cause.

await t.throwsAsync(handle.result(), { instanceOf: WorkflowFailedError });
return handle.fetchHistory();
});
const failures = activityFailures(history) as { message: string; type: string; nonRetryable: boolean }[];

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 (tests): the activityFailures helper (122) is annotated object[] although it builds { message, type, nonRetryable }, so this test re-casts it with a non-nullable type, maps message away and checks it with a separate startsWith, three statements where one exact t.deepEqual like its two siblings (898-900, 951-954) would do and would pin the full user-facing message. rewriteError returns exactly the fixed message with type Error for both test inputs, and the sibling assertions already prove t.deepEqual(activityFailures(history), [...]) type-checks. const malformed = { message: 'malformed interrupt: each entry needs a toJSON() that returns an object with a string name, and JSON-encodable values', type: 'Error', nonRetryable: true }; t.deepEqual(activityFailures(history), [malformed, malformed]); and drop the object[] annotation at 122 (or write the record type) so no call site needs a cast.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in db052d6: activityFailures returns typed records, and the malformed test uses one exact t.deepEqual.

@DABH DABH left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addendum to the fourth-round review above, same head 072b20e: a second verification pass finished after I posted and added four items (two confirmed, two plausible) plus one nuance on the malformed-interrupt item; everything else it found was already posted. Severity: P1 = wrong result or stuck run in realistic use; P2 = correctness or robustness in an edge case; P3 = docs, tests, cleanup.

Optional, on the replay fixture: one verifier proposes a fixture-free form of the handler-timing regression test, a probe workflow counting ticks until allHandlersFinished() and asserting withPlugin === withoutPlugin + 1; the fixture is fine as is (SDK 1.25.0 reproduces the identical outcome matrix), so only if you prefer not to carry a recorded history.

Comment thread typescript/strands-agents/README.md Outdated

Other changes when you upgrade:

- A run whose workflow task keeps failing on a Strands terminal error fails with it once this version runs, and a run stuck on a context overflow trims and continues.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: 'A run whose workflow task keeps failing on a Strands terminal error fails with it once this version runs, and a run stuck on a context overflow trims and continues' cannot hold for any run started on the only published prior version, 1.24.0. Such a run has already scheduled invokeModel, so the new strandsGetModelConfig read (this section's own first bullet at 381; TemporalAgent.initialize runs loadConfig before every first model call, temporal-agent.ts:90-93, and the published 1.24.0 has no loadConfig) fails its replay with [TMPRL1100] ... scheduled event invokeModel does not match activity command strandsGetModelConfig at event 5 before the interceptor or the overflow decode can act. Reproduced by replay against the PR bundle for both stuck shapes derived from 1.24 histories (a task loop on MaxTokensError, and a retried then failed invokeModel overflow). Core reports that as another task failure, so the run stays stuck on a different error, never 'fails with it' and never trims; only a reset or termination recovers it, which this section's remedy (let runs finish, or Worker Versioning) cannot do. The body repeats the claim twice; the evidence behind it was recorded on builds after #158, which is unreleased. Qualify the sentence to runs started on a build that already read the model config, and say 1.24 runs stuck this way must be reset or terminated.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Qualified in db052d6: this holds only for runs started on a build that already reads the model config. A 1.24 run stuck this way fails at the config read first, so it needs a reset or termination. The PR body says the same.

type: STRANDS_INTERRUPT_TYPE,
nonRetryable: true,
details,
category: ApplicationFailureCategory.BENIGN,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 (observability, docs): marking interrupts BENIGN also stops the Activity-failure metric, which the body, README and the comment at 53-57 describe only as a logging change, and which the Python twin does not do. common/lib/failure.js:77-80: BENIGN Activities emit DEBUG logs and do not record metrics; the category is encoded into the proto, and sdk-core's should_record_failure_metric skips benign failures (confirmed by the core-bridge 1.24.0 changelog); python/strands_agents/_failure_converter.py:49-56 builds the interrupt error with no category. An operator alerting on temporal_activity_execution_failed loses TypeScript interrupts after the upgrade while Python Workers keep counting them. Say in the Human-in-the-loop paragraph and the body that interrupts are benign failures that are neither logged at WARN nor counted in the metric, and consider the same choice, or the same documentation, for the Python twin.

Nuance on the malformed-interrupt item (65): the second pass also showed live that a malformed interrupt reaches the model as an error tool result and the agent continues (run COMPLETED with the model's follow-up text, zero failed tasks, one WARN), so README 235's 'fails a malformed interrupt without retries' reads as if the run fails. Either rethrow it from TemporalActivityTool so the task fails (recoverable by a code fix, matching the rationale at workflow-interceptors.ts:19-21) or document that it reaches the model as a tool error.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documented in db052d6: the README and the PR body say interrupts are benign failures, logged at DEBUG and not counted in the Activity failure metric. The Python plugin builds its interrupt without a category; I'd leave that for a Python change.

nonRetryable: true,
});
}
return ApplicationFailure.fromError(err, {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 (plausible, hardening): switching from ApplicationFailure.create to ApplicationFailure.fromError mutates an input that is already an ApplicationFailure in place, and if that failure was decoded from the wire its cached .failure proto is re-encoded unconditionally, silently dropping the plugin's type, nonRetryable, details and category overrides. Reproduced in process: a DefaultFailureConverter-decoded ApplicationFailure (for example a WorkflowFailedError.cause from a client call inside the Activity) with an interrupts array attached makes rewriteError return the same instance reporting type StrandsInterrupt, non-retryable and BENIGN, but errorToFailure yields the original type with nonRetryable: false and no details (failure-converter.js:206-207 returns err.failure when set; :190 sets it on every decoded TemporalFailure; failure.js:173-175 mutates in place), so the Worker logs DEBUG while the wire failure is retryable and the Activity is retried. The terminal branch is unreachable this way because ApplicationFailure.prototype.name is fixed. Contrived, but a regression relative to the base's create: after fromError set failure.failure = undefined, or build a fresh failure with ApplicationFailure.create({ ...overrides, cause: err }) when the input is already an ApplicationFailure.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in db052d6: when the input is already an ApplicationFailure, rewriteError builds a fresh failure with the input as its cause, so a decoded failure's cached proto can't hide the overrides.

@@ -0,0 +1,96 @@
import { ContextWindowOverflowError, MaxTokensError, StructuredOutputError } from '@strands-agents/sdk';

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 (plausible, efficiency): the workflow-side interceptor value-imports @strands-agents/sdk through this file only for the instanceof half of the check at line 44 (the err.name fallback already covers the three names), so a bundle built with bundleWorkflowCode({ plugins: [new StrandsPlugin()] }) that contains no Strands workflow still carries and evaluates the whole Strands, MCP and zod graph per workflow instantiation. Measured: a one-line non-Strands workflow bundles to 1.56 MB without the plugin and 7.58 MB with it (121 Strands modules reached only via lib/workflow-interceptors.js); with ReusableVMWorkflowCreator, createWorkflow goes from 0.17 ms to 5.03 ms and retains about 2.6 MiB per cached workflow, because importInterceptors() runs after setActivator(). Nil for bundles that already contain a Strands workflow, and pre-existing for the workflowsPath setup (main's converter value-imported the same classes), so new only for plugin-built bundles without Strands workflows. With name-only matching the same bundle is 1.57 MB; the only behaviour change is a user subclass of one of the three errors that overrides name (none exist in Strands 1.19.0). Round three kept both predicates for the subclass case; this is new evidence on the cost side: match by name only on the workflow side (a Set of the three names, which would also be the single home for the overflow literal at temporal-model.ts:120), keeping instanceof on the Activity side in plugin.ts if wanted, or move the classes behind a type-only import.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moot with the stream() override in db052d6: there's no Workflow interceptor module anymore, so a bundle without Strands workflows doesn't import this. The check runs only inside TemporalAgent, whose workflows already bundle Strands.

…alAgent.stream() (breaking)

A model activity that failed for good, timed out or was cancelled
reached workflow code wrapped in Strands' plain ModelError, which the
SDK does not recognize, so the workflow task failed on every attempt: a
failed model call left the run open and a cancel was ignored.
TemporalAgent.stream(), which invoke() uses, now rethrows the Temporal
failure inside such a ModelError, so the run fails or is cancelled.

The same override turns a Strands terminal error that ends a turn into
a typed, non-retryable ApplicationFailure with the original error as
its cause, so the run, or the update running the turn, fails. It
replaces the workflow interceptor module, whose handler wrappers
changed when signal and update handlers finish and broke replay of some
older runs; a history recorded on main now replays.

Activity side: any error with a non-empty interrupts array is taken as
an interrupt, every entry is checked, and a malformed one fails as
StrandsMalformedInterrupt (exported) with the activity's message.
TemporalActivityTool raises every entry in order. A context overflow on
the plugin's model activities is a benign failure. rewriteError builds
a fresh failure when given an ApplicationFailure, so a decoded one's
cached proto cannot hide the overrides. TemporalModel keeps the
activity failure as the overflow's cause and reads the failure type
from the proto when the converter returns another TemporalFailure.

README: Retries, Streaming, Human-in-the-loop, Observability, pre-built
bundles and the upgrade section follow. Tests: two-entry interrupts,
the plain-entry malformed case, an untrimmable overflow, an update
handler, a cancel during a model activity, a model activity that fails
for good, and the handler-timing history now replays.

Breaking change: workflow code that catches errors from invoke() by
class now sees an ApplicationFailure or ActivityFailure, and a run
whose workflow task kept failing on a failed or cancelled model call
fails or is cancelled once this version runs.
@campoy campoy changed the title fix(strands): rewrite Strands errors with interceptors and keep the caller's failure converter (breaking) fix(strands): keep the caller's failure converter and turn Strands errors into Temporal failures (breaking) Oct 30, 2026
@campoy

campoy commented Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

@DABH thanks for the fourth pass and the addendum. db052d6 covers both; replies are on each thread. The main changes:

  • The P1: fixed here, since it's the same failure class. TemporalAgent.stream() now rethrows the Temporal failure inside Strands' ModelError, so a cancel during a model call (or a tool Activity) cancels the run, and a model Activity that fails for good fails it. Two new tests cover the model cancel and the failed call.
  • Your stream() override: adopted. It replaces the Workflow interceptor, so the handler-timing replay break is gone, and the 63d7aec handler-timing history now replays (the test asserts it). Terminal errors keep the original error as cause.
  • Activity interrupts: we kept the error shape and fixed it. Any non-empty interrupts array counts, every entry is checked, a malformed interrupt has its own type and keeps the Activity's message, and every entry is raised in order. On your note outside the diff (temporal-activity-tool.ts:98): a new test throws two entries, and the agent pauses at each.
  • The addendum's fixture-free test: I kept the fixture. It now guards against wrapping handlers again.

CI is green on db052d6 in all four cells (46 tests). Let me know if anything still blocks an approval.

@DABH DABH left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fifth round, at head db052d6 (one commit since 072b20e: the TemporalAgent.stream() override in place of the Workflow interceptor, the Activity-side interrupt and overflow changes, five new tests and the README follow-ups). In a fresh worktree make sync and make lint are clean. The parallel make test hit 20 s Workflow Task durations on a machine at load 89 on 18 CPUs, the symptom the body describes, in tests this commit does not touch; make test TEST_ARGS=--serial passes 46 of 46, and CI is green in all four cells. I verified findings with in-process probes against Strands 1.19.0 and with Workers on a local dev server. Checked and not raised: the proto fallback in applicationFailureType and the BENIGN category on the overflow both hold with a converter whose failureToErrorInner returns another TemporalFailure, with and without encodeCommonAttributes (only a test gap, inline). The body's open question about the override's extra microtasks turns out to be real, but it adds no exposure for upgrades from 1.24 (inline). Not verifiable here: real providers, the external docs page and sample (acknowledged in the body), and the claim that a run stuck after a cancel request on an unreleased post-#158 build is cancelled once this version runs.

Severity: P1 = wrong result or stuck run in realistic use; P2 = correctness or robustness in an edge case; P3 = docs, tests, cleanup.

Previous rounds. Round four's P1 is fixed: TemporalAgent.stream() rethrows the Temporal failure a ModelError wraps, and the two new tests pass here (a cancel during the model Activity ends CANCELLED; a non-retryable provider failure fails the run with its ActivityFailure). Addressed: the first-entry gate (any non-empty interrupts array counts, every entry is checked, the plain { name, reason } case is in the malformed test, and the interrupt test asserts the first attempt); the split-Worker sentence; the overflow decode no longer depends on the converter class, and the README says so; the overflow keeps the ActivityFailure as its cause, asserted by the new untrimmable-overflow test; the shared CONTEXT_WINDOW_OVERFLOW_TYPE and InterruptDetails; the streaming-topic sentence; the update-handler test; BENIGN on the overflow, scoped to the model Activities, with the History-cost sentence; StrandsMalformedInterrupt keeps the Activity's message and has an exact assertion; the interceptor-order sentence and the advice to list OTel first; the typed activityFailures helper; multi-entry interrupts raised entry by entry, with a test; the 1.24 un-stick qualification; the benign-metrics wording; a fresh failure for an ApplicationFailure input. Superseded by the stream() override: the Strands import graph in non-Strands bundles and the handler-timing break (the 63d7aec fixture now replays). Accepted declines: the JSON oracle (reworded; the Worker-death half is the SDK's), the typed interrupt helper (the shape stays, now validated), not exporting the Activity interceptor, leaving the Python BENIGN choice to a Python change, and keeping the fixture instead of a fixture-free test. Nothing from earlier rounds is open.

Approving: no P1 or blocking P2 remains, every earlier item is addressed or settled, and CI is green. The three items below are non-blocking; the repeated-name check is the one I would still add before merge.

// Route each interrupt through the agent's interrupt() helper, in order: the first one with
// no response throws InterruptError and halts; on resume the answered ones return theirs.
const responses = (details.length > 0 ? details : [{ name: 'interrupt' }]).map((d: Partial<InterruptDetails>) =>
toolContext.interrupt<unknown>({ name: d.name ?? 'interrupt', reason: d.reason as JSONValue })

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: two entries with the same name raise one interrupt, and the second entry silently gets the first one's response, so README 237's 'With several entries, the agent pauses at each one in turn' does not hold and an approval nobody was asked for gets answered. toolContext.interrupt() keys each interrupt as tool:<toolUseId>:<name> (Strands 1.19.0 tools/executors/executor.js:181), and InterruptState.getOrCreateInterrupt returns the existing interrupt for a repeated id (interrupt.js:200-204). The entry's own id from toJSON(), which the new confirmThings fixture sets, is dropped here. Reproduced in process with Strands' real agent and this loop: entries [{ name: 'approval', reason: 'delete a.txt?' }, { name: 'approval', reason: 'delete b.txt?' }] pause once (tool:c1:approval, 'delete a.txt?'). After the answer 'yes to delete a.txt?' the tool result is ["yes to delete a.txt?","yes to delete a.txt?"], the turn ends, and 'delete b.txt?' is never shown. Strands requires names to be unique within one tool execution (InterruptParams.name), and an Activity that lists one entry per item to approve is exactly where a repeated name is likely. interruptDetails already turns unusable entries into StrandsMalformedInterrupt, so add a distinct-names check there (strands-errors.ts:135, for example new Set(details.map((d) => d.name)).size === details.length) and name it in the malformed message. Then say at README 237 that names must be distinct, and add a repeated-name case to the malformed test. Non-blocking.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, good catch. Fixed in cf3b928: interruptDetails now rejects entries that repeat a name, so the Activity fails as StrandsMalformedInterrupt, and the malformed message says each name must be unique. The README says so too, and the malformed test has a repeated-name case.

While in that function I also fixed one more case: an interrupts array with holes, such as Array(1). map and every skip holes, so a hole got through as a valid interrupt, and the Worker then failed to encode its details and stopped. Array.from now visits every slot, so a hole fails as malformed too. The test has a hole-only case and a hole-between-entries case.

Undoing either check fails the malformed test: without the name check the repeated-name case comes back as StrandsInterrupt, and with map in place of Array.from the Worker fails.

options?: InvokeOptions
): AsyncGenerator<AgentStreamEvent, AgentResult, undefined> {
try {
return yield* super.stream(args, options);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: the override's awaits are now part of the replay contract for concurrent turns: it adds microtasks to every agent event, which can change the order in which two turns running at once schedule activities in one Workflow Task. The body lists this as not checked. I checked it on a dev server with a workflow that runs two TemporalAgent turns under Promise.all, both resumed by one Signal in the same activation, as two activity completions in one Workflow Task would be. Agent A's tool does 1500 awaits, then A's next model call schedules markA; agent B's model reply has 20 text blocks before a tool call that schedules markB. With the agent as on main (stream delegating straight to Agent.prototype.stream), the history records markB then markA. At this head it records markA then markB, and each history fails replay on the other build with [TMPRL1100] Nondeterminism error: Activity type of scheduled event 'markB' does not match activity type of activity command 'markA' (or the reverse). Across about 800 combinations of event and await counts only that one diverged, so it takes an unusual imbalance. Every 1.24 agent run already fails replay at the config read, so this adds no exposure for upgrades from 1.24. The cost falls on the next change: removing this override, or adding another layer, would reorder such runs and nothing in the suite would notice. One sentence in this docstring saying its awaits matter for replay would cover it, and the body's 'Not covered' item can then say what was found. Non-blocking.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for checking this. Added a paragraph to the stream() docstring in cf3b928: its awaits are part of replay when two turns run at once, so removing the override or wrapping the stream in another layer can break replay of such runs. The body's "Not covered" item now says what you found.

* the failure's proto when the failure converter returned another kind of
* `TemporalFailure`.
*/
export function applicationFailureType(failure: unknown): string | undefined {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 (tests): the proto fallback here and the BENIGN category on a model-Activity overflow are both documented (README 241 and 113) but untested. Both work. In process, a DefaultFailureConverter subclass whose failureToErrorInner returns a DomainFailure extends TemporalFailure decodes the model Activity's failure so that this function returns ContextWindowOverflowError, with and without encodeCommonAttributes, and the encoded proto carries category: 1. But src/__tests__ has no reference to applicationFailureType, category or a converter that returns anything other than an ApplicationFailure, so either could regress silently (the first back into round three's Workflow Task loop for such converters). To pin both, assert applicationFailureInfo.category on the overflow's ActivityTaskFailed event in the trim test, and run that test once more with a failureConverterPath whose converter returns another TemporalFailure. Non-blocking.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added both in cf3b928. The trim test now checks that the overflow's ActivityTaskFailed event has type ContextWindowOverflowError and category BENIGN, and runs a second time on a Worker-built bundle whose converter (domain-failure-converter.ts) decodes application failures to a TemporalFailure subclass. The README says interrupts are benign too, so the activity-tool interrupt test checks the category as well.

Undoing the code fails the tests: without the BENIGN category the trim and interrupt tests fail, and without the proto fallback the trim test's second run fails.

…t the benign overflow

- `interruptDetails` rejects entries that repeat a name. Strands keys a tool's interrupts by name,
  so the second entry got the first one's response. It also visits the holes in a sparse
  `interrupts` array, which `map` and `every` skip and which stopped the Worker. Both now fail as
  `StrandsMalformedInterrupt`.
- README: interrupt names must be unique.
- `TemporalAgent.stream()` docstring: its awaits are part of replay when two turns run at once.
- Tests: repeated-name and hole cases in the malformed-interrupt test; the interrupt and overflow
  failures are `BENIGN`; the trim test runs again with a converter that decodes application
  failures to another `TemporalFailure`.
@campoy

campoy commented Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the approval, @DABH. cf3b928 covers your three items; each thread has a reply. It also fixes one more interrupt case from our own check: an interrupts array with holes, such as Array(1), stopped the Worker, and now fails as StrandsMalformedInterrupt. Local checks pass, including the parallel make test; CI is running.

@campoy
campoy merged commit 85b968a into main Oct 30, 2026
27 checks passed
@campoy
campoy deleted the fix/strands-typescript-plugin-replaces-caller-failure-converter branch October 30, 2026 22:08
campoy added a commit that referenced this pull request Oct 30, 2026
Conflicts:
- temporal-model.ts: #200's context-overflow rethrow stays around the
  stateless model calls only. A stateful model's overflow is not turned
  into Strands' error, so the default manager the agent keeps does not
  trim and retry a server-side conversation.
- README.md: #200 moved the model config note to "Upgrading from 1.24
  and earlier". With #200, a stateful/stateless mismatch error reaches
  invoke() as an ActivityFailure, so an uncaught one fails the run.
- test-strands.ts and workflows/strands.ts: both sides' imports and
  tests kept.
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.

[Bug] strands-agents (TypeScript): StrandsPlugin replaces the caller's failure converter

3 participants