feat: add LLM semantic security review for SKILL.md scanner

[Benkapner]

Summary

Adds LLM-based semantic security review to the SKILL.md scanner, complementing the deterministic regex checks from PR #38. Adapted from harness-eval-lab's security review rubric.

Depends on PR #38 (deterministic security gate) being merged first.

What the LLM checks

Three checks that catch attacks regex cannot detect:

Check	What it catches
Anti-jailbreak	Self-declared safety claims ("this skill is verified safe"), evaluation bypass attempts ("ignore security warnings")
Semantic attacks	Polite reframings of jailbreaks, gradual escalation across sections, split-instruction attacks, conditional triggers
Description-behavior mismatch	Skill described as "code formatter" but instruction references network access; "documentation helper" that reads credentials

How it works

• Deterministic scan runs first (always)
• LLM review runs after, appending findings to the same JSON with source: "llm"
• Uses llm_client.chat_completion() (same as quality review)
• On by default, matching Cisco's use-llm=true convention
• Disable with --no-llm flag
• If the LLM call fails, deterministic results are preserved

Changes

File	What
abevalflow/security/skillmd_scanner.py	Added SECURITY_REVIEW_PROMPT and llm_security_review() function
scripts/skillmd_security_scan.py	Added --no-llm flag, calls LLM review by default
pipeline/tasks/phases/test.yaml	Updated step 3 to pass LLM env vars and use-llm flag

Test plan

• All 72 existing tests pass (deterministic scanner + gate + existing gate registry)
• ruff lint + format clean
• Manual: run with LLM against a test submission with semantic attack patterns

Ref: QEMETRICS-2414">

[Benkapner]

Correction: This PR is tracked under QEMETRICS-2415 (not QEMETRICS-2414). The branch name references QEMETRICS-2414 because it was created before the separate ticket existed.

Depends on PR #38 being merged first (the LLM review adds to the files that PR creates).

[GuyZivRH]

Consolidated Review: LLM Semantic Security Review

Good work on extending the SKILL.md scanner with LLM-based semantic review. The design is sound — deterministic scan runs first, LLM review appends findings with source: "llm", and failures preserve deterministic results. A few issues need addressing before merge.

---

Must Fix (Blocking)

1. Rebase on latest main

The PR has conflicts with the lint cleanup (#41) that was merged. Please rebase:

git fetch origin
git rebase origin/main
git push --force-with-lease

2. LLM JSON parsing drops findings wrapped in markdown fences

File: abevalflow/security/skillmd_scanner.py (llm_security_review)

LLMs commonly return JSON wrapped in markdown fences like:

```json
[{"check": "anti_jailbreak", ...}]
```

The code calls json.loads(response) directly, which fails on fenced output. All semantic findings are silently dropped.

Fix: Add fence stripping before parsing:

def _extract_json(response: str) -> str:
    """Strip markdown fences if present."""
    response = response.strip()
    if response.startswith("```"):
        lines = response.split("\n")
        lines = [l for l in lines if not l.startswith("```")]
        response = "\n".join(lines).strip()
    return response

# Then use:
llm_findings = json.loads(_extract_json(response))

3. Normalize severity values to prevent block bypass

File: abevalflow/gates/security/base.py (evaluate_scan_json)

Severity from LLM output isn't normalized. Values like "critical " (trailing space) are treated as invalid and downgraded to info, causing block mode to incorrectly pass.

Fix: Add .strip() when parsing severity:

sev_str = f.get("severity", "info").lower().strip()

4. Add tests for the LLM security review path

File: tests/test_skillmd_scanner.py

The LLM review is enabled by default but has zero test coverage. Please add:

class TestLLMSecurityReview:
    def test_valid_findings(self, tmp_path, mocker):
        """LLM returns valid JSON findings."""
        mock_response = '[{"check": "anti_jailbreak", "severity": "high", "message": "test", "file_path": "SKILL.md"}]'
        mocker.patch("abevalflow.security.skillmd_scanner.llm_client.chat_completion", return_value=mock_response)
        (tmp_path / "SKILL.md").write_text("content")
        findings = llm_security_review(tmp_path)
        assert len(findings) == 1
        assert findings[0]["source"] == "llm"

    def test_fenced_json_parsed(self, tmp_path, mocker):
        """LLM returns JSON wrapped in markdown fences."""
        mock_response = '```json\n[{"check": "semantic_attack", "severity": "critical", "message": "test", "file_path": "SKILL.md"}]\n```'
        mocker.patch("abevalflow.security.skillmd_scanner.llm_client.chat_completion", return_value=mock_response)
        (tmp_path / "SKILL.md").write_text("content")
        findings = llm_security_review(tmp_path)
        assert len(findings) == 1  # Should parse successfully

    def test_llm_failure_returns_empty(self, tmp_path, mocker):
        """LLM call fails, deterministic results preserved."""
        mocker.patch("abevalflow.security.skillmd_scanner.llm_client.chat_completion", side_effect=Exception("API error"))
        (tmp_path / "SKILL.md").write_text("content")
        findings = llm_security_review(tmp_path)
        assert findings == []

    def test_invalid_json_returns_empty(self, tmp_path, mocker):
        """LLM returns invalid JSON."""
        mocker.patch("abevalflow.security.skillmd_scanner.llm_client.chat_completion", return_value="not json")
        (tmp_path / "SKILL.md").write_text("content")
        findings = llm_security_review(tmp_path)
        assert findings == []

---

Should Fix (Can Be Follow-up)

5. Add content size limit for large submissions

All markdown files are concatenated into one message. Large submissions can exceed the model's context window, causing the LLM review to silently fail.

Suggested fix:

MAX_TOTAL_CHARS = 40_000  # ~10k tokens

total_chars = 0
for md_file in md_files:
    content = md_file.read_text(...)
    if total_chars + len(content) > MAX_TOTAL_CHARS:
        logger.warning("Truncating LLM review input due to size")
        break
    file_contents.append(...)
    total_chars += len(content)

6. Expand pattern allowlists or demote severity

The translate evasion pattern still flags legitimate uses like "Translate this to Italian/Arabic/Hindi". The markdown image pattern still flags shields.io badges and raw.githubusercontent URLs.

Options:

• Expand the allowlists
• Demote to MEDIUM severity (informational rather than blocking)
• Remove translate evasion entirely (the LLM check covers semantic attacks better)

---

Testing Pipeline Changes

After fixing and rebasing, verify the integration using our test script:

./scripts/misc/trigger_test_runs.sh QEMETRICS-2414/security-llm-review

This triggers 6 PipelineRuns (Harbor, ASE, A2A + their CI variants) against your branch. Monitor results with:

oc get pipelineruns -n ab-eval-flow --sort-by=.metadata.creationTimestamp | tail -10

---

What's Working Well

• Clean separation: deterministic scan always runs, LLM is additive
• Graceful degradation: LLM failure preserves deterministic results
• Consistent conventions: --no-llm matches Cisco's use-llm=true pattern
• Well-designed prompt: 3-check rubric targets attacks regex cannot detect
• Base class refactoring significantly reduces duplication
• Commit messages are clean (no AI attribution violations)

---

Let me know when the fixes are ready and I'll re-review!