fix(security): access control, A2A ownership, GDPR & audit ledger (wave 2a) - #617
Merged
Conversation
…ership, GDPR & audit ledger
The guard triad (OwnershipValidator, ConversationAccessGuard, HitlAccessGuard)
already existed and is used correctly elsewhere. Every finding here is a place
that never called it.
Access control
- A1 (critical): the SSE turn endpoint had NO ownership check while its
non-streaming twin did, so a turn executed under the target conversation's
userId — loading that user's long-term memories into the prompt and running
tool calls in their context. The guard now lives in ConversationService.say/
sayStreaming, before the memory snapshot loads, so no future REST adapter can
omit it; the REST layer also checks so denial is a 403 rather than an error
event on an already-200 SSE stream.
- A2 (critical): attachment endpoints authorised the path parameter, not the
caller — the store checks that the NAMED conversation owns the blob, and the
caller supplies that name, so the check was self-satisfying.
- A3: ?deleteOlderThanDays=0 permanently deleted every ended conversation in the
deployment, from an endpoint with no role.
- A4: unroled tool control plane — rate-limiter reset, cost-budget reset, and a
history endpoint dumping raw tool arguments and results for any conversation.
- A5: /propertiesstore/properties/{userId} had neither role nor ownership check
over the same store RestUserMemoryStore guards on all nine of its methods.
- A6: returned another user's live conversationId — the discovery half of A1/A3.
- A7: template preview read any conversation's memory and returned a flattened
properties/context/memory dump, with a caller-supplied template.
- A8: config stores with full CRUD and no role. The review named one; the real
inventory is six, including two it missed. IRestVersionInfo deliberately left
alone (no @path, it is a mixin) — reasoning recorded in code.
- A9: A2A sat outside the ownership model entirely — caches keyed on a
caller-supplied id, contextId as an unauthenticated read+write handle,
conversations created with userId=null (treated as "legacy — allow", so
permanently unowned), and raw exception messages returned to peers.
- A12: the exception mapper returned the raw driver message as the 500 body.
GDPR & audit
- G14: erasure served stale indefinitely from a cache with no TTL that erasure
never invalidated.
- G15: the cascade missed checkpoints (carrying PII), group transcripts and
schedules, while the docs asserted it covered all data stores.
- G16: verifyHmac had zero production callers — the docs told operators to
recompute the HMAC and the product shipped no way to do it.
- G17: pseudonymisation rewrote the signed userId without recomputing the HMAC,
so every routine erasure produced rows indistinguishable from tampered ones.
- G18: no hash chain, so deletion and reordering were undetectable.
- G19: entries written unsigned by default; no startup check.
- G20: unbounded audit queue that re-offered failed batches into itself.
Docs (I7-I13): removed a documented stemming extension that does not exist and
would prevent an agent from starting, corrected the expression table to what the
code emits, and fixed two docs that contradicted AGENTS.md 5.1 on the template
model.
Full suite: 12,384 tests, 0 non-environmental failures. A1, A2 and F18 were
mutation-checked (revert the fix, confirm a test fails, restore).
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:
📝 WalkthroughWalkthroughThe PR adds REST authorization and ownership checks, peer-scoped A2A task handling, sequenced v3 audit verification, expanded GDPR erasure, opaque error responses, retention validation, and documentation updates for templates, behavior rules, and semantic-parser configuration. ChangesDocumentation alignment
Authorization and A2A isolation
Audit and compliance
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
This was referenced Jul 28, 2026
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/ai/labs/eddi/engine/triggermanagement/rest/RestUserConversationStore.java (1)
71-75: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
createUserConversationvalidates the pathuserIdbut persists the body's userId.
validateUserAccess(identity, userId)guards the path parameter, whileuserConversationStore.createUserConversation(userConversation)writes whateveruserConversation.getUserId()holds, and the cache is keyed on the path values. A body/path mismatch therefore stores a mapping under an unvalidated user while caching it under the caller's key. Rejecting the mismatch (or validating the body value too) keeps the guard and the persisted row aligned.🛡️ Proposed fix
public Response createUserConversation(String intent, String userId, UserConversation userConversation) { ownershipValidator.validateUserAccess(identity, userId); + if (userConversation != null && userConversation.getUserId() != null + && !userId.equals(userConversation.getUserId())) { + throw new BadRequestException("userId in path and body must match"); + } + ownershipValidator.validateUserAccess(identity, userConversation.getUserId()); try {🤖 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/triggermanagement/rest/RestUserConversationStore.java` around lines 71 - 75, Update createUserConversation in RestUserConversationStore to reject requests when userConversation.getUserId() does not match the path userId, before persisting or caching. Keep ownershipValidator.validateUserAccess tied to the path userId, and only call userConversationStore.createUserConversation and userConversationCache.put after the values are aligned.
🧹 Nitpick comments (19)
src/main/java/ai/labs/eddi/engine/a2a/RestA2AEndpoint.java (1)
202-213: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider counting the two error classes separately.
Since peers now only see a fixed
INTERNAL_ERROR_MESSAGE, the split between "peer sent garbage" and "we broke" is only visible in logs. Two Micrometer counters (initialized in@PostConstruct) tagged by method would make A2A error rates alertable.As per coding guidelines, "Add Micrometer counters, timers, or gauges to new features for observability, and initialize reusable metrics in
@PostConstruct".🤖 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/a2a/RestA2AEndpoint.java` around lines 202 - 213, Add separate Micrometer counters for InvalidA2ARequestException and unexpected Exception handling in RestA2AEndpoint, tagged by the A2A method. Initialize the reusable counters in the endpoint’s `@PostConstruct` method, then increment the corresponding counter in each catch block before returning the JSON-RPC error.Source: Coding guidelines
src/main/java/ai/labs/eddi/engine/a2a/A2ATaskHandler.java (1)
216-240: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd an expiring lifetime for these A2A conversation mappings.
taskConversationCacheandcontextConversationCacheare created withgetCache(CACHE_NAME)overloads, which cap size but do not apply TTLs; entries put without a lifespan are never expired on their own. Since each freshtaskId/contextIdadds a peer-scoped entry, use a TTL-aware cache such ascacheFactory.getCache(..., Duration.ofMinutes(...))orICache.put(..., lifespan, unit).🤖 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/a2a/A2ATaskHandler.java` around lines 216 - 240, Add a finite TTL to entries managed by resolveConversation in both taskConversationCache and contextConversationCache. Configure these caches with the duration-aware getCache overload or supply a lifespan to each put, including mappings reused from contextId and newly created conversation mappings, while preserving peer-scoped keys and existing lookup behavior.Source: Coding guidelines
src/main/java/ai/labs/eddi/engine/gdpr/GdprComplianceService.java (3)
228-239: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winOne failing conversation aborts the remaining checkpoint deletions.
The
trywraps the whole loop, so a singledeleteByConversationIdfailure leaves every later conversation's checkpoint — i.e. a full copy of the conversation properties — un-erased, with only an ERROR log. Isolating per conversation (asexportUserDatadoes for attachments) maximizes what the erasure actually removes.♻️ Per-conversation isolation
- try { - for (String convId : conversationIds) { - checkpointsDeleted += checkpointStore.deleteByConversationId(convId); - } - if (checkpointsDeleted > 0) { - LOGGER.infof("[GDPR] Deleted %d conversation checkpoints [%s]", - checkpointsDeleted, pseudonym); - } - } catch (Exception e) { - LOGGER.errorf(e, "[GDPR] Failed to delete conversation checkpoints [%s]", - pseudonym); - } + for (String convId : conversationIds) { + try { + checkpointsDeleted += checkpointStore.deleteByConversationId(convId); + } catch (Exception e) { + LOGGER.errorf(e, "[GDPR] Failed to delete checkpoints for conversation %s [%s]", + convId, pseudonym); + } + } + if (checkpointsDeleted > 0) { + LOGGER.infof("[GDPR] Deleted %d conversation checkpoints [%s]", + checkpointsDeleted, pseudonym); + }🤖 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/gdpr/GdprComplianceService.java` around lines 228 - 239, Update the checkpoint deletion loop in GdprComplianceService so each conversation’s deleteByConversationId call is isolated in its own try/catch, allowing later conversation IDs to continue when one deletion fails. Preserve the per-conversation error logging and aggregate successful deletion counts for the existing summary log.
97-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the cache name + key format into one shared place.
The duplicated
intent::userIdcontract silently breaks erasure ifRestUserConversationStore.calculateCacheKeyever changes; a pinning test detects it only if it keeps up. A small shared helper (e.g. in the triggermanagement package, referenced by both) removes the divergence risk without making the GDPR cascade depend on a REST resource.🤖 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/gdpr/GdprComplianceService.java` around lines 97 - 113, The cache name and key-format contract is duplicated between GdprComplianceService and RestUserConversationStore, allowing them to diverge. Extract USER_CONVERSATION_CACHE_NAME and the user-conversation cache-key construction into a shared helper in the triggermanagement package, then update GdprComplianceService and RestUserConversationStore to reuse it while keeping the GDPR cascade independent of the REST resource.
290-300: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the GDPR deletion method to the group conversation store interface.
GdprComplianceServiceinjects the concreteGroupConversationStoreand callsdeleteAllForUser(userId)through it, butIGroupConversationStoreonly exposes the CRUD/LCAS methods. AdddeleteAllForUsertoIGroupConversationStore, injectInstance<IGroupConversationStore>here, and have the Mongo implementation forward the call. This keeps the erasure cascade aligned with the existing store abstraction.🤖 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/gdpr/GdprComplianceService.java` around lines 290 - 300, Expose deleteAllForUser in IGroupConversationStore, change GdprComplianceService to inject and resolve Instance<IGroupConversationStore> rather than the concrete GroupConversationStore, and update the Mongo group conversation store implementation to forward this method to its persistence layer while preserving the existing GDPR deletion flow.src/test/java/ai/labs/eddi/engine/schedule/mongo/MongoScheduleStoreTest.java (1)
402-405: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport
ArgumentCaptorinstead of inlining the FQN.Also consider asserting the filter equals
Filters.eq("userId", "user-1")rather than atoString().contains(...)substring, which would also pass for a filter that merely mentions the field.♻️ Suggested tightening
- var captor = org.mockito.ArgumentCaptor.forClass(Bson.class); + var captor = ArgumentCaptor.forClass(Bson.class); verify(scheduleCollection).deleteMany(captor.capture()); - assertTrue(captor.getValue().toString().contains("userId"), - "the filter must scope the delete to the user: " + captor.getValue()); + assertEquals(Filters.eq("userId", "user-1"), captor.getValue(), + "the filter must scope the delete to the user");with
import org.mockito.ArgumentCaptor;andimport com.mongodb.client.model.Filters;added.As per coding guidelines: "Use simple names with top-level imports; do not inline fully qualified names except when disambiguating unavoidable same-named types."
🤖 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/schedule/mongo/MongoScheduleStoreTest.java` around lines 402 - 405, Update MongoScheduleStoreTest to import org.mockito.ArgumentCaptor and use the simple ArgumentCaptor name instead of the fully qualified reference. Also import com.mongodb.client.model.Filters and strengthen the deleteMany verification assertion to compare the captured filter with Filters.eq("userId", "user-1") rather than checking its string representation.Source: Coding guidelines
src/test/java/ai/labs/eddi/engine/gdpr/GdprComplianceServiceTest.java (1)
770-876: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSolid regression coverage — the cache test drives the real Caffeine instance through the real REST store.
One gap: no test asserts the eviction key actually matches
RestUserConversationStore's format for a non-default intent shape (e.g. an intent containing:), which is the one way the duplicated key contract can silently diverge.🤖 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/gdpr/GdprComplianceServiceTest.java` around lines 770 - 876, The cache invalidation tests cover only the default intent shape; extend deleteUserData cache coverage with an intent containing “:” and assert a post-erasure read through RestUserConversationStore misses the cache. Reuse the same mapping and cache-warming flow in deleteUserData_invalidatesCachedConversationMappings, ensuring the eviction key matches the store’s format for this non-default intent.src/main/java/ai/labs/eddi/engine/exception/ResourceStoreExceptionMapper.java (1)
30-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe opaque-error body format is authored twice. Both sites independently build
"Internal server error (correlationId: <uuid>)"and log the detail under that id; a change to the wording or the id format in one place silently desynchronizes the other (and the assertions that key offGENERIC_MESSAGEvs the literal"correlationId:").
src/main/java/ai/labs/eddi/engine/exception/ResourceStoreExceptionMapper.java#L30-L39: promoteGENERIC_MESSAGEand the "log under a fresh correlation id, return message + id" step into a small shared helper (e.g.OpaqueError.logAndDescribe(log, context, e)) and consume it here.src/main/java/ai/labs/eddi/engine/internal/RestAgentManagement.java#L122-L136: replace the privatelogAndBuildOpaqueMessagebody with a call to that shared helper instead of re-spelling the message and id format.🤖 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/exception/ResourceStoreExceptionMapper.java` around lines 30 - 39, Create a shared OpaqueError helper that owns the GENERIC_MESSAGE value and the correlation-id logging plus opaque response-message construction, then update ResourceStoreExceptionMapper.java lines 30-39 to use it. Also replace RestAgentManagement.java lines 122-136 private logAndBuildOpaqueMessage implementation with the same helper, preserving the existing logging context and exception details so both sites share one message and ID format.src/test/java/ai/labs/eddi/engine/audit/AuditHmacTest.java (1)
672-680: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThird copy of the same raw-HMAC helper.
signhere is byte-identical tolegacySignV1(Line 237) andlegacySign(Line 575). Hoisting oneprivate static String rawHmac(String canonical)to the class level would remove two copies.🤖 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/audit/AuditHmacTest.java` around lines 672 - 680, Consolidate the duplicate HMAC implementation by adding one class-level private static rawHmac(String canonical) helper, then replace sign, legacySignV1, and legacySign with calls to it while preserving their existing behavior and exception handling.src/main/java/ai/labs/eddi/engine/audit/rest/IRestAuditStore.java (1)
96-124: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffNew verification endpoints run a bounded-but-heavy sweep synchronously on the request thread.
Each call does a store read plus a per-entry canonicalization + HMAC recomputation for up to 1000 entries. The repository convention is to hand REST work off rather than block; consider an
AsyncResponse-based signature (or a reactive return type) for these two, since they are the heaviest operations on this interface.As per coding guidelines: "Backend code must be thread-safe and non-blocking; use
AsyncResponsefor REST endpoints and avoid extended blocking in task execution."🤖 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/audit/rest/IRestAuditStore.java` around lines 96 - 124, The synchronous verifyConversation and verifyAgent REST methods should be converted to non-blocking asynchronous endpoints. Update both signatures to accept an AsyncResponse parameter, return immediately, and dispatch the bounded store read and per-entry verification work asynchronously, resuming the response with the AuditVerificationReport or failure.Source: Coding guidelines
src/main/java/ai/labs/eddi/engine/compliance/ComplianceStartupChecks.java (1)
85-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a gauge alongside the warning.
"Ledger is unsigned" is exactly the kind of state operators want to alert on, and a startup log line is easy to scroll past. A
eddi_audit_signing_enabledgauge would make it queryable.As per coding guidelines: "Add Micrometer counters, timers, or gauges to new features for observability."
🤖 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/compliance/ComplianceStartupChecks.java` around lines 85 - 107, Add a Micrometer gauge named eddi_audit_signing_enabled in ComplianceStartupChecks, registering it with the existing MeterRegistry and exposing 1 when audit signing is enabled and 0 otherwise. Preserve the current unsigned-ledger warning and startup behavior while making the signing state queryable.Source: Coding guidelines
src/main/java/ai/labs/eddi/engine/audit/rest/RestAuditStore.java (2)
131-132: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
sequencesis already sorted — usegetLast().The stream
maxis a redundant O(n) pass (and the sole reason for theComparatorimport).♻️ Simplify
long first = sequences.getFirst(); - long last = sequences.stream().max(Comparator.naturalOrder()).orElse(first); + long last = sequences.getLast();🤖 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/audit/rest/RestAuditStore.java` around lines 131 - 132, Update the sequence range calculation in RestAuditStore to obtain the final value directly with sequences.getLast() instead of streaming with max and Comparator.naturalOrder(). Remove the now-unused Comparator import while preserving the existing first-value handling.
33-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo metrics for the new verification feature.
A sweep is expensive and security-relevant, yet nothing records how often it runs, how long it takes, or how many rows came back invalid. Add a Micrometer timer plus counters (e.g. invalid/unsigned/broken-chain outcomes) initialized in
@PostConstruct.As per coding guidelines: "Add Micrometer counters, timers, or gauges to new features for observability, and initialize reusable metrics in
@PostConstruct."🤖 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/audit/rest/RestAuditStore.java` around lines 33 - 43, Add Micrometer observability to RestAuditStore for verification sweeps: define reusable timer and counters for sweep executions/duration and invalid, unsigned, and broken-chain outcomes, then initialize them in an `@PostConstruct` method. Update the verification flow to record elapsed time and increment the appropriate outcome counters for each sweep result.Source: Coding guidelines
src/test/java/ai/labs/eddi/engine/audit/AuditStoreTest.java (1)
212-212: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInlined
org.mockito.ArgumentCaptorin both new test files. The shared root cause is a missing top-level import, which the guidelines disallow in favour of simple names.
src/test/java/ai/labs/eddi/engine/audit/AuditStoreTest.java#L212-L212: addimport org.mockito.ArgumentCaptor;and useArgumentCaptor.forClass(Document.class).src/test/java/ai/labs/eddi/engine/audit/AuditLedgerServiceTest.java#L271-L271: add the same import and replace the fully qualified usages in the new sequence/signing tests (Lines 271, 291, 311, 328, 353) withArgumentCaptor.forClass(List.class).As per coding guidelines: "Use simple names with top-level imports; do not inline fully qualified names except when disambiguating unavoidable same-named types."
🤖 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/audit/AuditStoreTest.java` at line 212, Add the top-level org.mockito.ArgumentCaptor import in AuditStoreTest.java and replace its fully qualified usage with ArgumentCaptor.forClass(Document.class). In AuditLedgerServiceTest.java, add the same import and replace all fully qualified ArgumentCaptor usages in the sequence/signing tests at lines 271, 291, 311, 328, and 353 with the simple name and existing List.class arguments.Source: Coding guidelines
src/test/java/ai/labs/eddi/engine/audit/AuditLedgerServiceTest.java (1)
212-227: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test never reaches the stalled-store path it claims to cover.
flush()is never called, soappendBatchis never invoked and thedoThrowstub is dead — the assertions are satisfied purely by the submit-path bound, making this a duplicate ofsubmitsPastTheBoundAreDropped(Lines 229-241). The regression described in the javadoc (a failed flush re-offering its batch and feeding itself) is left uncovered.Interleaving flushes would exercise
offerBoundedon the retry path:💚 Drive the retry path
for (int i = 0; i < 200; i++) { svc.submit(entry("id-" + i, "conv-1", "agent-1")); + if (i % 20 == 0) { + svc.flush(); // fails and re-offers, which must also respect the bound + } } - - assertEquals(5, svc.getQueueSize(), "the queue must stay at its bound instead of accumulating every entry"); - assertEquals(195.0, meterRegistry.counter("eddi_audit_entries_dropped_total").count(), - "dropped entries must be counted, not silently lost"); + + assertTrue(svc.getQueueSize() <= 5, "the queue must stay at its bound instead of accumulating every entry");🤖 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/audit/AuditLedgerServiceTest.java` around lines 212 - 227, Update stalledStoreDoesNotGrowTheQueueWithoutLimit to invoke flush() after submitting entries, ensuring the configured auditStore.appendBatch failure is exercised and the failed batch is re-offered through the retry path. Keep the queue-size and dropped-entry assertions focused on bounded behavior during that retry, distinguishing this test from submitsPastTheBoundAreDropped.src/main/java/ai/labs/eddi/engine/audit/AuditLedgerService.java (2)
288-297: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe reverse loop does not put entries "at the front".
ConcurrentLinkedQueue.offer()appends at the tail, so iterating the batch backwards simply re-queues it in reverse order rather than restoring head position. Sequences and HMACs are already assigned so nothing breaks, but the comment is wrong and the reversal makes the queue order worse than a plain forward loop would.♻️ Forward iteration with an accurate comment
- // Re-queue entries at the front so the next flush retries them. - // The re-offer respects the bound: whatever no longer fits goes - // straight to the dead-letter sink instead of growing the heap. + // Re-queue entries so the next flush retries them. offer() + // appends at the tail, so keep the original order; the + // re-offer respects the bound and whatever no longer fits + // goes straight to the dead-letter sink. List<AuditEntry> rejected = new ArrayList<>(); - for (int i = batch.size() - 1; i >= 0; i--) { - if (!offerBounded(batch.get(i))) { - rejected.add(batch.get(i)); + for (AuditEntry pending : batch) { + if (!offerBounded(pending)) { + rejected.add(pending); } }🤖 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/audit/AuditLedgerService.java` around lines 288 - 297, Update the re-queue loop in the batch retry logic to iterate batch in forward order, since offerBounded appends entries to the queue tail rather than the front. Revise the nearby comment to accurately describe re-queuing for retry without claiming front insertion, while preserving rejected-entry handling and the existing warning count.
221-232: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThrottle the drop log.
Once the queue saturates, every subsequent
submit()emits a WARN — at audit-submission rates that floods the log with an already-metered signal. Logging only on the transition into the full state (and again when it clears) keeps the diagnostic without the flood;eddi_audit_entries_dropped_totalcarries the volume.🤖 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/audit/AuditLedgerService.java` around lines 221 - 232, Throttle the WARN in offerBounded so it is emitted only when the queue transitions into the full state, rather than for every rejected entry; continue incrementing droppedCounter for every drop. Track the full-state transition and reset that state once capacity becomes available, allowing the next saturation to log again while preserving the existing queue behavior.src/test/java/ai/labs/eddi/engine/audit/rest/RestAuditStoreTest.java (1)
159-169: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider covering
duplicateSequences.
checkChainpopulates duplicates but no test asserts them, so the multi-writer case documented inAuditVerificationReport(two nodes seeding the same counter) is unverified — and it staysINTACT, which is a behavior worth pinning down.🤖 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/audit/rest/RestAuditStoreTest.java` around lines 159 - 169, Extend the RestAuditStoreTest coverage around verifyConversation and checkChain with a duplicate-sequence case representing two entries using the same sequence number. Assert that the resulting AuditVerificationReport exposes the duplicateSequences collection with the expected duplicate value while retaining ChainStatus.INTACT and an intact report.src/test/java/ai/labs/eddi/configs/properties/rest/RestPropertiesStoreTest.java (1)
176-183: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winReflection-only assertion cannot detect an inert role gate.
This passes as long as the annotation is present on the interface, which is exactly the case that may not be enforced at runtime. A
@QuarkusTestwith@TestSecurityhitting the endpoint with a non-matching role would actually prove the 403. Related to the interface-placement concern raised onIRestPropertiesStore.🤖 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/properties/rest/RestPropertiesStoreTest.java` around lines 176 - 183, Replace the reflection-only test restInterfaceShouldCarryTheSameRoleGateAsTheUserMemoryStore with a `@QuarkusTest` integration test that invokes the properties endpoint under `@TestSecurity` using a non-matching role and asserts HTTP 403. Keep coverage for the authorized eddi-admin/eddi-user roles as appropriate, and target the actual REST endpoint rather than only inspecting IRestPropertiesStore annotations.
🤖 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/properties.md`:
- Line 154: Resolve the Markdown lint violations in the documentation examples:
in docs/properties.md at lines 154-154 and 160-160, add a text language
identifier to the fenced blocks; in docs/conversation-memory.md at lines
526-526, remove the blank line inside the blockquote; and in
docs/semantic-parser.md at lines 267-267, add the text language identifier.
In `@src/main/java/ai/labs/eddi/configs/properties/IRestPropertiesStore.java`:
- Around line 27-29: Move the `@RolesAllowed` gates from IRestPropertiesStore and
IRestUserConversationStore onto the corresponding RestPropertiesStore,
RestUserConversationStore, and RestTemplatePreview implementation classes or
methods so Quarkus enforces them; update RestPropertiesStoreTest and
RestTemplatePreviewTest to replace reflection-only annotation assertions with
role-based requests that verify unauthorized roles receive 403, covering each
affected endpoint and preserving the intended eddi-admin/eddi-user permissions.
In `@src/main/java/ai/labs/eddi/engine/audit/AuditHmac.java`:
- Around line 85-120: Replace the unsalted SHA-256 implementation used by
pseudonymFor with a deterministic HMAC-based token derived through the existing
deriveHmacKey path and vault master key. Preserve identityToken’s handling of
already-pseudonymized values and ensure the resulting token remains stable for
the same userId while matching the established GDPR pseudonym format and v3
signing behavior.
In `@src/main/java/ai/labs/eddi/engine/audit/AuditLedgerService.java`:
- Around line 187-191: Stop consuming conversation sequence numbers during
submission in the flow using nextSequence; entries rejected by offerBounded or
abandoned by flush must not create gaps. Assign the sequence in flush
immediately before signing and persistence, so only entries that will be written
receive a signed sequence. Preserve the existing chain ordering and
signed-payload behavior.
- Around line 247-252: Update the MAX_TRACKED_CONVERSATIONS handling in the
sequence-table management flow so conversationSequences is not cleared while
pending queue entries remain unpersisted. Before clearing, flush or otherwise
drain the queued writes and ensure they are persisted successfully, then reseed;
preserve sequence uniqueness for active conversations and retain the existing
bounded-table behavior.
In `@src/main/java/ai/labs/eddi/engine/audit/rest/RestAuditStore.java`:
- Around line 60-72: Make verifyConversation and verifyAgent non-blocking by
updating their IRestAuditStore and RestAuditStore contracts to accept an
AsyncResponse and execute entry retrieval and HMAC verification off the request
thread. Preserve the existing verification modes and response results, and
resume the AsyncResponse with success or failure once the sweep completes.
- Around line 116-140: Update checkChain and the verifyConversation flow to
anchor the expected sequence range to the conversation’s stored count via
auditStore.countByConversation, rather than deriving both boundaries solely from
returned entries. Ensure missing newest or oldest entries are reported, and
propagate whether the skip/limit sweep was truncated so a partial window is not
reported as INTACT; preserve existing duplicate and interior-gap detection.
In `@src/main/java/ai/labs/eddi/engine/gdpr/GdprComplianceService.java`:
- Around line 105-110: Update the Javadoc near the duplicated cache-key format
to reference the existing test method
deleteUserData_invalidatesCachedConversationMappings instead of the nonexistent
erasureUsesTheSameCacheKeyAsTheRestStore, preserving the explanation that the
test keeps both key formats aligned.
In `@src/main/java/ai/labs/eddi/engine/internal/RestAgentManagement.java`:
- Around line 107-119: Update the 4xx branch in the exception handling around
response.resume to fall back to the HTTP status reason phrase when
e.getLocalizedMessage() is null. Preserve authored 4xx messages when present and
leave the existing logAndBuildOpaqueMessage handling for non-4xx responses
unchanged.
In `@src/main/java/ai/labs/eddi/engine/schedule/IScheduleStore.java`:
- Around line 86-101: The default deleteSchedulesByUserId implementation must
not report successful completion when readAllSchedules(ERASURE_SCAN_LIMIT)
returns a full page. Add a truncation guard that detects a result size equal to
ERASURE_SCAN_LIMIT and throws the appropriate ResourceStoreException before or
instead of returning a potentially incomplete deletion count; otherwise preserve
the existing matching deletion behavior.
---
Outside diff comments:
In
`@src/main/java/ai/labs/eddi/engine/triggermanagement/rest/RestUserConversationStore.java`:
- Around line 71-75: Update createUserConversation in RestUserConversationStore
to reject requests when userConversation.getUserId() does not match the path
userId, before persisting or caching. Keep ownershipValidator.validateUserAccess
tied to the path userId, and only call
userConversationStore.createUserConversation and userConversationCache.put after
the values are aligned.
---
Nitpick comments:
In `@src/main/java/ai/labs/eddi/engine/a2a/A2ATaskHandler.java`:
- Around line 216-240: Add a finite TTL to entries managed by
resolveConversation in both taskConversationCache and contextConversationCache.
Configure these caches with the duration-aware getCache overload or supply a
lifespan to each put, including mappings reused from contextId and newly created
conversation mappings, while preserving peer-scoped keys and existing lookup
behavior.
In `@src/main/java/ai/labs/eddi/engine/a2a/RestA2AEndpoint.java`:
- Around line 202-213: Add separate Micrometer counters for
InvalidA2ARequestException and unexpected Exception handling in RestA2AEndpoint,
tagged by the A2A method. Initialize the reusable counters in the endpoint’s
`@PostConstruct` method, then increment the corresponding counter in each catch
block before returning the JSON-RPC error.
In `@src/main/java/ai/labs/eddi/engine/audit/AuditLedgerService.java`:
- Around line 288-297: Update the re-queue loop in the batch retry logic to
iterate batch in forward order, since offerBounded appends entries to the queue
tail rather than the front. Revise the nearby comment to accurately describe
re-queuing for retry without claiming front insertion, while preserving
rejected-entry handling and the existing warning count.
- Around line 221-232: Throttle the WARN in offerBounded so it is emitted only
when the queue transitions into the full state, rather than for every rejected
entry; continue incrementing droppedCounter for every drop. Track the full-state
transition and reset that state once capacity becomes available, allowing the
next saturation to log again while preserving the existing queue behavior.
In `@src/main/java/ai/labs/eddi/engine/audit/rest/IRestAuditStore.java`:
- Around line 96-124: The synchronous verifyConversation and verifyAgent REST
methods should be converted to non-blocking asynchronous endpoints. Update both
signatures to accept an AsyncResponse parameter, return immediately, and
dispatch the bounded store read and per-entry verification work asynchronously,
resuming the response with the AuditVerificationReport or failure.
In `@src/main/java/ai/labs/eddi/engine/audit/rest/RestAuditStore.java`:
- Around line 131-132: Update the sequence range calculation in RestAuditStore
to obtain the final value directly with sequences.getLast() instead of streaming
with max and Comparator.naturalOrder(). Remove the now-unused Comparator import
while preserving the existing first-value handling.
- Around line 33-43: Add Micrometer observability to RestAuditStore for
verification sweeps: define reusable timer and counters for sweep
executions/duration and invalid, unsigned, and broken-chain outcomes, then
initialize them in an `@PostConstruct` method. Update the verification flow to
record elapsed time and increment the appropriate outcome counters for each
sweep result.
In `@src/main/java/ai/labs/eddi/engine/compliance/ComplianceStartupChecks.java`:
- Around line 85-107: Add a Micrometer gauge named eddi_audit_signing_enabled in
ComplianceStartupChecks, registering it with the existing MeterRegistry and
exposing 1 when audit signing is enabled and 0 otherwise. Preserve the current
unsigned-ledger warning and startup behavior while making the signing state
queryable.
In
`@src/main/java/ai/labs/eddi/engine/exception/ResourceStoreExceptionMapper.java`:
- Around line 30-39: Create a shared OpaqueError helper that owns the
GENERIC_MESSAGE value and the correlation-id logging plus opaque
response-message construction, then update ResourceStoreExceptionMapper.java
lines 30-39 to use it. Also replace RestAgentManagement.java lines 122-136
private logAndBuildOpaqueMessage implementation with the same helper, preserving
the existing logging context and exception details so both sites share one
message and ID format.
In `@src/main/java/ai/labs/eddi/engine/gdpr/GdprComplianceService.java`:
- Around line 228-239: Update the checkpoint deletion loop in
GdprComplianceService so each conversation’s deleteByConversationId call is
isolated in its own try/catch, allowing later conversation IDs to continue when
one deletion fails. Preserve the per-conversation error logging and aggregate
successful deletion counts for the existing summary log.
- Around line 97-113: The cache name and key-format contract is duplicated
between GdprComplianceService and RestUserConversationStore, allowing them to
diverge. Extract USER_CONVERSATION_CACHE_NAME and the user-conversation
cache-key construction into a shared helper in the triggermanagement package,
then update GdprComplianceService and RestUserConversationStore to reuse it
while keeping the GDPR cascade independent of the REST resource.
- Around line 290-300: Expose deleteAllForUser in IGroupConversationStore,
change GdprComplianceService to inject and resolve
Instance<IGroupConversationStore> rather than the concrete
GroupConversationStore, and update the Mongo group conversation store
implementation to forward this method to its persistence layer while preserving
the existing GDPR deletion flow.
In
`@src/test/java/ai/labs/eddi/configs/properties/rest/RestPropertiesStoreTest.java`:
- Around line 176-183: Replace the reflection-only test
restInterfaceShouldCarryTheSameRoleGateAsTheUserMemoryStore with a `@QuarkusTest`
integration test that invokes the properties endpoint under `@TestSecurity` using
a non-matching role and asserts HTTP 403. Keep coverage for the authorized
eddi-admin/eddi-user roles as appropriate, and target the actual REST endpoint
rather than only inspecting IRestPropertiesStore annotations.
In `@src/test/java/ai/labs/eddi/engine/audit/AuditHmacTest.java`:
- Around line 672-680: Consolidate the duplicate HMAC implementation by adding
one class-level private static rawHmac(String canonical) helper, then replace
sign, legacySignV1, and legacySign with calls to it while preserving their
existing behavior and exception handling.
In `@src/test/java/ai/labs/eddi/engine/audit/AuditLedgerServiceTest.java`:
- Around line 212-227: Update stalledStoreDoesNotGrowTheQueueWithoutLimit to
invoke flush() after submitting entries, ensuring the configured
auditStore.appendBatch failure is exercised and the failed batch is re-offered
through the retry path. Keep the queue-size and dropped-entry assertions focused
on bounded behavior during that retry, distinguishing this test from
submitsPastTheBoundAreDropped.
In `@src/test/java/ai/labs/eddi/engine/audit/AuditStoreTest.java`:
- Line 212: Add the top-level org.mockito.ArgumentCaptor import in
AuditStoreTest.java and replace its fully qualified usage with
ArgumentCaptor.forClass(Document.class). In AuditLedgerServiceTest.java, add the
same import and replace all fully qualified ArgumentCaptor usages in the
sequence/signing tests at lines 271, 291, 311, 328, and 353 with the simple name
and existing List.class arguments.
In `@src/test/java/ai/labs/eddi/engine/audit/rest/RestAuditStoreTest.java`:
- Around line 159-169: Extend the RestAuditStoreTest coverage around
verifyConversation and checkChain with a duplicate-sequence case representing
two entries using the same sequence number. Assert that the resulting
AuditVerificationReport exposes the duplicateSequences collection with the
expected duplicate value while retaining ChainStatus.INTACT and an intact
report.
In `@src/test/java/ai/labs/eddi/engine/gdpr/GdprComplianceServiceTest.java`:
- Around line 770-876: The cache invalidation tests cover only the default
intent shape; extend deleteUserData cache coverage with an intent containing “:”
and assert a post-erasure read through RestUserConversationStore misses the
cache. Reuse the same mapping and cache-warming flow in
deleteUserData_invalidatesCachedConversationMappings, ensuring the eviction key
matches the store’s format for this non-default intent.
In
`@src/test/java/ai/labs/eddi/engine/schedule/mongo/MongoScheduleStoreTest.java`:
- Around line 402-405: Update MongoScheduleStoreTest to import
org.mockito.ArgumentCaptor and use the simple ArgumentCaptor name instead of the
fully qualified reference. Also import com.mongodb.client.model.Filters and
strengthen the deleteMany verification assertion to compare the captured filter
with Filters.eq("userId", "user-1") rather than checking its string
representation.
🪄 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 Plus
Run ID: 0b560d40-b140-4e13-b285-885cf5f39fa4
📒 Files selected for processing (66)
AGENTS.mddocs/behavior-rules.mddocs/capability-match-guide.mddocs/conversation-memory.mddocs/prompt-snippets-guide.mddocs/properties.mddocs/semantic-parser.mdsrc/main/java/ai/labs/eddi/configs/agents/IRestCapabilityRegistry.javasrc/main/java/ai/labs/eddi/configs/dictionary/IRestAction.javasrc/main/java/ai/labs/eddi/configs/dictionary/IRestExpression.javasrc/main/java/ai/labs/eddi/configs/output/keys/IRestOutputActions.javasrc/main/java/ai/labs/eddi/configs/parser/IRestParserStore.javasrc/main/java/ai/labs/eddi/configs/properties/IRestPropertiesStore.javasrc/main/java/ai/labs/eddi/configs/properties/rest/RestPropertiesStore.javasrc/main/java/ai/labs/eddi/configs/workflows/IRestWorkflowStepStore.javasrc/main/java/ai/labs/eddi/engine/a2a/A2AModels.javasrc/main/java/ai/labs/eddi/engine/a2a/A2ATaskHandler.javasrc/main/java/ai/labs/eddi/engine/a2a/RestA2AEndpoint.javasrc/main/java/ai/labs/eddi/engine/audit/AuditHmac.javasrc/main/java/ai/labs/eddi/engine/audit/AuditLedgerService.javasrc/main/java/ai/labs/eddi/engine/audit/AuditStore.javasrc/main/java/ai/labs/eddi/engine/audit/AuditVerificationStatus.javasrc/main/java/ai/labs/eddi/engine/audit/IAuditStore.javasrc/main/java/ai/labs/eddi/engine/audit/model/AuditEntry.javasrc/main/java/ai/labs/eddi/engine/audit/model/AuditVerificationReport.javasrc/main/java/ai/labs/eddi/engine/audit/rest/IRestAuditStore.javasrc/main/java/ai/labs/eddi/engine/audit/rest/RestAuditStore.javasrc/main/java/ai/labs/eddi/engine/compliance/ComplianceStartupChecks.javasrc/main/java/ai/labs/eddi/engine/exception/ResourceStoreExceptionMapper.javasrc/main/java/ai/labs/eddi/engine/gdpr/GdprComplianceService.javasrc/main/java/ai/labs/eddi/engine/internal/ConversationService.javasrc/main/java/ai/labs/eddi/engine/internal/RestAgentEngineStreaming.javasrc/main/java/ai/labs/eddi/engine/internal/RestAgentManagement.javasrc/main/java/ai/labs/eddi/engine/memory/rest/IRestConversationStore.javasrc/main/java/ai/labs/eddi/engine/memory/rest/RestAttachmentUpload.javasrc/main/java/ai/labs/eddi/engine/memory/rest/RestConversationStore.javasrc/main/java/ai/labs/eddi/engine/schedule/IScheduleStore.javasrc/main/java/ai/labs/eddi/engine/schedule/mongo/MongoScheduleStore.javasrc/main/java/ai/labs/eddi/engine/triggermanagement/IRestUserConversationStore.javasrc/main/java/ai/labs/eddi/engine/triggermanagement/rest/RestUserConversationStore.javasrc/main/java/ai/labs/eddi/modules/llm/rest/RestToolHistory.javasrc/main/java/ai/labs/eddi/modules/templating/rest/IRestTemplatePreview.javasrc/main/java/ai/labs/eddi/modules/templating/rest/RestTemplatePreview.javasrc/test/java/ai/labs/eddi/configs/ConfigStoreRoleGateTest.javasrc/test/java/ai/labs/eddi/configs/properties/rest/RestPropertiesStoreTest.javasrc/test/java/ai/labs/eddi/engine/a2a/A2ATaskHandlerTest.javasrc/test/java/ai/labs/eddi/engine/a2a/RestA2AEndpointTest.javasrc/test/java/ai/labs/eddi/engine/audit/AuditHmacTest.javasrc/test/java/ai/labs/eddi/engine/audit/AuditLedgerServiceBranchTest.javasrc/test/java/ai/labs/eddi/engine/audit/AuditLedgerServiceTest.javasrc/test/java/ai/labs/eddi/engine/audit/AuditStoreTest.javasrc/test/java/ai/labs/eddi/engine/audit/rest/RestAuditStoreTest.javasrc/test/java/ai/labs/eddi/engine/compliance/ComplianceStartupChecksTest.javasrc/test/java/ai/labs/eddi/engine/exception/ExceptionMappersTest.javasrc/test/java/ai/labs/eddi/engine/gdpr/GdprComplianceServiceTest.javasrc/test/java/ai/labs/eddi/engine/internal/ConversationServiceAccessGuardTest.javasrc/test/java/ai/labs/eddi/engine/internal/RestAgentEngineStreamingExtendedTest.javasrc/test/java/ai/labs/eddi/engine/internal/RestAgentEngineStreamingTest.javasrc/test/java/ai/labs/eddi/engine/internal/RestAgentManagementExtendedTest.javasrc/test/java/ai/labs/eddi/engine/memory/rest/RestAttachmentUploadTest.javasrc/test/java/ai/labs/eddi/engine/memory/rest/RestConversationStoreTest.javasrc/test/java/ai/labs/eddi/engine/schedule/IScheduleStoreErasureDefaultTest.javasrc/test/java/ai/labs/eddi/engine/schedule/mongo/MongoScheduleStoreTest.javasrc/test/java/ai/labs/eddi/engine/triggermanagement/rest/RestUserConversationStoreTest.javasrc/test/java/ai/labs/eddi/modules/llm/rest/RestToolHistoryTest.javasrc/test/java/ai/labs/eddi/modules/templating/rest/RestTemplatePreviewTest.java
AGENTS.md section 2 rule 8 requires the changelog entry to land on the same branch as the work it documents. The wave 2a entry was written but swept into the wave 2b commit instead, because the file list for the 2a commit was computed before the entry existed. That left this PR with no record of the access-control, A2A, GDPR and audit findings it actually contains, and put two entries on the next PR. Content is unchanged — it is the same entry, moved to where it belongs. The duplicate on the wave 2b branch is removed in the merge that follows.
Dependency ReviewThe following issues were found:
License Issuespom.xml
OpenSSF Scorecard
Scanned Files
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 21: Correct the typo in the changelog entry by replacing “unroled” with
“unrolled,” leaving the surrounding description unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
…view feedback CI on this PR failed to compile: GdprComplianceService calls GroupConversationStore.deleteAllForUser(String), but that method was committed to the wave 2b branch while its caller landed here. The caller shipped without its implementation — the same split-across-branches mistake that made the wave 1 PR red, and one that only surfaces once a stacked PR is built in isolation. Moved the method and its test down to the branch that needs them. AgentGroupConfiguration is deliberately NOT moved: those are wave 2b's dynamic-agent guardrails and belong with that PR. Review feedback (github-code-quality): scanUsesAnExplicitPositiveLimit asserted `ERASURE_SCAN_LIMIT > 0`, a compile-time constant the compiler folds to `true`, so it asserted nothing. Rather than delete the assertion, the test now captures the argument actually passed to readAllSchedules and asserts on that runtime value — so it genuinely fails if the limit is ever set to 0, which would scan and therefore erase nothing while still reporting success. Review feedback (CodeRabbit): flagged "unroled" as a typo for "unrolled". It was not a typo — it meant "carrying no role annotation" — but the word is ambiguous enough that the reviewer misread it, so the sentence now says exactly that.
![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
Hardens EDDI’s security/compliance surfaces (wave 2a) by consistently applying the existing ownership/role-guard model across REST/SSE/A2A endpoints, expanding GDPR erasure coverage, and making the audit ledger integrity-verifiable (including tamper/deletion detection) while updating docs to match runtime behavior.
Changes:
- Enforced role/ownership checks for conversation-scoped endpoints (SSE turns, attachments, tool history, template preview, user↔conversation mapping, properties).
- Added audit-ledger integrity verification (verification endpoints + report model), strengthened signing to a v3 canonical form (GDPR pseudonymisation-safe) and introduced per-conversation sequencing for deletion detection.
- Expanded GDPR erasure to cover additional stores (schedules, group transcripts, checkpoints) plus added startup compliance checks and opaque 500 bodies to avoid leaking deployment internals; updated multiple docs to match actual emitted expressions/template model.
Reviewed changes
Copilot reviewed 69 out of 69 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/ai/labs/eddi/modules/templating/rest/RestTemplatePreviewTest.java | Adds tests for template preview role gate + conversation ownership enforcement and non-leakage. |
| src/test/java/ai/labs/eddi/modules/llm/rest/RestToolHistoryTest.java | Adds tests for admin-only control plane, owner-scoped history reads, and opaque 500 bodies. |
| src/test/java/ai/labs/eddi/engine/triggermanagement/rest/RestUserConversationStoreTest.java | Adds tests ensuring userId path ownership validation and admin-only interface gating. |
| src/test/java/ai/labs/eddi/engine/schedule/mongo/MongoScheduleStoreTest.java | Tests Mongo bulk delete of schedules by userId (GDPR erasure) and blank-user behavior. |
| src/test/java/ai/labs/eddi/engine/schedule/IScheduleStoreErasureDefaultTest.java | New test coverage for the portable default deleteSchedulesByUserId. |
| src/test/java/ai/labs/eddi/engine/memory/rest/RestConversationStoreTest.java | Adds tests for admin-only bulk delete + safer retention parameter validation/disable behavior. |
| src/test/java/ai/labs/eddi/engine/memory/rest/RestAttachmentUploadTest.java | Adds tests for caller-ownership enforcement on all attachment endpoints + non-anonymous role gate. |
| src/test/java/ai/labs/eddi/engine/internal/RestAgentManagementExtendedTest.java | Adds test ensuring internal errors do not leak store/driver details (correlation id only). |
| src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineStreamingTest.java | Adds tests enforcing conversation ownership guard on SSE turns before executing the turn. |
| src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineStreamingExtendedTest.java | Updates streaming test wiring for added access guard dependency. |
| src/test/java/ai/labs/eddi/engine/internal/ConversationServiceAccessGuardTest.java | New tests ensuring ownership gate is enforced inside ConversationService entry points. |
| src/test/java/ai/labs/eddi/engine/gdpr/GdprComplianceServiceTest.java | Extends GDPR erasure tests to cover new stores + cache invalidation behavior. |
| src/test/java/ai/labs/eddi/engine/exception/ExceptionMappersTest.java | Updates tests to require opaque 500 bodies + correlation id for store exceptions. |
| src/test/java/ai/labs/eddi/engine/compliance/ComplianceStartupChecksTest.java | Adds tests for audit-signing-required startup failure/warning behavior. |
| src/test/java/ai/labs/eddi/engine/audit/rest/RestAuditStoreTest.java | Adds integrity sweep tests (tamper detection, chain checks, limit clamping). |
| src/test/java/ai/labs/eddi/engine/audit/AuditStoreTest.java | Adds tests for sequence persistence and legacy/unsequenced row handling. |
| src/test/java/ai/labs/eddi/engine/audit/AuditLedgerServiceTest.java | Adds tests for bounded queue behavior and per-conversation sequencing in writes. |
| src/test/java/ai/labs/eddi/engine/audit/AuditLedgerServiceBranchTest.java | Updates branch tests for new max-queue-size constructor parameter. |
| src/test/java/ai/labs/eddi/engine/audit/AuditHmacTest.java | Updates/extends HMAC tests for v3 canonical form + GDPR pseudonymisation invariance + signed sequence. |
| src/test/java/ai/labs/eddi/engine/a2a/RestA2AEndpointTest.java | Adds tests for no exception-detail leakage and no task-id echo on not-found. |
| src/test/java/ai/labs/eddi/configs/properties/rest/RestPropertiesStoreTest.java | Adds tests ensuring per-user ownership validation and role gate on legacy properties endpoint. |
| src/test/java/ai/labs/eddi/configs/groups/mongo/GroupConversationStoreTest.java | Adds tests for user-scoped deletion with regex anchoring/escaping and exact-match verification. |
| src/test/java/ai/labs/eddi/configs/ConfigStoreRoleGateTest.java | New test ensuring all addressable config stores carry the authoring role gate. |
| src/main/java/ai/labs/eddi/modules/templating/rest/RestTemplatePreview.java | Enforces conversation ownership check before loading snapshot for real-data previews. |
| src/main/java/ai/labs/eddi/modules/templating/rest/IRestTemplatePreview.java | Adds authoring-only @RolesAllowed gate to the template preview REST interface. |
| src/main/java/ai/labs/eddi/modules/llm/rest/RestToolHistory.java | Adds admin-only control plane role gate, owner-scoped history read, and opaque 500 bodies with correlation id. |
| src/main/java/ai/labs/eddi/engine/triggermanagement/rest/RestUserConversationStore.java | Adds ownership validation against path userId for read/create/delete operations. |
| src/main/java/ai/labs/eddi/engine/triggermanagement/IRestUserConversationStore.java | Adds admin-only gate and clarifying docs (conversationId oracle risk). |
| src/main/java/ai/labs/eddi/engine/schedule/mongo/MongoScheduleStore.java | Adds userId index and indexed bulk delete for GDPR schedule erasure. |
| src/main/java/ai/labs/eddi/engine/schedule/IScheduleStore.java | Introduces portable deleteSchedulesByUserId default and erasure scan bound constant. |
| src/main/java/ai/labs/eddi/engine/memory/rest/RestConversationStore.java | Adds minimum retention guard + rejects dangerous deleteOlderThanDays values; disables scheduled sweep when below minimum. |
| src/main/java/ai/labs/eddi/engine/memory/rest/RestAttachmentUpload.java | Adds role gate and enforces caller ownership before async operations for all attachment endpoints. |
| src/main/java/ai/labs/eddi/engine/memory/rest/IRestConversationStore.java | Adds admin-only role gate for deployment-wide permanent-delete sweep endpoint. |
| src/main/java/ai/labs/eddi/engine/internal/RestAgentManagement.java | Replaces leaked exception bodies with opaque error messages + correlation ids. |
| src/main/java/ai/labs/eddi/engine/internal/RestAgentEngineStreaming.java | Adds explicit ownership check before starting SSE stream; keeps defense-in-depth with service-level guard. |
| src/main/java/ai/labs/eddi/engine/internal/ConversationService.java | Moves conversation ownership gate into say/sayStreaming conversationId-only entry points (adapter-safe). |
| src/main/java/ai/labs/eddi/engine/exception/ResourceStoreExceptionMapper.java | Returns generic 500 body + correlation id; logs full details server-side. |
| src/main/java/ai/labs/eddi/engine/compliance/ComplianceStartupChecks.java | Adds startup check for unsigned audit ledger with configurable hard-fail option. |
| src/main/java/ai/labs/eddi/engine/audit/rest/RestAuditStore.java | Adds verification endpoints implementation producing integrity reports (HMAC + chain gap detection). |
| src/main/java/ai/labs/eddi/engine/audit/rest/IRestAuditStore.java | Adds REST API for audit integrity verification by conversation and by agent. |
| src/main/java/ai/labs/eddi/engine/audit/model/AuditVerificationReport.java | New model for verification sweeps (per-entry status + chain verdict + metadata). |
| src/main/java/ai/labs/eddi/engine/audit/model/AuditEntry.java | Adds per-conversation sequence field, UNSEQUENCED sentinel, and copy helpers. |
| src/main/java/ai/labs/eddi/engine/audit/IAuditStore.java | Adds supportsSequence() capability flag for safe sequencing opt-in by stores. |
| src/main/java/ai/labs/eddi/engine/audit/AuditVerificationStatus.java | New enum for per-entry verification outcomes. |
| src/main/java/ai/labs/eddi/engine/audit/AuditStore.java | Persists/reads sequence, declares sequence support, and documents pseudonymisation semantics with v3 signing. |
| src/main/java/ai/labs/eddi/engine/audit/AuditHmac.java | Introduces v3 canonicalization (GDPR pseudonymisation-safe) and signs sequence; supports v1/v2 verification. |
| src/main/java/ai/labs/eddi/engine/a2a/RestA2AEndpoint.java | Prevents leaking internal exception details; returns curated messages and avoids task-id oracle. |
| src/main/java/ai/labs/eddi/engine/a2a/A2ATaskHandler.java | Scopes task/context caches by authenticated peer principal and stamps created conversations with a stable owner. |
| src/main/java/ai/labs/eddi/engine/a2a/A2AModels.java | Adds explicit InvalidA2ARequestException to distinguish peer-authored errors from internal failures. |
| src/main/java/ai/labs/eddi/configs/workflows/IRestWorkflowStepStore.java | Adds authoring role gate to workflow-step configuration surface. |
| src/main/java/ai/labs/eddi/configs/properties/rest/RestPropertiesStore.java | Adds per-user ownership validation for legacy properties endpoint over shared user memory store. |
| src/main/java/ai/labs/eddi/configs/properties/IRestPropertiesStore.java | Adds coarse role gate aligned with user-memory store role gating. |
| src/main/java/ai/labs/eddi/configs/parser/IRestParserStore.java | Adds authoring role gate to parser configuration store. |
| src/main/java/ai/labs/eddi/configs/output/keys/IRestOutputActions.java | Adds authoring role gate to output action keys config surface. |
| src/main/java/ai/labs/eddi/configs/groups/mongo/GroupConversationStore.java | Adds GDPR user-scoped transcript deletion with safe regex narrowing + exact-match check. |
| src/main/java/ai/labs/eddi/configs/dictionary/IRestExpression.java | Adds authoring role gate to expression config surface. |
| src/main/java/ai/labs/eddi/configs/dictionary/IRestAction.java | Adds authoring role gate to action config surface. |
| src/main/java/ai/labs/eddi/configs/agents/IRestCapabilityRegistry.java | Adds authoring role gate to capability registry REST surface. |
| docs/semantic-parser.md | Fixes incorrect/non-existent extension docs and corrects emitted expression names/examples. |
| docs/properties.md | Corrects template access patterns (properties are raw values) and clarifies recall-default inconsistencies. |
| docs/prompt-snippets-guide.md | Updates property-access examples to match raw-value template model. |
| docs/conversation-memory.md | Corrects template data model documentation (raw properties, adds snippets/vars keys). |
| docs/changelog.md | Adds wave 2a changelog entry describing findings fixed and verification notes. |
| docs/capability-match-guide.md | Updates property-access examples to match raw-value template model. |
| docs/behavior-rules.md | Clarifies negation and size-matcher semantics and edge cases based on current engine behavior. |
| AGENTS.md | Adds internal guideline note about @ConfigurationUpdate being a marker-only annotation currently. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…on this PR
All three were real. Verified against source before acting.
1. CodeQL / log injection (AuditLedgerService, 2 sites). A user-supplied
conversationId reached the log unsanitised, letting a crafted id forge log lines.
Both sites now go through LogSanitizer.sanitize, which the codebase already uses
for exactly this. This is what failed the CodeQL Analysis check.
2. GDPR erasure silently did nothing on PostgreSQL (Copilot). PostgresScheduleStore
never persisted userId — the column did not exist in the table and the store made
no reference to it — so ScheduleConfiguration.getUserId() was always null when
read back. The portable deleteSchedulesByUserId scan therefore compared against a
field that was never populated, matched nothing, and reported a successful erasure
while the user's schedules kept firing. Mongo was unaffected because it persists
the whole document, which is why the finding's Mongo-side fix looked complete.
- user_id added to the table DDL, the INSERT, the UPDATE and the row mapper
- ALTER TABLE ... ADD COLUMN IF NOT EXISTS for existing deployments, following
the precedent already set for the metadata column — without it an upgraded
instance stays broken
- an index on user_id, plus an override that erases with a single indexed DELETE
rather than the portable scan, which is capped by ERASURE_SCAN_LIMIT and would
otherwise erase only part of a large deployment's data
- the IScheduleStore javadoc claimed the portable default meant "every backend
erases correctly". It did not. Corrected to state the actual requirement.
3. Audit chain reported INTACT despite duplicate sequences (Copilot). checkChain
collected duplicates and then decided purely on missing.isEmpty(), so two entries
claiming the same position — one replaced, or one inserted — passed as intact when
no number was missing. That defeats the property the sequence exists to provide.
Duplicates now break the chain, and intact() rejects them independently as defence
in depth, since the report record is public and constructible elsewhere.
Mutation-checked: reverting the chain fix makes
RestAuditStoreTest.duplicateSequenceIsDetected fail with
"expected: <BROKEN> but was: <INTACT>".
The Postgres change cannot be verified locally (Testcontainers needs Docker) —
CI is the gate for it.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 70 out of 70 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
src/main/java/ai/labs/eddi/engine/memory/rest/IRestConversationStore.java:76
- The Javadoc says "unauthenticated request", but this operation is now @RolesAllowed("eddi-admin"). Update the wording so it doesn’t contradict the actual access control.
src/main/java/ai/labs/eddi/modules/templating/rest/RestTemplatePreview.java:101 - This comment claims converting a ForbiddenException into the "conversation not found" response would "leak existence", but 403 vs 200/404 is actually what can reveal existence. Consider rewording to reflect the real reason for not catching: returning a 403 is intentional and avoids masking an authorization failure.
src/main/java/ai/labs/eddi/engine/a2a/A2ATaskHandler.java:64 - Javadoc contains a literal "******" placeholder ("principal of the ****** it presented"), which reads like redacted text and is unclear. Replace it with the intended term (e.g., "access token").
/**
* The calling peer's identity. The JSON-RPC endpoint is {@code @Authenticated},
* so for a remote agent this is the principal of the Bearer token it presented
* (typically the OIDC subject / client id of the peer agent) — the only
* caller-independent identity available on this surface.
…id column, plus two doc corrections CI caught PostgresScheduleStoreUnitTest.updateSchedule_nullFireStatus_defaultsToPending: inserting user_id into the INSERT and UPDATE statements shifted every later parameter index, and that test pins fire_status at position 11. The production code is correct; the assertion hard-codes an index. Moved to 12. More importantly, the create-path assertion at position 5 did NOT fail — and that was worse than a failure. It reads `setString(5, null) // trigger_type`, but position 5 is now user_id. Because the fixture's userId is also null, the assertion kept passing while verifying an entirely different column. Retargeted to 6 and labelled, so the next index shift fails loudly instead of silently checking the wrong thing. Added two regression tests that pin the actual GDPR fix rather than its side effects: createSchedule and updateSchedule must both persist userId. Without the column a schedule read back has a null userId, the erasure scan matches nothing, and the sweep reports success while the user's schedules keep firing. Doc corrections from Copilot's low-confidence set, both of which were right: - IRestConversationStore said deleting with 0 days would wipe the deployment "in a single unauthenticated request", directly above the @RolesAllowed("eddi-admin") that now prevents exactly that. Reworded to describe the pre-fix state as history rather than as current behaviour. - RestTemplatePreview justified surfacing ForbiddenException by claiming a degrade to "conversation not found" "would leak existence". That reasoning is inverted: distinguishing 403 from 404 is what reveals which conversations exist; collapsing them discloses less. The real reason is that masking an authorization failure hides it from the operator, and this endpoint is admin/editor-gated anyway. Corrected, because a wrong security rationale in a comment misleads whoever edits it next. A third suppressed comment claimed A2ATaskHandler's javadoc contained a literal "******" placeholder. It does not — the source reads "the principal of the Bearer token it presented"; the masking was in the reviewer's own rendering. No change.
…ot prove Copilot review, two more instances of patterns already fixed elsewhere in this PR. 1. IScheduleStore's portable deleteSchedulesByUserId returned a count even when the scan filled its page. A full page means the ceiling was reached and there may be schedules it never examined — so the caller records the erasure request as satisfied while the remaining schedules keep firing under the erased user's id. It now throws, naming the limit and telling the operator the backend needs an indexed delete. PostgresScheduleStore already overrides with exactly that, so this only affects a backend relying on the portable path. 2. RestTemplatePreview.loadConversationData called requireConversationOwner purely for its side effect and discarded the return. That method answers null both for a missing descriptor and for a legacy unowned conversation, so an unknown conversationId proceeded. Switched to requireExistingConversationOwner, the variant added for the attachment endpoints, which 404s the missing case while still admitting legacy ones. Mutation-checked: removing the full-page guard fails fullPageMeansIncompleteErasure.
aisabella-ai
approved these changes
Jul 29, 2026
Integration Tests failed once on this PR with GroupConversationServiceHitlTest.secondResumeThrowsCas. The commit it failed on changed two lines of javadoc placement, so it was not a regression. Cause: the test drove a REAL first resumeDiscussion and then re-stubbed conversationStore. resumeDiscussion hands the remaining phases to executorService, so that background thread was still invoking the very mocks being re-stubbed — a Mockito data race in the test, not a defect in the service. The first call was not carrying any assurance. The store is a mock, so "another resume already moved the state" is expressed by the stub either way, and the successful-resume path is already covered by ResumePhaseIndex and TurnBudgetResume. Removing it leaves the contract intact — a resume that loses the CAS must surface ResourceModifiedException rather than swallow it and run the phases twice — and makes the test deterministic. Ran three times in a row, clean. The underlying testability gap remains: executorService is constructed inside GroupConversationService, so no test can make its background work synchronous. Injecting it would let these paths be tested directly rather than avoided; noted rather than done here, since that constructor is on the critical path for the decomposition work still to come.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 86 out of 86 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/templating/rest/RestTemplatePreview.java:116
loadConversationData()collapses store failures and not-found into the samenullreturn, whichpreviewTemplate()then reports as"Conversation not found: <id>". This is inaccurate forResourceStoreException(e.g., DB outage) and can mislead operators/clients troubleshooting authoring preview failures. Consider distinguishing not-found (return null) from store/driver failures (return 500 with a generic message).
…t found" Copilot review. loadConversationData caught ResourceStoreException and ResourceNotFoundException together and returned null for both, which previewTemplate then reported as "Conversation not found: <id>". During a database outage an author was told their conversation did not exist — sending whoever investigated to look for the wrong problem entirely, at the worst possible moment. Not-found still returns null and still reports not-found. A store failure now surfaces as a server error carrying a correlation id, with the driver detail in the log rather than the response body, matching the redaction established for A12. The existing test asserted the old collapsed behaviour, so it was pinning the misleading response; rewritten to the new contract, including that neither "not found" nor the driver message reaches the client.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 86 out of 86 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/main/java/ai/labs/eddi/datastore/postgres/PostgresResourceStorage.java:198
- fieldIndexName() uses sanitizedField.hashCode() as the digest. Java hashCode is only 32-bit, so collisions are possible and would reintroduce exactly the “two distinct JSON keys map to the same index name” failure mode this helper is meant to prevent (just less frequently). Consider using a stronger deterministic digest for the suffix (e.g., UUID.nameUUIDFromBytes) so distinct keys are overwhelmingly unlikely to share an index name.
String withDigest = historical + "_" + Integer.toHexString(sanitizedField.hashCode());
if (withDigest.length() <= MAX_IDENTIFIER_LENGTH) {
return withDigest;
}
Comment on lines
+118
to
+132
| /** | ||
| * Whether this store round-trips {@link AuditEntry#sequence()}. | ||
| * <p> | ||
| * The sequence is part of the signed payload, so a store that silently dropped | ||
| * it on write would make every one of its rows verify as tampered. | ||
| * Implementations therefore <em>opt in</em>: only when this returns true does | ||
| * {@code AuditLedgerService} assign a real sequence — otherwise entries are | ||
| * signed as {@link AuditEntry#UNSEQUENCED} and stay verifiable, at the cost of | ||
| * not being chained. | ||
| * | ||
| * @return true if the sequence survives a write/read round trip | ||
| */ | ||
| default boolean supportsSequence() { | ||
| return false; | ||
| } |
…ion works there too Copilot review. G18's deletion/reordering detection rests on a signed per-conversation sequence. PostgresAuditStore never persisted it and left supportsSequence() at its false default, so AuditLedgerService skipped assigning one entirely — every PostgreSQL deployment silently degraded to HMAC-only. That is the gap that matters most for this feature: a per-entry HMAC cannot see a DELETED row, because nothing is left behind to fail verification. The sequence is the only thing that makes the hole visible. MongoDB deployments had it; PostgreSQL ones did not, and nothing reported the difference — verification simply answered UNAVAILABLE, which reads like "not applicable" rather than "unprotected". Same shape as the schedule userId gap earlier in this PR: a feature implemented against one backend and silently absent on the other. - sequence column added to the DDL, the INSERT (appended last, so existing bind positions are untouched), and the row mapper - ALTER TABLE ... ADD COLUMN IF NOT EXISTS for existing ledgers, defaulting to the UNSEQUENCED sentinel so pre-existing rows keep reporting UNAVAILABLE rather than being mistaken for a broken chain - an index on (conversation_id, sequence), which is how verification reads - supportsSequence() now returns true Not verifiable here (Testcontainers needs Docker); CI is the gate for the schema.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 88 out of 88 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (6)
src/main/java/ai/labs/eddi/engine/memory/rest/RestAttachmentUpload.java:240
- This
@SuspendedAsyncResponse endpoint calls the ownership guard synchronously; if it throws, the AsyncResponse is never resumed and the request can hang. Catch guard failures, resume the AsyncResponse (with the exception), and return.
src/main/java/ai/labs/eddi/engine/memory/rest/RestAttachmentUpload.java:296 - This
@SuspendedAsyncResponse endpoint calls the ownership guard synchronously; if it throws, the AsyncResponse is never resumed and the request can hang. Catch guard failures, resume the AsyncResponse (with the exception), and return.
src/main/java/ai/labs/eddi/engine/memory/rest/RestAttachmentUpload.java:335 - This
@SuspendedAsyncResponse endpoint calls the ownership guard synchronously; if it throws, the AsyncResponse is never resumed and the request can hang. Catch guard failures, resume the AsyncResponse (with the exception), and return.
src/main/java/ai/labs/eddi/engine/memory/rest/RestAttachmentUpload.java:209 - This
@SuspendedAsyncResponse endpoint calls the ownership guard synchronously; if it throws, the AsyncResponse is never resumed and the request can hang. Catch guard failures, resume the AsyncResponse (with the exception), and return.
src/main/java/ai/labs/eddi/modules/llm/impl/PromptSnippetService.java:36 - The PromptSnippetService Javadoc still documents snippet access as
{{snippets.<name>}}, but the engine uses Qute single-brace placeholders ({snippets.<name>}). This can mislead users back into the double-brace form that does not resolve.
src/main/java/ai/labs/eddi/engine/memory/rest/RestAttachmentUpload.java:117 - These endpoints use
@SuspendedAsyncResponse, but the ownership guard is invoked before any async work and may throw (403/404). If it throws, the AsyncResponse is never resumed, so the request can hang. Resume the AsyncResponse on guard failures and return early (apply the same pattern to the other methods in this class too).
This issue also appears in the following locations of the same file:
- line 208
- line 239
- line 295
- line 334
7bfef02 added the audit chain sequence to the Postgres DDL, the INSERT and the row mapper — but not to SELECT_ALL. Every read path then called rs.getLong("sequence") on a ResultSet that had never selected it, so all five query methods threw "column name sequence was not found" instead of returning entries. Caught by CI: PostgresAuditStoreTest is a Testcontainers test, which cannot run in the local sandbox, so the local run that preceded that commit skipped exactly the tests that exercise a real schema. All read paths share SELECT_ALL, so the one line covers every one. Also adds the assertions that were missing: the earlier sequence tests went into the mocked class, leaving the real-database test with nothing that checks the value round-trips. Without them this fix would be proven only by "queries stopped throwing" — which would not catch reading back a value the writer never wrote. The three new tests pin the exact sequences, that pre-migration rows read as UNSEQUENCED rather than as a broken chain, and that the store advertises support (without which the ledger assigns no sequence at all — the original defect's shape).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 89 out of 89 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/main/java/ai/labs/eddi/datastore/postgres/PostgresResourceStorage.java:199
fieldIndexName()usesString.hashCode()to disambiguate case-variant field hints.hashCode()collisions are possible, which would reintroduce the exact failure mode this method is meant to prevent (the secondCREATE INDEX IF NOT EXISTSbecomes a silent no-op, leaving one field unindexed). Consider using a stronger, stable digest (e.g., UUID name-based digest over UTF-8 bytes) so distinct field names are overwhelmingly unlikely to collide, while still keeping the historical name for already-lowercase fields.
private static String fieldIndexName(String sanitizedField) {
String lowerCased = sanitizedField.toLowerCase(Locale.ROOT);
String historical = INDEX_NAME_PREFIX + lowerCased;
if (lowerCased.equals(sanitizedField) && historical.length() <= MAX_IDENTIFIER_LENGTH) {
return historical;
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 was referenced Aug 1, 2026
Merged
Merged
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 #616 (wave 1). Targets
fix/code-review-findings, so this diff shows only wave 2a. Kept to 66 files — CodeRabbit skipped #616 for being 51 files over its 100-file limit.Wave 2 of the 124-finding external review, split in two. This half is access control and compliance; the LLM/memory half is a separate stacked PR.
The pattern
EDDI already has a working guard triad —
OwnershipValidator,ConversationAccessGuard,HitlAccessGuard— used correctly inengine/internal,engine/hitland the MCP surface. Every finding here is a place that never called it. The fixes are mostly one added line; the work was the inventory.Sharpest items
A1 — the SSE turn endpoint had no ownership check, while its non-streaming twin did.
The turn then ran under the target conversation's
userId: that user's long-term memories loaded into the prompt, tool calls executed in their context. Anyone who learns aconversationIdgets this — and A6 hands outconversationIds.Rather than patch the one endpoint, the guard moved down into
ConversationService.say/sayStreaming, before the memory snapshot loads, so no future REST/SSE/MCP/Slack adapter can omit it. The REST layer keeps a check too, so a denial is a plain 403 instead of an error event on an already-200 SSE stream. Two pass-throughs are deliberate and documented in code: a null guard (bean built outside CDI, i.e. unit tests) andContextNotActiveException(Slack drivessay()on a plain virtual-thread executor with no CDI request context — that path was never checked before and would otherwise start throwing).A2 — attachment endpoints authorised the path parameter, not the caller.
IAttachmentStore.load(ref, requestingConversationId)verifies that the named conversation owns the blob — and the caller supplies that name. The check was self-satisfying. All five methods now require caller ownership, checked on the request thread before the async hop, sinceSecurityIdentityis request-scoped.A9 — A2A sat entirely outside the ownership model. Four separate defects, including conversations created with
userId = null, whichOwnershipValidatortreats as "legacy — allow" and therefore leaves permanently unowned.A17 → G17 — pseudonymisation silently broke the audit ledger's integrity guarantee.
updateMany($set userId)with no HMAC recompute, anduserIdis a signed field. So every routine GDPR erasure produced rows cryptographically indistinguishable from tampered ones. The class javadoc claiming a write-once contract was literally true and substantively false.A8 — the inventory was wider than reported. The review named
/parserstore/parsers; I enumerated every@Path-bearingIRest*interface without@RolesAllowedand found six, including two the review missed entirely (IRestCapabilityRegistry,IRestWorkflowStepStore).IRestVersionInfowas deliberately left alone — it has no@Pathand is a mixin; reasoning is in the code.Judgement calls worth reviewing
@RolesAllowedwent on the method, not theIRestConversationStoreclass.McpConversationToolsreaches this interface viaRestInterfaceFactory, which is a real loopback HTTP client that sends no auth header — a class-level role would 401 the MCPlist_conversationstool wheneverauthorization.enabled=true. Read paths are already owner-scoped; the admin role sits only on the one deployment-wide operation with no owner scoping to apply./usermemorystore, but EDDI-Manager is a separate repo so I could not prove there are no callers.Docs (I7–I13)
docs/semantic-parser.mddocumented a stemming extension that does not exist, four times, including inside the flagship copy-paste config — copying it throwsUnrecognizedExtensionExceptionand the agent will not start. Its expression table also claimednumber(42)/time(15:00)where the code emitsinteger(42)and epoch millis, so rules written from it never fired.docs/conversation-memory.mdanddocs/properties.mdboth contradicted AGENTS.md §5.1 ({properties.X.valueString}fails at runtime —MemoryItemConverterputs raw values).Verification
*IT.java.)-Dtest=Class#methodsilently runs 0 tests and exits 0 when the method is in a@Nestedclass, which reads exactly like a pass.Summary by CodeRabbit