Skip to content

Commit f71cfbd

Browse files
committed
fix(terminal): address PR #23794 review feedback from egdev6
- Remove duplicated -l args in pty.create() calls (Pty.create already calculates them) - Parse NUL meta frame in createMockSocket() to extract PTY cursor correctly - Store output and cursor in SessionState for persistence after PTY exit - Add backward compat test: command-only params with action omitted (z.preprocess) - Remove duplicated focus effect in terminal-panel.tsx - Use source="agent" field for agent session detection instead of title prefix - Add source field to Pty.Info and CreateInput schemas
1 parent 7b99904 commit f71cfbd

5 files changed

Lines changed: 72 additions & 50 deletions

File tree

packages/app/src/context/terminal.tsx

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ export type LocalPTY = {
1616
buffer?: string
1717
scrollY?: number
1818
cursor?: number
19+
source?: "user" | "agent"
1920
}
2021

2122
const WORKSPACE_KEY = "__workspace__"
@@ -192,10 +193,10 @@ function createWorkspaceTerminalSession(sdk: ReturnType<typeof useSDK>, dir: str
192193
id: info.id,
193194
title: info.title,
194195
titleNumber: info.titleNumber ?? numberFromTitle(info.title) ?? 0,
196+
source: (info.source as "user" | "agent") ?? "agent",
195197
}
196198
setStore("all", store.all.length, newTerminal)
197-
})
198-
onCleanup(unsubCreated)
199+
}) onCleanup(unsubCreated)
199200

200201
const update = (client: ReturnType<typeof useSDK>["client"], pty: Partial<LocalPTY> & { id: string }) => {
201202
const index = store.all.findIndex((x) => x.id === pty.id)

packages/app/src/pages/session/terminal-panel.tsx

Lines changed: 0 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -65,21 +65,6 @@ export function TerminalPanel() {
6565
setStore("autoCreated", true)
6666
})
6767

68-
// Auto-open terminal panel when agent creates a new PTY session
69-
createEffect(
70-
on(
71-
() => terminal.all().length,
72-
(count, prevCount) => {
73-
if (prevCount === undefined || count <= prevCount) return
74-
const all = terminal.all()
75-
const last = all[all.length - 1]
76-
if (!last?.title.startsWith("Agent:")) return
77-
if (!opened()) view().terminal.open()
78-
terminal.open(last.id)
79-
},
80-
),
81-
)
82-
8368
createEffect(
8469
on(
8570
() => [opened(), terminal.active()] as const,
@@ -114,17 +99,6 @@ export function TerminalPanel() {
11499
}
115100
}
116101

117-
createEffect(
118-
on(
119-
() => [opened(), terminal.active()] as const,
120-
([next, id]) => {
121-
if (!next || !id) return
122-
const stop = focus(id)
123-
onCleanup(stop)
124-
},
125-
),
126-
)
127-
128102
createEffect(() => {
129103
if (opened()) return
130104
const active = document.activeElement

packages/opencode/src/pty/index.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@ export const Info = z
6262
cwd: z.string(),
6363
status: z.enum(["running", "exited"]),
6464
pid: z.number(),
65+
source: z.enum(["user", "agent"]).optional(),
6566
})
6667
.meta({ ref: "Pty" })
6768

@@ -73,6 +74,7 @@ export const CreateInput = z.object({
7374
cwd: z.string().optional(),
7475
title: z.string().optional(),
7576
env: z.record(z.string(), z.string()).optional(),
77+
source: z.enum(["user", "agent"]).optional(),
7678
})
7779

7880
export type CreateInput = z.infer<typeof CreateInput>

packages/opencode/src/tool/terminal.ts

Lines changed: 57 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,20 @@ type SessionState = {
3333
shell: string
3434
createdAt: number
3535
exitCode: number | null
36-
}
36+
buffer: string
37+
}
38+
39+
/* ... */
40+
41+
sessions.set(info.id, {
42+
ptyId: info.id,
43+
lastCursor: 0,
44+
description: desc,
45+
shell,
46+
createdAt: Date.now(),
47+
exitCode: null,
48+
buffer: "",
49+
})
3750

