Skip to content

Commit ec4e0f8

Browse files
committed
fix(terminal): address CodeRabbit review findings
1 parent 987bfbb commit ec4e0f8

4 files changed

Lines changed: 22 additions & 7 deletions

File tree

apps/vscode-e2e/src/suite/tools/fast-exit-shell-race.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ suite("Fast-exit shell integration race", function () {
9090
},
9191
text: "FAST_EXIT_SHELL_RACE_E2E",
9292
}),
93-
timeout: 100_000, // TEMP: diagnostic probe, see if the marker ever arrives given patience
93+
timeout: 60_000,
9494
})
9595

9696
const elapsedMs = Date.now() - startedAt

src/core/tools/__tests__/executeCommandTool.spec.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -187,12 +187,12 @@ describe("executeCommandTool", () => {
187187
pushToolResult: mockPushToolResult as unknown as PushToolResult,
188188
})
189189

190-
// Verify - confirm the command was approved and result was pushed
191-
// The custom path handling is tested in integration tests
190+
// Verify - command approved, result pushed, and custom cwd passed to terminal
192191
expect(mockAskApproval).toHaveBeenCalledWith("command", "echo test")
193192
expect(mockPushToolResult).toHaveBeenCalled()
194-
const result = mockPushToolResult.mock.calls[0][0]
195-
expect(result).toContain("Command")
193+
const { TerminalRegistry } = await import("../../../integrations/terminal/TerminalRegistry")
194+
const firstArg = (TerminalRegistry.getOrCreateTerminal as ReturnType<typeof vitest.fn>).mock.calls[0][0]
195+
expect(firstArg).toBe("/custom/path")
196196
})
197197
})
198198

src/integrations/terminal/TerminalProcess.ts

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -546,7 +546,9 @@ export class TerminalProcess extends BaseTerminalProcess {
546546
return `begin\n${command}\nend`
547547
}
548548

549-
const scriptPath = path.join(os.tmpdir(), `roo-cmd-${Date.now()}-${Math.random().toString(36).slice(2)}.sh`)
549+
const scriptDir = fs.mkdtempSync(path.join(os.tmpdir(), "roo-cmd-"))
550+
this.scriptDir = scriptDir
551+
const scriptPath = path.join(scriptDir, "cmd.sh")
550552
fs.writeFileSync(scriptPath, command, { mode: 0o700 })
551553
this.scriptPath = scriptPath
552554

@@ -556,6 +558,7 @@ export class TerminalProcess extends BaseTerminalProcess {
556558
}
557559

558560
private scriptPath: string | undefined
561+
private scriptDir: string | undefined
559562

560563
private cleanupScriptFile() {
561564
if (this.scriptPath) {
@@ -566,6 +569,14 @@ export class TerminalProcess extends BaseTerminalProcess {
566569
}
567570
this.scriptPath = undefined
568571
}
572+
if (this.scriptDir) {
573+
try {
574+
fs.rmdirSync(this.scriptDir)
575+
} catch {
576+
// Best-effort: ignore if already removed or non-empty.
577+
}
578+
this.scriptDir = undefined
579+
}
569580
}
570581

571582
public override continue() {

src/integrations/terminal/TerminalRegistry.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,11 @@ export class TerminalRegistry {
6969
const process = terminal.process
7070
const isOwnExecution =
7171
!(process instanceof TerminalProcess) ||
72-
process.ownExecution === undefined ||
72+
// Allow undefined only when the process hasn't started yet (cold
73+
// terminal: process is assigned but run() hasn't called executeCommand).
74+
// Once isHot is true, ownExecution is always set — a stale start
75+
// event on a reused terminal must match exactly.
76+
(!process.isHot && process.ownExecution === undefined) ||
7377
process.ownExecution === e.execution
7478
if (!isOwnExecution) {
7579
console.info(

0 commit comments

Comments
 (0)