Add NeuroRift package skeleton, skill system, runtime/model checks, and hardened tool execution#37
Merged
Neuro-Rift merged 9 commits intoMar 22, 2026
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
… codex/improve-code-robustness-and-error-handling
|
Important Review skippedToo many files! This PR contains 189 files, which is 39 over the limit of 150. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (189)
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces a comprehensive modular agent framework for NeuroRift, establishing packages for session management, skills distribution, memory systems, communication channels, execution environments, and core agent orchestration. It includes security-hardened command execution, capability verification, environment checking, CLI routing, and foundational support for multiple integration points (Discord, Telegram, Web, APIs). Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can disable sequence diagrams in the walkthrough.Disable the |
codeant-ai
Bot
added
the
size:XXL
Sequence DiagramThis PR adds two core runtime flows: guarded autonomous startup and a modular skill lifecycle. The diagram shows how CLI commands now gate agent mode with environment and model checks, and how skills are fetched, installed, and executed through the new skill system. sequenceDiagram
participant User
participant CLI
participant RuntimeCheck
participant ModelCheck
participant SkillManager
participant SkillSource
participant NeuroRift
alt Run autonomous agent
User->>CLI: Run agent command
CLI->>RuntimeCheck: Verify runtime tools
RuntimeCheck-->>CLI: Environment status
CLI->>ModelCheck: Verify model capabilities
ModelCheck-->>CLI: Agent readiness
CLI->>NeuroRift: Start autonomous mode
NeuroRift->>NeuroRift: Run tools via secure runner
NeuroRift-->>User: Agent started or blocked
else Manage skills
User->>CLI: Install or run skill command
CLI->>SkillManager: Process skill action
SkillManager->>SkillSource: Fetch skill package
SkillManager->>SkillManager: Validate install register and load
SkillManager-->>User: Skill result
end
Generated by CodeAnt AI |
Contributor
 |
