fix: Auto-route tool call results to messages_history by emincihangeri · Pull Request #1967 · SAP/ai-sdk-js

emincihangeri · 2026-06-25T13:54:45Z

Context

Closes SAP/ai-sdk-js-backlog#471.

What this PR does and why it is needed

Taken over from @emincihangeri by @InjunPark-sap

hyperspace-pr-bot · 2026-06-25T13:55:02Z

Summary

The following content is AI-generated and provides a summary of the pull request:

Fix: Auto-Route Tool Call Results to `messages_history`

Bug Fix

🐛 Tool call results passed via messages could contain {{?...}} patterns from external systems (e.g., search results, API responses) that would be incorrectly interpreted as prompt template placeholders. This fix automatically routes tool messages — and any messages preceding them — to messages_history, bypassing prompt templating entirely while keeping the assistant+tool message pair intact for tool_call_id validation.

Changes

packages/orchestration/src/util/module-config.ts: Replaced the simple routeMessagesToHistory boolean flag with a splitIndex calculation. Messages up to and including the last tool message are routed to messages_history; remaining messages stay in prompt.template. The messagesHistory build logic is simplified to always spread messages.slice(0, splitIndex) after any existing messagesHistory.
packages/orchestration/src/orchestration-completion-post-request.test.ts: Added a tool message auto-routing test suite covering:
- Routing tool messages from messages to messages_history
- Preserving existing messagesHistory when appending tool messages
- Ensuring non-tool messages are unaffected
sample-code/src/orchestration.ts: Added two new sample functions:
- orchestrationToolResultInMessages — demonstrates auto-routing of tool results containing {{?...}} syntax
- orchestrationToolResultMaskingInMessagesHistory — verifies masking is still applied to auto-routed tool results
sample-code/src/index.ts: Exported the two new sample functions.
tests/e2e-tests/src/orchestration.test.ts: Added E2E tests for both new sample functions, asserting successful responses and that masking intermediates are present when expected.

🔄 Regenerate and Update Summary
✏️ Insert as PR Description (deletes this comment)
🗑️ Delete comment

PR Bot Information

Version: 1.26.11

Summary Prompt: Default Prompt
GithubContextProvider: chore(deps): Bump @langchain/core from 0.3.30 to 0.3.31 #471
Event Trigger: pull_request.opened
Correlation ID: 2d37aad0-30a2-477f-8ec5-3d6b4ea7f76d
Output Template: Default Template
File Content Strategy: Full file content
LLM: anthropic--claude-4.6-sonnet

davidkna-sap

Just had a quick look, at a glance it mostly looks good.

hyperspace-pr-bot

The PR introduces auto-routing of tool-call result messages to messages_history to prevent {{?...}} content in tool responses from being misinterpreted as prompt template placeholders. There are two logic issues: the TemplateRef branch now passes an empty [] as messages into moduleRequest instead of undefined (a behavioural change from before), and the unconditional messagesHistory array construction silently changes behaviour for callers that supply only messagesHistory with no tool messages — messages_history is now always emitted even when neither condition previously triggered. Please also address the two existing reviewer comments about findLastIndex and the messagesHistory spread condition.

PR Bot Information

Version: 1.26.11

Correlation ID: 319e6368-3269-4852-83ae-651353e8e144
File Content Strategy: Full file content
Event Trigger: issue_comment.created
LLM: anthropic--claude-4.6-sonnet

hyperspace-pr-bot · 2026-06-25T15:07:46Z

-    routeMessagesToHistory && request
-      ? { ...request, messages: undefined }
+    splitIndex > 0 && request
+      ? { ...request, messages: messages.slice(splitIndex) }


Bug: When usesTemplateRef is true, splitIndex is set to messages.length, and messages.slice(0, splitIndex) equals the full messages array. This means all messages are moved to messages_history, and moduleRequest.messages becomes an empty array (messages.slice(messages.length) → []). However, buildCompletionModulesConfig will then throw 'Either a prompt template or messages must be defined.' if the template is also empty — the same error that existed before this refactor when using a TemplateRef (the old code set messages: undefined). The real regression is that moduleRequest.messages is now [] instead of undefined, and buildCompletionModulesConfig checks !request?.messages?.length — so an empty array still passes that guard safely. But passing an empty messages array into prompt.template is a behavioral change from before (previously messages was undefined so nothing was spread). Verify this is intentional; if not, use messages: undefined for the TemplateRef branch.

Suggested change

? { ...request, messages: messages.slice(splitIndex) }

? { ...request, messages: messages.slice(splitIndex).length ? messages.slice(splitIndex) : undefined }

: request;

Double-check suggestion before committing. Edit this comment for amendments.

