Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Add a dedicated OpenAI-compatible LLM adapter#1895

Merged
athul-rs merged 18 commits into
Zipstack:mainZipstack/unstract:mainfrom
jimmyzhuu:codex/openai-compatible-llm-adapterjimmyzhuu/unstract:codex/openai-compatible-llm-adapterCopy head branch name to clipboard
May 19, 2026
Merged

Add a dedicated OpenAI-compatible LLM adapter#1895
athul-rs merged 18 commits into
Zipstack:mainZipstack/unstract:mainfrom
jimmyzhuu:codex/openai-compatible-llm-adapterjimmyzhuu/unstract:codex/openai-compatible-llm-adapterCopy head branch name to clipboard

Conversation

@jimmyzhuu

@jimmyzhuu jimmyzhuu commented Apr 4, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR adds a dedicated OpenAI Compatible LLM adapter for OpenAI-style chat completion endpoints that are not the official OpenAI service.

The implementation is intentionally small in scope:

  • adds a new OpenAI Compatible LLM adapter backed by LiteLLM's custom_openai path
  • keeps the existing OpenAI adapter unchanged
  • adds adapter registration and JSON schema
  • adds focused tests for registration, model normalization, schema loading, and usage recording
  • documents the new adapter in the README

Why

Users may already have access to OpenAI-compatible endpoints behind a private gateway or third-party provider, but the current OpenAI adapter is specifically shaped around official OpenAI semantics.

Using a separate adapter keeps those semantics explicit and avoids broadening the meaning of the existing OpenAI adapter.

Refs #1894
Refs #856
Refs #1443

Scope

This PR is limited to:

  • LLM only
  • no embedding changes
  • no x2text / OCR changes
  • no changes to the existing OpenAI adapter behavior

Notes

LLM._record_usage now prefers provider-reported prompt_tokens when they are present in the usage payload.

If prompt_tokens are missing, _record_usage still falls back to LiteLLM token estimation. If that estimation raises, it now logs a warning and records 0 prompt tokens for usage audit instead of bubbling the exception up after a successful LLM call.

This behavior change is in the shared _record_usage path, so it applies to every SDK1 LLM adapter that uses it, not just custom_openai.

This keeps successful LLM calls from failing at the usage-audit / billing step while preserving the current pricing semantics in this PR.

Validation

  • UV_SKIP_WHEEL_FILENAME_CHECK=1 uv run pytest tests/test_openai_compatible_adapter.py
  • UV_SKIP_WHEEL_FILENAME_CHECK=1 uv run ruff check src/unstract/sdk1/adapters/base1.py src/unstract/sdk1/adapters/llm1/__init__.py src/unstract/sdk1/adapters/llm1/openai_compatible.py src/unstract/sdk1/llm.py tests/test_openai_compatible_adapter.py

@CLAassistant

CLAassistant commented Apr 4, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@coderabbitai

coderabbitai Bot commented Apr 4, 2026

Copy link
Copy Markdown
Contributor

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

This PR adds support for OpenAI-compatible LLM providers (e.g., via LiteLLM) by introducing a new adapter type, parameter validation with model normalization, improved usage token handling, and corresponding tests and configuration.

Changes

OpenAI-Compatible LLM Adapter Feature

Layer / File(s) Summary
Parameter Definition
unstract/sdk1/src/unstract/sdk1/adapters/base1.py
OpenAICompatibleLLMParameters class adds api_key and api_base fields with validate() and validate_model() methods that prefix models with custom_openai/ and normalize empty API keys to None.
BaseAdapter Enhancement
unstract/sdk1/src/unstract/sdk1/adapters/base1.py
BaseAdapter gains SCHEMA_PATH class variable and get_json_schema() now respects custom schema paths for adapters with overridden paths.
Adapter Implementation
unstract/sdk1/src/unstract/sdk1/adapters/llm1/openai_compatible.py
OpenAICompatibleLLMAdapter class subclasses parameters and BaseAdapter, exposing fixed ID, metadata, name "OpenAI Compatible", provider "custom_openai", description, and icon through static methods.
Module Registration & Exports
unstract/sdk1/src/unstract/sdk1/adapters/llm1/__init__.py
OpenAICompatibleLLMAdapter is imported and added to __all__ for public module exposure.
Usage Recording
unstract/sdk1/src/unstract/sdk1/llm.py
_record_usage() now prefers provider-supplied prompt_tokens from usage_data; falls back to token_counter() if absent and logs a warning (with zero token count recorded) if token counting fails.
Configuration & Schema
unstract/sdk1/src/unstract/sdk1/adapters/llm1/static/openai_compatible.json
JSON Schema defines required fields (adapter_name, api_base) and optional fields (api_key, model, max_tokens, max_retries, timeout) with types and descriptions.
Documentation & Tests
README.md, unstract/sdk1/tests/test_openai_compatible_adapter.py
README updated to list "OpenAI Compatible" as a supported provider; comprehensive test suite validates adapter registration, parameter normalization, schema structure, metadata, and usage token recording edge cases.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive The PR description provides comprehensive coverage of What, Why, and Scope. However, it lacks several required template sections: 'How' details, breaking changes assessment, database migrations, env config, relevant docs, related issues clarity, dependencies versions, and testing notes. Fill in the missing template sections (How, breaking changes, databases, env config, docs, issues, dependencies, and testing notes) to provide complete context for reviewers and maintainers.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: adding a dedicated OpenAI-compatible LLM adapter. It directly matches the changeset's primary objective.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps

greptile-apps Bot commented Apr 4, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds a dedicated OpenAI Compatible LLM adapter backed by LiteLLM's custom_openai path, intended for OpenAI-style endpoints that are not the official OpenAI service. It also hardens LLM._record_usage to catch litellm.token_counter failures instead of propagating them after a successful LLM call.

  • New adapter (openai_compatible.py, base1.py, custom_openai.json): registers OpenAICompatibleLLMAdapter with model-prefix normalisation, optional api_key, and a focused JSON schema; validate_model raises early for empty models.
  • _record_usage resilience (llm.py): token_counter failures are now caught, logged as a warning, and recorded as 0 prompt tokens rather than bubbling up; affects all SDK1 LLM adapters.
  • Tests (test_openai_compatible_adapter.py): cover registration, model normalisation, schema content, and the three _record_usage paths (reported tokens, estimation fallback, estimation failure).

Confidence Score: 4/5

Safe to merge for users who always supply a key; keyless-endpoint configurations will either use an unintended process-level credential or fail with an auth error.

