fix(channels): unblock the first live deploy (slug, handshake, reference key, space kind, listing) - #6646
Draft
mmabrouk wants to merge 8 commits into
Draft
fix(channels): unblock the first live deploy (slug, handshake, reference key, space kind, listing)#6646mmabrouk wants to merge 8 commits into
mmabrouk wants to merge 8 commits into
Conversation
…ce key Three defects from the August live deploy (F70, F71, F73) kept a fresh stack from ever answering a Slack message. - A connection created from the setup form has no slug, so the insert died on a NOT NULL constraint and the error read "already exists". The service now names the connection after what verification discovered (Slack's workspace name) and derives a slug from that name with a short random suffix. A caller-supplied name or slug is kept. An unrelated integrity error now names the violated constraint, and a failed insert discards the credential secret it just wrote instead of leaving one orphan per retry. - Slack registers an events URL only after the endpoint echoes its url_verification challenge. The Slack ingress now echoes it, unsigned and shape-strict; everything else on the route stays signed. - The settings form filed the agent's workflow under a key nothing resolves. It now uses workflow_variant, and the agent data DTO refuses a reference key outside the families the workflows service resolves, so a bad key fails at write time instead of on the first turn. Claude-Session: https://claude.ai/code/session_01HXt8WBUK9MrWKNTYbUKpaT
The release moved two helpers the channels pages imported: the date formatter now lives in @agenta/shared, and the API key call in @agenta/settings, which returns the key string directly. The settings sidebar's exhaustive icon switch gets a channels case. The generated update-secret payload type gains the channel secret member the API already declares, so the secret state code type-checks again. Claude-Session: https://claude.ai/code/session_01HXt8WBUK9MrWKNTYbUKpaT
The ingress dropped the adapter's space classification when it stored an inbound event, so every space landed as a group and a kind-level grant such as "allow in DMs" never matched. The event now carries the kind through, and the DM seam test that exercises the real ingress passes. Three integration and acceptance fixtures also learn what the code grew after they were written: the ingress seam's duck-typed adapter answers the deactivation probe, the deactivation seam sets the hosted app secret the adapter verifies against, the DM seam marks its connection verified, and the live Slack adapter test builds the adapter the way the composition root does, with the real base URL. Claude-Session: https://claude.ai/code/session_01HXt8WBUK9MrWKNTYbUKpaT
… ones Space discovery called conversations.list once, with its arguments in a JSON body. Slack ignores a JSON body on its read methods, so the call always got the defaults: one page of 100 public channels, no cursor. A workspace with more channels than that silently hid the rest, the QA channel included. Read methods (conversations.list, .history, .replies) now send their arguments as query parameters, and discovery follows the cursor to the end of the listing, public and private channels alike. The fake workspace used by the unit tests reads query parameters the same way, and a live test against the real workspace now finds the channel. Claude-Session: https://claude.ai/code/session_01HXt8WBUK9MrWKNTYbUKpaT
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
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
WalkthroughThe pull request updates Slack ingress verification, channel discovery pagination, connection write handling, agent reference validation, and channel settings UI integration. It also updates test doubles and acceptance fixtures for the changed contracts. ChangesChannel integrations
Priority: ➖ Normal — Schedule the Channels stack changes because they address first-live-deploy blockers across Slack ingress, channel discovery, connection setup, agent references, and settings UI. Estimated code review effort: 4 (Complex) | ~45 minutes Severity of issue fixed: Low Merge Risk: 🟠 High · up to Editing some channel agents can break their target, empty targets remain accepted, and failed hosted installations can retain orphaned credentials. These should be fixed before merge. Sequence Diagram(s)sequenceDiagram
participant Slack
participant ingest_slack_event
participant _slack_url_verification_challenge
Slack->>ingest_slack_event: Send url_verification payload
ingest_slack_event->>_slack_url_verification_challenge: Parse request body
_slack_url_verification_challenge-->>ingest_slack_event: Return challenge
ingest_slack_event-->>Slack: Return JSON challenge
🚥 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 |
The two differential acceptance tests shared one Slack team id, app id, and bridge source. The connection identity key is unique across projects, so the second test collided with the first one's rows. Each fixture instance now mints its own ids, and the file passes as a whole. Claude-Session: https://claude.ai/code/session_01HXt8WBUK9MrWKNTYbUKpaT
Member
Author
|
Codex (astra, medium, with the simplify skill) reviewed this lane. Verdict: mostly sound, no architecture changes. Four points, all applied in the last commit:
Channels unit tier after the changes: 681 pass. |
- Space discovery follows the listing cursor until Slack returns none; the page cap is gone, since Slack filters after paging and a cap can truncate a real workspace. - The fake Slack workspace reads a read method's arguments from the query string only, as Slack does, so an adapter that sends a JSON body gets the defaults and the test sees it. The paging test asserts cursor progression rather than a call count. - The reference-key test spells out the contract instead of reading it back from the implementation, and a new test proves a failed secret cleanup still surfaces the real conflict. - The name-and-slug helper is named for what it does, and the agent form's field is named for what it holds: a variant id. Claude-Session: https://claude.ai/code/session_01HXt8WBUK9MrWKNTYbUKpaT
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/oss/src/core/channels/service.py (1)
310-313: 🔒 Security & Privacy | 🛡️ Analyzed with Security Review | 🟠 Major | ⚡ Quick winSensitive Data Exposure (CWE-459)
Reachability: External · Exploitability: Moderate
Clean up the new secret on this insert failure path.
When no connection exists,
install_connectionwrites credentials to the vault beforecreate_connection. If the insert raisesIntegrityError, the handler raises the conflict but leaves the new secret stored without a connection record. Delete only that newly created secret before raising the conflict.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Advanced
Run ID: 3f440bb9-c610-48fb-ba21-0aedf4e1edc7
⛔ Files ignored due to path filters (1)
web/packages/agenta-api-client/src/generated/api/types/UpdateSecretPayloadDto.tsis excluded by!**/generated/**
📒 Files selected for processing (21)
api/oss/src/apis/fastapi/channels/ingress.pyapi/oss/src/core/channels/adapters/slack/adapter.pyapi/oss/src/core/channels/dtos.pyapi/oss/src/core/channels/service.pyapi/oss/tests/pytest/acceptance/channels/differential/test_slack_in_process_vs_bridged.pyapi/oss/tests/pytest/acceptance/channels/test_slack_adapter_live.pyapi/oss/tests/pytest/integration/channels/test_channels_ingress_seam.pyapi/oss/tests/pytest/integration/channels/test_channels_ingress_slack_seam.pyapi/oss/tests/pytest/integration/channels/test_channels_slack_deactivation_seam.pyapi/oss/tests/pytest/unit/channels/slack/fake_slack.pyapi/oss/tests/pytest/unit/channels/slack/test_slack_adapter.pyapi/oss/tests/pytest/unit/channels/slack/test_slack_over_fake.pyapi/oss/tests/pytest/unit/channels/test_channels_agent_references.pyapi/oss/tests/pytest/unit/channels/test_channels_connection_write_path.pyapi/oss/tests/pytest/unit/channels/test_channels_ingress.pyweb/oss/src/components/pages/AgentaChannelSurface/components/ConversationPanel.tsxweb/oss/src/components/pages/settings/Channels/components/AgentFormDrawer.tsxweb/oss/src/components/pages/settings/Channels/components/ConnectionsSection.tsxweb/oss/src/components/pages/settings/Channels/components/InboxEventsSection.tsxweb/oss/src/components/pages/settings/Channels/components/OutboxEventsSection.tsxweb/packages/agenta-settings/src/sidebar.tsx
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.
- An agent's reference map must name something: an empty map is refused at write time, since such an agent would fail on its first turn. The placeholder fixtures name a revision now. - The settings form keeps the reference family an existing agent was saved with when it edits the agent; only a new agent is filed as a variant. Claude-Session: https://claude.ai/code/session_01HXt8WBUK9MrWKNTYbUKpaT
…O now requires Claude-Session: https://claude.ai/code/session_01HXt8WBUK9MrWKNTYbUKpaT
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Stacked on #6644. These are the defects that stopped a fresh channels stack from answering a Slack message at all, found on the first deploy of the takeover branch. Each one is small and has a unit test that fails without it.
Changes
url_verificationchallenge, unsigned and shape-strict. Everything else on the route stays signed.workflow_variant), and the API refuses a reference key outside the resolvable families at write time.Tests
https://claude.ai/code/session_01HXt8WBUK9MrWKNTYbUKpaT