feat(groups): shared artifacts - typed co-edited documents with CAS and validators (I17) - #637
Merged
Merged
Conversation
…nd validators (I17) Members co-edit typed documents (TEXT/MARKDOWN/JSON) through four tools gated by a new artifactConfig, instead of re-parsing each other's prose. Own collection (never embedded - the loop's stale-snapshot persists would clobber it); deterministic version CAS with a re-read-and-merge retry sentence, via a new numeric storeIfFieldEquals overload (Mongo's typed BSON equality never matches a number against a string); declarative validators (JSON Schema/regex/length) hard-checked at config save; writes announced as artifact_updated via a change queue the turn executor drains (tools have no listener reference); close/delete cascade + user-keyed GDPR erasure via a stamped ownerUserId.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
Contributor
|
Warning Review limit reached
Next review available in: 13 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds opt-in shared artifacts for group conversations. The change includes typed models, validation, CRUD tools, numeric CAS storage, lifecycle and GDPR cleanup, read-time loading, and SSE/Slack update notifications. ChangesShared artifacts
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AgentOrchestrator
participant ArtifactToolsProvider
participant ArtifactTools
participant SharedArtifactStore
participant GroupConversationEventSink
AgentOrchestrator->>ArtifactToolsProvider: assemble contextual tools
ArtifactToolsProvider->>ArtifactTools: expose eligible artifact tools
ArtifactTools->>SharedArtifactStore: persist artifact operation
SharedArtifactStore-->>ArtifactTools: return artifact or CAS result
ArtifactTools->>GroupConversationEventSink: queue artifact metadata update
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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: 10
🧹 Nitpick comments (5)
src/main/java/ai/labs/eddi/configs/groups/model/AgentGroupConfiguration.java (1)
180-196: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider an upper bound on
maxArtifactsPerDiscussion.This class bounds every other fan-out knob with a constant (
MAX_MEMBERS,MAX_DISCUSSION_ROUNDS).maxArtifactsPerDiscussionhas only a lower-bound normalization. Each artifact allows up to 256 KB of content and every create issues a full list scan inArtifactTools.createArtifact, so a large value multiplies both storage and per-turn read cost.Add a ceiling in the compact constructor, in the same shape as the other caps.
♻️ Proposed refactor
public static final int DEFAULT_MAX_ARTIFACTS = 5; + public static final int MAX_ARTIFACTS_CEILING = 50; /** Same normalization choke point as {`@link` GroupTaskConfig}. */ public ArtifactConfig { if (maxArtifactsPerDiscussion <= 0) { maxArtifactsPerDiscussion = DEFAULT_MAX_ARTIFACTS; } + maxArtifactsPerDiscussion = Math.min(maxArtifactsPerDiscussion, MAX_ARTIFACTS_CEILING);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/configs/groups/model/AgentGroupConfiguration.java` around lines 180 - 196, Update the compact constructor of ArtifactConfig to cap maxArtifactsPerDiscussion at a new class-level maximum constant, matching the existing fan-out limit pattern used by MAX_MEMBERS and MAX_DISCUSSION_ROUNDS. Preserve the DEFAULT_MAX_ARTIFACTS fallback for non-positive values and continue normalizing validators unchanged.src/main/java/ai/labs/eddi/configs/groups/ArtifactValidators.java (1)
154-160: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winParse each JSON schema once instead of on every write.
checkJsonSchemacallsSCHEMA_FACTORY.getSchema(spec)per validation. The spec is fixed per group configuration, so the parse repeats for every artifact create and update. Cache the compiledJsonSchemaby spec in a bounded map.♻️ Proposed refactor
+ /** + * Compiled schemas by spec. Specs come from group configs, so the key space is + * small and stable; the bound only guards against an unexpected churn of configs. + */ + private static final int SCHEMA_CACHE_LIMIT = 256; + private static final Map<String, JsonSchema> SCHEMA_CACHE = new ConcurrentHashMap<>(); + private static String checkJsonSchema(String spec, String content) { JsonSchema schema; try { - schema = SCHEMA_FACTORY.getSchema(spec); + if (SCHEMA_CACHE.size() >= SCHEMA_CACHE_LIMIT) { + SCHEMA_CACHE.clear(); + } + schema = SCHEMA_CACHE.computeIfAbsent(spec, SCHEMA_FACTORY::getSchema); } catch (Exception e) { return "This discussion's artifact schema validator is misconfigured; the write was refused."; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/configs/groups/ArtifactValidators.java` around lines 154 - 160, Update checkJsonSchema to cache compiled JsonSchema instances by spec in a bounded map, reusing the cached schema across artifact validations while parsing each distinct spec only once. Preserve the existing misconfiguration response when schema compilation fails, and ensure the cache cannot grow without bound.src/test/java/ai/labs/eddi/configs/groups/mongo/SharedArtifactStoreTest.java (1)
236-253: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a cascade-delete test for the no-progress case.
cascadeDelete_removesAllstubs an empty second pass, so the loop always terminates. It does not cover a row that keeps appearing in the listing. That path is the one described in thedeleteByGroupConversationIdcomment onSharedArtifactStore.javalines 151-170: the current loop repeats up toMAX_ERASURE_PASSEStimes and inflates the returned count.Add the test with the fix.
💚 Proposed test
`@Test` `@DisplayName`("a row that survives deletion stops the cascade instead of spinning and over-counting") void cascadeDelete_noProgress_stopsAndCountsOnce() throws Exception { var a1 = artifact("a-1", "gc-1", "user-1", 1); var r1 = resource("a-1", a1); // The row never disappears from the listing. when(storage.findResources(any(IResourceFilter.QueryFilters[].class), eq("createdAt"), eq(0), anyInt())) .thenReturn(List.of(resourceId("a-1"))); when(storage.read("a-1", 1)).thenReturn(r1); assertEquals(1, store.deleteByGroupConversationId("gc-1"), "each artifact is counted at most once"); verify(storage, times(1)).removeAllPermanently("a-1"); }🤖 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/groups/mongo/SharedArtifactStoreTest.java` around lines 236 - 253, Add a no-progress cascade-delete test alongside cascadeDelete_removesAll using a listing that repeatedly returns the same artifact, then assert deleteByGroupConversationId("gc-1") returns 1 and verify removeAllPermanently is called once for that artifact. Implement the corresponding deleteByGroupConversationId fix so repeated listings terminate without inflating the count.src/main/java/ai/labs/eddi/datastore/mongo/MongoResourceStorage.java (1)
128-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared CAS body from the two
storeIfFieldEqualsoverloads.The
longoverload duplicates theStringoverload apart from the comparand type and the message format. Both copies carry the same 404-versus-409 disambiguation, so a future fix must be applied twice.♻️ Proposed refactor
`@Override` public void storeIfFieldEquals(IResource<T> newResource, String fieldName, String expectedValue) throws IResourceStore.ResourceModifiedException, IResourceStore.ResourceNotFoundException { - Resource resource = checkInternalResource(newResource); - var result = currentCollection.replaceOne( - Filters.and( - Filters.eq(ID_FIELD, new ObjectId(resource.getId())), - Filters.eq(fieldName, expectedValue)), - resource.getMongoDocument()); - if (result.getMatchedCount() == 0) { - // Distinguish "deleted" (404) from "field mismatch" (409) — a bare - // matchedCount==0 conflates them and misleads callers/operators. - long exists = currentCollection.countDocuments(Filters.eq(ID_FIELD, new ObjectId(resource.getId()))); - if (exists == 0) { - throw new IResourceStore.ResourceNotFoundException( - String.format("Resource no longer exists (id=%s)", resource.getId())); - } - throw new IResourceStore.ResourceModifiedException( - String.format("Resource field '%s' was not '%s' (id=%s)", fieldName, expectedValue, resource.getId())); - } + casOnField(newResource, fieldName, expectedValue, "'" + expectedValue + "'"); } `@Override` public void storeIfFieldEquals(IResource<T> newResource, String fieldName, long expectedValue) throws IResourceStore.ResourceModifiedException, IResourceStore.ResourceNotFoundException { - Resource resource = checkInternalResource(newResource); // Typed BSON equality — the String overload's Filters.eq(field, "3") never // matches an int64 3, which is exactly why this overload exists. - var result = currentCollection.replaceOne( - Filters.and( - Filters.eq(ID_FIELD, new ObjectId(resource.getId())), - Filters.eq(fieldName, expectedValue)), - resource.getMongoDocument()); - if (result.getMatchedCount() == 0) { - long exists = currentCollection.countDocuments(Filters.eq(ID_FIELD, new ObjectId(resource.getId()))); - if (exists == 0) { - throw new IResourceStore.ResourceNotFoundException( - String.format("Resource no longer exists (id=%s)", resource.getId())); - } - throw new IResourceStore.ResourceModifiedException( - String.format("Resource field '%s' was not %d (id=%s)", fieldName, expectedValue, resource.getId())); - } + casOnField(newResource, fieldName, expectedValue, Long.toString(expectedValue)); } + + /** + * Replace the document only if {`@code` fieldName} equals {`@code` expectedValue}, + * distinguishing "deleted" (404) from "field mismatch" (409) — a bare + * matchedCount==0 conflates them and misleads callers/operators. + */ + private void casOnField(IResource<T> newResource, String fieldName, Object expectedValue, String renderedExpected) + throws IResourceStore.ResourceModifiedException, IResourceStore.ResourceNotFoundException { + Resource resource = checkInternalResource(newResource); + var idFilter = Filters.eq(ID_FIELD, new ObjectId(resource.getId())); + var result = currentCollection.replaceOne( + Filters.and(idFilter, Filters.eq(fieldName, expectedValue)), + resource.getMongoDocument()); + if (result.getMatchedCount() == 0) { + if (currentCollection.countDocuments(idFilter) == 0) { + throw new IResourceStore.ResourceNotFoundException( + String.format("Resource no longer exists (id=%s)", resource.getId())); + } + throw new IResourceStore.ResourceModifiedException( + String.format("Resource field '%s' was not %s (id=%s)", fieldName, renderedExpected, resource.getId())); + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/datastore/mongo/MongoResourceStorage.java` around lines 128 - 171, Extract the shared compare-and-swap implementation from both storeIfFieldEquals overloads into a private helper, parameterized by the field comparison value and mismatch-message formatting. Have the String and long overloads delegate to that helper while preserving typed BSON equality and the existing ResourceNotFoundException versus ResourceModifiedException behavior.src/test/java/ai/labs/eddi/modules/llm/tools/impl/ArtifactToolsTest.java (1)
317-353: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a deterministic test for the CAS-rejection branch.
This test can pass without ever reaching
proposeArtifactUpdate'sResourceModifiedExceptionhandler. If one virtual thread finishes before the other resolves, the loser fails the pre-CAS check atArtifactToolsline 185 and returns the same sentence. The handler at lines 195-201, including thecurrentVersionOffallback, then stays uncovered.Add a store stub whose
updateIfVersionthrowsResourceModifiedExceptionunconditionally, and assert the returned sentence names the current version.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/ai/labs/eddi/modules/llm/tools/impl/ArtifactToolsTest.java` around lines 317 - 353, Extend update_concurrentCas_oneWins with a deterministic test setup using a store stub whose updateIfVersion always throws ResourceModifiedException, ensuring proposeArtifactUpdate reaches its CAS-rejection handler. Assert the returned retry sentence includes the version obtained through currentVersionOf, while preserving the existing concurrent test for the pre-CAS rejection path.
🤖 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 8: Update the changelog entry heading for the shared artifacts release to
use the current date, 2026-08-07, or label it Unreleased if publication has not
occurred; do not leave the future date 2026-08-08.
In `@src/main/java/ai/labs/eddi/configs/groups/ArtifactValidators.java`:
- Around line 143-152: Update checkRegex to match against an interruptible
CharSequence wrapper around content, and add the interruptible helper that
checks Thread.currentThread().isInterrupted() in charAt before returning
characters. Preserve the existing mismatch and PatternSyntaxException messages,
while allowing an interruption-triggered exception to abort runaway matching
rather than pinning the calling thread.
- Around line 73-79: Update the JSON_SCHEMA branch in ArtifactValidators to
validate the parsed schema against the V202012 meta-schema before accepting it.
Keep parsing and existing IllegalArgumentException handling, but ensure invalid
schema keywords fail during requireValidSpecs rather than only during artifact
saving.
In
`@src/main/java/ai/labs/eddi/configs/groups/model/AgentGroupConfiguration.java`:
- Around line 185-190: Update the ArtifactConfig constructor to replace
List.copyOf(validators) with an unmodifiable copy that permits null elements,
while retaining the empty-list handling for null validators. This allows
ArtifactValidators.requireValidSpecs to receive null entries and produce its
intended validation message.
In `@src/main/java/ai/labs/eddi/configs/groups/mongo/SharedArtifactStore.java`:
- Around line 122-125: Update ISharedArtifactStore.listByGroupConversationId in
SharedArtifactStore so the resources returned by storage.findResources are
sorted by createdAt ascending in Java before being returned, preserving the
interface contract that artifacts are listed oldest first while leaving backend
query ordering unchanged.
- Around line 151-170: Update deleteByGroupConversationId to mirror
deleteAllForUser: track processed artifact IDs, delete each ID at most once,
count only newly processed artifacts, and use a newThisPass guard to stop when a
listing makes no progress. Preserve the existing validation, pass limit, and
empty-list termination behavior.
In `@src/main/java/ai/labs/eddi/engine/gdpr/GdprComplianceService.java`:
- Around line 313-329: Update the deleteUserData Javadoc operation list to
explicitly include deletion of shared artifacts in the cascade, preserving the
existing ordering and wording for group conversation transcripts and schedules.
In `@src/main/java/ai/labs/eddi/engine/internal/groups/MemberTurnExecutor.java`:
- Around line 155-179: The per-turn finally block in executeAgentTurn and
announceArtifactChanges must not independently drain and publish shared artifact
changes. Introduce or reuse a conversation-level serialized publisher that
remains active until all member work, including timed-out agent tasks, has
stopped; route queued changes through it so late writes are delivered and
versions retain write order. Add coverage for a late write after the final-turn
timeout and for concurrent versioned writes.
In
`@src/main/java/ai/labs/eddi/integrations/slack/SlackGroupDiscussionListener.java`:
- Around line 298-317: Escape event.name() and event.editorAgentId() for Slack
mrkdwn by replacing angle brackets with < and > before inserting them into
the StringBuilder formatting in onArtifactUpdated. Preserve the existing
null/blank checks and use the escaped values for both user-visible fields.
In `@src/main/java/ai/labs/eddi/modules/llm/tools/impl/ArtifactTools.java`:
- Around line 273-277: Update the oversize message in the artifact validation
logic to report the content size in KB using ceiling division rather than
truncating bytes / 1024. Keep the existing limit formatting and refusal behavior
unchanged, including the MAX_CONTENT_BYTES comparison.
---
Nitpick comments:
In `@src/main/java/ai/labs/eddi/configs/groups/ArtifactValidators.java`:
- Around line 154-160: Update checkJsonSchema to cache compiled JsonSchema
instances by spec in a bounded map, reusing the cached schema across artifact
validations while parsing each distinct spec only once. Preserve the existing
misconfiguration response when schema compilation fails, and ensure the cache
cannot grow without bound.
In
`@src/main/java/ai/labs/eddi/configs/groups/model/AgentGroupConfiguration.java`:
- Around line 180-196: Update the compact constructor of ArtifactConfig to cap
maxArtifactsPerDiscussion at a new class-level maximum constant, matching the
existing fan-out limit pattern used by MAX_MEMBERS and MAX_DISCUSSION_ROUNDS.
Preserve the DEFAULT_MAX_ARTIFACTS fallback for non-positive values and continue
normalizing validators unchanged.
In `@src/main/java/ai/labs/eddi/datastore/mongo/MongoResourceStorage.java`:
- Around line 128-171: Extract the shared compare-and-swap implementation from
both storeIfFieldEquals overloads into a private helper, parameterized by the
field comparison value and mismatch-message formatting. Have the String and long
overloads delegate to that helper while preserving typed BSON equality and the
existing ResourceNotFoundException versus ResourceModifiedException behavior.
In
`@src/test/java/ai/labs/eddi/configs/groups/mongo/SharedArtifactStoreTest.java`:
- Around line 236-253: Add a no-progress cascade-delete test alongside
cascadeDelete_removesAll using a listing that repeatedly returns the same
artifact, then assert deleteByGroupConversationId("gc-1") returns 1 and verify
removeAllPermanently is called once for that artifact. Implement the
corresponding deleteByGroupConversationId fix so repeated listings terminate
without inflating the count.
In `@src/test/java/ai/labs/eddi/modules/llm/tools/impl/ArtifactToolsTest.java`:
- Around line 317-353: Extend update_concurrentCas_oneWins with a deterministic
test setup using a store stub whose updateIfVersion always throws
ResourceModifiedException, ensuring proposeArtifactUpdate reaches its
CAS-rejection handler. Assert the returned retry sentence includes the version
obtained through currentVersionOf, while preserving the existing concurrent test
for the pre-CAS rejection path.
🪄 Autofix
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: 3a2c8003-aa03-4f58-815b-555dbb531b63
📒 Files selected for processing (33)
docs/changelog.mddocs/group-conversations.mdpom.xmlsrc/main/java/ai/labs/eddi/configs/groups/ArtifactValidators.javasrc/main/java/ai/labs/eddi/configs/groups/ISharedArtifactStore.javasrc/main/java/ai/labs/eddi/configs/groups/model/AgentGroupConfiguration.javasrc/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.javasrc/main/java/ai/labs/eddi/configs/groups/model/SharedArtifact.javasrc/main/java/ai/labs/eddi/configs/groups/mongo/AgentGroupStore.javasrc/main/java/ai/labs/eddi/configs/groups/mongo/SharedArtifactStore.javasrc/main/java/ai/labs/eddi/datastore/IResourceStorage.javasrc/main/java/ai/labs/eddi/datastore/mongo/MongoResourceStorage.javasrc/main/java/ai/labs/eddi/datastore/postgres/PostgresResourceStorage.javasrc/main/java/ai/labs/eddi/engine/api/IGroupConversationService.javasrc/main/java/ai/labs/eddi/engine/api/IRestGroupConversation.javasrc/main/java/ai/labs/eddi/engine/gdpr/GdprComplianceService.javasrc/main/java/ai/labs/eddi/engine/internal/GroupConversationService.javasrc/main/java/ai/labs/eddi/engine/internal/RestGroupConversation.javasrc/main/java/ai/labs/eddi/engine/internal/groups/GroupLifecycleOps.javasrc/main/java/ai/labs/eddi/engine/internal/groups/MemberTurnExecutor.javasrc/main/java/ai/labs/eddi/engine/lifecycle/GroupConversationEventSink.javasrc/main/java/ai/labs/eddi/integrations/slack/SlackGroupDiscussionListener.javasrc/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.javasrc/main/java/ai/labs/eddi/modules/llm/impl/ArtifactToolsProvider.javasrc/main/java/ai/labs/eddi/modules/llm/tools/impl/ArtifactTools.javasrc/test/java/ai/labs/eddi/configs/groups/ArtifactValidatorsTest.javasrc/test/java/ai/labs/eddi/configs/groups/mongo/SharedArtifactStoreTest.javasrc/test/java/ai/labs/eddi/engine/gdpr/GdprComplianceServiceTest.javasrc/test/java/ai/labs/eddi/engine/internal/groups/GroupLifecycleOpsTest.javasrc/test/java/ai/labs/eddi/engine/internal/groups/MemberTurnExecutorTest.javasrc/test/java/ai/labs/eddi/integrations/slack/SlackGroupDiscussionListenerTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/ArtifactToolsProviderTest.javasrc/test/java/ai/labs/eddi/modules/llm/tools/impl/ArtifactToolsTest.java
…ontracts - Save-time JSON schema specs now also validate against the bundled 2020-12 meta-schema; parse-only admitted invalid keyword values. - REGEX validators match through a 500ms deadline-guarded CharSequence so a catastrophically backtracking config pattern refuses the write instead of pinning the member turn. - ArtifactConfig tolerates a [null] validator entry so requireValidSpecs can report its position instead of an NPE. - Artifact events: drain+announce serialized on a per-conversation mutex (write order), plus a final announce pass per discussion leg for writes accepted after a timed-out turn drained. - listByGroupConversationId re-sorts oldest-first per its contract; deleteByGroupConversationId gets the processed-set/no-progress guard. - Slack mrkdwn-escapes artifact name/editor id; oversize refusal rounds up; GDPR cascade Javadoc names artifacts; log sanitized.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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/configs/groups/ArtifactValidators.java (1)
160-169: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject non-positive runtime length limits.
requireValidSpecsrejectsMAX_LENGTHvalues less than or equal to zero, butcheckMaxLengthaccepts them. Therefore,firstRejectionreturnsnullfor a malformed"0"specification and empty content. Rejectmax <= 0after parsing so corrupted stored configuration fails closed.Proposed fix
} catch (NumberFormatException e) { return "This discussion's artifact length validator is misconfigured; the write was refused."; } + if (max <= 0) { + return "This discussion's artifact length validator is misconfigured; the write was refused."; + } if (content.length() > max) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/configs/groups/ArtifactValidators.java` around lines 160 - 169, Update checkMaxLength after parsing spec to reject any max value less than or equal to zero using the same misconfiguration response as invalid numeric specifications, ensuring zero and negative limits fail closed even when content is empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/ai/labs/eddi/engine/internal/groups/MemberTurnExecutor.java`:
- Around line 176-185: Update announceArtifactChanges so it drains and snapshots
artifact updates while holding gc.artifactAnnounceMutex(), then releases the
mutex before invoking listener.onArtifactUpdated. Dispatch the queued payloads
through the existing ordered per-conversation writer, preserving artifact order
while preventing slow SSE callbacks from blocking turn completion.
---
Outside diff comments:
In `@src/main/java/ai/labs/eddi/configs/groups/ArtifactValidators.java`:
- Around line 160-169: Update checkMaxLength after parsing spec to reject any
max value less than or equal to zero using the same misconfiguration response as
invalid numeric specifications, ensuring zero and negative limits fail closed
even when content is empty.
🪄 Autofix
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: 858edb7a-748f-4471-9b62-7c5ea6c0472f
📒 Files selected for processing (13)
docs/changelog.mdsrc/main/java/ai/labs/eddi/configs/groups/ArtifactValidators.javasrc/main/java/ai/labs/eddi/configs/groups/model/AgentGroupConfiguration.javasrc/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.javasrc/main/java/ai/labs/eddi/configs/groups/mongo/SharedArtifactStore.javasrc/main/java/ai/labs/eddi/engine/gdpr/GdprComplianceService.javasrc/main/java/ai/labs/eddi/engine/internal/GroupConversationService.javasrc/main/java/ai/labs/eddi/engine/internal/groups/MemberTurnExecutor.javasrc/main/java/ai/labs/eddi/integrations/slack/SlackGroupDiscussionListener.javasrc/main/java/ai/labs/eddi/modules/llm/tools/impl/ArtifactTools.javasrc/test/java/ai/labs/eddi/configs/groups/ArtifactValidatorsTest.javasrc/test/java/ai/labs/eddi/configs/groups/mongo/SharedArtifactStoreTest.javasrc/test/java/ai/labs/eddi/engine/internal/groups/MemberTurnExecutorTest.java
🚧 Files skipped from review as they are similar to previous changes (7)
- src/main/java/ai/labs/eddi/modules/llm/tools/impl/ArtifactTools.java
- src/main/java/ai/labs/eddi/integrations/slack/SlackGroupDiscussionListener.java
- src/main/java/ai/labs/eddi/configs/groups/mongo/SharedArtifactStore.java
- src/main/java/ai/labs/eddi/engine/gdpr/GdprComplianceService.java
- src/test/java/ai/labs/eddi/configs/groups/mongo/SharedArtifactStoreTest.java
- src/main/java/ai/labs/eddi/configs/groups/model/GroupConversation.java
- src/main/java/ai/labs/eddi/engine/internal/GroupConversationService.java
This was referenced Aug 8, 2026
…backs One slow or backpressured SSE client inside onArtifactUpdated blocked every other turn's end-of-turn drain on artifactAnnounceMutex. Exactly one thread at a time is now the publisher: it drains under the mutex, releases it, fires the callbacks, and loops for late arrivals; other threads hand off and leave. Write order is preserved (single announcer over a FIFO queue) and no caller ever blocks on a listener.
aisabella-ai
self-requested a review
August 8, 2026 08:05
aisabella-ai
approved these changes
Aug 8, 2026
ginccc
added a commit
that referenced
this pull request
Aug 8, 2026
…ion, tool provenance - AGENTS.md phase-8 row said 60+ MCP tools while every other doc now says 80+ (actual 82) - the opt-in-by-absence convention over-claimed: only artifactConfig and taskListConfig assemble tools; contextWindow/facilitator/humanMemberConfig gate behaviour, not tool assembly - #636 carried the pre-feature defects, not one of the nine items — the changelog and planning header now say #637-#645 with #636 named separately - the MCP table notes which group tools come from the HITL tool set, since they are not declared in McpGroupTools
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
3 participants
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.
First Wave 2 queue item from
planning/group-collaboration-NEXT.md§3 (design:planning/group-collaboration-improvements-plan.md§I17). Agents can now create together: four member tools —createArtifact,readArtifact,proposeArtifactUpdate,listArtifacts— co-edit typed documents (TEXT/MARKDOWN/JSON) instead of every structured thing being prose the next agent re-parses.Design (the plan's decisions, and its rejections honored)
SharedArtifact+ISharedArtifactStore/SharedArtifactStorefollowGroupConversationStore's single-version runtime-document pattern. The discussion loop persists theGroupConversationdocument whole from stale snapshots, so an embedded list would be clobbered — and because artifacts have their own collection, the tools write through the store directly (unlike I5's task tools, which must mutate the live instance). The F1 registry is still consulted: membership at assembly (getForMember— the group conversation id is caller-supplied, existence is not authorization), liveness at write time.storeIfFieldEquals(String)text-compares, which works on Postgres (data ->> fieldrenders JSON numbers as text) and silently never matches on MongoDB (typed BSON equality). NewstoreIfFieldEquals(…, long)overload onIResourceStorage+ both backends, same no-silent-degrade contract.JSON_SCHEMA/REGEX/MAX_LENGTH, never code. New dependencycom.networknt:json-schema-validator(the victools libs only generate schemas). Specs hard-fail the config save; write-time failures reject with the validator's message; a broken spec fails closed. Content ≤ 256 KB;maxArtifactsPerDiscussion(default 5) counted under the live-instance monitor because PARALLEL phases genuinely race creation.artifact_updatedevents without a listener reference.ToolAssemblyContextcarries no listener (the structural gap that left I5's plannedtask_added_by_agentunfired). Accepted writes queue anArtifactChangeon the live instance;MemberTurnExecutordrains it in afinallyafter every turn and fires the new sink event → SSE (non-terminal) + Slack line. Drained even with a null listener so the queue cannot grow.read_group_conversationcarry them —availableActionsidiom,READ_ONLY). Close/delete cascades to the artifact collection (warn-and-continue: a broken artifact store must not make discussions undeletable). GDPR erasure sweeps user-keyed via a stampedownerUserIdwith the group store's page/exact-recheck/fail-loud contract, as a newGdprComplianceServicestep.markFinalonproposeArtifactUpdatefreezes an artifact (FINAL accepts no further updates) — a small extension beyond the plan's 3-arg signature so the model'sstatusfield is reachable; recorded in the changelog.Tests
148 across 8 classes, all green;
engine.internal+gdpr+ orchestrator regression suites (1807) green; checkstyle clean.enableBuiltInToolsfalse) → contribute nothing.verify(never()).store(…); anchored+escaped filters with Java exact-recheck; not-found message embeds no caller id; erasure paging/fail-loud. Mutation note: degrading the CAS to an unconditional store does not even compile — the gone-document catch becomes unreachable.inOrderartifact-cascade before document delete; cascade-failure still deletes; close cascade. GDPR: new step verified, not-resolvable skip, failure-continues.Summary by CodeRabbit