weiqingy opened a new pull request, #952: URL: https://github.com/apache/flink-agents/pull/952
Linked issue: #936 Addresses case 1 only. Cases 2 and 3 are design questions and are left as follow-ups, as recorded on the issue. ### Purpose of change Java and Python disagree on what a caller receives when a provider refuses a request. Java treats the refusal as recoverable information and Python discards it, so this is a parity fix with Java as the specification. On a refusal the provider returns an assistant message with no content and the reason in `refusal`. `OpenAIChatCompletionsUtils.convertFromOpenAIMessage` records it as `extraArgs["refusal"]`. The Python converter never read the field, so the reason was lost and `content` became `""`. A Python caller could not tell a refusal apart from a genuinely empty completion. The gap predates the native structured-output work, but `strict: true` makes the refusal path materially more reachable, because a model refuses rather than emitting non-conforming JSON. Runtime flow, unchanged except for one step. A connection calls `chat.completions.create`, builds a local `extra_args` holding `model_name` and token counts when usage is present, and passes it with `response.choices[0].message` to `convert_from_openai_message`. The converter builds the tool-call list, then, when `message.refusal is not None`, rebinds `extra_args` to a new dict carrying the reason before constructing the `ChatMessage`. Pydantic copies that dict during validation, so the write must precede construction to be visible. Both the OpenAI and the Azure OpenAI connections call this helper, so one change covers both. Key decisions. The guard is `is not None` rather than a truthiness test, so an empty refusal reason is still recorded. Java uses `Optional.ifPresent`, which keeps an empty string, and a truthiness test would have left a new divergence in the function meant to remove one. There is no `isinstance` check. The inbound Java path has none. Java's `instanceof String` guards the outbound direction, where `extra_args` holds arbitrary caller data, which is a different situation. The write rebinds rather than mutating the argument. Both callers pass a fresh local dict and pydantic copies it, so this is hygiene and not a guarantee. No test pins it, deliberately. Java gets tests but no production change. Its behavior had no coverage, so a refactor could have removed it and reopened the gap from the other side. ### Implementation Description Behavioral contracts. 1. A refusal reason present on the SDK message appears unchanged at `extra_args["refusal"]` on the returned `ChatMessage`. 2. An empty-string refusal is recorded, not skipped. 3. When the SDK message carries no refusal, no `refusal` key is added. 4. Keys already present in the `extra_args` passed by the caller survive, including the token metrics both connections put there. 5. A refusal leaves `content` as `""`. The reason is never written into `content`. 6. The Java converter's behavior is unchanged. Failure behavior. Nothing in this change raises, falls back, or retries. It adds one conditional dict write with no error path. Invalid configuration does not apply, since the converter takes none. An error from the provider is unaffected and still propagates out of the Python connection unwrapped, which is the divergence recorded as case 3 on the linked issue and is deliberately not touched here. A response violating the expected shape is rejected before the converter sees it, since `refusal` is declared `Optional[str]` on the SDK's pydantic model. The converter does no type checking of its own, so a caller bypassing that model and supplying another type would have it stored as-is. A message lacking the attribute would raise `AttributeError`. The field exists at the floor of the `openai>=1.66.3` pin, so no version guard is used. ### Tests There was no non-integration coverage of the response-conversion path before this, in either language. | Contract | Tests | |---|---| | 1, refusal preserved | `test_refusal_is_preserved_in_extra_args`, Python. `testRefusalPreservedInExtraArgs`, Java | | 2, empty refusal preserved | `test_refusal_is_preserved_in_extra_args`, the `""` parameter | | 3, no key when absent | `test_no_refusal_key_when_refusal_absent`, Python. `testNoRefusalKeyWhenAbsent`, Java | | 4, caller keys survive | `test_refusal_is_preserved_in_extra_args`, which passes a non-empty `extra_args` and asserts it survives | | 5, content not overwritten | `test_refusal_is_preserved_in_extra_args` | | 6, Java unchanged | both Java tests, which lock existing behavior | Each test was run against a deliberately broken implementation to confirm it fails when the behavior it covers breaks. A truthiness guard is caught only by the empty-string parameter, and replacing the merge with a plain assignment only by the surviving-keys assertion. One existing fixture in `test_openai_native_structured_output.py` mocked the SDK message with only `role`, `content` and `tool_calls`. It now sets `refusal` too, so the mock does not return an auto-generated attribute once the converter reads that field. Java runs 22 tests in the openai integration module, up from 20. The Python unit suite runs 657. Azure is covered through the shared helper rather than by a test of its own, because every Azure test in the repo is integration-marked. ### API No signature change, no new class, no new dependency. `extra_args` is an existing field on `ChatMessage`, and `refusal` is a key Java already writes, so the cross-language contract is unchanged. Python starts honoring it. For a caller who does nothing differently, one thing changes: on a refused response, `extra_args` now carries an extra key. Responses that were not refused are unaffected and gain no key. There is one effect beyond the changed files. `convert_to_openai_message` merges `extra_args` into the outbound assistant message, so a `ChatMessage` that carries a refusal and is later sent back to the provider will now include `refusal` in that request. It is a declared field on an assistant message, so this is valid, and it matches what Java already does. ### Documentation - [ ] `doc-needed` - [x] `doc-not-needed` - [ ] `doc-included` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
