refactor: use harness-eval CLI for SKILL.md scanning (removes ~1950 vendored lines)

[Benkapner]

Summary

Instead of continuing to manually copy security and quality rules from harness-eval (my repo) into this codebase, i created a new CLI tool in harness-eval called skill-submission-scan (PR #96) that contains all the relevant checks for agentic_eval_flow in one place.

This tool includes:

• Everything i previously added here (all 59 security regex patterns, all 12 quality checks, the file-completeness and circular reference checks)
• New rules that harness-eval already had but weren't ported yet: data exfiltration (8 patterns), reverse shell (11), trigger manipulation (4), scope grab description (5), homoglyph/RTL obfuscation detection, and 17 more tautological patterns
• False-positive fixes: "you are now a code reviewer" no longer flags (requires override qualifier), sudo apt install no longer flags (excludes safe commands), base64 mentions no longer flag (only pipe-to-shell), image URLs no longer flag (only query-param exfiltration), $HOME/.ssh/ is now caught (not just ~/)

This PR replaces the ~1950 lines of vendored scanner code with a single call to that tool. The LLM semantic security review is preserved in a small standalone module. All gates, JSON formats, and the LLM quality review stay exactly as they are.

Depends on: harness-eval v7.9.0 which has alreadt been merged.

What changed in the Tekton pipeline

The two old steps (skillmd-security-scan + skillmd-quality-scan) become one skillmd-scan step:

harness-eval skill-submission-scan "$SUBMISSION_PATH" \
  --output-security "$REPORT_DIR/skillmd-security-scan.json" \
  --output-quality "$WORKSPACE_ROOT/skillmd-quality-scan.json"

Skip conditions preserved independently:

• eval-engine=mcpchecker: skip everything (same as before)
• security-scan-mode=disabled: quality scan still runs, only --output-security is omitted

After the deterministic scan, if security-scan-use-llm=true, a small script appends LLM findings into the security JSON (same behavior as before).

What stayed untouched

• All gate modules (SkillMdScannerGate, SkillMdQualityGate, evaluate_scan_json)
• LLM quality review (test_quality_review.py / LLMReviewGate)
• Cisco AI Defense scanner step
• JSON format: {"findings": [{severity, rule_id, message, file_path, category, line}]}

Behavior parity notes

Intentional improvements from harness-eval's tighter patterns:

• Bare sudo no longer flags (excludes apt, dnf, pip, npm, etc.)
• chmod 777 / chown root on non-sensitive paths no longer flag
• generic_advice quality category has no equivalent in the submission preset
• Submissions without SKILL.md are now scanned instead of silently skipped
• "You are now a code reviewer" no longer flags (requires override qualifier)
• Base64 mentions no longer flag (only pipe-to-shell patterns)

Before merging

• Verify harness-eval v7.9.0 is available on PyPI
• Run golden-corpus diff: scan existing submissions with both old and new scanners, compare which would block
• Review behavior parity changes above with the team

Test plan

• 16 contract integration tests (harness-eval JSON format works with both gates in block/warn/missing modes)
• 5 LLM security review tests (preserved from deleted test file, updated imports)
• Pre-existing test failures are unrelated (test_publish.py, infra deps)

[GuyZivRH]

PR #73 Consolidated Review: Use harness-eval CLI for SKILL.md Scanning

PR: #73
Title: refactor: use harness-eval CLI for SKILL.md scanning (removes ~1950 vendored lines)
Author: Benjamin Kapner
Branch: refactor/harness-eval-skill-scan → main
Reviewed: 2026-08-12
Reviews Consolidated: 4

Executive Summary

This PR deletes ~2,500 lines of vendored SKILL.md security/quality scanning code and replaces the deterministic scanning with a single call to harness-eval skill-submission-scan CLI (v7.9.0). The LLM semantic security review is preserved as a standalone module (abevalflow/security/llm_review.py). The gate layer (SkillMdScannerGate, SkillMdQualityGate) is untouched — it already reads a generic {"findings": [...]} JSON shape.
Consensus verdict: Request Changes — The implementation is solid and verified, but blockers remain around commit hygiene and pre-merge validation.

Changes Overview

Area	Change
Deleted	abevalflow/security/skillmd_scanner.py (-429 lines)
Deleted	abevalflow/quality/skillmd_quality_scanner.py (-758 lines)
Deleted	scripts/skillmd_security_scan.py (-89 lines)
Deleted	scripts/skillmd_quality_scan.py (-65 lines)
Deleted	tests/test_skillmd_scanner.py (-574 lines)
Deleted	tests/test_skillmd_quality_scanner.py (-534 lines)
Added	abevalflow/security/llm_review.py (+146 lines)
Added	scripts/llm_security_review.py (+66 lines)
Added	tests/test_llm_security_review.py (+69 lines)
Added	tests/test_skillmd_scan_integration.py (+243 lines)
Modified	pipeline/tasks/phases/test.yaml (+45/-75 lines)
Net: +569/-2524 lines (removes ~1955 lines)

---

Independent Verification Performed

One reviewer independently verified the riskiest claims:

1. PyPI availability: pip index versions harness-eval confirms 7.9.0 is available ✅
2. JSON contract compatibility: Downloaded the actual wheel and confirmed:
   • --output-security / --output-quality flags exist exactly as invoked
   • JSON format {"findings": [{severity, rule_id, message, file_path, category, line}]} matches gate expectations
3. LLM review extraction: Diffed new module against deleted code — byte-for-byte identical logic
4. No dangling references: Grepped repo for deleted modules — nothing references them
5. Skip-condition parity: Confirmed both skip paths preserved (eval-engine=mcpchecker, security-scan-mode=disabled)

---

Must-Fix Issues

1. AI Co-Author in Commit (Critical — All Reviews)

Commit d42a52a contains:

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Per project rules: "Never use AI as commit author or co-author"
Fix: Strip the trailer via git filter-branch or rebase + amend.

2. Pre-Merge Checklist Incomplete (High — 3/4 Reviews)

The author's own checklist has unchecked items that matter for a security/quality gate:

• Verify harness-eval v7.9.0 is available on PyPI (verified by reviewer)
• Run golden-corpus diff — scan existing submissions with both old and new scanners
• Review behavior parity changes with team — rules are changing in both directions
  This is a real merge blocker: the blocking rules are loosening (bare sudo, chmod 777, base64 mentions) and new ones being added (data exfiltration, reverse shell, homoglyph/RTL). Without the diff, there's no evidence of what changes for real submissions.

3. Fail-Open Behavior in Scan Step (Medium — 1 Review)

The scan execution uses || true:

"${SCAN_CMD[@]}" 2>&1 || true

This causes a silent green pipeline when:

• CLI fails at runtime
• CLI crashes on edge case
• Output file not produced
  Fix: Check that expected output files exist after scan; fail the task if missing (when SCAN_MODE != disabled for security, always for quality).

---

Should-Fix Issues

4. Heavy Transitive Dependencies (Medium)

harness-eval pulls in scikit-learn>=1.0 unconditionally (numpy/scipy transitively). This is installed fresh on every pipeline run, unlike the old vendored code that only needed pydantic/pyyaml/openai.
Recommendation: Consider pre-baking harness-eval into a custom image (like the PyRIT Crescendo step uses).

5. LLM Review Append Error Handling (Low)

scripts/llm_security_review.py appends findings to an existing JSON. If harness-eval failed and the file doesn't exist, the append will fail. Consider explicit file existence check.

6. LLM Review Fail Semantics (Low)

The LLM append also runs with || true. This is probably fine (semantic review is "nice-to-have"), but should be explicitly documented as non-blocking.

Strengths (All Reviews Agree)

1. Real maintenance win — ~2,500 lines deleted; ruleset now lives in one place
2. Contract tests are well-targeted — 16 tests exercise the JSON boundary without requiring harness-eval installed
3. LLM review extraction is clean and lossless — with 5 preserved tests
4. Skip-condition semantics carried over correctly — no fail-open regression
5. Explicit version pinning — harness-eval-version param with "bump this" reminder
6. Well-documented behavior changes — FP fixes and new rules clearly listed in PR body

---

Behavior Parity Changes

Intentional improvements from harness-eval's tighter patterns:

Change	Impact
Bare sudo no longer flags	FP fix (excludes apt/dnf/pip/npm)
chmod 777 / chown root on non-sensitive paths	FP fix
Base64 mentions no longer flag	FP fix (only pipe-to-shell patterns)
"You are now a code reviewer" no longer flags	FP fix (requires override qualifier)
Submissions without SKILL.md now scanned	Coverage improvement
New: data exfiltration (8 patterns)	Security improvement
New: reverse shell (11 patterns)	Security improvement
New: homoglyph/RTL obfuscation	Security improvement

---

CI Status

Check	Status
test	✅ Pass (52s)
test (observability)	✅ Pass (59s)

---

Reviewer Verdicts

Reviewer	Verdict	Key Concern
Review 1 (detailed)	Request changes	AI co-author, pre-merge checklist
Review 2 (quick)	Approved	—
Review 3 (medium)	Request changes	Fail-open behavior, AI co-author
Review 4 (recent)	Request changes	AI co-author, rebase, checklist
Consolidated: Request Changes

---

Checklist for Author

• Strip AI co-author trailer from commit
• Run golden-corpus diff (or document results)
• Get team sign-off on behavior parity changes
• Add output file existence check after scan (fail if missing)
• Rebase on latest main
• Re-run CI after fixes

---

Final Verdict

Request Changes due to:

1. AI co-author in commit (policy violation)
2. Pre-merge checklist incomplete (golden-corpus diff, team review)
3. Fail-open behavior in scan step
   The implementation itself is verified correct and represents a significant maintenance improvement. Once the blockers are addressed, this is a strong Approve.

[Benkapner]

End-to-end validation on OpenShift @GuyZivRH

Ran the pipeline on my namespace (ben-evalflow) using this PR branch (refactor/harness-eval-skill-scan) against a test submission with intentionally bad content.

Test setup:

• Submission branch: eval/gate-validation-demo in skill-submissions
• Contains a SKILL.md with prompt injection, credential access, coercive overrides, stealth persistence, data exfiltration, reverse shell, obfuscation, stale references, hedging language, unfinished content, broken references, negative-only instructions, scope-grabbing description, and a test file without assertions
• Pipeline run: harness-eval-full-1786600032

Results: test phase succeeded, all rules fired correctly.

Security (9 deterministic + 8 LLM = 17 total):

• no-prompt-injection: caught "ignore previous instructions"
• no-credential-access: caught ~/.ssh/ and $OPENAI_API_KEY
• prompt-exfiltration: caught "output system prompt"
• coercive-override: caught "must always comply... never refuse"
• stealth-persistence: caught "write to .claude/settings.json"
• data-exfiltration: caught curl -d $(cat /etc/passwd)
• reverse-shell: caught bash -i >& /dev/tcp/
• obfuscation: caught eval(atob(...))
• LLM semantic review: 8 additional findings appended into security JSON

Quality (17 total):

• scope-grab-description: caught "use this for any request"
• stale-references: caught "gpt-3.5-turbo"
• negative-only: caught 4 "don't/never/avoid" lines without positive guidance
• example-gap: 8 instruction lines with no code examples
• broken-references: ./setup-guide.md doesn't exist
• imprecise-instruction: "try to", "consider using"
• redundant-guidance: "follow best practices", "ensure code quality"
• unfinished-content: "TODO:", "coming soon"
• description-quality: too vague, no use-case context
• file-completeness: test without assertions

Pipeline results:

• test phase: Succeeded (6/6 steps)
• security-passed: true (warn mode, findings are advisory)
• quality-passed: true (warn mode)
• tests-passed: true
• File existence checks passed (fail-closed fix works)
• Both output files written to correct paths (security to reports/, quality to workspace root)

24 out of 27 rule categories triggered. The 3 that didn't fire are by design (token-budget needs a 16K+ char file, circular-references needs 2+ skills referencing each other, trigger-manipulation targets SKILL.md body not instruction.md).

[GuyZivRH]

lgtm