[Bug Report] 13 Confirmed Bugs: Azure crash, prompt-sanitizer bypass, MCP tool collisions, uncatchable exceptions, SessionMap per-model expiry, and more

[wilsonhj]

🐛 Bug Report — 13 Confirmed Bugs (Independent Review Pass)

Reviewed by: @wilsonhj
Severity summary: 2 Critical · 5 High · 4 Medium · 1 Security regression (round 2)
Affected version: 0.0.1 (current main, post-MCP merge)
Review method: 7-angle independent multi-agent code review (Round 1: 10 bugs), followed by an 8-angle review of Round 1's own diff (Round 2: 3 corrections/regressions caught), plus a final hardening pass (Round 3: 3 additional fixes). Total: 13 distinct root causes, all fixed.
Note: These are entirely independent of Issue #6 / PR #7. Some findings touch the same files (lcel/model.py, utils/session_map.py) but different functions and lines.

A fix PR addressing all 13 bugs, with 99 passing unit tests and no regressions, accompanies this issue.

---

Round 1 — 7-Angle Review (10 Bugs)

Bug 1 — AzureOpenAI Constructed with api_version Supplied Twice [CRITICAL]

File: connectchain/lcel/model.py (_get_azure_model_, Azure branch of _get_direct_model_)
Severity: 🔴 Critical — Azure support non-functional against real client

_get_azure_model_() passes api_version both as a direct constructor kwarg and inside model_kwargs={"api_version": ...}. The real AzureOpenAI client raises TypeError: __init__() got multiple values for keyword argument 'api_version'. Every existing test mocks AzureOpenAI out, so this crash was invisible to the test suite. Reproduced directly against the real class.

Fix: Extract a shared _azure_model_kwargs_() helper that builds the dict once; pass api_version only as a top-level kwarg.

---

Bug 2 — hasattr()/getattr(default=...) Are No-ops Against ConfigWrapper [HIGH]

File: connectchain/lcel/model.py (Azure branch of _get_direct_model_)
Severity: 🟠 High — Silent wrong behavior (temperature forced to None)

ConfigWrapper.__getattr__ returns None for missing keys instead of raising AttributeError. This means hasattr(model_config, "api_version") always returns True, and getattr(model_config, "api_version", None) always returns None via __getattr__. The Azure engine/api_version fallback logic silently evaluates to None for every config that doesn't explicitly set those keys.

Fix (with Round 2 correction): Check model_config.api_version is not None explicitly. Add a guard that raises LCELModelException with a clear message when an Azure-shaped api_base is configured without api_version (so misconfiguration fails loudly, not silently).

---

Bug 3 — LCELRetry.invoke() Drops **kwargs That ainvoke() Forwards Correctly [HIGH]

File: connectchain/lcel/lcel_retry.py
Severity: 🟠 High — Sync/async inconsistency

LCELRetry.invoke() does not forward **kwargs to the underlying runnable. ainvoke() does. Identical calls with extra kwargs behave differently depending on whether they're sync or async.

Fix: Add **kwargs forwarding to invoke()'s call: self._runnable.invoke(input, config, **kwargs).

---

Bug 4 — ValidPromptTemplate Sanitizes Per-Field Inputs, Not the Rendered Prompt [CRITICAL]

File: connectchain/prompts/valid_prompt_template.py
Severity: 🔴 Critical — Sanitizer bypass via field splitting

The class docstring says it sanitizes "the rendered prompt". The implementation sanitizes each raw template field value individually before rendering. Disallowed content split across two template fields (e.g., {"word1": "bad", "word2": "word"} with template "{word1} {word2}") bypasses the sanitizer — the check runs on "bad" and "word" separately, neither triggers the filter, and the final rendered string "bad word" is never checked.

# Reproduction
sanitizer = lambda s: s if "bad word" not in s else (_ for _ in ()).throw(ValueError("blocked"))
template = ValidPromptTemplate(template="{a} {b}", input_variables=["a", "b"], sanitizer=sanitizer)
template.format(a="bad", b="word")  # No exception — sanitizer never sees "bad word"

