Skip to content

fix(ea1): reject footnote-legend '*', bound pre-colon gap, detect YAML block-list and JSON wildcard grants - #447

Open
yashrajp22 wants to merge 58 commits into
mainfrom
yashrajp22/ea1-wildcard-remaining-gaps
Open

yashrajp22 wants to merge 58 commits into
mainfrom
yashrajp22/ea1-wildcard-remaining-gaps

Conversation

@yashrajp22

@yashrajp22 yashrajp22 commented Aug 27, 2026 •

Copy link
Copy Markdown
Collaborator

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.

@yashrajp22
yashrajp22 requested a review from rng1995 August 27, 2026 11:02

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Comment thread src/skillspector/nodes/analyzers/static_patterns_excessive_agency.py Outdated
…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>
@yashrajp22
yashrajp22 force-pushed the yashrajp22/ea1-wildcard-remaining-gaps branch from 6a2a7d4 to a6b1d91 Compare August 31, 2026 15:52
@yashrajp22

Copy link
Copy Markdown
Collaborator Author

All three points addressed in the rewritten head (a6b1d91):

1. Nested-object false positive — fixed and regression-tested. The same-line list branch now excludes {/} (\[[^\]{}\r\n]*['\"]\*['\"]), so the match can never cross into an explicit tool object: a quoted * only counts as a top-level list element. Verified:

  • tools: [{"name": "grep", "pattern": "*"}] → no longer flagged (new test)
  • tools: [{name: search, glob: "*"}] → no longer flagged (new test)
  • tools: ["search", "*"] → still flagged (regression guard)

One documented non-detection this implies: a top-level "*" after an object element (tools: [{...}, "*"]) is not matched — crossing braces correctly would need balanced-brace matching, which a regex can't do reliably, and the FP cost of skipping braces outweighs that rare form.

2. DCO — the branch was rewritten with Signed-off-by on the (single) commit.

3. CI — the branch is rebased onto current main (d3a849a) and force-pushed, so hosted checks run against a signed, up-to-date head. Local run on the rebased head: 3,969 passed / 0 failed (-m "not integration and not provider"), ruff check and ruff format --check clean.

@yashrajp22
yashrajp22 requested a review from rng1995 August 31, 2026 15:53

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

github-actions Bot and others added 2 commits August 31, 2026 16:49
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>
@yashrajp22

Copy link
Copy Markdown
Collaborator Author

Fixed in d44a3f4 (signed, stacked on the updated branch — no history rewrite this time):

  • The list scan now excludes [ as well as {/} (\[[^\][{}\r\n]*['\"]\*['\"]), so it cannot descend into nested arrays or objects — a quoted * only matches as a top-level tool-list element.
  • Negative regressions added for both reported shapes:
    • tools: ["search", ["grep", "*"]] → not flagged
    • "tools": [{"name": "search", "arguments": {"glob": "*"}}] → not flagged
  • Guard kept: tools: ["search", "*"] → still flagged.
  • Documented ceiling in the code comment: a quoted string element containing one of those delimiters ahead of a genuine top-level "*" (e.g. tools: ["search {x}", "*"]) also stops the scan — walking quoted strings correctly needs a parser, not a pattern, and the miss is strictly safer than the traversal FPs.

Local on the rebased head: 3,995 passed / 0 failed (-m "not integration and not provider"), ruff check + format --check clean.

@yashrajp22
yashrajp22 requested a review from rng1995 August 31, 2026 17:29

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Comment thread src/skillspector/nodes/analyzers/static_patterns_excessive_agency.py Outdated

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

  1. Nested-object false positive: resolved.
  2. Nested-array false positive: resolved.
  3. 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.
  4. 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.
  5. Blocker, merge conflict: still open, and it now has structure.
    • #491 (175df11e) split EA1 into EA1_CODE_PATTERNS, matched with re.finditer, and EA1_PROSE_PATTERNS, which are paragraph-bounded through iter_paragraph_matches. #658 has since edited the same file.
    • When rebasing, put both new wildcard patterns in EA1_CODE_PATTERNS, next to the shell/file_write pattern that #491 moved there.
    • Do not restore the single EA1_PATTERNS list from this branch, because that would undo main's code/prose split.

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 after d44a3f4 is a bot merge of main.
  • git merge-tree against 530d3c5c reports a content conflict in static_patterns_excessive_agency.py. I inspected the conflict hunk and main's analyze() dispatch.
  • Traced the grep --include='*' example through the line-61 alternative by hand.
  • Nothing was executed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants