Feature/channel integrations#426
Merged
Merged
Conversation
…+ REST API Standalone config resource replacing ChannelConnector on agents. - ChannelIntegrationConfiguration with multi-target support - ChannelTarget with trigger keywords, AGENT/GROUP type, observe mode schema - ObserveConfig with dollar-based cost ceilings (schema-ready, impl deferred) - IChannelIntegrationStore + MongoChannelIntegrationStore (AbstractResourceStore) - IRestChannelIntegrationStore + RestChannelIntegrationStore (admin-only) - Validation: trigger uniqueness, channel type, default target, target completeness - Resource URI: eddi://ai.labs.channel/channelstore/channels/
…atching Platform-agnostic router replacing SlackChannelRouter: - Colon-required trigger syntax (architect: question) - Thread-target locking (prevents mid-thread target switching) - New-style config resolution from ChannelIntegrationConfiguration - Legacy ChannelConnector fallback (strict: new config wins per channelId) - 23 unit tests covering matching, aliases, help, edge cases, thread locking
Covers group discussion and regular conversation cancel/stop. Designed with HITL extension points (Phase 9b) built in. Includes DiscussionControlToken, safe-point analysis, cascading abort, and API design for POST .../cancel endpoints.
- SlackEventHandler now uses ChannelTargetRouter for all routing - Removed GROUP_PREFIX pattern (group: magic prefix), replaced by config triggers - Added target-based routing: AGENT and GROUP types routed via switch - Thread target locking: first message locks target for entire thread - postHelp() lists available targets with trigger keywords - postMessage() resolves bot token from ResolvedTarget or router fallback - getOrCreateConversation() uses integration-aware intent keys - RestSlackWebhook uses ChannelTargetRouter.getSigningSecrets() - SlackChannelRouter.java preserved (not deleted yet, tests reference it)
…tegrations New MCP tools (admin-only): - list_channel_integrations: list all channel integration descriptors - read_channel_integration: read full config by ID - create_channel_integration: create with validation - update_channel_integration: update existing config - delete_channel_integration: soft or permanent delete - migrate_channel_connectors: scan legacy ChannelConnectors on deployed agents and convert to standalone ChannelIntegrationConfigurations (dry-run by default, non-destructive)
- BUG 1: Fix compilation error — 4-arg getOrCreateConversation call in tryHandleAgentFollowUp; compose intent key from channelId + parentTs - BUG 2: Fix null strippedMessage in thread replies — resolveThreadTarget returned null strippedMessage which was passed to sendAndWait; add originalText fallback in handleAgentConversation and handleGroupDiscussion - BUG 3: Fix legacy group connectors misrouted as AGENT — LegacyTarget.toChannelTarget() now checks groupId and sets GROUP type - BUG 4: Fix unbounded memory leak in threadTargetLock — replaced ConcurrentHashMap with ICache (24h TTL) via ICacheFactory injection - Minor: null-safety in postHelp, demote refresh log to debug, update test mock to use proper MapCache + Mockito pattern
Review fix #1: Delete dead SlackChannelRouter + SlackChannelRouterTest - Class was @ApplicationScoped but never injected, causing double startup cost scanning all agents for channel connectors - Both files removed (265 + 351 LOC) Review fix #2: Migration tool now merges duplicate channelId entries - Old: created one config per (agent, channel) pair, last-write-wins - New: groups connectors by platformChannelId and creates a single multi-target config per channel with agent-derived trigger keywords - Multi-agent channels show mergedAgents in dry-run output Review fix #3: Deep-copy config before resolving secrets - resolvePlatformSecrets was mutating the store's instance in-place, which would leak plaintext secrets if a caching layer is added - Added deepCopyConfig() — router works on copies, REST returns vault references Review fix #5: Null/blank trigger guard in validation - Null triggers from loose JSON input now return 400 instead of NPE Review fix #6: Remove dead fields - Removed newStyleChannelIds (assigned but never read) - Removed cacheFactory field (only used in constructor) - Removed unused ConcurrentHashMap import Review fix #7/#12: Reject observeMode=true until implemented - Validation now blocks observeMode=true with clear error message Review fix #8: Preserve stack traces in router error logging - All LOGGER.warnf(msg, e.getMessage()) changed to LOGGER.warn(msg, e) for production diagnosability Review fix #10: Rename 'channelId' to 'resourceId' in MCP responses - Eliminates confusion between Slack channelId and Mongo resourceId Review fix #11: Fix deployAgent description typo - 'production' was listed twice in 4 environment descriptions Review fix #17: Temper platform-agnostic Javadoc claim - Javadoc now says 'currently Slack-only with platform-agnostic model'
N1: Restore per-agent error reporting (was silently swallowed)
N2: Detect credential conflicts — when multiple agents share a
channelId but have different botToken/signingSecret, skip with
action='credential_conflict' and actionable hint
N3: Deduplicate target names — agents with identical names get
suffixed with short agentId to avoid BadRequestException on
duplicate triggers
N4: Group key now includes channelType (channelType:channelId) to
prevent cross-platform collisions
N5: Sort entries by agentId before constructing targets for
deterministic defaultTargetName across JVM runs
N6: Replace brittle Map<String,Object> with typed MigrationEntry
record — eliminates unsafe casts
N7: Add shared-reference invariant comment to deepCopyConfig Javadoc
New tests: - RestChannelIntegrationStoreValidationTest (17 tests) Covers: name, channelType, targets, defaultTarget, trigger uniqueness, null/blank triggers, observeMode rejection - ChannelTargetRouterTest: 2 new edge case tests - 'help:' with colon is NOT a help signal (#4 from review) - 'architect:' with empty remainder matches trigger correctly Migration polish: - N3 counter fallback: dedup loop handles triple+ name collisions (extremely unlikely but now provably correct) - N2 comment: documents extending credential key list when Teams/Discord adapters arrive Validation visibility: - validateConfiguration() changed from private to package-private for direct unit testing Total channel integration tests: 42 (was 23)
…recate legacy channels - Remove migrate_channel_connectors MCP tool from McpAdminTools - Add ChannelConnectorMigration: startup one-shot migration following V6RenameMigration pattern (flag-based, idempotent, retry-safe) - Wire into AgentDeploymentManagement.autoDeployAgents() startup sequence - Deprecate ChannelConnector class and channels field in AgentConfiguration with @deprecated(since=6.1.0, forRemoval=true) Migration runs once at startup, sets flag in migrationlog collection. Legacy channel configs are auto-migrated to standalone ChannelIntegrationConfiguration documents.
…olved copy
R1: ChannelConnectorMigration called readAgent() which only exists on
IRestAgentStore. IAgentStore (via IResourceStore) uses read().
Caught by clean compile after incremental build masked the error.
R2: ChannelTargetRouter collected signing secrets from the original
config (with vault references like \) instead of
the deep-copied config with resolved secrets. HMAC verification
requires the actual secret value.
… fallback New test class: ChannelTargetRouterRefreshTest (31 tests) - resolveTarget with new-style integration (5 tests) Covers: trigger match via public API, default target, unknown channel, bot token vault resolution, signing secret vault resolution - Legacy fallback (3 tests) Covers: legacy agent routing, group type routing, new-style suppresses legacy - Signing secrets (3 tests) Covers: resolved from new-style, includes legacy, empty for non-slack - Channel detection (5 tests) Covers: hasAnyChannels (new-style, legacy, cross-type), getIntegration - Deep copy safety (2 tests) Covers: store original unchanged, returned config has resolved secrets - Refresh mechanism (5 tests) Covers: first-call load, no re-refresh within interval, store exception, null channelId, null config - Secret resolution (2 tests) Covers: absent secrets, resolver failure - ResolvedTarget accessors (4 tests) Covers: legacy fallback, integration preference, both null - LegacyTarget conversion (2 tests) Covers: with/without groupId Total channel integration tests: 73 (was 42)
…lId, reserved triggers, NPE guard, migration names - Bug #1: postMessage now uses getBotToken() which checks both integrationMap and legacyMap, fixing silent follow-up failure in legacy-only channels - Bug #3: REST validation rejects duplicate channelType:channelId across configs, preventing silent overwrites in the router's integrationMap - Bug #4: Reject reserved 'help' keyword as trigger (would never fire due to router short-circuit) - Bug #5: Null guard on trigger.toLowerCase() in resolveFromIntegration - Bug #2: Migration warns on credential divergence across agents sharing a channel - Bug #10: Migration uses agent descriptor name (slugified) for target names instead of raw ObjectId slugs, making triggers human-typeable - 7 new tests: 3 reserved trigger rejection, 4 getBotToken (80 total)
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a platform-agnostic Channel Integration subsystem: new channel models, store interfaces and REST/MCP APIs, a Mongo store and REST implementation, a runtime ChannelTargetRouter (replacing SlackChannelRouter), a one-shot migration from embedded connectors, Slack wiring updates, deprecations for legacy connector fields, many tests, and infra/testtime tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant SlackAPI as Slack API
participant RestSlackWebhook as RestSlackWebhook
participant ChannelTargetRouter as ChannelTargetRouter
participant ChannelIntegrationStore as ChannelIntegrationStore
participant SecretResolver as SecretResolver
SlackAPI->>RestSlackWebhook: POST event (channel_id, thread_ts, text)
RestSlackWebhook->>ChannelTargetRouter: resolveThreadTarget("slack", channel_id, thread_ts)
ChannelTargetRouter->>ChannelIntegrationStore: read/cache integrations (refresh if needed)
ChannelIntegrationStore-->>ChannelTargetRouter: ChannelIntegrationConfiguration(s)
ChannelTargetRouter->>SecretResolver: resolve platform secrets (botToken, signingSecret)
SecretResolver-->>ChannelTargetRouter: resolved secrets
ChannelTargetRouter-->>RestSlackWebhook: ResolvedTarget (target, strippedMessage, credentials)
RestSlackWebhook->>SlackAPI: postMessage / acknowledge (using resolved bot token)
sequenceDiagram
participant Startup as Startup
participant AgentDeploymentManagement as AgentDeploymentManagement
participant ChannelConnectorMigration as ChannelConnectorMigration
participant AgentStore as AgentStore
participant ChannelIntegrationStore as ChannelIntegrationStore
participant MigrationLogStore as MigrationLogStore
Startup->>AgentDeploymentManagement: autoDeployAgents()
AgentDeploymentManagement->>ChannelConnectorMigration: runIfNeeded()
ChannelConnectorMigration->>MigrationLogStore: check "channel-connector-migration-complete"
alt flag present
MigrationLogStore-->>ChannelConnectorMigration: skip
else
ChannelConnectorMigration->>AgentStore: read deployed deployments
loop for each channelType:channelId group
ChannelConnectorMigration->>ChannelIntegrationStore: create ChannelIntegrationConfiguration
ChannelIntegrationStore-->>ChannelConnectorMigration: created / error
end
ChannelConnectorMigration->>MigrationLogStore: write completion flag (if all succeeded)
end
ChannelConnectorMigration-->>AgentDeploymentManagement: return
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
There was a problem hiding this comment.
Pull request overview
This PR refactors EDDI’s channel integration subsystem from a legacy, agent-embedded Slack connector into a standalone, multi-target “channel integration” architecture with routing, migrations, admin tooling, and expanded test coverage.
Changes:
- Introduces
ChannelIntegrationConfiguration+ChannelTargetmodels and a newChannelTargetRouter(trigger-based routing + thread target locking + secret aggregation + legacy fallback). - Refactors Slack webhook + event handling to route via
ChannelTargetRouterand adds help/trigger behavior. - Adds startup migration (
ChannelConnectorMigration), new REST/MCP admin surfaces for channel integrations, and substantial unit test coverage.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/ai/labs/eddi/integrations/slack/SlackChannelRouterTest.java | Removes legacy router tests. |
| src/test/java/ai/labs/eddi/integrations/channels/ChannelTargetRouterTest.java | Adds unit tests for trigger matching + thread locking behavior. |
| src/test/java/ai/labs/eddi/integrations/channels/ChannelTargetRouterRefreshTest.java | Adds refresh/secret/legacy-fallback coverage for the new router. |
| src/test/java/ai/labs/eddi/configs/channels/rest/RestChannelIntegrationStoreValidationTest.java | Adds validation tests for channel integration CRUD validation rules. |
| src/main/java/ai/labs/eddi/integrations/slack/rest/RestSlackWebhook.java | Updates Slack signature secret lookup to use ChannelTargetRouter. |
| src/main/java/ai/labs/eddi/integrations/slack/SlackEventHandler.java | Refactors Slack routing, thread locking, help posting, and token resolution to use new router. |
| src/main/java/ai/labs/eddi/integrations/slack/SlackChannelRouter.java | Removes legacy Slack router implementation. |
| src/main/java/ai/labs/eddi/integrations/channels/ChannelTargetRouter.java | Adds new platform-agnostic router with new-style + legacy fallback. |
| src/main/java/ai/labs/eddi/engine/runtime/internal/AgentDeploymentManagement.java | Wires new startup migration into deployment startup flow. |
| src/main/java/ai/labs/eddi/engine/mcp/McpAdminTools.java | Adds MCP admin tools for listing/reading/creating/updating/deleting channel integrations (and doc tweaks). |
| src/main/java/ai/labs/eddi/configs/migration/ChannelConnectorMigration.java | Adds one-shot startup migration from legacy connectors to standalone channel integrations. |
| src/main/java/ai/labs/eddi/configs/channels/rest/RestChannelIntegrationStore.java | Adds REST CRUD implementation and validation for channel integrations. |
| src/main/java/ai/labs/eddi/configs/channels/mongo/ChannelIntegrationStore.java | Adds new resource store for channel integrations. |
| src/main/java/ai/labs/eddi/configs/channels/model/ObserveConfig.java | Adds schema for future “observe mode”. |
| src/main/java/ai/labs/eddi/configs/channels/model/ChannelTarget.java | Adds model for addressable targets (agent/group) with triggers. |
| src/main/java/ai/labs/eddi/configs/channels/model/ChannelIntegrationConfiguration.java | Adds top-level standalone channel integration model. |
| src/main/java/ai/labs/eddi/configs/channels/IRestChannelIntegrationStore.java | Adds JAX-RS interface for channel integration CRUD. |
| src/main/java/ai/labs/eddi/configs/channels/IChannelIntegrationStore.java | Adds store interface for channel integration configs. |
| src/main/java/ai/labs/eddi/configs/agents/model/AgentConfiguration.java | Deprecates legacy channels/ChannelConnector fields. |
| planning/conversation-cancel-plan.md | Adds future design plan doc (not executed in this PR). |
| docs/changelog.md | Documents the integration refactor, migrations, and test expansion. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/ai/labs/eddi/integrations/slack/SlackEventHandler.java (1)
385-406:⚠️ Potential issue | 🟠 MajorReturn the winning conversation after a create race.
If two events for the same
intent/user arrive concurrently, both can start a conversation; the loser catchesResourceAlreadyExistsExceptionbut returns its newly created, unmapped conversation ID. Re-read and return the persisted mapping instead.🔒 Proposed race fix
try { userConversationStore.createUserConversation(mapping); } catch (IResourceStore.ResourceAlreadyExistsException e) { LOGGER.debugf("Race condition: conversation mapping already exists for %s/%s", intent, slackUserId); + UserConversation raced = userConversationStore.readUserConversation(intent, slackUserId); + if (raced != null) { + return raced.getConversationId(); + } + throw e; } return result.conversationId();Based on learnings, Thread Safety — The
ConversationCoordinatorhandles concurrency between conversations. Code must be thread-safe and non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ai/labs/eddi/integrations/slack/SlackEventHandler.java` around lines 385 - 406, The current flow in SlackEventHandler starts a conversation and on createUserConversation ResourceAlreadyExistsException returns the caller's newly created conversationId (loser) instead of the persisted mapping; change the catch for IResourceStore.ResourceAlreadyExistsException in the block that creates the UserConversation to re-read the mapping via userConversationStore.readUserConversation(intent, slackUserId) and return the persisted existing.getConversationId() (falling back to the local result.conversationId() only if the re-read unexpectedly returns null); reference SlackEventHandler, userConversationStore.readUserConversation, conversationService.startConversation, userConversationStore.createUserConversation, IResourceStore.ResourceAlreadyExistsException, and UserConversation when making the change.
🧹 Nitpick comments (4)
docs/changelog.md (1)
16-74: Normalize these two entries to the same changelog schema used elsewhere.Please add explicit
What changed,Design decisions, andIn Progresssubsections for these two 2026-04-19 entries so scanning stays consistent across the file.Based on learnings: “Update
docs/changelog.md… Include: date, title, repo/branch, what changed, design decisions, and what's in progress.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/changelog.md` around lines 16 - 74, Normalize the two 2026-04-19 entries ("Channel Integration — External Review Round 4 (2026-04-19)" and "Channel Integration — Review Hardening & Test Coverage (2026-04-19)") to match the file's changelog schema: replace the current freeform bullet groups with explicit subsections titled "What changed", "Design decisions", and "In Progress" for each entry, moving the existing bullets into the appropriate subsection (e.g., bugs/fixes → What changed, rationale/implementation choices → Design decisions, unfinished tasks/tests → In Progress), and ensure each entry includes repo/branch and date in the header as already present.src/test/java/ai/labs/eddi/integrations/channels/ChannelTargetRouterTest.java (1)
381-423: Assert TTL usage in the thread-lock cache test helper.
MapCacheignores thelifespanarguments, so these tests would still pass ifChannelTargetRouteraccidentally stored thread locks without TTL. Capture the lifespan/unit in the helper or verify the TTL overload is invoked.🧪 Example test-helper direction
private static class MapCache<K, V> extends ConcurrentHashMap<K, V> implements ICache<K, V> { + long lastLifespan; + TimeUnit lastLifespanUnit; @@ public V put(K key, V value, long lifespan, TimeUnit unit) { + this.lastLifespan = lifespan; + this.lastLifespanUnit = unit; return put(key, value); }Then assert the router uses the expected TTL path in
lockedTargetReturned().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/ai/labs/eddi/integrations/channels/ChannelTargetRouterTest.java` around lines 381 - 423, MapCache (the test helper) currently ignores lifespan/unit parameters so TTL usage isn't verified; modify the MapCache class to record the lifespan and TimeUnit passed into the TTL-overloaded methods (e.g., put(K,V,long,TimeUnit), putIfAbsent(K,V,long,TimeUnit), putAll(Map<? extends K,? extends V>,long,TimeUnit), replace(K,V,long,TimeUnit), and their maxIdle overloads) by adding fields like lastLifespan and lastLifespanUnit (and reset/clear as needed) and use those fields in tests (for example in lockedTargetReturned()) to assert that the router invoked the TTL overload with the expected values (or alternatively make the helper throw if TTL parameters are missing/zero), ensuring the TTL path is actually exercised.src/main/java/ai/labs/eddi/engine/mcp/McpAdminTools.java (1)
1172-1303: New channel-integration tools look correct and consistent with the file's patterns.Nit — all five new tools reference
ai.labs.eddi.configs.channels.IRestChannelIntegrationStoreandai.labs.eddi.configs.channels.model.ChannelIntegrationConfigurationvia fully-qualified names. Other tools in this file import their REST interfaces and model classes. Adding the two imports would be consistent with the rest of the file:✏️ Proposed fix
+import ai.labs.eddi.configs.channels.IRestChannelIntegrationStore; +import ai.labs.eddi.configs.channels.model.ChannelIntegrationConfiguration;Then drop the
ai.labs.eddi...prefixes at lines 1184, 1203, 1228, 1230, 1261, 1263, 1291.Otherwise: role check, arg validation,
Location-header parsing, and error logging all mirror the existingcreateResource/updateResource/deleteResourceshape — LGTM.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ai/labs/eddi/engine/mcp/McpAdminTools.java` around lines 1172 - 1303, Add imports for ai.labs.eddi.configs.channels.IRestChannelIntegrationStore and ai.labs.eddi.configs.channels.model.ChannelIntegrationConfiguration and replace the fully-qualified class references in the new tool methods (listChannelIntegrations, readChannelIntegration, createChannelIntegration, updateChannelIntegration, deleteChannelIntegration) with the simple class names; update the getRestStore(...) and jsonSerialization.deserialize(...) calls to use IRestChannelIntegrationStore and ChannelIntegrationConfiguration respectively (remove the ai.labs.eddi... prefixes) so the file matches the existing import style.src/main/java/ai/labs/eddi/configs/channels/rest/RestChannelIntegrationStore.java (1)
216-260: Uniqueness scan reads up to 1000 descriptors on every create/update.
validateUniqueChannelIdperforms a full descriptor scan (up to 1000) and achannelStore.readper descriptor on every save. With low channel-integration counts this is fine, but it scales linearly with the number of configs and adds latency + DB load to every admin save. Consider either (a) adding a dedicatedchannelStore.findByChannelTypeAndChannelId(...)that pushes the predicate into MongoDB, or (b) maintaining a unique index on(channelType, platformConfig.channelId)in the Mongo collection so the database rejects the conflict at write time and this scan becomes redundant. Option (b) also closes the TOCTOU window between the scan and the subsequentrestVersionInfo.create/updatecall.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ai/labs/eddi/configs/channels/rest/RestChannelIntegrationStore.java` around lines 216 - 260, validateUniqueChannelId currently scans descriptors and calls channelStore.read for each entry which doesn't scale; replace this expensive scan with a database-level lookup or unique constraint. Option A: add a channelStore.findByChannelTypeAndChannelId(channelType, channelId) (with an overload to exclude an id) and call it from validateUniqueChannelId in RestChannelIntegrationStore to return an existing config quickly and throw BadRequestException if found. Option B (preferred): add a unique index on (channelType, platformConfig.channelId) in the Mongo collection and remove the descriptor scan; then handle the DB duplicate-key exception during save/update and translate it to a BadRequestException (preserving the excludeId logic if needed). Update channelStore implementation and RestChannelIntegrationStore to use the new lookup or catch duplicate-key errors accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@planning/conversation-cancel-plan.md`:
- Around line 35-44: The API `mode` strings in request examples (e.g.,
"GRACEFUL"/"IMMEDIATE") don't match the ControlSignal enum values
(CANCEL_GRACEFUL/CANCEL_IMMEDIATE), causing binding errors; fix by normalizing
one representation: either rename the enum members to GRACEFUL and IMMEDIATE, or
update the API docs/examples to use CANCEL_GRACEFUL and CANCEL_IMMEDIATE, or add
an explicit mapping function (e.g., mapModeToControlSignal) that converts
incoming "GRACEFUL"/"IMMEDIATE"/"CONTINUE" to ControlSignal.CONTINUE /
ControlSignal.CANCEL_GRACEFUL / ControlSignal.CANCEL_IMMEDIATE — update all
usages and the examples accordingly (refer to the ControlSignal enum and any
request parsing code/examples).
- Line 93: Replace unlabeled fenced code blocks in the markdown with
language-labeled fences so markdownlint MD040 is satisfied: change occurrences
of ``` to language-specific fences (e.g., ```text for plain examples like the
executeDiscussion() snippet, or ```java/```bash where the block contains code in
those languages). Update every unlabeled fence (including the blocks flagged
around the conversation cancel plan examples) to use the appropriate language
identifier.
- Around line 199-205: The document contains two inconsistent cancel endpoint
signatures (POST /groups/{groupId}/conversations/{groupConversationId}/cancel
vs. POST /{convId}/cancel) which risks contract drift; pick the canonical path
(e.g., POST /groups/{groupId}/conversations/{groupConversationId}/cancel), then
update every occurrence (including the Phase A section and the occurrences
around lines referenced as 319-320) so all examples, prose, and request/JSON
examples use that single endpoint string; ensure any parameter names (groupId,
groupConversationId or convId) are normalized throughout the doc and update
related example payloads and references accordingly.
In
`@src/main/java/ai/labs/eddi/configs/channels/model/ChannelIntegrationConfiguration.java`:
- Around line 78-91: The setters setPlatformConfig(Map<String, String>) and
setTargets(List<ChannelTarget>) can accept null and wipe out the
constructor-initialized defaults; update each setter to normalize nulls by
assigning an empty Map or empty List when the input is null (e.g.,
Collections.emptyMap()/Collections.emptyList() or new HashMap/ArrayList) so
downstream code always sees non-null collections; keep the existing field names
platformConfig and targets and preserve mutability semantics for consumers.
In
`@src/main/java/ai/labs/eddi/configs/channels/rest/RestChannelIntegrationStore.java`:
- Around line 130-204: The validation uses channelType.toLowerCase() which is
locale-sensitive; change the lowercasing to use Locale.ROOT to match how
triggers are normalized and avoid Turkish-I bugs. In validateConfiguration
replace channelType.toLowerCase() with channelType.toLowerCase(Locale.ROOT)
where REGISTERED_CHANNEL_TYPES is checked so the comparison is deterministic
(refer to symbols: validateConfiguration, channelType,
REGISTERED_CHANNEL_TYPES).
- Around line 268-299: syncDescriptor currently assumes
documentDescriptorStore.readDescriptor(...) never returns null and then calls
descriptor.getName(), causing an NPE; after reading the descriptor add a
null-check and handle it: if descriptor == null, instantiate a new
DocumentDescriptor (or equivalent DTO used by documentDescriptorStore), set its
name/description from config as needed, mark changed = true, and then call
documentDescriptorStore.setDescriptor(resourceId,
currentResourceId.getVersion(), descriptor); otherwise proceed with the existing
comparison logic — update only when values differ. Ensure you reference
syncDescriptor, documentDescriptorStore.readDescriptor,
descriptor.getName()/getDescription(), and documentDescriptorStore.setDescriptor
when making the change.
In `@src/main/java/ai/labs/eddi/configs/migration/ChannelConnectorMigration.java`:
- Around line 136-175: The migration writes configs directly via
channelStore.create in ChannelConnectorMigration, bypassing
RestChannelIntegrationStore.validateConfiguration and allowing reserved triggers
and leaked per-connector fields into platformConfig; fix by (1) filtering
platform config copied from first.connector().getConfig() to only allow
channel-level keys (e.g., channelId, botToken, signingSecret) before calling
config.setPlatformConfig, and (2) ensure generated target names from
slugify(...) used for ChannelTarget.setName and ChannelTarget.setTriggers are
validated/normalized against reserved triggers (as used by
ChannelTargetRouter.resolveFromIntegration) — if a slugified name is reserved,
append or replace it (use a deterministic suffix) until the name is valid;
finally call the same validation used by
RestChannelIntegrationStore.validateConfiguration (or invoke that validator)
before channelStore.create to prevent invalid configs from being written.
- Around line 84-121: migrateConnectors builds channelGroups and currently adds
a ConnectorEntry for each deployment result, causing duplicates when the same
agent appears in multiple deployments; change the population logic for
channelGroups (where you computeIfAbsent(groupKey, ...) and add new
ConnectorEntry) to first deduplicate by the tuple (groupKey, agentId) — e.g.,
maintain a Set<String> seenKeys or check the existing List<ConnectorEntry> for
an entry with the same agentId before adding — so that for a given groupKey you
only add one ConnectorEntry per agentId and avoid duplicate ChannelTarget
creation; keep references to migrateConnectors, channelGroups, ConnectorEntry,
groupKey, agentId, and readDeploymentInfos when locating the code to modify.
In `@src/main/java/ai/labs/eddi/integrations/channels/ChannelTargetRouter.java`:
- Around line 109-216: The lookup keys are inconsistent: stored keys are
lowercased but resolveTarget, getIntegration, getBotToken and hasAnyChannels
build keys from the caller-supplied channelType verbatim, and existing
toLowerCase calls lack Locale.ROOT; fix by normalizing all channelType usages to
channelType.toLowerCase(Locale.ROOT) when building lookup keys in resolveTarget
(key creation), getIntegration, getBotToken, and hasAnyChannels, and change the
existing copy.getChannelType().toLowerCase() calls in refresh/refresh helpers to
copy.getChannelType().toLowerCase(Locale.ROOT); also add import
java.util.Locale; so string normalization is consistent and locale-robust.
In `@src/main/java/ai/labs/eddi/integrations/slack/rest/RestSlackWebhook.java`:
- Around line 90-92: The current blocking refresh happens when RestSlackWebhook
calls ChannelTargetRouter.getSigningSecrets("slack") which triggers
refreshIfNeeded() -> refreshInternal() and performs
descriptorStore.readDescriptors() and channelStore.read() on the request thread;
refactor so getSigningSecrets only returns the volatile cached secrets and never
calls refreshInternal synchronously. Move the refresh logic out to a background
scheduler (e.g., Quarkus `@Scheduled` or a ScheduledExecutorService) that
periodically runs refreshInternal() (and run an initial refresh at startup),
keep refreshIfNeeded as a cheap non-blocking check or remove it, and ensure
getSigningSecrets only reads the cached field so RestSlackWebhook and other REST
endpoints never block on datastore reads.
In `@src/main/java/ai/labs/eddi/integrations/slack/SlackEventHandler.java`:
- Around line 213-216: The thread-lock currently uses only threadTs causing
cross-channel collisions; update ChannelTargetRouter.lockThreadTarget to accept
platform and channel identifiers (e.g., lockThreadTarget(String platform, String
channelId, String threadTs, ChannelTarget target)) and build the cache key as
"{platform}:{channelId}:{threadTs}". Change any related methods
(unlock/get/resolver methods) to use the same composite key and update callers
such as SlackEventHandler.resolveThreadTarget / the call site that currently
does channelTargetRouter.lockThreadTarget(threadTs, resolved.target()) to pass
the platform and channel id (or channelType/platformChannelId) so the lock is
scoped per platform+channel+thread.
In
`@src/test/java/ai/labs/eddi/configs/channels/rest/RestChannelIntegrationStoreValidationTest.java`:
- Around line 138-150: The test targetNullName currently fails due to the
default-target mismatch before the per-target name validation; modify the test
so the default-target check passes by adding a second ChannelTarget with name
"x" (and appropriate targetId/triggers) to config.setTargets(...) before calling
store.validateConfiguration(config), keeping config.setDefaultTargetName("x")
unchanged so the null-name branch in validateConfiguration(...) is actually
exercised and triggers the "Every target must have a name." BadRequestException.
---
Outside diff comments:
In `@src/main/java/ai/labs/eddi/integrations/slack/SlackEventHandler.java`:
- Around line 385-406: The current flow in SlackEventHandler starts a
conversation and on createUserConversation ResourceAlreadyExistsException
returns the caller's newly created conversationId (loser) instead of the
persisted mapping; change the catch for
IResourceStore.ResourceAlreadyExistsException in the block that creates the
UserConversation to re-read the mapping via
userConversationStore.readUserConversation(intent, slackUserId) and return the
persisted existing.getConversationId() (falling back to the local
result.conversationId() only if the re-read unexpectedly returns null);
reference SlackEventHandler, userConversationStore.readUserConversation,
conversationService.startConversation,
userConversationStore.createUserConversation,
IResourceStore.ResourceAlreadyExistsException, and UserConversation when making
the change.
---
Nitpick comments:
In `@docs/changelog.md`:
- Around line 16-74: Normalize the two 2026-04-19 entries ("Channel Integration
— External Review Round 4 (2026-04-19)" and "Channel Integration — Review
Hardening & Test Coverage (2026-04-19)") to match the file's changelog schema:
replace the current freeform bullet groups with explicit subsections titled
"What changed", "Design decisions", and "In Progress" for each entry, moving the
existing bullets into the appropriate subsection (e.g., bugs/fixes → What
changed, rationale/implementation choices → Design decisions, unfinished
tasks/tests → In Progress), and ensure each entry includes repo/branch and date
in the header as already present.
In
`@src/main/java/ai/labs/eddi/configs/channels/rest/RestChannelIntegrationStore.java`:
- Around line 216-260: validateUniqueChannelId currently scans descriptors and
calls channelStore.read for each entry which doesn't scale; replace this
expensive scan with a database-level lookup or unique constraint. Option A: add
a channelStore.findByChannelTypeAndChannelId(channelType, channelId) (with an
overload to exclude an id) and call it from validateUniqueChannelId in
RestChannelIntegrationStore to return an existing config quickly and throw
BadRequestException if found. Option B (preferred): add a unique index on
(channelType, platformConfig.channelId) in the Mongo collection and remove the
descriptor scan; then handle the DB duplicate-key exception during save/update
and translate it to a BadRequestException (preserving the excludeId logic if
needed). Update channelStore implementation and RestChannelIntegrationStore to
use the new lookup or catch duplicate-key errors accordingly.
In `@src/main/java/ai/labs/eddi/engine/mcp/McpAdminTools.java`:
- Around line 1172-1303: Add imports for
ai.labs.eddi.configs.channels.IRestChannelIntegrationStore and
ai.labs.eddi.configs.channels.model.ChannelIntegrationConfiguration and replace
the fully-qualified class references in the new tool methods
(listChannelIntegrations, readChannelIntegration, createChannelIntegration,
updateChannelIntegration, deleteChannelIntegration) with the simple class names;
update the getRestStore(...) and jsonSerialization.deserialize(...) calls to use
IRestChannelIntegrationStore and ChannelIntegrationConfiguration respectively
(remove the ai.labs.eddi... prefixes) so the file matches the existing import
style.
In
`@src/test/java/ai/labs/eddi/integrations/channels/ChannelTargetRouterTest.java`:
- Around line 381-423: MapCache (the test helper) currently ignores
lifespan/unit parameters so TTL usage isn't verified; modify the MapCache class
to record the lifespan and TimeUnit passed into the TTL-overloaded methods
(e.g., put(K,V,long,TimeUnit), putIfAbsent(K,V,long,TimeUnit), putAll(Map<?
extends K,? extends V>,long,TimeUnit), replace(K,V,long,TimeUnit), and their
maxIdle overloads) by adding fields like lastLifespan and lastLifespanUnit (and
reset/clear as needed) and use those fields in tests (for example in
lockedTargetReturned()) to assert that the router invoked the TTL overload with
the expected values (or alternatively make the helper throw if TTL parameters
are missing/zero), ensuring the TTL path is actually exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9180c1fe-7b5b-4c8f-9567-da6d2da0d94f
📒 Files selected for processing (21)
docs/changelog.mdplanning/conversation-cancel-plan.mdsrc/main/java/ai/labs/eddi/configs/agents/model/AgentConfiguration.javasrc/main/java/ai/labs/eddi/configs/channels/IChannelIntegrationStore.javasrc/main/java/ai/labs/eddi/configs/channels/IRestChannelIntegrationStore.javasrc/main/java/ai/labs/eddi/configs/channels/model/ChannelIntegrationConfiguration.javasrc/main/java/ai/labs/eddi/configs/channels/model/ChannelTarget.javasrc/main/java/ai/labs/eddi/configs/channels/model/ObserveConfig.javasrc/main/java/ai/labs/eddi/configs/channels/mongo/ChannelIntegrationStore.javasrc/main/java/ai/labs/eddi/configs/channels/rest/RestChannelIntegrationStore.javasrc/main/java/ai/labs/eddi/configs/migration/ChannelConnectorMigration.javasrc/main/java/ai/labs/eddi/engine/mcp/McpAdminTools.javasrc/main/java/ai/labs/eddi/engine/runtime/internal/AgentDeploymentManagement.javasrc/main/java/ai/labs/eddi/integrations/channels/ChannelTargetRouter.javasrc/main/java/ai/labs/eddi/integrations/slack/SlackChannelRouter.javasrc/main/java/ai/labs/eddi/integrations/slack/SlackEventHandler.javasrc/main/java/ai/labs/eddi/integrations/slack/rest/RestSlackWebhook.javasrc/test/java/ai/labs/eddi/configs/channels/rest/RestChannelIntegrationStoreValidationTest.javasrc/test/java/ai/labs/eddi/integrations/channels/ChannelTargetRouterRefreshTest.javasrc/test/java/ai/labs/eddi/integrations/channels/ChannelTargetRouterTest.javasrc/test/java/ai/labs/eddi/integrations/slack/SlackChannelRouterTest.java
💤 Files with no reviewable changes (2)
- src/test/java/ai/labs/eddi/integrations/slack/SlackChannelRouterTest.java
- src/main/java/ai/labs/eddi/integrations/slack/SlackChannelRouter.java
…ubsystem - ChannelModelTest: POJO tests for ChannelIntegrationConfiguration, ChannelTarget, ObserveConfig (all 100% instruction coverage) - ChannelConnectorMigrationTest: full migration logic coverage including skip logic, multi-agent merging, groupId routing, platformConfig cleaning, slugify edge cases, reserved triggers, credential divergence (97% instruction, 86.2% branch) - RestSlackWebhookTest: disabled state, signature verification, URL verification challenge, event callback delegation, malformed payloads (100% instruction, 100% branch) - ChannelTargetRouterRefreshTest: added legacy credential attachment for thread targets, deleted/null descriptor handling, non-slack connector skipping, blank groupId → AGENT type - ChannelTargetRouter: 97.4% instruction, 84.1% branch coverage Aggregate unit-testable class coverage: 95.9% instruction, 84.6% branch
Duplicate target names (case-insensitive) caused findDefaultTarget() to silently pick the wrong target via findFirst(). Added usedNames check in validateConfiguration() and corresponding test case.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
… null guards - ChannelIntegrationConfiguration: defensive copying in getters/setters to prevent internal representation exposure (getPlatformConfig, getTargets now return copies; setters handle null safely) - ChannelConnectorMigration: track per-channel create failures and skip migration flag write when any failed, ensuring retry on next startup - ChannelTargetRouter: null-guard on getTargets() in resolveFromIntegration and findDefaultTarget to prevent NPE from configs that bypassed REST validation (e.g. direct DB writes, older records)
getPlatformConfig() now returns a defensive copy, so the previous
remove('channelId') call was silently operating on a throwaway copy.
Fix: get the copy, remove channelId, then set it back via setter.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/main/java/ai/labs/eddi/configs/migration/ChannelConnectorMigration.java (1)
113-126:⚠️ Potential issue | 🟠 MajorStill missing dedup by
(groupKey, agentId)— agents deployed in multiple environments produce duplicate targets.
readDeploymentInfos(deployed)returns deployments across all environments, so an agent deployed in bothproductionandtestwill be visited twice with the sameagentId/agentVersion. Both iterations append aConnectorEntryto the samechannelGroupsbucket, and the downstream loop then creates twoChannelTargets for the same agent (the second receiving a-2trigger suffix).✏️ Suggested dedup
String channelType = connector.getType().toString().toLowerCase(Locale.ROOT); String groupKey = channelType + ":" + channelId; - channelGroups.computeIfAbsent(groupKey, k -> new ArrayList<>()) - .add(new ConnectorEntry(connector, agentId, channelType, - lookupAgentName(agentId, status.getAgentVersion()))); + var bucket = channelGroups.computeIfAbsent(groupKey, k -> new ArrayList<>()); + boolean alreadySeen = bucket.stream().anyMatch(e -> agentId.equals(e.agentId())); + if (!alreadySeen) { + bucket.add(new ConnectorEntry(connector, agentId, channelType, + lookupAgentName(agentId, status.getAgentVersion()))); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ai/labs/eddi/configs/migration/ChannelConnectorMigration.java` around lines 113 - 126, The loop that builds channelGroups currently adds a ConnectorEntry for each deployment and thus can add duplicates when the same agentId appears in multiple environments; to fix this, deduplicate by (groupKey, agentId) before adding: maintain a seen set keyed by groupKey + ":" + agentId (or a Pair) inside ChannelConnectorMigration and, in the loop that iterates over agentConfig.getChannels(), check this set and only add new ConnectorEntry to channelGroups if the (groupKey, agentId) pair is not present, then mark it seen; reference symbols: channelGroups, groupKey, agentId, ConnectorEntry, and the loop that reads readDeploymentInfos/deployed.src/main/java/ai/labs/eddi/configs/channels/rest/RestChannelIntegrationStore.java (1)
282-313:⚠️ Potential issue | 🟠 Major
syncDescriptorstill NPEs on create/duplicate — descriptor sync is silently lost.
DocumentDescriptorFilterruns after the resource method returns, so whencreateChannel/duplicateChannelinvokesyncDescriptorinline (lines 86, 111),documentDescriptorStore.readDescriptor(...)returnsnull. Line 291 then dereferencesdescriptor.getName()→ NPE swallowed by the outercatch (Exception e)at line 309 → name/description sync silently fails for every newly created channel integration. The descriptor ends up with whatever default the filter writes, never the configuredname/channelTypedescription.Add a null-check that returns early (and ideally invokes the descriptor sync via a deferred path, e.g., post-response, if you want create-time naming to actually work).
🛡️ Minimum guard
var descriptor = documentDescriptorStore.readDescriptor( resourceId, currentResourceId.getVersion()); + if (descriptor == null) { + LOG.warnf("No descriptor yet for channel id=%s v=%d — skipping sync", + sanitizeForLog(resourceId), currentResourceId.getVersion()); + return; + } boolean changed = false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ai/labs/eddi/configs/channels/rest/RestChannelIntegrationStore.java` around lines 282 - 313, syncDescriptor currently assumes documentDescriptorStore.readDescriptor(...) returns non-null and dereferences descriptor (descriptor.getName()), causing NPEs for createChannel/duplicateChannel; add a null-check after reading the descriptor in syncDescriptor (if descriptor == null then return early) to avoid swallowing NPEs, and optionally schedule a deferred sync (post-response/event or executor) from createChannel/duplicateChannel so descriptor updates happen after DocumentDescriptorFilter writes the descriptor; keep using documentDescriptorStore.setDescriptor only when descriptor is non-null and changed.
🧹 Nitpick comments (1)
src/main/java/ai/labs/eddi/integrations/channels/ChannelTargetRouter.java (1)
451-463: Optional: redundant copying indeepCopyConfig.Now that
getPlatformConfig()andgetTargets()return defensive copies and the corresponding setters also copy on write, everydeepCopyConfigcall performs three allocations per collection (getter copy → wrapper copy → setter copy). Not a correctness issue, but you can drop the wrappingnew HashMap<>(...)/new ArrayList<>(...)here without losing any isolation.♻️ Suggested simplification
- if (src.getPlatformConfig() != null) { - copy.setPlatformConfig(new HashMap<>(src.getPlatformConfig())); - } - if (src.getTargets() != null) { - copy.setTargets(new ArrayList<>(src.getTargets())); - } + copy.setPlatformConfig(src.getPlatformConfig()); // setter copies internally + copy.setTargets(src.getTargets()); // setter copies internally🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ai/labs/eddi/integrations/channels/ChannelTargetRouter.java` around lines 451 - 463, deepCopyConfig is redundantly wrapping collections returned by ChannelIntegrationConfiguration getters causing extra allocations; remove the unnecessary new HashMap<>(...) and new ArrayList<>(...) so you pass the defensive-copy results directly into copy.setPlatformConfig(...) and copy.setTargets(...). Update the deepCopyConfig method (the constructor-created copy, its setName/setChannelType/setDefaultTargetName calls, and the conditional blocks that handle platform config and targets) to assign the returned collections from getPlatformConfig() and getTargets() directly without creating another wrapper copy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@src/main/java/ai/labs/eddi/configs/channels/rest/RestChannelIntegrationStore.java`:
- Around line 282-313: syncDescriptor currently assumes
documentDescriptorStore.readDescriptor(...) returns non-null and dereferences
descriptor (descriptor.getName()), causing NPEs for
createChannel/duplicateChannel; add a null-check after reading the descriptor in
syncDescriptor (if descriptor == null then return early) to avoid swallowing
NPEs, and optionally schedule a deferred sync (post-response/event or executor)
from createChannel/duplicateChannel so descriptor updates happen after
DocumentDescriptorFilter writes the descriptor; keep using
documentDescriptorStore.setDescriptor only when descriptor is non-null and
changed.
In `@src/main/java/ai/labs/eddi/configs/migration/ChannelConnectorMigration.java`:
- Around line 113-126: The loop that builds channelGroups currently adds a
ConnectorEntry for each deployment and thus can add duplicates when the same
agentId appears in multiple environments; to fix this, deduplicate by (groupKey,
agentId) before adding: maintain a seen set keyed by groupKey + ":" + agentId
(or a Pair) inside ChannelConnectorMigration and, in the loop that iterates over
agentConfig.getChannels(), check this set and only add new ConnectorEntry to
channelGroups if the (groupKey, agentId) pair is not present, then mark it seen;
reference symbols: channelGroups, groupKey, agentId, ConnectorEntry, and the
loop that reads readDeploymentInfos/deployed.
---
Nitpick comments:
In `@src/main/java/ai/labs/eddi/integrations/channels/ChannelTargetRouter.java`:
- Around line 451-463: deepCopyConfig is redundantly wrapping collections
returned by ChannelIntegrationConfiguration getters causing extra allocations;
remove the unnecessary new HashMap<>(...) and new ArrayList<>(...) so you pass
the defensive-copy results directly into copy.setPlatformConfig(...) and
copy.setTargets(...). Update the deepCopyConfig method (the constructor-created
copy, its setName/setChannelType/setDefaultTargetName calls, and the conditional
blocks that handle platform config and targets) to assign the returned
collections from getPlatformConfig() and getTargets() directly without creating
another wrapper copy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a047075a-3cbf-4094-8784-5fd5f108dd56
📒 Files selected for processing (4)
src/main/java/ai/labs/eddi/configs/channels/model/ChannelIntegrationConfiguration.javasrc/main/java/ai/labs/eddi/configs/channels/rest/RestChannelIntegrationStore.javasrc/main/java/ai/labs/eddi/configs/migration/ChannelConnectorMigration.javasrc/main/java/ai/labs/eddi/integrations/channels/ChannelTargetRouter.java
Three-pronged fix for randomly failing Postgres ITs in GitHub Actions: 1. Increase Testcontainers startup timeout from 120s to 180s for both MongoDB and Postgres container ITs. CI runners are resource-constrained and EDDI with Postgres schema creation needs the headroom. 2. Add quarkus.mongodb.health.enabled=false to PostgresIntegrationTestProfile. Without this, the MongoDB health check still runs in the Postgres profile and returns DOWN (no MongoDB instance), dragging the readiness probe to 503. 3. Add rerunFailingTestsCount=1 to maven-failsafe-plugin. Testcontainers ITs can flake on CI due to Docker networking timing — this gives one automatic retry before failing the build.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pom.xml (1)
547-550: LGTM — reasonable retry for flaky Testcontainers ITs.
rerunFailingTestsCount=1is supported by failsafe 3.5.5 and an appropriate mitigation for transient Docker/container-startup timing on CI.One thing to keep in mind: this will silently mask genuinely flaky tests as well as transient infra issues. Consider surfacing reruns in CI output (failsafe summary already reports
Flakes) and tracking them so real flakiness doesn't accumulate behind the retry. If you want stricter behavior later, you can scope the retry to@Tag("integration")classes or add a follow-up to fail the build if flakes exceed a threshold.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pom.xml` around lines 547 - 550, Keep the current rerunFailingTestsCount=1 but add visibility and optional scoping: update the Maven Failsafe configuration (the rerunFailingTestsCount element) to ensure CI surfaces rerun events (e.g., enable Failsafe summary/logging of "Flakes") and consider scoping retries to integration tests by adding a failsafe includes/excludes pattern or using `@Tag`("integration") in surefire/failsafe configuration, and optionally add a follow-up build-failure guard that fails the build if total flake count exceeds a threshold; locate the rerunFailingTestsCount element and the Failsafe plugin configuration when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pom.xml`:
- Around line 547-550: Keep the current rerunFailingTestsCount=1 but add
visibility and optional scoping: update the Maven Failsafe configuration (the
rerunFailingTestsCount element) to ensure CI surfaces rerun events (e.g., enable
Failsafe summary/logging of "Flakes") and consider scoping retries to
integration tests by adding a failsafe includes/excludes pattern or using
`@Tag`("integration") in surefire/failsafe configuration, and optionally add a
follow-up build-failure guard that fails the build if total flake count exceeds
a threshold; locate the rerunFailingTestsCount element and the Failsafe plugin
configuration when making these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c26f5580-a2b1-4e2e-97ad-cad4b646d394
📒 Files selected for processing (4)
pom.xmlsrc/test/java/ai/labs/eddi/integration/ContainerBaseIT.javasrc/test/java/ai/labs/eddi/integration/PostgresIntegrationTestProfile.javasrc/test/java/ai/labs/eddi/integration/postgres/PostgresAgentUseCaseIT.java
✅ Files skipped from review due to trivial changes (2)
- src/test/java/ai/labs/eddi/integration/ContainerBaseIT.java
- src/test/java/ai/labs/eddi/integration/postgres/PostgresAgentUseCaseIT.java
…te MongoDB class ChannelConnectorMigration, V6RenameMigration, and V6QuteMigration all injected the concrete MigrationLogStore (MongoDB) instead of the IMigrationLogStore interface. In Postgres mode, CDI bypassed the DataStoreProducers routing and instantiated the MongoDB store directly. channelConnectorMigration.runIfNeeded() called readMigrationLog() outside any try-catch — the 30s MongoDB timeout exception killed autoDeployAgents(), preventing agentsReadiness from ever being set to true. Health check stayed DOWN indefinitely, causing 503 errors in PostgresInfrastructureIT and PostgresAgentUseCaseIT. Also: - Wrap each migration call in autoDeployAgents() with individual try-catch blocks so a single migration failure cannot block readiness or skip subsequent migrations - Disable MongoDB health check in Postgres test profile - Increase container startup timeouts to 180s - Add failsafe retry for flaky container startups in CI
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/java/ai/labs/eddi/configs/migration/ChannelConnectorMigration.java (1)
16-17: RedundantLocaleimport.
java.util.Localeis already covered by thejava.util.*star import on line 16.♻️ Proposed cleanup
import java.util.*; -import java.util.Locale;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ai/labs/eddi/configs/migration/ChannelConnectorMigration.java` around lines 16 - 17, The import list in ChannelConnectorMigration contains a redundant explicit import java.util.Locale alongside java.util.*, remove the specific java.util.Locale import to eliminate duplication and keep imports tidy; update the import section in ChannelConnectorMigration (remove the line importing Locale) and run a quick compile to ensure no unused-import warnings remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/ai/labs/eddi/configs/migration/ChannelConnectorMigration.java`:
- Around line 196-197: The current code always sets
config.setDefaultTargetName(targets.get(0).getName()), which can pick a target
that has an empty triggers list (e.g., the deterministic-first agent like
"help"); change this to pick the first target in targets whose getTriggers() is
non-empty and use its getName() for config.setDefaultTargetName, and if no
target has triggers fall back to targets.get(0).getName() but emit a logger.warn
explaining the default landed on a trigger-less target; update the logic around
config.setTargets and config.setDefaultTargetName and use the class logger to
warn.
- Around line 78-95: The migration currently uses migrateConnectors() which
calls channelStore.create(config) (via HistorizedResourceStore.create()) and
thus generates new UUIDs on retries causing duplicate
ChannelIntegrationConfiguration entries; update migrateConnectors() to first
query for an existing ChannelIntegrationConfiguration with the same channelType
and channelId (or perform an upsert via the store layer) and only call
channelStore.create(config) when no existing config is found, or alternatively
implement an idempotency check that records migrated channel groups (e.g., in
migrationLogStore or a dedicated tracking collection) and skips creation for
already-migrated groups.
---
Nitpick comments:
In `@src/main/java/ai/labs/eddi/configs/migration/ChannelConnectorMigration.java`:
- Around line 16-17: The import list in ChannelConnectorMigration contains a
redundant explicit import java.util.Locale alongside java.util.*, remove the
specific java.util.Locale import to eliminate duplication and keep imports tidy;
update the import section in ChannelConnectorMigration (remove the line
importing Locale) and run a quick compile to ensure no unused-import warnings
remain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cbeb5f6b-c721-432c-8158-3c11e8f59f54
📒 Files selected for processing (8)
docs/changelog.mdsrc/main/java/ai/labs/eddi/configs/migration/ChannelConnectorMigration.javasrc/main/java/ai/labs/eddi/configs/migration/V6QuteMigration.javasrc/main/java/ai/labs/eddi/configs/migration/V6RenameMigration.javasrc/main/java/ai/labs/eddi/engine/runtime/internal/AgentDeploymentManagement.javasrc/test/java/ai/labs/eddi/configs/migration/ChannelConnectorMigrationTest.javasrc/test/java/ai/labs/eddi/configs/migration/V6QuteMigrationTest.javasrc/test/java/ai/labs/eddi/configs/migration/V6RenameMigrationTest.java
✅ Files skipped from review due to trivial changes (1)
- src/test/java/ai/labs/eddi/configs/migration/ChannelConnectorMigrationTest.java
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 34 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…lp, NPE 1. Migration idempotency (Critical): channelStore.create() generates a new UUID each time. On retry after partial failure, previously succeeded groups would be re-created as duplicates. Now pre-loads existing configs and skips any channelType:channelId that already has a config. 2. Intent key stability (Medium): conversation intent key used mutable names (integration.getName() + target.getName()). Renames broke IUserConversationStore lookups, silently starting new conversations. Now uses stable IDs: channelId + agentId. 3. Legacy help inconsistency (Medium): resolveTarget() returned null for 'help' on new-style channels but routed legacy channels to the default agent. Now applies same help/blank check before legacy fallback. 4. postHelp NPE (Low): name.equalsIgnoreCase(config.getDefaultTargetName()) crashed when defaultTargetName was null (corrupted config). Added null guard. 5. Comment + import cleanup: Updated misleading Javadoc about cached configs; added proper static import for RestUtilities.
…rations # Conflicts: # src/main/java/ai/labs/eddi/integrations/slack/SlackChannelRouter.java # src/test/java/ai/labs/eddi/integrations/slack/SlackChannelRouterTest.java
…rations # Conflicts: # docs/changelog.md # src/main/java/ai/labs/eddi/integrations/slack/SlackChannelRouter.java # src/test/java/ai/labs/eddi/integrations/slack/SlackChannelRouterTest.java
…x, SPDX headers - C1: Replace ThreadLocal<ResolvedTarget> with explicit botToken parameter passing through postMessage/postMessageChunked/postHelp (Loom safety) - C2: Document intent key format change in changelog - M1: Fix stale eddivault -> vault Javadoc in ChannelIntegrationConfiguration - M4: Fix trigger backtick placement in postHelp() - L2: Add SPDX copyright headers to all 12 new files
…s, SPDX - M5: Fix stale eddivault -> vault in ChannelTargetRouter deepCopyConfig Javadoc - M6: Add SPDX headers to IRestChannelIntegrationStore, RestChannelIntegrationStore - L3: Apply LogSanitizer.sanitize() to all Slack-sourced log parameters (CodeQL) - L4: Return defensive copy from ChannelTarget.getTriggers() - L5: Add null guard to postMessageChunked() - L6: Add ObserveConfig bounds validation (cooldown, maxDaily, maxCost >= 0)
…rations # Conflicts: # docs/changelog.md # src/main/java/ai/labs/eddi/configs/agents/model/AgentConfiguration.java
…ven activation The webhook endpoint is now always active. Signing secret verification serves as the natural gate: if no ChannelIntegrationConfiguration entries exist for Slack, there are no signing secrets, and the webhook returns 403. This aligns with EDDI's config-driven philosophy — agent behavior is controlled by JSON configs, not server-level env vars. Removed: - SlackIntegrationConfig.java (ConfigMapping interface) - eddi.slack.enabled property from application.properties - config.enabled() checks in RestSlackWebhook and SlackEventHandler - 'Disabled' test case in RestSlackWebhookTest - Server-Level config section from slack-integration.md
RestSlackWebhook: timestamp from X-Slack-Request-Timestamp logged raw SlackSignatureVerifier: timestamp logged raw on NumberFormatException Both are attacker-controlled HTTP headers — CWE-117 log injection. Also fix stale doc reference to removed master toggle in lessons table.
- Fix silent DM drop: detect channel_type=im, add resolveDefaultForDm fallback - Extract channelType once to avoid redundant map lookups - Update SlackGroupDiscussionListener Javadoc (all styles use expanded mode) - Add App Home setup step to docs (checkbox required for DM input) - Add im:write scope and DM troubleshooting entries - Fix GroupConversationService OutputItem handling in response extraction - Add TEMPLATE_SKIP_PARAMS to LlmTask to protect secrets from template engine - Repair 8 failing tests, add 24 new tests (85 Slack tests, 0 failures) - Overhaul slack-integration.md and group-conversations.md
apiKey is now in TEMPLATE_SKIP_PARAMS and no longer processed by the template engine. Reduce expected processTemplate call counts by 1.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 46 out of 46 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/main/java/ai/labs/eddi/integrations/channels/ChannelTargetRouter.java:164
- resolveDefaultForDm() normalizes channelType for the new-style integration lookup, but the legacy fallback uses CHANNEL_TYPE_SLACK.equals(channelType) (non-normalized). If a caller passes a differently-cased value (e.g., "Slack"), the legacy DM fallback won’t activate even though other router APIs are case-insensitive. Compare against the normalized lower-case value for consistency.
// Fallback to first legacy entry (Slack only)
if (CHANNEL_TYPE_SLACK.equals(channelType) && !legacyMap.isEmpty()) {
var firstLegacy = legacyMap.values().iterator().next();
String trimmed = messageText != null ? messageText.trim() : "";
When a markdown table was immediately followed by a fenced code block, the table wrapper ` was left unclosed, producing malformed mrkdwn. Now emit the closing fence before toggling into the real code block. Added test: mrkdwn_tableFollowedByCodeBlock_closesTableFence Co-authored-by: Copilot <copilot@github.com>
rolandpickl
approved these changes
May 20, 2026
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
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.
Multi-Target Channel Integration & Slack Hardening
Summary
Replaces the 1:1 agent-per-channel
SlackChannelRouterwith a multi-targetChannelTargetRouterthat supports trigger-keyword routing, multi-agent group discussions, DM conversations, and legacy backward compatibility. The Slack integration is now fully config-driven (no server toggles) and production-hardened with comprehensive test coverage.What This PR Does
🏗️ New:
ChannelIntegrationConfigurationResourceA new first-class EDDI resource type (
ai.labs.channel) that replaces the per-agentChannelConnectormodel. Each configuration maps a platform channel to multiple targets (agents or groups) with trigger-keyword routing:{ "name": "Slack General", "channelType": "slack", "defaultTargetName": "assistant", "platformConfig": { "channelId": "C0123ABCDEF", "botToken": "${vault:slack-bot-token}", "signingSecret": "${vault:slack-signing-secret}" }, "targets": [ { "name": "assistant", "type": "AGENT", "targetId": "agent-123", "triggers": ["ask", "help"] }, { "name": "panel", "type": "GROUP", "targetId": "group-456", "triggers": ["panel", "debate"] } ] }Full REST CRUD at
/channelstore/channels.🔄 Multi-Target Routing (
ChannelTargetRouter)Replaces
SlackChannelRouterwith a platform-agnostic router supporting:@EDDI panel: Should we use K8s?→ GROUP target)D-prefixed DM channels fall back to the first available Slack integration's default targetChannelConnectorentries are still scanned and used when no new-style config covers the channel💬 Direct Message (IM) Support
Slack DMs were previously silently dropped because:
app_mentionin DMs — onlymessageevents withchannel_type: "im"D-prefixed, unique per user-bot pair) and never pre-configuredFix:
SlackEventHandlernow detectschannel_type: "im"andChannelTargetRouter.resolveDefaultForDm()resolves DMs to the first available Slack integration's default target.🗣️ Group Discussion UX (Header + Thread)
All 5 discussion styles use expanded mode in Slack:
🔧 Config-Driven Activation
Removed the
eddi.slack.enabledserver toggle. The Slack webhook endpoint is always available; signature verification against configured signing secrets is the sole gatekeeper. No configured secrets = all events rejected (HTTP 403).🔒 Security Hardening
TEMPLATE_SKIP_PARAMSprevents secrets (apiKey,botToken,signingSecret) from being processed by the template engineThreadLocalusage removed (unnecessary in Quarkus CDI context)📄 Auto-Migration
ChannelConnectorMigrationruns at startup and converts legacyChannelConnectorentries on agents intoChannelIntegrationConfigurationresources automatically. Migration is idempotent with structural matching (won't duplicate on restart).Files Changed
46 files changed, 6,270 insertions, 1,210 deletions
New Files (15)
ChannelIntegrationConfiguration.javaChannelTarget.javaObserveConfig.javaIChannelIntegrationStore.javaChannelIntegrationStore.javaIRestChannelIntegrationStore.javaRestChannelIntegrationStore.javaChannelTargetRouter.javaChannelConnectorMigration.javaChannelTargetRouterTest.javaChannelTargetRouterRefreshTest.javaChannelModelTest.javaRestChannelIntegrationStoreValidationTest.javaChannelConnectorMigrationTest.javaconversation-cancel-plan.mdDeleted Files (3)
SlackChannelRouter.javaChannelTargetRouterSlackChannelRouterTest.javaChannelTargetRouterTestSlackIntegrationConfig.javaeddi.slack.enabledtoggle removedModified Files (28)
Core routing, event handling, group discussion listener, docs, tests, and infrastructure.
Test Coverage
ChannelTargetRouterTestChannelTargetRouterRefreshTestChannelModelTestRestChannelIntegrationStoreValidationTestChannelConnectorMigrationTestSlackEventHandlerTestSlackGroupDiscussionListenerTestSlackWebApiClientTestSlackSignatureVerifierTestRestSlackWebhookTestDocumentation
slack-integration.md— Full rewrite: 7-step setup guide (including App Home for DMs), architecture diagram, trigger routing, UX modes, retry policy, Markdown→mrkdwn conversion table, troubleshootinggroup-conversations.md— New Slack Integration section: header+thread UX, all 5 styles' phase flow in Slack, trigger keywords, follow-up conversationschangelog.md— 12 detailed entries covering the full development historyBreaking Changes
SlackChannelRouterremoved — replaced byChannelTargetRouter. No external API consumers (internal class).eddi.slack.enabledproperty removed — Slack activation is now purely config-driven. Remove this property fromapplication.propertiesif set.ChannelConnectorstill works but is deprecated. Auto-migration creates equivalentChannelIntegrationConfigurationentries at startup.How to Test
@EDDI hello→ routes to default agent@EDDI panel: Should we adopt K8s?→ routes to GROUP target@EDDI help→ lists available targets with triggers