Skip to content

Commit d2d4d62

Browse files
fix: enforce machine attach stream contract
1 parent 3421f8e commit d2d4d62

7 files changed

Lines changed: 186 additions & 18 deletions

File tree

completions/pty.bash

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,9 @@ _pty() {
2727
COMPREPLY=($(compgen -W "-d --detach -a --attach -e --ephemeral --id --name --no-display-name --tag --env --cwd --isolate-env --force" -- "${cur}"))
2828
;;
2929
attach|a)
30+
if [[ "${prev}" == "--attach-stream-fd-v1" ]]; then
31+
return
32+
fi
3033
if [[ "${cur}" == -* ]]; then
3134
COMPREPLY=($(compgen -W "-r --auto-restart --no-restart --force --remote --attach-stream-fd-v1" -- "${cur}"))
3235
else

completions/pty.fish

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ complete -c pty -n '__pty_using_command attach a' -l auto-restart -s r -d 'Auto-
7979
complete -c pty -n '__pty_using_command attach a' -l no-restart -d 'Attach only; never prompt or restart an exited session'
8080
complete -c pty -n '__pty_using_command attach a' -l force -d 'Attach even from inside another pty'
8181
complete -c pty -n '__pty_using_command attach a' -l remote -d 'Attach a session on a fabric peer'
82-
complete -c pty -n '__pty_using_command attach a' -l attach-stream-fd-v1 -d 'Write framed machine events to an inherited fd'
82+
complete -c pty -n '__pty_using_command attach a' -l attach-stream-fd-v1 -x -d 'Write framed machine events to an inherited fd'
8383
complete -c pty -n '__pty_using_command attach a' -a '(__pty_sessions)' -d 'Session'
8484
complete -c pty -n '__pty_using_command exec' -F
8585
complete -c pty -n '__pty_using_command peek' -l follow -s f -d 'Follow output read-only'