The blank api_key path is the main concern: a None value flows from model_dump() into LiteLLM's OpenAI client, which falls back to a process-level credential rather than sending no key — potentially leaking it to a custom third-party endpoint or breaking keyless setups entirely. All other logic (model prefix normalisation, adapter registration, schema loading, usage recording) is correct and well-tested.

unstract/sdk1/src/unstract/sdk1/adapters/base1.py — the OpenAICompatibleLLMParameters.validate() method and its model_dump() return value.

Security Review

  • Credential forwarding via client fallback (base1.py, OpenAICompatibleLLMParameters.validate): A blank api_key is normalised to None and returned as-is from model_dump(). The LiteLLM custom_openai provider receives this None value and its underlying HTTP client falls back to a process-level secret rather than sending no credential — potentially forwarding an unrelated service key to the user-supplied custom endpoint.

Important Files Changed

Filename Overview
unstract/sdk1/src/unstract/sdk1/adapters/base1.py Adds OpenAICompatibleLLMParameters; validate() normalises blank api_key to None but model_dump() includes it, risking credential leak or auth failure for keyless endpoints.
unstract/sdk1/src/unstract/sdk1/adapters/llm1/openai_compatible.py New adapter file; minimal and correct — ID, metadata, provider, icon, and adapter type all look right.
unstract/sdk1/src/unstract/sdk1/adapters/llm1/static/custom_openai.json New JSON schema; model is intentionally excluded from required (relying on Python-layer validation), consistent with the validate_model guard.
unstract/sdk1/tests/test_openai_compatible_adapter.py New test file covering registration, model normalisation, schema loading, and usage recording; magic module stub is documented inline.
unstract/sdk1/src/unstract/sdk1/adapters/llm1/init.py Adds import and all export for OpenAICompatibleLLMAdapter; straightforward.

Fix All in Claude Code

Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
unstract/sdk1/src/unstract/sdk1/adapters/base1.py:356-363
**Blank `api_key` becomes `None` in `model_dump()` output**

`validate()` normalises blank strings to `None`, but Pydantic's `model_dump()` includes `None` fields by default. The resulting dict is spread directly into `litellm.completion()`. For the `custom_openai` provider, LiteLLM initialises an OpenAI client; receiving a `None` key causes the client to fall back to any service key present in the process environment — potentially forwarding it to a third-party endpoint — or raises an auth error when no environment key exists, breaking keyless-endpoint configurations that the schema description explicitly supports.

Fix: pop the key field from the `model_dump()` result when it is `None`, matching the pattern in `_resolve_bedrock_aws_credentials` which explicitly drops optional credential fields.

Reviews (11): Last reviewed commit: "Merge branch 'main' into codex/openai-co..." | Re-trigger Greptile

Comment thread unstract/sdk1/tests/test_openai_compatible_adapter.py Outdated
Comment thread unstract/sdk1/tests/test_openai_compatible_adapter.py

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 1

🧹 Nitpick comments (1)
unstract/sdk1/src/unstract/sdk1/llm.py (1)

542-557: Avoid unconditional token estimation when usage already includes prompt tokens.

This currently computes token_counter() even when provider usage already has prompt tokens, which can create repeated warnings/noise for unmapped models without improving recorded usage.

♻️ Proposed refinement
-        try:
-            prompt_tokens = token_counter(model=model, messages=messages)
-        except Exception as e:
-            prompt_tokens = 0
-            logger.warning(
-                "[sdk1][LLM][%s][%s] Failed to estimate prompt tokens: %s",
-                model,
-                llm_api,
-                e,
-            )
         usage_data: Mapping[str, int] = usage or {}
+        prompt_tokens = usage_data.get("prompt_tokens")
+        if prompt_tokens is None:
+            try:
+                prompt_tokens = token_counter(model=model, messages=messages)
+            except Exception as e:
+                prompt_tokens = 0
+                logger.warning(
+                    "[sdk1][LLM][%s][%s] Failed to estimate prompt tokens: %s",
+                    model,
+                    llm_api,
+                    e,
+                )
         all_tokens = TokenCounterCompat(
-            prompt_tokens=usage_data.get("prompt_tokens", 0),
+            prompt_tokens=usage_data.get("prompt_tokens", prompt_tokens or 0),
             completion_tokens=usage_data.get("completion_tokens", 0),
             total_tokens=usage_data.get("total_tokens", 0),
         )
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@unstract/sdk1/src/unstract/sdk1/llm.py` around lines 542 - 557, The code
unconditionally calls token_counter(model, messages) even when usage already
contains prompt token counts; change the logic in the block around token_counter
and TokenCounterCompat so you first check usage (usage_data = usage or {}) and
if usage_data.get("prompt_tokens") is present use that value for prompt_tokens
instead of calling token_counter; only call token_counter(model, messages)
inside the try/except when usage_data lacks prompt_tokens, preserving the
existing exception handling and the logger.warning path, and then construct
TokenCounterCompat using the values from usage_data (falling back to the
estimated prompt_tokens when used).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@unstract/sdk1/src/unstract/sdk1/adapters/llm1/static/custom_openai.json`:
- Around line 15-20: The schema for the "api_key" property currently only allows
a string which fails when runtime metadata contains null; update the "api_key"
entry in the JSON schema (the "api_key" property in custom_openai.json) to
permit null values by changing its type to accept both string and null (or add a
nullable:true equivalent) so stored configs with null pass validation and
editing flows.

---

Nitpick comments:
In `@unstract/sdk1/src/unstract/sdk1/llm.py`:
- Around line 542-557: The code unconditionally calls token_counter(model,
messages) even when usage already contains prompt token counts; change the logic
in the block around token_counter and TokenCounterCompat so you first check
usage (usage_data = usage or {}) and if usage_data.get("prompt_tokens") is
present use that value for prompt_tokens instead of calling token_counter; only
call token_counter(model, messages) inside the try/except when usage_data lacks
prompt_tokens, preserving the existing exception handling and the logger.warning
path, and then construct TokenCounterCompat using the values from usage_data
(falling back to the estimated prompt_tokens when used).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: bf841637-54b7-4802-9156-7f56e899ca54

📥 Commits

Reviewing files that changed from the base of the PR and between 7983c98 and 5090773.

📒 Files selected for processing (7)
  • README.md
  • unstract/sdk1/src/unstract/sdk1/adapters/base1.py
  • unstract/sdk1/src/unstract/sdk1/adapters/llm1/__init__.py
  • unstract/sdk1/src/unstract/sdk1/adapters/llm1/openai_compatible.py
  • unstract/sdk1/src/unstract/sdk1/adapters/llm1/static/custom_openai.json
  • unstract/sdk1/src/unstract/sdk1/llm.py
  • unstract/sdk1/tests/test_openai_compatible_adapter.py

