feat(agent-manager): allow sessions to move their worktree between sections or ungroup - #12815
Conversation
…ctions or ungroup Add a new to the tool so a session can reassign its own worktree to another section or ungroup it by passing . The move operation validates the target section and the session's worktree, then pushes refreshed state to the panel. Also tighten the model-facing contract so the list-to-move workflow is unambiguous: is required first, its result is the source of truth for section and session IDs, and direct edits to are rejected by , , and with an explicit pointer to the tool. - New / in the Agent Manager protocol - New domain function with section/session validation - New OpenAPI hook so stays nullable in the generated SDK - Tool schema descriptions now spell out the list-first workflow and the no-direct-edit rule - List output now includes an instruction block and is pretty-printed - Optional nullable Task fields tolerate from models - Focused CLI tests, bridge unit test, and protection test
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| import * as path from "path" | ||
|
|
||
| export function assertMutablePath(filepath: string) { | ||
| const parts = filepath.split(path.sep) |
There was a problem hiding this comment.
WARNING: Splitting on path.sep without normalizing lets the guard be bypassed
edit.ts and write.ts pass params.filePath through verbatim when it is absolute (path.isAbsolute(params.filePath) ? params.filePath : path.join(...)), so assertMutablePath can receive an unnormalized path:
- On Windows,
path.sepis\, but a model commonly emits forward slashes.C:/proj/.kilo/agent-manager.jsonsplits into a single element,filebecomes the whole string, and the guard returns early — the write goes through. - On POSIX,
/proj/.kilo/./agent-manager.jsonyieldsdir === ".", and/proj/.kilo//agent-manager.jsonyieldsdir === "". Both bypass the check.
(apply_patch.ts is fine because it resolves the path first.) Normalizing fixes all three cases:
| const parts = filepath.split(path.sep) | |
| const parts = path.normalize(filepath).split(path.sep) |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| const parts = filepath.split(path.sep) | ||
| const file = parts.at(-1) | ||
| const dir = parts.at(-2) | ||
| if (file !== "agent-manager.json" || ![".kilo", ".kilocode"].includes(dir ?? "")) return |
There was a problem hiding this comment.
SUGGESTION: This guard is global and unconditional, with no escape hatch
Two things worth confirming are intended:
- The block applies to every Kilo surface, including the TUI and
kilo runwhere Agent Manager does not exist, and a user cannot opt out..kilo/agent-manager.jsonis documented as recoverable state (packages/kilo-docs/pages/automate/agent-manager.md), so an explicit request like "the JSON in.kilo/agent-manager.jsonis corrupted, fix it" now hard-fails with a message pointing ataction=move, which cannot repair malformed JSON. Consider routing this through a permission ask instead of an unconditional throw. edit/write/apply_patchare covered, butbashis not —echo ... > .kilo/agent-manager.jsonstill succeeds. Fine for a nudge-level guardrail, but it means this should not be treated as an actual invariant.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| const bus = yield* Bus.Service | ||
| const host = yield* AgentManager.Service | ||
| const provider = yield* Provider.Service | ||
| const wire = ToolJsonSchema.fromSchema(WireParams) |
There was a problem hiding this comment.
WARNING: This mutates the schema object cached inside ToolJsonSchema.fromSchema
fromSchema memoizes results in a module-level WeakMap<Schema.Top, JSONSchema7> and returns the same object reference on every call (packages/opencode/src/tool/json-schema.ts:6-21). Since WireParams is a module-level constant, assigning to wire.properties.sectionID writes into the shared cache entry, so any future fromSchema(WireParams) caller sees the patched schema.
It is harmless today (the patch is idempotent and nothing else reads WireParams), but it is fragile. Copying before patching keeps the cache clean:
const base = ToolJsonSchema.fromSchema(WireParams)
const wire = { ...base, properties: { ...base.properties, sectionID: { anyOf: [...], description: "..." } } }A short comment on why the patch is needed would also help: normalize strips the { type: "null" } arm from any property absent from required, which is exactly what makes Schema.optional(Schema.NullOr(...)) lose its null here. Without that note the block reads as arbitrary.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| sessionID: SessionID.annotate({ | ||
| description: "Session ID returned by action=list. Do not use a worktree name, branch, or section name.", | ||
| }), | ||
| sectionID: Schema.NullOr(Schema.String).annotate({ |
There was a problem hiding this comment.
SUGGESTION: sectionID bounds disagree with the wire schema and the protocol
Three places describe the same field differently:
- Here:
Schema.NullOr(Schema.String)— accepts""and arbitrarily long strings. - The hand-patched JSON Schema (line 288): advertises
minLength: 1. protocol.ts:MoveRequest.sectionIDisSchema.NullOr(ID)whereID = Schema.String.check(Schema.isMinLength(1), Schema.isMaxLength(200)).
So sectionID: "" passes tool validation but violates both the advertised schema and the protocol contract, surfacing only later. Mirroring the protocol bound keeps all three aligned and turns it into a clean parameter error:
| sectionID: Schema.NullOr(Schema.String).annotate({ | |
| sectionID: Schema.NullOr(Schema.String.check(Schema.isMinLength(1), Schema.isMaxLength(200))).annotate({ |
Separately: MoveParams.sectionID is required while WireParams.sectionID is Schema.optional(...), so the advertised schema lets a model call action: "move" with no sectionID and then fail the whole Params union with a confusing multi-arm error. Making it Schema.optional(Schema.NullOr(...)) (treating omitted as ungroup, same as null) would be more forgiving and consistent with the rest of this PR's null tolerance.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| ...result.overview, | ||
| }, | ||
| null, | ||
| 2, |
There was a problem hiding this comment.
SUGGESTION: Pretty-printing the overview meaningfully inflates tool output
The overview can hold up to 100 sections × 100 worktrees plus 100 local sessions (see the isMaxLength checks in protocol.ts). Switching from compact JSON.stringify to 2-space indentation roughly doubles the byte count for the same data, and tool output is auto-truncated — so on a large workspace this makes it more likely the tail (ungrouped, local.sessions) gets cut off, which is exactly the data action=move needs.
Since the instructions block already tells the model where the IDs live, dropping null, 2 keeps the payload compact without hurting readability; models parse minified JSON fine.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (20 files)
Verified as correct (click to expand)
Fix these issues in Kilo Cloud Reviewed by claude-opus-5 · Input: 80 · Output: 26.4K · Cached: 5.1M Review guidance: REVIEW.md from base branch |
…ctions or ungroup (Kilo-Org#12815) * feat(agent-manager): allow sessions to move their worktree between sections or ungroup Add a new to the tool so a session can reassign its own worktree to another section or ungroup it by passing . The move operation validates the target section and the session's worktree, then pushes refreshed state to the panel. Also tighten the model-facing contract so the list-to-move workflow is unambiguous: is required first, its result is the source of truth for section and session IDs, and direct edits to are rejected by , , and with an explicit pointer to the tool. - New / in the Agent Manager protocol - New domain function with section/session validation - New OpenAPI hook so stays nullable in the generated SDK - Tool schema descriptions now spell out the list-first workflow and the no-direct-edit rule - List output now includes an instruction block and is pretty-printed - Optional nullable Task fields tolerate from models - Focused CLI tests, bridge unit test, and protection test * chore: remove local PR screenshot * style(vscode): format agent manager orchestration * fix(agent-manager): protect state paths on Windows
2 participants
The Agent Manager tool now supports a
moveaction so a session can reassign its own worktree to another section or ungroup it by passingsectionID: null. The move operation validates the target section and the session's worktree, then pushes refreshed state to the panel.The model-facing contract is now explicit:
action: "list"is required first, its result is the source of truth for section and session IDs, and direct edits to.kilo/agent-manager.jsonare rejected byedit,write, andapply_patchwith an explicit pointer to theagent_managertool.Changes
MoveRequest/MoveResultin the Agent Manager protocolmovedomain function with section/session validationrestoreAgentManagerNullsOpenAPI hook sosectionIDstays nullable in the generated SDKnullfrom modelsSelf-test