refactor(llm): make AgentOrchestrator injectable; cover agent-mode response metadata - #604
Merged
Merged
Conversation
…tadata
LlmTask built its own AgentOrchestrator, so a test could only substitute a
mock through getDeclaredField("agentOrchestrator") reflection. Make
AgentOrchestrator — and its ConversationHistoryBuilder dependency —
@ApplicationScoped, and field-inject the orchestrator into LlmTask while
keeping the constructor's instance as the direct-construction fallback, so
every existing call site compiles unchanged and the 41-parameter signature
is untouched (removing the 22 orchestrator-only parameters would leave them
dead rather than merely numerous; that is a separate mechanical change).
Add LlmTaskAgentModeMetadataTest covering responseMetadata surfacing on the
live agent branch, the legacy fallback when the orchestrator declines, and
the HITL resume continuation. Contrary to the note in the previous entry,
the agent branches were already covered — every stub just built its result
with the two-arg ExecutionResult convenience constructor, which hardcodes
an empty map, so ignoring responseMetadata() looked identical to honouring
it. Reverting 15b7a08 turns exactly three of the five new tests red.
Drop the three reflection hacks now that the field is assignable.
CDI wiring is not verifiable locally (quarkus:build augmentation needs a
loopback socket); these are the repo's first package-private
@ApplicationScoped beans, so CI is the gate for proxy generation.
Follow-up to 061e894, applying what a two-reviewer pass surfaced. LlmTask's constructor drops from 41 parameters to 20: the 22 that existed only to feed `new AgentOrchestrator(...)` leave with the orchestrator, as does attachmentStore, while AgentOrchestrator and ConversationHistoryBuilder come in. The agentOrchestrator field returns to `private final` — no field injection, no fallback instance, no reflection. All 10 call sites across 9 test files are rewritten, along with the mocks, imports and locals the removal orphaned. Net -147 lines. AgentOrchestrator now injects its own attachment services instead of having them pushed in by LlmTask's @PostConstruct. That wiring only ran when the lazily-created LlmTask happened to exist, so any other future injector of the newly-injectable bean would have received null attachment services and silently lost the readAttachment tool. setAttachmentServices survives only as a seam for directly-constructed orchestrators. responseMetadata is copied rather than aliased at all three agent sites. The published map reaches conversation memory and the {{llmMeta}} namespace, and ExecutionResult's two-arg constructor yields an immutable Map.of(), so a later metadata write would have thrown on the agent path in production only. Tests: a sixth case covers the skipCascade agent branch. The fix has three separately-revertable assignments and the original five reached only two of them — deleting the skipCascade line left all five green. Reverting only that line now fails only that test. Fixtures set enableBuiltInTools so isAgentMode() is true, instead of stubbing a non-null agent result onto a config for which the real orchestrator always returns null. Mutation score is 4 of 6, down from 5: the defensive copy costs the empty-metadata test its identity assertion, since pre- and post-fix both publish an empty HashMap. The javadoc and changelog say so at the assertion rather than leaving the stronger claim standing. A false dependency-cycle justification in the changelog is retracted in place. Full suite: 11700 tests, failure set byte-identical to baseline (302/302, no swaps). CDI wiring still needs CI — augmentation cannot run locally.
Corrects a claim I made repeatedly in this session and left standing in 84d2c67's message: that CDI wiring "cannot be validated locally" because augmentation needs a loopback socket. It does not. `./mvnw package -DskipTests` runs Quarkus augmentation in ~13s, binding no ports and touching no database, and ArC emits AgentOrchestrator_ClientProxy and ConversationHistoryBuilder_ClientProxy into generated-bytecode.jar. So the one open risk on this branch — whether ArC proxies package-private @ApplicationScoped beans, a first for this repo — is closed locally rather than deferred to CI. Resolution of the orchestrator's 29-dependency constructor is confirmed by the same run, since an unsatisfied injection point fails augmentation outright. The error was over-generalizing a real limitation: tests that bind a loopback socket (*IT.java, HTTP-server tests) genuinely cannot run here. That says nothing about quarkus:build, and I asserted the extension three times without testing it.
Contributor
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThe refactor makes ChangesAgent orchestration and metadata flow
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant LlmTask
participant AgentOrchestrator
participant ConversationMemory
participant TemplateData
LlmTask->>AgentOrchestrator: execute agent mode
AgentOrchestrator-->>LlmTask: return trace and responseMetadata
LlmTask->>ConversationMemory: publish copied metadata
LlmTask->>TemplateData: publish configured metadata
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR refactors the LLM module to make AgentOrchestrator a CDI-injectable collaborator (removing reflection-based seams in tests) and adds targeted tests that exercise agent-mode response metadata (not just branch coverage). It also includes several related backend hardening changes around quotas, orphan purge safety, and Jackson REST vs persistence serialization.
Changes:
- Refactor
LlmTaskto injectAgentOrchestrator+ConversationHistoryBuilder, reduce constructor parameters, and correctly surface/copy agent-moderesponseMetadata(including resume). - Split REST vs persistence Jackson behavior via
@PersistenceMapper(RESTInstantas ISO-8601; persistence remains numeric) and pinYearMonthREST shape forUsageSnapshot.costMonth. - Tighten operational safety/consistency: tenant quota boundary comparisons (
>=), agent-capacity quota enforcement on deploy, orphan purge refusal on incomplete reference scans, and consistent 429 surfacing for async quota denials.
Reviewed changes
Copilot reviewed 44 out of 44 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java | Injects orchestrator/history builder; surfaces agent-mode metadata (incl. resume) and removes reflection seams. |
| src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java | Makes orchestrator CDI-managed; moves attachment wiring into CDI field injection. |
| src/main/java/ai/labs/eddi/modules/llm/impl/ConversationHistoryBuilder.java | Marks history builder @ApplicationScoped so it can be injected. |
| src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskAgentModeMetadataTest.java | New tests proving agent-mode metadata (token usage) reaches memory + template namespace across branches and resume. |
| src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskTest.java | Updates direct-construction test wiring for the new LlmTask constructor signature. |
| src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskResumeModeTest.java | Removes reflection injection; passes orchestrator mock via constructor. |
| src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskExtendedTest.java | Updates constructor wiring due to orchestrator injection refactor. |
| src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskExtendedBranchTest.java | Updates constructor wiring; removes now-unneeded tool/tenant mocks in construction. |
| src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskDeepBranchTest.java | Updates constructor wiring; removes now-unneeded tool/tenant mocks in construction. |
| src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskCoverageTest.java | Removes reflection seam; steers agent branches through injected orchestrator mock. |
| src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskCoverage2Test.java | Removes reflection seam; steers agent branches through injected orchestrator mock. |
| src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskConfigureTest.java | Updates constructor wiring after refactor. |
| src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskBranchTest.java | Updates constructor wiring after refactor. |
| src/main/java/ai/labs/eddi/datastore/serialization/SerializationCustomizer.java | Applies REST-only Instant ISO override via mapper configOverride. |
| src/main/java/ai/labs/eddi/datastore/serialization/PersistenceMapper.java | New qualifier to select persistence ObjectMapper. |
| src/main/java/ai/labs/eddi/datastore/serialization/PersistenceMapperProducer.java | Produces persistence-qualified mapper built from shared configuration recipe. |
| src/main/java/ai/labs/eddi/datastore/serialization/JsonSerialization.java | Switches to injecting the persistence-qualified mapper. |
| src/test/java/ai/labs/eddi/datastore/serialization/SerializationCustomizerInstantFormatTest.java | New tests pinning REST vs persistence Instant serialization and back-compat deserialization. |
| src/main/java/ai/labs/eddi/engine/tenancy/model/UsageSnapshot.java | Pins REST YearMonth as ISO string via @JsonFormat. |
| src/test/java/ai/labs/eddi/engine/tenancy/UsageSnapshotSerializationTest.java | New tests pinning UsageSnapshot.costMonth wire shape and round-trip behavior. |
| src/main/java/ai/labs/eddi/engine/tenancy/TenantQuotaService.java | Adds checkAgentQuota read-only gate and metrics parity with other quota checks. |
| src/test/java/ai/labs/eddi/engine/tenancy/TenantQuotaServiceTest.java | Adds coverage for agent-capacity quota behavior + metrics semantics. |
| src/main/java/ai/labs/eddi/engine/tenancy/MongoTenantQuotaStore.java | Aligns at-limit comparison with service semantics (>=). |
| src/main/java/ai/labs/eddi/engine/tenancy/PostgresTenantQuotaStore.java | Aligns at-limit comparison with service semantics (>=). |
| src/test/java/ai/labs/eddi/engine/tenancy/MongoTenantQuotaStoreTest.java | Adds boundary test for “exactly at limit” behavior. |
| src/test/java/ai/labs/eddi/engine/tenancy/PostgresTenantQuotaStoreTest.java | Adds boundary test for “exactly at limit” behavior. |
| src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java | Ensures async say-path quota denials surface as 429 (mirroring mapper). |
| src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineTest.java | Adds test asserting 429 + Retry-After + body for async quota denial. |
| src/main/java/ai/labs/eddi/engine/internal/RestAgentAdministration.java | Enforces maxAgentsPerTenant on deploy and throws quota exception before async execution. |
| src/test/java/ai/labs/eddi/engine/internal/RestAgentAdministrationTest.java | Updates constructor wiring; makes quota gate transparent for existing behavior tests. |
| src/test/java/ai/labs/eddi/engine/internal/RestAgentAdministrationExtendedTest.java | Updates constructor wiring; makes quota gate transparent for existing behavior tests. |
| src/test/java/ai/labs/eddi/engine/internal/RestAgentAdministrationQuotaTest.java | New tests pinning agent-capacity gate behavior (deny, counting, fail-open, loophole coverage). |
| src/main/java/ai/labs/eddi/engine/api/IRestAgentAdministration.java | Documents deploy endpoint quota behavior and adds 429 response documentation. |
| src/main/java/ai/labs/eddi/engine/setup/AgentSetupService.java | Surfaces quota-denial reasons when calling CDI admin bean directly. |
| src/main/java/ai/labs/eddi/engine/mcp/McpAdminTools.java | Surfaces quota-denial reasons to MCP clients to prevent retry loops. |
| src/main/java/ai/labs/eddi/datastore/DescriptorStore.java | Fixes includeDeleted semantics to be inclusion-based (true = no deleted constraint). |
| src/test/java/ai/labs/eddi/datastore/DescriptorStoreTest.java | Adds tests asserting correct includeDeleted filter construction. |
| src/main/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdmin.java | Adds scan completeness tracking, purge refusal (409) on incomplete reference scan, and fixes descriptor paging semantics. |
| src/main/java/ai/labs/eddi/configs/admin/IRestOrphanAdmin.java | Updates REST docs/defaults: includeDeleted default false for purge; documents 409 on incomplete scan. |
| src/test/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdminSafetyTest.java | New tests pinning paging correctness and purge safety on incomplete scans + includeDeleted forwarding. |
| src/test/java/ai/labs/eddi/configs/admin/rest/RestOrphanAdminBranchTest.java | Updates display names to match renamed scan method naming. |
| docs/deployment-management-of-agents.md | Documents updated orphan scan/purge semantics, defaults, and 409 refusal behavior. |
| docs/superpowers/specs/2026-07-21-manager-coverage-backend-design.md | Adds design/verification notes for manager-coverage-related backend patches. |
| docs/changelog.md | Records the refactor, rationale, and verification notes for the changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
executeTask assigned agentResult.trace() straight into toolTrace while executeResume null-guarded it, so the later `!toolTrace.isEmpty()` check could throw. Both agent branches now guard, matching the resume path. Flagged by all three reviewers and previously deferred here as "pre-existing" — a thin defence when this branch already edits those exact lines. Scope, stated precisely because the review overstated it: no production path returns a null trace. Both ExecutionResult construction sites pass a fresh ArrayList, and executeIfToolsEnabled returns null rather than a partial result. This is hardening plus consistency, not a live bug fix. The shape is legal though — AgentOrchestratorTest constructs `new ExecutionResult(null, null)` — so the guard has a real contract to defend. Mutation-verified: removing it yields `NullPointerException: Cannot invoke "java.util.List.isEmpty()" because "toolTrace" is null`, failing exactly the new agentMode_nullTrace_doesNotThrow and nothing else. Full suite: 11701 tests, failure set byte-identical to baseline (302/302). Not applied from the same review: the PersistenceMapperProducer finding (rated High) is incorrect — WRITE_DATES_AS_TIMESTAMPS already defaults to true and a hand-built ObjectMapper ignores quarkus.jackson config, so the suggested change is a no-op. Verified by pointing the test at the real producer: 7/7 green. The genuine defect it brushed against is in that test, not production, and belongs to the branch that owns the file; recorded in the changelog.
…trator-injectable # Conflicts: # docs/changelog.md
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Copilot flagged that the javadoc and changelog did not add up: "four" red plus "the other two" guards cannot describe seven tests. The inconsistency was real; both the number and the proposed repair were wrong, in opposite directions. The review suggested naming resumeMode_nullResult_publishesEmptyMetadata as the third guard. That test discriminates: pre-fix, executeResume published no metadata at all, so its verify() fails outright rather than observing an empty map. Re-measuring by actually reverting the fix gives FIVE of seven red; the only survivors are agentMode_emptyMetadata_publishesEmptyMap and agentReturnsNull_fallsBackToLegacyChatExecutor. Root cause of the stale figure: the count was updated by hand across two changes that moved it in opposite directions. The defensive copy demoted agentMode_emptyMetadata_publishesEmptyMap from discriminator to guard (identity assertion became assertNotSame), and the later null-trace test added a discriminator back — it asserts tokenUsage is surfaced, not merely that nothing throws. Netting those by inspection produced a number that had never been observed. Both documents now carry the measured figure, and the javadoc tells the next reader to re-measure rather than adjust the count by hand.
![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: 1
🧹 Nitpick comments (1)
src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskBranchTest.java (1)
108-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a top-level
SimpleMeterRegistryimport.Add
import io.micrometer.core.instrument.simple.SimpleMeterRegistry;and replace the inline qualified references; no same-name ambiguity is shown.
src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskBranchTest.java#L108-L108: usenew SimpleMeterRegistry().src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskConfigureTest.java#L107-L107: usenew SimpleMeterRegistry().src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskCoverage2Test.java#L134-L134: usenew SimpleMeterRegistry().src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskCoverageTest.java#L122-L122: usenew SimpleMeterRegistry().src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskDeepBranchTest.java#L122-L122: usenew SimpleMeterRegistry().src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskDeepBranchTest.java#L498-L498: usenew SimpleMeterRegistry().src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskExtendedBranchTest.java#L114-L114: usenew SimpleMeterRegistry().src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskExtendedBranchTest.java#L479-L479: usenew SimpleMeterRegistry().As per coding guidelines, “Reference types and annotations through top-level imports; do not use inline fully qualified names except to disambiguate 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/modules/llm/impl/LlmTaskBranchTest.java` at line 108, Replace the inline fully qualified SimpleMeterRegistry references with a top-level import and the short type name. Apply this in src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskBranchTest.java:108, LlmTaskConfigureTest.java:107, LlmTaskCoverage2Test.java:134, LlmTaskCoverageTest.java:122, LlmTaskDeepBranchTest.java:122 and 498, and LlmTaskExtendedBranchTest.java:114 and 479; no same-name ambiguity requires qualification.Source: Coding guidelines
🤖 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/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskAgentModeMetadataTest.java`:
- Around line 249-255: Extend the assertion for stored metadata in
LlmTaskAgentModeMetadataTest so it performs a put operation on stored and
verifies the map is writable, while retaining the existing assertNotSame check
against emptyMetaResult.responseMetadata(). Use the put to confirm the published
defensive copy supports later metadata writes.
---
Nitpick comments:
In `@src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskBranchTest.java`:
- Line 108: Replace the inline fully qualified SimpleMeterRegistry references
with a top-level import and the short type name. Apply this in
src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskBranchTest.java:108,
LlmTaskConfigureTest.java:107, LlmTaskCoverage2Test.java:134,
LlmTaskCoverageTest.java:122, LlmTaskDeepBranchTest.java:122 and 498, and
LlmTaskExtendedBranchTest.java:114 and 479; no same-name ambiguity requires
qualification.
🪄 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: 91bc0b59-2f6e-47ef-80c1-e637d4b8174c
📒 Files selected for processing (14)
docs/changelog.mdsrc/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.javasrc/main/java/ai/labs/eddi/modules/llm/impl/ConversationHistoryBuilder.javasrc/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.javasrc/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskAgentModeMetadataTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskBranchTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskConfigureTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskCoverage2Test.javasrc/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskCoverageTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskDeepBranchTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskExtendedBranchTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskExtendedTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskResumeModeTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskTest.java
…ry FQNs Two review findings applied, one declined. Applied — the empty-metadata guard asserted assertNotSame, a property Map.copyOf also satisfies while still throwing the moment downstream code adds a metadata key. That is precisely the failure the defensive copy exists to prevent, so the assertion was pinning the wrong thing. It now probes an actual put. Applied — 25 inline `new io.micrometer...SimpleMeterRegistry()` across nine LlmTask*Test classes replaced with a top-level import (AGENTS.md 4.7). Pre-existing, but this branch carried them forward when it rewrote the constructor call sites. Declined — privatising AgentOrchestrator's two @Inject attachment fields. src/main has 43 package-private @Inject fields and zero private ones, so this would be the only exception; GroupConversationService.attachmentStore is the same shape and is written directly by its test; and Quarkus recommends package-private here to avoid reflective injection, which the roadmap's native-image work would pay for. The cited risk of in-package mutation is unchanged either way, since the package-private setAttachmentServices setter beside the fields exists so the eight AgentOrchestrator*Test classes can write them. Full suite: 11713 tests, failure set byte-identical to baseline (302/302).
aisabella-ai
approved these changes
Jul 22, 2026
pull Bot
pushed a commit
to Stars1233/EDDI
that referenced
this pull request
Jul 24, 2026
Brings in PR labsai#604, which made AgentOrchestrator an @ApplicationScoped bean injected into LlmTask rather than constructed with `new` from 22 pass-through constructor parameters. That is the same hot pair of files this branch reworked, so the merge needed real resolution rather than a rubber stamp. Conflicts (3): - AgentOrchestrator imports — kept both sides. labsai#604 added @ApplicationScoped and @Inject; this branch added ConfigProvider. - AgentOrchestrator constructor — kept both sides: this branch's tokenCounterFactory field and labsai#604's @Inject. CDI supplies the factory automatically now that the class is a bean. - LlmTask — took main's side wholesale; the orchestrator is injected, so the `new AgentOrchestrator(...)` call and its 22 parameters are gone. - docs/changelog.md — both sides prepend; kept both blocks, this branch's entries newest-first. Verified every heading present on main survives. Two semantic breaks git merged silently and cleanly, neither visible to a `mvn clean compile` of the main sources: 1. LlmTaskAuditLedgerTest (added by this branch) constructed LlmTask with the pre-labsai#604 40-argument signature and then reached in via reflection to set agentOrchestrator. Both sides edited different regions of the file, so the merge succeeded and produced code that cannot compile. Rewritten against the 20-argument constructor; the reflection hack is no longer needed. 2. LlmTaskAgentModeMetadataTest (added by labsai#604) stubs executeIfToolsEnabled and resumeToolLoop at the arity main had. This branch added a JsonResponseFormatPolicy parameter, so the stubs no longer matched, Mockito returned null, and LlmTask fell through to the legacy branch — 6 of 7 tests died on `ChatResponse.aiMessage() because "messageResponse" is null`. Matcher lists widened to the arity production actually calls. This also de-vacuums the `verify(never()).executeIfToolsEnabled(...)` at :380, which was checking an overload nothing invokes and would have passed regardless. Also corrects a javadoc claim this branch made that labsai#604 invalidated: the BUDGET_ENFORCE_DEFAULT comment justified reading through ConfigProvider on the grounds that AgentOrchestrator "is not a CDI bean". It is one now. The code is unchanged and still correct — the field is static final, which @ConfigProperty cannot target either way — but the stated reason was wrong and would have misled the next reader. Verification: ./mvnw clean compile, ./mvnw test-compile and ./mvnw validate all green; 830 tests across 32 classes covering the merge surface, 0 failures.
This was referenced Jul 28, 2026
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.
Problem
LlmTaskbuilt its ownAgentOrchestratorinside a 41-parameter constructor, so tests could only substitute a mock throughgetDeclaredField("agentOrchestrator")reflection.The premise that motivated this work — "no existing test exercises the agent-mode branches" — turned out to be false, and the real gap is more interesting.
LlmTaskCoverage2TestandLlmTaskResumeModeTestalready drove ~20 tests through those branches via that reflection hack. But every stub built its result with the two-argExecutionResultconvenience constructor, which hardcodesMap.of(). Against an always-empty map, ignoringresponseMetadata()is indistinguishable from honouring it.Branch coverage hid a data-flow gap. That is why agent-mode token usage could be computed and then silently dropped while every branch stayed green.
Changes
AgentOrchestratorandConversationHistoryBuilder→@ApplicationScoped; the orchestrator's constructor signature is unchanged, so the 8AgentOrchestrator*Testclasses are untouched.LlmTask's constructor: 41 parameters → 20. 22 existed solely to feednew AgentOrchestrator(...), plusattachmentStore; all 23 left with the orchestrator.agentOrchestratoris now a plainprivate finalconstructor argument — no reflection, no field-injection seam, no fallback instance.AgentOrchestratorinjects its own attachment services. PreviouslyLlmTask's@PostConstructpushed them in, which only ran if the lazily-createdLlmTaskexisted — so any other future injector of the newly-injectable bean would have received null attachment services and silently lost thereadAttachmenttool.responseMetadatais copied rather than aliased at all three agent sites: the published map reaches conversation memory and the{{llmMeta}}namespace, and an immutableMap.of()would have made a later metadata write throw on the agent path in production only.Verification
./mvnw package -DskipTestscompletes augmentation and ArC emitsAgentOrchestrator_ClientProxyandConversationHistoryBuilder_ClientProxy— these are the repo's first package-private@ApplicationScopedbeans, so proxyability was the one real unknown. An unsatisfied injection point in the 29-dependency constructor would have failed the build, so graph resolution is confirmed too.15b7a08a7turns 4 of the 6 new tests red. The fix has three separately-revertable call sites (skipCascade, standard,executeResume) and there is a discriminating test per site, so a partial revert cannot slip through — an earlier draft of these tests missed theskipCascadesite entirely and stayed green when that line was deleted.Known trade-off
Adding the defensive copy cost a discriminator. The empty-metadata test previously asserted map identity, which the copy breaks — pre- and post-fix both publish an empty
HashMap, genuinely indistinguishable. Mutation score is 4-of-6 rather than 5-of-6. Deliberate: the latentUnsupportedOperationExceptionwas judged worth more than the assertion.Not addressed (pre-existing)
executeTaskdoes not null-guardtrace()whileexecuteResumedoes;AgentOrchestratorTestconstructsnew ExecutionResult(null, null)as a legal shape, so a null trace would NPE at the!toolTrace.isEmpty()check.LlmTask*tests use bareopenMocks(this)with noMockitoExtension, so strict-stubs never runs and theirlenient()calls are decorative.Summary by CodeRabbit
Bug Fixes
Documentation