temporal-spring-ai: add side-effect replay tests for chat, activity tools, and @SideEffectTool#2856
Open
donald-pinckney wants to merge 3 commits intomasterfrom
Open
temporal-spring-ai: add side-effect replay tests for chat, activity tools, and @SideEffectTool#2856donald-pinckney wants to merge 3 commits intomasterfrom
donald-pinckney wants to merge 3 commits intomasterfrom
Conversation
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ools, and @SideEffectTool Three new tests under src/test/.../replay/: - ChatModelSideEffectTest: register a ChatModel with an AtomicInteger counter. Run a workflow that makes one chat call, assert counter=1. Replay the captured history, assert counter still 1 — the activity result comes from history, not from re-invoking the ChatModel. - ActivityToolSideEffectTest: activity-backed @tool whose impl increments a counter. ToolCallingStubChatModel asks for the tool on the first call and returns final text on the second. Same assertion shape: counter=1 after run, counter=1 after replay. - SideEffectToolReplayTest: @SideEffectTool body increments a counter via a file-scope static. Workflow drives a tool call through ToolCallingStubChatModel. The assertion proves that Workflow.sideEffect's marker is what's consulted on replay rather than re-invoking the @tool method. MCP is intentionally omitted — spring-ai-mcp is compileOnly and adding it just for one test isn't worth the dep weight. MCP tool calls go through the same Temporal activity machinery as ChatModel, which ChatModelSideEffectTest already covers. I verified the SideEffectToolReplayTest catches a real regression by temporarily dropping the Workflow.sideEffect wrap in SideEffectToolCallback; the test correctly failed with `expected: <1> but was: <2>`. Restored before this commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Planning scratchpad — not part of the shipped artifact. Removed before merge. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was changed
Three new tests under
src/test/.../replay/:ChatModelSideEffectTest— register aChatModelwith anAtomicIntegercounter. Run a workflow that makes one chat call, assert counter=1. Replay the captured history, assert counter still 1 — the activity result comes from history, not from re-invoking theChatModel.ActivityToolSideEffectTest— activity-backed@Toolwhose impl increments a counter. A tool-calling stubChatModelasks for the tool on the first call and returns final text on the second. Same shape: counter=1 after run, counter=1 after replay.SideEffectToolReplayTest—@SideEffectToolbody increments a file-scope counter. The workflow drives one tool call. Assertion proves thatWorkflow.sideEffect's marker is consulted on replay rather than re-invoking the@Toolmethod.MCP is intentionally omitted for this PR.
spring-ai-mcpiscompileOnly, and adding it just for one replay test isn't worth the dep weight — MCP tool calls go through the same Temporal activity machinery asChatModel, whichChatModelSideEffectTestalready exercises. The plan leaves this as an acceptable substitution.Why?
The existing
WorkflowDeterminismTestcatches history-vs-command mismatches but does not prove the plugin isn't re-invoking user side effects on replay. Temporal's AI partner review standards require side-effect safety tests, and a regression — someone adding an in-workflow cache or dropping theWorkflow.sideEffectwrap — would be easy to slip in without these.I verified the
SideEffectToolReplayTestactually catches a real regression: temporarily dropping theWorkflow.sideEffect(...)wrap inSideEffectToolCallback.call(...)producesexpected: <1> but was: <2>exactly as intended. Restored before this commit.