@jimmyzhuu

Copy link
Copy Markdown
Contributor Author

Addressed the review follow-ups.

  • allowed api_key = null in the OpenAI-compatible schema
  • avoided redundant token_counter() calls when provider usage already includes prompt_tokens
  • tightened the tests around both cases
  • added ERNIE / Baidu Qianfan as schema examples while keeping the adapter generic

Validation re-run:

  • UV_SKIP_WHEEL_FILENAME_CHECK=1 uv run pytest tests/test_openai_compatible_adapter.py
  • UV_SKIP_WHEEL_FILENAME_CHECK=1 uv run ruff check src/unstract/sdk1/llm.py tests/test_openai_compatible_adapter.py

@jimmyzhuu

Copy link
Copy Markdown
Contributor Author

Gentle follow-up on this PR in case it slipped through the queue. When someone has bandwidth, I would really appreciate a review. Happy to make any follow-up changes quickly. Thanks!

Comment thread unstract/sdk1/src/unstract/sdk1/llm.py Outdated
@jaseemjaskp jaseemjaskp self-requested a review April 23, 2026 09:01

@jaseemjaskp jaseemjaskp left a comment

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.

PR Review Toolkit — consolidated findings

Automated review aggregating six specialist agents (code-reviewer, code-simplifier, silent-failure-hunter, type-design-analyzer, pr-test-analyzer, comment-analyzer). No blocking defects; the adapter follows existing sibling-adapter conventions and the scope is appropriately narrow.

High-signal items worth addressing before merge

  • llm.py:556 — redundant/confusing fallback: prompt_tokens is already resolved at line 543 with an explicit estimation branch, so the usage_data.get("prompt_tokens", prompt_tokens or 0) expression double-handles the default and, worse, silently coerces an explicit None from usage_data to 0 without logging.
  • llm.py:547 — broad except Exception combined with a warning that says "failed to estimate" but not "recording 0 tokens" means billing/audit can silently under-report. Narrow the exception and either use logger.exception or rewrite the message to name the consequence.
  • base1.py:232 / schema api_base — the Pydantic type is plain str; URL shape lives only in the JSON schema, so direct construction accepts garbage. Consider HttpUrl / a field_validator.
  • base1.py:239 — prefix logic is only invoked by the validate classmethod. AzureOpenAILLMParameters uses @model_validator(mode="before") which cannot be bypassed; mirroring that tightens the invariant.
  • custom_openai.jsonapi_base is listed as required yet ships with a placeholder default URL, which lets users save an unchanged form and hit 404s at request time instead of validation errors. Vendor-specific examples (ERNIE-4.0-8K (Baidu Qianfan), qianfan.baidubce.com) are prone to rot and should be generic.
  • test_openai_compatible_adapter.py — the "tolerates unmapped models" test only asserts push_usage_data.assert_called_once(); it does not verify prompt_tokens=0 was actually pushed, so a regression silently pushing None or crashing inside TokenCounterCompat would still pass.

Suggested follow-ups (non-blocking)

  • Add tests for: api_base missing (Pydantic ValidationError), usage=None, usage["prompt_tokens"]=None, and the success branch of token_counter.
  • Drop the unused @lru_cache on _load_llm_module and the dead _load_llm_class.
  • Replace the tautological get_description() / metadata description with user-facing copy that distinguishes this adapter from OpenAILLMAdapter.

Inline comments below flag each item at its exact line with a concrete fix suggestion.

Comment thread unstract/sdk1/src/unstract/sdk1/adapters/llm1/openai_compatible.py Outdated
Comment thread unstract/sdk1/src/unstract/sdk1/adapters/llm1/openai_compatible.py Outdated
@sonarqubecloud

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 1

🧹 Nitpick comments (5)
unstract/sdk1/tests/test_openai_compatible_adapter.py (3)

102-192: LGTM — good coverage of the three _record_usage branches.

Tests exercise: (a) provider-supplied prompt_tokens bypasses token_counter, (b) token_counter raising falls back to 0 with a warning, and (c) prompt_tokens=None triggers estimation. The use of __new__ to bypass __init__ and the targeted patch.object(llm_module, ...) are appropriate here. Nice to see the warning-message assertion on line 162 pinning the audit-visible text.

One small suggestion: assert mock_warning is called exactly once and with model/llm_api substituted into the format string, to catch regressions that accidentally change the log signature.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@unstract/sdk1/tests/test_openai_compatible_adapter.py` around lines 102 -
192, Update the
test_record_usage_tolerates_unmapped_models_without_prompt_tokens test to assert
the warning logger was called exactly once and that the warning message includes
the model ("custom_openai/gateway-model") and llm_api ("complete") values;
locate the test function and the mock_warning (patched via
patch.object(llm_module.logger, "warning")) and after calling llm._record_usage
add assertions that mock_warning.assert_called_once() and that the call_args
contains both the model and llm_api strings in the formatted warning message to
catch signature regressions.

18-31: Minor: lru_cache around import_module is largely redundant.

sys.modules already caches modules after first import, so the lru_cache(maxsize=1) only saves the patch.dict context-manager overhead. Leaving it is harmless, but on second and subsequent calls the magic stub will not be re-installed (because the cached branch returns early), so any test that newly triggers import magic after the first call would see the real module. Not a problem today, but worth documenting with a short comment to prevent surprise.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@unstract/sdk1/tests/test_openai_compatible_adapter.py` around lines 18 - 31,
The `@lru_cache` on _load_llm_module() prevents the patch.dict stub for "magic"
from being re-applied on subsequent calls, which can lead to surprising behavior
if tests later import the real magic module; either remove the `@lru_cache`
decorator or (preferred) keep it but add a brief comment inside _load_llm_module
explaining that sys.modules already caches imports and that the cached result
means the "magic" stub will not be re-installed on later calls so tests should
call this once or manage stubbing themselves — reference the _load_llm_module
function and the patch.dict usage when adding the comment.

34-35: Dead helper: _load_llm_class is never called.