Fix: Move the sanitizer call to run on the final rendered string (output of super().format(...)).

---

Bug 5 — _wrap_method_'s hasattr() Can Raise the ValueError It Was Written to Guard Against [MEDIUM]

File: connectchain/utils/llm_proxy_wrapper.py
Severity: 🟡 Medium — Unexpected exception path

_wrap_method_ calls hasattr(llm, method_name) before the try block. On Pydantic v2 models (which LangChain 0.3.x uses), accessing a field that doesn't exist can trigger validators that raise ValueError. The surrounding try/except ValueError was written to catch exactly this — but hasattr runs before it.

Fix: Move the hasattr check inside the try block, or use getattr(llm, method_name, None) with a None guard.

---

Bug 6 — SessionMap.__new__ Silently Ignores expires_in After First Construction [HIGH]

File: connectchain/utils/session_map.py
Severity: 🟠 High — Wrong expiry policy applied per model

SessionMap is a singleton. After the first construction, every subsequent SessionMap(different_interval) call returns the existing instance without updating expires_in. If two models use different token refresh intervals, the second model's interval is silently ignored.

Fix (with Round 2 correction): Capture expires_in per-session at cache time (store it alongside the LLM in new_session()), rather than reading the singleton's current self.expires_in in is_expired(). Makes each session's expiry policy independent.

---

Bug 7 — Custom Exceptions Subclass BaseException Instead of Exception [HIGH + SECURITY]

File: connectchain/utils/exceptions.py, connectchain/lcel/model.py
Severity: 🟠 High (3 of 4 exceptions) + 🔒 Security note (1 of 4)

LCELModelException, ConfigException, and UtilException subclass BaseException instead of Exception. Standard except Exception: handlers will never catch them — they propagate uncaught through catch-all blocks, crashing the application.

ConnectChainNoAccessException is intentionally BaseException — it is the kill-switch that disables APIChain in connectchain/__init__.py. This one must remain BaseException so that ordinary application except Exception: code cannot swallow it.

Fix: Change LCELModelException, ConfigException, and UtilException to subclass Exception. Add an explanatory comment on ConnectChainNoAccessException documenting why it remains BaseException.

---

Bug 8 — MCPToolLoader.load_tools([]) Treats Explicit Empty List as "No Filter" [MEDIUM]

File: connectchain/tools/mcp/tool_loader.py
Severity: 🟡 Medium — Semantic mismatch; [] and None have opposite meanings

The filter parameter is checked with if not server_names:. An explicit empty list [] ("load from no servers") and None ("load from all servers") both evaluate falsy — so load_tools([]) loads all tools instead of none.

Fix: Check if server_names is None: to distinguish "unset" from "explicitly empty".

---

Bug 9 — MCPToolAgent Silently Drops Tools on Same-Name Collisions Across Servers [MEDIUM]

File: connectchain/tools/mcp/mcp_tool_agent.py
Severity: 🟡 Medium — Silent data loss, no warning

When two MCP servers expose tools with the same name, the second tool silently overwrites the first in the internal tools dict. No warning is logged; the dropped tool is unrecoverable.

Fix: Log a WARNING when a name collision is detected, including both server names and the tool name.

---

Bug 10 — MCPToolAgent.ainvoke Violates -> dict Return Type on No-Tool Path [MEDIUM]

File: connectchain/tools/mcp/mcp_tool_agent.py
Severity: 🟡 Medium — Type contract violation

ainvoke is annotated -> dict but returns a plain str when the agent decides no tool call is needed. Callers that unpack the return value as a dict crash on the direct-answer path.

Fix: Wrap the direct-answer string: return {"output": response_text}. Updated the one existing test that relied on the old shape.

---

Round 2 — 8-Angle Review of Round 1's Diff (3 Corrections)

Correction A — Security Regression: ConnectChainNoAccessException Over-Broadened

