fix(channels): an edit names what changes, and a failed post is written down - #6649
fix(channels): an edit names what changes, and a failed post is written down#6649mmabrouk wants to merge 3 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true📝 SummarySummary by CodeRabbit
WalkthroughChannel edit DTOs now support partial updates that preserve omitted stored values. The service layers connection and agent edits before persistence. Outbox delivery records failures, retries failed turn-start events, and reuses deterministic delivery keys. ChangesChannel edit semantics
Outbox delivery retries
Priority: ⬇️ Low — Defer this channel edit and outbox retry change because its supplied scope is limited to preserving partial edits and recording failed deliveries. Estimated code review effort: 3 (Moderate) | ~30 minutes Merge Risk: 🟡 Moderate · up to Retries can create duplicate Slack messages after an ambiguous failure, and invalid agent edits can fail after request validation. Resolve these issues before merge. Sequence Diagram(s)sequenceDiagram
participant OutboxWorker
participant ChannelsDAO
participant ChannelAdapter
OutboxWorker->>ChannelsDAO: Load CREATED or FAILED event
OutboxWorker->>ChannelAdapter: Post message with delivery key
ChannelAdapter-->>OutboxWorker: Return success or error
OutboxWorker->>ChannelsDAO: Persist FAILED or SENT status
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
|
Codex (astra, medium, simplify skill) reviewed this lane. Verdict: "request changes" on the delivery state, three points, all real, all applied in the last commit:
Also from the review: the agent edit's data gets its own model with both fields optional, so a policy-only edit is a valid request (the old model required @coderabbitai review |
|
✅ Action performedReview finished.
|
…en down - A plain rename nulled a connection's whole data blob, credential reference included, and bricked it for good (F98). A policy-only agent edit reset the default flag and muted the connection (F91). Both edit models now treat an omitted field as unchanged: the service lays the fields the caller sent over the stored row, merging data key by key and flags field by field, before the write. - A rejected post left the outbox row in CREATED forever, which reads as "not attempted yet" from outside (F87). The worker now writes FAILED with the platform's reason before it re-raises. Claude-Session: https://claude.ai/code/session_01HXt8WBUK9MrWKNTYbUKpaT
- The delivery token is keyed on the row and its content, not on the row's update time. A retry of the same content after a FAILED write reuses the token, so a post the platform accepted but whose reply timed out is never duplicated; an edit to new content mints a new one. - A FAILED indicator row is attempted again on redelivery instead of being skipped as "already sent". - A success after a failure replaces the recorded failure status. - An agent edit's data has its own model with both fields optional, so a policy-only edit is a valid request; an explicit null policy clears it. - Tests: failed-then-successful retry with a stable token, and the null policy clearing case. Claude-Session: https://claude.ai/code/session_01HXt8WBUK9MrWKNTYbUKpaT
9b41cba to
cc30cdd
Compare
c45f7e9 to
60f1363
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
api/oss/src/tasks/asyncio/channels/outbox.py (1)
301-306: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRoute outbox state transitions through
ChannelsService.The failure and success branches call
self.channels_service.channels_dao.transition_outbox_eventdirectly. This bypasses the requiredService -> DAO Interfaceboundary. Add aChannelsServicetransition method and call it from both branches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Advanced
Run ID: 670e0f50-5468-4302-a1ba-186485ca3dcc
📒 Files selected for processing (6)
api/oss/src/core/channels/dtos.pyapi/oss/src/core/channels/service.pyapi/oss/src/dbs/postgres/channels/mappings.pyapi/oss/src/tasks/asyncio/channels/outbox.pyapi/oss/tests/pytest/unit/channels/test_channels_edit_semantics.pyapi/oss/tests/pytest/unit/channels/test_channels_outbox_worker.py
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review.
…odeRabbit) An omitted references keeps the stored workflow; sending references: null would wipe the agent's only runnable target and surface as a confusing downstream ValidationError when the merged data revalidates. Refuse it at the edit boundary instead, with a clear message. policy: null still clears. Claude-Session: https://claude.ai/code/session_01HXt8WBUK9MrWKNTYbUKpaT
1 participant
Context
Stacked on #6647. Two robustness defects from the August live QA, F91, F98 and F87 in the ledger.
Changes
Tests
https://claude.ai/code/session_01HXt8WBUK9MrWKNTYbUKpaT