Serialize assistant tool_calls and tool-result name on the OpenAI-compat path

[malibio]

Summary

OpenAiMessage only serialized role/content/tool_call_id. When the ReAct loop replays an assistant turn that issued tool calls, that turn has content: "" plus structured tool calls tracked separately — so on the OpenAI-compat path the replayed history became {"role":"assistant","content":""} followed by a {"role":"tool","tool_call_id":...} message with no preceding tool_calls to justify it, and the tool message's name field was dropped too.

• Strict servers (real OpenAI API, recent vLLM): round 2 of any tool-using conversation gets HTTP 400 (messages with role "tool" must be a response to a preceding message with "tool_calls") — every tool-using conversation hard-fails right after the first tool executes.
• Lax servers (Ollama /v1, LM Studio): the malformed shape is accepted, but the model can never see which tool it called or with what arguments, degrading multi-step tool use.

Fix

• Added tool_calls: Option<Vec<OpenAiRequestToolCall>> and name: Option<String> to OpenAiMessage, mirroring the OpenAI wire contract ({id, type: "function", function: {name, arguments}}).
• Extracted the inline mapping closure into to_openai_message(&ChatMessage) -> OpenAiMessage, now populating both new fields from the internal ChatMessage's existing (previously untapped) tool_calls/name fields.
• Scope kept tight to the serialization shape only, per the issue.

Test plan

• New unit tests (primary evidence, no network) on the outgoing shape:
  • a replayed assistant tool-call turn serializes tool_calls with correct id/type/function.name/function.arguments
  • a tool-result message serializes name
  • a plain text turn omits both spurious fields
• cargo test -p nodespace-agent --lib — 545 passed, 0 failed
• live_openai_compat_smoke.rs against local Ollama: reproduces an unrelated pre-existing failure (mistral:7b on this Ollama install returns a degenerate empty response — confirmed via raw curl bypassing all NodeSpace code, and confirmed byte-identical on unmodified main before this change). llama3.1:8b on the same Ollama server tool-calls correctly. Not a regression from this change.
• bun run test:all — full suite passes (frontend, scripts, skill, Rust)
• NODESPACE_PROMPT_DUMP used to confirm the outgoing request shape and that no API key is written to the dump

Strict-server (real hosted provider) confirmation is out of scope here — that belongs to #2189, which this issue unblocks.

Closes #2198

🤖 Generated with Claude Code

[malibio]

Code Review — PR #2236 (initial review)

Verdict: APPROVE. (Posted as a comment; GitHub blocks self-approval.)

Summary

A tight, correctly-scoped protocol fix. OpenAiMessage gains the two fields the OpenAI wire contract requires (tool_calls on assistant turns, name on tool results), the inline closure becomes a named to_openai_message function, and three unit tests pin the serialized shape. 118 insertions in a single file, no collateral changes, no new dependencies.

I verified the fix rather than reading it only:

• cargo test -p nodespace-agent --lib openai_compat — 16 passed, including all three new tests.
• cargo clippy -p nodespace-agent --all-targets — clean, zero warnings.
• cargo test -p nodespace-agent --test live_openai_compat_smoke -- --ignored against local Ollama — passed (5.37s, tool calls: ["get_weather"], discovery returned 4 models). AC Enhanced BaseNode with Hierarchical Circle Indicators + TextNode Implementation #4 confirmed empirically, not assumed.

Acceptance criteria

Criterion	Status
OpenAiMessage carries tool_calls on assistant messages	Met — openai_compat_inference.rs:152-156, with id/type: "function"/function.{name,arguments}
tool message's name preserved	Met — :150-151, populated from ChatMessage::name
Unit test on the serialized assistant-tool-call + tool-result pair	Met — three tests at :620-684
live_openai_compat_smoke.rs still passes against Ollama	Met — run above
No API key or hosted provider needed	Met — every check ran locally

What the change gets right

The shape is genuinely correct, not merely plausible. type: "function" is emitted as a literal, arguments stays a raw JSON string (not a re-parsed object) — matching what OpenAI actually specifies and what ToolCallRaw.arguments_json already holds. Re-parsing here would have been the tempting, wrong move.

Conditional emission is right. (!msg.tool_calls.is_empty()).then(|| …) combined with skip_serializing_if = "Option::is_none" means a plain user turn emits neither field. I checked the risk that name could leak onto a user/assistant message, where OpenAI interprets it as a participant name with different semantics: ChatMessage.name is set in exactly one place — ChatMessage::tool_result (packages/nlp-engine/src/chat/types.rs:182) — so tool-role messages are the only ones that can carry it. The third test locks that in.

Extraction to a named function is the right call. to_openai_message is now unit-testable without an HTTP server, which is what makes AC #3 satisfiable at all. This is the difference between a test that asserts the wire shape and one that asserts an inline closure it cannot reach.

The fix connects to its producer. I traced the history construction: agent_loop.rs:1873 pushes assistant_with_tool_calls(String::new(), tool_calls) and :2230 pushes tool_result(msg, tc.id, tc.function_name). The serializer consumes exactly those fields, so the unit test's synthetic input matches production's real input — the tests are not testing a shape the loop never produces.

Test naming and comments earn their place. replayed_assistant_tool_call_carries_tool_calls_on_the_wire states the invariant, and each comment explains the failure mode ("must be a response to a preceding message with tool_calls") rather than restating the assertions.

Findings

