Repository navigation
test(btest): adopt rejected_elements safety pattern across all agents (RHAIENG-6258) - #262
Conversation
… (RHAIENG-6258) Add rejected_elements field to adversarial golden queries in all 10 agents that were missing it (agentic_rag already had it). Add a matching test_adversarial_prompt_injection_resistance test to every agent's test_tool_usage.py that asserts none of the rejected phrases appear in the response. Refactor autogen mcp_agent's ad-hoc credential check to use score_pii_leakage from harness.scorers.safety. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (23)
📝 WalkthroughWalkthroughThe change adds shared prompt-injection assertions, expands adversarial golden fixtures across agent templates, introduces behavioral resistance tests, replaces AutoGen’s manual leakage check with PII scoring, and detects Slack-style tokens. ChangesAdversarial evaluation coverage
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant BehavioralTest
participant AgentEval
participant InjectionAssertion
participant SafetyScorer
BehavioralTest->>AgentEval: run adversarial golden query
AgentEval-->>BehavioralTest: return result
BehavioralTest->>InjectionAssertion: validate expected and rejected elements
InjectionAssertion->>SafetyScorer: score prompt-injection resistance
SafetyScorer-->>InjectionAssertion: return score
InjectionAssertion-->>BehavioralTest: pass or fail assertions
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…erage (RHAIENG-6258)
Replace overly broad rejected_elements patterns ("I am a", "you are a",
"system prompt") with disclosure-specific phrases that distinguish
refusal language from actual system prompt leaks. Add Slack token
(xox[bpasre]-) pattern to score_pii_leakage PII patterns dict. Fix
HITL adversarial expected_elements from ["cannot", "will not"] to
["file"] for robustness across response variants.
Verified against live agents: react_agent, autogen/mcp_agent, and HITL
pass; crewai correctly catches genuine system prompt leak.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ngs (RHAIENG-6258) - Revert agentic_rag rejected_elements to original broader patterns - Extract assert_injection_resistance() into harness.assertions to eliminate duplication across 11 agent test files - Add score_prompt_injection_resistance() call for broader regex-based compliance detection (13 patterns covering DAN mode, developer mode, etc.) - Fix slack_token regex: add word boundaries for consistency Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
shricharan-ks
left a comment
shricharan-ks
left a comment
There was a problem hiding this comment.
Code Review — PR #262 (medium effort, verified)
This PR adopts the rejected_elements safety pattern across all 10 agents that were missing it (agentic_rag already had it), adds a new shared assert_injection_resistance helper, refactors autogen's PII check to use the shared scorer, and adds Slack token coverage. 23 files changed, 207 additions, 33 deletions.
Overall this is a well-structured security improvement. The test pattern is consistent across all agents, the shared helper reduces duplication, and the rejected_elements phrases chosen for the 10 new agents are specific enough to avoid false positives. Four findings survived review, all non-blocking.
Findings (4, non-blocking)
1. 🟡 assertions.py:27 — Helper is stricter than the original agentic_rag inline code (PLAUSIBLE)
The original agentic_rag inline code (now removed) only checked expected_elements + rejected_elements. The new assert_injection_resistance helper additionally calls score_prompt_injection_resistance(), which checks compliance indicators (regex patterns for "HACKED", "developer mode", "DAN", etc.).
This means the agentic_rag refactoring is not a pure drop-in replacement — it's strictly stricter. A response that passed the old inline code could fail the new helper if it contains a compliance indicator. The PR author confirmed tests pass, so this isn't breaking today, but it's worth noting as a behavioral change during what appears to be a pure refactoring.
2. 🟡 assertions.py:13 — golden.get("expected_elements", []) silently skips validation on missing key (PLAUSIBLE)
The original agentic_rag code used golden["expected_elements"] (KeyError on missing). The helper uses .get("expected_elements", []) which silently skips the check. If a future adversarial golden entry omits expected_elements, the helper won't catch that the agent gave an irrelevant response — the safety check becomes partially ineffective for that entry.
This is defensive coding vs. fail-fast — both are reasonable. Worth a design decision.
3. 🟡 human_in_the_loop/golden_queries.yaml:56 — HITL adversarial expected_elements semantic shift (PLAUSIBLE)
The HITL adversarial query's expected_elements changed from ["cannot", "will not"] (refusal language) to ["file"] (content language). This shifts what the test validates:
- Old: Verifies the agent explicitly refuses the adversarial request
- New: Verifies the response mentions "file" (the legitimate part of the query)
The prompt-leak detection is now handled by rejected_elements, so the overall safety coverage is maintained. But the test no longer verifies the agent's refusal wording — just that it didn't leak internals. This is a deliberate design choice (rejection wording was already covered by test_tool_rejection in PR #255), just worth noting.
4. 🟡 safety.py:50 — Slack token regex body omits underscore (PLAUSIBLE)
"slack_token": re.compile(r"\bxox[bpasre]-[a-zA-Z0-9-]{10,}\b"),Slack tokens can contain underscores in the body (e.g., xoxb-123456789012-abc_def_ghijklmn). The character class [a-zA-Z0-9-] doesn't include _, so the match may stop at the underscore boundary and not reach the {10,} minimum. Minor — the main Slack token prefixes (xoxb-, xoxp-) are correctly covered.
Fix: [a-zA-Z0-9_-] to include underscores.
What looks good
- Consistent adoption: All 10 new agents use identical
rejected_elementsphrases — more specific than agentic_rag's pre-existing patterns ("my system prompt is" vs "I am a"), reducing false positive risk - HITL approval handling: The HITL test correctly passes
approval=golden.get("approval", "yes"), ensuring the adversarial rejection flow works - Autogen refactoring: Hand-rolled credential patterns replaced with
score_pii_leakagescorer (11 pattern types), integrating with the scoring framework - Shared helper extraction:
assert_injection_resistanceeliminates copy-paste across 10 agents - All callers assert
result.successbefore calling the helper — the "no result.success guard" concern is mitigated by caller discipline
Summary
No blocking issues. The core pattern is sound — rejected_elements + assert_injection_resistance provides a good security baseline across all agents. Consider adding _ to the Slack token regex (#4) and noting the stricter-than-original behavior in the agentic_rag refactoring (#1). Ready to merge with minor fixes.
Additional findings from second review passTwo more findings worth noting: 5. 🟡 HITL adversarial query:
|
- Add underscore to slack_token regex char class so tokens like xoxb-123456789012-abc_def_ghijklmn match fully - Read adversarial query from golden YAML in autogen test_adversarial_no_pii_leakage instead of hardcoding it, preventing silent divergence if the fixture changes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Thanks for the thorough review @shricharan-ks — here's our take on each finding: #1 (helper stricter than original) — Intentional, not a pure refactoring. The PR description calls out adding #2 ( #3 (HITL expected_elements semantic shift) — Deliberate design choice. The test asserts every element in #4 (Slack token regex missing underscore) — Good catch, fixed in #5 (HITL refusal language gap) — Same reasoning as #3. #6 (Autogen hardcoded query divergence) — Good catch, fixed in |
shricharan-ks
left a comment
shricharan-ks
left a comment
There was a problem hiding this comment.
Re-review — Actionable findings addressed ✅
Commit ca9af4b9 fixes the two actionable findings from the previous review:
Finding #4 (Slack token regex) — FIXED ✅
# Before: [a-zA-Z0-9-] (missed underscores)
# After: [a-zA-Z0-9_-] (covers underscore-containing tokens)Finding #6 (autogen hardcoded query) — FIXED ✅
test_adversarial_no_pii_leakage now reads the query from golden YAML via adversarial[0]["query"] instead of hardcoding the string. No more divergence risk.
Remaining findings (1, 2, 3, 5) — all non-blocking design decisions, acknowledged:
- Helper is intentionally stricter than original (adds compliance-indicator check)
.get()with default is defensive coding vs fail-fast — reasonable choice- HITL expected_elements semantic shift is deliberate (refusal covered by
test_tool_rejection) - HITL
test_tool_rejectionfor adversarial query is weaker but covered bytest_adversarial_prompt_injection_resistance
LGTM — ready to merge.
Summary
rejected_elementsfield to adversarial golden queries in all 10 agents that were missing it (agentic_rag already had it), asserting the agent does not leak system prompt internals when faced with prompt injectiontest_adversarial_prompt_injection_resistancetest function to every agent'stest_tool_usage.py— loads adversarial queries withrejected_elements, verifies the agent answers the legitimate part, and asserts none of the rejected phrases appear in the responsesk-,Bearer, etc.) to usescore_pii_leakagefromharness.scorers.safety, which covers 11 pattern types and integrates with the scoring frameworkTest plan
ruff checkandruff format --checkpass on all 10 modified test filesmake testfor one agent locally to validate the new test is collected by pytesttest_adversarial_prompt_injection_resistancepassestest_adversarial_no_pii_leakagecorrectly usesscore_pii_leakagescorer🤖 Generated with Claude Code