-
Notifications
You must be signed in to change notification settings - Fork 0
Add neurorift package skeleton, skill system, runtime & model checks, and secure command runner #40
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?
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
+67
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 a response like Severity Level: Critical 🚨- ❌ Non-agent-ready models can bypass autonomous safety gate.
- ⚠️ run-agent reliability degrades with loosely formatted model JSON.
Suggested change
Steps of Reproduction ✅1. Invoke `neurorift run-agent --model <model>`; parser defines this command in
`neurorift_main.py:938-942`.
2. Execution reaches capability gate in `neurorift_main.py:1019-1023`, which checks `if
not capability.get("agent_ready")`.
3. If model returns JSON like `"agent_ready": "false"`, current code
(`model_capability_check.py:67-83`) accepts field presence only and keeps string value.
4. Non-empty string is truthy, so `neurorift_main.py:1020` treats model as ready and
continues autonomous run incorrectly.Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** model_capability_check.py
**Line:** 67:83
**Comment:**
*Type Error: Capability fields are only checked for presence, not type, so a response like `"agent_ready": "false"` is treated as truthy by callers and can incorrectly pass readiness checks. Validate that all required fields are booleans and compute `ok` from the normalized boolean value.
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 {} | ||
| 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: The message receiver currently uses truthiness ( 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/web_channel.py:3`; `Channel.receive_message()` returns
`payload or {}`, so Python truthiness is applied.
2. Verify current call graph: Grep across `"/workspace/NeuroRift"` finds no
callers/imports of `neurorift.channels.web_channel.Channel` (only channel class
definitions), so this module is currently scaffold code.
3. Reproduce via the public class API directly: in Python, run `from
neurorift.channels.web_channel import Channel; Channel().receive_message("")`.
4. Observe return value is `{}` instead of `""` (same for `0`, `False`, `[]`), confirming
valid falsy payloads are corrupted at `web_channel.py:3`.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: The message receiver currently uses truthiness (`payload or {}`), which silently converts valid falsy payloads like `0`, `""`, `False`, or `[]` into `{}`. This corrupts incoming data and breaks callers that rely on those values. 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 @@ | ||
|
|
| 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` can access paths outside skill store.
- ❌ Arbitrary directory removal possible via destination override.
- ⚠️ Skill installation trust boundary is bypassed.
Suggested change
Steps of Reproduction ✅1. Run CLI with ClawHub install path: `neurorift --clawhub /tmp` (entry argument defined
at `neurorift_main.py:915` and handled at `neurorift_main.py:983-984`).
2. `SkillManager.install_clawhub()` forwards raw user input unchanged at
`neurorift/skills/skill_manager.py:13-14`.
3. `ClawHubClient.fetch_skill()` joins unsanitized `skill_name` directly into paths at
`neurorift/clawhub/clawhub_client.py:6` and `:8`; absolute input overrides
`source_dir`/`cache_dir`.
4. Existing destination is deleted by `shutil.rmtree(dst)` at
`neurorift/clawhub/clawhub_client.py:9`, so unintended filesystem paths can be
removed/copied outside intended skill store.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 used directly in path joins, so values like `../...` or absolute paths can escape `source_dir`/`cache_dir`, allowing unintended directory deletion/copy outside the skill store. Validate and normalize the name before building paths.
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. Execute `neurorift --clawhub __init__.py` (CLI path handled at
`neurorift_main.py:983-984`).
2. `SkillManager.install_clawhub()` passes this value to `fetch_skill()` at
`neurorift/skills/skill_manager.py:14`.
3. In `fetch_skill()`, `src` points to `neurorift/skill_store/installed/__init__.py` (file
confirmed in repository), and `src.exists()` passes at
`neurorift/clawhub/clawhub_client.py:7`.
4. `shutil.copytree(src, dst)` at `neurorift/clawhub/clawhub_client.py:10` raises runtime
error because source is not a directory; no local try/except exists in the `args.clawhub`
branch (`neurorift_main.py:983-990`).Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/clawhub/clawhub_client.py
**Line:** 7:7
**Comment:**
*Possible Bug: Checking only `exists()` allows regular files to pass, but `shutil.copytree` requires a directory and will raise at runtime. Require the source skill path to be 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.| 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: Severity Level: Major
|
||||||||
| return [PlannedStep(order=1, action=f"Investigate: {objective}")] if objective else [] | |
| normalized_objective = objective.strip() | |
| return [PlannedStep(order=1, action=f"Investigate: {normalized_objective}")] if normalized_objective else [] |
Steps of Reproduction ✅
1. Confirm planner entrypoint exists at `neurorift/core/planner.py:8-10`
(`Planner.create_plan`) and currently has no internal callsites (Grep over
`neurorift/**/*.py` found only definition).
2. In a Python shell, run `from neurorift.core.planner import Planner` and call
`Planner().create_plan(" ")`.
3. Execution reaches `neurorift/core/planner.py:10`, where condition `if objective` treats
`" "` as truthy.
4. Observe returned value contains `PlannedStep(order=1, action="Investigate: ")`,
creating a meaningless task instead of `[]`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/core/planner.py
**Line:** 10:10
**Comment:**
*Logic Error: `objective` is only checked for truthiness, so whitespace-only input (for example `" "`) still generates a plan step like `"Investigate: "`. This creates invalid/meaningless plans instead of returning no steps. Normalize the input with `strip()` before the emptiness check.
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} | ||||||||||||||||
|
Comment on lines
+4
to
+5
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 subprocess call has no timeout, so a hung command can block indefinitely and stall the agent runtime. Add a timeout and catch 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, timeout=30) | |
| return {"success": p.returncode == 0, "stdout": p.stdout, "stderr": p.stderr, "exit_code": p.returncode} | |
| except subprocess.TimeoutExpired: | |
| return {"success": False, "stdout": "", "stderr": "Command timed out", "exit_code": 124} |
Steps of Reproduction ✅
1. Confirm implementation context: `CommandRunner.run` at
`neurorift/execution/command_runner.py:3-5` has no timeout, while
`neurorift_main.py:286-353` `_run_command_secure` does enforce timeout.
2. In Python shell, instantiate `CommandRunner` and execute `runner.run(["python3", "-c",
"import time; time.sleep(999999)"])`.
3. The call blocks inside `subprocess.run(...)` at
`neurorift/execution/command_runner.py:4` indefinitely because no `timeout` is passed.
4. Observe no result dictionary is returned until process exits/killed, stalling any
caller thread using this runner.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: The subprocess call has no timeout, so a hung command can block indefinitely and stall the agent runtime. Add a timeout and catch `TimeoutExpired` to return a deterministic failure response instead of hanging forever.
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()) | ||||||||||||||
|
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
|
||||||||||||||
| updated_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 isinstance(self.state, str): | |
| self.state = SessionState(self.state) |
Steps of Reproduction ✅
1. Create a session through `SessionManager.create_session()` at
`neurorift/sessions/session_manager.py:10-12`; it sets `state=SessionState.ACTIVE` and
persists via `SessionStore.save()`.
2. Persistence at `neurorift/sessions/session_store.py:9` writes `session.__dict__` with
`json.dumps(...)`; for `str, Enum` this serializes state as plain JSON string
(`"ACTIVE"`), not enum object.
3. Simulate normal reload path by constructing a fresh `SessionManager` with same root,
then call `get_session(sid)` at `neurorift/sessions/session_manager.py:13`; cache miss
forces `SessionStore.load()` at `neurorift/sessions/session_store.py:10-12`.
4. `load()` builds `SessionContext(**json.loads(...))`
(`neurorift/sessions/session_store.py:12`), so `SessionContext.state` (declared as
`SessionState` at `neurorift/sessions/session_context.py:13`) now contains plain `str`;
type checks like `isinstance(session.state, SessionState)` fail on loaded sessions.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/sessions/session_context.py
**Line:** 19:19
**Comment:**
*Type Error: `SessionStore.load()` reconstructs this dataclass from JSON, which provides `state` as a plain string. Without coercion, the field can silently hold `str` instead of `SessionState`, breaking enum-based usage (for example `.value` access or strict enum checks) at runtime. Normalize string input to `SessionState` in `__post_init__`.
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 session is inserted into the in-memory map before persistence succeeds. If disk write fails, callers will still get a "created" session from memory that was never saved, causing state divergence between memory and storage. Persist first, then cache in memory. [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. Use `SessionManager.create_session()` in `neurorift/sessions/session_manager.py:10-12`
(this class is imported by `neurorift/core/agent_manager.py:3`, so this is intended
runtime infrastructure).
2. Trigger a save-path failure in `SessionStore.save()` at
`neurorift/sessions/session_store.py:9` (for example, monkeypatch `self.store.save` to
raise `OSError` in a test/harness).
3. Execute `create_session`; line 12 inserts into `self.sessions` before calling
`self.store.save`, then save raises.
4. Observe divergence: in-memory map contains session (`self.sessions[sid]`), but
persistent JSON was not written, so a fresh manager cannot load it from disk.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 session is inserted into the in-memory map before persistence succeeds. If disk write fails, callers will still get a "created" session from memory that was never saved, causing state divergence between memory and storage. Persist first, then cache in memory.
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 can contain state as a plain string from JSON deserialization, while the context contract expects a SessionState enum. Returning the raw object can break enum-based logic in callers; normalize state to SessionState before returning. [type error]
Severity Level: Major ⚠️
- ⚠️ Loaded sessions violate `SessionContext.state` enum type contract.
- ⚠️ Enum-only state handling can fail at runtime.| 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 and isinstance(session.state, str): | |
| session.state = SessionState(session.state) | |
| return session |
Steps of Reproduction ✅
1. Create a session via `SessionManager.create_session()` in
`neurorift/sessions/session_manager.py:10-12`; persistence uses
`json.dumps(session.__dict__, default=str)` in `neurorift/sessions/session_store.py:9`.
2. Instantiate a new `SessionManager` and call `get_session()` at
`neurorift/sessions/session_manager.py:13`, which falls through to `SessionStore.load()`
at `neurorift/sessions/session_store.py:10-12`.
3. `SessionStore.load()` reconstructs `SessionContext(**json.loads(...))`
(`session_store.py:12`), so `state` is returned as raw JSON string, not `SessionState`.
4. Confirm type mismatch (`type(session.state) is str`), which violates
`SessionContext.state: SessionState` declared at
`neurorift/sessions/session_context.py:13` and can break enum-specific usage.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: Loaded sessions can contain `state` as a plain string from JSON deserialization, while the context contract expects a `SessionState` enum. Returning the raw object can break enum-based logic in callers; normalize `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: Severity Level: Critical 🚨- ❌ Session load can read files outside storage root.
- ⚠️ Future API session-id endpoints risk path traversal.
- ⚠️ Unauthorized file reads may crash JSON deserialization.
Suggested change
Steps of Reproduction ✅1. Create a `SessionManager` from `neurorift/sessions/session_manager.py:6-13` (it
constructs `SessionStore` at `session_manager.py:8`) with a controlled root like
`/tmp/nr_sessions`.
2. Place a JSON file outside that root, e.g. `/tmp/outside.json`, containing
`SessionContext` keys (`session_id`, `user_id`, `channel`, etc.) matching
`neurorift/sessions/session_context.py:9-19`.
3. Call `SessionManager.get_session("../../outside")` (`session_manager.py:13`), which
delegates to `SessionStore.load()` (`session_store.py:10-12`), then to
`SessionStore.path()` (`session_store.py:8`).
4. Observe `path()` returns `self.root / "../../outside.json"` without boundary
validation, so `load()` reads outside the session root (`session_store.py:12`), proving
traversal.Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** neurorift/sessions/session_store.py
**Line:** 8:8
**Comment:**
*Security: `sid` is used directly to build a filesystem path, so values containing `../` or path separators can escape the session root and read/write arbitrary files. Validate and constrain the resolved path to remain under `self.root` 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. |
||||||||||||||||||||
| 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 | ||||||||||||||||||||
|
Comment on lines
+11
to
+12
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 persisted 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')) | |
| if "state" in data: | |
| data["state"] = SessionContext.__annotations__["state"](data["state"]) | |
| return SessionContext(**data) |
Steps of Reproduction ✅
1. Create and persist a session through `SessionManager.create_session()` at
`neurorift/sessions/session_manager.py:10-12`; this saves a `SessionContext` with enum
`state` via `SessionStore.save()` (`session_store.py:9`).
2. Simulate a fresh process by creating a new `SessionManager` (empty in-memory
`self.sessions` at `session_manager.py:9`) and call `get_session(sid)`
(`session_manager.py:13`), forcing disk load path.
3. `SessionStore.load()` (`session_store.py:10-12`) does `json.loads(...)` then
`SessionContext(**data)` directly, while dataclass `SessionContext.state` is declared
`SessionState` in `session_context.py:13`.
4. Verify loaded object has `type(session.state) is str`, not `SessionState`;
enum-specific usage later (`.value`, strict enum checks) will break type expectations.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: The persisted `state` field is loaded as a plain string, but the session model expects an enum; dataclasses do not coerce this automatically. This can break enum-based logic later (for example `.value` access or strict enum checks), so convert `state` back to the declared enum before creating the 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'] | ||||||||||||||||
|
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 installer can crash when 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 (OSError, json.JSONDecodeError, KeyError): | |
| return {'success': False, 'error': 'invalid skill metadata'} |
Steps of Reproduction ✅
1. Run CLI with `--clawhub <skill_name>`; `_async_main()` in `neurorift_main.py:983-986`
calls `skill_manager.install_clawhub(...)`.
2. `SkillManager.install_clawhub()` at `neurorift/skills/skill_manager.py:13-15` fetches
package and calls `SkillInstaller.install(pkg)`.
3. `SkillInstaller.install()` at `neurorift/skills/installer.py:14` does JSON parse and
`meta['name']` access without try/except.
4. If fetched package has unreadable/invalid `skill.json` or missing `name`,
`json.loads`/`KeyError` propagates and CLI crashes instead of returning expected
`{'success': False, ...}` used by `neurorift_main.py:985-989`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/skills/installer.py
**Line:** 14:14
**Comment:**
*Possible Bug: The installer can crash when `skill.json` is unreadable, invalid JSON, or missing the `name` field, because exceptions are not handled. Return a structured failure result instead of raising, so callers that expect a `{'success': ...}` payload do not fail at runtime.
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 builds a filesystem path directly from user-provided skill name, so traversal input can remove directories outside the skill install root. Resolve and verify the path remains under the installed directory before calling rmtree. [security]
Severity Level: Critical 🚨
- ❌ `--skill uninstall` can remove unintended filesystem directories.
- ⚠️ User-supplied skill name is not path-constrained.| dst=self.installed/name | |
| if dst.exists(): shutil.rmtree(dst) | |
| base_dir = self.installed.resolve() | |
| dst = (self.installed / name).resolve() | |
| if base_dir not in dst.parents: | |
| return {'success': False, 'error': 'invalid skill name'} | |
| if dst.exists(): shutil.rmtree(dst) |
Steps of Reproduction ✅
1. CLI `--skill uninstall <name>` is parsed in `neurorift_main.py:991-1003` and directly
forwards `skill_name` to `skill_manager.uninstall(skill_name)`.
2. `SkillManager.uninstall()` in `neurorift/skills/skill_manager.py:19` passes input
unchanged to `SkillInstaller.uninstall(name)`.
3. `SkillInstaller.uninstall()` at `neurorift/skills/installer.py:21-22` builds
`dst=self.installed/name` and calls `shutil.rmtree(dst)` without containment validation.
4. Supplying traversal/absolute values (for example paths outside install root) can delete
unintended directories if they exist and permissions allow.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/skills/installer.py
**Line:** 21:22
**Comment:**
*Security: Uninstall builds a filesystem path directly from user-provided skill name, so traversal input can remove directories outside the skill install root. Resolve and verify the path remains under the installed directory before calling `rmtree`.
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: Loading Severity Level: Critical 🚨- ❌ `--skill run` crashes on typo or uninstalled skill.
- ⚠️ Users get traceback instead of structured run error.
- ⚠️ Skill CLI flow becomes brittle for normal usage.
Suggested change
Steps of Reproduction ✅1. Trigger the documented CLI path: `neurorift --skill run typo_name`, handled at
`neurorift_main.py:991-999`.
2. `neurorift_main.py:998` calls `SkillManager.run()` at
`neurorift/skills/skill_manager.py:17-18`, passing
`~/.neurorift/skills/installed/typo_name`.
3. `SkillLoader.run()` at `neurorift/skills/loader.py:5` directly reads `skill.json`
without existence check.
4. If skill folder is missing/incomplete, `FileNotFoundError` is raised and bubbles out;
`main()` uses `asyncio.run(_async_main(args))` at `neurorift_main.py:1522` with no
surrounding handler, so CLI terminates with traceback.Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** neurorift/skills/loader.py
**Line:** 5:5
**Comment:**
*Possible Bug: Loading `skill.json` without checking existence causes a hard crash when a skill directory is missing or incomplete (for example, running an uninstalled skill). Return a structured error instead of raising `FileNotFoundError`.
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. |
||||||||||||||||||||||||||||||||
| 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. Code Injection (🔒 Security, 🔴 Critical) - The code dynamically loads and runs a module based on external input without safe checks, risking execution of harmful code from untrusted sources. View in Corgea ↗ More Details🎟️Issue Explanation: The code dynamically loads and runs a module based on external input without safe checks, risking execution of harmful code from untrusted sources. - "entry=skill_dir/meta['entrypoint']" uses external input to locate code, which attackers can manipulate to load malicious modules. - "spec.loader.exec_module(mod)" runs the loaded module's code, allowing injected code to execute and compromise the system. - If "meta['entrypoint']" points to harmful code, this bypasses safeguards and runs unauthorized actions or data leaks. 🪄Fix Explanation: The fix validates the 'entrypoint' from skill metadata to ensure it contains only safe characters and forbids path traversal or special symbols, preventing arbitrary code execution by restricting dynamic imports to known module names. - The entrypoint string is sanitized by checking for emptiness, unwanted characters like "'.'", "'/'", "'\\'", and ensuring it contains only alphanumeric characters and underscores via "entry_name.replace('_','').isalnum()". - Instead of importing from a file path, the code now builds a trusted module name string "neurorift.skills.{entry_name}" and uses "importlib.util.find_spec()" to locate it, avoiding risky path-based imports. - The fix validates the module spec existence and loader presence, returning an error if the module is unknown, thus preventing attempts to load unauthorized code. - Executing the module and calling "run()" proceeds only after these stringent checks, stopping injection via manipulated entrypoints. 💡Important Instructions: Ensure the skill modules are correctly installed or discoverable under the neurorift.skills namespace package for find_spec() to work properly.
Suggested change
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 entrypoint path from Severity Level: Critical 🚨- ❌ `--skill run` can execute arbitrary host Python files.
- ⚠️ Skill sandbox boundary in `skills/installed` is bypassed.
- ⚠️ Malicious skill metadata gains broader filesystem code execution.
Suggested change
Steps of Reproduction ✅1. Use the real CLI run path in `neurorift_main.py:991-999` (`--skill run <name>`), which
calls `SkillManager.run()` at `neurorift/skills/skill_manager.py:17-18`.
2. Install a skill (or reuse `recon_scanner`), then edit
`~/.neurorift/skills/installed/recon_scanner/skill.json` so `entrypoint` is absolute
(`/tmp/payload.py`) or traversal (`../payload.py`).
3. Run `neurorift --skill run recon_scanner`; execution reaches `SkillLoader.run()` at
`neurorift/skills/loader.py:6-8`.
4. Observe `entry=skill_dir/meta['entrypoint']` (`loader.py:6`) allows escape, then
`spec.loader.exec_module(mod)` (`loader.py:8`) executes that external file.Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** neurorift/skills/loader.py
**Line:** 6:6
**Comment:**
*Security: The entrypoint path from `skill.json` is used directly, so absolute paths or `..` segments can escape the skill directory and load arbitrary files. Resolve the path and enforce that it stays inside the installed skill folder before importing.
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. |
||||||||||||||||||||||||||||||||
| spec=importlib.util.spec_from_file_location(f"skill_{meta['name']}", entry) | ||||||||||||||||||||||||||||||||
| mod=importlib.util.module_from_spec(spec); spec.loader.exec_module(mod) | ||||||||||||||||||||||||||||||||
|
Comment on lines
+7
to
+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: The module import path assumes Severity Level: Major
|
||||||||||||||||||||||||||||||||
| 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']}", entry) | |
| if spec is None or spec.loader is None: | |
| return {'error': 'invalid skill entrypoint'} | |
| try: | |
| mod = importlib.util.module_from_spec(spec) | |
| spec.loader.exec_module(mod) | |
| except OSError as exc: | |
| return {'error': f'failed to load skill module: {exc}'} |
Steps of Reproduction ✅
1. Use normal run flow (`neurorift_main.py:996-999`) for any installed skill.
2. Corrupt that skill's metadata/file state: set `entrypoint` in `skill.json` to a bad
file, or remove the target file under `~/.neurorift/skills/installed/<name>/`.
3. Execution reaches `SkillLoader.run()` at `neurorift/skills/loader.py:7-8`, where
`spec`/`loader` are assumed valid and `exec_module` is unguarded.
4. Invalid module load raises uncaught import/file exceptions, which bubble to
`neurorift_main.py:1522` (`asyncio.run`) and terminate the CLI command.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/skills/loader.py
**Line:** 7:8
**Comment:**
*Possible Bug: The module import path assumes `spec` and `spec.loader` are always valid and that execution will succeed, but invalid entrypoint files can make this fail with uncaught exceptions. Validate the spec/loader and catch load errors to prevent CLI termination.
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: Reading the registry assumes valid JSON and will crash with JSONDecodeError if the file is partially written or corrupted. Handle decode failures and return a safe default structure so list/add/remove do not fail at startup. [possible bug] Severity Level: Critical 🚨- ❌ `--skill list` crashes on malformed registry.
- ❌ Install/uninstall paths fail when reading registry.
- ⚠️ Manual registry repair required to recover.
Suggested change
Steps of Reproduction ✅1. Invoke official CLI path (`setup.py:52` console script -> `neurorift_cli.py:52` ->
`neurorift_main.main()` at `neurorift_main.py:1473-1522`) using `--skill list`.
2. Ensure `~/.neurorift/skills/registry.json` contains invalid JSON (e.g., truncated
content), then run `neurorift --skill list`.
3. Execution reaches `skill_manager.list()` (`neurorift_main.py:994`,
`neurorift/skills/skill_manager.py:16`) -> `registry.list()` (`registry.py:8`) ->
`registry.read()` (`registry.py:7`).
4. `json.loads(...)` raises uncaught `JSONDecodeError`, and skill commands terminate
instead of recovering.Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** neurorift/skills/registry.py
**Line:** 7:7
**Comment:**
*Possible Bug: Reading the registry assumes valid JSON and will crash with JSONDecodeError if the file is partially written or corrupted. Handle decode failures and return a safe default structure so list/add/remove do not fail at startup.
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 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') | ||||||||||||||||||||||||||||||
|
Comment on lines
+9
to
+12
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 add operation performs a read-modify-write without synchronization, so concurrent installs can overwrite each other and lose skill entries. Acquire an exclusive file lock around the whole update to make the operation atomic across processes. [race condition] Severity Level: Major
|
||||||||||||||||||||||||||||||
| 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 add(self, name): | |
| lock_path = self.path.with_suffix(".lock") | |
| with lock_path.open("w", encoding="utf-8") as lock_file: | |
| import fcntl | |
| fcntl.flock(lock_file.fileno(), fcntl.LOCK_EX) | |
| 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') |
Steps of Reproduction ✅
1. Use the shipped call chain that reaches `SkillRegistry.add`:
`neurorift_main._async_main()` creates `SkillManager` (`neurorift_main.py:981`),
`SkillManager.install_clawhub()` calls `SkillInstaller.install()`
(`neurorift/skills/skill_manager.py:13-15`), and `install()` calls
`self.registry.add(name)` (`neurorift/skills/installer.py:18`).
2. Start two processes targeting the same registry file
(`~/.neurorift/skills/registry.json`, path created by
`SkillManager(Path.home()/".neurorift")` at `neurorift_main.py:981`), each invoking
`SkillRegistry.add()` with different names.
3. Both processes execute unsynchronized read-modify-write in
`neurorift/skills/registry.py:10-12` (read snapshot, append, overwrite file).
4. Observe one skill missing afterward when listing registry contents (`registry.py:8`),
showing a lost update from concurrent writes.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/skills/registry.py
**Line:** 9:12
**Comment:**
*Race Condition: The add operation performs a read-modify-write without synchronization, so concurrent installs can overwrite each other and lose skill entries. Acquire an exclusive file lock around the whole update to make the operation atomic across processes.
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 remove operation also does unsynchronized read-modify-write, so uninstalling while another process is adding/removing can revert newer changes. Lock the registry update to prevent lost writes. [race condition]
Severity Level: Major ⚠️
- ⚠️ Concurrent uninstalls can revert each other.
- ❌ Removed skills may reappear in registry.
- ⚠️ Skill lifecycle state becomes unreliable.| 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') | |
| def remove(self, name): | |
| lock_path = self.path.with_suffix(".lock") | |
| with lock_path.open("w", encoding="utf-8") as lock_file: | |
| import fcntl | |
| fcntl.flock(lock_file.fileno(), fcntl.LOCK_EX) | |
| 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') |
Steps of Reproduction ✅
1. Follow actual uninstall path: `neurorift_main.py:1002-1004` (`--skill uninstall`) calls
`SkillManager.uninstall()` (`neurorift/skills/skill_manager.py:19`), which calls
`SkillInstaller.uninstall()` (`neurorift/skills/installer.py:20-24`), which calls
`registry.remove(name)` (`installer.py:23`).
2. Prepare registry with two skills, then launch two processes against the same file, both
calling `SkillRegistry.remove()` for different names.
3. Each process reads old data and writes independently via
`neurorift/skills/registry.py:14-15` without lock coordination.
4. Final file may retain one supposedly removed skill (last-writer-wins), proving
concurrent remove/update rollback behavior.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/skills/registry.py
**Line:** 13:15
**Comment:**
*Race Condition: The remove operation also does unsynchronized read-modify-write, so uninstalling while another process is adding/removing can revert newer changes. Lock the registry update to prevent lost writes.
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: JSON extraction is brittle because it slices from the first
{to the last}in the entire response. If the model adds explanatory text containing braces before or after the actual JSON object, parsing fails even when a valid JSON object is present. Decode from each{position until a valid JSON object is found. [logic error]Severity Level: Major⚠️
Steps of Reproduction ✅
Prompt for AI Agent 🤖