feat(models): add pluggable provider policy#14137
Open
erichare wants to merge 6 commits into
Open
Conversation
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis change adds pluggable model-provider authorization, stable provider identities, extension catalog loading, optional-credential support, and policy enforcement across unified models, backend APIs, runtime services, and flow-builder operations. It also adds extensive tests and refreshes secret-baseline metadata. ChangesModel-provider policy and catalog
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Extension
participant ProviderRegistry
participant PolicyService
participant BackendAPI
participant UnifiedModels
participant Credentials
Extension->>ProviderRegistry: Register descriptor and catalog loader
BackendAPI->>PolicyService: Resolve provider policy
PolicyService-->>BackendAPI: Return immutable provider snapshot
BackendAPI->>UnifiedModels: Request policy-filtered options or model
UnifiedModels->>PolicyService: Require selected provider
UnifiedModels->>Credentials: Resolve permitted secret/configuration
Credentials-->>UnifiedModels: Return credential values
UnifiedModels-->>BackendAPI: Return canonical model/provider result
🚥 Pre-merge checks | ✅ 8 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (8 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Contributor
✅ Test Coverage AdvisorNo source changes detected without accompanying tests. Thanks for keeping coverage up! 🎉
|
github-actions
Bot
added
enhancement
This comment has been minimized.
This comment has been minimized.
Contributor
Frontend Unit Test Coverage ReportCoverage Summary
Unit Test Results
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/backend/base/langflow/services/memory_base/service.py (2)
85-96: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAllow API-key-optional preprocessing providers.
Credentialless providers still fail validation because every missing key raises
PreprocessingValidationError, despite runtime construction supporting optional credentials.Proposed fix
+from lfx.base.models.provider_registry import is_api_key_optional - if not api_key: + if not api_key and not is_api_key_optional(provider):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/base/langflow/services/memory_base/service.py` around lines 85 - 96, Update _validate_preprocessing_api_key to recognize preprocessing providers that support credentialless runtime construction and allow a missing API key for them; continue raising PreprocessingValidationError for providers that require credentials, preserving the existing provider/model-specific error message.
212-223: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAuthorize against the patched values before mutating the record.
require_model_provider()and_validate_preprocessing_api_key()run onmbbeforepatchis applied, so a request can swap in a denied embedding/preprocessing provider and still be persisted. Update the test to change the provider fields, not justthreshold, and assert no mutation or commit when the new provider is rejected.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/base/langflow/services/memory_base/service.py` around lines 212 - 223, The memory update flow in service.py must validate the effective patched embedding and preprocessing provider values before mutating mb or committing; update the authorization checks around require_model_provider and _validate_preprocessing_api_key to use the patch values while preserving existing values for omitted fields. In test_memory_bases.py, update the rejection test to change provider fields rather than only threshold and assert the record remains unchanged and no commit occurs.
🧹 Nitpick comments (2)
src/backend/base/langflow/services/variable/service.py (1)
34-57: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
resolve_model_provider_policycall isn't covered by the surrounding fail-safe.The metadata resolution above (Lines 35-49) is guarded so any failure logs and returns early, keeping environment-variable initialization resilient. The
resolve_model_provider_policycall in theelse:branch (Lines 53-57) isn't covered by that same guard — if a pluggable policy service throws (e.g., a buggy enterprise policy extension), the exception propagates uncaught and crashesinitialize_user_variablesfor every user, instead of failing safe like the metadata step does.♻️ Proposed fix
try: from lfx.base.models.unified_models import get_model_provider_metadata from lfx.services.model_provider_policy import ModelProviderPolicyPurpose, resolve_model_provider_policy var_to_provider = {} var_to_info = {} metadata = get_model_provider_metadata() for provider, meta in metadata.items(): for var in meta.get("variables", []): var_key = var.get("variable_key") if var_key: var_to_provider[var_key] = provider var_to_info[var_key] = var + provider_policy = resolve_model_provider_policy( + user_id=user_id, + providers=metadata, + purpose=ModelProviderPolicyPurpose.CONFIGURE, + ) except Exception: # noqa: BLE001 await logger.aexception("Could not resolve model-provider metadata; skipping environment variable import") return - else: - provider_policy = resolve_model_provider_policy( - user_id=user_id, - providers=metadata, - purpose=ModelProviderPolicyPurpose.CONFIGURE, - )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/base/langflow/services/variable/service.py` around lines 34 - 57, Extend the fail-safe in the initialization flow to cover resolve_model_provider_policy, keeping policy resolution inside the guarded try path used for metadata loading. If it raises, log the exception with logger.aexception and return early, preserving resilient environment-variable initialization; update the surrounding try/except/else structure near get_model_provider_metadata and resolve_model_provider_policy without changing successful behavior.src/backend/tests/unit/api/v1/test_models_live_only_providers.py (1)
78-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
_unregisterreaches into 7+ privateprovider_registryattributes.This keeps the test mock-free (good), but couples it tightly to internal state (
_registered,_registered_ids,_registered_aliases,_live_discovery_cache,_validator_cache,_catalog_cache,_undo.*,_generation). A future internal refactor of the actively-evolvingprovider_registrymodule can silently break this and leak provider state into other tests.Consider adding a small public helper (e.g.
provider_registry.unregister(name)) that performs this same reversal, so tests use a stable contract instead of private fields.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/tests/unit/api/v1/test_models_live_only_providers.py` around lines 78 - 100, The test helper _unregister should stop manipulating provider_registry’s private attributes directly. Add a small public provider_registry.unregister(name) operation that reverses the registration and clears all associated metadata, aliases, caches, undo state, and derived state, then update _unregister to call that public API while retaining cleanup for the separate provider collections it owns.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/docs/Lfx/extensions-manifest.mdx`:
- Line 74: Update the extensions manifest documentation to use the loader’s
public multi-bundle error code, replacing
multi-bundle-deferred-in-this-milestone with multi-bundle-unsupported while
preserving the surrounding v0 behavior description.
- Line 133: Update the provider precedence documentation to distinguish ordinary
provider-name or core-alias collisions, which are skipped, from stable
provider_id or alias conflicts, which raise and surface as provider-invalid; do
not describe all colliding providers as skippable.
In `@src/lfx/src/lfx/base/models/provider_registry.py`:
- Around line 634-654: Normalize the model name with strip() before constructing
the duplicate-detection key in the catalog row validation flow. Update the key
and stored normalized row to use this trimmed name, while preserving the
existing validation and model_type checks in the surrounding loader logic.
- Around line 575-587: Make get_registry_snapshot build the generation and
descriptors atomically under the registry’s existing _lock. Keep optional
validate_registered_provider_catalogs() behavior, then acquire _lock before
reading _generation, _core_provider_ids(), _registered_ids, and provider
descriptors, and construct the complete ProviderRegistrySnapshot from that
single locked state so concurrent register_provider() or clear() mutations
cannot produce mixed-generation results.
In `@src/lfx/src/lfx/base/models/unified_models/build_config.py`:
- Around line 363-370: Update the sticky-default logic around
saved_provider_allowed and provider_policy so provider-less saved selections
remain valid, and add a non-empty saved provider to the policy candidates before
evaluating it. Preserve the existing hidden-provider rejection for explicitly
saved providers while allowing custom saved providers even when absent from the
current options_list.
In `@src/lfx/src/lfx/mcp/flow_builder_tools/mutate_tools.py`:
- Around line 210-220: Update _require_allowed_model_providers in
src/lfx/src/lfx/mcp/flow_builder_tools/mutate_tools.py:210-220 to resolve the
policy with ModelProviderPolicyPurpose.CONFIGURE instead of USE. In
src/lfx/tests/unit/test_flow_builder_tools.py:756-760, capture the resolver
arguments and assert that the CONFIGURE purpose is passed.
In `@src/lfx/tests/unit/test_flow_builder_tools.py`:
- Around line 756-760: Update the resolver stub used by the ConfigureComponent
tests to capture its invocation arguments instead of ignoring them, then assert
that the requested purpose is ModelProviderPolicyPurpose.CONFIGURE. Keep the
existing snapshot return behavior and verify the resolver call specifically
covers the configuration path.
---
Outside diff comments:
In `@src/backend/base/langflow/services/memory_base/service.py`:
- Around line 85-96: Update _validate_preprocessing_api_key to recognize
preprocessing providers that support credentialless runtime construction and
allow a missing API key for them; continue raising PreprocessingValidationError
for providers that require credentials, preserving the existing
provider/model-specific error message.
- Around line 212-223: The memory update flow in service.py must validate the
effective patched embedding and preprocessing provider values before mutating mb
or committing; update the authorization checks around require_model_provider and
_validate_preprocessing_api_key to use the patch values while preserving
existing values for omitted fields. In test_memory_bases.py, update the
rejection test to change provider fields rather than only threshold and assert
the record remains unchanged and no commit occurs.
---
Nitpick comments:
In `@src/backend/base/langflow/services/variable/service.py`:
- Around line 34-57: Extend the fail-safe in the initialization flow to cover
resolve_model_provider_policy, keeping policy resolution inside the guarded try
path used for metadata loading. If it raises, log the exception with
logger.aexception and return early, preserving resilient environment-variable
initialization; update the surrounding try/except/else structure near
get_model_provider_metadata and resolve_model_provider_policy without changing
successful behavior.
In `@src/backend/tests/unit/api/v1/test_models_live_only_providers.py`:
- Around line 78-100: The test helper _unregister should stop manipulating
provider_registry’s private attributes directly. Add a small public
provider_registry.unregister(name) operation that reverses the registration and
clears all associated metadata, aliases, caches, undo state, and derived state,
then update _unregister to call that public API while retaining cleanup for the
separate provider collections it owns.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4f33bc59-0778-4556-93dd-67bc297638fb
📒 Files selected for processing (54)
.secrets.baselineBUNDLE_API.mddocs/docs/Lfx/extensions-manifest.mdxsrc/backend/base/langflow/agentic/api/router.pysrc/backend/base/langflow/agentic/services/provider_service.pysrc/backend/base/langflow/api/v1/knowledge_bases.pysrc/backend/base/langflow/api/v1/models.pysrc/backend/base/langflow/api/v1/variable.pysrc/backend/base/langflow/services/factory.pysrc/backend/base/langflow/services/memory_base/preprocessing.pysrc/backend/base/langflow/services/memory_base/service.pysrc/backend/base/langflow/services/schema.pysrc/backend/base/langflow/services/utils.pysrc/backend/base/langflow/services/variable/service.pysrc/backend/tests/unit/agentic/api/test_iterations_limit_forwarding.pysrc/backend/tests/unit/agentic/services/test_provider_service.pysrc/backend/tests/unit/api/v1/test_detect_env_vars.pysrc/backend/tests/unit/api/v1/test_models_live_only_providers.pysrc/backend/tests/unit/api/v1/test_models_provider_policy.pysrc/backend/tests/unit/services/variable/test_service.pysrc/backend/tests/unit/test_knowledge_bases_api.pysrc/backend/tests/unit/test_memory_base_preprocessing.pysrc/backend/tests/unit/test_memory_bases.pysrc/backend/tests/unit/test_unified_models.pysrc/lfx/PLUGGABLE_SERVICES.mdsrc/lfx/lfx.toml.examplesrc/lfx/src/lfx/base/models/model_metadata.pysrc/lfx/src/lfx/base/models/provider_registry.pysrc/lfx/src/lfx/base/models/unified_models/__init__.pysrc/lfx/src/lfx/base/models/unified_models/build_config.pysrc/lfx/src/lfx/base/models/unified_models/credentials.pysrc/lfx/src/lfx/base/models/unified_models/instantiation.pysrc/lfx/src/lfx/base/models/unified_models/model_catalog.pysrc/lfx/src/lfx/base/models/unified_models/provider_queries.pysrc/lfx/src/lfx/components/agentics/helpers/llm_setup.pysrc/lfx/src/lfx/extension/loader/_orchestrator.pysrc/lfx/src/lfx/extension/manifest.pysrc/lfx/src/lfx/mcp/flow_builder_tools/mutate_tools.pysrc/lfx/src/lfx/services/deps.pysrc/lfx/src/lfx/services/manager.pysrc/lfx/src/lfx/services/model_provider_policy/__init__.pysrc/lfx/src/lfx/services/model_provider_policy/base.pysrc/lfx/src/lfx/services/model_provider_policy/service.pysrc/lfx/src/lfx/services/model_provider_policy/utils.pysrc/lfx/src/lfx/services/schema.pysrc/lfx/tests/unit/base/models/test_build_config_sticky_default.pysrc/lfx/tests/unit/base/models/test_provider_registry.pysrc/lfx/tests/unit/components/agentics/test_llm_setup.pysrc/lfx/tests/unit/extension/test_provider_extension.pysrc/lfx/tests/unit/inputs/test_model_input_static_options.pysrc/lfx/tests/unit/services/model_provider_policy/__init__.pysrc/lfx/tests/unit/services/model_provider_policy/test_policy.pysrc/lfx/tests/unit/services/test_service_manager.pysrc/lfx/tests/unit/test_flow_builder_tools.py
github-actions
Bot
added
enhancement
Collaborator
Author
|
@coderabbitai Addressed the outside-diff/nitpick review items in 74d2503:
|
github-actions
Bot
added
enhancement
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat --oneline --decorate --no-renames 74d2503bc
printf '\n=== affected paths ===\n'
git show --format='' --name-only 74d2503bc | sed '/^$/d'
printf '\n=== relevant definitions and tests ===\n'
for path in \
src/backend/base/langflow/services/memory_base/preprocessing.py \
src/backend/base/langflow/services/memory_base/service.py \
src/backend/base/langflow/services/variable/service.py \
src/lfx/src/lfx/base/models/provider_registry.py \
src/backend/tests/unit/test_memory_base_preprocessing.py \
src/backend/tests/unit/services/variable/test_service.py \
src/lfx/tests/unit/base/models/test_provider_registry.py
do
if [ -f "$path" ]; then
echo "--- $path ---"
ast-grep outline "$path" --items all --view compact || true
fi
done
printf '\n=== commit diff for relevant paths ===\n'
git show --no-ext-diff --unified=50 74d2503bc -- \
src/backend/base/langflow/services/memory_base/preprocessing.py \
src/backend/base/langflow/services/memory_base/service.py \
src/backend/base/langflow/services/variable/service.py \
src/lfx/src/lfx/base/models/provider_registry.py \
src/backend/tests/unit/test_memory_base_preprocessing.py \
src/backend/tests/unit/services/variable/test_service.py \
src/lfx/tests/unit/base/models/test_provider_registry.pyLength of output: 49028 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== MemoryBaseUpdate schema and validation call sites ==='
rg -n -A35 -B8 'class\s+MemoryBaseUpdate\b|def\s+_validate_preprocessing_api_key\b|async\s+def\s+update\b' \
src/backend/base/langflow/services/database/models/memory_base/model.py \
src/backend/base/langflow/services/memory_base/service.py
printf '%s\n' '=== credentialless preprocessing regression coverage ==='
rg -n -A35 -B8 'api_key_optional|Ollama|credentialless|_validate_preprocessing_api_key|preprocessing.*key' \
src/backend/tests/unit/test_memory_bases.py \
src/backend/tests/unit/test_memory_base_preprocessing.py
printf '%s\n' '=== fixture now uses public unregistration ==='
rg -n -A20 -B8 '\bunregister_provider\s*\(' \
src/backend/tests/unit/api/v1/test_models_live_only_providers.py \
src/lfx/tests/unit/base/models/test_provider_registry.pyLength of output: 88434
✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
added
enhancement
Contributor
|
Build successful! ✅ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Architecture and compatibility
Validation
Summary by CodeRabbit