fix: close wrapper gaps in tools add, serve async safety, adapter parity - #3771
fix: close wrapper gaps in tools add, serve async safety, adapter parity#3771praisonai-triage-agent[bot] wants to merge 2 commits into
Conversation
…ity (fixes #3770) Gap 2 (security): praisonai tools add now honours PRAISONAI_ALLOW_LOCAL_TOOLS like every other loader, inspects added files via ast.parse instead of exec_module, and hardens GitHub downloads (HTTPS-only, 1 MiB cap, safe basename). Gap 3 (async safety): OpenAI-compat handlers offload sync provider.invoke via asyncio.to_thread; simulated streaming emits word-by-word instead of per-char; A2UEventBus mutations are guarded by a threading.RLock, the singleton is double-checked-locked, and /a2u/health now requires authentication. Gap 1 (adapter parity): add warn_unsupported_fields to surface YAML fields a backend silently drops, wired into CrewAI/AutoGen adapters; AutoGen agent_callback is now fired per agent. Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
✅ Action performedReview finished.
|
MervinPraison
added
pipeline/blocked:ci
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 changes secure local and GitHub tool additions, synchronize A2U event-bus state, offload synchronous provider calls, group streaming content, and improve CrewAI and AutoGen adapter field and callback handling. ChangesTool addition security
Async serving and A2U concurrency
Framework adapter parity
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 closes wrapper gaps in tool installation, asynchronous serving, A2U concurrency and authentication, streaming granularity, and framework-adapter diagnostics.
Confidence Score: 5/5The PR appears safe to merge. The previously reported oversized-download failure is fixed because the downloader reads one byte beyond the limit and returns before writing when the response exceeds the cap; no blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/praisonai/praisonai/cli/features/tools.py | Adds local-tool authorization, static inspection, constrained GitHub downloads, and a one-byte-over-cap read that correctly rejects rather than installs oversized source. |
| src/praisonai/praisonai/endpoints/a2u_server.py | Adds locking around event-bus state and singleton creation and applies authentication to the health endpoint. |
| src/praisonai/praisonai/endpoints/providers/openai_compat.py | Changes simulated streaming from per-character frames to whitespace-preserving word chunks. |
| src/praisonai/praisonai/endpoints/server.py | Offloads synchronous provider invocations from asynchronous request handlers with asyncio.to_thread. |
| src/praisonai/praisonai/framework_adapters/base.py | Introduces shared warnings for YAML fields unsupported by selected framework adapters. |
| src/praisonai/praisonai/framework_adapters/autogen_adapter.py | Emits unsupported-field warnings and invokes the declared callback after each AutoGen agent is constructed. |
| src/praisonai/praisonai/framework_adapters/crewai_adapter.py | Applies shared unsupported-field diagnostics while constructing CrewAI agents. |
| src/praisonai/tests/unit/cli/test_interactive_surfaces_parity.py | Updates the test double to accept the execute method’s read_only argument. |
Reviews (2): Last reviewed commit: "fix: reject oversized GitHub tool downlo..." | 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 CI failed on HEAD Failed checks
Failures (extracted)
Critical review firstBefore changing code or tests, decide which side is wrong:
SDK guardrails (AGENTS.md):
What to do
|
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #3771
Verdict: Approve after fixes (both blockers resolved, pushed to this branch)CI failure —
|
| File | Assessment |
|---|---|
a2u_server.py |
RLock guarding, snapshot-under-lock/deliver-outside publish, double-checked singleton — correct. /a2u/health reads len(bus._subscriptions) outside the lock, but that's a benign stat read, not worth a lock. ✅ |
server.py |
Four asyncio.to_thread(provider.invoke, …) offloads — correct, no signature drift. ✅ |
openai_compat.py |
re.findall(r"\S+\s*|\s+") word-chunking preserves all whitespace exactly; frame count now ∝ tokens. Reshapes an already-completed reply, so no latency regression. ✅ |
base.py |
warn_unsupported_fields is pure visibility, silent for framework: praisonai and supported/structural fields; verified live. ✅ |
autogen_adapter.py |
spec.extras/.key/.role are AgentSpec dataclass fields (safe, no AttributeError). |
crewai_adapter.py |
warn_unsupported_fields wiring — correct. ✅ |
Non-blocking note (AutoGen agent_callback): the PR fires agent_callback({"agent":…, "spec":…}) at construction time, whereas CrewAI wires it as agent.step_callback (a per-step runtime hook). So it isn't strictly "matching CrewAIAdapter" — it's a one-shot construction notification. AutoGen v0.2 AssistantAgent has no step-callback equivalent, so this is a reasonable best-effort and is exception-guarded. Left as-is to avoid scope creep; flagging only for accuracy.
AGENTS.md compliance
Lightweight, backward-compatible, no new Agent params, no core-SDK bloat — all changes live in the praisonai/ wrapper. No new CLI knobs added (correctly dropped the --sha256 suggestion). ✅
Tests
test_interactive_surfaces_parity.py: 4 passedtest_endpoints_unified.py+test_async_daemon_deployment.py: 42 passed- Adapter suites: 17 passed, 1 skipped (1 unrelated failure:
test_hybrid_retrieverneedspytest-asyncioplugin — pre-existing env gap, not this PR)
Files modified in this review
src/praisonai/praisonai/cli/features/tools.py— reject oversized (> 1 MiB) GitHub downloadssrc/praisonai/tests/unit/cli/test_interactive_surfaces_parity.py— stub signature fix (read_only=)
Pushed to claude/issue-3770-20260807-0831 (df9960c89). Both CI and reviewer blockers resolved; PR is good to merge.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
src/praisonai/praisonai/endpoints/providers/openai_compat.py (1)
425-431: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse lazy iteration for grouped content.
re.findallbuilds a list of every match before the first content frame is yielded. A large response therefore creates an additional O(n) allocation and delays content-frame emission. Usere.finditerand extract each match inside the loop.Proposed fix
- for piece in re.findall(r"\S+\s*|\s+", content): + for match in re.finditer(r"\S+\s*|\s+", content): + piece = match.group(0)🤖 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/praisonai/endpoints/providers/openai_compat.py` around lines 425 - 431, Update the grouped-content loop in the streaming response path to use re.finditer instead of re.findall, extracting each match from the iterator as it is processed. Preserve the existing whitespace-preserving grouping pattern and SSE frame behavior while enabling lazy emission without materializing all pieces.
🤖 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/praisonai/cli/features/tools.py`:
- Around line 673-677: Update the filename construction in the tool download
flow around stem and dest to include a stable hash derived from the full GitHub
source path, not just Path(path).name. Preserve the existing user/repo and .py
naming behavior while ensuring distinct source paths produce distinct filenames
and cannot overwrite one another.
- Around line 580-586: Update the local-tool opt-in check in
src/praisonai/praisonai/cli/features/tools.py lines 580-586 to accept both
“true” and the documented value “1” after lowercasing. Apply the identical
accepted-value logic to the GitHub-tool check at lines 639-645, preserving the
existing refusal behavior for all other values.
- Around line 685-686: Update the response-reading logic around resp.read and
dest.write_bytes to read one byte beyond the 1 MiB limit, reject responses
containing that extra byte, and return an error without writing dest; only write
the file when the response fits within the limit.
- Around line 681-685: Update the download flow around the
urllib.request.urlopen call to prevent redirects from reaching resp.read(1024 *
1024). Use an opener with redirect handling disabled, or validate any final
response URL against the approved GitHub raw origin before reading; preserve the
existing raw_url scheme and host validation.
In `@src/praisonai/praisonai/endpoints/a2u_server.py`:
- Around line 143-146: The queue lookup and publication flow around _get_queue
and publish must validate that the subscription is still active while holding
the lifecycle lock, preventing queue recreation after unsubscribe. Make queue
lookup, active-subscription validation, and the publication snapshot one atomic
operation, and update unsubscribe/get_events so removing an active subscription
also terminates its event iterator.
- Around line 181-188: Update publish_sync and the subscription setup around
get_events to associate each asyncio.Queue with its owning event loop, then
dispatch each published item onto that loop rather than writing to the queue
directly from the caller thread. Preserve the existing lock-protected
subscription snapshot and ensure queued delivery remains safe when publish_sync
is invoked through run_sync from another thread or event loop.
In `@src/praisonai/praisonai/framework_adapters/base.py`:
- Around line 46-62: Extend warn_unsupported_fields in
src/praisonai/praisonai/framework_adapters/base.py#L46-L62 with task-level
supported-field mappings and identifiers, preserving the existing agent-field
behavior. In
src/praisonai/praisonai/framework_adapters/autogen_adapter.py#L163-L164, call
the helper for every task_spec.extras using AutoGen’s supported task fields; in
src/praisonai/praisonai/framework_adapters/crewai_adapter.py#L84-L85, do the
same using CrewAI’s explicit task_details.get(...) fields, so unsupported task
fields are reported.
---
Nitpick comments:
In `@src/praisonai/praisonai/endpoints/providers/openai_compat.py`:
- Around line 425-431: Update the grouped-content loop in the streaming response
path to use re.finditer instead of re.findall, extracting each match from the
iterator as it is processed. Preserve the existing whitespace-preserving
grouping pattern and SSE frame behavior while enabling lazy emission without
materializing all pieces.
🪄 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: eb5d8a2d-e507-47f7-9b96-658b1f66f49b
📒 Files selected for processing (7)
src/praisonai/praisonai/cli/features/tools.pysrc/praisonai/praisonai/endpoints/a2u_server.pysrc/praisonai/praisonai/endpoints/providers/openai_compat.pysrc/praisonai/praisonai/endpoints/server.pysrc/praisonai/praisonai/framework_adapters/autogen_adapter.pysrc/praisonai/praisonai/framework_adapters/base.pysrc/praisonai/praisonai/framework_adapters/crewai_adapter.py
| if os.environ.get("PRAISONAI_ALLOW_LOCAL_TOOLS", "").lower() != "true": | ||
| self.print_status( | ||
| "Refusing to add local tools: set PRAISONAI_ALLOW_LOCAL_TOOLS=true " | ||
| "to enable (same opt-in the runtime loader requires).", | ||
| "error", | ||
| ) | ||
| return {"success": False, "error": "PRAISONAI_ALLOW_LOCAL_TOOLS not set"} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Accept the documented opt-in value.
.env.example documents PRAISONAI_ALLOW_LOCAL_TOOLS=1, but both checks accept only true. Users who follow the documented configuration cannot add local or GitHub tools.
src/praisonai/praisonai/cli/features/tools.py#L580-L586: accept both1andtrueto preserve the documented opt-in contract.src/praisonai/praisonai/cli/features/tools.py#L639-L645: use the same accepted-value check for GitHub tools.
📍 Affects 1 file
src/praisonai/praisonai/cli/features/tools.py#L580-L586(this comment)src/praisonai/praisonai/cli/features/tools.py#L639-L645
🤖 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/praisonai/cli/features/tools.py` around lines 580 - 586, Update
the local-tool opt-in check in src/praisonai/praisonai/cli/features/tools.py
lines 580-586 to accept both “true” and the documented value “1” after
lowercasing. Apply the identical accepted-value logic to the GitHub-tool check
at lines 639-645, preserving the existing refusal behavior for all other values.
| stem = f"{user}_{repo}_{Path(path).name}" if path else f"{user}_{repo}_tools" | ||
| filename = Path(stem).name | ||
| if not filename.endswith(".py"): | ||
| filename += ".py" | ||
| dest = tools_dir / filename |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Prevent downloaded tool filename collisions.
Path(path).name discards the directory path. For example, github:user/repo/a/tools.py and github:user/repo/b/tools.py both write user_repo_tools.py. The second command silently replaces the first tool. Include a stable hash of the full GitHub source path in filename.
🤖 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/praisonai/cli/features/tools.py` around lines 673 - 677, Update
the filename construction in the tool download flow around stem and dest to
include a stable hash derived from the full GitHub source path, not just
Path(path).name. Preserve the existing user/repo and .py naming behavior while
ensuring distinct source paths produce distinct filenames and cannot overwrite
one another.
| req = urllib.request.Request( | ||
| raw_url, headers={"User-Agent": "praisonai-tools-add"} | ||
| ) | ||
| with urllib.request.urlopen(req, timeout=30) as resp: # nosec B310 - scheme checked above | ||
| data = resp.read(1024 * 1024) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 5 'urlopen\(|build_opener|HTTPRedirectHandler' \
src/praisonai/praisonai/cli/features/tools.pyRepository: MervinPraison/PraisonAI
Length of output: 881
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
fd -a features/tools.py src/praisonai/praisonai/cli/features/tools.py | sed 's#^\./##'
printf '\n--- source around raw_url construction ---\n'
sed -n '620,690p' src/praisonai/praisonai/cli/features/tools.py | nl -ba -v620
printf '\n--- related raw_url usage ---\n'
rg -n -C 3 'raw_url|github\.com|raw' src/praisonai/praisonai/cli/features/tools.py
printf '\n--- urllib redirect behavior probes ---\n'
python3 - <<'PY'
import urllib.request
print("default opener handlers:")
for h in urllib.request.build_opener().handlers:
print(" -", type(h).__module__, type(h).__qualname__)
print("\nHTTPRedirectHandler defaults:")
h = urllib.request.HTTPRedirectHandler()
for name in ("http_error_301", "http_error_302", "http_error_303", "http_error_307", "http_error_308"):
print(f" - {name}: {getattr(h, name).__code__.co_name if hasattr(getattr(h, name), '__code__') else type(getattr(h, name)).__name__}")
PYRepository: MervinPraison/PraisonAI
Length of output: 294
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- file existence ---'
test -f src/praisonai/praisonai/cli/features/tools.py && echo file exists || echo file missing
printf '%s\n' '--- source around raw_url construction ---'
sed -n '620,690p' src/praisonai/praisonai/cli/features/tools.py | nl -ba -v620
printf '%s\n' '--- related raw_url usage ---'
rg -n -C 3 'raw_url|https://raw|github\.com|raw' src/praisonai/praisonai/cli/features/tools.py || true
printf '%s\n' '--- urllib redirect handling probe ---'
python3 - <<'PY'
import urllib.request, http.client
class TrackingHTTPRedirectHandler(urllib.request.HTTPRedirectHandler):
seen = []
def http_error_308(self, *args, **kwargs):
TrackingHTTPRedirectHandler.seen.append(("http_error_308", args, kwargs))
return super().http_error_308(*args, **kwargs)
print("default opener handlers:")
for h in urllib.request.build_opener().handlers:
print(" -", type(h).__module__, ":", type(h).__qualname__)
print("\nHTTPRedirectHandler redirects handle:")
h = urllib.request.HTTPRedirectHandler()
for name in ["http_error_301", "http_error_302", "http_error_303", "http_error_307", "http_error_308"]:
attr = getattr(h, name)
print(f" - {name}: {getattr(attr, '__name__', str(type(attr)))}")
PYRepository: MervinPraison/PraisonAI
Length of output: 279
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- source around raw_url construction ---'
cat -n src/praisonai/praisonai/cli/features/tools.py | sed -n '620,690p'
printf '%s\n' '--- related raw_url usage ---'
rg -n -C 3 'raw_url|https://raw|github\.com|raw' src/praisonai/praisonai/cli/features/tools.py || true
printf '%s\n' '--- urllib redirect handling probe ---'
python3 - <<'PY'
import urllib.request
print("default opener handlers:")
for h in urllib.request.build_opener().handlers:
print(" -", type(h).__module__, ":", type(h).__qualname__)
print("\nHTTPRedirectHandler redirects handle:")
h = urllib.request.HTTPRedirectHandler()
for name in ["http_error_301", "http_error_302", "http_error_303", "http_error_307", "http_error_308"]:
attr = getattr(h, name)
print(f" - {name}: {getattr(attr, '__name__', str(type(attr)))}")
PYRepository: MervinPraison/PraisonAI
Length of output: 6437
🌐 Web query:
Python urllib.request HTTPRedirectHandler redirect allowed same location docs
💡 Result:
In Python's urllib.request module, the HTTPRedirectHandler class manages HTTP redirections and does not inherently block redirects to the same location [1][2]. Instead, it protects against infinite redirect loops using built-in limiters rather than a strict ban on same-location targets [3][4]. The HTTPRedirectHandler employs the following mechanisms to prevent infinite redirection loops: 1. Max Repeats: The handler tracks how many times a specific URL has been visited for a given request chain [3]. If the number of times a single URL is visited exceeds self.max_repeats, it raises an HTTPError [3]. 2. Max Redirections: The handler also enforces a total limit on the number of redirections allowed for a single request chain using self.max_redirections [3][4]. The redirect_request method is responsible for creating the new Request object when a redirect is received [1][2]. While the default implementation permits standard browser-like behavior—such as converting POST requests to GET for 301 and 302 status codes—it does not explicitly forbid redirecting to the same URL, relying instead on the aforementioned loop detection mechanisms to terminate excessive cycles [3][2]. If you need to implement custom logic to disallow or handle same-location redirects differently, you can override the redirect_request method in a subclass of HTTPRedirectHandler [5]. Within your overridden method, you can compare the current request URL (available via req.full_url) with the newurl argument to determine if they match and take appropriate action [1][2].
Citations:
- 1: https://docs.python.org/3.14/library/urllib.request.html
- 2: https://docs.python.org/3/library/urllib.request.html
- 3: https://github.com/python/cpython/blob/3.14/Lib/urllib/request.py
- 4: https://github.com/python/cpython/blob/master/Lib/test/test_urllib2.py
- 5: https://www.pythonlore.com/managing-http-redirects-with-http-client-httpredirecthandler/
Reject redirects before downloading the response.
urlopen() uses the default HTTPRedirectHandler, so a response from https://raw.githubusercontent.com/ can redirect to another host despite the input check. Use an opener that rejects redirects, or validate the final redirect URL against the GitHub raw origin before read().
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 683-683: Request-controlled URL passed to urlopen; validate against an allowlist to prevent SSRF.
Context: urllib.request.urlopen(req, timeout=30)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(urlopen-unsanitized-data)
🪛 Ruff (0.16.1)
[error] 681-683: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
[error] 684-684: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
🤖 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/praisonai/cli/features/tools.py` around lines 681 - 685, Update
the download flow around the urllib.request.urlopen call to prevent redirects
from reaching resp.read(1024 * 1024). Use an opener with redirect handling
disabled, or validate any final response URL against the approved GitHub raw
origin before reading; preserve the existing raw_url scheme and host validation.
Source: Linters/SAST tools
| data = resp.read(1024 * 1024) | ||
| dest.write_bytes(data) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject responses that exceed 1 MiB.
resp.read(1024 * 1024) silently truncates a larger response and then writes the incomplete file as a successful tool addition. Read one additional byte and return an error without writing dest when the limit is exceeded.
🤖 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/praisonai/cli/features/tools.py` around lines 685 - 686, Update
the response-reading logic around resp.read and dest.write_bytes to read one
byte beyond the 1 MiB limit, reject responses containing that extra byte, and
return an error without writing dest; only write the file when the response fits
within the limit.
| with self._lock: | ||
| if subscription_id not in self._queues: | ||
| self._queues[subscription_id] = asyncio.Queue(maxsize=_QUEUE_MAX) | ||
| return self._queues[subscription_id] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not recreate queues for unsubscribed IDs.
publish snapshots a subscription and then releases self._lock. If unsubscribe runs before publish calls _get_queue, this method recreates the removed queue for an inactive ID. The stale queue remains in self._queues because its removal already occurred. Repeated subscribe, unsubscribe, and publish races can grow this dictionary without the subscription limit.
Make queue lookup, active-subscription validation, and publication snapshot one lifecycle-safe operation. Do not create a queue when the subscription no longer exists. Also terminate an active get_events iterator when unsubscribe removes its subscription.
🤖 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/praisonai/endpoints/a2u_server.py` around lines 143 - 146, The
queue lookup and publication flow around _get_queue and publish must validate
that the subscription is still active while holding the lifecycle lock,
preventing queue recreation after unsubscribe. Make queue lookup,
active-subscription validation, and the publication snapshot one atomic
operation, and update unsubscribe/get_events so removing an active subscription
also terminates its event iterator.
| # Snapshot the target subscriptions under the lock, then deliver outside | ||
| # it so put_nowait / _get_queue cannot race against subscribe/unsubscribe. | ||
| with self._lock: | ||
| sub_ids = list(self._streams.get(stream_name, ())) | ||
| snapshot = {sid: self._subscriptions.get(sid) for sid in sub_ids} | ||
|
|
||
| count = 0 | ||
| for sub_id in list(self._streams[stream_name]): | ||
| subscription = self._subscriptions.get(sub_id) | ||
| for sub_id, subscription in snapshot.items(): |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect how run_sync selects and owns its event loop.
ast-grep outline src/praisonai/praisonai/_async_bridge.py --items all
rg -n -C 12 'def run_sync|new_event_loop|run_coroutine_threadsafe|call_soon_threadsafe' \
src/praisonai/praisonai/_async_bridge.py
# Trace synchronous publishers and queue consumers.
rg -n -C 6 --glob '*.py' '\bpublish_sync\s*\(|\bget_events\s*\(|asyncio\.Queue' src/praisonaiRepository: MervinPraison/PraisonAI
Length of output: 24898
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== A2U server A2UBus methods =="
ast-grep outline src/praisonai/praisonai/endpoints/a2u_server.py \
--match A2UBus --view expanded || true
echo
echo "== A2UBus implementation slice =="
sed -n '88,252p' src/praisonai/praisonai/endpoints/a2u_server.py
echo
echo "== emit wrappers and event bus references =="
rg -n -C 8 'def emit|get_event_bus|publish_sync\s*\(|A2UBus' src/praisonai/praisonai/endpoints/a2u_server.py
echo
echo "== broader publish_sync / run_sync_or_offload usages =="
rg -n -C 6 --glob '*.py' '\bpublish_sync\s*\(|\brun_sync_or_offload\s*\(|emit_agent|emit_agent_started|emit_agent_ended|emit_llm' .
echo
echo "== deterministic source extractor =="
python3 - <<'PY'
from pathlib import Path
import ast
path = Path("src/praisonai/praisonai/endpoints/a2u_server.py")
tree = ast.parse(path.read_text(), filename=str(path))
for node in ast.walk(tree):
if isinstance(node, ast.AsyncFunctionDef) and node.name == "get_events":
print(f"get_events async iterators/subscribers: {[sub.value.id for sub in ast.walk(node) if isinstance(sub, ast.AsyncFor) and isinstance(sub.target, ast.Name)]}")
if isinstance(node, ast.FunctionDef) and node.name == "publish_sync":
puts = [(sub.value.id, ast.get_text_source(path, sub.lineno, sub.end_lineno).strip())
for sub in ast.walk(node) if isinstance(sub, ast.Call) and isinstance(sub.func, ast.Attribute) and sub.func.attr == "put_nowait"]
print("publish_sync put_nowait calls:", len(puts), puts)
if isinstance(node, ast.AsyncFunctionDef) and node.name == "subscribe":
queue_creations = ast.walk(node)
for sub in queue_creations:
if isinstance(sub, ast.Call) and isinstance(sub.func, ast.Attribute) and sub.func.attr == "setdefault":
print("subscribe queue creation via setdefault:", ast.get_text_source(path, sub.lineno, sub.end_lineno).strip())
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) and node.name in {"_get_queue", "publish_sync", "get_events", "subscribe", "unsubscribe"}:
if any(isinstance(sub, ast.Call) and isinstance(sub.func, ast.Attribute) and sub.func.attr == "call_soon_threadsafe" for sub in ast.walk(node)):
print(f"queued loop safety in {node.name}: call_soon_threadsafe present")
if not any(isinstance(sub, ast.Call) and isinstance(sub.func, ast.Attribute) and sub.func.attr == "call_soon_threadsafe" for sub in ast.walk(node)):
print(f"queued loop safety in {node.name}: call_soon_threadsafe absent")
PYRepository: MervinPraison/PraisonAI
Length of output: 42658
Dispatch A2U queue writes on the queue owner event loop.
publish_sync calls can run from arbitrary threads via run_sync(), while get_events() owns and awaits the asyncio.Queue as a subscription stream. RLock secures the dictionaries, but it does not make asyncio.Queue.put() safe across threads/event loops. Record each queue’s owner loop and schedule put through that loop, or insert a thread-safe bridge between synchronous publishers and async stream consumers.
🤖 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/praisonai/endpoints/a2u_server.py` around lines 181 - 188,
Update publish_sync and the subscription setup around get_events to associate
each asyncio.Queue with its owning event loop, then dispatch each published item
onto that loop rather than writing to the queue directly from the caller thread.
Preserve the existing lock-protected subscription snapshot and ensure queued
delivery remains safe when publish_sync is invoked through run_sync from another
thread or event loop.
| def warn_unsupported_fields(adapter_name: str, spec_extras: Dict[str, Any]) -> None: | ||
| """Warn once per agent when a backend ignores declared YAML fields. | ||
|
|
||
| Non-breaking: pure visibility. ``framework: praisonai`` is treated as | ||
| supporting everything, so no warning is emitted there. | ||
| """ | ||
| if adapter_name not in _ADAPTER_SUPPORTED_FIELDS: | ||
| return | ||
| supported = _ADAPTER_SUPPORTED_FIELDS[adapter_name] | ||
| declared = set(spec_extras.keys()) | ||
| unhandled = declared - supported - _STRUCTURAL_FIELDS | ||
| if unhandled: | ||
| logger.warning( | ||
| "framework=%r ignores YAML field(s) %s for agent %r; " | ||
| "these are only honoured by framework=praisonai.", | ||
| adapter_name, sorted(unhandled), spec_extras.get("role"), | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Report ignored task fields.
The warning path only handles AgentSpec.extras. Task fields can still be silently dropped. AutoGen only uses task description data. CrewAI also ignores task fields outside its explicit task_details.get(...) list. Add task-specific supported-field mappings and call the warning helper for every task_spec.extras.
src/praisonai/praisonai/framework_adapters/base.py#L46-L62: Support task-level field classification and task identifiers.src/praisonai/praisonai/framework_adapters/autogen_adapter.py#L163-L164: Report unsupported fields from eachtask_spec.extras.src/praisonai/praisonai/framework_adapters/crewai_adapter.py#L84-L85: Report unknown fields from eachtask_spec.extras.
📍 Affects 3 files
src/praisonai/praisonai/framework_adapters/base.py#L46-L62(this comment)src/praisonai/praisonai/framework_adapters/autogen_adapter.py#L163-L164src/praisonai/praisonai/framework_adapters/crewai_adapter.py#L84-L85
🤖 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/praisonai/framework_adapters/base.py` around lines 46 - 62,
Extend warn_unsupported_fields in
src/praisonai/praisonai/framework_adapters/base.py#L46-L62 with task-level
supported-field mappings and identifiers, preserving the existing agent-field
behavior. In
src/praisonai/praisonai/framework_adapters/autogen_adapter.py#L163-L164, call
the helper for every task_spec.extras using AutoGen’s supported task fields; in
src/praisonai/praisonai/framework_adapters/crewai_adapter.py#L84-L85, do the
same using CrewAI’s explicit task_details.get(...) fields, so unsupported task
fields are reported.
…signature - tools.py: read max_bytes+1 and reject when a github: tool file exceeds the 1 MiB cap instead of silently persisting truncated (broken) source as success (Greptile P1). - test_interactive_surfaces_parity: _fake_execute now accepts read_only= to match _execute_prompt(prompt, read_only=...) — the stub predated the read-only review signature, so the real path swallowed a TypeError and never set the captured prompt (CI KeyError: 'prompt'). Test guarantee (expand-once) preserved. Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
MervinPraison
added
pipeline/blocked:stale-final
|
Claude finished @MervinPraison's task —— View job CI Failure Fix — PR #3771
Verdict: Already fixed — test bug, not a PR regressionThe CI failure was reported against HEAD Root cause: the test stub This Verification (local, on HEAD
|
praisonai-triage-agent
Bot
removed
the
pipeline/blocked:cooldown
1 participant
Fixes #3770
Summary
Surgical fixes for the three wrapper gaps identified in #3770, reusing patterns already present in the codebase. Scoped down per AGENTS.md to stay lightweight — no new user-facing knobs.
Gap 2 —
praisonai tools add(security, shipped first)PRAISONAI_ALLOW_LOCAL_TOOLSopt-in every other loader enforces (both local-file andgithub:paths).exec_moduleon add with staticast.parseintrospection — no code execution at add time.urlretrieveredirect-follow), safe single-basename filename.--sha256suggestion to avoid adding a new CLI knob (scope creep).Gap 3 —
serveasync correctnessendpoints/server.py: all four handlers (/v1/chat/completions,/v1/completions,/v1/models,/v1/tools/invoke) offload the syncprovider.invokeviaasyncio.to_threadso blocking LLM I/O no longer stalls the event loop.openai_compat.py: simulated streaming emits word-by-word (whitespace preserved) instead of one SSE frame per character.a2u_server.py:A2UEventBusmutations guarded bythreading.RLock(publish snapshots under the lock, delivers outside it);get_event_bus()uses double-checked locking;/a2u/healthnow calls_authenticate_requestlike every other A2U route.task_callbackvia a speculativemessage_callbackkwarg —initiate_chatsdoes not accept it and it would break.Gap 1 — adapter parity (visibility)
warn_unsupported_fieldshelper inframework_adapters/base.pylogs a warning when a backend silently drops declared YAML fields (e.g.approval), wired into the CrewAI and AutoGen adapters.agent_callback(declared but never fired) is now invoked per agent, matchingCrewAIAdapter.Test plan
warn_unsupported_fieldsverified: warns for unsupportedapproval, silent for supported fields and forframework: praisonai.tests/unit/test_endpoints_unified.py+test_async_daemon_deployment.py: 42 passed.Generated with Claude Code
Summary by CodeRabbit
Security
Reliability
Performance
Compatibility