fix(llm): LLM core, persistent memory, migration, import/export (wave 2b) - #618
Merged
Conversation
… migration, import/export The tool pipeline had a second door - F14: executeToolWrapped has one production call site and all seven tool sources route through it — except McpCallsTask, a rule-triggered lifecycle task calling the same external MCP tools directly. It also bypassed ToolApprovalGate, so hitlConfig.toolApprovals gated LLM-initiated calls and not rule-initiated ones. - F18: converse_with_agent had no guardrails at all — DynamicAgentConfig was never consulted and allowDelegation never checked. A->B->A cycles started a fresh conversation each hop, so the busy-guard never broke them; prompt injection was sufficient to start one. - F17: maxCreatedAgentsPerDiscussion was enforced per turn, not per discussion, so a 5-member x 3-phase discussion with a cap of 5 permitted up to 75 agents deployed to production. - F15/F16: remote MCP tools silently shadowed built-ins (specs in a List, executors in a Map), and their descriptions reached the prompt verbatim with no cap or sanitisation. - A10: the MCP client did no URL validation, unlike its A2A sibling, while a discovery endpoint echoed the response body. Two real bugs the tests caught, that the review did not The F12 traversal cache (a) memoized failure-derived results, replaying an empty config set for the whole TTL after one transient store blip — an agent silently losing its httpcalls/mcpcalls/RAG config; and (b) omitted the target class from the key while casting the value unchecked, so a future caller could get another caller's entry and a ClassCastException. F13 was fixed but unreachable The inheritedParameters overloads were added and tested directly, but no caller in src/main passed them, so the rolling summary still could not authenticate and still silently never materialised. Now threaded through LlmTask -> ConversationSummarizer -> SummarizationService, with tests at both hops. Memory & properties - G2: scope "secret" failed OPEN — on vault failure it returned the plaintext before the scrub block, persisting the secret as a property AND as raw input:initial data. Vault-disabled is the default, so this was the common path. - G1: user-memory search/delete crossed agent boundaries via an unscoped filter. - G13: token-aware windowing could emit a prompt with no user message at all. - G12: a turn could be silently lost when the document was deleted mid-turn. - G5/G6/G7: N+1 write inside a read cursor, self-reinforcing recall, updatedAt refreshed every turn (so most_recent meant nothing and retention never expired), and re-upsert silently flipping a memory's owner. - G9/G10/G11: Map contract broken so rollback left stale properties visible, step-scoped values persisted despite the docs, one bad field failing the whole conversation load. Migration & import/export - B12: migration irrecoverably dropped doubles, longs, booleans and arrays. - B13: the template migrator corrupted unrelated JSON and arithmetic. - B14: rename migration abandoned v5 data and still marked itself complete. - D11/D12: import had no rollback; neither import nor export cleaned up temp. Full suite: 12,384 tests, 0 non-environmental failures. G2 (twice, including a sharper mutation isolating the input scrub), G12 and F13's new wiring were all mutation-checked.
Contributor
|
Important Review skippedToo many files! This PR contains 120 files, which is 20 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (120)
You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 7 minutes. |
aisabella-ai
self-requested a review
July 28, 2026 20:41
aisabella-ai
approved these changes
Jul 28, 2026
…m-memory # Conflicts: # docs/changelog.md
…m-memory # Conflicts: # src/main/java/ai/labs/eddi/configs/groups/mongo/GroupConversationStore.java # src/test/java/ai/labs/eddi/configs/groups/mongo/GroupConversationStoreTest.java
…d, Postgres parity This PR reached "approved" without ever being seen by CI or a review bot: CodeRabbit reported "reviews are disabled for this base branch" because it was stacked on a disabled base, and a base retarget fires `edited`, which is not a default `pull_request` trigger — so no workflow ran on its head SHA. A review pass over the 91-file diff produced 16 findings that survived adversarial verification, plus 21 from completeness and test-quality critics. The two that mattered most were both guards that did not guard: - McpCallsTask routed rule-triggered MCP calls through executeToolWrapped, which catches every exception and RETURNS an error string. executeWithRetry therefore never saw a throwable: retry never retried, continueOnError was dead code, and a failed call was stored as a successful response. The metering wrapper stays; a real failure signal is restored on top of it. - The delegation-depth guard was inert in production. delegationDepth reached only the callee's startConversation context (step 0); the follow-up `say` carried none, so the turn that decides delegation read nothing. Mutation- checked: removing the propagation fails 2 tests. Third cross-backend gap in this stack: G12 (a turn is never silently discarded), G5 (most_accessed recency reservation) and G7 (global entries keep their owning agent) had all shipped MongoDB-only, with Postgres answering benignly rather than signalling the gap. Ported with tests. After schedule userId and the audit sequence, this pattern is not coincidence — the D4/J3 conformance suite is the real fix and is still outstanding. Also fixed: summary configs naming a different llmProvider inherited the parent's apiKey/baseUrl, sending one vendor's key to another's endpoint (the pre-PR defaults serialized llmProvider "anthropic" into stored configs, so this was reachable in ordinary deployments); validation moved off the read path and onto the write boundary for both MCP calls and RAG, so stored configs stay loadable while bad ones are refused on the way in; the v6 rename migration aborting permanently when a v6 collection merely exists; the pre-migration backup duplicating transcripts outside GDPR erasure; export cleanup deleting a shared tmp/<agentId> it could not prove it created; and the streaming no-partials fallback overwriting the warning key responseValidation dispatches on. I5 remains genuinely unfixed: maxCheckpointsPerConversation is still ignored at runtime, and the test that implied otherwise exercised an overload no production path calls. The misleading label is removed rather than left to imply coverage; the changelog says so explicitly. One critic finding was investigated and rejected — the ExpressionFactory setDomain removal really was a no-op. Full unit suite: 12,709 tests; the only non-environmental failures were two existing RestMcpCallsStore tests that built a bare config to assert delegation, updated to use a valid one now that the write boundary rejects unusable configs.
![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 is the second “wave 2b” tranche addressing external review findings around LLM execution, persistent memory correctness, migration/import safety, and MCP tool execution parity across call paths.
Changes:
- Routes rule-triggered MCP calls through the same
ToolExecutionServicepipeline used by LLM-initiated tool calls, closing approval/metrics/rate-limit bypasses and adding stricter config validation at write boundaries. - Fixes multiple persistent-memory correctness issues (fail-closed secret vaulting with input scrubbing, drift-tolerant snapshot conversion, “deleted mid-turn” write detection for Mongo/Postgres).
- Hardens LLM prompt assembly and summarization wiring (ensure current-turn survives token-aware windowing; thread inherited model parameters through summarization; bound RAG context growth; improve streaming fallback behavior).
Reviewed changes
Copilot reviewed 120 out of 120 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/ai/labs/eddi/utils/RestUtilitiesTest.java | Adds malformed-URI coverage for extractResourceId. |
| src/test/java/ai/labs/eddi/modules/rules/impl/RulesEvaluationTaskTest.java | Removes dropped-success test now that category is removed. |
| src/test/java/ai/labs/eddi/modules/rules/impl/RuleSetResultTest.java | New tests pin rule outcome categories and keys. |
| src/test/java/ai/labs/eddi/modules/properties/impl/PropertySetterTaskTest.java | Updates secret-scope behavior to fail closed + scrub input. |
| src/test/java/ai/labs/eddi/modules/properties/impl/PropertySetterTaskExtendedTest.java | Adds extended coverage for fail-closed secret scope behavior. |
| src/test/java/ai/labs/eddi/modules/output/model/OutputEntryComparisonTest.java | New ordering/compareTo contract tests for output bubble ordering. |
| src/test/java/ai/labs/eddi/modules/nlp/expressions/ExpressionFactoryTest.java | New tests covering connector domain propagation behavior. |
| src/test/java/ai/labs/eddi/modules/mcpcalls/McpCallsTaskTest.java | Updates tests for ToolExecutionService-wrapped MCP execution. |
| src/test/java/ai/labs/eddi/modules/mcpcalls/impl/McpCallsTaskTest.java | Same as above for impl test variant. |
| src/test/java/ai/labs/eddi/modules/mcpcalls/impl/McpCallsTaskBranchCoverageTest.java | Adjusts branch coverage tests for tool wrapping + config URL. |
| src/test/java/ai/labs/eddi/modules/llm/model/LlmConfigurationTest.java | Updates conversation-summary defaults to “inherit” (nulls). |
| src/test/java/ai/labs/eddi/modules/llm/model/LlmConfigurationModelsTest.java | Mirrors “inherit” default behavior in model tests. |
| src/test/java/ai/labs/eddi/modules/llm/impl/WorkflowTraversalCacheTest.java | New tests for traversal memoization behavior and TTL. |
| src/test/java/ai/labs/eddi/modules/llm/impl/SummarizationServiceInheritanceTest.java | New tests ensuring summarizer inherits credentials/params correctly. |
| src/test/java/ai/labs/eddi/modules/llm/impl/RagContextProviderExtendedTest.java | Clears traversal cache between tests; adds trace assertions. |
| src/test/java/ai/labs/eddi/modules/llm/impl/RagContextProviderCapTest.java | New tests for bounded RAG block formatting and cap resolution. |
| src/test/java/ai/labs/eddi/modules/llm/impl/ConversationSummarizerTest.java | Verifies inherited params reach SummarizationService. |
| src/test/java/ai/labs/eddi/modules/llm/impl/ConversationHistoryBuilderTest.java | Updates tests to ensure current-turn survives token-aware windowing. |
| src/test/java/ai/labs/eddi/integration/RagCrudIT.java | Adds chunkStrategy legacy rewrite + rejection integration coverage. |
| src/test/java/ai/labs/eddi/engine/setup/AgentSetupServiceTest.java | Moves environment parsing tests onto new validated resolution path. |
| src/test/java/ai/labs/eddi/engine/setup/AgentSetupServiceBranchCoverageTest.java | Ensures unknown environments fail validation and create nothing. |
| src/test/java/ai/labs/eddi/engine/runtime/rest/interceptors/LegacyPathRewriteFilterTest.java | Pins case-sensitive rewrite correctness and legacy env rewrites. |
| src/test/java/ai/labs/eddi/engine/model/DeploymentTest.java | Adds strict environment parser tests + lenient deserialization parity. |
| src/test/java/ai/labs/eddi/engine/memory/model/ConversationPropertiesTest.java | Updates expectation that toMap() reflects stored map even w/o memory. |
| src/test/java/ai/labs/eddi/engine/memory/model/ConversationPropertiesMapContractTest.java | New tests for toMap() correctness under all Map mutations + step-scope behavior. |
| src/test/java/ai/labs/eddi/engine/memory/MemorySnapshotServiceRollbackTest.java | New end-to-end rollback test ensuring template view drops post-checkpoint props. |
| src/test/java/ai/labs/eddi/engine/memory/MemoryItemConverterNamespacesTest.java | New tests verifying {snippets.*} / {vars.*} availability and CDI wiring. |
| src/test/java/ai/labs/eddi/engine/memory/ConversationMemoryUtilitiesDriftTest.java | New tests covering step/output drift tolerance in snapshot conversion. |
| src/test/java/ai/labs/eddi/engine/memory/ConversationMemoryStoreTest.java | Adds tests for “deleted mid-turn” behavior (Mongo). |
| src/test/java/ai/labs/eddi/engine/mcp/McpToolUtilsTest.java | Adds strict environment parsing tests + message contract checks. |
| src/test/java/ai/labs/eddi/engine/mcp/McpToolUtilsBranchCoverageTest.java | Updates branch coverage to assert unknown env rejection. |
| src/test/java/ai/labs/eddi/datastore/postgres/PostgresConversationMemoryStoreUnitTest.java | Adds unit test for “deleted mid-turn” surfaced as error (Postgres). |
| src/test/java/ai/labs/eddi/datastore/postgres/PostgresConversationMemoryStoreTest.java | Adds integration-style test for “deleted mid-turn” behavior + success path. |
| src/test/java/ai/labs/eddi/datastore/mongo/MongoUserMemoryStoreTest.java | Adds real-backend tests for recall ownership and most_accessed window behavior + index presence. |
| src/test/java/ai/labs/eddi/configs/rag/model/RagConfigurationValidationTest.java | New unit tests for supported/legacy chunkStrategy normalization and validation. |
| src/test/java/ai/labs/eddi/configs/properties/mongo/MongoUserMemoryStoreTest.java | Ensures most_accessed increments happen via one batched write (updateMany). |
| src/test/java/ai/labs/eddi/configs/migration/V6RenameMigrationBranchTest.java | Tightens rename failure reporting and dropTarget safety. |
| src/test/java/ai/labs/eddi/configs/migration/TemplateSyntaxMigratorTest.java | Adds regression tests ensuring non-Thymeleaf arithmetic/JSON isn’t corrupted. |
| src/test/java/ai/labs/eddi/configs/mcpcalls/rest/RestMcpCallsStoreWriteValidationTest.java | New tests pinning strict MCP config validation at write boundary. |
| src/test/java/ai/labs/eddi/configs/mcpcalls/rest/RestMcpCallsStoreTest.java | Updates create test to use valid config now that writes validate. |
| src/test/java/ai/labs/eddi/configs/mcpcalls/rest/RestMcpCallsStoreDeepCoverageTest.java | Ensures update delegation tests use valid URL under new validation. |
| src/test/java/ai/labs/eddi/configs/mcpcalls/model/McpCallsConfigurationValidationTest.java | New unit tests for MCP URL/transport validation semantics. |
| src/main/resources/application.properties | Adds audit queue sizing + “audit signing required” operator switch docs. |
| src/main/java/ai/labs/eddi/utils/RestUtilities.java | Makes extractResourceId robust to authority-only URIs without throwing. |
| src/main/java/ai/labs/eddi/modules/rules/impl/RulesEvaluationTask.java | Removes writing the never-populated dropped-success category. |
| src/main/java/ai/labs/eddi/modules/rules/impl/RuleSetResult.java | Removes dropped-success field/accessor and updates string form/docs. |
| src/main/java/ai/labs/eddi/modules/properties/impl/PropertySetterTask.java | Makes secret vaulting fail closed; scrubs input before aborting; preserves LifecycleException cause/message. |
| src/main/java/ai/labs/eddi/modules/output/model/OutputEntry.java | Documents compareTo contract; uses imported Objects helpers. |
| src/main/java/ai/labs/eddi/modules/nlp/expressions/ExpressionFactory.java | Stops self-assigning domain which wiped child expression domains. |
| src/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.java | Adds RAG/system prompt caps; makes summary provider/model inherit rather than hardcode. |
| src/main/java/ai/labs/eddi/modules/llm/impl/SummarizationService.java | Adds inherited-parameters overloads; strips responseFormat; avoids mutating caller map. |
| src/main/java/ai/labs/eddi/modules/llm/impl/StreamingLegacyChatExecutor.java | Adds fallback when providers emit no partials; emits metric and warning; clarifies timeout floor behavior. |
| src/main/java/ai/labs/eddi/modules/llm/impl/RagContextProvider.java | Warns on unsupported KB settings without failing retrieval; caps formatted RAG block size; stores trace. |
| src/main/java/ai/labs/eddi/modules/llm/impl/ConversationSummarizer.java | Threads inherited params through to summarization service call. |
| src/main/java/ai/labs/eddi/modules/llm/impl/ConversationHistoryBuilder.java | Ensures current turn is included in token-aware windowing; trims anchors first. |
| src/main/java/ai/labs/eddi/modules/llm/impl/CascadingModelExecutor.java | Prevents mid-cascade steps from live streaming into the shared sink. |
| src/main/java/ai/labs/eddi/engine/setup/AgentSetupService.java | Uses strict environment parsing with validation errors surfaced as 400s; removes lenient fallback-to-prod parser. |
| src/main/java/ai/labs/eddi/engine/runtime/rest/interceptors/LegacyPathRewriteFilter.java | Switches to ordered rewrite map; fixes trigger path case; rewrites legacy env segments to production. |
| src/main/java/ai/labs/eddi/engine/model/Deployment.java | Adds strict env parser; keeps lenient deserialization but logs unknown values. |
| src/main/java/ai/labs/eddi/engine/memory/model/ConversationProperties.java | Makes toMap() derived from map state; avoids stale template values; changes mirroring semantics for step scope. |
| src/main/java/ai/labs/eddi/engine/memory/MemorySnapshotService.java | Adds overloads for checkpoint retention and retrieval; clarifies rollback limitations; centralizes retention fallback. |
| src/main/java/ai/labs/eddi/engine/memory/MemoryItemConverter.java | Injects snippets/vars namespaces into the shared template data model with failure isolation. |
| src/main/java/ai/labs/eddi/engine/memory/IConversationCheckpointStore.java | Clarifies GDPR erasure relevance in docs for checkpoint deletion. |
| src/main/java/ai/labs/eddi/engine/memory/ConversationMemoryUtilities.java | Handles step/output drift safely with warnings and bounds checks. |
| src/main/java/ai/labs/eddi/engine/memory/ConversationMemoryStore.java | Surfaces “deleted mid-turn” writes as errors; degrades context conversion per-entry on unknown/missing type. |
| src/main/java/ai/labs/eddi/engine/mcp/McpToolUtils.java | Uses strict environment parsing; adds distinct exception type; simplifies errorJson signature. |
| src/main/java/ai/labs/eddi/datastore/postgres/PostgresConversationMemoryStore.java | Mirrors Mongo “deleted mid-turn” detection by checking affected row count. |
| src/main/java/ai/labs/eddi/configs/rag/rest/RestRagStore.java | Enforces KB validation at write boundary; normalizes legacy chunkStrategy; validates on duplicate. |
| src/main/java/ai/labs/eddi/configs/rag/model/RagConfiguration.java | Adds supported/legacy chunkStrategy sets; provides normalization + validation helpers. |
| src/main/java/ai/labs/eddi/configs/migration/TemplateSyntaxMigrator.java | Anchors concatenation rewrite to Thymeleaf delimiters to avoid corrupting JSON/arithmetic. |
| src/main/java/ai/labs/eddi/configs/mcpcalls/rest/RestMcpCallsStore.java | Validates MCP configs on create/update; keeps duplicate lenient for backward compatibility. |
| src/main/java/ai/labs/eddi/configs/mcpcalls/model/McpCallsConfiguration.java | Adds write-boundary validate() enforcing URL scheme and supported transports. |
| src/main/java/ai/labs/eddi/configs/IRestVersionInfo.java | Documents why this mixin carries no security annotation. |
| src/main/java/ai/labs/eddi/configs/groups/model/AgentGroupConfiguration.java | Adds delegation depth and allowlist fields to DynamicAgentConfig. |
| src/main/java/ai/labs/eddi/configs/agents/model/AgentConfiguration.java | Documents current checkpoint wiring limitations and reserved auto-snapshot fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
#613 merged to main while the review fixes were in flight, which left this PR CONFLICTING — and a conflicting PR has no computable merge ref, so no CI could run on it at all. Two real conflicts: - Deployment.java — both sides rewrote the same javadoc. #613 documented the legacy unrestricted/restricted aliases; this branch added parseStrict() and explained why fromString stays lenient. Kept both: the strict parser for call sites that ACT on an environment, the lenient one for deserializing stored documents, with #613's note that the aliases also appear in exported ZIPs. - docs/changelog.md — both added a top entry; both kept. The dangerous part was what merged silently: #613 added CallerIdentityResolver and CallerIdentityContext constructor parameters to ApiCallExecutor, CascadingModelExecutor and LlmTask. Git merged the production files cleanly, but four test files added by the review-fix commit were written against the old signatures and did not compile. Fixed by passing the new arguments the way main's own tests do. Verified beyond the compile: 860 tests across every class touched by the merge (LlmTask, CascadingModelExecutor, ApiCallExecutor, AgentSetupService, ConverseWithAgentTool, McpCallsTask, McpToolProviderManager, Deployment, ConversationProperties, the RAG and MCP REST stores) all pass.
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}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 120 out of 120 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/main/java/ai/labs/eddi/modules/llm/impl/ConversationHistoryBuilder.java:239
- The new “current turn is non‑negotiable” logic can still fail when anchorFirstSteps >= allMessages.size() and the conversation is over budget: the last message is treated as anchored, so the anchor-dropping loop is skipped and there’s no way to shed earlier messages while preserving the current turn. That can still produce a prompt over maxContextTokens.
Consider capping the anchor at most to (size - 1) so the current turn is always handled as the forced-included message and the anchor-dropping logic can do its job even for short-but-oversized conversations.
Copilot, CodeQL and the code-quality bot reached this PR once the merge ref became computable. Three behavioural findings and five log-injection alerts, all real: - ConversationHistoryBuilder: when anchorFirstSteps >= message count, effectiveAnchor equalled size, so lastIsAnchored turned true — which both zeroed the current-turn reservation AND disabled the anchor-trim loop, since that loop is guarded on !lastIsAnchored. Every message became an untrimmable anchor and the windowing path returned an over-budget prompt. The current turn is now never an anchor (it is force-included in step 2 instead), so the G13 guarantee holds and anchors stay trimmable. Reachable during any conversation's opening turns with a generous anchorFirstSteps. Mutation-checked: restoring the old ceiling fails the new test. - ConversationProperties.mirrorToCurrentStep keyed on property.getName() while toMap() falls back to the map key. Property is deserialized from stored config and may legally have a null name, so the same property appeared under its key in templates and under a null key in the conversation output. - ApiCallExecutor: the analyzer flagged an NPE risk in the retry loop, but the premise was inverted. `response` cannot be null there — single unconditional assignment, no continue/break, executeAndMeasureRequest dereferences it before returning, and the loop condition reads it. The dead `!= null` ternary after the loop was the sole reason the variable was inferred nullable, so it is removed rather than reinforced with a guard; a fabricated 500 would have masked a real defect. No behaviour change. - Five CodeQL log-injection alerts (CWE-117) in log statements added by the review fixes: RestRagStore, ConversationMemoryStore x2, ConversationMemoryUtilities and MongoUserMemoryStore now route user-controlled values through the existing LogSanitizer. 901 tests across every affected class pass.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 120 out of 120 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/main/java/ai/labs/eddi/modules/llm/impl/RagContextProvider.java:150
- This warning logs raw knowledge-base names and the unsupported-settings message. Since both originate from persisted configuration, they can contain newlines/control characters and enable log-forging. Consider sanitizing (e.g., via LogSanitizer) before logging, while keeping the raw value in the trace if needed.
src/test/java/ai/labs/eddi/modules/llm/impl/WorkflowTraversalCacheTest.java:83 - The test uses a non-existent workflow step type string ("eddi://ai.labs.mcpCalls"), so it doesn’t reflect production behavior (the real type is "eddi://ai.labs.mcpcalls"). This weakens the cache key regression coverage for F12.
Copilot suppressed both of these as "low confidence"; both are correct. - RagContextProvider logged the knowledge-base name and the unsupported-settings message raw. Both come from persisted configuration, so this is the same log-injection (CWE-117) class as the five CodeQL flagged — this site was simply missed because it sits on the retrieval path rather than the write path. Routed through LogSanitizer; the trace entry keeps the raw values, which is the right split (a trace is data, a log line is a record). - WorkflowTraversalCacheTest asserted cache-key separation using "eddi://ai.labs.mcpCalls", which is not a real step type — production is all-lowercase "eddi://ai.labs.mcpcalls". The test still passed, because two different strings do produce two cache entries, but it pinned a value that can never occur and so proved nothing about the F12 cache key in production.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 120 out of 120 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/main/java/ai/labs/eddi/configs/rag/rest/RestRagStore.java:118
duplicateRag()appliesprepareForWrite(config), which can now reject duplicating an already-stored (legacy) knowledge base config. Otherduplicate*endpoints (e.g.RestOutputStore#duplicateOutputSet,RestPropertySetterStore#duplicatePropertySetter,RestRuleSetStore#duplicateRuleSet, andRestMcpCallsStore#duplicateMcpCalls) intentionally do not validate on duplicate to avoid introducing a new failure mode when copying existing documents. Consider aligningduplicateRag()with that convention by only normalizing legacy chunkStrategy values (paragraph/sentence) but not rejecting other stored values during duplication.
public Response duplicateRag(String id, Integer version) {
restVersionInfo.validateParameters(id, version);
RagConfiguration config = restVersionInfo.read(id, version);
prepareForWrite(config);
return restVersionInfo.create(config);
src/main/java/ai/labs/eddi/utils/RestUtilities.java:72
- When parsing a full URI that has no path but does have a query (e.g.
eddi://ai.labs.agent?version=1),pathStartIndexis-1andrelativeUriStringbecomes empty, so theversionquery param is silently ignored andgetVersion()returns0. Since this method is documented as extracting both id and version (and already treats query-version parse errors as the one case that should throw), it should still preserve the query string even when there is no path. Usinguri.getRawQuery()here also avoids accidentally mixing fragments into the query parsing later.
… when a URI has no path Copilot suppressed both of these as low confidence. Both are right, and the first is an inconsistency I introduced myself. - duplicateRag ran the full write-boundary check, so a knowledge base the store happily serves through readRag could no longer be copied — a NEW failure mode for data that already exists, rather than a guard against creating bad data. Every other duplicate* endpoint declines to validate for exactly that reason, including duplicateMcpCalls, which I wrote in this same PR with that rationale spelled out in its javadoc. prepareForWrite is now split: duplication applies only the always-safe half (normalize a legacy strategy to what ingestion actually did), while create/update keep rejecting. - RestUtilities.extractResourceId dropped the query when a URI had no path. "eddi://ai.labs.agent?version=1" has no '/' after the scheme, so everything from that point was discarded and the version silently became 0 — which is the value callers already use to mean "unspecified", making an explicit version indistinguishable from none. The existing parameterized test feeds this very URI but only asserts the id, which is why it went unnoticed; the new tests assert the version in both the query and no-query cases.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 120 out of 120 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/main/java/ai/labs/eddi/engine/model/Deployment.java:78
- The warning log in fromString() includes the raw, potentially user-controlled environment string; sanitize it to avoid log injection (CWE-117).
public static Environment fromString(String value) {
try {
return parseStrict(value);
} catch (IllegalArgumentException e) {
LOGGER.warnv("Unknown environment ''{0}'' — falling back to ''{1}''. Valid values: {2}", value, production, VALID_ENVIRONMENTS);
return production;
src/main/java/ai/labs/eddi/engine/model/Deployment.java:9
- Add LogSanitizer import so log messages in this file can sanitize user-controlled values (prevents CWE-117 log injection).
This issue also appears on line 73 of the same file.
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonValue;
import org.jboss.logging.Logger;
Copilot flagged Deployment.fromString, which logs the raw unknown environment value — that value arrives as a JAX-RS path/query param or from stored JSON, so it is user-controlled (CWE-117). Fixed. Sweeping the rest of the PR's new log statements for the same pattern turned up two more the scanners never reported: - PostgresUserMemoryStore logged key/userId/agent ids raw in the cross-agent global-write warning, while its MongoUserMemoryStore twin — the same log statement, ported in this PR — was sanitized because CodeQL flagged it. CodeQL reports the path it can prove reachable, and the Postgres adapter is selected at runtime, so the ported copy sailed through. This is the FOURTH Mongo/Postgres divergence in this stack, and this one is inside the fix for the divergence itself: porting a fix duplicates the code, and a later fix to one copy does not follow. - RestImportService logged resource.id() raw in all three rollback paths. Those ids come from an uploaded ZIP, which is about as user-controlled as input gets. 489 tests across the affected classes pass.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 120 out of 120 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/main/java/ai/labs/eddi/modules/llm/impl/WorkflowTraversal.java:182
workflowVersionparsing can throwNumberFormatExceptionif the URI hasversion=but not a numeric value (or an unexpected query shape). That bypasses the intended “degrade and continue” behavior and can abort discovery for the whole turn. Consider treating an invalid version as a degraded traversal (warn + continue) just like the missing-version case above.
src/main/java/ai/labs/eddi/engine/memory/MemoryItemConverter.java:119- The warning logs interpolate
e.getMessage()directly. Exception messages can include user-controlled values, so this risks log-injection (CWE-117). Sanitize the message before logging (consistent with other recent changes using LogSanitizer).
} catch (RuntimeException e) {
LOGGER.warnf("Could not resolve prompt snippets for template data: %s", e.getMessage());
}
ginccc
added a commit
that referenced
this pull request
Jul 29, 2026
…review fixes) Both conflicts are two independent changes to the same code, where neither side subsumes the other: - ConversationService.processConversationStep — this branch extracted the body into runConversationStep so a try/finally could guarantee processingTurn.release() (C11); main added the caller-identity capture in the same method. Resolution keeps the C11 wrapper AND main's capture, with the capture staying OUTSIDE the returned lambda — it has to run on the REST request thread, since the lambda executes on a pool thread with no request to capture from. The bound callable is passed through runConversationStep into runGuardedConversationStep, which is exactly where main used it, so behaviour is unchanged in both directions. - GroupConversationService parallel phase — this branch added cancellation propagation (the `cancellation` argument plus a MemberTurnCancelledException catch that surfaces it instead of fabricating a contribution); main wrapped the supplier in withIdentitySupplying so the caller follows the fan-out onto further virtual threads. Both kept. The cancellation catch must stay ABOVE the generic Exception catch, which would otherwise turn a cancellation back into an error transcript entry — the exact behaviour that catch exists to prevent. The silent half, same as the #618 merge: main added a CallerIdentityContext constructor parameter to both services. Git merged the production files cleanly while three test files added by this branch no longer compiled against the new signatures. 1,257 tests pass across ConversationService, GroupConversationService, Conversation and CallerIdentity.
ginccc
added a commit
that referenced
this pull request
Jul 29, 2026
…coping, interrupt bookkeeping Like #618, this PR reached "approved" with no CI run and no bot review: a stacked base disables CodeRabbit, and a base retarget does not fire the CI trigger. A dedicated pass over the 37-file diff produced 6 findings that survived adversarial verification (12 of 18 refuted) plus 22 from critics. The one that matters most is an access-control escalation this PR created: - The schedule REST surface never checked schedule.userId. Inert on its own — but the new dreamType=dream_consolidation dispatch armed it, so any eddi-editor could create and fire a schedule that BULK-DELETES ANOTHER USER'S persistent memories. RestScheduleStore already injected OwnershipValidator and simply did not use it here. Now admin-or-self on create, update (the re-point path) and fireNow, refusing with 403 rather than silently rewriting userId — a rewrite would hand back a schedule that does something other than what was asked. system:scheduler and blank ids stay exempt so stored schedules and Manager round-trips keep working. - Dream consolidation crossed agent boundaries: process() read getAllEntries(userId) — agent-unscoped — while every knob came from ONE agent's config, so agent A's pruneStaleAfterDays deleted agent B's memories and A's model endpoint saw B's text. Now scoped to the firing agent's own writes, with crossAgentMaintenance as an explicit opt-in. Newly reachable here, because this PR gave process() its first scheduled caller. - The B2 interrupt fix destroyed the bookkeeping it protected. The restore in fire() ran BEFORE logFire(), and the sync Mongo driver throws MongoInterruptedException on connection checkout while the flag is set — so on exactly the interrupt it existed to handle, the FAILED fire log was lost and failCount never incremented. Parked and re-asserted in a finally after the store round trip. The residual half was in SchedulePollerService, which ran markFailed() on the same still-interrupted thread: the schedule stayed CLAIMED with nextFire in the past, was re-claimed every lease expiry, and could never reach maxRetries — an interrupt turned a failing schedule into an unbounded re-fire loop. - A draining node answered 500 instead of a retryable signal, because sayInternal's trailing catch (Exception) swallowed the shutdown RejectedExecutionException — defeating this PR's own graceful-shutdown work. - A single transient LLM failure aborted a whole Dream cycle and marked the fire FAILED, so three consecutive 429s permanently dead-lettered the schedule. Also: the parallel batch deadline was sized at one member ATTEMPT, so it always fired first and made the per-member RETRY/ABORT/attributed-SKIP branches unreachable; maxSummarizationCalls silently stopped being enforced for stored configs (now an explicit backstop, deprecated for maxCostPerRun); BaseRuntime swallowed onComplete failures with no identifying context; and WorkflowStoreClientLibrary documented an invariant nothing enforced — the component key no longer depends on it. Docs verified against the implementation rather than intent (third and fourth instance in this stack): architecture.md told operators to create Dream schedules with an MCP tool that has no metadata parameter and therefore cannot set the marker the dispatcher matches on, so the documented procedure produced a schedule that never consolidated. IEventBus and InMemoryConversationCoordinator both claimed runtime coordinator selection via eddi.messaging.type; it is @IfBuildProfile("nats"), build-time, and that property is read by no Java code. One disagreement adjudicated rather than settled by severity: the critic rated the NATS C13/C10 parity gap CRITICAL, two verifiers refuted it as unreachable in shipped builds. The defect was real and is fixed; the rating was not. Two tests relabelled rather than trusted: both GracefulShutdownService interrupt tests pass identically with and without the fix, so they now say so instead of implying coverage they lack. Full suite: 12,912 tests. The only failures are the 15 known network-dependent classes from the sandbox baseline — none from this change surface.
pull Bot
pushed a commit
to Stars1233/EDDI
that referenced
this pull request
Jul 30, 2026
Two findings Copilot raised against labsai#618 after it was already approved. Kept out of that PR so they land small enough for CodeRabbit to review — labsai#618 grew to 120 files, past CodeRabbit's 100-file limit, and merged without ever receiving a CodeRabbit pass. - WorkflowTraversal: every other malformed-URI branch in that loop warns, marks the traversal degraded and continues. The version parse did not. String.replaceAll returns its input UNCHANGED when the pattern does not match, so a workflow URI with "?version=abc" passed the contains("version=") guard and reached Integer.parseInt as the literal "version=abc". The NumberFormatException escaped discoverConfigs, so one bad workflow URI took out httpcall, mcpcall AND RAG tool discovery for the whole turn instead of skipping the single broken workflow. Now matched explicitly, treating "present but unusable" exactly like "absent"; a digit run too large for an int folds into the same path. - MemoryItemConverter logged raw exception messages in both catch blocks. That text can carry user-controlled values — the same CWE-117 class CodeQL flagged five times in labsai#618. Routed through LogSanitizer. 90 tests pass across WorkflowTraversal, MemoryItemConverter and Deployment.
ginccc
added a commit
that referenced
this pull request
Jul 30, 2026
…e 6.2 polish) #620 branched before three waves of security and correctness fixes landed, so this merge had 20 conflicted files / 58 hunks — and the conflicting files were precisely the ones whose current main versions ARE those fixes. Taking the wrong side anywhere would have reverted shipped security work while still compiling, and in several cases while still passing tests. Resolution was per-hunk, with both sides preserved unless they were genuinely irreconcilable. Two files show why no single rule would have worked: - RestAuditStore: both sides had a head-anchor check. #620 used `skip <= 0 && entries.size() < limit`; main uses `entries.size() == countByConversation(id)`. They are not variants — main's IS the fix for #620's, because getEntries pages NEWEST-first, so skip==0 is the most recent page rather than the start of the chain. Combining them either way provably breaks something: OR reopens the false-BROKEN regression that reported ~990 entries deleted, AND drops prefix detection when count==limit exactly. So main's anchor was taken whole, while #620's DEFAULT_VERIFY_LIMIT and its undelivered-attribution path (INCOMPLETE for gaps the ledger itself caused) were kept. - AuditLedgerService: the opposite shape. #620 refactored the queue-full check into reserveQueueSlot so a back-pressure drop can no longer burn a chain sequence and manufacture a BROKEN verdict; main had added LogSanitizer to the one log line that refactor deletes. Both applied — taking #620 alone would have silently dropped the log-injection hardening. Also resolved: an add/add collision where #618 and #620 each independently created LlmTaskStreamingDowngradeTest.java (merged into one file keeping every distinct test from both), and OutputEntry, where the two compareTo designs are contradictory by construction — main's declaration-order behaviour won, because that is what governs the order of chat bubbles the end user sees. RestAuditStoreTest auto-merged as a UNION of both sides and so got no conflict and no scrutiny — which left #620's deletedHeadEntryIsDetected asserting against the superseded heuristic without stubbing countByConversation. Mockito returned 0, the anchor never engaged, and the report came back INTACT. The test was stale, not the code; fixed by adding the stub rather than by restoring the old anchor, which is the tempting "fix" that would revert #617. Kept rather than deleted as a duplicate, because only that copy asserts tamperingSuspected(). Verification, because a green build proves very little on a merge like this: - full suite 13,312 tests — the only non-environmental failure was the audit test above, now fixed (734 tests green across every resolved area); - all 20 shipped fixes explicitly checked still present, by pattern where possible and by reading where not: the @?? jsonpath escape, the 63-byte index truncation, A2 caller-ownership on attachments, clampSkip, the whole-conversation anchor, MAX_REPORTED_MISSING, duplicates-are-BROKEN, SEQUENCE_ORIGIN, supportsSequence gating, global entries keeping their owning agent, most_accessed recency reservation, LlmTask credential isolation, cross-server MCP dedupe, delegation-depth propagation, the identity capture outside the lambda, the C11 release in a finally, the shutdown accept gate, the v6 rename's existing-empty-target handling, and OutputEntry's declaration order. One improvement came out of #620's side rather than main's: the delegation context is now handed to InputData as a mutable copy instead of the immutable Map.of, which closes the risk flagged when that fix was written.
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.
Stacked on #617 → #616. Targets
fix/code-review-access-control, so this diff shows only wave 2b. 97 files, under CodeRabbit's 100-file limit.The tool pipeline had a second door
executeToolWrappedis genuinely well-built — one production call site, all seven tool sources routed through it. ExceptMcpCallsTask, a behaviour-rule-triggered lifecycle task invoking the same external MCP tools, which resolved aToolExecutorand called it directly. It also bypassedToolApprovalGate, whose only production caller wasAgentOrchestrator— sohitlConfig.toolApprovalsgated LLM-initiated tool calls and not rule-initiated ones. A human-approval gate with a hole in it. (F14)F18 —
converse_with_agenthad no guardrails at all. It never consultedDynamicAgentConfig, andallowDelegationwas never checked anywhere in the codebase. Agent A calls B, which calls A — and because noconversationIdis passed, each hop starts a fresh conversation, so the busy-guard never breaks the cycle. Depth and cost grew unbounded. Prompt injection in a user message was sufficient to start it.F17 —
maxCreatedAgentsPerDiscussionwas enforced per turn, not per discussion.sharedCreatedIdswas created fresh in everybuildToolListcall. A 5-member × 3-phase discussion with the default cap of 5 permitted up to 75 agents deployed to production.Two real bugs the tests caught that the review did not
The review's own F12 fix (cache the workflow traversal that runs 3–4× per LLM task per turn) introduced two defects. They only surfaced because
WorkflowTraversalTeststarted returning 0 instead of 1 — and because the triage pass asked "stale test, bad fixture, or real bug?" for every failure instead of adjusting tests until green:(List<StepConfig<T>>), justified by a comment asserting a 1:1 mapping that nothing enforces. A future caller asking for the same step type with a different class would get another caller's entry and aClassCastExceptionfrom a cache hit with no connection to the calling code.F13 was "fixed" but unreachable — worth a look
The wave-2 agent added
inheritedParametersoverloads toSummarizationServiceand wrote tests that exercised them directly. Those tests passed. But no caller insrc/mainever passed the parameter —ConversationSummarizerstill called the 4-arg overload. So the rolling summary still could not authenticate and still silently never materialised: the exact bug F13 describes, with green tests over it.This is the failure mode workstream J is about, and it's why the finding is only closed now that the parameters are threaded
LlmTask → ConversationSummarizer → SummarizationService, with a test at each hop that fails if they're dropped. Mutation-verified.DreamServicedeliberately remains on the un-inherited path. It's a background job with no parent task to inherit from, so it needs a credential source of its own — that's part of wiring Dream up (finding I1) in wave 3. Flagging rather than half-fixing.Memory & properties
scope: "secret"failed OPEN to plaintext. On vault failure the method returned the plaintext before the scrub block, so the secret persisted twice: as a conversation property and as rawinput:initialdata. Vault-disabled is the default (eddi.vault.master-keyships empty), so this was the common path, not an edge case.replaceOnewith no upsert whoseUpdateResultwas discarded, so a conversation deleted mid-turn by erasure or retention discarded the turn while the caller got a normal response.most_accessedrecall did an N+1 write inside an open read cursor and was self-reinforcing (only already-top-N entries were incremented, so a new entry could never climb in). AndupdatedAtwas refreshed on every longTerm property every turn, somost_recentdegenerated to "everything is recent" anddeleteOlderThannever expired anything for an active user.Migration
valuefield unconditionally but only wrote a replacement for String/Map/Integer/Float. Doubles, longs, booleans and arrays were dropped with no error.{...+...}sequence, corrupting JSON bodies and arithmetic that merely sat in a document containing Thymeleaf syntax.Verification
LlmTaskhop.