diff --git a/CHANGELOG.md b/CHANGELOG.md index a7b97af7..1b83ba77 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/semantica/semantic_extract/providers.py b/semantica/semantic_extract/providers.py index 663108a2..e3189727 100644 --- a/semantica/semantic_extract/providers.py +++ b/semantica/semantic_extract/providers.py @@ -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: diff --git a/tests/test_issue_554_fixes.py b/tests/test_issue_554_fixes.py index 85dabd06..4bf21bd0 100644 --- a/tests/test_issue_554_fixes.py +++ b/tests/test_issue_554_fixes.py @@ -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,