refactor(llm): use Registry pattern for framework registry

[Pouyanpi]

Description

Migrates nemoguardrails/llm/frameworks.py from an ad-hoc module-level dict and state globals to an LLMFrameworkRegistry(Registry) subclass, matching the pattern used by LogAdapterRegistry and EmbeddingProviderRegistry.

Summary by CodeRabbit

• Refactor

  • Restructured the framework registration and retrieval system with stricter validation of framework implementations to ensure protocol compliance.

• Tests

  • Enhanced test coverage to enforce framework protocol requirements and updated error validation scenarios.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[greptile-apps]

Greptile Summary

This PR replaces the ad-hoc module-level dict and globals in nemoguardrails/llm/frameworks/ with an LLMFrameworkRegistry(Registry) singleton, aligning it with the LogAdapterRegistry / EmbeddingProviderRegistry pattern. The refactor adds protocol enforcement at registration time (validate() rejects non-async or missing reset), moves lazy framework construction into typed factory callables, and cleans up the public API surface by removing private reset helpers from __all__.

Confidence Score: 5/5

Safe to merge; the only finding is a now-unreachable defensive guard that can be removed as a follow-up.

All changes are structural/refactor with tests updated to match new error messages and new protocol-enforcement behaviour. No P0 or P1 issues found. Single P2 style finding (dead code guard) does not block correctness.

nemoguardrails/llm/frameworks/registry.py — the redundant reset-is-None guard in _areset_frameworks.

Important Files Changed

Filename	Overview
nemoguardrails/llm/frameworks/registry.py	Refactors framework state into LLMFrameworkRegistry(Registry) singleton with validate(), lazy factory get(), and active property; one dead guard remains after validate() was added.
nemoguardrails/llm/frameworks/init.py	Removes private reset helpers from public all and re-exports only the four stable public functions; clean and minimal.
tests/conftest.py	Updates langchain_framework fixture to import _reset_frameworks from the internal registry module instead of the public package; straightforward and correct.
tests/llm/frameworks/test_registry.py	Adds register_provider/get_provider_names to all fake framework stubs, updates error-message assertions, removes obsolete _FrameworkWithoutReset tests, and adds TestRegisterFrameworkProtocolEnforcement for the new validate() path.

Sequence Diagram

sequenceDiagram
    participant C as Caller
    participant RF as register_framework()
    participant GF as get_framework()
    participant R as LLMFrameworkRegistry (Singleton)
    participant V as validate()

    C->>RF: register_framework("custom", fw)
    RF->>R: .add("custom", fw)
    R->>V: validate("custom", fw)
    V-->>R: ✓ isinstance(LLMFramework) + async reset
    R-->>RF: stored in items

    C->>GF: get_framework("langchain")
    GF->>R: .get("langchain")
    R->>R: "langchain" not in items but in _factories
    R->>R: _langchain_factory() → LangChainFramework()
    R->>V: validate("langchain", fw)
    V-->>R: ✓
    R-->>GF: LangChainFramework instance

    C->>C: await _areset_frameworks()
    C->>R: items.values() → snapshot
    loop each framework
        C->>C: await fw.reset()
    end
    C->>R: registry.reset() → items cleared
    C->>R: _active_name = env or "default"

Loading

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
nemoguardrails/llm/frameworks/registry.py:92-99
**Dead guard code after protocol enforcement was added**

The `reset is None` guard (and the `getattr` that feeds it) is now unreachable. `validate()` already rejects any framework whose `reset` attribute is absent or synchronous before it enters `self.items`, so the path where `reset` could be `None` at teardown time no longer exists.

```suggestion
    try:
        for fw in frameworks_to_close:
            try:
                await fw.reset()
            except Exception as exc:
                log.warning("Error resetting framework %r: %s", fw, exc)
```

_{Reviews (5): Last reviewed commit: "refactor(llm): use Registry pattern for ..." | Re-trigger Greptile}

[coderabbitai]

