-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix(middleware): avoid event-loop blocking in WorkspaceContextMiddleware onSystemPrompt #2632
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
9c31ea6
b320802
f1c544d
1482a1e
f9e4ca4
3dacad1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,7 @@ | |
| import java.util.List; | ||
| import java.util.stream.Collectors; | ||
| import reactor.core.publisher.Mono; | ||
| import reactor.core.scheduler.Schedulers; | ||
|
|
||
| /** | ||
| * Appends workspace context (session info, AGENTS.md, MEMORY.md, knowledge) to the | ||
|
|
@@ -181,14 +182,15 @@ public boolean isDisableMemoryHooks() { | |
|
|
||
| @Override | ||
| public Mono<String> onSystemPrompt(Agent agent, RuntimeContext ctx, String currentPrompt) { | ||
| RuntimeContext rc = ctx != null ? ctx : RuntimeContext.empty(); | ||
| String section = buildWorkspaceSection(rc); | ||
| if (section.isEmpty()) { | ||
| return Mono.just(currentPrompt); | ||
| } | ||
| String base = currentPrompt != null ? currentPrompt : ""; | ||
| String separator = base.isEmpty() || base.endsWith("\n") ? "" : "\n"; | ||
| return Mono.just(base + separator + section); | ||
| return Mono.fromCallable( | ||
| () -> { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [minor] Removed empty-section early-return guard. The original code had |
||
| RuntimeContext rc = ctx != null ? ctx : RuntimeContext.empty(); | ||
| String base = currentPrompt != null ? currentPrompt : ""; | ||
| String section = buildWorkspaceSection(rc); | ||
| String separator = base.isEmpty() || base.endsWith("\n") ? "" : "\n"; | ||
| return base + separator + section; | ||
| }) | ||
| .subscribeOn(Schedulers.boundedElastic()); | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [major] Missing
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [major] Missing |
||
|
|
||
| private String buildWorkspaceSection(RuntimeContext rc) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ | |
|
|
||
| import static org.junit.jupiter.api.Assertions.assertFalse; | ||
| import static org.junit.jupiter.api.Assertions.assertNotNull; | ||
| import static org.junit.jupiter.api.Assertions.assertNotSame; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| import io.agentscope.core.agent.RuntimeContext; | ||
|
|
@@ -25,6 +26,7 @@ | |
| import java.nio.file.Path; | ||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
| import java.util.concurrent.atomic.AtomicReference; | ||
| import org.junit.jupiter.api.AfterEach; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.api.io.TempDir; | ||
|
|
@@ -52,6 +54,45 @@ private WorkspaceManager track(WorkspaceManager wm) { | |
|
|
||
| @TempDir Path workspace; | ||
|
|
||
| @Test | ||
| void onSystemPromptBuildsWorkspaceContextOnBoundedElastic() { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nitpick] The test assertion
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nitpick] The test assertion |
||
| Thread callerThread = Thread.currentThread(); | ||
| AtomicReference<Thread> readThread = new AtomicReference<>(); | ||
| WorkspaceManager wm = | ||
| track( | ||
| new WorkspaceManager(workspace) { | ||
| @Override | ||
| public String readAgentsMd(RuntimeContext rc) { | ||
| readThread.set(Thread.currentThread()); | ||
| return "agent persona"; | ||
| } | ||
| }); | ||
| WorkspaceContextMiddleware mw = new WorkspaceContextMiddleware(wm); | ||
|
|
||
| String prompt = mw.onSystemPrompt(null, RuntimeContext.empty(), "BASE\n").block(); | ||
|
|
||
| assertNotNull(prompt); | ||
| assertTrue(prompt.contains("agent persona")); | ||
| assertNotNull(readThread.get()); | ||
| assertNotSame( | ||
| callerThread, readThread.get(), "workspace context read ran on caller thread"); | ||
| } | ||
|
|
||
| @Test | ||
| void onSystemPromptHandlesNullAndNonNewlineBasePrompts() { | ||
| WorkspaceManager wm = track(new WorkspaceManager(workspace)); | ||
| WorkspaceContextMiddleware mw = new WorkspaceContextMiddleware(wm); | ||
|
|
||
| String promptWithoutBase = mw.onSystemPrompt(null, null, null).block(); | ||
| String promptWithBase = mw.onSystemPrompt(null, RuntimeContext.empty(), "BASE").block(); | ||
|
|
||
| assertNotNull(promptWithoutBase); | ||
| assertFalse(promptWithoutBase.startsWith("null")); | ||
| assertTrue(promptWithoutBase.contains("## Domain Knowledge")); | ||
| assertNotNull(promptWithBase); | ||
| assertTrue(promptWithBase.startsWith("BASE\n")); | ||
| } | ||
|
|
||
| @Test | ||
| void defaultFlags_includeMemoryRecallPersistenceAndContext() throws Exception { | ||
| Files.writeString(workspace.resolve("MEMORY.md"), "remember: cats prefer windowsills"); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[minor] Removed empty-section early-return guard. The original code had
if (section.isEmpty()) return Mono.just(currentPrompt)which short-circuited when no workspace files existed. The new code always concatenates the section (even if empty), which may append an unnecessary trailing newline. Consider restoring the isEmpty guard inside the callable.