Conversation
Contributor
Reviewer's GuideThis PR introduces v2 plugin structure support and stricter security/runtime behavior: manifests now return compatibility information, AST security scanning gains a reusable visitor and clearer messages, sandbox resource limits are generalized, plugin loading respects framework version constraints, and CI/linting/pre-commit tooling is modernized to enforce the new conventions. Sequence diagram for plugin loading with framework version compatibilitysequenceDiagram
actor Dev
participant PluginLoader
participant ManifestValidator
participant PluginsDir
participant Logger
Dev->>PluginLoader: load_all()
PluginLoader->>PluginsDir: list plugin directories
loop for each plugin_dir
PluginLoader->>ManifestValidator: load_and_validate(plugin_dir)
ManifestValidator->>ManifestValidator: read plugin.yaml or plugin.json
ManifestValidator->>ManifestValidator: validate required fields
ManifestValidator->>ManifestValidator: validate_version = check_compatibility(framework_version, __version__)
ManifestValidator-->>PluginLoader: manifest, validate_version, __version__
alt validate_version is True
PluginLoader->>PluginLoader: manifests.append(manifest)
else validate_version is False
PluginLoader->>Logger: warning incompatible framework version
end
end
PluginLoader-->>Dev: report (loaded, skipped plugins)
Sequence diagram for sandboxed plugin process start with ping checksequenceDiagram
actor Kernel
participant ProcessManager
participant SandboxProcess
participant IPCClient
Kernel->>ProcessManager: start()
alt state is not READY
ProcessManager-->>Kernel: return (no-op)
else state is READY
ProcessManager->>ProcessManager: _state = STARTING
ProcessManager->>ProcessManager: _spawn()
ProcessManager->>SandboxProcess: create subprocess worker
ProcessManager->>ProcessManager: _ping_check()
ProcessManager->>IPCClient: call("ping", {})
IPCClient-->>ProcessManager: IPCResponse(success=True)
ProcessManager->>ProcessManager: _state = RUNNING
ProcessManager->>ProcessManager: _started_at = now, _restarts = 0
ProcessManager-->>Kernel: started
end
Updated class diagram for security validation and AST scanningclassDiagram
class ManifestValidator {
+load_and_validate(plugin_dir: Path) tuple
-_read_raw(plugin_dir: Path) dict
}
class _SecurityVisitor {
+forbidden: set~str~
+allowed_exact: set~str~
+allowed_prefixes: set~str~
+filename: str
+path: Path
+errors: list~str~
+warnings: list~str~
+forbidden_builtins: set~str~
+_SecurityVisitor(forbidden: set~str~, allowed: set~str~, filename: str, path: Path)
+_is_allowed(module: str) bool
+_check(module: str, lineno: int) void
+visit_Import(node: ast.Import) void
+visit_ImportFrom(node: ast.ImportFrom) void
+visit_Name(node: ast.Name) void
+visit_Attribute(node: ast.Attribute) void
}
class ASTScanner {
-_forbidden: set~str~
-_allowed_exact: set~str~
-_allowed_prefixes: set~str~
+forbidden: set~str~
+allowed: set~str~
+scan(path: Path) ScanResult
+_scan_imports_python(path: Path, src: str, result: ScanResult) void
}
class ast_NodeVisitor {
+visit(node: ast.AST) Any
}
ast_NodeVisitor <|-- _SecurityVisitor
ASTScanner ..> _SecurityVisitor : uses for fine-grained tests
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- ManifestValidator.load_and_validate now returns a tuple, but loader.load still treats it as a single manifest object (e.g. iterating manifest.requires), which will break at runtime; either adjust load() to unpack the tuple as in load_all() or keep load_and_validate’s return type consistent.
- The new FrameworkVersionVersion exception in security/validation.py has a confusing name and is not used anywhere; consider renaming it to something clearer (e.g. FrameworkVersionError) and wiring it into the version compatibility logic, or remove it if it’s not needed.
- The auth/rbac API wiring appears inconsistent: api/init.py imports get_auth_backend from both .auth and .rbac and exports it multiple times, and auth.py now imports Any from typing_extensions instead of typing (typing_extensions.Any is not standard); clean up these imports/exports to avoid name collisions and use typing.Any for the TypedDict annotation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- ManifestValidator.load_and_validate now returns a tuple, but loader.load still treats it as a single manifest object (e.g. iterating manifest.requires), which will break at runtime; either adjust load() to unpack the tuple as in load_all() or keep load_and_validate’s return type consistent.
- The new FrameworkVersionVersion exception in security/validation.py has a confusing name and is not used anywhere; consider renaming it to something clearer (e.g. FrameworkVersionError) and wiring it into the version compatibility logic, or remove it if it’s not needed.
- The auth/rbac API wiring appears inconsistent: api/__init__.py imports get_auth_backend from both .auth and .rbac and exports it multiple times, and auth.py now imports Any from typing_extensions instead of typing (typing_extensions.Any is not standard); clean up these imports/exports to avoid name collisions and use typing.Any for the TypedDict annotation.
## Individual Comments
### Comment 1
<location path="xcore/kernel/runtime/loader.py" line_range="225-233" />
<code_context>
+ raise FileNotFoundError(
+ f"Dossier plugin introuvable : {plugin_dir}")
manifest = self._validator.load_and_validate(plugin_dir)
for dep in manifest.requires:
dep_name = dep.name if hasattr(dep, "name") else str(dep)
if dep_name not in self._handlers:
- logger.info(f"[{plugin_name}] Dépendance '{dep_name}' → chargement...")
+ logger.info(
+ f"[{plugin_name}] Dépendance '{dep_name}' → chargement...")
await self.load(dep_name)
await self._activate(manifest)
</code_context>
<issue_to_address>
**issue (bug_risk):** load() still assumes load_and_validate returns a manifest, but it now returns a tuple in load_all)
`load_all` now unpacks `(manifest, validate_version, frameversion)` from `load_and_validate`, but `load()` still assigns the whole return value to `manifest` and then accesses `manifest.requires`. If `load_and_validate` always returns a tuple, this will fail at runtime. Please either unpack the tuple in `load()` as well, or revert `load_and_validate` to returning just a manifest and handle version checks separately so both call sites agree on the return type.
</issue_to_address>
### Comment 2
<location path="xcore/kernel/api/auth.py" line_range="6-13" />
<code_context>
from typing import List, NotRequired, Protocol, TypedDict, runtime_checkable
+from typing_extensions import Any
+
class AuthPayload(TypedDict):
sub: str
roles: NotRequired[List[str]]
permissions: NotRequired[List[str]]
+ user: NotRequired[dict[str, Any]]
</code_context>
<issue_to_address>
**issue (bug_risk):** Using Any from typing_extensions will fail; use typing.Any instead
Importing `Any` from `typing_extensions` will raise an `ImportError` and prevent this module from loading. Please import it from `typing` instead, e.g. `from typing import Any, List, NotRequired, Protocol, TypedDict, runtime_checkable`.
</issue_to_address>
### Comment 3
<location path="xcore/kernel/security/validation.py" line_range="48-45" />
<code_context>
pass
+class FrameworkVersionVersion(Exception):
+ pass
+
+
</code_context>
<issue_to_address>
**nitpick (typo):** The exception name FrameworkVersionVersion looks like a typo or misnaming
The name `FrameworkVersionVersion` looks duplicated and could be confusing. If this is for framework-version mismatches, consider renaming to something like `FrameworkVersionError` or `FrameworkVersionMismatch`, or another name that clearly reflects the condition it represents.
Suggested implementation:
```python
class FrameworkVersionError(Exception):
"""Raised when there is a framework version mismatch or invalid framework version."""
pass
```
1. Search the codebase for all usages of `FrameworkVersionVersion` and replace them with `FrameworkVersionError`.
2. If this exception is part of the public API (imported/exported elsewhere), update any re-exports or documentation references to the new name.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
225
to
233
| manifest = self._validator.load_and_validate(plugin_dir) | ||
| for dep in manifest.requires: | ||
| dep_name = dep.name if hasattr(dep, "name") else str(dep) | ||
| if dep_name not in self._handlers: | ||
| logger.info(f"[{plugin_name}] Dépendance '{dep_name}' → chargement...") | ||
| logger.info( | ||
| f"[{plugin_name}] Dépendance '{dep_name}' → chargement...") | ||
| await self.load(dep_name) | ||
|
|
||
| await self._activate(manifest) |
Contributor
There was a problem hiding this comment.
issue (bug_risk): load() still assumes load_and_validate returns a manifest, but it now returns a tuple in load_all)
load_all now unpacks (manifest, validate_version, frameversion) from load_and_validate, but load() still assigns the whole return value to manifest and then accesses manifest.requires. If load_and_validate always returns a tuple, this will fail at runtime. Please either unpack the tuple in load() as well, or revert load_and_validate to returning just a manifest and handle version checks separately so both call sites agree on the return type.
Comment on lines
+6
to
+13
| from typing_extensions import Any | ||
|
|
||
|
|
||
| class AuthPayload(TypedDict): | ||
| sub: str | ||
| roles: NotRequired[List[str]] | ||
| permissions: NotRequired[List[str]] | ||
| user: NotRequired[dict[str, Any]] |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Using Any from typing_extensions will fail; use typing.Any instead
Importing Any from typing_extensions will raise an ImportError and prevent this module from loading. Please import it from typing instead, e.g. from typing import Any, List, NotRequired, Protocol, TypedDict, runtime_checkable.
| @@ -45,6 +45,10 @@ class ManifestError(Exception): | |||
| pass | |||
Contributor
There was a problem hiding this comment.
nitpick (typo): The exception name FrameworkVersionVersion looks like a typo or misnaming
The name FrameworkVersionVersion looks duplicated and could be confusing. If this is for framework-version mismatches, consider renaming to something like FrameworkVersionError or FrameworkVersionMismatch, or another name that clearly reflects the condition it represents.
Suggested implementation:
class FrameworkVersionError(Exception):
"""Raised when there is a framework version mismatch or invalid framework version."""
pass- Search the codebase for all usages of
FrameworkVersionVersionand replace them withFrameworkVersionError. - If this exception is part of the public API (imported/exported elsewhere), update any re-exports or documentation references to the new name.
- Fix 13 failing tests (security, configuration, lifecycle) - Integrate pytest-benchmark and create kernel benchmarks - Add automated 'make test' and 'lint-fix' to pre-commit hooks - Update documentation (README.md, GEMINI.md) and version to 2.1.1 - Apply project-wide formatting via make lint-fix
Contributor
|
|
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
2 participants
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 by Sourcery
Introduce v2 plugin structure and health validation while tightening sandbox security and CI linting.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Tests:
Chores: