fix+test: code quality improvements + 87 unit tests
Fixes: - Resource leaks: process cleanup in BashTool, GrepTool, NotificationService - Input validation: null/blank checks in BashTool, FileEditTool, GrepTool - Path traversal: FileEditTool blocks ../ escape from workDir - Thread safety: AgentLoop messageHistory now synchronizedList - Error handling: log instead of silently swallow exceptions - Bounds validation: RateLimiter constructor validates all params Tests (87 total): - TokenEstimationServiceTest: 14 tests (estimation, cost, formatting) - RateLimiterTest: 12 tests (limits, cooldown, reset, concurrent) - InternalLoggerTest: 12 tests (levels, structured, file output, export) - NotificationServiceTest: 6 tests (config, disabled mode) - FileEditToolTest: 8 tests (validation, traversal, core edit) - BashToolTest: 9 tests (validation, dangerous commands, metadata) - GrepToolTest: 5 tests (validation, metadata) - Phase4CommandsTest: 21 tests (all Phase 4B+4D command metadata) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
@@ -73,7 +73,7 @@ public class AgentLoop {
|
||||
private volatile boolean cancelled = false;
|
||||
|
||||
/** 消息历史 —— 自行管理,不依赖 Spring AI ChatMemory */
|
||||
private final List<Message> messageHistory = new ArrayList<>();
|
||||
private final List<Message> messageHistory = java.util.Collections.synchronizedList(new ArrayList<>());
|
||||
|
||||
/** 工具调用事件回调:在每次工具调用前/后通知 UI */
|
||||
private Consumer<ToolEvent> onToolEvent;
|
||||
@@ -371,7 +371,9 @@ public class AgentLoop {
|
||||
Map<String, Object> parsedArgs = Map.of();
|
||||
try {
|
||||
parsedArgs = MAPPER.readValue(toolArgs, Map.class);
|
||||
} catch (Exception ignored) {}
|
||||
} catch (Exception e) {
|
||||
log.debug("Failed to parse tool arguments for {}: {}", toolName, e.getMessage());
|
||||
}
|
||||
|
||||
// PreToolUse Hook
|
||||
var preHookCtx = new HookManager.HookContext(toolName, parsedArgs);
|
||||
|
||||
@@ -116,16 +116,19 @@ public class NotificationService {
|
||||
"$n.ShowBalloonTip(3000,'%s','%s','Info');" +
|
||||
"Start-Sleep 1;$n.Dispose()",
|
||||
escape(title), escape(message));
|
||||
new ProcessBuilder("powershell", "-NoProfile", "-Command", ps)
|
||||
Process p = new ProcessBuilder("powershell", "-NoProfile", "-Command", ps)
|
||||
.redirectErrorStream(true).start();
|
||||
// Don't block, but schedule cleanup
|
||||
p.onExit().thenRun(p::destroyForcibly);
|
||||
}
|
||||
|
||||
private void sendMac(String title, String message) throws IOException {
|
||||
String script = String.format(
|
||||
"display notification \"%s\" with title \"%s\"",
|
||||
escape(message), escape(title));
|
||||
new ProcessBuilder("osascript", "-e", script)
|
||||
Process p = new ProcessBuilder("osascript", "-e", script)
|
||||
.redirectErrorStream(true).start();
|
||||
p.onExit().thenRun(p::destroyForcibly);
|
||||
}
|
||||
|
||||
private void sendLinux(String title, String message, String level) throws IOException {
|
||||
@@ -134,8 +137,9 @@ public class NotificationService {
|
||||
case "warning" -> "normal";
|
||||
default -> "low";
|
||||
};
|
||||
new ProcessBuilder("notify-send", "-u", urgency, title, message)
|
||||
Process p = new ProcessBuilder("notify-send", "-u", urgency, title, message)
|
||||
.redirectErrorStream(true).start();
|
||||
p.onExit().thenRun(p::destroyForcibly);
|
||||
}
|
||||
|
||||
private String escape(String s) {
|
||||
|
||||
@@ -62,6 +62,15 @@ public class RateLimiter {
|
||||
* @param maxConcurrent 最大并发执行数
|
||||
*/
|
||||
public RateLimiter(int maxRequestsPerWindow, Duration windowDuration, int maxConcurrent) {
|
||||
if (maxRequestsPerWindow <= 0) {
|
||||
throw new IllegalArgumentException("maxRequestsPerWindow must be > 0, got: " + maxRequestsPerWindow);
|
||||
}
|
||||
if (windowDuration == null || windowDuration.isNegative() || windowDuration.isZero()) {
|
||||
throw new IllegalArgumentException("windowDuration must be positive");
|
||||
}
|
||||
if (maxConcurrent <= 0) {
|
||||
throw new IllegalArgumentException("maxConcurrent must be > 0, got: " + maxConcurrent);
|
||||
}
|
||||
this.maxRequestsPerWindow = maxRequestsPerWindow;
|
||||
this.windowDuration = windowDuration;
|
||||
this.maxConcurrent = maxConcurrent;
|
||||
|
||||
@@ -151,9 +151,17 @@ public class BashTool implements Tool {
|
||||
@Override
|
||||
public String execute(Map<String, Object> input, ToolContext context) {
|
||||
String command = (String) input.get("command");
|
||||
int timeout = input.containsKey("timeout")
|
||||
? ((Number) input.get("timeout")).intValue()
|
||||
: DEFAULT_TIMEOUT;
|
||||
if (command == null || command.isBlank()) {
|
||||
return "Error: 'command' parameter is required and must not be empty.";
|
||||
}
|
||||
int timeout;
|
||||
try {
|
||||
timeout = input.containsKey("timeout")
|
||||
? ((Number) input.get("timeout")).intValue()
|
||||
: DEFAULT_TIMEOUT;
|
||||
} catch (ClassCastException e) {
|
||||
timeout = DEFAULT_TIMEOUT;
|
||||
}
|
||||
Path workDir = context.getWorkDir();
|
||||
|
||||
// Sandbox check: block absolutely dangerous commands
|
||||
@@ -261,14 +269,17 @@ public class BashTool implements Tool {
|
||||
|
||||
/** 检查命令是否可用 */
|
||||
private static boolean isCommandAvailable(String... cmd) {
|
||||
Process p = null;
|
||||
try {
|
||||
Process p = new ProcessBuilder(cmd)
|
||||
p = new ProcessBuilder(cmd)
|
||||
.redirectErrorStream(true)
|
||||
.start();
|
||||
p.getInputStream().readAllBytes();
|
||||
return p.waitFor(5, TimeUnit.SECONDS) && p.exitValue() == 0;
|
||||
} catch (Exception e) {
|
||||
return false;
|
||||
} finally {
|
||||
if (p != null) p.destroyForcibly();
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -68,8 +68,24 @@ public class FileEditTool implements Tool {
|
||||
String filePath = (String) input.get("file_path");
|
||||
String oldString = (String) input.get("old_string");
|
||||
String newString = (String) input.get("new_string");
|
||||
|
||||
if (filePath == null || filePath.isBlank()) {
|
||||
return "Error: 'file_path' is required.";
|
||||
}
|
||||
if (oldString == null) {
|
||||
return "Error: 'old_string' is required.";
|
||||
}
|
||||
if (newString == null) {
|
||||
return "Error: 'new_string' is required.";
|
||||
}
|
||||
|
||||
Path path = context.getWorkDir().resolve(filePath).normalize();
|
||||
|
||||
// Path traversal protection
|
||||
if (!path.startsWith(context.getWorkDir().normalize())) {
|
||||
return "Error: Path traversal not allowed. Path must be within the working directory.";
|
||||
}
|
||||
|
||||
if (!Files.exists(path)) {
|
||||
return "Error: File not found: " + path;
|
||||
}
|
||||
|
||||
@@ -107,6 +107,9 @@ public class GrepTool implements Tool {
|
||||
@Override
|
||||
public String execute(Map<String, Object> input, ToolContext context) {
|
||||
String pattern = (String) input.get("pattern");
|
||||
if (pattern == null || pattern.isBlank()) {
|
||||
return "Error: 'pattern' parameter is required.";
|
||||
}
|
||||
String searchPath = (String) input.getOrDefault("path", ".");
|
||||
String include = (String) input.getOrDefault("include", null);
|
||||
String type = (String) input.getOrDefault("type", null);
|
||||
@@ -135,10 +138,12 @@ public class GrepTool implements Tool {
|
||||
while ((line = reader.readLine()) != null && lines.size() < headLimit) {
|
||||
lines.add(line);
|
||||
}
|
||||
} finally {
|
||||
if (!process.waitFor(30, TimeUnit.SECONDS)) {
|
||||
process.destroyForcibly();
|
||||
}
|
||||
}
|
||||
|
||||
process.waitFor(30, TimeUnit.SECONDS);
|
||||
|
||||
if (lines.isEmpty()) {
|
||||
return "No matches found for pattern: " + pattern;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user