Every test uses _load_llm_module().LLM directly (e.g., lines 104, 135, 167). Consider removing _load_llm_class or using it in place of the inline llm_module.LLM lookups for consistency.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@unstract/sdk1/tests/test_openai_compatible_adapter.py` around lines 34 - 35,
The helper function _load_llm_class is unused (dead code); either remove
_load_llm_class entirely or replace direct usages of _load_llm_module().LLM in
tests (e.g., the inline lookups at places that call llm_module.LLM) with calls
to _load_llm_class() for consistency. Locate the definition of _load_llm_class
and the test files referencing _load_llm_module().LLM and either delete the
unused _load_llm_class function or update those tests to call _load_llm_class()
instead, ensuring imports and type annotations still match.
unstract/sdk1/src/unstract/sdk1/adapters/base1.py (1)

234-242: Minor: validate() mutates the caller's dict.

Lines 236 and 241 write back into adapter_metadata (same pattern as OpenAILLMParameters, but unlike VertexAILLMParameters which copies first via {**adapter_metadata}). Given LLM.complete() calls self.adapter.validate({**self.kwargs, **kwargs}) (a fresh dict), there's no current bug — but if a future caller passes a long-lived dict, the api_key would be mutated in place and the model prefix would double-rewrite on a second call (the startswith("custom_openai/") guard in validate_model mitigates the latter).

Optional: copy first for defensive hygiene, matching the VertexAILLMParameters.validate() style.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@unstract/sdk1/src/unstract/sdk1/adapters/base1.py` around lines 234 - 242,
The validate() method currently mutates the incoming adapter_metadata dict (it
writes to adapter_metadata["model"] and adapter_metadata["api_key"]); to avoid
in-place side effects make a shallow copy first (e.g., metadata =
{**adapter_metadata}) and perform all modifications against that copy before
passing it to OpenAICompatibleLLMParameters.validate_model and constructing
OpenAICompatibleLLMParameters(**metadata).model_dump(); keep references to the
same symbols (validate, OpenAICompatibleLLMParameters.validate_model,
OpenAICompatibleLLMParameters, adapter_metadata) so the change is local and
preserves existing behavior while preventing caller dict mutation.
unstract/sdk1/src/unstract/sdk1/llm.py (1)

557-557: Minor: prompt_tokens or 0 also zeroes out legitimate 0 from provider.

If a provider ever reports usage.prompt_tokens == 0 (unusual, but possible for zero-content requests or certain gateways), the truthiness check collapses it the same as None. Given the prompt_tokens is None branch already assigns an int (or 0 on exception), this or 0 is only needed to satisfy the type checker. A more precise form:

