feat: support content + toolCalls together in fixture responses#92
feat: support content + toolCalls together in fixture responses#92
Conversation
jpr5
left a comment
jpr5
left a comment
There was a problem hiding this comment.
PR Review Summary
Reviewed by 3 specialized agents (code-reviewer, silent-failure + type-design, test-reviewer) across all 9 changed source files.
Zero code bugs found. Implementation is correct across all 4 providers.
The ContentWithToolCallsResponse type, routing, streaming, non-streaming, and stream-collapse changes are all well-implemented. Content streams first, then tool calls, with correct finish reasons per provider (tool_calls, tool_use, FUNCTION_CALL). Handler ordering is correct everywhere — isContentWithToolCallsResponse checked before isTextResponse/isToolCallResponse. 17 tests cover type guards, all 4 providers (streaming + non-streaming), and collapse coexistence.
Must fix (1)
1. Internal planning docs committed to repo
Files: docs/superpowers/plans/2026-04-08-content-with-toolcalls.md, docs/superpowers/specs/2026-04-08-content-with-toolcalls-design.md
These are internal development artifacts (2,105 lines) and should not be committed to the public repository. Remove before merge.
Needs rebase
The branch is ~20 commits behind main (merge base 7aac519). It will conflict with:
- PR #81 (reasoning support across all providers)
- PR #88 (reasoning_content for OpenRouter)
- PR #89 (action.query for web_search_call)
- PR #90 (docs link cleanup)
Rebase before merge.
Informational (no action required, but worth noting)
Type guard overlap
isTextResponse and isToolCallResponse both return true for combined responses (the test at line 30-38 documents this). Current handler ordering prevents bugs, but any future code that checks isTextResponse first without the combined guard would silently drop toolCalls.
Providers not updated
Bedrock, Bedrock Converse, Ollama, Cohere, and WebSocket handlers don't check for isContentWithToolCallsResponse. A combined fixture routed through them silently drops toolCalls (falls through to isTextResponse). Acceptable scope limitation for this PR.
Collapse inconsistency
collapseCohereSSE and collapseBedrockEventStream were NOT updated to preserve content alongside toolCalls, unlike the other 4 collapse functions. Minor inconsistency.
Test weakness
The Responses API streaming test (line 170-181) only asserts textDelta is defined, not the actual value. Every other provider's streaming test verifies exact content strings.
Dead field
ContentWithToolCallsResponse.finishReason is declared but never read — all handlers hardcode the finish reason.
🤖 Reviewed with Claude Code
jpr5
force-pushed
the
worktree-immutable-baking-graham
branch
from
6c9534d to
8c14ddd
Compare
commit: |
jpr5
left a comment
jpr5
left a comment
There was a problem hiding this comment.
Both findings from Round 1 addressed:
- Internal docs removed (2,105 lines) — ✓
- Responses API streaming test strengthened with actual content assertion — ✓
Branch rebased on current main. Implementation is correct across all 4 providers. LGTM.
🤖 Reviewed with Claude Code
New FixtureResponse variant for fixtures that specify both content and toolCalls. Includes isContentWithToolCallsResponse guard, streaming builder (buildContentWithToolCallsChunks), and non-streaming builder (buildContentWithToolCallsCompletion) for OpenAI Chat Completions.
Streaming and non-streaming support for ContentWithToolCallsResponse in OpenAI Chat Completions (server.ts), OpenAI Responses API (responses.ts), Anthropic Messages (messages.ts), and Gemini (gemini.ts). Content streams first, then tool calls, with correct finish reasons per provider (tool_calls, tool_use, FUNCTION_CALL). Includes shared helper extraction in responses.ts and parseToolCallPart refactor in gemini.ts.
When a recorded stream contains both content and tool calls, collapse functions now return both fields instead of dropping content. Updated collapseOpenAISSE, collapseAnthropicSSE, collapseGeminiSSE, and collapseOllamaNDJSON for record-and-replay fidelity.
Type guard tests (5), streaming + non-streaming per provider (8), stream-collapse coexistence tests for OpenAI/Anthropic/Gemini/Ollama (4).
jpr5
force-pushed
the
worktree-immutable-baking-graham
branch
from
ca08f8a to
6a17828
Compare
## Summary - **Per-test sequence isolation** via `X-Test-Id` header — each test gets its own fixture match counters across all 12 HTTP + 3 WebSocket handlers (#93) - **Combined content + toolCalls** in fixture responses — new `ContentWithToolCallsResponse` type across OpenAI Chat, Responses, Anthropic Messages, and Gemini with stream collapse support (#92) - **OpenRouter reasoning_content** support (#88) - **Clean URLs** for docs site — all pages restructured as directories, .html extensions removed from links - Fix `web_search_call` items to use `action.query` matching real OpenAI API (#89) - Bump aimock-pytest to 0.3.0 ## Version bumps - `package.json`: 1.8.0 → 1.9.0 - `pyproject.toml`: 0.2.0 → 0.3.0 - `Chart.yaml` appVersion: 1.8.0 → 1.9.0 - `plugin.json` / `marketplace.json`: 1.8.0 → 1.9.0 ## Test plan - [x] All 2206 tests pass (including 80 new clean-URL validation tests) - [x] Build passes - [x] Prettier clean - [ ] npm publish triggers automatically on merge via OIDC workflow - [ ] Verify v1.9.0 appears on npm after merge 🤖 Generated with [Claude Code](https://claude.com/claude-code)
2 participants
Summary
ContentWithToolCallsResponsetype toFixtureResponseunion, allowing fixtures to specify bothcontentandtoolCallsin a single responsetool_calls,tool_use,FUNCTION_CALL)Test plan