feat: Agentic Improvements#485
Merged
Merged
Conversation
- Fix round_robin: deterministic AtomicInteger rotation (was shuffle) - Add 'random' strategy for explicit shuffling - Reject inert security flags with HTTP 400 (create/update/duplicate) - Add public discovery: GET /.well-known/capabilities - Emit CAPABILITY_SELECTION audit events with full context - Add eddi.capability.miss.count and .strategy.applied metrics - Add OpenAPI annotations for new public endpoints
- 13 new tests for CapabilityRegistryService: round_robin determinism across 100 calls, counter reset on register/unregister, random strategy, miss/strategy metrics, null/blank/empty edge cases - 7 new tests for RestAgentStore: security flag rejection for each flag (create/update/duplicate), null/false-flags allowed - 4 new tests for CapabilityMatchCondition: audit event emission with full context, null collector handling, collector failure resilience, no audit on failure
- Update status table: Wave 3 = COMPLETE (2026-05-07) - Strike fixed bugs from §1.3 (round_robin, inert security flags) - Mark external discovery endpoint as done in §1.2 - Add completion note to §5 with test counts
- CounterweightService: engine-level safety injection with normal/cautious/strict levels - IdentityMaskingService: identity concealment rules prepended to system prompts - DeploymentContextCondition: behavior rule matching on deployment env and agent tags - CounterweightConfig/IdentityMaskingConfig: config-driven with safe defaults - Wired into LlmTask after system prompt compilation - 33 new tests, 55 existing LlmTask tests pass (no regressions)
- Add null coalescing for systemMessage in CounterweightService and IdentityMaskingService - Replace java.util.List FQN with imported List in CounterweightConfig
- Add PaginatedResponseStore (Caffeine-backed, 15min TTL) - Add FetchToolResponsePageTool (built-in tool for page retrieval) - Add DiscoverToolsTool (meta-tool for lazy/dynamic tool discovery) - Add ToolLoadingStrategy config (eager/lazy/dynamic) - Enhance ToolResponseTruncator with paginate/summarize strategies - Add truncationStrategy and summarizerModel to ToolResponseLimits - Wire FetchToolResponsePageTool into AgentOrchestrator - 42 new unit tests (126 total pass)
- Add MemoryCheckpoint record (immutable state snapshot) - Add IConversationCheckpointStore interface (CRUD + pruning + GDPR) - Add MongoConversationCheckpointStore (compound index) - Add PostgresConversationCheckpointStore (JSONB + indexed columns) - Add MemorySnapshotService (create/rollback with type-aware restore) - Add SessionManagement config to AgentConfiguration - Wire checkpoint store in DataStoreProducers (DB-agnostic) - 22 new unit tests (all pass)
- Add IAttachmentStore interface (store/load/delete/list) - Add PostgresAttachmentStore (BYTEA columns, 20MB cap) - Add GridFsAttachmentStore (MongoDB GridFS with metadata) - Add MimeValidator (magic-byte detection, 14 file types, no dep) - Add AttachmentForwarder (image→ImageContent, other→TextContent) - Wire attachment store in DataStoreProducers (DB-agnostic) - Cross-conversation access protection on all load operations - 26 new unit tests (all pass)
- Add JacksonCanonicalizer (RFC 8785, pure Jackson, no dep) - Add SignedEnvelope record (forSigning/withSignature/canonicalForm) - Add NonceCacheService (Caffeine replay protection, 3-stage validation) - Add AgentPublicKey record (versioned keys, isValidAt, rotation windows) - Extend AgentIdentity with multi-key rotation (getKeyForVersion/getKeyValidAt) - Extend AgentSigningService (signEnvelope, verifyEnvelope, rotateKey) - 30 new unit tests (all pass)
- Remove unused List import from MemoryCheckpoint - Use TTL-based cache in NonceCacheService (was computing ttlMs but not using it) - Fix missing blank line after package in MemorySnapshotService - Fix misleading canonicalForm() javadoc in SignedEnvelope - Replace Map<String,Boolean> with Set<String> for MIME_ZIP_SUBTYPES - Fix debugf ambiguity + int type in PostgresAttachmentStore.deleteByConversation
- Concern 1 (Pillar 1): CounterweightService now resolves presets from Prompt Snippets before falling back to built-in defaults. Admins can customize via REST API without recompilation. - Concern 2 (Pillar 8): IdentityMaskingConfig moved from AgentConfiguration to LlmConfiguration.Task alongside CounterweightConfig. Removed bespoke getter/setter from IConversationMemory — config no longer flows through memory. - Concern 3 (Organization): IAttachmentStore and MimeValidator moved from engine.memory to engine.attachments package. All 111 affected tests pass.
- MimeValidatorTest: +11 tests for all magic-byte branches (72.8% → 99.6%) - AgentSigningServiceTest: +7 tests for versioned keys, envelope sign/verify (48.5% → 86.7%) - MemorySnapshotServiceTest: +1 test for all property type branches (77.9% → 100%) - langchain.md: add Behavioral Safety (counterweight + identity masking) section - architecture.md: add System Prompt Modifiers and Attachment Storage subsections - changelog.md: document coverage audit and doc enrichment
…ability notes - langchain.md: placement values changed from append/prepend to suffix/prefix (matching code) - langchain.md: removed non-existent agentName field from identityMasking docs - langchain.md: added enabled:true to counterweight example + explicit usability notes - langchain.md: added counterweight.enabled row, fixed customInstructions type to string[] - changelog.md: added migration note for identityMasking config relocation - changelog.md: documented PostgreSQL compatibility verification
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:
📝 Walkthrough<review_stack_artifact> </review_stack_artifact> WalkthroughAdds multimodal attachment stores (Mongo/Postgres) and async REST, MIME validation, conversation checkpoint stores and snapshot service, LLM prompt modifiers (identity masking, counterweight) and truncation/pagination/summarization, capability discovery/audit, agent signing and transcript signatures, plus extensive tests and docs. ChangesAgentic Improvements
Sequence Diagram(s)sequenceDiagram
participant Client
participant RestAttachmentUpload
participant IAttachmentStore
participant PaginatedResponseStore
participant FetchToolResponsePageTool
Client->>RestAttachmentUpload: POST /conversations/{id}/attachments
RestAttachmentUpload->>IAttachmentStore: store(bytes, mime, filename, convId, tenantId)
IAttachmentStore-->>RestAttachmentUpload: Attachment(storageRef,...)
RestAttachmentUpload-->>Client: 201 {storageRef,...}
Client->>FetchToolResponsePageTool: fetchPage(responseId, pageNumber)
FetchToolResponsePageTool->>PaginatedResponseStore: getPage(responseId, pageNumber)
PaginatedResponseStore-->>FetchToolResponsePageTool: PageResult
FetchToolResponsePageTool-->>Client: JSON {page,totalPages,toolName,content}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR introduces multiple config-driven “agentic” capabilities across EDDI’s LLM execution, rule engine, storage backends (Mongo/Postgres), and A2A surface area—focused on safer prompting, tool-governance, session checkpointing, attachments, capability routing, and cryptographic identity primitives.
Changes:
- Added Wave 1 behavioral safety prompt modifiers (counterweight + identity masking) applied in
LlmTask. - Implemented Wave 2 tool governance additions: paginated tool responses + discovery meta-tool + truncation strategy extensions.
- Implemented Waves 3–6 foundations: deterministic capability registry routing + public discovery endpoints + session checkpoint stores + attachment stores + signing primitives (canonicalization/envelopes/nonce replay).
Reviewed changes
Copilot reviewed 58 out of 58 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/ai/labs/eddi/modules/rules/impl/conditions/DeploymentContextConditionTest.java | Unit tests for environment/tag-based rule condition. |
| src/test/java/ai/labs/eddi/modules/rules/impl/conditions/CapabilityMatchConditionTest.java | Added tests around audit emission behavior for capability selection. |
| src/test/java/ai/labs/eddi/modules/llm/tools/PaginatedResponseStoreTest.java | Tests for storing and retrieving paginated tool responses via cache. |
| src/test/java/ai/labs/eddi/modules/llm/tools/impl/FetchToolResponsePageToolTest.java | Tests for the built-in page fetch tool response JSON behavior. |
| src/test/java/ai/labs/eddi/modules/llm/tools/impl/DiscoverToolsToolTest.java | Tests for discovery meta-tool filtering/capping behavior. |
| src/test/java/ai/labs/eddi/modules/llm/impl/ToolResponseTruncatorExtendedTest.java | Coverage for truncate/paginate/summarize strategy selection and fallbacks. |
| src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskTest.java | Updated wiring for new services/tools in LLM task construction. |
| src/test/java/ai/labs/eddi/modules/llm/impl/IdentityMaskingServiceTest.java | Tests for identity masking prompt prepending + metrics. |
| src/test/java/ai/labs/eddi/modules/llm/impl/CounterweightServiceTest.java | Tests for counterweight presets/custom instructions + scheduled downgrade + metrics. |
| src/test/java/ai/labs/eddi/modules/llm/impl/AttachmentForwarderTest.java | Tests attachment-to-langchain4j Content conversion behavior. |
| src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorTest.java | Updated built-in tool collection expectations for fetch-page tool. |
| src/test/java/ai/labs/eddi/engine/memory/model/MemoryCheckpointTest.java | Tests for checkpoint record creation/immutability/parenting semantics. |
| src/test/java/ai/labs/eddi/engine/memory/MemorySnapshotServiceTest.java | Tests checkpoint create/rollback logic and property restoration branches. |
| src/test/java/ai/labs/eddi/engine/attachments/MimeValidatorTest.java | Tests for magic-byte detection and declared-vs-detected compatibility logic. |
| src/test/java/ai/labs/eddi/configs/agents/rest/RestAgentStoreTest.java | Added tests for rejecting inert security flags during agent operations. |
| src/test/java/ai/labs/eddi/configs/agents/model/SessionManagementTest.java | Tests for new session management config defaults and setters. |
| src/test/java/ai/labs/eddi/configs/agents/crypto/SignedEnvelopeTest.java | Tests for deterministic canonicalization behavior in envelope signing. |
| src/test/java/ai/labs/eddi/configs/agents/crypto/NonceCacheServiceTest.java | Tests replay protection freshness/clock-skew/replay detection. |
| src/test/java/ai/labs/eddi/configs/agents/crypto/JacksonCanonicalizerTest.java | Tests RFC8785-like canonicalization properties (sorting, determinism). |
| src/test/java/ai/labs/eddi/configs/agents/crypto/AgentPublicKeyTest.java | Tests key validity windows and factory helpers. |
| src/test/java/ai/labs/eddi/configs/agents/CapabilityRegistryServiceTest.java | Tests deterministic round-robin + random strategy + metrics behavior. |
| src/test/java/ai/labs/eddi/configs/agents/AgentSigningServiceTest.java | Added tests for versioned keys + envelope sign/verify + tamper detection. |
| src/main/java/ai/labs/eddi/modules/rules/impl/RuleDeserialization.java | Registers the new deploymentContext rule condition in deserialization. |
| src/main/java/ai/labs/eddi/modules/rules/impl/conditions/DeploymentContextCondition.java | New rule condition: deployment env + optional agent tags matching. |
| src/main/java/ai/labs/eddi/modules/rules/impl/conditions/CapabilityMatchCondition.java | Emits audit event on successful capability selection. |
| src/main/java/ai/labs/eddi/modules/llm/tools/PaginatedResponseStore.java | New cache-backed store for paginated tool results with TTL. |
| src/main/java/ai/labs/eddi/modules/llm/tools/impl/FetchToolResponsePageTool.java | New built-in LLM tool to fetch stored tool response pages by responseId. |
| src/main/java/ai/labs/eddi/modules/llm/tools/impl/DiscoverToolsTool.java | New meta-tool to list available tools by category/keywords. |
| src/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.java | Adds task-level counterweight/identity masking/tool loading strategy + truncation config extensions. |
| src/main/java/ai/labs/eddi/modules/llm/impl/ToolResponseTruncator.java | Adds paginate/summarize strategies and metrics tagging. |
| src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java | Applies identity masking then counterweight injection to system prompt; wires fetch-page tool. |
| src/main/java/ai/labs/eddi/modules/llm/impl/IdentityMaskingService.java | New service to prepend engine-enforced identity rules with metrics. |
| src/main/java/ai/labs/eddi/modules/llm/impl/CounterweightService.java | New service to inject safety counterweights (snippet-resolved presets + scheduled downgrade). |
| src/main/java/ai/labs/eddi/modules/llm/impl/AttachmentForwarder.java | Converts stored attachments into langchain4j Content (images as data-URIs). |
| src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java | Adds FetchToolResponsePageTool into the enabled tool set. |
| src/main/java/ai/labs/eddi/engine/memory/MongoConversationCheckpointStore.java | MongoDB checkpoint persistence implementation for conversation checkpoints. |
| src/main/java/ai/labs/eddi/engine/memory/model/MemoryCheckpoint.java | New checkpoint record model used for snapshot/rollback/fork metadata. |
| src/main/java/ai/labs/eddi/engine/memory/MemorySnapshotService.java | New snapshot/rollback service persisting checkpoints and restoring properties. |
| src/main/java/ai/labs/eddi/engine/memory/IConversationCheckpointStore.java | Store abstraction for checkpoints (CRUD + pruning + GDPR deletion). |
| src/main/java/ai/labs/eddi/engine/attachments/MimeValidator.java | New magic-byte MIME detection + compatibility checks. |
| src/main/java/ai/labs/eddi/engine/attachments/IAttachmentStore.java | New attachment store abstraction with conversation-scoped access control. |
| src/main/java/ai/labs/eddi/engine/a2a/RestA2AEndpoint.java | Adds gated public capability discovery endpoints under /.well-known/*. |
| src/main/java/ai/labs/eddi/datastore/postgres/PostgresConversationCheckpointStore.java | PostgreSQL implementation of checkpoint store (JSONB + indexed columns). |
| src/main/java/ai/labs/eddi/datastore/postgres/PostgresAttachmentStore.java | PostgreSQL implementation of attachment store using BYTEA with size cap. |
| src/main/java/ai/labs/eddi/datastore/mongo/GridFsAttachmentStore.java | Mongo GridFS implementation of attachment store with metadata scoping. |
| src/main/java/ai/labs/eddi/datastore/DataStoreProducers.java | Wires attachment/checkpoint stores for Mongo vs Postgres selection. |
| src/main/java/ai/labs/eddi/configs/agents/rest/RestAgentStore.java | Rejects inert crypto/security flags on create/update/duplicate. |
| src/main/java/ai/labs/eddi/configs/agents/model/AgentConfiguration.java | Adds session management config + versioned agent public keys. |
| src/main/java/ai/labs/eddi/configs/agents/crypto/SignedEnvelope.java | New signed envelope record with canonicalization support. |
| src/main/java/ai/labs/eddi/configs/agents/crypto/NonceCacheService.java | New replay protection service using cache + TTL window. |
| src/main/java/ai/labs/eddi/configs/agents/crypto/JacksonCanonicalizer.java | Deterministic JSON canonicalization helper for signing. |
| src/main/java/ai/labs/eddi/configs/agents/crypto/AgentPublicKey.java | Versioned public-key model supporting validity windows. |
| src/main/java/ai/labs/eddi/configs/agents/CapabilityRegistryService.java | Deterministic round-robin routing + random strategy + metrics. |
| src/main/java/ai/labs/eddi/configs/agents/AgentSigningService.java | Adds versioned keys + envelope sign/verify + rotation helpers. |
| planning/agentic-improvements-plan.md | Updates plan status notes for implemented waves/components. |
| docs/langchain.md | Adds behavioral safety documentation and configuration examples. |
| docs/changelog.md | Adds extensive changelog entries for multiple waves and components. |
| docs/architecture.md | Documents prompt modifiers and attachment storage subsystem. |
💡 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: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/java/ai/labs/eddi/configs/agents/rest/RestAgentStore.java (1)
180-184:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
duplicateAgentrejects source config — strands imported agents with security flags.The validation in
duplicateAgent()reads the source agent and immediately callsrejectInertSecurityFlags(agentConfig). However, agents imported viaRestImportService.createNewAgent()bypass the REST layer entirely and callIAgentStore.create()directly, bypassing all validation. This means:
- An agent imported from a backup/zip with crypto flags enabled will persist successfully
- Attempting to duplicate it later fails with
BadRequestException(confirmed in test: "duplicateAgent should reject security flags from source config")- Admins cannot fork/recover such agents
Two viable shapes:
- Strip/clear the offending flags on the duplicate before persisting (and audit-log the strip).
- Keep the rejection but only on the target config once the user explicitly opts into a duplicate-with-changes flow.
If the intent is genuinely "fail closed" until Wave 6 lands, document the rationale and a recovery path (e.g., a one-shot admin script) in the changelog.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/configs/agents/rest/RestAgentStore.java` around lines 180 - 184, The duplicateAgent method currently calls rejectInertSecurityFlags(agentConfig) on the source read by agentStore.read(id, version) which prevents duplicating imported agents that contain inert/crypto flags; change the flow so you do not reject the source config but instead clear/strip the inert security/crypto flags from the new duplicate before saving (and emit an audit/log entry about the strip), i.e., locate duplicateAgent and remove the rejectInertSecurityFlags(agentConfig) call on the source read, then apply a sanitize/strip step on the object that will be persisted (the new target config) to clear the offending flags (mirroring what RestImportService.createNewAgent and IAgentStore.create may have allowed) so admins can fork/recover agents while keeping security flags out of newly persisted duplicates.src/main/java/ai/labs/eddi/configs/agents/CapabilityRegistryService.java (1)
84-113:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRound-robin counters reset more aggressively than intended.
Two problems with the current reset logic:
unregisterresets counters for unrelated skills. TheremoveIfat lines 105-113 walks every skill in the index and resets the counter for each one whose entries are non-empty — even skills the unregistered agent never declared. Removing one agent therefore restarts round-robin for every other skill in the registry, breaking fair rotation across the system on each agent lifecycle event.registerresets the counter on every call (line 92), including idempotent re-registrations where the agent set for that skill is unchanged. Combined withunregister(agentId)at line 78, a no-op config reload effectively zeroes counters globally.Recommend scoping the reset to skills the affected agent actually participates in, and only resetting when membership for that skill changed.
♻️ Proposed fix — scope counter resets to affected skills
public void register(String agentId, AgentConfiguration config) { - // Remove any previous entries for this agent - unregister(agentId); + // Remove any previous entries for this agent + Set<String> previousSkills = unregisterInternal(agentId); if (config.getCapabilities() == null || config.getCapabilities().isEmpty()) { return; } + Set<String> newSkills = new HashSet<>(); for (Capability cap : config.getCapabilities()) { if (cap.getSkill() == null || cap.getSkill().isBlank()) { continue; } String skill = cap.getSkill().toLowerCase(Locale.ROOT).trim(); skillIndex.computeIfAbsent(skill, k -> new CopyOnWriteArrayList<>()) .add(new AgentCapabilityEntry(agentId, cap)); - // Reset round-robin counter on topology change - roundRobinCounters.put(skill, new AtomicInteger(0)); + newSkills.add(skill); } + // Only reset counters for skills whose membership actually changed. + Sets.symmetricDifference(previousSkills, newSkills) + .forEach(s -> roundRobinCounters.computeIfPresent(s, (k, v) -> new AtomicInteger(0))); + LOGGER.debugf("Registered %d capabilities for agent '%s'", config.getCapabilities().size(), agentId); } public void unregister(String agentId) { - skillIndex.values().forEach(entries -> entries.removeIf(e -> e.agentId().equals(agentId))); - // Clean up empty skill entries and reset round-robin counters - skillIndex.entrySet().removeIf(entry -> { - if (entry.getValue().isEmpty()) { - roundRobinCounters.remove(entry.getKey()); - return true; - } - // Reset counter on topology change even if skill still has entries - roundRobinCounters.put(entry.getKey(), new AtomicInteger(0)); - return false; - }); + unregisterInternal(agentId); + } + + private Set<String> unregisterInternal(String agentId) { + Set<String> affected = new HashSet<>(); + skillIndex.forEach((skill, entries) -> { + if (entries.removeIf(e -> e.agentId().equals(agentId))) { + affected.add(skill); + } + }); + skillIndex.entrySet().removeIf(entry -> { + if (entry.getValue().isEmpty()) { + roundRobinCounters.remove(entry.getKey()); + return true; + } + return false; + }); + // Reset only the counters for skills this agent actually touched. + affected.forEach(s -> roundRobinCounters.computeIfPresent(s, (k, v) -> new AtomicInteger(0))); + return affected; }(Replace
Sets.symmetricDifferencewith a small inline helper if Guava isn't available.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/configs/agents/CapabilityRegistryService.java` around lines 84 - 113, The register/unregister logic currently resets roundRobinCounters for every skill on any agent change; instead restrict resets to only the skills whose membership actually changed: in register(CapabilityConfig config, String agentId) iterate the capabilities and for each skill compute the previous entries (skillIndex.get(skill) or empty) and only replace/append the AgentCapabilityEntry and call roundRobinCounters.put(skill, new AtomicInteger(0)) if the list of agentIds for that skill actually changed (i.e., agent was newly added or removed); likewise, in unregister(String agentId) avoid iterating all skills — for each skill where you remove entries (entries.removeIf(e -> e.agentId().equals(agentId)) returned true or where the agent was present before removal), then if the resulting list is empty remove the skillIndex entry and roundRobinCounters.remove(skill), otherwise only reset roundRobinCounters.put(skill, new AtomicInteger(0)) for those specific skills whose membership changed; use skillIndex and roundRobinCounters as the authoritative maps and only touch counters for affected skills.src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorTest.java (1)
38-116:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStore the
FetchToolResponsePageToolmock as a field to enable identity assertions.The mock for
FetchToolResponsePageToolis created anonymously inside the constructor call insetUp()and is not captured in a field, unlike the other eight tools. The count assertionassertEquals(9, tools.size(), ...)at lines 107 and 127 verifies the total but cannot verify which tool is the 9th. A regression that swapsFetchToolResponsePageToolfor any other tool, or duplicates an existing tool, would pass these assertions undetected.🔧 Proposed fix
private WeatherTool weatherTool; + private FetchToolResponsePageTool fetchToolResponsePageTool; private IConversationMemory mockMemory; `@BeforeEach` void setUp() { ... weatherTool = mock(WeatherTool.class); + fetchToolResponsePageTool = mock(FetchToolResponsePageTool.class); orchestrator = new AgentOrchestrator(calculatorTool, dateTimeTool, webSearchTool, dataFormatterTool, webScraperTool, textSummarizerTool, - pdfReaderTool, weatherTool, mock(FetchToolResponsePageTool.class), mock(ToolExecutionService.class), + pdfReaderTool, weatherTool, fetchToolResponsePageTool, mock(ToolExecutionService.class),Then in
testCollectEnabledTools_AllToolsEnabled:assertTrue(tools.contains(weatherTool)); + assertTrue(tools.contains(fetchToolResponsePageTool));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorTest.java` around lines 38 - 116, The test creates the FetchToolResponsePageTool mock inline in setUp() so tests can't assert its identity; change the class to declare a private FetchToolResponsePageTool fetchTool field, instantiate and assign fetchTool = mock(FetchToolResponsePageTool.class) in setUp() and pass fetchTool into the AgentOrchestrator constructor (instead of mock(...)), then update assertions in testCollectEnabledTools_AllToolsEnabled to assertTrue(tools.contains(fetchTool)) so the 9th tool is explicitly verified.
🧹 Nitpick comments (15)
src/main/java/ai/labs/eddi/configs/agents/rest/RestAgentStore.java (1)
301-313: 💤 Low valueUse the imported short name and a constant for the error message.
jakarta.ws.rs.BadRequestExceptionis referenced by FQN here even though no other JAX-RS exception is imported in this style elsewhere in the file. Addingimport jakarta.ws.rs.BadRequestException;and using the short name keeps the file consistent. Also consider hoisting the message to aprivate static final Stringso the same wording flows to all three call sites and is easy to test against.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/configs/agents/rest/RestAgentStore.java` around lines 301 - 313, The method rejectInertSecurityFlags currently throws a BadRequestException using its FQN; add an import for jakarta.ws.rs.BadRequestException and replace the FQN with the short class name in rejectInertSecurityFlags (and other places in this file that use the FQN) and hoist the literal error text into a private static final String (e.g., SECURITY_NOT_AVAILABLE_MSG) at class scope so all three call sites use that constant for consistency and easier testing.src/test/java/ai/labs/eddi/configs/agents/rest/RestAgentStoreTest.java (1)
205-233: ⚡ Quick winAsserting validation passed by catching NPE is fragile.
createAgent_allowsNullSecurityandcreateAgent_allowsAllFlagsFalseboth rely on the fact that the next unmocked call after validation throwsNullPointerException. Any future refactor that adds another guard between the security check andcreateDocument(or that mocksrestVersionInfo/jsonSchemaCreatordifferently) silently turns these into false positives — they'll still "pass" while no longer exercising the validation path.Stub
restVersionInfo/createDocument(or extractrejectInertSecurityFlagsto a package-private method) and assert it returns normally, or useassertDoesNotThrow(BadRequestException.class, ...)style by spying on the validator directly.♻️ Sketch — exercise the guard, not the NPE
- // Validation passes (no BadRequestException), but downstream createDocument - // is not mocked — NPE is expected. We only test the validation guard. - var thrown = assertThrows(NullPointerException.class, - () -> restAgentStore.createAgent(config)); - // If we got here, the security validation did NOT throw BadRequestException - assertNotNull(thrown); + // Assert specifically that BadRequestException is NOT raised. + // Any downstream exception (including NPE from unmocked deps) is acceptable + // — what we care about is that the security guard didn't fire. + try { + restAgentStore.createAgent(config); + } catch (jakarta.ws.rs.BadRequestException e) { + fail("Security guard should not reject: " + e.getMessage()); + } catch (Exception expectedDownstream) { + // OK — guard passed + }Or, cleaner, make
rejectInertSecurityFlagspackage-private and unit-test it in isolation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/ai/labs/eddi/configs/agents/rest/RestAgentStoreTest.java` around lines 205 - 233, The tests createAgent_allowsNullSecurity and createAgent_allowsAllFlagsFalse are fragile because they infer validation success from a downstream NullPointerException; instead, either stub/mock the downstream calls (restVersionInfo and createDocument) so createAgent(config) returns normally and assert no BadRequestException is thrown, or extract rejectInertSecurityFlags to a package-private method and unit-test it directly; update the tests to spy/mock restVersionInfo/createDocument (or call the new package-private rejectInertSecurityFlags) and use assertDoesNotThrow or direct assertions on the validator to prove the security validation path succeeds rather than relying on an NPE.src/test/java/ai/labs/eddi/configs/agents/model/SessionManagementTest.java (1)
16-107: ⚡ Quick winTests cover defaults and accessor wiring.
Note these tests will need to be rewritten if
SessionManagement/AutoSnapshotare migrated to records (see the comment onAgentConfiguration.java). Consider also adding a Jackson round-trip test asserting unsetsessionManagementis omitted from serialized JSON — the cross-cutting rule in the plan (§2.3) requires "absent from the ZIP export if left at default", and a unit-level guard catches regressions before integration tests do.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/ai/labs/eddi/configs/agents/model/SessionManagementTest.java` around lines 16 - 107, Tests currently validate defaults and getters for AgentConfiguration.SessionManagement and AutoSnapshot but lack a Jackson round-trip asserting that a null/absent sessionManagement is omitted from serialized JSON; add a unit test that serializes an AgentConfiguration with no sessionManagement (using the same ObjectMapper/config used in production), asserts the resulting JSON does not contain "sessionManagement", then deserializes back to ensure behavior stays consistent; also note that if you migrate SessionManagement/AutoSnapshot to records you must update these tests to construct/compare record instances and adjust any setters/getters usage (refer to AgentConfiguration, AgentConfiguration.SessionManagement, AgentConfiguration.SessionManagement.AutoSnapshot, getSessionManagement, setSessionManagement).src/main/java/ai/labs/eddi/configs/agents/model/AgentConfiguration.java (1)
290-299: 💤 Low valueUndocumented
version == 0legacy fallback.
AgentPublicKey.versionis documented as "monotonically increasing key version (1-based)", yetgetKeyForVersionreturns the legacypublicKeywhenversion == 0andkeysis empty. That's a useful escape hatch but it's invisible to callers. Either document this sentinel in the Javadoc or rename/route it through a dedicatedgetLegacyPublicKey()method so call sites are explicit about asking for the unversioned key.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/configs/agents/model/AgentConfiguration.java` around lines 290 - 299, The method AgentConfiguration.getKeyForVersion currently treats version==0 as a legacy sentinel returning the field publicKey when keys is empty; surface this behavior explicitly by adding a new method getLegacyPublicKey() that returns the legacy publicKey (and mark it clearly in its Javadoc), then change getKeyForVersion to stop treating 0 as a special case (remove the version==0 branch) and only search the keys list by AgentPublicKey.version; update or instruct callers that need the unversioned key to call getLegacyPublicKey() instead of passing 0 to getKeyForVersion.src/main/java/ai/labs/eddi/configs/agents/crypto/NonceCacheService.java (1)
86-103: ⚡ Quick winConsider raising rejection log levels for security visibility.
Replay/clock-skew/freshness rejections are security-relevant signals — at DEBUG they are typically off in production, leaving only the Micrometer counters as evidence. WARN (or at minimum INFO for clock-skew, WARN for replay) makes incidents visible in standard log aggregation without needing dashboards wired up.
As per coding guidelines: "Use appropriate levels: DEBUG (verbose), INFO (important events), ERROR (failures)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/configs/agents/crypto/NonceCacheService.java` around lines 86 - 103, Change the rejection log levels in NonceCacheService so security-relevant events are visible: replace LOGGER.debugf calls for the TOO_OLD and REPLAY branches with LOGGER.warnf, and replace the CLOCK_SKEW LOGGER.debugf with LOGGER.infof (or warnf if you prefer stricter visibility). Keep the existing counter increments (clockSkewRejections, replayRejections) and return values (NonceValidation.TOO_OLD, CLOCK_SKEW, REPLAY) unchanged; only update the log method invocations around the checks that use nonce, timestampMs, now, maxAgeMs, and clockSkewMs.src/main/java/ai/labs/eddi/configs/agents/crypto/SignedEnvelope.java (1)
64-73: ⚡ Quick winDefensive-copy
payloadto preserve immutability.The record stores the caller's
Mapby reference. If the caller mutates it afterforSigning(...)runs (or afterwithSignature(...)),canonicalForm()will silently produce a JSON that differs from what was signed/verified previously. Wrapping in an unmodifiable copy makes the "immutable signed envelope" guarantee actually hold.♻️ Proposed defensive copy
public static SignedEnvelope forSigning(String senderId, String recipientId, Map<String, Object> payload) { return new SignedEnvelope( senderId, recipientId, - payload, + payload == null ? Map.of() : Map.copyOf(payload), UUID.randomUUID().toString(), Instant.now().toEpochMilli(), null, // no signature yet 0); }Note:
Map.copyOfrejects null values, which is fine for canonical JSON; if your payloads can include nulls, useCollections.unmodifiableMap(new LinkedHashMap<>(payload))instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/configs/agents/crypto/SignedEnvelope.java` around lines 64 - 73, forSigning currently stores the caller's payload Map by reference which breaks immutability and can change canonicalForm() after signing; update SignedEnvelope.forSigning to defensively copy and wrap the payload as an unmodifiable Map (e.g., Map.copyOf(payload) or Collections.unmodifiableMap(new LinkedHashMap<>(payload)) to preserve insertion order and allow nulls) and apply the same defensive-copy approach in any factory/transform method that accepts a payload (e.g., withSignature) so the record always holds an immutable copy used by canonicalForm().src/main/java/ai/labs/eddi/configs/agents/AgentSigningService.java (2)
252-281: ⚡ Quick win
signEnvelopecollapses all failure modes into a single wrapped exception.
sign(...)(line 119-141) deliberately distinguishesSecretNotFoundException("No signing key found for agent ...") from other failures.signEnvelopecatches the bareExceptionand produces the generic message "Envelope signing failed for agent ...", so callers can no longer tell a missing/rotated-out key from a genuine crypto/IO failure. This matters for operability when troubleshooting rotation issues.♻️ Match the error taxonomy used by `sign()`
- } catch (Exception e) { - throw new AgentSigningException("Envelope signing failed for agent " + agentId, e); - } + } catch (ISecretProvider.SecretNotFoundException e) { + throw new AgentSigningException( + "No signing key (v" + keyVersion + ") found for agent " + agentId, e); + } catch (Exception e) { + throw new AgentSigningException("Envelope signing failed for agent " + agentId, e); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/configs/agents/AgentSigningService.java` around lines 252 - 281, The signEnvelope method currently catches Exception and always throws a generic AgentSigningException; change its error handling to mirror sign() by detecting and rethrowing SecretNotFoundException with the specific "No signing key found for agent ..." message (or rethrow the original SecretNotFoundException) so callers can distinguish missing/rotated keys, and only wrap other exceptions into AgentSigningException with the existing "Envelope signing failed for agent <agentId>" message; locate this logic inside signEnvelope and use the same exception class and message text used by sign() to keep taxonomy consistent.
318-322: 🏗️ Heavy lift
rotateKeyhas no story for the previous key version.Rotation here just provisions a new version and returns its public key. There's no record of which version was active before, no grace period during which both
v(N-1)andvNaccept verifications, and no eventual cleanup of the superseded vault entry. In practice this means:
- In-flight envelopes signed with
v(N-1)cannot be verified once callers swap to the new public key (no overlap window).- Old private keys accumulate in the secret store indefinitely.
- There is no
listKeyVersions(agentId)API to audit what's live.Consider tracking active versions in
AgentConfiguration(active + previous with overlap deadline) and adding adeleteKeyPairVersioned(...)to be invoked after the deadline. Documenting the operator workflow in the rotation runbook would also help.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/configs/agents/AgentSigningService.java` around lines 318 - 322, rotateKey currently only creates a new key via generateKeyPairVersioned and returns it, leaving no record of the prior active version, no grace/overlap acceptance window, and no cleanup; modify rotateKey to update AgentConfiguration to store an activeVersion and previousVersion with an overlapDeadline timestamp, ensure verification logic accepts both activeVersion and previousVersion until overlapDeadline, add a deleteKeyPairVersioned(tenantId, agentId, version) method and schedule/trigger its call after the overlapDeadline to remove superseded private keys, and expose listKeyVersions(agentId) to report active/previous/expired versions for audit; update relevant methods named rotateKey, generateKeyPairVersioned, and AgentConfiguration to include these fields and behaviors and document the operator runbook for rotation workflow.src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java (1)
255-267: ⚡ Quick winUnguarded
RuntimeExceptionfromidentityMaskingServiceorcounterweightServicecan escape the pipelineThe catch block at
execute()line 198 handles onlyTemplateEngineException | UnsupportedLlmTaskException | IOException | LifecycleException. ARuntimeExceptionthrown by either newapply(...)call bypasses that catch entirely and propagates uncaught up the lifecycle stack. Per the task guidelines, exceptions must never kill the pipeline.🛡️ Proposed fix — wrap each call with graceful degradation
-systemMessage = identityMaskingService.apply(systemMessage, task.getIdentityMasking()); +try { + systemMessage = identityMaskingService.apply(systemMessage, task.getIdentityMasking()); +} catch (RuntimeException e) { + LOGGER.warnf(e, "[%s] IdentityMaskingService failed for task '%s'; using unmasked system message.", + sanitize(memory.getConversationId()), taskId); +} // ... channel-tag resolution ... -systemMessage = counterweightService.apply(systemMessage, task.getCounterweight(), channelTag); +try { + systemMessage = counterweightService.apply(systemMessage, task.getCounterweight(), channelTag); +} catch (RuntimeException e) { + LOGGER.warnf(e, "[%s] CounterweightService failed for task '%s'; continuing without counterweight.", + sanitize(memory.getConversationId()), taskId); +}As per coding guidelines: "Never let exceptions kill the pipeline — handle gracefully."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java` around lines 255 - 267, Wrap the two service calls identityMaskingService.apply(systemMessage, task.getIdentityMasking()) and counterweightService.apply(systemMessage, task.getCounterweight(), channelTag) in individual try/catch blocks that catch RuntimeException (or Exception) so an unexpected runtime error cannot escape execute(); on exception, log the error and continue using the pre-call systemMessage (or a safe fallback) so the pipeline degrades gracefully; use the existing logger and reference currentStep.getLatestData("channel:tag")/channelTag to preserve behavior when calling counterweightService.apply.src/main/java/ai/labs/eddi/modules/llm/impl/CounterweightService.java (1)
111-113: ⚡ Quick winAdd conversation context to Counterweight logs.
Both log lines are missing conversation context, which makes production traceability harder. Thread through a conversation identifier (or structured equivalent available at this layer) and include it in these log messages.
As per coding guidelines, "Include conversation context in log messages."
Also applies to: 172-173
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/modules/llm/impl/CounterweightService.java` around lines 111 - 113, Add the conversation context to the CounterweightService log statements: locate the debug calls in class CounterweightService (the lines logging "counterweight.strict.downgraded.scheduled" and the similar lines at 172-173) and include a conversation identifier from the available context (e.g., conversationId or a context.getConversationId()/getConversationContext() accessor provided at this layer) in the log message or as structured key/value (e.g., LOGGER.debug("counterweight.strict.downgraded.scheduled - conversationId={}", conversationId)). Keep the metric increments (strictDowngradedCounter.increment()) as-is but ensure the log entries immediately before/after the increment include the conversation id so traces can be correlated.src/main/java/ai/labs/eddi/modules/llm/impl/AttachmentForwarder.java (1)
62-63: ⚡ Quick winInclude
conversationIdin attachment forwarding logs.Both log statements omit conversation context even though
conversationIdis available in scope. Include it to make failures/audit trails actionable.As per coding guidelines, "Include conversation context in log messages."
Also applies to: 78-79
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/modules/llm/impl/AttachmentForwarder.java` around lines 62 - 63, AttachmentForwarder currently logs attachment load failures without conversation context; update the LOGGER.warnf calls to include the conversationId variable in the formatted message (e.g., include conversationId alongside attachment.storageRef()) and likewise include conversationId in the other warnf occurrence referenced (the block around the TextContent.from call). Also augment the TextContent.from message to include conversationId so the failure is traceable in content/audit trails; target the LOGGER.warnf and TextContent.from usages in the AttachmentForwarder class that reference attachment.storageRef() and attachment.filename() and add conversationId to their formatted strings.src/main/java/ai/labs/eddi/datastore/postgres/PostgresConversationCheckpointStore.java (1)
144-170: 💤 Low value
pruneOldestusesNOT INwith a correlated subquery — consider a CTE for correctness under concurrent insertsThe current
NOT IN (SELECT ... LIMIT ?)pattern is valid PostgreSQL, butNOT INwith a subquery that can returnNULLis a known foot-gun (if anycheckpoint_idin the subquery isNULL, the outer delete matches nothing). Thecheckpoint_idcolumn isPRIMARY KEYsoNULLis impossible here — this is safe. However, under concurrent checkpoint creation for the same conversation theLIMITsubquery reads a snapshot that may not include rows being inserted concurrently, potentially keeping fewer thankeepCountrows. This is a minor race (the next write triggers a follow-up prune), not data-loss.No change strictly required, but replacing with a
ROW_NUMBER()CTE would make the intent clearer and more resilient:♻️ Alternative using window function
WITH ranked AS ( SELECT checkpoint_id, ROW_NUMBER() OVER (PARTITION BY conversation_id ORDER BY created_at DESC) AS rn FROM conversation_checkpoints WHERE conversation_id = ? ) DELETE FROM conversation_checkpoints WHERE checkpoint_id IN (SELECT checkpoint_id FROM ranked WHERE rn > ?)(Bind
conversationIdonce,keepCountonce.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/datastore/postgres/PostgresConversationCheckpointStore.java` around lines 144 - 170, pruneOldest currently deletes using a NOT IN correlated subquery which can be fragile under concurrent inserts; replace the DELETE SQL in pruneOldest with a CTE that ranks checkpoints by created_at (e.g., use a WITH ranked AS (SELECT checkpoint_id, ROW_NUMBER() OVER (PARTITION BY conversation_id ORDER BY created_at DESC) AS rn FROM conversation_checkpoints WHERE conversation_id = ?) DELETE FROM conversation_checkpoints WHERE checkpoint_id IN (SELECT checkpoint_id FROM ranked WHERE rn > ?)) — bind conversationId once and keepCount once, update PreparedStatement parameter bindings accordingly in PostgresConversationCheckpointStore.pruneOldest and preserve existing logging/exception behavior.src/main/java/ai/labs/eddi/datastore/postgres/PostgresAttachmentStore.java (1)
61-71: 💤 Low value
ensureSchema()is correct but consider double-checked locking to avoid synchronization on every call after initCurrently, every method entry goes through the
synchronizedmethod. SinceschemaInitializedisvolatile, the standard double-checked locking pattern would avoid the lock overhead after first initialization.♻️ Proposed refactor
- private synchronized void ensureSchema() { - if (schemaInitialized) - return; + private void ensureSchema() { + if (schemaInitialized) return; + synchronized (this) { + if (schemaInitialized) return; try (Connection conn = dataSourceInstance.get().getConnection(); Statement stmt = conn.createStatement()) { stmt.execute(CREATE_TABLE); stmt.execute(CREATE_INDEX_CONV); schemaInitialized = true; } catch (SQLException e) { throw new RuntimeException("Failed to initialize attachments table", e); } + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/datastore/postgres/PostgresAttachmentStore.java` around lines 61 - 71, The synchronized ensureSchema() causes unnecessary locking after initialization; change it to use double-checked locking: make ensureSchema() non-synchronized, first check the volatile field schemaInitialized and return if true, then enter a synchronized(this) block and check schemaInitialized again before performing the initialization (getting Connection from dataSourceInstance.get().getConnection(), executing CREATE_TABLE and CREATE_INDEX_CONV, and setting schemaInitialized = true), and keep the same SQLException handling. This preserves thread-safety while avoiding the lock on every call.src/test/java/ai/labs/eddi/modules/llm/tools/PaginatedResponseStoreTest.java (1)
34-48: ⚡ Quick win
openMocks(this)result is never closedThe
AutoCloseablereturned byopenMocks(this)is discarded, so mock validation and cleanup are skipped after each test. This can suppressUnnecessaryStubbingExceptionand similar Mockito strictness checks.♻️ Proposed fix
+import org.junit.jupiter.api.AfterEach; ... + private AutoCloseable mocks; + `@BeforeEach` void setUp() { - openMocks(this); + mocks = openMocks(this); ... } + + `@AfterEach` + void tearDown() throws Exception { + mocks.close(); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/ai/labs/eddi/modules/llm/tools/PaginatedResponseStoreTest.java` around lines 34 - 48, The test's call to openMocks(this) discards the AutoCloseable, so mocks aren't closed; update PaginatedResponseStoreTest to keep the returned AutoCloseable from openMocks(this) (e.g., assign to a field like "autoCloseable" in setUp) and add an `@AfterEach` method that calls autoCloseable.close() to ensure Mockito releases mocks and enforces strict validation; reference the existing setUp method and openMocks call so you inject the stored AutoCloseable and close it after each test.src/main/java/ai/labs/eddi/modules/llm/impl/ToolResponseTruncator.java (1)
125-156:summarizeResponsealways falls back to truncation — offer to implementThe summarize path is a documented stub (TODO at Line 149). All execution paths within this method currently call
truncateResponseand emit asummarize_fallbackcounter, making the "summarize" strategy configuration silently degrade without user-visible indication other than a DEBUG log.Do you want me to open a tracking issue or draft the
ConversationSummarizerwiring for the actual summarize model call?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/modules/llm/impl/ToolResponseTruncator.java` around lines 125 - 156, The summarizeResponse method currently always falls back to truncateResponse; implement the actual summarization call: when limits.getSummarizerModel() is non-empty and result.length() <= 200_000, call the ConversationSummarizer (or ChatModelRegistry) to produce a concise summary (respecting maxChars), return that summary, and only call incrementCounter(toolName, "summarize_fallback") and truncateResponse(toolName, result, maxChars) in the catch block or when summarization is unavailable/too-large; ensure you log errors (LOGGER.warn/error) when summarizer fails and preserve existing debug logs for missing model or size checks so behavior is visible without always degrading silently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 304860ec-dd38-4d84-8d53-c57a31fbe9a9
📒 Files selected for processing (58)
docs/architecture.mddocs/changelog.mddocs/langchain.mdplanning/agentic-improvements-plan.mdsrc/main/java/ai/labs/eddi/configs/agents/AgentSigningService.javasrc/main/java/ai/labs/eddi/configs/agents/CapabilityRegistryService.javasrc/main/java/ai/labs/eddi/configs/agents/crypto/AgentPublicKey.javasrc/main/java/ai/labs/eddi/configs/agents/crypto/JacksonCanonicalizer.javasrc/main/java/ai/labs/eddi/configs/agents/crypto/NonceCacheService.javasrc/main/java/ai/labs/eddi/configs/agents/crypto/SignedEnvelope.javasrc/main/java/ai/labs/eddi/configs/agents/model/AgentConfiguration.javasrc/main/java/ai/labs/eddi/configs/agents/rest/RestAgentStore.javasrc/main/java/ai/labs/eddi/datastore/DataStoreProducers.javasrc/main/java/ai/labs/eddi/datastore/mongo/GridFsAttachmentStore.javasrc/main/java/ai/labs/eddi/datastore/postgres/PostgresAttachmentStore.javasrc/main/java/ai/labs/eddi/datastore/postgres/PostgresConversationCheckpointStore.javasrc/main/java/ai/labs/eddi/engine/a2a/RestA2AEndpoint.javasrc/main/java/ai/labs/eddi/engine/attachments/IAttachmentStore.javasrc/main/java/ai/labs/eddi/engine/attachments/MimeValidator.javasrc/main/java/ai/labs/eddi/engine/memory/IConversationCheckpointStore.javasrc/main/java/ai/labs/eddi/engine/memory/MemorySnapshotService.javasrc/main/java/ai/labs/eddi/engine/memory/MongoConversationCheckpointStore.javasrc/main/java/ai/labs/eddi/engine/memory/model/MemoryCheckpoint.javasrc/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.javasrc/main/java/ai/labs/eddi/modules/llm/impl/AttachmentForwarder.javasrc/main/java/ai/labs/eddi/modules/llm/impl/CounterweightService.javasrc/main/java/ai/labs/eddi/modules/llm/impl/IdentityMaskingService.javasrc/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.javasrc/main/java/ai/labs/eddi/modules/llm/impl/ToolResponseTruncator.javasrc/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.javasrc/main/java/ai/labs/eddi/modules/llm/tools/PaginatedResponseStore.javasrc/main/java/ai/labs/eddi/modules/llm/tools/impl/DiscoverToolsTool.javasrc/main/java/ai/labs/eddi/modules/llm/tools/impl/FetchToolResponsePageTool.javasrc/main/java/ai/labs/eddi/modules/rules/impl/RuleDeserialization.javasrc/main/java/ai/labs/eddi/modules/rules/impl/conditions/CapabilityMatchCondition.javasrc/main/java/ai/labs/eddi/modules/rules/impl/conditions/DeploymentContextCondition.javasrc/test/java/ai/labs/eddi/configs/agents/AgentSigningServiceTest.javasrc/test/java/ai/labs/eddi/configs/agents/CapabilityRegistryServiceTest.javasrc/test/java/ai/labs/eddi/configs/agents/crypto/AgentPublicKeyTest.javasrc/test/java/ai/labs/eddi/configs/agents/crypto/JacksonCanonicalizerTest.javasrc/test/java/ai/labs/eddi/configs/agents/crypto/NonceCacheServiceTest.javasrc/test/java/ai/labs/eddi/configs/agents/crypto/SignedEnvelopeTest.javasrc/test/java/ai/labs/eddi/configs/agents/model/SessionManagementTest.javasrc/test/java/ai/labs/eddi/configs/agents/rest/RestAgentStoreTest.javasrc/test/java/ai/labs/eddi/engine/attachments/MimeValidatorTest.javasrc/test/java/ai/labs/eddi/engine/memory/MemorySnapshotServiceTest.javasrc/test/java/ai/labs/eddi/engine/memory/model/MemoryCheckpointTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/AttachmentForwarderTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/CounterweightServiceTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/IdentityMaskingServiceTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/ToolResponseTruncatorExtendedTest.javasrc/test/java/ai/labs/eddi/modules/llm/tools/PaginatedResponseStoreTest.javasrc/test/java/ai/labs/eddi/modules/llm/tools/impl/DiscoverToolsToolTest.javasrc/test/java/ai/labs/eddi/modules/llm/tools/impl/FetchToolResponsePageToolTest.javasrc/test/java/ai/labs/eddi/modules/rules/impl/conditions/CapabilityMatchConditionTest.javasrc/test/java/ai/labs/eddi/modules/rules/impl/conditions/DeploymentContextConditionTest.java
Code quality (CodeQL + Copilot): - JacksonCanonicalizer: rename canonicalize(Object) to canonicalizeObject() to eliminate overload ambiguity - DiscoverToolsTool: use proper escapeJson() for all interpolated fields (name, description) - FetchToolResponsePageTool: apply escapeJson() to error and toolName fields - DeploymentContextCondition: clear fields on null configs to prevent stale state on reuse - CapabilityMatchCondition: fix stepIndex off-by-one (size() -> size()-1 for 0-based consistency) - CapabilityRegistryService: extract lookupBySkill() to avoid double-counting strategy metrics Javadoc accuracy: - LlmConfiguration.summarizerModel: remove phantom config-property default claim - MemorySnapshotService.rollbackToCheckpoint: state only properties are restored, not step stack - MemoryCheckpoint: class-level doc now accurately describes stored state (stepIndex + properties) Documentation: - langchain.md: auto-downgrade only applies to 'scheduled' channel, not 'batch' Tests: - RestAgentStoreTest: replace brittle NPE assertions with proper agentStore.create() mocking - CapabilityMatchConditionTest: update stepIndex assertion to match 0-based fix
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (6)
src/main/java/ai/labs/eddi/modules/llm/tools/impl/DiscoverToolsTool.java (2)
71-71:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
totalAvailablestill includes the self-excluded meta-toolAt Line 71 and Line 86,
totalAvailableusesallToolSpecs.size(), but the method excludes"discover_tools"from results. This reports an inflated count.Proposed fix
+ long discoverableCount = allToolSpecs.stream() + .filter(spec -> !"discover_tools".equals(spec.name())) + .count(); + if (matches.isEmpty()) { - return "{\"tools\": [], \"message\": \"No tools found matching the criteria.\", \"totalAvailable\": " + allToolSpecs.size() + "}"; + return "{\"tools\": [], \"message\": \"No tools found matching the criteria.\", \"totalAvailable\": " + discoverableCount + "}"; } ... - sb.append(", \"totalAvailable\": ").append(allToolSpecs.size()).append("}"); + sb.append(", \"totalAvailable\": ").append(discoverableCount).append("}");Also applies to: 86-86
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/modules/llm/tools/impl/DiscoverToolsTool.java` at line 71, The returned totalAvailable currently uses allToolSpecs.size() which inflates the count because the method excludes the meta-tool "discover_tools"; update both return points in DiscoverToolsTool (the method that produces the JSON with tools/message/totalAvailable) to compute totalAvailable from the filtered collection you actually return (e.g., the list after removing "discover_tools" — use that filtered list's size instead of allToolSpecs.size()) so the reported total matches the returned results.
111-117:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeyword splitting can match every tool when input starts with whitespace
At Line 111,
split("\\s+")on untrimmed input can produce an empty term (e.g.," search"), andname.contains("")is always true, so filtering breaks.Proposed fix
- String[] terms = keywords.toLowerCase().split("\\s+"); + String[] terms = keywords.trim().toLowerCase().split("\\s+");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/modules/llm/tools/impl/DiscoverToolsTool.java` around lines 111 - 117, The keyword-splitting logic in DiscoverToolsTool (where `keywords.toLowerCase().split("\\s+")` is used to produce `terms`) can produce empty strings when `keywords` has leading/trailing whitespace, causing `name.contains("")` to always match; fix by trimming and removing empty tokens before checking: compute the terms from `keywords.trim()`, lower-case and split, then filter out any zero-length strings (or use a stream/filter) so the loop over `terms` only checks non-empty `term` values when testing `name.contains(term)` or `desc.contains(term)`.src/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.java (1)
1223-1340: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftConvert new configuration POJOs to records for immutability
ToolLoadingStrategy,CounterweightConfig, andIdentityMaskingConfigare newly added mutable config POJOs with setters. They should berecords in this file to satisfy the repo rule.Proposed direction
- public static class ToolLoadingStrategy { - private String type = "eager"; - private int maxToolsInContext = 20; - private boolean discoveryToolEnabled = true; - // getters/setters... - } + public static record ToolLoadingStrategy( + String type, + int maxToolsInContext, + boolean discoveryToolEnabled) { + public ToolLoadingStrategy() { + this("eager", 20, true); + } + }Apply the same pattern to
CounterweightConfigandIdentityMaskingConfig, preserving current defaults.As per coding guidelines: "Configuration POJOs for new features MUST use Java records for immutability."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.java` around lines 1223 - 1340, Replace the mutable classes ToolLoadingStrategy, CounterweightConfig, and IdentityMaskingConfig with Java records to enforce immutability: declare records ToolLoadingStrategy(String type,int maxToolsInContext,boolean discoveryToolEnabled), CounterweightConfig(boolean enabled,String level,String placement,List<String> customInstructions), and IdentityMaskingConfig(boolean enabled,List<String> rules); remove all setters and mutable fields, add compact/canonical constructors to apply the current defaults ("eager",20,true for ToolLoadingStrategy; false,"normal","suffix",null for CounterweightConfig; false,new ArrayList<>() for IdentityMaskingConfig) and null-checks/defensive copies for lists, and update any call sites that used setter methods or JavaBean getters (isEnabled()/getLevel()/getRules()) to use the record accessors (enabled(), level(), rules(), etc.).src/main/java/ai/labs/eddi/engine/memory/MemorySnapshotService.java (1)
145-160:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve
Long/Doubleproperty types during rollback
LongandDoublevalues currently fall into the string fallback path, which mutates property types after restore.🐛 Proposed fix
propertiesCopy.forEach((key, value) -> { Property property; if (value instanceof String s) { property = new Property(key, s, Property.Scope.conversation); } else if (value instanceof Map<?, ?> m) { property = new Property(key, (Map<String, Object>) m, Property.Scope.conversation); } else if (value instanceof List<?> l) { property = new Property(key, (List<Object>) l, Property.Scope.conversation); } else if (value instanceof Integer i) { property = new Property(key, i, Property.Scope.conversation); + } else if (value instanceof Long l) { + property = new Property(key, l, Property.Scope.conversation); } else if (value instanceof Float f) { property = new Property(key, f, Property.Scope.conversation); + } else if (value instanceof Double d) { + property = new Property(key, d, Property.Scope.conversation); } else if (value instanceof Boolean b) { property = new Property(key, b, Property.Scope.conversation); } else { // Fallback: convert to string property = new Property(key, String.valueOf(value), Property.Scope.conversation);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/engine/memory/MemorySnapshotService.java` around lines 145 - 160, In MemorySnapshotService where the snapshot restore builds Property objects (the branch that assigns to variable property), add explicit checks for Long and Double (e.g., value instanceof Long l and value instanceof Double d) before the fallback so Longs and Doubles are passed into new Property(key, l, Property.Scope.conversation) and new Property(key, d, Property.Scope.conversation) respectively; this preserves numeric types during rollback instead of converting them to strings.src/main/java/ai/labs/eddi/modules/rules/impl/conditions/CapabilityMatchCondition.java (2)
201-203:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
LOGGER.debugfat line 202 is missing conversation context (coding guideline violation).The audit-failure log message omits the conversation ID.
memoryis in scope.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/modules/rules/impl/conditions/CapabilityMatchCondition.java` around lines 201 - 203, Update the exception log in CapabilityMatchCondition so the LOGGER.debugf call includes the conversation ID from the in-scope memory object; replace the current message that only logs e.getMessage() with one that also appends the conversation identifier (e.g., memory.getConversationId() or the appropriate memory accessor) so the audit-failure log includes both the error and conversation context in the LOGGER.debugf invocation.
196-196:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
matchedAgentIds.get(0)still unguarded whenmatchedAgentIdsis empty.This issue from the previous review remains unaddressed. When
minResultsis configured to0,matches.size() >= 0is alwaystrue,matchedAgentIdsis empty, andmatchedAgentIds.get(0)throwsIndexOutOfBoundsException— silently swallowed by the surroundingcatch, producing a phantom SUCCESS with no audit output.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/modules/rules/impl/conditions/CapabilityMatchCondition.java` at line 196, The code in CapabilityMatchCondition uses matchedAgentIds.get(0) without checking for emptiness, causing IndexOutOfBoundsException when matchedAgentIds is empty (e.g., minResults == 0); update the logic in the method that builds the audit/context (the block referencing "selectedAgentId" and matchedAgentIds) to first check matchedAgentIds.isEmpty() and only add "selectedAgentId" when a value exists (or set a clear placeholder like null/"none"), and ensure the overall success outcome uses matches.size() >= minResults (or explicitly checks matchedAgentIds.size() >= minResults) so you do not treat an empty match list as a silent SUCCESS; locate references to matchedAgentIds.get(0) in CapabilityMatchCondition and guard them with an if (!matchedAgentIds.isEmpty()) branch that populates audit data conditionally.
🧹 Nitpick comments (2)
src/main/java/ai/labs/eddi/engine/memory/MemorySnapshotService.java (1)
89-99: ⚡ Quick winInclude conversation context and align failure log level in rollback failure paths
The not-found rollback log omits conversation context, and rollback failures should use the failure-level log guidance consistently.
📝 Proposed fix
MemoryCheckpoint checkpoint = checkpointStore.findById(checkpointId); if (checkpoint == null) { - LOGGER.warnf("Checkpoint '%s' not found for rollback", checkpointId); + LOGGER.errorf("Checkpoint '%s' not found for rollback in conversation '%s'", + checkpointId, memory.getConversationId()); incrementCounter("rollback_failed"); return false; } // Verify this checkpoint belongs to the same conversation if (!checkpoint.conversationId().equals(memory.getConversationId())) { - LOGGER.warnf("Checkpoint '%s' belongs to conversation '%s', not '%s'", + LOGGER.errorf("Checkpoint '%s' belongs to conversation '%s', not '%s'", checkpointId, checkpoint.conversationId(), memory.getConversationId()); incrementCounter("rollback_failed"); return false; }As per coding guidelines, "Use JBoss Logger for logging, not System.out. Include conversation context in log messages. Use appropriate levels: DEBUG (verbose), INFO (important events), ERROR (failures)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/engine/memory/MemorySnapshotService.java` around lines 89 - 99, The rollback failure paths currently log with LOGGER.warnf and the "checkpoint not found" message lacks conversation context; update both failure logs to use LOGGER.errorf and include the conversation id and checkpoint id for context. Specifically, replace the LOGGER.warnf("Checkpoint '%s' not found for rollback", checkpointId) call with an error-level message that includes checkpointId and memory.getConversationId(), and change the existing LOGGER.warnf(...) in the conversation-mismatch branch to LOGGER.errorf(...) while keeping the same formatted arguments checkpointId, checkpoint.conversationId(), memory.getConversationId(); keep the incrementCounter("rollback_failed") calls unchanged.src/test/java/ai/labs/eddi/modules/rules/impl/conditions/CapabilityMatchConditionTest.java (1)
222-241: ⚡ Quick winPrefer
ArgumentCaptoroverassertEqualsinsideargThat.Throwing
AssertionErrorinsideargThatcauses Mockito to treat the matcher as non-matching. The test will then fail with a confusing "Wanted but not invoked" verification error instead of a pinpointed assertion failure, making debugging harder.♻️ Proposed refactor using ArgumentCaptor
- verify(auditCollector).collect(argThat(entry -> { - assertEquals("conv-123", entry.conversationId()); - assertEquals("agent-owner", entry.agentId()); - assertEquals(2, entry.agentVersion()); - assertEquals("user-456", entry.userId()); - assertEquals("capabilityMatch", entry.taskId()); - assertEquals("CAPABILITY_SELECTION", entry.taskType()); - assertEquals(2, entry.stepIndex()); // 0-based: allSteps.size() - 1 - - // Check input contains skill and strategy - assertNotNull(entry.input()); - assertEquals("coding", entry.input().get("skill")); - assertEquals("all", entry.input().get("strategy")); - - // Check output contains selectedAgentId - assertNotNull(entry.output()); - assertEquals("agent-1", entry.output().get("selectedAgentId")); - - return true; - })); + var captor = ArgumentCaptor.forClass(ai.labs.eddi.engine.audit.model.AuditEntry.class); + verify(auditCollector).collect(captor.capture()); + var entry = captor.getValue(); + + assertEquals("conv-123", entry.conversationId()); + assertEquals("agent-owner", entry.agentId()); + assertEquals(2, entry.agentVersion()); + assertEquals("user-456", entry.userId()); + assertEquals("capabilityMatch", entry.taskId()); + assertEquals("CAPABILITY_SELECTION", entry.taskType()); + assertEquals(2, entry.stepIndex()); // 0-based: allSteps.size() - 1 + assertNotNull(entry.input()); + assertEquals("coding", entry.input().get("skill")); + assertEquals("all", entry.input().get("strategy")); + assertNotNull(entry.output()); + assertEquals("agent-1", entry.output().get("selectedAgentId"));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/ai/labs/eddi/modules/rules/impl/conditions/CapabilityMatchConditionTest.java` around lines 222 - 241, Replace the inline assertions inside the verify(argThat(...)) matcher with an ArgumentCaptor for the AuditEntry passed to auditCollector.collect so test failures produce clear assertion errors; specifically, in CapabilityMatchConditionTest capture the argument from auditCollector.collect using ArgumentCaptor<AuditEntry> (or the actual entry type), then perform assertEquals/assertNotNull assertions against captor.getValue() for conversationId, agentId, agentVersion, userId, taskId, taskType, stepIndex, input.skill, input.strategy, and output.selectedAgentId instead of asserting inside argThat.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/main/java/ai/labs/eddi/engine/memory/MemorySnapshotService.java`:
- Around line 145-160: In MemorySnapshotService where the snapshot restore
builds Property objects (the branch that assigns to variable property), add
explicit checks for Long and Double (e.g., value instanceof Long l and value
instanceof Double d) before the fallback so Longs and Doubles are passed into
new Property(key, l, Property.Scope.conversation) and new Property(key, d,
Property.Scope.conversation) respectively; this preserves numeric types during
rollback instead of converting them to strings.
In `@src/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.java`:
- Around line 1223-1340: Replace the mutable classes ToolLoadingStrategy,
CounterweightConfig, and IdentityMaskingConfig with Java records to enforce
immutability: declare records ToolLoadingStrategy(String type,int
maxToolsInContext,boolean discoveryToolEnabled), CounterweightConfig(boolean
enabled,String level,String placement,List<String> customInstructions), and
IdentityMaskingConfig(boolean enabled,List<String> rules); remove all setters
and mutable fields, add compact/canonical constructors to apply the current
defaults ("eager",20,true for ToolLoadingStrategy; false,"normal","suffix",null
for CounterweightConfig; false,new ArrayList<>() for IdentityMaskingConfig) and
null-checks/defensive copies for lists, and update any call sites that used
setter methods or JavaBean getters (isEnabled()/getLevel()/getRules()) to use
the record accessors (enabled(), level(), rules(), etc.).
In `@src/main/java/ai/labs/eddi/modules/llm/tools/impl/DiscoverToolsTool.java`:
- Line 71: The returned totalAvailable currently uses allToolSpecs.size() which
inflates the count because the method excludes the meta-tool "discover_tools";
update both return points in DiscoverToolsTool (the method that produces the
JSON with tools/message/totalAvailable) to compute totalAvailable from the
filtered collection you actually return (e.g., the list after removing
"discover_tools" — use that filtered list's size instead of allToolSpecs.size())
so the reported total matches the returned results.
- Around line 111-117: The keyword-splitting logic in DiscoverToolsTool (where
`keywords.toLowerCase().split("\\s+")` is used to produce `terms`) can produce
empty strings when `keywords` has leading/trailing whitespace, causing
`name.contains("")` to always match; fix by trimming and removing empty tokens
before checking: compute the terms from `keywords.trim()`, lower-case and split,
then filter out any zero-length strings (or use a stream/filter) so the loop
over `terms` only checks non-empty `term` values when testing
`name.contains(term)` or `desc.contains(term)`.
In
`@src/main/java/ai/labs/eddi/modules/rules/impl/conditions/CapabilityMatchCondition.java`:
- Around line 201-203: Update the exception log in CapabilityMatchCondition so
the LOGGER.debugf call includes the conversation ID from the in-scope memory
object; replace the current message that only logs e.getMessage() with one that
also appends the conversation identifier (e.g., memory.getConversationId() or
the appropriate memory accessor) so the audit-failure log includes both the
error and conversation context in the LOGGER.debugf invocation.
- Line 196: The code in CapabilityMatchCondition uses matchedAgentIds.get(0)
without checking for emptiness, causing IndexOutOfBoundsException when
matchedAgentIds is empty (e.g., minResults == 0); update the logic in the method
that builds the audit/context (the block referencing "selectedAgentId" and
matchedAgentIds) to first check matchedAgentIds.isEmpty() and only add
"selectedAgentId" when a value exists (or set a clear placeholder like
null/"none"), and ensure the overall success outcome uses matches.size() >=
minResults (or explicitly checks matchedAgentIds.size() >= minResults) so you do
not treat an empty match list as a silent SUCCESS; locate references to
matchedAgentIds.get(0) in CapabilityMatchCondition and guard them with an if
(!matchedAgentIds.isEmpty()) branch that populates audit data conditionally.
---
Nitpick comments:
In `@src/main/java/ai/labs/eddi/engine/memory/MemorySnapshotService.java`:
- Around line 89-99: The rollback failure paths currently log with LOGGER.warnf
and the "checkpoint not found" message lacks conversation context; update both
failure logs to use LOGGER.errorf and include the conversation id and checkpoint
id for context. Specifically, replace the LOGGER.warnf("Checkpoint '%s' not
found for rollback", checkpointId) call with an error-level message that
includes checkpointId and memory.getConversationId(), and change the existing
LOGGER.warnf(...) in the conversation-mismatch branch to LOGGER.errorf(...)
while keeping the same formatted arguments checkpointId,
checkpoint.conversationId(), memory.getConversationId(); keep the
incrementCounter("rollback_failed") calls unchanged.
In
`@src/test/java/ai/labs/eddi/modules/rules/impl/conditions/CapabilityMatchConditionTest.java`:
- Around line 222-241: Replace the inline assertions inside the
verify(argThat(...)) matcher with an ArgumentCaptor for the AuditEntry passed to
auditCollector.collect so test failures produce clear assertion errors;
specifically, in CapabilityMatchConditionTest capture the argument from
auditCollector.collect using ArgumentCaptor<AuditEntry> (or the actual entry
type), then perform assertEquals/assertNotNull assertions against
captor.getValue() for conversationId, agentId, agentVersion, userId, taskId,
taskType, stepIndex, input.skill, input.strategy, and output.selectedAgentId
instead of asserting inside argThat.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 07369791-7633-4d90-ac2c-ca0153169bb1
📒 Files selected for processing (14)
docs/changelog.mddocs/langchain.mdsrc/main/java/ai/labs/eddi/configs/agents/CapabilityRegistryService.javasrc/main/java/ai/labs/eddi/configs/agents/crypto/JacksonCanonicalizer.javasrc/main/java/ai/labs/eddi/configs/agents/crypto/SignedEnvelope.javasrc/main/java/ai/labs/eddi/engine/memory/MemorySnapshotService.javasrc/main/java/ai/labs/eddi/engine/memory/model/MemoryCheckpoint.javasrc/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.javasrc/main/java/ai/labs/eddi/modules/llm/tools/impl/DiscoverToolsTool.javasrc/main/java/ai/labs/eddi/modules/llm/tools/impl/FetchToolResponsePageTool.javasrc/main/java/ai/labs/eddi/modules/rules/impl/conditions/CapabilityMatchCondition.javasrc/main/java/ai/labs/eddi/modules/rules/impl/conditions/DeploymentContextCondition.javasrc/test/java/ai/labs/eddi/configs/agents/rest/RestAgentStoreTest.javasrc/test/java/ai/labs/eddi/modules/rules/impl/conditions/CapabilityMatchConditionTest.java
✅ Files skipped from review due to trivial changes (4)
- docs/langchain.md
- src/main/java/ai/labs/eddi/engine/memory/model/MemoryCheckpoint.java
- src/main/java/ai/labs/eddi/configs/agents/crypto/SignedEnvelope.java
- docs/changelog.md
🚧 Files skipped from review as they are similar to previous changes (4)
- src/test/java/ai/labs/eddi/configs/agents/rest/RestAgentStoreTest.java
- src/main/java/ai/labs/eddi/configs/agents/crypto/JacksonCanonicalizer.java
- src/main/java/ai/labs/eddi/configs/agents/CapabilityRegistryService.java
- src/main/java/ai/labs/eddi/modules/rules/impl/conditions/DeploymentContextCondition.java
- LlmTaskTest: add null MemorySnapshotService + IAttachmentStore - AgentOrchestratorTest: add null MemorySnapshotService - MultimodalMessageEnhancer*Test: add null IAttachmentStore - GroupConversationServiceTest: add null AgentSigningService + IAgentStore - RestAttachmentUploadTest: rewrite for IAttachmentStore API (10 tests) - RestAttachmentUpload: fix filename() accessor on Attachment record - DiscoverToolsToolTest: accept Jackson compact JSON format - FetchToolResponsePageToolTest: accept Jackson compact JSON format - MultimodalMessageEnhancerExtendedTest: update STORED path assertion 264 targeted tests pass, 0 failures, 0 checkstyle violations
…ry snapshots Production integration wiring for agentic pipeline stabilization: - AgentSigningService: ConcurrentHashMap PrivateKey caching, atomic eviction - GroupConversationService: inject AgentSigningService + IAgentStore for signed inter-agent turns with backward-compatible TranscriptEntry - RestAttachmentUpload: refactor to IAttachmentStore-only pipeline - MultimodalMessageEnhancer: implement STORED path with IAttachmentStore load, base64 data URI forwarding, graceful null-store fallback - AgentOrchestrator: inject MemorySnapshotService for checkpoint support - LlmTask: accept MemorySnapshotService + IAttachmentStore dependencies - DiscoverToolsTool: Jackson serialization, discoveredToolNames tracking - FetchToolResponsePageTool: Jackson serialization via toJson() helper - CounterweightService: PromptSnippetService snippet-first preset resolution - RestAgentStore: strict AgentIdentity validation (publicKey required) - PostgresAttachmentStore/CheckpointStore: checkstyle compliance fixes - MongoConversationCheckpointStore: remove unused import - MimeValidator/MemoryCheckpoint: minor cleanups
…verage Dead code removed (10 files): - AttachmentForwarder + test (never injected) - NonceCacheService + test (never injected) - SignedEnvelope + test (envelope API never called) - JacksonCanonicalizer + test (only consumer was SignedEnvelope) - DiscoverToolsTool + test (never instantiated by orchestrator) Dead code removed (from live files): - AgentSigningService: signEnvelope/verifyEnvelope/rotateKey - LlmConfiguration: ToolLoadingStrategy inner class + field - AgentSigningServiceTest: 5 tests for deleted methods Bug fix: - DeepCopyUtil.deepCopy() now returns unmodifiable map - DeepCopyUtil.java added to Git (was untracked) Documentation: - architecture.md: replaced AttachmentForwarder with MultimodalMessageEnhancer - ToolResponseTruncator: summarize fallback log upgraded to WARN Coverage: - DeepCopyUtilTest (NEW, 8 tests) - DeploymentContextConditionTest (4 new edge cases) Verified: compile OK, 350 targeted tests pass (0 failures)
AgentSigningServiceTest: add vault-failure and key-not-found error paths - generateKeyPair: SecretProviderException catch block - generateKeyPairVersioned: SecretProviderException catch block - sign: SecretNotFoundException unwrap path Coverage: 86.5% → 91.5% RestAttachmentUploadTest: add IOException error path - Upload with nonexistent file path triggers 500 response Coverage: 88.5% → 98.7% All 15 tracked classes now ≥90.8% (354 tests, 0 failures)
…e double deep-copy, remove dead code - MemoryCheckpoint.propertiesCopy changed from Map<String, Object> to Map<String, Property> — scope, visibility, and all value types are now preserved through checkpoint/rollback cycle - Eliminated double deep-copy: extractProperties() returns shallow copy, MemoryCheckpoint.create() handles the single deep-copy via copyProperties() - Removed dead generateKeyPairVersioned() + vaultKeyNameVersioned() from AgentSigningService (only caller was deleted rotateKey()) - Removed redundant import in DeepCopyUtil - Added 3 new checkpoint tests (scope, visibility, mutation isolation) - Updated rollback test to assert scope preservation
…ontext inheritance Wire the 'summarize' tool-response truncation strategy to ChatModelRegistry, replacing the WARN stub with a functional LLM summarization pipeline. Key design: the summarizer inherits the parent task's provider type, API key, baseUrl, and other config — only overriding modelName with summarizerModel. This avoids the API key gap in SummarizationService (which only passes modelName) and requires zero new config fields. Fallback chain: no model -> no task context -> cost ceiling (200K chars) -> model/LLM failure -> empty summary -> summary-longer-than-limit -> truncate. Security: all logged user/LLM inputs sanitized via LogSanitizer. Safety: responseFormat stripped from summarizer params (plain text output). 37 tests covering all strategies, all fallback paths, API key inheritance, parameter immutability, responseFormat stripping, and case-insensitive strategy selection.
ginccc
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
src/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.java (1)
1205-1269:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftConvert new config POJOs to records for immutability.
CounterweightConfigandIdentityMaskingConfigare newly introduced mutable configuration classes; they should be records.As per coding guidelines, "src/main/java/**/*Configuration.java: Configuration POJOs for new features MUST use Java records for immutability."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.java` around lines 1205 - 1269, Replace the mutable classes CounterweightConfig and IdentityMaskingConfig with Java records to enforce immutability: change the class declarations to record CounterweightConfig(boolean enabled, String level, String placement, List<String> customInstructions) and record IdentityMaskingConfig(boolean enabled, List<String> rules) and remove all setters/getters; add compact constructors (or explicit canonical constructors) that apply the current defaults (enabled -> false, level -> "normal", placement -> "suffix", rules -> new ArrayList<>(), customInstructions -> null or empty list as desired) and defensive-copy/normalize collection inputs (e.g., Collections.unmodifiableList or new ArrayList<>(list) when non-null) so behavior and defaults of the original methods (isEnabled/getLevel/getPlacement/getCustomInstructions/getRules) are preserved via the record component accessors.
🧹 Nitpick comments (3)
src/main/java/ai/labs/eddi/configs/agents/AgentSigningService.java (1)
56-56: ⚖️ Poor tradeoffConsider adding cache eviction policy to prevent unbounded growth.
The
privateKeyCachehas no size limit or TTL. In a multi-tenant system with many agents, this cache will grow indefinitely unless agents are explicitly deleted viadeleteKeyPair(). Key rotation scenarios (where new keys are generated without deleting old agent entries) will also accumulate stale entries.Consider adding a size-bounded cache (e.g., Caffeine with maximum size + LRU eviction) or a TTL-based eviction policy.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/configs/agents/AgentSigningService.java` at line 56, The privateKeyCache field in AgentSigningService is an unbounded ConcurrentHashMap causing potential unbounded growth; replace it with a bounded cache supporting eviction/TTL (e.g., Caffeine or Guava Cache) configured with a sensible maximumSize and either expireAfterAccess/expireAfterWrite to remove stale keys, and update usages of privateKeyCache.get/put/remove to the cache API while preserving deleteKeyPair() behavior to invalidate entries; alternatively, implement a scheduled cleanup task that evicts entries based on last-access time if adding a third‑party cache is not allowed.src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java (1)
596-597: ⚖️ Poor tradeoffAgent configuration is read on every turn without caching.
The agent configuration is loaded from the store on every agent turn (lines 596-597) to check the
signInterAgentMessagesflag. In a group discussion with multiple rounds and agents, this results in repeated database reads for the same agent configuration.Consider caching agent configurations at the start of
executeDiscussion()or using a request-scoped cache to avoid repeated store reads.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java` around lines 596 - 597, The code reads agent configuration on every agent turn using agentStore.getCurrentResourceId(member.agentId()) and agentStore.read(... ) to check signInterAgentMessages; modify executeDiscussion() in GroupConversationService to preload and cache each agent's configuration (e.g., Map keyed by member.agentId()) at the start of the discussion or use a request-scoped cache, then replace per-turn calls to agentStore.getCurrentResourceId/agentStore.read with lookups against that cache so signInterAgentMessages is read once per agent per discussion.src/test/java/ai/labs/eddi/engine/memory/rest/RestAttachmentUploadTest.java (1)
67-87: ⚡ Quick winUse test-managed temp file lifecycle for upload tests.
These methods manually delete temp files at the end; if an assertion fails earlier, cleanup is skipped. Prefer
@TempDirortry/finallyfor deterministic cleanup.Proposed refactor pattern
+import org.junit.jupiter.api.io.TempDir; ... class RestAttachmentUploadTest { + `@TempDir` + Path tempDir; ... - Path tempFile = Files.createTempFile("test-upload", ".png"); + Path tempFile = tempDir.resolve("test-upload.png"); Files.write(tempFile, new byte[42]); ... - Files.deleteIfExists(tempFile);Also applies to: 99-115, 124-141, 153-170
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/ai/labs/eddi/engine/memory/rest/RestAttachmentUploadTest.java` around lines 67 - 87, The test in RestAttachmentUploadTest creates temp files (tempFile) and currently deletes them with Files.deleteIfExists(...) at the end, which can leak if assertions fail; update the uploadAttachment tests to use a test-managed temp directory or guaranteed cleanup: either add a JUnit `@TempDir` parameter and create temp files there or wrap the temp file creation + test call in a try/finally and delete the file in the finally block; apply this pattern for all methods that create tempFile (the blocks calling uploadAttachment(...), tempFile, Files.write(...), and Files.deleteIfExists(...)) so cleanup always runs even on assertion failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/changelog.md`:
- Line 347: The changelog has contradictory wording about CounterweightService
strict preset auto-downgrading (mentions both "scheduled" and
"scheduled/batch"); pick one canonical behavior and make the text consistent
across all occurrences (references to CounterweightService and its presets) —
update the entries that mention strict auto-downgrade so they uniformly state
either "scheduled" or "scheduled/batch", or explicitly mark earlier lines as
historical pre-fix, and ensure the explanatory note about strict->cautious
behavior and the exception for custom instructions is consistent in each
mention.
In `@src/main/java/ai/labs/eddi/engine/memory/rest/RestAttachmentUpload.java`:
- Around line 137-144: The deleteAttachments method currently performs blocking
work on the request thread by calling
attachmentStore.deleteByConversation(conversationId); refactor deleteAttachments
to accept a javax.ws.rs.container.AsyncResponse (e.g., change signature to
deleteAttachments(`@Suspended` AsyncResponse asyncResponse,
`@PathParam`("conversationId") String conversationId)), run the deletion
asynchronously (use an executor or CompletableFuture) and resume the
AsyncResponse with Response.ok(Map.of("conversationId", conversationId,
"deletedCount", deleted)).build() on success or asyncResponse.resume(...) with
an appropriate error Response on failure; keep method name deleteAttachments and
the attachmentStore.deleteByConversation call but move its invocation into the
async task.
- Around line 122-127: The listAttachments method in RestAttachmentUpload
currently blocks on attachmentStore.listByConversation; change its signature to
accept a `@Suspended` AsyncResponse asyncResponse and perform the DB call
asynchronously (e.g., CompletableFuture.supplyAsync or an injected Executor) to
call attachmentStore.listByConversation(conversationId) off the request thread,
then on completion resume the AsyncResponse with
Response.ok(attachments).build(); on failure resume with an appropriate error
Response (e.g., server error with exception message). Ensure you reference
listAttachments, attachmentStore.listByConversation and AsyncResponse in your
changes and don’t block the JAX-RS request thread.
- Around line 79-82: RestAttachmentUpload currently calls
Files.readAllBytes(file.uploadedFile()) which can OOM on large or concurrent
uploads; replace the eager byte[] read with a streaming approach by passing an
InputStream (e.g., Files.newInputStream(file.uploadedFile()) or
file.uploadedFile().toFile() stream) into attachmentStore.store (add/use an
overload that accepts InputStream, mimeType, fileName, conversationId, tenantId)
or, if changing attachmentStore is not possible, first validate
Files.size(file.uploadedFile()) against a safe limit and fail early; update
RestAttachmentUpload to stream the file into attachmentStore.store or perform
size validation instead of reading all bytes into memory.
- Around line 62-67: The uploadAttachment REST method is performing blocking I/O
(Files.readAllBytes and attachmentStore.store) on the request thread; change the
method signature to accept a JAX-RS `@Suspended` AsyncResponse and move the
blocking work into a CompletableFuture (or dedicated executor) so the request
thread is released, perform file bytes reading and attachmentStore.store(...)
inside that async task, and then resume the AsyncResponse with the Response
(success or error) on completion or exceptionally; reference the
uploadAttachment method, the Files.readAllBytes call and attachmentStore.store
call when locating code to change.
In `@src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java`:
- Around line 257-267: The log call in AgentOrchestrator (inside the
counterweight strict-mode block using variables maxIterations and strictCap and
LOGGER.infof) omits conversation context and does not sanitize dynamic fields;
update the LOGGER.infof invocation(s) around this block (and the related log at
lines ~284-293) to include the conversation context (e.g., conversationId and
tenantId retrieved from the current Task/Conversation object) and ensure any
dynamic values are sanitized/validated before logging (escape or redact PII/long
strings), so the message becomes: include conversationId/tenantId and the capped
values while sanitizing inputs.
In `@src/main/java/ai/labs/eddi/modules/llm/impl/MultimodalMessageEnhancer.java`:
- Around line 176-197: In MultimodalMessageEnhancer, update the
stored-attachment log lines to include the conversation context: when
attachmentStore is null, when load succeeds (LOGGER.debugf) and when load fails
(LOGGER.warnf in the catch), append conversationId to the format string and pass
the conversationId argument so every stored-attachment log call (the
LOGGER.warnf/debugf invocations around attachmentStore.load and the initial
warn) contains the conversationId for traceability; keep the existing messages
and parameters (attachment.getFileName(), bytes.length, e.getMessage(), etc.)
and only add conversationId to each logger invocation.
In `@src/main/java/ai/labs/eddi/modules/llm/impl/ToolResponseTruncator.java`:
- Around line 103-104: The truncator's log messages are missing conversation
context; update ToolResponseTruncator to include conversation identifiers from
the taskParameters (e.g., conversationId, userId, taskId keys) in all logs
emitted by truncateIfNeeded and the other methods in this class (the logging
block spanning lines ~127-251), by extracting those values from the incoming
taskParameters Map<String,String> and appending them (or including them as
structured/log fields) to every processLogger or logger call so each log
contains the conversation context for correlation.
- Around line 240-245: The returned string can exceed maxChars because
SUMMARY_HEADER is prepended after truncating summary; update the logic in
ToolResponseTruncator.summarize to build header =
SUMMARY_HEADER.formatted(result.length(), toolName), compute available =
maxChars - header.length() (clamp to 0), then truncate summary to available
chars (e.g., substring(0, available) if needed), update the debug log to report
final lengths using header.length()+truncatedSummary.length(), keep
incrementCounter(toolName, "summarize"), and return header + truncatedSummary so
the total length never exceeds maxChars.
---
Duplicate comments:
In `@src/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.java`:
- Around line 1205-1269: Replace the mutable classes CounterweightConfig and
IdentityMaskingConfig with Java records to enforce immutability: change the
class declarations to record CounterweightConfig(boolean enabled, String level,
String placement, List<String> customInstructions) and record
IdentityMaskingConfig(boolean enabled, List<String> rules) and remove all
setters/getters; add compact constructors (or explicit canonical constructors)
that apply the current defaults (enabled -> false, level -> "normal", placement
-> "suffix", rules -> new ArrayList<>(), customInstructions -> null or empty
list as desired) and defensive-copy/normalize collection inputs (e.g.,
Collections.unmodifiableList or new ArrayList<>(list) when non-null) so behavior
and defaults of the original methods
(isEnabled/getLevel/getPlacement/getCustomInstructions/getRules) are preserved
via the record component accessors.
---
Nitpick comments:
In `@src/main/java/ai/labs/eddi/configs/agents/AgentSigningService.java`:
- Line 56: The privateKeyCache field in AgentSigningService is an unbounded
ConcurrentHashMap causing potential unbounded growth; replace it with a bounded
cache supporting eviction/TTL (e.g., Caffeine or Guava Cache) configured with a
sensible maximumSize and either expireAfterAccess/expireAfterWrite to remove
stale keys, and update usages of privateKeyCache.get/put/remove to the cache API
while preserving deleteKeyPair() behavior to invalidate entries; alternatively,
implement a scheduled cleanup task that evicts entries based on last-access time
if adding a third‑party cache is not allowed.
In `@src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java`:
- Around line 596-597: The code reads agent configuration on every agent turn
using agentStore.getCurrentResourceId(member.agentId()) and agentStore.read(...
) to check signInterAgentMessages; modify executeDiscussion() in
GroupConversationService to preload and cache each agent's configuration (e.g.,
Map keyed by member.agentId()) at the start of the discussion or use a
request-scoped cache, then replace per-turn calls to
agentStore.getCurrentResourceId/agentStore.read with lookups against that cache
so signInterAgentMessages is read once per agent per discussion.
In `@src/test/java/ai/labs/eddi/engine/memory/rest/RestAttachmentUploadTest.java`:
- Around line 67-87: The test in RestAttachmentUploadTest creates temp files
(tempFile) and currently deletes them with Files.deleteIfExists(...) at the end,
which can leak if assertions fail; update the uploadAttachment tests to use a
test-managed temp directory or guaranteed cleanup: either add a JUnit `@TempDir`
parameter and create temp files there or wrap the temp file creation + test call
in a try/finally and delete the file in the finally block; apply this pattern
for all methods that create tempFile (the blocks calling uploadAttachment(...),
tempFile, Files.write(...), and Files.deleteIfExists(...)) so cleanup always
runs even on assertion failures.
🪄 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: 6f56c34e-49d9-4133-a557-d5651ab446b1
📒 Files selected for processing (38)
docs/architecture.mddocs/changelog.mdplanning/agentic-improvements-plan.mdplanning/manager-ui-handoff.mdsrc/main/java/ai/labs/eddi/configs/agents/AgentSigningService.javasrc/main/java/ai/labs/eddi/configs/agents/CapabilityRegistryService.javasrc/main/java/ai/labs/eddi/configs/agents/rest/RestAgentStore.javasrc/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.javasrc/main/java/ai/labs/eddi/datastore/postgres/PostgresAttachmentStore.javasrc/main/java/ai/labs/eddi/datastore/postgres/PostgresConversationCheckpointStore.javasrc/main/java/ai/labs/eddi/engine/attachments/MimeValidator.javasrc/main/java/ai/labs/eddi/engine/internal/GroupConversationService.javasrc/main/java/ai/labs/eddi/engine/memory/DeepCopyUtil.javasrc/main/java/ai/labs/eddi/engine/memory/MemorySnapshotService.javasrc/main/java/ai/labs/eddi/engine/memory/MongoConversationCheckpointStore.javasrc/main/java/ai/labs/eddi/engine/memory/model/MemoryCheckpoint.javasrc/main/java/ai/labs/eddi/engine/memory/rest/RestAttachmentUpload.javasrc/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.javasrc/main/java/ai/labs/eddi/modules/llm/impl/CounterweightService.javasrc/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.javasrc/main/java/ai/labs/eddi/modules/llm/impl/MultimodalMessageEnhancer.javasrc/main/java/ai/labs/eddi/modules/llm/impl/ToolResponseTruncator.javasrc/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.javasrc/main/java/ai/labs/eddi/modules/llm/tools/impl/FetchToolResponsePageTool.javasrc/test/java/ai/labs/eddi/configs/agents/AgentSigningServiceTest.javasrc/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceTest.javasrc/test/java/ai/labs/eddi/engine/memory/DeepCopyUtilTest.javasrc/test/java/ai/labs/eddi/engine/memory/MemorySnapshotServiceTest.javasrc/test/java/ai/labs/eddi/engine/memory/model/MemoryCheckpointTest.javasrc/test/java/ai/labs/eddi/engine/memory/rest/RestAttachmentUploadTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/MultimodalMessageEnhancerExtendedTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/MultimodalMessageEnhancerTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/ToolResponseTruncatorExtendedTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/ToolResponseTruncatorTest.javasrc/test/java/ai/labs/eddi/modules/llm/tools/impl/FetchToolResponsePageToolTest.javasrc/test/java/ai/labs/eddi/modules/rules/impl/conditions/DeploymentContextConditionTest.java
✅ Files skipped from review due to trivial changes (4)
- planning/manager-ui-handoff.md
- docs/architecture.md
- src/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceTest.java
- planning/agentic-improvements-plan.md
🚧 Files skipped from review as they are similar to previous changes (10)
- src/test/java/ai/labs/eddi/modules/rules/impl/conditions/DeploymentContextConditionTest.java
- src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java
- src/main/java/ai/labs/eddi/engine/attachments/MimeValidator.java
- src/main/java/ai/labs/eddi/datastore/postgres/PostgresConversationCheckpointStore.java
- src/main/java/ai/labs/eddi/engine/memory/MongoConversationCheckpointStore.java
- src/main/java/ai/labs/eddi/datastore/postgres/PostgresAttachmentStore.java
- src/main/java/ai/labs/eddi/modules/llm/impl/CounterweightService.java
- src/test/java/ai/labs/eddi/modules/llm/tools/impl/FetchToolResponsePageToolTest.java
- src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorTest.java
- src/main/java/ai/labs/eddi/configs/agents/CapabilityRegistryService.java
… key rotation validation, summary header overflow
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/ai/labs/eddi/engine/memory/rest/RestAttachmentUpload.java (1)
80-80: ⚖️ Poor tradeoffConsider using a dedicated executor for blocking I/O operations.
CompletableFuture.runAsync()defaults to the commonForkJoinPool, which is optimized for CPU-bound tasks. TheuploadAttachmentmethod performs blocking file I/O (Files.size()andFiles.readAllBytes()), which can starve the pool under concurrent load and degrade throughput.Inject a dedicated executor (e.g., a cached or fixed thread pool configured for I/O) and pass it to
runAsync(Runnable, Executor)for better resource isolation.⚡ Example: injecting a dedicated executor
+import java.util.concurrent.Executor; public class RestAttachmentUpload { private final IAttachmentStore attachmentStore; private final long maxUploadBytes; + private final Executor ioExecutor; `@Inject` public RestAttachmentUpload(IAttachmentStore attachmentStore, `@ConfigProperty`(name = "eddi.attachments.max-size-bytes", - defaultValue = "20971520") long maxUploadBytes) { + defaultValue = "20971520") long maxUploadBytes, + `@Named`("io-executor") Executor ioExecutor) { this.attachmentStore = attachmentStore; this.maxUploadBytes = maxUploadBytes; + this.ioExecutor = ioExecutor; } - CompletableFuture.runAsync(() -> { + CompletableFuture.runAsync(() -> { + ... - }); + }, ioExecutor);Also applies to: 160-160, 184-184
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/engine/memory/rest/RestAttachmentUpload.java` at line 80, The async upload uses CompletableFuture.runAsync(...) which defaults to the ForkJoinPool and is running blocking file I/O (Files.size, Files.readAllBytes); inject or add a dedicated Executor (e.g., ioExecutor or an ExecutorService created/configured for I/O such as a cached or bounded thread pool) into RestAttachmentUpload and replace all CompletableFuture.runAsync(...) calls (including the uploadAttachment lambda and the other two occurrences) with runAsync(Runnable, executor) so blocking I/O runs on the dedicated executor rather than the common ForkJoinPool; ensure the executor is provided via constructor or DI (and documented/managed lifecycle) and referenced by name (e.g., ioExecutor) when updating the runAsync calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/ai/labs/eddi/engine/memory/rest/RestAttachmentUpload.java`:
- Around line 95-96: The log statements in RestAttachmentUpload (e.g., the
LOGGER.warnf call that currently logs "Attachment rejected: %s (%d bytes exceeds
%d byte limit)") must include the conversationId to provide context; update each
upload-related log invocation in RestAttachmentUpload (including the ones around
the reject/accept checks referenced at the same locations as the existing logger
calls) to interpolate or pass conversationId alongside sanitize(fileName),
fileSize and maxUploadBytes so the message includes conversationId (use the
existing variable name conversationId) — apply the same change to the other
upload log sites in this class where fileName/fileSize are logged so all
messages include conversationId for correlation.
---
Nitpick comments:
In `@src/main/java/ai/labs/eddi/engine/memory/rest/RestAttachmentUpload.java`:
- Line 80: The async upload uses CompletableFuture.runAsync(...) which defaults
to the ForkJoinPool and is running blocking file I/O (Files.size,
Files.readAllBytes); inject or add a dedicated Executor (e.g., ioExecutor or an
ExecutorService created/configured for I/O such as a cached or bounded thread
pool) into RestAttachmentUpload and replace all CompletableFuture.runAsync(...)
calls (including the uploadAttachment lambda and the other two occurrences) with
runAsync(Runnable, executor) so blocking I/O runs on the dedicated executor
rather than the common ForkJoinPool; ensure the executor is provided via
constructor or DI (and documented/managed lifecycle) and referenced by name
(e.g., ioExecutor) when updating the runAsync calls.
🪄 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: 83bb2a6f-01a1-472d-bf02-615630461ef7
📒 Files selected for processing (8)
docs/changelog.mdsrc/main/java/ai/labs/eddi/configs/agents/rest/RestAgentStore.javasrc/main/java/ai/labs/eddi/engine/internal/GroupConversationService.javasrc/main/java/ai/labs/eddi/engine/memory/rest/RestAttachmentUpload.javasrc/main/java/ai/labs/eddi/modules/llm/impl/ToolResponseTruncator.javasrc/test/java/ai/labs/eddi/configs/agents/rest/RestAgentStoreTest.javasrc/test/java/ai/labs/eddi/engine/internal/GroupConversationServiceTest.javasrc/test/java/ai/labs/eddi/engine/memory/rest/RestAttachmentUploadTest.java
✅ Files skipped from review due to trivial changes (1)
- docs/changelog.md
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/java/ai/labs/eddi/configs/agents/rest/RestAgentStore.java
- src/test/java/ai/labs/eddi/configs/agents/rest/RestAgentStoreTest.java
- src/main/java/ai/labs/eddi/modules/llm/impl/ToolResponseTruncator.java
- src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 60 out of 60 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
src/main/java/ai/labs/eddi/engine/memory/rest/RestAttachmentUpload.java:60
- The resource path is inconsistent with the existing Conversation Store API (
IRestConversationStoreis rooted at/conversationstore/conversations). Exposing attachments at/conversations/{conversationId}/attachmentsintroduces a parallel URL space and likely breaks client expectations and OpenAPI grouping. Consider moving this under/conversationstore/conversations/{conversationId}/attachments(or otherwise aligning with the existing conversation REST namespace).
@Path("/conversations")
@Tag(name = "Attachments")
public class RestAttachmentUpload {
private static final Logger LOGGER = Logger.getLogger(RestAttachmentUpload.class);
private final IAttachmentStore attachmentStore;
private final long maxUploadBytes;
@Inject
public RestAttachmentUpload(IAttachmentStore attachmentStore,
@ConfigProperty(name = "eddi.attachments.max-size-bytes",
defaultValue = "20971520") long maxUploadBytes) {
this.attachmentStore = attachmentStore;
this.maxUploadBytes = maxUploadBytes;
}
@POST
@Path("/{conversationId}/attachments")
- RestAttachmentUpload: conversationId in all logs, ManagedExecutor, tenantId sanitization (regex validation, not trusted for security) - AgentSigningService: evict privateKeyCache on generateKeyPair() - MultimodalMessageEnhancer: 10MB forwarding limit for stored images - ToolResponseTruncator: 500K pagination ceiling, fallback to truncate - PostgresAttachmentStore/GridFsAttachmentStore: MIME normalization - MimeValidator: add normalize() utility Tests: 12 new/updated, 104 targeted pass (0 failures)
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/java/ai/labs/eddi/configs/agents/AgentSigningService.java (2)
128-139: ⚡ Quick winPreserve exception type information in the cache loader.
The lambda wraps all exceptions in
RuntimeExceptionwith a generic message. When exceptions other thanSecretNotFoundExceptionoccur (e.g.,InvalidKeySpecExceptionfrom corrupt key data,IllegalArgumentExceptionfrom invalid Base64, orNoSuchAlgorithmException), the unwrapping logic at lines 148-153 will treat them as generic signing failures, losing diagnostic context.Consider preserving exception types or wrapping in a custom unchecked exception that the unwrapping logic can inspect:
♻️ Improved exception handling
PrivateKey privateKey = privateKeyCache.computeIfAbsent(cacheKey, k -> { try { SecretReference ref = new SecretReference(tenantId, vaultKeyName(agentId)); String privateKeyB64 = secretProvider.resolve(ref); byte[] privateKeyBytes = Base64.getDecoder().decode(privateKeyB64); KeyFactory keyFactory = KeyFactory.getInstance(ALGORITHM); return keyFactory.generatePrivate( new java.security.spec.PKCS8EncodedKeySpec(privateKeyBytes)); + } catch (ISecretProvider.SecretNotFoundException e) { + throw new RuntimeException("Secret not found", e); + } catch (NoSuchAlgorithmException | java.security.spec.InvalidKeySpecException e) { + throw new RuntimeException("Invalid key format", e); + } catch (IllegalArgumentException e) { + throw new RuntimeException("Invalid Base64 encoding", e); } catch (Exception e) { throw new RuntimeException("Failed to load private key for agent " + agentId, e); } });Then refine the unwrapping logic to distinguish these cases.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/configs/agents/AgentSigningService.java` around lines 128 - 139, The cache loader inside privateKeyCache.computeIfAbsent currently catches all exceptions and rethrows them as a plain RuntimeException, losing original exception types; change it to either rethrow the original checked exceptions as-is (where allowed) or wrap them in a custom unchecked exception (e.g., PrivateKeyLoadException) that preserves the original cause and exception type information from operations like secretProvider.resolve, Base64 decoding, KeyFactory.getInstance, and keyFactory.generatePrivate; then update the existing unwrapping logic that inspects the cached loader exception to specially handle the new PrivateKeyLoadException (or preserved exception types) so InvalidKeySpecException, IllegalArgumentException, SecretNotFoundException, NoSuchAlgorithmException, etc. can be distinguished and reported appropriately.
127-127: 💤 Low valueConsider a collision-resistant cache key format.
The current concatenation
tenantId + ":" + agentIdcould collide iftenantIdcontains colons (e.g.,tenantId="a:b", agentId="c"produces the same key astenantId="a", agentId="b:c"). If tenant IDs are validated elsewhere to exclude colons, this is safe. Otherwise, consider a structured format:-String cacheKey = tenantId + ":" + agentId; +String cacheKey = String.format("tenant=%s;agent=%s", tenantId, agentId);Or apply escaping if colons are permitted in tenant IDs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/configs/agents/AgentSigningService.java` at line 127, The cache key built in AgentSigningService using String cacheKey = tenantId + ":" + agentId is vulnerable to collisions when tenantId can contain colons; change the key format to a collision-resistant representation by either (a) using a structured encoding (e.g., JSON object or prefixed-length fields like tenantLength + ":" + tenantId + agentId) or (b) escaping delimiters in tenantId/agentId before joining, or (c) computing a stable hash (e.g., SHA-256 or HMAC) of tenantId and agentId and using that as cacheKey; update the code that references cacheKey accordingly so creation and lookup use the same scheme.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/main/java/ai/labs/eddi/configs/agents/AgentSigningService.java`:
- Around line 128-139: The cache loader inside privateKeyCache.computeIfAbsent
currently catches all exceptions and rethrows them as a plain RuntimeException,
losing original exception types; change it to either rethrow the original
checked exceptions as-is (where allowed) or wrap them in a custom unchecked
exception (e.g., PrivateKeyLoadException) that preserves the original cause and
exception type information from operations like secretProvider.resolve, Base64
decoding, KeyFactory.getInstance, and keyFactory.generatePrivate; then update
the existing unwrapping logic that inspects the cached loader exception to
specially handle the new PrivateKeyLoadException (or preserved exception types)
so InvalidKeySpecException, IllegalArgumentException, SecretNotFoundException,
NoSuchAlgorithmException, etc. can be distinguished and reported appropriately.
- Line 127: The cache key built in AgentSigningService using String cacheKey =
tenantId + ":" + agentId is vulnerable to collisions when tenantId can contain
colons; change the key format to a collision-resistant representation by either
(a) using a structured encoding (e.g., JSON object or prefixed-length fields
like tenantLength + ":" + tenantId + agentId) or (b) escaping delimiters in
tenantId/agentId before joining, or (c) computing a stable hash (e.g., SHA-256
or HMAC) of tenantId and agentId and using that as cacheKey; update the code
that references cacheKey accordingly so creation and lookup use the same scheme.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0a76790-96c2-4b8a-96d3-6cbc0c5ad89f
📒 Files selected for processing (13)
docs/changelog.mdsrc/main/java/ai/labs/eddi/configs/agents/AgentSigningService.javasrc/main/java/ai/labs/eddi/datastore/mongo/GridFsAttachmentStore.javasrc/main/java/ai/labs/eddi/datastore/postgres/PostgresAttachmentStore.javasrc/main/java/ai/labs/eddi/engine/attachments/MimeValidator.javasrc/main/java/ai/labs/eddi/engine/memory/rest/RestAttachmentUpload.javasrc/main/java/ai/labs/eddi/modules/llm/impl/MultimodalMessageEnhancer.javasrc/main/java/ai/labs/eddi/modules/llm/impl/ToolResponseTruncator.javasrc/test/java/ai/labs/eddi/configs/agents/AgentSigningServiceTest.javasrc/test/java/ai/labs/eddi/engine/attachments/MimeValidatorTest.javasrc/test/java/ai/labs/eddi/engine/memory/rest/RestAttachmentUploadTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/MultimodalMessageEnhancerExtendedTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/ToolResponseTruncatorExtendedTest.java
✅ Files skipped from review due to trivial changes (1)
- docs/changelog.md
🚧 Files skipped from review as they are similar to previous changes (9)
- src/test/java/ai/labs/eddi/configs/agents/AgentSigningServiceTest.java
- src/main/java/ai/labs/eddi/engine/attachments/MimeValidator.java
- src/main/java/ai/labs/eddi/datastore/postgres/PostgresAttachmentStore.java
- src/main/java/ai/labs/eddi/datastore/mongo/GridFsAttachmentStore.java
- src/test/java/ai/labs/eddi/engine/memory/rest/RestAttachmentUploadTest.java
- src/main/java/ai/labs/eddi/modules/llm/impl/MultimodalMessageEnhancer.java
- src/main/java/ai/labs/eddi/modules/llm/impl/ToolResponseTruncator.java
- src/test/java/ai/labs/eddi/modules/llm/impl/ToolResponseTruncatorExtendedTest.java
- src/main/java/ai/labs/eddi/engine/memory/rest/RestAttachmentUpload.java
…e key - Replace generic RuntimeException in computeIfAbsent lambda with PrivateKeyLoadException (typed unchecked). Unwrapping logic now catches it specifically and preserves original cause type (SecretNotFound, InvalidKeySpec, bad Base64, etc.) in the AgentSigningException message. - Use structured cache key format 'tenant=X;agent=Y' across all 3 sites (generateKeyPair, sign, deleteKeyPair) to prevent collisions when tenantId contains colons.
…vements # Conflicts: # docs/changelog.md
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Add LogSanitizer.sanitize() to all log statements that interpolate user-provided conversationId, filename, or triggeredBy values. Files: GridFsAttachmentStore, PostgresAttachmentStore, PostgresConversationCheckpointStore, MongoConversationCheckpointStore, MemorySnapshotService
rolandpickl
requested changes
May 14, 2026
Address Roland's review: unbounded recursion could StackOverflow on circular or adversarially nested structures. Added MAX_DEPTH=32 (conversation properties are typically 2-3 levels; 32 is generous). Throws IllegalStateException with clear message when exceeded. Tests: 10 pass (2 new depth guard tests)
rolandpickl
approved these changes
May 15, 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.
Summary
This PR evolves EDDI from a configured pipeline to a governed agent ecosystem. Agents can now discover each other's capabilities, carry multimodal context, self-regulate behavior through counterweights, recover from failed operations via memory checkpoints, and prove their identity cryptographically.
All 9 features are config-driven and off by default — existing agents work unchanged.
70 files changed · +7,118 / −280 lines · 55 Java files · 23 test classes · 21 commits
What's New
🛡️ Behavioral Counterweights
Safety instructions injected into the system prompt at engine level. Three preset levels (
normal→cautious→strict) with auto-downgrade in batch/scheduled runs. Customizable via Prompt Snippets without redeployment.🎭 Identity Masking
Prevents agents from revealing their model name, provider, or architecture when probed. Configurable rules prepended to the system prompt.
📏 Tool Response Truncation (3 strategies)
Prevents context window blowout from verbose tool outputs:
truncate— hard cut with note (default, backward-compatible)paginate— split into pages, LLM fetches more viafetch_tool_response_pagebuilt-in toolsummarize— routes through a cheap model (inherits parent task's API key), falls back to truncate on failure. 6-guard fallback chain with cost ceiling, empty/oversized summary protection, and full log sanitization.🔍 A2A Capability Registry
Runtime agent discovery via structured skills. Agents advertise capabilities with confidence levels; behavior rules query the registry with 4 selection strategies (
highest_confidence,round_robin,all,random). Optional public discovery endpoint at/.well-known/capabilities.💾 Memory Checkpoints & Rollback
Snapshot conversation state before risky tool executions, roll back cleanly on failure. Properties preserve full scope metadata (
longTerm/step/conversation/secret) through rollback. Configurable retention (default: 10 checkpoints per conversation).📎 Multimodal Attachments
Images, audio, and documents flow through the pipeline to vision-capable LLMs. Dual-backend storage (GridFS + PostgreSQL), magic-byte MIME validation, automatic
ImageContent/AudioContentconversion.🔐 Cryptographic Agent Identity
Ed25519 key generation, signing, and verification with vault-backed private keys. Key rotation support with versioned public keys and temporal validity windows. Security flags validated but intentionally not yet wired (HTTP 400 on premature activation).
📝 Memory Policy (Commit Flags)
Strict write discipline hides failed task data from subsequent LLM turns, injecting a concise error digest instead. Three modes:
digest,exclude_all,keep_all.🌍 Deployment Context Condition
Environment-aware behavior rules. Match on deployment environment (
production/staging/development) and agent tags to auto-activate safety features per environment.Configuration Touchpoints
Per-task (
langchain.json){ "counterweight": { "enabled": true, "level": "cautious" }, "identityMasking": { "enabled": true, "rules": ["..."] }, "toolResponseLimits": { "defaultMaxChars": 5000, "truncationStrategy": "summarize", "summarizerModel": "gpt-4o-mini" } }Per-agent (
AgentConfiguration){ "capabilities": [{ "skill": "translation", "confidence": "high" }], "sessionManagement": { "autoSnapshot": { "enabled": true, "triggerOn": ["before_tool"] } }, "memoryPolicy": { "strictWriteDiscipline": { "enabled": true, "onFailure": "digest" } } }Testing
mainBreaking Changes
None. All features default to off. Existing agent configurations, REST API contracts, and MongoDB document schemas are fully backward-compatible.
Documentation
docs/changelog.md— detailed entries for every change with design rationaleplanning/manager-ui-handoff.md— complete spec for Manager UI integration (JSON paths, field types, defaults, suggested widgets)planning/agentic-improvements-plan.md— original design plan with updated statusSummary by CodeRabbit
New Features
Improvements
Documentation
Tests