fix: restore _delivery.py removed while still imported by durable delivery#2129
fix: restore _delivery.py removed while still imported by durable delivery#2129cursor[bot] wants to merge 2 commits into
Conversation
…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>
|
@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 |
…t for error detection - Store asyncio task reference to prevent premature garbage collection - Extract PERMANENT_ERROR_PREFIX constant for robust error detection - Replace fragile string matching with startswith() for permanent errors Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
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