Repository navigation
fix(ea1): reject footnote-legend '*', bound pre-colon gap, detect YAML block-list and JSON wildcard grants - #447
yashrajp22 wants to merge 58 commits into
Conversation
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Requesting changes: the new same-line list branch introduces an EA1 false positive when an explicit tool object has a nested argument value of "*"; the wildcard must be a top-level tool-list element. The focused suite passes, but this head has no hosted checks, and the feature commit also lacks the required DCO sign-off. Please add the nested-object regression, narrow the match, and rerun signed CI.
…ck-list and JSON wildcard grants Fixes #444. Fixes #445. Follow-up to #405/#417. Remaining false positives (#444): - A bare '*' now counts only when it ends the line (optionally ']' and/or a '#' comment), so footnote legends like 'Tools: * = requires auth' no longer fire. Quoted '*' stays unambiguous as a scalar or top-level list element. - The gap before the colon is bounded to the same line ([ \t]*:), so a blank line followed by a markdown definition-list ': *' can no longer bridge paragraphs. Detection gaps (#445): - The key may be quoted, catching JSON forms: "tools": ["*"] and "permissions": "*". - New block-list branch catches the idiomatic YAML form (tools: newline '- "*"'), bounded to a single newline with a standalone-star item so markdown lists of bold/italic names cannot collide. - A quoted '*' as a later top-level element of a same-line list now matches (tools: ["search", "*"]). The list branch excludes braces so a '*' nested inside an explicit tool object (tools: [{name: grep, pattern: "*"}]) is treated as an argument value for a named tool, not a wildcard grant. Validated against a 49-case matrix (20 genuine grant forms, 11 new detections, 18 false-positive classes); full suite 2966 passed, ruff check/format clean. Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
6a2a7d4 to
a6b1d91
Compare
|
All three points addressed in the rewritten head ( 1. Nested-object false positive — fixed and regression-tested. The same-line list branch now excludes
One documented non-detection this implies: a top-level 2. DCO — the branch was rewritten with 3. CI — the branch is rebased onto current |
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Re-review: the rewritten head is signed, rebased directly onto current main, and all hosted checks are green. It fixes the reported nested-object case, and the focused and relevant static-pattern suites pass. However, the same list regex still descends into nested arrays because [ remains allowed in its scan: tools: ["search", ["grep", "*"]] still emits EA1/MEDIUM even though * is not a top-level tool-list element. Please prevent traversal into nested arrays and add this negative regression; the existing review thread remains open.
The same-line list branch excluded '{'/'}' but still allowed '[', so a
quoted '*' inside a nested array (tools: ["search", ["grep", "*"]])
was treated as a top-level tool-list element. Exclude '[' from the scan
as well and document the quoted-delimiter ceiling. Adds the nested-array
and nested-arguments-object negative regressions from review.
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
|
Fixed in
Local on the rebased head: 3,995 passed / 0 failed ( |
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Reviewed current head 375eafe12a76f073efbc1167319e7bb8636c7a59. The prior nested-object and nested-array EA1 false positives are both resolved by the current delimiter handling, with regressions for each shape. I found no remaining required code or test change.
This head was refreshed by the bot during review and has no hosted check results yet. The PR is currently blocked, and the outdated review thread should be resolved before merging.
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Corrective re-review of current head 3f6f63a3cd1d104dcb35fd1d7858d86461aaaacf. A required EA1 correctness issue remains: the inline-list branch treats a quoted wildcard embedded inside a named tool string as a top-level unrestricted-tool grant. Please make the matcher element/quote-aware and add the negative regression described inline.
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Re-reviewed current head f66907ab24d01c7acd9911d81eb9b9331959ff6f after the latest main synchronization. Both PR-owned blobs are byte-identical to reviewed head 3f6f63a3cd1d104dcb35fd1d7858d86461aaaacf; the current inline thread remains unresolved and has no reply. The blocker therefore remains: the same-line list regex is not quote-aware and still treats the embedded '*' in tools: ["grep --include='*'"] as a top-level unrestricted-tool grant. Make the matcher element/quote-aware and add that negative regression.
This head has no hosted check results and is not merge-ready.
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Re-reviewed current head e92374e86a91d709c02a1261bc3e2ead9b42d818 after the marked review at f66907ab24d01c7acd9911d81eb9b9331959ff6f. Both PR-owned blobs are byte-for-byte unchanged. The earlier nested-object and nested-array false positives remain fixed.
The current blocker at static_patterns_excessive_agency.py:61 remains: the inline-list regex is not quote-aware and treats the embedded '*' in tools: ["grep --include='*'"] as a top-level unrestricted-tool grant. Make list matching element/quote-aware and add this negative regression. The current inline thread already contains the exact case.
No required checks are reported, unresolved review threads remain, and GitHub reports mergeStateStatus=BLOCKED.
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Re-reviewed exact head aeb4a66eb159dcb224a5d6260f9da3d56a6b5d11. The commits since the last marked review only synchronize main; the PR-owned matcher and regression file retain the same current blocker.
The inline-list branch is still not quote-aware. In tools: ["grep --include='*'"], [^\\][{}\\r\\n]* can consume the surrounding scalar and treat its embedded quoted '*' as a top-level unrestricted-tool element. Parse or narrowly match actual top-level list elements and add this scalar-string negative regression. The existing unresolved inline thread has the exact line and reproduction, so I have not duplicated it.
No exact-head hosted checks are attached. The correctness fix, regression, and unresolved conversation remain required before approval.
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Re-reviewed exact head af99a9f41c306ebc218360fd9028e3e87d894a08. The intervening commits only synchronize merged #421/main content; both PR-owned blobs are byte-identical to the prior reviewed head. The earlier nested-object and nested-array false positives remain fixed.
The current blocker remains at src/skillspector/nodes/analyzers/static_patterns_excessive_agency.py:61: the inline-list branch is not quote-aware. In tools: ["grep --include='*'"], the broad character class consumes the surrounding scalar and treats its embedded quoted '*' as a top-level unrestricted-tool element. Parse or narrowly match actual top-level list elements and add this scalar-string negative regression. The existing current inline thread records the exact reproduction, so I have not duplicated it.
No exact-head hosted checks are attached, the current thread remains unresolved, and GitHub now reports a merge conflict (CONFLICTING/DIRTY). The correctness fix, regression, and conflict resolution are required before approval.
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
@yashrajp22 thank you for the careful edge-case matrix behind #444 and #445. The nested-object and nested-array fixes have held up well.
Value and readiness
The PR still fills real EA1 gaps. Main's wildcard pattern is unchanged apart from #491 moving it into EA1_CODE_PATTERNS, and #444 and #445 are still open. It is not mergeable yet: the quote-awareness blocker is unchanged, and the branch conflicts with main.
Material findings
Prior findings
- Nested-object false positive: resolved.
- Nested-array false positive: resolved.
- DCO / hosted checks: DCO is resolved (both commits are signed). The only exact-head CI run (35334370803) is
action_required, which is awaiting approval and is not a defect. - Blocker,
src/skillspector/nodes/analyzers/static_patterns_excessive_agency.py:61: the inline-list branch is not quote-aware (tools: ["grep --include='*'"]). Still open. Nothing has changed and there has been no reply since the Sep 12 thread. - Blocker, merge conflict: still open, and it now has structure.
- #491 (
175df11e) split EA1 intoEA1_CODE_PATTERNS, matched withre.finditer, andEA1_PROSE_PATTERNS, which are paragraph-bounded throughiter_paragraph_matches. #658 has since edited the same file. - When rebasing, put both new wildcard patterns in
EA1_CODE_PATTERNS, next to theshell/file_writepattern that #491 moved there. - Do not restore the single
EA1_PATTERNSlist from this branch, because that would undo main's code/prose split.
- #491 (
The PR-owned lines have no new issues. I found no test added to main since the branch point that relies on the old wildcard regex. test_static_code_blank_lines exercises only EA1's shell/http pattern, which this PR leaves unchanged.
PIC tradeoffs
None identified.
Verification and gaps
- Re-read both PR files at
af99a9f4, all rng1995 reviews and the author's replies. Every commit afterd44a3f4is a bot merge ofmain. git merge-treeagainst530d3c5creports a content conflict instatic_patterns_excessive_agency.py. I inspected the conflict hunk and main'sanalyze()dispatch.- Traced the
grep --include='*'example through the line-61 alternative by hand. - Nothing was executed.
…rd-remaining-gaps Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com> # Conflicts: # src/skillspector/nodes/analyzers/static_patterns_excessive_agency.py
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Named tool commands containing quoted stars, such as
grep --include='*', are now kept separate from unrestricted wildcard grants. The matcher respects complete comma-separated values, quotes, and escapes, while nested objects and arrays remain excluded.Validation: 297 focused tests passed from source and a fresh installed wheel. The new cases reproduce 11 failures on the previous code; genuine wildcard grants remain detected.