mirror of
https://github.com/MervinPraison/PraisonAI.git
synced 2026-11-07 04:35:58 -10:00
fix: input-guardrail rejection returns fixed string; lock learned-rule updates (#6104)
* fix: return fixed blocked string on input-guardrail rejection; lock learned-rule read-modify-write (fixes #5920) Co-Authored-By: Claude Opus 5.5 <[email protected]> * fix: expose input-guardrail rejection reason via last_guardrail_error Co-Authored-By: Claude Opus 5.5 <[email protected]> --------- Co-authored-by: MervinPraison <[email protected]> Co-authored-by: Claude Opus 5.5 <[email protected]>
This commit is contained in:
5 files changed
+107
-22
No files matched your search
@@ -7223,7 +7223,7 @@ Answer:"""
|
||||
return getattr(self, "_last_guardrail_error", None)
|
||||
|
||||
def _guardrail_blocked_message(self, error=None):
|
||||
"""Fixed, user-facing string returned when an output guardrail blocks a run.
|
||||
"""Fixed, user-facing string returned when an input or output guardrail blocks a run.
|
||||
|
||||
A blocked response must never surface the rejected (potentially unsafe)
|
||||
model text, but it must also never be ``None`` - callers expect a
|
||||
|
||||
@@ -3696,7 +3696,8 @@ Your Goal: {self.goal}"""
|
||||
_in_ok, _in_prompt, _in_err = self._validate_input_with_guardrail(prompt)
|
||||
if not _in_ok:
|
||||
logging.warning(f"Agent {self.name}: input blocked by guardrail: {_in_err}")
|
||||
return None
|
||||
self._last_guardrail_error = _in_err
|
||||
return self._guardrail_blocked_message(_in_err)
|
||||
prompt = _in_prompt
|
||||
|
||||
# Check if external managed backend is configured
|
||||
@@ -4546,7 +4547,8 @@ Output MUST be JSON with 'reflection' and 'satisfactory'.
|
||||
)
|
||||
if not _in_ok:
|
||||
logging.warning(f"Agent {self.name}: input blocked by guardrail: {_in_err}")
|
||||
return None
|
||||
self._last_guardrail_error = _in_err
|
||||
return self._guardrail_blocked_message(_in_err)
|
||||
prompt = _in_prompt
|
||||
|
||||
# Check if external managed backend is configured (async parity with the
|
||||
@@ -5666,7 +5668,8 @@ Output MUST be JSON with 'reflection' and 'satisfactory'.
|
||||
_in_ok, _in_prompt, _in_err = self._validate_input_with_guardrail(prompt)
|
||||
if not _in_ok:
|
||||
logging.warning(f"Agent {getattr(self, 'name', '')}: input blocked by guardrail: {_in_err}")
|
||||
yield f"[Input blocked by guardrail: {_in_err}]"
|
||||
self._last_guardrail_error = _in_err
|
||||
yield self._guardrail_blocked_message(_in_err)
|
||||
return "failed"
|
||||
prompt = _in_prompt
|
||||
|
||||
|
||||
@@ -87,7 +87,8 @@ class PolicyConfig:
|
||||
def save_learned_rule(self, resource: str, action: str = "allow") -> bool:
|
||||
"""Persist a learned rule to the rules file.
|
||||
|
||||
Uses atomic write (tmp + rename) for safety.
|
||||
Uses a locked, atomic read-modify-write so concurrent savers keep
|
||||
each other's rules.
|
||||
|
||||
Args:
|
||||
resource: Resource pattern (e.g., "tool:pip install *")
|
||||
@@ -100,24 +101,21 @@ class PolicyConfig:
|
||||
return False
|
||||
|
||||
try:
|
||||
# Load existing rules
|
||||
existing = self.load_learned_rules()
|
||||
|
||||
# Check for duplicates
|
||||
for rule in existing:
|
||||
if rule.get("resource") == resource:
|
||||
return True # Already exists
|
||||
|
||||
# Add new rule
|
||||
existing.append({"resource": resource, "action": action})
|
||||
|
||||
# Ensure directory exists
|
||||
rules_dir = os.path.dirname(self.rules_file)
|
||||
os.makedirs(rules_dir, exist_ok=True)
|
||||
|
||||
from ..utils.atomic_io import atomic_write_json
|
||||
atomic_write_json(self.rules_file, {"rules": existing}, indent=2)
|
||||
|
||||
if rules_dir:
|
||||
os.makedirs(rules_dir, exist_ok=True)
|
||||
|
||||
# Read, dedupe and append under the per-path lock so concurrent
|
||||
# writers never drop each other's rules (last-writer-wins).
|
||||
def _add(data):
|
||||
rules = data.setdefault("rules", [])
|
||||
if not any(r.get("resource") == resource for r in rules):
|
||||
rules.append({"resource": resource, "action": action})
|
||||
|
||||
from ..utils.atomic_io import update_json
|
||||
update_json(self.rules_file, _add, default=lambda: {"rules": []}, indent=2)
|
||||
|
||||
logger.debug(f"Saved learned rule: {resource} -> {action}")
|
||||
return True
|
||||
except OSError as e:
|
||||
|
||||
@@ -0,0 +1,84 @@
|
||||
"""Regression tests for issue #5920.
|
||||
|
||||
* A rejected *input* returns the same fixed blocked string as a rejected
|
||||
output (never ``None``, never the validator's free-text reason).
|
||||
* Concurrent ``PolicyConfig.save_learned_rule`` calls keep every rule.
|
||||
"""
|
||||
import asyncio
|
||||
import threading
|
||||
|
||||
from praisonaiagents import Agent
|
||||
from praisonaiagents.policy.config import PolicyConfig
|
||||
|
||||
SECRET_REASON = "contains secret: hunter2"
|
||||
|
||||
|
||||
class RejectInput:
|
||||
def validate_input(self, content, **kwargs):
|
||||
return False, SECRET_REASON
|
||||
|
||||
def validate_output(self, content, **kwargs):
|
||||
return True, content
|
||||
|
||||
|
||||
def _agent():
|
||||
return Agent(name="T", instructions="test", llm="gpt-4o-mini", guardrails=RejectInput())
|
||||
|
||||
|
||||
def _assert_blocked(agent, result):
|
||||
assert isinstance(result, str)
|
||||
assert result == agent._guardrail_blocked_message()
|
||||
assert "hunter2" not in result
|
||||
|
||||
|
||||
def test_chat_input_block_returns_fixed_string():
|
||||
a = _agent()
|
||||
_assert_blocked(a, a.chat("hi"))
|
||||
|
||||
|
||||
def test_start_input_block_returns_fixed_string():
|
||||
a = _agent()
|
||||
_assert_blocked(a, a.start("hi"))
|
||||
|
||||
|
||||
def test_achat_input_block_returns_fixed_string():
|
||||
a = _agent()
|
||||
_assert_blocked(a, asyncio.run(a.achat("hi")))
|
||||
|
||||
|
||||
def test_stream_input_block_does_not_leak_reason():
|
||||
a = _agent()
|
||||
chunks = list(a.start("hi", stream=True))
|
||||
_assert_blocked(a, "".join(chunks))
|
||||
|
||||
|
||||
def test_input_block_reason_exposed_via_last_guardrail_error():
|
||||
a = _agent()
|
||||
_assert_blocked(a, a.chat("hi"))
|
||||
assert a.last_guardrail_error == SECRET_REASON
|
||||
|
||||
|
||||
def test_concurrent_learned_rules_are_not_lost(tmp_path):
|
||||
rules_file = str(tmp_path / "rules" / "default.rules")
|
||||
threads, per_thread = 8, 25
|
||||
|
||||
def worker(i):
|
||||
cfg = PolicyConfig(rules_file=rules_file)
|
||||
for j in range(per_thread):
|
||||
assert cfg.save_learned_rule(f"tool_{i}_{j}")
|
||||
|
||||
ts = [threading.Thread(target=worker, args=(i,)) for i in range(threads)]
|
||||
for t in ts:
|
||||
t.start()
|
||||
for t in ts:
|
||||
t.join()
|
||||
|
||||
rules = PolicyConfig(rules_file=rules_file).load_learned_rules()
|
||||
assert len(rules) == threads * per_thread
|
||||
|
||||
|
||||
def test_learned_rule_is_deduplicated(tmp_path):
|
||||
cfg = PolicyConfig(rules_file=str(tmp_path / "r.rules"))
|
||||
assert cfg.save_learned_rule("tool:ls", "allow")
|
||||
assert cfg.save_learned_rule("tool:ls", "deny")
|
||||
assert cfg.load_learned_rules() == [{"resource": "tool:ls", "action": "allow"}]
|
||||
@@ -237,7 +237,7 @@ def test_streamed_turn_blocked_by_guardrail_is_failed(sink):
|
||||
|
||||
agent = Agent(name="Writer", instructions="x")
|
||||
with patch.object(agent, "_validate_input_with_guardrail", return_value=(False, None, "no")):
|
||||
assert "blocked" in "".join(agent._start_stream("hi"))
|
||||
assert "".join(agent._start_stream("hi")) == agent._guardrail_blocked_message()
|
||||
assert _of(sink, "agent_end")[0].data == {"status": "failed"}
|
||||
|
||||
|
||||
|
||||
Reference in new issue
Block a user