Skip to content

FIX: raise when a completed Responses output has nothing readable - #2906

Open
fei (feiiiiii5) wants to merge 3 commits into
microsoft:mainfrom
feiiiiii5:fix/response-target-no-readable-output
Open

fei (feiiiiii5) wants to merge 3 commits into
microsoft:mainfrom
feiiiiii5:fix/response-target-no-readable-output

Conversation

@feiiiiii5

Copy link
Copy Markdown
Contributor

Description

OpenAIResponseTarget reports success for a completed Responses call whose output carries nothing PyRIT can read. With a built-in tool enabled (image_generation, code_interpreter, file_search, …) the API answers with sections such as image_generation_call, which _parse_response_output_section skips. The target then returns a Message whose only piece is the reasoning dump, with response_error="none", so the attack loop records that JSON as the model's answer and scores it.

has_visible_response is already computed while looping over the sections, but was only consulted on the truncated path. OpenAIChatTarget._construct_message_from_response_async raises EmptyResponseException in this same situation; this makes the Responses target match it, while still returning the message when a readable section sits next to an unmodelled one. The method docstring now documents the raise.

Tests and Documentation

Two cases in tests/unit/prompt_target/target/test_openai_response_target.py: a completed response of reasoning plus an unmodelled section now raises EmptyResponseException (fails on main with DID NOT RAISE), and a readable message next to an unmodelled section is still returned.

pytest tests/unit/prompt_target/ -q gives 1494 passed; ruff check and ruff format --check are clean on both files.

fei (feiiiiii5) and others added 2 commits September 29, 2026 19:26
_construct_message_from_response_async tracks has_visible_response but
only consulted it on the truncated path, so a completed response whose
output PyRIT cannot read came back as a successful Message holding the
reasoning dump. With a built-in tool enabled (image_generation,
code_interpreter, file_search, ...) the Responses API returns sections
such as image_generation_call, which _parse_response_output_section
skips with `return None`; the run then scored the reasoning JSON as the
model's answer with response_error="none".

OpenAIChatTarget already raises EmptyResponseException when a response
that is not truncated yields no content. Do the same here, and keep
returning the message when a readable section is present next to an
unmodelled one.
@hannahwestra25 hannahwestra25 self-assigned this Oct 1, 2026
# A response that completed without a readable section is a failure, not an answer.
# The chat target raises in the same situation; reporting reasoning or a section
# type PyRIT does not model as the model's response would let it be scored as one.
raise EmptyResponseException(message="Failed to extract any response content.")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

EmptyResponseException is retried by @pyrit_target_retry, but an unmodelled section type recurs every attempt — so this costs 10 billed calls plus backoff, and drops the agentic loop's collected tool messages. Per doc/contributing/9_exception.md, deterministic failures shouldn't retry.

Suggested change
raise EmptyResponseException(message="Failed to extract any response content.")
raise PyritException(message="Failed to extract any readable response content.")

`_send_model_request_async` is wrapped in `@pyrit_target_retry`, which
retries `RateLimitError | EmptyResponseException | RateLimitException`.
The check added in this branch raised `EmptyResponseException` for a
response that *completed* with no section PyRIT models, so every retry
reproduced the same shape: ten billed calls plus backoff before the
agentic loop gave up, and the tool messages it had collected were dropped
with the exception.

`doc/contributing/9_exception.md` scopes retry to rate limits and parse
failures, so raise `PyritException` instead. The docstring now says so
rather than naming an exception that is no longer raised.

The test asserts `type(excinfo.value) is PyritException` and that it is
not an `EmptyResponseException`. Asserting only `PyritException` would
not have caught this, since `EmptyResponseException` subclasses it --
the weaker assertion passes on the old code too.

Reported by @hannahwestra25.
@feiiiiii5

Copy link
Copy Markdown
Contributor Author

Right, and I checked the mechanism before changing it: _send_model_request_async is decorated @pyrit_target_retry, and pyrit_target_retry retries RateLimitError | EmptyResponseException | RateLimitException (exception_classes.py:403-405), with doc/contributing/9_exception.md scoping retry to rate limits and parse failures. A section type we do not model comes back identically on every attempt, so the ten attempts only re-bill the same outcome.

Done in 1de72b6c — PyritException instead, verbatim from your suggestion, and the docstring updated since it named an exception that is no longer raised.

One thing worth flagging because my first attempt got it wrong: asserting only pytest.raises(PyritException) does not catch this, because EmptyResponseException subclasses BadRequestException subclasses PyritException. I confirmed that, and the weaker assertion passed on the pre-fix head. The test now pins the actual behaviour:

with pytest.raises(PyritException) as excinfo:
    ...
assert not isinstance(excinfo.value, EmptyResponseException)
assert type(excinfo.value) is PyritException

which fails on 21187667 and passes here.

pytest tests/unit/prompt_target/target/test_openai_response_target.py -q   108 passed
ruff check / ruff format --check on both changed files                    clean

One asymmetry I did not change, because it is outside this PR and may be deliberate: openai_chat_target.py:334 raises EmptyResponseException for the same "completed with nothing readable" condition, so the chat and Responses targets now disagree on whether that is retryable. Your reasoning would apply there too, but it is a behaviour change for a target this PR does not touch, so I would rather you decide than have me widen the diff — happy to send it as a one-line follow-up if you want the two aligned.

This branch has not been deployed

No deployments
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.

2 participants