feat!: copy middleware context inputs to prevent accidental mutation

[zastrowm]

Description

Middleware previously received live references to the agent's internal state. A middleware that accidentally mutated the messages array, a toolSpec, or modelState would corrupt agent state for subsequent turns.

This PR makes middleware context inputs defensive copies across all three stages:

InvokeModelStage:

• messages: deep copy via Message.clone()
• systemPrompt: deep copy via cloneSystemPrompt()
• toolSpecs / toolChoice: deep copy via deepCopy()
• modelState: removed from context — snapshotted before the chain and written back after it completes
• invocationState: shared by reference (hooks and tools write to it during streaming)

ExecuteToolStage:

• toolUse: deep copy (prevents mutation of the parsed tool input in the assistant message)
• invocationState: shared by reference

AgentStreamStage:

• args: shallow copy when array (prevents push/splice on the caller's array)
• options: shared by reference (contains non-cloneable objects and shared invocationState)

Related Issues

Documentation PR

Design rationale documented in strands-ts/src/middleware/README.md.

Type of Change

Breaking change

Breaking Changes

modelState has been removed from InvokeModelContext. Middleware that previously read or wrote context.modelState will fail to compile.

Migration

// Before
agent.addMiddleware(InvokeModelStage, async function* (context, next) {
  const id = context.modelState.get('responseId')
  return yield* next(context)
})

// After — use the agent escape hatch if you need to inspect model state
agent.addMiddleware(InvokeModelStage, async function* (context, next) {
  const id = context.agent.modelState.get('responseId')
  return yield* next(context)
})

Note: writes to agent.modelState within middleware will be overwritten by the post-chain writeback. This is intentional — model state is provider-internal bookkeeping.

Testing

23 tests in copy-on-input.test.ts covering:

• Messages deep copy (array reference, content blocks, metadata isolation)

• Array systemPrompt deep copy; string passthrough

• toolSpecs fresh copy per invocation

• toolChoice deep copy when present (structured output)

• modelState excluded from context; model writes sync back; middleware mutations discarded

• invocationState shared by reference with mutations visible on result

• ExecuteToolStage toolUse.input isolation

• AgentStreamStage args array isolation

• Functional style: passing modified context to next()

• I ran hatch run prepare

Checklist

• I have read the CONTRIBUTING document
• I have added any necessary tests that prove my fix is effective or my feature works
• I have updated the documentation accordingly
• I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
• My changes generate no new warnings
• Any dependent changes have been merged and published

---

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[github-actions]

Assessment: Comment

Solid, well-tested defensive-copy change with a clear design rationale documented in the middleware README and PR description. The snapshot/writeback approach for modelState is a clean way to enforce the isolation contract. Two items are worth addressing before merge: an incomplete deep-copy in Message.clone() (metadata is shared by reference, which leaks through the isolation boundary), and the public-API process flag for removing modelState from InvokeModelContext.

Review Categories

• Correctness: Message.clone() shares metadata (incl. nested custom) by reference, partially defeating the isolation guarantee this PR establishes.
• Public API: InvokeModelContext is an exported type; the modelState removal should carry api/needs-review per the bar-raising process. The design itself is well-argued.
• Behavior: modelState writeback is skipped if the model call throws — a subtle change from the previous live-store behavior; worth confirming intent.
• Testing: Excellent coverage of messages/systemPrompt/toolSpecs/modelState; gaps remain for defined-toolChoice copying and message-metadata isolation. Minor: prefer whole-shape key assertion over not.toContain.

Nice work isolating middleware from agent internals — the functional-style test cases passing modified contexts to next() are a great touch.

[github-actions]

Issue (Suggestion): The AgentStreamStage copy of args is shallow ([...args]), so push/splice on the array is isolated (nicely tested), but the Message objects inside the array are still shared by reference with the caller. A middleware that mutates context.args[0].content (or a block's fields) would leak back into the caller's Message — the same class of accidental-mutation bug this PR closes elsewhere.

This is also inconsistent with InvokeModelStage, which deep-copies each message via msg.clone().

Suggestion: Decide whether this is intentional and either:

• deep-copy the messages here too (e.g. args.map((m) => m.clone()) when the array holds Messages) for consistency with InvokeModelStage, or
• if shallow-by-design (perf / args may hold non-Message entries), document it alongside the invocationState/modelState sections in middleware/README.md and add a test asserting the shallow boundary (mutating context.args[0] does surface, so callers know the contract).

Minor, since args is the raw caller input rather than agent-internal state — but worth a deliberate call given the rest of the PR's isolation guarantees.

[github-actions]

Re-review (round 2) — Assessment: Approve (one minor suggestion)

All five items from the previous review have been addressed, and the PR's scope has grown thoughtfully (now feat!: covering all three stages, linked to #2469, with a documented Breaking Changes section and the api/needs-review label applied).

Prior findings — all resolved ✅

• Message.clone() metadata — now deep-copies metadata via JSON round-trip, with a dedicated isolation test.
• API process — api/needs-review label added; breaking change documented.
• modelState writeback-on-error — now has an explicit comment confirming skip-on-error is intentional.
• toolChoice deep-copy — defined-toolChoice isolation test added.
• Whole-shape key assertion — adopted (Object.keys(context).sort() over not.toContain).

Verification

Checked out the PR head and ran the suites locally: copy-on-input.test.ts (24 tests) ✅, plus agent.stateful-model + agent-middleware (71 tests) ✅ — no type errors.

One new minor suggestion on the expanded scope: AgentStreamStage copies args shallowly, so Message objects inside the array remain shared by reference (inline comment on agent.ts). Worth a deliberate decision for consistency with InvokeModelStage, but non-blocking.

Nice iteration — the three-stage isolation story is now coherent and well-tested.

[notowen333]

would it be more or less organized to write these cloning utilities in middleware/ instead of inline

[zastrowm]

I tried it, but it just obscures the code IMO

[notowen333]

"and the SDK should never make mutations directly"

[notowen333]

"and the SDK should never write to it"

[zastrowm]

Fair, though this is larger than just middleware IMO

[opieter-aws]

The same toJSON() reference-sharing problem as flagged by the bot still affects ToolUseBlock.input and JsonBlock.json. Why not do a full JSON round-trip on the entire MessageData?

[zastrowm]

I have to look more into this; it's either a miss that that metadata isn't part of toJSON() and fromMessageData OR metadata isn't normally copied and it's just in this case that we need it

[zastrowm]

Yeah, switching to doin ga deep copy of the entire thing to avoid special casing metadata

[github-actions]

Confirming this empirically — I checked out HEAD (07cb13f) and ran a probe: cloning a Message with a ToolUseBlock, then mutating clone.content[0].input.nested.a, also mutates the original (1 → 999). So clone()'s targeted metadata round-trip closes only one leak; ToolUseBlock.input and JsonBlock.json still pass through toJSON() as live references. The full-MessageData round-trip you suggest fixes all three at once and removes the metadata special-case — agree that's the cleaner direction.

One thing to flag when that fix lands: the existing "message content blocks are deep copied" test only asserts receivedContent).not.toBe(...) (instance identity), which already passes today despite the nested-input leak — so it won't guard against regressions here. Worth upgrading it to mutate a nested field of a ToolUseBlock.input and assert the original is unchanged (same shape as the metadata-isolation test).

For the record, this also corrects my round-2 summary, which prematurely marked the clone() issue fully resolved — the metadata fix was real, but the underlying toJSON() reference-sharing wasn't.

[opieter-aws]

Is this true for the InvokeModelStage, given that we do { ...invocationState }?

[opieter-aws]

I think that'd break invocationState changes that any ModelStreamUpdateEvent and ContentBlockEvent hooks make

[opieter-aws]

These copies run on every model call even when no InvokeModelStage middleware is registered. Consider gating this, since it introduces latency as the conversation history grow

[zastrowm]

going to defer this until we see performance problems. In general and compared to network requests, this is trivial and I think code clarity is more important until we're seen impact here

[github-actions]

Re-review (round 3) — Assessment: Request Changes (one correctness item)

Catching up after @opieter-aws and @notowen333's review and the latest push (07cb13f9). The invocationState-shared-by-reference cleanup across all three stages reads well and resolves the { ...invocationState } inconsistency raised on the README thread.

One correctness item should block merge, and I want to correct my own round-2 summary on it:

Correction to round-2: clone() deep-copy is still incomplete

My round-2 note marked the Message.clone() issue fully resolved — that was premature. The targeted metadata round-trip is real, but ToolUseBlock.input and JsonBlock.json are still shared by reference because toJSON() returns them live. I confirmed empirically on HEAD: mutating a clone's toolUse.input.nested field also mutates the original. @opieter-aws flagged the same on messages.ts:129, and @zastrowm has agreed to switch to a full MessageData round-trip — tracking that as the fix.

Test gap to close alongside the fix

The existing "content blocks are deep copied" test only asserts instance identity (not.toBe), which passes today despite the nested-input leak. It should be upgraded to a nested-mutation assertion (mirroring the metadata-isolation test) so this can't regress.

Remaining open: my args shallow-copy thread (agent.ts:1115) is still awaiting a decision (deep-copy vs. document-the-boundary). The perf-gating thread is reasonably deferred. Once the clone() round-trip lands with a strengthened test, I'd flip to Approve.

[github-actions]

Re-review (round 4) — Assessment: Request Changes (one item remaining)

Reviewed cba6b85d. The args thread is resolved cleanly: removing the shallow copy and treating args/options as shared-by-reference (with the README section + JSDoc Shared by reference notes) is a reasonable, deliberate call — the non-cloneable-objects rationale (Zod schemas, AbortSignals) is sound, and it keeps the contract consistent with invocationState. Ran the suite locally: 21 tests pass, no type errors.

One blocker still stands — Message.clone():

Still reproduces on cba6b85d

This commit doesn't touch messages.ts, so the nested-reference leak is unchanged. Re-confirmed on this HEAD: cloning a Message with a ToolUseBlock, then mutating clone.content[0].input.nested.a, still mutates the original (999). The full-MessageData round-trip you agreed to (thread) will close it — plus upgrading the "content blocks are deep copied" test to a nested-mutation assertion so it can't regress.

That's the only thing between this and Approve — everything else is in good shape.

[github-actions]

Re-review (round 5) — Assessment: Approve ✅

Reviewed 8f5766a8. The clone() fix is exactly the agreed direction — Message.fromMessageData(deepCopy(this.toJSON())) does a full MessageData round-trip, dropping the metadata special-case and closing the ToolUseBlock.input / JsonBlock.json reference leaks in one shot.

Verified locally on this HEAD:

• The leak probe that failed in rounds 3–4 (mutate clone.content[0].input.nested.a → original) now passes — the original is untouched.
• Full suite green: 97 tests pass (copy-on-input 21 + messages 76), no type errors.

All blocking items are resolved. The args boundary is documented, invocationState sharing is consistent across stages, and clone() is now a true deep copy. Nice work iterating through this.

One optional, non-blocking follow-up

The "message content blocks are deep copied" test (copy-on-input.test.ts:48) still asserts only instance identity (not.toBe). The code is correct now, but that assertion would still pass if a future change reintroduced a nested-input leak. Upgrading it to a nested-mutation assertion (mirroring the metadata-isolation test right below it) would guard the regression — worth doing whenever convenient, not a merge blocker.