Please provide feedback on the review comment by checking the appropriate box:

🌟 Awesome comment, a human might have missed that.

✅ Helpful comment

🤷 Neutral

❌ This comment is not helpful

hyperspace-pr-bot · 2026-06-25T15:07:46Z

+  const messagesHistory = [
+    ...(request?.messagesHistory || []),
+    ...messages.slice(0, splitIndex)
+  ];


Logic Error: When splitIndex === 0 and request?.messagesHistory is non-empty, messagesHistory will be a non-empty array (just the existing history), but the condition messagesHistory.length will correctly include it. However, the existing comment already flags the related issue at line 415 — the suggestion there restores the pre-refactor behaviour of preserving request.messagesHistory even when no tool-message splitting occurs. Without that fix, a call with only messagesHistory (no messages with tool results) will still include messages_history in the output, which is a change from the old behaviour where messagesHistory was only spread when the routing condition was true. This is a silent breaking change for callers that rely on messages_history not being emitted.

Suggested change

];

const messagesHistory =

splitIndex > 0 || request?.messagesHistory?.length

? [...(request?.messagesHistory || []), ...messages.slice(0, splitIndex)]

: undefined;

Double-check suggestion before committing. Edit this comment for amendments.

Please provide feedback on the review comment by checking the appropriate box:

🌟 Awesome comment, a human might have missed that.

✅ Helpful comment

🤷 Neutral

❌ This comment is not helpful

jerome-benoit · 2026-06-26T07:25:34Z

+  // 1. TemplateRef configs — the template lives remotely, so messages cannot be merged in.
+  // 2. Tool results — their content comes from external systems and may contain {{?...}}
+  //    patterns that are not user-defined placeholders. We route through the last tool
+  //    message so the assistant+tool pair stays together for tool_call_id validation.


You are changing the initial intent and semantic of a content behind the scene. The user is not even aware of it and has no way to control it.
What is the rationale behind such a debatable decision?
How can you guarantee that a LLM will behave the same way if a content is moved from a designed container for an intent to another one with another intent?
The whole point of an AI SDK is to interface with an API behind an structured abstraction and not to play with the specified structure of the abstraction.

Message history and prompt messages are joined and both sent to the LLM. The primary impact of moving the messages is that it changes how orchestration processes the messages.

Message history and prompt messages are joined and both sent to the LLM. The primary impact of moving the messages is that it changes how orchestration processes the messages.

Where in the context window? The chronological ordering is altered or not? What is the diff of the context window sent to the LLM with or without it? The placement of content in the context window matters, really matters.

Again, why the API is not fixed properly instead of forcing its consumer to workaround it? It's a simple boolean flag triggering two different code paths.