|
Overall Grade |
Security  Reliability Complexity  Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Python |  |
Mar 22, 2026 2:27p.m. | Review ↗ |
Nitpicks 🔍
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6558efb994
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+252
to
+254
| if not stdout.strip() and not stderr.strip(): | ||
| validation_passed = False | ||
| error_type = error_type or "empty_output" |
There was a problem hiding this comment.
_validate_tool_output marks empty stdout+stderr as a validation failure, and _run_command_secure then requires validation_passed for success, so any command that exits 0 without output is misclassified as failed. This breaks valid silent operations (for example go install can be quiet on success), which in turn makes install_missing_tools report false failures and trigger unnecessary retries.
Useful? React with 👍 / 👎.
Comment on lines
+1015
to
+1017
| model_name = args.model or os.getenv("OLLAMA_MODEL") | ||
| if model_name: | ||
| capability = verify_model_capabilities(model_name) |
There was a problem hiding this comment.
The run-agent path verifies args.model/OLLAMA_MODEL but never applies that model to the actual runtime, because NeuroRift() (and its OllamaClient) is already instantiated earlier and uses its own model settings. This means capability checks can pass for one model while execution happens with another unverified/default model, causing readiness checks to give a false sense of safety.
Useful? React with 👍 / 👎.
Comment on lines
+8
to
+10
| dst = self.cache_dir / skill_name | ||
| if dst.exists(): shutil.rmtree(dst) | ||
| shutil.copytree(src, dst) |
There was a problem hiding this comment.
fetch_skill joins untrusted skill_name directly into both source and destination paths, so inputs like ../../tmp/foo can escape source_dir and cache_dir; because the code calls shutil.rmtree(dst) before copy, this can delete directories outside the skill store. A malformed --clawhub value therefore has destructive filesystem impact beyond the intended sandbox.
Useful? React with 👍 / 👎.
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Comment on lines
+27
to
+32
| raw_text = (raw_text or "").strip() | ||
| start = raw_text.find("{") | ||
| end = raw_text.rfind("}") | ||
| if start == -1 or end == -1 or end <= start: | ||
| raise ValueError("No JSON object found in model response") | ||
| return json.loads(raw_text[start : end + 1]) |
There was a problem hiding this comment.
Suggestion: The JSON extraction uses the first { and last } in the whole response, so any extra brace text before/after the actual JSON object can make parsing fail even when the model returned valid JSON. Parse the first decodable JSON object instead of slicing by global brace positions. [logic error]
Severity Level: Major ⚠️
- ❌ run-agent startup fails despite valid model output.
- ⚠️ Capability check becomes brittle to normal LLM formatting.
Suggested change
| raw_text = (raw_text or "").strip() | |
| start = raw_text.find("{") | |
| end = raw_text.rfind("}") | |
| if start == -1 or end == -1 or end <= start: | |
| raise ValueError("No JSON object found in model response") | |
| return json.loads(raw_text[start : end + 1]) | |
| raw_text = (raw_text or "").strip() | |
| decoder = json.JSONDecoder() | |
| for i, ch in enumerate(raw_text): | |
| if ch != "{": | |
| continue | |
| try: | |
| obj, _ = decoder.raw_decode(raw_text[i:]) | |
| except json.JSONDecodeError: | |
| continue | |
| if isinstance(obj, dict): | |
| return obj | |
| raise ValueError("No JSON object found in model response") |
Steps of Reproduction ✅
1. Start through CLI entrypoint `neurorift_cli.py:39-53` (`main()`), which forwards args
into `neurorift_main.main()` at line 52.
2. Run autonomous mode command parsed at `neurorift_main.py:937-940` (`run-agent --model
...`), then `_async_main` executes model check at `neurorift_main.py:1015-1018`.
3. `verify_model_capabilities()` in `model_capability_check.py:35-58` calls Ollama and
passes `proc.stdout` into `_extract_json()` at line 58.
4. If stdout contains any non-payload brace text plus valid JSON, `_extract_json()`
(`model_capability_check.py:27-32`) slices first `{` to last `}`, causing
`json.loads(...)` failure; exception is converted to `invalid_capability_json` at
`model_capability_check.py:59-64`, and `run-agent` is blocked by
`neurorift_main.py:1018-1021`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** model_capability_check.py
**Line:** 27:32
**Comment:**
*Logic Error: The JSON extraction uses the first `{` and last `}` in the whole response, so any extra brace text before/after the actual JSON object can make parsing fail even when the model returned valid JSON. Parse the first decodable JSON object instead of slicing by global brace positions.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+82
to
+83
| parsed["ok"] = bool(parsed.get("agent_ready")) | ||
| return parsed |
There was a problem hiding this comment.
Suggestion: Converting agent_ready with bool(...) treats non-empty strings like "false" as True, which can incorrectly mark an incapable model as ready. Require agent_ready to be a real boolean and then copy that value directly. [type error]
Severity Level: Critical 🚨
- ❌ Non-ready models can pass autonomous safety gate.
- ❌ run-agent may execute with invalid capability contract.
Suggested change
| parsed["ok"] = bool(parsed.get("agent_ready")) | |
| return parsed | |
| if not isinstance(parsed.get("agent_ready"), bool): | |
| return { | |
| "ok": False, | |
| "error": "capability_field_invalid_type", | |
| "parsed": parsed, | |
| "agent_ready": False, | |
| } | |
| parsed["ok"] = parsed["agent_ready"] | |
| return parsed |
Steps of Reproduction ✅
1. Invoke autonomous flow via `run-agent` (parser wiring at `neurorift_main.py:937-940`,
execution gate at `neurorift_main.py:1006-1021`).
2. In `verify_model_capabilities()` (`model_capability_check.py:67-83`), only key presence
is checked (`required.issubset(...)` at lines 67-75), not field types.
3. When model returns JSON like `"agent_ready": "false"` (string), line 82 computes
`bool("false") == True`, so `ok` is incorrectly marked true.
4. `neurorift_main.py:1018` tests `if not capability.get("agent_ready")`; string `"false"`
is truthy, so the rejection branch is skipped and orchestrated agent mode proceeds
(`neurorift_main.py:1066+`) even though readiness value is semantically false.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** model_capability_check.py
**Line:** 82:83
**Comment:**
*Type Error: Converting `agent_ready` with `bool(...)` treats non-empty strings like `"false"` as `True`, which can incorrectly mark an incapable model as ready. Require `agent_ready` to be a real boolean and then copy that value directly.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| @@ -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.
Suggestion: receive_message currently uses payload or {}, which silently replaces valid falsy payloads (such as "", 0, or False) with an empty dict. This loses message content and can break downstream logic that expects the original payload. Only default to {} when the payload is actually None. [logic error]
Severity Level: Major ⚠️
- ⚠️ Channel payload integrity breaks for valid falsy messages.
- ⚠️ Future channel consumers may mis-handle empty user inputs.
Suggested change
| def receive_message(self, payload=None): return payload or {} | |
| def receive_message(self, payload=None): return {} if payload is None else payload |
Steps of Reproduction ✅
1. Inspect `neurorift/channels/telegram_channel.py:3`; `receive_message()` returns
`payload or {}`, so any falsy payload is replaced.
2. Verify current execution context: codebase search found no imports/usages of
`neurorift.channels` and no `Channel(` instantiations (Grep on `*.py` returned no
matches), so this is currently unintegrated skeleton code.
3. Reproduce directly via Python entrypoint: `from neurorift.channels.telegram_channel
import Channel`; then call `Channel().receive_message("")` or
`Channel().receive_message(0)`.
4. Observe returned value is `{}` (not original `""`/`0`), demonstrating data loss for
valid falsy payloads due to line 3 logic.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/channels/telegram_channel.py
**Line:** 3:3
**Comment:**
*Logic Error: `receive_message` currently uses `payload or {}`, which silently replaces valid falsy payloads (such as `""`, `0`, or `False`) with an empty dict. This loses message content and can break downstream logic that expects the original payload. Only default to `{}` when the payload is actually `None`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| @@ -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.
Suggestion: receive_message uses payload or {}, which incorrectly converts valid falsy payloads (such as 0, False, "", or []) into {}. This causes silent data corruption and type changes at runtime. Only substitute {} when the payload is actually None. [logic error]
Severity Level: Major ⚠️
- ⚠️ Web channel coerces falsy payloads into dictionaries.
- ⚠️ Downstream message handlers can receive wrong payload types.
- ⚠️ Current skeleton has no active callers in repository.
Suggested change
| def receive_message(self, payload=None): return payload or {} | |
| def receive_message(self, payload=None): return {} if payload is None else payload |
Steps of Reproduction ✅
1. Confirm execution context: `receive_message` is defined at
`neurorift/channels/web_channel.py:3`, and Grep shows no in-repo callers/instantiations
yet (`web_channel`, `Channel(`, and channel imports had no matches).
2. Execute direct runtime call path by importing `Channel` from
`neurorift/channels/web_channel.py` and invoking `Channel.receive_message(...)` (verified
via `python3 -c` in this repo).
3. Pass realistic falsy values (`0`, `False`, `""`, `[]`) to `receive_message`; observed
output is `{}` for each value because `payload or {}` at line 3 treats all falsy values as
missing.
4. This reproduces silent type/data coercion: caller-provided payload is replaced rather
than preserved; only `None` should map to `{}`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/channels/web_channel.py
**Line:** 3:3
**Comment:**
*Logic Error: `receive_message` uses `payload or {}`, which incorrectly converts valid falsy payloads (such as `0`, `False`, `""`, or `[]`) into `{}`. This causes silent data corruption and type changes at runtime. Only substitute `{}` when the payload is actually `None`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+6
to
+7
| src = source_dir / skill_name | ||
| if not src.exists(): raise FileNotFoundError(skill_name) |
There was a problem hiding this comment.
Suggestion: skill_name is joined directly into source_dir without containment checks, so values like ../... or absolute paths can escape the allowed skill source tree and copy arbitrary local directories. Resolve both paths and enforce that the resolved source stays under source_dir before copying. [security]
Severity Level: Critical 🚨
- ❌ --clawhub can copy directories outside packaged skill_store.
- ⚠️ Unexpected local data enters ~/.neurorift/skills/cache.
Suggested change
| src = source_dir / skill_name | |
| if not src.exists(): raise FileNotFoundError(skill_name) | |
| src = (source_dir / skill_name).resolve() | |
| if source_dir.resolve() not in src.parents or not src.exists(): | |
| raise FileNotFoundError(skill_name) |
Steps of Reproduction ✅
1. Run CLI entrypoint `neurorift --clawhub ../../tmp/evilskill` (console script defined at
`setup.py:50-53`, forwarded by `neurorift_cli.py:51-53` into `neurorift_main.main()` at
`neurorift_main.py:1471`).
2. Parser accepts raw `--clawhub` string (`neurorift_main.py:914`) and passes it with only
`.strip()` to `SkillManager.install_clawhub()` (`neurorift_main.py:981-983`).
3. `SkillManager.install_clawhub()` forwards `skill_name` unchanged to
`ClawHubClient.fetch_skill()` (`neurorift/skills/skill_manager.py:13-14`).
4. In `fetch_skill()`, `src = source_dir / skill_name`
(`neurorift/clawhub/clawhub_client.py:6`) allows `..`/absolute-path escape; if escaped
path exists, line 7 passes and line 10 copies that external directory.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/clawhub/clawhub_client.py
**Line:** 6:7
**Comment:**
*Security: `skill_name` is joined directly into `source_dir` without containment checks, so values like `../...` or absolute paths can escape the allowed skill source tree and copy arbitrary local directories. Resolve both paths and enforce that the resolved source stays under `source_dir` before copying.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+21
to
+24
| dst=self.installed/name | ||
| if dst.exists(): shutil.rmtree(dst) | ||
| self.registry.remove(name) | ||
| return {'success': True, 'name': name} |
There was a problem hiding this comment.
Suggestion: The uninstall path is built directly from user-controlled skill name, so inputs like ../../... or absolute paths can escape the installed-skills directory and delete arbitrary folders via shutil.rmtree. Reject non-basename skill names before constructing the destination path. [security]
Severity Level: Critical 🚨
- ❌ `--skill uninstall` can delete arbitrary user directories.
- ⚠️ Skill registry may desync from actual filesystem state.
Suggested change
| dst=self.installed/name | |
| if dst.exists(): shutil.rmtree(dst) | |
| self.registry.remove(name) | |
| return {'success': True, 'name': name} | |
| safe_name = Path(name).name | |
| if safe_name != name or Path(name).is_absolute(): | |
| return {'success': False, 'error': 'invalid skill name'} | |
| dst = self.installed / safe_name | |
| if dst.exists(): shutil.rmtree(dst) | |
| self.registry.remove(safe_name) | |
| return {'success': True, 'name': safe_name} |
Steps of Reproduction ✅
1. Run CLI uninstall entrypoint using user-provided name: `python
/workspace/NeuroRift/neurorift_main.py --skill uninstall ../../../../tmp/demo` (`--skill`
defined at `neurorift_main.py:915`).
2. `_async_main` forwards raw `args.skill[1]` to `skill_manager.uninstall(skill_name)` at
`neurorift_main.py:999-1001`.
3. `SkillManager.uninstall` directly delegates at `neurorift/skills/skill_manager.py:19`
to `SkillInstaller.uninstall(name)`.
4. `SkillInstaller.uninstall` builds `dst=self.installed/name` and executes
`shutil.rmtree(dst)` at `neurorift/skills/installer.py:21-22`, allowing traversal outside
`~/.neurorift/skills/installed`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/skills/installer.py
**Line:** 21:24
**Comment:**
*Security: The uninstall path is built directly from user-controlled skill name, so inputs like `../../...` or absolute paths can escape the installed-skills directory and delete arbitrary folders via `shutil.rmtree`. Reject non-basename skill names before constructing the destination path.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| 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.
Suggestion: Running a non-installed or incomplete skill raises FileNotFoundError because skill.json is read unconditionally, which crashes the CLI path instead of returning a structured error. Check for the metadata file before reading it. [possible bug]
Severity Level: Major ⚠️
- ❌ `--skill run <name>` crashes for missing skill directories.
- ⚠️ No structured error returned to CLI users.
Suggested change
| meta=json.loads((skill_dir/'skill.json').read_text(encoding='utf-8')) | |
| meta_path = skill_dir / 'skill.json' | |
| if not meta_path.exists(): | |
| return {'error':'skill not installed or missing skill.json'} | |
| meta=json.loads(meta_path.read_text(encoding='utf-8')) |
Steps of Reproduction ✅
1. Invoke skill run command with unknown name using parser flow in
`neurorift_main.py:989-997` (e.g., `--skill run does_not_exist`).
2. `SkillManager.run()` at `neurorift/skills/skill_manager.py:17-18` builds path
`~/.neurorift/skills/installed/does_not_exist` and calls `SkillLoader.run()`.
3. `SkillLoader.run()` immediately reads `(skill_dir/'skill.json')` at
`neurorift/skills/loader.py:5`; missing file raises `FileNotFoundError`.
4. No guard around this call path; top-level `asyncio.run(_async_main(args))` at
`neurorift_main.py:1520` bubbles exception and CLI aborts.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/skills/loader.py
**Line:** 5:5
**Comment:**
*Possible Bug: Running a non-installed or incomplete skill raises `FileNotFoundError` because `skill.json` is read unconditionally, which crashes the CLI path instead of returning a structured error. Check for the metadata file before reading it.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+7
to
+8
| spec=importlib.util.spec_from_file_location(f"skill_{meta['name']}", entry) | ||
| mod=importlib.util.module_from_spec(spec); spec.loader.exec_module(mod) |
There was a problem hiding this comment.
Suggestion: Module spec creation is not validated; invalid entrypoint targets can produce None spec/loader and then crash with AttributeError during exec_module. Guard against invalid specs before loading. [null pointer]
Severity Level: Major ⚠️
- ❌ Malformed `entrypoint` crashes skill execution command.
- ⚠️ Developer-authored skills fail without actionable loader errors.
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) | |
| spec=importlib.util.spec_from_file_location(f"skill_{meta['name']}", str(entry)) | |
| if spec is None or spec.loader is None: | |
| return {'error':'invalid skill entrypoint'} | |
| mod=importlib.util.module_from_spec(spec); spec.loader.exec_module(mod) |
Steps of Reproduction ✅
1. Use normal skill execution path `neurorift_main.py:994-997` ->
`neurorift/skills/skill_manager.py:17-18`.
2. Set installed skill metadata `skill.json` `entrypoint` to an invalid target like
`"skill"` (no loadable module path).
3. `spec_from_file_location(...)` in `neurorift/skills/loader.py:7` can return `None` for
such targets.
4. Line 8 dereferences spec immediately (`module_from_spec(spec)` /
`spec.loader.exec_module`), causing runtime crash instead of returning structured error.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/skills/loader.py
**Line:** 7:8
**Comment:**
*Null Pointer: Module spec creation is not validated; invalid entrypoint targets can produce `None` spec/loader and then crash with `AttributeError` during `exec_module`. Guard against invalid specs before loading.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| entry=skill_dir/meta['entrypoint'] | ||
| spec=importlib.util.spec_from_file_location(f"skill_{meta['name']}", entry) | ||
| mod=importlib.util.module_from_spec(spec); spec.loader.exec_module(mod) | ||
| return mod.run(**kwargs) if hasattr(mod, 'run') else {'error':'missing run()'} |
There was a problem hiding this comment.
Suggestion: The code only checks for attribute presence and then calls it, so a non-callable run attribute triggers TypeError. Verify callability before invocation. [type error]
Severity Level: Major ⚠️
- ❌ Skill run crashes when `run` symbol is non-callable.
- ⚠️ Poor diagnostics for broken or incomplete skills.
Suggested change
| return mod.run(**kwargs) if hasattr(mod, 'run') else {'error':'missing run()'} | |
| run_fn = getattr(mod, 'run', None) | |
| return run_fn(**kwargs) if callable(run_fn) else {'error':'missing callable run()'} |
Steps of Reproduction ✅
1. Skill installation validation (`neurorift/skills/skill_validator.py:3-6`) only checks
required files exist, not that `run` is callable.
2. Run path `neurorift_main.py:994-997` -> `SkillManager.run()`
(`neurorift/skills/skill_manager.py:17-18`) loads module.
3. If module defines `run` as non-callable (e.g., `run = "v1"`), `hasattr(mod, 'run')` at
`neurorift/skills/loader.py:9` is true.
4. Loader then executes `mod.run(**kwargs)` and raises `TypeError`, aborting command
instead of returning structured loader error.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/skills/loader.py
**Line:** 9:9
**Comment:**
*Type Error: The code only checks for attribute presence and then calls it, so a non-callable `run` attribute triggers `TypeError`. Verify callability before invocation.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| 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.
Suggestion: read() can raise FileNotFoundError or JSONDecodeError (for example after an interrupted write), and that exception is not handled by callers like skill_manager.list(), so CLI skill commands can crash. Return a safe default registry and recreate the file when parsing fails. [possible bug]
Severity Level: Major ⚠️
- ❌ `--skill list` can crash on broken registry JSON.
- ⚠️ Skill uninstall/list flows depend on same unsafe read.
Suggested change
| def read(self): return json.loads(self.path.read_text(encoding='utf-8')) | |
| def read(self): | |
| try: | |
| data = json.loads(self.path.read_text(encoding='utf-8')) | |
| except (FileNotFoundError, json.JSONDecodeError): | |
| data = {'installed_skills': []} | |
| self.path.write_text(json.dumps(data, indent=2), encoding='utf-8') | |
| return data |
Steps of Reproduction ✅
1. Run the CLI entrypoint `neurorift` (wired in `setup.py:50-53` to `neurorift_cli:main`,
then forwarded to `neurorift_main.main()` at `neurorift_cli.py:50-53`).
2. Execute `neurorift --skill list`; `_async_main()` in `neurorift_main.py:989-993` calls
`skill_manager.list()`.
3. `SkillManager.list()` in `neurorift/skills/skill_manager.py:16` calls
`self.installer.registry.list()`, and `SkillRegistry.list()` in
`neurorift/skills/registry.py:8` calls `read()` at line 7.
4. If `~/.neurorift/skills/registry.json` is missing or contains invalid JSON (e.g.,
truncated file after interrupted write from `registry.py:12`/`registry.py:15`),
`json.loads(...)` at `registry.py:7` raises and is not caught in this call chain, so the
skill CLI path exits with an uncaught exception.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift/skills/registry.py
**Line:** 7:7
**Comment:**
*Possible Bug: `read()` can raise `FileNotFoundError` or `JSONDecodeError` (for example after an interrupted write), and that exception is not handled by callers like `skill_manager.list()`, so CLI skill commands can crash. Return a safe default registry and recreate the file when parsing fails.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
setup.py (1)
69-69:⚠️ Potential issue | 🟡 MinorFile handle left open.
Using
open("README.md").read()without a context manager leaves the file handle unclosed. While this is minor for a short-lived setup script, it's better practice to use a context manager orPath.read_text().🔧 Proposed fix
- long_description=open("README.md").read(), + long_description=Path("README.md").read_text(encoding="utf-8"),Add
Pathimport at the top:-from setuptools import setup, find_packages +from pathlib import Path +from setuptools import setup, find_packages🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setup.py` at line 69, The long_description in setup.py currently uses open("README.md").read() leaving the file handle unclosed; replace that with a safe read using pathlib.Path (e.g., import Path from pathlib at top and use Path("README.md").read_text()) or use a with open(...) as f: f.read() pattern so the file is closed; update the import by adding Path from pathlib and change the long_description reference accordingly (target symbols: setup.py long_description and the open("README.md").read() expression).
🟠 Major comments (18)
neurorift/models/prompt_builder.py-1-3 (1)
1-3:⚠️ Potential issue | 🟠 MajorSerialize
contextexplicitly; raw dictrepris brittle for prompt construction.Using
f"{context}"emits Python representation, not a stable interchange format. That can make model behavior less predictable.Suggested fix
+import json + class PromptBuilder: - def build(self, message: str, context: dict): - return f"{context}\n{message}" + def build(self, message: str, context: dict) -> str: + context_blob = json.dumps(context, ensure_ascii=False, sort_keys=True, default=str) + return f"{context_blob}\n{message}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/models/prompt_builder.py` around lines 1 - 3, The PromptBuilder.build method currently interpolates the context dict using f"{context}", which emits Python's unstable repr; change it to explicitly serialize context (e.g., import json and use json.dumps(context, sort_keys=True, ensure_ascii=False, separators=(",", ":"))) before concatenation to produce a stable, predictable prompt; update PromptBuilder.build to call json.dumps on the context variable and handle potential non-serializable values (either by providing a default=str to json.dumps or falling back to repr) so the returned string is deterministic.neurorift/tools/tool_sandbox.py-1-3 (1)
1-3:⚠️ Potential issue | 🟠 Major
ToolSandbox.wrap()is currently a no-op, so sandboxing is not actually applied.This returns raw commands unchanged, which undermines the hardening objective and can lead to unsandboxed execution if callers trust this abstraction.
Suggested fix
class ToolSandbox: def wrap(self, command: list[str]) -> list[str]: - return command + if not command: + raise ValueError("command must not be empty") + # Example hardened wrapper; adjust flags to your threat model. + return ["firejail", "--quiet", "--net=none", "--", *command]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/tools/tool_sandbox.py` around lines 1 - 3, ToolSandbox.wrap currently returns commands unchanged, so either implement real sandboxing or fail fast: change ToolSandbox to hold a configurable sandbox command (e.g., self.sandbox_cmd) and have wrap(command: list[str]) return the command prefixed/translated into the sandbox invocation (for example: [*self.sandbox_cmd, *sandbox_args, "--", *command]) or, if no sandbox_cmd is configured, raise an explicit exception (RuntimeError or NotImplementedError) instead of returning the raw command; update callers of ToolSandbox.wrap accordingly so unsandboxed execution cannot silently occur.neurorift/clawhub/clawhub_api.py-1-2 (1)
1-2:⚠️ Potential issue | 🟠 Major
base_urluses a placeholder endpoint, which will break real ClawHub integration.Hardcoding an
.exampleURL in production path risks guaranteed runtime failure for skill fetch/install flows.Suggested fix
+import os + class ClawHubAPI: - base_url = "https://clawhub.example/api" + base_url = os.getenv("NEURORIFT_CLAWHUB_BASE_URL") + + `@classmethod` + def get_base_url(cls) -> str: + if not cls.base_url: + raise RuntimeError("NEURORIFT_CLAWHUB_BASE_URL is not configured") + return cls.base_url🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/clawhub/clawhub_api.py` around lines 1 - 2, ClawHubAPI currently hardcodes base_url to a placeholder "https://clawhub.example/api" which will break integrations; change ClawHubAPI so base_url is not hardcoded: read the endpoint from configuration or an environment variable (e.g., CLAWHUB_BASE_URL) with a sensible production default or raise a clear error if missing, validate the URL format before use, and update any constructors or uses of ClawHubAPI to accept or pass the configured base_url instead of relying on the class-level constant.neurorift/skills/loader.py-6-12 (1)
6-12:⚠️ Potential issue | 🟠 MajorHandle potential
Nonevalues and missing keys to prevent runtime crashes.Several unhandled failure modes exist:
spec_from_file_locationreturnsNoneif the file doesn't exist, causingAttributeErroron line 11- Missing
"name"or"entrypoint"keys inskill.jsonraisesKeyError- Invalid JSON causes
JSONDecodeError🛡️ Proposed fix with error handling
-import importlib.util, json +import importlib.util +import json from pathlib import Path class SkillLoader: def run(self, skill_dir: Path, **kwargs): - meta = json.loads((skill_dir / "skill.json").read_text(encoding="utf-8")) - entry = skill_dir / meta["entrypoint"] - spec = importlib.util.spec_from_file_location(f"skill_{meta['name']}", entry) + meta_path = skill_dir / "skill.json" + if not meta_path.exists(): + return {"error": f"skill.json not found in {skill_dir}"} + try: + meta = json.loads(meta_path.read_text(encoding="utf-8")) + except json.JSONDecodeError as e: + return {"error": f"Invalid skill.json: {e}"} + if "entrypoint" not in meta or "name" not in meta: + return {"error": "skill.json missing required fields"} + entry = skill_dir / meta["entrypoint"] + if not entry.exists(): + return {"error": f"Entrypoint {entry} not found"} + spec = importlib.util.spec_from_file_location(f"skill_{meta['name']}", entry) + if spec is None or spec.loader is None: + return {"error": f"Failed to load module from {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()"}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/skills/loader.py` around lines 6 - 12, The run method currently assumes skill.json exists and contains "name" and "entrypoint", that the entry file exists, and that spec_from_file_location returns a valid spec; update run to robustly handle these failure modes by: wrap json.loads((skill_dir / "skill.json").read_text(...)) in try/except to catch FileNotFoundError and JSONDecodeError and return a descriptive error dict, validate meta contains "name" and "entrypoint" keys before using them and return an error if missing, verify the entry Path exists before calling importlib.util.spec_from_file_location, check that spec is not None and spec.loader is present before calling spec.loader.exec_module, and finally ensure mod.run exists before calling and return consistent error dicts (use the same return shape as current {"error": ...}) for each failure path; refer to run, meta, entry, spec, mod, and spec.loader in your changes.neurorift/channels/web_channel.py-5-6 (1)
5-6:⚠️ Potential issue | 🟠 Major
receive_messageshould not rewrite falsy values.Using
payload or {}replaces legitimate payloads (0,"",False) with{}. Use an explicitNonecheck.Proposed fix
def receive_message(self, payload=None): - return payload or {} + return {} if payload is None else payload🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/channels/web_channel.py` around lines 5 - 6, The receive_message method currently returns payload or {} which incorrectly replaces legitimate falsy payloads (0, "", False) with an empty dict; change the logic in receive_message to only substitute {} when payload is exactly None (e.g., use an explicit None check) so falsy but valid values are returned unchanged — update the receive_message function accordingly.neurorift/channels/telegram_channel.py-5-6 (1)
5-6:⚠️ Potential issue | 🟠 MajorPreserve valid falsy payloads in
receive_message.
payload or {}treats valid payloads like""or0as missing and mutates them into{}. Return{}only when payload is actuallyNone.Proposed fix
def receive_message(self, payload=None): - return payload or {} + return {} if payload is None else payload🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/channels/telegram_channel.py` around lines 5 - 6, The receive_message method currently uses "payload or {}" which converts valid falsy payloads (e.g., "" or 0) into an empty dict; change receive_message to only substitute {} when payload is exactly None (e.g., return {} if payload is None else payload) so valid falsy values are preserved — update the receive_message implementation accordingly.neurorift/channels/cli_channel.py-5-6 (1)
5-6:⚠️ Potential issue | 🟠 MajorFix falsy payload handling in
receive_message.The current expression can accidentally transform valid payloads into
{}. Default only onNone.Proposed fix
def receive_message(self, payload=None): - return payload or {} + return {} if payload is None else payload🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/channels/cli_channel.py` around lines 5 - 6, The receive_message function currently uses a falsy check that converts valid falsy payloads into {}; change the fallback to only apply when payload is None by returning payload if payload is not None, otherwise {}—update the receive_message signature/return logic to explicitly test for None (e.g., in receive_message) so valid values like 0, False, or empty lists/dicts are preserved.neurorift/channels/discord_channel.py-5-6 (1)
5-6:⚠️ Potential issue | 🟠 MajorAvoid coercing falsy payloads into
{}inreceive_message.Current logic drops valid falsy messages. Only default to
{}when payload isNone.Proposed fix
def receive_message(self, payload=None): - return payload or {} + return {} if payload is None else payload🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/channels/discord_channel.py` around lines 5 - 6, The receive_message function currently returns payload or {}, which incorrectly coerces valid falsy payloads (e.g., "", 0, False, empty list) to {} — change the logic in receive_message to only default to {} when payload is exactly None (e.g., use an explicit None check) so legitimate falsy values are preserved; update the function signature/return to return the incoming payload when payload is not None and only return {} when payload is None, keeping the function name receive_message unchanged.neurorift/tools/tool_validator.py-2-3 (1)
2-3:⚠️ Potential issue | 🟠 MajorHarden validation for non-dict inputs and return actionable errors.
tool_call.get(...)will fail if upstream passes a non-dict. Also, failures currently returnNoneerror details, which makes troubleshooting harder.Proposed fix
class ToolValidator: def validate(self, tool_call: dict): - return bool(tool_call.get("name")), None + if not isinstance(tool_call, dict): + return False, "tool_call must be a dictionary" + name = tool_call.get("name") + if not isinstance(name, str) or not name.strip(): + return False, "tool_call.name must be a non-empty string" + return True, None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/tools/tool_validator.py` around lines 2 - 3, The validate method currently assumes tool_call is a dict and returns None for errors; change validate(tool_call: dict) to first check isinstance(tool_call, dict) and return (False, "tool_call must be a dict") if not, then check for a non-empty "name" key using tool_call.get("name") and return (False, "missing or empty 'name' in tool_call") on failure, otherwise return (True, None); update the validate function body to perform these checks (reference the validate method and the tool_call parameter) so callers receive actionable error messages.neurorift/skills/registry.py-10-12 (1)
10-12:⚠️ Potential issue | 🟠 MajorUse atomic file writes for
registry.json.Direct overwrite can leave a partially written JSON file on interruption/crash, which can break all skill lifecycle operations.
🔒 Suggested atomic-write helper
import json +import os +import tempfile from pathlib import Path @@ class SkillRegistry: @@ + def _write(self, data: dict) -> None: + fd, tmp_path = tempfile.mkstemp(dir=self.path.parent, text=True) + try: + with os.fdopen(fd, "w", encoding="utf-8") as tmp: + json.dump(data, tmp, indent=2) + tmp.flush() + os.fsync(tmp.fileno()) + os.replace(tmp_path, self.path) + finally: + if os.path.exists(tmp_path): + os.unlink(tmp_path) @@ if not self.path.exists(): - self.path.write_text( - json.dumps({"installed_skills": []}, indent=2), encoding="utf-8" - ) + self._write({"installed_skills": []}) @@ - self.path.write_text(json.dumps(data, indent=2), encoding="utf-8") + self._write(data) @@ - self.path.write_text(json.dumps(data, indent=2), encoding="utf-8") + self._write(data)Also applies to: 25-25, 32-32
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/skills/registry.py` around lines 10 - 12, Replace the direct self.path.write_text(...) calls that overwrite registry.json with an atomic-write helper: create a helper (e.g., atomic_write(path, data_bytes)) that writes to a temporary file in the same directory, flushes and fsyncs the file descriptor, closes it, then does an atomic os.replace(temp_path, target_path); update every occurrence of self.path.write_text (the three spots flagged) to call this helper with the UTF-8 encoded JSON bytes so registry.json is never left partially written.neurorift_launch.sh-27-27 (1)
27-27:⚠️ Potential issue | 🟠 MajorDo not bypass preflight/boot failures, and resolve helper scripts from script dir.
Line 27 and Line 38 currently force success, so startup can continue in an invalid state; and Lines 27/30 rely on current working directory.
🔧 Suggested fix
+SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)" @@ echo "[1/5] Verifying runtime environment..." -python3 runtime_environment_check.py || true +python3 "$SCRIPT_DIR/runtime_environment_check.py" @@ echo "[2/5] Verifying model capabilities for $MODEL_NAME..." -CAP_JSON="$(python3 model_capability_check.py --model "$MODEL_NAME")" +CAP_JSON="$(python3 "$SCRIPT_DIR/model_capability_check.py" --model "$MODEL_NAME")" @@ echo "[3/5] Bootstrapping model session..." -ollama run "$MODEL_NAME" "$BOOT_PROMPT" >/dev/null || true +ollama run "$MODEL_NAME" "$BOOT_PROMPT" >/dev/nullAlso applies to: 30-30, 38-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift_launch.sh` at line 27, Remove the "|| true" that masks failures so preflight/boot failures abort startup, and resolve helper scripts relative to the launch script directory instead of the current working directory: compute SCRIPT_DIR like SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" near the top, then invoke runtime_environment_check.py and the other helper scripts using "$SCRIPT_DIR/runtime_environment_check.py" (and the other script filenames referenced at lines ~30 and ~38) so failures propagate and scripts are found reliably.neurorift/sessions/session_manager.py-17-19 (1)
17-19:⚠️ Potential issue | 🟠 MajorPersist before caching to avoid split-brain session state.
If Line 18 fails, Line 17 has already published a session that does not exist in durable storage.
🔧 Suggested ordering fix
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) + self.sessions[sid] = s return s🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/sessions/session_manager.py` around lines 17 - 19, The session is being added to the in-memory cache (self.sessions[sid] = s) before being persisted (self.store.save(s)), risking a cached session that isn't durable if save fails; change the order in the method that creates/stores sessions so you call self.store.save(s) first and only after it succeeds assign self.sessions[sid] = s and return s (use the existing symbols s, sid, self.store.save, self.sessions), optionally wrapping the save in a try/except to avoid adding to the cache on failure.neurorift_launch.sh-32-33 (1)
32-33:⚠️ Potential issue | 🟠 MajorParse
agent_readyfrom JSON structurally instead of usinggrep.String matching on JSON is fragile and can produce incorrect gate decisions.
🔧 Suggested robust gate
-if ! echo "$CAP_JSON" | grep -q '"agent_ready": true'; then +if ! python3 -c 'import json,sys; sys.exit(0 if json.load(sys.stdin).get("agent_ready") is True else 1)' <<<"$CAP_JSON"; then echo "❌ Model is not agent-ready. Aborting NeuroRift startup." exit 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift_launch.sh` around lines 32 - 33, Replace the brittle grep check against CAP_JSON with a structural JSON parse (e.g., using jq) so the if-condition tests the boolean field directly; specifically, evaluate CAP_JSON with jq (e.g., test '.agent_ready == true' or use -e to set exit code) in the existing if that currently references CAP_JSON and the grep, and log/abort when jq indicates agent_ready is not true (ensure CAP_JSON is passed to jq via stdin or here-string and handle jq failure by treating it as not agent-ready).neurorift/execution/command_runner.py-5-11 (1)
5-11:⚠️ Potential issue | 🟠 MajorHandle process spawn failures and hangs explicitly.
Line 6 can raise
OSError(e.g., command not found), and without a timeout the call can block indefinitely. This breaks the method's structured-result contract and can stall runtime flows. The inheritedSandboxRunnerclass is affected as well.🔧 Suggested hardening
class CommandRunner: - def run(self, cmd: list[str]): - p = subprocess.run(cmd, capture_output=True, text=True, check=False) + def run(self, cmd: list[str], timeout_seconds: float = 30.0): + try: + p = subprocess.run( + cmd, + capture_output=True, + text=True, + check=False, + timeout=timeout_seconds, + ) + except subprocess.TimeoutExpired as e: + return { + "success": False, + "stdout": e.stdout or "", + "stderr": (e.stderr or "") + f"\nTimed out after {timeout_seconds}s", + "exit_code": 124, + } + except OSError as e: + return { + "success": False, + "stdout": "", + "stderr": str(e), + "exit_code": 127, + } return { "success": p.returncode == 0, "stdout": p.stdout, "stderr": p.stderr, "exit_code": p.returncode, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/execution/command_runner.py` around lines 5 - 11, The run method in neurorift.execution.command_runner.CommandRunner should catch process spawn errors and enforce a timeout so it never blocks: wrap the subprocess.run call in try/except to catch OSError (e.g., command not found) and subprocess.TimeoutExpired, pass a sensible timeout value into subprocess.run, and return the same structured dict (keys "success", "stdout", "stderr", "exit_code") enriched for failure cases (set "success" to False, provide stderr with the exception message and a distinct exit_code or include a "timeout" flag) so callers (including SandboxRunner) still receive a consistent result instead of raising or hanging.neurorift/sessions/session_store.py-11-12 (1)
11-12:⚠️ Potential issue | 🟠 MajorPath traversal vulnerability in session ID handling.
The
sidparameter is used directly in path construction without sanitization. A malicious session ID like../../etc/passwdcould allow writing or reading files outside the intended directory.🛡️ Proposed fix to sanitize session ID
def path(self, sid: str) -> Path: + # Prevent path traversal by ensuring sid is a simple filename + if not sid or "/" in sid or "\\" in sid or sid.startswith("."): + raise ValueError(f"Invalid session ID: {sid}") return self.root / f"{sid}.json"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/sessions/session_store.py` around lines 11 - 12, The path method is vulnerable because it inserts raw sid into the filesystem path; sanitize sid inside path(self, sid: str) by stripping any directory components and rejecting/controling unsafe characters: obtain the basename (e.g., using pathlib.Path(sid).name) or validate against an allowlist (alphanumeric, hyphen, underscore) and raise on invalid sids, then construct the path as self.root / f"{safe_sid}.json"; additionally, after building the Path you can resolve it and assert it is within self.root (e.g., ensure resolved_path.resolve().is_relative_to(self.root.resolve()) or equivalent) to prevent traversal.neurorift/skills/installer.py-30-35 (1)
30-35:⚠️ Potential issue | 🟠 MajorPath traversal risk in
uninstallmethod.The
nameparameter is used directly to construct the path forshutil.rmtree. A malicious name like../../important_dircould delete arbitrary directories.🛡️ Proposed fix
def uninstall(self, name: str): + if not name or "/" in name or "\\" in name or name.startswith("."): + return {"success": False, "error": f"invalid_skill_name:{name}"} dst = self.installed / name + # Additional safety: ensure dst is actually under self.installed + if not dst.resolve().is_relative_to(self.installed.resolve()): + return {"success": False, "error": "path_traversal_attempt"} if dst.exists(): shutil.rmtree(dst)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/skills/installer.py` around lines 30 - 35, The uninstall method uses the raw name to build dst and then calls shutil.rmtree, allowing path traversal; fix this by validating/sanitizing name before deletion: ensure name contains no path separators (e.g., no "/" or os.path.pardir) or compute dst = (self.installed / name).resolve() and verify dst is a subpath of self.installed.resolve() (using Path.relative_to or comparing parents) and raise/return an error if it escapes; only call shutil.rmtree(dst) and self.registry.remove(name) after the check to guarantee deletions are confined to the installed directory.neurorift/sessions/session_store.py-14-24 (1)
14-24:⚠️ Potential issue | 🟠 MajorRound-trip serialization may fail for
SessionStateenum.The
savemethod serializesSessionStateenum values to strings viadefault=str, butloadpasses the raw JSON dict toSessionContext(**...). SinceSessionContext.stateexpects aSessionStateenum (not a string), deserialization will fail or produce incorrect types.🔧 Proposed fix to handle enum conversion
def load(self, sid: str): p = self.path(sid) - return ( - SessionContext(**json.loads(p.read_text(encoding="utf-8"))) - if p.exists() - else None - ) + if not p.exists(): + return None + data = json.loads(p.read_text(encoding="utf-8")) + # Convert state string back to enum + if "state" in data and isinstance(data["state"], str): + data["state"] = SessionState(data["state"]) + return SessionContext(**data)You'll also need to import
SessionState:-from neurorift.sessions.session_context import SessionContext +from neurorift.sessions.session_context import SessionContext, SessionState🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/sessions/session_store.py` around lines 14 - 24, The save/load pair currently serializes SessionContext with default=str which turns SessionState enum members into strings, but load() blindly does SessionContext(**json.loads(...)) so state becomes a string; update load (the load method in session_store.py) to parse the JSON, convert the "state" field back to the SessionState enum (import SessionState) before constructing SessionContext (e.g., load dict -> if "state" in data: data["state"] = SessionState(data["state"]) ), then pass the converted dict into SessionContext(**data); keep save() unchanged.neurorift/skills/installer.py-21-23 (1)
21-23:⚠️ Potential issue | 🟠 MajorMissing
namefield causes unhandledKeyError; path traversal risk.
- If
skill.jsonlacks a"name"key, line 22 raisesKeyErrorinstead of returning a structured error like the validation failure path.- The
nameextracted from untrusted JSON is used directly in path construction (self.installed / name), allowing path traversal attacks (e.g.,"name": "../../../etc").🛡️ Proposed fix
meta = json.loads((pkg / "skill.json").read_text(encoding="utf-8")) - name = meta["name"] + name = meta.get("name") + if not name or not isinstance(name, str): + return {"success": False, "error": "missing_or_invalid:name"} + # Prevent path traversal + if "/" in name or "\\" in name or name.startswith("."): + return {"success": False, "error": f"invalid_skill_name:{name}"} dst = self.installed / name🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/skills/installer.py` around lines 21 - 23, Handle missing or malicious "name" when loading skill metadata: after reading meta in the installer code that sets meta = json.loads(...) (the block that then does name = meta["name"] and dst = self.installed / name), validate that "name" exists and is a non-empty string and raise the same structured validation error used elsewhere instead of letting KeyError escape; additionally sanitize the name to prevent path traversal by rejecting names containing path separators or ".." or by normalizing via pathlib and using only the final path component (e.g., ensure name == Path(name).name and not absolute) before constructing dst = self.installed / name, failing with a clear validation error if checks fail.
🟡 Minor comments (6)
neurorift/memory/short_term_memory.py-5-6 (1)
5-6:⚠️ Potential issue | 🟡 MinorValidate
limitto prevent silent no-op buffering.When
limitis 0 or negative, memory retention behavior becomes invalid or surprising; guard it in__init__.Proposed fix
class ShortTermMemory: def __init__(self, limit: int = 30): + if limit <= 0: + raise ValueError("limit must be > 0") self.buffers = defaultdict(lambda: deque(maxlen=limit))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/memory/short_term_memory.py` around lines 5 - 6, The __init__ of ShortTermMemory currently sets self.buffers = defaultdict(lambda: deque(maxlen=limit)) without validating limit; add validation in ShortTermMemory.__init__ to ensure limit is a positive integer (e.g., check isinstance(limit, int) and limit > 0) and raise a ValueError with a clear message if not, so buffer creation using defaultdict(lambda: deque(maxlen=limit)) cannot silently become a no-op for limit <= 0.neurorift/memory/memory_compaction.py-2-3 (1)
2-3:⚠️ Potential issue | 🟡 MinorHandle non-string message items defensively.
" ".join(messages[:10])will fail if any item is not astr. A lightweight normalization step avoids runtime crashes.Proposed fix
class MemoryCompaction: def compact(self, messages: list[str]): - return {"summary": " ".join(messages[:10]), "kept": messages[-10:]} + normalized = [m if isinstance(m, str) else str(m) for m in messages] + return {"summary": " ".join(normalized[:10]), "kept": normalized[-10:]}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/memory/memory_compaction.py` around lines 2 - 3, The compact method currently does " ".join(messages[:10]) which will raise if any item in messages is not a str; update MemoryCompaction.compact (function compact) to defensively normalize items before joining and slicing: coerce each message to a string (or filter/skip None) for the summary and ensure the "kept" list contains the original items or their string representations consistently; implement a small normalization step (e.g., map(str, ...) or list comprehension that handles None) applied to messages[:10] for the summary and to messages[-10:] for kept so the method never fails on non-string items.neurorift/core/planner.py-11-15 (1)
11-15:⚠️ Potential issue | 🟡 MinorHandle whitespace-only objectives before creating a step.
A whitespace-only objective currently generates a non-actionable plan step. Trimming first avoids emitting empty intent.
Proposed fix
class Planner: def create_plan(self, objective: str) -> list[PlannedStep]: + objective = objective.strip() return ( [PlannedStep(order=1, action=f"Investigate: {objective}")] if objective else [] )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/core/planner.py` around lines 11 - 15, The create_plan function creates a PlannedStep even for whitespace-only objectives; trim the input objective first and only create and return [PlannedStep(...)] when objective.strip() is non-empty, otherwise return an empty list—update the create_plan implementation (reference function create_plan and PlannedStep/order/action) to perform the trim and conditional check before constructing the PlannedStep.neurorift_cli.py-61-64 (1)
61-64:⚠️ Potential issue | 🟡 MinorUnhandled exceptions from
neurorift_main.main().If
neurorift_main.main()raises an exception, it will propagate uncaught. The function unconditionally returns0, which may not reflect actual failures.🔧 Proposed fix with exception handling
# Route into existing app entrypoint; keep compatibility with current parser. - sys.argv = ["neurorift"] + forwarded - neurorift_main.main() - return 0 + sys.argv = ["neurorift", *forwarded] + try: + neurorift_main.main() + return 0 + except SystemExit as e: + return e.code if isinstance(e.code, int) else 1 + except Exception as exc: + print(f"Error: {exc}", file=sys.stderr) + return 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift_cli.py` around lines 61 - 64, The current wrapper replaces sys.argv then calls neurorift_main.main() and always returns 0, so any exception from neurorift_main.main() will propagate and real failures are hidden; update the wrapper to call neurorift_main.main() inside a try/except, catch Exception as e, log or print a clear error including e, and return a non-zero exit code on failure (or re-raise if appropriate); reference the sys.argv assignment, the call to neurorift_main.main(), and the current unconditional return 0 when making the change.neurorift/skills/skill_manager.py-17-19 (1)
17-19:⚠️ Potential issue | 🟡 MinorMissing error handling for failed skill fetch.
If
fetch_skillfails or returnsNone, passing the result toinstall()will cause an unclear error. Consider validating the fetch result before proceeding.🔧 Proposed fix
def install_clawhub(self, skill_name: str): pkg = self.client.fetch_skill(skill_name, self.examples) + if pkg is None: + return {"success": False, "error": f"skill_not_found:{skill_name}"} return self.installer.install(pkg)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift/skills/skill_manager.py` around lines 17 - 19, The install_clawhub method currently calls self.client.fetch_skill(skill_name, self.examples) and immediately passes the result to self.installer.install, which will raise unclear errors if fetch_skill fails or returns None; update install_clawhub to check the result of self.client.fetch_skill (and catch exceptions it may raise), and if the fetch returned None or an exception occurred, log or raise a clear error (including skill_name and the underlying error) instead of calling self.installer.install with an invalid package; use the symbols install_clawhub, client.fetch_skill, self.examples, and installer.install to locate and modify the code.neurorift_main.py-457-467 (1)
457-467:⚠️ Potential issue | 🟡 MinorHandle potential bytes type from TimeoutExpired exception.
When
subprocess.TimeoutExpiredis raised withcapture_output=True, thestdoutandstderrattributes may bebytesrather thanstr(since decoding may not have completed). The_sanitize_outputmethod expects astr, which could cause issues.🐛 Proposed fix
except subprocess.TimeoutExpired as exc: - stdout = self._sanitize_output(exc.stdout or "", max_output_len) - stderr = self._sanitize_output(exc.stderr or "", max_output_len) + raw_stdout = exc.stdout or "" + raw_stderr = exc.stderr or "" + if isinstance(raw_stdout, bytes): + raw_stdout = raw_stdout.decode("utf-8", errors="replace") + if isinstance(raw_stderr, bytes): + raw_stderr = raw_stderr.decode("utf-8", errors="replace") + stdout = self._sanitize_output(raw_stdout, max_output_len) + stderr = self._sanitize_output(raw_stderr, max_output_len) last_result = { "success": False, "stdout": stdout, "stderr": stderr or f"Command timed out after {timeout}s", "exit_code": 124, "error_type": "timeout", "validation_passed": False, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@neurorift_main.py` around lines 457 - 467, The TimeoutExpired handler may receive bytes for exc.stdout/exc.stderr but _sanitize_output expects strings; in the except block for subprocess.TimeoutExpired convert exc.stdout and exc.stderr to str first (e.g. decode bytes using 'utf-8' with errors='replace' or call .decode('utf-8', 'replace') when isinstance(..., bytes)) before passing them to self._sanitize_output, updating the variables stdout and stderr in that block where last_result is built (refer to the TimeoutExpired except block and the stdout/stderr variables).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cb9d1868-47bd-4b7f-87d0-ed0b907f0c69
📒 Files selected for processing (82)
model_capability_check.pyneurorift/__init__.pyneurorift/channels/__init__.pyneurorift/channels/channel_router.pyneurorift/channels/cli_channel.pyneurorift/channels/discord_channel.pyneurorift/channels/telegram_channel.pyneurorift/channels/web_channel.pyneurorift/clawhub/__init__.pyneurorift/clawhub/clawhub_api.pyneurorift/clawhub/clawhub_client.pyneurorift/clawhub/clawhub_resolver.pyneurorift/cli/__init__.pyneurorift/cli/neurorift_cli.pyneurorift/config/__init__.pyneurorift/config/channel_config.pyneurorift/config/model_config.pyneurorift/config/settings.pyneurorift/core/__init__.pyneurorift/core/agent_loop.pyneurorift/core/agent_manager.pyneurorift/core/planner.pyneurorift/core/task_router.pyneurorift/execution/__init__.pyneurorift/execution/command_runner.pyneurorift/execution/resource_limits.pyneurorift/execution/retry_manager.pyneurorift/execution/sandbox_runner.pyneurorift/gateway/__init__.pyneurorift/gateway/api_server.pyneurorift/gateway/auth_manager.pyneurorift/gateway/websocket_gateway.pyneurorift/memory/__init__.pyneurorift/memory/long_term_memory.pyneurorift/memory/memory_compaction.pyneurorift/memory/short_term_memory.pyneurorift/memory/vector_memory.pyneurorift/models/__init__.pyneurorift/models/context_builder.pyneurorift/models/model_failover.pyneurorift/models/model_router.pyneurorift/models/prompt_builder.pyneurorift/sessions/__init__.pyneurorift/sessions/session_context.pyneurorift/sessions/session_manager.pyneurorift/sessions/session_pruner.pyneurorift/sessions/session_store.pyneurorift/skill_store/__init__.pyneurorift/skill_store/cache/__init__.pyneurorift/skill_store/installed/__init__.pyneurorift/skill_store/installed/recon_scanner/README.mdneurorift/skill_store/installed/recon_scanner/__init__.pyneurorift/skill_store/installed/recon_scanner/requirements.txtneurorift/skill_store/installed/recon_scanner/skill.jsonneurorift/skill_store/installed/recon_scanner/skill.pyneurorift/skills/__init__.pyneurorift/skills/installer.pyneurorift/skills/loader.pyneurorift/skills/registry.pyneurorift/skills/skill_manager.pyneurorift/skills/skill_validator.pyneurorift/storage/__init__.pyneurorift/storage/database.pyneurorift/storage/logs_db.pyneurorift/storage/memory_db.pyneurorift/storage/session_db.pyneurorift/tools/__init__.pyneurorift/tools/tool_executor.pyneurorift/tools/tool_registry.pyneurorift/tools/tool_sandbox.pyneurorift/tools/tool_validator.pyneurorift/utils/__init__.pyneurorift/utils/logger.pyneurorift/utils/serializer.pyneurorift/utils/time_utils.pyneurorift/utils/validator.pyneurorift_cli.pyneurorift_launch.shneurorift_main.pyneurorift_wrapper.shruntime_environment_check.pysetup.py
Comment on lines
+1105
to
+1120
| if args.skill: | ||
| action = (args.skill[0] or "").lower() | ||
| if action == "list": | ||
| print(json.dumps({"installed_skills": skill_manager.list()}, indent=2)) | ||
| return | ||
| if action == "run" and len(args.skill) > 1: | ||
| skill_name = args.skill[1] | ||
| run_result = skill_manager.run( | ||
| skill_name, target=args.target or "127.0.0.1" | ||
| ) | ||
| print(json.dumps({"skill": skill_name, "result": run_result}, indent=2)) | ||
| return | ||
| if action == "uninstall" and len(args.skill) > 1: | ||
| skill_name = args.skill[1] | ||
| print(json.dumps(skill_manager.uninstall(skill_name), indent=2)) | ||
| return |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if SkillManager validates skill names internally
ast-grep --pattern 'def run(self, $NAME, $$$) {
$$$
}'
ast-grep --pattern 'def uninstall(self, $NAME) {
$$$
}'
# Also check for any validation in skill_manager.py
rg -n "skill.*name" --type py -g '*skill_manager*' -C3Repository: demonking369/NeuroRift
Length of output: 1154
🏁 Script executed:
# Find the installer and loader implementations
fd -t f '*.py' | xargs grep -l "class.*Installer\|class.*Loader" | head -20Repository: demonking369/NeuroRift
Length of output: 349
🏁 Script executed:
# Get more context on what happens with skill_name in these methods
rg -A 5 "def uninstall\(self, skill_name" neurorift/skills/Repository: demonking369/NeuroRift
Length of output: 227
🏁 Script executed:
# Check the loader.run() method
rg -A 5 "def run\(" neurorift/ --type py -g '*loader*'Repository: demonking369/NeuroRift
Length of output: 577
🏁 Script executed:
# Find the installer class and uninstall method
rg -B 3 -A 5 "def uninstall" neurorift/ --type pyRepository: demonking369/NeuroRift
Length of output: 1037
🏁 Script executed:
# Check what self.installer.installed is and how it's used
rg -B 5 -A 5 "installed\s*=" neurorift/skills/ --type pyRepository: demonking369/NeuroRift
Length of output: 758
🏁 Script executed:
# Look at the skill manager initialization to understand what installer is
rg -B 5 -A 10 "class SkillManager" neurorift/skills/skill_manager.pyRepository: demonking369/NeuroRift
Length of output: 628
🏁 Script executed:
# Check if registry.remove() validates the name
rg -B 3 -A 8 "def remove\(" neurorift/skills/ --type pyRepository: demonking369/NeuroRift
Length of output: 680
🏁 Script executed:
# Search for any skill name validation patterns in the codebase
rg -i "skill.*name.*valid\|valid.*skill\|sanitize\|safe.*name" neurorift/ --type pyRepository: demonking369/NeuroRift
Length of output: 48
🏁 Script executed:
# Check if there's any pattern used elsewhere for validating directory names or paths
rg -B 2 -A 2 "Path.*valid\|pathlib.*valid\|os.path.basename" neurorift/ --type pyRepository: demonking369/NeuroRift
Length of output: 48
Validate skill name to prevent path traversal attacks.
The skill name from args.skill[1] is passed directly to skill_manager.run() and skill_manager.uninstall() without validation. The underlying implementations construct file paths using path division (self.installer.installed / skill_name), which does not prevent directory traversal. An attacker could provide a skill name like "../../../" to access and potentially delete arbitrary directories via shutil.rmtree() in the uninstall operation.
Validate that the skill name contains only safe characters (e.g., alphanumeric, underscores, hyphens) before passing to SkillManager methods.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@neurorift_main.py` around lines 1105 - 1120, Validate the skill name coming
from args.skill[1] before calling skill_manager.run() or
skill_manager.uninstall(): ensure it matches a safe whitelist (e.g., regex
allowing only [A-Za-z0-9_-]+ and no path separators like "/" or ".."); if it
fails validation, print an error and return. Do this check in the block handling
action == "run" and action == "uninstall" (using the local variable skill_name
derived from args.skill[1]), rejecting or normalizing unsafe names to prevent
path traversal when the SkillManager uses self.installer.installed / skill_name
and shutil.rmtree().
Comment on lines
+10
to
+17
| def fetch_skill(self, skill_name: str, source_dir: Path) -> Path: | ||
| src = source_dir / skill_name | ||
| if not src.exists(): | ||
| raise FileNotFoundError(skill_name) | ||
| dst = self.cache_dir / skill_name | ||
| if dst.exists(): | ||
| shutil.rmtree(dst) | ||
| shutil.copytree(src, dst) |
There was a problem hiding this comment.
Prevent path traversal/arbitrary delete via skill_name.
skill_name is used directly in path joins (Line 11 and Line 14). Absolute paths or traversal segments can escape source_dir/cache_dir, and the delete at Line 16 can then target unintended directories.
🔒 Suggested hardening patch
def fetch_skill(self, skill_name: str, source_dir: Path) -> Path:
- src = source_dir / skill_name
- if not src.exists():
+ if not skill_name or Path(skill_name).name != skill_name:
+ raise ValueError(f"Invalid skill name: {skill_name!r}")
+
+ source_root = source_dir.resolve()
+ cache_root = self.cache_dir.resolve()
+ src = (source_root / skill_name).resolve()
+ dst = (cache_root / skill_name).resolve()
+
+ if source_root not in src.parents:
+ raise ValueError(f"Skill path escapes source_dir: {skill_name!r}")
+ if cache_root not in dst.parents:
+ raise ValueError(f"Skill path escapes cache_dir: {skill_name!r}")
+
+ if not src.is_dir():
raise FileNotFoundError(skill_name)
- dst = self.cache_dir / skill_name📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 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) | |
| def fetch_skill(self, skill_name: str, source_dir: Path) -> Path: | |
| if not skill_name or Path(skill_name).name != skill_name: | |
| raise ValueError(f"Invalid skill name: {skill_name!r}") | |
| source_root = source_dir.resolve() | |
| cache_root = self.cache_dir.resolve() | |
| src = (source_root / skill_name).resolve() | |
| dst = (cache_root / skill_name).resolve() | |
| if source_root not in src.parents: | |
| raise ValueError(f"Skill path escapes source_dir: {skill_name!r}") | |
| if cache_root not in dst.parents: | |
| raise ValueError(f"Skill path escapes cache_dir: {skill_name!r}") | |
| if not src.is_dir(): | |
| raise FileNotFoundError(skill_name) | |
| if dst.exists(): | |
| shutil.rmtree(dst) | |
| shutil.copytree(src, dst) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@neurorift/clawhub/clawhub_client.py` around lines 10 - 17, fetch_skill
currently uses skill_name directly to build src and dst (src = source_dir /
skill_name, dst = self.cache_dir / skill_name) which allows absolute paths or
traversal segments to escape source_dir/cache_dir and enables accidental or
malicious deletes; fix by validating skill_name is a simple basename (reject
path separators and absolute paths), then resolve both src and dst
(src.resolve() and dst.resolve()) and check that src is within
source_dir.resolve() and dst is within self.cache_dir.resolve() before
proceeding, and only perform shutil.rmtree(dst) after confirming dst is inside
the allowed cache_dir; raise a clear error (ValueError or FileNotFoundError) if
validation fails.
Comment on lines
+1
to
+3
| class AuthManager: | ||
| def validate(self, token: str): | ||
| return bool(token) |
There was a problem hiding this comment.
Fail-closed auth is required; current token check is bypassable.
bool(token) accepts any non-empty string, so arbitrary tokens validate successfully. If this path is wired into gateway authorization, it permits unauthorized access.
🔒 Proposed safe placeholder until real verification is implemented
class AuthManager:
- def validate(self, token: str):
- return bool(token)
+ def validate(self, token: str) -> bool:
+ raise NotImplementedError(
+ "Token verification is not implemented. Wire a real verifier before enabling auth."
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@neurorift/gateway/auth_manager.py` around lines 1 - 3, The current validate
method in AuthManager (AuthManager.validate) uses bool(token) which accepts any
non-empty string — change it to fail-closed: add an expected token/configurable
verifier (e.g., an AuthManager.__init__ that reads a configured AUTH_TOKEN or
accepts a verification callable) and update validate to return True only when
the provided token exactly matches that expected token or the verifier
explicitly succeeds; otherwise always return False (do not accept arbitrary
non-empty strings). Ensure the method defaults to False if no expected
token/verifier is configured so the gateway remains locked until real
verification is implemented.
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:XXL
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| EXECUTOR_SYSTEM = open("ai/prompts/executor_system.txt").read() |
There was a problem hiding this comment.
Suggestion: Loading the prompt with a relative path at import time depends on the process working directory and will fail with FileNotFoundError when launched outside repo root. Resolve the prompt path relative to the module file. [possible bug]
Severity Level: Major ⚠️
- ❌ Executor import can fail before scan starts.
- ⚠️ Startup depends on caller working directory.
- ⚠️ Packaging/launcher usage becomes brittle.
Suggested change
| EXECUTOR_SYSTEM = open("ai/prompts/executor_system.txt").read() | |
| EXECUTOR_SYSTEM = (__import__("pathlib").Path(__file__).resolve().parent / "prompts" / "executor_system.txt").read_text(encoding="utf-8") |
Steps of Reproduction ✅
1. `run_assessment()` imports `Executor` at runtime (`main.py:62`), which executes module
top-level statements in `ai/executor.py`.
2. At import time, `ai/executor.py:13` opens `ai/prompts/executor_system.txt` using
process CWD, not module location.
3. Launching from a non-repo working directory (while pointing to project files) makes
that relative path unresolved before execution starts.
4. Import fails before `Executor` initialization, preventing assessment execution from
reaching planning/execution steps (`main.py:115-127`).Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** ai/executor.py
**Line:** 13:13
**Comment:**
*Possible Bug: Loading the prompt with a relative path at import time depends on the process working directory and will fail with `FileNotFoundError` when launched outside repo root. Resolve the prompt path relative to the module file.
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.| {"type": "function", "function": { | ||
| "name": name, | ||
| "description": fn.__doc__ or name, | ||
| "parameters": getattr(fn, "schema", lambda: {})(), |
There was a problem hiding this comment.
Suggestion: Tool schemas are read from fn.schema, but registry entries are scope-wrapped callables that usually do not expose that attribute, so parameter schemas silently become {} and the model loses required argument contracts. Resolve schema from wrapped bound methods as a fallback. [logic error]
Severity Level: Major ⚠️
- ⚠️ Model receives empty tool parameter schema objects.
- ⚠️ Argument quality degrades across all wrapped tools.
- ⚠️ More invalid tool-call payloads likely generated.
Suggested change
| "parameters": getattr(fn, "schema", lambda: {})(), | |
| "parameters": (getattr(fn, "schema", None) or getattr(getattr(getattr(fn, "__wrapped__", None), "__self__", None), "schema", lambda: {}))(), |
Steps of Reproduction ✅
1. Tool registry wraps each `instance.run` with scope decorator in `main.py:46-50`;
wrappers come from `scope/enforcer.py:82-94` (`functools.wraps` preserves `__wrapped__`).
2. Tool schemas exist on tool instances/classes (example: `tools/sqli.py:22-23`,
`tools/race_condition.py:32-33`).
3. Executor builds tool schemas from wrapped function list in `ai/executor.py:27-34`; line
31 checks only `fn.schema`, which wrapped callables typically do not expose.
4. Resulting tool metadata sent to model (`ai/executor.py:52-54`) has empty/default
parameter schemas, reducing argument-contract guidance for all wrapped tools.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** ai/executor.py
**Line:** 31:31
**Comment:**
*Logic Error: Tool schemas are read from `fn.schema`, but registry entries are scope-wrapped callables that usually do not expose that attribute, so parameter schemas silently become `{}` and the model loses required argument contracts. Resolve schema from wrapped bound methods as a fallback.
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.|
|
||
| for call in response["calls"]: | ||
| fn_name = call.get("function", {}).get("name", "") | ||
| fn_args = json.loads(call.get("function", {}).get("arguments", "{}")) |
There was a problem hiding this comment.
Suggestion: Tool-call arguments are parsed with json.loads without error handling, so one malformed JSON argument payload from the model will raise JSONDecodeError and crash execution. Parse defensively and fall back to an empty dict when decoding fails or when the decoded value is not an object. [possible bug]
Severity Level: Critical 🚨
- ❌ Executor loop aborts on malformed model tool arguments.
- ⚠️ Planned tool sequence stops before completion.
- ⚠️ Session captures partial findings only.
Suggested change
| fn_args = json.loads(call.get("function", {}).get("arguments", "{}")) | |
| raw_args = call.get("function", {}).get("arguments", "{}") | |
| try: | |
| fn_args = json.loads(raw_args) if isinstance(raw_args, str) else (raw_args or {}) | |
| except json.JSONDecodeError: | |
| logger.warning("Invalid tool arguments JSON for %s: %r", fn_name, raw_args) | |
| fn_args = {} | |
| if not isinstance(fn_args, dict): | |
| logger.warning("Tool arguments must be an object for %s; got %s", fn_name, type(fn_args).__name__) | |
| fn_args = {} |
Steps of Reproduction ✅
1. Start the normal assessment entrypoint `main()` in `main.py:144-170`, which runs
`run_assessment()` via `asyncio.run(...)` at `main.py:163`.
2. In `run_assessment()` (`main.py:125-127`), `Executor.run(...)` is called after
planning.
3. During tool-call handling in `ai/executor.py:68-73`, if model response contains
malformed `function.arguments`, `json.loads(...)` at line 73 raises `JSONDecodeError` (no
local try/except).
4. Exception bubbles out of `Executor.run` (only tool execution is wrapped in try/except
at `ai/executor.py:93-106`), aborting execution flow since `main.py` only catches
`KeyboardInterrupt` (`main.py:164-166`).Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** ai/executor.py
**Line:** 73:73
**Comment:**
*Possible Bug: Tool-call arguments are parsed with `json.loads` without error handling, so one malformed JSON argument payload from the model will raise `JSONDecodeError` and crash execution. Parse defensively and fall back to an empty dict when decoding fails or when the decoded value is not an 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.| if not fn: | ||
| return {"error": f"Unknown tool: {tool_name}"} | ||
| try: | ||
| result = fn(**args) |
There was a problem hiding this comment.
Suggestion: Calling tools directly inside async code blocks the event loop and also breaks tools that internally use asyncio.run() (they will raise at runtime when already inside an event loop). Dispatch sync tools in a worker thread and only await truly async callables. [possible bug]
Severity Level: Major ⚠️
- ❌ race_condition tool fails under async executor loop.
- ⚠️ Long sync tools block executor responsiveness.
- ⚠️ Slower model-tool interaction cycles.
Suggested change
| result = fn(**args) | |
| import asyncio | |
| if asyncio.iscoroutinefunction(fn): | |
| result = await fn(**args) | |
| else: | |
| result = await asyncio.to_thread(fn, **args) |
Steps of Reproduction ✅
1. `main.py:31-50` builds tool registry with scope-wrapped sync callables
(`enforce_scope(...)(instance.run)`), then `Executor._dispatch` invokes them from async
context (`main.py:125-127`, `ai/executor.py:88-94`).
2. The `race_condition` tool implementation uses `asyncio.run(...)` inside sync `run()`
(`tools/race_condition.py:35-38`).
3. When executor dispatches `race_condition`, it currently calls `fn(**args)` directly
inside active event loop (`ai/executor.py:94`), so `asyncio.run()` path is hit while loop
is running.
4. Tool returns error path (`tools/race_condition.py:38-39`) and execution quality
degrades; additionally other long sync tools (subprocess/network in `tools/*.py`) block
the async loop while running.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** ai/executor.py
**Line:** 94:94
**Comment:**
*Possible Bug: Calling tools directly inside async code blocks the event loop and also breaks tools that internally use `asyncio.run()` (they will raise at runtime when already inside an event loop). Dispatch sync tools in a worker thread and only `await` truly async callables.
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 arch_lower not in TEMPLATES: | ||
| arch_lower = "x64" | ||
|
|
||
| template_name = TEMPLATES[arch_lower].get(shellcode_type, "linux_x64_shell.bin") |
There was a problem hiding this comment.
Suggestion: The fallback shellcode template is hardcoded to an x64 file, so selecting x86 or arm with an unknown shellcode type silently loads the wrong architecture payload. Use an architecture-specific default to avoid generating invalid PoCs. [logic error]
Severity Level: Major ⚠️
- ⚠️ x86/ARM PoC generation can select wrong payload template.
- ⚠️ Generated exploit scripts become unreliable for non-x64 targets.
Suggested change
| template_name = TEMPLATES[arch_lower].get(shellcode_type, "linux_x64_shell.bin") | |
| template_name = TEMPLATES[arch_lower].get(shellcode_type, TEMPLATES[arch_lower]["reverse_shell"]) |
Steps of Reproduction ✅
1. Use the public PoC path in `exploits/exploit_bridge.py:39`
(`ExploitBridge.generate_poc`) which calls `self.generator.generate(...)` at
`exploits/exploit_bridge.py:44`.
2. Trigger buffer-overflow generation with non-default kwargs (programmatic call):
`arch="x86"` and an unknown `shellcode_type` (accepted via `**kwargs` in
`exploits/poc_generator.py:118`).
3. Execution reaches `exploits/poc_generator.py:51`, where unknown type falls back to
hardcoded `"linux_x64_shell.bin"` even though `arch_lower=="x86"`.
4. Generated PoC embeds x64 template selection for an x86 request, producing
architecture-mismatched payload content/path and invalid exploit script behavior.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** exploits/poc_generator.py
**Line:** 51:51
**Comment:**
*Logic Error: The fallback shellcode template is hardcoded to an x64 file, so selecting `x86` or `arm` with an unknown shellcode type silently loads the wrong architecture payload. Use an architecture-specific default to avoid generating invalid PoCs.
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.| 4. Remediation Recommendations | ||
| """ | ||
| return await self.ollama.generate(prompt) | ||
| return await self.llm_client.generate(prompt) |
There was a problem hiding this comment.
Suggestion: generate_report returns the raw result of llm_client.generate, but that client can return None when AI is disabled or unavailable. Downstream callers treat this as a string (slice/write/markdown), which will raise runtime errors. Return a guaranteed string fallback. [type error]
Severity Level: Critical 🚨
- ❌ run-agent CLI crashes during report save/preview.
- ⚠️ Streamlit report rendering can receive invalid content.
Suggested change
| return await self.llm_client.generate(prompt) | |
| report = await self.llm_client.generate(prompt) | |
| return report or "Report generation failed: AI service unavailable." |
Steps of Reproduction ✅
1. Set `AI_ENABLED=false` (read in `modules/ai/ai_integration.py:34`), which makes
`LocalAIClient.generate()` return `None` at `modules/ai/ai_integration.py:73-77`.
2. Run agent flow (`neurorift_main.py` run-agent path) where report generation is invoked
at `neurorift_main.py:1273` via `vf.scribe.generate_report(...)`.
3. `NRScribe.generate_report()` returns raw nullable value from
`modules/ai/agents.py:196`, so `report` becomes `None`.
4. CLI then executes `f.write(report)` at `neurorift_main.py:1277` and `report[:1000]` at
`neurorift_main.py:1280`, both requiring a string and crashing with `TypeError`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** modules/ai/agents.py
**Line:** 196:196
**Comment:**
*Type Error: `generate_report` returns the raw result of `llm_client.generate`, but that client can return `None` when AI is disabled or unavailable. Downstream callers treat this as a string (slice/write/markdown), which will raise runtime errors. Return a guaranteed string fallback.
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.| self.prompt_dir = prompt_dir | ||
| self.ollama = OllamaClient() | ||
| self.llm_client = LocalAIClient() | ||
| self.prompts = self._load_prompts() |
There was a problem hiding this comment.
Suggestion: The orchestrator uses orchestrator.analyzer in CLI flow, but the constructor no longer initializes that attribute. This will raise an AttributeError when --analyze-nmap is used. Initialize the analyzer with the same LLM client during orchestrator setup. [possible bug]
Severity Level: Major ⚠️
- ❌ `--analyze-nmap` CLI path crashes immediately.
- ⚠️ AI scan-analysis workflow unavailable from this module CLI.
Suggested change
| self.prompts = self._load_prompts() | |
| self.analyzer = AIAnalyzer(self.llm_client) |
Steps of Reproduction ✅
1. Run `python modules/ai/ai_integration.py --analyze-nmap <file>` to enter the CLI path
in `modules/ai/ai_integration.py:545-568`.
2. The script constructs `AIOrchestrator(Path(args.prompt_dir))` at
`modules/ai/ai_integration.py:544`, which executes `AIOrchestrator.__init__` at
`modules/ai/ai_integration.py:138-142`.
3. `__init__` initializes `self.llm_client` but never defines `self.analyzer`
(`modules/ai/ai_integration.py:140-142`).
4. The `--analyze-nmap` branch calls `orchestrator.analyzer.analyze_nmap_output(...)` at
`modules/ai/ai_integration.py:567`, raising `AttributeError: 'AIOrchestrator' object has
no attribute 'analyzer'`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** modules/ai/ai_integration.py
**Line:** 401:401
**Comment:**
*Possible Bug: The orchestrator uses `orchestrator.analyzer` in CLI flow, but the constructor no longer initializes that attribute. This will raise an `AttributeError` when `--analyze-nmap` is used. Initialize the analyzer with the same LLM client during orchestrator setup.
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.| action="store_true", | ||
| help="Enable the advanced multi-prompt AI pipeline.", | ||
| ) | ||
| parser.add_argument( |
There was a problem hiding this comment.
Suggestion: The AI pipeline branch reads args.target, but the parser never defines a --target argument. Accessing it will raise AttributeError as soon as pipeline mode is used. Add the missing CLI argument. [possible bug]
Severity Level: Critical 🚨
- ❌ AI pipeline mode cannot start successfully.
- ⚠️ Prompt orchestration path blocked in module CLI.
Suggested change
| parser.add_argument( | |
| parser.add_argument( | |
| "--target", | |
| help="Target description for the AI pipeline task.", | |
| ) |
Steps of Reproduction ✅
1. In `modules/ai/ai_integration.py` parser setup (`lines 523-541`), `--ai-pipeline`
exists but no `--target` argument is defined.
2. Run `python modules/ai/ai_integration.py --ai-pipeline` to enter the pipeline branch at
`modules/ai/ai_integration.py:571`.
3. The code accesses `args.target` at `modules/ai/ai_integration.py:572` and again at
`583`.
4. Because argparse namespace lacks `target`, execution raises `AttributeError` before
pipeline task execution.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** modules/ai/ai_integration.py
**Line:** 536:536
**Comment:**
*Possible Bug: The AI pipeline branch reads `args.target`, but the parser never defines a `--target` argument. Accessing it will raise `AttributeError` as soon as pipeline mode is used. Add the missing CLI argument.
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 args.test_connection: | ||
| if orchestrator.ollama.is_available(): | ||
| if orchestrator.llm_client.is_available(): |
There was a problem hiding this comment.
Suggestion: is_available is async, but it is called without awaiting in the CLI connection test. This produces a coroutine object instead of a boolean and makes the check behave incorrectly. Run it through the event loop. [possible bug]
Severity Level: Major ⚠️
- ⚠️ `--test-connection` can report false AI readiness.
- ⚠️ Runtime warnings from un-awaited coroutine objects.
Suggested change
| if orchestrator.llm_client.is_available(): | |
| if asyncio.run(orchestrator.llm_client.is_available()): |
Steps of Reproduction ✅
1. Run `python modules/ai/ai_integration.py --test-connection` to execute the branch at
`modules/ai/ai_integration.py:546-551`.
2. `LocalAIClient.is_available` is declared `async def` at
`modules/ai/ai_integration.py:37`.
3. The condition `if orchestrator.llm_client.is_available():` at
`modules/ai/ai_integration.py:547` evaluates a coroutine object, not a boolean result.
4. The check reports incorrect readiness behavior (and can emit "coroutine was never
awaited"), so connection status is not actually verified.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** modules/ai/ai_integration.py
**Line:** 547:547
**Comment:**
*Possible Bug: `is_available` is async, but it is called without awaiting in the CLI connection test. This produces a coroutine object instead of a boolean and makes the check behave incorrectly. Run it through the event loop.
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.|
|
||
| elif args.list_models: | ||
| models = orchestrator.ollama.list_models() | ||
| models = orchestrator.llm_client.list_models() |
There was a problem hiding this comment.
Suggestion: list_models is async but is used as a normal value, so models becomes a coroutine and iteration fails at runtime. Execute the coroutine before iterating. [possible bug]
Severity Level: Major ⚠️
- ❌ `--list-models` command crashes at runtime.
- ⚠️ Users cannot inspect configured local model.
Suggested change
| models = orchestrator.llm_client.list_models() | |
| models = asyncio.run(orchestrator.llm_client.list_models()) |
Steps of Reproduction ✅
1. Run `python modules/ai/ai_integration.py --list-models` to trigger the branch at
`modules/ai/ai_integration.py:558-563`.
2. `LocalAIClient.list_models` is async (`modules/ai/ai_integration.py:56`), so line 559
stores a coroutine.
3. The loop `for model in models:` at `modules/ai/ai_integration.py:561` attempts
iterating that coroutine object.
4. Python raises a runtime type error (`'coroutine' object is not iterable`), so model
listing fails.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** modules/ai/ai_integration.py
**Line:** 559:559
**Comment:**
*Possible Bug: `list_models` is async but is used as a normal value, so `models` becomes a coroutine and iteration fails at runtime. Execute the coroutine before iterating.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI Incremental review completed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Motivation
neuroriftpackage with basic components (core, execution, skills, sessions, memory, tools, storage, gateway, channels) to enable modular development and packaging.Description
model_capability_check.pyto query an Ollama model with a capability prompt and parse a JSON capability response viaverify_model_capabilities.runtime_environment_check.pyto probe required runtime tools and report a structuredverify_runtime_environmentresult.neuroriftpackage skeleton containing many lightweight/stub modules (core, execution, skills, sessions, memory, models, tools, gateway, utils, skill_store, etc.) and a samplerecon_scannerskill to support skill installation and runtime loading.SkillManager/SkillInstaller/SkillLoader/SkillRegistryandClawHubClientintegration to install and run local example skills.neurorift_main.pyto integrate the skill system, addrun-agentCLI handling, and replace previous tool checks with a hardened command runner (_run_command_secure) that enforces restricted env, argument sanitization, working-directory checks, resource limits, output sanitization, retry logic, and optional firejail sandboxing.neurorift_cli.py, launcher scriptneurorift_launch.sh, and updatedsetup.pyto include the new package and entrypoint change toneurorift_cli:main.Testing
python -m compileall .across the repository, which completed without syntax errors.python3 runtime_environment_check.pyandpython3 model_capability_check.py --model <model>in CI-like environment to verify expected failure and success paths for missing tools and Ollama availability, and both scripts returned structured JSON as designed.SkillManager,_run_command_secure, and model capability parsing logic.Codex Task
Summary by CodeRabbit
CodeAnt-AI Description
Switch NeuroRift to llama.cpp-based AI and add safer autonomous running
What Changed
run-agentcommand that checks the runtime and model before starting autonomous execution, and stops early if the environment is not readyImpact
✅ Fewer startup failures when the local AI server is missing✅ Faster access to autonomous agent mode after environment checks pass✅ Less lost context during long-running sessions💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.