UN-4017 [FIX] Reject non-mapping outputs at the prompt-output internal API - #2251
Merged
Conversation
…l API
`outputs` is indexed by prompt key downstream — OutputManagerHelper.
handle_prompt_output_update does outputs.get(prompt.prompt_key) — so a list
raised AttributeError inside the helper and surfaced to the caller as a bare
500 with no usable reason:
internal_views.py:84 prompt_output
-> output_manager_helper.py:179 handle_prompt_output_update
output = outputs.get(prompt.prompt_key)
AttributeError: 'list' object has no attribute 'get'
Reachable from ordinary traffic, not just a malformed client: single-pass
extraction passes the LLM's parsed JSON straight through as the outputs map,
and that parse yields a list whenever the model wraps its answer in prose, a
reasoning block or a stray fence marker. That is why it was intermittent and
appeared model-dependent.
The view already validated prompt_ids and document_id and simply did not
check this one. Validating it here means no future executor can 500 the
backend the same way, and the caller gets the reason instead of a 500.
Absent `outputs` still defaults to {} and is accepted, unchanged.
Contributor
|
| Filename | Overview |
|---|---|
| backend/prompt_studio/prompt_studio_core_v2/internal_views.py | Adds early object-shape validation before ORM loading and output persistence. |
| backend/prompt_studio/prompt_studio_core_v2/prompt_studio_helper.py | Adds matching response-shape validation while preserving structured API exception propagation. |
| backend/prompt_studio/prompt_studio_core_v2/tests/test_prompt_output_outputs_validation.py | Covers non-object rejection, dictionary acceptance, defaults, helper isolation, status codes, and tailored error guidance. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Prompt execution response] --> B{Path}
B -->|Internal callback API| C{outputs and metadata are objects?}
B -->|In-backend response handling| D{outputs and metadata are dicts?}
C -->|No| E[Return HTTP 400]
C -->|Yes| F[Load prompts and persist output]
D -->|No| G[Raise AnswerFetchError with HTTP 422]
D -->|Yes| F
Reviews (10): Last reviewed commit: "UN-4017 [MISC] Drop the json-repair decl..." | Re-trigger Greptile
… error A JSON array is valid JSON. Calling the response "non-object JSON" reads as "malformed JSON" and sends the reader looking for a parse failure that did not happen. Name the actual shape and why it cannot be used here.
…nternal API handle_prompt_output_update has two callers, and the first commit only covered one. prompt_studio_helper._handle_response is the other: it feeds response["output"] straight in, and _fetch_single_pass_response dispatches the identical single_pass_extraction executor, so it receives the identical shapes. Guarding only the worker -> internal API route left this path able to 500 exactly as before — and the stated point of the boundary check was that no executor can 500 the backend this way. _handle_response is the shared choke point for both in-backend callers (single prompt and single pass), so the guard goes there. Raises the local AnswerFetchError with 422, matching the source-side guard rather than the generic 500 that class defaults to. The single-pass advice about prompts asking for lists is only emitted when is_single_pass, since that interaction does not exist on the single-prompt path and would misdirect there.
…than claimed Review found the comment's central claim was false. `metadata` reaches handle_prompt_output_update unchecked and is indexed five times at its lines 135-139 — context, challenge_data, highlight_data, confidence_data, word_confidence_data — all unconditional and evaluated BEFORE the `if not prompts` early exit and before the outputs.get at 179. So a caller posting valid `outputs` with `"metadata": []` still produced the exact 500 this PR claims to eliminate. Both fields are now checked. test_helper_is_never_reached_for_invalid_outputs passed either way: with the guard removed, filter() raised on the fake prompt id before the helper was reached, and patching `status` hid the resulting ValidationError. Patch the ORM instead, and assert the 400. The docstring's "no pytest-django in this repo yet" note was copy-pasted from test_task_status.py and is wrong in both places: pytest-django>=4.12.0 is declared at backend/pyproject.toml:79, backend/conftest.py already imports django.test, and the unit-backend rig group collects this file today. `prompt_ids` is deliberately still unchecked: a non-list makes filter() raise ValidationError which the existing except already turns into a 500, the producer always sends a list, and the `if not prompt_ids` check covers the realistic cases. Noting it so the omission is a decision, not an oversight.
json_repair_helper imports json_repair, but nothing in OSS declares it — the image only gets it transitively, from cloud plugin dependencies that copy_cloud_deps merges into requirements.txt. So the OSS test environment is the one place it is absent, and the helper silently takes `except ImportError: return json_str` there. That makes any test asserting real repair behaviour worse than useless: it goes green while pinning the fallback. Verified — with json_repair blocked, prose and object-plus-prose come back as str and are still rejected, and a clean object takes the json.loads fast path, so three of four such tests pass for the wrong reason. Test group only. Declaring it as a runtime dependency of workers is a separate question, tracked with the broader parser work.
![uv-lock-automation[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
pk-zipstack
reviewed
Aug 20, 2026
pk-zipstack
reviewed
Aug 20, 2026
pk-zipstack
reviewed
Aug 20, 2026
…kfile revert Three points from @pk-zipstack. The in-backend detail hardcoded "LLM returned a JSON array" while interpolating the real type, so `response["output"] = None` produced "LLM returned a JSON array ... (got NoneType)" — self-contradictory, and it sends the reader looking for an array that was never there. The cloud half of this ticket makes exactly that argument and pins it with a test; this half did the opposite. Names the type now, no shape claim. `metadata` was still passed unchecked three lines below the guard, and handle_prompt_output_update indexes it five times before its `if not prompts` early exit — the same exposure this PR fixes in internal_views and describes as "missed on the first pass". Both fields are checked here now. Reverts the root uv.lock. The "Commit uv.lock changes" automation regenerated it with an older uv, rolling `revision = 3` back to `1` and stripping every upload-time field: ~2100 lines of pure churn, no dependency, version or hash actually changed. Nothing in this PR touches a root or backend/ dependency — the json-repair addition is in workers/ and lands in workers/uv.lock. Left as it was, it would silently roll the lock format back for everyone and conflict with any other in-flight lock change.
Resolves the uv.lock conflict by taking main's copy. This branch changes no root or backend/ dependency — the json-repair addition is in workers/ and lands in workers/uv.lock. The root-lock diff came entirely from the 'Commit uv.lock changes' automation regenerating it with an older uv (revision 3 -> 1, every upload-time field stripped), which is churn, not a change.
…n from this PR Two reasons converge on removing it. The uv-lock automation (.github/workflows/uv-lock-automation.yaml) fires on any `**/pyproject.toml` change and installs uv 0.6.14, which is older than whatever wrote the repo's root uv.lock at revision 3. It regenerates every lock, rolling that one back to revision 1 and stripping ~1070 upload-time fields. It re-runs on every synchronize, so reverting by hand cannot stick while this PR touches a pyproject — @pk-zipstack's request to revert is only achievable by not triggering it. And the declaration does not belong here. This PR is two guards; the json-repair dependency question — test group vs runtime, and the live OSS consequence @pk-zipstack identified at answer_prompt.py:438 — is the subject of #2250, which already declares it. Consequence, stated plainly: the cloud PR's real-repair tests keep skipping in CI until #2250 lands, exactly as they do today. That is visible in the test report rather than silent.
harini-venkataraman
approved these changes
Aug 20, 2026
Contributor
Author
Review round addressed — ready for another look@pk-zipstack all three points are handled; replies are in-thread.
Why the dependency moved rather than getting promoted. Your two comments were in direct tension. Your underlying finding is untouched by that and I've carried it over: the image ships without Also: merged Worth its own fix: that automation downgrades the root lockfile on every PR touching a pyproject. Pinning the workflow's |
pk-zipstack
reviewed
Aug 20, 2026
…adata Regression I introduced when merging the two fields into one loop, and the same class of misdirection the shape-claim rewrite had just removed. `metadata` is executor-assembled — run_id, file_name, context, plus the highlight/confidence blocks — not LLM output. A non-dict there is our defect, so "Rephrase that prompt to describe the value of its own field" is a dead end: no wording change can affect it. The neutral message in internal_views had this right; this path regressed relative to it. Gated on the field. Test asserts the advice is absent for metadata, alongside the existing one asserting it is absent for single-prompt — the field dimension was untested, which is why generalising into a loop slipped past.
…s PR alone These two PRs need to stand on their own, so the dependency comes back here rather than being deferred. json_repair_helper imports json_repair, and nothing in OSS declares it — the image only receives it transitively, from cloud plugin dependencies that copy_cloud_deps merges into requirements.txt. The rig syncs the workers `test` group instead, so json_repair is absent there and the helper silently takes `except ImportError: return json_str`. That is what made the cloud PR's TestAgainstRealRepairOutput skip. Left alone, the suite that exists precisely because stubbing everything hid the original bad split would ship inert. Test group only, and a lower bound rather than a pin. Note on the lockfile: the uv-lock automation regenerates every lock with an older uv on any pyproject change, which rewrites the root uv.lock. Its trigger is `**/pyproject.toml`, so a follow-up commit touching only uv.lock reverts that churn without re-firing it.
The uv-lock automation installs uv 0.6.14, older than whatever wrote the root lock at revision 3, so it rolls the format back to revision 1 and strips every upload-time field — ~2100 lines, with no dependency, version or hash actually changing. This PR alters no root or backend dependency; its json-repair addition is in workers/ and lands in workers/uv.lock. Reverting held last time only until the next push. This commit touches no pyproject, and the workflow's trigger is `**/pyproject.toml`, so it will not re-fire and the revert stands. The automation does this to every PR that touches any pyproject. Pinning its uv to the version that produced revision 3 would fix it at the source.
Contributor
Unstract test resultsPer-group results
Critical paths
|
… churn
Keeps this PR to the two guards. The dependency cannot be declared here
without cost: GitHub matches the uv-lock automation's `paths` filter against
the whole PR diff, not the individual push, so any PR containing a pyproject
change re-fires it on every sync — and that workflow installs uv 0.6.14, older
than whatever wrote the root uv.lock at revision 3, so it rewrites ~2100 lines
with no dependency, version or hash actually changing. Reverting it cannot
stick while the declaration is present; it came back on both attempts.
Consequence, stated plainly: TestAgainstRealRepairOutput in the paired cloud PR
skips wherever json_repair is absent, which includes the rig. It runs in the
image, where cloud plugin dependencies supply json-repair transitively, and
locally with the plugin installed. That is visible in the test report rather
than silent.
The underlying gap is unchanged and independent of these PRs: OSS declares
json_repair nowhere, so answer_prompt.py:438-442 persists {} for any answer
that is not strictly valid JSON. Worth its own fix, along with pinning that
workflow's uv.
|
Contributor
Author
Correction: disregard earlier references to the other parser PRThat PR is stale and is not being merged. This PR and Zipstack/unstract-cloud#1739 are self-contained — neither depends on anything else. Description updated; please ignore the earlier thread where I said the Where that leaves your finding, @pk-zipstack: it stands and is not fixed here. OSS declares I tried declaring it here twice and reverted both times. GitHub matches the uv-lock automation's Tracked for a follow-up, together with pinning that workflow's This PR is now three files, no dependency or lockfile changes. |
Contributor
Unstract test resultsPer-group results
Critical paths
|
pk-zipstack
approved these changes
Aug 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
outputsormetadataat theprompt_outputinternal API with a 400, instead of letting them raiseAttributeErrorinsideOutputManagerHelperand surface as a 500._handle_responsepath.Why
Production throws this on Prompt Studio single-pass runs (
unstract-prod/unstract-production, callerunstract-worker-ide-callback-v2):Reachable from ordinary traffic: single-pass extraction passes the LLM's parsed JSON straight through as the outputs map, and the repair returns a list whenever the model adds commentary or answers in prose. Hence intermittent and seemingly model-dependent.
metadatahas the identical exposure — indexed five times atoutput_manager_helper.py:135-139, unconditionally and before theif not promptsearly exit — so validoutputswith"metadata": []produced the same 500.How
_parse_json_bodyand theprompt_ids/document_idvalidation, before any ORM work.outputs.metadatais executor-assembled, so telling a user to rephrase a prompt would be a dead end for a defect that is ours._handle_responsegets the same guard, raising the localAnswerFetchErrorwith 422.Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
.get— so nothing that used to succeed now fails.dictpasses through unchanged; absentoutputs/metadatastill default to{}and are accepted. Both covered.dictis correct here, notMapping: both values come fromjson.loadswith noobject_pairs_hook, andOrderedDict/defaultdictaredictsubclasses anyway. Plain Django view, so no DRFQueryDict/ReturnDictinvolved.Database Migrations
Env Config
Relevant Docs
Related Issues or PRs
Dependencies Versions
Notes on Testing
metadatarejected, other non-mappings rejected, the helper never reached for invalid input, dict outputs still reach it, absentoutputsstill accepted, the prompt advice absent formetadata, and four for the in-backend path including the negative case that the advice is omitted for single-prompt.Notes for the reviewer
_handle_responseis reached only from_execute_single_prompt/_execute_prompts_in_single_pass, and both only fromPromptStudioHelper.prompt_responder, which has zero callers in either repo — the live route isviews.single_pass_extraction→dispatch_with_callback→ executor →ide_prompt_complete→ this internal API. Kept as defence-in-depth; deleting the chain instead is a reasonable alternative.prompt_idsis deliberately unchecked. A non-list makesfilter()raiseValidationError, which the existingexceptturns into a 500 — but the producer always sends a list and theif not prompt_idscheck covers the realistic cases.json_repairnowhere, soanswer_prompt.py:438-442persists{}for any answer that is not strictly valid JSON. Declaring it here forced a ~2100-line rootuv.lockdowngrade from the uv-lock automation (pinned touv 0.6.14, older than whatever wroterevision = 3) that could not be reverted while the declaration was present. Tracked for a follow-up, along with pinning that workflow'suv.Screenshots
Checklist
I have read and understood the Contribution Guidelines.