Skip to content

feat(analyzer): reduce false positive for emoji-zwj-sequence#260

Open
badnoodle wants to merge 3 commits into
NVIDIA:mainfrom
badnoodle:dev
Open

feat(analyzer): reduce false positive for emoji-zwj-sequence#260
badnoodle wants to merge 3 commits into
NVIDIA:mainfrom
badnoodle:dev

Conversation

@badnoodle

Copy link
Copy Markdown

Fixes #259 , check the valid Emoji ZWJ sequences to reduce false positive

Signed-off-by: Yangnian <ydyangnian@163.com>

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

[Automated SkillSpector Review]

Requesting changes. The emoji carve-out misses common valid ZWJ sequences containing skin-tone modifiers or VS16, and the PR adds no regressions. Handle the full emoji sequence around ZWJ, add representative positive/negative tests, declare regex as a direct dependency, and run formatting.

Comment thread src/skillspector/nodes/analyzers/common.py
… unit test

Signed-off-by: Yangnian <ydyangnian@163.com>

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

[Automated SkillSpector Review]

Re-review of head d2dd3e6. The PR carves ZWJ (U+200D) out of the P2 hidden-instruction rule when it joins a valid emoji sequence (fixes #259). The new commit substantially addresses the sequence-handling feedback and adds good test coverage, but one prior blocker remains open and the new code introduces a crash on attacker-controllable input.

Prior-issue resolution checklist

Prior issue Status
Handle full emoji sequence around ZWJ (Emoji_Modifier, FE0F), e.g. 👩🏽‍💻, ❤️‍🔥 Resolvedis_emoji_zwj_sequence now walks left past U+1F3FB–U+1F3FF and FE0E/FE0F to the \p{Extended_Pictographic} base; tests cover those exact emoji
Representative positive/negative tests incl. non-emoji ZWJ staying detectable Resolved — added in tests/unit/test_patterns.py and tests/nodes/analyzers/test_static_patterns.py, incl. ignore\u200dall system\u200d\u200dprompt still yielding P2
Declare regex as a direct dependency Not resolved — it was added to the dev optional-dependency group in pyproject.toml, but common.py (runtime code imported by every analyzer) does import regex at module level. A plain pip install skillspector now fails at import time. Move it to [project] dependencies
Run formatting Not resolved — leftover debug print(findings) in a test, test-name typo seuquence, inline-comment style, fullwidth colon in a comment, missing return-type annotations

New blockers

  1. Crash on ZWJ near start of file (src/skillspector/nodes/analyzers/common.py). _read returns "" for out-of-range indexes, but _check_modifier/_check_selector call ord(cp) unconditionally, and ord("") raises TypeError. Reproduced: content starting with \u200d (ZWJ at index 0), or a modifier at index 0 followed by ZWJ, crashes is_emoji_zwj_sequence. Neither analyze() nor static_runner.run_static_patterns() catches analyzer exceptions, so a skill file beginning with a ZWJ takes down the entire prompt-injection node — an attacker can prepend one character to suppress all P1–P4 scanning. Guard the empty-string case (e.g. return False from the _check_* helpers when cp is empty) and add a test with ZWJ as the first character of the file.
  2. Runtime ImportError in production installs (same as unresolved prior issue above): regex must move from dev extras to [project] dependencies.

Non-blocking nits

  • _check_emoji_zwj_sequence: content[match.start() : match.end()] is identical to match.group(0); the condition can simply be match.group(0) != "\u200d". Prefer conventional operand order over Yoda conditions, and add -> bool annotations (repo convention).
  • tests/nodes/analyzers/test_static_patterns.py: remove debug print(findings); fix test-name typo test_p2_emoji_zwj_seuquence_still_keep_detectable; zwj_with_modifier actually contains no skin-tone modifier (VS16 only) — rename or use a modifier-bearing sequence.
  • Consider precompiling the \p{Extended_Pictographic} pattern at module level instead of per-character regex.match calls.

Once the crash is fixed (with a boundary test) and regex is a runtime dependency, this looks mergeable.

Note for maintainers: PR #264 fixes the same issue #259 with a stdlib-only carve-out in the same hunk — the two PRs conflict and only one should land.

Comment thread src/skillspector/nodes/analyzers/common.py Outdated
Comment thread pyproject.toml Outdated
Comment thread src/skillspector/nodes/analyzers/static_patterns_prompt_injection.py Outdated
Comment thread tests/nodes/analyzers/test_static_patterns.py Outdated
… zero index test; add crash problem; precompile pattern; move regex to project level; and all relative test passed.

Signed-off-by: Yangnian <ydyangnian@163.com>
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.

Static analyzers flag emoji-zwj-sequence as Prompt Injection

2 participants