From 2f7eda05fdb678fbbd42f5a74ee41277643c6af2 Mon Sep 17 00:00:00 2001 From: jwalker Date: Tue, 11 Aug 2026 10:31:14 -0500 Subject: [PATCH] Derive a conversation id for tasks instead of using the task file path TaskHandler passed the task id straight to the agent as the conversation id. A task id is the absolute path of its markdown file, so it reached FileSystemChatMemoryRepository.resolveFile, which builds conversations/chat-{id}.yaml. With separators still in the string that is not one file but a directory tree mirroring the task's own location on disk: the memory is scattered, findConversationIds() never lists it because it only looks at the top level, and on Windows the colon in the drive letter makes the name invalid. Derive the id from the last two segments of the task path instead, so {date}/{time}-{name}.md becomes task-{date}-{time}-{name} and recurring/{name}.md becomes task-recurring-{name}. Keeping the date directory matters: without it two tasks with the same name and time on different days would share one conversation. The path is normalized first and anything outside the character set task file names already use is replaced, so the result is always a single safe file name component. Deriving nothing throws rather than falling back to the raw path, which would quietly reintroduce the bug. TaskHandler had no test coverage; add TaskHandlerTest. One assertion in TaskManagerTest pinned the old format and is updated. Co-Authored-By: Claude Opus 5 --- .../java/ai/javaclaw/tasks/TaskHandler.java | 61 ++++++- .../ai/javaclaw/tasks/TaskHandlerTest.java | 150 ++++++++++++++++++ .../ai/javaclaw/tasks/TaskManagerTest.java | 3 +- 3 files changed, 212 insertions(+), 2 deletions(-) create mode 100644 base/src/test/java/ai/javaclaw/tasks/TaskHandlerTest.java diff --git a/base/src/main/java/ai/javaclaw/tasks/TaskHandler.java b/base/src/main/java/ai/javaclaw/tasks/TaskHandler.java index 91529f5f..6d33ba08 100644 --- a/base/src/main/java/ai/javaclaw/tasks/TaskHandler.java +++ b/base/src/main/java/ai/javaclaw/tasks/TaskHandler.java @@ -9,6 +9,9 @@ import org.slf4j.LoggerFactory; import org.springframework.stereotype.Component; +import java.nio.file.Path; +import java.util.regex.Pattern; + import static ai.javaclaw.tasks.Task.Status.awaiting_human_input; import static ai.javaclaw.tasks.Task.Status.completed; import static java.util.Optional.ofNullable; @@ -18,6 +21,12 @@ public class TaskHandler { private static final Logger LOGGER = new JobRunrDashboardLogger(LoggerFactory.getLogger(TaskHandler.class)); + private static final String CONVERSATION_ID_PREFIX = "task-"; + private static final String TASK_FILE_EXTENSION = ".md"; + /** The character set task file names are already restricted to by {@code FileSystemTaskRepository}. */ + private static final Pattern UNSAFE_CHARACTERS = Pattern.compile("[^a-zA-Z0-9._-]"); + private static final Pattern REPEATED_SEPARATORS = Pattern.compile("_{2,}"); + private final Agent agent; private final TaskRepository taskRepository; private final ChannelRegistry channelRegistry; @@ -40,7 +49,7 @@ public void executeTask(String taskId) { try { LOGGER.info("Starting task: {}", task.getName()); String agentInput = formatTaskForAgent(inProgress); - TaskResult result = agent.prompt(taskId, agentInput, TaskResult.class); + TaskResult result = agent.prompt(getConversationId(taskId), agentInput, TaskResult.class); taskRepository.save(inProgress.withFeedback(result.feedback()).withStatus(result.newStatus())); notifyUser(task.getName(), result); LOGGER.info("Finished task: {} with status {}", task.getName(), result.newStatus()); @@ -66,6 +75,56 @@ private void notifyUser(String taskName, TaskResult result) { } } + /** + * Derives the conversation id used for a task's chat memory. + * + *

A task's id is the absolute path of its markdown file. Passed straight through it reaches + * {@code FileSystemChatMemoryRepository}, which resolves {@code conversations/chat-{id}.yaml} - + * so each task creates a directory tree mirroring its own location on disk instead of a single + * conversation file, never appears in the conversation list, and on Windows is rejected outright + * because of the colon in the drive letter. + * + *

