fix: enforce OpenAI tool-choice contract - #650
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens FastFlowLM’s OpenAI-compatible Chat Completions boundary by enforcing tool_choice / allowed_tools / parallel_tool_calls, validating emitted tool calls before returning them to clients, and buffering tool-call streaming to fail closed on malformed generations.
Changes:
- Add a centralized tool-policy parser + response validator (
openai_tool_policy.hpp) and integrate it into/v1/chat/completionshandling (including buffered SSE streaming with optional usage chunk). - Add a focused CTest unit suite plus a manual live-server Python contract harness for tool policy / SSE behavior.
- Wire the contract test into CMake/CTest and Debian builds, and document the supported contract and failure semantics.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/include/openai_tool_policy.hpp | Implements tool policy parsing, tool-call validation, and buffered streaming chunk builder. |
| src/server/rest_handler.cpp | Applies tool policy to requests, validates model tool calls, buffers tool-call streaming, and clears state on rejected generations. |
| src/server/server.cpp | Maps structured error code values to HTTP 400/500 for OpenAI-style error responses. |
| src/test/openai_tool_policy/test.cpp | Adds CTest unit coverage for policy parsing, validation, and buffered SSE chunk shape. |
| src/test/openai_tool_policy/CMakeLists.txt | Builds the new unit test executable and registers it with CTest. |
| src/test/openai_tool_policy/test_server_contract.py | Adds a manual live-server contract harness for HTTP + SSE behavior (including recovery). |
| src/test/openai_tool_policy/README.md | Documents how to run the unit test and the manual live-server harness. |
| src/CMakeLists.txt | Enables CTest and adds the new openai_tool_policy test directory when BUILD_TESTING is on. |
| docs/docs/instructions/server/tool_calling.md | Documents the supported OpenAI tool-selection contract, buffered streaming semantics, and safe dispatch guidance. |
| debian/rules | Runs ctest during Debian builds to ensure the contract test is executed in packaging CI. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This fixes a missing Missing tool_calls[].index API compatibility bug
| meta_info.load_duration = (uint64_t)time_utils::duration_ns(load_start_time, load_end_time).first; | ||
| meta_info.max_prefill_len = this->prefill_chunk_len; | ||
| if (stream){ | ||
| if (stream && !tool_policy.requires_buffered_validation()){ |
| meta_info.load_duration = (uint64_t)time_utils::duration_ns(load_start_time, load_end_time).first; | ||
| meta_info.max_prefill_len = this->prefill_chunk_len; | ||
| if (stream){ | ||
| if (stream && !tool_policy.requires_buffered_validation()){ |
There was a problem hiding this comment.
this is really not a good way to fix this. I am working on a better fix.
There was a problem hiding this comment.
You might be able to adapt my 1.0.1 fix -- got it working with a full context (128000) full of turns and tools in OpenCode. I also updated the chat template in my models and that helped with it some. Works with pi as well.
It shouldn't be super hard!
There was a problem hiding this comment.
So i looked at your fix. It does fix a very similar kind of issue but in an area I would consider a separate boundary and area of concern making these patches not so similar. The above bug that is noted by the copilot really is addressing the streaming/non streaming pairty behavior that is expected in openai.
There is a distinct lack of boundary responsibility distinction in this library. My goal in the patch was also so move this closer to an architecture rather than collections of patches. Roughly with the goal of creating sensible boundaries of responsibility.
Something like this
HTTP request
↓
OpenAI request
↓
Validate OpenAI contract
↓
Convert to canonical conversation
↓
Adapt canonical conversation for model/template
↓
Inference
↓
Parse model-specific output
↓
Canonical assistant result
↓
Enforce requested tool policy
↓
Convert to OpenAI response/chunks
↓
HTTP JSON or SSE response
Your change would live near and handle boundary in canonical conversation to template layer. This patch really is trying to build out a proper openai API boundary.
I would love help/to help with building these layers.
There was a problem hiding this comment.
Makes sense to me! Standards and best practices are good.
There was a problem hiding this comment.
@Atomic-Germ This project needs a large refactor to be anywhere as close to usable, stable, maintainable and I kinda wonder if anyone is home on this thing too.
|
@waw2637 I'm really interested in this functionality, mainly because OpenCode is not receiving correctly the context information so it's missing compactation every single time, and not efficiently managing contexts. If you need any help, ping me |
|
@Javinator9889 does your issue match the issue linked to this PR #649. Or is it a new behavior. From your brief description it is possible that my code could help, but it does not per se fix tool handling, and compaction is kinda another area all together that i have not gotten into at all with fastflowlm. A lot can still go wrong other layers giving you trash output. Convert to canonical conversation The code i have here simply makes sure that the api contract for OpenAI is enforced correctly. It doesn't recheck all earlier layers parsing and handling. |
|
Yeah, it's the missing piece for OpenCode AFAIK. The So your PR, as it enforces a correct OPEN API endpoint should fix that |
|
@Javinator9889 It does do that so what can I do to help you. My fork (is level with main and contains my patch) compiles under Ubuntu. I think I am going to collect some of the prs and patches languishing and pull them into my fork (not ideal but if others find it useful). |
What's the deal with merging upstream? No maintainers, reviewers? |
Exactly. They basically aren't here, and it's only partly open source, just enough for AMD to celebrate themselves. |
Summary
tool_choicemodes (none,auto,required, named function, andallowed_tools) andparallel_tool_callsstream_options.include_usageCloses #649
Why
FastFlowLM 0.9.46 could return HTTP 200 with
finish_reason: "tool_calls"even when a truncated model response produced an empty or undeclared function name or malformed arguments. This was reproduced across Gemma 4, Qwen 3, and Qwen 3.5. Clients then attempted to dispatch calls that were not usable.The REST boundary now treats the model/parser output as untrusted and validates it against the request contract before exposing it to clients.
Supported contract
/v1/chat/completionstool_choice:none,auto,required, named function, andallowed_toolsparallel_tool_callsExplicit limitations
strict: trueis rejected because constrained JSON Schema decoding is not implementedVerification
Build and unit tests
flmtarget successfully (96 build steps)openai_tool_policy_contractpassedLive NPU contract matrix
All models were loaded and tested serially with the binary built from
11c4081.tool_choice: nonetool_choice: requiredallowed_toolsrequired subsetThe recovery test deliberately truncates a required call with
max_tokens: 1, expects a structured HTTP 500 model error, and immediately sends a valid required call to verify that rejected state is not reused.Related reports
These can improve how often a model generates a valid call, while this PR protects the API boundary when it does not.
Review requested
This PR is intentionally draft until independent review confirms the contract and test coverage.