fix: parse space-separated allowed-tools strings (fixes #327) - #330
fix: parse space-separated allowed-tools strings (fixes #327)#330Bimpebabs wants to merge 3 commits into
Conversation
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
[Automated SkillSpector Review]
Requesting changes. The space-separated allowed-tools behavior is correct and its focused tests pass (11 tests), but the exact patch fails the repository format gate: ruff format --check reports that src/skillspector/nodes/build_context.py would be reformatted. Please run Ruff format on that file and update the PR.
| [str(p) for p in permissions] if isinstance(permissions, list) else [] | ||
| ) | ||
| # `allowed-tools` (Agent Skills standard) — accept list or comma string. | ||
| # `allowed-tools` (Agent Skills standard) — accept list, comma string, or space-separated string. |
There was a problem hiding this comment.
Blocking CI issue: the exact head fails ruff format --check; Ruff reports this file would be reformatted. Please run ruff format src/skillspector/nodes/build_context.py and commit the result. The focused behavior tests already pass.
|
@Bimpebabs - Please address review comments, CI failures and resolve merge conflicts. |
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Re-review: still requesting changes. The prior formatting blocker remains visible in the patch and the current lint check is failing; DCO also fails. Please run the repository formatter on the touched Python files, restore green lint, and sign off the commits. The allowed-tools parsing behavior and unit tests otherwise remain sound.
2 participants
Fixes #327
What
Fixes the
allowed-toolsparsing bug where space-separated tool names were treated as a single tool, causing false positives on fully-declared skills.Why
The current parsing splits on commas but not whitespace. A declaration like
allowed-tools: Bash Readis parsed as a single tool namedBash Read, which never matches any actual tool name. This causes the LP1 check to fire on skills that have correctly declared their tools.How
build_context.py: split theallowed-toolsstring on whitespace (spaces, tabs, newlines) when no comma is present. Comma-separated strings continue to work as before.mcp_least_privilege.py: same fix in_normalize_allowed_toolsfor consistency.Testing
Manual verification
Tested on: Windows 11, Python 3.14
Steps:
allowed-tools: Bash Read(space-separated)skillspector scanon the test skillallowed-tools: Bash, Read(comma-separated) — backward compat confirmedallowed-tools: [Bash, Read](list form) — backward compat confirmedResult: PASS — space-separated parsed correctly, LP1 false positive eliminated, backward compatibility maintained.