fix(rest): clear error when response_json_field matches a non-text response value#1892
fix(rest): clear error when response_json_field matches a non-text response value#1892xr843 wants to merge 2 commits into
Conversation
RestGenerator wrapped whatever response_json_field resolved to in a Message without checking its type. When the endpoint's JSON response shape differed from the configured field (e.g. an Azure-style response where the field is a nested object), a dict/list was stored as the Message text. This surfaced much later in the detector as an opaque "'dict' object has no attribute 'lower'" AttributeError after the whole probe had already run, giving the user no hint about the root cause. Validate that an extracted value is text (or None) before building a Message and raise a clear, actionable BadGeneratorException naming response_json_field and showing the offending structure. Also raise the same clear error when the field is missing/unsubscriptable (previously a bare KeyError/TypeError) and when a single JSONPath match resolves to a non-text object. This fails fast at generation time instead of crashing opaquely during detection. Fixes NVIDIA#1888 (also addresses the list-shaped variant in NVIDIA#319). Signed-off-by: xr843 <xianren843@protonmail.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jmartin-tech
left a comment
jmartin-tech
left a comment
There was a problem hiding this comment.
The core issue is valid, however I am not sure raising an unrecoverable error is the right answer for a single request failure.
| # detectors rather than an actionable configuration error. See #1888. | ||
| for value in response: | ||
| if value is not None and not isinstance(value, str): | ||
| raise BadGeneratorException( |
There was a problem hiding this comment.
BadGeneratorException will terminate the run, this may need to either raise a retry or return None to indicate a request failure that may be related to the specific request.
There might be some value in detecting if this was the first failure of this type or if any successful requests completed before hitting this error to better decide if the generator is bad or if is this request produces a bad response.
There was a problem hiding this comment.
Done in 04e97fa — this path now logs an actionable error (naming response_json_field and the offending type/shape) and returns [None], so a single mismatched response is recorded as a failed generation and the run continues, mirroring the existing "JSONPath yielded nothing" branch just below.
On distinguishing a bad generator from a bad request: I checked #1888's expected behaviour — the reporter's endpoint returns a non-text object for every response, and they still expect the run to "finish and present the report". Gating on "first failure with no prior success → raise" would regress that exact case, since the very first call would abort. So I went with unconditional log-and-skip rather than success-tracking. If you'd rather the misconfiguration be surfaced more loudly than a per-response log line, I can add a one-time warning on the first occurrence — without ever aborting the run.
There was a problem hiding this comment.
Gating on "first failure with no prior success → raise" would regress that exact case
While the reporter's expectations are a good data point, they are not always requirements and do not define a regression.
In general it make sense to continue when the error that was reported occurs with some probes or prompts and not others. However, when the user has provided an invalid extraction value and no valid results can be obtained it does not really make sense to continue to spend inference costs for an empty report.
| else: | ||
| response = [response_object[self.response_json_field]] | ||
| except (KeyError, TypeError) as e: | ||
| raise BadGeneratorException( |
There was a problem hiding this comment.
Same as other comment, this exception would terminate the run, is that the correct behavior here?
There was a problem hiding this comment.
Same treatment here in 04e97fa: the type-validation loop now logs the actionable error and returns [None] instead of raising, so a non-text match skips that generation rather than terminating the run.
…ing the run Per review on NVIDIA#1892: raising BadGeneratorException on a single response_json_field mismatch terminates the whole run, which is too aggressive and contradicts the behaviour NVIDIA#1888 asks for ("it should instead finish and present the report"). In that report the endpoint returns a non-text object for every response, yet the run is expected to complete and produce a report. Log a clear, actionable error naming response_json_field and the offending type/shape, then return [None] so the generation is recorded as a failure and the run continues -- mirroring the existing "JSONPath yielded nothing" path in the same method. This still removes the opaque downstream "'dict' object has no attribute 'lower'" crash from NVIDIA#1888 without ending the run early. Update the three tests to assert [None] is returned and the error is logged, rather than expecting BadGeneratorException. Signed-off-by: xr843 <xianren843@protonmail.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jmartin-tech
changed the title
2 participants
Fixes #1888 (also addresses the list-shaped variant in #319).
RestGeneratorwrapped whateverresponse_json_fieldresolved to in aMessagewithout checking its type. When an endpoint's JSON response shape differs from the configured field (e.g. Azure AI Foundry, where the field is a nested object), a dict/list got stored as theMessagetext. This only surfaced much later in the detector as an opaque'dict' object has no attribute 'lower'AttributeError — after the whole probe had already run and spent API calls — giving no hint about the root cause (as the reporter noted).This validates that an extracted value is text (or
None) before building aMessageand raises an actionableBadGeneratorExceptionnamingresponse_json_fieldand showing the offending structure. The same clear error is raised when the field is missing/unsubscriptable (previously a bareKeyError) and when a single JSONPath match resolves to a non-text object. Net effect: fail fast at generation time with guidance, instead of crashing opaquely during detection.Tests added covering dict-valued plain field (#1888 repro), dict-valued JSONPath, and a missing field. All 35 REST generator tests pass; the 3 new tests fail pre-fix and pass post-fix.