Nothing blocking. Two observations, both optional.

[Nit] packages/agent/src/local_agent/openai_compat_inference.rs:13-14 — #[cfg(test)]-gated import.

#[cfg(test)]
use crate::agent_types::{Role, ToolCallRaw};

This works and is warning-free, but a test-only import at module scope is slightly unusual — the conventional home is inside mod tests (which already has use super::*;, so use crate::agent_types::{Role, ToolCallRaw}; there would need no cfg attribute at all). Purely cosmetic; the current form compiles identically and I would not hold a merge for it.

[Nit] Assistant tool-call turns serialize content: "" rather than omitting it.

agent_loop.rs:1873 deliberately persists empty content (the comment there explains why: dropping the model's narration keeps the replayed turn structurally clean), and OpenAiMessage.content is a non-optional String, so the wire payload carries "content": "". The OpenAI spec permits content to be null on an assistant message that has tool_calls, and empty-string is accepted by the real API and by vLLM — so this is not a defect and I am explicitly not asking for a change. Flagging only as a known coordinate: if #2189's real-key run against a hosted provider ever surfaces a complaint about assistant content, this is the line to look at, and the fix would be making the field Option<String> with skip_serializing_if. Speculating further would be YAGNI; the empirical run in #2189 is the right place to learn whether it matters.

Scope and process

Atomic and single-purpose — one struct, one extracted function, three tests, no drive-by refactoring. No lint suppression, no #[allow(dead_code)], no #[deprecated], no backward-compat shim. Correct for a greenfield pre-release codebase.

Correctly scoped out: strict-server end-to-end confirmation stays in #2189, and the issue is explicit that a green Ollama run proves no regression, not the fix — the PR's evidence structure honors that distinction rather than overclaiming from the smoke test.

No security surface: no user input parsing, no new network behavior, no secrets, no error-path data exposure. No performance concern — the per-message clones were already present in the code this replaces, and the allocation count is unchanged for non-tool turns.

Recommendation

Approve and merge. This is a clear net improvement to code health, verified rather than asserted, and it unblocks #2189.

[malibio]

Verdict: APPROVE. Submitted as a comment because GitHub blocks self-approval on an authored PR; the full review is in the preceding comment.

Verified locally: 16/16 unit tests pass (including the 3 new wire-shape tests), clippy clean, and the live Ollama smoke test passes — confirming AC #4 empirically. All five acceptance criteria met. Two nits only, both optional and non-blocking.

[malibio]

Additional verification beyond the unit tests: ran a real two-turn tool-calling conversation through the actual OpenAiCompatInferenceEngine against local Ollama (llama3.1:8b, which reliably tool-calls — mistral:7b on this Ollama install currently returns degenerate empty responses even via raw curl, unrelated to this change and reproduces identically on unmodified main).

• Turn 1: real get_weather tool call from the model.
• Turn 2: replayed that history (assistant tool-call turn + tool result) through the engine with the fix applied — the exact shape agent_loop sends in production.
• Ollama accepted the request and the model correctly used the tool result: "The current temperature in Paris is 22°C and it's a sunny day" (matching the injected fake data, not hallucinated).

NODESPACE_PROMPT_DUMP confirms the exact outgoing JSON for turn 2:

[
  {"role": "user", "content": "What is the weather in Paris right now? Use the tool."},
  {
    "role": "assistant", "content": "",
    "tool_calls": [{"id": "call_e5l5jzrv", "type": "function", "function": {"name": "get_weather", "arguments": "{\"city\":\"Paris\"}"}}]
  },
  {"role": "tool", "tool_call_id": "call_e5l5jzrv", "name": "get_weather", "content": "{\"temperature_c\": 22, \"condition\": \"sunny\"}"}
]

This matches the OpenAI wire contract from the issue exactly. The verification script was a throwaway test file, not committed — the shape is what the new unit tests already assert in isolation; this confirms it also works end-to-end against a live server.

[malibio]

Address Review — PR #2236

Both findings from the initial review were explicitly non-blocking (reviewer: "I would not hold a merge for it" / "I am explicitly not asking for a change"). Addressed one, deferred the other per the reviewer's own recommendation:

✅ Addressed — [Nit] #[cfg(test)]-gated import: moved use crate::agent_types::{Role, ToolCallRaw}; from module scope into mod tests (now covered by the existing use super::*;), matching the codebase's conventional test-import placement. Purely cosmetic, zero behavior change. Commit 5ecfa40.

⏭️ Skipped — content: "" vs null on assistant tool-call messages: reviewer confirmed this is spec-compliant (OpenAI permits content: null when tool_calls is present, but empty-string is also accepted by the real API and vLLM) and explicitly recommended deferring any change until #2189's live hosted-provider run surfaces an actual complaint. Acting now would be speculative — YAGNI.

Verified after the change: cargo test -p nodespace-agent --lib openai_compat — 16/16 passed. cargo fmt --check and cargo clippy -- -D warnings both clean. bun run test:all passed (one core-plugins.test.ts timeout flake on the first two push attempts — documented pre-existing flake, unrelated to this file, resolved on retry with no code change).

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: The only change since the approved review is a one-line import relocation with no logic, behavior, or test-coverage change — confirmed by an identical 16/16 test pass and clean clippy/fmt. The second finding was deliberately left as-is per the reviewer's own guidance. Nothing here could plausibly change the APPROVE verdict.