Skip to content

Fix: emit LlmResponse.error_code as a plain string from the OpenAI Responses model - #67

Open
AmaadMartin wants to merge 2 commits into
mainfrom
fix/openai-responses-error-code-str
Open

Fix: emit LlmResponse.error_code as a plain string from the OpenAI Responses model#67
AmaadMartin wants to merge 2 commits into
mainfrom
fix/openai-responses-error-code-str

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Jul 31, 2026

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):

No existing issue.

  1. Or, if no issue exists, describe the change:

Problem: LlmResponse.error_code is declared Optional[str]
(src/google/adk/models/llm_response.py:93), but the OpenAI Responses model
stores a google.genai.types.FinishReason enum member in it.

_response_to_llm_response sets the field by post-construction attribute
assignment
(src/google/adk/labs/openai/_openai_responses_llm.py:731):

llm_response.error_code = finish_reason

LlmResponse.model_config (llm_response.py:52-56) sets only extra,
alias_generator and populate_by_name — it does not enable pydantic's
validate_assignment. Pydantic v2 therefore runs no validation on assignment,
so the str coercion that every other producer of this field gets for free
from the constructor never runs, and the enum member survives in a str-typed
field.

FinishReason is a str-based enum that does not override Enum.__str__ /
Enum.__format__, so the leak is user-visible:

r = _response_to_llm_response({
    'id': 'resp_1', 'model': 'gpt-5', 'status': 'incomplete',
    'incomplete_details': {'reason': 'max_output_tokens'}, 'output': [],
})
type(r.error_code)             # <enum 'FinishReason'>            (want <class 'str'>)
f'{r.error_code}'              # 'FinishReason.MAX_TOKENS'        (want 'MAX_TOKENS')
r.model_dump()['error_code']   # <FinishReason.MAX_TOKENS: ...>   (want 'MAX_TOKENS')

That reaches users through both public entry points, since the same converter
backs non-streaming generation (_openai_responses_llm.py:1079) and the
streaming finalizer (_openai_responses_llm.py:991), and Event subclasses
LlmResponse (events/event.py:92). Consumers that interpolate or persist the
value — a2a/converters/event_converter.py:457,463 (str(event.error_code)),
plugins/logging_plugin.py:202, plugins/debug_logging_plugin.py:461 and the
String session column at sessions/schemas/v0.py:277 — all see the
FinishReason.MAX_TOKENS form.

Solution: assign finish_reason.value instead of the enum member, so the
field holds the bare code. This is the one-token change that puts this producer
on the same footing as every other one in the repo (LlmResponse.create() at
llm_response.py:218,231, utils/streaming_utils.py:387,407, and the sibling
branch at _openai_responses_llm.py:877), all of which pass through the
constructor and are already coerced.

Alternatives considered and rejected:

  • validate_assignment=True on LlmResponse. It would fix this at the
    root, but it silently changes coercion and validation behaviour for every
    field on LlmResponse and every subclass — including the public Event
    surface — which is far out of proportion to this defect.
  • Fixing the identical sites in src/google/adk/models/lite_llm.py
    (:2126, :2896, :2917) in the same PR.
    Deliberately out of scope: that
    is a separate provider with its own test suite, and it is handled by a
    separate change so the two can be reviewed independently.

Intentional behaviour change (please read). This is a deliberate correction
on a public field, not an internal refactor.

  • Changes: str(error_code) / f-string interpolation now yields
    'MAX_TOKENS' instead of 'FinishReason.MAX_TOKENS'; python-mode
    model_dump() yields a str instead of the enum object;
    isinstance(error_code, types.FinishReason) becomes False;
    error_code.value / .name now raise AttributeError.
  • Does not change: equality against FinishReason members
    (FinishReason.MAX_TOKENS == 'MAX_TOKENS' is True, so existing comparisons
    keep working — the pre-existing assertions in the tests below prove it),
    model_dump_json() (already serialised by value), and everything about
    finish_reason, which stays a types.FinishReason.
  • Risk: low. Every in-repo consumer is an equality/truthiness check or a
    string interpolation; all are unaffected or strictly improved.

Collision check. Ran
gh pr list --repo AmaadMartin/adk-python --state open --limit 100 and diffed
the file lists of the plausibly adjacent PRs. The only related work is the
lite_llm equivalent (fix/litellm-error-code-plain-str) and
fix/lite-llm-streaming-none-finish-reason-guard; both touch only
src/google/adk/models/lite_llm.py and
tests/unittests/models/test_litellm.py, so there is no overlap with this
change and no reason to stack. No open PR touches
_openai_responses_llm.py.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.

