fix(agent): preserve multi-round ReAct tool results in message history

[cursor]

Summary

• Multi-round ReAct now strdups each tool result instead of pointing at reused tool_result_bufs, so earlier rounds stay intact in message history.
• ReAct-owned slots (assistant text, tool results, tool_calls copies) are tracked from base_msg_count and freed on cleanup. const tool_calls are released via uintptr_t to compile under -Werror=discarded-qualifiers.
• Reimplemented on current development (includes fix(memory): preserve existing DB when sqlite3_open fails #63). Not a cherry-pick of the original Bot commit.

Test plan

• test_react_loop_preserves_prior_tool_results (red: third chat() saw overwritten tool_output_1; green: two tool rounds then multi tool done)
• make test_agent
• CI=true make test
• make static

Refs: #59

---

Note

Medium Risk
Changes ReAct message ownership and lifetime in the core agent loop; behavior fix is localized but affects every multi-tool-turn run and adds per-round allocations.

Overview
Fixes a bug where multi-round ReAct left tool-result messages pointing at reused tool_result_bufs scratch, so a later tool round could overwrite what the provider still saw in earlier turns.

Each tool output is now strdup'd into heap-owned message content before the conversation grows for the next iteration. ReAct-added slots (assistant text, tool results, copied tool_calls) are tracked from base_msg_count and released in agent_run_cleanup via agent_free_heap_messages, replacing per-iteration prev_assistant / prev_calls frees. Tool-call teardown uses a uintptr_t cast so free_tool_calls_copy stays valid under const and -Werror=discarded-qualifiers.

Adds test_react_loop_preserves_prior_tool_results (two tool rounds; third chat() must still see tool_output_1 and matching tool_use_id). Changelog [Unreleased] notes the fix (Refs: #59).

^{Reviewed by Cursor Bugbot for commit c7a0b13. Bugbot is set up for automated code reviews on this repo. Configure here.}

[cursor]

Stale comment

Could not approve: GitHub rejects self-approval on this Cursor-authored PR. Cursor Bugbot and Cursor Security Agent completed successfully with no findings that need human review. adriannoes is already assigned; no additional reviewers were assigned.

  

_{Sent by Cursor Approval Agent: Pull Request Router and Approver}

[cursor]

Summary

This is a correct, tightly scoped fix for a real ReAct bug. Tool-result messages used to alias tool_result_bufs, so round N overwrote what the model still needed from round 1. The loop now strdups each result, records ReAct-owned slots from base_msg_count, and frees them in agent_run_cleanup. Dropping prev_assistant / prev_calls is not incidental: those frees ran while the pointers were still in the message list (UAF of assistant text, tool_calls, and tool_use_id). Local CI=true make test_agent passed, and a GCC ASan+UBSan rebuild of test_agent passed with leak detection on.

Must Fix

None.

Should Fix

• test_react_loop_preserves_prior_tool_results only searches messages[i].content for tool_output_1 (tests/test_agent.c 608–618). It never reads tool_calls or tool_use_id. Production Anthropic and OpenAI body builders dereference both on later chat() calls. Assert those pointers are still live on the third chat() so the UAF half cannot regress behind a green content check.

Nice to Have

• The call_count >= 2 content check also runs on the second chat(), which is before the second execute overwrites scratch. Tightening it to call_count >= 3 makes the test’s intent obvious.
• tool_use_id is an alias of our_calls[k].id (src/core/agent.c 551). agent_free_heap_messages frees the assistant’s tool_calls first, then walks tool-result slots with a dangling tool_use_id. Harmless today because teardown does not dereference it. Clearing tool_use_id in that loop, or giving results their own copy of the id, would make teardown order-safe.
• multi_tool_round_init is never called (agent_run does not invoke provider->init). Reset the counters at the start of the test function if you keep the helper. Same pattern as the older ReAct tests.

Positive Highlights

• Ownership is explicit: scratch stays scratch, history gets heap copies, and base_msg_count keeps system/history/user pointers out of free.
• The mid-round strdup OOM path (src/core/agent.c 540–547) frees only the new slot’s copies and leaves ctx->messages on the previous array, so cleanup cannot double-free.
• Combining response.content ? strdup(response.content) : strdup("") into one OOM check is cleaner than the old empty-string fallback that could skip the failure path.
• CHANGELOG [Unreleased] includes the fix in the same PR.

  

_{Sent by Cursor Automation: Adrianno’s personal code review}

[cursor]

Could not approve: GitHub rejects self-approval on this Cursor-authored PR. Cursor Bugbot and Cursor Security Agent completed successfully with no findings that need human review. adriannoes is already assigned; no additional reviewers were assigned.

  

_{Sent by Cursor Approval Agent: Pull Request Router and Approver}