feat(memory): add Insight type hierarchy, DetectedPattern model, and ToolMetricsCollector (#13)

[xinhuagu]

Summary

• Sealed Insight interface with ErrorInsight, SuccessInsight, PatternInsight variants
• PatternType enum, DetectedPattern record with validation and toInsight() conversion
• Thread-safe ToolMetricsCollector wired into AgentLoopConfig and StreamingAgentLoop

Test plan

• InsightTest — 13 tests (sealed exhaustiveness, targetCategory, confidence bounds, defensive copies)
• DetectedPatternTest — 8 tests (validation, immutability, toInsight conversion)
• ToolMetricsCollectorTest — 10 tests (single/multi tool, concurrent access, derived methods)
• Full build passes (./gradlew clean build — 64 tasks, BUILD SUCCESSFUL)
• No regressions in existing daemon tests

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Added tool execution metrics: per-tool invocation counts, success/error rates, and timing; non-blocking recording integrated into agent loop.
  • Added pattern detection for recurring agent behaviors and a memory insights framework capturing error-recovery and successful workflow insights.

• Tests

  • Added comprehensive tests for metrics collection, pattern detection, insight types, immutability, validation, and concurrency.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

---

📝 Walkthrough

Walkthrough

Adds per-tool metrics collection and memory insight types: a thread-safe ToolMetricsCollector and ToolMetrics record; integrates an optional metricsCollector into AgentLoopConfig and records metrics in StreamingAgentLoop; introduces PatternType, DetectedPattern, and a sealed Insight hierarchy with unit tests for metrics and memory types.

Changes

Cohort / File(s)	Summary
Metrics types & collector aceclaw-core/src/main/java/dev/aceclaw/core/agent/ToolMetrics.java, aceclaw-core/src/main/java/dev/aceclaw/core/agent/ToolMetricsCollector.java	Add immutable ToolMetrics record and a thread-safe ToolMetricsCollector with record, getMetrics, and allMetrics APIs; uses atomic counters and returns snapshot ToolMetrics.
Agent loop integration aceclaw-core/src/main/java/dev/aceclaw/core/agent/AgentLoopConfig.java, aceclaw-core/src/main/java/dev/aceclaw/core/agent/StreamingAgentLoop.java	Add optional ToolMetricsCollector metricsCollector to AgentLoopConfig and instrument StreamingAgentLoop to record per-tool metrics (success/failure and duration) with a guarded helper that logs on collector failures.
Memory insight types aceclaw-memory/src/main/java/dev/aceclaw/memory/PatternType.java, aceclaw-memory/src/main/java/dev/aceclaw/memory/DetectedPattern.java, aceclaw-memory/src/main/java/dev/aceclaw/memory/Insight.java	Introduce PatternType enum, DetectedPattern record with validation and toInsight() conversion, and a sealed Insight interface with ErrorInsight, SuccessInsight, and PatternInsight implementations (validation, tags, category mapping).
Tests aceclaw-core/src/test/java/dev/aceclaw/core/agent/ToolMetricsCollectorTest.java, aceclaw-memory/src/test/java/dev/aceclaw/memory/DetectedPatternTest.java, aceclaw-memory/src/test/java/dev/aceclaw/memory/InsightTest.java	Add comprehensive unit tests for ToolMetricsCollector (including concurrency) and for DetectedPattern/Insight validation, immutability, derived values, and category/tag behaviour.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant Loop as StreamingAgentLoop
participant Tool as Tool (executor)
participant Config as AgentLoopConfig
participant Collector as ToolMetricsCollector
Loop->>Tool: execute tool
Tool-->>Loop: result (success/failure) + durationMs
Loop->>Config: access metricsCollector
alt metricsCollector present
Config->>Collector: record(toolName, success, durationMs)
end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

• Insight type hierarchy and DetectedPattern model #13: Implements Insight/DetectedPattern/PatternType and adds ToolMetrics/ToolMetricsCollector plus streaming-loop instrumentation referenced by the issue.

Possibly related PRs

• feat(core): integrate EventBus, permissions, and auto-memory into agent loop #5: Continues the AgentLoopConfig and StreamingAgentLoop instrumentation work by adding a ToolMetricsCollector field and recording per-tool metrics in the same execution path.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 17.24% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main changes: introducing Insight type hierarchy, DetectedPattern model, and ToolMetricsCollector. It's specific, clear, and directly reflects the primary work across multiple files.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feature/13-insight-types

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (2)

aceclaw-core/src/main/java/dev/aceclaw/core/agent/StreamingAgentLoop.java (2)

456-458: Consider defensive wrapping for metrics recording.

If metricsCollector.record() throws unexpectedly (e.g., due to a bug or resource exhaustion), it would propagate up and fail the tool result return. Since metrics are non-critical, consider wrapping in a try-catch to ensure tool execution isn't affected by metrics failures.

Proposed defensive wrapper

             if (config.metricsCollector() != null) {
-                config.metricsCollector().record(tool.name(), !result.isError(), toolDuration);
+                try {
+                    config.metricsCollector().record(tool.name(), !result.isError(), toolDuration);
+                } catch (Exception e) {
+                    log.warn("Failed to record tool metrics for {}: {}", tool.name(), e.getMessage());
+                }
             }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@aceclaw-core/src/main/java/dev/aceclaw/core/agent/StreamingAgentLoop.java`
around lines 456 - 458, In StreamingAgentLoop, the call to
config.metricsCollector().record(tool.name(), !result.isError(), toolDuration)
should be made defensive: wrap that invocation in a try-catch so any thrown
exception from metrics collection does not propagate and affect returning the
tool result; on catch, log the exception using the class logger (or existing
logging mechanism) with context (tool.name(), toolDuration) and continue normal
flow so metrics failures remain non-critical.

---

465-467: Same defensive consideration applies here.

The metrics recording in the exception path has the same potential issue. If you apply defensive wrapping to the success path, apply it here as well for consistency.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@aceclaw-core/src/main/java/dev/aceclaw/core/agent/StreamingAgentLoop.java`
around lines 465 - 467, In StreamingAgentLoop, the metrics recording in the
exception path should mirror the defensive handling used in the success path:
check config.metricsCollector() for null and guard the call to
config.metricsCollector().record(tool.name(), false, toolDuration) inside the
same safe wrapper (or a try/catch) so that exceptions from the metrics collector
don't propagate; update the exception-path block that references
config.metricsCollector(), record(), tool.name(), and toolDuration to perform
the null check and handle any thrown errors consistently with the success-path
implementation.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@aceclaw-core/src/main/java/dev/aceclaw/core/agent/StreamingAgentLoop.java`:
- Around line 456-458: In StreamingAgentLoop, the call to
config.metricsCollector().record(tool.name(), !result.isError(), toolDuration)
should be made defensive: wrap that invocation in a try-catch so any thrown
exception from metrics collection does not propagate and affect returning the
tool result; on catch, log the exception using the class logger (or existing
logging mechanism) with context (tool.name(), toolDuration) and continue normal
flow so metrics failures remain non-critical.
- Around line 465-467: In StreamingAgentLoop, the metrics recording in the
exception path should mirror the defensive handling used in the success path:
check config.metricsCollector() for null and guard the call to
config.metricsCollector().record(tool.name(), false, toolDuration) inside the
same safe wrapper (or a try/catch) so that exceptions from the metrics collector
don't propagate; update the exception-path block that references
config.metricsCollector(), record(), tool.name(), and toolDuration to perform
the null check and handle any thrown errors consistently with the success-path
implementation.