From 7275f8bddc1c171e43dde282cdb44a49a45e42c6 Mon Sep 17 00:00:00 2001 From: beatwade Date: Sun, 14 Jun 2026 23:11:53 +0800 Subject: [PATCH 1/2] fix(agentscope-admin): engage per-session permission mode so live write_file is gated (D-08) D-08 follow-up: the prior commit (admin-service 34d25c5d) seeded a non-trivial permission context on the agent builder (b.permissionContext(...)) and its unit test passed, but live write_file still ran unchecked. Root cause, confirmed by reading the load path: ReActAgent.loadOrCreateAgentStateForSlot returns a persisted AgentState verbatim, and only falls back to freshState(initialCtx) on a store MISS. So the builder's initialPermissionContext only reached FRESH sessions; any session persisted before the builder seeded a non-trivial context kept its trivial context (DEFAULT, no rules) forever -> isTrivial() true -> evaluatePermissions took the lightweight path -> write_file's passthrough self-check ALLOWed it -> RequireUserConfirmEvent never fired. Fix: in loadOrCreateAgentStateForSlot, merge the builder's initial permission context onto the loaded state when (and only when) the loaded state's context is still trivial. This upgrades stale persisted sessions in place while preserving their conversation/tool state. A session whose mode was deliberately toggled via setPermissionMode is left untouched (non-trivial by mode change). Acceptance test (ReActAgentStalePermissionContextTest) drives the real ReActAgent + real AgentStateStore against a pre-seeded trivial persisted session and a passthrough write_file tool (mirroring the harness FilesystemTool), asserting PERMISSION_ASKING + RequireUserConfirmEvent. Red without the merge, green with. Co-Authored-By: Claude Opus 4.8 --- .../java/io/agentscope/core/ReActAgent.java | 78 ++++- .../ReActAgentStalePermissionContextTest.java | 277 ++++++++++++++++++ 2 files changed, 339 insertions(+), 16 deletions(-) create mode 100644 agentscope-core/src/test/java/io/agentscope/core/agent/ReActAgentStalePermissionContextTest.java diff --git a/agentscope-core/src/main/java/io/agentscope/core/ReActAgent.java b/agentscope-core/src/main/java/io/agentscope/core/ReActAgent.java index 0fd76fed6..c4b452b6a 100644 --- a/agentscope-core/src/main/java/io/agentscope/core/ReActAgent.java +++ b/agentscope-core/src/main/java/io/agentscope/core/ReActAgent.java @@ -365,22 +365,24 @@ private static AgentState loadOrCreateAgentStateForSlot( return fresh; } try { - return stateStore - .get(userId, sessionId, "agent_state", AgentState.class) - .orElseGet( - () -> { - AgentState legacy = - LegacyStateLoader.loadFromLegacySession( - stateStore, userId, sessionId); - if (legacy != null - && (!legacy.getContext().isEmpty() - || !legacy.getToolContext() - .getActivatedGroups() - .isEmpty())) { - return legacy; - } - return fresh; - }); + AgentState loaded = + stateStore + .get(userId, sessionId, "agent_state", AgentState.class) + .orElseGet( + () -> { + AgentState legacy = + LegacyStateLoader.loadFromLegacySession( + stateStore, userId, sessionId); + if (legacy != null + && (!legacy.getContext().isEmpty() + || !legacy.getToolContext() + .getActivatedGroups() + .isEmpty())) { + return legacy; + } + return fresh; + }); + return mergeInitialPermissionContext(loaded, permCtx); } catch (Exception e) { log.warn( "Failed to load AgentState for slot (userId={}, sessionId={}): {}", @@ -404,6 +406,50 @@ private static AgentState freshState( return asb.build(); } + /** + * Repair a stale persisted session whose {@link PermissionContextState} is {@linkplain + * PermissionContextState#isTrivial() trivial} by applying the builder's initial (non-trivial) + * {@code permCtx} on top of it. + * + *

Background (D-08): a non-trivial permission context is required for the full {@link + * PermissionEngine} to engage; otherwise {@code evaluatePermissions} falls back to the + * lightweight path that only honours each tool's own {@code checkPermissions} self-check — + * which returns {@code passthrough} for file/shell tools, so {@code write_file} runs unchecked + * and {@link io.agentscope.core.event.RequireUserConfirmEvent} never fires. Sessions persisted + * before the builder seeded a non-trivial context carry a trivial one forever, since the load + * path returns the persisted state verbatim. Merging the initial context here upgrades those + * stale sessions in place, preserving their conversation/tool state while restoring HITL + * gating. This only fires for trivial persisted contexts (mode DEFAULT, no rules), so a session + * whose mode was deliberately changed via {@link #setPermissionMode} — which makes the context + * non-trivial by flipping the mode away from DEFAULT — is left untouched. + * + * @param loaded the state loaded from the store (or legacy fallback) + * @param initialPermCtx the builder's initial permission context (may be {@code null}) + * @return {@code loaded} unchanged when it already has a non-trivial context or no initial + * context was supplied; otherwise a copy of {@code loaded} with the initial context applied + */ + private static AgentState mergeInitialPermissionContext( + AgentState loaded, PermissionContextState initialPermCtx) { + if (loaded == null + || initialPermCtx == null + || !loaded.getPermissionContext().isTrivial()) { + return loaded; + } + return AgentState.builder() + .sessionId(loaded.getSessionId()) + .userId(loaded.getUserId()) + .summary(loaded.getSummary()) + .context(loaded.getContext()) + .replyId(loaded.getReplyId()) + .curIter(loaded.getCurIter()) + .shutdownInterrupted(loaded.isShutdownInterrupted()) + .permissionContext(initialPermCtx) + .toolContext(loaded.getToolContext()) + .tasksContext(loaded.getTasksContext()) + .planModeContext(loaded.getPlanModeContext()) + .build(); + } + /** * Persist the current {@link AgentState} via the configured {@link AgentStateStore}, or {@code * Mono.empty()} when no AgentStateStore was provided. Synchronises toolkit activeGroups into the state diff --git a/agentscope-core/src/test/java/io/agentscope/core/agent/ReActAgentStalePermissionContextTest.java b/agentscope-core/src/test/java/io/agentscope/core/agent/ReActAgentStalePermissionContextTest.java new file mode 100644 index 000000000..0bc68e585 --- /dev/null +++ b/agentscope-core/src/test/java/io/agentscope/core/agent/ReActAgentStalePermissionContextTest.java @@ -0,0 +1,277 @@ +/* + * Copyright 2024-2026 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.agentscope.core.agent; + +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import io.agentscope.core.ReActAgent; +import io.agentscope.core.event.AgentEvent; +import io.agentscope.core.event.RequestStopEvent; +import io.agentscope.core.event.RequireUserConfirmEvent; +import io.agentscope.core.message.ContentBlock; +import io.agentscope.core.message.GenerateReason; +import io.agentscope.core.message.Msg; +import io.agentscope.core.message.TextBlock; +import io.agentscope.core.message.ToolResultBlock; +import io.agentscope.core.message.ToolUseBlock; +import io.agentscope.core.model.ChatModelBase; +import io.agentscope.core.model.ChatResponse; +import io.agentscope.core.model.GenerateOptions; +import io.agentscope.core.model.ToolSchema; +import io.agentscope.core.permission.PermissionBehavior; +import io.agentscope.core.permission.PermissionContextState; +import io.agentscope.core.permission.PermissionDecision; +import io.agentscope.core.permission.PermissionMode; +import io.agentscope.core.permission.PermissionRule; +import io.agentscope.core.state.AgentState; +import io.agentscope.core.state.InMemoryAgentStateStore; +import io.agentscope.core.tool.ToolBase; +import io.agentscope.core.tool.ToolCallParam; +import io.agentscope.core.tool.Toolkit; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.Supplier; +import org.junit.jupiter.api.Test; +import reactor.core.publisher.Flux; +import reactor.core.publisher.Mono; + +/** + * Acceptance test for defect D-08 live gating gap: a {@link ReActAgent} built with a non-trivial + * initial {@link PermissionContextState} must still gate a passthrough tool ({@code write_file}-like) + * on a stale persisted session whose stored permission context is trivial. + * + *

The companion unit test {@code PermissionContextFactoryTest} (admin-service) only drives the + * engine in isolation and so passes regardless of whether the live agent actually loads the + * non-trivial context for a stale session. This test closes that gap by exercising the real + * {@link ReActAgent} slot-activation + permission-engine load path against a pre-seeded trivial + * session state. + */ +class ReActAgentStalePermissionContextTest { + + /** + * Real-world file-write tool stand-in: {@code checkPermissions} returns {@code passthrough} + * (exactly like the harness {@code FilesystemTool} for non-readOnly tools), so under a trivial + * context the lightweight path allows it unchecked; under a non-trivial context with no allow + * rule it falls through to {@code defaultDecisionAsk} → ASK → {@link RequireUserConfirmEvent}. + */ + private static final class PassthroughWriteTool extends ToolBase { + PassthroughWriteTool() { + super( + "write_file", + "write a file", + schemaFor(), + false, // not readOnly + true, + false, + null, + false, + false); + } + + private static Map schemaFor() { + Map schema = new HashMap<>(); + schema.put("type", "object"); + Map props = new HashMap<>(); + Map path = new HashMap<>(); + path.put("type", "string"); + props.put("path", path); + Map content = new HashMap<>(); + content.put("type", "string"); + props.put("content", content); + schema.put("properties", props); + return schema; + } + + @Override + public Mono checkPermissions( + Map toolInput, PermissionContextState context) { + return Mono.just(PermissionDecision.passthrough("passthrough: " + getName())); + } + + @Override + public Mono callAsync(ToolCallParam param) { + return Mono.just(ToolResultBlock.text("written")); + } + } + + /** + * A non-trivial DEFAULT context equivalent to what {@code PermissionContextFactory.build(DEFAULT)} + * produces on the admin-service side: read-only built-ins ALLOWed (which is what makes the + * context non-trivial), write_file has no allow rule so the engine's {@code defaultDecisionAsk} + * fires. + */ + private static PermissionContextState defaultContext() { + PermissionContextState.Builder b = + PermissionContextState.builder().mode(PermissionMode.DEFAULT); + b.addAllowRule( + "read_file", + new PermissionRule("read_file", null, PermissionBehavior.ALLOW, "test")); + return b.build(); + } + + private static final class ScriptedModel extends ChatModelBase { + private final List>> scripts; + private final AtomicInteger idx = new AtomicInteger(0); + + ScriptedModel(List>> scripts) { + this.scripts = scripts; + } + + @Override + public String getModelName() { + return "scripted"; + } + + @Override + protected Flux doStream( + List messages, List tools, GenerateOptions options) { + int i = idx.getAndIncrement(); + if (i >= scripts.size()) { + return Flux.just(textResponse("")); + } + return scripts.get(i).get(); + } + } + + private static ChatResponse textResponse(String text) { + return ChatResponse.builder() + .content(List.of(TextBlock.builder().text(text).build())) + .build(); + } + + private static ChatResponse writeToolResponse(String toolId) { + Map input = new HashMap<>(); + input.put("path", "stale.txt"); + input.put("content", "hi"); + return ChatResponse.builder() + .content( + List.of( + ToolUseBlock.builder() + .id(toolId) + .name("write_file") + .input(input) + .build())) + .build(); + } + + private static Toolkit toolkitWith(ToolBase... tools) { + Toolkit tk = new Toolkit(); + for (ToolBase t : tools) { + tk.registerAgentTool(t); + } + return tk; + } + + private static int indexOf(List events, Class type) { + for (int i = 0; i < events.size(); i++) { + if (type.isInstance(events.get(i))) { + return i; + } + } + return -1; + } + + @Test + void staleTrivialSession_isUpgradedToInitialContext_andGatesWriteFile() { + InMemoryAgentStateStore store = new InMemoryAgentStateStore(); + // Pre-seed a STALE session with a TRIVIAL permission context — this is the live failure + // scenario: a session persisted before the agent seeded a non-trivial context. + String userId = "u1"; + String sessionId = "stale-session"; + AgentState stale = + AgentState.builder().sessionId(sessionId).userId(userId).build(); // trivial ctx + store.save(userId, sessionId, "agent_state", stale); + + ReActAgent agent = + ReActAgent.builder() + .name("asst") + .defaultSessionId(sessionId) + .stateStore(store) + .permissionContext(defaultContext()) + .toolkit(toolkitWith(new PassthroughWriteTool())) + .model( + new ScriptedModel( + List.of(() -> Flux.just(writeToolResponse("tc1"))))) + .build(); + + Msg result = + agent.call( + List.of(), + RuntimeContext.builder() + .userId(userId) + .sessionId(sessionId) + .build()) + .block(); + assertNotNull(result); + // Without the merge fix the result would be a normal completion (write_file ran). With the + // fix the agent pauses with PERMISSION_ASKING. + assertTrue( + result.getGenerateReason() == GenerateReason.PERMISSION_ASKING, + "expected PERMISSION_ASKING after stale-session upgrade, got " + + result.getGenerateReason()); + } + + @Test + void staleTrivialSession_emitsRequireUserConfirmEvent() { + InMemoryAgentStateStore store = new InMemoryAgentStateStore(); + String userId = "u1"; + String sessionId = "stale-session-2"; + store.save( + userId, + sessionId, + "agent_state", + AgentState.builder().sessionId(sessionId).userId(userId).build()); + + ReActAgent agent = + ReActAgent.builder() + .name("asst") + .defaultSessionId(sessionId) + .stateStore(store) + .permissionContext(defaultContext()) + .toolkit(toolkitWith(new PassthroughWriteTool())) + .model( + new ScriptedModel( + List.of(() -> Flux.just(writeToolResponse("tc1"))))) + .build(); + + List events = + agent.streamEvents( + List.of(), + RuntimeContext.builder() + .userId(userId) + .sessionId(sessionId) + .build()) + .collectList() + .block(); + assertNotNull(events); + + int iReq = indexOf(events, RequireUserConfirmEvent.class); + int iStop = indexOf(events, RequestStopEvent.class); + assertTrue(iReq >= 0, "RequireUserConfirmEvent must be emitted on stale session"); + assertTrue(iStop > iReq, "RequestStopEvent must follow RequireUserConfirmEvent"); + RequireUserConfirmEvent req = (RequireUserConfirmEvent) events.get(iReq); + assertTrue( + req.getToolCalls().stream().anyMatch(tc -> "write_file".equals(tc.getName())), + "RequireUserConfirmEvent must name write_file"); + RequestStopEvent stop = (RequestStopEvent) events.get(iStop); + assertTrue( + stop.getGenerateReason() == GenerateReason.PERMISSION_ASKING, + "RequestStopEvent must carry PERMISSION_ASKING"); + } +} From 104b4b3be6d277bbe4ad77145b7b699ef4a0aa64 Mon Sep 17 00:00:00 2001 From: beatwade Date: Tue, 16 Jun 2026 19:13:11 +0800 Subject: [PATCH 2/2] fix(agui): give reasoning messages a distinct messageId MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AguiAgentAdapter used the same msg.getId() for both the TEXT_MESSAGE_* (answer) and REASONING_MESSAGE_* (thinking) derived from a single agent message. In AG-UI these are independent messages (roles 'assistant' and 'reasoning') associated only by runId and order, never by messageId. Sharing one messageId makes clients group them into a single message bubble — e.g. the answer gets folded into the reasoning/thought panel instead of rendering as a standalone, always-visible message. Use msg.getId() + "-reasoning" for the reasoning message so it renders independently and stays collapsible. A suffix (rather than a fresh UUID) keeps multiple ThinkingBlocks in one message deduped into a single reasoning message, mirroring the text answer. Added a regression test asserting distinct messageIds. --- .../core/agui/adapter/AguiAgentAdapter.java | 10 +++- .../agui/adapter/AguiAgentAdapterTest.java | 55 ++++++++++++++++++- 2 files changed, 63 insertions(+), 2 deletions(-) diff --git a/agentscope-extensions/agentscope-extensions-protocol/agentscope-extensions-agui/src/main/java/io/agentscope/core/agui/adapter/AguiAgentAdapter.java b/agentscope-extensions/agentscope-extensions-protocol/agentscope-extensions-agui/src/main/java/io/agentscope/core/agui/adapter/AguiAgentAdapter.java index 2314e72b9..38bbe2ac9 100644 --- a/agentscope-extensions/agentscope-extensions-protocol/agentscope-extensions-agui/src/main/java/io/agentscope/core/agui/adapter/AguiAgentAdapter.java +++ b/agentscope-extensions/agentscope-extensions-protocol/agentscope-extensions-agui/src/main/java/io/agentscope/core/agui/adapter/AguiAgentAdapter.java @@ -183,7 +183,15 @@ private List convertEvent(Event event, EventConversionState state) { if (config.isEnableReasoning()) { String thinking = thinkingBlock.getThinking(); if (thinking != null && !thinking.isEmpty()) { - String messageId = msg.getId(); + // Reasoning is a distinct AG-UI message (role "reasoning") from the + // text answer (role "assistant"); they are associated only by runId + // and order, never by messageId. Reusing msg.getId() for both makes + // clients group them into a single message bubble (e.g. the answer + // gets folded into the reasoning/thought panel). Use a distinct id so + // the reasoning message renders independently and stays collapsible. + // Suffix (not a fresh UUID) so multiple ThinkingBlocks in one Msg + // dedupe into a single reasoning message, mirroring the text message. + String messageId = msg.getId() + "-reasoning"; // Start reasoning message if not started if (!state.hasStartedReasoningMessage(messageId)) { diff --git a/agentscope-extensions/agentscope-extensions-protocol/agentscope-extensions-agui/src/test/java/io/agentscope/core/agui/adapter/AguiAgentAdapterTest.java b/agentscope-extensions/agentscope-extensions-protocol/agentscope-extensions-agui/src/test/java/io/agentscope/core/agui/adapter/AguiAgentAdapterTest.java index e1ac649a6..24af36627 100644 --- a/agentscope-extensions/agentscope-extensions-protocol/agentscope-extensions-agui/src/test/java/io/agentscope/core/agui/adapter/AguiAgentAdapterTest.java +++ b/agentscope-extensions/agentscope-extensions-protocol/agentscope-extensions-agui/src/test/java/io/agentscope/core/agui/adapter/AguiAgentAdapterTest.java @@ -17,6 +17,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; @@ -1017,7 +1018,7 @@ void testRunWithThinkingBlockEvent() { .orElse(null); assertNotNull(reasoningMessageStart, "Should have ReasoningMessageStart"); - assertEquals("msg-r1", reasoningMessageStart.messageId()); + assertEquals("msg-r1-reasoning", reasoningMessageStart.messageId()); assertEquals("reasoning", reasoningMessageStart.role()); AguiEvent.ReasoningMessageContent reasoningMessageContent = @@ -1033,6 +1034,58 @@ void testRunWithThinkingBlockEvent() { "Should contain thinking content"); } + @Test + void testReasoningAndTextAnswerHaveDistinctMessageIds() { + // Regression: a single assistant Msg carrying both a ThinkingBlock and a TextBlock must + // emit REASONING_MESSAGE_* and TEXT_MESSAGE_* with DIFFERENT messageIds. They are + // independent AG-UI messages (associated only by runId + order). Reusing one messageId + // makes clients collapse reasoning and answer into a single message bubble, folding the + // answer into the reasoning/thought panel. + AguiAdapterConfig config = AguiAdapterConfig.builder().enableReasoning(true).build(); + AguiAgentAdapter adapterWithReasoning = new AguiAgentAdapter(mockAgent, config); + + Msg msg = + Msg.builder() + .id("msg-both") + .role(MsgRole.ASSISTANT) + .content( + ThinkingBlock.builder().thinking("reasoning here").build(), + TextBlock.builder().text("the answer").build()) + .build(); + + Event event = new Event(EventType.REASONING, msg, true); + when(mockAgent.stream(anyList(), any(StreamOptions.class))).thenReturn(Flux.just(event)); + + RunAgentInput input = + RunAgentInput.builder() + .threadId("thread-1") + .runId("run-1") + .messages(List.of(AguiMessage.userMessage("msg-1", "Hi"))) + .build(); + + List events = adapterWithReasoning.run(input).collectList().block(); + assertNotNull(events); + + String reasoningId = + events.stream() + .filter(e -> e instanceof AguiEvent.ReasoningMessageStart) + .map(e -> ((AguiEvent.ReasoningMessageStart) e).messageId()) + .findFirst() + .orElse(null); + String textId = + events.stream() + .filter(e -> e instanceof AguiEvent.TextMessageStart) + .map(e -> ((AguiEvent.TextMessageStart) e).messageId()) + .findFirst() + .orElse(null); + + assertNotNull(reasoningId, "Should emit ReasoningMessageStart"); + assertNotNull(textId, "Should emit TextMessageStart"); + assertNotEquals(reasoningId, textId, "reasoning and text must have distinct messageIds"); + assertEquals("msg-both-reasoning", reasoningId); + assertEquals("msg-both", textId); + } + @Test void testRunWithStreamingThinkingBlockEvents() { // Test streaming reasoning events when enabled