Skip to content

fix(desktop): v1 terminal — emit legacy marker + gate reattach on streamReady #3389

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Kitenite merged 5 commits into from
Apr 12, 2026
Merged

fix(desktop): v1 terminal — emit legacy marker + gate reattach on streamReady #3389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b578346
Revert "fix(desktop): accept legacy OSC 777 marker in v1 terminal (#3…
Kitenite Apr 12, 2026
e95208b
fix(desktop): emit legacy OSC 777 marker alongside 133;A in shell wra…
Kitenite Apr 12, 2026
384ec72
fix(desktop): v1 terminal reattach gate must check streamReady, not c…
Kitenite Apr 12, 2026
2d28085
test(desktop): session resolves readiness on combined 777+133;A wrapp…
Kitenite Apr 12, 2026
c15eb8e
Lint
Kitenite Apr 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions apps/desktop/src/main/lib/agent-setup/shell-wrappers.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -839,6 +839,9 @@ export SUPERSET_WORKSPACE_PATH="/wrong/path"
expect(args[0]).toBe("-l");
expect(args[1]).toBe("--init-command");
expect(args[2]).toContain(`set -l _superset_bin "${TEST_BIN_DIR}"`);
// Both markers are emitted so old v1 daemons (777 scanner) and new
// scanners (133;A) both detect readiness without a daemon restart.
expect(args[2]).toContain("\\033]777;superset-shell-ready\\007");
expect(args[2]).toContain("\\033]133;A\\007");
});

Expand All @@ -854,7 +857,21 @@ export SUPERSET_WORKSPACE_PATH="/wrong/path"
expect(args[2]).toContain(
'set -l _superset_bin "/tmp/with space/quote\\"buck\\$slash\\\\bin"',
);
expect(args[2]).toContain("777;superset-shell-ready");
expect(args[2]).toContain("133;A");
});

