-
Notifications
You must be signed in to change notification settings - Fork 0
Add NeuroRift package skeleton, skill system, runtime/model checks, and hardened tool execution #37
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
Changes from 1 commit
6558efb
f4393f6
7736c15
b362984
fcd7962
d5e4cea
d6fc44a
6c97451
b8b8e8d
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
+72
to
+73
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: Converting Severity Level: Critical 🚨- ❌ Non-ready models can pass autonomous safety gate.
- ❌ run-agent may execute with invalid capability contract.
Suggested change
Steps of Reproduction ✅1. Invoke autonomous flow via `run-agent` (parser wiring at `neurorift_main.py:937-940`,
execution gate at `neurorift_main.py:1006-1021`).
2. In `verify_model_capabilities()` (`model_capability_check.py:67-83`), only key presence
is checked (`required.issubset(...)` at lines 67-75), not field types.
3. When model returns JSON like `"agent_ready": "false"` (string), line 82 computes
`bool("false") == True`, so `ok` is incorrectly marked true.
4. `neurorift_main.py:1018` tests `if not capability.get("agent_ready")`; string `"false"`
is truthy, so the rejection branch is skipped and orchestrated agent mode proceeds
(`neurorift_main.py:1066+`) even though readiness value is semantically false.Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** model_capability_check.py
**Line:** 82:83
**Comment:**
*Type Error: Converting `agent_ready` with `bool(...)` treats non-empty strings like `"false"` as `True`, which can incorrectly mark an incapable model as ready. Require `agent_ready` to be a real boolean and then copy that value directly.
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: 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. Inspect `neurorift/channels/telegram_channel.py:3`; `receive_message()` returns
`payload or {}`, so any falsy payload is replaced.
2. Verify current execution context: codebase search found no imports/usages of
`neurorift.channels` and no `Channel(` instantiations (Grep on `*.py` returned no
matches), so this is currently unintegrated skeleton code.
3. Reproduce directly via Python entrypoint: `from neurorift.channels.telegram_channel
import Channel`; then call `Channel().receive_message("")` or
`Channel().receive_message(0)`.
4. Observe returned value is `{}` (not original `""`/`0`), demonstrating data loss for
valid falsy payloads due to line 3 logic.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: `receive_message` currently uses `payload or {}`, which silently replaces valid falsy payloads (such as `""`, `0`, or `False`) with an empty dict. This loses message content and can break downstream logic that expects the original payload. Only default to `{}` 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 {} | ||||||
|
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
|
||||||
| 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. Confirm execution context: `receive_message` is defined at
`neurorift/channels/web_channel.py:3`, and Grep shows no in-repo callers/instantiations
yet (`web_channel`, `Channel(`, and channel imports had no matches).
2. Execute direct runtime call path by importing `Channel` from
`neurorift/channels/web_channel.py` and invoking `Channel.receive_message(...)` (verified
via `python3 -c` in this repo).
3. Pass realistic falsy values (`0`, `False`, `""`, `[]`) to `receive_message`; observed
output is `{}` for each value because `payload or {}` at line 3 treats all falsy values as
missing.
4. This reproduces silent type/data coercion: caller-provided payload is replaced rather
than preserved; only `None` should map to `{}`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/channels/web_channel.py
**Line:** 3:3
**Comment:**
*Logic Error: `receive_message` uses `payload or {}`, which incorrectly converts valid falsy payloads (such as `0`, `False`, `""`, or `[]`) into `{}`. This causes silent data corruption and type changes 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 @@ | ||
|
|
| 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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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: Severity Level: Critical 🚨- ❌ --clawhub can copy directories outside packaged skill_store.
- ⚠️ Unexpected local data enters ~/.neurorift/skills/cache.
Suggested change
Steps of Reproduction ✅1. Run CLI entrypoint `neurorift --clawhub ../../tmp/evilskill` (console script defined at
`setup.py:50-53`, forwarded by `neurorift_cli.py:51-53` into `neurorift_main.main()` at
`neurorift_main.py:1471`).
2. Parser accepts raw `--clawhub` string (`neurorift_main.py:914`) and passes it with only
`.strip()` to `SkillManager.install_clawhub()` (`neurorift_main.py:981-983`).
3. `SkillManager.install_clawhub()` forwards `skill_name` unchanged to
`ClawHubClient.fetch_skill()` (`neurorift/skills/skill_manager.py:13-14`).
4. In `fetch_skill()`, `src = source_dir / skill_name`
(`neurorift/clawhub/clawhub_client.py:6`) allows `..`/absolute-path escape; if escaped
path exists, line 7 passes and line 10 copies that external directory.Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** neurorift/clawhub/clawhub_client.py
**Line:** 6:7
**Comment:**
*Security: `skill_name` is joined directly into `source_dir` without containment checks, so values like `../...` or absolute paths can escape the allowed skill source tree and copy arbitrary local directories. Resolve both paths and enforce that the resolved source stays under `source_dir` 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. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| dst = self.cache_dir / skill_name | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if dst.exists(): shutil.rmtree(dst) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| shutil.copytree(src, dst) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+14
to
+17
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.
Useful? React with 👍 / 👎.
Comment on lines
+14
to
+17
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 destination path is built from untrusted Severity Level: Critical 🚨- ❌ rmtree can delete ~/.neurorift/skills/installed unexpectedly.
- ❌ copytree can overwrite directories outside cache root.
Suggested change
Steps of Reproduction ✅1. Install any skill once via `neurorift --clawhub recon_scanner` so
`~/.neurorift/skills/installed` exists (`SkillInstaller` paths created at
`neurorift/skills/installer.py:8-10`).
2. Run `neurorift --clawhub ../installed`; argument is accepted at `neurorift_main.py:914`
and passed into `SkillManager.install_clawhub()` at `neurorift_main.py:981-983`.
3. `fetch_skill()` computes `dst = self.cache_dir / skill_name`
(`neurorift/clawhub/clawhub_client.py:8`), so `../installed` targets sibling directory
outside cache.
4. Because line 9 executes `shutil.rmtree(dst)` and line 10 executes `shutil.copytree(src,
dst)`, paths outside `cache_dir` can be deleted/overwritten.Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** neurorift/clawhub/clawhub_client.py
**Line:** 8:10
**Comment:**
*Security: The destination path is built from untrusted `skill_name` and then passed to `rmtree`/`copytree`, which allows path traversal to delete or overwrite directories outside the cache. Resolve and validate the destination path is contained within `cache_dir` before any filesystem mutation.
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
+10
to
+17
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. Prevent path traversal/arbitrary delete via
🔒 Suggested hardening patch def fetch_skill(self, skill_name: str, source_dir: Path) -> Path:
- src = source_dir / skill_name
- if not src.exists():
+ if not skill_name or Path(skill_name).name != skill_name:
+ raise ValueError(f"Invalid skill name: {skill_name!r}")
+
+ source_root = source_dir.resolve()
+ cache_root = self.cache_dir.resolve()
+ src = (source_root / skill_name).resolve()
+ dst = (cache_root / skill_name).resolve()
+
+ if source_root not in src.parents:
+ raise ValueError(f"Skill path escapes source_dir: {skill_name!r}")
+ if cache_root not in dst.parents:
+ raise ValueError(f"Skill path escapes cache_dir: {skill_name!r}")
+
+ if not src.is_dir():
raise FileNotFoundError(skill_name)
- dst = self.cache_dir / skill_name📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return dst | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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 [] | ||||||||||||||
|
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 planner interface is incompatible with the existing orchestration contract: other planner implementations are awaited and called with Severity Level: Major
|
||||||||||||||
| def create_plan(self, objective: str) -> list[PlannedStep]: | |
| return [PlannedStep(order=1, action=f"Investigate: {objective}")] if objective else [] | |
| async def create_plan(self, objective: str, available_tools: list[dict] | None = None) -> list[PlannedStep]: | |
| if not objective: | |
| return [] | |
| return [PlannedStep(order=1, action=f"Investigate: {objective}")] |
Steps of Reproduction ✅
1. Confirm orchestration contract in active flow: `neurorift_main.py:90` sets
`self.planner = NRPlanner(self.ollama)`, and `neurorift_main.py:1113` executes `requests =
await vf.planner.create_plan(task_desc, available_tools)`.
2. Confirm planner API contract definition used by active implementation:
`modules/ai/agents.py:11` defines `async def create_plan(self, task: str, available_tools:
List[Dict])`.
3. Confirm new skeleton planner differs: `neurorift/core/planner.py:9-10` defines
synchronous `def create_plan(self, objective: str)` with only one parameter.
4. When this new `Planner` is wired into the same planner slot (the existing awaited call
site in `neurorift_main.py:1113` or `modules/web/ui/orchestration_view.py:106`), runtime
fails immediately with argument-count mismatch and/or await-on-non-coroutine behavior.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/core/planner.py
**Line:** 9:10
**Comment:**
*Logic Error: The planner interface is incompatible with the existing orchestration contract: other planner implementations are awaited and called with `(task, available_tools)`, but this method is synchronous and only accepts one argument. When this class is wired into the agent flow, it will fail with a wrong-arguments error or an await-on-non-coroutine type error. Make this method async and accept the tool list parameter (even if unused for now) to keep runtime compatibility.
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 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]): | ||||||||||||||||
| p=subprocess.run(cmd, capture_output=True, text=True, check=False) | ||||||||||||||||
| return {"success": p.returncode==0, "stdout": p.stdout, "stderr": p.stderr, "exit_code": p.returncode} | ||||||||||||||||
|
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: If the executable does not exist, Severity Level: Major
|
||||||||||||||||
| p=subprocess.run(cmd, capture_output=True, text=True, check=False) | |
| return {"success": p.returncode==0, "stdout": p.stdout, "stderr": p.stderr, "exit_code": p.returncode} | |
| try: | |
| p = subprocess.run(cmd, capture_output=True, text=True, check=False) | |
| return {"success": p.returncode == 0, "stdout": p.stdout, "stderr": p.stderr, "exit_code": p.returncode} | |
| except FileNotFoundError as exc: | |
| return {"success": False, "stdout": "", "stderr": str(exc), "exit_code": 127} |
Steps of Reproduction ✅
1. Trace usage context: `CommandRunner` is defined in
`neurorift/execution/command_runner.py:2-5` and only inherited by `SandboxRunner`
(`neurorift/execution/sandbox_runner.py:1-3`); no in-repo caller handles exceptions for
this method.
2. Call the method with a missing binary:
`CommandRunner().run(["definitely_missing_binary_xyz"])`.
3. Execution hits `subprocess.run(...)` at `neurorift/execution/command_runner.py:4`.
4. Observe uncaught `FileNotFoundError` (verified), so callers receive a crash instead of
the method's expected `{"success": ..., "stderr": ...}` payload shape.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/execution/command_runner.py
**Line:** 4:5
**Comment:**
*Possible Bug: If the executable does not exist, `subprocess.run` raises `FileNotFoundError` and this method currently propagates that exception. Catch it and return a failed result payload so callers get consistent behavior instead of a crash.
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) | ||||||||||
|
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 backoff delay is computed directly from Severity Level: Major
|
||||||||||
| def wait(self, attempt): time.sleep(2**attempt) | |
| def wait(self, attempt): | |
| capped_attempt = max(0, min(attempt, self.retries)) | |
| time.sleep(2 ** capped_attempt) |
Steps of Reproduction ✅
1. Confirm `RetryManager.wait()` implementation at
`neurorift/execution/retry_manager.py:4` directly executes `time.sleep(2**attempt)` with
no clamp to `self.retries` (`__init__` is at line 3).
2. Confirm there are currently no in-repo callers (`Grep` for `RetryManager(` and `from
neurorift.execution.retry_manager` returns no matches), so reproduce via the public class
API directly.
3. In a Python shell, run: `from neurorift.execution.retry_manager import RetryManager;
RetryManager(retries=2).wait(20)`.
4. Observe the call blocks for `2**20` seconds (~12 days), demonstrating `retries=2` is
ignored and a large `attempt` causes runaway delay.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/execution/retry_manager.py
**Line:** 4:4
**Comment:**
*Logic Error: The backoff delay is computed directly from `attempt` without any upper bound, so large values can cause extremely long blocking sleeps (or overflow on some platforms) and the configured retry limit is ignored. Clamp the attempt used for delay to `self.retries` (and non-negative) before sleeping.
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 @@ | ||
| 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)) | ||||||||||||
|
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 constructor accepts any integer for the buffer size, but Severity Level: Major
|
||||||||||||
| def __init__(self, limit: int = 30): self.buffers=defaultdict(lambda: deque(maxlen=limit)) | |
| def __init__(self, limit: int = 30): | |
| if limit <= 0: | |
| raise ValueError("limit must be a positive integer") | |
| self.buffers = defaultdict(lambda: deque(maxlen=limit)) |
Steps of Reproduction ✅
1. Verify actual call graph: `Grep` over `/workspace/NeuroRift/**/*.py` shows
`ShortTermMemory` only in `neurorift/memory/short_term_memory.py:2-4` (no in-repo
callers), so reproduction is direct instantiation of this module.
2. Run `PYTHONPATH=/workspace/NeuroRift python3` and execute `from
neurorift.memory.short_term_memory import ShortTermMemory; stm = ShortTermMemory(-1)`.
This succeeds because the `defaultdict` factory at `short_term_memory.py:3` is lazy.
3. Execute `stm.add("s1", "hello")`; `add()` at `short_term_memory.py:4` accesses
`self.buffers[sid]`, triggering the lambda at line 3 and raising `ValueError: maxlen must
be non-negative` from `deque(maxlen=limit)`.
4. Execute `stm0 = ShortTermMemory(0); stm0.add("s1","hello"); len(stm0.buffers["s1"])`
and observe length `0` (message silently discarded), demonstrating data loss behavior from
zero-capacity deque.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/memory/short_term_memory.py
**Line:** 3:3
**Comment:**
*Logic Error: The constructor accepts any integer for the buffer size, but `deque(maxlen=limit)` fails for negative values and a zero limit silently drops all messages. Validate that the limit is positive so invalid configuration fails early with a clear error.
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 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 | ||||||||||||||
|
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 | |
| state: SessionState = SessionState.CREATED | |
| def __post_init__(self): | |
| if not isinstance(self.state, SessionState): | |
| self.state = SessionState(self.state) |
Steps of Reproduction ✅
1. Create a persisted session through `SessionManager.create_session()` at
`neurorift/sessions/session_manager.py:10-12`; it stores JSON via `SessionStore.save()` at
`neurorift/sessions/session_store.py:9`.
2. Load that same session through `SessionManager.get_session()` at
`neurorift/sessions/session_manager.py:13`, which calls `SessionStore.load()` at
`neurorift/sessions/session_store.py:10-12`.
3. `SessionStore.load()` reconstructs with `SessionContext(**json.loads(...))`
(`neurorift/sessions/session_store.py:12`), so `state` is passed as plain JSON string.
4. `SessionContext` currently has no `__post_init__` validation
(`neurorift/sessions/session_context.py:8-19`), so loaded objects can carry `state` as
`str` instead of `SessionState`, violating the dataclass contract immediately after
deserialization.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/sessions/session_context.py
**Line:** 13:13
**Comment:**
*Type Error: `state` is annotated as an enum, but sessions loaded from JSON pass a plain string into this dataclass, so the object can carry an invalid/non-enum state and break enum-based logic (for example code expecting enum behavior). Coerce and validate `state` in `__post_init__` so deserialized sessions always have a real `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 | ||||||||||||||
| def get_session(self, sid: str): return self.sessions.get(sid) or self.store.load(sid) | ||||||||||||||
|
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: Sessions loaded from JSON can carry Severity Level: Major
|
||||||||||||||
| 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) or self.store.load(sid) | |
| if session is not None and isinstance(session.state, str): | |
| session.state = SessionState(session.state) | |
| return session |
Steps of Reproduction ✅
1. Create and save a session through `SessionManager.create_session()` at
`neurorift/sessions/session_manager.py:10-12`; persistence uses `json.dumps(...,
default=str)` in `neurorift/sessions/session_store.py:9`.
2. Create a fresh `SessionManager` instance and call `get_session(sid)`
(`neurorift/sessions/session_manager.py:13`) so it loads from disk via `SessionStore.load`
(`neurorift/sessions/session_store.py:10-12`).
3. `SessionStore.load` builds `SessionContext(**json.loads(...))` (`session_store.py:12`),
so `state` remains a plain string even though `SessionContext.state` is typed
`SessionState` (`neurorift/sessions/session_context.py:13`).
4. Any caller expecting enum behavior (e.g., `.state.value`) fails with `AttributeError`
(validated by runtime repro: `loaded_state_type str`, `value_exc AttributeError`).Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/sessions/session_manager.py
**Line:** 13:13
**Comment:**
*Type Error: Sessions loaded from JSON can carry `state` as a plain string, but this API is typed to return `SessionContext` with `SessionState`; leaving it as `str` can break downstream code that expects enum behavior (for example accessing `.value`). Normalize loaded state to `SessionState` 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.| 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, so a crafted identifier containing path separators (for example Severity Level: Critical 🚨- ❌ Arbitrary file read via traversed `get_session` identifier.
- ❌ Arbitrary file overwrite via crafted `session_id`.
- ⚠️ Session storage boundary isolation is bypassed.
Suggested change
Steps of Reproduction ✅1. Instantiate `SessionManager` from `neurorift/sessions/session_manager.py:6-9`, which
internally creates `SessionStore` and uses `SessionStore.path()`
(`neurorift/sessions/session_store.py:8`) for filesystem paths.
2. Call `SessionManager.get_session("../outside")`
(`neurorift/sessions/session_manager.py:13`), which forwards attacker-controlled `sid`
into `SessionStore.load()` (`neurorift/sessions/session_store.py:10-12`).
3. `load()` calls `path(sid)` (`neurorift/sessions/session_store.py:8`) and builds
`self.root / "../outside.json"` without validation, allowing path traversal outside the
configured session root.
4. If the traversed file exists, `load()` reads it via `p.read_text(...)`
(`neurorift/sessions/session_store.py:12`); similarly, `save()`
(`neurorift/sessions/session_store.py:9`) can overwrite traversed targets when
`session.session_id` is crafted.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, so a crafted identifier containing path separators (for example `../`) can escape the session directory and overwrite/read arbitrary files. Validate the identifier before joining it into the 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. |
||||||||||||||||||||
| 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 | ||||||||||||||||||||
|
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: Persisted state is loaded as a plain string and passed into the dataclass without conversion, which breaks the declared enum contract and can cause runtime failures when code later expects enum behavior. Convert the serialized state back to Severity Level: Major
|
||||||||||||||||||||
| p=self.path(sid) | |
| return SessionContext(**json.loads(p.read_text(encoding='utf-8'))) if p.exists() else None | |
| p = self.path(sid) | |
| if not p.exists(): | |
| return None | |
| data = json.loads(p.read_text(encoding='utf-8')) | |
| from neurorift.sessions.session_context import SessionState | |
| data["state"] = SessionState(data["state"]) | |
| return SessionContext(**data) |
Steps of Reproduction ✅
1. Create a session through `SessionManager.create_session()`
(`neurorift/sessions/session_manager.py:10-12`), which persists `session.__dict__` via
`SessionStore.save()` (`neurorift/sessions/session_store.py:9`).
2. Request the same session after cache miss/restart through
`SessionManager.get_session()` (`neurorift/sessions/session_manager.py:13`), which calls
`SessionStore.load()` (`neurorift/sessions/session_store.py:10-12`).
3. `load()` deserializes JSON and directly constructs `SessionContext(**data)`
(`neurorift/sessions/session_store.py:12`) without converting `data["state"]` back to
`SessionState` from `neurorift/sessions/session_context.py:5-13`.
4. Returned object has `session.state` as plain string, not enum instance, violating
`SessionContext.state: SessionState` contract defined at
`neurorift/sessions/session_context.py:13`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/sessions/session_store.py
**Line:** 11:12
**Comment:**
*Type Error: Persisted state is loaded as a plain string and passed into the dataclass without conversion, which breaks the declared enum contract and can cause runtime failures when code later expects enum behavior. Convert the serialized state back to `SessionState` before creating the session object.
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 @@ | ||
|
|
| 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'] | ||||||||||||||||||||||||||||
| 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)} | ||||||||||||||||||||||||||||
|
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 install destination uses Severity Level: Critical 🚨- ❌ Crafted metadata can overwrite folders outside skills directory.
- ⚠️ Installed skill path in response becomes attacker-controlled.
Suggested change
Steps of Reproduction ✅1. Create a local skill package under `neurorift/skill_store/installed/evil/` with
required files (`skill_validator.py` only checks existence at
`neurorift/skills/skill_validator.py:3-6`) and set `skill.json` `"name"` to
`../../../../tmp/pwn`.
2. Execute `python /workspace/NeuroRift/neurorift_main.py --clawhub evil` (`--clawhub`
argument wired at `neurorift_main.py:914`).
3. `_async_main` calls `skill_manager.install_clawhub(args.clawhub.strip())` at
`neurorift_main.py:981-983`; `SkillManager` then calls `installer.install(pkg)` at
`neurorift/skills/skill_manager.py:13-15`.
4. `installer.install` reads metadata name and uses it as path
(`neurorift/skills/installer.py:14-17`), so copy/remove operations target a traversal path
outside installed-skills root.Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** neurorift/skills/installer.py
**Line:** 15:19
**Comment:**
*Security: The install destination uses `name` from `skill.json` without sanitization, so a malicious package can set a traversal/absolute value and cause overwrite or deletion outside the intended install root. Validate and normalize the name before using it as a filesystem 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. |
||||||||||||||||||||||||||||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The uninstall path is built directly from user-controlled skill name, so inputs like Severity Level: Critical 🚨- ❌ `--skill uninstall` can delete arbitrary user directories.
- ⚠️ Skill registry may desync from actual filesystem state.
Suggested change
Steps of Reproduction ✅1. Run CLI uninstall entrypoint using user-provided name: `python
/workspace/NeuroRift/neurorift_main.py --skill uninstall ../../../../tmp/demo` (`--skill`
defined at `neurorift_main.py:915`).
2. `_async_main` forwards raw `args.skill[1]` to `skill_manager.uninstall(skill_name)` at
`neurorift_main.py:999-1001`.
3. `SkillManager.uninstall` directly delegates at `neurorift/skills/skill_manager.py:19`
to `SkillInstaller.uninstall(name)`.
4. `SkillInstaller.uninstall` builds `dst=self.installed/name` and executes
`shutil.rmtree(dst)` at `neurorift/skills/installer.py:21-22`, allowing traversal outside
`~/.neurorift/skills/installed`.Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** neurorift/skills/installer.py
**Line:** 21:24
**Comment:**
*Security: The uninstall path is built directly from user-controlled skill name, so inputs like `../../...` or absolute paths can escape the installed-skills directory and delete arbitrary folders via `shutil.rmtree`. Reject non-basename skill names before constructing 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. |
||||||||||||||||||||||||||||
| 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')) | ||||||||||||||
|
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: Running a non-installed or incomplete skill raises Severity Level: Major
|
||||||||||||||
| meta=json.loads((skill_dir/'skill.json').read_text(encoding='utf-8')) | |
| meta_path = skill_dir / 'skill.json' | |
| if not meta_path.exists(): | |
| return {'error':'skill not installed or missing skill.json'} | |
| meta=json.loads(meta_path.read_text(encoding='utf-8')) |
Steps of Reproduction ✅
1. Invoke skill run command with unknown name using parser flow in
`neurorift_main.py:989-997` (e.g., `--skill run does_not_exist`).
2. `SkillManager.run()` at `neurorift/skills/skill_manager.py:17-18` builds path
`~/.neurorift/skills/installed/does_not_exist` and calls `SkillLoader.run()`.
3. `SkillLoader.run()` immediately reads `(skill_dir/'skill.json')` at
`neurorift/skills/loader.py:5`; missing file raises `FileNotFoundError`.
4. No guard around this call path; top-level `asyncio.run(_async_main(args))` at
`neurorift_main.py:1520` bubbles exception and CLI aborts.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/skills/loader.py
**Line:** 5:5
**Comment:**
*Possible Bug: Running a non-installed or incomplete skill raises `FileNotFoundError` because `skill.json` is read unconditionally, which crashes the CLI path instead of returning a structured error. Check for the metadata file before reading 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.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: Module spec creation is not validated; invalid entrypoint targets can produce None spec/loader and then crash with AttributeError during exec_module. Guard against invalid specs before loading. [null pointer]
Severity Level: Major ⚠️
- ❌ Malformed `entrypoint` crashes skill execution command.
- ⚠️ Developer-authored skills fail without actionable loader errors.| spec=importlib.util.spec_from_file_location(f"skill_{meta['name']}", entry) | |
| mod=importlib.util.module_from_spec(spec); spec.loader.exec_module(mod) | |
| spec=importlib.util.spec_from_file_location(f"skill_{meta['name']}", str(entry)) | |
| if spec is None or spec.loader is None: | |
| return {'error':'invalid skill entrypoint'} | |
| mod=importlib.util.module_from_spec(spec); spec.loader.exec_module(mod) |
Steps of Reproduction ✅
1. Use normal skill execution path `neurorift_main.py:994-997` ->
`neurorift/skills/skill_manager.py:17-18`.
2. Set installed skill metadata `skill.json` `entrypoint` to an invalid target like
`"skill"` (no loadable module path).
3. `spec_from_file_location(...)` in `neurorift/skills/loader.py:7` can return `None` for
such targets.
4. Line 8 dereferences spec immediately (`module_from_spec(spec)` /
`spec.loader.exec_module`), causing runtime crash instead of returning structured error.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/skills/loader.py
**Line:** 7:8
**Comment:**
*Null Pointer: Module spec creation is not validated; invalid entrypoint targets can produce `None` spec/loader and then crash with `AttributeError` during `exec_module`. Guard against invalid specs before 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: The code only checks for attribute presence and then calls it, so a non-callable run attribute triggers TypeError. Verify callability before invocation. [type error]
Severity Level: Major ⚠️
- ❌ Skill run crashes when `run` symbol is non-callable.
- ⚠️ Poor diagnostics for broken or incomplete skills.| 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. Skill installation validation (`neurorift/skills/skill_validator.py:3-6`) only checks
required files exist, not that `run` is callable.
2. Run path `neurorift_main.py:994-997` -> `SkillManager.run()`
(`neurorift/skills/skill_manager.py:17-18`) loads module.
3. If module defines `run` as non-callable (e.g., `run = "v1"`), `hasattr(mod, 'run')` at
`neurorift/skills/loader.py:9` is true.
4. Loader then executes `mod.run(**kwargs)` and raises `TypeError`, aborting command
instead of returning structured loader error.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/skills/loader.py
**Line:** 9:9
**Comment:**
*Type Error: The code only checks for attribute presence and then calls it, so a non-callable `run` attribute triggers `TypeError`. Verify callability before invocation.
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')) | ||||||||||||||||||
|
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
|
||||||||||||||||||
| def read(self): return json.loads(self.path.read_text(encoding='utf-8')) | |
| def read(self): | |
| try: | |
| data = json.loads(self.path.read_text(encoding='utf-8')) | |
| except (FileNotFoundError, json.JSONDecodeError): | |
| data = {'installed_skills': []} | |
| self.path.write_text(json.dumps(data, indent=2), encoding='utf-8') | |
| return data |
Steps of Reproduction ✅
1. Run the CLI entrypoint `neurorift` (wired in `setup.py:50-53` to `neurorift_cli:main`,
then forwarded to `neurorift_main.main()` at `neurorift_cli.py:50-53`).
2. Execute `neurorift --skill list`; `_async_main()` in `neurorift_main.py:989-993` calls
`skill_manager.list()`.
3. `SkillManager.list()` in `neurorift/skills/skill_manager.py:16` calls
`self.installer.registry.list()`, and `SkillRegistry.list()` in
`neurorift/skills/registry.py:8` calls `read()` at line 7.
4. If `~/.neurorift/skills/registry.json` is missing or contains invalid JSON (e.g.,
truncated file after interrupted write from `registry.py:12`/`registry.py:15`),
`json.loads(...)` at `registry.py:7` raises and is not caught in this call chain, so the
skill CLI path exits with an uncaught exception.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/skills/registry.py
**Line:** 7:7
**Comment:**
*Possible Bug: `read()` can raise `FileNotFoundError` or `JSONDecodeError` (for example after an interrupted write), and that exception is not handled by callers like `skill_manager.list()`, so CLI skill commands can crash. Return a safe default registry and recreate the file when parsing fails.
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 JSON extraction uses the first
{and last}in the whole response, so any extra brace text before/after the actual JSON object can make parsing fail even when the model returned valid JSON. Parse the first decodable JSON object instead of slicing by global brace positions. [logic error]Severity Level: Major⚠️
Steps of Reproduction ✅
Prompt for AI Agent 🤖