Skip to content

Commit 18a1be0

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 4b08f6a commit 18a1be0

5 files changed

Lines changed: 74 additions & 35 deletions

File tree

packages/app/src/context/terminal.tsx

Lines changed: 2 additions & 0 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,6 +193,7 @@ 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,
195197
}
196198
setStore("all", store.all.length, newTerminal)
197199
})

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

Lines changed: 1 addition & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ export function TerminalPanel() {
7373
if (prevCount === undefined || count <= prevCount) return
7474
const all = terminal.all()
7575
const last = all[all.length - 1]
76-
if (!last?.title.startsWith("Agent:")) return
76+
if (last?.source !== "agent") return
7777
if (!opened()) view().terminal.open()
7878
terminal.open(last.id)
7979
},
@@ -114,17 +114,6 @@ export function TerminalPanel() {
114114
}
115115
}
116116

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-
128117
createEffect(() => {
129118
if (opened()) return
130119
const active = document.activeElement

packages/opencode/src/pty/index.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,18 +62,20 @@ export const Info = Schema.Struct({
6262
cwd: Schema.String,
6363
status: Schema.Literals(["running", "exited"]),
6464
pid: Schema.Number,
65+
source: Schema.optional(Schema.Literals(["user", "agent"])),
6566
})
6667
.annotate({ identifier: "Pty" })
6768
.pipe(withStatics((s) => ({ zod: zod(s) })))
6869

69-
export type Info = Types.DeepMutable<Schema.Schema.Type<typeof Info>>
70+
export type Info = Types.DeepMutable<Schema.Schema.Type<typof Info>>
7071

7172
export const CreateInput = Schema.Struct({
7273
command: Schema.optional(Schema.String),
7374
args: Schema.optional(Schema.Array(Schema.String)),
7475
cwd: Schema.optional(Schema.String),
7576
title: Schema.optional(Schema.String),
7677
env: Schema.optional(Schema.Record(Schema.String, Schema.String)),
78+
source: Schema.optional(Schema.Literals(["user", "agent"])),
7779
}).pipe(withStatics((s) => ({ zod: zod(s) })))
7880

7981
export type CreateInput = Types.DeepMutable<Schema.Schema.Type<typeof CreateInput>>
@@ -215,6 +217,7 @@ export const layer = Layer.effect(
215217
cwd,
216218
status: "running",
217219
pid: proc.pid,
220+
source: input.source ?? ("user" as const),
218221
} as const
219222
const session: Active = {
220223
info,

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)