feat: implement dual-channel architecture for concurrent task streaming (#48)#49
Merged
Conversation
…ng (#48) Each agent task now runs on its own DaemonConnection and virtual thread, enabling concurrent prompts and background task management. The CLI maintains the same synchronous UX by default while adding /tasks, /bg, /fg, and /cancel slash commands for multi-task workflows. Key changes: - DaemonConnection: wraps single UDS with write lock + own line buffer - DaemonClient: delegates to controlConnection, openTaskConnection() - TaskManager + TaskHandle: concurrent task lifecycle management - PermissionBridge: inter-thread permission relay via CompletableFuture - OutputSink + ForegroundOutputSink + BackgroundOutputBuffer: extracted streaming rendering from TerminalRepl into composable sinks - TaskStreamReader: virtual-thread stream reader per task - TerminalRepl: major refactor for task-based concurrent I/O - Ctrl+C now cancels foreground task only, background tasks unaffected
|
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:
📝 WalkthroughWalkthroughAdds a multi-connection JSON-RPC client and task subsystem: per-connection DaemonConnection, control/task channels in DaemonClient, an OutputSink abstraction with foreground renderer and background buffer, TaskManager/TaskHandle/TaskStreamReader for concurrent tasks, and PermissionBridge to mediate REPL-thread permission prompts. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TerminalRepl
participant TaskManager
participant TaskStreamReader
participant DaemonConnection
participant Daemon
participant OutputSink
User->>TerminalRepl: Submit prompt
TerminalRepl->>TaskManager: submit(prompt,...)
TaskManager->>TaskStreamReader: start (virtual thread)
TaskStreamReader->>DaemonConnection: sendRequest(agent.prompt, prompt)
DaemonConnection->>Daemon: JSON-RPC request
Daemon-->>DaemonConnection: streaming notifications
loop notifications
DaemonConnection-->>TaskStreamReader: JSON message
TaskStreamReader->>OutputSink: onThinkingDelta/onTextDelta/onToolUse...
OutputSink->>TerminalRepl: render or buffer events
end
Daemon-->>DaemonConnection: final response
TaskStreamReader->>TaskManager: task complete
TaskManager->>OutputSink: onTurnComplete
OutputSink->>TerminalRepl: final render
sequenceDiagram
participant TaskThread as TaskStreamReader
participant PermissionBridge
participant MainThread as TerminalRepl
participant User
participant DaemonConnection
TaskThread->>DaemonConnection: receive permission.request
TaskThread->>PermissionBridge: requestPermission(request)
PermissionBridge->>PermissionBridge: enqueue + future
PermissionBridge-->>TaskThread: block awaiting answer
MainThread->>PermissionBridge: pollPending()/drainPermissions()
MainThread->>User: show permission prompt
User->>MainThread: respond
MainThread->>PermissionBridge: submitAnswer(id, answer)
PermissionBridge-->>TaskThread: resolve future with answer
TaskThread->>DaemonConnection: sendNotification(permission.response, answer)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryImplements dual-channel architecture enabling concurrent agent task streaming without wire corruption. Each task runs on its own Key changes:
Architecture improvements:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User as User (REPL)
participant TM as TaskManager
participant TC as Task Connection
participant PB as PermissionBridge
participant TS as TaskStreamReader (Virtual Thread)
participant D as Daemon
User->>TM: submit(prompt, connection, sink)
TM->>TC: open new DaemonConnection
TM->>TS: start virtual thread
activate TS
TS->>D: agent.prompt request
D-->>TS: stream.text notifications
TS->>User: render via ForegroundOutputSink
D-->>TS: permission.request
TS->>PB: requestPermission() [blocks]
User->>PB: pollPending(50ms)
PB-->>User: PermissionRequest
User->>User: prompt for y/n/a
User->>PB: submitAnswer(approved)
PB-->>TS: unblock with answer
TS->>D: permission.response
D-->>TS: stream.tool_use
TS->>User: render tool spinner
D-->>TS: final JSON-RPC response
TS->>User: onTurnComplete()
deactivate TS
TM->>TC: close connection
Last reviewed commit: 3790411 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (6)
aceclaw-cli/src/main/java/dev/aceclaw/cli/DaemonClient.java (1)
48-51: Consider adding null validation forsocketPath.While
DaemonConnection.connect()validatessocketPath, adding validation here provides clearer error messages at the point of construction.As per coding guidelines: "Use
Objects.requireNonNull(param, "param")on parameters... passed to downstream calls."🛡️ Optional: add defensive null check
public DaemonClient(Path socketPath) { - this.socketPath = socketPath; + this.socketPath = Objects.requireNonNull(socketPath, "socketPath"); this.objectMapper = new ObjectMapper(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/DaemonClient.java` around lines 48 - 51, Add a defensive null check in the DaemonClient constructor: validate the socketPath parameter with Objects.requireNonNull(socketPath, "socketPath") (or similar) at the start of the constructor so the field assignment this.socketPath = socketPath happens only after validation; keep the existing ObjectMapper initialization unchanged and ensure the constructor throws a clear NPE if socketPath is null.aceclaw-cli/src/main/java/dev/aceclaw/cli/DaemonConnection.java (2)
203-246: Same recursive call issue in timed variant.Lines 210-212 and 237-239 have the same recursive call pattern that could cause stack overflow with many empty lines. Consider converting to an iterative loop similar to the suggested fix for the blocking
readLine().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/DaemonConnection.java` around lines 203 - 246, The timed readLine(long timeoutMs) method currently uses recursive calls when encountering empty lines (via the readLine(...) call after trimming), which can overflow the stack; replace those recursive calls with an iterative loop that keeps reading until a non-empty trimmed line is returned or the deadline expires, reusing the existing deadline calculation and lineBuffer handling; ensure you preserve the non-blocking channel configuration and sleep/retry behavior, and mirror the iterative approach used in the blocking readLine() path so the method returns null on timeout or EOF rather than recursing.
164-194: Recursive call on empty lines could cause stack overflow.The recursive
readLine()call on line 169 (when trimmed line is empty) could cause a stack overflow if the daemon sends many consecutive empty lines or newline-only data.♻️ Suggested iterative approach
public String readLine() throws IOException { - int newlineIdx = lineBuffer.indexOf("\n"); - if (newlineIdx != -1) { - var line = lineBuffer.substring(0, newlineIdx).trim(); - lineBuffer.delete(0, newlineIdx + 1); - return line.isEmpty() ? readLine() : line; - } - - var buffer = ByteBuffer.allocate(BUFFER_SIZE); - while (true) { - buffer.clear(); - int bytesRead = channel.read(buffer); - if (bytesRead == -1) { - return lineBuffer.isEmpty() ? null : lineBuffer.toString().trim(); - } - if (bytesRead == 0) continue; - - buffer.flip(); - String chunk = StandardCharsets.UTF_8.decode(buffer).toString(); - lineBuffer.append(chunk); - - newlineIdx = lineBuffer.indexOf("\n"); - if (newlineIdx != -1) { - var line = lineBuffer.substring(0, newlineIdx).trim(); - lineBuffer.delete(0, newlineIdx + 1); - if (!line.isEmpty()) { - return line; - } + var buffer = ByteBuffer.allocate(BUFFER_SIZE); + while (true) { + int newlineIdx = lineBuffer.indexOf("\n"); + if (newlineIdx != -1) { + var line = lineBuffer.substring(0, newlineIdx).trim(); + lineBuffer.delete(0, newlineIdx + 1); + if (!line.isEmpty()) { + return line; + } + continue; // Skip empty lines iteratively + } + + buffer.clear(); + int bytesRead = channel.read(buffer); + if (bytesRead == -1) { + return lineBuffer.isEmpty() ? null : lineBuffer.toString().trim(); } + if (bytesRead == 0) continue; + + buffer.flip(); + String chunk = StandardCharsets.UTF_8.decode(buffer).toString(); + lineBuffer.append(chunk); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/DaemonConnection.java` around lines 164 - 194, In DaemonConnection.readLine() the recursive call when a trimmed line is empty can cause stack overflow for many consecutive empty lines; replace recursion with an iterative loop: whenever you consume an empty trimmed line (both in the initial newlineIdx branch and after reading a chunk), do not call readLine() recursively but continue the outer processing loop to search for the next non-empty line, preserving the existing behavior for end-of-stream (return null or remaining trimmed buffer) and using the same lineBuffer, BUFFER_SIZE, and channel handling in readLine().aceclaw-cli/src/main/java/dev/aceclaw/cli/PermissionBridge.java (1)
82-89: Consider logging when no matching future is found for a permission answer.If
submitAnsweris called with arequestIdthat doesn't exist in the futures map (e.g., due to a race condition or bug), the answer is silently dropped. A debug log would help troubleshoot such cases.🔍 Add debug logging
+import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + public final class PermissionBridge { + + private static final Logger log = LoggerFactory.getLogger(PermissionBridge.class); // ... existing code ... public void submitAnswer(String requestId, PermissionAnswer answer) { Objects.requireNonNull(requestId, "requestId"); Objects.requireNonNull(answer, "answer"); var future = futures.get(requestId); if (future != null) { future.complete(answer); + } else { + log.debug("No pending permission request for id: {}", requestId); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/PermissionBridge.java` around lines 82 - 89, The submitAnswer method currently drops answers when futures.get(requestId) returns null; add a debug log in PermissionBridge.submitAnswer that logs the missing future along with the requestId and answer (and optionally current futures.keySet() size) so dropped answers are visible during debugging; use the class logger (e.g., PermissionBridge logger) and log at debug level before returning, leaving the existing complete(answer) behavior unchanged when future is non-null.aceclaw-cli/src/main/java/dev/aceclaw/cli/TaskStreamReader.java (1)
158-184: EmptyrequestIdcould cause issues in permission bridge.Line 163 uses
params.path("requestId").asText("")which returns an empty string if the field is missing. WhilePermissionBridge.PermissionRequestaccepts this (empty string ≠ null), using empty string as a map key infuturescould lead to collisions or confusion.Consider validating that
requestIdis non-empty before proceeding, or logging a warning.🛡️ Validate non-empty requestId
private void handlePermissionRequest(JsonNode params) { if (params == null) return; String tool = params.path("tool").asText("unknown"); String description = params.path("description").asText(""); String requestId = params.path("requestId").asText(""); + + if (requestId.isEmpty()) { + log.warn("Task {}: permission request missing requestId, ignoring", handle.taskId()); + return; + } var request = new PermissionBridge.PermissionRequest( handle.taskId(), tool, description, requestId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/TaskStreamReader.java` around lines 158 - 184, In handlePermissionRequest, validate that the extracted requestId is non-empty before constructing PermissionBridge.PermissionRequest; if requestId is blank, log a warning (including handle.taskId() and the offending params) and return early instead of creating the request or calling permissionBridge.requestPermission, to avoid using an empty-string key in the PermissionBridge.futures map and prevent collisions; keep the existing InterruptedException/IOException handling for the normal path (references: handlePermissionRequest, PermissionBridge.PermissionRequest, permissionBridge.requestPermission, connection.sendNotification).aceclaw-cli/src/main/java/dev/aceclaw/cli/ForegroundOutputSink.java (1)
194-203: SynchronizestopSpinnerto keep terminal writes atomic
stopSpinner()is public and prints viaspinner.clear(); if invoked without the lock, output can interleave with other writers.Proposed fix
public void stopSpinner() { - var s = spinner; - if (s != null && s.isSpinning()) { - s.clear(); - spinner = null; - } + synchronized (lock) { + var s = spinner; + if (s != null && s.isSpinning()) { + s.clear(); + spinner = null; + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/ForegroundOutputSink.java` around lines 194 - 203, stopSpinner currently manipulates the spinner and calls spinner.clear() without holding the terminal write lock, allowing output to interleave; fix by acquiring the same lock used for terminal writes (the class' lock/writeLock) when executing stopSpinner: inside synchronized(lock) (or make the method synchronized if that matches the class convention) check spinner != null && spinner.isSpinning(), call spinner.clear(), and set spinner = null before releasing the lock so spinner.clear() and the spinner field update are atomic with other writers.
🤖 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-cli/src/main/java/dev/aceclaw/cli/BackgroundOutputBuffer.java`:
- Around line 5-7: BackgroundOutputBuffer.replay forwards the sink parameter
directly to downstream calls and can cause a late NPE; add an upfront null-guard
using Objects.requireNonNull(sink, "sink") at the start of replay (and likewise
any other methods in the class that immediately forward or call .equals() on
sink, e.g., the methods around lines 60-73) so the NPE occurs immediately with a
clear message. Ensure java.util.Objects is imported if not already and place the
requireNonNull check as the first statement in the method(s).
In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/ForegroundOutputSink.java`:
- Around line 27-31: The scanner currently inspects only each incoming delta and
flips inCodeFence when it sees ``` within that chunk, which desyncs when fences
span chunk boundaries; update the chunk-processing logic to detect fence markers
that may be split across chunks by preserving a small trailing context (e.g., up
to 2 chars) or a pendingFence state between calls, prepend that context to the
next delta before scanning, and toggle inCodeFence only when a full ``` sequence
is observed; ensure the same stateful detection is used wherever the current
single-chunk scan runs (the code that manipulates inCodeFence and flushes
textBuffer / receivedTextOutput / wasThinking) so paragraph flushes do not occur
mid-code-block.
- Around line 145-153: The code in ForegroundOutputSink uses
error.get("code").asInt() and error.get("message").asText() which can NPE if
fields are missing; update the handling of JsonNode error in the message
processing to guard against missing fields by using
error.path("code").asInt(<fallback>) and
error.path("message").asText(<fallback>) or check
error.hasNonNull("code")/hasNonNull("message") before calling asInt()/asText(),
then use those safe values when printing (i.e., keep the special check for code
== -32601 but compare against the safe int and print a fallback error message
when message is absent).
In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/TaskHandle.java`:
- Around line 1-7: The review notes that if you add Objects.requireNonNull calls
for null validation, you must import java.util.Objects; update the TaskHandle
class to include the import for java.util.Objects alongside the existing imports
so any new Objects.requireNonNull(...) usages in methods or constructors (e.g.,
in TaskHandle or related constructors/methods) compile correctly.
- Around line 41-49: The TaskHandle constructor must validate non-null inputs:
add Objects.requireNonNull(taskId, "taskId") and
Objects.requireNonNull(connection, "connection") at the start of the constructor
in class TaskHandle to prevent NPEs when taskId is later compared (e.g.,
handle.taskId().equals(...)) and when connection is used downstream; keep the
existing promptSummary truncation logic afterward.
In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/TaskManager.java`:
- Around line 53-61: The submit method currently forwards several parameters to
downstream components (new TaskHandle(...) and new TaskStreamReader(...))
without null checks; add Objects.requireNonNull(...) guards at the start of
submit for prompt, connection, sessionId, outputSink, and permissionBridge (each
with a descriptive message like "prompt" or "connection") so any null is caught
early before creating TaskHandle or invoking the TaskStreamReader constructor;
update imports if necessary to reference java.util.Objects.requireNonNull.
In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/TerminalRepl.java`:
- Around line 599-618: handleBgCommand creates a BackgroundOutputBuffer but
never attaches it to the running task, so output keeps going to the original
ForegroundOutputSink; either document this limitation or fix it by making the
sink swappable: add a mutable wrapper sink (e.g., MutableOutputSink) used by
TaskStreamReader instead of a direct ForegroundOutputSink, then in
handleBgCommand swap the wrapper's delegate to the new BackgroundOutputBuffer
(or call a TaskStreamReader.setSink(bgBuffer) if you add that setter) and keep
taskManager.clearForeground() and activeForegroundSink = null; update
TaskStreamReader and any consumers to use the mutable wrapper so backgrounding
actually reroutes output.
- Around line 660-672: TerminalRepl creates a new ForegroundOutputSink but
doesn't rewire the running TaskStreamReader, so the reader keeps writing to its
original sink; fix by making the reader/scheduler swap its sink when
foregrounding: add or use an API on TaskStreamReader (e.g.,
setOutputSink(OutputSink)) or change taskManager.setForeground to accept the new
sink and internally call the reader's setter, then in TerminalRepl after
creating ForegroundOutputSink call that setter (or
taskManager.setForeground(taskId, fgSink)), and on taskManager.clearForeground
restore the reader's previous/original sink and set activeForegroundSink = null
so streaming output is actually redirected while in foreground.
- Around line 387-403: notifyCompletedBackgroundTasks computes stateLabel for
finished background tasks but never uses it; either remove the dead code or,
preferably, implement the notification: when stateLabel is non-empty call
out.println(...) (or out.printf) to print a concise message including
handle.taskId() (or handle.name() if available) and stateLabel, and ensure you
still skip the foreground task via taskManager.foregroundTaskId(); keep the
looping over taskManager.list() and the state switch as-is and add any necessary
deduplication if multiple prints are a concern (e.g., mark handles as notified
or filter by a "notified" property on handle).
---
Nitpick comments:
In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/DaemonClient.java`:
- Around line 48-51: Add a defensive null check in the DaemonClient constructor:
validate the socketPath parameter with Objects.requireNonNull(socketPath,
"socketPath") (or similar) at the start of the constructor so the field
assignment this.socketPath = socketPath happens only after validation; keep the
existing ObjectMapper initialization unchanged and ensure the constructor throws
a clear NPE if socketPath is null.
In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/DaemonConnection.java`:
- Around line 203-246: The timed readLine(long timeoutMs) method currently uses
recursive calls when encountering empty lines (via the readLine(...) call after
trimming), which can overflow the stack; replace those recursive calls with an
iterative loop that keeps reading until a non-empty trimmed line is returned or
the deadline expires, reusing the existing deadline calculation and lineBuffer
handling; ensure you preserve the non-blocking channel configuration and
sleep/retry behavior, and mirror the iterative approach used in the blocking
readLine() path so the method returns null on timeout or EOF rather than
recursing.
- Around line 164-194: In DaemonConnection.readLine() the recursive call when a
trimmed line is empty can cause stack overflow for many consecutive empty lines;
replace recursion with an iterative loop: whenever you consume an empty trimmed
line (both in the initial newlineIdx branch and after reading a chunk), do not
call readLine() recursively but continue the outer processing loop to search for
the next non-empty line, preserving the existing behavior for end-of-stream
(return null or remaining trimmed buffer) and using the same lineBuffer,
BUFFER_SIZE, and channel handling in readLine().
In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/ForegroundOutputSink.java`:
- Around line 194-203: stopSpinner currently manipulates the spinner and calls
spinner.clear() without holding the terminal write lock, allowing output to
interleave; fix by acquiring the same lock used for terminal writes (the class'
lock/writeLock) when executing stopSpinner: inside synchronized(lock) (or make
the method synchronized if that matches the class convention) check spinner !=
null && spinner.isSpinning(), call spinner.clear(), and set spinner = null
before releasing the lock so spinner.clear() and the spinner field update are
atomic with other writers.
In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/PermissionBridge.java`:
- Around line 82-89: The submitAnswer method currently drops answers when
futures.get(requestId) returns null; add a debug log in
PermissionBridge.submitAnswer that logs the missing future along with the
requestId and answer (and optionally current futures.keySet() size) so dropped
answers are visible during debugging; use the class logger (e.g.,
PermissionBridge logger) and log at debug level before returning, leaving the
existing complete(answer) behavior unchanged when future is non-null.
In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/TaskStreamReader.java`:
- Around line 158-184: In handlePermissionRequest, validate that the extracted
requestId is non-empty before constructing PermissionBridge.PermissionRequest;
if requestId is blank, log a warning (including handle.taskId() and the
offending params) and return early instead of creating the request or calling
permissionBridge.requestPermission, to avoid using an empty-string key in the
PermissionBridge.futures map and prevent collisions; keep the existing
InterruptedException/IOException handling for the normal path (references:
handlePermissionRequest, PermissionBridge.PermissionRequest,
permissionBridge.requestPermission, connection.sendNotification).
added 2 commits
February 21, 2026 16:50
…ve readLine - TaskHandle: AtomicReference<OutputSink> enables runtime sink swap for /bg and /fg - TaskStreamReader: read sink from handle.outputSink() instead of holding direct ref - TerminalRepl /bg: atomically swap sink to BackgroundOutputBuffer - TerminalRepl /fg: swap to ForegroundOutputSink + replay buffered events - ForegroundOutputSink: fix NPE with error.path() instead of error.get() - ForegroundOutputSink: synchronize stopSpinner() for signal handler safety - ForegroundOutputSink: fix backtick fence tracking across chunk boundaries - DaemonConnection: convert recursive readLine() to iterative loop - BackgroundOutputBuffer: null-guard in replay() - TaskHandle/TaskManager/DaemonClient: null-guards per coding conventions - PermissionBridge: log warning when no matching future found - TaskStreamReader: validate non-empty requestId on permission requests - notifyCompletedBackgroundTasks: actually print notifications instead of dead code
Background tasks now automatically display their buffered output at the next prompt, instead of just showing a bare [bg task #N completed] label. - TaskHandle: add completionNotified flag (AtomicBoolean) with markNotified() - notifyCompletedBackgroundTasks: replay BackgroundOutputBuffer content via a temporary ForegroundOutputSink, then show token usage summary - Each completion is shown exactly once (markNotified uses CAS)
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
aceclaw-cli/src/main/java/dev/aceclaw/cli/TaskStreamReader.java (1)
158-188: Consider replying on permission interrupt to unblock the daemon.If the stream thread is interrupted while waiting for permission, the code logs and returns without sending a
permission.response, which can leave the daemon waiting. Consider sending a default deny (approved=false) or closing the connection on interrupt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/TaskStreamReader.java` around lines 158 - 188, In handlePermissionRequest, when permissionBridge.requestPermission throws InterruptedException you must still send a permission.response to unblock the daemon: build an ObjectNode like in the success path with requestId, approved=false and remember=false, call connection.sendNotification("permission.response", ...) before returning, re-set the thread interrupt (Thread.currentThread().interrupt()), and handle any IOException from sendNotification (log it). This touches handlePermissionRequest, PermissionBridge.PermissionRequest, permissionBridge.requestPermission and connection.sendNotification.
🤖 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-cli/src/main/java/dev/aceclaw/cli/DaemonConnection.java`:
- Around line 278-283: The writeRaw method currently assumes a single
SocketChannel.write(...) call writes the entire ByteBuffer; change it to loop
while the ByteBuffer has remaining() bytes and call channel.write(buf)
repeatedly until fully written to avoid partial writes corrupting the protocol
(caller still holds writeLock). Locate writeRaw and replace the single write
invocation with a loop that repeatedly writes the ByteBuffer returned from
ByteBuffer.wrap(data) (or reuse a local ByteBuffer) until buf.remaining() == 0,
propagating IOException on failure.
- Around line 100-105: The JSON‑RPC error handling block using
responseNode.get("error") can NPE when error fields are missing; update the
logic in DaemonConnection where you create responseNode and throw new
DaemonClient.DaemonClientException to use safe lookups (use error.path("code")
and error.path("message") or check for null before calling asInt()/asText()),
provide sensible defaults or a descriptive message when fields are absent, and
ensure the thrown DaemonClient.DaemonClientException includes those
safe/explained values instead of calling asInt()/asText() on a potentially null
JsonNode.
In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/ForegroundOutputSink.java`:
- Around line 99-129: When clearing code-fence state in ForegroundOutputSink
(e.g., in onToolUse and onStreamError), also reset the backtickRun flag so it
can't carry over to the next delta; locate the blocks that set inCodeFence =
false and receivedTextOutput = false (and call flushMarkdown()) and add
backtickRun = false there (and anywhere else you clear inCodeFence), ensuring
flushMarkdown(), stopSpinner(), and the spinner handling remain unchanged.
In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/PermissionBridge.java`:
- Around line 101-110: PermissionRequest's canonical constructor currently only
null-checks taskId and requestId; add null-guards for the remaining record
components by calling Objects.requireNonNull(tool, "tool") and
Objects.requireNonNull(description, "description") inside the PermissionRequest
canonical constructor (or alternatively assign a default ""), so tool and
description are never null when used in equals/toString or passed downstream.
- Around line 38-47: The requestPermission method uses CompletableFuture.join()
which ignores interrupts and can leak entries in the futures map if
pending.put(request) is interrupted; replace join() with the interruptible
future.get() and ensure the futures.remove(request.requestId()) happens in a
finally that also runs when InterruptedException is thrown (catch
InterruptedException, remove the future, restore interrupt status and rethrow)
so pending futures are always cleaned up; update references in
PermissionBridge.requestPermission to use future.get(), handle
InterruptedException correctly, and keep futures.remove(request.requestId())
guaranteed.
In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/TerminalRepl.java`:
- Around line 252-265: waitForForeground currently blocks the REPL by only
polling permissionBridge.pollPending(50, TimeUnit.MILLISECONDS) and never
reading user input, preventing slash commands like /bg from being processed;
change the design so user input is still read while waiting for the foreground
task—either convert waitForForeground to perform non-blocking/timeout reads from
LineReader (process input lines and route slash commands, calling
taskManager.clearForeground() when /bg is received) or run a dedicated input
thread that uses LineReader to handle commands and calls
taskManager.clearForeground() (and still invokes handlePermissionFromBridge when
permReq != null); ensure interrupt handling and existing permission handling
(handlePermissionFromBridge) remain intact.
- Around line 387-401: notifyCompletedBackgroundTasks is re-printing terminal
tasks every REPL cycle; fix by recording which tasks have already been announced
and only print on the transition to a terminal state. Add a field on
TerminalRepl (e.g., Set<String> notifiedCompletedTaskIds) or a
Map<String,TaskState> lastSeenState; in notifyCompletedBackgroundTasks iterate
taskManager.list(), skip any handle.taskId() already in the notified set (or
only print when lastSeenState shows a non-terminal→terminal transition), and
when you print for a task call notifiedCompletedTaskIds.add(handle.taskId()) (or
update lastSeenState.put(handle.taskId(), handle.state())). Ensure you still
call out.flush() after processing.
- Around line 214-239: The submitAndWait method unconditionally renders
completion and closes the task connection after waitForForeground returns, which
kills backgrounded tasks; change submitAndWait so that after waitForForeground()
it checks the submitted handle.isTerminal() (or equivalent terminal state)
before calling renderTaskCompletion(out, handle), taskManager.clearForeground(),
setting activeForegroundSink = null, and conn.close(); alternatively move
conn.close() into the TaskManager completion callback that runs when the task
truly finishes. Ensure you reference submitAndWait, waitForForeground,
renderTaskCompletion, taskManager.clearForeground, activeForegroundSink,
conn.close, and handle.isTerminal when making the guard or moving connection
cleanup.
---
Nitpick comments:
In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/TaskStreamReader.java`:
- Around line 158-188: In handlePermissionRequest, when
permissionBridge.requestPermission throws InterruptedException you must still
send a permission.response to unblock the daemon: build an ObjectNode like in
the success path with requestId, approved=false and remember=false, call
connection.sendNotification("permission.response", ...) before returning, re-set
the thread interrupt (Thread.currentThread().interrupt()), and handle any
IOException from sendNotification (log it). This touches
handlePermissionRequest, PermissionBridge.PermissionRequest,
permissionBridge.requestPermission and connection.sendNotification.
Comment on lines
+100
to
+105
| JsonNode responseNode = objectMapper.readTree(responseLine); | ||
| if (responseNode.has("error") && !responseNode.get("error").isNull()) { | ||
| JsonNode error = responseNode.get("error"); | ||
| int code = error.get("code").asInt(); | ||
| String message = error.get("message").asText(); | ||
| throw new DaemonClient.DaemonClientException(code, message); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Jackson JsonNode get() method returns null when field missing asText() asInt()
💡 Result:
In Jackson’s tree model:
node.get("field")returnsnullwhen the field is absent, sonode.get("field").asText()/.asInt()will throwNullPointerException. Also note: if the JSON contains"field": null,get()returns aNullNode(notnull). [1]- Use
node.path("field")instead: it returns aMissingNodewhen absent, so you can safely call accessors (they yield “empty/default-ish” values). [1][2]
Examples:
JsonNode root = mapper.readTree(json);
// 1) Safe navigation (no NPE):
String s = root.path("name").asText(); // "" if missing
int n = root.path("age").asInt(); // 0 if missing
// 2) Provide explicit defaults:
String s2 = root.path("name").asText("NA"); // "NA" if missing OR explicit null
int n2 = root.path("age").asInt(42); // 42 if missing OR not convertible
// 3) If you need to distinguish missing vs null:
boolean hasField = root.has("name"); // true even if {"name": null}
boolean hasValue = root.hasNonNull("name"); // false if missing OR nullIf you must use get(), do a null-check first:
JsonNode n = root.get("age");
int age = (n == null) ? 42 : n.asInt(42);Sources: Jackson JsonNode#get() / path() Javadoc [1]; MissingNode behavior [2]; NullNode / asText(defaultValue) notes [3].
[1] (javadoc.io)
[2] (fasterxml.github.io)
[3] (javadoc.io)
Citations:
- 1: https://javadoc.io/static/com.fasterxml.jackson.core/jackson-databind/2.17.0/com/fasterxml/jackson/databind/JsonNode.html?utm_source=openai
- 2: https://fasterxml.github.io/jackson-databind/javadoc/2.4/com/fasterxml/jackson/databind/node/MissingNode.html?utm_source=openai
- 3: https://javadoc.io/static/com.fasterxml.jackson.core/jackson-databind/2.17.2/com/fasterxml/jackson/databind/node/NullNode.html?utm_source=openai
🏁 Script executed:
fd -e java DaemonConnectionRepository: xinhuagu/AceClaw
Length of output: 123
🏁 Script executed:
cat -n aceclaw-cli/src/main/java/dev/aceclaw/cli/DaemonConnection.java | sed -n '90,120p'Repository: xinhuagu/AceClaw
Length of output: 1499
🏁 Script executed:
cat -n aceclaw-cli/src/main/java/dev/aceclaw/cli/DaemonConnection.java | sed -n '1,50p'Repository: xinhuagu/AceClaw
Length of output: 2368
Guard missing error fields to avoid NPEs in JSON‑RPC error handling.
error.get("code") and error.get("message") return null when the daemon omits those fields, causing NullPointerException on .asInt() and .asText(). Use path() instead, which safely returns MissingNode for absent fields.
🔧 Suggested fix
- if (responseNode.has("error") && !responseNode.get("error").isNull()) {
- JsonNode error = responseNode.get("error");
- int code = error.get("code").asInt();
- String message = error.get("message").asText();
+ JsonNode error = responseNode.path("error");
+ if (!error.isMissingNode() && !error.isNull()) {
+ int code = error.path("code").asInt(-1);
+ String message = error.path("message").asText("Unknown error");
throw new DaemonClient.DaemonClientException(code, message);
}Per coding guidelines: Check for null return values before using methods that may return null and throw meaningful exceptions.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| JsonNode responseNode = objectMapper.readTree(responseLine); | |
| if (responseNode.has("error") && !responseNode.get("error").isNull()) { | |
| JsonNode error = responseNode.get("error"); | |
| int code = error.get("code").asInt(); | |
| String message = error.get("message").asText(); | |
| throw new DaemonClient.DaemonClientException(code, message); | |
| JsonNode responseNode = objectMapper.readTree(responseLine); | |
| JsonNode error = responseNode.path("error"); | |
| if (!error.isMissingNode() && !error.isNull()) { | |
| int code = error.path("code").asInt(-1); | |
| String message = error.path("message").asText("Unknown error"); | |
| throw new DaemonClient.DaemonClientException(code, message); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/DaemonConnection.java` around lines
100 - 105, The JSON‑RPC error handling block using responseNode.get("error") can
NPE when error fields are missing; update the logic in DaemonConnection where
you create responseNode and throw new DaemonClient.DaemonClientException to use
safe lookups (use error.path("code") and error.path("message") or check for null
before calling asInt()/asText()), provide sensible defaults or a descriptive
message when fields are absent, and ensure the thrown
DaemonClient.DaemonClientException includes those safe/explained values instead
of calling asInt()/asText() on a potentially null JsonNode.
Comment on lines
+278
to
+283
| /** | ||
| * Writes a line to the socket without locking (caller must hold writeLock). | ||
| */ | ||
| private void writeRaw(String line) throws IOException { | ||
| var data = (line + "\n").getBytes(StandardCharsets.UTF_8); | ||
| channel.write(ByteBuffer.wrap(data)); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Java SocketChannel.write() blocking mode guarantee buffer fully written
💡 Result:
SocketChannel.write(ByteBuffer) does not guarantee that it will write the entire buffer, even in blocking mode.
The spec says the call attempts to write up to src.remaining() bytes, but the actual number written is n where 0 <= n <= r, and it explicitly notes that “some types of channels… may write only some of the bytes” (it only gives non-blocking socket channels as an example of why this can happen, but it does not restrict partial writes to non-blocking mode). In all cases you must check the returned count and the buffer’s position. [1]
So the correct pattern is to loop until the buffer is fully drained:
while (buf.hasRemaining()) {
sc.write(buf);
}This is also the common guidance in NIO examples/tutorials. [1][2]
Practical rules:
- Blocking mode: the call may block, but you still must handle partial writes by looping. [1]
- Non-blocking mode:
write()may write 0 bytes, so you typically wait forOP_WRITEand retry. [1]
Sources: Oracle JDK SocketChannel / WritableByteChannel javadoc [1], Jenkov NIO tutorial example [2].
[1] https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/nio/channels/SocketChannel.html
[2] https://jenkov.com/tutorials/java-nio/socketchannel.html
🏁 Script executed:
# Check the file exists and examine the writeRaw method
find . -name "DaemonConnection.java" -type fRepository: xinhuagu/AceClaw
Length of output: 125
🏁 Script executed:
# Search for SocketChannel.write usage in the codebase
rg "channel\.write" --type javaRepository: xinhuagu/AceClaw
Length of output: 2381
🏁 Script executed:
# Examine the writeRaw method in DaemonConnection.java
sed -n '278,283p' aceclaw-cli/src/main/java/dev/aceclaw/cli/DaemonConnection.javaRepository: xinhuagu/AceClaw
Length of output: 329
🏁 Script executed:
# Also check context around the method
sed -n '270,290p' aceclaw-cli/src/main/java/dev/aceclaw/cli/DaemonConnection.javaRepository: xinhuagu/AceClaw
Length of output: 539
Loop until the entire buffer is written to the SocketChannel.
SocketChannel.write() does not guarantee writing the entire buffer, even in blocking mode. Per the Java SocketChannel specification, it attempts to write up to src.remaining() bytes but may write fewer, returning the actual count written (where 0 ≤ n ≤ r). Without looping, partial JSON writes corrupt the wire protocol.
🔧 Suggested fix
private void writeRaw(String line) throws IOException {
var data = (line + "\n").getBytes(StandardCharsets.UTF_8);
- channel.write(ByteBuffer.wrap(data));
+ ByteBuffer buf = ByteBuffer.wrap(data);
+ while (buf.hasRemaining()) {
+ channel.write(buf);
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/DaemonConnection.java` around lines
278 - 283, The writeRaw method currently assumes a single
SocketChannel.write(...) call writes the entire ByteBuffer; change it to loop
while the ByteBuffer has remaining() bytes and call channel.write(buf)
repeatedly until fully written to avoid partial writes corrupting the protocol
(caller still holds writeLock). Locate writeRaw and replace the single write
invocation with a loop that repeatedly writes the ByteBuffer returned from
ByteBuffer.wrap(data) (or reuse a local ByteBuffer) until buf.remaining() == 0,
propagating IOException on failure.
Comment on lines
+99
to
+129
| @Override | ||
| public void onToolUse(String toolName) { | ||
| synchronized (lock) { | ||
| if (receivedTextOutput) { | ||
| flushMarkdown(); | ||
| inCodeFence = false; | ||
| receivedTextOutput = false; | ||
| } | ||
| if (wasThinking) { | ||
| out.println(); | ||
| wasThinking = false; | ||
| } | ||
| stopSpinner(); | ||
| String verb = TerminalSpinner.verbForTool(toolName); | ||
| spinner = new TerminalSpinner(out); | ||
| spinner.start(verb + " " + toolName + "..."); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void onStreamError(String error) { | ||
| synchronized (lock) { | ||
| if (receivedTextOutput) { | ||
| flushMarkdown(); | ||
| inCodeFence = false; | ||
| receivedTextOutput = false; | ||
| } | ||
| stopSpinner(); | ||
| out.printf("%s[stream error: %s]%s%n", ERROR, error, RESET); | ||
| out.flush(); | ||
| } |
There was a problem hiding this comment.
Reset backtickRun whenever code‑fence state is cleared.
When you flush/reset due to tool use or stream error, inCodeFence is cleared but backtickRun can carry over and toggle fences on the next delta. Reset it alongside inCodeFence.
🔧 Suggested fix
public void onToolUse(String toolName) {
synchronized (lock) {
if (receivedTextOutput) {
flushMarkdown();
inCodeFence = false;
+ backtickRun = 0;
receivedTextOutput = false;
}
@@
public void onStreamError(String error) {
synchronized (lock) {
if (receivedTextOutput) {
flushMarkdown();
inCodeFence = false;
+ backtickRun = 0;
receivedTextOutput = false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @Override | |
| public void onToolUse(String toolName) { | |
| synchronized (lock) { | |
| if (receivedTextOutput) { | |
| flushMarkdown(); | |
| inCodeFence = false; | |
| receivedTextOutput = false; | |
| } | |
| if (wasThinking) { | |
| out.println(); | |
| wasThinking = false; | |
| } | |
| stopSpinner(); | |
| String verb = TerminalSpinner.verbForTool(toolName); | |
| spinner = new TerminalSpinner(out); | |
| spinner.start(verb + " " + toolName + "..."); | |
| } | |
| } | |
| @Override | |
| public void onStreamError(String error) { | |
| synchronized (lock) { | |
| if (receivedTextOutput) { | |
| flushMarkdown(); | |
| inCodeFence = false; | |
| receivedTextOutput = false; | |
| } | |
| stopSpinner(); | |
| out.printf("%s[stream error: %s]%s%n", ERROR, error, RESET); | |
| out.flush(); | |
| } | |
| `@Override` | |
| public void onToolUse(String toolName) { | |
| synchronized (lock) { | |
| if (receivedTextOutput) { | |
| flushMarkdown(); | |
| inCodeFence = false; | |
| backtickRun = 0; | |
| receivedTextOutput = false; | |
| } | |
| if (wasThinking) { | |
| out.println(); | |
| wasThinking = false; | |
| } | |
| stopSpinner(); | |
| String verb = TerminalSpinner.verbForTool(toolName); | |
| spinner = new TerminalSpinner(out); | |
| spinner.start(verb + " " + toolName + "..."); | |
| } | |
| } | |
| `@Override` | |
| public void onStreamError(String error) { | |
| synchronized (lock) { | |
| if (receivedTextOutput) { | |
| flushMarkdown(); | |
| inCodeFence = false; | |
| backtickRun = 0; | |
| receivedTextOutput = false; | |
| } | |
| stopSpinner(); | |
| out.printf("%s[stream error: %s]%s%n", ERROR, error, RESET); | |
| out.flush(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/ForegroundOutputSink.java` around
lines 99 - 129, When clearing code-fence state in ForegroundOutputSink (e.g., in
onToolUse and onStreamError), also reset the backtickRun flag so it can't carry
over to the next delta; locate the blocks that set inCodeFence = false and
receivedTextOutput = false (and call flushMarkdown()) and add backtickRun =
false there (and anywhere else you clear inCodeFence), ensuring flushMarkdown(),
stopSpinner(), and the spinner handling remain unchanged.
Comment on lines
+38
to
+47
| public PermissionAnswer requestPermission(PermissionRequest request) throws InterruptedException { | ||
| Objects.requireNonNull(request, "request"); | ||
| var future = new CompletableFuture<PermissionAnswer>(); | ||
| futures.put(request.requestId(), future); | ||
| pending.put(request); | ||
| try { | ||
| return future.join(); | ||
| } finally { | ||
| futures.remove(request.requestId()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
CompletableFuture join vs get interrupt handling difference
💡 Result:
get()is interruption-aware (interruptible wait). It is specified to throwInterruptedException“if the current thread was interrupted while waiting”. When it throws, the thread’s interrupt status has typically been cleared (standardInterruptedExceptionbehavior), so callers often re-assert it withThread.currentThread().interrupt()if they’re not propagating the exception. [1]join()does not provide interruption handling (non-interruptible wait). It does not declareInterruptedException; instead it just waits and returns (or throws unchecked exceptions for failure). In OpenJDK’s implementation,join()waits via an internal path equivalent towaitingGet(false)(i.e., “not interruptible”), so an interrupt won’t makejoin()stop waiting; the interrupt status is generally left set for later code to observe. [2][3]
Related (often confused with “interrupt handling”):
- Exceptional completion wrapping differs:
get()wraps failures inExecutionException, whilejoin()throwsCompletionException(unchecked) for exceptional completion. [1][3] CompletableFuture.cancel(true)doesn’t interrupt the running computation (the parameter “has no effect” forCompletableFuture), so interruption is about the waiting thread (get/joincaller), not about stopping the async task. [1]
Rule of thumb: use get() when you need cooperative cancellation via thread interruption; use join() when you deliberately don’t want to deal with checked exceptions—then explicitly check/handle the thread’s interrupt status afterward. [1][2]
Sources:
[1] Oracle JDK CompletableFuture Javadoc (Java 25): get() throws InterruptedException; cancel(true) “has no effect… interrupts are not used”; join()/getNow() throw CompletionException directly. (docs.oracle.com)
[2] Stack Overflow discussion showing join() implemented as waitingGet(false) (ignores interrupts). (stackoverflow.com)
[3] Oracle JDK CompletableFuture Javadoc (Java 12): join() throws CompletionException / CancellationException and does not mention InterruptedException. (docs.oracle.com)
Citations:
- 1: https://docs.oracle.com/en/java/javase/25/docs/api/java.base/java/util/concurrent/CompletableFuture.html?utm_source=openai
- 2: https://stackoverflow.com/questions/44231376/how-to-interrupt-completablefuturejoin?utm_source=openai
- 3: https://docs.oracle.com/en/java/javase/12/docs/api/java.base/java/util/concurrent/CompletableFuture.html?utm_source=openai
Make permission waits interruptible and clean up futures on interruption.
CompletableFuture.join() ignores thread interrupts, so an InterruptedException during pending.put() will leak a future in the map, and a cancelled task can hang indefinitely. Replace with get() (which is interruptible) and ensure cleanup on interruption.
🔧 Suggested fix
import java.util.concurrent.BlockingQueue;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ExecutionException;
@@
public PermissionAnswer requestPermission(PermissionRequest request) throws InterruptedException {
Objects.requireNonNull(request, "request");
var future = new CompletableFuture<PermissionAnswer>();
futures.put(request.requestId(), future);
- pending.put(request);
try {
- return future.join();
+ pending.put(request);
+ try {
+ return future.get();
+ } catch (ExecutionException e) {
+ throw new IllegalStateException("Permission request failed", e);
+ }
+ } catch (InterruptedException e) {
+ future.cancel(true);
+ throw e;
} finally {
futures.remove(request.requestId());
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/PermissionBridge.java` around lines
38 - 47, The requestPermission method uses CompletableFuture.join() which
ignores interrupts and can leak entries in the futures map if
pending.put(request) is interrupted; replace join() with the interruptible
future.get() and ensure the futures.remove(request.requestId()) happens in a
finally that also runs when InterruptedException is thrown (catch
InterruptedException, remove the future, restore interrupt status and rethrow)
so pending futures are always cleaned up; update references in
PermissionBridge.requestPermission to use future.get(), handle
InterruptedException correctly, and keep futures.remove(request.requestId())
guaranteed.
Comment on lines
+101
to
+110
| public record PermissionRequest( | ||
| String taskId, | ||
| String tool, | ||
| String description, | ||
| String requestId | ||
| ) { | ||
| public PermissionRequest { | ||
| Objects.requireNonNull(taskId, "taskId"); | ||
| Objects.requireNonNull(requestId, "requestId"); | ||
| } |
There was a problem hiding this comment.
Null‑guard tool and description in PermissionRequest.
These record components flow into equals/toString and downstream display; allowing nulls risks NPEs or "null" output. Add Objects.requireNonNull (or default to empty strings).
🔧 Suggested fix
public PermissionRequest {
Objects.requireNonNull(taskId, "taskId");
+ Objects.requireNonNull(tool, "tool");
+ Objects.requireNonNull(description, "description");
Objects.requireNonNull(requestId, "requestId");
}As per coding guidelines: Use Objects.requireNonNull(param, "param") on parameters used in .equals() or passed to downstream calls.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/PermissionBridge.java` around lines
101 - 110, PermissionRequest's canonical constructor currently only null-checks
taskId and requestId; add null-guards for the remaining record components by
calling Objects.requireNonNull(tool, "tool") and
Objects.requireNonNull(description, "description") inside the PermissionRequest
canonical constructor (or alternatively assign a default ""), so tool and
description are never null when used in equals/toString or passed downstream.
Background task results now appear immediately when the task completes, even if the user is idle at the prompt. No user input required. Mechanism: - TaskManager.onTaskComplete callback fires on the task's virtual thread - pushBackgroundCompletion() renders result via TerminalMarkdownRenderer to a StringWriter, then calls reader.printAbove(AttributedString) - JLine's printAbove() is thread-safe: displays above current prompt, then redraws the prompt without disturbing user input Also fixes: - Foreground tasks now marked as notified in submitAndWait() to prevent stale "[bg task #N completed]" re-notifications - BackgroundOutputBuffer.extractTextContent() for safe text extraction without spinner side effects (avoids replay through ForegroundOutputSink) - notifyCompletedBackgroundTasks() kept as fallback, also uses safe path
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
aceclaw-cli/src/main/java/dev/aceclaw/cli/TerminalRepl.java (1)
95-103:⚠️ Potential issue | 🟠 MajorAdd null-guards for constructor parameters.
These values are passed downstream immediately; enforce non-null to prevent late NPEs.🔧 Proposed fix
import java.time.LocalTime; import java.time.format.DateTimeFormatter; +import java.util.Objects; import java.util.concurrent.TimeUnit; @@ public TerminalRepl(DaemonClient client, String sessionId, SessionInfo sessionInfo) { - this.client = client; - this.sessionId = sessionId; - this.sessionInfo = sessionInfo; + this.client = Objects.requireNonNull(client, "client"); + this.sessionId = Objects.requireNonNull(sessionId, "sessionId"); + this.sessionInfo = Objects.requireNonNull(sessionInfo, "sessionInfo"); this.effectiveModel = sessionInfo.model(); this.markdownRenderer = new TerminalMarkdownRenderer(); this.taskManager = new TaskManager(); this.permissionBridge = new PermissionBridge(); }As per coding guidelines: "Use
Objects.requireNonNull(param, "param")on parameters used in.equals()or passed to downstream calls".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/TerminalRepl.java` around lines 95 - 103, The TerminalRepl constructor should validate its inputs to avoid downstream NPEs: add non-null guards using Objects.requireNonNull for the constructor parameters (client, sessionId, sessionInfo) at the start of TerminalRepl(DaemonClient client, String sessionId, SessionInfo sessionInfo), then assign the validated values to this.client, this.sessionId, and this.sessionInfo before using sessionInfo.model(), this.markdownRenderer, this.taskManager, and this.permissionBridge so callers get immediate, clear failures instead of latent NPEs.
🤖 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-cli/src/main/java/dev/aceclaw/cli/TerminalRepl.java`:
- Around line 270-287: renderTaskCompletion currently computes elapsedMs using
the shared promptStartNanos which is wrong for concurrent tasks; change it to
use the task's own start timestamp from TaskHandle (e.g., a
startNanos/startTimeNanos or getStartNanos()/getStartTime() method on
TaskHandle) when available, falling back to promptStartNanos only if the handle
does not expose a start time; update the elapsedMs calculation to use
(System.nanoTime() - taskStartNanos) / 1_000_000 and keep the existing
formatting logic for elapsed string.
---
Outside diff comments:
In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/TerminalRepl.java`:
- Around line 95-103: The TerminalRepl constructor should validate its inputs to
avoid downstream NPEs: add non-null guards using Objects.requireNonNull for the
constructor parameters (client, sessionId, sessionInfo) at the start of
TerminalRepl(DaemonClient client, String sessionId, SessionInfo sessionInfo),
then assign the validated values to this.client, this.sessionId, and
this.sessionInfo before using sessionInfo.model(), this.markdownRenderer,
this.taskManager, and this.permissionBridge so callers get immediate, clear
failures instead of latent NPEs.
---
Duplicate comments:
In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/TerminalRepl.java`:
- Around line 252-265: The waitForForeground loop in waitForForeground blocks
REPL input because it only polls permissionBridge.pollPending(50,
TimeUnit.MILLISECONDS) while taskManager.hasForegroundTask() is true; change it
to allow processing user input concurrently—either spawn a dedicated input
thread that reads from the LineReader (or uses LineReader.readLine with a short
timeout) and dispatches commands (so `/bg` can call taskManager.moveToBackground
or similar), or perform a non-blocking read in the loop and if input equals
"/bg" call the taskManager to background the foreground task; ensure permission
handling still calls handlePermissionFromBridge(out, reader, permReq) and
preserve interruption handling.
- Around line 214-239: After waitForForeground returns, guard the completion
rendering and connection close so they only run if this task is still the
foreground task; check using the submitted handle/task id or the
activeForegroundSink (e.g., taskManager.isForeground(handle.taskId()) or
activeForegroundSink == fgSink) before calling renderTaskCompletion(handle),
taskManager.clearForeground(), setting activeForegroundSink = null, and
conn.close(); if the foreground was cleared (e.g., via /bg) skip those steps so
the background-running task and its connection are not prematurely
rendered/closed.
Comment on lines
+270
to
+287
| private void renderTaskCompletion(PrintWriter out, TaskHandle handle) { | ||
| JsonNode message = handle.result(); | ||
| if (message == null) return; | ||
|
|
||
| JsonNode result = message.get("result"); | ||
| if (result != null && result.has("usage")) { | ||
| var usage = result.get("usage"); | ||
| int turnIn = usage.path("inputTokens").asInt(0); | ||
| int turnOut = usage.path("outputTokens").asInt(0); | ||
| totalInputTokens += turnIn; | ||
| totalOutputTokens += turnOut; | ||
| latestInputTokens = turnIn; | ||
|
|
||
| long elapsedMs = (System.nanoTime() - promptStartNanos) / 1_000_000; | ||
| String elapsed = elapsedMs >= 1000 | ||
| ? String.format("%.1fs", elapsedMs / 1000.0) | ||
| : elapsedMs + "ms"; | ||
|
|
There was a problem hiding this comment.
Elapsed time should be per-task, not based on the last prompt timestamp.
With multiple concurrent tasks, promptStartNanos produces misleading durations. Prefer the task’s own start time.
🔧 Proposed fix
- long elapsedMs = (System.nanoTime() - promptStartNanos) / 1_000_000;
+ long elapsedMs = Duration.between(handle.startedAt(), Instant.now()).toMillis();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/TerminalRepl.java` around lines 270
- 287, renderTaskCompletion currently computes elapsedMs using the shared
promptStartNanos which is wrong for concurrent tasks; change it to use the
task's own start timestamp from TaskHandle (e.g., a startNanos/startTimeNanos or
getStartNanos()/getStartTime() method on TaskHandle) when available, falling
back to promptStartNanos only if the handle does not expose a start time; update
the elapsedMs calculation to use (System.nanoTime() - taskStartNanos) /
1_000_000 and keep the existing formatting logic for elapsed string.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-cli/src/main/java/dev/aceclaw/cli/TerminalRepl.java`:
- Around line 742-767: The task isn't marked as notified after bringing it to
foreground, so call the task's markNotified() method immediately after
renderTaskCompletion(out, target) and before clearing foreground state; i.e.,
insert target.markNotified() between renderTaskCompletion(out, target) and
taskManager.clearForeground()/activeForegroundSink = null so completed tasks
won't be re-announced by notifyCompletedBackgroundTasks().
---
Duplicate comments:
In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/TerminalRepl.java`:
- Around line 219-246: In submitAndWait, avoid closing the task connection or
rendering completion if the task was backgrounded; change the flow so after
waitForForeground() you check the submitted handle.isTerminal() (or equivalent
terminal state) before calling renderTaskCompletion(out, handle),
handle.markNotified(), taskManager.clearForeground(), setting
activeForegroundSink = null, and conn.close(); if not terminal, leave the
connection open and skip closing/rendering so streaming continues (alternatively
move conn.close() to TaskManager's completion callback that runs when the task
truly finishes).
- Around line 260-272: waitForForeground currently loops only polling
permissionBridge and blocks user input so slash commands like `/bg` can’t be
handled; update waitForForeground (or delegate to a dedicated input thread) to
keep the input loop alive by performing non-blocking reads from the LineReader
and dispatching commands to the existing command handler (so `/bg` can call
taskManager.backgroundCurrentTask or similar), while still polling
permissionBridge.pollPending and honoring InterruptedException; alternatively
spawn an input thread that reads lines continuously and routes slash commands
(e.g., `/bg`) to the same handler used elsewhere, ensuring
handlePermissionFromBridge(out, reader, permReq) still executes when permReq !=
null.
- Around line 278-303: The elapsed-time calculation in renderTaskCompletion uses
the global promptStartNanos, which is wrong for concurrent or resumed tasks;
instead read the task-specific start timestamp from the TaskHandle (e.g. a
method like getStartNanos()/startNanos()/getStartTimeNanos() or similar on
TaskHandle) and compute elapsedMs = (System.nanoTime() - startNanos) /
1_000_000; if TaskHandle lacks such a getter add one that records the task’s own
start time when the task is created/started and fall back to promptStartNanos
only as a last-resort default; update renderTaskCompletion to use that
handle-level start time when formatting elapsed.
Comment on lines
+742
to
+767
| if (!target.isRunning()) { | ||
| out.println(WARNING + "Task #" + target.taskId() + " is not running (" | ||
| + target.state().name().toLowerCase() + ")." + RESET); | ||
| out.flush(); | ||
| return; | ||
| } | ||
|
|
||
| out.printf("%sBringing task #%s to foreground...%s%n", MUTED, target.taskId(), RESET); | ||
| out.flush(); | ||
|
|
||
| // Create new foreground sink and swap it in atomically | ||
| var fgSink = new ForegroundOutputSink(out, markdownRenderer); | ||
| var oldSink = target.swapOutputSink(fgSink); | ||
| activeForegroundSink = fgSink; | ||
| taskManager.setForeground(target.taskId()); | ||
|
|
||
| // Replay buffered events if the task was backgrounded | ||
| if (oldSink instanceof BackgroundOutputBuffer bgBuffer) { | ||
| bgBuffer.replay(fgSink); | ||
| } | ||
|
|
||
| waitForForeground(out, reader); | ||
| renderTaskCompletion(out, target); | ||
| taskManager.clearForeground(); | ||
| activeForegroundSink = null; | ||
| } |
There was a problem hiding this comment.
Mark foregrounded tasks as notified after /fg completes.
After renderTaskCompletion, the task will be terminal and no longer foreground; without markNotified(), it will be re-announced by notifyCompletedBackgroundTasks() on the next loop.
🔧 Proposed fix
waitForForeground(out, reader);
renderTaskCompletion(out, target);
+ target.markNotified();
taskManager.clearForeground();
activeForegroundSink = null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/TerminalRepl.java` around lines 742
- 767, The task isn't marked as notified after bringing it to foreground, so
call the task's markNotified() method immediately after
renderTaskCompletion(out, target) and before clearing foreground state; i.e.,
insert target.markNotified() between renderTaskCompletion(out, target) and
taskManager.clearForeground()/activeForegroundSink = null so completed tasks
won't be re-announced by notifyCompletedBackgroundTasks().
When a task is streaming output, the terminal enters raw mode to detect individual keypresses. If the user starts typing, the foreground task is automatically backgrounded and readLine() takes over — the pressed key becomes the first character of their new input. Flow: 1. User submits prompt → task streams to terminal (real-time output) 2. User presses any key → task auto-backgrounds to BackgroundOutputBuffer 3. Prompt appears, user types new instruction 4. When backgrounded task completes → pushBackgroundCompletion() renders the result above the prompt via JLine printAbove() (no user action needed) Implementation: - waitForForeground() enters raw mode (ICANON off, ECHO off) - Polls terminal.reader().peek(1) to detect keypresses without consuming - Ctrl+C during streaming → cancels the foreground task - Any other key → autoBackground() swaps sink and breaks out of wait - Character stays in NonBlockingReader buffer for readLine() to pick up - Permission dialogs temporarily restore terminal attributes - Connection lifecycle moved to TaskManager.handleTaskComplete() (single cleanup point, idempotent via DaemonConnection.close())
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
aceclaw-cli/src/main/java/dev/aceclaw/cli/TerminalRepl.java (2)
101-109: 🛠️ Refactor suggestion | 🟠 MajorAdd null guards for constructor parameters.
The
client,sessionId, andsessionInfoparameters are used throughout the class in downstream calls. Per coding guidelines, addObjects.requireNonNullchecks.🛡️ Proposed fix
public TerminalRepl(DaemonClient client, String sessionId, SessionInfo sessionInfo) { + Objects.requireNonNull(client, "client"); + Objects.requireNonNull(sessionId, "sessionId"); + Objects.requireNonNull(sessionInfo, "sessionInfo"); this.client = client; this.sessionId = sessionId; this.sessionInfo = sessionInfo;As per coding guidelines: Use
Objects.requireNonNull(param, "param")on parameters used in.equals()or passed to downstream calls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/TerminalRepl.java` around lines 101 - 109, Add null-guards to the TerminalRepl constructor by calling Objects.requireNonNull on the incoming parameters used downstream: client, sessionId, and sessionInfo (the TerminalRepl(DaemonClient client, String sessionId, SessionInfo sessionInfo) constructor). Ensure each requireNonNull includes the parameter name string (e.g., Objects.requireNonNull(client, "client")) before assigning to the fields (this.client, this.sessionId, this.sessionInfo) so subsequent uses like sessionInfo.model() and any equals/pass-through calls are protected.
958-977:⚠️ Potential issue | 🟡 MinorGuard against null from
readTreein model list handling.Same issue as in
/tools— ifresponseLineis empty or whitespace,readTreereturnsnulland subsequent calls likeresponse.has("error")will throw NPE.🛡️ Proposed fix
var response = client.objectMapper().readTree(responseLine); + if (response == null) { + out.println(WARNING + "Empty response from daemon" + RESET); + out.flush(); + return; + } if (response.has("error")) {As per coding guidelines: Check for null return values before using methods that may return null (e.g.,
Jackson readTree("")).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/TerminalRepl.java` around lines 958 - 977, The JSON parsing can return null (client.objectMapper().readTree) when responseLine is empty/whitespace; update the model list handling in TerminalRepl to check the parsed node (response) for null immediately after calling client.objectMapper().readTree(responseLine) and treat it like an invalid response: print the same warning ("Invalid model.list response from daemon" or a timeout-style message), flush out, and return; specifically modify the block around client.readLine(5000) / client.objectMapper().readTree(...) to guard that the local variable response is not null before calling response.has("error") or response.get("result").
🧹 Nitpick comments (2)
aceclaw-cli/src/main/java/dev/aceclaw/cli/TaskManager.java (1)
8-9: Redundant import.
java.util.*already includesObjects. The explicit import on line 9 is unnecessary.import java.util.*; -import java.util.Objects;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/TaskManager.java` around lines 8 - 9, Remove the redundant explicit import of java.util.Objects in TaskManager.java because java.util.* already covers it; open the import section at the top (the lines importing java.util.* and java.util.Objects) and delete the separate java.util.Objects import so only the wildcard import remains.aceclaw-cli/src/main/java/dev/aceclaw/cli/TerminalRepl.java (1)
536-541: Consider reusing the existingmarkdownRendererinstance.A new
TerminalMarkdownRendereris created for each background completion. The class-levelmarkdownRendererfield could be reused—it's already safely shared inForegroundOutputSinkwith synchronized access, and the renderer creates fresh state perrender()call.- var sw = new StringWriter(); - var pw = new PrintWriter(sw); - new TerminalMarkdownRenderer().render(textContent, pw); + var sw = new StringWriter(); + var pw = new PrintWriter(sw); + markdownRenderer.render(textContent, pw);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/TerminalRepl.java` around lines 536 - 541, Replace the per-call instantiation of TerminalMarkdownRenderer in TerminalRepl (the snippet that does "new TerminalMarkdownRenderer().render(textContent, pw)") with the class-level markdownRenderer instance: call markdownRenderer.render(textContent, pw) instead; ensure you use the same synchronization semantics used by ForegroundOutputSink when accessing markdownRenderer (wrap render call in the same synchronized block or otherwise synchronize on the same monitor) so rendering remains thread-safe, then flush and append the StringWriter as before.
🤖 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-cli/src/main/java/dev/aceclaw/cli/TerminalRepl.java`:
- Around line 907-912: Guard against ObjectMapper.readTree returning null by
checking the parsed JsonNode before using it: after calling
client.objectMapper().readTree(responseLine) (in the block that handles
responseLine from client.readLine(5000)), verify that the returned variable
(response) is not null and that it is an array (response != null &&
response.has("result") && response.get("result").isArray()) before accessing it;
treat empty/whitespace input the same as a timeout/error (log a warning or skip
processing) and optionally trim responseLine and handle JsonProcessingException
if readTree can throw for malformed JSON.
---
Outside diff comments:
In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/TerminalRepl.java`:
- Around line 101-109: Add null-guards to the TerminalRepl constructor by
calling Objects.requireNonNull on the incoming parameters used downstream:
client, sessionId, and sessionInfo (the TerminalRepl(DaemonClient client, String
sessionId, SessionInfo sessionInfo) constructor). Ensure each requireNonNull
includes the parameter name string (e.g., Objects.requireNonNull(client,
"client")) before assigning to the fields (this.client, this.sessionId,
this.sessionInfo) so subsequent uses like sessionInfo.model() and any
equals/pass-through calls are protected.
- Around line 958-977: The JSON parsing can return null
(client.objectMapper().readTree) when responseLine is empty/whitespace; update
the model list handling in TerminalRepl to check the parsed node (response) for
null immediately after calling client.objectMapper().readTree(responseLine) and
treat it like an invalid response: print the same warning ("Invalid model.list
response from daemon" or a timeout-style message), flush out, and return;
specifically modify the block around client.readLine(5000) /
client.objectMapper().readTree(...) to guard that the local variable response is
not null before calling response.has("error") or response.get("result").
---
Duplicate comments:
In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/TerminalRepl.java`:
- Around line 864-868: After renderTaskCompletion(out, target) the completed
task must be marked notified to prevent notifyCompletedBackgroundTasks() from
re-announcing it; call target.markNotified() (or the task instance's
markNotified method) immediately after renderTaskCompletion and before
clearing/resetting the foreground (i.e., before or right after
taskManager.clearForeground()/activeForegroundSink = null), with a null-check on
target to avoid NPEs.
- Around line 392-396: The elapsed-time calculation currently uses the shared
promptStartNanos which is incorrect for concurrent tasks; update the calculation
to use the task-specific start time by replacing promptStartNanos with
handle.startedAt() when computing elapsedMs/elapsed (keep the same formatting
logic that produces "%.1fs" vs "Xms"); ensure you reference the variables
elapsedMs and elapsed and the method handle.startedAt() so each task shows
accurate per-task timing.
---
Nitpick comments:
In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/TaskManager.java`:
- Around line 8-9: Remove the redundant explicit import of java.util.Objects in
TaskManager.java because java.util.* already covers it; open the import section
at the top (the lines importing java.util.* and java.util.Objects) and delete
the separate java.util.Objects import so only the wildcard import remains.
In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/TerminalRepl.java`:
- Around line 536-541: Replace the per-call instantiation of
TerminalMarkdownRenderer in TerminalRepl (the snippet that does "new
TerminalMarkdownRenderer().render(textContent, pw)") with the class-level
markdownRenderer instance: call markdownRenderer.render(textContent, pw)
instead; ensure you use the same synchronization semantics used by
ForegroundOutputSink when accessing markdownRenderer (wrap render call in the
same synchronized block or otherwise synchronize on the same monitor) so
rendering remains thread-safe, then flush and append the StringWriter as before.
Comment on lines
+907
to
+912
| String responseLine = client.readLine(5000); | ||
| if (responseLine == null) { | ||
| out.println(WARNING + "Timed out waiting for tool list from daemon" + RESET); | ||
| } else { | ||
| var response = client.objectMapper().readTree(responseLine); | ||
| if (response.has("result") && response.get("result").isArray()) { |
There was a problem hiding this comment.
Guard against null from readTree on malformed input.
ObjectMapper.readTree() can return null for empty or whitespace-only input. If the daemon sends an empty line, line 912 would throw NPE.
🛡️ Proposed fix
String responseLine = client.readLine(5000);
if (responseLine == null) {
out.println(WARNING + "Timed out waiting for tool list from daemon" + RESET);
} else {
var response = client.objectMapper().readTree(responseLine);
+ if (response == null) {
+ out.println(WARNING + "Empty response from daemon" + RESET);
+ out.flush();
+ return;
+ }
if (response.has("result") && response.get("result").isArray()) {As per coding guidelines: Check for null return values before using methods that may return null (e.g., Jackson readTree("")).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/TerminalRepl.java` around lines 907
- 912, Guard against ObjectMapper.readTree returning null by checking the parsed
JsonNode before using it: after calling
client.objectMapper().readTree(responseLine) (in the block that handles
responseLine from client.readLine(5000)), verify that the returned variable
(response) is not null and that it is an array (response != null &&
response.has("result") && response.get("result").isArray()) before accessing it;
treat empty/whitespace input the same as a timeout/error (log a warning or skip
processing) and optionally trim responseLine and handle JsonProcessingException
if readTree can throw for malformed JSON.
This was referenced Feb 21, 2026
This was referenced Feb 28, 2026
5 tasks
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
DaemonConnectionand virtual thread, enabling concurrent prompts without wire corruptionTaskManagerwith foreground/background semantics,/tasks,/bg,/fg,/cancelslash commandsCompletableFuture— task threads block until main REPL thread prompts user and resolvesDaemonConnection.writeLine()now guarded byReentrantLock(was unguarded in oldDaemonClient)waitForForeground()blocks like before — dual-channel is opt-in via/bgNew files (8)
DaemonConnectionTaskHandlePermissionBridgeTaskManagerOutputSinkForegroundOutputSinkBackgroundOutputBufferTaskStreamReaderModified files (3)
DaemonClient— usesDaemonConnectioninternally, addedopenTaskConnection()TerminalRepl— major refactor for task-based concurrent I/OTerminalReplTest— updated signature + 3 new tests for /tasks, /bg, /cancelTest plan
./gradlew clean build— all 13 modules compile (pre-existing SkillRegistryTest failures excluded)./gradlew :aceclaw-cli:test— 12/12 tests pass./gradlew :aceclaw-cli:installDist— distribution builds/bgsends task to background,/tasksshows it,/fgbrings it back🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements