Conversation
Fixes agentscope-ai#3167 Tool group activation is resolved from each (userId, sessionId) slot's AgentState, while the toolkit's own activation flags are per-call scratch state re-seeded from that AgentState at every call entry. Calling agent.getToolkit().updateToolGroups(...) between calls therefore appeared to succeed (toolkit.getActiveGroups() reported the change) but never reached the model, and there was no supported API to change a session's active groups from application code. - ReActAgent: add getActiveToolGroups / updateToolGroups / setActiveToolGroups keyed by (userId, sessionId) or RuntimeContext. They validate group names against the toolkit, reload the latest persisted state when a store is configured, mutate the session's AgentState and persist it, following the existing get -> mutate -> save pattern used by setPermissionMode. - ReActAgent: log a one-shot warning when the toolkit's active groups are found changed outside of any call, pointing to the per-session API. - HarnessAgent: delegate the new methods; document on getToolkit() that mutating the returned toolkit between calls has no effect. - Docs (en/zh): new "Activating groups programmatically (per session)" section in building-blocks/tool.md with a warning about the old pattern. - Tests: per-session activation visibility and isolation, replacement semantics with persistence across engine restarts, RuntimeContext and default-session overloads, unknown-group rejection, reload-on-top-of- latest-persisted-state, discarded out-of-call toolkit mutation, and meta-tool (reset_equipped_tools) in-call activation not being flagged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved critical correctness issues in ReActAgent can violate session isolation and configured tool-deletion behavior.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 3
Open (4)
What changed in this PR
Adds per-session tool-group activation APIs for ReActAgent and HarnessAgent, with persistence, diagnostics, tests, and bilingual documentation.
Changes:
- Adds session- and
RuntimeContext-scoped activation, update, and replacement APIs. - Adds warnings for ineffective direct toolkit mutations.
- Updates delegation, tests, and tool documentation.
| File | Summary |
|---|---|
docs/v2/zh/docs/building-blocks/tool.md |
Documents per-session tool-group activation. |
docs/v2/en/docs/building-blocks/tool.md |
Documents per-session tool-group activation. |
agentscope-harness/src/test/java/io/agentscope/harness/agent/HarnessAgentTest.java |
Tests delegation and session isolation. |
agentscope-harness/src/main/java/io/agentscope/harness/agent/HarnessAgent.java |
Delegates the new APIs and updates Javadocs. |
agentscope-core/src/test/java/io/agentscope/core/agent/ReActAgentPerSessionStateTest.java |
Tests activation, persistence, and diagnostics. |
agentscope-core/src/main/java/io/agentscope/core/ReActAgent.java |
Implements APIs and diagnostics. Critical issues remain around concurrent state persistence, deletion protection, and conflict-policy merging; a lifecycle accounting nit also remains. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // The toolkit's activation flags are per-call scratch state seeded from the session: | ||
| // the model-facing tool surface is resolved from state.getToolContext() (see | ||
| // reasoning()), and in-call changes (meta tool, skills) flow back via | ||
| // syncToolkitToState. Anything written to the flags between calls is discarded here. | ||
| toolkit.setActiveGroups(loaded.getToolContext().getActivatedGroups()); |
| } else { | ||
| groups.remove(groupName); | ||
| } | ||
| } | ||
| state.getToolContext().setActivatedGroups(groups); |
| state.getToolContext().setActivatedGroups(groups); | ||
| saveAgentState(userId, sid); |
| // Floor at zero: a call cancelled while waiting on the same-session gate never ran | ||
| // beforeAgentExecution but still reaches this cleanup. | ||
| inFlightCalls.updateAndGet(n -> Math.max(0, n - 1)); |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Adds the missing per-session tool-group activation API (getActiveToolGroups / updateToolGroups / setActiveToolGroups on ReActAgent, delegated by HarnessAgent), following the same load-latest → mutate → saveAgentState shape already used by clearContext / setPermissionMode, plus a one-shot diagnostic for the silently-discarded agent.getToolkit().updateToolGroups(...) pattern and matching en/zh docs. The direction is right, the Javadoc is honest about the caveats, and the 7 core + 1 harness tests cover the sequential paths well. Two concurrency questions keep this from being a clean approve; both are about what happens when an update lands while a call on the same (userId, sessionId) slot is in flight. CLA signed; validate / Check License / Module Sync green, ubuntu and windows builds still pending at review time.
Findings
- [Warning]
ReActAgent.java:4467— under the defaultConflictPolicy.OVERWRITE, an update during an in-flight call on the same session is silently reverted by that call's ownsyncToolkitToState+ stale-version save; the javadoc asks callers not to do this but nothing enforces it. - [Warning]
ReActAgent.java:894—inFlightCallsis incremented inbeforeAgentExecutionbut decremented unconditionally inafterAgentExecution, so a call cancelled at the same-session gate can drop the counter to 0 while another call runs, firing a false out-of-call warning and consuming the one-shot flag. - [Info]
ReActAgent.java:4498— the setters persist even when nothing changed (extraagent_stateIO, version bump, and the CAS conflict above); an equality early-return would make them idempotent. - [Info]
ReActAgentPerSessionStateTest.java:640— update-while-in-flight and two concurrent sessions (single per-agenttoolkitGroupsAtLastCallBoundary) are the two behaviours the safety story rests on, and neither is asserted.
Suggestions
For the in-flight case, either reject the call for a slot that has a live call (a per-slot in-flight set you already half-have), or apply the mutation to the slot's cached state and let the next activateSlotForContext pick it up without the in-flight call writing its own list back afterwards. For the diagnostic, carrying a per-call boolean on CallExecution (or reading it back from the Reactor context in the cleanup hook) avoids the counter asymmetry entirely.
Automated review by github-manager-bot
| } | ||
| } | ||
| state.getToolContext().setActivatedGroups(groups); | ||
| saveAgentState(userId, sid); |
There was a problem hiding this comment.
An out-of-call update on a slot that has a call in flight is silently reverted under the default ConflictPolicy.OVERWRITE, which is exactly the failure mode this PR exists to remove. Sequence: the in-flight CallExecution holds its own AgentState seeded with G0; updateToolGroups reloads + persists G0+X; the in-flight call then hits syncToolkitToState(...) (consumeSystemMsgAfterPreCall, the per-iteration write-back, and saveStateToSession), which writes the toolkit's G0 back into its state and persists with the now-stale scope.loadedVersion -> CAS conflict -> OVERWRITE -> the session is back on G0 and the user's activation is lost, with only a generic agent_state CAS conflict — OVERWRITE applied line to explain it. The javadoc asks callers to invoke this after the current call completes, but nothing enforces it and the outcome is a lost write rather than a no-op. Suggested: keep a per-slot in-flight marker (you already have the counter idea here) and either throw/reject for a slot with a live call, or defer the mutation into stateCache and let the next activateSlotForContext pick it up. Either way a test for "update while the same session's call is in flight" would pin the chosen behaviour.
| } | ||
| // Floor at zero: a call cancelled while waiting on the same-session gate never ran | ||
| // beforeAgentExecution but still reaches this cleanup. | ||
| inFlightCalls.updateAndGet(n -> Math.max(0, n - 1)); |
There was a problem hiding this comment.
inFlightCalls is incremented only in beforeAgentExecution but decremented unconditionally in afterAgentExecution, and Math.max(0, n - 1) hides the imbalance instead of surfacing it. A call cancelled at the same-session gate still decrements, so with call X in flight (1) and call Y cancelled at the gate, the counter reaches 0 while X is genuinely running; the next beforeAgentExecution then runs warnIfToolkitGroupsChangedOutsideCall() while X's in-call meta-tool / skill activation is still on the toolkit flags, producing a false "changed outside of a call" warning and consuming the one-shot warnedToolkitGroupsChangedOutsideCall flag — after which a real instance of the misuse goes unlogged for the lifetime of the agent. Since this is diagnostics-only, a per-call boolean carried on the CallExecution (or read back from the Reactor context in the cleanup hook) is a safer gate than a global counter.
| @DisplayName( | ||
| "in-call toolkit activation via the meta tool is synced to the session and not" | ||
| + " flagged as misuse") | ||
| void inCallToolkitActivationIsNotFlagged() throws Exception { |
There was a problem hiding this comment.
Good coverage of the sequential paths (target-session-only visibility, replacement semantics across a restart, RuntimeContext / default-session overloads, unknown-group rejection without touching state, latest-persisted reload, in-call activation not flagged). The two cases the safety of the new API actually rests on are not asserted: (1) an update while a call on the same (userId, sessionId) is in flight, and (2) two sessions called concurrently, where the single per-agent toolkitGroupsAtLastCallBoundary ping-pongs between the two slots' values. Adding those two would turn the current "best-effort diagnostics" claim into something enforced.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…eeping Follow-up to the review on agentscope-ai#3206. - ReActAgent.updateToolGroups / setActiveToolGroups now run under a per-slot mutation lock and throw IllegalStateException when a call on the same (userId, sessionId) is in flight on this instance. Previously the in-flight call's own syncToolkitToState + stale-version save reverted the update under the default OVERWRITE policy. Admission in beforeAgentExecution takes the same lock, so an update either lands before the call loads its state or sees the call as in flight. - Both setters return early (no store write) when the resolved list equals the current one, and honour ToolkitConfig.allowToolDeletion like Toolkit.updateToolGroups. Toolkit gains getConfig(). - AgentBase: new releaseCallScope(Object) hook, invoked exactly once via Mono.using for calls that ran beforeAgentExecution. ReActAgent moves the in-flight counter decrement and boundary snapshot there, so a call cancelled while queued at the same-session gate (which only reaches afterAgentExecution) no longer drops the counter and fires a false out-of-call warning. - APPEND_MERGE now carries the writer's activated groups into the merged state, alongside the permission context. - Tests: update rejected while same-session call in flight, concurrent session with in-call activation not flagged, cancelled queued call keeps tracking balanced, idempotent setters skip the store, allowToolDeletion(false) honoured, APPEND_MERGE conflict keeps the activation. Docs updated accordingly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks for the reviews — addressed in de81492. Summary of what I verified and changed, per finding: In-flight update silently reverted (oss-maintainer Unbalanced Setters persist when nothing changed (oss-maintainer ℹ️
Shared toolkit flags leak across concurrent sessions (Copilot high, Tests (oss-maintainer ℹ️ |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review of the new commit de81492d (previous round was on b032f330). Thanks for the thorough response — all three findings from the last round are genuinely fixed, and the fix went further than asked:
- In-flight update silently reverted → resolved. Admission and the out-of-call setters now share
slotMutationLocks, the update is rejected withIllegalStateExceptionwhile the same session has a call in flight, and thetoolGroupUpdateRejectedWhileSameSessionCallInFlighttest asserts the persisted session is left untouched and that a later update lands. - Unbalanced
inFlightCallson a gate-cancelled call → resolved via the newAgentBase.releaseCallScopehook, withcancelledQueuedCallDoesNotUnbalanceInFlightTrackingcovering the exact path. - Needless store writes → resolved by the
updated.equals(current)no-op short-circuit, plusallowToolDeletion(false)is now honoured andAPPEND_MERGEcarries the activation list.toolGroupSettersAreIdempotentandtoolGroupDeactivationHonoursAllowToolDeletioncover both.
What is left is about the shape of the new synchronisation, not the correctness of the fix: the big one is that blocking store I/O now happens inside a 64-stripe monitor that used to be off the hot path, so distinct sessions can now wait on each other. The rest is a rollback gap after the in-flight registration and a per-subscription/idempotency question around the new Mono.using. None of it looks like it breaks the behaviour the PR is about, so I am leaving these as inline comments rather than change requests — happy to see this land once a maintainer has weighed in on the first one.
Two non-blocking notes: Codecov reports 80.95% patch coverage with 20 lines uncovered — likely the IllegalStateException / allowToolDeletion branches under other overloads is worth a look; and CI is green (build on ubuntu + windows, validate, CLA signed).
Automated review by github-manager-bot
| CallExecution scope; | ||
| // Admission (registering the call as in flight + loading its state) is atomic with respect | ||
| // to out-of-call mutators on the same slot, see slotMutationLocks. | ||
| synchronized (slotMutationLock(slot)) { |
There was a problem hiding this comment.
Admission now runs activateSlotForContext — which always reads the session's AgentState from the configured AgentStateStore — inside the new striped monitor. Because the stripe is slot.hashCode() % 64, an unrelated session that hashes to the same stripe is blocked for the full duration of this blocking store read, so a slow file/Redis/JDBC store turns into head-of-line blocking between sessions on the hottest path in the agent. Before this PR, distinct sessions never waited on each other (serializeOnKey only serialised same-slot calls). Could the critical section be narrowed to the in-flight registration only (register under the lock, do the store load outside it, re-check after loading), or the stripe count raised / made per-slot with clean-up?
| inFlightCalls.decrementAndGet(); | ||
| throw e; | ||
| } | ||
| inFlightBySlot.put(scope.slotKey, scope); |
There was a problem hiding this comment.
The rollback catch only covers activateSlotForContext; everything after the put (ctx.setAgentState, bindRuntimeContextToHooks, state.interruptControl().reset()) can still throw while the slot is already registered and inFlightCalls already incremented. Such a throw never reaches releaseCallScope (the scope is never returned to Mono.using), so the slot stays in inFlightBySlot forever: every later updateToolGroups / setActiveToolGroups on that session then fails with a misleading IllegalStateException, and inFlightCalls never returns to 0 so the one-shot out-of-call diagnostic stops firing. Either move the put to the end of admission or wrap the remainder in the same try { … } catch (RuntimeException) { rollback; throw; }.
| // Release the scope exactly once on any terminal signal of the admitted call (complete, | ||
| // error or cancel), before the signal propagates to the caller. Calls cancelled while still | ||
| // waiting on the serialization gate never reach this point, so they never see this hook. | ||
| return Mono.using(() -> scope, s -> body, this::releaseCallScope, true) |
There was a problem hiding this comment.
Mono.using invokes its resource supplier per subscription, but here the supplier is the constant () -> scope for a scope that was built exactly once, in beforeAgentExecution. The cleanup is therefore not tied to the acquisition: any extra subscription of the returned Mono (an operator that re-subscribes upstream, .cache(), a retry added later) calls releaseCallScope(scope) again and decrements inFlightCalls a second time. Since the Math.max(0, …) floor was deliberately removed, the counter then goes negative and getAndIncrement() == 0 never becomes true again, permanently disarming the drift diagnostic. Would it be safer to make the release idempotent for the scope — e.g. gate it on inFlightBySlot.remove(ce.slotKey, ce) returning true, or set a one-shot flag on CallExecution?
| */ | ||
| public List<String> getActiveToolGroups(String userId, String sessionId) { | ||
| String sid = (sessionId == null || sessionId.isBlank()) ? defaultSessionId : sessionId; | ||
| return getAgentState(userId, sid).getToolContext().getActivatedGroups(); |
There was a problem hiding this comment.
This getter goes through getAgentState, unlike the setters which go through loadLatestAgentState. Two consequences worth a look: (1) with a store configured it can return a list that this node cached before another node persisted a change, while the next call() on that session activates the newer list — so getActiveToolGroups can disagree with what the model is about to see, which is the exact class of surprise this API is meant to remove; (2) as a side effect it computeIfAbsents a fresh AgentState (and a slotVersions entry) into the cache for any userId/sessionId a caller probes, so a read-only lookup can grow the per-session cache and later make saveAgentState persist an otherwise-non-existent session. Consider reloading from the store when one is configured (mirroring the setters), or stating in the Javadoc that the value is this instance's cached view.
| String userId, String sessionId, List<String> groupNames, boolean active) { | ||
| Objects.requireNonNull(groupNames, "groupNames must not be null"); | ||
| validateToolGroupsExist(groupNames); | ||
| if (!active && !toolkit.getConfig().isAllowToolDeletion()) { |
There was a problem hiding this comment.
This guard returns before the slot lock, so a deactivation attempted while the session has a call in flight is silently dropped with a log line rather than throwing IllegalStateException. The method Javadoc (@throws IllegalStateException if a call on this session is currently in flight) and the new wording in docs/v2/{en,zh}/docs/building-blocks/tool.md both promise the throw, so with allowToolDeletion(false) the documented contract and the behaviour diverge. Moving this check into the applyToolGroupsToSession mutation lambda (after the in-flight rejection), or carving the case out in the docs/Javadoc, would keep them consistent.
| // owned by this writer (in-call meta-tool / skill activation, or the per-session | ||
| // updateToolGroups / setActiveToolGroups API); carry it over so the merge does not | ||
| // silently drop it. | ||
| baseline.getToolContext() |
There was a problem hiding this comment.
Under APPEND_MERGE the conversation of both writers is kept, but the activated-group list becomes strictly last-writer-wins: a concurrent updateToolGroups from another node is dropped even though its messages survive. That is a reasonable trade-off (the list is a whole-value setting), yet it is a new silent-loss path for exactly the API that exists to make activation explicit. Worth one line in the updateToolGroups Javadoc next to the existing "calls in flight on other instances cannot be detected" note, so operators on a distributed harness are not surprised.
|
This PR currently conflicts with git fetch origin
git checkout fix/core-per-session-tool-groups-3167
git rebase origin/main
# resolve conflicts, then:
git push --force-with-leaseNo further code changes are needed for the review itself — the last review on head Automated notification by github-manager-bot |
AgentScope-Java Version
2.0.3-SNAPSHOT (main @
70686e4a)Description
Fixes #3167
Problem. Tool group activation is resolved from each
(userId, sessionId)slot'sAgentState.toolContext.activatedGroups, while the toolkit's own activation flags are per-call scratch state thatactivateSlotForContextre-seeds from thatAgentStateat every call entry. Callingagent.getToolkit().updateToolGroups(...)between calls therefore looks successful (toolkit.getActiveGroups()reports the change) but is silently discarded at the next call and never reaches the model — and there was no supported API to change a session's active groups from application code.This implements options 1 + 2 from the issue evaluation (new convenience API + warning + docs). Option 3 (making the shared toolkit flags authoritative again) was deliberately not taken, as it would regress per-session isolation for concurrent calls.
Changes
ReActAgent: newgetActiveToolGroups/updateToolGroups(groups, active)/setActiveToolGroups(groups), keyed by(userId, sessionId)orRuntimeContext. They validate group names against the toolkit (IllegalArgumentExceptionon unknown names), reload the latest persisted state first when anAgentStateStoreis configured, mutate the session'sAgentStateand persist it — the same get → mutate → save pattern already used bysetPermissionMode/replacePermissionContext. Other sessions are unaffected. A request that leaves the list unchanged is a no-op (no store write); deactivation honoursToolkitConfig.allowToolDeletion(false)likeToolkit.updateToolGroups.ReActAgent: an update while a call on the same session is in flight on this instance is rejected withIllegalStateException(the in-flight call's ownsyncToolkitToState+ stale-version save would otherwise silently revert it underOVERWRITE). Call admission inbeforeAgentExecutionand the setters share a striped per-slot lock, so an update either lands before the call loads its state or observes the call as in flight — no window in between.AgentBase: newreleaseCallScope(Object callScope)hook, invoked exactly once (viaMono.using) for calls that actually ranbeforeAgentExecution.ReActAgentkeeps its in-flight bookkeeping there instead of inafterAgentExecution(), which also runs for calls cancelled while queued at the same-session gate and previously unbalanced the counter (false out-of-call warning + consumed one-shot flag).ReActAgent: logs a one-shot warning when the toolkit's active groups are found changed outside of any call (compared against a snapshot taken at call boundaries, gated by the in-flight counter so concurrent sessions and in-call meta-tool / skill activations are not flagged). Diagnostics only; no behavioural effect.ReActAgent:APPEND_MERGEnow carries the writer's activated groups into the merged state (alongside the permission context) so a conflicting per-session update is not dropped.Toolkit:getConfig()accessor.HarnessAgent: delegates the new methods;getToolkit()Javadoc (core + harness) now states that mutating the returned toolkit between calls has no effect and points to the per-session API.docs/v2/{en,zh}/docs/building-blocks/tool.md): new "Activating groups programmatically (per session)" section with a warning about the old pattern and the in-flight / no-op /allowToolDeletionsemantics.No existing public signatures changed; everything is additive.
Not in scope (pre-existing, will file separately): concurrent calls on distinct sessions share the toolkit's activation flags; a session entering while another has an in-call
reset_equipped_tools/ skill activation resets the flags, and the earlier session's finalsyncToolkitToStatethen persists the reset list. This exists onmainindependently of this PR and needs the meta tool / skills to target the call's state rather than the shared toolkit.How to test
mvn -pl agentscope-core test -Dtest=ReActAgentPerSessionStateTest— 13 new tests: activation visible on the next call of the target session only, replacement semantics persisted across an engine restart over the same store,RuntimeContext/ default-session overloads, unknown-group rejection without touching state, update applied on top of the latest persisted state (not a stale cache), out-of-call toolkit mutation discarded and flagged, in-callreset_equipped_toolsactivation synced to the session and not flagged, update rejected while the same session's call is in flight (other session still updatable; persisted state untouched), concurrent session entering during another's in-call activation not flagged (one-shot diagnostic still armed afterwards), same-session call cancelled at the gate keeps tracking balanced, idempotent setters skip the store,allowToolDeletion(false)honoured,APPEND_MERGEconflict keeps the requested activation and the competing writer's context.mvn -pl agentscope-harness test -Dtest=HarnessAgentTest— new delegation test (target session activated, other session untouched, stored onAgentState).agentscope-coresuite: 2377 tests, 0 failures;HarnessAgentTest: 52/52;mvn spotless:checkclean.Checklist
mvn spotless:applymvn test)🤖 Generated with Claude Code