fix(core): route per-build hook/middleware mutations through local copies

[xiaowu0203]

Closes #2539

Problem

ReActAgent.Builder.build() mutates the (possibly shared) builder's hooks / middlewares collections in five places — configureLongTermMemory, configureRAG, configureTodoTools, configureSkillBox, and the DynamicSkillMiddleware branch. Reusing a shared builder (as the A2A server does with a single builder bean) races on the non-thread-safe LinkedHashSet / ArrayList (write-while-read during new ArrayList<>(builder.hooks) / mws.addAll(builder.middlewares)), failing construction with AIOOBE/NPE. Because the installed hooks don't override equals/hashCode, each build also leaks a fresh instance into the shared set — rebuilding yields agents carrying 2/3/… copies.

Fix

build() now snapshots hooks / middlewares into per-build local copies, passes them to the configure* methods and the ReActAgent constructor, and never mutates the builder. registerToolsFromHooks runs on the copy taken before configure*, so configure-installed hooks never contribute tools() — matching first-build behaviour and staying consistent across rebuilds.

Tests

ReActAgentSharedBuilderRegressionTest: a deterministic rebuild test — rebuilding a shared builder (task-list + long-term-memory + RAG configured) always yields exactly one TaskReminderMiddleware, one StaticLongTermMemoryHook and one GenericRAGHook. All existing ReActAgent unit tests pass.

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[codecov]

Codecov Report

❌ Patch coverage is 83.33333% with 3 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...e/src/main/java/io/agentscope/core/ReActAgent.java	83.33%	3 Missing ⚠️

📢 Thoughts on this report? Let us know!

[xiaowu0203]

Hi @kratos0718 — the fix is up: #2542
Implemented exactly as discussed: per-build local copies for the five mutation points, plus the deterministic rebuild regression test you suggested (one TaskReminderMiddleware / StaticLongTermMemoryHook/ GenericRAGHook per build, no accumulation). CI is all green.
Would appreciate a review when you have a moment. Thanks again for the precise pointers!

[kratos0718]

Approach looks right to me, and it matches what I'd verified locally — all five mutation points routed through the per-build copies, and passing buildHooks into registerToolsFromHooks is cleaner than leaving it reading the field. Passing the list straight to super(...) is safe too, since AgentBase re-wraps it in a CopyOnWriteArrayList.

Two notes, neither a blocker.

1. configureSkillBox still mutates shared state — same bug family, not covered here.

The hooks/middlewares collections are fixed, but the skillBox field is itself a shared object, and build() still writes to it:

private void configureSkillBox(Toolkit agentToolkit, List<Hook> buildHooks) {
    skillBox.bindToolkit(agentToolkit);   // SkillBox.java:146 -> this.toolkit = toolkit
    skillBox.registerSkillLoadTool();
    if (skillBox.isAutoUploadSkill()) {
        skillBox.uploadSkillFiles();
    }
    buildHooks.add(new SkillHook(skillBox));
}

SkillHook holds private final SkillBox skillBox (SkillHook.java:46) — the same instance for every agent. So on a shared builder:

• build #1 → skillBox.toolkit = agent 1's toolkit copy
• build #2 → skillBox.toolkit = agent 2's toolkit copy

and agent 1's SkillHook now reaches agent 2's toolkit. setSkillActive() uses that field at runtime (this.toolkit.updateToolGroups(...), SkillBox.java:343-354), so activating a skill on agent 1 mutates agent 2's tool groups. Concurrently, two build() calls race on the same write.

That's the same failure mode as #2539, just through an object field rather than a collection, and it's the exact configuration the A2A server uses. Might be worth a follow-up issue rather than growing this PR — the fix is a different shape (per-agent SkillBox view, or move the bind to construction time).

registerSkillLoadTool() and uploadSkillFiles() also re-run on every build against the shared box, which is at least wasted work on rebuild.

2. Minor: codecov flags 3 uncovered lines. The skillBox != null and dynamic-skills branches aren't exercised by the new test — the RAG / long-term-memory / task-list paths are. Not important if you'd rather keep the test focused, just noting which of the five are actually covered.

[xiaowu0203]

