mirror of
https://github.com/mvschwarz/openrig.git
synced 2026-11-08 17:31:56 -10:00
fix(terminal): let writable tiles follow their visible size (#2614)
tileAttachArgv chose ignore-size for a writable tile whenever another normal client was attached to the session, and under window-size latest such a tile could never size the pane, so the pane kept an older client's dimensions. Writable initial, fallback and replacement attaches, and the writable recovery command, now attach without ignore-size; read-only attaches keep -r and are byte-identical to before. This covers writable rig, pod and seat views, saved writable members through composeView, and interactive task viewers, locally, over inherited SSH and through the remote helper. The tmux control client's no-output,ignore-size and the CLI's attach-flag recognizer are unchanged. A real tmux test with a competing older client fails on the previous code (14x18 where 101x32 is expected) and passes now (101x32, then 121x43 on resize), with a larger read-only observer keeping its size. Limits: different simultaneous sizes on one shared pane follow tmux's latest-client rule; clients attached before this change keep their flags until re-attached.
This commit is contained in:
1 parent
bf5e8d8c21
commit
d539277a5f
11 files changed
+154
-40
No files matched your search
@@ -204,7 +204,7 @@ export class HerdrViewer {
|
||||
// shared attach wrapper may be its parent; no mutable title/agent label.
|
||||
const argv = entry.argv;
|
||||
const server = argv.slice(6); // tileAttachArgv: sh,-c,script,name,session,tmux,...server
|
||||
// A writable tile drops ignore-size when its session has no other sizing client (tile-attach).
|
||||
// Recognize legacy ignore-size writable clients as well as current sizing attaches.
|
||||
const flags = entry.readOnly ? [["-r"]] : [["-f", "ignore-size"], []];
|
||||
const targetName = argv[4]!;
|
||||
const verified = info?.foreground_processes?.some(p => {
|
||||
|
||||
@@ -5,32 +5,24 @@ export function tileAttachArgv(
|
||||
tmux: string, server: readonly string[], session: string, readOnly: boolean,
|
||||
retrySeconds = 30,
|
||||
): string[] {
|
||||
// tmux honours ignore-size server-wide: while any client of any session sizes
|
||||
// its window, ignore-size clients size nothing, so a session they alone view
|
||||
// keeps its 80x24 creation size. A writable tile therefore ignores size only
|
||||
// when its session already has a sizing client.
|
||||
const ro = readOnly ? "-r " : "$ro ";
|
||||
const pick = (target: string) => (readOnly ? "" : `pick ${target} "$@"; `);
|
||||
// Writable tiles contribute their visible size even when an older client is attached.
|
||||
// Read-only tiles keep tmux's observer sizing behaviour.
|
||||
const ro = readOnly ? "-r " : "";
|
||||
// No apostrophes or control characters in the script: Herdr also stores this
|
||||
// argv as its restart command. All external values stay positional arguments.
|
||||
const script = [
|
||||
'target=$1; shift;',
|
||||
...(readOnly ? [] : [
|
||||
// Shell builtins only: the provider PATH may hold nothing but tmux.
|
||||
'pick() { s=$1; shift; ro=; for f in $("$@" list-clients -t "$s" -F "#{client_flags}" 2>/dev/null); do',
|
||||
'case ",$f," in *,ignore-size,*) ;; *) ro="-f ignore-size";; esac; done; };',
|
||||
]),
|
||||
`snapshot=$("$@" display-message -p -t "=$target:" "#{pid} #{session_id}:#{session_name}" 2>/dev/null) || { ${pick('"=$target"')}exec "$@" attach ${ro}-t "=$target"; };`,
|
||||
`snapshot=$("$@" display-message -p -t "=$target:" "#{pid} #{session_id}:#{session_name}" 2>/dev/null) || { exec "$@" attach ${ro}-t "=$target"; };`,
|
||||
// display-message may succeed with empty/fallback context for a missing target.
|
||||
`if [ "\${snapshot#*:}" != "$target" ]; then ${pick('"=$target"')}exec "$@" attach ${ro}-t "=$target"; fi;`,
|
||||
`if [ "\${snapshot#*:}" != "$target" ]; then exec "$@" attach ${ro}-t "=$target"; fi;`,
|
||||
'snapshot=${snapshot%%:*};',
|
||||
'node=$("$@" show-environment -t "${snapshot#* }" OPENRIG_NODE_ID 2>/dev/null) || node=;',
|
||||
'case "$node" in OPENRIG_NODE_ID=?*) ;; *) node=;; esac;',
|
||||
'if [ -z "$node" ]; then',
|
||||
'printf "%s\\n" "No managed identity for $target; reopen this view after replacement.";',
|
||||
`${pick('"${snapshot#* }"')}exec "$@" attach ${ro}-t "\${snapshot#* }"; fi;`,
|
||||
`exec "$@" attach ${ro}-t "\${snapshot#* }"; fi;`,
|
||||
'while :; do',
|
||||
`${pick('"${snapshot#* }"')}"$@" attach ${ro}-t "\${snapshot#* }";`,
|
||||
`"$@" attach ${ro}-t "\${snapshot#* }";`,
|
||||
`left=${retrySeconds}; same=0;`,
|
||||
'while [ "$left" -gt 0 ]; do',
|
||||
'next=$("$@" display-message -p -t "=$target:" "#{pid} #{session_id}:#{session_name}" 2>/dev/null) || next=;',
|
||||
|
||||
@@ -121,7 +121,7 @@ export function composeView(
|
||||
const localTmux = `${ctx.localTmux ? shellQuote(ctx.localTmux) : "tmux"}${localServer ? ` ${localServer}` : ""}`;
|
||||
|
||||
for (const m of members) {
|
||||
const attachFlag = m.readOnly ? "-r " : "-f ignore-size ";
|
||||
const attachFlag = m.readOnly ? "-r " : "";
|
||||
|
||||
if (m.host !== null) {
|
||||
// Remote member: classify by the registry entry's transport.
|
||||
|
||||
@@ -0,0 +1,106 @@
|
||||
# A private tmux server and real PTYs only: no OpenRig daemon, agents or desktop.
|
||||
import fcntl
|
||||
import json
|
||||
import os
|
||||
import pty
|
||||
import shutil
|
||||
import struct
|
||||
import subprocess
|
||||
import sys
|
||||
import tempfile
|
||||
import termios
|
||||
import time
|
||||
|
||||
writable, readonly = json.loads(sys.argv[1]), json.loads(sys.argv[2])
|
||||
tmux = shutil.which("tmux")
|
||||
# Honor TMPDIR: CI permits socket connections only inside its isolated IPC root.
|
||||
with tempfile.TemporaryDirectory(prefix="tile-size-") as temporary:
|
||||
root = os.path.realpath(temporary)
|
||||
socket = root + "/s"
|
||||
env = {"HOME": root, "PATH": os.environ["PATH"], "TERM": "xterm-256color", "LC_ALL": "C"}
|
||||
clients = []
|
||||
|
||||
def call(*args):
|
||||
return subprocess.check_output([tmux, "-S", socket, *args], env=env,
|
||||
stderr=subprocess.PIPE, text=True, timeout=3).strip()
|
||||
|
||||
def drain_output():
|
||||
for process, master, slave, tty in clients:
|
||||
try:
|
||||
while os.read(master, 65536):
|
||||
pass
|
||||
except (BlockingIOError, OSError):
|
||||
pass
|
||||
|
||||
def attach(argv, columns, rows):
|
||||
master, slave = pty.openpty()
|
||||
fcntl.ioctl(slave, termios.TIOCSWINSZ, struct.pack("HHHH", rows, columns, 0, 0))
|
||||
def terminal():
|
||||
os.setsid()
|
||||
fcntl.ioctl(slave, termios.TIOCSCTTY, 0)
|
||||
process = subprocess.Popen([socket if arg == "TILE_SOCKET" else arg for arg in argv],
|
||||
stdin=slave, stdout=slave, stderr=slave, env=env,
|
||||
preexec_fn=terminal, close_fds=True)
|
||||
os.set_blocking(master, False)
|
||||
client = (process, master, slave, os.ttyname(slave))
|
||||
clients.append(client)
|
||||
return client
|
||||
|
||||
def geometry():
|
||||
return call("display-message", "-p", "-t", "seat:", "#{pane_width}x#{pane_height}")
|
||||
|
||||
def flags(tty):
|
||||
for line in call("list-clients", "-F", "#{client_tty}|#{client_flags}").splitlines():
|
||||
name, value = line.split("|", 1)
|
||||
if name == tty:
|
||||
return value
|
||||
return None
|
||||
|
||||
def wait_for(predicate, expected):
|
||||
deadline = time.monotonic() + 4
|
||||
while time.monotonic() < deadline:
|
||||
drain_output()
|
||||
if predicate():
|
||||
return
|
||||
time.sleep(0.04)
|
||||
raise AssertionError(expected + "; actual pane=" + geometry() + "; clients=" +
|
||||
call("list-clients", "-F", "#{client_width}x#{client_height}:#{client_flags}"))
|
||||
|
||||
try:
|
||||
call("-f", "/dev/null", "new-session", "-d", "-x", "80", "-y", "24", "-s", "seat", "/bin/sleep 60")
|
||||
call("set-option", "-g", "status", "off")
|
||||
call("set-option", "-g", "window-size", "latest")
|
||||
call("set-environment", "-t", "seat", "OPENRIG_NODE_ID", "tile-fixture")
|
||||
attach([tmux, "-S", socket, "attach", "-t", "=seat"], 14, 18)
|
||||
wait_for(lambda: geometry() == "14x18", "old sizing client must establish 14x18")
|
||||
viewer = attach(writable, 101, 32)
|
||||
wait_for(lambda: flags(viewer[3]) is not None, "writable tile must attach")
|
||||
wait_for(lambda: geometry() == "101x32", "writable tile must size the pane to 101x32")
|
||||
writable_flags = flags(viewer[3])
|
||||
assert "ignore-size" not in writable_flags and "read-only" not in writable_flags
|
||||
fcntl.ioctl(viewer[2], termios.TIOCSWINSZ, struct.pack("HHHH", 43, 121, 0, 0))
|
||||
wait_for(lambda: geometry() == "121x43", "real PTY resize must reach the pane")
|
||||
observer = attach(readonly, 111, 44)
|
||||
wait_for(lambda: flags(observer[3]) is not None, "read-only observer must attach")
|
||||
observer_flags = flags(observer[3])
|
||||
assert "read-only" in observer_flags and "ignore-size" in observer_flags
|
||||
assert geometry() == "121x43", "read-only observer must not change geometry"
|
||||
result = {"initial": "101x32", "resized": geometry(),
|
||||
"writableFlags": writable_flags, "observerFlags": observer_flags}
|
||||
finally:
|
||||
try:
|
||||
call("kill-server")
|
||||
except subprocess.CalledProcessError:
|
||||
pass
|
||||
for process, master, slave, tty in clients:
|
||||
os.close(master)
|
||||
os.close(slave)
|
||||
for process, master, slave, tty in clients:
|
||||
try:
|
||||
process.wait(timeout=5)
|
||||
except subprocess.TimeoutExpired:
|
||||
process.terminate()
|
||||
process.wait(timeout=3)
|
||||
assert all(process.poll() is not None for process, *_ in clients)
|
||||
result["clientsReaped"] = len(clients)
|
||||
print(json.dumps(result))
|
||||
@@ -170,14 +170,14 @@ describe("owned Herdr viewer", () => {
|
||||
expect(await new HerdrViewer(socket, async () => { throw new Error("should not run"); }, "darwin", f.root).ensure(key, f.target)).toMatchObject({ ok: false, code: "viewer_unsupported" });
|
||||
expect(await new HerdrViewer(socket, async () => JSON.stringify({ running: true, version: "0.9.5", protocol: 22, socket: "/other.sock" }), "linux", f.root).ensure(key, f.target)).toMatchObject({ ok: false });
|
||||
});
|
||||
it("decides ignore-size per writable initial/fallback/replacement attach, keeps it in recovery, and preserves read-only", () => {
|
||||
it("sizes writable initial/fallback/replacement attaches and recovery, and preserves read-only", () => {
|
||||
const rw = tileAttachArgv("tmux", [], "a@team", false)[2]!;
|
||||
expect(rw.match(/pick "[^"]+" "\$@"; (exec )?"\$@" attach \$ro -t/g)).toHaveLength(4);
|
||||
expect(rw).toContain('ro="-f ignore-size"');
|
||||
expect(rw.match(/attach -t/g)).toHaveLength(4);
|
||||
expect(rw).not.toContain("ignore-size");
|
||||
const ro = tileAttachArgv("tmux", [], "a@team", true)[2]!;
|
||||
expect(ro.match(/attach -r -t/g)).toHaveLength(4);
|
||||
expect(ro).not.toContain("pick");
|
||||
const f = fixture(); expect(f.target.recoveryCommand).toContain("attach -f ignore-size -t");
|
||||
const f = fixture(); expect(f.target.recoveryCommand).toContain("attach -t");
|
||||
});
|
||||
it("verifies a writable viewer that sized its session (no ignore-size), but not a read-only spelling", async () => {
|
||||
const f = fixture(); f.setAttachFlags([]);
|
||||
@@ -195,7 +195,7 @@ describe("owned Herdr viewer", () => {
|
||||
const provider = { name: "herdr", status: async () => ({ provider: "herdr", available: true, capabilities: {} }), liveness: async () => ({ alive: true }),
|
||||
openView: async () => { throw new Error("Whole-plan open must not run"); },
|
||||
ensureViewer: async (_key: unknown, pane: typeof f.target, stillCurrent?: () => Promise<boolean>) => {
|
||||
calls++; expect(pane.readOnly).toBe(false); expect(pane.attachArgv![2]).toContain("attach $ro -t");
|
||||
calls++; expect(pane.readOnly).toBe(false); expect(pane.attachArgv![2]).toContain("attach -t");
|
||||
expect(await stillCurrent!()).toBe(true); return { ok: true as const, viewer: { workspaceId: "w", paneId: "p", terminalId: "t", readOnly: false }, created: true };
|
||||
} };
|
||||
const svc = new TerminalService({ viewsStore: { get: () => null, list: () => [] }, listRigSeats: () => null, listPodSeats: () => null,
|
||||
|
||||
@@ -267,7 +267,7 @@ describe("S08 correction — the starting tab is kept unless it is known blank",
|
||||
// No pane is confirmed, so the service preserves its existing route to the live seat sessions.
|
||||
expect(res.notes).toEqual([
|
||||
"In a new terminal on the daemon's host, attach directly using one of these commands:",
|
||||
...rows.map(row => `${row.logicalId}: env -u TMUX tmux attach -f ignore-size -t '${row.tmuxSession}'`),
|
||||
...rows.map(row => `${row.logicalId}: env -u TMUX tmux attach -t '${row.tmuxSession}'`),
|
||||
"Each labelled command above opens a different seat. When sharing how to join this team, include every command in full, unchanged, including env -u TMUX, paths and quoting.",
|
||||
TEAM_HANDOFF_NOTE,
|
||||
HERDR_PANES_UNCONFIRMED_NOTE,
|
||||
|
||||
@@ -39,7 +39,7 @@ it.each(["inherit", "isolated"] as const)("keeps remote session names intact thr
|
||||
});
|
||||
expect(readFileSync(argsFile, "utf8").trimEnd().split("\n")).toEqual(mode === "isolated"
|
||||
? ["local", "terminal", "attach", ...(readOnly ? ["--read-only"] : []), "--", session]
|
||||
: ["attach", ...(readOnly ? ["-r"] : ["-f", "ignore-size"]), "-t", session]);
|
||||
: ["attach", ...(readOnly ? ["-r"] : []), "-t", session]);
|
||||
}
|
||||
}
|
||||
} finally {
|
||||
|
||||
@@ -164,8 +164,8 @@ describe("TerminalService — view resolution + one-shape result", () => {
|
||||
expect(notes).toContain("Can you see the team?");
|
||||
expect(notes).toContain("does not confirm");
|
||||
if (unavailable) {
|
||||
expect(result.notes).toContain("dev.driver: env -u TMUX '/opt/tools/tmux' attach -f ignore-size -t 'dev-driver@acme-build'");
|
||||
expect(result.notes).toContain("rev.r1: env -u TMUX '/opt/tools/tmux' attach -f ignore-size -t 'rev-r1@acme-build'");
|
||||
expect(result.notes).toContain("dev.driver: env -u TMUX '/opt/tools/tmux' attach -t 'dev-driver@acme-build'");
|
||||
expect(result.notes).toContain("rev.r1: env -u TMUX '/opt/tools/tmux' attach -t 'rev-r1@acme-build'");
|
||||
expect(notes).toContain("include every command in full, unchanged");
|
||||
} else {
|
||||
expect(notes).not.toContain("attach -t");
|
||||
@@ -467,7 +467,7 @@ describe("default saved kernel conversations", () => {
|
||||
expect(result.opened).toEqual(unavailable ? [] : ["operator-bound", "tui-bound", "advisor-bound"]);
|
||||
expect(result.notes?.join("\n")).toContain("Default kernel view:");
|
||||
expect(result.notes?.join("\n")).not.toMatch(/lead pane|Can you see the team/);
|
||||
if (unavailable) expect(result.notes).toContain("operator: env -u TMUX tmux attach -f ignore-size -t 'operator-bound'");
|
||||
if (unavailable) expect(result.notes).toContain("operator: env -u TMUX tmux attach -t 'operator-bound'");
|
||||
});
|
||||
|
||||
it.each(["herdr", "cmux"])("offers existing conversation attachments when %s is unavailable", async providerName => {
|
||||
@@ -482,8 +482,8 @@ describe("default saved kernel conversations", () => {
|
||||
const result = await new TerminalService(deps).openView({ view: "saved:kernel", provider: providerName });
|
||||
expect(result).toMatchObject({ ok: false, opened: [], code: `${providerName}_unavailable` });
|
||||
expect(result.absent.map(member => member.seat)).toContain("advisor-bound");
|
||||
expect(result.notes).toContain("operator: env -u TMUX tmux attach -f ignore-size -t 'operator-bound'");
|
||||
expect(result.notes).toContain("dashboard: env -u TMUX tmux attach -f ignore-size -t 'actual-tui'");
|
||||
expect(result.notes).toContain("operator: env -u TMUX tmux attach -t 'operator-bound'");
|
||||
expect(result.notes).toContain("dashboard: env -u TMUX tmux attach -t 'actual-tui'");
|
||||
expect(result.notes).toContain("Original provider detail");
|
||||
expect(result.notes!.join("\n")).not.toContain("attach -t 'advisor-bound'");
|
||||
expect(result.notes!.join("\n")).not.toContain("queue-bound");
|
||||
|
||||
@@ -0,0 +1,16 @@
|
||||
import { execFileSync, spawnSync } from "node:child_process";
|
||||
import { fileURLToPath } from "node:url";
|
||||
import { expect, it } from "vitest";
|
||||
import { tileAttachArgv } from "../src/domain/terminal/tile-attach.js";
|
||||
|
||||
const available = spawnSync("tmux", ["-V"]).status === 0 && spawnSync("python3", ["--version"]).status === 0;
|
||||
it.skipIf(!available)("a writable tile sizes and resizes with an older competing client, while a read-only observer does not", () => {
|
||||
const result = JSON.parse(execFileSync("python3", [
|
||||
fileURLToPath(new URL("./helpers/tmux-tile-size.py", import.meta.url)),
|
||||
JSON.stringify(tileAttachArgv("tmux", ["-S", "TILE_SOCKET"], "seat", false, 1)),
|
||||
JSON.stringify(tileAttachArgv("tmux", ["-S", "TILE_SOCKET"], "seat", true, 1)),
|
||||
], { encoding: "utf8", timeout: 15000 }));
|
||||
expect(result).toMatchObject({ initial: "101x32", resized: "121x43", clientsReaped: 3 });
|
||||
expect(result.writableFlags).not.toContain("ignore-size");
|
||||
expect(result.observerFlags).toContain("read-only");
|
||||
}, 20000);
|
||||
@@ -34,10 +34,10 @@ describe("#707 local tiles run the tmux the daemon resolves", () => {
|
||||
expect(spawnSync("/bin/sh", ["-c", bare.opened[0]!.paneCommand], { env }).status).toBe(127);
|
||||
|
||||
const resolved = composeView("v", [local("seat'a", readOnly)], { resolveHost: () => null, localTmux: tmux });
|
||||
// A session nobody else sizes takes the tile's size; one that already has a sizing client keeps it.
|
||||
for (const [clients, writable] of [["", []], ["attached,ignore-size,read-only attached,focused", ["-f", "ignore-size"]], ["attached,ignore-size", []]] as const) {
|
||||
// A writable tile sizes its session even when an older sizing client is still attached.
|
||||
for (const clients of ["", "attached,ignore-size,read-only attached,focused", "attached,ignore-size"]) {
|
||||
execFileSync("/bin/sh", ["-c", resolved.opened[0]!.paneCommand], { env: { ...env, OPR_CLIENTS: clients } });
|
||||
expect(readFileSync(argsFile, "utf8").trimEnd().split("\n")).toEqual(["attach", ...(readOnly ? ["-r"] : writable), "-t", "session-id"]);
|
||||
expect(readFileSync(argsFile, "utf8").trimEnd().split("\n")).toEqual(["attach", ...(readOnly ? ["-r"] : []), "-t", "session-id"]);
|
||||
}
|
||||
expect(resolved.opened[0]!.readOnly).toBe(readOnly);
|
||||
}
|
||||
@@ -94,11 +94,11 @@ function service(resolveLocalTmux?: TerminalServiceDeps["resolveLocalTmux"]): {
|
||||
|
||||
describe("#707 the service passes only an absolute resolved tmux to the composer", () => {
|
||||
const cases: Array<[string, TerminalServiceDeps["resolveLocalTmux"] | undefined, string]> = [
|
||||
["an absolute path", () => "/usr/local/bin/tmux", "'/usr/local/bin/tmux' attach -f ignore-size -t 'a@r'"],
|
||||
["a bare name", () => "tmux", "tmux attach -f ignore-size -t 'a@r'"],
|
||||
["nothing found", async () => null, "tmux attach -f ignore-size -t 'a@r'"],
|
||||
["a failed probe", () => { throw new Error("probe failed"); }, "tmux attach -f ignore-size -t 'a@r'"],
|
||||
["no resolver", undefined, "tmux attach -f ignore-size -t 'a@r'"],
|
||||
["an absolute path", () => "/usr/local/bin/tmux", "'/usr/local/bin/tmux' attach -t 'a@r'"],
|
||||
["a bare name", () => "tmux", "tmux attach -t 'a@r'"],
|
||||
["nothing found", async () => null, "tmux attach -t 'a@r'"],
|
||||
["a failed probe", () => { throw new Error("probe failed"); }, "tmux attach -t 'a@r'"],
|
||||
["no resolver", undefined, "tmux attach -t 'a@r'"],
|
||||
];
|
||||
it.each(cases)("%s", async (_name, resolver, expected) => {
|
||||
const { svc, herdr } = service(resolver);
|
||||
|
||||
@@ -61,12 +61,12 @@ describe("the daemon's tmux server", () => {
|
||||
const resolveHost = () => ({ id: "box", transport: "ssh" as const, target: "box.example" });
|
||||
const plain = composeView("v", [local, remote], { resolveHost });
|
||||
expect(plain.opened.map((p) => p.recoveryCommand)).toEqual([
|
||||
"tmux attach -f ignore-size -t 'dev-review@starter'",
|
||||
"ssh -t 'box.example' 'tmux attach -f ignore-size -t '\"'\"'dev-build@starter'\"'\"''",
|
||||
"tmux attach -t 'dev-review@starter'",
|
||||
"ssh -t 'box.example' 'tmux attach -t '\"'\"'dev-build@starter'\"'\"''",
|
||||
]);
|
||||
const onServer = composeView("v", [local, remote], { resolveHost, localTmux: "/usr/bin/tmux", localTmuxServer: ["-L", "person"] });
|
||||
expect(onServer.opened.map((p) => p.recoveryCommand)).toEqual([
|
||||
"'/usr/bin/tmux' -L 'person' attach -f ignore-size -t 'dev-review@starter'",
|
||||
"'/usr/bin/tmux' -L 'person' attach -t 'dev-review@starter'",
|
||||
plain.opened[1]!.recoveryCommand,
|
||||
]);
|
||||
});
|
||||
|
||||
Reference in new issue
Block a user