Unit Tests:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.
$ .venv/bin/python -m pytest tests/unittests/labs/openai/test_openai_responses_llm.py -q
53 passed in 2.05s

Added test_error_code_is_plain_string_not_finish_reason_enum, parametrized
over both reachable outcomes of the finish_reason and finish_reason != STOP
branch (MAX_TOKENS from status: 'incomplete', OTHER from
status: 'failed'), so a future edit to _map_finish_reason cannot silently
regress one arm. The type check is written type(x) is str rather than
isinstance(x, str) on purpose: FinishReason subclasses str, so an
isinstance check passes against the buggy value and proves nothing.

Also extended existing tests rather than cloning them:

  • test_streaming_incomplete_event_sets_max_tokens — proves the fix through
    the public generate_content_async(..., stream=True) API, not just the
    private converter.
  • test_streaming_generation_failed_event_is_terminal — pins the
    already-correct constructor path (_openai_responses_llm.py:877) to the same
    str contract, so it cannot drift.
  • test_response_parsing_maps_text_reasoning_tool_calls_and_usage — negative
    case: a completed response still leaves error_code is None.

No existing test or assertion was deleted, skipped, or weakened. In particular
the pre-existing error_code == types.FinishReason.MAX_TOKENS (line 1286) and
error_code == types.FinishReason.OTHER (line 1303) assertions are kept
untouched and still pass — they are the guard that this change does not break
callers comparing against enum members.

Coverage. 100% line and branch coverage of the changed code. Measured with
--cov=google.adk.labs.openai._openai_responses_llm --cov-branch --cov-report=term-missing: the changed line and the enclosing branch at line
727 are absent from the Missing column, with all three outcomes exercised
(MAX_TOKENS, OTHER, and the STOPerror_code is None negative arm).

Mutation check — every new test was proven able to fail. Reverted the
source hunk to the unfixed llm_response.error_code = finish_reason and re-ran
the file: 3 failed, 50 passed. The failures are exactly the tests that pin the
new behaviour:

FAILED ...::test_error_code_is_plain_string_not_finish_reason_enum[response0-MAX_TOKENS]
FAILED ...::test_error_code_is_plain_string_not_finish_reason_enum[response1-OTHER]
FAILED ...::test_streaming_incomplete_event_sets_max_tokens

E  AssertionError: assert <enum 'FinishReason'> is str
E   +  where <enum 'FinishReason'> = type(<FinishReason.MAX_TOKENS: 'MAX_TOKENS'>)

The two assertions that correctly keep passing under the mutation are the
constructor-path guard and the error_code is None negative case — neither is
affected by this defect, which is the expected result.

Manual End-to-End (E2E) Tests:

No credentials, network, or live model needed — the defect is deterministic
from a plain Mapping payload. To reproduce and confirm, run before and after
the change:

python -c "
from google.adk.labs.openai._openai_responses_llm import _response_to_llm_response
r = _response_to_llm_response({
    'id': 'resp_1', 'model': 'gpt-5', 'status': 'incomplete',
    'incomplete_details': {'reason': 'max_output_tokens'}, 'output': [],
})
print(type(r.error_code), f'{r.error_code}', repr(r.model_dump()['error_code']))
"

Before: <enum 'FinishReason'> FinishReason.MAX_TOKENS <FinishReason.MAX_TOKENS: 'MAX_TOKENS'>
After: <class 'str'> MAX_TOKENS 'MAX_TOKENS'

Formatting/lint on the two touched files, matching the repo's pre-commit
configuration (pyink 25.12.0, isort, ruff 0.15.17):

$ .venv/bin/pyink --check src/google/adk/labs/openai/_openai_responses_llm.py tests/unittests/labs/openai/test_openai_responses_llm.py
2 files would be left unchanged.
$ .venv/bin/isort --check-only <same two files>          # clean
$ .venv/bin/ruff check src/google/adk/labs/openai/_openai_responses_llm.py
All checks passed!

No type-checker or linter suppressions were added, and no new dependencies or
imports were introduced.

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.

CI status on this PR

All three red jobs are pre-existing failures unrelated to this change, each
already the subject of a separate in-flight PR. I deliberately did not fix them
here: doing so would duplicate that work, and CI workflow changes are out of
scope for this fix.

