UN-2930 [MISC] Add plugin infrastructure to unstract-core#1618
UN-2930 [MISC] Add plugin infrastructure to unstract-core#1618chandrasekharan-zipstack merged 15 commits intomainfrom
Conversation
Introduce a generic, framework-agnostic plugin system that supports Django, Flask, and Workers. This foundation enables dynamic loading of plugins with metadata validation, singleton/non-singleton patterns, and support for compiled extensions. Key components: - Generic PluginManager with plugin discovery and validation - DjangoPluginManager wrapper for Django apps - FlaskPluginManager wrapper for Flask apps - Redis client enhancement (REDIS_USERNAME support) - Plugin directory exclusions in .gitignore - Development dependency: debugpy for debugging This is the first PR in a series for UN-2930, establishing the infrastructure needed for subscription usage tracking and other plugin-based features. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Summary by CodeRabbitRelease Notes
Summary by CodeRabbit
WalkthroughAdds a framework-agnostic PluginManager with Flask and Django wrappers, wires plugin loading into app startup, converts many hard-coded plugin usages to runtime plugin lookups, introduces subscription-usage tracking (decorator + callback integration), removes legacy per-app loaders, updates .gitignore entries, adds REDIS_USERNAME fallback, and adds debugpy to dev deps. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application (Django/Flask)
participant Wrapper as Framework PluginManager
participant Generic as Generic PluginManager
participant FS as Filesystem (plugins dir)
App->>Wrapper: instantiate (app?/plugins_dir, plugins_pkg, logger)
Note right of Wrapper `#e6f7ff`: lazy init / singleton storage
App->>Wrapper: load_plugins()
Wrapper->>Generic: init manager (plugins_dir, plugins_pkg, logger, registration_callback)
Generic->>FS: scan plugins directory
FS-->>Generic: plugin entries
loop per plugin
Generic->>Generic: import plugin module
Generic->>Generic: validate metadata (name, version, is_active, disable)
alt valid & active
Generic->>Wrapper: call registration_callback(plugin_data)
Wrapper->>App: register artifacts (e.g., Flask blueprint)
Generic-->>Wrapper: record plugin loaded
else skip / invalid
Generic-->>Wrapper: skip / log
end
end
Generic-->>Wrapper: finished
App->>Wrapper: get_plugin(name) / has_plugin(name)
Wrapper-->>App: metadata / boolean
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
workers/pyproject.toml (1)
49-50: Minor: Consider pinning upper version bound for reproducibility.The addition of
debugpy>=1.8.14follows the existing project pattern of lower-bound-only versioning. However, for dev tools that may introduce breaking changes in future major versions, consider adding an upper bound constraint (e.g.,debugpy>=1.8.14,<2.0.0) to ensure reproducible development environments and prevent unexpected breakage in CI pipelines.This is an optional improvement and applies broadly to the dev dependencies section; the change itself is sound and appropriate.
unstract/core/src/unstract/core/flask/__init__.py (1)
3-3: Consider removing the unused noqa directive.Modern linters understand re-export patterns, making the
noqa: F401directive unnecessary here.Apply this diff if you'd like to clean it up:
-from .plugin import PluginManager, plugin_loader # noqa: F401 +from .plugin import PluginManager, plugin_loaderunstract/core/src/unstract/core/django/__init__.py (1)
1-1: Consider removing the unused noqa directive.Modern linters understand re-export patterns, making the
noqa: F401directive unnecessary here.Apply this diff if you'd like to clean it up:
-from .plugin import DjangoPluginManager, PluginManager, plugin_loader # noqa: F401 +from .plugin import DjangoPluginManager, PluginManager, plugin_loader
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
workers/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.gitignore(1 hunks)unstract/core/src/unstract/core/cache/redis_client.py(2 hunks)unstract/core/src/unstract/core/django/__init__.py(1 hunks)unstract/core/src/unstract/core/django/plugin.py(1 hunks)unstract/core/src/unstract/core/flask/__init__.py(1 hunks)unstract/core/src/unstract/core/flask/plugin.py(1 hunks)unstract/core/src/unstract/core/plugins/.gitignore(1 hunks)unstract/core/src/unstract/core/plugins/__init__.py(1 hunks)unstract/core/src/unstract/core/plugins/plugin_manager.py(1 hunks)workers/pyproject.toml(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.2)
unstract/core/src/unstract/core/flask/__init__.py
3-3: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
unstract/core/src/unstract/core/flask/plugin.py
87-90: Avoid specifying long messages outside the exception class
(TRY003)
unstract/core/src/unstract/core/django/plugin.py
76-79: Avoid specifying long messages outside the exception class
(TRY003)
unstract/core/src/unstract/core/plugins/plugin_manager.py
62-62: Unused static method argument: args
(ARG004)
113-113: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
113-113: Use explicit conversion flag
Replace with conversion flag
(RUF010)
156-156: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
156-156: Use explicit conversion flag
Replace with conversion flag
(RUF010)
unstract/core/src/unstract/core/django/__init__.py
1-1: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (18)
unstract/core/src/unstract/core/cache/redis_client.py (2)
255-255: LGTM! Documentation clearly explains the alternative variable.The documentation update appropriately describes REDIS_USERNAME as an optional alternative to REDIS_USER, providing clarity for configuration flexibility.
267-267: No changes required—the implementation is correct and intentional.The fallback logic at line 267 is documented (see the docstring at line 255) and serves as a backward-compatibility mechanism. The behavior where empty
REDIS_USERfalls back toREDIS_USERNAMEis by design, allowing flexible configuration. Verification shows:
REDIS_USERNAMEis actively used (e.g., platform-service) as an alternative toREDIS_USER- Sample configurations consistently use
REDIS_USER=default(never empty), with no issues found- The implementation correctly handles all cases: missing variables return
None, and the fallback provides configuration flexibility.gitignore (1)
646-647: LGTM! Plugin directory exclusion is consistent.The addition properly excludes the subscription usage plugin directory, aligning with the plugin infrastructure strategy.
unstract/core/src/unstract/core/plugins/.gitignore (1)
1-7: LGTM! Effective OSS/enterprise separation strategy.The ignore pattern properly isolates enterprise plugins while preserving core infrastructure files.
unstract/core/src/unstract/core/plugins/plugin_manager.py (5)
1-25: LGTM! Well-documented framework-agnostic plugin manager.The module docstring and class documentation clearly communicate the purpose and capabilities.
30-61: LGTM! Comprehensive initialization with good defaults.The parameter documentation is thorough, and the path normalization handles both string and Path inputs appropriately.
62-72: LGTM! Singleton pattern correctly implemented.The conditional singleton behavior based on
use_singletonis properly implemented. The static analysis warning about unusedargsis a false positive—the parameter is necessary for the__new__signature even though it's not directly used in this method.
74-161: LGTM! Robust plugin loading with comprehensive error handling.The plugin loading logic handles multiple scenarios well:
- Missing plugin directory with graceful degradation
- Support for both directory and .so modules
- Metadata validation with disabled plugin filtering
- Proper error isolation per plugin
The static analysis suggestions to use
logging.exception(lines 113, 156) are style preferences—usinglogging.errorin a loop context is acceptable since you want to continue processing other plugins.
162-199: LGTM! Clean accessor API with safe defaults.The plugin accessor methods provide a consistent interface with appropriate return values for missing plugins.
unstract/core/src/unstract/core/plugins/__init__.py (1)
1-12: LGTM! Clean package initialization with clear documentation.The module docstring effectively explains the plugin system's purpose and framework integration options.
unstract/core/src/unstract/core/flask/plugin.py (4)
1-14: LGTM! Well-documented Flask integration.The module docstring clearly explains the Flask-specific functionality.
25-60: LGTM! Flexible singleton pattern with parameter updates.The
__new__implementation properly handles the singleton pattern while allowing parameter updates when plugins directory or package changes. The conditional reinitialization ensures the manager stays synchronized with configuration changes.
74-91: LGTM! Appropriate initialization and loading with error checking.The manager initialization with Flask-specific parameters (app.logger, blueprint callback, src submodule) is correct, and the RuntimeError in
load_pluginsprovides clear guidance when used incorrectly.
93-155: LGTM! Consistent accessor API with safe defaults.The accessor methods and convenience function provide a clean, user-friendly API with appropriate safety checks.
unstract/core/src/unstract/core/django/plugin.py (4)
1-14: LGTM! Well-documented Django integration.The module docstring clearly explains Django-specific functionality and differences from the Flask wrapper.
24-71: LGTM! Clean Django-specific implementation.The singleton pattern and manager initialization are simpler than the Flask version, appropriately reflecting Django's different lifecycle. The comment on Line 69 helpfully explains why no registration callback is needed.
73-122: LGTM! Consistent accessor API with safe defaults.The methods mirror the Flask wrapper's interface, providing consistency across framework integrations.
128-163: LGTM! Excellent documentation with practical example.The docstring example demonstrates the complete usage pattern clearly, making it easy for developers to integrate the plugin system into Django apps.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
.gitignore(0 hunks)backend/plugins/.gitignore(1 hunks)
💤 Files with no reviewable changes (1)
- .gitignore
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
NOTE: Existing mechanism is left untouched to avoid breaking anything
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/plugins/__init__.py (1)
37-60: Convenience function simplifies plugin access.The get_plugin() wrapper provides a clean single-line interface for retrieving plugin metadata, which is well-documented with clear examples.
Optionally, you can sort
__all__alphabetically to address the static analysis hint (RUF022), though this is a minor formatting preference.Apply this diff if you want to address the linting hint:
-__all__ = ["get_plugin_manager", "get_plugin"] +__all__ = ["get_plugin", "get_plugin_manager"]backend/plugins/apps.py (1)
20-34: Plugin loading implementation is appropriate for opt-in plugin system.The ready() method correctly:
- Uses local import to avoid AppRegistryNotReady errors
- Loads plugins after Django initialization
- Implements graceful error handling that allows Django to continue even if plugins fail
This is the right approach for an additive, backward-compatible plugin system.
Optionally, consider using
logger.debug()instead oflogger.info()on line 31 to reduce log noise in production, since successful plugin loading is expected behavior rather than noteworthy information.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
backend/backend/settings/base.py(1 hunks)backend/plugins/__init__.py(1 hunks)backend/plugins/apps.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.2)
backend/plugins/__init__.py
60-60: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
backend/plugins/__init__.py (2)
1-11: LGTM!The module initialization is clean and follows Python best practices with proper docstring, imports, and logger configuration.
17-34: Lazy initialization pattern is appropriate for Django AppConfig integration.The singleton pattern correctly defers plugin loading until requested, and the docstring clearly documents that loading happens separately in PluginsConfig.ready(). The implementation is clean and follows the module-level singleton pattern.
Note: The pattern is not thread-safe (race condition possible if multiple threads call this concurrently), but since Django's ready() hook is called once during startup and the PR describes this as opt-in, this should be acceptable for the current use case.
backend/backend/settings/base.py (1)
289-289: LGTM!Explicitly referencing the AppConfig class (
plugins.apps.PluginsConfig) follows Django best practices and ensures thePluginsConfig.ready()hook is properly invoked during Django startup to load plugins.backend/plugins/apps.py (1)
1-18: LGTM!The AppConfig class is properly configured with appropriate name and verbose_name attributes. The docstring clearly explains the purpose of deferred loading to avoid Django initialization issues.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/plugins/.gitignore (1)
1-15: Good—critical fix has been properly applied.The past review flagged a critical issue where the broad
*/pattern (line 3) would prevent negations on child paths because git doesn't descend into ignored parent directories. The current code correctly addresses this by un-ignoring parent directories (!authentication/,!api/) before their child paths, allowing git to descend and apply the negation rules.The structure is now semantically correct: each child path negation is now reachable because its parent has been un-ignored first.
Minor note: Line 15 uses
!workflow_managerwhile other directories use the trailing slash form (!authentication/,!api/,!api/dto/). For consistency and explicitness, consider changing it to!workflow_manager/if it's a directory.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
backend/plugins/.gitignore(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
unstract/core/src/unstract/core/plugins/README.md (1)
101-106: Add language specifier to fenced code block.The directory structure example should specify a language identifier for the code fence (e.g.,
textorplaintext) to improve rendering and satisfy markdown linting rules.Apply this diff:
-``` +```text unstract/core/src/unstract/core/plugins/ ├── README.md # This file ├── __init__.py # Package initialization ├── plugin_manager.py # Core plugin discovery and loading</blockquote></details> <details> <summary>unstract/core/src/unstract/core/flask/plugin.py (2)</summary><blockquote> `25-60`: **Consider documenting the three-parameter initialization requirement.** The singleton initialization logic requires all three parameters (`app`, `plugins_dir`, `plugins_pkg`) to be present before initializing the manager (line 50). This requirement isn't immediately obvious from the method signature, which marks all parameters as optional. Consider either: 1. Adding a note in the docstring explaining that initialization only occurs when all three parameters are provided, or 2. Validating at instantiation time and providing a clear error if only some parameters are provided This would prevent confusing usage patterns where a user provides only some parameters and expects initialization to occur. --- `84-91`: **Error message could be extracted to a constant.** Ruff suggests extracting long error messages to a constant or defining them in a custom exception class. This is a minor style improvement that can enhance maintainability. If desired, apply this refactor: ```diff +_NOT_INITIALIZED_ERROR = ( + "FlaskPluginManager not initialized. " + "Call with app, plugins_dir, and plugins_pkg first." +) + def load_plugins(self) -> None: """Load plugins using the generic manager.""" if not self._initialized: - raise RuntimeError( - "FlaskPluginManager not initialized. " - "Call with app, plugins_dir, and plugins_pkg first." - ) + raise RuntimeError(_NOT_INITIALIZED_ERROR) self._manager.load_plugins()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
backend/plugins/.gitignore(1 hunks)backend/plugins/README.md(1 hunks)unstract/core/src/unstract/core/flask/plugin.py(1 hunks)unstract/core/src/unstract/core/plugins/.gitignore(1 hunks)unstract/core/src/unstract/core/plugins/README.md(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- backend/plugins/README.md
- backend/plugins/.gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- unstract/core/src/unstract/core/plugins/.gitignore
🧰 Additional context used
🪛 LanguageTool
unstract/core/src/unstract/core/plugins/README.md
[grammar] ~1-~1: Ensure spelling is correct
Context: # Unstract Plugin System ## Plugin Metadata Forma...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
unstract/core/src/unstract/core/plugins/README.md
101-101: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.2)
unstract/core/src/unstract/core/flask/plugin.py
87-90: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
unstract/core/src/unstract/core/flask/plugin.py (3)
65-72: LGTM! Blueprint registration correctly uses metadata.The blueprint registration callback correctly retrieves the plugin name from
metadata.get('name', 'unknown'), addressing the previous review comment. The implementation properly registers Flask blueprints and logs the activity.
93-124: LGTM! Safe default pattern for uninitialized access.The implementation correctly returns safe defaults (empty dict or False) when the manager is not yet initialized, preventing errors while maintaining a clean API. This is good defensive programming.
131-155: LGTM! Clean convenience function with helpful documentation.The
plugin_loaderconvenience function provides a clean, straightforward API for the common use case of creating a plugin manager and loading plugins in one call. The docstring example is clear and helpful.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
unstract/core/src/unstract/core/flask/plugin.py(1 hunks)unstract/core/src/unstract/core/plugins/plugin_manager.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.2)
unstract/core/src/unstract/core/flask/plugin.py
86-89: Avoid specifying long messages outside the exception class
(TRY003)
unstract/core/src/unstract/core/plugins/plugin_manager.py
61-61: Unused static method argument: args
(ARG004)
133-133: Consider (*rel_parts, pkg_name) instead of concatenation
Replace with (*rel_parts, pkg_name)
(RUF005)
142-142: Consider (*rel_parts, item.name) instead of concatenation
Replace with (*rel_parts, item.name)
(RUF005)
179-179: Loop control variable plugin_path not used within loop body
Rename unused plugin_path to _plugin_path
(B007)
191-191: Use explicit conversion flag
Replace with conversion flag
(RUF010)
237-239: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
238-238: Use explicit conversion flag
Replace with conversion flag
(RUF010)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
- Update plugin_loader docstring to clarify plugins_dir and plugins_pkg are required - Add parameter validation with helpful ValueError message - Include practical example showing correct usage pattern - Improves developer experience with clear setup requirements Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
unstract/core/src/unstract/core/flask/plugin.py (1)
83-123: Consider consistent error handling across methods.There's an inconsistency in how methods handle the uninitialized state:
load_plugins()raisesRuntimeErrorwith a helpful messageget_plugin(),has_plugin(), andpluginsreturn safe defaults ({}orFalse)While this may be intentional (mutations vs. queries), it could surprise callers who expect consistent behavior. Consider either:
- Having all methods raise when uninitialized (fail-fast approach), or
- Having all methods return safe defaults (defensive approach)
The current approach is acceptable if documented, but consistency improves predictability.
Note: Static analysis (Ruff TRY003) suggests extracting the long error messages on lines 86-89 into custom exception classes, though this is a style preference.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
unstract/core/src/unstract/core/flask/plugin.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.2)
unstract/core/src/unstract/core/flask/plugin.py
86-89: Avoid specifying long messages outside the exception class
(TRY003)
158-164: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
unstract/core/src/unstract/core/flask/plugin.py (4)
1-14: LGTM! Clear module documentation and appropriate imports.The module docstring clearly describes the Flask-specific wrapper purpose, and all imports are necessary for the implementation.
62-81: LGTM! Blueprint registration correctly implemented.The Flask registration callback properly retrieves the plugin name from metadata (line 71), and the generic manager is correctly initialized with Flask-specific parameters. The past review issue regarding incorrect name source has been properly addressed.
126-127: LGTM! Backward compatibility maintained.The alias ensures existing code using
PluginManagercontinues to work.
130-167: LGTM! Parameter validation addresses previous concerns.The function now properly validates that both
plugins_dirandplugins_pkgare provided before creating the manager (lines 157-164), which addresses the previous review comment about failing with default usage. The clear error message with example usage helps developers understand the requirements.
Note: Static analysis (Ruff TRY003) suggests extracting the long error message on lines 158-164 into a custom exception class, though this is a style preference and the current inline message with example is quite helpful.
- Add threading.Lock at class level to protect singleton instance - Guard instance creation and state mutations with lock - Prevent race conditions in multi-threaded Flask environments - Ensure thread-safe initialization of app, plugins_dir, and plugins_pkg 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
unstract/core/src/unstract/core/flask/plugin.py (1)
87-94: Consider extracting exception messages to exception classes.The error message is clear and helpful. However, static analysis flags TRY003 for long messages defined inline. This is a minor style consideration—the current implementation is functional.
If you prefer to align with Ruff's recommendation, you could define custom exception classes:
class PluginManagerNotInitializedError(RuntimeError): """Raised when FlaskPluginManager is used before initialization.""" def __init__(self): super().__init__( "FlaskPluginManager not initialized. " "Call with app, plugins_dir, and plugins_pkg first." )Then raise with
raise PluginManagerNotInitializedError(). The same pattern applies to line 162-168. However, this adds complexity for minimal benefit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
unstract/core/src/unstract/core/flask/plugin.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.2)
unstract/core/src/unstract/core/flask/plugin.py
90-93: Avoid specifying long messages outside the exception class
(TRY003)
162-168: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (6)
unstract/core/src/unstract/core/flask/plugin.py (6)
1-15: LGTM: Clean imports and clear documentation.The module docstring effectively describes the Flask integration purpose, and all imports are necessary and properly organized.
24-64: LGTM: Thread-safe singleton pattern correctly implemented.The previous thread-safety concern has been properly addressed. The lock guards the entire instance creation and initialization logic, preventing race conditions in multi-threaded Flask environments. The short-circuit evaluation on line 56 ensures
_plugins_dirand_plugins_pkgare only checked after they've been set.
66-85: LGTM: Flask integration callback correctly implemented.The previous issue with blueprint registration logging has been fixed—line 75 now correctly retrieves the plugin name from
metadatarather thanplugin_data. The registration callback properly integrates Flask blueprints and logging.
96-127: LGTM: Consistent defensive query API.The query methods (
get_plugin,has_plugin, andpluginsproperty) all follow a consistent pattern of returning safe defaults when the manager is not initialized, preventing crashes while signaling the uninitialized state. This is a good defensive design that maintains usability.
130-131: LGTM: Clean backward compatibility alias.The alias maintains compatibility with existing code while the new
FlaskPluginManagername better reflects the class's purpose.
134-171: LGTM: Well-designed convenience function with proper validation.The previous issue where
plugin_loadercould fail with default parameters has been properly fixed. The validation on lines 161-168 now ensures both required parameters are provided before initialization, and the error message includes helpful usage examples. The function signature and documentation clearly indicate the requirements.
…dling - Merge nested if statements in FlaskPluginManager.getInstance() into single compound condition - Remove redundant PermissionError from plugin_manager (OSError parent class covers it) 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
unstract/core/src/unstract/core/plugins/plugin_manager.py (3)
133-133: Consider using tuple unpacking for concatenation.The tuple concatenations can be simplified using unpacking syntax for slightly better readability and performance.
Apply this diff:
- module_path = ".".join(rel_parts + (pkg_name,)) + module_path = ".".join((*rel_parts, pkg_name))- new_rel_parts = rel_parts + (item.name,) + new_rel_parts = (*rel_parts, item.name)Also applies to: 142-142
179-179: Rename unused loop variable to indicate it's intentionally unused.The
plugin_pathvariable is not used within the loop body. Following Python conventions, prefix it with an underscore to indicate it's intentionally unused.Apply this diff:
- for plugin_path, module_name in plugin_candidates: + for _plugin_path, module_name in plugin_candidates:
236-239: Uselogging.exceptionfor better error diagnostics.When logging errors in an exception handler, use
logging.exception()instead oflogging.error()to automatically include the stack trace, which aids debugging.Apply this diff:
except KeyError as e: - self.logger.error( + self.logger.exception( f"Invalid metadata for plugin '{module_name}': {str(e)}" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
unstract/core/src/unstract/core/flask/plugin.py(1 hunks)unstract/core/src/unstract/core/plugins/plugin_manager.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.2)
unstract/core/src/unstract/core/flask/plugin.py
94-97: Avoid specifying long messages outside the exception class
(TRY003)
166-172: Avoid specifying long messages outside the exception class
(TRY003)
unstract/core/src/unstract/core/plugins/plugin_manager.py
61-61: Unused static method argument: args
(ARG004)
133-133: Consider (*rel_parts, pkg_name) instead of concatenation
Replace with (*rel_parts, pkg_name)
(RUF005)
142-142: Consider (*rel_parts, item.name) instead of concatenation
Replace with (*rel_parts, item.name)
(RUF005)
179-179: Loop control variable plugin_path not used within loop body
Rename unused plugin_path to _plugin_path
(B007)
191-191: Use explicit conversion flag
Replace with conversion flag
(RUF010)
237-239: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
238-238: Use explicit conversion flag
Replace with conversion flag
(RUF010)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
unstract/core/src/unstract/core/flask/plugin.py (2)
24-68: Thread-safe singleton implementation looks good!The double-checked locking pattern with
threading.Lockcorrectly handles concurrent access in multi-threaded Flask applications. The lock guards both instance creation and state updates, preventing race conditions.
165-175: Parameter validation prevents initialization errors effectively.The validation requiring both
plugins_dirandplugins_pkgcorrectly prevents the RuntimeError that would occur if the manager were created without proper initialization parameters. The helpful error message with usage example improves developer experience.
muhammad-ali-e
left a comment
There was a problem hiding this comment.
Added some minor comments .. please address those
…alized get_plugin() mechanism (#1626) Refactor plugin loading to use centralized get_plugin() mechanism Replaced manual plugin loading implementations across backend, prompt-service, and core modules with the new centralized get_plugin() function. This aligns all plugin loading with the new infrastructure, reduces code duplication, and simplifies maintenance. Removed obsolete loader files (subscription_loader, modifier_loader, processor_loader) that are now replaced by get_plugin(). Generated with Claude Code Co-authored-by: Claude <noreply@anthropic.com>
UN-2930 [FEAT] Implement subscription tracking for successful executions only This PR implements the complete UN-2930 fix to track subscription usage only for successful API calls and workflow executions. Key changes: - Add subscription usage tracking decorator for Django views - Apply decorator to Prompt Studio index and answer-prompt operations - Add workflow completion/failure handlers for defer/commit/discard pattern - Implement batch tracking in workers for completed workflows - Add platform-service plugin integration for subscription tracking - Remove hardcoded SQL from platform-service, use plugin-based approach Components: 1. Decorator (backend/utils/subscription_usage_decorator.py): - Auto-commits usage on success - Auto-discards usage on failure - Non-blocking error handling 2. Prompt Studio Integration: - Track usage for index_document() - Track usage for prompt_responder() 3. Workflow Integration: - Completion handler commits deferred usage for COMPLETED workflows - Failure handler discards all deferred usage 4. Workers Integration: - Batch commit for successful file executions - Calls internal API for efficient batch processing 5. Platform Service: - Plugin-based subscription tracking - Cleaner separation of concerns This ensures usage is only recorded when operations actually succeed, fixing the issue where failed operations were incorrectly tracked. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/prompt_studio/prompt_studio_core_v2/prompt_studio_helper.py (1)
484-517: Preventsummarizefrom returningNonewhen the plugin is absentIf
get_plugin("summarizer")returns an empty dict (the common OSS setup), we skip the plugin block and fall off the end ofsummarizewithout returning a file path.index_documentstill expects a path and passesNoneintoIndexingUtils.generate_index_key, triggering a runtimeTypeErrorand breaking every summarization-enabled tool. Previously this path worked without any plugin. Please restore a fallback that produces the same filepath we used before (or fail fast with a controlled exception) whenever the plugin isn’t available so that OSS deployments keep functioning.
🧹 Nitpick comments (8)
prompt-service/.gitignore (1)
1-2: Gitignore entry aligns with plugin infrastructure changes.The entry correctly excludes the prompt-service plugins directory from version control, supporting the new plugin discovery and loading system. Consider adding a trailing slash to explicitly indicate this is a directory match.
- src/unstract/prompt_service/plugins + src/unstract/prompt_service/plugins/backend/configuration/config_registry.py (1)
64-64: Preferlogger.exceptionto keep stack trace.Switching to
logger.exceptionpreserves the traceback, which is invaluable when diagnosing plugin load failures, while keeping the log at error level.- logger.error("Error loading cloud configuration plugin: %s", e) + logger.exception("Error loading cloud configuration plugin: %s", e)backend/adapter_processor_v2/views.py (1)
345-380: Plugin-based notification integration looks solid.The conditional plugin loading, availability guard, and exception handling ensure backward compatibility. The flow correctly:
- Checks plugin availability before use
- Imports ResourceType only when needed
- Instantiates the service from plugin metadata
- Gracefully handles failures without disrupting the main flow
Minor logging improvement suggestion:
Line 380 uses
logger.exception(f"Failed to send sharing notification: {e}"). The redundant exception object in the f-string can be removed sincelogger.exceptionautomatically includes exception details.Apply this diff:
- logger.exception(f"Failed to send sharing notification: {e}") + logger.exception("Failed to send sharing notification")backend/workflow_manager/workflow_v2/workflow_helper.py (1)
239-255: Plugin-based failure notification added correctly.The code extracts organization_id, checks plugin availability, and invokes the failure handler without disrupting the primary error flow. The exception handling ensures robustness.
Logging improvement suggestion:
Line 253-255 uses
logger.errorwith exception details in the f-string. Per the static analysis hint,logger.exceptionis preferred within exception handlers as it automatically captures stack traces.Apply this diff:
except Exception as e: - logger.error( - f"Error in subscription usage plugin failure handler: {e}" - ) + logger.exception( + "Error in subscription usage plugin failure handler" + )platform-service/src/unstract/platform_service/controller/platform.py (1)
167-179: Plugin-based subscription usage handling implemented correctly.The code checks plugin availability, instantiates the handler, and invokes
handle_subscription_usage. Exception handling ensures the main page_usage flow continues even if the plugin fails.Logging refinement (per static analysis):
Line 179 includes the exception object in the f-string when using
logger.exception. This is redundant sincelogger.exceptionautomatically captures exception details.Apply this diff:
except Exception as e: - app.logger.exception(f"Error from subscription usage plugin: {e}") + app.logger.exception("Error from subscription usage plugin")backend/utils/subscription_usage_decorator.py (1)
89-139: Commit/discard logic correctly implements subscription tracking.The try/except structure commits usage on success and discards on failure. Plugin availability guards and error handling ensure robustness.
Logging improvements (per static analysis):
- Lines 107-110 and 129-132 should use
logger.exceptioninstead oflogger.errorto automatically capture stack traces- Line 112's return could be moved to an else block for clarity (though current form is acceptable)
Apply these diffs:
except Exception as commit_error: - logger.error( - f"Error committing subscription usage for " - f"file_execution_id {file_execution_id}: {commit_error}" - ) + logger.exception( + f"Error committing subscription usage for " + f"file_execution_id {file_execution_id}" + )except Exception as discard_error: - logger.error( - f"Error discarding subscription usage for " - f"file_execution_id {file_execution_id}: {discard_error}" - ) + logger.exception( + f"Error discarding subscription usage for " + f"file_execution_id {file_execution_id}" + )Note: The broad
Exceptioncatching is justified here as this is a defensive wrapper that should not crash the wrapped function due to tracking issues.backend/workflow_manager/workflow_v2/file_execution_tasks.py (1)
373-389: Uselogging.exceptionfor better error diagnostics.The error handling correctly prevents plugin failures from blocking execution. However, for better debugging, use
logger.exception()instead oflogger.error()to automatically include the stack trace.Apply this diff:
- except Exception as e: - logger.error( - f"Error in subscription usage plugin completion handler: {e}" - ) + except Exception: + logger.exception( + "Error in subscription usage plugin completion handler" + )backend/prompt_studio/prompt_studio_core_v2/views.py (1)
162-192: Consider aligning notification plugin pattern with other views.Unlike other view files in this PR (pipeline_v2/views.py, workflow_v2/views.py, api_deployment_views.py), this file imports and checks the notification plugin inline within the method rather than at module level. Consider moving the plugin availability check to module level for consistency.
Example pattern from other files:
# At module level (after other imports) notification_plugin = get_plugin("notification") NOTIFICATION_PLUGIN_AVAILABLE = notification_plugin is not None if NOTIFICATION_PLUGIN_AVAILABLE: from plugins.notification.constants import ResourceTypeThen in the method:
if NOTIFICATION_PLUGIN_AVAILABLE: # Use pluginAdditionally, per static analysis, use
logging.exception()without the redundant exception object in the call:- logger.exception( - f"Failed to send sharing notification for " - f"custom tool {custom_tool.tool_id}: {str(e)}" - ) + logger.exception( + "Failed to send sharing notification for " + f"custom tool {custom_tool.tool_id}" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (28)
backend/account_v2/subscription_loader.py(0 hunks)backend/adapter_processor_v2/views.py(1 hunks)backend/api_v2/api_deployment_dto_registry.py(2 hunks)backend/api_v2/api_deployment_views.py(3 hunks)backend/configuration/config_registry.py(1 hunks)backend/pipeline_v2/views.py(3 hunks)backend/plugins/.gitignore(1 hunks)backend/plugins/api/dto/__init__.py(1 hunks)backend/prompt_studio/modifier_loader.py(0 hunks)backend/prompt_studio/processor_loader.py(0 hunks)backend/prompt_studio/prompt_studio_core_v2/prompt_studio_helper.py(7 hunks)backend/prompt_studio/prompt_studio_core_v2/views.py(5 hunks)backend/prompt_studio/prompt_studio_registry_v2/prompt_studio_registry_helper.py(2 hunks)backend/scheduler/tasks.py(1 hunks)backend/utils/subscription_usage_decorator.py(1 hunks)backend/workflow_manager/workflow_v2/file_execution_tasks.py(1 hunks)backend/workflow_manager/workflow_v2/views.py(14 hunks)backend/workflow_manager/workflow_v2/workflow_helper.py(1 hunks)platform-service/.gitignore(1 hunks)platform-service/src/unstract/platform_service/config.py(2 hunks)platform-service/src/unstract/platform_service/controller/platform.py(2 hunks)prompt-service/.gitignore(1 hunks)prompt-service/src/unstract/prompt_service/config.py(2 hunks)prompt-service/src/unstract/prompt_service/controllers/answer_prompt.py(1 hunks)prompt-service/src/unstract/prompt_service/helpers/plugin.py(0 hunks)prompt-service/src/unstract/prompt_service/services/answer_prompt.py(2 hunks)unstract/core/src/unstract/core/plugins/plugin_manager.py(1 hunks)workers/callback/tasks.py(10 hunks)
💤 Files with no reviewable changes (4)
- backend/prompt_studio/modifier_loader.py
- backend/prompt_studio/processor_loader.py
- backend/account_v2/subscription_loader.py
- prompt-service/src/unstract/prompt_service/helpers/plugin.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/plugins/.gitignore
🧰 Additional context used
🪛 Ruff (0.14.4)
backend/utils/subscription_usage_decorator.py
106-106: Do not catch blind exception: Exception
(BLE001)
107-110: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
112-112: Consider moving this statement to an else block
(TRY300)
128-128: Do not catch blind exception: Exception
(BLE001)
129-132: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
backend/plugins/api/dto/__init__.py
31-31: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
backend/workflow_manager/workflow_v2/file_execution_tasks.py
386-386: Do not catch blind exception: Exception
(BLE001)
387-389: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
backend/workflow_manager/workflow_v2/workflow_helper.py
252-252: Do not catch blind exception: Exception
(BLE001)
253-255: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
unstract/core/src/unstract/core/plugins/plugin_manager.py
61-61: Unused static method argument: args
(ARG004)
133-133: Consider (*rel_parts, pkg_name) instead of concatenation
Replace with (*rel_parts, pkg_name)
(RUF005)
142-142: Consider (*rel_parts, item.name) instead of concatenation
Replace with (*rel_parts, item.name)
(RUF005)
179-179: Loop control variable plugin_path not used within loop body
Rename unused plugin_path to _plugin_path
(B007)
191-191: Use explicit conversion flag
Replace with conversion flag
(RUF010)
237-239: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
238-238: Use explicit conversion flag
Replace with conversion flag
(RUF010)
platform-service/src/unstract/platform_service/controller/platform.py
179-179: Redundant exception object included in logging.exception call
(TRY401)
workers/callback/tasks.py
1343-1343: Consider moving this statement to an else block
(TRY300)
backend/scheduler/tasks.py
109-109: Do not catch blind exception: Exception
(BLE001)
backend/configuration/config_registry.py
63-63: Do not catch blind exception: Exception
(BLE001)
64-64: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
backend/prompt_studio/prompt_studio_core_v2/views.py
191-191: Use explicit conversion flag
Replace with conversion flag
(RUF010)
191-191: Redundant exception object included in logging.exception call
(TRY401)
🔇 Additional comments (24)
platform-service/.gitignore (1)
1-2: No issues found. This is a sound refactoring.The platform-service/.gitignore previously contained a full duplicate of the root .gitignore patterns. The change simplifies it to include only directory-specific patterns (
src/unstract/platform_service/plugins), since all global Python, build, IDE, and OS artifacts are already handled by the root-level .gitignore. Git hierarchically applies .gitignore files, so this redundancy has been safely removed without losing any safeguards. No Python artifacts are currently being tracked in the platform-service directory.prompt-service/src/unstract/prompt_service/controllers/answer_prompt.py (1)
8-8: LGTM - Import path correctly updated.The PluginManager import now sources from the centralized
unstract.core.flaskmodule, aligning with the framework-agnostic plugin infrastructure introduced in this PR. No behavioral changes.platform-service/src/unstract/platform_service/config.py (3)
3-3: Path import added for plugin directory resolution.
8-12: Import reorganization includes PluginManager.The imports are now grouped logically with the plugin manager alongside error handlers and middleware registration utilities.
49-53: Plugin loading correctly integrated into app creation.The plugin directory is resolved relative to the config file, the package name is specified, and
load_plugins()is called before returning the app. This ensures plugins are discovered and registered during startup.prompt-service/src/unstract/prompt_service/services/answer_prompt.py (2)
9-9: LGTM - Import path correctly updated.Consistent with the framework-agnostic plugin system refactor.
225-230: Helpful logging addition for highlight plugin initialization.The debug information (file_path, execution_source, fs_instance) will assist in troubleshooting highlight plugin setup issues.
platform-service/src/unstract/platform_service/controller/platform.py (1)
11-11: PluginManager import added.backend/prompt_studio/prompt_studio_registry_v2/prompt_studio_registry_helper.py (2)
8-8: Dynamic plugin import added.
355-363: Payload modifier successfully migrated to plugin-based approach.The code checks for plugin availability, instantiates the service from the plugin metadata, and calls the
updatemethod. The conditional guard ensures backward compatibility when the plugin is unavailable.backend/utils/subscription_usage_decorator.py (2)
1-51: Well-designed decorator for subscription usage tracking.The decorator factory provides a clean, reusable pattern for tracking subscription usage with automatic commit/discard logic. The docstring clearly explains usage patterns and parameters.
53-87: Parameter extraction logic handles multiple call patterns correctly.The code handles:
- Regular function calls with named parameters
- Django view methods (self, request, ...)
- Extraction from request object when needed
- Graceful skip when identifiers are missing
prompt-service/src/unstract/prompt_service/config.py (2)
4-4: LGTM! Clean import additions for plugin infrastructure.The imports are necessary for the new plugin loading approach and are well-organized.
Also applies to: 9-13
49-52: LGTM! Clean plugin initialization.The plugin manager initialization follows a clear pattern with proper directory and package configuration.
backend/api_v2/api_deployment_dto_registry.py (2)
4-4: LGTM! Clean import change for plugin-based architecture.The switch to
get_pluginenables dynamic plugin loading.
15-25: LGTM! Well-designed plugin-based DTO loading with fallback.The implementation gracefully handles both plugin availability and OSS scenarios with a sensible default.
backend/pipeline_v2/views.py (2)
36-42: LGTM! Clean plugin availability pattern.The conditional import approach gracefully handles both plugin presence and absence scenarios.
151-161: LGTM! Consistent plugin-based notification pattern.The implementation follows the established pattern for dynamic plugin usage with proper error handling.
backend/plugins/api/dto/__init__.py (1)
1-29: LGTM! Clean plugin structure with clear documentation.The ApiDtoService provides a well-defined interface for cloud-specific DTO extensions, and the metadata follows the plugin system conventions.
backend/workflow_manager/workflow_v2/views.py (1)
61-67: LGTM! Consistent plugin-based notification implementation.The implementation follows the established pattern across the codebase with proper availability checks and error handling.
Also applies to: 170-180
backend/api_v2/api_deployment_views.py (2)
35-40: LGTM! Clean plugin availability pattern.The conditional plugin loading follows the established pattern consistently.
298-308: LGTM! Proper notification service usage with error handling.The implementation correctly uses the plugin-provided service class with appropriate error handling.
backend/prompt_studio/prompt_studio_core_v2/views.py (2)
439-446: LGTM! Clean plugin usage with sensible fallback.The file_converter plugin usage gracefully handles both plugin presence and absence with appropriate defaults.
474-487: LGTM! Proper conditional file conversion.The plugin-based file conversion correctly handles both availability and file type conditions.
Signed-off-by: Chandrasekharan M <117059509+chandrasekharan-zipstack@users.noreply.github.com>
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/prompt_studio/prompt_studio_core_v2/prompt_studio_helper.py (2)
484-516: Fix missing return value when summarizer plugin is unavailable.The
summarize()method doesn't return a value whensummarizer_pluginis None. The caller at Line 427 assigns the result tosummarize_file_path, which is then used at Line 436 to generatesummarize_doc_id. This will cause a runtime error.Add appropriate handling for when the plugin is unavailable:
@staticmethod def summarize(file_name, org_id, document_id, run_id, tool, doc_id) -> str: summarizer_plugin = get_plugin("summarizer") usage_kwargs: dict[Any, Any] = dict() usage_kwargs[ToolStudioPromptKeys.RUN_ID] = run_id prompts: list[ToolStudioPrompt] = PromptStudioHelper.fetch_prompt_from_tool( tool.tool_id ) - if summarizer_plugin: + if not summarizer_plugin: + raise OperationNotSupported("Summarizer plugin is not available") + + try: summarizer_service = summarizer_plugin["service_class"]() summarize_file_path = summarizer_service.process( tool_id=str(tool.tool_id), file_name=file_name, org_id=org_id, user_id=tool.created_by.user_id, usage_kwargs=usage_kwargs.copy(), prompts=prompts, ) # Trigger migration if needed SummarizeMigrationUtils.migrate_tool_to_adapter_based(tool) # Validate that summarization is properly configured if not tool.summarize_llm_adapter: # Fallback to old approach if no adapter - just validate it exists try: ProfileManager.objects.get( prompt_studio_tool=tool, is_summarize_llm=True ) except ProfileManager.DoesNotExist: logger.warning( f"No summarize profile found for tool {tool.tool_id}, using default profile" ) return summarize_file_path + except Exception as e: + logger.error(f"Error during summarization: {e}") + raise
1037-1048: Add error handling for plugin service usage.Similar to the issue in
get_select_fields(), this code lacks error handling around plugin service instantiation and method calls.Wrap the plugin usage in error handling:
# Update output with payload modifier plugin if available payload_modifier_plugin = get_plugin("payload_modifier") if payload_modifier_plugin: - modifier_service = payload_modifier_plugin["service_class"]() - output = modifier_service.update( - output=output, - tool_id=tool_id, - prompt_id=str(prompt.prompt_id), - prompt=prompt.prompt, - input_file=extract_doc_path, - clean_pages=True, - ) + try: + modifier_service = payload_modifier_plugin["service_class"]() + output = modifier_service.update( + output=output, + tool_id=tool_id, + prompt_id=str(prompt.prompt_id), + prompt=prompt.prompt, + input_file=extract_doc_path, + clean_pages=True, + ) + except Exception as e: + logger.error(f"Failed to update output with payload modifier: {e}") + raise
🧹 Nitpick comments (1)
backend/api_v2/api_deployment_views.py (1)
320-320: LGTM: Notification sending properly guarded and error-handled.The plugin availability check (line 320) ensures notifications are only attempted when the plugin is present, and the try-except wrapper (lines 322-340) ensures that notification failures don't disrupt the main update flow. The dynamic service class instantiation is appropriate for this plugin architecture.
Optionally, you could add a defensive check before accessing the plugin dictionary to provide a more specific error message for debugging:
if newly_shared_users: # Get notification service from plugin - service_class = notification_plugin["service_class"] + service_class = notification_plugin.get("service_class") + if not service_class: + logger.warning("Notification plugin missing 'service_class'") + return response notification_service = service_class()However, this is purely optional since the existing try-except already provides adequate error handling and logging.
Also applies to: 328-338
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
workers/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
backend/api_v2/api_deployment_views.py(3 hunks)backend/backend/settings/base.py(1 hunks)backend/pipeline_v2/views.py(3 hunks)backend/prompt_studio/prompt_studio_core_v2/prompt_studio_helper.py(7 hunks)workers/callback/tasks.py(10 hunks)workers/pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/backend/settings/base.py
- backend/pipeline_v2/views.py
- workers/pyproject.toml
🧰 Additional context used
🪛 Ruff (0.14.4)
workers/callback/tasks.py
1345-1345: Consider moving this statement to an else block
(TRY300)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (11)
backend/api_v2/api_deployment_views.py (1)
10-10: LGTM: Clean plugin availability pattern.The module-level plugin check and conditional import approach is clean and appropriate for opt-in notification support. Note that plugin availability is determined at module import time, which is standard for Django but means runtime plugin changes require a module reload.
Also applies to: 37-42
backend/prompt_studio/prompt_studio_core_v2/prompt_studio_helper.py (4)
15-15: LGTM! Plugin system imports added correctly.The imports for
get_pluginandtrack_subscription_usage_if_availableproperly enable the dynamic plugin infrastructure.Also applies to: 20-20
325-325: LGTM! Subscription tracking decorator correctly applied.The decorator is properly configured with the
run_idparameter for tracking subscription usage.
519-519: LGTM! Subscription tracking decorator correctly applied.The decorator is properly configured for tracking prompt execution usage.
588-594: Good defensive check for plugin availability.The code correctly validates that the
payload_modifierplugin is available before allowing TABLE or RECORD operations, preventing runtime errors downstream.workers/callback/tasks.py (6)
69-69: LGTM - File executions tracking added to context.The new
file_executionsfield appropriately stores file execution metadata needed for subscription usage tracking, with proper type hints and initialization.
723-723: LGTM - File executions consistently populated across all callback paths.The
file_executionsdata is correctly extracted from API responses in all code paths (fast/fallback in core callback, API callback), with safe fallback to empty list.Also applies to: 759-759, 1664-1664
832-834: Good logging enhancement for observability.Including the
file_executionscount helps trace subscription tracking data flow through the callback process.
1424-1428: LGTM - Subscription tracking correctly integrated into core callback flow.The tracking is called at the appropriate point (after execution status is determined) and the result is included for observability. The non-blocking error handling ensures workflow completion isn't affected by tracking failures.
Also applies to: 1449-1449
1684-1688: LGTM - Subscription tracking correctly integrated into API callback flow.The integration follows the same pattern as the core callback, with proper placement and observability. Note that the idempotency concern mentioned in the
_track_subscription_usage_if_availablereview applies here as well, given themax_retries=3configuration for this task.Also applies to: 1718-1718
1269-1358: Verify idempotency of subscription usage plugin or add deduplication safeguard.The retry scenario is confirmed to be possible and problematic:
process_batch_callback_apihasmax_retries=3(line 1536)file_executionsis fetched fresh from the database on each context initialization (lines 723, 759, 1664), making the same file execution IDs available for tracking on each retry- If the task fails after
commit_batch_subscription_usagesucceeds but before the task completes, a retry will re-send the samefile_execution_idsto the plugin- No idempotency markers exist to prevent duplicate tracking
Recommended actions:
- Confirm the subscription usage plugin's
commit_batch_subscription_usagemethod is idempotent (handles duplicate file_execution_ids gracefully)- Or: Add a safeguard such as marking file executions as tracked in the database after successful commit, then check this status before calling the plugin on retry
- Or: Document the idempotency guarantee if the plugin design already handles this scenario
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Chandrasekharan M <117059509+chandrasekharan-zipstack@users.noreply.github.com>
Test ResultsSummary
Runner Tests - Full Report
SDK1 Tests - Full Report
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/configuration/config_registry.py (1)
64-65: Consider usinglogging.exceptionfor better debugging.Using
logging.exceptioninstead oflogging.errorautomatically includes the full traceback, which is valuable when diagnosing plugin loading failures.As per static analysis.
Apply this diff:
except Exception as e: - logger.error("Error loading cloud configuration plugin: %s", e) + logger.exception("Error loading cloud configuration plugin: %s", e)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
backend/configuration/config_registry.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.4)
backend/configuration/config_registry.py
64-64: Do not catch blind exception: Exception
(BLE001)
65-65: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (5)
backend/configuration/config_registry.py (5)
26-33: LGTM: Clean plugin retrieval with early return.The lazy import pattern and early return logic are appropriate for this optional plugin system.
35-39: LGTM: Active plugin validation.The activation check ensures that only enabled plugins contribute configuration, which is good practice for plugin systems.
41-45: LGTM: Defensive module validation.The null check prevents AttributeErrors on missing modules.
48-52: LGTM: Correct fix for module path resolution.The
__package__fallback to__name__properly handles both package-based and top-level module plugins (including .so extensions), addressing the concern from the previous review.
55-60: LGTM: Clear success logging.The informative log message aids debugging and the default plugin name handles missing metadata gracefully.
Changed ConfigPluginConstants.METADATA to ConfigPluginConstants.CONFIG_METADATA to fix silent AttributeError that prevented cloud configuration plugins from loading. This caused ENABLE_HIGHLIGHT_API_DEPLOYMENT and other cloud config keys to always return False. Bug was introduced in PR #1618 (commit 808040c). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…1772) [FIX] Fix incorrect constant reference in config_registry.py Changed ConfigPluginConstants.METADATA to ConfigPluginConstants.CONFIG_METADATA to fix silent AttributeError that prevented cloud configuration plugins from loading. This caused ENABLE_HIGHLIGHT_API_DEPLOYMENT and other cloud config keys to always return False. Bug was introduced in PR #1618 (commit 808040c). Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
#1774) [FIX] Fix incorrect constant reference breaking cloud config loading (#1772) [FIX] Fix incorrect constant reference in config_registry.py Changed ConfigPluginConstants.METADATA to ConfigPluginConstants.CONFIG_METADATA to fix silent AttributeError that prevented cloud configuration plugins from loading. This caused ENABLE_HIGHLIGHT_API_DEPLOYMENT and other cloud config keys to always return False. Bug was introduced in PR #1618 (commit 808040c). Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> Co-authored-by: ali <117142933+muhammad-ali-e@users.noreply.github.com>
NOTE
UN-2930 [FIX] Track subscription usage only for successful runs #1620
[MISC] Refactor plugin loading for BE and prompt-service to use centralized get_plugin() mechanism #1626
What
unstract-coreas wellWhy
How
Core Plugin Infrastructure:
unstract/core/src/unstract/core/plugins/plugin_manager.py- Generic PluginManager with:.socompiled extensionsFramework-Specific Wrappers:
unstract/core/src/unstract/core/django/plugin.py- DjangoPluginManager wrapperplugin_loader()functionunstract/core/src/unstract/core/flask/plugin.py- FlaskPluginManager wrapperSupporting Changes:
.gitignoreto exclude plugin directories that will contain cloud-specific codedebugpy>=1.8.14to workers dev dependencies for debugging supportREDIS_USERNAMEenv var (alternative toREDIS_USER)Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
No, this PR will not break any existing features.
Reasoning:
REDIS_USERsupportEnv Config
Optional (backward compatible):
REDIS_USERNAME- Alternative toREDIS_USERfor Redis authentication (not required)Related Issues or PRs
Dependencies Versions
Added:
debugpy>=1.8.14(dev dependency in workers/pyproject.toml)No changes to runtime dependencies.
Notes on Testing
Pre-commit Validation
Screenshots
N/A - Infrastructure changes only, no UI changes
Checklist
I have read and understood the Contribution Guidelines.
🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com