it("zsh/bash wrappers emit both legacy 777 and current 133;A markers", () => {
createZshWrapper(TEST_PATHS);
createBashWrapper(TEST_PATHS);

const zlogin = readFileSync(path.join(TEST_ZSH_DIR, ".zlogin"), "utf-8");
const rcfile = readFileSync(path.join(TEST_BASH_DIR, "rcfile"), "utf-8");

for (const wrapper of [zlogin, rcfile]) {
expect(wrapper).toContain("\\033]777;superset-shell-ready\\007");
expect(wrapper).toContain("\\033]133;A\\007");
}
});
});
});
18 changes: 12 additions & 6 deletions apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,10 +218,15 @@ ${SUPERSET_ENV_RESTORE}
${buildZshPrecmdHook(paths.BIN_DIR)}
${buildPathPrependFunction(paths.BIN_DIR)}
rehash 2>/dev/null || true
# OSC 133;A prompt marker (FinalTerm standard) — signals shell readiness.
# Shell readiness markers. Emitting both keeps us compatible across daemon
# versions: the legacy v1 daemon scans for OSC 777, the current scanner (v1
# post-refactor + v2 host-service) scans for OSC 133;A (FinalTerm standard).
# Wrappers are rewritten on every app launch, so main always ships the
# superset of markers; daemons that only get restarted on protocol bumps
# still match against their own scanner.
# Protocol ref: https://gitlab.freedesktop.org/Per_Bothner/specifications/blob/master/proposals/semantic-prompts.md
__superset_prompt_mark() {
printf "\\033]133;A\\007"
printf "\\033]777;superset-shell-ready\\007\\033]133;A\\007"
}
# Keep our hook LAST so it fires after direnv and other precmd hooks complete.
precmd_functions=(\${precmd_functions[@]} __superset_prompt_mark)
Expand Down Expand Up @@ -268,10 +273,10 @@ ${buildPathPrependFunction(paths.BIN_DIR)}
hash -r 2>/dev/null || true
# Minimal prompt (path/env shown in toolbar) - emerald to match app theme
export PS1=$'\\[\\e[1;38;2;52;211;153m\\]❯\\[\\e[0m\\] '
# OSC 133;A prompt marker (FinalTerm standard) — signals shell readiness.
# Shell readiness markers — see zsh wrapper for rationale on emitting both.
# Protocol ref: https://gitlab.freedesktop.org/Per_Bothner/specifications/blob/master/proposals/semantic-prompts.md
__superset_prompt_mark() {
printf "\\033]133;A\\007"
printf "\\033]777;superset-shell-ready\\007\\033]133;A\\007"
}
# Hook via PROMPT_COMMAND. Supports both scalar and array forms (Bash 5.1+).
if [[ "$(declare -p PROMPT_COMMAND 2>/dev/null)" == "declare -a"* ]]; then
Expand Down Expand Up @@ -315,7 +320,8 @@ export function getShellArgs(
if (shellName === "fish") {
// Use --init-command to prepend BIN_DIR to PATH after config is loaded.
// Use fish list-aware checks to avoid duplicate PATH entries across nested shells.
// OSC 133;A emitted on fish_prompt — signals shell readiness.
// Emit both OSC 777 (legacy v1 daemon) and OSC 133;A (current scanner)
// on fish_prompt. See zsh wrapper for rationale.
const escapedBinDir = escapeFishDoubleQuoted(paths.BIN_DIR);
return [
"-l",
Expand All @@ -325,7 +331,7 @@ export function getShellArgs(
`contains -- "$_superset_bin" $PATH`,
`or set -gx PATH "$_superset_bin" $PATH`,
`function _superset_prompt_mark --on-event fish_prompt`,
`printf '\\033]133;A\\007'`,
`printf '\\033]777;superset-shell-ready\\007\\033]133;A\\007'`,
`end`,
].join("; "),
];
Expand Down
42 changes: 10 additions & 32 deletions apps/desktop/src/main/terminal-host/session-shell-ready.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@ import {

/** OSC 133;A marker emitted by shell wrappers (FinalTerm standard). */
const SHELL_READY_MARKER = "\x1b]133;A\x07";
/** Legacy OSC 777 marker emitted by pre-#3348 wrappers / stale daemons. */
const LEGACY_READY_MARKER = "\x1b]777;superset-shell-ready\x07";
import "./xterm-env-polyfill";

const { Session } = await import("./session");
Expand Down Expand Up @@ -270,46 +268,26 @@ describe("Session shell-ready: marker detection", () => {
sendData(proc, SHELL_READY_MARKER);
expect(getWrittenData(proc)).toEqual(["buffered\n"]);
});
});

describe("Session shell-ready: legacy OSC 777 marker", () => {
it("unblocks writes when only the legacy marker arrives", () => {
// Wrappers now emit both the legacy OSC 777 and the current OSC 133;A in
// a single printf so either daemon version can detect readiness without a
// restart. The scanner only matches 133;A — 777 passes through to the
// emulator, which drops unknown OSC sequences silently. This test guards
// against a future wrapper regression that swaps the order (which would
// leave 133;A in the pre-777 slice and still work) or drops 133;A
// entirely (which would regress readiness on the current scanner).
it("resolves readiness when wrapper emits both 777 and 133;A markers together", () => {
const { session, proc } = createTestSession("/bin/zsh");
spawnAndReady(session, proc);

session.write("buffered\n");
expect(getWrittenData(proc)).toEqual([]);

// Stale daemon / old wrapper emits the pre-#3348 OSC 777 marker.
sendData(proc, `direnv output...${LEGACY_READY_MARKER}prompt$ `);
const COMBINED_MARKER = "\x1b]777;superset-shell-ready\x07\x1b]133;A\x07";
sendData(proc, `direnv output...${COMBINED_MARKER}prompt$ `);

expect(getWrittenData(proc)).toEqual(["buffered\n"]);
});

it("detects legacy marker split across two PTY data frames", () => {
const { session, proc } = createTestSession("/bin/bash");
spawnAndReady(session, proc);

const half = Math.floor(LEGACY_READY_MARKER.length / 2);
sendData(proc, `output${LEGACY_READY_MARKER.slice(0, half)}`);

session.write("buffered\n");
expect(getWrittenData(proc)).toEqual([]);

sendData(proc, `${LEGACY_READY_MARKER.slice(half)}prompt`);
expect(getWrittenData(proc)).toEqual(["buffered\n"]);
});

it("strips legacy marker from emitted output", () => {
const { session, proc } = createTestSession("/bin/zsh");
spawnAndReady(session, proc);

sendData(proc, `before${LEGACY_READY_MARKER}after`);

// Shell is ready — write passes through.
session.write("ok\n");
expect(getWrittenData(proc)).toEqual(["ok\n"]);
});
});

Comment on lines 270 to 292
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot Apr 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Missing session-level test for the dual-marker scenario

The three removed tests covered the old daemon-side scanner; the replacement wrapper-level tests (in shell-wrappers.test.ts) verify that both markers are emitted, but there is no session-level test that verifies the new combined path end-to-end.

Concretely, when the session receives \x1b]777;superset-shell-ready\x07\x1b]133;A\x07:

  1. scanForShellReady strips \x1b]133;A\x07 and sets shellReadyState = "ready".
  2. \x1b]777;superset-shell-ready\x07 is not stripped — it is enqueued via enqueueEmulatorWrite and broadcast to clients. The PR's "silently dropped by the emulator" claim is correct for xterm.js, but nothing in the test suite currently verifies this pass-through is benign.

