-
Notifications
You must be signed in to change notification settings - Fork 0
Add NeuroRift package scaffold, runtime & model checks, secure command runner, CLI and skill system #39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add NeuroRift package scaffold, runtime & model checks, secure command runner, CLI and skill system #39
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, | ||||||||||||
|
Comment on lines
+82
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: Severity Level: Critical 🚨- ❌ `run-agent` readiness gate bypassed by string booleans.
- ⚠️ Autonomous mode may run with unsupported models.
Suggested change
Steps of Reproduction ✅1. Start from the real gate path: `run-agent` in `_async_main`
(`neurorift_main.py:1007-1023`) calls `verify_model_capabilities()` at
`neurorift_main.py:1018`.
2. Return a capability JSON object where required keys exist but `agent_ready` is a
string, e.g. `"agent_ready": "false"` (parsed in `model_capability_check.py:58`, required
fields validated at `model_capability_check.py:67-80`).
3. Current code sets `parsed["ok"] = bool(parsed.get("agent_ready"))`
(`model_capability_check.py:82`), which converts non-empty `"false"` to `True`.
4. `run-agent` then checks `if not capability.get("agent_ready")`
(`neurorift_main.py:1019`); string `"false"` is truthy, so the failure branch is skipped
and agent runtime proceeds incorrectly.Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** model_capability_check.py
**Line:** 82:83
**Comment:**
*Logic Error: `agent_ready` is used as a truthy value without enforcing boolean type, so a model returning `"false"` (string) is treated as ready by callers because non-empty strings are truthy. Normalize `agent_ready` to a strict boolean before returning.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. |
||||||||||||
| "agent_ready": False, | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| parsed["ok"] = bool(parsed.get("agent_ready")) | ||||||||||||
| return parsed | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| 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 {} | ||
| def send_message(self, message): return message | ||
| def close(self): return True |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| class ClawHubAPI: | ||
| base_url = 'https://clawhub.example/api' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| from pathlib import Path | ||
| import shutil | ||
| class ClawHubClient: | ||
| def __init__(self, cache_dir: Path): self.cache_dir=cache_dir; cache_dir.mkdir(parents=True, exist_ok=True) | ||
| def fetch_skill(self, skill_name: str, source_dir: Path) -> Path: | ||
| src = source_dir / skill_name | ||
| if not src.exists(): raise FileNotFoundError(skill_name) | ||
| dst = self.cache_dir / skill_name | ||
| if dst.exists(): shutil.rmtree(dst) | ||
| shutil.copytree(src, dst) | ||
|
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. Reject traversal in
Suggested fix def fetch_skill(self, skill_name: str, source_dir: Path) -> Path:
- src = source_dir / skill_name
- if not src.exists():
+ src_root = source_dir.resolve()
+ dst_root = self.cache_dir.resolve()
+ src = (src_root / skill_name).resolve()
+ dst = (dst_root / skill_name).resolve()
+ if src_root not in src.parents or dst_root not in dst.parents:
+ raise ValueError("invalid skill name")
+ if not src.is_dir():
raise FileNotFoundError(skill_name)
- dst = self.cache_dir / skill_name
if dst.exists():
shutil.rmtree(dst)
shutil.copytree(src, dst)
return dst🤖 Prompt for AI Agents |
||
| 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 [] |
| 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} |
| 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 | ||||||||||||||
|
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) -> None: | |
| if isinstance(self.state, str): | |
| self.state = SessionState(self.state) |
Steps of Reproduction ✅
1. Create and persist a session through `SessionManager.create_session()` at
`neurorift/sessions/session_manager.py:10-12`; this calls `SessionStore.save()` at
`neurorift/sessions/session_store.py:9`.
2. Simulate a new process (new `SessionManager` instance) and call
`SessionManager.get_session()` at `neurorift/sessions/session_manager.py:13`; this falls
through to `SessionStore.load()` at `neurorift/sessions/session_store.py:10-12`.
3. `SessionStore.load()` reconstructs via `SessionContext(**json.loads(...))`
(`neurorift/sessions/session_store.py:12`), while `SessionContext` has no `__post_init__`
normalization (`neurorift/sessions/session_context.py:8-19`), so `state` remains plain
`str`.
4. Access enum semantics on the loaded object (for example `loaded.state.name`) and
observe runtime failure: `AttributeError: 'str' object has no attribute 'name'` (verified
by executing this exact call chain with `PYTHONPATH=/workspace/NeuroRift`).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()` rebuilds this dataclass from JSON, where `state` comes back as a plain string. Without normalization, the object violates its own contract (`SessionState`) and will break when enum behavior is expected (for example accessing enum attributes). Coerce 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 | ||
| def get_session(self, sid: str): return self.sessions.get(sid) or self.store.load(sid) |
| 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" | ||
| def save(self, session: SessionContext): self.path(session.session_id).write_text(json.dumps(session.__dict__, default=str), encoding='utf-8') | ||
| def load(self, sid: str): | ||
| p=self.path(sid) | ||
| return SessionContext(**json.loads(p.read_text(encoding='utf-8'))) if p.exists() else None |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| # recon_scanner |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| requests |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| { | ||
| "name": "recon_scanner", | ||
| "version": "1.0", | ||
| "description": "basic reconnaissance scanner", | ||
| "entrypoint": "skill.py", | ||
| "command": "scan", | ||
| "dependencies": ["nmap"] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| def run(target: str = '127.0.0.1'): | ||
| return {'action': 'scan', 'target': target, 'status': 'simulated'} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| import json, shutil | ||
| from pathlib import Path | ||
| from neurorift.skills.skill_validator import SkillValidator | ||
| from neurorift.skills.registry import SkillRegistry | ||
|
|
||
| class SkillInstaller: | ||
| def __init__(self, base: Path): | ||
| self.base=base; self.installed=base/'installed'; self.cache=base/'cache' | ||
| self.installed.mkdir(parents=True, exist_ok=True); self.cache.mkdir(parents=True, exist_ok=True) | ||
| self.validator=SkillValidator(); self.registry=SkillRegistry(base) | ||
| def install(self, pkg: Path): | ||
| ok,missing=self.validator.validate(pkg) | ||
| if not ok: return {'success': False, 'error': f'missing:{missing}'} | ||
| meta=json.loads((pkg/'skill.json').read_text(encoding='utf-8')); name=meta['name'] | ||
| dst=self.installed/name | ||
| if dst.exists(): shutil.rmtree(dst) | ||
|
Comment on lines
+14
to
+16
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.
The installer trusts Useful? React with 👍 / 👎. |
||
| shutil.copytree(pkg,dst) | ||
| self.registry.add(name) | ||
| return {'success': True, 'name': name, 'path': str(dst)} | ||
| def uninstall(self, name: str): | ||
| dst=self.installed/name | ||
| if dst.exists(): shutil.rmtree(dst) | ||
| self.registry.remove(name) | ||
| return {'success': True, 'name': name} | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,9 @@ | ||||||||||||||||||||
| import importlib.util, json | ||||||||||||||||||||
| from pathlib import Path | ||||||||||||||||||||
| class SkillLoader: | ||||||||||||||||||||
| def run(self, skill_dir: Path, **kwargs): | ||||||||||||||||||||
| meta=json.loads((skill_dir/'skill.json').read_text(encoding='utf-8')) | ||||||||||||||||||||
| entry=skill_dir/meta['entrypoint'] | ||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
|
||||||||||||||||||||
| spec=importlib.util.spec_from_file_location(f"skill_{meta['name']}", entry) | ||||||||||||||||||||
| mod=importlib.util.module_from_spec(spec); spec.loader.exec_module(mod) | ||||||||||||||||||||
| return mod.run(**kwargs) if hasattr(mod, 'run') else {'error':'missing run()'} | ||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| import json | ||
| from pathlib import Path | ||
| class SkillRegistry: | ||
| def __init__(self, root: Path): | ||
| self.path = root / 'registry.json'; root.mkdir(parents=True, exist_ok=True) | ||
| if not self.path.exists(): self.path.write_text(json.dumps({'installed_skills': []}, indent=2), encoding='utf-8') | ||
| def read(self): return json.loads(self.path.read_text(encoding='utf-8')) | ||
| def list(self): return self.read().get('installed_skills', []) | ||
| def add(self, name): | ||
| data=self.read(); skills=data.setdefault('installed_skills', []) | ||
| if name not in skills: skills.append(name) | ||
| self.path.write_text(json.dumps(data, indent=2), encoding='utf-8') | ||
| def remove(self, name): | ||
| data=self.read(); data['installed_skills']=[s for s in data.get('installed_skills',[]) if s!=name] | ||
| self.path.write_text(json.dumps(data, indent=2), encoding='utf-8') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: The JSON parser currently slices from the first
{to the last}in the whole response, which breaks when the model outputs extra brace-containing text before or after the JSON object. This causes valid capability responses to be rejected as invalid JSON. Parse the first decodable JSON object instead of using greedy brace slicing. [logic error]Severity Level: Major⚠️
Steps of Reproduction ✅
Prompt for AI Agent 🤖