fix: make chat approval backends durable across restart#3188
fix: make chat approval backends durable across restart#3188praisonai-triage-agent[bot] wants to merge 2 commits into
Conversation
Chat approval backends (Slack, Telegram, Discord, Webhook, HTTP) held the pending approval only in an in-memory polling coroutine, so a gateway/bot restart while an approval was outstanding stranded the blocked agent run. Add an opt-in DurableApprovalMixin to the shared _approval_base module that wraps the existing ApprovalStore: it persists the request before polling, records the final decision, and exposes rehydrate() to recover outstanding approvals on startup. Each chat backend now accepts an optional store= and a shared DEFAULT_APPROVAL_TIMEOUT. When no store is given behaviour is unchanged (fully backward-compatible); no new dependency is introduced. Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
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? |
✅ 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:
📝 WalkthroughWalkthroughChat approval backends now share a 300-second timeout constant and optional durable approval storage. Pending requests are persisted before polling, final decisions are recorded, and pending approvals can be rehydrated after restart. Unit tests cover lifecycle behavior, compatibility without a store, and HTTP timeout resolution. ChangesDurable approval lifecycle
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Agent
participant ChatApprovalBackend
participant DurableApprovalMixin
participant ApprovalStore
Agent->>ChatApprovalBackend: request approval
ChatApprovalBackend->>DurableApprovalMixin: persist pending request
DurableApprovalMixin->>ApprovalStore: save pending state
ChatApprovalBackend-->>Agent: poll for approval decision
ChatApprovalBackend->>DurableApprovalMixin: resolve decision
DurableApprovalMixin->>ApprovalStore: record final decision
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
Greptile SummaryThis PR wires the existing
Confidence Score: 5/5Safe to merge — the mixin is fully opt-in and the no-store path is unchanged, so existing deployments are unaffected. All five backends correctly call _resolve_pending on every exit path (early-return guards, happy path, and outer except handlers). The one finding — _build_blocks/_build_fallback_text being called after _persist_pending but before the Slack try/except — is an edge case that auto-expires within the configured timeout window and does not affect correctness for the normal approval flow. _slack_approval.py: the two message-builder calls sit outside the try/except scope; moving them inside would close the last potential dangling-row path. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Agent
participant Backend as ChatBackend (Slack/Telegram/etc.)
participant Mixin as DurableApprovalMixin
participant Store as ApprovalStore (SQLite)
participant Platform as Chat Platform
Agent->>Backend: request_approval(request)
Backend->>Mixin: _persist_pending(request, timeout)
Mixin->>Store: persist(approval_id, request, expires_at)
Store-->>Mixin: ok
Backend->>Platform: POST message / open poll
alt Happy path
Platform-->>Backend: user replies approve/deny
Backend->>Mixin: _resolve_pending(request, decision)
Mixin->>Store: resolve(approval_id, decision)
else Early exit (no channel / post failed)
Backend->>Mixin: _resolve_pending(request, denial)
Mixin->>Store: resolve(approval_id, denial)
else Exception
Backend->>Mixin: _resolve_pending(request, denial)
Mixin->>Store: resolve(approval_id, denial)
end
Backend-->>Agent: ApprovalDecision
note over Backend,Store: On restart
Backend->>Mixin: rehydrate()
Mixin->>Store: list_pending()
Store-->>Mixin: [(approval_id, request), ...]
Mixin-->>Backend: pending list for operator re-attachment
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Agent
participant Backend as ChatBackend (Slack/Telegram/etc.)
participant Mixin as DurableApprovalMixin
participant Store as ApprovalStore (SQLite)
participant Platform as Chat Platform
Agent->>Backend: request_approval(request)
Backend->>Mixin: _persist_pending(request, timeout)
Mixin->>Store: persist(approval_id, request, expires_at)
Store-->>Mixin: ok
Backend->>Platform: POST message / open poll
alt Happy path
Platform-->>Backend: user replies approve/deny
Backend->>Mixin: _resolve_pending(request, decision)
Mixin->>Store: resolve(approval_id, decision)
else Early exit (no channel / post failed)
Backend->>Mixin: _resolve_pending(request, denial)
Mixin->>Store: resolve(approval_id, denial)
else Exception
Backend->>Mixin: _resolve_pending(request, denial)
Mixin->>Store: resolve(approval_id, denial)
end
Backend-->>Agent: ApprovalDecision
note over Backend,Store: On restart
Backend->>Mixin: rehydrate()
Mixin->>Store: list_pending()
Store-->>Mixin: [(approval_id, request), ...]
Mixin-->>Backend: pending list for operator re-attachment
Reviews (2): Last reviewed commit: "fix: resolve durable pending row on all ..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/praisonai-bot/praisonai_bot/bots/_discord_approval.py (1)
196-201: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDangling pending state on error and early-return paths.
The
request_approvalmethod persists the pending state early via_persist_pending, but constructs and returns a deniedApprovalDecisionon failure paths without calling_resolve_pending. This leaves the request durably dangling as "pending" in the store, meaning a process restart will erroneously rehydrate a request that already failed.Consider capturing the fallback decision in a variable and resolving it before returning, or using a
finallyblock to ensure resolution.
src/praisonai-bot/praisonai_bot/bots/_discord_approval.py#L196-L201: Call_resolve_pendinghere, as well as on the early returns for missingchannel_id(L157) and missingmsg_id(L178).src/praisonai-bot/praisonai_bot/bots/_slack_approval.py#L204-L209: Call_resolve_pendinghere, as well as on the early returns for missing channel (L166) and failed post (L183).src/praisonai-bot/praisonai_bot/bots/_telegram_approval.py#L184-L189: Call_resolve_pendinghere, as well as on the early returns for missingchat_id(L143) and failed send (L164).src/praisonai-bot/praisonai_bot/bots/_webhook_approval.py#L182-L187: Call_resolve_pendinghere before returning the error decision.🤖 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-bot/praisonai_bot/bots/_discord_approval.py` around lines 196 - 201, Ensure every failure and early-return path in request_approval resolves persisted pending state before returning a denied decision. Update _discord_approval.py:196-201 and its missing channel_id/msg_id paths, _slack_approval.py:204-209 and its missing channel/failed post paths, _telegram_approval.py:184-189 and its missing chat_id/failed send paths, and _webhook_approval.py:182-187 to call _resolve_pending with the fallback decision before returning; preserve successful approval flows.
🤖 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.
Outside diff comments:
In `@src/praisonai-bot/praisonai_bot/bots/_discord_approval.py`:
- Around line 196-201: Ensure every failure and early-return path in
request_approval resolves persisted pending state before returning a denied
decision. Update _discord_approval.py:196-201 and its missing channel_id/msg_id
paths, _slack_approval.py:204-209 and its missing channel/failed post paths,
_telegram_approval.py:184-189 and its missing chat_id/failed send paths, and
_webhook_approval.py:182-187 to call _resolve_pending with the fallback decision
before returning; preserve successful approval flows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9ba95eae-b507-4f6f-ab89-4a527c3ae313
📒 Files selected for processing (7)
src/praisonai-bot/praisonai_bot/bots/_approval_base.pysrc/praisonai-bot/praisonai_bot/bots/_discord_approval.pysrc/praisonai-bot/praisonai_bot/bots/_http_approval.pysrc/praisonai-bot/praisonai_bot/bots/_slack_approval.pysrc/praisonai-bot/praisonai_bot/bots/_telegram_approval.pysrc/praisonai-bot/praisonai_bot/bots/_webhook_approval.pysrc/praisonai-bot/tests/unit/bots/test_durable_channel_approval.py
![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 #3188 ✅ Approve (with fixes pushed)Reviewed against AGENTS.md, validated all reviewer findings, and pushed fixes to this branch ( Phase 1 — Architecture
Phase 2 — Valid issues fixedConfirmed valid (CodeRabbit + Greptile, same root cause): backends called Files modified:
Skipped: the CodeRabbit pre-merge "actor authorization / typed actions" scope note — those already exist in Phase 3 — Tests
Verdict: All blocking review findings resolved. LGTM — ready to merge. ✅ |
… paths Slack, Telegram, Discord, and Webhook approval backends persisted the pending approval before polling but skipped _resolve_pending on their early-return guards (missing chat/channel id, failed send) and outer except handlers. This left stale status='pending' rows that rehydrate() surfaced as false-positive outstanding approvals after a restart. Each missed exit path now records the fallback denial via _resolve_pending before returning, matching the HTTP reference backend. Adds regression tests for the early-exit and exception paths. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
MervinPraison
added
pipeline/blocked:stale-final
praisonai-triage-agent
Bot
removed
the
pipeline/blocked:cooldown
1 participant
Fixes #3183
Summary
Chat approval backends (Slack, Telegram, Discord, Webhook, HTTP) held the pending human-in-the-loop approval only in an in-memory polling coroutine. If the gateway/bot restarted while an approval was outstanding, the pending request was lost and the blocked agent run was stranded.
This wires those backends to the durable
ApprovalStorethat already exists in-tree — kept intentionally minimal per the "lightweight and powerful" mandate (no newDurableChannelApprovalbase class, no new CLI/YAML knobs, no new dependency).Changes
_approval_base.py: new opt-inDurableApprovalMixin(_persist_pending/_resolve_pending/rehydrate) that reuses the existing SQLite+WALApprovalStore; plus a single sharedDEFAULT_APPROVAL_TIMEOUT(300s).store=. The request is persisted before polling and the final decision recorded;rehydrate()recovers outstanding approvals on startup.Backward compatibility
Fully backward-compatible: when no
storeis supplied every durable method is a no-op and behaviour is exactly as before.Test plan
test_durable_channel_approval.py(6 tests): persist survives "restart", resolve closes the row, no-store no-op, end-to-end through HTTP backend.Generated with Claude Code
Summary by CodeRabbit
New Features
Tests