diff --git a/mateclaw-server/src/main/java/vip/mate/cron/model/CronJobEntity.java b/mateclaw-server/src/main/java/vip/mate/cron/model/CronJobEntity.java index a36eca0a..60640113 100644 --- a/mateclaw-server/src/main/java/vip/mate/cron/model/CronJobEntity.java +++ b/mateclaw-server/src/main/java/vip/mate/cron/model/CronJobEntity.java @@ -64,7 +64,12 @@ public class CronJobEntity { * RFC-063r §2.9: originating channel binding. Null when this job was * created from the web (no proactive delivery target). The single * indexed column lets ops query "all jobs delivering to channel X". + * + *

{@code FieldStrategy.ALWAYS} so clearing the binding from the edit + * form actually writes NULL — the default NOT_NULL strategy drops the + * column from the UPDATE and the old channel silently survives. */ + @TableField(updateStrategy = FieldStrategy.ALWAYS) private Long channelId; /** @@ -72,7 +77,7 @@ public class CronJobEntity { * persisted as JSON via MyBatis Plus JacksonTypeHandler so future fields * don't require schema migrations. */ - @TableField(typeHandler = JacksonTypeHandler.class) + @TableField(typeHandler = JacksonTypeHandler.class, updateStrategy = FieldStrategy.ALWAYS) private DeliveryConfig deliveryConfig; @TableField(fill = FieldFill.INSERT) diff --git a/mateclaw-server/src/main/java/vip/mate/cron/repository/CronJobMapper.java b/mateclaw-server/src/main/java/vip/mate/cron/repository/CronJobMapper.java index e2038bd8..091e25e8 100644 --- a/mateclaw-server/src/main/java/vip/mate/cron/repository/CronJobMapper.java +++ b/mateclaw-server/src/main/java/vip/mate/cron/repository/CronJobMapper.java @@ -1,8 +1,12 @@ package vip.mate.cron.repository; import com.baomidou.mybatisplus.core.mapper.BaseMapper; +import com.baomidou.mybatisplus.extension.handlers.JacksonTypeHandler; import org.apache.ibatis.annotations.Mapper; import org.apache.ibatis.annotations.Param; +import org.apache.ibatis.annotations.Result; +import org.apache.ibatis.annotations.ResultMap; +import org.apache.ibatis.annotations.Results; import org.apache.ibatis.annotations.Select; import vip.mate.cron.model.CronJobEntity; @@ -31,6 +35,18 @@ public interface CronJobMapper extends BaseMapper { *

Filters out logically-deleted rows and orders by create_time DESC * to mirror the existing {@code list()} ordering. */ + // Shared result map for every hand-written query in this mapper. The + // typeHandler declared on CronJobEntity.deliveryConfig only reaches the + // result map MyBatis Plus generates for the injected BaseMapper methods; + // annotation-driven statements build their own, and auto-mapping finds no + // handler for the DeliveryConfig record, so MyBatis silently skips the + // column (default AutoMappingUnknownColumnBehavior.NONE) and every job + // read through these queries came back with a null deliveryConfig. + // Restating the handler here fixes it — the other columns still auto-map. + @Results(id = "cronJobResultMap", value = { + @Result(column = "delivery_config", property = "deliveryConfig", + typeHandler = JacksonTypeHandler.class) + }) @Select(""" SELECT j.*, (SELECT r.delivery_status FROM mate_cron_job_run r @@ -51,6 +67,7 @@ public interface CronJobMapper extends BaseMapper { * (cross-workspace access returns null → caller throws not_found, matching * the "deleted" shape so workspace existence isn't enumerable). */ + @ResultMap("cronJobResultMap") @Select(""" SELECT j.*, (SELECT r.delivery_status FROM mate_cron_job_run r @@ -70,6 +87,7 @@ public interface CronJobMapper extends BaseMapper { * toggle / runNow). Skips the delivery-status subquery — those paths * don't need it and pay for the correlated lookup otherwise. */ + @ResultMap("cronJobResultMap") @Select("SELECT * FROM mate_cron_job WHERE id = #{id} AND deleted = 0 AND workspace_id = #{workspaceId}") CronJobEntity selectByIdAndWorkspace(@Param("id") Long id, @Param("workspaceId") Long workspaceId); diff --git a/mateclaw-server/src/main/java/vip/mate/cron/service/CronJobService.java b/mateclaw-server/src/main/java/vip/mate/cron/service/CronJobService.java index c83c4923..221526e2 100644 --- a/mateclaw-server/src/main/java/vip/mate/cron/service/CronJobService.java +++ b/mateclaw-server/src/main/java/vip/mate/cron/service/CronJobService.java @@ -353,6 +353,13 @@ public class CronJobService implements ApplicationRunner { existing.setTaskType(dto.getTaskType()); existing.setTriggerMessage(dto.getTriggerMessage()); existing.setRequestBody(dto.getRequestBody()); + // The edit form always submits the full delivery binding (channel + + // target + suppress flag), so the request is authoritative for both + // fields — including a null pair, which means "unbind this job from + // its channel". FieldStrategy.ALWAYS on the entity lets the null + // through to the UPDATE. + existing.setChannelId(dto.getChannelId()); + existing.setDeliveryConfig(dto.getDeliveryConfig()); if (dto.getEnabled() != null) { existing.setEnabled(dto.getEnabled()); } diff --git a/mateclaw-server/src/main/java/vip/mate/tool/builtin/CronJobTool.java b/mateclaw-server/src/main/java/vip/mate/tool/builtin/CronJobTool.java index d497b990..2d8c90f5 100644 --- a/mateclaw-server/src/main/java/vip/mate/tool/builtin/CronJobTool.java +++ b/mateclaw-server/src/main/java/vip/mate/tool/builtin/CronJobTool.java @@ -10,7 +10,9 @@ import org.springframework.lang.Nullable; import org.springframework.stereotype.Component; import vip.mate.agent.context.ChatOrigin; import vip.mate.cron.model.CronJobDTO; +import vip.mate.cron.model.DeliveryConfig; import vip.mate.cron.service.CronJobService; +import vip.mate.tool.ConcurrencyUnsafe; import java.util.ArrayList; import java.util.LinkedHashMap; @@ -25,7 +27,7 @@ import java.util.Map; * from natural language (e.g. "every day at 9am" → "0 9 * * *"). * * @author MateClaw Team - * @see vip.mate.cron.service.CronJobService + * @see CronJobService */ @Slf4j @Component @@ -42,7 +44,7 @@ public class CronJobTool { */ private final ObjectMapper objectMapper; - @vip.mate.tool.ConcurrencyUnsafe("cron job creation persists to mate_cron_job; concurrent creates can race on name") + @ConcurrencyUnsafe("cron job creation persists to mate_cron_job; concurrent creates can race on name") @Tool(description = "Create a scheduled task that asks the agent to do something at a specific time — " + "the trigger message is sent to the LLM, which can use tools (search, weather, etc.) to produce the answer. " + "Use this for queries like 'every morning give me a weather report' or 'daily news summary'. " @@ -119,7 +121,7 @@ public class CronJobTool { } } - @vip.mate.tool.ConcurrencyUnsafe("reminder creation persists to mate_cron_job; concurrent creates can race on name") + @ConcurrencyUnsafe("reminder creation persists to mate_cron_job; concurrent creates can race on name") @Tool(description = "Create a scheduled REMINDER. The reminder text is delivered to the user verbatim at the " + "scheduled time — no LLM call, no rephrasing, no token cost. " + "Use this when the user wants a notification with specific content (e.g. 'remind me at 3pm to leave for the meeting' → " @@ -211,7 +213,7 @@ public class CronJobTool { } } - @vip.mate.tool.ConcurrencyUnsafe("toggles row state in mate_cron_job; serialize to keep enabled/disabled deterministic") + @ConcurrencyUnsafe("toggles row state in mate_cron_job; serialize to keep enabled/disabled deterministic") @Tool(description = "Enable or disable a scheduled task by its job ID. " + "Use list_cron_jobs first to find the job ID.") public String toggle_cron_job( @@ -236,7 +238,7 @@ public class CronJobTool { } } - @vip.mate.tool.ConcurrencyUnsafe("destructive — removes row from mate_cron_job") + @ConcurrencyUnsafe("destructive — removes row from mate_cron_job") @Tool(description = "Delete a scheduled task by its job ID. This action requires user approval. " + "Use list_cron_jobs first to find the job ID.") public String delete_cron_job( @@ -306,7 +308,7 @@ public class CronJobTool { // store it as session.targetId, which never equals the cron's own // chatId/senderId-derived targetId — the senderId match is the // stable common key. - dto.setDeliveryConfig(vip.mate.cron.model.DeliveryConfig.from( + dto.setDeliveryConfig(DeliveryConfig.from( origin.channelTarget(), origin.requesterId())); } } diff --git a/mateclaw-server/src/test/java/vip/mate/cron/service/CronJobDeliveryPersistenceTest.java b/mateclaw-server/src/test/java/vip/mate/cron/service/CronJobDeliveryPersistenceTest.java new file mode 100644 index 00000000..5821b9f2 --- /dev/null +++ b/mateclaw-server/src/test/java/vip/mate/cron/service/CronJobDeliveryPersistenceTest.java @@ -0,0 +1,123 @@ +package vip.mate.cron.service; + +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.test.context.TestPropertySource; +import vip.mate.MateClawApplication; +import vip.mate.cron.model.CronJobDTO; +import vip.mate.cron.model.DeliveryConfig; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Regression coverage for the delivery binding on the cron CRUD path: + * {@code channelId} and {@code deliveryConfig} must survive a create, be + * readable back through the list/detail queries, and be mutable through + * {@code update()} — including clearing the binding entirely. + */ +@SpringBootTest( + classes = MateClawApplication.class, + webEnvironment = SpringBootTest.WebEnvironment.NONE +) +@TestPropertySource(properties = { + "spring.datasource.url=jdbc:h2:mem:cron_delivery_test_${random.uuid};MODE=MySQL;DATABASE_TO_LOWER=TRUE;CASE_INSENSITIVE_IDENTIFIERS=TRUE;DB_CLOSE_DELAY=-1", + "spring.ai.dashscope.api-key=test-key", + "spring.main.web-application-type=none" +}) +class CronJobDeliveryPersistenceTest { + + private static final long WORKSPACE_ID = 1L; + + @Autowired + private CronJobService cronJobService; + + private CronJobDTO newDto(String name, Long channelId, DeliveryConfig deliveryConfig) { + CronJobDTO dto = new CronJobDTO(); + dto.setName(name); + dto.setCronExpression("*/5 * * * *"); + dto.setTimezone("Asia/Shanghai"); + dto.setAgentId(9001L); + dto.setTaskType("text"); + dto.setTriggerMessage("ping"); + dto.setEnabled(false); + dto.setChannelId(channelId); + dto.setDeliveryConfig(deliveryConfig); + return dto; + } + + @Test + @DisplayName("create persists the delivery binding and the read path returns it") + void createPersistsDeliveryBinding() { + CronJobDTO created = cronJobService.create( + newDto("delivery-create", 7001L, + new DeliveryConfig("target-1", null, null, "user-1", Boolean.FALSE)), + WORKSPACE_ID); + + CronJobDTO loaded = cronJobService.getById(created.getId(), WORKSPACE_ID); + assertEquals(7001L, loaded.getChannelId()); + assertNotNull(loaded.getDeliveryConfig(), "deliveryConfig must round-trip through the detail query"); + assertEquals("target-1", loaded.getDeliveryConfig().targetId()); + assertEquals("user-1", loaded.getDeliveryConfig().userId()); + } + + @Test + @DisplayName("update rewrites channelId and deliveryConfig") + void updateRewritesDeliveryBinding() { + CronJobDTO created = cronJobService.create( + newDto("delivery-update", 7001L, + new DeliveryConfig("target-1", null, null, "user-1", Boolean.FALSE)), + WORKSPACE_ID); + + CronJobDTO patch = newDto("delivery-update", 7002L, + new DeliveryConfig("target-2", null, null, "user-2", Boolean.TRUE)); + cronJobService.update(created.getId(), patch, WORKSPACE_ID); + + CronJobDTO loaded = cronJobService.getById(created.getId(), WORKSPACE_ID); + assertEquals(7002L, loaded.getChannelId(), "channel rebinding must persist"); + assertNotNull(loaded.getDeliveryConfig()); + assertEquals("target-2", loaded.getDeliveryConfig().targetId()); + assertEquals("user-2", loaded.getDeliveryConfig().userId()); + assertTrue(loaded.getDeliveryConfig().isAgentReplySuppressed(), + "suppressAgentReply toggle must persist"); + } + + @Test + @DisplayName("toggle preserves the delivery binding") + void togglePreservesDeliveryBinding() { + CronJobDTO created = cronJobService.create( + newDto("delivery-toggle", 7001L, + new DeliveryConfig("target-1", null, null, "user-1", Boolean.TRUE)), + WORKSPACE_ID); + + cronJobService.toggle(created.getId(), true, WORKSPACE_ID); + cronJobService.toggle(created.getId(), false, WORKSPACE_ID); + + CronJobDTO loaded = cronJobService.getById(created.getId(), WORKSPACE_ID); + assertEquals(7001L, loaded.getChannelId()); + assertNotNull(loaded.getDeliveryConfig(), + "enable/disable must not wipe the delivery binding"); + assertEquals("target-1", loaded.getDeliveryConfig().targetId()); + assertTrue(loaded.getDeliveryConfig().isAgentReplySuppressed()); + } + + @Test + @DisplayName("update can clear the delivery binding") + void updateClearsDeliveryBinding() { + CronJobDTO created = cronJobService.create( + newDto("delivery-clear", 7001L, + new DeliveryConfig("target-1", null, null, "user-1", Boolean.FALSE)), + WORKSPACE_ID); + + CronJobDTO patch = newDto("delivery-clear", null, null); + cronJobService.update(created.getId(), patch, WORKSPACE_ID); + + CronJobDTO loaded = cronJobService.getById(created.getId(), WORKSPACE_ID); + assertNull(loaded.getChannelId(), "unbinding a channel must persist"); + assertNull(loaded.getDeliveryConfig(), "clearing the delivery target must persist"); + } +}