bytechefhq / bytechefhq/bytechef

[feature] Conditionally place chat-memory advisor inside the tool loop when the memory type supports persisting tool messages

Open
#5,349 1 comment 0 reactions 1 assignee Claimed by @ivicac View on GitHub
ai backend enhancement
Dominant language
Java
Stars
1k
Forks
170
Avg merge
11h 25m
Merged PRs (30d)
115

Description

## Summary

Conditionally place the chat‑memory advisor **inside** the `ToolCallingAdvisor` tool loop when — and only when — the selected chat‑memory type supports persisting tool messages (the full tool request/response transcript). For memory types that don't, keep the current **outside‑the‑loop** placement.

Reference: Spring AI blog — [Composable Tool Calling → "Memory and the tool loop"](https://spring.io/blog/2026/06/15/spring-ai-composable-tool-calling#memory-and-the-tool-loop).

## Background

Since the migration to composable tool calling (Spring AI 2.0.0), we build the `ToolCallingAdvisor` manually in `AbstractAiAgentChatAction.getAdvisors(...)` so we can wrap the manager in `SuspendableToolCallingManager` for suspend/resume:

`server/libs/modules/components/ai/agent/src/main/java/com/bytechef/component/ai/agent/action/AbstractAiAgentChatAction.java:453`

```java
advisors.add(
ToolCallingAdvisor.builder()
.toolCallingManager(
new SuspendableToolCallingManager(toolCallingManager, (ActionContextAware) context))
.build());
```

The chat‑memory advisor is added just before it, using each memory component's default order:

- `MessageChatMemoryAdvisor` default order = `-2147483448` (`DEFAULT_CHAT_MEMORY_PRECEDENCE_ORDER`, `MIN+200`)
- `ToolCallingAdvisor` default order = `-2147483348` (`ToolCallingAdvisor.DEFAULT_ORDER`, `MIN+300`)

Because memory's order (`MIN+200`) is lower/higher‑precedence than the tool loop (`MIN+300`), the memory advisor always sits **outside** the loop today. That is correct and safe — but it means memory persists **only the final user/assistant exchange**; tool request/response messages are never written to the store. The model therefore cannot recall, on a later turn, which tools it called and what they returned.

## Current vs. desired behavior

**Current (outside the loop, always):**
- Loads history once before the loop; persists only final user + assistant messages.
- Matches Spring AI 1.x behavior; safe for all memory implementations.
- No `disableInternalConversationHistory()` needed (correct as‑is).

**Desired (conditional):**
- If the memory type declares it can persist tool messages, place its advisor **inside** the loop (order `> ToolCallingAdvisor.DEFAULT_ORDER`) so it captures the full tool transcript, giving the LLM richer cross‑turn context.
- When inside, the manually built `ToolCallingAdvisor` **must** call `.disableInternalConversationHistory()` to avoid double‑writing the transcript (both histories otherwise record the same intra‑turn messages). Note: the auto‑registered `ToolCallingAdvisor` handles this automatically, but we register it manually, so we must call it explicitly.
- Otherwise, keep the current outside‑the‑loop placement.

## Which memory types qualify

Per the blog, only some repositories persist tool messages safely inside the loop. Built‑in safe options: `InMemoryChatMemoryRepository`, `RedisChatMemoryRepository`, `Neo4jChatMemoryRepository`. JDBC‑backed memory needs the Spring‑AI‑Session community project.

Our chat‑memory components (`server/libs/modules/components/ai/agent/chat-memory/`):

| Component | Likely supports tool messages inside loop? |
|---|---|
| `chat-memory-in-memory` | Yes |
| `chat-memory-redis` | Yes |
| `chat-memory-neo4j` | Yes |
| `chat-memory-jdbc` / `chat-memory-builtin` | Needs verification (Spring‑AI‑Session) |
| `chat-memory-mongodb` / `chat-memory-cassandra` | Needs verification |
| `chat-memory-aws` | Needs verification |
| `chat-memory-vectorstore` | Separate advisor type (`VectorStoreChatMemoryAdvisor`) — verify semantics |
| `chat-memory-session` | Needs verification |

Each component owns its own advisor build site, so capability must be declared per component rather than centrally inferred.

## Proposed approach (sketch — open to design)

1. Add a capability flag to `ChatMemoryFunction.Result` (e.g. `boolean supportsToolMessagePersistence`) that each memory component sets. Default `false` to preserve today's behavior.
2. In `AbstractAiAgentChatAction.getAdvisors(...)`:
- If the flag is `true`, add the memory advisor with an order **greater than** the tool advisor's order (i.e. `> ToolCallingAdvisor.DEFAULT_ORDER`, or `> advisorOrder` if we ever override it), and call `.disableInternalConversationHistory()` on the `ToolCallingAdvisor` at line 453.
- Else keep the current placement/behavior unchanged.
3. Prefer adjusting the **memory advisor's** order over the tool advisor's order, so we don't disturb guardrail ordering (guardrails sit at `HIGHEST_PRECEDENCE` and `DEFAULT_CHAT_MEMORY_PRECEDENCE_ORDER - 1`). Note `MessageChatMemoryAdvisor` is immutable, so the order must be set at build time in each component (or the advisor rebuilt from `Result.chatMemory()` where the type allows).

## Risks / things to verify

- **Suspend/resume:** interaction with `SuspendableToolCallingManager` and `ConversationResume` — confirm that persisting intermediate tool messages doesn't conflict with the serialized `ConversationState` on resume.
- **Token/storage cost:** inside‑the‑loop persistence stores the full transcript → larger memory rows and more replayed tokens. Consider making it opt‑in per component only where it adds value.
- **`disableInternalConversationHistory()` pairing is mandatory** whenever a memory advisor goes inside the loop; missing it → duplicate writes.

## Acceptance criteria

- [ ] `ChatMemoryFunction.Result` (or equivalent) exposes whether the memory type persists tool messages.
- [ ] Memory components that support it (at least in‑memory/redis/neo4j) declare the capability; others remain `false`.
- [ ] `getAdvisors(...)` places supporting memory advisors inside the loop and calls `disableInternalConversationHistory()`; non‑supporting ones stay outside.
- [ ] No duplicate persistence for the inside‑the‑loop path (verified by test asserting a single write of tool messages).
- [ ] Existing outside‑the‑loop behavior is unchanged for non‑supporting memory types (regression test).
- [ ] Suspend/resume continues to work with an inside‑the‑loop memory type.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.