fix(ea1): bound wildcard-tool-access match to a single line and a sta… - #417
Conversation
…ndalone asterisk
Two independent EA1 false positives, both confirmed against the official
anthropics/skills repo (mcp-builder/SKILL.md):
- The pattern used bare \s* between the colon and the expected wildcard
value. Python's \s matches newlines, so the gap could span a blank line
and bridge two unrelated headings ("For each tool:" + blank line +
"**Input Schema:**"). Changed to [ \t]* so the match is bounded to a
single line.
- Nothing required the matched * to be a standalone token, so the first
* of a closing ** bold-markdown span satisfied it ("**API Coverage vs.
Workflow Tools:**" matched as "Tools:*"). Added a negative lookahead
(?!\*|\w) so the asterisk must not be immediately followed by another
asterisk or a word character.
New test file added (test_ea1_wildcard_line_boundary.py) covering both
false-positive cases plus regression tests confirming genuine single-line
wildcard grants (tools: "*", tools: [*], permissions: '*', tools: *)
still fire.
Testing:
- make test (unit + integration): 2804 passed, 31 passed, 0 failed
- make lint: clean
- make format: clean
Closes NVIDIA#405
Signed-off-by: Benedict Kwok <bkwok.oracle@gmail.com>
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]\n\nApproved. The EA1 boundary change correctly limits whitespace to the current line and rejects wildcard prefixes while preserving true wildcard declarations. The dedicated regression matrix is strong. This overlaps PR #414, so maintainers should merge only the preferred implementation. Current required checks pass.
|
[SkillSpector Maintainer] Selecting this implementation over #414. Both PRs apply the same production regex correction, but #417 carries the broader focused regression matrix: it also covers a single-newline boundary, an unquoted wildcard grant, and bolded named-tool prose. The reviewed head is clean and all required checks pass. Merged via the repository's squash convention. Thank you @benedictkwok for the fix, the issue-specific test coverage, and the clear validation notes. |
2 participants
…ndalone asterisk
Two independent EA1 false positives, both confirmed against the official anthropics/skills repo (mcp-builder/SKILL.md):
The pattern used bare \s* between the colon and the expected wildcard value. Python's \s matches newlines, so the gap could span a blank line and bridge two unrelated headings ("For each tool:" + blank line + "Input Schema:"). Changed to [ \t]* so the match is bounded to a single line.
Nothing required the matched * to be a standalone token, so the first
New test file added (test_ea1_wildcard_line_boundary.py) covering both false-positive cases plus regression tests confirming genuine single-line wildcard grants (tools: "", tools: [], permissions: '*', tools: *) still fire.
Testing:
Closes #405