Fix/ExtensionEndpoint#505
Merged
Merged
Conversation
- so its clear if the full URI is the type or only the identifier needs to be used
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughIntroduce TaskId record and migrate lifecycle identifiers from String to TaskId across interfaces, event/streaming handlers, task implementations, observability, cache keys, tests, and related docs/config. ChangesTaskId Type & Contract Migration
Streaming & Event Infrastructure
Lifecycle Task Implementation Updates
LifecycleManager & Storage Integration
Testing & Configuration Updates
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 7
🧹 Nitpick comments (2)
src/main/java/ai/labs/eddi/modules/rules/impl/RulesEvaluationTask.java (1)
198-198: ⚡ Quick winReuse
TASK_IDwhen buildingExtensionDescriptor.Line 198 recreates
new TaskId(ID)instead of using the existing constant. UsingTASK_IDkeeps a single source of truth for task identity.Proposed change
- var extensionDescriptor = new ExtensionDescriptor(new TaskId(ID)); + var extensionDescriptor = new ExtensionDescriptor(TASK_ID);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/modules/rules/impl/RulesEvaluationTask.java` at line 198, Replace the recreated TaskId instance with the existing TASK_ID constant when constructing the ExtensionDescriptor to avoid duplicating the task identity; locate the ExtensionDescriptor creation in RulesEvaluationTask (the line that currently does new ExtensionDescriptor(new TaskId(ID))) and change it to use TASK_ID so ExtensionDescriptor receives the single source-of-truth TaskId.src/main/java/ai/labs/eddi/modules/templating/OutputTemplateTask.java (1)
207-207: ⚡ Quick winUse
TASK_IDinstead of creating a newTaskId.Line 207 duplicates task-id construction. Reusing
TASK_IDavoids divergence ifIDhandling changes later.Proposed change
- ExtensionDescriptor extensionDescriptor = new ExtensionDescriptor(new TaskId(ID)); + ExtensionDescriptor extensionDescriptor = new ExtensionDescriptor(TASK_ID);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/modules/templating/OutputTemplateTask.java` at line 207, In OutputTemplateTask replace the duplicated TaskId construction by using the existing TASK_ID constant: change the ExtensionDescriptor creation from new ExtensionDescriptor(new TaskId(ID)) to new ExtensionDescriptor(TASK_ID) so the class reuses the canonical TASK_ID (avoid creating a new TaskId instance).
🤖 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 `@AGENTS.md`:
- Around line 444-445: The example was updated to return a TaskId from getId(),
but getExtensionDescriptor() still constructs an ExtensionDescriptor with the
old signature; update getExtensionDescriptor() to create the descriptor using a
TaskId (e.g., pass new TaskId(ID) or the getId() result) so the
ExtensionDescriptor constructor matches the new TaskId type; adjust any
references to ID/KEY_MYFEATURE accordingly to use TaskId where required.
In `@planning/memory-architecture-plan.md`:
- Around line 330-333: Replace the outdated call task.getId().type() with the
updated TaskId field accessor per the new TaskId API (e.g., task.getId().type or
task.getId().getType() depending on whether the API exposes a public field or a
getter) in both places: the auditLedger.logTaskFailure(...) call and the
"task_failed_..." string passed into memory.getCurrentStep().storeData(...),
ensuring you reference the current TaskId contract rather than the removed
.type() method.
In `@planning/observability-and-pipeline-plan.md`:
- Line 34: Update the tracing example to stop calling the deprecated TaskId
accessor: replace the use of task.getId().type() inside the tracer.spanBuilder
call with the current identifier accessor (e.g. task.getId().name() or
task.getId().getName(), depending on your TaskId API). Keep the surrounding
expression Span span = tracer.spanBuilder("eddi.task." + ...) and only change
the TaskId accessor so the span name uses the new name/identifier method.
In
`@src/main/java/ai/labs/eddi/configs/workflows/rest/RestWorkflowStepStore.java`:
- Around line 29-34: The listing currently calls filter.isEmpty() which throws
when filter is null; update the stream filter to treat a null or empty filter as
"no filter" by checking (filter == null || filter.isEmpty()) before calling
type.contains(filter), e.g. in the method that builds the return from
lifecycleExtensionsProvider.keySet().stream() (referencing
lifecycleExtensionsProvider, Provider<ILifecycleTask>, and
getExtensionDescriptor()) so callers passing a null query param no longer cause
an NPE.
In `@src/main/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManager.java`:
- Line 239: The code dereferences task.getId() before calling .name() (e.g., the
.setAttribute call in LifecycleManager where you currently use
Objects.requireNonNullElse(task.getId().name(), "unknown")); change these sites
to null-guard task.getId() first and use a safe extractor such as
Optional.ofNullable(task.getId()).map(TaskId::name).orElse("unknown") or a
ternary guard (task.getId() != null ? task.getId().name() : "unknown") so the
attribute always receives a non-null string; apply the same fix to every
occurrence that currently calls task.getId().name() (lines noted in the review).
In `@src/main/java/ai/labs/eddi/engine/lifecycle/TaskId.java`:
- Around line 45-46: TaskId currently allows blank or malformed names; add
validation in the TaskId canonical constructor to reject null/blank or invalid
formats (throw IllegalArgumentException) and normalize input (e.g., trim) so
instances always have a non-empty valid name; also update any deserialization
path (implement or update TaskId deserializer or a static factory like
TaskId.fromString used by Jackson) to perform the same validation/normalization
so deserialized objects cannot carry invalid values — reference the TaskId
record, its canonical constructor, and the deserialization factory/class (e.g.,
TaskId.fromString or TaskIdDeserializer) when making the changes.
In
`@src/main/java/ai/labs/eddi/engine/runtime/client/workflows/WorkflowStoreClientLibrary.java`:
- Line 87: The code calls componentCache.put(lifecycleTask.getId().name(),
componentKey, component) without validating lifecycleTask.getId(); add a
null-check for lifecycleTask.getId() before deriving the cache key and handle it
by throwing a clear, descriptive exception or logging a configuration/extension
error (e.g., including lifecycleTask and extension identifiers) so
initialization fails with an informative message instead of an NPE; update the
same block where componentCache.put(...) is invoked (referencing
lifecycleTask.getId() and componentCache.put) to perform this validation and
error handling.
---
Nitpick comments:
In `@src/main/java/ai/labs/eddi/modules/rules/impl/RulesEvaluationTask.java`:
- Line 198: Replace the recreated TaskId instance with the existing TASK_ID
constant when constructing the ExtensionDescriptor to avoid duplicating the task
identity; locate the ExtensionDescriptor creation in RulesEvaluationTask (the
line that currently does new ExtensionDescriptor(new TaskId(ID))) and change it
to use TASK_ID so ExtensionDescriptor receives the single source-of-truth
TaskId.
In `@src/main/java/ai/labs/eddi/modules/templating/OutputTemplateTask.java`:
- Line 207: In OutputTemplateTask replace the duplicated TaskId construction by
using the existing TASK_ID constant: change the ExtensionDescriptor creation
from new ExtensionDescriptor(new TaskId(ID)) to new ExtensionDescriptor(TASK_ID)
so the class reuses the canonical TASK_ID (avoid creating a new TaskId
instance).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 89d719b3-5671-4f12-ab6a-8278ddd08bb7
📒 Files selected for processing (36)
AGENTS.mddocs/architecture.mddocs/developer-quickstart.mdmise.tomlplanning/memory-architecture-plan.mdplanning/observability-and-pipeline-plan.mdsrc/main/java/ai/labs/eddi/configs/workflows/model/ExtensionDescriptor.javasrc/main/java/ai/labs/eddi/configs/workflows/rest/RestWorkflowStepStore.javasrc/main/java/ai/labs/eddi/engine/api/IConversationService.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/lifecycle/ConversationEventSink.javasrc/main/java/ai/labs/eddi/engine/lifecycle/ILifecycleTask.javasrc/main/java/ai/labs/eddi/engine/lifecycle/TaskId.javasrc/main/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManager.javasrc/main/java/ai/labs/eddi/engine/runtime/client/workflows/WorkflowStoreClientLibrary.javasrc/main/java/ai/labs/eddi/modules/apicalls/impl/ApiCallsTask.javasrc/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.javasrc/main/java/ai/labs/eddi/modules/mcpcalls/impl/McpCallsTask.javasrc/main/java/ai/labs/eddi/modules/nlp/InputParserTask.javasrc/main/java/ai/labs/eddi/modules/output/impl/OutputGenerationTask.javasrc/main/java/ai/labs/eddi/modules/properties/impl/PropertySetterTask.javasrc/main/java/ai/labs/eddi/modules/rules/impl/RulesEvaluationTask.javasrc/main/java/ai/labs/eddi/modules/templating/OutputTemplateTask.javasrc/test/java/ai/labs/eddi/engine/internal/RestAgentEngineStreamingExtendedTest.javasrc/test/java/ai/labs/eddi/engine/lifecycle/LifecycleManagerTest.javasrc/test/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManagerStreamingTest.javasrc/test/java/ai/labs/eddi/engine/lifecycle/internal/LifecycleManagerTest.javasrc/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallsTaskTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskTest.javasrc/test/java/ai/labs/eddi/modules/mcpcalls/McpCallsTaskTest.javasrc/test/java/ai/labs/eddi/modules/nlp/InputParserTaskTest.javasrc/test/java/ai/labs/eddi/modules/output/OutputGenerationTaskTest.javasrc/test/java/ai/labs/eddi/modules/properties/PropertySetterTaskTest.javasrc/test/java/ai/labs/eddi/modules/properties/impl/PropertySetterTaskTest.javasrc/test/java/ai/labs/eddi/modules/rules/impl/RulesEvaluationTaskTest.java
ginccc
approved these changes
Jun 1, 2026
Merged
This was referenced Jul 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Summary
Frontend branch PR is labsai/EDDI-Manager#85
Introduces a
TaskIdrecord to replace rawStringidentifiers for lifecycle tasks, making task identity type-safe and canonical. Also renames the record field fromtype→nameto avoid confusion withILifecycleTask.getType()which returns a different value.The
/extensionstore/extensionsendpoint was returning bare strings like"ai.labs.parser"in itstypefield, but should always have returned the fulleddi://URI format (e.g.,"eddi://ai.labs.parser") to comply with the MSW. This was an existing bug — the workflow step type is defined as a URI throughout the config model (WorkflowStep.typeisjava.net.URI), and the extension descriptor'stypefield should have been consistent with that. This PR fixes it by wrapping the value inTaskId, which serializes to the canonical URI format.Type of Change
Related Issue
Closes #
Changes Made
TaskIdrecord (ai.labs.eddi.engine.lifecycle.TaskId) — immutable wrapper with canonical URI format (eddi://ai.labs.parser). Serializes as full URI via@JsonValue, deserializes from both"eddi://ai.labs.parser"and bare"ai.labs.parser"via@JsonCreatorfor backward compatibility.ILifecycleTask.getId()return type changed fromString→TaskId. All 8 task implementations updated withstatic final TaskId TASK_IDconstant.ExtensionDescriptor.typechanged fromString→TaskId. Constructor, getter, and setter updated; all taskgetExtensionDescriptor()calls now passnew TaskId(ID). This fixes the/extensionstore/extensionsendpoint to return the canonicaleddi://URI format as required by the MSW.ConversationEventSink,IConversationService.StreamingResponseHandler) —taskIdparameter type changed fromString→TaskId.RestAgentEngineStreamingusestaskId.getIdentifier()for JSON output.LifecycleManager— 8 call sites updated to usetask.getId().name()for bare identifier extraction (metrics, cache keys, audit entries, tracing).WorkflowStoreClientLibrary— component cache key uses.name().TaskId.type→TaskId.name— eliminates naming clash withILifecycleTask.getType()which returns a pipeline stage name (e.g.,"expressions"), not the task identifier.How to Test
./mvnw compile— confirms all type changes compile./mvnw test -pl . -Dtest="InputParserTaskTest,RulesEvaluationTaskTest,ApiCallsTaskTest,PropertySetterTaskTest,OutputGenerationTaskTest,LlmTaskTest,McpCallsTaskTest"— verifies allgetId()andgetExtensionDescriptor()assertionsGET /extensionstore/extensions— verify thattypefields serialize as"eddi://ai.labs.parser"(not bare"ai.labs.parser"), matching the MSW URI conventionPOST /conversation/{id}/saywith SSE — verifytask_startandtask_completeevents contain"taskId": "eddi://ai.labs.parser"formatWorkflowStep.typeviaTaskId.fromValue()backward compatChecklist
./mvnw clean verify -DskipITs)Breaking change details for reviewers:
GET /extensionstore/extensionstypefield"ai.labs.parser""eddi://ai.labs.parser"type— bug fix for MSW compliancetask_start/task_completetaskIdfield"ai.labs.parser""eddi://ai.labs.parser"ILifecycleTask.getId()StringTaskIdBackward compat:
TaskId.fromValue(@JsonCreator)accepts both"eddi://ai.labs.parser"and bare"ai.labs.parser"on deserialization, so existing agent configs and workflow step definitions continue to work without changes.Summary by CodeRabbit
Documentation
Refactor
Chores