fix: round-trip long select/reply callback values via durable payload store#3327
fix: round-trip long select/reply callback values via durable payload store#3327praisonai-triage-agent[bot] wants to merge 2 commits into
Conversation
β¦ store (fixes #3312) Generalise the durable-reference pattern PraisonAI already uses for approvals to ordinary interactive callbacks, so long reply/select values round-trip losslessly even on 64-byte-cap channels. - protocols.py: new CallbackPayloadStoreProtocol + bounded, zero-dependency InMemoryCallbackPayloadStore (TTL + FIFO cap). - presentation.py: encoders persist an overflowing value under a short ref and emit reply:@<ref> / select:<action_id>:@<ref>. Inline values unchanged; no store preserves prior hash fallback. adapt_presentation gains an optional callback_store= kwarg. - interactive.py: InteractiveRegistry(store=...) resolves @<ref> back to the exact value on dispatch (preserving the select action_id: prefix), fails closed on unknown/expired refs, and never mistakes an ordinary @ value for a ref. - bots/__init__.py: export the new symbols. 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.
|
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review detailsβοΈ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: π Files selected for processing (5)
β¨ 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 SummaryAdds stored references for oversized interactive callback values.
Confidence Score: 5/5This follow-up appears safe to merge because no new blocking failure eligible for an additional comment was identified. No blocking failure remains in the accepted follow-up findings. Important Files Changed
Sequence DiagramsequenceDiagram
participant Renderer
participant Store as Callback Payload Store
participant Channel
participant Registry as Interactive Registry
participant Handler
Renderer->>Store: Persist oversized callback value
Store-->>Renderer: Short reference
Renderer->>Channel: "Send callback containing @reference"
Channel->>Registry: Dispatch callback
Registry->>Store: Resolve reference
alt Reference exists
Store-->>Registry: Original value
Registry->>Handler: Dispatch decoded value
else Missing or expired
Store-->>Registry: No value
Registry-->>Channel: Decline callback
end
Reviews (2): Last reviewed commit: "fix(bots): declare put_sync in CallbackP..." | Re-trigger Greptile |
|
@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 Final Architecture Review β PR #3327
Verdict: β Approve (with one fix applied)The change is a clean, additive, protocol-driven generalisation of the existing Reviewer feedback triage1. Greptile P1 β "Protocol stores cannot persist" ( 2. Greptile P1 β "Callback store remains unwired" β BY DESIGN, not a blocker. Independent checks
Files modified (this review)
Skipped (intentionally)
Tests
--- Β· Branch: |
The synchronous render path (adapt_presentation) persists oversize callback values via put_sync, but the protocol only declared async put/get. A durable backend implementing only the declared contract was silently rejected by the encoder, degrading long callbacks to the non-routable hash fallback. Declaring put_sync (with docs on both entry points) makes the contract match what the encoder requires. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
|
@claude this PR has merge conflicts with |
|
Claude finished @MervinPraison's task ββ View job Rebase onto main β PR #3327
Result:
|
| Piece | On main? |
Where |
|---|---|---|
CallbackPayloadStoreProtocol + InMemoryCallbackPayloadStore |
β | bots/protocols.py:1280 |
@<ref> encoding for oversize reply/select |
β | bots/presentation.py (adapt_presentation(callback_store=)) |
| Registry resolution + fail-closed dispatch | β | bots/interactive.py:286, create_registry(store=) |
| Public exports | β | bots/__init__.py:34,123 |
| Production wiring (Telegram render + registry share one store) | β | praisonai-bot (_presentation_renderer.py, telegram.py) |
Notably, main's merged design is cleaner and more complete than this branch:
- It uses a simple synchronous
put/get(noput/put_syncasync split), so the "protocol stores cannot persist" concern Greptile raised here doesn't exist there. - It fully wires the store through to the
praisonai-botruntime β the exact follow-up this PR's own review left open (long values now round-trip in production, not just tests).
Decision (per AGENTS.md β lightweight, no scope creep)
I did not complete the rebase or force-push. Rebasing would only resurrect a divergent, redundant design (~223 lines of churn against main) for a feature that is already merged and better implemented β that is scope creep, not value.
- Files I would have resolved:
protocols.py,interactive.py,presentation.py,test_callback_payload_store.pyβ all four are add/add or content conflicts against the already-merged feature. - Action taken: aborted the rebase; branch left untouched (no force-push).
Recommendation: Close PR #3327 as superseded by #3322. Issue #3312 is already fixed on main. I can't close the PR myself, so leaving that to a maintainer.
Β· Branch: claude/issue-3312-20260723-1030
Fixes #3312
Summary
Interactive
reply/selectmenus whose option values exceed the channel callback byte-cap (_MAX_CALLBACK_LEN = 64, e.g. Telegram's 64-byte inline limit) were silently broken: oversize values were hashed with no server-side store to reverse them, so quick-reply clicks did nothing and select clicks delivered an unusable digest.This generalises the durable-reference pattern PraisonAI already uses for approvals (
ApprovalStoreProtocol) to ordinary interactive callbacks, so long values round-trip losslessly on every channel.Changes (core-only, additive, backward compatible)
bots/protocols.pyβ newCallbackPayloadStoreProtocol(peer ofApprovalStoreProtocol) + bounded, zero-dependencyInMemoryCallbackPayloadStore(TTL + FIFO cap;put_sync/put/get).bots/presentation.pyβ encoders persist an overflowing value under a short, collision-resistant ref and emitreply:@<ref>/select:<action_id>:@<ref>. Inline values are unchanged; with no store the prior hash fallback is preserved.adapt_presentationgains an optionalcallback_store=kwarg.bots/interactive.pyβInteractiveRegistry(store=...)resolves@<ref>β exact value on dispatch (preserving the selectaction_id:prefix), fails closed on unknown/expired refs, and only treats a strict@<16-hex>token as a ref so an ordinary@value (e.g.@channel,user@example.com) is never misrouted.bots/__init__.pyβ exports the new symbols.Design notes (kept lightweight per AGENTS.md)
praisonai-botruntime (injected into the registry), mirroringApprovalStore. No new dependencies, no new user-facing API β all new params are optional.Testing
tests/unit/test_callback_payload_store.py(18 tests): store lifecycle (TTL, FIFO cap, protocol conformance), inline passthrough, ref encoding within the byte-cap (incl. longaction_id), no-store hash fallback, end-to-end select/reply round-trip, fail-closed paths, and@-in-value handling. Ran alongside existing presentation/interactive/reply/bots suites β 84 passed, 0 regressions. (Pre-existingtest_botos_protocol.pyfailures are unrelated: they require thepraisonaiwrapper package.)Generated with Claude Code