Consider adding a test analogous to the removed "strips legacy marker from emitted output" case, but for the new combined sequence:

it("resolves readiness when both markers arrive together, passing 777 through", () => {
    const { session, proc } = createTestSession("/bin/zsh");
    spawnAndReady(session, proc);

    session.write("buffered\n");
    expect(getWrittenData(proc)).toEqual([]);

    // New wrappers emit both markers; 133;A triggers readiness
    const BOTH = "\x1b]777;superset-shell-ready\x07\x1b]133;A\x07";
    sendData(proc, `direnv output...${BOTH}prompt$ `);

    expect(getWrittenData(proc)).toEqual(["buffered\n"]);
});

This also catches any future regression where the order of the two markers is accidentally swapped (777 would no longer be in output before 133;A is consumed).

describe("Session shell-ready: kill/exit before readiness", () => {
Expand Down
81 changes: 7 additions & 74 deletions apps/desktop/src/main/terminal-host/session.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,55 +42,6 @@ import {
PtySubprocessIpcType,
} from "./pty-subprocess-ipc";

// =============================================================================
// Legacy OSC 777 shell-ready scanner (pre-#3348 wrappers)
// =============================================================================

/**
* Marker emitted by the previous generation of shell wrappers
* (see commit 2d1885a3f). Fixed literal — no optional params.
*/
const LEGACY_OSC_777 = "\x1b]777;superset-shell-ready\x07";

interface LegacyShellReadyScanState {
matchPos: number;
heldBytes: string;
}

function createLegacyScanState(): LegacyShellReadyScanState {
return { matchPos: 0, heldBytes: "" };
}

function scanForLegacyShellReady(
state: LegacyShellReadyScanState,
data: string,
): { output: string; matched: boolean } {
let output = "";
for (let i = 0; i < data.length; i++) {
const ch = data[i] as string;
if (ch === LEGACY_OSC_777[state.matchPos]) {
state.heldBytes += ch;
state.matchPos++;
if (state.matchPos === LEGACY_OSC_777.length) {
state.heldBytes = "";
state.matchPos = 0;
return { output: output + data.slice(i + 1), matched: true };
}
} else {
output += state.heldBytes;
state.heldBytes = "";
state.matchPos = 0;
if (ch === LEGACY_OSC_777[0]) {
state.heldBytes = ch;
state.matchPos = 1;
} else {
output += ch;
}
}
}
return { output, matched: false };
}

// =============================================================================
// Constants
// =============================================================================
Expand Down Expand Up @@ -125,12 +76,10 @@ const EMULATOR_WRITE_QUEUE_LOW_WATERMARK_BYTES = 250_000;

/**
* How long to wait for the shell-ready marker before unblocking writes.
* On timeout, buffered writes flush immediately (same behavior as before
* this feature). Kept short so a broken/missing marker doesn't leave the
* terminal feeling frozen — slower shell startups (direnv, nix) can still
* race us, but the user can always re-type.
* 15s covers heavy setups like Nix-based devenv via direnv. On timeout,
* buffered writes flush immediately (same behavior as before this feature).
*/
const SHELL_READY_TIMEOUT_MS = 5_000;
const SHELL_READY_TIMEOUT_MS = 15_000;

/**
* Shell readiness lifecycle:
Expand Down Expand Up @@ -217,11 +166,6 @@ export class Session {
private preReadyStdinQueue: string[] = [];
// OSC 133;A scanner state — shared with v2 host-service via @superset/shared
private scanState: ShellReadyScanState = createScanState();
// Legacy OSC 777 scanner state. Matches the pre-#3348 marker emitted by
// older shell wrappers still present on users' disks, or by terminals
// spawned through a stale daemon that bundles the old scanner code.
// Without this fallback, input stays buffered until the 15s timeout.
private legacyScanState: LegacyShellReadyScanState = createLegacyScanState();

private emulatorWriteQueue: string[] = [];
private emulatorWriteQueuedBytes = 0;
Expand Down Expand Up @@ -418,17 +362,10 @@ export class Session {
let data = payload.toString("utf8");

// Scan for OSC 133;A (shell ready) and strip from output.
// Also scan for the legacy OSC 777 marker emitted by older
// shell wrappers / stale daemons — see legacyScanState.
if (this.shellReadyState === "pending") {
const result = scanForShellReady(this.scanState, data);
data = result.output;
const legacyResult = scanForLegacyShellReady(
this.legacyScanState,
data,
);
data = legacyResult.output;
if (result.matched || legacyResult.matched) {
if (result.matched) {
this.resolveShellReady("ready");
Comment thread
Kitenite marked this conversation as resolved.
}
}
Expand Down Expand Up @@ -1057,7 +994,6 @@ export class Session {
}
this.preReadyStdinQueue = [];
this.scanState = createScanState();
this.legacyScanState = createLegacyScanState();
this.subprocessStdinQueue = [];
this.subprocessStdinQueuedBytes = 0;
this.subprocessStdinDrainArmed = false;
Expand Down Expand Up @@ -1101,18 +1037,15 @@ export class Session {
this.shellReadyTimeoutId = null;
}
// Flush held marker bytes — they weren't part of a full marker
const heldBytes = this.scanState.heldBytes + this.legacyScanState.heldBytes;
if (heldBytes.length > 0) {
this.enqueueEmulatorWrite(heldBytes);
if (this.scanState.heldBytes.length > 0) {
this.enqueueEmulatorWrite(this.scanState.heldBytes);
this.broadcastEvent("data", {
type: "data",
data: heldBytes,
data: this.scanState.heldBytes,
} satisfies TerminalDataEvent);
this.scanState.heldBytes = "";
this.legacyScanState.heldBytes = "";
}
this.scanState.matchPos = 0;
this.legacyScanState.matchPos = 0;
// Flush queued writes in FIFO order
const queue = this.preReadyStdinQueue;
this.preReadyStdinQueue = [];
Expand Down
9 changes: 8 additions & 1 deletion ...n/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,14 @@ export function useTerminalLifecycle({

// Use the v1 terminal cache: reuse existing xterm instance across tab
// switches instead of creating/disposing each time (v2 "hide attach" pattern).
const isReattach = v1TerminalCache.has(paneId);
// Only treat as reattach when the prior mount actually completed attach —
// a cache entry can exist with streamReady=false if the previous mount
// unmounted before createOrAttach finished (e.g. bulk tab creation where
// React remounts a pane mid-attach). Taking the reattach fast path in
// that state leaves the pane permanently disconnected with no daemon
// session and no stream subscription.
const cachedBeforeCreate = v1TerminalCache.get(paneId);
const isReattach = cachedBeforeCreate?.streamReady === true;
if (DEBUG_TERMINAL) {
console.log(`[Terminal] isReattach=${isReattach} paneId=${paneId}`);
}
Expand Down
Loading