Sprint 4 [FIX] Backend fixes: UN-2900, UN-2902, UN-2953, UN-3038, UN-3133, UN-3176, UN-3333 - #2257
Sprint 4 [FIX] Backend fixes: UN-2900, UN-2902, UN-2953, UN-3038, UN-3133, UN-3176, UN-3333#2257hari-kuriakose wants to merge 12 commits into
Conversation
…OCR threshold UN-2953: validate_adapter_permissions read adapter ids straight out of tool_meta and added them unconditionally, so a tool instance holding "" for an adapter id made the JSON schema validator compare "" against the UUID enum and raise. The error was logged but not handled, repeating every validation pass until the pod stopped answering health checks. Skips empty/missing ids and uses .get() so a missing key no longer raises KeyError. Also initialises adapter_id per iteration -- previously a disabled entry could re-add the previous loop's id. UN-3038: push_usage_details billed len(pdf.pages) for every PDF, ignoring the adapter's pages_to_extract range, so a 5-page extraction from a 100-page document was charged 100 pages. Narrows the count to the selected pages, handling ranges, open-ended ranges, overlaps and out-of-range values, and falling back to the full count when the setting is absent or unparseable so usage is never under-reported. UN-3333: adds word_confidence_threshold to the LLMWhisperer v2 adapter schema (number, default 0.3, 0.0-1.0) so it is configurable from the adapter UI. The parameter is implemented in the LLMWhisperer backend but was never exposed. Not added to the v1 schema, which predates the OCR tuning parameters. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011sXFBEu2GHatXq2CV2ShPF
KeywordTableIndex's retriever caps results with `num_chunks_per_query` (default 10), not `similarity_top_k`. The code passed similarity_top_k, which as_retriever accepts and ignores, so the configured limit never took effect -- a profile set to 3 chunks still retrieved 10. Passes num_chunks_per_query instead. Filed as a frontend ticket, but the setting was being displayed correctly; only the retriever ignored it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011sXFBEu2GHatXq2CV2ShPF
_sanitize_for_bigquery documents a 15-significant-figure cap for PARSE_JSON compatibility, but derived a DECIMAL-place count from the value's magnitude. That only equals 15 significant figures for values >= 1. For 0.0053325 the magnitude is -2, so it asked for 17 decimals -- more precision than the safe zone permits -- and the value was returned unchanged. Formats with `.15g` so the significant-figure limit applies at any magnitude. Verified round-tripping for small values, Unix timestamps, large mantissas and binary-artifact values such as 0.1 + 0.2. NOTE: this corrects a real precision defect but is NOT confirmed to be the whole of the reported failure. The ticket's rejected value (0.0053325) is already representable and survives sanitization unchanged, so reproducing the BigQuery-side error needs the customer's table and PARSE_JSON expression. Flagged for follow-up rather than closed on this commit alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011sXFBEu2GHatXq2CV2ShPF
litellm's ContextWindowExceededError derives from BadRequestError, so it was caught by the generic openai.APIError branch in parse_litellm_err and wrapped as a plain SdkError reading "Error from <provider>." plus the raw 400 text. Users hitting the token limit saw a generic failure and had to read worker logs to find the cause (reported on execution f308a67f-02a1-437e-8531-05e067a94e02). Adds a ContextWindowExceededError SdkError subclass, maps it ahead of the generic wrap, and returns early so the actionable guidance (reduce chunk size, limit pages extracted, or use a larger-context model) is not overwritten by the generic tail. The provider's own text is kept in a code block underneath for support. Other litellm errors are unchanged. Context from the source thread: the customer's PRIMARY complaint there was inconsistent JSON structure across questions, which Jagadeesh identified as a prompting issue, not this one. Only the mis-surfaced token-limit error is in scope for UN-3133. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011sXFBEu2GHatXq2CV2ShPF
…traction
Single pass builds ONE combined prompt -- every field declared up front in a
single JSON schema, answered in one LLM call -- so no prompt's output exists to
feed another prompt's variable. The runtime reflects that: the enterprise
single_pass_extraction plugin calls the shared replacement service with
structured_output={}, and both replace_static_variable and
replace_dynamic_variable return the prompt UNCHANGED when their lookup misses.
The literal {{...}} is then sent to the LLM, silently degrading the answer.
This is not specific to custom_data, despite the ticket title. CUSTOM_DATA is
in fact the ONLY variable type that survives single pass, because it resolves
from the tool's own custom_data and never consults the variable map. STATIC and
DYNAMIC variables both fail on their own, with no custom_data involved:
static : "check {{invoice_number}}" -> "check {{invoice_number}}"
dynamic : "via {{https://.../x[cust_id]}}" -> unchanged
custom : "{{custom_data.client.name}}" -> "Acme GmbH" (works)
Adds find_unresolvable_single_pass_variables() and surfaces the result per
prompt as single_pass_unresolvable_variables when the tool has single-pass
enabled. Warning only -- deliberately NOT a save-time block, because existing
projects may already carry this combination and users toggle single pass on and
off; a hard refusal would break them retroactively and be order-dependent.
Classification pairs with VariableReplacementService in the worker, which keeps
its own copy of the variable regexes; noted in the docstring since drift would
make validation and runtime disagree.
Known gap: this covers Prompt Studio authoring, not an already-exported tool
running single pass via API deployment, which keeps failing silently until the
tool is re-saved. That argues for pairing this with placeholder-stripping at
runtime later, not for widening this change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011sXFBEu2GHatXq2CV2ShPF
Follow-up to f45d4e0, which computed the warning only in CustomToolSerializer.to_representation -- so it appeared on tool fetch but not on the prompt save response, and the UI had no fresh value after an edit. Moves the computation to ToolStudioPromptSerializer as a SerializerMethodField. That serializer is what the prompt CRUD view returns AND what CustomToolSerializer nests per prompt, so one implementation now covers both page load and save. Removes the duplicated assignment from CustomToolSerializer. Adds select_related("tool_id") to the prompt query in CustomToolSerializer.to_representation: the new field reads the parent tool's single_pass_extraction_mode, which would otherwise be one query per prompt -- the exact N+1 CustomToolListSerializer's docstring calls out. Verified: single-pass off -> [] regardless of content; on -> static and dynamic variables reported, custom_data excluded, empty prompt safe (5/5). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011sXFBEu2GHatXq2CV2ShPF
Every google.api_core BadRequest was mapped to ColumnMissingException, whose message tells the user to "make sure all the columns exist in your table as per the destination DB configuration". BigQuery also returns BadRequest for VALUE-level failures -- including this ticket's "cannot round-trip through string representation; error in PARSE_JSON expression" -- so users were sent to check a schema that was never wrong. That misdirection is likely why this was filed as a datatype-conversion bug. Adds BigQueryValueException and discriminates before wrapping: prefers the structured errors[] payload, falls back to message signatures for the round-trip / PARSE_JSON / invalid-JSON cases BigQuery does not tag. Anything unrecognised falls through to the existing ColumnMissingException, so this only narrows messages that were already wrong. Verified 7/7 including the ticket's verbatim error text and two genuine missing-column messages that must NOT be reclassified. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011sXFBEu2GHatXq2CV2ShPF
for more information, see https://pre-commit.ci
) #2 _sanitize_for_bigquery: the UN-3176 comment said the old magnitude-derived decimal count over-preserved precision for values BELOW 1, citing 0.0053325 as a value "passed through unchanged". That is inverted. For magnitude m < 0 the value carries |m| leading zeros after the point, so `15 - m` decimals preserves exactly 15 significant figures; the old form was already correct for every m <= 15. It over-preserves only ABOVE 10^15, where the count floors at 0 and the full integer part is emitted. A 200k-sample sweep over magnitudes 10^-12 to 10^+20 shows old and new differ only at 10^15 and above -- and old(0.0053325) == new(0.0053325) exactly. The code change is right; the stated cause was not. #5 _is_value_error: the docstring claimed it "prefers the structured errors payload" because "invalidQuery covers the value-level rejections". Neither is true -- the message text is checked first and returns before the payload is reached, and `reason` is never inspected at all. Restated to describe what the function does. The e.errors loop is kept and is provably live: str() of a google.api_core BadRequest is just "400 <message>" and omits the payload, so a marker present only there is still matched. #6 serializers.py: drop a stray blank line added by the diff. No behaviour change.
) Adversarial verification refuted the causal story in the previous commit. It claimed the old and new forms agree below 10^15. They do not: when |x| sits just below a power of ten, log10 returns an exact integer (the true value is within half an ULP), so the derived magnitude is one too large and the old form emitted 14 significant figures, not 15. Reproduced against the shipped function -- old(9.99999999999999e-05) == 0.0001 while new() preserves all 15 digits, and the same happens in every decade below 1e-4. That is the second false account of this line in as many commits, so this one stops narrating the old form's failure mode and states only the invariant that matters: `g` asks for significant figures, the old form asked for decimal places, and the two are not the same quantity. _is_value_error: the "str(e) does not include the payload" claim was true for the path this code sees but stated absolutely. GoogleAPICallError.__str__ (api-core 2.24.2) folds an errors entry into the string only when it exposes .code/.message ATTRIBUTES; BigQuery's REST path fills errors with plain dicts, which fail that hasattr filter. Says that instead. Verified the loop is still load-bearing: a marker present only in the dict payload is absent from str(e) ('400 Schema mismatch on insert') and the function still returns True. No behaviour change. unstract/connectors tests/databases: 36 passed.
The PR shipped no tests. These pin the three behaviours whose regressions would be silent, and each was mutation-checked -- reverted or neutered the fix, confirmed the test fails, restored. BigQuery BadRequest discrimination (UN-3176), in the existing test_bigquery_db.py alongside the Forbidden/NotFound cases: - a value-level BadRequest routes to BigQueryValueException, and the message no longer tells the user to check columns that were never wrong - the same, with the marker present ONLY in the structured errors payload. GoogleAPICallError.__str__ folds an entry into the string only when it has .code/.message ATTRIBUTES, and BigQuery's REST path supplies plain dicts, so this is reachable solely through the payload loop. Deleting that loop fails this test and nothing else -- verified. - a genuine schema BadRequest still routes to ColumnMissingException, so the discrimination cannot drift into matching everything. _sanitize_for_bigquery (UN-3176): the two values where the old magnitude- derived decimal count and `:.15g` actually disagree -- 1234567890123456.0 at the high end, and 9.99999999999999e-05 at the low end, where log10 returns an exact integer, inflating the magnitude and costing a significant figure. A round number like 3.14159 passes under both forms and would prove nothing. Plus the NaN/Inf/zero guards and nested-structure recursion. Page-range billing (UN-3038): _parse_pages_to_extract over single pages, ranges, open-ended ranges, overlaps, inverted ranges and out-of-range values; the four degenerate configs that must fall back to the full count rather than bill zero; and one test asserting the count Audit actually receives, because asserting on _get_billable_page_count alone still passes when the call is dropped from push_usage_details -- which is the only place the number becomes a bill. Confirmed: removing that call site fails only the new test. No new test files -- both suites extend files already in the repo. unstract/connectors tests/databases: 44 passed (was 36). unstract/sdk1: 570 passed (was 554); the 11 failures are pre-existing and byte-identical to the base commit (missing pytest-asyncio, network-bound bedrock tests).
|
|
| Filename | Overview |
|---|---|
| backend/prompt_studio/prompt_studio_core_v2/prompt_variable_service.py | Adds ordered detection of variables that single-pass extraction cannot resolve while exempting custom-data variables. |
| backend/prompt_studio/prompt_studio_v2/serializers.py | Exposes unresolved single-pass variables on prompt serialization for both detail and save responses. |
| backend/tool_instance_v2/tool_instance_helper.py | Makes adapter-ID collection tolerant of missing and empty metadata values before access validation. |
| unstract/connectors/src/unstract/connectors/databases/bigquery/bigquery.py | Corrects significant-figure sanitization and introduces targeted classification of BigQuery value errors. |
| unstract/connectors/src/unstract/connectors/databases/exceptions.py | Adds a user-facing exception for BigQuery row-value rejection. |
| unstract/sdk1/src/unstract/sdk1/exceptions.py | Maps LiteLLM context-window overflow to a distinct actionable SDK error. |
| unstract/sdk1/src/unstract/sdk1/x2txt.py | Parses configured page selections and reports the resulting PDF page count to usage accounting. |
| unstract/sdk1/src/unstract/sdk1/adapters/x2text/llm_whisperer_v2/src/static/json_schema.json | Adds a bounded OCR word-confidence threshold to LLMWhisperer v2 configuration. |
| workers/executor/executors/retrievers/keyword_table.py | Applies the requested retrieval limit using KeywordTableIndex’s effective chunk-count parameter. |
Reviews (1): Last reviewed commit: "Merge branch 'main' into un-sprint4-D-ba..." | Re-trigger Greptile
Unstract test resultsPer-group results
Critical paths
|
chandrasekharan-zipstack
left a comment
chandrasekharan-zipstack
left a comment
There was a problem hiding this comment.
Standardized review — High-severity findings only
Verdict: BLOCK — Critical 0 · High 6 · Medium 11 · Low 8 · Lenses 16/16.
This review posts only the 6 High findings. The 11 Medium and 8 Low are held back to keep the thread actionable; happy to post them on request.
Why BLOCK: three of the seven tickets do not deliver their stated fix (UN-3333, UN-3133, and UN-2953's documented mechanism); the BigQuery change introduces a silent data-corruption regression outside its ticket's scope that the PR's own test enshrines as intended; and the billing change silently under-reports on a class of input its docstring promises to protect against. Each is individually merge-blocking; together they mean the branch does not do what the description says.
Tests: both new suites were executed at PR head and pass — unstract/connectors 12 passed (full unit suite 71), unstract/sdk1 26 passed (full unit suite 579). Both are wired into CI via tests/groups.yaml. Mutation-checked: deleting the errors-payload loop in _is_value_error fails exactly its dedicated test; deleting the _get_billable_page_count call in push_usage_details fails exactly its dedicated test. All four numeric assertions independently verified.
On the authorization change — cleared, no privilege escalation
Since the permission rewrite is the riskiest-looking part of the diff, I traced it end to end rather than inferring:
adapter_idsfeeds exactly one consumer —AdapterInstance.objects.filter(id__in=adapter_ids)(tool_instance_helper.py:544).AdapterInstance.idis aUUIDFieldprimary key (backend/adapter_processor_v2/models.py:63), so""andNonecan never match a row. They contributed zero rows to the permission loop; dropping them removes no check. Pre-change,""did not fail closed by permission — it failed closed by crashing the ORM on UUID coercion.- Every truthy id still flows through unchanged, so a real adapter UUID the user cannot access still raises
PermissionDeniedexactly as before. - The only mechanism by which an absent id could later resolve to a real adapter is
DefaultsGeneratingValidator(unstract/sdk1/src/unstract/sdk1/tool/validator.py:26-33), whichsetdefaults schema defaults intotool_metaat:451— after the permission check. Prompt Studio specs do carry a real adapter UUID as a default (prompt_studio_registry_helper.py:87). Butset_defaultsyields intovalidate_propertiesin the same pass, and that enum is built fromAdapterProcessor.get_adapters_by_type(..., user=user)→AdapterInstance.objects.for_user(user). An injected default the user cannot access fails enum validation. Fail-closed. setdefaultdoes not overwrite a present-but-empty"", so that value is never upgraded to a real id.
What the change does do is convert two fail-closed crashes into silent passes — a robustness/observability regression, rated Medium (held back with the rest).
Open questions — the two that would move severities
- Is UN-3133 about the indexing/embedding path or the LLM completion path?
parse_litellm_erris only reachable fromembedding.pyandindex.py. If the ticket's repro is a prompt run, H2 stands and the fix belongs inllm.py. If it is an indexing failure, H2 resolves and only the message wording ("the document and prompt together") needs adjusting. - Does
Audit.push_page_usage_datafeed a charged meter or only telemetry? If it is billing, H4 is revenue-affecting and arguably Critical. - What was UN-2953's actual reported symptom? If it was the schema validator rejecting
""against the UUID enum, that was already fixed by #2209 (66a3363fb), whose comment inprompt_studio_registry_helper.py:67-72narrates exactly it — in which case this ticket may already be closed, and this diff is really three undocumented bug fixes. - Was the
%.15gchange intended to affect values ≥ 1e15? The ticket title says "for values below 1". - Is
word_confidence_thresholdmeant to ship inert here, with the wiring on another branch (as the description notes forsingle_pass_unresolvable_variables)? The description does not say so for UN-3333. - Was the keyword-table retrieval change evaluated against extraction quality? It silently changes answers for every existing user of that strategy (details in the held-back Mediums).
Assumptions that, if wrong, change severities
- Floats ≥ 1e15 reach
_sanitize_for_bigqueryin real tool-output metadata. If they never do, H3 drops to Low. - The LLMWhisperer service defaults read from
unstract-llm-whispererat its current HEAD match what is deployed. - Also noted: the PR description says "Six tickets" while the title and its own table list seven; and the three highest-risk items (UN-3038 billing, UN-2953 permissions, UN-3333 OCR default) get no "Notes for reviewers" entry while the three lower-risk ones do.
| "word_confidence_threshold": { | ||
| "type": "number", | ||
| "title": "Word confidence threshold", | ||
| "default": 0.3, | ||
| "minimum": 0.0, | ||
| "maximum": 1.0, | ||
| "description": "Minimum OCR confidence a word must reach to be included in the extracted text. Lower this when words are dropped because the scan is faint or noisy. Note: This parameter is not applicable if `mode` chosen is `native_text`." | ||
| }, |
There was a problem hiding this comment.
[High] [Lens 1, 3, 7] — word_confidence_threshold is a dead UI control, and its default is 6× the service's
This setting renders in the adapter form, persists into adapter_metadata, and is never sent to /whisper. A user with a faint scan lowers it, saves, re-runs — output is byte-identical. UN-3333 is not delivered.
Evidence: a repo-wide grep for word_confidence_threshold returns exactly one hit — this schema line. get_whisperer_params (.../src/helper.py:189-246) builds an explicit allowlist with no **config passthrough; WhispererConfig / WhispererDefaults (.../src/constants.py:60-127) have no such key.
Compounding it: the service default is 0.05 (unstract-llm-whisperer/backend/app/llm_whisperer_v2.py:144, providers/ocr/unstract_ocr_base.py:49, sample.env:65). Once someone does wire this up, default: 0.3 silently applies a 6× stricter word floor to every existing V2 adapter, dropping low-confidence words with no migration.
Note this same file already carries a comment about line_splitter_strategy having had this exact defect (helper.py:179-181) — this is the second instance.
Fix: wire WORD_CONFIDENCE_THRESHOLD through WhispererConfig + WhispererDefaults + get_whisperer_params (gated off native_text as the description claims), set the default to 0.05, and add a case to the params test file this PR already edits. Or drop the schema entry rather than ship a dead control.
Confidence: High.
| # UN-3133: surface a context-window overflow as its own error type with an | ||
| # actionable message. It is a BadRequestError subclass, so without this it | ||
| # collapses into the generic wrap below and the user sees only the | ||
| # provider's raw 400. | ||
| if isinstance(e, litellm_exceptions.ContextWindowExceededError): | ||
| err = ContextWindowExceededError( | ||
| ContextWindowExceededError.DEFAULT_MESSAGE, | ||
| actual_err=e, | ||
| status_code=status_code, | ||
| ) | ||
| # Return early: the generic tail below would overwrite the actionable | ||
| # guidance with "Error from <provider>." and leave only the raw 400. | ||
| # The provider text is kept underneath it for support/debugging. | ||
| err.message = ( | ||
| f"{ContextWindowExceededError.DEFAULT_MESSAGE}" | ||
| f"\n```\n{cleaned_message}\n```" | ||
| ) | ||
| return err |
There was a problem hiding this comment.
[High] [Lens 1, 3] — The UN-3133 fix cannot fire on the LLM completion path it describes
A prompt run whose document + prompt overflow the context window still shows Error from LLM adapter '<x>': <raw 400> — the exact symptom the ticket describes. This new branch only reaches embedding and indexing calls.
Evidence: parse_litellm_err has three call sites repo-wide — embedding.py:119,149,165,179,195 and workers/executor/executors/index.py:182. llm.py:18 imports only LLMError, SdkError, strip_litellm_prefix; the completion handler at llm.py:593-611 does except Exception as e: ... raise LLMError(message=f"Error from LLM adapter '{...}': {strip_litellm_prefix(str(e))}"), never consulting parse_litellm_err.
Fix: call parse_litellm_err from llm.py's generic handler before the LLMError fallback, or replicate the isinstance check there.
Confidence: High on reachability. See open question #1 in the review body — if UN-3133 is actually about indexing/embedding, this resolves and only the message wording is off.
| # form asked for a DECIMAL-place count derived from the magnitude, | ||
| # which is not the same quantity and drifted from 15 sig figs at | ||
| # both ends of the range. | ||
| return float(f"{data:.15g}") |
There was a problem hiding this comment.
[High] [Lens 3, 5] — %.15g destroys precision on ≥1e15 floats that the old code preserved exactly
A 16–17-significant-digit float that is exactly representable as an IEEE-754 double is now silently rewritten before landing in the customer's BigQuery table. No error, no log — the warehouse just holds a wrong number. The stated contract is "values that round-trip cleanly"; these values already did.
Both implementations run side by side:
1760509016282637.0 old=1760509016282637.0 new=1760509016282640.0 old_exact=True new_exact=False
1234567890123456.0 old=1234567890123456.0 new=1234567890123460.0 old_exact=True new_exact=False
9.99999999999999e-05 old=0.0001 new=9.99999999999999e-05 old_exact=False new_exact=True
Only the third line is the bug UN-3176 describes ("values below 1"). The first two are a new regression — and test_large_magnitude_is_limited_to_15_significant_figures (test_bigquery_db.py:206-211) locks it in as desired behaviour.
Fix: skip the cap when the value already round-trips — e.g. return data unchanged when data.is_integer() and abs(data) < 2**53, or when float(f"{data:.17g}") == data and the 15g form differs. Update the test accordingly.
Confidence: High on the numeric behaviour (independently reproduced). Medium on frequency — the sanitizer is only reached for floats nested in dict/list payloads (:270,287,410). If tool metadata carries epoch-microsecond floats or large numeric IDs, this is Critical.
| selected: set[int] = set() | ||
| for part in pages_to_extract.split(","): | ||
| part = part.strip() | ||
| if not part: | ||
| continue | ||
| if "-" in part: | ||
| start_str, _, end_str = part.partition("-") | ||
| try: | ||
| start = int(start_str) | ||
| except ValueError: | ||
| continue | ||
| end = total_pages | ||
| if end_str: | ||
| try: | ||
| end = int(end_str) | ||
| except ValueError: | ||
| continue | ||
| selected.update(range(max(start, 1), min(end, total_pages) + 1)) | ||
| else: | ||
| try: | ||
| page = int(part) | ||
| except ValueError: | ||
| continue | ||
| if 1 <= page <= total_pages: | ||
| selected.add(page) | ||
| return len(selected) |
There was a problem hiding this comment.
[High] [Lens 3, 16] — A partially-parseable pages_to_extract under-bills, contradicting the docstring's explicit guarantee
The docstring at :160-161 states usage is "never under-reported by a malformed value." That holds only when every component fails. One bad component is dropped by continue while previously-accumulated pages survive.
Confirmed against a 10-page document:
"3-x,5" -> billed 1 of 10
"1-3,junk,7" -> billed 4 of 10
"2-4,1x" -> billed 3 of 10
Silent revenue loss — and a maintainer trusting the docstring will not add validation.
:145-146's except ValueError: continue abandons the range component but leaves selected populated; :168's return selected or page_count only rescues the all-zero case.
Fix: track a parse_failed flag set by any of the four continue paths and return page_count when set; then correct the docstring.
Confidence: High. See open question #2 in the review body — if push_page_usage_data feeds a charged meter, this is arguably Critical.
| # UN-2953: a tool instance may carry "" for an adapter id. | ||
| # Adding it made the schema validator compare "" against the | ||
| # UUID enum and raise, once per validation pass. |
There was a problem hiding this comment.
[High] [Lens 16, 15] — This comment (repeated ×4, verbatim) names a mechanism that does not exist on this path, and hides the bugs actually fixed
Also at :509-511, :523-525, :535-537.
The comment says adding "" "made the schema validator compare "" against the UUID enum and raise." adapter_ids is a function-local set whose only consumer is validate_adapter_access (:553) → AdapterInstance.objects.filter(id__in=adapter_ids). No schema validator, no enum on that path. The only validator in this file (:451) validates tool_meta — a different input — and runs after this function returns. A maintainer will go read the JSON schema and find nothing.
The real failure was a Django UUID-coercion error (AdapterInstance.id is a UUIDField, backend/adapter_processor_v2/models.py:63), or the old KeyError.
And the comment never mentions the two other bugs this diff silently fixes: the old code left adapter_id unassigned when is_enabled was False, so adapter_ids.add(adapter_id) re-added the previous iteration's id (leaking across all four loops) or raised UnboundLocalError on the first. The new adapter_id = None initialiser is the load-bearing change and is undocumented.
Fix: one accurate comment above the loop group — # Skip unset adapter ids: "" is not a valid UUID and breaks the id__in filter. Hoist the four near-identical loops into a helper.
Confidence: High. Independently reached by three agents and a manual trace.
| if adapter_id: | ||
| adapter_ids.add(adapter_id) | ||
|
|
||
| ToolInstanceHelper.validate_adapter_access(user=user, adapter_ids=adapter_ids) |
There was a problem hiding this comment.
[High] [Lens 13 — Testing] — This permission-check rewrite ships with zero tests
Every tool_meta[...] became tool_meta.get(...) plus if adapter_id:, changing raise-semantics to skip-semantics on the sole input to an authorization check (validate_adapter_access, :553). Nothing exercises it.
grep -rn "validate_adapter_permissions" --include=*.py . returns only the definition (:482) and its one caller (:439) — no test file. The app's only suite, backend/tool_instance_v2/tests/test_challenge_llm_seed_overlay.py, touches neither adapters nor permissions.
Fix — three cases against a stubbed ToolProcessor.get_tool_by_uid:
""for an enabled adapter → no exception,""excluded from the set;- a real inaccessible adapter id →
PermissionDeniedstill raised; - key missing entirely → pin the intended contract explicitly (this is the case the
.get()widening newly swallows).
Confidence: High.
2 participants
Sprint 4 — backend fixes
Six tickets investigated and fixed across the backend and SDK. Grouped onto one branch because they were worked as a single sprint-4 investigation pass; each commit is self-contained and can be reviewed independently.
be9d7747da198d0eaa0df6f8aa4f086af45d4e0fc830880f1f8a4a7cNotes for reviewers
UN-3133 — litellm's
ContextWindowExceededErrorsubclassesBadRequestErrorand thereforeopenai.APIError, soparse_litellm_errcaught it in the generic branch and wrapped it as a plain "Error from <provider>" plus raw 400 text. It now maps ahead of the generic wrap, so the actionable message (reduce chunk size / limit pages / larger-context model) survives.UN-3176 —
bigquery.pymapped everygoogle.api_core.exceptions.BadRequesttoColumnMissingException, so a value-level failure told users to "make sure all the columns exist". Adds a_is_value_errordiscriminator; unrecognised shapes still fall through toColumnMissingException, so this only narrows messages that were already wrong.UN-2900 — the ticket title points at the one thing that works: under single pass,
custom_dataresolves correctly while every other variable type silently fails. This surfaces the unresolvable ones as an authoring-time warning rather than a save-time block, since existing projects may already carry the combination and blocking would break them retroactively.Caveats
single_pass_unresolvable_variablesfield; that lives on the C branch, which is inert without this one.🤖 Generated with Claude Code
https://claude.ai/code/session_0197gRFp4QFxu2VzxhTEozGn