mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-09-06 08:18:39 +00:00
fix: audit improvements for batch tool-call UI
- Make batchConsecutive() generic instead of ClineMessage-specific
- Add batch-aware button text for edit-file batches ("Save All"/"Deny All")
- Add dedicated list-batch/edit-batch i18n keys (stop reusing read-batch)
- Add JSON.parse defense-in-depth in all three synthesizers
- Fix mixed list_files batch icon to default to FolderTree
- Add 6 missing test cases (all-match, immutability, spy, single-dir)
This commit is contained in:
parent
3034b55f1d
commit
9ffc778491
6 changed files with 140 additions and 33 deletions
|
|
@ -743,12 +743,20 @@ export const ChatRowContent = ({
|
|||
case "listFilesTopLevel":
|
||||
case "listFilesRecursive": {
|
||||
const isRecursive = tool.tool === "listFilesRecursive"
|
||||
const DirIcon = isRecursive ? FolderTree : ListTree
|
||||
const dirIconLabel = isRecursive ? "Folder tree icon" : "List files icon"
|
||||
|
||||
// Check if this is a batch directory listing request
|
||||
const isBatchDirRequest = message.type === "ask" && tool.batchDirs && Array.isArray(tool.batchDirs)
|
||||
|
||||
// When batching, check if all dirs share the same recursive value
|
||||
const allTopLevel = tool.batchDirs?.every((d: { recursive: boolean }) => !d.recursive)
|
||||
const DirIcon = isBatchDirRequest && !allTopLevel ? FolderTree : isRecursive ? FolderTree : ListTree
|
||||
const dirIconLabel =
|
||||
isBatchDirRequest && !allTopLevel
|
||||
? "Folder tree icon"
|
||||
: isRecursive
|
||||
? "Folder tree icon"
|
||||
: "List files icon"
|
||||
|
||||
if (isBatchDirRequest) {
|
||||
return (
|
||||
<>
|
||||
|
|
|
|||
|
|
@ -312,6 +312,14 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
|
|||
case "editedExistingFile":
|
||||
case "appliedDiff":
|
||||
case "newFileCreated":
|
||||
if (tool.batchDiffs && Array.isArray(tool.batchDiffs)) {
|
||||
setPrimaryButtonText(t("chat:edit-batch.approve.title"))
|
||||
setSecondaryButtonText(t("chat:edit-batch.deny.title"))
|
||||
} else {
|
||||
setPrimaryButtonText(t("chat:save.title"))
|
||||
setSecondaryButtonText(t("chat:reject.title"))
|
||||
}
|
||||
break
|
||||
case "generateImage":
|
||||
setPrimaryButtonText(t("chat:save.title"))
|
||||
setSecondaryButtonText(t("chat:reject.title"))
|
||||
|
|
@ -332,8 +340,8 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
|
|||
case "listFilesTopLevel":
|
||||
case "listFilesRecursive":
|
||||
if (tool.batchDirs && Array.isArray(tool.batchDirs)) {
|
||||
setPrimaryButtonText(t("chat:read-batch.approve.title"))
|
||||
setSecondaryButtonText(t("chat:read-batch.deny.title"))
|
||||
setPrimaryButtonText(t("chat:list-batch.approve.title"))
|
||||
setSecondaryButtonText(t("chat:list-batch.deny.title"))
|
||||
} else {
|
||||
setPrimaryButtonText(t("chat:approve.title"))
|
||||
setSecondaryButtonText(t("chat:reject.title"))
|
||||
|
|
@ -1215,7 +1223,12 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
|
|||
}
|
||||
})
|
||||
|
||||
const firstTool = JSON.parse(batch[0].text || "{}")
|
||||
let firstTool
|
||||
try {
|
||||
firstTool = JSON.parse(batch[0].text || "{}")
|
||||
} catch {
|
||||
return batch[0]
|
||||
}
|
||||
return {
|
||||
...batch[0],
|
||||
text: JSON.stringify({ ...firstTool, batchFiles }),
|
||||
|
|
@ -1238,7 +1251,12 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
|
|||
}
|
||||
})
|
||||
|
||||
const firstTool = JSON.parse(batch[0].text || "{}")
|
||||
let firstTool
|
||||
try {
|
||||
firstTool = JSON.parse(batch[0].text || "{}")
|
||||
} catch {
|
||||
return batch[0]
|
||||
}
|
||||
return {
|
||||
...batch[0],
|
||||
text: JSON.stringify({ ...firstTool, batchDirs }),
|
||||
|
|
@ -1262,7 +1280,12 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
|
|||
}
|
||||
})
|
||||
|
||||
const firstTool = JSON.parse(batch[0].text || "{}")
|
||||
let firstTool
|
||||
try {
|
||||
firstTool = JSON.parse(batch[0].text || "{}")
|
||||
} catch {
|
||||
return batch[0]
|
||||
}
|
||||
return {
|
||||
...batch[0],
|
||||
text: JSON.stringify({ ...firstTool, batchDiffs }),
|
||||
|
|
|
|||
|
|
@ -84,4 +84,28 @@ describe("BatchListFilesPermission", () => {
|
|||
// All 3 dirs should be inside this container
|
||||
expect(container?.querySelectorAll(".flex.items-center.gap-2")).toHaveLength(mockDirs.length)
|
||||
})
|
||||
|
||||
it("renders a single directory", () => {
|
||||
const singleDir = [
|
||||
{
|
||||
key: "apps/cli",
|
||||
path: "apps/cli",
|
||||
recursive: false,
|
||||
isOutsideWorkspace: false,
|
||||
},
|
||||
]
|
||||
|
||||
render(
|
||||
<TranslationProvider>
|
||||
<BatchListFilesPermission dirs={singleDir} ts={Date.now()} />
|
||||
</TranslationProvider>,
|
||||
)
|
||||
|
||||
expect(screen.getByText("apps/cli")).toBeInTheDocument()
|
||||
|
||||
// Single directory should still be rendered inside the container
|
||||
const bordered = screen.getByText("apps/cli").closest(".border.border-border.rounded-md")
|
||||
expect(bordered).toBeInTheDocument()
|
||||
expect(bordered?.querySelectorAll(".flex.items-center.gap-2")).toHaveLength(1)
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -74,6 +74,22 @@
|
|||
"title": "Deny All"
|
||||
}
|
||||
},
|
||||
"list-batch": {
|
||||
"approve": {
|
||||
"title": "Approve All"
|
||||
},
|
||||
"deny": {
|
||||
"title": "Deny All"
|
||||
}
|
||||
},
|
||||
"edit-batch": {
|
||||
"approve": {
|
||||
"title": "Save All"
|
||||
},
|
||||
"deny": {
|
||||
"title": "Deny All"
|
||||
}
|
||||
},
|
||||
"runCommand": {
|
||||
"title": "Run",
|
||||
"tooltip": "Execute this command"
|
||||
|
|
|
|||
|
|
@ -1,17 +1,21 @@
|
|||
import type { ClineMessage } from "@roo-code/types"
|
||||
|
||||
import { batchConsecutive } from "../batchConsecutive"
|
||||
|
||||
/** Helper: create a minimal ClineMessage with an identifiable text field. */
|
||||
function msg(text: string, type: ClineMessage["type"] = "say"): ClineMessage {
|
||||
interface TestItem {
|
||||
ts: number
|
||||
type: string
|
||||
text: string
|
||||
}
|
||||
|
||||
/** Helper: create a minimal test item with an identifiable text field. */
|
||||
function msg(text: string, type = "say"): TestItem {
|
||||
return { ts: Date.now(), type, text }
|
||||
}
|
||||
|
||||
/** Predicate: matches messages whose text starts with "match". */
|
||||
const isMatch = (m: ClineMessage) => !!m.text?.startsWith("match")
|
||||
/** Predicate: matches items whose text starts with "match". */
|
||||
const isMatch = (m: TestItem) => !!m.text?.startsWith("match")
|
||||
|
||||
/** Synthesize: merges a batch into a single message with a "BATCH:" marker. */
|
||||
const synthesizeBatch = (batch: ClineMessage[]): ClineMessage => ({
|
||||
/** Synthesize: merges a batch into a single item with a "BATCH:" marker. */
|
||||
const synthesizeBatch = (batch: TestItem[]): TestItem => ({
|
||||
...batch[0],
|
||||
text: `BATCH:${batch.map((m) => m.text).join(",")}`,
|
||||
})
|
||||
|
|
@ -71,4 +75,42 @@ describe("batchConsecutive", () => {
|
|||
expect(result[3].text).toBe("other-2")
|
||||
expect(result[4].text).toBe("BATCH:match-5,match-6")
|
||||
})
|
||||
|
||||
test("all items match → single synthetic message", () => {
|
||||
const items = [msg("match-1"), msg("match-2"), msg("match-3")]
|
||||
const result = batchConsecutive(items, isMatch, synthesizeBatch)
|
||||
expect(result).toHaveLength(1)
|
||||
expect(result[0].text).toBe("BATCH:match-1,match-2,match-3")
|
||||
})
|
||||
|
||||
test("does not mutate the input array", () => {
|
||||
const items = [msg("match-1"), msg("match-2")]
|
||||
const original = [...items]
|
||||
batchConsecutive(items, isMatch, synthesizeBatch)
|
||||
expect(items).toHaveLength(2)
|
||||
expect(items).toEqual(original)
|
||||
})
|
||||
|
||||
test("returns a new array, not the same reference", () => {
|
||||
const items = [msg("a"), msg("b")]
|
||||
const result = batchConsecutive(items, isMatch, synthesizeBatch)
|
||||
expect(result).not.toBe(items)
|
||||
})
|
||||
|
||||
test("synthesize callback receives the correct batches", () => {
|
||||
const spy = vi.fn(synthesizeBatch)
|
||||
const items = [msg("match-1"), msg("match-2"), msg("other"), msg("match-3"), msg("match-4")]
|
||||
batchConsecutive(items, isMatch, spy)
|
||||
expect(spy).toHaveBeenCalledTimes(2)
|
||||
expect(spy.mock.calls[0][0]).toHaveLength(2)
|
||||
expect(spy.mock.calls[1][0]).toHaveLength(2)
|
||||
})
|
||||
|
||||
test("batch at the end of the array", () => {
|
||||
const items = [msg("other"), msg("match-1"), msg("match-2")]
|
||||
const result = batchConsecutive(items, isMatch, synthesizeBatch)
|
||||
expect(result).toHaveLength(2)
|
||||
expect(result[0].text).toBe("other")
|
||||
expect(result[1].text).toBe("BATCH:match-1,match-2")
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -1,29 +1,23 @@
|
|||
import type { ClineMessage } from "@roo-code/types"
|
||||
|
||||
/**
|
||||
* Walk a message array and batch runs of consecutive messages that match
|
||||
* `predicate` into synthetic messages produced by `synthesize`.
|
||||
* Walk an item array and batch runs of consecutive items that match
|
||||
* `predicate` into synthetic items produced by `synthesize`.
|
||||
*
|
||||
* - Runs of length 1 are passed through unchanged.
|
||||
* - Runs of length >= 2 are replaced by a single synthetic message.
|
||||
* - Non-matching messages are preserved in-order.
|
||||
* - Runs of length >= 2 are replaced by a single synthetic item.
|
||||
* - Non-matching items are preserved in-order.
|
||||
*/
|
||||
export function batchConsecutive(
|
||||
messages: ClineMessage[],
|
||||
predicate: (msg: ClineMessage) => boolean,
|
||||
synthesize: (batch: ClineMessage[]) => ClineMessage,
|
||||
): ClineMessage[] {
|
||||
const result: ClineMessage[] = []
|
||||
export function batchConsecutive<T>(items: T[], predicate: (item: T) => boolean, synthesize: (batch: T[]) => T): T[] {
|
||||
const result: T[] = []
|
||||
let i = 0
|
||||
|
||||
while (i < messages.length) {
|
||||
if (predicate(messages[i])) {
|
||||
while (i < items.length) {
|
||||
if (predicate(items[i])) {
|
||||
// Collect consecutive matches into a batch
|
||||
const batch: ClineMessage[] = [messages[i]]
|
||||
const batch: T[] = [items[i]]
|
||||
let j = i + 1
|
||||
|
||||
while (j < messages.length && predicate(messages[j])) {
|
||||
batch.push(messages[j])
|
||||
while (j < items.length && predicate(items[j])) {
|
||||
batch.push(items[j])
|
||||
j++
|
||||
}
|
||||
|
||||
|
|
@ -35,7 +29,7 @@ export function batchConsecutive(
|
|||
|
||||
i = j
|
||||
} else {
|
||||
result.push(messages[i])
|
||||
result.push(items[i])
|
||||
i++
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue