-
Notifications
You must be signed in to change notification settings - Fork 0
Add NeuroRift package scaffold, runtime & model checks, secure command runner, CLI and skill system #38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add NeuroRift package scaffold, runtime & model checks, secure command runner, CLI and skill system #38
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,92 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Ollama model capability verification for autonomous NeuroRift agent mode.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from __future__ import annotations | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import json | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import subprocess | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from typing import Any | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| CAPABILITY_PROMPT = """Evaluate whether you can reliably perform the following tasks: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * structured tool invocation | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Linux command generation | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * file creation and modification | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * interpreting tool outputs | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * multi-step reasoning for autonomous agents | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Return a JSON response: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "tool_usage": true/false, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "command_generation": true/false, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "filesystem_operations": true/false, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "multi_step_reasoning": true/false, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "agent_ready": true/false | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def _extract_json(raw_text: str) -> dict[str, Any]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return json.loads(raw_text[start : end + 1]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def verify_model_capabilities(model_name: str) -> dict[str, Any]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| proc = subprocess.run( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ["ollama", "run", model_name, CAPABILITY_PROMPT], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| capture_output=True, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| text=True, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| timeout=120, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| check=False, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except FileNotFoundError: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return {"ok": False, "error": "ollama_missing", "agent_ready": False} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except Exception as exc: # defensive for unstable environments | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return {"ok": False, "error": f"ollama_exec_error:{type(exc).__name__}", "agent_ready": False} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if proc.returncode != 0: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "ok": False, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "error": f"ollama_returned_{proc.returncode}", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "stderr": proc.stderr, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "agent_ready": False, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| parsed = _extract_json(proc.stdout) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except Exception as exc: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "ok": False, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "error": f"invalid_capability_json:{type(exc).__name__}", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "raw": proc.stdout[:1000], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "agent_ready": False, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| required = { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "tool_usage", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "command_generation", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "filesystem_operations", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "multi_step_reasoning", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "agent_ready", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if not required.issubset(parsed): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "ok": False, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "error": "capability_fields_missing", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "parsed": parsed, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "agent_ready": False, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| parsed["ok"] = bool(parsed.get("agent_ready")) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return parsed | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+74
to
+83
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Capability fields are only checked for presence, not type, so string values like Severity Level: Critical 🚨- ❌ Non-ready models can pass autonomous readiness gate.
- ⚠️ Autonomous execution reliability drops with weak model capability.
Suggested change
Steps of Reproduction ✅1. Trigger autonomous startup via `neurorift_cli.py:51-52` into `run-agent` path in
`neurorift_main.py:1007`.
2. Execute with a model output where capability fields are strings (e.g., `"agent_ready":
"false"`), which is realistic for LLM formatting drift; `verify_model_capabilities` parses
it at `model_capability_check.py:57-58`.
3. Current code only checks key presence (`model_capability_check.py:74`) and then
computes `ok` using `bool(parsed.get("agent_ready"))` (`model_capability_check.py:82`), so
`"false"` is treated truthy.
4. Caller checks raw `capability.get("agent_ready")` at `neurorift_main.py:1019`;
non-empty string bypasses rejection, allowing autonomous run with a non-ready model.Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** model_capability_check.py
**Line:** 74:83
**Comment:**
*Type Error: Capability fields are only checked for presence, not type, so string values like `"false"` are treated as truthy by callers and can incorrectly allow non-ready models to pass. Enforce boolean types for all required fields and compute readiness from a real boolean.
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. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if __name__ == "__main__": | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import argparse | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| parser = argparse.ArgumentParser(description="Check Ollama model capability for NeuroRift agent mode") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| parser.add_argument("--model", required=True) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| args = parser.parse_args() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| print(json.dumps(verify_model_capabilities(args.model), indent=2)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| class ChannelRouter: | ||
| def route(self, msg: dict): return msg |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| class Channel: | ||
| def connect(self): return True | ||
| def receive_message(self, payload=None): return payload or {} | ||
| def send_message(self, message): return message | ||
| def close(self): return True |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| class Channel: | ||
| def connect(self): return True | ||
| def receive_message(self, payload=None): return payload or {} | ||
| def send_message(self, message): return message | ||
| def close(self): return True |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,5 @@ | ||||||
| class Channel: | ||||||
| def connect(self): return True | ||||||
| def receive_message(self, payload=None): return payload or {} | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Using Severity Level: Major
|
||||||
| def receive_message(self, payload=None): return payload or {} | |
| def receive_message(self, payload=None): return {} if payload is None else payload |
Steps of Reproduction ✅
1. Open `neurorift/channels/telegram_channel.py:1-4` and instantiate `Channel` (class
defined at line 1), then call `receive_message("")` on it.
2. Execution reaches `receive_message` at `neurorift/channels/telegram_channel.py:3`,
which evaluates `payload or {}`.
3. Because `""` is falsy, method returns `{}` instead of the original payload, changing
both value and type.
4. This is reproducible in current code and is not isolated: identical logic exists in
`neurorift/channels/cli_channel.py:3`, `discord_channel.py:3`, and `web_channel.py:3`
(verified by search), so the same payload-rewrite behavior appears across all channel
stubs.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/channels/telegram_channel.py
**Line:** 3:3
**Comment:**
*Logic Error: Using `payload or {}` incorrectly rewrites valid falsy payloads (such as `""`, `0`, `[]`, or `False`) into an empty dict, which can break message handling and type expectations at runtime. Only substitute `{}` when the payload is actually `None`.
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.| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| class Channel: | ||
| def connect(self): return True | ||
| def receive_message(self, payload=None): return payload or {} | ||
| def send_message(self, message): return message | ||
| def close(self): return True |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| class ClawHubAPI: | ||
| base_url = 'https://clawhub.example/api' |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,11 @@ | ||||||||||
| from pathlib import Path | ||||||||||
| import shutil | ||||||||||
| class ClawHubClient: | ||||||||||
| def __init__(self, cache_dir: Path): self.cache_dir=cache_dir; cache_dir.mkdir(parents=True, exist_ok=True) | ||||||||||
| def fetch_skill(self, skill_name: str, source_dir: Path) -> Path: | ||||||||||
| src = source_dir / skill_name | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Severity Level: Critical 🚨- ⚠️ ClawHub install reads directories outside skill_store.
- ❌ Unauthorized local folder contents copied into cache.
Suggested change
Steps of Reproduction ✅1. Run CLI install flow with user-controlled argument: `python3 neurorift_main.py
--clawhub ../../..`; `--clawhub` is accepted at `neurorift_main.py:914` and executed at
`neurorift_main.py:983`.
2. Call chain is `main()` (`neurorift_main.py:1472`) → `_async_main()`
(`neurorift_main.py:973`) → `SkillManager.install_clawhub()`
(`neurorift/skills/skill_manager.py:13-14`) → `ClawHubClient.fetch_skill()`
(`neurorift/clawhub/clawhub_client.py:5-6`).
3. At `clawhub_client.py:6`, `src = source_dir / skill_name` allows `..` traversal, so
`src` can resolve outside `self.examples` (`skill_manager.py:12`) without any containment
check.
4. `shutil.copytree(src, dst)` at `clawhub_client.py:10` then copies an out-of-scope local
directory into cache before validation, proving source-directory escape.Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** neurorift/clawhub/clawhub_client.py
**Line:** 6:6
**Comment:**
*Security: `skill_name` is directly joined into the source path without containment checks, so values like `../...` can escape the intended skill directory and copy arbitrary local folders. Resolve the path and verify it still stays under `source_dir` before using it.
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. |
||||||||||
| if not src.exists(): raise FileNotFoundError(skill_name) | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Checking only Severity Level: Major
|
||||||||||
| if not src.exists(): raise FileNotFoundError(skill_name) | |
| if not src.is_dir(): raise FileNotFoundError(skill_name) |
Steps of Reproduction ✅
1. Confirm source store contains a file:
`/workspace/NeuroRift/neurorift/skill_store/installed/__init__.py` (listed by `LS`), not
only skill directories.
2. Run `python3 neurorift_main.py --clawhub __init__.py`; this reaches
`skill_manager.install_clawhub()` at `neurorift/skills/skill_manager.py:13-14`.
3. In `fetch_skill()` (`neurorift/clawhub/clawhub_client.py:7`), `src.exists()` passes for
that file, so no early rejection occurs.
4. `shutil.copytree(src, dst)` at `clawhub_client.py:10` raises `NotADirectoryError`;
there is no surrounding handler in `_async_main`/`main` (`neurorift_main.py:973`, `1472`,
`1521`), so CLI run fails with traceback.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/clawhub/clawhub_client.py
**Line:** 7:7
**Comment:**
*Logic Error: Checking only `exists()` lets regular files pass, and then `shutil.copytree` raises `NotADirectoryError` at runtime. Validate that the source is a directory before copying.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: The destination path is also built from untrusted skill_name, so traversal like ../../... can make rmtree and copytree operate outside the cache directory. Resolve and enforce that the destination remains inside cache_dir. [security]
Severity Level: Critical 🚨
- ❌ Recursive delete may target non-cache directories.
- ❌ Skill copy may overwrite paths outside cache.| dst = self.cache_dir / skill_name | |
| dst = (self.cache_dir / skill_name).resolve() | |
| if dst.parent != self.cache_dir.resolve(): | |
| raise ValueError(f"Invalid skill name: {skill_name}") |
Steps of Reproduction ✅
1. Trigger ClawHub install path with traversal input, e.g. `python3 neurorift_main.py
--clawhub ../../..`; this argument is passed unchanged except `.strip()` at
`neurorift_main.py:983`.
2. The same call chain reaches `ClawHubClient.fetch_skill()`
(`neurorift/skills/skill_manager.py:14` → `neurorift/clawhub/clawhub_client.py:5`).
3. At `clawhub_client.py:8`, `dst` is built with untrusted `skill_name`; with `../../..`,
`dst` points outside `self.cache_dir` (created from `SkillInstaller.cache` at
`skill_manager.py:11`, `installer.py:8-9`).
4. If that external path exists, `shutil.rmtree(dst)` at `clawhub_client.py:9` attempts
recursive deletion outside cache, then `copytree` writes there at line 10.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/clawhub/clawhub_client.py
**Line:** 8:8
**Comment:**
*Security: The destination path is also built from untrusted `skill_name`, so traversal like `../../...` can make `rmtree` and `copytree` operate outside the cache directory. Resolve and enforce that the destination remains inside `cache_dir`.
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.| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| class ClawHubResolver: | ||
| def resolve(self, skill_name: str) -> str: | ||
| return f'https://clawhub.example/skills/{skill_name}' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| """Package-level CLI helper that delegates to global entrypoint.""" | ||
| from __future__ import annotations | ||
|
|
||
| from neurorift_cli import build_parser, install_global_wrapper, main | ||
|
|
||
| __all__ = ["build_parser", "install_global_wrapper", "main"] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| CHANNELS = ['cli','websocket','discord','telegram','api'] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| MODEL_PROVIDERS = ['deepseek','mistral','llama','openai'] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| from dataclasses import dataclass | ||
| from pathlib import Path | ||
| @dataclass | ||
| class Settings: | ||
| data_dir: Path = Path.home()/'.neurorift' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| class AgentLoop: | ||
| async def handle_message(self, session, message: str) -> dict: | ||
| return {"success": True, "response": f"Session {session.session_id}: {message}"} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| from dataclasses import dataclass | ||
| from neurorift.core.agent_loop import AgentLoop | ||
| from neurorift.sessions.session_manager import SessionManager | ||
|
|
||
| @dataclass | ||
| class AgentHandle: | ||
| session_id: str | ||
| user_id: str | ||
| channel: str | ||
|
|
||
| class AgentManager: | ||
| def __init__(self, session_manager: SessionManager, agent_loop: AgentLoop): | ||
| self.session_manager = session_manager | ||
| self.agent_loop = agent_loop | ||
| self.handles: dict[str, AgentHandle] = {} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| from dataclasses import dataclass | ||
|
|
||
| @dataclass | ||
| class PlannedStep: | ||
| order: int | ||
| action: str | ||
|
|
||
| class Planner: | ||
| def create_plan(self, objective: str) -> list[PlannedStep]: | ||
| return [PlannedStep(order=1, action=f"Investigate: {objective}")] if objective else [] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| class TaskRouter: | ||
| def route(self, task: str) -> dict: | ||
| return {"task": task, "route": "agent_loop"} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
|
|
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,5 @@ | ||||||||||||
| import subprocess | ||||||||||||
| class CommandRunner: | ||||||||||||
| def run(self, cmd: list[str]): | ||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Calling Severity Level: Major
|
||||||||||||
| def run(self, cmd: list[str]): | |
| def run(self, cmd: list[str]): | |
| if not cmd: | |
| return {"success": False, "stdout": "", "stderr": "empty command", "exit_code": -1} |
Steps of Reproduction ✅
1. Confirm actual execution surface: `CommandRunner` is defined at
`neurorift/execution/command_runner.py:2-5`, and `SandboxRunner` inherits it at
`neurorift/execution/sandbox_runner.py:1-3`.
2. Verify caller context: repo-wide search shows no current instantiation sites
(`CommandRunner(` has no matches), so direct module invocation is the only current
reproducible path.
3. Run `PYTHONPATH=/workspace/NeuroRift python3 -c "from
neurorift.execution.command_runner import CommandRunner; CommandRunner().run([])"`.
4. Observe crash at `neurorift/execution/command_runner.py:4` with `IndexError: list index
out of range` from `subprocess.run`, so method contract (dict result) is violated.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/execution/command_runner.py
**Line:** 3:3
**Comment:**
*Logic Error: Calling `subprocess.run` with an empty command list raises `IndexError`, so this method can crash instead of returning the structured result object. Add an explicit empty-input check and return a failure payload when no executable is provided.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: If the executable is not installed, subprocess.run raises FileNotFoundError and bypasses the method's dict-based contract, which can break callers expecting a normal result object. Catch this exception and return a failure result with a nonzero exit code. [possible bug]
Severity Level: Major ⚠️
- ❌ Missing tool binary causes unhandled runtime exception.
- ⚠️ Callers cannot reliably process command failure responses.| 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 e: | |
| return {"success": False, "stdout": "", "stderr": str(e), "exit_code": 127} |
Steps of Reproduction ✅
1. Locate failure point at `neurorift/execution/command_runner.py:4`, where
`subprocess.run(...)` is called without exception handling.
2. Confirm structural usage: `SandboxRunner` directly subclasses `CommandRunner`
(`neurorift/execution/sandbox_runner.py:2`), so inherited behavior is unchanged.
3. Execute `PYTHONPATH=/workspace/NeuroRift python3 -c "from
neurorift.execution.command_runner import CommandRunner;
CommandRunner().run(['definitely_missing_binary_12345'])"`.
4. Observe uncaught `FileNotFoundError` from line 4 instead of returned dict payload; this
breaks the method's `{success, stdout, stderr, exit_code}` response contract at line 5.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/execution/command_runner.py
**Line:** 4:4
**Comment:**
*Possible Bug: If the executable is not installed, `subprocess.run` raises `FileNotFoundError` and bypasses the method's dict-based contract, which can break callers expecting a normal result object. Catch this exception and return a failure result with a nonzero exit code.
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.| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| from dataclasses import dataclass | ||
| @dataclass | ||
| class ResourceLimits: | ||
| timeout_seconds:int=30 | ||
| memory_limit_mb:int=512 | ||
| cpu_seconds:int=20 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| import time | ||
| class RetryManager: | ||
| def __init__(self, retries=2): self.retries=retries | ||
| def wait(self, attempt): time.sleep(2**attempt) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| from neurorift.execution.command_runner import CommandRunner | ||
| class SandboxRunner(CommandRunner): | ||
| pass |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| class APIServer: | ||
| def start(self): return True |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| class AuthManager: | ||
| def validate(self, token: str): return bool(token) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| class WebSocketGateway: | ||
| def start(self): return True |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| class LongTermMemory: | ||
| def __init__(self): self.data={} | ||
| def store(self, user_id: str, key: str, value): self.data.setdefault(user_id,{})[key]=value |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| class MemoryCompaction: | ||
| def compact(self, messages: list[str]): return {"summary": " ".join(messages[:10]), "kept": messages[-10:]} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| from collections import defaultdict, deque | ||
| class ShortTermMemory: | ||
| 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) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| class VectorMemory: | ||
| def semantic_search(self, user_id: str, query: str, top_k: int = 5): return [] | ||
| def time_based_retrieval(self, user_id: str, limit: int = 10): return [] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| class ContextBuilder: | ||
| def build(self, session, memories): return {"session": session.session_id, "memories": memories} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| class ModelFailover: | ||
| def pick(self, providers): return providers[0] if providers else "fallback" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| class ModelRouter: | ||
| async def generate(self, prompt: str): return {"response": prompt, "model": "openai", "tokens": len(prompt.split()), "cost": 0.0} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| class PromptBuilder: | ||
| def build(self, message: str, context: dict): return f"{context}\n{message}" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
|
|
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,19 @@ | ||||||||||||||||||||||||||||||||||||||||
| from dataclasses import dataclass, field | ||||||||||||||||||||||||||||||||||||||||
| from enum import Enum | ||||||||||||||||||||||||||||||||||||||||
| from datetime import datetime, timezone | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| class SessionState(str, Enum): | ||||||||||||||||||||||||||||||||||||||||
| CREATED="CREATED"; ACTIVE="ACTIVE"; IDLE="IDLE"; PAUSED="PAUSED"; CLOSED="CLOSED" | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| @dataclass | ||||||||||||||||||||||||||||||||||||||||
| class SessionContext: | ||||||||||||||||||||||||||||||||||||||||
| session_id: str | ||||||||||||||||||||||||||||||||||||||||
| user_id: str | ||||||||||||||||||||||||||||||||||||||||
| channel: str | ||||||||||||||||||||||||||||||||||||||||
| state: SessionState = SessionState.CREATED | ||||||||||||||||||||||||||||||||||||||||
| message_history: list[dict] = field(default_factory=list) | ||||||||||||||||||||||||||||||||||||||||
| tool_usage: list[dict] = field(default_factory=list) | ||||||||||||||||||||||||||||||||||||||||
| context_window: list[str] = field(default_factory=list) | ||||||||||||||||||||||||||||||||||||||||
| memory_references: list[str] = field(default_factory=list) | ||||||||||||||||||||||||||||||||||||||||
| created_at: str = field(default_factory=lambda: datetime.now(timezone.utc).isoformat()) | ||||||||||||||||||||||||||||||||||||||||
| updated_at: str = field(default_factory=lambda: datetime.now(timezone.utc).isoformat()) | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+13
to
+19
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Severity Level: Major
|
||||||||||||||||||||||||||||||||||||||||
| state: SessionState = SessionState.CREATED | |
| message_history: list[dict] = field(default_factory=list) | |
| tool_usage: list[dict] = field(default_factory=list) | |
| context_window: list[str] = field(default_factory=list) | |
| memory_references: list[str] = field(default_factory=list) | |
| created_at: str = field(default_factory=lambda: datetime.now(timezone.utc).isoformat()) | |
| updated_at: str = field(default_factory=lambda: datetime.now(timezone.utc).isoformat()) | |
| state: SessionState = SessionState.CREATED | |
| message_history: list[dict] = field(default_factory=list) | |
| tool_usage: list[dict] = field(default_factory=list) | |
| context_window: list[str] = field(default_factory=list) | |
| memory_references: list[str] = field(default_factory=list) | |
| created_at: str = field(default_factory=lambda: datetime.now(timezone.utc).isoformat()) | |
| updated_at: str = field(default_factory=lambda: datetime.now(timezone.utc).isoformat()) | |
| def __post_init__(self): | |
| if not isinstance(self.state, SessionState): | |
| raw_state = self.state.split(".")[-1] if isinstance(self.state, str) else self.state | |
| self.state = SessionState(raw_state) |
Steps of Reproduction ✅
1. Create and persist a session through `SessionManager.create_session()` at
`neurorift/sessions/session_manager.py:10-12`; this writes `SessionContext.__dict__` using
`SessionStore.save()` at `neurorift/sessions/session_store.py:9`.
2. Simulate a fresh process (empty in-memory cache) and request the same session via
`SessionManager.get_session()` at `neurorift/sessions/session_manager.py:13`; execution
falls back to `SessionStore.load()` when `self.sessions` misses.
3. In `SessionStore.load()` (`neurorift/sessions/session_store.py:12`), JSON is read and
passed directly as `SessionContext(**json.loads(...))`, so `"state": "ACTIVE"` is supplied
as a plain string.
4. Observe contract break in loaded object from `SessionContext`
(`neurorift/sessions/session_context.py:13`): `type(session.state)` is `str`, not
`SessionState`; enum-dependent access (e.g., `.value`) would fail for consumers expecting
annotated enum type.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/sessions/session_context.py
**Line:** 13:19
**Comment:**
*Type Error: `SessionContext` instances loaded from JSON can carry `state` as a plain string, but the dataclass does not normalize it back to `SessionState`. This breaks the class contract and can cause runtime failures when code later treats `state` as an enum (for example accessing enum attributes or relying on strict enum handling). Add a post-init conversion so deserialized values are always converted to `SessionState`.
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.| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,13 @@ | ||||||||||||||||||
| import uuid | ||||||||||||||||||
| from pathlib import Path | ||||||||||||||||||
| from neurorift.sessions.session_context import SessionContext, SessionState | ||||||||||||||||||
| from neurorift.sessions.session_store import SessionStore | ||||||||||||||||||
|
|
||||||||||||||||||
| class SessionManager: | ||||||||||||||||||
| def __init__(self, root: Path): | ||||||||||||||||||
| self.store = SessionStore(root) | ||||||||||||||||||
| self.sessions: dict[str, SessionContext] = {} | ||||||||||||||||||
| def create_session(self, user_id: str, channel: str) -> SessionContext: | ||||||||||||||||||
| sid=str(uuid.uuid4()); s=SessionContext(session_id=sid,user_id=user_id,channel=channel,state=SessionState.ACTIVE) | ||||||||||||||||||
| self.sessions[sid]=s; self.store.save(s); return s | ||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The in-memory session map is updated before persistence. If disk write fails, the method raises but leaves a non-persisted session in memory, creating inconsistent state between cache and storage. Persist first, then cache only after save succeeds. [logic error] Severity Level: Major
|
||||||||||||||||||
| self.sessions[sid]=s; self.store.save(s); return s | |
| self.store.save(s); self.sessions[sid]=s; return s |
Steps of Reproduction ✅
1. Instantiate `SessionManager(root)` using `neurorift/sessions/session_manager.py:7-9`,
which wires `SessionStore` for JSON persistence.
2. Ensure a write failure condition for `SessionStore.save()` at
`neurorift/sessions/session_store.py:9` (e.g., filesystem becomes read-only or I/O error
occurs during `write_text`).
3. Call `SessionManager.create_session(user_id, channel)` at
`neurorift/sessions/session_manager.py:10-12`; line 12 inserts into `self.sessions` before
calling `self.store.save(s)`.
4. Observe `save()` raises, but in-memory map already contains the session
(`self.sessions[sid]`), creating cache/storage inconsistency in the same process.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/sessions/session_manager.py
**Line:** 12:12
**Comment:**
*Logic Error: The in-memory session map is updated before persistence. If disk write fails, the method raises but leaves a non-persisted session in memory, creating inconsistent state between cache and storage. Persist first, then cache only after save succeeds.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Loaded sessions are returned but never inserted into the in-memory map, so repeated lookups for the same ID keep reloading from disk and any in-process mutations on a previously returned object are lost on subsequent calls. Cache the loaded session before returning it. [possible bug]
Severity Level: Major ⚠️
- ⚠️ Repeated `get_session` calls trigger unnecessary disk I/O.
- ⚠️ Session object mutations are not retained in memory.| def get_session(self, sid: str): return self.sessions.get(sid) or self.store.load(sid) | |
| def get_session(self, sid: str): | |
| session = self.sessions.get(sid) | |
| if session is None: | |
| session = self.store.load(sid) | |
| if session is not None: | |
| self.sessions[sid] = session | |
| return session |
Steps of Reproduction ✅
1. Create and persist a session via `create_session()`
(`neurorift/sessions/session_manager.py:10-12`), which writes JSON through
`SessionStore.save()` (`neurorift/sessions/session_store.py:9`).
2. Simulate a fresh process by creating a new `SessionManager(root)`
(`neurorift/sessions/session_manager.py:7-9`) so `self.sessions` is empty.
3. Call `get_session(sid)` twice; current implementation at
`neurorift/sessions/session_manager.py:13` returns `self.store.load(sid)` each time when
cache miss occurs.
4. Verify repeated disk reads (`neurorift/sessions/session_store.py:10-12`) and non-sticky
in-memory mutations: edits on first returned object are absent on second call because
loaded object is never cached.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/sessions/session_manager.py
**Line:** 13:13
**Comment:**
*Possible Bug: Loaded sessions are returned but never inserted into the in-memory map, so repeated lookups for the same ID keep reloading from disk and any in-process mutations on a previously returned object are lost on subsequent calls. Cache the loaded session before returning it.
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.| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| class SessionPruner: | ||
| def prune(self, sessions: dict, max_idle_hours: int = 24) -> list[str]: | ||
| return [] |
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,12 @@ | ||||||||||||||
| import json | ||||||||||||||
| from pathlib import Path | ||||||||||||||
| from neurorift.sessions.session_context import SessionContext | ||||||||||||||
|
|
||||||||||||||
| class SessionStore: | ||||||||||||||
| def __init__(self, root: Path): | ||||||||||||||
| self.root = root; self.root.mkdir(parents=True, exist_ok=True) | ||||||||||||||
| def path(self, sid: str) -> Path: return self.root / f"{sid}.json" | ||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The session identifier is used directly to build a filesystem path, which allows path traversal (for example with Severity Level: Critical 🚨- ❌ Session file access can escape configured storage root.
- ⚠️ `get_session` accepts unsanitized identifiers from callers.
Suggested change
Steps of Reproduction ✅1. Instantiate `SessionManager(root)` via `neurorift/sessions/session_manager.py:7-8`,
which internally constructs `SessionStore`.
2. Call `SessionManager.get_session("../outside")` at
`neurorift/sessions/session_manager.py:13` (this method forwards caller-provided `sid`
directly).
3. `get_session()` calls `SessionStore.load(sid)`
(`neurorift/sessions/session_store.py:10-12`), which calls `path(sid)`
(`session_store.py:8`) with no validation.
4. `path()` builds `self.root / "../outside.json"`, allowing traversal outside the
intended session directory; `load()` then attempts to read that external file.Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** neurorift/sessions/session_store.py
**Line:** 8:8
**Comment:**
*Security: The session identifier is used directly to build a filesystem path, which allows path traversal (for example with `../` or absolute paths) and can read/write files outside the session directory. Validate identifiers and reject path separators or absolute paths before joining.
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. |
||||||||||||||
| 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() else None | ||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| # recon_scanner |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| requests |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| { | ||
| "name": "recon_scanner", | ||
| "version": "1.0", | ||
| "description": "basic reconnaissance scanner", | ||
| "entrypoint": "skill.py", | ||
| "command": "scan", | ||
| "dependencies": ["nmap"] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| def run(target: str = '127.0.0.1'): | ||
| return {'action': 'scan', 'target': target, 'status': 'simulated'} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
|
|
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,24 @@ | ||||||||||||
| import json, shutil | ||||||||||||
| from pathlib import Path | ||||||||||||
| from neurorift.skills.skill_validator import SkillValidator | ||||||||||||
| from neurorift.skills.registry import SkillRegistry | ||||||||||||
|
|
||||||||||||
| class SkillInstaller: | ||||||||||||
| def __init__(self, base: Path): | ||||||||||||
| self.base=base; self.installed=base/'installed'; self.cache=base/'cache' | ||||||||||||
| self.installed.mkdir(parents=True, exist_ok=True); self.cache.mkdir(parents=True, exist_ok=True) | ||||||||||||
| self.validator=SkillValidator(); self.registry=SkillRegistry(base) | ||||||||||||
| 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'] | ||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Parsing Severity Level: Major
|
||||||||||||
| meta=json.loads((pkg/'skill.json').read_text(encoding='utf-8')); name=meta['name'] | |
| try: | |
| meta=json.loads((pkg/'skill.json').read_text(encoding='utf-8')); name=meta['name'] | |
| except (json.JSONDecodeError, KeyError): | |
| return {'success': False, 'error': 'invalid_skill_metadata'} |
Steps of Reproduction ✅
1. Add a package at `neurorift/skill_store/installed/badmeta/` with required files, but
make `skill.json` malformed JSON or omit `name`.
2. Run `python3 neurorift_main.py --clawhub badmeta`; flow enters `_async_main()`
`neurorift_main.py:982-988` and calls `SkillManager.install_clawhub()`
(`skill_manager.py:13-15`).
3. `SkillValidator.validate()` (`skill_validator.py:4-6`) passes because it only checks
file presence, not JSON schema/content.
4. `SkillInstaller.install()` at `installer.py:14` raises `JSONDecodeError`/`KeyError`; no
handler in this path, so `asyncio.run(_async_main(args))` at `neurorift_main.py:1521`
terminates with traceback.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/skills/installer.py
**Line:** 14:14
**Comment:**
*Possible Bug: Parsing `skill.json` and directly indexing `meta['name']` can raise `JSONDecodeError` or `KeyError`, and this exception propagates to the CLI path without handling, crashing install flow instead of returning an error result. Handle malformed JSON and missing fields explicitly.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: The skill name from skill.json is used directly as a filesystem path component, so values like ../outside can escape the installed directory and make rmtree/copytree operate on unintended paths. Validate the name as a single safe path segment before building the destination path. [security]
Severity Level: Critical 🚨
- ❌ `--clawhub` install can write outside installed directory.
- ⚠️ `rmtree` may delete unintended user directories.| dst=self.installed/name | |
| if Path(name).is_absolute() or len(Path(name).parts) != 1 or name in {".", ".."}: | |
| return {'success': False, 'error': 'invalid_skill_name'} | |
| dst=self.installed/name |
Steps of Reproduction ✅
1. Create a skill package under `neurorift/skill_store/installed/evil/` with required
files (`skill_validator.py:3-6` only checks existence), and set `skill.json` name to
`"../escaped_dir"`.
2. Run `python3 neurorift_main.py --clawhub evil`; CLI arg is accepted at
`neurorift_main.py:914`, then `_async_main()` handles it at `neurorift_main.py:982-988`.
3. Call chain is `SkillManager.install_clawhub()`
(`neurorift/skills/skill_manager.py:13-15`) → `SkillInstaller.install()`
(`neurorift/skills/installer.py:11-19`).
4. `installer.py:14-17` reads untrusted `meta['name']`, builds `dst=self.installed/name`
(line 15) without validation, then executes `rmtree/copytree` on that escaped path.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/skills/installer.py
**Line:** 15:15
**Comment:**
*Security: The skill name from `skill.json` is used directly as a filesystem path component, so values like `../outside` can escape the installed directory and make `rmtree`/`copytree` operate on unintended paths. Validate the name as a single safe path segment before building the destination path.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Uninstall uses user-provided skill names directly in a filesystem path, which allows traversal strings like ../../... to target directories outside the skills install root. Reject non-local path segments before computing the removal target. [security]
Severity Level: Critical 🚨
- ❌ `--skill uninstall` can remove non-skill directories.
- ⚠️ Registry updated despite unintended filesystem deletion.| dst=self.installed/name | |
| if Path(name).is_absolute() or len(Path(name).parts) != 1 or name in {".", ".."}: | |
| return {'success': False, 'error': 'invalid_skill_name'} | |
| dst=self.installed/name |
Steps of Reproduction ✅
1. Invoke uninstall from CLI: `python3 neurorift_main.py --skill uninstall ../cache`;
parser accepts skill args at `neurorift_main.py:915`.
2. `_async_main()` routes uninstall at `neurorift_main.py:1000-1003` into
`SkillManager.uninstall()` (`neurorift/skills/skill_manager.py:19`).
3. `SkillInstaller.uninstall()` computes `dst=self.installed/name` at
`neurorift/skills/installer.py:21` directly from user input.
4. If resolved path exists, `installer.py:22` runs `shutil.rmtree(dst)`, allowing deletion
outside `installed/` (e.g., sibling `cache/`).Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/skills/installer.py
**Line:** 21:21
**Comment:**
*Security: Uninstall uses user-provided skill names directly in a filesystem path, which allows traversal strings like `../../...` to target directories outside the skills install root. Reject non-local path segments before computing the removal target.
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.| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,9 @@ | ||||||||||||
| import importlib.util, 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'] | ||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Accessing required metadata keys with direct indexing will raise a runtime exception when a skill package omits Severity Level: Major
|
||||||||||||
| entry=skill_dir/meta['entrypoint'] | |
| entrypoint = meta.get('entrypoint') | |
| if not entrypoint: | |
| return {'error': 'missing entrypoint in skill.json'} | |
| entry = skill_dir / entrypoint |
Steps of Reproduction ✅
1. Use CLI skill flow in `neurorift_main.py` (`_async_main`, lines 91-99 from Grep output)
by running `python3 neurorift_main.py --skill run recon_scanner`; this path calls
`SkillManager.run()` at `neurorift/skills/skill_manager.py:17-18`.
2. Ensure installed skill metadata is incomplete by removing `entrypoint` from
`~/.neurorift/skills/installed/recon_scanner/skill.json` (validator only checks file
presence at `neurorift/skills/skill_validator.py:3-5`, not required JSON keys).
3. Re-run `--skill run recon_scanner`; execution reaches `SkillLoader.run()` at
`neurorift/skills/loader.py:4-6`.
4. Observe crash: `meta['entrypoint']` at `loader.py:6` raises `KeyError`, and
`_async_main` has no try/except around `skill_manager.run` branch, so skill execution
terminates.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/skills/loader.py
**Line:** 6:6
**Comment:**
*Logic Error: Accessing required metadata keys with direct indexing will raise a runtime exception when a skill package omits `entrypoint`, which crashes skill execution instead of returning a controlled error response.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: The import spec and loader are dereferenced without checking for None; invalid module specs will raise runtime exceptions during module loading. [null pointer]
Severity Level: Major ⚠️
- ❌ Invalid entrypoint crashes skill execution flow.
- ⚠️ Users receive traceback instead of structured error JSON.| mod=importlib.util.module_from_spec(spec); spec.loader.exec_module(mod) | |
| if spec is None or spec.loader is None: | |
| return {'error': 'invalid skill entrypoint module'} | |
| mod = importlib.util.module_from_spec(spec) | |
| spec.loader.exec_module(mod) |
Steps of Reproduction ✅
1. Run skill path `--skill run <name>` via `_async_main` (`neurorift_main.py:91-99`),
which always calls `SkillLoader.run` through `SkillManager.run`
(`neurorift/skills/skill_manager.py:18`).
2. Set skill `entrypoint` in `skill.json` to a directory-like target (for example `"."`),
producing a non-loadable module location.
3. At `neurorift/skills/loader.py:7`, `spec_from_file_location(...)` returns `None` for
directory paths (verified in runtime check), but code immediately continues.
4. `module_from_spec(spec)` at `loader.py:8` raises `AttributeError` (`NoneType` loader),
causing unhandled crash instead of controlled error.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/skills/loader.py
**Line:** 8:8
**Comment:**
*Null Pointer: The import spec and loader are dereferenced without checking for `None`; invalid module specs will raise runtime exceptions during module loading.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Checking only attribute existence is insufficient because run may exist but be non-callable, which raises a TypeError when invoked. [type error]
Severity Level: Major ⚠️
- ❌ Broken skills crash `--skill run` command.
- ⚠️ No graceful validation for malformed skill modules.| return mod.run(**kwargs) if hasattr(mod, 'run') else {'error':'missing run()'} | |
| run_fn = getattr(mod, 'run', None) | |
| return run_fn(**kwargs) if callable(run_fn) else {'error': 'missing callable run()'} |
Steps of Reproduction ✅
1. Execute skill command path `python3 neurorift_main.py --skill run recon_scanner`
(`neurorift_main.py:91-99`), which calls `SkillLoader.run` via `SkillManager.run`
(`neurorift/skills/skill_manager.py:18`).
2. Make skill module define non-callable `run`, e.g., `run = "not_callable"` in installed
`skill.py`.
3. Loader condition at `neurorift/skills/loader.py:9` passes because `hasattr(mod, 'run')`
is true.
4. Call `mod.run(**kwargs)` throws `TypeError: 'str' object is not callable`, crashing CLI
skill invocation.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/skills/loader.py
**Line:** 9:9
**Comment:**
*Type Error: Checking only attribute existence is insufficient because `run` may exist but be non-callable, which raises a `TypeError` when invoked.
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.| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| import json | ||
| from pathlib import Path | ||
| class SkillRegistry: | ||
| def __init__(self, root: Path): | ||
| self.path = root / 'registry.json'; root.mkdir(parents=True, exist_ok=True) | ||
| if not self.path.exists(): self.path.write_text(json.dumps({'installed_skills': []}, indent=2), encoding='utf-8') | ||
| def read(self): return json.loads(self.path.read_text(encoding='utf-8')) | ||
| def list(self): return self.read().get('installed_skills', []) | ||
| def add(self, name): | ||
| data=self.read(); skills=data.setdefault('installed_skills', []) | ||
| if name not in skills: skills.append(name) | ||
| self.path.write_text(json.dumps(data, indent=2), encoding='utf-8') | ||
| def remove(self, name): | ||
| data=self.read(); data['installed_skills']=[s for s in data.get('installed_skills',[]) if s!=name] | ||
| self.path.write_text(json.dumps(data, indent=2), encoding='utf-8') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: JSON extraction is fragile because it slices from the first
{to the last}in the whole response. If the model returns a valid JSON object plus any extra brace in trailing commentary, parsing fails and the model is incorrectly rejected. Parse the first valid JSON object instead of using a greedy substring. [logic error]Severity Level: Major⚠️
Steps of Reproduction ✅
Prompt for AI Agent 🤖