feat: add mission control-inspired multi-agent features#1
Open
feat: add mission control-inspired multi-agent features#1
Conversation
- Activity Feed: surface lifecycle events (agent spawns/completions/errors) in the Control UI agents tab via real-time WebSocket stream - SOUL.md Editor: dedicated Soul tab for focused agent persona editing - Squad Builder: designer agent example that creates agents conversationally via config.patch and agents_files_set - Task Persistence: file-based task store with agent tool + gateway RPCs (tasks.list, tasks.get, tasks.create, tasks.update)
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds multi-agent POC docs and workspaces; a workspace-scoped Tasks store and Tasks tool; gateway RPC/HTTP handlers for tasks and teams; teams config/schema, zod validation, resolution/autowire helpers and tests; Slack Changes
Sequence DiagramsequenceDiagram
participant User as User / Slack
participant Slash as Slash Handler
participant Gateway as Gateway
participant Teams as Teams Handler
participant Router as Router / Orchestrator
participant Agent as Agent Runtime
participant Store as Tasks Store
participant UI as UI Activity Feed
User->>Slash: /agent <agentId> <message>
Slash->>Gateway: forward normalized agentId + message
Gateway->>Teams: teams.run (or route to agent)
Teams->>Router: build team context, start run (async)
Router->>Agent: deliver message / sessions_spawn
Agent->>Store: createTask(...)
Store-->>Agent: task created (id)
Agent-->>Gateway: emit lifecycle event (phase, runId, agentId, task)
Gateway->>UI: stream lifecycle -> append ActivityEntry
Agent->>Store: updateTask(status="done")
Agent-->>User: final response/result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/src/ui/app-tool-stream.ts (1)
168-173:⚠️ Potential issue | 🟠 Major
resetToolStreamclears the activity feed inconsistently with how the lifecycle handler populates it.The lifecycle handler at lines 231–246 collects agent lifecycle events (
spawn,complete,error) from all sessions without filtering onsessionKeyorchatRunId. However,resetToolStreamat line 172 wipes the entireactivityFeedwhen called during:
- Session switching (app-render.ts:218)
- Chat run completion (app-gateway.ts:218)
- Message sends (app-chat.ts:106)
If the activity feed should persist across sessions (to display full agent spawn history), clearing it on session switch loses that data. If the feed should be per-session, the handler must filter events by session context. Currently, events accumulate globally but are cleared per-session—causing either data loss or mixed session data in the activity panel.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/ui/app-tool-stream.ts` around lines 168 - 173, resetToolStream is wiping host.activityFeed globally while the lifecycle handler currently aggregates agent lifecycle events across all sessions (using sessionKey/chatRunId), causing either lost history on session switch or mixed-session activity; change the behavior so activityFeed is scoped and consistent: either (A) make activityFeed per-session by storing session identifiers with each entry and update resetToolStream to only remove entries matching the current session (use host.currentSessionKey or host.currentChatRunId) before calling flushToolStreamSync, and ensure the lifecycle handler only pushes events whose sessionKey/chatRunId matches the host's current session; or (B) keep activityFeed global and remove its clearing from resetToolStream so it is not wiped on session switch/message send, and adjust the lifecycle handler to continue pushing global events. Update the code paths that call resetToolStream accordingly so the chosen scoping is consistent across resetToolStream, the lifecycle event push logic, and flushToolStreamSync.
🧹 Nitpick comments (8)
ui/src/ui/app-tool-stream.ts (2)
17-24:phasefield onActivityEntryis untyped (string).The known lifecycle phases are a finite set (e.g.,
"spawn","complete","error"). Using a string-literal union makes the intent explicit and catches typos at compile time.♻️ Proposed narrowing
export type ActivityEntry = { ts: number; runId: string; - phase: string; + phase: "spawn" | "complete" | "error" | (string & {}); // extend as new phases are added agentId?: string; task?: string; error?: string; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/ui/app-tool-stream.ts` around lines 17 - 24, The ActivityEntry type currently declares phase as a plain string; change it to a string-literal union of the known lifecycle phases (e.g., "spawn" | "complete" | "error") in the ActivityEntry type so TypeScript enforces valid phases and prevents typos; update any places that construct or check ActivityEntry.phase to use the narrowed union values (or cast/adjust values) so the code compiles with the new type.
243-243: Spread+slice allocates a throw-away intermediate array on every lifecycle event.
[...host.activityFeed, entry].slice(-ACTIVITY_FEED_LIMIT)materialises the full array twice per event. At 100 entries this is negligible, but a push-and-trim idiom is more conventional and avoids the extra allocation.♻️ Proposed alternative
- host.activityFeed = [...host.activityFeed, entry].slice(-ACTIVITY_FEED_LIMIT); + host.activityFeed.push(entry); + if (host.activityFeed.length > ACTIVITY_FEED_LIMIT) { + host.activityFeed = host.activityFeed.slice(-ACTIVITY_FEED_LIMIT); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/ui/app-tool-stream.ts` at line 243, The current line creates a throwaway intermediate array by spreading and slicing host.activityFeed; replace it with an in-place push-and-trim to avoid extra allocations: call host.activityFeed.push(entry) and then, if host.activityFeed.length > ACTIVITY_FEED_LIMIT, remove the excess from the front (e.g., host.activityFeed.splice(0, host.activityFeed.length - ACTIVITY_FEED_LIMIT)); update the single statement that currently sets host.activityFeed = [...host.activityFeed, entry].slice(-ACTIVITY_FEED_LIMIT) to this push-and-trim approach using host.activityFeed, ACTIVITY_FEED_LIMIT, and entry.examples/multi-agent-poc/workspaces/designer/SOUL.md (1)
8-51: Config patch template is missing thesubagentsblock gathered in step 2.Step 2 asks users whether the new agent should be able to spawn subagents (line 13), but the
## Config Patch Formattemplate (lines 41–51) doesn't include asubagents.allowAgentsfield. As a result, the designer will always generate configs without spawn permissions, silently ignoring the user's answer.📝 Suggested template addition
agents: list: - id: <agent-id> name: <Agent Name> workspace: ~/.openclaw/workspaces/<agent-id> model: primary: <model-id> tools: profile: <profile> + # Include the block below only if the user wants subagent spawning: + subagents: + allowAgents: ["*"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/multi-agent-poc/workspaces/designer/SOUL.md` around lines 8 - 51, The config patch template is missing the subagents block so user choices about spawning subagents are ignored; update the Config Patch Format `raw` YAML to include a subagents section under each agent item (e.g., add `subagents:` with `allowAgents: <true|false>` or similar flag) so the gateway's config.patch will persist the user's choice when patching `agents.list`; ensure the field name matches the runtime setting (e.g., `subagents.allowAgents`) and follows the existing merge/mergeObjectArraysById semantics used for `agents.list`.ui/src/ui/views/agents-panels-activity.ts (2)
36-36: Feed order depends on insertion order, notts— out-of-order WebSocket delivery will corrupt the timeline.
toReversed()only inverts the array as received. If events arrive out of order (WebSocket reconnect, buffered replay), newest-first rendering breaks. Sorting bytsbefore reversing ensures chronological correctness regardless of delivery order.♻️ Proposed fix: sort by `ts` before reversing
- const sorted = entries.toReversed(); + const sorted = [...entries].sort((a, b) => a.ts - b.ts).reverse();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/ui/views/agents-panels-activity.ts` at line 36, The feed currently uses entries.toReversed() which only flips arrival order and can display items out-of-order on WebSocket replay; instead, sort entries by their ts property (e.g., ascending numeric compare of item.ts) and then reverse for newest-first rendering before assigning to sorted (preserve use of the sorted variable and toReversed behavior by replacing the single toReversed() call with a sort by ts followed by reversing). Ensure you handle missing/undefined ts values safely (fallback to 0) so the sort is stable.
4-28:phaseIcon/phaseClassswitches don't cover phases present inActivityEntrytype.
ActivityEntry.phaseis typed asstring, meaning the system can emit arbitrary phase values. The current switches only handle"start","end","error", falling back to"·"/"". Any future lifecycle phase (e.g.,"waiting","cancelled") silently gets a blank class and a dot icon with no label disambiguation. Consider documenting expected phases or narrowing theActivityEntry.phasetype to a union.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/ui/views/agents-panels-activity.ts` around lines 4 - 28, The switch handlers in phaseIcon and phaseClass only cover "start","end","error" while ActivityEntry.phase is an open string; update these functions to handle additional known lifecycle values (e.g., "waiting","cancelled","running", etc.) or narrow ActivityEntry.phase to a union type so the compiler enforces allowed phases. Specifically, add explicit cases for any additional phases your app emits and return meaningful icons/classes, and change the default branch to return a predictable fall‑back (for example a generic icon and a neutral class or return the phase string as a label) so new/unknown phases are visible rather than silent; update the ActivityEntry type (or the callers that produce it) to a union of expected phase literals if you prefer strictness.ui/src/ui/views/agents-panels-soul.ts (1)
9-9:agentFileSavingis a global flag — disables SOUL.md's Save button while any file save is in-flight.If the parent ever saves multiple files concurrently, this flag would incorrectly lock SOUL.md's Save button. Consider scoping it to a
Record<string, boolean>(file → saving) if the editor is later extended to handle more files from the same panel.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/ui/views/agents-panels-soul.ts` at line 9, agentFileSaving is a single boolean shared across all files which will wrongly disable SOUL.md's Save button when any other file is being saved; change agentFileSaving to a per-file map like Record<string, boolean> (e.g., agentFileSaving: Record<string, boolean> = {}) and update all usages (checks, sets and clears) in agents-panels-soul.ts to key by the file identifier (filename or id) instead of toggling the global boolean so concurrent saves only affect their own file; ensure the Save button's disabled condition reads agentFileSaving[currentFileId] || false and that you initialize/cleanup entries when save starts/finishes.ui/src/ui/app-render.ts (1)
413-418: Extract"SOUL.md"as a named constant.The filename literal appears three times in these lines. Extracting it reduces coupling should the convention change.
♻️ Proposed refactor
+ const SOUL_FILE = "SOUL.md"; if (panel === "soul") { - state.agentFileActive = "SOUL.md"; - if (resolvedAgentId && !state.agentFileContents["SOUL.md"]) { - void loadAgentFileContent(state, resolvedAgentId, "SOUL.md"); + state.agentFileActive = SOUL_FILE; + if (resolvedAgentId && !state.agentFileContents[SOUL_FILE]) { + void loadAgentFileContent(state, resolvedAgentId, SOUL_FILE); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/ui/app-render.ts` around lines 413 - 418, Extract the repeated literal "SOUL.md" into a named constant (e.g., SOUL_FILENAME) and use that constant wherever the string appears: replace assignments to state.agentFileActive, the lookup state.agentFileContents["SOUL.md"], and the call to loadAgentFileContent(..., "SOUL.md") with the constant; ensure the constant is declared in the same module scope (near the top of ui/src/ui/app-render.ts) and exported if needed so references remain correct.ui/src/ui/views/agents.ts (1)
293-297: Activity feed is intentionally system-wide, not scoped to selected agentThe entire global
props.activityFeedis passed torenderActivityFeed()without filtering. WhileActivityEntryincludes an optionalagentIdfield, the function receives no agent scope parameter and applies no filtering. This means switching between agents displays the same unfiltered stream of all system lifecycle events.This appears to be intentional design—the activity panel presents a system-wide view embedded in the agent tab. However, confirm this behavior is desired and consider documenting it if it may confuse users expecting agent-specific activity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/ui/views/agents.ts` around lines 293 - 297, The activity panel currently passes the global props.activityFeed directly into renderActivityFeed (with ActivityEntry having optional agentId), which shows system-wide events regardless of selected agent; decide whether to scope or document this: if you want agent-scoped view, filter props.activityFeed by agentId === props.selectedAgentId (or similar) before calling renderActivityFeed; if system-wide is intentional, add a clear code comment and UI tooltip/documentation indicating that renderActivityFeed displays global system activity (mention renderActivityFeed, ActivityEntry, props.activityFeed, and props.selectedAgentId to locate the change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/multi-agent-poc/config.yaml`:
- Around line 41-49: The writer agent config block (id: writer) lacks the
subagents.allowAgents field so sessions_spawn calls from the Writing Agent will
be rejected; add a subagents.allowAgents entry under the writer block listing
the agent IDs the writer is permitted to spawn (e.g., "research" or the exact
Research Agent id used elsewhere) so sessions_spawn can succeed, ensuring the
field name is exactly subagents.allowAgents in the writer config.
In `@examples/multi-agent-poc/README.md`:
- Around line 107-110: The README example claims the Writing Agent can spawn a
Research Agent but the writer's configuration in config.yaml lacks a
subagents.allowAgents entry so sessions_spawn will be denied; update the writer
agent's config by adding subagents: allowAgents: ["researcher"] (or the
equivalent agent identifier used in config.yaml) to enable spawning, and ensure
any referenced permission key (sessions_spawn / subagents.allowAgents) is
consistent with the runtime checks, or alternatively remove/qualify the spawning
line in the README so it matches the actual config.
- Around line 36-39: The README currently suggests running the direct copy "cp
config.yaml ~/.openclaw/config.yaml" which will silently overwrite an existing
config; change the instruction to a non-destructive workflow by replacing that
one-liner with guidance to first check for an existing ~/.openclaw/config.yaml
and, if present, create a timestamped backup before copying, or instruct users
to use a non-overwrite copy option or a YAML-merge tool to merge the agents
section into the existing config.yaml instead of overwriting it.
In `@examples/multi-agent-poc/workspaces/router/SOUL.md`:
- Around line 5-25: Add the missing designer agent to the routing table and
classification rules so the router can dispatch to it: update the "Available
Agents" table to include a row for `designer` with a short specialization like
"design, product strategy, squad building" and extend the "Classification Rules"
to include a bullet (or rule) mapping design/squad-building intents (keywords
such as "design", "UX", "product", "squad", "team", "build a squad", "create a
squad") to the designer agent; ensure any examples/instructions that call
sessions_spawn use the designer ID when appropriate so the new workspace can be
reached.
In `@src/agents/tasks-store.ts`:
- Around line 66-91: In createTask validate that params.title and params.agentId
are non-empty (e.g., trim() and check length > 0) at the start of the function
and throw a clear Error (or return a rejected Promise) if either is blank;
update the createTask parameter guard near the top of the function to reject
empty title/agentId with a descriptive message (e.g., "title is required" /
"agentId is required") before generating id, timestamps, or writing the file.
- Around line 39-42: The writeTasksFile function currently writes directly to
filePath which can leave tasks.json corrupted if the process crashes mid-write;
change writeTasksFile to perform an atomic write by creating the directory (keep
fs.mkdir(path.dirname(filePath), { recursive: true })), writing the JSON to a
temporary file in the same directory (e.g., filePath +
`.tmp-${process.pid}-${Date.now()}`), fsyncing the temp file (ensure data and
metadata hit disk), closing it, then fs.rename the temp file to filePath to
atomically replace the target; ensure any temp file is cleaned up on errors. Use
the existing parameter names (filePath, data, TasksFile) and update error
handling accordingly.
- Around line 26-37: The readTasksFile function currently swallows all errors
returning an empty tasks list; change the catch to inspect the thrown error (e)
and only return { tasks: [] } when e.code === 'ENOENT' (file not found),
otherwise rethrow the error so permission, disk, or JSON parse failures surface;
ensure you keep the initial JSON.parse behavior by not catching parse errors
silently and reference readTasksFile and the catch block when making this
change.
- Around line 66-119: createTask and updateTask suffer a TOCTOU race because
they perform read→mutate→write without serialization; fix by introducing a
per-workspace async write queue/lock (e.g., a Map<string, Promise<void>> or
Map<string, Promise<any>> that chains operations) and run the entire
read-modify-write sequence inside a queued task so concurrent callers for the
same workspaceDir execute serially. Modify createTask and updateTask to use that
enqueue/withLock helper when calling
resolveTasksPath/readTasksFile/writeTasksFile so each operation awaits the
previous one for the same workspaceDir; ensure the helper returns the operation
result and clears/errors the queue entry on completion.
In `@src/gateway/protocol/schema/CLAUDE.md`:
- Around line 1-7: Remove the auto-generated CLAUDE.md produced by claude-mem
from the repository (delete the src/gateway/protocol/schema/CLAUDE.md file) and
update the repository ignore rules by adding an entry for CLAUDE.md in
.gitignore so future claude-mem artifacts are not committed; ensure the change
references the tool name "claude-mem" in the git commit message for clarity.
In `@src/gateway/server-methods/tasks.ts`:
- Around line 108-117: The handler for tasks.update casts params.status to
TaskStatus without validation, allowing invalid values to be persisted; update
the tasks.update handler to validate params.status against the allowed enum
values (e.g., ["open","in_progress","done"]) before assigning it to status or
patch.status, using a type-guard or explicit membership check on params.status
and returning/omitting the field when invalid; ensure updateTask only receives a
validated status (referencing params.status, TaskStatus, tasks.update, and
updateTask/patch.status) so arbitrary strings like "pending" are never written
to the store.
- Around line 21-41: The TOCTOU bug is caused by un-synchronized
read→modify→write in tasks-store operations (createTask, updateTask, listTasks,
getTask) allowing concurrent requests (e.g., tasks.list or tasks.create) to
overwrite each other; fix it by adding an in-process async mutex keyed by the
tasks JSON file path inside tasks-store.ts and wrapping every file access (reads
that are followed by writes and any write) with acquire/release so operations
are serialized; reuse the existing createAsyncLock() helper (from
src/infra/pairing-files.ts) or its pattern to create a per-file lock and ensure
createTask, updateTask, listTasks, and getTask all use that lock when performing
read-modify-write sequences.
- Around line 12-18: The resolveWorkspace function currently forwards a
client-supplied workspace string to resolveWorkspaceRoot without constraining
it, allowing path traversal to arbitrary directories; fix this by validating the
canonicalized path returned by resolveWorkspaceRoot against the server's
configured workspace root(s) (use the same prefix/allowlist pattern as in
session-utils around lines 117–118), e.g., obtain the allowed root(s) from your
config or existing helper, compute path.resolve on the input, then assert the
resolved path startsWith one of the allowed roots (or is equal) and return
undefined or throw if it does not; update resolveWorkspace to perform that check
before returning the resolved path.
In `@src/slack/monitor/slash.ts`:
- Around line 394-405: The guard using normalizeAgentId is ineffective because
normalizeAgentId(...) returns DEFAULT_AGENT_ID ("main") for invalid inputs;
update the logic that sets route.agentId/route.matchedBy and agentPrompt to
explicitly check whether the normalized id equals DEFAULT_AGENT_ID (from
session-key.ts) and treat that as an invalid user-supplied id instead of
silently accepting it; specifically, in the block that reads
commandArgs.values.agentId call normalizeAgentId, if the result ===
DEFAULT_AGENT_ID then surface an error or user-facing message (and do not
override route.agentId/route.matchedBy), otherwise set route.agentId =
targetAgentId, route.matchedBy = "default" and set agentPrompt from
commandArgs.values.message as before.
- Around line 399-404: When an agent override is present but no explicit message
is provided, the code leaves agentPrompt as the constructed prompt (from
buildCommandTextFromArgs) and ends up sending command syntax to the agent;
update the logic around commandArgs, agentPrompt and serializeCommandArgs so
that if commandArgs.values.agent (or any agent override) exists and
commandArgs.values.message is empty/whitespace, you surface a validation error
(reject the slash command or return a user-facing error) instead of falling back
to prompt; specifically change the block that reads commandArgs.values.message
and sets agentPrompt (and mirror the same guard for the similar block at
422-425) to require a non-empty message when agent override is specified,
referencing agentPrompt, commandArgs, buildCommandTextFromArgs,
serializeCommandArgs, prompt, and Body/BodyForAgent to locate and fix the
behavior.
In `@ui/src/styles/chat/layout.css`:
- Around line 339-345: The CSS rules on the .chat-controls__agent select (and
the similar .chat-controls__session select) use overflow: hidden and
text-overflow: ellipsis which have no effect on native <select> elements; either
remove those declarations from the .chat-controls__agent select (and the
matching .chat-controls__session select) to avoid dead CSS, or replace the
native <select> with a custom-styled dropdown component and apply truncation
styles there (ensuring the truncation CSS is applied to the custom element/text
container rather than the native select).
In `@ui/src/ui/app-render.helpers.ts`:
- Around line 93-120: switchToAgent currently calls
buildAgentSessionKey(agentId) so the generated session key always uses the
literal "main" and ignores gateway-configured main keys; change switchToAgent to
pass the actual main key resolved by resolveSidebarChatSessionKey(state) into
buildAgentSessionKey (i.e., call buildAgentSessionKey(agentId,
resolveSidebarChatSessionKey(state))) so the agent session is created against
the same mainSessionKey used elsewhere; keep all other state updates
(sessionKey, applySettings, loadAssistantIdentity, loadChatHistory,
refreshChatAvatar, resetToolStream/resetChatScroll) the same but use the new
nextSessionKey returned from buildAgentSessionKey with the resolved main key.
---
Outside diff comments:
In `@ui/src/ui/app-tool-stream.ts`:
- Around line 168-173: resetToolStream is wiping host.activityFeed globally
while the lifecycle handler currently aggregates agent lifecycle events across
all sessions (using sessionKey/chatRunId), causing either lost history on
session switch or mixed-session activity; change the behavior so activityFeed is
scoped and consistent: either (A) make activityFeed per-session by storing
session identifiers with each entry and update resetToolStream to only remove
entries matching the current session (use host.currentSessionKey or
host.currentChatRunId) before calling flushToolStreamSync, and ensure the
lifecycle handler only pushes events whose sessionKey/chatRunId matches the
host's current session; or (B) keep activityFeed global and remove its clearing
from resetToolStream so it is not wiped on session switch/message send, and
adjust the lifecycle handler to continue pushing global events. Update the code
paths that call resetToolStream accordingly so the chosen scoping is consistent
across resetToolStream, the lifecycle event push logic, and flushToolStreamSync.
---
Nitpick comments:
In `@examples/multi-agent-poc/workspaces/designer/SOUL.md`:
- Around line 8-51: The config patch template is missing the subagents block so
user choices about spawning subagents are ignored; update the Config Patch
Format `raw` YAML to include a subagents section under each agent item (e.g.,
add `subagents:` with `allowAgents: <true|false>` or similar flag) so the
gateway's config.patch will persist the user's choice when patching
`agents.list`; ensure the field name matches the runtime setting (e.g.,
`subagents.allowAgents`) and follows the existing merge/mergeObjectArraysById
semantics used for `agents.list`.
In `@ui/src/ui/app-render.ts`:
- Around line 413-418: Extract the repeated literal "SOUL.md" into a named
constant (e.g., SOUL_FILENAME) and use that constant wherever the string
appears: replace assignments to state.agentFileActive, the lookup
state.agentFileContents["SOUL.md"], and the call to loadAgentFileContent(...,
"SOUL.md") with the constant; ensure the constant is declared in the same module
scope (near the top of ui/src/ui/app-render.ts) and exported if needed so
references remain correct.
In `@ui/src/ui/app-tool-stream.ts`:
- Around line 17-24: The ActivityEntry type currently declares phase as a plain
string; change it to a string-literal union of the known lifecycle phases (e.g.,
"spawn" | "complete" | "error") in the ActivityEntry type so TypeScript enforces
valid phases and prevents typos; update any places that construct or check
ActivityEntry.phase to use the narrowed union values (or cast/adjust values) so
the code compiles with the new type.
- Line 243: The current line creates a throwaway intermediate array by spreading
and slicing host.activityFeed; replace it with an in-place push-and-trim to
avoid extra allocations: call host.activityFeed.push(entry) and then, if
host.activityFeed.length > ACTIVITY_FEED_LIMIT, remove the excess from the front
(e.g., host.activityFeed.splice(0, host.activityFeed.length -
ACTIVITY_FEED_LIMIT)); update the single statement that currently sets
host.activityFeed = [...host.activityFeed, entry].slice(-ACTIVITY_FEED_LIMIT) to
this push-and-trim approach using host.activityFeed, ACTIVITY_FEED_LIMIT, and
entry.
In `@ui/src/ui/views/agents-panels-activity.ts`:
- Line 36: The feed currently uses entries.toReversed() which only flips arrival
order and can display items out-of-order on WebSocket replay; instead, sort
entries by their ts property (e.g., ascending numeric compare of item.ts) and
then reverse for newest-first rendering before assigning to sorted (preserve use
of the sorted variable and toReversed behavior by replacing the single
toReversed() call with a sort by ts followed by reversing). Ensure you handle
missing/undefined ts values safely (fallback to 0) so the sort is stable.
- Around line 4-28: The switch handlers in phaseIcon and phaseClass only cover
"start","end","error" while ActivityEntry.phase is an open string; update these
functions to handle additional known lifecycle values (e.g.,
"waiting","cancelled","running", etc.) or narrow ActivityEntry.phase to a union
type so the compiler enforces allowed phases. Specifically, add explicit cases
for any additional phases your app emits and return meaningful icons/classes,
and change the default branch to return a predictable fall‑back (for example a
generic icon and a neutral class or return the phase string as a label) so
new/unknown phases are visible rather than silent; update the ActivityEntry type
(or the callers that produce it) to a union of expected phase literals if you
prefer strictness.
In `@ui/src/ui/views/agents-panels-soul.ts`:
- Line 9: agentFileSaving is a single boolean shared across all files which will
wrongly disable SOUL.md's Save button when any other file is being saved; change
agentFileSaving to a per-file map like Record<string, boolean> (e.g.,
agentFileSaving: Record<string, boolean> = {}) and update all usages (checks,
sets and clears) in agents-panels-soul.ts to key by the file identifier
(filename or id) instead of toggling the global boolean so concurrent saves only
affect their own file; ensure the Save button's disabled condition reads
agentFileSaving[currentFileId] || false and that you initialize/cleanup entries
when save starts/finishes.
In `@ui/src/ui/views/agents.ts`:
- Around line 293-297: The activity panel currently passes the global
props.activityFeed directly into renderActivityFeed (with ActivityEntry having
optional agentId), which shows system-wide events regardless of selected agent;
decide whether to scope or document this: if you want agent-scoped view, filter
props.activityFeed by agentId === props.selectedAgentId (or similar) before
calling renderActivityFeed; if system-wide is intentional, add a clear code
comment and UI tooltip/documentation indicating that renderActivityFeed displays
global system activity (mention renderActivityFeed, ActivityEntry,
props.activityFeed, and props.selectedAgentId to locate the change).
Comment on lines
+41
to
+49
| - id: writer | ||
| name: Writing Agent | ||
| workspace: ~/.openclaw/workspaces/writer | ||
| model: | ||
| primary: claude-sonnet-4-5-20250929 | ||
| tools: | ||
| profile: messaging | ||
| alsoAllow: | ||
| - read |
There was a problem hiding this comment.
Writer agent is missing subagents.allowAgents — documented spawn capability won't work.
README.md line 110 describes a "Writing Agent needs technical details -> spawns Research Agent" handoff, and the writer's SOUL.md presumably includes similar guidance. Without subagents.allowAgents in this config block, any sessions_spawn call from the writer will be rejected at runtime. This is the root cause of the documentation/config mismatch also visible in the README.
📝 Suggested fix
- id: writer
name: Writing Agent
workspace: ~/.openclaw/workspaces/writer
model:
primary: claude-sonnet-4-5-20250929
tools:
profile: messaging
alsoAllow:
- read
+ subagents:
+ allowAgents: ["researcher"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/multi-agent-poc/config.yaml` around lines 41 - 49, The writer agent
config block (id: writer) lacks the subagents.allowAgents field so
sessions_spawn calls from the Writing Agent will be rejected; add a
subagents.allowAgents entry under the writer block listing the agent IDs the
writer is permitted to spawn (e.g., "research" or the exact Research Agent id
used elsewhere) so sessions_spawn can succeed, ensuring the field name is
exactly subagents.allowAgents in the writer config.
Comment on lines
+36
to
+39
| ```bash | ||
| # Or merge the agents section manually | ||
| cp config.yaml ~/.openclaw/config.yaml | ||
| ``` |
There was a problem hiding this comment.
cp overwrites an existing ~/.openclaw/config.yaml without warning.
A user with an existing configuration will silently lose it. Consider using a safer merge instruction or at least adding a backup step.
📝 Safer setup snippet
-# Or merge the agents section manually
-cp config.yaml ~/.openclaw/config.yaml
+# Backup existing config first, then merge manually or copy if starting fresh:
+cp ~/.openclaw/config.yaml ~/.openclaw/config.yaml.bak 2>/dev/null || true
+# Then manually merge the 'agents' section from config.yaml into your config,
+# or copy directly if you have no existing config:
+# cp config.yaml ~/.openclaw/config.yaml🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/multi-agent-poc/README.md` around lines 36 - 39, The README
currently suggests running the direct copy "cp config.yaml
~/.openclaw/config.yaml" which will silently overwrite an existing config;
change the instruction to a non-destructive workflow by replacing that one-liner
with guidance to first check for an existing ~/.openclaw/config.yaml and, if
present, create a timestamped backup before copying, or instruct users to use a
non-overwrite copy option or a YAML-merge tool to merge the agents section into
the existing config.yaml instead of overwriting it.
Comment on lines
+107
to
+110
| - Research Agent discovers an API -> spawns Code Agent to write the integration | ||
| - Code Agent finishes a feature -> spawns Writing Agent to document it | ||
| - Writing Agent needs technical details -> spawns Research Agent to look them up | ||
|
|
There was a problem hiding this comment.
Writer's spawn capability is documented but not configured.
Line 110 states "Writing Agent needs technical details -> spawns Research Agent", but the writer agent in config.yaml has no subagents.allowAgents entry, so sessions_spawn will be denied for that agent. The documentation overstates what the config supports.
Either add subagents: allowAgents: ["researcher"] to the writer in config.yaml, or remove/qualify this example from the README.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/multi-agent-poc/README.md` around lines 107 - 110, The README
example claims the Writing Agent can spawn a Research Agent but the writer's
configuration in config.yaml lacks a subagents.allowAgents entry so
sessions_spawn will be denied; update the writer agent's config by adding
subagents: allowAgents: ["researcher"] (or the equivalent agent identifier used
in config.yaml) to enable spawning, and ensure any referenced permission key
(sessions_spawn / subagents.allowAgents) is consistent with the runtime checks,
or alternatively remove/qualify the spawning line in the README so it matches
the actual config.
Comment on lines
+5
to
+25
| ## Available Agents | ||
|
|
||
| | Agent ID | Specialization | | ||
| | ------------ | -------------------------------------------------------- | | ||
| | `researcher` | Research, fact-finding, summarization, comparing options | | ||
| | `writer` | Writing, editing, communications, documentation | | ||
| | `coder` | Code, debugging, reviews, technical implementation | | ||
|
|
||
| ## How You Work | ||
|
|
||
| 1. Read the user's message | ||
| 2. Classify the intent | ||
| 3. Spawn the right agent using `sessions_spawn` with the user's message as the task | ||
| 4. If the intent is ambiguous or spans multiple specializations, pick the primary one and note in the task that the agent can hand off to others if needed | ||
|
|
||
| ## Classification Rules | ||
|
|
||
| - **Research**: questions, "what is", "how does", "compare", "find", "look up", fact-checking | ||
| - **Writing**: "write", "draft", "edit", "summarize for", "email", "document", "README" | ||
| - **Code**: "code", "implement", "debug", "fix", "review", "build", "deploy", "test", programming languages | ||
| - **Ambiguous**: if the message could go either way, prefer the agent whose specialization is most critical to the task. "Write a Python script" -> coder. "Write a blog post about Python" -> writer. |
There was a problem hiding this comment.
designer agent is missing from the routing table and classification rules.
This PR introduces the designer agent workspace (with its own IDENTITY.md and SOUL.md) and explicitly calls out the Squad Builder as a key feature, yet the router's Available Agents table (lines 7–11) only lists researcher, writer, and coder. The Classification Rules section (lines 22–25) has no corresponding entry for agent-design/squad-building intent. As written, the router can never dispatch to the designer, making the Squad Builder feature unreachable through the intent-routing path.
📝 Suggested addition
| `coder` | Code, debugging, reviews, technical implementation |
+| `designer` | Agent design, building new agents, squad configuration | - **Code**: "code", "implement", "debug", "fix", "review", "build", "deploy", "test", programming languages
+- **Design**: "create agent", "new agent", "build a squad", "agent config", "design a bot"
- **Ambiguous**: if the message could go either way...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/multi-agent-poc/workspaces/router/SOUL.md` around lines 5 - 25, Add
the missing designer agent to the routing table and classification rules so the
router can dispatch to it: update the "Available Agents" table to include a row
for `designer` with a short specialization like "design, product strategy, squad
building" and extend the "Classification Rules" to include a bullet (or rule)
mapping design/squad-building intents (keywords such as "design", "UX",
"product", "squad", "team", "build a squad", "create a squad") to the designer
agent; ensure any examples/instructions that call sessions_spawn use the
designer ID when appropriate so the new workspace can be reached.
Comment on lines
+26
to
+37
| async function readTasksFile(filePath: string): Promise<TasksFile> { | ||
| try { | ||
| const raw = await fs.readFile(filePath, "utf-8"); | ||
| const parsed = JSON.parse(raw) as TasksFile; | ||
| if (!Array.isArray(parsed.tasks)) { | ||
| return { tasks: [] }; | ||
| } | ||
| return parsed; | ||
| } catch { | ||
| return { tasks: [] }; | ||
| } | ||
| } |
There was a problem hiding this comment.
Broad catch silently discards all read errors — risk of data loss
The catch block returns { tasks: [] } for every error, not just ENOENT (file not found). If tasks.json exists but is unreadable (e.g., EPERM, disk error, or even a JSON parse failure on a non-empty file), the function silently returns an empty list. A subsequent createTask call then writes a new file containing only that single task, irrecoverably discarding every previously stored task without any warning.
🛡️ Proposed fix — only suppress ENOENT
async function readTasksFile(filePath: string): Promise<TasksFile> {
try {
const raw = await fs.readFile(filePath, "utf-8");
const parsed = JSON.parse(raw) as TasksFile;
if (!Array.isArray(parsed.tasks)) {
return { tasks: [] };
}
return parsed;
} catch (err) {
- return { tasks: [] };
+ if ((err as NodeJS.ErrnoException).code === "ENOENT") {
+ return { tasks: [] };
+ }
+ throw err;
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agents/tasks-store.ts` around lines 26 - 37, The readTasksFile function
currently swallows all errors returning an empty tasks list; change the catch to
inspect the thrown error (e) and only return { tasks: [] } when e.code ===
'ENOENT' (file not found), otherwise rethrow the error so permission, disk, or
JSON parse failures surface; ensure you keep the initial JSON.parse behavior by
not catching parse errors silently and reference readTasksFile and the catch
block when making this change.
Comment on lines
+108
to
+117
| const title = | ||
| typeof params.title === "string" && params.title.trim() ? params.title.trim() : undefined; | ||
| const status = | ||
| typeof params.status === "string" && params.status.trim() | ||
| ? (params.status.trim() as TaskStatus) | ||
| : undefined; | ||
| const metadata = | ||
| params.metadata && typeof params.metadata === "object" && !Array.isArray(params.metadata) | ||
| ? (params.metadata as Record<string, unknown>) | ||
| : undefined; |
There was a problem hiding this comment.
status is cast to TaskStatus without enum validation — invalid values are persisted.
The tasks.update handler casts params.status directly to TaskStatus with no check against the allowed set ["open", "in_progress", "done"]. updateTask in the store unconditionally writes patch.status onto the task object. A client passing status: "pending" or any arbitrary string will have it persisted, leaving the task invisible to any status-filtered query.
🛡️ Proposed fix
+const VALID_TASK_STATUSES = new Set<string>(["open", "in_progress", "done"]);
const status =
typeof params.status === "string" && params.status.trim()
- ? (params.status.trim() as TaskStatus)
+ ? VALID_TASK_STATUSES.has(params.status.trim())
+ ? (params.status.trim() as TaskStatus)
+ : (() => {
+ respond(false, undefined, errorShape(ErrorCodes.INVALID_REQUEST, `invalid status: ${params.status.trim()}`));
+ return "INVALID" as TaskStatus; // sentinel; respond called above, block continues to early return
+ })()
: undefined;Alternatively, validate and return early:
const status =
typeof params.status === "string" && params.status.trim()
? params.status.trim()
: undefined;
+if (status !== undefined && !["open", "in_progress", "done"].includes(status)) {
+ respond(false, undefined, errorShape(ErrorCodes.INVALID_REQUEST, `invalid status: ${status}`));
+ return;
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gateway/server-methods/tasks.ts` around lines 108 - 117, The handler for
tasks.update casts params.status to TaskStatus without validation, allowing
invalid values to be persisted; update the tasks.update handler to validate
params.status against the allowed enum values (e.g.,
["open","in_progress","done"]) before assigning it to status or patch.status,
using a type-guard or explicit membership check on params.status and
returning/omitting the field when invalid; ensure updateTask only receives a
validated status (referencing params.status, TaskStatus, tasks.update, and
updateTask/patch.status) so arbitrary strings like "pending" are never written
to the store.
Comment on lines
+394
to
+405
| const targetAgentId = normalizeAgentId(String(commandArgs.values.agentId)); | ||
| if (targetAgentId) { | ||
| route.agentId = targetAgentId; | ||
| route.matchedBy = "default"; | ||
| // Use just the message portion as the prompt, not "/agent <id> <message>" | ||
| const message = commandArgs.values.message | ||
| ? String(commandArgs.values.message).trim() | ||
| : ""; | ||
| if (message) { | ||
| agentPrompt = message; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the files exist and examine normalizeAgentId implementation
fd "session-key.ts" --type fRepository: SpotDraft/openclaw
Length of output: 157
🏁 Script executed:
# Find and examine the normalizeAgentId function
rg -n "export.*normalizeAgentId|function normalizeAgentId|const normalizeAgentId" --type ts -A 2Repository: SpotDraft/openclaw
Length of output: 594
🏁 Script executed:
# Check for DEFAULT_AGENT_ID export and value
rg -n "export.*DEFAULT_AGENT_ID|DEFAULT_AGENT_ID.*=" --type tsRepository: SpotDraft/openclaw
Length of output: 361
🏁 Script executed:
# Examine the specific code section in slash.ts around lines 394-405
fd "slash.ts" -p "slack/monitor" --type fRepository: SpotDraft/openclaw
Length of output: 162
🏁 Script executed:
# Find slash.ts in the slack monitor directory
fd "slash\.ts" --type fRepository: SpotDraft/openclaw
Length of output: 88
🏁 Script executed:
# Get the full implementation of normalizeAgentId from session-key.ts
cat -n src/routing/session-key.ts | head -100Repository: SpotDraft/openclaw
Length of output: 3909
🏁 Script executed:
# Check the actual code in slash.ts around lines 394-405
sed -n '390,410p' src/slack/monitor/slash.tsRepository: SpotDraft/openclaw
Length of output: 874
🏁 Script executed:
# Also check what normalizeAgentId is imported as in slash.ts and from where
rg -n "import.*normalizeAgentId|from.*session-key" src/slack/monitor/slash.tsRepository: SpotDraft/openclaw
Length of output: 129
if (targetAgentId) is always true — normalizeAgentId never returns an empty string.
Per its implementation (lines 76–94 of src/routing/session-key.ts), normalizeAgentId falls back to DEFAULT_AGENT_ID ("main") when the input is blank, invalid, or produces an empty string after sanitisation. Since DEFAULT_AGENT_ID is non-empty, the guard at line 395 is always truthy when this block is reached. The practical consequence: a user supplying a purely-special-character agent ID (e.g., /agent !!!! Hello) will have their route silently overridden to the default agent, with no error returned to the user.
To give meaningful feedback when the provided ID normalises to the fallback, compare against the sentinel value explicitly:
🐛 Proposed fix
+import { normalizeAgentId, DEFAULT_AGENT_ID } from "../../routing/session-key.js";
if (commandDefinition?.key === "agent" && commandArgs?.values?.agentId) {
const targetAgentId = normalizeAgentId(String(commandArgs.values.agentId));
- if (targetAgentId) {
+ if (targetAgentId && targetAgentId !== DEFAULT_AGENT_ID) {
route.agentId = targetAgentId;
route.matchedBy = "default";
const message = commandArgs.values.message
? String(commandArgs.values.message).trim()
: "";
if (message) {
agentPrompt = message;
}
+ } else if (!targetAgentId || targetAgentId === DEFAULT_AGENT_ID) {
+ await respond({
+ text: `Unknown agent ID. Use \`/agent <id> <message>\`.`,
+ response_type: "ephemeral",
+ });
+ return;
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/slack/monitor/slash.ts` around lines 394 - 405, The guard using
normalizeAgentId is ineffective because normalizeAgentId(...) returns
DEFAULT_AGENT_ID ("main") for invalid inputs; update the logic that sets
route.agentId/route.matchedBy and agentPrompt to explicitly check whether the
normalized id equals DEFAULT_AGENT_ID (from session-key.ts) and treat that as an
invalid user-supplied id instead of silently accepting it; specifically, in the
block that reads commandArgs.values.agentId call normalizeAgentId, if the result
=== DEFAULT_AGENT_ID then surface an error or user-facing message (and do not
override route.agentId/route.matchedBy), otherwise set route.agentId =
targetAgentId, route.matchedBy = "default" and set agentPrompt from
commandArgs.values.message as before.
Comment on lines
+399
to
+404
| const message = commandArgs.values.message | ||
| ? String(commandArgs.values.message).trim() | ||
| : ""; | ||
| if (message) { | ||
| agentPrompt = message; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file structure and locate the relevant lines
wc -l src/slack/monitor/slash.tsRepository: SpotDraft/openclaw
Length of output: 92
🏁 Script executed:
# Read the context around lines 399-404 and 422-425
sed -n '390,430p' src/slack/monitor/slash.tsRepository: SpotDraft/openclaw
Length of output: 1624
🏁 Script executed:
# Also look at line 168 for the empty-prompt check mentioned
sed -n '160,175p' src/slack/monitor/slash.tsRepository: SpotDraft/openclaw
Length of output: 534
🏁 Script executed:
# Find buildCommandTextFromArgs function
rg -n 'buildCommandTextFromArgs' src/slack/monitor/slash.ts -A 15Repository: SpotDraft/openclaw
Length of output: 2384
🏁 Script executed:
# Find and examine buildCommandTextFromArgs function
fd -name "*.ts" -o -name "*.js" | xargs rg -l 'buildCommandTextFromArgs' | head -5Repository: SpotDraft/openclaw
Length of output: 487
🏁 Script executed:
# Search for the function definition
rg -n 'export.*buildCommandTextFromArgs|function buildCommandTextFromArgs' -A 20Repository: SpotDraft/openclaw
Length of output: 1530
🏁 Script executed:
# Find buildCommandText and serializeCommandArgs functions
rg -n 'function buildCommandText|export function serializeCommandArgs' src/auto-reply/commands-registry.ts -A 15Repository: SpotDraft/openclaw
Length of output: 1695
🏁 Script executed:
# Get more context on serializeCommandArgs
rg -n 'function serializeCommandArgs' src/auto-reply/commands-registry.ts -A 25Repository: SpotDraft/openclaw
Length of output: 835
🏁 Script executed:
# Let me check what happens at lines 422-425 mentioned in "Also applies to"
sed -n '415,435p' src/slack/monitor/slash.tsRepository: SpotDraft/openclaw
Length of output: 877
🏁 Script executed:
# Let me also verify what the agent command definition looks like
rg -n 'key.*:.*"agent"' src -A 10 -B 2Repository: SpotDraft/openclaw
Length of output: 978
🏁 Script executed:
# Check if agent command has formatArgs
rg -n '"agent"' src/auto-reply/commands-registry.data.ts -A 30 | head -50Repository: SpotDraft/openclaw
Length of output: 1142
🏁 Script executed:
# Let me trace through what serializeCommandArgs returns for agent command with only agentId
rg -n 'formatPositionalArgs' src/auto-reply/commands-registry.ts -A 20Repository: SpotDraft/openclaw
Length of output: 1357
🏁 Script executed:
# Let me see the full context around lines 422-425 to understand what "also applies to" means
sed -n '390,450p' src/slack/monitor/slash.ts | cat -nRepository: SpotDraft/openclaw
Length of output: 3017
🏁 Script executed:
# Check if there's similar message handling logic elsewhere in the same function
rg -n 'commandArgs.values.message' src/slack/monitor/slash.tsRepository: SpotDraft/openclaw
Length of output: 178
🏁 Script executed:
# Let me search for where Body/BodyForAgent is used to understand the complete flow
rg -n 'BodyForAgent' src/slack/monitor/slash.ts -B 5 -A 10Repository: SpotDraft/openclaw
Length of output: 730
🏁 Script executed:
# Check if there are other places in the handleSlashCommand function that might need similar fixes
sed -n '370,450p' src/slack/monitor/slash.ts | grep -n "commandArgs.values\|agentPrompt"Repository: SpotDraft/openclaw
Length of output: 404
Require explicit message when agent override is specified.
When /agent researcher is sent without a trailing message, commandArgs.values.message is falsy, leaving agentPrompt as prompt (the result of buildCommandTextFromArgs). Since serializeCommandArgs only includes non-empty argument values, this produces /agent researcher, which is then sent to the agent as Body/BodyForAgent—a confusing payload that contains command syntax rather than the intended message. The initial empty-prompt check at line 168 only guards against a fully empty prompt, not this scenario.
Consider requiring a message when an agent override is in effect:
🛡️ Suggested guard
const message = commandArgs.values.message
? String(commandArgs.values.message).trim()
: "";
if (message) {
agentPrompt = message;
+ } else {
+ await ack();
+ await respond({
+ text: "Please provide a message: `/agent <id> <message>`",
+ response_type: "ephemeral",
+ });
+ return;
}Also applies to: 422-425
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/slack/monitor/slash.ts` around lines 399 - 404, When an agent override is
present but no explicit message is provided, the code leaves agentPrompt as the
constructed prompt (from buildCommandTextFromArgs) and ends up sending command
syntax to the agent; update the logic around commandArgs, agentPrompt and
serializeCommandArgs so that if commandArgs.values.agent (or any agent override)
exists and commandArgs.values.message is empty/whitespace, you surface a
validation error (reject the slash command or return a user-facing error)
instead of falling back to prompt; specifically change the block that reads
commandArgs.values.message and sets agentPrompt (and mirror the same guard for
the similar block at 422-425) to require a non-empty message when agent override
is specified, referencing agentPrompt, commandArgs, buildCommandTextFromArgs,
serializeCommandArgs, prompt, and Body/BodyForAgent to locate and fix the
behavior.
Comment on lines
+339
to
+345
| .chat-controls__agent select { | ||
| padding: 6px 10px; | ||
| font-size: 13px; | ||
| max-width: 240px; | ||
| overflow: hidden; | ||
| text-overflow: ellipsis; | ||
| } |
There was a problem hiding this comment.
text-overflow: ellipsis has no effect on native <select> elements.
Browsers render <select> using OS-native widgets, so overflow: hidden and text-overflow: ellipsis are silently ignored. Long agent names will overflow or be clipped by the browser's own truncation, not this rule. The same pre-existing pattern exists for .chat-controls__session select.
If visible text truncation is important, a custom styled dropdown (not a native <select>) would be needed; otherwise these declarations can be removed without behavioral change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/styles/chat/layout.css` around lines 339 - 345, The CSS rules on the
.chat-controls__agent select (and the similar .chat-controls__session select)
use overflow: hidden and text-overflow: ellipsis which have no effect on native
<select> elements; either remove those declarations from the
.chat-controls__agent select (and the matching .chat-controls__session select)
to avoid dead CSS, or replace the native <select> with a custom-styled dropdown
component and apply truncation styles there (ensuring the truncation CSS is
applied to the custom element/text container rather than the native select).
Comment on lines
+93
to
+120
| function buildAgentSessionKey(agentId: string, mainKey?: string): string { | ||
| const key = mainKey || "main"; | ||
| return `agent:${agentId}:${key}`; | ||
| } | ||
|
|
||
| function switchToAgent(state: AppViewState, agentId: string) { | ||
| const nextSessionKey = buildAgentSessionKey(agentId); | ||
| state.sessionKey = nextSessionKey; | ||
| state.chatMessage = ""; | ||
| state.chatStream = null; | ||
| (state as unknown as OpenClawApp).chatStreamStartedAt = null; | ||
| state.chatRunId = null; | ||
| (state as unknown as OpenClawApp).resetToolStream(); | ||
| (state as unknown as OpenClawApp).resetChatScroll(); | ||
| state.applySettings({ | ||
| ...state.settings, | ||
| sessionKey: nextSessionKey, | ||
| lastActiveSessionKey: nextSessionKey, | ||
| }); | ||
| void state.loadAssistantIdentity(); | ||
| syncUrlWithSessionKey( | ||
| state as unknown as Parameters<typeof syncUrlWithSessionKey>[0], | ||
| nextSessionKey, | ||
| true, | ||
| ); | ||
| void loadChatHistory(state as unknown as ChatState); | ||
| void refreshChatAvatar(state as unknown as Parameters<typeof refreshChatAvatar>[0]); | ||
| } |
There was a problem hiding this comment.
buildAgentSessionKey is called without mainKey — agent sessions are always pinned to "main".
switchToAgent calls buildAgentSessionKey(agentId) on line 99, leaving mainKey as undefined, so the generated key is always agent:${agentId}:main. The mainKey parameter exists but is dead code.
If the gateway-configured main session key is anything other than "main" (e.g., sessionDefaults.mainSessionKey returns "project-alpha"), agent sessions will be on a different base session than the rest of the UI, and chat history won't align.
The intent appears to be threading the current main key through. resolveSidebarChatSessionKey already resolves the actual main key — that value should be passed here.
🐛 Proposed fix
function switchToAgent(state: AppViewState, agentId: string) {
- const nextSessionKey = buildAgentSessionKey(agentId);
+ const mainKey = resolveSidebarChatSessionKey(state);
+ const nextSessionKey = buildAgentSessionKey(agentId, mainKey);
state.sessionKey = nextSessionKey;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/ui/app-render.helpers.ts` around lines 93 - 120, switchToAgent
currently calls buildAgentSessionKey(agentId) so the generated session key
always uses the literal "main" and ignores gateway-configured main keys; change
switchToAgent to pass the actual main key resolved by
resolveSidebarChatSessionKey(state) into buildAgentSessionKey (i.e., call
buildAgentSessionKey(agentId, resolveSidebarChatSessionKey(state))) so the agent
session is created against the same mainSessionKey used elsewhere; keep all
other state updates (sessionKey, applySettings, loadAssistantIdentity,
loadChatHistory, refreshChatAvatar, resetToolStream/resetChatScroll) the same
but use the new nextSessionKey returned from buildAgentSessionKey with the
resolved main key.
…and REST API Introduces a `teams` config key that declares teams (lead + members), auto-wires spawn permissions for team leads, injects team context into lead agent prompts, and exposes teams via HTTP REST API + WebSocket RPC for external tools. - Config: TeamConfig type, Zod validation (lead/members must exist in agents.list) - Resolution: resolveTeam, listTeamsResolved, buildTeamContextPrompt, isTeamLead - Auto-wire: team leads automatically get spawn permissions for their members - Gateway RPC: teams.list, teams.run, teams.status handlers - HTTP REST: GET /v1/teams, POST /v1/teams/:id/run, GET /v1/teams/:id/status - Control UI: Teams tab with overview, chat, and status panels - Tests: 31 tests covering resolve, auto-wire, and config validation
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/config/zod-schema.ts (1)
638-669:⚠️ Potential issue | 🟠 MajorTeams validation is silently skipped when
agents.listis emptyThe
superRefinereturns early at line 641 whenagents.length === 0, which causes the entire teams cross-check block (lines 645–669) to be unreachable for that case. A config that definesteamswithlead/membersbut omitsagents.listpasses validation with no errors at all, silently accepting orphaned team references.The fix is to hoist the teams validation before the early return, or gate it independently:
🛡️ Proposed fix
.superRefine((cfg, ctx) => { const agents = cfg.agents?.list ?? []; - if (agents.length === 0) { - return; - } const agentIds = new Set(agents.map((agent) => agent.id)); const teams = cfg.teams; if (Array.isArray(teams)) { for (let ti = 0; ti < teams.length; ti += 1) { const team = teams[ti]; if (team.lead && !agentIds.has(team.lead)) { ctx.addIssue({ code: z.ZodIssueCode.custom, path: ["teams", ti, "lead"], message: `Unknown agent id "${team.lead}" (not in agents.list).`, }); } if (Array.isArray(team.members)) { for (let mi = 0; mi < team.members.length; mi += 1) { const memberId = team.members[mi]; if (!agentIds.has(memberId)) { ctx.addIssue({ code: z.ZodIssueCode.custom, path: ["teams", ti, "members", mi], message: `Unknown agent id "${memberId}" (not in agents.list).`, }); } } } } } + if (agents.length === 0) { + return; + } const broadcast = cfg.broadcast; // ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/zod-schema.ts` around lines 638 - 669, The teams cross-check is skipped because superRefine returns early when agents.list is empty; move or perform the teams validation regardless of agents.length by hoisting the teams loop before the early return (or remove the agents.length === 0 return and instead only skip agent-derived checks), using the same ctx.addIssue calls and referencing cfg, agents, agentIds, teams, and the existing path entries (["teams", ti, "lead"] and ["teams", ti, "members", mi]) so that unknown lead/member IDs are reported even when agents.list is absent or empty.
🧹 Nitpick comments (10)
src/gateway/protocol/schema/teams.ts (1)
11-16: Optional identifier fields allow empty strings — useNonEmptyStringforsessionKey,channel,to, andthinking.
idempotencyKey(line 17) correctly wrapsNonEmptyString, butsessionKey,channel,to, andthinkinguse the bareType.String(), which permits"". An emptysessionKeyorchannelpassed to the handler would likely be treated as a missing or invalid reference at runtime. Follow the same pattern used foridempotencyKey.🛡️ Proposed fix
- sessionKey: Type.Optional(Type.String()), + sessionKey: Type.Optional(NonEmptyString), deliver: Type.Optional(Type.Boolean()), timeout: Type.Optional(Type.Integer({ minimum: 0 })), - channel: Type.Optional(Type.String()), - to: Type.Optional(Type.String()), - thinking: Type.Optional(Type.String()), + channel: Type.Optional(NonEmptyString), + to: Type.Optional(NonEmptyString), + thinking: Type.Optional(NonEmptyString),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gateway/protocol/schema/teams.ts` around lines 11 - 16, The schema currently allows empty strings for sessionKey, channel, to, and thinking because they use Type.Optional(Type.String()); change each to wrap the NonEmptyString type like idempotencyKey does (e.g., Type.Optional(NonEmptyString)) so empty values are rejected; update the definitions for sessionKey, channel, to, and thinking in the teams schema to use NonEmptyString while keeping them Optional and retaining existing decorators (Type.Optional).ui/src/ui/types.ts (1)
557-574: Field naming inconsistency acrossTeamSummaryandTeamsStatusResult.
TeamSummaryuseslead.id/members[].id, whileTeamsStatusResultusesleadAgentId/members[].agentIdfor agent identifiers. This asymmetry makes cross-referencing between list and status responses less intuitive.The inconsistency is particularly notable since both types represent the same domain entities but with different naming conventions. Consider standardizing on one approach—either use
agentIdconsistently or alignTeamsStatusResultto useleadId/idfor parity withTeamSummary:♻️ Suggested alignment
export type TeamsStatusResult = { teamId: string; name?: string; - leadAgentId: string; + leadId: string; leadActiveRuns: number; totalActiveRuns: number; - members: Array<{ agentId: string; activeRuns: number }>; + members: Array<{ id: string; activeRuns: number }>; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/ui/types.ts` around lines 557 - 574, TeamsStatusResult uses leadAgentId and members[].agentId while TeamSummary uses lead.id and members[].id; update TeamsStatusResult to match TeamSummary by replacing leadAgentId with a lead object containing id (e.g., lead: { id: string; name?: string }) and change members to Array<{ id: string; activeRuns: number }>, keeping other fields (teamId, name, leadActiveRuns, totalActiveRuns) unchanged so the identifier names are consistent across TeamSummary and TeamsStatusResult.src/teams/resolve.test.ts (1)
89-107: Missing case-insensitive test forresolveTeamAllowAgents
resolveTeamis tested for case-insensitivity (Line 50–53), butresolveTeamAllowAgentsis not. SinceresolveTeamAllowAgentsinresolve.tsalso normalises theagentIdbefore comparing it againstteam.lead, an uppercase input like"ENG-LEAD"should still return the member list. Without a test, any regression in normalisation would go undetected.✅ Suggested additional test case
it("is case-insensitive for lead lookup", () => { const cfg = makeConfig(); const members = resolveTeamAllowAgents(cfg, "ENG-LEAD"); expect(members).toContain("eng-backend"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/teams/resolve.test.ts` around lines 89 - 107, Add a case-insensitivity test for resolveTeamAllowAgents: call resolveTeamAllowAgents(makeConfig(), "ENG-LEAD") and assert the returned members include "eng-backend" (and/or other expected members), mirroring the existing resolveTeam case; this verifies the function normalises agentId when comparing against team.lead.src/teams/auto-wire.ts (1)
38-39: Consider exporting constants for the hardcoded tool names.
"sessions_spawn"and"agents_list"are string literals that must stay in sync with the actual tool registrations. If a tool is renamed, this breaks silently.♻️ Suggested refactor
+export const SESSIONS_SPAWN_TOOL = "sessions_spawn"; +export const AGENTS_LIST_TOOL = "agents_list"; const merged = new Set(base); - merged.add("sessions_spawn"); - merged.add("agents_list"); + merged.add(SESSIONS_SPAWN_TOOL); + merged.add(AGENTS_LIST_TOOL);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/teams/auto-wire.ts` around lines 38 - 39, Replace the hardcoded tool name literals used in merged.add("sessions_spawn") and merged.add("agents_list") with exported constants (e.g., export const TOOL_SESSIONS_SPAWN = "sessions_spawn"; export const TOOL_AGENTS_LIST = "agents_list") and import/use those constants wherever the tools are registered or referenced (including tool registration functions/classes and any tests) so renames stay in sync; update the merged.add calls to merged.add(TOOL_SESSIONS_SPAWN) and merged.add(TOOL_AGENTS_LIST) and ensure the constant names are exported from a single source-of-truth module where tool registrations use the same constants.ui/src/ui/app.ts (1)
237-241:teamsPanelandteamsChatMessageselement type duplicate their counterparts inviews/teams.ts.
TeamsPanelis already exported fromui/src/ui/views/teams.ts. Using the inline union"overview" | "chat" | "status"in both files means a panel addition requires two edits and TypeScript won't enforce they stay in sync. The{ role: "user" | "assistant"; text: string; ts: number }inline shape is also duplicated betweenTeamsProps.chatMessagesandteamsChatMessages.♻️ Suggested fix
In
ui/src/ui/views/teams.ts, add:+export type TeamChatMessage = { role: "user" | "assistant"; text: string; ts: number }; export type TeamsProps = { ... - chatMessages: Array<{ role: "user" | "assistant"; text: string; ts: number }>; + chatMessages: TeamChatMessage[];In
ui/src/ui/app.ts:-import type { AgentIdentityResult, TeamsListResult } from "../types.ts"; +import type { TeamsPanel, TeamChatMessage } from "./views/teams.ts"; +import type { AgentIdentityResult, TeamsListResult } from "../types.ts"; ... - `@state`() teamsPanel: "overview" | "chat" | "status" = "overview"; + `@state`() teamsPanel: TeamsPanel = "overview"; - `@state`() teamsChatMessages: Array<{ role: "user" | "assistant"; text: string; ts: number }> = []; + `@state`() teamsChatMessages: TeamChatMessage[] = [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/ui/app.ts` around lines 237 - 241, Replace the duplicated inline types in ui/src/ui/app.ts by reusing the exported types from ui/src/ui/views/teams.ts: import the TeamsPanel union type (or the exported symbol that represents panel values, e.g., TeamsPanel) and replace the inline "overview" | "chat" | "status" on the teamsPanel state, and import the chat message type (or reuse TeamsProps.chatMessages type / exported ChatMessage type) to type teamsChatMessages instead of the inline { role: "user" | "assistant"; text: string; ts: number } shape; update the state declarations teamsPanel and teamsChatMessages to use those imported types so both files share the same definitions.src/gateway/teams-http.ts (1)
74-167: Run and status logic is duplicated verbatim withserver-methods/teams.ts.The idempotency key / session key generation (lines 93–107), the
countActiveRunsForSessionloop (lines 144–165), and theagentCommandinvocation shape all exist inserver-methods/teams.tsat lines 71–127 and 154–166 respectively. A future change to either copy won't propagate to the other.Extract the shared logic — e.g., a
buildTeamRunParamshelper and acollectTeamStatushelper — so both the HTTP and WebSocket handlers call the same implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gateway/teams-http.ts` around lines 74 - 167, The HTTP handler duplicates logic from server-methods/teams.ts — extract shared helpers for run parameter construction and status collection and use them from both places: create a helper (e.g., buildTeamRunParams) that takes {cfg, team, body} and returns idem/runId, sessionKey, leadAgentId, deliver, timeout, thinking, extraSystemPrompt (use normalizeAgentId and buildTeamContextPrompt inside it) and call that from the POST "run" path instead of lines 93–107 and the agentCommand invocation; also create a collectTeamStatus(team) helper that uses normalizeAgentId and countActiveRunsForSession to produce leadActive, totalActive, and members array and call it from the GET "status" path instead of duplicating the loop; update both the HTTP file and server-methods/teams.ts to import and use these helpers so changes stay single-sourced.ui/src/ui/views/teams.ts (1)
157-162:handleKeyDownclosure is recreated on every render.Defining
handleKeyDowninsiderenderTeamChatcreates a new function reference each call, causing Lit to re-bind the@keydownevent listener on every render cycle. Extract it outside the function or memoize it.♻️ Suggested fix
+function handleTeamChatKeyDown(onSendChat: (teamId: string) => void, teamId: string) { + return (e: KeyboardEvent) => { + if (e.key === "Enter" && !e.shiftKey) { + e.preventDefault(); + onSendChat(teamId); + } + }; +} function renderTeamChat(teamId: string, props: TeamsProps) { - const handleKeyDown = (e: KeyboardEvent) => { - if (e.key === "Enter" && !e.shiftKey) { - e.preventDefault(); - props.onSendChat(teamId); - } - };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/ui/views/teams.ts` around lines 157 - 162, The inline handleKeyDown closure inside renderTeamChat causes a new function each render; move it to the component/class scope as a stable method (e.g., handleKeyDown(e: KeyboardEvent)) and stop creating it per render. Update the chat input element in renderTeamChat to include a data attribute for the team id (e.g., data-team-id="${teamId}") and have handleKeyDown read the team id from e.currentTarget.dataset.teamId (or locate the containing element) so it can call props.onSendChat(teamId) when Enter is pressed; this ensures the `@keydown` listener uses a stable function reference instead of being re-bound every render.ui/src/ui/app-render.ts (1)
397-408: Duplicated SOUL.md loading logic betweenonSelectAgentandonSelectPanel.The
loadSoulasync function is near-identical in both callbacks (load agent files → set active file to"SOUL.md"→ conditionally load content). Consider extracting a shared helper to reduce duplication and keep the two paths in sync.Example extraction
// At module scope or in a helpers file async function loadSoulForAgent( state: AppViewState, agentId: string, opts?: { skipFileList?: boolean }, ) { if (!opts?.skipFileList) { await loadAgentFiles(state, agentId); } state.agentFileActive = "SOUL.md"; if (!state.agentFileContents["SOUL.md"]) { void loadAgentFileContent(state, agentId, "SOUL.md"); } }Then use
void loadSoulForAgent(state, agentId)inonSelectAgentandvoid loadSoulForAgent(state, resolvedAgentId, { skipFileList: !needsFileList })inonSelectPanel.Also applies to: 424-434
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/ui/app-render.ts` around lines 397 - 408, The SOUL.md loading logic duplicated in onSelectAgent and onSelectPanel should be extracted into a single helper (e.g., loadSoulForAgent) that wraps await loadAgentFiles(state, agentId) (optional via opts.skipFileList), sets state.agentFileActive = "SOUL.md", and conditionally calls loadAgentFileContent(state, agentId, "SOUL.md") if missing; replace the inline loadSoul functions and both call sites (the loadSoul in the onSelectAgent block and the branch in onSelectPanel) with void loadSoulForAgent(state, agentId) or void loadSoulForAgent(state, agentId, { skipFileList: true/false }) as appropriate so both paths stay in sync.src/teams/resolve.ts (1)
6-13: Inconsistent ID normalization betweenresolveTeamand other functions.
resolveTeamnormalizes team IDs withtrim().toLowerCase(), butlistTeamIdsreturns raw IDs. If callers use alistTeamIdsresult to feed intoresolveTeam, the lookup will succeed due to case-insensitive comparison — so this isn't a correctness issue. However, the mix of manual normalization vs.normalizeAgentIdacross this module is worth noting for maintainability. Team IDs are not agent IDs, so using a different strategy is defensible; just ensure it's intentional.Also applies to: 15-21
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/teams/resolve.ts` around lines 6 - 13, resolveTeam currently normalizes teamId with trim().toLowerCase() while listTeamIds returns raw IDs, leading to inconsistent normalization in this module; decide on a consistent team-ID normalization strategy and apply it across functions (e.g., create or reuse a normalizeTeamId helper and use it in resolveTeam and listTeamIds) or explicitly document why team IDs differ from agent IDs and consistently use normalizeAgentId where appropriate; update references to the helper in resolveTeam and the other affected functions (including listTeamIds) so all lookups and listings use the same normalization routine.ui/src/ui/controllers/teams.ts (1)
57-68: No guard against concurrent sends.
sendTeamMessagedoesn't checkstate.teamsChatSendingbefore proceeding, so rapid double-clicks (or programmatic calls) can fire duplicate requests. The user message is appended optimistically each time, resulting in duplicate messages even if the UI button is disabled.Add a sending guard
export async function sendTeamMessage(state: TeamsState, teamId: string) { const message = state.teamsChatMessage.trim(); - if (!message || !state.client || !state.connected) { + if (!message || !state.client || !state.connected || state.teamsChatSending) { return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/ui/controllers/teams.ts` around lines 57 - 68, sendTeamMessage currently allows concurrent sends because it doesn't check state.teamsChatSending, so fast double-clicks append duplicate entries to state.teamsChatMessages; add an early guard in sendTeamMessage that returns immediately if state.teamsChatSending is true, keep setting state.teamsChatSending = true before the network call, and ensure you clear it (false) in a finally block after the send completes or errors so subsequent sends are allowed; only append the optimistic user message to state.teamsChatMessages when the guard allows sending (i.e., after verifying teamsChatSending was false) to avoid duplicates. Use the existing identifiers sendTeamMessage, state.teamsChatSending, and state.teamsChatMessages to locate and change the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config/zod-schema.teams.ts`:
- Around line 3-13: Update TeamEntrySchema so identifier strings cannot be
empty: change id and lead from z.string() to z.string().min(1) and change
members to z.array(z.string().min(1)) (optionally add .nonempty() on the members
array if you want to forbid zero-member teams). This ensures TeamEntrySchema
(and therefore TeamsSchema) rejects empty "" for id, lead, and each member entry
and prevents resolveTeam/membership logic from seeing phantom empty identifiers.
In `@src/config/zod-schema.ts`:
- Around line 645-669: The teams array validation is missing a uniqueness check
for team.id, so duplicate team entries silently override in resolveTeam; add a
set (e.g., seenTeamIds) while iterating the teams in the same validation block
and if a team.id is already present call ctx.addIssue with code
z.ZodIssueCode.custom, path ["teams", ti, "id"] and a message like `Duplicate
team id "<id>"`; ensure this runs alongside the existing lead/member checks and
handles missing/undefined ids consistently.
In `@src/gateway/server-http.ts`:
- Around line 499-507: The POST /v1/teams/:id/run handler calls agentCommand
fire-and-forget (via void agentCommand(...)) which can produce unhandled promise
rejections; update the handler in teams-http.ts to attach a .catch(...) to the
returned promise from agentCommand to log structured errors (include context
like teamId/requestId) and optionally record metrics or persistent error state;
ensure you still return 202 to the client but never leave agentCommand
rejections unobserved—search for agentCommand(...) and replace the
fire-and-forget call with agentCommand(...).catch(err => { /* structured
processLogger.error or similar */ }).
In `@src/gateway/server-methods/teams.ts`:
- Around line 86-127: The current agentCommand(...).then(...).catch(...) flow
can cause unhandled rejections if respond(...) throws inside either handler;
update the handlers so calls to respond are protected: wrap the respond(true,
...) inside a try/catch in the .then handler and wrap the respond(false, ...)
inside a try/catch in the .catch handler (or use the .then(success, failure)
form and still guard respond), and on respond throwing, convert that error to a
safe log via formatForLog and surface an errorShape(ErrorCodes.UNAVAILABLE,
String(err)) (or similar) so the outer promise never rejects from respond
itself; reference functions: agentCommand, respond, formatForLog, errorShape,
and ErrorCodes.UNAVAILABLE to locate and implement the guarded calls.
In `@src/gateway/teams-http.ts`:
- Around line 118-133: The call to agentCommand is fire-and-forget but currently
unhandled; wrap the returned promise from agentCommand(...) with a .catch
handler to log errors (using the existing logger) to prevent unhandled rejection
crashes—e.g., call agentCommand(..., defaultRuntime,
createDefaultDeps()).catch(err => /* log with context that agentCommand failed
for leadAgentId/runId/sessionKey */). Ensure the catch references
leadAgentId/runId/sessionKey to aid debugging and does not rethrow so the
already-sent 202 remains valid.
In `@ui/src/ui/app-render.ts`:
- Around line 709-734: The onSelectTeam prop currently only sets
state.teamsSelectedId; update the onSelectTeam handler passed into renderTeams
so it also clears the per-team chat state: reset state.teamsChatMessages to an
empty array, state.teamsChatRunId to null/undefined, and state.teamsChatMessage
to an empty string whenever selecting a new team (in the same block where
state.teamsSelectedId is assigned) to avoid showing stale chat from the previous
team.
In `@ui/src/ui/app-view-state.ts`:
- Around line 132-140: Update the SettingsHost.agentsPanel type in
app-settings.ts to include the new "activity" and "soul" variants so it matches
AppViewState.agentsPanel; locate the SettingsHost declaration and the
refreshActiveTab logic (references: SettingsHost.agentsPanel,
AppViewState.agentsPanel, refreshActiveTab) and extend the union type to include
"activity" and "soul" to avoid the never/always-false type issues and assignment
errors.
In `@ui/src/ui/controllers/teams.ts`:
- Around line 4-15: The team chat state (TeamsState: teamsChatMessages and
teamsChatRunId) is shared across all teams so switching teams leaks chat
history; update the onSelectTeam handler (where it currently only sets
teamsSelectedId) to also reset teamsChatMessages to an empty array and
teamsChatRunId to null so each team shows a fresh chat, or alternatively
refactor TeamsState to scope chat per team (e.g., change teamsChatMessages and
teamsChatRunId to maps keyed by team id like Record<string, ChatMessage[]> and
Record<string, string | null> and update all usages to read/write by
teamsSelectedId) — pick one approach and apply the corresponding changes to
onSelectTeam and any components that read/write
teamsChatMessages/teamsChatRunId.
In `@ui/src/ui/views/teams.ts`:
- Around line 211-237: renderTeamStatus currently only renders static team
config (team: TeamsListResult["teams"][number"]) so active run counts from the
teams.status RPC never appear; change the data flow so renderTeamStatus consumes
status data (e.g., a TeamsStatusResult or a combined TeamWithStatus type) and
update the parent that calls renderTeamStatus to call the teams.status gateway
RPC and pass the returned {leadActiveRuns, totalActiveRuns,
members[].activeRuns} into renderTeamStatus. Inside renderTeamStatus (the
function named renderTeamStatus and the JSX that maps team.members) render the
summary (totalActiveRuns and leadActiveRuns) in the card header and show each
member's activeRuns next to their name (use member.activeRuns), falling back to
0 if missing. Ensure the prop/type signatures (TeamsProps or the
renderTeamStatus parameter) are updated to include the status shape so the view
is populated with run counts.
---
Outside diff comments:
In `@src/config/zod-schema.ts`:
- Around line 638-669: The teams cross-check is skipped because superRefine
returns early when agents.list is empty; move or perform the teams validation
regardless of agents.length by hoisting the teams loop before the early return
(or remove the agents.length === 0 return and instead only skip agent-derived
checks), using the same ctx.addIssue calls and referencing cfg, agents,
agentIds, teams, and the existing path entries (["teams", ti, "lead"] and
["teams", ti, "members", mi]) so that unknown lead/member IDs are reported even
when agents.list is absent or empty.
---
Nitpick comments:
In `@src/gateway/protocol/schema/teams.ts`:
- Around line 11-16: The schema currently allows empty strings for sessionKey,
channel, to, and thinking because they use Type.Optional(Type.String()); change
each to wrap the NonEmptyString type like idempotencyKey does (e.g.,
Type.Optional(NonEmptyString)) so empty values are rejected; update the
definitions for sessionKey, channel, to, and thinking in the teams schema to use
NonEmptyString while keeping them Optional and retaining existing decorators
(Type.Optional).
In `@src/gateway/teams-http.ts`:
- Around line 74-167: The HTTP handler duplicates logic from
server-methods/teams.ts — extract shared helpers for run parameter construction
and status collection and use them from both places: create a helper (e.g.,
buildTeamRunParams) that takes {cfg, team, body} and returns idem/runId,
sessionKey, leadAgentId, deliver, timeout, thinking, extraSystemPrompt (use
normalizeAgentId and buildTeamContextPrompt inside it) and call that from the
POST "run" path instead of lines 93–107 and the agentCommand invocation; also
create a collectTeamStatus(team) helper that uses normalizeAgentId and
countActiveRunsForSession to produce leadActive, totalActive, and members array
and call it from the GET "status" path instead of duplicating the loop; update
both the HTTP file and server-methods/teams.ts to import and use these helpers
so changes stay single-sourced.
In `@src/teams/auto-wire.ts`:
- Around line 38-39: Replace the hardcoded tool name literals used in
merged.add("sessions_spawn") and merged.add("agents_list") with exported
constants (e.g., export const TOOL_SESSIONS_SPAWN = "sessions_spawn"; export
const TOOL_AGENTS_LIST = "agents_list") and import/use those constants wherever
the tools are registered or referenced (including tool registration
functions/classes and any tests) so renames stay in sync; update the merged.add
calls to merged.add(TOOL_SESSIONS_SPAWN) and merged.add(TOOL_AGENTS_LIST) and
ensure the constant names are exported from a single source-of-truth module
where tool registrations use the same constants.
In `@src/teams/resolve.test.ts`:
- Around line 89-107: Add a case-insensitivity test for resolveTeamAllowAgents:
call resolveTeamAllowAgents(makeConfig(), "ENG-LEAD") and assert the returned
members include "eng-backend" (and/or other expected members), mirroring the
existing resolveTeam case; this verifies the function normalises agentId when
comparing against team.lead.
In `@src/teams/resolve.ts`:
- Around line 6-13: resolveTeam currently normalizes teamId with
trim().toLowerCase() while listTeamIds returns raw IDs, leading to inconsistent
normalization in this module; decide on a consistent team-ID normalization
strategy and apply it across functions (e.g., create or reuse a normalizeTeamId
helper and use it in resolveTeam and listTeamIds) or explicitly document why
team IDs differ from agent IDs and consistently use normalizeAgentId where
appropriate; update references to the helper in resolveTeam and the other
affected functions (including listTeamIds) so all lookups and listings use the
same normalization routine.
In `@ui/src/ui/app-render.ts`:
- Around line 397-408: The SOUL.md loading logic duplicated in onSelectAgent and
onSelectPanel should be extracted into a single helper (e.g., loadSoulForAgent)
that wraps await loadAgentFiles(state, agentId) (optional via
opts.skipFileList), sets state.agentFileActive = "SOUL.md", and conditionally
calls loadAgentFileContent(state, agentId, "SOUL.md") if missing; replace the
inline loadSoul functions and both call sites (the loadSoul in the onSelectAgent
block and the branch in onSelectPanel) with void loadSoulForAgent(state,
agentId) or void loadSoulForAgent(state, agentId, { skipFileList: true/false })
as appropriate so both paths stay in sync.
In `@ui/src/ui/app.ts`:
- Around line 237-241: Replace the duplicated inline types in ui/src/ui/app.ts
by reusing the exported types from ui/src/ui/views/teams.ts: import the
TeamsPanel union type (or the exported symbol that represents panel values,
e.g., TeamsPanel) and replace the inline "overview" | "chat" | "status" on the
teamsPanel state, and import the chat message type (or reuse
TeamsProps.chatMessages type / exported ChatMessage type) to type
teamsChatMessages instead of the inline { role: "user" | "assistant"; text:
string; ts: number } shape; update the state declarations teamsPanel and
teamsChatMessages to use those imported types so both files share the same
definitions.
In `@ui/src/ui/controllers/teams.ts`:
- Around line 57-68: sendTeamMessage currently allows concurrent sends because
it doesn't check state.teamsChatSending, so fast double-clicks append duplicate
entries to state.teamsChatMessages; add an early guard in sendTeamMessage that
returns immediately if state.teamsChatSending is true, keep setting
state.teamsChatSending = true before the network call, and ensure you clear it
(false) in a finally block after the send completes or errors so subsequent
sends are allowed; only append the optimistic user message to
state.teamsChatMessages when the guard allows sending (i.e., after verifying
teamsChatSending was false) to avoid duplicates. Use the existing identifiers
sendTeamMessage, state.teamsChatSending, and state.teamsChatMessages to locate
and change the logic.
In `@ui/src/ui/types.ts`:
- Around line 557-574: TeamsStatusResult uses leadAgentId and members[].agentId
while TeamSummary uses lead.id and members[].id; update TeamsStatusResult to
match TeamSummary by replacing leadAgentId with a lead object containing id
(e.g., lead: { id: string; name?: string }) and change members to Array<{ id:
string; activeRuns: number }>, keeping other fields (teamId, name,
leadActiveRuns, totalActiveRuns) unchanged so the identifier names are
consistent across TeamSummary and TeamsStatusResult.
In `@ui/src/ui/views/teams.ts`:
- Around line 157-162: The inline handleKeyDown closure inside renderTeamChat
causes a new function each render; move it to the component/class scope as a
stable method (e.g., handleKeyDown(e: KeyboardEvent)) and stop creating it per
render. Update the chat input element in renderTeamChat to include a data
attribute for the team id (e.g., data-team-id="${teamId}") and have
handleKeyDown read the team id from e.currentTarget.dataset.teamId (or locate
the containing element) so it can call props.onSendChat(teamId) when Enter is
pressed; this ensures the `@keydown` listener uses a stable function reference
instead of being re-bound every render.
Comment on lines
+3
to
+13
| export const TeamEntrySchema = z | ||
| .object({ | ||
| id: z.string(), | ||
| name: z.string().optional(), | ||
| lead: z.string(), | ||
| members: z.array(z.string()), | ||
| description: z.string().optional(), | ||
| }) | ||
| .strict(); | ||
|
|
||
| export const TeamsSchema = z.array(TeamEntrySchema).optional(); |
There was a problem hiding this comment.
Missing .min(1) on identifier strings allows empty-string values through validation
id, lead, and array elements in members are all declared as z.string(), which accepts "". Consequences:
- An empty
idbreaksresolveTeam's lookup (any team withid: ""would match unexpected queries). - An empty
leadpasses theif (team.lead && ...)guard inzod-schema.tssilently, leaving a team with no effective orchestrator. - Empty strings in
membersadd phantom agents to the spawn allow-list.
🛡️ Proposed fix
export const TeamEntrySchema = z
.object({
- id: z.string(),
+ id: z.string().min(1),
name: z.string().optional(),
- lead: z.string(),
- members: z.array(z.string()),
+ lead: z.string().min(1),
+ members: z.array(z.string().min(1)),
description: z.string().optional(),
})
.strict();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/config/zod-schema.teams.ts` around lines 3 - 13, Update TeamEntrySchema
so identifier strings cannot be empty: change id and lead from z.string() to
z.string().min(1) and change members to z.array(z.string().min(1)) (optionally
add .nonempty() on the members array if you want to forbid zero-member teams).
This ensures TeamEntrySchema (and therefore TeamsSchema) rejects empty "" for
id, lead, and each member entry and prevents resolveTeam/membership logic from
seeing phantom empty identifiers.
Comment on lines
+645
to
+669
| const teams = cfg.teams; | ||
| if (Array.isArray(teams)) { | ||
| for (let ti = 0; ti < teams.length; ti += 1) { | ||
| const team = teams[ti]; | ||
| if (team.lead && !agentIds.has(team.lead)) { | ||
| ctx.addIssue({ | ||
| code: z.ZodIssueCode.custom, | ||
| path: ["teams", ti, "lead"], | ||
| message: `Unknown agent id "${team.lead}" (not in agents.list).`, | ||
| }); | ||
| } | ||
| if (Array.isArray(team.members)) { | ||
| for (let mi = 0; mi < team.members.length; mi += 1) { | ||
| const memberId = team.members[mi]; | ||
| if (!agentIds.has(memberId)) { | ||
| ctx.addIssue({ | ||
| code: z.ZodIssueCode.custom, | ||
| path: ["teams", ti, "members", mi], | ||
| message: `Unknown agent id "${memberId}" (not in agents.list).`, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Duplicate team IDs are not validated
Two entries with the same id will silently resolve to whichever appears first in resolveTeam. Consider adding a uniqueness check alongside the lead/member cross-checks:
🛡️ Proposed addition
const teams = cfg.teams;
if (Array.isArray(teams)) {
+ const teamIds = new Set<string>();
for (let ti = 0; ti < teams.length; ti += 1) {
const team = teams[ti];
+ const normalizedId = team.id.toLowerCase().trim();
+ if (teamIds.has(normalizedId)) {
+ ctx.addIssue({
+ code: z.ZodIssueCode.custom,
+ path: ["teams", ti, "id"],
+ message: `Duplicate team id "${team.id}".`,
+ });
+ } else {
+ teamIds.add(normalizedId);
+ }
if (team.lead && !agentIds.has(team.lead)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/config/zod-schema.ts` around lines 645 - 669, The teams array validation
is missing a uniqueness check for team.id, so duplicate team entries silently
override in resolveTeam; add a set (e.g., seenTeamIds) while iterating the teams
in the same validation block and if a team.id is already present call
ctx.addIssue with code z.ZodIssueCode.custom, path ["teams", ti, "id"] and a
message like `Duplicate team id "<id>"`; ensure this runs alongside the existing
lead/member checks and handles missing/undefined ids consistently.
Comment on lines
+499
to
+507
| if ( | ||
| await handleTeamsHttpRequest(req, res, { | ||
| auth: resolvedAuth, | ||
| trustedProxies, | ||
| rateLimiter, | ||
| }) | ||
| ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Unhandled rejection in agentCommand fire-and-forget (in teams-http.ts)
The handleTeamsHttpRequest integration is structurally correct, but the POST /v1/teams/:id/run handler in src/gateway/teams-http.ts (visible in the snippet) fires the agent run with no error handler:
void agentCommand(...); // rejection silently becomes unhandledRejectionIf agentCommand rejects, the error is completely invisible — no log, no observable failure, and the client only received a 202. A .catch() for at minimum structured logging should be attached:
🛡️ Proposed fix in src/gateway/teams-http.ts
- void agentCommand(
+ agentCommand(
{ ... },
defaultRuntime,
createDefaultDeps(),
- );
+ ).catch((err) => {
+ // Log the failure; the 202 was already sent to the caller.
+ console.error(`[teams] agentCommand failed for runId=${runId}:`, err);
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gateway/server-http.ts` around lines 499 - 507, The POST
/v1/teams/:id/run handler calls agentCommand fire-and-forget (via void
agentCommand(...)) which can produce unhandled promise rejections; update the
handler in teams-http.ts to attach a .catch(...) to the returned promise from
agentCommand to log structured errors (include context like teamId/requestId)
and optionally record metrics or persistent error state; ensure you still return
202 to the client but never leave agentCommand rejections unobserved—search for
agentCommand(...) and replace the fire-and-forget call with
agentCommand(...).catch(err => { /* structured processLogger.error or similar */
}).
Comment on lines
+86
to
+127
| void agentCommand( | ||
| { | ||
| message: p.message, | ||
| agentId: leadAgentId, | ||
| sessionKey, | ||
| runId, | ||
| deliver: p.deliver ?? false, | ||
| extraSystemPrompt: teamContext, | ||
| messageChannel: "webchat", | ||
| bestEffortDeliver: false, | ||
| thinking: p.thinking, | ||
| timeout: p.timeout?.toString(), | ||
| channel: p.channel, | ||
| to: p.to, | ||
| }, | ||
| defaultRuntime, | ||
| context.deps, | ||
| ) | ||
| .then((result) => { | ||
| const payload = { | ||
| runId, | ||
| teamId: team.id, | ||
| leadAgentId, | ||
| status: "ok" as const, | ||
| result, | ||
| }; | ||
| respond(true, payload, undefined, { runId }); | ||
| }) | ||
| .catch((err) => { | ||
| const error = errorShape(ErrorCodes.UNAVAILABLE, String(err)); | ||
| const payload = { | ||
| runId, | ||
| teamId: team.id, | ||
| leadAgentId, | ||
| status: "error" as const, | ||
| summary: String(err), | ||
| }; | ||
| respond(false, payload, error, { | ||
| runId, | ||
| error: formatForLog(err), | ||
| }); | ||
| }); |
There was a problem hiding this comment.
respond thrown in .then propagates to .catch; if it throws there too, unhandled rejection results.
void agentCommand(...)
.then((result) => { respond(true, ...); }) // respond() throws → goes to .catch
.catch((err) => { respond(false, ...); }); // respond() throws → unhandled rejection
If the WebSocket connection is closed before completion, the framework's respond implementation may throw. Using rejection-argument form of .then or wrapping respond calls in try/catch prevents the leak:
♻️ Suggested fix
- void agentCommand(...)
- .then((result) => {
- const payload = { ... };
- respond(true, payload, undefined, { runId });
- })
- .catch((err) => {
- const error = errorShape(ErrorCodes.UNAVAILABLE, String(err));
- const payload = { ... };
- respond(false, payload, error, { runId, error: formatForLog(err) });
- });
+ void agentCommand(...).then(
+ (result) => {
+ try {
+ respond(true, { runId, teamId: team.id, leadAgentId, status: "ok" as const, result }, undefined, { runId });
+ } catch { /* connection closed */ }
+ },
+ (err) => {
+ try {
+ respond(false, { runId, teamId: team.id, leadAgentId, status: "error" as const, summary: String(err) }, errorShape(ErrorCodes.UNAVAILABLE, String(err)), { runId, error: formatForLog(err) });
+ } catch { /* connection closed */ }
+ },
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gateway/server-methods/teams.ts` around lines 86 - 127, The current
agentCommand(...).then(...).catch(...) flow can cause unhandled rejections if
respond(...) throws inside either handler; update the handlers so calls to
respond are protected: wrap the respond(true, ...) inside a try/catch in the
.then handler and wrap the respond(false, ...) inside a try/catch in the .catch
handler (or use the .then(success, failure) form and still guard respond), and
on respond throwing, convert that error to a safe log via formatForLog and
surface an errorShape(ErrorCodes.UNAVAILABLE, String(err)) (or similar) so the
outer promise never rejects from respond itself; reference functions:
agentCommand, respond, formatForLog, errorShape, and ErrorCodes.UNAVAILABLE to
locate and implement the guarded calls.
Comment on lines
+118
to
+133
| void agentCommand( | ||
| { | ||
| message, | ||
| agentId: leadAgentId, | ||
| sessionKey, | ||
| runId, | ||
| deliver, | ||
| extraSystemPrompt: teamContext, | ||
| messageChannel: "webchat", | ||
| bestEffortDeliver: false, | ||
| thinking, | ||
| timeout: timeout?.toString(), | ||
| }, | ||
| defaultRuntime, | ||
| createDefaultDeps(), | ||
| ); |
There was a problem hiding this comment.
void agentCommand() without .catch risks process termination.
In Node.js ≥ 15 (--unhandled-rejections=throw default), an uncaught rejection from agentCommand will crash the server process. The 202 has already been sent so the error can't reach the client, but it must at least be logged.
🐛 Proposed fix
- void agentCommand(
+ agentCommand(
{
message,
agentId: leadAgentId,
sessionKey,
runId,
deliver,
extraSystemPrompt: teamContext,
messageChannel: "webchat",
bestEffortDeliver: false,
thinking,
timeout: timeout?.toString(),
},
defaultRuntime,
createDefaultDeps(),
- );
+ ).catch((err: unknown) => {
+ console.error(`[teams.run] agentCommand failed for team "${team.id}" runId=${runId}: ${String(err)}`);
+ });📝 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
| void agentCommand( | |
| { | |
| message, | |
| agentId: leadAgentId, | |
| sessionKey, | |
| runId, | |
| deliver, | |
| extraSystemPrompt: teamContext, | |
| messageChannel: "webchat", | |
| bestEffortDeliver: false, | |
| thinking, | |
| timeout: timeout?.toString(), | |
| }, | |
| defaultRuntime, | |
| createDefaultDeps(), | |
| ); | |
| agentCommand( | |
| { | |
| message, | |
| agentId: leadAgentId, | |
| sessionKey, | |
| runId, | |
| deliver, | |
| extraSystemPrompt: teamContext, | |
| messageChannel: "webchat", | |
| bestEffortDeliver: false, | |
| thinking, | |
| timeout: timeout?.toString(), | |
| }, | |
| defaultRuntime, | |
| createDefaultDeps(), | |
| ).catch((err: unknown) => { | |
| console.error(`[teams.run] agentCommand failed for team "${team.id}" runId=${runId}: ${String(err)}`); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gateway/teams-http.ts` around lines 118 - 133, The call to agentCommand
is fire-and-forget but currently unhandled; wrap the returned promise from
agentCommand(...) with a .catch handler to log errors (using the existing
logger) to prevent unhandled rejection crashes—e.g., call agentCommand(...,
defaultRuntime, createDefaultDeps()).catch(err => /* log with context that
agentCommand failed for leadAgentId/runId/sessionKey */). Ensure the catch
references leadAgentId/runId/sessionKey to aid debugging and does not rethrow so
the already-sent 202 remains valid.
Comment on lines
+709
to
+734
| ${ | ||
| state.tab === "teams" | ||
| ? renderTeams({ | ||
| loading: state.teamsLoading, | ||
| error: state.teamsError, | ||
| teamsList: state.teamsList, | ||
| selectedTeamId: state.teamsSelectedId, | ||
| activePanel: state.teamsPanel, | ||
| chatMessage: state.teamsChatMessage, | ||
| chatSending: state.teamsChatSending, | ||
| chatMessages: state.teamsChatMessages, | ||
| agentIdentityById: state.agentIdentityById, | ||
| onRefresh: () => loadTeams(state), | ||
| onSelectTeam: (teamId) => { | ||
| state.teamsSelectedId = teamId; | ||
| }, | ||
| onSelectPanel: (panel) => { | ||
| state.teamsPanel = panel; | ||
| }, | ||
| onChatMessageChange: (msg) => { | ||
| state.teamsChatMessage = msg; | ||
| }, | ||
| onSendChat: (teamId) => sendTeamMessage(state, teamId), | ||
| }) | ||
| : nothing | ||
| } |
There was a problem hiding this comment.
Teams tab wiring looks correct, but onSelectTeam doesn't reset chat state.
The props are properly mapped and the rendering block follows the established pattern of other tabs. However, as noted in the teams controller review, onSelectTeam (Line 722) only updates teamsSelectedId without clearing teamsChatMessages, teamsChatRunId, or teamsChatMessage — so switching teams shows stale chat from the previous team.
Reset chat state on team switch
onSelectTeam: (teamId) => {
+ state.teamsChatMessages = [];
+ state.teamsChatRunId = null;
+ state.teamsChatMessage = "";
state.teamsSelectedId = teamId;
},📝 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
| ${ | |
| state.tab === "teams" | |
| ? renderTeams({ | |
| loading: state.teamsLoading, | |
| error: state.teamsError, | |
| teamsList: state.teamsList, | |
| selectedTeamId: state.teamsSelectedId, | |
| activePanel: state.teamsPanel, | |
| chatMessage: state.teamsChatMessage, | |
| chatSending: state.teamsChatSending, | |
| chatMessages: state.teamsChatMessages, | |
| agentIdentityById: state.agentIdentityById, | |
| onRefresh: () => loadTeams(state), | |
| onSelectTeam: (teamId) => { | |
| state.teamsSelectedId = teamId; | |
| }, | |
| onSelectPanel: (panel) => { | |
| state.teamsPanel = panel; | |
| }, | |
| onChatMessageChange: (msg) => { | |
| state.teamsChatMessage = msg; | |
| }, | |
| onSendChat: (teamId) => sendTeamMessage(state, teamId), | |
| }) | |
| : nothing | |
| } | |
| ${ | |
| state.tab === "teams" | |
| ? renderTeams({ | |
| loading: state.teamsLoading, | |
| error: state.teamsError, | |
| teamsList: state.teamsList, | |
| selectedTeamId: state.teamsSelectedId, | |
| activePanel: state.teamsPanel, | |
| chatMessage: state.teamsChatMessage, | |
| chatSending: state.teamsChatSending, | |
| chatMessages: state.teamsChatMessages, | |
| agentIdentityById: state.agentIdentityById, | |
| onRefresh: () => loadTeams(state), | |
| onSelectTeam: (teamId) => { | |
| state.teamsChatMessages = []; | |
| state.teamsChatRunId = null; | |
| state.teamsChatMessage = ""; | |
| state.teamsSelectedId = teamId; | |
| }, | |
| onSelectPanel: (panel) => { | |
| state.teamsPanel = panel; | |
| }, | |
| onChatMessageChange: (msg) => { | |
| state.teamsChatMessage = msg; | |
| }, | |
| onSendChat: (teamId) => sendTeamMessage(state, teamId), | |
| }) | |
| : nothing | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/ui/app-render.ts` around lines 709 - 734, The onSelectTeam prop
currently only sets state.teamsSelectedId; update the onSelectTeam handler
passed into renderTeams so it also clears the per-team chat state: reset
state.teamsChatMessages to an empty array, state.teamsChatRunId to
null/undefined, and state.teamsChatMessage to an empty string whenever selecting
a new team (in the same block where state.teamsSelectedId is assigned) to avoid
showing stale chat from the previous team.
Comment on lines
+132
to
+140
| agentsPanel: | ||
| | "overview" | ||
| | "files" | ||
| | "tools" | ||
| | "skills" | ||
| | "channels" | ||
| | "cron" | ||
| | "activity" | ||
| | "soul"; |
There was a problem hiding this comment.
SettingsHost.agentsPanel in app-settings.ts not updated to include "activity" | "soul".
AppViewState.agentsPanel now includes "activity" and "soul", but SettingsHost at line 54 of app-settings.ts still uses the old union. TypeScript will flag any future host.agentsPanel === "activity" check inside refreshActiveTab as always-false, and assignment of these new values to a SettingsHost-typed reference would be a type error.
🛠️ Proposed fix in app-settings.ts line 54
- agentsPanel?: "overview" | "files" | "tools" | "skills" | "channels" | "cron";
+ agentsPanel?: "overview" | "files" | "tools" | "skills" | "channels" | "cron" | "activity" | "soul";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/ui/app-view-state.ts` around lines 132 - 140, Update the
SettingsHost.agentsPanel type in app-settings.ts to include the new "activity"
and "soul" variants so it matches AppViewState.agentsPanel; locate the
SettingsHost declaration and the refreshActiveTab logic (references:
SettingsHost.agentsPanel, AppViewState.agentsPanel, refreshActiveTab) and extend
the union type to include "activity" and "soul" to avoid the never/always-false
type issues and assignment errors.
Comment on lines
+4
to
+15
| export type TeamsState = { | ||
| client: GatewayBrowserClient | null; | ||
| connected: boolean; | ||
| teamsLoading: boolean; | ||
| teamsError: string | null; | ||
| teamsList: TeamsListResult | null; | ||
| teamsSelectedId: string | null; | ||
| teamsChatMessage: string; | ||
| teamsChatSending: boolean; | ||
| teamsChatMessages: Array<{ role: "user" | "assistant"; text: string; ts: number }>; | ||
| teamsChatRunId: string | null; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'teamsChatMessages' --type=ts -A 2 -B 2Repository: SpotDraft/openclaw
Length of output: 2701
🏁 Script executed:
# Also check for the onSelectTeam handler
rg -n 'onSelectTeam' --type=ts -A 5Repository: SpotDraft/openclaw
Length of output: 1378
🏁 Script executed:
# Search for any team switch related logic that might clear chat
rg -n 'teamsSelectedId' --type=ts -A 2 -B 2Repository: SpotDraft/openclaw
Length of output: 2347
Chat messages are scoped globally, not per-team — switching teams leaks chat history.
teamsChatMessages and teamsChatRunId are defined at the app state level and shared across all teams. When the user selects a different team (see onSelectTeam in app-render.ts lines 722–724, which only sets teamsSelectedId), the previous team's chat messages remain visible. Either scope the chat state per team (e.g., Record<string, ChatMessage[]>) or clear it on team switch.
Option A: clear chat on team switch (in app-render.ts onSelectTeam)
onSelectTeam: (teamId) => {
+ state.teamsChatMessages = [];
+ state.teamsChatRunId = null;
+ state.teamsChatMessage = "";
state.teamsSelectedId = teamId;
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/ui/controllers/teams.ts` around lines 4 - 15, The team chat state
(TeamsState: teamsChatMessages and teamsChatRunId) is shared across all teams so
switching teams leaks chat history; update the onSelectTeam handler (where it
currently only sets teamsSelectedId) to also reset teamsChatMessages to an empty
array and teamsChatRunId to null so each team shows a fresh chat, or
alternatively refactor TeamsState to scope chat per team (e.g., change
teamsChatMessages and teamsChatRunId to maps keyed by team id like
Record<string, ChatMessage[]> and Record<string, string | null> and update all
usages to read/write by teamsSelectedId) — pick one approach and apply the
corresponding changes to onSelectTeam and any components that read/write
teamsChatMessages/teamsChatRunId.
Comment on lines
+211
to
+237
| function renderTeamStatus(team: NonNullable<TeamsListResult["teams"][number]>) { | ||
| return html` | ||
| <div class="card" style="margin-top: 16px;"> | ||
| <div class="card-title" style="margin-bottom: 8px;">Team Status</div> | ||
| <div class="card-sub" style="margin-bottom: 12px;"> | ||
| Active runs across team members. Refresh the Teams tab to update. | ||
| </div> | ||
| <div class="agent-row" style="cursor: default;"> | ||
| <div class="agent-info"> | ||
| <div class="agent-title">${team.lead.name || team.lead.id}</div> | ||
| <div class="agent-sub mono">${team.lead.id}</div> | ||
| </div> | ||
| <span class="agent-pill">lead</span> | ||
| </div> | ||
| ${team.members.map( | ||
| (member) => html` | ||
| <div class="agent-row" style="cursor: default;"> | ||
| <div class="agent-info"> | ||
| <div class="agent-title">${member.name || member.id}</div> | ||
| <div class="agent-sub mono">${member.id}</div> | ||
| </div> | ||
| </div> | ||
| `, | ||
| )} | ||
| </div> | ||
| `; | ||
| } |
There was a problem hiding this comment.
Status tab is structurally hollow — run counts are absent.
renderTeamStatus only receives a TeamsListResult["teams"][number] (static config data) and renders member names. The teams.status gateway RPC returns { leadActiveRuns, totalActiveRuns, members[].activeRuns } — none of which flows here. The Status tab needs to either fetch teams.status and pass a TeamsStatusResult prop, or the status data needs to be included in TeamsProps. Without this, the tab is identical to a subset of the Overview.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/ui/views/teams.ts` around lines 211 - 237, renderTeamStatus currently
only renders static team config (team: TeamsListResult["teams"][number"]) so
active run counts from the teams.status RPC never appear; change the data flow
so renderTeamStatus consumes status data (e.g., a TeamsStatusResult or a
combined TeamWithStatus type) and update the parent that calls renderTeamStatus
to call the teams.status gateway RPC and pass the returned {leadActiveRuns,
totalActiveRuns, members[].activeRuns} into renderTeamStatus. Inside
renderTeamStatus (the function named renderTeamStatus and the JSX that maps
team.members) render the summary (totalActiveRuns and leadActiveRuns) in the
card header and show each member's activeRuns next to their name (use
member.activeRuns), falling back to 0 if missing. Ensure the prop/type
signatures (TeamsProps or the renderTeamStatus parameter) are updated to include
the status shape so the view is populated with run counts.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
ui/src/ui/app-render.helpers.ts (3)
127-151:switchToTeamandswitchToAgentshare ~90% identical reset logic — consider extracting a common helper.Both functions reset
chatMessage,chatStream,chatStreamStartedAt,chatRunId, callresetToolStream,resetChatScroll,applySettings,loadAssistantIdentity,syncUrlWithSessionKey,loadChatHistory, andrefreshChatAvatar. The same pattern also appears inresetChatStateForSessionSwitch(Lines 35-48). Extracting a shared session-switch helper would reduce duplication and ensure future state resets are applied consistently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/ui/app-render.helpers.ts` around lines 127 - 151, Extract the duplicated reset logic from switchToTeam, switchToAgent and resetChatStateForSessionSwitch into a single helper (e.g., resetSessionStateForSwitch) that accepts the AppViewState and nextSessionKey (and optionally team/agent metadata), and moves the shared operations—clearing state.chatMessage, state.chatStream, (OpenClawApp).chatStreamStartedAt, state.chatRunId, calling (OpenClawApp).resetToolStream(), (OpenClawApp).resetChatScroll(), state.applySettings({... , sessionKey: nextSessionKey, lastActiveSessionKey: nextSessionKey}), void state.loadAssistantIdentity(), syncUrlWithSessionKey(..., nextSessionKey, true), void loadChatHistory(state as ChatState), and void refreshChatAvatar(...)—into that helper; then call this helper from switchToTeam, switchToAgent and resetChatStateForSessionSwitch, keeping only the minimal per-function specifics (like computing leadAgentId/nextSessionKey or setting state.chatTeamId/state.chatAgentId) in those functions.
103-125:switchToAgentdoes not clearchatTeamId— relies on caller to do it.
switchToAgentresets various chat state fields but never setsstate.chatTeamId = null. The current call site at Line 190-191 clears it beforehand, but ifswitchToAgentis ever called from another context, messages will continue routing toteams.run.Move
state.chatTeamId = nullintoswitchToAgentfor safety.♻️ Proposed fix
function switchToAgent(state: AppViewState, agentId: string) { + state.chatTeamId = null; const nextSessionKey = buildAgentSessionKey(agentId); state.sessionKey = nextSessionKey;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/ui/app-render.helpers.ts` around lines 103 - 125, switchToAgent currently resets many chat fields but doesn't clear state.chatTeamId, relying on callers to do so; update the switchToAgent function to set state.chatTeamId = null (e.g., alongside state.sessionKey/state.chatMessage resets) so team routing is always cleared when switching agents. Make the change inside switchToAgent (the function that calls buildAgentSessionKey, resets chatStream/chatRunId, calls resetToolStream/resetChatScroll, applySettings, loadAssistantIdentity, syncUrlWithSessionKey, loadChatHistory, refreshChatAvatar) to ensure any caller gets a clean team state.
7-7: Cross-boundary imports from UI into server-sidesrc/affect multiple files.
parseAgentSessionKeyis imported from../../../src/sessions/session-key-utils.jsin at least three UI files (app-render.helpers.ts,app-chat.ts,app-render.ts), creating coupling between the UI build and server internals. This pattern breaks independent package deployment and creates fragile dependency chains.Consider extracting session-key utilities into a shared package that both UI and server depend on, or if the functions are lightweight enough (they are—pure string parsing logic), duplicate them in the UI layer to eliminate the coupling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/ui/app-render.helpers.ts` at line 7, The UI imports parseAgentSessionKey from server-side code, causing cross-boundary coupling; remove the import from app-render.helpers.ts (and the other UI files app-chat.ts and app-render.ts), then either (A) extract the session-key parsing logic into a new shared utility package that both UI and server depend on and update these modules to import parseAgentSessionKey from that shared package, or (B) copy the lightweight parseAgentSessionKey implementation directly into the UI codebase (e.g., a new ui/session-key-utils.ts) and update imports to use the local function; ensure you update all references to parseAgentSessionKey, run builds/tests, and keep the function signature/behavior identical to the original.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/gateway/server-methods/teams.ts`:
- Around line 170-182: teams.status incorrectly relies on
countActiveRunsForSession (which only tracks subagent spawns) while teams.run
calls agentCommand directly; to fix, update teams.run to register the team run
with the subagent registry by calling registerSubagentRun (using the same
session key format like `agent:${normalizeAgentId(...)}` or the key expected by
countActiveRunsForSession) when a team run starts and ensure it deregisters on
completion/failure so that countActiveRunsForSession used in teams.status
reports the correct activeRuns; reference teams.run, teams.status,
countActiveRunsForSession, registerSubagentRun, agentCommand, and sessions_spawn
when making the change.
In `@ui/src/ui/controllers/chat.ts`:
- Around line 127-143: The teams.run branch omits attachments so messages shown
with attachments are never sent; update the teams.run request in the send flow
to include the same attachments payload as chat.send (add attachments:
apiAttachments and preserve idempotencyKey/sessionKey/deliver), i.e. modify the
payload passed to state.client.request("teams.run", { ... }) to include
apiAttachments under the attachments key (or the backend-expected field name),
and if teams.run does not support attachments adjust the UI to hide/disable the
attachment UI when state.chatTeamId is set.
---
Duplicate comments:
In `@src/gateway/server-methods/teams.ts`:
- Around line 120-143: The respond calls inside the Promise chain can themselves
throw and escape into an unhandled rejection; wrap each respond invocation in a
try/catch (or replace .then(...).catch(...) with .then(onFulfilled, onRejected))
so any exception from respond is caught and handled. Specifically, in the
success branch where respond(true, payload, ...) is called and in the error
branch where respond(false, payload, error, ...) is called (references: respond,
errorShape, formatForLog, runId, team.id, leadAgentId), catch exceptions from
respond and convert them into a safe error response (e.g., call respond(false, {
runId, teamId: team.id, leadAgentId, status: "error", summary:
String(errFromRespond) }, errorShape(ErrorCodes.UNAVAILABLE,
String(errFromRespond)), { runId, error: formatForLog(errFromRespond) })) so no
thrown error escapes the Promise chain.
In `@ui/src/ui/app-render.helpers.ts`:
- Around line 103-104: switchToAgent currently calls
buildAgentSessionKey(agentId) without the mainKey, which pins agent sessions to
"main"; update switchToAgent to compute the current main session key by calling
resolveSidebarChatSessionKey(state) and pass that value as the second argument
to buildAgentSessionKey(agentId, mainKey). Ensure you use the resolved key from
resolveSidebarChatSessionKey(state) when assigning nextSessionKey so agent
sessions are scoped properly.
In `@ui/src/ui/app-view-state.ts`:
- Around line 133-141: The SettingsHost.agentsPanel type in app-settings.ts must
be updated to match the union in ui/src/ui/app-view-state.ts (which now includes
"activity" and "soul"); open the SettingsHost declaration (SettingsHost or
agentsPanel property) and add "activity" and "soul" to its union/allowed values
so both files use the same agentsPanel type, then run TypeScript checks to
ensure no other type mismatches remain.
---
Nitpick comments:
In `@ui/src/ui/app-render.helpers.ts`:
- Around line 127-151: Extract the duplicated reset logic from switchToTeam,
switchToAgent and resetChatStateForSessionSwitch into a single helper (e.g.,
resetSessionStateForSwitch) that accepts the AppViewState and nextSessionKey
(and optionally team/agent metadata), and moves the shared operations—clearing
state.chatMessage, state.chatStream, (OpenClawApp).chatStreamStartedAt,
state.chatRunId, calling (OpenClawApp).resetToolStream(),
(OpenClawApp).resetChatScroll(), state.applySettings({... , sessionKey:
nextSessionKey, lastActiveSessionKey: nextSessionKey}), void
state.loadAssistantIdentity(), syncUrlWithSessionKey(..., nextSessionKey, true),
void loadChatHistory(state as ChatState), and void refreshChatAvatar(...)—into
that helper; then call this helper from switchToTeam, switchToAgent and
resetChatStateForSessionSwitch, keeping only the minimal per-function specifics
(like computing leadAgentId/nextSessionKey or setting
state.chatTeamId/state.chatAgentId) in those functions.
- Around line 103-125: switchToAgent currently resets many chat fields but
doesn't clear state.chatTeamId, relying on callers to do so; update the
switchToAgent function to set state.chatTeamId = null (e.g., alongside
state.sessionKey/state.chatMessage resets) so team routing is always cleared
when switching agents. Make the change inside switchToAgent (the function that
calls buildAgentSessionKey, resets chatStream/chatRunId, calls
resetToolStream/resetChatScroll, applySettings, loadAssistantIdentity,
syncUrlWithSessionKey, loadChatHistory, refreshChatAvatar) to ensure any caller
gets a clean team state.
- Line 7: The UI imports parseAgentSessionKey from server-side code, causing
cross-boundary coupling; remove the import from app-render.helpers.ts (and the
other UI files app-chat.ts and app-render.ts), then either (A) extract the
session-key parsing logic into a new shared utility package that both UI and
server depend on and update these modules to import parseAgentSessionKey from
that shared package, or (B) copy the lightweight parseAgentSessionKey
implementation directly into the UI codebase (e.g., a new
ui/session-key-utils.ts) and update imports to use the local function; ensure
you update all references to parseAgentSessionKey, run builds/tests, and keep
the function signature/behavior identical to the original.
Comment on lines
+170
to
+182
| const leadAgentId = normalizeAgentId(team.lead); | ||
| const leadKey = `agent:${leadAgentId}`; | ||
| const leadActive = countActiveRunsForSession(leadKey); | ||
| let totalActive = leadActive; | ||
|
|
||
| const members: Array<{ agentId: string; activeRuns: number }> = []; | ||
| for (const memberId of team.members) { | ||
| const id = normalizeAgentId(memberId); | ||
| const memberKey = `agent:${id}`; | ||
| const active = countActiveRunsForSession(memberKey); | ||
| totalActive += active; | ||
| members.push({ agentId: id, activeRuns: active }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the countActiveRunsForSession function
echo "=== Searching for countActiveRunsForSession implementation ==="
rg -n "countActiveRunsForSession" --type=ts -B2 -A10
echo ""
echo "=== Searching for requesterSessionKey assignments/matches ==="
rg -n "requesterSessionKey" --type=ts -B2 -A2
echo ""
echo "=== Check teams.run session key format ==="
rg -n "sessionKey.*team" --type=ts -B2 -A2Repository: SpotDraft/openclaw
Length of output: 50374
🏁 Script executed:
#!/bin/bash
# Find teams.run method (around line 78)
echo "=== teams.run method with sessionKey construction ==="
rg -n "teams.*run|run.*teams" --type=ts src/gateway/server-methods/teams.ts -B5 -A15 | head -60
echo ""
echo "=== Search for how agentCommand is called in teams context ==="
rg -n "agentCommand" src/gateway/server-methods/teams.ts -B3 -A3
echo ""
echo "=== Check how sessionKey is constructed for teams ==="
rg -n "sessionKey.*team|team.*sessionKey" --type=ts -B2 -A2 | head -40Repository: SpotDraft/openclaw
Length of output: 2047
🏁 Script executed:
#!/bin/bash
# Find the agentCommand invocation with sessionKey
echo "=== agentCommand call in teams.run with sessionKey ==="
sed -n '102,120p' src/gateway/server-methods/teams.ts
echo ""
echo "=== agentCommand function signature and how it handles sessionKey ==="
rg -n "export.*function agentCommand|export const agentCommand" --type=ts -A20 | head -50
echo ""
echo "=== How registerSubagentRun uses requesterSessionKey parameter ==="
rg -n "registerSubagentRun" src/commands/agent.js src/commands/agent.ts -B5 -A5 | head -60Repository: SpotDraft/openclaw
Length of output: 4193
🏁 Script executed:
#!/bin/bash
# Find registerSubagentRun invocation in agentCommand
echo "=== registerSubagentRun calls in agent.ts ==="
rg -n "registerSubagentRun" src/commands/agent.ts -B5 -A5
echo ""
echo "=== How sessionKey is used when registering ==="
rg -n "requesterSessionKey.*sessionKey|sessionKey.*requesterSessionKey" --type=ts -B3 -A3Repository: SpotDraft/openclaw
Length of output: 2183
🏁 Script executed:
#!/bin/bash
# Search for all registerSubagentRun callers
echo "=== All registerSubagentRun calls in codebase ==="
rg -n "registerSubagentRun\(" --type=ts | head -20
echo ""
echo "=== Search in isolated-agent/run.ts context ==="
rg -n "registerSubagentRun" src/cron/isolated-agent/run.ts -B3 -A3 | head -30
echo ""
echo "=== Check if teams runs are treated as subagent runs ==="
rg -n "team.*subagent|subagent.*team" --type=ts src/ -i | head -20Repository: SpotDraft/openclaw
Length of output: 1317
🏁 Script executed:
#!/bin/bash
# Check sessions-spawn-tool registerSubagentRun usage
echo "=== sessions-spawn-tool registerSubagentRun context ==="
sed -n '310,330p' src/agents/tools/sessions-spawn-tool.ts
echo ""
echo "=== Full context around registerSubagentRun in sessions-spawn-tool ==="
rg -n "registerSubagentRun" src/agents/tools/sessions-spawn-tool.ts -B10 -A10
echo ""
echo "=== What does countActiveRunsForSession measure? ==="
rg -n "countActiveRunsForSession" --type=ts | grep -v test | grep -v "\.ts-"Repository: SpotDraft/openclaw
Length of output: 3474
🏁 Script executed:
#!/bin/bash
# Check what type of run teams.run creates
echo "=== teams.run full implementation ==="
sed -n '39,130p' src/gateway/server-methods/teams.ts
echo ""
echo "=== Check if agentCommand is for subagents or main agents ==="
rg -n "agentCommand.*opts" src/commands/agent.ts -A30 | head -50Repository: SpotDraft/openclaw
Length of output: 2773
🏁 Script executed:
#!/bin/bash
# Check if agentCommand eventually calls registerSubagentRun
echo "=== agentCommand implementation to see if it calls registerSubagentRun ==="
sed -n '66,150p' src/commands/agent.ts
echo ""
echo "=== Check if there's a different way teams.run tracks active runs ==="
rg -n "teams.*active|active.*teams" --type=ts src/ -i | grep -v test | head -20Repository: SpotDraft/openclaw
Length of output: 3381
🏁 Script executed:
#!/bin/bash
# Check how agentCommand handles the subagent lane
echo "=== Full agentCommand to see subagent handling ==="
rg -n "isSubagentLane|AGENT_LANE_SUBAGENT" src/commands/agent.ts -B3 -A3
echo ""
echo "=== Check registerAgentRunContext to see if it's related to subagent registry ==="
rg -n "registerAgentRunContext" --type=ts -B2 -A2 | head -40
echo ""
echo "=== Final verification: Does teams.run pass lane parameter ==="
grep -n "lane\|AGENT_LANE" src/gateway/server-methods/teams.tsRepository: SpotDraft/openclaw
Length of output: 3407
🏁 Script executed:
#!/bin/bash
# Confirm the purpose of countActiveRunsForSession
echo "=== countActiveRunsForSession function definition and comments ==="
sed -n '661,680p' src/agents/subagent-registry.ts
echo ""
echo "=== Check if teams.run is supposed to track via subagent registry or another way ==="
rg -n "class.*Gateway|constructor" src/gateway/server-methods/teams.ts -B2 -A5 | head -20Repository: SpotDraft/openclaw
Length of output: 732
teams.status incorrectly uses countActiveRunsForSession, which only tracks subagent spawns, not team runs.
teams.run doesn't register runs with the subagent registry; it calls agentCommand directly without invoking registerSubagentRun. countActiveRunsForSession is designed to track active subagent children spawned via sessions_spawn, not direct agent command executions. Therefore, teams.status will always report 0 active runs regardless of actual activity, since there are no registry entries for team runs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gateway/server-methods/teams.ts` around lines 170 - 182, teams.status
incorrectly relies on countActiveRunsForSession (which only tracks subagent
spawns) while teams.run calls agentCommand directly; to fix, update teams.run to
register the team run with the subagent registry by calling registerSubagentRun
(using the same session key format like `agent:${normalizeAgentId(...)}` or the
key expected by countActiveRunsForSession) when a team run starts and ensure it
deregisters on completion/failure so that countActiveRunsForSession used in
teams.status reports the correct activeRuns; reference teams.run, teams.status,
countActiveRunsForSession, registerSubagentRun, agentCommand, and sessions_spawn
when making the change.
Comment on lines
+127
to
+143
| if (state.chatTeamId) { | ||
| await state.client.request("teams.run", { | ||
| teamId: state.chatTeamId, | ||
| message: msg, | ||
| sessionKey: state.sessionKey, | ||
| idempotencyKey: runId, | ||
| deliver: false, | ||
| }); | ||
| } else { | ||
| await state.client.request("chat.send", { | ||
| sessionKey: state.sessionKey, | ||
| message: msg, | ||
| deliver: false, | ||
| idempotencyKey: runId, | ||
| attachments: apiAttachments, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Attachments are silently dropped when sending to a team.
When chatTeamId is set, the teams.run call (Lines 128-134) omits apiAttachments. However, the user's message is still rendered with attachment content blocks (Lines 84-91, 93-100), so the UI shows attachments as "sent" even though the backend never receives them.
Either forward attachments to teams.run, or disable/hide the attachment UI when a team is selected, to avoid confusing users.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/ui/controllers/chat.ts` around lines 127 - 143, The teams.run branch
omits attachments so messages shown with attachments are never sent; update the
teams.run request in the send flow to include the same attachments payload as
chat.send (add attachments: apiAttachments and preserve
idempotencyKey/sessionKey/deliver), i.e. modify the payload passed to
state.client.request("teams.run", { ... }) to include apiAttachments under the
attachments key (or the backend-expected field name), and if teams.run does not
support attachments adjust the UI to hide/disable the attachment UI when
state.chatTeamId is set.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/agents/tools/session-status-tool.ts (1)
311-316:⚠️ Potential issue | 🟡 MinorConfirm sessionId scoping is intentional but worth clarifying for team-lead users.
The asymmetry is real: when
a2aPolicy.enabledisfalse,resolveSessionKeyFromSessionIdat line 315 restricts torequesterAgentId, which prevents team leads from finding members' sessions via sessionId. Meanwhile, explicitagent:keys can bypass this restriction via the team-lead check inisAllowed(lines 125–128).This design is intentional—the requester-scoped store resolution (line 303) is meant to avoid leaking default agent data. However, it does create an asymmetry where team leads can access members via
agent:member-idbut not viasessionIdwhen A2A is disabled. Consider documenting this distinction or adding a clarifying comment about why sessionId resolution is scoped differently than explicit key resolution.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agents/tools/session-status-tool.ts` around lines 311 - 316, Add a brief clarifying comment above the call to resolveSessionKeyFromSessionId that explains the intentional asymmetry: when a2aPolicy.enabled is false we scope sessionId resolution to requesterAgentId to avoid leaking default agent data, which means team-leads can still access member sessions via explicit agent: keys (checked in isAllowed) but cannot resolve by sessionId unless A2A is enabled; reference resolveSessionKeyFromSessionId, a2aPolicy.enabled, requesterAgentId and isAllowed in the comment so reviewers understand this design decision.
🧹 Nitpick comments (2)
src/agents/tools/sessions-spawn-tool.ts (1)
362-369: Consider propagating the sub-agent's error status in the wait response.When
agent.waitreturnsstatus: "error", the code still returnsstatus: "completed"(line 381) to the caller. This could mask sub-agent failures. Consider returning a distinct status (e.g.,"error") with the error details fromwait.error, while still includingresultTextwhen available.Also applies to: 380-388
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agents/tools/sessions-spawn-tool.ts` around lines 362 - 369, The current spawn session flow checks wait?.status for "timeout" but treats all other non-timeout outcomes as "completed", which hides sub-agent failures; update the logic in the sessions-spawn handler (the block around the wait?.status check in sessions-spawn-tool.ts where jsonResult is returned) to detect when wait?.status === "error" and return jsonResult with status: "error", including wait.error (and wait.resultText if present) alongside childSessionKey and runId (childRunId), instead of returning status: "completed"; keep the existing "timeout" handling intact.src/agents/tools/sessions-helpers.ts (1)
3-4: Merge duplicate imports from the same module.Lines 3 and 4 both import from
../../routing/session-key.js. Consider consolidating into a single import statement.♻️ Proposed fix
-import { normalizeAgentId } from "../../routing/session-key.js"; -import { isAcpSessionKey, normalizeMainKey } from "../../routing/session-key.js"; +import { isAcpSessionKey, normalizeAgentId, normalizeMainKey } from "../../routing/session-key.js";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agents/tools/sessions-helpers.ts` around lines 3 - 4, The file imports normalizeAgentId and isAcpSessionKey/normalizeMainKey from the same module twice; consolidate them into one import from "../../routing/session-key.js" that includes normalizeAgentId, isAcpSessionKey, and normalizeMainKey so there are no duplicate import statements (look for the existing imports of normalizeAgentId and isAcpSessionKey/normalizeMainKey and combine them into a single import).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/agents/tools/sessions-helpers.ts`:
- Around line 83-99: The team lead field can be an empty string which
normalizeAgentId("") maps to DEFAULT_AGENT_ID and unintentionally grants the
default agent access; update the teams schema (the zod schema that defines
team.lead) to require a non-empty lead (e.g., add .min(1) or otherwise reject
empty strings) or add an explicit guard when iterating cfg.teams to skip teams
with falsy/empty team.lead before calling normalizeAgentId; ensure the logic in
the block building teamLeadMembers (references: teamLeadMembers, cfg.teams,
normalizeAgentId, team.lead) only processes valid leads. Also consolidate the
two separate imports from the same module into a single import statement for
cleanliness.
In `@src/agents/tools/sessions-spawn-tool.ts`:
- Around line 370-398: The current flow calls markSubagentRunCleanupHandled
before awaiting readLatestAssistantReply, which can suppress the announce path
and lose the sub-agent output if readLatestAssistantReply throws; move the
markSubagentRunCleanupHandled(childRunId) call to immediately after successfully
obtaining resultText (i.e., after await readLatestAssistantReply({ sessionKey:
childSessionKey })) so cleanup is only suppressed once the reply is secured,
leaving the catch branch to return "accepted" and still allow announce to run
when readLatestAssistantReply fails; update references in this block that set
jsonResult (status: "completed"/"accepted") and keep modelApplied/modelWarning
handling unchanged.
---
Outside diff comments:
In `@src/agents/tools/session-status-tool.ts`:
- Around line 311-316: Add a brief clarifying comment above the call to
resolveSessionKeyFromSessionId that explains the intentional asymmetry: when
a2aPolicy.enabled is false we scope sessionId resolution to requesterAgentId to
avoid leaking default agent data, which means team-leads can still access member
sessions via explicit agent: keys (checked in isAllowed) but cannot resolve by
sessionId unless A2A is enabled; reference resolveSessionKeyFromSessionId,
a2aPolicy.enabled, requesterAgentId and isAllowed in the comment so reviewers
understand this design decision.
---
Nitpick comments:
In `@src/agents/tools/sessions-helpers.ts`:
- Around line 3-4: The file imports normalizeAgentId and
isAcpSessionKey/normalizeMainKey from the same module twice; consolidate them
into one import from "../../routing/session-key.js" that includes
normalizeAgentId, isAcpSessionKey, and normalizeMainKey so there are no
duplicate import statements (look for the existing imports of normalizeAgentId
and isAcpSessionKey/normalizeMainKey and combine them into a single import).
In `@src/agents/tools/sessions-spawn-tool.ts`:
- Around line 362-369: The current spawn session flow checks wait?.status for
"timeout" but treats all other non-timeout outcomes as "completed", which hides
sub-agent failures; update the logic in the sessions-spawn handler (the block
around the wait?.status check in sessions-spawn-tool.ts where jsonResult is
returned) to detect when wait?.status === "error" and return jsonResult with
status: "error", including wait.error (and wait.resultText if present) alongside
childSessionKey and runId (childRunId), instead of returning status:
"completed"; keep the existing "timeout" handling intact.
Comment on lines
+83
to
+99
|
|
||
| // Build a set of team-lead -> members relationships so leads can always | ||
| // access their own team members without requiring the global a2a toggle. | ||
| const teamLeadMembers = new Map<string, Set<string>>(); | ||
| if (Array.isArray(cfg.teams)) { | ||
| for (const team of cfg.teams) { | ||
| const leadId = normalizeAgentId(team.lead); | ||
| let members = teamLeadMembers.get(leadId); | ||
| if (!members) { | ||
| members = new Set(); | ||
| teamLeadMembers.set(leadId, members); | ||
| } | ||
| for (const memberId of team.members) { | ||
| members.add(normalizeAgentId(memberId)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the type definition for cfg.teams
rg -n "cfg\s*:" --type=ts src/agents/tools/ -A5 -B5
# Look for interface/type definitions related to teams
rg -n "interface|type.*Team" --type=ts src/ --glob='!node_modules/**' | head -30
# Check the config type definition
rg -n "teams\s*[\?:]" --type=ts src/ --glob='!node_modules/**' | head -20Repository: SpotDraft/openclaw
Length of output: 40848
🏁 Script executed:
# Look at the top of sessions-helpers.ts to check imports
head -20 src/agents/tools/sessions-helpers.tsRepository: SpotDraft/openclaw
Length of output: 739
🏁 Script executed:
# Find normalizeAgentId implementation
rg -n "normalizeAgentId" --type=ts src/ --glob='!node_modules/**' -A10 | head -40Repository: SpotDraft/openclaw
Length of output: 3002
🏁 Script executed:
# Check the DEFAULT_AGENT_ID usage
rg -n "DEFAULT_AGENT_ID" --type=ts src/ --glob='!node_modules/**'Repository: SpotDraft/openclaw
Length of output: 5409
🏁 Script executed:
# Find TeamConfig type definition
rg -n "type TeamConfig|interface TeamConfig" --type=ts src/ --glob='!node_modules/**' -A10
# Check the types.teams file directly
find src -name "types.teams.ts" -o -name "*teams*type*" | head -5Repository: SpotDraft/openclaw
Length of output: 548
🏁 Script executed:
# Check if types.teams.js or types.teams.ts exists
ls -la src/config/types.teams.* 2>/dev/null || echo "File not found in types"
# Try to find TeamConfig in all config files
rg -n "TeamConfig\|lead.*:\|members.*:" --type=ts src/config/ | head -30Repository: SpotDraft/openclaw
Length of output: 135
🏁 Script executed:
# Check the zod schema for teams to see if validation enforces these fields
rg -n "TeamsSchema\|teams.*z\." --type=ts src/config/ -A15 | head -50Repository: SpotDraft/openclaw
Length of output: 44
🏁 Script executed:
# Check how cfg.teams is parsed/validated
rg -n "teams" --type=ts src/config/zod-schema.ts -B2 -A2Repository: SpotDraft/openclaw
Length of output: 1149
🏁 Script executed:
# Find and check the TeamsSchema definition
rg -n "TeamsSchema\|export.*const.*Teams" --type=ts src/config/zod-schema.teams.ts -A20Repository: SpotDraft/openclaw
Length of output: 44
🏁 Script executed:
# Also check the actual validation of team.members to understand if it could be undefined
cat src/config/zod-schema.teams.tsRepository: SpotDraft/openclaw
Length of output: 364
🏁 Script executed:
# Check if there are tests for the createAgentToAgentPolicy function with edge cases
rg -n "createAgentToAgentPolicy\|teamLeadMembers" --type=ts src/agents/tools/ -B3 -A10 | head -80
# Check how this function is used to understand if cfg is guaranteed to be validated
rg -n "createAgentToAgentPolicy" --type=ts src/ --glob='!node_modules/**' -B2 -A2Repository: SpotDraft/openclaw
Length of output: 3719
🏁 Script executed:
# Look at the sessions-helpers.ts lines around 125-129 mentioned in the scratchpad to see how teamLeadMembers is used
sed -n '120,135p' src/agents/tools/sessions-helpers.tsRepository: SpotDraft/openclaw
Length of output: 568
🏁 Script executed:
# Verify that cfg parameter is always validated before calling createAgentToAgentPolicy
rg -n "createAgentToAgentPolicy\|loadConfig" --type=ts src/agents/tools/sessions-send-tool.ts -B5 -A3 | head -40Repository: SpotDraft/openclaw
Length of output: 44
🏁 Script executed:
# Check if there's any code path where an unvalidated cfg could be passed
rg -n "OpenClawConfig" --type=ts src/agents/tools/ | grep -E "param|arg|function" | head -10Repository: SpotDraft/openclaw
Length of output: 1119
🏁 Script executed:
# Double-check if the zod schema has any empty string validation
cat src/config/zod-schema.teams.ts | grep -A5 "lead:"Repository: SpotDraft/openclaw
Length of output: 176
🏁 Script executed:
# Check if there's any upstream processing that would prevent empty strings
rg -n "team.lead|lead.*trim|lead.*empty" --type=ts src/config/ -B2 -A2Repository: SpotDraft/openclaw
Length of output: 946
🏁 Script executed:
# Verify what happens with empty string in normalizeAgentId
sed -n '75,115p' src/routing/session-key.tsRepository: SpotDraft/openclaw
Length of output: 1171
Guard against empty team.lead values bypassing validation.
The zod schema allows empty strings for team.lead without a minimum length constraint. When team.lead is an empty string, the validation at line 649 treats it as falsy and skips the check. Since normalizeAgentId("") returns DEFAULT_AGENT_ID ("main"), an empty team.lead would silently grant the default agent access to all members of that team—likely unintended. Consider adding .min(1) to the lead field schema, or validate/reject empty team entries during config parsing.
Also, lines 3–4 import from the same module; combine them into a single import statement for cleanliness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agents/tools/sessions-helpers.ts` around lines 83 - 99, The team lead
field can be an empty string which normalizeAgentId("") maps to DEFAULT_AGENT_ID
and unintentionally grants the default agent access; update the teams schema
(the zod schema that defines team.lead) to require a non-empty lead (e.g., add
.min(1) or otherwise reject empty strings) or add an explicit guard when
iterating cfg.teams to skip teams with falsy/empty team.lead before calling
normalizeAgentId; ensure the logic in the block building teamLeadMembers
(references: teamLeadMembers, cfg.teams, normalizeAgentId, team.lead) only
processes valid leads. Also consolidate the two separate imports from the same
module into a single import statement for cleanliness.
Comment on lines
+370
to
+398
|
|
||
| // Suppress the announce flow — the lead already has the result inline. | ||
| markSubagentRunCleanupHandled(childRunId); | ||
|
|
||
| const resultText = await readLatestAssistantReply({ sessionKey: childSessionKey }); | ||
| const runtimeMs = | ||
| typeof wait?.startedAt === "number" && typeof wait?.endedAt === "number" | ||
| ? Math.max(0, wait.endedAt - wait.startedAt) | ||
| : undefined; | ||
|
|
||
| return jsonResult({ | ||
| status: "completed", | ||
| result: resultText ?? "(no output)", | ||
| childSessionKey, | ||
| runId: childRunId, | ||
| runtimeMs, | ||
| modelApplied: resolvedModel ? modelApplied : undefined, | ||
| warning: modelWarning, | ||
| }); | ||
| } catch { | ||
| // On RPC failure, fall back to accepted — announce will still fire. | ||
| return jsonResult({ | ||
| status: "accepted", | ||
| childSessionKey, | ||
| runId: childRunId, | ||
| modelApplied: resolvedModel ? modelApplied : undefined, | ||
| warning: modelWarning, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Bug: markSubagentRunCleanupHandled called before readLatestAssistantReply can silently lose the sub-agent result.
If readLatestAssistantReply (line 374) throws after markSubagentRunCleanupHandled (line 372) has already suppressed the announce flow, the catch block at line 389 returns "accepted" with the comment "announce will still fire" — but it won't, because cleanupHandled is already true. The sub-agent's output is lost through both channels (inline and announce).
Move the cleanup suppression after the reply is successfully read:
🐛 Proposed fix — defer cleanup suppression until reply is secured
- // Suppress the announce flow — the lead already has the result inline.
- markSubagentRunCleanupHandled(childRunId);
-
const resultText = await readLatestAssistantReply({ sessionKey: childSessionKey });
const runtimeMs =
typeof wait?.startedAt === "number" && typeof wait?.endedAt === "number"
? Math.max(0, wait.endedAt - wait.startedAt)
: undefined;
+ // Suppress the announce flow — the lead already has the result inline.
+ markSubagentRunCleanupHandled(childRunId);
+
return jsonResult({
status: "completed",
result: resultText ?? "(no output)",📝 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
| // Suppress the announce flow — the lead already has the result inline. | |
| markSubagentRunCleanupHandled(childRunId); | |
| const resultText = await readLatestAssistantReply({ sessionKey: childSessionKey }); | |
| const runtimeMs = | |
| typeof wait?.startedAt === "number" && typeof wait?.endedAt === "number" | |
| ? Math.max(0, wait.endedAt - wait.startedAt) | |
| : undefined; | |
| return jsonResult({ | |
| status: "completed", | |
| result: resultText ?? "(no output)", | |
| childSessionKey, | |
| runId: childRunId, | |
| runtimeMs, | |
| modelApplied: resolvedModel ? modelApplied : undefined, | |
| warning: modelWarning, | |
| }); | |
| } catch { | |
| // On RPC failure, fall back to accepted — announce will still fire. | |
| return jsonResult({ | |
| status: "accepted", | |
| childSessionKey, | |
| runId: childRunId, | |
| modelApplied: resolvedModel ? modelApplied : undefined, | |
| warning: modelWarning, | |
| }); | |
| } | |
| const resultText = await readLatestAssistantReply({ sessionKey: childSessionKey }); | |
| const runtimeMs = | |
| typeof wait?.startedAt === "number" && typeof wait?.endedAt === "number" | |
| ? Math.max(0, wait.endedAt - wait.startedAt) | |
| : undefined; | |
| // Suppress the announce flow — the lead already has the result inline. | |
| markSubagentRunCleanupHandled(childRunId); | |
| return jsonResult({ | |
| status: "completed", | |
| result: resultText ?? "(no output)", | |
| childSessionKey, | |
| runId: childRunId, | |
| runtimeMs, | |
| modelApplied: resolvedModel ? modelApplied : undefined, | |
| warning: modelWarning, | |
| }); | |
| } catch { | |
| // On RPC failure, fall back to accepted — announce will still fire. | |
| return jsonResult({ | |
| status: "accepted", | |
| childSessionKey, | |
| runId: childRunId, | |
| modelApplied: resolvedModel ? modelApplied : undefined, | |
| warning: modelWarning, | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agents/tools/sessions-spawn-tool.ts` around lines 370 - 398, The current
flow calls markSubagentRunCleanupHandled before awaiting
readLatestAssistantReply, which can suppress the announce path and lose the
sub-agent output if readLatestAssistantReply throws; move the
markSubagentRunCleanupHandled(childRunId) call to immediately after successfully
obtaining resultText (i.e., after await readLatestAssistantReply({ sessionKey:
childSessionKey })) so cleanup is only suppressed once the reply is secured,
leaving the catch branch to return "accepted" and still allow announce to run
when readLatestAssistantReply fails; update references in this block that set
jsonResult (status: "completed"/"accepted") and keep modelApplied/modelWarning
handling unchanged.
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
Describe the problem and fix in 2–5 bullets:
Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
List user-visible changes (including defaults/config).
If none, write
None.Security Impact (required)
Yes/No)Yes/No)Yes/No)Yes/No)Yes/No)Yes, explain risk + mitigation:Repro + Verification
Environment
Steps
Expected
Actual
Evidence
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
Compatibility / Migration
Yes/No)Yes/No)Yes/No)Failure Recovery (if this breaks)
Risks and Mitigations
List only real risks for this PR. Add/remove entries as needed. If none, write
None.Summary by CodeRabbit
New Features
Documentation