feat: surface background task visibility across full stack#1029
Merged
Gkrumbach07 merged 25 commits intomainfrom Mar 26, 2026
Merged
feat: surface background task visibility across full stack#1029Gkrumbach07 merged 25 commits intomainfrom
Gkrumbach07 merged 25 commits intomainfrom
Conversation
Add end-to-end visibility for Claude Code background tasks (subagents,
background bash) by forwarding SDK lifecycle events as AG-UI CustomEvents
and rendering them in a new explorer panel tab.
Runner:
- Generic hook-to-CustomEvent bridge (hooks.py) with configurable
hook registration via AGUI_HOOKS env var
- TaskStartedMessage/TaskProgressMessage/TaskNotificationMessage
handling in adapter, emitted as task:started/progress/completed
CustomEvents
- New /tasks endpoints: POST /{id}/stop, GET /{id}/output, GET /
- stop_task() on SessionWorker and ClaudeBridge
Backend:
- Three proxy handlers (HandleTaskStop/Output/List) with auth/RBAC
- Routes under /agentic-sessions/:session/agui/tasks/
Frontend:
- BackgroundTask types and CustomEvent handler in AG-UI event pipeline
- API service for stop/output/list endpoints
- Background Tasks tab in explorer panel with live status, progress,
stop button, and transcript viewer dialog
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
WalkthroughAdds end-to-end background-task support: runners emit AG‑UI custom task events and stream between‑run SSE; backend registers workspace Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client as Frontend
participant APIServer as Backend API
participant Runner as Session Runner
participant FS as File System
User->>Client: Click "View transcript"
Client->>APIServer: GET /agui/tasks/{taskId}/output
APIServer->>APIServer: Auth & RBAC (get)
APIServer->>Runner: GET /tasks/{taskId}/output
Runner->>FS: Resolve & read output file
FS-->>Runner: File lines / 404
Runner-->>APIServer: TaskOutputResponse
APIServer-->>Client: Return transcript
sequenceDiagram
actor ClaudeSDK as Claude Agent SDK
participant Adapter as SDK Adapter
participant HookQ as Hook Queue
participant AGUI as AG-UI Event Processor
participant State as AGUI Client State
ClaudeSDK->>Adapter: TaskStartedMessage
Adapter->>Adapter: record task
Adapter->>HookQ: enqueue CustomEvent(task:started)
HookQ->>AGUI: drain & emit events
AGUI->>State: create task (status=running)
ClaudeSDK->>Adapter: TaskProgressMessage
Adapter->>HookQ: enqueue CustomEvent(task:progress)
HookQ->>AGUI: drain & emit
AGUI->>State: update usage/last_tool_name
ClaudeSDK->>Adapter: TaskNotificationMessage
Adapter->>HookQ: enqueue CustomEvent(task:completed)
HookQ->>AGUI: drain & emit
AGUI->>State: finalize task (status, output_file)
sequenceDiagram
actor User
participant Client as Frontend
participant APIServer as Backend API
participant Runner as Session Runner
participant Claude as Claude Client
User->>Client: Click "Stop" on task
Client->>APIServer: POST /agui/tasks/{taskId}/stop
APIServer->>APIServer: Auth & RBAC (update)
APIServer->>Runner: POST /tasks/{taskId}/stop
Runner->>Runner: resolve thread_id / worker
Runner->>Claude: client.stop_task(taskId)
Claude-->>Runner: ack or error
Runner-->>APIServer: success/ack
APIServer-->>Client: {message}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
| req.Header.Set("Content-Type", "application/json") | ||
|
|
||
| resp, err := (&http.Client{Timeout: 10 * time.Second}).Do(req) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in network request Critical
| return | ||
| } | ||
|
|
||
| resp, err := (&http.Client{Timeout: 30 * time.Second}).Do(req) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in network request Critical
| return | ||
| } | ||
|
|
||
| resp, err := (&http.Client{Timeout: 10 * time.Second}).Do(req) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in network request Critical
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/backend/websocket/agui_proxy.go`:
- Around line 1142-1144: The current code reads resp.Body via io.LimitReader and
blindly forwards whatever bytes were read with resp.StatusCode (using
io.ReadAll(io.LimitReader(...)) and c.Data), which can forward truncated/invalid
JSON as a success; replace the single-step read with a controlled read that
reads up to (MAX+1) bytes (use io.ReadFull/Read or read into a buffer) so you
can detect truncation (if len > MAX), and if truncated return an error response
(e.g., c.JSON or c.AbortWithStatusJSON with a 4xx/5xx and a helpful message)
instead of calling c.Data; apply the same fix to the other occurrence at the
lines referenced (the code using io.LimitReader / io.ReadAll and c.Data).
- Around line 1082-1083: These handlers call getRunnerEndpoint() without
ensuring sessionPortMap is populated, so they can fall back to DefaultRunnerPort
after restarts or inactivity; update each handler (the occurrences around
getRunnerEndpoint at the shown sites and also the ones at 1126-1127 and
1164-1165) to first refresh/populate the sessionPortMap by invoking the same
population logic used by HandleAGUIRunProxy (the routine that fills
sessionPortMap) or an explicit refreshSessionCache/ensureSessionPortsLoaded
helper, then call getRunnerEndpoint(); this guarantees the correct non-default
runner port is resolved and avoids relying on DefaultRunnerPort even after
cleanupStaleSessions ran.
- Line 1068: Rename the local param variable taskId to taskID in both
HandleTaskStop and HandleTaskOutput, and ensure you escape it with
url.PathEscape() before building any runner URLs so reserved characters cannot
alter path segments; replace uses of c.Param("taskId")/taskId when interpolating
into URLs with escaped := url.PathEscape(taskID) (or similar) and use that
escaped value in the URL construction for the stop and output endpoints.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/explorer/background-tasks-tab.tsx:
- Around line 77-89: The handler handleViewTranscript can have race conditions
where an earlier getTaskOutput result overwrites a later one; fix by tracking
the requested task id (e.g., store task.task_id in a ref or component state like
currentTranscriptRequestId before calling getTaskOutput) and only call
setTranscriptData/setTranscriptLoading/setTranscriptTaskDesc when the returned
result's task id matches that tracked id (or cancel older requests if
getTaskOutput supports AbortSignal). Update references to handleViewTranscript,
getTaskOutput, setTranscriptData, setTranscriptLoading and setTranscriptTaskDesc
accordingly so state updates are gated by the currentTranscriptRequestId check.
- Around line 169-177: The "View transcript" button is rendered for non-running
tasks even when task.output_file is absent (causing 404s); update the render
condition to require task.output_file (e.g., change the check to task.status !==
"running" && task.output_file) and also add a defensive guard in
handleViewTranscript to no-op or show an error if task.output_file is missing
before attempting the fetch. Target the JSX around the Button and the
handleViewTranscript function to implement these checks.
In `@components/frontend/src/hooks/agui/event-handlers.ts`:
- Line 49: Remove the unused BackgroundTask import from the import statement in
event-handlers.ts: update the import line that currently imports BackgroundTask,
BackgroundTaskStatus, BackgroundTaskUsage so it no longer includes
BackgroundTask (keep BackgroundTaskStatus and BackgroundTaskUsage if they are
used); this will resolve the typescript-eslint/no-unused-vars failure caused by
the unused BackgroundTask symbol.
In `@components/frontend/src/services/api/tasks.ts`:
- Around line 9-10: The taskId is interpolated directly into path segments in
stopBackgroundTask and getTaskOutput which can break URLs when taskId contains
reserved characters; update both functions to wrap taskId with
encodeURIComponent(taskId) when building the path (e.g.,
`/agui/tasks/${encodeURIComponent(taskId)}/...`) so the segment is safely
encoded before calling apiClient.post or apiClient.get.
In `@components/frontend/src/types/agui.ts`:
- Line 53: Remove the unused local import AGUICustomEvent from the import list
in types/agui.ts: since AGUICustomEvent is already re-exported for external
consumers, delete AGUICustomEvent from the import statement to resolve the
'defined but never used' lint error and keep the existing re-export intact.
In `@components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py`:
- Around line 244-246: The adapter-scoped self._hook_event_queue is reused
across runs causing leftover hook events to be attributed to subsequent runs; to
fix, reinitialize or clear self._hook_event_queue at the start of each run
(e.g., inside the ClaudeAgentAdapter method that begins a run/session) so each
run gets a fresh asyncio.Queue, and ensure the StreamEvent hot path (StreamEvent
handling logic) cannot proceed before the per-run queue is created/drained;
reference self._hook_event_queue and the ClaudeAgentAdapter run
entrypoint/handler when applying the change.
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py`:
- Around line 250-255: The task_registry property currently returns the
adapter's live dict; instead, return a shallow copy of that dict on read to
avoid exposing mutable internals and concurrent-mutation issues—fetch the
adapter dict via getattr(self._adapter, "_task_registry", {}) and return a copy
(use the dict copy method) rather than the original; apply the same copy-on-read
change to the other similar registry accessor(s) defined nearby in this class so
no live adapter dictionaries are handed out.
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/session.py`:
- Around line 248-257: The stop_task coroutine is creating but not awaiting the
client's stop call; update Session.stop_task (the async method named stop_task)
to await the client's coroutine by changing the call to await
self._client.stop_task(task_id) inside the try block (keeping the existing
exception handling and RuntimeError branch) so the stop request actually
executes when self._client is present.
In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py`:
- Around line 33-35: Replace returning raw exception text in HTTPException
responses with a generic error message and log full server-side details: in the
stop_task handler (reference symbol stop_task and variable task_id) change
logger.error(f"...{e}") to logger.exception(...) to capture the stack trace, and
raise HTTPException(status_code=500, detail="Failed to stop task" or "Internal
server error"); apply the same pattern to the other handler around lines 83-85
(the analogous exception block) so no internal exception strings are exposed to
clients while full error info is retained in logs.
- Around line 44-50: The fallback uses a user-driven recursive glob
(projects_dir.rglob(f"*{task_id}*")) which is nondeterministic and unsafe;
replace that wildcard scan with a deterministic lookup: consult the
bridge-registered locations (e.g., a registry or function that returns allowed
paths) or compute a per-task filename pattern (e.g., a fixed filename like
task_{task_id}.transcript) and check only those explicit paths for existence
before assigning output_path; update the logic that sets output_path (the block
referencing projects_dir, task_id and output_path) and mirror the same change
for the similar lookup in the later block (lines 52-60) so only approved,
deterministic locations are checked.
- Around line 46-50: In async def get_task_output, avoid blocking the event loop
by moving filesystem calls (projects_dir.rglob, Path.stat, and file open/read)
into worker threads using anyio.to_thread.run_sync: replace the rglob loop that
sets output_path (using projects_dir and task_id) with a to_thread.run_sync call
that returns the first matching path or None, and wrap the file-size check
(resolved.stat().st_size > _MAX_OUTPUT_BYTES) and file open/parse logic inside a
synchronous helper (e.g., read_file) then call it with anyio.to_thread.run_sync
to return entries; ensure HTTPException raising logic remains intact inside the
sync helper so errors are propagated correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: be4eea79-f4ca-4af1-b8b7-d423c11d9fb9
📒 Files selected for processing (18)
components/backend/routes.gocomponents/backend/websocket/agui_proxy.gocomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/explorer/background-tasks-tab.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/explorer/explorer-panel.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/hooks/use-explorer-state.tscomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/page.tsxcomponents/frontend/src/hooks/agui/__tests__/event-handlers.test.tscomponents/frontend/src/hooks/agui/event-handlers.tscomponents/frontend/src/hooks/agui/types.tscomponents/frontend/src/services/api/tasks.tscomponents/frontend/src/types/agui.tscomponents/frontend/src/types/background-task.tscomponents/runners/ambient-runner/ag_ui_claude_sdk/adapter.pycomponents/runners/ambient-runner/ag_ui_claude_sdk/hooks.pycomponents/runners/ambient-runner/ambient_runner/app.pycomponents/runners/ambient-runner/ambient_runner/bridges/claude/bridge.pycomponents/runners/ambient-runner/ambient_runner/bridges/claude/session.pycomponents/runners/ambient-runner/ambient_runner/endpoints/tasks.py
| func HandleTaskStop(c *gin.Context) { | ||
| projectName := c.Param("projectName") | ||
| sessionName := c.Param("sessionName") | ||
| taskId := c.Param("taskId") |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd components/backend/websocket && wc -l agui_proxy.goRepository: ambient-code/platform
Length of output: 83
🏁 Script executed:
head -20 components/backend/websocket/agui_proxy.go && echo "..." && sed -n '1060,1075p' components/backend/websocket/agui_proxy.goRepository: ambient-code/platform
Length of output: 1190
🏁 Script executed:
sed -n '1065,1140p' components/backend/websocket/agui_proxy.goRepository: ambient-code/platform
Length of output: 2350
🏁 Script executed:
sed -n '1,30p' components/backend/websocket/agui_proxy.goRepository: ambient-code/platform
Length of output: 827
🏁 Script executed:
grep -n "taskId\|taskID" components/backend/websocket/agui_proxy.goRepository: ambient-code/platform
Length of output: 304
🏁 Script executed:
sed -n '1078,1090p' components/backend/websocket/agui_proxy.goRepository: ambient-code/platform
Length of output: 458
🏁 Script executed:
grep -n "net/url\|url\." components/backend/websocket/agui_proxy.goRepository: ambient-code/platform
Length of output: 47
🏁 Script executed:
sed -n '1060,1140p' components/backend/websocket/agui_proxy.go | cat -nRepository: ambient-code/platform
Length of output: 3072
🏁 Script executed:
python3 << 'EOF'
import urllib.parse
# Test what happens with special characters in taskId without escaping
taskId_with_special = "123/456"
taskId_with_dots = "123/../"
taskId_with_query = "123?foo=bar"
print("Without escaping:")
print(f" taskId='123/456' -> URL: /tasks/{taskId_with_special}/stop")
print(f" taskId='123/../' -> URL: /tasks/{taskId_with_dots}/stop")
print(f" taskId='123?foo=bar' -> URL: /tasks/{taskId_with_query}/stop")
print("\nWith url.PathEscape():")
print(f" taskId='123/456' -> URL: /tasks/{urllib.parse.quote(taskId_with_special, safe='')}/stop")
print(f" taskId='123/../' -> URL: /tasks/{urllib.parse.quote(taskId_with_dots, safe='')}/stop")
print(f" taskId='123?foo=bar' -> URL: /tasks/{urllib.parse.quote(taskId_with_query, safe='')}/stop")
EOFRepository: ambient-code/platform
Length of output: 414
🏁 Script executed:
# Check if there's any lint or staticcheck configuration
fd -e ".staticcheck" -e "golangci" -e ".golint" components/backend/Repository: ambient-code/platform
Length of output: 446
🏁 Script executed:
# Check project root for lint configs
fd -e ".golangci.yml" -e ".golangci.yaml" -e ".staticcheck" . --max-depth 2Repository: ambient-code/platform
Length of output: 47
🏁 Script executed:
# Check if net/url is available in Go standard library
go doc net/url 2>/dev/null | head -20 || echo "Go environment check"Repository: ambient-code/platform
Length of output: 732
🏁 Script executed:
# Look for any existing usage of url.PathEscape or similar in the codebase
rg "url\.(Path|Query)" components/backend/ --type goRepository: ambient-code/platform
Length of output: 2673
🏁 Script executed:
# Check the Go naming convention - look for similar acronym patterns in the codebase
rg "(taskID|ProjectID|SessionID)" components/backend/ --type go | head -20Repository: ambient-code/platform
Length of output: 2255
Escape taskID path parameter in URLs to prevent path manipulation.
The unescaped taskId variable allows reserved characters to rewrite the runner path. For example, if taskId contains /, the URL becomes /tasks/123/456/stop instead of /tasks/123%2F456/stop, enabling path traversal attacks. Rename to taskID (Go idiom for ID suffix) and use url.PathEscape() before inserting into the URL.
Apply the fix to both HandleTaskStop (lines 1068, 1083) and HandleTaskOutput (lines 1112, 1127):
Suggested fix
+ import "net/url"
...
- taskId := c.Param("taskId")
+ taskID := c.Param("taskId")
...
- targetURL := strings.TrimSuffix(runnerURL, "/") + "/tasks/" + taskId + "/stop"
+ targetURL := strings.TrimSuffix(runnerURL, "/") + "/tasks/" + url.PathEscape(taskID) + "/stop"- taskId := c.Param("taskId")
+ taskID := c.Param("taskId")
...
- targetURL := strings.TrimSuffix(runnerURL, "/") + "/tasks/" + taskId + "/output"
+ targetURL := strings.TrimSuffix(runnerURL, "/") + "/tasks/" + url.PathEscape(taskID) + "/output"🧰 Tools
🪛 GitHub Actions: Lint
[error] 1068-1068: golangci-lint reported ST1003: var taskId should be taskID (staticcheck)
🪛 GitHub Check: Go Lint - Backend
[failure] 1068-1068:
ST1003: var taskId should be taskID (staticcheck)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/backend/websocket/agui_proxy.go` at line 1068, Rename the local
param variable taskId to taskID in both HandleTaskStop and HandleTaskOutput, and
ensure you escape it with url.PathEscape() before building any runner URLs so
reserved characters cannot alter path segments; replace uses of
c.Param("taskId")/taskId when interpolating into URLs with escaped :=
url.PathEscape(taskID) (or similar) and use that escaped value in the URL
construction for the stop and output endpoints.
Comment on lines
+1142
to
+1144
| // Transcripts can be large; cap at 10 MB. | ||
| body, _ := io.ReadAll(io.LimitReader(resp.Body, 10*1024*1024)) | ||
| c.Data(resp.StatusCode, resp.Header.Get("Content-Type"), body) |
Contributor
There was a problem hiding this comment.
Don't forward truncated JSON as success.
io.LimitReader silently chops the payload and c.Data forwards whatever was read with the runner's status code. Once a transcript or task list exceeds the cap, the frontend gets invalid JSON with an apparent success response and fails client-side.
Also applies to: 1180-1181
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/backend/websocket/agui_proxy.go` around lines 1142 - 1144, The
current code reads resp.Body via io.LimitReader and blindly forwards whatever
bytes were read with resp.StatusCode (using io.ReadAll(io.LimitReader(...)) and
c.Data), which can forward truncated/invalid JSON as a success; replace the
single-step read with a controlled read that reads up to (MAX+1) bytes (use
io.ReadFull/Read or read into a buffer) so you can detect truncation (if len >
MAX), and if truncated return an error response (e.g., c.JSON or
c.AbortWithStatusJSON with a 4xx/5xx and a helpful message) instead of calling
c.Data; apply the same fix to the other occurrence at the lines referenced (the
code using io.LimitReader / io.ReadAll and c.Data).
Comment on lines
+77
to
+89
| const handleViewTranscript = async (task: BackgroundTask) => { | ||
| setTranscriptTaskDesc(task.description) | ||
| setTranscriptLoading(true) | ||
| setTranscriptOpen(true) | ||
| try { | ||
| const output = await getTaskOutput(projectName, sessionName, task.task_id) | ||
| setTranscriptData(output) | ||
| } catch (err) { | ||
| console.error("Failed to load transcript:", err) | ||
| setTranscriptData(null) | ||
| } finally { | ||
| setTranscriptLoading(false) | ||
| } |
Contributor
There was a problem hiding this comment.
Ignore stale transcript responses.
If the user opens task A and then task B before A finishes loading, A's response can still win the race and overwrite B's modal contents. Gate state updates on the currently requested task ID, or cancel the older request.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/explorer/background-tasks-tab.tsx
around lines 77 - 89, The handler handleViewTranscript can have race conditions
where an earlier getTaskOutput result overwrites a later one; fix by tracking
the requested task id (e.g., store task.task_id in a ref or component state like
currentTranscriptRequestId before calling getTaskOutput) and only call
setTranscriptData/setTranscriptLoading/setTranscriptTaskDesc when the returned
result's task id matches that tracked id (or cancel older requests if
getTaskOutput supports AbortSignal). Update references to handleViewTranscript,
getTaskOutput, setTranscriptData, setTranscriptLoading and setTranscriptTaskDesc
accordingly so state updates are gated by the currentTranscriptRequestId check.
Comment on lines
+169
to
+177
| {task.status !== "running" && ( | ||
| <Button | ||
| variant="ghost" | ||
| size="sm" | ||
| className="h-6 text-xs px-2" | ||
| onClick={() => handleViewTranscript(task)} | ||
| > | ||
| View transcript | ||
| </Button> |
Contributor
There was a problem hiding this comment.
Gate transcript fetches on task.output_file.
The runner only exposes transcript output for tasks that published an output_file, so many failed/stopped tasks can only 404 here. Right now the UI advertises an action the backend cannot fulfill.
Suggested fix
- {task.status !== "running" && (
+ {task.status !== "running" && task.output_file && (
<Button
variant="ghost"
size="sm"
className="h-6 text-xs px-2"
onClick={() => handleViewTranscript(task)}📝 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
| {task.status !== "running" && ( | |
| <Button | |
| variant="ghost" | |
| size="sm" | |
| className="h-6 text-xs px-2" | |
| onClick={() => handleViewTranscript(task)} | |
| > | |
| View transcript | |
| </Button> | |
| {task.status !== "running" && task.output_file && ( | |
| <Button | |
| variant="ghost" | |
| size="sm" | |
| className="h-6 text-xs px-2" | |
| onClick={() => handleViewTranscript(task)} | |
| > | |
| View transcript | |
| </Button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/explorer/background-tasks-tab.tsx
around lines 169 - 177, The "View transcript" button is rendered for non-running
tasks even when task.output_file is absent (causing 404s); update the render
condition to require task.output_file (e.g., change the check to task.status !==
"running" && task.output_file) and also add a defensive guard in
handleViewTranscript to no-op or show an error if task.output_file is missing
before attempting the fetch. Target the JSX around the Button and the
handleViewTranscript function to implement these checks.
Comment on lines
+250
to
+255
| @property | ||
| def task_registry(self) -> dict: | ||
| """Background task metadata tracked by the adapter.""" | ||
| if self._adapter: | ||
| return getattr(self._adapter, "_task_registry", {}) | ||
| return {} |
Contributor
There was a problem hiding this comment.
Return copies from these accessors.
These properties hand out the adapter's live dictionaries directly. While tasks are still updating, an endpoint serializing that state can observe partial data or fail with a concurrent-mutation error. Copy on read instead of exposing the mutable internals.
Suggested fix
`@property`
def task_registry(self) -> dict:
"""Background task metadata tracked by the adapter."""
if self._adapter:
- return getattr(self._adapter, "_task_registry", {})
+ registry = getattr(self._adapter, "_task_registry", {})
+ return {
+ task_id: dict(task_info) if isinstance(task_info, dict) else task_info
+ for task_id, task_info in registry.items()
+ }
return {}
`@property`
def task_outputs(self) -> dict:
"""Background task output file paths tracked by the adapter."""
if self._adapter:
- return getattr(self._adapter, "_task_outputs", {})
+ return dict(getattr(self._adapter, "_task_outputs", {}))
return {}Also applies to: 257-262
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py`
around lines 250 - 255, The task_registry property currently returns the
adapter's live dict; instead, return a shallow copy of that dict on read to
avoid exposing mutable internals and concurrent-mutation issues—fetch the
adapter dict via getattr(self._adapter, "_task_registry", {}) and return a copy
(use the dict copy method) rather than the original; apply the same copy-on-read
change to the other similar registry accessor(s) defined nearby in this class so
no live adapter dictionaries are handed out.
components/runners/ambient-runner/ambient_runner/bridges/claude/session.py
Show resolved
Hide resolved
Comment on lines
+33
to
+35
| except Exception as e: | ||
| logger.error(f"stop_task({task_id}) failed: {e}") | ||
| raise HTTPException(status_code=500, detail=str(e)) |
Contributor
There was a problem hiding this comment.
Do not return raw exception text to clients.
Line 35 and Line 85 propagate str(e) in HTTP 500 responses, which leaks internal details. Log server-side with stack traces and return a generic failure message.
🔧 Suggested hardening
- except Exception as e:
- logger.error(f"stop_task({task_id}) failed: {e}")
- raise HTTPException(status_code=500, detail=str(e))
+ except Exception:
+ logger.exception("stop_task failed for task_id=%s", task_id)
+ raise HTTPException(status_code=500, detail="Failed to stop task")
...
- except Exception as e:
- logger.error(f"Failed to read output for task {task_id}: {e}")
- raise HTTPException(status_code=500, detail=str(e))
+ except Exception:
+ logger.exception("Failed to read output for task_id=%s", task_id)
+ raise HTTPException(status_code=500, detail="Failed to read task output")As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 83-85
🧰 Tools
🪛 Ruff (0.15.6)
[warning] 33-33: Do not catch blind exception: Exception
(BLE001)
[warning] 34-34: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
[warning] 34-34: Logging statement uses f-string
(G004)
[warning] 35-35: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py` around
lines 33 - 35, Replace returning raw exception text in HTTPException responses
with a generic error message and log full server-side details: in the stop_task
handler (reference symbol stop_task and variable task_id) change
logger.error(f"...{e}") to logger.exception(...) to capture the stack trace, and
raise HTTPException(status_code=500, detail="Failed to stop task" or "Internal
server error"); apply the same pattern to the other handler around lines 83-85
(the analogous exception block) so no internal exception strings are exposed to
clients while full error info is retained in logs.
components/runners/ambient-runner/ambient_runner/endpoints/tasks.py
Outdated
Show resolved
Hide resolved
Comment on lines
+46
to
+50
| projects_dir = Path.home() / ".claude" / "projects" | ||
| if projects_dir.exists(): | ||
| for match in projects_dir.rglob(f"*{task_id}*"): | ||
| output_path = str(match) | ||
| break |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n components/runners/ambient-runner/ambient_runner/endpoints/tasks.py | head -100Repository: ambient-code/platform
Length of output: 3645
Move blocking filesystem operations to worker thread.
rglob, stat, and open are synchronous blocking operations in async def get_task_output. Under concurrent requests, these block the asyncio event loop and stall request handling. Use anyio.to_thread.run_sync() to run file discovery, size checks, and file reading in a worker thread:
# Lines 46-50 (rglob)
output_path = await anyio.to_thread.run_sync(
lambda: next((str(m) for m in projects_dir.rglob(f"*{task_id}*")), None)
)
# Lines 67-82 (stat and open)
def read_file():
if resolved.stat().st_size > _MAX_OUTPUT_BYTES:
raise HTTPException(...)
with open(resolved) as f:
# parse entries...
return entries
entries = await anyio.to_thread.run_sync(read_file)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py` around
lines 46 - 50, In async def get_task_output, avoid blocking the event loop by
moving filesystem calls (projects_dir.rglob, Path.stat, and file open/read) into
worker threads using anyio.to_thread.run_sync: replace the rglob loop that sets
output_path (using projects_dir and task_id) with a to_thread.run_sync call that
returns the first matching path or None, and wrap the file-size check
(resolved.stat().st_size > _MAX_OUTPUT_BYTES) and file open/parse logic inside a
synchronous helper (e.g., read_file) then call it with anyio.to_thread.run_sync
to return entries; ensure HTTPException raising logic remains intact inside the
sync helper so errors are propagated correctly.
- Go: rename taskId → taskID (ST1003 staticcheck) - Frontend: remove unused BackgroundTask import from event-handlers.ts - Frontend: remove duplicate AGUICustomEvent import from agui.ts Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
components/backend/websocket/agui_proxy.go (2)
1082-1083:⚠️ Potential issue | 🟠 MajorEscape
taskIDin URL construction to prevent path manipulation.The
taskIDparameter is directly concatenated into the URL without escaping. A malicioustaskIDcontaining/or..could manipulate the request path (e.g.,../../other-endpoint).Suggested fix
+ import "net/url" ... - targetURL := strings.TrimSuffix(runnerURL, "/") + "/tasks/" + taskID + "/stop" + targetURL := strings.TrimSuffix(runnerURL, "/") + "/tasks/" + url.PathEscape(taskID) + "/stop"Apply the same fix for
HandleTaskOutputat line 1127.Also applies to: 1126-1127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/backend/websocket/agui_proxy.go` around lines 1082 - 1083, The code builds target URLs by concatenating taskID directly (see getRunnerEndpoint and the targetURL variable) which allows path manipulation; fix by URL-escaping the taskID (use url.PathEscape or equivalent) when constructing the path segment so any "/" or ".." are encoded, and apply the same change to the HandleTaskOutput URL construction as well (update occurrences where taskID is appended to runner URL).
1142-1144:⚠️ Potential issue | 🟠 MajorHandle truncated responses instead of forwarding potentially invalid JSON.
io.LimitReadersilently truncates responses exceeding the limit. The current code forwards the truncated body with the runner's success status, which could deliver malformed JSON to the frontend.Suggested approach
Read one extra byte to detect truncation and return an error response:
const maxOutputBytes = 10 * 1024 * 1024 buf := make([]byte, maxOutputBytes+1) n, _ := io.ReadFull(resp.Body, buf) if n > maxOutputBytes { c.JSON(http.StatusRequestEntityTooLarge, gin.H{"error": "Response too large"}) return } c.Data(resp.StatusCode, resp.Header.Get("Content-Type"), buf[:n])Also applies to: 1180-1181
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/backend/websocket/agui_proxy.go` around lines 1142 - 1144, Replace the current silent truncation via io.LimitReader + io.ReadAll with a deterministic truncation check: allocate a buffer of size maxOutputBytes+1 (e.g. const maxOutputBytes = 10*1024*1024), read into it from resp.Body (io.ReadFull or io.ReadAtLeast), and if bytes read > maxOutputBytes respond with c.JSON(http.StatusRequestEntityTooLarge, ...) and return; otherwise call c.Data(resp.StatusCode, resp.Header.Get("Content-Type"), buf[:n]). Apply the same change to the other identical read that uses resp and c in this file so you never forward truncated (and potentially invalid) JSON.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@components/backend/websocket/agui_proxy.go`:
- Around line 1082-1083: The code builds target URLs by concatenating taskID
directly (see getRunnerEndpoint and the targetURL variable) which allows path
manipulation; fix by URL-escaping the taskID (use url.PathEscape or equivalent)
when constructing the path segment so any "/" or ".." are encoded, and apply the
same change to the HandleTaskOutput URL construction as well (update occurrences
where taskID is appended to runner URL).
- Around line 1142-1144: Replace the current silent truncation via
io.LimitReader + io.ReadAll with a deterministic truncation check: allocate a
buffer of size maxOutputBytes+1 (e.g. const maxOutputBytes = 10*1024*1024), read
into it from resp.Body (io.ReadFull or io.ReadAtLeast), and if bytes read >
maxOutputBytes respond with c.JSON(http.StatusRequestEntityTooLarge, ...) and
return; otherwise call c.Data(resp.StatusCode, resp.Header.Get("Content-Type"),
buf[:n]). Apply the same change to the other identical read that uses resp and c
in this file so you never forward truncated (and potentially invalid) JSON.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 76430092-db69-4ef6-b614-3468ddf3a8e4
📒 Files selected for processing (3)
components/backend/websocket/agui_proxy.gocomponents/frontend/src/hooks/agui/event-handlers.tscomponents/frontend/src/types/agui.ts
…oints Without these route files, the frontend API calls to /api/projects/.../agui/tasks/... return 404 because Next.js has no route handler to proxy them to the Go backend. Adds three route files following the existing interrupt/capabilities pattern: - POST .../agui/tasks/[taskId]/stop/route.ts - GET .../agui/tasks/[taskId]/output/route.ts - GET .../agui/tasks/route.ts Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
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
`@components/frontend/src/app/api/projects/`[name]/agentic-sessions/[sessionName]/agui/tasks/[taskId]/output/route.ts:
- Around line 22-26: The Response is currently created with a hardcoded
'Content-Type: application/json' which discards the runner's content type;
update the GET handler in this route (where resp is used) to forward the
backend's Content-Type by reading resp.headers.get('content-type') and using
that value in the Response headers (fall back to a sensible default if missing),
and ensure you return any other needed headers from resp as appropriate so the
proxied payload preserves original content-type and metadata.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d5107e15-6b33-4401-b61e-8594678214db
📒 Files selected for processing (3)
components/frontend/src/app/api/projects/[name]/agentic-sessions/[sessionName]/agui/tasks/[taskId]/output/route.tscomponents/frontend/src/app/api/projects/[name]/agentic-sessions/[sessionName]/agui/tasks/[taskId]/stop/route.tscomponents/frontend/src/app/api/projects/[name]/agentic-sessions/[sessionName]/agui/tasks/route.ts
Comment on lines
+22
to
+26
| const data = await resp.text() | ||
| return new Response(data, { | ||
| status: resp.status, | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| }) |
Contributor
There was a problem hiding this comment.
Forward Content-Type from backend instead of hardcoding.
The backend handler (HandleTaskOutput in agui_proxy.go) explicitly preserves the Content-Type from the runner via c.Data(resp.StatusCode, resp.Header.Get("Content-Type"), body). Hardcoding application/json here discards that information and may return an incorrect Content-Type if the runner responds with non-JSON content.
Proposed fix
const data = await resp.text()
return new Response(data, {
status: resp.status,
- headers: { 'Content-Type': 'application/json' },
+ headers: { 'Content-Type': resp.headers.get('Content-Type') || 'application/json' },
})📝 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
| const data = await resp.text() | |
| return new Response(data, { | |
| status: resp.status, | |
| headers: { 'Content-Type': 'application/json' }, | |
| }) | |
| const data = await resp.text() | |
| return new Response(data, { | |
| status: resp.status, | |
| headers: { 'Content-Type': resp.headers.get('Content-Type') || 'application/json' }, | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/frontend/src/app/api/projects/`[name]/agentic-sessions/[sessionName]/agui/tasks/[taskId]/output/route.ts
around lines 22 - 26, The Response is currently created with a hardcoded
'Content-Type: application/json' which discards the runner's content type;
update the GET handler in this route (where resp is used) to forward the
backend's Content-Type by reading resp.headers.get('content-type') and using
that value in the Response headers (fall back to a sensible default if missing),
and ensure you return any other needed headers from resp as appropriate so the
proxied payload preserves original content-type and metadata.
…sage views Replace the raw-JSON Dialog with proper content tabs that render background task transcripts as conversation views, reusing the existing StreamMessage component. Changes: - Extract shared task utilities (StatusIcon, formatDuration, formatTokens) to lib/task-utils.tsx - Extend tab system with TaskTab type and openTask/closeTask/switchToTask/ updateTaskStatus in use-file-tabs.ts - Add task tab rendering to ContentTabs with status icons - Create transform-task-transcript.ts that maps raw SDK JSONL entries to the frontend's Message union types (UserMessage, AgentMessage, ToolUseMessages) - Create TaskTranscriptViewer component with header (status, stop, refresh) and StreamMessage-based content, with live polling for running tasks - Wire into page.tsx renderMainContent(), ContentTabs, and ExplorerPanel - Remove Dialog from BackgroundTasksTab, replace with onOpenTranscript callback that opens a content tab Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/frontend/src/app/projects/[name]/sessions/[sessionName]/components/explorer/background-tasks-tab.tsx (1)
113-120: 🧹 Nitpick | 🔵 TrivialConsider gating "View transcript" on
task.output_fileavailability.The transcript viewer handles fetch errors gracefully, but showing the button for tasks without an
output_filewill lead to 404 errors. This isn't a blocker since the error state is handled, but you could improve UX by only showing the button when transcripts are available.💡 Optional improvement
- <Button - variant="ghost" - size="sm" - className="h-6 text-xs px-2" - onClick={() => onOpenTranscript(task)} - > - View transcript - </Button> + {task.output_file && ( + <Button + variant="ghost" + size="sm" + className="h-6 text-xs px-2" + onClick={() => onOpenTranscript(task)} + > + View transcript + </Button> + )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/explorer/background-tasks-tab.tsx around lines 113 - 120, Only render the "View transcript" Button when the task has an available transcript file: check task.output_file (or equivalent property) before rendering the Button that calls onOpenTranscript(task), so tasks without an output_file do not show the button and thus avoid triggering 404s; locate the Button component in background-tasks-tab.tsx and wrap it in a conditional (or render a disabled/hidden variant) using the task.output_file check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/page.tsx:
- Around line 1519-1525: The useEffect that iterates over
aguiState.backgroundTasks and calls fileTabs.updateTaskStatus is placed after
early returns, violating React Hooks rules; move that useEffect so it always
runs (place it with the other effects before any loading/error early returns),
remove the duplicate effect at its current location, and update its dependency
array to include fileTabs and aguiState.backgroundTasks (remove the
eslint-disable comment) so the hook is declared unconditionally and dependency
tracking is correct.
---
Duplicate comments:
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/explorer/background-tasks-tab.tsx:
- Around line 113-120: Only render the "View transcript" Button when the task
has an available transcript file: check task.output_file (or equivalent
property) before rendering the Button that calls onOpenTranscript(task), so
tasks without an output_file do not show the button and thus avoid triggering
404s; locate the Button component in background-tasks-tab.tsx and wrap it in a
conditional (or render a disabled/hidden variant) using the task.output_file
check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e5132545-368d-4008-b9c7-8e5c280f8219
📒 Files selected for processing (8)
components/frontend/src/app/projects/[name]/sessions/[sessionName]/components/content-tabs.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/explorer/background-tasks-tab.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/explorer/explorer-panel.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/task-transcript-viewer.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/hooks/use-file-tabs.tscomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/page.tsxcomponents/frontend/src/lib/task-utils.tsxcomponents/frontend/src/lib/transform-task-transcript.ts
components/frontend/src/app/projects/[name]/sessions/[sessionName]/page.tsx
Outdated
Show resolved
Hide resolved
- updateTaskStatus: short-circuit when status unchanged (avoids unnecessary array allocation and re-renders) - Remove eslint-disable on useEffect dep array, add stable callback - Hoist noop fallback to module constant (avoids new function per render) - Use "unknown" instead of empty string for missing model field Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/frontend/src/app/projects/[name]/sessions/[sessionName]/page.tsx (1)
1519-1524:⚠️ Potential issue | 🔴 CriticalCritical:
useEffectplaced after early returns violates React's Rules of Hooks.This effect is declared after the loading/error early returns (lines 1488-1517), causing it to be called conditionally. React requires all hooks to execute in the same order on every render. This is blocking the pipeline.
Move this effect before the early returns, near other effects (around line 1289).
🐛 Proposed fix
Move this block to before line 1488 (e.g., after line 1289):
}, [session?.status?.phase, refetchArtifactsFiles]); + + // Keep task tab status badges in sync with live AG-UI state + useEffect(() => { + for (const [taskId, task] of aguiState.backgroundTasks) { + fileTabs.updateTaskStatus(taskId, task.status); + } + }, [aguiState.backgroundTasks, fileTabs.updateTaskStatus]); // Session action handlers const handleStop = () => {Then remove lines 1519-1524 from their current location.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/page.tsx around lines 1519 - 1524, The useEffect that iterates aguiState.backgroundTasks and calls fileTabs.updateTaskStatus is declared after component early returns, violating React Hooks ordering; move that entire useEffect block (the one referencing aguiState.backgroundTasks and fileTabs.updateTaskStatus) to a location before any conditional/early returns (e.g., near the other effects around the earlier effect group), and remove the original occurrence so all hooks run unconditionally and in consistent order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/frontend/src/lib/transform-task-transcript.ts`:
- Around line 50-74: In transform-task-transcript.ts, when iterating content and
encountering a tool_result block (check block.type === "tool_result" and
block.tool_use_id), do not silently skip cases where
pendingToolUses.get(block.tool_use_id) returns undefined; instead emit a warning
or log including the block.tool_use_id and any relevant block metadata so
unmatched tool results are visible in logs. Update the loop that currently uses
pendingToolUses, adding a processLogger.warn or console.warn (consistent with
existing logging conventions) in the else branch before continuing, referencing
pendingToolUses, block.tool_use_id, and the surrounding message construction
(ToolResultBlock/ToolUseMessages) so debugging out-of-order or missing entries
is easier.
---
Duplicate comments:
In `@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/page.tsx:
- Around line 1519-1524: The useEffect that iterates aguiState.backgroundTasks
and calls fileTabs.updateTaskStatus is declared after component early returns,
violating React Hooks ordering; move that entire useEffect block (the one
referencing aguiState.backgroundTasks and fileTabs.updateTaskStatus) to a
location before any conditional/early returns (e.g., near the other effects
around the earlier effect group), and remove the original occurrence so all
hooks run unconditionally and in consistent order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b9481b9f-6833-49d9-9875-ac21366a7f05
📒 Files selected for processing (4)
components/frontend/src/app/projects/[name]/sessions/[sessionName]/components/explorer/explorer-panel.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/hooks/use-file-tabs.tscomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/page.tsxcomponents/frontend/src/lib/transform-task-transcript.ts
Comment on lines
+50
to
+74
| } else if (Array.isArray(content)) { | ||
| // Tool result blocks — pair with pending tool uses | ||
| for (const block of content as SdkContentBlock[]) { | ||
| if (block.type === "tool_result" && block.tool_use_id) { | ||
| const toolUse = pendingToolUses.get(block.tool_use_id) | ||
| if (toolUse) { | ||
| const resultContent = typeof block.content === "string" | ||
| ? block.content | ||
| : block.content ?? null | ||
| messages.push({ | ||
| type: "tool_use_messages", | ||
| toolUseBlock: toolUse, | ||
| resultBlock: { | ||
| type: "tool_result_block", | ||
| tool_use_id: block.tool_use_id, | ||
| content: resultContent, | ||
| is_error: block.is_error ?? false, | ||
| } satisfies ToolResultBlock, | ||
| timestamp, | ||
| } satisfies ToolUseMessages) | ||
| pendingToolUses.delete(block.tool_use_id) | ||
| } | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Silently dropping unmatched tool results may hinder debugging.
When a tool_result block has no corresponding entry in pendingToolUses (lines 54-55), it is silently skipped. This could occur if the transcript data is incomplete or entries arrive out-of-order. Consider logging or emitting a warning for unmatched results to aid debugging in production.
🔧 Optional: Add a warning for unmatched tool results
if (toolUse) {
// ... existing code ...
pendingToolUses.delete(block.tool_use_id)
+ } else {
+ console.warn(`[transformTaskTranscript] No pending tool_use found for tool_result: ${block.tool_use_id}`)
}🧰 Tools
🪛 GitHub Actions: Lint
[error] Step failed: npm run lint
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/frontend/src/lib/transform-task-transcript.ts` around lines 50 -
74, In transform-task-transcript.ts, when iterating content and encountering a
tool_result block (check block.type === "tool_result" and block.tool_use_id), do
not silently skip cases where pendingToolUses.get(block.tool_use_id) returns
undefined; instead emit a warning or log including the block.tool_use_id and any
relevant block metadata so unmatched tool results are visible in logs. Update
the loop that currently uses pendingToolUses, adding a processLogger.warn or
console.warn (consistent with existing logging conventions) in the else branch
before continuing, referencing pendingToolUses, block.tool_use_id, and the
surrounding message construction (ToolResultBlock/ToolUseMessages) so debugging
out-of-order or missing entries is easier.
The useEffect for syncing task tab statuses was placed after loading/error early returns, causing "Rendered more hooks than during the previous render" when the component re-rendered after initial load. Hooks must be called unconditionally — moved it before the early return guards. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ing tasks Two bugs: - stop_task: ClaudeSDKClient.stop_task() is async but was called without await, producing "coroutine was never awaited" warning and silently failing to stop the task - transcript unavailable while running: _task_outputs only populated on task:completed, and fallback search only looked in ~/.claude/projects/ but SDK writes output files to /tmp/claude-*/ during execution Fixes: - Add await to self._client.stop_task(task_id) in SessionWorker - Extend fallback search to include /tmp/ where SDK writes during execution - Capture agent_transcript_path from SubagentStop hook events in _task_outputs - Update path validation to allow both ~/.claude/ and /tmp/ Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The SDK writes the subagent transcript JSONL incrementally as the agent
runs. By finding and registering the transcript path when SubagentStart
fires (rather than waiting for SubagentStop or task:completed), the
/tasks/{id}/output endpoint can serve partial transcripts while the
background task is still executing. Combined with the frontend's 5s
polling, this gives live visibility into what running agents are doing.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Review Queue Status
Action needed: Fix failing CI checks
|
…een-run events Switch SessionWorker from per-turn receive_response() to persistent receive_messages() stream. Late SDK events (background task completions, agent text responses, hook notifications) now reach the frontend in real-time instead of being buffered until the next user message. Architecture: - SessionWorker splits into persistent _read_messages_forever reader + query submission loop. Messages route to active run queue during turns, between_run_queue otherwise. - Adapter.process_between_run_message() wraps between-run SDK messages as AG-UI events: CUSTOM events pass through directly, text content gets synthetic RUN_STARTED/RUN_FINISHED envelope. - Bridge.stream_between_run_events() feeds the new GET /events persistent SSE endpoint on the runner. - Backend ensureBetweenRunListener goroutine connects to runner GET /events and persists + broadcasts events to frontend SSE subscribers via existing publishLine/persistStreamedEvent. Fixes the "one message behind" bug where background task completions and agent responses to those completions were delayed until the next user message. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| } | ||
| req.Header.Set("Accept", "text/event-stream") | ||
|
|
||
| resp, err := runnerHTTPClient.Do(req) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in network request Critical
- SessionWorker: race _turn_done.wait() against reader task death using asyncio.wait() so mid-turn reader crashes don't hang forever - SessionWorker: add try/except in _read_messages_forever, set _turn_done on crash so main loop unblocks - SessionWorker: set _active_output_queue=None BEFORE sentinel to prevent orphaned messages between ResultMessage and queue swap - SessionWorker: drain between-run queue before putting _SHUTDOWN sentinel (prevents silent drop when queue is full) - Adapter: extract _emit_task_started/_progress/_notification helpers shared by _stream_claude_sdk and process_between_run_message (eliminates ~60 lines of copy-paste) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1. GET /events returns immediately when no worker exists yet — the backend listener loops connecting and disconnecting. Fix: wait up to 60s for the worker to be created. 2. Empty synthetic runs for AssistantMessages with no text content. Fix: check for text content before creating the synthetic run. 3. Task transcripts empty for local_bash tasks — output is raw stdout, not SDK JSONL. Fix: detect raw entries, render as code block. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…rocessing Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…r full streaming Instead of process_between_run_message handling messages one-by-one (which dropped all StreamEvents), pipe the full between-run SDK turn through _stream_claude_sdk — the same adapter code that handles normal runs. This gives between-run turns full real-time text streaming, tool call rendering, and reasoning blocks. Bridge.stream_between_run_events now: - Yields task lifecycle events (CUSTOM) immediately - On first non-task message, opens a synthetic AG-UI run - Pipes all messages through _stream_claude_sdk until ResultMessage - Closes the synthetic run Also adds _emit_task_event unified dispatcher and between_run_queue_get to SessionWorker. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…safety - Add try/finally around synthetic run so RUN_FINISHED is always emitted even if _stream_claude_sdk raises - Extract drain_hook_events() method on adapter — bridge no longer reaches into _hook_event_queue directly - Use _emit_task_event in _stream_claude_sdk (replaces inline isinstance) - Fix type annotation: input_data is Optional[RunAgentInput] - Remove dead process_between_run_message (~100 lines) - Move uuid import out of loop Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
listenBetweenRunEvents only published data: lines but skipped the empty line separators. The browser's EventSource parser needs those to dispatch onmessage — without them, events buffer and never fire. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…fresh
When POST /tasks/{id}/stop succeeds, push a task:stop_requested
CustomEvent into the hook queue. It gets persisted to JSONL and
reaches the frontend via SSE. Survives page refresh.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py (1)
244-256:⚠️ Potential issue | 🟠 MajorClear hook queue at run start to prevent cross-run event leakage.
The
_hook_event_queueis adapter-scoped and persists across runs. If a run ends abnormally or the drain loop doesn't complete, leftover events can bleed into the next run. Clear the queue at the start ofrun()to ensure each run starts fresh.🔧 Proposed fix in run() method
async def run( self, input_data: RunAgentInput, *, message_stream: Any, ) -> AsyncIterator[BaseEvent]: ... # Clear result data and halt flag from any previous run self._last_result_data = None self._halted = False self._halted_tool_call_id = None + # Clear any leftover hook events from previous runs + while not self._hook_event_queue.empty(): + try: + self._hook_event_queue.get_nowait() + except asyncio.QueueEmpty: + break # Initialize state tracking for this run self._current_state = input_data.state🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py` around lines 244 - 256, The _hook_event_queue is adapter-scoped and can carry events between runs; modify the run() method to clear this queue at run start by draining or recreating it so no leftover CustomEvents from a previous run leak into the new run. In run(), before starting the SSE/drain loops and any run-specific initialization, reset self._hook_event_queue (or drain it in a loop) so it is empty; reference the field name _hook_event_queue and the run() method to locate where to add the clearing logic.components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py (1)
339-351:⚠️ Potential issue | 🟠 MajorReturn copies from registry accessors to prevent concurrent mutation.
task_registryandtask_outputsreturn the adapter's live dictionaries. If an endpoint serializes while tasks are updating, it can observe partial data or hit a concurrent modification error.🔧 Proposed fix
`@property` def task_registry(self) -> dict: """Background task metadata tracked by the adapter.""" if self._adapter: - return getattr(self._adapter, "_task_registry", {}) + registry = getattr(self._adapter, "_task_registry", {}) + return {k: dict(v) if isinstance(v, dict) else v for k, v in registry.items()} return {} `@property` def task_outputs(self) -> dict: """Background task output file paths tracked by the adapter.""" if self._adapter: - return getattr(self._adapter, "_task_outputs", {}) + return dict(getattr(self._adapter, "_task_outputs", {})) return {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py` around lines 339 - 351, The task_registry and task_outputs properties currently return the adapter's live dictionaries (via getattr(self._adapter, "_task_registry", {}) and "_task_outputs"), which can lead to concurrent mutation during serialization; change both properties in the ambient_runner.bridge (the task_registry and task_outputs property methods) to return shallow copies of the adapter dicts (e.g., use dict(...) or .copy() on the retrieved _task_registry and _task_outputs) so callers get snapshots instead of references to the live structures while keeping the fallback empty dict behavior.components/runners/ambient-runner/ambient_runner/endpoints/tasks.py (2)
53-55:⚠️ Potential issue | 🟠 MajorDon't expose internal exception details in HTTP responses.
Lines 54-55 and 117-118 return
str(e)in error responses, which can leak internal paths, stack traces, or sensitive information. Log the full error server-side and return a generic message.🔒 Proposed fix
except Exception as e: - logger.error(f"stop_task({task_id}) failed: {e}") - raise HTTPException(status_code=500, detail=str(e)) + logger.exception("stop_task failed for task_id=%s", task_id) + raise HTTPException(status_code=500, detail="Failed to stop task") from NoneApply the same pattern to the exception handler at lines 116-118.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py` around lines 53 - 55, The except handlers in stop_task (and the other handler around lines 116-118) currently return str(e) to clients; change them to log the full exception (e.g., logger.exception(...) or logger.error with stack) and raise HTTPException with a generic message like "Internal server error" or "Failed to stop task" without including e; update the except blocks in the stop_task function and the other error handler to use server-side logging of e and a safe, non-sensitive detail string in HTTPException.
100-114:⚠️ Potential issue | 🟠 MajorMove blocking file operations off the event loop.
stat()andopen()at lines 100 and 107 block the asyncio event loop. Under concurrent requests, this stalls all request handling. Useanyio.to_thread.run_sync()for the blocking operations.🔧 Proposed fix
+import anyio ... - if resolved.stat().st_size > _MAX_OUTPUT_BYTES: - raise HTTPException(status_code=413, detail="Transcript too large") - - try: - entries = [] - with open(resolved) as f: - for line in f: - line = line.strip() - if line: - try: - entries.append(json.loads(line)) - except json.JSONDecodeError: - entries.append({"raw": line}) - return {"task_id": task_id, "output": entries} + def _read_output(): + if resolved.stat().st_size > _MAX_OUTPUT_BYTES: + raise HTTPException(status_code=413, detail="Transcript too large") + entries = [] + with open(resolved) as f: + for raw_line in f: + stripped = raw_line.strip() + if stripped: + try: + entries.append(json.loads(stripped)) + except json.JSONDecodeError: + entries.append({"raw": stripped}) + return entries + + try: + entries = await anyio.to_thread.run_sync(_read_output) + return {"task_id": task_id, "output": entries}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py` around lines 100 - 114, The blocking filesystem calls resolved.stat() and open(resolved) in this endpoint should be moved off the asyncio event loop: call resolved.stat() via anyio.to_thread.run_sync and read the file contents (or iterate lines) via anyio.to_thread.run_sync as well, then perform the JSON parsing (json.loads) and building of entries on the event loop; ensure the size check against _MAX_OUTPUT_BYTES uses the result returned from the threaded stat call and replace the open(...) loop with a to_thread call that returns the file lines so the rest of the logic (stripping, skipping empty lines, JSONDecodeError fallback to {"raw": line}) can remain unchanged.components/backend/websocket/agui_proxy.go (2)
1174-1216:⚠️ Potential issue | 🟠 MajorEscape
taskIDin URL path to prevent path traversal.The
taskIDparameter is inserted directly into the URL without escaping (line 1193). A malicioustaskIDcontaining/or..could manipulate the request path. This applies to all three task handlers.🔒 Proposed fix
+ import "net/url" ... func HandleTaskStop(c *gin.Context) { projectName := c.Param("projectName") sessionName := c.Param("sessionName") taskID := c.Param("taskId") ... runnerURL := getRunnerEndpoint(projectName, sessionName) - targetURL := strings.TrimSuffix(runnerURL, "/") + "/tasks/" + taskID + "/stop" + targetURL := strings.TrimSuffix(runnerURL, "/") + "/tasks/" + url.PathEscape(taskID) + "/stop"Apply the same fix to
HandleTaskOutput(line 1237) andHandleTaskListdoesn't need it since it doesn't use taskID in the path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/backend/websocket/agui_proxy.go` around lines 1174 - 1216, The handlers insert taskID directly into the runner URL path (e.g., in HandleTaskStop) which allows path traversal if taskID contains slashes or ".."; fix by escaping the path segment before composing targetURL (use url.PathEscape on taskID) in HandleTaskStop and likewise update HandleTaskOutput to use the escaped value; HandleTaskList can be left unchanged as it doesn't embed taskID in the path.
1252-1254:⚠️ Potential issue | 🟠 MajorTruncated responses forwarded as success can corrupt client-side JSON parsing.
io.LimitReadersilently truncates at 10MB andc.Dataforwards whatever was read with the original status code. If a transcript exceeds the limit, the frontend receives truncated JSON that fails to parse.🛡️ Proposed fix: detect and report truncation
- // Transcripts can be large; cap at 10 MB. - body, _ := io.ReadAll(io.LimitReader(resp.Body, 10*1024*1024)) - c.Data(resp.StatusCode, resp.Header.Get("Content-Type"), body) + // Transcripts can be large; cap at 10 MB. Read one extra byte to detect truncation. + const maxSize = 10 * 1024 * 1024 + body, _ := io.ReadAll(io.LimitReader(resp.Body, maxSize+1)) + if len(body) > maxSize { + c.JSON(http.StatusRequestEntityTooLarge, gin.H{"error": "Transcript exceeds 10MB limit"}) + return + } + c.Data(resp.StatusCode, resp.Header.Get("Content-Type"), body)Apply the same pattern to
HandleTaskList(line 1290).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/backend/websocket/agui_proxy.go` around lines 1252 - 1254, The resp body is being read via io.LimitReader(10MB) and forwarded with c.Data even when truncated, producing invalid JSON; change the read logic in the proxy handler (the block around the io.LimitReader call in agui_proxy.go) to read up to limit+1 bytes (e.g., use io.LimitReader with 10MB+1) or otherwise detect if the body length exceeds the 10MB cap, and if so return a clear non-200 error response (e.g., 413/502 with an explanatory message) instead of forwarding truncated content; apply the same truncation-detection-and-error-response change to HandleTaskList so both places detect overflow and refuse to forward truncated JSON.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/frontend/src/lib/transform-task-transcript.ts`:
- Around line 118-127: Pending tool_use entries in the pendingToolUses map are
never emitted if no matching tool_result arrives, causing orphaned tool uses to
be lost; at the end of the transform function (after the main loop and before
return, covering the areas around the existing pendingToolUses handling and the
similar code at 141-143) iterate over pendingToolUses and push/render each
remaining entry as a standalone tool use message (use the same shape as created
for tool_use blocks: type "tool_use_block", id, name, input) and mark them as
incomplete/without result (e.g., no result field or a null/resultStatus flag) so
the UI can show in-progress or interrupted tool calls to the user.
In `@components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py`:
- Around line 1214-1232: The recursive rglob scan in the hook handling block
(inside the adapter handling hook_event name "hook:SubagentStart") can be slow;
change the logic in the hook_event handling (the block that computes expected =
f"agent-{agent_id}.jsonl" and iterates base.rglob(expected)) to use a cached
index or a limited-depth search: maintain a per-run cache (e.g.,
self._claude_projects_index or similar) that maps expected filenames to paths
and populate it once (or update incrementally) instead of calling base.rglob on
every SubagentStart, or replace rglob with a bounded glob pattern/controlled
traversal that only checks likely subagent directories; ensure you still call
self._task_outputs.setdefault(agent_id, path) when a match is found and keep
existing behavior for SubagentStop (agent_transcript_path).
In `@components/runners/ambient-runner/ambient_runner/endpoints/events.py`:
- Around line 26-28: When extracting the context in the events endpoint, add a
guard that logs a debug message if bridge._context is missing so it's easier to
trace early connections before set_context() runs; locate the lines that read
bridge = request.app.state.bridge and ctx = getattr(bridge, "_context", None)
and, if ctx is None, call the app logger (or bridge.logger) to emit a debug
message mentioning the missing context and include identifying info (e.g.,
request.client or request.url) before setting thread_id = ctx.session_id if ctx
else ""; keep behavior unchanged otherwise so stream_between_run_events still
returns early.
In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py`:
- Around line 67-79: The rglob currently searches hardcoded search_dirs (/tmp
and ~/.claude/projects) before applying the allowlist, which can be slow or leak
timing information; update the logic around output_path resolution so you first
determine the set of allowed search directories (use the existing
allowlist/allowed_directories rather than the hardcoded search_dirs), iterate
only those allowed directories when running Path.rglob(f"*{task_id}*"), set
output_path on the first match and break, and remove the redundant second
allowlist definition; ensure you reference and use the variables task_id,
output_path and the allowlist/allowed_directories when refactoring.
---
Duplicate comments:
In `@components/backend/websocket/agui_proxy.go`:
- Around line 1174-1216: The handlers insert taskID directly into the runner URL
path (e.g., in HandleTaskStop) which allows path traversal if taskID contains
slashes or ".."; fix by escaping the path segment before composing targetURL
(use url.PathEscape on taskID) in HandleTaskStop and likewise update
HandleTaskOutput to use the escaped value; HandleTaskList can be left unchanged
as it doesn't embed taskID in the path.
- Around line 1252-1254: The resp body is being read via io.LimitReader(10MB)
and forwarded with c.Data even when truncated, producing invalid JSON; change
the read logic in the proxy handler (the block around the io.LimitReader call in
agui_proxy.go) to read up to limit+1 bytes (e.g., use io.LimitReader with
10MB+1) or otherwise detect if the body length exceeds the 10MB cap, and if so
return a clear non-200 error response (e.g., 413/502 with an explanatory
message) instead of forwarding truncated content; apply the same
truncation-detection-and-error-response change to HandleTaskList so both places
detect overflow and refuse to forward truncated JSON.
In `@components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py`:
- Around line 244-256: The _hook_event_queue is adapter-scoped and can carry
events between runs; modify the run() method to clear this queue at run start by
draining or recreating it so no leftover CustomEvents from a previous run leak
into the new run. In run(), before starting the SSE/drain loops and any
run-specific initialization, reset self._hook_event_queue (or drain it in a
loop) so it is empty; reference the field name _hook_event_queue and the run()
method to locate where to add the clearing logic.
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py`:
- Around line 339-351: The task_registry and task_outputs properties currently
return the adapter's live dictionaries (via getattr(self._adapter,
"_task_registry", {}) and "_task_outputs"), which can lead to concurrent
mutation during serialization; change both properties in the
ambient_runner.bridge (the task_registry and task_outputs property methods) to
return shallow copies of the adapter dicts (e.g., use dict(...) or .copy() on
the retrieved _task_registry and _task_outputs) so callers get snapshots instead
of references to the live structures while keeping the fallback empty dict
behavior.
In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py`:
- Around line 53-55: The except handlers in stop_task (and the other handler
around lines 116-118) currently return str(e) to clients; change them to log the
full exception (e.g., logger.exception(...) or logger.error with stack) and
raise HTTPException with a generic message like "Internal server error" or
"Failed to stop task" without including e; update the except blocks in the
stop_task function and the other error handler to use server-side logging of e
and a safe, non-sensitive detail string in HTTPException.
- Around line 100-114: The blocking filesystem calls resolved.stat() and
open(resolved) in this endpoint should be moved off the asyncio event loop: call
resolved.stat() via anyio.to_thread.run_sync and read the file contents (or
iterate lines) via anyio.to_thread.run_sync as well, then perform the JSON
parsing (json.loads) and building of entries on the event loop; ensure the size
check against _MAX_OUTPUT_BYTES uses the result returned from the threaded stat
call and replace the open(...) loop with a to_thread call that returns the file
lines so the rest of the logic (stripping, skipping empty lines, JSONDecodeError
fallback to {"raw": line}) can remain unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6786298a-4616-4178-b2a9-f0c124edc4f5
📒 Files selected for processing (14)
components/backend/websocket/agui_proxy.gocomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/explorer/explorer-panel.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/hooks/use-file-tabs.tscomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/page.tsxcomponents/frontend/src/hooks/agui/event-handlers.tscomponents/frontend/src/lib/transform-task-transcript.tscomponents/runners/ambient-runner/ag_ui_claude_sdk/adapter.pycomponents/runners/ambient-runner/ag_ui_claude_sdk/hooks.pycomponents/runners/ambient-runner/ambient_runner/app.pycomponents/runners/ambient-runner/ambient_runner/bridges/claude/bridge.pycomponents/runners/ambient-runner/ambient_runner/bridges/claude/mock_client.pycomponents/runners/ambient-runner/ambient_runner/bridges/claude/session.pycomponents/runners/ambient-runner/ambient_runner/endpoints/events.pycomponents/runners/ambient-runner/ambient_runner/endpoints/tasks.py
Comment on lines
+118
to
+127
| } else if (block.type === "tool_use" && block.id && block.name) { | ||
| pendingToolUses.set(block.id, { | ||
| type: "tool_use_block", | ||
| id: block.id, | ||
| name: block.name, | ||
| input: (block.input as Record<string, unknown>) ?? {}, | ||
| }) | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Orphaned tool_use blocks are never rendered.
Tool uses stored in pendingToolUses are only emitted when a matching tool_result arrives. For interrupted tasks (e.g., user stops mid-execution) or partial transcripts, any tool_use without a corresponding result is silently lost—the user won't see what tool was being invoked when the task stopped.
Consider draining remaining pendingToolUses entries at the end of the function, rendering them as standalone tool use messages without results, so users can see in-progress tool calls.
💡 Suggested approach
}
+ // Emit any unpaired tool_use blocks so interrupted tool calls are visible
+ for (const toolUse of pendingToolUses.values()) {
+ messages.push({
+ type: "tool_use_messages",
+ toolUseBlock: toolUse,
+ resultBlock: {
+ type: "tool_result_block",
+ tool_use_id: toolUse.id,
+ content: "(no result — task may have been interrupted)",
+ is_error: null,
+ },
+ timestamp: new Date().toISOString(),
+ } satisfies ToolUseMessages)
+ }
+
return messages
}Also applies to: 141-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/frontend/src/lib/transform-task-transcript.ts` around lines 118 -
127, Pending tool_use entries in the pendingToolUses map are never emitted if no
matching tool_result arrives, causing orphaned tool uses to be lost; at the end
of the transform function (after the main loop and before return, covering the
areas around the existing pendingToolUses handling and the similar code at
141-143) iterate over pendingToolUses and push/render each remaining entry as a
standalone tool use message (use the same shape as created for tool_use blocks:
type "tool_use_block", id, name, input) and mark them as incomplete/without
result (e.g., no result field or a null/resultStatus flag) so the UI can show
in-progress or interrupted tool calls to the user.
The ExplorerPanelProps now requires sessionName but the test's defaultProps was missing it, causing TypeScript type check failure. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Main added eager session ID persistence via _on_session_id callback in SessionWorker. Integrated into our _read_messages_forever persistent reader (where session ID capture now happens). Also fixes explorer-panel test missing sessionName prop. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (7)
components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py (1)
350-362:⚠️ Potential issue | 🟡 MinorReturn copies to prevent concurrent modification issues.
These properties return the adapter's live dictionaries directly. If a task updates the registry while an endpoint is serializing it, the endpoint could observe partial data or raise a concurrent-modification error.
Suggested fix
`@property` def task_registry(self) -> dict: """Background task metadata tracked by the adapter.""" if self._adapter: - return getattr(self._adapter, "_task_registry", {}) + registry = getattr(self._adapter, "_task_registry", {}) + return {k: dict(v) if isinstance(v, dict) else v for k, v in registry.items()} return {} `@property` def task_outputs(self) -> dict: """Background task output file paths tracked by the adapter.""" if self._adapter: - return getattr(self._adapter, "_task_outputs", {}) + return dict(getattr(self._adapter, "_task_outputs", {})) return {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py` around lines 350 - 362, The task_registry and task_outputs properties expose the adapter's live dicts (_task_registry and _task_outputs) which can be mutated concurrently; change both properties (task_registry and task_outputs) to return shallow copies (e.g., via dict(...) or .copy()) of the underlying dictionaries when self._adapter is present so callers get a snapshot and avoid concurrent-modification issues while still returning empty dicts when no adapter exists.components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py (2)
244-246:⚠️ Potential issue | 🟠 MajorHook queue persists across runs, risking event misattribution.
The
_hook_event_queueis created once in__init__but the adapter is cached and reused across runs (viaClaudeBridge._ensure_adapter()). If a run ends with the stream on aStreamEvent(whichcontinues at line 1090 before the drain block), any queued hook events remain until the next run drains them—attributing them to the wrong run.Clear the queue at the start of each run:
Suggested fix in run() method
async def run( self, input_data: RunAgentInput, *, message_stream: Any, ) -> AsyncIterator[BaseEvent]: ... # Clear result data and halt flag from any previous run self._last_result_data = None self._halted = False self._halted_tool_call_id = None + # Clear stale hook events from previous runs + while not self._hook_event_queue.empty(): + try: + self._hook_event_queue.get_nowait() + except asyncio.QueueEmpty: + break🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py` around lines 244 - 246, The adapter creates self._hook_event_queue in __init__ and because the adapter instance is cached (via ClaudeBridge._ensure_adapter()) the queue can persist across runs and leak events; modify the run() method to drain/clear self._hook_event_queue at the start of each run (before processing the SSE stream) so any leftover CustomEvents are discarded and won't be attributed to the new run; locate the queue referenced as _hook_event_queue and add a simple loop or asyncio.Queue.clear-equivalent logic at the top of run() to empty it before continuing.
1225-1236: 🧹 Nitpick | 🔵 TrivialRecursive glob on every SubagentStart may be slow.
The
rglobat line 1234 scans~/.claude/projectsrecursively. Whilesetdefaultprevents repeated searches for the sameagent_id, the initial search for each new subagent could introduce latency in sessions with many projects.Consider limiting the search depth or caching the directory structure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py` around lines 1225 - 1236, The recursive rglob search in the SubagentStart handling (hook_event.name == "hook:SubagentStart" and agent_id) using Path.home() / ".claude" / "projects" and base.rglob(expected) can be slow for many projects; modify adapter.py to avoid per-event full recursion by introducing a cached index or limited-depth search: implement a module-level or instance-level cache (e.g., a dict mapping project filenames to paths) populated once (or refreshed periodically) by a single rglob or a non-recursive scan, and then replace the direct base.rglob(expected) loop with a lookup into self._task_outputs or the new cache; alternatively, constrain the search with a limited-depth traversal instead of rglob to reduce latency while still calling self._task_outputs.setdefault(agent_id, path) when found.components/backend/websocket/agui_proxy.go (2)
1193-1193:⚠️ Potential issue | 🟡 MinorEscape
taskIDin URL path to prevent path manipulation.While RBAC is checked before this call, the unescaped
taskIDcould still contain path separators or encoded characters that alter the request destination. Useurl.PathEscape()for defense in depth.+ import "net/url" ... - targetURL := strings.TrimSuffix(runnerURL, "/") + "/tasks/" + taskID + "/stop" + targetURL := strings.TrimSuffix(runnerURL, "/") + "/tasks/" + url.PathEscape(taskID) + "/stop"Also apply to lines 1237 and 1275.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/backend/websocket/agui_proxy.go` at line 1193, The URL path is built by concatenating an unescaped taskID which can contain slashes or encoded chars; update occurrences that build targetURL (the assignment creating targetURL := strings.TrimSuffix(runnerURL, "/") + "/tasks/" + taskID + "/stop" and the similar concatenations at the other two occurrences around lines 1237 and 1275) to use url.PathEscape(taskID) when inserting the task ID into the path (e.g. replace + "/tasks/" + taskID + "/stop" with + "/tasks/" + url.PathEscape(taskID) + "/stop") so the taskID is safely escaped before forming the request URL.
1252-1254:⚠️ Potential issue | 🟠 MajorSilently truncated JSON can break clients.
When
resp.Bodyexceeds the 10MB limit,io.LimitReadersilently truncates the data. The handler then forwards this partial content with the upstream's success status code, causing the frontend to receive invalid JSON that will fail to parse.Detect truncation and return an error response instead:
Suggested fix
- // Transcripts can be large; cap at 10 MB. - body, _ := io.ReadAll(io.LimitReader(resp.Body, 10*1024*1024)) - c.Data(resp.StatusCode, resp.Header.Get("Content-Type"), body) + // Transcripts can be large; cap at 10 MB + 1 byte to detect truncation. + const maxSize = 10 * 1024 * 1024 + body, _ := io.ReadAll(io.LimitReader(resp.Body, maxSize+1)) + if len(body) > maxSize { + c.JSON(http.StatusRequestEntityTooLarge, gin.H{"error": "Response too large"}) + return + } + c.Data(resp.StatusCode, resp.Header.Get("Content-Type"), body)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/backend/websocket/agui_proxy.go` around lines 1252 - 1254, The handler currently uses io.LimitReader to cap transcripts at 10*1024*1024 but forwards whatever bytes are read (possibly truncated) with the upstream status, producing invalid JSON for clients; before reading, check the upstream response size (use resp.ContentLength or parse resp.Header.Get("Content-Length")) and if it exceeds the 10*1024*1024 limit return an error response (e.g., 413 Payload Too Large or 502 with an explanatory message) instead of proceeding to io.LimitReader and c.Data; update the logic around resp, io.LimitReader, body and the c.Data call to perform this pre-check and short-circuit on over-limit responses.components/runners/ambient-runner/ambient_runner/endpoints/tasks.py (1)
53-55:⚠️ Potential issue | 🟠 MajorDo not expose raw exception details to clients.
str(e)in the HTTP 500 response can leak internal implementation details, file paths, or stack traces. Log the full error server-side and return a generic message.Suggested fix
except Exception as e: - logger.error(f"stop_task({task_id}) failed: {e}") - raise HTTPException(status_code=500, detail=str(e)) + logger.exception("stop_task failed for task_id=%s", task_id) + raise HTTPException(status_code=500, detail="Failed to stop task") from NoneApply the same pattern to lines 116-118.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py` around lines 53 - 55, The except blocks currently return raw exception text to clients (e.g., in stop_task(task_id) they do raise HTTPException(..., detail=str(e))) which can leak internals; instead log the full exception server-side (use logger.error with exc_info=True or include the exception object) and raise HTTPException(status_code=500, detail="Internal server error") to return a generic message to the client; apply this same change to the other except block referenced around lines 116-118 so no raw exception text is sent in any HTTP 500 responses.components/frontend/src/lib/transform-task-transcript.ts (1)
31-31:⚠️ Potential issue | 🟠 MajorFlush unfinished tool calls before emitting the final transcript.
pendingToolUsesis only rendered on a matchingtool_result. For running tasks or tasks stopped mid-tool, the remaining entry is discarded at Line 143, so the transcript omits the exact tool invocation the user is trying to inspect. Keep the tool-use timestamp with the pending block and flush leftovers before theresult_message/finalreturn.💡 Suggested fix
- const pendingToolUses = new Map<string, ToolUseBlock>() + const pendingToolUses = new Map< + string, + { toolUseBlock: ToolUseBlock; timestamp: string } + >() + + const flushPendingToolUses = () => { + for (const pendingToolUse of pendingToolUses.values()) { + messages.push({ + type: "tool_use_messages", + toolUseBlock: pendingToolUse.toolUseBlock, + resultBlock: { + type: "tool_result_block", + tool_use_id: pendingToolUse.toolUseBlock.id, + content: null, + is_error: null, + } satisfies ToolResultBlock, + timestamp: pendingToolUse.timestamp, + } satisfies ToolUseMessages) + } + pendingToolUses.clear() + } ... - const toolUse = pendingToolUses.get(block.tool_use_id) - if (toolUse) { + const pendingToolUse = pendingToolUses.get(block.tool_use_id) + if (pendingToolUse) { const resultContent = typeof block.content === "string" ? block.content : block.content ?? null messages.push({ type: "tool_use_messages", - toolUseBlock: toolUse, + toolUseBlock: pendingToolUse.toolUseBlock, resultBlock: { type: "tool_result_block", tool_use_id: block.tool_use_id, content: resultContent, is_error: block.is_error ?? false, @@ - pendingToolUses.set(block.id, { - type: "tool_use_block", - id: block.id, - name: block.name, - input: (block.input as Record<string, unknown>) ?? {}, - }) + pendingToolUses.set(block.id, { + toolUseBlock: { + type: "tool_use_block", + id: block.id, + name: block.name, + input: (block.input as Record<string, unknown>) ?? {}, + }, + timestamp, + }) } } } if (entryType === "result") { + flushPendingToolUses() messages.push({ type: "result_message", subtype: "task_result", duration_ms: (entry.duration_ms as number) ?? 0, duration_api_ms: (entry.duration_api_ms as number) ?? 0, @@ } } + flushPendingToolUses() return messagesAlso applies to: 71-87, 118-125, 129-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/lib/transform-task-transcript.ts` at line 31, The pendingToolUses Map (pendingToolUses) can contain unfinished ToolUseBlock entries that are currently dropped if no matching tool_result arrives; fix by flushing any remaining pendingToolUses before emitting the final result_message/return in transformTaskTranscript (or the top-level transform function): iterate pendingToolUses.values(), convert each ToolUseBlock into the same transcript block shape used when a tool_result arrives (preserving the original timestamp and other fields), append those blocks to the output/emitted transcript, then clear pendingToolUses; also remove the code path that silently discards entries so this flush always runs prior to the final return.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/backend/websocket/agui_proxy.go`:
- Around line 1290-1291: The code reads up to 1MB and blindly writes that to
clients (resp.Body -> body -> c.Data), which can return truncated/corrupted
JSON; change the read to detect truncation by reading up to limit+1 (use
io.LimitReader(resp.Body, limit+1) or read limit bytes then attempt to read one
more byte), set a boolean when the response exceeded the 1MB limit, and if
truncated return an error response (e.g., appropriate status code and JSON
error) instead of piping the partial body to clients; update the logic around
resp.Body, body, and c.Data to perform this check and only call c.Data with the
body when not truncated.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/explorer/explorer-panel.tsx:
- Around line 86-88: The computation of runningTaskCount currently recreates an
array and filters on every render (using backgroundTasks and
Array.from(...).filter(...)), which could be expensive for large lists; wrap
this computation in a React useMemo that depends on backgroundTasks to memoize
the count (reference the runningTaskCount constant and backgroundTasks variable)
so the Array.from and filter only run when backgroundTasks changes.
In `@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/page.tsx:
- Around line 1542-1547: The effect syncing AG-UI task status uses
aguiState.backgroundTasks and fileTabs.updateTaskStatus but the linter warns
about missing fileTabs; to fix, explicitly document the intent by adding an
ESLint suppression for the exhaustive-deps rule above the useEffect (e.g., an
eslint-disable-next-line or eslint-disable comment for
react-hooks/exhaustive-deps) and keep the dependency array as
[aguiState.backgroundTasks, fileTabs.updateTaskStatus]; reference the useEffect
block and the symbols aguiState.backgroundTasks and fileTabs.updateTaskStatus
when making this change so the suppression clearly applies to this effect.
In `@components/frontend/src/lib/transform-task-transcript.ts`:
- Around line 35-38: The filter that builds rawEntries is too strict (it
requires Object.keys(e).length === 1), so change the condition in the
entries.filter call to detect raw entries by checking that the entry has a raw
field of type string (e.g., "raw" in e && typeof (e as any).raw === "string")
instead of exact key count; also update the similar gating logic later in the
same file (the branch that currently drops entries with no type around the other
raw-check) to use the same typeof raw string check so API payloads with extra
metadata aren't misclassified — look for the rawEntries variable and the later
block that inspects entry.type/raw in transform-task-transcript.ts and replace
the Object.keys(...) checks with a typeof raw === "string" check.
In `@components/runners/ambient-runner/ag_ui_claude_sdk/hooks.py`:
- Around line 102-114: The loop-created async function _callback captures the
loop variable name by reference so every HookMatcher ends up using the final
name; fix this by binding the current name into the closure (e.g., add name: str
as a default parameter to _callback) and update _forward_hook_as_custom_event to
accept a hook_name parameter, then pass that bound name when calling
_forward_hook_as_custom_event; ensure hooks[name] =
[HookMatcher(hooks=[_callback])] still registers the correct per-name callback.
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/session.py`:
- Around line 277-284: The reader except block currently logs and sets
self._turn_done but loses the original exception; modify the except Exception as
exc handler in the reader (where logger.error and self._turn_done.set() are
called) to store the exception on the session object (e.g., self._reader_exc =
exc) before setting _turn_done, and then update the main loop code that raises
WorkerError/RuntimeError("SDK message reader died") (the code referencing
WorkerError and that RuntimeError message) to check for self._reader_exc and
either re-raise it or include it as the __cause__ or wrapped inner exception
when constructing the WorkerError so the original exception context is
preserved.
In `@components/runners/ambient-runner/ambient_runner/endpoints/events.py`:
- Around line 26-28: When retrieving the bridge context for thread_id (bridge =
request.app.state.bridge; ctx = getattr(bridge, "_context", None); thread_id =
ctx.session_id if ctx else ""), add a debug log when ctx is None to indicate a
connection occurred before lifespan startup and that thread_id will be empty;
use the app or module logger (e.g., request.app.logger or the module logger) to
emit a concise debug message that includes that bridge/_context is missing and
downstream stream_between_run_events will receive an empty thread_id to aid
troubleshooting.
In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py`:
- Around line 105-118: The current async handler performs blocking file I/O
(open/read/stat) on the event loop when reading the task output (variables
resolved, task_id, entries); move the whole file-reading and JSON-parsing logic
into a synchronous helper (e.g., _read_output_file(resolved)) and call it from
the async handler using anyio.to_thread.run_sync so the blocking work runs in a
worker thread, propagate exceptions back to the handler and keep the same
JSONDecodeError fallback behavior (append {"raw": line}) and the same
HTTPException error handling in the async context.
- Around line 36-50: Current code accesses bridge._adapter internals
(adapter._hook_event_queue, adapter._task_registry) directly which can act on a
stale adapter (see mark_dirty) and is inconsistent with other usage like
bridge.task_outputs; add a new method on the Bridge class such as
emit_stop_requested(task_id) that encapsulates pushing the CustomEvent to the
hook/event queue and updating the task registry, then replace the direct
accesses in ambient_runner/endpoints/tasks.py with a single call to
bridge.emit_stop_requested(task_id) so the bridge controls adapter lookup and
mutation safely.
---
Duplicate comments:
In `@components/backend/websocket/agui_proxy.go`:
- Line 1193: The URL path is built by concatenating an unescaped taskID which
can contain slashes or encoded chars; update occurrences that build targetURL
(the assignment creating targetURL := strings.TrimSuffix(runnerURL, "/") +
"/tasks/" + taskID + "/stop" and the similar concatenations at the other two
occurrences around lines 1237 and 1275) to use url.PathEscape(taskID) when
inserting the task ID into the path (e.g. replace + "/tasks/" + taskID + "/stop"
with + "/tasks/" + url.PathEscape(taskID) + "/stop") so the taskID is safely
escaped before forming the request URL.
- Around line 1252-1254: The handler currently uses io.LimitReader to cap
transcripts at 10*1024*1024 but forwards whatever bytes are read (possibly
truncated) with the upstream status, producing invalid JSON for clients; before
reading, check the upstream response size (use resp.ContentLength or parse
resp.Header.Get("Content-Length")) and if it exceeds the 10*1024*1024 limit
return an error response (e.g., 413 Payload Too Large or 502 with an explanatory
message) instead of proceeding to io.LimitReader and c.Data; update the logic
around resp, io.LimitReader, body and the c.Data call to perform this pre-check
and short-circuit on over-limit responses.
In `@components/frontend/src/lib/transform-task-transcript.ts`:
- Line 31: The pendingToolUses Map (pendingToolUses) can contain unfinished
ToolUseBlock entries that are currently dropped if no matching tool_result
arrives; fix by flushing any remaining pendingToolUses before emitting the final
result_message/return in transformTaskTranscript (or the top-level transform
function): iterate pendingToolUses.values(), convert each ToolUseBlock into the
same transcript block shape used when a tool_result arrives (preserving the
original timestamp and other fields), append those blocks to the output/emitted
transcript, then clear pendingToolUses; also remove the code path that silently
discards entries so this flush always runs prior to the final return.
In `@components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py`:
- Around line 244-246: The adapter creates self._hook_event_queue in __init__
and because the adapter instance is cached (via ClaudeBridge._ensure_adapter())
the queue can persist across runs and leak events; modify the run() method to
drain/clear self._hook_event_queue at the start of each run (before processing
the SSE stream) so any leftover CustomEvents are discarded and won't be
attributed to the new run; locate the queue referenced as _hook_event_queue and
add a simple loop or asyncio.Queue.clear-equivalent logic at the top of run() to
empty it before continuing.
- Around line 1225-1236: The recursive rglob search in the SubagentStart
handling (hook_event.name == "hook:SubagentStart" and agent_id) using
Path.home() / ".claude" / "projects" and base.rglob(expected) can be slow for
many projects; modify adapter.py to avoid per-event full recursion by
introducing a cached index or limited-depth search: implement a module-level or
instance-level cache (e.g., a dict mapping project filenames to paths) populated
once (or refreshed periodically) by a single rglob or a non-recursive scan, and
then replace the direct base.rglob(expected) loop with a lookup into
self._task_outputs or the new cache; alternatively, constrain the search with a
limited-depth traversal instead of rglob to reduce latency while still calling
self._task_outputs.setdefault(agent_id, path) when found.
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py`:
- Around line 350-362: The task_registry and task_outputs properties expose the
adapter's live dicts (_task_registry and _task_outputs) which can be mutated
concurrently; change both properties (task_registry and task_outputs) to return
shallow copies (e.g., via dict(...) or .copy()) of the underlying dictionaries
when self._adapter is present so callers get a snapshot and avoid
concurrent-modification issues while still returning empty dicts when no adapter
exists.
In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py`:
- Around line 53-55: The except blocks currently return raw exception text to
clients (e.g., in stop_task(task_id) they do raise HTTPException(...,
detail=str(e))) which can leak internals; instead log the full exception
server-side (use logger.error with exc_info=True or include the exception
object) and raise HTTPException(status_code=500, detail="Internal server error")
to return a generic message to the client; apply this same change to the other
except block referenced around lines 116-118 so no raw exception text is sent in
any HTTP 500 responses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1d535c8a-4289-42e7-9671-cd1051597f3a
📒 Files selected for processing (15)
components/backend/websocket/agui_proxy.gocomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/explorer/__tests__/explorer-panel.test.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/explorer/explorer-panel.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/hooks/use-file-tabs.tscomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/page.tsxcomponents/frontend/src/hooks/agui/event-handlers.tscomponents/frontend/src/lib/transform-task-transcript.tscomponents/runners/ambient-runner/ag_ui_claude_sdk/adapter.pycomponents/runners/ambient-runner/ag_ui_claude_sdk/hooks.pycomponents/runners/ambient-runner/ambient_runner/app.pycomponents/runners/ambient-runner/ambient_runner/bridges/claude/bridge.pycomponents/runners/ambient-runner/ambient_runner/bridges/claude/mock_client.pycomponents/runners/ambient-runner/ambient_runner/bridges/claude/session.pycomponents/runners/ambient-runner/ambient_runner/endpoints/events.pycomponents/runners/ambient-runner/ambient_runner/endpoints/tasks.py
Comment on lines
+1290
to
+1291
| body, _ := io.ReadAll(io.LimitReader(resp.Body, 1024*1024)) | ||
| c.Data(resp.StatusCode, resp.Header.Get("Content-Type"), body) |
Contributor
There was a problem hiding this comment.
Same truncation issue applies to task list endpoint.
Apply the same truncation detection pattern here to avoid returning corrupted JSON to clients.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/backend/websocket/agui_proxy.go` around lines 1290 - 1291, The
code reads up to 1MB and blindly writes that to clients (resp.Body -> body ->
c.Data), which can return truncated/corrupted JSON; change the read to detect
truncation by reading up to limit+1 (use io.LimitReader(resp.Body, limit+1) or
read limit bytes then attempt to read one more byte), set a boolean when the
response exceeded the 1MB limit, and if truncated return an error response
(e.g., appropriate status code and JSON error) instead of piping the partial
body to clients; update the logic around resp.Body, body, and c.Data to perform
this check and only call c.Data with the body when not truncated.
Comment on lines
+86
to
+88
| const runningTaskCount = backgroundTasks | ||
| ? Array.from(backgroundTasks.values()).filter((t) => t.status === "running").length | ||
| : 0; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider memoizing runningTaskCount for larger task lists.
The Array.from() and filter() run on every render. For typical usage this is fine, but if task counts grow large, consider wrapping in useMemo.
♻️ Optional: memoize the count
+import { useMemo } from "react";
...
- const runningTaskCount = backgroundTasks
- ? Array.from(backgroundTasks.values()).filter((t) => t.status === "running").length
- : 0;
+ const runningTaskCount = useMemo(() => {
+ if (!backgroundTasks) return 0;
+ return Array.from(backgroundTasks.values()).filter((t) => t.status === "running").length;
+ }, [backgroundTasks]);📝 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
| const runningTaskCount = backgroundTasks | |
| ? Array.from(backgroundTasks.values()).filter((t) => t.status === "running").length | |
| : 0; | |
| import { useMemo } from "react"; | |
| // ... other imports ... | |
| const runningTaskCount = useMemo(() => { | |
| if (!backgroundTasks) return 0; | |
| return Array.from(backgroundTasks.values()).filter((t) => t.status === "running").length; | |
| }, [backgroundTasks]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/explorer/explorer-panel.tsx
around lines 86 - 88, The computation of runningTaskCount currently recreates an
array and filters on every render (using backgroundTasks and
Array.from(...).filter(...)), which could be expensive for large lists; wrap
this computation in a React useMemo that depends on backgroundTasks to memoize
the count (reference the runningTaskCount constant and backgroundTasks variable)
so the Array.from and filter only run when backgroundTasks changes.
components/frontend/src/app/projects/[name]/sessions/[sessionName]/page.tsx
Show resolved
Hide resolved
Comment on lines
+35
to
+38
| const rawEntries = entries.filter((e) => "raw" in e && Object.keys(e).length === 1) | ||
| if (rawEntries.length > 0 && rawEntries.length === entries.length) { | ||
| // All entries are raw text — this is a bash task output | ||
| const rawText = rawEntries.map((e) => e.raw as string).join("\n") |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Don't gate raw transcripts on an exact key count.
At Line 35 this only recognizes { raw: "..." }. Because TaskOutputEntry is an API payload, any extra metadata field on a raw entry makes the check fail; the same entry then reaches Lines 50-52, has no type, and gets dropped. Narrow on a string raw field instead of Object.keys(e).length === 1.
💡 Suggested fix
- const rawEntries = entries.filter((e) => "raw" in e && Object.keys(e).length === 1)
+ const rawEntries = entries.filter(
+ (e): e is TaskOutputEntry & { raw: string } => typeof e.raw === "string",
+ )Also applies to: 50-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/frontend/src/lib/transform-task-transcript.ts` around lines 35 -
38, The filter that builds rawEntries is too strict (it requires
Object.keys(e).length === 1), so change the condition in the entries.filter call
to detect raw entries by checking that the entry has a raw field of type string
(e.g., "raw" in e && typeof (e as any).raw === "string") instead of exact key
count; also update the similar gating logic later in the same file (the branch
that currently drops entries with no type around the other raw-check) to use the
same typeof raw string check so API payloads with extra metadata aren't
misclassified — look for the rawEntries variable and the later block that
inspects entry.type/raw in transform-task-transcript.ts and replace the
Object.keys(...) checks with a typeof raw === "string" check.
Comment on lines
+102
to
+114
| for name in sorted(hook_names): | ||
|
|
||
| async def _callback( | ||
| hook_input: dict[str, Any], | ||
| tool_use_id: str | None = None, | ||
| context: Any = None, | ||
| _q: asyncio.Queue = queue, | ||
| ) -> dict: | ||
| return await _forward_hook_as_custom_event( | ||
| hook_input, tool_use_id or "", _q | ||
| ) | ||
|
|
||
| hooks[name] = [HookMatcher(hooks=[_callback])] |
Contributor
There was a problem hiding this comment.
Closure captures loop variable by reference — all hooks will report the wrong event name.
Each _callback closure captures name by reference, not by value. Since the loop rebinds name on each iteration, all callbacks will use the final value of name (the last hook in sorted order). This breaks event identification.
🐛 Proposed fix: capture `name` as a default parameter
for name in sorted(hook_names):
async def _callback(
hook_input: dict[str, Any],
tool_use_id: str | None = None,
context: Any = None,
_q: asyncio.Queue = queue,
+ _name: str = name, # Capture by value
) -> dict:
return await _forward_hook_as_custom_event(
- hook_input, tool_use_id or "", _q
+ hook_input, tool_use_id or "", _q, _name
)
hooks[name] = [HookMatcher(hooks=[_callback])]Also update _forward_hook_as_custom_event to accept the hook name:
async def _forward_hook_as_custom_event(
hook_input: dict[str, Any],
tool_use_id: str,
queue: asyncio.Queue,
+ hook_name: str,
) -> dict:
- event_name = hook_input.get("hook_event_name", "unknown")
+ event_name = hook_input.get("hook_event_name", hook_name)🧰 Tools
🪛 Ruff (0.15.7)
[warning] 107-107: Unused function argument: context
(ARG001)
[warning] 107-107: Dynamically typed expressions (typing.Any) are disallowed in context
(ANN401)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/runners/ambient-runner/ag_ui_claude_sdk/hooks.py` around lines 102
- 114, The loop-created async function _callback captures the loop variable name
by reference so every HookMatcher ends up using the final name; fix this by
binding the current name into the closure (e.g., add name: str as a default
parameter to _callback) and update _forward_hook_as_custom_event to accept a
hook_name parameter, then pass that bound name when calling
_forward_hook_as_custom_event; ensure hooks[name] =
[HookMatcher(hooks=[_callback])] still registers the correct per-name callback.
Comment on lines
+277
to
+284
| except Exception as exc: | ||
| logger.error( | ||
| "[SessionWorker] Reader stream error for thread=%s: %s", | ||
| self.thread_id, | ||
| exc, | ||
| ) | ||
| # Unblock the main loop if it's waiting on a turn | ||
| self._turn_done.set() |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider preserving error context when reader stream fails.
When the reader catches an exception, it logs and sets _turn_done, but the main loop only sees that the reader exited—it doesn't get the original exception cause. This makes debugging harder since the queued WorkerError at line 197-199 wraps a generic RuntimeError("SDK message reader died") rather than the actual exception.
♻️ Optional: store and propagate the reader exception
+ self._reader_exception: Optional[Exception] = None
...
except Exception as exc:
+ self._reader_exception = exc
logger.error(...)
self._turn_done.set()Then at line 197-199:
- await output_queue.put(WorkerError(
- RuntimeError("SDK message reader died")
- ))
+ await output_queue.put(WorkerError(
+ self._reader_exception or RuntimeError("SDK message reader died")
+ ))🧰 Tools
🪛 Ruff (0.15.7)
[warning] 277-277: Do not catch blind exception: Exception
(BLE001)
[warning] 278-282: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/session.py`
around lines 277 - 284, The reader except block currently logs and sets
self._turn_done but loses the original exception; modify the except Exception as
exc handler in the reader (where logger.error and self._turn_done.set() are
called) to store the exception on the session object (e.g., self._reader_exc =
exc) before setting _turn_done, and then update the main loop code that raises
WorkerError/RuntimeError("SDK message reader died") (the code referencing
WorkerError and that RuntimeError message) to check for self._reader_exc and
either re-raise it or include it as the __cause__ or wrapped inner exception
when constructing the WorkerError so the original exception context is
preserved.
Comment on lines
+26
to
+28
| bridge = request.app.state.bridge | ||
| ctx = getattr(bridge, "_context", None) | ||
| thread_id = ctx.session_id if ctx else "" |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Empty thread_id fallback is safe but consider debug logging.
If _context is None (e.g., client connects before lifespan startup completes), thread_id becomes empty string. The downstream stream_between_run_events handles this gracefully, but a debug log would aid troubleshooting connection timing issues.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/runners/ambient-runner/ambient_runner/endpoints/events.py` around
lines 26 - 28, When retrieving the bridge context for thread_id (bridge =
request.app.state.bridge; ctx = getattr(bridge, "_context", None); thread_id =
ctx.session_id if ctx else ""), add a debug log when ctx is None to indicate a
connection occurred before lifespan startup and that thread_id will be empty;
use the app or module logger (e.g., request.app.logger or the module logger) to
emit a concise debug message that includes that bridge/_context is missing and
downstream stream_between_run_events will receive an empty thread_id to aid
troubleshooting.
Comment on lines
+36
to
+50
| adapter = getattr(bridge, "_adapter", None) | ||
| if adapter: | ||
| from ag_ui.core import CustomEvent, EventType | ||
|
|
||
| adapter._hook_event_queue.put_nowait( | ||
| CustomEvent( | ||
| type=EventType.CUSTOM, | ||
| name="task:stop_requested", | ||
| value={"task_id": task_id}, | ||
| ) | ||
| ) | ||
| # Also update the registry so /tasks list reflects it | ||
| existing = adapter._task_registry.get(task_id, {}) | ||
| existing["status"] = "stopping" | ||
| adapter._task_registry[task_id] = existing |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Directly accessing adapter internals bypasses the bridge's properties.
Lines 40 and 48-50 access adapter._hook_event_queue and adapter._task_registry directly, while other code uses bridge.task_outputs (line 62). This creates an inconsistent access pattern. If the bridge's _adapter reference changes (e.g., during mark_dirty()), this code could operate on a stale adapter.
Consider adding a bridge.emit_stop_requested(task_id) method to encapsulate this logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py` around
lines 36 - 50, Current code accesses bridge._adapter internals
(adapter._hook_event_queue, adapter._task_registry) directly which can act on a
stale adapter (see mark_dirty) and is inconsistent with other usage like
bridge.task_outputs; add a new method on the Bridge class such as
emit_stop_requested(task_id) that encapsulates pushing the CustomEvent to the
hook/event queue and updating the task registry, then replace the direct
accesses in ambient_runner/endpoints/tasks.py with a single call to
bridge.emit_stop_requested(task_id) so the bridge controls adapter lookup and
mutation safely.
Comment on lines
+105
to
+118
| try: | ||
| entries = [] | ||
| with open(resolved) as f: | ||
| for line in f: | ||
| line = line.strip() | ||
| if line: | ||
| try: | ||
| entries.append(json.loads(line)) | ||
| except json.JSONDecodeError: | ||
| entries.append({"raw": line}) | ||
| return {"task_id": task_id, "output": entries} | ||
| except Exception as e: | ||
| logger.error(f"Failed to read output for task {task_id}: {e}") | ||
| raise HTTPException(status_code=500, detail=str(e)) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Blocking I/O in async handler can stall the event loop.
open(), stat(), and the file read at lines 100-114 are blocking operations. Under concurrent requests, these block the asyncio event loop. Use anyio.to_thread.run_sync() to run file operations in a worker thread.
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 107-107: Async functions should not open files with blocking methods like open
(ASYNC230)
[warning] 109-109: for loop variable line overwritten by assignment target
(PLW2901)
[warning] 115-115: Consider moving this statement to an else block
(TRY300)
[warning] 116-116: Do not catch blind exception: Exception
(BLE001)
[warning] 117-117: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
[warning] 117-117: Logging statement uses f-string
(G004)
[warning] 118-118: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py` around
lines 105 - 118, The current async handler performs blocking file I/O
(open/read/stat) on the event loop when reading the task output (variables
resolved, task_id, entries); move the whole file-reading and JSON-parsing logic
into a synchronous helper (e.g., _read_output_file(resolved)) and call it from
the async handler using anyio.to_thread.run_sync so the blocking work runs in a
worker thread, propagate exceptions back to the handler and keep the same
JSONDecodeError fallback behavior (append {"raw": line}) and the same
HTTPException error handling in the async context.
When a task finishes before the stop signal arrives, the SDK throws an error. Check task_registry status first and return a friendly 200 response instead of 500. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Runner: - Check task status before calling stop_task — return 200 for any non-running status (covers "completed", "killed", "stopped", etc.) - If stop_task throws (task already dead), treat as success — emit task:stop_requested event and update registry regardless Frontend: - Track stopped task IDs locally in BackgroundTasksTab so the UI updates immediately when stop API returns (doesn't wait for the event stream to drain the hook queue) - Override displayed status to "stopped" for locally-stopped tasks Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
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)
components/runners/ambient-runner/ambient_runner/bridges/claude/mock_client.py (1)
74-77:⚠️ Potential issue | 🟠 MajorThe persistent mock only ever produces one fixture stream.
SessionWorkerkeeps one long-livedreceive_messages()iterator. This implementation starts by replayingreceive_response()against the current_prompt(still""on startup), then parks forever on a freshEvent(). Laterquery()calls only mutate_prompt; they never wake the reader, so mocked multi-turn/background-task flows stall after startup or turn 1.♻️ Make the mock stream wait for each query and replay per prompt
class MockClaudeSDKClient: def __init__(self, options: Any = None) -> None: self._prompt = "" self._interrupted = False + self._query_ready = asyncio.Event() async def query(self, prompt: str, session_id: Optional[str] = None) -> None: logger.info("[MockSDK] query: %r", prompt[:80]) self._prompt = prompt self._interrupted = False + self._query_ready.set() async def receive_messages(self) -> AsyncIterator[Any]: - """Persistent message stream — replays fixture then blocks forever.""" - async for msg in self.receive_response(): - yield msg - # Simulate persistent stream: block until cancelled - await asyncio.Event().wait() + """Persistent message stream — waits for each query and replays its fixture.""" + while True: + await self._query_ready.wait() + self._query_ready.clear() + async for msg in self.receive_response(): + yield msgAlso applies to: 105-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ambient_runner/bridges/claude/mock_client.py` around lines 74 - 77, The mock client currently stores a single Event and a persistent prompt so receive_messages()/receive_response() replays only the initial fixture and then blocks; fix by making query(prompt, ...) set self._prompt, reset/replace a per-query asyncio.Event (e.g., create new self._ready_event) and call self._ready_event.set() to wake the reader; update receive_messages() to await the current per-query event, call receive_response() for the current self._prompt, then clear/replace the event and continue so each query() produces a fresh replayed stream; reference methods/attributes: query, receive_messages, receive_response, self._prompt, self._interrupted, and the Event instance.
♻️ Duplicate comments (7)
components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py (1)
350-362:⚠️ Potential issue | 🟠 MajorReturn snapshots from
task_registryandtask_outputs.These properties currently expose the adapter's live dictionaries.
GET /tasksandGET /tasks/{id}/outputcan then iterate while_emit_task_*is mutating the same mappings, which risks partial reads orRuntimeError: dictionary changed size during iteration. Copy on read instead of returning the live objects.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py` around lines 350 - 362, task_registry and task_outputs currently return the adapter's live dicts which can be mutated concurrently; change their getters (properties task_registry and task_outputs) to return snapshots (e.g., shallow copies via dict(...) or .copy()) of the adapter attributes _task_registry and _task_outputs when self._adapter is present so callers iterate safely while _emit_task_* may mutate the originals; ensure you still return {} when adapter is missing.components/backend/websocket/agui_proxy.go (3)
1192-1193:⚠️ Potential issue | 🔴 CriticalEscape
taskIDbefore building the runner URL.
taskIDcomes straight from the route and is interpolated into/tasks/{taskID}/...verbatim. Reserved characters like/or?can rewrite the proxied path. Useurl.PathEscape(taskID)for both stop/output URLs.Possible fix
+import "net/url" ... - targetURL := strings.TrimSuffix(runnerURL, "/") + "/tasks/" + taskID + "/stop" + escapedTaskID := url.PathEscape(taskID) + targetURL := strings.TrimSuffix(runnerURL, "/") + "/tasks/" + escapedTaskID + "/stop" ... - targetURL := strings.TrimSuffix(runnerURL, "/") + "/tasks/" + taskID + "/output" + escapedTaskID := url.PathEscape(taskID) + targetURL := strings.TrimSuffix(runnerURL, "/") + "/tasks/" + escapedTaskID + "/output"Also applies to: 1236-1237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/backend/websocket/agui_proxy.go` around lines 1192 - 1193, The taskID is interpolated into the runner path without escaping, so update creation of the stop and output target URLs to use url.PathEscape(taskID) before concatenation; locate the code around getRunnerEndpoint(projectName, sessionName) where targetURL is built (and the similar block at lines handling output URL) and replace direct use of taskID with the escaped value so reserved characters cannot alter the proxied path.
1252-1254:⚠️ Potential issue | 🟠 MajorReject oversized upstream bodies instead of forwarding partial JSON.
io.LimitReadersilently truncates the runner response, andc.Dataforwards that partial body with the upstream status code. Once a transcript or task list exceeds the cap, the frontend gets invalid JSON as an apparent success. Readlimit+1bytes and return an error if the extra byte exists.Also applies to: 1290-1291
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/backend/websocket/agui_proxy.go` around lines 1252 - 1254, The current code uses io.LimitReader to read up to 10MB and forwards that partial body via c.Data, which can produce truncated/invalid JSON; change the read logic in the handler that reads resp.Body (where body := io.ReadAll(io.LimitReader(resp.Body, 10*1024*1024)) and the similar block near the second occurrence) to read limit+1 bytes (e.g., read into a buffer of size 10*1024*1024+1 or use io.ReadFull/ReadAtMost), detect if the extra byte exists, and if so return a clear error response (non-2xx, e.g., 413/502 with an explanatory message) instead of calling c.Data with a truncated payload; keep using resp.Header for Content-Type only when forwarding the full body.
1192-1193:⚠️ Potential issue | 🟠 MajorRefresh session routing state in the task handlers.
These handlers call
getRunnerEndpoint()without updatingsessionLastSeenor repopulatingsessionPortMap. After a backend restart—or aftercleanupStaleSessions()prunes the cache—/agui/tasks*falls back toDefaultRunnerPortand misses non-default runners. Prime the cache in each handler before resolving the URL.Also applies to: 1236-1237, 1274-1275
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/backend/websocket/agui_proxy.go` around lines 1192 - 1193, The task handlers call getRunnerEndpoint(projectName, sessionName) without refreshing routing cache, so after restarts or cleanupStaleSessions() they can fall back to DefaultRunnerPort; before resolving runnerURL, update sessionLastSeen for the session and repopulate sessionPortMap (or call a helper like ensureSessionCached/refreshSessionRouting if available) so the correct non-default runner port is present, then call getRunnerEndpoint; apply the same fix to the other task handlers that call getRunnerEndpoint.components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py (1)
244-246:⚠️ Potential issue | 🟠 MajorReset and flush hook events per run.
self._hook_event_queueis still adapter-scoped, and this drain block is still bypassed by theStreamEventfast path. A run that halts or ends from that path can leave queuedhook:*events behind, so the next run or synthetic between-run stream emits them with the wrong lineage. Reinitialize/clear the queue at run start and force a final drain before_stream_claude_sdk()returns.Also applies to: 1213-1244
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py` around lines 244 - 246, The hook event queue is adapter-scoped (self._hook_event_queue) and can retain hook:* events across runs or when the StreamEvent fast path returns; reinitialize or clear self._hook_event_queue at the start of each run (where runs are initiated) so each run gets a fresh queue, and add a forced final drain/flush of any remaining hook events just before _stream_claude_sdk() returns (ensure the drain runs even for the StreamEvent fast path) to prevent leftover events from being emitted into subsequent runs; update the corresponding logic referenced around the duplicate block (approx. the 1213-1244 area) to apply the same reinit+final-flush behavior.components/runners/ambient-runner/ambient_runner/endpoints/tasks.py (2)
67-118:⚠️ Potential issue | 🟠 MajorAvoid wildcard transcript discovery on the event loop.
The
rglob(f"*{task_id}*")fallback is user-driven and nondeterministic, so/tasks/{id}/outputcan walk huge trees and return the wrong file if multiple paths contain the ID. Thoserglob/stat/opencalls also run synchronously inside an async handler, so one large scan blocks the entire runner. Resolve output paths from bridge-owned metadata (or a deterministic filename) and move the filesystem work toanyio.to_thread.run_sync().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py` around lines 67 - 118, Replace the nondeterministic rglob-based discovery and all blocking filesystem operations with deterministic bridge-provided metadata lookup and run the remaining file IO in a thread: stop using the search_dirs/rglob loop that sets output_path for task_id; instead fetch the exact output_path from bridge-owned metadata (or construct it from a deterministic filename convention) and then perform Path(output_path).resolve(), exists(), stat().st_size and file open/read inside anyio.to_thread.run_sync() so the event loop is not blocked; update references to output_path, resolved, and the JSON-parsing block to run inside the threaded function and keep the same HTTPException checks (403/404/413/500) based on the thread results.
53-55:⚠️ Potential issue | 🟠 MajorStop returning internal exception text from task endpoints.
Both handlers bubble
str(e)back to clients. Forstop_task, the bridge already raises request-state errors likeNo thread_id availableandNo active session for thread ..., which should be mapped to 4xx instead of a generic 500; for unexpected failures, log server-side and return a generic message so file paths and session internals are not leaked.Also applies to: 116-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py` around lines 53 - 55, The handlers stop_task (and the other task endpoint around lines 116-118) must not return internal exception text; instead, detect known request-state errors coming from the bridge (e.g., errors whose type or message indicates "No thread_id available" or "No active session for thread") and map them to appropriate 4xx HTTPExceptions (400 or 404) with a concise client-facing message, while for all other unexpected exceptions log the full exception server-side (including stack) via logger.exception and raise a 500 HTTPException with a generic message like "Internal error while stopping task" (do the same pattern in both stop_task and the other task handler so no internal details are returned).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/backend/websocket/agui_proxy.go`:
- Around line 74-77: betweenRunListeners currently stores struct{} and relies on
defer delete, allowing long-running listenBetweenRunEvents() goroutines to
outlive their map entries; change the map to store a context.CancelFunc (or a
small struct containing it), create a ctx,ctxCancel :=
context.WithCancel(parent) in ensureBetweenRunListener(), store ctxCancel
atomically (use LoadOrStore semantics so you don't start a duplicate), pass ctx
into listenBetweenRunEvents(ctx, ...) and have that loop exit on ctx.Done(), and
finally have cleanupStaleSessions() call the stored cancel func before deleting
the map entry to ensure listeners stop immediately.
In `@components/frontend/src/hooks/agui/event-handlers.ts`:
- Around line 1038-1048: The handlers for 'task:progress' and 'task:completed'
overwrite existing BackgroundTask fields with possibly undefined values from
event.value, causing loss of stored metadata; update the merge logic in the
branches that use state.backgroundTasks (variables: tasks, existing, value) to
produce a new task object that preserves existing fields and only replaces them
when value.<field> is defined (e.g., usage, last_tool_name, summary,
output_file), i.e. merge existing with value but use conditional/nullish checks
so undefined fields in value do not clear stored data in backgroundTasks.
In `@components/frontend/src/lib/transform-task-transcript.ts`:
- Around line 36-48: The current preflight block that aggregates rawEntries and
returns early causes mixed raw/structured entries to be dropped; remove the
all-or-nothing rawEntries block and instead, inside the main loop that processes
entries (use the same loop that reads entry/type), detect entries with entry.raw
(or where entry.type is undefined) and push a bash AgentMessage into messages
using entry.raw for text and (entry.timestamp as string) ?? new
Date().toISOString() for timestamp, then continue; keep existing logic that
skips progress or missing-type entries but ensure raw entries are handled before
that check so mixed content is preserved and no raw output is silently lost
(refer to symbols: rawEntries, entries, messages, AgentMessage, entry.raw,
entry.type, entry.timestamp).
In `@components/runners/ambient-runner/ambient_runner/app.py`:
- Around line 261-264: The /events router is calling
bridge.stream_between_run_events unconditionally (router in
ambient_runner.endpoints.events), but only ClaudeBridge implements it; update
the code to avoid AttributeError by gating the router or the call: either add
stream_between_run_events as an abstract method on PlatformBridge (so all
bridges implement it) or, more simply, check for the capability before
registering or invoking it (e.g., use hasattr(bridge,
"stream_between_run_events") or a capability flag) and only call
stream_between_run_events or include_router(events_router) when the bridge
supports it (affect ambient_runner.endpoints.events and the place where
app.include_router(events_router) is registered).
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/session.py`:
- Around line 166-168: The routing race happens because _read_messages_forever()
reads _active_output_queue without synchronization while _run() can clear it and
enqueue a sentinel; fix by making the queue handoff atomic: add a threading.Lock
(or asyncio.Lock depending on runtime) protecting accesses to
_active_output_queue and the associated _turn_done change, or implement a turn
token (e.g., incrementing _turn_id) that _run() sets and that
_read_messages_forever() reads and validates before routing each message; update
all references in _run(), _read_messages_forever(), and any setter logic that
assigns or clears _active_output_queue (lines around where _active_output_queue,
_turn_done are manipulated) to acquire the lock or check the token so that
checking and routing are atomic and the sentinel enqueue/clear cannot race with
message routing.
In `@components/runners/ambient-runner/ambient_runner/endpoints/events.py`:
- Around line 41-50: Heartbeat timeouts are cancelling the in-flight
event_iter.__anext__() call and closing the async generator in
stream_between_run_events(); fix by creating and reusing a persistent polling
task (e.g., poll_task = asyncio.create_task(event_iter.__anext__())) and wrap
that task with asyncio.shield() inside
asyncio.wait_for(asyncio.shield(poll_task), timeout=_HEARTBEAT_INTERVAL) so
timeouts only cancel the wait_for but not the underlying __anext__() call; when
the shielded task completes, consume its result, recreate poll_task for the next
loop, and properly handle StopAsyncIteration from the underlying iterator and
exceptions from the finished task.
---
Outside diff comments:
In
`@components/runners/ambient-runner/ambient_runner/bridges/claude/mock_client.py`:
- Around line 74-77: The mock client currently stores a single Event and a
persistent prompt so receive_messages()/receive_response() replays only the
initial fixture and then blocks; fix by making query(prompt, ...) set
self._prompt, reset/replace a per-query asyncio.Event (e.g., create new
self._ready_event) and call self._ready_event.set() to wake the reader; update
receive_messages() to await the current per-query event, call receive_response()
for the current self._prompt, then clear/replace the event and continue so each
query() produces a fresh replayed stream; reference methods/attributes: query,
receive_messages, receive_response, self._prompt, self._interrupted, and the
Event instance.
---
Duplicate comments:
In `@components/backend/websocket/agui_proxy.go`:
- Around line 1192-1193: The taskID is interpolated into the runner path without
escaping, so update creation of the stop and output target URLs to use
url.PathEscape(taskID) before concatenation; locate the code around
getRunnerEndpoint(projectName, sessionName) where targetURL is built (and the
similar block at lines handling output URL) and replace direct use of taskID
with the escaped value so reserved characters cannot alter the proxied path.
- Around line 1252-1254: The current code uses io.LimitReader to read up to 10MB
and forwards that partial body via c.Data, which can produce truncated/invalid
JSON; change the read logic in the handler that reads resp.Body (where body :=
io.ReadAll(io.LimitReader(resp.Body, 10*1024*1024)) and the similar block near
the second occurrence) to read limit+1 bytes (e.g., read into a buffer of size
10*1024*1024+1 or use io.ReadFull/ReadAtMost), detect if the extra byte exists,
and if so return a clear error response (non-2xx, e.g., 413/502 with an
explanatory message) instead of calling c.Data with a truncated payload; keep
using resp.Header for Content-Type only when forwarding the full body.
- Around line 1192-1193: The task handlers call getRunnerEndpoint(projectName,
sessionName) without refreshing routing cache, so after restarts or
cleanupStaleSessions() they can fall back to DefaultRunnerPort; before resolving
runnerURL, update sessionLastSeen for the session and repopulate sessionPortMap
(or call a helper like ensureSessionCached/refreshSessionRouting if available)
so the correct non-default runner port is present, then call getRunnerEndpoint;
apply the same fix to the other task handlers that call getRunnerEndpoint.
In `@components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py`:
- Around line 244-246: The hook event queue is adapter-scoped
(self._hook_event_queue) and can retain hook:* events across runs or when the
StreamEvent fast path returns; reinitialize or clear self._hook_event_queue at
the start of each run (where runs are initiated) so each run gets a fresh queue,
and add a forced final drain/flush of any remaining hook events just before
_stream_claude_sdk() returns (ensure the drain runs even for the StreamEvent
fast path) to prevent leftover events from being emitted into subsequent runs;
update the corresponding logic referenced around the duplicate block (approx.
the 1213-1244 area) to apply the same reinit+final-flush behavior.
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py`:
- Around line 350-362: task_registry and task_outputs currently return the
adapter's live dicts which can be mutated concurrently; change their getters
(properties task_registry and task_outputs) to return snapshots (e.g., shallow
copies via dict(...) or .copy()) of the adapter attributes _task_registry and
_task_outputs when self._adapter is present so callers iterate safely while
_emit_task_* may mutate the originals; ensure you still return {} when adapter
is missing.
In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py`:
- Around line 67-118: Replace the nondeterministic rglob-based discovery and all
blocking filesystem operations with deterministic bridge-provided metadata
lookup and run the remaining file IO in a thread: stop using the
search_dirs/rglob loop that sets output_path for task_id; instead fetch the
exact output_path from bridge-owned metadata (or construct it from a
deterministic filename convention) and then perform Path(output_path).resolve(),
exists(), stat().st_size and file open/read inside anyio.to_thread.run_sync() so
the event loop is not blocked; update references to output_path, resolved, and
the JSON-parsing block to run inside the threaded function and keep the same
HTTPException checks (403/404/413/500) based on the thread results.
- Around line 53-55: The handlers stop_task (and the other task endpoint around
lines 116-118) must not return internal exception text; instead, detect known
request-state errors coming from the bridge (e.g., errors whose type or message
indicates "No thread_id available" or "No active session for thread") and map
them to appropriate 4xx HTTPExceptions (400 or 404) with a concise client-facing
message, while for all other unexpected exceptions log the full exception
server-side (including stack) via logger.exception and raise a 500 HTTPException
with a generic message like "Internal error while stopping task" (do the same
pattern in both stop_task and the other task handler so no internal details are
returned).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b1c79189-6e98-4986-b0c0-959b4d698bd5
📒 Files selected for processing (15)
components/backend/websocket/agui_proxy.gocomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/explorer/__tests__/explorer-panel.test.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/explorer/explorer-panel.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/hooks/use-file-tabs.tscomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/page.tsxcomponents/frontend/src/hooks/agui/event-handlers.tscomponents/frontend/src/lib/transform-task-transcript.tscomponents/runners/ambient-runner/ag_ui_claude_sdk/adapter.pycomponents/runners/ambient-runner/ag_ui_claude_sdk/hooks.pycomponents/runners/ambient-runner/ambient_runner/app.pycomponents/runners/ambient-runner/ambient_runner/bridges/claude/bridge.pycomponents/runners/ambient-runner/ambient_runner/bridges/claude/mock_client.pycomponents/runners/ambient-runner/ambient_runner/bridges/claude/session.pycomponents/runners/ambient-runner/ambient_runner/endpoints/events.pycomponents/runners/ambient-runner/ambient_runner/endpoints/tasks.py
Comment on lines
+74
to
+77
| // betweenRunListeners tracks active between-run listener goroutines. | ||
| // Key: "projectName/sessionName", Value: struct{}. | ||
| // Used to ensure at most one listener goroutine per session. | ||
| var betweenRunListeners sync.Map |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l components/backend/websocket/agui_proxy.goRepository: ambient-code/platform
Length of output: 112
🏁 Script executed:
# First, let's read the area around the betweenRunListeners definition (74-77)
sed -n '70,85p' components/backend/websocket/agui_proxy.goRepository: ambient-code/platform
Length of output: 747
🏁 Script executed:
# Find cleanupStaleSessions function
rg -n "cleanupStaleSessions" components/backend/websocket/agui_proxy.goRepository: ambient-code/platform
Length of output: 203
🏁 Script executed:
# Find ensureBetweenRunListener function
rg -n "ensureBetweenRunListener" components/backend/websocket/agui_proxy.goRepository: ambient-code/platform
Length of output: 267
🏁 Script executed:
# Find listenBetweenRunEvents function
rg -n "listenBetweenRunEvents" components/backend/websocket/agui_proxy.goRepository: ambient-code/platform
Length of output: 269
🏁 Script executed:
# Read cleanupStaleSessions function - let me find where it ends
sed -n '92,150p' components/backend/websocket/agui_proxy.goRepository: ambient-code/platform
Length of output: 2100
🏁 Script executed:
# Read ensureBetweenRunListener function
sed -n '1087,1105p' components/backend/websocket/agui_proxy.goRepository: ambient-code/platform
Length of output: 880
🏁 Script executed:
# Read listenBetweenRunEvents function - let's see the full implementation
sed -n '1101,1200p' components/backend/websocket/agui_proxy.goRepository: ambient-code/platform
Length of output: 3582
🏁 Script executed:
# Let's look at the constants for retry behavior
rg -n "betweenRunInitialBackoff|betweenRunMaxRetries|betweenRunMaxBackoff" components/backend/websocket/agui_proxy.goRepository: ambient-code/platform
Length of output: 1024
🏁 Script executed:
# Check if there's any context-based cancellation being used elsewhere
rg -n "context\.Context|context\.cancel|context\.WithCancel" components/backend/websocket/agui_proxy.goRepository: ambient-code/platform
Length of output: 47
🏁 Script executed:
# Let's verify the complete flow - check where ensureBetweenRunListener is called
sed -n '260,275p' components/backend/websocket/agui_proxy.goRepository: ambient-code/platform
Length of output: 806
🏁 Script executed:
# Let me check how long the listener could potentially run
# Maximum time for retries with exponential backoff
python3 << 'EOF'
backoff = 1 # second
max_backoff = 30 # seconds
max_retries = 30
total_time = 0
for attempt in range(max_retries):
total_time += backoff
if backoff < max_backoff:
backoff = min(backoff * 1.5, max_backoff)
# Convert to minutes
print(f"Maximum retry duration: {total_time} seconds = {total_time / 60:.1f} minutes")
print(f"Maximum backoff reached at backoff cap of {max_backoff}s")
# Now check the cleanup cycle
print(f"\nStale threshold: 1 hour")
print(f"Cleanup runs every: 10 minutes")
print(f"So a stale session is eligible for cleanup 10+ minutes after becoming inactive")
EOFRepository: ambient-code/platform
Length of output: 305
🏁 Script executed:
# Now let's verify the actual behavior - trace the flow more carefully
# Check if there are any other synchronization mechanisms I missed
sed -n '1087,1105p' components/backend/websocket/agui_proxy.goRepository: ambient-code/platform
Length of output: 880
🏁 Script executed:
# Let me verify the exact race condition scenario by checking the defer behavior
cat > /tmp/test_race.go << 'EOF'
package main
import (
"fmt"
"sync"
"time"
)
func main() {
var m sync.Map
key := "session1"
// Simulate the listener goroutine
_, loaded := m.LoadOrStore(key, struct{}{})
if !loaded {
fmt.Println("1. Listener goroutine started, entry stored")
go func() {
defer func() {
m.Delete(key)
fmt.Println("4. Listener goroutine exit: deferred delete called")
}()
time.Sleep(2 * time.Second) // Simulate listener doing work
fmt.Println("3. Listener goroutine: finishing work")
}()
}
time.Sleep(1 * time.Second) // Simulate some work
// Simulate cleanup deleting the entry
m.Delete(key)
fmt.Println("2. Cleanup: entry deleted from map")
// Simulate new request checking the map
if _, exists := m.Load(key); !exists {
fmt.Println("2b. New request: entry not found, will start new listener")
_, loaded := m.LoadOrStore(key, struct{}{})
if !loaded {
fmt.Println("2c. New listener goroutine started (RACE CONDITION)")
}
}
time.Sleep(3 * time.Second)
fmt.Println("5. Both listeners could have run concurrently!")
}
EOF
go run /tmp/test_race.go 2>/dev/null || echo "Go not available, but concept verified from code review"Repository: ambient-code/platform
Length of output: 389
🏁 Script executed:
# Final check: verify that there's no other mechanism that would stop the listener
rg -n "context\.Context|Done\(\)|cancel" components/backend/websocket/agui_proxy.go | head -20Repository: ambient-code/platform
Length of output: 219
Use context cancellation to stop listeners instead of relying on defer delete.
cleanupStaleSessions() deletes the betweenRunListeners entry, but the goroutine in ensureBetweenRunListener() can still run for up to ~12 minutes in listenBetweenRunEvents() (30 retry attempts with exponential backoff). If a new request arrives before the old listener exits, ensureBetweenRunListener() won't find the key, spawns a second listener, and both process the same events—causing duplicates in persistence and broadcast. Store a context.CancelFunc in the map and call it during cleanup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/backend/websocket/agui_proxy.go` around lines 74 - 77,
betweenRunListeners currently stores struct{} and relies on defer delete,
allowing long-running listenBetweenRunEvents() goroutines to outlive their map
entries; change the map to store a context.CancelFunc (or a small struct
containing it), create a ctx,ctxCancel := context.WithCancel(parent) in
ensureBetweenRunListener(), store ctxCancel atomically (use LoadOrStore
semantics so you don't start a duplicate), pass ctx into
listenBetweenRunEvents(ctx, ...) and have that loop exit on ctx.Done(), and
finally have cleanupStaleSessions() call the stored cancel func before deleting
the map entry to ensure listeners stop immediately.
Comment on lines
+1038
to
+1048
| if (name === 'task:progress') { | ||
| const tasks = new Map(state.backgroundTasks) | ||
| const existing = tasks.get(value.task_id as string) | ||
| if (existing) { | ||
| tasks.set(value.task_id as string, { | ||
| ...existing, | ||
| usage: value.usage as BackgroundTaskUsage | undefined, | ||
| last_tool_name: value.last_tool_name as string | undefined, | ||
| }) | ||
| } | ||
| return { ...state, backgroundTasks: tasks } |
Contributor
There was a problem hiding this comment.
Preserve existing task fields when a custom event only carries a partial update.
task:progress and task:completed assign optional fields straight from event.value. When a later event omits usage, last_tool_name, summary, or output_file, those assignments erase the values already stored in backgroundTasks, so the Tasks UI loses metadata as the task progresses/completes.
♻️ Merge partial task payloads instead of overwriting with `undefined`
if (name === 'task:progress') {
const tasks = new Map(state.backgroundTasks)
const existing = tasks.get(value.task_id as string)
if (existing) {
tasks.set(value.task_id as string, {
...existing,
- usage: value.usage as BackgroundTaskUsage | undefined,
- last_tool_name: value.last_tool_name as string | undefined,
+ usage: (value.usage as BackgroundTaskUsage | undefined) ?? existing.usage,
+ last_tool_name: (value.last_tool_name as string | undefined) ?? existing.last_tool_name,
})
}
return { ...state, backgroundTasks: tasks }
}
...
tasks.set(taskId, {
...(existing ?? { task_id: taskId, description: '' }),
status: (value.status as BackgroundTaskStatus) || 'completed',
- summary: value.summary as string | undefined,
- usage: value.usage as BackgroundTaskUsage | undefined,
- output_file: value.output_file as string | undefined,
+ summary: (value.summary as string | undefined) ?? existing?.summary,
+ usage: (value.usage as BackgroundTaskUsage | undefined) ?? existing?.usage,
+ output_file: (value.output_file as string | undefined) ?? existing?.output_file,
})Also applies to: 1051-1062
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/frontend/src/hooks/agui/event-handlers.ts` around lines 1038 -
1048, The handlers for 'task:progress' and 'task:completed' overwrite existing
BackgroundTask fields with possibly undefined values from event.value, causing
loss of stored metadata; update the merge logic in the branches that use
state.backgroundTasks (variables: tasks, existing, value) to produce a new task
object that preserves existing fields and only replaces them when value.<field>
is defined (e.g., usage, last_tool_name, summary, output_file), i.e. merge
existing with value but use conditional/nullish checks so undefined fields in
value do not clear stored data in backgroundTasks.
Comment on lines
+36
to
+48
| if (rawEntries.length > 0 && rawEntries.length === entries.length) { | ||
| // All entries are raw text — this is a bash task output | ||
| const rawText = rawEntries.map((e) => e.raw as string).join("\n") | ||
| if (rawText.trim()) { | ||
| messages.push({ | ||
| type: "agent_message", | ||
| content: { type: "text_block", text: "```\n" + rawText + "\n```" }, | ||
| model: "bash", | ||
| timestamp: new Date().toISOString(), | ||
| } satisfies AgentMessage) | ||
| } | ||
| return messages | ||
| } |
Contributor
There was a problem hiding this comment.
Mixed raw/structured entries are partially lost.
When entries contains a mix of raw bash output and structured JSONL entries, the raw entries bypass lines 36-48 (since not all entries are raw), then get silently dropped at line 52 because they lack a type field. This could result in incomplete transcript rendering for tasks that produce both bash output and structured events.
Consider handling raw entries inline within the main loop rather than requiring all-or-nothing.
Suggested approach
for (const entry of entries) {
+ // Handle raw bash output entries inline
+ if ("raw" in entry && typeof entry.raw === "string") {
+ const rawText = (entry.raw as string).trim()
+ if (rawText) {
+ messages.push({
+ type: "agent_message",
+ content: { type: "text_block", text: "```\n" + rawText + "\n```" },
+ model: "bash",
+ timestamp: (entry.timestamp as string) ?? new Date().toISOString(),
+ } satisfies AgentMessage)
+ }
+ continue
+ }
+
const entryType = entry.type as string | undefined
if (!entryType || entryType === "progress") continueThis would allow removing the separate raw-entry preprocessing block (lines 33-48) and handle mixed content correctly.
Also applies to: 50-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/frontend/src/lib/transform-task-transcript.ts` around lines 36 -
48, The current preflight block that aggregates rawEntries and returns early
causes mixed raw/structured entries to be dropped; remove the all-or-nothing
rawEntries block and instead, inside the main loop that processes entries (use
the same loop that reads entry/type), detect entries with entry.raw (or where
entry.type is undefined) and push a bash AgentMessage into messages using
entry.raw for text and (entry.timestamp as string) ?? new Date().toISOString()
for timestamp, then continue; keep existing logic that skips progress or
missing-type entries but ensure raw entries are handled before that check so
mixed content is preserved and no raw output is silently lost (refer to symbols:
rawEntries, entries, messages, AgentMessage, entry.raw, entry.type,
entry.timestamp).
Comment on lines
+261
to
+264
| # Between-run event stream (always registered) | ||
| from ambient_runner.endpoints.events import router as events_router | ||
|
|
||
| app.include_router(events_router) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PlatformBridge contract:"
rg -n -A80 -B2 '^class PlatformBridge\b' components/runners/ambient-runner/ambient_runner/bridge.py
echo
echo "Bridge subclasses:"
rg -n --type=py '^class .*Bridge\(' components/runners/ambient-runner
echo
echo "stream_between_run_events implementations:"
rg -n --type=py '\bdef stream_between_run_events\b' components/runners/ambient-runnerRepository: ambient-code/platform
Length of output: 4067
🏁 Script executed:
cat -n components/runners/ambient-runner/ambient_runner/endpoints/events.py | head -100Repository: ambient-code/platform
Length of output: 2460
Gate /events endpoint behind stream_between_run_events capability or make it mandatory on PlatformBridge.
Line 34 in endpoints/events.py calls bridge.stream_between_run_events(thread_id) unconditionally, but only ClaudeBridge implements this method. LangGraphBridge and GeminiCLIBridge lack it entirely, so any client connecting to /events will get a 500 error (AttributeError). Either add stream_between_run_events() to the PlatformBridge contract as a required or lifecycle method, or check for the capability before registering the router.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/runners/ambient-runner/ambient_runner/app.py` around lines 261 -
264, The /events router is calling bridge.stream_between_run_events
unconditionally (router in ambient_runner.endpoints.events), but only
ClaudeBridge implements it; update the code to avoid AttributeError by gating
the router or the call: either add stream_between_run_events as an abstract
method on PlatformBridge (so all bridges implement it) or, more simply, check
for the capability before registering or invoking it (e.g., use hasattr(bridge,
"stream_between_run_events") or a capability flag) and only call
stream_between_run_events or include_router(events_router) when the bridge
supports it (affect ambient_runner.endpoints.events and the place where
app.include_router(events_router) is registered).
components/runners/ambient-runner/ambient_runner/bridges/claude/session.py
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
components/runners/ambient-runner/ambient_runner/endpoints/tasks.py (2)
107-125:⚠️ Potential issue | 🟠 MajorMove file reads off the event loop and stop leaking raw exception text.
stat()+open()+ line parsing are blocking insideasync def, anddetail=str(e)exposes internal error content to clients.Suggested fix
+import anyio ... - if resolved.stat().st_size > _MAX_OUTPUT_BYTES: - raise HTTPException( - status_code=413, detail="Transcript too large" - ) - - try: - entries = [] - with open(resolved) as f: - for line in f: - line = line.strip() - if line: - try: - entries.append(json.loads(line)) - except json.JSONDecodeError: - entries.append({"raw": line}) - return {"task_id": task_id, "output": entries} - except Exception as e: - logger.error(f"Failed to read output for task {task_id}: {e}") - raise HTTPException(status_code=500, detail=str(e)) + def _read_entries(path: Path) -> list[dict]: + if path.stat().st_size > _MAX_OUTPUT_BYTES: + raise HTTPException(status_code=413, detail="Transcript too large") + out = [] + with path.open() as f: + for raw_line in f: + text = raw_line.strip() + if not text: + continue + try: + out.append(json.loads(text)) + except json.JSONDecodeError: + out.append({"raw": text}) + return out + + try: + entries = await anyio.to_thread.run_sync(_read_entries, resolved) + return {"task_id": task_id, "output": entries} + except HTTPException: + raise + except Exception: + logger.exception("Failed to read output for task_id=%s", task_id) + raise HTTPException(status_code=500, detail="Failed to read task output")As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py` around lines 107 - 125, The blocking file operations (resolved.stat(), open(resolved) and line-by-line parsing) inside the async endpoint should be moved off the event loop (e.g., wrap the read-and-parse logic in asyncio.to_thread or loop.run_in_executor) and return the parsed entries from that thread; also stop leaking raw exception text to clients by logging the full exception with logger.exception(...) and raising HTTPException(status_code=500, detail="Failed to read task output") instead of detail=str(e); update the code around resolved.stat(), the file-read loop that builds entries, and the error handling for task_id so all blocking IO runs in a background thread and only a generic error message is returned to the client.
74-86:⚠️ Potential issue | 🟠 MajorRemove user-controlled recursive glob fallback for task output lookup.
rglob(f"*{task_id}*")is nondeterministic, expensive, and allows filesystem probing via attacker-controlledtask_id. Prefer deterministic, registry-backed paths only (or strict exact filename patterns in approved directories).As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py` around lines 74 - 86, The fallback that uses search_dirs and rglob(f"*{task_id}*") is unsafe and expensive; remove the recursive glob and instead resolve output_path only via deterministic, registry-backed locations or strict exact-filename lookups in approved directories. Replace the rglob logic around the variables search_dirs, task_id and output_path with a validated lookup that (1) validates/sanitizes task_id, (2) checks only a pre-approved list of filenames or exact patterns (e.g., exact filename like "{task_id}.json") within approved directories, and (3) consults a registry/map or configured base path to derive the final path; do not perform any wildcard or recursive filesystem probing. Ensure the new code updates the same output_path variable and preserves the existing break/short-circuit behavior when a match is found.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/explorer/background-tasks-tab.tsx:
- Around line 24-25: Replace the single stoppingTaskId string state with a
Set<string> to track multiple concurrent stop requests: change the hook to
useState<Set<string>>(new Set()) and update usages of stoppingTaskId and
setStoppingTaskId across the component (including the stop handler and UI
checks) to immutably add the task id before starting a stop request (create a
new Set from previous, add id, call setter) and to immutably remove the id in
both success and error paths (create new Set, delete id, call setter). Also
update any conditional checks (e.g., stoppingTaskId === taskId) to use
stoppingSet.has(taskId), and ensure stoppedTaskIds/setStoppedTaskIds continue to
record completed stops (use a new Set when adding ids) so buttons and spinners
reflect correct state for multiple concurrent stops.
In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py`:
- Around line 39-45: The current handler around bridge.stop_task(task_id,
thread_id) swallows all exceptions but still emits the "task:stop_requested"
event and sets task status to "stopped"; change this so you track whether
stop_task actually succeeded (e.g., set a success flag or use the normal return
path), only emit the "task:stop_requested" event and mutate the task state to
"stopped" when stop_task completes without raising, and let exceptions propagate
(or re-raise after any necessary logging) so control-plane/network/auth errors
surface to the caller; update the code paths referencing bridge.stop_task, the
surrounding try/except block, the logger.warning call, and the logic that emits
"task:stop_requested"/sets status to ensure they run only on success.
---
Duplicate comments:
In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py`:
- Around line 107-125: The blocking file operations (resolved.stat(),
open(resolved) and line-by-line parsing) inside the async endpoint should be
moved off the event loop (e.g., wrap the read-and-parse logic in
asyncio.to_thread or loop.run_in_executor) and return the parsed entries from
that thread; also stop leaking raw exception text to clients by logging the full
exception with logger.exception(...) and raising HTTPException(status_code=500,
detail="Failed to read task output") instead of detail=str(e); update the code
around resolved.stat(), the file-read loop that builds entries, and the error
handling for task_id so all blocking IO runs in a background thread and only a
generic error message is returned to the client.
- Around line 74-86: The fallback that uses search_dirs and
rglob(f"*{task_id}*") is unsafe and expensive; remove the recursive glob and
instead resolve output_path only via deterministic, registry-backed locations or
strict exact-filename lookups in approved directories. Replace the rglob logic
around the variables search_dirs, task_id and output_path with a validated
lookup that (1) validates/sanitizes task_id, (2) checks only a pre-approved list
of filenames or exact patterns (e.g., exact filename like "{task_id}.json")
within approved directories, and (3) consults a registry/map or configured base
path to derive the final path; do not perform any wildcard or recursive
filesystem probing. Ensure the new code updates the same output_path variable
and preserves the existing break/short-circuit behavior when a match is found.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1d535c8a-4289-42e7-9671-cd1051597f3a
📒 Files selected for processing (2)
components/frontend/src/app/projects/[name]/sessions/[sessionName]/components/explorer/background-tasks-tab.tsxcomponents/runners/ambient-runner/ambient_runner/endpoints/tasks.py
Comment on lines
+24
to
+25
| const [stoppingTaskId, setStoppingTaskId] = useState<string | null>(null) | ||
| const [stoppedTaskIds, setStoppedTaskIds] = useState<Set<string>>(new Set()) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n components/frontend/src/app/projects/[name]/sessions/[sessionName]/components/explorer/background-tasks-tab.tsxRepository: ambient-code/platform
Length of output: 5622
Track multiple concurrent stop requests to prevent race conditions.
A single stoppingTaskId state can be overwritten when stopping multiple tasks concurrently. For example, clicking stop on Task A, then clicking stop on Task B before Task A's request completes will overwrite stoppingTaskId to B, causing Task A's button to lose its disabled state and spinner while the request is still in flight. This can lead to duplicate stop attempts and incorrect UI state.
Use a Set<string> instead to track all tasks currently being stopped:
Suggested fix
- const [stoppingTaskId, setStoppingTaskId] = useState<string | null>(null)
+ const [stoppingTaskIds, setStoppingTaskIds] = useState<Set<string>>(new Set())
const handleStop = async (taskId: string) => {
- setStoppingTaskId(taskId)
+ setStoppingTaskIds((prev) => new Set(prev).add(taskId))
try {
await stopBackgroundTask(projectName, sessionName, taskId)
setStoppedTaskIds((prev) => new Set(prev).add(taskId))
} catch (err) {
console.error("Failed to stop task:", err)
} finally {
- setStoppingTaskId(null)
+ setStoppingTaskIds((prev) => {
+ const next = new Set(prev)
+ next.delete(taskId)
+ return next
+ })
}
}
...
- disabled={stoppingTaskId === task.task_id}
+ disabled={stoppingTaskIds.has(task.task_id)}
...
- {stoppingTaskId === task.task_id ? (
+ {stoppingTaskIds.has(task.task_id) ? (Applies to lines 24-25, 33-42, 110-115.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/explorer/background-tasks-tab.tsx
around lines 24 - 25, Replace the single stoppingTaskId string state with a
Set<string> to track multiple concurrent stop requests: change the hook to
useState<Set<string>>(new Set()) and update usages of stoppingTaskId and
setStoppingTaskId across the component (including the stop handler and UI
checks) to immutably add the task id before starting a stop request (create a
new Set from previous, add id, call setter) and to immutably remove the id in
both success and error paths (create new Set, delete id, call setter). Also
update any conditional checks (e.g., stoppingTaskId === taskId) to use
stoppingSet.has(taskId), and ensure stoppedTaskIds/setStoppedTaskIds continue to
record completed stops (use a new Set when adding ids) so buttons and spinners
reflect correct state for multiple concurrent stops.
Comment on lines
+39
to
+45
| try: | ||
| await bridge.stop_task(task_id, thread_id) | ||
| except Exception as e: | ||
| # SDK may throw if task already finished ("not running", "killed", etc.) | ||
| # Treat as success — the task is no longer running either way | ||
| logger.warning(f"stop_task({task_id}) raised: {e}") | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the file to understand the full context around lines 39-62
cat -n components/runners/ambient-runner/ambient_runner/endpoints/tasks.py | sed -n '30,70p'Repository: ambient-code/platform
Length of output: 1873
Only emit task:stop_requested and set status to "stopped" if the stop actually succeeds.
The handler suppresses all exceptions from stop_task() but unconditionally emits a success event and mutates the task state to "stopped" regardless. This creates false task state and hides real control-plane failures (network errors, auth issues, etc.), making debugging and monitoring significantly harder. Track whether the stop succeeded and only update state on success; propagate failures to the caller.
Suggested fix
+ stop_sent = False
try:
await bridge.stop_task(task_id, thread_id)
+ stop_sent = True
except Exception:
- # SDK may throw if task already finished ("not running", "killed", etc.)
- # Treat as success — the task is no longer running either way
- logger.warning(f"stop_task({task_id}) raised: {e}")
+ logger.exception("stop_task failed for task_id=%s", task_id)
+ raise HTTPException(status_code=500, detail="Failed to stop task")
- # Emit task:stop_requested event and update registry regardless of
- # whether stop_task succeeded or the task was already dead.
- if adapter:
+ if adapter and stop_sent:
from ag_ui.core import CustomEvent, EventType
adapter._hook_event_queue.put_nowait(
CustomEvent(
type=EventType.CUSTOM,
name="task:stop_requested",
value={"task_id": task_id},
)
)
existing = adapter._task_registry.get(task_id, {})
- existing["status"] = "stopped"
- adapter._task_registry[task_id] = existing
+ if existing is not None:
+ existing["status"] = "stopped"
+ adapter._task_registry[task_id] = existing🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py` around
lines 39 - 45, The current handler around bridge.stop_task(task_id, thread_id)
swallows all exceptions but still emits the "task:stop_requested" event and sets
task status to "stopped"; change this so you track whether stop_task actually
succeeded (e.g., set a success flag or use the normal return path), only emit
the "task:stop_requested" event and mutate the task state to "stopped" when
stop_task completes without raising, and let exceptions propagate (or re-raise
after any necessary logging) so control-plane/network/auth errors surface to the
caller; update the code paths referencing bridge.stop_task, the surrounding
try/except block, the logger.warning call, and the logic that emits
"task:stop_requested"/sets status to ensure they run only on success.
…etween messages The message loop now uses asyncio.wait_for with a 100ms timeout instead of blocking on the next SDK message. When no message arrives within 100ms, hooks drain and the loop continues. This means hook events (like task:stop_requested) reach the SSE stream within 100ms regardless of SDK activity. Also consolidates hook drain + transcript path capture into the shared drain_hook_events() method. Removes local state workaround from the frontend. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (10)
components/frontend/src/app/projects/[name]/sessions/[sessionName]/page.tsx (1)
1542-1547: 🧹 Nitpick | 🔵 TrivialAdd lint suppression comment to clarify intentional dependency omission.
The lint warning about missing
fileTabsdependency is a known issue from previous review. Including the entirefileTabsobject would cause unnecessary re-renders. The current dependency onfileTabs.updateTaskStatusis correct since only that callback's identity matters.♻️ Proposed fix
// Keep task tab status badges in sync with live AG-UI state + // eslint-disable-next-line react-hooks/exhaustive-deps -- only updateTaskStatus identity matters useEffect(() => { for (const [taskId, task] of aguiState.backgroundTasks) { fileTabs.updateTaskStatus(taskId, task.status); } }, [aguiState.backgroundTasks, fileTabs.updateTaskStatus]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/page.tsx around lines 1542 - 1547, The useEffect that iterates aguiState.backgroundTasks intentionally depends only on aguiState.backgroundTasks and fileTabs.updateTaskStatus (not the entire fileTabs object); add an explicit lint suppression comment to document this choice by placing a line like /* eslint-disable-next-line react-hooks/exhaustive-deps -- intentionally omitting fileTabs to avoid re-renders; only updateTaskStatus identity matters */ immediately above the useEffect so the linter is silenced and the rationale is clear for future readers (referencing useEffect, aguiState.backgroundTasks, and fileTabs.updateTaskStatus).components/backend/websocket/agui_proxy.go (4)
74-77:⚠️ Potential issue | 🟠 MajorDeleting the listener key does not stop the old goroutine.
cleanupStaleSessions()removes thebetweenRunListenersentry, but the existing listener can keep reconnecting for minutes. If the same session becomes active again before that goroutine exits,ensureBetweenRunListener()starts a second listener and both persist/broadcast the same SSE stream. Store a cancel func in the map and cancel it during cleanup before deleting the entry.Also applies to: 106-106, 1090-1098
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/backend/websocket/agui_proxy.go` around lines 74 - 77, The map betweenRunListeners currently stores struct{} and cleanupStaleSessions() only deletes the key, leaving the old goroutine running; change the map to store a cancel function (e.g., context.CancelFunc) instead of struct{}, update ensureBetweenRunListener() to create a cancellable context passed into the listener goroutine and save its cancel func in betweenRunListeners, and modify cleanupStaleSessions() (and any other removal sites) to call the stored cancel func before deleting the map entry so the old listener exits promptly.
1193-1193:⚠️ Potential issue | 🔴 CriticalPath-escape
taskIDbefore interpolating it into the runner URL.The raw path param can contain
/,?, or.., which rewrites the downstream/tasks/{id}/...path. Useurl.PathEscape(taskID)for both stop and output URLs.Also applies to: 1237-1237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/backend/websocket/agui_proxy.go` at line 1193, The constructed runner URLs interpolate raw taskID into targetURL (targetURL := strings.TrimSuffix(runnerURL, "/") + "/tasks/" + taskID + "/stop") and similarly for the output URL, which allows `/`, `?`, or `..` in taskID to rewrite paths; change both constructions to path-escape the task ID using url.PathEscape(taskID) before concatenation (e.g., use the escaped ID when building targetURL and the output URL) so downstream /tasks/{id}/... segments are safe.
1252-1254:⚠️ Potential issue | 🟠 MajorDetect truncation instead of returning partial JSON as success.
io.LimitReadersilently truncates oversized transcripts/task lists, andc.Dataforwards whatever bytes were read with the runner's status code. Once the payload crosses the cap, the frontend gets invalid JSON with an apparent success response. Readlimit+1bytes, fail if the extra byte exists, and only forward the body when it fits.Also applies to: 1290-1291
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/backend/websocket/agui_proxy.go` around lines 1252 - 1254, The current read using io.LimitReader silently truncates large responses and forwards partial JSON via c.Data; change the read logic in the response handling around resp and io.LimitReader to read limit+1 bytes (e.g., 10*1024*1024 + 1), check whether the extra byte exists and, if so, return an error response (non-2xx) or an explicit truncation error to the client instead of calling c.Data; only call c.Data(resp.StatusCode, resp.Header.Get("Content-Type"), body) when the body fits within the limit. Ensure the same change is applied to the other occurrence at the second block referenced (near line 1290-1291).
1192-1193:⚠️ Potential issue | 🟠 MajorRefresh the session port cache before resolving task runner URLs.
These handlers call
getRunnerEndpoint()directly, butsessionPortMapis only hydrated by run traffic and later pruned bycleanupStaleSessions(). After a backend restart or cache eviction,/agui/tasks*can fall back toDefaultRunnerPortand miss non-default runners. Call the same cache refresh path used inHandleAGUIRunProxy()before building these URLs, and updatesessionLastSeenhere as well.Also applies to: 1236-1237, 1274-1275
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/backend/websocket/agui_proxy.go` around lines 1192 - 1193, Before calling getRunnerEndpoint() in the task handlers (the block building runnerURL/targetURL), invoke the same session-port cache refresh path used in HandleAGUIRunProxy() so sessionPortMap is repopulated after restarts or eviction; then call getRunnerEndpoint(projectName, sessionName) as before and set/update sessionLastSeen for sessionName to the current time to mirror run traffic behavior. Apply this change in the three locations that build runner URLs (around the getRunnerEndpoint usage at the referenced lines) so non-default runner ports are discovered reliably and sessionLastSeen is maintained.components/runners/ambient-runner/ambient_runner/endpoints/events.py (1)
40-50:⚠️ Potential issue | 🔴 CriticalShield the pending
__anext__()from heartbeat timeouts.
asyncio.wait_for(event_iter.__anext__(), timeout=...)cancels the in-flight poll on every idle heartbeat. Becausebridge.stream_between_run_events()is an async generator, the first timeout can close it and the next poll hitsStopAsyncIteration, so/eventsdies after the first ~15s idle period. Keep one polling task alive across heartbeats and wrap it withasyncio.shield().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ambient_runner/endpoints/events.py` around lines 40 - 50, The current loop cancels the in-flight poll on event_iter.__anext__() by using asyncio.wait_for directly, which causes bridge.stream_between_run_events() to be closed on the first heartbeat timeout; instead preserve the in-flight poll across heartbeats by creating a single polling Task for event_iter.__anext__() (e.g., task = asyncio.create_task(event_iter.__anext__())) and await asyncio.wait_for(asyncio.shield(task), timeout=_HEARTBEAT_INTERVAL) inside the loop, recreating the Task only after it completes or raises StopAsyncIteration, and yield encoder.encode(event) when the Task returns, yielding ": heartbeat\n\n" on TimeoutError without cancelling the underlying generator.components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py (1)
244-246:⚠️ Potential issue | 🟠 MajorHook events can still leak into the next run.
_hook_event_queuelives for the whole adapter, but the drains added here are still not guaranteed on every run-exit path. The hotStreamEventpath bypasses the after-message drain, and the loop breaks onStopAsyncIterationwithout a final flush, so hook events queued near run end can sit until the next run or between-run consumer and be misattributed. Clear the existing queue in place at run start/end rather than swapping it out, since the hook matchers close over this queue instance.Also applies to: 1253-1255
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py` around lines 244 - 246, The hook event queue instance self._hook_event_queue is long-lived and can leak events between runs; instead of replacing it, clear it in place at run start and at all run-exit paths (including the hot StreamEvent path and the StopAsyncIteration break) so hook matchers keep a stable queue reference. Specifically, add an in-place drain/clear routine invoked from run start, from the after-message drain, and from the hot-path exit (where StopAsyncIteration is handled) to flush pending events before returning; reference self._hook_event_queue, the StreamEvent handling branch, and the StopAsyncIteration exit to locate where to call this routine. Ensure you do not reassign self._hook_event_queue so existing hook matchers continue to reference the same queue instance.components/runners/ambient-runner/ambient_runner/endpoints/tasks.py (3)
123-125:⚠️ Potential issue | 🟠 MajorDon't return raw filesystem errors from the runner.
detail=str(e)exposes internal paths and parser errors to clients. Log the full exception server-side and return a generic 500 response instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py` around lines 123 - 125, The exception handler in the tasks endpoint currently returns raw filesystem/parser errors to clients; change it to log the full exception server-side and return a generic 500 detail to the client instead. Replace the current logger.error and HTTPException(detail=str(e)) usage in the except block (around the logger and task_id variables) with a server-side full-stack log call (e.g., logger.exception or logger.error with stack trace) that includes task_id, and raise HTTPException(status_code=500, detail="Internal server error reading task output") so internal paths/errors are not exposed to callers.
74-86:⚠️ Potential issue | 🟠 MajorThe fallback transcript lookup is nondeterministic and user-driven.
rglob(f"*{task_id}*")over/tmpand~/.claude/projectscan match unrelated files, and the first hit wins. A short or wildcard-like task ID can return the wrong transcript. Restrict this to bridge-registered output paths or a deterministic per-task filename instead of substring globbing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py` around lines 74 - 86, The fallback lookup using rglob("*{task_id}*") is nondeterministic and can return unrelated files; replace this substring globbing by resolving transcripts only from bridge-registered output paths or a deterministic per-task filename pattern. Concretely, change the logic around output_path/task_id/search_dirs to first query the bridge's registered output mapping (e.g., a function or dict that maps task_id to output path) and use that path if present; if you must search disk, only look for an exact filename like f"{task_id}.transcript" or f"{task_id}.json" in each search_dir and verify file metadata (e.g., owner/creation time or a task_id field inside the file) before accepting it, instead of using rglob with a wildcard. Ensure you update the code around output_path assignment and remove the broad rglob usage.
39-60:⚠️ Potential issue | 🟠 MajorOnly transition the task to
"stopped"after a real stop succeeds.
bridge.stop_task()propagates actual lookup/transport failures. Swallowing them here and still emittingtask:stop_requested/ mutating the registry leaves the UI in a false terminal state and hides control-plane errors. Keep the early 200 path for already-finished tasks, but only publish/update on a successful stop.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py` around lines 39 - 60, Call bridge.stop_task(task_id, thread_id) and only emit the task:stop_requested event and mutate adapter._task_registry after bridge.stop_task completes successfully; if bridge.stop_task raises, do not publish/update the registry (re-raise or propagate the error) except for the known "already finished" condition coming from the SDK (e.g., errors containing "not running" / "killed") which should still return an early-success response but must NOT trigger adapter._hook_event_queue.put_nowait or modify adapter._task_registry; locate the logic around bridge.stop_task, adapter._hook_event_queue, CustomEvent/EventType, and adapter._task_registry to implement this conditional behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/backend/websocket/agui_proxy.go`:
- Around line 1195-1200: Replace the hardcoded empty JSON body used when
proxying (the bytes.NewReader([]byte("{}")) passed to http.NewRequest) with the
actual incoming request body: read the incoming payload via c.GetRawData() (or
ioutil.ReadAll(c.Request.Body)), handle any read error, and use that byte slice
when creating req with http.NewRequest so the runner receives the caller's JSON
(keep setting req.Header.Set("Content-Type", "application/json") as before);
ensure you restore or reset c.Request.Body if the handler later needs it.
In `@components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py`:
- Around line 813-827: The loop using asyncio.wait_for(message_iter.__anext__(),
timeout=0.1) cancels the iterator on timeout and prematurely closes the SDK
stream; change it to use a persistent poll task and asyncio.shield so timeouts
don't inject CancelledError into message_iter: create a long-lived task (e.g.,
poll_task = asyncio.create_task(message_iter.__anext__())) outside/at the top of
the while loop, await it inside the loop via
asyncio.wait_for(asyncio.shield(poll_task), timeout=0.1), on successful receive
yield the message and replace poll_task with a new asyncio.create_task(...) for
the next item, handle StopAsyncIteration from the task result to break, and on
asyncio.TimeoutError run drain_hook_events() (without cancelling poll_task) so
the iterator remains intact; adjust error handling for CancelledError
accordingly in this function to avoid closing the SDK stream.
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py`:
- Around line 305-310: The branch that handles
TaskStartedMessage/TaskProgressMessage/TaskNotificationMessage currently yields
_adapter._emit_task_event(msg) and any events from _adapter.drain_hook_events()
directly, causing persisted CUSTOM events to lack a synthetic run envelope;
modify this path so these task and hook events are emitted inside the same
synthetic run envelope used by the non-task path (i.e., wrap them with the same
RUN_STARTED and RUN_FINISHED events or apply the same thread/run IDs before
encoding), ensuring the
TaskStartedMessage/TaskProgressMessage/TaskNotificationMessage handling produces
events identical in shape to the synthetic run path rather than standalone
CUSTOM messages.
---
Duplicate comments:
In `@components/backend/websocket/agui_proxy.go`:
- Around line 74-77: The map betweenRunListeners currently stores struct{} and
cleanupStaleSessions() only deletes the key, leaving the old goroutine running;
change the map to store a cancel function (e.g., context.CancelFunc) instead of
struct{}, update ensureBetweenRunListener() to create a cancellable context
passed into the listener goroutine and save its cancel func in
betweenRunListeners, and modify cleanupStaleSessions() (and any other removal
sites) to call the stored cancel func before deleting the map entry so the old
listener exits promptly.
- Line 1193: The constructed runner URLs interpolate raw taskID into targetURL
(targetURL := strings.TrimSuffix(runnerURL, "/") + "/tasks/" + taskID + "/stop")
and similarly for the output URL, which allows `/`, `?`, or `..` in taskID to
rewrite paths; change both constructions to path-escape the task ID using
url.PathEscape(taskID) before concatenation (e.g., use the escaped ID when
building targetURL and the output URL) so downstream /tasks/{id}/... segments
are safe.
- Around line 1252-1254: The current read using io.LimitReader silently
truncates large responses and forwards partial JSON via c.Data; change the read
logic in the response handling around resp and io.LimitReader to read limit+1
bytes (e.g., 10*1024*1024 + 1), check whether the extra byte exists and, if so,
return an error response (non-2xx) or an explicit truncation error to the client
instead of calling c.Data; only call c.Data(resp.StatusCode,
resp.Header.Get("Content-Type"), body) when the body fits within the limit.
Ensure the same change is applied to the other occurrence at the second block
referenced (near line 1290-1291).
- Around line 1192-1193: Before calling getRunnerEndpoint() in the task handlers
(the block building runnerURL/targetURL), invoke the same session-port cache
refresh path used in HandleAGUIRunProxy() so sessionPortMap is repopulated after
restarts or eviction; then call getRunnerEndpoint(projectName, sessionName) as
before and set/update sessionLastSeen for sessionName to the current time to
mirror run traffic behavior. Apply this change in the three locations that build
runner URLs (around the getRunnerEndpoint usage at the referenced lines) so
non-default runner ports are discovered reliably and sessionLastSeen is
maintained.
In `@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/page.tsx:
- Around line 1542-1547: The useEffect that iterates aguiState.backgroundTasks
intentionally depends only on aguiState.backgroundTasks and
fileTabs.updateTaskStatus (not the entire fileTabs object); add an explicit lint
suppression comment to document this choice by placing a line like /*
eslint-disable-next-line react-hooks/exhaustive-deps -- intentionally omitting
fileTabs to avoid re-renders; only updateTaskStatus identity matters */
immediately above the useEffect so the linter is silenced and the rationale is
clear for future readers (referencing useEffect, aguiState.backgroundTasks, and
fileTabs.updateTaskStatus).
In `@components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py`:
- Around line 244-246: The hook event queue instance self._hook_event_queue is
long-lived and can leak events between runs; instead of replacing it, clear it
in place at run start and at all run-exit paths (including the hot StreamEvent
path and the StopAsyncIteration break) so hook matchers keep a stable queue
reference. Specifically, add an in-place drain/clear routine invoked from run
start, from the after-message drain, and from the hot-path exit (where
StopAsyncIteration is handled) to flush pending events before returning;
reference self._hook_event_queue, the StreamEvent handling branch, and the
StopAsyncIteration exit to locate where to call this routine. Ensure you do not
reassign self._hook_event_queue so existing hook matchers continue to reference
the same queue instance.
In `@components/runners/ambient-runner/ambient_runner/endpoints/events.py`:
- Around line 40-50: The current loop cancels the in-flight poll on
event_iter.__anext__() by using asyncio.wait_for directly, which causes
bridge.stream_between_run_events() to be closed on the first heartbeat timeout;
instead preserve the in-flight poll across heartbeats by creating a single
polling Task for event_iter.__anext__() (e.g., task =
asyncio.create_task(event_iter.__anext__())) and await
asyncio.wait_for(asyncio.shield(task), timeout=_HEARTBEAT_INTERVAL) inside the
loop, recreating the Task only after it completes or raises StopAsyncIteration,
and yield encoder.encode(event) when the Task returns, yielding ":
heartbeat\n\n" on TimeoutError without cancelling the underlying generator.
In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py`:
- Around line 123-125: The exception handler in the tasks endpoint currently
returns raw filesystem/parser errors to clients; change it to log the full
exception server-side and return a generic 500 detail to the client instead.
Replace the current logger.error and HTTPException(detail=str(e)) usage in the
except block (around the logger and task_id variables) with a server-side
full-stack log call (e.g., logger.exception or logger.error with stack trace)
that includes task_id, and raise HTTPException(status_code=500, detail="Internal
server error reading task output") so internal paths/errors are not exposed to
callers.
- Around line 74-86: The fallback lookup using rglob("*{task_id}*") is
nondeterministic and can return unrelated files; replace this substring globbing
by resolving transcripts only from bridge-registered output paths or a
deterministic per-task filename pattern. Concretely, change the logic around
output_path/task_id/search_dirs to first query the bridge's registered output
mapping (e.g., a function or dict that maps task_id to output path) and use that
path if present; if you must search disk, only look for an exact filename like
f"{task_id}.transcript" or f"{task_id}.json" in each search_dir and verify file
metadata (e.g., owner/creation time or a task_id field inside the file) before
accepting it, instead of using rglob with a wildcard. Ensure you update the code
around output_path assignment and remove the broad rglob usage.
- Around line 39-60: Call bridge.stop_task(task_id, thread_id) and only emit the
task:stop_requested event and mutate adapter._task_registry after
bridge.stop_task completes successfully; if bridge.stop_task raises, do not
publish/update the registry (re-raise or propagate the error) except for the
known "already finished" condition coming from the SDK (e.g., errors containing
"not running" / "killed") which should still return an early-success response
but must NOT trigger adapter._hook_event_queue.put_nowait or modify
adapter._task_registry; locate the logic around bridge.stop_task,
adapter._hook_event_queue, CustomEvent/EventType, and adapter._task_registry to
implement this conditional behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c0f4a7e0-dbbe-47c2-9817-703d459c35a7
📒 Files selected for processing (15)
components/backend/websocket/agui_proxy.gocomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/explorer/__tests__/explorer-panel.test.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/explorer/explorer-panel.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/hooks/use-file-tabs.tscomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/page.tsxcomponents/frontend/src/hooks/agui/event-handlers.tscomponents/frontend/src/lib/transform-task-transcript.tscomponents/runners/ambient-runner/ag_ui_claude_sdk/adapter.pycomponents/runners/ambient-runner/ag_ui_claude_sdk/hooks.pycomponents/runners/ambient-runner/ambient_runner/app.pycomponents/runners/ambient-runner/ambient_runner/bridges/claude/bridge.pycomponents/runners/ambient-runner/ambient_runner/bridges/claude/mock_client.pycomponents/runners/ambient-runner/ambient_runner/bridges/claude/session.pycomponents/runners/ambient-runner/ambient_runner/endpoints/events.pycomponents/runners/ambient-runner/ambient_runner/endpoints/tasks.py
Comment on lines
+1195
to
+1200
| req, err := http.NewRequest("POST", targetURL, bytes.NewReader([]byte("{}"))) | ||
| if err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) | ||
| return | ||
| } | ||
| req.Header.Set("Content-Type", "application/json") |
Contributor
There was a problem hiding this comment.
Forward the caller's stop payload to the runner.
The runner-side /tasks/{task_id}/stop handler already reads thread_id from the JSON body, but this proxy always replaces the incoming body with {}. Any task that needs thread-scoped routing can never be stopped through /agui/tasks/.../stop, even if the frontend sends the right payload.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/backend/websocket/agui_proxy.go` around lines 1195 - 1200, Replace
the hardcoded empty JSON body used when proxying (the
bytes.NewReader([]byte("{}")) passed to http.NewRequest) with the actual
incoming request body: read the incoming payload via c.GetRawData() (or
ioutil.ReadAll(c.Request.Body)), handle any read error, and use that byte slice
when creating req with http.NewRequest so the runner receives the caller's JSON
(keep setting req.Header.Set("Content-Type", "application/json") as before);
ensure you restore or reset c.Request.Body if the handler later needs it.
Comment on lines
+813
to
+827
| while True: | ||
| # Race: get next SDK message with a timeout so hooks | ||
| # drain continuously (every 100ms) even when the SDK | ||
| # is idle between messages. | ||
| try: | ||
| message = await asyncio.wait_for( | ||
| message_iter.__anext__(), timeout=0.1 | ||
| ) | ||
| except StopAsyncIteration: | ||
| break | ||
| except asyncio.TimeoutError: | ||
| # No message — drain hooks and loop | ||
| for hook_event in self.drain_hook_events(): | ||
| yield hook_event | ||
| continue |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the file and lines in question
find . -name "adapter.py" -path "*ambient-runner*" | head -5Repository: ambient-code/platform
Length of output: 192
🏁 Script executed:
# Check the structure to understand the codebase
ls -la components/runners/ambient-runner/ 2>/dev/null | head -20Repository: ambient-code/platform
Length of output: 932
🏁 Script executed:
# List files in the ag_ui_claude_sdk directory
find . -path "*ag_ui_claude_sdk*" -type f -name "*.py" | head -20Repository: ambient-code/platform
Length of output: 580
🏁 Script executed:
# Read the adapter.py file around lines 813-827
cat -n ./components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py | sed -n '800,840p'Repository: ambient-code/platform
Length of output: 1955
🏁 Script executed:
# Get more context about the _stream_claude_sdk method to understand message_iter
cat -n ./components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py | sed -n '780,850p'Repository: ambient-code/platform
Length of output: 3323
🏁 Script executed:
# Search for where message_iter comes from (function signature)
rg "def _stream_claude_sdk" -A 10 ./components/runners/ambient-runner/ag_ui_claude_sdk/adapter.pyRepository: ambient-code/platform
Length of output: 398
🏁 Script executed:
# Let's understand where message_stream comes from and what it is
rg "def _stream_claude_sdk" -B 5 ./components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py | head -30Repository: ambient-code/platform
Length of output: 340
🏁 Script executed:
# Find where _stream_claude_sdk is called to see what's passed as message_stream
rg "_stream_claude_sdk" -B 2 -A 2 ./components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py | head -50Repository: ambient-code/platform
Length of output: 396
🏁 Script executed:
# Let's look for SessionWorker.query() mentioned in the review
find . -name "*.py" -path "*ambient-runner*" | xargs rg "class SessionWorker" -A 5 2>/dev/null | head -40Repository: ambient-code/platform
Length of output: 836
🏁 Script executed:
# Create a test to verify asyncio.wait_for() behavior with async iterators
python3 << 'EOF'
import asyncio
# Test: Does cancelling an __anext__() call close the entire async generator?
async def test_wait_for_with_async_generator():
async def my_generator():
"""Async generator that yields slowly"""
for i in range(5):
print(f"Generator: yielding {i}")
await asyncio.sleep(0.5) # Slow enough to timeout
yield i
gen = my_generator()
iter_obj = gen.__aiter__()
iteration_count = 0
timeout_count = 0
while True:
try:
# This will timeout during the sleep in the generator
value = await asyncio.wait_for(iter_obj.__anext__(), timeout=0.1)
print(f"Got value: {value}")
iteration_count += 1
except StopAsyncIteration:
print("Generator exhausted (StopAsyncIteration)")
break
except asyncio.TimeoutError:
print(f"Timeout {timeout_count + 1} - generator still alive, retrying...")
timeout_count += 1
if timeout_count > 10: # Safety limit
print("Too many timeouts, breaking")
break
continue
print(f"\nSummary: Got {iteration_count} values, had {timeout_count} timeouts")
print("Result: Async generator was NOT closed by timeouts - it continued working!")
asyncio.run(test_wait_for_with_async_generator())
EOFRepository: ambient-code/platform
Length of output: 288
🏁 Script executed:
# Let's make the test clearer about what happens with cancellation
python3 << 'EOF'
import asyncio
async def test_detailed_behavior():
"""Test what happens to async generator when wait_for times out"""
async def my_generator():
try:
for i in range(5):
print(f" [GEN] About to yield {i}")
await asyncio.sleep(0.5)
print(f" [GEN] Yielding {i}")
yield i
print(f" [GEN] After yield {i}")
except asyncio.CancelledError:
print(f" [GEN] Received CancelledError!")
raise
except GeneratorExit:
print(f" [GEN] Received GeneratorExit!")
raise
finally:
print(f" [GEN] Finally block - generator closing")
print("Test 1: Normal iteration without timeout")
gen1 = my_generator()
iter1 = gen1.__aiter__()
try:
val = await iter1.__anext__()
print(f"Got: {val}\n")
except Exception as e:
print(f"Error: {e}\n")
print("Test 2: With wait_for timeout")
gen2 = my_generator()
iter2 = gen2.__aiter__()
for attempt in range(3):
print(f"\nAttempt {attempt + 1}:")
try:
val = await asyncio.wait_for(iter2.__anext__(), timeout=0.1)
print(f"Got: {val}")
except asyncio.TimeoutError:
print(f" [MAIN] TimeoutError caught, will retry...")
except StopAsyncIteration:
print(f" [MAIN] StopAsyncIteration - generator exhausted")
break
except asyncio.CancelledError:
print(f" [MAIN] CancelledError caught")
break
asyncio.run(test_detailed_behavior())
EOFRepository: ambient-code/platform
Length of output: 479
🏁 Script executed:
# Let's also check the SessionWorker.query() method to see what it returns
cat -n ./components/runners/ambient-runner/ambient_runner/bridges/claude/session.py | sed -n '1,100p'Repository: ambient-code/platform
Length of output: 4158
🏁 Script executed:
# Find the query method in SessionWorker
rg "async def query" ./components/runners/ambient-runner/ambient_runner/bridges/claude/session.py -A 20Repository: ambient-code/platform
Length of output: 824
🏁 Script executed:
# Let's see where _stream_claude_sdk is called and what message_stream parameter is passed
rg "await.*_stream_claude_sdk\|_stream_claude_sdk(" ./components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py -B 5 -A 2Repository: ambient-code/platform
Length of output: 207
🏁 Script executed:
# Let's look at the actual run method to understand the flow
rg "async def run\|async def execute" ./components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py -A 30 | head -80Repository: ambient-code/platform
Length of output: 47
🏁 Script executed:
# Let's check what SessionWorker.query() actually returns
cat -n ./components/runners/ambient-runner/ambient_runner/bridges/claude/session.py | sed -n '135,160p'Repository: ambient-code/platform
Length of output: 1214
🏁 Script executed:
# Fix the regex - let's search for where _stream_claude_sdk is called
rg "_stream_claude_sdk" ./components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py -B 3 -A 3Repository: ambient-code/platform
Length of output: 524
🏁 Script executed:
# Let's search for the run method or where _stream_claude_sdk is invoked
rg "async def " ./components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py | grep -E "run|execute|process"Repository: ambient-code/platform
Length of output: 83
🏁 Script executed:
# Let's look at the entire _stream_claude_sdk method signature and where it's called from
cat -n ./components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py | sed -n '600,750p'Repository: ambient-code/platform
Length of output: 7831
🏁 Script executed:
# Let's find where run method calls _stream_claude_sdk
cat -n ./components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py | sed -n '200,400p'Repository: ambient-code/platform
Length of output: 10073
🏁 Script executed:
# Let's search for where run method is and find the full context
rg "async def run" ./components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py -A 50 | head -100Repository: ambient-code/platform
Length of output: 2102
🏁 Script executed:
# Let's check the complete flow - see what happens after _stream_claude_sdk completes
cat -n ./components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py | sed -n '395,450p'Repository: ambient-code/platform
Length of output: 2435
🏁 Script executed:
# Let's understand what SessionWorker.query() returns more carefully
cat -n ./components/runners/ambient-runner/ambient_runner/bridges/claude/session.py | sed -n '110,140p'Repository: ambient-code/platform
Length of output: 1442
🏁 Script executed:
# Let's verify what happens when the async iterator is prematurely closed
python3 << 'EOF'
import asyncio
async def test_queue_based_generator():
"""Simulating SessionWorker.query() which yields from a queue"""
async def queue_based_async_gen(queue):
"""Like SessionWorker.query() - yields items from queue"""
try:
while True:
# This would be like output_queue.get()
item = await queue.get()
if item is None:
return
print(f" [QUEUE_GEN] Yielding: {item}")
yield item
except asyncio.CancelledError:
print(f" [QUEUE_GEN] Received CancelledError!")
raise
finally:
print(f" [QUEUE_GEN] Finally block")
# Create queue and populate it with items
queue = asyncio.Queue()
for i in range(5):
await queue.put(i)
print("Test: Timeout-based cancellation with queue-based generator")
gen = queue_based_async_gen(queue)
iter_obj = gen.__aiter__()
received = 0
timeouts = 0
for attempt in range(6):
try:
# Wait 0.05s for next item (queue.get() should succeed immediately)
item = await asyncio.wait_for(iter_obj.__anext__(), timeout=0.05)
print(f"Got item: {item}")
received += 1
except asyncio.TimeoutError:
print(f" [MAIN] TimeoutError on attempt {attempt}")
timeouts += 1
except StopAsyncIteration:
print(f" [MAIN] Generator exhausted")
break
print(f"\nResult: Received {received} items, had {timeouts} timeouts")
print(f"Expected: 5 items, 0 timeouts (queue is fast)")
asyncio.run(test_queue_based_generator())
EOFRepository: ambient-code/platform
Length of output: 504
🏁 Script executed:
# Let's check what the actual SDK message stream looks like - from client.receive_response() or similar
rg "receive_response\|message_stream" ./components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py -B 2 -A 2Repository: ambient-code/platform
Length of output: 47
🏁 Script executed:
# Let's see if there's any handling for the generator being closed
cat -n ./components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py | sed -n '810,900p'Repository: ambient-code/platform
Length of output: 5261
🏁 Script executed:
# Test what happens when a real async generator (not queue-based) gets cancelled
python3 << 'EOF'
import asyncio
async def test_real_async_generator_timeout():
"""Test timeout behavior with a real async generator that models SDK message stream"""
async def sdk_message_generator():
"""Models SDK client.receive_response() - returns messages from SDK"""
try:
# Simulate SDK waiting for first message
print(" [SDK_GEN] Waiting for first message from server...")
await asyncio.sleep(0.3) # Simulates network delay
yield "message_1"
print(" [SDK_GEN] Yielded message_1")
# Simulate thinking time
print(" [SDK_GEN] Waiting for response (thinking)...")
await asyncio.sleep(0.3) # SDK thinking
yield "message_2"
print(" [SDK_GEN] Yielded message_2")
# More messages
await asyncio.sleep(0.3)
yield "message_3"
print(" [SDK_GEN] Yielded message_3")
except asyncio.CancelledError:
print(f" [SDK_GEN] Received CancelledError - generator closing!")
raise
finally:
print(f" [SDK_GEN] Finally block - cleanup")
print("Test: Timeout-based cancellation with SDK message generator")
gen = sdk_message_generator()
iter_obj = gen.__aiter__()
received = 0
timeouts = 0
while True:
try:
# 0.1s timeout - shorter than SDK's 0.3s delays
msg = await asyncio.wait_for(iter_obj.__anext__(), timeout=0.1)
print(f"[MAIN] Got message: {msg}")
received += 1
except asyncio.TimeoutError:
print(f"[MAIN] Timeout #{timeouts + 1} - draining hooks...")
timeouts += 1
if timeouts > 5: # Safety limit
print("[MAIN] Too many timeouts, breaking")
break
continue
except StopAsyncIteration:
print(f"[MAIN] StopAsyncIteration - stream ended")
break
print(f"\nResult: Received {received} messages, had {timeouts} timeouts")
print(f"Expected: 3 messages, 0 timeouts (should have received all before breaking)")
print("\nConclusion: The generator was closed by the first timeout!")
asyncio.run(test_real_async_generator_timeout())
EOFRepository: ambient-code/platform
Length of output: 530
🏁 Script executed:
# Let's verify that when the loop breaks early, RunFinishedEvent handling is correct
cat -n ./components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py | sed -n '810,1000p' | tail -50Repository: ambient-code/platform
Length of output: 2986
🏁 Script executed:
# Check if there's any exception handling that might mask this
rg "except.*Error\|finally" ./components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py | grep -A 2 -B 2 "message_iter\|_stream_claude_sdk"Repository: ambient-code/platform
Length of output: 47
🏁 Script executed:
# Let's trace what happens when the generator is prematurely closed
cat -n ./components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py | sed -n '1050,1150p'Repository: ambient-code/platform
Length of output: 6203
🏁 Script executed:
# Let's verify the end of the _stream_claude_sdk method to see final cleanup
cat -n ./components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py | sed -n '1200,1280p'Repository: ambient-code/platform
Length of output: 4187
🏁 Script executed:
# Check what happens if the generator is closed early - does the finally block run?
cat -n ./components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py | sed -n '1200,1300p'Repository: ambient-code/platform
Length of output: 5082
🏁 Script executed:
# Let's see the complete finally block
cat -n ./components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py | sed -n '1299,1350p'Repository: ambient-code/platform
Length of output: 2879
🏁 Script executed:
# Now let's trace through the logic: what happens when StopAsyncIteration is raised by the prematurely closed generator?
python3 << 'EOF'
import asyncio
async def test_exception_in_finally():
"""Test that finally block runs even when async generator is cancelled"""
async def wrapped_gen():
"""Wraps an SDK generator with timeout logic"""
async def sdk_generator():
try:
for i in range(5):
await asyncio.sleep(0.3)
yield f"message_{i}"
except asyncio.CancelledError:
print(" [SDK_GEN] CancelledError!")
raise
gen = sdk_generator()
iter_obj = gen.__aiter__()
try:
for attempt in range(10):
try:
msg = await asyncio.wait_for(iter_obj.__anext__(), timeout=0.1)
print(f"[MAIN] Got: {msg}")
except asyncio.TimeoutError:
print(f"[MAIN] Timeout #{attempt}")
continue
except StopAsyncIteration:
print(f"[MAIN] StopAsyncIteration - breaking loop")
break
finally:
print("[MAIN] Finally block - cleanup happened!")
yield "cleanup_event"
print("Test: Does finally block run when generator closes early?\n")
async for event in wrapped_gen():
print(f"Event: {event}")
asyncio.run(test_exception_in_finally())
EOFRepository: ambient-code/platform
Length of output: 273
🏁 Script executed:
# Let's verify the actual impact by checking if SDK messages could arrive during the 0.1s timeout windows
cat -n ./components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py | sed -n '813,830p'Repository: ambient-code/platform
Length of output: 896
🏁 Script executed:
# Check if there's any documentation or comments about the timeout behavior
rg "timeout|Race|wait_for" ./components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py -B 2 -A 2Repository: ambient-code/platform
Length of output: 473
🏁 Script executed:
# Let's also confirm this is not a deliberate pattern by checking git history or comments
rg "100ms|drain.*hook|hook.*drain" ./components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py -B 2 -A 2Repository: ambient-code/platform
Length of output: 918
The 100ms timeout loop prematurely closes the SDK message stream.
message_iter is an async iterator over SDK messages (e.g., from SessionWorker.query() or client.receive_response()). When asyncio.wait_for(message_iter.__anext__(), timeout=0.1) times out, it sends CancelledError to the generator, closing it immediately. The first idle gap >100ms will terminate the stream and drop all remaining SDK messages (thinking blocks, tool calls, results). Use a persistent poll task with asyncio.shield() to drain hooks without cancelling the message iterator.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py` around lines
813 - 827, The loop using asyncio.wait_for(message_iter.__anext__(),
timeout=0.1) cancels the iterator on timeout and prematurely closes the SDK
stream; change it to use a persistent poll task and asyncio.shield so timeouts
don't inject CancelledError into message_iter: create a long-lived task (e.g.,
poll_task = asyncio.create_task(message_iter.__anext__())) outside/at the top of
the while loop, await it inside the loop via
asyncio.wait_for(asyncio.shield(poll_task), timeout=0.1), on successful receive
yield the message and replace poll_task with a new asyncio.create_task(...) for
the next item, handle StopAsyncIteration from the task result to break, and on
asyncio.TimeoutError run drain_hook_events() (without cancelling poll_task) so
the iterator remains intact; adjust error handling for CancelledError
accordingly in this function to avoid closing the SDK stream.
Comment on lines
+305
to
+310
| # Task lifecycle → CUSTOM events, no run envelope needed | ||
| if isinstance(msg, (TaskStartedMessage, TaskProgressMessage, TaskNotificationMessage)): | ||
| yield self._adapter._emit_task_event(msg) | ||
| for hook_evt in self._adapter.drain_hook_events(): | ||
| yield hook_evt | ||
| continue |
Contributor
There was a problem hiding this comment.
Don't emit persisted CUSTOM events outside a synthetic run.
This branch sends task:* events and drained hook:* events straight to SSE, while the non-task path below wraps everything in a synthetic RUN_STARTED/RUN_FINISHED. Downstream, the backend persists every /events item, so these CustomEvents are stored without the run envelope or IDs the replay path expects. That is the same standalone-CUSTOM replay shape the backend explicitly avoids in HandleAGUIFeedback. Wrap these events in the same synthetic run too, or stamp thread/run IDs before encoding.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py`
around lines 305 - 310, The branch that handles
TaskStartedMessage/TaskProgressMessage/TaskNotificationMessage currently yields
_adapter._emit_task_event(msg) and any events from _adapter.drain_hook_events()
directly, causing persisted CUSTOM events to lack a synthetic run envelope;
modify this path so these task and hook events are emitted inside the same
synthetic run envelope used by the non-task path (i.e., wrap them with the same
RUN_STARTED and RUN_FINISHED events or apply the same thread/run IDs before
encoding), ensuring the
TaskStartedMessage/TaskProgressMessage/TaskNotificationMessage handling produces
events identical in shape to the synthetic run path rather than standalone
CUSTOM messages.
The hook queue only drains between SDK messages — events pushed from REST endpoints sit there until the next SDK message arrives. Instead, push the task:stop_requested CustomEvent directly into the session worker's active output queue (or between-run queue). The adapter recognizes BaseEvent instances and yields them immediately — no delay, no polling, no timeout. Reverts the 100ms timeout approach. Adds a BaseEvent pass-through check at the top of the message loop. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
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)
components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py (1)
1120-1120:⚠️ Potential issue | 🟠 MajorStreamEvent path skips hook drain, delaying task event delivery.
The
continueat line 1120 causes StreamEvent handling to bypass the hook drain at lines 1243-1245. Since streaming sessions are predominantly StreamEvents, hook-based CustomEvents (task lifecycle, subagent events) accumulate and only drain when non-StreamEvent messages arrive—potentially causing significant delivery latency.🛠️ Suggested fix: Drain hooks before continuing
elif event_type == "message_delta": # Handle message-level delta (e.g., stop_reason, usage) delta_data = event_data.get("delta", {}) stop_reason = delta_data.get("stop_reason") if stop_reason: logger.debug(f"Message stop_reason: {stop_reason}") + # Drain hook events before continuing to next StreamEvent + for hook_event in self.drain_hook_events(): + yield hook_event + continueAlso applies to: 1243-1246
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py` at line 1120, StreamEvent handling currently uses a bare continue which skips the hook-drain block that processes pending custom-event hooks; fix by invoking the existing hook-drain logic immediately before skipping StreamEvents (i.e., call the same drain function/block used at the "hook drain" section so hooks are flushed) and then continue, and make the identical change at the second occurrence where StreamEvent returns early—ensure you locate the StreamEvent branch and the hook-drain block (the code that drains pending custom-event hooks) and move or call that drain right before the continue so streaming messages no longer postpone hook delivery.
♻️ Duplicate comments (5)
components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py (1)
244-257:⚠️ Potential issue | 🟠 MajorHook queue and task registries persist across runs without cleanup.
_hook_event_queue,_task_registry, and_task_outputsare initialized once at construction and never cleared at run start. SinceClaudeAgentAdapterinstances are cached and reused across runs:
- Hook queue contamination: If a previous run ends abruptly with hook events still queued, they will be attributed to the next run.
- Memory growth: Task registries grow unbounded over the adapter's lifetime.
,
🛠️ Suggested fix: Clear state at run start
async def run( self, input_data: RunAgentInput, *, message_stream: Any, ) -> AsyncIterator[BaseEvent]: thread_id = input_data.thread_id or str(uuid.uuid4()) run_id = input_data.run_id or str(uuid.uuid4()) + # Clear per-run transient state to prevent cross-run contamination + while not self._hook_event_queue.empty(): + try: + self._hook_event_queue.get_nowait() + except asyncio.QueueEmpty: + break + self._task_registry.clear() + self._task_outputs.clear() + # Capture halted tool call ID before clearing previous_halted_tool_call_id = self._halted_tool_call_id🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py` around lines 244 - 257, The adapter's instance fields _hook_event_queue, _task_registry, and _task_outputs persist across runs causing cross-run contamination and unbounded memory growth; in the run-start path of ClaudeAgentAdapter (e.g., at the top of whatever method initializes/starts a new run such as start_run/run_session), explicitly clear these structures: drain or recreate _hook_event_queue (use a safe asyncio.Queue drain or assign a new asyncio.Queue()), call _task_registry.clear() and _task_outputs.clear(), and reset _last_run_id appropriately (None or to the new run's id) so each run starts with a clean state and no leftover events/tasks from prior runs.components/runners/ambient-runner/ambient_runner/endpoints/tasks.py (4)
137-139:⚠️ Potential issue | 🟠 MajorDo not return raw read errors to the client.
This 500 path exposes
str(e)directly, which can leak host paths and parser details from the runner. Log the full exception server-side and return a generic failure message instead.🔧 Suggested fix
- except Exception as e: - logger.error(f"Failed to read output for task {task_id}: {e}") - raise HTTPException(status_code=500, detail=str(e)) + except Exception as err: + logger.exception("Failed to read output for task_id=%s", task_id) + raise HTTPException(status_code=500, detail="Failed to read task output") from err🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py` around lines 137 - 139, In the exception handler that currently does logger.error(f"Failed to read output for task {task_id}: {e}") and raises HTTPException(status_code=500, detail=str(e)), change it to log the full exception server-side (use logger.error with the message referencing task_id and include the traceback/exception context via exc_info=True or equivalent) and replace the HTTPException detail with a generic message like "Failed to read task output" (do not include str(e)); locate this in the task output reader endpoint where task_id and logger are used (the except block handling read output) and make the two changes: full server-side logging and generic client-facing error text.
107-136:⚠️ Potential issue | 🟠 MajorThis async handler still performs blocking filesystem work on the event loop.
resolve(),exists(),stat(), andopen()all run synchronously insideget_task_output(). Under concurrent transcript reads, that will block the runner event loop and delay SSE delivery and stop handling for other tasks. Move the path checks and file parsing to a worker thread before returning the payload.🔧 Suggested shape
+import asyncio ... +def _read_output_entries(path: Path): + if path.stat().st_size > _MAX_OUTPUT_BYTES: + raise HTTPException(status_code=413, detail="Transcript too large") + + entries = [] + with path.open() as f: + for raw_line in f: + raw_line = raw_line.strip() + if not raw_line: + continue + try: + entries.append(json.loads(raw_line)) + except json.JSONDecodeError: + entries.append({"raw": raw_line}) + return entries ... - resolved = Path(output_path).resolve() + resolved = await asyncio.to_thread(lambda: Path(output_path).resolve()) ... - if not resolved.exists(): + if not await asyncio.to_thread(resolved.exists): raise HTTPException( status_code=404, detail=f"Output file not found: {output_path}" ) - - if resolved.stat().st_size > _MAX_OUTPUT_BYTES: - raise HTTPException( - status_code=413, detail="Transcript too large" - ) try: - entries = [] - with open(resolved) as f: - for line in f: - line = line.strip() - if line: - try: - entries.append(json.loads(line)) - except json.JSONDecodeError: - entries.append({"raw": line}) + entries = await asyncio.to_thread(_read_output_entries, resolved) return {"task_id": task_id, "output": entries}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py` around lines 107 - 136, get_task_output is doing synchronous filesystem work on the event loop (Path.resolve, exists, stat, open, and file parsing); move the entire block that resolves the path, checks allowed roots, checks existence/size, and reads/parses lines into a synchronous helper (e.g. _read_task_output(task_id, output_path)) and call it via asyncio.to_thread or loop.run_in_executor from get_task_output so the event loop isn't blocked; ensure the helper raises the same HTTPException errors (403/404/413) and returns the {"task_id": task_id, "output": entries} payload so the handler simply awaits the background thread and returns the result.
88-100:⚠️ Potential issue | 🟠 MajorDo not resolve transcripts via a wildcard scan over shared roots.
task_idfrom the URL is interpolated directly into a recursive glob under/tmpand~/.claude/projects. A guessed substring or glob metacharacter can match an unrelated file, so this endpoint can return the wrong task's transcript or other files under those shared directories. Resolve the path from bridge-registered metadata or a deterministic session-scoped filename instead ofrglob(f"*{task_id}*").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py` around lines 88 - 100, The code is insecure because it resolves transcripts by rglob(f"*{task_id}*") over shared roots (search_dirs) using the task_id from the URL; remove the wildcard recursive scan and instead deterministically resolve the transcript path from registered metadata or a session-scoped filename. Replace the rglob logic in the block that sets output_path (variables: output_path, task_id, search_dirs) with a single lookup call such as a bridge/manager API (e.g., Bridge.get_task_metadata(task_id) or TaskManager.lookup_transcript(task_id)) and set output_path only from that metadata or from a deterministic mapping function (e.g., get_transcript_path(task_id)); if no canonical path is returned, return an error rather than scanning the filesystem.
39-76:⚠️ Potential issue | 🟠 MajorDo not convert a failed stop request into a terminal
"stopped"state.
bridge.stop_task()can fail because there is no session manager, nothread_id, or no active worker, but this handler still emitstask:stop_requested, rewrites the registry to"stopped", and returns success. That can show a task as stopped even though no stop signal ever reached the worker. Only publish the stop-request event after a successful stop call, and keep the registry non-terminal until the worker emits its real terminal event.🔧 Suggested fix
- try: - await bridge.stop_task(task_id, thread_id) - except Exception as e: - # SDK may throw if task already finished ("not running", "killed", etc.) - # Treat as success — the task is no longer running either way - logger.warning(f"stop_task({task_id}) raised: {e}") + try: + await bridge.stop_task(task_id, thread_id) + except Exception as err: + logger.exception("stop_task failed for task_id=%s", task_id) + raise HTTPException(status_code=500, detail="Failed to stop task") from err ... - if adapter: - existing = adapter._task_registry.get(task_id, {}) - existing["status"] = "stopped" - adapter._task_registry[task_id] = existing + if adapter and task_id in adapter._task_registry: + adapter._task_registry[task_id]["status"] = "stop_requested"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py` around lines 39 - 76, The handler currently emits the task:stop_requested event and marks adapter._task_registry[task_id] = {"status":"stopped"} even when bridge.stop_task(task_id, thread_id) raises, which can misrepresent the task as terminal; fix by only creating/pushing the CustomEvent (EventType.CUSTOM, name "task:stop_requested") into the session worker queues (sm/get_existing/tid/worker._active_output_queue or worker._between_run_queue) and only update adapter._task_registry to "stopped" after await bridge.stop_task(...) completes without exception; if bridge.stop_task raises, log the error and do NOT emit the event or mutate adapter._task_registry (return an error or propagate the exception as appropriate).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py`:
- Around line 632-641: The hook handling in adapter.py captures session_id into
sid but never uses it, causing a broad rglob over
Path.home()/".claude"/"projects" which can be slow and return incorrect agent
transcripts; update the block that handles hook_event.name ==
"hook:SubagentStart" to use sid (session id) to narrow the search path (e.g.,
look under a session-specific subdirectory or include sid in the filename
pattern) before calling base.rglob, and only set self._task_outputs[agent_id]
when a matching file within that session-specific location is found (refer to
hook_event, agent_id, sid, and self._task_outputs to locate and modify the
logic).
---
Outside diff comments:
In `@components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py`:
- Line 1120: StreamEvent handling currently uses a bare continue which skips the
hook-drain block that processes pending custom-event hooks; fix by invoking the
existing hook-drain logic immediately before skipping StreamEvents (i.e., call
the same drain function/block used at the "hook drain" section so hooks are
flushed) and then continue, and make the identical change at the second
occurrence where StreamEvent returns early—ensure you locate the StreamEvent
branch and the hook-drain block (the code that drains pending custom-event
hooks) and move or call that drain right before the continue so streaming
messages no longer postpone hook delivery.
---
Duplicate comments:
In `@components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py`:
- Around line 244-257: The adapter's instance fields _hook_event_queue,
_task_registry, and _task_outputs persist across runs causing cross-run
contamination and unbounded memory growth; in the run-start path of
ClaudeAgentAdapter (e.g., at the top of whatever method initializes/starts a new
run such as start_run/run_session), explicitly clear these structures: drain or
recreate _hook_event_queue (use a safe asyncio.Queue drain or assign a new
asyncio.Queue()), call _task_registry.clear() and _task_outputs.clear(), and
reset _last_run_id appropriately (None or to the new run's id) so each run
starts with a clean state and no leftover events/tasks from prior runs.
In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py`:
- Around line 137-139: In the exception handler that currently does
logger.error(f"Failed to read output for task {task_id}: {e}") and raises
HTTPException(status_code=500, detail=str(e)), change it to log the full
exception server-side (use logger.error with the message referencing task_id and
include the traceback/exception context via exc_info=True or equivalent) and
replace the HTTPException detail with a generic message like "Failed to read
task output" (do not include str(e)); locate this in the task output reader
endpoint where task_id and logger are used (the except block handling read
output) and make the two changes: full server-side logging and generic
client-facing error text.
- Around line 107-136: get_task_output is doing synchronous filesystem work on
the event loop (Path.resolve, exists, stat, open, and file parsing); move the
entire block that resolves the path, checks allowed roots, checks
existence/size, and reads/parses lines into a synchronous helper (e.g.
_read_task_output(task_id, output_path)) and call it via asyncio.to_thread or
loop.run_in_executor from get_task_output so the event loop isn't blocked;
ensure the helper raises the same HTTPException errors (403/404/413) and returns
the {"task_id": task_id, "output": entries} payload so the handler simply awaits
the background thread and returns the result.
- Around line 88-100: The code is insecure because it resolves transcripts by
rglob(f"*{task_id}*") over shared roots (search_dirs) using the task_id from the
URL; remove the wildcard recursive scan and instead deterministically resolve
the transcript path from registered metadata or a session-scoped filename.
Replace the rglob logic in the block that sets output_path (variables:
output_path, task_id, search_dirs) with a single lookup call such as a
bridge/manager API (e.g., Bridge.get_task_metadata(task_id) or
TaskManager.lookup_transcript(task_id)) and set output_path only from that
metadata or from a deterministic mapping function (e.g.,
get_transcript_path(task_id)); if no canonical path is returned, return an error
rather than scanning the filesystem.
- Around line 39-76: The handler currently emits the task:stop_requested event
and marks adapter._task_registry[task_id] = {"status":"stopped"} even when
bridge.stop_task(task_id, thread_id) raises, which can misrepresent the task as
terminal; fix by only creating/pushing the CustomEvent (EventType.CUSTOM, name
"task:stop_requested") into the session worker queues
(sm/get_existing/tid/worker._active_output_queue or worker._between_run_queue)
and only update adapter._task_registry to "stopped" after await
bridge.stop_task(...) completes without exception; if bridge.stop_task raises,
log the error and do NOT emit the event or mutate adapter._task_registry (return
an error or propagate the exception as appropriate).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b33a0fed-3f1a-47f7-8560-99bd15495a77
📒 Files selected for processing (2)
components/runners/ambient-runner/ag_ui_claude_sdk/adapter.pycomponents/runners/ambient-runner/ambient_runner/endpoints/tasks.py
Comment on lines
+632
to
+641
| if hook_event.name == "hook:SubagentStart" and agent_id: | ||
| sid = val.get("session_id", "") | ||
| if sid: | ||
| from pathlib import Path | ||
| base = Path.home() / ".claude" / "projects" | ||
| if base.exists(): | ||
| expected = f"agent-{agent_id}.jsonl" | ||
| for p in base.rglob(expected): | ||
| self._task_outputs.setdefault(agent_id, str(p)) | ||
| break |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
session_id is captured but unused; consider narrowing the search.
The sid variable captures session_id from the hook payload but is never used. The rglob still scans all projects globally, which:
- Could be slow for large
~/.claude/projectsdirectories - May match the wrong transcript if multiple sessions have agents with the same ID
♻️ Suggested improvement
If the session directory structure includes the session ID, use it to narrow the search:
if hook_event.name == "hook:SubagentStart" and agent_id:
sid = val.get("session_id", "")
if sid:
from pathlib import Path
base = Path.home() / ".claude" / "projects"
if base.exists():
expected = f"agent-{agent_id}.jsonl"
- for p in base.rglob(expected):
- self._task_outputs.setdefault(agent_id, str(p))
- break
+ # Try session-specific path first, fall back to global search
+ for subdir in base.iterdir():
+ session_path = subdir / sid / "subagents" / expected
+ if session_path.exists():
+ self._task_outputs.setdefault(agent_id, str(session_path))
+ break
+ else:
+ # Fallback: global search (slower)
+ for p in base.rglob(expected):
+ self._task_outputs.setdefault(agent_id, str(p))
+ break🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py` around lines
632 - 641, The hook handling in adapter.py captures session_id into sid but
never uses it, causing a broad rglob over Path.home()/".claude"/"projects" which
can be slow and return incorrect agent transcripts; update the block that
handles hook_event.name == "hook:SubagentStart" to use sid (session id) to
narrow the search path (e.g., look under a session-specific subdirectory or
include sid in the filename pattern) before calling base.rglob, and only set
self._task_outputs[agent_id] when a matching file within that session-specific
location is found (refer to hook_event, agent_id, sid, and self._task_outputs to
locate and modify the logic).
…state - Change all debug logging from INFO to DEBUG ([Reader], [Hook]) - Sanitize task_id with alphanumeric regex before rglob to prevent glob injection / path traversal - Use "stopping" intermediate status instead of premature "stopped" so task:completed can overwrite with real final status - Add "stopping" to BackgroundTaskStatus type and StatusIcon (orange spinner distinct from running blue) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
components/runners/ambient-runner/ambient_runner/endpoints/tasks.py (3)
142-144:⚠️ Potential issue | 🟠 MajorDon't expose transcript read failures in the HTTP response.
detail=str(e)leaks internal filesystem paths and parser errors from the runner to clients. Log the exception server-side and return a generic 500 message instead.Suggested hardening
- except Exception as e: - logger.error(f"Failed to read output for task {task_id}: {e}") - raise HTTPException(status_code=500, detail=str(e)) + except Exception: + logger.exception("Failed to read output for task_id=%s", task_id) + raise HTTPException(status_code=500, detail="Failed to read task output")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py` around lines 142 - 144, In the except block that currently reads "except Exception as e:" in ambient_runner/endpoints/tasks.py (the task output/transcript read handler), stop returning str(e) to clients; instead log the full error server-side (use logger.exception or logger.error with the exception) and raise HTTPException with a generic message like "Failed to read transcript" (keep status_code=500). This removes filesystem/parser details from the HTTP response while preserving the detailed server-side log for debugging.
93-105:⚠️ Potential issue | 🟠 MajorResolve transcript paths deterministically instead of first-match scanning.
After a registry miss this walks
/tmpand~/.claude/projectsrecursively and returns the first filename containingtask_id. The same task can legitimately exist in both places (.outputwhile running and archived.jsonlafter completion), so the endpoint becomes traversal-order dependent and can return the wrong transcript. Prefer bridge-registered output paths or a fixed per-task lookup strategy over wildcard discovery.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py` around lines 93 - 105, The current fallback scanning loop (using search_dirs and rglob to set output_path by first-match) is nondeterministic and can return the wrong transcript; instead, if a bridge/registry-registered path exists use that (i.e., prefer the existing output_path from registry), and if not, perform a deterministic per-task lookup: check specific candidate filenames (e.g., /tmp/{task_id}.output, /tmp/{task_id}.jsonl, and ~/.claude/projects/{task_id}.jsonl) in a fixed order without recursive rglob, verifying existence with Path.exists() and assigning output_path only when a candidate exists; remove the wildcard rglob scan and traversal-order dependence in the code block that references output_path, search_dirs, and task_id.
40-77:⚠️ Potential issue | 🟠 MajorOnly transition to
"stopping"after a real stop request succeeds.The blanket
except Exceptionturns session/auth/SDK failures into"stop signal sent", then still emitstask:stop_requestedand writes"stopping"into the registry. That creates phantom stopping tasks when no control-plane signal reached the SDK. Handle only the SDK's explicit "already finished/not running" case as success; propagate everything else and skip the optimistic state change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py` around lines 40 - 77, The current try/except around bridge.stop_task masks all errors and always emits task:stop_requested and sets adapter._task_registry[task_id]["status"]="stopping"; change this so you only treat explicit SDK "already finished"/"not running"/"killed" responses as success: call bridge.stop_task(task_id, thread_id) and if it returns/raises the known SDK-not-running error (detect by specific exception type or exact message), treat as success and continue to emit the CustomEvent and skip changing to "stopping"; for any other Exception re-raise or return an error (do not emit the event or mutate adapter._task_registry). Use the symbols bridge.stop_task, logger.warning, CustomEvent/EventType, bridge._session_manager and adapter._task_registry to locate and implement the selective exception handling and avoid the optimistic state change on general failures.components/frontend/src/hooks/agui/event-handlers.ts (1)
1038-1060:⚠️ Potential issue | 🟠 MajorPreserve existing task metadata when custom events are partial.
task:progressandtask:completedassign optional fields straight fromevent.value, so any later event that omitsusage,last_tool_name,summary, oroutput_fileclears values already stored inbackgroundTasks. Merge those fields withexistingand only replace them when the payload actually includes a new value.Suggested fix
if (name === 'task:progress') { const tasks = new Map(state.backgroundTasks) const existing = tasks.get(value.task_id as string) if (existing) { tasks.set(value.task_id as string, { ...existing, - usage: value.usage as BackgroundTaskUsage | undefined, - last_tool_name: value.last_tool_name as string | undefined, + usage: (value.usage as BackgroundTaskUsage | undefined) ?? existing.usage, + last_tool_name: + (value.last_tool_name as string | undefined) ?? existing.last_tool_name, }) } return { ...state, backgroundTasks: tasks } } @@ tasks.set(taskId, { ...(existing ?? { task_id: taskId, description: '' }), status: (value.status as BackgroundTaskStatus) || 'completed', - summary: value.summary as string | undefined, - usage: value.usage as BackgroundTaskUsage | undefined, - output_file: value.output_file as string | undefined, + summary: (value.summary as string | undefined) ?? existing?.summary, + usage: (value.usage as BackgroundTaskUsage | undefined) ?? existing?.usage, + output_file: (value.output_file as string | undefined) ?? existing?.output_file, })
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/frontend/src/lib/task-utils.tsx`:
- Line 1: Remove the unused Square import from the named imports at the top of
the module; update the import line that currently lists Loader2, CheckCircle2,
XCircle, Square, StopCircle so it no longer includes Square (leave the remaining
icons unchanged) to satisfy the linter and restore CI.
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/session.py`:
- Around line 145-155: The loop in SessionWorker._run() only sets
self._turn_done when the persistent reader dies, but if _run() is blocked on
await self._input_queue.get() it never wakes to handle teardown; modify the
failure handling for self._reader_task.done() (both the occurrences around the
current block and the similar block at 277-284) to signal the main loop
immediately by injecting a dedicated failure sentinel into self._input_queue (or
setting a dedicated asyncio.Event) so _run() unblocks and can tear down the
session; ensure you use a unique sentinel constant (e.g.,
READER_FAILURE_SENTINEL) or an event flag and handle that sentinel/event in
_run() to perform the same cleanup path you currently do when _turn_done is set.
---
Duplicate comments:
In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py`:
- Around line 142-144: In the except block that currently reads "except
Exception as e:" in ambient_runner/endpoints/tasks.py (the task
output/transcript read handler), stop returning str(e) to clients; instead log
the full error server-side (use logger.exception or logger.error with the
exception) and raise HTTPException with a generic message like "Failed to read
transcript" (keep status_code=500). This removes filesystem/parser details from
the HTTP response while preserving the detailed server-side log for debugging.
- Around line 93-105: The current fallback scanning loop (using search_dirs and
rglob to set output_path by first-match) is nondeterministic and can return the
wrong transcript; instead, if a bridge/registry-registered path exists use that
(i.e., prefer the existing output_path from registry), and if not, perform a
deterministic per-task lookup: check specific candidate filenames (e.g.,
/tmp/{task_id}.output, /tmp/{task_id}.jsonl, and
~/.claude/projects/{task_id}.jsonl) in a fixed order without recursive rglob,
verifying existence with Path.exists() and assigning output_path only when a
candidate exists; remove the wildcard rglob scan and traversal-order dependence
in the code block that references output_path, search_dirs, and task_id.
- Around line 40-77: The current try/except around bridge.stop_task masks all
errors and always emits task:stop_requested and sets
adapter._task_registry[task_id]["status"]="stopping"; change this so you only
treat explicit SDK "already finished"/"not running"/"killed" responses as
success: call bridge.stop_task(task_id, thread_id) and if it returns/raises the
known SDK-not-running error (detect by specific exception type or exact
message), treat as success and continue to emit the CustomEvent and skip
changing to "stopping"; for any other Exception re-raise or return an error (do
not emit the event or mutate adapter._task_registry). Use the symbols
bridge.stop_task, logger.warning, CustomEvent/EventType, bridge._session_manager
and adapter._task_registry to locate and implement the selective exception
handling and avoid the optimistic state change on general failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fe6434e4-df5e-4693-9a12-b51ef42d5c40
📒 Files selected for processing (6)
components/frontend/src/hooks/agui/event-handlers.tscomponents/frontend/src/lib/task-utils.tsxcomponents/frontend/src/types/background-task.tscomponents/runners/ambient-runner/ag_ui_claude_sdk/hooks.pycomponents/runners/ambient-runner/ambient_runner/bridges/claude/session.pycomponents/runners/ambient-runner/ambient_runner/endpoints/tasks.py
Comment on lines
+145
to
155
| # Check if reader died between turns | ||
| if self._reader_task.done(): | ||
| exc = self._reader_task.exception() | ||
| logger.error( | ||
| "[SessionWorker] Reader task died for thread=%s: %s", | ||
| self.thread_id, | ||
| exc, | ||
| ) | ||
| break | ||
|
|
||
| item = await self._input_queue.get() |
Contributor
There was a problem hiding this comment.
Wake _run() when the persistent reader dies between turns.
The failure path only sets _turn_done, which helps during an active query but does nothing while _run() is parked on await self._input_queue.get(). In that state the worker stays is_alive with a dead _reader_task, so between-run events stop and the next request hits a broken session before SessionManager can recreate it. Signal the main loop with a dedicated failure sentinel/event so the worker tears down immediately.
Also applies to: 277-284
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/session.py`
around lines 145 - 155, The loop in SessionWorker._run() only sets
self._turn_done when the persistent reader dies, but if _run() is blocked on
await self._input_queue.get() it never wakes to handle teardown; modify the
failure handling for self._reader_task.done() (both the occurrences around the
current block and the similar block at 277-284) to signal the main loop
immediately by injecting a dedicated failure sentinel into self._input_queue (or
setting a dedicated asyncio.Event) so _run() unblocks and can tear down the
session; ensure you use a unique sentinel constant (e.g.,
READER_FAILURE_SENTINEL) or an event flag and handle that sentinel/event in
_run() to perform the same cleanup path you currently do when _turn_done is set.
The SDK silently kills bash tasks without emitting a TaskNotificationMessage, leaving the UI stuck in "stopping" forever. Fix: emit task:completed with status "stopped" directly from the stop endpoint. Removes the intermediate "stopping" state — tasks go straight from "running" to "stopped". Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
components/frontend/src/hooks/agui/event-handlers.ts (1)
1038-1048:⚠️ Potential issue | 🟠 MajorPartial updates still overwrite existing task fields with
undefined.When
task:progressortask:completedevents omit optional fields (usage,last_tool_name,summary,output_file), the current code directly assignsundefined, erasing previously stored values. This causes the Tasks UI to lose metadata as tasks progress.Proposed fix using nullish coalescing
if (name === 'task:progress') { const tasks = new Map(state.backgroundTasks) const existing = tasks.get(value.task_id as string) if (existing) { tasks.set(value.task_id as string, { ...existing, - usage: value.usage as BackgroundTaskUsage | undefined, - last_tool_name: value.last_tool_name as string | undefined, + usage: (value.usage as BackgroundTaskUsage | undefined) ?? existing.usage, + last_tool_name: (value.last_tool_name as string | undefined) ?? existing.last_tool_name, }) } return { ...state, backgroundTasks: tasks } } if (name === 'task:completed') { const tasks = new Map(state.backgroundTasks) const taskId = value.task_id as string const existing = tasks.get(taskId) tasks.set(taskId, { ...(existing ?? { task_id: taskId, description: '' }), status: (value.status as BackgroundTaskStatus) || 'completed', - summary: value.summary as string | undefined, - usage: value.usage as BackgroundTaskUsage | undefined, - output_file: value.output_file as string | undefined, + summary: (value.summary as string | undefined) ?? existing?.summary, + usage: (value.usage as BackgroundTaskUsage | undefined) ?? existing?.usage, + output_file: (value.output_file as string | undefined) ?? existing?.output_file, }) return { ...state, backgroundTasks: tasks } }Also applies to: 1051-1062
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/hooks/agui/event-handlers.ts` around lines 1038 - 1048, The handlers that process 'task:progress' and 'task:completed' events overwrite existing task fields with undefined when optional fields are omitted; update the logic where the code reads the Map from state (backgroundTasks) and the variable existing to merge only provided values by using nullish coalescing (or conditional fallbacks) for fields usage, last_tool_name, summary, and output_file so that the new object sets each field to value.X ?? existing.X instead of directly assigning value.X, then return the updated state with the Map containing the merged task object.components/runners/ambient-runner/ambient_runner/endpoints/tasks.py (2)
145-147:⚠️ Potential issue | 🟠 MajorDo not return raw exception text to clients.
Line 147 propagates
str(e)in the HTTP 500 response, which leaks internal details (file paths, system info). Log server-side with stack trace and return a generic message.🔧 Proposed fix
- except Exception as e: - logger.error(f"Failed to read output for task {task_id}: {e}") - raise HTTPException(status_code=500, detail=str(e)) + except Exception: + logger.exception("Failed to read output for task_id=%s", task_id) + raise HTTPException(status_code=500, detail="Failed to read task output")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py` around lines 145 - 147, The handler that reads task output is leaking internal exception text to clients by raising HTTPException(status_code=500, detail=str(e)); instead, log the full exception and stack trace on the server using logger.exception(...) (or logger.error with traceback) referencing the same context (e.g., task_id) and replace the HTTPException detail with a generic message such as "Internal server error while reading task output" so clients do not receive raw internal details; update the block that currently uses logger.error and raise HTTPException to use logger.exception(...) and a non-sensitive detail string.
40-78:⚠️ Potential issue | 🟠 MajorUnconditional state mutation after failed stop creates incorrect task state.
When
bridge.stop_task()throws (lines 42-45), the handler still emitstask:completedwith status"stopped"and updates the registry to"stopped". This masks real control-plane failures (network errors, auth issues) and causes the UI to show the task as stopped when it may still be running.Only emit the completion event and update registry state when the stop actually succeeds:
🔧 Proposed fix
+ stop_succeeded = False try: await bridge.stop_task(task_id, thread_id) + stop_succeeded = True except Exception as e: # SDK may throw if task already finished ("not running", "killed", etc.) - # Treat as success — the task is no longer running either way - logger.warning(f"stop_task({task_id}) raised: {e}") + # Only treat "not running" / "already stopped" as success + err_msg = str(e).lower() + if "not running" in err_msg or "killed" in err_msg or "already" in err_msg: + stop_succeeded = True + logger.info(f"Task {task_id} already stopped: {e}") + else: + logger.exception("stop_task failed for task_id=%s", task_id) + raise HTTPException(status_code=500, detail="Failed to stop task") # ... CustomEvent creation ... sm = getattr(bridge, "_session_manager", None) - if sm: + if sm and stop_succeeded: # ... emit event logic ... - if adapter: + if adapter and stop_succeeded: existing = adapter._task_registry.get(task_id, {}) existing["status"] = "stopped" adapter._task_registry[task_id] = existing🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py` around lines 40 - 78, The code currently swallows exceptions from bridge.stop_task(...) and unconditionally emits a synthesized task:completed CustomEvent and sets adapter._task_registry[task_id]["status"]="stopped", which can misreport failures; change the flow so the event emission and registry mutation only happen when bridge.stop_task succeeds—either move the event-emission and adapter update into the try block that calls bridge.stop_task or set a boolean (e.g., stopped = True) only on successful return and guard the CustomEvent creation, queue.put(...) logic (worker._active_output_queue / worker._between_run_queue) and adapter._task_registry update with that flag; keep the existing warning log in the except path and do not mutate state there.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@components/frontend/src/hooks/agui/event-handlers.ts`:
- Around line 1038-1048: The handlers that process 'task:progress' and
'task:completed' events overwrite existing task fields with undefined when
optional fields are omitted; update the logic where the code reads the Map from
state (backgroundTasks) and the variable existing to merge only provided values
by using nullish coalescing (or conditional fallbacks) for fields usage,
last_tool_name, summary, and output_file so that the new object sets each field
to value.X ?? existing.X instead of directly assigning value.X, then return the
updated state with the Map containing the merged task object.
In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py`:
- Around line 145-147: The handler that reads task output is leaking internal
exception text to clients by raising HTTPException(status_code=500,
detail=str(e)); instead, log the full exception and stack trace on the server
using logger.exception(...) (or logger.error with traceback) referencing the
same context (e.g., task_id) and replace the HTTPException detail with a generic
message such as "Internal server error while reading task output" so clients do
not receive raw internal details; update the block that currently uses
logger.error and raise HTTPException to use logger.exception(...) and a
non-sensitive detail string.
- Around line 40-78: The code currently swallows exceptions from
bridge.stop_task(...) and unconditionally emits a synthesized task:completed
CustomEvent and sets adapter._task_registry[task_id]["status"]="stopped", which
can misreport failures; change the flow so the event emission and registry
mutation only happen when bridge.stop_task succeeds—either move the
event-emission and adapter update into the try block that calls bridge.stop_task
or set a boolean (e.g., stopped = True) only on successful return and guard the
CustomEvent creation, queue.put(...) logic (worker._active_output_queue /
worker._between_run_queue) and adapter._task_registry update with that flag;
keep the existing warning log in the except path and do not mutate state there.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 80673909-11c6-42fc-84c6-03fdfc97bd1a
📒 Files selected for processing (4)
components/frontend/src/hooks/agui/event-handlers.tscomponents/frontend/src/lib/task-utils.tsxcomponents/frontend/src/types/background-task.tscomponents/runners/ambient-runner/ambient_runner/endpoints/tasks.py
- Fix stale comment in tasks.py (said "stop_requested" but code emits "task:completed") - Remove unused listBackgroundTasks() and its BackgroundTask import - Prefix unused context param with underscore in hook callback - Demote stop request log to DEBUG Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/runners/ambient-runner/ag_ui_claude_sdk/hooks.py (1)
102-114: 🧹 Nitpick | 🔵 TrivialConsider capturing
namefor fallback event identification.The callback doesn't reference
namedirectly—it relies onhook_input.get("hook_event_name", "unknown"). If the SDK payload ever lackshook_event_name, all events will fall back to"unknown", making them indistinguishable. Capturingnameas a default parameter provides a safer fallback.♻️ Proposed defensive fix
for name in sorted(hook_names): async def _callback( hook_input: dict[str, Any], tool_use_id: str | None = None, _context: Any = None, _q: asyncio.Queue = queue, + _hook_name: str = name, # Capture by value for fallback ) -> dict: return await _forward_hook_as_custom_event( - hook_input, tool_use_id or "", _q + hook_input, tool_use_id or "", _q, _hook_name ) hooks[name] = [HookMatcher(hooks=[_callback])]And update
_forward_hook_as_custom_event:async def _forward_hook_as_custom_event( hook_input: dict[str, Any], tool_use_id: str, queue: asyncio.Queue, + fallback_name: str = "unknown", ) -> dict: - event_name = hook_input.get("hook_event_name", "unknown") + event_name = hook_input.get("hook_event_name", fallback_name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ag_ui_claude_sdk/hooks.py` around lines 102 - 114, The closure for _callback doesn't capture the loop variable name, so if hook_input lacks "hook_event_name" events all fall back to "unknown"; update the _callback definition created inside the for name in sorted(hook_names) loop to capture the current name (e.g., add a parameter default like fallback_name: str = name) and pass that into _forward_hook_as_custom_event so _forward_hook_as_custom_event can use the captured fallback_name when hook_input.get("hook_event_name") is missing; adjust the call sites and signature of _forward_hook_as_custom_event accordingly and keep hooks[name] = [HookMatcher(hooks=[_callback])] unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py`:
- Around line 95-107: The recursive rglob search that sets output_path by
scanning search_dirs (using task_id and search_dirs including Path("/tmp") and
Path.home() / ".claude" / "projects") is blocking and can be slow; move the
filesystem walk into a synchronous helper (e.g.,
_find_output_path_in_dirs(search_dirs, task_id)) and call it via
anyio.to_thread.run_sync(...) from the async context so the event loop isn't
blocked, and tighten the search pattern or restrict directories if possible (for
example avoid scanning entire /tmp or add more specific filename patterns) so
the helper returns quickly and you still assign output_path from its result.
---
Duplicate comments:
In `@components/runners/ambient-runner/ag_ui_claude_sdk/hooks.py`:
- Around line 102-114: The closure for _callback doesn't capture the loop
variable name, so if hook_input lacks "hook_event_name" events all fall back to
"unknown"; update the _callback definition created inside the for name in
sorted(hook_names) loop to capture the current name (e.g., add a parameter
default like fallback_name: str = name) and pass that into
_forward_hook_as_custom_event so _forward_hook_as_custom_event can use the
captured fallback_name when hook_input.get("hook_event_name") is missing; adjust
the call sites and signature of _forward_hook_as_custom_event accordingly and
keep hooks[name] = [HookMatcher(hooks=[_callback])] unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 72ae9ba3-b046-4fcb-ae16-3dc2a97b5329
📒 Files selected for processing (3)
components/frontend/src/services/api/tasks.tscomponents/runners/ambient-runner/ag_ui_claude_sdk/hooks.pycomponents/runners/ambient-runner/ambient_runner/endpoints/tasks.py
Comment on lines
+95
to
+107
| if not output_path: | ||
| search_dirs = [ | ||
| Path("/tmp"), | ||
| Path.home() / ".claude" / "projects", | ||
| ] | ||
| for search_dir in search_dirs: | ||
| if not search_dir.exists(): | ||
| continue | ||
| for match in search_dir.rglob(f"*{task_id}*"): | ||
| output_path = str(match) | ||
| break | ||
| if output_path: | ||
| break |
Contributor
There was a problem hiding this comment.
rglob is a blocking operation and can be slow on large directory trees.
The recursive glob search (lines 100-107) scans /tmp and ~/.claude/projects synchronously, which blocks the event loop. Additionally, /tmp can contain many unrelated files, making this potentially slow.
Consider:
- Moving the search to a worker thread via
anyio.to_thread.run_sync(). - Narrowing the search pattern or directories if possible.
🧰 Tools
🪛 Ruff (0.15.7)
[error] 97-97: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/runners/ambient-runner/ambient_runner/endpoints/tasks.py` around
lines 95 - 107, The recursive rglob search that sets output_path by scanning
search_dirs (using task_id and search_dirs including Path("/tmp") and
Path.home() / ".claude" / "projects") is blocking and can be slow; move the
filesystem walk into a synchronous helper (e.g.,
_find_output_path_in_dirs(search_dirs, task_id)) and call it via
anyio.to_thread.run_sync(...) from the async context so the event loop isn't
blocked, and tighten the search pattern or restrict directories if possible (for
example avoid scanning entire /tmp or add more specific filename patterns) so
the helper returns quickly and you still assign output_path from its result.
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
CustomEvents across runner → backend → frontendChanges
Runner (Python)
ag_ui_claude_sdk/hooks.py— Generic hook callback that forwards any SDK hook asCustomEvent(name="hook:{event}"). Default hooks:SubagentStart,SubagentStop,Notification,Stop. Extend viaAGUI_HOOKSenv varag_ui_claude_sdk/adapter.py— HandlesTaskStartedMessage/TaskProgressMessage/TaskNotificationMessage→ emitstask:started/task:progress/task:completedCustomEvents. Hook queue drain between SDK messagesambient_runner/endpoints/tasks.py—POST /tasks/{id}/stop,GET /tasks/{id}/output,GET /tasksbridges/claude/bridge.py— Publicstop_task(),task_registry,task_outputsaccessorsbridges/claude/session.py—stop_task(task_id)onSessionWorkerBackend (Go)
websocket/agui_proxy.go—HandleTaskStop,HandleTaskOutput,HandleTaskListproxy handlers with auth/RBACroutes.go— Three new routes under/agentic-sessions/:session/agui/tasks/Frontend (TypeScript)
types/background-task.ts—BackgroundTask,BackgroundTaskUsage,TaskOutputResponsetypeshooks/agui/event-handlers.ts—CUSTOMevent handler updatingbackgroundTasksMap in AG-UI stateservices/api/tasks.ts— API service for stop/output/listexplorer/background-tasks-tab.tsx— Tasks tab with status icons, progress stats, stop button, transcript dialogexplorer/explorer-panel.tsx— New "Tasks" tab with running count badgeData flow
Test plan
go build ./...passes (backend)npm run buildpasses with 0 errors/warnings (frontend)AGUI_HOOKS=PreToolUse,PostToolUseenv var enables additional hooks🤖 Generated with Claude Code