Add NeuroRift package scaffold, runtime & model checks, secure command runner, CLI and skill system#39
Open
Neuro-Rift wants to merge 2 commits into
Open
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughThis pull request establishes the foundational architecture for NeuroRift, a Python-based autonomous agent framework. It introduces a modular package structure with subsystems for session management, memory, skill execution, channel communications, and security enforcement, alongside new CLI tooling for environment validation and model capability checks. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
codeant-ai
Bot
added
the
size:XXL
Sequence DiagramThis PR introduces a new global CLI wrapper that routes commands into NeuroRift main flow. It adds two core paths: skill management actions and a guarded run agent startup that requires runtime and model readiness checks before autonomous execution. sequenceDiagram
participant User
participant CLI
participant MainApp
participant SkillManager
participant RuntimeCheck
participant ModelCheck
User->>CLI: Run neurorift command
CLI->>MainApp: Forward parsed arguments
alt Skill operation command
MainApp->>SkillManager: Install list run or uninstall skill
SkillManager-->>MainApp: Skill operation result
MainApp-->>User: Return skill output
else Run agent command
MainApp->>RuntimeCheck: Verify required tools and workdir
RuntimeCheck-->>MainApp: Runtime ready
MainApp->>ModelCheck: Verify selected model capability
ModelCheck-->>MainApp: Agent ready
MainApp-->>User: Start orchestrated agent runtime
end
Generated by CodeAnt AI |
![corgea[bot]](https://avatars.githubusercontent.com/in/673913?s=60&v=4){kind=small}
| pkg=self.client.fetch_skill(skill_name, self.examples) | ||
| return self.installer.install(pkg) | ||
| def list(self): return self.installer.registry.list() | ||
| def run(self, skill_name: str, **kwargs): |
There was a problem hiding this comment.
Code Injection (🔒 Security, 🔴 Critical) - The code builds a path using user input "skill_name" without checks, risking unintended behavior when special characters are used. View in Corgea ↗
More Details
🎟️Issue Explanation: The code builds a path using user input "skill_name" without checks, risking unintended behavior when special characters are used.
- "skill_name" is used directly to form a filepath: "self.installer.installed / skill_name" which can be manipulated.
- An attacker could input "../" sequences, causing directory traversal to access files outside expected paths.
- This can let unauthorized code run or leak sensitive info, as the path controls which code "self.loader.run()" executes.
🪄Fix Explanation: The fix validates the skill name against an allowlist and ensures no path traversal by resolving paths and checking directory containment, preventing arbitrary code execution via crafted inputs.
- Retrieves the canonical installed directory path using "installed_dir = self.installer.installed.resolve()" to avoid symbolic link tricks.
- Fetches a set of valid skill names from "self.installer.registry.list()" to enforce an allowlist, raising if the skill is unknown.
- Resolves the full skill path with "skill_path = (installed_dir / skill_name).resolve()" to normalize and expand any path components.
- Checks that the resolved skill path is a child of the installed directory with "if installed_dir not in skill_path.parents", detecting and blocking path traversal attempts.
- Only after validation, it calls "self.loader.run(skill_path, **kwargs)", ensuring safe and intended skill execution.
💡Important Instructions: Ensure that the registry listing is always accurate and reflects current installed skills to avoid blocking valid inputs or allowing revoked skills.
Suggested change
| def run(self, skill_name: str, **kwargs): | |
| def run(self, skill_name: str, **kwargs): | |
| # Validate skill name using allowlist of installed skills and prevent path traversal | |
| installed_dir = self.installer.installed.resolve() | |
| try: | |
| installed_names = set(self.installer.registry.list()) | |
| except Exception: | |
| installed_names = None | |
| if installed_names is not None and skill_name not in installed_names: | |
| raise ValueError(f"Unknown or uninstalled skill: {skill_name}") | |
| skill_path = (installed_dir / skill_name).resolve() | |
| if installed_dir not in skill_path.parents: | |
| raise ValueError("Invalid skill name or path traversal detected") | |
| return self.loader.run(skill_path, **kwargs) |
| return self.installer.install(pkg) | ||
| def list(self): return self.installer.registry.list() | ||
| def run(self, skill_name: str, **kwargs): | ||
| return self.loader.run(self.installer.installed / skill_name, **kwargs) |
There was a problem hiding this comment.
Path Traversal (🔒 Security, 🔴 High) - The code may allow attackers to trick the program into accessing files outside a safe folder by manipulating file paths using the "skill_name" input. View in Corgea ↗
More Details
🎟️Issue Explanation: The code may allow attackers to trick the program into accessing files outside a safe folder by manipulating file paths using the "skill_name" input.
- The input "skill_name" is used to build file paths without cleaning special parts like "../" that go up folders.
- Attackers can input something like "../secret" to break out of the restricted folder and uninstall or access unintended files.
- Since the method handles skill uninstallation, improper path handling risks affecting files outside safe skill directories, which is sensitive.
🪄Fix Explanation: The fix validates the skill_name input to prevent path traversal by rejecting absolute paths, parent directory references, and multi-part paths. It resolves and verifies the target path is within the allowed directory before proceeding.
- The fix converts "skill_name" to a "Path" object and rejects absolute paths or those containing "'..'" components to block path traversal.
- It ensures "skill_name" has only one path part ("len(p.parts) != 1") preventing nested paths.
- The base installation directory is resolved with "self.installer.installed.resolve()" and the target path is resolved similarly.
- The code checks if "target.relative_to(base)" succeeds to confirm the resolved target is within the allowed installation directory.
- If any validation fails, it raises "ValueError("Invalid skill_name")", stopping unsafe path usage in "run" and "uninstall".
💡Important Instructions: Ensure all other usage of skill_name for file access is similarly protected or constrained; no further immediate changes needed in this code snippet.
Suggested change
| return self.loader.run(self.installer.installed / skill_name, **kwargs) | |
| p = Path(skill_name) | |
| if p.is_absolute() or any(part == '..' for part in p.parts) or len(p.parts) != 1: | |
| raise ValueError("Invalid skill_name") | |
| base = self.installer.installed.resolve() | |
| target = (base / p.name).resolve() | |
| try: | |
| target.relative_to(base) | |
| except ValueError: | |
| raise ValueError("Invalid skill_name") | |
| return self.loader.run(target, **kwargs) | |
| def uninstall(self, skill_name: str): | |
| p = Path(skill_name) | |
| if p.is_absolute() or any(part == '..' for part in p.parts) or len(p.parts) != 1: | |
| raise ValueError("Invalid skill_name") | |
| base = self.installer.installed.resolve() | |
| target = (base / p.name).resolve() | |
| try: | |
| target.relative_to(base) | |
| except ValueError: | |
| raise ValueError("Invalid skill_name") | |
| return self.installer.uninstall(p.name) |
| class SkillLoader: | ||
| def run(self, skill_dir: Path, **kwargs): | ||
| meta=json.loads((skill_dir/'skill.json').read_text(encoding='utf-8')) | ||
| entry=skill_dir/meta['entrypoint'] |
There was a problem hiding this comment.
Code Injection (🔒 Security, 🔴 Critical) - The code dynamically loads and runs a module based on external input without safe checks, risking execution of harmful code from untrusted sources. View in Corgea ↗
More Details
🎟️Issue Explanation: The code dynamically loads and runs a module based on external input without safe checks, risking execution of harmful code from untrusted sources.
- "entry=skill_dir/meta['entrypoint']" uses external input to locate code, which attackers can manipulate to load malicious modules.
- "spec.loader.exec_module(mod)" runs the loaded module's code, allowing injected code to execute and compromise the system.
- If "meta['entrypoint']" points to harmful code, this bypasses safeguards and runs unauthorized actions or data leaks.
🪄Fix Explanation: The fix validates the 'entrypoint' from skill metadata to ensure it contains only safe characters and forbids path traversal or special symbols, preventing arbitrary code execution by restricting dynamic imports to known module names.
- The entrypoint string is sanitized by checking for emptiness, unwanted characters like "'.'", "'/'", "'\\'", and ensuring it contains only alphanumeric characters and underscores via "entry_name.replace('_','').isalnum()".
- Instead of importing from a file path, the code now builds a trusted module name string "neurorift.skills.{entry_name}" and uses "importlib.util.find_spec()" to locate it, avoiding risky path-based imports.
- The fix validates the module spec existence and loader presence, returning an error if the module is unknown, thus preventing attempts to load unauthorized code.
- Executing the module and calling "run()" proceeds only after these stringent checks, stopping injection via manipulated entrypoints.
💡Important Instructions: Ensure the skill modules are correctly installed or discoverable under the
neurorift.skills namespace package for find_spec() to work properly.
Suggested change
| entry=skill_dir/meta['entrypoint'] | |
| entry_name=str(meta.get('entrypoint','')).strip() | |
| if (not entry_name) or ('.' in entry_name) or ('/' in entry_name) or ('\\' in entry_name) or (not entry_name.replace('_','').isalnum()): | |
| return {'error':'invalid entrypoint'} | |
| module_name=f"neurorift.skills.{entry_name}" | |
| spec=importlib.util.find_spec(module_name) | |
| if spec is None or spec.loader is None: | |
| return {'error':'unknown skill'} | |
| mod=importlib.util.module_from_spec(spec); spec.loader.exec_module(mod) |
Contributor
 |
|
Overall Grade Focus Area: Complexity |
Security  Reliability Complexity  Hygiene |
Feedback
- Instance methods used as static utilities
- Many methods never reference instance state and act as free functions, bloating class footprints across files; converting them to module-level functions or explicit @staticmethods, or introducing clear instance state, will simplify interfaces and lower cognitive complexity.
- Fragile path and temp handling
- Path logic relies on ad-hoc checks and partial executable names, and temp files are created insecurely; normalize and resolve paths, use secure tempfile APIs, and invoke executables with fully qualified paths to eliminate these risks.
- Orchestrator touching internals and dead code
- The main runner reaches into client private methods while accumulating unused imports/arguments, signaling blurred boundaries; restrict orchestration to public APIs, remove dead symbols, and encapsulate runner responsibilities to stop internal coupling and name collisions.
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Python |  |
Mar 14, 2026 12:52p.m. | Review ↗ |
Comment on lines
+21
to
+35
| meta = json.loads((pkg / "skill.json").read_text(encoding="utf-8")) | ||
| name = meta["name"] | ||
| dst = self.installed / name | ||
| if dst.exists(): | ||
| shutil.rmtree(dst) | ||
| shutil.copytree(pkg, dst) | ||
| self.registry.add(name) | ||
| return {"success": True, "name": name, "path": str(dst)} | ||
|
|
||
| def uninstall(self, name: str): | ||
| dst = self.installed / name | ||
| if dst.exists(): | ||
| shutil.rmtree(dst) | ||
| self.registry.remove(name) | ||
| return {"success": True, "name": name} |
There was a problem hiding this comment.
Path Traversal (🔒 Security, 🔴 Critical) - The code uses external input to build a directory path but doesn't properly check if the path is inside a safe folder, risking unauthorized access or overwriting files outside it. View in Corgea ↗
More Details
🎟️Issue Explanation: The code uses external input to build a directory path but doesn't properly check if the path is inside a safe folder, risking unauthorized access or overwriting files outside it.
- The code sets "dst = self.installed / name", where "name" comes from external JSON data without validation.
- If "name" contains path tricks like "../", it can escape the "self.installed" folder and overwrite important files.
- Example: "name = "../../etc"" leads "shutil.rmtree(dst)" to delete system files outside the intended safe directory.
🪄Fix Explanation: The fix prevents path traversal by validating skill names against a strict pattern and ensuring resolved paths are within the intended installed directory, stopping attackers from escaping to unauthorized locations.
- Added regex validation "re.fullmatch(r"[A-Za-z0-9._-]+", name)" to allow only safe characters in skill names, preventing malicious input.
- Used "Path.resolve()" on both the installed directory and target path to get absolute paths, preventing traversal via symbolic links or ../ segments.
- Verified that the resolved target path is a subpath of the resolved installed directory using "dst.relative_to(installed_root)" to detect and reject traversal attempts.
- Applied the same validation both when installing and uninstalling to ensure consistent protections on all file operations regarding skill names.
- On validation failure, the code returns clear error messages like ""invalid skill name"" or ""path traversal detected"", safely aborting unsafe operations.
💡Important Instructions: Ensure that any other components calling these install/uninstall methods also handle the error responses correctly and avoid proceeding on failure. No further code changes are required.
diff --git a/neurorift/skills/installer.py b/neurorift/skills/installer.py
index 5899d99..44a5ad3 100644
--- a/neurorift/skills/installer.py
+++ b/neurorift/skills/installer.py
@@ -1,3 +1,4 @@
+import json, shutil, re
import json, shutil
from pathlib import Path
from neurorift.skills.skill_validator import SkillValidator
@@ -19,8 +20,15 @@ class SkillInstaller:
if not ok:
return {"success": False, "error": f"missing:{missing}"}
meta = json.loads((pkg / "skill.json").read_text(encoding="utf-8"))
- name = meta["name"]
- dst = self.installed / name
+ name = str(meta.get("name", "")).strip()
+ if not name or not re.fullmatch(r"[A-Za-z0-9._-]+", name):
+ return {"success": False, "error": "invalid skill name"}
+ installed_root = self.installed.resolve()
+ dst = (installed_root / name).resolve()
+ try:
+ dst.relative_to(installed_root)
+ except Exception:
+ return {"success": False, "error": "path traversal detected"}
if dst.exists():
shutil.rmtree(dst)
shutil.copytree(pkg, dst)
@@ -28,7 +36,14 @@ class SkillInstaller:
return {"success": True, "name": name, "path": str(dst)}
def uninstall(self, name: str):
- dst = self.installed / name
+ if not name or not isinstance(name, str) or not re.fullmatch(r"[A-Za-z0-9._-]+", name):
+ return {"success": False, "error": "invalid skill name"}
+ installed_root = self.installed.resolve()
+ dst = (installed_root / name).resolve()
+ try:
+ dst.relative_to(installed_root)
+ except Exception:
+ return {"success": False, "error": "path traversal detected"}
if dst.exists():
shutil.rmtree(dst)
self.registry.remove(name)
To apply the fix, Download .patch.
| return {"success": True, "name": name, "path": str(dst)} | ||
|
|
||
| def uninstall(self, name: str): | ||
| dst = self.installed / name |
There was a problem hiding this comment.
Path Traversal (🔒 Security, 🔴 Critical) - The code lets users delete files by name but doesn't stop them from using special path parts to delete files outside the allowed folder, risking unwanted file removal. View in Corgea ↗
More Details
🎟️Issue Explanation: The code lets users delete files by name but doesn't stop them from using special path parts to delete files outside the allowed folder, risking unwanted file removal.
- "dst = self.installed / name" builds a path from user input "name" without sanitizing it.
- Special elements like "../" in "name" let attackers delete files outside the restricted directory.
- An attacker can pass "../../important_folder" as "name" to remove sensitive files outside "self.installed".
🪄Fix Explanation: The fix prevents directory traversal by validating and resolving the input path before use, ensuring the target directory is always within the allowed installation folder.
- Uses "Path(name).is_absolute()" and checks for forbidden segments ("..", "", ".") to reject absolute paths and path traversal attempts.
- Resolves the base directory with "self.installed.resolve()" to get a canonical path for comparison.
- Combines and resolves the target path "dst = (base_dir / name).resolve()" to prevent symlink or traversal exploits.
- Verifies that "dst" is a subdirectory of "base_dir" using "dst.relative_to(base_dir)" to ensure containment.
- Only removes "dst" if it passes validation and exists, avoiding accidental deletion outside the intended folder.
💡Important Instructions: Ensure
self.installed is correctly set and absolute before this method is called to maintain consistent base directory resolution.
Suggested change
| dst = self.installed / name | |
| base_dir = self.installed.resolve() | |
| # Reject absolute paths and traversal sequences in 'name' | |
| if Path(name).is_absolute() or any(part in ("..", "", ".") for part in Path(name).parts): | |
| return {"success": False, "error": "invalid name"} | |
| dst = (base_dir / name).resolve() | |
| try: | |
| dst.relative_to(base_dir) | |
| except ValueError: | |
| return {"success": False, "error": "invalid name"} | |
| if dst.exists(): | |
| shutil.rmtree(dst) |
Comment on lines
+12
to
+23
| return self.root / f"{sid}.json" | ||
|
|
||
| def save(self, session: SessionContext): | ||
| self.path(session.session_id).write_text( | ||
| json.dumps(session.__dict__, default=str), encoding="utf-8" | ||
| ) | ||
|
|
||
| def load(self, sid: str): | ||
| p = self.path(sid) | ||
| return ( | ||
| SessionContext(**json.loads(p.read_text(encoding="utf-8"))) | ||
| if p.exists() |
There was a problem hiding this comment.
Path Traversal (🔒 Security, 🔴 High) - This code builds a file path using external input "sid" without checking it, risking access outside the allowed folder, which can expose or modify unintended files. View in Corgea ↗
More Details
🎟️Issue Explanation: This code builds a file path using external input "sid" without checking it, risking access outside the allowed folder, which can expose or modify unintended files.
- The code uses "self.root / f"{sid}.json"" to create paths, but "sid" may include "../" to move outside the restricted directory.
- Attackers can supply input like "../../etc/passwd" to read or change files outside the intended folder, leading to data leaks or damage.
- Since the function deals with file paths tied to sensitive data (".json"), unrestricted input can compromise security or system integrity.
🪄Fix Explanation: The fix sanitizes the session ID by rejecting absolute paths, separator-containing names, and traversal tokens, then ensures the resolved file path lies within the intended root directory, effectively preventing path traversal attacks.
The code resolves "self.root" to an absolute path for consistent comparison using "root = self.root.resolve(strict=True)".
It converts the session ID to a "Path" object and rejects invalid IDs that are absolute, contain multiple parts, or equal "." or "..", blocking path traversal attempts.
The candidate file path is constructed with "(root / f"{candidate_name.name}.json")" and resolved without requiring existence.
It verifies the resolved candidate path is within the root directory by confirming "root in candidate.parents", raising an error otherwise.
This combination prevents an attacker from escaping the restricted directory, closing the vulnerability.
💡Important Instructions: Ensure that all session ID inputs at the API boundary are validated before reaching this method to avoid unexpected exceptions.
Suggested change
| return self.root / f"{sid}.json" | |
| def save(self, session: SessionContext): | |
| self.path(session.session_id).write_text( | |
| json.dumps(session.__dict__, default=str), encoding="utf-8" | |
| ) | |
| def load(self, sid: str): | |
| p = self.path(sid) | |
| return ( | |
| SessionContext(**json.loads(p.read_text(encoding="utf-8"))) | |
| if p.exists() | |
| # Sanitize session ID to prevent path traversal and enforce containment within the store root | |
| root = self.root.resolve(strict=True) | |
| candidate_name = Path(sid) | |
| # Reject absolute paths and any path containing separators or traversal components | |
| if candidate_name.is_absolute() or len(candidate_name.parts) != 1 or candidate_name.name in (".", ".."): | |
| raise ValueError("invalid session id") | |
| candidate = (root / f"{candidate_name.name}.json").resolve(strict=False) | |
| # Verify the path is within the root directory after resolution | |
| if root not in candidate.parents: | |
| raise ValueError("invalid session id") | |
| return candidate |
|
|
||
| class SkillLoader: | ||
| def run(self, skill_dir: Path, **kwargs): | ||
| meta = json.loads((skill_dir / "skill.json").read_text(encoding="utf-8")) |
There was a problem hiding this comment.
Code Injection (🔒 Security, 🔴 Critical) - The code loads and runs a Python module based on external input without sanitizing it, risking malicious code execution inside the system. View in Corgea ↗
More Details
🎟️Issue Explanation: The code loads and runs a Python module based on external input without sanitizing it, risking malicious code execution inside the system.
- The "entrypoint" in "meta" is from an external file "skill.json", letting attackers specify any file to run.
- Using "exec_module" runs code in the specified file, so if attacker controls "entrypoint", they can execute arbitrary code.
- This is risky because the loaded module’s "run()" function has full access to the program’s environment and sensitive data.
🪄Fix Explanation: The fix restricts module loading to a fixed filename within the skill directory and validates the path to prevent path traversal or symlink bypass, ensuring only trusted code is executed.
"entry = (skill_dir / "skill.py").resolve()" forces a fixed, safe entrypoint filename to eliminate arbitrary file loading risks.
The code verifies "entry" is inside "skill_dir", is a regular file, and not a symlink to block path traversal and symlink attacks.
Safe module name creation uses sanitized directory name via "re.sub(r"[^0-9A-Za-z_]", "_", skill_dir.name)" to prevent malformed or malicious module names.
Module spec and loader presence are checked explicitly, returning errors for invalid specs to avoid undefined behavior.
Loading errors during "spec.loader.exec_module(mod)" are caught to prevent crashes, safely handling malformed modules or runtime errors at import.
💡Important Instructions: Ensure all skill packages follow the fixed entrypoint convention by containing a "skill.py" with a callable run() function; otherwise, skill loading will fail.
diff --git a/neurorift/skills/loader.py b/neurorift/skills/loader.py
index ddb9a53..0c2b65c 100644
--- a/neurorift/skills/loader.py
+++ b/neurorift/skills/loader.py
@@ -1,3 +1,4 @@
+import importlib.util, json, re
import importlib.util, json
from pathlib import Path
@@ -5,8 +6,28 @@ from pathlib import Path
class SkillLoader:
def run(self, skill_dir: Path, **kwargs):
meta = json.loads((skill_dir / "skill.json").read_text(encoding="utf-8"))
- entry = skill_dir / meta["entrypoint"]
- spec = importlib.util.spec_from_file_location(f"skill_{meta['name']}", entry)
+ # Only allow a fixed, safe entrypoint name to avoid path traversal and arbitrary imports
+ entry = (skill_dir / "skill.py").resolve()
+ base_dir = skill_dir.resolve()
+ if not isinstance(meta, dict):
+ return {"error": "invalid metadata"}
+ # Ensure the resolved entrypoint is within the skill directory and is a regular file (no symlinks)
+ try:
+ if entry.is_symlink() or not entry.is_file() or base_dir not in entry.parents:
+ return {"error": "invalid entrypoint"}
+ except OSError:
+ return {"error": "invalid entrypoint"}
+ # Build a safe module name derived from the directory name, not untrusted metadata
+ safe_mod_name = "skill_" + re.sub(r"[^0-9A-Za-z_]", "_", skill_dir.name)
+ spec = importlib.util.spec_from_file_location(safe_mod_name, str(entry))
+ if spec is None or spec.loader is None:
+ return {"error": "invalid module spec"}
mod = importlib.util.module_from_spec(spec)
- spec.loader.exec_module(mod)
- return mod.run(**kwargs) if hasattr(mod, "run") else {"error": "missing run()"}
+ try:
+ spec.loader.exec_module(mod)
+ except Exception:
+ return {"error": "failed to load skill"}
+ run_fn = getattr(mod, "run", None)
+ if not callable(run_fn):
+ return {"error": "missing run()"}
+ return run_fn(**kwargs)
To apply the fix, Download .patch.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3785f4657
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+252
to
+253
| if not stdout.strip() and not stderr.strip(): | ||
| validation_passed = False |
There was a problem hiding this comment.
The output validator currently marks any command with both stdout and stderr empty as a failure, and _run_command_secure then requires validation_passed for success. This incorrectly fails legitimate commands that intentionally produce no output (for example, go install in install_missing_tools commonly exits 0 silently), causing false negatives and unnecessary retry/error paths even when execution succeeded.
Useful? React with 👍 / 👎.
Comment on lines
+14
to
+16
| meta=json.loads((pkg/'skill.json').read_text(encoding='utf-8')); name=meta['name'] | ||
| dst=self.installed/name | ||
| if dst.exists(): shutil.rmtree(dst) |
There was a problem hiding this comment.
The installer trusts name from skill.json and directly builds self.installed / name without normalization or containment checks. A crafted skill name containing path traversal segments (for example ../../...) can make install/uninstall operations copy to or remove directories outside the skill store, which is especially risky because skills are fetched from an external source path.
Useful? React with 👍 / 👎.
| def build_parser() -> argparse.ArgumentParser: | ||
| parser = argparse.ArgumentParser(description="NeuroRift global CLI launcher") | ||
| parser.add_argument("--install-global-wrapper", action="store_true", help="Install /usr/local/bin/neurorift wrapper") | ||
| parser.add_argument("args", nargs=argparse.REMAINDER, help="Arguments forwarded to neurorift_main.py") |
There was a problem hiding this comment.
The wrapper parser uses a single REMAINDER positional to forward arguments, but argparse still rejects unknown options that appear before that positional. After switching the console entrypoint to this wrapper, invocations like neurorift --target example.com now error with “unrecognized arguments” unless users add a manual --, which breaks existing top-level flag usage.
Useful? React with 👍 / 👎.
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Comment on lines
+28
to
+32
| raw_text = (raw_text or "").strip() | ||
| start = raw_text.find("{") | ||
| end = raw_text.rfind("}") | ||
| if start == -1 or end == -1 or end <= start: | ||
| raise ValueError("No JSON object found in model response") |
There was a problem hiding this comment.
Suggestion: The JSON parser currently slices from the first { to the last } in the whole response, which breaks when the model outputs extra brace-containing text before or after the JSON object. This causes valid capability responses to be rejected as invalid JSON. Parse the first decodable JSON object instead of using greedy brace slicing. [logic error]
Severity Level: Major ⚠️
- ❌ `run-agent` startup blocked by false JSON parse failure.
- ⚠️ Capability check emits misleading `invalid_capability_json` errors.
Suggested change
| raw_text = (raw_text or "").strip() | |
| start = raw_text.find("{") | |
| end = raw_text.rfind("}") | |
| if start == -1 or end == -1 or end <= start: | |
| raise ValueError("No JSON object found in model response") | |
| decoder = json.JSONDecoder() | |
| for i, ch in enumerate(raw_text): | |
| if ch != "{": | |
| continue | |
| try: | |
| obj, _ = decoder.raw_decode(raw_text[i:]) | |
| except json.JSONDecodeError: | |
| continue | |
| if isinstance(obj, dict): | |
| return obj | |
| raise ValueError("No JSON object found in model response") |
Steps of Reproduction ✅
1. Run NeuroRift through the real agent entrypoint `run-agent` (subcommand defined at
`neurorift_main.py:937`), which reaches model checking in `_async_main` at
`neurorift_main.py:1007-1019`.
2. Use a model/runner output that includes extra brace text around valid JSON (the code
path is `verify_model_capabilities()` in `model_capability_check.py:35`, called at
`neurorift_main.py:1018`).
3. `_extract_json()` at `model_capability_check.py:28-32` slices from first `{` to last
`}`, so mixed text with additional braces creates an invalid combined JSON slice.
4. `json.loads(...)` fails, `verify_model_capabilities()` returns
`invalid_capability_json` at `model_capability_check.py:62`, and `run-agent` is blocked by
the readiness gate at `neurorift_main.py:1019-1023`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** model_capability_check.py
**Line:** 28:32
**Comment:**
*Logic Error: The JSON parser currently slices from the first `{` to the last `}` in the whole response, which breaks when the model outputs extra brace-containing text before or after the JSON object. This causes valid capability responses to be rejected as invalid JSON. Parse the first decodable JSON object instead of using greedy brace slicing.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+82
to
+83
| "error": "capability_fields_missing", | ||
| "parsed": parsed, |
There was a problem hiding this comment.
Suggestion: agent_ready is used as a truthy value without enforcing boolean type, so a model returning "false" (string) is treated as ready by callers because non-empty strings are truthy. Normalize agent_ready to a strict boolean before returning. [logic error]
Severity Level: Critical 🚨
- ❌ `run-agent` readiness gate bypassed by string booleans.
- ⚠️ Autonomous mode may run with unsupported models.
Suggested change
| "error": "capability_fields_missing", | |
| "parsed": parsed, | |
| parsed["agent_ready"] = parsed.get("agent_ready") is True | |
| parsed["ok"] = parsed["agent_ready"] | |
| return parsed |
Steps of Reproduction ✅
1. Start from the real gate path: `run-agent` in `_async_main`
(`neurorift_main.py:1007-1023`) calls `verify_model_capabilities()` at
`neurorift_main.py:1018`.
2. Return a capability JSON object where required keys exist but `agent_ready` is a
string, e.g. `"agent_ready": "false"` (parsed in `model_capability_check.py:58`, required
fields validated at `model_capability_check.py:67-80`).
3. Current code sets `parsed["ok"] = bool(parsed.get("agent_ready"))`
(`model_capability_check.py:82`), which converts non-empty `"false"` to `True`.
4. `run-agent` then checks `if not capability.get("agent_ready")`
(`neurorift_main.py:1019`); string `"false"` is truthy, so the failure branch is skipped
and agent runtime proceeds incorrectly.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** model_capability_check.py
**Line:** 82:83
**Comment:**
*Logic Error: `agent_ready` is used as a truthy value without enforcing boolean type, so a model returning `"false"` (string) is treated as ready by callers because non-empty strings are truthy. Normalize `agent_ready` to a strict boolean before returning.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+12
to
+13
| if not src.exists(): | ||
| raise FileNotFoundError(skill_name) |
There was a problem hiding this comment.
Suggestion: The existence check accepts regular files, but shutil.copytree only works on directories; if a file exists with that name, this path raises NotADirectoryError at runtime. Validate that the source is a directory. [possible bug]
Severity Level: Major ⚠️
- ❌ `--clawhub` install crashes on file-like skill names.
- ⚠️ User-facing skill install flow becomes unreliable.
Suggested change
| if not src.exists(): | |
| raise FileNotFoundError(skill_name) | |
| if not src.is_dir(): | |
| raise FileNotFoundError(skill_name) |
Steps of Reproduction ✅
1. Execute `python3 /workspace/NeuroRift/neurorift_main.py --clawhub __init__.py`;
`--clawhub` input is accepted by parser at `neurorift_main.py:914` and routed to
`install_clawhub()` at `neurorift_main.py:982-983`.
2. `SkillManager.install_clawhub()` forwards `__init__.py` directly to `fetch_skill()` at
`neurorift/skills/skill_manager.py:13-14`.
3. In `fetch_skill()`, existence-only check passes (`src.exists()`) at
`neurorift/clawhub/clawhub_client.py:7` because
`/workspace/NeuroRift/neurorift/skill_store/installed/__init__.py` is a real file
(verified in `neurorift/skill_store/installed` listing).
4. `shutil.copytree(src, dst)` at `neurorift/clawhub/clawhub_client.py:10` then raises
`NotADirectoryError`, aborting the install path.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/clawhub/clawhub_client.py
**Line:** 12:13
**Comment:**
*Possible Bug: The existence check accepts regular files, but `shutil.copytree` only works on directories; if a file exists with that name, this path raises `NotADirectoryError` at runtime. Validate that the source is a directory.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.|
|
||
| class CommandRunner: | ||
| def run(self, cmd: list[str]): | ||
| p = subprocess.run(cmd, capture_output=True, text=True, check=False) |
There was a problem hiding this comment.
Suggestion: If the executable in the command does not exist, subprocess.run raises FileNotFoundError and the method crashes. Catch this exception and return a normal error payload so callers can handle missing tools safely. [possible bug]
Severity Level: Major ⚠️
- ⚠️ Future execution APIs crash on missing binaries.
- ⚠️ Error handling contract breaks for CommandRunner callers.
Suggested change
| p = subprocess.run(cmd, capture_output=True, text=True, check=False) | |
| try: | |
| p = subprocess.run(cmd, capture_output=True, text=True, check=False) | |
| except FileNotFoundError as exc: | |
| return { | |
| "success": False, | |
| "stdout": "", | |
| "stderr": str(exc), | |
| "exit_code": 127, | |
| } |
Steps of Reproduction ✅
1. Confirm this runner is currently scaffolded but callable:
`neurorift/execution/command_runner.py:2-5`; no internal caller found except subclass
declaration `neurorift/execution/sandbox_runner.py:1-3`.
2. Execute `from neurorift.execution.command_runner import CommandRunner;
CommandRunner().run(["definitely_not_installed_bin"])`.
3. Call reaches `subprocess.run(...)` at `command_runner.py:4` with nonexistent
executable.
4. Observe uncaught `FileNotFoundError`, so no `{success, stderr, exit_code}` response is
returned.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/execution/command_runner.py
**Line:** 6:6
**Comment:**
*Possible Bug: If the executable in the command does not exist, `subprocess.run` raises `FileNotFoundError` and the method crashes. Catch this exception and return a normal error payload so callers can handle missing tools safely.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+5
to
+9
| def __init__(self, limit: int = 30): | ||
| self.buffers = defaultdict(lambda: deque(maxlen=limit)) | ||
|
|
||
| def add(self, sid: str, msg: str): | ||
| self.buffers[sid].append(msg) |
There was a problem hiding this comment.
Suggestion: buffers is bounded per session but unbounded in number of sessions, so repeatedly adding messages for new sid values grows memory forever and can eventually exhaust process memory. Add a cap on tracked sessions and evict the oldest session buffer when that cap is reached. [resource leak]
Severity Level: Major ⚠️
- ⚠️ Long-running agents accumulate per-session buffers indefinitely.
- ⚠️ neurorift memory module risks process RAM growth.
Suggested change
| def __init__(self, limit: int = 30): | |
| self.buffers = defaultdict(lambda: deque(maxlen=limit)) | |
| def add(self, sid: str, msg: str): | |
| self.buffers[sid].append(msg) | |
| def __init__(self, limit: int = 30, max_sessions: int = 1000): | |
| self.buffers = defaultdict(lambda: deque(maxlen=limit)) | |
| self.max_sessions = max_sessions | |
| self._session_order = deque() | |
| def add(self, sid: str, msg: str): | |
| if sid not in self.buffers: | |
| if len(self._session_order) >= self.max_sessions: | |
| oldest_sid = self._session_order.popleft() | |
| self.buffers.pop(oldest_sid, None) | |
| self._session_order.append(sid) | |
| self.buffers[sid].append(msg) |
Steps of Reproduction ✅
1. Verify exposure: `setup.py:6` packages `neurorift.*`, so
`neurorift.memory.short_term_memory.ShortTermMemory` is importable API.
2. Verify current implementation: `neurorift/memory/short_term_memory.py:3-4` stores
messages in `self.buffers[sid]` with no session-count guard.
3. Use realistic session IDs from `neurorift/sessions/session_manager.py:10-12`
(`uuid4`-style `sid`) and repeatedly call `ShortTermMemory.add(sid, msg)` with new IDs.
4. Observe unbounded key growth (`len(memory.buffers)` keeps increasing) because each new
`sid` creates a new deque and no eviction path exists.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/memory/short_term_memory.py
**Line:** 5:9
**Comment:**
*Resource Leak: `buffers` is bounded per session but unbounded in number of sessions, so repeatedly adding messages for new `sid` values grows memory forever and can eventually exhaust process memory. Add a cap on tracked sessions and evict the oldest session buffer when that cap is reached.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+24
to
+25
| def run(self, skill_name: str, **kwargs): | ||
| return self.loader.run(self.installer.installed / skill_name, **kwargs) |
There was a problem hiding this comment.
Suggestion: The skill name is joined directly into a filesystem path and executed, so values like absolute paths or .. segments can escape the installed-skill directory and load arbitrary Python code. Resolve and validate the path remains under the installed directory before executing. [security]
Severity Level: Critical 🚨
- ❌ `--skill run` executes arbitrary local Python code.
- ❌ Installed-skill directory boundary is bypassed.
Suggested change
| def run(self, skill_name: str, **kwargs): | |
| return self.loader.run(self.installer.installed / skill_name, **kwargs) | |
| def run(self, skill_name: str, **kwargs): | |
| base = self.installer.installed.resolve() | |
| skill_path = (base / skill_name).resolve() | |
| if base not in skill_path.parents: | |
| return {"success": False, "error": "invalid_skill_name"} | |
| return self.loader.run(skill_path, **kwargs) |
Steps of Reproduction ✅
1. Create an external directory with executable skill metadata, e.g.
`/tmp/evil_skill/skill.json` and `/tmp/evil_skill/skill.py`.
2. Execute `python3 /workspace/NeuroRift/neurorift_main.py --skill run /tmp/evil_skill`;
parser accepts this through `--skill` at `neurorift_main.py:915`.
3. `_async_main` forwards unvalidated `skill_name` to `skill_manager.run(...)` at
`neurorift_main.py:996-998`, and `SkillManager.run` directly builds
`self.installer.installed / skill_name` at `neurorift/skills/skill_manager.py:17-18`.
4. `SkillLoader.run` then reads `/tmp/evil_skill/skill.json` and executes module code via
`spec.loader.exec_module(mod)` at `neurorift/skills/loader.py:5-9`, enabling code
execution outside installed-skill sandbox.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/skills/skill_manager.py
**Line:** 24:25
**Comment:**
*Security: The skill name is joined directly into a filesystem path and executed, so values like absolute paths or `..` segments can escape the installed-skill directory and load arbitrary Python code. Resolve and validate the path remains under the installed directory before executing.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+27
to
+28
| def uninstall(self, skill_name: str): | ||
| return self.installer.uninstall(skill_name) |
There was a problem hiding this comment.
Suggestion: Uninstall forwards user-controlled names directly, and downstream code can recursively delete the resolved path. Without validating that the target stays inside the installed-skill directory, crafted names can trigger deletion outside the intended sandbox. [security]
Severity Level: Critical 🚨
- ❌ `--skill uninstall` may delete arbitrary directories.
- ⚠️ Skill registry may drift from actual installs.
Suggested change
| def uninstall(self, skill_name: str): | |
| return self.installer.uninstall(skill_name) | |
| def uninstall(self, skill_name: str): | |
| base = self.installer.installed.resolve() | |
| skill_path = (base / skill_name).resolve() | |
| if base not in skill_path.parents: | |
| return {"success": False, "error": "invalid_skill_name"} | |
| return self.installer.uninstall(skill_path.name) |
Steps of Reproduction ✅
1. Prepare a non-skill directory with contents, e.g. `/tmp/delete_me/test.txt`.
2. Run `python3 /workspace/NeuroRift/neurorift_main.py --skill uninstall /tmp/delete_me`;
CLI accepts raw name via `--skill` (`neurorift_main.py:915`), then calls
`skill_manager.uninstall(skill_name)` at `neurorift_main.py:1001-1002`.
3. `SkillManager.uninstall` forwards input unchanged at
`neurorift/skills/skill_manager.py:19`.
4. In `SkillInstaller.uninstall`, `dst = self.installed / name` at
`neurorift/skills/installer.py:21`; absolute `name` becomes `/tmp/delete_me`, and
`shutil.rmtree(dst)` at line 22 recursively deletes that external directory.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/skills/skill_manager.py
**Line:** 27:28
**Comment:**
*Security: Uninstall forwards user-controlled names directly, and downstream code can recursively delete the resolved path. Without validating that the target stays inside the installed-skill directory, crafted names can trigger deletion outside the intended sandbox.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.|
|
||
|
|
||
| class SkillValidator: | ||
| required = ["skill.json", "skill.py", "README.md"] |
There was a problem hiding this comment.
Suggestion: The validator hardcodes skill.py as mandatory, but the loader executes the file declared in skill.json (entrypoint). This will incorrectly reject valid skills that use a different entrypoint filename. Remove the hardcoded skill.py requirement so validation aligns with the loader contract. [logic error]
Severity Level: Major ⚠️
- ⚠️ `--clawhub` rejects valid custom-entrypoint skills.
- ❌ Skill installation blocked despite valid `entrypoint` metadata.
Suggested change
| required = ["skill.json", "skill.py", "README.md"] | |
| required = ["skill.json", "README.md"] |
Steps of Reproduction ✅
1. Use the CLI entrypoint (`neurorift_cli.py:50-52`) to run `neurorift_main.main()`, then
invoke skill install flow with `--clawhub <name>`; `_async_main` handles this at
`neurorift_main.py:982-989`.
2. This calls `SkillManager.install_clawhub()`
(`neurorift/skills/skill_manager.py:13-15`), which fetches a package and passes it to
`SkillInstaller.install()`.
3. `SkillInstaller.install()` calls validator (`neurorift/skills/installer.py:12-13`);
`SkillValidator.required` currently enforces `skill.py`
(`neurorift/skills/skill_validator.py:3`), so any package whose `skill.json` uses another
filename is rejected as `missing:['skill.py']`.
4. Rejection conflicts with runtime loader contract: loader executes `meta['entrypoint']`
from `skill.json` (`neurorift/skills/loader.py:5-7`), so valid non-`skill.py` packages are
blocked before install.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/skills/skill_validator.py
**Line:** 5:5
**Comment:**
*Logic Error: The validator hardcodes `skill.py` as mandatory, but the loader executes the file declared in `skill.json` (`entrypoint`). This will incorrectly reject valid skills that use a different entrypoint filename. Remove the hardcoded `skill.py` requirement so validation aligns with the loader contract.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| required = ["skill.json", "skill.py", "README.md"] | ||
|
|
||
| def validate(self, path: Path): | ||
| missing = [f for f in self.required if not (path / f).exists()] |
There was a problem hiding this comment.
Suggestion: Using .exists() allows directories named like required files to pass validation; later code reads skill.json as a file, which will crash at runtime if it is actually a directory. Validate required artifacts as files with .is_file() to prevent this failure. [possible bug]
Severity Level: Major ⚠️
- ❌ `--clawhub` install crashes on malformed skill packages.
- ⚠️ Invalid package shape bypasses validator checks.
Suggested change
| missing = [f for f in self.required if not (path / f).exists()] | |
| missing = [f for f in self.required if not (path / f).is_file()] |
Steps of Reproduction ✅
1. Trigger install path via CLI `--clawhub` (`neurorift_main.py:982-989`), which routes to
`SkillManager.install_clawhub()` (`neurorift/skills/skill_manager.py:13-15`) and then
`SkillInstaller.install()`.
2. Provide a malformed skill package where required names exist as directories (for
example `skill.json/`), under the source directory used by fetch
(`neurorift/skills/skill_manager.py:12`, copied by `clawhub_client.py:5-10`).
3. Validator passes it because `.exists()` is true
(`neurorift/skills/skill_validator.py:5`), so installer continues.
4. Installer then reads `(pkg/'skill.json').read_text(...)`
(`neurorift/skills/installer.py:14`), which raises `IsADirectoryError`; no local catch
exists in skill install path, so command fails instead of returning structured validation
error.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/skills/skill_validator.py
**Line:** 8:8
**Comment:**
*Possible Bug: Using `.exists()` allows directories named like required files to pass validation; later code reads `skill.json` as a file, which will crash at runtime if it is actually a directory. Validate required artifacts as files with `.is_file()` to prevent this failure.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+17
to
+18
| main_path = (Path(__file__).resolve().parent / "neurorift_main.py").resolve() | ||
| script = f'#!/usr/bin/env bash\npython3 {main_path} "$@"\n' |
There was a problem hiding this comment.
Suggestion: The generated shell wrapper embeds the script path unquoted, so a checkout path containing spaces or shell metacharacters can break execution or trigger shell injection when the wrapper runs. Quote the path before writing it into the bash script. [security]
Severity Level: Major ⚠️
- ⚠️ Global wrapper can mis-execute from spaced install paths.
- ❌ Shell metacharacters may alter executed command unexpectedly.
Suggested change
| main_path = (Path(__file__).resolve().parent / "neurorift_main.py").resolve() | |
| script = f'#!/usr/bin/env bash\npython3 {main_path} "$@"\n' | |
| main_path = (Path(__file__).resolve().parent / "neurorift_main.py").resolve() | |
| import shlex | |
| script = f'#!/usr/bin/env bash\npython3 {shlex.quote(str(main_path))} "$@"\n' |
Steps of Reproduction ✅
1. Install and invoke the console entrypoint `neurorift` (wired in `setup.py:50-53` to
`neurorift_cli:main`).
2. Run `neurorift --install-global-wrapper`; `main()` in `neurorift_cli.py:50-56` calls
`install_global_wrapper()` (`neurorift_cli.py:14-29`).
3. In `install_global_wrapper()`, wrapper content is built at `neurorift_cli.py:16-18` as
`python3 {main_path} "$@"` without shell quoting.
4. If the installation path contains shell-breaking characters (e.g., space/`;`),
executing `/usr/local/bin/neurorift` causes shell tokenization/injection behavior,
producing wrong command execution or failure.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift_cli.py
**Line:** 17:18
**Comment:**
*Security: The generated shell wrapper embeds the script path unquoted, so a checkout path containing spaces or shell metacharacters can break execution or trigger shell injection when the wrapper runs. Quote the path before writing it into the bash script.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (17)
neurorift/gateway/api_server.py-2-3 (1)
2-3:⚠️ Potential issue | 🟠 Major
APIServer.start()currently returns a false success state.Line [2]-Line [3] always return
True, even though no server startup occurs. This can cause callers to proceed as if the API gateway is live when it is not.Proposed fail-fast scaffold
class APIServer: def start(self): - return True + raise NotImplementedError("APIServer.start() is not implemented yet")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/gateway/api_server.py` around lines 2 - 3, APIServer.start currently always returns True without starting anything; update APIServer.start to perform real startup steps (initialize and bind the server socket or framework instance, start the event loop/thread, and verify the server is listening/responding) and return True only on success or False/raise on failure; call/log errors via the existing logger and ensure any created resources are cleaned up on failure so callers don't proceed with a false-positive success; locate and modify the APIServer.start method to implement these checks and status returns.neurorift/gateway/websocket_gateway.py-2-3 (1)
2-3:⚠️ Potential issue | 🟠 Major
WebSocketGateway.start()also returns unconditional success.Line [2]-Line [3] report a successful start without initializing any websocket listener. This can hide runtime failures and break gateway lifecycle assumptions.
Proposed fail-fast scaffold
class WebSocketGateway: def start(self): - return True + raise NotImplementedError("WebSocketGateway.start() is not implemented yet")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/gateway/websocket_gateway.py` around lines 2 - 3, WebSocketGateway.start currently returns True unconditionally without initializing any listener; change it to actually initialize the gateway (e.g., bind/create the websocket server or listener), set a running/state flag (like self._server or self._running), wrap the initialization in a try/except to catch and log errors via the module/class logger, and return True only on successful startup and False (or re-raise) on failure so callers can detect startup errors; update references to start/stop lifecycle to rely on the new state flag (methods: WebSocketGateway.start, any self._server/self._running usage).neurorift/execution/command_runner.py-5-12 (1)
5-12:⚠️ Potential issue | 🟠 MajorMissing timeout - commands can hang indefinitely.
For an autonomous agent execution module,
subprocess.runwithout atimeoutparameter is a reliability risk. A misbehaving or malicious command could block the agent forever.Additionally, given the PR objectives mention "hardened command runner with validation, sandboxing, resource limits," consider integrating
ResourceLimitshere.🛡️ Proposed fix to add timeout
+from neurorift.execution.resource_limits import ResourceLimits + + class CommandRunner: - def run(self, cmd: list[str]): - p = subprocess.run(cmd, capture_output=True, text=True, check=False) + def __init__(self, limits: ResourceLimits | None = None): + self.limits = limits or ResourceLimits() + + def run(self, cmd: list[str]) -> dict: + try: + p = subprocess.run( + cmd, + capture_output=True, + text=True, + check=False, + timeout=self.limits.timeout_seconds, + ) + except subprocess.TimeoutExpired as e: + return { + "success": False, + "stdout": e.stdout or "", + "stderr": e.stderr or "", + "exit_code": -1, + "error": f"Command timed out after {self.limits.timeout_seconds}s", + } return { "success": p.returncode == 0, "stdout": p.stdout, "stderr": p.stderr, "exit_code": p.returncode, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/execution/command_runner.py` around lines 5 - 12, The run function currently calls subprocess.run without a timeout; update neurorift.execution.command_runner.CommandRunner.run to pass a sensible timeout (configurable via an argument or attribute) and wrap the call in a try/except to catch subprocess.TimeoutExpired, returning success: False, an informative stderr message (including timeout value and command), and a non-zero exit_code; also invoke the ResourceLimits routine (e.g., ResourceLimits.apply() or appropriate static method) before launching the subprocess to enforce CPU/memory/sandbox limits so the runner is hardened against hangs and resource abuse.neurorift/memory/vector_memory.py-2-6 (1)
2-6:⚠️ Potential issue | 🟠 MajorAvoid silent success for unimplemented retrieval APIs.
Line 2 through Line 6 currently return empty lists regardless of input, which can hide missing implementation and mislead callers into treating failures as legitimate “no results.”
Suggested guard until implementation is ready
class VectorMemory: def semantic_search(self, user_id: str, query: str, top_k: int = 5): - return [] + raise NotImplementedError("VectorMemory.semantic_search is not implemented yet") def time_based_retrieval(self, user_id: str, limit: int = 10): - return [] + raise NotImplementedError("VectorMemory.time_based_retrieval is not implemented yet")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/memory/vector_memory.py` around lines 2 - 6, The methods semantic_search and time_based_retrieval currently silently return empty lists; replace those returns with explicit guards that raise NotImplementedError (or a custom UnimplementedAPIError) including a clear message like "semantic_search not implemented for VectorMemory" and "time_based_retrieval not implemented for VectorMemory" respectively, so callers cannot mistake unimplemented behavior for valid empty results; update the method docstrings to state they are unimplemented and adjust callers/tests to handle the raised exception if necessary.runtime_environment_check.py-53-64 (1)
53-64:⚠️ Potential issue | 🟠 MajorReturn a structured failure when the workspace is unusable.
Line 55 can raise on a read-only home directory or when
~/.neuroriftalready exists as a file, and an existing non-writable directory still reportsok=True. The launcher will get a traceback instead of the JSON report this script is supposed to emit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime_environment_check.py` around lines 53 - 64, The function verify_runtime_environment should not raise when the workspace is unusable: before calling workdir.mkdir handle the cases where Path.home()/".neurorift" exists as a file or where an existing directory is not writable, and wrap the mkdir call in a try/except to catch OSError; on any failure return the structured dict with "ok": False, "workdir": str(workdir), "tools": checks (or empty/partial) and an "error" or "reason" string describing the failure. Specifically, in verify_runtime_environment check workdir.exists() and workdir.is_dir() and os.access(workdir, os.W_OK) (or try creating a temp file) to detect non-writable dirs, catch exceptions from workdir.mkdir, and ensure REQUIRED_TOOLS/check_tool logic still runs or is skipped but the returned JSON reflects the workspace error.neurorift/skills/registry.py-10-11 (1)
10-11:⚠️ Potential issue | 🟠 MajorPersist
registry.jsonatomically.These writes are in-place, so an interruption can leave invalid JSON on disk and two concurrent installs can overwrite each other's changes. Use temp-file + replace for persistence, and lock the read/modify/write path if multiple processes may manage skills.
Also applies to: 20-32
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/skills/registry.py` around lines 10 - 11, The current writes to self.path (e.g., the initial json dump shown) are in-place and must be made atomic and concurrency-safe: when writing registry JSON (the spots using self.path.write_text around lines 10 and 20-32), write to a temporary file in the same directory and atomically rename/replace the temp into place (os.replace or pathlib.Path.replace) to avoid partial files, and wrap the entire read/modify/write sequence in a process-level file lock (e.g., filelock.FileLock or fcntl-based lock) so concurrent installers cannot interleave; update any helper method that performs registry writes (the method that currently calls self.path.write_text) to implement temp-file + replace + lock for all registry updates.runtime_environment_check.py-32-34 (1)
32-34:⚠️ Potential issue | 🟠 MajorProbe the resolved executable, not a second PATH lookup.
You already validated
resolvedat line 14, but line 33 executestoolagain via PATH. That can probe a different binary and makes the validation less trustworthy than it appears.Suggested fix
probe = subprocess.run( - [tool, "--help"], capture_output=True, text=True, timeout=8, check=False + [resolved, "--help"], capture_output=True, text=True, timeout=8, check=False )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime_environment_check.py` around lines 32 - 34, The probe is invoking subprocess.run with the original tool name (variable "tool") which re-resolves via PATH and can point to a different binary than the previously validated "resolved" path; change the subprocess.run call that creates "probe" to call the validated "resolved" executable instead of "tool" (i.e., run [resolved, "--help"] with the same capture_output/text/timeout/check args) so the help probe targets the exact file you've already validated (and keep any existing executable/path checks around "resolved").neurorift/tools/tool_executor.py-1-3 (1)
1-3:⚠️ Potential issue | 🟠 MajorDo not report tool success without executing anything.
execute()always returns"success": Trueeven though no validation, dispatch, or sandboxed execution happened. In an agent loop that makes skipped side effects indistinguishable from a real tool run.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/tools/tool_executor.py` around lines 1 - 3, The ToolExecutor.execute method currently returns {"success": True} without performing validation or execution; change execute in class ToolExecutor to not claim success by default — either implement real validation/dispatch/sandboxing for the provided tool_call dict (validate required keys, run the tool, capture stdout/exit/status and include results in the returned dict) or make execute raise NotImplementedError / return {"success": False, "error": "not implemented"} so callers cannot treat skipped runs as successful; update callers to handle the error/false result accordingly.neurorift/sessions/session_store.py-14-22 (1)
14-22:⚠️ Potential issue | 🟠 MajorPersisted sessions lose their original types.
json.dumps(session.__dict__, default=str)stringifies non-JSON fields, andload()feeds those strings straight back intoSessionContext. If the context contains enums, datetimes,Paths, or nested objects, loaded sessions will not behave like freshly created ones. Add explicitto_dict/from_dictserialization instead of round-tripping__dict__.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/sessions/session_store.py` around lines 14 - 22, The save/load pair currently serializes session.__dict__ with json.dumps(default=str) causing loss of types; update SessionContext to implement a to_dict(self) that converts enums, datetimes, Path, and nested objects into JSON-serializable primitives and a classmethod from_dict(cls, data) that reconstructs original types, then change session_store.save to write json.dump(session.to_dict()) and session_store.load to call SessionContext.from_dict(json.load(...)) (use the existing path(...) and session.session_id/ sid symbols to locate where to read/write).neurorift/models/model_router.py-2-7 (1)
2-7:⚠️ Potential issue | 🟠 MajorDo not return a fake generation result.
This method never routes to a model backend: it just echoes the prompt and reports
"openai"regardless of the active provider. That makes missing integration look like a successful model response, which is much harder to detect than failing closed here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/models/model_router.py` around lines 2 - 7, The generate method in model_router.py currently returns a fake echo response (hardcoding "openai") instead of dispatching to the configured model backend; replace the fake result in async def generate(self, prompt: str) with real routing logic: call the router/dispatcher or backend client used elsewhere in this module (e.g., the method/attribute that sends requests to the active provider—refer to your router/dispatcher function or self.<client> or self._dispatch/_route_to_backend), await its response, and return the backend's response object (including accurate model identifier, token count and cost); if no backend is configured, raise a clear exception (e.g., RuntimeError or NotImplementedError) instead of returning a fake success so missing integrations fail loudly.model_capability_check.py-72-88 (1)
72-88:⚠️ Potential issue | 🟠 MajorValidate capability values as booleans before computing
ok.Right now any truthy value passes. A response like
"agent_ready": "false"satisfies the key check and thenbool("false")makesoktrue, so the launcher can accept a non-ready model.Suggested hardening
if not required.issubset(parsed): return { "ok": False, "error": "capability_fields_missing", "parsed": parsed, "agent_ready": False, } - parsed["ok"] = bool(parsed.get("agent_ready")) + invalid = { + key: value + for key, value in parsed.items() + if key in required and not isinstance(value, bool) + } + if invalid: + return { + "ok": False, + "error": "capability_fields_invalid", + "parsed": parsed, + "agent_ready": False, + } + + parsed["ok"] = parsed["agent_ready"] return parsed🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_capability_check.py` around lines 72 - 88, The code currently only checks keys in the required set but treats any truthy value as True (e.g., "false" -> True); fix model_capability_check.py by validating and normalizing capability values to booleans before computing ok: after the existing required subset check, iterate the fields in the required set (including "agent_ready") and for each value in parsed ensure it's either a bool or a string equal to "true"/"false" (case-insensitive); convert valid string values to actual booleans and replace parsed[field] with the boolean, and if any field is missing a parseable boolean return a failure (ok: False, error: "capability_values_invalid", agent_ready: False) instead of proceeding; finally set parsed["ok"] = parsed["agent_ready"] (which is now a real boolean).neurorift/models/model_failover.py-2-3 (1)
2-3:⚠️ Potential issue | 🟠 MajorFail fast instead of returning a synthetic provider.
When
providersis empty, this returns"fallback"as if it were a real backend. That pushes the misconfiguration downstream into a harder-to-debug routing failure instead of surfacing it here.Minimal fix
class ModelFailover: def pick(self, providers): - return providers[0] if providers else "fallback" + if not providers: + raise ValueError("No model providers configured") + return providers[0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/models/model_failover.py` around lines 2 - 3, The pick method in model_failover.py should fail fast instead of returning the synthetic string "fallback": update the pick(self, providers) implementation to raise a clear exception (e.g., ValueError or RuntimeError) when providers is empty, with a descriptive message like "no providers available for model selection", and otherwise return providers[0]; adjust any callers if necessary to handle or propagate this exception so misconfiguration is surfaced immediately.neurorift_launch.sh-30-35 (1)
30-35:⚠️ Potential issue | 🟠 MajorGate startup on parsed JSON, not a grep match.
Using
grephere makes the launch decision depend on output formatting instead of the checker’s structured result. A small formatting change or extra diagnostic text can flip startup behavior without any real capability change.Parse the result explicitly
CAP_JSON="$(python3 "$SCRIPT_DIR/model_capability_check.py" --model "$MODEL_NAME")" echo "$CAP_JSON" -if ! echo "$CAP_JSON" | grep -q '"agent_ready": true'; then +if ! python3 -c 'import json,sys; sys.exit(0 if json.load(sys.stdin).get("agent_ready") is True else 1)' <<<"$CAP_JSON"; then echo "❌ Model is not agent-ready. Aborting NeuroRift startup." exit 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift_launch.sh` around lines 30 - 35, The startup currently gates on a grep against CAP_JSON which is fragile; instead parse the JSON in CAP_JSON and check the boolean field explicitly (e.g. use jq -e '.agent_ready == true' or run a short python snippet to load CAP_JSON and test data["agent_ready"]). Update the block that sets and inspects CAP_JSON (and the branch that echoes and exits) to perform a proper JSON parse/boolean check against the "agent_ready" field rather than using grep, keeping the CAP_JSON variable and error/exit behavior intact.neurorift_launch.sh-27-30 (1)
27-30:⚠️ Potential issue | 🟠 MajorResolve helper scripts relative to the launcher, not the caller’s CWD.
Lines 27 and 30 invoke
runtime_environment_check.pyandmodel_capability_check.pyby bare relative path. If this script is launched from anywhere except the repo root, startup fails before it even reachesneurorift.Safer path resolution
#!/usr/bin/env bash set -euo pipefail +SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)" + MODEL_NAME="${1:-}" BOOT_PROMPT="You are NeuroRift, an autonomous cybersecurity agent. Boot in safe authorized mode, load skills, verify tools, and await tasking." @@ echo "[1/5] Verifying runtime environment..." -python3 runtime_environment_check.py || true +python3 "$SCRIPT_DIR/runtime_environment_check.py" || true @@ echo "[2/5] Verifying model capabilities for $MODEL_NAME..." -CAP_JSON="$(python3 model_capability_check.py --model "$MODEL_NAME")" +CAP_JSON="$(python3 "$SCRIPT_DIR/model_capability_check.py" --model "$MODEL_NAME")"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift_launch.sh` around lines 27 - 30, The helper scripts are invoked via bare relative paths so launching the launcher from a different CWD can fail; update calls to use the launcher's directory (e.g., compute SCRIPT_DIR from "${BASH_SOURCE[0]}" or equivalent) and invoke the helpers as "$SCRIPT_DIR/runtime_environment_check.py" and "$SCRIPT_DIR/model_capability_check.py" (preserving the existing `|| true` behavior and the variable usage for $MODEL_NAME) so the scripts are resolved relative to the launcher rather than the caller's CWD.neurorift_launch.sh-40-41 (1)
40-41:⚠️ Potential issue | 🟠 MajorPass the validated model to the
neurorift run-agentcommand.Steps 2-3 select and verify
$MODEL_NAME, but Step 4 launches the agent without it. Therun-agentcommand supports--modelparameter but falls back to theOLLAMA_MODELenvironment variable if not provided. Add--model "$MODEL_NAME"to ensure the runtime uses the same validated model.Current code
echo "[4/5] Starting NeuroRift agent runtime..." neurorift run-agent --target "local-environment" --mode defensive🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift_launch.sh` around lines 40 - 41, The run step currently calls neurorift run-agent without specifying the validated model, so modify the command that invokes neurorift run-agent to pass the confirmed MODEL_NAME by adding --model "$MODEL_NAME" (ensuring it uses the same validated model rather than relying on OLLAMA_MODEL); update the invocation of neurorift run-agent to include the --model "$MODEL_NAME" flag alongside --target "local-environment" and --mode defensive.neurorift/skills/installer.py-30-35 (1)
30-35:⚠️ Potential issue | 🟠 MajorSame path traversal risk in uninstall.
The
nameparameter is used directly without sanitization, similar to theinstallmethod. Apply the same validation here.🔒 Proposed fix
def uninstall(self, name: str): + if not name or "/" in name or "\\" in name or name in (".", ".."): + return {"success": False, "error": "invalid skill name"} dst = self.installed / name + if not dst.resolve().is_relative_to(self.installed.resolve()): + return {"success": False, "error": "invalid skill name"} if dst.exists(): shutil.rmtree(dst) self.registry.remove(name) return {"success": True, "name": name}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/skills/installer.py` around lines 30 - 35, The uninstall method uses the incoming name directly to build dst = self.installed / name, creating the same path traversal risk as install; validate and sanitize the name before deleting and before calling self.registry.remove. Fix by resolving the constructed path (dst = (self.installed / name).resolve()), ensure the resolved dst is inside self.installed.resolve() (or reject names containing '..' or absolute paths), only perform shutil.rmtree(dst) and self.registry.remove(name) after that validation, and avoid removing registry entries if validation fails.neurorift/skills/installer.py-17-28 (1)
17-28:⚠️ Potential issue | 🟠 MajorPotential path traversal vulnerability via skill name.
The
namefield is read directly fromskill.jsonand used to construct the destination path without sanitization. A malicious skill package could use a name like../../etc/maliciousto write outside the intended directory.🔒 Proposed fix to sanitize skill name
def install(self, pkg: Path): ok, missing = self.validator.validate(pkg) if not ok: return {"success": False, "error": f"missing:{missing}"} meta = json.loads((pkg / "skill.json").read_text(encoding="utf-8")) name = meta["name"] + # Sanitize name to prevent path traversal + if not name or "/" in name or "\\" in name or name in (".", ".."): + return {"success": False, "error": "invalid skill name"} dst = self.installed / name + # Ensure dst is still under installed directory + if not dst.resolve().is_relative_to(self.installed.resolve()): + return {"success": False, "error": "invalid skill name"} if dst.exists(): shutil.rmtree(dst) shutil.copytree(pkg, dst)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/skills/installer.py` around lines 17 - 28, The install method reads meta["name"] and uses it to build dst which allows path traversal; fix install in installer.py by validating/sanitizing the skill name before using it: reject or normalize names containing path separators or traversal components (e.g., "/" "\" ".."), enforce a safe pattern (e.g., allow only alphanumerics, hyphen, underscore) and/or replace invalid chars, then compute dst = (self.installed / safe_name).resolve() and verify dst.is_relative_to(self.installed.resolve()) (or compare path prefixes) to ensure the final destination is inside the intended directory; if validation fails, return the existing error response instead of writing, and keep registry.add(name) using the sanitized safe_name.
🟡 Minor comments (9)
neurorift/execution/sandbox_runner.py-4-5 (1)
4-5:⚠️ Potential issue | 🟡 MinorEmpty
SandboxRunneris misleading - no actual sandboxing is implemented.The class name implies security isolation, but it inherits
CommandRunnerwithout adding any sandboxing behavior. This could give users a false sense of security. Consider either:
- Adding a docstring/TODO clarifying this is a placeholder, or
- Implementing actual sandboxing (e.g., chroot, containers, seccomp, or at minimum restricted PATH/environment)
📝 Minimal fix to clarify intent
class SandboxRunner(CommandRunner): - pass + """Placeholder for sandboxed command execution. + + TODO: Implement actual sandboxing (e.g., restricted environment, + resource limits, namespace isolation). + """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/execution/sandbox_runner.py` around lines 4 - 5, The SandboxRunner class currently just subclasses CommandRunner without any sandboxing, which is misleading; update SandboxRunner to either (a) include a clear docstring/TODO stating it is a placeholder and does not provide security/isolation, referencing the class name SandboxRunner and base class CommandRunner, or (b) implement minimal sandboxing in its run/execute method by restricting environment/PATH and dropping privileges (e.g., explicit env dict, limited PATH, uid/gid change) and note where stronger sandboxing (chroot/containers/seccomp) should be added; pick one approach and add the docstring/TODO and/or the implementation hooks (run/execute) so intent and behavior are explicit.neurorift/memory/short_term_memory.py-5-6 (1)
5-6:⚠️ Potential issue | 🟡 MinorValidate
limitto prevent degenerate buffer behavior.Line 5 should reject non-positive values; otherwise retention can silently break (e.g., zero-capacity buffers).
Proposed validation
class ShortTermMemory: def __init__(self, limit: int = 30): + if limit <= 0: + raise ValueError("limit must be > 0") self.buffers = defaultdict(lambda: deque(maxlen=limit))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/memory/short_term_memory.py` around lines 5 - 6, The constructor for ShortTermMemory currently allows non-positive limits which creates zero-capacity deques; in __init__ validate the limit parameter (ensure it's an int and > 0) and raise a clear ValueError (or TypeError for wrong type) if invalid, then use the validated limit when creating self.buffers = defaultdict(lambda: deque(maxlen=limit)); reference the __init__, buffers, defaultdict, deque and limit symbols when making this change.neurorift/storage/database.py-5-7 (1)
5-7:⚠️ Potential issue | 🟡 MinorAdd explicit directory validation for
rootbefore creation.Line 7 currently relies on
mkdirto fail ifrootis a file, which yields a generic error. A direct check gives a clearer and safer failure mode for callers.Suggested patch
class Database: def __init__(self, root: Path): - self.root = root - root.mkdir(parents=True, exist_ok=True) + self.root = Path(root) + if self.root.exists() and not self.root.is_dir(): + raise NotADirectoryError(f"Storage root is not a directory: {self.root}") + self.root.mkdir(parents=True, exist_ok=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/storage/database.py` around lines 5 - 7, In Database.__init__, add an explicit validation before calling root.mkdir: check if root exists and is a file (root.exists() and root.is_file()) and raise a clear exception (e.g., raise NotADirectoryError or ValueError with a descriptive message) so callers get a specific error instead of a generic mkdir failure; then proceed to call root.mkdir(parents=True, exist_ok=True) to create the directory when validation passes.neurorift/config/settings.py-5-7 (1)
5-7:⚠️ Potential issue | 🟡 MinorUse
field(default_factory=...)for lazy evaluation ofdata_dir.Currently,
Path.home()is evaluated at class definition time, not instance creation. While this pattern isn't problematic in practice, it's best to defer the evaluation usingfield(default_factory=...)to avoid potential issues if the code is used in contexts wherePath.home()might change or need to be mocked in tests.-from dataclasses import dataclass +from dataclasses import dataclass, field from pathlib import Path `@dataclass` class Settings: - data_dir: Path = Path.home() / ".neurorift" + data_dir: Path = field(default_factory=lambda: Path.home() / ".neurorift")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/config/settings.py` around lines 5 - 7, The Settings dataclass currently sets data_dir = Path.home() at class definition time; change it to use dataclasses.field(default_factory=...) so Path.home() is evaluated when an instance is created. Update the Settings class's data_dir attribute to use field(default_factory=lambda: Path.home() / ".neurorift") (import field from dataclasses) so that Settings and any code constructing Settings get a lazily-evaluated home path.neurorift/channels/discord_channel.py-5-6 (1)
5-6:⚠️ Potential issue | 🟡 MinorPreserve falsy payloads in
receive_message.
payload or {}rewrites valid empty payloads like"",0,False, or[]into{}. Only fall back when the caller passedNone.Suggested fix
def receive_message(self, payload=None): - return payload or {} + return payload if payload is not None else {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/channels/discord_channel.py` around lines 5 - 6, The current receive_message method overwrites valid falsy payloads (e.g., "", 0, False, []) because it uses "payload or {}", so change it to only default when the caller passed None: in receive_message, check if payload is None and return {} in that case, otherwise return the original payload (keep the function name receive_message to locate the change).neurorift/channels/telegram_channel.py-5-6 (1)
5-6:⚠️ Potential issue | 🟡 MinorPreserve falsy payloads in
receive_message.
payload or {}rewrites valid empty payloads like"",0,False, or[]into{}. Only fall back when the caller passedNone.Suggested fix
def receive_message(self, payload=None): - return payload or {} + return payload if payload is not None else {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/channels/telegram_channel.py` around lines 5 - 6, The receive_message function currently uses a truthy fallback (payload or {}) which converts valid falsy payloads (e.g., "", 0, False, []) into {}, so change the fallback logic to only substitute {} when payload is None; update receive_message to return payload unchanged for all non-None values and return {} only when payload is exactly None (e.g., use an explicit None check in receive_message).neurorift/sessions/session_manager.py-21-22 (1)
21-22:⚠️ Potential issue | 🟡 MinorCache sessions loaded from store to avoid repeated I/O.
When
get_sessionloads a session from the store, it doesn't add it toself.sessions. This causes repeated disk reads for the same session and potential inconsistencies if the loaded session is later modified in memory but not reflected in the cache.🐛 Proposed fix
- def get_session(self, sid: str): - return self.sessions.get(sid) or self.store.load(sid) + def get_session(self, sid: str) -> SessionContext | None: + if sid in self.sessions: + return self.sessions[sid] + loaded = self.store.load(sid) + if loaded: + self.sessions[sid] = loaded + return loaded🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/sessions/session_manager.py` around lines 21 - 22, get_session currently returns self.store.load(sid) without caching the result, causing repeated I/O; modify get_session so that if self.sessions.get(sid) is falsy it calls self.store.load(sid), stores the loaded session into self.sessions[sid] (if not None), and then returns the cached session; reference the method name get_session and attributes self.sessions and self.store.load when making the change.neurorift_cli.py-61-64 (1)
61-64:⚠️ Potential issue | 🟡 MinorReturn value from
neurorift_main.main()is ignored.If
neurorift_main.main()returns a non-zero exit code to indicate failure, this function will still return 0. Consider capturing and returning its exit code.🔧 Proposed fix
# Route into existing app entrypoint; keep compatibility with current parser. - sys.argv = ["neurorift"] + forwarded - neurorift_main.main() - return 0 + sys.argv = ["neurorift", *forwarded] + return neurorift_main.main() or 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift_cli.py` around lines 61 - 64, The wrapper currently overwrites sys.argv and calls neurorift_main.main() but always returns 0; change it to capture the return value (e.g., rc = neurorift_main.main()), treat None as 0 if necessary, and return rc so non-zero exit codes propagate; update the call site around sys.argv and neurorift_main.main() to return the captured exit code instead of the hardcoded return 0.neurorift_cli.py-15-32 (1)
15-32:⚠️ Potential issue | 🟡 MinorValidate that
main_pathexists before writing the wrapper.If
neurorift_main.pydoesn't exist at the resolved path, the wrapper script will be created but will fail at runtime. Consider adding an existence check.🛡️ Proposed fix
def install_global_wrapper() -> int: wrapper_path = Path("/usr/local/bin/neurorift") main_path = (Path(__file__).resolve().parent / "neurorift_main.py").resolve() + if not main_path.exists(): + print(f"Error: neurorift_main.py not found at {main_path}") + return 1 script = f'#!/usr/bin/env bash\npython3 {main_path} "$@"\n'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift_cli.py` around lines 15 - 32, The install_global_wrapper function currently writes a wrapper pointing to main_path without verifying neurorift_main.py exists; update install_global_wrapper to check that main_path.exists() (or main_path.is_file()) before creating the wrapper, and if it does not exist print a clear error (including the resolved main_path) and return non-zero so the wrapper is not written; keep the existing PermissionError/Exception handling and messages for wrapper_path to avoid changing other behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2c92e4f7-f40f-4bc2-9396-30e461514f42
📒 Files selected for processing (82)
model_capability_check.pyneurorift/__init__.pyneurorift/channels/__init__.pyneurorift/channels/channel_router.pyneurorift/channels/cli_channel.pyneurorift/channels/discord_channel.pyneurorift/channels/telegram_channel.pyneurorift/channels/web_channel.pyneurorift/clawhub/__init__.pyneurorift/clawhub/clawhub_api.pyneurorift/clawhub/clawhub_client.pyneurorift/clawhub/clawhub_resolver.pyneurorift/cli/__init__.pyneurorift/cli/neurorift_cli.pyneurorift/config/__init__.pyneurorift/config/channel_config.pyneurorift/config/model_config.pyneurorift/config/settings.pyneurorift/core/__init__.pyneurorift/core/agent_loop.pyneurorift/core/agent_manager.pyneurorift/core/planner.pyneurorift/core/task_router.pyneurorift/execution/__init__.pyneurorift/execution/command_runner.pyneurorift/execution/resource_limits.pyneurorift/execution/retry_manager.pyneurorift/execution/sandbox_runner.pyneurorift/gateway/__init__.pyneurorift/gateway/api_server.pyneurorift/gateway/auth_manager.pyneurorift/gateway/websocket_gateway.pyneurorift/memory/__init__.pyneurorift/memory/long_term_memory.pyneurorift/memory/memory_compaction.pyneurorift/memory/short_term_memory.pyneurorift/memory/vector_memory.pyneurorift/models/__init__.pyneurorift/models/context_builder.pyneurorift/models/model_failover.pyneurorift/models/model_router.pyneurorift/models/prompt_builder.pyneurorift/sessions/__init__.pyneurorift/sessions/session_context.pyneurorift/sessions/session_manager.pyneurorift/sessions/session_pruner.pyneurorift/sessions/session_store.pyneurorift/skill_store/__init__.pyneurorift/skill_store/cache/__init__.pyneurorift/skill_store/installed/__init__.pyneurorift/skill_store/installed/recon_scanner/README.mdneurorift/skill_store/installed/recon_scanner/__init__.pyneurorift/skill_store/installed/recon_scanner/requirements.txtneurorift/skill_store/installed/recon_scanner/skill.jsonneurorift/skill_store/installed/recon_scanner/skill.pyneurorift/skills/__init__.pyneurorift/skills/installer.pyneurorift/skills/loader.pyneurorift/skills/registry.pyneurorift/skills/skill_manager.pyneurorift/skills/skill_validator.pyneurorift/storage/__init__.pyneurorift/storage/database.pyneurorift/storage/logs_db.pyneurorift/storage/memory_db.pyneurorift/storage/session_db.pyneurorift/tools/__init__.pyneurorift/tools/tool_executor.pyneurorift/tools/tool_registry.pyneurorift/tools/tool_sandbox.pyneurorift/tools/tool_validator.pyneurorift/utils/__init__.pyneurorift/utils/logger.pyneurorift/utils/serializer.pyneurorift/utils/time_utils.pyneurorift/utils/validator.pyneurorift_cli.pyneurorift_launch.shneurorift_main.pyneurorift_wrapper.shruntime_environment_check.pysetup.py
Comment on lines
+10
to
+17
| def fetch_skill(self, skill_name: str, source_dir: Path) -> Path: | ||
| src = source_dir / skill_name | ||
| if not src.exists(): | ||
| raise FileNotFoundError(skill_name) | ||
| dst = self.cache_dir / skill_name | ||
| if dst.exists(): | ||
| shutil.rmtree(dst) | ||
| shutil.copytree(src, dst) |
There was a problem hiding this comment.
Reject traversal in skill_name before touching the filesystem.
skill_name is joined directly into both source_dir and cache_dir. Values like ../foo can make Line 16 delete outside the cache root and Line 17 copy arbitrary directories into it.
Suggested fix
def fetch_skill(self, skill_name: str, source_dir: Path) -> Path:
- src = source_dir / skill_name
- if not src.exists():
+ src_root = source_dir.resolve()
+ dst_root = self.cache_dir.resolve()
+ src = (src_root / skill_name).resolve()
+ dst = (dst_root / skill_name).resolve()
+ if src_root not in src.parents or dst_root not in dst.parents:
+ raise ValueError("invalid skill name")
+ if not src.is_dir():
raise FileNotFoundError(skill_name)
- dst = self.cache_dir / skill_name
if dst.exists():
shutil.rmtree(dst)
shutil.copytree(src, dst)
return dst🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@neurorift/clawhub/clawhub_client.py` around lines 10 - 17, Reject path
traversal in fetch_skill by validating skill_name before any filesystem
operations: ensure skill_name is a single path segment (no "..", no absolute
paths, no path separators) and normalize/resolve only after validation; use the
validated name to build src = source_dir / skill_name and dst = self.cache_dir /
skill_name, then proceed to check existence and perform
shutil.rmtree/shutil.copytree. Refer to symbols fetch_skill, skill_name,
source_dir, cache_dir, src, and dst when making the change.
Comment on lines
+2
to
+3
| def validate(self, token: str): | ||
| return bool(token) |
There was a problem hiding this comment.
AuthManager.validate() is effectively an auth bypass for any non-empty token.
Line [3] accepts arbitrary non-empty input as valid credentials. This is a critical auth flaw; validation must be cryptographic/authoritative and fail closed on errors.
Safer pattern (inject verifier, fail closed)
class AuthManager:
- def validate(self, token: str):
- return bool(token)
+ def __init__(self, token_verifier):
+ self._token_verifier = token_verifier
+
+ def validate(self, token: str) -> bool:
+ if not isinstance(token, str) or not token.strip():
+ return False
+ try:
+ return bool(self._token_verifier(token))
+ except Exception:
+ return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@neurorift/gateway/auth_manager.py` around lines 2 - 3, AuthManager.validate
currently treats any non-empty string as valid; replace the bool(token) bypass
with a real verification call by injecting or using an authoritative verifier
(e.g., a provided token_verifier.verify or verify_token method) inside
AuthManager.validate (or accept a verifier in __init__), call that verifier with
the token, and return its boolean result; wrap the call in try/except and always
fail closed (return False) on exceptions or if no verifier is configured to
ensure invalid/errored tokens are rejected.
Comment on lines
+11
to
+12
| def path(self, sid: str) -> Path: | ||
| return self.root / f"{sid}.json" |
There was a problem hiding this comment.
Reject session IDs that escape self.root.
Line 12 interpolates sid directly into a filesystem path. Values like ../../tmp/pwned or foo/bar will read or write outside the session directory, which turns session persistence into a path traversal primitive.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@neurorift/sessions/session_store.py` around lines 11 - 12, The path
construction in path(self, sid: str) -> Path currently interpolates sid directly
into self.root, allowing path traversal; fix by first validating/sanitizing sid
(e.g., reject if it contains path separators or '..' or enforce a safe regex
like alphanumeric/underscore/hyphen, or use Path(sid).name to take only the
basename), then build the candidate path and verify the resolved path is inside
self.root (compare candidate.resolve() against self.root.resolve(), or use
Path.is_relative_to) and raise an exception (e.g., ValueError) if the check
fails; update path(self, sid) to perform these checks before returning the file
path.
Comment on lines
+7
to
+12
| meta = json.loads((skill_dir / "skill.json").read_text(encoding="utf-8")) | ||
| entry = skill_dir / meta["entrypoint"] | ||
| spec = importlib.util.spec_from_file_location(f"skill_{meta['name']}", entry) | ||
| mod = importlib.util.module_from_spec(spec) | ||
| spec.loader.exec_module(mod) | ||
| return mod.run(**kwargs) if hasattr(mod, "run") else {"error": "missing run()"} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "loader.py" -path "*/skills/*" | head -20Repository: demonking369/NeuroRift
Length of output: 94
🏁 Script executed:
git ls-files | grep -E "skills.*loader\.py"Repository: demonking369/NeuroRift
Length of output: 92
🏁 Script executed:
cd . && find . -type f -name "*.py" | grep loaderRepository: demonking369/NeuroRift
Length of output: 94
🏁 Script executed:
cat -n neurorift/skills/loader.pyRepository: demonking369/NeuroRift
Length of output: 661
🏁 Script executed:
python3 << 'EOF'
import importlib.util
from pathlib import Path
import tempfile
import os
# Test 1: Path traversal with relative paths
with tempfile.TemporaryDirectory() as tmpdir:
skill_dir = Path(tmpdir) / "skill"
skill_dir.mkdir()
outside_dir = Path(tmpdir) / "outside"
outside_dir.mkdir()
# Create a test file outside skill_dir
outside_file = outside_dir / "outside.py"
outside_file.write_text("def run(): return 'outside'")
# Try to construct a path that escapes
malicious_entry = skill_dir / "../outside/outside.py"
print(f"skill_dir: {skill_dir}")
print(f"malicious_entry: {malicious_entry}")
print(f"malicious_entry.resolve(): {malicious_entry.resolve()}")
print(f"skill_dir.resolve(): {skill_dir.resolve()}")
print(f"Path escapes: {skill_dir.resolve() not in malicious_entry.resolve().parents and malicious_entry.resolve() != skill_dir.resolve()}")
print()
# Test 2: spec_from_file_location with None/invalid
print("Test 2: spec_from_file_location behavior")
spec_valid = importlib.util.spec_from_file_location("test", __file__)
print(f"Valid file spec: {spec_valid}")
print(f"spec.loader: {spec_valid.loader if spec_valid else 'N/A'}")
print()
spec_invalid = importlib.util.spec_from_file_location("test", "/nonexistent/file.py")
print(f"Invalid file spec: {spec_invalid}")
print()
# Test 3: hasattr vs callable
print("Test 3: hasattr vs callable")
class TestMod:
run = "not_callable"
mod = TestMod()
print(f"hasattr(mod, 'run'): {hasattr(mod, 'run')}")
print(f"callable(getattr(mod, 'run', None)): {callable(getattr(mod, 'run', None))}")
EOFRepository: demonking369/NeuroRift
Length of output: 620
🏁 Script executed:
python3 << 'EOF'
# Test what happens when spec is None
spec = None
try:
spec.loader
except AttributeError as e:
print(f"AttributeError when accessing spec.loader on None: {e}")
# Also check the actual line 11 behavior
try:
mod = None
spec.loader.exec_module(mod)
except AttributeError as e:
print(f"AttributeError on line 11 equivalent: {e}")
EOFRepository: demonking369/NeuroRift
Length of output: 242
Validate the entrypoint path and ensure spec/loader exist before executing.
meta["entrypoint"] is not validated, allowing path traversal attacks like ../outside.py to execute code outside the skill directory. Additionally, spec_from_file_location() can return None, causing a crash when accessing spec.loader on line 11. The hasattr check also fails to verify that run is callable, not just present.
Suggested fix
-import importlib.util, json
+import importlib.util
+import json
from pathlib import Path
class SkillLoader:
def run(self, skill_dir: Path, **kwargs):
meta = json.loads((skill_dir / "skill.json").read_text(encoding="utf-8"))
- entry = skill_dir / meta["entrypoint"]
+ skill_root = skill_dir.resolve()
+ entry = (skill_root / meta["entrypoint"]).resolve()
+ if skill_root not in entry.parents or not entry.is_file():
+ return {"error": "invalid entrypoint"}
spec = importlib.util.spec_from_file_location(f"skill_{meta['name']}", entry)
+ if spec is None or spec.loader is None:
+ return {"error": "unsupported entrypoint"}
mod = importlib.util.module_from_spec(spec)
spec.loader.exec_module(mod)
- return mod.run(**kwargs) if hasattr(mod, "run") else {"error": "missing run()"}
+ run = getattr(mod, "run", None)
+ return run(**kwargs) if callable(run) else {"error": "missing run()"}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@neurorift/skills/loader.py` around lines 7 - 12, Validate and sanitize the
entrypoint before loading: resolve meta["entrypoint"] against skill_dir (use
pathlib.Path(skill_dir) / meta["entrypoint"] then .resolve()) and ensure the
resolved path is inside skill_dir.resolve() and is a file to prevent path
traversal; after calling importlib.util.spec_from_file_location check that spec
is not None and spec.loader is present before calling
spec.loader.exec_module(mod) and handle the error case with a clear {"error":
"..."} return; finally, when returning the module result verify hasattr(mod,
"run") and callable(mod.run) (or use getattr(mod, "run", None) and callable) and
call it safely inside try/except to return an error dict on exceptions.
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.
User description
Motivation
neuroriftpackage and CLI, add runtime and model capability checks, and harden tool execution for safer autonomous agent operation.Description
model_capability_check.pyto query an Ollama model with a capability prompt and parse a JSON readiness response, and addedruntime_environment_check.pyto validate required system tools and workspace.neuroriftpackage with many lightweight module stubs and implementations (channels, core, execution, gateway, memory, models, sessions, skills, storage, tools, utils) and an examplerecon_scannerskill underskill_store/installed.neurorift_main.pyto initialize the new skill manager, wire in model/runtime checks, and replace ad-hoc subprocess usage with a hardened_run_command_securerunner plus validation, sandboxing, resource limits, retries, and improved logging.neurorift_cli.pyas the global CLI entry wrapper,neurorift_launch.shfor guided startup, and updatedsetup.pyto include the new package and entry point adjustment.Testing
python3 runtime_environment_check.pywhich ran the automated tool probes and returned a JSON report of tool statuses successfully.python3 model_capability_check.py --model nonexistent_modelin an environment withoutollamaand observed the expectedollama_missingerror JSON response from the script.python3 neurorift_cli.py --helpto validate the CLI parser and confirmed the help output returned successfully.Codex Task
CodeAnt-AI Description
Add NeuroRift package scaffold, runtime & model checks, secure command runner, CLI and skill system
What Changed
Impact
✅ Clearer model readiness errors✅ Safer tool execution (retries, resource limits, sanitized inputs)✅ Ability to install and run skills from a local/ClawHub registry💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
Release Notes
New Features
Chores