mirror of
https://github.com/semantica-agi/semantica.git
synced 2026-08-29 04:26:20 +00:00
fix(providers): address review feedback on PR #556 + changelog
Four issues raised in code review: - Mode.JSON retry now strips response_format from create_kwargs before calling json_client.chat.completions.create, preventing incompatible kwargs from being forwarded to a client configured for a different mode. - Add exc_info=True to the generate_structured fallback warning in the manual repair loop so the gateway rejection traceback is visible in production logs, consistent with the other warnings added in this PR. - Remove the duplicate is_available definition in GroqProvider. Python silently kept only the second definition; the first (with diagnostic branching) was dead code and could cause confusion on future edits. - Validate base_url scheme in OpenAIProvider._init_client. Non-HTTP(S) schemes (file://, ftp://, javascript:, etc.) are now rejected with a ValueError at init time, preventing SSRF if base_url originates from configuration rather than hardcoded values. Add 3 new tests: SSRF scheme rejection, valid-URL acceptance, and exc_info presence on the generate_structured fallback warning (20/20 pass). Update CHANGELOG.md with full description of all fixes under [Unreleased].
This commit is contained in:
@@ -7,6 +7,31 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
||||
|
||||
---
|
||||
|
||||
## [Unreleased]
|
||||
|
||||
### Fixed
|
||||
|
||||
- **NERExtractor LLM method returning pattern-based output on custom gateways** (#554, PR #556) by @KaifAhmad1
|
||||
|
||||
`NERExtractor(method="llm")` silently fell back to regex/pattern extraction when used with OpenAI-compatible enterprise or self-hosted gateways (Qwen, LLaMA proxies, internal routing layers). Returned entities carried `extraction_method='pattern'` even though the LLM itself was producing correct tool-call output. Three root causes fixed:
|
||||
|
||||
- **Silent exception swallowing** — `exc_info=True` was missing from the method-failure `WARNING` in `NERExtractor.extract_entities`. The full gateway-rejection traceback was invisible in logs even with `DEBUG` level enabled, making the failure impossible to diagnose without reading source code.
|
||||
|
||||
- **`response_format=json_object` sent to incompatible gateways** — `OpenAIProvider.generate_structured` unconditionally included `response_format={"type": "json_object"}` in every API call. Custom/enterprise gateways frequently reject this parameter, causing both the `instructor` path and the manual repair loop to fail with the same error on every retry, eventually triggering `_extract_fallback` (pattern extraction).
|
||||
|
||||
- **No fallback in the `generate_typed` manual repair loop** — when `generate_structured` itself raised (due to gateway rejection), the repair loop retried the identical failing call up to `max_retries` times before giving up. There was no path to recover via plain `generate()` + JSON parsing.
|
||||
|
||||
**Additional fixes applied during PR review:**
|
||||
|
||||
- Mode.JSON retry in `generate_typed` now strips `response_format` from `create_kwargs` before forwarding to the retry client, preventing incompatible kwargs from being sent to a client configured for a different instructor mode.
|
||||
- `exc_info=True` added to the `generate_structured` fallback warning in the manual repair loop for consistent observability across all failure paths.
|
||||
- Removed dead duplicate `is_available` definition in `GroqProvider` — Python silently kept only the second definition; the first was unreachable.
|
||||
- `OpenAIProvider._init_client` now validates `base_url` scheme at construction time. Non-HTTP(S) schemes (`file://`, `ftp://`, `javascript:`, etc.) raise `ValueError` immediately, preventing SSRF if `base_url` originates from configuration rather than hardcoded values.
|
||||
|
||||
**17 regression tests** added in `tests/test_issue_554_fixes.py` covering all bug paths, including harshalizode's exact gateway configuration.
|
||||
|
||||
---
|
||||
|
||||
## [0.5.0] - 2026-05-11
|
||||
|
||||
### Added
|
||||
|
||||
@@ -424,7 +424,15 @@ class BaseProvider:
|
||||
exc_info=True,
|
||||
)
|
||||
json_client = instructor.from_openai(self.client, mode=instructor.Mode.JSON)
|
||||
response = json_client.chat.completions.create(**create_kwargs)
|
||||
# Build a clean kwargs dict for the Mode.JSON retry: drop
|
||||
# response_format (Mode.JSON handles schema differently)
|
||||
# but keep response_model/max_retries so instructor still
|
||||
# validates the typed output.
|
||||
retry_kwargs = {
|
||||
k: v for k, v in create_kwargs.items()
|
||||
if k != "response_format"
|
||||
}
|
||||
response = json_client.chat.completions.create(**retry_kwargs)
|
||||
else:
|
||||
raise
|
||||
|
||||
@@ -455,6 +463,7 @@ class BaseProvider:
|
||||
self.logger.warning(
|
||||
"generate_structured failed (%s); retrying with plain generate() + JSON parse.",
|
||||
struct_err,
|
||||
exc_info=True,
|
||||
)
|
||||
raw_content = self.generate(current_prompt, **kwargs)
|
||||
json_result = self._parse_json(raw_content)
|
||||
@@ -579,6 +588,17 @@ class OpenAIProvider(BaseProvider):
|
||||
|
||||
def _init_client(self):
|
||||
"""Initialize OpenAI client, respecting a custom base_url if provided."""
|
||||
if self.base_url:
|
||||
# Reject non-HTTP schemes (file://, ftp://, etc.) to prevent SSRF
|
||||
# when base_url originates from configuration rather than hardcoded values.
|
||||
from urllib.parse import urlparse
|
||||
scheme = urlparse(self.base_url).scheme
|
||||
if scheme not in ("http", "https"):
|
||||
raise ValueError(
|
||||
f"OpenAIProvider base_url must use http or https, got scheme {scheme!r}. "
|
||||
f"Only HTTP(S) endpoints are permitted."
|
||||
)
|
||||
|
||||
try:
|
||||
from openai import OpenAI
|
||||
|
||||
@@ -763,19 +783,6 @@ class GroqProvider(BaseProvider):
|
||||
self.client = None
|
||||
self.logger.error(f"Failed to initialize Groq client: {e}")
|
||||
|
||||
def is_available(self) -> bool:
|
||||
"""Check if provider is available and return diagnostic info."""
|
||||
if self.client is None:
|
||||
if not self.api_key:
|
||||
return False # Missing API key
|
||||
try:
|
||||
from groq import Groq
|
||||
except ImportError:
|
||||
return False # Library not installed
|
||||
return False
|
||||
|
||||
return True
|
||||
|
||||
def _test_connection(self):
|
||||
"""Internal method to verify connection."""
|
||||
if not self.client:
|
||||
|
||||
@@ -265,6 +265,38 @@ class TestBug2GenerateStructuredCustomGateway(unittest.TestCase):
|
||||
result = provider.generate_structured("Find entities.")
|
||||
self.assertEqual(result, payload)
|
||||
|
||||
def test_invalid_base_url_scheme_raises(self):
|
||||
"""Non-HTTP(S) base_url must be rejected at init time to prevent SSRF."""
|
||||
for bad_url in ("file:///etc/passwd", "ftp://internal.host/v1", "javascript:void"):
|
||||
with self.assertRaises(ValueError, msg=f"Expected ValueError for {bad_url!r}"):
|
||||
provider = object.__new__(OpenAIProvider)
|
||||
provider.config = {}
|
||||
provider.logger = get_logger("test_provider")
|
||||
provider.api_key = "test-key"
|
||||
provider.model = "test-model"
|
||||
provider.base_url = bad_url
|
||||
provider.client = None
|
||||
provider._init_client()
|
||||
|
||||
def test_valid_http_base_url_accepted(self):
|
||||
"""http:// and https:// base_url values must pass validation."""
|
||||
for good_url in ("https://gateway.corp/api/v1", "http://localhost:8080/v1"):
|
||||
provider = object.__new__(OpenAIProvider)
|
||||
provider.config = {}
|
||||
provider.logger = get_logger("test_provider")
|
||||
provider.api_key = "test-key"
|
||||
provider.model = "test-model"
|
||||
provider.base_url = good_url
|
||||
provider.client = None
|
||||
# _init_client will fail to import openai (not installed) but must
|
||||
# not raise ValueError before reaching the import
|
||||
try:
|
||||
provider._init_client()
|
||||
except ValueError:
|
||||
self.fail(f"_init_client raised ValueError for valid URL {good_url!r}")
|
||||
except Exception:
|
||||
pass # ImportError / OSError from missing openai package is expected
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Bug 3 – generate_typed manual repair loop must fall back to plain generate()
|
||||
@@ -355,6 +387,31 @@ class TestBug3GenerateTypedFallbackToPlainGenerate(unittest.TestCase):
|
||||
"Extract entities.", schema=_StubEntitiesResponse, max_retries=2
|
||||
)
|
||||
|
||||
def test_generate_structured_fallback_warning_has_exc_info(self):
|
||||
"""
|
||||
The warning logged when generate_structured fails must carry exc_info so
|
||||
the full traceback is visible in production logs (consistent with other
|
||||
warnings added in this PR).
|
||||
"""
|
||||
provider = _bare_openai_provider(base_url="https://gateway.local/api/v1")
|
||||
|
||||
provider.generate_structured = MagicMock(
|
||||
side_effect=ProcessingError("response_format rejected")
|
||||
)
|
||||
provider.generate = MagicMock(return_value=self._valid_json())
|
||||
|
||||
with self.assertLogs("semantica.test_provider", level="WARNING") as log_ctx:
|
||||
provider.generate_typed(
|
||||
"Extract entities.", schema=_StubEntitiesResponse, max_retries=1
|
||||
)
|
||||
|
||||
has_traceback = any(r.exc_info is not None for r in log_ctx.records)
|
||||
self.assertTrue(
|
||||
has_traceback,
|
||||
"generate_structured fallback warning must include exc_info=True "
|
||||
"so the gateway rejection traceback is visible in logs.",
|
||||
)
|
||||
|
||||
def test_fallback_preserves_error_on_bad_json(self):
|
||||
"""
|
||||
If plain generate() returns malformed JSON the error must propagate,
|
||||
|
||||
Reference in New Issue
Block a user