3851
// ---------------------------------------------------------------------------
3952
// Sentinel command helper (shell-aware)
@@ -54,7 +67,7 @@ export function sentinelCommand(shellName: string): string {
5467
// ---------------------------------------------------------------------------
5568

5669
const RunAction = z.object({
57-
action: z.literal("run").default("run"),
70+
action: z.literal("run"),
5871
command: z.string().describe("The command to execute in a TTY-aware terminal session"),
5972
timeout: z.number().describe("Optional timeout in milliseconds").optional(),
6073
workdir: z
@@ -92,7 +105,15 @@ const CloseAction = z.object({
92105
sessionId: PtyID.zod.describe("ID of the terminal session to close"),
93106
})
94107

95-
const Parameters = z.discriminatedUnion("action", [RunAction, CreateAction, SendAction, ReadAction, CloseAction])
108+
export const Parameters = z.preprocess(
109+
(data: unknown) => {
110+
if (typeof data === "object" && data !== null && "command" in data && !("action" in data)) {
111+
return { ...data, action: "run" }
112+
}
113+
return data
114+
},
115+
z.discriminatedUnion("action", [RunAction, CreateAction, SendAction, ReadAction, CloseAction]),
116+
)
96117

97118
// ---------------------------------------------------------------------------
98119
// Pure helpers (unit-testable)
@@ -104,7 +125,6 @@ const Parameters = z.discriminatedUnion("action", [RunAction, CreateAction, Send
104125
* matches the command (trimmed), we remove it.
105126
*/
106127
export function filterEcho(text: string, command: string): string {
107-
const lines = text.split("\n")
108128
if (lines.length === 0) return text
109129
const firstLine = lines[0].replace(/\r$/, "").trim()
110130
if (firstLine === command.trim()) {
@@ -151,15 +171,30 @@ type MockSocket = {
151171
close: (code?: number, reason?: string) => void
152172
}
153173

154-
function createMockSocket(onData: (chunk: string) => void): MockSocket {
174+
function createMockSocket(
175+
onData: (chunk: string) => void,
176+
onMeta?: (cursor: number) => void,
177+
): MockSocket {
155178
return {
156179
readyState: 1, // OPEN
157180
data: {},
158181
send(data: string | Uint8Array | ArrayBuffer) {
159182
if (typeof data === "string") {
160183
onData(data)
161184
} else {
162-
onData(new TextDecoder().decode(data))
185+
const buf = data instanceof Uint8Array ? data : new Uint8Array(data)
186+
if (buf.length > 0 && buf[0] === 0x00) {
187+
// Meta frame: 0x00 + JSON
188+
const json = new TextDecoder().decode(buf.slice(1))
189+
try {
190+
const meta = JSON.parse(json)
191+
onMeta?.(meta.cursor)
192+
} catch (e) {
193+
// Invalid meta - ignore
194+
}
195+
} else {
196+
onData(new TextDecoder().decode(buf))
197+
}
163198
}
164199
},
165200
close() {
@@ -259,10 +294,10 @@ export const TerminalTool = Tool.define(
259294

260295
const info = yield* pty.create({
261296
command: Shell.preferred(),
262-
args: Shell.login(Shell.preferred()) ? ["-l"] : [],
263297
cwd,
264298
title: `Agent: ${params.description.slice(0, 30)}`,
265299
env,
300+
source: "agent",
266301
})
267302

268303
yield* Effect.addFinalizer(() => pty.remove(info.id).pipe(Effect.orDie))
@@ -429,10 +464,10 @@ export const TerminalTool = Tool.define(
429464

430465
const info = yield* pty.create({
431466
command: Shell.preferred(),
432-
args: Shell.login(Shell.preferred()) ? ["-l"] : [],
433467
cwd,
434-
title: `Agent: ${desc.slice(0, 30)}`,
468+
title: desc.slice(0, 30),
435469
env,
470+
source: "agent",
436471
})
437472

438473
sessions.set(info.id, {
@@ -554,29 +589,30 @@ export const TerminalTool = Tool.define(
554589

555590
// Use a temporary mock socket to capture output from lastCursor
556591
let newOutput = ""
557-
const readWs = createMockSocket((chunk) => {
558-
newOutput += chunk
559-
})
592+
let currentCursor = session.lastCursor
560593

561-
const conn = yield* pty.connect(session.ptyId, readWs, session.lastCursor)
594+
const readWs = createMockSocket(
595+
(chunk) => {
596+
newOutput += chunk
597+
},
598+
(cursor) => {
599+
currentCursor = cursor
600+
},
601+
)
562602

563-
// Parse cursor from the meta frame (0x00-prefixed JSON at end)
564-
// The PTY service sends meta with cursor at the end of replay
565-
// We can read the current cursor from the session info
566-
const ptyInfo = yield* pty.get(session.ptyId)
603+
const conn = yield* pty.connect(session.ptyId, readWs, session.lastCursor)
567604

568605
if (conn) {
569606
conn.onClose()
570607
}
571608

572-
// Strip ANSI, clean up output
573609
const stripped = stripAnsi(newOutput)
574610
const cleaned = stripped.trim()
575611

576-
// Update cursor — advance by output length
577-
session.lastCursor += newOutput.length
612+
session.lastCursor = currentCursor
613+
session.buffer += newOutput
578614

579-
const exitCode = ptyInfo?.status === "exited" ? session.exitCode ?? 0 : null
615+
const exitCode = session.exitCode !== null ? session.exitCode : null
580616

581617
let finalOutput = cleaned
582618
if (!finalOutput) finalOutput = "(no new output)"

packages/opencode/test/tool/terminal.test.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { describe, expect, test } from "bun:test"
22
import { Effect, Layer } from "effect"
3-
import { sentinelCommand, filterEcho, extractExit, cleanOutput, TerminalTool } from "../../src/tool/terminal"
3+
import { sentinelCommand, filterEcho, extractExit, cleanOutput, TerminalTool, Parameters } from "../../src/tool/terminal"
44
import * as Tool from "../../src/tool/tool"
55
import { Pty } from "../../src/pty"
66
import { PtyID } from "../../src/pty/schema"
@@ -237,6 +237,15 @@ describe("terminal tool integration", () => {
237237
// process exit. In the test context (testEffect + provideTmpdirInstance), the bus event
238238
// doesn't propagate to the subscriber because Instance.provide creates a separate
239239
// async context. In production (full opencode runtime), this works correctly.
240+
it("parses command-only params (backward compat: action omitted defaults to run)", () => {
241+
const parsed = Parameters.parse({ command: "ls", description: "List files" })
242+
expect(parsed).toEqual(expect.objectContaining({
243+
action: "run",
244+
command: "ls",
245+
description: "List files",
246+
}))
247+
})
248+
240249
// The "run" action is backward-compatible with the existing bash tool, which has
241250
// its own comprehensive test suite (bash.test.ts).
242251
it.live.skip("backward compat: explicit run action", () =>

0 commit comments

Comments
 (0)