Skip to content

fix(agent): emit function_call items from fromChatMessages and return Item[] (#11, #41) - #93

Draft
LukasParke wants to merge 1 commit into
mainfrom
fix/11-41-from-chat-messages
Draft

fix(agent): emit function_call items from fromChatMessages and return Item[] (#11, #41)#93
LukasParke wants to merge 1 commit into
mainfrom
fix/11-41-from-chat-messages

Conversation

@LukasParke

Copy link
Copy Markdown
Contributor

Two bugs in the same function, fromChatMessages — one runtime, one type-level.

Fixes #11
Fixes #41

#11 — assistant toolCalls were silently dropped

The assistant branch only read msg.content and never msg.toolCalls:

if (isAssistantMessage(msg)) {
  return { role: mapChatRole('assistant'), content: contentToString(msg.content) };
}

A tool-calling assistant message conventionally carries content: null, so this produced { role: 'assistant', content: '' } — the tool call vanished, and the following function_call_output items were left orphaned with no call to answer. Any agentic loop replayed through this helper lost its tool calls.

The .map() callback's declared return type (EasyInputMessage | FunctionCallOutputItem) also structurally prevented emitting function_call items at all, and .map() is 1:1 while one chat message can fan out to several items.

Converted to an accumulator loop mirroring fromClaudeMessages: push a message item, then one function_call item per toolCalls entry.

Two details worth flagging for review:

  • arguments is forwarded as-is. ChatToolCall.function.arguments is already a JSON string in the chat format, unlike the Claude path's structured input which needs JSON.stringify. Double-stringifying would send "{\"a\":1}" to the model. There is a dedicated regression test pinning this.
  • Minimal behavior change on empty content. The message item is skipped only when content is empty and tool calls take its place. A content-less assistant message with no tool calls still round-trips as an empty message, so the pre-existing handles null content in assistant message test is unchanged.

#41 — return type was not assignable to callModel input

fromChatMessages declared models.InputsUnion, but callModel's input is FieldOrAsyncFunction<Item[]> | string. So the usage in the function's own doc comment did not typecheck:

error TS2322: Type 'InputsUnion' is not assignable to type 'Item[]'.
  Type 'string' is not assignable to type 'Item[]'.

Both fromChatMessages and fromClaudeMessages now return Item[]. fromClaudeMessages had the identical problem and is fixed here too, since this PR already touches the union.

The Item union had no id-less member for the assistant or system roles — AssistantMessageItem is the model's OutputMessage, which requires an id and structured content. Added NewAssistantMessageItem and NewSystemMessageItem following the existing New* pattern, and exported both. CallFunctionToolItem (= OutputFunctionCallItem, with id?: string) already covered the emitted function_call items as-is.

Role construction moved into per-role narrowed helpers (createMessageItem / createEasyInputMessage). This is load-bearing rather than cosmetic: TypeScript will not distribute a union-typed role across the per-role members of Item, so returning the wide EasyInputMessageRoleUnion from a shared mapChatRole cannot produce an assignable value. mapChatRole became dead and was removed.

Testing

Red-first. Both regressions were pinned and confirmed failing before the fix:

  • Runtime: 4 failures, each showing the tool call collapsing to { role: 'assistant', content: '' }.
  • Types: error TS2322: Type 'InputsUnion' is not assignable to type 'Item[]'.

New tests cover the exact #11 repro (user → assistant with content: null + toolCall → tool), assistant with both text and toolCalls, parallel tool calls, the no-double-stringify guarantee, an empty toolCalls array, and a compile-level const items: Item[] = fromChatMessages(msgs) assertion for #41.

After: 649 passed (649) for @openrouter/agent; build, typecheck, and lint green workspace-wide.

Follow-up: CI typecheck blind spot

packages/agent/tsconfig.typecheck.json includes only src/**/*.ts and one .test-d.ts, excluding tests/e2e/. That is why ~48 fromChatMessages / fromClaudeMessages call sites in tests/e2e/call-model.test.ts passed a wrong-typed value for so long without failing the build.

Measured, as evidence the type fix is real: typechecking that file against main's src yields 69 errors; against this branch, 21 — a reduction of exactly 48, matching the call-site count, with zero overlap between the remaining error lines and the call-site lines.

I deliberately did not widen the tsconfig include here — the remaining 21 errors are pre-existing and unrelated (implicit any, index-signature access, stale SDK module paths), so fixing them is its own PR. Worth doing as a follow-up so this class of bug fails CI.

🤖 Generated with Claude Code

… Item[] (#11, #41)

The assistant branch of `fromChatMessages` never read `msg.toolCalls`, so a
tool-calling assistant message (conventionally `content: null`) converted to
`{ role: 'assistant', content: '' }` — the tool call vanished and the following
`function_call_output` items were left orphaned, breaking agentic loops replayed
through the helper. The `.map()` callback's return type also structurally
prevented emitting function_call items at all.

Converted the conversion to an accumulator loop that emits one `function_call`
item per `toolCalls` entry, mirroring `fromClaudeMessages`. `arguments` is
already a JSON string in the chat format, so it is forwarded as-is rather than
re-stringified. The message item is skipped only when content is empty *and*
tool calls take its place, keeping the pre-existing empty-message behavior for
content-less assistant messages with no tool calls.

Separately, both `fromChatMessages` and `fromClaudeMessages` declared
`models.InputsUnion`, which is not assignable to callModel's
`FieldOrAsyncFunction<Item[]> | string` input — so the documented
`callModel({ input: fromChatMessages(msgs) })` usage did not typecheck. Both now
return `Item[]`, and the `Item` union gains `NewAssistantMessageItem` and
`NewSystemMessageItem` since it had no id-less member for either role.

Role construction moved to per-role narrowed helpers: TypeScript will not
distribute a union-typed `role` across the per-role members of `Item`, so
mapping to the wide `EasyInputMessageRoleUnion` cannot produce an assignable
value.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@perry-the-pr-reviewer perry-the-pr-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perry's Review

Verdict: 💬 Comments / questions

Details

Solid fix for both bugs — #11 (runtime: assistant tool calls dropped) and #41 (types: return not assignable to callModel input). CI is green across all five checks. The accumulator loop, per-role narrowed helpers, and the New*MessageItem types are the right design. The type-level argument about TypeScript not distributing a union-typed role across Item members is correct — a shared mapChatRole returning the wide EasyInputMessageRoleUnion genuinely cannot produce an assignable value, so per-branch narrowing is load-bearing, not cosmetic.

Regression tests are thorough: exact #11 repro (user → assistant content: null + toolCall → tool), assistant with both text and toolCalls, parallel tool calls, no-double-stringify guarantee, empty toolCalls array, and a compile-level Item[] assertion for #41. The no-double-stringify test is a particularly good edge — the chat format's arguments is already a JSON string, and re-stringifying would corrupt it.

One observation (not a blocker for this PR):

Item ordering differs between the two conversion paths. fromChatMessages emits the assistant message item before the function_call items — the natural conversation order (assistant speaks, then calls a tool). fromClaudeMessages emits function_call items before the message item. Both are semantically correct for replay since the API correlates function_call and function_call_output by callId, but aligning the Claude path's ordering in a follow-up would reduce surprise. The chat path's ordering is the better one.

The CI typecheck blind spot (e2e tests excluded from the typecheck tsconfig) is correctly scoped out of this PR. Widening the include so this class of type error fails CI is worth doing as a separate PR — the author's measurement (69 → 21 errors, matching 48 call sites) is good evidence the type fix is real.

Posting as comments since this is a draft. The code looks ready to mark ready-for-review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fromChatMessages does not produce correct object for input in callModel TypeScript fromChatMessages does not properly interpret function_calls

1 participant