Assuming the split is done properly, the LLM context window will not change (for this PR I haven't done in-depth checks if this is the case here yet). If this is not the case we may have to re-evaluate doing this, and explore alternatives such as documentation changes, as the service rejected your proposed changes. I would also be hesistant to add escaping to avoid influencing the model with invisible unicode characters.

Anyway in some cases is already message history after consultation with the service.

and explore alternatives such as documentation changes, as the service rejected your proposed changes.

But I think it should be put again on the table by people working directly on AI tooling at SAP in a better way that could lead to a proper fix.

I would also be hesistant to add escaping to avoid influencing the model with invisible unicode characters.

I agree that inserting invisible unicode characters is an uglier workaround than moving the culprit content to a place without template syntax interpretation if the moving it not altering the chronological content in the context window, or at least at a minimal level.

Co-authored-by: David Knaack <david.knaack@sap.com>

- Skip tool-message auto-routing to messages_history when any config has a non-empty prompt.template (static template = user opted into templating, service handles it correctly without interference) - Move lib: [es2023] from root tsconfig.json to packages/orchestration/tsconfig.json to resolve merge conflict with main's tsconfig restructuring - Update unit tests to use a no-template config for auto-routing cases and add a test asserting auto-routing is disabled with static templates - Remove static system prompt from orchestrationToolResultInMessages sample so auto-routing is actually exercised in e2e

prompt.tools (without template) still means the user opted into orchestration's prompt handling — auto-routing tool messages to messages_history in this case leaves messages empty and throws 'Either a prompt template or messages must be defined'. Extend hasStaticPrompt check to cover prompt.tools so that multi-turn tool-calling flows (orchestrationMessageHistoryWithToolCalling) are not broken by the auto-routing logic.

… TemplateRef check

InjunPark-sap · 2026-07-15T09:11:01Z

This CodeQL failure (v4.37.0 config vs v4.36.3 runner) is a pre-existing issue in main unrelated to this PR — it was introduced before this branch was updated.

… service behavior

… behavior

…are routed to history

…l message

InjunPark-sap · 2026-07-15T12:35:04Z

@davidkna-sap
Your feedbacks are applied and also e2e test is passed: "https://github.com/SAP/ai-sdk-js/actions/runs/29415289767"
Please let me know if there is anything to do.

davidkna-sap

Please avoid large-scale changes in the final stages of the review changes without justification, as these may (re)introduce new issues and increases review burden.

I believe the issues raised on 780ad79 could be solved with some minor adjustments. If you disagree I would be open to discuss this.

…l message detection

InjunPark-sap · 2026-07-16T13:44:23Z

Replaced the splitIndex approach with a minimal extension of 780ad79: extracted routing into shouldRouteMessagesToHistory() and added role === 'tool' detection to route all messages to messages_history when tool results are present.

Also fixed two issues found during review:

prompt.template: [] caused service rejection with tools-only configs — now removed when empty
Message duplication when both messages and messagesHistory are provided — now only appended when routing is active

davidkna-sap

Putting this on hold.

The auto-routing is skipped whenever prompt.tools is set, which is exactly the scenario that produces tool call results, so the fix is inert in the real tool-calling flow.

davidkna-sap · 2026-07-17T10:57:29Z

[req] Make sure this is up to date.

davidkna-sap · 2026-07-17T11:02:03Z

+  if (
+    !!request?.placeholderValues &&
+    Object.keys(request.placeholderValues).length > 0
+  ) {
+    return false;
+  }
+
+  if (configs.some(c => isTemplateRef(c?.promptTemplating?.prompt || {}))) {
+    return true;
+  }


[req] Swap these.

davidkna-sap · 2026-07-17T11:04:55Z

[req] Remove duplicate lib, and it is now possible to replace this with target: "ES2024" fully.

@davidkna-sap
Tried target: "ES2024" but the check-public-api CI tool doesn't recognize ES2024 and falls back to ES2021, causing compilation errors in packages/core. Used target: "ES2022" with lib: ["es2024"] as a compromise — removes the duplicate lib and enables ES2024 type definitions. Let me know if there's a preferred approach.

Co-authored-by: David Knaack <david.knaack@sap.com>

InjunPark-sap · 2026-07-17T12:49:01Z

The current fix covers routing for the no-prompt and TemplateRef cases, but you're right that it doesn't address the core tool-calling scenario where prompt.tools is set — routing is skipped there to avoid the service's template required constraint.

Looking forward to the service-side requirement being lifted so we can extend routing to cover that case as well.

emincihangeri and others added 2 commits June 25, 2026 15:51

fix: Auto-route tool call results to messages_history

e6e7467

Merge branch 'main' into investigate/tool-result-prompt-templating

a78b377

emincihangeri changed the title ~~fix: Auto-route tool call results to messages_history~~ chore: Auto-route tool call results to messages_history Jun 25, 2026

chore: Fix lint

02e73cb

davidkna-sap changed the title ~~chore: Auto-route tool call results to messages_history~~ fix: Auto-route tool call results to messages_history Jun 25, 2026

davidkna-sap reviewed Jun 25, 2026

View reviewed changes

Comment thread packages/orchestration/src/util/module-config.ts Outdated

Comment thread packages/orchestration/src/util/module-config.ts Outdated

hyperspace-pr-bot Bot reviewed Jun 25, 2026

View reviewed changes

hyperspace-pr-bot Bot deleted a comment from davidkna-sap Jun 25, 2026

jerome-benoit reviewed Jun 26, 2026

View reviewed changes

emincihangeri and others added 4 commits June 29, 2026 14:10

Update packages/orchestration/src/util/module-config.ts

0170e97

Co-authored-by: David Knaack <david.knaack@sap.com>

chore: Add changeset

9772061

Merge branch 'main' into investigate/tool-result-prompt-templating

4711464

chore: Enable ES2023 array methods in tsconfig

ba033a7

davidkna-sap reviewed Jun 29, 2026

View reviewed changes

Comment thread tsconfig.json Outdated

emincihangeri and others added 3 commits June 29, 2026 15:57

Update tsconfig.json

b733f83

Co-authored-by: David Knaack <david.knaack@sap.com>

fix: address review comments and add message order tests

06831ae

fix: Changes from lint

889db2c

InjunPark-sap requested a review from davidkna-sap June 30, 2026 14:39

davidkna-sap reviewed Jul 2, 2026

View reviewed changes

Comment thread .changeset/yummy-ducks-run.md Outdated

Comment thread tests/e2e-tests/src/orchestration.test.ts Outdated

InjunPark-sap and others added 2 commits July 3, 2026 14:35

Apply suggestions from code review

00deb0c

Co-authored-by: David Knaack <david.knaack@sap.com>

apply feedbacks from david

7c91e68

davidkna-sap requested changes Jul 3, 2026

View reviewed changes

Comment thread tests/e2e-tests/src/orchestration.test.ts Outdated

InjunPark-sap added 2 commits July 6, 2026 13:48

davidkna-sap requested changes Jul 6, 2026

View reviewed changes

Comment thread packages/orchestration/src/util/module-config.ts Outdated

Comment thread packages/orchestration/src/util/module-config.ts Outdated

Comment thread .changeset/yummy-ducks-run.md Outdated

Comment thread packages/orchestration/src/util/module-config.ts Outdated

InjunPark-sap added 4 commits July 6, 2026 17:06

fix: extract split logic, skip routing when placeholder values are set

937d6d2

chore: merge main and resolve tsconfig conflict

7615e45

fix: add missing JSDoc params to getMessageSplitIndex

8f49bad

fix: add lib es2023 to tsconfig.base.json for findLastIndex support

e02c07d

sap-ai-sdk-bot Bot and others added 2 commits July 15, 2026 08:28

fix: Changes from lint

2f1c629

fix: prevent message duplication and respect placeholderValues before…

4ee6f52

… TemplateRef check

InjunPark-sap and others added 9 commits July 15, 2026 12:58

fix: restore prompt template initialization and align unit tests with…

30286d2

… service behavior

fix: Changes from lint

2d73010

fix: restore prompt initialization order and align tests with service…

4b4b4b6

… behavior

fix: allow empty messages array, only throw when messages is undefined

1320ac5

fix: use splitIndex routing to bypass templating for tool results

2de8093

fix: Changes from lint

96c2af2

fix: pass empty messages array instead of removing when all messages …

2f5d648

…are routed to history

fix: omit empty prompt.template when all messages are routed to history

831828a

fix: only route to history when there are messages after the last too…

826f5f6

…l message

InjunPark-sap requested a review from davidkna-sap July 15, 2026 12:35

davidkna-sap requested changes Jul 15, 2026

View reviewed changes

InjunPark-sap added 2 commits July 16, 2026 15:13

fix: replace splitIndex with shouldRouteMessagesToHistory and add too…

76a54cd

…l message detection

fix: fix JSDoc indentation and skip routing when prompt.tools is present

a2a10e0

InjunPark-sap requested a review from davidkna-sap July 16, 2026 13:44

InjunPark-sap self-assigned this Jul 16, 2026

Merge branch 'main' into investigate/tool-result-prompt-templating

3fe1785

davidkna-sap requested changes Jul 17, 2026

View reviewed changes

davidkna-sap marked this pull request as draft July 17, 2026 11:14

davidkna-sap added blocked Issue or PR blocked due to other issues. don't merge labels Jul 17, 2026

InjunPark-sap and others added 4 commits July 17, 2026 14:19

Apply suggestions from code review

cce280b

Co-authored-by: David Knaack <david.knaack@sap.com>

apply feedbacks

73d0fdd

fix: Changes from generation

8de127f

Merge branch 'main' into investigate/tool-result-prompt-templating

cfbaa42

	? { ...request, messages: messages.slice(splitIndex) }
	? { ...request, messages: messages.slice(splitIndex).length ? messages.slice(splitIndex) : undefined }
	: request;

-  ];
+  const messagesHistory =
+    splitIndex > 0 || request?.messagesHistory?.length
+      ? [...(request?.messagesHistory || []), ...messages.slice(0, splitIndex)]
+      : undefined;

Uh oh!

Conversation

emincihangeri commented Jun 25, 2026 • edited by InjunPark-sap Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

What this PR does and why it is needed

Uh oh!

hyperspace-pr-bot Bot commented Jun 25, 2026

Summary

Fix: Auto-Route Tool Call Results to messages_history

Bug Fix

Changes

Uh oh!

davidkna-sap left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hyperspace-pr-bot Bot left a comment

Choose a reason for hiding this comment

Uh oh!

hyperspace-pr-bot Bot Jun 25, 2026

Choose a reason for hiding this comment

Uh oh!

hyperspace-pr-bot Bot Jun 25, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerome-benoit Jun 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

InjunPark-sap commented Jul 15, 2026

Uh oh!

InjunPark-sap commented Jul 15, 2026

Uh oh!

davidkna-sap left a comment

Choose a reason for hiding this comment

Uh oh!

InjunPark-sap commented Jul 16, 2026

Uh oh!

davidkna-sap left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

InjunPark-sap commented Jul 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

emincihangeri commented Jun 25, 2026 •

edited by InjunPark-sap

Loading

Fix: Auto-Route Tool Call Results to `messages_history`

jerome-benoit Jun 26, 2026 •

edited

Loading