Skip to content

Commit 1f59a77

Browse files
committed
refactor: address PR review comments from daniel-lxs
- Extract duplicated delegation logic into delegateToParent() method - Add DelegationProvider interface to replace 'any' type cast - Remove all '// NEW:' comments from codebase
1 parent 701ae62 commit 1f59a77

File tree

8 files changed

+60
-53
lines changed

8 files changed

+60
-53
lines changed

packages/cloud/src/bridge/ExtensionChannel.ts

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -188,12 +188,9 @@ export class ExtensionChannel extends BaseChannel<
188188
{ from: RooCodeEventName.TaskPaused, to: ExtensionBridgeEventName.TaskPaused },
189189
{ from: RooCodeEventName.TaskUnpaused, to: ExtensionBridgeEventName.TaskUnpaused },
190190
{ from: RooCodeEventName.TaskSpawned, to: ExtensionBridgeEventName.TaskSpawned },
191-
192-
// NEW: Delegation events
193191
{ from: RooCodeEventName.TaskDelegated, to: ExtensionBridgeEventName.TaskDelegated },
194192
{ from: RooCodeEventName.TaskDelegationCompleted, to: ExtensionBridgeEventName.TaskDelegationCompleted },
195193
{ from: RooCodeEventName.TaskDelegationResumed, to: ExtensionBridgeEventName.TaskDelegationResumed },
196-
197194
{ from: RooCodeEventName.TaskUserMessage, to: ExtensionBridgeEventName.TaskUserMessage },
198195
{ from: RooCodeEventName.TaskTokenUsageUpdated, to: ExtensionBridgeEventName.TaskTokenUsageUpdated },
199196
] as const