The last two path segments carry everything needed: tasks are stored as + * {@code {date}/{time}-{name}.md} and recurring ones as {@code recurring/{name}.md}, so the pair + * is unique across days and stays readable. Characters outside the set task file names already + * use are replaced, which keeps the result a single safe file name component whatever the id + * turns out to contain. + * + * @throws IllegalArgumentException if no id can be derived, rather than falling back to the + * path and recreating the problem this method exists to avoid + */ + static String getConversationId(String taskId) { + if (taskId == null || taskId.isBlank()) { + throw new IllegalArgumentException("Cannot derive a conversation id from an empty task id"); + } + + Path taskFile = Path.of(taskId).normalize(); + Path fileName = taskFile.getFileName(); + Path parent = taskFile.getParent(); + if (fileName == null) { + throw new IllegalArgumentException("Cannot derive a conversation id from a task id that names no file"); + } + + String name = removeExtension(fileName.toString()); + String group = parent == null || parent.getFileName() == null ? "" : parent.getFileName().toString(); + if (name.isBlank() && group.isBlank()) { + throw new IllegalArgumentException("Cannot derive a conversation id from a task id that names no file"); + } + + return sanitize(CONVERSATION_ID_PREFIX + (group.isBlank() ? name : group + "-" + name)); + } + + private static String removeExtension(String fileName) { + return fileName.endsWith(TASK_FILE_EXTENSION) + ? fileName.substring(0, fileName.length() - TASK_FILE_EXTENSION.length()) + : fileName; + } + + private static String sanitize(String conversationId) { + String replaced = UNSAFE_CHARACTERS.matcher(conversationId).replaceAll("_"); + return REPEATED_SEPARATORS.matcher(replaced).replaceAll("_"); + } + private String formatTaskForAgent(Task task) { return String.format(""" Handle the following task and report the new status ('completed' or 'awaiting_human_input') with the feedback what was done diff --git a/base/src/test/java/ai/javaclaw/tasks/TaskHandlerTest.java b/base/src/test/java/ai/javaclaw/tasks/TaskHandlerTest.java new file mode 100644 index 00000000..b4e8ea88 --- /dev/null +++ b/base/src/test/java/ai/javaclaw/tasks/TaskHandlerTest.java @@ -0,0 +1,150 @@ +package ai.javaclaw.tasks; + +import ai.javaclaw.agent.Agent; +import ai.javaclaw.channels.ChannelRegistry; +import ai.javaclaw.tasks.TaskHandler.TaskResult; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.core.io.FileSystemResource; + +import java.io.IOException; +import java.nio.file.Path; +import java.time.Instant; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.when; + +@ExtendWith(MockitoExtension.class) +class TaskHandlerTest { + + @TempDir + Path workspaceDir; + + @Mock + Agent agentMock; + @Mock + ChannelRegistry channelRegistryMock; + + TaskRepository taskRepository; + TaskHandler taskHandler; + + @BeforeEach + void setUp() throws IOException { + taskRepository = new FileSystemTaskRepository(new FileSystemResource(workspaceDir)); + taskHandler = new TaskHandler(agentMock, taskRepository, channelRegistryMock); + } + + // ----------------------------------------------------------------------- + // The conversation id the agent is called with + // + // A task's id is the absolute path of its markdown file. Handing that to + // the agent as a conversation id makes the memory repository resolve + // conversations/chat-.yaml, which spreads one file per task + // across a directory tree mirroring the filesystem. + // ----------------------------------------------------------------------- + + @Test + void callsTheAgentWithAConversationIdThatIsNotTheTaskFilePath() { + Task task = givenATodoTask("Buy milk"); + when(agentMock.prompt(anyString(), anyString(), eq(TaskResult.class))) + .thenReturn(new TaskResult(Task.Status.completed, "done")); + + taskHandler.executeTask(task.getId()); + + assertThat(conversationIdPassedToAgent()) + .isNotEqualTo(task.getId()) + .doesNotContain("/") + .doesNotContain("\\") + .startsWith("task-"); + } + + @Test + void derivesTheConversationIdFromTheTasksDateDirectoryAndFileName() { + Task task = givenATodoTask("Buy milk"); + when(agentMock.prompt(anyString(), anyString(), eq(TaskResult.class))) + .thenReturn(new TaskResult(Task.Status.completed, "done")); + + taskHandler.executeTask(task.getId()); + + Path taskFile = Path.of(task.getId()); + String dateDirectory = taskFile.getParent().getFileName().toString(); + String fileName = taskFile.getFileName().toString().replace(".md", ""); + assertThat(conversationIdPassedToAgent()).isEqualTo("task-" + dateDirectory + "-" + fileName); + } + + private String conversationIdPassedToAgent() { + ArgumentCaptor conversationId = ArgumentCaptor.forClass(String.class); + org.mockito.Mockito.verify(agentMock) + .prompt(conversationId.capture(), anyString(), eq(TaskResult.class)); + return conversationId.getValue(); + } + + private Task givenATodoTask(String name) { + return taskRepository.save(new Task(null, name, Instant.now(), Task.Status.todo, "Some description")); + } + + // ----------------------------------------------------------------------- + // Deriving the id, in isolation + // ----------------------------------------------------------------------- + + @Test + void keepsTheDateDirectorySoTasksOnDifferentDaysDoNotShareAConversation() { + String monday = TaskHandler.getConversationId("/workspace/tasks/2026-08-10/103000-buy_milk.md"); + String tuesday = TaskHandler.getConversationId("/workspace/tasks/2026-08-11/103000-buy_milk.md"); + + assertThat(monday).isEqualTo("task-2026-08-10-103000-buy_milk"); + assertThat(tuesday).isEqualTo("task-2026-08-11-103000-buy_milk"); + assertThat(monday).isNotEqualTo(tuesday); + } + + @Test + void usesTheNameAloneWhenTheTaskIdHasNoDirectory() { + String id = TaskHandler.getConversationId("some-id"); + + assertThat(id).isEqualTo("task-some-id"); + } + + @Test + void namesRecurringTasksAfterTheirOwnDirectory() { + String id = TaskHandler.getConversationId("/workspace/tasks/recurring/water_the_plants.md"); + + assertThat(id).isEqualTo("task-recurring-water_the_plants"); + } + + @Test + void replacesCharactersThatCannotSafelyAppearInAFileName() { + String id = TaskHandler.getConversationId("/workspace/tasks/2026-08-11/103000-a b:c.md"); + + assertThat(id).isEqualTo("task-2026-08-11-103000-a_b_c"); + } + + @Test + void collapsesTheSeparatorsIntroducedByReplacement() { + String id = TaskHandler.getConversationId("/workspace/tasks/2026-08-11/103000-a???b.md"); + + assertThat(id).isEqualTo("task-2026-08-11-103000-a_b"); + } + + @Test + void resolvesRelativeSegmentsBeforeDerivingTheId() { + String id = TaskHandler.getConversationId("/workspace/tasks/2026-08-11/../2026-08-10/103000-buy_milk.md"); + + assertThat(id).isEqualTo("task-2026-08-10-103000-buy_milk"); + } + + @Test + void refusesATaskIdItCannotDeriveAnIdFrom() { + assertThatThrownBy(() -> TaskHandler.getConversationId(null)) + .isInstanceOf(IllegalArgumentException.class); + assertThatThrownBy(() -> TaskHandler.getConversationId(" ")) + .isInstanceOf(IllegalArgumentException.class); + } +} diff --git a/base/src/test/java/ai/javaclaw/tasks/TaskManagerTest.java b/base/src/test/java/ai/javaclaw/tasks/TaskManagerTest.java index cfcf9ab3..155d363a 100644 --- a/base/src/test/java/ai/javaclaw/tasks/TaskManagerTest.java +++ b/base/src/test/java/ai/javaclaw/tasks/TaskManagerTest.java @@ -70,7 +70,8 @@ void createEnqueuesJob() { Task saved = new Task("some-id", "handle-email", Instant.now(), Task.Status.todo, "Process unread email messages"); when(taskRepositoryMock.save(any(Task.class))).thenReturn(saved); when(taskRepositoryMock.getTaskById("some-id")).thenReturn(saved); - when(agentMock.prompt(eq("some-id"), anyString(), any())).thenReturn(new TaskResult(Status.completed, "All mail was summarized!")); + // The agent is called with the task's conversation id, not with the task id itself + when(agentMock.prompt(eq("task-some-id"), anyString(), any())).thenReturn(new TaskResult(Status.completed, "All mail was summarized!")); taskManager.create("handle-email", "Process unread email messages");