1. Unit Tests (3.10–3.14) — pre-existing, reproduced on pristine main.
The run is 1 failed, 9362 passed; the single failure is
`tests/unittests/cli/utils/test_cli_tools_click.py::test_telemetry_cli_commands

  • assert 2 == 0(bare CLI group exit code on click >= 8.2). Every test intests/unittests/labs/openai/passed in CI. Verified locally that the failure is not mine by checking outmain` unmodified and running the same test:
# on main, no local changes:
$ .venv/bin/python -m pytest tests/unittests/cli/utils/test_cli_tools_click.py::test_telemetry_cli_commands -q
FAILED ...::test_telemetry_cli_commands
1 failed

2. Pre-commit Linter — pre-existing infrastructure failure. The job runs
pre-commit run --all-files, and the update-constraints hook
(pass_filenames: false) dies with ./scripts/update_constraints.sh: line 103: uv: command not found on the runner. It fails identically on other open PRs.
It does not apply to my files: running pre-commit against just the two files in
this diff passes every applicable hook, and update-constraints is skipped
because neither file matches its files: ^(pyproject\.toml|constraints-.*\.txt)$
pattern.

$ .venv/bin/pre-commit run --files src/google/adk/labs/openai/_openai_responses_llm.py tests/unittests/labs/openai/test_openai_responses_llm.py
fix end of files.........................................................Passed
trim trailing whitespace.................................................Passed
ruff (legacy alias)......................................................Passed
isort....................................................................Passed
pyink....................................................................Passed
addlicense...............................................................Passed
Check new Python files have _ prefix.....................................Passed
ADK Compliance Checks....................................................Passed
update-constraints...................................(no files to check)Skipped
codespell................................................................Passed

3. Mypy Check (3.10–3.13) — false positive from the gate's comparison
step.
The job reports two "NEW" errors, both in
src/google/adk/models/lite_llm.py. That file is not in this diff
(git diff main --name-only lists exactly two files, neither of them
lite_llm.py), and it cannot be affected by this change: its entire ADK import
closure is base_llm, llm_request, llm_response and
utils._google_client_headers, none of which import labs.openai. The job's
own output shows the totals are identical on both sides:

Generate Baseline: Found 2521 errors on main.
Check PR Branch:   Found 2521 errors on PR branch.

Equal totals with two lines reported as "new" means the comm -13 comparison is
mismatching duplicate lines, not detecting a regression — the step pipes mypy
through sed 's/:\([0-9]\+\):/::/g', which collapses distinct errors in the same
file to identical strings, so comm becomes sensitive to multiplicity rather
than content. Re-running the job on the same commit reproduces it, so it is
deterministic rather than flaky. I also ran the gate's exact
baseline-vs-PR logic locally against the file this PR does change and got no new
errors:

$ mypy src/google/adk/labs/openai/_openai_responses_llm.py ... | sed 's/:\([0-9]\+\):/::/g' | sort   # at main -> base_errs
$ mypy src/google/adk/labs/openai/_openai_responses_llm.py ... | sed 's/:\([0-9]\+\):/::/g' | sort   # at HEAD -> pr_errs
$ comm -13 base_errs pr_errs
(empty)

Since CI could not be made green without stepping on other in-flight PRs, the
change was validated locally on the exact pushed commit (77c127a):

$ .venv/bin/python -m pytest tests/unittests/labs/openai/ -q
65 passed in 2.02s
$ .venv/bin/python -m pytest tests/unittests/models/test_llm_response.py -q
20 passed in 1.22s

Amaad Martin added 2 commits July 30, 2026 18:42
_response_to_llm_response assigned the FinishReason enum member to
LlmResponse.error_code after construction. LlmResponse does not enable
pydantic's validate_assignment, so the str coercion that every other
producer gets from the constructor never ran and the field kept the enum.
Callers that log or persist the Optional[str] field saw
"FinishReason.MAX_TOKENS" instead of "MAX_TOKENS", and a python-mode
model_dump() leaked the enum object.

Assign finish_reason.value so the field holds the bare code. Equality
against FinishReason members still holds because it is a str-based enum.
str(x), f'{x}' and model_dump() on a field that is already exactly a str
cannot fail once `type(...) is str` and the value assertion pass. Also trim
the fix's comment to the one fact a reader cannot see from the types.
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.

1 participant