completions/pty.zsh

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ _pty() {
7272
'--no-restart[Attach only; never prompt or restart an exited session]' \
7373
'--force[Attach even from inside another pty]' \
7474
'--remote[Attach a session on a fabric peer]' \
75-
'--attach-stream-fd-v1[Write framed machine events to an inherited fd]' \
75+
'--attach-stream-fd-v1[Write framed machine events to an inherited fd]:fd:' \
7676
'1:session:_pty_sessions'
7777
;;
7878
exec)
@@ -112,7 +112,7 @@ _pty() {
112112
'--tags[Include internal bookkeeping tags]' \
113113
'--filter-tag[Filter to k=v (repeatable, ALL match)]' \
114114
'--remote[Include remote sessions via pty-relay]' \
115-
'--status[Filter by status]:running|exited|vanished' \
115+
'--status[Filter by status]:status:(running exited vanished)' \
116116
'--older-than[Only sessions older than a duration]' \
117117
'--newer-than[Only sessions newer than a duration]' \
118118
'--summary[One-line count summary instead of the list]'

src/client.ts

Lines changed: 25 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -464,7 +464,7 @@ export function attach(options: AttachOptions): void {
464464
let inputWired = false;
465465
let stdinDataHandler: ((data: Buffer) => void) | null = null;
466466
let resizeHandler: (() => void) | null = null;
467-
let sawGeometry = false;
467+
let machineInitialState: "geometry" | "screen" | "ready" = "geometry";
468468
let streamBackpressured = false;
469469
const attachStream = attachStreamFd === undefined
470470
? null
@@ -587,14 +587,29 @@ export function attach(options: AttachOptions): void {
587587
packet.type === MessageType.DATA ||
588588
packet.type === MessageType.EXIT;
589589
if (!isStreamEvent) continue;
590-
if (!sawGeometry && packet.type !== MessageType.GEOMETRY) {
590+
if (machineInitialState === "geometry" && packet.type !== MessageType.GEOMETRY) {
591591
console.error(
592592
"pty attach: daemon does not support attach stream v1 (expected GEOMETRY before terminal events)",
593593
);
594594
finish(1);
595595
return;
596596
}
597-
if (packet.type === MessageType.GEOMETRY) sawGeometry = true;
597+
if (
598+
machineInitialState === "screen" &&
599+
packet.type !== MessageType.GEOMETRY &&
600+
packet.type !== MessageType.SCREEN
601+
) {
602+
console.error(
603+
`pty attach: daemon does not support attach stream v1 (expected SCREEN before ${packet.type === MessageType.DATA ? "DATA" : "EXIT"})`,
604+
);
605+
finish(1);
606+
return;
607+
}
608+
if (machineInitialState === "geometry" && packet.type === MessageType.GEOMETRY) {
609+
machineInitialState = "screen";
610+
} else if (machineInitialState === "screen" && packet.type === MessageType.SCREEN) {
611+
machineInitialState = "ready";
612+
}
598613
if (!attachStream.write(encodePacket(packet.type, packet.payload)) && !streamBackpressured) {
599614
streamBackpressured = true;
600615
socket.pause();
@@ -636,6 +651,11 @@ export function attach(options: AttachOptions): void {
636651
// No reconnect: preserve the original not-found / exit-code behavior.
637652
if (err) {
638653
cleanExit();
654+
if (attachStream && !sessionExited) {
655+
console.error(`pty attach: machine stream truncated before EXIT: ${err.message}`);
656+
finish(1);
657+
return;
658+
}
639659
const notReachable = err.code === "ENOENT" || err.code === "ECONNREFUSED"
640660
|| err.code === "ECONNRESET" || err.code === "EPIPE";
641661
if (notReachable) {
@@ -648,7 +668,7 @@ export function attach(options: AttachOptions): void {
648668
finish(1);
649669
} else {
650670
if (attachStream && !sessionExited) {
651-
console.error("pty attach: machine stream ended before an EXIT event");
671+
console.error("pty attach: machine stream truncated before EXIT: connection closed");
652672
finish(1);
653673
} else {
654674
finish(exitCode);
@@ -658,7 +678,7 @@ export function attach(options: AttachOptions): void {
658678

659679
function bindSocket(s: net.Socket, preConnected: boolean): void {
660680
reader = new PacketReader();
661-
sawGeometry = false;
681+
machineInitialState = "geometry";
662682
if (streamBackpressured) s.pause();
663683
s.on("data", handleData);
664684
s.on("error", (err: NodeJS.ErrnoException) => handleDisconnect(err));

src/completions.ts

Lines changed: 39 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -24,15 +24,15 @@
2424

2525
// ─── Spec ──────────────────────────────────────────────────────────────────
2626

27-
/** A `--flag`. `values` (when present) is the closed set of completions for
28-
* the flag's argument; absence means a boolean flag or a free-form value. */
27+
/** A `--flag`, optionally with one required argument. */
2928
interface FlagSpec {
3029
name: string;
3130
desc: string;
3231
/** `-x` short spelling, for fish `-s` and bash/zsh bundled forms. */
3332
short?: string;
34-
/** Closed set of values for the flag's argument; absence = free-form/boolean. */
35-
values?: readonly string[];
33+
argument?:
34+
| { _tag: "free"; name: string }
35+
| { _tag: "choices"; name: string; values: readonly string[] };
3636
}
3737

3838
/** A leaf command: a top-level subcommand. */
@@ -88,7 +88,11 @@ const COMMANDS: readonly CommandSpec[] = [
8888
{ name: "no-restart", desc: "Attach only; never prompt or restart an exited session" },
8989
{ name: "force", desc: "Attach even from inside another pty" },
9090
{ name: "remote", desc: "Attach a session on a fabric peer" },
91-
{ name: "attach-stream-fd-v1", desc: "Write framed machine events to an inherited fd" },
91+
{
92+
name: "attach-stream-fd-v1",
93+
desc: "Write framed machine events to an inherited fd",
94+
argument: { _tag: "free", name: "fd" },
95+
},
9296
],
9397
},
9498
{
@@ -141,7 +145,11 @@ const COMMANDS: readonly CommandSpec[] = [
141145
{ name: "tags", desc: "Include internal bookkeeping tags" },
142146
{ name: "filter-tag", desc: "Filter to k=v (repeatable, ALL match)" },
143147
{ name: "remote", desc: "Include remote sessions via pty-relay" },
144-
{ name: "status", desc: "Filter by status", values: STATUS_VALUES },
148+
{
149+
name: "status",
150+
desc: "Filter by status",
151+
argument: { _tag: "choices", name: "status", values: STATUS_VALUES },
152+
},
145153
{ name: "older-than", desc: "Only sessions older than a duration" },
146154
{ name: "newer-than", desc: "Only sessions newer than a duration" },
147155
{ name: "summary", desc: "One-line count summary instead of the list" },
@@ -332,9 +340,13 @@ function fishScript(): string {
332340
// Flags.
333341
for (const f of c.flags ?? []) {
334342
const short = f.short ? ` -s ${f.short}` : "";
335-
if (f.values) {
343+
if (f.argument?._tag === "choices") {
336344
out.push(
337-
`complete -c pty -n ${q(guard)} -l ${f.name}${short} -x -a ${q(f.values.join(" "))} -d ${q(f.desc)}`,
345+
`complete -c pty -n ${q(guard)} -l ${f.name}${short} -x -a ${q(f.argument.values.join(" "))} -d ${q(f.desc)}`,
346+
);
347+
} else if (f.argument?._tag === "free") {
348+
out.push(
349+
`complete -c pty -n ${q(guard)} -l ${f.name}${short} -x -d ${q(f.desc)}`,
338350
);
339351
} else {
340352
out.push(
@@ -410,6 +422,19 @@ function bashScript(): string {
410422
const guard = ` ${names})`;
411423
if (takesSessions) {
412424
lines.push(guard);
425+
for (const f of c.flags ?? []) {
426+
if (!f.argument) continue;
427+
const spellings = [f.short ? `-${f.short}` : null, `--${f.name}`].filter(Boolean);
428+
const condition = spellings.map((spelling) => `"\${prev}" == "${spelling}"`).join(" || ");
429+
lines.push(` if [[ ${condition} ]]; then`);
430+
if (f.argument._tag === "choices") {
431+
lines.push(
432+
` COMPREPLY=($(compgen -W "${f.argument.values.join(" ")}" -- "\${cur}"))`,
433+
);
434+
}
435+
lines.push(" return");
436+
lines.push(" fi");
437+
}
413438
lines.push(" if [[ \"${cur}\" == -* ]]; then");
414439
lines.push(
415440
` COMPREPLY=($(compgen -W "${flagWords}" -- "\${cur}"))`,
@@ -491,7 +516,12 @@ function zshScript(): string {
491516
const opt = f.short
492517
? `(${f.short} --${f.name}){${f.short},--${f.name}}`
493518
: `--${f.name}`;
494-
specs.push(`'${opt}[${f.desc}]${f.values ? ":" + f.values.join("|") : ""}'`);
519+
const argument = f.argument?._tag === "choices"
520+
? `:${f.argument.name}:(${f.argument.values.join(" ")})`
521+
: f.argument?._tag === "free"
522+
? `:${f.argument.name}:`
523+
: "";
524+
specs.push(`'${opt}[${f.desc}]${argument}'`);
495525
}
496526
if (c.dynamic === "sessions") specs.push("'1:session:_pty_sessions'");
497527
if (c.takesPath) specs.push("'1:directory:_directories'");

tests/attach-stream.test.ts

Lines changed: 90 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -210,20 +210,52 @@ describe("pty attach --attach-stream-fd-v1", () => {
210210
expect(result.stderr.toString()).toMatch(/daemon does not support attach stream v1/i);
211211
});
212212

213+
for (const premature of [
214+
encodePacket(MessageType.DATA, Buffer.from("too early")),
215+
encodeExit(0),
216+
]) {
217+
const type = new PacketReader().feed(premature)[0].type === MessageType.DATA ? "DATA" : "EXIT";
218+
it(`rejects a partial daemon that sends ${type} before the initial SCREEN`, async () => {
219+
const result = await runAgainstFakeDaemon((socket) => {
220+
socket.write(Buffer.concat([encodeGeometry(24, 80), premature]));
221+
});
222+
223+
expect(result.status).toBe(1);
224+
expect(result.stdout).toEqual(Buffer.alloc(0));
225+
expect(result.stderr.toString()).toMatch(new RegExp(`expected SCREEN before ${type}`, "i"));
226+
expect(new PacketReader().feed(result.stream).map((packet) => packet.type)).toEqual([
227+
MessageType.GEOMETRY,
228+
]);
229+
});
230+
}
231+
213232
it("fails when the connection closes without a framed EXIT event", async () => {
214233
const result = await runAgainstFakeDaemon((socket) => {
215234
socket.end(Buffer.concat([encodeGeometry(24, 80), encodeScreen("truncated")]));
216235
});
217236

218237
expect(result.status).toBe(1);
219238
expect(result.stdout).toEqual(Buffer.alloc(0));
220-
expect(result.stderr.toString()).toMatch(/machine stream ended before an EXIT event/i);
239+
expect(result.stderr.toString()).toMatch(/machine stream truncated before EXIT: connection closed/i);
221240
expect(new PacketReader().feed(result.stream).map((packet) => packet.type)).toEqual([
222241
MessageType.GEOMETRY,
223242
MessageType.SCREEN,
224243
]);
225244
});
226245

246+
it("diagnoses a transport reset as a truncated stream, not a missing session", async () => {
247+
const result = await runAgainstFakeDaemon((socket) => {
248+
socket.write(Buffer.concat([encodeGeometry(24, 80), encodeScreen("partial")]), () => {
249+
socket.resetAndDestroy();
250+
});
251+
});
252+
253+
expect(result.status).toBe(1);
254+
expect(result.stdout).toEqual(Buffer.alloc(0));
255+
expect(result.stderr.toString()).toMatch(/machine stream truncated before EXIT/i);
256+
expect(result.stderr.toString()).not.toMatch(/session .* not found/i);
257+
});
258+
227259
it("fails instead of hanging when the inherited stream breaks", async () => {
228260
const { server, port } = await listen();
229261
let streamReader: { destroy(): void } | undefined;
@@ -324,4 +356,61 @@ describe("pty attach --attach-stream-fd-v1", () => {
324356
]);
325357
expect(decodeSize(packets[3].payload)).toEqual({ rows: 21, cols: 71 });
326358
});
359+
360+
it("requires a fresh SCREEN after GEOMETRY on reconnect", async () => {
361+
const { server, port } = await listen();
362+
let connection = 0;
363+
server.on("connection", (socket) => {
364+
const current = ++connection;
365+
socket.once("data", () => {
366+
if (current === 1) {
367+
socket.end(Buffer.concat([
368+
encodeGeometry(20, 70),
369+
encodeScreen("first"),
370+
encodePacket(MessageType.DATA, Buffer.from("before reconnect")),
371+
]));
372+
} else {
373+
socket.write(Buffer.concat([
374+
encodeGeometry(21, 71),
375+
encodePacket(MessageType.DATA, Buffer.from("too early")),
376+
]));
377+
}
378+
});
379+
});
380+
const script = `
381+
import net from "node:net";
382+
import { attach } from ${JSON.stringify(clientUrl)};
383+
const dial = () => new Promise((resolve, reject) => {
384+
const socket = net.createConnection({ host: "127.0.0.1", port: ${port} });
385+
socket.once("connect", () => resolve(socket));
386+
socket.once("error", reject);
387+
});
388+
const socket = await dial();
389+
attach({
390+
name: "fixture",
391+
socket,
392+
attachStreamFdV1: 3,
393+
reconnect: dial,
394+
onExit: (code) => process.exit(code),
395+
});
396+
`;
397+
const child = spawn(nodeBin, ["--input-type=module", "-e", script], {
398+
stdio: ["pipe", "pipe", "pipe", "pipe"],
399+
});
400+
const stdout = collect(child.stdout);
401+
const stderr = collect(child.stderr);
402+
const stream = collect(child.stdio[3] as NodeJS.ReadableStream);
403+
const status = await new Promise<number | null>((resolve) => child.once("exit", resolve));
404+
server.close();
405+
406+
expect(status).toBe(1);
407+
expect(await stdout).toEqual(Buffer.alloc(0));
408+
expect((await stderr).toString()).toMatch(/expected SCREEN before DATA/i);
409+
expect(new PacketReader().feed(await stream).map((packet) => packet.type)).toEqual([
410+
MessageType.GEOMETRY,
411+
MessageType.SCREEN,
412+
MessageType.DATA,
413+
MessageType.GEOMETRY,
414+
]);
415+
});
327416
});

tests/completions.test.ts

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,32 @@ describe("pty completions <shell>", () => {
8080
}
8181
});
8282

83+
it("models --attach-stream-fd-v1 as consuming a required free-form value", () => {
84+
expect(gen("fish")).toContain("-l attach-stream-fd-v1 -x ");
85+
expect(gen("bash")).toContain('"${prev}" == "--attach-stream-fd-v1"');
86+
expect(gen("zsh")).toMatch(/--attach-stream-fd-v1\[[^\]]+\]:fd:/);
87+
88+
const bash = which("bash");
89+
if (!bash) return;
90+
const root = fs.mkdtempSync(path.join(os.tmpdir(), "pty-completion-fd-"));
91+
try {
92+
fs.writeFileSync(path.join(root, "target.json"), "{}");
93+
const script = `${gen("bash")}
94+
COMP_WORDS=(pty attach --attach-stream-fd-v1 3 "")
95+
COMP_CWORD=4
96+
_pty
97+
printf '%s\n' "\${COMPREPLY[@]}"`;
98+
const result = spawnSync(bash, ["-c", script], {
99+
encoding: "utf8",
100+
env: { ...process.env, PTY_ROOT: root },
101+
});
102+
expect(result.status, result.stderr).toBe(0);
103+
expect(result.stdout.trim()).toBe("target");
104+
} finally {
105+
fs.rmSync(root, { recursive: true, force: true });
106+
}
107+
});
108+
83109
it("prints fish, bash, zsh to stdout", () => {
84110
for (const shell of ["fish", "bash", "zsh"] as const) {
85111
const out = gen(shell);

0 commit comments

Comments
 (0)