diff --git a/mateclaw-server/src/main/java/vip/mate/tool/builtin/SkillScriptTool.java b/mateclaw-server/src/main/java/vip/mate/tool/builtin/SkillScriptTool.java index 378dc165..1f9bb38b 100644 --- a/mateclaw-server/src/main/java/vip/mate/tool/builtin/SkillScriptTool.java +++ b/mateclaw-server/src/main/java/vip/mate/tool/builtin/SkillScriptTool.java @@ -2,6 +2,9 @@ package vip.mate.tool.builtin; import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.annotation.JsonPropertyDescription; +import com.fasterxml.jackson.databind.DeserializationFeature; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.springframework.ai.tool.annotation.Tool; @@ -13,6 +16,7 @@ import vip.mate.skill.runtime.model.ResolvedSkill; import vip.mate.skill.secret.SkillSecretService; import java.nio.file.Path; +import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Map; @@ -30,6 +34,7 @@ public class SkillScriptTool { private final SkillFileAccessPolicy accessPolicy; private final SkillScriptExecutionService executionService; private final SkillSecretService skillSecretService; + private final ObjectMapper objectMapper; @vip.mate.tool.ConcurrencyUnsafe("script execution can have arbitrary side effects on the host process and filesystem") @Tool(description = """ @@ -39,10 +44,12 @@ public class SkillScriptTool { Parameters: - skillName: Name of the skill - scriptPath: Relative path to script under scripts/ directory (e.g., "scripts/run.py") - - args: Optional list of script arguments. Each element is passed as a separate - CLI argument exactly as written — no shell interpretation, no splitting. - For a JSON payload, wrap it as a single-element list, e.g. - ["{\\"date\\":\\"2026-05-12\\",\\"topic\\":\\"meeting\\"}"]. + - args: Optional script arguments, given as ONE JSON-encoded string: + * a JSON array for multiple positional arguments, e.g. ["--verbose","input.txt"]; + * a JSON object when the script expects a single JSON payload — it is + forwarded as one argument, e.g. {"date":"2026-05-19","topic":"meeting"}; + * any other plain text is forwarded verbatim as a single argument. + Pass the JSON object directly — do not wrap it in an array or escape it. Returns: JSON with exitCode, stdout, stderr @@ -59,8 +66,8 @@ public class SkillScriptTool { String scriptPath, @JsonProperty(required = false) - @JsonPropertyDescription("Optional list of script arguments. Each element is passed as one CLI arg verbatim. Wrap a JSON payload as a single-element list.") - List args + @JsonPropertyDescription("Optional script arguments as ONE JSON-encoded string: a JSON array for multiple positional args, a JSON object for a single JSON payload, or plain text for one literal argument.") + String args ) { log.info("Executing skill script: skill={}, script={}, args={}", skillName, scriptPath, args); @@ -81,11 +88,13 @@ public class SkillScriptTool { return formatError("Invalid or unsafe script path: " + scriptPath); } - // Pass args straight through. No splitting — arbitrary delimiters - // (notably commas inside JSON payloads) used to shatter a single - // logical argument into multiple positional args, which broke any - // skill expecting a JSON-encoded payload. - List argList = (args == null || args.isEmpty()) ? null : args; + // Normalize the JSON-encoded args into a positional argument list. + // Taking one JSON string (rather than a raw array) keeps the model + // out of nested-array-of-escaped-JSON territory — the failure mode + // where a JSON payload arrived shattered across array elements or + // type-mismatched, and the receiving script then rejected it as + // malformed JSON. + List argList = normalizeArgs(args); // RFC-091 settings bridge — pull this skill's stored secrets // (e.g. AIRTABLE_API_KEY) and inject them as env vars for the @@ -106,6 +115,66 @@ public class SkillScriptTool { } } + /** + * Decode the JSON-encoded {@code args} string into a positional argument + * list for the subprocess. + * + *
    + *
  • A JSON array becomes one CLI argument per element. Non-string + * elements are re-serialized to compact JSON, so an object the + * model wrapped in a single-element array still reaches the script + * as a JSON payload.
    • + *
  • A JSON object is forwarded as a single argument — its compact + * JSON text — which is what a script reading {@code json.loads(argv[1])} + * expects.
    • + *
  • Anything else (a bare date, topic, number, or malformed JSON) is + * forwarded verbatim as one literal argument. A bare scalar is never + * JSON-decoded: that would mangle e.g. {@code 2026-05-19} into + * {@code 2026}.
    • + *
+ * + *

Package-private for direct unit testing of the decode rules. + * + * @param args the raw {@code args} tool parameter, may be {@code null} + * @return the positional argument list, or {@code null} when empty + */ + List normalizeArgs(String args) { + if (args == null) { + return null; + } + String trimmed = args.trim(); + if (trimmed.isEmpty()) { + return null; + } + // Only decode when the text clearly intends JSON structure. The + // lead-char gate keeps a plain argument that merely looks numeric + // (a date, a version string) from being parsed and truncated. + char lead = trimmed.charAt(0); + if (lead == '[' || lead == '{') { + try { + JsonNode node = objectMapper.reader() + .with(DeserializationFeature.FAIL_ON_TRAILING_TOKENS) + .readTree(trimmed); + if (node != null && node.isArray()) { + List out = new ArrayList<>(node.size()); + for (JsonNode el : node) { + out.add(el.isTextual() ? el.asText() : el.toString()); + } + return out.isEmpty() ? null : out; + } + if (node != null && node.isObject()) { + return List.of(node.toString()); + } + } catch (Exception e) { + // Looked like JSON but didn't parse — forward it unchanged so + // the script reports its own input error rather than us + // silently reshaping a malformed payload. + log.debug("runSkillScript: args not valid JSON, forwarding verbatim: {}", e.getMessage()); + } + } + return List.of(trimmed); + } + private String formatResult(SkillScriptExecutionService.ScriptResult result) { return String.format( "{\n \"exitCode\": %d,\n \"stdout\": %s,\n \"stderr\": %s\n}", diff --git a/mateclaw-server/src/test/java/vip/mate/tool/builtin/SkillScriptToolArgsTest.java b/mateclaw-server/src/test/java/vip/mate/tool/builtin/SkillScriptToolArgsTest.java new file mode 100644 index 00000000..642d38d3 --- /dev/null +++ b/mateclaw-server/src/test/java/vip/mate/tool/builtin/SkillScriptToolArgsTest.java @@ -0,0 +1,90 @@ +package vip.mate.tool.builtin; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Unit tests for {@link SkillScriptTool#normalizeArgs(String)} — the decode + * step that turns the JSON-encoded {@code args} tool parameter into the + * positional argument list handed to a skill script. + * + *

The decisive cases are the ones a model gets wrong when a script needs a + * JSON payload: the object passed directly, the object wrapped in an array, + * and the object pre-escaped into an array of one string all have to converge + * on the same single JSON argument. A bare scalar must survive untouched — + * decoding {@code 2026-05-19} would otherwise truncate it to {@code 2026}. + */ +class SkillScriptToolArgsTest { + + private final ObjectMapper objectMapper = new ObjectMapper(); + + /** Unused collaborators are null — {@code normalizeArgs} only needs the mapper. */ + private final SkillScriptTool tool = + new SkillScriptTool(null, null, null, null, objectMapper); + + @Test + @DisplayName("null / blank args yield no argument list") + void emptyInputs() { + assertThat(tool.normalizeArgs(null)).isNull(); + assertThat(tool.normalizeArgs("")).isNull(); + assertThat(tool.normalizeArgs(" ")).isNull(); + assertThat(tool.normalizeArgs("[]")).isNull(); + } + + @Test + @DisplayName("a JSON object is forwarded as a single JSON argument") + void objectBecomesOneArg() throws Exception { + List args = tool.normalizeArgs("{\"date\":\"2026-05-19\",\"topic\":\"智能体\"}"); + assertThat(args).hasSize(1); + JsonNode parsed = objectMapper.readTree(args.get(0)); + assertThat(parsed.get("date").asText()).isEqualTo("2026-05-19"); + assertThat(parsed.get("topic").asText()).isEqualTo("智能体"); + } + + @Test + @DisplayName("an object wrapped in a single-element array still reaches the script as JSON") + void objectWrappedInArray() throws Exception { + List args = tool.normalizeArgs("[{\"date\":\"x\"}]"); + assertThat(args).hasSize(1); + assertThat(objectMapper.readTree(args.get(0)).get("date").asText()).isEqualTo("x"); + } + + @Test + @DisplayName("an object pre-escaped into an array of one string is unwrapped") + void objectPreEscapedInArray() throws Exception { + List args = tool.normalizeArgs("[\"{\\\"date\\\":\\\"x\\\"}\"]"); + assertThat(args).hasSize(1); + assertThat(objectMapper.readTree(args.get(0)).get("date").asText()).isEqualTo("x"); + } + + @Test + @DisplayName("a plain JSON array maps to one positional argument per element") + void plainArrayKeepsElements() { + assertThat(tool.normalizeArgs("[\"--verbose\",\"input.txt\"]")) + .containsExactly("--verbose", "input.txt"); + assertThat(tool.normalizeArgs("[1,2,3]")) + .containsExactly("1", "2", "3"); + } + + @Test + @DisplayName("a bare scalar is forwarded verbatim, never JSON-decoded") + void bareScalarUntouched() { + // Decoding would truncate this to "2026" — it must survive intact. + assertThat(tool.normalizeArgs("2026-05-19")).containsExactly("2026-05-19"); + assertThat(tool.normalizeArgs("智能体")).containsExactly("智能体"); + assertThat(tool.normalizeArgs(" hello world ")).containsExactly("hello world"); + } + + @Test + @DisplayName("text that looks like JSON but does not parse is forwarded verbatim") + void malformedJsonForwardedVerbatim() { + assertThat(tool.normalizeArgs("{bad json")).containsExactly("{bad json"); + assertThat(tool.normalizeArgs("[1,2")).containsExactly("[1,2"); + } +}