Fix(harness): session state update and tool call result notifications.#1703
Conversation
…t streaming capabilities
…ltEvictionMiddleware timing ToolCall/ToolResult delta and end events now carry toolCallName so consumers don't need to cache the start event's name mapping. ToolResultEvictionMiddleware moved from onActing (where state hadn't been written yet, making it a no-op) to onReasoning where tool results are already in state.
…n activateSlotForContext
…path normalization
There was a problem hiding this comment.
Pull request overview
This PR strengthens the harness + core agent runtime by unifying ReActAgent invocation and lifecycle event streaming, improving distributed session-state freshness, and enriching event payloads (tool name, result notification, metadata). It also tightens filesystem path behavior so skills/tools can work across different filesystem modes without hard-coded absolute paths.
Changes:
- Refactors
ReActAgentto use a sharedbuildAgentStreamcore for bothcall()andstreamEvents(), addingAgentResultEventplus new extensible event types (HINT_BLOCK,CUSTOM) and optional per-event metadata. - Improves harness multi-tenant behavior: isolation-scope-aware throttling for memory flush/maintenance, remote filesystem routing for
plans/, and workspace path normalization wired into filesystem tools. - Documentation/test updates to match the new path/event semantics (relative path guidance, updated tool-result events, updated glob path expectations).
PR title note: the title uses Fix(harness): ... (capitalized type). Conventional commits typically use lowercase fix(harness): ....
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/v2/zh/docs/harness/skill.md | Adds guidance to use only relative paths in skills/scripts to stay compatible across filesystem modes. |
| docs/v2/en/docs/harness/skill.md | English counterpart of the relative-path guidance for skills/scripts. |
| agentscope-harness/src/test/java/io/agentscope/harness/agent/middleware/MemoryFlushMiddlewareTriggerTest.java | Updates/extends tests for isolation-scope-aware flush throttling behavior. |
| agentscope-harness/src/test/java/io/agentscope/harness/agent/filesystem/FilesystemGlobTest.java | Adjusts local glob test expectations to match new path shapes returned by filesystem entries. |
| agentscope-harness/src/main/java/io/agentscope/harness/agent/workspace/WorkspacePathNormalizer.java | Introduces a utility to normalize absolute paths into workspace-relative paths. |
| agentscope-harness/src/main/java/io/agentscope/harness/agent/workspace/plan/PlanModeManager.java | Clarifies PLAN.md path semantics across isolation scopes. |
| agentscope-harness/src/main/java/io/agentscope/harness/agent/tool/FilesystemTool.java | Injects optional path normalization so tools accept absolute “workspace root” paths safely. |
| agentscope-harness/src/main/java/io/agentscope/harness/agent/middleware/ToolResultEvictionMiddleware.java | Moves eviction hook to onReasoning to evict oversized tool results before they hit downstream reasoning. |
| agentscope-harness/src/main/java/io/agentscope/harness/agent/middleware/PlanModeMiddleware.java | Enriches denied tool-result events with tool name. |
| agentscope-harness/src/main/java/io/agentscope/harness/agent/middleware/MemoryMaintenanceMiddleware.java | Adds isolation-scope keyed throttling for maintenance runs. |
| agentscope-harness/src/main/java/io/agentscope/harness/agent/middleware/MemoryFlushMiddleware.java | Adds isolation-scope keyed throttling for flush runs. |
| agentscope-harness/src/main/java/io/agentscope/harness/agent/middleware/HarnessSkillMiddleware.java | Ensures skill tool registration targets the agent’s live toolkit instance. |
| agentscope-harness/src/main/java/io/agentscope/harness/agent/HarnessAgent.java | Wires isolation scope into memory middlewares; injects filesystem/workspace/path normalizer into runtime context; registers filesystem tool with a normalizer. |
| agentscope-harness/src/main/java/io/agentscope/harness/agent/filesystem/spec/RemoteFilesystemSpec.java | Adds plans/ routing and exposes isolation scope via getter. |
| agentscope-harness/src/main/java/io/agentscope/harness/agent/filesystem/local/LocalFilesystem.java | Normalizes entry paths via a shared resolver and returns cwd-relative paths for non-namespaced local mode. |
| agentscope-examples/documentation/src/main/java/io/agentscope/examples/documentation2/quickstart/UserIsolatedMultiTurnsExample.java | Adds a quickstart example for session persistence / user isolation behavior. |
| agentscope-core/src/test/java/io/agentscope/core/tracing/OtelTracingMiddlewareTest.java | Updates tool-result end event constructor usage to include tool name. |
| agentscope-core/src/main/java/io/agentscope/core/ReActAgent.java | Major refactor: shared stream core for call/stream; always reload state from store at call start; tool event enrichment; emit missing tool-result deltas for non-streaming tools. |
| agentscope-core/src/main/java/io/agentscope/core/message/HintBlock.java | Adds source field to hint blocks. |
| agentscope-core/src/main/java/io/agentscope/core/event/ToolResultTextDeltaEvent.java | Adds tool call name to tool-result text delta events. |
| agentscope-core/src/main/java/io/agentscope/core/event/ToolResultEndEvent.java | Adds tool call name to tool-result end events. |
| agentscope-core/src/main/java/io/agentscope/core/event/ToolResultDataDeltaEvent.java | Adds tool call name to tool-result data delta events. |
| agentscope-core/src/main/java/io/agentscope/core/event/ToolCallEndEvent.java | Adds tool call name to tool-call end events. |
| agentscope-core/src/main/java/io/agentscope/core/event/ToolCallDeltaEvent.java | Adds tool call name to tool-call delta events. |
| agentscope-core/src/main/java/io/agentscope/core/event/HintBlockEvent.java | New event type for one-shot hint blocks. |
| agentscope-core/src/main/java/io/agentscope/core/event/CustomEvent.java | New generic extensible “custom” event with name + payload. |
| agentscope-core/src/main/java/io/agentscope/core/event/AgentResultEvent.java | New event type carrying the final Msg result. |
| agentscope-core/src/main/java/io/agentscope/core/event/AgentEventType.java | Adds AGENT_RESULT, HINT_BLOCK, CUSTOM enum entries. |
| agentscope-core/src/main/java/io/agentscope/core/event/AgentEvent.java | Registers new event subtypes and adds optional metadata map support. |
| agentscope-core/src/main/java/io/agentscope/core/agent/AgentBase.java | Introduces callInternal extension point; makes runLifecycle protected so subclasses can wrap consistently. |
| agentscope-core/src/main/java/io/agentscope/core/agent/Agent.java | Adds getToolkit() accessor for middleware that needs the runtime toolkit instance. |
| ctx).block(); | ||
| System.out.println("[turn1] " + turn1.getTextContent()); | ||
|
|
||
| // |
AgentScopeJavaBot
left a comment
AgentScopeJavaBot
left a comment
There was a problem hiding this comment.
🤖 AI Review
This PR introduces significant architectural improvements to unify agent invocation paths and enhance distributed deployment support. The core refactoring consolidates call() and streamEvents() through a shared buildAgentStream implementation, ensuring consistent middleware chain execution. Session state management is strengthened by always reloading from the store when AgentStateStore is configured, addressing stale cache issues in distributed environments. The addition of new event types (AgentResultEvent, CustomEvent, HintBlockEvent) and metadata support on AgentEvent provides richer extensibility for event-driven consumers. Tool events are enriched with tool names, and the harness layer gains isolation scope awareness for memory middleware. The changes are well-structured, but there are a few areas that warrant attention.
| private final Model model; | ||
| private final String flushPrompt; | ||
| private final MemoryConfig.FlushTrigger flushTrigger; | ||
| private final IsolationScope isolationScope; |
There was a problem hiding this comment.
[major] Potential memory leak in long-running services: The lastRunAtByKey ConcurrentHashMap grows unbounded as unique userId/sessionId combinations are encountered. In production services handling thousands of unique sessions over time, this map will never shrink, even after sessions become inactive. Consider implementing an LRU eviction policy, periodic cleanup of stale entries (e.g., entries not accessed within a retention window), or using a bounded cache like Caffeine with expireAfterAccess. Without this, memory usage will grow linearly with unique session count.
| * namespace (see {@link MemoryFlushMiddleware} for the identical pattern). | ||
| */ | ||
| private final ConcurrentHashMap<String, AtomicReference<Instant>> lastRunAtByKey = | ||
| new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
[major] Same unbounded growth issue as MemoryFlushMiddleware: The lastRunAtByKey ConcurrentHashMap has no cleanup mechanism. Since this middleware tracks maintenance windows per isolation key, the map will accumulate entries for every unique user/session/agent seen during the agent's lifetime. For long-running services, this creates a memory leak. Apply the same mitigation strategy as suggested for MemoryFlushMiddleware.
| slot, k -> new PermissionEngine(loaded.getPermissionContext())); | ||
| AgentState loaded; | ||
| if (stateStore != null) { | ||
| loaded = |
There was a problem hiding this comment.
[minor] Code duplication in state loading logic: The activateSlotForContext method now has two nearly identical code paths (stateStore != null vs null) that differ only in whether they use put or computeIfAbsent. This duplication makes the method harder to maintain. Consider extracting a helper method like loadState(slot, shouldReload) that encapsulates the conditional logic, or use a single path with a comment explaining why the put vs computeIfAbsent distinction matters for the distributed case.
| @JsonProperty("value") Map<String, Object> value) { | ||
| super(id, createdAt); | ||
| this.name = name; | ||
| this.value = value != null ? value : Map.of(); |
There was a problem hiding this comment.
[minor] Immutable empty map may surprise callers: When value is null, the constructor assigns Map.of() which returns an immutable empty map. While this is safe, callers who expect to mutate the returned map (e.g., via event.getValue().put(...)) will get an UnsupportedOperationException. Consider documenting this immutability contract in the Javadoc for getValue(), or defensively copy to a mutable map when the input is null.
| private final String createdAt; | ||
| private String source; | ||
|
|
||
| @JsonInclude(JsonInclude.Include.NON_EMPTY) |
There was a problem hiding this comment.
[minor] Metadata field lacks validation: The withMetadata method accepts any Map<String, Object> but the metadata field is typed as Map<String, Object>. If callers store non-serializable objects or complex types in this map, Jackson serialization may fail at runtime. Consider adding a note in the Javadoc that values should be JSON-serializable, or provide a builder/validator that enforces this constraint.
| this.abstractFilesystem = abstractFilesystem; | ||
| this.pathNormalizer = pathNormalizer; | ||
| } | ||
|
|
There was a problem hiding this comment.
[minor] Silent pass-through when pathNormalizer is null: The norm() method returns the path unchanged when pathNormalizer is null. While this preserves backward compatibility, it could lead to subtle bugs if a caller expects normalization but the normalizer wasn't configured. Consider logging a debug message when normalization is skipped, or document this behavior in the FilesystemTool class Javadoc.
| .CONTEXT_KEY, | ||
| (AgentEventEmitter) | ||
| sink::next)) | ||
| .doFinally( |
There was a problem hiding this comment.
[minor] Complex conditional logic in buildAgentStream: The isSubagentBusPath check and the conditional contextWrite for AgentEventEmitter.CONTEXT_KEY is intricate and could be error-prone for future maintainers. The inline comment explains the 'why' but the logic itself is dense. Consider extracting this into a clearly-named helper method like shouldInstallEventEmitterContext(subscriberCtx) that returns a boolean, making the main flow easier to follow.
| public String normalize(String path) { | ||
| if (path == null || path.isBlank()) { | ||
| return path; | ||
| } |
There was a problem hiding this comment.
[nitpick] Missing @nullable annotation on normalize return: The normalize method can return the input path unchanged if no prefix matches, but the Javadoc doesn't explicitly state this. Consider adding a @Nullable annotation if null input is possible, or clarify in the Javadoc that the method never returns null and always returns either a normalized path or the original path.
| * <p>Named {@code hintSource} to avoid collision with {@link AgentEvent#getSource()} which | ||
| * carries the subagent forwarding path. | ||
| */ | ||
| public String getHintSource() { |
There was a problem hiding this comment.
[nitpick] hintSource naming rationale could be clearer: The Javadoc explains that hintSource avoids collision with AgentEvent.getSource(), but the name itself might confuse readers who expect 'source' to mean the event origin. Consider whether hintSender, hintOrigin, or hintAuthor would be more intuitive, or add a concrete example in the Javadoc showing typical values.
| @@ -1905,6 +1923,15 @@ public HarnessAgent build() { | |||
| } | |||
There was a problem hiding this comment.
[nitpick] Isolation scope resolution could be extracted: The logic to determine effectiveIsolationScope by checking remoteFilesystemSpec then sandboxFilesystemSpec is straightforward but repeated. If this pattern appears elsewhere, consider extracting it into a helper method like resolveEffectiveIsolationScope(remoteSpec, sandboxSpec) to improve readability and ensure consistency.
AgentScopeJavaBot
left a comment
AgentScopeJavaBot
left a comment
There was a problem hiding this comment.
🤖 AI Review
This PR introduces significant architectural improvements to unify agent invocation paths and enhance distributed deployment support. The core refactoring consolidates call() and streamEvents() through a shared buildAgentStream implementation, ensuring consistent middleware chain execution. Session state management is strengthened by always reloading from the store when AgentStateStore is configured, addressing stale cache issues in distributed environments. The addition of new event types (AgentResultEvent, CustomEvent, HintBlockEvent) and metadata support on AgentEvent provides richer extensibility for event-driven consumers. Tool events are enriched with tool names, and the harness layer gains isolation scope awareness for memory middleware. The changes are well-structured, but there are a few areas that warrant attention.
| private final Model model; | ||
| private final String flushPrompt; | ||
| private final MemoryConfig.FlushTrigger flushTrigger; | ||
| private final IsolationScope isolationScope; |
There was a problem hiding this comment.
[major] Potential memory leak in long-running services: The lastRunAtByKey ConcurrentHashMap grows unbounded as unique userId/sessionId combinations are encountered. In production services handling thousands of unique sessions over time, this map will never shrink, even after sessions become inactive. Consider implementing an LRU eviction policy, periodic cleanup of stale entries (e.g., entries not accessed within a retention window), or using a bounded cache like Caffeine with expireAfterAccess. Without this, memory usage will grow linearly with unique session count.
| * namespace (see {@link MemoryFlushMiddleware} for the identical pattern). | ||
| */ | ||
| private final ConcurrentHashMap<String, AtomicReference<Instant>> lastRunAtByKey = | ||
| new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
[major] Same unbounded growth issue as MemoryFlushMiddleware: The lastRunAtByKey ConcurrentHashMap has no cleanup mechanism. Since this middleware tracks maintenance windows per isolation key, the map will accumulate entries for every unique user/session/agent seen during the agent's lifetime. For long-running services, this creates a memory leak. Apply the same mitigation strategy as suggested for MemoryFlushMiddleware.
| slot, k -> new PermissionEngine(loaded.getPermissionContext())); | ||
| AgentState loaded; | ||
| if (stateStore != null) { | ||
| loaded = |
There was a problem hiding this comment.
[minor] Code duplication in state loading logic: The activateSlotForContext method now has two nearly identical code paths (stateStore != null vs null) that differ only in whether they use put or computeIfAbsent. This duplication makes the method harder to maintain. Consider extracting a helper method like loadState(slot, shouldReload) that encapsulates the conditional logic, or use a single path with a comment explaining why the put vs computeIfAbsent distinction matters for the distributed case.
| @JsonProperty("value") Map<String, Object> value) { | ||
| super(id, createdAt); | ||
| this.name = name; | ||
| this.value = value != null ? value : Map.of(); |
There was a problem hiding this comment.
[minor] Immutable empty map may surprise callers: When value is null, the constructor assigns Map.of() which returns an immutable empty map. While this is safe, callers who expect to mutate the returned map (e.g., via event.getValue().put(...)) will get an UnsupportedOperationException. Consider documenting this immutability contract in the Javadoc for getValue(), or defensively copy to a mutable map when the input is null.
| private final String createdAt; | ||
| private String source; | ||
|
|
||
| @JsonInclude(JsonInclude.Include.NON_EMPTY) |
There was a problem hiding this comment.
[minor] Metadata field lacks validation: The withMetadata method accepts any Map<String, Object> but the metadata field is typed as Map<String, Object>. If callers store non-serializable objects or complex types in this map, Jackson serialization may fail at runtime. Consider adding a note in the Javadoc that values should be JSON-serializable, or provide a builder/validator that enforces this constraint.
| this.abstractFilesystem = abstractFilesystem; | ||
| this.pathNormalizer = pathNormalizer; | ||
| } | ||
|
|
There was a problem hiding this comment.
[minor] Silent pass-through when pathNormalizer is null: The norm() method returns the path unchanged when pathNormalizer is null. While this preserves backward compatibility, it could lead to subtle bugs if a caller expects normalization but the normalizer wasn't configured. Consider logging a debug message when normalization is skipped, or document this behavior in the FilesystemTool class Javadoc.
| .CONTEXT_KEY, | ||
| (AgentEventEmitter) | ||
| sink::next)) | ||
| .doFinally( |
There was a problem hiding this comment.
[minor] Complex conditional logic in buildAgentStream: The isSubagentBusPath check and the conditional contextWrite for AgentEventEmitter.CONTEXT_KEY is intricate and could be error-prone for future maintainers. The inline comment explains the 'why' but the logic itself is dense. Consider extracting this into a clearly-named helper method like shouldInstallEventEmitterContext(subscriberCtx) that returns a boolean, making the main flow easier to follow.
| public String normalize(String path) { | ||
| if (path == null || path.isBlank()) { | ||
| return path; | ||
| } |
There was a problem hiding this comment.
[nitpick] Missing @nullable annotation on normalize return: The normalize method can return the input path unchanged if no prefix matches, but the Javadoc doesn't explicitly state this. Consider adding a @Nullable annotation if null input is possible, or clarify in the Javadoc that the method never returns null and always returns either a normalized path or the original path.
| * <p>Named {@code hintSource} to avoid collision with {@link AgentEvent#getSource()} which | ||
| * carries the subagent forwarding path. | ||
| */ | ||
| public String getHintSource() { |
There was a problem hiding this comment.
[nitpick] hintSource naming rationale could be clearer: The Javadoc explains that hintSource avoids collision with AgentEvent.getSource(), but the name itself might confuse readers who expect 'source' to mean the event origin. Consider whether hintSender, hintOrigin, or hintAuthor would be more intuitive, or add a concrete example in the Javadoc showing typical values.
| @@ -1905,6 +1923,15 @@ public HarnessAgent build() { | |||
| } | |||
There was a problem hiding this comment.
[nitpick] Isolation scope resolution could be extracted: The logic to determine effectiveIsolationScope by checking remoteFilesystemSpec then sandboxFilesystemSpec is straightforward but repeated. If this pattern appears elsewhere, consider extracting it into a helper method like resolveEffectiveIsolationScope(remoteSpec, sandboxSpec) to improve readability and ensure consistency.
This pull request refactors and enhances the
ReActAgentclass to unify agent invocation and event streaming, improve state management for distributed deployments, and provide richer event metadata. The most significant changes include a new shared core for bothcall()andstreamEvents(), improved session state loading, and enriched tool event handling.Unified agent invocation and event streaming:
buildAgentStreammethod that serves as the shared implementation for bothcall()andstreamEvents(). This ensures the middleware chain is consistently applied, and the agent result is always emitted as anAgentResultEventbefore the end event. The publiccall()andstreamEvents()methods now delegate to this shared core, simplifying the code and guaranteeing consistent behavior.Session state management improvements:
activateSlotForContextso that when anAgentStateStoreis configured, the agent state and permission engine are always reloaded from the store at the start of each call. This ensures that distributed deployments see the latest persisted state, avoiding stale cache issues.Tool event enrichment:
Set<String>of tool IDs to aMap<String, String>of tool IDs to tool names. This allows tool events (ToolCallStartEvent,ToolCallEndEvent, etc.) to carry both the tool ID and name, providing richer event data for downstream consumers.Codebase cleanup and deprecation:
agentImplmethod and related obsolete streaming logic, consolidating all agent event streaming into the new shared core.These changes collectively make agent invocation more robust, consistent, and easier to extend, especially in distributed and event-driven environments.