Proposed tweak
-    all_tokens = TokenCounterCompat(
-        prompt_tokens=prompt_tokens or 0,
+    all_tokens = TokenCounterCompat(
+        prompt_tokens=prompt_tokens if prompt_tokens is not None else 0,
         completion_tokens=usage_data.get("completion_tokens", 0),
         total_tokens=usage_data.get("total_tokens", 0),
     )

Low-impact since 0 vs None ends up the same in the audit row, but semantically cleaner.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@unstract/sdk1/src/unstract/sdk1/llm.py` at line 557, Replace the truthiness
fallback that zeroes out legitimate zero values: instead of using
"prompt_tokens=prompt_tokens or 0" keep the explicit None-check so only None
becomes 0 (e.g., use a conditional expression that assigns prompt_tokens if
prompt_tokens is not None else 0). Locate the occurrence of the
"prompt_tokens=prompt_tokens or 0" assignment and change it to an explicit
None-check for the variable prompt_tokens so a reported 0 remains 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@unstract/sdk1/src/unstract/sdk1/llm.py`:
- Around line 543-560: The current catch in the prompt token estimation around
token_counter (used when building TokenCounterCompat) silently sets
prompt_tokens=0; update this to (1) narrow the except to only expected errors
from the estimator (e.g., KeyError/ValueError and litellm-specific exceptions
raised by token_counter) so unexpected errors still propagate, and (2) add a
sentinel field to the usage payload (e.g., prompt_tokens_source or
estimation_failed) before calling Audit().push_usage_data to mark that prompt
tokens were estimated/failed, and/or increment an ops metric/counter when the
fallback path occurs; reference the token_counter call, TokenCounterCompat
construction, Audit().push_usage_data, and the existing logger to emit a clear
warning and metric.

---

Nitpick comments:
In `@unstract/sdk1/src/unstract/sdk1/adapters/base1.py`:
- Around line 234-242: The validate() method currently mutates the incoming
adapter_metadata dict (it writes to adapter_metadata["model"] and
adapter_metadata["api_key"]); to avoid in-place side effects make a shallow copy
first (e.g., metadata = {**adapter_metadata}) and perform all modifications
against that copy before passing it to
OpenAICompatibleLLMParameters.validate_model and constructing
OpenAICompatibleLLMParameters(**metadata).model_dump(); keep references to the
same symbols (validate, OpenAICompatibleLLMParameters.validate_model,
OpenAICompatibleLLMParameters, adapter_metadata) so the change is local and
preserves existing behavior while preventing caller dict mutation.

In `@unstract/sdk1/src/unstract/sdk1/llm.py`:
- Line 557: Replace the truthiness fallback that zeroes out legitimate zero
values: instead of using "prompt_tokens=prompt_tokens or 0" keep the explicit
None-check so only None becomes 0 (e.g., use a conditional expression that
assigns prompt_tokens if prompt_tokens is not None else 0). Locate the
occurrence of the "prompt_tokens=prompt_tokens or 0" assignment and change it to
an explicit None-check for the variable prompt_tokens so a reported 0 remains 0.

In `@unstract/sdk1/tests/test_openai_compatible_adapter.py`:
- Around line 102-192: Update the
test_record_usage_tolerates_unmapped_models_without_prompt_tokens test to assert
the warning logger was called exactly once and that the warning message includes
the model ("custom_openai/gateway-model") and llm_api ("complete") values;
locate the test function and the mock_warning (patched via
patch.object(llm_module.logger, "warning")) and after calling llm._record_usage
add assertions that mock_warning.assert_called_once() and that the call_args
contains both the model and llm_api strings in the formatted warning message to
catch signature regressions.
- Around line 18-31: The `@lru_cache` on _load_llm_module() prevents the
patch.dict stub for "magic" from being re-applied on subsequent calls, which can
lead to surprising behavior if tests later import the real magic module; either
remove the `@lru_cache` decorator or (preferred) keep it but add a brief comment
inside _load_llm_module explaining that sys.modules already caches imports and
that the cached result means the "magic" stub will not be re-installed on later
calls so tests should call this once or manage stubbing themselves — reference
the _load_llm_module function and the patch.dict usage when adding the comment.
- Around line 34-35: The helper function _load_llm_class is unused (dead code);
either remove _load_llm_class entirely or replace direct usages of
_load_llm_module().LLM in tests (e.g., the inline lookups at places that call
llm_module.LLM) with calls to _load_llm_class() for consistency. Locate the
definition of _load_llm_class and the test files referencing
_load_llm_module().LLM and either delete the unused _load_llm_class function or
update those tests to call _load_llm_class() instead, ensuring imports and type
annotations still match.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: bb92694d-b745-40e9-8bd9-0bc1fa3628b1

📥 Commits

Reviewing files that changed from the base of the PR and between d3e1cad and f6a2a7d.

⛔ Files ignored due to path filters (1)
  • frontend/public/icons/adapter-icons/OpenAICompatible.png is excluded by !**/*.png
📒 Files selected for processing (5)
  • unstract/sdk1/src/unstract/sdk1/adapters/base1.py
  • unstract/sdk1/src/unstract/sdk1/adapters/llm1/openai_compatible.py
  • unstract/sdk1/src/unstract/sdk1/adapters/llm1/static/custom_openai.json
  • unstract/sdk1/src/unstract/sdk1/llm.py
  • unstract/sdk1/tests/test_openai_compatible_adapter.py
✅ Files skipped from review due to trivial changes (2)
  • unstract/sdk1/src/unstract/sdk1/adapters/llm1/openai_compatible.py
  • unstract/sdk1/src/unstract/sdk1/adapters/llm1/static/custom_openai.json

Comment thread unstract/sdk1/src/unstract/sdk1/llm.py Outdated
Comment thread unstract/sdk1/src/unstract/sdk1/adapters/base1.py
@jimmyzhuu jimmyzhuu closed this May 6, 2026
@athul-rs

athul-rs commented May 6, 2026

Copy link
Copy Markdown
Contributor

Hi @jimmyzhuu - sorry we let this sit so long without a review, that's on us. The change looks reasonable in scope and the validation looks solid. If you're still interested, we'd be happy to have you reopen it and we'll get a maintainer on it this week. Either way, thanks for the careful work and the patience.
cc: @hari-kuriakose @ritwik-g @jaseemjaskp

@jimmyzhuu jimmyzhuu reopened this May 6, 2026
@jimmyzhuu

Copy link
Copy Markdown
Contributor Author

Absolutely — I’d be happy to work on this. Thanks for the suggestion!

@jimmyzhuu

Copy link
Copy Markdown
Contributor Author

@jimmyzhuu The one thing I'd note is that the exception-swallowing path now applies to every adapter, not just custom_openai. I think that's the right tradeoff (a successful LLM call shouldn't fail at the billing step), but flagging it for the merge commit.我注意到的一点是,现在异常捕获路径适用于所有适配器,而不仅仅是 custom_openai。我认为这是正确的权衡(一个成功的 LLM 调用不应该在计费步骤失败),但为合并提交标记一下。 Could you call out the _record_usage change explicitly in the PR description?你能明确在 PR 描述中提到 _record_usage 的变更吗? cc: @hari-kuriakose @pk-zipstack

@athul-rs Updated the PR description.

Resolve conflict in unstract/sdk1/src/unstract/sdk1/llm.py by taking
main's version of _record_usage. This PR's prompt_tokens fallback is
subsumed by main's existing litellm.token_counter() fallback, and the
TokenCounterCompat / Audit().push_usage_data() shape this PR retained
is dead post-Lookups-V2 refactor (usage now flows through
self._pending_usage).

Update the three _record_usage tests in
test_openai_compatible_adapter.py to assert on the _pending_usage
carrier rather than the removed Audit pattern. Also fix a latent
sys.modules rollback in _load_llm_module that broke litellm attribute
resolution after the patch.dict context exited.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment thread unstract/sdk1/src/unstract/sdk1/adapters/base1.py
validate_model previously produced "custom_openai/" for an empty model,
surfacing as a confusing LiteLLM error at call time. Match the existing
GeminiLLMParameters.validate_model pattern: strip whitespace, raise
ValueError on empty input.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment thread unstract/sdk1/src/unstract/sdk1/adapters/base1.py Outdated
@ritwik-g ritwik-g added enhancement New feature or request good first issue Good for newcomers labels May 11, 2026
Addresses Ritwik's review feedback. The new BaseAdapter.SCHEMA_PATH
class variable and the conditional branch in get_json_schema() are
unnecessary: OpenAICompatibleLLMAdapter.get_provider() returns
"custom_openai", and the default path resolution already builds
…/llm1/static/{get_provider()}.json. Renaming the schema file lets
the default lookup find it and keeps the base class untouched, which
is the convention every other adapter follows.

- Rename openai_compatible.json -> custom_openai.json
- Drop SCHEMA_PATH class var and the if-None branch from BaseAdapter
- Drop SCHEMA_PATH override (and unused os/ClassVar imports) from
  OpenAICompatibleLLMAdapter
- Update test_openai_compatible_schema_is_loadable to read schema via
  get_json_schema() instead of touching SCHEMA_PATH directly
@athul-rs

Copy link
Copy Markdown
Contributor

Hi @jimmyzhuu — thanks for the PR! I pushed a small follow-up commit (27215c2) addressing @ritwik-g's
▎ review on base1.py.

▎ The change keeps BaseAdapter untouched by aligning with the convention every other adapter follows: name
▎ the schema file after get_provider(). Since OpenAICompatibleLLMAdapter.get_provider() returns
▎ "custom_openai", renaming the JSON from openai_compatible.json → custom_openai.json lets the default
▎ get_json_schema() path resolution find it, so the introduced SCHEMA_PATH class variable and the if
▎ schema_path is None branch are no longer needed.

▎ Specifically:
▎ - Renamed unstract/sdk1/src/unstract/sdk1/adapters/llm1/static/openai_compatible.json → custom_openai.json
▎ - Dropped SCHEMA_PATH and the ClassVar import from BaseAdapter; simplified get_json_schema() back to the
▎ default path build
▎ - Dropped the SCHEMA_PATH override and unused os / ClassVar imports from OpenAICompatibleLLMAdapter
▎ - Updated test_openai_compatible_schema_is_loadable to read via get_json_schema() instead of touching
▎ SCHEMA_PATH directly

Comment thread README.md
@sonarqubecloud

Copy link
Copy Markdown

Comment thread unstract/sdk1/src/unstract/sdk1/adapters/base1.py
@athul-rs athul-rs merged commit 0559057 into Zipstack:main May 19, 2026
9 checks passed
harini-venkataraman pushed a commit that referenced this pull request May 21, 2026
* Add OpenAI-compatible LLM adapter

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Address review feedback for custom OpenAI adapter

* Fix import formatting after rebase

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Address follow-up review comments for OpenAI-compatible adapter

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Refine OpenAI compatible adapter schema naming

* Reject empty model string in OpenAICompatibleLLMParameters

validate_model previously produced "custom_openai/" for an empty model,
surfacing as a confusing LiteLLM error at call time. Match the existing
GeminiLLMParameters.validate_model pattern: strip whitespace, raise
ValueError on empty input.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* Revert SCHEMA_PATH plumbing; rename schema to custom_openai.json

Addresses Ritwik's review feedback. The new BaseAdapter.SCHEMA_PATH
class variable and the conditional branch in get_json_schema() are
unnecessary: OpenAICompatibleLLMAdapter.get_provider() returns
"custom_openai", and the default path resolution already builds
…/llm1/static/{get_provider()}.json. Renaming the schema file lets
the default lookup find it and keeps the base class untouched, which
is the convention every other adapter follows.

- Rename openai_compatible.json -> custom_openai.json
- Drop SCHEMA_PATH class var and the if-None branch from BaseAdapter
- Drop SCHEMA_PATH override (and unused os/ClassVar imports) from
  OpenAICompatibleLLMAdapter
- Update test_openai_compatible_schema_is_loadable to read schema via
  get_json_schema() instead of touching SCHEMA_PATH directly

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Hari John Kuriakose <hari@zipstack.com>
Co-authored-by: Chandrasekharan M <chandrasekharan@zipstack.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Athul <athul@zipstack.com>
Co-authored-by: Athul <89829560+athul-rs@users.noreply.github.com>
Co-authored-by: vishnuszipstack <117254672+vishnuszipstack@users.noreply.github.com>
harini-venkataraman added a commit that referenced this pull request Jun 29, 2026
)

* [MISC] Decommission prompt-service, old tools, and SDK1 prompt module (Phase 5)

Remove prompt-service source, Dockerfiles, and docker-compose entries.
Remove tools/classifier, tools/structure, tools/text_extractor directories.
Remove SDK1 prompt.py module and its tests.
Clean up PROMPT_HOST/PROMPT_PORT from backend settings, sample envs,
docker configs, and CI workflows. Remove prompt-service from uv-lock
scripts and production build workflow.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* [MISC] Remove prompt-service from tox.ini env_list

The prompt-service directory was deleted in the prior commit but tox.ini
still referenced it, which would break CI test runs.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* UN-2888 [FIX] Add hook for setting default triad for invited users (#1877)

* [FIX] Add hook for setting default adapters for invited users

Add setup_default_adapters_for_user() hook to AuthenticationService
and call it from set_user_organization() when an invited user joins
an existing organization. This allows the cloud plugin to set up
default triad adapters (LLM, embedding, vector DB, x2text) for
invited users, fixing silent failures in API deployment creation.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Update backend/account_v2/authentication_controller.py

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Signed-off-by: Praveen Kumar <praveen@zipstack.com>

* [FIX] Improve log message for setup_default_adapters_for_user

Address review comment: log user email and explain that default
adapters will not be set when the method is not implemented.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* [MISC] Rename Default Triad to Default LLM Profile in UI

Update display label from "Default Triad" to "Default LLM Profile"
in the page heading and side navigation menu.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Signed-off-by: Praveen Kumar <praveen@zipstack.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: Deepak K <89829542+Deepak-Kesavan@users.noreply.github.com>

* UN-3465 [FIX] Wrap set_user_organization in transaction.atomic (#1954)

* [FIX] Wrap set_user_organization in transaction.atomic

The new-org branch creates the org row, then calls frictionless onboarding
and the initial platform key. Failures mid-flow leave an orphan org with no
adapters or key, and subsequent logins skip onboarding entirely (gated on
new_organization). Atomic ensures the org rolls back on any failure so
retries get a clean fresh-org path.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* [MISC] Worktree skill — use --no-track to prevent accidental main pushes

Without --no-track, a later `git push -u origin <branch>` can be reported
by the server as also fast-forwarding main, landing commits on main.

* [FIX] Use logger.exception in authorization_callback

Preserves the traceback when the OAuth callback hits the safety-net
catch. Behaviour unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Athul <89829560+athul-rs@users.noreply.github.com>
Co-authored-by: vishnuszipstack <117254672+vishnuszipstack@users.noreply.github.com>

* UN-3386 [FEAT] Add Prompt Studio HITL change indicator plugin slot (#1930)

* UN-3386 [FEAT] Add Prompt Studio HITL change indicator plugin slot

Wires up the host-side hooks for the prompt-change-indicator plugin
(implementation lives in unstract-cloud): a dynamic-import slot in
the prompt card Header for the indicator button, and a route at
:orgName/review/readonly/:documentId for the read-only audit view.
Both gates fall through gracefully when the plugin is absent (OSS).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* UN-3386 [FIX] Warn when ReadOnlyReviewPage loads without ReviewLayout

Addresses review feedback: the readonly route nests inside ReviewLayout
(manual-review plugin), so a deployment that ships prompt-change-indicator
without manual-review would silently fail to register the route. Log a
console.warn in that case to make the misconfiguration discoverable.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* UN-3386 [FIX] Surface real plugin import errors in route loader

Bare catch in the prompt-change-indicator dynamic import was swallowing
syntax/runtime errors in the plugin file alongside the expected
"plugin missing in OSS" case. Detect the missing-module messages
explicitly and console.error anything else so a broken cloud plugin
no longer disables the readonly route silently.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* Add a dedicated OpenAI-compatible LLM adapter (#1895)

* Add OpenAI-compatible LLM adapter

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Address review feedback for custom OpenAI adapter

* Fix import formatting after rebase

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Address follow-up review comments for OpenAI-compatible adapter

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Refine OpenAI compatible adapter schema naming

* Reject empty model string in OpenAICompatibleLLMParameters

validate_model previously produced "custom_openai/" for an empty model,
surfacing as a confusing LiteLLM error at call time. Match the existing
GeminiLLMParameters.validate_model pattern: strip whitespace, raise
ValueError on empty input.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* Revert SCHEMA_PATH plumbing; rename schema to custom_openai.json

Addresses Ritwik's review feedback. The new BaseAdapter.SCHEMA_PATH
class variable and the conditional branch in get_json_schema() are
unnecessary: OpenAICompatibleLLMAdapter.get_provider() returns
"custom_openai", and the default path resolution already builds
…/llm1/static/{get_provider()}.json. Renaming the schema file lets
the default lookup find it and keeps the base class untouched, which
is the convention every other adapter follows.

- Rename openai_compatible.json -> custom_openai.json
- Drop SCHEMA_PATH class var and the if-None branch from BaseAdapter
- Drop SCHEMA_PATH override (and unused os/ClassVar imports) from
  OpenAICompatibleLLMAdapter
- Update test_openai_compatible_schema_is_loadable to read schema via
  get_json_schema() instead of touching SCHEMA_PATH directly

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Hari John Kuriakose <hari@zipstack.com>
Co-authored-by: Chandrasekharan M <chandrasekharan@zipstack.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Athul <athul@zipstack.com>
Co-authored-by: Athul <89829560+athul-rs@users.noreply.github.com>
Co-authored-by: vishnuszipstack <117254672+vishnuszipstack@users.noreply.github.com>

* ReverseMerge: V0.163.4 hotfix (#1980)

* [HOTFIX] Use importlib.util.find_spec for pluggable worker discovery (#1918)

* [FIX] Use importlib.util.find_spec for pluggable worker discovery

_verify_pluggable_worker_exists() previously checked for the literal file
`pluggable_worker/<name>/worker.py` on disk, which breaks when the plugin
has been compiled to a .so (Nuitka, Cython, or any C extension) — the
module is perfectly importable but the pre-check rejects it because only
the .py extension is considered.

Replace the filesystem check with importlib.util.find_spec(), which is
Python's standard way to ask "is this module resolvable by the import
system?". It honors every registered finder — source .py, compiled .so,
bytecode .pyc, namespace packages, zipimports — so the function now
matches what its docstring claims: verifying the module can be loaded,
not that a specific file extension is present.

Behavior is preserved for existing deployments:
- Images with no `pluggable_worker/<name>/` subpackage → find_spec
  raises ModuleNotFoundError (ImportError subclass) → returns False.
- Images with source .py → find_spec resolves the .py → returns True.
- Images with compiled .so → find_spec resolves the .so → returns True.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* [FIX] Handle ValueError from find_spec in pluggable worker verification

Greptile-flagged edge case: importlib.util.find_spec() can raise
ValueError (not just ImportError) when sys.modules has a partially
initialised module entry with __spec__ = None from a prior failed import.
Broaden the except to catch both.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* [FIX] Resolve api-deployment worker directory from enum import path

worker.py:452 did worker_type.value.replace("-", "_") to derive the
on-disk dir name. All WorkerType enum values already use underscores,
so the replace was a no-op; for API_DEPLOYMENT whose dir is
"api-deployment" (hyphen), it resolved to "api_deployment" and the
os.path.exists() check failed. Boot then logged a spurious
"❌ Worker directory not found: /app/api_deployment" at ERROR level.

The task registration path (builder + celery autodiscover via
to_import_path) is unaffected, so this was purely log noise — but
noise at ERROR level that masks real failures in log scans.

Fix: derive the directory from the authoritative to_import_path()
which already handles the hyphen case (api_deployment -> api-deployment).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* [HOTFIX] Add IAM Role / Instance Profile auth mode to AWS Bedrock adapter (#1944)

* [FEAT] Allow Bedrock to fall through to boto3's default credential chain

Match the S3/MinIO connector pattern: when AWS access keys are left blank
on the Bedrock LLM and embedding adapter forms, drop them from the kwargs
dict so boto3's default credential chain handles authentication. This
unlocks IAM role / instance profile / IRSA / AWS Profile scenarios on
hosts that already have ambient AWS credentials (e.g. EKS workers with
IRSA, EC2 with an instance profile).

- llm1/static/bedrock.json: clarify access-key descriptions to mention
  IRSA and instance profile (already non-required at v0.163.2 base).
- embedding1/static/bedrock.json: drop aws_access_key_id and
  aws_secret_access_key from top-level required; same description fix;
  expose aws_profile_name for parity with the LLM form.
- base1.py: AWSBedrockLLMParameters and AWSBedrockEmbeddingParameters
  now strip empty access-key values from the validated kwargs before
  returning, so empty strings don't override boto3's default chain.
  AWSBedrockEmbeddingParameters fields gain explicit None defaults
  and an aws_profile_name field.

Backward-compatible: existing adapters with access keys filled in
continue to work unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* [FEAT] Add Authentication Type selector to Bedrock adapter form

Add an explicit `auth_type` selector with two options, making the auth
choice clear to users:

- "Access Keys" (default): existing flow, keys required
- "IAM Role / Instance Profile (on-prem AWS only)": no fields; relies on
  boto3's default credential chain (IRSA on EKS, task role on ECS,
  instance profile on EC2). Description on the selector explicitly notes
  this option is only for AWS-hosted Unstract deployments.

The form-only auth_type field is stripped before LiteLLM validation in
both AWSBedrockLLMParameters.validate() and AWSBedrockEmbeddingParameters.
validate(). Empty access keys continue to be stripped so boto3 falls
through to the default chain even when the access_keys arm is selected
without values (matches the S3/MinIO connector pattern).

Backward-compatible: legacy adapters without auth_type behave as
"Access Keys" mode (the default), and existing keys are forwarded
unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* [REVIEW] Address Bedrock auth_type review feedback

Fixes the P0/P1 issues raised by greptile-apps and jaseemjaskp on
PR #1944.

Behaviour fixes:
- Stale-key leak in IAM Role mode: switching an existing adapter from
  Access Keys to IAM Role would carry truthy stored access keys through
  the strip-empty-only loop, so boto3 silently authenticated with the
  old long-lived credentials instead of falling through to the host's
  IRSA / instance-profile identity. Both LLM and embedding paths were
  affected.
- Silent acceptance of unknown auth_type: a typo (e.g. "access_key") or
  a malformed payload from a non-UI client passed through the dict
  comprehension untouched, with no enum guard.
- Cross-field validation gap: explicit Access Keys mode with blank or
  whitespace-only values silently fell through to the default
  credential chain instead of surfacing the misconfiguration.

Implementation:
- Add a module-level _resolve_bedrock_aws_credentials helper used by
  both AWSBedrockLLMParameters.validate() and AWSBedrock
  EmbeddingParameters.validate(), so the auth-type contract is
  expressed once.
  - Validates auth_type against an allowlist (None | "access_keys" |
    "iam_role"); raises ValueError on anything else.
  - iam_role: unconditionally drops aws_access_key_id and
    aws_secret_access_key.
  - access_keys (explicit): requires non-blank values; raises ValueError
    if either is empty or whitespace-only.
  - Legacy (auth_type absent): retains the lenient strip behaviour so
    pre-PR adapter configurations continue to deserialise unchanged.
- Restore aws_region_name as required (no `= None` default) on
  AWSBedrockEmbeddingParameters; only credentials may legitimately be
  absent.
- Drop the orphan aws_profile_name field from
  embedding1/static/bedrock.json: it was added for parity with the LLM
  form but lives outside the auth_type oneOf and contradicts the
  selector's "no further input" semantics. The LLM form already had
  aws_profile_name pre-PR and is left alone for backwards compatibility.

Tests:
- New tests/test_bedrock_adapter.py covers 15 cases across LLM and
  embedding adapters: legacy-no-auth-type, explicit access_keys with
  valid/blank/whitespace keys, iam_role with stale/no keys, unknown
  auth_type rejection, cross-field validation, and preservation of
  unrelated params (model_id, aws_profile_name, region, thinking).

Skipped (P2 nice-to-have):
- Comment-scope clarification, MinIO reference rewording,
  validate-mutates-caller'\''s-dict, and the LLM form description nit
  about aws_profile_name visibility. These don'\''t change behaviour
  and can be addressed in a follow-up.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* [HOTFIX] Bump litellm to 1.83.10 from PyPI to clear CVE-2026-42208 (#1976)

Hotfix for cloud v0.159.3 (OSS v0.163.4). Customer scanner flagged
litellm 1.82.3 for CVE-2026-42208 (SQL injection in litellm proxy auth
path, affects 1.81.16-1.83.6). We do not use litellm.proxy, but
vulnerability scanners flag the installed package regardless of which
code path is reachable.

Bump to 1.83.10 — the exact version recommended by the upstream advisory
(v1.83.10-stable) and the smallest jump that clears the CVE range while
keeping python-dotenv==1.0.1 compatible (1.83.14 would force bumping
python-dotenv across 7+ pyproject.toml files). Only tiktoken needed to
move 0.9 -> 0.12 to satisfy litellm's pin.

Switch source back to PyPI now that the PyPI quarantine is over,
reversing the temporary fork in #1873.

Cohere embed timeout patch: verified that
litellm/llms/cohere/embed/handler.py is byte-identical between v1.82.3,
v1.83.10-stable, and v1.83.14-stable (the timeout-not-forwarded bug
fixed in #1848 is still present upstream — BerriAI/litellm#14635 remains
OPEN). Version guard bumped 1.82.3 -> 1.83.10; 6/6 patch tests pass on
the new version, confirming the monkey-patch still binds correctly.

Other cleanup from #1873:
- Drop git apt-install from worker-unified and tool Dockerfiles (no
  git-sourced deps remain in any uv.lock)
- Bump tool versions: structure 0.0.100 -> 0.0.101,
  classifier 0.0.79 -> 0.0.80, text_extractor 0.0.75 -> 0.0.76

Note on root uv.lock churn: the v0.163.4 root uv.lock had a pre-existing
corruption (banks v2.4.1 entry pointing at banks-2.2.0 wheel) that
blocked incremental resolution. Regenerated from scratch.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* [FIX] Align cohere patch docstring with version-guard semantics

Reviewer flagged that the docstring claimed the patch is "confirmed in
every release between 1.82.3 and 1.83.14-stable", but the guard at
_PATCHED_LITELLM_VERSION activates only on the exact pinned version. A
future maintainer reading the old text could reasonably expect bumping
to e.g. 1.83.11 to keep the fix active; in reality it silently turns
off.

Rewritten to reference _PATCHED_LITELLM_VERSION as the single source of
truth and to drop the rot-prone "as of 2026-05-20" calendar date.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Chandrasekharan M <117059509+chandrasekharan-zipstack@users.noreply.github.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* UN-3476 [FIX] Revert atomic wrap on set_user_organization (#1977)

The atomic wrap from #1954 uncommits the new org row when
frictionless_onboarding HTTP-calls the LLMW portal mid-transaction.
The portal runs on a separate DB session and under READ COMMITTED
cannot see the uncommitted row, so the call returns 400 and the
caller silently persists an adapter with an empty unstract_key.
Every new signup since 2026-05-19 09:47 UTC ships a broken
free-trial X2Text adapter (401 on first OCR).

Hotfix only — Phase 2 (UN-3476) restructures the function so the
atomic guarantee is reapplied around just the pure-DB writes, with
HTTP and non-DB side effects moved outside the transaction.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* Restore text_extractor tool removed in Phase 5 decommission

The Phase 5 decommission commit removed classifier, structure,
text_extractor, and prompt-service. However, text_extractor is still
in active use by customers. This surgically restores only the
text_extractor tool while keeping the other decommissions in place.

- Restore tools/text_extractor/ directory (14 files from origin/main)
- Add tool-text_extractor back to docker-compose.build.yaml
- Add tool-text-extractor back to docker-tools-build-push.yaml workflow

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Restore classifier tool removed in Phase 5 decommission

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Remove unit-prompt-service group from test rig manifest

The prompt-service directory was deleted in the decommission PR, but
the test rig groups.yaml still referenced it, causing CI to fail with
"workdir does not exist" during validate and integration steps.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Remove deleted prompt-service and structure tool refs from bump script

prompt-service/ and tools/structure/ are deleted by this PR, so
remove their variables, reset_file calls, and the entire
update_structure_tool_version function from bump_sdk_v0_version.sh.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Fix stale references from decommissioned components

- Fix tool-text_extractor image name to tool-text-extractor in
  docker-compose.build.yaml to match CI, registry, and cloud naming
- Remove stale tool-structure from run-platform.sh ignore list
- Drop prompt-service from is_retryable_error docstring in retry_utils.py

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Trigger CI re-run

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Signed-off-by: Praveen Kumar <praveen@zipstack.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Praveen Kumar <praveen@zipstack.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: Deepak K <89829542+Deepak-Kesavan@users.noreply.github.com>
Co-authored-by: Chandrasekharan M <117059509+chandrasekharan-zipstack@users.noreply.github.com>
Co-authored-by: Athul <89829560+athul-rs@users.noreply.github.com>
Co-authored-by: vishnuszipstack <117254672+vishnuszipstack@users.noreply.github.com>
Co-authored-by: jimmy <ziming_zhu2002@163.com>
Co-authored-by: Hari John Kuriakose <hari@zipstack.com>
Co-authored-by: Chandrasekharan M <chandrasekharan@zipstack.com>
Co-authored-by: Athul <athul@zipstack.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request good first issue Good for newcomers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.