feat(skill): generate runtime skills within active sessions#219
Merged
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds session-scoped runtime skill support: SkillRegistry gains per-session overlays and runtime registration; DynamicSkillGenerator creates, registers, and persists session-only draft skills from repeated tool sequences; StreamingAgentHandler, SkillTool, and related tools are updated to pass sessionId, use session-aware descriptions/lookups, and invoke generation during post-turn learning. Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as Agent/SelfImprovement
participant Gen as DynamicSkillGenerator
participant LLM as LLM Client
participant Reg as SkillRegistry
participant Disk as Draft Persistence
Agent->>Gen: maybeGenerate(sessionId, projectPath, turn, history, insights, sessionToolNames)
Note over Gen: analyze turn for repeated tool sequence\ncheck disallowed tools & cap
alt eligible (sequence detected & allowed & < 3)
Gen->>LLM: proposeDraft(systemPrompt, toolSequence)
LLM-->>Gen: draft JSON (name, description, body)
Gen->>Gen: sanitize & resolve unique runtime name
Gen->>Reg: registerRuntime(sessionId, SkillConfig)
Reg-->>Gen: registration result
Gen->>Disk: persistDrafts(sessionId, projectPath)
Disk-->>Gen: file written
Gen-->>Agent: Optional.of(skill)
else not eligible
Gen-->>Agent: Optional.empty()
end
sequenceDiagram
participant Client as Client Request
participant Handler as StreamingAgentHandler
participant Reg as SkillRegistry
participant Tool as SkillTool
Client->>Handler: incoming request(sessionId)
Handler->>Reg: formatDescriptions(sessionId)
Reg-->>Handler: merged disk + runtime descriptions
Handler->>Tool: forRequest(sessionId, eventHandler)
Tool->>Reg: names(sessionId)
Reg-->>Tool: session-visible names
Client->>Handler: invoke skillName
Handler->>Tool: execute(skillName)
Tool->>Reg: get(sessionId, skillName)
Reg-->>Tool: SkillConfig (runtime or disk)
Tool-->>Handler: execution result
Handler-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan Review Summary by QodoGenerate runtime skills within active sessions with session-scoped visibility WalkthroughsDescription• Add session-scoped runtime skill overlay support to skill registry • Generate FORK-mode runtime skills from repeated tool sequence patterns • Persist generated runtime skills as drafts on session end • Refresh skill descriptions per request for dynamic visibility Diagramflowchart LR
A["Turn with<br/>Repeated Sequence"] -->|"insights"| B["DynamicSkillGenerator"]
B -->|"maybeGenerate"| C["Runtime Skill<br/>Created"]
C -->|"registerRuntime"| D["SkillRegistry<br/>Session Overlay"]
D -->|"names/get<br/>with sessionId"| E["SkillTool"]
E -->|"execute"| F["Skill Invoked<br/>in Session"]
G["Session End"] -->|"persistDrafts"| H["Draft Files<br/>in .aceclaw/skills-drafts"]
D -->|"clearRuntime"| I["Session Cleanup"]
File Changes1. aceclaw-core/src/main/java/dev/aceclaw/core/agent/SkillRegistry.java
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
aceclaw-tools/src/main/java/dev/aceclaw/tools/SkillTool.java (1)
40-76:⚠️ Potential issue | 🔴 CriticalDon't store session context on the shared
SkillToolinstance.
AceClawDaemonregisters oneSkillTool, andStreamingAgentHandlermutates this field per request. Two overlapping sessions can overwritecurrentSessionIdbetweeninputSchema()andexecute(), which lets one session see or invoke another session's runtime skills and can also route sub-agent events through the wrong handler. Make the skill tool request-scoped in the permission-aware registry, or pass session context down the call path instead of keeping it on the singleton.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-tools/src/main/java/dev/aceclaw/tools/SkillTool.java` around lines 40 - 76, SkillTool stores mutable request-specific state (currentSessionId, currentHandler) on a shared instance causing cross-request leaks; fix by removing those fields and either making SkillTool request-scoped in the permission-aware SkillRegistry or by threading the request context (sessionId and StreamEventHandler) through the call path: stop using setCurrentSessionId/setCurrentHandler on the singleton and instead (a) create a per-request SkillTool (or a RequestContext wrapper) inside StreamingAgentHandler/AceClawDaemon when resolving skills from SkillRegistry, or (b) change APIs that use currentSessionId/currentHandler (e.g. inputSchema(), execute(), and where SubAgentRunner is invoked) to accept sessionId and StreamEventHandler as parameters so runtime-scoped skills and sub-agent event routing use the request-local values.
🧹 Nitpick comments (1)
aceclaw-daemon/src/main/java/dev/aceclaw/daemon/DynamicSkillGenerator.java (1)
437-445: Null-guard the record's list field.Current call sites pass a value, but the compact constructor still turns a future null into an NPE inside
List.copyOf(...). The repo guideline asks records to normalize nullable lists up front.♻️ Proposed fix
private RuntimeSkillRecord { - allowedTools = List.copyOf(allowedTools); + allowedTools = allowedTools != null ? List.copyOf(allowedTools) : List.of(); }As per coding guidelines, "Always null-guard
Listfields in record constructors:signals = signals != null ? List.copyOf(signals) : List.of()."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/DynamicSkillGenerator.java` around lines 437 - 445, RuntimeSkillRecord's compact constructor calls List.copyOf(allowedTools) which will NPE if allowedTools is null; modify the compact constructor in RuntimeSkillRecord to null-guard allowedTools (e.g., assign allowedTools = allowedTools != null ? List.copyOf(allowedTools) : List.of()) so the record normalizes nullable list inputs safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@aceclaw-core/src/main/java/dev/aceclaw/core/agent/SkillRegistry.java`:
- Around line 159-165: The all(String sessionId) method currently uses
putIfAbsent which preserves disk-backed SkillConfig entries even when a runtime
skill with the same name exists (causing mismatch with get(sessionId, name)
which prefers runtime); update all to allow runtime entries to overwrite disk
ones by replacing the putIfAbsent call with a direct put from runtime into
combined so the overlay formatting matches lookup (or alternatively, if you
prefer rejecting collisions, implement collision detection inside
registerRuntime(sessionId, name, skill) and throw/log there); adjust the logic
in all(String) and/or registerRuntime(...) so both lookup (get) and listing
(all) are consistent.
In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/DynamicSkillGenerator.java`:
- Around line 133-159: The code removes the session state from sessionStates
before performing disk I/O, which loses the only in-memory copy on failure and
allows races with maybeGenerate and clearRuntime; change the logic in
DynamicSkillGenerator so you retrieve (but do not remove) the state from
sessionStates (use sessionStates.get(sessionId) or peek), set a "closing" flag
on the state (or call state.markClosing()) while synchronized to prevent new
generation, then perform the directory/create/write/move operations inside the
synchronized block; only after all writes succeed remove the state from
sessionStates and then call skillRegistry.clearRuntime(sessionId); ensure
synchronization uses the same state object and that maybeGenerate checks the
closing flag so it won't create a fresh state while flush is in progress.
- Around line 105-108: The SkillConfig is constructed with draft.argumentHint(),
but the markdown serializer/renderer that persists the draft never includes this
field so the persisted draft loses its invocation hint; update the draft
serialization path in DynamicSkillGenerator (the markdown renderer/serializer
used when saving drafts) to include argumentHint (draft.argumentHint()) in the
rendered output so the persisted draft round-trips the same metadata as the
runtime SkillConfig; ensure the serializer that produces the persisted markdown
(the method responsible for converting a draft to markdown/serialized form)
writes the argument hint and that the deserializer/loader restores it into
SkillConfig on load.
- Around line 79-93: The code currently checks
hasRepeatedSequenceInsight(insights) but then dedupes the current turn via
extractToolSequence before matching, causing distinct ordered repeats (e.g.,
search->open->search) to be lost; update DynamicSkillGenerator so you first keep
the full ordered tool sequence returned by
extractToolSequence(turn.newMessages()) and use that ordered sequence (not a
deduped list) when matching against hasRepeatedSequenceInsight and when
computing the runtime-skill duplicate signature via signatureOf; only after
those checks derive the deduped/allowed list by calling filterAllowedTools on
the ordered sequence to produce allowedTools used for skill generation. Apply
the same change at the other occurrence (the block around methods/lines
referenced by the reviewer, e.g., the later block at ~294-305) so insight
matching and duplicate detection always use the full ordered sequence while
allowed-tools are derived separately.
- Around line 183-193: The fallbackDraft is returned/registered verbatim and may
contain disallowed tool mentions; ensure you run the same post-generation
validation/sanitization used for LLM drafts on the fallback before returning or
persisting it. In generateDraft wrap the fallbackDraft result with the existing
validation/sanitization routine (the same logic you call after proposeDraft) so
that any banned tool names or unsafe content is removed/normalized before
registration; apply the identical change to the other fallback/registration path
in this class that currently uses fallbackDraft so both code paths use the
validated/sanitized draft.
In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/StreamingAgentHandler.java`:
- Around line 594-597: The current fire-and-forget call to
dynamicSkillGenerator.maybeGenerate(sessionIdRef, projectPathRef, turnRef,
historyRef, insights) should be removed from the fast-path and moved into a
synchronized post-request hook invoked by both executePlannedPrompt and
executeResumedPlan; implement a per-session generation queue (or executor) that
enqueues generation tasks instead of launching untracked work, ensure the
post-request hook drains that per-session queue before calling
persistDrafts(sessionId, ...) and before session teardown, and add proper
lifecycle hooks to the UDS listener/shutdown sequence to wait for these drained
tasks (pay attention to thread safety and virtual-thread usage when interacting
with dynamicSkillGenerator and session state).
---
Outside diff comments:
In `@aceclaw-tools/src/main/java/dev/aceclaw/tools/SkillTool.java`:
- Around line 40-76: SkillTool stores mutable request-specific state
(currentSessionId, currentHandler) on a shared instance causing cross-request
leaks; fix by removing those fields and either making SkillTool request-scoped
in the permission-aware SkillRegistry or by threading the request context
(sessionId and StreamEventHandler) through the call path: stop using
setCurrentSessionId/setCurrentHandler on the singleton and instead (a) create a
per-request SkillTool (or a RequestContext wrapper) inside
StreamingAgentHandler/AceClawDaemon when resolving skills from SkillRegistry, or
(b) change APIs that use currentSessionId/currentHandler (e.g. inputSchema(),
execute(), and where SubAgentRunner is invoked) to accept sessionId and
StreamEventHandler as parameters so runtime-scoped skills and sub-agent event
routing use the request-local values.
---
Nitpick comments:
In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/DynamicSkillGenerator.java`:
- Around line 437-445: RuntimeSkillRecord's compact constructor calls
List.copyOf(allowedTools) which will NPE if allowedTools is null; modify the
compact constructor in RuntimeSkillRecord to null-guard allowedTools (e.g.,
assign allowedTools = allowedTools != null ? List.copyOf(allowedTools) :
List.of()) so the record normalizes nullable list inputs safely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 87a4eda5-eeb5-4bac-b687-1f7dda60720f
📒 Files selected for processing (8)
aceclaw-core/src/main/java/dev/aceclaw/core/agent/SkillRegistry.javaaceclaw-core/src/test/java/dev/aceclaw/core/agent/SkillRegistryTest.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/DynamicSkillGenerator.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/StreamingAgentHandler.javaaceclaw-daemon/src/test/java/dev/aceclaw/daemon/DynamicSkillGeneratorTest.javaaceclaw-tools/src/main/java/dev/aceclaw/tools/SkillTool.javaaceclaw-tools/src/test/java/dev/aceclaw/tools/SkillToolTest.java
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
aceclaw-daemon/src/main/java/dev/aceclaw/daemon/StreamingAgentHandler.java (1)
578-581:⚠️ Potential issue | 🟠 MajorMove dynamic-skill generation out of this fire-and-forget hook.
This is the only place in the handler that invokes
maybeGenerate(), so successfulexecutePlannedPrompt()andexecuteResumedPlan()paths never register runtime skills. Launching it on an untracked virtual thread also makes the next request observe stale skill descriptions if prompt assembly wins the race. Put generation behind a common post-request hook that covers direct, planned, and resumed flows and has a defined completion point. As per coding guidelines,aceclaw-daemon/**: Check UDS listener lifecycle, shutdown ordering, and thread safety with virtual threads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/StreamingAgentHandler.java` around lines 578 - 581, The dynamic-skill generation call in StreamingAgentHandler (dynamicSkillGenerator.maybeGenerate(...)) must be removed from this fire-and-forget hook and moved into a single post-request completion hook that is invoked by all request exit paths (direct, executePlannedPrompt, executeResumedPlan) so skill registration happens deterministically; implement the hook (e.g., onRequestComplete or postRequestFinalize) to call dynamicSkillGenerator.maybeGenerate(sessionIdRef, projectPathRef, turnRef, historyRef, insights, requestToolNames) and await its completion (no untracked virtual thread spawn), ensure the hook is called synchronously or its future joined before responding, and audit UDS listener lifecycle, shutdown ordering, and thread-safety around dynamicSkillGenerator use per aceclaw-daemon guidelines.
🧹 Nitpick comments (1)
aceclaw-core/src/main/java/dev/aceclaw/core/agent/SkillRegistry.java (1)
139-168: Keep runtime-skill ordering deterministic.Because the overlay is backed by
ConcurrentHashMap,names(sessionId),all(sessionId), and the derivedformatDescriptions(sessionId)expose runtime skills in hash iteration order. Since that text is injected into tool schemas and prompts, the same session can advertise a different ordering after rehashes or additional registrations. Sorting on read, or storing the overlay in an ordered map, would make prompt assembly stable.🤖 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/SkillRegistry.java` around lines 139 - 168, The runtime overlay uses a ConcurrentHashMap so iteration order is non-deterministic; fix names(String) and all(String) (and any callers like formatDescriptions(sessionId)) by deterministically merging runtime entries: when you retrieve var runtime = runtimeRegistry.get(sessionId), if runtime is non-null and non-empty, iterate over its keys/entries in a stable order (e.g., runtime.keySet().stream().sorted() or runtime.entrySet().stream().sorted(Comparator.comparing(Map.Entry::getKey))) and add them into the LinkedHashSet/LinkedHashMap respectively rather than using runtime.keySet() or runtime.forEach directly; this guarantees stable ordering without changing the runtimeRegistry type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/StreamingAgentHandler.java`:
- Around line 1296-1303: The constructor currently sets
skillDescriptionsProvider to a non-null Function but doesn't guard against that
function returning null; ensure any invocation yields a non-null String by
wrapping the provided Function (in the StreamingAgentHandler constructor) with a
null-coalescing wrapper that calls the original Function and returns "" when the
result is null, so downstream code (e.g., where the provider is used during
request assembly) always receives a non-null string; update the constructor
assignment of skillDescriptionsProvider to use this wrapper and add a quick unit
test or comment noting the non-null guarantee.
---
Duplicate comments:
In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/StreamingAgentHandler.java`:
- Around line 578-581: The dynamic-skill generation call in
StreamingAgentHandler (dynamicSkillGenerator.maybeGenerate(...)) must be removed
from this fire-and-forget hook and moved into a single post-request completion
hook that is invoked by all request exit paths (direct, executePlannedPrompt,
executeResumedPlan) so skill registration happens deterministically; implement
the hook (e.g., onRequestComplete or postRequestFinalize) to call
dynamicSkillGenerator.maybeGenerate(sessionIdRef, projectPathRef, turnRef,
historyRef, insights, requestToolNames) and await its completion (no untracked
virtual thread spawn), ensure the hook is called synchronously or its future
joined before responding, and audit UDS listener lifecycle, shutdown ordering,
and thread-safety around dynamicSkillGenerator use per aceclaw-daemon
guidelines.
---
Nitpick comments:
In `@aceclaw-core/src/main/java/dev/aceclaw/core/agent/SkillRegistry.java`:
- Around line 139-168: The runtime overlay uses a ConcurrentHashMap so iteration
order is non-deterministic; fix names(String) and all(String) (and any callers
like formatDescriptions(sessionId)) by deterministically merging runtime
entries: when you retrieve var runtime = runtimeRegistry.get(sessionId), if
runtime is non-null and non-empty, iterate over its keys/entries in a stable
order (e.g., runtime.keySet().stream().sorted() or
runtime.entrySet().stream().sorted(Comparator.comparing(Map.Entry::getKey))) and
add them into the LinkedHashSet/LinkedHashMap respectively rather than using
runtime.keySet() or runtime.forEach directly; this guarantees stable ordering
without changing the runtimeRegistry type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1e10f21c-d041-4ac1-93e0-3c5b570ab288
📒 Files selected for processing (7)
aceclaw-core/src/main/java/dev/aceclaw/core/agent/SkillRegistry.javaaceclaw-core/src/test/java/dev/aceclaw/core/agent/SkillRegistryTest.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/DynamicSkillGenerator.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/StreamingAgentHandler.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferCheckTool.javaaceclaw-daemon/src/test/java/dev/aceclaw/daemon/DynamicSkillGeneratorTest.javaaceclaw-tools/src/main/java/dev/aceclaw/tools/SkillTool.java
🚧 Files skipped from review as they are similar to previous changes (1)
- aceclaw-daemon/src/main/java/dev/aceclaw/daemon/DynamicSkillGenerator.java
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
aceclaw-daemon/src/main/java/dev/aceclaw/daemon/StreamingAgentHandler.java (1)
1534-1541:⚠️ Potential issue | 🟠 MajorAvoid dropping in-flight post-processing before teardown.
clearSessionMetrics()removessessionPostProcessingon Line 1540. If cleanup calls this beforeawaitSessionPostProcessing(), waiting becomes a no-op and late runtime-skill draft work can be skipped during session close.As per coding guidelines, `aceclaw-daemon/**`: Check UDS listener lifecycle, shutdown ordering, and thread safety with virtual threads.Suggested fix
public void clearSessionMetrics(String sessionId) { java.util.Objects.requireNonNull(sessionId, "sessionId"); + awaitSessionPostProcessing(sessionId); sessionMetrics.remove(sessionId); sessionInjectedCandidateIds.remove(sessionId); sessionDoomLoops.remove(sessionId); sessionProgressDetectors.remove(sessionId); sessionPostProcessing.remove(sessionId); }Also applies to: 1543-1553
♻️ Duplicate comments (1)
aceclaw-daemon/src/main/java/dev/aceclaw/daemon/StreamingAgentHandler.java (1)
1357-1359:⚠️ Potential issue | 🟡 MinorCoalesce null results from
skillDescriptionsProvider.The function reference is null-guarded, but its return value is not. A custom provider can still return
null, which is forwarded at Line 1644.As per coding guidelines, `**/*.java`: Check for null return values before using methods that may return null.Suggested fix
- this.skillDescriptionsProvider = skillDescriptionsProvider != null - ? skillDescriptionsProvider : ignored -> ""; + this.skillDescriptionsProvider = skillDescriptionsProvider != null + ? sessionId -> { + var value = skillDescriptionsProvider.apply(sessionId); + return value != null ? value : ""; + } + : ignored -> "";Also applies to: 1644-1644
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/StreamingAgentHandler.java` around lines 1357 - 1359, The assigned skillDescriptionsProvider may return null even though the reference is non-null; ensure callers never receive null by coalescing its return value to an empty string either by wrapping the provider in the constructor (store a lambda that calls the original and returns result == null ? "" : result) or by null-checking the result at use sites (e.g., where skillDescriptionsProvider.apply(...) is invoked in StreamingAgentHandler, referenced at the call around line 1644) so that skillDescriptionsProvider.apply(...) always yields a non-null String; update the constructor or the call site to use Objects.requireNonNullElse(value, "") or equivalent.
🧹 Nitpick comments (1)
aceclaw-daemon/src/main/java/dev/aceclaw/daemon/DynamicSkillGenerator.java (1)
509-511: Null-guardallowedToolsinRuntimeSkillRecordconstructor.The record constructor copies
allowedToolsbut does not null-guard it. Align this with the repo’s record-constructor rule.♻️ Suggested patch
private RuntimeSkillRecord { - allowedTools = List.copyOf(allowedTools); + allowedTools = allowedTools != null ? List.copyOf(allowedTools) : List.of(); }As per coding guidelines, "Always null-guard
Listfields in record constructors:signals = signals != null ? List.copyOf(signals) : List.of()".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/DynamicSkillGenerator.java` around lines 509 - 511, The record constructor for RuntimeSkillRecord copies allowedTools without a null check; update the compact constructor so allowedTools is null-guarded (e.g., set allowedTools = allowedTools != null ? List.copyOf(allowedTools) : List.of()) to ensure the field is never null and follows the repo rule for List fields in record constructors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.java`:
- Around line 550-554: DynamicSkillGenerator wiring and session-end persistence
are wrongly guarded by the memoryStore null check, disabling runtime-skill
features when memoryStore fails; move the construction of DynamicSkillGenerator
and the call to agentHandler.setDynamicSkillGenerator(dynamicSkillGenerator) out
of the if (memoryStore != null) block so the generator is always created (using
only llmClient, agentHandler::getModelForSession, skillRegistry), and separately
keep only the persistence-related subscription/handlers (the session-end
persistence logic) inside the memoryStore null-guard so that persistence is
skipped when memoryStore is unavailable but runtime-skill generation still
functions.
In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/DynamicSkillGenerator.java`:
- Around line 204-210: hasRepeatedSequenceInsight currently only checks pattern
type and signature match; update it to also require the insight's occurrence
count be >= 3 so weak repeated-sequence insights are ignored. Inside
hasRepeatedSequenceInsight, after casting to Insight.PatternInsight (the stream
branch using Insight.PatternInsight.class::cast), add a check that the insight's
occurrence count (e.g., insight.occurrenceCount() or the existing
property/method that returns how many times the sequence was seen) is >= 3
before matching signature; keep using signatureOf(toolSequence) and
extractInsightSequenceSignature(insight.description()) for the signature
comparison and ensure you reference PatternType.REPEATED_TOOL_SEQUENCE as
already done.
In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/StreamingAgentHandler.java`:
- Around line 1549-1553: The current catch-all around future.get(30,
TimeUnit.SECONDS) masks InterruptedException and conflates different failures;
update the try/catch to explicitly catch InterruptedException (call
Thread.currentThread().interrupt() and log that the wait was interrupted for
sessionId), TimeoutException (log the timeout for sessionId), and
ExecutionException (log the execution failure and include e.getCause()/message
for sessionId), rather than catching Exception; reference the existing
future.get call and sessionId/log.warn usage so you adjust the exception
handling in StreamingAgentHandler around the post-request learning wait
accordingly.
---
Duplicate comments:
In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/StreamingAgentHandler.java`:
- Around line 1357-1359: The assigned skillDescriptionsProvider may return null
even though the reference is non-null; ensure callers never receive null by
coalescing its return value to an empty string either by wrapping the provider
in the constructor (store a lambda that calls the original and returns result ==
null ? "" : result) or by null-checking the result at use sites (e.g., where
skillDescriptionsProvider.apply(...) is invoked in StreamingAgentHandler,
referenced at the call around line 1644) so that
skillDescriptionsProvider.apply(...) always yields a non-null String; update the
constructor or the call site to use Objects.requireNonNullElse(value, "") or
equivalent.
---
Nitpick comments:
In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/DynamicSkillGenerator.java`:
- Around line 509-511: The record constructor for RuntimeSkillRecord copies
allowedTools without a null check; update the compact constructor so
allowedTools is null-guarded (e.g., set allowedTools = allowedTools != null ?
List.copyOf(allowedTools) : List.of()) to ensure the field is never null and
follows the repo rule for List fields in record constructors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: db7eb7cf-b67f-4b5e-97fb-fe49d63d638a
📒 Files selected for processing (6)
aceclaw-core/src/main/java/dev/aceclaw/core/planner/PlanExecutionResult.javaaceclaw-core/src/main/java/dev/aceclaw/core/planner/SequentialPlanExecutor.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/DynamicSkillGenerator.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/StreamingAgentHandler.javaaceclaw-daemon/src/test/java/dev/aceclaw/daemon/DynamicSkillGeneratorTest.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
Closes #132
Summary by CodeRabbit
New Features
Tests