fix: enforce hierarchical id send-back, async tool timeout, and policy-string guardrails - #3790
fix: enforce hierarchical id send-back, async tool timeout, and policy-string guardrails#3790praisonai-triage-agent[bot] wants to merge 2 commits into
Conversation
|
@coderabbitai review |
|
/review |
MervinPraison
added
pipeline/blocked:ci
✅ Action performedReview finished.
|
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR validates unsupported guardrail policy configurations, adds timeout handling for asynchronous tools, and updates synchronous and asynchronous hierarchical task execution to send assigned task IDs back into process generators. ChangesAgent execution controls
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Agent
participant ExecutionMixin
participant Tool
Agent->>ExecutionMixin: invoke tool
ExecutionMixin->>Tool: execute through shared coroutine
ExecutionMixin-->>Agent: return result or timeout error
sequenceDiagram
participant AgentTeam
participant ProcessGenerator
participant TaskRegistry
participant TaskExecutor
AgentTeam->>ProcessGenerator: request next task
ProcessGenerator-->>AgentTeam: yield task
AgentTeam->>TaskRegistry: register task
AgentTeam->>TaskExecutor: execute task
TaskExecutor-->>AgentTeam: assigned task ID
AgentTeam->>ProcessGenerator: send task ID
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
Greptile SummaryThe PR fixes hierarchical generator ID send-back, makes async tool timeouts non-retryable, and rejects unsupported policy-string guardrail configurations.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/praisonai-agents/praisonaiagents/agent/agent.py | Rejects unsupported policy-string and GuardrailConfig policy configurations instead of silently accepting unenforced policies. |
| src/praisonai-agents/praisonaiagents/agent/execution_mixin.py | Enforces async tool timeouts and marks uncancellable timeout results non-retryable, resolving the previously reported duplicate-execution path. |
| src/praisonai-agents/praisonaiagents/agents/agents.py | Manually drives synchronous and asynchronous hierarchical generators so generated task IDs are returned through the two-way protocol. |
| src/praisonai-agents/praisonaiagents/config/feature_configs.py | Updates guardrail documentation to direct tool-policy enforcement through the supported policy parameter. |
Reviews (3): Last reviewed commit: "fix: make async tool timeout non-retryab..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
MervinPraison
added
pipeline/awaiting-merge-gate
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #3790 ✅
Phase 1 — Architecture Review (per AGENTS.md)All three fixes are protocol-respecting, minimal, backward-compatible, add no new Agent params, deps, or exports, and touch only
Phase 2 — Valid issue found & fixedGreptile P1 (async timeout retries duplicate tool execution) — CONFIRMED & FIXED. Fix (
Verified with a targeted harness:
This is strictly safer than — and independent of — the pre-existing sync path, whose identical Skipped (intentionally)
Files modified this review
Phase 3 — VerdictAPPROVE. All three original fixes are sound and the one valid blocking issue (Greptile P1) is resolved and verified. No remaining blocking issues. Ready to merge once CI is green. |
MervinPraison
added
pipeline/blocked:stale-final
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/praisonai-agents/praisonaiagents/agent/execution_mixin.py (1)
1510-1510: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse structured timeout logging with correlation fields.
Line 1510 uses a formatted
logging.warning()message. Use the module logger with structured fields forfunction_name,tool_timeout, and available agent, run, and session identifiers.As per coding guidelines,
src/praisonai-agents/praisonaiagents/agent/**/*.pymust use structured logging with correlation IDs.Suggested logging change
- logging.warning(f"Tool {function_name} timed out after {tool_timeout}s") + logger.warning( + "Tool execution timed out", + extra={ + "tool_name": function_name, + "timeout_s": tool_timeout, + "agent_name": self.name, + "run_id": getattr(self, "_current_run_id", None), + "session_id": getattr(self, "_session_id", None), + }, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/praisonai-agents/praisonaiagents/agent/execution_mixin.py` at line 1510, Replace the formatted logging.warning call in the timeout-handling path with the module logger’s structured warning API. Record function_name and tool_timeout as structured fields, and include any available agent, run, and session correlation identifiers from the surrounding execution context.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/praisonai-agents/praisonaiagents/agent/agent.py`:
- Around line 1446-1449: Provide a working policy migration path before
publishing these messages: update the Agent policy guidance at
src/praisonai-agents/praisonaiagents/agent/agent.py:1446-1449 and the
GuardrailConfig guidance at
src/praisonai-agents/praisonaiagents/agent/agent.py:1475-1478 to reference an
existing supported API, or add a valid policy entry point to Agent.__init__.
Align the policy-related example and field documentation at
src/praisonai-agents/praisonaiagents/config/feature_configs.py:508-509 with the
same supported API.
In `@src/praisonai-agents/praisonaiagents/agent/execution_mixin.py`:
- Around line 1508-1511: Update _execute_tool_async_with_retry() to distinguish
its own deadline from asyncio.TimeoutError raised by the tool: create and retain
an owned task for _invoke(), enforce the timeout using an explicit deadline
pattern matching _astart_with_outcome(), then call task.result() after
completion so tool-raised exceptions flow through normal error handling. Only
return the timeout response when the locally enforced deadline expires.
---
Nitpick comments:
In `@src/praisonai-agents/praisonaiagents/agent/execution_mixin.py`:
- Line 1510: Replace the formatted logging.warning call in the timeout-handling
path with the module logger’s structured warning API. Record function_name and
tool_timeout as structured fields, and include any available agent, run, and
session correlation identifiers from the surrounding execution context.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1168dbc3-4b92-4fab-9784-83da435afcee
📒 Files selected for processing (4)
src/praisonai-agents/praisonaiagents/agent/agent.pysrc/praisonai-agents/praisonaiagents/agent/execution_mixin.pysrc/praisonai-agents/praisonaiagents/agents/agents.pysrc/praisonai-agents/praisonaiagents/config/feature_configs.py
| raise ValueError( | ||
| f"Policy-string guardrails {_policy_strs} are not enforced. " | ||
| "Use Agent(policy=PolicyEngine(...)) for tool policy enforcement, " | ||
| "or pass a validator via guardrails=... / GuardrailConfig(validator=...)." |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Expose or replace the policy API before publishing this guidance.
The new errors and documentation direct callers to Agent(policy=PolicyEngine(...)), but the reviewed Agent.__init__ does not accept policy. Following this guidance raises TypeError, so users have no working migration path from rejected guardrail policy settings.
src/praisonai-agents/praisonaiagents/agent/agent.py#L1446-L1449: point to an existing supported API or wire a policy entry point.src/praisonai-agents/praisonaiagents/agent/agent.py#L1475-L1478: apply the same correction to the GuardrailConfig error.src/praisonai-agents/praisonaiagents/config/feature_configs.py#L508-L509: align the example and policy-related field documentation with the supported API.
📍 Affects 2 files
src/praisonai-agents/praisonaiagents/agent/agent.py#L1446-L1449(this comment)src/praisonai-agents/praisonaiagents/agent/agent.py#L1475-L1478src/praisonai-agents/praisonaiagents/config/feature_configs.py#L508-L509
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai-agents/praisonaiagents/agent/agent.py` around lines 1446 -
1449, Provide a working policy migration path before publishing these messages:
update the Agent policy guidance at
src/praisonai-agents/praisonaiagents/agent/agent.py:1446-1449 and the
GuardrailConfig guidance at
src/praisonai-agents/praisonaiagents/agent/agent.py:1475-1478 to reference an
existing supported API, or add a valid policy entry point to Agent.__init__.
Align the policy-related example and field documentation at
src/praisonai-agents/praisonaiagents/config/feature_configs.py:508-509 with the
same supported API.
| result = await asyncio.wait_for(_invoke(), timeout=tool_timeout) | ||
| except asyncio.TimeoutError: | ||
| logging.warning(f"Tool {function_name} timed out after {tool_timeout}s") | ||
| return {"error": f"Tool timed out after {tool_timeout}s", "timeout": True} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/praisonai-agents/praisonaiagents/agent/execution_mixin.py"
rg -n -C 10 'asyncio\.wait_for|asyncio\.TimeoutError|asyncio\.wait\(|_astart_with_outcome|timeout=True' "$file"
rg -n -C 8 'timeout=True|Tool timed out|asyncio\.TimeoutError' --glob '*.py'Repository: MervinPraison/PraisonAI
Length of output: 6077
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/praisonai-agents/praisonaiagents/agent/execution_mixin.py"
echo "== function outline =="
ast-grep outline "$file" --match '_execute_tool_async_with_retry' --view expanded || true
echo "== retry/timeout handling section =="
sed -n '1524,1585p' "$file" | cat -n
echo "== _tool_timeout occurrences =="
rg -n "_tool_timeout|tool_timeout|timeout=True|retryable" "$file"
echo "== Python asyncio TimeoutError behavior =="
python3 - <<'PY'
import asyncio
import sys
print("python", sys.version)
async def inner_timeout():
raise asyncio.TimeoutError("inner")
async def budget_timeout():
await asyncio.sleep(2)
print("inner TimeoutError is TimeoutError:", issubclass(asyncio.TimeoutError, TimeoutError))
print("TimeoutError is alias:", asyncio.TimeoutError is TimeoutError)
async def main():
for coro_name, coro in [("inner", inner_timeout()), ("budget", budget_timeout())]:
try:
try:
await asyncio.wait_for(coro, timeout=0.01)
except asyncio.TimeoutError:
print(coro_name, "caught by except asyncio.TimeoutError")
else:
print(coro_name, "no exception")
finally:
pass
asyncio.run(main())
PYRepository: MervinPraison/PraisonAI
Length of output: 4951
🌐 Web query:
Python 3.11 asyncio.TimeoutError is alias of TimeoutError asyncio.wait_for documentation
💡 Result:
In Python 3.11, asyncio.TimeoutError was made a deprecated alias of the built-in TimeoutError [1][2]. Furthermore, starting in Python 3.11, the asyncio.wait_for function was updated to raise the built-in TimeoutError instead of asyncio.TimeoutError [3][4][5]. Because asyncio.TimeoutError is now an alias for the built-in exception, existing code that catches asyncio.TimeoutError will continue to function correctly in Python 3.11 and later, but it is recommended to use the built-in TimeoutError.
Citations:
- 1: https://docs.python.org/3.11/library/asyncio-exceptions.html
- 2: https://docs.python.org/release/3.11.0/library/asyncio-exceptions.html
- 3: https://github.com/python/cpython/blob/3.11/Doc/library/asyncio-task.rst
- 4: https://docs.python.org/3/library/asyncio-task.html
- 5: https://docs.python.org/3/_sources/library/asyncio-task.rst.txt
Distinguish the local tool timeout from tool-raised timeouts.
_execute_tool_async_with_retry() currently catches every inner asyncio.TimeoutError and returns {"timeout": True, "retryable": False} before the tool timeout can expire. A tool can raise that exception as a normal failure, so the caller then treats it as a non-retryable tool timeout. Use an owned task with an explicit deadline as _astart_with_outcome() does, call task.result() after the task completes, and let inner exceptions follow normal error handling.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai-agents/praisonaiagents/agent/execution_mixin.py` around lines
1508 - 1511, Update _execute_tool_async_with_retry() to distinguish its own
deadline from asyncio.TimeoutError raised by the tool: create and retain an
owned task for _invoke(), enforce the timeout using an explicit deadline pattern
matching _astart_with_outcome(), then call task.result() after completion so
tool-raised exceptions flow through normal error handling. Only return the
timeout response when the locally enforced deadline expires.
Source: MCP tools
praisonai-triage-agent
Bot
removed
pipeline/blocked:ci
|
@claude this PR has merge conflicts with |
|
Claude finished @MervinPraison's task —— View job Rebased PR #3790 onto latest
|
| File | Conflict | Resolution |
|---|---|---|
praisonaiagents/agent/execution_mixin.py |
main added the tool_progress_channel streaming sink around tool dispatch; this PR wrapped dispatch in an async asyncio.wait_for timeout |
Merged both — moved the streaming with tool_progress_channel(_progress_sink): blocks inside the new _invoke() coroutine, then wrapped _invoke() in the timeout. Streaming + timeout now coexist. The 2nd commit's retryable: False (non-retryable timeout) marker re-applied cleanly on top. |
The other 3 files (agent.py, agents.py, feature_configs.py) auto-merged with no conflicts.
Tests (targeted on changed paths)
test_workflow_hierarchical.py+test_agent_centric_enhancements.py— 39 passedtest_tool_timeout.py— 7 passed, 1 failed only fromlitellmnot being installed in the CI sandbox (ImportError: LiteLLM is required) — pre-existing env limitation, unrelated to the rebase.
Skipped (intentionally)
- No new Agent params, deps, or exports added — resolution kept minimal, no scope creep.
praisonai-rust/praisonai-tsuntouched (out of scope).
Branch is now clean on top of main and ready to merge once CI is green.
Branch: claude/issue-3789-20260808-0713 •
praisonai-triage-agent
Bot
added
pipeline/blocked:conflict
…y-string guardrails (fixes #3789) - process=hierarchical: drive the ahierarchical()/hierarchical() generators manually so the real manager_task id is sent back via .send()/.asend(), restoring the self-delegation guard and manager-task exclusion (was always None under plain for/async for). - execute_tool_async: apply the per-agent ToolConfig.timeout via asyncio.wait_for so the async path no longer hangs indefinitely on a stuck tool, matching the sync path. - guardrails: fail loud with a clear ValueError when unenforced policy-string guardrails (guardrails=["policy:..."] or GuardrailConfig.policy/.policies) are passed, instead of running with silent zero enforcement; fixed the misleading GuardrailConfig docstring. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
…effects asyncio.wait_for cannot cancel a sync tool already running in the executor, so the async retry loop was re-invoking timed-out tools up to max_attempts times while the original invocation kept running — duplicating DB writes, API calls, and file mutations (Greptile P1). Mark the async timeout result with retryable=False and short-circuit it in _execute_tool_async_with_retry alongside the other non-retryable outcomes. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
1331d36 to
781deb9
Compare
MervinPraison
added
pipeline/blocked:ci
praisonai-triage-agent
Bot
removed
pipeline/blocked:ci
1 participant
Fixes #3789
Summary
Three distinct, independently-verified defects in
praisonaiagentscore, each reachable via documented first-class usage. All fixes are minimal and backward-compatible.1.
process="hierarchical"— manager id never sent back into the generatorahierarchical()/hierarchical()use the two-way generator protocol (manager_task_id = yield manager_task) but both consumers inagents/agents.pydrove them with plainfor/async for, which only calls__next__()/send(None). Somanager_task_idwas permanentlyNone, defeating the manager-task exclusion filter and the anti-self-delegation guard.Fix: drive both generators manually with
next()/gen.send(task_id)(sync) and__anext__()/gen.asend(task_id)(async), sending the real id back on the first yield.2.
execute_tool_asyncignored the tool timeoutThe sync path enforces
ToolConfig.timeout, but_execute_tool_async_implnever read_tool_timeout, so a hung tool underachat()/async workflows hung the coroutine forever.Fix: wrap the invocation in
asyncio.wait_for(..., timeout=self._tool_timeout)and return the same{"error": ..., "timeout": True}shape on timeout.3.
GuardrailConfig.policy/.policieswere silently unenforcedThe documented
guardrails=["policy:strict", "pii:redact"]shorthand andGuardrailConfig.policy/.policiesfields have no enforcement path — construction succeeded with zero enforcement.Fix (minimal, no new subsystem): raise a clear
ValueErrorpointing users to the workingAgent(policy=PolicyEngine(...))param, so silent no-enforcement is never the default. Also corrected the misleadingGuardrailConfigdocstring. Non-policy presets ("strict",["strict", {...}], validator/llm_validator configs) are unaffected.Test plan
tests/unit/config/test_agent_centric_enhancements.py— 27 passedtests/unit/config/test_param_resolver_comprehensive.py+test_precedence_ladder.py— 65 passedtests/unit/workflows/test_workflow_hierarchical.py— 12 passedtests/test_tool_timeout.py— passed (1 unrelated failure requireslitellm, not installed in CI sandbox)ValueErrorwhile valid presets still build🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation