fix: restore _delivery.py removed while still imported by durable delivery#2129
Conversation
|
@coderabbitai review |
|
/review |
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? |
✅ Action performedReview finished.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a new ChangesBot Delivery Layer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first — incorporate their findings. Review areas:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/praisonai/praisonai/bots/_delivery.py (2)
89-89: 💤 Low valueStore the task reference to prevent premature garbage collection.
The task created for typing indicator is not stored, which could lead to the task being garbage collected before completion if no other references exist. While
_show_typinghandles exceptions internally, storing the reference is the recommended pattern.♻️ Suggested fix
# Show typing indicator if supported if typing and caps.supports_typing: - asyncio.create_task(self._show_typing(channel_id)) + self._typing_task = asyncio.create_task(self._show_typing(channel_id))Add the instance variable in
__init__:self._typing_task: Optional[asyncio.Task] = 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/praisonai/bots/_delivery.py` at line 89, The asyncio.create_task call for self._show_typing(channel_id) at line 89 does not store the returned task reference, which can cause premature garbage collection. Add an instance variable self._typing_task of type Optional[asyncio.Task] initialized to None in the __init__ method, and then assign the result of asyncio.create_task(self._show_typing(channel_id)) to self._typing_task instead of discarding it. This ensures the task reference is maintained and prevents it from being garbage collected before completion.Source: Linters/SAST tools
503-504: ⚡ Quick winFragile string-based permanent error detection.
The permanent error check relies on matching the exact prefix
"Permanent error:"fromdeliver_with_retry(line 323). This coupling is fragile—if the error message format changes, this detection silently fails and permanent errors get retried indefinitely.Consider using a structured return value or a sentinel constant for error classification instead of string matching.
♻️ Suggested approach
Define a constant for the prefix and use it in both places:
# At module level PERMANENT_ERROR_PREFIX = "Permanent error:" # In deliver_with_retry (line 323) return False, f"{PERMANENT_ERROR_PREFIX} {last_error}" # In DurableDelivery.send (line 503) permanent = error and error.startswith(PERMANENT_ERROR_PREFIX)Or use a structured return like
DeliveryResult(success: bool, error: Optional[str], permanent: bool).🤖 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/bots/_delivery.py` around lines 503 - 504, The permanent error detection at the mark_failed call relies on substring matching "Permanent error:" which is fragile and tightly coupled to the error message format in deliver_with_retry. Extract the error prefix into a module-level constant (define it near the top of the file), then update the permanent error check to use startswith() with this constant instead of the in operator, and ensure the deliver_with_retry function also uses this same constant when constructing its error message so that both locations reference the same value.
🤖 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.
Nitpick comments:
In `@src/praisonai/praisonai/bots/_delivery.py`:
- Line 89: The asyncio.create_task call for self._show_typing(channel_id) at
line 89 does not store the returned task reference, which can cause premature
garbage collection. Add an instance variable self._typing_task of type
Optional[asyncio.Task] initialized to None in the __init__ method, and then
assign the result of asyncio.create_task(self._show_typing(channel_id)) to
self._typing_task instead of discarding it. This ensures the task reference is
maintained and prevents it from being garbage collected before completion.
- Around line 503-504: The permanent error detection at the mark_failed call
relies on substring matching "Permanent error:" which is fragile and tightly
coupled to the error message format in deliver_with_retry. Extract the error
prefix into a module-level constant (define it near the top of the file), then
update the permanent error check to use startswith() with this constant instead
of the in operator, and ensure the deliver_with_retry function also uses this
same constant when constructing its error message so that both locations
reference the same value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cf9e037a-c755-4390-8d14-9242947062ae
📒 Files selected for processing (2)
src/praisonai/praisonai/bots/_delivery.pysrc/praisonai/tests/unit/bots/test_delivery_imports.py
|
@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 |
|
Claude finished @MervinPraison's task —— View job Architecture Review Complete ✅Phase 1: Architecture Review✅ Protocol-driven design: The module properly imports protocols and capabilities from TYPE_CHECKING. Heavy implementations like ✅ Backward compatibility: The PR correctly restores a module that was still being imported. No API changes, just restoring existing functionality. All imports verified working. ✅ Performance: Uses lazy imports with TYPE_CHECKING, no hot-path regressions identified. No heavy module-level imports. Phase 2: Fixed IssuesImplemented both fixes identified by CodeRabbit:
Files Modified
Phase 3: Final Verdict ✅APPROVED - This PR correctly fixes the critical bug where
The changes are minimal, focused, and necessary to fix the |
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? |
|
@claude Merge conflict resolution audit (fresh rebase onto
Merge & CI status
CI summary (latest rollup)
CI is green on the pre-merge branch tip, but GitHub reports merge conflicts with current Conflict inventory (rebase
|
| File | Conflict hunks | Type |
|---|---|---|
src/praisonai/praisonai/bots/_delivery.py |
10 | add/add (both sides added the file independently) |
No other files reached conflict during rebase (failure on commit 34e4c6341).
src/praisonai/praisonai/bots/_delivery.py
Hunk 1 — typing task lifetime (UnifiedDelivery.__init__)
main (HEAD during rebase):
self._capabilities: Optional["PlatformCapabilities"] = None
self._rate_limiter: Optional[RateLimiter] = None
self._streamer: Optional[DraftStreamer] = None
self._background_tasks: set[asyncio.Task] = set()PR side (34e4c6341): omits _background_tasks (first commit had fire-and-forget typing; second commit 34e7ee0c0 adds _typing_task instead).
Guidance: Keep main’s _background_tasks set (stronger GC/lifetime than a single _typing_task or fire-and-forget). If you port anything from the PR follow-up commit, prefer retaining _background_tasks unless you deliberately unify on one pattern.
Hunk 2 — start typing indicator
main:
if typing and caps.supports_typing:
task = asyncio.create_task(self._show_typing(channel_id))
self._background_tasks.add(task)
task.add_done_callback(self._background_tasks.discard)PR (34e4c6341):
if typing and caps.supports_typing:
asyncio.create_task(self._show_typing(channel_id))PR tip (34e7ee0c0 on branch, not yet applied during failed rebase):
if typing and caps.supports_typing:
self._typing_task = asyncio.create_task(self._show_typing(channel_id))Guidance: Take main (background task tracking). Do not regress to fire-and-forget.
Hunks 3, 5, 7, 8 (partial) — rate limiting calls
main:
if caps.needs_rate_limit:
await self._acquire_rate_limit(channel_id)PR:
if caps.needs_rate_limit:
await self._acquire_rate_limit()Guidance: Keep main — per-channel rate limiting is intentional on latest main.
Hunks 4 & 6 — streaming completion flag
main:
streaming_completed = False
while current_pos < len(text):
...
if next_pos >= len(text):
streaming_completed = True
breakPR: removes streaming_completed tracking.
Guidance: Keep main’s streaming_completed logic; it avoids duplicate final edits when streaming already completed.
Hunk 8 — final edit vs elif not streaming_completed
main:
elif not streaming_completed:
# Final edit with complete content (fits in limit and wasn't already sent)
await asyncio.sleep(caps.edit_interval_ms / 1000.0)
if caps.needs_rate_limit:
await self._acquire_rate_limit(channel_id)
await self.bot.edit_message(...)PR:
else:
# Final edit with complete content (fits in limit)
...
await self._acquire_rate_limit()Guidance: Keep main’s elif not streaming_completed branch and channel_id rate-limit calls.
Hunks 9–10 — _acquire_rate_limit signature
main:
async def _acquire_rate_limit(self, channel_id: Optional[str] = None) -> None:
"""Acquire rate limit token if platform needs it.
Args:
channel_id: Optional channel ID for per-channel limiting
"""
...
await self._rate_limiter.acquire(channel_id)PR:
async def _acquire_rate_limit(self) -> None:
...
await self._rate_limiter.acquire()Guidance: Keep main signature and acquire(channel_id).
Non-conflicting value from this PR (port onto main base)
PR branch tip adds (worth keeping if not already on main):
PERMANENT_ERROR_PREFIX = "Permanent error:"
...
return False, f"{PERMANENT_ERROR_PREFIX} {last_error}"
...
permanent = error and error.startswith(PERMANENT_ERROR_PREFIX)
...
if not success and error and error.startswith(PERMANENT_ERROR_PREFIX):main still uses inline "Permanent error:" strings and "Permanent error:" in error — port the constant + startswith pattern from commit 34e7ee0c0 after taking main’s file as the conflict base.
Tests only on this PR branch
src/praisonai/tests/unit/bots/test_delivery_imports.py exists on the PR branch but not on main. After rebase, re-add this file unchanged (or equivalent coverage) so the #2054 regression stays covered.
Recommended resolution strategy
- Rebase onto current
main:git fetch origin main && git rebase origin/main - For
_delivery.pyadd/add conflict:git checkout --ours src/praisonai/praisonai/bots/_delivery.pyduring rebase (ours = updatedmain), then manually apply:PERMANENT_ERROR_PREFIX+startswithusages from34e7ee0c0- Restore
test_delivery_imports.pyfrom the PR branch
- Do not re-introduce the full pre-fix: gateway session data loss and restore deleted _delivery module #2102 module from
34e4c6341— it diverges on rate limiting and streaming. - Continue rebase; verify commit 2/2 (
34e7ee0c0) — it may become empty or need manual squashing into the port above. - Run:
pytest src/praisonai/tests/unit/bots/test_delivery_imports.pyand relevant bot delivery tests. - Push:
git push --force-with-lease origin cursor/critical-bug-investigation-3971 - Re-run / wait for CI on the rebased tip and confirm GitHub shows mergeable.
Action items for @claude
- Rebase onto
origin/mainand resolve all 10 hunks in_delivery.pyas above (main base + port constant/tests). - Push with
--force-with-lease. - Fix any CI regressions from the 50-commit drift (expect none if scope stays minimal).
- Post a final verdict comment after the rebased branch is mergeable:
- If the only remaining value is
PERMANENT_ERROR_PREFIX+ regression test → approve as a small follow-up fix. - If the branch still tries to replace fix: gateway session data loss and restore deleted _delivery module #2102’s
_delivery.pywholesale → request changes / consider closing as superseded by fix: gateway session data loss and restore deleted _delivery module #2102.
- If the only remaining value is
Interim verdict (pre-rebase): Blocked — conflicting with main. The original bug (missing _delivery.py) is already fixed on main; this PR should shrink to a targeted port of review fixes + tests, not a full module re-add.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| import asyncio | ||
| import logging | ||
| from typing import TYPE_CHECKING, Optional, Union, Dict, Any, List, Awaitable, Callable, Protocol |
There was a problem hiding this comment.
Awaitable and Callable are imported but never referenced anywhere in this file. Removing them keeps the import line clean and avoids lint warnings.
| from typing import TYPE_CHECKING, Optional, Union, Dict, Any, List, Awaitable, Callable, Protocol | |
| from typing import TYPE_CHECKING, Optional, Union, Dict, Any, List, Protocol |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| if len(content) <= max_length: | ||
| await adapter.send_message(channel_id, content, **send_kwargs) | ||
| return 1 |
There was a problem hiding this comment.
deliver_chunked guards the fast path with len(content) (character count) but chunk_message can operate in other length units (e.g. bytes). When a caller passes a max_length tuned for a byte-counting platform, a message whose len() is below the limit may still be over-limit in bytes, so chunk_message would split it — but the leading return 1 branch would have already sent it unsplit via a single send_message call, potentially exceeding the platform's limit. Consider using _calculate_length with an explicit length_unit here, or at minimum document that this function always counts in characters.
| if len(content) <= max_length: | |
| await adapter.send_message(channel_id, content, **send_kwargs) | |
| return 1 | |
| if _calculate_length(content, "chars") <= max_length: | |
| await adapter.send_message(channel_id, content, **send_kwargs) | |
| return 1 |
| "DurableDelivery", | ||
| ] No newline at end of file |
There was a problem hiding this comment.
.
| "DurableDelivery", | |
| ] | |
| "DurableDelivery", | |
| ] |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Claude finished @MervinPraison's task —— View job ✅ Merge Conflict Resolution CompleteSuccessfully rebased the branch onto the latest main and resolved all conflicts. Todo List:
Changes SummaryAfter rebasing onto main (which already restored
Files Modified
Final Verdict: ✅ APPROVEDWhile the original bug (missing
The changes are minimal, focused, and improve maintainability. Ready to merge once CI passes. --- | |
…ivery PR #2054 deleted bots/_delivery.py believing it was superseded by bots/delivery.py, but delivery.py only provides DeliveryRouter and ChannelDirectory. DurableDelivery, deliver_with_retry, and deliver_chunked remain referenced by _durable_adapter.py and bots/__init__.py, causing ModuleNotFoundError on any durable outbound delivery usage. Restore the module and add regression tests for the public exports. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
praisonai-triage-agent
Bot
force-pushed
the
cursor/critical-bug-investigation-3971
branch
from
34e7ee0 to
bc34800
Compare
2 participants
Bug and impact
PR #2054 (
fix: remove three orphaned wrapper modules) deletedbots/_delivery.py, believing it was superseded bybots/delivery.py. Those modules are unrelated:delivery.py—DeliveryRouter/ChannelDirectory(proactive routing)_delivery.py—DurableDelivery,deliver_with_retry,deliver_chunked(crash-safe outbound delivery)_durable_adapter.pyandbots/__init__.pystill import from._delivery, so any code path that enables durable outbound delivery crashes withModuleNotFoundError. This breaks crash-safe channel reply delivery added in PR #2036.Concrete trigger:
from praisonai.bots import DurableDeliveryor enablingDurableAdapterMixin.setup_durable_delivery(outbox_path=...).Root cause
Dead-code removal grep only checked for
UnifiedDelivery/create_deliverycall sites and missed active imports ofDurableDeliveryand related exports.Fix
bots/_delivery.pyfrom the pre-deletion revisiontests/unit/bots/test_delivery_imports.pyregression tests for the public exportsValidation
python3 -c "from praisonai.bots import DurableDelivery"succeedspytest tests/unit/bots/test_delivery_imports.py— 5 passedSummary by CodeRabbit
New Features
Tests