📝 Walkthrough

Walkthrough

The registry module refactors from module-level globals to an LLMFrameworkRegistry class that manages framework instances via lazy factory functions. The active framework state is now encapsulated within the registry rather than stored as module-level variables. Public helper functions delegate to corresponding registry methods.

Changes

Cohort / File(s)	Summary
Registry Core nemoguardrails/llm/frameworks/registry.py	Introduces LLMFrameworkRegistry subclass with factory-backed lazy instantiation, active framework property management, and validates framework objects as LLMFramework instances. Public functions (register_framework, get_framework, set_default_framework, get_default_framework) rewired to delegate to registry methods.
Test Updates tests/llm/frameworks/test_registry.py	Updates error messages for duplicate and unknown framework scenarios. Extends test framework doubles with register_provider and get_provider_names methods. Adds protocol enforcement test ensuring registration fails for frameworks lacking reset method.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 2

❌ Failed checks (2 warnings)

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.
Test Results For Major Changes	⚠️ Warning	PR includes comprehensive test updates validating the refactoring, but lacks explicit documentation of test execution results or evidence that tests have been run and passed.	Include test execution results in PR description, such as 'All tests pass' or '8 tests passed', to confirm no regressions from the major Registry pattern refactoring.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main refactoring: adopting the Registry pattern for framework management, which is the core architectural change in the changeset.
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 docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch refactor/stack-12-framework-registry

---

_{Review rate limit: 8/10 reviews remaining, refill in 7 minutes and 5 seconds.}

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

[coderabbitai]

🧹 Nitpick comments (1)

nemoguardrails/llm/frameworks/registry.py (1)

40-43: ⚡ Quick win

Make LLMFrameworkRegistry._factories immutable to avoid shared-state drift.

At Line 40, _factories is a mutable class-level dict shared across the singleton. Free mutation from tests/extensions can silently alter built-in framework resolution for the whole process.

Proposed patch

+from types import MappingProxyType
-from typing import Any, Callable, Dict
+from typing import Any, Callable, ClassVar, Mapping
...
 class LLMFrameworkRegistry(Registry):
-    _factories: Dict[str, Callable[[], LLMFramework]] = {
+    _factories: ClassVar[Mapping[str, Callable[[], LLMFramework]]] = MappingProxyType({
         "default": _default_factory,
         "langchain": _langchain_factory,
-    }
+    })

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@nemoguardrails/llm/frameworks/registry.py` around lines 40 - 43,
LLMFrameworkRegistry currently defines a mutable class-level dict _factories
that can be mutated by tests/extensions; make it immutable by replacing the
plain dict with an immutable mapping (e.g., wrap the dict in
types.MappingProxyType or use an immutable mapping type) so consumers still
lookup by key but cannot change entries at runtime; update the declaration of
_factories and any places that assume mutability (keep _default_factory and
_langchain_factory as the callable values) and ensure any tests or code that
previously mutated LLMFrameworkRegistry._factories instead use a provided
registration API or patching helper.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@nemoguardrails/llm/frameworks/registry.py`:
- Around line 40-43: LLMFrameworkRegistry currently defines a mutable
class-level dict _factories that can be mutated by tests/extensions; make it
immutable by replacing the plain dict with an immutable mapping (e.g., wrap the
dict in types.MappingProxyType or use an immutable mapping type) so consumers
still lookup by key but cannot change entries at runtime; update the declaration
of _factories and any places that assume mutability (keep _default_factory and
_langchain_factory as the callable values) and ensure any tests or code that
previously mutated LLMFrameworkRegistry._factories instead use a provided
registration API or patching helper.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: d9df53c5-332e-404a-ba87-07f361718ff5

📥 Commits

Reviewing files that changed from the base of the PR and between 9360fb2 and 4ed1167.

📒 Files selected for processing (2)

• nemoguardrails/llm/frameworks/registry.py
• tests/llm/frameworks/test_registry.py