-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Fix issues with subtasks attempting completion along with commands #3156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
4a36340
0cb3e23
9fb83ba
1f103b7
7c9943d
82dee38
6e86438
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,7 @@ import { ToolUse, AskApproval, HandleError, PushToolResult, RemoveClosingTag, To | |
| import { formatResponse } from "../prompts/responses" | ||
| import { unescapeHtmlEntities } from "../../utils/text-normalization" | ||
| import { telemetryService } from "../../services/telemetry/TelemetryService" | ||
| import { ExitCodeDetails, RooTerminalCallbacks, RooTerminalProcess } from "../../integrations/terminal/types" | ||
| import { ExitCodeDetails, RooTerminalCallbacks } from "../../integrations/terminal/types" | ||
| import { TerminalRegistry } from "../../integrations/terminal/TerminalRegistry" | ||
| import { Terminal } from "../../integrations/terminal/Terminal" | ||
|
|
||
|
|
@@ -48,14 +48,15 @@ export async function executeCommandTool( | |
|
|
||
| cline.consecutiveMistakeCount = 0 | ||
|
|
||
| const executionId = Date.now().toString() | ||
| command = unescapeHtmlEntities(command) // Unescape HTML entities. | ||
| const didApprove = await askApproval("command", command, { id: executionId }) | ||
| const didApprove = await askApproval("command", command) | ||
|
|
||
| if (!didApprove) { | ||
| return | ||
| } | ||
|
|
||
| const executionId = cline.lastMessageTs?.toString() ?? Date.now().toString() | ||
|
|
||
| const clineProvider = await cline.providerRef.deref() | ||
| const clineProviderState = await clineProvider?.getState() | ||
| const { terminalOutputLineLimit = 500, terminalShellIntegrationDisabled = false } = clineProviderState ?? {} | ||
|
|
@@ -140,7 +141,6 @@ export async function executeCommand( | |
| } | ||
|
|
||
| let message: { text?: string; images?: string[] } | undefined | ||
| let runInBackground = false | ||
| let completed = false | ||
| let result: string = "" | ||
| let exitDetails: ExitCodeDetails | undefined | ||
|
|
@@ -150,23 +150,9 @@ export async function executeCommand( | |
| const clineProvider = await cline.providerRef.deref() | ||
|
|
||
| const callbacks: RooTerminalCallbacks = { | ||
| onLine: async (output: string, process: RooTerminalProcess) => { | ||
| onLine: async (output: string) => { | ||
| const status: CommandExecutionStatus = { executionId, status: "output", output } | ||
| clineProvider?.postMessageToWebview({ type: "commandExecutionStatus", text: JSON.stringify(status) }) | ||
|
|
||
| if (runInBackground) { | ||
| return | ||
| } | ||
|
|
||
| try { | ||
| const { response, text, images } = await cline.ask("command_output", "") | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To expand on this - we changed the way "Continue while command executes" works. It's triggered directly by the webview with a |
||
| runInBackground = true | ||
|
|
||
| if (response === "messageResponse") { | ||
| message = { text, images } | ||
| process.continue() | ||
| } | ||
| } catch (_error) {} | ||
| }, | ||
| onCompleted: (output: string | undefined) => { | ||
| result = Terminal.compressTerminalOutput(output ?? "", terminalOutputLineLimit) | ||
|
|
@@ -177,6 +163,9 @@ export async function executeCommand( | |
| console.log(`[executeCommand] onShellExecutionStarted: ${pid}`) | ||
| const status: CommandExecutionStatus = { executionId, status: "started", pid, command } | ||
| clineProvider?.postMessageToWebview({ type: "commandExecutionStatus", text: JSON.stringify(status) }) | ||
| // This `command_output` message tells the webview to render the | ||
| // appropriate primary and secondary buttons. | ||
| cline.say("command_output", "") | ||
| }, | ||
| onShellExecutionComplete: (details: ExitCodeDetails) => { | ||
| const status: CommandExecutionStatus = { executionId, status: "exited", exitCode: details.exitCode } | ||
|
|
@@ -216,8 +205,7 @@ export async function executeCommand( | |
| // Wait for a short delay to ensure all messages are sent to the webview. | ||
| // This delay allows time for non-awaited promises to be created and | ||
| // for their associated messages to be sent to the webview, maintaining | ||
| // the correct order of messages (although the webview is smart about | ||
| // grouping command_output messages despite any gaps anyways). | ||
| // the correct order of messages. | ||
| await delay(50) | ||
|
|
||
| if (message) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -184,7 +184,9 @@ export const webviewMessageHandler = async (provider: ClineProvider, message: We | |||||
| await provider.postStateToWebview() | ||||||
| break | ||||||
| case "askResponse": | ||||||
| provider.getCurrentCline()?.handleWebviewAskResponse(message.askResponse!, message.text, message.images) | ||||||
| const instance = provider.getCurrentCline() | ||||||
| console.log("askResponse ->", message, !!instance) | ||||||
|
||||||
| console.log("askResponse ->", message, !!instance) | |
| provider.log(`askResponse ->`, message, !!instance) |
This comment was generated because it violated a code review rule: mrule_OR1S8PRRHcvbdFib.
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -116,7 +116,7 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro | |||
| const [selectedImages, setSelectedImages] = useState<string[]>([]) | ||||
|
|
||||
| // we need to hold on to the ask because useEffect > lastMessage will always let us know when an ask comes in and handle it, but by the time handleMessage is called, the last message might not be the ask anymore (it could be a say that followed) | ||||
| const [clineAsk, setClineAsk] = useState<ClineAsk | undefined>(undefined) | ||||
| const [clineAsk, setClineAsk] = useState<ClineAsk | "command_output" | undefined>(undefined) | ||||
| const [enableButtons, setEnableButtons] = useState<boolean>(false) | ||||
| const [primaryButtonText, setPrimaryButtonText] = useState<string | undefined>(undefined) | ||||
| const [secondaryButtonText, setSecondaryButtonText] = useState<string | undefined>(undefined) | ||||
|
|
@@ -229,13 +229,6 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro | |||
| setPrimaryButtonText(t("chat:runCommand.title")) | ||||
| setSecondaryButtonText(t("chat:reject.title")) | ||||
| break | ||||
| case "command_output": | ||||
| setTextAreaDisabled(false) | ||||
| setClineAsk("command_output") | ||||
| setEnableButtons(true) | ||||
| setPrimaryButtonText(t("chat:proceedWhileRunning.title")) | ||||
| setSecondaryButtonText(t("chat:killCommand.title")) | ||||
| break | ||||
| case "use_mcp_server": | ||||
| if (!isAutoApproved(lastMessage) && !isPartial) { | ||||
| playSound("notification") | ||||
|
|
@@ -289,8 +282,8 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro | |||
| setTextAreaDisabled(true) | ||||
| break | ||||
| case "api_req_started": | ||||
| if (secondLastMessage?.ask === "command_output") { | ||||
| // If the last ask is a command_output, and we | ||||
| if (secondLastMessage?.ask === "command") { | ||||
| // If the last ask is a command, and we | ||||
| // receive an api_req_started, then that means | ||||
| // the command has finished and we don't need | ||||
| // input from the user anymore (in every other | ||||
|
|
@@ -304,12 +297,18 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro | |||
| setEnableButtons(false) | ||||
| } | ||||
| break | ||||
| case "command_output": | ||||
| setTextAreaDisabled(false) | ||||
| setClineAsk("command_output") | ||||
| setEnableButtons(true) | ||||
| setPrimaryButtonText(t("chat:proceedWhileRunning.title")) | ||||
| setSecondaryButtonText(t("chat:killCommand.title")) | ||||
| break | ||||
| case "api_req_finished": | ||||
| case "error": | ||||
| case "text": | ||||
| case "browser_action": | ||||
| case "browser_action_result": | ||||
| case "command_output": | ||||
| case "mcp_server_request_started": | ||||
| case "mcp_server_response": | ||||
| case "completion_result": | ||||
|
|
@@ -399,7 +398,6 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro | |||
| case "tool": | ||||
| case "browser_action_launch": | ||||
| case "command": // User can provide feedback to a tool or command use. | ||||
| case "command_output": // User can send input to command stdin. | ||||
| case "use_mcp_server": | ||||
| case "completion_result": // If this happens then the user has feedback for the completion result. | ||||
| case "resume_task": | ||||
|
|
@@ -441,6 +439,8 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro | |||
| (text?: string, images?: string[]) => { | ||||
| const trimmedInput = text?.trim() | ||||
|
|
||||
| console.log(`handlePrimaryButtonClick -> clineAsk=${clineAsk}`, text) | ||||
|
||||
| console.log(`handlePrimaryButtonClick -> clineAsk=${clineAsk}`, text) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to make a lot more sense - hopefully not missing any obvious reason to use the lastMessage text
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only the
resultparameter from theattempt_completiontool right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes exactly. Seems like that's what we want to return to the parent task.