From 5235e3013884ae1809ea37b4e01b1b70e4d4fd3c Mon Sep 17 00:00:00 2001 From: matevip Date: Thu, 11 Jun 2026 15:17:51 +0800 Subject: [PATCH] fix(tool): keep Snowflake ids precise across the tool boundary (#319) --- .../vip/mate/tool/builtin/CronJobTool.java | 126 +++++++++++------- .../vip/mate/tool/builtin/DatasourceTool.java | 41 ++++-- .../builtin/CronJobToolIdPrecisionTest.java | 55 ++++++++ .../DatasourceToolIdPrecisionTest.java | 59 ++++++++ 4 files changed, 216 insertions(+), 65 deletions(-) create mode 100644 mateclaw-server/src/test/java/vip/mate/tool/builtin/CronJobToolIdPrecisionTest.java create mode 100644 mateclaw-server/src/test/java/vip/mate/tool/builtin/DatasourceToolIdPrecisionTest.java 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 cf3ca8f7..d497b990 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 @@ -1,8 +1,6 @@ package vip.mate.tool.builtin; -import cn.hutool.json.JSONArray; -import cn.hutool.json.JSONObject; -import cn.hutool.json.JSONUtil; +import com.fasterxml.jackson.databind.ObjectMapper; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.springframework.ai.chat.model.ToolContext; @@ -14,7 +12,10 @@ import vip.mate.agent.context.ChatOrigin; import vip.mate.cron.model.CronJobDTO; import vip.mate.cron.service.CronJobService; +import java.util.ArrayList; +import java.util.LinkedHashMap; import java.util.List; +import java.util.Map; /** * Built-in tool: scheduled task (cron job) management via chat. @@ -32,6 +33,14 @@ import java.util.List; public class CronJobTool { private final CronJobService cronJobService; + /** + * The application ObjectMapper serializes every {@code Long} as a JSON string + * (see {@code JacksonConfig}). Tool output goes through it so a 19-digit + * Snowflake {@code jobId} reaches the model as a string — never a JSON number + * that loses its low digits in a double / JS Number round-trip on the way + * back into toggle_cron_job / delete_cron_job. + */ + private final ObjectMapper objectMapper; @vip.mate.tool.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 — " @@ -94,15 +103,15 @@ public class CronJobTool { Long workspaceId = origin.workspaceId() != null ? origin.workspaceId() : 1L; CronJobDTO created = cronJobService.create(dto, workspaceId); - JSONObject result = new JSONObject(); - result.set("success", true); - result.set("jobId", created.getId()); - result.set("name", created.getName()); - result.set("cronExpression", created.getCronExpression()); - result.set("timezone", created.getTimezone()); - result.set("nextRunTime", created.getNextRunTime() != null ? created.getNextRunTime().toString() : ""); - result.set("enabled", created.getEnabled()); - return JSONUtil.toJsonPrettyStr(result); + Map result = new LinkedHashMap<>(); + result.put("success", true); + result.put("jobId", created.getId()); + result.put("name", created.getName()); + result.put("cronExpression", created.getCronExpression()); + result.put("timezone", created.getTimezone()); + result.put("nextRunTime", created.getNextRunTime() != null ? created.getNextRunTime().toString() : ""); + result.put("enabled", created.getEnabled()); + return writeJson(result); } catch (Exception e) { log.error("[CronJobTool] create failed: {}", e.getMessage()); @@ -154,16 +163,16 @@ public class CronJobTool { Long workspaceId = origin.workspaceId() != null ? origin.workspaceId() : 1L; CronJobDTO created = cronJobService.create(dto, workspaceId); - JSONObject result = new JSONObject(); - result.set("success", true); - result.set("jobId", created.getId()); - result.set("name", created.getName()); - result.set("taskType", "reminder"); - result.set("cronExpression", created.getCronExpression()); - result.set("timezone", created.getTimezone()); - result.set("nextRunTime", created.getNextRunTime() != null ? created.getNextRunTime().toString() : ""); - result.set("enabled", created.getEnabled()); - return JSONUtil.toJsonPrettyStr(result); + Map result = new LinkedHashMap<>(); + result.put("success", true); + result.put("jobId", created.getId()); + result.put("name", created.getName()); + result.put("taskType", "reminder"); + result.put("cronExpression", created.getCronExpression()); + result.put("timezone", created.getTimezone()); + result.put("nextRunTime", created.getNextRunTime() != null ? created.getNextRunTime().toString() : ""); + result.put("enabled", created.getEnabled()); + return writeJson(result); } catch (Exception e) { log.error("[CronJobTool] create_reminder failed: {}", e.getMessage()); @@ -179,23 +188,23 @@ public class CronJobTool { // sees the cron jobs of the workspace it's running in. Long workspaceId = workspaceFromContext(ctx); List jobs = cronJobService.list(workspaceId); - JSONArray arr = new JSONArray(); + List> arr = new ArrayList<>(); for (CronJobDTO job : jobs) { - JSONObject obj = new JSONObject(); - obj.set("jobId", job.getId()); - obj.set("name", job.getName()); - obj.set("cronExpression", job.getCronExpression()); - obj.set("timezone", job.getTimezone()); - obj.set("enabled", job.getEnabled()); - obj.set("nextRunTime", job.getNextRunTime() != null ? job.getNextRunTime().toString() : ""); - obj.set("lastRunTime", job.getLastRunTime() != null ? job.getLastRunTime().toString() : ""); - obj.set("agentName", job.getAgentName()); + Map obj = new LinkedHashMap<>(); + obj.put("jobId", job.getId()); + obj.put("name", job.getName()); + obj.put("cronExpression", job.getCronExpression()); + obj.put("timezone", job.getTimezone()); + obj.put("enabled", job.getEnabled()); + obj.put("nextRunTime", job.getNextRunTime() != null ? job.getNextRunTime().toString() : ""); + obj.put("lastRunTime", job.getLastRunTime() != null ? job.getLastRunTime().toString() : ""); + obj.put("agentName", job.getAgentName()); arr.add(obj); } - JSONObject result = new JSONObject(); - result.set("totalJobs", jobs.size()); - result.set("jobs", arr); - return JSONUtil.toJsonPrettyStr(result); + Map result = new LinkedHashMap<>(); + result.put("totalJobs", jobs.size()); + result.put("jobs", arr); + return writeJson(result); } catch (Exception e) { log.error("[CronJobTool] list failed: {}", e.getMessage()); return errorResult("Failed to list cron jobs: " + e.getMessage()); @@ -214,13 +223,13 @@ public class CronJobTool { Long workspaceId = workspaceFromContext(ctx); cronJobService.toggle(jobId, enabled, workspaceId); CronJobDTO updated = cronJobService.getById(jobId, workspaceId); - JSONObject result = new JSONObject(); - result.set("success", true); - result.set("jobId", jobId); - result.set("name", updated.getName()); - result.set("enabled", updated.getEnabled()); - result.set("nextRunTime", updated.getNextRunTime() != null ? updated.getNextRunTime().toString() : ""); - return JSONUtil.toJsonPrettyStr(result); + Map result = new LinkedHashMap<>(); + result.put("success", true); + result.put("jobId", jobId); + result.put("name", updated.getName()); + result.put("enabled", updated.getEnabled()); + result.put("nextRunTime", updated.getNextRunTime() != null ? updated.getNextRunTime().toString() : ""); + return writeJson(result); } catch (Exception e) { log.error("[CronJobTool] toggle failed: {}", e.getMessage()); return errorResult("Failed to toggle cron job: " + e.getMessage()); @@ -239,10 +248,10 @@ public class CronJobTool { CronJobDTO job = cronJobService.getById(jobId, workspaceId); String jobName = job.getName(); cronJobService.delete(jobId, workspaceId); - JSONObject result = new JSONObject(); - result.set("success", true); - result.set("deleted", jobName); - return JSONUtil.toJsonPrettyStr(result); + Map result = new LinkedHashMap<>(); + result.put("success", true); + result.put("deleted", jobName); + return writeJson(result); } catch (Exception e) { log.error("[CronJobTool] delete failed: {}", e.getMessage()); return errorResult("Failed to delete cron job: " + e.getMessage()); @@ -250,10 +259,25 @@ public class CronJobTool { } private String errorResult(String message) { - JSONObject result = new JSONObject(); - result.set("success", false); - result.set("error", message); - return JSONUtil.toJsonPrettyStr(result); + Map result = new LinkedHashMap<>(); + result.put("success", false); + result.put("error", message); + return writeJson(result); + } + + /** + * Serialize tool output through the id-safe application ObjectMapper so every + * {@code Long} (notably {@code jobId}) is rendered as a string. Falls back to + * a minimal literal on the rare serialization failure rather than throwing + * out of a tool call. + */ + private String writeJson(Object value) { + try { + return objectMapper.writerWithDefaultPrettyPrinter().writeValueAsString(value); + } catch (Exception e) { + log.error("[CronJobTool] result serialization failed: {}", e.getMessage()); + return "{\"success\":false,\"error\":\"result serialization failed\"}"; + } } /** diff --git a/mateclaw-server/src/main/java/vip/mate/tool/builtin/DatasourceTool.java b/mateclaw-server/src/main/java/vip/mate/tool/builtin/DatasourceTool.java index 069510a6..9c464a7b 100644 --- a/mateclaw-server/src/main/java/vip/mate/tool/builtin/DatasourceTool.java +++ b/mateclaw-server/src/main/java/vip/mate/tool/builtin/DatasourceTool.java @@ -1,8 +1,8 @@ package vip.mate.tool.builtin; -import cn.hutool.json.JSONArray; import cn.hutool.json.JSONObject; import cn.hutool.json.JSONUtil; +import com.fasterxml.jackson.databind.ObjectMapper; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.springframework.ai.tool.annotation.Tool; @@ -14,7 +14,9 @@ import vip.mate.datasource.service.DatasourceService; import java.sql.*; import java.util.ArrayList; +import java.util.LinkedHashMap; import java.util.List; +import java.util.Map; import java.util.regex.Pattern; /** @@ -32,6 +34,14 @@ public class DatasourceTool { private final DatasourceService datasourceService; private final DatasourceConnectionManager connectionManager; + /** + * The application ObjectMapper, which serializes every {@code Long} as a JSON + * string (see {@code JacksonConfig}). Tool output goes through it so 19-digit + * Snowflake ids reach the model as strings — exactly like the HTTP API — and + * never as JSON numbers that lose their low digits in a double/JS-number + * round-trip on the way back into a tool call. + */ + private final ObjectMapper objectMapper; /** SQL identifier whitelist: letters, digits, underscore, dot, hyphen only */ private static final Pattern SAFE_IDENTIFIER = Pattern.compile("^[a-zA-Z0-9_][a-zA-Z0-9_.\\-]{0,127}$"); @@ -67,22 +77,25 @@ public class DatasourceTool { } } - private String listDatasources() { + private String listDatasources() throws Exception { List list = datasourceService.listEnabled(); - JSONArray arr = new JSONArray(); + List> rows = new ArrayList<>(); for (DatasourceEntity ds : list) { - JSONObject obj = new JSONObject(); - obj.set("id", ds.getId()); - obj.set("name", ds.getName()); - obj.set("dbType", ds.getDbType()); - obj.set("databaseName", ds.getDatabaseName()); - obj.set("description", ds.getDescription()); - arr.add(obj); + Map obj = new LinkedHashMap<>(); + // ds.getId() is a Long; the shared ObjectMapper renders it as a JSON + // string so the model copies an exact id back into list_tables / + // execute_sql / describe_table calls. + obj.put("id", ds.getId()); + obj.put("name", ds.getName()); + obj.put("dbType", ds.getDbType()); + obj.put("databaseName", ds.getDatabaseName()); + obj.put("description", ds.getDescription()); + rows.add(obj); } - JSONObject result = new JSONObject(); - result.set("datasources", arr); - result.set("count", arr.size()); - return result.toStringPretty(); + Map result = new LinkedHashMap<>(); + result.put("datasources", rows); + result.put("count", rows.size()); + return objectMapper.writerWithDefaultPrettyPrinter().writeValueAsString(result); } private String listTables(Long datasourceId) throws SQLException { diff --git a/mateclaw-server/src/test/java/vip/mate/tool/builtin/CronJobToolIdPrecisionTest.java b/mateclaw-server/src/test/java/vip/mate/tool/builtin/CronJobToolIdPrecisionTest.java new file mode 100644 index 00000000..3c175c76 --- /dev/null +++ b/mateclaw-server/src/test/java/vip/mate/tool/builtin/CronJobToolIdPrecisionTest.java @@ -0,0 +1,55 @@ +package vip.mate.tool.builtin; + +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.json.JsonMapper; +import com.fasterxml.jackson.databind.module.SimpleModule; +import com.fasterxml.jackson.databind.ser.std.ToStringSerializer; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import vip.mate.cron.model.CronJobDTO; +import vip.mate.cron.service.CronJobService; + +import java.util.List; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * Issue #319 (same class as the datasource fix): a 19-digit Snowflake {@code jobId} + * must reach the model as a JSON string, not a number — otherwise it loses its low + * digits when the model copies it back into toggle_cron_job / delete_cron_job and + * the wrong (or no) job is hit. + */ +class CronJobToolIdPrecisionTest { + + private static ObjectMapper idSafeMapper() { + SimpleModule m = new SimpleModule(); + m.addSerializer(Long.class, ToStringSerializer.instance); + m.addSerializer(Long.TYPE, ToStringSerializer.instance); + return JsonMapper.builder().addModule(m).build(); + } + + @Test + @DisplayName("list_cron_jobs emits jobId as a quoted JSON string, never a bare number") + void listCronJobs_jobIdIsString() { + long bigId = 2064875200729235458L; + CronJobDTO job = new CronJobDTO(); + job.setId(bigId); + job.setName("Daily summary"); + job.setEnabled(true); + + CronJobService service = mock(CronJobService.class); + when(service.list(any())).thenReturn(List.of(job)); + CronJobTool tool = new CronJobTool(service, idSafeMapper()); + + String out = tool.list_cron_jobs(null); + + assertTrue(out.contains("\"" + bigId + "\""), + "jobId must appear as a quoted string so its 19 digits survive; got: " + out); + assertFalse(out.contains(": " + bigId) || out.contains(":" + bigId), + "jobId must NOT appear as a bare JSON number"); + } +} diff --git a/mateclaw-server/src/test/java/vip/mate/tool/builtin/DatasourceToolIdPrecisionTest.java b/mateclaw-server/src/test/java/vip/mate/tool/builtin/DatasourceToolIdPrecisionTest.java new file mode 100644 index 00000000..ca20f78a --- /dev/null +++ b/mateclaw-server/src/test/java/vip/mate/tool/builtin/DatasourceToolIdPrecisionTest.java @@ -0,0 +1,59 @@ +package vip.mate.tool.builtin; + +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.json.JsonMapper; +import com.fasterxml.jackson.databind.ser.std.ToStringSerializer; +import com.fasterxml.jackson.databind.module.SimpleModule; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import vip.mate.datasource.model.DatasourceEntity; +import vip.mate.datasource.service.DatasourceConnectionManager; +import vip.mate.datasource.service.DatasourceService; + +import java.util.List; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * Issue #319: a 19-digit Snowflake datasource id must reach the model as a JSON + * string, not a number. As a number it loses its low digits when it round-trips + * through a double / JS Number on the way back into a follow-up tool call (and in + * the chat UI), so the looked-up datasource is "not found". The tool serializes + * through the application ObjectMapper, which renders every Long as a string — + * the same id-safety policy the HTTP API already applies. + */ +class DatasourceToolIdPrecisionTest { + + /** Mirrors the application ObjectMapper: every Long serializes as a string. */ + private static ObjectMapper idSafeMapper() { + SimpleModule m = new SimpleModule(); + m.addSerializer(Long.class, ToStringSerializer.instance); + m.addSerializer(Long.TYPE, ToStringSerializer.instance); + return JsonMapper.builder().addModule(m).build(); + } + + @Test + @DisplayName("list_datasources emits the id as a quoted JSON string, never a bare number") + void listDatasources_idIsString() { + long bigId = 2064875200729235458L; + DatasourceEntity ds = new DatasourceEntity(); + ds.setId(bigId); + ds.setName("prod-mysql"); + ds.setDbType("mysql"); + ds.setDatabaseName("app"); + + DatasourceService service = mock(DatasourceService.class); + when(service.listEnabled()).thenReturn(List.of(ds)); + DatasourceTool tool = new DatasourceTool(service, mock(DatasourceConnectionManager.class), idSafeMapper()); + + String out = tool.query_datasource("list_datasources", null, null); + + assertTrue(out.contains("\"" + bigId + "\""), + "id must appear as a quoted string so its 19 digits survive the round-trip; got: " + out); + assertFalse(out.contains(": " + bigId) || out.contains(":" + bigId), + "id must NOT appear as a bare JSON number (precision-lossy across double/JS Number)"); + } +}