UN-4017 [FIX] Recover single-pass output from malformed LLM JSON, with always-on parse diagnostics - #2250
Open
Deepak-Kesavan wants to merge 8 commits into
Open
UN-4017 [FIX] Recover single-pass output from malformed LLM JSON, with always-on parse diagnostics#2250Deepak-Kesavan wants to merge 8 commits into
Deepak-Kesavan wants to merge 8 commits into
Conversation
…LM JSON Single-pass extraction passes the parsed LLM response straight through as the outputs map. When the model wraps its JSON in prose, a reasoning block or a stray fence marker, the repair helper returns a list, and the backend 500s on outputs.get(prompt.prompt_key) in handle_prompt_output_update. The repair logic encodes two years of observed edge cases, so it is not touched. _repair_legacy holds it verbatim and callers that pass no contract reach exactly the same result as before — the new behaviour is unreachable without opting in. A frozen copy of the pre-change function lives in the tests; 3322 fuzzed inputs show zero differences on the default path, and any future edit to the legacy branch fails the pin. With a JsonContract the helper offers competing candidates — the legacy parse, the unwrapped parse, and five salvage strategies — and picks the one carrying the most expected prompt keys. Scoring rather than first-match matters: "first dict" returns a schema example quoted in the model's prose instead of the answer. Handling a new response shape is one @salvager plus a corpus entry. Responses carrying highlight/confidence metadata are never reshaped. Those `// 0x..` comments are mapped onto the parsed structure positionally, so dropping or merging an element silently attaches line numbers to the wrong fields — worse than the 500. Detected via `//` and `%%%`, with an opt-in escape hatch for callers that do no positional mapping. Adds diagnostics for the shapes this class of bug keeps producing. A PII-free trace (shapes, key counts, which parse step produced what) is always logged on failure and is safe in production. The raw response can additionally be captured behind DEBUG_LOG_RAW_LLM_RESPONSE, which is ignored unless REGION is non-prod so it cannot dump document content in US/EU. The webhook sink POSTs the untruncated payload out of band; the log sink elides the middle rather than the tail, where parse-breaking debris lives. Tests: 155 covering the legacy pin, model explanations, code blocks, token-limit truncation at each awkward boundary, annotated responses and the debug sinks.
A highlight or confidence mismatch is reported against a response that parsed perfectly well, so capturing only failures misses exactly the cases that generate those reports. The parse trace now runs on every repair — WARNING on failure, DEBUG on success — and carries request_id via the log filter plus execution_id explicitly. Adds a metadata alignment check for responses carrying `// 0x..` comments. It compares the comment stream against the structure those comments are mapped onto and warns on two signatures that are invisible in the extracted output: a count mismatch, where a phantom comment shifted every later field, and a malformed comment, where the greedy `//(.*)` capture swallowed document text and that field's line number is garbage while the counts still line up. Word-confidence blocks are stripped first, mirroring the consumer, so they do not invent leaves. This is the trail a "highlight is on the wrong line" report follows, and it detects the known `//`-inside-a-URL gap without fixing it. Raw payload capture stays behind DEBUG_LOG_RAW_LLM_RESPONSE and a non-prod REGION allowlist. DEBUG_LOG_RAW_LLM_MAX_CHARS=0 now disables clipping; staging sets it, since the log backend's own per-entry cap is the only real limit below the webhook sink.
Selecting the webhook sink is a decision to keep document content out of the log pipeline. Falling back to the log sink on a missing or mistyped DEBUG_RAW_LLM_WEBHOOK_URL inverted that, turning a config typo into a privacy regression. Fail closed and warn instead.
REGION carries two different things: geography in production (US, EU) and an environment name everywhere else (STAGING, DEV). Keying the raw-capture gate off it answered "is this production" only by coincidence, and the chart defaults REGION to "STAGING", so an environment that forgot to override it would have read as non-prod and opened the gate. DEPLOYMENT_ENV says only what it means and defaults to "production", so unset values, unrecognised values, and any region added in future all fail closed. REGION goes back to meaning geography.
Contributor
|
| Filename | Overview |
|---|---|
| workers/executor/executors/json_repair_helper.py | Adds contract-aware JSON salvage and privacy-conscious diagnostics; both previously reported diagnostic disclosure defects are fixed. |
| workers/tests/test_json_repair_characterisation.py | Adds legacy compatibility, malformed-response recovery, annotation safety, diagnostic redaction, clipping, and debug-sink tests. |
| workers/pyproject.toml | Declares json-repair as a direct worker dependency. |
| workers/sample.env | Documents structural diagnostics and gated raw-response debug settings. |
| workers/uv.lock | Updates the worker lockfile for the declared JSON repair dependency. |
| uv.lock | Regenerates the repository lockfile metadata and dependency resolution. |
Reviews (4): Last reviewed commit: "UN-4017 [FIX] Address review: input narr..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…clared
CI proved the ImportError fallback is live: every contract test failed with
"still a str" because json_repair is in no pyproject.toml and no uv.lock,
so `uv sync --locked` never installs it and _repair_legacy returns the raw
string for everything.
That is not confined to CI. The worker image builds the same way, and
json_repair reaches it only via the `uv pip install -e "$plugin_dir" || true`
step in worker-unified.Dockerfile — a step that swallows its own failure.
Wherever it is absent, every JSON-enforced prompt silently degrades to a raw
string and answer_prompt stores {} with no error.
Declared with a lower bound rather than a pin: the library is a lenient
parser whose heuristics are expected to evolve, and the salvagers are
verified against 0.42.0, 0.57.1 and 0.63.2 (the version this lock resolves).
The characterisation pin compares against whatever is installed, so it
cannot paper over a behaviour change.
![uv-lock-automation[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…imit=1
Two review findings, both real.
The shape trace claimed to be PII-free on the grounds that dict keys are
prompt field names rather than document content. That holds for a
well-formed response and this code runs on malformed ones, where repair
routinely promotes document text into key position — prose containing
"{field: value}" parses to a dict keyed on document text. Since the trace
is logged unconditionally, including in production, that was a leak.
Only keys the caller declared in its contract are echoed now; every other
key becomes its length.
_head_tail computed both halves as limit//2, so DEBUG_LOG_RAW_LLM_MAX_CHARS=1
gave a tail of 0 and text[-0:] is the whole string — the cap emitted the
entire payload it was set to bound.
…ve, guard
Six findings from review, all reproduced before fixing.
Input narrowing (breaks this PR's core promise). _is_annotated ran a regex
over the payload on every call, so repair_json_with_best_structure(b'{"a":1}')
raised TypeError where it previously returned {"a": 1} — json.loads accepts
bytes, so _repair_legacy always did. The diagnostics now ignore non-str
input. The differential fuzz covers bytes: 2102 inputs, 0 differences.
Alignment false positive on the exact class it was built for. The consumer
strips word-confidence blocks and THEN parses; the check stripped them for
the comment count but took leaves from a parse of the UNSTRIPPED text, so
every well-formed highlight+word-confidence response reported MISMATCH
(400 fields -> "comments=400 leaves=401"). Both counts now come from the
stripped text.
The reshape guard silently disabled the fix. Matching a bare "//" meant any
document value containing one — "N/A // pending", a path — was treated as
annotated, so the salvage was skipped and the AttributeError this PR exists
to prevent still happened. The annotation is hex, so require "// 0x"; the
word-confidence marker must appear as a pair, not as one stray separator.
The marker was hardcoded while the consumer reads WORD_CONFIDENCE_MARKER,
so an operator override broke the guard and the check silently. Same env
var, same default.
The webhook posted synchronously on the calling thread — up to its timeout
per prompt against a slow collector, on every repair, contradicting the
"cannot slow an extraction" claim. Dispatched on a daemon thread.
_log_cleansing_chain was unguarded, so any future addition inside it could
fail an extraction. Wrapped.
Also: absolute-value tests against dependency drift. The characterisation
pin cannot catch it — _original_impl calls the same installed json_repair,
so a version bump moves both sides together — and json-repair is declared
with a lower bound. And test hygiene: a real DNS call, a thread-local
LogContext leaking across tests, and an assertion that accepted either
outcome.
Contributor
Author
Review pass — findings addressedCorrection to an earlier claim in this PRI wrote that "the characterisation pin compares against whatever is installed, so it cannot paper over a behaviour change." That is backwards. Since One of those pins documents current behaviour rather than endorsing it: on the no-contract path a fenced response is shredded into Fixed
226 tests pass. Differential fuzz: 2102 inputs including bytes, 0 differences on the no-contract path. Deliberately not fixedThe consumer-side No activated call site in this repo — by designWorth stating plainly for reviewers: nothing in this repo passes a contract, so the reported 500 is not fixed by this PR alone. The only tracked caller is |
|
Contributor
Unstract test resultsPer-group results
Critical paths
|
Deepak-Kesavan
added a commit
that referenced
this pull request
Aug 20, 2026
…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.
Deepak-Kesavan
added a commit
that referenced
this pull request
Aug 20, 2026
…l API (#2251) * UN-4017 [FIX] Reject non-mapping outputs at the prompt-output internal 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. * UN-4017 [FIX] Say "JSON array", not "non-object JSON", in the outputs 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. * UN-4017 [FIX] Guard the in-backend execution path too, not just the internal 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. * UN-4017 [FIX] Guard metadata too, and fix two tests that proved less 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. * UN-4017 [FIX] Declare json-repair in the workers test group 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. * Commit uv.lock changes * UN-4017 [FIX] Address review: in-backend message, metadata guard, lockfile 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. * Commit uv.lock changes * Commit uv.lock changes * UN-4017 [MISC] Drop the json-repair declaration and the lockfile churn 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. * UN-4017 [FIX] Only offer the prompt advice for outputs, never for metadata 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. * UN-4017 [FIX] Declare json-repair so the real-repair tests run on this 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. * Commit uv.lock changes * UN-4017 [MISC] Revert the automation's root uv.lock churn 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. * Commit uv.lock changes * UN-4017 [MISC] Drop the json-repair declaration and the root lockfile 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.
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
1 participant
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
json-repair, which the workers imported but nothing declared.Why
output_manager_helper.py:179. Confirmed in production.json_repairwas installed nowhere: every contract test returned a raw string. The image builds the same way, so wherever it is absent every JSON prompt silently degrades to a raw string and stores{}with no error.How
_repair_legacyholds the existing logic verbatim, with a comment recording why the"[" + json_strwrap exists (fragmentation recovery) so nobody "cleans it up".JsonContract, the helper offers competing candidates — legacy parse, unwrapped parse, five salvage strategies — and picks the one carrying the most expected prompt keys. Scoring rather than first-match matters: "first dict" returns a schema example quoted in the model's prose instead of the answer.// 0x..metadata are never reshaped. Those comments are mapped onto the parsed structure positionally, so dropping or merging an element silently attaches line numbers to the wrong fields — worse than the 500. Detection requires the hex payload, because matching a bare//would treat ordinary document values ("N/A // pending", a path) as annotated and silently switch the fix off.@salvagerplus a corpus entry;_repair_legacystays closed for modification.request_idandexecution_id. Only contract-declared keys are echoed, because repair promotes document text into key position on malformed input.DEBUG_LOG_RAW_LLM_RESPONSEand a non-prodDEPLOYMENT_ENV, defaulting toproductionso unset and unrecognised values fail closed.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)
_repair_legacyfails that pin.bytesused to parse, then raisedTypeError), and the annotation guard was broad enough to disable the fix for documents containing//in a value.json-repairchanges what ships: builds that were silently running without it will now have it. That is the intended fix, but it is a real behaviour change for any such environment.Database Migrations
Env Config
DEBUG_LOG_RAW_LLM_RESPONSE(defaultfalse),DEBUG_LOG_RAW_LLM_SINK(log|webhook),DEBUG_RAW_LLM_WEBHOOK_URL,DEBUG_LOG_RAW_LLM_MAX_CHARS(0= no clipping),DEPLOYMENT_ENV(defaultproduction). All documented inworkers/sample.env; chart wiring in Zipstack/unstract-cloud#1738.Relevant Docs
Related Issues or PRs
Dependencies Versions
json-repair>=0.42inworkers/pyproject.toml; the lock resolves 0.63.2. A lower bound rather than a pin, by decision — the salvagers are verified against 0.42.0, 0.57.1 and 0.63.2.Notes on Testing
_original_implcalls the same installedjson_repair, so a version bump moves both sides together. One of those pins documents current behaviour rather than endorsing it: on the no-contract path a fenced response is shredded into["json\n{", 'invoice_number": "INV-001', {...}].Screenshots
Checklist
I have read and understood the Contribution Guidelines.