Round 1 grouped all four exceptions together and changed them all to Exception. This broke the APIChain kill-switch in connectchain/__init__.py — ConnectChainNoAccessException is specifically designed to not be catchable by ordinary application code. Reverted to BaseException for this exception only, with a comment.

Correction B — SessionMap Per-Session Expiry Fix Was Incomplete

Round 1 updated expires_in on every construction — stopping a later config from being silently ignored — but expires_in was still one shared singleton field applied to all cached sessions. A later SessionMap(different_interval) for model B would retroactively change model A's effective expiry. Fixed by storing expires_in per-session at new_session() call time.

Correction C — Azure Missing api_version Now Fails Silently Instead of Loudly

The hasattr→getattr fix for the Azure branch changed the gating condition from "always true" (guaranteed visible crash) to "false when api_version unset" — falling through to ChatOpenAI pointed at the Azure endpoint, sending non-Azure-shaped requests that fail confusingly at call time. Added an explicit guard that raises LCELModelException with a clear message.

---

Round 3 — Hardening Pass (3 Additional Fixes)

Bug 11 — SessionMap Crashes with None-Valued expires_in [HIGH]

File: connectchain/utils/session_map.py

If SessionMap is constructed with expires_in=None (from a config that doesn't set the field — ConfigWrapper returns None for missing keys), is_expired() crashes with TypeError: '>' not supported between instances of 'float' and 'NoneType'.

Fix: Validate expires_in at construction time; raise ValueError immediately if None or non-positive.

Bug 12 — Azure Detection Too Narrow: Sovereign Cloud Endpoints Not Recognized [HIGH]

File: connectchain/lcel/model.py

Azure detection checks "openai.azure.com" in str(api_base). Sovereign cloud endpoints (*.openai.azure.us, *.cognitive.microsoft.com) don't match, silently falling through to ChatOpenAI instead of AzureOpenAI.

Fix: Broaden the detection to cover known sovereign patterns; also support an explicit azure: true flag in model config as a reliable override.

Bug 13 — YAML Date Coercion Breaks api_version String [MEDIUM]

File: Config loading path (connectchain/utils/config.py)

YAML parses values like 2024-02-01 as datetime.date objects, not strings. Azure OpenAI API versions use this exact date format. They arrive as date objects and cause TypeError when passed to the Azure client.

Fix: Coerce api_version to str during config loading; document the quoting requirement in the example config.

---

Summary Table

#	File	Severity	One-Line Description
1	lcel/model.py	🔴 Critical	api_version passed twice to AzureOpenAI — always crashes
2	lcel/model.py	🟠 High	hasattr/getattr no-ops against ConfigWrapper → silent None values
3	lcel/lcel_retry.py	🟠 High	invoke() drops **kwargs; ainvoke() doesn't — sync/async inconsistency
4	prompts/valid_prompt_template.py	🔴 Critical	Sanitizer runs per-field, not rendered prompt → bypass via field splitting
5	utils/llm_proxy_wrapper.py	🟡 Medium	hasattr before try-block can raise the ValueError the block catches
6	utils/session_map.py	🟠 High	Singleton ignores expires_in after first construction → wrong expiry per model
7	utils/exceptions.py	🟠 High + 🔒	3 exceptions inherit BaseException; uncatchable by standard except Exception:
8	tools/mcp/tool_loader.py	🟡 Medium	Empty list [] treated as None (no filter) instead of "no servers"
9	tools/mcp/mcp_tool_agent.py	🟡 Medium	Tool name collisions across servers silently dropped with no warning
10	tools/mcp/mcp_tool_agent.py	🟡 Medium	ainvoke returns str on no-tool path, violates -> dict annotation
11	utils/session_map.py	🟠 High	None-valued expires_in crashes is_expired() at runtime
12	lcel/model.py	🟠 High	Sovereign cloud Azure endpoints bypass detection heuristic
13	Config loading	🟡 Medium	YAML parses api_version date strings as date objects → TypeError

Fix PR with 99 passing unit tests, mypy-clean, and pylint score unchanged follows.