Conversation
Greptile SummaryThis PR introduces two new agent tools — However, three issues require attention:
Confidence Score: 2/5
Last reviewed commit: a157086 |
strix/tools/skills/skills_actions.py
Outdated
| validation = validate_skill_names(skill_list) | ||
| invalid_skills = validation.get("invalid", []) | ||
| valid_skills = validation.get("valid", []) |
There was a problem hiding this comment.
The documented input format for load_skills allows category-prefixed paths (e.g., "vulnerabilities/sql_injection"), as stated in the schema on line 84:
You can specify skills by name only (e.g., "sql_injection") or with category path (e.g., "vulnerabilities/sql_injection")
However, validate_skill_names (in strix/skills/__init__.py line 43) only validates against bare skill stems returned by get_all_skill_names(). Any path-formatted input is silently rejected and placed in invalid_skills before the actual load_skills_content function (which can resolve paths) ever sees it.
To fix: Normalize path-format names before validation, or update validate_skill_names to accept both forms:
def _bare_name(s: str) -> str:
return s.split("/")[-1]
validation = validate_skill_names([_bare_name(s) for s in skill_list])
invalid_bare = set(validation.get("invalid", []))
valid_skills = [s for s in skill_list if _bare_name(s) not in invalid_bare]
invalid_skills = [s for s in skill_list if _bare_name(s) in invalid_bare]Prompt To Fix With AI
This is a comment left during a code review.
Path: strix/tools/skills/skills_actions.py
Line: 150-152
Comment:
The documented input format for `load_skills` allows category-prefixed paths (e.g., `"vulnerabilities/sql_injection"`), as stated in the schema on line 84:
> You can specify skills by name only (e.g., "sql_injection") or with category path (e.g., "vulnerabilities/sql_injection")
However, `validate_skill_names` (in `strix/skills/__init__.py` line 43) only validates against bare skill stems returned by `get_all_skill_names()`. Any path-formatted input is silently rejected and placed in `invalid_skills` before the actual `load_skills_content` function (which can resolve paths) ever sees it.
**To fix:** Normalize path-format names before validation, or update `validate_skill_names` to accept both forms:
```python
def _bare_name(s: str) -> str:
return s.split("/")[-1]
validation = validate_skill_names([_bare_name(s) for s in skill_list])
invalid_bare = set(validation.get("invalid", []))
valid_skills = [s for s in skill_list if _bare_name(s) not in invalid_bare]
invalid_skills = [s for s in skill_list if _bare_name(s) in invalid_bare]
```
How can I resolve this? If you propose a fix, please make it concise.| BLACK-BOX TESTING - PHASE 1 (RECON & MAPPING): | ||
|
|
||
| When spawning testing agents: call list_skills first to get valid skill names, then create_agent with skills param matching recon findings. | ||
|
|
||
| BLACK-BOX - PHASE 1: |
There was a problem hiding this comment.
The insertion of the skill-loading instruction splits the "BLACK-BOX TESTING - PHASE 1" heading into two separate headings with different names:
BLACK-BOX TESTING - PHASE 1 (RECON & MAPPING):(line 192)BLACK-BOX - PHASE 1:(line 196)
This creates a confusing structure that may cause the model to treat them as distinct phases. Merge the new instruction into the existing section:
BLACK-BOX TESTING - PHASE 1 (RECON & MAPPING):
When spawning testing agents: call list_skills first to get valid skill names, then create_agent with skills param matching recon findings.
- COMPLETE full reconnaissance: subdomain enumeration, port scanning, service detection
- MAP entire attack surface: ...
Prompt To Fix With AI
This is a comment left during a code review.
Path: strix/agents/StrixAgent/system_prompt.jinja
Line: 192-196
Comment:
The insertion of the skill-loading instruction splits the "BLACK-BOX TESTING - PHASE 1" heading into two separate headings with different names:
- `BLACK-BOX TESTING - PHASE 1 (RECON & MAPPING):` (line 192)
- `BLACK-BOX - PHASE 1:` (line 196)
This creates a confusing structure that may cause the model to treat them as distinct phases. Merge the new instruction into the existing section:
```
BLACK-BOX TESTING - PHASE 1 (RECON & MAPPING):
When spawning testing agents: call list_skills first to get valid skill names, then create_agent with skills param matching recon findings.
- COMPLETE full reconnaissance: subdomain enumeration, port scanning, service detection
- MAP entire attack surface: ...
```
How can I resolve this? If you propose a fix, please make it concise.
strix/tools/skills/skills_actions.py
Outdated
| return { | ||
| "success": len(loaded_content) > 0, | ||
| "loaded_skills": loaded_content, | ||
| "loaded_count": len(loaded_content), | ||
| "invalid_skills": invalid_skills, | ||
| "warnings": warnings, | ||
| } |
There was a problem hiding this comment.
When all requested skills are invalid (i.e., all fail validate_skill_names), this function returns {"success": False, ...} without an "error" key. This breaks the response consistency pattern — every other failure path in this file and in list_skills includes an "error" key.
Downstream code that checks result["error"] will raise KeyError. While a warning is added to the warnings list, callers expect a dedicated error key for failure responses.
Fix: Add an error key when success is False and no skills loaded:
result: dict[str, Any] = {
"success": len(loaded_content) > 0,
"loaded_skills": loaded_content,
"loaded_count": len(loaded_content),
"invalid_skills": invalid_skills,
"warnings": warnings,
}
if not result["success"] and not loaded_content:
result["error"] = (
"No skills could be loaded. "
+ (warnings[0] if warnings else "Check skill names with list_skills.")
)
return resultPrompt To Fix With AI
This is a comment left during a code review.
Path: strix/tools/skills/skills_actions.py
Line: 170-176
Comment:
When all requested skills are invalid (i.e., all fail `validate_skill_names`), this function returns `{"success": False, ...}` without an `"error"` key. This breaks the response consistency pattern — every other failure path in this file and in `list_skills` includes an `"error"` key.
Downstream code that checks `result["error"]` will raise `KeyError`. While a warning is added to the `warnings` list, callers expect a dedicated error key for failure responses.
**Fix:** Add an error key when success is False and no skills loaded:
```python
result: dict[str, Any] = {
"success": len(loaded_content) > 0,
"loaded_skills": loaded_content,
"loaded_count": len(loaded_content),
"invalid_skills": invalid_skills,
"warnings": warnings,
}
if not result["success"] and not loaded_content:
result["error"] = (
"No skills could be loaded. "
+ (warnings[0] if warnings else "Check skill names with list_skills.")
)
return result
```
How can I resolve this? If you propose a fix, please make it concise.
Issue Link: #280
added load_skills and list_skills tools + added context in the system prompt so strix can dynamically choose to load a new skill when required
this will allow for the agent to add in exact context for a vulnerability type right before acting
below check out strix calling the new tools in action
e.g.