packages/cloud/src/bridge/__tests__/ExtensionChannel.test.ts

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -120,8 +120,6 @@ describe("ExtensionChannel", () => {
120120
RooCodeEventName.TaskPaused,
121121
RooCodeEventName.TaskUnpaused,
122122
RooCodeEventName.TaskSpawned,
123-
124-
// NEW: Delegation events
125123
RooCodeEventName.TaskDelegated,
126124
RooCodeEventName.TaskDelegationCompleted,
127125
RooCodeEventName.TaskDelegationResumed,

packages/types/src/cloud.ts

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -440,8 +440,6 @@ export enum ExtensionBridgeEventName {
440440
TaskPaused = RooCodeEventName.TaskPaused,
441441
TaskUnpaused = RooCodeEventName.TaskUnpaused,
442442
TaskSpawned = RooCodeEventName.TaskSpawned,
443-
444-
// NEW: Delegation events
445443
TaskDelegated = RooCodeEventName.TaskDelegated,
446444
TaskDelegationCompleted = RooCodeEventName.TaskDelegationCompleted,
447445
TaskDelegationResumed = RooCodeEventName.TaskDelegationResumed,
@@ -525,8 +523,6 @@ export const extensionBridgeEventSchema = z.discriminatedUnion("type", [
525523
instance: extensionInstanceSchema,
526524
timestamp: z.number(),
527525
}),
528-
529-
// NEW: Delegation events
530526
z.object({
531527
type: z.literal(ExtensionBridgeEventName.TaskDelegated),
532528
instance: extensionInstanceSchema,

packages/types/src/events.ts

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,6 @@ export enum RooCodeEventName {
2626
TaskPaused = "taskPaused",
2727
TaskUnpaused = "taskUnpaused",
2828
TaskSpawned = "taskSpawned",
29-
30-
// NEW: Metadata-driven subtask lifecycle
3129
TaskDelegated = "taskDelegated",
3230
TaskDelegationCompleted = "taskDelegationCompleted",
3331
TaskDelegationResumed = "taskDelegationResumed",
@@ -78,8 +76,6 @@ export const rooCodeEventsSchema = z.object({
7876
[RooCodeEventName.TaskPaused]: z.tuple([z.string()]),
7977
[RooCodeEventName.TaskUnpaused]: z.tuple([z.string()]),
8078
[RooCodeEventName.TaskSpawned]: z.tuple([z.string(), z.string()]),
81-
82-
// NEW: Delegation events
8379
[RooCodeEventName.TaskDelegated]: z.tuple([
8480
z.string(), // parentTaskId
8581
z.string(), // childTaskId
@@ -189,7 +185,6 @@ export const taskEventSchema = z.discriminatedUnion("eventName", [
189185
payload: rooCodeEventsSchema.shape[RooCodeEventName.TaskSpawned],
190186
taskId: z.number().optional(),
191187
}),
192-
// NEW: Delegation events
193188
z.object({
194189
eventName: z.literal(RooCodeEventName.TaskDelegated),
195190
payload: rooCodeEventsSchema.shape[RooCodeEventName.TaskDelegated],

packages/types/src/history.ts

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,6 @@ export const historyItemSchema = z.object({
1919
size: z.number().optional(),
2020
workspace: z.string().optional(),
2121
mode: z.string().optional(),
22-
23-
// NEW: Delegation metadata
2422
status: z.enum(["active", "completed", "aborted", "delegated"]).optional(),
2523
delegatedToId: z.string().optional(), // Last child this parent delegated to
2624
childIds: z.array(z.string()).optional(), // All children spawned by this task

packages/types/src/task.ts

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -72,8 +72,6 @@ export type TaskProviderEvents = {
7272
[RooCodeEventName.TaskPaused]: [taskId: string]
7373
[RooCodeEventName.TaskUnpaused]: [taskId: string]
7474
[RooCodeEventName.TaskSpawned]: [taskId: string]
75-
76-
// NEW: Delegation events (provider-level)
7775
[RooCodeEventName.TaskDelegated]: [parentTaskId: string, childTaskId: string]
7876
[RooCodeEventName.TaskDelegationCompleted]: [parentTaskId: string, childTaskId: string, summary: string]
7977
[RooCodeEventName.TaskDelegationResumed]: [parentTaskId: string, childTaskId: string]

src/core/tools/AttemptCompletionTool.ts

Lines changed: 60 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import * as vscode from "vscode"
22

3-
import { RooCodeEventName } from "@roo-code/types"
3+
import { RooCodeEventName, type HistoryItem } from "@roo-code/types"
44
import { TelemetryService } from "@roo-code/telemetry"
55

66
import { Task } from "../task/Task"
@@ -19,6 +19,18 @@ export interface AttemptCompletionCallbacks extends ToolCallbacks {
1919
toolDescription: () => string
2020
}
2121

22+
/**
23+
* Interface for provider methods needed by AttemptCompletionTool for delegation handling.
24+
*/
25+
interface DelegationProvider {
26+
getTaskWithId(id: string): Promise<{ historyItem: HistoryItem }>
27+
reopenParentFromDelegation(params: {
28+
parentTaskId: string
29+
childTaskId: string
30+
completionResultSummary: string
31+
}): Promise<void>
32+
}
33+
2234
export class AttemptCompletionTool extends BaseTool<"attempt_completion"> {
2335
readonly name = "attempt_completion" as const
2436

@@ -70,51 +82,36 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> {
7082
if (task.parentTaskId) {
7183
// Check if this subtask has already completed and returned to parent
7284
// to prevent duplicate tool_results when user revisits from history
73-
const provider = task.providerRef.deref()
85+
const provider = task.providerRef.deref() as DelegationProvider | undefined
7486
if (provider) {
7587
try {
76-
const { historyItem } = await (provider as any).getTaskWithId(task.taskId)
88+
const { historyItem } = await provider.getTaskWithId(task.taskId)
7789
if (historyItem?.status === "completed") {
7890
// Subtask already completed - skip delegation flow entirely
7991
// Fall through to normal completion ask flow below (outside this if block)
8092
// This shows the user the completion result and waits for acceptance
8193
// without injecting another tool_result to the parent
8294
} else {
8395
// Normal subtask completion - do delegation
84-
const didApprove = await askFinishSubTaskApproval()
85-
86-
if (!didApprove) {
87-
pushToolResult(formatResponse.toolDenied())
88-
return
89-
}
90-
91-
pushToolResult("")
92-
93-
// Use the new metadata-driven delegation flow
94-
await (provider as any).reopenParentFromDelegation({
95-
parentTaskId: task.parentTaskId,
96-
childTaskId: task.taskId,
97-
completionResultSummary: result,
98-
})
99-
return
96+
const delegated = await this.delegateToParent(
97+
task,
98+
result,
99+
provider,
100+
askFinishSubTaskApproval,
101+
pushToolResult,
102+
)
103+
if (delegated) return
100104
}
101105
} catch {
102106
// If we can't get the history, proceed with normal delegation flow
103-
const didApprove = await askFinishSubTaskApproval()
104-
105-
if (!didApprove) {
106-
pushToolResult(formatResponse.toolDenied())
107-
return
108-
}
109-
110-
pushToolResult("")
111-
112-
await (provider as any).reopenParentFromDelegation({
113-
parentTaskId: task.parentTaskId,
114-
childTaskId: task.taskId,
115-
completionResultSummary: result,
116-
})
117-
return
107+
const delegated = await this.delegateToParent(
108+
task,
109+
result,
110+
provider,
111+
askFinishSubTaskApproval,
112+
pushToolResult,
113+
)
114+
if (delegated) return
118115
}
119116
}
120117
}
@@ -135,6 +132,35 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> {
135132
}
136133
}
137134

135+
/**
136+
* Handles the common delegation flow when a subtask completes.
137+
* Returns true if delegation was performed and the caller should return early.
138+
*/
139+
private async delegateToParent(
140+
task: Task,
141+
result: string,
142+
provider: DelegationProvider,
143+
askFinishSubTaskApproval: () => Promise<boolean>,
144+
pushToolResult: (result: string) => void,
145+
): Promise<boolean> {
146+
const didApprove = await askFinishSubTaskApproval()
147+
148+
if (!didApprove) {
149+
pushToolResult(formatResponse.toolDenied())
150+
return true
151+
}
152+
153+
pushToolResult("")
154+
155+
await provider.reopenParentFromDelegation({
156+
parentTaskId: task.parentTaskId!,
157+
childTaskId: task.taskId,
158+
completionResultSummary: result,
159+
})
160+
161+
return true
162+
}
163+
138164
override async handlePartial(task: Task, block: ToolUse<"attempt_completion">): Promise<void> {
139165
const result: string | undefined = block.params.result
140166
const command: string | undefined = block.params.command

src/extension/api.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -271,7 +271,6 @@ export class API extends EventEmitter<RooCodeEvents> implements RooCodeAPI {
271271
this.emit(RooCodeEventName.TaskSpawned, task.taskId, childTaskId)
272272
})
273273

274-
// NEW: Delegation events
275274
task.on(RooCodeEventName.TaskDelegated as any, (childTaskId: string) => {
276275
;(this.emit as any)(RooCodeEventName.TaskDelegated, task.taskId, childTaskId)
277276
})

0 commit comments

Comments
 (0)