Thanks @kratos0718 for the review!
Re: 1. SkillBox shared state — you're right, it's the same failure family (shared object field written on every build) but a different shape. I agree it deserves a follow-up rather than growing this PR happy to file an issue for it, or leave it to your team. I can help with the fix either way.
Re: 2. codecov lines — acknowledged; I kept the regression test focused on the collection paths, happy to add skill-box/dynamic-skills coverage if you'd prefer.
Thanks again for verifying the approach locally!

[kratos0718]

Agreed on both, and no need to file it — I'd already opened #2543 for the SkillBox half just before your reply, so we don't collide.

It's scoped to stay independent of this PR: it only changes configureSkillBox to take a per-agent SkillBox.copy(agentToolkit) (mirroring the existing Toolkit.copy()) and leaves the hooks.add line alone, so the two apply cleanly in either order. That's also the direction @fang-tech described back in #979 — "the same automatic copying method as the toolkit" — and it drops the "create a new skillBox for each ReActAgent" workaround given there.

On coverage: I'd leave your test focused where it is. #2543 carries its own tests for the skill-box path, so between the two PRs the branches are covered without either test having to reach across.

[xiaowu0203]

Thanks @kratos0718
appreciate you picking up the SkillBox half (#2543) and the #979 context. I'll leave the regression test focused as suggested. Looking forward to both landing.

[oss-maintainer]

⚠️ Merge conflict detected

This PR has conflicts with the base branch and cannot be merged. Please rebase or merge the base branch into your branch and resolve the conflicts:

git fetch origin
git checkout fix/shared-builder-build-mutation
git rebase origin/main
# resolve conflicts, then:
git push --force-with-lease

This is a one-time reminder. Feel free to @mention me for a re-review after conflicts are resolved.

---

Automated notification by "github-manager-bot"

[oss-maintainer]

Summary

Reviewed the changes in this PR. The modifications look reasonable and follow the project conventions.

---

Automated review by "github-manager-bot"

[xiaowu0203]

⚠️ Merge conflict detected

This PR has conflicts with the base branch and cannot be merged. Please rebase or merge the base branch into your branch and resolve the conflicts:

git fetch origin
git checkout fix/shared-builder-build-mutation
git rebase origin/main
# resolve conflicts, then:
git push --force-with-lease

This is a one-time reminder. Feel free to @mention me for a re-review after conflicts are resolved.

Automated notification by "github-manager-bot"

@oss-maintainer Conflicts resolved
rebased onto current main and full mvn clean verify is green. Please re-review. Thanks!

[AgentScopeJavaBot]

🤖 AI Review

Fixes shared builder race condition by snapshotting hooks/middlewares into per-build local copies. Core fix is correct and well-tested. Two pre-existing concurrency concerns (ToolkitAware rebind, skillBox bindToolkit) noted as follow-up items, not blocking. Recommendation: APPROVE.

[AgentScopeJavaBot]

🤖 AI Review

Fixes shared builder race condition by snapshotting hooks/middlewares into per-build local copies. Core fix is correct and well-tested. Two pre-existing concurrency concerns (ToolkitAware rebind, skillBox bindToolkit) noted as follow-up items, not blocking. Recommendation: APPROVE.

[kratos0718]

On the two follow-ups the AI review flagged — the skillBox bindToolkit one already has a PR open: #2543, raised alongside this one.

configureSkillBox() calls skillBox.bindToolkit(agentToolkit) on the shared SkillBox builder field, and SkillHook holds that same instance, so a second build() repoints it at agent 2's toolkit and setSkillActive() then mutates the wrong agent's tool groups. #2543 gives each agent its own copy, with a regression test that reproduces it deterministically from two sequential build() calls — no threads, so it will not be flaky. It is green, the CLA is signed, and it merges cleanly against main.

So that item is covered whenever a maintainer has a moment for it; it does not need to hold up this PR.

The other one, ToolkitAware rebind, is not covered by either PR. Happy to take that as a third if it would be useful — just say the word and I will open it against main.

[xiaowu0203]

Thanks @kratos0718 — yes, please go ahead and open that third PR for the ToolkitAware rebind. Much appreciated!